Jeff King <peff@xxxxxxxx> 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.