On Sat, Sep 12, 2015 at 11:19 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Fri, Sep 4, 2015 at 5:39 PM, Michael Rappazzo <rappazzo@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
>
I reversed the the check here; it is intended to check if the symref
is _not_ the head, since the head
ref has already been parsed. This is used in notes.c to find
"NOTES_MERGE_REF". I will move the
check out of the loop as you suggest above.
>> + } 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.
>
I have rebased the history a bit for the reroll, and hopefully it will
show the changes a little
more clearly.
>> +extern char *find_shared_symref(const char *symref, const char *target);
>> +
>> #endif
>> --
>> 2.5.0
--
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