[PATCH] branch: strip refs/heads/ from the --set-upstream-to hint

[PATCH] branch: strip refs/heads/ from the --set-upstream-to hint

[tsukasa-ino via GitGitGadget]

From: tsukasa inoue <inoue.tsukasa.4@gmail.com>

When git switch fails to write the tracking config, it prints a hint
with the command to set the upstream by hand. For example, if
.git/config.lock exists:

    error: could not lock config file .git/config: File exists
    error: unable to write upstream branch configuration
    hint:
    hint: After fixing the error cause you may try to fix up
    hint: the remote tracking information by invoking:
    hint:   git branch --set-upstream-to=origin/refs/heads/feat

The hint says origin/refs/heads/feat, but the correct name is
origin/feat. Running the command as printed fails, because the full
ref name is not a valid remote-tracking branch name here.

This has been wrong since a3f40ec4b0 (branch: accept multiple upstream
branches for tracking, 2021-12-20).

Strip refs/heads/ from the hint, as the success message in the same
function already does. The git config --add line for multiple upstreams
still needs the full ref name, so it is not changed.

Signed-off-by: tsukasa inoue <inoue.tsukasa.4@gmail.com>
---
    branch: strip refs/heads/ from the --set-upstream-to hint

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2268%2FtsukasaI%2Ffix%2Fbranch-upstream-hint-shortname-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2268/tsukasaI/fix/branch-upstream-hint-shortname-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2268

 branch.c          |  8 +++++---
 t/t3200-branch.sh | 15 +++++++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/branch.c b/branch.c
index 22f4f46b96..bb07846911 100644
--- a/branch.c
+++ b/branch.c
@@ -186,12 +186,14 @@ out_err:
 
 	advise(_("\nAfter fixing the error cause you may try to fix up\n"
 		"the remote tracking information by invoking:"));
