Re: What's cooking in git.git (Apr 2017, #04; Wed, 19)

[Jeff King <peff@peff.net>]

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