[PATCH v3 0/3] Assorted improvements salvaged from an earlier series

[PATCH v3 0/3] Assorted improvements salvaged from an earlier series

[Dragan Simic]

This series contains patches salvaged from my earlier series, [1] for
which it has been concluded to be not acceptable for merging, because of
possible issues with various git scripts. [2]

Changes introduced to the patches are described separately in each patch.

Link to v1: https://lore.kernel.org/git/cover.1710968761.git.dsimic@manjaro.org/T/#u
Link to v2: https://lore.kernel.org/git/cover.1711302588.git.dsimic@manjaro.org/T/#u

[1] https://lore.kernel.org/git/cover.1710781235.git.dsimic@manjaro.org/T/#u
[2] https://lore.kernel.org/git/d8475579f014a90b27efaf6207bc6fb0@manjaro.org/

Dragan Simic (3):
  grep: perform some minor code and comment cleanups
  grep docs: describe --recurse-submodules further and improve
    formatting a bit
  grep docs: describe --no-index further and improve formatting a bit

 Documentation/config/grep.txt |  2 +-
 Documentation/git-grep.txt    | 36 +++++++++++++++++++++--------------
 builtin/grep.c                | 21 ++++++++------------
 3 files changed, 31 insertions(+), 28 deletions(-)

[PATCH v3 1/3] grep: perform some minor code and comment cleanups

[Dragan Simic]

Move some variable definitions around, and reflow one comment block, to
make the code a bit neater after spotting those slightly unpolished areas.
There are no functional changes to the source code.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---

