{"thread":{"id":"50377","subject":"[PATCH v4 1/2] describe: setup working tree for --dirty","startedAt":"2019-02-01T13:55:50Z","lastAt":"2019-02-03T03:35:16Z","messageCount":8,"participants":["Sebastian Staudt","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"368326","messageId":"20190201135512.68220-1-koraktor@gmail.com","threadId":"50377","inReplyTo":null,"subject":"[PATCH v4 1/2] describe: setup working tree for --dirty","fromName":"Sebastian Staudt","fromEmail":"koraktor@gmail.com","sentAt":"2019-02-01T13:55:11Z","receivedAt":"2019-02-01T13:55:50Z","isPatch":true,"sender":{"key":"koraktor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66112?v=4"},"body":"We don't use NEED_WORK_TREE when running the git-describe builtin,\nsince you should be able to describe a commit even in a bare repository.\nHowever, the --dirty flag does need a working tree. Since we don't call\nsetup_work_tree(), it uses whatever directory we happen to be in. That's\nunlikely to match our index, meaning we'd say \"dirty\" even when the real\nworking tree is clean.\n\nWe can fix that by calling setup_work_tree() once we know that the user\nhas asked for --dirty.\n\nThe --broken option also needs a working tree. But because its\nimplementation calls git-diff-index we don‘t have to setup the working\ntree in the git-describe process.\n\nSigned-off-by: Sebastian Staudt \nHelped-by: Jeff King \n---\n builtin/describe.c | 1 +\n t/t6120-describe.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex cc118448ee..b5b7abdc8f 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -629,6 +629,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n \t\t\tint fd, result;\n \n+\t\t\tsetup_work_tree();\n \t\t\tread_cache();\n \t\t\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,\n \t\t\t\t NULL, NULL, NULL);\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex d639d94696..7cfed77c52 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -145,14 +145,38 @@ check_describe A-* HEAD\n \n check_describe \"A-*[0-9a-f]\" --dirty\n \n+test_expect_success 'describe --dirty with --work-tree' '\n+\t(\n+\t\tcd \"$TEST_DIRECTORY\" &&\n+\t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty >\"$TRASH_DIRECTORY/out\"\n+\t) &&\n+\tgrep \"^A-[1-9][0-9]\\?-g[0-9a-f]\\+$\" out\n+'\n+\n test_expect_success 'set-up dirty work tree' '\n \techo >>file\n '\n \n check_describe \"A-*[0-9a-f]-dirty\" --dirty\n \n+test_expect_success 'describe --dirty with --work-tree' '\n+\t(\n+\t\tcd \"$TEST_DIRECTORY\" &&\n+\t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty >\"$TRASH_DIRECTORY/out\"\n+\t) &&\n+\tgrep \"^A-[1-9][0-9]\\?-g[0-9a-f]\\+-dirty$\" out\n+'\n+\n check_describe \"A-*[0-9a-f].mod\" --dirty=.mod\n \n+test_expect_success 'describe --dirty=.mod with --work-tree' '\n+\t(\n+\t\tcd \"$TEST_DIRECTORY\" &&\n+\t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty=.mod >\"$TRASH_DIRECTORY/out\"\n+\t) &&\n+\tgrep \"^A-[1-9][0-9]\\?-g[0-9a-f]\\+.mod$\" out\n+'\n+\n test_expect_success 'describe --dirty HEAD' '\n \ttest_must_fail git describe --dirty HEAD\n '\n@@ -303,8 +327,17 @@ test_expect_success 'describe chokes on severely broken submodules' '\n \tmv .git/modules/sub1/ .git/modules/sub_moved &&\n \ttest_must_fail git describe --dirty\n '\n+\n test_expect_success 'describe ignoring a broken submodule' '\n \tgit describe --broken >out &&\n+\tgrep broken out\n+'\n+\n+test_expect_success 'describe with --work-tree ignoring a broken submodule' '\n+\t(\n+\t\tcd \"$TEST_DIRECTORY\" &&\n+\t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --broken >\"$TRASH_DIRECTORY/out\"\n+\t) &&\n \ttest_when_finished \"mv .git/modules/sub_moved .git/modules/sub1\" &&\n \tgrep broken out\n '\n-- \n2.20.1\n\n"},{"id":"368327","messageId":"20190201135512.68220-2-koraktor@gmail.com","threadId":"50377","inReplyTo":"20190201135512.68220-1-koraktor@gmail.com","subject":"[PATCH v4 2/2] t6120: test for describe with a bare repository","fromName":"Sebastian Staudt","fromEmail":"koraktor@gmail.com","sentAt":"2019-02-01T13:55:12Z","receivedAt":"2019-02-01T13:55:51Z","isPatch":true,"sender":{"key":"koraktor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66112?v=4"},"body":"This ensures that nothing breaks the basic functionality of describe for\nbare repositories. Please note that --broken and --dirty need a working\ntree.\n\nSigned-off-by: Sebastian Staudt \n---\n t/t6120-describe.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 7cfed77c52..ea2c3dbe1c 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -418,4 +418,9 @@ test_expect_success 'describe complains about missing object' '\n \ttest_must_fail git describe $ZERO_OID\n '\n \n+test_expect_success 'describe works from outside repo using --git-dir' ' \n+ git clone --bare \"$TRASH_DIRECTORY\" \"$TRASH_DIRECTORY/bare\" &&\n+ git --git-dir \"$TRASH_DIRECTORY/bare\" describe\n+'\n+\n test_done\n-- \n2.20.1\n\n"},{"id":"368363","messageId":"xmqq7eejz6fs.fsf@gitster-ct.c.googlers.com","threadId":"50377","inReplyTo":"20190201135512.68220-2-koraktor@gmail.com","subject":"Re: [PATCH v4 2/2] t6120: test for describe with a bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-01T18:53:27Z","receivedAt":"2019-02-01T18:53:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Staudt writes:\n\n> This ensures that nothing breaks the basic functionality of describe for\n> bare repositories. Please note that --broken and --dirty need a working\n> tree.\n>\n> Signed-off-by: Sebastian Staudt \n> ---\n> t/t6120-describe.sh | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index 7cfed77c52..ea2c3dbe1c 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -418,4 +418,9 @@ test_expect_success 'describe complains about missing object' '\n> \ttest_must_fail git describe $ZERO_OID\n> '\n> \n> +test_expect_success 'describe works from outside repo using --git-dir' ' \n> + git clone --bare \"$TRASH_DIRECTORY\" \"$TRASH_DIRECTORY/bare\" &&\n> + git --git-dir \"$TRASH_DIRECTORY/bare\" describe\n> +'\n\nOK, it demonstrates that the command exits with status 0. Do we\nwant to validate its output, too?\n"},{"id":"368368","messageId":"CAPig+cQgEVJ6OGn9BRUJoALxE7aDCHK308sq7V0EQVRRn6Vn8g@mail.gmail.com","threadId":"50377","inReplyTo":"20190201135512.68220-1-koraktor@gmail.com","subject":"Re: [PATCH v4 1/2] describe: setup working tree for --dirty","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-01T20:12:03Z","receivedAt":"2019-02-01T20:12:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 1, 2019 at 8:55 AM Sebastian Staudt wrote:\n> We don't use NEED_WORK_TREE when running the git-describe builtin,\n> since you should be able to describe a commit even in a bare repository.\n> However, the --dirty flag does need a working tree. Since we don't call\n> setup_work_tree(), it uses whatever directory we happen to be in. That's\n> unlikely to match our index, meaning we'd say \"dirty\" even when the real\n> working tree is clean.\n> [...]\n> Signed-off-by: Sebastian Staudt \n> ---\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> @@ -145,14 +145,38 @@ check_describe A-* HEAD\n> +test_expect_success 'describe --dirty with --work-tree' '\n> + [...]\n> +'\n>\n> +test_expect_success 'describe --dirty with --work-tree' '\n> + [...]\n> +'\n\nCan you give these two new tests different titles to make it easier to\nnarrow down a problem to one or the other if one of them does fail?\nPerhaps the second test could be titled:\n\n test_expect_success 'describe --dirty with dirty --work-tree' '\n\nor something.\n"},{"id":"368370","messageId":"xmqqzhrfxmco.fsf@gitster-ct.c.googlers.com","threadId":"50377","inReplyTo":"20190201135512.68220-1-koraktor@gmail.com","subject":"Re: [PATCH v4 1/2] describe: setup working tree for --dirty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-01T20:52:39Z","receivedAt":"2019-02-01T20:52:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Staudt writes:\n\n> We don't use NEED_WORK_TREE when running the git-describe builtin,\n> since you should be able to describe a commit even in a bare repository.\n> However, the --dirty flag does need a working tree. Since we don't call\n> setup_work_tree(), it uses whatever directory we happen to be in. That's\n> unlikely to match our index, meaning we'd say \"dirty\" even when the real\n> working tree is clean.\n>\n> We can fix that by calling setup_work_tree() once we know that the user\n> has asked for --dirty.\n>\n> The --broken option also needs a working tree. But because its\n> implementation calls git-diff-index we don‘t have to setup the working\n> tree in the git-describe process.\n\nVery nicely described.\n\n>\n> Signed-off-by: Sebastian Staudt \n> Helped-by: Jeff King \n> ---\n> builtin/describe.c | 1 +\n> t/t6120-describe.sh | 33 +++++++++++++++++++++++++++++++++\n> 2 files changed, 34 insertions(+)\n>\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index cc118448ee..b5b7abdc8f 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -629,6 +629,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n> \t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> \t\t\tint fd, result;\n> \n> +\t\t\tsetup_work_tree();\n> \t\t\tread_cache();\n> \t\t\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,\n> \t\t\t\t NULL, NULL, NULL);\n\nAnd the implementation is as promised in the proposed log message.\nQuite straight-forward and obviously right ;-)\n\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index d639d94696..7cfed77c52 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -145,14 +145,38 @@ check_describe A-* HEAD\n> \n> check_describe \"A-*[0-9a-f]\" --dirty\n> \n> +test_expect_success 'describe --dirty with --work-tree' '\n> +\t(\n> +\t\tcd \"$TEST_DIRECTORY\" &&\n> +\t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty >\"$TRASH_DIRECTORY/out\"\n> +\t) &&\n> +\tgrep \"^A-[1-9][0-9]\\?-g[0-9a-f]\\+$\" out\n> +'\n> +\n\nWithout the fix to the code, this test piece fails with \"-dirty\"\nsuffix in 'out'. Good.\n\n> +test_expect_success 'describe --dirty with --work-tree' '\n> +\t(\n> +\t\tcd \"$TEST_DIRECTORY\" &&\n> +\t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty >\"$TRASH_DIRECTORY/out\"\n> +\t) &&\n> +\tgrep \"^A-[1-9][0-9]\\?-g[0-9a-f]\\+-dirty$\" out\n> +'\n\nThis succeeds with or without the fix to the code; the buggy\nbehaviour was to claim \"-dirty\"-ness when the working tree files are\nclean. This new test is not about that buggy behaviour. It is\nrather about the updated code does not mistakenly claim cleanness in\na dirty working tree. Good.\n\n> check_describe \"A-*[0-9a-f].mod\" --dirty=.mod\n> \n> +test_expect_success 'describe --dirty=.mod with --work-tree' '\n> +\t(\n> +\t\tcd \"$TEST_DIRECTORY\" &&\n> +\t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty=.mod >\"$TRASH_DIRECTORY/out\"\n> +\t) &&\n> +\tgrep \"^A-[1-9][0-9]\\?-g[0-9a-f]\\+.mod$\" out\n> +'\n> +\n\nLikewise.\n\n> test_expect_success 'describe --dirty HEAD' '\n> \ttest_must_fail git describe --dirty HEAD\n> '\n> @@ -303,8 +327,17 @@ test_expect_success 'describe chokes on severely broken submodules' '\n> \tmv .git/modules/sub1/ .git/modules/sub_moved &&\n> \ttest_must_fail git describe --dirty\n> '\n> +\n> test_expect_success 'describe ignoring a broken submodule' '\n> \tgit describe --broken >out &&\n> +\tgrep broken out\n> +'\n> +\n> +test_expect_success 'describe with --work-tree ignoring a broken submodule' '\n> +\t(\n> +\t\tcd \"$TEST_DIRECTORY\" &&\n> +\t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --broken >\"$TRASH_DIRECTORY/out\"\n> +\t) &&\n\nOK, this checks the same repository as the existing test does, but\ndoes so from outside the repository.\n\nLooks good.\n\n> \ttest_when_finished \"mv .git/modules/sub_moved .git/modules/sub1\" &&\n> \tgrep broken out\n> '\n"},{"id":"368401","messageId":"CA+xP2SZ96XqZbCtub9SkxPUNTUVeNZOcXxYvwhMitLgiCVmchg@mail.gmail.com","threadId":"50377","inReplyTo":"xmqq7eejz6fs.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4 2/2] t6120: test for describe with a bare repository","fromName":"Sebastian Staudt","fromEmail":"koraktor@gmail.com","sentAt":"2019-02-02T10:00:45Z","receivedAt":"2019-02-02T10:01:26Z","isPatch":true,"sender":{"key":"koraktor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66112?v=4"},"body":"Am Fr., 1. Feb. 2019 um 19:53 Uhr schrieb Junio C Hamano :\n>\n> Sebastian Staudt writes:\n>\n> > This ensures that nothing breaks the basic functionality of describe for\n> > bare repositories. Please note that --broken and --dirty need a working\n> > tree.\n> >\n> > Signed-off-by: Sebastian Staudt \n> > ---\n> > t/t6120-describe.sh | 5 +++++\n> > 1 file changed, 5 insertions(+)\n> >\n> > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> > index 7cfed77c52..ea2c3dbe1c 100755\n> > --- a/t/t6120-describe.sh\n> > +++ b/t/t6120-describe.sh\n> > @@ -418,4 +418,9 @@ test_expect_success 'describe complains about missing object' '\n> > test_must_fail git describe $ZERO_OID\n> > '\n> >\n> > +test_expect_success 'describe works from outside repo using --git-dir' '\n> > + git clone --bare \"$TRASH_DIRECTORY\" \"$TRASH_DIRECTORY/bare\" &&\n> > + git --git-dir \"$TRASH_DIRECTORY/bare\" describe\n> > +'\n>\n> OK, it demonstrates that the command exits with status 0. Do we\n> want to validate its output, too?\n\nIt won‘t hurt.\nI‘ll move this test further up, so it is executed when the working tree is still\nclean. That way we can use the same regex as for the other checks.\n\nThanks.\n"},{"id":"368402","messageId":"CA+xP2Sa2zLeLg-6pFn9UEufurd=EOOf8UFx3Qh7o0dwzJgN-ag@mail.gmail.com","threadId":"50377","inReplyTo":"CAPig+cQgEVJ6OGn9BRUJoALxE7aDCHK308sq7V0EQVRRn6Vn8g@mail.gmail.com","subject":"Re: [PATCH v4 1/2] describe: setup working tree for --dirty","fromName":"Sebastian Staudt","fromEmail":"koraktor@gmail.com","sentAt":"2019-02-02T10:04:17Z","receivedAt":"2019-02-02T10:04:57Z","isPatch":true,"sender":{"key":"koraktor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66112?v=4"},"body":"Am Fr., 1. Feb. 2019 um 21:12 Uhr schrieb Eric Sunshine\n:\n>\n> On Fri, Feb 1, 2019 at 8:55 AM Sebastian Staudt wrote:\n> > We don't use NEED_WORK_TREE when running the git-describe builtin,\n> > since you should be able to describe a commit even in a bare repository.\n> > However, the --dirty flag does need a working tree. Since we don't call\n> > setup_work_tree(), it uses whatever directory we happen to be in. That's\n> > unlikely to match our index, meaning we'd say \"dirty\" even when the real\n> > working tree is clean.\n> > [...]\n> > Signed-off-by: Sebastian Staudt \n> > ---\n> > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> > @@ -145,14 +145,38 @@ check_describe A-* HEAD\n> > +test_expect_success 'describe --dirty with --work-tree' '\n> > + [...]\n> > +'\n> >\n> > +test_expect_success 'describe --dirty with --work-tree' '\n> > + [...]\n> > +'\n>\n> Can you give these two new tests different titles to make it easier to\n> narrow down a problem to one or the other if one of them does fail?\n> Perhaps the second test could be titled:\n>\n> test_expect_success 'describe --dirty with dirty --work-tree' '\n>\n> or something.\n\nThanks, didn‘t notice this.\nI‘d use a suffix (dirty) for my test titles. But this won‘t work for tests\nusing check_describe(). Any objections?\n"},{"id":"368423","messageId":"CAPig+cRg55g_x7XvfHuP4J17ODnmWp+o7x=Wqpq6v2TCWcgcuw@mail.gmail.com","threadId":"50377","inReplyTo":"CA+xP2Sa2zLeLg-6pFn9UEufurd=EOOf8UFx3Qh7o0dwzJgN-ag@mail.gmail.com","subject":"Re: [PATCH v4 1/2] describe: setup working tree for --dirty","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-03T03:35:03Z","receivedAt":"2019-02-03T03:35:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 2, 2019 at 5:05 AM Sebastian Staudt wrote:\n> Am Fr., 1. Feb. 2019 um 21:12 Uhr schrieb Eric Sunshine\n> :\n> > On Fri, Feb 1, 2019 at 8:55 AM Sebastian Staudt wrote:\n> > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> > > @@ -145,14 +145,38 @@ check_describe A-* HEAD\n> > > +test_expect_success 'describe --dirty with --work-tree' '\n> > > + [...]\n> > > +'\n> > > +test_expect_success 'describe --dirty with --work-tree' '\n> > > + [...]\n> > > +'\n> >\n> > Can you give these two new tests different titles to make it easier to\n> > narrow down a problem to one or the other if one of them does fail?\n> > Perhaps the second test could be titled:\n> >\n> > test_expect_success 'describe --dirty with dirty --work-tree' '\n> >\n> > or something.\n>\n> Thanks, didn‘t notice this.\n> I‘d use a suffix (dirty) for my test titles. But this won‘t work for tests\n> using check_describe(). Any objections?\n\nI have no objections to using suffix \"(dirty)\".\n\nIt's true that there are a few duplicate test titles due to\ncheck_describe() invocations, however, this patch isn't introducing\nany new callers, so it's not strictly a concern of this series. If you\ndid want to address that issue, one possibility would be to add a\n'title' argument to check_describe() and adjust callers to use a\nunique title, however, such a change would not be part of this\nparticular patch, and I'm not convinced that it's even worth the\neffort and churn at this point.\n"}]}