threads / patch / 55631

patchOptimize the description of the configuration in packfile-uris doc

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

## tl;dr

12 messages between May 6, 2021 and May 25, 2021. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Teng Long· May 6, 2021, 07:33 UTC · lore

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(-)
Show changes to Documentation/technical/packfile-uri.txt +8 −7
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
Jonathan Tan· May 6, 2021, 16:47 UTC · re: Teng Long · lore

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

> 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.

Junio C Hamano· May 6, 2021, 21:39 UTC · re: Jonathan Tan · lore

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

Jonathan Tan <jonathantanmy@google.com> writes:
Show 12 quoted lines
>> 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.
Teng Long· May 11, 2021, 06:45 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to Documentation/technical/packfile-uri.txt +8 −7
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
Junio C Hamano· May 11, 2021, 20:50 UTC · re: Teng Long · lore

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

Teng Long <dyroneteng@gmail.com> writes:
Show 24 quoted lines
> 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.
Show 29 quoted lines
>  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
>  -------------
Long Teng· May 12, 2021, 14:10 UTC · re: Junio C Hamano · lore

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

Show 6 quoted lines
>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写道:
Show 66 quoted lines
>
> 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
> >  -------------
Junio C Hamano· May 12, 2021, 23:32 UTC · re: Long Teng · lore

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

Long Teng <dyroneteng@gmail.com> writes:
Show 13 quoted lines
>>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.
Teng Long· May 13, 2021, 07:15 UTC · re: Teng Long · lore

packfile-uri.txt: fix blobPackfileUri description

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(-)
Show changes to Documentation/technical/packfile-uri.txt +8 −7
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
Junio C Hamano· May 13, 2021, 21:33 UTC · re: Teng Long · lore

Re: packfile-uri.txt: fix blobPackfileUri description

Teng Long <dyroneteng@gmail.com> writes:
Show 7 quoted lines
> 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?
Show 26 quoted lines
> 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
>  -------------
Junio C Hamano· May 18, 2021, 00:44 UTC · re: Junio C Hamano · lore

Re: packfile-uri.txt: fix blobPackfileUri description

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> 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?
Show 26 quoted lines
>> 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
>>  -------------
Jonathan Tan· May 24, 2021, 15:43 UTC · re: Junio C Hamano · lore

Re: packfile-uri.txt: fix blobPackfileUri description

Show 17 quoted lines
> 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/
Junio C Hamano· May 25, 2021, 00:34 UTC · re: Jonathan Tan · lore

Re: packfile-uri.txt: fix blobPackfileUri description

Jonathan Tan <jonathantanmy@google.com> writes:
Show 20 quoted lines
>> 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.
Show 6 quoted lines
>
> 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/

← back to recent threads