git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: git@vger.kernel.org, anmolmago@gmail.com, briankyho@gmail.com,
	david.lu97@outlook.com, shirui.wang@hotmail.com,
	f.francet@hotmail.com
Subject: Re: [RFC PATCH] remote: add --fetch option to git remote set-url
Date: Tue, 30 Oct 2018 19:11:06 +0900	[thread overview]
Message-ID: <xmqq8t2f218l.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20181030075653.GA4114@archbookpro.localdomain> (Denton Liu's message of "Tue, 30 Oct 2018 03:56:53 -0400")

Denton Liu <liu.denton@gmail.com> writes:

> On Mon, Oct 29, 2018 at 02:57:28PM +0900, Junio C Hamano wrote:
> ...
>> Earlier you had a check like this:
>> 
>> > +	if (push_mode && fetch_mode)
>> > +		die(_("--push --fetch doesn't make sense"));
>> 
>> If a request to "--fetch" is ignored when "--add" is given, for
>> example, shouldn't the combination also be diagnosed as "not making
>> sense, we'd ignore your wish to use the --fetch option"?  Similarly
>> for the case where there already is pushurl defined for the remote.

Clarification.  Here I am suggesting that a part of the logic in the
earlier assignment to move_fetch_to_push should become a similar
call to die(), detecting a competing and unsatisfiable wish, rather
than getting silently ignored.

>> This is a different tangent on the same line, but it could be that
>> the user wants to have two (or more) pushURLs because the user wants
>> to push to two remotes at the same time with "git push this-remote",
>> so silently ignoring "--force" may not be the right thing in the

Correction.  s/--force/--fetch/ was what I meant here.

>> first place.  We may instead need to make the value of URL to an
>> extra pushURL entry (if we had one, we now have two).

Also, additionally, since there is no use to have two or more URL,
because unlike "git push $there" that can push to two places,
fetching from two places into the same set of remote-tracking
branches would not make sense, --move-pushURL-to-URL-and-set-pushURL
operation that is the symmetry of what the patch under discussion
proposes should fail instead of creating an extra URL entry, breaking
an apparent symmetry.

> Perhaps I should motivate the use-case for this option. There have been
> times when I've had the URL set to something but no pushURL. I've
> wanted to only change the fetching URL and keep the pushing URL the same

URL (plus optionally pushURL) split was done because most everybody
were fetching from and pushing to the same place; it would not have
made any sense to have fetchURL and pushURL that are separate, as
that would have forced everybody to have both, when majority of the
users would have to set them to the same value.

Quite honestly, tweaking URL and/or pushURL is not something you'd
do every three months or more frequently, so I do not particularly
feel sympathetic to the cause of this patch, which would allow
setting one to the value that happens to be set to the other one
while setting an arbitrary new value to the other one.  Once the
user's need deviates from that single niche pattern, the user needs
to update both, and setting these two independently is quite simple
and straight-forward in the first place.  And that same simple pattern
to set two independently can be used without learning this new option.

> My implementation of --fetch is supposed to emulate what would happen if
> git were implemented with fetchurl/pushurl instead. Does the patch make
> more sense in this context?

Hmph.  As a system that has fetchURL and pushURL independently and
forces everybody to set both does not make much sense in the first
place, the patch making sense in that context is not a very strong
reason to support it.

> Please let me know if you think that the concept behind this patch is a
> good idea. Thanks!

If I don't then I do not have to let you know, then ;-) 

I do not particularly think it is a horrible idea, but I do not see
that it would be a feature that helps particuarly wide audience.

  reply	other threads:[~2018-10-30 10:11 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-27  8:09 [RFC PATCH] remote: add --fetch option to git remote set-url Denton Liu
2018-10-29  5:57 ` Junio C Hamano
2018-10-30  7:56   ` Denton Liu
2018-10-30 10:11     ` Junio C Hamano [this message]
2018-11-09  2:37       ` [RFC PATCH v2] remote: add --save-push " Denton Liu
2018-11-09  3:15         ` Junio C Hamano
2018-11-09  5:20           ` [PATCH v3] remote: add --save-to-push " Denton Liu
2018-11-13  9:46             ` Junio C Hamano
2018-12-10 14:15             ` [PATCH v4] " Denton Liu

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=xmqq8t2f218l.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=anmolmago@gmail.com \
    --cc=briankyho@gmail.com \
    --cc=david.lu97@outlook.com \
    --cc=f.francet@hotmail.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    --cc=shirui.wang@hotmail.com \
    /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).