[PATCH] ls-files: respect 'submodule.recurse' config

[PATCH] ls-files: respect 'submodule.recurse' config

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

`git ls-files` learned to recurse into submodules when given
'--recurse-submodules' back in e77aa336f1 (ls-files: optionally recurse
into submodules, 2016-10-07) but it does not respect the
'submodule.recurse' config option which was later introduced in
046b48239e (Introduce 'submodule.recurse' option for worktree
manipulators, 2017-05-31).

Add a 'git_ls_files_config' function to read this configuration
variable, and adjust the documentation and tests accordingly.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
    ls-files: respect 'submodule.recurse' config
    
    This follows the approach of 121e43fa53 (pull: honor submodule.recurse
    config option, 2017-09-06)

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-732%2Fphil-blain%2Fls-files-submodule.recurse-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-732/phil-blain/ls-files-submodule.recurse-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/732

 Documentation/config/submodule.txt     |  4 +--
 builtin/ls-files.c                     | 16 ++++++++++-
 t/t3007-ls-files-recurse-submodules.sh | 37 ++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
index d7a63c8c12..9ba3adbf48 100644
--- a/Documentation/config/submodule.txt
+++ b/Documentation/config/submodule.txt
@@ -60,8 +60,8 @@ submodule.active::
 submodule.recurse::
 	Specifies if commands recurse into submodules by default. This
 	applies to all commands that have a `--recurse-submodules` option
-	(`checkout`, `fetch`, `grep`, `pull`, `push`, `read-tree`, `reset`,
-	`restore` and `switch`) except `clone` and `ls-files`.
+	(`checkout`, `fetch`, `grep`, `ls-files`, `pull`, `push`, `read-tree`,
+	`reset`, `restore` and `switch`) except `clone` .
 	Defaults to false.
 	When set to true, it can be deactivated via the
 	`--no-recurse-submodules` option. Note that some Git commands
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index c8eae899b8..43c7c9bd62 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -512,6 +512,20 @@ static int option_parse_exclude_standard(const struct option *opt,
 	return 0;
 }
 
