Johannes Schindelin [off-list ref] writes:
On Sat, 9 Aug 2008, Peter Valdemar Mørch wrote:
quoted
Whether or not a check fails is stored in the
DIFF_OPT_CHECK_FAILED field of flags in struct diff_options.
This flag-field is only set (diff.c:1644), never cleared.
That is a side effect. How wise is it to rely on that?
Hmm, good point.
The bit will never be cleared during a single diff run by design, because
it needs to be cumulative in order to check a patch that describes changes
to multiple paths --- iow, the API sequence is (1) the caller to the diff
machinery resets the bit to zero and then (2) the caller exercises the
diff machinery and expects the machinery to set the bit if even a single
failure is detected, or leaves it unset if there is none.
So unless you (log_tree_diff(), the caller of diff machinery), decide to
explicitly reset the bit (or decide to use a freshly allocated and
initialized diff_options for each commit it feeds diff_tree_sha1()), the
assumption would hold. We need to see how plausible it would be for us to
break that assumption in the future.
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.
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.
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