threads / patch / 50377

v4, 2 partsdescribe: setup working tree for --dirty

Subject: [PATCH v4 1/2] describe: setup working tree for --dirty

## tl;dr

8 messages between Feb 1, 2019 and Feb 3, 2019. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Sebastian Staudt· Feb 1, 2019, 13:55 UTC · lore

We don't use NEED_WORK_TREE when running the git-describe builtin, since you should be able to describe a commit even in a bare repository. However, the --dirty flag does need a working tree. Since we don't call setup_work_tree(), it uses whatever directory we happen to be in. That's unlikely to match our index, meaning we'd say "dirty" even when the real working tree is clean.

We can fix that by calling setup_work_tree() once we know that the user has asked for --dirty.

The --broken option also needs a working tree. But because its implementation calls git-diff-index we don‘t have to setup the working tree in the git-describe process.

Signed-off-by: Sebastian Staudt <koraktor@gmail.com>
Helped-by: Jeff King <peff@peff.net>
---
 builtin/describe.c  |  1 +
 t/t6120-describe.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)
Show changes to 2 files +34 −0

builtin/describe.c, t/t6120-describe.sh

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);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index d639d94696..7cfed77c52 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -145,14 +145,38 @@ check_describe A-* HEAD
 
 check_describe "A-*[0-9a-f]" --dirty
 
+test_expect_success 'describe --dirty with --work-tree' '
+	(
+		cd "$TEST_DIRECTORY" &&
+		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out"
+	) &&
+	grep "^A-[1-9][0-9]\?-g[0-9a-f]\+$" out
+'
+
 test_expect_success 'set-up dirty work tree' '
 	echo >>file
 '
 
 check_describe "A-*[0-9a-f]-dirty" --dirty
 
+test_expect_success 'describe --dirty with --work-tree' '
+	(
+		cd "$TEST_DIRECTORY" &&
+		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out"
+	) &&
+	grep "^A-[1-9][0-9]\?-g[0-9a-f]\+-dirty$" out
+'
+
 check_describe "A-*[0-9a-f].mod" --dirty=.mod
 
+test_expect_success 'describe --dirty=.mod with --work-tree' '
+	(
+		cd "$TEST_DIRECTORY" &&
+		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty=.mod >"$TRASH_DIRECTORY/out"
+	) &&
+	grep "^A-[1-9][0-9]\?-g[0-9a-f]\+.mod$" out
+'
+
 test_expect_success 'describe --dirty HEAD' '
 	test_must_fail git describe --dirty HEAD
 '
@@ -303,8 +327,17 @@ test_expect_success 'describe chokes on severely broken submodules' '
 	mv .git/modules/sub1/ .git/modules/sub_moved &&
 	test_must_fail git describe --dirty
 '
+
 test_expect_success 'describe ignoring a broken submodule' '
 	git describe --broken >out &&
+	grep broken out
+'
+
+test_expect_success 'describe with --work-tree ignoring a broken submodule' '
+	(
+		cd "$TEST_DIRECTORY" &&
+		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --broken >"$TRASH_DIRECTORY/out"
+	) &&
 	test_when_finished "mv .git/modules/sub_moved .git/modules/sub1" &&
 	grep broken out
 '

-- 
2.20.1
Sebastian Staudt· Feb 1, 2019, 13:55 UTC · re: Sebastian Staudt · lore

[PATCH v4 2/2] t6120: test for describe with a bare repository

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 <koraktor@gmail.com>
---
 t/t6120-describe.sh | 5 +++++
 1 file changed, 5 insertions(+)
Show changes to t/t6120-describe.sh +5 −0
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 7cfed77c52..ea2c3dbe1c 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -418,4 +418,9 @@ 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' ' 
+  git clone --bare "$TRASH_DIRECTORY" "$TRASH_DIRECTORY/bare" &&
+  git --git-dir "$TRASH_DIRECTORY/bare" describe
+'
+
 test_done

-- 
2.20.1
Junio C Hamano· Feb 1, 2019, 18:53 UTC · re: Sebastian Staudt · lore

Re: [PATCH v4 2/2] t6120: test for describe with a bare repository

Sebastian Staudt <koraktor@gmail.com> writes:
Show 21 quoted lines
> 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 <koraktor@gmail.com>
> ---
>  t/t6120-describe.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index 7cfed77c52..ea2c3dbe1c 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -418,4 +418,9 @@ 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' ' 
> +  git clone --bare "$TRASH_DIRECTORY" "$TRASH_DIRECTORY/bare" &&
> +  git --git-dir "$TRASH_DIRECTORY/bare" describe
> +'

OK, it demonstrates that the command exits with status 0. Do we want to validate its output, too?

