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

From: Stephen & Linda Smith <ischis2@cox.net>
Date: Aug 31, 2018, 18:13 UTC
Message-ID: <1863304.0psXOz24iy@thunderbird>
In-Reply-To: <87a7p3c83g.fsf@evledraar.gmail.com>

On Friday, August 31, 2018 9:54:50 AM MST Junio C Hamano wrote:

Show 9 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> >> 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.
>

I didn't figure it was, but I haven't yet figured out how to explain what what I saw last evening. I wanted to send something out to get feedback from someone who may know the code far better than I.

Show 19 quoted lines

> 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

thatwt_status_collect(), isYes

> expeted from the code gets violated if we change the code in
> show_merge_in_progress() function.

Agreed. I'm just green at this code base, and so don't quite know what I should see as opposed to what I do see.

Show 17 quoted lines

> 
> 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;
>

I placed the following in wt_status_collect() last evening, and received errors from three early tests in 7501-commit.sh. Thanks for a hint.

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

Maybe the missing first condition was what I needed.

Which leads me to asking: Do you want a preparatory patch moving has_unmerged() further up in the file before adding a reference to has_unmerged() in wt_status_collect().

> 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.

Would you explain what you are thinking for passing moving the "stat" think into wt_status. I haven't figured out how the "collect" sequence, relates to the "print/show" squence.

> 
> Thanks for working on this.

I decided sometime back to work on something I didn't know using a process I don't normally use to broaden my experience. I'm enjoying this and hope you don't mind lots of questions from someone new.

sps