Re: [PATCH v2 2/2] stash: remove the stash.useBuiltin setting

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Mar 4, 2020, 11:49 UTC
Message-ID: <20200304114909.GD1571684@cat>
In-Reply-To: <xmqqeeu98egp.fsf@gitster-ct.c.googlers.com>

On 03/03, Junio C Hamano wrote:

Show 5 quoted lines

> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > +	Unused configuration Variable.  Used in Git versions 2.22 to
> 
> s/Variable/variable/;

Whoops. Do you want me to re-send an updated version, or could you fix this up locally?

Show 13 quoted lines

> > +	2.26 as an escape hatch to enable the legacy shellscript
> > +	implementation of rebase. Now the built-in rewrite of it in C
> > +	is always used. Setting this will emit a warning, to alert any
> > +	remaining users that setting this now does nothing.
> 
> I am a bit torn between "just document it here as a stale variable
> that can safely be removed from your configuration, and do not warn"
> and what you did here.  Even if we warn for a few releases, this
> entry cannot be removed from the documentation for a lot longer
> time, if ever, anyway.  It's not like having an extra entry that is
> unused in the configuration file would hurt users, anyway, and it is
> not obvious that the warning helps more than it disrupts them by
> bugging users to remove it from their configuration file.

Hmm I'm thinking of the warning mostly as preventing the "I have legacy stash enabled, why do I see a new bug all of a sudden?" question, as opposed to it helping removing this from the docs. I suspect we have very few people in the wild that did enable legacy stash, but if they did it might be better for them to know about the config variable not having any effect anymore.

FWIW we went with this same approach with removing the 'rebase.useBuiltin' config, and I don't think we heard any complaints about it in that transition.