Re: [PATCH v5 20/20] rebase: rename the two primary rebase backends

[Emily Shaffer <emilyshaffer@google.com>]

On Sat, Feb 15, 2020 at 09:36:41PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> Two related changes, with separate rationale for each:
> 
> Rename the 'interactive' backend to 'merge' because:
>   * 'interactive' as a name caused confusion; this backend has been used
>     for many kinds of non-interactive rebases, and will probably be used
>     in the future for more non-interactive rebases than interactive ones
>     given that we are making it the default.
>   * 'interactive' is not the underlying strategy; merging is.
>   * the directory where state is stored is not called
>     .git/rebase-interactive but .git/rebase-merge.
> 
> Rename the 'am' backend to 'apply' because:
>   * Few users are familiar with git-am as a reference point.
>   * Related to the above, the name 'am' makes sentences in the
>     documentation harder for users to read and comprehend (they may read
>     it as the verb from "I am"); avoiding this difficult places a large
>     burden on anyone writing documentation about this backend to be very
>     careful with quoting and sentence structure and often forces
>     annoying redundancy to try to avoid such problems.
>   * Users stumble over pronunciation ("am" as in "I am a person not a
>     backend" or "am" as in "the first and thirteenth letters in the
>     alphabet in order are "A-M"); this may drive confusion when one user
>     tries to explain to another what they are doing.
>   * While "am" is the tool driving this backend, the tool driving git-am
>     is git-apply, and since we are driving towards lower-level tools
>     for the naming of the merge backend we may as well do so here too.
>   * The directory where state is stored has never been called
>     .git/rebase-am, it was always called .git/rebase-apply.
> 
> For all the reasons listed above:
>   * Modify the documentation to refer to the backends with the new names
>   * Provide a brief note in the documentation connecting the new names
>     to the old names in case users run across the old names anywhere
>     (e.g. in old release notes or older versions of the documentation)
>   * Change the (new) --am command line flag to --apply
>   * Rename some enums, variables, and functions to reinforce the new
>     backend names for us as well.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>

Hi,

This broke quite a few upstream users for us today when we rolled out a
next with this patch added on top. To shim around the post-commit hook
issue, we had set a system config for all our users to use
merge.backend=am; the machinery is pretty intolerant to a wrongly
configured backend name (die() rather than a warning and fallback).

Would it make more sense to deal with an unrecognized backend by falling
back to the default backend, instead?

>  	if (options.type == REBASE_UNSPECIFIED) {
>  		if (!strcmp(options.default_backend, "merge"))
> -			imply_interactive(&options, "--merge");
> -		else if (!strcmp(options.default_backend, "am"))
> -			options.type = REBASE_AM;
> +			imply_merge(&options, "--merge");
> +		else if (!strcmp(options.default_backend, "apply"))
> +			options.type = REBASE_APPLY;
>  		else
>  			die(_("Unknown rebase backend: %s"),
>  			    options.default_backend);

At the very least, can this die() explain that it found that string in
the config so the user can have a guess as to how to fix it?

(I realize the complained code is from earlier in the series, but this
patch - renaming something that used to be valid without a fallback -
invalidated our configs, highlighting the problem for us. So I'm
replying here instead.)

 - Emily