Re: [PATCH] doc: sparse-checkout: Fix list markers

Re: [PATCH] doc: sparse-checkout: Fix list markers

[oneee]

Hello, I was wondering if there’s any reason this hasn’t been reviewed yet. It’s been a month, and I believe this is a relatively simple change to go over.

Thanks.

Re: [PATCH] doc: sparse-checkout: Fix list markers

[Kristoffer Haugsbakk]

On Wed, May 28, 2025, at 08:19, oneee wrote:
> Hello, I was wondering if there’s any reason this hasn’t been reviewed
> yet. It’s been a month, and I believe this is a relatively simple
> change to go over.
>
> Thanks.

I’ve +Cc the original author.

20d87d32915 (sparse-checkout.txt: new document with sparse-checkout
directions, 2022-11-06)

-- 
Kristoffer Haugsbakk

Re: [PATCH] doc: sparse-checkout: Fix list markers

[Patrick Steinhardt]

On Wed, May 28, 2025 at 03:19:54PM +0900, oneee wrote:
> Hello, I was wondering if there’s any reason this hasn’t been reviewed
> yet. It’s been a month, and I believe this is a relatively simple
> change to go over.

It probably just fell through the cracks. I've left some feedback now,
thanks for the ping!

Patrick

[PATCH] doc: sparse-checkout: Fix list markers

[oneee via GitGitGadget]

From: onee-only <kimww0306@gmail.com>

Changed the list markers from a mix of "(1)" and "(b)" to a
consistent numeric format "(1)", "(2)" for clarity and consistency.

Signed-off-by: onee-only <kimww0306@gmail.com>
---
    doc: sparse-checkout: Fix list markers

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1948%2Fonee-only%2Ffix-sparse-checkout-doc-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1948/onee-only/fix-sparse-checkout-doc-v1
Pull-Request: https://github.com/git/git/pull/1948

 Documentation/technical/sparse-checkout.adoc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/technical/sparse-checkout.adoc b/Documentation/technical/sparse-checkout.adoc
index dc2e763bbec..8202172b70b 100644
--- a/Documentation/technical/sparse-checkout.adoc
+++ b/Documentation/technical/sparse-checkout.adoc
@@ -66,7 +66,7 @@ sparsity patterns: patterns from $GIT_DIR/info/sparse-checkout used to
 	reasons: (1) users in cone mode specify directories rather than
 	patterns (their directories are transformed into patterns, but
 	users may think you are talking about non-cone mode if you use the
-	word "patterns"), and (b) the sparse specification might
+	word "patterns"), and (2) the sparse specification might
 	transiently differ in the working tree or index from the sparsity
 	patterns (see "Sparse specification vs. sparsity patterns").
 

base-commit: 4bbb303af69990ccd05fe3a2eb58a1ce036f8220
-- 
gitgitgadget

Re: [PATCH] doc: sparse-checkout: Fix list markers

[Kristoffer Haugsbakk]

> doc: sparse-checkout: Fix list markers

s/Fix/fix/. The initial word after the area should be lower-case.  See
`Documentation/SubmittingPatches` (SP), “describe-changes”.

On Mon, Apr 21, 2025, at 14:41, oneee via GitGitGadget wrote:
> From: onee-only <kimww0306@gmail.com>
>
> Changed the list markers from a mix of "(1)" and "(b)" to a
> consistent numeric format "(1)", "(2)" for clarity and consistency.

Commits are described in imperative mood (SB, “imperative-mood”).

Reading the message I thought it was about a larger change to
consistently use one style over the other.  But you are fixing one
application of inline lists to use either numeric or alphabetic(?),
specifically falling on numeric.

I would write it as:

    doc: sparse-checkout: use consistent inline list style

    Fix this inline list to use a single style, namely numeric, instead of
    `(1)` followed by `(b)`.

(Numeric or numeral, I’m not sure)

> Signed-off-by: onee-only <kimww0306@gmail.com>

I’m not saying the following is not the case.  But FYI the project
demands that commit identification needs to be your real name.

Just saying in case it is a pseudonym.

> +	word "patterns"), and (2) the sparse specification might

I would also use a semicolon to separate `1` and `2`.  I think this is
the correct style for any list introduced by a colon:

    see here: (1) ...; (2) ...; and (5) ...

Because then you can use commas freely inside each point.

Re: [PATCH] doc: sparse-checkout: Fix list markers

[Eric Sunshine]

