git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Alex Riesen <alexander.riesen@cetitec.com>
To: Jeff King <peff@peff.net>
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 13:50:25 +0200	[thread overview]
Message-ID: <20200915115025.GA18984@pflmari> (raw)
In-Reply-To: <20200914194951.GA2819729@coredump.intra.peff.net>

Jeff King, Mon, Sep 14, 2020 21:49:51 +0200:
> On Mon, Sep 14, 2020 at 02:19:06PM +0200, Alex Riesen wrote:
> 
> > Unfortunately, it only worked for the fetches which didn't use --all or
> > --multiple. After a light searching, I failed to find an explanation as to
> > why --all|--multiple are handled so inconsistently with single remote fetches
> > and added the options (similar to --force or --keep) to the argument list for
> > sub-fetches: ...
> >  
> > Am I missing something obvious?
> 
> I don't think so. When we're starting fetch sub-processes, some options
> will make sense to pass along and some won't. The parent has to either
> pass all options and omit some, or explicitly pass ones it knows are
> useful. It looks like the code chooses the latter, but these particular
> options never got added (and it seems like they should be, as they are
> only useful to the child fetch processes that actually touch the
> network).
> 
> 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?

> 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 :)

> Also, regarding these two specific options, it sounds like you'd want
> them set for all fetches during the time your IPv6 setup is broken. In
> which case I think a config option might have served you better. So that
> might be something worth implementing (though either way I think the fix
> above is worth doing independently).

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?

I'll be sending the first change reformatted as patch shortly. Just in case.


  reply	other threads:[~2020-09-15 11:53 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 [this message]
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
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=20200915115025.GA18984@pflmari \
    --to=alexander.riesen@cetitec.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=normalperson@yhbt.net \
    --cc=peff@peff.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).