Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.

[Stephen & Linda Smith <ischis2@cox.net>]

On Tuesday, February 16, 2016 07:33:54 PM Junio C Hamano wrote:
> "Stephen P. Smith" <ischis2@cox.net> writes:
> 
> > The 'commit --dry-run' and commit return values differed if a
> > conflicted merge had been resolved and the commit would be the same as
> > the parent.
> >
> > Update show_merge_in_progress to set the commitable bit if conflicts
> > have been resolved and a merge is in progress.
> >
> > Signed-off-by: Stephen P. Smith <ischis2@cox.net>
> > ---
> 
> I think I mislead you into a slightly wrong direction.  While the
> single liner does improve the situation, I think this is merely a
> band-aid upon closer inspection.  For example, if you changed your
> "commit --dry-run" in your test to "commit --dry-run --short", you
> would notice that the test would fail.
> 
> In fact, "commit --dry-run" is already broken without this "a merge
> ends up in a no-op" corner case.  The management of s->commitable
> flag and dry_run_commit() that uses it are unfortunately more broken
> than I originally thought.
> 
> If we check for places where s->committable is set, we notice that
> there is only one location: wt_status_print_updated().  This function
> runs an equivalent of "diff-index --cached" and flips s->committable
> on when it sees any difference.
> 
> This function is only called from wt_status_print(), which in turn
> is only called from run_status() in commit.c when the status format
> is unspecified or set to STATUS_FORMAT_LONG.
> 
> So if you do this:
> 
>     $ git reset --hard HEAD
>     $ >a-new-file && git add a-new-file
>     $ git commit --dry-run --short; echo $?
> 
> you'd get "No, there is nothing interesting to commit", which is
> clearly bogus.
> 
> I said s->committable is flipped on only when there is any change in
> "diff-index --cached".  There is nothing that flips it off, by
> noticing that there are unmerged paths, for example.  This is
> another brokenness around "git commit --dry-run".  Imagine that you
> are in a middle of a conflicted cherry-pick.  You did "git add" on a
> resolved path and you still have another path whose conflict has not
> been resolved.  If you run a "git commit --dry-run", you will hear
> "Yes, you can make a meaningful commit", which again is clearly
> bogus.
> 
> These things need to be eventually fixed, and I think the fix will
> involve revamping how we compute s->committable flag.  Most likely,
> we won't be doing any of that in any wt_status function whose name
> has "print" or "show" in it.  As the original designer of the wt_*
> suite (before these multiple output formats are added), I would say
> everything should happen inside the "collect" phase, if we wanted to
> make s->committable bit usable.
> 
> So in the sense, eventually the code updated by this patch will have
> to be discarded when we fix the "commit --dry-run" in the right way,
> but in the meantime, the patch does not make things worse, so let's
> think about queuing it as-is for now as a stop-gap measure.
> 
Hmmmm....  that makes sense.   

Let me fix the commit message per what I told Philip and then 
I will start working on a rework of the commitable bit with 
this in mind as a follow on patch.  

> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html