ls-files: respect 'submodule.recurse' config

From: Philippe Blain <[email protected]>

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.

---
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(-)

index d7a63c8c12..9ba3adbf48 100644
@@ -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

index c8eae899b8..43c7c9bd62 100644
@@ -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");

index 4a08000713..30c2c1e0bd 100755
@@ -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

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.

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.

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

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.

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

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.

Hi Junio,

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).

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 ?

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

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.

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.

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

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.

Danh

Hi Junio,

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).

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 ?

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