| Message ID | pull.732.git.1599707259907.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ls-files: respect 'submodule.recurse' config | expand |
"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.
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.
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
> 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.
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
> 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.
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.
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
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
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