{"thread":{"id":"35183","subject":"[PATCH 01/15] t5510: use the correct tag name in test","startedAt":"2013-10-23T15:50:33Z","lastAt":"2013-10-30T04:26:41Z","messageCount":34,"participants":["Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"229336","messageId":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"52327E62.2040301@alum.mit.edu","subject":"[PATCH 00/15] Change semantics of \"fetch --tags\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:33Z","receivedAt":"2013-10-23T15:50:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is my proposed fix for the \"local tag killer\" problem that I\nreported recently [1].\n\nThere are three main things changed by this patch series:\n\n1. \"git fetch --tags\" causes tags to be fetched *in addition to* any\n other refspecs that are configured for the remote, rather than\n *instead of*. I believe this is more useful behavior. It is also\n consistent with the documentation as it was written before it was\n disambiguated in 1.8.0.3.\n\n2. \"git fetch --prune\" only prunes references that match an explicit\n refspec (either from the command line or from the\n remote..fetch configuration. In particular, using \"--prune\"\n and \"--tag\" together do *not* make tags subject to pruning. (Tags\n can still be pruned if the user specifies an explicit refspec\n \"refs/tags/*:refs/tags/*\".)\n\n3. Previously, if the user invoked one of the following commands with\n --no-prune, the --no-prune option was not passed to the \"git fetch\"\n subprocesses that they invoked to do their work:\n\n git fetch --all\n git fetch --multiple\n git fetch --recurse-submodules\n git remote update\n\n If fetch.prune or remote..prune were set to true, this could\n result in unwanted reference pruning. The last commit in the\n series fixes this bug and should not be controversial.\n\nI had originally planned to solve the \"local tag killer\" problem by\nadding a new configuration option to define which reference namespaces\nwere subject to pruning (e.g.,\nremote..pruneRef=\"refs/remotes/*\"). I may yet submit that patch\nseries as a separate feature. But while working on it I hit on the\npresent solution, which I think is simpler and more elegant (albeit a\nbit less flexible).\n\nChanges (1) and (2) introduce behavior changes, but I think that they\nare improvements and that the resulting backwards-incompatibility is\nacceptable:\n\nChange (1) means that \"git fetch --tags \" without any\nadditional refspec arguments will fetch more references than it did\nbefore. But I don't think it is very useful to want to fetch tags\nwithout fetching other configured references, so I think it is OK [2].\n\nChange (2) means that using \"git fetch --tags --prune\" will *not*\nprune tags. (This is the whole point of the change!) As discussed in\nthe mailing list, it is usually bad policy to prune tags, because tags\nfor the local repository and for all remote repositories currently\nshare a single namespace, \"refs/tags/*\". Therefore, pruning tags\nbased on information from a single remote risks pruning local tags or\ntags that have been obtained from another remote. The main exception,\nwhen one probably *does* want to prune tags, is when fetching into a\nmirror clone. But mirror clones have\n\"remote..fetch=+refs/*:refs/*\", and so even after this change\ntags will be subject to pruning when fetching into a mirror clone.\n\nThe only other place I can find that does reference pruning is \"git\nremote prune\", but that codepath didn't respect remote..tagopt\nanyway and therefore it *didn't* prune tags unless they were part of\nan explicit refspec; i.e., this codepath already behaved the \"new\" way\nthat other pruning codepaths now behave.\n\nPatches 1-9 are just preliminary cleanup and documentation\nimprovements.\n\nPatch 10 implements change (1) described above.\n\nPatch 11 implements change (2).\n\nPatches 12-14 are some more minor cleanups.\n\nPatch 15 implements change (3).\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/234723\n\n[2] Indeed, I bet that most scripts that invoke \"git fetch --tags\n \" also invoke a plain \"git fetch\" immediately before or\n after to get the rest of the references.\n\nMichael Haggerty (15):\n t5510: use the correct tag name in test\n t5510: prepare test refs more straightforwardly\n t5510: check that \"git fetch --prune --tags\" does not prune branches\n api-remote.txt: correct section \"struct refspect\"\n get_ref_map(): rename local variables\n ref_remove_duplicates(): avoid redundant bisection\n ref_remove_duplicates(): simplify function\n ref_remove_duplicates(): improve documentation comment\n builtin/fetch.c: reorder function definitions\n fetch --tags: fetch tags *in addition to* other stuff\n fetch --prune: prune only based on explicit refspecs\n query_refspecs(): move some constants out of the loop\n builtin/remote.c: reorder function definitions\n builtin/remote.c:update(): use struct argv_array\n fetch, remote: properly convey --no-prune options to subprocesses\n\n Documentation/config.txt | 2 +-\n Documentation/fetch-options.txt | 21 ++-\n Documentation/technical/api-remote.txt | 20 +--\n builtin/fetch.c | 253 +++++++++++++++----------------\n builtin/remote.c | 196 ++++++++++++------------\n git-pull.sh | 2 +-\n remote.c | 44 +++---\n remote.h | 9 +-\n t/t5510-fetch.sh | 36 ++++-\n t/t5515/fetch.br-unconfig_--tags_.._.git | 1 +\n t/t5515/fetch.master_--tags_.._.git | 1 +\n t/t5525-fetch-tagopt.sh | 23 ++-\n 12 files changed, 322 insertions(+), 286 deletions(-)\n\n-- \n1.8.4\n"},{"id":"229329","messageId":"1382543448-2586-2-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 01/15] t5510: use the correct tag name in test","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:34Z","receivedAt":"2013-10-23T15:50:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Fix an apparent copy-paste error: A few lines earlier, a tag\n\"refs/tags/sometag\" is created. Check for the (non-)existence of that\ntag, not \"somebranch\", which is otherwise never mentioned in the\nscript.\n\nSigned-off-by: Michael Haggerty \n---\n t/t5510-fetch.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 1f0f8e6..c5e5dfc 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -121,7 +121,7 @@ test_expect_success 'fetch --prune --tags does not delete the remote-tracking br\n \n \tgit fetch --prune --tags origin &&\n \tgit rev-parse origin/master &&\n-\ttest_must_fail git rev-parse somebranch\n+\ttest_must_fail git rev-parse sometag\n '\n \n test_expect_success 'fetch --prune --tags with branch does not delete other remote-tracking branches' '\n-- \n1.8.4\n"},{"id":"229330","messageId":"1382543448-2586-3-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 02/15] t5510: prepare test refs more straightforwardly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:35Z","receivedAt":"2013-10-23T15:50:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\"git fetch\" was being used with contrived refspecs to create tags and\nremote-tracking branches in test repositories in preparation for the\nactual tests. This is obscure and also makes one wonder whether this\nis indeed just preparation or whether some side-effect of \"git fetch\"\nis being tested.\n\nSo use the more straightforward commands \"git tag\" / \"git update-ref\"\nwhen preparing branches in test repositories.\n\nSigned-off-by: Michael Haggerty \n---\n t/t5510-fetch.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex c5e5dfc..08d8dbb 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -88,7 +88,7 @@ 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+\tgit update-ref refs/remotes/origin/extrabranch master &&\n \n \tgit fetch --prune origin &&\n \ttest_must_fail git rev-parse origin/extrabranch\n@@ -98,7 +98,7 @@ test_expect_success '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+\tgit update-ref refs/remotes/origin/extrabranch master &&\n \n \tgit fetch --prune origin master &&\n \tgit rev-parse origin/extrabranch\n@@ -117,7 +117,7 @@ test_expect_success 'fetch --prune --tags does not delete the remote-tracking br\n \tcd \"$D\" &&\n \tgit clone . prune-tags &&\n \tcd prune-tags &&\n-\tgit fetch origin refs/heads/master:refs/tags/sometag &&\n+\tgit tag sometag master &&\n \n \tgit fetch --prune --tags origin &&\n \tgit rev-parse origin/master &&\n@@ -128,7 +128,7 @@ test_expect_success 'fetch --prune --tags with branch does not delete other remo\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+\tgit update-ref refs/remotes/origin/extrabranch master &&\n \n \tgit fetch --prune --tags origin master &&\n \tgit rev-parse origin/extrabranch\n-- \n1.8.4\n"},{"id":"229337","messageId":"1382543448-2586-4-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 03/15] t5510: check that \"git fetch --prune --tags\" does not prune branches","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:36Z","receivedAt":"2013-10-23T15:50:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\"git fetch --prune --tags\" is currently interpreted as follows:\n\n* \"--tags\" is equivalent to specifying a refspec\n \"refs/tags/*:refs/tags/*\", and supersedes any default refspecs\n configured via remote.$REMOTE.fetch.\n\n* \"--prune\" only operates on the refspecs being fetched.\n\nTherefore, \"git fetch --prune --tags\" prunes tags in refs/tags/* but\ndoes not fetch or prune other references. The fact that this command\ndoes not prune references outside of refs/tags/* was previously\nuntested. So add a test that verifies the status quo.\n\nHowever, the status quo is surprising, so it will be changed later in\nthis patch series.\n\nSigned-off-by: Michael Haggerty \n---\n t/t5510-fetch.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 08d8dbb..8328be1 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -118,9 +118,13 @@ test_expect_success 'fetch --prune --tags does not delete the remote-tracking br\n \tgit clone . prune-tags &&\n \tcd prune-tags &&\n \tgit tag sometag master &&\n+\t# Create what looks like a remote-tracking branch from an earlier\n+\t# fetch that has since been deleted from the remote:\n+\tgit update-ref refs/remotes/origin/fake-remote master &&\n \n \tgit fetch --prune --tags origin &&\n \tgit rev-parse origin/master &&\n+\tgit rev-parse origin/fake-remote &&\n \ttest_must_fail git rev-parse sometag\n '\n \n-- \n1.8.4\n"},{"id":"229338","messageId":"1382543448-2586-5-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 04/15] api-remote.txt: correct section \"struct refspect\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:37Z","receivedAt":"2013-10-23T15:50:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"* Replace reference to function parse_ref_spec() with references to\n functions parse_fetch_refspec() and parse_push_refspec().\n\n* Correct description of src and dst: they *do* include the '*'\n characters.\n\nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-remote.txt | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/technical/api-remote.txt b/Documentation/technical/api-remote.txt\nindex 4be8776..5d245aa 100644\n--- a/Documentation/technical/api-remote.txt\n+++ b/Documentation/technical/api-remote.txt\n@@ -58,16 +58,16 @@ default remote, given the current branch and configuration.\n struct refspec\n --------------\n \n-A struct refspec holds the parsed interpretation of a refspec. If it\n-will force updates (starts with a '+'), force is true. If it is a\n-pattern (sides end with '*') pattern is true. src and dest are the two\n-sides (if a pattern, only the part outside of the wildcards); if there\n-is only one side, it is src, and dst is NULL; if sides exist but are\n-empty (i.e., the refspec either starts or ends with ':'), the\n-corresponding side is \"\".\n-\n-This parsing can be done to an array of strings to give an array of\n-struct refpsecs with parse_ref_spec().\n+A struct refspec holds the parsed interpretation of a refspec. If it\n+will force updates (starts with a '+'), force is true. If it is a\n+pattern (sides end with '*') pattern is true. src and dest are the\n+two sides (including '*' characters if present); if there is only one\n+side, it is src, and dst is NULL; if sides exist but are empty (i.e.,\n+the refspec either starts or ends with ':'), the corresponding side is\n+\"\".\n+\n+An array of strings can be parsed into an array of struct refspecs\n+using parse_fetch_refspec() or parse_push_refspec().\n \n remote_find_tracking(), given a remote and a struct refspec with\n either src or dst filled out, will fill out the other such that the\n-- \n1.8.4\n"},{"id":"229344","messageId":"1382543448-2586-6-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 05/15] get_ref_map(): rename local variables","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:38Z","receivedAt":"2013-10-23T15:50:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Rename \"refs\" -> \"refspecs\" and \"ref_count\" -> \"refspec_count\" to\nreduce confusion, because they describe an array of \"struct refspec\",\nas opposed to the \"struct ref\" objects that are also used in this\nfunction.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex bd7a101..2248abf 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -165,8 +165,8 @@ static void find_non_local_tags(struct transport *transport,\n \t\t\tstruct ref ***tail);\n \n static struct ref *get_ref_map(struct transport *transport,\n-\t\t\t struct refspec *refs, int ref_count, int tags,\n-\t\t\t int *autotags)\n+\t\t\t struct refspec *refspecs, int refspec_count,\n+\t\t\t int tags, int *autotags)\n {\n \tint i;\n \tstruct ref *rm;\n@@ -175,12 +175,12 @@ static struct ref *get_ref_map(struct transport *transport,\n \n \tconst struct ref *remote_refs = transport_get_remote_refs(transport);\n \n-\tif (ref_count || tags == TAGS_SET) {\n+\tif (refspec_count || tags == TAGS_SET) {\n \t\tstruct ref **old_tail;\n \n-\t\tfor (i = 0; i < ref_count; i++) {\n-\t\t\tget_fetch_map(remote_refs, &refs[i], &tail, 0);\n-\t\t\tif (refs[i].dst && refs[i].dst[0])\n+\t\tfor (i = 0; i < refspec_count; i++) {\n+\t\t\tget_fetch_map(remote_refs, &refspecs[i], &tail, 0);\n+\t\t\tif (refspecs[i].dst && refspecs[i].dst[0])\n \t\t\t\t*autotags = 1;\n \t\t}\n \t\t/* Merge everything on the command line, but not --tags */\n-- \n1.8.4\n"},{"id":"229331","messageId":"1382543448-2586-7-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 06/15] ref_remove_duplicates(): avoid redundant bisection","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:39Z","receivedAt":"2013-10-23T15:50:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old code called string_list_lookup(), and if that failed called\nstring_list_insert(), thus doing the bisection search through the\nstring list twice in the latter code path.\n\nInstead, just call string_list_insert() right away. If an entry for\nthat peer reference name already existed, then its util pointer is\nalways non-NULL.\n\nOf course this doesn't change the fact that the repeated\nstring_list_insert() calls make the function scale like O(N^2) if the\ninput reference list is not already approximately sorted.\n\nSigned-off-by: Michael Haggerty \n---\n\nThe O(N^2) algorithm mentioned above essentially boils down to\n\n for each reference:\n insert reference into string_list (sorted)\n\nBecause the insertion requires later array entries to be shifted to\nthe right, the algorithm scales like O(N^2) if the entries are not\nalready approximately in order.\n\nI can think of three easy ways to fix this:\n\n* Use a hash map instead of a sorted string_list to record which\n entries have already been seen.\n\n* If the order of the result doesn't matter and it doesn't matter\n *which* of the duplicates is retained, then insert all of the names\n into the string_list unsorted, then sort them all in one go, then\n iterate through looking for duplicates and stringing the survivors\n together in a new linked list.\n\n* If the order is important, or it is important that the *first* of\n duplicates be retained, then iterate through the linked list twice.\n The first time, just add the entries to the string_list. Then sort\n the string_list using a stable sort (e.g., git_qsort()). Then\n iterate through the linked list a second time as in the current\n code.\n\nSee also my comments to commit 10.\n\nHowever, I believe that the O(N^2) worst case is unlikely to bite in\nmost cases. This function is called from\nbuiltin/fetch.c:get_ref_map(), and the input list is the concatenation\nof several sub-lists. Most sublists come from resolving refspecs, and\nthe last comes from find_non_local_tags(). I believe that each of the\nsublists is sorted. So the O(N^2) worst-case could only occur if more\nthan one of the sublists is large and at least two of the large\nsublists are not in the correct order relative to each other.\n\nI tried to concoct such a test case by creating a repository with\nabout 20k branches and 20k tags, instrumenting\nref_remove_duplicates(), and then fetching with something like\n\n git fetch --prune origin refs/heads/*:refs/remotes/origin/* \\\n refs/tags/*:refs/tags/*\n\nIndeed, the running time of ref_remove_duplicates() became significant\nwhen the order of the refspecs was reversed, but it was still less\nthan a second out of a much longer command invocation.\n\nSo I didn't bother fixing the O(N^2) algorithm. If somebody reports\nreal-world slowness here, we can always come back to it.\n\n remote.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex e9fedfa..a44e897 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -750,13 +750,15 @@ void ref_remove_duplicates(struct ref *ref_map)\n \tstruct string_list refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item = NULL;\n \tstruct ref *prev = NULL, *next = NULL;\n+\n \tfor (; ref_map; prev = ref_map, ref_map = next) {\n \t\tnext = ref_map->next;\n \t\tif (!ref_map->peer_ref)\n \t\t\tcontinue;\n \n-\t\titem = string_list_lookup(&refs, ref_map->peer_ref->name);\n-\t\tif (item) {\n+\t\titem = string_list_insert(&refs, ref_map->peer_ref->name);\n+\t\tif (item->util) {\n+\t\t\t/* Entry already existed */\n \t\t\tif (strcmp(((struct ref *)item->util)->name,\n \t\t\t\t ref_map->name))\n \t\t\t\tdie(\"%s tracks both %s and %s\",\n@@ -767,11 +769,9 @@ void ref_remove_duplicates(struct ref *ref_map)\n \t\t\tfree(ref_map->peer_ref);\n \t\t\tfree(ref_map);\n \t\t\tref_map = prev; /* skip this; we freed it */\n-\t\t\tcontinue;\n+\t\t} else {\n+\t\t\titem->util = ref_map;\n \t\t}\n-\n-\t\titem = string_list_insert(&refs, ref_map->peer_ref->name);\n-\t\titem->util = ref_map;\n \t}\n \tstring_list_clear(&refs, 0);\n }\n-- \n1.8.4\n"},{"id":"229339","messageId":"1382543448-2586-8-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 07/15] ref_remove_duplicates(): simplify function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:40Z","receivedAt":"2013-10-23T15:50:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"* Use a dedicated variable, ref, for referring to the current item\n rather than using the ref_map pointer for this purpose.\n\n* Use a (struct ref **) as iteration variable to avoid having to keep\n track of prev and next in addition to the pointer to the current\n item.\n\nSigned-off-by: Michael Haggerty \n---\n remote.c | 32 ++++++++++++++++++--------------\n 1 file changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex a44e897..5ade07f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -748,29 +748,33 @@ int for_each_remote(each_remote_fn fn, void *priv)\n void ref_remove_duplicates(struct ref *ref_map)\n {\n \tstruct string_list refs = STRING_LIST_INIT_NODUP;\n-\tstruct string_list_item *item = NULL;\n-\tstruct ref *prev = NULL, *next = NULL;\n+\tstruct ref **p;\n \n-\tfor (; ref_map; prev = ref_map, ref_map = next) {\n-\t\tnext = ref_map->next;\n-\t\tif (!ref_map->peer_ref)\n+\tfor (p = &ref_map; *p; ) {\n+\t\tstruct ref *ref = *p;\n+\t\tstruct string_list_item *item;\n+\n+\t\tif (!ref->peer_ref) {\n+\t\t\tp = &ref->next;\n \t\t\tcontinue;\n+\t\t}\n \n-\t\titem = string_list_insert(&refs, ref_map->peer_ref->name);\n+\t\titem = string_list_insert(&refs, ref->peer_ref->name);\n \t\tif (item->util) {\n \t\t\t/* Entry already existed */\n \t\t\tif (strcmp(((struct ref *)item->util)->name,\n-\t\t\t\t ref_map->name))\n+\t\t\t\t ref->name))\n \t\t\t\tdie(\"%s tracks both %s and %s\",\n-\t\t\t\t ref_map->peer_ref->name,\n+\t\t\t\t ref->peer_ref->name,\n \t\t\t\t ((struct ref *)item->util)->name,\n-\t\t\t\t ref_map->name);\n-\t\t\tprev->next = ref_map->next;\n-\t\t\tfree(ref_map->peer_ref);\n-\t\t\tfree(ref_map);\n-\t\t\tref_map = prev; /* skip this; we freed it */\n+\t\t\t\t ref->name);\n+\t\t\t/* item is a duplicate; remove and free it */\n+\t\t\t*p = ref->next;\n+\t\t\tfree(ref->peer_ref);\n+\t\t\tfree(ref);\n \t\t} else {\n-\t\t\titem->util = ref_map;\n+\t\t\titem->util = ref;\n+\t\t\tp = &ref->next;\n \t\t}\n \t}\n \tstring_list_clear(&refs, 0);\n-- \n1.8.4\n"},{"id":"229332","messageId":"1382543448-2586-9-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 08/15] ref_remove_duplicates(): improve documentation comment","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:41Z","receivedAt":"2013-10-23T15:50:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n remote.h | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.h b/remote.h\nindex 131130a..40293c0 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -149,7 +149,14 @@ int resolve_remote_symref(struct ref *ref, struct ref *list);\n int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1);\n \n /*\n- * Removes and frees any duplicate refs in the map.\n+ * Remove and free all but the first of any entries in the input list\n+ * that map the same remote reference to the same local reference. If\n+ * there are two entries that map different remote references to the\n+ * same local reference, die.\n+ *\n+ * Note that the first entry is never removed; therefore, the pointer\n+ * passed in as argument still points to the head of the list after\n+ * the function returns.\n */\n void ref_remove_duplicates(struct ref *ref_map);\n \n-- \n1.8.4\n"},{"id":"229340","messageId":"1382543448-2586-10-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 09/15] builtin/fetch.c: reorder function definitions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:42Z","receivedAt":"2013-10-23T15:50:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Reorder function definitions to avoid the need for a forward\ndeclaration of function find_non_local_tags().\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch.c | 198 +++++++++++++++++++++++++++-----------------------------\n 1 file changed, 97 insertions(+), 101 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 2248abf..61e8117 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -160,9 +160,105 @@ static void add_merge_config(struct ref **head,\n \t}\n }\n \n+static int add_existing(const char *refname, const unsigned char *sha1,\n+\t\t\tint flag, void *cbdata)\n+{\n+\tstruct string_list *list = (struct string_list *)cbdata;\n+\tstruct string_list_item *item = string_list_insert(list, refname);\n+\titem->util = xmalloc(20);\n+\thashcpy(item->util, sha1);\n+\treturn 0;\n+}\n+\n+static int will_fetch(struct ref **head, const unsigned char *sha1)\n+{\n+\tstruct ref *rm = *head;\n+\twhile (rm) {\n+\t\tif (!hashcmp(rm->old_sha1, sha1))\n+\t\t\treturn 1;\n+\t\trm = rm->next;\n+\t}\n+\treturn 0;\n+}\n+\n static void find_non_local_tags(struct transport *transport,\n \t\t\tstruct ref **head,\n-\t\t\tstruct ref ***tail);\n+\t\t\tstruct ref ***tail)\n+{\n+\tstruct string_list existing_refs = STRING_LIST_INIT_DUP;\n+\tstruct string_list remote_refs = STRING_LIST_INIT_NODUP;\n+\tconst struct ref *ref;\n+\tstruct string_list_item *item = NULL;\n+\n+\tfor_each_ref(add_existing, &existing_refs);\n+\tfor (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {\n+\t\tif (prefixcmp(ref->name, \"refs/tags/\"))\n+\t\t\tcontinue;\n+\n+\t\t/*\n+\t\t * The peeled ref always follows the matching base\n+\t\t * ref, so if we see a peeled ref that we don't want\n+\t\t * to fetch then we can mark the ref entry in the list\n+\t\t * as one to ignore by setting util to NULL.\n+\t\t */\n+\t\tif (!suffixcmp(ref->name, \"^{}\")) {\n+\t\t\tif (item && !has_sha1_file(ref->old_sha1) &&\n+\t\t\t !will_fetch(head, ref->old_sha1) &&\n+\t\t\t !has_sha1_file(item->util) &&\n+\t\t\t !will_fetch(head, item->util))\n+\t\t\t\titem->util = NULL;\n+\t\t\titem = NULL;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/*\n+\t\t * If item is non-NULL here, then we previously saw a\n+\t\t * ref not followed by a peeled reference, so we need\n+\t\t * to check if it is a lightweight tag that we want to\n+\t\t * fetch.\n+\t\t */\n+\t\tif (item && !has_sha1_file(item->util) &&\n+\t\t !will_fetch(head, item->util))\n+\t\t\titem->util = NULL;\n+\n+\t\titem = NULL;\n+\n+\t\t/* skip duplicates and refs that we already have */\n+\t\tif (string_list_has_string(&remote_refs, ref->name) ||\n+\t\t string_list_has_string(&existing_refs, ref->name))\n+\t\t\tcontinue;\n+\n+\t\titem = string_list_insert(&remote_refs, ref->name);\n+\t\titem->util = (void *)ref->old_sha1;\n+\t}\n+\tstring_list_clear(&existing_refs, 1);\n+\n+\t/*\n+\t * We may have a final lightweight tag that needs to be\n+\t * checked to see if it needs fetching.\n+\t */\n+\tif (item && !has_sha1_file(item->util) &&\n+\t !will_fetch(head, item->util))\n+\t\titem->util = NULL;\n+\n+\t/*\n+\t * For all the tags in the remote_refs string list,\n+\t * add them to the list of refs to be fetched\n+\t */\n+\tfor_each_string_list_item(item, &remote_refs) {\n+\t\t/* Unless we have already decided to ignore this item... */\n+\t\tif (item->util)\n+\t\t{\n+\t\t\tstruct ref *rm = alloc_ref(item->string);\n+\t\t\trm->peer_ref = alloc_ref(item->string);\n+\t\t\thashcpy(rm->old_sha1, item->util);\n+\t\t\t**tail = rm;\n+\t\t\t*tail = &rm->next;\n+\t\t}\n+\t}\n+\n+\tstring_list_clear(&remote_refs, 0);\n+}\n \n static struct ref *get_ref_map(struct transport *transport,\n \t\t\t struct refspec *refspecs, int refspec_count,\n@@ -612,106 +708,6 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)\n \treturn result;\n }\n \n-static int add_existing(const char *refname, const unsigned char *sha1,\n-\t\t\tint flag, void *cbdata)\n-{\n-\tstruct string_list *list = (struct string_list *)cbdata;\n-\tstruct string_list_item *item = string_list_insert(list, refname);\n-\titem->util = xmalloc(20);\n-\thashcpy(item->util, sha1);\n-\treturn 0;\n-}\n-\n-static int will_fetch(struct ref **head, const unsigned char *sha1)\n-{\n-\tstruct ref *rm = *head;\n-\twhile (rm) {\n-\t\tif (!hashcmp(rm->old_sha1, sha1))\n-\t\t\treturn 1;\n-\t\trm = rm->next;\n-\t}\n-\treturn 0;\n-}\n-\n-static void find_non_local_tags(struct transport *transport,\n-\t\t\tstruct ref **head,\n-\t\t\tstruct ref ***tail)\n-{\n-\tstruct string_list existing_refs = STRING_LIST_INIT_DUP;\n-\tstruct string_list remote_refs = STRING_LIST_INIT_NODUP;\n-\tconst struct ref *ref;\n-\tstruct string_list_item *item = NULL;\n-\n-\tfor_each_ref(add_existing, &existing_refs);\n-\tfor (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {\n-\t\tif (prefixcmp(ref->name, \"refs/tags/\"))\n-\t\t\tcontinue;\n-\n-\t\t/*\n-\t\t * The peeled ref always follows the matching base\n-\t\t * ref, so if we see a peeled ref that we don't want\n-\t\t * to fetch then we can mark the ref entry in the list\n-\t\t * as one to ignore by setting util to NULL.\n-\t\t */\n-\t\tif (!suffixcmp(ref->name, \"^{}\")) {\n-\t\t\tif (item && !has_sha1_file(ref->old_sha1) &&\n-\t\t\t !will_fetch(head, ref->old_sha1) &&\n-\t\t\t !has_sha1_file(item->util) &&\n-\t\t\t !will_fetch(head, item->util))\n-\t\t\t\titem->util = NULL;\n-\t\t\titem = NULL;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/*\n-\t\t * If item is non-NULL here, then we previously saw a\n-\t\t * ref not followed by a peeled reference, so we need\n-\t\t * to check if it is a lightweight tag that we want to\n-\t\t * fetch.\n-\t\t */\n-\t\tif (item && !has_sha1_file(item->util) &&\n-\t\t !will_fetch(head, item->util))\n-\t\t\titem->util = NULL;\n-\n-\t\titem = NULL;\n-\n-\t\t/* skip duplicates and refs that we already have */\n-\t\tif (string_list_has_string(&remote_refs, ref->name) ||\n-\t\t string_list_has_string(&existing_refs, ref->name))\n-\t\t\tcontinue;\n-\n-\t\titem = string_list_insert(&remote_refs, ref->name);\n-\t\titem->util = (void *)ref->old_sha1;\n-\t}\n-\tstring_list_clear(&existing_refs, 1);\n-\n-\t/*\n-\t * We may have a final lightweight tag that needs to be\n-\t * checked to see if it needs fetching.\n-\t */\n-\tif (item && !has_sha1_file(item->util) &&\n-\t !will_fetch(head, item->util))\n-\t\titem->util = NULL;\n-\n-\t/*\n-\t * For all the tags in the remote_refs string list,\n-\t * add them to the list of refs to be fetched\n-\t */\n-\tfor_each_string_list_item(item, &remote_refs) {\n-\t\t/* Unless we have already decided to ignore this item... */\n-\t\tif (item->util)\n-\t\t{\n-\t\t\tstruct ref *rm = alloc_ref(item->string);\n-\t\t\trm->peer_ref = alloc_ref(item->string);\n-\t\t\thashcpy(rm->old_sha1, item->util);\n-\t\t\t**tail = rm;\n-\t\t\t*tail = &rm->next;\n-\t\t}\n-\t}\n-\n-\tstring_list_clear(&remote_refs, 0);\n-}\n-\n static void check_not_current_branch(struct ref *ref_map)\n {\n \tstruct branch *current_branch = branch_get(NULL);\n-- \n1.8.4\n"},{"id":"229343","messageId":"1382543448-2586-11-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:43Z","receivedAt":"2013-10-23T15:50:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Previously, fetch's \"--tags\" option was considered equivalent to\nspecifying the refspec \"refs/tags/*:refs/tags/*\" on the command line;\nin particular, it caused the remote..refspec configuration to be\nignored.\n\nBut it is not very useful to fetch tags without also fetching other\nreferences, whereas it *is* quite useful to be able to fetch tags *in\naddition to* other references. So change the semantics of this option\nto do the latter.\n\nIf a user wants to fetch *only* tags, then it is still possible to\nspecifying an explicit refspec:\n\n git fetch 'refs/tags/*:refs/tags/*'\n\nPlease note that the documentation prior to 1.8.0.3 was ambiguous\nabout this aspect of \"fetch --tags\" behavior. Commit\n\n f0cb2f137c 2012-12-14 fetch --tags: clarify documentation\n\nmade the documentation match the old behavior. This commit changes\nthe documentation to match the new behavior.\n\nSigned-off-by: Michael Haggerty \n---\n\nThe change to builtin/fetch.c:get_ref_map() has the side effect of\nchanging the order that the (struct ref) objects are listed in\nref_map. It seems to me that this could probably only matter in the\ncase of duplicates. But later ref_remove_duplicates() is called, so\nduplicates are eliminated. However, ref_remove_duplicates() always\nretains the first instance of duplicates and discards the rest. It is\nconceivable that the boolean flags (which are not inspected by\nref_remove_duplicates()) could differ among the duplicates, and that\ntherefore changing the order of the items in the original list has the\neffect of changing the flags on the items that are retained.\n\nI don't know this code well enough to judge whether this might be a\nproblem.\n\nIf it is, then the correct approach is probably either to teach\nref_remove_duplicates() to ensure that the flags are also consistent\nacross duplicates, or to somehow combine the flags from all duplicates\ninto the one that is retained. Please let me know if this is needed.\n\n Documentation/fetch-options.txt | 8 +++---\n builtin/fetch.c | 46 +++++++++++++++++++-------------\n git-pull.sh | 2 +-\n t/t5510-fetch.sh | 22 ++++++++++++---\n t/t5515/fetch.br-unconfig_--tags_.._.git | 1 +\n t/t5515/fetch.master_--tags_.._.git | 1 +\n t/t5525-fetch-tagopt.sh | 23 ++++++++++++----\n 7 files changed, 71 insertions(+), 32 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex ba1fe49..0e6d2ac 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -61,11 +61,9 @@ endif::git-pull[]\n ifndef::git-pull[]\n -t::\n --tags::\n-\tThis is a short-hand for giving `refs/tags/*:refs/tags/*`\n-\trefspec from the command line, to ask all tags to be fetched\n-\tand stored locally. Because this acts as an explicit\n-\trefspec, the default refspecs (configured with the\n-\tremote.$name.fetch variable) are overridden and not used.\n+\tThis is a short-hand requesting that all tags be fetched from\n+\tthe remote in addition to whatever else is being fetched. It\n+\tis similar to using the refspec `refs/tags/*:refs/tags/*`.\n \n --recurse-submodules[=yes|on-demand|no]::\n \tThis option controls if and under what conditions new commits of\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 61e8117..7edb1ea 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -271,7 +271,7 @@ static struct ref *get_ref_map(struct transport *transport,\n \n \tconst struct ref *remote_refs = transport_get_remote_refs(transport);\n \n-\tif (refspec_count || tags == TAGS_SET) {\n+\tif (refspec_count) {\n \t\tstruct ref **old_tail;\n \n \t\tfor (i = 0; i < refspec_count; i++) {\n@@ -279,11 +279,9 @@ static struct ref *get_ref_map(struct transport *transport,\n \t\t\tif (refspecs[i].dst && refspecs[i].dst[0])\n \t\t\t\t*autotags = 1;\n \t\t}\n-\t\t/* Merge everything on the command line, but not --tags */\n+\t\t/* Merge everything on the command line (but not --tags) */\n \t\tfor (rm = ref_map; rm; rm = rm->next)\n \t\t\trm->fetch_head_status = FETCH_HEAD_MERGE;\n-\t\tif (tags == TAGS_SET)\n-\t\t\tget_fetch_map(remote_refs, tag_refspec, &tail, 0);\n \n \t\t/*\n \t\t * For any refs that we happen to be fetching via command-line\n@@ -334,8 +332,13 @@ static struct ref *get_ref_map(struct transport *transport,\n \t\t\ttail = &ref_map->next;\n \t\t}\n \t}\n-\tif (tags == TAGS_DEFAULT && *autotags)\n+\n+\tif (tags == TAGS_SET)\n+\t\t/* also fetch all tags */\n+\t\tget_fetch_map(remote_refs, tag_refspec, &tail, 0);\n+\telse if (tags == TAGS_DEFAULT && *autotags)\n \t\tfind_non_local_tags(transport, &ref_map, &tail);\n+\n \tref_remove_duplicates(ref_map);\n \n \treturn ref_map;\n@@ -826,31 +829,38 @@ static int do_fetch(struct transport *transport,\n \t\tgoto cleanup;\n \t}\n \tif (prune) {\n-\t\t/*\n-\t\t * If --tags was specified, pretend that the user gave us\n-\t\t * the canonical tags refspec\n-\t\t */\n+\t\tstruct refspec *prune_refspecs;\n+\t\tint prune_refspec_count;\n+\n+\t\tif (ref_count) {\n+\t\t\tprune_refspecs = refs;\n+\t\t\tprune_refspec_count = ref_count;\n+\t\t} else {\n+\t\t\tprune_refspecs = transport->remote->fetch;\n+\t\t\tprune_refspec_count = transport->remote->fetch_refspec_nr;\n+\t\t}\n+\n \t\tif (tags == TAGS_SET) {\n+\t\t\t/*\n+\t\t\t * --tags was specified. Pretend that the user also\n+\t\t\t * gave us the canonical tags refspec\n+\t\t\t */\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\trefspec = xcalloc(prune_refspec_count + 1, sizeof(*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+\t\t\tmemcpy(refspec, prune_refspecs, prune_refspec_count * sizeof(*refspec));\n+\t\t\tmemcpy(&refspec[prune_refspec_count], tags_refspec, sizeof(*refspec));\n \n-\t\t\tprune_refs(refspec, ref_count, ref_map);\n+\t\t\tprune_refs(refspec, prune_refspec_count + 1, 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\t} else {\n-\t\t\tprune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);\n+\t\t\tprune_refs(prune_refspecs, prune_refspec_count, ref_map);\n \t\t}\n \t}\n \tfree_refs(ref_map);\ndiff --git a/git-pull.sh b/git-pull.sh\nindex b946fd9..dac7e1c 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -172,7 +172,7 @@ error_on_no_merge_candidates () {\n \tdo\n \t\tcase \"$opt\" in\n \t\t-t|--t|--ta|--tag|--tags)\n-\t\t\techo \"Fetching tags only, you probably meant:\"\n+\t\t\techo \"It doesn't make sense to pull tags; you probably meant:\"\n \t\t\techo \" git fetch --tags\"\n \t\t\texit 1\n \t\tesac\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 8328be1..02e5901 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_success 'fetch --prune --tags does not delete the remote-tracking branches' '\n+test_expect_success 'fetch --prune --tags prunes tags and branches' '\n \tcd \"$D\" &&\n \tgit clone . prune-tags &&\n \tcd prune-tags &&\n@@ -124,7 +124,7 @@ test_expect_success 'fetch --prune --tags does not delete the remote-tracking br\n \n \tgit fetch --prune --tags origin &&\n \tgit rev-parse origin/master &&\n-\tgit rev-parse origin/fake-remote &&\n+\ttest_must_fail git rev-parse origin/fake-remote &&\n \ttest_must_fail git rev-parse sometag\n '\n \n@@ -132,10 +132,26 @@ test_expect_success 'fetch --prune --tags with branch does not delete other remo\n \tcd \"$D\" &&\n \tgit clone . prune-tags-branch &&\n \tcd prune-tags-branch &&\n+\tgit tag sometag master &&\n \tgit update-ref refs/remotes/origin/extrabranch master &&\n \n \tgit fetch --prune --tags origin master &&\n-\tgit rev-parse origin/extrabranch\n+\tgit rev-parse origin/extrabranch &&\n+\ttest_must_fail git rev-parse sometag\n+'\n+\n+test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' '\n+\tcd \"$D\" &&\n+\tgit clone . prune-tags-refspec &&\n+\tcd prune-tags-refspec &&\n+\tgit tag sometag master &&\n+\tgit update-ref refs/remotes/origin/foo/otherbranch master &&\n+\tgit update-ref refs/remotes/origin/extrabranch master &&\n+\n+\tgit fetch --prune --tags origin refs/heads/foo/*:refs/remotes/origin/foo/* &&\n+\ttest_must_fail git rev-parse refs/remotes/origin/foo/otherbranch &&\n+\tgit rev-parse origin/extrabranch &&\n+\ttest_must_fail git rev-parse sometag\n '\n \n test_expect_success 'fetch tags when there is no tags' '\ndiff --git a/t/t5515/fetch.br-unconfig_--tags_.._.git b/t/t5515/fetch.br-unconfig_--tags_.._.git\nindex 1669cc4..0f70f66 100644\n--- a/t/t5515/fetch.br-unconfig_--tags_.._.git\n+++ b/t/t5515/fetch.br-unconfig_--tags_.._.git\n@@ -1,4 +1,5 @@\n # br-unconfig --tags ../.git\n+0567da4d5edd2ff4bb292a465ba9e64dcad9536b\t\t../\n 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5\tnot-for-merge\ttag 'tag-master' of ../\n 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\ttag 'tag-one' of ../\n 22feea448b023a2d864ef94b013735af34d238ba\tnot-for-merge\ttag 'tag-one-tree' of ../\ndiff --git a/t/t5515/fetch.master_--tags_.._.git b/t/t5515/fetch.master_--tags_.._.git\nindex 8a74935..ab473a6 100644\n--- a/t/t5515/fetch.master_--tags_.._.git\n+++ b/t/t5515/fetch.master_--tags_.._.git\n@@ -1,4 +1,5 @@\n # master --tags ../.git\n+0567da4d5edd2ff4bb292a465ba9e64dcad9536b\t\t../\n 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5\tnot-for-merge\ttag 'tag-master' of ../\n 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\ttag 'tag-one' of ../\n 22feea448b023a2d864ef94b013735af34d238ba\tnot-for-merge\ttag 'tag-one-tree' of ../\ndiff --git a/t/t5525-fetch-tagopt.sh b/t/t5525-fetch-tagopt.sh\nindex 4fbf7a1..45815f7 100755\n--- a/t/t5525-fetch-tagopt.sh\n+++ b/t/t5525-fetch-tagopt.sh\n@@ -8,7 +8,8 @@ setup_clone () {\n \tgit clone --mirror . $1 &&\n \tgit remote add remote_$1 $1 &&\n \t(cd $1 &&\n-\tgit tag tag_$1)\n+\tgit tag tag_$1 &&\n+\tgit branch branch_$1)\n }\n \n test_expect_success setup '\n@@ -21,21 +22,33 @@ test_expect_success setup '\n \n test_expect_success \"fetch with tagopt=--no-tags does not get tag\" '\n \tgit fetch remote_one &&\n-\ttest_must_fail git show-ref tag_one\n+\ttest_must_fail git show-ref tag_one &&\n+\tgit show-ref remote_one/branch_one\n \t'\n \n test_expect_success \"fetch --tags with tagopt=--no-tags gets tag\" '\n+\t(\n+\t\tcd one &&\n+\t\tgit branch second_branch_one\n+\t) &&\n \tgit fetch --tags remote_one &&\n-\tgit show-ref tag_one\n+\tgit show-ref tag_one &&\n+\tgit show-ref remote_one/second_branch_one\n \t'\n \n test_expect_success \"fetch --no-tags with tagopt=--tags does not get tag\" '\n \tgit fetch --no-tags remote_two &&\n-\ttest_must_fail git show-ref tag_two\n+\ttest_must_fail git show-ref tag_two &&\n+\tgit show-ref remote_two/branch_two\n \t'\n \n test_expect_success \"fetch with tagopt=--tags gets tag\" '\n+\t(\n+\t\tcd two &&\n+\t\tgit branch second_branch_two\n+\t) &&\n \tgit fetch remote_two &&\n-\tgit show-ref tag_two\n+\tgit show-ref tag_two &&\n+\tgit show-ref remote_two/second_branch_two\n \t'\n test_done\n-- \n1.8.4\n"},{"id":"229341","messageId":"1382543448-2586-12-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 11/15] fetch --prune: prune only based on explicit refspecs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:44Z","receivedAt":"2013-10-23T15:50:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old behavior of \"fetch --prune\" was to prune whatever was being\nfetched. In particular, \"fetch --prune --tags\" caused tags not only\nto be fetched, but also to be pruned. This is inappropriate because\nthere is only one tags namespace that is shared among the local\nrepository and all remotes. Therefore, if the user defines a local\ntag and then runs \"git fetch --prune --tags\", then the local tag is\ndeleted. Moreover, \"--prune\" and \"--tags\" can also be configured via\nfetch.prune / remote..prune and remote..tagopt, making it\neven less obvious that an invocation of \"git fetch\" could result in\ntag lossage.\n\nSince the command \"git remote update\" invokes \"git fetch\", it had the\nsame problem.\n\nThe command \"git remote prune\", on the other hand, disregarded the\nsetting of remote..tagopt, and so its behavior was inconsistent\nwith that of the other commands.\n\nSo the old behavior made it too easy to lose tags. To fix this\nproblem, change \"fetch --prune\" to prune references based only on\nrefspecs specified explicitly by the user, either on the command line\nor via remote..fetch. Thus, tags are no longer made subject to\npruning by the --tags option or the remote..tagopt setting.\n\nHowever, tags *are* still subject to pruning if they are fetched as\npart of a refspec, and that is good. For example:\n\n* On the command line,\n\n git fetch --prune 'refs/tags/*:refs/tags/*'\n\n causes tags, and only tags, to be fetched and pruned, and is\n therefore a simple way for the user to get the equivalent of the old\n behavior of \"--prune --tag\".\n\n* For a remote that was configured with the \"--mirror\" option, the\n configuration is set to include\n\n [remote \"name\"]\n fetch = +refs/*:refs/*\n\n , which causes tags to be subject to pruning along with all other\n references. This is the behavior that will typically be desired for\n a mirror.\n\nSigned-off-by: Michael Haggerty \n---\n Documentation/config.txt | 2 +-\n Documentation/fetch-options.txt | 15 ++++++++++++---\n builtin/fetch.c | 39 +++++++++------------------------------\n t/t5510-fetch.sh | 10 +++++-----\n 4 files changed, 27 insertions(+), 39 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex d4d93c9..83c1700 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2086,7 +2086,7 @@ remote..vcs::\n remote..prune::\n \tWhen set to true, fetching from this remote by default will also\n \tremove any remote-tracking branches which no longer exist on the\n-\tremote (as if the `--prune` option was give on the command line).\n+\tremote (as if the `--prune` option was given on the command line).\n \tOverrides `fetch.prune` settings, if any.\n \n remotes.::\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 0e6d2ac..5d12219 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -41,8 +41,14 @@ ifndef::git-pull[]\n \n -p::\n --prune::\n-\tAfter fetching, remove any remote-tracking branches which\n-\tno longer exist\ton the remote.\n+\tAfter fetching, remove any remote-tracking branches that\n+\tno longer exist\ton the remote. Tags are not subject to\n+\tpruning in the usual case that they are fetched because of the\n+\t--tags option or remote..tagopt. However, if tags are\n+\tfetched due to an explicit refspec (either on the command line\n+\tor in the remote configuration, for example if the remote was\n+\tcloned with the --mirror option), then they are also subject\n+\tto pruning.\n endif::git-pull[]\n \n ifdef::git-pull[]\n@@ -63,7 +69,10 @@ ifndef::git-pull[]\n --tags::\n \tThis is a short-hand requesting that all tags be fetched from\n \tthe remote in addition to whatever else is being fetched. It\n-\tis similar to using the refspec `refs/tags/*:refs/tags/*`.\n+\tis similar to using the refspec `refs/tags/*:refs/tags/*`,\n+\texcept that it doesn't subject tags to pruning, regardless of\n+\ta --prune option or the configuration settings of fetch.prune\n+\tor remote..prune.\n \n --recurse-submodules[=yes|on-demand|no]::\n \tThis option controls if and under what conditions new commits of\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7edb1ea..47b63a7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -829,38 +829,17 @@ static int do_fetch(struct transport *transport,\n \t\tgoto cleanup;\n \t}\n \tif (prune) {\n-\t\tstruct refspec *prune_refspecs;\n-\t\tint prune_refspec_count;\n-\n+\t\t/*\n+\t\t * We only prune based on refspecs specified\n+\t\t * explicitly (via command line or configuration); we\n+\t\t * don't care whether --tags was specified.\n+\t\t */\n \t\tif (ref_count) {\n-\t\t\tprune_refspecs = refs;\n-\t\t\tprune_refspec_count = ref_count;\n-\t\t} else {\n-\t\t\tprune_refspecs = transport->remote->fetch;\n-\t\t\tprune_refspec_count = transport->remote->fetch_refspec_nr;\n-\t\t}\n-\n-\t\tif (tags == TAGS_SET) {\n-\t\t\t/*\n-\t\t\t * --tags was specified. Pretend that the user also\n-\t\t\t * gave us the canonical tags refspec\n-\t\t\t */\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(prune_refspec_count + 1, sizeof(*refspec));\n-\t\t\ttags_refspec = parse_fetch_refspec(1, &tags_str);\n-\t\t\tmemcpy(refspec, prune_refspecs, prune_refspec_count * sizeof(*refspec));\n-\t\t\tmemcpy(&refspec[prune_refspec_count], tags_refspec, sizeof(*refspec));\n-\n-\t\t\tprune_refs(refspec, prune_refspec_count + 1, ref_map);\n-\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\tprune_refs(refs, ref_count, ref_map);\n \t\t} else {\n-\t\t\tprune_refs(prune_refspecs, prune_refspec_count, ref_map);\n+\t\t\tprune_refs(transport->remote->fetch,\n+\t\t\t\t transport->remote->fetch_refspec_nr,\n+\t\t\t\t ref_map);\n \t\t}\n \t}\n \tfree_refs(ref_map);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 02e5901..5d4581d 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_success 'fetch --prune --tags prunes tags and branches' '\n+test_expect_success 'fetch --prune --tags prunes branches but not tags' '\n \tcd \"$D\" &&\n \tgit clone . prune-tags &&\n \tcd prune-tags &&\n@@ -125,10 +125,10 @@ test_expect_success 'fetch --prune --tags prunes tags and branches' '\n \tgit fetch --prune --tags origin &&\n \tgit rev-parse origin/master &&\n \ttest_must_fail git rev-parse origin/fake-remote &&\n-\ttest_must_fail git rev-parse sometag\n+\tgit rev-parse sometag\n '\n \n-test_expect_success 'fetch --prune --tags with branch does not delete other remote-tracking branches' '\n+test_expect_success 'fetch --prune --tags with branch does not prune other things' '\n \tcd \"$D\" &&\n \tgit clone . prune-tags-branch &&\n \tcd prune-tags-branch &&\n@@ -137,7 +137,7 @@ test_expect_success 'fetch --prune --tags with branch does not delete other remo\n \n \tgit fetch --prune --tags origin master &&\n \tgit rev-parse origin/extrabranch &&\n-\ttest_must_fail git rev-parse sometag\n+\tgit rev-parse sometag\n '\n \n test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' '\n@@ -151,7 +151,7 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'\n \tgit fetch --prune --tags origin refs/heads/foo/*:refs/remotes/origin/foo/* &&\n \ttest_must_fail git rev-parse refs/remotes/origin/foo/otherbranch &&\n \tgit rev-parse origin/extrabranch &&\n-\ttest_must_fail git rev-parse sometag\n+\tgit rev-parse sometag\n '\n \n test_expect_success 'fetch tags when there is no tags' '\n-- \n1.8.4\n"},{"id":"229333","messageId":"1382543448-2586-13-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 12/15] query_refspecs(): move some constants out of the loop","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:45Z","receivedAt":"2013-10-23T15:50:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n remote.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 5ade07f..ff5b62a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -829,6 +829,8 @@ static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *q\n {\n \tint i;\n \tint find_src = !query->src;\n+\tconst char *needle = find_src ? query->dst : query->src;\n+\tchar **result = find_src ? &query->src : &query->dst;\n \n \tif (find_src && !query->dst)\n \t\treturn error(\"query_refspecs: need either src or dst\");\n@@ -837,8 +839,6 @@ static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *q\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-- \n1.8.4\n"},{"id":"229345","messageId":"1382543448-2586-14-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 13/15] builtin/remote.c: reorder function definitions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:46Z","receivedAt":"2013-10-23T15:50:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Reorder function definitions to remove the need for forward\ndeclarations.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/remote.c | 159 +++++++++++++++++++++++++++----------------------------\n 1 file changed, 78 insertions(+), 81 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 4e14891..ecedd96 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -77,9 +77,6 @@ static const char * const builtin_remote_seturl_usage[] = {\n \n static int verbose;\n \n-static int show_all(void);\n-static int prune_remote(const char *remote, int dry_run);\n-\n static inline int postfixcmp(const char *string, const char *postfix)\n {\n \tint len1 = strlen(string), len2 = strlen(postfix);\n@@ -1084,6 +1081,64 @@ static int show_push_info_item(struct string_list_item *item, void *cb_data)\n \treturn 0;\n }\n \n+static int get_one_entry(struct remote *remote, void *priv)\n+{\n+\tstruct string_list *list = priv;\n+\tstruct strbuf url_buf = STRBUF_INIT;\n+\tconst char **url;\n+\tint i, url_nr;\n+\n+\tif (remote->url_nr > 0) {\n+\t\tstrbuf_addf(&url_buf, \"%s (fetch)\", remote->url[0]);\n+\t\tstring_list_append(list, remote->name)->util =\n+\t\t\t\tstrbuf_detach(&url_buf, NULL);\n+\t} else\n+\t\tstring_list_append(list, remote->name)->util = NULL;\n+\tif (remote->pushurl_nr) {\n+\t\turl = remote->pushurl;\n+\t\turl_nr = remote->pushurl_nr;\n+\t} else {\n+\t\turl = remote->url;\n+\t\turl_nr = remote->url_nr;\n+\t}\n+\tfor (i = 0; i < url_nr; i++)\n+\t{\n+\t\tstrbuf_addf(&url_buf, \"%s (push)\", url[i]);\n+\t\tstring_list_append(list, remote->name)->util =\n+\t\t\t\tstrbuf_detach(&url_buf, NULL);\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int show_all(void)\n+{\n+\tstruct string_list list = STRING_LIST_INIT_NODUP;\n+\tint result;\n+\n+\tlist.strdup_strings = 1;\n+\tresult = for_each_remote(get_one_entry, &list);\n+\n+\tif (!result) {\n+\t\tint i;\n+\n+\t\tsort_string_list(&list);\n+\t\tfor (i = 0; i < list.nr; i++) {\n+\t\t\tstruct string_list_item *item = list.items + i;\n+\t\t\tif (verbose)\n+\t\t\t\tprintf(\"%s\\t%s\\n\", item->string,\n+\t\t\t\t\titem->util ? (const char *)item->util : \"\");\n+\t\t\telse {\n+\t\t\t\tif (i && !strcmp((item - 1)->string, item->string))\n+\t\t\t\t\tcontinue;\n+\t\t\t\tprintf(\"%s\\n\", item->string);\n+\t\t\t}\n+\t\t}\n+\t}\n+\tstring_list_clear(&list, 1);\n+\treturn result;\n+}\n+\n static int show(int argc, const char **argv)\n {\n \tint no_query = 0, result = 0, query_flag = 0;\n@@ -1246,26 +1301,6 @@ static int set_head(int argc, const char **argv)\n \treturn result;\n }\n \n-static int prune(int argc, const char **argv)\n-{\n-\tint dry_run = 0, result = 0;\n-\tstruct option options[] = {\n-\t\tOPT__DRY_RUN(&dry_run, N_(\"dry run\")),\n-\t\tOPT_END()\n-\t};\n-\n-\targc = parse_options(argc, argv, NULL, options, builtin_remote_prune_usage,\n-\t\t\t 0);\n-\n-\tif (argc < 1)\n-\t\tusage_with_options(builtin_remote_prune_usage, options);\n-\n-\tfor (; argc; argc--, argv++)\n-\t\tresult |= prune_remote(*argv, dry_run);\n-\n-\treturn result;\n-}\n-\n static int prune_remote(const char *remote, int dry_run)\n {\n \tint result = 0, i;\n@@ -1304,6 +1339,26 @@ static int prune_remote(const char *remote, int dry_run)\n \treturn result;\n }\n \n+static int prune(int argc, const char **argv)\n+{\n+\tint dry_run = 0, result = 0;\n+\tstruct option options[] = {\n+\t\tOPT__DRY_RUN(&dry_run, N_(\"dry run\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, NULL, options, builtin_remote_prune_usage,\n+\t\t\t 0);\n+\n+\tif (argc < 1)\n+\t\tusage_with_options(builtin_remote_prune_usage, options);\n+\n+\tfor (; argc; argc--, argv++)\n+\t\tresult |= prune_remote(*argv, dry_run);\n+\n+\treturn result;\n+}\n+\n static int get_remote_default(const char *key, const char *value, void *priv)\n {\n \tif (strcmp(key, \"remotes.default\") == 0) {\n@@ -1505,64 +1560,6 @@ static int set_url(int argc, const char **argv)\n \treturn 0;\n }\n \n-static int get_one_entry(struct remote *remote, void *priv)\n-{\n-\tstruct string_list *list = priv;\n-\tstruct strbuf url_buf = STRBUF_INIT;\n-\tconst char **url;\n-\tint i, url_nr;\n-\n-\tif (remote->url_nr > 0) {\n-\t\tstrbuf_addf(&url_buf, \"%s (fetch)\", remote->url[0]);\n-\t\tstring_list_append(list, remote->name)->util =\n-\t\t\t\tstrbuf_detach(&url_buf, NULL);\n-\t} else\n-\t\tstring_list_append(list, remote->name)->util = NULL;\n-\tif (remote->pushurl_nr) {\n-\t\turl = remote->pushurl;\n-\t\turl_nr = remote->pushurl_nr;\n-\t} else {\n-\t\turl = remote->url;\n-\t\turl_nr = remote->url_nr;\n-\t}\n-\tfor (i = 0; i < url_nr; i++)\n-\t{\n-\t\tstrbuf_addf(&url_buf, \"%s (push)\", url[i]);\n-\t\tstring_list_append(list, remote->name)->util =\n-\t\t\t\tstrbuf_detach(&url_buf, NULL);\n-\t}\n-\n-\treturn 0;\n-}\n-\n-static int show_all(void)\n-{\n-\tstruct string_list list = STRING_LIST_INIT_NODUP;\n-\tint result;\n-\n-\tlist.strdup_strings = 1;\n-\tresult = for_each_remote(get_one_entry, &list);\n-\n-\tif (!result) {\n-\t\tint i;\n-\n-\t\tsort_string_list(&list);\n-\t\tfor (i = 0; i < list.nr; i++) {\n-\t\t\tstruct string_list_item *item = list.items + i;\n-\t\t\tif (verbose)\n-\t\t\t\tprintf(\"%s\\t%s\\n\", item->string,\n-\t\t\t\t\titem->util ? (const char *)item->util : \"\");\n-\t\t\telse {\n-\t\t\t\tif (i && !strcmp((item - 1)->string, item->string))\n-\t\t\t\t\tcontinue;\n-\t\t\t\tprintf(\"%s\\n\", item->string);\n-\t\t\t}\n-\t\t}\n-\t}\n-\tstring_list_clear(&list, 1);\n-\treturn result;\n-}\n-\n int cmd_remote(int argc, const char **argv, const char *prefix)\n {\n \tstruct option options[] = {\n-- \n1.8.4\n"},{"id":"229334","messageId":"1382543448-2586-15-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 14/15] builtin/remote.c:update(): use struct argv_array","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:47Z","receivedAt":"2013-10-23T15:50:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Use struct argv_array for calling the \"git fetch\" subprocesses.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/remote.c | 33 +++++++++++++++++----------------\n 1 file changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex ecedd96..bffe2f9 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -6,6 +6,7 @@\n #include \"strbuf.h\"\n #include \"run-command.h\"\n #include \"refs.h\"\n+#include \"argv-array.h\"\n \n static const char * const builtin_remote_usage[] = {\n \tN_(\"git remote [-v | --verbose]\"),\n@@ -1376,36 +1377,36 @@ static int update(int argc, const char **argv)\n \t\t\t N_(\"prune remotes after fetching\")),\n \t\tOPT_END()\n \t};\n-\tconst char **fetch_argv;\n-\tint fetch_argc = 0;\n+\tstruct argv_array fetch_argv = ARGV_ARRAY_INIT;\n \tint default_defined = 0;\n-\n-\tfetch_argv = xmalloc(sizeof(char *) * (argc+5));\n+\tint retval;\n \n \targc = parse_options(argc, argv, NULL, options, builtin_remote_update_usage,\n \t\t\t PARSE_OPT_KEEP_ARGV0);\n \n-\tfetch_argv[fetch_argc++] = \"fetch\";\n+\targv_array_push(&fetch_argv, \"fetch\");\n \n \tif (prune)\n-\t\tfetch_argv[fetch_argc++] = \"--prune\";\n+\t\targv_array_push(&fetch_argv, \"--prune\");\n \tif (verbose)\n-\t\tfetch_argv[fetch_argc++] = \"-v\";\n-\tfetch_argv[fetch_argc++] = \"--multiple\";\n+\t\targv_array_push(&fetch_argv, \"-v\");\n+\targv_array_push(&fetch_argv, \"--multiple\");\n \tif (argc < 2)\n-\t\tfetch_argv[fetch_argc++] = \"default\";\n+\t\targv_array_push(&fetch_argv, \"default\");\n \tfor (i = 1; i < argc; i++)\n-\t\tfetch_argv[fetch_argc++] = argv[i];\n+\t\targv_array_push(&fetch_argv, argv[i]);\n \n-\tif (strcmp(fetch_argv[fetch_argc-1], \"default\") == 0) {\n+\tif (strcmp(fetch_argv.argv[fetch_argv.argc-1], \"default\") == 0) {\n \t\tgit_config(get_remote_default, &default_defined);\n-\t\tif (!default_defined)\n-\t\t\tfetch_argv[fetch_argc-1] = \"--all\";\n+\t\tif (!default_defined) {\n+\t\t\targv_array_pop(&fetch_argv);\n+\t\t\targv_array_push(&fetch_argv, \"--all\");\n+\t\t}\n \t}\n \n-\tfetch_argv[fetch_argc] = NULL;\n-\n-\treturn run_command_v_opt(fetch_argv, RUN_GIT_CMD);\n+\tretval = run_command_v_opt(fetch_argv.argv, RUN_GIT_CMD);\n+\targv_array_clear(&fetch_argv);\n+\treturn retval;\n }\n \n static int remove_all_fetch_refspecs(const char *remote, const char *key)\n-- \n1.8.4\n"},{"id":"229342","messageId":"1382543448-2586-16-git-send-email-mhagger@alum.mit.edu","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 15/15] fetch, remote: properly convey --no-prune options to subprocesses","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-23T15:50:48Z","receivedAt":"2013-10-23T15:50:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If --no-prune is passed to one of the following commands:\n\n git fetch --all\n git fetch --multiple\n git fetch --recurse-submodules\n git remote update\n\nthen it must also be passed to the \"fetch\" subprocesses that those\ncommands use to do their work. Otherwise there might be a fetch.prune\nor remote..prune configuration setting that causes pruning to\noccur, contrary to the user's express wish.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch.c | 4 ++--\n builtin/remote.c | 6 +++---\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 47b63a7..8711df0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -915,8 +915,8 @@ static void add_options_to_argv(struct argv_array *argv)\n {\n \tif (dry_run)\n \t\targv_array_push(argv, \"--dry-run\");\n-\tif (prune > 0)\n-\t\targv_array_push(argv, \"--prune\");\n+\tif (prune != -1)\n+\t\targv_array_push(argv, prune ? \"--prune\" : \"--no-prune\");\n \tif (update_head_ok)\n \t\targv_array_push(argv, \"--update-head-ok\");\n \tif (force)\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex bffe2f9..f532f35 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1371,7 +1371,7 @@ static int get_remote_default(const char *key, const char *value, void *priv)\n \n static int update(int argc, const char **argv)\n {\n-\tint i, prune = 0;\n+\tint i, prune = -1;\n \tstruct option options[] = {\n \t\tOPT_BOOL('p', \"prune\", &prune,\n \t\t\t N_(\"prune remotes after fetching\")),\n@@ -1386,8 +1386,8 @@ static int update(int argc, const char **argv)\n \n \targv_array_push(&fetch_argv, \"fetch\");\n \n-\tif (prune)\n-\t\targv_array_push(&fetch_argv, \"--prune\");\n+\tif (prune != -1)\n+\t\targv_array_push(&fetch_argv, prune ? \"--prune\" : \"--no-prune\");\n \tif (verbose)\n \t\targv_array_push(&fetch_argv, \"-v\");\n \targv_array_push(&fetch_argv, \"--multiple\");\n-- \n1.8.4\n"},{"id":"229349","messageId":"xmqqr4bbrkin.fsf@gitster.dls.corp.google.com","threadId":"35183","inReplyTo":"1382543448-2586-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 00/15] Change semantics of \"fetch --tags\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-23T16:59:12Z","receivedAt":"2013-10-23T16:59:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This is my proposed fix for the \"local tag killer\" problem that I\n> reported recently [1].\n>\n> There are three main things changed by this patch series:\n> ...\n\nHaven't looked at any of the 1-15 messages, but the basic design to\ndemote \"--tags\" from being an explicit \"refs/tags/*:refs/tags/*\"\ngiven from the command line to a more special case option sounds\nvery sane and solid.\n\nWith the auto-following of tags, I think it is a _mistake_ to have\n\"--tags\" anywhere on the command line (and \"tagopt\" config) these\ndays, and I do not expect a huge fallout from incompatibility that\narises from the change in behaviour.\n\nThanks---looking forward to reading it through.\n"},{"id":"229358","messageId":"xmqq61snrg0q.fsf@gitster.dls.corp.google.com","threadId":"35183","inReplyTo":"1382543448-2586-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 02/15] t5510: prepare test refs more straightforwardly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-23T18:36:21Z","receivedAt":"2013-10-23T18:36:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> \"git fetch\" was being used with contrived refspecs to create tags and\n> remote-tracking branches in test repositories in preparation for the\n> actual tests. This is obscure and also makes one wonder whether this\n> is indeed just preparation or whether some side-effect of \"git fetch\"\n> is being tested.\n>\n> So use the more straightforward commands \"git tag\" / \"git update-ref\"\n> when preparing branches in test repositories.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> t/t5510-fetch.sh | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index c5e5dfc..08d8dbb 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -88,7 +88,7 @@ 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> +\tgit update-ref refs/remotes/origin/extrabranch master &&\n\nAs long as you have checked that our local 'master' should be at the\nsame commit as the origin's 'master' at this point, I think this\nchange is OK.\n\nI wouldn't call the use of \"very explicit, without any room for\nmistake\" refspecs \"contrived\", though.\n"},{"id":"229359","messageId":"xmqq1u3brfp5.fsf@gitster.dls.corp.google.com","threadId":"35183","inReplyTo":"1382543448-2586-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 04/15] api-remote.txt: correct section \"struct refspect\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-23T18:43:18Z","receivedAt":"2013-10-23T18:43:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>> Subject: Re: [PATCH 04/15] api-remote.txt: correct section \"struct refspect\"\n\nrefspect???\n\n> * Replace reference to function parse_ref_spec() with references to\n> functions parse_fetch_refspec() and parse_push_refspec().\n>\n> * Correct description of src and dst: they *do* include the '*'\n> characters.\n\n * Replace a single SP after a full-stop at the end of sentence with\n double SP as if we were back in the typewriter age.\n\nThe last one made it hard to spot what actually got changed,\nthough. The updated text otherwise looks correct.\n\nThanks.\n\n> Signed-off-by: Michael Haggerty \n> ---\n> Documentation/technical/api-remote.txt | 20 ++++++++++----------\n> 1 file changed, 10 insertions(+), 10 deletions(-)\n>\n> diff --git a/Documentation/technical/api-remote.txt b/Documentation/technical/api-remote.txt\n> index 4be8776..5d245aa 100644\n> --- a/Documentation/technical/api-remote.txt\n> +++ b/Documentation/technical/api-remote.txt\n> @@ -58,16 +58,16 @@ default remote, given the current branch and configuration.\n> struct refspec\n> --------------\n> \n> -A struct refspec holds the parsed interpretation of a refspec. If it\n> -will force updates (starts with a '+'), force is true. If it is a\n> -pattern (sides end with '*') pattern is true. src and dest are the two\n> -sides (if a pattern, only the part outside of the wildcards); if there\n> -is only one side, it is src, and dst is NULL; if sides exist but are\n> -empty (i.e., the refspec either starts or ends with ':'), the\n> -corresponding side is \"\".\n> -\n> -This parsing can be done to an array of strings to give an array of\n> -struct refpsecs with parse_ref_spec().\n> +A struct refspec holds the parsed interpretation of a refspec. If it\n> +will force updates (starts with a '+'), force is true. If it is a\n> +pattern (sides end with '*') pattern is true. src and dest are the\n> +two sides (including '*' characters if present); if there is only one\n> +side, it is src, and dst is NULL; if sides exist but are empty (i.e.,\n> +the refspec either starts or ends with ':'), the corresponding side is\n> +\"\".\n> +\n> +An array of strings can be parsed into an array of struct refspecs\n> +using parse_fetch_refspec() or parse_push_refspec().\n> \n> remote_find_tracking(), given a remote and a struct refspec with\n> either src or dst filled out, will fill out the other such that the\n"},{"id":"229360","messageId":"xmqqwql3q11q.fsf@gitster.dls.corp.google.com","threadId":"35183","inReplyTo":"1382543448-2586-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 05/15] get_ref_map(): rename local variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-23T18:45:05Z","receivedAt":"2013-10-23T18:45:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Rename \"refs\" -> \"refspecs\" and \"ref_count\" -> \"refspec_count\" to\n> reduce confusion, because they describe an array of \"struct refspec\",\n> as opposed to the \"struct ref\" objects that are also used in this\n> function.\n\nGood. In general, we'd prefer to name an array of things that are\nprimarily walked in the index order \"thing[]\", so that \"thing number\n3\" can be spelled thing[3] (not things[3]) in the code, though.\n\n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/fetch.c | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index bd7a101..2248abf 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -165,8 +165,8 @@ static void find_non_local_tags(struct transport *transport,\n> \t\t\tstruct ref ***tail);\n> \n> static struct ref *get_ref_map(struct transport *transport,\n> -\t\t\t struct refspec *refs, int ref_count, int tags,\n> -\t\t\t int *autotags)\n> +\t\t\t struct refspec *refspecs, int refspec_count,\n> +\t\t\t int tags, int *autotags)\n> {\n> \tint i;\n> \tstruct ref *rm;\n> @@ -175,12 +175,12 @@ static struct ref *get_ref_map(struct transport *transport,\n> \n> \tconst struct ref *remote_refs = transport_get_remote_refs(transport);\n> \n> -\tif (ref_count || tags == TAGS_SET) {\n> +\tif (refspec_count || tags == TAGS_SET) {\n> \t\tstruct ref **old_tail;\n> \n> -\t\tfor (i = 0; i < ref_count; i++) {\n> -\t\t\tget_fetch_map(remote_refs, &refs[i], &tail, 0);\n> -\t\t\tif (refs[i].dst && refs[i].dst[0])\n> +\t\tfor (i = 0; i < refspec_count; i++) {\n> +\t\t\tget_fetch_map(remote_refs, &refspecs[i], &tail, 0);\n> +\t\t\tif (refspecs[i].dst && refspecs[i].dst[0])\n> \t\t\t\t*autotags = 1;\n> \t\t}\n> \t\t/* Merge everything on the command line, but not --tags */\n"},{"id":"229361","messageId":"xmqqsivrq0xq.fsf@gitster.dls.corp.google.com","threadId":"35183","inReplyTo":"1382543448-2586-9-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 08/15] ref_remove_duplicates(): improve documentation comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-23T18:47:29Z","receivedAt":"2013-10-23T18:47:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Signed-off-by: Michael Haggerty \n\nUp to this point the patches all look very sensible (modulo minor\nnits I sent separately). Will come back to the rest of the topics\nlater.\n\nThanks.\n\n> ---\n> remote.h | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/remote.h b/remote.h\n> index 131130a..40293c0 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -149,7 +149,14 @@ int resolve_remote_symref(struct ref *ref, struct ref *list);\n> int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1);\n> \n> /*\n> - * Removes and frees any duplicate refs in the map.\n> + * Remove and free all but the first of any entries in the input list\n> + * that map the same remote reference to the same local reference. If\n> + * there are two entries that map different remote references to the\n> + * same local reference, die.\n> + *\n> + * Note that the first entry is never removed; therefore, the pointer\n> + * passed in as argument still points to the head of the list after\n> + * the function returns.\n> */\n> void ref_remove_duplicates(struct ref *ref_map);\n"},{"id":"229381","messageId":"5268C30E.4050906@alum.mit.edu","threadId":"35183","inReplyTo":"xmqq61snrg0q.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 02/15] t5510: prepare test refs more straightforwardly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-24T06:49:50Z","receivedAt":"2013-10-24T06:49:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/23/2013 08:36 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> \"git fetch\" was being used with contrived refspecs to create tags and\n>> remote-tracking branches in test repositories in preparation for the\n>> actual tests. This is obscure and also makes one wonder whether this\n>> is indeed just preparation or whether some side-effect of \"git fetch\"\n>> is being tested.\n>>\n>> So use the more straightforward commands \"git tag\" / \"git update-ref\"\n>> when preparing branches in test repositories.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> t/t5510-fetch.sh | 8 ++++----\n>> 1 file changed, 4 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n>> index c5e5dfc..08d8dbb 100755\n>> --- a/t/t5510-fetch.sh\n>> +++ b/t/t5510-fetch.sh\n>> @@ -88,7 +88,7 @@ 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>> +\tgit update-ref refs/remotes/origin/extrabranch master &&\n> \n> As long as you have checked that our local 'master' should be at the\n> same commit as the origin's 'master' at this point, I think this\n> change is OK.\n\nIt doesn't matter what the reference points at; the only point of these\ntests is to check whether branches that look like stale remote-tracking\nbranches are pruned or not by the later command.\n\n> I wouldn't call the use of \"very explicit, without any room for\n> mistake\" refspecs \"contrived\", though.\n\nAccording to Wiktionary, contrived means \"unnatural, forced\".\n\nWhen the goal is just to create a local reference whose contents are\nirrelevant, \"fetch\" is not the first command that comes to my mind. It\nbrings a lot of unnecessary machinery to bear on such a trivial task.\nPlus it is not very common in daily life to invoke \"fetch\" with a\ncomplicated, asymmetic refspec like this. Because of that it cost me a\nlittle extra time to convince myself that the \"fetch\" command wasn't\ntrying to do something more. In that sense it seems \"contrived\" to me.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"229382","messageId":"5268C704.4020505@alum.mit.edu","threadId":"35183","inReplyTo":"xmqq1u3brfp5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 04/15] api-remote.txt: correct section \"struct refspect\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-24T07:06:44Z","receivedAt":"2013-10-24T07:06:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/23/2013 08:43 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>>> Subject: Re: [PATCH 04/15] api-remote.txt: correct section \"struct refspect\"\n> \n> refspect???\n\nThanks for catching the typo.\n\n>> * Replace reference to function parse_ref_spec() with references to\n>> functions parse_fetch_refspec() and parse_push_refspec().\n>>\n>> * Correct description of src and dst: they *do* include the '*'\n>> characters.\n> \n> * Replace a single SP after a full-stop at the end of sentence with\n> double SP as if we were back in the typewriter age.\n\n:-)\n\nI agree it's archaic, but it is standard practice in English.\n\nAlso, emacs, with the default sentence-end-double-space setting, doesn't\ntreat punctuation followed by a single space as an end of sentence and\nwhen reflowing text even goes to extra effort not to break a line at\nsuch a position (because that would make it look like it *were* the end\nof the sentence).\n\nTeX also distinguishes between interword spaces and end-of-sentence\nspaces, but it uses a different heuristic (which can be overridden by\nexplicit markup). It also typesets them differently: end-of-sentence\nspaces are a bit wider and more elastic.\n\nNevertheless it is probably good that there is no Unicode\nEND_OF_SENTENCE_SPACE character; otherwise we would never get *any* work\ndone for all the arguing about how to encode empty pixels :-)\n\n> The last one made it hard to spot what actually got changed,\n> though. The updated text otherwise looks correct.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"229383","messageId":"5268CB42.4080709@alum.mit.edu","threadId":"35183","inReplyTo":"xmqqwql3q11q.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 05/15] get_ref_map(): rename local variables","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-24T07:24:50Z","receivedAt":"2013-10-24T07:24:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/23/2013 08:45 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Rename \"refs\" -> \"refspecs\" and \"ref_count\" -> \"refspec_count\" to\n>> reduce confusion, because they describe an array of \"struct refspec\",\n>> as opposed to the \"struct ref\" objects that are also used in this\n>> function.\n> \n> Good. In general, we'd prefer to name an array of things that are\n> primarily walked in the index order \"thing[]\", so that \"thing number\n> 3\" can be spelled thing[3] (not things[3]) in the code, though.\n\nSince I didn't change singular -> plural or vice versa in this patch,\nit's a bit off topic, but in case you are curious I prefer plural to\ndistinguish which pointers point at lists or arrays as opposed to single\nobjects. This convention conveniently leaves the singular available to\nname a variable that is used for a single object; for example, in a loop\n\n struct thing thing = things[i];\n\n(The convention in SQL is different: tables are usually named using\nsingular nouns. But that makes sense in SQL because there is not really\na way to reference a single row in a table as an aggregate, so there is\nno need to reserve the singular noun for that purpose. In fact, in\nSELECT statements the table name often appears in a context that makes\nit look like it does refer to a single row:\n\n SELECT employee.name, employee.salary FROM ...\n\nSo I think it makes sense to use different conventions in C vs. SQL.)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"229428","messageId":"xmqq4n86o3c3.fsf@gitster.dls.corp.google.com","threadId":"35183","inReplyTo":"5268C30E.4050906@alum.mit.edu","subject":"Re: [PATCH 02/15] t5510: prepare test refs more straightforwardly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-24T19:50:52Z","receivedAt":"2013-10-24T19:50:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>> As long as you have checked that our local 'master' should be at the\n>> same commit as the origin's 'master' at this point, I think this\n>> change is OK.\n>\n> It doesn't matter what the reference points at; the only point of these\n> tests is to check whether branches that look like stale remote-tracking\n> branches are pruned or not by the later command.\n\nOK, thanks.\n"},{"id":"229433","messageId":"xmqqob6emlxu.fsf@gitster.dls.corp.google.com","threadId":"35183","inReplyTo":"1382543448-2586-11-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-24T20:51:57Z","receivedAt":"2013-10-24T20:51:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Previously, fetch's \"--tags\" option was considered equivalent to\n> specifying the refspec \"refs/tags/*:refs/tags/*\" on the command line;\n> in particular, it caused the remote..refspec configuration to be\n> ignored.\n>\n> But it is not very useful to fetch tags without also fetching other\n> references, whereas it *is* quite useful to be able to fetch tags *in\n> addition to* other references.\n\nTrue but when fetching other references, tags relevant to the\nhistory being fetched by default should automatically follow, so the\nabove explains why \"fetch --tags\" is not a useful thing to do daily.\n\n> So change the semantics of this option\n> to do the latter.\n\nAnd I am not opposed to that change in the semantics; it makes an\noperation that is not so useful less annoying.\n\n> If a user wants to fetch *only* tags, then it is still possible to\n> specifying an explicit refspec:\n>\n> git fetch 'refs/tags/*:refs/tags/*'\n>\n> Please note that the documentation prior to 1.8.0.3 was ambiguous\n> about this aspect of \"fetch --tags\" behavior. Commit\n>\n> f0cb2f137c 2012-12-14 fetch --tags: clarify documentation\n>\n> made the documentation match the old behavior. This commit changes\n> the documentation to match the new behavior.\n\nThe \"old behaviour as designed\". The documentation change was not\nabout refusing to fix a bug but the above makes it sound as if it\nwere a such commit.\n\n> The change to builtin/fetch.c:get_ref_map() has the side effect of\n> changing the order that the (struct ref) objects are listed in\n> ref_map. It seems to me that this could probably only matter in the\n> case of duplicates. But later ref_remove_duplicates() is called, so\n> duplicates are eliminated. However, ref_remove_duplicates() always\n> retains the first instance of duplicates and discards the rest. It is\n> conceivable that the boolean flags (which are not inspected by\n> ref_remove_duplicates()) could differ among the duplicates, and that\n> therefore changing the order of the items in the original list has the\n> effect of changing the flags on the items that are retained.\n>\n> I don't know this code well enough to judge whether this might be a\n> problem.\n\n> If it is, then the correct approach is probably either to teach\n> ref_remove_duplicates() to ensure that the flags are also consistent\n> across duplicates, or to somehow combine the flags from all duplicates\n> into the one that is retained. Please let me know if this is needed.\n\nI do not think either of these two is sufficient if you want to fix\nit; ref_remove_duplicates() needs to be taught to retain the first\none it encounters among the duplicates, not \"ensure the flags are\nconsistent\" by erroring out when they are not among duplicates, nor\n\"somehow combine\" flags from later one to the first one.\n\nBut I suspect that, if this behaviour change were a problem, such a\nconfiguration was a problem before this change to most people; the\norder of duplicated [remote \"foo\"] section would not be under\ncontrol of those who only use \"git remote\" without using an editor\nto tweak .git/config file anyway. So I do not think this regression\nis too big a deal; it is something you can fix later on top.\n\n> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\n> index ba1fe49..0e6d2ac 100644\n> --- a/Documentation/fetch-options.txt\n> +++ b/Documentation/fetch-options.txt\n> @@ -61,11 +61,9 @@ endif::git-pull[]\n> ifndef::git-pull[]\n> -t::\n> --tags::\n> -\tThis is a short-hand for giving `refs/tags/*:refs/tags/*`\n> -\trefspec from the command line, to ask all tags to be fetched\n> -\tand stored locally. Because this acts as an explicit\n> -\trefspec, the default refspecs (configured with the\n> -\tremote.$name.fetch variable) are overridden and not used.\n> +\tThis is a short-hand requesting that all tags be fetched from\n> +\tthe remote in addition to whatever else is being fetched. It\n> +\tis similar to using the refspec `refs/tags/*:refs/tags/*`.\n\nThis is no longer a short-hand, is it? There is no other way to ask\n\"fetch the usual stuff, and then refs/tags/*:refs/tags/* as well\".\n\nIt should be sufficient for me to locally do:\n\n s/This is a short-hand requesting/Request/;\n\nI think.\n\n> diff --git a/git-pull.sh b/git-pull.sh\n> index b946fd9..dac7e1c 100755\n> --- a/git-pull.sh\n> +++ b/git-pull.sh\n> @@ -172,7 +172,7 @@ error_on_no_merge_candidates () {\n> \tdo\n> \t\tcase \"$opt\" in\n> \t\t-t|--t|--ta|--tag|--tags)\n> -\t\t\techo \"Fetching tags only, you probably meant:\"\n> +\t\t\techo \"It doesn't make sense to pull tags; you probably meant:\"\n\ns/pull tags/pull all tags/; perhaps?\n\n> \t\t\techo \" git fetch --tags\"\n> \t\t\texit 1\n> \t\tesac\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 8328be1..02e5901 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_success 'fetch --prune --tags does not delete the remote-tracking branches' '\n> +test_expect_success 'fetch --prune --tags prunes tags and branches' '\n> \tcd \"$D\" &&\n> \tgit clone . prune-tags &&\n> \tcd prune-tags &&\n> @@ -124,7 +124,7 @@ test_expect_success 'fetch --prune --tags does not delete the remote-tracking br\n> \n> \tgit fetch --prune --tags origin &&\n> \tgit rev-parse origin/master &&\n> -\tgit rev-parse origin/fake-remote &&\n> +\ttest_must_fail git rev-parse origin/fake-remote &&\n\nNice.\n\n> \ttest_must_fail git rev-parse sometag\n> '\n> \n> @@ -132,10 +132,26 @@ test_expect_success 'fetch --prune --tags with branch does not delete other remo\n> \tcd \"$D\" &&\n> \tgit clone . prune-tags-branch &&\n> \tcd prune-tags-branch &&\n> +\tgit tag sometag master &&\n> \tgit update-ref refs/remotes/origin/extrabranch master &&\n> \n> \tgit fetch --prune --tags origin master &&\n> -\tgit rev-parse origin/extrabranch\n> +\tgit rev-parse origin/extrabranch &&\n> +\ttest_must_fail git rev-parse sometag\n\nOK, and I'd imagine this will be an issue for a later patch to\naddress...\n\n> diff --git a/t/t5515/fetch.br-unconfig_--tags_.._.git b/t/t5515/fetch.br-unconfig_--tags_.._.git\n> index 1669cc4..0f70f66 100644\n> --- a/t/t5515/fetch.br-unconfig_--tags_.._.git\n> +++ b/t/t5515/fetch.br-unconfig_--tags_.._.git\n> @@ -1,4 +1,5 @@\n> # br-unconfig --tags ../.git\n> +0567da4d5edd2ff4bb292a465ba9e64dcad9536b\t\t../\n> 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5\tnot-for-merge\ttag 'tag-master' of ../\n> 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\ttag 'tag-one' of ../\n> 22feea448b023a2d864ef94b013735af34d238ba\tnot-for-merge\ttag 'tag-one-tree' of ../\n> diff --git a/t/t5515/fetch.master_--tags_.._.git b/t/t5515/fetch.master_--tags_.._.git\n> index 8a74935..ab473a6 100644\n> --- a/t/t5515/fetch.master_--tags_.._.git\n> +++ b/t/t5515/fetch.master_--tags_.._.git\n> @@ -1,4 +1,5 @@\n> # master --tags ../.git\n> +0567da4d5edd2ff4bb292a465ba9e64dcad9536b\t\t../\n> 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5\tnot-for-merge\ttag 'tag-master' of ../\n> 8e32a6d901327a23ef831511badce7bf3bf46689\tnot-for-merge\ttag 'tag-one' of ../\n> 22feea448b023a2d864ef94b013735af34d238ba\tnot-for-merge\ttag 'tag-one-tree' of ../\n\nThis shows us that the updated error message for \"git pull --tags\"\nis much less likely to be given to the user, as we are much more\nlikely to be fetching something we would want to fetch and merge.\nGood.\n\nThanks.\n"},{"id":"229437","messageId":"xmqqiowmml0y.fsf@gitster.dls.corp.google.com","threadId":"35183","inReplyTo":"1382543448-2586-12-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 11/15] fetch --prune: prune only based on explicit refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-24T21:11:41Z","receivedAt":"2013-10-24T21:11:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> ...\n> Signed-off-by: Michael Haggerty \n\nEverything in the proposed log message made sense to me.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index d4d93c9..83c1700 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2086,7 +2086,7 @@ remote..vcs::\n> remote..prune::\n> \tWhen set to true, fetching from this remote by default will also\n> \tremove any remote-tracking branches which no longer exist on the\n> -\tremote (as if the `--prune` option was give on the command line).\n> +\tremote (as if the `--prune` option was given on the command line).\n\nShouldn't we stop saying \"branches\" here?\n\n> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\n> index 0e6d2ac..5d12219 100644\n> --- a/Documentation/fetch-options.txt\n> +++ b/Documentation/fetch-options.txt\n> @@ -41,8 +41,14 @@ ifndef::git-pull[]\n> \n> -p::\n> --prune::\n> -\tAfter fetching, remove any remote-tracking branches which\n> -\tno longer exist\ton the remote.\n> +\tAfter fetching, remove any remote-tracking branches that\n\nLikewise. This is a lot more important than the one in\nremote..prune documentation, as the next sentence \"Tags are\nnot subject to ...\" implies that they fall into the same category as\nwhat gets pruned here, i.e. \"remote-tracking branches\" in the above\nsentence, but nobody calls refs/tags/v1.0.0 a \"remote-tracking\nbranch\" even if it came from your 'origin'.\n\n> +\tno longer exist\ton the remote. Tags are not subject to\n> +\tpruning in the usual case that they are fetched because of the\n> +\t--tags option or remote..tagopt. \n\nWe should mention the most usual case tags are fetched, before\nmentioning the case the unusual option \"--tags\" was used from the\ncommand line or .tagopt configuration was used. Namely, when the\ntags are automatically followed.\n\n> + However, if tags are\n> +\tfetched due to an explicit refspec (either on the command line\n> +\tor in the remote configuration, for example if the remote was\n> +\tcloned with the --mirror option), then they are also subject\n> +\tto pruning.\n\nVery good.\n\n> @@ -63,7 +69,10 @@ ifndef::git-pull[]\n> --tags::\n> \tThis is a short-hand requesting that all tags be fetched from\n> \tthe remote in addition to whatever else is being fetched. It\n> -\tis similar to using the refspec `refs/tags/*:refs/tags/*`.\n> +\tis similar to using the refspec `refs/tags/*:refs/tags/*`,\n> +\texcept that it doesn't subject tags to pruning, regardless of\n> +\ta --prune option or the configuration settings of fetch.prune\n> +\tor remote..prune.\n\nUsing --tags is not similar to using refs/tags/*:refs/tags/* after\nthe previous patch already; \"git fetch origin --tags\" and \"git fetch\norigin refs/tags/*:refs/tags/*\" are vastly different and that was\nthe whole point of the previous step. And that \"calling something\nnot so similar similar\" needs to be fixed further here to clarify\nthat they are not similar in yet another way.\n\nWe should just lose \"It is similar to using\" from 10/15 and start\nover, perhaps? Add the first paragraph of the below in 10/15 and\nadd the rest in 11/15, or something.\n\n\t--tags::\n\t\tRequest that all tags be fetched from the remote\n\t\tunder the same name (i.e. `refs/tags/X` is created in\n\t\tour repository by copying their `refs/tags/X`), in\n\t\taddition to whatever is fetched by the same `git\n\t\tfetch` command without this option on the command\n\t\tline.\n\t+\n When `refs/tags/*` hierarchy from the remote is copied only\n because this option was given, they are not subject to be\n\tpruned when `--prune` option (or configuration variables\n\tlike `fetch.prune` or `remote..prune`) is in effect.\n\nThat would make it clear that they are subject to pruning when --mirror\nor an explicit refspec refs/tags/*:refs/tags/* is given, as tags are\nnot fetched \"only because of --tags\" in such cases.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 7edb1ea..47b63a7 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -829,38 +829,17 @@ static int do_fetch(struct transport *transport,\n> \t\tgoto cleanup;\n> \t}\n> \tif (prune) {\n> -\t\tstruct refspec *prune_refspecs;\n> -\t\tint prune_refspec_count;\n> -\n> +\t\t/*\n> +\t\t * We only prune based on refspecs specified\n> +\t\t * explicitly (via command line or configuration); we\n> +\t\t * don't care whether --tags was specified.\n> +\t\t */\n> \t\tif (ref_count) {\n> -\t\t\tprune_refspecs = refs;\n> -\t\t\tprune_refspec_count = ref_count;\n> -\t\t} else {\n> -\t\t\tprune_refspecs = transport->remote->fetch;\n> -\t\t\tprune_refspec_count = transport->remote->fetch_refspec_nr;\n> -\t\t}\n> -\n> -\t\tif (tags == TAGS_SET) {\n> -\t\t\t/*\n> -\t\t\t * --tags was specified. Pretend that the user also\n> -\t\t\t * gave us the canonical tags refspec\n> -\t\t\t */\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(prune_refspec_count + 1, sizeof(*refspec));\n> -\t\t\ttags_refspec = parse_fetch_refspec(1, &tags_str);\n> -\t\t\tmemcpy(refspec, prune_refspecs, prune_refspec_count * sizeof(*refspec));\n> -\t\t\tmemcpy(&refspec[prune_refspec_count], tags_refspec, sizeof(*refspec));\n> -\n> -\t\t\tprune_refs(refspec, prune_refspec_count + 1, ref_map);\n> -\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\tprune_refs(refs, ref_count, ref_map);\n> \t\t} else {\n> -\t\t\tprune_refs(prune_refspecs, prune_refspec_count, ref_map);\n> +\t\t\tprune_refs(transport->remote->fetch,\n> +\t\t\t\t transport->remote->fetch_refspec_nr,\n> +\t\t\t\t ref_map);\n> \t\t}\n> \t}\n\nNice.\n"},{"id":"229438","messageId":"xmqqeh7amkrs.fsf@gitster.dls.corp.google.com","threadId":"35183","inReplyTo":"1382543448-2586-16-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 15/15] fetch, remote: properly convey --no-prune options to subprocesses","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-24T21:17:11Z","receivedAt":"2013-10-24T21:17:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I finished reading thru the remainder and all looked good (again\nexcept minor nits I sent reviews for separately).\n\nThanks, will queue.\n"},{"id":"229518","messageId":"526A896D.7050801@alum.mit.edu","threadId":"35183","inReplyTo":"xmqqob6emlxu.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-25T15:08:29Z","receivedAt":"2013-10-25T15:08:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/24/2013 10:51 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Previously, fetch's \"--tags\" option was considered equivalent to\n>> specifying the refspec \"refs/tags/*:refs/tags/*\" on the command line;\n>> in particular, it caused the remote..refspec configuration to be\n>> ignored.\n>>\n>> But it is not very useful to fetch tags without also fetching other\n>> references, whereas it *is* quite useful to be able to fetch tags *in\n>> addition to* other references.\n> \n> True but when fetching other references, tags relevant to the\n> history being fetched by default should automatically follow, so the\n> above explains why \"fetch --tags\" is not a useful thing to do daily.\n\nMaybe not necessary in many scenarios, but is it harmful for the common\ncase of cloning from and then periodically fetching from an upstream?\nISTM that the result of the declarative --tags option\n\n we have all upstream tags\n\nis easier to reason about than the history-dependent tag-following behavior\n\n we have all upstream tags that point to commits that were\n reachable from an upstream branch at the time of this or one\n of the previous fetches\n\nI just noticed that this is not explained very clearly in the fetch man\npage. I will try to improve it.\n\n>> If a user wants to fetch *only* tags, then it is still possible to\n>> specifying an explicit refspec:\n>>\n>> git fetch 'refs/tags/*:refs/tags/*'\n>>\n>> Please note that the documentation prior to 1.8.0.3 was ambiguous\n>> about this aspect of \"fetch --tags\" behavior. Commit\n>>\n>> f0cb2f137c 2012-12-14 fetch --tags: clarify documentation\n>>\n>> made the documentation match the old behavior. This commit changes\n>> the documentation to match the new behavior.\n> \n> The \"old behaviour as designed\". The documentation change was not\n> about refusing to fix a bug but the above makes it sound as if it\n> were a such commit.\n\nI didn't mean to imply this. My point in mentioning the documentation\nchange was that even though the old behavior has been in effect for a\nwhile, it was not clearly documented until quite recently. But I guess\nit is a moot point.\n\n>> The change to builtin/fetch.c:get_ref_map() has the side effect of\n>> changing the order that the (struct ref) objects are listed in\n>> ref_map. It seems to me that this could probably only matter in the\n>> case of duplicates. But later ref_remove_duplicates() is called, so\n>> duplicates are eliminated. However, ref_remove_duplicates() always\n>> retains the first instance of duplicates and discards the rest. It is\n>> conceivable that the boolean flags (which are not inspected by\n>> ref_remove_duplicates()) could differ among the duplicates, and that\n>> therefore changing the order of the items in the original list has the\n>> effect of changing the flags on the items that are retained.\n>>\n>> I don't know this code well enough to judge whether this might be a\n>> problem.\n> \n>> If it is, then the correct approach is probably either to teach\n>> ref_remove_duplicates() to ensure that the flags are also consistent\n>> across duplicates, or to somehow combine the flags from all duplicates\n>> into the one that is retained. Please let me know if this is needed.\n> \n> I do not think either of these two is sufficient if you want to fix\n> it; ref_remove_duplicates() needs to be taught to retain the first\n> one it encounters among the duplicates, not \"ensure the flags are\n> consistent\" by erroring out when they are not among duplicates, nor\n> \"somehow combine\" flags from later one to the first one.\n> \n> But I suspect that, if this behaviour change were a problem, such a\n> configuration was a problem before this change to most people; the\n> order of duplicated [remote \"foo\"] section would not be under\n> control of those who only use \"git remote\" without using an editor\n> to tweak .git/config file anyway. So I do not think this regression\n> is too big a deal; it is something you can fix later on top.\n\nI'll address your second point first: I agree that the order among\nexplicit refspecs couldn't really be a problem because it is already\narbitrary. However, get_ref_map() adds references from other sources to\nthe result list. For example, if there are command-line arguments, it adds\n\n1. command-line arguments (with fetch_head_status=FETCH_HEAD_MERGE)\n\n2. configured refspecs (with fetch_head_status=FETCH_HEAD_IGNORE)\n\n3. if (--tags)\n then tags_refspec (with fetch_head_status=FETCH_HEAD_NOT_FOR_MERGE)\n else the result of find_non_local_tags() (with\n fetch_head_status=FETCH_HEAD_NOT_FOR_MERGE)\n\nIt could very well be that the order matters across different classes of\nreference.\n\nRegarding your first point: if it matters which of the duplicates is\nchosen, then it can only matter because they differ in some other way\nthan their reference names, for example in their flags. So a robust way\nof de-duping them (if it is possible) would be to compare the two\nrecords and decide which one should take precedence based on this other\ninformation rather than based on which one happened to come earlier in\nthe list. Then the list order would be immaterial and (for example) it\nwouldn't be a problem to reorder the items.\n\nI'm being very wishy-washy because I haven't yet invested the time to\nlearn how these lists of refs are used. Maybe I need to do so.\n\nI have run out of time, so I will address your comments about the\ndocumentation changes in a separate email.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"229546","messageId":"526B4EB7.7020004@alum.mit.edu","threadId":"35183","inReplyTo":"xmqqob6emlxu.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-26T05:10:15Z","receivedAt":"2013-10-26T05:10:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/24/2013 10:51 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\n>> index ba1fe49..0e6d2ac 100644\n>> --- a/Documentation/fetch-options.txt\n>> +++ b/Documentation/fetch-options.txt\n>> @@ -61,11 +61,9 @@ endif::git-pull[]\n>> ifndef::git-pull[]\n>> -t::\n>> --tags::\n>> -\tThis is a short-hand for giving `refs/tags/*:refs/tags/*`\n>> -\trefspec from the command line, to ask all tags to be fetched\n>> -\tand stored locally. Because this acts as an explicit\n>> -\trefspec, the default refspecs (configured with the\n>> -\tremote.$name.fetch variable) are overridden and not used.\n>> +\tThis is a short-hand requesting that all tags be fetched from\n>> +\tthe remote in addition to whatever else is being fetched. It\n>> +\tis similar to using the refspec `refs/tags/*:refs/tags/*`.\n> \n> This is no longer a short-hand, is it? There is no other way to ask\n> \"fetch the usual stuff, and then refs/tags/*:refs/tags/* as well\".\n> \n> It should be sufficient for me to locally do:\n> \n> s/This is a short-hand requesting/Request/;\n> \n> I think.\n\nYes, that's better.\n\n>> diff --git a/git-pull.sh b/git-pull.sh\n>> index b946fd9..dac7e1c 100755\n>> --- a/git-pull.sh\n>> +++ b/git-pull.sh\n>> @@ -172,7 +172,7 @@ error_on_no_merge_candidates () {\n>> \tdo\n>> \t\tcase \"$opt\" in\n>> \t\t-t|--t|--ta|--tag|--tags)\n>> -\t\t\techo \"Fetching tags only, you probably meant:\"\n>> +\t\t\techo \"It doesn't make sense to pull tags; you probably meant:\"\n> \n> s/pull tags/pull all tags/; perhaps?\n\nYes, that's also an improvement.\n\nThanks,\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"229549","messageId":"526B65E8.1070900@alum.mit.edu","threadId":"35183","inReplyTo":"xmqqiowmml0y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 11/15] fetch --prune: prune only based on explicit refspecs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-26T06:49:12Z","receivedAt":"2013-10-26T06:49:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/24/2013 11:11 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> ...\n>> Signed-off-by: Michael Haggerty \n> \n> Everything in the proposed log message made sense to me.\n> \n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index d4d93c9..83c1700 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -2086,7 +2086,7 @@ remote..vcs::\n>> remote..prune::\n>> \tWhen set to true, fetching from this remote by default will also\n>> \tremove any remote-tracking branches which no longer exist on the\n>> -\tremote (as if the `--prune` option was give on the command line).\n>> +\tremote (as if the `--prune` option was given on the command line).\n> \n> Shouldn't we stop saying \"branches\" here?\n> \n>> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\n>> index 0e6d2ac..5d12219 100644\n>> --- a/Documentation/fetch-options.txt\n>> +++ b/Documentation/fetch-options.txt\n>> @@ -41,8 +41,14 @@ ifndef::git-pull[]\n>> \n>> -p::\n>> --prune::\n>> -\tAfter fetching, remove any remote-tracking branches which\n>> -\tno longer exist\ton the remote.\n>> +\tAfter fetching, remove any remote-tracking branches that\n> \n> Likewise. This is a lot more important than the one in\n> remote..prune documentation, as the next sentence \"Tags are\n> not subject to ...\" implies that they fall into the same category as\n> what gets pruned here, i.e. \"remote-tracking branches\" in the above\n> sentence, but nobody calls refs/tags/v1.0.0 a \"remote-tracking\n> branch\" even if it came from your 'origin'.\n\nOK, I will change both of the above from \"remote-tracking branches\" to\n\"remote-tracking references\".\n\n>> +\tno longer exist\ton the remote. Tags are not subject to\n>> +\tpruning in the usual case that they are fetched because of the\n>> +\t--tags option or remote..tagopt. \n> \n> We should mention the most usual case tags are fetched, before\n> mentioning the case the unusual option \"--tags\" was used from the\n> command line or .tagopt configuration was used. Namely, when the\n> tags are automatically followed.\n\nOK, I will change this in the next draft.\n\n>> @@ -63,7 +69,10 @@ ifndef::git-pull[]\n>> --tags::\n>> \tThis is a short-hand requesting that all tags be fetched from\n>> \tthe remote in addition to whatever else is being fetched. It\n>> -\tis similar to using the refspec `refs/tags/*:refs/tags/*`.\n>> +\tis similar to using the refspec `refs/tags/*:refs/tags/*`,\n>> +\texcept that it doesn't subject tags to pruning, regardless of\n>> +\ta --prune option or the configuration settings of fetch.prune\n>> +\tor remote..prune.\n> \n> Using --tags is not similar to using refs/tags/*:refs/tags/* after\n> the previous patch already; \"git fetch origin --tags\" and \"git fetch\n> origin refs/tags/*:refs/tags/*\" are vastly different and that was\n> the whole point of the previous step. And that \"calling something\n> not so similar similar\" needs to be fixed further here to clarify\n> that they are not similar in yet another way.\n> \n> We should just lose \"It is similar to using\" from 10/15 and start\n> over, perhaps? Add the first paragraph of the below in 10/15 and\n> add the rest in 11/15, or something.\n> \n> \t--tags::\n> \t\tRequest that all tags be fetched from the remote\n> \t\tunder the same name (i.e. `refs/tags/X` is created in\n> \t\tour repository by copying their `refs/tags/X`), in\n> \t\taddition to whatever is fetched by the same `git\n> \t\tfetch` command without this option on the command\n> \t\tline.\n> \t+\n> When `refs/tags/*` hierarchy from the remote is copied only\n> because this option was given, they are not subject to be\n> \tpruned when `--prune` option (or configuration variables\n> \tlike `fetch.prune` or `remote..prune`) is in effect.\n> \n> That would make it clear that they are subject to pruning when --mirror\n> or an explicit refspec refs/tags/*:refs/tags/* is given, as tags are\n> not fetched \"only because of --tags\" in such cases.\n\nI see your point. What do you think about the following version, which\nis a bit more compact and refers the reader to --prune for the full story:\n\n-t::\n--tags::\n\tFetch all tags from the remote (i.e., fetch remote tags\n\t`refs/tags/*` into local tags with the same name), in addition\n\tto whatever else would otherwise be fetched. Using this\n\toption does not subject tags to pruning, even if --prune is\n\tused (though tags may be pruned anyway if they are also the\n\tdestination of an explicit refspec; see '--prune').\n\nI also want to improve the description of tag auto-following in general.\n I will send a re-rolled patch series in the next couple of days.\n\nThanks for your prompt and helpful advice!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"229641","messageId":"xmqqmwlth1r5.fsf@gitster.dls.corp.google.com","threadId":"35183","inReplyTo":"526B65E8.1070900@alum.mit.edu","subject":"Re: [PATCH 11/15] fetch --prune: prune only based on explicit refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-28T15:08:14Z","receivedAt":"2013-10-28T15:08:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 10/24/2013 11:11 PM, Junio C Hamano wrote:\n> ...\n>> We should just lose \"It is similar to using\" from 10/15 and start\n>> over, perhaps? Add the first paragraph of the below in 10/15 and\n>> add the rest in 11/15, or something.\n>> \n>> \t--tags::\n>> \t\tRequest that all tags be fetched from the remote\n>> \t\tunder the same name (i.e. `refs/tags/X` is created in\n>> \t\tour repository by copying their `refs/tags/X`), in\n>> \t\taddition to whatever is fetched by the same `git\n>> \t\tfetch` command without this option on the command\n>> \t\tline.\n>> \t+\n>> When `refs/tags/*` hierarchy from the remote is copied only\n>> because this option was given, they are not subject to be\n>> \tpruned when `--prune` option (or configuration variables\n>> \tlike `fetch.prune` or `remote..prune`) is in effect.\n>> \n>> That would make it clear that they are subject to pruning when --mirror\n>> or an explicit refspec refs/tags/*:refs/tags/* is given, as tags are\n>> not fetched \"only because of --tags\" in such cases.\n>\n> I see your point. What do you think about the following version, which\n> is a bit more compact and refers the reader to --prune for the full story:\n>\n> -t::\n> --tags::\n> \tFetch all tags from the remote (i.e., fetch remote tags\n> \t`refs/tags/*` into local tags with the same name), in addition\n> \tto whatever else would otherwise be fetched. Using this\n> \toption does not subject tags to pruning, even if --prune is\n> \tused (though tags may be pruned anyway if they are also the\n> \tdestination of an explicit refspec; see '--prune').\n\nI like the first sentence of yours better. The second sentence feels\nsomewhat iffy, though. --tags refs/tags/*:refs/tags/* will allow\ntags to be pruned, so s/option does not/option alone does not/ needs\nto be done to be precise, at least.\n\nThanks.\n"},{"id":"229657","messageId":"xmqqa9htfbzn.fsf@gitster.dls.corp.google.com","threadId":"35183","inReplyTo":"526A896D.7050801@alum.mit.edu","subject":"Re: [PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-28T19:10:04Z","receivedAt":"2013-10-28T19:10:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>> True but when fetching other references, tags relevant to the\n>> history being fetched by default should automatically follow, so the\n>> above explains why \"fetch --tags\" is not a useful thing to do daily.\n>\n> Maybe not necessary in many scenarios, but is it harmful for the common\n> case of cloning from and then periodically fetching from an upstream?\n\nThere is no mention of \"harmful\"; I only said \"not useful\". And it\ncomes primarily from knowing why \"--tags\" was introduced in the\nfirst place; I should have said \"less useful than before, ever since\nwe started to reliably auto-follow\".\n\nThe only \"harmful\" part is its interaction with \"--prune\", which\nyour series nicely addresses.\n\n> ISTM that the result of the declarative --tags option\n>\n> we have all upstream tags\n>\n> is easier to reason about than the history-dependent tag-following behavior\n\nI'd say \"we have all the relevant tags\" and \"we have all the tags\nthe other side has\" are equally valid things to wish for, depending\nwho you are and how your project is organized, and one is not\nnecessarily easy/useful than the other. In case it was unclear, I\nwas not seriously advocating to deprecate/remove \"--tags\".\n\n> Regarding your first point: if it matters which of the duplicates is\n> chosen, then it can only matter because they differ in some other way\n> than their reference names, for example in their flags. So a robust way\n> of de-duping them (if it is possible) would be to compare the two\n> records and decide which one should take precedence based on this other\n> information rather than based on which one happened to come earlier in\n> the list. Then the list order would be immaterial and (for example) it\n> wouldn't be a problem to reorder the items.\n\nBut that changes the behaviour to those who has cared to rely on the\nordering. With the change to first collect and then sort unstably\nbefore deduping, the series already tells them not to rely on the\norder, and two of us tentatively agreed in this discussion that it\nis not likely to matter. If later this agreement turns out to be a\nmistake and there are people who *do* rely on the ordering, the only\nacceptable fix for them is by making sure we restore the \"first one\ntrumps\" semantics, not by defining an alternative, arguably better,\nbehaviour---that is not a regression fix.\n\nIn any case, thanks for working on this topic.\n"},{"id":"229753","messageId":"52708A81.7060300@alum.mit.edu","threadId":"35183","inReplyTo":"xmqqa9htfbzn.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-10-30T04:26:41Z","receivedAt":"2013-10-30T04:26:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/28/2013 08:10 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>>> True but when fetching other references, tags relevant to the\n>>> history being fetched by default should automatically follow, so the\n>>> above explains why \"fetch --tags\" is not a useful thing to do daily.\n>>\n>> Maybe not necessary in many scenarios, but is it harmful for the common\n>> case of cloning from and then periodically fetching from an upstream?\n> \n> There is no mention of \"harmful\"; I only said \"not useful\". And it\n> comes primarily from knowing why \"--tags\" was introduced in the\n> first place; I should have said \"less useful than before, ever since\n> we started to reliably auto-follow\".\n> \n> The only \"harmful\" part is its interaction with \"--prune\", which\n> your series nicely addresses.\n\nOK, then we are in agreement.\n\n>> ISTM that the result of the declarative --tags option\n>>\n>> we have all upstream tags\n>>\n>> is easier to reason about than the history-dependent tag-following behavior\n> \n> I'd say \"we have all the relevant tags\" and \"we have all the tags\n> the other side has\" are equally valid things to wish for, depending\n> who you are and how your project is organized, and one is not\n> necessarily easy/useful than the other. In case it was unclear, I\n> was not seriously advocating to deprecate/remove \"--tags\".\n\nYes, I agree that both are valid things to wish for. I guess I was\nmostly thinking about which would be a better default.\n\n\"clone\" and \"remote add\", by default, configure the repo to fetch all\nbranches on the remote. For this default setup, what are the pros and\ncons of the two tag-fetching options (assuming that this patch series\nhas fixed the problem with tag pruning)?\n\nPros of auto-following:\n\n* Doesn't require a change to the status quo\n\n* The the user can change the refspec to be more restrictive without\nhaving to change the tag-following option and it continues to do the\nright thing.\n\n* If the project has branches outside of refs/heads (which would not, by\ndefault, be fetched--think continuous integration artifacts) and also\nhas tags pointing at those branches, the user might get unwanted\ncontents with \"--tags\", but not with auto-following.\n\n\nPros of --tags:\n\n* Easier to understand (?) because result is not history-dependent.\n\n* Avoids missing tags that are not directly on a branch:\n\n o---o---o---o <- branch\n \\\n o <- tag\n\nSo it's not obvious that one is better than the other. I think if I\nwere choosing the behavior for the first time, I would favor --tags.\nBut I don't think the advantage is strong enough to make it worth\nchanging the existing default.\n\n>> Regarding your first point: if it matters which of the duplicates is\n>> chosen, then it can only matter because they differ in some other way\n>> than their reference names, for example in their flags. So a robust way\n>> of de-duping them (if it is possible) would be to compare the two\n>> records and decide which one should take precedence based on this other\n>> information rather than based on which one happened to come earlier in\n>> the list. Then the list order would be immaterial and (for example) it\n>> wouldn't be a problem to reorder the items.\n> \n> But that changes the behaviour to those who has cared to rely on the\n> ordering. With the change to first collect and then sort unstably\n> before deduping, the series already tells them not to rely on the\n> order, and two of us tentatively agreed in this discussion that it\n> is not likely to matter. If later this agreement turns out to be a\n> mistake and there are people who *do* rely on the ordering, the only\n> acceptable fix for them is by making sure we restore the \"first one\n> trumps\" semantics, not by defining an alternative, arguably better,\n> behaviour---that is not a regression fix.\n\nPlease note that the current patch series does *not* change the\nalgorithm to use an unstable sort; that was only a suggestion for the\nfuture should somebody discover that the O(N^2) algorithm in this\nfunction is a performance bottleneck.\n\nWhat the old patch series *did* do was change the ordering that\nget_ref_map() adds references to the list in the case of (refspec_count\n&& tags == TAGS_SET) by moving the (tags == TAGS_SET) handling outside\nof the \"true\" branch of the (refspec_count) conditional. This had the\nresult that the references added by\n\n get_fetch_map(remote_refs, tag_refspec, &tail, 0);\n\ncame after, rather than before, the references opportunistically being\nfetched with FETCH_HEAD_IGNORE status.\n\nBut I dug even deeper and found that there was a (rather obscure)\nsituation where the ordering change could lead to incorrect behavior,\nnamely if all of the following are true:\n\n* there is a configured refspec for tags, like refs/tags/*:refs/tags/*\n\n* the user runs fetch with the --tags option and also with an explicit\nrefspec on the command line to fetch a remote tag (e.g.,\nrefs/tags/foo:refs/heads/foo).\n\nIn that case (after this version of this patch), an entry for\nrefs/tags/foo:refs/heads/foo would be added to the list, then the\nopportunistic \"refs/tags/foo:refs/tags/foo\" would be added with\nFETCH_HEAD_IGNORE. Then the --tags option would be processed, adding a\nsecond record \"refs/tags/foo:refs/tags/foo\" to the list, but this time\nwith FETCH_HEAD_NOT_FOR_MERGE. The call to ref_remove_duplicates()\nwould remove the last entry, leaving the entry with FETCH_HEAD_IGNORE\ninstead of the (correct) entry with FETCH_HEAD_NOT_FOR_MERGE. The\nresult would be that refs/tags/foo would not be written to FETCH_HEAD.\n\nSo I will re-roll this series with a few extra patches that first, avoid\nsubjecting the --tags entries to the opportunistic-update machinery (it\nis redundant), and also preserve the old order where the --tags entries\nprecede the opportunistic entries in the list.\n\nThanks again for your comments!\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}