From owner-svn-ports-head@freebsd.org Mon Jan 30 14:02:55 2017 Return-Path: Delivered-To: svn-ports-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 67757CC812B; Mon, 30 Jan 2017 14:02:55 +0000 (UTC) (envelope-from freebsd.contact@marino.st) Received: from shepard.synsport.com (mail.synsport.com [208.69.230.148]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 191E11349; Mon, 30 Jan 2017 14:02:54 +0000 (UTC) (envelope-from freebsd.contact@marino.st) Received: from [127.0.0.1] (ip72-204-83-236.fv.ks.cox.net [72.204.83.236]) (using TLSv1 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by shepard.synsport.com (Postfix) with ESMTP id A743843BF0; Mon, 30 Jan 2017 08:01:15 -0600 (CST) Subject: Re: svn commit: r432561 - head/lang/php71 To: Torsten Zuehlsdorff , ports-committers@freebsd.org, svn-ports-all@freebsd.org, svn-ports-head@freebsd.org References: <201701271852.v0RIqOvW033749@repo.freebsd.org> <6aebcd6a-0439-f23a-be9d-e06f46d3511b@toco-domains.de> <4dd6d654-4525-c440-83b1-6a22215f6020@toco-domains.de> <5725262c-4152-c711-e53b-a509742bcba1@marino.st> <23d56a5a-f7a0-5d03-cbef-c08bdafa417b@toco-domains.de> Cc: Mathieu Arnold Reply-To: marino@freebsd.org From: John Marino Message-ID: <5eac2fff-ed34-7e54-574e-1134b290a736@marino.st> Date: Mon, 30 Jan 2017 08:02:51 -0600 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <23d56a5a-f7a0-5d03-cbef-c08bdafa417b@toco-domains.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Antivirus: avast! (VPS 170130-0, 01/30/2017), Outbound message X-Antivirus-Status: Clean X-BeenThere: svn-ports-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the ports tree for head List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 Jan 2017 14:02:55 -0000 On 1/30/2017 07:50, Torsten Zuehlsdorff wrote: > On 30.01.2017 14:28, John Marino wrote: >> >> EXACTLY. >> Your mistake is leaving this at two months ago. >> I meant check differences the day of committing, not check differences 2 >> months ago. If you had not assumed that zero changes happened to php70 >> that would affect php71 in a two month period, then you would rechecked >> the diff before you actually commit and seen them. I would have noticed >> the changes because I always run "svn diff" before committing. > > I do a svn diff before every commit. Sadly i did not see this mistake > within this some hundred lines. nor checked commit log for php71 (or freshports). svn diff wouldn't have helped here because the files were added, not repocopied. > >>>> You misunderstood r432567. >>>> That wasn't to correct my commit. >>>> That was to add the second missing item, DTRACE for aarch. >>>> >>>>> I just noted in this 2 month how hard it is to find somebody for >>>>> review >>>>> and second how hard it is to get such a patch reviewed. First the >>>>> review.freebsd.org didn't work for me because it contains a bug with >>>>> umlaut for years. And second a repocopy could not be displayed. >>>> >>>> Yes, the diff would have been displayed. >>>> That's the benefit of repocopy. >>> >>> Maybe i wasn't clear enough: the repo-copy could not displayed in >>> review.freebsd.org. If you say: its possible. Okay, than its just the >>> umlaut-bug preventing the display of my patch. >> >> I am not talking about review.freebsd.org. >> I'm talking about svn. > > Read my statement again. I explicitly talk about reviews.freebsd.org and > also about its incapability of displaying a repo-copy. And this is basically a separate and irrelevant topic. After a two month period, the review could have been updated to capture commits to the original port since the original review as well. > >>> But - no, you are not right this time. Doing a repo-copy without >>> changing the copy will cause a svn diff to just list the files. The diff >>> didn't even contain the revision copied from. To bring a path like this >>> into the review tool you need at least the --show-copies-as-adds param, >>> which defeats the purpose of a repo-copy in review. >> >> I don't think anybody was talking about review. > > I was - i explicitly wrote it. > >> If you do repocopy correctly, we all get a commit mail that shows the >> differences between the new version and the version it was copied from. >> In this case, we got what looked like complete files added which is how >> mat know that svncopy wasn't used. > > I know. You missed my point: making an error with repo-copy is hard to > catch - even with the help of others. reviews.freebsd.org is not able to > display a repo-copy. "svn diff" doesn't even show the revision of the > copy used. So you need to do it timely (with is hard with such a big > patch) or you need to track everything in the meantime to decide if its > needed to do all the work again. And again. And again... I don't think it's that hard. Freshports shows commits in a quick fashion. So yes, I'm saying if a review is old, new changes should be tracked. I think that's expected. I do appreciate that it was a very big patch set and this is all easier said than done. I was focusing on the main port rather than the extensions. water under the bridge. basically its a learning opportunity now. - review time - svnrepo - tracking original - diff Bottom line: You did a good jobs with php71 and there are some minor things you could have done better. Those things weren't unavoidable IMO. John --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus