{"thread":{"id":"33795","subject":"[RFC/PATCH 0/4] update tracking refs on explicit fetch","startedAt":"2013-05-11T16:13:21Z","lastAt":"2013-08-06T22:12:51Z","messageCount":12,"participants":["Jeff King","Thomas Rast","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"217008","messageId":"20130511161320.GA14990@sigill.intra.peff.net","threadId":"33795","inReplyTo":null,"subject":"[RFC/PATCH 0/4] update tracking refs on explicit fetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-11T16:13:21Z","receivedAt":"2013-05-11T16:13:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a cleaned-up version of the oft-discussed[1] concept that \"git\npull origin master\" should update \"refs/remotes/origin/master\".\n\nIt is a little bit of a risky change, in that anybody who deeply cares\nabout when their tracking ref branches are updated would be impacted.\nBut I think the general consensus is that while you can come up with a\nhypothetical use case, the logic is quite tortured, and you could\naccomplish the same thing by keeping a local branch. Most people seem to\nhave the mental model that remote tracking branches are basically a\ncache for what's on the remote, updated opportunistically as\nappropriate.\n\n [1/4]: t5510: start tracking-ref tests from a known state\n [2/4]: fetch/pull doc: untangle meaning of bare \n [3/4]: refactor \"ref->merge\" flag\n [4/4]: fetch: opportunistically update tracking refs\n\nThe final patch is the moral equivalent of the patch I've posted before\nwhen this topic comes up (and referenced in the threads below). But that\npatch did not take care not to write duplicate (mergable!) entries into\nFETCH_HEAD, leading to great confusion from git-pull. This series deals\nwith that, and takes care to write the exact same FETCH_HEAD we would\nhave written before the series.\n\nHowever, Jonathan raised an interesting question: what is the point of\nnot-for-merge entries at all? We are not going to merge them, so none of\nthe pull/merge machinery cares about them. We do not consider extra\nentries in FETCH_HEAD for reachability or anything like that. Is there\nanybody who reads them?\n\nIf we do not care about them, we can simplify this much further, and\neither write the duplicates as not-for-merge, or even omit not-for-merge\nentries entirely. However, it is possible that some third-party scripts\nprocess FETCH_HEAD, so I took the conservative route.\n\nNote that this patch is as greedy as possible[2]; whenever we see the\nLHS is fetched, we update the RHS side, even if the user did:\n\n git fetch origin master:foobar\n\nSome of the discussions below argue that the behavior should not kick in\nfor such a case. I am not sure I agree (and argue against it in those\ndiscussions). I think at this point that is the only potentially\ncontentious item.\n\n-Peff\n\n[1] Past discussions:\n\n http://thread.gmane.org/gmane.comp.version-control.git/127163/focus=127215\n\n http://thread.gmane.org/gmane.comp.version-control.git/192252\n\n http://thread.gmane.org/gmane.comp.version-control.git/203357/focus=203442\n\n http://article.gmane.org/gmane.comp.version-control.git/221234\n"},{"id":"217009","messageId":"20130511161400.GA3270@sigill.intra.peff.net","threadId":"33795","inReplyTo":"20130511161320.GA14990@sigill.intra.peff.net","subject":"[PATCH 1/4] t5510: start tracking-ref tests from a known state","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-11T16:14:03Z","receivedAt":"2013-05-11T16:14:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have three sequential tests for for whether tracking refs\nare updated by various fetches and pulls; the first two\nshould not update the ref, and the third should. Each test\ndepends on the state left by the test before.\n\nThis is fragile (a failing early test will confuse later\ntests), and means we cannot add more \"should update\" tests\nafter the third one.\n\nLet's instead save the initial state before these tests, and\nthen reset to a known state before running each test.\n\nSigned-off-by: Jeff King \n---\n t/t5510-fetch.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex d7a19a1..789c228 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -370,12 +370,20 @@ test_expect_success 'explicit fetch should not update tracking' '\n \n '\n \n+test_expect_success 'mark initial state of origin/master' '\n+\t(\n+\t\tcd three &&\n+\t\tgit tag base-origin-master refs/remotes/origin/master\n+\t)\n+'\n+\n test_expect_success 'explicit fetch should not update tracking' '\n \n \tcd \"$D\" &&\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n+\t\tgit update-ref refs/remotes/origin/master base-origin-master &&\n \t\to=$(git rev-parse --verify refs/remotes/origin/master) &&\n \t\tgit fetch origin master &&\n \t\tn=$(git rev-parse --verify refs/remotes/origin/master) &&\n@@ -390,6 +398,7 @@ test_expect_success 'explicit pull should not update tracking' '\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n+\t\tgit update-ref refs/remotes/origin/master base-origin-master &&\n \t\to=$(git rev-parse --verify refs/remotes/origin/master) &&\n \t\tgit pull origin master &&\n \t\tn=$(git rev-parse --verify refs/remotes/origin/master) &&\n@@ -404,6 +413,7 @@ test_expect_success 'configured fetch updates tracking' '\n \tgit branch -f side &&\n \t(\n \t\tcd three &&\n+\t\tgit update-ref refs/remotes/origin/master base-origin-master &&\n \t\to=$(git rev-parse --verify refs/remotes/origin/master) &&\n \t\tgit fetch origin &&\n \t\tn=$(git rev-parse --verify refs/remotes/origin/master) &&\n-- \n1.8.3.rc1.2.g12db477\n"},{"id":"217010","messageId":"20130511161425.GB3270@sigill.intra.peff.net","threadId":"33795","inReplyTo":"20130511161320.GA14990@sigill.intra.peff.net","subject":"[PATCH 2/4] fetch/pull doc: untangle meaning of bare ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-11T16:14:25Z","receivedAt":"2013-05-11T16:14:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Thomas Rast \n\nThe documentation erroneously used the same wording for both fetch and\npull, stating that something will be merged even in git-fetch(1).\n\nIn addition, saying that \" is equivalent to :\" doesn't\nreally help anyone who still needs to read manpages. Clarify what is\nactually going on.\n\nSigned-off-by: Thomas Rast \nSigned-off-by: Jeff King \n---\n Documentation/pull-fetch-param.txt | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt\nindex 94a9d32..6f5ca21 100644\n--- a/Documentation/pull-fetch-param.txt\n+++ b/Documentation/pull-fetch-param.txt\n@@ -68,6 +68,11 @@ Some short-cut notations are also supported.\n +\n * `tag ` means the same as `refs/tags/:refs/tags/`;\n it requests fetching everything up to the given tag.\n-* A parameter without a colon is equivalent to\n- : when pulling/fetching, so it merges into the current\n- branch without storing the remote branch anywhere locally\n+ifndef::git-pull[]\n+* A parameter without a colon fetches that ref into FETCH_HEAD,\n+endif::git-pull[]\n+ifdef::git-pull[]\n+* A parameter without a colon merges into the current\n+ branch,\n+endif::git-pull[]\n+ while not storing the branch anywhere locally.\n-- \n1.8.3.rc1.2.g12db477\n"},{"id":"217011","messageId":"20130511161558.GC3270@sigill.intra.peff.net","threadId":"33795","inReplyTo":"20130511161320.GA14990@sigill.intra.peff.net","subject":"[PATCH 3/4] refactor \"ref->merge\" flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-11T16:15:59Z","receivedAt":"2013-05-11T16:15:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Each \"struct ref\" has a boolean flag that is set by the\nfetch code to determine whether the ref should be marked as\n\"not-for-merge\" or not when we write it out to FETCH_HEAD.\n\nIt would be useful to turn this boolean into a tri-state,\nwith the third state meaning \"do not bother writing it out\nto FETCH_HEAD at all\". That would let us add extra refs to\nthe set of refs to be stored (e.g., to store copies of\nthings we fetched) without impacting FETCH_HEAD.\n\nThis patch turns it into an enum that covers the tri-state\ncase, and hopefully makes the code more explicit and easier\nto read.\n\nSigned-off-by: Jeff King \n---\n builtin/fetch.c | 57 +++++++++++++++++++++++++++++++++++----------------------\n cache.h | 14 +++++++++++++-\n 2 files changed, 48 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 4b6b1df..287cf4c 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -119,7 +119,7 @@ static void add_merge_config(struct ref **head,\n \n \t\tfor (rm = *head; rm; rm = rm->next) {\n \t\t\tif (branch_merge_matches(branch, i, rm->name)) {\n-\t\t\t\trm->merge = 1;\n+\t\t\t\trm->fetch_head_status = FETCH_HEAD_MERGE;\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n@@ -140,7 +140,7 @@ static void add_merge_config(struct ref **head,\n \t\trefspec.src = branch->merge[i]->src;\n \t\tget_fetch_map(remote_refs, &refspec, tail, 1);\n \t\tfor (rm = *old_tail; rm; rm = rm->next)\n-\t\t\trm->merge = 1;\n+\t\t\trm->fetch_head_status = FETCH_HEAD_MERGE;\n \t}\n }\n \n@@ -167,7 +167,7 @@ static struct ref *get_ref_map(struct transport *transport,\n \t\t}\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->merge = 1;\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 \t} else {\n@@ -186,7 +186,7 @@ static struct ref *get_ref_map(struct transport *transport,\n \t\t\t\t\t*autotags = 1;\n \t\t\t\tif (!i && !has_merge && ref_map &&\n \t\t\t\t !remote->fetch[0].pattern)\n-\t\t\t\t\tref_map->merge = 1;\n+\t\t\t\t\tref_map->fetch_head_status = FETCH_HEAD_MERGE;\n \t\t\t}\n \t\t\t/*\n \t\t\t * if the remote we're fetching from is the same\n@@ -202,7 +202,7 @@ static struct ref *get_ref_map(struct transport *transport,\n \t\t\tref_map = get_remote_ref(remote_refs, \"HEAD\");\n \t\t\tif (!ref_map)\n \t\t\t\tdie(_(\"Couldn't find remote ref HEAD\"));\n-\t\t\tref_map->merge = 1;\n+\t\t\tref_map->fetch_head_status = FETCH_HEAD_MERGE;\n \t\t\ttail = &ref_map->next;\n \t\t}\n \t}\n@@ -389,7 +389,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url, *filename = dry_run ? \"/dev/null\" : git_path(\"FETCH_HEAD\");\n-\tint want_merge;\n+\tint want_status;\n \n \tfp = fopen(filename, \"a\");\n \tif (!fp)\n@@ -407,19 +407,22 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t}\n \n \t/*\n-\t * The first pass writes objects to be merged and then the\n-\t * second pass writes the rest, in order to allow using\n-\t * FETCH_HEAD as a refname to refer to the ref to be merged.\n+\t * We do a pass for each fetch_head_status type in their enum order, so\n+\t * merged entries are written before not-for-merge. That lets readers\n+\t * use FETCH_HEAD as a refname to refer to the ref to be merged.\n \t */\n-\tfor (want_merge = 1; 0 <= want_merge; want_merge--) {\n+\tfor (want_status = FETCH_HEAD_MERGE;\n+\t want_status <= FETCH_HEAD_IGNORE;\n+\t want_status++) {\n \t\tfor (rm = ref_map; rm; rm = rm->next) {\n \t\t\tstruct ref *ref = NULL;\n+\t\t\tconst char *merge_status_marker = \"\";\n \n \t\t\tcommit = lookup_commit_reference_gently(rm->old_sha1, 1);\n \t\t\tif (!commit)\n-\t\t\t\trm->merge = 0;\n+\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n \n-\t\t\tif (rm->merge != want_merge)\n+\t\t\tif (rm->fetch_head_status != want_status)\n \t\t\t\tcontinue;\n \n \t\t\tif (rm->peer_ref) {\n@@ -465,16 +468,26 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\t\tstrbuf_addf(¬e, \"%s \", kind);\n \t\t\t\tstrbuf_addf(¬e, \"'%s' of \", what);\n \t\t\t}\n-\t\t\tfprintf(fp, \"%s\\t%s\\t%s\",\n-\t\t\t\tsha1_to_hex(rm->old_sha1),\n-\t\t\t\trm->merge ? \"\" : \"not-for-merge\",\n-\t\t\t\tnote.buf);\n-\t\t\tfor (i = 0; i < url_len; ++i)\n-\t\t\t\tif ('\\n' == url[i])\n-\t\t\t\t\tfputs(\"\\\\n\", fp);\n-\t\t\t\telse\n-\t\t\t\t\tfputc(url[i], fp);\n-\t\t\tfputc('\\n', fp);\n+\t\t\tswitch (rm->fetch_head_status) {\n+\t\t\tcase FETCH_HEAD_NOT_FOR_MERGE:\n+\t\t\t\tmerge_status_marker = \"not-for-merge\";\n+\t\t\t\t/* fall-through */\n+\t\t\tcase FETCH_HEAD_MERGE:\n+\t\t\t\tfprintf(fp, \"%s\\t%s\\t%s\",\n+\t\t\t\t\tsha1_to_hex(rm->old_sha1),\n+\t\t\t\t\tmerge_status_marker,\n+\t\t\t\t\tnote.buf);\n+\t\t\t\tfor (i = 0; i < url_len; ++i)\n+\t\t\t\t\tif ('\\n' == url[i])\n+\t\t\t\t\t\tfputs(\"\\\\n\", fp);\n+\t\t\t\t\telse\n+\t\t\t\t\t\tfputc(url[i], fp);\n+\t\t\t\tfputc('\\n', fp);\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\t/* do not write anything to FETCH_HEAD */\n+\t\t\t\tbreak;\n+\t\t\t}\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\ndiff --git a/cache.h b/cache.h\nindex 94ca1ac..9670d99 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1024,9 +1024,21 @@ struct ref {\n \tunsigned int\n \t\tforce:1,\n \t\tforced_update:1,\n-\t\tmerge:1,\n \t\tdeletion:1,\n \t\tmatched:1;\n+\n+\t/*\n+\t * Order is important here, as we write to FETCH_HEAD\n+\t * in numeric order. And the default NOT_FOR_MERGE\n+\t * should be 0, so that xcalloc'd structures get it\n+\t * by default.\n+\t */\n+\tenum {\n+\t\tFETCH_HEAD_MERGE = -1,\n+\t\tFETCH_HEAD_NOT_FOR_MERGE = 0,\n+\t\tFETCH_HEAD_IGNORE = 1\n+\t} fetch_head_status;\n+\n \tenum {\n \t\tREF_STATUS_NONE = 0,\n \t\tREF_STATUS_OK,\n-- \n1.8.3.rc1.2.g12db477\n"},{"id":"217012","messageId":"20130511161652.GD3270@sigill.intra.peff.net","threadId":"33795","inReplyTo":"20130511161320.GA14990@sigill.intra.peff.net","subject":"[PATCH 4/4] fetch: opportunistically update tracking refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-11T16:16:52Z","receivedAt":"2013-05-11T16:16:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we run a regular \"git fetch\" without arguments, we\nupdate the tracking refs according to the configured\nrefspec. However, when we run \"git fetch origin master\" (or\n\"git pull origin master\"), we do not look at the configured\nrefspecs at all, and just update FETCH_HEAD.\n\nWe miss an opportunity to update \"refs/remotes/origin/master\"\n(or whatever the user has configured). Some users find this\nconfusing, because they would want to do further comparisons\nagainst the old state of the remote master, like:\n\n $ git pull origin master\n $ git log HEAD...origin/master\n\nIn the currnet code, they are comparing against whatever\ncommit happened to be in origin/master from the last time\nthey did a complete \"git fetch\". This patch will update a\nref from the RHS of a configured refspec whenever we happen\nto be fetching its LHS. That makes the case above work.\n\nThe downside is that any users who really care about whether\nand when their tracking branches are updated may be\nsurprised.\n\nSigned-off-by: Jeff King \n---\n Documentation/pull-fetch-param.txt | 2 +-\n builtin/fetch.c | 16 ++++++++++++++++\n t/t5510-fetch.sh | 8 ++++----\n 3 files changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt\nindex 6f5ca21..18cffc2 100644\n--- a/Documentation/pull-fetch-param.txt\n+++ b/Documentation/pull-fetch-param.txt\n@@ -75,4 +75,4 @@ endif::git-pull[]\n * A parameter without a colon merges into the current\n branch,\n endif::git-pull[]\n- while not storing the branch anywhere locally.\n+ and updates the remote-tracking branches (if any).\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 287cf4c..e41cc0d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -160,6 +160,8 @@ static struct ref *get_ref_map(struct transport *transport,\n \tconst struct ref *remote_refs = transport_get_remote_refs(transport);\n \n \tif (ref_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@@ -170,6 +172,20 @@ static struct ref *get_ref_map(struct transport *transport,\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+\t\t * arguments, take the opportunity to update their configured\n+\t\t * counterparts. However, we do not want to mention these\n+\t\t * entries in FETCH_HEAD at all, as they would simply be\n+\t\t * duplicates of existing entries.\n+\t\t */\n+\t\told_tail = tail;\n+\t\tfor (i = 0; i < transport->remote->fetch_refspec_nr; i++)\n+\t\t\tget_fetch_map(ref_map, &transport->remote->fetch[i],\n+\t\t\t\t &tail, 0);\n+\t\tfor (rm = *old_tail; rm; rm = rm->next)\n+\t\t\trm->fetch_head_status = FETCH_HEAD_IGNORE;\n \t} else {\n \t\t/* Use the defaults */\n \t\tstruct remote *remote = transport->remote;\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 789c228..ff43e08 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -377,7 +377,7 @@ test_expect_success 'mark initial state of origin/master' '\n \t)\n '\n \n-test_expect_success 'explicit fetch should not update tracking' '\n+test_expect_success 'explicit fetch should update tracking' '\n \n \tcd \"$D\" &&\n \tgit branch -f side &&\n@@ -387,12 +387,12 @@ test_expect_success 'explicit fetch should not update tracking' '\n \t\to=$(git rev-parse --verify refs/remotes/origin/master) &&\n \t\tgit fetch origin master &&\n \t\tn=$(git rev-parse --verify refs/remotes/origin/master) &&\n-\t\ttest \"$o\" = \"$n\" &&\n+\t\ttest \"$o\" != \"$n\" &&\n \t\ttest_must_fail git rev-parse --verify refs/remotes/origin/side\n \t)\n '\n \n-test_expect_success 'explicit pull should not update tracking' '\n+test_expect_success 'explicit pull should update tracking' '\n \n \tcd \"$D\" &&\n \tgit branch -f side &&\n@@ -402,7 +402,7 @@ test_expect_success 'explicit pull should not update tracking' '\n \t\to=$(git rev-parse --verify refs/remotes/origin/master) &&\n \t\tgit pull origin master &&\n \t\tn=$(git rev-parse --verify refs/remotes/origin/master) &&\n-\t\ttest \"$o\" = \"$n\" &&\n+\t\ttest \"$o\" != \"$n\" &&\n \t\ttest_must_fail git rev-parse --verify refs/remotes/origin/side\n \t)\n '\n-- \n1.8.3.rc1.2.g12db477\n"},{"id":"217062","messageId":"871u9cftjv.fsf@linux-k42r.v.cablecom.net","threadId":"33795","inReplyTo":"20130511161652.GD3270@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] fetch: opportunistically update tracking refs","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-05-12T09:15:16Z","receivedAt":"2013-05-12T09:15:16Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King writes:\n\n> When we run a regular \"git fetch\" without arguments, we\n> update the tracking refs according to the configured\n> refspec. However, when we run \"git fetch origin master\" (or\n> \"git pull origin master\"), we do not look at the configured\n> refspecs at all, and just update FETCH_HEAD.\n>\n> We miss an opportunity to update \"refs/remotes/origin/master\"\n> (or whatever the user has configured). Some users find this\n> confusing, because they would want to do further comparisons\n> against the old state of the remote master, like:\n>\n> $ git pull origin master\n> $ git log HEAD...origin/master\n\nI agree with the patch, but I would use a different reasoning. Your\nexample here is not even correct because the range in the second command\nwould be empty unless the merge conflicted.\n\nI believe the more common confusion stems from the tracking displays in\ngit-checkout and others. The issue there is that we *internally* treat\norigin/masters as a kind of mirror, and the tracking displays all go\nagainst user expectations once they have seen a newer state of origin's\nmaster by explicit pull or fetch.\n\nMichael also pointed out yesterday that short of\n\n git fetch origin master:refs/remotes/origin/master\n\nthere is no way to fetch only one branch.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"217063","messageId":"87r4hcedra.fsf@linux-k42r.v.cablecom.net","threadId":"33795","inReplyTo":"871u9cftjv.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 4/4] fetch: opportunistically update tracking refs","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-05-12T09:41:45Z","receivedAt":"2013-05-12T09:41:45Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast writes:\n\n> Jeff King writes:\n>\n>> When we run a regular \"git fetch\" without arguments, we\n>> update the tracking refs according to the configured\n>> refspec. However, when we run \"git fetch origin master\" (or\n>> \"git pull origin master\"), we do not look at the configured\n>> refspecs at all, and just update FETCH_HEAD.\n>>\n>> We miss an opportunity to update \"refs/remotes/origin/master\"\n>> (or whatever the user has configured). Some users find this\n>> confusing, because they would want to do further comparisons\n>> against the old state of the remote master, like:\n>>\n>> $ git pull origin master\n>> $ git log HEAD...origin/master\n>\n> I agree with the patch, but I would use a different reasoning. Your\n> example here is not even correct because the range in the second command\n> would be empty unless the merge conflicted.\n\nMeh, I just read this again and saw that you actually had *three* dots.\nServes me right for writing a reply on the phone.\n\nSo the quoted part is indeed correct.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"217387","messageId":"CAPig+cQk2GCL4z=xo1m86uvKzE2QrP_1ajzkJvDTadxV_dkDuw@mail.gmail.com","threadId":"33795","inReplyTo":"20130511161400.GA3270@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] t5510: start tracking-ref tests from a known state","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-14T22:18:07Z","receivedAt":"2013-05-14T22:18:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, May 11, 2013 at 12:14 PM, Jeff King wrote:\n> We have three sequential tests for for whether tracking refs\n\ns/for for/for/\n[or]\ns/for for/for checking/\n\n> are updated by various fetches and pulls; the first two\n> should not update the ref, and the third should. Each test\n> depends on the state left by the test before.\n>\n> This is fragile (a failing early test will confuse later\n> tests), and means we cannot add more \"should update\" tests\n> after the third one.\n>\n> Let's instead save the initial state before these tests, and\n> then reset to a known state before running each test.\n>\n> Signed-off-by: Jeff King \n"},{"id":"217498","messageId":"20130516033748.GC13296@sigill.intra.peff.net","threadId":"33795","inReplyTo":"87r4hcedra.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 4/4] fetch: opportunistically update tracking refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-16T03:37:48Z","receivedAt":"2013-05-16T03:37:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 12, 2013 at 11:41:45AM +0200, Thomas Rast wrote:\n\n> >> We miss an opportunity to update \"refs/remotes/origin/master\"\n> >> (or whatever the user has configured). Some users find this\n> >> confusing, because they would want to do further comparisons\n> >> against the old state of the remote master, like:\n> >>\n> >> $ git pull origin master\n> >> $ git log HEAD...origin/master\n> >\n> > I agree with the patch, but I would use a different reasoning. Your\n> > example here is not even correct because the range in the second command\n> > would be empty unless the merge conflicted.\n> \n> Meh, I just read this again and saw that you actually had *three* dots.\n> Serves me right for writing a reply on the phone.\n> \n> So the quoted part is indeed correct.\n\nYes, it's correct, but it is a bit subtle. A better example would\nprobably be:\n\n $ git status\n # On branch foo\n # Your branch is ahead of 'origin/master' by 2 commits.\n\n $ git pull origin master\n [pull 10 new commits]\n\n $ git status\n # On branch foo\n # Your branch is ahead of 'origin/master' by 13 commits.\n\nThat is technically true, but only because origin/master is now out of\ndate. The more interesting number is 3: the two commits we had already,\nplus the new merge commit.\n\nAnd of course there are infinite variations where you pull on one\nbranch, and then switch branches to compare on another. But they all\nboil down to having an out-of-date view of origin/master that does not\nreflect the most recent pull.\n\n-Peff\n"},{"id":"224684","messageId":"7vsiymhjgz.fsf@alter.siamese.dyndns.org","threadId":"33795","inReplyTo":"20130511161652.GD3270@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] fetch: opportunistically update tracking refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-06T16:28:28Z","receivedAt":"2013-08-06T16:28:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> @@ -170,6 +172,20 @@ static struct ref *get_ref_map(struct transport *transport,\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> +\t\t * arguments, take the opportunity to update their configured\n> +\t\t * counterparts. However, we do not want to mention these\n> +\t\t * entries in FETCH_HEAD at all, as they would simply be\n> +\t\t * duplicates of existing entries.\n> +\t\t */\n> +\t\told_tail = tail;\n> +\t\tfor (i = 0; i < transport->remote->fetch_refspec_nr; i++)\n> +\t\t\tget_fetch_map(ref_map, &transport->remote->fetch[i],\n> +\t\t\t\t &tail, 0);\n> +\t\tfor (rm = *old_tail; rm; rm = rm->next)\n> +\t\t\trm->fetch_head_status = FETCH_HEAD_IGNORE;\n\nWas there a reason why this change was done by appending new ref at\nthe tail of the ref_map list? I would have expected that a naïve\nand obvious implementation would be to iterate existing refs over\nref_map to find refs with an empty RHS whose LHS is configured to\nusually store the fetched result to somewhere and to update their\nRHS in place.\n\nBeing curious.\n"},{"id":"224714","messageId":"20130806214622.GA31766@sigill.intra.peff.net","threadId":"33795","inReplyTo":"7vsiymhjgz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] fetch: opportunistically update tracking refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-06T21:46:22Z","receivedAt":"2013-08-06T21:46:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 06, 2013 at 09:28:28AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > @@ -170,6 +172,20 @@ static struct ref *get_ref_map(struct transport *transport,\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> > +\t\t * arguments, take the opportunity to update their configured\n> > +\t\t * counterparts. However, we do not want to mention these\n> > +\t\t * entries in FETCH_HEAD at all, as they would simply be\n> > +\t\t * duplicates of existing entries.\n> > +\t\t */\n> > +\t\told_tail = tail;\n> > +\t\tfor (i = 0; i < transport->remote->fetch_refspec_nr; i++)\n> > +\t\t\tget_fetch_map(ref_map, &transport->remote->fetch[i],\n> > +\t\t\t\t &tail, 0);\n> > +\t\tfor (rm = *old_tail; rm; rm = rm->next)\n> > +\t\t\trm->fetch_head_status = FETCH_HEAD_IGNORE;\n> \n> Was there a reason why this change was done by appending new ref at\n> the tail of the ref_map list? I would have expected that a naïve\n> and obvious implementation would be to iterate existing refs over\n> ref_map to find refs with an empty RHS whose LHS is configured to\n> usually store the fetched result to somewhere and to update their\n> RHS in place.\n> \n> Being curious.\n\nTwo reasons:\n\n 1. The implementation you suggest above behaves differently than the\n current code. It does not look for refspecs with an empty RHS. It\n looks for any LHS that matches our configured entries. So if you do\n \"git fetch origin master:foobar\", we will update both\n \"refs/heads/foobar\" as well as \"refs/remotes/origin/master\".\n\n That means it is purely an opportunistic \"hey, during another\n operation we happened to find out something new about\n origin/master, so let's update our tracking branch\". Whereas what\n you stated above is more like \"we are fetching into FETCH_HEAD, so\n let's also update the tracking branch\".\n\n 2. The list of refs after get_ref_map is actually more of an\n instruction/command list for the rest of the code to follow. It\n gets fed to store_updated_ref, but also impacts the status table we\n show. You could implement it such that a single ref entry had\n multiple storage destinations, but that would require changes to\n all of the consumers of the instruction list. Since we already need\n to handle extra refspecs (e.g., you can say \"master:foobar\n master:refs/remotes/origin/master\" on the command line already), we\n can treat these the same way. We append to the instruction list,\n and the rest of the code treats it as if you specified it on the\n command line.\n\n-Peff\n"},{"id":"224716","messageId":"7vob9aeae4.fsf@alter.siamese.dyndns.org","threadId":"33795","inReplyTo":"20130806214622.GA31766@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] fetch: opportunistically update tracking refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-06T22:12:51Z","receivedAt":"2013-08-06T22:12:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Two reasons:\n\nOK, both boils down to \"Junio did not consider 'master:foobar'\ncase\".\n\nThanks; it makes sense now.\n"}]}