Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Jeff King <peff@peff.net>]

On Wed, Nov 19, 2014 at 08:42:19PM +0100, Peter Wu wrote:

> >   git remote set-url --push gh $(git config remote.gh.url)
> >   git remote set-url gh new-fetch-url
> 
> Indeed, and not having a push URL is not uncommon (that is, always the
> case when a new remote is added or just cloned). Compare the above
> against this one:
> 
>     git remote set-url --fetch new-fetch-url
> 
> This is less verbose and is much more intuitive.

I agree your suggestion is a nicer way to do this. I'm just not sure
that this swapping is all that common an operation. If there were no
cost, I wouldn't mind. But I'm mostly concerned about the funny,
non-intuitive behavior of "git remote set-url foo" that is created by
this.

> > would replace both the "url" _and_ "pushurl" values in the third step,
> > since we did not specify --fetch.  But it is in fact identical whether
> > you run it with "--fetch" or not.  That is, it creates a weirdly
> > non-orthogonal interface.
> 
> Step three currently only replaces the fetch URL as an explicit push URL
> (remote.gh.pushurl) is set in step two (and therefore remote.gh.url does
> not become the implicit push URL).
> 
> This might be a bug, but since it has been so long this way I was
> worried that people actually rely on this behavior.

I don't think this is a bug. I think that "git fetch set-url" without
"--push" is a de-facto "--fetch" already. Which makes sense, as there
isn't a "--fetch" now (and the "--push" variant and "pushurl" grew after
the fact, so the "url" option serves double-duty as both the single url
and the "fetch" half).

And that's what makes the proposed interface funny. Omitting "--fetch"
is already a de-facto "--fetch", and sometimes the two behave the same,
and sometimes differently. Calling the option "--keep-push" would be a
more accurate description, but that is rather clunky.

> The patch is not tiny, but also not overly large either even if it has
> similar pieces of code duplicated. Care has been taken of to keep
> backwards compatibility which has also increased the size.

Yeah, sorry, I shouldn't have even mentioned patch size. I appreciate
your attention to backwards-compatibility, and the patch should be as
large as it needs to safely implement the desired behavior. It's really
the resulting interface that I'm a bit negative on (which can be related
to code size, in the sense that a simple interface can often be
implemented simply, but here we have to deal with historical corner
cases).

So I dunno. I do not think what you are proposing is the end of the
world, but it would be nice to resolve the inconsistency I mentioned.
Perhaps we can see what others think.

> By the way, in the old code there was a memleak as strbuf was not
> released at the end of the function, isn't it?

Yes. We are often quite lazy about such leaks in functions that are
known to return straight to the program exit, but it does not hurt to
clean up here.

-Peff