{"thread":{"id":"41374","subject":"Re: Bug report: 'git commit --dry-run' corner case: returns error (\"nothing to commit\") when all conflicts resolved to HEAD","startedAt":"2016-02-09T01:55:17Z","lastAt":"2018-08-23T01:28:19Z","messageCount":16,"participants":["Stephen & Linda Smith","Stephen P. Smith","Philip Oakley","Junio C Hamano","Stephen Smith"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"277792","messageId":"72756249.nAoBccgOj7@thunderbird","threadId":"41374","inReplyTo":"1649296.sC1eN3ni6k@thunderbird","subject":"Re: Bug report: 'git commit --dry-run' corner case: returns error (\"nothing to commit\") when all conflicts resolved to HEAD","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2016-02-09T01:55:17Z","receivedAt":"2016-02-09T01:55:17Z","isPatch":false,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"Ovidiu Gheorghioiu wrote:\n> Hi git guys,\n>\n> The bug is fairly simple: if we have a conflicted merge, AND all the\n> conflicts have been resolved to the version in HEAD, the commit\n> --dry-run error code says nothing to commit. As expected, git commit\n> goes through.\n> \n> The commit message IS correct (-ish), just not the error code:\n> \n> \"\"\"\n> All conflicts fixed but you are still merging.\n> (use \"git commit\" to conclude merge)\n> \n> nothing to commit, working directory clean\n> \"\"\"\n> \n> The script below demonstrates the problem; version tested was 2.5.0.\n> Let me know if you need any more details.\n> \n> Best,\n> Ovidiu\n> \n> ------\n> #!/bin/bash\n> mkdir test-repository || exit 1\n> cd test-repository\n> git init\n> echo \"Initial contents, unimportant\" > test-file\n> git add test-file\n> git commit -m \"Initial commit\"\n> echo \"commit-1-state\" > test-file\n> git commit -m \"commit 1\" -i test-file\n> git tag commit-1\n> git checkout -b branch-2 HEAD^1\n> echo \"commit-2-state\" > test-file\n> git commit -m \"commit 2\" -i test-file\n> \n> # Creates conflicted state.\n> git merge --no-commit commit-1\n> \n> # Resolved entirely to commit-2, aka HEAD.\n> echo \"commit-2-state\" > test-file\n> # If we'd set to commit-1=state, all would work as expected (changes vs HEAD).\n> git add test-file\n> \n> # ===== Bug is here.\n> git commit --dry-run && echo \"Git said something to commit\" \\\n> || echo \"Git said NOTHING to commit\"\n> \n> git commit -m \"Something to commit after all\" && echo \"Commit went through\"\n> \n> git log --pretty=oneline\n> \n> cd ..\n\nI've reproduced the bug with git 2.7.1. [1]\nI plan on adding a test case and a patch to fix this.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/276591\n\nI would have responded to the original email but \nthe gmane \"follow-up\" is not sending out the email.\n"},{"id":"277983","messageId":"2471685.OJU1dNe0yn@thunderbird","threadId":"41374","inReplyTo":"1649296.sC1eN3ni6k@thunderbird","subject":"Re: Bug report: 'git commit --dry-run' corner case: returns error (\"nothing to commit\") when all conflicts resolved to HEAD","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2016-02-12T01:04:03Z","receivedAt":"2016-02-12T01:04:03Z","isPatch":false,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"On Monday, February 08, 2016 06:55:17 PM Stephen & Linda Smith wrote:\n> > #!/bin/bash\n> > mkdir test-repository || exit 1\n> > cd test-repository\n> > git init\n> > echo \"Initial contents, unimportant\" > test-file\n> > git add test-file\n> > git commit -m \"Initial commit\"\n> > echo \"commit-1-state\" > test-file\n> > git commit -m \"commit 1\" -i test-file\n> > git tag commit-1\n> > git checkout -b branch-2 HEAD^1\n> > echo \"commit-2-state\" > test-file\n> > git commit -m \"commit 2\" -i test-file\n> > \n> > # Creates conflicted state.\n> > git merge --no-commit commit-1\n> > \n> > # Resolved entirely to commit-2, aka HEAD.\n> > echo \"commit-2-state\" > test-file\n> > # If we'd set to commit-1=state, all would work as expected (changes vs HEAD).\n> > git add test-file\n> > \n> > # ===== Bug is here.\n> > git commit --dry-run && echo \"Git said something to commit\" \\\n> > || echo \"Git said NOTHING to commit\"\n\nWith the '--dry-run' switch, dry_run_commit() is called which returns 1 \nsince run_status() is returning the wt_status commitable field which has a value of 0.\n\nThat field is only set in one place (wt_status_print_updated) which isn't getting called\ndirectly or indirectly by run_status. I checked this by code inspection as well as by \ninstrumenting the code.\n\nI'm not sure that we want to add a call to wt_status_print_updated in run_status since \nI don't believe we want the print statements. An alternative might be to create a\nnew function.\n\n> > \n> > git commit -m \"Something to commit after all\" && echo \"Commit went through\"\n> > \n> > git log --pretty=oneline\n"},{"id":"278262","messageId":"1455590305-30923-1-git-send-email-ischis2@cox.net","threadId":"41374","inReplyTo":"72756249.nAoBccgOj7@thunderbird","subject":"[PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Stephen P. Smith","fromEmail":"ischis2@cox.net","sentAt":"2016-02-16T02:38:25Z","receivedAt":"2016-02-16T02:38:25Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"The 'commit --dry-run' and commit return values differed if a\nconflicted merge had been resolved and the commit would be the same as\nthe parent.\n\nUpdate show_merge_in_progress to set the commitable bit if conflicts\nhave been resolved and a merge is in progress.\n\nSigned-off-by: Stephen P. Smith \n---\n\nNotes:\n In the original report when the dry run switch was passed and after\n the merge commit was resolved head and index matched leading to a\n returned value of 1. [1]\n \n If the dry run switch was not passed, the commit would succeed to\n correctly record the resolution.\n \n The result was that a dry run would report that there would be a\n failure, but there really isn't a failure if the commit is actually\n attemped.\n \n [1] $gmane/276591\n \n It appeared that the conditional for 'Reject an attempt to record a\n non-merge empty commit without * explicit --allow-empty.' could be\n simplified after adding this patch.\n \n This change can't be propagated to the conditional because it allows\n a commit that was previously disallowed.\n\n t/t7501-commit.sh | 20 ++++++++++++++++++++\n wt-status.c | 1 +\n 2 files changed, 21 insertions(+)\n\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 63e0427..363abb1 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -587,4 +587,24 @@ test_expect_success '--only works on to-be-born branch' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success '--dry-run with conflicts fixed from a merge' '\n+\t# setup two branches with conflicting information\n+\t# in the same file, resolve the conflict,\n+\t# call commit with --dry-run\n+\techo \"Initial contents, unimportant\" >test-file &&\n+\tgit add test-file &&\n+\tgit commit -m \"Initial commit\" &&\n+\techo \"commit-1-state\" >test-file &&\n+\tgit commit -m \"commit 1\" -i test-file &&\n+\tgit tag commit-1 &&\n+\tgit checkout -b branch-2 HEAD^1 &&\n+\techo \"commit-2-state\" >test-file &&\n+\tgit commit -m \"commit 2\" -i test-file &&\n+\t! $(git merge --no-commit commit-1) &&\n+\techo \"commit-2-state\" >test-file &&\n+\tgit add test-file &&\n+\tgit commit --dry-run &&\n+\tgit commit -m \"conflicts fixed from merge.\"\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex ab4f80d..1374b48 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -950,6 +950,7 @@ static void show_merge_in_progress(struct wt_status *s,\n \t\t\tstatus_printf_ln(s, color,\n \t\t\t\t_(\" (fix conflicts and run \\\"git commit\\\")\"));\n \t} else {\n+\t\ts-> commitable = 1;\n \t\tstatus_printf_ln(s, color,\n \t\t\t_(\"All conflicts fixed but you are still merging.\"));\n \t\tif (s->hints)\n-- \n2.7.0.GIT\n"},{"id":"278283","messageId":"C8BDC3289C184F40BFBE3B150CFBB50B@PhilipOakley","threadId":"41374","inReplyTo":"1455590305-30923-1-git-send-email-ischis2@cox.net","subject":"Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2016-02-16T06:33:57Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Stephen P. Smith\" \n> The 'commit --dry-run' and commit return values differed if a\n\nShould this have quotes around the second 'commit' as they both refer to the \ncommand, rather than the action?\n\n> conflicted merge had been resolved and the commit would be the same as\n> the parent.\n>\n> Update show_merge_in_progress to set the commitable bit if conflicts\n> have been resolved and a merge is in progress.\n>\n> Signed-off-by: Stephen P. Smith \n> ---\n>\n> Notes:\n> In the original report when the dry run switch was passed and after\n> the merge commit was resolved head and index matched leading to a\n> returned value of 1. [1]\n>\n> If the dry run switch was not passed, the commit would succeed to\n> correctly record the resolution.\n>\n> The result was that a dry run would report that there would be a\n> failure, but there really isn't a failure if the commit is actually\n> attemped.\n>\n> [1] $gmane/276591\n>\n> It appeared that the conditional for 'Reject an attempt to record a\n> non-merge empty commit without * explicit --allow-empty.' could be\n> simplified after adding this patch.\n>\n> This change can't be propagated to the conditional because it allows\n> a commit that was previously disallowed.\n>\n> t/t7501-commit.sh | 20 ++++++++++++++++++++\n> wt-status.c | 1 +\n> 2 files changed, 21 insertions(+)\n>\n> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\n> index 63e0427..363abb1 100755\n> --- a/t/t7501-commit.sh\n> +++ b/t/t7501-commit.sh\n> @@ -587,4 +587,24 @@ test_expect_success '--only works on to-be-born \n> branch' '\n> test_cmp expected actual\n> '\n>\n> +test_expect_success '--dry-run with conflicts fixed from a merge' '\n> + # setup two branches with conflicting information\n> + # in the same file, resolve the conflict,\n> + # call commit with --dry-run\n> + echo \"Initial contents, unimportant\" >test-file &&\n> + git add test-file &&\n> + git commit -m \"Initial commit\" &&\n> + echo \"commit-1-state\" >test-file &&\n> + git commit -m \"commit 1\" -i test-file &&\n> + git tag commit-1 &&\n> + git checkout -b branch-2 HEAD^1 &&\n> + echo \"commit-2-state\" >test-file &&\n> + git commit -m \"commit 2\" -i test-file &&\n> + ! $(git merge --no-commit commit-1) &&\n> + echo \"commit-2-state\" >test-file &&\n> + git add test-file &&\n> + git commit --dry-run &&\n> + git commit -m \"conflicts fixed from merge.\"\n> +'\n> +\n> test_done\n> diff --git a/wt-status.c b/wt-status.c\n> index ab4f80d..1374b48 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -950,6 +950,7 @@ static void show_merge_in_progress(struct wt_status \n> *s,\n> status_printf_ln(s, color,\n> _(\" (fix conflicts and run \\\"git commit\\\")\"));\n> } else {\n> + s-> commitable = 1;\n> status_printf_ln(s, color,\n> _(\"All conflicts fixed but you are still merging.\"));\n> if (s->hints)\n> -- \n> 2.7.0.GIT\n>\n> --\nPhilip \n"},{"id":"278394","messageId":"xmqqa8n0mic7.fsf@gitster.mtv.corp.google.com","threadId":"41374","inReplyTo":"C8BDC3289C184F40BFBE3B150CFBB50B@PhilipOakley","subject":"Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-16T21:54:48Z","receivedAt":"2016-02-16T21:54:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" writes:\n\n>> It appeared that the conditional for 'Reject an attempt to record a\n>> non-merge empty commit without * explicit --allow-empty.' could be\n>> simplified after adding this patch.\n>>\n>> This change can't be propagated to the conditional because it allows\n>> a commit that was previously disallowed.\n\nThis last sentence sounds somewhat worrysome. Does that mean some\ncommit that was previously disallowed (which ones?) is still\nforbidden by \"commit\" without \"--dry-run\" (which is correct--we are\nnot interested in changing the behaviour of the main codepath), but\n\"--dry-run\", even with this update, will say \"OK you will make a\nmeaningful commit\" by exiting with 0 for such disallowed commit?\n"},{"id":"278410","messageId":"1501768.nZTW69Q7aq@thunderbird","threadId":"41374","inReplyTo":"C8BDC3289C184F40BFBE3B150CFBB50B@PhilipOakley","subject":"Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2016-02-16T23:26:38Z","receivedAt":"2016-02-16T23:26:38Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"On Tuesday, February 16, 2016 01:54:48 PM Junio C Hamano wrote:\n> \"Philip Oakley\" writes:\n> \n> >> It appeared that the conditional for 'Reject an attempt to record a\n> >> non-merge empty commit without * explicit --allow-empty.' could be\n> >> simplified after adding this patch.\n> >>\n> >> This change can't be propagated to the conditional because it allows\n> >> a commit that was previously disallowed.\n> \n> This last sentence sounds somewhat worrysome. Does that mean some\n> commit that was previously disallowed (which ones?) is still\n> forbidden by \"commit\" without \"--dry-run\" (which is correct--we are\n> not interested in changing the behaviour of the main codepath), but\n> \"--dry-run\", even with this update, will say \"OK you will make a\n> meaningful commit\" by exiting with 0 for such disallowed commit?\nI tried to think of a better set of wording. Finally I decided to make it part of the note \nrather than the commit message so that it could be debated as part of the review \nbut not be part of the commit record for the line being changed.\n\nThe patch doesn't change behaviour other than the dry-run return code \nwhich now matches the return code of commit. The one line change is not changing the\nmain code path behaviour\n\nThe main code path for the case being fixed executes through the main code path \nsuccessfully returning zero. The ''--dry-run' was predicitng failure if a script was \nchecking the return code, but successs if looking at the messages.\n\nThe final couple of paragraphs explain why I chose not to change the if() statement. \nThe reason I didn't is so that expected behaviour is maintained.\n\nThe condition that can not be removed in the if is the 'whence != FROM_MERGE'. Removing \nthat caused t7502 to generate errors. Therefore I left ' if (!commitable && whence != FROM_MERGE \n&& !allow_empty && !(amend && is_a_merge(current_head)))' in the commit.c file.\n"},{"id":"278413","messageId":"1619848.U5ErtRW7d8@thunderbird","threadId":"41374","inReplyTo":"1455590305-30923-1-git-send-email-ischis2@cox.net","subject":"Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2016-02-16T23:30:07Z","receivedAt":"2016-02-16T23:30:07Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"On Tuesday, February 16, 2016 08:20:43 AM Philip Oakley wrote:\n> From: \"Stephen P. Smith\" \n> > The 'commit --dry-run' and commit return values differed if a\n> \n> Should this have quotes around the second 'commit' as they both refer to the \n> command, rather than the action?\nOK\n\n> \n> > conflicted merge had been resolved and the commit would be the same as\n> > the parent.\n> >\n> > Update show_merge_in_progress to set the commitable bit if conflicts\n> > have been resolved and a merge is in progress.\n> >\n> > Signed-off-by: Stephen P. Smith \n> > ---\n> >\n> > Notes:\n> > In the original report when the dry run switch was passed and after\n> > the merge commit was resolved head and index matched leading to a\n> > returned value of 1. [1]\n> >\n> > If the dry run switch was not passed, the commit would succeed to\n> > correctly record the resolution.\n> >\n> > The result was that a dry run would report that there would be a\n> > failure, but there really isn't a failure if the commit is actually\n> > attemped.\n> >\n> > [1] $gmane/276591\n> >\n> > It appeared that the conditional for 'Reject an attempt to record a\n> > non-merge empty commit without * explicit --allow-empty.' could be\n> > simplified after adding this patch.\n> >\n> > This change can't be propagated to the conditional because it allows\n> > a commit that was previously disallowed.\n> >\n> > t/t7501-commit.sh | 20 ++++++++++++++++++++\n> > wt-status.c | 1 +\n> > 2 files changed, 21 insertions(+)\n> >\n> > diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\n> > index 63e0427..363abb1 100755\n> > --- a/t/t7501-commit.sh\n> > +++ b/t/t7501-commit.sh\n> > @@ -587,4 +587,24 @@ test_expect_success '--only works on to-be-born \n> > branch' '\n> > test_cmp expected actual\n> > '\n> >\n> > +test_expect_success '--dry-run with conflicts fixed from a merge' '\n> > + # setup two branches with conflicting information\n> > + # in the same file, resolve the conflict,\n> > + # call commit with --dry-run\n> > + echo \"Initial contents, unimportant\" >test-file &&\n> > + git add test-file &&\n> > + git commit -m \"Initial commit\" &&\n> > + echo \"commit-1-state\" >test-file &&ply to the list and the cc's. Junio also had \n> > + git commit -m \"commit 1\" -i test-file &&\n> > + git tag commit-1 &&\n> > + git checkout -b branch-2 HEAD^1 &&\n> > + echo \"commit-2-state\" >test-file &&\n> > + git commit -m \"commit 2\" -i test-file &&\n> > + ! $(git merge --no-commit commit-1) &&\n> > + echo \"commit-2-state\" >test-file &&\n> > + git add test-file &&\n> > + git commit --dry-run &&\n> > + git commit -m \"conflicts fixed from merge.\"\n> > +'\n> > +\n> > test_done\n> > diff --git a/wt-status.c b/wt-status.c\n> > index ab4f80d..1374b48 100644\n> > --- a/wt-status.c\n> > +++ b/wt-status.c\n> > @@ -950,6 +950,7 @@ static void show_merge_in_progress(struct wt_status \n> > *s,\n> > status_printf_ln(s, color,\n> > _(\" (fix conflicts and run \\\"git commit\\\")\"));\n> > } else {\n> > + s-> commitable = 1;\n> > status_printf_ln(s, color,\n> > _(\"All conflicts fixed but you are still merging.\"));\n> > if (s->hints)\n> >\n> > --\n> Philip \n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"278414","messageId":"3639645.80CNmLM0I7@thunderbird","threadId":"41374","inReplyTo":"C8BDC3289C184F40BFBE3B150CFBB50B@PhilipOakley","subject":"Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2016-02-16T23:40:05Z","receivedAt":"2016-02-16T23:40:05Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"On Tuesday, February 16, 2016 04:26:38 PM Stephen & Linda Smith wrote:\n> On Tuesday, February 16, 2016 01:54:48 PM Junio C Hamano wrote:\n> > \"Philip Oakley\" writes:\n> > \n> > >> It appeared that the conditional for 'Reject an attempt to record a\n> > >> non-merge empty commit without * explicit --allow-empty.' could be\n> > >> simplified after adding this patch.\n> > >>\n> > >> This change can't be propagated to the conditional because it allows\n> > >> a commit that was previously disallowed.\n> > \n> > This last sentence sounds somewhat worrysome. Does that mean some\n> > commit that was previously disallowed (which ones?) is still\n> > forbidden by \"commit\" without \"--dry-run\" (which is correct--we are\n> > not interested in changing the behaviour of the main codepath), but\n> > \"--dry-run\", even with this update, will say \"OK you will make a\n> > meaningful commit\" by exiting with 0 for such disallowed commit?\n> I tried to think of a better set of wording. Finally I decided to make it part of the note \n> rather than the commit message so that it could be debated as part of the review \n> but not be part of the commit record for the line being changed.\n> \n> The patch doesn't change behaviour other than the dry-run return code \n> which now matches the return code of commit. The one line change is not changing the\n> main code path behaviour\n> \nI am not adverse to moving the change to dry_run_commit() proper, \nbut that would mean a slightly larger patch.\n\n> The main code path for the case being fixed executes through the main code path \n> successfully returning zero. The ''--dry-run' was predicitng failure if a script was \n> checking the return code, but successs if looking at the messages.\n> \n> The final couple of paragraphs explain why I chose not to change the if() statement. \n> The reason I didn't is so that expected behaviour is maintained.\n> \n> The condition that can not be removed in the if is the 'whence != FROM_MERGE'. Removing \n> that caused t7502 to generate errors. Therefore I left ' if (!commitable && whence != FROM_MERGE \n> && !allow_empty && !(amend && is_a_merge(current_head)))' in the commit.c file.\n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"278436","messageId":"xmqqr3gcj9i5.fsf@gitster.mtv.corp.google.com","threadId":"41374","inReplyTo":"1455590305-30923-1-git-send-email-ischis2@cox.net","subject":"Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-17T03:33:54Z","receivedAt":"2016-02-17T03:33:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stephen P. Smith\" writes:\n\n> The 'commit --dry-run' and commit return values differed if a\n> conflicted merge had been resolved and the commit would be the same as\n> the parent.\n>\n> Update show_merge_in_progress to set the commitable bit if conflicts\n> have been resolved and a merge is in progress.\n>\n> Signed-off-by: Stephen P. Smith \n> ---\n\nI think I mislead you into a slightly wrong direction. While the\nsingle liner does improve the situation, I think this is merely a\nband-aid upon closer inspection. For example, if you changed your\n\"commit --dry-run\" in your test to \"commit --dry-run --short\", you\nwould notice that the test would fail.\n\nIn fact, \"commit --dry-run\" is already broken without this \"a merge\nends up in a no-op\" corner case. The management of s->commitable\nflag and dry_run_commit() that uses it are unfortunately more broken\nthan I originally thought.\n\nIf we check for places where s->committable is set, we notice that\nthere is only one location: wt_status_print_updated(). This function\nruns an equivalent of \"diff-index --cached\" and flips s->committable\non when it sees any difference.\n\nThis function is only called from wt_status_print(), which in turn\nis only called from run_status() in commit.c when the status format\nis unspecified or set to STATUS_FORMAT_LONG.\n\nSo if you do this:\n\n $ git reset --hard HEAD\n $ >a-new-file && git add a-new-file\n $ git commit --dry-run --short; echo $?\n\nyou'd get \"No, there is nothing interesting to commit\", which is\nclearly bogus.\n\nI said s->committable is flipped on only when there is any change in\n\"diff-index --cached\". There is nothing that flips it off, by\nnoticing that there are unmerged paths, for example. This is\nanother brokenness around \"git commit --dry-run\". Imagine that you\nare in a middle of a conflicted cherry-pick. You did \"git add\" on a\nresolved path and you still have another path whose conflict has not\nbeen resolved. If you run a \"git commit --dry-run\", you will hear\n\"Yes, you can make a meaningful commit\", which again is clearly\nbogus.\n\nThese things need to be eventually fixed, and I think the fix will\ninvolve revamping how we compute s->committable flag. Most likely,\nwe won't be doing any of that in any wt_status function whose name\nhas \"print\" or \"show\" in it. As the original designer of the wt_*\nsuite (before these multiple output formats are added), I would say\neverything should happen inside the \"collect\" phase, if we wanted to\nmake s->committable bit usable.\n\nSo in the sense, eventually the code updated by this patch will have\nto be discarded when we fix the \"commit --dry-run\" in the right way,\nbut in the meantime, the patch does not make things worse, so let's\nthink about queuing it as-is for now as a stop-gap measure.\n\nThanks.\n"},{"id":"278438","messageId":"2247870.Eu9nZ6eGrl@thunderbird","threadId":"41374","inReplyTo":"1455590305-30923-1-git-send-email-ischis2@cox.net","subject":"Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2016-02-17T04:58:15Z","receivedAt":"2016-02-17T04:58:15Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"On Tuesday, February 16, 2016 07:33:54 PM Junio C Hamano wrote:\n> \"Stephen P. Smith\" writes:\n> \n> > The 'commit --dry-run' and commit return values differed if a\n> > conflicted merge had been resolved and the commit would be the same as\n> > the parent.\n> >\n> > Update show_merge_in_progress to set the commitable bit if conflicts\n> > have been resolved and a merge is in progress.\n> >\n> > Signed-off-by: Stephen P. Smith \n> > ---\n> \n> I think I mislead you into a slightly wrong direction. While the\n> single liner does improve the situation, I think this is merely a\n> band-aid upon closer inspection. For example, if you changed your\n> \"commit --dry-run\" in your test to \"commit --dry-run --short\", you\n> would notice that the test would fail.\n> \n> In fact, \"commit --dry-run\" is already broken without this \"a merge\n> ends up in a no-op\" corner case. The management of s->commitable\n> flag and dry_run_commit() that uses it are unfortunately more broken\n> than I originally thought.\n> \n> If we check for places where s->committable is set, we notice that\n> there is only one location: wt_status_print_updated(). This function\n> runs an equivalent of \"diff-index --cached\" and flips s->committable\n> on when it sees any difference.\n> \n> This function is only called from wt_status_print(), which in turn\n> is only called from run_status() in commit.c when the status format\n> is unspecified or set to STATUS_FORMAT_LONG.\n> \n> So if you do this:\n> \n> $ git reset --hard HEAD\n> $ >a-new-file && git add a-new-file\n> $ git commit --dry-run --short; echo $?\n> \n> you'd get \"No, there is nothing interesting to commit\", which is\n> clearly bogus.\n> \n> I said s->committable is flipped on only when there is any change in\n> \"diff-index --cached\". There is nothing that flips it off, by\n> noticing that there are unmerged paths, for example. This is\n> another brokenness around \"git commit --dry-run\". Imagine that you\n> are in a middle of a conflicted cherry-pick. You did \"git add\" on a\n> resolved path and you still have another path whose conflict has not\n> been resolved. If you run a \"git commit --dry-run\", you will hear\n> \"Yes, you can make a meaningful commit\", which again is clearly\n> bogus.\n> \n> These things need to be eventually fixed, and I think the fix will\n> involve revamping how we compute s->committable flag. Most likely,\n> we won't be doing any of that in any wt_status function whose name\n> has \"print\" or \"show\" in it. As the original designer of the wt_*\n> suite (before these multiple output formats are added), I would say\n> everything should happen inside the \"collect\" phase, if we wanted to\n> make s->committable bit usable.\n> \n> So in the sense, eventually the code updated by this patch will have\n> to be discarded when we fix the \"commit --dry-run\" in the right way,\n> but in the meantime, the patch does not make things worse, so let's\n> think about queuing it as-is for now as a stop-gap measure.\n> \nHmmmm.... that makes sense. \n\nLet me fix the commit message per what I told Philip and then \nI will start working on a rework of the commitable bit with \nthis in mind as a follow on patch. \n\n> Thanks.\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"278439","messageId":"1455685597-22445-1-git-send-email-ischis2@cox.net","threadId":"41374","inReplyTo":"xmqqr3gcj9i5.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Stephen P. Smith","fromEmail":"ischis2@cox.net","sentAt":"2016-02-17T05:06:37Z","receivedAt":"2016-02-17T05:06:37Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"The 'commit --dry-run' and 'commit' return values differed if a\nconflicted merge had been resolved and the commit would be the same as\nthe parent.\n\nUpdate show_merge_in_progress to set the commitable bit if conflicts\nhave been resolved and a merge is in progress.\n\nSigned-off-by: Stephen P. Smith \n---\n\nNotes:\n Updated the commit message.\n\n t/t7501-commit.sh | 20 ++++++++++++++++++++\n wt-status.c | 1 +\n 2 files changed, 21 insertions(+)\n\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 63e0427..363abb1 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -587,4 +587,24 @@ test_expect_success '--only works on to-be-born branch' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success '--dry-run with conflicts fixed from a merge' '\n+\t# setup two branches with conflicting information\n+\t# in the same file, resolve the conflict,\n+\t# call commit with --dry-run\n+\techo \"Initial contents, unimportant\" >test-file &&\n+\tgit add test-file &&\n+\tgit commit -m \"Initial commit\" &&\n+\techo \"commit-1-state\" >test-file &&\n+\tgit commit -m \"commit 1\" -i test-file &&\n+\tgit tag commit-1 &&\n+\tgit checkout -b branch-2 HEAD^1 &&\n+\techo \"commit-2-state\" >test-file &&\n+\tgit commit -m \"commit 2\" -i test-file &&\n+\t! $(git merge --no-commit commit-1) &&\n+\techo \"commit-2-state\" >test-file &&\n+\tgit add test-file &&\n+\tgit commit --dry-run &&\n+\tgit commit -m \"conflicts fixed from merge.\"\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex ab4f80d..1374b48 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -950,6 +950,7 @@ static void show_merge_in_progress(struct wt_status *s,\n \t\t\tstatus_printf_ln(s, color,\n \t\t\t\t_(\" (fix conflicts and run \\\"git commit\\\")\"));\n \t} else {\n+\t\ts-> commitable = 1;\n \t\tstatus_printf_ln(s, color,\n \t\t\t_(\"All conflicts fixed but you are still merging.\"));\n \t\tif (s->hints)\n-- \n2.7.0.GIT\n"},{"id":"286024","messageId":"5686039.PQA9zH74Pi@thunderbird","threadId":"41374","inReplyTo":"1455590305-30923-1-git-send-email-ischis2@cox.net","subject":"Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2016-05-10T04:51:19Z","receivedAt":"2016-05-10T04:51:19Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"On Tuesday, February 16, 2016 07:33:54 PM Junio C Hamano wrote:\n> > ---\n> \n> I think I mislead you into a slightly wrong direction. While the\n> single liner does improve the situation, I think this is merely a\n> band-aid upon closer inspection. For example, if you changed your\n> \"commit --dry-run\" in your test to \"commit --dry-run --short\", you\n> would notice that the test would fail.\n> \nI understand.\n\n> In fact, \"commit --dry-run\" is already broken without this \"a merge\n> ends up in a no-op\" corner case. The management of s->commitable\n> flag and dry_run_commit() that uses it are unfortunately more broken\n> than I originally thought.\n> \n> If we check for places where s->committable is set, we notice that\n> there is only one location: wt_status_print_updated(). This function\n> runs an equivalent of \"diff-index --cached\" and flips s->committable\n> on when it sees any difference.\n> \n> This function is only called from wt_status_print(), which in turn\n> is only called from run_status() in commit.c when the status format\n> is unspecified or set to STATUS_FORMAT_LONG.\n> \n> So if you do this:\n> \n> $ git reset --hard HEAD\n> $ >a-new-file && git add a-new-file\n> $ git commit --dry-run --short; echo $?\n> \n> you'd get \"No, there is nothing interesting to commit\", which is\n> clearly bogus.\n> \n> I said s->committable is flipped on only when there is any change in\n> \"diff-index --cached\". There is nothing that flips it off, by\n> noticing that there are unmerged paths, for example. This is\n> another brokenness around \"git commit --dry-run\". Imagine that you\n> are in a middle of a conflicted cherry-pick. You did \"git add\" on a\n> resolved path and you still have another path whose conflict has not\n> been resolved. If you run a \"git commit --dry-run\", you will hear\n> \"Yes, you can make a meaningful commit\", which again is clearly\n> bogus.\n> \nMakes sense.\n\n> These things need to be eventually fixed, and I think the fix will\n> involve revamping how we compute s->committable flag. Most likely,\n> we won't be doing any of that in any wt_status function whose name\n> has \"print\" or \"show\" in it. As the original designer of the wt_*\n> suite (before these multiple output formats are added), I would say\n> everything should happen inside the \"collect\" phase, if we wanted to\n> make s->committable bit usable.\nTonight I started work on a patch to remove the two locations where \ncommittable was set in the *print* and *show* functions. \n\nI believe that what you mean by the \"collect\" phase is the set of functions \nthat are in wt_status.c and have collect in the function name.\n\n> \n> So in the sense, eventually the code updated by this patch will have\n> to be discarded when we fix the \"commit --dry-run\" in the right way,\n> but in the meantime, the patch does not make things worse, so let's\n> think about queuing it as-is for now as a stop-gap measure.\n> \nI'm happy with moveing the patch from pu (where it is now) to next. I've\nre-started my work on this.\n\n> Thanks.\n"},{"id":"356232","messageId":"28440975.G22uFktzHy@thunderbird","threadId":"41374","inReplyTo":"1455590305-30923-1-git-send-email-ischis2@cox.net","subject":"Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Stephen Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-08-22T05:33:09Z","receivedAt":"2018-08-22T05:42:31Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"On Tuesday, February 16, 2016 8:33:54 PM MST Junio C Hamano wrote:\n> In fact, \"commit --dry-run\" is already broken without this \"a merge\n> ends up in a no-op\" corner case. The management of s->commitable\n> flag and dry_run_commit() that uses it are unfortunately more broken\n> than I originally thought.\n> \n\n> This function is only called from wt_status_print(), which in turn\n> is only called from run_status() in commit.c when the status format\n> is unspecified or set to STATUS_FORMAT_LONG.\n> \n> So if you do this:\n> \n> $ git reset --hard HEAD\n> $ >a-new-file && git add a-new-file\n> $ git commit --dry-run --short; echo $?\n> \n> you'd get \"No, there is nothing interesting to commit\", which is\n> clearly bogus.\n\nI was about to start working on working on this and ran the test you suggested \nback in 2016. I don't get the error message from that time period. I \nbelieve that this was fixed. \n\n\n\n"},{"id":"356259","messageId":"xmqq4lfm76ly.fsf@gitster-ct.c.googlers.com","threadId":"41374","inReplyTo":"28440975.G22uFktzHy@thunderbird","subject":"Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-22T15:39:53Z","receivedAt":"2018-08-22T15:39:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":""},{"id":"356260","messageId":"xmqqzhxe5rcc.fsf@gitster-ct.c.googlers.com","threadId":"41374","inReplyTo":"28440975.G22uFktzHy@thunderbird","subject":"Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-22T15:54:59Z","receivedAt":"2018-08-22T15:55:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Smith writes:\n\n> On Tuesday, February 16, 2016 8:33:54 PM MST Junio C Hamano wrote:\n\nWow, that's quite an old discussion ;-)\n\n>> So if you do this:\n>> \n>> $ git reset --hard HEAD\n>> $ >a-new-file && git add a-new-file\n>> $ git commit --dry-run --short; echo $?\n>> \n>> you'd get \"No, there is nothing interesting to commit\", which is\n>> clearly bogus.\n>\n> I was about to start working on working on this and ran the test you suggested \n> back in 2016. I don't get the error message from that time period. I \n> believe that this was fixed. \n\nAfter these:\n\n $ git reset --hard HEAD\n $ >a-new-file && git add a-new-file\n\nrunning \n\n $ git commit --dry-run; echo $?\n $ git commit --dry-run --short; echo $?\n\ntells me that \"--short\" still does not notice that there _is_\nsomething to be committed, either with an ancient version like\nv2.10.5 or more modern versions of Git. The \"long\" version exits\nwith 0, while \"--short\" one exists with 1.\n\nSo...?\n"},{"id":"356331","messageId":"4269794.LOdZa0ZRjb@thunderbird","threadId":"41374","inReplyTo":"28440975.G22uFktzHy@thunderbird","subject":"Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-08-23T01:28:14Z","receivedAt":"2018-08-23T01:28:19Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"> > On Tuesday, February 16, 2016 8:33:54 PM MST Junio C Hamano wrote:\n> Wow, that's quite an old discussion ;-)\nIt wasn't intended to be so\n\n> After these:\n\n> tells me that \"--short\" still does not notice that there _is_\n> something to be committed, either with an ancient version like\n> v2.10.5 or more modern versions of Git. The \"long\" version exits\n> with 0, while \"--short\" one exists with 1.\n\n$ git commit --dry-run --short; echo $?\nA a-new-file\n0\n\n$ git commit --dry-run --short; echo $?\nA a-new-file\n1\n\n> \n> So...?\n\nFor me it is the return code that is faulty .... Starting to investigate. \nThanks\n\n\n\n\n"}]}