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

2015-07-29 Thread Karthik Nayak
On Wed, Jul 29, 2015 at 3:26 PM, Matthieu Moy
 wrote:
> Karthik Nayak  writes:
>
>> On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy
>>  wrote:
>>>
 -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.
>
> Ah, OK. Previously, detached HEAD and branches were completely
> different, each having its own if (is_descendant_of(...)), and you're
> now using only one in append_ref() before removing it completely in
> favor of ref-filter.
>
> That would deserve an explanation for other reviewers I think.
>

Will include a small explanation in the commit message :)

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


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

2015-07-29 Thread Matthieu Moy
Karthik Nayak  writes:

> On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy
>  wrote:
>>
>>> -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.

Ah, OK. Previously, detached HEAD and branches were completely
different, each having its own if (is_descendant_of(...)), and you're
now using only one in append_ref() before removing it completely in
favor of ref-filter.

That would deserve an explanation for other reviewers I think.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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


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

2015-07-28 Thread Karthik Nayak
On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy
 wrote:
> Karthik Nayak  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


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

2015-07-28 Thread Matthieu Moy
Karthik Nayak  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/
--
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


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

2015-07-27 Thread Karthik Nayak
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().

Bump get_head_description() to the top.

Based-on-patch-by: Jeff King 
Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 builtin/branch.c | 122 ---
 1 file changed, 63 insertions(+), 59 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b058b74..f3d83d0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -30,6 +30,7 @@ static const char * const builtin_branch_usage[] = {
 
 #define REF_LOCAL_BRANCH0x01
 #define REF_REMOTE_BRANCH   0x02
+#define REF_DETACHED_HEAD   0x04
 
 static const char *head;
 static unsigned char head_sha1[20];
@@ -352,8 +353,12 @@ static int append_ref(const char *refname, const struct 
object_id *oid, int flag
break;
}
}
-   if (ARRAY_SIZE(ref_kind) <= i)
-   return 0;
+   if (ARRAY_SIZE(ref_kind) <= i) {
+   if (!strcmp(refname, "HEAD"))
+   kind = REF_DETACHED_HEAD;
+   else
+   return 0;
+   }
 
/* Don't add types the caller doesn't want */
if ((kind & ref_list->kinds) == 0)
@@ -497,6 +502,37 @@ static void add_verbose_info(struct strbuf *out, struct 
ref_item *item,
strbuf_release(&subject);
 }
 
+static char *get_head_description(void)
+{
+   struct strbuf desc = STRBUF_INIT;
+   struct wt_status_state state;
+   memset(&state, 0, sizeof(state));
+   wt_status_get_state(&state, 1);
+   if (state.rebase_in_progress ||
+   state.rebase_interactive_in_progress)
+   strbuf_addf(&desc, _("(no branch, rebasing %s)"),
+   state.branch);
+   else if (state.bisect_in_progress)
+   strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
+   state.branch);
+   else if (state.detached_from) {
+   /* TRANSLATORS: make sure these match _("HEAD detached at ")
+  and _("HEAD detached from ") in wt-status.c */
+   if (state.detached_at)
+   strbuf_addf(&desc, _("(HEAD detached at %s)"),
+   state.detached_from);
+   else
+   strbuf_addf(&desc, _("(HEAD detached from %s)"),
+   state.detached_from);
+   }
+   else
+   strbuf_addstr(&desc, _("(no branch)"));
+   free(state.branch);
+   free(state.onto);
+   free(state.detached_from);
+   return strbuf_detach(&desc, NULL);
+}
+
 static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
   int abbrev, int current, const char *remote_prefix)
 {
@@ -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();
+   break;
default:
color = BRANCH_COLOR_PLAIN;
break;
@@ -527,7 +568,7 @@ static void print_ref_item(struct ref_item *item, int 
maxwidth, int verbose,
color = BRANCH_COLOR_CURRENT;
}
 
-   strbuf_addf(&name, "%s%s", prefix, item->name);
+   strbuf_addf(&name, "%s%s", prefix, desc);
if (verbose) {
int utf8_compensation = strlen(name.buf) - 
utf8_strwidth(name.buf);
strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
@@ -569,56 +610,9 @@ static int calc_maxwidth(struct ref_list *refs, int 
remote_bonus)
return max;
 }
 
-static char *get_head_description(void)
-{
-   struct strbuf desc = STRBUF_INIT;
-   struct wt_status_state state;
-   memset(&state, 0, sizeof(state));
-   wt_status_get_state(&state, 1);
-   if (state.rebase_in_progress ||
-   state.rebase_interactive_in_progress)
-   strbuf_addf(&desc, _("(no branch, rebasing %s)"),
-   state.branch);
-   else if (state.bisect_in_progress)
-   strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
-   state.branch);
-   else if (state.detached_from) {
-   /* TRANSLATORS: make sure these match _("HEAD detached at ")
-  and _("HEAD detached from ") in wt-status.c */
-   if (