| Message ID | 20211109030028.2196416-2-andersk@mit.edu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/4] fetch: Protect branches checked out in all worktrees | expand |
Hi Anders, looks good, just one suggestion, see inlined. On Mon, 8 Nov 2021, Anders Kaseorg wrote: > update_worktree() can only be called with a non-NULL worktree parameter, > because that’s the only case where we set do_update_worktree = 1. > worktree->path is always initialized to non-NULL. > > Signed-off-by: Anders Kaseorg <andersk@mit.edu> > --- > builtin/receive-pack.c | 20 +++++--------------- > 1 file changed, 5 insertions(+), 15 deletions(-) > > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index 49b846d960..cf575280fc 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -1449,29 +1449,19 @@ static const char *push_to_checkout(unsigned char *hash, > > static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree) > { > - const char *retval, *work_tree, *git_dir = NULL; > + const char *retval, *git_dir; > struct strvec env = STRVEC_INIT; > > - if (worktree && worktree->path) > - work_tree = worktree->path; > - else if (git_work_tree_cfg) > - work_tree = git_work_tree_cfg; > - else > - work_tree = ".."; We might want to make sure that `worktree` and `worktree->path` are non-`NULL`, and otherwise call a `BUG()`. > - > if (is_bare_repository()) Okay, I lied, I have two suggestions. Shouldn't this be turned into `worktree->is_bare`? Of course, `find_shared_symref()` will currently not return any worktree with non-zero `is_bare`... > return "denyCurrentBranch = updateInstead needs a worktree"; > - if (worktree) > - git_dir = get_worktree_git_dir(worktree); > - if (!git_dir) > - git_dir = get_git_dir(); > + git_dir = get_worktree_git_dir(worktree); > > strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir)); > > if (!hook_exists(push_to_checkout_hook)) > - retval = push_to_deploy(sha1, &env, work_tree); > + retval = push_to_deploy(sha1, &env, worktree->path); > else > - retval = push_to_checkout(sha1, &env, work_tree); > + retval = push_to_checkout(sha1, &env, worktree->path); > > strvec_clear(&env); > return retval; > @@ -1579,7 +1569,7 @@ static const char *update(struct command *cmd, struct shallow_info *si) > } > > if (do_update_worktree) { > - ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name)); > + ret = update_worktree(new_oid->hash, worktree); Makes sense. If `worktree` is `NULL`, `do_update_worktree` won't ever be set, and if `worktree` is not `NULL`, even before we remove that `is_bare_repository()` ternary, it is set to `find_shared_symref("HEAD", name)` (and `name` is not changed in the `update()` function). Thanks, Dscho > if (ret) > return ret; > } > -- > 2.33.1 > >
On 11/9/21 08:16, Johannes Schindelin wrote: > We might want to make sure that `worktree` and `worktree->path` are > non-`NULL`, and otherwise call a `BUG()`. Okay. > Okay, I lied, I have two suggestions. Shouldn't this be turned into > `worktree->is_bare`? Sure (but in the next commit, since removing is_bare_repository() is a bug fix, not pure cleanup). Anders
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 49b846d960..cf575280fc 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1449,29 +1449,19 @@ static const char *push_to_checkout(unsigned char *hash, static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree) { - const char *retval, *work_tree, *git_dir = NULL; + const char *retval, *git_dir; struct strvec env = STRVEC_INIT; - if (worktree && worktree->path) - work_tree = worktree->path; - else if (git_work_tree_cfg) - work_tree = git_work_tree_cfg; - else - work_tree = ".."; - if (is_bare_repository()) return "denyCurrentBranch = updateInstead needs a worktree"; - if (worktree) - git_dir = get_worktree_git_dir(worktree); - if (!git_dir) - git_dir = get_git_dir(); + git_dir = get_worktree_git_dir(worktree); strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir)); if (!hook_exists(push_to_checkout_hook)) - retval = push_to_deploy(sha1, &env, work_tree); + retval = push_to_deploy(sha1, &env, worktree->path); else - retval = push_to_checkout(sha1, &env, work_tree); + retval = push_to_checkout(sha1, &env, worktree->path); strvec_clear(&env); return retval; @@ -1579,7 +1569,7 @@ static const char *update(struct command *cmd, struct shallow_info *si) } if (do_update_worktree) { - ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name)); + ret = update_worktree(new_oid->hash, worktree); if (ret) return ret; }
update_worktree() can only be called with a non-NULL worktree parameter, because that’s the only case where we set do_update_worktree = 1. worktree->path is always initialized to non-NULL. Signed-off-by: Anders Kaseorg <andersk@mit.edu> --- builtin/receive-pack.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-)