[PATCH] fetch: prefer suffix substitution in compact fetch.output

[PATCH] fetch: prefer suffix substitution in compact fetch.output

[Nguyễn Thái Ngọc Duy]

I have a remote named "jch" and it has a branch with the same name. And
fetch.output is set to "compact". Fetching this remote looks like this

 From https://github.com/gitster/git
  + eb7fd39f6b...835363af2f jch                -> */jch  (forced update)
    6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*
  * [new branch]            nd/diff-parseopt   -> jch/*
  * [new branch]            nd/the-index-final -> jch/*

Notice that the local side of branch jch starts with "*" instead of
ending with it like the rest. It's not exactly wrong. It just looks
weird.

This patch changes the find-and-replace code a bit to try finding prefix
first before falling back to strstr() which finds a substring from left
to right. Now we have something less OCD

 From https://github.com/gitster/git
  + eb7fd39f6b...835363af2f jch                -> jch/*  (forced update)
    6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*
  * [new branch]            nd/diff-parseopt   -> jch/*
  * [new branch]            nd/the-index-final -> jch/*

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/fetch.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e0140327aa..e0173f8a33 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -629,9 +629,14 @@ static int find_and_replace(struct strbuf *haystack,
 			    const char *needle,
 			    const char *placeholder)
 {
-	const char *p = strstr(haystack->buf, needle);
+	const char *p = NULL;
 	int plen, nlen;
 
+	nlen = strlen(needle);
+	if (ends_with(haystack->buf, needle))
+		p = haystack->buf + haystack->len - nlen;
+	else
+		p = strstr(haystack->buf, needle);
 	if (!p)
 		return 0;
 
@@ -639,7 +644,6 @@ static int find_and_replace(struct strbuf *haystack,
 		return 0;
 
 	plen = strlen(p);
-	nlen = strlen(needle);
 	if (plen > nlen && p[nlen] != '/')
 		return 0;
 
-- 
2.20.1.560.g70ca8b83ee

Re: [PATCH] fetch: prefer suffix substitution in compact fetch.output

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> I have a remote named "jch" and it has a branch with the same name. And
> fetch.output is set to "compact". Fetching this remote looks like this
>
>  From https://github.com/gitster/git
>   + eb7fd39f6b...835363af2f jch                -> */jch  (forced update)
>     6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*
>   * [new branch]            nd/diff-parseopt   -> jch/*
>   * [new branch]            nd/the-index-final -> jch/*
>
> Notice that the local side of branch jch starts with "*" instead of
> ending with it like the rest. It's not exactly wrong. It just looks
> weird.
>
> This patch changes the find-and-replace code a bit to try finding prefix
> first before falling back to strstr() which finds a substring from left
> to right. Now we have something less OCD
>
>  From https://github.com/gitster/git
>   + eb7fd39f6b...835363af2f jch                -> jch/*  (forced update)
>     6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*
>   * [new branch]            nd/diff-parseopt   -> jch/*
>   * [new branch]            nd/the-index-final -> jch/*

Sounds good.  I do not think strstr() would ever be correct in this
application in the first place.  In what situation would it produce
a reasonable result, I wonder?

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/fetch.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index e0140327aa..e0173f8a33 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -629,9 +629,14 @@ static int find_and_replace(struct strbuf *haystack,
>  			    const char *needle,
>  			    const char *placeholder)
>  {
> -	const char *p = strstr(haystack->buf, needle);
> +	const char *p = NULL;
>  	int plen, nlen;
>  
> +	nlen = strlen(needle);
> +	if (ends_with(haystack->buf, needle))
> +		p = haystack->buf + haystack->len - nlen;
> +	else
> +		p = strstr(haystack->buf, needle);
>  	if (!p)
>  		return 0;
>  
> @@ -639,7 +644,6 @@ static int find_and_replace(struct strbuf *haystack,
>  		return 0;
>  
>  	plen = strlen(p);
> -	nlen = strlen(needle);
>  	if (plen > nlen && p[nlen] != '/')
>  		return 0;

Re: [PATCH] fetch: prefer suffix substitution in compact fetch.output

[Duy Nguyen]

On Sat, Jan 26, 2019 at 4:36 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
> > I have a remote named "jch" and it has a branch with the same name. And
> > fetch.output is set to "compact". Fetching this remote looks like this
> >
> >  From https://github.com/gitster/git
> >   + eb7fd39f6b...835363af2f jch                -> */jch  (forced update)
> >     6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*
> >   * [new branch]            nd/diff-parseopt   -> jch/*
> >   * [new branch]            nd/the-index-final -> jch/*
> >
> > Notice that the local side of branch jch starts with "*" instead of
> > ending with it like the rest. It's not exactly wrong. It just looks
> > weird.
> >
> > This patch changes the find-and-replace code a bit to try finding prefix
> > first before falling back to strstr() which finds a substring from left
> > to right. Now we have something less OCD
> >
> >  From https://github.com/gitster/git
> >   + eb7fd39f6b...835363af2f jch                -> jch/*  (forced update)
> >     6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*
> >   * [new branch]            nd/diff-parseopt   -> jch/*
> >   * [new branch]            nd/the-index-final -> jch/*
>
> Sounds good.  I do not think strstr() would ever be correct in this
> application in the first place.  In what situation would it produce
> a reasonable result, I wonder?

I think it's useful for github pull requests. The remote side is
usually pull/<id>/head but when mapping to a local ref I think we
often don't want a ref ending with "head", just pull/<id>. In this
case, strstr() can pick the middle part and substitute it with "*".
-- 
Duy

Ideas for even more compact fetch.output=compact output

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 25 2019, Nguyễn Thái Ngọc Duy wrote:

> I have a remote named "jch" and it has a branch with the same name. And
> fetch.output is set to "compact". Fetching this remote looks like this
>
>  From https://github.com/gitster/git
>   + eb7fd39f6b...835363af2f jch                -> */jch  (forced update)
>     6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*
>   * [new branch]            nd/diff-parseopt   -> jch/*
>   * [new branch]            nd/the-index-final -> jch/*
>
> Notice that the local side of branch jch starts with "*" instead of
> ending with it like the rest. It's not exactly wrong. It just looks
> weird.
>
> This patch changes the find-and-replace code a bit to try finding prefix
> first before falling back to strstr() which finds a substring from left
> to right. Now we have something less OCD
>
>  From https://github.com/gitster/git
>   + eb7fd39f6b...835363af2f jch                -> jch/*  (forced update)
>     6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*
>   * [new branch]            nd/diff-parseopt   -> jch/*
>   * [new branch]            nd/the-index-final -> jch/*

This patch works great. The existence of fetch.output=compact had
somehow passed me by until a few weeks ago, now using it and it looks
great. Thanks.

Just using this as a bounce-off point for a related discussion, one case
where I still see duplicates is things like:

    From github.com:git/git
       a7da99ff1b..28d0017056  next                -> origin/*
     + e911e946c2...9cc6aca6e9 pu                  -> origin/*  (forced update)
       a7da99ff1b..28d0017056  refs/pull/412/head  -> origin/pull/412/head
     + 1dbcd06490...6b1f08d3ef refs/pull/412/merge -> origin/pull/412/merge  (forced update)
     + e911e946c2...9cc6aca6e9 refs/pull/444/head  -> origin/pull/444/head  (forced update)
     + 8131760e3b...ed5bbbbcec refs/pull/444/merge -> origin/pull/444/merge  (forced update)

I.e. the duplicate strings for the "pull" namespace I'm fetching.

Now, there's no room with the current syntax to represent that
unambiguously, I started to patch it, but wasn't sure I liked the syntax
I came up with.

This is also one of the rare cases where bikeshedding the idea can
productively done without a patch so I thought I'd start that discussion
first.

If we had this:

    From github.com:git/git
       a7da99ff1b..28d0017056  next                -> origin/*
     + e911e946c2...9cc6aca6e9 pu                  -> origin/*  (forced update)
       a7da99ff1b..28d0017056  refs/[pull/412/head]  -> origin/*
     + 1dbcd06490...6b1f08d3ef refs/[pull/412/merge] -> origin/*  (forced update)
     + e911e946c2...9cc6aca6e9 refs/[pull/444/head]  -> origin/*  (forced update)
     + 8131760e3b...ed5bbbbcec refs/[pull/444/merge] -> origin/*  (forced update)

We could de-duplicate such output. I.e. used [] as "capture" delimiters
for the subsequent "*" since "[" and "]" aren't valid in ref names (but
"()" and "{}" are!).

Or maybe more generally using it consistently throughout, also for next/pu:

    From github.com:git/git
       a7da99ff1b..28d0017056  [next]                -> origin/*
     + e911e946c2...9cc6aca6e9 [pu]                  -> origin/*  (forced update)
       a7da99ff1b..28d0017056  refs/[pull/412/head]  -> origin/*
    [...]

The things that suck the most about this are that you can no longer
copy/paste the ref on the LHS as-is, and that it'll only show up in rare
cases, so it'll probably confuse even experienced users.

What do you think?

Re: Ideas for even more compact fetch.output=compact output

[Duy Nguyen]

On Fri, Feb 22, 2019 at 4:52 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Just using this as a bounce-off point for a related discussion, one case
> where I still see duplicates is things like:
>
>     From github.com:git/git
>        a7da99ff1b..28d0017056  next                -> origin/*
>      + e911e946c2...9cc6aca6e9 pu                  -> origin/*  (forced update)
>        a7da99ff1b..28d0017056  refs/pull/412/head  -> origin/pull/412/head
>      + 1dbcd06490...6b1f08d3ef refs/pull/412/merge -> origin/pull/412/merge  (forced update)
>      + e911e946c2...9cc6aca6e9 refs/pull/444/head  -> origin/pull/444/head  (forced update)
>      + 8131760e3b...ed5bbbbcec refs/pull/444/merge -> origin/pull/444/merge  (forced update)
>
> I.e. the duplicate strings for the "pull" namespace I'm fetching.
>
> Now, there's no room with the current syntax to represent that
> unambiguously, I started to patch it, but wasn't sure I liked the syntax
> I came up with.
>
> This is also one of the rare cases where bikeshedding the idea can
> productively done without a patch so I thought I'd start that discussion
> first.
>
> If we had this:
>
>     From github.com:git/git
>        a7da99ff1b..28d0017056  next                -> origin/*
>      + e911e946c2...9cc6aca6e9 pu                  -> origin/*  (forced update)
>        a7da99ff1b..28d0017056  refs/[pull/412/head]  -> origin/*
>      + 1dbcd06490...6b1f08d3ef refs/[pull/412/merge] -> origin/*  (forced update)
>      + e911e946c2...9cc6aca6e9 refs/[pull/444/head]  -> origin/*  (forced update)
>      + 8131760e3b...ed5bbbbcec refs/[pull/444/merge] -> origin/*  (forced update)
>
> We could de-duplicate such output. I.e. used [] as "capture" delimiters
> for the subsequent "*" since "[" and "]" aren't valid in ref names (but
> "()" and "{}" are!).

First impression, I think the square brackets makes it harder to read
the left column.

I was going to suggest coloring, which could be used to highlight the
common parts. But I think that would mess it up even more because it
kinda steals focus.

Another option is simply display refspec on the right hand side, e.g.

 refs/pull/412/merge -> refs/*:origin/*  (forced update)
 refs/pull/444/head  -> refs/*:origin/*  (forced update)
 refs/pull/444/merge -> refs/*:origin/*  (forced update)

This keeps the right column boring and mostly the same without losing
meaning, while the left column is left untouched. It does make you
think a bit to find out what the actual ref on the right hand side is
though.

> Or maybe more generally using it consistently throughout, also for next/pu:
>
>     From github.com:git/git
>        a7da99ff1b..28d0017056  [next]                -> origin/*
>      + e911e946c2...9cc6aca6e9 [pu]                  -> origin/*  (forced update)
>        a7da99ff1b..28d0017056  refs/[pull/412/head]  -> origin/*
>     [...]
>
> The things that suck the most about this are that you can no longer
> copy/paste the ref on the LHS as-is, and that it'll only show up in rare
> cases, so it'll probably confuse even experienced users.
>
> What do you think?



-- 
Duy

Re: Ideas for even more compact fetch.output=compact output

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>> If we had this:
>>
>>     From github.com:git/git
>>        a7da99ff1b..28d0017056  next                -> origin/*
>>      + e911e946c2...9cc6aca6e9 pu                  -> origin/*  (forced update)
>>        a7da99ff1b..28d0017056  refs/[pull/412/head]  -> origin/*
>>      + 1dbcd06490...6b1f08d3ef refs/[pull/412/merge] -> origin/*  (forced update)
>>      + e911e946c2...9cc6aca6e9 refs/[pull/444/head]  -> origin/*  (forced update)
>>      + 8131760e3b...ed5bbbbcec refs/[pull/444/merge] -> origin/*  (forced update)
>>
>> We could de-duplicate such output. I.e. used [] as "capture" delimiters
>> for the subsequent "*" since "[" and "]" aren't valid in ref names (but
>> "()" and "{}" are!).
>
> First impression, I think the square brackets makes it harder to read
> the left column.
>
> I was going to suggest coloring, which could be used to highlight the
> common parts. But I think that would mess it up even more because it
> kinda steals focus.
>
> Another option is simply display refspec on the right hand side, e.g.
>
>  refs/pull/412/merge -> refs/*:origin/*  (forced update)
>  refs/pull/444/head  -> refs/*:origin/*  (forced update)
>  refs/pull/444/merge -> refs/*:origin/*  (forced update)
>
> This keeps the right column boring and mostly the same without losing
> meaning, while the left column is left untouched. It does make you
> think a bit to find out what the actual ref on the right hand side is
> though.

None of the above, including the existing "origin/*" lets people cut
and paste the left-hand-side (which is what is available locally to
them) to a command line, e.g. after seeing

     From github.com:git/git
        a7da99ff1b..28d0017056  next                -> origin/*

you cannot append "origin/next" after "git log .." with a few
mouse-clicks.  As the actual object name after the update appear
with the double-dot, "git log ..28d0017056" is also hard to create
without dragging a7da99ff1b part from the output.

Having said that, I do not do pointy-and-clicky cut&paste myself, so
it would not bother me that much and any of these "compaction" ideas
may be OK.  Using the refmap notation would start bothering people
for perceived repetition of that right-hand-side, though.

Re: Ideas for even more compact fetch.output=compact output

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Duy Nguyen <pclouds@gmail.com> writes:
>
>>> If we had this:
>>>
>>>     From github.com:git/git
>>>        a7da99ff1b..28d0017056  next                -> origin/*
>>>      + e911e946c2...9cc6aca6e9 pu                  -> origin/*  (forced update)
>>>        a7da99ff1b..28d0017056  refs/[pull/412/head]  -> origin/*
>>>      + 1dbcd06490...6b1f08d3ef refs/[pull/412/merge] -> origin/*  (forced update)
>>>      + e911e946c2...9cc6aca6e9 refs/[pull/444/head]  -> origin/*  (forced update)
>>>      + 8131760e3b...ed5bbbbcec refs/[pull/444/merge] -> origin/*  (forced update)
>>>
>>> We could de-duplicate such output. I.e. used [] as "capture" delimiters
>>> for the subsequent "*" since "[" and "]" aren't valid in ref names (but
>>> "()" and "{}" are!).
>>
>> First impression, I think the square brackets makes it harder to read
>> the left column.
>>
>> I was going to suggest coloring, which could be used to highlight the
>> common parts. But I think that would mess it up even more because it
>> kinda steals focus.
>>
>> Another option is simply display refspec on the right hand side, e.g.
>>
>>  refs/pull/412/merge -> refs/*:origin/*  (forced update)
>>  refs/pull/444/head  -> refs/*:origin/*  (forced update)
>>  refs/pull/444/merge -> refs/*:origin/*  (forced update)
>>
>> This keeps the right column boring and mostly the same without losing
>> meaning, while the left column is left untouched. It does make you
>> think a bit to find out what the actual ref on the right hand side is
>> though.
>
> None of the above, including the existing "origin/*" lets people cut
> and paste the left-hand-side (which is what is available locally to

Aw, I meant right-hand-side (i.e. knife hand, not fork hand).

> them) to a command line, e.g. after seeing
>
>      From github.com:git/git
>         a7da99ff1b..28d0017056  next                -> origin/*
>
> you cannot append "origin/next" after "git log .." with a few
> mouse-clicks.  As the actual object name after the update appear
> with the double-dot, "git log ..28d0017056" is also hard to create
> without dragging a7da99ff1b part from the output.
>
> Having said that, I do not do pointy-and-clicky cut&paste myself, so
> it would not bother me that much and any of these "compaction" ideas
> may be OK.  Using the refmap notation would start bothering people
> for perceived repetition of that right-hand-side, though.