On Thu, Apr 20, 2017 at 04:59:21PM +0700, Duy Nguyen wrote:
> On Wed, Apr 19, 2017 at 10:37:21PM -0700, Junio C Hamano wrote:
> > * nd/worktree-add-lock (2017-04-16) 2 commits
> > - SQUASH???
> > - worktree add: add --lock option
> >
> > Allow to lock a worktree immediately after it's created. This helps
> > prevent a race between "git worktree add; git worktree lock" and
> > "git worktree prune".
> >
> > Waiting for a response to SQUASH???
>
> Looking good. I would add some comment, lest ';' feel lonely. But it's
> really personal taste.
>
> -- 8< --
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 5ebdcce793..bc75676bf3 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -310,7 +310,7 @@ static int add_worktree(const char *path, const char *refname,
> strbuf_reset(&sb);
> strbuf_addf(&sb, "%s/locked", sb_repo.buf);
> if (!ret && opts->keep_locked)
> - ;
> + ; /* --lock wants to keep "locked" file */
> else
> unlink_or_warn(sb.buf);
I know this is just a drive-by comment, but given that the "else" is the
only thing that does anything, would it be simpler to flip the
conditional? The strbuf manipulation above also could go inside it.
E.g.:
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5ebdcce79..05ade547c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -307,12 +307,11 @@ static int add_worktree(const char *path, const char *refname,
junk_git_dir = NULL;
done:
- strbuf_reset(&sb);
- strbuf_addf(&sb, "%s/locked", sb_repo.buf);
- if (!ret && opts->keep_locked)
- ;
- else
+ if (ret || !opts->keep_locked) {
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%s/locked", sb_repo.buf);
unlink_or_warn(sb.buf);
+ }
argv_array_clear(&child_env);
strbuf_release(&sb);
strbuf_release(&symref);
(Since it's in its own block I'd also consider just introducing a new
variable for the path, but I guess reusing "sb" for each path is already
a pattern in the function).
-Peff