{"thread":{"id":"28627","subject":"[PATCH 3/4] fetch: honor the user-provided refspecs when pruning refs","startedAt":"2011-10-07T22:51:05Z","lastAt":"2011-10-12T23:18:39Z","messageCount":7,"participants":["Carlos Martín Nieto","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"177198","messageId":"1318027869-4037-1-git-send-email-cmn@elego.de","threadId":"28627","inReplyTo":null,"subject":"[PATCHv3 0/4] Be more careful when prunning","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-10-07T22:51:05Z","receivedAt":"2011-10-07T22:51:05Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Hello,\n\nThe first patch is not that big a deal, but it's better if we're\nfreeing the refspecs, we might as well free all of them.\n\nThe second patch introduces expected failures for the features that\nthis series fixes.\n\nThe third patch changes prune_resf and get_stale_heads so the caller\nhas to decide which refspecs are the appropriate ones to use. For\nexample, running\n\n    git fetch --prune origin refs/heads/master:refs/heads/master\n\ndoesn't remove the other branches anymore. For a more interesting (and\nbelievable) example, let's take\n\n    git fetch --prune origin refs/heads/b/*:refs/heads/b/*\n\nbecause you want to prune the refs inside the b/ namespace\nonly. Currently git will delete all the refs that aren't under that\nnamespace. With the second patch applied, git won't remove any refs\noutside the b/ namespace.\n\nWhat is probably the most usual case is covered by the forth patch,\nwhich pretends that a \"refs/tags/*:refs/tags/*\" refspec was given on\nthe command-line. That fixes the\n\n    git fetch --prune --tags origin\n\ncase. The non-tag refs are kept now.\n\nCheers,\n   cmn\n\nCarlos Martín Nieto (4):\n  fetch: free all the additional refspecs\n  t5510: add tests for fetch --prune\n  fetch: honor the user-provided refspecs when pruning refs\n  fetch: treat --tags like refs/tags/*:refs/tags/* when pruning\n\n builtin/fetch.c  |   33 +++++++++++++++++++++++----\n builtin/remote.c |    3 +-\n remote.c         |   66 ++++++++++++++++++++++++++++++++++++++++++++++-------\n remote.h         |    2 +-\n t/t5510-fetch.sh |   50 ++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 138 insertions(+), 16 deletions(-)\n\n-- \n1.7.5.2.354.g349bf\n"},{"id":"177199","messageId":"1318027869-4037-2-git-send-email-cmn@elego.de","threadId":"28627","inReplyTo":"1318027869-4037-1-git-send-email-cmn@elego.de","subject":"[PATCH 1/4] fetch: free all the additional refspecs","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-10-07T22:51:06Z","receivedAt":"2011-10-07T22:51:06Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Signed-off-by: Carlos Martín Nieto <cmn@elego.de>\n---\n builtin/fetch.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7a4e41c..30b485e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -883,7 +883,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)\n \tatexit(unlock_pack);\n \trefspec = parse_fetch_refspec(ref_nr, refs);\n \texit_code = do_fetch(transport, refspec, ref_nr);\n-\tfree(refspec);\n+\tfree_refspec(ref_nr, refspec);\n \ttransport_disconnect(transport);\n \ttransport = NULL;\n \treturn exit_code;\n-- \n1.7.5.2.354.g349bf\n"},{"id":"177197","messageId":"1318027869-4037-3-git-send-email-cmn@elego.de","threadId":"28627","inReplyTo":"1318027869-4037-1-git-send-email-cmn@elego.de","subject":"[PATCH 2/4] t5510: add tests for fetch --prune","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-10-07T22:51:07Z","receivedAt":"2011-10-07T22:51:07Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"The failures will be fixed in later commits.\n\nSigned-off-by: Carlos Martín Nieto <cmn@elego.de>\n---\n t/t5510-fetch.sh |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 50 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 7e433b1..8b5e925 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -76,6 +76,56 @@ test_expect_success \"fetch test for-merge\" '\n \tcut -f -2 .git/FETCH_HEAD >actual &&\n \ttest_cmp expected actual'\n \n+test_expect_success 'fetch --prune on its own works as expected' '\n+\tcd \"$D\" &&\n+\tgit clone . prune &&\n+\tcd prune &&\n+\tgit fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&\n+\n+\tgit fetch --prune origin &&\n+\ttest_must_fail git rev-parse origin/extrabranch\n+'\n+\n+test_expect_failure 'fetch --prune with a branch name keeps branches' '\n+\tcd \"$D\" &&\n+\tgit clone . prune-branch &&\n+\tcd prune-branch &&\n+\tgit fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&\n+\n+\tgit fetch --prune origin master &&\n+\tgit rev-parse origin/extrabranch\n+'\n+\n+test_expect_failure 'fetch --prune with a namespace keeps other namespaces' '\n+\tcd \"$D\" &&\n+\tgit clone . prune-namespace &&\n+\tcd prune-namespace &&\n+\n+\tgit fetch --prune origin refs/heads/a/*:refs/remotes/origin/a/* &&\n+\tgit rev-parse origin/master\n+'\n+\n+test_expect_failure 'fetch --prune --tags does not delete the remote-tracking branches' '\n+\tcd \"$D\" &&\n+\tgit clone . prune-tags &&\n+\tcd prune-tags &&\n+\tgit fetch origin refs/heads/master:refs/tags/sometag &&\n+\n+\tgit fetch --prune --tags origin &&\n+\tgit rev-parse origin/master &&\n+\ttest_must_fail git rev-parse somebranch\n+'\n+\n+test_expect_failure 'fetch --prune --tags with branch does not delete other remote-tracking branches' '\n+\tcd \"$D\" &&\n+\tgit clone . prune-tags-branch &&\n+\tcd prune-tags-branch &&\n+\tgit fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&\n+\n+\tgit fetch --prune --tags origin master &&\n+\tgit rev-parse origin/extrabranch\n+'\n+\n test_expect_success 'fetch tags when there is no tags' '\n \n     cd \"$D\" &&\n-- \n1.7.5.2.354.g349bf\n"},{"id":"177196","messageId":"1318027869-4037-4-git-send-email-cmn@elego.de","threadId":"28627","inReplyTo":"1318027869-4037-1-git-send-email-cmn@elego.de","subject":"[PATCH 3/4] fetch: honor the user-provided refspecs when pruning refs","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-10-07T22:51:08Z","receivedAt":"2011-10-07T22:51:08Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"If the user gave us refspecs on the command line, we should use those\nwhen deciding whether to prune a ref instead of relying on the\nrefspecs in the config.\n\nPreviously, running\n\n    git fetch --prune origin refs/heads/master:refs/remotes/origin/master\n\nwould delete every other ref under the origin namespace because we\nwere using the refspec to filter the available refs but using the\nconfigured refspec to figure out if a ref had been deleted on the\nremote. This is clearly the wrong thing to do.\n\nChange prune_refs and get_stale_heads to simply accept a list of\nreferences and a list of refspecs. The caller of either function needs\nto decide what refspecs should be used to decide whether a ref is\nstale.\n\nSigned-off-by: Carlos Martín Nieto <cmn@elego.de>\n---\n builtin/fetch.c  |   12 ++++++---\n builtin/remote.c |    3 +-\n remote.c         |   66 ++++++++++++++++++++++++++++++++++++++++++++++-------\n remote.h         |    2 +-\n t/t5510-fetch.sh |    4 +-\n 5 files changed, 70 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 30b485e..041f79e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -505,10 +505,10 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)\n \treturn ret;\n }\n \n-static int prune_refs(struct transport *transport, struct ref *ref_map)\n+static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)\n {\n \tint result = 0;\n-\tstruct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map);\n+\tstruct ref *ref, *stale_refs = get_stale_heads(ref_map, refs, ref_count);\n \tconst char *dangling_msg = dry_run\n \t\t? _(\"   (%s will become dangling)\\n\")\n \t\t: _(\"   (%s has become dangling)\\n\");\n@@ -699,8 +699,12 @@ static int do_fetch(struct transport *transport,\n \t\tfree_refs(ref_map);\n \t\treturn 1;\n \t}\n-\tif (prune)\n-\t\tprune_refs(transport, ref_map);\n+\tif (prune) {\n+\t\tif (ref_count)\n+\t\t\tprune_refs(refs, ref_count, ref_map);\n+\t\telse\n+\t\t\tprune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);\n+\t}\n \tfree_refs(ref_map);\n \n \t/* if neither --no-tags nor --tags was specified, do automated tag\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex f2a9c26..79d898b 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -349,7 +349,8 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n \t\telse\n \t\t\tstring_list_append(&states->tracked, abbrev_branch(ref->name));\n \t}\n-\tstale_refs = get_stale_heads(states->remote, fetch_map);\n+\tstale_refs = get_stale_heads(fetch_map, states->remote->fetch,\n+\t\t\t\t     states->remote->fetch_refspec_nr);\n \tfor (ref = stale_refs; ref; ref = ref->next) {\n \t\tstruct string_list_item *item =\n \t\t\tstring_list_append(&states->stale, abbrev_branch(ref->name));\ndiff --git a/remote.c b/remote.c\nindex b8ecfa5..13c9153 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1681,36 +1681,84 @@ struct ref *guess_remote_head(const struct ref *head,\n }\n \n struct stale_heads_info {\n-\tstruct remote *remote;\n \tstruct string_list *ref_names;\n \tstruct ref **stale_refs_tail;\n+\tstruct refspec *refs;\n+\tint ref_count;\n };\n \n+/* Returns 0 on success, -1 if it couldn't find a match in the refspecs. */\n+static int find_in_refs(struct refspec *refs, int ref_count, struct refspec *query)\n+{\n+\tint i;\n+\tstruct refspec *refspec;\n+\n+\tfor (i = 0; i < ref_count; ++i) {\n+\t\trefspec = &refs[i];\n+\n+\t\t/* No dst means it can't be used for prunning. */\n+\t\tif (!refspec->dst)\n+\t\t\tcontinue;\n+\n+\t\t/*\n+\t\t * No '*' means that it must match exactly. If it does\n+\t\t * have it, try to match it against the pattern. If\n+\t\t * the refspec matches, store the ref name as it would\n+\t\t * appear in the server in query->src.\n+\t\t */\n+\t\tif (!strchr(refspec->dst, '*')) {\n+\t\t\tif (!strcmp(query->dst, refspec->dst)) {\n+\t\t\t\tquery->src = xstrdup(refspec->src);\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t} else if (match_name_with_pattern(refspec->dst, query->dst,\n+\t\t\t\t\t\t    refspec->src, &query->src)) {\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\treturn -1;\n+}\n+\n 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 refspec refspec;\n+\tint ret;\n \tmemset(&refspec, 0, sizeof(refspec));\n \trefspec.dst = (char *)refname;\n-\tif (!remote_find_tracking(info->remote, &refspec)) {\n-\t\tif (!((flags & REF_ISSYMREF) ||\n-\t\t    string_list_has_string(info->ref_names, refspec.src))) {\n-\t\t\tstruct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);\n-\t\t\thashcpy(ref->new_sha1, sha1);\n-\t\t}\n+\n+\tret = find_in_refs(info->refs, info->ref_count, &refspec);\n+\n+\t/* No matches */\n+\tif (ret)\n+\t\treturn 0;\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 */\n+\tif (!((flags & REF_ISSYMREF) ||\n+\t      string_list_has_string(info->ref_names, refspec.src))) {\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(refspec.src);\n \treturn 0;\n }\n \n-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map)\n+struct ref *get_stale_heads(struct ref *fetch_map, struct refspec *refs, int ref_count)\n {\n \tstruct ref *ref, *stale_refs = NULL;\n \tstruct string_list ref_names = STRING_LIST_INIT_NODUP;\n \tstruct stale_heads_info info;\n-\tinfo.remote = remote;\n \tinfo.ref_names = &ref_names;\n \tinfo.stale_refs_tail = &stale_refs;\n+\tinfo.refs = refs;\n+\tinfo.ref_count = ref_count;\n \tfor (ref = fetch_map; ref; ref = ref->next)\n \t\tstring_list_append(&ref_names, ref->name);\n \tsort_string_list(&ref_names);\ndiff --git a/remote.h b/remote.h\nindex 9a30a9d..5d70aff 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -164,6 +164,6 @@ struct ref *guess_remote_head(const struct ref *head,\n \t\t\t      int all);\n \n /* Return refs which no longer exist on remote */\n-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map);\n+struct ref *get_stale_heads(struct ref *fetch_map, struct refspec *refs, int ref_count);\n \n #endif\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 8b5e925..581049b 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -86,7 +86,7 @@ test_expect_success 'fetch --prune on its own works as expected' '\n \ttest_must_fail git rev-parse origin/extrabranch\n '\n \n-test_expect_failure 'fetch --prune with a branch name keeps branches' '\n+test_expect_success 'fetch --prune with a branch name keeps branches' '\n \tcd \"$D\" &&\n \tgit clone . prune-branch &&\n \tcd prune-branch &&\n@@ -96,7 +96,7 @@ test_expect_failure 'fetch --prune with a branch name keeps branches' '\n \tgit rev-parse origin/extrabranch\n '\n \n-test_expect_failure 'fetch --prune with a namespace keeps other namespaces' '\n+test_expect_success 'fetch --prune with a namespace keeps other namespaces' '\n \tcd \"$D\" &&\n \tgit clone . prune-namespace &&\n \tcd prune-namespace &&\n-- \n1.7.5.2.354.g349bf\n"},{"id":"177200","messageId":"1318027869-4037-5-git-send-email-cmn@elego.de","threadId":"28627","inReplyTo":"1318027869-4037-1-git-send-email-cmn@elego.de","subject":"[PATCH 4/4] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-10-07T22:51:09Z","receivedAt":"2011-10-07T22:51:09Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"If --tags is specified, add that refspec to the list given to\nprune_refs so it knows to treat it as a filter on what refs to\nshould consider for prunning. This way\n\n    git fetch --prune --tags origin\n\nonly prunes tags and doesn't delete the branch refs.\n\nSigned-off-by: Carlos Martín Nieto <cmn@elego.de>\n---\n builtin/fetch.c  |   23 +++++++++++++++++++++--\n t/t5510-fetch.sh |    4 ++--\n 2 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 041f79e..0f8170c 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -700,10 +700,29 @@ static int do_fetch(struct transport *transport,\n \t\treturn 1;\n \t}\n \tif (prune) {\n-\t\tif (ref_count)\n+\t\t/* If --tags was specified, pretend the user gave us the canonical tags refspec */\n+\t\tif (tags == TAGS_SET) {\n+\t\t\tconst char *tags_str = \"refs/tags/*:refs/tags/*\";\n+\t\t\tstruct refspec *tags_refspec, *refspec;\n+\n+\t\t\t/* Copy the refspec and add the tags to it */\n+\t\t\trefspec = xcalloc(ref_count + 1, sizeof(struct refspec));\n+\t\t\ttags_refspec = parse_fetch_refspec(1, &tags_str);\n+\t\t\tmemcpy(refspec, refs, ref_count * sizeof(struct refspec));\n+\t\t\tmemcpy(&refspec[ref_count], tags_refspec, sizeof(struct refspec));\n+\t\t\tref_count++;\n+\n+\t\t\tprune_refs(refspec, ref_count, ref_map);\n+\n+\t\t\tref_count--;\n+\t\t\t/* The rest of the strings belong to fetch_one */\n+\t\t\tfree_refspec(1, tags_refspec);\n+\t\t\tfree(refspec);\n+\t\t} else if (ref_count) {\n \t\t\tprune_refs(refs, ref_count, ref_map);\n-\t\telse\n+\t\t} else {\n \t\t\tprune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);\n+\t\t}\n \t}\n \tfree_refs(ref_map);\n \ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 581049b..e0af4c4 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -105,7 +105,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 --tags does not delete the remote-tracking branches' '\n+test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' '\n \tcd \"$D\" &&\n \tgit clone . prune-tags &&\n \tcd prune-tags &&\n@@ -116,7 +116,7 @@ test_expect_failure 'fetch --prune --tags does not delete the remote-tracking br\n \ttest_must_fail git rev-parse somebranch\n '\n \n-test_expect_failure 'fetch --prune --tags with branch does not delete other remote-tracking branches' '\n+test_expect_success 'fetch --prune --tags with branch does not delete other remote-tracking branches' '\n \tcd \"$D\" &&\n \tgit clone . prune-tags-branch &&\n \tcd prune-tags-branch &&\n-- \n1.7.5.2.354.g349bf\n"},{"id":"177488","messageId":"7vsjmx7uur.fsf@alter.siamese.dyndns.org","threadId":"28627","inReplyTo":"1318027869-4037-4-git-send-email-cmn@elego.de","subject":"Re: [PATCH 3/4] fetch: honor the user-provided refspecs when pruning refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T21:39:24Z","receivedAt":"2011-10-12T21:39:24Z","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> -static int prune_refs(struct transport *transport, struct ref *ref_map)\n> +static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)\n>  {\n>  \tint result = 0;\n> -\tstruct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map);\n> +\tstruct ref *ref, *stale_refs = get_stale_heads(ref_map, refs, ref_count);\n\nSo in short, get_state_heads() used to take a ref_map and a remote. The\nref_map is what we actually observed from the remote after talking\nls-remote with it. It tried to see if any existing ref in our refspace may\nhave come from that remote by inspecting the fetch refspec associated with\nthat remote (and the ones that does not exist anymore are queued in the\nstale ref list).\n\nNow get_state_heads() takes a ref_map and <refs, ref_count> (you made the\npatch unnecessarily harder to read by swapping the order of parameters).\nThe latter \"pair\" roughly corresponds to what the \"remote\" parameter used\nto mean, but instead of using the refspec associated with that remote, we\nwould use the refspec used for this particular fetch to determine which\nrefs we have are stale.\n\n> @@ -699,8 +699,12 @@ static int do_fetch(struct transport *transport,\n>  \t\tfree_refs(ref_map);\n>  \t\treturn 1;\n>  \t}\n> -\tif (prune)\n> -\t\tprune_refs(transport, ref_map);\n> +\tif (prune) {\n> +\t\tif (ref_count)\n> +\t\t\tprune_refs(refs, ref_count, ref_map);\n> +\t\telse\n> +\t\t\tprune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);\n> +\t}\n\nAnd this is consistent to my two paragraph commentary above.\n\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index f2a9c26..79d898b 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -349,7 +349,8 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n>  \t\telse\n>  \t\t\tstring_list_append(&states->tracked, abbrev_branch(ref->name));\n>  \t}\n> -\tstale_refs = get_stale_heads(states->remote, fetch_map);\n> +\tstale_refs = get_stale_heads(fetch_map, states->remote->fetch,\n> +\t\t\t\t     states->remote->fetch_refspec_nr);\n\nSo is this.\n\n> diff --git a/remote.c b/remote.c\n> index b8ecfa5..13c9153 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1681,36 +1681,84 @@ struct ref *guess_remote_head(const struct ref *head,\n>  }\n>  \n>  struct stale_heads_info {\n> -\tstruct remote *remote;\n>  \tstruct string_list *ref_names;\n>  \tstruct ref **stale_refs_tail;\n> +\tstruct refspec *refs;\n> +\tint ref_count;\n>  };\n>  \n> +/* Returns 0 on success, -1 if it couldn't find a match in the refspecs. */\n> +static int find_in_refs(struct refspec *refs, int ref_count, struct refspec *query)\n> +{\n> +\tint i;\n> +\tstruct refspec *refspec;\n\nThis function replaces the role remote_find_tracking() used to play in the\nold code and the difference in the behaviour (except the obvious lack of\n\"find_src/find_dst\") feels gratuitous.\n\nThe original code in remote_find_tracking() uses \"->pattern\" to see if a\npattern match is necessary, but this scans the refspec for an asterisk,\nassuring a breakage when the refspec language is updated to understand\nother glob magic in the future. Why isn't refspec->pattern used here?\n\nCan't these two functions share more logic?  It appears to me that by\nenhancing the logic here a little bit, it may be possible to implement\nremote_find_tracking() ed in terms of this function as a helper.\n\n> +\tfor (i = 0; i < ref_count; ++i) {\n> +\t\trefspec = &refs[i];\n> +\n> +\t\t/* No dst means it can't be used for prunning. */\n> +\t\tif (!refspec->dst)\n> +\t\t\tcontinue;\n> +\n> +\t\t/*\n> +\t\t * No '*' means that it must match exactly. If it does\n> +\t\t * have it, try to match it against the pattern. If\n> +\t\t * the refspec matches, store the ref name as it would\n> +\t\t * appear in the server in query->src.\n> +\t\t */\n> +\t\tif (!strchr(refspec->dst, '*')) {\n> +\t\t\tif (!strcmp(query->dst, refspec->dst)) {\n> +\t\t\t\tquery->src = xstrdup(refspec->src);\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n> +\t\t} else if (match_name_with_pattern(refspec->dst, query->dst,\n> +\t\t\t\t\t\t    refspec->src, &query->src)) {\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t}\n> +\n> +\treturn -1;\n> +}\n"},{"id":"177501","messageId":"1318461519.4484.26.camel@centaur.lab.cmartin.tk","threadId":"28627","inReplyTo":"7vsjmx7uur.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] fetch: honor the user-provided refspecs when pruning refs","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2011-10-12T23:18:39Z","receivedAt":"2011-10-12T23:18:39Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Wed, 2011-10-12 at 14:39 -0700, Junio C Hamano wrote:\n> Carlos Martín Nieto <cmn@elego.de> writes:\n> \n> > -static int prune_refs(struct transport *transport, struct ref *ref_map)\n> > +static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)\n> >  {\n> >  \tint result = 0;\n> > -\tstruct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map);\n> > +\tstruct ref *ref, *stale_refs = get_stale_heads(ref_map, refs, ref_count);\n> \n> So in short, get_state_heads() used to take a ref_map and a remote. The\n> ref_map is what we actually observed from the remote after talking\n> ls-remote with it. It tried to see if any existing ref in our refspace may\n> have come from that remote by inspecting the fetch refspec associated with\n> that remote (and the ones that does not exist anymore are queued in the\n> stale ref list).\n> \n> Now get_state_heads() takes a ref_map and <refs, ref_count> (you made the\n> patch unnecessarily harder to read by swapping the order of parameters).\n> The latter \"pair\" roughly corresponds to what the \"remote\" parameter used\n> to mean, but instead of using the refspec associated with that remote, we\n> would use the refspec used for this particular fetch to determine which\n> refs we have are stale.\n\nRight. The only reason that the remote was passed was in order to use\nits refspec. The order reversal wasn't on purpose, I'll change that.\n\n> \n> > @@ -699,8 +699,12 @@ static int do_fetch(struct transport *transport,\n> >  \t\tfree_refs(ref_map);\n> >  \t\treturn 1;\n> >  \t}\n> > -\tif (prune)\n> > -\t\tprune_refs(transport, ref_map);\n> > +\tif (prune) {\n> > +\t\tif (ref_count)\n> > +\t\t\tprune_refs(refs, ref_count, ref_map);\n> > +\t\telse\n> > +\t\t\tprune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);\n> > +\t}\n> \n> And this is consistent to my two paragraph commentary above.\n> \n> > diff --git a/builtin/remote.c b/builtin/remote.c\n> > index f2a9c26..79d898b 100644\n> > --- a/builtin/remote.c\n> > +++ b/builtin/remote.c\n> > @@ -349,7 +349,8 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n> >  \t\telse\n> >  \t\t\tstring_list_append(&states->tracked, abbrev_branch(ref->name));\n> >  \t}\n> > -\tstale_refs = get_stale_heads(states->remote, fetch_map);\n> > +\tstale_refs = get_stale_heads(fetch_map, states->remote->fetch,\n> > +\t\t\t\t     states->remote->fetch_refspec_nr);\n> \n> So is this.\n> \n> > diff --git a/remote.c b/remote.c\n> > index b8ecfa5..13c9153 100644\n> > --- a/remote.c\n> > +++ b/remote.c\n> > @@ -1681,36 +1681,84 @@ struct ref *guess_remote_head(const struct ref *head,\n> >  }\n> >  \n> >  struct stale_heads_info {\n> > -\tstruct remote *remote;\n> >  \tstruct string_list *ref_names;\n> >  \tstruct ref **stale_refs_tail;\n> > +\tstruct refspec *refs;\n> > +\tint ref_count;\n> >  };\n> >  \n> > +/* Returns 0 on success, -1 if it couldn't find a match in the refspecs. */\n> > +static int find_in_refs(struct refspec *refs, int ref_count, struct refspec *query)\n> > +{\n> > +\tint i;\n> > +\tstruct refspec *refspec;\n> \n> This function replaces the role remote_find_tracking() used to play in the\n> old code and the difference in the behaviour (except the obvious lack of\n> \"find_src/find_dst\") feels gratuitous.\n\nremote_find_tracking wants a remote, and that's what we don't have\nanymore. The main reason was that it does \"too much\". The previous\nversions had the callback doing more by itself, so I overlooked the\npossibilities of remote_find_tracking when rewriting it. Looking at the\ncode again, it does look like what we want.\n\n> \n> The original code in remote_find_tracking() uses \"->pattern\" to see if a\n> pattern match is necessary, but this scans the refspec for an asterisk,\n> assuring a breakage when the refspec language is updated to understand\n> other glob magic in the future. Why isn't refspec->pattern used here?\n\nTrees, forest etc. I noticed that a bit late. I have a patch on top of\nthis one that does use ->pattern, which I was going to ask you to squash\nin, but it's moot now, as I need to rewrite the patch anyway.\n\n> \n> Can't these two functions share more logic?  It appears to me that by\n> enhancing the logic here a little bit, it may be possible to implement\n> remote_find_tracking() ed in terms of this function as a helper.\n\nYes, remote_find_tracking should use a version of this function; or\nprobably better, its loop should become the next version of\nfind_in_refs, so remote_find_tracking is just a wrapper for when we want\nto use the remote's fetch refspec.\n\n\nI'll resend the series with these changes.\n\n   cmn\n\n"}]}