Re: [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase.

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Fri, Aug 31 2018, Stephen P. Smith wrote:
>
>> In an update to fix a bug with "commit --dry-run" it was found that
>> the commitable flag was broken.  The update was, at the time,
>> accepted as it was better than the previous version.
>
> What update is this? I.e. git.git commit id? See the "or this invocation
> of `git show`" part of SubmittingPatches for how to quote it in the
> commit message.
>
>> Since the set of the flag had been done in wt_longstatus_print_updated,
>> set the flag in wt_status_collect_updated_cb.
>>
>> Set the commitable flag in wt_status_collect_changes_initial to keep
>> from introducing a rebase regression.
>>
>> Leave the setting of the commitable flag in show_merge_in_progress. If
>> a check for merged commits is moved to the collect phase then other
>> --dry-run tests fail.

"Some tests fail" is not a good explanation why you keep the setting
of commitable bit in the "show" codepath.  The test coverage may not
be thorough, and the tests that fail might be expecting wrong things.

The change in this patch makes the internal "diff-index" invocation
responsible for setting the commitable bit.  This is better for non
merge commits than the current code because the current code only
sets the commitable bit when longstatus is asked for (and the code
to show the longstatus detects changes to be committed), so the
short-form will not have chance to set the bit, but the internal
"diff-index" is what determines if the resulting commit would have
difference relative to the HEAD, so it is a better place to make
that decision.

Merge commits need to be allowed even when the resulting commit
records a tree that is identical to that of the current HEAD
(i.e. we merged a side branch, but we already had all the necessary
changes done on it).  So it is insufficient to let "diff-index"
invocation to set the committable bit.  Is that why "other --dry-run
tests fail"?  What I am getting at is to have a reasonable "because
..."  to explain why "other --dry-run tests fail" after it, to make
it clear to the readers that the failure is not because tests are
checking wrong things but because a specific condition that is
expeted from the code gets violated if we change the code in
show_merge_in_progress() function.

That brings us to another point.  Is there a case where we want to
see s->commitable bit set correctly but we do not make any call to
show_merge_in_progress() function?  It is conceivable to have a new
"git commit --dry-run --quiet [[--] <pathspec>]" mode that is
totally silent but reports if what we have is committable with the
exit status, and for that we would not want to call any show_*
functions.  That leads me to suspect that ideally we would want to
see wt_status_collect_changes_index() to be the one that is setting
the commitable bit.  Or even its caller wt_status_collect(), which
would give us a better chance of being correct even during the
initial commit.  For the "during merge" case, we would need to say
something like

	if (state->merge_in_progress && !has_unmerged(s))
		s->commitable = 1;

but the "state" thing is passed around only among the "print/show"
level of functions in the current code.  We might need to merge that
into the wt_status structure to pass it down to the "collect" phase
at the lower level before/while doing so.  I dunno.

Thanks for working on this.