| Message ID | 20190125095122.28719-1-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fetch: prefer suffix substitution in compact fetch.output | expand |
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;
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 "*".
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?
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 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.
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.
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;
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(-)