Re: [PATCH 1/6] doc: log, gitk: move '-L' description to 'line-range-options.txt'

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 29, 2020, 20:16 UTC
Message-ID: <xmqqft5w7poo.fsf@gitster.c.googlers.com>
In-Reply-To: <96f6f95abcbd79d432073cb294ba12b71300580f.1603889270.git.gitgitgadget@gmail.com>

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> The description of the '-L' option for `git log` and `gitk` is the
> same, but is repeated in both 'git-log.txt' and 'gitk.txt'.

They are moral equivalents but differ slightly, both in an insignificant way (i.e. SP after -L and its parameters) and a significant way (i.e. Note about gitk usage), so "the same, but is repeated" is not sufficient to explain why the contents of the new file looks the way it is.

> Remove the duplication by creating a new file, 'line-range-options.txt',
> and include it in both files.

Makes sense. As to the conditional, I actually think the version with SP after -L do not have to be listed and instead to show just the "stuck" form as the standardised way. If the option takes an optional value, you must have to use the "stuck" form anyway, and showing that you _could_ have SP there unnecessarily throws extra bytes at the reader with no real gain.

Show 20 quoted lines

> diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
> index c653ebb6a8..761b764c18 100644
> --- a/Documentation/gitk.txt
> +++ b/Documentation/gitk.txt
> @@ -98,25 +98,8 @@ linkgit:git-rev-list[1] for a complete list.
>  	(See "History simplification" in linkgit:git-log[1] for a more
>  	detailed explanation.)
>  
> --L<start>,<end>:<file>::
> --L:<funcname>:<file>::
> ...
> -	`--name-only`, `--name-status`, `--check`) are not currently implemented.
> -+
> -*Note:* gitk (unlike linkgit:git-log[1]) currently only understands
> -this option if you specify it "glued together" with its argument.  Do
> -*not* put a space after `-L`.
> -+
> -include::line-range-format.txt[]
> +:gitk: 1
> +include::line-range-options.txt[]

It looked puzzling to see the inclusion of line-range-format is lost from here, but it turns out to be OK after all when we look at the new line-range-options file.

Show 14 quoted lines

> diff --git a/Documentation/line-range-options.txt b/Documentation/line-range-options.txt
> new file mode 100644
> index 0000000000..9e3d98d44f
> --- /dev/null
> +++ b/Documentation/line-range-options.txt
> @@ -0,0 +1,27 @@
> +ifdef::git-log[]
> +-L <start>,<end>:<file>::
> +-L :<funcname>:<file>::
> +endif::git-log[]
> +ifdef::gitk[]
> +-L<start>,<end>:<file>::
> +-L:<funcname>:<file>::
> +endif::gitk[]

As I said, my vote goes to reducing the variations and using only the latter form for simplicity.

Show 10 quoted lines

> +	Trace the evolution of the line range given by "<start>,<end>"
> +	(or the function name regex <funcname>) within the <file>.  You may
> +	not give any pathspec limiters.  This is currently limited to
> +	a walk starting from a single revision, i.e., you may only
> +	give zero or one positive revision arguments, and
> +	<start> and <end> (or <funcname>) must exist in the starting revision.
> +	You can specify this option more than once. Implies `--patch`.
> +	Patch output can be suppressed using `--no-patch`, but other diff formats
> +	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
> +	`--name-only`, `--name-status`, `--check`) are not currently implemented.

This text is the same as both originals, which is good.

Show 6 quoted lines

> ++
> +ifdef::gitk[]
> +*Note:* gitk (unlike linkgit:git-log[1]) currently only understands
> +this option if you specify it "glued together" with its argument.  Do
> +*not* put a space after `-L`.
> +endif::gitk[]

I think it may make sense to keep this part, mostly from "keep the result as close to the original as possible while refactoring" principle, but if we only teach the "stuck" form in "git log", it becomes irrelevant---one fewer thing the users need to learn and remember, it becomes easier for them to concentrate on what really matters.

> +include::line-range-format.txt[]

And the answer to my earlier puzzlement is here. I actually would have expected that it would be the matter of moving the common text you moved from gitk/log documentation to this file instead to the top of the line-range-format.txt file, but the latter is included also by blame-options.txt, and the description of the option needs to be somewhat different between blame and log anyway, so I think this is the best we can do at this step in the series.

Looking good so far...