From: Derrick Stolee <stolee@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>,
git@vger.kernel.org, "Taylor Blau" <me@ttaylorr.com>
Subject: Re: [PATCH] scalar: accept -C and -c options before the subcommand
Date: Fri, 28 Jan 2022 14:37:22 -0500 [thread overview]
Message-ID: <5283a1b2-a31f-1657-1a67-cab6d9fec0ac@gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2201281148310.347@tvgsbejvaqbjf.bet>
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
next prev parent reply other threads:[~2022-01-28 19:37 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-26 11:15 [PATCH] scalar: accept -C and -c options before the subcommand Johannes Schindelin via GitGitGadget
2022-01-26 20:53 ` Taylor Blau
2022-01-28 11:43 ` Johannes Schindelin
2022-01-27 2:55 ` Ævar Arnfjörð Bjarmason
2022-01-27 14:46 ` Derrick Stolee
2022-01-28 11:27 ` Johannes Schindelin
2022-01-28 18:21 ` Ævar Arnfjörð Bjarmason
2022-01-28 19:52 ` Derrick Stolee
2022-01-29 6:39 ` Ævar Arnfjörð Bjarmason
2022-01-28 19:37 ` Derrick Stolee [this message]
2022-01-28 18:05 ` Junio C Hamano
2022-01-28 19:38 ` Derrick Stolee
2022-01-28 14:31 ` [PATCH v2] " Johannes Schindelin via GitGitGadget
2022-01-28 19:40 ` Derrick Stolee
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5283a1b2-a31f-1657-1a67-cab6d9fec0ac@gmail.com \
--to=stolee@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=me@ttaylorr.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).