From: Peter Valdemar Mørch (Lists) <4ux6as402@sneakemail.com>
Date: Sun, 10 Aug 2008 17:05:40 GMT
Subject: Re: [PATCH] Teach git log --check to return an appropriate error code
Message-ID: <489F1FE4.6090400@sneakemail.com>
In-Reply-To: <7v8wv66l8d.fsf@gitster.siamese.dyndns.org>

Junio C Hamano gitster-at-pobox.com |Lists| wrote:
> Future versions of log_tree_diff() may want to tweak opt->diffopt per
> commit, when we have options for "use larger -U<lines> value after hitting
> this commit", or "use this pathspec to limit the diff output after hitting
> this commit", for example.  But even in these cases, I think it is
> implausible to start from a freshly initialized diff_options structure.
> The code most likely would start from the copy of what was in use and
> update only the necessary fields, without disturbing the state variables.
> 
> So I think you are worried a bit too much in this case, even though it is
> a valid concern in principle.  It might warrant a comment somewhere inside
> log_tree_diff() to tell people not to re-initialize opt->diffopt per
> commit without thinking, though.

Hmm... I've looked at the code... The while loop that iterates through 
the revisions is in cmd_log_walk(), which calls log_tree_commit(), which 
in turn calls log_tree_diff().

I'm thinking that cmd_log_walk() is where one "would want" to change 
rev->diffopt / opt->diffopt in the future, and hence I suggest to put 
the comment there - given my limited understanding of connecting tissue. 
Something like:

/* For --check, the exit code is based on CHECK_FAILED
    being accumulated in rev->diffopt, so be careful to retain
    that state information if replacing rev->diffopt in this
    loop */

That would also be 10-15 lines above the patch I posted earlier, so the 
connection with retrieving the error code would be visible 15 lines below.

Would such a comment in that place constiture and acceptable patch? I've 
tried to follow Dscho's write up and contribute a patch, even though 
git-log's exit code was never my itch to begin with, because I'm exited 
to contribute.

> One interesting option that might be interesting to add to the log family
> would be to show only commits that fail the checkdiff tests.  I suspect
> necessary change for doing so would go to log_tree_diff() codepath.

I'm hoping that this is meant as "aside from this current patch, one 
interesting option..." or do you mean "in order for this patch to be 
accepted, I suggest this to be added ..." ?

This is growing. I originally suggested a patch to documentation to make 
it match the code, but took on Dscho's invitation to contribute a code 
patch instead. But given that this patch, although working, still isn't 
good enough and the new proposals : the new option above and --exit-code 
proposal elsewhere in this thread, I'm getting a little discouraged. I'm 
not saying you meant it that way.

Peter
-- 
Peter Valdemar Mørch
http://www.morch.com