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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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.

> 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.

> @@ -504,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>  	int color;
>  	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
>  	const char *prefix = "";
> +	const char *desc = item->name;
>  
>  	if (item->ignore)
>  		return;
> @@ -516,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>  		color = BRANCH_COLOR_REMOTE;
>  		prefix = remote_prefix;
>  		break;
> +	case REF_DETACHED_HEAD:
> +		color = BRANCH_COLOR_CURRENT;
> +		desc = get_head_description();

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

> -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?

> @@ -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 ;-).

> +	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).

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).

> @@ -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 */

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/