[PATCH] Optimize the description of the configuration in packfile-uris doc

[PATCH] Optimize the description of the configuration in packfile-uris doc

[Teng Long]

There is a description problem in the document about packfile-uris. This
patch describes the configuration format of `uploadpack.blobPackfileUri`
more accurately.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/technical/packfile-uri.txt | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt
index f7eabc6c76..1eb525fe76 100644
--- a/Documentation/technical/packfile-uri.txt
+++ b/Documentation/technical/packfile-uri.txt
@@ -35,13 +35,14 @@ include some sort of non-trivial implementation in the Minimum Viable Product,
 at least so that we can test the client.
 
 This is the implementation: a feature, marked experimental, that allows the
-server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>
-<uri>` entries. Whenever the list of objects to be sent is assembled, all such
-blobs are excluded, replaced with URIs. As noted in "Future work" below, the
-server can evolve in the future to support excluding other objects (or other
-implementations of servers could be made that support excluding other objects)
-without needing a protocol change, so clients should not expect that packfiles
-downloaded in this way only contain single blobs.
+server to be configured by one or more `uploadpack.blobPackfileUri=
+<object-hash> <pack-hash> <uri>` entries. Whenever the list of objects to be
+sent is assembled, all such blobs are excluded, replaced with URIs. As noted
+in "Future work" below, the server can evolve in the future to support
+excluding other objects (or other implementations of servers could be made
+that support excluding other objects) without needing a protocol change, so
+clients should not expect that packfiles downloaded in this way only contain
+single blobs.
 
 Client design
 -------------
-- 
2.31.1

Re: [PATCH] Optimize the description of the configuration in packfile-uris doc

[Jonathan Tan]

> There is a description problem in the document about packfile-uris. This
> patch describes the configuration format of `uploadpack.blobPackfileUri`
> more accurately.

Thanks for noticing this. The diff itself is correct, of course.

As for the commit message, limit the title to 50 characters or fewer if
possible. Maybe something like "packfile-uri.txt: fix blobPackfileUri
description" or something like that.

Also in the commit message, maybe mention that the correct format can be
seen in t5702.

Re: [PATCH] Optimize the description of the configuration in packfile-uris doc

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>> There is a description problem in the document about packfile-uris. This
>> patch describes the configuration format of `uploadpack.blobPackfileUri`
>> more accurately.
>
> Thanks for noticing this. The diff itself is correct, of course.
>
> As for the commit message, limit the title to 50 characters or fewer if
> possible. Maybe something like "packfile-uri.txt: fix blobPackfileUri
> description" or something like that.
>
> Also in the commit message, maybe mention that the correct format can be
> seen in t5702.

Thanks for giving a review with clear suggestion for improvements,
Jonathan.  And thanks for contributing, Teng.

Will look forward to seeing an updated patch.

[PATCH v2] packfile-uri.txt: fix blobPackfileUri description

[Teng Long]

Fix the 'uploadpack.blobPackfileUri' description in packfile-uri.txt
and the correct format also can be seen in t5702.

Jonathan Tan <jonathantanmy@google.com> writes:

>As for the commit message, limit the title to 50 characters or fewer if
>possible. Maybe something like "packfile-uri.txt: fix blobPackfileUri
>description" or something like that.

Thanks for mention this, "packfile-uri.txt: fix blobPackfileUri
description" is good and meets the "50 characters" requirement. So the
title is modified. 

>Also in the commit message, maybe mention that the correct format can be
>seen in t5702.

Because I am implementing another patch[1] about supporting the commit
object in packfile-uri, I noticed the `configure_exclusion` function in
t5702, which is now mentioned in the commit message.

[1]https://public-inbox.org/git/20210507021140.31372-1-dyroneteng@gmail.com

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/technical/packfile-uri.txt | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt
index f7eabc6c76..1eb525fe76 100644
--- a/Documentation/technical/packfile-uri.txt
+++ b/Documentation/technical/packfile-uri.txt
@@ -35,13 +35,14 @@ include some sort of non-trivial implementation in the Minimum Viable Product,
 at least so that we can test the client.
 
 This is the implementation: a feature, marked experimental, that allows the
-server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>
-<uri>` entries. Whenever the list of objects to be sent is assembled, all such
-blobs are excluded, replaced with URIs. As noted in "Future work" below, the
-server can evolve in the future to support excluding other objects (or other
-implementations of servers could be made that support excluding other objects)
-without needing a protocol change, so clients should not expect that packfiles
-downloaded in this way only contain single blobs.
+server to be configured by one or more `uploadpack.blobPackfileUri=
+<object-hash> <pack-hash> <uri>` entries. Whenever the list of objects to be
+sent is assembled, all such blobs are excluded, replaced with URIs. As noted
+in "Future work" below, the server can evolve in the future to support
+excluding other objects (or other implementations of servers could be made
+that support excluding other objects) without needing a protocol change, so
+clients should not expect that packfiles downloaded in this way only contain
+single blobs.
 
 Client design
 -------------
