{"thread":{"id":"31519","subject":"[PATCH/RFC] remote-helper: allow fetch to discover sha1 later","startedAt":"2012-09-13T10:10:13Z","lastAt":"2012-09-14T20:49:12Z","messageCount":3,"participants":["Devin J. Pohly","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"198907","messageId":"1347531013-10371-1-git-send-email-djpohly@gmail.com","threadId":"31519","inReplyTo":null,"subject":"[PATCH/RFC] remote-helper: allow fetch to discover sha1 later","fromName":"Devin J. Pohly","fromEmail":"djpohly@gmail.com","sentAt":"2012-09-13T10:10:13Z","receivedAt":"2012-09-13T10:10:13Z","isPatch":true,"sender":{"key":"djpohly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/521294?v=4"},"body":"From: \"Devin J. Pohly\" \n\nAllow a fetch-style remote helper to issue the notification\n ref \nto specify a ref's value during the fetch operation.\n\nCurrently, a remote helper with the \"fetch\" capability cannot fetch a\nref unless its sha1 was known when the \"list\" command was executed.\nThis limitation is arbitrary, as the helper may eventually be able to\ndetermine the correct sha1 as it fetches objects.\n\nSigned-off-by: Devin J. Pohly \n---\n\nSoliciting general opinions - first git patch. :)\n\nThe fetch command can be a lot more convenient than import if you're working\nwith a remote that doesn't give you a commit and the associated objects at the\nsame time. And there's no apparent reason that something like this isn't\npossible.\n\nHow it works: the old_sha1 field is currently set when the output from \"list\"\nis parsed, then not used again until after the \"fetch\" command completes, just\nbefore updating refs. Changing it during the fetch only affects the final\nvalue of the ref. (Forgetting to resolve a ref will result in exactly the\nsame behavior as before: an error from check_everything_connected.)\n\nNot sure if either or both of the warning()s should be a die() instead.\n\n\n Documentation/git-remote-helpers.txt | 8 ++++++--\n transport-helper.c | 24 ++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt\nindex f5836e4..fb4240f 100644\n--- a/Documentation/git-remote-helpers.txt\n+++ b/Documentation/git-remote-helpers.txt\n@@ -229,8 +229,12 @@ Supported if the helper has the \"option\" capability.\n \tto the database. Fetch commands are sent in a batch, one\n \tper line, terminated with a blank line.\n \tOutputs a single blank line when all fetch commands in the\n-\tsame batch are complete. Only objects which were reported\n-\tin the ref list with a sha1 may be fetched this way.\n+\tsame batch are complete.\n++\n+If the named ref was reported in the ref list without a sha1, must\n+output a 'ref ' line once the sha1 is known. This is\n+also required for a symref if its target did not have a sha1 in the\n+list.\n +\n Optionally may output a 'lock ' line indicating a file under\n GIT_DIR/objects/pack which is keeping a pack until refs can be\ndiff --git a/transport-helper.c b/transport-helper.c\nindex cfe0988..6fce419 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -363,6 +363,30 @@ static int fetch_with_fetch(struct transport *transport,\n \t\t\t\twarning(\"%s also locked %s\", data->name, name);\n \t\t\telse\n \t\t\t\ttransport->pack_lockfile = xstrdup(name);\n+\t\t} else if (!prefixcmp(buf.buf, \"ref \")) {\n+\t\t\tconst char *rest = buf.buf + 4;\n+\t\t\tchar *eov, *eon;\n+\t\t\tstruct ref *posn;\n+\n+\t\t\teov = strchr(rest, ' ');\n+\t\t\tif (!eov)\n+\t\t\t\tdie(\"Malformed ref command: %s\", buf.buf);\n+\t\t\teon = strchr(eov + 1, ' ');\n+\t\t\t*eov = '\\0';\n+\t\t\tif (eon)\n+\t\t\t\t*eon = '\\0';\n+\t\t\tfor (i = 0; i < nr_heads; i++) {\n+\t\t\t\tposn = to_fetch[i];\n+\t\t\t\tif (!strcmp(eov + 1, posn->name))\n+\t\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (i == nr_heads || (posn->status & REF_STATUS_UPTODATE)) {\n+\t\t\t\twarning(\"Ref %s is not being fetched\", eov + 1);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (!is_null_sha1(posn->old_sha1))\n+\t\t\t\twarning(\"Ref %s is being overwritten\", eov + 1);\n+\t\t\tget_sha1_hex(rest, posn->old_sha1);\n \t\t}\n \t\telse if (!buf.len)\n \t\t\tbreak;\n-- \n1.7.12\n"},{"id":"199000","messageId":"7vmx0t6sod.fsf@alter.siamese.dyndns.org","threadId":"31519","inReplyTo":"1347531013-10371-1-git-send-email-djpohly@gmail.com","subject":"Re: [PATCH/RFC] remote-helper: allow fetch to discover sha1 later","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-14T06:10:26Z","receivedAt":"2012-09-14T06:10:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Devin J. Pohly\" writes:\n\n> From: \"Devin J. Pohly\" \n>\n> Allow a fetch-style remote helper to issue the notification\n> ref \n> to specify a ref's value during the fetch operation.\n>\n> Currently, a remote helper with the \"fetch\" capability cannot fetch a\n> ref unless its sha1 was known when the \"list\" command was executed.\n> This limitation is arbitrary, as the helper may eventually be able to\n> determine the correct sha1 as it fetches objects.\n\nWhile I am not fundamentally against supporting a remote helper that\nis not capable of saying what the object names of the tip of its\nhistories are before doing a lot of work, I do not think it is a\ngood idea to allow such a helper to claim that it supports \"fetch\"\ncapability, for at least two reasons:\n\n * Being able to \"list\" is essential for \"fetch\" based helpers, I\n think, far from \"arbitrary\". When running a \"git fetch\" against\n such a remote, we can first issue \"list\" to see what it has and\n avoid asking for the refs that point at the histories we already\n have (or the refs that are the same as the last time we fetched\n from the remote). I do not offhand know if we do this kind of\n optimization, but if we don't, we should.\n\n * Existing versions of \"git\" that can drive remote helpers that\n claim to have \"fetch\" capability are not prepared to accept an\n unknown \"refs\" protocol message from the helper, so a helper\n written for your new semantics of \"fetch\" capability will not\n work with them. The point of capability mechanism is to prevent\n such a compatibility issue from breaking the system, and this\n patch goes against that spirit.\n\nSuch a remote helper should not advertise to support \"fetch\"\ncapability, because it does not support it. It can advertise that\nit supports \"something else\" and it is OK to make an updated Git\nthat knows how to drive a remote helper that lack \"fetch\" but\nsupport that \"something else\" work with it.\n"},{"id":"199071","messageId":"20120914204912.GG1072@chiri.hsd1.pa.comcast.net","threadId":"31519","inReplyTo":"7vmx0t6sod.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] remote-helper: allow fetch to discover sha1 later","fromName":"Devin J. Pohly","fromEmail":"djpohly@gmail.com","sentAt":"2012-09-14T20:49:12Z","receivedAt":"2012-09-14T20:49:12Z","isPatch":true,"sender":{"key":"djpohly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/521294?v=4"},"body":"On Thu, Sep 13, 2012 at 11:10:26PM -0700, Junio C Hamano wrote:\n> I do not think it is a good idea to allow such a helper to claim that\n> it supports \"fetch\" capability, for at least two reasons:\n> \n> * Being able to \"list\" is essential for \"fetch\" based helpers.\n> think, far from \"arbitrary\".\n> ...\n\nOh, I don't mean to change the semantics of the list command at all.\nWhat I thought seemed arbitrary was limiting the *fetch* command to refs\nwith pre-known sha1s. Any list-time optimization in place or possible\nfor such refs wouldn't be affected.\n\n(In light of this, specifying a new sha1 for a ref which already had one\nin the list should probably be an error rather than a warning. I'll\nupdate this in the next version and make it clear that a \"ref\" message\nmust only be issued for refs reported without a sha1.)\n\n> * Existing versions of \"git\" that can drive remote helpers that\n> claim to have \"fetch\" capability are not prepared to accept an\n> unknown \"refs\" protocol message from the helper, so a helper\n> written for your new semantics of \"fetch\" capability will not\n> work with them.\n> ...\n\nI see. I didn't realize that new helper + old Git client is a supported\ncombination. Still, hear me out.\n\n1. A new helper must only send a \"ref\" message when git asks for a ref\n without a known sha1.\n2. Asking for a ref without a known sha1 is unsupported, according to\n git-remote-helpers.txt: \"Only objects which were reported in the ref\n list with a sha1 may be fetched [with fetch].\"\n3. Furthermore, asking for a ref without a known sha1 *already* breaks\n in existing versions of git with current handlers:\n\n $ git fetch testfetch::../git1 foo\n fatal: bad object 0000000000000000000000000000000000000000\n error: testfetch::../git1 did not send all necessary objects\n\n Compare - existing version of git, with a new handler:\n\n $ git fetch testref::../git1 foo\n warning: testref unexpectedly said: 'ref 0f31 refs/heads/foo'\n fatal: bad object 0000000000000000000000000000000000000000\n error: testref::../git1 did not send all necessary objects\n\nSo the proposed change doesn't break anything that isn't already broken.\n:)\n\nThat said, if you're still not sold, a new capability is fine. Though I\nthink it can be done without it, I'm certainly not opposed to adding\none.\n"}]}