git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Christian Couder <christian.couder@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git <git@vger.kernel.org>,
	Christian Couder <chriscool@tuxfamily.org>,
	Derrick Stolee <derrickstolee@github.com>,
	Jacob Vosmaer <jacob@gitlab.com>
Subject: Re: [PATCH] http: add custom hostname to IP address resolves
Date: Thu, 5 May 2022 12:48:50 +0200	[thread overview]
Message-ID: <CAP8UFD0V6scSD8LuUiRQ0BWmG2mFATVFNQ2DgwaWFp6bigBkZw@mail.gmail.com> (raw)
In-Reply-To: <xmqqzgjxnz73.fsf@gitster.g>

On Wed, May 4, 2022 at 4:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

> > I am Ok with just "http.resolve". I think using "curlopt" is perhaps
> > going into too many details about the implementation of the feature,
> > which could theoretically change if we ever decided to use something
> > other than curl.
>
> You may want to step back a bit and rethink.
>
> Even if we decide to rewrite that part of the system not to depend
> on cURL, end-user facing documented interface, i.e. how the mappings
> are given to the system, will stay with us, and it is clear that it
> was modeled after CURLOPT_RESOLVE---well, it was stolen from them
> verbatim ;-).
>
> So we may wean ourselves off of cURL, but CURLOPT_RESOLVE will stay
> with us for this particular feature.

Yeah, the CURLOPT_RESOLVE format will stay with us, so Ok, I will
rename it "http.curloptResolve" in the next iteration then.

> >> I am wondering if we want to mention the expected use case here
> >> as well, something like
> >>
> >>     This is designed to be used primarily from the command line
> >>     configuration variable override, e.g.
> >>
> >>         $ git -c http.resolve=example.com:443:127.0.0.1 \
> >>             clone https://example.com/user/project.git
> >>
> >> perhaps?  Not a suggestion, but soliciting thoughts.
> >
> > I am also interested in others' thoughts about this. If no one thinks
> > that a config option could be useful, I am Ok with making it a
> > "--resolve" command line option that can be passed to any Git command
> > similar to "-c <name>=<value>":
> >
> > git --resolve=... <command> [<args>]
>
> Absolutely not.
>
> "git [push|fetch|clone|ls-remote] --dns-pre-resolve=..." that is
> *NOT* git wide, but is only for transport commands might be a
> possibility, but even then, you'd need to invent a way to do the
> same for non cURL transports (we want to be able to pin the IP when
> going over SSH to a certain host, for the same reason) if we promote
> it to an officially supported command line option.

Ok with renaming and implementing it only in transport commands. I
don't want, and I don't think it should be necessary, to invent a way
to do the same for non cURL transports though. I think it should be Ok
with the doc saying that the option has only been implemented for
HTTP(S) yet and will have no effect when other transports are used.

If there is a simple way to do the same thing for ssh, then I might
take a look at it later. For "file" or bundle transports, I don't
think it makes sense, and the "git" transport is not used much in big
hosting services where this feature is likely to be used.

> Unless we do that, it is probably better to leave it as an obscure
> configuration meant to help server operators.  At least, with the
> name of the configuration variable prefixed with http.*, we have a
> valid excuse when somebody complains "the feature does not do
> anything for git:// transport".

I am happy with leaving it as an obscure configuration meant to help
server operators. So I will just rename it "http.curloptResolve" in
the next iteration.

Thanks!

  reply	other threads:[~2022-05-05 10:49 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-02  8:36 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 [this message]
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
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=CAP8UFD0V6scSD8LuUiRQ0BWmG2mFATVFNQ2DgwaWFp6bigBkZw@mail.gmail.com \
    --to=christian.couder@gmail.com \
    --cc=chriscool@tuxfamily.org \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jacob@gitlab.com \
    --subject='Re: [PATCH] http: add custom hostname to IP address resolves' \
    /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).