-- 
2.31.1.443.g1f2fb5c85f.dirty

Re: [PATCH v2] packfile-uri.txt: fix blobPackfileUri description

[Junio C Hamano]

Teng Long <dyroneteng@gmail.com> writes:

> Fix the 'uploadpack.blobPackfileUri' description in packfile-uri.txt
> and the correct format also can be seen in t5702.
>
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>>As for the commit message, limit the title to 50 characters or fewer if
>>possible. Maybe something like "packfile-uri.txt: fix blobPackfileUri
>>description" or something like that.
>
> Thanks for mention this, "packfile-uri.txt: fix blobPackfileUri
> description" is good and meets the "50 characters" requirement. So the
> title is modified. 
>
>>Also in the commit message, maybe mention that the correct format can be
>>seen in t5702.
>
> Because I am implementing another patch[1] about supporting the commit
> object in packfile-uri, I noticed the `configure_exclusion` function in
> t5702, which is now mentioned in the commit message.
>
> [1]https://public-inbox.org/git/20210507021140.31372-1-dyroneteng@gmail.com
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---

It seems that the above needs a bit more polishing?

I am not sure if moving the sign-off higher and inserting a
three-dash line before "Jonathan Tan writes" would be sufficient,
but with everything under that quoted material does not seem to
belong to a proposed commit log message proper.

Thanks.

