{"thread":{"id":"57992","subject":"[PATCH] remote: handle negative refspecs in git remote show","startedAt":"2022-06-14T00:32:57Z","lastAt":"2022-06-16T22:33:50Z","messageCount":11,"participants":["Jacob Keller","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"457157","messageId":"20220614003251.16765-1-jacob.e.keller@intel.com","threadId":"57992","inReplyTo":null,"subject":"[PATCH] remote: handle negative refspecs in git remote show","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2022-06-14T00:32:51Z","receivedAt":"2022-06-14T00:32:57Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"By default, the git remote show command will query data from remotes to\nshow data about what might be done on a future git fetch. This process\ncurrently does not handle negative refspecs. This can be confusing,\nbecause the show command will list refs as if they would be fetched. For\nexample if the fetch refspec \"^refs/heads/pr/*\", it still displays the\nfollowing:\n\n * remote jdk19\n Fetch URL: git@github.com:openjdk/jdk19.git\n Push URL: git@github.com:openjdk/jdk19.git\n HEAD branch: master\n Remote branches:\n master tracked\n pr/1 new (next fetch will store in remotes/jdk19)\n pr/2 new (next fetch will store in remotes/jdk19)\n pr/3 new (next fetch will store in remotes/jdk19)\n Local ref configured for 'git push':\n master pushes to master (fast-forwardable)\n\nFix this by checking negative refspecs inside of get_ref_states. For\neach ref which matches a negative refspec, copy it into a \"skipped\" list\nand remove it from the fetch map. This allows us to show the following\noutput instead:\n\n * remote jdk19\n Fetch URL: git@github.com:openjdk/jdk19.git\n Push URL: git@github.com:openjdk/jdk19.git\n HEAD branch: master\n Remote branches:\n master tracked\n pr/1 skipped\n pr/2 skipped\n pr/3 skipped\n Local ref configured for 'git push':\n master pushes to master (fast-forwardable)\n\nBy showing the refs as skipped, it helps clarify that these references\nwon't actually be fetched. Alternatively, we could simply remove them\nentirely.\n\nAdd a new test case to cover this functionality.\n\nReported-by: Pavel Rappo \nSigned-off-by: Jacob Keller \n---\n builtin/remote.c | 28 ++++++++++++++++++++++++++--\n remote.c | 2 +-\n remote.h | 6 ++++++\n t/t5505-remote.sh | 27 +++++++++++++++++++++++++++\n 4 files changed, 60 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex d4b69fe77898..243e60e19bdb 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -344,12 +344,13 @@ static void read_branches(void)\n \n struct ref_states {\n \tstruct remote *remote;\n-\tstruct string_list new_refs, stale, tracked, heads, push;\n+\tstruct string_list new_refs, skipped, stale, tracked, heads, push;\n \tint queried;\n };\n \n #define REF_STATES_INIT { \\\n \t.new_refs = STRING_LIST_INIT_DUP, \\\n+\t.skipped = STRING_LIST_INIT_DUP, \\\n \t.stale = STRING_LIST_INIT_DUP, \\\n \t.tracked = STRING_LIST_INIT_DUP, \\\n \t.heads = STRING_LIST_INIT_DUP, \\\n@@ -367,6 +368,24 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n \t\t\tdie(_(\"Could not get fetch map for refspec %s\"),\n \t\t\t\tstates->remote->fetch.raw[i]);\n \n+\t/* handle negative refspecs first */\n+\tfor (tail = &fetch_map; *tail; ) {\n+\t\tref = *tail;\n+\n+\t\tif (omit_name_by_refspec(ref->name, &states->remote->fetch)) {\n+\t\t\tstring_list_append(&states->skipped, abbrev_branch(ref->name));\n+\n+\t\t\t/* Matched a negative refspec, so remove this ref from\n+\t\t\t * consideration for being a new or tracked ref.\n+\t\t\t */\n+\t\t\t*tail = ref->next;\n+\t\t\tfree(ref->peer_ref);\n+\t\t\tfree(ref);\n+\t\t} else {\n+\t\t\ttail = &ref->next;\n+\t\t}\n+\t}\n+\n \tfor (ref = fetch_map; ref; ref = ref->next) {\n \t\tif (!ref->peer_ref || !ref_exists(ref->peer_ref->name))\n \t\t\tstring_list_append(&states->new_refs, abbrev_branch(ref->name));\n@@ -383,6 +402,7 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n \tfree_refs(fetch_map);\n \n \tstring_list_sort(&states->new_refs);\n+\tstring_list_sort(&states->skipped);\n \tstring_list_sort(&states->tracked);\n \tstring_list_sort(&states->stale);\n \n@@ -941,6 +961,7 @@ static void clear_push_info(void *util, const char *string)\n static void free_remote_ref_states(struct ref_states *states)\n {\n \tstring_list_clear(&states->new_refs, 0);\n+\tstring_list_clear(&states->skipped, 0);\n \tstring_list_clear(&states->stale, 1);\n \tstring_list_clear(&states->tracked, 0);\n \tstring_list_clear(&states->heads, 0);\n@@ -1033,7 +1054,9 @@ static int show_remote_info_item(struct string_list_item *item, void *cb_data)\n \t\tif (string_list_has_string(&states->new_refs, name)) {\n \t\t\tfmt = _(\" new (next fetch will store in remotes/%s)\");\n \t\t\targ = states->remote->name;\n-\t\t} else if (string_list_has_string(&states->tracked, name))\n+\t\t} else if (string_list_has_string(&states->skipped, name))\n+\t\t\targ = _(\" skipped\");\n+\t\telse if (string_list_has_string(&states->tracked, name))\n \t\t\targ = _(\" tracked\");\n \t\telse if (string_list_has_string(&states->stale, name))\n \t\t\targ = _(\" stale (use 'git remote prune' to remove)\");\n@@ -1308,6 +1331,7 @@ static int show(int argc, const char **argv)\n \t\t/* remote branch info */\n \t\tinfo.width = 0;\n \t\tfor_each_string_list(&info.states.new_refs, add_remote_to_show_info, &info);\n+\t\tfor_each_string_list(&info.states.skipped, add_remote_to_show_info, &info);\n \t\tfor_each_string_list(&info.states.tracked, add_remote_to_show_info, &info);\n \t\tfor_each_string_list(&info.states.stale, add_remote_to_show_info, &info);\n \t\tif (info.list.nr)\ndiff --git a/remote.c b/remote.c\nindex 404e1e0a0ddb..7d68b5632bb5 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -804,7 +804,7 @@ static int refspec_match(const struct refspec_item *refspec,\n \treturn !strcmp(refspec->src, name);\n }\n \n-static int omit_name_by_refspec(const char *name, struct refspec *rs)\n+int omit_name_by_refspec(const char *name, struct refspec *rs)\n {\n \tint i;\n \ndiff --git a/remote.h b/remote.h\nindex dd4402436f1f..448675e11259 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -247,6 +247,12 @@ int resolve_remote_symref(struct ref *ref, struct ref *list);\n */\n struct ref *ref_remove_duplicates(struct ref *ref_map);\n \n+/*\n+ * Check whether a name matches any negative refspec in rs. Returns 1 if the\n+ * name matches at least one negative refspec, and 0 otherwise.\n+ */\n+int omit_name_by_refspec(const char *name, struct refspec *rs);\n+\n /*\n * Remove all entries in the input list which match any negative refspec in\n * the refspec list.\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex fff14e13ed43..e19b8d666c73 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -302,6 +302,33 @@ test_expect_success 'show' '\n \t)\n '\n \n+cat >test/expect <output &&\n+\t\ttest_cmp expect output\n+\t)\n+'\n+\n cat >test/expect < Fix this by checking negative refspecs inside of get_ref_states. For\n> each ref which matches a negative refspec, copy it into a \"skipped\" list\n> and remove it from the fetch map. This allows us to show the following\n> output instead:\n\nSeems sensible.\n\n> +\t/* handle negative refspecs first */\n> +\tfor (tail = &fetch_map; *tail; ) {\n> +\t\tref = *tail;\n> +\n> +\t\tif (omit_name_by_refspec(ref->name, &states->remote->fetch)) {\n> +\t\t\tstring_list_append(&states->skipped, abbrev_branch(ref->name));\n> +\n> +\t\t\t/* Matched a negative refspec, so remove this ref from\n> +\t\t\t * consideration for being a new or tracked ref.\n> +\t\t\t */\n> +\t\t\t*tail = ref->next;\n> +\t\t\tfree(ref->peer_ref);\n> +\t\t\tfree(ref);\n> +\t\t} else {\n> +\t\t\ttail = &ref->next;\n> +\t\t}\n> +\t}\n> +\n\nNot being overly familiar with the \"git remote show\" code, this\nimplementation looks very reasonable to me. If we see a negative\nrefspec, we remove it from the fetch_map list and append it to the\nskipped list. Otherwise, we increment our pointer, and continue along\nuntil we reach the end of the list.\n\n> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> index fff14e13ed43..e19b8d666c73 100755\n> --- a/t/t5505-remote.sh\n> +++ b/t/t5505-remote.sh\n> @@ -302,6 +302,33 @@ test_expect_success 'show' '\n> \t)\n> '\n>\n> +cat >test/expect < +* remote origin\n> + Fetch URL: $(pwd)/one\n> + Push URL: $(pwd)/one\n> + HEAD branch: main\n> + Remote branches:\n> + main skipped\n> + side tracked\n> + upstream stale (use 'git remote prune' to remove)\n> + Local branches configured for 'git pull':\n> + ahead merges with remote main\n> + main merges with remote main\n> + Local refs configured for 'git push':\n> + main pushes to main (local out of date)\n> + main pushes to upstream (create)\n> +EOF\n> +\n> +test_expect_success 'show with negative refspecs' '\n> +\ttest_when_finished \"git -C test config --fixed-value --unset remote.origin.fetch ^refs/heads/main\" &&\n> +\t(\n> +\t\tcd test &&\n> +\t\tgit config --add remote.origin.fetch ^refs/heads/main &&\n\nDoing \"git config --unset\" outside of the subshell could be avoided by\nditching the subshell altogether, perhaps with something like:\n\n test_config -C test remote.origin.fetch ^refs/heads/main &&\n git -C test remote show origin >actual &&\n test_cmp test/expect actual\n\nThanks,\nTaylor\n"},{"id":"457165","messageId":"CA+P7+xrgWVh_GPMiziiMnsrESH_Z+HzB+D2un8FWSs0dpcDFdA@mail.gmail.com","threadId":"57992","inReplyTo":"Yqfec9yvT3LKomNK@nand.local","subject":"Re: [PATCH] remote: handle negative refspecs in git remote show","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2022-06-14T01:56:55Z","receivedAt":"2022-06-14T01:57:12Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Jun 13, 2022 at 6:03 PM Taylor Blau wrote:\n>\n> On Mon, Jun 13, 2022 at 05:32:51PM -0700, Jacob Keller wrote:\n> > Fix this by checking negative refspecs inside of get_ref_states. For\n> > each ref which matches a negative refspec, copy it into a \"skipped\" list\n> > and remove it from the fetch map. This allows us to show the following\n> > output instead:\n>\n> Seems sensible.\n>\n> > + /* handle negative refspecs first */\n> > + for (tail = &fetch_map; *tail; ) {\n> > + ref = *tail;\n> > +\n> > + if (omit_name_by_refspec(ref->name, &states->remote->fetch)) {\n> > + string_list_append(&states->skipped, abbrev_branch(ref->name));\n> > +\n> > + /* Matched a negative refspec, so remove this ref from\n> > + * consideration for being a new or tracked ref.\n> > + */\n> > + *tail = ref->next;\n> > + free(ref->peer_ref);\n> > + free(ref);\n> > + } else {\n> > + tail = &ref->next;\n> > + }\n> > + }\n> > +\n>\n> Not being overly familiar with the \"git remote show\" code, this\n> implementation looks very reasonable to me. If we see a negative\n> refspec, we remove it from the fetch_map list and append it to the\n> skipped list. Otherwise, we increment our pointer, and continue along\n> until we reach the end of the list.\n>\n> > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> > index fff14e13ed43..e19b8d666c73 100755\n> > --- a/t/t5505-remote.sh\n> > +++ b/t/t5505-remote.sh\n> > @@ -302,6 +302,33 @@ test_expect_success 'show' '\n> > )\n> > '\n> >\n> > +cat >test/expect < > +* remote origin\n> > + Fetch URL: $(pwd)/one\n> > + Push URL: $(pwd)/one\n> > + HEAD branch: main\n> > + Remote branches:\n> > + main skipped\n> > + side tracked\n> > + upstream stale (use 'git remote prune' to remove)\n> > + Local branches configured for 'git pull':\n> > + ahead merges with remote main\n> > + main merges with remote main\n> > + Local refs configured for 'git push':\n> > + main pushes to main (local out of date)\n> > + main pushes to upstream (create)\n> > +EOF\n> > +\n> > +test_expect_success 'show with negative refspecs' '\n> > + test_when_finished \"git -C test config --fixed-value --unset remote.origin.fetch ^refs/heads/main\" &&\n> > + (\n> > + cd test &&\n> > + git config --add remote.origin.fetch ^refs/heads/main &&\n>\n> Doing \"git config --unset\" outside of the subshell could be avoided by\n> ditching the subshell altogether, perhaps with something like:\n>\n> test_config -C test remote.origin.fetch ^refs/heads/main &&\n\nWe need \"--add\" semantics here which test_config doesn't seem to\nsupport at the moment.\n\n> git -C test remote show origin >actual &&\n> test_cmp test/expect actual\n>\n> Thanks,\n> Taylor\n"},{"id":"457166","messageId":"Yqfx3P8qUpQ7Y0ux@nand.local","threadId":"57992","inReplyTo":"CA+P7+xrgWVh_GPMiziiMnsrESH_Z+HzB+D2un8FWSs0dpcDFdA@mail.gmail.com","subject":"Re: [PATCH] remote: handle negative refspecs in git remote show","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-14T02:26:36Z","receivedAt":"2022-06-14T02:49:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jun 13, 2022 at 06:56:55PM -0700, Jacob Keller wrote:\n> > > +test_expect_success 'show with negative refspecs' '\n> > > + test_when_finished \"git -C test config --fixed-value --unset remote.origin.fetch ^refs/heads/main\" &&\n> > > + (\n> > > + cd test &&\n> > > + git config --add remote.origin.fetch ^refs/heads/main &&\n> >\n> > Doing \"git config --unset\" outside of the subshell could be avoided by\n> > ditching the subshell altogether, perhaps with something like:\n> >\n> > test_config -C test remote.origin.fetch ^refs/heads/main &&\n>\n> We need \"--add\" semantics here which test_config doesn't seem to\n> support at the moment.\n\nMakes sense, thanks for explaining. This patch looks good to me.\n\nThanks,\nTaylor\n"},{"id":"457169","messageId":"CA+P7+xos3vpPjb3m_BkR4Qp3OPf+R+-A1B=jTD3-Q6FeO4RpMg@mail.gmail.com","threadId":"57992","inReplyTo":"Yqfec9yvT3LKomNK@nand.local","subject":"Re: [PATCH] remote: handle negative refspecs in git remote show","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2022-06-14T06:09:02Z","receivedAt":"2022-06-14T06:09:15Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Jun 13, 2022 at 6:03 PM Taylor Blau wrote:\n>\n> On Mon, Jun 13, 2022 at 05:32:51PM -0700, Jacob Keller wrote:\n> > Fix this by checking negative refspecs inside of get_ref_states. For\n> > each ref which matches a negative refspec, copy it into a \"skipped\" list\n> > and remove it from the fetch map. This allows us to show the following\n> > output instead:\n>\n> Seems sensible.\n>\n> > + /* handle negative refspecs first */\n> > + for (tail = &fetch_map; *tail; ) {\n> > + ref = *tail;\n> > +\n> > + if (omit_name_by_refspec(ref->name, &states->remote->fetch)) {\n> > + string_list_append(&states->skipped, abbrev_branch(ref->name));\n> > +\n> > + /* Matched a negative refspec, so remove this ref from\n> > + * consideration for being a new or tracked ref.\n> > + */\n> > + *tail = ref->next;\n> > + free(ref->peer_ref);\n> > + free(ref);\n> > + } else {\n> > + tail = &ref->next;\n> > + }\n> > + }\n> > +\n>\n> Not being overly familiar with the \"git remote show\" code, this\n> implementation looks very reasonable to me. If we see a negative\n> refspec, we remove it from the fetch_map list and append it to the\n> skipped list. Otherwise, we increment our pointer, and continue along\n> until we reach the end of the list.\n>\n\nThe specific way the loop works is similar to other ref looping code\nbut it feels a little odd to me. Still, it seems to be the right\napproach overall.\n\n> > +test_expect_success 'show with negative refspecs' '\n> > + test_when_finished \"git -C test config --fixed-value --unset remote.origin.fetch ^refs/heads/main\" &&\n> > + (\n> > + cd test &&\n> > + git config --add remote.origin.fetch ^refs/heads/main &&\n>\n> Doing \"git config --unset\" outside of the subshell could be avoided by\n> ditching the subshell altogether, perhaps with something like:\n>\n> test_config -C test remote.origin.fetch ^refs/heads/main &&\n> git -C test remote show origin >actual &&\n> test_cmp test/expect actual\n>\n\nI still think that removing the subshell is a good idea here. I'll\ninvestigate this.\n\nI also wonder if it would be difficult to enable \"--add\" semantics for\ntest_config.\n\n> Thanks,\n> Taylor\n"},{"id":"457322","messageId":"xmqqzgidy52c.fsf@gitster.g","threadId":"57992","inReplyTo":"20220614003251.16765-1-jacob.e.keller@intel.com","subject":"Re: [PATCH] remote: handle negative refspecs in git remote show","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-15T21:44:59Z","receivedAt":"2022-06-15T21:45:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> Fix this by checking negative refspecs inside of get_ref_states. For\n> each ref which matches a negative refspec, copy it into a \"skipped\" list\n> and remove it from the fetch map. This allows us to show the following\n> output instead:\n>\n> * remote jdk19\n> Fetch URL: git@github.com:openjdk/jdk19.git\n> Push URL: git@github.com:openjdk/jdk19.git\n> HEAD branch: master\n> Remote branches:\n> master tracked\n> pr/1 skipped\n> pr/2 skipped\n> pr/3 skipped\n> Local ref configured for 'git push':\n> master pushes to master (fast-forwardable)\n>\n> By showing the refs as skipped, it helps clarify that these references\n> won't actually be fetched. Alternatively, we could simply remove them\n> entirely.\n\nVery sensible.\n\n> @@ -367,6 +368,24 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n> \t\t\tdie(_(\"Could not get fetch map for refspec %s\"),\n> \t\t\t\tstates->remote->fetch.raw[i]);\n> \n> +\t/* handle negative refspecs first */\n> +\tfor (tail = &fetch_map; *tail; ) {\n> +\t\tref = *tail;\n> +\n> +\t\tif (omit_name_by_refspec(ref->name, &states->remote->fetch)) {\n> +\t\t\tstring_list_append(&states->skipped, abbrev_branch(ref->name));\n> +\n> +\t\t\t/* Matched a negative refspec, so remove this ref from\n> +\t\t\t * consideration for being a new or tracked ref.\n> +\t\t\t */\n> +\t\t\t*tail = ref->next;\n> +\t\t\tfree(ref->peer_ref);\n> +\t\t\tfree(ref);\n> +\t\t} else {\n> +\t\t\ttail = &ref->next;\n> +\t\t}\n> +\t}\n\n\nThis is somewhat curious. Do we really need to destroy the\nfetch_map like the above? I know by removing skipped items from the\nlist, the existing loop (below) can stop having to worry about them,\nbut the caller of get_ref_states() may later want to iterate over\nthe full fetch_map for other reasons (even if the current one does\nnot, a future version of the caller may have a reason to do so that\nwe do not know right now yet).\n\n> +\n> \tfor (ref = fetch_map; ref; ref = ref->next) {\n> \t\tif (!ref->peer_ref || !ref_exists(ref->peer_ref->name))\n> \t\t\tstring_list_append(&states->new_refs, abbrev_branch(ref->name));\n\nIOW, is adding a new condition to this existing loop insufficient?\n\n\tfor (ref = fetch_map; ref; ref = ref->next) {\n-\t\tif (!ref->peer_ref || !ref_exists(ref->peer_ref->name))\n+\t\tif (omit_name_by_refspec(ref->name, &states->remote->fetch))\n+\t\t\tstring_list_append(&states->skipped, abbrev_branch(ref->name));\n+\t\telse if (!ref->peer_ref || !ref_exists(ref->peer_ref->name))\n\t\t\tstring_list_append(&states->new_refs, abbrev_branch(ref->name));\n\t\telse\n\t\t\tstring_list_append(&states->tracked, abbrev_branch(ref->name));\n\t}\n\n\nThanks.\n"},{"id":"457407","messageId":"CA+P7+xpS0v9Oi3t4S76AcNRazdzJEEj6HxzO+z+dAFDPJ=yHJA@mail.gmail.com","threadId":"57992","inReplyTo":"xmqqzgidy52c.fsf@gitster.g","subject":"Re: [PATCH] remote: handle negative refspecs in git remote show","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2022-06-16T20:41:26Z","receivedAt":"2022-06-16T20:41:40Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Jun 15, 2022 at 2:45 PM Junio C Hamano wrote:\n>\n> Jacob Keller writes:\n>\n> > Fix this by checking negative refspecs inside of get_ref_states. For\n> > each ref which matches a negative refspec, copy it into a \"skipped\" list\n> > and remove it from the fetch map. This allows us to show the following\n> > output instead:\n> >\n> > * remote jdk19\n> > Fetch URL: git@github.com:openjdk/jdk19.git\n> > Push URL: git@github.com:openjdk/jdk19.git\n> > HEAD branch: master\n> > Remote branches:\n> > master tracked\n> > pr/1 skipped\n> > pr/2 skipped\n> > pr/3 skipped\n> > Local ref configured for 'git push':\n> > master pushes to master (fast-forwardable)\n> >\n> > By showing the refs as skipped, it helps clarify that these references\n> > won't actually be fetched. Alternatively, we could simply remove them\n> > entirely.\n>\n> Very sensible.\n>\n> > @@ -367,6 +368,24 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n> > die(_(\"Could not get fetch map for refspec %s\"),\n> > states->remote->fetch.raw[i]);\n> >\n> > + /* handle negative refspecs first */\n> > + for (tail = &fetch_map; *tail; ) {\n> > + ref = *tail;\n> > +\n> > + if (omit_name_by_refspec(ref->name, &states->remote->fetch)) {\n> > + string_list_append(&states->skipped, abbrev_branch(ref->name));\n> > +\n> > + /* Matched a negative refspec, so remove this ref from\n> > + * consideration for being a new or tracked ref.\n> > + */\n> > + *tail = ref->next;\n> > + free(ref->peer_ref);\n> > + free(ref);\n> > + } else {\n> > + tail = &ref->next;\n> > + }\n> > + }\n>\n>\n> This is somewhat curious. Do we really need to destroy the\n> fetch_map like the above? I know by removing skipped items from the\n> list, the existing loop (below) can stop having to worry about them,\n> but the caller of get_ref_states() may later want to iterate over\n> the full fetch_map for other reasons (even if the current one does\n> not, a future version of the caller may have a reason to do so that\n> we do not know right now yet).\n>\n\nGood point. I'll fix this. I think we can just move the\nomit_name_by_refspec into the other loop.\n\n> > +\n> > for (ref = fetch_map; ref; ref = ref->next) {\n> > if (!ref->peer_ref || !ref_exists(ref->peer_ref->name))\n> > string_list_append(&states->new_refs, abbrev_branch(ref->name));\n>\n> IOW, is adding a new condition to this existing loop insufficient?\n>\n\nThe tricky part here is that we don't have a simple check, and we're\ncurrently iterating over all of the refspecs each time. But we have to\ndo that regardless so I think this makes sense. Will fix.\n\n> for (ref = fetch_map; ref; ref = ref->next) {\n> - if (!ref->peer_ref || !ref_exists(ref->peer_ref->name))\n> + if (omit_name_by_refspec(ref->name, &states->remote->fetch))\n> + string_list_append(&states->skipped, abbrev_branch(ref->name));\n> + else if (!ref->peer_ref || !ref_exists(ref->peer_ref->name))\n> string_list_append(&states->new_refs, abbrev_branch(ref->name));\n> else\n> string_list_append(&states->tracked, abbrev_branch(ref->name));\n> }\n>\n>\n> Thanks.\n"},{"id":"457409","messageId":"CA+P7+xpzH_PiTKk4wb25pBJOCXwTE6_RYKhpRQj84gknNqV0Kg@mail.gmail.com","threadId":"57992","inReplyTo":"Yqfx3P8qUpQ7Y0ux@nand.local","subject":"Re: [PATCH] remote: handle negative refspecs in git remote show","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2022-06-16T20:48:19Z","receivedAt":"2022-06-16T20:49:28Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Jun 13, 2022 at 7:26 PM Taylor Blau wrote:\n>\n> On Mon, Jun 13, 2022 at 06:56:55PM -0700, Jacob Keller wrote:\n> > > > +test_expect_success 'show with negative refspecs' '\n> > > > + test_when_finished \"git -C test config --fixed-value --unset remote.origin.fetch ^refs/heads/main\" &&\n> > > > + (\n> > > > + cd test &&\n> > > > + git config --add remote.origin.fetch ^refs/heads/main &&\n> > >\n> > > Doing \"git config --unset\" outside of the subshell could be avoided by\n> > > ditching the subshell altogether, perhaps with something like:\n> > >\n> > > test_config -C test remote.origin.fetch ^refs/heads/main &&\n> >\n> > We need \"--add\" semantics here which test_config doesn't seem to\n> > support at the moment.\n>\n> Makes sense, thanks for explaining. This patch looks good to me.\n>\n> Thanks,\n> Taylor\n\nI ended up digging into this, and have now additionally added some\npatches to clean these tests up:\n\n1) drop all the subshells in t5505 where possible\n2) modify test_config and test_unconfig to handle options, including\n--global, --add, and --fixed-value\n3) make test_config use --fixed-value with test_unconfig so that only\nthe keys with the exact value are removed\n4) use test_config more so that we don't have config values from\nprevious tests kept around\n"},{"id":"457424","messageId":"xmqqletwqnse.fsf@gitster.g","threadId":"57992","inReplyTo":"CA+P7+xpS0v9Oi3t4S76AcNRazdzJEEj6HxzO+z+dAFDPJ=yHJA@mail.gmail.com","subject":"Re: [PATCH] remote: handle negative refspecs in git remote show","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T21:52:17Z","receivedAt":"2022-06-16T21:52:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n>> This is somewhat curious. Do we really need to destroy the\n>> fetch_map like the above? I know by removing skipped items from the\n>> list, the existing loop (below) can stop having to worry about them,\n>> but the caller of get_ref_states() may later want to iterate over\n>> the full fetch_map for other reasons (even if the current one does\n>> not, a future version of the caller may have a reason to do so that\n>> we do not know right now yet).\n>>\n>\n> Good point. I'll fix this. I think we can just move the\n> omit_name_by_refspec into the other loop.\n>\n>> > +\n>> > for (ref = fetch_map; ref; ref = ref->next) {\n>> > if (!ref->peer_ref || !ref_exists(ref->peer_ref->name))\n>> > string_list_append(&states->new_refs, abbrev_branch(ref->name));\n>>\n>> IOW, is adding a new condition to this existing loop insufficient?\n>>\n>\n> The tricky part here is that we don't have a simple check, and we're\n> currently iterating over all of the refspecs each time. But we have to\n> do that regardless so I think this makes sense. Will fix.\n\nAnother thing that worries me is that get_stale_heads() will not see\nthe filtered refs with your original implementation, because you cull\nthem from the fetch_map in the extra loop upfront.\n\nI do not know offhand what its effect would be, but it probably is\nworth testing. In your original scenario, if we locally have\nrefs/remotes/jdk19/old and refs/remotes/jdk19/pr/1 (perhaps obtained\nbefore we configured ^refs/pr/* negative refspec), we'd want to see\nthat pr/1 exists here but will not be updated.\n\n * remote jdk19\n Fetch URL: git@github.com:openjdk/jdk19.git\n Push URL: git@github.com:openjdk/jdk19.git\n HEAD branch: master\n Remote branches:\n master tracked\n old stale\n pr/1 stale\n pr/2 skipped\n pr/3 skipped\n Local ref configured for 'git push':\n master pushes to master (fast-forwardable)\n"},{"id":"457429","messageId":"CA+P7+xrx-Cec0mUJvDtWn0Fxu5vLdVkTbteWKiZyouH=64pWvA@mail.gmail.com","threadId":"57992","inReplyTo":"xmqqletwqnse.fsf@gitster.g","subject":"Re: [PATCH] remote: handle negative refspecs in git remote show","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2022-06-16T22:09:54Z","receivedAt":"2022-06-16T22:10:08Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Jun 16, 2022 at 2:52 PM Junio C Hamano wrote:\n>\n> Jacob Keller writes:\n>\n> >> This is somewhat curious. Do we really need to destroy the\n> >> fetch_map like the above? I know by removing skipped items from the\n> >> list, the existing loop (below) can stop having to worry about them,\n> >> but the caller of get_ref_states() may later want to iterate over\n> >> the full fetch_map for other reasons (even if the current one does\n> >> not, a future version of the caller may have a reason to do so that\n> >> we do not know right now yet).\n> >>\n> >\n> > Good point. I'll fix this. I think we can just move the\n> > omit_name_by_refspec into the other loop.\n> >\n> >> > +\n> >> > for (ref = fetch_map; ref; ref = ref->next) {\n> >> > if (!ref->peer_ref || !ref_exists(ref->peer_ref->name))\n> >> > string_list_append(&states->new_refs, abbrev_branch(ref->name));\n> >>\n> >> IOW, is adding a new condition to this existing loop insufficient?\n> >>\n> >\n> > The tricky part here is that we don't have a simple check, and we're\n> > currently iterating over all of the refspecs each time. But we have to\n> > do that regardless so I think this makes sense. Will fix.\n>\n> Another thing that worries me is that get_stale_heads() will not see\n> the filtered refs with your original implementation, because you cull\n> them from the fetch_map in the extra loop upfront.\n>\n\nI think the new implementation fixed this, but I'll see about adding a test!\n\n> I do not know offhand what its effect would be, but it probably is\n> worth testing. In your original scenario, if we locally have\n> refs/remotes/jdk19/old and refs/remotes/jdk19/pr/1 (perhaps obtained\n> before we configured ^refs/pr/* negative refspec), we'd want to see\n> that pr/1 exists here but will not be updated.\n>\n\nYea, I will see if I can check that.\n\n> * remote jdk19\n> Fetch URL: git@github.com:openjdk/jdk19.git\n> Push URL: git@github.com:openjdk/jdk19.git\n> HEAD branch: master\n> Remote branches:\n> master tracked\n> old stale\n> pr/1 stale\n> pr/2 skipped\n> pr/3 skipped\n> Local ref configured for 'git push':\n> master pushes to master (fast-forwardable)\n"},{"id":"457430","messageId":"CA+P7+xrxskcnoARjc5fL6zKys312B8XdQezrT7wHuN3wPaP-sQ@mail.gmail.com","threadId":"57992","inReplyTo":"CA+P7+xrx-Cec0mUJvDtWn0Fxu5vLdVkTbteWKiZyouH=64pWvA@mail.gmail.com","subject":"Re: [PATCH] remote: handle negative refspecs in git remote show","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2022-06-16T22:33:33Z","receivedAt":"2022-06-16T22:33:50Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Jun 16, 2022 at 3:09 PM Jacob Keller wrote:\n>\n> On Thu, Jun 16, 2022 at 2:52 PM Junio C Hamano wrote:\n> >\n> > Jacob Keller writes:\n> >\n> > >> This is somewhat curious. Do we really need to destroy the\n> > >> fetch_map like the above? I know by removing skipped items from the\n> > >> list, the existing loop (below) can stop having to worry about them,\n> > >> but the caller of get_ref_states() may later want to iterate over\n> > >> the full fetch_map for other reasons (even if the current one does\n> > >> not, a future version of the caller may have a reason to do so that\n> > >> we do not know right now yet).\n> > >>\n> > >\n> > > Good point. I'll fix this. I think we can just move the\n> > > omit_name_by_refspec into the other loop.\n> > >\n> > >> > +\n> > >> > for (ref = fetch_map; ref; ref = ref->next) {\n> > >> > if (!ref->peer_ref || !ref_exists(ref->peer_ref->name))\n> > >> > string_list_append(&states->new_refs, abbrev_branch(ref->name));\n> > >>\n> > >> IOW, is adding a new condition to this existing loop insufficient?\n> > >>\n> > >\n> > > The tricky part here is that we don't have a simple check, and we're\n> > > currently iterating over all of the refspecs each time. But we have to\n> > > do that regardless so I think this makes sense. Will fix.\n> >\n> > Another thing that worries me is that get_stale_heads() will not see\n> > the filtered refs with your original implementation, because you cull\n> > them from the fetch_map in the extra loop upfront.\n> >\n>\n> I think the new implementation fixed this, but I'll see about adding a test!\n>\n> > I do not know offhand what its effect would be, but it probably is\n> > worth testing. In your original scenario, if we locally have\n> > refs/remotes/jdk19/old and refs/remotes/jdk19/pr/1 (perhaps obtained\n> > before we configured ^refs/pr/* negative refspec), we'd want to see\n> > that pr/1 exists here but will not be updated.\n> >\n>\n> Yea, I will see if I can check that.\n>\n\nOk so this raises another issue: get_stale_heads currently never\ntreats a refname as stale if it matches a negative refspec. The\nfunction first queries the name against all refspecs via\nquery_refspecs_mutiple. It only considers a ref stale if it matches at\nleast one refspec but no longer exists in the remote.\n\nThe problem is that query_refspecs_multple returns no matches if the\nrefname matches any negative refspec. Thus, all refs matching a\nnegative refspec will be ignored by get_stale_heads.\n\nThe query_refspecs_multiple is only used by get_stale_heads_cb, so we\ncould change its implementation possibly, but that would make it not\nquite match the name. I'll try to think of a better solution.\n\nThanks,\nJake\n\n> > * remote jdk19\n> > Fetch URL: git@github.com:openjdk/jdk19.git\n> > Push URL: git@github.com:openjdk/jdk19.git\n> > HEAD branch: master\n> > Remote branches:\n> > master tracked\n> > old stale\n> > pr/1 stale\n> > pr/2 skipped\n> > pr/3 skipped\n> > Local ref configured for 'git push':\n> > master pushes to master (fast-forwardable)\n"}]}