git@vger.kernel.org list mirror (unofficial, 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: Tue, 15 Sep 2020 09:05:06 -0400	[thread overview]
Message-ID: <20200915130506.GA2839276@coredump.intra.peff.net> (raw)
In-Reply-To: <20200915115025.GA18984@pflmari>

On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:

> > So your patch above looks quite sensible (modulo useful bits like a
> > signoff and maybe a test, though I guess the impact of those options
> > is probably hard to cover in our tests).
> 
> I tried to come up with one, but (aside from rather pointless checking of
> option presence in the trace output) failed to.
> 
> Or may be precisely this could be the point of the test: just do a fetch with
> all options we intend to pass down to sub-fetches and check that they are
> indeed present in the invocation of fetch --all/--multiple/--recurse-submodules?

Unfortunately I don't think that accomplishes much, since the main bug
we're worried about is missing options. And it would require somebody
adding the new options to the test, at which point you could just assume
they would add it to add_options_to_argv().

Though I guess we can automatically get the list of options these days.
So perhaps something like:

  subopts=
  for opt in $(git fetch --git-completion-helper)
  do
        case "$opt" in
        # options that we know do not go to sub-fetches
        --all|--jobs|etc...)
                ;;
	# try/match only the positive versions
	--no-*)
	        ;;
	# give a fake value for options with values
	*=)
                subopts="$subopts ${opt}1"
		;;
	# and pass through any boolean options
	*)
                subopts="$subopts $opt"
		;;
        esac
  done
  GIT_TRACE=$PWD/trace.out git fetch --all $subopts
  perl -lne '
    BEGIN { @want = @ARGV; @ARGV = () }
    /run_command: git fetch (.*)/ and $seen{$_}++ for split(/ /, $1);
    END { print for grep { !$seen{$_} } @want }
  ' <trace.out -- $subopts

Except that doesn't quite work, because the parent fetch will complain
about nonsense values (e.g., --filter=1). So it would probably need a
bit more manual intelligence to cover those options. It looks like some
options are mutually exclusive, too (--deepen/--depth), so maybe we'd
need to run an individual "fetch --all" for each option.

I dunno. It's getting pretty complicated. :)

> > It is rather unfortunate that anybody adding new fetch options needs to
> > remember to (maybe) add them to add_options_to_argv() themselves.
> 
> Maybe make add_options_to_argv to go through builtin_fetch_options[] and copy
> the options with a special marker if they were provided?
> And use the word "recursive" in help text as the marker :)

Yeah, that would solve the duplication problem. We could probably add a
"recursive" bit to the parse-options flag variable. Even if
parse-options itself doesn't use it, it could be a convenience for
callers like this one. It is a little inconvenient to set flags there,
just because it usually means ditching our wrapper macros in favor of a
raw struct declaration.

> Sure! Thinking about it, I actually would have preferred to have both: a
> config option and a command-line option. So that I can set --ipv4 in, say,
> ~/.config/git/config file, but still have the option to try --ipv6 from time
> to time to check if the network setup magically fixed itself.
> 
> What would the preferred name for that config option be? fetch.ipv?

It looks like we've got similar options for clone/pull (which are really
fetch under the hood of course) and push. We have the "transfer.*"
namespace which applies to both already. So maybe "transfer.ipversion"
or something?

-Peff

  parent reply	other threads:[~2020-09-16  0:40 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-14 12:19 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     ` Jeff King [this message]
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
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=20200915130506.GA2839276@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 \
    --subject='Re: sub-fetches discard --ipv4|6 option' \
    /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

Code repositories for project(s) associated with this 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).