On Wed, May 28, 2025 at 2:58 AM Kristoffer Haugsbakk
<kristofferhaugsbakk@fastmail.com> wrote:
> On Mon, Apr 21, 2025, at 14:41, oneee via GitGitGadget wrote:
> > Changed the list markers from a mix of "(1)" and "(b)" to a
> > consistent numeric format "(1)", "(2)" for clarity and consistency.
>
> Reading the message I thought it was about a larger change to
> consistently use one style over the other.  But you are fixing one
> application of inline lists to use either numeric or alphabetic(?),
> specifically falling on numeric.

I seem to recall that I had a similar reaction when I originally read
the commit message.

> I would write it as:
>
>     doc: sparse-checkout: use consistent inline list style
>
>     Fix this inline list to use a single style, namely numeric, instead of
>     `(1)` followed by `(b)`.

This does make it clearer.

> > +     word "patterns"), and (2) the sparse specification might
>
> I would also use a semicolon to separate `1` and `2`.  I think this is
> the correct style for any list introduced by a colon:
>
>     see here: (1) ...; (2) ...; and (5) ...
>
> Because then you can use commas freely inside each point.

I'd like to push back on this suggestion, at least in the context of
this particular patch, the purpose of which is to fix an actual
problem in the documentation. The sort of improvement you suggest
probably ought to be done as a separate patch, but let's not make it
seem as if we are heaping extra work on the patch submitter; it's more
important to get the fix itself landed.

As such, the patch itself looks obviously correct and is a definite
improvement as-is. It's not clear why it didn't get picked up; perhaps
it just fell through the cracks.

Re: [PATCH] doc: sparse-checkout: Fix list markers

[Kristoffer Haugsbakk]

On Wed, May 28, 2025, at 09:20, Eric Sunshine wrote:
>> Because then you can use commas freely inside each point.
>
> I'd like to push back on this suggestion, at least in the context of
> this particular patch, the purpose of which is to fix an actual
> problem in the documentation. The sort of improvement you suggest
> probably ought to be done as a separate patch, but let's not make it
> seem as if we are heaping extra work on the patch submitter; it's more
> important to get the fix itself landed.
>
> As such, the patch itself looks obviously correct and is a definite
> improvement as-is. It's not clear why it didn't get picked up; perhaps
> it just fell through the cracks.

Okay, totally reasonable and makes sense IMO.

Re: [PATCH] doc: sparse-checkout: Fix list markers

[oneee]

(I'm resending this email  since the previous message was blocked due
to accidently containing HTML. Thanks for your understanding!)

I'm sorry for the delay.

On Wed, May 28, 2025 at 3:58 PM Kristoffer Haugsbakk
<kristofferhaugsbakk@fastmail.com> wrote:
>
> I would write it as:
>
>     doc: sparse-checkout: use consistent inline list style
>
>     Fix this inline list to use a single style, namely numeric, instead of
>     `(1)` followed by `(b)`.
>
> (Numeric or numeral, I’m not sure)


Thanks for the suggestion. I'll update the message as you wrote.

>
> > Signed-off-by: onee-only <kimww0306@gmail.com>
>
> I’m not saying the following is not the case.  But FYI the project
> demands that commit identification needs to be your real name.
>
> Just saying in case it is a pseudonym.


I wasn't aware of it. I'll change it into my real name. Thanks for
pointing it out.

On Wed, May 28, 2025 at 4:21 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> > > +     word "patterns"), and (2) the sparse specification might
> >
> > I would also use a semicolon to separate `1` and `2`.  I think this is
> > the correct style for any list introduced by a colon:
> >
> >     see here: (1) ...; (2) ...; and (5) ...
> >
> > Because then you can use commas freely inside each point.
>
> I'd like to push back on this suggestion, at least in the context of
> this particular patch, the purpose of which is to fix an actual
> problem in the documentation. The sort of improvement you suggest
> probably ought to be done as a separate patch, but let's not make it
> seem as if we are heaping extra work on the patch submitter; it's more
> important to get the fix itself landed.


