"Stephen P. Smith" <[email protected]> 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 <[email protected]>
> ---
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