Notes:
    Changes in v3:
        - No changes were introduced
    
    Changes in v2:
        - No changes were introduced
    
    This patch is salvaged from my earlier series, [1] for which it has been
    concluded to be not acceptable for merging, because of possible issues
    with various git scripts. [2]
    
    Compared to the version in the earlies series, there are no changes in
    this version.  As expected and as already discussed, patches like this one
    inevitably raise a few eyebrows. [3][4][5]
    
    [1] https://lore.kernel.org/git/cover.1710781235.git.dsimic@manjaro.org/T/#u
    [2] https://lore.kernel.org/git/d8475579f014a90b27efaf6207bc6fb0@manjaro.org/
    [3] https://lore.kernel.org/git/CAPig+cQ6Y2oOaPkKFsD41beXLHjhD++nmf59xrcswpb6_Q-sdA@mail.gmail.com/
    [4] https://lore.kernel.org/git/xmqqjzlzt61d.fsf@gitster.g/#t
    [5] https://lore.kernel.org/git/24093dca675c49cfde39f6d6efca2342@manjaro.org/

 builtin/grep.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 982bcfc4b1df..af89c8b5cb19 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -623,13 +623,13 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
 		     struct tree_desc *tree, struct strbuf *base, int tn_len,
 		     int check_attr)
 {
-	struct repository *repo = opt->repo;
-	int hit = 0;
+	int hit = 0, name_base_len = 0;
+	int old_baselen = base->len;
 	enum interesting match = entry_not_interesting;
+	struct repository *repo = opt->repo;
 	struct name_entry entry;
-	int old_baselen = base->len;
 	struct strbuf name = STRBUF_INIT;
-	int name_base_len = 0;
+
 	if (repo->submodule_prefix) {
 		strbuf_addstr(&name, repo->submodule_prefix);
 		name_base_len = name.len;
@@ -890,19 +890,15 @@ static int pattern_callback(const struct option *opt, const char *arg,
 
 int cmd_grep(int argc, const char **argv, const char *prefix)
 {
-	int hit = 0;
+	int hit = 0, seen_dashdash = 0, use_index = 1;
 	int cached = 0, untracked = 0, opt_exclude = -1;
-	int seen_dashdash = 0;
 	int external_grep_allowed__ignored;
+	int i, dummy, allow_revs;
 	const char *show_in_pager = NULL, *default_pager = "dummy";
 	struct grep_opt opt;
 	struct object_array list = OBJECT_ARRAY_INIT;
 	struct pathspec pathspec;
 	struct string_list path_list = STRING_LIST_INIT_DUP;
-	int i;
-	int dummy;
-	int use_index = 1;
-	int allow_revs;
 
 	struct option options[] = {
 		OPT_BOOL(0, "cached", &cached,
@@ -1059,9 +1055,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		recurse_submodules = 0;
 
 	/*
-	 * skip a -- separator; we know it cannot be
-	 * separating revisions from pathnames if
-	 * we haven't even had any patterns yet
+	 * skip a -- separator; we know it cannot be separating revisions
+	 * from pathnames if we haven't even had any patterns yet
 	 */
 	if (argc > 0 && !opt.pattern_list && !strcmp(argv[0], "--")) {
 		argv++;

[PATCH v3 2/3] grep docs: describe --recurse-submodules further and improve formatting a bit

[Dragan Simic]

Clarify that --recurse-submodules cannot be used together with --untracked,
and improve the formatting in a couple of places, to make it visually clear
that those are the commands or the names of configuration options.

While there, change a couple of "<tree>" placeholders to "_<tree>_", to help
with an ongoing translation improvement effort. [1]

[1] https://lore.kernel.org/git/CAPig+cQc8W4JOpB+TMP=czketU1U7wcY_x9bsP5T=3-XjGLhRQ@mail.gmail.com/

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---

Notes:
    Changes in v3:
        - No changes were introduced
    
    Changes in v2:
        - No changes were introduced
    
    This patch is salvaged from my earlier series, [2] for which it has been
    concluded to be not acceptable for merging, because of possible issues
    with various git scripts. [3]
    
    Compared to the version in the earlier series, this version adds some more
    small formatting improvements of the same kind, and also changes a couple
    of "<tree>" placeholders to "_<tree>_", as suggested by Eric Sunshine. [1]
    
    [2] https://lore.kernel.org/git/cover.1710781235.git.dsimic@manjaro.org/T/#u
    [3] https://lore.kernel.org/git/d8475579f014a90b27efaf6207bc6fb0@manjaro.org/

 Documentation/config/grep.txt |  2 +-
 Documentation/git-grep.txt    | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt
index e521f20390ce..10041f27b0c8 100644
--- a/Documentation/config/grep.txt
+++ b/Documentation/config/grep.txt
@@ -24,5 +24,5 @@ grep.fullName::
 	If set to true, enable `--full-name` option by default.
 
 grep.fallbackToNoIndex::
-	If set to true, fall back to git grep --no-index if git grep
+	If set to true, fall back to `git grep --no-index` if `git grep`
 	is executed outside of a git repository.  Defaults to false.
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 0d0103c780af..f64f40e9775a 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -64,9 +64,9 @@ OPTIONS
 --recurse-submodules::
 	Recursively search in each submodule that is active and
 	checked out in the repository.  When used in combination with the
-	<tree> option the prefix of all submodule output will be the name of
-	the parent project's <tree> object. This option has no effect
-	if `--no-index` is given.
+	_<tree>_ option the prefix of all submodule output will be the name of
+	the parent project's _<tree>_ object.  This option cannot be used together
+	with `--untracked`, and it has no effect if `--no-index` is specified.
 
 -a::
 --text::
@@ -178,7 +178,7 @@ providing this option will cause it to die.
 	Use \0 as the delimiter for pathnames in the output, and print
 	them verbatim. Without this option, pathnames with "unusual"
 	characters are quoted as explained for the configuration
-	variable core.quotePath (see linkgit:git-config[1]).
+	variable `core.quotePath` (see linkgit:git-config[1]).
 
 -o::
 --only-matching::
@@ -332,7 +332,7 @@ EXAMPLES
 NOTES ON THREADS
 ----------------
 
-The `--threads` option (and the grep.threads configuration) will be ignored when
+The `--threads` option (and the `grep.threads` configuration) will be ignored when
 `--open-files-in-pager` is used, forcing a single-threaded execution.
 
 When grepping the object store (with `--cached` or giving tree objects), running

[PATCH v3 3/3] grep docs: describe --no-index further and improve formatting a bit

[Dragan Simic]

Improve the description of --no-index, to make it more clear to the users
what this option actually does under the hood, and what's its purpose.
Describe the dependency between --no-index and either of the --cached and
--untracked options, which cannot be used together.

As part of that, shuffle a couple of the options, to make the documentation
flow a bit better, because it makes more sense to describe first the options
that have something in common, and to after that describe an option that does
something differently.  In more detail, --cached and --untracked both leave
git-grep(1) in the usual state, in which it treats the directory as a local
git repository, unlike --no-index that makes git-grep(1) treat the directory
not as a git repository.

While there, improve the descriptions of grep worker threads a bit, to give
them better context.  Adjust the language a bit, to avoid addressing the
reader directly, which is in general preferred in technical documentation,
because it eliminates the possible element of persuading the user to do
something.  In other words, we should be telling the user what our software
can do, instead of telling the user what to do.

Also perform some minor formatting improvements, to make it clear it's the
git commands, command parameters, and configuration option names.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---

Notes:
    Changes in v3:
        - Removed references to speedups caused by multi-threaded searches,
          as pointed out by Junio, [1] because it all depends on the actual
          workloads and the actual system I/O speed and CPU power
        - One of the removed references replaced with a reference to pathspec
          patterns, which is a more suitable example of the benefits
    
    Changes in v2:
        - Improved the patch description a bit, to make it more clear why
          this patch shuffles some of the options around, and why it changes
          some of the wording to passive voice
        - Reworded the description of --no-index a bit, to not mention the
          name of the utility we're describing, which avoids any possible
          confusion, as pointed out by Jean-Noel Avila [2]
    
    This patch is salvaged from my earlier series, [3] for which it has been
    concluded to be not acceptable for merging, because of possible issues
    with various git scripts. [4]
    
    Compared to the version in the earlier series, this version continues
    the effort to improve the description of --no-index, by also incorporating
    the possible improvements pointed out by Junio. [5]  This version also
    improves the wording of some related descriptions, mainly related to
    grep.threads, and performs some additional small formatting improvements.
    
    [1] https://lore.kernel.org/git/xmqqa5mmjf2f.fsf@gitster.g/
    [2] https://lore.kernel.org/git/ed050f2d496a6db07e698fd2f1094b81@manjaro.org/
    [3] https://lore.kernel.org/git/cover.1710781235.git.dsimic@manjaro.org/T/#u
    [4] https://lore.kernel.org/git/d8475579f014a90b27efaf6207bc6fb0@manjaro.org/
    [5] https://lore.kernel.org/git/xmqqwmpzrqfv.fsf@gitster.g/

 Documentation/git-grep.txt | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index f64f40e9775a..1e6d7b65c84e 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -28,7 +28,7 @@ SYNOPSIS
 	   [-f <file>] [-e] <pattern>
 	   [--and|--or|--not|(|)|-e <pattern>...]
 	   [--recurse-submodules] [--parent-basename <basename>]
-	   [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
+	   [ [--[no-]exclude-standard] [--cached | --untracked | --no-index] | <tree>...]
 	   [--] [<pathspec>...]
 
 DESCRIPTION
@@ -45,13 +45,21 @@ OPTIONS
 	Instead of searching tracked files in the working tree, search
 	blobs registered in the index file.
 
---no-index::
-	Search files in the current directory that is not managed by Git.
-
 --untracked::
 	In addition to searching in the tracked files in the working
 	tree, search also in untracked files.
 
+--no-index::
+	Search files in the current directory that is not managed by Git,
+	or by ignoring that the current directory is managed by Git.  This
+	is rather similar to running the regular `grep(1)` utility with its
+	`-r` option specified, but with some additional benefits, such as
+	using pathspec patterns to limit paths;  see the 'pathspec' entry
+	in linkgit:gitglossary[7] for more information.
++
+This option cannot be used together with `--cached` or `--untracked`.
+See also `grep.fallbackToNoIndex` in 'CONFIGURATION' below.
+
 --no-exclude-standard::
 	Also search in ignored files by not honoring the `.gitignore`
 	mechanism. Only useful with `--untracked`.
@@ -248,8 +256,8 @@ providing this option will cause it to die.
 	a non-zero status.
 
 --threads <num>::
-	Number of grep worker threads to use.
-	See `grep.threads` in 'CONFIGURATION' for more information.
+	Number of `grep` worker threads to use.  See 'NOTES ON THREADS'
+	and `grep.threads` in 'CONFIGURATION' for more information.
 
 -f <file>::
 	Read patterns from <file>, one per line.
@@ -336,9 +344,9 @@ The `--threads` option (and the `grep.threads` configuration) will be ignored wh
 `--open-files-in-pager` is used, forcing a single-threaded execution.
 
 When grepping the object store (with `--cached` or giving tree objects), running
-with multiple threads might perform slower than single threaded if `--textconv`
-is given and there are too many text conversions. So if you experience low
-performance in this case, it might be desirable to use `--threads=1`.
+with multiple threads might perform slower than single-threaded if `--textconv`
+is given and there are too many text conversions.  Thus, if low performance is
+experienced in this case, it might be desirable to use `--threads=1`.
 
 CONFIGURATION
 -------------

Re: [PATCH v3 0/3] Assorted improvements salvaged from an earlier series

[Eric Sunshine]

On Mon, Mar 25, 2024 at 4:34 PM Dragan Simic <dsimic@manjaro.org> wrote:
> This series contains patches salvaged from my earlier series, [1] for
> which it has been concluded to be not acceptable for merging, because of
> possible issues with various git scripts. [2]
>
> Changes introduced to the patches are described separately in each patch.

Prose description of the changes between versions is very much welcome
by reviewers, but please also include a range-diff[1] or interdiff[2]
when preparing a reroll.

[1]: git format-patch --range-diff=<prev> ...
[2]: git format-patch --interdiff=<prev> ...

Re: [PATCH v3 0/3] Assorted improvements salvaged from an earlier series

[Dragan Simic]

On 2024-03-25 21:39, Eric Sunshine wrote:
> On Mon, Mar 25, 2024 at 4:34 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> This series contains patches salvaged from my earlier series, [1] for
>> which it has been concluded to be not acceptable for merging, because 
>> of
>> possible issues with various git scripts. [2]
>> 
>> Changes introduced to the patches are described separately in each 
>> patch.
> 
> Prose description of the changes between versions is very much welcome
> by reviewers, but please also include a range-diff[1] or interdiff[2]
> when preparing a reroll.
> 
> [1]: git format-patch --range-diff=<prev> ...
> [2]: git format-patch --interdiff=<prev> ...

Ah, sorry, I didn't forget about that, but I just intended to start
providing range-diffs for future patches.  These were more like new
versions of old patches.

Re: [PATCH v3 0/3] Assorted improvements salvaged from an earlier series

[Eric Sunshine]

On Mon, Mar 25, 2024 at 4:41 PM Dragan Simic <dsimic@manjaro.org> wrote:
> On 2024-03-25 21:39, Eric Sunshine wrote:
> > Prose description of the changes between versions is very much welcome
> > by reviewers, but please also include a range-diff[1] or interdiff[2]
> > when preparing a reroll.
>
> Ah, sorry, I didn't forget about that, but I just intended to start
> providing range-diffs for future patches.  These were more like new
> versions of old patches.

The range-diff is particularly valuable exactly for this case (new
versions of old patches) since it is a "diff" between the old patches
and the new patches. Thus, it provides reviewers with a precise
summary of what changed between, say, v2 and v3 of a series, and
manifests as a concrete representation of what your prose explanation
describes.

Re: [PATCH v3 0/3] Assorted improvements salvaged from an earlier series

[Dragan Simic]

On 2024-03-25 21:49, Eric Sunshine wrote:
> On Mon, Mar 25, 2024 at 4:41 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-03-25 21:39, Eric Sunshine wrote:
>> > Prose description of the changes between versions is very much welcome
>> > by reviewers, but please also include a range-diff[1] or interdiff[2]
>> > when preparing a reroll.
>> 
>> Ah, sorry, I didn't forget about that, but I just intended to start
>> providing range-diffs for future patches.  These were more like new
>> versions of old patches.
> 
> The range-diff is particularly valuable exactly for this case (new
> versions of old patches) since it is a "diff" between the old patches
> and the new patches. Thus, it provides reviewers with a precise
> summary of what changed between, say, v2 and v3 of a series, and
> manifests as a concrete representation of what your prose explanation
> describes.

I see, and I do understand the purpose of range-diffs:  they make
reviewing new patch versions significantly easier, by relieving the
reviewers from the need to track down what has been changed in the
current patch version.

Re: [PATCH v3 0/3] Assorted improvements salvaged from an earlier series

[Dragan Simic]

Hello Eric,

On 2024-03-25 21:54, Dragan Simic wrote:
> On 2024-03-25 21:49, Eric Sunshine wrote:
>> On Mon, Mar 25, 2024 at 4:41 PM Dragan Simic <dsimic@manjaro.org> 
>> wrote:
>>> On 2024-03-25 21:39, Eric Sunshine wrote:
>>> > Prose description of the changes between versions is very much welcome
>>> > by reviewers, but please also include a range-diff[1] or interdiff[2]
>>> > when preparing a reroll.
>>> 
>>> Ah, sorry, I didn't forget about that, but I just intended to start
>>> providing range-diffs for future patches.  These were more like new
>>> versions of old patches.
>> 
>> The range-diff is particularly valuable exactly for this case (new
>> versions of old patches) since it is a "diff" between the old patches
>> and the new patches. Thus, it provides reviewers with a precise
>> summary of what changed between, say, v2 and v3 of a series, and
>> manifests as a concrete representation of what your prose explanation
>> describes.
> 
> I see, and I do understand the purpose of range-diffs:  they make
> reviewing new patch versions significantly easier, by relieving the
> reviewers from the need to track down what has been changed in the
> current patch version.

As a note, I kept my promise :) and have started providing range-diffs
in patch submissions. [1]

[1] 
https://lore.kernel.org/git/e3212c0a4ad331685c68c13afcdbced20982ab32.1712364420.git.dsimic@manjaro.org/T/#u

Re: [PATCH v3 0/3] Assorted improvements salvaged from an earlier series

[Eric Sunshine]

On Fri, Apr 5, 2024 at 8:52 PM Dragan Simic <dsimic@manjaro.org> wrote:
> On 2024-03-25 21:54, Dragan Simic wrote:
> > On 2024-03-25 21:49, Eric Sunshine wrote:
> >> The range-diff is particularly valuable exactly for this case (new
> >> versions of old patches) since it is a "diff" between the old patches
> >> and the new patches. Thus, it provides reviewers with a precise
> >> summary of what changed between, say, v2 and v3 of a series, and
> >> manifests as a concrete representation of what your prose explanation
> >> describes.
> >
> > I see, and I do understand the purpose of range-diffs:  they make
> > reviewing new patch versions significantly easier, by relieving the
> > reviewers from the need to track down what has been changed in the
> > current patch version.
>
> As a note, I kept my promise :) and have started providing range-diffs
> in patch submissions. [1]

Glad to hear. Thanks.