+/**
+ * Read config variables.
+ */
+static int git_ls_files_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "submodule.recurse")) {
+		recurse_submodules = git_config_bool(var, value) ?
+			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
+		return 0;
+	}
+
+	return git_default_config(var, value, cb);
+}
+
 int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 {
 	int require_work_tree = 0, show_tag = 0, i;
@@ -588,7 +602,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	prefix = cmd_prefix;
 	if (prefix)
 		prefix_len = strlen(prefix);
-	git_config(git_default_config, NULL);
+	git_config(git_ls_files_config, NULL);
 
 	if (repo_read_index(the_repository) < 0)
 		die("index file corrupt");
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index 4a08000713..30c2c1e0bd 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -34,6 +34,43 @@ test_expect_success 'ls-files correctly outputs files in submodule' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ls-files respects submodule.recurse' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git -c submodule.recurse ls-files >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--[no-]recurse-submodule and submodule.recurse' '
+	cat >expect-recurse <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	cat >expect-no-recurse <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule
+	EOF
+
+	git -c submodule.recurse ls-files --no-recurse-submodules >actual &&
+	test_cmp expect-no-recurse actual &&
+	git -c submodule.recurse=false ls-files --recurse-submodules >actual &&
+	test_cmp expect-recurse actual &&
+	git -c submodule.recurse=false ls-files --no-recurse-submodules >actual &&
+	test_cmp expect-no-recurse actual &&
+	git -c submodule.recurse ls-files --recurse-submodules >actual &&
+	test_cmp expect-recurse actual
+'
+
 test_expect_success 'ls-files correctly outputs files in submodule with -z' '
 	lf_to_nul >expect <<-\EOF &&
 	.gitmodules

base-commit: e19713638985533ce461db072b49112da5bd2042
-- 
gitgitgadget

Re: [PATCH] ls-files: respect 'submodule.recurse' config

[Junio C Hamano]

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

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> `git ls-files` learned to recurse into submodules when given
> '--recurse-submodules' back in e77aa336f1 (ls-files: optionally recurse
> into submodules, 2016-10-07) but it does not respect the
> 'submodule.recurse' config option which was later introduced in
> 046b48239e (Introduce 'submodule.recurse' option for worktree
> manipulators, 2017-05-31).
>
> Add a 'git_ls_files_config' function to read this configuration
> variable, and adjust the documentation and tests accordingly.

I am afraid that this will break existing scripts big time, and I
would not be surprised if 046b48239e refrained to do the equivalent
of this patch very much on purpose to avoid such a regression.

Anybody who writes a script using ls-files _without_ passing the
--recurse-submodules option expects that the ls-files command will
stop at submodule boundary without recursing, that the script can
notice and pick up mode 160000 entries in the output from ls-files,
and that the script can decide if it wants to descend into
submodules it discovered that way.

It is easy to imagine that such a script will break badly when run
by a user who has the configuration variable set, I would think.

It also is easy to imagine a script derived from "git-submodule.sh"
back from the time before we started rewriting pieces of it in C.
The main "discover and list the immediate submodules we have" code
was ls-files piped to a loop that picks up entries with mode 160000,
and it was used to drive all the "git submodule" subcommands.  As
the scripted version was not the world's greatest code, it is quite
plausible that somebody forked and improved it, without feeding the
changes back to us.  Such a script is also a candidate to be broken
with this patch.

So, no.  I am not enthused to see this change.

Re: [PATCH] ls-files: respect 'submodule.recurse' config

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> So, no.  I am not enthused to see this change.

A more relevant question to ask you is what the motivation behind
this change is.  

If it is an apparent "inconsistency" that the plumbing command in
question takes an explicit command line option but ignores user
configuration, then we can stop there---I think we would want to
keep it that way.

But if it is because "I use this command interactively quite often,
and I find it inconvenient because I need to type the long command
line option", we may want to step back and understand why you need
to run the low level plumbing command in your interactive use case.
Perhaps most, but not quite all, of your need, whatever it is, is
already satisfied by higher level commands (like "status"?) and what
we need is to enrich these end-user facing higher level commands to
fill the "gap" to satisfy the need in your use case.

Re: [PATCH] ls-files: respect 'submodule.recurse' config

[Philippe Blain]

> Le 10 sept. 2020 à 00:50, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> So, no.  I am not enthused to see this change.
> 
> A more relevant question to ask you is what the motivation behind
> this change is.  
> 
> If it is an apparent "inconsistency" that the plumbing command in
> question takes an explicit command line option but ignores user
> configuration, then we can stop there---I think we would want to
> keep it that way.

Yes, in part this (and as I said in my previous email, to bring the code in
line with 'gitsubmodules(7)').

> But if it is because "I use this command interactively quite often,
> and I find it inconvenient because I need to type the long command
> line option", we may want to step back and understand why you need
> to run the low level plumbing command in your interactive use case.
> Perhaps most, but not quite all, of your need, whatever it is, is
> already satisfied by higher level commands (like "status"?) and what
> we need is to enrich these end-user facing higher level commands to
> fill the "gap" to satisfy the need in your use case.

Yes, I use ls-files interactively quite often. For example, I know a source file
is named 'such_and_such' but I don't remember where in the directory hierarchy
of the project it is located, so `git ls-files **/such_and_such` is the quickest way to 
find it (and I have to then add '--recurse-submodules' when I get no results, remember
that the file in in a submodule, and rerun the command). So I use it mostly
as a Git-aware find(1) replacement.

Re: [PATCH] ls-files: respect 'submodule.recurse' config

[Philippe Blain]

> Le 10 sept. 2020 à 00:50, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> So, no.  I am not enthused to see this change.
> 
> A more relevant question to ask you is what the motivation behind
> this change is.  
> 
> If it is an apparent "inconsistency" that the plumbing command in
> question takes an explicit command line option but ignores user
> configuration, then we can stop there---I think we would want to
> keep it that way.

Yes, in part this (and as I said in my previous email, to bring the code in
line with 'gitsubmodules(7)').

> But if it is because "I use this command interactively quite often,
> and I find it inconvenient because I need to type the long command
> line option", we may want to step back and understand why you need
> to run the low level plumbing command in your interactive use case.
> Perhaps most, but not quite all, of your need, whatever it is, is
> already satisfied by higher level commands (like "status"?) and what
> we need is to enrich these end-user facing higher level commands to
> fill the "gap" to satisfy the need in your use case.

Yes, I use ls-files interactively quite often. For example, I know a source file
is named 'such_and_such' but I don't remember where in the directory hierarchy
of the project it is located, so `git ls-files **/such_and_such` is the quickest way to 
find it (and I have to then add '--recurse-submodules' when I get no results, remember
that the file in in a submodule, and rerun the command). So I use it mostly
as a Git-aware find(1) replacement.

Re: [PATCH] ls-files: respect 'submodule.recurse' config

[Philippe Blain]

Hi Junio,

> Le 10 sept. 2020 à 00:25, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>> 
>> `git ls-files` learned to recurse into submodules when given
>> '--recurse-submodules' back in e77aa336f1 (ls-files: optionally recurse
>> into submodules, 2016-10-07) but it does not respect the
>> 'submodule.recurse' config option which was later introduced in
>> 046b48239e (Introduce 'submodule.recurse' option for worktree
>> manipulators, 2017-05-31).
>> 
>> Add a 'git_ls_files_config' function to read this configuration
>> variable, and adjust the documentation and tests accordingly.
> 
> I am afraid that this will break existing scripts big time, and I
> would not be surprised if 046b48239e refrained to do the equivalent
> of this patch very much on purpose to avoid such a regression.

As I read it, 046b48239e was just the introduction of the config and
it's implementation for read-tree/checkout/reset, and the other commands
with '--recurse-submodules' would come later (as was done in the following 
commits in branch 'sb/submodule-blanket-recursive').

Also, in gitsubmodules(7) [1], 'ls-files' is used as if it respects 'submodule.recurse',
so I think that was Stefan's original plan. (It's been that way since the introduction
of that man page in d48034551a).

