| Message ID | 20211109230941.2518143-2-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: > + if (!worktree || !worktree->path) > + BUG("worktree->path must be non-NULL"); Perhaps a metter of taste, but I think BUG() should really be used for things that need a custom message over and beyond what assert() gives us. In this case using BUG() gives you a worse message, if you do: assert(worktree && worktree->path) You'll get a sensible message from any modern compiler quotign the variable etc, all of which says the same thing as that BUG() message, just with less verbosity.
Hi, On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Nov 09 2021, Anders Kaseorg wrote: > > > + if (!worktree || !worktree->path) > > + BUG("worktree->path must be non-NULL"); > > Perhaps a metter of taste, but I think BUG() should really be used for > things that need a custom message over and beyond what assert() gives > us. > > In this case using BUG() gives you a worse message, if you do: > > assert(worktree && worktree->path) > > You'll get a sensible message from any modern compiler quotign the > variable etc, all of which says the same thing as that BUG() message, > just with less verbosity. Maybe code reviews should stay away from contentious matters of taste. This claim that `assert()` would somehow be preferable to `BUG()` is not backed up by our very own coding guidelines. See for yourself: https://github.com/git/git/blob/v2.33.1/Documentation/CodingGuidelines does not mention it. The question of `assert()` vs `BUG()` has been brought up on this mailing list before, without a clear preference for `assert()`, in contrast to what the comment quoted above would want to make believe. And the fact that BUG() allows for a well-crafted message without having to rely on the compiler to guess as to what would make for a helpful message, that alone speaks volumes. Ciao, Johannes
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 49b846d960..542431e692 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1449,29 +1449,22 @@ 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 (!worktree || !worktree->path) + BUG("worktree->path must be non-NULL"); 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 +1572,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 | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)