| Message ID | 20200610063049.74666-7-sunshine@sunshineco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/7] worktree: factor out repeated string literal | expand |
On 10/06 02:30, Eric Sunshine wrote: > "git worktree add" checks that the specified path is a valid location > for a new worktree by ensuring that the path does not already exist and > is not already registered to another worktree (a path can be registered > but missing, for instance, if it resides on removable media). Since "git > worktree add" is not the only command which should perform such > validation ("git worktree move" ought to also), generalize the the > validation function for use by other callers, as well. There is an extra 'the' after generalize. > -static void validate_worktree_add(const char *path, const struct add_opts *opts) > +/* check that path is viable location for worktree */ > +static void check_candidate_path(const char *path, > + int force, > + struct worktree **worktrees, > + const char *cmd) > { > - struct worktree **worktrees; > struct worktree *wt; > int locked; > > if (file_exists(path) && !is_empty_dir(path)) > die(_("'%s' already exists"), path); > > - worktrees = get_worktrees(0); > wt = find_worktree_by_path(worktrees, path); > if (!wt) > - goto done; > + return; Should we do a 'return 1' on failure instead of just a blank 'return' so that we can denote failure of finding a worktree? > locked = !!worktree_lock_reason(wt); > - if ((!locked && opts->force) || (locked && opts->force > 1)) { > + if ((!locked && force) || (locked && force > 1)) { > if (delete_git_dir(wt->id)) > - die(_("unable to re-add worktree '%s'"), path); > - goto done; > + die(_("unusable worktree destination '%s'"), path); > + return; > } > > if (locked) > - die(_("'%s' is a missing but locked worktree;\nuse 'add -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), path); > + die(_("'%s' is a missing but locked worktree;\nuse '%s -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), cmd, path); Let's wrap this to 72 characters at maximum per line maybe? Meaning that the error message gets split into 2 lines. > - die(_("'%s' is a missing but already registered worktree;\nuse 'add -f' to override, or 'prune' or 'remove' to clear"), path); > - > -done: > - free_worktrees(worktrees); > + die(_("'%s' is a missing but already registered worktree;\nuse '%s -f' to override, or 'prune' or 'remove' to clear"), cmd, path); Similarly here. > > static int add_worktree(const char *path, const char *refname, > @@ -324,8 +323,12 @@ static int add_worktree(const char *path, const char *refname, > struct commit *commit = NULL; > int is_branch = 0; > struct strbuf sb_name = STRBUF_INIT; > + struct worktree **worktrees; > > - validate_worktree_add(path, opts); > + worktrees = get_worktrees(0); > + check_candidate_path(path, opts->force, worktrees, "add"); > + free_worktrees(worktrees); > + worktrees = NULL; > > /* is 'refname' a branch or commit? */ > if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) && It is necessary to call 'free_worktrees(worktrees)' at the end right? The 'get_worktrees()' function states that The caller is responsible for freeing the memory from the returned worktree(s).
On Wed, Jun 10, 2020 at 1:12 PM Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > On 10/06 02:30, Eric Sunshine wrote: > > "git worktree add" checks that the specified path is a valid location > > for a new worktree by ensuring that the path does not already exist and > > is not already registered to another worktree (a path can be registered > > but missing, for instance, if it resides on removable media). Since "git > > worktree add" is not the only command which should perform such > > validation ("git worktree move" ought to also), generalize the the > > validation function for use by other callers, as well. > > There is an extra 'the' after generalize. Thanks for noticing. I'll fix it if I re-roll, otherwise it can stay (unless Junio happens to fix it when queuing). > > if (!wt) > > - goto done; > > + return; > > Should we do a 'return 1' on failure instead of just a blank 'return' so > that we can denote failure of finding a worktree? This function is declared as returning 'void', so we can't "return 1". The function instead signals a problem by die()'ing. Changing it to return a success or failure result rather than dying is a different matter which can be done later if someone wants to do so, but is outside the scope of this patch series which is only making the minimal necessary changes to adapt the function for wider use. > > - die(_("'%s' is a missing but locked worktree;\nuse 'add -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), path); > > + die(_("'%s' is a missing but locked worktree;\nuse '%s -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), cmd, path); > > Let's wrap this to 72 characters at maximum per line maybe? Meaning that > the error message gets split into 2 lines. I'm not sure what you want to see wrapped; the warning message itself or the source code line? As for the warning message, it already is wrapped (see the embedded "\n"). At any rate, this patch makes the minimal change necessary to meet the goal of making the function re-usable. Anything beyond that (such as wrapping long lines) is outside the scope of the patch and would make it harder to reason about the changes. Wrapping the line is certainly something that someone can do later as a follow-up, but is not the goal of this series. > > - validate_worktree_add(path, opts); > > + worktrees = get_worktrees(0); > > + check_candidate_path(path, opts->force, worktrees, "add"); > > + free_worktrees(worktrees); > > + worktrees = NULL; > > It is necessary to call 'free_worktrees(worktrees)' at the end? The > 'get_worktrees()' states that > The caller is responsible for freeing the memory from the returned > worktree(s). This code _does_ call free_worktrees() as it should, so I'm not sure what you're asking or saying. (Perhaps you're confusing "caller" and "callee"?)
diff --git a/builtin/worktree.c b/builtin/worktree.c index 350108eba0..8fcf3f38fb 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -280,34 +280,33 @@ static const char *worktree_basename(const char *path, int *olen) return name; } -static void validate_worktree_add(const char *path, const struct add_opts *opts) +/* check that path is viable location for worktree */ +static void check_candidate_path(const char *path, + int force, + struct worktree **worktrees, + const char *cmd) { - struct worktree **worktrees; struct worktree *wt; int locked; if (file_exists(path) && !is_empty_dir(path)) die(_("'%s' already exists"), path); - worktrees = get_worktrees(0); wt = find_worktree_by_path(worktrees, path); if (!wt) - goto done; + return; locked = !!worktree_lock_reason(wt); - if ((!locked && opts->force) || (locked && opts->force > 1)) { + if ((!locked && force) || (locked && force > 1)) { if (delete_git_dir(wt->id)) - die(_("unable to re-add worktree '%s'"), path); - goto done; + die(_("unusable worktree destination '%s'"), path); + return; } if (locked) - die(_("'%s' is a missing but locked worktree;\nuse 'add -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), path); + die(_("'%s' is a missing but locked worktree;\nuse '%s -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), cmd, path); else - die(_("'%s' is a missing but already registered worktree;\nuse 'add -f' to override, or 'prune' or 'remove' to clear"), path); - -done: - free_worktrees(worktrees); + die(_("'%s' is a missing but already registered worktree;\nuse '%s -f' to override, or 'prune' or 'remove' to clear"), cmd, path); } static int add_worktree(const char *path, const char *refname, @@ -324,8 +323,12 @@ static int add_worktree(const char *path, const char *refname, struct commit *commit = NULL; int is_branch = 0; struct strbuf sb_name = STRBUF_INIT; + struct worktree **worktrees; - validate_worktree_add(path, opts); + worktrees = get_worktrees(0); + check_candidate_path(path, opts->force, worktrees, "add"); + free_worktrees(worktrees); + worktrees = NULL; /* is 'refname' a branch or commit? */ if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
"git worktree add" checks that the specified path is a valid location for a new worktree by ensuring that the path does not already exist and is not already registered to another worktree (a path can be registered but missing, for instance, if it resides on removable media). Since "git worktree add" is not the only command which should perform such validation ("git worktree move" ought to also), generalize the the validation function for use by other callers, as well. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- builtin/worktree.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)