Re: Bug report: 'git commit --dry-run' corner case: returns error ("nothing to commit") when all conflicts resolved to HEAD

>  Hi git guys,
>
> The bug is fairly simple: if we have a conflicted merge, AND all the
> conflicts have been resolved to the version in HEAD, the commit
> --dry-run error code says nothing to commit. As expected, git commit
> goes through.
> 
> The commit message IS correct (-ish), just not the error code:
> 
> """
> All conflicts fixed but you are still merging.
>   (use "git commit" to conclude merge)
> 
> nothing to commit, working directory clean
> """
> 
> The script below demonstrates the problem; version tested was 2.5.0.
> Let me know if you need any more details.
> 
> Best,
> Ovidiu
> 
> ------
> #!/bin/bash
> mkdir test-repository || exit 1
> cd test-repository
> git init
> echo "Initial contents, unimportant" > test-file
> git add test-file
> git commit -m "Initial commit"
> echo "commit-1-state" > test-file
> git commit -m "commit 1" -i test-file
> git tag commit-1
> git checkout -b branch-2 HEAD^1
> echo "commit-2-state" > test-file
> git commit -m "commit 2" -i test-file
> 
> # Creates conflicted state.
> git merge --no-commit commit-1
> 
> # Resolved entirely to commit-2, aka HEAD.
> echo "commit-2-state" > test-file
> # If we'd set to commit-1=state, all would work as expected (changes vs HEAD).
> git add test-file
> 
> # =====  Bug is here.
> git commit --dry-run && echo "Git said something to commit" \
>         || echo "Git said NOTHING to commit"
> 
> git commit -m "Something to commit after all" && echo "Commit went through"
> 
> git log --pretty=oneline
> 
> cd ..

> 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>
> ---
>
> Notes:
>    In the original report when the dry run switch was passed and after
>    the merge commit was resolved head and index matched leading to a
>    returned value of 1. [1]
>
>    If the dry run switch was not passed, the commit would succeed to
>    correctly record the resolution.
>
>    The result was that a dry run would report that there would be a
>    failure, but there really isn't a failure if the commit is actually
>    attemped.
>
>    [1] $gmane/276591
>
>    It appeared that the conditional for 'Reject an attempt to record a
>    non-merge empty commit without * explicit --allow-empty.' could be
>    simplified after adding this patch.
>
>    This change can't be propagated to the conditional because it allows
>    a commit that was previously disallowed.
>
> t/t7501-commit.sh | 20 ++++++++++++++++++++
> wt-status.c       |  1 +
> 2 files changed, 21 insertions(+)
>
> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> index 63e0427..363abb1 100755
> --- a/t/t7501-commit.sh
> +++ b/t/t7501-commit.sh
> @@ -587,4 +587,24 @@ test_expect_success '--only works on to-be-born 
> branch' '
>  test_cmp expected actual
> '
>
> +test_expect_success '--dry-run with conflicts fixed from a merge' '
> + # setup two branches with conflicting information
> + # in the same file, resolve the conflict,
> + # call commit with --dry-run
> + echo "Initial contents, unimportant" >test-file &&
> + git add test-file &&
> + git commit -m "Initial commit" &&
> + echo "commit-1-state" >test-file &&
> + git commit -m "commit 1" -i test-file &&
> + git tag commit-1 &&
> + git checkout -b branch-2 HEAD^1 &&
> + echo "commit-2-state" >test-file &&
> + git commit -m "commit 2" -i test-file &&
> + ! $(git merge --no-commit commit-1) &&
> + echo "commit-2-state" >test-file &&
> + git add test-file &&
> + git commit --dry-run &&
> + git commit -m "conflicts fixed from merge."
> +'
> +
> test_done
> diff --git a/wt-status.c b/wt-status.c
> index ab4f80d..1374b48 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -950,6 +950,7 @@ static void show_merge_in_progress(struct wt_status 
> *s,
>  status_printf_ln(s, color,
>  _("  (fix conflicts and run \"git commit\")"));
>  } else {
> + s-> commitable = 1;
>  status_printf_ln(s, color,
>  _("All conflicts fixed but you are still merging."));
>  if (s->hints)
> -- 
> 2.7.0.GIT
>
> --

>>    It appeared that the conditional for 'Reject an attempt to record a
>>    non-merge empty commit without * explicit --allow-empty.' could be
>>    simplified after adding this patch.
>>
>>    This change can't be propagated to the conditional because it allows
>>    a commit that was previously disallowed.

> "Philip Oakley" <philipoakley@iee.org> writes:
> 
> >>    It appeared that the conditional for 'Reject an attempt to record a
> >>    non-merge empty commit without * explicit --allow-empty.' could be
> >>    simplified after adding this patch.
> >>
> >>    This change can't be propagated to the conditional because it allows
> >>    a commit that was previously disallowed.
> 
> This last sentence sounds somewhat worrysome.  Does that mean some
> commit that was previously disallowed (which ones?) is still
> forbidden by "commit" without "--dry-run" (which is correct--we are
> not interested in changing the behaviour of the main codepath), but
> "--dry-run", even with this update, will say "OK you will make a
> meaningful commit" by exiting with 0 for such disallowed commit?

> On Tuesday, February 16, 2016 01:54:48 PM Junio C Hamano wrote:
> > "Philip Oakley" <philipoakley@iee.org> writes:
> > 
> > >>    It appeared that the conditional for 'Reject an attempt to record a
> > >>    non-merge empty commit without * explicit --allow-empty.' could be
> > >>    simplified after adding this patch.
> > >>
> > >>    This change can't be propagated to the conditional because it allows
> > >>    a commit that was previously disallowed.
> > 
> > This last sentence sounds somewhat worrysome.  Does that mean some
> > commit that was previously disallowed (which ones?) is still
> > forbidden by "commit" without "--dry-run" (which is correct--we are
> > not interested in changing the behaviour of the main codepath), but
> > "--dry-run", even with this update, will say "OK you will make a
> > meaningful commit" by exiting with 0 for such disallowed commit?
> I tried to think of a better set of wording.  Finally I decided to make it part of the note 
> rather than the commit message so that it could be debated as part of the review 
> but not be part of the commit record for the line being changed.
> 
> The patch doesn't change behaviour other than the dry-run return code 
> which now matches the return code of commit.  The one line change is not changing the
> main code path behaviour
> 

> The main code path for the case being fixed executes through the main code path 
> successfully returning zero.  The ''--dry-run' was predicitng failure if a script was 
> checking the return code, but successs if looking at the messages.
> 
> The final couple of paragraphs explain why I chose not to change the if() statement. 
> The reason I didn't is so that expected behaviour is maintained.
> 
> The condition that can not be removed in the if is the 'whence != FROM_MERGE'.   Removing 
> that caused t7502 to generate errors.   Therefore I left ' if (!commitable && whence != FROM_MERGE 
> && !allow_empty &&  !(amend && is_a_merge(current_head)))' in the commit.c file.
> 
> --
> 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

