Re: [PATCH] config doc: indent descriptions of feature.* variables

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jun 03 2021, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Jun 02 2021, Andrei Rybak wrote:
>
>> Config variables feature.experimental and feature.manyFiles are grouped
>> together under "feature.*".  However, this is not easily visible when
>> scanning the help page of git-config.
>>
>> Indent the descriptions of individual feature.* config variables to help
>> the reader distinguish this group of variables.
>>
>> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
>> ---
>>  Documentation/config/feature.txt | 40 +++++++++++++++++---------------
>>  1 file changed, 21 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt
>> index cdecd04e5b..2c4dee170b 100644
>> --- a/Documentation/config/feature.txt
>> +++ b/Documentation/config/feature.txt
>> @@ -3,24 +3,26 @@ feature.*::
>>  	a group of other config settings. These groups are created by the Git
>>  	developer community as recommended defaults and are subject to change.
>>  	In particular, new config options may be added with different defaults.
>> -
>> -feature.experimental::
>> -	Enable config options that are new to Git, and are being considered for
>> -	future defaults. Config settings included here may be added or removed
>> -	with each release, including minor version updates. These settings may
>> -	have unintended interactions since they are so new. Please enable this
>> -	setting if you are interested in providing feedback on experimental
>> -	features. The new default values are:
>>  +
>> -* `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by
>> -skipping more commits at a time, reducing the number of round trips.
>> +--
>> +	feature.experimental::
>> +		Enable config options that are new to Git, and are being considered for
>> +		future defaults. Config settings included here may be added or removed
>> +		with each release, including minor version updates. These settings may
>> +		have unintended interactions since they are so new. Please enable this
>> +		setting if you are interested in providing feedback on experimental
>> +		features. The new default values are:
>> +	+
>> +	* `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by
>> +	skipping more commits at a time, reducing the number of round trips.
>>  
>> -feature.manyFiles::
>> -	Enable config options that optimize for repos with many files in the
>> -	working directory. With many files, commands such as `git status` and
>> -	`git checkout` may be slow and these new defaults improve performance:
>> -+
>> -* `index.version=4` enables path-prefix compression in the index.
>> -+
>> -* `core.untrackedCache=true` enables the untracked cache. This setting assumes
>> -that mtime is working on your machine.
>> +	feature.manyFiles::
>> +		Enable config options that optimize for repos with many files in the
>> +		working directory. With many files, commands such as `git status` and
>> +		`git checkout` may be slow and these new defaults improve performance:
>> +	+
>> +	* `index.version=4` enables path-prefix compression in the index.
>> +	+
>> +	* `core.untrackedCache=true` enables the untracked cache. This setting assumes
>> +	that mtime is working on your machine.
>> +--
>
> I don't know if/how this helps readability, but this breaks the
> feature.* generation of these variables in config-list.h, see
> generate-configlist.sh.
>
> So if you make this change you need to fix that script as well.

Being a bit more awake I'd prefer not to see this patch go in in its
current form, here's why:

I've got a WIP series I was going to submit soon that:

 1. Ensures that all config variables are discussed canonically in
    git-config(1), e.g. for sendemail.* we now point elsewhere.

 2. Adds a "CONFIGURATION" section to all relevant manpages, which then
    include the relevant section(s) for variables that affect those
    commands.

 3. #2 requires splitting up many of Documentation/config/whatever.txt
    into Documentation/config/whatever/<sub-lists>.txt, notably things
    like Documentation/config/color.txt need to become
    Documentation/config/color/{diff,status,...}.txt so
    Documentation/git-{status,diff,...}.txt can include them.

This only works because we don't have something like your proposed patch
here.

I.e. asciidoc isn't going to take kindly to feature.manyFiles being
indented when I include it in git-{status,checkout}.txt, in its
CONFIGURATION section that'll be either a syntax error, or look out of
place.

But more importantly there seems to just be a better way to do it,
looking at the current config file we have the pattern you're trying to
introduce for advice.* already, except it's differently formatted, i.e.:

    advice.*
        SubVariaBle::

