Re: [PATCH 2/2] add: respect add.updateroot config option

From: Jeff King <peff@peff.net>
Date: Mar 13, 2013, 09:27 UTC
Message-ID: <20130313092754.GA12658@sigill.intra.peff.net>
In-Reply-To: <vpqr4jjd5wp.fsf@grenoble-inp.fr>

On Wed, Mar 13, 2013 at 10:07:34AM +0100, Matthieu Moy wrote:

Show 10 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > For users who have read and accepted the warning, there is no way to
> > jump directly to the future behavior and silence the warning.
> 
> I think the idea makes sense. The transition period is necessary for
> people who use different versions of Git (which includes anybody
> writting and distributing scripts), but for poor mortals who only use a
> single version of Git, it's nice to be able to jump to the future
> behavior once for all as soon as possible.

I think the biggest risk is from people who think they are safe to jump, and then find out that some script they depend on is not ready. Even if they do not even realize they are relying on it. Part of the point of the transition period is to get script authors to update their scripts, and to let the new versions trickle down to the users.

> Your patch doesn't advertise the option in the warning message, which I
> think is good. You may mention it the commit message that this is a
> deliberate choice.

Yes, it was deliberate. I can add a note.

> > +add.updateroot::
> 
> Detail: option names are normally camelCased => updateRoot.

Good point, thanks.

Show 8 quoted lines

> I think the option name needs a bit more thinking. Without reading the
> doc,
> 
> [add]
> 	updateRoot = false
> 
> would be a very tempting thing to try. Perhaps explicitly warning when
> add.updateroot is set to false would avoid possible confusion.

Yeah, that occurred to me, too, hence the note in the doc. Since it isn't advertised elsewhere, I had hoped that anybody who discovered it would see the note. I suppose we can warn when we see add.updateRoot set to anything but true. That feels a bit hacky, as it's possible the user could be overriding an earlier setting (although that is getting kind of crazy).

> I liked David's suggestion of using future.* to mean "start using
> behavior from the future right now".

I do like that idea, as it makes the meaning of the variable more clear.

I dunno. I am not all that excited about this patch, due to all of the caveats that we need to communicate to the user. The current warning has annoyed me a few times, but perhaps it is still too soon, and my fingers and brain just need retraining. I think a config option could help some people, but maybe it will end up hurting more. I'm inclined at this point to table the patch for now and see how I feel in a few weeks.

I do think patch 1/2 makes sense regardless.

-Peff