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

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

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

Thanks.