Re: [PATCH v4 3/8] branch: roll show_detached HEAD into regular ref_list

Karthik Nayak [off-list ref] writes:

I think Eric already mentionned it, but I don't remember the conclusion
and can't find it in the archives. Wouldn't it be cleaner to actually
remove the detached head from the array (do "ref_list.index -= 1"
instead of "index -= 1", and possibly free() what needs to be freed?

If you did so, you wouldn't have any possible confusion between the
local variable "index" and ref_list.index in the code below:

-	detached = (detached && (kinds & REF_LOCAL_BRANCH));
-	if (detached && match_patterns(pattern, "HEAD"))
-		show_detached(&ref_list, maxwidth);
+	qsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);
 
-	for (i = 0; i < ref_list.index; i++) {
-		int current = !detached &&
-			(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
+	for (i = 0; i < index; i++) {
+		int current = !detached && (ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
 			!strcmp(ref_list.list[i].name, head);
 		print_ref_item(&ref_list.list[i], maxwidth, verbose,
 			       abbrev, current, remote_prefix);

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

On Sun, Sep 13, 2015 at 5:42 PM, Matthieu Moy
[off-list ref] wrote:

Karthik Nayak [off-list ref] writes:

quoted

@@ -679,15 +682,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 HEAD before sorting and printing the rest */
+     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;
+     }

I think Eric already mentionned it, but I don't remember the conclusion
and can't find it in the archives. Wouldn't it be cleaner to actually
remove the detached head from the array (do "ref_list.index -= 1"
instead of "index -= 1", and possibly free() what needs to be freed?

If you did so, you wouldn't have any possible confusion between the
local variable "index" and ref_list.index in the code below:

This is cleared out in [PATCH 6/8].

-- 
Regards,
Karthik Nayak

On Sun, Sep 13, 2015 at 8:12 AM, Matthieu Moy
[off-list ref] wrote:

Karthik Nayak [off-list ref] writes:

quoted

@@ -679,15 +682,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 HEAD before sorting and printing the rest */
+     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;
+     }

I think Eric already mentionned it, but I don't remember the conclusion
and can't find it in the archives. Wouldn't it be cleaner to actually
remove the detached head from the array (do "ref_list.index -= 1"
instead of "index -= 1", and possibly free() what needs to be freed?

I think Michael Haggerty mentioned something along those lines...

On Sun, Sep 13, 2015 at 12:46 PM, Eric Sunshine [off-list ref] wrote:

On Sun, Sep 13, 2015 at 8:12 AM, Matthieu Moy
[off-list ref] wrote:

quoted

Karthik Nayak [off-list ref] writes:

quoted

@@ -679,15 +682,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 HEAD before sorting and printing the rest */
+     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;
+     }

I think Eric already mentionned it, but I don't remember the conclusion
and can't find it in the archives. Wouldn't it be cleaner to actually
remove the detached head from the array (do "ref_list.index -= 1"
instead of "index -= 1", and possibly free() what needs to be freed?

I think Michael Haggerty mentioned something along those lines...

Specifically, I think you're referring to [1] (?).

[1]: http://thread.gmane.org/gmane.comp.version-control.git/276363/focus=276676

On Mon, Sep 14, 2015 at 12:01 AM, Eric Sunshine [off-list ref] wrote:

On Sun, Sep 13, 2015 at 12:46 PM, Eric Sunshine [off-list ref] wrote:

quoted

On Sun, Sep 13, 2015 at 8:12 AM, Matthieu Moy
[off-list ref] wrote:

quoted

Karthik Nayak [off-list ref] writes:

quoted

@@ -679,15 +682,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 HEAD before sorting and printing the rest */
+     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;
+     }

I think Eric already mentionned it, but I don't remember the conclusion
and can't find it in the archives. Wouldn't it be cleaner to actually
remove the detached head from the array (do "ref_list.index -= 1"
instead of "index -= 1", and possibly free() what needs to be freed?

I think Michael Haggerty mentioned something along those lines...

Specifically, I think you're referring to [1] (?).

[1]: http://thread.gmane.org/gmane.comp.version-control.git/276363/focus=276676

No not that, that is handled in the previous patch series.

I can't find the reference either, but the comment was along the lines of what
Matthieu just mentioned above, But like I replied on [Patch 6/8] Its
taken care of
in that particular patch. Here it doesn't seem to be needed.

-- 
Regards,
Karthik Nayak