[PATCH] clarify documentation for remote helpers

[PATCH] clarify documentation for remote helpers

[David Turner]

Signed-off-by: David Turner <dturner@twosigma.com>
---

This doesn't address the connectivity-ok problem, which I continue to
worry is a real bug.  But it would have saved me a few minutes of
debugging.

 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..30001b2054 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 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] clarify documentation for remote helpers

[Martin Ågren]

On Thu, 29 Aug 2019 at 23:06, David Turner <dturner@twosigma.com> wrote:

> -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 under GIT_DIR/objects/pack which is keeping a pack until
> +refs can be suitably updated.  The path must end with ".keep".

"under under".

Also -- and I realize this is nothing new in your patch -- "GIT_DIR"
should be prefixed with a '$' and that whole path wrapped in backticks
so it gets monospaced. In total, my suggestion would be

-+a file under under GIT_DIR/objects/pack which is keeping a pack until
++a file under `$GIT_DIR/objects/pack` which is keeping a pack until

Whether what you're saying is actually *true*, sorry, no idea. I just
have those nits above to offer.

Martin

RE: [PATCH] clarify documentation for remote helpers

[David Turner]

I was confused, because I read "a file under GIT_DIR/objects/pack" to mean "just the filename".  Some of the things that deal with packs take just the filename (e.g. --keep-pack for git repack).  I'll fix the under under and add $, but I do want to clarify that it's the full path.

> -----Original Message-----
> From: Martin Ågren <martin.agren@gmail.com>
> Sent: Friday, August 30, 2019 12:03 AM
> To: David Turner <David.Turner@twosigma.com>
> Cc: Git Mailing List <git@vger.kernel.org>
> Subject: Re: [PATCH] clarify documentation for remote helpers
> 
> On Thu, 29 Aug 2019 at 23:06, David Turner <dturner@twosigma.com> wrote:
> 
> > -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 under GIT_DIR/objects/pack which is keeping a pack
> > +until refs can be suitably updated.  The path must end with ".keep".
> 
> "under under".
> 
> Also -- and I realize this is nothing new in your patch -- "GIT_DIR"
> should be prefixed with a '$' and that whole path wrapped in backticks so it
> gets monospaced. In total, my suggestion would be
> 
> -+a file under under GIT_DIR/objects/pack which is keeping a pack until
> ++a file under `$GIT_DIR/objects/pack` which is keeping a pack until
> 
> Whether what you're saying is actually *true*, sorry, no idea. I just have those
> nits above to offer.
> 
> Martin

Re: [PATCH] clarify documentation for remote helpers

[Junio C Hamano]

David Turner <David.Turner@twosigma.com> writes:

> I was confused, because I read "a file under GIT_DIR/objects/pack"
> to mean "just the filename".  Some of the things that deal with
> packs take just the filename (e.g. --keep-pack for git repack).
> I'll fix the under under and add $, but I do want to clarify that
> it's the full path.

I think that the phrase wanted to say that the file named with the
option must live under that directory, without any implication that
the directory is used as the base when a relative path is used.  If
the helper MUST give a full pathname and a pathname relative to that
directory is not accepted, it is a good idea to spell it out (also
if it must end with ".keep", that also should be documented---is
there any other restrictions?).

RE: [PATCH] clarify documentation for remote helpers

[David Turner]

> -----Original Message-----
> From: Junio C Hamano <gitster@pobox.com>
> Sent: Friday, August 30, 2019 1:35 PM
> To: David Turner <David.Turner@twosigma.com>
> Cc: Martin Ågren <martin.agren@gmail.com>; Git Mailing List
> <git@vger.kernel.org>
> Subject: Re: [PATCH] clarify documentation for remote helpers
> 
> David Turner <David.Turner@twosigma.com> writes:
> 
> > I was confused, because I read "a file under GIT_DIR/objects/pack"
> > to mean "just the filename".  Some of the things that deal with packs
> > take just the filename (e.g. --keep-pack for git repack).
> > I'll fix the under under and add $, but I do want to clarify that it's
> > the full path.
> 
> I think that the phrase wanted to say that the file named with the option must
> live under that directory, without any implication that the directory is used as
> the base when a relative path is used.  If the helper MUST give a full pathname
> and a pathname relative to that directory is not accepted, it is a good idea to
> spell it out (also if it must end with ".keep", that also should be documented---
> is there any other restrictions?).

The only other restriction I see is: in order for the connectivity-skipping 
optimization to be used, the file with s/.keep/.idx/, and the corresponding 
pack, must exist.  Do you think that's worth mentioning? It seems to be 
implied by the rest of the text.

Re: [PATCH] clarify documentation for remote helpers

[Junio C Hamano]

David Turner <David.Turner@twosigma.com> writes:

> The only other restriction I see is: in order for the connectivity-skipping 
> optimization to be used, the file with s/.keep/.idx/, and the corresponding 
> pack, must exist.  Do you think that's worth mentioning? It seems to be 
> implied by the rest of the text.

Perhaps if you come up with a way to clarify what is merely implied
(e.g.  "this is a mechanism to name a <pack,idx,keep> tuple by
giving only the keep component to do X"), I would imaging that would
be ideal.

Thanks.