Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 28, 2015, 19:19 UTC
Message-ID: <CAOLa=ZR4DSzdnR8+3eiVTCX92aeFu2o6=iDm7QpdOewizSuxng@mail.gmail.com>
In-Reply-To: <vpqlhe0xwpv.fsf@anie.imag.fr>

On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 7 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Remove show_detached() and make detached HEAD to be rolled into
>> regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
>> supporting the same in append_ref().
>
> Again, this lacks the "why?" explanation.

Will include a small explanation.

Show 5 quoted lines

>
>> Bump get_head_description() to the top.
>
> Here also: why? And this could easily go to a separate patch to let the
> reviewer focus on actual changes.

I'll move this to a preparatory patch.

Show 5 quoted lines

>
> I think you're leaking a string here: get_head_description() builds an
> strbuf and returns the dynamically allocated string, which you never
> free.
>

Ah! good catch, will fix that.

Show 17 quoted lines

>> -static void show_detached(struct ref_list *ref_list, int maxwidth)
>> -{
>> -     struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>> -
>> -     if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
>
> I'm not sure what this if was doing, and why you can get rid of it. My
> understanding is that with_commit comes from --contains, and in the
> previous code the filtering was done at display time (detached HEAD was
> not shown if it was not contained in commits specified with --contains).
>
> Eventually, you'll use ref-filter to do this filtering so you won't need
> this check at display time.
>
> But am I correct that for a few commits, you ignore --contains on
> detached HEAD?
>

No we don't ignore --contains on detached HEAD.

Since detached HEAD now gets its data from append_ref(). The function also checks for the --contains option.

Show 11 quoted lines

>> @@ -678,15 +674,20 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
>>       if (verbose)
>>               maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>>
>> -     qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
>> +     index = ref_list.index;
>> +
>> +     /* Print detached heads before sorting and printing the rest */
>
> I think you mean head (no s) or HEAD. It's not Mercurial ;-).
>

I meant HEAD, lol.

Show 14 quoted lines

>> +     if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&
>> +         !strcmp(ref_list.list[index - 1].name, head)) {
>> +             print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
>> +                            1, remote_prefix);
>> +             index -= 1;
>> +     }
>
> This relies on the assumption that HEAD has to be the last in the array.
> The assumption is correct since you call head_ref(append_ref, &cb) after
> for_each_rawref(append_ref, &cb). I think this deserves a comment to
> remind the reader that HEAD is always the last (here or at the call to
> head_ref to make sure nobody try to change the order between head_ref
> and for_each_rawref).
>

Yeah! makes sense, will add at the comment at the call to head_ref().

> It may be more natural to do it the other way around: call head_ref
> first and get HEAD first, as you are going to display it first (but in
> any case, you'll have to call qsort on a subset of the array so it
> doesn't change much).

That sorta messes things up with qsort, this keeps it clean I feel.

Show 14 quoted lines

>
>> @@ -913,7 +914,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>                       die(_("branch name required"));
>>               return delete_branches(argc, argv, delete > 1, kinds, quiet);
>>       } else if (list) {
>> -             int ret = print_ref_list(kinds, detached, verbose, abbrev,
>> +             int ret;
>> +             if (kinds & REF_LOCAL_BRANCH)
>> +                     kinds |= REF_DETACHED_HEAD;
>
> Perhaps add
>
>         /* git branch --local also shows HEAD when it is detached */
>

Yeah definitely!

-- 
Regards,
Karthik Nayak