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

"Stephen P. Smith" <ischis2@cox.net> writes:

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.