[PATCH v2] clarify documentation for remote helpers

[PATCH v2] clarify documentation for remote helpers

[David Turner]

Signed-off-by: David Turner <dturner@twosigma.com>
---
 Documentation/gitremote-helpers.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index 43f80c8068..f4a165387f 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -297,9 +297,9 @@ Supported if the helper has the "option" capability.
 	same batch are complete. Only objects which were reported
 	in the output of 'list' with a sha1 may be fetched this way.
 +
-Optionally may output a 'lock <file>' line indicating a file under
-GIT_DIR/objects/pack which is keeping a pack until refs can be
-suitably updated.
+Optionally may output a 'lock <file>' line indicating the full path of
+a file under $GIT_DIR/objects/pack which is keeping a pack until refs
+can be suitably updated.  The path must end with ".keep".
 +
 If option 'check-connectivity' is requested, the helper must output
 'connectivity-ok' if the clone is self-contained and connected.
-- 
2.20.1

Re: [PATCH v2] clarify documentation for remote helpers

[Martin Ågren]

On Fri, 30 Aug 2019 at 16:00, David Turner <dturner@twosigma.com> wrote:
>
> Signed-off-by: David Turner <dturner@twosigma.com>
> ---
>  Documentation/gitremote-helpers.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
> index 43f80c8068..f4a165387f 100644
> --- a/Documentation/gitremote-helpers.txt
> +++ b/Documentation/gitremote-helpers.txt
> @@ -297,9 +297,9 @@ Supported if the helper has the "option" capability.
>         same batch are complete. Only objects which were reported
>         in the output of 'list' with a sha1 may be fetched this way.
>  +
> -Optionally may output a 'lock <file>' line indicating a file under
> -GIT_DIR/objects/pack which is keeping a pack until refs can be
> -suitably updated.
> +Optionally may output a 'lock <file>' line indicating the full path of
> +a file under $GIT_DIR/objects/pack which is keeping a pack until refs

I'd still like to suggest backticks: `$GIT_DIR/objects/pack`

> +can be suitably updated.  The path must end with ".keep".
>  +
>  If option 'check-connectivity' is requested, the helper must output
>  'connectivity-ok' if the clone is self-contained and connected.

Here's how I interpret the motivation behind the patch:

  Reading the text on the optional 'lock <file>' line, it's easy to
  believe that the filename should be given relative to
  $GIT_DIR/objects/pack. Actually, one needs to provide the full path.
  Also, we fail to mention that the filename must end with ".pack".

But I'd have to take your word for these two claims, since I haven't
actually tested or dug myself. ;-)

I do wonder... If we're giving a full path which points outside of
$GIT_DIR/objects/pack, what will happen? Will /mnt/my-packs/foo.pack
work just as fine? If yes, then I don't see a reason to mention
$GIT_DIR/objects/pack at all. If no, it sort of begs the question of why
we don't accept relative filenames. (Or mandate them, but that boat has
obviously sailed.) Either way, I'm wondering if there isn't a bug left
here. What do you think?

Martin

RE: [PATCH v2] clarify documentation for remote helpers

[David Turner]

> -----Original Message-----
> From: Martin Ågren <martin.agren@gmail.com>
> Sent: Friday, August 30, 2019 10:09 AM
> To: David Turner <David.Turner@twosigma.com>
> Cc: Git Mailing List <git@vger.kernel.org>
> Subject: Re: [PATCH v2] clarify documentation for remote helpers
> 
> On Fri, 30 Aug 2019 at 16:00, David Turner <dturner@twosigma.com> wrote:
> >
> > Signed-off-by: David Turner <dturner@twosigma.com>
> > ---
> >  Documentation/gitremote-helpers.txt | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/gitremote-helpers.txt
> > b/Documentation/gitremote-helpers.txt
> > index 43f80c8068..f4a165387f 100644
> > --- a/Documentation/gitremote-helpers.txt
> > +++ b/Documentation/gitremote-helpers.txt
> > @@ -297,9 +297,9 @@ Supported if the helper has the "option" capability.
> >         same batch are complete. Only objects which were reported
> >         in the output of 'list' with a sha1 may be fetched this way.
> >  +
> > -Optionally may output a 'lock <file>' line indicating a file under
> > -GIT_DIR/objects/pack which is keeping a pack until refs can be
> > -suitably updated.
> > +Optionally may output a 'lock <file>' line indicating the full path
> > +of a file under $GIT_DIR/objects/pack which is keeping a pack until
> > +refs
> 
> I'd still like to suggest backticks: `$GIT_DIR/objects/pack`
> 
> > +can be suitably updated.  The path must end with ".keep".
> >  +
> >  If option 'check-connectivity' is requested, the helper must output
> > 'connectivity-ok' if the clone is self-contained and connected.
> 
> Here's how I interpret the motivation behind the patch:
> 
>   Reading the text on the optional 'lock <file>' line, it's easy to
>   believe that the filename should be given relative to
>   $GIT_DIR/objects/pack. Actually, one needs to provide the full path.
>   Also, we fail to mention that the filename must end with ".pack".
> 
> But I'd have to take your word for these two claims, since I haven't actually
> tested or dug myself. ;-)

I did test it. 

> I do wonder... If we're giving a full path which points outside of
> $GIT_DIR/objects/pack, what will happen? Will /mnt/my-packs/foo.pack work
> just as fine? If yes, then I don't see a reason to mention $GIT_DIR/objects/pack
> at all. If no, it sort of begs the question of why we don't accept relative
> filenames. (Or mandate them, but that boat has obviously sailed.) Either way,
> I'm wondering if there isn't a bug left here. What do you think?

It  does appear from code inspection that it will accept a path from anywhere.  
And then, I think, that path gets deleted (even if it does not end with ".keep"). 
Is this a "bug"?  I don't know.  I don't really understand what the feature is 
intended to be used for.  Somewhere, there ought to be an explanation of how
git is supposed to handle races during various parts of pack manipulation.
I wouldn't even be using this feature if I could just pass connectivity-ok:
https://www.spinics.net/lists/git/msg365153.html

Will wait to see if anyone has further thoughts before I file v3.