But yours is:

    feature.*
        feature.SubVariaBle::

The advice.* ones also have the config-list.h issue I noted. But in any
case, it seems to me that the issue is that we group these things at the
top-level under git-config(1)'s "CONFIGURATION FILE" section, we should
really split them into a CONFIGURATION VARIABLES, and then have these
advice.* or whatever be a section on the level of the current
"Variables", i.e. something like the incomplete patch below[1].

That would both look nicer IMO, and allow us to not indent things in
Documentation/config/*.txt. If you agree I'd very much like you to pick
up & run with that alternate approach instead, what do you think?

1.

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bf82766a6a2..5e52b0db1fa 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -299,9 +299,8 @@ pathname::
 	is expanded to the value of `$HOME`, and `~user/` to the
 	specified user's home directory.
 
-
-Variables
-~~~~~~~~~
+CONFIGURATION VARIABLES
+-----------------------
 
 Note that this list is non-comprehensive and not necessarily complete.
 For command-specific variables, you will find a more detailed description
@@ -312,10 +311,24 @@ inventing new variables for use in your own tool, make sure their
 names do not conflict with those that are used by Git itself and
 other popular tools, and describe them in your documentation.
 
+
+advice.*
+~~~~~~~~
+
+These variables control various optional help messages designed to aid
+new users. All 'advice.*' variables default to 'true', and you can
+tell Git that you do not need help by setting these to 'false':
+
 include::config/advice.txt[]
 
+core.*
+~~~~~~
+
 include::config/core.txt[]
 
+add.*
+~~~~~
+
 include::config/add.txt[]
 
 include::config/alias.txt[]
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 8b2849ff7b3..35d6b0e20ff 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -1,126 +1,119 @@
-advice.*::
-	These variables control various optional help messages designed to
-	aid new users. All 'advice.*' variables default to 'true', and you
-	can tell Git that you do not need help by setting these to 'false':
-+
---
-	fetchShowForcedUpdates::
-		Advice shown when linkgit:git-fetch[1] takes a long time
-		to calculate forced updates after ref updates, or to warn
-		that the check is disabled.
-	pushUpdateRejected::
-		Set this variable to 'false' if you want to disable
-		'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists',
-		'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
-		simultaneously.
-	pushNonFFCurrent::
-		Advice shown when linkgit:git-push[1] fails due to a
-		non-fast-forward update to the current branch.
-	pushNonFFMatching::
-		Advice shown when you ran linkgit:git-push[1] and pushed
-		'matching refs' explicitly (i.e. you used ':', or
-		specified a refspec that isn't your current branch) and
-		it resulted in a non-fast-forward error.
-	pushAlreadyExists::
-		Shown when linkgit:git-push[1] rejects an update that
-		does not qualify for fast-forwarding (e.g., a tag.)
-	pushFetchFirst::
-		Shown when linkgit:git-push[1] rejects an update that
-		tries to overwrite a remote ref that points at an
-		object we do not have.
-	pushNeedsForce::
-		Shown when linkgit:git-push[1] rejects an update that
-		tries to overwrite a remote ref that points at an
-		object that is not a commit-ish, or make the remote
-		ref point at an object that is not a commit-ish.
-	pushUnqualifiedRefname::
-		Shown when linkgit:git-push[1] gives up trying to
-		guess based on the source and destination refs what
-		remote ref namespace the source belongs in, but where
-		we can still suggest that the user push to either
-		refs/heads/* or refs/tags/* based on the type of the
-		source object.
-	pushRefNeedsUpdate::
-		Shown when linkgit:git-push[1] rejects a forced update of
-		a branch when its remote-tracking ref has updates that we
-		do not have locally.
-	statusAheadBehind::
-		Shown when linkgit:git-status[1] computes the ahead/behind
-		counts for a local ref compared to its remote tracking ref,
-		and that calculation takes longer than expected. Will not
-		appear if `status.aheadBehind` is false or the option
-		`--no-ahead-behind` is given.
-	statusHints::
-		Show directions on how to proceed from the current
-		state in the output of linkgit:git-status[1], in
-		the template shown when writing commit messages in
-		linkgit:git-commit[1], and in the help message shown
-		by linkgit:git-switch[1] or
-		linkgit:git-checkout[1] when switching branch.
-	statusUoption::
-		Advise to consider using the `-u` option to linkgit:git-status[1]
-		when the command takes more than 2 seconds to enumerate untracked
-		files.
-	commitBeforeMerge::
-		Advice shown when linkgit:git-merge[1] refuses to
-		merge to avoid overwriting local changes.
-	resetQuiet::
-		Advice to consider using the `--quiet` option to linkgit:git-reset[1]
-		when the command takes more than 2 seconds to enumerate unstaged
-		changes after reset.
-	resolveConflict::
-		Advice shown by various commands when conflicts
-		prevent the operation from being performed.
-	sequencerInUse::
-		Advice shown when a sequencer command is already in progress.
-	implicitIdentity::
-		Advice on how to set your identity configuration when
-		your information is guessed from the system username and
-		domain name.
-	detachedHead::
-		Advice shown when you used
-		linkgit:git-switch[1] or linkgit:git-checkout[1]
-		to move to the detach HEAD state, to instruct how to
-		create a local branch after the fact.
-	checkoutAmbiguousRemoteBranchName::
-		Advice shown when the argument to
-		linkgit:git-checkout[1] and linkgit:git-switch[1]
-		ambiguously resolves to a
-		remote tracking branch on more than one remote in
-		situations where an unambiguous argument would have
-		otherwise caused a remote-tracking branch to be
-		checked out. See the `checkout.defaultRemote`
-		configuration variable for how to set a given remote
-		to used by default in some situations where this
-		advice would be printed.
-	amWorkDir::
-		Advice that shows the location of the patch file when
-		linkgit:git-am[1] fails to apply it.
-	rmHints::
-		In case of failure in the output of linkgit:git-rm[1],
-		show directions on how to proceed from the current state.
-	addEmbeddedRepo::
-		Advice on what to do when you've accidentally added one
-		git repo inside of another.
-	ignoredHook::
-		Advice shown if a hook is ignored because the hook is not
-		set as executable.
-	waitingForEditor::
-		Print a message to the terminal whenever Git is waiting for
-		editor input from the user.
-	nestedTag::
-		Advice shown if a user attempts to recursively tag a tag object.
-	submoduleAlternateErrorStrategyDie::
-		Advice shown when a submodule.alternateErrorStrategy option
-		configured to "die" causes a fatal error.
-	addIgnoredFile::
-		Advice shown if a user attempts to add an ignored file to
-		the index.
-	addEmptyPathspec::
-		Advice shown if a user runs the add command without providing
-		the pathspec parameter.
-	updateSparsePath::
-		Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1]
-		is asked to update index entries outside the current sparse
-		checkout.
---
+advice.*fetchShowForcedUpdates::
+	Advice shown when linkgit:git-fetch[1] takes a long time
+	to calculate forced updates after ref updates, or to warn
+	that the check is disabled.
+advice.pushUpdateRejected::
+	Set this variable to 'false' if you want to disable
+	'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists',
+	'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
+	simultaneously.
+advice.pushNonFFCurrent::
+	Advice shown when linkgit:git-push[1] fails due to a
+	non-fast-forward update to the current branch.
+advice.pushNonFFMatching::
+	Advice shown when you ran linkgit:git-push[1] and pushed
+	'matching refs' explicitly (i.e. you used ':', or
+	specified a refspec that isn't your current branch) and
+	it resulted in a non-fast-forward error.
+advice.pushAlreadyExists::
+	Shown when linkgit:git-push[1] rejects an update that
+	does not qualify for fast-forwarding (e.g., a tag.)
+pushFetchFirst::
+	Shown when linkgit:git-push[1] rejects an update that
+	tries to overwrite a remote ref that points at an
+	object we do not have.
+pushNeedsForce::
+	Shown when linkgit:git-push[1] rejects an update that
+	tries to overwrite a remote ref that points at an
+	object that is not a commit-ish, or make the remote
+	ref point at an object that is not a commit-ish.
+pushUnqualifiedRefname::
+	Shown when linkgit:git-push[1] gives up trying to
+	guess based on the source and destination refs what
+	remote ref namespace the source belongs in, but where
+	we can still suggest that the user push to either
+	refs/heads/* or refs/tags/* based on the type of the
+	source object.
+pushRefNeedsUpdate::
+	Shown when linkgit:git-push[1] rejects a forced update of
+	a branch when its remote-tracking ref has updates that we
+	do not have locally.
+statusAheadBehind::
+	Shown when linkgit:git-status[1] computes the ahead/behind
+	counts for a local ref compared to its remote tracking ref,
+	and that calculation takes longer than expected. Will not
+	appear if `status.aheadBehind` is false or the option
+	`--no-ahead-behind` is given.
+statusHints::
+	Show directions on how to proceed from the current
+	state in the output of linkgit:git-status[1], in
+	the template shown when writing commit messages in
+	linkgit:git-commit[1], and in the help message shown
+	by linkgit:git-switch[1] or
+	linkgit:git-checkout[1] when switching branch.
+statusUoption::
+	Advise to consider using the `-u` option to linkgit:git-status[1]
+	when the command takes more than 2 seconds to enumerate untracked
+	files.
+commitBeforeMerge::
+	Advice shown when linkgit:git-merge[1] refuses to
+	merge to avoid overwriting local changes.
+resetQuiet::
+	Advice to consider using the `--quiet` option to linkgit:git-reset[1]
+	when the command takes more than 2 seconds to enumerate unstaged
+	changes after reset.
+resolveConflict::
+	Advice shown by various commands when conflicts
+	prevent the operation from being performed.
+sequencerInUse::
+	Advice shown when a sequencer command is already in progress.
+implicitIdentity::
+	Advice on how to set your identity configuration when
+	your information is guessed from the system username and
+	domain name.
+detachedHead::
+	Advice shown when you used
+	linkgit:git-switch[1] or linkgit:git-checkout[1]
+	to move to the detach HEAD state, to instruct how to
+	create a local branch after the fact.
+checkoutAmbiguousRemoteBranchName::
+	Advice shown when the argument to
+	linkgit:git-checkout[1] and linkgit:git-switch[1]
+	ambiguously resolves to a
+	remote tracking branch on more than one remote in
+	situations where an unambiguous argument would have
+	otherwise caused a remote-tracking branch to be
+	checked out. See the `checkout.defaultRemote`
+	configuration variable for how to set a given remote
+	to used by default in some situations where this
+	advice would be printed.
+amWorkDir::
+	Advice that shows the location of the patch file when
+	linkgit:git-am[1] fails to apply it.
+rmHints::
+	In case of failure in the output of linkgit:git-rm[1],
+	show directions on how to proceed from the current state.
+addEmbeddedRepo::
+	Advice on what to do when you've accidentally added one
+	git repo inside of another.
+ignoredHook::
+	Advice shown if a hook is ignored because the hook is not
+	set as executable.
+waitingForEditor::
+	Print a message to the terminal whenever Git is waiting for
+	editor input from the user.
+nestedTag::
+	Advice shown if a user attempts to recursively tag a tag object.
+submoduleAlternateErrorStrategyDie::
+	Advice shown when a submodule.alternateErrorStrategy option
+	configured to "die" causes a fatal error.
+addIgnoredFile::
+	Advice shown if a user attempts to add an ignored file to
+	the index.
+addEmptyPathspec::
+	Advice shown if a user runs the add command without providing
+	the pathspec parameter.
+updateSparsePath::
+	Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1]
+	is asked to update index entries outside the current sparse
+	checkout.