Once upon a time, I dreamt of an interactive rebase that would not flatten branch structure, but instead recreate the commit topology faithfully. My original attempt was --preserve-merges, but that design was so limited that I did not even enable it in interactive mode. Subsequently, it *was* enabled in interactive mode, with the predictable consequences: as the --preserve-merges design does not allow for specifying the parents of merge commits explicitly, all the new commits' parents are defined *implicitly* by the previous commit history, and hence it is *not possible to even reorder commits*. This design flaw cannot be fixed. Not without a complete re-design, at least. This patch series offers such a re-design. Think of --rebase-merges as "--preserve-merges done right". It introduces new verbs for the todo list, `label`, `reset` and `merge`. For a commit topology like this: A - B - C \ / D the generated todo list would look like this: # branch D pick 0123 A label branch-point pick 1234 D label D reset branch-point pick 2345 B merge -C 3456 D # C There are more patches in the pipeline, based on this patch series, but left for later in the interest of reviewable patch series: one mini series to use the sequencer even for `git rebase -i --root`, and another one to add support for octopus merges to --rebase-merges. And then one to allow for rebasing merge commits in a smarter way (this one will need a bit more work, though, as it can result in very complicated, nested merge conflicts *very* easily). Changes since v5 (sorry, this one is big, and so is the interdiff): - rebased to `master`, resolving conflicts with `ws/rebase-p` and `pw/rebase-keep-empty-fixes` (these changes are not reflected in the interdiff because I still did not find a good way to represent such fixups). - just like `git merge` refuses to merge ancestors of HEAD, so does now the todo command `merge`. - `git remote -v`'s output now differs when pulling with --rebase-merges vs pulling with --interactive. - the `merge` command now also gives rerere a chance (just like `pick` already does). - simplified test for rebase-cousins (no need to run --rebase-merges interactively, so there is no need to override the editor either). - fixed `safe_append()` to roll back the lock file even when *reading* failed. - used `reflog_message()` in `do_reset()` rather than duplicating the logic. - reworded misleading commit message talking about fast-forwarding merge commits, when we just fast-forward `merge` commands *to* those merge commits whenever possible. - removed duplicate `if (can_fast_forward)` clause. - stopped promising support for octopus merges in --make-script in this patch series (it will be added in a later patch series). - fixed grammar error in the message of the commit adding support for post-rewrite hooks to handle commits processed via the `merge` command. - folded TODO_MERGE_AND_EDIT into TODO_MERGE by using a new `flags` field. - the code of `do_merge()` has been made more obvious by using a variable `oneline_offset` instead of the non-descriptive `p`. - renamed the option to `rebase-merges`, in preparation for doing it smarter using Phillip Wood's strategy (this will be contributed in a follow-up patch series after two others that add support for octopus merges and for handling --root via the sequencer). - included Phillip Wood's test for --keep-empty with the new mode (and folded in a fix into the code of the `merge` command). - added -r as shortcut for --rebase-merges - added an entire section about "REBASING MERGES" to git-rebase's man page. Johannes Schindelin (13): sequencer: avoid using errno clobbered by rollback_lock_file() sequencer: make rearrange_squash() a bit more obvious sequencer: introduce new commands to reset the revision sequencer: introduce the `merge` command sequencer: fast-forward `merge` commands, if possible rebase-helper --make-script: introduce a flag to rebase merges rebase: introduce the --rebase-merges option sequencer: make refs generated by the `label` command worktree-local sequencer: handle post-rewrite for merge commands rebase --rebase-merges: avoid "empty merges" pull: accept --rebase=merges to recreate the branch topology rebase -i: introduce --rebase-merges=[no-]rebase-cousins rebase -i --rebase-merges: add a section to the man page Phillip Wood (1): rebase --rebase-merges: add test for --keep-empty Stefan Beller (1): git-rebase--interactive: clarify arguments Documentation/config.txt | 8 + Documentation/git-pull.txt | 5 +- Documentation/git-rebase.txt | 140 ++++- builtin/pull.c | 14 +- builtin/rebase--helper.c | 13 +- builtin/remote.c | 18 +- contrib/completion/git-completion.bash | 4 +- git-rebase--interactive.sh | 22 +- git-rebase.sh | 16 + refs.c | 3 +- sequencer.c | 775 ++++++++++++++++++++++++- sequencer.h | 7 + t/t3421-rebase-topology-linear.sh | 1 + t/t3430-rebase-merges.sh | 211 +++++++ 14 files changed, 1203 insertions(+), 34 deletions(-) create mode 100755 t/t3430-rebase-merges.sh base-commit: 0b0cc9f86731f894cff8dd25299a9b38c254569e Published-As: https://github.com/dscho/git/releases/tag/recreate-merges-v6 Fetch-It-Via: git fetch https://github.com/dscho/git recreate-merges-v6 Interdiff vs v5: diff --git a/Documentation/config.txt b/Documentation/config.txt index 85dc3a0c429..45916ea8104 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1058,7 +1058,7 @@ branch.<name>.rebase:: "git pull" is run. See "pull.rebase" for doing this in a non branch-specific manner. + -When recreate, also pass `--recreate-merges` along to 'git rebase' +When `merges`, pass the `--rebase-merges` option to 'git rebase' so that locally committed merge commits will not be flattened by running 'git pull'. + @@ -2620,7 +2620,7 @@ pull.rebase:: pull" is run. See "branch.<name>.rebase" for setting this on a per-branch basis. + -When recreate, also pass `--recreate-merges` along to 'git rebase' +When `merges`, pass the `--rebase-merges` option to 'git rebase' so that locally committed merge commits will not be flattened by running 'git pull'. + diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt index b4f9f057ea9..6f76d815dd3 100644 --- a/Documentation/git-pull.txt +++ b/Documentation/git-pull.txt @@ -101,15 +101,15 @@ Options related to merging include::merge-options.txt[] -r:: ---rebase[=false|true|recreate|preserve|interactive]:: +--rebase[=false|true|merges|preserve|interactive]:: When true, rebase the current branch on top of the upstream branch after fetching. If there is a remote-tracking branch corresponding to the upstream branch and the upstream branch was rebased since last fetched, the rebase uses that information to avoid rebasing non-local changes. + -When set to recreate, rebase with the `--recreate-merges` option passed -to `git rebase` so that locally created merge commits will not be flattened. +When set to `merges`, rebase using `git rebase --rebase-merges` so that +locally created merge commits will not be flattened. + When set to preserve, rebase with the `--preserve-merges` option passed to `git rebase` so that locally created merge commits will not be flattened. diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 2b85416f969..be946de2efb 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -378,16 +378,19 @@ The commit list format can be changed by setting the configuration option rebase.instructionFormat. A customized instruction format will automatically have the long commit hash prepended to the format. ---recreate-merges[=(rebase-cousins|no-rebase-cousins)]:: - Recreate merge commits instead of flattening the history by replaying +-r:: +--rebase-merges[=(rebase-cousins|no-rebase-cousins)]:: + Rebase merge commits instead of flattening the history by replaying merges. Merge conflict resolutions or manual amendments to merge - commits are not recreated automatically, but have to be recreated + commits are not rebased automatically, but have to be applied manually. + By default, or when `no-rebase-cousins` was specified, commits which do not -have `<upstream>` as direct ancestor keep their original branch point. +have `<upstream>` as direct ancestor will keep their original branch point. If the `rebase-cousins` mode is turned on, such commits are rebased onto `<upstream>` (or `<onto>`, if specified). ++ +See also REBASING MERGES below. -p:: --preserve-merges:: @@ -786,13 +789,136 @@ The ripple effect of a "hard case" recovery is especially bad: 'everyone' downstream from 'topic' will now have to perform a "hard case" recovery too! +REBASING MERGES +----------------- + +The interactive rebase command was originally designed to handle +individual patch series. As such, it makes sense to exclude merge +commits from the todo list, as the developer may have merged the +current `master` while working on the branch, only to eventually +rebase all the commits onto `master` (skipping the merge commits). + +However, there are legitimate reasons why a developer may want to +recreate merge commits: to keep the branch structure (or "commit +topology") when working on multiple, inter-related branches. + +In the following example, the developer works on a topic branch that +refactors the way buttons are defined, and on another topic branch +that uses that refactoring to implement a "Report a bug" button. The +output of `git log --graph --format=%s -5` may look like this: + +------------ +* Merge branch 'report-a-bug' +|\ +| * Add the feedback button +* | Merge branch 'refactor-button' +|\ \ +| |/ +| * Use the Button class for all buttons +| * Extract a generic Button class from the DownloadButton one +------------ + +The developer might want to rebase those commits to a newer `master` +while keeping the branch topology, for example when the first topic +branch is expected to be integrated into `master` much earlier than the +second one, say, to resolve merge conflicts with changes to the +DownloadButton class that made it into `master`. + +This rebase can be performed using the `--rebase-merges` option. +It will generate a todo list looking like this: + +------------ +label onto + +# Branch: refactor-button +reset onto +pick 123456 Extract a generic Button class from the DownloadButton one +pick 654321 Use the Button class for all buttons +label refactor-button + +# Branch: report-a-bug +reset refactor-button # Use the Button class for all buttons +pick abcdef Add the feedback button +label report-a-bug + +reset onto +merge -C a1b2c3 refactor-button # Merge 'refactor-button' +merge -C 6f5e4d report-a-bug # Merge 'report-a-bug' +------------ + +In contrast to a regular interactive rebase, there are `label`, `reset` and +`merge` commands in addition to `pick` ones. + +The `label` command puts a label to whatever will be the current +revision when that command is executed. Internally, these labels are +worktree-local refs that will be deleted when the rebase finishes or +when it is aborted. That way, rebase operations in multiple worktrees +linked to the same repository do not interfere with one another. + +The `reset` command is essentially a `git reset --hard` to the specified +revision (typically a previously-labeled one). + +The `merge` command will merge the specified revision into whatever is +HEAD at that time. With `-C <original-commit>`, the commit message of +the specified merge commit will be used. When the `-C` is changed to +a lower-case `-c`, the message will be opened in an editor after a +successful merge so that the user can edit the message. + +At this time, the `merge` command will *always* use the `recursive` +merge strategy, with no way to choose a different one. To work around +this, an `exec` command can be used to call `git merge` explicitly, +using the fact that the labels are worktree-local refs (the ref +`refs/rewritten/onto` would correspond to the label `onto`). + +Note: the first command (`reset onto`) labels the revision onto which +the commits are rebased; The name `onto` is just a convention, as a nod +to the `--onto` option. + +It is also possible to introduce completely new merge commits from scratch +by adding a command of the form `merge <merge-head>`. This form will +generate a tentative commit message and always open an editor to let the +user edit it. This can be useful e.g. when a topic branch turns out to +address more than a single concern and wants to be split into two or +even more topic branches. Consider this todo list: + +------------ +pick 192837 Switch from GNU Makefiles to CMake +pick 5a6c7e Document the switch to CMake +pick 918273 Fix detection of OpenSSL in CMake +pick afbecd http: add support for TLS v1.3 +pick fdbaec Fix detection of cURL in CMake on Windows +------------ + +The one commit in this list that is not related to CMake may very well +have been motivated by working on fixing all those bugs introduced by +switching to CMake, but it addresses a different concern. To split this +branch into two topic branches, the todo list could be edited like this: + +------------ +label onto + +pick afbecd http: add support for TLS v1.3 +label tlsv1.3 + +reset onto +pick 192837 Switch from GNU Makefiles to CMake +pick 918273 Fix detection of OpenSSL in CMake +pick fdbaec Fix detection of cURL in CMake on Windows +pick 5a6c7e Document the switch to CMake +label cmake + +reset onto +merge tlsv1.3 +merge cmake +------------ + BUGS ---- The todo list presented by `--preserve-merges --interactive` does not represent the topology of the revision graph. Editing commits and rewording their commit messages should work fine, but attempts to reorder commits tend to produce counterintuitive results. Use ---recreate-merges for a more faithful representation. +--rebase-merges for a more faithful representation. For example, an attempt to rearrange ------------ diff --git a/builtin/pull.c b/builtin/pull.c index 3d1cc60eed6..70b44146ce4 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -27,14 +27,14 @@ enum rebase_type { REBASE_FALSE = 0, REBASE_TRUE, REBASE_PRESERVE, - REBASE_RECREATE, + REBASE_MERGES, REBASE_INTERACTIVE }; /** * Parses the value of --rebase. If value is a false value, returns * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is - * "recreate", returns REBASE_RECREATE. If value is "preserve", returns + * "merges", returns REBASE_MERGES. If value is "preserve", returns * REBASE_PRESERVE. If value is a invalid value, dies with a fatal error if * fatal is true, otherwise returns REBASE_INVALID. */ @@ -49,8 +49,8 @@ static enum rebase_type parse_config_rebase(const char *key, const char *value, return REBASE_TRUE; else if (!strcmp(value, "preserve")) return REBASE_PRESERVE; - else if (!strcmp(value, "recreate")) - return REBASE_RECREATE; + else if (!strcmp(value, "merges")) + return REBASE_MERGES; else if (!strcmp(value, "interactive")) return REBASE_INTERACTIVE; @@ -134,7 +134,7 @@ static struct option pull_options[] = { /* Options passed to git-merge or git-rebase */ OPT_GROUP(N_("Options related to merging")), { OPTION_CALLBACK, 'r', "rebase", &opt_rebase, - "false|true|recreate|preserve|interactive", + "false|true|merges|preserve|interactive", N_("incorporate changes by rebasing rather than merging"), PARSE_OPT_OPTARG, parse_opt_rebase }, OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL, @@ -804,8 +804,8 @@ static int run_rebase(const struct object_id *curr_head, argv_push_verbosity(&args); /* Options passed to git-rebase */ - if (opt_rebase == REBASE_RECREATE) - argv_array_push(&args, "--recreate-merges"); + if (opt_rebase == REBASE_MERGES) + argv_array_push(&args, "--rebase-merges"); else if (opt_rebase == REBASE_PRESERVE) argv_array_push(&args, "--preserve-merges"); else if (opt_rebase == REBASE_INTERACTIVE) diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c index 5d1f12de57b..f7c2a5fdc81 100644 --- a/builtin/rebase--helper.c +++ b/builtin/rebase--helper.c @@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] = { int cmd_rebase__helper(int argc, const char **argv, const char *prefix) { struct replay_opts opts = REPLAY_OPTS_INIT; - unsigned flags = 0, keep_empty = 0, recreate_merges = 0; + unsigned flags = 0, keep_empty = 0, rebase_merges = 0; int abbreviate_commands = 0, rebase_cousins = -1; enum { CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS, @@ -24,7 +24,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")), OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message, N_("allow commits with empty messages")), - OPT_BOOL(0, "recreate-merges", &recreate_merges, N_("recreate merge commits")), + OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")), OPT_BOOL(0, "rebase-cousins", &rebase_cousins, N_("keep original branch points of cousins")), OPT_CMDMODE(0, "continue", &command, N_("continue rebase"), @@ -60,13 +60,13 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix) flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0; flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0; - flags |= recreate_merges ? TODO_LIST_RECREATE_MERGES : 0; + flags |= rebase_merges ? TODO_LIST_REBASE_MERGES : 0; flags |= rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0; flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0; - if (rebase_cousins >= 0 && !recreate_merges) + if (rebase_cousins >= 0 && !rebase_merges) warning(_("--[no-]rebase-cousins has no effect without " - "--recreate-merges")); + "--rebase-merges")); if (command == CONTINUE && argc == 1) return !!sequencer_continue(&opts); diff --git a/builtin/remote.c b/builtin/remote.c index 210890c8a8e..45c9219e07a 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -245,7 +245,9 @@ static int add(int argc, const char **argv) struct branch_info { char *remote_name; struct string_list merge; - enum { NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE } rebase; + enum { + NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES + } rebase; }; static struct string_list branch_list = STRING_LIST_INIT_NODUP; @@ -306,8 +308,8 @@ static int config_read_branches(const char *key, const char *value, void *cb) info->rebase = v; else if (!strcmp(value, "preserve")) info->rebase = NORMAL_REBASE; - else if (!strcmp(value, "recreate")) - info->rebase = NORMAL_REBASE; + else if (!strcmp(value, "merges")) + info->rebase = REBASE_MERGES; else if (!strcmp(value, "interactive")) info->rebase = INTERACTIVE_REBASE; } @@ -965,9 +967,15 @@ static int show_local_info_item(struct string_list_item *item, void *cb_data) printf(" %-*s ", show_info->width, item->string); if (branch_info->rebase) { - printf_ln(branch_info->rebase == INTERACTIVE_REBASE - ? _("rebases interactively onto remote %s") - : _("rebases onto remote %s"), merge->items[0].string); + const char *msg; + if (branch_info->rebase == INTERACTIVE_REBASE) + msg = _("rebases interactively onto remote %s"); + else if (branch_info->rebase == REBASE_MERGES) + msg = _("rebases interactively (with merges) onto " + "remote %s"); + else + msg = _("rebases onto remote %s"); + printf_ln(msg, merge->items[0].string); return 0; } else if (show_info->any_rebase) { printf_ln(_(" merges with remote %s"), merge->items[0].string); diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 7d2e7062919..6af65155c59 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1949,7 +1949,7 @@ _git_rebase () --*) __gitcomp " --onto --merge --strategy --interactive - --recreate-merges --preserve-merges --stat --no-stat + --rebase-merges --preserve-merges --stat --no-stat --committer-date-is-author-date --ignore-date --ignore-whitespace --whitespace= --autosquash --no-autosquash @@ -2120,7 +2120,7 @@ _git_config () return ;; branch.*.rebase) - __gitcomp "false true recreate preserve interactive" + __gitcomp "false true merges preserve interactive" return ;; remote.pushdefault) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 4c21faaccb1..b4ad130e8b1 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -970,7 +970,7 @@ git_rebase__interactive () { init_revisions_and_shortrevisions git rebase--helper --make-script ${keep_empty:+--keep-empty} \ - ${recreate_merges:+--recreate-merges} \ + ${rebase_merges:+--rebase-merges} \ ${rebase_cousins:+--rebase-cousins} \ $revisions ${restrict_revision+^$restrict_revision} >"$todo" || die "$(gettext "Could not generate todo list")" diff --git a/git-rebase.sh b/git-rebase.sh index dd39dfb1112..157705d2a72 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -17,7 +17,7 @@ q,quiet! be quiet. implies --no-stat autostash automatically stash/stash pop before and after fork-point use 'merge-base --fork-point' to refine upstream onto=! rebase onto given branch instead of upstream -recreate-merges? try to recreate merges instead of skipping them +r,rebase-merges? try to rebase merges instead of skipping them p,preserve-merges! try to recreate merges instead of ignoring them s,strategy=! use the given merge strategy no-ff! cherry-pick all commits, even if unchanged @@ -89,7 +89,7 @@ type= state_dir= # One of {'', continue, skip, abort}, as parsed from command line action= -recreate_merges= +rebase_merges= rebase_cousins= preserve_merges= autosquash= @@ -273,12 +273,12 @@ do --allow-empty-message) allow_empty_message=--allow-empty-message ;; - --recreate-merges) - recreate_merges=t + --rebase-merges) + rebase_merges=t test -z "$interactive_rebase" && interactive_rebase=implied ;; - --recreate-merges=*) - recreate_merges=t + --rebase-merges=*) + rebase_merges=t case "${1#*=}" in rebase-cousins) rebase_cousins=t;; no-rebase-cousins) rebase_cousins=;; diff --git a/sequencer.c b/sequencer.c index 0b6aaced9a5..809df1ce484 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1308,7 +1308,6 @@ enum todo_command { TODO_LABEL, TODO_RESET, TODO_MERGE, - TODO_MERGE_AND_EDIT, /* commands that do nothing but are counted for reporting progress */ TODO_NOOP, TODO_DROP, @@ -1330,7 +1329,6 @@ static struct { { 'l', "label" }, { 't', "reset" }, { 'm', "merge" }, - { 0, "merge" }, /* MERGE_AND_EDIT */ { 0, "noop" }, { 'd', "drop" }, { 0, NULL } @@ -1758,9 +1756,14 @@ static int read_and_refresh_cache(struct replay_opts *opts) return 0; } +enum todo_item_flags { + TODO_EDIT_MERGE_MSG = 1 +}; + struct todo_item { enum todo_command command; struct commit *commit; + unsigned int flags; const char *arg; int arg_len; size_t offset_in_buf; @@ -1795,6 +1798,8 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol) char *end_of_object_name; int i, saved, status, padding; + item->flags = 0; + /* left-trim */ bol += strspn(bol, " \t"); @@ -1849,9 +1854,9 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol) bol += strspn(bol, " \t"); else if (skip_prefix(bol, "-c", &bol)) { bol += strspn(bol, " \t"); - item->command = TODO_MERGE_AND_EDIT; + item->flags |= TODO_EDIT_MERGE_MSG; } else { - item->command = TODO_MERGE_AND_EDIT; + item->flags |= TODO_EDIT_MERGE_MSG; item->commit = NULL; item->arg = bol; item->arg_len = (int)(eol - bol); @@ -2511,8 +2516,11 @@ static int safe_append(const char *filename, const char *fmt, ...) if (fd < 0) return -1; - if (strbuf_read_file(&buf, filename, 0) < 0 && errno != ENOENT) - return error_errno(_("could not read '%s'"), filename); + if (strbuf_read_file(&buf, filename, 0) < 0 && errno != ENOENT) { + error_errno(_("could not read '%s'"), filename); + rollback_lock_file(&lock); + return -1; + } strbuf_complete(&buf, '\n'); va_start(ap, fmt); strbuf_vaddf(&buf, fmt, ap); @@ -2574,6 +2582,9 @@ static int do_label(const char *name, int len) return ret; } +static const char *reflog_message(struct replay_opts *opts, + const char *sub_action, const char *fmt, ...); + static int do_reset(const char *name, int len, struct replay_opts *opts) { struct strbuf ref_name = STRBUF_INIT; @@ -2638,33 +2649,50 @@ static int do_reset(const char *name, int len, struct replay_opts *opts) ret = error(_("could not write index")); free((void *)desc.buffer); - if (!ret) { - struct strbuf msg = STRBUF_INIT; - - strbuf_addf(&msg, "(rebase -i) reset '%.*s'", len, name); - ret = update_ref(msg.buf, "HEAD", &oid, NULL, 0, - UPDATE_REFS_MSG_ON_ERR); - strbuf_release(&msg); - } + if (!ret) + ret = update_ref(reflog_message(opts, "reset", "'%.*s'", + len, name), "HEAD", &oid, + NULL, 0, UPDATE_REFS_MSG_ON_ERR); strbuf_release(&ref_name); return ret; } static int do_merge(struct commit *commit, const char *arg, int arg_len, - int run_commit_flags, struct replay_opts *opts) + int flags, struct replay_opts *opts) { - int merge_arg_len; + int run_commit_flags = (flags & TODO_EDIT_MERGE_MSG) ? + EDIT_MSG | VERIFY_MSG : 0; struct strbuf ref_name = STRBUF_INIT; struct commit *head_commit, *merge_commit, *i; - struct commit_list *common, *j, *reversed = NULL; + struct commit_list *bases, *j, *reversed = NULL; struct merge_options o; - int can_fast_forward, ret; + int merge_arg_len, oneline_offset, can_fast_forward, ret; static struct lock_file lock; + const char *p; - for (merge_arg_len = 0; merge_arg_len < arg_len; merge_arg_len++) - if (isspace(arg[merge_arg_len])) - break; + oneline_offset = arg_len; + merge_arg_len = strcspn(arg, " \t\n"); + p = arg + merge_arg_len; + p += strspn(p, " \t\n"); + if (*p == '#' && (!p[1] || isspace(p[1]))) { + p += 1 + strspn(p + 1, " \t\n"); + oneline_offset = p - arg; + } else if (p - arg < arg_len) + BUG("octopus merges are not supported yet: '%s'", p); + + strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg); + merge_commit = lookup_commit_reference_by_name(ref_name.buf); + if (!merge_commit) { + /* fall back to non-rewritten ref or commit */ + strbuf_splice(&ref_name, 0, strlen("refs/rewritten/"), "", 0); + merge_commit = lookup_commit_reference_by_name(ref_name.buf); + } + if (!merge_commit) { + error(_("could not resolve '%s'"), ref_name.buf); + strbuf_release(&ref_name); + return -1; + } if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) return -1; @@ -2697,7 +2725,6 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, } unuse_commit_buffer(commit, message); } else { - const char *p = arg + merge_arg_len; struct strbuf buf = STRBUF_INIT; int len; @@ -2705,12 +2732,10 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, write_author_script(buf.buf); strbuf_reset(&buf); - p += strspn(p, " \t"); - if (*p == '#' && isspace(p[1])) - p += 1 + strspn(p + 1, " \t"); - if (*p) - len = strlen(p); - else { + if (oneline_offset < arg_len) { + p = arg + oneline_offset; + len = arg_len - oneline_offset; + } else { strbuf_addf(&buf, "Merge branch '%.*s'", merge_arg_len, arg); p = buf.buf; @@ -2728,25 +2753,24 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, } /* - * If HEAD is not identical to the parent of the original merge commit, - * we cannot fast-forward. + * If HEAD is not identical to the first parent of the original merge + * commit, we cannot fast-forward. */ can_fast_forward = opts->allow_ff && commit && commit->parents && !oidcmp(&commit->parents->item->object.oid, &head_commit->object.oid); - strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg); - merge_commit = lookup_commit_reference_by_name(ref_name.buf); - if (!merge_commit) { - /* fall back to non-rewritten ref or commit */ - strbuf_splice(&ref_name, 0, strlen("refs/rewritten/"), "", 0); - merge_commit = lookup_commit_reference_by_name(ref_name.buf); - } - if (!merge_commit) { - error(_("could not resolve '%s'"), ref_name.buf); - strbuf_release(&ref_name); - rollback_lock_file(&lock); - return -1; + /* + * If the merge head is different from the original one, we cannot + * fast-forward. + */ + if (can_fast_forward) { + struct commit_list *second_parent = commit->parents->next; + + if (second_parent && !second_parent->next && + oidcmp(&merge_commit->object.oid, + &second_parent->item->object.oid)) + can_fast_forward = 0; } if (can_fast_forward && commit->parents->next && @@ -2763,10 +2787,18 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, git_path_merge_head(), 0); write_message("no-ff", 5, git_path_merge_mode(), 0); - common = get_merge_bases(head_commit, merge_commit); - for (j = common; j; j = j->next) + bases = get_merge_bases(head_commit, merge_commit); + if (bases && !oidcmp(&merge_commit->object.oid, + &bases->item->object.oid)) { + strbuf_release(&ref_name); + rollback_lock_file(&lock); + /* skip merging an ancestor of HEAD */ + return 0; + } + + for (j = bases; j; j = j->next) commit_list_insert(j->item, &reversed); - free_commit_list(common); + free_commit_list(bases); read_cache(); init_merge_options(&o); @@ -2775,6 +2807,8 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, o.buffer_output = 2; ret = merge_recursive(&o, head_commit, merge_commit, reversed, &i); + if (!ret) + rerere(opts->allow_rerere_auto); if (ret <= 0) fputs(o.obuf.buf, stdout); strbuf_release(&o.obuf); @@ -2986,11 +3020,9 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) res = do_label(item->arg, item->arg_len); else if (item->command == TODO_RESET) res = do_reset(item->arg, item->arg_len, opts); - else if (item->command == TODO_MERGE || - item->command == TODO_MERGE_AND_EDIT) { + else if (item->command == TODO_MERGE) { res = do_merge(item->commit, item->arg, item->arg_len, - item->command == TODO_MERGE_AND_EDIT ? - EDIT_MSG | VERIFY_MSG : 0, opts); + item->flags, opts); if (item->commit) record_in_rewritten(&item->commit->object.oid, peek_command(todo_list, 1)); @@ -3404,7 +3436,7 @@ static const char *label_oid(struct object_id *oid, const char *label, strbuf_grow(&state->buf, GIT_SHA1_HEXSZ); label = p = state->buf.buf; - find_unique_abbrev_r(p, oid->hash, default_abbrev); + find_unique_abbrev_r(p, oid, default_abbrev); /* * We may need to extend the abbreviated hash so that there is @@ -3508,11 +3540,13 @@ static int make_script_with_merges(struct pretty_print_context *pp, int is_octopus; const char *p1, *p2; struct object_id *oid; + int is_empty; tail = &commit_list_insert(commit, tail)->next; oidset_insert(&interesting, &commit->object.oid); - if ((commit->object.flags & PATCHSAME)) + is_empty = is_original_commit_empty(commit); + if (!is_empty && (commit->object.flags & PATCHSAME)) continue; strbuf_reset(&oneline); @@ -3522,7 +3556,7 @@ static int make_script_with_merges(struct pretty_print_context *pp, if (!to_merge) { /* non-merge commit: easy case */ strbuf_reset(&buf); - if (!keep_empty && is_original_commit_empty(commit)) + if (!keep_empty && is_empty) strbuf_addf(&buf, "%c ", comment_line_char); strbuf_addf(&buf, "%s %s %s", cmd_pick, oid_to_hex(&commit->object.oid), @@ -3698,11 +3732,11 @@ int sequencer_make_script(FILE *out, int argc, const char **argv, struct commit *commit; int keep_empty = flags & TODO_LIST_KEEP_EMPTY; const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick"; - int recreate_merges = flags & TODO_LIST_RECREATE_MERGES; + int rebase_merges = flags & TODO_LIST_REBASE_MERGES; init_revisions(&revs, NULL); revs.verbose_header = 1; - if (recreate_merges) + if (rebase_merges) revs.cherry_mark = 1; else { revs.max_parents = 1; @@ -3731,7 +3765,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv, if (prepare_revision_walk(&revs) < 0) return error(_("make_script: error preparing revisions")); - if (recreate_merges) + if (rebase_merges) return make_script_with_merges(&pp, &revs, out, flags); while ((commit = get_revision(&revs))) { @@ -3823,10 +3857,12 @@ int transform_todos(unsigned flags) short_commit_name(item->commit) : oid_to_hex(&item->commit->object.oid); - if (item->command == TODO_MERGE) - strbuf_addstr(&buf, " -C"); - else if (item->command == TODO_MERGE_AND_EDIT) - strbuf_addstr(&buf, " -c"); + if (item->command == TODO_MERGE) { + if (item->flags & TODO_EDIT_MERGE_MSG) + strbuf_addstr(&buf, " -c"); + else + strbuf_addstr(&buf, " -C"); + } strbuf_addf(&buf, " %s", oid); } diff --git a/sequencer.h b/sequencer.h index 739dd0fa92b..d9570d92b11 100644 --- a/sequencer.h +++ b/sequencer.h @@ -59,9 +59,9 @@ int sequencer_remove_state(struct replay_opts *opts); #define TODO_LIST_KEEP_EMPTY (1U << 0) #define TODO_LIST_SHORTEN_IDS (1U << 1) #define TODO_LIST_ABBREVIATE_CMDS (1U << 2) -#define TODO_LIST_RECREATE_MERGES (1U << 3) +#define TODO_LIST_REBASE_MERGES (1U << 3) /* - * When recreating merges, commits that do have the base commit as ancestor + * When rebasing merges, commits that do have the base commit as ancestor * ("cousins") are *not* rebased onto the new base by default. If those * commits should be rebased onto the new base, this flag needs to be passed. */ diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh index 68fe2003ef5..fbae5dab7e2 100755 --- a/t/t3421-rebase-topology-linear.sh +++ b/t/t3421-rebase-topology-linear.sh @@ -217,6 +217,7 @@ test_run_rebase success '' test_run_rebase failure -m test_run_rebase failure -i test_run_rebase failure -p +test_run_rebase success --rebase-merges # m # / diff --git a/t/t3430-rebase-recreate-merges.sh b/t/t3430-rebase-merges.sh similarity index 87% rename from t/t3430-rebase-recreate-merges.sh rename to t/t3430-rebase-merges.sh index 9a59f12b670..ee006810573 100755 --- a/t/t3430-rebase-recreate-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -1,9 +1,9 @@ #!/bin/sh # -# Copyright (c) 2017 Johannes E. Schindelin +# Copyright (c) 2018 Johannes E. Schindelin # -test_description='git rebase -i --recreate-merges +test_description='git rebase -i --rebase-merges This test runs git rebase "interactively", retaining the branch structure by recreating merge commits. @@ -19,6 +19,13 @@ Initial setup: . ./test-lib.sh . "$TEST_DIRECTORY"/lib-rebase.sh +test_cmp_graph () { + cat >expect && + git log --graph --boundary --format=%s "$@" >output && + sed "s/ *$//" <output >output.trimmed && + test_cmp expect output.trimmed +} + test_expect_success 'setup' ' write_script replace-editor.sh <<-\EOF && mv "$1" "$(git rev-parse --git-path ORIGINAL-TODO)" @@ -63,17 +70,10 @@ merge -C H second merge onebranch # Merge the topic branch 'onebranch' EOF -test_cmp_graph () { - cat >expect && - git log --graph --boundary --format=%s "$@" >output && - sed "s/ *$//" <output >output.trimmed && - test_cmp expect output.trimmed -} - test_expect_success 'create completely different structure' ' test_config sequence.editor \""$PWD"/replace-editor.sh\" && test_tick && - git rebase -i --recreate-merges A && + git rebase -i -r A && test_cmp_graph <<-\EOF * Merge the topic branch '\''onebranch'\'' |\ @@ -132,7 +132,7 @@ test_expect_success 'with a branch tip that was cherry-picked already' ' git checkout already-upstream && test_tick && - git rebase -i --recreate-merges upstream-with-a2 && + git rebase -i -r upstream-with-a2 && test_cmp_graph upstream-with-a2.. <<-\EOF * Merge branch A |\ @@ -144,18 +144,13 @@ test_expect_success 'with a branch tip that was cherry-picked already' ' ' test_expect_success 'do not rebase cousins unless asked for' ' - write_script copy-editor.sh <<-\EOF && - cp "$1" "$(git rev-parse --git-path ORIGINAL-TODO)" - EOF - - test_config sequence.editor \""$PWD"/copy-editor.sh\" && git checkout -b cousins master && before="$(git rev-parse --verify HEAD)" && test_tick && - git rebase -i --recreate-merges HEAD^ && + git rebase -r HEAD^ && test_cmp_rev HEAD $before && test_tick && - git rebase -i --recreate-merges=rebase-cousins HEAD^ && + git rebase --rebase-merges=rebase-cousins HEAD^ && test_cmp_graph HEAD^.. <<-\EOF * Merge the topic branch '\''onebranch'\'' |\ @@ -196,7 +191,7 @@ test_expect_success 'post-rewrite hook and fixups work for merges' ' echo "cat >actual" | write_script .git/hooks/post-rewrite && test_tick && - git rebase -i --autosquash --recreate-merges HEAD^^^ && + git rebase -i --autosquash -r HEAD^^^ && printf "%s %s\n%s %s\n%s %s\n%s %s\n" >expect $(git rev-parse \ $fixup^^2 HEAD^2 \ $fixup^^ HEAD^ \ @@ -205,4 +200,12 @@ test_expect_success 'post-rewrite hook and fixups work for merges' ' test_cmp expect actual ' +test_expect_success 'refuse to merge ancestors of HEAD' ' + echo "merge HEAD^" >script-from-scratch && + test_config -C wt sequence.editor \""$PWD"/replace-editor.sh\" && + before="$(git rev-parse HEAD)" && + git rebase -i HEAD && + test_cmp_rev HEAD $before +' + test_done -- 2.17.0.windows.1.4.g7e4058d72e3