Re: [PATCH 2/2] add: respect add.updateroot config option
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 13, 2013, 15:51 UTC
- Message-ID
- <7vmwu747tf.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20130313092754.GA12658@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 20 quoted lines
>> 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. > >> 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.
This is essentially the same as Matthieu's add.use2dot0Behavior but with that "it is an error to explicitly set it to false" twist, it alleviates one of the worries. It still has the same "the user saw it mentioned on stackoverflow and sets it without understanding that the behaviour is getting changed" effect.
Also it won't give chance for scripts to get fixed, even though I suspect that instances of "add -u/-A" without pathspec end users write in their custom scripts almost always would want to be tree-wide and it is a bug that they do not pass ":/" to them.
The "future.*" (I'd rather call that "migration.*") approach with the "never set it to false" twist is probably OK from the "complaint avoidance" perspective. The user cannot later complain "I tried to squelch the advice but at the same time I ended up adding updated contents outside my diretory", without getting told "That is the documented behaviour, isn't it?" But I still think it is much less nice from "avoid hurting the users" perspective. If the way to squelch the user learns were to say "git add -u .", where the user explicitly says "take the updated contents from this directory and below", there is no room for confusion. We won't hear complaints either way, but with the "future.*" the reason why we won't hear complaints is because the users do not have excuse to complain. With the "require explicit .", it is because the users won't get hurt in the first place.
Show 8 quoted lines
> 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.
These two concluding paragraphs match my current thinking.
Thanks.