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

From: Jeff King <peff@peff.net>
Date: Nov 19, 2014, 20:17 UTC
Message-ID: <20141119201721.GC10361@peff.net>
In-Reply-To: <1456931.yaNzKr1t0X@al>

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

Show 10 quoted lines

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

Show 11 quoted lines

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