{"thread":{"id":"22986","subject":"[PATCH] git status: Fix false positive \"new commits\" output for dirty submodules","startedAt":"2010-03-11T15:19:18Z","lastAt":"2010-03-12T21:23:52Z","messageCount":4,"participants":["Jens Lehmann","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"136602","messageId":"4B9909F6.5090104@web.de","threadId":"22986","inReplyTo":null,"subject":"[PATCH] git status: Fix false positive \"new commits\" output for dirty submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-03-11T15:19:18Z","receivedAt":"2010-03-11T15:19:18Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Testing if the output \"new commits\" should appear in the long format of\n\"git status\" was done by comparing the hashes of the diffpair. This always\nresulted in printing \"new commits\" for submodules that contained untracked\nor modified content, no matter if they also contained new commits. The\nreason was that the two->sha1 is always the null_sha1 when diffing against\nthe work tree. This patch gets the correct ref to compare against by\ncalling resolve_gitlink_ref(.., \"HEAD\", ..).\n\nUnfortunately the test case that should have found this bug had been\nchanged incorrectly too. It is fixed and extended to test for other\ncombinations too.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n\n\nAnother way to fix this bug would have been to enter the correct sha1\ninto two->sha1 for submodules with new commits. But i am not sure about\nthe side effects such a change would have and thus fixed it this way so\nit could not hit anyone else.\n\n\n t/t7506-status-submodule.sh |   84 +++++++++++++++++++++++++++++++++++++++++--\n wt-status.c                 |    9 ++++-\n 2 files changed, 88 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 1c8d32a..dc9150a 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -34,7 +34,7 @@ test_expect_success 'status with modified file in submodule' '\n \t(cd sub && git reset --hard) &&\n \techo \"changed\" >sub/foo &&\n \tgit status >output &&\n-\tgrep \"modified:   sub (new commits, modified content)\" output\n+\tgrep \"modified:   sub (modified content)\" output\n '\n\n test_expect_success 'status with modified file in submodule (porcelain)' '\n@@ -49,7 +49,7 @@ test_expect_success 'status with modified file in submodule (porcelain)' '\n test_expect_success 'status with added file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \tgit status >output &&\n-\tgrep \"modified:   sub (new commits, modified content)\" output\n+\tgrep \"modified:   sub (modified content)\" output\n '\n\n test_expect_success 'status with added file in submodule (porcelain)' '\n@@ -64,7 +64,7 @@ test_expect_success 'status with untracked file in submodule' '\n \t(cd sub && git reset --hard) &&\n \techo \"content\" >sub/new-file &&\n \tgit status >output &&\n-\tgrep \"modified:   sub (new commits, untracked content)\" output\n+\tgrep \"modified:   sub (untracked content)\" output\n '\n\n test_expect_success 'status with untracked file in submodule (porcelain)' '\n@@ -74,6 +74,84 @@ test_expect_success 'status with untracked file in submodule (porcelain)' '\n \tEOF\n '\n\n+test_expect_success 'status with added and untracked file in submodule' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\techo \"content\" >sub/new-file &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub (modified content, untracked content)\" output\n+'\n+\n+test_expect_success 'status with added and untracked file in submodule (porcelain)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\techo \"content\" >sub/new-file &&\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n+'\n+\n+test_expect_success 'status with modified file in modified submodule' '\n+\t(cd sub && git reset --hard) &&\n+\trm sub/new-file &&\n+\t(cd sub && echo \"next change\" >foo && git commit -m \"next change\" foo) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub (new commits, modified content)\" output\n+'\n+\n+test_expect_success 'status with modified file in modified submodule (porcelain)' '\n+\t(cd sub && git reset --hard) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n+'\n+\n+test_expect_success 'status with added file in modified submodule' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub (new commits, modified content)\" output\n+'\n+\n+test_expect_success 'status with added file in modified submodule (porcelain)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n+'\n+\n+test_expect_success 'status with untracked file in modified submodule' '\n+\t(cd sub && git reset --hard) &&\n+\techo \"content\" >sub/new-file &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub (new commits, untracked content)\" output\n+'\n+\n+test_expect_success 'status with untracked file in modified submodule (porcelain)' '\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n+'\n+\n+test_expect_success 'status with added and untracked file in modified submodule' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\techo \"content\" >sub/new-file &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub (new commits, modified content, untracked content)\" output\n+'\n+\n+test_expect_success 'status with added and untracked file in modified submodule (porcelain)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\techo \"content\" >sub/new-file &&\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n+'\n+\n test_expect_success 'rm submodule contents' '\n \trm -rf sub/* sub/.git\n '\ndiff --git a/wt-status.c b/wt-status.c\nindex e0e915e..d5d23de 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -9,6 +9,7 @@\n #include \"quote.h\"\n #include \"run-command.h\"\n #include \"remote.h\"\n+#include \"refs.h\"\n\n static char default_wt_status_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_NORMAL, /* WT_STATUS_HEADER */\n@@ -238,8 +239,12 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,\n \t\tif (!d->worktree_status)\n \t\t\td->worktree_status = p->status;\n \t\td->dirty_submodule = p->two->dirty_submodule;\n-\t\tif (S_ISGITLINK(p->two->mode))\n-\t\t\td->new_submodule_commits = !!hashcmp(p->one->sha1, p->two->sha1);\n+\t\tif (S_ISGITLINK(p->two->mode)) {\n+\t\t\tunsigned char sha1[20];\n+\n+\t\t\tif (resolve_gitlink_ref(p->two->path, \"HEAD\", sha1) == 0)\n+\t\t\t\td->new_submodule_commits = !!hashcmp(p->one->sha1, sha1);\n+\t\t}\n \t}\n }\n\n-- \n1.7.0.2.387.g275c3b\n"},{"id":"136616","messageId":"7v3a061v7m.fsf@alter.siamese.dyndns.org","threadId":"22986","inReplyTo":"4B9909F6.5090104@web.de","subject":"Re: [PATCH] git status: Fix false positive \"new commits\" output for dirty submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-11T20:04:13Z","receivedAt":"2010-03-11T20:04:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Another way to fix this bug would have been to enter the correct sha1\n> into two->sha1 for submodules with new commits.\n\nI didn't follow all the codepaths involved, but it might be worth a try.\nThe beginning of the patch may look something like this, and then you\nwould probably have to fix potential fallouts in diff.c from this change.\n\n diff-lib.c |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex d7e13cb..a147d9c 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -182,10 +182,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t    && !DIFF_OPT_TST(&revs->diffopt, IGNORE_SUBMODULES)\n \t\t    && (!changed || (revs->diffopt.output_format & DIFF_FORMAT_PATCH))\n \t\t    && is_submodule_modified(ce->name)) {\n-\t\t\tchanged = 1;\n \t\t\tdirty_submodule = 1;\n \t\t}\n-\t\tif (!changed) {\n+\t\tif (!changed && !dirty_submodule) {\n \t\t\tce_mark_uptodate(ce);\n \t\t\tif (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))\n \t\t\t\tcontinue;\n"},{"id":"136617","messageId":"7vpr3azk9h.fsf@alter.siamese.dyndns.org","threadId":"22986","inReplyTo":"7v3a061v7m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git status: Fix false positive \"new commits\" output for dirty submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-11T20:16:42Z","receivedAt":"2010-03-11T20:16:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>\n>> Another way to fix this bug would have been to enter the correct sha1\n>> into two->sha1 for submodules with new commits.\n>\n> I didn't follow all the codepaths involved, but it might be worth a try.\n> The beginning of the patch may look something like this, and then you\n> would probably have to fix potential fallouts in diff.c from this change.\n\nHere is a fix of one of the fallouts.\n\n diff.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex dfdfa1a..eefe836 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3221,6 +3221,8 @@ static void diff_resolve_rename_copy(void)\n \t\t}\n \t\telse if (hashcmp(p->one->sha1, p->two->sha1) ||\n \t\t\t p->one->mode != p->two->mode ||\n+\t\t\t p->one->dirty_submodule ||\n+\t\t\t p->two->dirty_submodule ||\n \t\t\t is_null_sha1(p->one->sha1))\n \t\t\tp->status = DIFF_STATUS_MODIFIED;\n \t\telse {\n"},{"id":"136687","messageId":"4B9AB0E8.90902@web.de","threadId":"22986","inReplyTo":"7vpr3azk9h.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] git status: Fix false positive \"new commits\" output for dirty submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-03-12T21:23:52Z","receivedAt":"2010-03-12T21:23:52Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Testing if the output \"new commits\" should appear in the long format of\n\"git status\" is done by comparing the hashes of the diffpair. This always\nresulted in printing \"new commits\" for submodules that contained untracked\nor modified content, even if they did not contain new commits. The reason\nwas that match_stat_with_submodule() did set the \"changed\" flag for dirty\nsubmodules, resulting in two->sha1 being set to the null_sha1 at the call\nsites, which indicates that new commits are present. This is changed so\nthat when no new commits are present, the same object names are in the\nsha1 field for both sides of the filepair, and the working tree side will\nhave the \"dirty_submodule\" flag set when appropriate. For a submodule to\nbe seen as modified even when it just has a dirty work tree, some\nconditions had to be extended to also check for the \"dirty_submodule\"\nflag.\n\nUnfortunately the test case that should have found this bug had been\nchanged incorrectly too. It is fixed and extended to test for other\ncombinations too.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n\n\nAm 11.03.2010 21:16, schrieb Junio C Hamano:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>>\n>>> Another way to fix this bug would have been to enter the correct sha1\n>>> into two->sha1 for submodules with new commits.\n>>\n>> I didn't follow all the codepaths involved, but it might be worth a try.\n>> The beginning of the patch may look something like this, and then you\n>> would probably have to fix potential fallouts in diff.c from this change.\n> \n> Here is a fix of one of the fallouts.\n\nThanks for the pointers, here is version 2. This patch applies on top of\nthe \"Refactor dirty submodule detection in diff-lib.c\" patch I sent\nrecently. A few more eyeballs to make sure I touched all the relevant\ncodepaths would be highly appreciated.\n\n\n diff-lib.c                  |    6 +--\n diff.c                      |    7 +++-\n t/t7506-status-submodule.sh |   84 +++++++++++++++++++++++++++++++++++++++++--\n 3 files changed, 88 insertions(+), 9 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 64be827..6d4922a 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -72,8 +72,6 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n \t    && !DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES)\n \t    && (!changed || DIFF_OPT_TST(diffopt, DIRTY_SUBMODULES))) {\n \t\t*dirty_submodule = is_submodule_modified(ce->name);\n-\t\tif (*dirty_submodule)\n-\t\t\tchanged = 1;\n \t}\n \treturn changed;\n }\n@@ -202,7 +200,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t}\n \t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n     \t    \t\t\t\t\t    ce_option, &dirty_submodule);\n-\t\tif (!changed) {\n+\t\tif (!changed && !dirty_submodule) {\n \t\t\tce_mark_uptodate(ce);\n \t\t\tif (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))\n \t\t\t\tcontinue;\n@@ -333,7 +331,7 @@ static int show_modified(struct rev_info *revs,\n \t}\n\n \toldmode = old->ce_mode;\n-\tif (mode == oldmode && !hashcmp(sha1, old->sha1) &&\n+\tif (mode == oldmode && !hashcmp(sha1, old->sha1) && !dirty_submodule &&\n \t    !DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))\n \t\treturn 0;\n\ndiff --git a/diff.c b/diff.c\nindex 5aefdcb..f5d93e9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2032,7 +2032,7 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)\n \tchar *data = xmalloc(100), *dirty = \"\";\n\n \t/* Are we looking at the work tree? */\n-\tif (!s->sha1_valid && s->dirty_submodule)\n+\tif (s->dirty_submodule)\n \t\tdirty = \"-dirty\";\n\n \tlen = snprintf(data, 100,\n@@ -3092,7 +3092,8 @@ int diff_unmodified_pair(struct diff_filepair *p)\n \t * dealing with a change.\n \t */\n \tif (one->sha1_valid && two->sha1_valid &&\n-\t    !hashcmp(one->sha1, two->sha1))\n+\t    !hashcmp(one->sha1, two->sha1) &&\n+\t    !one->dirty_submodule && !two->dirty_submodule)\n \t\treturn 1; /* no change */\n \tif (!one->sha1_valid && !two->sha1_valid)\n \t\treturn 1; /* both look at the same file on the filesystem. */\n@@ -3227,6 +3228,8 @@ static void diff_resolve_rename_copy(void)\n \t\t}\n \t\telse if (hashcmp(p->one->sha1, p->two->sha1) ||\n \t\t\t p->one->mode != p->two->mode ||\n+\t\t\t p->one->dirty_submodule ||\n+\t\t\t p->two->dirty_submodule ||\n \t\t\t is_null_sha1(p->one->sha1))\n \t\t\tp->status = DIFF_STATUS_MODIFIED;\n \t\telse {\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 1c8d32a..dc9150a 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -34,7 +34,7 @@ test_expect_success 'status with modified file in submodule' '\n \t(cd sub && git reset --hard) &&\n \techo \"changed\" >sub/foo &&\n \tgit status >output &&\n-\tgrep \"modified:   sub (new commits, modified content)\" output\n+\tgrep \"modified:   sub (modified content)\" output\n '\n\n test_expect_success 'status with modified file in submodule (porcelain)' '\n@@ -49,7 +49,7 @@ test_expect_success 'status with modified file in submodule (porcelain)' '\n test_expect_success 'status with added file in submodule' '\n \t(cd sub && git reset --hard && echo >foo && git add foo) &&\n \tgit status >output &&\n-\tgrep \"modified:   sub (new commits, modified content)\" output\n+\tgrep \"modified:   sub (modified content)\" output\n '\n\n test_expect_success 'status with added file in submodule (porcelain)' '\n@@ -64,7 +64,7 @@ test_expect_success 'status with untracked file in submodule' '\n \t(cd sub && git reset --hard) &&\n \techo \"content\" >sub/new-file &&\n \tgit status >output &&\n-\tgrep \"modified:   sub (new commits, untracked content)\" output\n+\tgrep \"modified:   sub (untracked content)\" output\n '\n\n test_expect_success 'status with untracked file in submodule (porcelain)' '\n@@ -74,6 +74,84 @@ test_expect_success 'status with untracked file in submodule (porcelain)' '\n \tEOF\n '\n\n+test_expect_success 'status with added and untracked file in submodule' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\techo \"content\" >sub/new-file &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub (modified content, untracked content)\" output\n+'\n+\n+test_expect_success 'status with added and untracked file in submodule (porcelain)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\techo \"content\" >sub/new-file &&\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n+'\n+\n+test_expect_success 'status with modified file in modified submodule' '\n+\t(cd sub && git reset --hard) &&\n+\trm sub/new-file &&\n+\t(cd sub && echo \"next change\" >foo && git commit -m \"next change\" foo) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub (new commits, modified content)\" output\n+'\n+\n+test_expect_success 'status with modified file in modified submodule (porcelain)' '\n+\t(cd sub && git reset --hard) &&\n+\techo \"changed\" >sub/foo &&\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n+'\n+\n+test_expect_success 'status with added file in modified submodule' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub (new commits, modified content)\" output\n+'\n+\n+test_expect_success 'status with added file in modified submodule (porcelain)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n+'\n+\n+test_expect_success 'status with untracked file in modified submodule' '\n+\t(cd sub && git reset --hard) &&\n+\techo \"content\" >sub/new-file &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub (new commits, untracked content)\" output\n+'\n+\n+test_expect_success 'status with untracked file in modified submodule (porcelain)' '\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n+'\n+\n+test_expect_success 'status with added and untracked file in modified submodule' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\techo \"content\" >sub/new-file &&\n+\tgit status >output &&\n+\tgrep \"modified:   sub (new commits, modified content, untracked content)\" output\n+'\n+\n+test_expect_success 'status with added and untracked file in modified submodule (porcelain)' '\n+\t(cd sub && git reset --hard && echo >foo && git add foo) &&\n+\techo \"content\" >sub/new-file &&\n+\tgit status --porcelain >output &&\n+\tdiff output - <<-\\EOF\n+\t M sub\n+\tEOF\n+'\n+\n test_expect_success 'rm submodule contents' '\n \trm -rf sub/* sub/.git\n '\n-- \n1.7.0.2.386.ge30d.dirty\n"}]}