git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: Carlo Arenas <carenas@gmail.com>,
	Christian Couder <christian.couder@gmail.com>,
	git@vger.kernel.org, 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 08:58:47 -0700	[thread overview]
Message-ID: <xmqqa6bmhhe0.fsf@gitster.g> (raw)
In-Reply-To: <Yn0FPkoUNacvctAp@ncase> (Patrick Steinhardt's message of "Thu, 12 May 2022 15:01:50 +0200")

Patrick Steinhardt <ps@pks.im> writes:

>> 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.

It is not an argument against the command line option that you find
it more work to implement and cumbersome to plumb the information
through the callgraph, though.

Subcommands like "git commit" shouldn't have to know how host names
are mapped to IP addresses, and teaching the option only to
subcommands that the feature is relevant, and documenting the option
in their manual pages, would make it much easier to discover and
learn.

> 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 favor implementing this as a configuration that is primarily meant
to be used from the command line (i.e. "git -c var=val"), ONLY
BECAUSE the feature itself is not something that should be widely
used (the users should futz with their DNS if they need something
more permanent), and adding it as a configuration would be a more
quick and dirty way that needs less developer resources now ;-)

To purists, it may make more sense to add this feature and make it
accessible only from the command line without matching configuration
variable---that would enforce the assumed use case (i.e. only after
another part of the system asked DNS, performed some check on the
resulting IP address, and decided to ask Git to interact with that
URL, use this mechanism to ensure Git interacts with that IP address
that was vetted, to avoid TOCTOU mistakes) more clearly.

I am personally open to such a purer counterproposal with working
code ;-)

>> 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.
>
> 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, and
> there is a high chance that they might actually be parsing output of any
> such commands. Forcing them to always disable this advise doesn't feel
> like the right thing to do.

All correct.  If the users set configuration variables, the fact
that we honored their configuration variables settings and behaved
accordingly is *NOT* an advise-worthy event.

Thanks.

  parent reply	other threads:[~2022-05-12 15:59 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
2022-05-12 15:58         ` Junio C Hamano [this message]
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=xmqqa6bmhhe0.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=carenas@gmail.com \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=ps@pks.im \
    /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).