On Mon, Aug 3, 2015 at 7:06 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Mon, Aug 3, 2015 at 2:48 PM, David Turner <dturner@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
>> ---
>> 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().
>> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html