> Anybody who writes a script using ls-files _without_ passing the
> --recurse-submodules option expects that the ls-files command will
> stop at submodule boundary without recursing, that the script can
> notice and pick up mode 160000 entries in the output from ls-files,
> and that the script can decide if it wants to descend into
> submodules it discovered that way.
> 
> It is easy to imagine that such a script will break badly when run
> by a user who has the configuration variable set, I would think.

I understand, but I would argue that such a user could easily adapt their
script to add '--no-recurse-submodules' to their ls-files invocation if that 
is the case, no ?

> So, no.  I am not enthused to see this change.

OK, if I'm not able to change your mind, what would you think of a separate
config variable then, say `ls-files.recurseSubmodules` ? This would be more granular,
so less chance of breaking existing scripts, but still provide for a way to configure 
Git to always recurse in submodules, including for 'ls-files'...

Thanks,
Philippe.


[1] https://git-scm.com/docs/gitsubmodules#_workflow_for_an_artificially_split_repo

Re: [PATCH] ls-files: respect 'submodule.recurse' config

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> I understand, but I would argue that such a user could easily adapt their
> script to add '--no-recurse-submodules' to their ls-files invocation if that 
> is the case, no ?

It would have been quite a different story if we were designing
"ls-files" and adding support for "--[no-]recurse-submodules" and
"submodule.recurse" to the command at the same time.  To those who
write scripts with "ls-files" and complain that the command behaves
differently depending on the configuration, you can legitimately say
"you can use --no-recurse-submodules and you are fine" in that case.

But not after all these years.  The same statement becomes "even if
I broke the command, users could work around the breakage I caused".
That is nothing more than a lame excuse that does not explay why you
think you have the right to break their script in the first place.

So, no, I am not enthused to see this change.  Regardless of which
configuration variable affects the feature.  For those who wrote and
use scripts that run ls-files, it is a regression to invite unneeded
complaints from their end-users who suddenly see the breakage in the
scripts.

Re: [PATCH] ls-files: respect 'submodule.recurse' config

[Đoàn Trần Công Danh]

On 2020-09-11 09:05:42-0400, Philippe Blain <levraiphilippeblain@gmail.com> wrote:
> I understand, but I would argue that such a user could easily adapt their
> script to add '--no-recurse-submodules' to their ls-files invocation if that 
> is the case, no ?

