{"thread":{"id":"47246","subject":"Re: git pull","startedAt":"2017-11-20T05:38:06Z","lastAt":"2017-11-20T06:04:14Z","messageCount":2,"participants":["Junio C Hamano","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"332895","messageId":"xmqqr2stldcm.fsf@gitster.mtv.corp.google.com","threadId":"47246","inReplyTo":"CA+55aFzhK+GPO1_Zm2PoQiq5-cHAq6nz1ejrBhGhWX-KAjTU9A@mail.gmail.com","subject":"Re: git pull","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-20T05:37:45Z","receivedAt":"2017-11-20T05:38:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> A few notes for other people who end up doing this:\n\n[this meaning use of insteadOf to redirect public URLs to\nssh://git@gitolite URL]\n\n>  (a) ssh is slower, and the gitolite machine is not as reachable.\n>\n>  (b) it affects your merge commit message.\n>\n> As to (a), yes it's noticeable, but the extra couple of seconds isn't\n> really that big of a deal. Depending on exactly where you are, though,\n> you might end up wanting to use https:// to the public servers\n> instead.\n>\n> But (b) actually ends up being annoying, because I don't like my merge\n> commits to contain references to repositories that aren't actually\n> available unless you have a kernel.org account.\n> ...\n> Which is simple. Just create a .git/hooks/prepare-commit-msg file that contains\n>\n>   #!/bin/sh\n>   sed -i 's|ssh://gitolite.kernel.org/|git://git.kernel.org/|g' \"$1\"\n>\n> and make it executable, and git will do that commit message editing for you.\n\nThis should work with any recent versions of Git (1.7.4.2 and\nupwards), but it still is a workaround.  Should we mark it as a\nfeature request in the Git land to record the URL you typed as-is in\nthe builtin/fetch.c::store_updated_refs() function, instead of the\none that was rewritten by the insteadOf mechanism?\n\nIt would probably need an update to \"struct remote\" to have new\nfields, to teach remote.c::alias_all_urls() not to overwrite the\nurl[] (and pushurl[] merely for symmetry) fields, to add a field to\n\"struct transport\" and teach transport.c::transport_get() to record\nthe original URL in it so that builtin/fetch.c::fetch_refs() can\ngive it to store_updated_refs() instead of the rewritten one.\n\nMany tiny changes in a rather deep codepath, but feels fairly a safe\nchange that is isolated and straightforward.\n\n\n\n"},{"id":"332896","messageId":"CA+55aFxoNsnKvwAX+bub+amL3s3Znvi-fpRTENP31mrgdZZHRw@mail.gmail.com","threadId":"47246","inReplyTo":"xmqqr2stldcm.fsf@gitster.mtv.corp.google.com","subject":"Re: git pull","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-11-20T06:04:07Z","receivedAt":"2017-11-20T06:04:14Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Sun, Nov 19, 2017 at 7:37 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> ...\n>> Which is simple. Just create a .git/hooks/prepare-commit-msg file that contains\n>>\n>>   #!/bin/sh\n>>   sed -i 's|ssh://gitolite.kernel.org/|git://git.kernel.org/|g' \"$1\"\n>>\n>> and make it executable, and git will do that commit message editing for you.\n>\n> This should work with any recent versions of Git (1.7.4.2 and\n> upwards), but it still is a workaround.  Should we mark it as a\n> feature request in the Git land to record the URL you typed as-is in\n> the builtin/fetch.c::store_updated_refs() function, instead of the\n> one that was rewritten by the insteadOf mechanism?\n\nThe main problem with the prepare-commit-msg thing is actually that is\nsuch a nasty hack, and it can change other things than just the remote\nname. Maybe \"gitolite\" might be mentioned in the shortlog of the\nmerge, and then the sed script comes and edits that part too.\n\nIt is really not a huge issue simply because those things don't really\nhappen in real life, but it's the main thing about that\nprepare-commit-msg hook that makes me go \"eww, what an ugly hack\".\n\nBut it's an ugly hack that just happens to work pretty well in practice.\n\nAnd yes, I looked at alternatives. In fact, I  looked at a couple of\nother approaches:\n\n - the one you mention, namely to remember the original url, and use\nthat instead\n\n - the one I'd actually prefer, which is to generalize the whole\n\"insteadOf\" to work in more situations.\n\nWhy would I prefer that second one? It turns out that the \"original\"\nisn't actually necessarily what I'd want either. Several people send\nme pointers to \"https://git.kernel.org/\" and I prefer rewriting them\nto git:// just to be consistent. And now that people have started\ndoing the \"insteadOf\", their pull requests end up containing that\n\"git@gitolite\" version of the URL, so again, I'd actually like to\nrewrite the url _anyway_ in the commit message.\n\nFor example, for the kernel, the git.kernel.org address is very\ncommon, but it also has a very common path prefix, so almost all pull\nmessages for the kernel have that\n\n   \"git://git.kernel.org/pub/scm/linux/kernel/git/\"\n\npart in common, and I have occasioally felt that it's not adding a lot\nof value particularly as it shows up in shortlogs and gitk. I could\nchange my own rules for the first line (instead of the \"Merge tag xyz\nfrom git://...\" maybe I should just have my human-legible version),\nbut I have also considered just rewriting the url to something that\nshortens that very common thing.\n\nSo maybe\n\n   Merge tag 'sound-4.10-rc4' of\ngit://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound\n\ncould be instead\n\n   Merge tag 'sound-4.10-rc4' of git://kernel.org/../tiwai/sound\n\nwhich would keep the _important_ part, and shorten the boilerplate\npart away. But that kind of \"insteadOf\" would only be for the message,\nsince the end result isn't actually a \"real\" URL at all, it's\nliterally a rewritten shorthand.\n\nAgain, I can do all of this with the sed script. But again, it's kind\nof wrong to do it on the whole message, when it's really only the url\nthat it should affect.\n\nSo it would potentially be nice to just have a generic \"rewrite the\nurl\" model, where you can do it for remote fetches, but you could also\ndo it for just the commit message, or you could do it for doing pushes\n(we do have that \"pushinsteadof\" already - exactly because you might\nwant to pull and push from different versions, with the push having to\nuse ssh).\n\nBut, as you say:\n\n> It would probably need an update to \"struct remote\" to have new\n> fields, to teach remote.c::alias_all_urls() not to overwrite the\n> url[] (and pushurl[] merely for symmetry) fields, to add a field to\n> \"struct transport\" and teach transport.c::transport_get() to record\n> the original URL in it so that builtin/fetch.c::fetch_refs() can\n> give it to store_updated_refs() instead of the rewritten one.\n\nYes, the existing \"insteadOf\" is actually hidden surprisingly deep in\nthe remote code, and it's very non-obvious. That works ok for the pull\nand push case, but really not for just the message rewriting case\n(which doesn't happen as part of the pull, but as part of the \"git\nmerge\" that then takes the FETCH_HEAD or MERGE_HEAD that contains the\nurl, and parses that).\n\nAnyway, it's not a big deal. The sed script works. It's a bit hacky,\nbut it does the job.\n\nI might have wished for a different model, but it's almost certainly\nnot worth the effort unless somebody really gets fired up about this\nand decides they really want to do it.\n\n            Linus\n"}]}