{"thread":{"id":"35967","subject":"[PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs","startedAt":"2014-02-27T09:00:09Z","lastAt":"2014-03-24T19:24:12Z","messageCount":11,"participants":["Carlos Martín Nieto","Michael Haggerty","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"235445","messageId":"1393491610-19476-1-git-send-email-cmn@elego.de","threadId":"35967","inReplyTo":null,"subject":"[PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2014-02-27T09:00:09Z","receivedAt":"2014-02-27T09:00:09Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"From: Carlos Martín Nieto <cmn@dwim.me>\n\nWhen a remote has multiple fetch refspecs and these overlap in the\ntarget namespace, fetch may prune a remote-tracking branch which still\nexists in the remote. The test uses a popular form of this, by putting\npull requests as stored in a popular hosting platform alongside \"real\"\nremote-tracking branches.\n\nThe fetch command makes a decision of whether to prune based\non the first matching refspec, which in this case is insufficient, as it\ncovers the pull request names. This pair of refspecs does work as\nexpected if the more \"specific\" refspec is the first in the list.\n\nSigned-off-by: Carlos Martín Nieto <cmn@elego.de>\n---\n\nThis setup is used by GitHub for Windows, but nobody has noticed this\nbreak because it puts the PR refspec in the system config, which makes\nthat one the first. I was alerted to this by someone who had done this\nsetup manually and thus added the PR refspec after the default one.\n\n t/t5510-fetch.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 1f0f8e6..4949e3d 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -113,6 +113,26 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' '\n \tgit rev-parse origin/master\n '\n \n+test_expect_failure 'fetch --prune handles overlapping refspecs' '\n+\tcd \"$D\" &&\n+\tgit update-ref refs/pull/42/head master &&\n+\tgit clone . prune-overlapping &&\n+\tcd prune-overlapping &&\n+\tgit config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&\n+\n+\tgit fetch --prune origin &&\n+\tgit rev-parse origin/master &&\n+\tgit rev-parse origin/pr/42 &&\n+\n+\tgit config --unset-all remote.origin.fetch\n+\tgit config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&\n+\tgit config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* &&\n+\n+\tgit fetch --prune origin &&\n+\tgit rev-parse origin/master &&\n+\tgit rev-parse origin/pr/42\n+'\n+\n test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' '\n \tcd \"$D\" &&\n \tgit clone . prune-tags &&\n-- \n1.9.0.rc3.244.g3497008\n"},{"id":"235446","messageId":"1393491610-19476-2-git-send-email-cmn@elego.de","threadId":"35967","inReplyTo":"1393491610-19476-1-git-send-email-cmn@elego.de","subject":"[PATCH 2/2] fetch: handle overlaping refspecs on --prune","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2014-02-27T09:00:10Z","receivedAt":"2014-02-27T09:00:10Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"From: Carlos Martín Nieto <cmn@dwim.me>\n\nWe need to consider that a remote-tracking branch may match more than\none rhs of a fetch refspec. In such a case, it is not enough to stop at\nthe first match but look at all of the matches in order to determine\nwhether a head is stale.\n\nTo this goal, introduce a variant of query_refspecs which returns all of\nthe matching refspecs and loop over those answers to check for\nstaleness.\n\nSigned-off-by: Carlos Martín Nieto <cmn@elego.de>\n---\n\nThere is an unfortunate duplication of code here, as\nquery_refspecs_multiple is mostly query_refspecs but we only care\nabout the other side of matching refspecs and disregard the 'force'\ninformation which query_refspecs does want.\n\nI thought about putting both together via callbacks and having\nquery_refspecs stop at the first one, but I'm not sure that it would\nmake it easier to read or manage.\n\n remote.c         | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----\n t/t5510-fetch.sh |  2 +-\n 2 files changed, 48 insertions(+), 6 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 9f1a8aa..26140c7 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -821,6 +821,33 @@ static int match_name_with_pattern(const char *key, const char *name,\n \treturn ret;\n }\n \n+static void query_refspecs_multiple(struct refspec *refs, int ref_count, struct refspec *query, struct string_list *results)\n+{\n+\tint i;\n+\tint find_src = !query->src;\n+\n+\tif (find_src && !query->dst)\n+\t\terror(\"query_refspecs_multiple: need either src or dst\");\n+\n+\tfor (i = 0; i < ref_count; i++) {\n+\t\tstruct refspec *refspec = &refs[i];\n+\t\tconst char *key = find_src ? refspec->dst : refspec->src;\n+\t\tconst char *value = find_src ? refspec->src : refspec->dst;\n+\t\tconst char *needle = find_src ? query->dst : query->src;\n+\t\tchar **result = find_src ? &query->src : &query->dst;\n+\n+\t\tif (!refspec->dst)\n+\t\t\tcontinue;\n+\t\tif (refspec->pattern) {\n+\t\t\tif (match_name_with_pattern(key, needle, value, result)) {\n+\t\t\t\tstring_list_append_nodup(results, *result);\n+\t\t\t}\n+\t\t} else if (!strcmp(needle, key)) {\n+\t\t\tstring_list_append(results, value);\n+\t\t}\n+\t}\n+}\n+\n static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *query)\n {\n \tint i;\n@@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname,\n \tconst unsigned char *sha1, int flags, void *cb_data)\n {\n \tstruct stale_heads_info *info = cb_data;\n+\tstruct string_list matches = STRING_LIST_INIT_DUP;\n \tstruct refspec query;\n+\tint i, stale = 1;\n \tmemset(&query, 0, sizeof(struct refspec));\n \tquery.dst = (char *)refname;\n \n-\tif (query_refspecs(info->refs, info->ref_count, &query))\n+\tquery_refspecs_multiple(info->refs, info->ref_count, &query, &matches);\n+\tif (matches.nr == 0)\n \t\treturn 0; /* No matches */\n \n \t/*\n \t * If we did find a suitable refspec and it's not a symref and\n \t * it's not in the list of refs that currently exist in that\n-\t * remote we consider it to be stale.\n+\t * remote we consider it to be stale. In order to deal with\n+\t * overlapping refspecs, we need to go over all of the\n+\t * matching refs.\n \t */\n-\tif (!((flags & REF_ISSYMREF) ||\n-\t      string_list_has_string(info->ref_names, query.src))) {\n+\tif (flags & REF_ISSYMREF)\n+\t\treturn 0;\n+\n+\tfor (i = 0; i < matches.nr; i++) {\n+\t\tif (string_list_has_string(info->ref_names, matches.items[i].string)) {\n+\t\t\tstale = 0;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tstring_list_clear(&matches, 0);\n+\n+\tif (stale) {\n \t\tstruct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);\n \t\thashcpy(ref->new_sha1, sha1);\n \t}\n \n-\tfree(query.src);\n \treturn 0;\n }\n \ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 4949e3d..a86f6bc 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -113,7 +113,7 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' '\n \tgit rev-parse origin/master\n '\n \n-test_expect_failure 'fetch --prune handles overlapping refspecs' '\n+test_expect_success 'fetch --prune handles overlapping refspecs' '\n \tcd \"$D\" &&\n \tgit update-ref refs/pull/42/head master &&\n \tgit clone . prune-overlapping &&\n-- \n1.9.0.rc3.244.g3497008\n"},{"id":"235452","messageId":"530F11C1.7040407@alum.mit.edu","threadId":"35967","inReplyTo":"1393491610-19476-2-git-send-email-cmn@elego.de","subject":"Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-02-27T10:21:53Z","receivedAt":"2014-02-27T10:21:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/27/2014 10:00 AM, Carlos Martín Nieto wrote:\n> From: Carlos Martín Nieto <cmn@dwim.me>\n> \n> We need to consider that a remote-tracking branch may match more than\n> one rhs of a fetch refspec. In such a case, it is not enough to stop at\n> the first match but look at all of the matches in order to determine\n> whether a head is stale.\n> \n> To this goal, introduce a variant of query_refspecs which returns all of\n> the matching refspecs and loop over those answers to check for\n> staleness.\n> \n> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>\n> ---\n> \n> There is an unfortunate duplication of code here, as\n> query_refspecs_multiple is mostly query_refspecs but we only care\n> about the other side of matching refspecs and disregard the 'force'\n> information which query_refspecs does want.\n> \n> I thought about putting both together via callbacks and having\n> query_refspecs stop at the first one, but I'm not sure that it would\n> make it easier to read or manage.\n> \n>  remote.c         | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----\n>  t/t5510-fetch.sh |  2 +-\n>  2 files changed, 48 insertions(+), 6 deletions(-)\n> \n> diff --git a/remote.c b/remote.c\n> index 9f1a8aa..26140c7 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -821,6 +821,33 @@ static int match_name_with_pattern(const char *key, const char *name,\n>  \treturn ret;\n>  }\n>  \n> +static void query_refspecs_multiple(struct refspec *refs, int ref_count, struct refspec *query, struct string_list *results)\n> +{\n> +\tint i;\n> +\tint find_src = !query->src;\n> +\n> +\tif (find_src && !query->dst)\n> +\t\terror(\"query_refspecs_multiple: need either src or dst\");\n> +\n> +\tfor (i = 0; i < ref_count; i++) {\n> +\t\tstruct refspec *refspec = &refs[i];\n> +\t\tconst char *key = find_src ? refspec->dst : refspec->src;\n> +\t\tconst char *value = find_src ? refspec->src : refspec->dst;\n> +\t\tconst char *needle = find_src ? query->dst : query->src;\n> +\t\tchar **result = find_src ? &query->src : &query->dst;\n> +\n> +\t\tif (!refspec->dst)\n> +\t\t\tcontinue;\n> +\t\tif (refspec->pattern) {\n> +\t\t\tif (match_name_with_pattern(key, needle, value, result)) {\n> +\t\t\t\tstring_list_append_nodup(results, *result);\n> +\t\t\t}\n> +\t\t} else if (!strcmp(needle, key)) {\n> +\t\t\tstring_list_append(results, value);\n> +\t\t}\n> +\t}\n> +}\n> +\n>  static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *query)\n>  {\n>  \tint i;\n> @@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname,\n>  \tconst unsigned char *sha1, int flags, void *cb_data)\n>  {\n>  \tstruct stale_heads_info *info = cb_data;\n> +\tstruct string_list matches = STRING_LIST_INIT_DUP;\n>  \tstruct refspec query;\n> +\tint i, stale = 1;\n>  \tmemset(&query, 0, sizeof(struct refspec));\n>  \tquery.dst = (char *)refname;\n>  \n> -\tif (query_refspecs(info->refs, info->ref_count, &query))\n> +\tquery_refspecs_multiple(info->refs, info->ref_count, &query, &matches);\n> +\tif (matches.nr == 0)\n>  \t\treturn 0; /* No matches */\n>  \n>  \t/*\n>  \t * If we did find a suitable refspec and it's not a symref and\n>  \t * it's not in the list of refs that currently exist in that\n> -\t * remote we consider it to be stale.\n> +\t * remote we consider it to be stale. In order to deal with\n> +\t * overlapping refspecs, we need to go over all of the\n> +\t * matching refs.\n>  \t */\n> -\tif (!((flags & REF_ISSYMREF) ||\n> -\t      string_list_has_string(info->ref_names, query.src))) {\n> +\tif (flags & REF_ISSYMREF)\n> +\t\treturn 0;\n> +\n> +\tfor (i = 0; i < matches.nr; i++) {\n> +\t\tif (string_list_has_string(info->ref_names, matches.items[i].string)) {\n> +\t\t\tstale = 0;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tstring_list_clear(&matches, 0);\n> +\n> +\tif (stale) {\n>  \t\tstruct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);\n>  \t\thashcpy(ref->new_sha1, sha1);\n>  \t}\n>  \n> -\tfree(query.src);\n>  \treturn 0;\n>  }\n\nI didn't have time to review this fully, but I think you are missing\ncalls to string_list_clear(&matches) on a couple of code paths.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"235494","messageId":"1393529343.5277.3.camel@centaur.cmartin.tk","threadId":"35967","inReplyTo":"530F11C1.7040407@alum.mit.edu","subject":"Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2014-02-27T19:29:03Z","receivedAt":"2014-02-27T19:29:03Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Thu, 2014-02-27 at 11:21 +0100, Michael Haggerty wrote:\n> On 02/27/2014 10:00 AM, Carlos Martín Nieto wrote:\n> > From: Carlos Martín Nieto <cmn@dwim.me>\n> > \n> > We need to consider that a remote-tracking branch may match more than\n> > one rhs of a fetch refspec. In such a case, it is not enough to stop at\n> > the first match but look at all of the matches in order to determine\n> > whether a head is stale.\n> > \n> > To this goal, introduce a variant of query_refspecs which returns all of\n> > the matching refspecs and loop over those answers to check for\n> > staleness.\n> > \n> > Signed-off-by: Carlos Martín Nieto <cmn@elego.de>\n> > ---\n> > \n> > There is an unfortunate duplication of code here, as\n> > query_refspecs_multiple is mostly query_refspecs but we only care\n> > about the other side of matching refspecs and disregard the 'force'\n> > information which query_refspecs does want.\n> > \n> > I thought about putting both together via callbacks and having\n> > query_refspecs stop at the first one, but I'm not sure that it would\n> > make it easier to read or manage.\n> > \n> >  remote.c         | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----\n> >  t/t5510-fetch.sh |  2 +-\n> >  2 files changed, 48 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/remote.c b/remote.c\n> > index 9f1a8aa..26140c7 100644\n> > --- a/remote.c\n> > +++ b/remote.c\n> > @@ -821,6 +821,33 @@ static int match_name_with_pattern(const char *key, const char *name,\n> >  \treturn ret;\n> >  }\n> >  \n> > +static void query_refspecs_multiple(struct refspec *refs, int ref_count, struct refspec *query, struct string_list *results)\n> > +{\n> > +\tint i;\n> > +\tint find_src = !query->src;\n> > +\n> > +\tif (find_src && !query->dst)\n> > +\t\terror(\"query_refspecs_multiple: need either src or dst\");\n> > +\n> > +\tfor (i = 0; i < ref_count; i++) {\n> > +\t\tstruct refspec *refspec = &refs[i];\n> > +\t\tconst char *key = find_src ? refspec->dst : refspec->src;\n> > +\t\tconst char *value = find_src ? refspec->src : refspec->dst;\n> > +\t\tconst char *needle = find_src ? query->dst : query->src;\n> > +\t\tchar **result = find_src ? &query->src : &query->dst;\n> > +\n> > +\t\tif (!refspec->dst)\n> > +\t\t\tcontinue;\n> > +\t\tif (refspec->pattern) {\n> > +\t\t\tif (match_name_with_pattern(key, needle, value, result)) {\n> > +\t\t\t\tstring_list_append_nodup(results, *result);\n> > +\t\t\t}\n> > +\t\t} else if (!strcmp(needle, key)) {\n> > +\t\t\tstring_list_append(results, value);\n> > +\t\t}\n> > +\t}\n> > +}\n> > +\n> >  static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *query)\n> >  {\n> >  \tint i;\n> > @@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname,\n> >  \tconst unsigned char *sha1, int flags, void *cb_data)\n> >  {\n> >  \tstruct stale_heads_info *info = cb_data;\n> > +\tstruct string_list matches = STRING_LIST_INIT_DUP;\n> >  \tstruct refspec query;\n> > +\tint i, stale = 1;\n> >  \tmemset(&query, 0, sizeof(struct refspec));\n> >  \tquery.dst = (char *)refname;\n> >  \n> > -\tif (query_refspecs(info->refs, info->ref_count, &query))\n> > +\tquery_refspecs_multiple(info->refs, info->ref_count, &query, &matches);\n> > +\tif (matches.nr == 0)\n> >  \t\treturn 0; /* No matches */\n> >  \n> >  \t/*\n> >  \t * If we did find a suitable refspec and it's not a symref and\n> >  \t * it's not in the list of refs that currently exist in that\n> > -\t * remote we consider it to be stale.\n> > +\t * remote we consider it to be stale. In order to deal with\n> > +\t * overlapping refspecs, we need to go over all of the\n> > +\t * matching refs.\n> >  \t */\n> > -\tif (!((flags & REF_ISSYMREF) ||\n> > -\t      string_list_has_string(info->ref_names, query.src))) {\n> > +\tif (flags & REF_ISSYMREF)\n> > +\t\treturn 0;\n> > +\n> > +\tfor (i = 0; i < matches.nr; i++) {\n> > +\t\tif (string_list_has_string(info->ref_names, matches.items[i].string)) {\n> > +\t\t\tstale = 0;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tstring_list_clear(&matches, 0);\n> > +\n> > +\tif (stale) {\n> >  \t\tstruct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);\n> >  \t\thashcpy(ref->new_sha1, sha1);\n> >  \t}\n> >  \n> > -\tfree(query.src);\n> >  \treturn 0;\n> >  }\n> \n> I didn't have time to review this fully, but I think you are missing\n> calls to string_list_clear(&matches) on a couple of code paths.\n\nYep, you're right. I'll fix this and hold off new version for a bit to\nsee if there's more input. \n\n   cmn\n"},{"id":"235501","messageId":"CAPig+cRXhn=rX4YsBg0iBKcJ-F8Rp41w5MOeXKdyecwu5+6gNA@mail.gmail.com","threadId":"35967","inReplyTo":"1393491610-19476-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-02-27T20:18:35Z","receivedAt":"2014-02-27T20:18:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 27, 2014 at 4:00 AM, Carlos Martín Nieto <cmn@elego.de> wrote:\n> Subject: fetch: add a failing test for prunning with overlapping refspecs\n\ns/prunning/pruning/\n\n> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>\n> ---\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 1f0f8e6..4949e3d 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -113,6 +113,26 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' '\n>         git rev-parse origin/master\n>  '\n>\n> +test_expect_failure 'fetch --prune handles overlapping refspecs' '\n> +       cd \"$D\" &&\n> +       git update-ref refs/pull/42/head master &&\n> +       git clone . prune-overlapping &&\n> +       cd prune-overlapping &&\n> +       git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&\n> +\n> +       git fetch --prune origin &&\n> +       git rev-parse origin/master &&\n> +       git rev-parse origin/pr/42 &&\n> +\n> +       git config --unset-all remote.origin.fetch\n\nBroken &&-chain.\n\n> +       git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&\n> +       git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* &&\n> +\n> +       git fetch --prune origin &&\n> +       git rev-parse origin/master &&\n> +       git rev-parse origin/pr/42\n> +'\n> +\n>  test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' '\n>         cd \"$D\" &&\n>         git clone . prune-tags &&\n> --\n> 1.9.0.rc3.244.g3497008\n"},{"id":"235499","messageId":"CAPig+cTjvkNEfv8ThdPBVUXEGvZKAmhv9PvH1-sf4eJALUTHww@mail.gmail.com","threadId":"35967","inReplyTo":"1393491610-19476-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-02-27T20:19:27Z","receivedAt":"2014-02-27T20:19:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 27, 2014 at 4:00 AM, Carlos Martín Nieto <cmn@elego.de> wrote:\n> Subject: fetch: add a failing test for prunning with overlapping refspecs\n\ns/prunning/pruning/\n\n> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>\n> ---\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 1f0f8e6..4949e3d 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -113,6 +113,26 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' '\n>         git rev-parse origin/master\n>  '\n>\n> +test_expect_failure 'fetch --prune handles overlapping refspecs' '\n> +       cd \"$D\" &&\n> +       git update-ref refs/pull/42/head master &&\n> +       git clone . prune-overlapping &&\n> +       cd prune-overlapping &&\n> +       git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&\n> +\n> +       git fetch --prune origin &&\n> +       git rev-parse origin/master &&\n> +       git rev-parse origin/pr/42 &&\n> +\n> +       git config --unset-all remote.origin.fetch\n\nBroken &&-chain.\n\n> +       git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* &&\n> +       git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* &&\n> +\n> +       git fetch --prune origin &&\n> +       git rev-parse origin/master &&\n> +       git rev-parse origin/pr/42\n> +'\n> +\n>  test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' '\n>         cd \"$D\" &&\n>         git clone . prune-tags &&\n> --\n> 1.9.0.rc3.244.g3497008\n"},{"id":"235500","messageId":"xmqqob1sxq8v.fsf@gitster.dls.corp.google.com","threadId":"35967","inReplyTo":"1393491610-19476-2-git-send-email-cmn@elego.de","subject":"Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-27T20:19:44Z","receivedAt":"2014-02-27T20:19:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto <cmn@elego.de> writes:\n\n> From: Carlos Martín Nieto <cmn@dwim.me>\n>\n> We need to consider that a remote-tracking branch may match more than\n> one rhs of a fetch refspec.\n\nHmph, do we *need* to, really?\n\nDo you mean fetching one ref on the remote side and storing that in\nmultiple remote-tracking refs on our side?  What benefit does such\nan arrangement give the user?  When we \"git fetch $there $that_ref\"\nto obtain that single ref, do we update both remote-tracking refs?\nWhen the user asks \"git log $that_ref@{upstream}\", which one of two\nor more remote-tracking refs should we consult?  Should we report\nan error if these remote-tracking refs that are supposed to track\nthe same remote ref not all match?  Does \"git push $there $that_ref\"\nto update that remote ref update all of these remote-tracking refs\non our side?  Should it?\n\nMy knee-jerk reaction is that it may not be worth supporting such an\narrangement as broken (we may even want to diagnose it as an error),\nbut assuming we do need to, the approach to solve it, i.e. this...\n\n> In such a case, it is not enough to stop at\n> the first match but look at all of the matches in order to determine\n> whether a head is stale.\n\n... sounds sensible.\n"},{"id":"235508","messageId":"xmqqbnxsxp8u.fsf@gitster.dls.corp.google.com","threadId":"35967","inReplyTo":"xmqqob1sxq8v.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-27T20:41:21Z","receivedAt":"2014-02-27T20:41:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Carlos Martín Nieto <cmn@elego.de> writes:\n>\n>> From: Carlos Martín Nieto <cmn@dwim.me>\n>>\n>> We need to consider that a remote-tracking branch may match more than\n>> one rhs of a fetch refspec.\n>\n> Hmph, do we *need* to, really?\n>\n> Do you mean fetching one ref on the remote side and storing that in\n> multiple remote-tracking refs on our side?  What benefit does such\n> an arrangement give the user?  When we \"git fetch $there $that_ref\"\n> to obtain that single ref, do we update both remote-tracking refs?\n> When the user asks \"git log $that_ref@{upstream}\", which one of two\n> or more remote-tracking refs should we consult?  Should we report\n> an error if these remote-tracking refs that are supposed to track\n> the same remote ref not all match?  Does \"git push $there $that_ref\"\n> to update that remote ref update all of these remote-tracking refs\n> on our side?  Should it?\n>\n> My knee-jerk reaction is that it may not be worth supporting such an\n> arrangement as broken (we may even want to diagnose it as an error),\n> but assuming we do need to, the approach to solve it, i.e. this...\n>\n>> In such a case, it is not enough to stop at\n>> the first match but look at all of the matches in order to determine\n>> whether a head is stale.\n>\n> ... sounds sensible.\n\nHaving said that, if we need to support such a configuration, I\nwould not be surprised if there are many other corner case bugs\ncoming from the same root cause---query_refspecs() does not allow us\nto see more than one destination.  It would be prudent to squash\nthem before we officially say we do support such a configuration.\n\nThanks.\n"},{"id":"235598","messageId":"1393590104.5277.19.camel@centaur.cmartin.tk","threadId":"35967","inReplyTo":"xmqqob1sxq8v.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2014-02-28T12:21:44Z","receivedAt":"2014-02-28T12:21:44Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Thu, 2014-02-27 at 12:19 -0800, Junio C Hamano wrote:\n> Carlos Martín Nieto <cmn@elego.de> writes:\n> \n> > From: Carlos Martín Nieto <cmn@dwim.me>\n> >\n> > We need to consider that a remote-tracking branch may match more than\n> > one rhs of a fetch refspec.\n> \n> Hmph, do we *need* to, really?\n> \n> Do you mean fetching one ref on the remote side and storing that in\n> multiple remote-tracking refs on our side?  What benefit does such\n> an arrangement give the user?  When we \"git fetch $there $that_ref\"\n\nNo, I mean a different kind of overlap, where the right-hand side\nmatches more refs than appear on the left side. In this particular case,\nwe would have something like\n\n    refs/heads/*:refs/remotes/origin/*\n    refs/pull/*/head:refs/remotes/origin/pr/*\n\nas fetch refspecs. Going remote -> remote-tracking branch is not an\nissue, as each remote head only matches one refspec. However, we now\nhave 'origin/master' and 'origin/pr/5' both of which match the\n'refs/remotes/origin/*' pattern. The current behaviour is to stop at the\nfirst match, which would mark it as stale as there is no\n'refs/heads/pr/5' branch in the remote.\n\nIn lieu of \"real\" namespacing support for remotes, this seems like a\nreasonable way of coalescing the namespaces in the remote repo. I'll\nupdate the commit message with more exact explanation of what kind of\noverlap we're dealing with, as it seems it could do with help. Is there\nmaybe a better word to describe this setup than \"overlapping\"?\n\n> to obtain that single ref, do we update both remote-tracking refs?\n> When the user asks \"git log $that_ref@{upstream}\", which one of two\n> or more remote-tracking refs should we consult?  Should we report\n> an error if these remote-tracking refs that are supposed to track\n> the same remote ref not all match?  Does \"git push $there $that_ref\"\n> to update that remote ref update all of these remote-tracking refs\n> on our side?  Should it?\n> \n> My knee-jerk reaction is that it may not be worth supporting such an\n> arrangement as broken (we may even want to diagnose it as an error),\n> but assuming we do need to, the approach to solve it, i.e. this...\n> \n\nFor this (other) situation, where you duplicate refs, the issue we're\ndealing with in these patches wouldn't arise. I have argued similarly\nagainst built-in support in libgit2 for this kind of shenanigans, but\napparently there's people who use it, though their motivations remain a\nmystery to me. Luckily we can support *that* quite well by just going\nthrough the refspecs one by one and applying the rules (both in git and\nlibgit2).\n\n   cmn\n\n> > In such a case, it is not enough to stop at\n> > the first match but look at all of the matches in order to determine\n> > whether a head is stale.\n> \n> ... sounds sensible.\n"},{"id":"235650","messageId":"xmqqy50vun9r.fsf@gitster.dls.corp.google.com","threadId":"35967","inReplyTo":"1393590104.5277.19.camel@centaur.cmartin.tk","subject":"Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-28T18:04:32Z","receivedAt":"2014-02-28T18:04:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto <cmn@elego.de> writes:\n\n> ... However, we now\n> have 'origin/master' and 'origin/pr/5' both of which match the\n> 'refs/remotes/origin/*' pattern. The current behaviour is to stop at the\n> first match, which would mark it as stale as there is no\n> 'refs/heads/pr/5' branch in the remote.\n\nOK, but with a later pattern, we can find out that it came from pull/5\nthat was advertised by the remote.  If we had origin/pr/1 when the\nremote no longer has pull/1, then we can say that is stale.\n\nMakes sense.  Thanks for an explanation.\n\nI wonder how well --prune would work on a repository in pre 1.5\nlayout, where all branches were copied to local refs/heads/\nhierarchy except for 'master' (which is renamed to 'origin').  Does\nit have a similar issue?  Do we end up pruning refs/heads/origin\naway because we do not see it on the remote end, or we somehow\nalready deal with it and not have to worry about it?\n"},{"id":"237484","messageId":"xmqqvbv3pfhf.fsf@gitster.dls.corp.google.com","threadId":"35967","inReplyTo":"1393491610-19476-2-git-send-email-cmn@elego.de","subject":"Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-24T19:24:12Z","receivedAt":"2014-03-24T19:24:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto <cmn@elego.de> writes:\n\n> From: Carlos Martín Nieto <cmn@dwim.me>\n>\n> We need to consider that a remote-tracking branch may match more than\n> one rhs of a fetch refspec. In such a case, it is not enough to stop at\n> the first match but look at all of the matches in order to determine\n> whether a head is stale.\n>\n> To this goal, introduce a variant of query_refspecs which returns all of\n> the matching refspecs and loop over those answers to check for\n> staleness.\n>\n> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>\n> ---\n>\n> There is an unfortunate duplication of code here, as\n> query_refspecs_multiple is mostly query_refspecs but we only care\n> about the other side of matching refspecs and disregard the 'force'\n> information which query_refspecs does want.\n>\n> I thought about putting both together via callbacks and having\n> query_refspecs stop at the first one, but I'm not sure that it would\n> make it easier to read or manage.\n\nSorry for a belated review.\n\nI agree with your analysis of the root-cause of the symptom exposed\nby new tests in [1/2] and the proposed solution.\n\n> @@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname,\n>  \tconst unsigned char *sha1, int flags, void *cb_data)\n>  {\n>  \tstruct stale_heads_info *info = cb_data;\n> +\tstruct string_list matches = STRING_LIST_INIT_DUP;\n>  \tstruct refspec query;\n> +\tint i, stale = 1;\n>  \tmemset(&query, 0, sizeof(struct refspec));\n>  \tquery.dst = (char *)refname;\n>  \n> -\tif (query_refspecs(info->refs, info->ref_count, &query))\n> +\tquery_refspecs_multiple(info->refs, info->ref_count, &query, &matches);\n> +\tif (matches.nr == 0)\n>  \t\treturn 0; /* No matches */\n>  \n>  \t/*\n>  \t * If we did find a suitable refspec and it's not a symref and\n>  \t * it's not in the list of refs that currently exist in that\n> -\t * remote we consider it to be stale.\n> +\t * remote we consider it to be stale. In order to deal with\n> +\t * overlapping refspecs, we need to go over all of the\n> +\t * matching refs.\n>  \t */\n> -\tif (!((flags & REF_ISSYMREF) ||\n> -\t      string_list_has_string(info->ref_names, query.src))) {\n> +\tif (flags & REF_ISSYMREF)\n> +\t\treturn 0;\n\nWho frees \"matches\"?  At this point matches.nr != 0 so there must be\nsomething we need to free, no?\n\n> +\tfor (i = 0; i < matches.nr; i++) {\n> +\t\tif (string_list_has_string(info->ref_names, matches.items[i].string)) {\n> +\t\t\tstale = 0;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tstring_list_clear(&matches, 0);\n> +\n> +\tif (stale) {\n>  \t\tstruct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);\n>  \t\thashcpy(ref->new_sha1, sha1);\n>  \t}\n>  \n> -\tfree(query.src);\n\nIn the new code, query_refspecs_multiple() uses the result allocated\nby match_name_with_pattern() to the results list, taking it out of\nquery.src without copying, so losing this free() is the right thing\nto do---\"matches\" must be cleared.\n\nAnd \"string_list matches\" is initialized as INIT_DUP, so we can rely\non string_list_clear() to free these strings.\n\n>  \treturn 0;\n>  }\n\nRegarding the seemingly duplicated logic in the new function, I\nwonder if the callers of non-duplicated variant may benefit if they\nnotice there are multiple hits, even if they cannot use more than\none in their context.  That is, what would happen if we changed\nthese callers to instead of calling query-refspecs call the \"multi\"\nvariant, and if that call finds multiple matches, do something about\nit (e.g. warn if they use \"the first hit\" because they are not\nacting on later hits, possibly losing information)?\n\nHere is a minor clean-ups, both to fix style and plug leaks, that\ncan be squashed to this patch.  How does it look?\n\nThanks.\n\n remote.c | 20 ++++++++------------\n 1 file changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 26140c7..fde7b52 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -839,9 +839,8 @@ static void query_refspecs_multiple(struct refspec *refs, int ref_count, struct\n \t\tif (!refspec->dst)\n \t\t\tcontinue;\n \t\tif (refspec->pattern) {\n-\t\t\tif (match_name_with_pattern(key, needle, value, result)) {\n+\t\t\tif (match_name_with_pattern(key, needle, value, result))\n \t\t\t\tstring_list_append_nodup(results, *result);\n-\t\t\t}\n \t\t} else if (!strcmp(needle, key)) {\n \t\t\tstring_list_append(results, value);\n \t\t}\n@@ -1989,32 +1988,29 @@ static int get_stale_heads_cb(const char *refname,\n \n \tquery_refspecs_multiple(info->refs, info->ref_count, &query, &matches);\n \tif (matches.nr == 0)\n-\t\treturn 0; /* No matches */\n+\t\tgoto clean_exit; /* No matches */\n \n \t/*\n \t * If we did find a suitable refspec and it's not a symref and\n \t * it's not in the list of refs that currently exist in that\n-\t * remote we consider it to be stale. In order to deal with\n+\t * remote, we consider it to be stale. In order to deal with\n \t * overlapping refspecs, we need to go over all of the\n \t * matching refs.\n \t */\n \tif (flags & REF_ISSYMREF)\n-\t\treturn 0;\n+\t\tgoto clean_exit;\n \n-\tfor (i = 0; i < matches.nr; i++) {\n-\t\tif (string_list_has_string(info->ref_names, matches.items[i].string)) {\n+\tfor (i = 0; stale && i < matches.nr; i++)\n+\t\tif (string_list_has_string(info->ref_names, matches.items[i].string))\n \t\t\tstale = 0;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\n-\tstring_list_clear(&matches, 0);\n \n \tif (stale) {\n \t\tstruct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);\n \t\thashcpy(ref->new_sha1, sha1);\n \t}\n \n+clean_exit:\n+\tstring_list_clear(&matches, 0);\n \treturn 0;\n }\n \n"}]}