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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 10, 2020, 04:25 UTC
Message-ID: <xmqqv9gms1fp.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.732.git.1599707259907.gitgitgadget@gmail.com>

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

Show 11 quoted lines

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