{"thread":{"id":"50318","subject":"[PATCH] fetch: prefer suffix substitution in compact fetch.output","startedAt":"2019-01-25T09:51:33Z","lastAt":"2019-02-22T20:00:32Z","messageCount":7,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"367640","messageId":"20190125095122.28719-1-pclouds@gmail.com","threadId":"50318","inReplyTo":null,"subject":"[PATCH] fetch: prefer suffix substitution in compact fetch.output","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-25T09:51:22Z","receivedAt":"2019-01-25T09:51:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"I have a remote named \"jch\" and it has a branch with the same name. And\nfetch.output is set to \"compact\". Fetching this remote looks like this\n\n From https://github.com/gitster/git\n  + eb7fd39f6b...835363af2f jch                -> */jch  (forced update)\n    6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*\n  * [new branch]            nd/diff-parseopt   -> jch/*\n  * [new branch]            nd/the-index-final -> jch/*\n\nNotice that the local side of branch jch starts with \"*\" instead of\nending with it like the rest. It's not exactly wrong. It just looks\nweird.\n\nThis patch changes the find-and-replace code a bit to try finding prefix\nfirst before falling back to strstr() which finds a substring from left\nto right. Now we have something less OCD\n\n From https://github.com/gitster/git\n  + eb7fd39f6b...835363af2f jch                -> jch/*  (forced update)\n    6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*\n  * [new branch]            nd/diff-parseopt   -> jch/*\n  * [new branch]            nd/the-index-final -> jch/*\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/fetch.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e0140327aa..e0173f8a33 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -629,9 +629,14 @@ static int find_and_replace(struct strbuf *haystack,\n \t\t\t    const char *needle,\n \t\t\t    const char *placeholder)\n {\n-\tconst char *p = strstr(haystack->buf, needle);\n+\tconst char *p = NULL;\n \tint plen, nlen;\n \n+\tnlen = strlen(needle);\n+\tif (ends_with(haystack->buf, needle))\n+\t\tp = haystack->buf + haystack->len - nlen;\n+\telse\n+\t\tp = strstr(haystack->buf, needle);\n \tif (!p)\n \t\treturn 0;\n \n@@ -639,7 +644,6 @@ static int find_and_replace(struct strbuf *haystack,\n \t\treturn 0;\n \n \tplen = strlen(p);\n-\tnlen = strlen(needle);\n \tif (plen > nlen && p[nlen] != '/')\n \t\treturn 0;\n \n-- \n2.20.1.560.g70ca8b83ee\n\n"},{"id":"367685","messageId":"xmqqef90fmi6.fsf@gitster-ct.c.googlers.com","threadId":"50318","inReplyTo":"20190125095122.28719-1-pclouds@gmail.com","subject":"Re: [PATCH] fetch: prefer suffix substitution in compact fetch.output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-25T21:36:17Z","receivedAt":"2019-01-25T21:36:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> I have a remote named \"jch\" and it has a branch with the same name. And\n> fetch.output is set to \"compact\". Fetching this remote looks like this\n>\n>  From https://github.com/gitster/git\n>   + eb7fd39f6b...835363af2f jch                -> */jch  (forced update)\n>     6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*\n>   * [new branch]            nd/diff-parseopt   -> jch/*\n>   * [new branch]            nd/the-index-final -> jch/*\n>\n> Notice that the local side of branch jch starts with \"*\" instead of\n> ending with it like the rest. It's not exactly wrong. It just looks\n> weird.\n>\n> This patch changes the find-and-replace code a bit to try finding prefix\n> first before falling back to strstr() which finds a substring from left\n> to right. Now we have something less OCD\n>\n>  From https://github.com/gitster/git\n>   + eb7fd39f6b...835363af2f jch                -> jch/*  (forced update)\n>     6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*\n>   * [new branch]            nd/diff-parseopt   -> jch/*\n>   * [new branch]            nd/the-index-final -> jch/*\n\nSounds good.  I do not think strstr() would ever be correct in this\napplication in the first place.  In what situation would it produce\na reasonable result, I wonder?\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  builtin/fetch.c | 8 ++++++--\n>  1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index e0140327aa..e0173f8a33 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -629,9 +629,14 @@ static int find_and_replace(struct strbuf *haystack,\n>  \t\t\t    const char *needle,\n>  \t\t\t    const char *placeholder)\n>  {\n> -\tconst char *p = strstr(haystack->buf, needle);\n> +\tconst char *p = NULL;\n>  \tint plen, nlen;\n>  \n> +\tnlen = strlen(needle);\n> +\tif (ends_with(haystack->buf, needle))\n> +\t\tp = haystack->buf + haystack->len - nlen;\n> +\telse\n> +\t\tp = strstr(haystack->buf, needle);\n>  \tif (!p)\n>  \t\treturn 0;\n>  \n> @@ -639,7 +644,6 @@ static int find_and_replace(struct strbuf *haystack,\n>  \t\treturn 0;\n>  \n>  \tplen = strlen(p);\n> -\tnlen = strlen(needle);\n>  \tif (plen > nlen && p[nlen] != '/')\n>  \t\treturn 0;\n"},{"id":"367724","messageId":"CACsJy8CzKoq41o09vNsQ=56mAtND_EaHxYN1VgVeLVTo=U2TUA@mail.gmail.com","threadId":"50318","inReplyTo":"xmqqef90fmi6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch: prefer suffix substitution in compact fetch.output","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-26T23:57:24Z","receivedAt":"2019-01-26T23:57:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 26, 2019 at 4:36 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n> > I have a remote named \"jch\" and it has a branch with the same name. And\n> > fetch.output is set to \"compact\". Fetching this remote looks like this\n> >\n> >  From https://github.com/gitster/git\n> >   + eb7fd39f6b...835363af2f jch                -> */jch  (forced update)\n> >     6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*\n> >   * [new branch]            nd/diff-parseopt   -> jch/*\n> >   * [new branch]            nd/the-index-final -> jch/*\n> >\n> > Notice that the local side of branch jch starts with \"*\" instead of\n> > ending with it like the rest. It's not exactly wrong. It just looks\n> > weird.\n> >\n> > This patch changes the find-and-replace code a bit to try finding prefix\n> > first before falling back to strstr() which finds a substring from left\n> > to right. Now we have something less OCD\n> >\n> >  From https://github.com/gitster/git\n> >   + eb7fd39f6b...835363af2f jch                -> jch/*  (forced update)\n> >     6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*\n> >   * [new branch]            nd/diff-parseopt   -> jch/*\n> >   * [new branch]            nd/the-index-final -> jch/*\n>\n> Sounds good.  I do not think strstr() would ever be correct in this\n> application in the first place.  In what situation would it produce\n> a reasonable result, I wonder?\n\nI think it's useful for github pull requests. The remote side is\nusually pull/<id>/head but when mapping to a local ref I think we\noften don't want a ref ending with \"head\", just pull/<id>. In this\ncase, strstr() can pick the middle part and substitute it with \"*\".\n-- \nDuy\n"},{"id":"369898","messageId":"875ztc5evt.fsf@evledraar.gmail.com","threadId":"50318","inReplyTo":"20190125095122.28719-1-pclouds@gmail.com","subject":"Ideas for even more compact fetch.output=compact output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-22T09:52:38Z","receivedAt":"2019-02-22T09:52:43Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 25 2019, Nguyễn Thái Ngọc Duy wrote:\n\n> I have a remote named \"jch\" and it has a branch with the same name. And\n> fetch.output is set to \"compact\". Fetching this remote looks like this\n>\n>  From https://github.com/gitster/git\n>   + eb7fd39f6b...835363af2f jch                -> */jch  (forced update)\n>     6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*\n>   * [new branch]            nd/diff-parseopt   -> jch/*\n>   * [new branch]            nd/the-index-final -> jch/*\n>\n> Notice that the local side of branch jch starts with \"*\" instead of\n> ending with it like the rest. It's not exactly wrong. It just looks\n> weird.\n>\n> This patch changes the find-and-replace code a bit to try finding prefix\n> first before falling back to strstr() which finds a substring from left\n> to right. Now we have something less OCD\n>\n>  From https://github.com/gitster/git\n>   + eb7fd39f6b...835363af2f jch                -> jch/*  (forced update)\n>     6f11fd5edb..59b12ae96a  nd/config-move-to  -> jch/*\n>   * [new branch]            nd/diff-parseopt   -> jch/*\n>   * [new branch]            nd/the-index-final -> jch/*\n\nThis patch works great. The existence of fetch.output=compact had\nsomehow passed me by until a few weeks ago, now using it and it looks\ngreat. Thanks.\n\nJust using this as a bounce-off point for a related discussion, one case\nwhere I still see duplicates is things like:\n\n    From github.com:git/git\n       a7da99ff1b..28d0017056  next                -> origin/*\n     + e911e946c2...9cc6aca6e9 pu                  -> origin/*  (forced update)\n       a7da99ff1b..28d0017056  refs/pull/412/head  -> origin/pull/412/head\n     + 1dbcd06490...6b1f08d3ef refs/pull/412/merge -> origin/pull/412/merge  (forced update)\n     + e911e946c2...9cc6aca6e9 refs/pull/444/head  -> origin/pull/444/head  (forced update)\n     + 8131760e3b...ed5bbbbcec refs/pull/444/merge -> origin/pull/444/merge  (forced update)\n\nI.e. the duplicate strings for the \"pull\" namespace I'm fetching.\n\nNow, there's no room with the current syntax to represent that\nunambiguously, I started to patch it, but wasn't sure I liked the syntax\nI came up with.\n\nThis is also one of the rare cases where bikeshedding the idea can\nproductively done without a patch so I thought I'd start that discussion\nfirst.\n\nIf we had this:\n\n    From github.com:git/git\n       a7da99ff1b..28d0017056  next                -> origin/*\n     + e911e946c2...9cc6aca6e9 pu                  -> origin/*  (forced update)\n       a7da99ff1b..28d0017056  refs/[pull/412/head]  -> origin/*\n     + 1dbcd06490...6b1f08d3ef refs/[pull/412/merge] -> origin/*  (forced update)\n     + e911e946c2...9cc6aca6e9 refs/[pull/444/head]  -> origin/*  (forced update)\n     + 8131760e3b...ed5bbbbcec refs/[pull/444/merge] -> origin/*  (forced update)\n\nWe could de-duplicate such output. I.e. used [] as \"capture\" delimiters\nfor the subsequent \"*\" since \"[\" and \"]\" aren't valid in ref names (but\n\"()\" and \"{}\" are!).\n\nOr maybe more generally using it consistently throughout, also for next/pu:\n\n    From github.com:git/git\n       a7da99ff1b..28d0017056  [next]                -> origin/*\n     + e911e946c2...9cc6aca6e9 [pu]                  -> origin/*  (forced update)\n       a7da99ff1b..28d0017056  refs/[pull/412/head]  -> origin/*\n    [...]\n\nThe things that suck the most about this are that you can no longer\ncopy/paste the ref on the LHS as-is, and that it'll only show up in rare\ncases, so it'll probably confuse even experienced users.\n\nWhat do you think?\n"},{"id":"369899","messageId":"CACsJy8DJvxuhVJBpgTNiFHt+0ZOFqm4agvxrJfa2D1jvDojaRQ@mail.gmail.com","threadId":"50318","inReplyTo":"875ztc5evt.fsf@evledraar.gmail.com","subject":"Re: Ideas for even more compact fetch.output=compact output","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-22T10:05:56Z","receivedAt":"2019-02-22T10:06:25Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 22, 2019 at 4:52 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> Just using this as a bounce-off point for a related discussion, one case\n> where I still see duplicates is things like:\n>\n>     From github.com:git/git\n>        a7da99ff1b..28d0017056  next                -> origin/*\n>      + e911e946c2...9cc6aca6e9 pu                  -> origin/*  (forced update)\n>        a7da99ff1b..28d0017056  refs/pull/412/head  -> origin/pull/412/head\n>      + 1dbcd06490...6b1f08d3ef refs/pull/412/merge -> origin/pull/412/merge  (forced update)\n>      + e911e946c2...9cc6aca6e9 refs/pull/444/head  -> origin/pull/444/head  (forced update)\n>      + 8131760e3b...ed5bbbbcec refs/pull/444/merge -> origin/pull/444/merge  (forced update)\n>\n> I.e. the duplicate strings for the \"pull\" namespace I'm fetching.\n>\n> Now, there's no room with the current syntax to represent that\n> unambiguously, I started to patch it, but wasn't sure I liked the syntax\n> I came up with.\n>\n> This is also one of the rare cases where bikeshedding the idea can\n> productively done without a patch so I thought I'd start that discussion\n> first.\n>\n> If we had this:\n>\n>     From github.com:git/git\n>        a7da99ff1b..28d0017056  next                -> origin/*\n>      + e911e946c2...9cc6aca6e9 pu                  -> origin/*  (forced update)\n>        a7da99ff1b..28d0017056  refs/[pull/412/head]  -> origin/*\n>      + 1dbcd06490...6b1f08d3ef refs/[pull/412/merge] -> origin/*  (forced update)\n>      + e911e946c2...9cc6aca6e9 refs/[pull/444/head]  -> origin/*  (forced update)\n>      + 8131760e3b...ed5bbbbcec refs/[pull/444/merge] -> origin/*  (forced update)\n>\n> We could de-duplicate such output. I.e. used [] as \"capture\" delimiters\n> for the subsequent \"*\" since \"[\" and \"]\" aren't valid in ref names (but\n> \"()\" and \"{}\" are!).\n\nFirst impression, I think the square brackets makes it harder to read\nthe left column.\n\nI was going to suggest coloring, which could be used to highlight the\ncommon parts. But I think that would mess it up even more because it\nkinda steals focus.\n\nAnother option is simply display refspec on the right hand side, e.g.\n\n refs/pull/412/merge -> refs/*:origin/*  (forced update)\n refs/pull/444/head  -> refs/*:origin/*  (forced update)\n refs/pull/444/merge -> refs/*:origin/*  (forced update)\n\nThis keeps the right column boring and mostly the same without losing\nmeaning, while the left column is left untouched. It does make you\nthink a bit to find out what the actual ref on the right hand side is\nthough.\n\n> Or maybe more generally using it consistently throughout, also for next/pu:\n>\n>     From github.com:git/git\n>        a7da99ff1b..28d0017056  [next]                -> origin/*\n>      + e911e946c2...9cc6aca6e9 [pu]                  -> origin/*  (forced update)\n>        a7da99ff1b..28d0017056  refs/[pull/412/head]  -> origin/*\n>     [...]\n>\n> The things that suck the most about this are that you can no longer\n> copy/paste the ref on the LHS as-is, and that it'll only show up in rare\n> cases, so it'll probably confuse even experienced users.\n>\n> What do you think?\n\n\n\n-- \nDuy\n"},{"id":"369963","messageId":"xmqqftsfr8hw.fsf@gitster-ct.c.googlers.com","threadId":"50318","inReplyTo":"CACsJy8DJvxuhVJBpgTNiFHt+0ZOFqm4agvxrJfa2D1jvDojaRQ@mail.gmail.com","subject":"Re: Ideas for even more compact fetch.output=compact output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-22T18:19:39Z","receivedAt":"2019-02-22T18:19:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n>> If we had this:\n>>\n>>     From github.com:git/git\n>>        a7da99ff1b..28d0017056  next                -> origin/*\n>>      + e911e946c2...9cc6aca6e9 pu                  -> origin/*  (forced update)\n>>        a7da99ff1b..28d0017056  refs/[pull/412/head]  -> origin/*\n>>      + 1dbcd06490...6b1f08d3ef refs/[pull/412/merge] -> origin/*  (forced update)\n>>      + e911e946c2...9cc6aca6e9 refs/[pull/444/head]  -> origin/*  (forced update)\n>>      + 8131760e3b...ed5bbbbcec refs/[pull/444/merge] -> origin/*  (forced update)\n>>\n>> We could de-duplicate such output. I.e. used [] as \"capture\" delimiters\n>> for the subsequent \"*\" since \"[\" and \"]\" aren't valid in ref names (but\n>> \"()\" and \"{}\" are!).\n>\n> First impression, I think the square brackets makes it harder to read\n> the left column.\n>\n> I was going to suggest coloring, which could be used to highlight the\n> common parts. But I think that would mess it up even more because it\n> kinda steals focus.\n>\n> Another option is simply display refspec on the right hand side, e.g.\n>\n>  refs/pull/412/merge -> refs/*:origin/*  (forced update)\n>  refs/pull/444/head  -> refs/*:origin/*  (forced update)\n>  refs/pull/444/merge -> refs/*:origin/*  (forced update)\n>\n> This keeps the right column boring and mostly the same without losing\n> meaning, while the left column is left untouched. It does make you\n> think a bit to find out what the actual ref on the right hand side is\n> though.\n\nNone of the above, including the existing \"origin/*\" lets people cut\nand paste the left-hand-side (which is what is available locally to\nthem) to a command line, e.g. after seeing\n\n     From github.com:git/git\n        a7da99ff1b..28d0017056  next                -> origin/*\n\nyou cannot append \"origin/next\" after \"git log ..\" with a few\nmouse-clicks.  As the actual object name after the update appear\nwith the double-dot, \"git log ..28d0017056\" is also hard to create\nwithout dragging a7da99ff1b part from the output.\n\nHaving said that, I do not do pointy-and-clicky cut&paste myself, so\nit would not bother me that much and any of these \"compaction\" ideas\nmay be OK.  Using the refmap notation would start bothering people\nfor perceived repetition of that right-hand-side, though.\n"},{"id":"369970","messageId":"xmqqr2bzpp9i.fsf@gitster-ct.c.googlers.com","threadId":"50318","inReplyTo":"xmqqftsfr8hw.fsf@gitster-ct.c.googlers.com","subject":"Re: Ideas for even more compact fetch.output=compact output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-22T20:00:25Z","receivedAt":"2019-02-22T20:00:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>>> If we had this:\n>>>\n>>>     From github.com:git/git\n>>>        a7da99ff1b..28d0017056  next                -> origin/*\n>>>      + e911e946c2...9cc6aca6e9 pu                  -> origin/*  (forced update)\n>>>        a7da99ff1b..28d0017056  refs/[pull/412/head]  -> origin/*\n>>>      + 1dbcd06490...6b1f08d3ef refs/[pull/412/merge] -> origin/*  (forced update)\n>>>      + e911e946c2...9cc6aca6e9 refs/[pull/444/head]  -> origin/*  (forced update)\n>>>      + 8131760e3b...ed5bbbbcec refs/[pull/444/merge] -> origin/*  (forced update)\n>>>\n>>> We could de-duplicate such output. I.e. used [] as \"capture\" delimiters\n>>> for the subsequent \"*\" since \"[\" and \"]\" aren't valid in ref names (but\n>>> \"()\" and \"{}\" are!).\n>>\n>> First impression, I think the square brackets makes it harder to read\n>> the left column.\n>>\n>> I was going to suggest coloring, which could be used to highlight the\n>> common parts. But I think that would mess it up even more because it\n>> kinda steals focus.\n>>\n>> Another option is simply display refspec on the right hand side, e.g.\n>>\n>>  refs/pull/412/merge -> refs/*:origin/*  (forced update)\n>>  refs/pull/444/head  -> refs/*:origin/*  (forced update)\n>>  refs/pull/444/merge -> refs/*:origin/*  (forced update)\n>>\n>> This keeps the right column boring and mostly the same without losing\n>> meaning, while the left column is left untouched. It does make you\n>> think a bit to find out what the actual ref on the right hand side is\n>> though.\n>\n> None of the above, including the existing \"origin/*\" lets people cut\n> and paste the left-hand-side (which is what is available locally to\n\nAw, I meant right-hand-side (i.e. knife hand, not fork hand).\n\n> them) to a command line, e.g. after seeing\n>\n>      From github.com:git/git\n>         a7da99ff1b..28d0017056  next                -> origin/*\n>\n> you cannot append \"origin/next\" after \"git log ..\" with a few\n> mouse-clicks.  As the actual object name after the update appear\n> with the double-dot, \"git log ..28d0017056\" is also hard to create\n> without dragging a7da99ff1b part from the output.\n>\n> Having said that, I do not do pointy-and-clicky cut&paste myself, so\n> it would not bother me that much and any of these \"compaction\" ideas\n> may be OK.  Using the refmap notation would start bothering people\n> for perceived repetition of that right-hand-side, though.\n"}]}