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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

>>  	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).

Yeah, when I looked at Duy's version and thought about changing to
lose the empty statement myself, I was afraid that the "if"
condition might become less transparent to grasp, but now I see what
you actually written down, I think "if we got an error, or the
caller didn't ask us to, remove that file" is easy enough to
understand.

Thanks.