Sebastian Staudt· Feb 2, 2019, 10:00 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 2/2] t6120: test for describe with a bare repository

Am Fr., 1. Feb. 2019 um 19:53 Uhr schrieb Junio C Hamano <gitster@pobox.com>:
Show 27 quoted lines
>
> Sebastian Staudt <koraktor@gmail.com> writes:
>
> > 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 <koraktor@gmail.com>
> > ---
> >  t/t6120-describe.sh | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> > index 7cfed77c52..ea2c3dbe1c 100755
> > --- a/t/t6120-describe.sh
> > +++ b/t/t6120-describe.sh
> > @@ -418,4 +418,9 @@ 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' '
> > +  git clone --bare "$TRASH_DIRECTORY" "$TRASH_DIRECTORY/bare" &&
> > +  git --git-dir "$TRASH_DIRECTORY/bare" describe
> > +'
>
> OK, it demonstrates that the command exits with status 0.  Do we
> want to validate its output, too?

It won‘t hurt. I‘ll move this test further up, so it is executed when the working tree is still clean. That way we can use the same regex as for the other checks.

Thanks.
Eric Sunshine· Feb 1, 2019, 20:12 UTC · re: Sebastian Staudt · lore

Re: [PATCH v4 1/2] describe: setup working tree for --dirty

On Fri, Feb 1, 2019 at 8:55 AM Sebastian Staudt <koraktor@gmail.com> wrote:
Show 18 quoted lines
> We don't use NEED_WORK_TREE when running the git-describe builtin,
> since you should be able to describe a commit even in a bare repository.
> However, the --dirty flag does need a working tree. Since we don't call
> setup_work_tree(), it uses whatever directory we happen to be in. That's
> unlikely to match our index, meaning we'd say "dirty" even when the real
> working tree is clean.
> [...]
> Signed-off-by: Sebastian Staudt <koraktor@gmail.com>
> ---
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> @@ -145,14 +145,38 @@ check_describe A-* HEAD
> +test_expect_success 'describe --dirty with --work-tree' '
> +       [...]
> +'
>
> +test_expect_success 'describe --dirty with --work-tree' '
> +       [...]
> +'

Can you give these two new tests different titles to make it easier to narrow down a problem to one or the other if one of them does fail? Perhaps the second test could be titled:

    test_expect_success 'describe --dirty with dirty --work-tree' '
or something.
Sebastian Staudt· Feb 2, 2019, 10:04 UTC · re: Eric Sunshine · lore

Re: [PATCH v4 1/2] describe: setup working tree for --dirty

Am Fr., 1. Feb. 2019 um 21:12 Uhr schrieb Eric Sunshine <sunshine@sunshineco.com>:

Show 28 quoted lines
>
> On Fri, Feb 1, 2019 at 8:55 AM Sebastian Staudt <koraktor@gmail.com> wrote:
> > We don't use NEED_WORK_TREE when running the git-describe builtin,
> > since you should be able to describe a commit even in a bare repository.
> > However, the --dirty flag does need a working tree. Since we don't call
> > setup_work_tree(), it uses whatever directory we happen to be in. That's
> > unlikely to match our index, meaning we'd say "dirty" even when the real
> > working tree is clean.
> > [...]
> > Signed-off-by: Sebastian Staudt <koraktor@gmail.com>
> > ---
> > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> > @@ -145,14 +145,38 @@ check_describe A-* HEAD
> > +test_expect_success 'describe --dirty with --work-tree' '
> > +       [...]
> > +'
> >
> > +test_expect_success 'describe --dirty with --work-tree' '
> > +       [...]
> > +'
>
> Can you give these two new tests different titles to make it easier to
> narrow down a problem to one or the other if one of them does fail?
> Perhaps the second test could be titled:
>
>     test_expect_success 'describe --dirty with dirty --work-tree' '
>
> or something.

Thanks, didn‘t notice this. I‘d use a suffix (dirty) for my test titles. But this won‘t work for tests using check_describe(). Any objections?

Eric Sunshine· Feb 3, 2019, 03:35 UTC · re: Sebastian Staudt · lore

Re: [PATCH v4 1/2] describe: setup working tree for --dirty

On Sat, Feb 2, 2019 at 5:05 AM Sebastian Staudt <koraktor@gmail.com> wrote:
Show 23 quoted lines
> Am Fr., 1. Feb. 2019 um 21:12 Uhr schrieb Eric Sunshine
> <sunshine@sunshineco.com>:
> > On Fri, Feb 1, 2019 at 8:55 AM Sebastian Staudt <koraktor@gmail.com> wrote:
> > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> > > @@ -145,14 +145,38 @@ check_describe A-* HEAD
> > > +test_expect_success 'describe --dirty with --work-tree' '
> > > +       [...]
> > > +'
> > > +test_expect_success 'describe --dirty with --work-tree' '
> > > +       [...]
> > > +'
> >
> > Can you give these two new tests different titles to make it easier to
> > narrow down a problem to one or the other if one of them does fail?
> > Perhaps the second test could be titled:
> >
> >     test_expect_success 'describe --dirty with dirty --work-tree' '
> >
> > or something.
>
> Thanks, didn‘t notice this.
> I‘d use a suffix (dirty) for my test titles. But this won‘t work for tests
> using check_describe(). Any objections?
I have no objections to using suffix "(dirty)".

