[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase.
From:       Stephen & Linda Smith <ischis2 () cox ! net>
Date:       2018-08-31 18:13:24
Message-ID: 1863304.0psXOz24iy () thunderbird
[Download RAW message or body]

On Friday, August 31, 2018 9:54:50 AM MST Junio C Hamano wrote:
> Æ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.

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

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



[prev in list] [next in list] [prev in thread] [next in thread]