Re: [PATCH v5 1/2] worktrees: add find_shared_symref

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Aug 3, 2015 at 7:06 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Aug 3, 2015 at 2:48 PM, David Turner <dturner@twopensource.com> wrote:
>> Add a new function, find_shared_symref, which contains the heart of
>> die_if_checked_out, but works for any symref, not just HEAD.  Refactor
>> die_if_checked_out to use the same infrastructure as
>> find_shared_symref.
>>
>> Soon, we will use find_shared_symref to protect notes merges in
>> worktrees.
>>
>> Signed-off-by: David Turner <dturner@twopensource.com>
>> ---
>> diff --git a/branch.c b/branch.c
>> index c85be07..d2b3586 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -349,31 +350,53 @@ static void check_linked_checkout(const char *branch, const char *id)
>>                 strbuf_addstr(&gitdir, get_git_common_dir());
>>         skip_prefix(branch, "refs/heads/", &branch);
>>         strbuf_strip_suffix(&gitdir, ".git");
>> -       die(_("'%s' is already checked out at '%s'"), branch, gitdir.buf);

In addition to the other issues already mentioned, this one is a bit
more serious (though still rather superficial):

With this change, the skip_prefix(branch, "refs/heads/", &branch)
becomes meaningless. It was used by the die() to provide a nicer
looking error message, however, 'branch' now is never used after
skip_prefix(). More below...

>> +
>> +       strbuf_release(&path);
>> +       strbuf_release(&sb);
>> +       return strbuf_detach(&gitdir, NULL);
>>  done:
>>         strbuf_release(&path);
>>         strbuf_release(&sb);
>>         strbuf_release(&gitdir);
>> +
>> +       return NULL;
>>  }
>
> This would be cleaner and less redundant if you assign the existing
> location to a variable and just fall through to the 'done' label:
>
>     char *existing = NULL;
>     ...
>         skip_prefix(branch, "refs/heads/", &branch);
>         strbuf_strip_suffix(&gitdir, ".git");
>         existing = strbuf_detach(&gitdir, NULL);
>     done:
>         strbuf_release(&path);
>         strbuf_release(&sb);
>         strbuf_release(&gitdir);
>         return existing;
>
> There's no worry that the "existing" path will be clobbered by
> strbuf_release(&gitdir) since it's been detached already (and it's
> safe to release the strbuf without affecting what has been detached
> from it).
>
>> -void die_if_checked_out(const char *branch)
>> +char *find_shared_symref(const char *symref, const char *target)
>>  {
>>         struct strbuf path = STRBUF_INIT;
>>         DIR *dir;
>>         struct dirent *d;
>> +       char *existing;
>>
>> -       check_linked_checkout(branch, NULL);
>> +       if ((existing = find_linked_symref(symref, target, NULL)))
>> +               return existing;
>>
>>         strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
>>         dir = opendir(path.buf);
>>         strbuf_release(&path);
>>         if (!dir)
>> -               return;
>> +               return NULL;
>>
>>         while ((d = readdir(dir)) != NULL) {
>>                 if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
>>                         continue;
>> -               check_linked_checkout(branch, d->d_name);
>> +               existing = find_linked_symref(symref, target, d->d_name);
>> +               if (existing) {
>> +                       closedir(dir);
>> +                       return existing;
>
> For consistency with code nearby, this could have been handled by
> adding a 'done' label above the closedir() below and jumping to it
> from here, and then 'return existing'.
>
>> +               }
>>         }
>>         closedir(dir);
>> +
>> +       return NULL;
>> +}
>> +
>> +void die_if_checked_out(const char *branch)
>> +{
>> +       char *existing;
>> +
>> +       existing = find_shared_symref("HEAD", branch);
>> +       if (existing)
>> +               die(_("'%s' is already checked out at '%s'"), branch, existing);

Unlike the original die() in check_linked_checkout() which printed the
branch named shortened by skip_prefix(), this one still prints the
long-form branch name. An easy fix would be to move the skip_prefix()
invocation down to here to skip the "refs/heads/" prefix just before
die().

>>  }