On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy <matthieu....@grenoble-inp.fr> wrote: > Karthik Nayak <karthik....@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. > >> 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. > > 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. >> -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. >> @@ -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. >> + 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. > >> @@ -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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html