Re: [PATCH v2 3/3] fetch: reduce duplicate in ref update status lines

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

+static int common_suffix_length(const char *a, const char *b)
+{
+	const char *pa = a + strlen(a);
+	const char *pb = b + strlen(b);
+	int count = 0;
+
+	while (pa > a && pb > b && pa[-1] == pb[-1]) {
+		pa--;
+		pb--;
+		count++;
+	}
+
+	/* stick to '/' boundary, do not break in the middle of a word */
+	while (count) {
+		if (*pa == '/' ||
+		    (pa == a && pb > b && pb[-1] == '/') ||
+		    (pb == b && pa > a && pa[-1] == '/'))
+			break;
+		pa++;
+		pb++;
+		count--;
+	}
+
+	return count;
+}
+

Why do you need two loops, one going backward from the tail and then
going forward toward '/'?  Wouldn't it be sufficient to keep track
of the last slash you saw in a while scanning backwards?  I.e
something along the lines of:

	tail_a = a + strlen(a);
	for (pa = tail_a, pb = b + strlen(b), slash_in_a = NULL;
             a < pa && b < pb && pa[-1] == pb[-1];
	     pa--, pb--) {
		if (*pa == '/')
			slash_in_a = pa;
	}
	count = a + strlen(a) - slash_in_a;
		
perhaps?

These are somewhat cryptic ;-)

Other than that, the patch looks OK.

Thanks.

On Sat, Jun 4, 2016 at 12:00 AM, Junio C Hamano [off-list ref] wrote:

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

+static int common_suffix_length(const char *a, const char *b)
+{
+     const char *pa = a + strlen(a);
+     const char *pb = b + strlen(b);
+     int count = 0;
+
+     while (pa > a && pb > b && pa[-1] == pb[-1]) {
+             pa--;
+             pb--;
+             count++;
+     }
+
+     /* stick to '/' boundary, do not break in the middle of a word */
+     while (count) {
+             if (*pa == '/' ||
+                 (pa == a && pb > b && pb[-1] == '/') ||
+                 (pb == b && pa > a && pa[-1] == '/'))
+                     break;
+             pa++;
+             pb++;
+             count--;
+     }
+
+     return count;
+}
+

Why do you need two loops, one going backward from the tail and then
going forward toward '/'?

I wanted to check something else, then settled for slashes, but the
two loops remained.

Wouldn't it be sufficient to keep track
of the last slash you saw in a while scanning backwards?  I.e
something along the lines of:

        tail_a = a + strlen(a);
        for (pa = tail_a, pb = b + strlen(b), slash_in_a = NULL;
             a < pa && b < pb && pa[-1] == pb[-1];
             pa--, pb--) {
                if (*pa == '/')
                        slash_in_a = pa;
        }
        count = a + strlen(a) - slash_in_a;

perhaps?

Actually, Jeff's suggestion about common prefix makes me realize, why
not reuse diff.c:pprint_rename()? Your {a -> b} idea came from that.
We may need some tweaking there because right now it will not show {
-> origin/}master.

quoted

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 454d896..9a7649c 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh

@@ -222,11 +222,11 @@ test_expect_success 'fetch uses remote ref names to describe new refs' '
      (
              cd descriptive &&
              git fetch o 2>actual &&
-             grep " -> refs/crazyheads/descriptive-branch$" actual |
+             grep " -> refs/crazyheads/.descriptive-branch$" actual |
              test_i18ngrep "new branch" &&
              grep " -> descriptive-tag$" actual |
              test_i18ngrep "new tag" &&
-             grep " -> crazy$" actual |
+             grep " -> .crazy$" actual |
              test_i18ngrep "new ref"
      ) &&

These are somewhat cryptic ;-)

Yeah... too lazy to check if } needs to be quoted or not :D
-- 
Duy