public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed
From: "Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail•com>
To: "Gabriel Scherer" <gabriel.scherer@inria•fr>, git@vger•kernel.org
Cc: "Junio C Hamano" <gitster@pobox•com>,
	"D. Ben Knoble" <ben.knoble@gmail•com>,
	"Phillip Wood" <phillip.wood@dunelm•org.uk>
Subject: Re: [PATCH 1/3] checkout: provide hint when failing due to another worktree
Date: Sat, 13 Sep 2025 22:55:01 +0200	[thread overview]
Message-ID: <be510685-3be1-4f71-806a-6b580bb1cf21@app.fastmail.com> (raw)
In-Reply-To: <20250913141327.2775228-2-gabriel.scherer@inria.fr>

Unrelated: I found it confusing that my `co = checkout` alias did not
work with this fresh-off-the-press Advice:[1]

```
$ ./git co master
fatal: 'master' is already used by worktree at '<path>'
$ ./git checkout master
fatal: 'master' is already used by worktree at '<patch>'
hint: Use --detach to avoid this restriction,
hint: or --ignore-other-worktrees to ignore it.
hint: Disable this message with "git config set advice.branchUsedInOtherWorktree false"
```

But it did for this older Advice (which is in my installed git(1)):

```
$ ./git co -b .. @
fatal: '..' is not a valid branch name
hint: See `man git check-ref-format`
hint: Disable this message with "git config set advice.refSyntax false"
```

It’s because aliases are run as a subprocess from the `git` in `PATH`:

```
strvec_push(&cmd.args, "git");
```

[1]: Chain of events:

1. Try to trigger the Advice in this series
2. ... but it doesn’t
3. Is the code wrong?
4. Wait, I’m using my alias (which I always use; I don’t think about it)
5. I test with `git checkout`: it works
6. ... so aliases don’t work with Advice?
7. Test an existing Advice that I know about
8. ... but it does work with aliases
9.–15. ...

It was part of the process.  I didn’t *decide* to get hung up on it. ;)

On Sat, Sep 13, 2025, at 16:13, Gabriel Scherer wrote:
> From: "Gabriel.Scherer" <gabriel.scherer@inria•fr>
>
> When checkout/switch fails because the target branch is already used
> by another worktree, we now hint that the user could use --detach or
> --ignore-other-worktrees.

The commit message is supposed to discuss what the code does without the
patch in the present tense.  What the patch does is in the imperative
mood.  Refer to `Documentation/SubmittingPatches`, “imperative mood”.

Maybe there should be a paragraph which motivates the need for an Advice
and these two in particular?  You could also concievably advise removing
the worktree. :)

> Note: this error can also happen on rebase, which unfortunately does
> not support --ignore-other-worktrees. We do not show advice in this
> case, and leave 'rebase --ignore-other-worktrees' to future work.

After reading this I thought this was not handled by this patch series.
But you add the this option to git-rebase(1) in the second patch.

“Future work” suggests to me something ranging from:

• there is another patch series that deal with this; or
• there are no plans to do this (but it could be done in theory).

>
> Signed-off-by: Gabriel Scherer <gabriel.scherer@inria•fr>
> ---
>  Documentation/config/advice.adoc |  3 +++
>  advice.c                         |  1 +
>  advice.h                         |  1 +
>  branch.c                         | 13 +++++++++++--
>  branch.h                         |  4 ++++
>  builtin/checkout.c               | 12 ++++++++++--
>  6 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/config/advice.adoc
> b/Documentation/config/advice.adoc
> index 257db58918..9ee64f44ea 100644
> --- a/Documentation/config/advice.adoc
> +++ b/Documentation/config/advice.adoc
> @@ -27,6 +27,9 @@ all advice messages.
>  		Shown when a fetch refspec for multiple remotes maps to
>  		the same remote-tracking branch namespace and causes branch
>  		tracking set-up to fail.
> +	branchUsedInOtherWorktree::
> +		Shown when the user attemps to switch to a branch
> +		that is already checked out in another worktree.

This maintains sort-order which is good.  It is also consistent in all
points with 95c987e6fad (advice: make all entries stylistically
consistent, 2024-03-05) for what it’s worth.

I also think the wording/formulation is great.

