# [PATCH v2 1/3] Add tests for describe with --work-tree 11 messages from 2019-01-26 to 2019-01-30. Participants: Sebastian Staudt, Duy Nguyen, Junio C Hamano. Thread: https://gitlist.dev/t/50329 ## Sebastian Staudt, 2019-01-26 20:49 Subject: [PATCH v2 1/3] Add tests for describe with --work-tree Message-ID: <20190126204951.42455-1-koraktor@gmail.com> URL: https://gitlist.dev/e/20190126204951.42455-1-koraktor%40gmail.com ``` The dirty ones are already passing, but just because describe is comparing with the wrong working tree. Signed-off-by: Sebastian Staudt --- t/t6120-describe.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index d639d94696..9a6bd1541f 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -28,6 +28,24 @@ check_describe () { ' } +check_describe_worktree () { + cd "$TEST_DIRECTORY" + expect="$1" + shift + R=$(git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe "$@" 2>err.actual) + S=$? + cat err.actual >&3 + test_expect_success "describe with --work-tree $*" ' + test $S = 0 && + case "$R" in + $expect) echo happy ;; + *) echo "Oops - $R is not $expect"; + false ;; + esac + ' + cd "$TRASH_DIRECTORY" +} + test_expect_success setup ' test_tick && @@ -145,14 +163,20 @@ check_describe A-* HEAD check_describe "A-*[0-9a-f]" --dirty +check_describe_worktree "A-*[0-9a-f]" --dirty + test_expect_success 'set-up dirty work tree' ' echo >>file ' check_describe "A-*[0-9a-f]-dirty" --dirty +check_describe_worktree "A-*[0-9a-f]-dirty" --dirty + check_describe "A-*[0-9a-f].mod" --dirty=.mod +check_describe_worktree "A-*[0-9a-f].mod" --dirty=.mod + test_expect_success 'describe --dirty HEAD' ' test_must_fail git describe --dirty HEAD ' -- 2.20.1 ``` ## Sebastian Staudt, 2019-01-26 20:49 Subject: [PATCH v2 2/3] Setup working tree in describe Message-ID: <20190126204951.42455-2-koraktor@gmail.com> URL: https://gitlist.dev/e/20190126204951.42455-2-koraktor%40gmail.com In-Reply-To: <20190126204951.42455-1-koraktor@gmail.com> ``` This ensures the given working tree is used for --dirty. The implementation of --broken uses diff-index which calls setup_work_tree() itself. Signed-off-by: Sebastian Staudt --- builtin/describe.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/describe.c b/builtin/describe.c index cc118448ee..b5b7abdc8f 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -629,6 +629,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix) struct argv_array args = ARGV_ARRAY_INIT; int fd, result; + setup_work_tree(); read_cache(); refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL); -- 2.20.1 ``` ## Sebastian Staudt, 2019-01-26 20:49 Subject: [PATCH v2 3/3] Add test for describe with a bare repository Message-ID: <20190126204951.42455-3-koraktor@gmail.com> URL: https://gitlist.dev/e/20190126204951.42455-3-koraktor%40gmail.com In-Reply-To: <20190126204951.42455-1-koraktor@gmail.com> ``` This ensures that nothing breaks the basic functionality of describe for bare repositories. Please note that --broken and --dirty need a working tree. Signed-off-by: Sebastian Staudt --- t/t6120-describe.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index 9a6bd1541f..ddd8cc307d 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -409,4 +409,11 @@ test_expect_success 'describe complains about missing object' ' test_must_fail git describe $ZERO_OID ' +test_expect_success 'describe works from outside repo using --git-dir' " + BARE_CLONE=$(mktemp -d) && + git clone --bare '$TRASH_DIRECTORY' \$BARE_CLONE >/Users/koraktor/open-source/others/git/t/out && + echo $PWD >/Users/koraktor/open-source/others/git/t/out && + git --git-dir \$BARE_CLONE describe 2>&1 >/Users/koraktor/open-source/others/git/t/out +" + test_done -- 2.20.1 ``` ## Duy Nguyen, 2019-01-27 00:07 Subject: Re: [PATCH v2 1/3] Add tests for describe with --work-tree Message-ID: URL: https://gitlist.dev/e/CACsJy8A6hF%2BSxitpt3J9EmDEXjGb2e%2BAeMtJDaE7Y32fg4NEKA%40mail.gmail.com In-Reply-To: <20190126204951.42455-1-koraktor@gmail.com> ``` On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote: > > The dirty ones are already passing, but just because describe is comparing > with the wrong working tree. > > Signed-off-by: Sebastian Staudt > --- > t/t6120-describe.sh | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh > index d639d94696..9a6bd1541f 100755 > --- a/t/t6120-describe.sh > +++ b/t/t6120-describe.sh > @@ -28,6 +28,24 @@ check_describe () { > ' > } > > +check_describe_worktree () { > + cd "$TEST_DIRECTORY" Strange alignment. We normally do it in a subshell... > + expect="$1" > + shift > + R=$(git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe "$@" 2>err.actual) These commands should be executed inside test_expect_success, not outside. And you need to chain commands with && to make sure if something breaks, then the whole test will fai. If it's too ugly to generate test_expect_success with a shell function, then just write a shell function that "describe" and compare (i.e. the test body). Then you can write something like this later test_expect_sucesss 'describe with --worktree foo' ' check_describe_worktree foo ' and check_describe_worktree can now do ( cd "$TEST_DIRECTORY" && .... ) > + S=$? > + cat err.actual >&3 > + test_expect_success "describe with --work-tree $*" ' > + test $S = 0 && > + case "$R" in > + $expect) echo happy ;; > + *) echo "Oops - $R is not $expect"; > + false ;; > + esac > + ' > + cd "$TRASH_DIRECTORY" > +} > + > test_expect_success setup ' > > test_tick && > @@ -145,14 +163,20 @@ check_describe A-* HEAD > > check_describe "A-*[0-9a-f]" --dirty > > +check_describe_worktree "A-*[0-9a-f]" --dirty > + > test_expect_success 'set-up dirty work tree' ' > echo >>file > ' > > check_describe "A-*[0-9a-f]-dirty" --dirty > > +check_describe_worktree "A-*[0-9a-f]-dirty" --dirty > + > check_describe "A-*[0-9a-f].mod" --dirty=.mod > > +check_describe_worktree "A-*[0-9a-f].mod" --dirty=.mod > + > test_expect_success 'describe --dirty HEAD' ' > test_must_fail git describe --dirty HEAD > ' > -- > 2.20.1 > -- Duy ``` ## Duy Nguyen, 2019-01-27 00:21 Subject: Re: [PATCH v2 2/3] Setup working tree in describe Message-ID: URL: https://gitlist.dev/e/CACsJy8A%2BQsSDfL_RX2ctNsV35VpBaSp4M7hR00nSObmmULUmvg%40mail.gmail.com In-Reply-To: <20190126204951.42455-2-koraktor@gmail.com> ``` On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote: > > This ensures the given working tree is used for --dirty. > > The implementation of --broken uses diff-index which calls > setup_work_tree() itself. It would be nice to have a test case covering --broken even if no fix is needed (so that somebody else will not accidentally break it later). I did a quick test and thought it was broken, but it turns out I tested it wrong :P > Signed-off-by: Sebastian Staudt > --- > builtin/describe.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/builtin/describe.c b/builtin/describe.c > index cc118448ee..b5b7abdc8f 100644 > --- a/builtin/describe.c > +++ b/builtin/describe.c > @@ -629,6 +629,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix) > struct argv_array args = ARGV_ARRAY_INIT; > int fd, result; > > + setup_work_tree(); > read_cache(); > refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, > NULL, NULL, NULL); > -- > 2.20.1 > -- Duy ``` ## Duy Nguyen, 2019-01-27 00:25 Subject: Re: [PATCH v2 3/3] Add test for describe with a bare repository Message-ID: URL: https://gitlist.dev/e/CACsJy8C27hKNTyGE5P%2BF6nzszN_z85chEq8MvQ%2BBgiDAi%3D4jww%40mail.gmail.com In-Reply-To: <20190126204951.42455-3-koraktor@gmail.com> ``` On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote: > > This ensures that nothing breaks the basic functionality of describe for > bare repositories. Please note that --broken and --dirty need a working > tree. > > Signed-off-by: Sebastian Staudt > --- > t/t6120-describe.sh | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh > index 9a6bd1541f..ddd8cc307d 100755 > --- a/t/t6120-describe.sh > +++ b/t/t6120-describe.sh > @@ -409,4 +409,11 @@ test_expect_success 'describe complains about missing object' ' > test_must_fail git describe $ZERO_OID > ' > > +test_expect_success 'describe works from outside repo using --git-dir' " > + BARE_CLONE=$(mktemp -d) && No, keep everything in $TRASH_DIRECTORY so it will be automatically cleaned. > + git clone --bare '$TRASH_DIRECTORY' \$BARE_CLONE >/Users/koraktor/open-source/others/git/t/out && Ehh.. I'm pretty sure I don't have /Users/koraktor on my system :) This looks like just debug code, I think you can drop ">.." part for all commands. > + echo $PWD >/Users/koraktor/open-source/others/git/t/out && > + git --git-dir \$BARE_CLONE describe 2>&1 >/Users/koraktor/open-source/others/git/t/out > +" > + > test_done > -- > 2.20.1 > -- Duy ``` ## Sebastian Staudt, 2019-01-27 06:54 Subject: Re: [PATCH v2 3/3] Add test for describe with a bare repository Message-ID: URL: https://gitlist.dev/e/CA%2BxP2SahEsQKxmjasA5XjuaEVNjbRNiKQv9uKkFkWoORv7WAXQ%40mail.gmail.com In-Reply-To: ``` Am So., 27. Jan. 2019 um 01:25 Uhr schrieb Duy Nguyen : > > On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote: > > > > This ensures that nothing breaks the basic functionality of describe for > > bare repositories. Please note that --broken and --dirty need a working > > tree. > > > > Signed-off-by: Sebastian Staudt > > --- > > t/t6120-describe.sh | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh > > index 9a6bd1541f..ddd8cc307d 100755 > > --- a/t/t6120-describe.sh > > +++ b/t/t6120-describe.sh > > @@ -409,4 +409,11 @@ test_expect_success 'describe complains about missing object' ' > > test_must_fail git describe $ZERO_OID > > ' > > > > +test_expect_success 'describe works from outside repo using --git-dir' " > > + BARE_CLONE=$(mktemp -d) && > > No, keep everything in $TRASH_DIRECTORY so it will be automatically cleaned. Looks like a relic from trying to get Git to not find an appropriate working tree. $TRASH_DIRECTORY/bare works here, too. > > > + git clone --bare '$TRASH_DIRECTORY' \$BARE_CLONE >/Users/koraktor/open-source/others/git/t/out && > > Ehh.. I'm pretty sure I don't have /Users/koraktor on my system :) > This looks like just debug code, I think you can drop ">.." part for > all commands. > Sorry. A bit embarrassing to leave debugging code in place. I will remove the output redirection and echo. > > + echo $PWD >/Users/koraktor/open-source/others/git/t/out && > > + git --git-dir \$BARE_CLONE describe 2>&1 >/Users/koraktor/open-source/others/git/t/out > > +" > > + > > test_done > > -- > > 2.20.1 > > > > > -- > Duy ``` ## Sebastian Staudt, 2019-01-27 07:13 Subject: Re: [PATCH v2 1/3] Add tests for describe with --work-tree Message-ID: URL: https://gitlist.dev/e/CA%2BxP2SbLwmeWpqmjCiqojra5Mwrbok7sjvUsvCsaAo6XsWBbtA%40mail.gmail.com In-Reply-To: ``` Am So., 27. Jan. 2019 um 01:07 Uhr schrieb Duy Nguyen : > > On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote: > > > > The dirty ones are already passing, but just because describe is comparing > > with the wrong working tree. > > > > Signed-off-by: Sebastian Staudt > > --- > > t/t6120-describe.sh | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh > > index d639d94696..9a6bd1541f 100755 > > --- a/t/t6120-describe.sh > > +++ b/t/t6120-describe.sh > > @@ -28,6 +28,24 @@ check_describe () { > > ' > > } > > > > +check_describe_worktree () { > > + cd "$TEST_DIRECTORY" > > Strange alignment. We normally do it in a subshell... Sure, will fix this. > > > + expect="$1" > > + shift > > + R=$(git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe "$@" 2>err.actual) > > These commands should be executed inside test_expect_success, not > outside. And you need to chain commands with && to make sure if > something breaks, then the whole test will fai. > > If it's too ugly to generate test_expect_success with a shell > function, then just write a shell function that "describe" and compare > (i.e. the test body). Then you can write something like this later > > test_expect_sucesss 'describe with --worktree foo' ' > check_describe_worktree foo > ' > > and check_describe_worktree can now do > > ( cd "$TEST_DIRECTORY" && .... ) > > My function is a modified version of check_describe(). Which does the same thing. I‘m not really experienced in Shell programming, so I didn‘t see a cleaner way. But having the cd commands in the && chain looks broken as it would break the following tests when one test fails and the code was executed in the wrong directory afterwards. > > > + S=$? > > + cat err.actual >&3 > > + test_expect_success "describe with --work-tree $*" ' > > + test $S = 0 && > > + case "$R" in > > + $expect) echo happy ;; > > + *) echo "Oops - $R is not $expect"; > > + false ;; > > + esac > > + ' > > + cd "$TRASH_DIRECTORY" > > +} > > + > > test_expect_success setup ' > > > > test_tick && > > @@ -145,14 +163,20 @@ check_describe A-* HEAD > > > > check_describe "A-*[0-9a-f]" --dirty > > > > +check_describe_worktree "A-*[0-9a-f]" --dirty > > + > > test_expect_success 'set-up dirty work tree' ' > > echo >>file > > ' > > > > check_describe "A-*[0-9a-f]-dirty" --dirty > > > > +check_describe_worktree "A-*[0-9a-f]-dirty" --dirty > > + > > check_describe "A-*[0-9a-f].mod" --dirty=.mod > > > > +check_describe_worktree "A-*[0-9a-f].mod" --dirty=.mod > > + > > test_expect_success 'describe --dirty HEAD' ' > > test_must_fail git describe --dirty HEAD > > ' > > -- > > 2.20.1 > > > > > -- > Duy ``` ## Sebastian Staudt, 2019-01-27 07:19 Subject: Re: [PATCH v2 2/3] Setup working tree in describe Message-ID: URL: https://gitlist.dev/e/CA%2BxP2SYk-NtvQf3c6ahwdO-W5zfV6D%2Bh-oQOmkuqee%3DZsEHHTw%40mail.gmail.com In-Reply-To: ``` Am So., 27. Jan. 2019 um 01:22 Uhr schrieb Duy Nguyen : > > On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote: > > > > This ensures the given working tree is used for --dirty. > > > > The implementation of --broken uses diff-index which calls > > setup_work_tree() itself. > > It would be nice to have a test case covering --broken even if no fix > is needed (so that somebody else will not accidentally break it > later). I did a quick test and thought it was broken, but it turns out > I tested it wrong :P > There‘s only one test ("describe ignoring a broken submodule") which effectively tests --broken. I could reuse this. BTW, is "describe ignoring a broken submodule" the right description here? In fact, this should probably be named "describe detects a broken submodule". > > Signed-off-by: Sebastian Staudt > > --- > > builtin/describe.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/builtin/describe.c b/builtin/describe.c > > index cc118448ee..b5b7abdc8f 100644 > > --- a/builtin/describe.c > > +++ b/builtin/describe.c > > @@ -629,6 +629,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix) > > struct argv_array args = ARGV_ARRAY_INIT; > > int fd, result; > > > > + setup_work_tree(); > > read_cache(); > > refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, > > NULL, NULL, NULL); > > -- > > 2.20.1 > > > > > -- > Duy ``` ## Duy Nguyen, 2019-01-28 10:06 Subject: Re: [PATCH v2 1/3] Add tests for describe with --work-tree Message-ID: <20190128100659.GA6257@ash> URL: https://gitlist.dev/e/20190128100659.GA6257%40ash In-Reply-To: ``` On Sun, Jan 27, 2019 at 08:13:51AM +0100, Sebastian Staudt wrote: > Am So., 27. Jan. 2019 um 01:07 Uhr schrieb Duy Nguyen : > > > > On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt wrote: > > > > > > The dirty ones are already passing, but just because describe is comparing > > > with the wrong working tree. > > > > > > Signed-off-by: Sebastian Staudt > > > --- > > > t/t6120-describe.sh | 24 ++++++++++++++++++++++++ > > > 1 file changed, 24 insertions(+) > > > > > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh > > > index d639d94696..9a6bd1541f 100755 > > > --- a/t/t6120-describe.sh > > > +++ b/t/t6120-describe.sh > > > @@ -28,6 +28,24 @@ check_describe () { > > > ' > > > } > > > > > > +check_describe_worktree () { > > > + cd "$TEST_DIRECTORY" > > > > Strange alignment. We normally do it in a subshell... > > Sure, will fix this. > > > > > > + expect="$1" > > > + shift > > > + R=$(git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe "$@" 2>err.actual) > > > > These commands should be executed inside test_expect_success, not > > outside. And you need to chain commands with && to make sure if > > something breaks, then the whole test will fai. > > > > If it's too ugly to generate test_expect_success with a shell > > function, then just write a shell function that "describe" and compare > > (i.e. the test body). Then you can write something like this later > > > > test_expect_sucesss 'describe with --worktree foo' ' > > check_describe_worktree foo > > ' > > > > and check_describe_worktree can now do > > > > ( cd "$TEST_DIRECTORY" && .... ) > > > > > > My function is a modified version of check_describe(). Whoa. That function is 12 years old! I think our style has evolved a bit since then. > Which does the same thing. I‘m not really experienced in Shell > programming, so I didn‘t see a cleaner way. > > But having the cd commands in the && chain looks broken as it would > break the following tests when one test fails and the code was executed > in the wrong directory afterwards. I mean chaining within a test. This is to make sure any failure triggers the test failure (as it should, if some command is expected to fail, we have other ways to catch it). I would start with something simple, not using shell function at all. Something like this as an example (I added run_describe() because that "git" command becomes too long). Have a look at the "do's and don'ts" in t/README too. -- 8< -- diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index d639d94696..646bedf4e9 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -28,6 +28,10 @@ check_describe () { ' } +run_describe() { + git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe "$@" +} + test_expect_success setup ' test_tick && @@ -145,6 +149,14 @@ check_describe A-* HEAD check_describe "A-*[0-9a-f]" --dirty +test_expect_success 'describe with --work-tree --dirty' ' + ( + cd "$TEST_DIRECTORY" && + run_describe --dirty 2>err.actual >actual && + grep "^A-.*[0-9a-f]$" actual + ) +' + test_expect_success 'set-up dirty work tree' ' echo >>file ' -- 8< -- BTW, careful about _success or _failure. You need to make sure bisect is not broken. If you add a test to confirm a broken case then it should be test_expect_failure (and the test suite will pass). Then when you fix it you can flip it to test_expect_success. -- Duy ``` ## Junio C Hamano, 2019-01-30 16:47 Subject: Re: [PATCH v2 1/3] Add tests for describe with --work-tree Message-ID: URL: https://gitlist.dev/e/xmqq5zu65bz5.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20190128100659.GA6257@ash> ``` Duy Nguyen writes: >> My function is a modified version of check_describe(). > > Whoa. That function is 12 years old! I think our style has evolved a > bit since then. ;-). > I mean chaining within a test. This is to make sure any failure > triggers the test failure (as it should, if some command is expected > to fail, we have other ways to catch it). > > I would start with something simple, not using shell function at > all. Something like this as an example (I added run_describe() because > that "git" command becomes too long). Have a look at the "do's and > don'ts" in t/README too. Thanks for guiding new contributors with an easy to understand example. > BTW, careful about _success or _failure. You need to make sure bisect > is not broken. If you add a test to confirm a broken case then it > should be test_expect_failure (and the test suite will pass). Then > when you fix it you can flip it to test_expect_success. And if the fix is simple enough (i.e. a good rule of thumb is if the fixes themselves without tests need to be multi-patch series, it is not simple enough), have a single patch that has both fix and test that expects success, instead of splitting them into two to make a two patch series whose first step expects a failure and whose second step fixes and flips failure to success. ```