Re: [PATCH v7 2/3] worktree: move/refactor find_shared_symref from branch.c

> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
>> The code formerly in branch.c was largely the basis of the
>> get_worktree_list implementation is now moved to worktree.c,
>> and the find_shared_symref implementation has been refactored
>> to use get_worktree_list
>
> Some comments below in addition to those by Junio...
>
>> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
>> ---
>> diff --git a/branch.h b/branch.h
>> index d3446ed..58aa45f 100644
>> --- a/branch.h
>> +++ b/branch.h
>> @@ -59,12 +59,4 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
>>   */
>>  extern void die_if_checked_out(const char *branch);
>>
>> -/*
>> - * Check if a per-worktree symref points to a ref in the main worktree
>> - * or any linked worktree, and return the path to the exising worktree
>> - * if it is.  Returns NULL if there is no existing ref.  The caller is
>> - * responsible for freeing the returned path.
>> - */
>> -extern char *find_shared_symref(const char *symref, const char *target);
>> -
>>  #endif
>> diff --git a/worktree.c b/worktree.c
>> index 33d2e57..e45b651 100644
>> --- a/worktree.c
>> +++ b/worktree.c
>> @@ -155,3 +155,43 @@ done:
>>         return list;
>>  }
>>
>> +char *find_shared_symref(const char *symref, const char *target)
>> +{
>> +       char *existing = NULL;
>> +       struct strbuf path = STRBUF_INIT;
>> +       struct strbuf sb = STRBUF_INIT;
>> +       struct worktree_list *worktree_list = get_worktree_list();
>> +       struct worktree_list *orig_list = worktree_list;
>> +       struct worktree *matched = NULL;
>> +
>> +       while (!matched && worktree_list) {
>> +               if (strcmp("HEAD", symref)) {
>
> The result of strcmp() never changes, so this (expensive) check could
> be lifted out of the 'while' loop, however...
>
>> +                       strbuf_reset(&path);
>> +                       strbuf_reset(&sb);
>> +                       strbuf_addf(&path, "%s/%s", worktree_list->worktree->git_dir, symref);
>> +
>> +                       if (_parse_ref(path.buf, &sb, NULL)) {
>> +                               continue;
>> +                       }
>> +
>> +                       if (!strcmp(sb.buf, target))
>> +                               matched = worktree_list->worktree;
>
> The original code in branch.c, which this patch removes, did not need
> to make a special case of HEAD, so it's not immediately clear why this
> replacement code does so. This is the sort of issue which argues in
> favor of mutating the existing code (slowly) over the course of
> several patches into the final form, rather than having the final form
> come into existence out of thin air. When the changes are made
> incrementally, it is easier for reviewers to understand why such
> modifications are needed, which (hopefully) should lead to fewer
> questions such as this one.
>

>> +               } else {
>> +                       if (worktree_list->worktree->head_ref && !strcmp(worktree_list->worktree->head_ref, target))
>> +                               matched = worktree_list->worktree;
>> +               }
>> +               worktree_list = worktree_list->next ? worktree_list->next : NULL;
>> +       }
>> +
>> +       if (matched) {
>> +               existing = malloc(strlen(matched->path) + 1);
>> +               strcpy(existing, matched->path);
>
> A couple issues:
>
> This can be done more concisely and safely with xstrdup().
>
> In this codebase, it probably would be more idiomatic to use goto to
> drop out of the loop rather than setting 'matched' and then having to
> check 'matched' in the loop condition. So, for instance, each place
> which sets 'matched' could instead say:
>
>     existing = xstrdup(...);
>     goto done;
>
>> +       }
>> +
>> +done:
>
> This label doesn't have any matching goto's.
>
>> +       strbuf_release(&path);
>> +       strbuf_release(&sb);
>> +       worktree_list_release(orig_list);
>> +
>> +       return existing;
>> +}
>> diff --git a/worktree.h b/worktree.h
>> index 2bc0ab8..320f17e 100644
>> --- a/worktree.h
>> +++ b/worktree.h
>> @@ -45,4 +45,11 @@ struct worktree *get_worktree(const char *id);
>>  extern void worktree_list_release(struct worktree_list *);
>>  extern void worktree_release(struct worktree *);
>>
>> +/*
>> + * Look for a symref in any worktree, and return the path to the first
>> + * worktree found or NULL if not found.  The caller is responsible for
>> + * freeing the returned path.
>> + */
>
> For some reason, this comment differs a fair bit from the original in
> branch.h which was removed by this patch, however, the original
> comment was a bit more explanatory (I think).
>
> As a general rule, it's better to do code movement and code changes in
> separate patches since it's hard for reviewers to detect and
> comprehend differences in code which has been both moved and changed
> at the same time.
>

>> +extern char *find_shared_symref(const char *symref, const char *target);
>> +
>>  #endif
>> --
>> 2.5.0