Re: [PATCH 2/3] worktree add: stop reading ".git/HEAD"

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The function can_use_local_refs() prints a warning if there are no local
> branches and HEAD is invalid or points to an unborn branch. As part of
> the warning it prints the contents of ".git/HEAD". In a repository using
> the reftable backend HEAD is not stored in the filesystem so reading
> that file is pointless. In a repository using the files backend it is
> unclear how useful printing it is - it would be better to diagnose the
> problem for the user. For now, simplify the warning by not printing
> the file contents and adjust the relevant test case accordingly. Also
> fixup the test case to use test_grep so that anyone trying to debug a
> test failure in the future is not met by a wall of silence.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/worktree.c      | 21 ++-------------------
>  t/t2400-worktree-add.sh | 28 ++++++++++++----------------
>  2 files changed, 14 insertions(+), 35 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index bc2d0d645ba..70410b53df3 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -692,25 +692,8 @@ static int can_use_local_refs(const struct add_opts *opts)
>  	if (refs_head_ref(get_main_ref_store(the_repository), first_valid_ref, NULL)) {
>  		return 1;
>  	} else if (refs_for_each_branch_ref(get_main_ref_store(the_repository), first_valid_ref, NULL)) {
> -		if (!opts->quiet) {
> -			struct strbuf path = STRBUF_INIT;
> -			struct strbuf contents = STRBUF_INIT;
> -			char *wt_gitdir = get_worktree_git_dir(NULL);
> -
> -			strbuf_add_real_path(&path, wt_gitdir);
> -			strbuf_addstr(&path, "/HEAD");
> -			strbuf_read_file(&contents, path.buf, 64);
> -			strbuf_stripspace(&contents, NULL);
> -			strbuf_strip_suffix(&contents, "\n");
> -
> -			warning(_("HEAD points to an invalid (or orphaned) reference.\n"
> -				  "HEAD path: '%s'\n"
> -				  "HEAD contents: '%s'"),
> -				  path.buf, contents.buf);
> -			strbuf_release(&path);
> -			strbuf_release(&contents);
> -			free(wt_gitdir);
> -		}
> +		if (!opts->quiet)
> +				warning(_("HEAD points to an invalid (or orphaned) reference.\n"));

This is indented one level too deep, it seems.

Other than that, I fully agree with the reasoning of the removal
explained in the proposed log message, and the updated test to use
test_grep does look much better.

We seem to use "[ a = b ]" instead of "test a = b" in this test
file, unlike everybody else, which I didn't notice before.  Of
course, this series has no need to touch them; it is just a tangent
I happened to have noticed.

Thanks.

> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index 023e1301c8e..58b4445cc44 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -987,7 +987,7 @@ test_dwim_orphan () {
>  				then
>  					test_must_be_empty actual
>  				else
> -					grep "$info_text" actual
> +					test_grep "$info_text" actual
>  				fi
>  			elif [ "$outcome" = "no_infer" ]
>  			then
> @@ -996,39 +996,35 @@ test_dwim_orphan () {
>  				then
>  					test_must_be_empty actual
>  				else
> -					! grep "$info_text" actual
> +					test_grep ! "$info_text" actual
>  				fi
>  			elif [ "$outcome" = "fetch_error" ]
>  			then
>  				test_must_fail git $dashc_args worktree add $args 2>actual &&
> -				grep "$fetch_error_text" actual
> +				test_grep "$fetch_error_text" actual
>  			elif [ "$outcome" = "fatal_orphan_bad_combo" ]
>  			then
>  				test_must_fail git $dashc_args worktree add $args 2>actual &&
>  				if [ $use_quiet -eq 1 ]
>  				then
> -					! grep "$info_text" actual
> +					test_grep ! "$info_text" actual
>  				else
> -					grep "$info_text" actual
> +					test_grep "$info_text" actual
>  				fi &&
> -				grep "$bad_combo_regex" actual
> +				test_grep "$bad_combo_regex" actual
>  			elif [ "$outcome" = "warn_bad_head" ]
>  			then
>  				test_must_fail git $dashc_args worktree add $args 2>actual &&
>  				if [ $use_quiet -eq 1 ]
>  				then
> -					grep "$invalid_ref_regex" actual &&
> -					! grep "$orphan_hint" actual
> +					test_grep "$invalid_ref_regex" actual &&
> +					test_grep ! "$orphan_hint" actual
>  				else
> -					headpath=$(git $dashc_args rev-parse --path-format=absolute --git-path HEAD) &&
> -					headcontents=$(cat "$headpath") &&
> -					grep "HEAD points to an invalid (or orphaned) reference" actual &&
> -					grep "HEAD path: .$headpath." actual &&
> -					grep "HEAD contents: .$headcontents." actual &&
> -					grep "$orphan_hint" actual &&
> -					! grep "$info_text" actual
> +					test_grep "HEAD points to an invalid (or orphaned) reference" actual &&
> +					test_grep "$orphan_hint" actual &&
> +					test_grep ! "$info_text" actual
>  				fi &&
> -				grep "$invalid_ref_regex" actual
> +				test_grep "$invalid_ref_regex" actual
>  			else
>  				# Unreachable
>  				false