I'd like to apply it to the same PR in GitHub, producing an additional
commit. (I don't fully understand how the system works, sorry).

Thank you all for the reviews!

Re: [PATCH] doc: sparse-checkout: Fix list markers

[Eric Sunshine]

On Thu, May 29, 2025 at 11:43 PM oneee <kimww0306@gmail.com> wrote:
> On Wed, May 28, 2025 at 4:21 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > > +     word "patterns"), and (2) the sparse specification might
> > >
> > > I would also use a semicolon to separate `1` and `2`.  I think this is
> > > the correct style for any list introduced by a colon:
> > >
> > >     see here: (1) ...; (2) ...; and (5) ...
> > >
> > > Because then you can use commas freely inside each point.
> >
> > I'd like to push back on this suggestion, at least in the context of
> > this particular patch, the purpose of which is to fix an actual
> > problem in the documentation. The sort of improvement you suggest
> > probably ought to be done as a separate patch, but let's not make it
> > seem as if we are heaping extra work on the patch submitter; it's more
> > important to get the fix itself landed.
>
> I'd like to apply it to the same PR in GitHub, producing an additional
> commit. (I don't fully understand how the system works, sorry).

You're welcome to submit an additional patch implementing Kristoffer's
suggestion. Normally, it would make sense to send them both as a
two-patch series, however, I'm not so sure that that would be a good
idea in this case. The reason I question it is that the original patch
is an obvious and uncontroversial fix, however, Kristoffer's
suggestion -- because it is a subjective change -- may get pushback
from reviewers. If it does get pushback, then that pushback could hold
up the entire series as discussion ensues, thus slowing or even
completely derailing the original "fix" patch from being accepted.

Re: [PATCH] doc: sparse-checkout: Fix list markers

[oneee]

On Fri, May 30, 2025 at 2:01 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> You're welcome to submit an additional patch implementing Kristoffer's
> suggestion. Normally, it would make sense to send them both as a
> two-patch series, however, I'm not so sure that that would be a good
> idea in this case. The reason I question it is that the original patch
> is an obvious and uncontroversial fix, however, Kristoffer's
> suggestion -- because it is a subjective change -- may get pushback
> from reviewers. If it does get pushback, then that pushback could hold
> up the entire series as discussion ensues, thus slowing or even
> completely derailing the original "fix" patch from being accepted.

That makes sense. I'll submit an additional patch after this one is accepted.

Much appreciated!

Re: [PATCH] doc: sparse-checkout: Fix list markers

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> You're welcome to submit an additional patch implementing Kristoffer's
> suggestion. Normally, it would make sense to send them both as a
> two-patch series, however, I'm not so sure that that would be a good
> idea in this case. The reason I question it is that the original patch
> is an obvious and uncontroversial fix, however, Kristoffer's
> suggestion -- because it is a subjective change -- may get pushback
> from reviewers.

I had the same thought.  If I were working on this topic, I'd send
the original as-is with the suggested name fix, let it graduate, and
then after all the dust settled, do enumeration-style changes as a
new and totally independent patch, which may cover enumerations
other than this single instance.

Thanks.

Re: [PATCH] doc: sparse-checkout: Fix list markers

[Patrick Steinhardt]

On Mon, Apr 21, 2025 at 12:41:40PM +0000, oneee via GitGitGadget wrote:
> From: onee-only <kimww0306@gmail.com>
> 
> Changed the list markers from a mix of "(1)" and "(b)" to a
> consistent numeric format "(1)", "(2)" for clarity and consistency.

In Git we use imperative style for commit messages, as if requesting the
code to change. So something like this for example:

  We use a mix of "(1)" and "(b)" to refer to different cases when
  explaining sparsity patterns. Adapt these markers to use the numeric
  format for both clarity and consistency.

> Signed-off-by: onee-only <kimww0306@gmail.com>

We typically prefer your full name instead of using nicknames here. So
if you want to provide your full name, please do so.

> diff --git a/Documentation/technical/sparse-checkout.adoc b/Documentation/technical/sparse-checkout.adoc
> index dc2e763bbec..8202172b70b 100644
> --- a/Documentation/technical/sparse-checkout.adoc
> +++ b/Documentation/technical/sparse-checkout.adoc
> @@ -66,7 +66,7 @@ sparsity patterns: patterns from $GIT_DIR/info/sparse-checkout used to
>  	reasons: (1) users in cone mode specify directories rather than
>  	patterns (their directories are transformed into patterns, but
>  	users may think you are talking about non-cone mode if you use the
> -	word "patterns"), and (b) the sparse specification might
> +	word "patterns"), and (2) the sparse specification might
>  	transiently differ in the working tree or index from the sparsity
>  	patterns (see "Sparse specification vs. sparsity patterns").

The change itself looks like an obvious improvement to me, thanks!

Patrick