On Sat, Feb 15, 2020 at 09:36:41PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> 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@xxxxxxxxx>
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
