| Message ID | pull.1130.git.1643195729608.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scalar: accept -C and -c options before the subcommand | expand |
On Wed, Jan 26, 2022 at 11:15:29AM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > The `git` executable has these two very useful options: > > -C <directory>: > switch to the specified directory before performing any actions > > -c <key>=<value>: > temporarily configure this setting for the duration of the > specified scalar subcommand > > With this commit, we teach the `scalar` executable the same trick. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > scalar: accept -C and -c options > > This makes the scalar command a bit more handy by offering the same -c > <key>=<value> and -C <directory> options as the git command. > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1130%2Fdscho%2Fscalar-c-and-C-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1130/dscho/scalar-c-and-C-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1130 > > contrib/scalar/scalar.c | 22 +++++++++++++++++++++- > contrib/scalar/scalar.txt | 10 ++++++++++ > 2 files changed, 31 insertions(+), 1 deletion(-) > > diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c > index 1ce9c2b00e8..7db2a97416e 100644 > --- a/contrib/scalar/scalar.c > +++ b/contrib/scalar/scalar.c > @@ -808,6 +808,25 @@ int cmd_main(int argc, const char **argv) > struct strbuf scalar_usage = STRBUF_INIT; > int i; > > + while (argc > 1 && *argv[1] == '-') { > + if (!strcmp(argv[1], "-C")) { > + if (argc < 3) > + die(_("-C requires a <directory>")); > + if (chdir(argv[2]) < 0) > + die_errno(_("could not change to '%s'"), > + argv[2]); > + argc -= 2; > + argv += 2; > + } else if (!strcmp(argv[1], "-c")) { > + if (argc < 3) > + die(_("-c requires a <key>=<value> argument")); > + git_config_push_parameter(argv[2]); > + argc -= 2; > + argv += 2; > + } else > + break; > + } > + All looks right to me based on a cursory scan. It's too bad that we have to copy this code from git.c::handle_options(). Could we call handle_options() (assuming that it was available to Scalar's compilation unit) instead? I'm not sure if that's a naive question or not, but it might be nice to explain it out in the commit message in case other reviewers have the same question that I did. On a more practical note: is there an easy way to test this? Like I said, I'm not necessarily worried about the code you wrote, just that it seems like this sort of thing *should* be easy enough to test. It's fine if there isn't, too, but again as somebody new to this area some explanation would be helpful. Thanks, Taylor
On Wed, Jan 26 2022, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > The `git` executable has these two very useful options: > > -C <directory>: > switch to the specified directory before performing any actions > > -c <key>=<value>: > temporarily configure this setting for the duration of the > specified scalar subcommand > > With this commit, we teach the `scalar` executable the same trick. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > scalar: accept -C and -c options > > This makes the scalar command a bit more handy by offering the same -c > <key>=<value> and -C <directory> options as the git command. > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1130%2Fdscho%2Fscalar-c-and-C-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1130/dscho/scalar-c-and-C-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1130 I think it would help for context to note that this patch had at least 6 submissions on the ML already as part of early versions of the scalar series. Here's the CL of the iteration that ejected it: https://lore.kernel.org/git/pull.1005.v7.git.1637158762.gitgitgadget@gmail.com/ Where you summarized: * The patch that adds support for -c <key>=<value> and -C <directory> was moved to its own add-on patch series: While it is obvious that those options are valuable to have, an open question is whether there are other "pre-command" options in git that would be useful, too, and I would like to postpone that discussion to that date. Having been involved in those discussions I can't remember what the pre-command options you're referring to there are, but it seems "that date" is probably upon us.
On 1/26/2022 9:55 PM, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Jan 26 2022, Johannes Schindelin via GitGitGadget wrote: > >> From: Johannes Schindelin <johannes.schindelin@gmx.de> >> >> The `git` executable has these two very useful options: >> >> -C <directory>: >> switch to the specified directory before performing any actions >> >> -c <key>=<value>: >> temporarily configure this setting for the duration of the >> specified scalar subcommand >> >> With this commit, we teach the `scalar` executable the same trick. >> >> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >> --- >> scalar: accept -C and -c options >> >> This makes the scalar command a bit more handy by offering the same -c >> <key>=<value> and -C <directory> options as the git command. >> >> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1130%2Fdscho%2Fscalar-c-and-C-v1 >> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1130/dscho/scalar-c-and-C-v1 >> Pull-Request: https://github.com/gitgitgadget/git/pull/1130 > > I think it would help for context to note that this patch had at least 6 > submissions on the ML already as part of early versions of the scalar > series. > > Here's the CL of the iteration that ejected it: > https://lore.kernel.org/git/pull.1005.v7.git.1637158762.gitgitgadget@gmail.com/ > > Where you summarized: > > * The patch that adds support for -c <key>=<value> and -C <directory> was > moved to its own add-on patch series: While it is obvious that those > options are valuable to have, an open question is whether there are other > "pre-command" options in git that would be useful, too, and I would like > to postpone that discussion to that date. > > Having been involved in those discussions I can't remember what the > pre-command options you're referring to there are, but it seems "that > date" is probably upon us. My understanding was that this was ejected so we could find the right time to lib-ify handle_options() (as Taylor suggested), but we didn't want to do that while Scalar was still in a tentative state (in contrib/ with a plan to move it out after more is implemented). The biggest benefits of using handle_options() is for other pre-command options such as --exec-path, which I use on a regular basis when testing new functionality. There are other options in handle_options() that might not be appropriate, or might be incorrect if we just make handle_options() non-static. For example, `scalar --list-cmds=parseopt` wouldn't show the scalar commands and would instead show the git commands. So my feeling is that we should continue to delay this functionality until Scalar is more stable, perhaps even until after it moves out of contrib/. The need to change handle_options() to work with a new top-level command is novel enough to be worth careful scrutiny, but that effort is only valuable if the Git community is more committed to having Scalar in the tree for the long term. Thanks, -Stolee
Hi Stolee, On Thu, 27 Jan 2022, Derrick Stolee wrote: > The biggest benefits of using handle_options() is for other pre-command > options such as --exec-path, which I use on a regular basis when testing > new functionality. > > There are other options in handle_options() that might not be > appropriate, or might be incorrect if we just make handle_options() > non-static. For example, `scalar --list-cmds=parseopt` wouldn't show the > scalar commands and would instead show the git commands. Right, and since `handle_options()` lives in the same file as `git`'s `cmd_main()` function, we would not only have to disentangle options that work only for `git` from those that would also work for `scalar`, but we would have to extract the `handle_options()` function into a separate file. And while at it, a tangent someone with infinite time on their hands might suggest is: why not convert `handle_options()` to the `parse_options()` machinery? Which would of course solve one issue by adding several new ones. Don't get me wrong: I would find it useful to convert `git.c:handle_options()` to a function in `libgit.a` which uses the `parse_options()` machinery. It'll just require a lot of time, and I do not see enough benefit that would make it worth embarking on that particular journey. But since I had a look at `handle_options()` anyway, I might just as well summarize my insights about how applicable the supported options are for `scalar` here: # Beneficial -c <key>=<value> --config-env <key>=<value> -C <directory> Since I added support for these (except for the long form `--config-env` that I actually only learned while researching this email), it is obvious that I'd like `scalar` to support them. # Won't hurt --html-path --man-path --info-path Sure, for `scalar help` (which I implement in a patch series I have not yet formally contributed to the Git mailing list), these options might even make some sense. --paginate --no-pager There are no Scalar commands that would benefit from using the pager. But if we parse those options, we also have to handle them. Which would mean extracting _even more_ code from `git.c` just so that we can reuse `handle_options()` in Scalar. --no-replace-objects --git-dir --work-tree These options would only be relevant to the `scalar run` command, no other Scalar command works on an existing Git worktree. And even for that command, I doubt that they are actually useful in Scalar's context. --namespace This option seems relevant mostly for repositories that are served up for cloning and fetching, which is not what the Scalar command supports directly. --super-prefix --bare These options do not make sense in Scalar's context (it's neither about bare repositories not about submodules). That is the case for most `git` commands, too, of course. --literal-pathspecs --no-literal-pathspecs --glob-pathspecs --noglob-pathspecs --icase-pathspecs --no-optional-locks None of Scalar's commands take pathspecs, so these options won't have any effect. --shallow-file This option (which I learned about today) is purposefully not documented, as it is merely an internal option for use e.g. by `receive-pack`. # Detrimental --exec-path Since `scalar` is tightly coupled to a specific Git version, it would cause much more harm than benefit to encourage users to use a different Git version by offering them this option. --list-cmds As you pointed out, this option would produce misleading output. Given that only the `-c` and `-C` options are _actually_ useful in the context of the `scalar` command, I would argue that I chose the best approach, as the benefit of the intrusive refactorings that would be necessary to share code with `git.c` is rather small compared with the amount of work. > So my feeling is that we should continue to delay this functionality > until Scalar is more stable, perhaps even until after it moves out of > contrib/. The need to change handle_options() to work with a new > top-level command is novel enough to be worth careful scrutiny, but that > effort is only valuable if the Git community is more committed to having > Scalar in the tree for the long term. I am okay with holding off with this, for now. On the other hand, as I pointed out above: I do not really see it worth the effort to refactor `git.c:handle_options()` for the minimal benefit it would give us over the approach I chose in the patch under discussion. Ciao, Dscho
Hi Taylor, On Wed, 26 Jan 2022, Taylor Blau wrote: > On Wed, Jan 26, 2022 at 11:15:29AM +0000, Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > The `git` executable has these two very useful options: > > > > -C <directory>: > > switch to the specified directory before performing any actions > > > > -c <key>=<value>: > > temporarily configure this setting for the duration of the > > specified scalar subcommand > > > > With this commit, we teach the `scalar` executable the same trick. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > scalar: accept -C and -c options > > > > This makes the scalar command a bit more handy by offering the same -c > > <key>=<value> and -C <directory> options as the git command. > > > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1130%2Fdscho%2Fscalar-c-and-C-v1 > > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1130/dscho/scalar-c-and-C-v1 > > Pull-Request: https://github.com/gitgitgadget/git/pull/1130 > > > > contrib/scalar/scalar.c | 22 +++++++++++++++++++++- > > contrib/scalar/scalar.txt | 10 ++++++++++ > > 2 files changed, 31 insertions(+), 1 deletion(-) > > > > diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c > > index 1ce9c2b00e8..7db2a97416e 100644 > > --- a/contrib/scalar/scalar.c > > +++ b/contrib/scalar/scalar.c > > @@ -808,6 +808,25 @@ int cmd_main(int argc, const char **argv) > > struct strbuf scalar_usage = STRBUF_INIT; > > int i; > > > > + while (argc > 1 && *argv[1] == '-') { > > + if (!strcmp(argv[1], "-C")) { > > + if (argc < 3) > > + die(_("-C requires a <directory>")); > > + if (chdir(argv[2]) < 0) > > + die_errno(_("could not change to '%s'"), > > + argv[2]); > > + argc -= 2; > > + argv += 2; > > + } else if (!strcmp(argv[1], "-c")) { > > + if (argc < 3) > > + die(_("-c requires a <key>=<value> argument")); > > + git_config_push_parameter(argv[2]); > > + argc -= 2; > > + argv += 2; > > + } else > > + break; > > + } > > + > > All looks right to me based on a cursory scan. It's too bad that we have > to copy this code from git.c::handle_options(). It's only a dozen lines, though, and they are pretty stable, so I doubt that we risk divergent copied code. > Could we call handle_options() (assuming that it was available to > Scalar's compilation unit) instead? I'm not sure if that's a naive > question or not, but it might be nice to explain it out in the commit > message in case other reviewers have the same question that I did. I just responded to Stolee elsewhere in this thread with a lengthy analysis of the options, and the conclusion that it would not be worth the effort to refactor `handle_options()`. > On a more practical note: is there an easy way to test this? It would be pretty easy to test `-C`: git init sub && scalar -C sub register && [... verify that `sub/` is now a Scalar repository ...] For `-c`, we would need to configure something parsed by `git_default_config()` that would influence what `scalar register` does, then verify that. Or even better, use a config setting that is in the "Optional" section of `set_recommended_config()`, i.e. it will refuse to override an already-configured value. Something like `status.aheadBehind`. I added this: -- snip -- diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh index 2e1502ad45e..89781568f43 100755 --- a/contrib/scalar/t/t9099-scalar.sh +++ b/contrib/scalar/t/t9099-scalar.sh @@ -85,4 +85,12 @@ test_expect_success 'scalar delete with enlistment' ' test_path_is_missing cloned ' +test_expect_success 'scalar supports -c/-C' ' + test_when_finished "scalar delete sub" && + git init sub && + scalar -C sub -c status.aheadBehind=bogus register && + test -z "$(git -C sub config --local status.aheadBehind)" && + test true = "$(git -C sub config core.preloadIndex)" +' + test_done -- snap -- Ciao, Dscho
Derrick Stolee <stolee@gmail.com> writes: > My understanding was that this was ejected so we could find the right time > to lib-ify handle_options() (as Taylor suggested), but we didn't want to do > that while Scalar was still in a tentative state (in contrib/ with a plan > to move it out after more is implemented). > > The biggest benefits of using handle_options() is for other pre-command > options such as --exec-path, which I use on a regular basis when testing > new functionality. > > There are other options in handle_options() that might not be appropriate, > or might be incorrect if we just make handle_options() non-static. For > example, `scalar --list-cmds=parseopt` wouldn't show the scalar commands > and would instead show the git commands. > > So my feeling is that we should continue to delay this functionality until > Scalar is more stable, perhaps even until after it moves out of contrib/. > The need to change handle_options() to work with a new top-level command > is novel enough to be worth careful scrutiny, but that effort is only > valuable if the Git community is more committed to having Scalar in the > tree for the long term. The usual caveat that little things tend to accumulate and you can die from thousands of paper cuts aside, if "run in that directory" and "pretend these configuration variables are set" are useful enough features to Scalar, I think that the straight-forward option parser this patch adds is very much acceptable. If we need more options (i.e. when we need to add the third thing), that would be the best time to see how handle_options() can be restructured to serve the different set of options git and Scaler need. And this loop, which is too small to be even called "duplicated implementation", should suffice in the meantime.
On Fri, Jan 28 2022, Johannes Schindelin wrote: > On Thu, 27 Jan 2022, Derrick Stolee wrote: > >> The biggest benefits of using handle_options() is for other pre-command >> options such as --exec-path, which I use on a regular basis when testing >> new functionality. >> >> There are other options in handle_options() that might not be >> appropriate, or might be incorrect if we just make handle_options() >> non-static. For example, `scalar --list-cmds=parseopt` wouldn't show the >> scalar commands and would instead show the git commands. > > Right, and since `handle_options()` lives in the same file as `git`'s > `cmd_main()` function, we would not only have to disentangle options that > work only for `git` from those that would also work for `scalar`, but we > would have to extract the `handle_options()` function into a separate > file. > > And while at it, a tangent someone with infinite time on their hands might > suggest is: why not convert `handle_options()` to the `parse_options()` > machinery? Which would of course solve one issue by adding several new > ones. Don't get me wrong: I would find it useful to convert > `git.c:handle_options()` to a function in `libgit.a` which uses the > `parse_options()` machinery. It'll just require a lot of time, and I do > not see enough benefit that would make it worth embarking on that > particular journey. > > But since I had a look at `handle_options()` anyway, I might just as well > summarize my insights about how applicable the supported options are for > `scalar` here: > [...] > # Detrimental > > --exec-path > > Since `scalar` is tightly coupled to a specific Git version, it > would cause much more harm than benefit to encourage users to use > a different Git version by offering them this option. So just to clarify, do you and Stolee disagree about scalar supporting --exec-path, per his comments above? > > --list-cmds > > As you pointed out, this option would produce misleading output. > > Given that only the `-c` and `-C` options are _actually_ useful in the > context of the `scalar` command, I would argue that I chose the best > approach, as the benefit of the intrusive refactorings that would be > necessary to share code with `git.c` is rather small compared with the > amount of work. > >> So my feeling is that we should continue to delay this functionality >> until Scalar is more stable, perhaps even until after it moves out of >> contrib/. The need to change handle_options() to work with a new >> top-level command is novel enough to be worth careful scrutiny, but that >> effort is only valuable if the Git community is more committed to having >> Scalar in the tree for the long term. > > I am okay with holding off with this, for now. > > On the other hand, as I pointed out above: I do not really see it worth > the effort to refactor `git.c:handle_options()` for the minimal benefit it > would give us over the approach I chose in the patch under discussion. I'm fine with just integrating this patch as-is, I just wanted to chime in upthread with a question/context about a thing stated in previous discussion/a link to that discussion. It does seem to me that you're making a mountain out of a molehill here. The below quick hack is a lib-ification of -c, -C and --config-env that took me around 10 minutes. For ease of reading I squashed it into your patch. I mean, we're just talking about copy/pasting existing working code to a new header file, giving it a function parameter, and other small bits of scaffolding. One thing we'll be doing by doing that is not wasting the time of (checks $(ls -l po/*.po|wc -l) ...) around 20 translator on 2 new strings you introduced, and if you view it with the move detection you can see it's almost entirely just moving existing code around. In this case I don't mind much, but speaking generally I see you and Stolee tying yourselves in knots again about scalar being in contrib so we shouldn't use libgit. It already uses libgit, there's even (last I checked) at least one function in it only used directly by the scalar code. I don't remember anyone having any objection to scalar using libgit code, or even that there's libgit code just to help it along. That's a self-imposed limitation you two seem to have invented. Personally I find a patch like the below much easier to review. It's the parts that aren't easy to review boilerplate are all things that we have in-tree already. Whereas proposing a new way to parse -c or -C will lead (at least me) to carefully eyeballing that new implementation, looking at how it differs (if at all) from the existing one, wondering why the i18n strings are subtly different etc (I saw one reason is that since the code was copy/pasted initially the git.c version was updated, but your patch wasn't updated to copy it). diff --git a/Makefile b/Makefile index 5580859afdb..2d0a6611cd5 100644 --- a/Makefile +++ b/Makefile @@ -900,6 +900,7 @@ LIB_OBJS += fmt-merge-msg.o LIB_OBJS += fsck.o LIB_OBJS += fsmonitor.o LIB_OBJS += gettext.o +LIB_OBJS += gitcmd.o LIB_OBJS += gpg-interface.o LIB_OBJS += graph.o LIB_OBJS += grep.o diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 1ce9c2b00e8..ee793ff6ccc 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -11,6 +11,7 @@ #include "dir.h" #include "packfile.h" #include "help.h" +#include "gitcmd.h" /* * Remove the deepest subdirectory in the provided path string. Path must not @@ -808,17 +809,33 @@ int cmd_main(int argc, const char **argv) struct strbuf scalar_usage = STRBUF_INIT; int i; - if (argc > 1) { argv++; argc--; + while (argc > 1 && *argv[0] == '-') { + int show_usage = 0; + + if (gity_handle_options(&argv, &argc, NULL, &show_usage)) { + (argv)++; + (argc)--; + + if (show_usage) + goto usage; + } else { + break; + } + } + + if (argc) { for (i = 0; builtins[i].name; i++) if (!strcmp(builtins[i].name, argv[0])) return !!builtins[i].fn(argc, argv); } +usage: strbuf_addstr(&scalar_usage, - N_("scalar <command> [<options>]\n\nCommands:\n")); + N_("scalar [-C <directory>] [-c <key>=<value>] " + "<command> [<options>]\n\nCommands:\n")); for (i = 0; builtins[i].name; i++) strbuf_addf(&scalar_usage, "\t%s\n", builtins[i].name); diff --git a/Makefile b/Makefile index 5580859afdb..2d0a6611cd5 100644 --- a/Makefile +++ b/Makefile @@ -900,6 +900,7 @@ LIB_OBJS += fmt-merge-msg.o LIB_OBJS += fsck.o LIB_OBJS += fsmonitor.o LIB_OBJS += gettext.o +LIB_OBJS += gitcmd.o LIB_OBJS += gpg-interface.o LIB_OBJS += graph.o LIB_OBJS += grep.o diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 1ce9c2b00e8..ee793ff6ccc 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -11,6 +11,7 @@ #include "dir.h" #include "packfile.h" #include "help.h" +#include "gitcmd.h" /* * Remove the deepest subdirectory in the provided path string. Path must not @@ -808,17 +809,33 @@ int cmd_main(int argc, const char **argv) struct strbuf scalar_usage = STRBUF_INIT; int i; - if (argc > 1) { argv++; argc--; + while (argc > 1 && *argv[0] == '-') { + int show_usage = 0; + + if (gity_handle_options(&argv, &argc, NULL, &show_usage)) { + (argv)++; + (argc)--; + + if (show_usage) + goto usage; + } else { + break; + } + } + + if (argc) { for (i = 0; builtins[i].name; i++) if (!strcmp(builtins[i].name, argv[0])) return !!builtins[i].fn(argc, argv); } +usage: strbuf_addstr(&scalar_usage, - N_("scalar <command> [<options>]\n\nCommands:\n")); + N_("scalar [-C <directory>] [-c <key>=<value>] " + "<command> [<options>]\n\nCommands:\n")); for (i = 0; builtins[i].name; i++) strbuf_addf(&scalar_usage, "\t%s\n", builtins[i].name); diff --git a/contrib/scalar/scalar.txt b/contrib/scalar/scalar.txt index f416d637289..cf4e5b889cc 100644 --- a/contrib/scalar/scalar.txt +++ b/contrib/scalar/scalar.txt @@ -36,6 +36,16 @@ The `scalar` command implements various subcommands, and different options depending on the subcommand. With the exception of `clone`, `list` and `reconfigure --all`, all subcommands expect to be run in an enlistment. +The following options can be specified _before_ the subcommand: + +-C <directory>:: + Before running the subcommand, change the working directory. This + option imitates the same option of linkgit:git[1]. + +-c <key>=<value>:: + For the duration of running the specified subcommand, configure this + setting. This option imitates the same option of linkgit:git[1]. + COMMANDS -------- diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh index 2e1502ad45e..89781568f43 100755 --- a/contrib/scalar/t/t9099-scalar.sh +++ b/contrib/scalar/t/t9099-scalar.sh @@ -85,4 +85,12 @@ test_expect_success 'scalar delete with enlistment' ' test_path_is_missing cloned ' +test_expect_success 'scalar supports -c/-C' ' + test_when_finished "scalar delete sub" && + git init sub && + scalar -C sub -c status.aheadBehind=bogus register && + test -z "$(git -C sub config --local status.aheadBehind)" && + test true = "$(git -C sub config core.preloadIndex)" +' + test_done diff --git a/git.c b/git.c index edda922ce6d..dc75b3f0294 100644 --- a/git.c +++ b/git.c @@ -5,6 +5,7 @@ #include "run-command.h" #include "alias.h" #include "shallow.h" +#include "gitcmd.h" #define RUN_SETUP (1<<0) #define RUN_SETUP_GENTLY (1<<1) @@ -138,6 +139,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) while (*argc > 0) { const char *cmd = (*argv)[0]; + int show_usage = 0; if (cmd[0] != '-') break; @@ -247,22 +249,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1); if (envchanged) *envchanged = 1; - } else if (!strcmp(cmd, "-c")) { - if (*argc < 2) { - fprintf(stderr, _("-c expects a configuration string\n" )); - usage(git_usage_string); - } - git_config_push_parameter((*argv)[1]); - (*argv)++; - (*argc)--; - } else if (!strcmp(cmd, "--config-env")) { - if (*argc < 2) { - fprintf(stderr, _("no config key given for --config-env\n" )); - usage(git_usage_string); - } - git_config_push_env((*argv)[1]); - (*argv)++; - (*argc)--; } else if (skip_prefix(cmd, "--config-env=", &cmd)) { git_config_push_env(cmd); } else if (!strcmp(cmd, "--literal-pathspecs")) { @@ -295,19 +281,6 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) set_alternate_shallow_file(the_repository, (*argv)[0], 1); if (envchanged) *envchanged = 1; - } else if (!strcmp(cmd, "-C")) { - if (*argc < 2) { - fprintf(stderr, _("no directory given for '%s' option\n" ), "-C"); - usage(git_usage_string); - } - if ((*argv)[1][0]) { - if (chdir((*argv)[1])) - die_errno("cannot change to '%s'", (*argv)[1]); - if (envchanged) - *envchanged = 1; - } - (*argv)++; - (*argc)--; } else if (skip_prefix(cmd, "--list-cmds=", &cmd)) { trace2_cmd_name("_query_"); if (!strcmp(cmd, "parseopt")) { @@ -322,6 +295,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) } else { exit(list_cmds(cmd)); } + } else if (gity_handle_options(argv, argc, envchanged, + &show_usage)) { + if (show_usage) + usage(git_usage_string); } else { fprintf(stderr, _("unknown option: %s\n"), cmd); usage(git_usage_string); diff --git a/gitcmd.c b/gitcmd.c new file mode 100644 index 00000000000..0d8630ec2a4 --- /dev/null +++ b/gitcmd.c @@ -0,0 +1,47 @@ +#include "cache.h" +#include "gitcmd.h" +#include "config.h" + +int gity_handle_options(const char ***argv, int *argc, + int *envchanged, int *show_usage) +{ + const char *cmd = (*argv)[0]; + + if (!strcmp(cmd, "-c")) { + if (*argc < 2) { + fprintf(stderr, _("-c expects a configuration string\n" )); + goto usage; + } + git_config_push_parameter((*argv)[1]); + (*argv)++; + (*argc)--; + } else if (!strcmp(cmd, "--config-env")) { + if (*argc < 2) { + fprintf(stderr, _("no config key given for --config-env\n" )); + goto usage; + } + git_config_push_env((*argv)[1]); + (*argv)++; + (*argc)--; + } else if (!strcmp(cmd, "-C")) { + if (*argc < 2) { + fprintf(stderr, _("no directory given for '%s' option\n" ), "-C"); + goto usage; + } + if ((*argv)[1][0]) { + if (chdir((*argv)[1])) + die_errno("cannot change to '%s'", (*argv)[1]); + if (envchanged) + *envchanged = 1; + } + (*argv)++; + (*argc)--; + } else { + return 0; + } + return 1; +usage: + *show_usage = 1; + return 1; +} + diff --git a/gitcmd.h b/gitcmd.h new file mode 100644 index 00000000000..95c934b1500 --- /dev/null +++ b/gitcmd.h @@ -0,0 +1,11 @@ +#ifndef GITCMD_H +#define GITCMD_H + +/** + * Handle options like in a "git"-y way, i.e. to emulate a command + * that works like "git" itself. This is the part of handle_options() + * that contrib/scalar needs from git.c. + */ +int gity_handle_options(const char ***argv, int *argc, int *envchanged, + int *show_usage); +#endif
On 1/28/2022 6:27 AM, Johannes Schindelin wrote: > Hi Stolee, > > On Thu, 27 Jan 2022, Derrick Stolee wrote: > >> The biggest benefits of using handle_options() is for other pre-command >> options such as --exec-path, which I use on a regular basis when testing >> new functionality. >> >> There are other options in handle_options() that might not be >> appropriate, or might be incorrect if we just make handle_options() >> non-static. For example, `scalar --list-cmds=parseopt` wouldn't show the >> scalar commands and would instead show the git commands. > > Right, and since `handle_options()` lives in the same file as `git`'s > `cmd_main()` function, we would not only have to disentangle options that > work only for `git` from those that would also work for `scalar`, but we > would have to extract the `handle_options()` function into a separate > file. I agree that these would be necessary steps. > But since I had a look at `handle_options()` anyway, I might just as well > summarize my insights about how applicable the supported options are for > `scalar` here: > > # Beneficial > > -c <key>=<value> > --config-env <key>=<value> > -C <directory> > > Since I added support for these (except for the long form > `--config-env` that I actually only learned while researching this > email), it is obvious that I'd like `scalar` to support them. > > # Won't hurt These "Won't hurt" items look to me as "they probably don't matter, but it would be nice if "scalar <option>" didn't just fail for users who are used to "git <option>". > # Detrimental I think your "detrimental" choices are actually more useful than any of your "won't hurt" choices. > --exec-path > > Since `scalar` is tightly coupled to a specific Git version, it > would cause much more harm than benefit to encourage users to use > a different Git version by offering them this option. As mentioned, I use this option in my local development all the time. This compatibility issue you discuss is something that happens within Git itself, too, when it calls a subcommand. So, users can already hurt themselves in this way and should be cautious about using it. > --list-cmds > > As you pointed out, this option would produce misleading output. It would, but I also think that a correct implementation would be helpful to users. It just takes more work than just calling handle_options() as-is. > Given that only the `-c` and `-C` options are _actually_ useful in the > context of the `scalar` command, I would argue that I chose the best > approach, as the benefit of the intrusive refactorings that would be > necessary to share code with `git.c` is rather small compared with the > amount of work. > >> So my feeling is that we should continue to delay this functionality >> until Scalar is more stable, perhaps even until after it moves out of >> contrib/. The need to change handle_options() to work with a new >> top-level command is novel enough to be worth careful scrutiny, but that >> effort is only valuable if the Git community is more committed to having >> Scalar in the tree for the long term. > > I am okay with holding off with this, for now. > > On the other hand, as I pointed out above: I do not really see it worth > the effort to refactor `git.c:handle_options()` for the minimal benefit it > would give us over the approach I chose in the patch under discussion. I was thinking that it would be good to maybe extract just the "-C", "-c" options from handle_options() and call that from scalar.c, but it wouldn't work to "just parse "-C" and "-c" and then parse all the other options" because if someone did "git --exec-path=<X> -C <Y> status" then the "-C" parser would want to stop after seeing "--exec-path". So, perhaps the code copy is really the least intrusive approach we have until we see a need for more of these options. Thanks, -Stolee
On 1/28/2022 1:05 PM, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >> My understanding was that this was ejected so we could find the right time >> to lib-ify handle_options() (as Taylor suggested), but we didn't want to do >> that while Scalar was still in a tentative state (in contrib/ with a plan >> to move it out after more is implemented). >> >> The biggest benefits of using handle_options() is for other pre-command >> options such as --exec-path, which I use on a regular basis when testing >> new functionality. >> >> There are other options in handle_options() that might not be appropriate, >> or might be incorrect if we just make handle_options() non-static. For >> example, `scalar --list-cmds=parseopt` wouldn't show the scalar commands >> and would instead show the git commands. >> >> So my feeling is that we should continue to delay this functionality until >> Scalar is more stable, perhaps even until after it moves out of contrib/. >> The need to change handle_options() to work with a new top-level command >> is novel enough to be worth careful scrutiny, but that effort is only >> valuable if the Git community is more committed to having Scalar in the >> tree for the long term. > > The usual caveat that little things tend to accumulate and you can > die from thousands of paper cuts aside, if "run in that directory" > and "pretend these configuration variables are set" are useful > enough features to Scalar, I think that the straight-forward option > parser this patch adds is very much acceptable. > > If we need more options (i.e. when we need to add the third thing), > that would be the best time to see how handle_options() can be > restructured to serve the different set of options git and Scaler > need. > > And this loop, which is too small to be even called "duplicated > implementation", should suffice in the meantime. I have come around to this line of thinking. My response to Johannes' detailed examination of my idea goes into that more deeply. Thanks, -Stolee
On 1/28/2022 1:21 PM, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Jan 28 2022, Johannes Schindelin wrote: > >> On Thu, 27 Jan 2022, Derrick Stolee wrote: >> >>> The biggest benefits of using handle_options() is for other pre-command >>> options such as --exec-path, which I use on a regular basis when testing >>> new functionality. >>> >>> There are other options in handle_options() that might not be >>> appropriate, or might be incorrect if we just make handle_options() >>> non-static. For example, `scalar --list-cmds=parseopt` wouldn't show the >>> scalar commands and would instead show the git commands. >> >> Right, and since `handle_options()` lives in the same file as `git`'s >> `cmd_main()` function, we would not only have to disentangle options that >> work only for `git` from those that would also work for `scalar`, but we >> would have to extract the `handle_options()` function into a separate >> file. >> >> And while at it, a tangent someone with infinite time on their hands might >> suggest is: why not convert `handle_options()` to the `parse_options()` >> machinery? Which would of course solve one issue by adding several new >> ones. Don't get me wrong: I would find it useful to convert >> `git.c:handle_options()` to a function in `libgit.a` which uses the >> `parse_options()` machinery. It'll just require a lot of time, and I do >> not see enough benefit that would make it worth embarking on that >> particular journey. >> >> But since I had a look at `handle_options()` anyway, I might just as well >> summarize my insights about how applicable the supported options are for >> `scalar` here: >> [...] >> # Detrimental >> >> --exec-path >> >> Since `scalar` is tightly coupled to a specific Git version, it >> would cause much more harm than benefit to encourage users to use >> a different Git version by offering them this option. > > So just to clarify, do you and Stolee disagree about scalar supporting > --exec-path, per his comments above? I think it would be nice, but it's also not a blocker for me. > In this case I don't mind much, but speaking generally I see you and > Stolee tying yourselves in knots again about scalar being in contrib so > we shouldn't use libgit. > > It already uses libgit, there's even (last I checked) at least one > function in it only used directly by the scalar code. My concern is not that we shouldn't use libgit (because we do) but that we shouldn't make significant changes to libgit.a only for Scalar's benefit until it is incorporated in a more final way. In my opinion, well-architected code is code that is easy to delete. Until we have Scalar mostly feature-complete and can make a decision about it living in the Git tree long-term (and _where_ it resides), I would like to have the following property: If I were to revert all commits that touch contrib/scalar/ and squash them into a single commit, then we would have a bunch of file deletions and a very small set of edits to the Makefile. I don't know how much the ship has sailed there already, but keeping the size of that "revert diff" small means that we are keeping the coupling low during this review process. > I don't remember anyone having any objection to scalar using libgit > code, or even that there's libgit code just to help it along. That's a > self-imposed limitation you two seem to have invented. > > Personally I find a patch like the below much easier to review. It's the > parts that aren't easy to review boilerplate are all things that we have > in-tree already. > > Whereas proposing a new way to parse -c or -C will lead (at least me) to > carefully eyeballing that new implementation, looking at how it differs > (if at all) from the existing one, wondering why the i18n strings are > subtly different etc (I saw one reason is that since the code was > copy/pasted initially the git.c version was updated, but your patch > wasn't updated to copy it). > diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c > index 1ce9c2b00e8..ee793ff6ccc 100644 > --- a/contrib/scalar/scalar.c > +++ b/contrib/scalar/scalar.c > diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c > index 1ce9c2b00e8..ee793ff6ccc 100644 > --- a/contrib/scalar/scalar.c > +++ b/contrib/scalar/scalar.c Was this diff double-copied or something? > + while (argc > 1 && *argv[0] == '-') { > + int show_usage = 0; > + > + if (gity_handle_options(&argv, &argc, NULL, &show_usage)) { > + (argv)++; > + (argc)--; > + > + if (show_usage) > + goto usage; > + } else { > + break; > + } > + } > + > + if (argc) { This loop seemed a tad cumbersome for something calling a helper method, BUT.... > @@ -322,6 +295,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) > } else { > exit(list_cmds(cmd)); > } > + } else if (gity_handle_options(argv, argc, envchanged, > + &show_usage)) { > + if (show_usage) > + usage(git_usage_string); ...by checking this at the end of your loop here, you have solved the concern I was talking about with something like git --no-pager -C <X> status having the -C parsing fail because it doesn't recognize --no-pager. I think this patch you present here would be a good approach to start transitioning options out of handle_options(), perhaps one at a time. But due to my stated goal of "let's not modify things in libgit.a for Scalar unless absolutely necessary", this approach should be delayed until at least Scalar is more final. Thanks, -Stolee
On Fri, Jan 28 2022, Derrick Stolee wrote: > On 1/28/2022 1:21 PM, Ævar Arnfjörð Bjarmason wrote: >> >> On Fri, Jan 28 2022, Johannes Schindelin wrote: >> >>> On Thu, 27 Jan 2022, Derrick Stolee wrote: >>> >>>> The biggest benefits of using handle_options() is for other pre-command >>>> options such as --exec-path, which I use on a regular basis when testing >>>> new functionality. >>>> >>>> There are other options in handle_options() that might not be >>>> appropriate, or might be incorrect if we just make handle_options() >>>> non-static. For example, `scalar --list-cmds=parseopt` wouldn't show the >>>> scalar commands and would instead show the git commands. >>> >>> Right, and since `handle_options()` lives in the same file as `git`'s >>> `cmd_main()` function, we would not only have to disentangle options that >>> work only for `git` from those that would also work for `scalar`, but we >>> would have to extract the `handle_options()` function into a separate >>> file. >>> >>> And while at it, a tangent someone with infinite time on their hands might >>> suggest is: why not convert `handle_options()` to the `parse_options()` >>> machinery? Which would of course solve one issue by adding several new >>> ones. Don't get me wrong: I would find it useful to convert >>> `git.c:handle_options()` to a function in `libgit.a` which uses the >>> `parse_options()` machinery. It'll just require a lot of time, and I do >>> not see enough benefit that would make it worth embarking on that >>> particular journey. >>> >>> But since I had a look at `handle_options()` anyway, I might just as well >>> summarize my insights about how applicable the supported options are for >>> `scalar` here: >>> [...] >>> # Detrimental >>> >>> --exec-path >>> >>> Since `scalar` is tightly coupled to a specific Git version, it >>> would cause much more harm than benefit to encourage users to use >>> a different Git version by offering them this option. >> >> So just to clarify, do you and Stolee disagree about scalar supporting >> --exec-path, per his comments above? > > I think it would be nice, but it's also not a blocker for me. > >> In this case I don't mind much, but speaking generally I see you and >> Stolee tying yourselves in knots again about scalar being in contrib so >> we shouldn't use libgit. >> >> It already uses libgit, there's even (last I checked) at least one >> function in it only used directly by the scalar code. > > My concern is not that we shouldn't use libgit (because we do) but that > we shouldn't make significant changes to libgit.a only for Scalar's > benefit until it is incorporated in a more final way. > > In my opinion, well-architected code is code that is easy to delete. > Until we have Scalar mostly feature-complete and can make a decision > about it living in the Git tree long-term (and _where_ it resides), I > would like to have the following property: If I were to revert all > commits that touch contrib/scalar/ and squash them into a single commit, > then we would have a bunch of file deletions and a very small set of > edits to the Makefile. I don't know how much the ship has sailed there > already, but keeping the size of that "revert diff" small means that we > are keeping the coupling low during this review process. Fair enough. I think there's cases where it won't make sense, and cases where it will. Maybe it doesn't make sense here, but generally I wouldn't take it being needed for scalar heavily into account per-se, for the review & i18n reasons I mentioned. I.e. the list is already looking at these patches, and translators are being presented these strings as part of our existing set. So "this reuses existing tested code" and "you won't need to translate this new thing" will be benefits whatever the current state of scalar is. >> I don't remember anyone having any objection to scalar using libgit >> code, or even that there's libgit code just to help it along. That's a >> self-imposed limitation you two seem to have invented. >> >> Personally I find a patch like the below much easier to review. It's the >> parts that aren't easy to review boilerplate are all things that we have >> in-tree already. >> >> Whereas proposing a new way to parse -c or -C will lead (at least me) to >> carefully eyeballing that new implementation, looking at how it differs >> (if at all) from the existing one, wondering why the i18n strings are >> subtly different etc (I saw one reason is that since the code was >> copy/pasted initially the git.c version was updated, but your patch >> wasn't updated to copy it). > >> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c >> index 1ce9c2b00e8..ee793ff6ccc 100644 >> --- a/contrib/scalar/scalar.c >> +++ b/contrib/scalar/scalar.c > >> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c >> index 1ce9c2b00e8..ee793ff6ccc 100644 >> --- a/contrib/scalar/scalar.c >> +++ b/contrib/scalar/scalar.c > > Was this diff double-copied or something? Yes, sorry the first 3 hunks are the same. I manually re-copied this into my editor (I forgot a stray debugging printf) and screwed it up.
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 1ce9c2b00e8..7db2a97416e 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -808,6 +808,25 @@ int cmd_main(int argc, const char **argv) struct strbuf scalar_usage = STRBUF_INIT; int i; + while (argc > 1 && *argv[1] == '-') { + if (!strcmp(argv[1], "-C")) { + if (argc < 3) + die(_("-C requires a <directory>")); + if (chdir(argv[2]) < 0) + die_errno(_("could not change to '%s'"), + argv[2]); + argc -= 2; + argv += 2; + } else if (!strcmp(argv[1], "-c")) { + if (argc < 3) + die(_("-c requires a <key>=<value> argument")); + git_config_push_parameter(argv[2]); + argc -= 2; + argv += 2; + } else + break; + } + if (argc > 1) { argv++; argc--; @@ -818,7 +837,8 @@ int cmd_main(int argc, const char **argv) } strbuf_addstr(&scalar_usage, - N_("scalar <command> [<options>]\n\nCommands:\n")); + N_("scalar [-C <directory>] [-c <key>=<value>] " + "<command> [<options>]\n\nCommands:\n")); for (i = 0; builtins[i].name; i++) strbuf_addf(&scalar_usage, "\t%s\n", builtins[i].name); diff --git a/contrib/scalar/scalar.txt b/contrib/scalar/scalar.txt index f416d637289..cf4e5b889cc 100644 --- a/contrib/scalar/scalar.txt +++ b/contrib/scalar/scalar.txt @@ -36,6 +36,16 @@ The `scalar` command implements various subcommands, and different options depending on the subcommand. With the exception of `clone`, `list` and `reconfigure --all`, all subcommands expect to be run in an enlistment. +The following options can be specified _before_ the subcommand: + +-C <directory>:: + Before running the subcommand, change the working directory. This + option imitates the same option of linkgit:git[1]. + +-c <key>=<value>:: + For the duration of running the specified subcommand, configure this + setting. This option imitates the same option of linkgit:git[1]. + COMMANDS --------