-	if (remotes->nr == 1)
+	if (remotes->nr == 1) {
+		shortname = remotes->items[0].string;
+		skip_prefix(shortname, "refs/heads/", &shortname);
 		advise("  git branch --set-upstream-to=%s%s%s",
 			origin ? origin : "",
 			origin ? "/" : "",
-			remotes->items[0].string);
-	else {
+			shortname);
+	} else {
 		advise("  git config --add branch.\"%s\".remote %s",
 			local, origin ? origin : ".");
 		for_each_string_list_item(item, remotes)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index cdb6c6a634..ce478d85cb 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1080,6 +1080,21 @@ test_expect_success '--set-upstream-to fails on locked config' '
 	test_grep "could not lock config file .git/config" err
 '
 
+test_expect_success '--set-upstream-to hint uses short remote-tracking branch name on locked config' '
+	test_when_finished "git branch -D hint-local" &&
+	test_when_finished "git update-ref -d refs/remotes/hintremote/hintbranch" &&
+	test_config remote.hintremote.url . &&
+	test_config remote.hintremote.fetch "+refs/heads/*:refs/remotes/hintremote/*" &&
+	git update-ref refs/remotes/hintremote/hintbranch HEAD &&
+	git branch hint-local &&
+	test_when_finished "rm -f .git/config.lock" &&
+	>.git/config.lock &&
+	test_must_fail git -c core.configLockTimeout=0 \
+		branch --set-upstream-to=hintremote/hintbranch hint-local 2>err &&
+	test_grep "git branch --set-upstream-to=hintremote/hintbranch\$" err &&
+	test_grep ! "refs/heads/" err
+'
+
 test_expect_success 'use --set-upstream-to modify HEAD' '
 	test_config branch.main.remote foo &&
 	test_config branch.main.merge foo &&

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
-- 
gitgitgadget

Re: [PATCH] branch: strip refs/heads/ from the --set-upstream-to hint

[Junio C Hamano]

"tsukasa-ino via GitGitGadget" <gitgitgadget@gmail.com> writes:

>     hint:   git branch --set-upstream-to=origin/refs/heads/feat
>
> The hint says origin/refs/heads/feat, but the correct name is
> origin/feat. Running the command as printed fails, because the full
> ref name is not a valid remote-tracking branch name here.

Good observation.  Thanks for noticing.

> +test_expect_success '--set-upstream-to hint uses short remote-tracking branch name on locked config' '
> +	test_when_finished "git branch -D hint-local" &&
> +	test_when_finished "git update-ref -d refs/remotes/hintremote/hintbranch" &&
> +	test_config remote.hintremote.url . &&
> +	test_config remote.hintremote.fetch "+refs/heads/*:refs/remotes/hintremote/*" &&
> +	git update-ref refs/remotes/hintremote/hintbranch HEAD &&
> +	git branch hint-local &&
> +	test_when_finished "rm -f .git/config.lock" &&
> +	>.git/config.lock &&
> +	test_must_fail git -c core.configLockTimeout=0 \
> +		branch --set-upstream-to=hintremote/hintbranch hint-local 2>err &&
> +	test_grep "git branch --set-upstream-to=hintremote/hintbranch\$" err &&
> +	test_grep ! "refs/heads/" err

Perhaps it makes sense to also test that the suggested command line
is accepted after all of the above?

> +'

Thanks.

Re: [PATCH] branch: strip refs/heads/ from the --set-upstream-to hint

[Harald Nordgren]

> When git switch fails to write the tracking config, it prints a hint
> with the command to set the upstream by hand. For example, if
> .git/config.lock exists:
>
>     error: could not lock config file .git/config: File exists
>     error: unable to write upstream branch configuration
>     hint:
>     hint: After fixing the error cause you may try to fix up
>     hint: the remote tracking information by invoking:
>     hint:   git branch --set-upstream-to=origin/refs/heads/feat
>
> The hint says origin/refs/heads/feat, but the correct name is
> origin/feat. Running the command as printed fails, because the full
> ref name is not a valid remote-tracking branch name here.

Very nice find!

> +test_expect_success '--set-upstream-to hint uses short remote-tracking branch name on locked config' '
> +       test_when_finished "git branch -D hint-local" &&
> +       test_when_finished "git update-ref -d refs/remotes/hintremote/hintbranch" &&
> +       test_config remote.hintremote.url . &&
> +       test_config remote.hintremote.fetch "+refs/heads/*:refs/remotes/hintremote/*" &&
> +       git update-ref refs/remotes/hintremote/hintbranch HEAD &&
> +       git branch hint-local &&
> +       test_when_finished "rm -f .git/config.lock" &&
> +       >.git/config.lock &&
> +       test_must_fail git -c core.configLockTimeout=0 \
> +               branch --set-upstream-to=hintremote/hintbranch hint-local 2>err &&
> +       test_grep "git branch --set-upstream-to=hintremote/hintbranch\$" err &&
> +       test_grep ! "refs/heads/" err
> +'

Would be good to test the full multiline hint message. After all, it's
the point of the topic.

Right now, you are asserting that the correct form
'hintremote/hintbranch' works, but we want to make sure that the hint
actually gives that.


Harald

[PATCH v2 0/3] checkout: finish switch when tracking config cannot be written

[tsukasa-ino via GitGitGadget]

When the tracking config cannot be written (for example, because
.git/config.lock exists), git switch -c and checkout -b stop halfway. The
worktree, the index and the new branch ref are already updated, but HEAD
stays on the old branch, and the changes are left staged. The hint printed
at that point was also wrong: it suggested origin/refs/heads/.

1/3 strips refs/heads/ from the hint. 2/3 makes setup_tracking() return an
error instead of calling exit(). No behavior change yet. 3/3 makes switch
and checkout finish the switch and exit with status 0, as push -u, clone and
fetch already do. git branch still fails.

Changes since v1:

 * 1/3: the test now compares the whole hint message, as suggested by Harald
   Nordgren. It also checks that the command in the hint works once the lock
   is removed, as suggested by Junio
   (https://lore.kernel.org/git/xmqq7bjcup46.fsf@gitster.g/).
 * 2/3 and 3/3 are new. They implement the direction agreed on the bug
   report thread (https://lore.kernel.org/git/xmqqecdjsryk.fsf@gitster.g/).

Making the switch fully atomic, so that a failure undoes the worktree, the
index, the ref and the config, is left for a separate topic.

tsukasa inoue (3):
  branch: strip refs/heads/ from the --set-upstream-to hint
  branch: return an error from setup_tracking()
  checkout: finish switching when the tracking config cannot be written

 branch.c                  | 43 +++++++++++++++++++++++----------------
 branch.h                  | 21 +++++++++++--------
 builtin/branch.c          | 17 +++++++++++-----
 builtin/checkout.c        |  6 ++++++
 t/t2027-checkout-track.sh | 20 ++++++++++++++++++
 t/t2060-switch.sh         | 20 ++++++++++++++++++
 t/t3200-branch.sh         | 38 ++++++++++++++++++++++++++++++++++
 7 files changed, 135 insertions(+), 30 deletions(-)


base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2268%2FtsukasaI%2Ffix%2Fbranch-upstream-hint-shortname-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2268/tsukasaI/fix/branch-upstream-hint-shortname-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2268

Range-diff vs v1:

 1:  14eac9118e ! 1:  e705918378 branch: strip refs/heads/ from the --set-upstream-to hint
     @@ t/t3200-branch.sh: test_expect_success '--set-upstream-to fails on locked config
      +	>.git/config.lock &&
      +	test_must_fail git -c core.configLockTimeout=0 \
      +		branch --set-upstream-to=hintremote/hintbranch hint-local 2>err &&
     -+	test_grep "git branch --set-upstream-to=hintremote/hintbranch\$" err &&
     -+	test_grep ! "refs/heads/" err
     ++	cat >expect <<-\EOF &&
     ++	hint:
     ++	hint: After fixing the error cause you may try to fix up
     ++	hint: the remote tracking information by invoking:
     ++	hint:   git branch --set-upstream-to=hintremote/hintbranch
     ++	EOF
     ++	grep "^hint:" err >actual &&
     ++	test_cmp expect actual &&
     ++	rm -f .git/config.lock &&
     ++	git branch --set-upstream-to=hintremote/hintbranch hint-local &&
     ++	test_cmp_config hintremote branch.hint-local.remote &&
     ++	test_cmp_config refs/heads/hintbranch branch.hint-local.merge
      +'
      +
       test_expect_success 'use --set-upstream-to modify HEAD' '
 -:  ---------- > 2:  4d2cbaf170 branch: return an error from setup_tracking()
 -:  ---------- > 3:  be94ac75bf checkout: finish switching when the tracking config cannot be written

-- 
gitgitgadget

[PATCH v2 1/3] branch: strip refs/heads/ from the --set-upstream-to hint

[tsukasa inoue via GitGitGadget]

From: tsukasa inoue <inoue.tsukasa.4@gmail.com>

When git switch fails to write the tracking config, it prints a hint
with the command to set the upstream by hand. For example, if
.git/config.lock exists:

    error: could not lock config file .git/config: File exists
    error: unable to write upstream branch configuration
    hint:
    hint: After fixing the error cause you may try to fix up
    hint: the remote tracking information by invoking:
    hint:   git branch --set-upstream-to=origin/refs/heads/feat

The hint says origin/refs/heads/feat, but the correct name is
origin/feat. Running the command as printed fails, because the full
ref name is not a valid remote-tracking branch name here.

This has been wrong since a3f40ec4b0 (branch: accept multiple upstream
branches for tracking, 2021-12-20).

Strip refs/heads/ from the hint, as the success message in the same
function already does. The git config --add line for multiple upstreams
still needs the full ref name, so it is not changed.

Signed-off-by: tsukasa inoue <inoue.tsukasa.4@gmail.com>
---
 branch.c          |  8 +++++---
 t/t3200-branch.sh | 25 +++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/branch.c b/branch.c
index 22f4f46b96..bb07846911 100644
--- a/branch.c
+++ b/branch.c
@@ -186,12 +186,14 @@ out_err:
 
 	advise(_("\nAfter fixing the error cause you may try to fix up\n"
 		"the remote tracking information by invoking:"));
-	if (remotes->nr == 1)
+	if (remotes->nr == 1) {
+		shortname = remotes->items[0].string;
+		skip_prefix(shortname, "refs/heads/", &shortname);
 		advise("  git branch --set-upstream-to=%s%s%s",
 			origin ? origin : "",
 			origin ? "/" : "",
-			remotes->items[0].string);
-	else {
+			shortname);
+	} else {
 		advise("  git config --add branch.\"%s\".remote %s",
 			local, origin ? origin : ".");
 		for_each_string_list_item(item, remotes)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index cdb6c6a634..ccfb53efa3 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1080,6 +1080,31 @@ test_expect_success '--set-upstream-to fails on locked config' '
 	test_grep "could not lock config file .git/config" err
 '
 
+test_expect_success '--set-upstream-to hint uses short remote-tracking branch name on locked config' '
+	test_when_finished "git branch -D hint-local" &&
+	test_when_finished "git update-ref -d refs/remotes/hintremote/hintbranch" &&
+	test_config remote.hintremote.url . &&
+	test_config remote.hintremote.fetch "+refs/heads/*:refs/remotes/hintremote/*" &&
+	git update-ref refs/remotes/hintremote/hintbranch HEAD &&
+	git branch hint-local &&
+	test_when_finished "rm -f .git/config.lock" &&
+	>.git/config.lock &&
+	test_must_fail git -c core.configLockTimeout=0 \
+		branch --set-upstream-to=hintremote/hintbranch hint-local 2>err &&
+	cat >expect <<-\EOF &&
+	hint:
+	hint: After fixing the error cause you may try to fix up
+	hint: the remote tracking information by invoking:
+	hint:   git branch --set-upstream-to=hintremote/hintbranch
+	EOF
+	grep "^hint:" err >actual &&
+	test_cmp expect actual &&
+	rm -f .git/config.lock &&
+	git branch --set-upstream-to=hintremote/hintbranch hint-local &&
+	test_cmp_config hintremote branch.hint-local.remote &&
+	test_cmp_config refs/heads/hintbranch branch.hint-local.merge
+'
+
 test_expect_success 'use --set-upstream-to modify HEAD' '
 	test_config branch.main.remote foo &&
 	test_config branch.main.merge foo &&
-- 
gitgitgadget

[PATCH v2 2/3] branch: return an error from setup_tracking()

[tsukasa inoue via GitGitGadget]

From: tsukasa inoue <inoue.tsukasa.4@gmail.com>

setup_tracking() calls exit(1) when it cannot write the tracking
configuration. This exit was introduced in 27852b2c53 (branch: report
errors in tracking branch setup, 2016-02-22). Because of it, callers
cannot continue after a tracking failure, even though the ref has
already been created.

Change setup_tracking() to return an error instead of exiting, and make
create_branch() and dwim_and_setup_tracking() return that value too.

This does not change behavior. Callers still exit with status 1 on
failure, as before. Changing checkout

Signed-off-by: tsukasa inoue <inoue.tsukasa.4@gmail.com>
---
 branch.c           | 35 +++++++++++++++++++++--------------
 branch.h           | 21 +++++++++++++--------
 builtin/branch.c   | 17 ++++++++++++-----
 builtin/checkout.c | 17 +++++++++--------
 4 files changed, 55 insertions(+), 35 deletions(-)

diff --git a/branch.c b/branch.c
index bb07846911..7876892cc5 100644
--- a/branch.c
+++ b/branch.c
@@ -251,12 +251,13 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
  * will not be expanded to "refs/remotes/origin/main", so it is not safe
  * for 'orig_ref' to be raw user input.
  */
-static void setup_tracking(const char *new_ref, const char *orig_ref,
-			   enum branch_track track, int quiet)
+static int setup_tracking(const char *new_ref, const char *orig_ref,
+			  enum branch_track track, int quiet)
 {
 	struct tracking tracking;
 	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
 	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
+	int ret = 0;
 	struct find_tracked_branch_cb ftb_cb = {
 		.tracking = &tracking,
 		.ambiguous_remotes = STRING_LIST_INIT_DUP,
@@ -345,11 +346,12 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 		string_list_append(tracking.srcs, orig_ref);
 	if (install_branch_config_multiple_remotes(config_flags, new_ref,
 				tracking.remote, tracking.srcs) < 0)
-		exit(1);
+		ret = -1;
 
 cleanup:
 	string_list_clear(&tracking_srcs, 0);
 	string_list_clear(&ftb_cb.ambiguous_remotes, 0);
+	return ret;
 }
 
 int read_branch_desc(struct strbuf *buf, const char *branch_name)
@@ -629,10 +631,10 @@ static void dwim_branch_start(struct repository *r, const char *start_name,
 	FREE_AND_NULL(real_ref);
 }
 
-void create_branch(struct repository *r,
-		   const char *name, const char *start_name,
-		   int force, int clobber_head_ok, int reflog,
-		   int quiet, enum branch_track track, int dry_run)
+int create_branch(struct repository *r,
+		  const char *name, const char *start_name,
+		  int force, int clobber_head_ok, int reflog,
+		  int quiet, enum branch_track track, int dry_run)
 {
 	struct object_id oid;
 	char *real_ref;
@@ -641,6 +643,7 @@ void create_branch(struct repository *r,
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 	int flags = 0;
+	int ret = 0;
 	char *msg;
 
 	if (track == BRANCH_TRACK_OVERRIDE)
@@ -678,21 +681,25 @@ void create_branch(struct repository *r,
 	free(msg);
 
 	if (real_ref && track)
-		setup_tracking(ref.buf + 11, real_ref, track, quiet);
+		ret = setup_tracking(ref.buf + 11, real_ref, track, quiet);
 
 cleanup:
 	strbuf_release(&ref);
 	free(real_ref);
+	return ret;
 }
 
-void dwim_and_setup_tracking(struct repository *r, const char *new_ref,
-			     const char *orig_ref, enum branch_track track,
-			     int quiet)
+int dwim_and_setup_tracking(struct repository *r, const char *new_ref,
+			    const char *orig_ref, enum branch_track track,
+			    int quiet)
 {
 	char *real_orig_ref = NULL;
+	int ret;
+
 	dwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL);
-	setup_tracking(new_ref, real_orig_ref, track, quiet);
+	ret = setup_tracking(new_ref, real_orig_ref, track, quiet);
 	free(real_orig_ref);
+	return ret;
 }
 
 /**
@@ -842,8 +849,8 @@ void create_branches_recursively(struct repository *r, const char *name,
 	 * tedious to determine whether or not tracking was set up in the
 	 * superproject.
 	 */
-	if (track)
-		setup_tracking(name, tracking_name, track, quiet);
+	if (track && setup_tracking(name, tracking_name, track, quiet))
+		exit(1);
 
 	for (i = 0; i < submodule_entry_list.entry_nr; i++) {
 		if (submodule_create_branch(
diff --git a/branch.h b/branch.h
index e9b1f7b37d..4bc4527284 100644
--- a/branch.h
+++ b/branch.h
@@ -34,10 +34,13 @@ enum branch_track {
  *     causes new_ref to track whatever orig_ref tracks.
  *
  *   - quiet suppresses tracking information.
+ *
+ * Returns 0 on success, or -1 if the tracking configuration could not be
+ * written.
  */
-void dwim_and_setup_tracking(struct repository *r, const char *new_ref,
-			     const char *orig_ref, enum branch_track track,
-			     int quiet);
+int dwim_and_setup_tracking(struct repository *r, const char *new_ref,
+			    const char *orig_ref, enum branch_track track,
+			    int quiet);
 
 /*
  * Creates a new branch, where:
@@ -63,12 +66,14 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref,
  *
  *   - dry_run causes the branch to be validated but not created.
  *
+ * Returns 0 on success, or -1 if the tracking configuration could not be
+ * written.
  */
-void create_branch(struct repository *r,
-		   const char *name, const char *start_name,
-		   int force, int clobber_head_ok,
-		   int reflog, int quiet, enum branch_track track,
-		   int dry_run);
+int create_branch(struct repository *r,
+		  const char *name, const char *start_name,
+		  int force, int clobber_head_ok,
+		  int reflog, int quiet, enum branch_track track,
+		  int dry_run);
 
 /*
  * Creates a new branch in a repository and its submodules (and its
diff --git a/builtin/branch.c b/builtin/branch.c
index a613148fc7..b536b619bb 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -1253,9 +1253,13 @@ int cmd_branch(int argc,
 			die(_("branch '%s' does not exist"), branch->name);
 		}
 
-		dwim_and_setup_tracking(the_repository, branch->name,
-					new_upstream, BRANCH_TRACK_OVERRIDE,
-					quiet);
+		if (dwim_and_setup_tracking(the_repository, branch->name,
+					    new_upstream, BRANCH_TRACK_OVERRIDE,
+					    quiet)) {
+			strbuf_release(&buf);
+			ret = 1;
+			goto out;
+		}
 		strbuf_release(&buf);
 	} else if (unset_upstream) {
 		struct branch *branch;
@@ -1305,8 +1309,11 @@ int cmd_branch(int argc,
 			ret = 0;
 			goto out;
 		}
-		create_branch(the_repository, branch_name, start_name, force, 0,
-			      reflog, quiet, track, 0);
+		if (create_branch(the_repository, branch_name, start_name,
+				  force, 0, reflog, quiet, track, 0)) {
+			ret = 1;
+			goto out;
+		}
 	} else
 		usage_with_options(builtin_branch_usage, options);
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index c0f0d2c700..87e067b45b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -979,14 +979,15 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 			free(refname);
 		}
 		else
-			create_branch(the_repository,
-				      opts->new_branch, new_branch_info->name,
-				      opts->new_branch_force ? 1 : 0,
-				      opts->new_branch_force ? 1 : 0,
-				      opts->new_branch_log,
-				      opts->quiet,
-				      opts->track,
-				      0);
+			if (create_branch(the_repository,
+					  opts->new_branch, new_branch_info->name,
+					  opts->new_branch_force ? 1 : 0,
+					  opts->new_branch_force ? 1 : 0,
+					  opts->new_branch_log,
+					  opts->quiet,
+					  opts->track,
+					  0))
+				exit(1);
 		free(new_branch_info->name);
 		free(new_branch_info->refname);
 		new_branch_info->name = xstrdup(opts->new_branch);
-- 
gitgitgadget

[PATCH v2 3/3] checkout: finish switching when the tracking config cannot be written

[tsukasa inoue via GitGitGadget]

From: tsukasa inoue <inoue.tsukasa.4@gmail.com>

When git switch -c <new> <remote>/<branch> (or checkout -b) cannot write
the tracking config, for example because .git/config.lock exists, the
worktree, the index and the new branch ref have already been updated.
Git then exits with status 1 while HEAD still points to the original
branch. The changes of the switch remain staged, and the post-checkout
hook does not run.

create_branch() now returns an error instead of exiting. Make checkout
ignore that error so that the switch completes and the command exits
with status 0. The error and the hint are still printed.

Match push -u, clone and fetch, which also print the error and continue.
The upstream can be set later with the command shown in the hint.

Keep git branch <new> <start> failing when the tracking config cannot be
written, so its behavior does not change; t3200 covers this. Making the
switch fully atomic, undoing the worktree, the index, the ref and the
config when the tracking config fails, is a separate topic.

Signed-off-by: tsukasa inoue <inoue.tsukasa.4@gmail.com>
---
 builtin/checkout.c        | 23 ++++++++++++++---------
 t/t2027-checkout-track.sh | 20 ++++++++++++++++++++
 t/t2060-switch.sh         | 20 ++++++++++++++++++++
 t/t3200-branch.sh         | 13 +++++++++++++
 4 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 87e067b45b..a203f9e2ca 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -979,15 +979,20 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 			free(refname);
 		}
 		else
-			if (create_branch(the_repository,
-					  opts->new_branch, new_branch_info->name,
-					  opts->new_branch_force ? 1 : 0,
-					  opts->new_branch_force ? 1 : 0,
-					  opts->new_branch_log,
-					  opts->quiet,
-					  opts->track,
-					  0))
-				exit(1);
+			/*
+			 * Not fatal: the error and hint have already been
+			 * shown, and like "push -u" and "clone" we finish
+			 * the switch instead of leaving HEAD on the old
+			 * branch with the worktree already updated.
+			 */
+			create_branch(the_repository,
+				      opts->new_branch, new_branch_info->name,
+				      opts->new_branch_force ? 1 : 0,
+				      opts->new_branch_force ? 1 : 0,
+				      opts->new_branch_log,
+				      opts->quiet,
+				      opts->track,
+				      0);
 		free(new_branch_info->name);
 		free(new_branch_info->refname);
 		new_branch_info->name = xstrdup(opts->new_branch);
diff --git a/t/t2027-checkout-track.sh b/t/t2027-checkout-track.sh
index c01f1cd617..c3be183bfc 100755
--- a/t/t2027-checkout-track.sh
+++ b/t/t2027-checkout-track.sh
@@ -47,6 +47,26 @@ test_expect_success 'checkout --track -b overrides autoSetupMerge=inherit' '
 	test_cmp_config refs/heads/main branch.b4.merge
 '
 
+test_expect_success 'checkout -b completes when tracking config cannot be written' '
+	test_when_finished "git update-ref -d refs/remotes/lockremote/lockbranch" &&
+	test_when_finished "git branch -D lock-new" &&
+	test_when_finished "git checkout main" &&
+	test_config remote.lockremote.url . &&
+	test_config remote.lockremote.fetch "+refs/heads/*:refs/remotes/lockremote/*" &&
+	git update-ref refs/remotes/lockremote/lockbranch one &&
+	test_when_finished "rm -f .git/config.lock" &&
+	>.git/config.lock &&
+	git -c core.configLockTimeout=0 \
+		checkout -b lock-new lockremote/lockbranch 2>err &&
+	test_grep "unable to write upstream branch configuration" err &&
+	echo refs/heads/lock-new >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual &&
+	git diff --cached --exit-code &&
+	rm -f .git/config.lock &&
+	test_must_fail git config branch.lock-new.remote
+'
+
 test_expect_success 'ambiguous tracking info' '
 	# Set up a few remote repositories
 	git init --bare --initial-branch=trunk src1 &&
diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index cec2fe6876..b4f137acc3 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -146,6 +146,26 @@ test_expect_success 'tracking info copied with autoSetupMerge=inherit' '
 	test_cmp_config "" --default "" branch.main2.merge
 '
 
+test_expect_success 'switch -c completes when tracking config cannot be written' '
+	test_when_finished "git update-ref -d refs/remotes/lockremote/lockbranch" &&
+	test_when_finished "git branch -D lock-new" &&
+	test_when_finished "git switch main" &&
+	test_config remote.lockremote.url . &&
+	test_config remote.lockremote.fetch "+refs/heads/*:refs/remotes/lockremote/*" &&
+	git update-ref refs/remotes/lockremote/lockbranch first-branch &&
+	test_when_finished "rm -f .git/config.lock" &&
+	>.git/config.lock &&
+	git -c core.configLockTimeout=0 \
+		switch -c lock-new lockremote/lockbranch 2>err &&
+	test_grep "unable to write upstream branch configuration" err &&
+	echo refs/heads/lock-new >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual &&
+	git diff --cached --exit-code &&
+	rm -f .git/config.lock &&
+	test_must_fail git config branch.lock-new.remote
+'
+
 test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '
 	test_when_finished "
 		git worktree remove wt1 ||:
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ccfb53efa3..b9c169a91a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1105,6 +1105,19 @@ test_expect_success '--set-upstream-to hint uses short remote-tracking branch na
 	test_cmp_config refs/heads/hintbranch branch.hint-local.merge
 '
 
+test_expect_success 'branch with start point fails when tracking config cannot be written' '
+	test_when_finished "git branch -D lock-new || :" &&
+	test_when_finished "git update-ref -d refs/remotes/lockremote/lockbranch" &&
+	test_config remote.lockremote.url . &&
+	test_config remote.lockremote.fetch "+refs/heads/*:refs/remotes/lockremote/*" &&
+	git update-ref refs/remotes/lockremote/lockbranch HEAD &&
+	test_when_finished "rm -f .git/config.lock" &&
+	>.git/config.lock &&
+	test_must_fail git -c core.configLockTimeout=0 \
+		branch lock-new lockremote/lockbranch 2>err &&
+	test_grep "unable to write upstream branch configuration" err
+'
+
 test_expect_success 'use --set-upstream-to modify HEAD' '
 	test_config branch.main.remote foo &&
 	test_config branch.main.merge foo &&
-- 
gitgitgadget

Re: [PATCH v2 0/3] checkout: finish switch when tracking config cannot be written

[Junio C Hamano]

"tsukasa-ino via GitGitGadget" <gitgitgadget@gmail.com> writes:

> When the tracking config cannot be written (for example, because
> .git/config.lock exists), git switch -c and checkout -b stop halfway. The
> worktree, the index and the new branch ref are already updated, but HEAD
> stays on the old branch, and the changes are left staged. The hint printed
> at that point was also wrong: it suggested origin/refs/heads/.
>
> 1/3 strips refs/heads/ from the hint. 2/3 makes setup_tracking() return an
> error instead of calling exit(). No behavior change yet. 3/3 makes switch
> and checkout finish the switch and exit with status 0, as push -u, clone and
> fetch already do. git branch still fails.
>
> Changes since v1:
>
>  * 1/3: the test now compares the whole hint message, as suggested by Harald
>    Nordgren. It also checks that the command in the hint works once the lock
>    is removed, as suggested by Junio
>    (https://lore.kernel.org/git/xmqq7bjcup46.fsf@gitster.g/).

OK.

>  * 2/3 and 3/3 are new. They implement the direction agreed on the bug
>    report thread (https://lore.kernel.org/git/xmqqecdjsryk.fsf@gitster.g/).

I am not sure if these two should be part of the same topic as the
first one, or they should be treated separately.  IOW, if the way
these two patches implement the fix proves to be contentious, does
it make sense to hold the change in 1/3 until 2&3/3 are resolved
(and vice versa), or can they be treated as two separate issues?

> Making the switch fully atomic, so that a failure undoes the worktree, the
> index, the ref and the config, is left for a separate topic.

Sensible.

Re: [PATCH v2 0/3] checkout: finish switch when tracking config cannot be written

[井上司]

> I am not sure if these two should be part of the same topic as the
> first one, or they should be treated separately.  IOW, if the way
> these two patches implement the fix proves to be contentious, does
> it make sense to hold the change in 1/3 until 2&3/3 are resolved
> (and vice versa), or can they be treated as two separate issues?

Thanks for the question. I checked whether the patches can be split.

- 2/3 applies without 1/3.
- 3/3 conflicts with 1/3 only in t3200, because its new test is placed
  right after the one added by 1/3. The code does not depend on 1/3. The
  hint sentence in the 3/3 commit message assumes the corrected hint, but
  that is only a wording dependency.

So 1/3 can be taken on its own. 2/3 and 3/3 can be discussed separately,
and changes to them do not need to hold up 1/3. I will resend 2/3 and 3/3
as a separate series on top of the current master, so that 1/3 can be
applied first.

2026年10月22日(木) 7:20 Junio C Hamano <gitster@pobox.com>:
>
> "tsukasa-ino via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > When the tracking config cannot be written (for example, because
> > .git/config.lock exists), git switch -c and checkout -b stop halfway. The
> > worktree, the index and the new branch ref are already updated, but HEAD
> > stays on the old branch, and the changes are left staged. The hint printed
> > at that point was also wrong: it suggested origin/refs/heads/.
> >
> > 1/3 strips refs/heads/ from the hint. 2/3 makes setup_tracking() return an
> > error instead of calling exit(). No behavior change yet. 3/3 makes switch
> > and checkout finish the switch and exit with status 0, as push -u, clone and
> > fetch already do. git branch still fails.
> >
> > Changes since v1:
> >
> >  * 1/3: the test now compares the whole hint message, as suggested by Harald
> >    Nordgren. It also checks that the command in the hint works once the lock
> >    is removed, as suggested by Junio
> >    (https://lore.kernel.org/git/xmqq7bjcup46.fsf@gitster.g/).
>
> OK.
>
> >  * 2/3 and 3/3 are new. They implement the direction agreed on the bug
> >    report thread (https://lore.kernel.org/git/xmqqecdjsryk.fsf@gitster.g/).
>
> I am not sure if these two should be part of the same topic as the
> first one, or they should be treated separately.  IOW, if the way
> these two patches implement the fix proves to be contentious, does
> it make sense to hold the change in 1/3 until 2&3/3 are resolved
> (and vice versa), or can they be treated as two separate issues?
>
> > Making the switch fully atomic, so that a failure undoes the worktree, the
> > index, the ref and the config, is left for a separate topic.
>
> Sensible.