git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Carlo Arenas <carenas@gmail.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: Christian Couder <christian.couder@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Christian Couder <chriscool@tuxfamily.org>,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH v3] http: add custom hostname to IP address resolutions
Date: Thu, 12 May 2022 06:56:24 -0700	[thread overview]
Message-ID: <CAPUEsphh_3oFkfAdB-mZ9ZH7F0tCm_iwapy8=6VFE45yWoOK3w@mail.gmail.com> (raw)
In-Reply-To: <Yn0FPkoUNacvctAp@ncase>

On thu, May 12, 2022 at 6:01 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Tue, May 10, 2022 at 11:20:41AM -0700, Carlo Arenas wrote:
> > On Mon, May 9, 2022 at 8:38 AM Christian Couder
> > <christian.couder@gmail.com> wrote:
> > > diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> > > index f92c79c132..4a8dbb7eee 100755
> > > --- a/t/t5551-http-fetch-smart.sh
> > > +++ b/t/t5551-http-fetch-smart.sh
> > > @@ -567,4 +567,11 @@ test_expect_success 'client falls back from v2 to v0 to match server' '
> > >         grep symref=HEAD:refs/heads/ trace
> > >  '
> > >
> > > +test_expect_success 'passing hostname resolution information works' '
> > > +       BOGUS_HOST=gitbogusexamplehost.com &&
> > > +       BOGUS_HTTPD_URL=$HTTPD_PROTO://$BOGUS_HOST:$LIB_HTTPD_PORT &&
> > > +       test_must_fail git ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null &&
> > > +       git -c "http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1" ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null
> > > +'
> >
> > Is setting it up as a command line config option the way you expect to
> > use this, and if so why not make it a full blown command line option
> > with the previous caveats that were discussed before?
>
> If you did this as a command-line option, you'd now be forced to add it
> to every single command you want to support this: git-fetch, git-pull,
> git-remote, git-ls-remote and maybe others I forgot about. On the other
> hand, by having this as a configuration variable in `http.c` all of
> those commands benefit the same.

There are ways to add common options to all commands that would help
here, but as Junio pointed out it is not ideal because then you have
to ALSO provide implementations, which you don't seem interested in
doing.

> Furthermore, using a config option is a lot more flexible: you can
> persist it at different levels of your gitconfig, can easily inject it
> in a script via the use of environment variables, or directly override
> it when spawning a command with `-c`.
>
> Overall, I think it is preferable to keep this as an option as opposed
> to adding such an obscure parameter to all of the commands.

I think we had already decided that a config is more flexible, even if
I personally don't agree.

> > I also think it might be a little confusing (and probably warranted of
> > an advice message) if git will decide based on a configuration
> > somewhere in its resolution tree that the IP I am connecting is
> > different than the one I expect it to use through the system
> > configured resolution mechanism for such a thing.
>
> That's true already though, isn't it? A user may set `url.*.insteadOf`
> and be surprised at a later point that their URLs are getting redirected
> somewhere else. And there's probably a lot more examples where a user
> may be confused when forgetting about certain configuration variables
> that change the way Git behaves.

That is a good point, but unlike url.*.insteadOf, this is meant to be
an obscure setting that shouldn't be enabled by default (or under
common circumstances), so having the advice there is helpful for when
we find ourselves in an unexpected situation and to avoid confusion.

I would even argue YOUR use of it in a server might even benefit from
this advice, because it could be strange to get a different IP than
the one you set in the command line if there is also another entry in
some config that you happened to read.

> I also don't think that using an advise here would be ideal. The main
> use case of this configuration variable is going to be servers.

My feedback about servers is indeed below, so I won't repeat myself
but keeping a global config that has this advice disabled in a server
shouldn't be that difficult; indeed it MIGHT be already there since
most features that are meant for interactive users (like an advice)
are better disabled in servers.

> > I assume that if you want to use this frequently, having that advice
> > disabled in your global config wouldn't be a hassle, but it might be
> > useful to know that I am interacting with a potentially different IP
> > when referring to some host by name in my local repo, maybe because I
> > forgot to change that setting after some debugging.
> >
> > I am sure all those folks that forget to edit their /etc/hosts after
> > they are done with their local site versions might instead use this
> > and then be happy to be warned about it later.
> >
> > Carlo

  reply	other threads:[~2022-05-12 13:57 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-02  8:36 [PATCH] http: add custom hostname to IP address resolves Christian Couder
2022-05-02 19:04 ` Junio C Hamano
2022-05-04 10:07   ` Christian Couder
2022-05-04 14:34     ` Junio C Hamano
2022-05-05 10:48       ` Christian Couder
2022-05-05 11:16         ` Carlo Marcelo Arenas Belón
2022-05-09 15:40           ` Christian Couder
2022-05-04 10:46 ` [PATCH v2] http: add custom hostname to IP address resolutions Christian Couder
2022-05-05 11:21   ` Carlo Marcelo Arenas Belón
2022-05-12  8:52     ` Christian Couder
2022-05-12 16:22       ` Junio C Hamano
2022-05-12 18:57         ` Christian Couder
2022-05-09 15:38   ` [PATCH v3] " Christian Couder
2022-05-10 18:20     ` Carlo Arenas
2022-05-12  8:29       ` Christian Couder
2022-05-12 11:55         ` Carlo Arenas
2022-05-12 13:01       ` Patrick Steinhardt
2022-05-12 13:56         ` Carlo Arenas [this message]
2022-05-12 15:58         ` Junio C Hamano
2022-05-16  8:38     ` [PATCH v4] " Christian Couder

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='CAPUEsphh_3oFkfAdB-mZ9ZH7F0tCm_iwapy8=6VFE45yWoOK3w@mail.gmail.com' \
    --to=carenas@gmail.com \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=ps@pks.im \
    --subject='Re: [PATCH v3] http: add custom hostname to IP address resolutions' \
    /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).