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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2019, 20:52 UTC
Message-ID: <xmqqzhrfxmco.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190201135512.68220-1-koraktor@gmail.com>

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
>  '