It's true that there are a few duplicate test titles due to check_describe() invocations, however, this patch isn't introducing any new callers, so it's not strictly a concern of this series. If you did want to address that issue, one possibility would be to add a 'title' argument to check_describe() and adjust callers to use a unique title, however, such a change would not be part of this particular patch, and I'm not convinced that it's even worth the effort and churn at this point.

Junio C Hamano· Feb 1, 2019, 20:52 UTC · re: Sebastian Staudt · lore

Re: [PATCH v4 1/2] describe: setup working tree for --dirty

Sebastian Staudt <koraktor@gmail.com> writes:
Show 13 quoted lines
> We don't use NEED_WORK_TREE when running the git-describe builtin,
> since you should be able to describe a commit even in a bare repository.
> However, the --dirty flag does need a working tree. Since we don't call
> setup_work_tree(), it uses whatever directory we happen to be in. That's
> unlikely to match our index, meaning we'd say "dirty" even when the real
> working tree is clean.
>
> We can fix that by calling setup_work_tree() once we know that the user
> has asked for --dirty.
>
> The --broken option also needs a working tree. But because its
> implementation calls git-diff-index we don‘t have to setup the working
> tree in the git-describe process.
Very nicely described.
Show 20 quoted lines
>
> Signed-off-by: Sebastian Staudt <koraktor@gmail.com>
> Helped-by: Jeff King <peff@peff.net>
> ---
>  builtin/describe.c  |  1 +
>  t/t6120-describe.sh | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
>
> 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);

And the implementation is as promised in the proposed log message. Quite straight-forward and obviously right ;-)

Show 16 quoted lines
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index d639d94696..7cfed77c52 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -145,14 +145,38 @@ check_describe A-* HEAD
>  
>  check_describe "A-*[0-9a-f]" --dirty
>  
> +test_expect_success 'describe --dirty with --work-tree' '
> +	(
> +		cd "$TEST_DIRECTORY" &&
> +		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out"
> +	) &&
> +	grep "^A-[1-9][0-9]\?-g[0-9a-f]\+$" out
> +'
> +

Without the fix to the code, this test piece fails with "-dirty" suffix in 'out'. Good.

Show 7 quoted lines
> +test_expect_success 'describe --dirty with --work-tree' '
> +	(
> +		cd "$TEST_DIRECTORY" &&
> +		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out"
> +	) &&
> +	grep "^A-[1-9][0-9]\?-g[0-9a-f]\+-dirty$" out
> +'

This succeeds with or without the fix to the code; the buggy behaviour was to claim "-dirty"-ness when the working tree files are clean. This new test is not about that buggy behaviour. It is rather about the updated code does not mistakenly claim cleanness in a dirty working tree. Good.

Show 10 quoted lines
>  check_describe "A-*[0-9a-f].mod" --dirty=.mod
>  
> +test_expect_success 'describe --dirty=.mod with --work-tree' '
> +	(
> +		cd "$TEST_DIRECTORY" &&
> +		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty=.mod >"$TRASH_DIRECTORY/out"
> +	) &&
> +	grep "^A-[1-9][0-9]\?-g[0-9a-f]\+.mod$" out
> +'
> +
Likewise.
Show 18 quoted lines
>  test_expect_success 'describe --dirty HEAD' '
>  	test_must_fail git describe --dirty HEAD
>  '
> @@ -303,8 +327,17 @@ test_expect_success 'describe chokes on severely broken submodules' '
>  	mv .git/modules/sub1/ .git/modules/sub_moved &&
>  	test_must_fail git describe --dirty
>  '
> +
>  test_expect_success 'describe ignoring a broken submodule' '
>  	git describe --broken >out &&
> +	grep broken out
> +'
> +
> +test_expect_success 'describe with --work-tree ignoring a broken submodule' '
> +	(
> +		cd "$TEST_DIRECTORY" &&
> +		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --broken >"$TRASH_DIRECTORY/out"
> +	) &&

OK, this checks the same repository as the existing test does, but does so from outside the repository.

Looks good.
>  	test_when_finished "mv .git/modules/sub_moved .git/modules/sub1" &&
>  	grep broken out
>  '

← back to recent threads