> From: "Stephen P. Smith" <ischis2@cox.net>
> > The 'commit --dry-run' and commit return values differed if a
> 
> Should this have quotes around the second 'commit' as they both refer to the 
> command, rather than the action?

> 
> > 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>
> > ---
> >
> > Notes:
> >    In the original report when the dry run switch was passed and after
> >    the merge commit was resolved head and index matched leading to a
> >    returned value of 1. [1]
> >
> >    If the dry run switch was not passed, the commit would succeed to
> >    correctly record the resolution.
> >
> >    The result was that a dry run would report that there would be a
> >    failure, but there really isn't a failure if the commit is actually
> >    attemped.
> >
> >    [1] $gmane/276591
> >
> >    It appeared that the conditional for 'Reject an attempt to record a
> >    non-merge empty commit without * explicit --allow-empty.' could be
> >    simplified after adding this patch.
> >
> >    This change can't be propagated to the conditional because it allows
> >    a commit that was previously disallowed.
> >
> > t/t7501-commit.sh | 20 ++++++++++++++++++++
> > wt-status.c       |  1 +
> > 2 files changed, 21 insertions(+)
> >
> > diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> > index 63e0427..363abb1 100755
> > --- a/t/t7501-commit.sh
> > +++ b/t/t7501-commit.sh
> > @@ -587,4 +587,24 @@ test_expect_success '--only works on to-be-born 
> > branch' '
> >  test_cmp expected actual
> > '
> >
> > +test_expect_success '--dry-run with conflicts fixed from a merge' '
> > + # setup two branches with conflicting information
> > + # in the same file, resolve the conflict,
> > + # call commit with --dry-run
> > + echo "Initial contents, unimportant" >test-file &&
> > + git add test-file &&
> > + git commit -m "Initial commit" &&
> > + echo "commit-1-state" >test-file &&ply to the list and the cc's. Junio also had 
> > + git commit -m "commit 1" -i test-file &&
> > + git tag commit-1 &&
> > + git checkout -b branch-2 HEAD^1 &&
> > + echo "commit-2-state" >test-file &&
> > + git commit -m "commit 2" -i test-file &&
> > + ! $(git merge --no-commit commit-1) &&
> > + echo "commit-2-state" >test-file &&
> > + git add test-file &&
> > + git commit --dry-run &&
> > + git commit -m "conflicts fixed from merge."
> > +'
> > +
> > test_done
> > diff --git a/wt-status.c b/wt-status.c
> > index ab4f80d..1374b48 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -950,6 +950,7 @@ static void show_merge_in_progress(struct wt_status 
> > *s,
> >  status_printf_ln(s, color,
> >  _("  (fix conflicts and run \"git commit\")"));
> >  } else {
> > + s-> commitable = 1;
> >  status_printf_ln(s, color,
> >  _("All conflicts fixed but you are still merging."));
> >  if (s->hints)
> >
> > --
> Philip 
> 
> --
> 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

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

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

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

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

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

>> 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 was about to start working on working on this and ran the test you suggested 
> back in 2016.   I don't get the error message from that time period.  I 
> believe that this was fixed.   

> > #!/bin/bash
> > mkdir test-repository || exit 1
> > cd test-repository
> > git init
> > echo "Initial contents, unimportant" > test-file
> > git add test-file
> > git commit -m "Initial commit"
> > echo "commit-1-state" > test-file
> > git commit -m "commit 1" -i test-file
> > git tag commit-1
> > git checkout -b branch-2 HEAD^1
> > echo "commit-2-state" > test-file
> > git commit -m "commit 2" -i test-file
> > 
> > # Creates conflicted state.
> > git merge --no-commit commit-1
> > 
> > # Resolved entirely to commit-2, aka HEAD.
> > echo "commit-2-state" > test-file
> > # If we'd set to commit-1=state, all would work as expected (changes vs HEAD).
> > git add test-file
> > 
> > # =====  Bug is here.
> > git commit --dry-run && echo "Git said something to commit" \
> >         || echo "Git said NOTHING to commit"