>  	checkoutAmbiguousRemoteBranchName::
>  		Shown when the argument to
>  		linkgit:git-checkout[1] and linkgit:git-switch[1]
> diff --git a/advice.c b/advice.c
> index e5f0ff8449..5c9b763472 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -50,6 +50,7 @@ static struct {
>  	[ADVICE_AMBIGUOUS_FETCH_REFSPEC]		= { "ambiguousFetchRefspec" },
>  	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir" },
>  	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName" },
> +	[ADVICE_BRANCH_USED_IN_OTHER_WORKTREE]		= { "branchUsedInOtherWorktree" },

This too looks like it should be in sort order.  If so you are inserting
at the wrong place.

>  	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge" },
>  	[ADVICE_DEFAULT_BRANCH_NAME]			= { "defaultBranchName" },
>  	[ADVICE_DETACHED_HEAD]				= { "detachedHead" },
> diff --git a/advice.h b/advice.h
> index 727dcecf4a..6b11df945b 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -16,6 +16,7 @@ enum advice_type {
>  	ADVICE_ADD_IGNORED_FILE,
>  	ADVICE_AMBIGUOUS_FETCH_REFSPEC,
>  	ADVICE_AM_WORK_DIR,
> +	ADVICE_BRANCH_USED_IN_OTHER_WORKTREE,

Correct sort order (if there is one).

>  	ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
>  	ADVICE_COMMIT_BEFORE_MERGE,
>  	ADVICE_DEFAULT_BRANCH_NAME,
> diff --git a/branch.c b/branch.c
> index 26be358347..76aa2cbf44 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -844,7 +844,7 @@ void remove_branch_state(struct repository *r, int
> verbose)
>  	remove_merge_branch_state(r);
>  }
>[snip diff]
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index f9453473fe..e4b78f4a05 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1582,8 +1582,16 @@ static void
> die_if_switching_to_a_branch_in_use(struct checkout_opts *opts,
>  		return;
>  	head_ref = refs_resolve_refdup(get_main_ref_store(the_repository),
>  				       "HEAD", 0, NULL, &flags);
> -	if (head_ref && (!(flags & REF_ISSYMREF) || strcmp(head_ref,
> full_ref)))
> -		die_if_checked_out(full_ref, 1);
> +	if (head_ref && (!(flags & REF_ISSYMREF) || strcmp(head_ref,
> full_ref))) {
> +		int code = die_message_if_checked_out(full_ref, 1);
> +		if (code) {
> +			advise_if_enabled(
> +				ADVICE_BRANCH_USED_IN_OTHER_WORKTREE,
> +				_("Use --detach to avoid this restriction,\n"
> +				"or --ignore-other-worktrees to ignore it."));

I don’t know if `--detach` will “avoid” the restriction.  (In fact
`--ignore-other-worktrees` might be the one that *avoids* it (turns it
off)?)

Technically the only point of being-on-a-branch is to be able to advance
it.  You know that.  But does the advice-receiver?  Because they might
use the hint to get what they want immediately.  Then later wonder why
all the work they did on the branch “had no effect”.

> +			exit(code);
> +		}
> +	}
>  	free(head_ref);
>  }
>
> --
> 2.51.0

No updates to tests were covered in another email.

-- 
Kristoffer Haugsbakk

  reply	other threads:[~2025-09-13 20:56 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-09-13 14:13 [PATCH 0/3] extend --ignore-other-worktrees to 'rebase', add hints Gabriel Scherer
2025-09-13 14:13 ` [PATCH 1/3] checkout: provide hint when failing due to another worktree Gabriel Scherer
2025-09-13 20:55   ` Kristoffer Haugsbakk [this message]
2025-09-14  7:50     ` Gabriel Scherer
2025-09-15  8:53       ` Junio C Hamano
2025-09-15 19:52         ` Gabriel Scherer
2025-09-16  5:32           ` Junio C Hamano
2025-09-17 14:17           ` Junio C Hamano
2025-09-17 15:25           ` Gabriel Scherer
2025-09-19 14:13             ` Phillip Wood
2025-09-14 19:03     ` Ben Knoble
2025-09-14 19:06       ` Ben Knoble
2025-09-14 19:32       ` Kristoffer Haugsbakk
2025-09-13 14:13 ` [PATCH 2/3] rebase: support --ignore-other-worktrees Gabriel Scherer
2025-09-15 10:09   ` Phillip Wood
2025-09-15 16:23     ` Junio C Hamano
2025-09-13 14:13 ` [PATCH 3/3] rebase: hint when failing on branch used by another worktree Gabriel Scherer
2025-09-13 19:30 ` [PATCH 0/3] extend --ignore-other-worktrees to 'rebase', add hints Ben Knoble
2025-09-13 20:02   ` Gabriel Scherer
2025-09-15 10:09 ` Phillip Wood

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=be510685-3be1-4f71-806a-6b580bb1cf21@app.fastmail.com \
    --to=kristofferhaugsbakk@fastmail$(echo .)com \
    --cc=ben.knoble@gmail$(echo .)com \
    --cc=gabriel.scherer@inria$(echo .)fr \
    --cc=git@vger$(echo .)kernel.org \
    --cc=gitster@pobox$(echo .)com \
    --cc=phillip.wood@dunelm$(echo .)org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox