{"thread":{"id":"28280","subject":"Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","startedAt":"2011-09-01T18:25:31Z","lastAt":"2011-09-07T22:42:45Z","messageCount":27,"participants":["Junio C Hamano","Shawn Pearce","Michael J Gruber","Matthieu Moy","Jeff King","Johannes Sixt","Thomas Rast"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"174695","messageId":"7vliu8w25g.fsf@alter.siamese.dyndns.org","threadId":"28280","inReplyTo":null,"subject":"Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-01T18:25:31Z","receivedAt":"2011-09-01T18:25:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Suggested reading:\n\n http://git-blame.blogspot.com/2011/08/how-to-inject-malicious-commit-to-git.html\n\nI am wondering if we are better off applying something along the lines of\nthis patch, so that with the default configuration, users can notice if\ntheir upstream unexpectedly rewound their branches.\n\nIt would produce\n\n\t[remote]\n \turl = git://.../git.git/\n fetch = refs/heads/*:refs/remotes/origin/*\n\nupon cloning from my repository, and your \"git fetch\" will fail because\nthe pu (proposed updates) branch is constantly unwinding, but that can be\neasily fixed with\n\n\n\t[remote]\n \turl = git://.../git.git/\n fetch = refs/heads/*:refs/remotes/origin/*\n fetch = +refs/heads/pu:refs/remotes/origin/pu\n\nas the explicit refspec trumps the wildcard one.\n\n builtin/remote.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex f2a9c26..081fbbf 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -116,11 +116,11 @@ static int add_branch(const char *key, const char *branchname,\n \t\tconst char *remotename, int mirror, struct strbuf *tmp)\n {\n \tstrbuf_reset(tmp);\n-\tstrbuf_addch(tmp, '+');\n-\tif (mirror)\n+\tif (mirror) {\n+\t\tstrbuf_addch(tmp, '+');\n \t\tstrbuf_addf(tmp, \"refs/%s:refs/%s\",\n \t\t\t\tbranchname, branchname);\n-\telse\n+\t} else\n \t\tstrbuf_addf(tmp, \"refs/heads/%s:refs/remotes/%s/%s\",\n \t\t\t\tbranchname, remotename, branchname);\n \treturn git_config_set_multivar(key, tmp->buf, \"^$\", 0);\n"},{"id":"174698","messageId":"7vhb4ww1id.fsf@alter.siamese.dyndns.org","threadId":"28280","inReplyTo":"7vliu8w25g.fsf@alter.siamese.dyndns.org","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-01T18:39:22Z","receivedAt":"2011-09-01T18:39:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Suggested reading:\n>\n> http://git-blame.blogspot.com/2011/08/how-to-inject-malicious-commit-to-git.html\n>\n> I am wondering if we are better off applying something along the lines of\n> this patch, so that with the default configuration, users can notice if\n> their upstream unexpectedly rewound their branches.\n>\n> It would produce\n>\n> \t[remote]\n> \turl = git://.../git.git/\n> fetch = refs/heads/*:refs/remotes/origin/*\n>\n> upon cloning from my repository, and your \"git fetch\" will fail because\n> the pu (proposed updates) branch is constantly unwinding, but that can be\n> easily fixed with\n>\n>\n> \t[remote]\n> \turl = git://.../git.git/\n> fetch = refs/heads/*:refs/remotes/origin/*\n> fetch = +refs/heads/pu:refs/remotes/origin/pu\n>\n> as the explicit refspec trumps the wildcard one.\n\nIt appears that we have a glitch somewhere in the implementation. We\nshould make the explicit refspec trump the wildcarded ones.\n"},{"id":"174700","messageId":"CAJo=hJv55wVpgXjp4ZjiPSLahdPLS9TH+yWAd2ayf8wcHJFRGw@mail.gmail.com","threadId":"28280","inReplyTo":"7vhb4ww1id.fsf@alter.siamese.dyndns.org","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-09-01T19:14:06Z","receivedAt":"2011-09-01T19:14:06Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Thu, Sep 1, 2011 at 11:39, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Suggested reading:\n>>\n>>   http://git-blame.blogspot.com/2011/08/how-to-inject-malicious-commit-to-git.html\n>>\n>> I am wondering if we are better off applying something along the lines of\n>> this patch, so that with the default configuration, users can notice if\n>> their upstream unexpectedly rewound their branches.\n>>\n>> It would produce\n>>\n>>       [remote]\n>>               url = git://.../git.git/\n>>                 fetch = refs/heads/*:refs/remotes/origin/*\n>>\n>> upon cloning from my repository, and your \"git fetch\" will fail because\n>> the pu (proposed updates) branch is constantly unwinding, but that can be\n>> easily fixed with\n>>\n>>\n>>       [remote]\n>>               url = git://.../git.git/\n>>                 fetch = refs/heads/*:refs/remotes/origin/*\n>>                 fetch = +refs/heads/pu:refs/remotes/origin/pu\n>>\n>> as the explicit refspec trumps the wildcard one.\n>\n> It appears that we have a glitch somewhere in the implementation. We\n> should make the explicit refspec trump the wildcarded ones.\n\nThis is a great idea. :-)\n\n-- \nShawn.\n"},{"id":"174701","messageId":"4E5FDAFE.6050004@drmicha.warpmail.net","threadId":"28280","inReplyTo":"7vliu8w25g.fsf@alter.siamese.dyndns.org","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-09-01T19:20:30Z","receivedAt":"2011-09-01T19:20:30Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 01.09.2011 20:25:\n> Suggested reading:\n> \n> http://git-blame.blogspot.com/2011/08/how-to-inject-malicious-commit-to-git.html\n> \n> I am wondering if we are better off applying something along the lines of\n> this patch, so that with the default configuration, users can notice if\n> their upstream unexpectedly rewound their branches.\n> \n> It would produce\n> \n> \t[remote]\n> \turl = git://.../git.git/\n> fetch = refs/heads/*:refs/remotes/origin/*\n> \n> upon cloning from my repository, and your \"git fetch\" will fail because\n> the pu (proposed updates) branch is constantly unwinding, but that can be\n> easily fixed with\n> \n> \n> \t[remote]\n> \turl = git://.../git.git/\n> fetch = refs/heads/*:refs/remotes/origin/*\n> fetch = +refs/heads/pu:refs/remotes/origin/pu\n> \n> as the explicit refspec trumps the wildcard one.\n> \n> builtin/remote.c | 6 +++---\n> 1 files changed, 3 insertions(+), 3 deletions(-)\n\nMy first thought was \"Oh no\", even though I saw it coming since I read\nyour blog entry. But I realized that it was probably due to the usual\ninertia when habits have to change.\n\nThinking more about it, we try to encourage a workflow where locally\nhistory may be rewritten a lot, and distribution points fast-forward\nonly. We have defaults and settings to discourage (pushes to checked out\nbranches and) non-ff pushes, for example. So I think the above change is\npretty much in line with that reasoning.\n\nThe kernel.org problems gave git some pretty good PR even without that\nchange, but with it we have an even stronger stop put in. On the other\nhand, adding \"+\" to the config for \"pu\" (and peff...) isn't that much of\nan issue, though we might also want \"fetch -f\" as an override - I guess\nwe have that already.\n\nPlus fetch with fsck, yes.\n\nTo\n\n\"I don't need backups, I let others clone.\"\n\nadd\n\n\"I don't need intrusion detection, I let others fetch.\"\n\n;)\n\nMichael\n"},{"id":"174703","messageId":"vpqippcm4x4.fsf@bauges.imag.fr","threadId":"28280","inReplyTo":"4E5FDAFE.6050004@drmicha.warpmail.net","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-09-01T19:35:51Z","receivedAt":"2011-09-01T19:35:51Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Michael J Gruber writes:\n\n> Thinking more about it, we try to encourage a workflow where locally\n> history may be rewritten a lot, and distribution points fast-forward\n> only. We have defaults and settings to discourage (pushes to checked out\n> branches and) non-ff pushes, for example. So I think the above change is\n> pretty much in line with that reasoning.\n\nAgreed. It's not only a security thing, it's also about\nteaching/encourraging workflows.\n\nBy asking users to explicitely say \"yes, I know, this branch can be\nrewond\", we also ask them to think about it before making a mistake.\n\nThat said, enabling the check by default may also become painful. I'd\nvote for a configuration option, defaulting to the current behavior for\nnow. Then we can try living with it for a while and see how painful it\nis.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"174705","messageId":"CAJo=hJv4CkmaJuVvCA2VdO68zn4Xb9EQsdP8p1W-7B9zbvXSaQ@mail.gmail.com","threadId":"28280","inReplyTo":"vpqippcm4x4.fsf@bauges.imag.fr","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-09-01T19:50:48Z","receivedAt":"2011-09-01T19:50:48Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Thu, Sep 1, 2011 at 12:35, Matthieu Moy wrote:\n> By asking users to explicitely say \"yes, I know, this branch can be\n> rewond\", we also ask them to think about it before making a mistake.\n>\n> That said, enabling the check by default may also become painful. I'd\n> vote for a configuration option, defaulting to the current behavior for\n> now. Then we can try living with it for a while and see how painful it\n> is.\n\nI suspect the vast majority of branches in the wild do not rewind\nunder normal conditions. Users who work against branches that rewind\n(e.g. those of us basing on a topic in pu) are already sophisticated\nenough with Git to understand what the fetch error would mean and fix\nit.\n\nIMHO, just change the default in clone, and better, add a warning to\nfetch if that default pattern is still in the configuration file. Let\nthe user either remove the wildcarded force fetch spec, or add a new\nconfiguration variable to his remote block to silence the warning.\n\n-- \nShawn.\n"},{"id":"174721","messageId":"20110902000039.GB9339@sigill.intra.peff.net","threadId":"28280","inReplyTo":"7vliu8w25g.fsf@alter.siamese.dyndns.org","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-02T00:00:39Z","receivedAt":"2011-09-02T00:00:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 01, 2011 at 11:25:31AM -0700, Junio C Hamano wrote:\n\n> Suggested reading:\n> \n> http://git-blame.blogspot.com/2011/08/how-to-inject-malicious-commit-to-git.html\n> \n> I am wondering if we are better off applying something along the lines of\n> this patch, so that with the default configuration, users can notice if\n> their upstream unexpectedly rewound their branches.\n\nHmm. This feels like it's subtly changing the meaning of\nrefs/remotes/$remote/*.\n\nRight now, I think of it as a local cache for whatever the remote side\nhas. In other words, a way of separating the network-fetching parts of\nthe workflow from the local parts. And in that sense, it is perfectly\nreasonable to overwrite with what the other side has, whether they\nrewind or not, because we are just representing what they have. And\nsince we keep a reflog, it's not as if the previous state is lost to us\nlocally.\n\nBut with this change, we are making a policy judgement about what to\nfetch. And as you noticed, it means that users need to start telling git\nabout their policy (e.g., mentioning in the refspecs that pu can rewind)\nin order to keep fetch working.\n\nSo I consider that a downside, because it's extra work for the user[1].\nWhat are the upsides?\n\nIs this about preventing workflow-related mistakes where people\naccidentally merge in rebased commits, creating annoying shadow\nhistories?\n\nIs it about preventing malicious rewinds from infecting downstream\nrepositories?\n\nIf the former, then I suspect we need to give more guidance to the user\nthan saying \"reject, non-fast-forward\". What then? Should they \"fetch\n-f\"? Or \"pull --rebase\" (actually, how do they even fetch the branch\nnow for such a pull --rebase)? Or talk out-of-band to the repo owner?\n\nIf the latter, then I think we should be specific about the attack\nscenarios, and what happens with and without this config. And if it's a\nsecurity precaution, what cases doesn't it cover (e.g., initial clone is\nstill vulnerable, as is a one-off pull. As are are malicious insertion\nattacks that don't involve rewinding).\n\nAnd then we can weigh the upsides and the downsides.\n\n-Peff\n\n[1] What I really don't like is that cloning git.git is no longer:\n\n git clone git://git.kernel.org/pub/scm/git/git.git\n\n which is a minimal as it can be, but becomes:\n\n git clone git://git.kernel.org/pub/scm/git/git.git\n cd git\n git config --add remote.origin.fetch +refs/heads/pu:refs/remotes/origin/pu\n\n It's not that my fingers are too tired to do all that typing, but\n rather that the first set of instructions is very easy to explain,\n and the second one is full of magic and head-scratching about why\n git isn't handling this magic itself.\n\n It would be considerably nicer if the server had some way of saying\n \"I expect this branch to be rewound\". Which has been discussed off\n and on over the years, as I recall.\n"},{"id":"174733","messageId":"vpqliu7lc8d.fsf@bauges.imag.fr","threadId":"28280","inReplyTo":"CAJo=hJv4CkmaJuVvCA2VdO68zn4Xb9EQsdP8p1W-7B9zbvXSaQ@mail.gmail.com","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-09-02T05:55:30Z","receivedAt":"2011-09-02T05:55:30Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Shawn Pearce writes:\n\n> On Thu, Sep 1, 2011 at 12:35, Matthieu Moy wrote:\n>> By asking users to explicitely say \"yes, I know, this branch can be\n>> rewond\", we also ask them to think about it before making a mistake.\n>>\n>> That said, enabling the check by default may also become painful. I'd\n>> vote for a configuration option, defaulting to the current behavior for\n>> now. Then we can try living with it for a while and see how painful it\n>> is.\n>\n> I suspect the vast majority of branches in the wild do not rewind\n> under normal conditions. Users who work against branches that rewind\n> (e.g. those of us basing on a topic in pu)\n\nErr, I don't think it's about people basing their work on pu, but rather\nabout anybody fetching from pu, i.e. everybody calling \"git fetch\" or\n\"git pull\" in their clone.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"174734","messageId":"4E607F27.2000405@viscovery.net","threadId":"28280","inReplyTo":"20110902000039.GB9339@sigill.intra.peff.net","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-09-02T07:00:55Z","receivedAt":"2011-09-02T07:00:55Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/2/2011 2:00, schrieb Jeff King:\n> Right now, I think of it as a local cache for whatever the remote side\n> has. In other words, a way of separating the network-fetching parts of\n> the workflow from the local parts.\n\nThis is also my interpretation. For this reason, I don't think a change is\nnecessary.\n\n> So I consider that a downside, because it's extra work for the user[1].\n> What are the upsides?\n> \n> Is this about preventing workflow-related mistakes where people\n> accidentally merge in rebased commits, creating annoying shadow\n> histories?\n> \n> Is it about preventing malicious rewinds from infecting downstream\n> repositories?\n\nAll good questions to ask.\n\n> [1] What I really don't like is that cloning git.git is no longer:\n> \n> git clone git://git.kernel.org/pub/scm/git/git.git\n> \n> which is a minimal as it can be, but becomes:\n> \n> git clone git://git.kernel.org/pub/scm/git/git.git\n> cd git\n> git config --add remote.origin.fetch +refs/heads/pu:refs/remotes/origin/pu\n> \n> It's not that my fingers are too tired to do all that typing, but\n> rather that the first set of instructions is very easy to explain,\n> and the second one is full of magic and head-scratching about why\n> git isn't handling this magic itself.\n\nAbsolutely.\n\n> It would be considerably nicer if the server had some way of saying\n> \"I expect this branch to be rewound\". Which has been discussed off\n> and on over the years, as I recall.\n\nSo, if such a feature were available, wouldn't it be nicer if the initial\nclone set up the refspec like this:\n\n [remote \"origin\"]\n url = git://git.kernel.org/pub/scm/git/git.git\n fetch = +refs/heads/*:refs/remotes/origin/*\n fetch = refs/heads/maint:refs/remotes/origin/maint\n fetch = refs/heads/master:refs/remotes/origin/master\n\ni.e., the non-wildcard refspec are about which branches are *not* expected\nto be rewound rather than the other way around.\n\n-- Hannes\n"},{"id":"174737","messageId":"4E6088F9.5070102@drmicha.warpmail.net","threadId":"28280","inReplyTo":"20110902000039.GB9339@sigill.intra.peff.net","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-09-02T07:42:49Z","receivedAt":"2011-09-02T07:42:49Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 02.09.2011 02:00:\n> On Thu, Sep 01, 2011 at 11:25:31AM -0700, Junio C Hamano wrote:\n> \n>> Suggested reading:\n>>\n>> http://git-blame.blogspot.com/2011/08/how-to-inject-malicious-commit-to-git.html\n>>\n>> I am wondering if we are better off applying something along the lines of\n>> this patch, so that with the default configuration, users can notice if\n>> their upstream unexpectedly rewound their branches.\n> \n> Hmm. This feels like it's subtly changing the meaning of\n> refs/remotes/$remote/*.\n> \n> Right now, I think of it as a local cache for whatever the remote side\n> has. In other words, a way of separating the network-fetching parts of\n> the workflow from the local parts. And in that sense, it is perfectly\n> reasonable to overwrite with what the other side has, whether they\n> rewind or not, because we are just representing what they have. And\n> since we keep a reflog, it's not as if the previous state is lost to us\n> locally.\n> \n> But with this change, we are making a policy judgement about what to\n> fetch. And as you noticed, it means that users need to start telling git\n> about their policy (e.g., mentioning in the refspecs that pu can rewind)\n> in order to keep fetch working.\n> \n> So I consider that a downside, because it's extra work for the user[1].\n> What are the upsides?\n> \n> Is this about preventing workflow-related mistakes where people\n> accidentally merge in rebased commits, creating annoying shadow\n> histories?\n> \n> Is it about preventing malicious rewinds from infecting downstream\n> repositories?\n> \n> If the former, then I suspect we need to give more guidance to the user\n> than saying \"reject, non-fast-forward\". What then? Should they \"fetch\n> -f\"? Or \"pull --rebase\" (actually, how do they even fetch the branch\n> now for such a pull --rebase)? Or talk out-of-band to the repo owner?\n> \n> If the latter, then I think we should be specific about the attack\n> scenarios, and what happens with and without this config. And if it's a\n> security precaution, what cases doesn't it cover (e.g., initial clone is\n> still vulnerable, as is a one-off pull. As are are malicious insertion\n> attacks that don't involve rewinding).\n> \n> And then we can weigh the upsides and the downsides.\n> \n> -Peff\n> \n> [1] What I really don't like is that cloning git.git is no longer:\n> \n> git clone git://git.kernel.org/pub/scm/git/git.git\n> \n> which is a minimal as it can be, but becomes:\n> \n> git clone git://git.kernel.org/pub/scm/git/git.git\n> cd git\n> git config --add remote.origin.fetch +refs/heads/pu:refs/remotes/origin/pu\n> \n> It's not that my fingers are too tired to do all that typing, but\n> rather that the first set of instructions is very easy to explain,\n> and the second one is full of magic and head-scratching about why\n> git isn't handling this magic itself.\n> \n> It would be considerably nicer if the server had some way of saying\n> \"I expect this branch to be rewound\". Which has been discussed off\n> and on over the years, as I recall.\n\nHmm, that sounds like the often requested feature to have part of the\nconfig distributed by \"clone\" as well, possibly after displaying it and\ngetting user confirmation.\n\nMichael\n"},{"id":"174748","messageId":"20110902152650.GA19213@sigill.intra.peff.net","threadId":"28280","inReplyTo":"4E607F27.2000405@viscovery.net","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-02T15:26:50Z","receivedAt":"2011-09-02T15:26:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 02, 2011 at 09:00:55AM +0200, Johannes Sixt wrote:\n\n> > It would be considerably nicer if the server had some way of saying\n> > \"I expect this branch to be rewound\". Which has been discussed off\n> > and on over the years, as I recall.\n> \n> So, if such a feature were available, wouldn't it be nicer if the initial\n> clone set up the refspec like this:\n> \n> [remote \"origin\"]\n> url = git://git.kernel.org/pub/scm/git/git.git\n> fetch = +refs/heads/*:refs/remotes/origin/*\n> fetch = refs/heads/maint:refs/remotes/origin/maint\n> fetch = refs/heads/master:refs/remotes/origin/master\n> \n> i.e., the non-wildcard refspec are about which branches are *not* expected\n> to be rewound rather than the other way around.\n\nI don't see the advantage one way or the other. Doesn't it just amount\nto what the default will be? And isn't \"not rewind\" generally the more\ncommon, and hence a better default?\n\nOr are you saying that for backwards compatibility, it would be better\nto end up with a refspec more like what we have now? That I can see the\nadvantage of.\n\n-Peff\n"},{"id":"174749","messageId":"20110902152947.GB19213@sigill.intra.peff.net","threadId":"28280","inReplyTo":"4E6088F9.5070102@drmicha.warpmail.net","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-02T15:29:47Z","receivedAt":"2011-09-02T15:29:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 02, 2011 at 09:42:49AM +0200, Michael J Gruber wrote:\n\n> > It would be considerably nicer if the server had some way of saying\n> > \"I expect this branch to be rewound\". Which has been discussed off\n> > and on over the years, as I recall.\n> \n> Hmm, that sounds like the often requested feature to have part of the\n> config distributed by \"clone\" as well, possibly after displaying it and\n> getting user confirmation.\n\nYeah, if distributed config existed, that would be a good place to put\nthis information.\n\nAnd I personally thing a limited form[1] of distributed config is a\nsensible idea, but I'm not sure everybody else agrees.\n\n-Peff\n\n[1] My idea of \"limited\" would be an allow-known-good list of harmless\nconfig keys which we would respect when they came from the remote, with\nthe option for the user to whitelist or blacklist more keys if they\nwanted.\n"},{"id":"174753","messageId":"7v4o0uncq0.fsf@alter.siamese.dyndns.org","threadId":"28280","inReplyTo":"20110902152947.GB19213@sigill.intra.peff.net","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-02T16:14:15Z","receivedAt":"2011-09-02T16:14:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> [1] My idea of \"limited\" would be an allow-known-good list of harmless\n> config keys which we would respect when they came from the remote, with\n> the option for the user to whitelist or blacklist more keys if they\n> wanted.\n\nIt coincides with my idea too, but it might be a very limited set. For\nexample, there may be a good \"suggested by upstream\" default for LHS of\nfetch refspecs (e.g. somebody may have 47 topics published but majority of\npeople are expected to follow only his \"master\" branch), but it is up to\nthe user of that suggestion what the RHS would be.\n"},{"id":"174755","messageId":"20110902162524.GC19690@sigill.intra.peff.net","threadId":"28280","inReplyTo":"7v4o0uncq0.fsf@alter.siamese.dyndns.org","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-02T16:25:24Z","receivedAt":"2011-09-02T16:25:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 02, 2011 at 09:14:15AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > [1] My idea of \"limited\" would be an allow-known-good list of harmless\n> > config keys which we would respect when they came from the remote, with\n> > the option for the user to whitelist or blacklist more keys if they\n> > wanted.\n> \n> It coincides with my idea too, but it might be a very limited set. For\n> example, there may be a good \"suggested by upstream\" default for LHS of\n> fetch refspecs (e.g. somebody may have 47 topics published but majority of\n> people are expected to follow only his \"master\" branch), but it is up to\n> the user of that suggestion what the RHS would be.\n\nYeah. That leads to synthesizing local keys based on what remote keys\nsay. Which is pretty straightforward if you are just fetching the\nremote's config during clone, and then copying or creating local keys\nbased on that in your own .git/config (e.g., by creating full refspecs\nwith upstream's idea of the LHS, and our idea of the RHS).\n\nBut it becomes hard to keep your local config in sync with updates on\nthe remote end. When the remote now adds \"next\" to the list of\ninteresting branches, by what mechanism do you fix up your local config?\nCertainly we wouldn't want to rewrite the local config without\nconsulting the user, because they may have reviewed or modified it since\nit was created.\n\nOne possible solution is that the local config could dynamically depend\non the remote config. E.g., the fetch refspec has something like a\nwildcard that matches only the branches that the remote provides to us\nvia some \"interesting branches\" config key. Then it's OK for git to\nupdate the \"interesting branches\" key from the remote. Either the user\nis OK with respecting that (because they have left the wildcard in\nplace), or not (because they have changed the refspec not to use that\nwildcard).\n\nI do worry that could quickly get complex, and people would start\nwanting a Turing-complete config language. :)\n\n-Peff\n"},{"id":"174758","messageId":"7vsjoelwms.fsf@alter.siamese.dyndns.org","threadId":"28280","inReplyTo":"20110902162524.GC19690@sigill.intra.peff.net","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-02T16:47:07Z","receivedAt":"2011-09-02T16:47:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> It coincides with my idea too, but it might be a very limited set. For\n>> example, there may be a good \"suggested by upstream\" default for LHS of\n>> fetch refspecs (e.g. somebody may have 47 topics published but majority of\n>> people are expected to follow only his \"master\" branch), but it is up to\n>> the user of that suggestion what the RHS would be.\n>\n> Yeah. That leads to synthesizing local keys based on what remote keys\n> say. Which is pretty straightforward if you are just fetching the\n> remote's config during clone, and then copying or creating local keys\n> based on that in your own .git/config (e.g., by creating full refspecs\n> with upstream's idea of the LHS, and our idea of the RHS).\n> ...\n> I do worry that could quickly get complex, and people would start\n> wanting a Turing-complete config language. :)\n\nMy real point was that more often than not the settings of configuration\nvariables are inherently per repository not per project, and even though\nwe may hear people want shared configs, possibly in-tree, distributed as\npart of the projects, such a set-up would not be very useful, before you\neven consider merging updates but just taking them as suggested initial\nvalues. The choice to take, ignore, or tweak the suggestions all depend\non the semantics of each variable, and it becomes more so once you start\ntalking about merging updates.\n"},{"id":"174879","messageId":"CAJo=hJtuUe1ajjW9dNU4JzjE+P94a42W7ZvC+iQBQTeGXVvS8Q@mail.gmail.com","threadId":"28280","inReplyTo":"20110902162524.GC19690@sigill.intra.peff.net","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-09-05T18:15:26Z","receivedAt":"2011-09-05T18:15:26Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Sep 2, 2011 at 09:25, Jeff King wrote:\n> On Fri, Sep 02, 2011 at 09:14:15AM -0700, Junio C Hamano wrote:\n>>\n>> It coincides with my idea too, but it might be a very limited set. For\n>> example, there may be a good \"suggested by upstream\" default for LHS of\n>> fetch refspecs (e.g. somebody may have 47 topics published but majority of\n>> people are expected to follow only his \"master\" branch), but it is up to\n>> the user of that suggestion what the RHS would be.\n...\n> One possible solution is that the local config could dynamically depend\n> on the remote config. E.g., the fetch refspec has something like a\n> wildcard that matches only the branches that the remote provides to us\n> via some \"interesting branches\" config key. Then it's OK for git to\n> update the \"interesting branches\" key from the remote. Either the user\n> is OK with respecting that (because they have left the wildcard in\n> place), or not (because they have changed the refspec not to use that\n> wildcard).\n>\n> I do worry that could quickly get complex, and people would start\n> wanting a Turing-complete config language. :)\n\nWhat are we trying to do here?\n\nI had some thought that dropping the \"+\" might prevent a remote\nrepository from being fetched from if it was rewound by an evil\nattacker that now controls it. Unfortunately that attack is a\npointless one. Which makes this change to remove the \"+\" from fetch\nspecs also pointless.\n\nIf the attacker knows Git clients always fetch rewinds, he might be\ntempted to rewrite some part of history and serve his modified history\nof events to clients. But the repository owner (if using a private\nper-user repository model like the Linux kernel developers use) would\nnotice on their next push, and sound the alarm that her repository has\nbeen damaged and should not be trusted.\n\nIf on the other hand Git clients never fetch rewinds, the attacker\nwould just add a new commit to the tip of the history, and serve that.\nAgain, the repository owner would notice on their next push, and\nnotify people the repository is not to be trusted.\n\nEither way, the \"+\" in the fetch spec has no impact on the attack. The\ndefault just changes the attacker's choices slightly.\n\n\nMaybe instead of getting a project policy from the server, we observe\nthe server's behavior over time from the client's reflog. If every\nupdate to \"maint\" that _I_ have observed has always been a\nfast-forward, a rewind on that branch should be a lot more verbose in\nthe fetch output than \"force update\". That is pretty easy to observe\nfrom the reflog too, its just a scan of the records and either\nmatching the message, or checking the merge status of the old-new\npairs listed in the record. We don't even need to read the entire log\non every fetch, we could cache this data.\n\nThe main reason to alert the user that a branch rewound is to give\nthem a chance to correct their client if they need to. If a branch\nnormally doesn't rewind (e.g. next) but then suddenly does (e.g.\nrelease cycle), but I haven't used this client in 3 weeks, its nice to\ngive me more of a \"HEY STUPID FIX YOUR TOPICS\" warning than just the\nlittle quiet \"force update\" we give.\n\n-- \nShawn.\n"},{"id":"174887","messageId":"20110905204729.GB4221@sigill.intra.peff.net","threadId":"28280","inReplyTo":"CAJo=hJtuUe1ajjW9dNU4JzjE+P94a42W7ZvC+iQBQTeGXVvS8Q@mail.gmail.com","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-05T20:47:29Z","receivedAt":"2011-09-05T20:47:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2011 at 11:15:26AM -0700, Shawn O. Pearce wrote:\n\n> > One possible solution is that the local config could dynamically depend\n> > on the remote config. E.g., the fetch refspec has something like a\n> [...]\n> \n> What are we trying to do here?\n\nWe veered way off topic into the idea of generally pulling config from a\nremote. This was just one specific example of how it could be used, and\nwhat kinds of complications that might entail.\n\n> If the attacker knows Git clients always fetch rewinds, he might be\n> tempted to rewrite some part of history and serve his modified history\n> of events to clients. But the repository owner (if using a private\n> per-user repository model like the Linux kernel developers use) would\n> notice on their next push, and sound the alarm that her repository has\n> been damaged and should not be trusted.\n> \n> If on the other hand Git clients never fetch rewinds, the attacker\n> would just add a new commit to the tip of the history, and serve that.\n> Again, the repository owner would notice on their next push, and\n> notify people the repository is not to be trusted.\n> \n> Either way, the \"+\" in the fetch spec has no impact on the attack. The\n> default just changes the attacker's choices slightly.\n\nExactly. This is what I was hinting at in my original email in this\nthread. My gut feeling is that it's not useful as a security measure,\nbut I was trying to challenge people to prove me wrong by showing a case\nwhere the attacker can't just trivially modify his attack to get the\nsame result.\n\n> Maybe instead of getting a project policy from the server, we observe\n> the server's behavior over time from the client's reflog. If every\n> update to \"maint\" that _I_ have observed has always been a\n> fast-forward, a rewind on that branch should be a lot more verbose in\n> the fetch output than \"force update\". That is pretty easy to observe\n> from the reflog too, its just a scan of the records and either\n> matching the message, or checking the merge status of the old-new\n> pairs listed in the record. We don't even need to read the entire log\n> on every fetch, we could cache this data.\n\nHmm. That would probably work most of the time in practice. But it seems\nlike it would be quite confusing when the heuristic is wrong (e.g.,\nJunio rewinds next once every few months, and other than that, it always\nfast forwards). On the other hand, if the failure mode of the heuristic\nis only a slightly bigger warning, then it's not that big a deal.\n\n> The main reason to alert the user that a branch rewound is to give\n> them a chance to correct their client if they need to. If a branch\n> normally doesn't rewind (e.g. next) but then suddenly does (e.g.\n> release cycle), but I haven't used this client in 3 weeks, its nice to\n> give me more of a \"HEY STUPID FIX YOUR TOPICS\" warning than just the\n> little quiet \"force update\" we give.\n\nSure. I'm totally open to the idea of making the non-fast-forward\nwarning more obvious. Suggestions for wording (though I am tempted by\n\"HEY STUPID\" above ;) )?\n\n-Peff\n"},{"id":"174888","messageId":"CAJo=hJtvU+ujYBMvrgVJpBdaTUq+NOsQwVFkL-A4pHv-CRPdDg@mail.gmail.com","threadId":"28280","inReplyTo":"20110905204729.GB4221@sigill.intra.peff.net","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-09-05T20:53:42Z","receivedAt":"2011-09-05T20:53:42Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Mon, Sep 5, 2011 at 13:47, Jeff King wrote:\n> On Mon, Sep 05, 2011 at 11:15:26AM -0700, Shawn O. Pearce wrote:\n>> Maybe instead of getting a project policy from the server, we observe\n>> the server's behavior over time from the client's reflog.\n>\n> Hmm. That would probably work most of the time in practice. But it seems\n> like it would be quite confusing when the heuristic is wrong (e.g.,\n> Junio rewinds next once every few months, and other than that, it always\n> fast forwards). On the other hand, if the failure mode of the heuristic\n> is only a slightly bigger warning, then it's not that big a deal.\n\nRight. Its probably a bigger failure not to warn than to warn here too.\n\n>> The main reason to alert the user that a branch rewound is to give\n>> them a chance to correct their client if they need to. If a branch\n>> normally doesn't rewind (e.g. next) but then suddenly does (e.g.\n>> release cycle), but I haven't used this client in 3 weeks, its nice to\n>> give me more of a \"HEY STUPID FIX YOUR TOPICS\" warning than just the\n>> little quiet \"force update\" we give.\n>\n> Sure. I'm totally open to the idea of making the non-fast-forward\n> warning more obvious. Suggestions for wording (though I am tempted by\n> \"HEY STUPID\" above ;) )?\n\nI'm not suggesting all non-fast-forward should issue a bigger warning.\npu updates daily with a non-fast-forward. That isn't useful.\n\nBut if the local reflog hints that this reference almost never does a\nnon-fast-forward, and then it does, that should be a big warning.\n\n-- \nShawn.\n"},{"id":"174889","messageId":"20110905205735.GA5578@sigill.intra.peff.net","threadId":"28280","inReplyTo":"CAJo=hJtvU+ujYBMvrgVJpBdaTUq+NOsQwVFkL-A4pHv-CRPdDg@mail.gmail.com","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-05T20:57:35Z","receivedAt":"2011-09-05T20:57:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2011 at 01:53:42PM -0700, Shawn O. Pearce wrote:\n\n> > Sure. I'm totally open to the idea of making the non-fast-forward\n> > warning more obvious. Suggestions for wording (though I am tempted by\n> > \"HEY STUPID\" above ;) )?\n> \n> I'm not suggesting all non-fast-forward should issue a bigger warning.\n> pu updates daily with a non-fast-forward. That isn't useful.\n> \n> But if the local reflog hints that this reference almost never does a\n> non-fast-forward, and then it does, that should be a big warning.\n\nRight. What I mean is, what should the bigger warning look like?\n\nAlso, you suggested caching to avoid looking through the whole reflog\neach time. I think you could probably just sample the last 10 or so\nreflog entries to get an idea.\n\n-Peff\n"},{"id":"174890","messageId":"CAJo=hJvFSegSzTOMj824PoG=soj75JMChfRnjyz4rNgUcVM=Jw@mail.gmail.com","threadId":"28280","inReplyTo":"20110905205735.GA5578@sigill.intra.peff.net","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-09-05T21:14:57Z","receivedAt":"2011-09-05T21:14:57Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Mon, Sep 5, 2011 at 13:57, Jeff King wrote:\n> On Mon, Sep 05, 2011 at 01:53:42PM -0700, Shawn O. Pearce wrote:\n>\n>> > Sure. I'm totally open to the idea of making the non-fast-forward\n>> > warning more obvious. Suggestions for wording (though I am tempted by\n>> > \"HEY STUPID\" above ;) )?\n>>\n>> I'm not suggesting all non-fast-forward should issue a bigger warning.\n>> pu updates daily with a non-fast-forward. That isn't useful.\n>>\n>> But if the local reflog hints that this reference almost never does a\n>> non-fast-forward, and then it does, that should be a big warning.\n>\n> Right. What I mean is, what should the bigger warning look like?\n\nIts a bikeshed. I refuse to paint bikesheds. :-)\n\n> Also, you suggested caching to avoid looking through the whole reflog\n> each time. I think you could probably just sample the last 10 or so\n> reflog entries to get an idea.\n\nGood point. 10 or so last records might be representative of the\nbranch's recent behavior, which is all that matters to the user who\nwants this warning.\n\n-- \nShawn.\n"},{"id":"174900","messageId":"vpqvct69l2e.fsf@bauges.imag.fr","threadId":"28280","inReplyTo":"CAJo=hJtuUe1ajjW9dNU4JzjE+P94a42W7ZvC+iQBQTeGXVvS8Q@mail.gmail.com","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-09-06T07:39:05Z","receivedAt":"2011-09-06T07:39:05Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Shawn Pearce writes:\n\n> Again, the repository owner would notice on their next push, and\n> notify people the repository is not to be trusted.\n\nFor simple attack, yes. But if the server is compromised, you can't\ntrust it anymore to error out on non-fast-forward. I don't think it\nwould be very complex to write a modified Git server that would come\nback to the official history before a push, and re-introduce faulty\ncommits right after. pushers wouldn't notice, and fetchers would get\ncompromised history.\n\nOTOH, non-fast-forward fetches can be reliably detected client-side, and\nI like being able to think \"whatever the server does, I don't care\nbecause I'm using Git\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"174904","messageId":"4E65D11C.3050105@drmicha.warpmail.net","threadId":"28280","inReplyTo":"vpqvct69l2e.fsf@bauges.imag.fr","subject":"Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-09-06T07:51:56Z","receivedAt":"2011-09-06T07:51:56Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Matthieu Moy venit, vidit, dixit 06.09.2011 09:39:\n> Shawn Pearce writes:\n> \n>> Again, the repository owner would notice on their next push, and\n>> notify people the repository is not to be trusted.\n> \n> For simple attack, yes. But if the server is compromised, you can't\n> trust it anymore to error out on non-fast-forward. I don't think it\n> would be very complex to write a modified Git server that would come\n> back to the official history before a push, and re-introduce faulty\n> commits right after. pushers wouldn't notice, and fetchers would get\n> compromised history.\n\nExactly. Even on fetch, it could serve different histories depending on\nthe ip so that the pusher does not notice when fetching from the same ip.\n\n> OTOH, non-fast-forward fetches can be reliably detected client-side, and\n> I like being able to think \"whatever the server does, I don't care\n> because I'm using Git\".\n\nreflog based warnings should provide a sane default. We could amend the\nupdate hook for those who want to allow/deny specific branches to\nrewind, or make receiveDeny... branch.-specific, if the multiple\nrefspecs approach is too complicated.\n\nMichael\n"},{"id":"175038","messageId":"20110907212042.GG13364@sigill.intra.peff.net","threadId":"28280","inReplyTo":"CAJo=hJvFSegSzTOMj824PoG=soj75JMChfRnjyz4rNgUcVM=Jw@mail.gmail.com","subject":"[RFC/PATCH] fetch: bigger forced-update warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-07T21:20:42Z","receivedAt":"2011-09-07T21:20:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2011 at 02:14:57PM -0700, Shawn O. Pearce wrote:\n\n> > Right. What I mean is, what should the bigger warning look like?\n> \n> Its a bikeshed. I refuse to paint bikesheds. :-)\n\nHmph. Somebody has to write the patch. :P\n\n> > Also, you suggested caching to avoid looking through the whole reflog\n> > each time. I think you could probably just sample the last 10 or so\n> > reflog entries to get an idea.\n> \n> Good point. 10 or so last records might be representative of the\n> branch's recent behavior, which is all that matters to the user who\n> wants this warning.\n\nActually, because recent ones are near the end, it's much easier to say\n\"look at the last 4096 bytes of reflogs\" rather than \"look at exactly\n10\". For our purposes, it's about the same (actually 4096 is probably\nmore like 18-20, depending on the exact size of each entry. But it's a\npage, so it's probably reasonable).\n\n-- >8 --\nSubject: fetch: bigger forced-update warnings\n\nThe default fetch refspec allows forced-updates. We already\nprint \"forced update\" in the status table, but it's easy to\nmiss. Let's make the warning a little more prominent.\n\nSome branches are expected to rewind, so the prominent\nwarning would be annoying. However, git doesn't know what\nthe expectation is for a particular branch. We can have it\nguess by peeking at the lost couple of reflog entries. If we\nsee all fast forwards, then a new forced-update is probably\nnoteworthy. If we see something that force-updates all the\ntime, it's probably boring and not worth displaying the big\nwarning (we keep the status table \"forced update\" note, of\ncourse).\n\nSigned-off-by: Jeff King \n---\n builtin/fetch.c | 39 +++++++++++++++++++++++++++++++++++++--\n 1 files changed, 37 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 93c9938..93bfefa 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -208,6 +208,34 @@ static struct ref *get_ref_map(struct transport *transport,\n \treturn ref_map;\n }\n \n+struct update_counts {\n+\tunsigned fastforward;\n+\tunsigned forced;\n+};\n+\n+static int count_updates(unsigned char *osha1, unsigned char *nsha1,\n+\t\t\t const char *email, unsigned long timestamp, int tz,\n+\t\t\t const char *message, void *data)\n+{\n+\tstruct update_counts *uc = data;\n+\t/* We could check the ancestry of osha1 and nsha1, but this is way\n+\t * cheaper */\n+\tif (!prefixcmp(message, \"fetch: fast-forward\"))\n+\t\tuc->fastforward++;\n+\telse if (!prefixcmp(message, \"fetch: forced-update\\n\"))\n+\t\tuc->forced++;\n+\treturn 0;\n+}\n+\n+static int forced_update_is_uncommon(const char *ref)\n+{\n+\tstruct update_counts uc;\n+\tmemset(&uc, 0, sizeof(&uc));\n+\tif (for_each_recent_reflog_ent(ref, count_updates, 4096, &uc) < 0)\n+\t\tfor_each_reflog_ent(ref, count_updates, &uc);\n+\treturn uc.fastforward && uc.forced <= 1; /* 1 for the one we just did */\n+}\n+\n #define STORE_REF_ERROR_OTHER 1\n #define STORE_REF_ERROR_DF_CONFLICT 2\n \n@@ -239,7 +267,8 @@ static int s_update_ref(const char *action,\n \n static int update_local_ref(struct ref *ref,\n \t\t\t const char *remote,\n-\t\t\t char *display)\n+\t\t\t char *display,\n+\t\t\t int *uncommon_forced_update)\n {\n \tstruct commit *current = NULL, *updated;\n \tenum object_type type;\n@@ -336,6 +365,8 @@ static int update_local_ref(struct ref *ref,\n \t\t\tTRANSPORT_SUMMARY_WIDTH, quickref, REFCOL_WIDTH, remote,\n \t\t\tpretty_ref,\n \t\t\tr ? _(\"unable to update local ref\") : _(\"forced update\"));\n+\t\tif (!r && forced_update_is_uncommon(ref->name))\n+\t\t\t*uncommon_forced_update = 1;\n \t\treturn r;\n \t} else {\n \t\tsprintf(display, \"! %-*s %-*s -> %s %s\",\n@@ -355,6 +386,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 uncommon_forced_update = 0;\n \n \tfp = fopen(filename, \"a\");\n \tif (!fp)\n@@ -428,7 +460,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\tfputc('\\n', fp);\n \n \t\tif (ref) {\n-\t\t\trc |= update_local_ref(ref, what, note);\n+\t\t\trc |= update_local_ref(ref, what, note,\n+\t\t\t\t\t &uncommon_forced_update);\n \t\t\tfree(ref);\n \t\t} else\n \t\t\tsprintf(note, \"* %-*s %-*s -> FETCH_HEAD\",\n@@ -450,6 +483,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\terror(_(\"some local refs could not be updated; try running\\n\"\n \t\t \" 'git remote prune %s' to remove any old, conflicting \"\n \t\t \"branches\"), remote_name);\n+\tif (uncommon_forced_update)\n+\t\twarning(\"HEY STUPID FIX YOUR TOPICS\");\n \treturn rc;\n }\n \n-- \n1.7.6.10.g62f04\n"},{"id":"175043","messageId":"CAJo=hJuyCQXRfQniNw-spJuDDzbdgNwgNFT0yL5qmgsZe4=K1Q@mail.gmail.com","threadId":"28280","inReplyTo":"20110907212042.GG13364@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] fetch: bigger forced-update warnings","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-09-07T21:39:53Z","receivedAt":"2011-09-07T21:39:53Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Sep 7, 2011 at 14:20, Jeff King wrote:\n> +       if (uncommon_forced_update)\n> +               warning(\"HEY STUPID FIX YOUR TOPICS\");\n\n\n paint\n bikeshed\n because-i-can\n\nHow about:\n\n warning(\"!!! REMOTE BRANCH REWOUND HISTORY !!!\");\n warning(\" Check status report for branches that rewound.\");\n\n\n\n-- \nShawn.\n"},{"id":"175044","messageId":"7vty8o10kj.fsf@alter.siamese.dyndns.org","threadId":"28280","inReplyTo":"20110907212042.GG13364@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] fetch: bigger forced-update warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-07T21:53:32Z","receivedAt":"2011-09-07T21:53:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Subject: fetch: bigger forced-update warnings\n>\n> The default fetch refspec allows forced-updates. We already\n> print \"forced update\" in the status table, but it's easy to\n> miss. Let's make the warning a little more prominent.\n>\n> Some branches are expected to rewind, so the prominent\n> warning would be annoying. However, git doesn't know what\n> the expectation is for a particular branch. We can have it\n> guess by peeking at the lost couple of reflog entries. If we\n\ns/lost/last/\n\n> see all fast forwards, then a new forced-update is probably\n> noteworthy. If we see something that force-updates all the\n> time, it's probably boring and not worth displaying the big\n> warning (we keep the status table \"forced update\" note, of\n> course).\n>\n> Signed-off-by: Jeff King \n\nThis is slightly offtopic, but I have been wondering if this approach do\nthe right thing for \"git pull\". Wouldn't the underlying \"git fetch\" give a\nwarning, and then the calling \"git pull\" go ahead and make a merge,\nscrolling the warning away with the merge/update summary diffstat? That\nwould be a larger change if \"git pull\" needs to stash away the warning\nmessage, do its thing and then spit out the warning later.\n\n> +static int forced_update_is_uncommon(const char *ref)\n> +{\n> +\tstruct update_counts uc;\n> +\tmemset(&uc, 0, sizeof(&uc));\n> +\tif (for_each_recent_reflog_ent(ref, count_updates, 4096, &uc) < 0)\n> +\t\tfor_each_reflog_ent(ref, count_updates, &uc);\n> +\treturn uc.fastforward && uc.forced <= 1; /* 1 for the one we just did */\n> +}\n\nLooks sensible.\n"},{"id":"175045","messageId":"20110907215700.GA27292@sigill.intra.peff.net","threadId":"28280","inReplyTo":"7vty8o10kj.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] fetch: bigger forced-update warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-07T21:57:00Z","receivedAt":"2011-09-07T21:57:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2011 at 02:53:32PM -0700, Junio C Hamano wrote:\n\n> > Some branches are expected to rewind, so the prominent\n> > warning would be annoying. However, git doesn't know what\n> > the expectation is for a particular branch. We can have it\n> > guess by peeking at the lost couple of reflog entries. If we\n> \n> s/lost/last/\n\nOops, thanks.\n\n> This is slightly offtopic, but I have been wondering if this approach do\n> the right thing for \"git pull\". Wouldn't the underlying \"git fetch\" give a\n> warning, and then the calling \"git pull\" go ahead and make a merge,\n> scrolling the warning away with the merge/update summary diffstat? That\n> would be a larger change if \"git pull\" needs to stash away the warning\n> message, do its thing and then spit out the warning later.\n\nI think this particular warning has nothing to do with git-pull. But\nrather, that we should _always_ abort a pull with a forced-update.\nBecause the only sane things to do there are:\n\n 1. Stop and look around, and see if you should be doing a \"git reset\"\n first.\n\nor\n\n 2. \"git pull --rebase\"\n\nBut proceeding with the pull just seems like a disaster. So it is not\nabout \"this usually fast forwards, but isn't now, so let's make the\nwarning bigger\". It is more about noticing that it is a forced-update at\nall. Maybe that's what you meant by \"off-topic\". :)\n\n> > +static int forced_update_is_uncommon(const char *ref)\n> > +{\n> > +\tstruct update_counts uc;\n> > +\tmemset(&uc, 0, sizeof(&uc));\n> > +\tif (for_each_recent_reflog_ent(ref, count_updates, 4096, &uc) < 0)\n> > +\t\tfor_each_reflog_ent(ref, count_updates, &uc);\n> > +\treturn uc.fastforward && uc.forced <= 1; /* 1 for the one we just did */\n> > +}\n> \n> Looks sensible.\n\nNow we just need to paint the shed. :)\n\n-Peff\n"},{"id":"175050","messageId":"201109080042.45980.trast@student.ethz.ch","threadId":"28280","inReplyTo":"20110907212042.GG13364@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] fetch: bigger forced-update warnings","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-09-07T22:42:45Z","receivedAt":"2011-09-07T22:42:45Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King wrote:\n> On Mon, Sep 05, 2011 at 02:14:57PM -0700, Shawn O. Pearce wrote:\n> \n> > > Right. What I mean is, what should the bigger warning look like?\n> > \n> > Its a bikeshed. I refuse to paint bikesheds. :-)\n[...]\n> +\tif (uncommon_forced_update)\n> +\t\twarning(\"HEY STUPID FIX YOUR TOPICS\");\n\nWhatever comes out of the bikeshedding, I'm going to keep a patch\nlocally that refreshes the mental picture of Shawn shouting that!\n\nThat being said, I think there should be a multiline warning pointing\nthe user at the \"recovering from upstream rebase\" section in\ngit-rebase(1). At least by default with an advice.* setting to\ndisable it.\n\n> + if (!prefixcmp(message, \"fetch: fast-forward\"))\n> + uc->fastforward++;\n> + else if (!prefixcmp(message, \"fetch: forced-update\\n\"))\n> + uc->forced++;\n\nThat doesn't work: fetch puts the whole command line there.\nE.g.\n\n git fetch altgit\n --> fetch altgit: fast-forward\n\n git fetch altgit next:refs/remotes/next\n --> fetch altgit next:remotes/altgit/next: fast-forward\n\nThere's also a minor subtlety here that I had to double-check first:\nthe message for a branch creation is 'storing head', so the later\ncheck\n\n> + return uc.fastforward && uc.forced <= 1; /* 1 for the one we just did */\n\nnever triggers at the second fetch.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}