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

Jeff King [off-list ref] writes:

quoted

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.

quoted

quoted

+add.updateroot::

Detail: option names are normally camelCased => updateRoot.

Good point, thanks.

quoted

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.

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.