git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Alex Riesen <alexander.riesen@cetitec.com>
Cc: git@vger.kernel.org, Eric Wong <normalperson@yhbt.net>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: sub-fetches discard --ipv4|6 option
Date: Wed, 16 Sep 2020 12:32:18 -0400	[thread overview]
Message-ID: <20200916163218.GA17726@coredump.intra.peff.net> (raw)
In-Reply-To: <20200915160357.GC18984@pflmari>

On Tue, Sep 15, 2020 at 06:03:57PM +0200, Alex Riesen wrote:

> > If you go that route, we have some "_F" macros that take flags. Probably
> > would make sense to add it more consistently, which lets you convert:
> > 
> >   OPT_BOOL('f', "foo", &foo, "the foo option");
> > 
> > into:
> > 
> >   OPT_BOOL_F('f', "foo", &foo, "the foo option", PARSE_OPT_RECURSIVE);
> > 
> > but could also be used for other flags.
> 
> This part (marking of the options) was easy. What's left is finding out if an
> option was actually specified in the command-line. The ...options[] arrays are
> not update by parse_options() with what was given, are they?

Oh right. Having the list of options is not that helpful because
add_options_argv() is actually working off the parsed data in individual
variables. Sorry for leading you in a (maybe) wrong direction.

I think this approach would have to be coupled with some mechanism for
looking over the original list of options (either saving the original
argv before parsing, or teaching parse-options the kind of two-pass
"don't parse these the first time" mechanism discussed elsewhere in the
thread).

> Maybe extend struct option with a field to store given command-line argument
> (as it was specified) and parse_options() will update the field if
> PARSE_OPT_RECURSIVE is present in .flags?
> Is it allowed for parse_options() to modify the options array?

No, we take the options array as a const pointer, so many callers would
likely need to be updated to handle that. Plus it's possible some may
actually re-use the array multiple times in some cases.

> Or is it possible to use something in parse-options.h API to note the
> arguments somewhere while they are parse? I mean, there are
> parse_options_start/step/end, can cmd_fetch argument parsing use those
> so that the options marked recursive can be saved for sub-fetches?

Possibly the step-wise parsing could help. But I think it might be
easier to just let parse_options() save a copy of parsed options. And
then our PARSE_OPT_RECURSIVE really becomes PARSE_OPT_SAVE or similar,
which would cause parse-options to save the original option (and any
value argument) in its original form.

There's one slight complication, which is how the array of saved options
gets communicated back to the caller. Leaving them in the original argv
probably isn't a good idea (because the caller relies on it having
options removed in order to find the non-option arguments).

Adding a new strvec pointer to parse_options() works, but means updating
all of the callers, most of which will pass NULL. Possibly the existing
"flags" parameter to parse_options() could grow into a struct. That
requires modifying each caller, but at least solves the problem once and
for all.

Another option is to stick it into parse_opt_ctx_t. That's used only be
step-wise callers, of which there are very few.

-Peff

  reply	other threads:[~2020-09-16 20:15 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-14 12:19 sub-fetches discard --ipv4|6 option Alex Riesen
2020-09-14 19:49 ` Jeff King
2020-09-15 11:50   ` Alex Riesen
2020-09-15 11:54     ` [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules Alex Riesen
2020-09-15 13:06       ` Jeff King
2020-09-16  4:17         ` Junio C Hamano
2020-09-16  7:27           ` Alex Riesen
2020-09-15 21:19       ` Junio C Hamano
2020-09-16  7:25         ` Alex Riesen
2020-09-15 13:05     ` sub-fetches discard --ipv4|6 option Jeff King
2020-09-15 13:54       ` [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches Alex Riesen
2020-09-16 20:02         ` Jeff King
2020-09-17  8:07           ` Alex Riesen
2020-09-17 13:20           ` [PATCH] Config option to set the " Alex Riesen
2020-09-17 13:26             ` Alex Riesen
2020-09-17 13:31             ` Jeff King
2020-09-17 13:35               ` Alex Riesen
2020-09-17 14:51                 ` Jeff King
2020-09-17 15:17                   ` Alex Riesen
2020-12-22 19:55                     ` Junio C Hamano
2021-01-07 10:06                       ` Alex Riesen
2020-09-17 16:05             ` Alex Riesen
2020-09-16 20:14         ` [PATCH] config: option transfer.ipversion to set " Junio C Hamano
2020-09-16 20:18           ` Jeff King
2020-09-16 22:50             ` Junio C Hamano
2020-09-16 22:52               ` Junio C Hamano
2020-09-17  0:48                 ` Jeff King
2020-09-17  0:57                   ` Junio C Hamano
2020-09-16 21:35           ` Junio C Hamano
2020-09-17 14:02             ` Alex Riesen
2020-09-17 22:31               ` Junio C Hamano
2020-09-18  7:16                 ` Alex Riesen
2020-09-18 16:37                   ` Junio C Hamano
2020-09-21 16:39                     ` Alex Riesen
2020-09-22  5:03                       ` Jeff King
2020-09-17  8:04           ` Alex Riesen
2020-09-17  8:18           ` Alex Riesen
2020-09-15 14:06       ` sub-fetches discard --ipv4|6 option Alex Riesen
2020-09-15 15:27         ` Jeff King
2020-09-15 16:03           ` Alex Riesen
2020-09-16 16:32             ` Jeff King [this message]
2020-09-17 14:33               ` Alex Riesen
2020-09-22  5:08                 ` Jeff King
2020-09-15 20:09           ` Junio C Hamano
2020-09-15 21:23             ` Jeff King
2020-09-15 21:32               ` Junio C Hamano
2020-09-16 16:34                 ` Jeff King
2020-09-16 21:20                   ` Junio C Hamano
2020-09-14 20:06 ` Junio C Hamano

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=20200916163218.GA17726@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=alexander.riesen@cetitec.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=normalperson@yhbt.net \
    /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).