>  Documentation/technical/packfile-uri.txt | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt
> index f7eabc6c76..1eb525fe76 100644
> --- a/Documentation/technical/packfile-uri.txt
> +++ b/Documentation/technical/packfile-uri.txt
> @@ -35,13 +35,14 @@ include some sort of non-trivial implementation in the Minimum Viable Product,
>  at least so that we can test the client.
>  
>  This is the implementation: a feature, marked experimental, that allows the
> -server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>
> -<uri>` entries. Whenever the list of objects to be sent is assembled, all such
> -blobs are excluded, replaced with URIs. As noted in "Future work" below, the
> -server can evolve in the future to support excluding other objects (or other
> -implementations of servers could be made that support excluding other objects)
> -without needing a protocol change, so clients should not expect that packfiles
> -downloaded in this way only contain single blobs.
> +server to be configured by one or more `uploadpack.blobPackfileUri=
> +<object-hash> <pack-hash> <uri>` entries. Whenever the list of objects to be
> +sent is assembled, all such blobs are excluded, replaced with URIs. As noted
> +in "Future work" below, the server can evolve in the future to support
> +excluding other objects (or other implementations of servers could be made
> +that support excluding other objects) without needing a protocol change, so
> +clients should not expect that packfiles downloaded in this way only contain
> +single blobs.
>  
>  Client design
>  -------------

Re: [PATCH v2] packfile-uri.txt: fix blobPackfileUri description

[Long Teng]

>It seems that the above needs a bit more polishing?
>
>I am not sure if moving the sign-off higher and inserting a
>three-dash line before "Jonathan Tan writes" would be sufficient,
>but with everything under that quoted material does not seem to
>belong to a proposed commit log message proper.

Sorry, I misunderstood.

I looked at some patches in the community. If I reply to the
reviewer’s suggestion separately and then submit a new patch, is it
the recommended way? (Distinguish between the ‘reply‘ and the
'patch').

Another question is, if I need to continue to complete this patch,
what do I need to do? I think it is to reply to Jonathan Tan
separately, and then resubmit Patch v2. Is this way correct?

Thanks for your reply.

Junio C Hamano <gitster@pobox.com> 于2021年5月12日周三 上午4:50写道：

>
> Teng Long <dyroneteng@gmail.com> writes:
>
> > Fix the 'uploadpack.blobPackfileUri' description in packfile-uri.txt
> > and the correct format also can be seen in t5702.
> >
> > Jonathan Tan <jonathantanmy@google.com> writes:
> >
> >>As for the commit message, limit the title to 50 characters or fewer if
> >>possible. Maybe something like "packfile-uri.txt: fix blobPackfileUri
> >>description" or something like that.
> >
> > Thanks for mention this, "packfile-uri.txt: fix blobPackfileUri
> > description" is good and meets the "50 characters" requirement. So the
> > title is modified.
> >
> >>Also in the commit message, maybe mention that the correct format can be
> >>seen in t5702.
> >
> > Because I am implementing another patch[1] about supporting the commit
> > object in packfile-uri, I noticed the `configure_exclusion` function in
> > t5702, which is now mentioned in the commit message.
> >
> > [1]https://public-inbox.org/git/20210507021140.31372-1-dyroneteng@gmail.com
> >
> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> > ---
>
> It seems that the above needs a bit more polishing?
>
> I am not sure if moving the sign-off higher and inserting a
> three-dash line before "Jonathan Tan writes" would be sufficient,
> but with everything under that quoted material does not seem to
> belong to a proposed commit log message proper.
>
> Thanks.
>
> >  Documentation/technical/packfile-uri.txt | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt
> > index f7eabc6c76..1eb525fe76 100644
> > --- a/Documentation/technical/packfile-uri.txt
> > +++ b/Documentation/technical/packfile-uri.txt
> > @@ -35,13 +35,14 @@ include some sort of non-trivial implementation in the Minimum Viable Product,
> >  at least so that we can test the client.
> >
> >  This is the implementation: a feature, marked experimental, that allows the
> > -server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>
> > -<uri>` entries. Whenever the list of objects to be sent is assembled, all such
> > -blobs are excluded, replaced with URIs. As noted in "Future work" below, the
> > -server can evolve in the future to support excluding other objects (or other
> > -implementations of servers could be made that support excluding other objects)
> > -without needing a protocol change, so clients should not expect that packfiles
> > -downloaded in this way only contain single blobs.
> > +server to be configured by one or more `uploadpack.blobPackfileUri=
> > +<object-hash> <pack-hash> <uri>` entries. Whenever the list of objects to be
> > +sent is assembled, all such blobs are excluded, replaced with URIs. As noted
> > +in "Future work" below, the server can evolve in the future to support
> > +excluding other objects (or other implementations of servers could be made
> > +that support excluding other objects) without needing a protocol change, so
> > +clients should not expect that packfiles downloaded in this way only contain
> > +single blobs.
> >
> >  Client design
> >  -------------

Re: [PATCH v2] packfile-uri.txt: fix blobPackfileUri description

[Junio C Hamano]

Long Teng <dyroneteng@gmail.com> writes:

