{"thread":{"id":"51780","subject":"[PATCH v2] clarify documentation for remote helpers","startedAt":"2019-08-30T13:58:21Z","lastAt":"2019-08-30T14:33:10Z","messageCount":3,"participants":["David Turner","Martin Ågren"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"381575","messageId":"20190830134805.9588-1-dturner@twosigma.com","threadId":"51780","inReplyTo":null,"subject":"[PATCH v2] clarify documentation for remote helpers","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2019-08-30T13:48:05Z","receivedAt":"2019-08-30T13:58:21Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Signed-off-by: David Turner \n---\n Documentation/gitremote-helpers.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex 43f80c8068..f4a165387f 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -297,9 +297,9 @@ Supported if the helper has the \"option\" capability.\n \tsame batch are complete. Only objects which were reported\n \tin the output of 'list' with a sha1 may be fetched this way.\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\n-suitably updated.\n+Optionally may output a 'lock ' line indicating the full path of\n+a file under $GIT_DIR/objects/pack which is keeping a pack until refs\n+can be suitably updated. The path must end with \".keep\".\n +\n If option 'check-connectivity' is requested, the helper must output\n 'connectivity-ok' if the clone is self-contained and connected.\n-- \n2.20.1\n\n"},{"id":"381576","messageId":"CAN0heSouGV+0Z+j_Th794u-RNiLLLNNEpJu__nAH38_q_TSuig@mail.gmail.com","threadId":"51780","inReplyTo":"20190830134805.9588-1-dturner@twosigma.com","subject":"Re: [PATCH v2] clarify documentation for remote helpers","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-08-30T14:09:17Z","receivedAt":"2019-08-30T14:09:32Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Fri, 30 Aug 2019 at 16:00, David Turner wrote:\n>\n> Signed-off-by: David Turner \n> ---\n> Documentation/gitremote-helpers.txt | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\n> index 43f80c8068..f4a165387f 100644\n> --- a/Documentation/gitremote-helpers.txt\n> +++ b/Documentation/gitremote-helpers.txt\n> @@ -297,9 +297,9 @@ Supported if the helper has the \"option\" capability.\n> same batch are complete. Only objects which were reported\n> in the output of 'list' with a sha1 may be fetched this way.\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\n> -suitably updated.\n> +Optionally may output a 'lock ' line indicating the full path of\n> +a file under $GIT_DIR/objects/pack which is keeping a pack until refs\n\nI'd still like to suggest backticks: `$GIT_DIR/objects/pack`\n\n> +can be suitably updated. The path must end with \".keep\".\n> +\n> If option 'check-connectivity' is requested, the helper must output\n> 'connectivity-ok' if the clone is self-contained and connected.\n\nHere's how I interpret the motivation behind the patch:\n\n Reading the text on the optional 'lock ' line, it's easy to\n believe that the filename should be given relative to\n $GIT_DIR/objects/pack. Actually, one needs to provide the full path.\n Also, we fail to mention that the filename must end with \".pack\".\n\nBut I'd have to take your word for these two claims, since I haven't\nactually tested or dug myself. ;-)\n\nI do wonder... If we're giving a full path which points outside of\n$GIT_DIR/objects/pack, what will happen? Will /mnt/my-packs/foo.pack\nwork just as fine? If yes, then I don't see a reason to mention\n$GIT_DIR/objects/pack at all. If no, it sort of begs the question of why\nwe don't accept relative filenames. (Or mandate them, but that boat has\nobviously sailed.) Either way, I'm wondering if there isn't a bug left\nhere. What do you think?\n\nMartin\n"},{"id":"381580","messageId":"5b44a8bd476946b4990bbcb0112757f0@exmbdft7.ad.twosigma.com","threadId":"51780","inReplyTo":"CAN0heSouGV+0Z+j_Th794u-RNiLLLNNEpJu__nAH38_q_TSuig@mail.gmail.com","subject":"RE: [PATCH v2] clarify documentation for remote helpers","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2019-08-30T14:33:08Z","receivedAt":"2019-08-30T14:33:10Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"\n\n> -----Original Message-----\n> From: Martin Ågren \n> Sent: Friday, August 30, 2019 10:09 AM\n> To: David Turner \n> Cc: Git Mailing List \n> Subject: Re: [PATCH v2] clarify documentation for remote helpers\n> \n> On Fri, 30 Aug 2019 at 16:00, David Turner wrote:\n> >\n> > Signed-off-by: David Turner \n> > ---\n> > Documentation/gitremote-helpers.txt | 6 +++---\n> > 1 file changed, 3 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/Documentation/gitremote-helpers.txt\n> > b/Documentation/gitremote-helpers.txt\n> > index 43f80c8068..f4a165387f 100644\n> > --- a/Documentation/gitremote-helpers.txt\n> > +++ b/Documentation/gitremote-helpers.txt\n> > @@ -297,9 +297,9 @@ Supported if the helper has the \"option\" capability.\n> > same batch are complete. Only objects which were reported\n> > in the output of 'list' with a sha1 may be fetched this way.\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\n> > -suitably updated.\n> > +Optionally may output a 'lock ' line indicating the full path\n> > +of a file under $GIT_DIR/objects/pack which is keeping a pack until\n> > +refs\n> \n> I'd still like to suggest backticks: `$GIT_DIR/objects/pack`\n> \n> > +can be suitably updated. The path must end with \".keep\".\n> > +\n> > If option 'check-connectivity' is requested, the helper must output\n> > 'connectivity-ok' if the clone is self-contained and connected.\n> \n> Here's how I interpret the motivation behind the patch:\n> \n> Reading the text on the optional 'lock ' line, it's easy to\n> believe that the filename should be given relative to\n> $GIT_DIR/objects/pack. Actually, one needs to provide the full path.\n> Also, we fail to mention that the filename must end with \".pack\".\n> \n> But I'd have to take your word for these two claims, since I haven't actually\n> tested or dug myself. ;-)\n\nI did test it. \n\n> I do wonder... If we're giving a full path which points outside of\n> $GIT_DIR/objects/pack, what will happen? Will /mnt/my-packs/foo.pack work\n> just as fine? If yes, then I don't see a reason to mention $GIT_DIR/objects/pack\n> at all. If no, it sort of begs the question of why we don't accept relative\n> filenames. (Or mandate them, but that boat has obviously sailed.) Either way,\n> I'm wondering if there isn't a bug left here. What do you think?\n\nIt does appear from code inspection that it will accept a path from anywhere. \nAnd then, I think, that path gets deleted (even if it does not end with \".keep\"). \nIs this a \"bug\"? I don't know. I don't really understand what the feature is \nintended to be used for. Somewhere, there ought to be an explanation of how\ngit is supposed to handle races during various parts of pack manipulation.\nI wouldn't even be using this feature if I could just pass connectivity-ok:\nhttps://www.spinics.net/lists/git/msg365153.html\n\nWill wait to see if anyone has further thoughts before I file v3.\n"}]}