From: Eric Chamberland <eric.chamberland@giref.ulaval.ca>
Date: Fri, 23 Mar 2012 19:02:04 GMT
Subject: Re: [PATCH] Make 'cvs -n commit ...' not to commit
Message-ID: <4F6CC8AC.4050907@giref.ulaval.ca>
In-Reply-To: <7vhaxftb54.fsf@alter.siamese.dyndns.org>

On 03/23/2012 02:39 PM, Junio C Hamano wrote:
> ericc<eric.chamberland@giref.ulaval.ca>  writes:
>
>> Actually, doing a 'cvs -n commit' will _do_ the commit...
>> With this patch, it now goes through the code, but don't do the commit.
>
> OK.
>
>> A further progress would be to do the pre-commit hook is possible...

Sorry, I wanted to write:

"A further progress would be to do the pre-commit hook *if* possible..."

here, we are used to do "cvs -n commit" just to check if the "hooks" on 
the cvs server will fail or not...


>
> I understand that you tried to make the patch smaller by avoiding
> re-indenting, but this is *yucky*.
>
> It looks to me that the above part could be solved with:
>
> 	unless (...) {
> 		next;
> 	}
>
> I think the function being patched is too big.  Wouldn't it be better to
> have a refactoring patch to move the above per-path logic to a helper
> function that deals with a single path, and then insert the "omit call to
> that helper when run with -n" code in a separate patch?
>
> The same comment applies to the other hunk.
>
> Also I notice that the indentation used throughout the file is somewhat
> broken (e.g. "Emulate by running hooks/update" part is indented to 8
> columns, but earlier parts use 4 space indent).  The right structure for
> this change may be:
>
>   Patch 1: Fix indentation (and do nothing else) to uniformly indent with
>            HT;
>
>   Patch 2: Refactor this big funciton using a handful of helper functions
> 	  (and do nothing else);
>
>   Patch 3: Omit calls to these helper functions under -n option.
>
>

Ok you are right...  These were my very first lines in Perl... I just 
wanted to catch the attention of someone who is able to do the changes 
correctly... and in a more clean way than I...

Eric