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

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

[Ovidiu Gheorghioiu]

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

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

[Stephen & Linda Smith]

Ovidiu Gheorghioiu <ovidiug@gmail.com> wrote:
>  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 ..

I've reproduced the bug with git 2.7.1. [1]
I plan on adding a test case and a patch to fix this.

[1] http://article.gmane.org/gmane.comp.version-control.git/276591

I would have responded to the original email but 
the gmane "follow-up" is not sending out the email.

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

[Stephen & Linda Smith]

On Monday, February 08, 2016 06:55:17 PM Stephen & Linda Smith wrote:
> > #!/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"

With the  '--dry-run' switch, dry_run_commit() is called which returns 1 
since run_status() is returning the wt_status commitable field which has a value of 0.

That field is only set in one place (wt_status_print_updated) which isn't getting called
directly or indirectly by run_status.   I checked this by code inspection as well as by 
instrumenting the code.

I'm not sure that we want to add a call to wt_status_print_updated in run_status since 
I don't believe we want the print statements.   An alternative might be to create a
new function.

> > 
> > git commit -m "Something to commit after all" && echo "Commit went through"
> > 
> > git log --pretty=oneline

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

[Stephen P. Smith]

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

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

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

[Philip Oakley]

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 &&
> + 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
>
> --
Philip 

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

[Junio C Hamano]

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

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

[Stephen & Linda Smith]

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.

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

[Stephen & Linda Smith]

On Tuesday, February 16, 2016 08:20:43 AM Philip Oakley wrote:
> 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?
OK

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

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

[Stephen & Linda Smith]

On Tuesday, February 16, 2016 04:26:38 PM Stephen & Linda Smith wrote:
> 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
> 
I am not adverse to moving the change to dry_run_commit() proper, 
but that would mean a slightly larger patch.

> 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

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

[Junio C Hamano]

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

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

[Stephen & Linda Smith]

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

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

[Stephen P. Smith]

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

Notes:
    Updated the commit message.

 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

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

[Stephen & Linda Smith]

On Tuesday, February 16, 2016 07:33:54 PM Junio C Hamano wrote:
> > ---
> 
> 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.
> 
I understand.

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

> 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.
Tonight I started work on a patch to remove the two locations where 
committable was set in  the *print* and *show* functions.  

I believe that what you mean by the "collect" phase is the set of functions 
that are in wt_status.c and have collect in the function name.

> 
> 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.
> 
I'm happy with moveing the patch from pu (where it is now) to next.   I've
re-started my work on this.

> Thanks.

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

[Stephen Smith]

On Tuesday, February 16, 2016 8:33:54 PM MST Junio C Hamano wrote:
> 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.
> 
<snip>
> 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 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.   

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

[Junio C Hamano]

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

[Junio C Hamano]

Stephen Smith <ischis2@cox.net> writes:

> On Tuesday, February 16, 2016 8:33:54 PM MST Junio C Hamano wrote:

Wow, that's quite an old discussion ;-)

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

After these:

    $ git reset --hard HEAD
    $ >a-new-file && git add a-new-file

running 

    $ git commit --dry-run; echo $?
    $ git commit --dry-run --short; echo $?

tells me that "--short" still does not notice that there _is_
something to be committed, either with an ancient version like
v2.10.5 or more modern versions of Git.  The "long" version exits
with 0, while "--short" one exists with 1.

So...?

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

[Stephen & Linda Smith]

> > On Tuesday, February 16, 2016 8:33:54 PM MST Junio C Hamano wrote:
> Wow, that's quite an old discussion ;-)
It wasn't intended to be so

> After these:
<snip>
> tells me that "--short" still does not notice that there _is_
> something to be committed, either with an ancient version like
> v2.10.5 or more modern versions of Git.  The "long" version exits
> with 0, while "--short" one exists with 1.

$ git commit --dry-run --short; echo $?
A  a-new-file
0

$ git commit --dry-run --short; echo $?
A  a-new-file
1

> 
> So...?

For me it is the return code that is faulty  .... Starting to investigate.   
Thanks