Re: [PATCH v4 3/3] worktree add: Add hint to use --orphan when bad ref

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Dec 12 2022, Jacob Abel wrote:

>  int git_default_advice_config(const char *var, const char *value);
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 51b247b2a7..ea306e18de 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -744,6 +744,14 @@ static int add(int ac, const char **av, const char *prefix)
>  		 * If `branch` does not reference a valid commit, a new
>  		 * worktree (and/or branch) cannot be created based off of it.
>  		 */
> +		advise_if_enabled(ADVICE_WORKTREE_ADD_ORPHAN,
> +			"If you meant to create a worktree containing a new orphan branch\n"
> +			"(branch with no commits) for this repository, e.g. '%s',\n"

I think this '%s' is just confusing, how is repeating the name of the
branch at the user (which we're about to mention in the example command)
helpful?

Shouldn't we just omit it, or reword this to e.g.:

	If you'd like the '%s' branch to be a worktree containing a
	new....


> +			"you can do so using the --orphan option:\n"
> +			"\n"
> +			"	git worktree add --orphan %s %s\n"
> +			"\n",

Missing the usual:

	"Turn this message off by running\n"
	"\"git config advice.worktreeAddOrphan false\""

blurb.

Also, should we really add twe newlines at the end? I see some other API
users that don't add a \n at all.

> +# Helper function to test hints for using --orphan in an empty repo.

FWIW I think we can do without the comment...

> +test_wt_add_empty_repo_orphan_hint() {
> +	local context="$1" &&
> +	local opts="${@:2}" &&

This appears to be some shell pseudo-syntax, and my shell hard-errors on
this.

But as we don't "shift" after the "$1" I don't see how what you
*probably* meant could work, i.e. we always have arguments, so surely
we'd always use "$@"?


> +	test_expect_success "'worktree add' show orphan hint in empty repo w/ $context" '
> +		test_when_finished "rm -rf empty_repo" &&
> +		GIT_DIR="empty_repo" git init --bare &&
> +		test_must_fail git -C empty_repo worktree add $opts 2> actual &&
> +		grep "hint: If you meant to create a worktree containing a new orphan branch" actual
> +	'
> +}
> +
> +test_wt_add_empty_repo_orphan_hint 'DWIM' foobar/
> +test_wt_add_empty_repo_orphan_hint '-b' -b foobar_branch foobar/
> +test_wt_add_empty_repo_orphan_hint '-B' -B foobar_branch foobar/

You're just testing how these options interact, so let's have the
"foobar" part provided by the test function itself.