>>It seems that the above needs a bit more polishing?
>>
>>I am not sure if moving the sign-off higher and inserting a
>>three-dash line before "Jonathan Tan writes" would be sufficient,
>>but with everything under that quoted material does not seem to
>>belong to a proposed commit log message proper.
>
> Sorry, I misunderstood.
>
> I looked at some patches in the community. If I reply to the
> reviewer’s suggestion separately and then submit a new patch, is it
> the recommended way? (Distinguish between the ‘reply‘ and the
> 'patch').

Yes.  Some people seem to omit the final response to reviewer
suggestions on the previous round and just send a revised patch, but
it is much nicer to cleanly conclude the review cycle for the
previous round with a separate response (it could just be "yes,
you're right---I'll incorporate your suggestions in the next round,
thanks") before starting a new cycle.

And the "patch" side should be written to be understandable even by
those who do not have access to the review history of the previous
round(s)---imagine how it appears in "git log" output to those who
did not read the discussion on this mailing list, and write for
them.

Thanks.

packfile-uri.txt: fix blobPackfileUri description

[Teng Long]

Fix the 'uploadpack.blobPackfileUri' description in packfile-uri.txt
and the correct format also can be seen in t5702.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/technical/packfile-uri.txt | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt
index f7eabc6c76..1eb525fe76 100644
--- a/Documentation/technical/packfile-uri.txt
+++ b/Documentation/technical/packfile-uri.txt
@@ -35,13 +35,14 @@ include some sort of non-trivial implementation in the Minimum Viable Product,
 at least so that we can test the client.
 
 This is the implementation: a feature, marked experimental, that allows the
-server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>
-<uri>` entries. Whenever the list of objects to be sent is assembled, all such
-blobs are excluded, replaced with URIs. As noted in "Future work" below, the
-server can evolve in the future to support excluding other objects (or other
-implementations of servers could be made that support excluding other objects)
-without needing a protocol change, so clients should not expect that packfiles
-downloaded in this way only contain single blobs.
+server to be configured by one or more `uploadpack.blobPackfileUri=
+<object-hash> <pack-hash> <uri>` entries. Whenever the list of objects to be
+sent is assembled, all such blobs are excluded, replaced with URIs. As noted
+in "Future work" below, the server can evolve in the future to support
+excluding other objects (or other implementations of servers could be made
+that support excluding other objects) without needing a protocol change, so
+clients should not expect that packfiles downloaded in this way only contain
+single blobs.
 
 Client design
 -------------
-- 
2.31.1.443.g2f6b6330e7.dirty

Re: packfile-uri.txt: fix blobPackfileUri description

[Junio C Hamano]

Teng Long <dyroneteng@gmail.com> writes:

> Fix the 'uploadpack.blobPackfileUri' description in packfile-uri.txt
> and the correct format also can be seen in t5702.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  Documentation/technical/packfile-uri.txt | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)

Will queue; thanks.

Jonathan, how does this look?

> diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt
> index f7eabc6c76..1eb525fe76 100644
> --- a/Documentation/technical/packfile-uri.txt
> +++ b/Documentation/technical/packfile-uri.txt
> @@ -35,13 +35,14 @@ include some sort of non-trivial implementation in the Minimum Viable Product,
>  at least so that we can test the client.
>  
>  This is the implementation: a feature, marked experimental, that allows the
> -server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>
> -<uri>` entries. Whenever the list of objects to be sent is assembled, all such
> -blobs are excluded, replaced with URIs. As noted in "Future work" below, the
> -server can evolve in the future to support excluding other objects (or other
> -implementations of servers could be made that support excluding other objects)
> -without needing a protocol change, so clients should not expect that packfiles
> -downloaded in this way only contain single blobs.
> +server to be configured by one or more `uploadpack.blobPackfileUri=
> +<object-hash> <pack-hash> <uri>` entries. Whenever the list of objects to be
> +sent is assembled, all such blobs are excluded, replaced with URIs. As noted
> +in "Future work" below, the server can evolve in the future to support
> +excluding other objects (or other implementations of servers could be made
> +that support excluding other objects) without needing a protocol change, so
> +clients should not expect that packfiles downloaded in this way only contain
> +single blobs.
>  
>  Client design
>  -------------

Re: packfile-uri.txt: fix blobPackfileUri description

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Teng Long <dyroneteng@gmail.com> writes:
>
>> Fix the 'uploadpack.blobPackfileUri' description in packfile-uri.txt
>> and the correct format also can be seen in t5702.
>>
>> Signed-off-by: Teng Long <dyroneteng@gmail.com>
>> ---
>>  Documentation/technical/packfile-uri.txt | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> Will queue; thanks.
>
> Jonathan, how does this look?

Ping?

>> diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt
>> index f7eabc6c76..1eb525fe76 100644
>> --- a/Documentation/technical/packfile-uri.txt
>> +++ b/Documentation/technical/packfile-uri.txt
>> @@ -35,13 +35,14 @@ include some sort of non-trivial implementation in the Minimum Viable Product,
>>  at least so that we can test the client.
>>  
>>  This is the implementation: a feature, marked experimental, that allows the
>> -server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>
>> -<uri>` entries. Whenever the list of objects to be sent is assembled, all such
>> -blobs are excluded, replaced with URIs. As noted in "Future work" below, the
>> -server can evolve in the future to support excluding other objects (or other
>> -implementations of servers could be made that support excluding other objects)
>> -without needing a protocol change, so clients should not expect that packfiles
>> -downloaded in this way only contain single blobs.
>> +server to be configured by one or more `uploadpack.blobPackfileUri=
>> +<object-hash> <pack-hash> <uri>` entries. Whenever the list of objects to be
>> +sent is assembled, all such blobs are excluded, replaced with URIs. As noted
>> +in "Future work" below, the server can evolve in the future to support
>> +excluding other objects (or other implementations of servers could be made
>> +that support excluding other objects) without needing a protocol change, so
>> +clients should not expect that packfiles downloaded in this way only contain
>> +single blobs.
>>  
>>  Client design
>>  -------------

Re: packfile-uri.txt: fix blobPackfileUri description

[Jonathan Tan]

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Teng Long <dyroneteng@gmail.com> writes:
> >
> >> Fix the 'uploadpack.blobPackfileUri' description in packfile-uri.txt
> >> and the correct format also can be seen in t5702.
> >>
> >> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> >> ---
> >>  Documentation/technical/packfile-uri.txt | 15 ++++++++-------
> >>  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > Will queue; thanks.
> >
> > Jonathan, how does this look?
> 
> Ping?

Sorry, I just got back on vacation. This looks good - it's exactly the
same as the change I approved previously [1].

Teng Long, for future reviews, the review would go faster if you
mentioned the changes in between versions (e.g. in this case, you
updated the commit message following my suggestions).

[1] https://lore.kernel.org/git/20210506164728.336409-1-jonathantanmy@google.com/

Re: packfile-uri.txt: fix blobPackfileUri description

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > Teng Long <dyroneteng@gmail.com> writes:
>> >
>> >> Fix the 'uploadpack.blobPackfileUri' description in packfile-uri.txt
>> >> and the correct format also can be seen in t5702.
>> >>
>> >> Signed-off-by: Teng Long <dyroneteng@gmail.com>
>> >> ---
>> >>  Documentation/technical/packfile-uri.txt | 15 ++++++++-------
>> >>  1 file changed, 8 insertions(+), 7 deletions(-)
>> >
>> > Will queue; thanks.
>> >
>> > Jonathan, how does this look?
>> 
>> Ping?
>
> Sorry, I just got back on vacation. This looks good - it's exactly the
> same as the change I approved previously [1].

Thanks.

>
> Teng Long, for future reviews, the review would go faster if you
> mentioned the changes in between versions (e.g. in this case, you
> updated the commit message following my suggestions).
>
> [1] https://lore.kernel.org/git/20210506164728.336409-1-jonathantanmy@google.com/