| Message ID | alpine.DEB.2.21.999.2111081514460.100671@scrubbing-bubbles.mit.edu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] fetch: Protect branches checked out in all worktrees | expand |
Anders Kaseorg <andersk@mit.edu> writes: > 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. True. > - 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)) && > !is_null_oid(&ref->old_oid)) { We used to allow "git fetch" into a bare repository to update the branch that happens to be pointed at by the HEAD symref. The new code still allow it, but the way it does so is subtle [*]. Side note: The new code only works because find-shared-symref ignores a bare repository or a bare worktree. I would not be surprised if somebody starts arguing that the behaviour to ignore bare worktrees is a bug in that function and may accept a patch to correct it, and when I do so, I may not remember that this new code depends on that "bug". I would sleep better if we were one bit more careful, perhaps like so: + if (!update_head_ok && + (wt = find_shared_symref(...)) && + !wt->is_bare && !is_null_oid(...)) { to make sure we do not rely on that particular aspect of how find_shared_symref() works. The function asks "please find a worktree, if any, whose HEAD points at this ref", and it feels unnatural for the answer to the question is affected by the bare-ness of the worktree. > /* > * 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") : > + _("branch checked out in worktree"), OK, the former is about this worktree, and the latter is about worktree somewhere else. It may clarify if we phrased the latter a bit differently, e.g. "checked out in another worktree". Once we say "check(ed) out", we know we are talking about a branch, and format_display() would be showing the name of the branch on the same line anyway, so we could save the 6 letter spaces and tell the user that it is not happening here, but some other place. > +test_expect_success 'refuse fetch to current branch of worktree' ' > + test_commit -C cloned second && > + test_must_fail git fetch cloned HEAD:new-wt && This is because at this point in the test sequence, new-wt is the current branch for the worktree we added in the test immediately before this one. And we refuse unless update-head-ok is given. OK. > + git clone --bare . bare.git && > + git -C bare.git worktree add bare-wt && > + test_must_fail git -C bare.git fetch ../cloned HEAD:bare-wt && What is being tested here? We created a bare clone bare.git and added a worktree bare-wt to it. And we try to fetch into that bare repository, which would allow overwriting the branch pointed at by HEAD (which is new-wt) or any branch if there weren't a worktree that has a working tree. But because it has a working tree attached to it, namely, bare-wt, overwriting the current branch for that worktree is prevented. Good. > + git fetch -u cloned HEAD:new-wt && > + git -C bare.git fetch -u ../cloned HEAD:bare-wt These are to ensure that overriding the safety still works fine. Good. I cannot shake the feeling that this single test step is testing way too many things and burden future developers who break one of the steps to understand which step was broken, but these three are good things to test. Overall, looks quite good. Thanks. > +' > + > test_done
On 11/8/21 15:00, Junio C Hamano wrote: > I would sleep better if we were one bit more careful, perhaps like > so: > > + if (!update_head_ok && > + (wt = find_shared_symref(...)) && > + !wt->is_bare && > !is_null_oid(...)) { > > to make sure we do not rely on that particular aspect of how > find_shared_symref() works. The function asks "please find a > worktree, if any, whose HEAD points at this ref", and it feels > unnatural for the answer to the question is affected by the > bare-ness of the worktree. Not unreasonable to be explicit at the call site. Will update. I note that branch.c already relies on this aspect of find_shared_symref(). And, now that I’m reading branch.c again, I see that it also has a worktree-awareness bug in its safety check, deserving of a third patch in this series. $ git init $ git commit --allow-empty -am foo $ git worktree add wt $ git branch -M wt main # correctly disallowed fatal: Cannot force update the current branch. $ git branch -M wt # incorrectly allowed $ git worktree list /tmp/test 6de7a5d [wt] /tmp/test/wt 6de7a5d [wt] > OK, the former is about this worktree, and the latter is about > worktree somewhere else. It may clarify if we phrased the latter a > bit differently, e.g. "checked out in another worktree". Alright. (Of course, as noted in previous discussion, this error path is currently reachable.) > I cannot shake the feeling that this single test step is testing way > too many things and burden future developers who break one of the > steps to understand which step was broken, but these three are good > things to test. I can split the bare repository part into a separate test_expect_success step? This duplicates some commands but maybe helps readability overall. Anders
On 09/11/21 03.15, Anders Kaseorg wrote: > 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. > We can say "we'll also refuse fetching when we're on a detached HEAD (for example if we're rebasing on the branch in question or bisecting)."
On 11/8/21 21:19, Bagas Sanjaya wrote: > On 09/11/21 03.15, Anders Kaseorg wrote: >> 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. > > We can say "we'll also refuse fetching when we're on a detached HEAD > (for example if we're rebasing on the branch in question or bisecting)." No, I meant what I wrote. We don’t refuse the fetch if we’re on a detached HEAD for some other reason. That would be quite unhelpful. Anders
diff --git a/builtin/fetch.c b/builtin/fetch.c index f7abbc31ff..61c8fc9983 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,17 @@ 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)) && !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") : + _("branch checked out in worktree"), remote, pretty_ref, summary_width); return 1; } @@ -1387,16 +1389,13 @@ 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))) + 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..4ef4ecbe71 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1771,4 +1771,15 @@ 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_commit -C cloned second && + test_must_fail git fetch cloned HEAD:new-wt && + git clone --bare . bare.git && + git -C bare.git worktree add bare-wt && + test_must_fail git -C bare.git fetch ../cloned HEAD:bare-wt && + git fetch -u cloned HEAD:new-wt && + git -C bare.git fetch -u ../cloned HEAD:bare-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 | 27 +++++++++++++-------------- t/t5516-fetch-push.sh | 11 +++++++++++ 2 files changed, 24 insertions(+), 14 deletions(-)