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@xxxxxx> > > > > 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@xxxxxx> > > --- > > 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
