{"thread":{"id":"50329","subject":"[PATCH v2 1/3] Add tests for describe with --work-tree","startedAt":"2019-01-26T20:51:07Z","lastAt":"2019-01-30T16:47:32Z","messageCount":11,"participants":["Sebastian Staudt","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"367714","messageId":"20190126204951.42455-1-koraktor@gmail.com","threadId":"50329","inReplyTo":null,"subject":"[PATCH v2 1/3] Add tests for describe with --work-tree","fromName":"Sebastian Staudt","fromEmail":"koraktor@gmail.com","sentAt":"2019-01-26T20:49:49Z","receivedAt":"2019-01-26T20:51:07Z","isPatch":true,"sender":{"key":"koraktor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66112?v=4"},"body":"The dirty ones are already passing, but just because describe is comparing\nwith the wrong working tree.\n\nSigned-off-by: Sebastian Staudt \n---\n t/t6120-describe.sh | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex d639d94696..9a6bd1541f 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -28,6 +28,24 @@ check_describe () {\n \t'\n }\n \n+check_describe_worktree () {\n+ cd \"$TEST_DIRECTORY\"\n+\texpect=\"$1\"\n+\tshift\n+\tR=$(git --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe \"$@\" 2>err.actual)\n+\tS=$?\n+\tcat err.actual >&3\n+\ttest_expect_success \"describe with --work-tree $*\" '\n+\ttest $S = 0 &&\n+\tcase \"$R\" in\n+\t$expect)\techo happy ;;\n+\t*)\techo \"Oops - $R is not $expect\";\n+\t\tfalse ;;\n+\tesac\n+\t'\n+ cd \"$TRASH_DIRECTORY\"\n+}\n+\n test_expect_success setup '\n \n \ttest_tick &&\n@@ -145,14 +163,20 @@ check_describe A-* HEAD\n \n check_describe \"A-*[0-9a-f]\" --dirty\n \n+check_describe_worktree \"A-*[0-9a-f]\" --dirty\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+check_describe_worktree \"A-*[0-9a-f]-dirty\" --dirty\n+\n check_describe \"A-*[0-9a-f].mod\" --dirty=.mod\n \n+check_describe_worktree \"A-*[0-9a-f].mod\" --dirty=.mod\n+\n test_expect_success 'describe --dirty HEAD' '\n \ttest_must_fail git describe --dirty HEAD\n '\n-- \n2.20.1\n\n"},{"id":"367715","messageId":"20190126204951.42455-2-koraktor@gmail.com","threadId":"50329","inReplyTo":"20190126204951.42455-1-koraktor@gmail.com","subject":"[PATCH v2 2/3] Setup working tree in describe","fromName":"Sebastian Staudt","fromEmail":"koraktor@gmail.com","sentAt":"2019-01-26T20:49:50Z","receivedAt":"2019-01-26T20:51:09Z","isPatch":true,"sender":{"key":"koraktor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66112?v=4"},"body":"This ensures the given working tree is used for --dirty.\n\nThe implementation of --broken uses diff-index which calls\nsetup_work_tree() itself.\n\nSigned-off-by: Sebastian Staudt \n---\n builtin/describe.c | 1 +\n 1 file changed, 1 insertion(+)\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);\n-- \n2.20.1\n\n"},{"id":"367716","messageId":"20190126204951.42455-3-koraktor@gmail.com","threadId":"50329","inReplyTo":"20190126204951.42455-1-koraktor@gmail.com","subject":"[PATCH v2 3/3] Add test for describe with a bare repository","fromName":"Sebastian Staudt","fromEmail":"koraktor@gmail.com","sentAt":"2019-01-26T20:49:51Z","receivedAt":"2019-01-26T20:51:10Z","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 | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 9a6bd1541f..ddd8cc307d 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -409,4 +409,11 @@ 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+ BARE_CLONE=$(mktemp -d) &&\n+ git clone --bare '$TRASH_DIRECTORY' \\$BARE_CLONE >/Users/koraktor/open-source/others/git/t/out &&\n+ echo $PWD >/Users/koraktor/open-source/others/git/t/out &&\n+ git --git-dir \\$BARE_CLONE describe 2>&1 >/Users/koraktor/open-source/others/git/t/out\n+\"\n+\n test_done\n-- \n2.20.1\n\n"},{"id":"367725","messageId":"CACsJy8A6hF+Sxitpt3J9EmDEXjGb2e+AeMtJDaE7Y32fg4NEKA@mail.gmail.com","threadId":"50329","inReplyTo":"20190126204951.42455-1-koraktor@gmail.com","subject":"Re: [PATCH v2 1/3] Add tests for describe with --work-tree","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-27T00:07:14Z","receivedAt":"2019-01-27T00:07:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote:\n>\n> The dirty ones are already passing, but just because describe is comparing\n> with the wrong working tree.\n>\n> Signed-off-by: Sebastian Staudt \n> ---\n> t/t6120-describe.sh | 24 ++++++++++++++++++++++++\n> 1 file changed, 24 insertions(+)\n>\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index d639d94696..9a6bd1541f 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -28,6 +28,24 @@ check_describe () {\n> '\n> }\n>\n> +check_describe_worktree () {\n> + cd \"$TEST_DIRECTORY\"\n\nStrange alignment. We normally do it in a subshell...\n\n> + expect=\"$1\"\n> + shift\n> + R=$(git --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe \"$@\" 2>err.actual)\n\nThese commands should be executed inside test_expect_success, not\noutside. And you need to chain commands with && to make sure if\nsomething breaks, then the whole test will fai.\n\nIf it's too ugly to generate test_expect_success with a shell\nfunction, then just write a shell function that \"describe\" and compare\n(i.e. the test body). Then you can write something like this later\n\ntest_expect_sucesss 'describe with --worktree foo' '\n check_describe_worktree foo\n'\n\nand check_describe_worktree can now do\n\n( cd \"$TEST_DIRECTORY\" && .... )\n\n\n\n> + S=$?\n> + cat err.actual >&3\n> + test_expect_success \"describe with --work-tree $*\" '\n> + test $S = 0 &&\n> + case \"$R\" in\n> + $expect) echo happy ;;\n> + *) echo \"Oops - $R is not $expect\";\n> + false ;;\n> + esac\n> + '\n> + cd \"$TRASH_DIRECTORY\"\n> +}\n> +\n> test_expect_success setup '\n>\n> test_tick &&\n> @@ -145,14 +163,20 @@ check_describe A-* HEAD\n>\n> check_describe \"A-*[0-9a-f]\" --dirty\n>\n> +check_describe_worktree \"A-*[0-9a-f]\" --dirty\n> +\n> test_expect_success 'set-up dirty work tree' '\n> echo >>file\n> '\n>\n> check_describe \"A-*[0-9a-f]-dirty\" --dirty\n>\n> +check_describe_worktree \"A-*[0-9a-f]-dirty\" --dirty\n> +\n> check_describe \"A-*[0-9a-f].mod\" --dirty=.mod\n>\n> +check_describe_worktree \"A-*[0-9a-f].mod\" --dirty=.mod\n> +\n> test_expect_success 'describe --dirty HEAD' '\n> test_must_fail git describe --dirty HEAD\n> '\n> --\n> 2.20.1\n>\n\n\n-- \nDuy\n"},{"id":"367727","messageId":"CACsJy8A+QsSDfL_RX2ctNsV35VpBaSp4M7hR00nSObmmULUmvg@mail.gmail.com","threadId":"50329","inReplyTo":"20190126204951.42455-2-koraktor@gmail.com","subject":"Re: [PATCH v2 2/3] Setup working tree in describe","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-27T00:21:59Z","receivedAt":"2019-01-27T00:22:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote:\n>\n> This ensures the given working tree is used for --dirty.\n>\n> The implementation of --broken uses diff-index which calls\n> setup_work_tree() itself.\n\nIt would be nice to have a test case covering --broken even if no fix\nis needed (so that somebody else will not accidentally break it\nlater). I did a quick test and thought it was broken, but it turns out\nI tested it wrong :P\n\n> Signed-off-by: Sebastian Staudt \n> ---\n> builtin/describe.c | 1 +\n> 1 file changed, 1 insertion(+)\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> struct argv_array args = ARGV_ARRAY_INIT;\n> int fd, result;\n>\n> + setup_work_tree();\n> read_cache();\n> refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,\n> NULL, NULL, NULL);\n> --\n> 2.20.1\n>\n\n\n-- \nDuy\n"},{"id":"367728","messageId":"CACsJy8C27hKNTyGE5P+F6nzszN_z85chEq8MvQ+BgiDAi=4jww@mail.gmail.com","threadId":"50329","inReplyTo":"20190126204951.42455-3-koraktor@gmail.com","subject":"Re: [PATCH v2 3/3] Add test for describe with a bare repository","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-27T00:25:23Z","receivedAt":"2019-01-27T00:28:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote:\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 | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index 9a6bd1541f..ddd8cc307d 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -409,4 +409,11 @@ 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> + BARE_CLONE=$(mktemp -d) &&\n\nNo, keep everything in $TRASH_DIRECTORY so it will be automatically cleaned.\n\n> + git clone --bare '$TRASH_DIRECTORY' \\$BARE_CLONE >/Users/koraktor/open-source/others/git/t/out &&\n\nEhh.. I'm pretty sure I don't have /Users/koraktor on my system :)\nThis looks like just debug code, I think you can drop \">..\" part for\nall commands.\n\n> + echo $PWD >/Users/koraktor/open-source/others/git/t/out &&\n> + git --git-dir \\$BARE_CLONE describe 2>&1 >/Users/koraktor/open-source/others/git/t/out\n> +\"\n> +\n> test_done\n> --\n> 2.20.1\n>\n\n\n-- \nDuy\n"},{"id":"367746","messageId":"CA+xP2SahEsQKxmjasA5XjuaEVNjbRNiKQv9uKkFkWoORv7WAXQ@mail.gmail.com","threadId":"50329","inReplyTo":"CACsJy8C27hKNTyGE5P+F6nzszN_z85chEq8MvQ+BgiDAi=4jww@mail.gmail.com","subject":"Re: [PATCH v2 3/3] Add test for describe with a bare repository","fromName":"Sebastian Staudt","fromEmail":"koraktor@gmail.com","sentAt":"2019-01-27T06:54:14Z","receivedAt":"2019-01-27T06:54:55Z","isPatch":true,"sender":{"key":"koraktor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66112?v=4"},"body":"Am So., 27. Jan. 2019 um 01:25 Uhr schrieb Duy Nguyen :\n>\n> On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote:\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 | 7 +++++++\n> > 1 file changed, 7 insertions(+)\n> >\n> > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> > index 9a6bd1541f..ddd8cc307d 100755\n> > --- a/t/t6120-describe.sh\n> > +++ b/t/t6120-describe.sh\n> > @@ -409,4 +409,11 @@ 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> > + BARE_CLONE=$(mktemp -d) &&\n>\n> No, keep everything in $TRASH_DIRECTORY so it will be automatically cleaned.\n\nLooks like a relic from trying to get Git to not find an appropriate\nworking tree.\n$TRASH_DIRECTORY/bare works here, too.\n\n>\n> > + git clone --bare '$TRASH_DIRECTORY' \\$BARE_CLONE >/Users/koraktor/open-source/others/git/t/out &&\n>\n> Ehh.. I'm pretty sure I don't have /Users/koraktor on my system :)\n> This looks like just debug code, I think you can drop \">..\" part for\n> all commands.\n>\n\nSorry. A bit embarrassing to leave debugging code in place.\nI will remove the output redirection and echo.\n\n\n> > + echo $PWD >/Users/koraktor/open-source/others/git/t/out &&\n> > + git --git-dir \\$BARE_CLONE describe 2>&1 >/Users/koraktor/open-source/others/git/t/out\n> > +\"\n> > +\n> > test_done\n> > --\n> > 2.20.1\n> >\n>\n>\n> --\n> Duy\n"},{"id":"367747","messageId":"CA+xP2SbLwmeWpqmjCiqojra5Mwrbok7sjvUsvCsaAo6XsWBbtA@mail.gmail.com","threadId":"50329","inReplyTo":"CACsJy8A6hF+Sxitpt3J9EmDEXjGb2e+AeMtJDaE7Y32fg4NEKA@mail.gmail.com","subject":"Re: [PATCH v2 1/3] Add tests for describe with --work-tree","fromName":"Sebastian Staudt","fromEmail":"koraktor@gmail.com","sentAt":"2019-01-27T07:13:51Z","receivedAt":"2019-01-27T07:14:31Z","isPatch":true,"sender":{"key":"koraktor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66112?v=4"},"body":"Am So., 27. Jan. 2019 um 01:07 Uhr schrieb Duy Nguyen :\n>\n> On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote:\n> >\n> > The dirty ones are already passing, but just because describe is comparing\n> > with the wrong working tree.\n> >\n> > Signed-off-by: Sebastian Staudt \n> > ---\n> > t/t6120-describe.sh | 24 ++++++++++++++++++++++++\n> > 1 file changed, 24 insertions(+)\n> >\n> > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> > index d639d94696..9a6bd1541f 100755\n> > --- a/t/t6120-describe.sh\n> > +++ b/t/t6120-describe.sh\n> > @@ -28,6 +28,24 @@ check_describe () {\n> > '\n> > }\n> >\n> > +check_describe_worktree () {\n> > + cd \"$TEST_DIRECTORY\"\n>\n> Strange alignment. We normally do it in a subshell...\n\nSure, will fix this.\n\n>\n> > + expect=\"$1\"\n> > + shift\n> > + R=$(git --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe \"$@\" 2>err.actual)\n>\n> These commands should be executed inside test_expect_success, not\n> outside. And you need to chain commands with && to make sure if\n> something breaks, then the whole test will fai.\n>\n> If it's too ugly to generate test_expect_success with a shell\n> function, then just write a shell function that \"describe\" and compare\n> (i.e. the test body). Then you can write something like this later\n>\n> test_expect_sucesss 'describe with --worktree foo' '\n> check_describe_worktree foo\n> '\n>\n> and check_describe_worktree can now do\n>\n> ( cd \"$TEST_DIRECTORY\" && .... )\n>\n>\n\nMy function is a modified version of check_describe(). Which does the\nsame thing. I‘m not really experienced in Shell programming, so I didn‘t\nsee a cleaner way.\nBut having the cd commands in the && chain looks broken as it would\nbreak the following tests when one test fails and the code was executed\nin the wrong directory afterwards.\n\n\n>\n> > + S=$?\n> > + cat err.actual >&3\n> > + test_expect_success \"describe with --work-tree $*\" '\n> > + test $S = 0 &&\n> > + case \"$R\" in\n> > + $expect) echo happy ;;\n> > + *) echo \"Oops - $R is not $expect\";\n> > + false ;;\n> > + esac\n> > + '\n> > + cd \"$TRASH_DIRECTORY\"\n> > +}\n> > +\n> > test_expect_success setup '\n> >\n> > test_tick &&\n> > @@ -145,14 +163,20 @@ check_describe A-* HEAD\n> >\n> > check_describe \"A-*[0-9a-f]\" --dirty\n> >\n> > +check_describe_worktree \"A-*[0-9a-f]\" --dirty\n> > +\n> > test_expect_success 'set-up dirty work tree' '\n> > echo >>file\n> > '\n> >\n> > check_describe \"A-*[0-9a-f]-dirty\" --dirty\n> >\n> > +check_describe_worktree \"A-*[0-9a-f]-dirty\" --dirty\n> > +\n> > check_describe \"A-*[0-9a-f].mod\" --dirty=.mod\n> >\n> > +check_describe_worktree \"A-*[0-9a-f].mod\" --dirty=.mod\n> > +\n> > test_expect_success 'describe --dirty HEAD' '\n> > test_must_fail git describe --dirty HEAD\n> > '\n> > --\n> > 2.20.1\n> >\n>\n>\n> --\n> Duy\n"},{"id":"367748","messageId":"CA+xP2SYk-NtvQf3c6ahwdO-W5zfV6D+h-oQOmkuqee=ZsEHHTw@mail.gmail.com","threadId":"50329","inReplyTo":"CACsJy8A+QsSDfL_RX2ctNsV35VpBaSp4M7hR00nSObmmULUmvg@mail.gmail.com","subject":"Re: [PATCH v2 2/3] Setup working tree in describe","fromName":"Sebastian Staudt","fromEmail":"koraktor@gmail.com","sentAt":"2019-01-27T07:19:42Z","receivedAt":"2019-01-27T07:20:23Z","isPatch":true,"sender":{"key":"koraktor@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66112?v=4"},"body":"Am So., 27. Jan. 2019 um 01:22 Uhr schrieb Duy Nguyen :\n>\n> On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote:\n> >\n> > This ensures the given working tree is used for --dirty.\n> >\n> > The implementation of --broken uses diff-index which calls\n> > setup_work_tree() itself.\n>\n> It would be nice to have a test case covering --broken even if no fix\n> is needed (so that somebody else will not accidentally break it\n> later). I did a quick test and thought it was broken, but it turns out\n> I tested it wrong :P\n>\n\nThere‘s only one test (\"describe ignoring a broken submodule\") which\neffectively tests --broken.\nI could reuse this.\n\nBTW, is \"describe ignoring a broken submodule\" the right description here?\nIn fact, this should probably be named \"describe detects a broken submodule\".\n\n> > Signed-off-by: Sebastian Staudt \n> > ---\n> > builtin/describe.c | 1 +\n> > 1 file changed, 1 insertion(+)\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> > struct argv_array args = ARGV_ARRAY_INIT;\n> > int fd, result;\n> >\n> > + setup_work_tree();\n> > read_cache();\n> > refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,\n> > NULL, NULL, NULL);\n> > --\n> > 2.20.1\n> >\n>\n>\n> --\n> Duy\n"},{"id":"367819","messageId":"20190128100659.GA6257@ash","threadId":"50329","inReplyTo":"CA+xP2SbLwmeWpqmjCiqojra5Mwrbok7sjvUsvCsaAo6XsWBbtA@mail.gmail.com","subject":"Re: [PATCH v2 1/3] Add tests for describe with --work-tree","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-28T10:06:59Z","receivedAt":"2019-01-28T10:07:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Jan 27, 2019 at 08:13:51AM +0100, Sebastian Staudt wrote:\n> Am So., 27. Jan. 2019 um 01:07 Uhr schrieb Duy Nguyen :\n> >\n> > On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote:\n> > >\n> > > The dirty ones are already passing, but just because describe is comparing\n> > > with the wrong working tree.\n> > >\n> > > Signed-off-by: Sebastian Staudt \n> > > ---\n> > > t/t6120-describe.sh | 24 ++++++++++++++++++++++++\n> > > 1 file changed, 24 insertions(+)\n> > >\n> > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> > > index d639d94696..9a6bd1541f 100755\n> > > --- a/t/t6120-describe.sh\n> > > +++ b/t/t6120-describe.sh\n> > > @@ -28,6 +28,24 @@ check_describe () {\n> > > '\n> > > }\n> > >\n> > > +check_describe_worktree () {\n> > > + cd \"$TEST_DIRECTORY\"\n> >\n> > Strange alignment. We normally do it in a subshell...\n> \n> Sure, will fix this.\n> \n> >\n> > > + expect=\"$1\"\n> > > + shift\n> > > + R=$(git --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe \"$@\" 2>err.actual)\n> >\n> > These commands should be executed inside test_expect_success, not\n> > outside. And you need to chain commands with && to make sure if\n> > something breaks, then the whole test will fai.\n> >\n> > If it's too ugly to generate test_expect_success with a shell\n> > function, then just write a shell function that \"describe\" and compare\n> > (i.e. the test body). Then you can write something like this later\n> >\n> > test_expect_sucesss 'describe with --worktree foo' '\n> > check_describe_worktree foo\n> > '\n> >\n> > and check_describe_worktree can now do\n> >\n> > ( cd \"$TEST_DIRECTORY\" && .... )\n> >\n> >\n> \n> My function is a modified version of check_describe().\n\nWhoa. That function is 12 years old! I think our style has evolved a\nbit since then.\n\n> Which does the same thing. I‘m not really experienced in Shell\n> programming, so I didn‘t see a cleaner way.\n>\n> But having the cd commands in the && chain looks broken as it would\n> break the following tests when one test fails and the code was executed\n> in the wrong directory afterwards.\n\nI mean chaining within a test. This is to make sure any failure\ntriggers the test failure (as it should, if some command is expected\nto fail, we have other ways to catch it).\n\nI would start with something simple, not using shell function at\nall. Something like this as an example (I added run_describe() because\nthat \"git\" command becomes too long). Have a look at the \"do's and\ndon'ts\" in t/README too.\n\n-- 8< --\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex d639d94696..646bedf4e9 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -28,6 +28,10 @@ check_describe () {\n \t'\n }\n \n+run_describe() {\n+\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe \"$@\"\n+}\n+\n test_expect_success setup '\n \n \ttest_tick &&\n@@ -145,6 +149,14 @@ check_describe A-* HEAD\n \n check_describe \"A-*[0-9a-f]\" --dirty\n \n+test_expect_success 'describe with --work-tree --dirty' '\n+\t(\n+\t\tcd \"$TEST_DIRECTORY\" &&\n+\t\trun_describe --dirty 2>err.actual >actual &&\n+\t\tgrep \"^A-.*[0-9a-f]$\" actual\n+\t)\n+'\n+\n test_expect_success 'set-up dirty work tree' '\n \techo >>file\n '\n-- 8< --\n\nBTW, careful about _success or _failure. You need to make sure bisect\nis not broken. If you add a test to confirm a broken case then it\nshould be test_expect_failure (and the test suite will pass). Then\nwhen you fix it you can flip it to test_expect_success.\n--\nDuy\n"},{"id":"368142","messageId":"xmqq5zu65bz5.fsf@gitster-ct.c.googlers.com","threadId":"50329","inReplyTo":"20190128100659.GA6257@ash","subject":"Re: [PATCH v2 1/3] Add tests for describe with --work-tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-30T16:47:26Z","receivedAt":"2019-01-30T16:47:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> My function is a modified version of check_describe().\n>\n> Whoa. That function is 12 years old! I think our style has evolved a\n> bit since then.\n\n;-).\n\n> I mean chaining within a test. This is to make sure any failure\n> triggers the test failure (as it should, if some command is expected\n> to fail, we have other ways to catch it).\n>\n> I would start with something simple, not using shell function at\n> all. Something like this as an example (I added run_describe() because\n> that \"git\" command becomes too long). Have a look at the \"do's and\n> don'ts\" in t/README too.\n\nThanks for guiding new contributors with an easy to understand example.\n\n> BTW, careful about _success or _failure. You need to make sure bisect\n> is not broken. If you add a test to confirm a broken case then it\n> should be test_expect_failure (and the test suite will pass). Then\n> when you fix it you can flip it to test_expect_success.\n\nAnd if the fix is simple enough (i.e. a good rule of thumb is if the\nfixes themselves without tests need to be multi-patch series, it is\nnot simple enough), have a single patch that has both fix and test\nthat expects success, instead of splitting them into two to make a\ntwo patch series whose first step expects a failure and whose second\nstep fixes and flips failure to success.\n\n"}]}