From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id DA21C1F466 for ; Fri, 17 Jan 2020 16:58:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728949AbgAQQ66 (ORCPT ); Fri, 17 Jan 2020 11:58:58 -0500 Received: from mail-wr1-f68.google.com ([209.85.221.68]:45885 "EHLO mail-wr1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726559AbgAQQ65 (ORCPT ); Fri, 17 Jan 2020 11:58:57 -0500 Received: by mail-wr1-f68.google.com with SMTP id j42so23366350wrj.12 for ; Fri, 17 Jan 2020 08:58:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=reply-to:subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=Hk32CW0YKLzgW48E6VUjdU2qPZ2RnaIG0tnk3FxOEIA=; b=s713YbZZgvz44ctkx4mHGgcNmtz8bKCFoJ9cThGkvZUga15bNPZr5NBtfqebAqfX2T 89oaC4/OV9GxB0+suCAzvh79Izd1qmYrztAvlG8XNZFxbuAOYLWeQ45RgeCJM80VroYu xv8ALPJ0GZpjaGvMc6Hv1525YPqXqD7cZghGP9qh66+9IL89d9VczWASr6Lq/FB8LaAN mbwqI0Y1KwjDbHtFSir6kiP0ely5JCf0TfAdmTZcHQvFXzB0AUzg2muJxIMBqxRffHA1 aB6IvmH4NdeMMJ26y0Yrn3xBdCswM7bUWDBDW0e2pETzDtTknYJcuToR/ggpDt6Dz2zp iCOw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:reply-to:subject:to:cc:references:from :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=Hk32CW0YKLzgW48E6VUjdU2qPZ2RnaIG0tnk3FxOEIA=; b=QhQ26lq/a63U8ZpDR3WfSeOjfL8g93/nuaPfXJOrE0OWTynho2qDCXjUJfgV6eB0OY 2+MXOXLYx9SlBCjwf164WbvnYggVyIj5slqJ3nupZUWT8yS7W/+8AX+VyriINrWiTVTw 077Yq+VuoCgNGfHjsKRS2pu3wYdQd/ROHKSoEYehg7BlhTSRkgSI9YlI05fF+zXVSty5 ovpoHwWOUqFYHVYyRKozOpnW14akEWvwHjH4nt+L8PU3mMod7sbQOv7gCHsQLScJUjJl VW/lXksuMq7O2fTJddvLIr5IkcY3xCWQCGXZPfNyL3pjwXmh1niOEB5KsKIBM3u1KQ9j qUJQ== X-Gm-Message-State: APjAAAUDbfxWdytFByklbgeKBZy8UUJjYUuwYFVlHV5FKD8zZcRTThm+ dHTQO+19D3vT2V+qpB3nkEE= X-Google-Smtp-Source: APXvYqxjQvWjqG06V1y3l6eglO1NzGSKTbg7VHMcUiiWd8Lzua81p/xLFgqyPJts8IC8aeDNXVsqJw== X-Received: by 2002:a5d:6652:: with SMTP id f18mr4253530wrw.246.1579280331707; Fri, 17 Jan 2020 08:58:51 -0800 (PST) Received: from [192.168.2.240] (host-92-22-19-5.as13285.net. [92.22.19.5]) by smtp.gmail.com with ESMTPSA id g18sm9966553wmh.48.2020.01.17.08.58.44 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 17 Jan 2020 08:58:51 -0800 (PST) Reply-To: phillip.wood@dunelm.org.uk Subject: Re: [PATCH v4 00/19] rebase: make the default backend configurable To: Elijah Newren via GitGitGadget , git@vger.kernel.org Cc: Johannes.Schindelin@gmx.de, phillip.wood@dunelm.org.uk, liu.denton@gmail.com, gitster@pobox.com, plroskin@gmail.com, alban.gruin@gmail.com, szeder.dev@gmail.com, jrnieder@gmail.com, emilyshaffer@google.com, Elijah Newren References: From: Phillip Wood Message-ID: <47e7c9e6-7d83-185d-792d-f8e084c1a7a1@gmail.com> Date: Fri, 17 Jan 2020 16:58:31 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB-large Content-Transfer-Encoding: 7bit Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Hi Elijah On 16/01/2020 06:14, Elijah Newren via GitGitGadget wrote: > This is a re-roll of en/rebase-backend which has been rebased on v2.25.0 and > updated to remove the dependence on ra/rebase-i-more-options, and also tries > to address feedback from Phillip, Junio, and Jonathan. This series does a > lot of work around making the default rebase backend configurable, and > switches the default from the am backend to the merge/interactive one. > > Changes since v3: > > * Rebased on origin/master and updated to remove the dependence on > ra/rebase-i-more-options. > * Added two new patches at the start of the series. > * Split the old first patch into two, while modifying them based on > Phillip's feedback (though slightly differently than discussed on the > list; instead of making --keep-empty a synonym for --empty=keep, I > instead kept backward compatibility by making it a no-op). > * I noted the post-commit hook in the differences between backends. Emily > is investigating what changes need to happen there, so I merely > documented the existing differences. > * dropped '-i' from the reflog messages; now they just refer to 'rebase' > > Changes possibly missing from this version, for discussion: > > * I did not remove the --am option as suggested by Phillip, since Junio and > Phillip were still discussing whether it is wanted/needed. > * I did not address the last two items Jonathan brought up as I couldn't > find enough information to reproduce or understand the problems. I think I've got a fix for the failure with --committer-date-is-author-date that Jonathan reported which I'll post next week - the bug was not in this series, it was just exposed by it. I'll try and read through this series next week as well. Best Wishes Phillip > > Elijah Newren (19): > git-rebase.txt: update description of --allow-empty-message > t3404: directly test the behavior of interest > rebase (interactive-backend): make --keep-empty the default > rebase (interactive-backend): fix handling of commits that become > empty > t3406: simplify an already simple test > rebase, sequencer: remove the broken GIT_QUIET handling > rebase: make sure to pass along the quiet flag to the sequencer > rebase: fix handling of restrict_revision > t3432: make these tests work with either am or merge backends > rebase: allow more types of rebases to fast-forward > git-rebase.txt: add more details about behavioral differences of > backends > rebase: move incompatibility checks between backend options a bit > earlier > rebase: add an --am option > git-prompt: change the prompt for interactive-based rebases > rebase: drop '-i' from the reflog for interactive-based rebases > rebase tests: mark tests specific to the am-backend with --am > rebase tests: repeat some tests using the merge backend instead of am > rebase: make the backend configurable via config setting > rebase: change the default backend from "am" to "merge" > > Documentation/config/rebase.txt | 8 ++ > Documentation/git-rebase.txt | 150 +++++++++++++++++--- > builtin/rebase.c | 186 +++++++++++++++++++------ > contrib/completion/git-prompt.sh | 6 +- > rebase-interactive.c | 7 +- > rebase-interactive.h | 2 +- > sequencer.c | 84 ++++++----- > sequencer.h | 3 +- > t/t3400-rebase.sh | 36 ++++- > t/t3401-rebase-and-am-rename.sh | 4 +- > t/t3404-rebase-interactive.sh | 19 +-- > t/t3406-rebase-message.sh | 19 ++- > t/t3407-rebase-abort.sh | 6 +- > t/t3420-rebase-autostash.sh | 2 +- > t/t3421-rebase-topology-linear.sh | 16 +-- > t/t3424-rebase-empty.sh | 108 ++++++++++++++ > t/t3425-rebase-topology-merges.sh | 8 +- > t/t3427-rebase-subtree.sh | 12 +- > t/t3432-rebase-fast-forward.sh | 54 ++++--- > t/t5407-post-rewrite-hook.sh | 12 +- > t/t5520-pull.sh | 27 +++- > t/t6047-diff3-conflict-markers.sh | 13 +- > t/t7512-status-help.sh | 12 +- > t/t9106-git-svn-commit-diff-clobber.sh | 3 +- > t/t9903-bash-prompt.sh | 8 +- > 25 files changed, 595 insertions(+), 210 deletions(-) > create mode 100755 t/t3424-rebase-empty.sh > > > base-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783 > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-679%2Fnewren%2Frebase-fixes-v4 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-679/newren/rebase-fixes-v4 > Pull-Request: https://github.com/git/git/pull/679 > > Range-diff vs v3: > > -: ---------- > 1: 3ea48d5394 git-rebase.txt: update description of --allow-empty-message > -: ---------- > 2: 10fdd162a0 t3404: directly test the behavior of interest > 1: 1c2b77e94d ! 3: 179f82ab83 rebase: extend the options for handling of empty commits > @@ -1,49 +1,46 @@ > Author: Elijah Newren > > - rebase: extend the options for handling of empty commits > + rebase (interactive-backend): make --keep-empty the default > > - Extend the interactive machinery with the ability to handle the full > - spread of options for how to handle commits that either start or become > - empty (by "become empty" I mean the changes in a commit are a subset of > - changes that exist upstream, so the net effect of applying the commit is > - no changes). Introduce a new command line flag for selecting the > - desired behavior: > - --empty={drop,keep,ask} > - with the definitions: > - drop: drop empty commits > - keep: keep empty commits > - ask: provide the user a chance to interact and pick what to do with > - empty commits on a case-by-case basis > + Different rebase backends have different treatment for commits which > + start empty (i.e. have no changes relative to their parent), and the > + --keep-empty option was added at some point to allow adjusting behavior > + for the interactive backend. The handling of commits which start empty > + is actually quite similar to commit b00bf1c9a8dd (git-rebase: make > + --allow-empty-message the default, 2018-06-27), which pointed out that > + the behavior for various backends is often more happenstance than > + design. The specific change made in that commit is actually quite > + relevant as well and much of the logic there directly applies here. > > - Note that traditionally, am-based rebases have always dropped commits > - that either started or became empty, while interactive-based rebases > - have defaulted to ask (and provided an option to keep commits that > - started empty). This difference made sense since users of an am-based > - rebase just wanted to quickly batch apply a sequence of commits, while > - users editing a todo list will likely want the chance to interact and > - handle unusual cases on a case-by-case basis. However, not all rebases > - using the interactive machinery are explicitly interactive anymore. In > - particular --merge was always meant to behave more like --am: just > - rebase a batch of commits without popping up a todo list. > + It makes a lot of sense in 'git commit' to error out on the creation of > + empty commits, unless an override flag is provided. However, once > + someone determines that there is a rare case that merits using the > + manual override to create such a commit, it is somewhere between > + annoying and harmful to have to take extra steps to keep such > + intentional commits around. Granted, empty commits are quite rare, > + which is why handling of them doesn't get considered much and folks tend > + to defer to existing (accidental) behavior and assume there was a reason > + for it, leading them to just add flags (--keep-empty in this case) that > + allow them to override the bad defaults. Fix the interactive backend so > + that --keep-empty is the default, much like we did with > + --allow-empty-message. The am backend should also be fixed to have > + --keep-empty semantics for commits that start empty, but that is not > + included in this patch other than a testcase documenting the failure. > > - If the --empty flag is not specified, pick defaults as follows: > - explicitly interactive: ask > - --exec: keep (exec is about checking existing commits, and often > - used without actually changing the base. Thus the > - expectation is that the user doesn't necessarily want > - anything to change; they just want to test). > - otherwise: drop > - > - Also, this commit makes --keep-empty just imply --empty=keep, and hides > - it from help so that we aren't confusing users with different ways to do > - the same thing. (I could have added a --drop-empty flag, but then that > - invites users to specify both --keep-empty and --drop-empty and we have > - to add sanity checking around that; it seems cleaner to have a single > - multi-valued option.) This actually fixes --keep-empty too; previously, > - it only meant to sometimes keep empty commits, in particular commits > - which started empty would be kept. But it would still error out and ask > - the user what to do with commits that became empty. Now it keeps empty > - commits, as instructed. > + Note that there was one test in t3421 which appears to have been written > + expecting --keep-empty to not be the default as correct behavior. This > + test was introduced in commit 00b8be5a4d38 ("add tests for rebasing of > + empty commits", 2013-06-06), which was part of a series focusing on > + rebase topology and which had an interesting original cover letter at > + https://lore.kernel.org/git/1347949878-12578-1-git-send-email-martinvonz@gmail.com/ > + which noted > + Your input especially appreciated on whether you agree with the > + intent of the test cases. > + and then went into a long example about how one of the many tests added > + had several questions about whether it was correct. As such, I believe > + most the tests in that series were about testing rebase topology with as > + many different flags as possible and were not trying to state in general > + how those flags should behave otherwise. > > Signed-off-by: Elijah Newren > > @@ -51,126 +48,62 @@ > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ > - original branch. The index and working tree are also left > unchanged as a result. > > -+--empty={drop,keep,ask}:: > -+ How to handle commits that become empty (because they contain a > -+ subset of already upstream changes) or start empty. With drop > -+ (the default), commits that start or become empty are dropped. > -+ With keep (implied by --exec), such commits are kept. With ask > -+ (implied by --interactive), the rebase will halt when an empty > -+ commit is applied allowing you to choose whether to drop it or > -+ commit it. Also with ask, if the rebase is interactive then > -+ commits which start empty will be commented out in the todo > -+ action list (giving you a chance to uncomment). > -++ > -+Note that this has no effect on commits which are already upstream (as > -+can be checked via `git log --cherry-mark ...`), which are always > -+dropped by rebase. > -++ > -+See also INCOMPATIBLE OPTIONS below. > -+ > --keep-empty:: > - Keep the commits that do not change anything from its > - parents in the result. > -+ Deprecated alias for what is now known as --empty=keep. > ++ No-op. Rebasing commits that started empty (had no change > ++ relative to their parent) used to fail and this option would > ++ override that behavior, allowing commits with empty changes to > ++ be rebased. Now commits with no changes do not cause rebasing > ++ to halt. > + > - See also INCOMPATIBLE OPTIONS below. > - > -@@ > - * --interactive > - * --exec > - * --keep-empty > -+ * --empty= > - * --edit-todo > - * --root when used in combination with --onto > - > -@@ > - * --preserve-merges and --ignore-whitespace > - * --preserve-merges and --committer-date-is-author-date > - * --preserve-merges and --ignore-date > -+ * --preserve-merges and --empty= > - * --keep-base and --onto > - * --keep-base and --root > +-See also INCOMPATIBLE OPTIONS below. > ++See also BEHAVIORAL DIFFERENCES and INCOMPATIBLE OPTIONS below. > > + --allow-empty-message:: > + No-op. Rebasing commits with an empty message used to fail > @@ > + Empty commits > + ~~~~~~~~~~~~~ > > - There are some subtle differences how the backends behave. > - > --Empty commits > --~~~~~~~~~~~~~ > -- > -The am backend drops any "empty" commits, regardless of whether the > -commit started empty (had no changes relative to its parent to > -start with) or ended empty (all changes were already applied > -upstream in other commits). > -- > ++The am backend unfortunately drops intentionally empty commits, i.e. > ++commits that started empty, though these are rare in practice. It > ++also drops commits that become empty and has no option for controlling > ++this behavior. > + > -The interactive backend drops commits by default that > -started empty and halts if it hits a commit that ended up empty. > -The `--keep-empty` option exists for the interactive backend to allow > -it to keep commits that started empty. > -- > ++The interactive backend keeps intentionally empty commits. > ++Unfortunately, it always halts whenever it runs across a commit that > ++becomes empty, even when the rebase is not explicitly interactive. > + > Directory rename detection > ~~~~~~~~~~~~~~~~~~~~~~~~~~ > - > > diff --git a/builtin/rebase.c b/builtin/rebase.c > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > -@@ > - REBASE_PRESERVE_MERGES > - }; > - > -+enum empty_type { > -+ EMPTY_UNSPECIFIED = -1, > -+ EMPTY_DROP, > -+ EMPTY_KEEP, > -+ EMPTY_ASK > -+}; > -+ > - struct rebase_options { > - enum rebase_type type; > -+ enum empty_type empty; > - const char *state_dir; > - struct commit *upstream; > - const char *upstream_name; > @@ > const char *action; > int signoff; > int allow_rerere_autoupdate; > - int keep_empty; > int autosquash; > - int ignore_whitespace; > char *gpg_sign_opt; > -@@ > - > - #define REBASE_OPTIONS_INIT { \ > - .type = REBASE_UNSPECIFIED, \ > -+ .empty = EMPTY_UNSPECIFIED, \ > - .flags = REBASE_NO_QUIET, \ > - .git_am_opts = ARGV_ARRAY_INIT, \ > - .git_format_patch_opt = STRBUF_INIT \ > -@@ > - replay.allow_rerere_auto = opts->allow_rerere_autoupdate; > - replay.allow_empty = 1; > - replay.allow_empty_message = opts->allow_empty_message; > -+ replay.drop_redundant_commits = (opts->empty == EMPTY_DROP); > -+ replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP); > -+ replay.ask_on_initially_empty = (opts->empty == EMPTY_ASK && > -+ !(opts->flags & REBASE_INTERACTIVE_EXPLICIT)); > - replay.verbose = opts->flags & REBASE_VERBOSE; > - replay.reschedule_failed_exec = opts->reschedule_failed_exec; > - replay.committer_date_is_author_date = > + int autostash; > @@ > > git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands); > > - flags |= opts->keep_empty ? TODO_LIST_KEEP_EMPTY : 0; > -+ flags |= (opts->empty == EMPTY_DROP) ? TODO_LIST_DROP_EMPTY : 0; > -+ flags |= (opts->empty == EMPTY_ASK && > -+ opts->flags & REBASE_INTERACTIVE_EXPLICIT) ? > -+ TODO_LIST_ASK_EMPTY : 0; > flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0; > flags |= opts->rebase_merges ? TODO_LIST_REBASE_MERGES : 0; > flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0; > @@ -183,10 +116,8 @@ > +{ > + struct rebase_options *opts = opt->value; > + > -+ BUG_ON_OPT_NEG(unset); > + BUG_ON_OPT_ARG(arg); > + > -+ opts->empty = EMPTY_KEEP; > + opts->type = REBASE_INTERACTIVE; > + return 0; > +} > @@ -201,62 +132,28 @@ > - OPT_BOOL(0, "keep-empty", &opts.keep_empty, N_("keep empty commits")), > + { OPTION_CALLBACK, 'k', "keep-empty", &options, NULL, > + N_("(DEPRECATED) keep empty commits"), > -+ PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, > ++ PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, > + parse_opt_keep_empty }, > - OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message, > - N_("allow commits with empty messages")), > - OPT_BOOL(0, "rebase-merges", &opts.rebase_merges, N_("rebase merge commits")), > + OPT_BOOL_F(0, "allow-empty-message", &opts.allow_empty_message, > + N_("allow commits with empty messages"), > + PARSE_OPT_HIDDEN), > @@ > opts->allow_rerere_autoupdate ? > opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE ? > "--rerere-autoupdate" : "--no-rerere-autoupdate" : ""); > - add_var(&script_snippet, "keep_empty", opts->keep_empty ? "yes" : ""); > -+ add_var(&script_snippet, "empty", opts->empty == EMPTY_KEEP ? "yes" : ""); > add_var(&script_snippet, "autosquash", opts->autosquash ? "t" : ""); > add_var(&script_snippet, "gpg_sign_opt", opts->gpg_sign_opt); > add_var(&script_snippet, "cmd", opts->cmd); > -@@ > - return 0; > - } > - > -+static enum empty_type parse_empty_value(const char *value) > -+{ > -+ if (!strcasecmp(value, "drop")) > -+ return EMPTY_DROP; > -+ else if (!strcasecmp(value, "keep")) > -+ return EMPTY_KEEP; > -+ else if (!strcasecmp(value, "ask")) > -+ return EMPTY_ASK; > -+ > -+ die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value); > -+} > -+ > -+static int parse_opt_empty(const struct option *opt, const char *arg, int unset) > -+{ > -+ struct rebase_options *options = opt->value; > -+ enum empty_type value = parse_empty_value(arg); > -+ > -+ BUG_ON_OPT_NEG(unset); > -+ > -+ options->empty = value; > -+ return 0; > -+} > -+ > - static void NORETURN error_on_missing_default_upstream(void) > - { > - struct branch *current_branch = branch_get(NULL); > @@ > "ignoring them"), > REBASE_PRESERVE_MERGES, PARSE_OPT_HIDDEN), > OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate), > - OPT_BOOL('k', "keep-empty", &options.keep_empty, > - N_("preserve empty commits during rebase")), > -+ OPT_CALLBACK_F(0, "empty", &options, N_("{drop,keep,ask}"), > -+ N_("how to handle empty commits"), > -+ PARSE_OPT_NONEG, parse_opt_empty), > + { OPTION_CALLBACK, 'k', "keep-empty", &options, NULL, > + N_("(DEPRECATED) keep empty commits"), > -+ PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, > ++ PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, > + parse_opt_keep_empty }, > OPT_BOOL(0, "autosquash", &options.autosquash, > N_("move commits that begin with " > @@ -267,26 +164,10 @@ > > - if (options.keep_empty) > - imply_interactive(&options, "--keep-empty"); > -+ if (options.empty != EMPTY_UNSPECIFIED) > -+ imply_interactive(&options, "--empty"); > - > +- > if (gpg_sign) { > free(options.gpg_sign_opt); > -@@ > - break; > - } > - > -+ if (options.empty == EMPTY_UNSPECIFIED) { > -+ if (options.flags & REBASE_INTERACTIVE_EXPLICIT) > -+ options.empty = EMPTY_ASK; > -+ else if (exec.nr > 0) > -+ options.empty = EMPTY_KEEP; > -+ else > -+ options.empty = EMPTY_DROP; > -+ } > - if (reschedule_failed_exec > 0 && !is_interactive(&options)) > - die(_("--reschedule-failed-exec requires " > - "--exec or --interactive")); > + options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign); > > diff --git a/rebase-interactive.c b/rebase-interactive.c > --- a/rebase-interactive.c > @@ -296,19 +177,22 @@ > } > > -void append_todo_help(unsigned keep_empty, int command_count, > -+void append_todo_help(unsigned no_ask_empty, int command_count, > ++void append_todo_help(int command_count, > const char *shortrevisions, const char *shortonto, > struct strbuf *buf) > { > @@ > + "the rebase will be aborted.\n\n"); > > strbuf_add_commented_lines(buf, msg, strlen(msg)); > - > +- > - if (!keep_empty) { > -+ if (!no_ask_empty) { > - msg = _("Note that empty commits are commented out"); > - strbuf_add_commented_lines(buf, msg, strlen(msg)); > - } > +- msg = _("Note that empty commits are commented out"); > +- strbuf_add_commented_lines(buf, msg, strlen(msg)); > +- } > + } > + > + int edit_todo_list(struct repository *r, struct todo_list *todo_list, > > diff --git a/rebase-interactive.h b/rebase-interactive.h > --- a/rebase-interactive.h > @@ -318,7 +202,7 @@ > struct todo_list; > > -void append_todo_help(unsigned keep_empty, int command_count, > -+void append_todo_help(unsigned no_ask_empty, int command_count, > ++void append_todo_help(int command_count, > const char *shortrevisions, const char *shortonto, > struct strbuf *buf); > int edit_todo_list(struct repository *r, struct todo_list *todo_list, > @@ -327,157 +211,51 @@ > --- a/sequencer.c > +++ b/sequencer.c > @@ > - static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts") > - static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate") > - static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec") > -+static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits") > -+static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits") > -+static GIT_PATH_FUNC(rebase_path_ask_on_initially_empty, "rebase-merge/ask_on_initially_empty") > - > - static int git_sequencer_config(const char *k, const char *v, void *cb) > + struct replay_opts *opts, > + struct commit *commit) > { > +- int index_unchanged, empty_commit; > ++ int index_unchanged, originally_empty; > + > + /* > + * Three cases: > @@ > - empty_commit = is_original_commit_empty(commit); > - if (empty_commit < 0) > - return empty_commit; > + if (opts->keep_redundant_commits) > + return 1; > + > +- empty_commit = is_original_commit_empty(commit); > +- if (empty_commit < 0) > +- return empty_commit; > - if (!empty_commit) > -+ if (!empty_commit || opts->ask_on_initially_empty) > ++ originally_empty = is_original_commit_empty(commit); > ++ if (originally_empty < 0) > ++ return originally_empty; > ++ if (!originally_empty) > return 0; > else > return 1; > -@@ > - char *author = NULL; > - struct commit_message msg = { NULL, NULL, NULL, NULL }; > - struct strbuf msgbuf = STRBUF_INIT; > -- int res, unborn = 0, reword = 0, allow; > -+ int res, unborn = 0, reword = 0, allow, drop_commit; > - > - if (opts->no_commit) { > - /* > -@@ > - goto leave; > - } > - > -- allow = allow_empty(r, opts, commit); > -- if (allow < 0) { > -- res = allow; > -- goto leave; > -- } else if (allow) > -- flags |= ALLOW_EMPTY; > -- if (!opts->no_commit) { > -+ drop_commit = 0; > -+ if (opts->drop_redundant_commits && is_index_unchanged(r)) { > -+ drop_commit = 1; > -+ fprintf(stderr, _("No changes -- Patch already applied.")); > -+ } else { > -+ allow = allow_empty(r, opts, commit); > -+ if (allow < 0) { > -+ res = allow; > -+ goto leave; > -+ } else if (allow) { > -+ flags |= ALLOW_EMPTY; > -+ } > -+ } > -+ if (!opts->no_commit && !drop_commit) { > - if (author || command == TODO_REVERT || (flags & AMEND_MSG)) > - res = do_commit(r, msg_file, author, opts, flags); > - else > -@@ > - else if (!strcmp(key, "options.allow-empty-message")) > - opts->allow_empty_message = > - git_config_bool_or_int(key, value, &error_flag); > -+ else if (!strcmp(key, "options.drop-redundant-commits")) > -+ opts->drop_redundant_commits = > -+ git_config_bool_or_int(key, value, &error_flag); > - else if (!strcmp(key, "options.keep-redundant-commits")) > - opts->keep_redundant_commits = > - git_config_bool_or_int(key, value, &error_flag); > -+ else if (!strcmp(key, "options.ask_on_initially_empty")) > -+ opts->ask_on_initially_empty = > -+ git_config_bool_or_int(key, value, &error_flag); > - else if (!strcmp(key, "options.signoff")) > - opts->signoff = git_config_bool_or_int(key, value, &error_flag); > - else if (!strcmp(key, "options.record-origin")) > -@@ > - if (file_exists(rebase_path_reschedule_failed_exec())) > - opts->reschedule_failed_exec = 1; > - > -+ if (file_exists(rebase_path_drop_redundant_commits())) > -+ opts->drop_redundant_commits = 1; > -+ > -+ if (file_exists(rebase_path_keep_redundant_commits())) > -+ opts->keep_redundant_commits = 1; > -+ > -+ if (file_exists(rebase_path_ask_on_initially_empty())) > -+ opts->ask_on_initially_empty = 1; > -+ > - read_strategy_opts(opts, &buf); > - strbuf_release(&buf); > - > -@@ > - write_file(rebase_path_cdate_is_adate(), "%s", ""); > - if (opts->ignore_date) > - write_file(rebase_path_ignore_date(), "%s", ""); > -+ if (opts->drop_redundant_commits) > -+ write_file(rebase_path_drop_redundant_commits(), "%s", ""); > -+ if (opts->keep_redundant_commits) > -+ write_file(rebase_path_keep_redundant_commits(), "%s", ""); > -+ if (opts->ask_on_initially_empty) > -+ write_file(rebase_path_ask_on_initially_empty(), "%s", ""); > - if (opts->reschedule_failed_exec) > - write_file(rebase_path_reschedule_failed_exec(), "%s", ""); > - > -@@ > - if (opts->allow_empty_message) > - res |= git_config_set_in_file_gently(opts_file, > - "options.allow-empty-message", "true"); > -+ if (opts->drop_redundant_commits) > -+ res |= git_config_set_in_file_gently(opts_file, > -+ "options.drop-redundant-commits", "true"); > - if (opts->keep_redundant_commits) > - res |= git_config_set_in_file_gently(opts_file, > - "options.keep-redundant-commits", "true"); > -+ if (opts->ask_on_initially_empty) > -+ res |= git_config_set_in_file_gently(opts_file, > -+ "options.ask_on_initially_empty", "true"); > - if (opts->signoff) > - res |= git_config_set_in_file_gently(opts_file, > - "options.signoff", "true"); > @@ > struct rev_info *revs, struct strbuf *out, > unsigned flags) > { > - int keep_empty = flags & TODO_LIST_KEEP_EMPTY; > -+ int drop_empty = flags & TODO_LIST_DROP_EMPTY; > -+ int ask_empty = flags & TODO_LIST_ASK_EMPTY; > int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS; > int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO; > struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT; > -@@ > - is_empty = is_original_commit_empty(commit); > - if (!is_empty && (commit->object.flags & PATCHSAME)) > - continue; > -+ if (is_empty && drop_empty) > -+ continue; > - > - strbuf_reset(&oneline); > - pretty_print_commit(pp, commit, &oneline); > @@ > if (!to_merge) { > /* non-merge commit: easy case */ > strbuf_reset(&buf); > - if (!keep_empty && is_empty) > -+ if (is_empty && ask_empty) > - strbuf_addf(&buf, "%c ", comment_line_char); > +- strbuf_addf(&buf, "%c ", comment_line_char); > strbuf_addf(&buf, "%s %s %s", cmd_pick, > oid_to_hex(&commit->object.oid), > + oneline.buf); > @@ > struct pretty_print_context pp = {0}; > struct rev_info revs; > struct commit *commit; > - int keep_empty = flags & TODO_LIST_KEEP_EMPTY; > -+ int drop_empty = flags & TODO_LIST_DROP_EMPTY; > -+ int ask_empty = flags & TODO_LIST_ASK_EMPTY; > const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick"; > int rebase_merges = flags & TODO_LIST_REBASE_MERGES; > > @@ -491,19 +269,16 @@ > if (!is_empty && (commit->object.flags & PATCHSAME)) > continue; > - if (!keep_empty && is_empty) > -+ if (is_empty && drop_empty) > -+ continue; > -+ if (is_empty && ask_empty) > - strbuf_addf(out, "%c ", comment_line_char); > +- strbuf_addf(out, "%c ", comment_line_char); > strbuf_addf(out, "%s %s ", insn, > oid_to_hex(&commit->object.oid)); > + pretty_print_commit(&pp, commit, out); > @@ > > todo_list_to_strbuf(r, todo_list, &buf, num, flags); > if (flags & TODO_LIST_APPEND_TODO_HELP) > - append_todo_help(flags & TODO_LIST_KEEP_EMPTY, count_commands(todo_list), > -+ append_todo_help(!(flags & TODO_LIST_ASK_EMPTY), > -+ count_commands(todo_list), > ++ append_todo_help(count_commands(todo_list), > shortrevisions, shortonto, &buf); > > res = write_message(buf.buf, buf.len, file, 0); > @@ -511,16 +286,6 @@ > diff --git a/sequencer.h b/sequencer.h > --- a/sequencer.h > +++ b/sequencer.h > -@@ > - int allow_rerere_auto; > - int allow_empty; > - int allow_empty_message; > -+ int drop_redundant_commits; > - int keep_redundant_commits; > -+ int ask_on_initially_empty; > - int verbose; > - int quiet; > - int reschedule_failed_exec; > @@ > int sequencer_skip(struct repository *repo, struct replay_opts *opts); > int sequencer_remove_state(struct replay_opts *opts); > @@ -530,19 +295,34 @@ > #define TODO_LIST_SHORTEN_IDS (1U << 1) > #define TODO_LIST_ABBREVIATE_CMDS (1U << 2) > #define TODO_LIST_REBASE_MERGES (1U << 3) > -@@ > - * `--onto`, we do not want to re-generate the root commits. > - */ > - #define TODO_LIST_ROOT_WITH_ONTO (1U << 6) > -+#define TODO_LIST_DROP_EMPTY (1U << 7) > -+#define TODO_LIST_ASK_EMPTY (1U << 8) > - > - > - int sequencer_make_script(struct repository *r, struct strbuf *out, int argc, > > diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh > --- a/t/t3421-rebase-topology-linear.sh > +++ b/t/t3421-rebase-topology-linear.sh > +@@ > + test_run_rebase () { > + result=$1 > + shift > +- test_expect_$result "rebase $* drops empty commit" " > ++ test_expect_$result "rebase $* keeps begin-empty commits" " > + reset_rebase && > +- git rebase $* c l && > +- test_cmp_rev c HEAD~2 && > +- test_linear_range 'd l' c.. > ++ git rebase $* j l && > ++ test_cmp_rev c HEAD~4 && > ++ test_linear_range 'j d k l' c.. > + " > + } > +-test_run_rebase success '' > ++test_run_rebase failure '' > + test_run_rebase success -m > + test_run_rebase success -i > +-test_have_prereq !REBASE_P || test_run_rebase success -p > ++test_have_prereq !REBASE_P || test_run_rebase failure -p > + > + test_run_rebase () { > + result=$1 > @@ > test_run_rebase success '' > test_run_rebase success -m > @@ -603,31 +383,22 @@ > + git commit -m "Five letters ought to be enough for anybody" > +' > + > -+test_expect_success 'rebase --merge --empty=drop' ' > ++test_expect_failure 'rebase (am-backend) with a variety of empty commits' ' > ++ test_when_finished "git rebase --abort" && > + git checkout -B testing localmods && > -+ git rebase --merge --empty=drop upstream && > -+ > -+ test_write_lines C B A >expect && > -+ git log --format=%s >actual && > -+ test_cmp expect actual > -+' > ++ # rebase (--am) should not drop commits that start empty > ++ git rebase upstream && > + > -+test_expect_success 'rebase --merge --empty=keep' ' > -+ git checkout -B testing localmods && > -+ git rebase --merge --empty=keep upstream && > -+ > -+ test_write_lines D C2 C B A >expect && > ++ test_write_lines D C B A >expect && > + git log --format=%s >actual && > + test_cmp expect actual > +' > + > -+test_expect_success 'rebase --merge --empty=ask' ' > ++test_expect_failure 'rebase --merge with a variety of empty commits' ' > ++ test_when_finished "git rebase --abort" && > + git checkout -B testing localmods && > -+ test_must_fail git rebase --merge --empty=ask upstream && > -+ > -+ test_must_fail git rebase --skip && > -+ git commit --allow-empty && > -+ git rebase --continue && > ++ # rebase --merge should not halt on the commit that becomes empty > ++ git rebase --merge upstream && > + > + test_write_lines D C B A >expect && > + git log --format=%s >actual && > @@ -636,25 +407,17 @@ > + > +GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR > + > -+test_expect_success 'rebase --interactive --empty=drop' ' > ++test_expect_success 'rebase --interactive with a variety of empty commits' ' > + git checkout -B testing localmods && > -+ git rebase --interactive --empty=drop upstream && > ++ test_must_fail git rebase --interactive upstream && > + > -+ test_write_lines C B A >expect && > -+ git log --format=%s >actual && > -+ test_cmp expect actual > -+' > -+ > -+test_expect_success 'rebase --interactive --empty=keep' ' > -+ git checkout -B testing localmods && > -+ git rebase --interactive --empty=keep upstream && > ++ git rebase --skip && > + > -+ test_write_lines D C2 C B A >expect && > ++ test_write_lines D C B A >expect && > + git log --format=%s >actual && > + test_cmp expect actual > +' > + > -+ > +test_done > > diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh > @@ -665,32 +428,28 @@ > ' > > -test_expect_success 'Rebase -Xsubtree --keep-empty --onto commit' ' > -+test_expect_success 'Rebase -Xsubtree --empty=ask --onto commit' ' > ++test_expect_success 'Rebase -Xsubtree --onto commit' ' > reset_rebase && > git checkout -b rebase-onto to-rebase && > - test_must_fail git rebase -Xsubtree=files_subtree --keep-empty --onto files-master master && > -+ test_must_fail git rebase -Xsubtree=files_subtree --empty=ask --onto files-master master && > ++ test_must_fail git rebase -Xsubtree=files_subtree --onto files-master master && > : first pick results in no changes && > - git rebase --continue && > -+ test_must_fail git rebase --skip && > -+ : last pick was an empty commit that has no changes, but we want to keep it && > -+ git commit --allow-empty && > ++ git rebase --skip && > verbose test "$(commit_message HEAD~2)" = "master4" && > verbose test "$(commit_message HEAD~)" = "files_subtree/master5" && > verbose test "$(commit_message HEAD)" = "Empty commit" > ' > > -test_expect_success 'Rebase -Xsubtree --keep-empty --rebase-merges --onto commit' ' > -+test_expect_success 'Rebase -Xsubtree --empty=ask --rebase-merges --onto commit' ' > ++test_expect_success 'Rebase -Xsubtree --rebase-merges --onto commit' ' > reset_rebase && > git checkout -b rebase-merges-onto to-rebase && > - test_must_fail git rebase -Xsubtree=files_subtree --keep-empty --rebase-merges --onto files-master --root && > -+ test_must_fail git rebase -Xsubtree=files_subtree --empty=ask --rebase-merges --onto files-master --root && > ++ test_must_fail git rebase -Xsubtree=files_subtree --rebase-merges --onto files-master --root && > : first pick results in no changes && > - git rebase --continue && > -+ test_must_fail git rebase --skip && > -+ : last pick was an empty commit that has no changes, but we want to keep it && > -+ git commit --allow-empty && > ++ git rebase --skip && > verbose test "$(commit_message HEAD~2)" = "master4" && > verbose test "$(commit_message HEAD~)" = "files_subtree/master5" && > verbose test "$(commit_message HEAD)" = "Empty commit" > -: ---------- > 4: c9542a2abe rebase (interactive-backend): fix handling of commits that become empty > 2: bd3c5ec155 = 5: 9f66229d5c t3406: simplify an already simple test > 3: 49388b79fd = 6: 8d731fa39c rebase, sequencer: remove the broken GIT_QUIET handling > 4: 478479358f ! 7: b6b6597eef rebase: make sure to pass along the quiet flag to the sequencer > @@ -8,13 +8,13 @@ > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ > + replay.allow_empty_message = opts->allow_empty_message; > + replay.drop_redundant_commits = (opts->empty == EMPTY_DROP); > replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP); > - replay.ask_on_initially_empty = (opts->empty == EMPTY_ASK && > - !(opts->flags & REBASE_INTERACTIVE_EXPLICIT)); > + replay.quiet = !(opts->flags & REBASE_NO_QUIET); > replay.verbose = opts->flags & REBASE_VERBOSE; > replay.reschedule_failed_exec = opts->reschedule_failed_exec; > - replay.committer_date_is_author_date = > + replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt); > @@ > N_("allow pre-rebase hook to run")), > OPT_NEGBIT('q', "quiet", &options.flags, > 5: ee26f5a161 = 8: 0acefa988b rebase: fix handling of restrict_revision > 6: 34a69def33 = 9: 8c5b5b5133 t3432: make these tests work with either am or merge backends > 7: f2c92853b4 ! 10: b8c087d6fb rebase: allow more types of rebases to fast-forward > @@ -35,8 +35,8 @@ > OPT_STRING(0, "onto", &options.onto_name, > N_("revision"), > @@ > - options.ignore_date) > - options.flags |= REBASE_FORCE; > + state_dir_base, cmd_live_rebase, buf.buf); > + } > > + if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) || > + (action != ACTION_NONE) || > @@ -47,7 +47,9 @@ > + > for (i = 0; i < options.git_am_opts.argc; i++) { > const char *option = options.git_am_opts.argv[i], *p; > - if (!strcmp(option, "--whitespace=fix") || > + if (!strcmp(option, "--committer-date-is-author-date") || > + !strcmp(option, "--ignore-date") || > + !strcmp(option, "--whitespace=fix") || > !strcmp(option, "--whitespace=strip")) > - options.flags |= REBASE_FORCE; > + allow_preemptive_ff = 0; > 8: b307340f7c ! 11: b50a1741e0 git-rebase.txt: add more details about behavioral differences of backends > @@ -8,23 +8,24 @@ > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ > - with `--keep-base` in order to drop those commits from your branch. > > --ignore-whitespace:: > -- Behaves differently depending on which backend is selected. > --+ > --'am' backend: When applying a patch, ignore changes in whitespace in > --context lines if necessary. > --+ > --'interactive' backend: Treat lines with only whitespace changes as > --unchanged for the sake of a three-way merge. > -+ Ignore whitespace-only changes in the commits being rebased, > -+ which may avoid "unnecessary" conflicts. (Both backends > -+ currently have differing edgecase bugs with this option; see > -+ BEHAVIORAL DIFFERENCES.) > - > --whitespace=