Message ID | 20211109230941.2518143-1-andersk@mit.edu (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v5,1/4] fetch: Protect branches checked out in all worktrees | expand |
On Tue, Nov 09 2021, Anders Kaseorg wrote: > Refuse to fetch into the currently checked out branch of any working > tree, not just the current one. > > Fixes this previously reported bug: > > https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de > > As a side effect of using find_shared_symref, we’ll also refuse the > fetch when we’re on a detached HEAD because we’re rebasing or bisecting > on the branch in question. This seems like a sensible change. Missing tests though, would be nice to have a test that saw what happened when the branch is in that "git bisect start" or rebasing state. Also what those commands to if the branch is updated, e.g. with git-update-ref.
Hi Anders, responding here instead of to the cover letter (because there is none ;-)): great work! Others pointed out the really tiny nit that some phrases should start with lower-case, which would be nice to see addressed. I did not find any major issue anymore (apart from the slightly iffy assumption that `buf->ref` starts with `refs/heads/` and therefore `buf->ref + strlen("refs/heads/")` would not overrun, but I _think_ the current code enforces that prefix somewhere along the lines), so: Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de> Thank you for this contribution! Dscho On Tue, 9 Nov 2021, Anders Kaseorg wrote: > Refuse to fetch into the currently checked out branch of any working > tree, not just the current one. > > Fixes this previously reported bug: > > https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de > > As a side effect of using find_shared_symref, we’ll also refuse the > fetch when we’re on a detached HEAD because we’re rebasing or bisecting > on the branch in question. This seems like a sensible change. > > Signed-off-by: Anders Kaseorg <andersk@mit.edu> > --- > builtin/fetch.c | 29 +++++++++++++++-------------- > t/t5516-fetch-push.sh | 18 ++++++++++++++++++ > 2 files changed, 33 insertions(+), 14 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index f7abbc31ff..ed8a906717 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -28,6 +28,7 @@ > #include "promisor-remote.h" > #include "commit-graph.h" > #include "shallow.h" > +#include "worktree.h" > > #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000) > > @@ -854,7 +855,7 @@ static int update_local_ref(struct ref *ref, > int summary_width) > { > struct commit *current = NULL, *updated; > - struct branch *current_branch = branch_get(NULL); > + const struct worktree *wt; > const char *pretty_ref = prettify_refname(ref->name); > int fast_forward = 0; > > @@ -868,16 +869,18 @@ static int update_local_ref(struct ref *ref, > return 0; > } > > - if (current_branch && > - !strcmp(ref->name, current_branch->name) && > - !(update_head_ok || is_bare_repository()) && > + if (!update_head_ok && > + (wt = find_shared_symref("HEAD", ref->name)) && > + !wt->is_bare && > !is_null_oid(&ref->old_oid)) { > /* > * If this is the head, and it's not okay to update > * the head, and the old value of the head isn't empty... > */ > format_display(display, '!', _("[rejected]"), > - _("can't fetch in current branch"), > + wt->is_current ? > + _("can't fetch in current branch") : > + _("checked out in another worktree"), > remote, pretty_ref, summary_width); > return 1; > } > @@ -1387,16 +1390,14 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map, > > static void check_not_current_branch(struct ref *ref_map) > { > - struct branch *current_branch = branch_get(NULL); > - > - if (is_bare_repository() || !current_branch) > - return; > - > + const struct worktree *wt; > for (; ref_map; ref_map = ref_map->next) > - if (ref_map->peer_ref && !strcmp(current_branch->refname, > - ref_map->peer_ref->name)) > - die(_("Refusing to fetch into current branch %s " > - "of non-bare repository"), current_branch->refname); > + if (ref_map->peer_ref && > + (wt = find_shared_symref("HEAD", ref_map->peer_ref->name)) && > + !wt->is_bare) > + die(_("Refusing to fetch into branch '%s' " > + "checked out at '%s'"), > + ref_map->peer_ref->name, wt->path); > } > > static int truncate_fetch_head(void) > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index 8212ca56dc..f07e32126f 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -1771,4 +1771,22 @@ test_expect_success 'denyCurrentBranch and worktrees' ' > git -C cloned push origin HEAD:new-wt && > test_must_fail git -C cloned push --delete origin new-wt > ' > + > +test_expect_success 'refuse fetch to current branch of worktree' ' > + test_when_finished "git worktree remove --force wt && git branch -D wt" && > + git worktree add wt && > + test_commit apple && > + test_must_fail git fetch . HEAD:wt && > + git fetch -u . HEAD:wt > +' > + > +test_expect_success 'refuse fetch to current branch of bare repository worktree' ' > + test_when_finished "rm -fr bare.git" && > + git clone --bare . bare.git && > + git -C bare.git worktree add wt && > + test_commit banana && > + test_must_fail git -C bare.git fetch .. HEAD:wt && > + git -C bare.git fetch -u .. HEAD:wt > +' > + > test_done > -- > 2.33.1 > >
Anders Kaseorg <andersk@mit.edu> writes: > + if (!update_head_ok && > + (wt = find_shared_symref("HEAD", ref->name)) && > + !wt->is_bare && > !is_null_oid(&ref->old_oid)) { > /* > * If this is the head, and it's not okay to update > * the head, and the old value of the head isn't empty... > */ > format_display(display, '!', _("[rejected]"), > - _("can't fetch in current branch"), > + wt->is_current ? > + _("can't fetch in current branch") : > + _("checked out in another worktree"), > remote, pretty_ref, summary_width); > return 1; > } > @@ -1387,16 +1390,14 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map, > > static void check_not_current_branch(struct ref *ref_map) > { > - struct branch *current_branch = branch_get(NULL); > - > - if (is_bare_repository() || !current_branch) > - return; > - > + const struct worktree *wt; > for (; ref_map; ref_map = ref_map->next) > - if (ref_map->peer_ref && !strcmp(current_branch->refname, > - ref_map->peer_ref->name)) > - die(_("Refusing to fetch into current branch %s " > - "of non-bare repository"), current_branch->refname); > + if (ref_map->peer_ref && > + (wt = find_shared_symref("HEAD", ref_map->peer_ref->name)) && > + !wt->is_bare) > + die(_("Refusing to fetch into branch '%s' " > + "checked out at '%s'"), > + ref_map->peer_ref->name, wt->path); > } Another thing for the next development cycle. The find_shared_symref() function is handy (and more correct than dereferencing HEAD in the current worktree alone, of course), but its memory ownership model may need to be rethought. The current semantics is a caller can call find_shared_symref() to receive at most one worktree, and the caller can use it UNTIL anybody makes another call to find_shared_symref(), at which point, the worktree instance becomes unusable and off limit. The caller cannot, and should not attempt to, free the worktree instance. Each time find_shared_symref() is called, we enumerate all the worktrees and store them in a list that is static to the function. The returned worktree instance points into that list. It is not technically leaked because the static "worktrees" list in the function holds onto it, and each time the function is called, the old list of worktrees is discarded and rebuilt anew. What it means is that a code like the above, a loop in check_not_current_branch(), that repeatedly calls find_shared_symref() is both inefficient (because it takes many "snapshots" of the worktrees attached to the repository), and also risks an inconsistent view of the world (because it takes many "snapshots", and each iteration uses different ones). I suspect that having a new API function that lets the above be rewritten along the lines of ... struct worktree **all = get_worktrees(); for ( ; ref_map; ref_map = ref_map->next) { if (!ref_map->peer_ref) continue; wt = find_wt_with_HEAD(all, ref->map->peer_ref->name); if (!wt->is_bare) die(_("...")); } free_worktrees(all); ... would help. Thanks.
On Wed, 10 Nov 2021, Junio C Hamano wrote: > The find_shared_symref() function is handy (and more correct than > dereferencing HEAD in the current worktree alone, of course), but > its memory ownership model may need to be rethought. I wasn’t sure if we wanted to expand the scope of this series, but I do agree. How about moving worktrees from the static variable to a parameter of find_shared_symref()? Would you like me to rebase the series onto this patch? Anders -- >8 -- Subject: [PATCH] worktree: simplify find_shared_symref() memory ownership model Storing the worktrees list in a static variable meant that find_shared_symref() had to rebuild the list on each call (which is inefficient when the call site is in a loop), and also that each call invalidated the pointer returned by the previous call (which is confusing). Instead, make it the caller’s responsibility to pass in the worktrees list and manage its lifetime. Signed-off-by: Anders Kaseorg <andersk@mit.edu> --- branch.c | 14 ++++++---- builtin/branch.c | 7 ++++- builtin/notes.c | 6 +++- builtin/receive-pack.c | 63 +++++++++++++++++++++++++++--------------- worktree.c | 8 ++---- worktree.h | 5 ++-- 6 files changed, 65 insertions(+), 38 deletions(-) diff --git a/branch.c b/branch.c index 07a46430b3..302cc5a04d 100644 --- a/branch.c +++ b/branch.c @@ -357,14 +357,16 @@ void remove_branch_state(struct repository *r, int verbose) void die_if_checked_out(const char *branch, int ignore_current_worktree) { + struct worktree **worktrees = get_worktrees(); const struct worktree *wt; - wt = find_shared_symref("HEAD", branch); - if (!wt || (ignore_current_worktree && wt->is_current)) - return; - skip_prefix(branch, "refs/heads/", &branch); - die(_("'%s' is already checked out at '%s'"), - branch, wt->path); + wt = find_shared_symref(worktrees, "HEAD", branch); + if (wt && (!ignore_current_worktree || !wt->is_current)) { + skip_prefix(branch, "refs/heads/", &branch); + die(_("'%s' is already checked out at '%s'"), branch, wt->path); + } + + free_worktrees(worktrees); } int replace_each_worktree_head_symref(const char *oldref, const char *newref, diff --git a/builtin/branch.c b/builtin/branch.c index 7a1d1eeb07..d8f2164cd7 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -193,6 +193,7 @@ static void delete_branch_config(const char *branchname) static int delete_branches(int argc, const char **argv, int force, int kinds, int quiet) { + struct worktree **worktrees; struct commit *head_rev = NULL; struct object_id oid; char *name = NULL; @@ -229,6 +230,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, if (!head_rev) die(_("Couldn't look up commit object for HEAD")); } + + worktrees = get_worktrees(); + for (i = 0; i < argc; i++, strbuf_reset(&bname)) { char *target = NULL; int flags = 0; @@ -239,7 +243,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, if (kinds == FILTER_REFS_BRANCHES) { const struct worktree *wt = - find_shared_symref("HEAD", name); + find_shared_symref(worktrees, "HEAD", name); if (wt) { error(_("Cannot delete branch '%s' " "checked out at '%s'"), @@ -300,6 +304,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, free(name); strbuf_release(&bname); + free_worktrees(worktrees); return ret; } diff --git a/builtin/notes.c b/builtin/notes.c index 71c59583a1..7f60408dbb 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -861,15 +861,19 @@ static int merge(int argc, const char **argv, const char *prefix) update_ref(msg.buf, default_notes_ref(), &result_oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR); else { /* Merge has unresolved conflicts */ + struct worktree **worktrees; const struct worktree *wt; /* Update .git/NOTES_MERGE_PARTIAL with partial merge result */ update_ref(msg.buf, "NOTES_MERGE_PARTIAL", &result_oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR); /* Store ref-to-be-updated into .git/NOTES_MERGE_REF */ - wt = find_shared_symref("NOTES_MERGE_REF", default_notes_ref()); + worktrees = get_worktrees(); + wt = find_shared_symref(worktrees, "NOTES_MERGE_REF", + default_notes_ref()); if (wt) die(_("a notes merge into %s is already in-progress at %s"), default_notes_ref(), wt->path); + free_worktrees(worktrees); if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL)) die(_("failed to store link to current notes ref (%s)"), default_notes_ref()); diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 49b846d960..017c365298 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1486,12 +1486,17 @@ static const char *update(struct command *cmd, struct shallow_info *si) struct object_id *old_oid = &cmd->old_oid; struct object_id *new_oid = &cmd->new_oid; int do_update_worktree = 0; - const struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref("HEAD", name); + struct worktree **worktrees = get_worktrees(); + const struct worktree *worktree = + is_bare_repository() ? + NULL : + find_shared_symref(worktrees, "HEAD", name); /* only refs/... are allowed */ if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) { rp_error("refusing to create funny ref '%s' remotely", name); - return "funny refname"; + ret = "funny refname"; + goto out; } strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name); @@ -1510,7 +1515,8 @@ static const char *update(struct command *cmd, struct shallow_info *si) rp_error("refusing to update checked out branch: %s", name); if (deny_current_branch == DENY_UNCONFIGURED) refuse_unconfigured_deny(); - return "branch is currently checked out"; + ret = "branch is currently checked out"; + goto out; case DENY_UPDATE_INSTEAD: /* pass -- let other checks intervene first */ do_update_worktree = 1; @@ -1521,13 +1527,15 @@ static const char *update(struct command *cmd, struct shallow_info *si) if (!is_null_oid(new_oid) && !has_object_file(new_oid)) { error("unpack should have generated %s, " "but I can't find it!", oid_to_hex(new_oid)); - return "bad pack"; + ret = "bad pack"; + goto out; } if (!is_null_oid(old_oid) && is_null_oid(new_oid)) { if (deny_deletes && starts_with(name, "refs/heads/")) { rp_error("denying ref deletion for %s", name); - return "deletion prohibited"; + ret = "deletion prohibited"; + goto out; } if (worktree || (head_name && !strcmp(namespaced_name, head_name))) { @@ -1543,9 +1551,11 @@ static const char *update(struct command *cmd, struct shallow_info *si) if (deny_delete_current == DENY_UNCONFIGURED) refuse_unconfigured_deny_delete_current(); rp_error("refusing to delete the current branch: %s", name); - return "deletion of the current branch prohibited"; + ret = "deletion of the current branch prohibited"; + goto out; default: - return "Invalid denyDeleteCurrent setting"; + ret = "Invalid denyDeleteCurrent setting"; + goto out; } } } @@ -1563,25 +1573,30 @@ static const char *update(struct command *cmd, struct shallow_info *si) old_object->type != OBJ_COMMIT || new_object->type != OBJ_COMMIT) { error("bad sha1 objects for %s", name); - return "bad ref"; + ret = "bad ref"; + goto out; } old_commit = (struct commit *)old_object; new_commit = (struct commit *)new_object; if (!in_merge_bases(old_commit, new_commit)) { rp_error("denying non-fast-forward %s" " (you should pull first)", name); - return "non-fast-forward"; + ret = "non-fast-forward"; + goto out; } } if (run_update_hook(cmd)) { rp_error("hook declined to update %s", name); - return "hook declined"; + ret = "hook declined"; + goto out; } if (do_update_worktree) { - ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name)); + ret = update_worktree(new_oid->hash, + find_shared_symref(worktrees, "HEAD", + name)); if (ret) - return ret; + goto out; } if (is_null_oid(new_oid)) { @@ -1600,17 +1615,19 @@ static const char *update(struct command *cmd, struct shallow_info *si) old_oid, 0, "push", &err)) { rp_error("%s", err.buf); - strbuf_release(&err); - return "failed to delete"; + ret = "failed to delete"; + } else { + ret = NULL; /* good */ } strbuf_release(&err); - return NULL; /* good */ } else { struct strbuf err = STRBUF_INIT; if (shallow_update && si->shallow_ref[cmd->index] && - update_shallow_ref(cmd, si)) - return "shallow error"; + update_shallow_ref(cmd, si)) { + ret = "shallow error"; + goto out; + } if (ref_transaction_update(transaction, namespaced_name, @@ -1618,14 +1635,16 @@ static const char *update(struct command *cmd, struct shallow_info *si) 0, "push", &err)) { rp_error("%s", err.buf); - strbuf_release(&err); - - return "failed to update ref"; + ret = "failed to update ref"; + } else { + ret = NULL; /* good */ } strbuf_release(&err); - - return NULL; /* good */ } + +out: + free_worktrees(worktrees); + return ret; } static void run_update_post_hook(struct command *commands) diff --git a/worktree.c b/worktree.c index 092a4f92ad..cf13d63845 100644 --- a/worktree.c +++ b/worktree.c @@ -402,17 +402,13 @@ int is_worktree_being_bisected(const struct worktree *wt, * bisect). New commands that do similar things should update this * function as well. */ -const struct worktree *find_shared_symref(const char *symref, +const struct worktree *find_shared_symref(struct worktree **worktrees, + const char *symref, const char *target) { const struct worktree *existing = NULL; - static struct worktree **worktrees; int i = 0; - if (worktrees) - free_worktrees(worktrees); - worktrees = get_worktrees(); - for (i = 0; worktrees[i]; i++) { struct worktree *wt = worktrees[i]; const char *symref_target; diff --git a/worktree.h b/worktree.h index 8b7c408132..9e06fcbdf3 100644 --- a/worktree.h +++ b/worktree.h @@ -143,9 +143,10 @@ void free_worktrees(struct worktree **); /* * Check if a per-worktree symref points to a ref in the main worktree * or any linked worktree, and return the worktree that holds the ref, - * or NULL otherwise. The result may be destroyed by the next call. + * or NULL otherwise. */ -const struct worktree *find_shared_symref(const char *symref, +const struct worktree *find_shared_symref(struct worktree **worktrees, + const char *symref, const char *target); /*
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > ... (apart from the slightly iffy assumption that `buf->ref` > starts with `refs/heads/` and therefore `buf->ref + strlen("refs/heads/")` > would not overrun, but I _think_ the current code enforces that prefix > somewhere along the lines) I think that is in 4/4, where the existing code does this: > diff --git a/branch.c b/branch.c > index 7a88a4861e..1aaf694b39 100644 > --- a/branch.c > +++ b/branch.c > @@ -199,18 +199,20 @@ int validate_branchname(const char *name, struct strbuf *ref) > */ > int validate_new_branchname(const char *name, struct strbuf *ref, int force) > { > - const char *head; > + const struct worktree *wt; > > if (!validate_branchname(name, ref)) > return 0; This takes a bare branch name in "name" (or a shorthand like @{-1}), expand that into a full refname into "ref". Before passing the ref into check_refname_format(), "refs/heads/" is unconditionally added at the beginning. So we know ref begins with "refs/heads/" after this point. > if (!force) > die(_("A branch named '%s' already exists."), > ref->buf + strlen("refs/heads/")); And we already assume ref->buf has "refs/heads/" as its prefix. It may be nice to use skip_prefix(), but it probably is not worth it. > + wt = find_shared_symref("HEAD", ref->buf); > + if (wt && !wt->is_bare) > + die(_("Cannot force update the branch '%s'" > + "checked out at '%s'."), > + ref->buf + strlen("refs/heads/"), wt->path); And this new use just reuses what we assume to be valid. So, correctness-wise, I do not think there is much to tweak further on top of this round. I've always queued this round more or less as-is. In preparation for the next development cycle, however, it might make sense to add a preparatory clean-up step to downcase the first word of "die()" messages in the files that are involved in this series (not necessarily the ones that are touched by the patches, but all of them) and then apply these four patches (with matching adjustments, like "Cannot force update" -> "cannot force update") on top. In another review message, I also noticed some inefficient code that is due to insufficient support from the worktree.c API, but that is not about correctness and can be left out of the series to get these fixes early in the next cycle. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > So, correctness-wise, I do not think there is much to tweak further > on top of this round. I've always queued this round more or less > as-is. Gahh. Sorry for the typo: "I've already queued", of course.
diff --git a/builtin/fetch.c b/builtin/fetch.c index f7abbc31ff..ed8a906717 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -28,6 +28,7 @@ #include "promisor-remote.h" #include "commit-graph.h" #include "shallow.h" +#include "worktree.h" #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000) @@ -854,7 +855,7 @@ static int update_local_ref(struct ref *ref, int summary_width) { struct commit *current = NULL, *updated; - struct branch *current_branch = branch_get(NULL); + const struct worktree *wt; const char *pretty_ref = prettify_refname(ref->name); int fast_forward = 0; @@ -868,16 +869,18 @@ static int update_local_ref(struct ref *ref, return 0; } - if (current_branch && - !strcmp(ref->name, current_branch->name) && - !(update_head_ok || is_bare_repository()) && + if (!update_head_ok && + (wt = find_shared_symref("HEAD", ref->name)) && + !wt->is_bare && !is_null_oid(&ref->old_oid)) { /* * If this is the head, and it's not okay to update * the head, and the old value of the head isn't empty... */ format_display(display, '!', _("[rejected]"), - _("can't fetch in current branch"), + wt->is_current ? + _("can't fetch in current branch") : + _("checked out in another worktree"), remote, pretty_ref, summary_width); return 1; } @@ -1387,16 +1390,14 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map, static void check_not_current_branch(struct ref *ref_map) { - struct branch *current_branch = branch_get(NULL); - - if (is_bare_repository() || !current_branch) - return; - + const struct worktree *wt; for (; ref_map; ref_map = ref_map->next) - if (ref_map->peer_ref && !strcmp(current_branch->refname, - ref_map->peer_ref->name)) - die(_("Refusing to fetch into current branch %s " - "of non-bare repository"), current_branch->refname); + if (ref_map->peer_ref && + (wt = find_shared_symref("HEAD", ref_map->peer_ref->name)) && + !wt->is_bare) + die(_("Refusing to fetch into branch '%s' " + "checked out at '%s'"), + ref_map->peer_ref->name, wt->path); } static int truncate_fetch_head(void) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 8212ca56dc..f07e32126f 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1771,4 +1771,22 @@ test_expect_success 'denyCurrentBranch and worktrees' ' git -C cloned push origin HEAD:new-wt && test_must_fail git -C cloned push --delete origin new-wt ' + +test_expect_success 'refuse fetch to current branch of worktree' ' + test_when_finished "git worktree remove --force wt && git branch -D wt" && + git worktree add wt && + test_commit apple && + test_must_fail git fetch . HEAD:wt && + git fetch -u . HEAD:wt +' + +test_expect_success 'refuse fetch to current branch of bare repository worktree' ' + test_when_finished "rm -fr bare.git" && + git clone --bare . bare.git && + git -C bare.git worktree add wt && + test_commit banana && + test_must_fail git -C bare.git fetch .. HEAD:wt && + git -C bare.git fetch -u .. HEAD:wt +' + test_done
Refuse to fetch into the currently checked out branch of any working tree, not just the current one. Fixes this previously reported bug: https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de As a side effect of using find_shared_symref, we’ll also refuse the fetch when we’re on a detached HEAD because we’re rebasing or bisecting on the branch in question. This seems like a sensible change. Signed-off-by: Anders Kaseorg <andersk@mit.edu> --- builtin/fetch.c | 29 +++++++++++++++-------------- t/t5516-fetch-push.sh | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+), 14 deletions(-)