{"thread":{"id":"33948","subject":"[PATCH] fetch: don't try to update unfetched tracking refs","startedAt":"2013-05-27T11:40:25Z","lastAt":"2013-05-27T16:35:00Z","messageCount":6,"participants":["John Keeping","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"218600","messageId":"9c6fd36368cd6b0fba4855be4d31106781c02f58.1369654825.git.john@keeping.me.uk","threadId":"33948","inReplyTo":null,"subject":"[PATCH] fetch: don't try to update unfetched tracking refs","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-27T11:40:25Z","receivedAt":"2013-05-27T11:40:25Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Since commit f269048 (fetch: opportunistically update tracking refs,\n2013-05-11) we update tracking refs opportunistically when fetching\nremote branches.  However, if a refspec is given on the command line\nthat does not include a configured (non-pattern) refspec a fatal error\noccurs.\n\nFix this by setting the \"missing_ok\" flag when calling get_fetch_map.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n builtin/fetch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e41cc0d..d15a734 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -183,7 +183,7 @@ static struct ref *get_ref_map(struct transport *transport,\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\t\t\t      &tail, 1);\n \t\tfor (rm = *old_tail; rm; rm = rm->next)\n \t\t\trm->fetch_head_status = FETCH_HEAD_IGNORE;\n \t} else {\n-- \n1.8.3.rc3.438.gb3e4ae3\n"},{"id":"218613","messageId":"20130527154252.GB8250@sigill.intra.peff.net","threadId":"33948","inReplyTo":"9c6fd36368cd6b0fba4855be4d31106781c02f58.1369654825.git.john@keeping.me.uk","subject":"Re: [PATCH] fetch: don't try to update unfetched tracking refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-27T15:42:52Z","receivedAt":"2013-05-27T15:42:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 27, 2013 at 12:40:25PM +0100, John Keeping wrote:\n\n> Since commit f269048 (fetch: opportunistically update tracking refs,\n> 2013-05-11) we update tracking refs opportunistically when fetching\n> remote branches.  However, if a refspec is given on the command line\n> that does not include a configured (non-pattern) refspec a fatal error\n> occurs.\n\nI'm not sure I understand what the last sentence means. I tried to add a\ntest like:\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex ff43e08..02e30e1 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -422,6 +422,16 @@ test_expect_success 'configured fetch updates tracking' '\n \t)\n '\n \n+test_expect_success 'non-configured ref does not confuse tracking update' '\n+\tcd \"$D\" &&\n+\tgit update-ref refs/odd/location HEAD &&\n+\t(\n+\t\tcd three &&\n+\t\tgit fetch origin refs/odd/location &&\n+\t\tgit rev-parse --verify FETCH_HEAD\n+\t)\n+'\n+\n test_expect_success 'pushing nonexistent branch by mistake should not segv' '\n \n \tcd \"$D\" &&\n\nbut it does not fail with the existing code. Can you give an example\nthat fails?\n\n-Peff\n"},{"id":"218615","messageId":"20130527160129.GN27005@serenity.lan","threadId":"33948","inReplyTo":"20130527154252.GB8250@sigill.intra.peff.net","subject":"Re: [PATCH] fetch: don't try to update unfetched tracking refs","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-27T16:01:29Z","receivedAt":"2013-05-27T16:01:29Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, May 27, 2013 at 11:42:52AM -0400, Jeff King wrote:\n> On Mon, May 27, 2013 at 12:40:25PM +0100, John Keeping wrote:\n> \n> > Since commit f269048 (fetch: opportunistically update tracking refs,\n> > 2013-05-11) we update tracking refs opportunistically when fetching\n> > remote branches.  However, if a refspec is given on the command line\n> > that does not include a configured (non-pattern) refspec a fatal error\n> > occurs.\n> \n> I'm not sure I understand what the last sentence means. I tried to add a\n> test like:\n> \n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index ff43e08..02e30e1 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -422,6 +422,16 @@ test_expect_success 'configured fetch updates tracking' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'non-configured ref does not confuse tracking update' '\n> +\tcd \"$D\" &&\n> +\tgit update-ref refs/odd/location HEAD &&\n> +\t(\n> +\t\tcd three &&\n> +\t\tgit fetch origin refs/odd/location &&\n> +\t\tgit rev-parse --verify FETCH_HEAD\n> +\t)\n> +'\n> +\n>  test_expect_success 'pushing nonexistent branch by mistake should not segv' '\n>  \n>  \tcd \"$D\" &&\n> \n> but it does not fail with the existing code. Can you give an example\n> that fails?\n\nI have this in my .git/config for git.git:\n\n    [remote \"origin\"]\n            url = git://github.com/gitster/git\n            fetch = +refs/heads/*:refs/remotes/origin/*\n            fetch = +refs/notes/amlog:refs/notes/amlog\n\nThen doing \"git fetch origin master\" fails because:\n\n    fatal: Couldn't find remote ref refs/notes/amlog\n\nThe following test fails for me (and passes with my patch) - note that\nin \"two\", remote.one.fetch is configured as\n\"refs/heads/master:refs/heads/one\".\n\n-- >8 --\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex ff43e08..c540257 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -422,6 +422,19 @@ test_expect_success 'configured fetch updates tracking' '\n \t)\n '\n \n+test_expect_success 'configured ref does not confuse tracking' '\n+\n+\tcd \"$D\" &&\n+\t(\n+\t\tcd one &&\n+\t\tgit branch -f side\n+\t) &&\n+\t(\n+\t\tcd two &&\n+\t\tgit fetch one side\n+\t)\n+'\n+\n test_expect_success 'pushing nonexistent branch by mistake should not segv' '\n \n \tcd \"$D\" &&\n"},{"id":"218617","messageId":"20130527161934.GB9235@sigill.intra.peff.net","threadId":"33948","inReplyTo":"20130527160129.GN27005@serenity.lan","subject":"Re: [PATCH] fetch: don't try to update unfetched tracking refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-27T16:19:34Z","receivedAt":"2013-05-27T16:19:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 27, 2013 at 05:01:29PM +0100, John Keeping wrote:\n\n> > I'm not sure I understand what the last sentence means. I tried to add a\n> > test like:\n> [...]\n> > but it does not fail with the existing code. Can you give an example\n> > that fails?\n> \n> I have this in my .git/config for git.git:\n> \n>     [remote \"origin\"]\n>             url = git://github.com/gitster/git\n>             fetch = +refs/heads/*:refs/remotes/origin/*\n>             fetch = +refs/notes/amlog:refs/notes/amlog\n\nAh, I see. It is not \"the refspec on the command-line does not match a\nconfigured refspec\", but rather \"there exists a configured non-pattern\nrefspec that does not match what was on the command-line (even if what\nwas on the command-line did match another refspec)\".\n\nSo your fix makes perfect sense. Do you mind squashing in this test\nbelow? I think it is a little less subtle than what you posted, as it\nsets up the situation explicitly in the test. It also checks that the\nrefs we _did_ match still get updated (\"master\" in this case).\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex ff43e08..fde6891 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -422,6 +422,22 @@ test_expect_success 'configured fetch updates tracking' '\n \t)\n '\n \n+test_expect_success 'non-matching refspecs do not confuse tracking update' '\n+\tcd \"$D\" &&\n+\tgit update-ref refs/odd/location HEAD &&\n+\t(\n+\t\tcd three &&\n+\t\tgit update-ref refs/remotes/origin/master base-origin-master &&\n+\t\tgit config --add remote.origin.fetch \\\n+\t\t\trefs/odd/location:refs/remotes/origin/odd &&\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_must_fail git rev-parse --verify refs/remotes/origin/odd\n+\t)\n+'\n+\n test_expect_success 'pushing nonexistent branch by mistake should not segv' '\n \n \tcd \"$D\" &&\n\nThanks for the fix.\n\nAcked-by: Jeff King <peff@peff.net>\n\n-Peff\n"},{"id":"218619","messageId":"20130527163308.GO27005@serenity.lan","threadId":"33948","inReplyTo":"20130527161934.GB9235@sigill.intra.peff.net","subject":"[PATCH v2] fetch: don't try to update unfetched tracking refs","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-27T16:33:09Z","receivedAt":"2013-05-27T16:33:09Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Since commit f269048 (fetch: opportunistically update tracking refs,\n2013-05-11) we update tracking refs opportunistically when fetching\nremote branches.  However, if there is a configured non-pattern refspec\nthat does not match any of the refspecs given on the command line then a\nfatal error occurs.\n\nFix this by setting the \"missing_ok\" flag when calling get_fetch_map.\n\nTest-added-by: Jeff King <peff@peff.net>\nSigned-off-by: John Keeping <john@keeping.me.uk>\nAcked-by: Jeff King <peff@peff.net>\n---\nOn Mon, May 27, 2013 at 12:19:34PM -0400, Jeff King wrote:\n> On Mon, May 27, 2013 at 05:01:29PM +0100, John Keeping wrote:\n> \n> > > I'm not sure I understand what the last sentence means. I tried to add a\n> > > test like:\n> > [...]\n> > > but it does not fail with the existing code. Can you give an example\n> > > that fails?\n> > \n> > I have this in my .git/config for git.git:\n> > \n> >     [remote \"origin\"]\n> >             url = git://github.com/gitster/git\n> >             fetch = +refs/heads/*:refs/remotes/origin/*\n> >             fetch = +refs/notes/amlog:refs/notes/amlog\n> \n> Ah, I see. It is not \"the refspec on the command-line does not match a\n> configured refspec\", but rather \"there exists a configured non-pattern\n> refspec that does not match what was on the command-line (even if what\n> was on the command-line did match another refspec)\".\n\nExactly.  I've changed the commit message to (hopefully) make this\nclearer.\n\n> So your fix makes perfect sense. Do you mind squashing in this test\n> below? I think it is a little less subtle than what you posted, as it\n> sets up the situation explicitly in the test. It also checks that the\n> refs we _did_ match still get updated (\"master\" in this case).\n\nDone.\n\n builtin/fetch.c  |  2 +-\n t/t5510-fetch.sh | 16 ++++++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e41cc0d..d15a734 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -183,7 +183,7 @@ static struct ref *get_ref_map(struct transport *transport,\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\t\t\t      &tail, 1);\n \t\tfor (rm = *old_tail; rm; rm = rm->next)\n \t\t\trm->fetch_head_status = FETCH_HEAD_IGNORE;\n \t} else {\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex ff43e08..fde6891 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -422,6 +422,22 @@ test_expect_success 'configured fetch updates tracking' '\n \t)\n '\n \n+test_expect_success 'non-matching refspecs do not confuse tracking update' '\n+\tcd \"$D\" &&\n+\tgit update-ref refs/odd/location HEAD &&\n+\t(\n+\t\tcd three &&\n+\t\tgit update-ref refs/remotes/origin/master base-origin-master &&\n+\t\tgit config --add remote.origin.fetch \\\n+\t\t\trefs/odd/location:refs/remotes/origin/odd &&\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_must_fail git rev-parse --verify refs/remotes/origin/odd\n+\t)\n+'\n+\n test_expect_success 'pushing nonexistent branch by mistake should not segv' '\n \n \tcd \"$D\" &&\n-- \n1.8.3.rc3.438.gb3e4ae3\n"},{"id":"218620","messageId":"20130527163500.GA18405@sigill.intra.peff.net","threadId":"33948","inReplyTo":"20130527163308.GO27005@serenity.lan","subject":"Re: [PATCH v2] fetch: don't try to update unfetched tracking refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-27T16:35:00Z","receivedAt":"2013-05-27T16:35:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 27, 2013 at 05:33:09PM +0100, John Keeping wrote:\n\n> > Ah, I see. It is not \"the refspec on the command-line does not match a\n> > configured refspec\", but rather \"there exists a configured non-pattern\n> > refspec that does not match what was on the command-line (even if what\n> > was on the command-line did match another refspec)\".\n> \n> Exactly.  I've changed the commit message to (hopefully) make this\n> clearer.\n\nIt does. To me, anyway. :)\n\nThanks again.\n\n-Peff\n"}]}