There're people still live in the past, for example those poor souls
still live in Ubuntu 16.04 LTS, they don't have the luxury of using
Git with ls-files that understand --recurse-submodules and
--no-recurse-submodules.

> > So, no.  I am not enthused to see this change.
> 
> OK, if I'm not able to change your mind, what would you think of a separate
> config variable then, say `ls-files.recurseSubmodules` ? This would be more granular,
> so less chance of breaking existing scripts, but still provide for a way to configure 
> Git to always recurse in submodules, including for 'ls-files'...

If you're really buy into configuration and using ls-files
interactively, I think it's better to make a Git-alias instead.

	alias.ls = ls-files --recurse-submodules

-- 
Danh

Re: [PATCH] ls-files: respect 'submodule.recurse' config

[Đoàn Trần Công Danh]

On 2020-09-13 17:47:03+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> On 2020-09-11 09:05:42-0400, Philippe Blain <levraiphilippeblain@gmail.com> wrote:
> > I understand, but I would argue that such a user could easily adapt their
> > script to add '--no-recurse-submodules' to their ls-files invocation if that 
> > is the case, no ?
> 
> There're people still live in the past, for example those poor souls
> still live in Ubuntu 16.04 LTS, they don't have the luxury of using
> Git with ls-files that understand --recurse-submodules and
> --no-recurse-submodules.

For this statement, I meant:

: those poor souls that needs to write script for both old and new machine.

Sorry for missing that information, and this noise.

> > > So, no.  I am not enthused to see this change.
> > 
> > OK, if I'm not able to change your mind, what would you think of a separate
> > config variable then, say `ls-files.recurseSubmodules` ? This would be more granular,
> > so less chance of breaking existing scripts, but still provide for a way to configure 
> > Git to always recurse in submodules, including for 'ls-files'...
> 
> If you're really buy into configuration and using ls-files
> interactively, I think it's better to make a Git-alias instead.
> 
> 	alias.ls = ls-files --recurse-submodules

-- 
Danh

Re: [PATCH] ls-files: respect 'submodule.recurse' config

[Philippe Blain]

Hi Junio,

> Le 10 sept. 2020 à 00:25, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>> 
>> `git ls-files` learned to recurse into submodules when given
>> '--recurse-submodules' back in e77aa336f1 (ls-files: optionally recurse
>> into submodules, 2016-10-07) but it does not respect the
>> 'submodule.recurse' config option which was later introduced in
>> 046b48239e (Introduce 'submodule.recurse' option for worktree
>> manipulators, 2017-05-31).
>> 
>> Add a 'git_ls_files_config' function to read this configuration
>> variable, and adjust the documentation and tests accordingly.
> 
> I am afraid that this will break existing scripts big time, and I
> would not be surprised if 046b48239e refrained to do the equivalent
> of this patch very much on purpose to avoid such a regression.

As I read it, 046b48239e was just the introduction of the config and
it's implementation for read-tree/checkout/reset, and the other commands
with '--recurse-submodules' would come later (as was done in the following 
commits in branch 'sb/submodule-blanket-recursive').

Also, in gitsubmodules(7) [1], 'ls-files' is used as if it respects 'submodule.recurse',
so I think that was Stefan's original plan. (It's been that way since the introduction
of that man page in d48034551a).

> Anybody who writes a script using ls-files _without_ passing the
> --recurse-submodules option expects that the ls-files command will
> stop at submodule boundary without recursing, that the script can
> notice and pick up mode 160000 entries in the output from ls-files,
> and that the script can decide if it wants to descend into
> submodules it discovered that way.
> 
> It is easy to imagine that such a script will break badly when run
> by a user who has the configuration variable set, I would think.

I understand, but I would argue that such a user could easily adapt their
script to add '--no-recurse-submodules' to their ls-files invocation if that 
is the case, no ?

> So, no.  I am not enthused to see this change.

OK, if I'm not able to change your mind, what would you think of a separate
config variable then, say `ls-files.recurseSubmodules` ? This would be more granular,
so less chance of breaking existing scripts, but still provide for a way to configure 
Git to always recurse in submodules, including for 'ls-files'...

Thanks,
Philippe.


[1] https://git-scm.com/docs/gitsubmodules#_workflow_for_an_artificially_split_repo