{"thread":{"id":"52960","subject":"[PATCH] rebase --merge: optionally skip upstreamed commits","startedAt":"2020-03-09T20:55:39Z","lastAt":"2020-04-11T02:46:21Z","messageCount":36,"participants":["Jonathan Tan","Taylor Blau","Johannes Schindelin","Elijah Newren","Junio C Hamano","Danh Doan","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"392973","messageId":"20200309205523.121319-1-jonathantanmy@google.com","threadId":"52960","inReplyTo":null,"subject":"[PATCH] rebase --merge: optionally skip upstreamed commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-09T20:55:23Z","receivedAt":"2020-03-09T20:55:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When rebasing against an upstream that has had many commits since the\noriginal branch was created:\n\n O -- O -- ... -- O -- O (upstream)\n \\\n -- O (my-dev-branch)\n\nbecause \"git rebase\" attempts to exclude commits that are duplicates of\nupstream ones, it must read the contents of every novel upstream commit,\nin addition to the tip of the upstream and the merge base. This can be a\nsignificant performance hit, especially in a partial clone, wherein a\nread of an object may end up being a fetch.\n\nAdd a flag to \"git rebase\" to allow suppression of this feature. This\nflag only works when using the \"merge\" backend.\n\nThis flag changes the behavior of sequencer_make_script(), called from\ndo_interactive_rebase() <- run_rebase_interactive() <-\nrun_specific_rebase() <- cmd_rebase(). With this flag, limit_list()\n(indirectly called from sequencer_make_script() through\nprepare_revision_walk()) will no longer call cherry_pick_list(), and\nthus PATCHSAME is no longer set. Refraining from setting PATCHSAME both\nmeans that the intermediate commits in upstream are no longer read (as\nshown by the test) and means that no PATCHSAME-caused skipping of\ncommits is done by sequencer_make_script(), either directly or through\nmake_script_with_merges().\n\nSigned-off-by: Jonathan Tan \n---\nMore improvements for partial clone, but this is a benefit for\nnon-partial-clone as well, hence the way I wrote the commit message (not\nfocusing too much on partial clone) and the documentation.\n\nI've chosen --skip-already-present and --no-skip-already-present to\nreuse the language already existing in the documentation and to avoid a\ndouble negative (e.g. --avoid-checking-if-already-present and\n--no-avoid-checking-if-already-present) but this causes some clumsiness\nin the documentation and in the code. Any suggestions for the name are\nwelcome.\n\nI've only implemented this for the \"merge\" backend since I think that\nthere is an effort to migrate \"rebase\" to use the \"merge\" backend by\ndefault, and also because \"merge\" uses diff internally which already has\nthe (per-commit) blob batch prefetching.\n---\n Documentation/git-rebase.txt | 12 +++++-\n builtin/rebase.c | 10 ++++-\n sequencer.c | 3 +-\n sequencer.h | 2 +-\n t/t3402-rebase-merge.sh | 77 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 100 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 0c4f038dd6..f73a82b4a9 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -318,6 +318,15 @@ See also INCOMPATIBLE OPTIONS below.\n +\n See also INCOMPATIBLE OPTIONS below.\n \n+--skip-already-present::\n+--no-skip-already-present::\n+\tSkip commits that are already present in the new upstream.\n+\tThis is the default.\n++\n+If the skip-if-already-present feature is unnecessary or undesired,\n+`--no-skip-already-present` may improve performance since it avoids\n+the need to read the contents of every commit in the new upstream.\n+\n --rerere-autoupdate::\n --no-rerere-autoupdate::\n \tAllow the rerere mechanism to update the index with the\n@@ -866,7 +875,8 @@ Only works if the changes (patch IDs based on the diff contents) on\n 'subsystem' did.\n \n In that case, the fix is easy because 'git rebase' knows to skip\n-changes that are already present in the new upstream. So if you say\n+changes that are already present in the new upstream (unless\n+`--no-skip-already-present` is given). So if you say\n (assuming you're on 'topic')\n ------------\n $ git rebase subsystem\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 6154ad8fa5..943211e5bb 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -88,13 +88,15 @@ struct rebase_options {\n \tstruct strbuf git_format_patch_opt;\n \tint reschedule_failed_exec;\n \tint use_legacy_rebase;\n+\tint skip_already_present;\n };\n \n #define REBASE_OPTIONS_INIT {\t\t\t \t\\\n \t\t.type = REBASE_UNSPECIFIED,\t \t\\\n \t\t.flags = REBASE_NO_QUIET, \t\t\\\n \t\t.git_am_opts = ARGV_ARRAY_INIT,\t\t\\\n-\t\t.git_format_patch_opt = STRBUF_INIT\t\\\n+\t\t.git_format_patch_opt = STRBUF_INIT,\t\\\n+\t\t.skip_already_present =\t1\t\t\\\n \t}\n \n static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n@@ -373,6 +375,7 @@ static int run_rebase_interactive(struct rebase_options *opts,\n \tflags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;\n \tflags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;\n \tflags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;\n+\tflags |= opts->skip_already_present ? TODO_LIST_SKIP_ALREADY_PRESENT : 0;\n \n \tswitch (command) {\n \tcase ACTION_NONE: {\n@@ -1507,6 +1510,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"reschedule-failed-exec\",\n \t\t\t &reschedule_failed_exec,\n \t\t\t N_(\"automatically re-schedule any `exec` that fails\")),\n+\t\tOPT_BOOL(0, \"skip-already-present\", &options.skip_already_present,\n+\t\t\t N_(\"skip changes that are already present in the new upstream\")),\n \t\tOPT_END(),\n \t};\n \tint i;\n@@ -1840,6 +1845,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t \"interactive or merge options\"));\n \t}\n \n+\tif (!options.skip_already_present && !is_interactive(&options))\n+\t\tdie(_(\"--no-skip-already-present does not work with the 'am' backend\"));\n+\n \tif (options.signoff) {\n \t\tif (options.type == REBASE_PRESERVE_MERGES)\n \t\t\tdie(\"cannot combine '--signoff' with \"\ndiff --git a/sequencer.c b/sequencer.c\nindex ba90a513b9..752580c017 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4797,12 +4797,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \tint keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n \tconst char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? \"p\" : \"pick\";\n \tint rebase_merges = flags & TODO_LIST_REBASE_MERGES;\n+\tint skip_already_present = !!(flags & TODO_LIST_SKIP_ALREADY_PRESENT);\n \n \trepo_init_revisions(r, &revs, NULL);\n \trevs.verbose_header = 1;\n \tif (!rebase_merges)\n \t\trevs.max_parents = 1;\n-\trevs.cherry_mark = 1;\n+\trevs.cherry_mark = skip_already_present;\n \trevs.limited = 1;\n \trevs.reverse = 1;\n \trevs.right_only = 1;\ndiff --git a/sequencer.h b/sequencer.h\nindex 393571e89a..39bb12f624 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -149,7 +149,7 @@ int sequencer_remove_state(struct replay_opts *opts);\n * `--onto`, we do not want to re-generate the root commits.\n */\n #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)\n-\n+#define TODO_LIST_SKIP_ALREADY_PRESENT (1U << 7)\n \n int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \t\t\t const char **argv, unsigned flags);\ndiff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\nindex a1ec501a87..9b52739a10 100755\n--- a/t/t3402-rebase-merge.sh\n+++ b/t/t3402-rebase-merge.sh\n@@ -162,4 +162,81 @@ test_expect_success 'rebase --skip works with two conflicts in a row' '\n \tgit rebase --skip\n '\n \n+test_expect_success '--no-skip-already-present' '\n+\tgit init repo &&\n+\n+\t# O(1-10) -- O(1-11) -- O(0-10) master\n+\t# \\\n+\t# -- O(1-11) -- O(1-12) otherbranch\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 10) >repo/file.txt &&\n+\tgit -C repo add file.txt &&\n+\tgit -C repo commit -m \"base commit\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 11) >repo/file.txt &&\n+\tgit -C repo commit -a -m \"add 11\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 0 10) >repo/file.txt &&\n+\tgit -C repo commit -a -m \"add 0 delete 11\" &&\n+\n+\tgit -C repo checkout -b otherbranch HEAD^^ &&\n+\tprintf \"Line %d\\n\" $(test_seq 1 11) >repo/file.txt &&\n+\tgit -C repo commit -a -m \"add 11 in another branch\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 12) >repo/file.txt &&\n+\tgit -C repo commit -a -m \"add 12 in another branch\" &&\n+\n+\t# Regular rebase fails, because the 1-11 commit is deduplicated\n+\ttest_must_fail git -C repo rebase --merge master 2> err &&\n+\ttest_i18ngrep \"error: could not apply.*add 12 in another branch\" err &&\n+\tgit -C repo rebase --abort &&\n+\n+\t# With --no-skip-already-present, it works\n+\tgit -C repo rebase --merge --no-skip-already-present master\n+'\n+\n+test_expect_success '--no-skip-already-present refrains from reading unneeded blobs' '\n+\tgit init server &&\n+\n+\t# O(1-10) -- O(1-11) -- O(1-12) master\n+\t# \\\n+\t# -- O(0-10) otherbranch\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 10) >server/file.txt &&\n+\tgit -C server add file.txt &&\n+\tgit -C server commit -m \"merge base\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 11) >server/file.txt &&\n+\tgit -C server commit -a -m \"add 11\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 12) >server/file.txt &&\n+\tgit -C server commit -a -m \"add 12\" &&\n+\n+\tgit -C server checkout -b otherbranch HEAD^^ &&\n+\tprintf \"Line %d\\n\" $(test_seq 0 10) >server/file.txt &&\n+\tgit -C server commit -a -m \"add 0\" &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n+\tgit -C client checkout origin/master &&\n+\tgit -C client checkout origin/otherbranch &&\n+\n+\t# Sanity check to ensure that the blobs from the merge base and \"add\n+\t# 11\" are missing\n+\tgit -C client rev-list --objects --all --missing=print >missing_list &&\n+\tMERGE_BASE_BLOB=$(git -C server rev-parse master^^:file.txt) &&\n+\tADD_11_BLOB=$(git -C server rev-parse master^:file.txt) &&\n+\tgrep \"\\\\?$MERGE_BASE_BLOB\" missing_list &&\n+\tgrep \"\\\\?$ADD_11_BLOB\" missing_list &&\n+\n+\tgit -C client rebase --merge --no-skip-already-present origin/master &&\n+\n+\t# The blob from the merge base had to be fetched, but not \"add 11\"\n+\tgit -C client rev-list --objects --all --missing=print >missing_list &&\n+\t! grep \"\\\\?$MERGE_BASE_BLOB\" missing_list &&\n+\tgrep \"\\\\?$ADD_11_BLOB\" missing_list\n+'\n+\n test_done\n-- \n2.25.1.481.gfbce0eb801-goog\n\n"},{"id":"392975","messageId":"20200310021015.GC42010@syl.local","threadId":"52960","inReplyTo":"20200309205523.121319-1-jonathantanmy@google.com","subject":"Re: [PATCH] rebase --merge: optionally skip upstreamed commits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-03-10T02:10:15Z","receivedAt":"2020-03-10T02:17:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Jonathan,\n\nThis patch makes good sense to me. I left a few notes below, but they\nare relatively minor, and this seems to be all in a good direction.\n\nAs a (somewhat) interesting aside, this feature would be useful to me\noutside of partial clones, since I often have this workflow in my local\ndevelopment wherein 'git rebase' spends quite a bit of time comparing\npatches on my branch to everything new upstream.\n\nOn Mon, Mar 09, 2020 at 01:55:23PM -0700, Jonathan Tan wrote:\n> When rebasing against an upstream that has had many commits since the\n> original branch was created:\n>\n> O -- O -- ... -- O -- O (upstream)\n> \\\n> -- O (my-dev-branch)\n>\n> because \"git rebase\" attempts to exclude commits that are duplicates of\n> upstream ones, it must read the contents of every novel upstream commit,\n> in addition to the tip of the upstream and the merge base.\n\nThis sentence is a little confusing if you skip over the graph, since it\nreads: \"When rebasing against an because ... because ...\". It may be\nclearer if you swap the order of the last two clauses to instead be:\n\n it must read the contents of every novel upstream commit, in addition to\n the tip of the upstream and the merge base, because \"git rebase\"\n attempts to exclude commits that are duplicates of upstream ones.\n\n> This can be a significant performance hit, especially in a partial\n> clone, wherein a read of an object may end up being a fetch.\n>\n> Add a flag to \"git rebase\" to allow suppression of this feature. This\n> flag only works when using the \"merge\" backend.\n>\n> This flag changes the behavior of sequencer_make_script(), called from\n> do_interactive_rebase() <- run_rebase_interactive() <-\n> run_specific_rebase() <- cmd_rebase(). With this flag, limit_list()\n> (indirectly called from sequencer_make_script() through\n> prepare_revision_walk()) will no longer call cherry_pick_list(), and\n> thus PATCHSAME is no longer set. Refraining from setting PATCHSAME both\n> means that the intermediate commits in upstream are no longer read (as\n> shown by the test) and means that no PATCHSAME-caused skipping of\n> commits is done by sequencer_make_script(), either directly or through\n> make_script_with_merges().\n\nThis all sounds good to me.\n\n> Signed-off-by: Jonathan Tan \n> ---\n> More improvements for partial clone, but this is a benefit for\n> non-partial-clone as well, hence the way I wrote the commit message (not\n> focusing too much on partial clone) and the documentation.\n>\n> I've chosen --skip-already-present and --no-skip-already-present to\n> reuse the language already existing in the documentation and to avoid a\n> double negative (e.g. --avoid-checking-if-already-present and\n> --no-avoid-checking-if-already-present) but this causes some clumsiness\n> in the documentation and in the code. Any suggestions for the name are\n> welcome.\n>\n> I've only implemented this for the \"merge\" backend since I think that\n> there is an effort to migrate \"rebase\" to use the \"merge\" backend by\n> default, and also because \"merge\" uses diff internally which already has\n> the (per-commit) blob batch prefetching.\n\nThis also makes sense to me.\n\n> ---\n> Documentation/git-rebase.txt | 12 +++++-\n> builtin/rebase.c | 10 ++++-\n> sequencer.c | 3 +-\n> sequencer.h | 2 +-\n> t/t3402-rebase-merge.sh | 77 ++++++++++++++++++++++++++++++++++++\n> 5 files changed, 100 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 0c4f038dd6..f73a82b4a9 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -318,6 +318,15 @@ See also INCOMPATIBLE OPTIONS below.\n> +\n> See also INCOMPATIBLE OPTIONS below.\n>\n> +--skip-already-present::\n> +--no-skip-already-present::\n> +\tSkip commits that are already present in the new upstream.\n> +\tThis is the default.\n\nI believe that you mean '--skip-already-present' is the default, here,\nbut the placement makes it ambiguous, since it is in a paragraph with a\nheader that contains both the positive and negated version of this flag.\n\nMaybe this could changed to: s/This/--skip-already-present/'.\n\n> ++\n> +If the skip-if-already-present feature is unnecessary or undesired,\n> +`--no-skip-already-present` may improve performance since it avoids\n> +the need to read the contents of every commit in the new upstream.\n> +\n> --rerere-autoupdate::\n> --no-rerere-autoupdate::\n> \tAllow the rerere mechanism to update the index with the\n> @@ -866,7 +875,8 @@ Only works if the changes (patch IDs based on the diff contents) on\n> 'subsystem' did.\n>\n> In that case, the fix is easy because 'git rebase' knows to skip\n> -changes that are already present in the new upstream. So if you say\n> +changes that are already present in the new upstream (unless\n> +`--no-skip-already-present` is given). So if you say\n\nExtremely minor nit: there is a whitespace change on this line where the\noriginal has two spaces between the '.' and 'So', and the new version\nhas only one.\n\n> (assuming you're on 'topic')\n> ------------\n> $ git rebase subsystem\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 6154ad8fa5..943211e5bb 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -88,13 +88,15 @@ struct rebase_options {\n> \tstruct strbuf git_format_patch_opt;\n> \tint reschedule_failed_exec;\n> \tint use_legacy_rebase;\n> +\tint skip_already_present;\n> };\n>\n> #define REBASE_OPTIONS_INIT {\t\t\t \t\\\n> \t\t.type = REBASE_UNSPECIFIED,\t \t\\\n> \t\t.flags = REBASE_NO_QUIET, \t\t\\\n> \t\t.git_am_opts = ARGV_ARRAY_INIT,\t\t\\\n> -\t\t.git_format_patch_opt = STRBUF_INIT\t\\\n> +\t\t.git_format_patch_opt = STRBUF_INIT,\t\\\n> +\t\t.skip_already_present =\t1\t\t\\\n> \t}\n>\n> static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n> @@ -373,6 +375,7 @@ static int run_rebase_interactive(struct rebase_options *opts,\n> \tflags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;\n> \tflags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;\n> \tflags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;\n> +\tflags |= opts->skip_already_present ? TODO_LIST_SKIP_ALREADY_PRESENT : 0;\n>\n> \tswitch (command) {\n> \tcase ACTION_NONE: {\n> @@ -1507,6 +1510,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BOOL(0, \"reschedule-failed-exec\",\n> \t\t\t &reschedule_failed_exec,\n> \t\t\t N_(\"automatically re-schedule any `exec` that fails\")),\n> +\t\tOPT_BOOL(0, \"skip-already-present\", &options.skip_already_present,\n> +\t\t\t N_(\"skip changes that are already present in the new upstream\")),\n\nI scratched my head a little bit about why we weren't using OPT_BIT and\n&flags directly here, but it matches the pattern in the surrounding, so\nI think that 'OPT_BOOL' and target '&options.skip_already_present' here.\n\n> \t\tOPT_END(),\n> \t};\n> \tint i;\n> @@ -1840,6 +1845,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t \"interactive or merge options\"));\n> \t}\n>\n> +\tif (!options.skip_already_present && !is_interactive(&options))\n> +\t\tdie(_(\"--no-skip-already-present does not work with the 'am' backend\"));\n> +\n> \tif (options.signoff) {\n> \t\tif (options.type == REBASE_PRESERVE_MERGES)\n> \t\t\tdie(\"cannot combine '--signoff' with \"\n> diff --git a/sequencer.c b/sequencer.c\n> index ba90a513b9..752580c017 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4797,12 +4797,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> \tint keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n> \tconst char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? \"p\" : \"pick\";\n> \tint rebase_merges = flags & TODO_LIST_REBASE_MERGES;\n> +\tint skip_already_present = !!(flags & TODO_LIST_SKIP_ALREADY_PRESENT);\n>\n> \trepo_init_revisions(r, &revs, NULL);\n> \trevs.verbose_header = 1;\n> \tif (!rebase_merges)\n> \t\trevs.max_parents = 1;\n> -\trevs.cherry_mark = 1;\n> +\trevs.cherry_mark = skip_already_present;\n\n:-). All of that plumbing just to poke at this variable. Looks good to\nme.\n\n> \trevs.limited = 1;\n> \trevs.reverse = 1;\n> \trevs.right_only = 1;\n> diff --git a/sequencer.h b/sequencer.h\n> index 393571e89a..39bb12f624 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -149,7 +149,7 @@ int sequencer_remove_state(struct replay_opts *opts);\n> * `--onto`, we do not want to re-generate the root commits.\n> */\n> #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)\n> -\n> +#define TODO_LIST_SKIP_ALREADY_PRESENT (1U << 7)\n\nThis was another spot that I thought could maybe be turned into an enum,\nbut it's clearly not the fault of your patch, and could easily be turned\ninto #leftoverbits.\n\n> int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> \t\t\t const char **argv, unsigned flags);\n> diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\n> index a1ec501a87..9b52739a10 100755\n> --- a/t/t3402-rebase-merge.sh\n> +++ b/t/t3402-rebase-merge.sh\n> @@ -162,4 +162,81 @@ test_expect_success 'rebase --skip works with two conflicts in a row' '\n> \tgit rebase --skip\n> '\n>\n> +test_expect_success '--no-skip-already-present' '\n> +\tgit init repo &&\n> +\n> +\t# O(1-10) -- O(1-11) -- O(0-10) master\n> +\t# \\\n> +\t# -- O(1-11) -- O(1-12) otherbranch\n> +\n> +\tprintf \"Line %d\\n\" $(test_seq 1 10) >repo/file.txt &&\n> +\tgit -C repo add file.txt &&\n> +\tgit -C repo commit -m \"base commit\" &&\n> +\n> +\tprintf \"Line %d\\n\" $(test_seq 1 11) >repo/file.txt &&\n> +\tgit -C repo commit -a -m \"add 11\" &&\n> +\n> +\tprintf \"Line %d\\n\" $(test_seq 0 10) >repo/file.txt &&\n> +\tgit -C repo commit -a -m \"add 0 delete 11\" &&\n> +\n> +\tgit -C repo checkout -b otherbranch HEAD^^ &&\n> +\tprintf \"Line %d\\n\" $(test_seq 1 11) >repo/file.txt &&\n> +\tgit -C repo commit -a -m \"add 11 in another branch\" &&\n> +\n> +\tprintf \"Line %d\\n\" $(test_seq 1 12) >repo/file.txt &&\n> +\tgit -C repo commit -a -m \"add 12 in another branch\" &&\n> +\n> +\t# Regular rebase fails, because the 1-11 commit is deduplicated\n> +\ttest_must_fail git -C repo rebase --merge master 2> err &&\n> +\ttest_i18ngrep \"error: could not apply.*add 12 in another branch\" err &&\n> +\tgit -C repo rebase --abort &&\n> +\n> +\t# With --no-skip-already-present, it works\n> +\tgit -C repo rebase --merge --no-skip-already-present master\n> +'\n> +\n> +test_expect_success '--no-skip-already-present refrains from reading unneeded blobs' '\n> +\tgit init server &&\n> +\n> +\t# O(1-10) -- O(1-11) -- O(1-12) master\n> +\t# \\\n> +\t# -- O(0-10) otherbranch\n> +\n> +\tprintf \"Line %d\\n\" $(test_seq 1 10) >server/file.txt &&\n> +\tgit -C server add file.txt &&\n> +\tgit -C server commit -m \"merge base\" &&\n> +\n> +\tprintf \"Line %d\\n\" $(test_seq 1 11) >server/file.txt &&\n> +\tgit -C server commit -a -m \"add 11\" &&\n> +\n> +\tprintf \"Line %d\\n\" $(test_seq 1 12) >server/file.txt &&\n> +\tgit -C server commit -a -m \"add 12\" &&\n> +\n> +\tgit -C server checkout -b otherbranch HEAD^^ &&\n> +\tprintf \"Line %d\\n\" $(test_seq 0 10) >server/file.txt &&\n> +\tgit -C server commit -a -m \"add 0\" &&\n> +\n> +\ttest_config -C server uploadpack.allowfilter 1 &&\n> +\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n> +\n> +\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n> +\tgit -C client checkout origin/master &&\n> +\tgit -C client checkout origin/otherbranch &&\n> +\n> +\t# Sanity check to ensure that the blobs from the merge base and \"add\n> +\t# 11\" are missing\n> +\tgit -C client rev-list --objects --all --missing=print >missing_list &&\n> +\tMERGE_BASE_BLOB=$(git -C server rev-parse master^^:file.txt) &&\n> +\tADD_11_BLOB=$(git -C server rev-parse master^:file.txt) &&\n> +\tgrep \"\\\\?$MERGE_BASE_BLOB\" missing_list &&\n> +\tgrep \"\\\\?$ADD_11_BLOB\" missing_list &&\n> +\n> +\tgit -C client rebase --merge --no-skip-already-present origin/master &&\n> +\n> +\t# The blob from the merge base had to be fetched, but not \"add 11\"\n> +\tgit -C client rev-list --objects --all --missing=print >missing_list &&\n> +\t! grep \"\\\\?$MERGE_BASE_BLOB\" missing_list &&\n> +\tgrep \"\\\\?$ADD_11_BLOB\" missing_list\n> +'\n> +\n> test_done\n> --\n> 2.25.1.481.gfbce0eb801-goog\n\nThe tests look good to me. Thanks for working on this!\n\nThanks,\nTaylor\n"},{"id":"392988","messageId":"nycvar.QRO.7.76.6.2003101315100.46@tvgsbejvaqbjf.bet","threadId":"52960","inReplyTo":"20200309205523.121319-1-jonathantanmy@google.com","subject":"Re: [PATCH] rebase --merge: optionally skip upstreamed commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-10T12:17:28Z","receivedAt":"2020-03-10T12:17:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Mon, 9 Mar 2020, Jonathan Tan wrote:\n\n> When rebasing against an upstream that has had many commits since the\n> original branch was created:\n>\n> O -- O -- ... -- O -- O (upstream)\n> \\\n> -- O (my-dev-branch)\n>\n> because \"git rebase\" attempts to exclude commits that are duplicates of\n> upstream ones, it must read the contents of every novel upstream commit,\n> in addition to the tip of the upstream and the merge base. This can be a\n> significant performance hit, especially in a partial clone, wherein a\n> read of an object may end up being a fetch.\n>\n> Add a flag to \"git rebase\" to allow suppression of this feature. This\n> flag only works when using the \"merge\" backend.\n\nI wonder whether we can make this a bit more user-friendly by defaulting\nto `--right-only` if there are no promised objects in the symmetric range,\nand if there _are_ promised objects, to skip `--right-only`, possibly with\nan advice that we did that and how to force it to download the promised\nobjects?\n\nCiao,\nDscho\n"},{"id":"393002","messageId":"20200310155105.18184-1-jonathantanmy@google.com","threadId":"52960","inReplyTo":"20200310021015.GC42010@syl.local","subject":"Re: [PATCH] rebase --merge: optionally skip upstreamed commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-10T15:51:05Z","receivedAt":"2020-03-10T15:51:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Hi Jonathan,\n> \n> This patch makes good sense to me. I left a few notes below, but they\n> are relatively minor, and this seems to be all in a good direction.\n> \n> As a (somewhat) interesting aside, this feature would be useful to me\n> outside of partial clones, since I often have this workflow in my local\n> development wherein 'git rebase' spends quite a bit of time comparing\n> patches on my branch to everything new upstream.\n\nThanks for your review, and it's great to know of a use case that this\nhelps.\n\n> This sentence is a little confusing if you skip over the graph, since it\n> reads: \"When rebasing against an because ... because ...\". It may be\n> clearer if you swap the order of the last two clauses to instead be:\n> \n> it must read the contents of every novel upstream commit, in addition to\n> the tip of the upstream and the merge base, because \"git rebase\"\n> attempts to exclude commits that are duplicates of upstream ones.\n\nSounds good; will do.\n\n> > +--skip-already-present::\n> > +--no-skip-already-present::\n> > +\tSkip commits that are already present in the new upstream.\n> > +\tThis is the default.\n> \n> I believe that you mean '--skip-already-present' is the default, here,\n> but the placement makes it ambiguous, since it is in a paragraph with a\n> header that contains both the positive and negated version of this flag.\n> \n> Maybe this could changed to: s/This/--skip-already-present/'.\n\nWill do.\n\n> > In that case, the fix is easy because 'git rebase' knows to skip\n> > -changes that are already present in the new upstream. So if you say\n> > +changes that are already present in the new upstream (unless\n> > +`--no-skip-already-present` is given). So if you say\n> \n> Extremely minor nit: there is a whitespace change on this line where the\n> original has two spaces between the '.' and 'So', and the new version\n> has only one.\n\nOK - I'll change it to 2 spaces.\n\n> > diff --git a/sequencer.h b/sequencer.h\n> > index 393571e89a..39bb12f624 100644\n> > --- a/sequencer.h\n> > +++ b/sequencer.h\n> > @@ -149,7 +149,7 @@ int sequencer_remove_state(struct replay_opts *opts);\n> > * `--onto`, we do not want to re-generate the root commits.\n> > */\n> > #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)\n> > -\n> > +#define TODO_LIST_SKIP_ALREADY_PRESENT (1U << 7)\n> \n> This was another spot that I thought could maybe be turned into an enum,\n> but it's clearly not the fault of your patch, and could easily be turned\n> into #leftoverbits.\n\nThere was a recent discussion on the list [1] about whether bitsets\nshould be enums, and we decided against it. But anyway we can revisit\nthis later if need be.\n\n[1] https://lore.kernel.org/git/20191016193750.258148-1-jonathantanmy@google.com/\n\nThe changes Taylor suggested were minor, so I'll hold off sending\nanother version until there are more substantial changes requested.\n"},{"id":"393003","messageId":"20200310160035.20252-1-jonathantanmy@google.com","threadId":"52960","inReplyTo":"nycvar.QRO.7.76.6.2003101315100.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] rebase --merge: optionally skip upstreamed commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-10T16:00:35Z","receivedAt":"2020-03-10T16:00:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> I wonder whether we can make this a bit more user-friendly by defaulting\n> to `--right-only` if there are no promised objects in the symmetric range,\n> and if there _are_ promised objects, to skip `--right-only`, possibly with\n> an advice that we did that and how to force it to download the promised\n> objects?\n\nThanks for your suggestion. I'm inclined to think that in a partial\nclone, whether an object is missing or not should not affect the\nbehavior of a Git command (except for lazy fetching and performance),\nbut I can see how this is useful, at least for the purposes of\ndiscoverability and ease of use (good diagnostics for the\nnon-partial-clone case, and better performance for the partial clone\ncase).\n\nBut in any case, I think that this can be built later on top of my\npatch. Even if we have automatic detection of missing objects and\nautomatic selection of functionality, we will still need the CLI\narguments for manual override, so the CLI flags and functionality in\nthis patch are still useful.\n"},{"id":"393029","messageId":"CABPp-BF17qYVZE6BWEh56QYKGojDG6yNz8QawX14XD8zeR=jig@mail.gmail.com","threadId":"52960","inReplyTo":"20200309205523.121319-1-jonathantanmy@google.com","subject":"Re: [PATCH] rebase --merge: optionally skip upstreamed commits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-03-10T18:56:23Z","receivedAt":"2020-03-10T18:56:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Mar 9, 2020 at 1:58 PM Jonathan Tan wrote:\n>\n> When rebasing against an upstream that has had many commits since the\n> original branch was created:\n>\n> O -- O -- ... -- O -- O (upstream)\n> \\\n> -- O (my-dev-branch)\n>\n> because \"git rebase\" attempts to exclude commits that are duplicates of\n> upstream ones, it must read the contents of every novel upstream commit,\n> in addition to the tip of the upstream and the merge base. This can be a\n> significant performance hit, especially in a partial clone, wherein a\n> read of an object may end up being a fetch.\n\nDoes this suggest that the cherry-pick detection is suboptimal and\nneeds to be improved? When rebasing, it is typical that you are just\nrebasing a small number of patches compared to how many exist\nupstream. As such, any upstream patch modifying files outside the set\nof files modified on the rebased side is known to not be PATCHSAME\nwithout looking at those new files. Or is the issue just the sheer\nnumber of upstream commits that modify only the files also modified on\nthe rebased side is large?\n\n> Add a flag to \"git rebase\" to allow suppression of this feature. This\n> flag only works when using the \"merge\" backend.\n\nInteresting. A little over a year ago we discussed not only making\nsuch a change in behavior, but making it the default; see\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.1901211635080.41@tvgsbejvaqbjf.bet/\n(from \"Ooh, that's interesting\" to \"VFS for Git\").\n\nSince that time, we did indeed change the handling of commits that\nbecome empty (so that they now default to dropping), which certainly\ngoes well with the new behavior to skip the cherry-pick detection.\n\nOne note: I think that old thread was wrong about the apply versus the\nmerge backends (which were referred to as \"am\" and \"interactive\" at\nthe time): both the apply and the merge backends have done the\ncherry-pick detection so it wasn't a difference between the two.\n\n> This flag changes the behavior of sequencer_make_script(), called from\n> do_interactive_rebase() <- run_rebase_interactive() <-\n> run_specific_rebase() <- cmd_rebase(). With this flag, limit_list()\n> (indirectly called from sequencer_make_script() through\n> prepare_revision_walk()) will no longer call cherry_pick_list(), and\n> thus PATCHSAME is no longer set. Refraining from setting PATCHSAME both\n> means that the intermediate commits in upstream are no longer read (as\n> shown by the test) and means that no PATCHSAME-caused skipping of\n> commits is done by sequencer_make_script(), either directly or through\n> make_script_with_merges().\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> More improvements for partial clone, but this is a benefit for\n> non-partial-clone as well, hence the way I wrote the commit message (not\n> focusing too much on partial clone) and the documentation.\n\nYes, I've wanted to kill that performance overhead too even without\npartial clones, though I was slightly worried about cases of a known\ncherry-pick no longer cleanly applying and thus forcing the user to\ndetect that it has become empty. I guess that's why it's a flag\ninstead of the new default, but there's something inside of me that\nasks why this special case is detected for the user when other\nconflict cases aren't... Not sure if I'm just pigeonholing on\nperformance too much.\n\n> I've chosen --skip-already-present and --no-skip-already-present to\n> reuse the language already existing in the documentation and to avoid a\n> double negative (e.g. --avoid-checking-if-already-present and\n> --no-avoid-checking-if-already-present) but this causes some clumsiness\n> in the documentation and in the code. Any suggestions for the name are\n> welcome.\n\nI'll add comments on this below.\n\n> I've only implemented this for the \"merge\" backend since I think that\n> there is an effort to migrate \"rebase\" to use the \"merge\" backend by\n> default, and also because \"merge\" uses diff internally which already has\n> the (per-commit) blob batch prefetching.\n\nI understand the first half of your reason here, but I don't follow\nthe second half. The apply backend uses diff to generate the patches,\nbut diff isn't the relevant operation here; it's the rev-list walking,\nand both call the exact same rev-list walk the last time I checked so\nI'm not sure what the difference is here. Am I misunderstanding one\nor more things?\n\n> ---\n> Documentation/git-rebase.txt | 12 +++++-\n> builtin/rebase.c | 10 ++++-\n> sequencer.c | 3 +-\n> sequencer.h | 2 +-\n> t/t3402-rebase-merge.sh | 77 ++++++++++++++++++++++++++++++++++++\n> 5 files changed, 100 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 0c4f038dd6..f73a82b4a9 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -318,6 +318,15 @@ See also INCOMPATIBLE OPTIONS below.\n> +\n> See also INCOMPATIBLE OPTIONS below.\n>\n> +--skip-already-present::\n> +--no-skip-already-present::\n> + Skip commits that are already present in the new upstream.\n> + This is the default.\n> ++\n> +If the skip-if-already-present feature is unnecessary or undesired,\n> +`--no-skip-already-present` may improve performance since it avoids\n> +the need to read the contents of every commit in the new upstream.\n> +\n\nI'm afraid the naming might be pretty opaque and confusing to users.\nEven if we do keep the names, it might help to be clearer about the\nramifications. And there's a missing reference to the option\nincompatibility. Perhaps something like:\n\n--skip-cherry-pick-detection\n--no-skip-cherry-pick-detection\n\n Whether rebase tries to determine if commits are already present\nupstream, i.e. if there are commits which are cherry-picks. If such\ndetection is done, any commits being rebased which are cherry-picks\nwill be dropped, since those commits are already found upstream. If\nsuch detection is not done, those commits will be re-applied, which\nmost likely will result in no new changes (as the changes are already\nupstream) and result in the commit being dropped anyway. cherry-pick\ndetection is the default, but can be expensive in repos with a large\nnumber of upstream commits that need to be read.\n\nSee also INCOMPATIBLE OPTIONS below.\n\n> --rerere-autoupdate::\n> --no-rerere-autoupdate::\n> Allow the rerere mechanism to update the index with the\n> @@ -866,7 +875,8 @@ Only works if the changes (patch IDs based on the diff contents) on\n> 'subsystem' did.\n>\n> In that case, the fix is easy because 'git rebase' knows to skip\n> -changes that are already present in the new upstream. So if you say\n> +changes that are already present in the new upstream (unless\n> +`--no-skip-already-present` is given). So if you say\n> (assuming you're on 'topic')\n> ------------\n> $ git rebase subsystem\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 6154ad8fa5..943211e5bb 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -88,13 +88,15 @@ struct rebase_options {\n> struct strbuf git_format_patch_opt;\n> int reschedule_failed_exec;\n> int use_legacy_rebase;\n> + int skip_already_present;\n> };\n>\n> #define REBASE_OPTIONS_INIT { \\\n> .type = REBASE_UNSPECIFIED, \\\n> .flags = REBASE_NO_QUIET, \\\n> .git_am_opts = ARGV_ARRAY_INIT, \\\n> - .git_format_patch_opt = STRBUF_INIT \\\n> + .git_format_patch_opt = STRBUF_INIT, \\\n> + .skip_already_present = 1 \\\n> }\n>\n> static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n> @@ -373,6 +375,7 @@ static int run_rebase_interactive(struct rebase_options *opts,\n> flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;\n> flags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;\n> flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;\n> + flags |= opts->skip_already_present ? TODO_LIST_SKIP_ALREADY_PRESENT : 0;\n>\n> switch (command) {\n> case ACTION_NONE: {\n> @@ -1507,6 +1510,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> OPT_BOOL(0, \"reschedule-failed-exec\",\n> &reschedule_failed_exec,\n> N_(\"automatically re-schedule any `exec` that fails\")),\n> + OPT_BOOL(0, \"skip-already-present\", &options.skip_already_present,\n> + N_(\"skip changes that are already present in the new upstream\")),\n> OPT_END(),\n> };\n> int i;\n> @@ -1840,6 +1845,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \"interactive or merge options\"));\n> }\n>\n> + if (!options.skip_already_present && !is_interactive(&options))\n> + die(_(\"--no-skip-already-present does not work with the 'am' backend\"));\n> +\n\nwith the *apply* backend, not the 'am' one (the backend was renamed in\ncommit 10cdb9f38a (\"rebase: rename the two primary rebase backends\",\n2020-02-15))\n\n> if (options.signoff) {\n> if (options.type == REBASE_PRESERVE_MERGES)\n> die(\"cannot combine '--signoff' with \"\n> diff --git a/sequencer.c b/sequencer.c\n> index ba90a513b9..752580c017 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4797,12 +4797,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> int keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n> const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? \"p\" : \"pick\";\n> int rebase_merges = flags & TODO_LIST_REBASE_MERGES;\n> + int skip_already_present = !!(flags & TODO_LIST_SKIP_ALREADY_PRESENT);\n>\n> repo_init_revisions(r, &revs, NULL);\n> revs.verbose_header = 1;\n> if (!rebase_merges)\n> revs.max_parents = 1;\n> - revs.cherry_mark = 1;\n> + revs.cherry_mark = skip_already_present;\n> revs.limited = 1;\n> revs.reverse = 1;\n> revs.right_only = 1;\n> diff --git a/sequencer.h b/sequencer.h\n> index 393571e89a..39bb12f624 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -149,7 +149,7 @@ int sequencer_remove_state(struct replay_opts *opts);\n> * `--onto`, we do not want to re-generate the root commits.\n> */\n> #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)\n> -\n> +#define TODO_LIST_SKIP_ALREADY_PRESENT (1U << 7)\n>\n> int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n> const char **argv, unsigned flags);\n> diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\n> index a1ec501a87..9b52739a10 100755\n> --- a/t/t3402-rebase-merge.sh\n> +++ b/t/t3402-rebase-merge.sh\n> @@ -162,4 +162,81 @@ test_expect_success 'rebase --skip works with two conflicts in a row' '\n> git rebase --skip\n> '\n>\n> +test_expect_success '--no-skip-already-present' '\n> + git init repo &&\n> +\n> + # O(1-10) -- O(1-11) -- O(0-10) master\n> + # \\\n> + # -- O(1-11) -- O(1-12) otherbranch\n> +\n> + printf \"Line %d\\n\" $(test_seq 1 10) >repo/file.txt &&\n> + git -C repo add file.txt &&\n> + git -C repo commit -m \"base commit\" &&\n> +\n> + printf \"Line %d\\n\" $(test_seq 1 11) >repo/file.txt &&\n> + git -C repo commit -a -m \"add 11\" &&\n> +\n> + printf \"Line %d\\n\" $(test_seq 0 10) >repo/file.txt &&\n> + git -C repo commit -a -m \"add 0 delete 11\" &&\n> +\n> + git -C repo checkout -b otherbranch HEAD^^ &&\n> + printf \"Line %d\\n\" $(test_seq 1 11) >repo/file.txt &&\n> + git -C repo commit -a -m \"add 11 in another branch\" &&\n> +\n> + printf \"Line %d\\n\" $(test_seq 1 12) >repo/file.txt &&\n> + git -C repo commit -a -m \"add 12 in another branch\" &&\n> +\n> + # Regular rebase fails, because the 1-11 commit is deduplicated\n> + test_must_fail git -C repo rebase --merge master 2> err &&\n> + test_i18ngrep \"error: could not apply.*add 12 in another branch\" err &&\n> + git -C repo rebase --abort &&\n> +\n> + # With --no-skip-already-present, it works\n> + git -C repo rebase --merge --no-skip-already-present master\n> +'\n> +\n> +test_expect_success '--no-skip-already-present refrains from reading unneeded blobs' '\n> + git init server &&\n> +\n> + # O(1-10) -- O(1-11) -- O(1-12) master\n> + # \\\n> + # -- O(0-10) otherbranch\n> +\n> + printf \"Line %d\\n\" $(test_seq 1 10) >server/file.txt &&\n> + git -C server add file.txt &&\n> + git -C server commit -m \"merge base\" &&\n> +\n> + printf \"Line %d\\n\" $(test_seq 1 11) >server/file.txt &&\n> + git -C server commit -a -m \"add 11\" &&\n> +\n> + printf \"Line %d\\n\" $(test_seq 1 12) >server/file.txt &&\n> + git -C server commit -a -m \"add 12\" &&\n> +\n> + git -C server checkout -b otherbranch HEAD^^ &&\n> + printf \"Line %d\\n\" $(test_seq 0 10) >server/file.txt &&\n> + git -C server commit -a -m \"add 0\" &&\n> +\n> + test_config -C server uploadpack.allowfilter 1 &&\n> + test_config -C server uploadpack.allowanysha1inwant 1 &&\n> +\n> + git clone --filter=blob:none \"file://$(pwd)/server\" client &&\n> + git -C client checkout origin/master &&\n> + git -C client checkout origin/otherbranch &&\n> +\n> + # Sanity check to ensure that the blobs from the merge base and \"add\n> + # 11\" are missing\n> + git -C client rev-list --objects --all --missing=print >missing_list &&\n> + MERGE_BASE_BLOB=$(git -C server rev-parse master^^:file.txt) &&\n> + ADD_11_BLOB=$(git -C server rev-parse master^:file.txt) &&\n> + grep \"\\\\?$MERGE_BASE_BLOB\" missing_list &&\n> + grep \"\\\\?$ADD_11_BLOB\" missing_list &&\n> +\n> + git -C client rebase --merge --no-skip-already-present origin/master &&\n> +\n> + # The blob from the merge base had to be fetched, but not \"add 11\"\n> + git -C client rev-list --objects --all --missing=print >missing_list &&\n> + ! grep \"\\\\?$MERGE_BASE_BLOB\" missing_list &&\n> + grep \"\\\\?$ADD_11_BLOB\" missing_list\n> +'\n> +\n> test_done\n> --\n> 2.25.1.481.gfbce0eb801-goog\n\nShould there be a config setting to flip the default? And should\nfeature.experimental and/or feature.manyFiles enable it by default?\n"},{"id":"393047","messageId":"20200310225641.96556-1-jonathantanmy@google.com","threadId":"52960","inReplyTo":"CABPp-BF17qYVZE6BWEh56QYKGojDG6yNz8QawX14XD8zeR=jig@mail.gmail.com","subject":"Re: [PATCH] rebase --merge: optionally skip upstreamed commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-10T22:56:41Z","receivedAt":"2020-03-10T22:56:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Mon, Mar 9, 2020 at 1:58 PM Jonathan Tan wrote:\n> >\n> > When rebasing against an upstream that has had many commits since the\n> > original branch was created:\n> >\n> > O -- O -- ... -- O -- O (upstream)\n> > \\\n> > -- O (my-dev-branch)\n> >\n> > because \"git rebase\" attempts to exclude commits that are duplicates of\n> > upstream ones, it must read the contents of every novel upstream commit,\n> > in addition to the tip of the upstream and the merge base. This can be a\n> > significant performance hit, especially in a partial clone, wherein a\n> > read of an object may end up being a fetch.\n> \n> Does this suggest that the cherry-pick detection is suboptimal and\n> needs to be improved? When rebasing, it is typical that you are just\n> rebasing a small number of patches compared to how many exist\n> upstream. As such, any upstream patch modifying files outside the set\n> of files modified on the rebased side is known to not be PATCHSAME\n> without looking at those new files.\n\nThat's true - and this would drastically reduce the fetches necessary in\npartial clone, perhaps enough that we no longer need this check.\n\nIn the absence of partial clone, this also might improve performance\nsufficiently, such that we no longer need my new option. (Or it might\nnot.)\n\n> Or is the issue just the sheer\n> number of upstream commits that modify only the files also modified on\n> the rebased side is large?\n> \n> > Add a flag to \"git rebase\" to allow suppression of this feature. This\n> > flag only works when using the \"merge\" backend.\n> \n> Interesting. A little over a year ago we discussed not only making\n> such a change in behavior, but making it the default; see\n> https://public-inbox.org/git/nycvar.QRO.7.76.6.1901211635080.41@tvgsbejvaqbjf.bet/\n> (from \"Ooh, that's interesting\" to \"VFS for Git\").\n\nThanks for the pointer! Dscho did propose again to make it the default\n[1] and I replied that this can be done later [2].\n\n[1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.2003101315100.46@tvgsbejvaqbjf.bet/\n[2] https://lore.kernel.org/git/20200310160035.20252-1-jonathantanmy@google.com/\n\n> Yes, I've wanted to kill that performance overhead too even without\n> partial clones, though I was slightly worried about cases of a known\n> cherry-pick no longer cleanly applying and thus forcing the user to\n> detect that it has become empty. I guess that's why it's a flag\n> instead of the new default, but there's something inside of me that\n> asks why this special case is detected for the user when other\n> conflict cases aren't... Not sure if I'm just pigeonholing on\n> performance too much.\n\nI haven't dug into this, but the email you linked [3] shows that this\nbehavior was once-upon-a-time relied upon (\"For example, when I did not\nuse GitGitGadget yet to submit patches...\"). So I don't think we should\nchange it.\n\n[3] https://public-inbox.org/git/nycvar.QRO.7.76.6.1901211635080.41@tvgsbejvaqbjf.bet/\n\n\n> > I've only implemented this for the \"merge\" backend since I think that\n> > there is an effort to migrate \"rebase\" to use the \"merge\" backend by\n> > default, and also because \"merge\" uses diff internally which already has\n> > the (per-commit) blob batch prefetching.\n> \n> I understand the first half of your reason here, but I don't follow\n> the second half. The apply backend uses diff to generate the patches,\n> but diff isn't the relevant operation here; it's the rev-list walking,\n> and both call the exact same rev-list walk the last time I checked so\n> I'm not sure what the difference is here. Am I misunderstanding one\n> or more things?\n\nMaybe just ignore the second half :-)\n\nI thought and wrote the second half because I noticed that somewhere in\nthe \"am\"-related code, blobs were being fetched one by one, but no such\nthing was happening when I used the \"merge\" backend. The rev-list\nwalking doesn't access blobs, I believe.\n\n> > +--skip-already-present::\n> > +--no-skip-already-present::\n> > + Skip commits that are already present in the new upstream.\n> > + This is the default.\n> > ++\n> > +If the skip-if-already-present feature is unnecessary or undesired,\n> > +`--no-skip-already-present` may improve performance since it avoids\n> > +the need to read the contents of every commit in the new upstream.\n> > +\n> \n> I'm afraid the naming might be pretty opaque and confusing to users.\n> Even if we do keep the names, it might help to be clearer about the\n> ramifications. And there's a missing reference to the option\n> incompatibility. Perhaps something like:\n> \n> --skip-cherry-pick-detection\n> --no-skip-cherry-pick-detection\n> \n> Whether rebase tries to determine if commits are already present\n> upstream, i.e. if there are commits which are cherry-picks. If such\n> detection is done, any commits being rebased which are cherry-picks\n> will be dropped, since those commits are already found upstream. If\n> such detection is not done, those commits will be re-applied, which\n> most likely will result in no new changes (as the changes are already\n> upstream) and result in the commit being dropped anyway. cherry-pick\n> detection is the default, but can be expensive in repos with a large\n> number of upstream commits that need to be read.\n> \n> See also INCOMPATIBLE OPTIONS below.\n\nI understand that commits being already present in upstream is usually\ndue to cherry-picking, but I don't think that's always the case, so\nperhaps there is some imprecision here. But this might be better - in\nparticular, documentation and code will not be so clumsy (the \"no\", or\n0, is the status quo, and the lack of \"no\", or 1, requires special\nhandling).\n\n> > + if (!options.skip_already_present && !is_interactive(&options))\n> > + die(_(\"--no-skip-already-present does not work with the 'am' backend\"));\n> > +\n> \n> with the *apply* backend, not the 'am' one (the backend was renamed in\n> commit 10cdb9f38a (\"rebase: rename the two primary rebase backends\",\n> 2020-02-15))\n\nThanks. Will do.\n\n> Should there be a config setting to flip the default? And should\n> feature.experimental and/or feature.manyFiles enable it by default?\n\nAs above, I think this can be done in a separate patch.\n"},{"id":"393151","messageId":"20200312180427.192096-1-jonathantanmy@google.com","threadId":"52960","inReplyTo":"20200310225641.96556-1-jonathantanmy@google.com","subject":"Re: [PATCH] rebase --merge: optionally skip upstreamed commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-12T18:04:27Z","receivedAt":"2020-03-12T18:04:32Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Does this suggest that the cherry-pick detection is suboptimal and\n> > needs to be improved? When rebasing, it is typical that you are just\n> > rebasing a small number of patches compared to how many exist\n> > upstream. As such, any upstream patch modifying files outside the set\n> > of files modified on the rebased side is known to not be PATCHSAME\n> > without looking at those new files.\n> \n> That's true - and this would drastically reduce the fetches necessary in\n> partial clone, perhaps enough that we no longer need this check.\n> \n> In the absence of partial clone, this also might improve performance\n> sufficiently, such that we no longer need my new option. (Or it might\n> not.)\n\nI took a further look at this. patch-ids.c and its caller\n(cherry_pick_list() in revision.c) implement duplicate checking by first\ngenerating full diff outputs for the commits in the shorter side,\nputting them in a hashmap keyed by the SHA-1 of the diff output (and\nvalues being the commit itself), and then generating full diff outputs\nfor the commits in the longer side and checking them against the\nhashmap. When processing the shorter side, we could also generate\nfilename-only diffs and put their hashes into a hashset; so when\nprocessing the longer side, we could generate the filename-only diff\nfirst (without reading any blobs) and checking them against our new\nhashset, and only if it appears in our new hashset, then do we generate\nthe full diff (thus reading blobs).\n\nOne issue with this is unpredictability to the user (since which blobs\nget read depend on which side is longer), but that seems resolvable by\nnot doing any length checks but always reading the blobs on the right\nside (that is, the non-upstream side).\n\nSo I would say that yes, the cherry-pick detection is suboptimal and\ncould be improved. So the question is...what to do with my patch? An\nargument could be made that my patch should be dropped because an\nimprovement in cherry-pick detection would eliminate the need for the\noption I'm introducing in my patch, but after some thought, I think that\nthis option will still be useful even with cherry-pick detection. If we\nmove in a direction where not only blobs but also trees (or even\ncommits) are omitted, we'll definitely want this new option. And even if\na user is not using partial clone at all, I think it is still useful to\nsuppress both the filtering of commits (e.g. when upstream has a commit\nthen revert, it would be reasonable to cherry-pick the same commit on\ntop) and reduce disk reads (although I don't know if this will be the\ncase in practice).\n"},{"id":"393182","messageId":"CABPp-BE83ZhezkgmwatxAhqh4rptMUggcjSwBeiSByyPTUi6Lw@mail.gmail.com","threadId":"52960","inReplyTo":"20200312180427.192096-1-jonathantanmy@google.com","subject":"Re: [PATCH] rebase --merge: optionally skip upstreamed commits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-03-12T22:40:50Z","receivedAt":"2020-03-12T22:41:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":" On Thu, Mar 12, 2020 at 11:04 AM Jonathan Tan wrote:\n>\n> > > Does this suggest that the cherry-pick detection is suboptimal and\n> > > needs to be improved? When rebasing, it is typical that you are just\n> > > rebasing a small number of patches compared to how many exist\n> > > upstream. As such, any upstream patch modifying files outside the set\n> > > of files modified on the rebased side is known to not be PATCHSAME\n> > > without looking at those new files.\n> >\n> > That's true - and this would drastically reduce the fetches necessary in\n> > partial clone, perhaps enough that we no longer need this check.\n> >\n> > In the absence of partial clone, this also might improve performance\n> > sufficiently, such that we no longer need my new option. (Or it might\n> > not.)\n>\n> I took a further look at this. patch-ids.c and its caller\n> (cherry_pick_list() in revision.c) implement duplicate checking by first\n> generating full diff outputs for the commits in the shorter side,\n> putting them in a hashmap keyed by the SHA-1 of the diff output (and\n> values being the commit itself), and then generating full diff outputs\n> for the commits in the longer side and checking them against the\n> hashmap. When processing the shorter side, we could also generate\n> filename-only diffs and put their hashes into a hashset; so when\n> processing the longer side, we could generate the filename-only diff\n> first (without reading any blobs) and checking them against our new\n> hashset, and only if it appears in our new hashset, then do we generate\n> the full diff (thus reading blobs).\n>\n> One issue with this is unpredictability to the user (since which blobs\n> get read depend on which side is longer), but that seems resolvable by\n> not doing any length checks but always reading the blobs on the right\n> side (that is, the non-upstream side).\n>\n> So I would say that yes, the cherry-pick detection is suboptimal and\n> could be improved.\n\nSweet, thanks for doing this investigative work! Sounds promising.\n\n> So the question is...what to do with my patch? An\n> argument could be made that my patch should be dropped because an\n> improvement in cherry-pick detection would eliminate the need for the\n> option I'm introducing in my patch, but after some thought, I think that\n> this option will still be useful even with cherry-pick detection.\n\nThe option may be totally justified here, but can I go on a possible\ntangent and vent for a little bit? We seem to introduce options an\nawful lot. While options can be valuable they also have a cost, and I\nthink we tend not to acknowledge that. Some of the negatives:\n\n- Developers add options when they run into bugs instead of fixing\nbugs (usually they don't realize that the behavior in question is a\nbug, but that's exacerbated by a willingness to add options and not\nconsider costs)\n- Developers add options without considering combinations of options\nand what they mean (though it's hard to fault them because considering\ncombinations becomes harder and harder with the more options we have;\nit's a negative feedback cycle)\n- Growth in number of options leads to code that is hard or impossible\nto refactor based on a maze of competing options with myriad edge and\ncorner cases that are fundamentally broken\n- Users get overloaded by the sheer number of options and minor distinctions\n\nThe fourth case is probably obvious, so let me just include some\nexamples of the first three cases above:\n* Commits b00bf1c9a8 (\"git-rebase: make --allow-empty-message the\ndefault\", 2018-06-27), 22a69fda19 (\"git-rebase.txt: update description\nof --allow-empty-message\", 2020-01-16), and d48e5e21da (\"rebase\n(interactive-backend): make --keep-empty the default\", 2020-02-15)\nnoted that options that previously existed were just workarounds to\nbuggy behavior and the flags should have been always on.\n* Commit e86bbcf987 (\"clean: disambiguate the definition of -d\",\n2019-09-17) showed a pretty hairy case where the combination of\noptions led to cases where I not only didn't know how to implement\ncorrect behavior, I didn't even know how to state what the desired\nbehavior for end-users was. Despite a few different reports over a\nyear and a half that I had a series that fixed some known issues for\nusers the series languished because I couldn't get an answer on what\nwas right. See also\nhttps://lore.kernel.org/git/20190905154735.29784-1-newren@gmail.com/\n* See the huge \"Behavioral differences\" section of the git-rebase\nmanpage, and a combination of rants from me on dir.c:\n - https://lore.kernel.org/git/20190905154735.29784-1-newren@gmail.com/\n - https://lore.kernel.org/git/CABPp-BFG3FkTkC=L1v97LUksndkOmCN8ZhNJh5eoNdquE7v9DA@mail.gmail.com/\n - https://lore.kernel.org/git/pull.676.v3.git.git.1576571586.gitgitgadget@gmail.com/\n - The commit message of\nhttps://lore.kernel.org/git/d3136ef52f3306d465a5a6004cdc9ba5b1ae4148.1580495486.git.gitgitgadget@gmail.com/\n\n> If we\n> move in a direction where not only blobs but also trees (or even\n> commits) are omitted, we'll definitely want this new option.\n\nWhy would this new option be needed if we omitted trees? If trees\nare omitted based on something like sparse-checkouts, then they are\nomitted based on path; shouldn't we be able to avoid walking trees\njust by noting they modified some path outside a requested sparse\ncheckout?\n\nI want grep, log, etc. to behave within the cone of a sparse checkout,\nwhich means that I need trees of upstream branches within the relevant\npaths anyway. But theoretically I should certainly be able to avoid\nwalking trees outside those paths.\n\n> And even if\n> a user is not using partial clone at all, I think it is still useful to\n> suppress both the filtering of commits (e.g. when upstream has a commit\n> then revert, it would be reasonable to cherry-pick the same commit on\n> top) and reduce disk reads (although I don't know if this will be the\n> case in practice).\n\nThat sounds like yet another argument that the behavior you're arguing\nfor should be the default, not a flag we make the users pick to\nworkaround bugs. Yes, sometimes weird behaviors beget usecases (cue\nlink to xkcd's comic on emacs spacebar overheating) and we need to\nprovide transition plans, but I think this might be a case where\ntransitioning makes sense. From a high level, here's my guess\n(emphasis on guess) at the history:\n\n* am checked for upstream patches, because apply would get confused\ntrying to apply an already applied patch\n* legacy-interactive-rebase would check for upstream patches as a\nperformance optimization because having to shell out to a separate\ncherry-pick process for each commit is slow (and may have also been\ndone partially to match am, even though am did it as a workaround)\n\nAnd now we're in the state where:\n* The check-for-upstream bits hurt performance, significantly enough\nthat we have three different reports of folks not liking it (you, me,\nand Taylor)\n* It actively does the wrong thing in cases such as revert + re-apply\nsequences, which exist in practice (and exist a lot more than they\nshould, but they absolutely do exist)\n\nWe've made changes in other places (e.g. opening an editor for merge\nor rebase, push.default, etc.); is there any reason a similar change\nwouldn't be justified here?\n"},{"id":"393255","messageId":"CABPp-BGcQHT5PkqYw3YRWOv8kbiSumJ+YQ3owkd_7viKWs3q7A@mail.gmail.com","threadId":"52960","inReplyTo":"CABPp-BE83ZhezkgmwatxAhqh4rptMUggcjSwBeiSByyPTUi6Lw@mail.gmail.com","subject":"Re: [PATCH] rebase --merge: optionally skip upstreamed commits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-03-14T08:04:55Z","receivedAt":"2020-03-15T03:22:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Mar 12, 2020 at 3:40 PM Elijah Newren wrote:\n>\n> On Thu, Mar 12, 2020 at 11:04 AM Jonathan Tan wrote:\n> >\n> > > > Does this suggest that the cherry-pick detection is suboptimal and\n> > > > needs to be improved? When rebasing, it is typical that you are just\n> > > > rebasing a small number of patches compared to how many exist\n> > > > upstream. As such, any upstream patch modifying files outside the set\n> > > > of files modified on the rebased side is known to not be PATCHSAME\n> > > > without looking at those new files.\n> > >\n> > > That's true - and this would drastically reduce the fetches necessary in\n> > > partial clone, perhaps enough that we no longer need this check.\n> > >\n> > > In the absence of partial clone, this also might improve performance\n> > > sufficiently, such that we no longer need my new option. (Or it might\n> > > not.)\n> >\n> > I took a further look at this. patch-ids.c and its caller\n> > (cherry_pick_list() in revision.c) implement duplicate checking by first\n> > generating full diff outputs for the commits in the shorter side,\n> > putting them in a hashmap keyed by the SHA-1 of the diff output (and\n> > values being the commit itself), and then generating full diff outputs\n> > for the commits in the longer side and checking them against the\n> > hashmap. When processing the shorter side, we could also generate\n> > filename-only diffs and put their hashes into a hashset; so when\n> > processing the longer side, we could generate the filename-only diff\n> > first (without reading any blobs) and checking them against our new\n> > hashset, and only if it appears in our new hashset, then do we generate\n> > the full diff (thus reading blobs).\n> >\n> > One issue with this is unpredictability to the user (since which blobs\n> > get read depend on which side is longer), but that seems resolvable by\n> > not doing any length checks but always reading the blobs on the right\n> > side (that is, the non-upstream side).\n> >\n> > So I would say that yes, the cherry-pick detection is suboptimal and\n> > could be improved.\n>\n> Sweet, thanks for doing this investigative work! Sounds promising.\n>\n> > So the question is...what to do with my patch? An\n> > argument could be made that my patch should be dropped because an\n> > improvement in cherry-pick detection would eliminate the need for the\n> > option I'm introducing in my patch, but after some thought, I think that\n> > this option will still be useful even with cherry-pick detection.\n>\n> The option may be totally justified here, but can I go on a possible\n> tangent and vent for a little bit? We seem to introduce options an\n> awful lot. While options can be valuable they also have a cost, and I\n> think we tend not to acknowledge that. Some of the negatives:\n>\n> - Developers add options when they run into bugs instead of fixing\n> bugs (usually they don't realize that the behavior in question is a\n> bug, but that's exacerbated by a willingness to add options and not\n> consider costs)\n> - Developers add options without considering combinations of options\n> and what they mean (though it's hard to fault them because considering\n> combinations becomes harder and harder with the more options we have;\n> it's a negative feedback cycle)\n> - Growth in number of options leads to code that is hard or impossible\n> to refactor based on a maze of competing options with myriad edge and\n> corner cases that are fundamentally broken\n> - Users get overloaded by the sheer number of options and minor distinctions\n>\n> The fourth case is probably obvious, so let me just include some\n> examples of the first three cases above:\n> * Commits b00bf1c9a8 (\"git-rebase: make --allow-empty-message the\n> default\", 2018-06-27), 22a69fda19 (\"git-rebase.txt: update description\n> of --allow-empty-message\", 2020-01-16), and d48e5e21da (\"rebase\n> (interactive-backend): make --keep-empty the default\", 2020-02-15)\n> noted that options that previously existed were just workarounds to\n> buggy behavior and the flags should have been always on.\n> * Commit e86bbcf987 (\"clean: disambiguate the definition of -d\",\n> 2019-09-17) showed a pretty hairy case where the combination of\n> options led to cases where I not only didn't know how to implement\n> correct behavior, I didn't even know how to state what the desired\n> behavior for end-users was. Despite a few different reports over a\n> year and a half that I had a series that fixed some known issues for\n> users the series languished because I couldn't get an answer on what\n> was right. See also\n> https://lore.kernel.org/git/20190905154735.29784-1-newren@gmail.com/\n> * See the huge \"Behavioral differences\" section of the git-rebase\n> manpage, and a combination of rants from me on dir.c:\n> - https://lore.kernel.org/git/20190905154735.29784-1-newren@gmail.com/\n> - https://lore.kernel.org/git/CABPp-BFG3FkTkC=L1v97LUksndkOmCN8ZhNJh5eoNdquE7v9DA@mail.gmail.com/\n> - https://lore.kernel.org/git/pull.676.v3.git.git.1576571586.gitgitgadget@gmail.com/\n> - The commit message of\n> https://lore.kernel.org/git/d3136ef52f3306d465a5a6004cdc9ba5b1ae4148.1580495486.git.gitgitgadget@gmail.com/\n>\n> > If we\n> > move in a direction where not only blobs but also trees (or even\n> > commits) are omitted, we'll definitely want this new option.\n>\n> Why would this new option be needed if we omitted trees? If trees\n> are omitted based on something like sparse-checkouts, then they are\n> omitted based on path; shouldn't we be able to avoid walking trees\n> just by noting they modified some path outside a requested sparse\n> checkout?\n>\n> I want grep, log, etc. to behave within the cone of a sparse checkout,\n> which means that I need trees of upstream branches within the relevant\n> paths anyway. But theoretically I should certainly be able to avoid\n> walking trees outside those paths.\n>\n> > And even if\n> > a user is not using partial clone at all, I think it is still useful to\n> > suppress both the filtering of commits (e.g. when upstream has a commit\n> > then revert, it would be reasonable to cherry-pick the same commit on\n> > top) and reduce disk reads (although I don't know if this will be the\n> > case in practice).\n>\n> That sounds like yet another argument that the behavior you're arguing\n> for should be the default, not a flag we make the users pick to\n> workaround bugs. Yes, sometimes weird behaviors beget usecases (cue\n> link to xkcd's comic on emacs spacebar overheating) and we need to\n> provide transition plans, but I think this might be a case where\n> transitioning makes sense. From a high level, here's my guess\n> (emphasis on guess) at the history:\n>\n> * am checked for upstream patches, because apply would get confused\n> trying to apply an already applied patch\n> * legacy-interactive-rebase would check for upstream patches as a\n> performance optimization because having to shell out to a separate\n> cherry-pick process for each commit is slow (and may have also been\n> done partially to match am, even though am did it as a workaround)\n\nOr maybe that wasn't the reasoning? I'm having a hard time parsing the\nhistory to verify:\n\na6ec3c1599 (\"git-rebase: Use --ignore-if-in-upstream option when\n executing git-format-patch.\", 2006-10-03)\n '''This reduces the number of conflicts when rebasing after a series of\n patches to the same piece of code is committed upstream.'''\n\n96ffe892e3 (\"rebase -i: ignore patches that are already in the\n upstream\", 2007-08-01)\n '''Non-interactive rebase had this from the beginning -- match it by\n using --cherry-pick option to rev-list.'''\n\nand related: 1e0dacdbdb (\"rebase: omit patch-identical commits with\n --fork-point\", 2014-07-16)\n\n> And now we're in the state where:\n> * The check-for-upstream bits hurt performance, significantly enough\n> that we have three different reports of folks not liking it (you, me,\n> and Taylor)\n> * It actively does the wrong thing in cases such as revert + re-apply\n> sequences, which exist in practice (and exist a lot more than they\n> should, but they absolutely do exist)\n\nThough these are definitely still problems with\ncheck-if-upstream-already behavior.\n\n> We've made changes in other places (e.g. opening an editor for merge\n> or rebase, push.default, etc.); is there any reason a similar change\n> wouldn't be justified here?\n\nAfter another day of thought, and my attempt to figure out the reason\nabove, perhaps my assumptions about the reason behind the original\nbehavior, any my assumptions about the sanity of switching the default\nmight not be as grounded as I thought. Thus, my worries about yet\nanother flag may be overstated as well, at least in this case.\n"},{"id":"393341","messageId":"20200317030324.190751-1-jonathantanmy@google.com","threadId":"52960","inReplyTo":"CABPp-BGcQHT5PkqYw3YRWOv8kbiSumJ+YQ3owkd_7viKWs3q7A@mail.gmail.com","subject":"Re: [PATCH] rebase --merge: optionally skip upstreamed commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-17T03:03:24Z","receivedAt":"2020-03-17T03:03:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > We've made changes in other places (e.g. opening an editor for merge\n> > or rebase, push.default, etc.); is there any reason a similar change\n> > wouldn't be justified here?\n> \n> After another day of thought, and my attempt to figure out the reason\n> above, perhaps my assumptions about the reason behind the original\n> behavior, any my assumptions about the sanity of switching the default\n> might not be as grounded as I thought. Thus, my worries about yet\n> another flag may be overstated as well, at least in this case.\n\nThanks for the further investigation. For what it's worth, I do agree\nthat we should think of the cost of introducing an option before\nintroducing it. Admittedly in my write-up [1], I mentioned my\ninvestigation into speeding up existing behavior enough to not need my\nnew feature, but didn't mention the possibility of just changing the\nexisting behavior. (But it seemed to me that this existing behavior is\npresented as a desirable feature, so I didn't think of changing it.)\n\n[1] https://lore.kernel.org/git/20200312180427.192096-1-jonathantanmy@google.com/\n\nThis question (from your other email [2]) is probably moot if we're\ngoing to introduce this option anyway, but just to answer it:\n\n> > If we\n> > move in a direction where not only blobs but also trees (or even\n> > commits) are omitted, we'll definitely want this new option.\n> \n> Why would this new option be needed if we omitted trees? If trees\n> are omitted based on something like sparse-checkouts, then they are\n> omitted based on path; shouldn't we be able to avoid walking trees\n> just by noting they modified some path outside a requested sparse\n> checkout?\n> \n> I want grep, log, etc. to behave within the cone of a sparse checkout,\n> which means that I need trees of upstream branches within the relevant\n> paths anyway. But theoretically I should certainly be able to avoid\n> walking trees outside those paths.\n\nI haven't given much thought to it, but the diffing mechanism will need\nto receive a whitelist of paths and, if it ever needs to traverse\noutside those, will need to abort with \"there's a difference outside\nthis whitelist\". I don't know if it supports such a thing now.\n\n[2] https://lore.kernel.org/git/CABPp-BE83ZhezkgmwatxAhqh4rptMUggcjSwBeiSByyPTUi6Lw@mail.gmail.com/\n\nI'll give some time for others to respond, and will send out a v2 with\nyour and Taylor's suggestions implemented.\n"},{"id":"393403","messageId":"20200318173051.25875-1-jonathantanmy@google.com","threadId":"52960","inReplyTo":"20200309205523.121319-1-jonathantanmy@google.com","subject":"[PATCH v2] rebase --merge: optionally skip upstreamed commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-18T17:30:51Z","receivedAt":"2020-03-18T17:30:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When rebasing against an upstream that has had many commits since the\noriginal branch was created:\n\n O -- O -- ... -- O -- O (upstream)\n \\\n -- O (my-dev-branch)\n\nit must read the contents of every novel upstream commit, in addition to\nthe tip of the upstream and the merge base, because \"git rebase\"\nattempts to exclude commits that are duplicates of upstream ones. This\ncan be a significant performance hit, especially in a partial clone,\nwherein a read of an object may end up being a fetch.\n\nAdd a flag to \"git rebase\" to allow suppression of this feature. This\nflag only works when using the \"merge\" backend.\n\nThis flag changes the behavior of sequencer_make_script(), called from\ndo_interactive_rebase() <- run_rebase_interactive() <-\nrun_specific_rebase() <- cmd_rebase(). With this flag, limit_list()\n(indirectly called from sequencer_make_script() through\nprepare_revision_walk()) will no longer call cherry_pick_list(), and\nthus PATCHSAME is no longer set. Refraining from setting PATCHSAME both\nmeans that the intermediate commits in upstream are no longer read (as\nshown by the test) and means that no PATCHSAME-caused skipping of\ncommits is done by sequencer_make_script(), either directly or through\nmake_script_with_merges().\n\nSigned-off-by: Jonathan Tan \n---\nNew in V2: changed parameter name, used Taylor's commit message\nsuggestions, and used Elijah's documentation suggestions.\n---\n Documentation/git-rebase.txt | 20 +++++++++-\n builtin/rebase.c | 7 ++++\n sequencer.c | 3 +-\n sequencer.h | 2 +-\n t/t3402-rebase-merge.sh | 77 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 106 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 0c4f038dd6..4629eb573f 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -318,6 +318,20 @@ See also INCOMPATIBLE OPTIONS below.\n +\n See also INCOMPATIBLE OPTIONS below.\n \n+--skip-cherry-pick-detection::\n+--no-skip-cherry-pick-detection::\n+\tWhether rebase tries to determine if commits are already present\n+\tupstream, i.e. if there are commits which are cherry-picks. If such\n+\tdetection is done, any commits being rebased which are cherry-picks\n+\twill be dropped, since those commits are already found upstream. If\n+\tsuch detection is not done, those commits will be re-applied, which\n+\tmost likely will result in no new changes (as the changes are already\n+\tupstream) and result in the commit being dropped anyway. cherry-pick\n+\tdetection is the default, but can be expensive in repos with a large\n+\tnumber of upstream commits that need to be read.\n++\n+See also INCOMPATIBLE OPTIONS below.\n+\n --rerere-autoupdate::\n --no-rerere-autoupdate::\n \tAllow the rerere mechanism to update the index with the\n@@ -568,6 +582,9 @@ In addition, the following pairs of options are incompatible:\n * --keep-base and --onto\n * --keep-base and --root\n \n+Also, the --skip-cherry-pick-detection option requires the use of the merge\n+backend (e.g., through --merge).\n+\n BEHAVIORAL DIFFERENCES\n -----------------------\n \n@@ -866,7 +883,8 @@ Only works if the changes (patch IDs based on the diff contents) on\n 'subsystem' did.\n \n In that case, the fix is easy because 'git rebase' knows to skip\n-changes that are already present in the new upstream. So if you say\n+changes that are already present in the new upstream (unless\n+`--skip-cherry-pick-detection` is given). So if you say\n (assuming you're on 'topic')\n ------------\n $ git rebase subsystem\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 6154ad8fa5..100b8872af 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -88,6 +88,7 @@ struct rebase_options {\n \tstruct strbuf git_format_patch_opt;\n \tint reschedule_failed_exec;\n \tint use_legacy_rebase;\n+\tint skip_cherry_pick_detection;\n };\n \n #define REBASE_OPTIONS_INIT {\t\t\t \t\\\n@@ -373,6 +374,7 @@ static int run_rebase_interactive(struct rebase_options *opts,\n \tflags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;\n \tflags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;\n \tflags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;\n+\tflags |= opts->skip_cherry_pick_detection ? TODO_LIST_SKIP_CHERRY_PICK_DETECTION : 0;\n \n \tswitch (command) {\n \tcase ACTION_NONE: {\n@@ -1507,6 +1509,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"reschedule-failed-exec\",\n \t\t\t &reschedule_failed_exec,\n \t\t\t N_(\"automatically re-schedule any `exec` that fails\")),\n+\t\tOPT_BOOL(0, \"skip-cherry-pick-detection\", &options.skip_cherry_pick_detection,\n+\t\t\t N_(\"skip changes that are already present in the new upstream\")),\n \t\tOPT_END(),\n \t};\n \tint i;\n@@ -1840,6 +1844,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t \"interactive or merge options\"));\n \t}\n \n+\tif (options.skip_cherry_pick_detection && !is_interactive(&options))\n+\t\tdie(_(\"--skip-cherry-pick-detection does not work with the 'apply' backend\"));\n+\n \tif (options.signoff) {\n \t\tif (options.type == REBASE_PRESERVE_MERGES)\n \t\t\tdie(\"cannot combine '--signoff' with \"\ndiff --git a/sequencer.c b/sequencer.c\nindex ba90a513b9..8b2cae3b69 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4797,12 +4797,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \tint keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n \tconst char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? \"p\" : \"pick\";\n \tint rebase_merges = flags & TODO_LIST_REBASE_MERGES;\n+\tint skip_cherry_pick_detection = flags & TODO_LIST_SKIP_CHERRY_PICK_DETECTION;\n \n \trepo_init_revisions(r, &revs, NULL);\n \trevs.verbose_header = 1;\n \tif (!rebase_merges)\n \t\trevs.max_parents = 1;\n-\trevs.cherry_mark = 1;\n+\trevs.cherry_mark = !skip_cherry_pick_detection;\n \trevs.limited = 1;\n \trevs.reverse = 1;\n \trevs.right_only = 1;\ndiff --git a/sequencer.h b/sequencer.h\nindex 393571e89a..a54ea696c2 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -149,7 +149,7 @@ int sequencer_remove_state(struct replay_opts *opts);\n * `--onto`, we do not want to re-generate the root commits.\n */\n #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)\n-\n+#define TODO_LIST_SKIP_CHERRY_PICK_DETECTION (1U << 7)\n \n int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \t\t\t const char **argv, unsigned flags);\ndiff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\nindex a1ec501a87..290c79e0f6 100755\n--- a/t/t3402-rebase-merge.sh\n+++ b/t/t3402-rebase-merge.sh\n@@ -162,4 +162,81 @@ test_expect_success 'rebase --skip works with two conflicts in a row' '\n \tgit rebase --skip\n '\n \n+test_expect_success '--skip-cherry-pick-detection' '\n+\tgit init repo &&\n+\n+\t# O(1-10) -- O(1-11) -- O(0-10) master\n+\t# \\\n+\t# -- O(1-11) -- O(1-12) otherbranch\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 10) >repo/file.txt &&\n+\tgit -C repo add file.txt &&\n+\tgit -C repo commit -m \"base commit\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 11) >repo/file.txt &&\n+\tgit -C repo commit -a -m \"add 11\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 0 10) >repo/file.txt &&\n+\tgit -C repo commit -a -m \"add 0 delete 11\" &&\n+\n+\tgit -C repo checkout -b otherbranch HEAD^^ &&\n+\tprintf \"Line %d\\n\" $(test_seq 1 11) >repo/file.txt &&\n+\tgit -C repo commit -a -m \"add 11 in another branch\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 12) >repo/file.txt &&\n+\tgit -C repo commit -a -m \"add 12 in another branch\" &&\n+\n+\t# Regular rebase fails, because the 1-11 commit is deduplicated\n+\ttest_must_fail git -C repo rebase --merge master 2> err &&\n+\ttest_i18ngrep \"error: could not apply.*add 12 in another branch\" err &&\n+\tgit -C repo rebase --abort &&\n+\n+\t# With --skip-cherry-pick-detection, it works\n+\tgit -C repo rebase --merge --skip-cherry-pick-detection master\n+'\n+\n+test_expect_success '--skip-cherry-pick-detection refrains from reading unneeded blobs' '\n+\tgit init server &&\n+\n+\t# O(1-10) -- O(1-11) -- O(1-12) master\n+\t# \\\n+\t# -- O(0-10) otherbranch\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 10) >server/file.txt &&\n+\tgit -C server add file.txt &&\n+\tgit -C server commit -m \"merge base\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 11) >server/file.txt &&\n+\tgit -C server commit -a -m \"add 11\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 12) >server/file.txt &&\n+\tgit -C server commit -a -m \"add 12\" &&\n+\n+\tgit -C server checkout -b otherbranch HEAD^^ &&\n+\tprintf \"Line %d\\n\" $(test_seq 0 10) >server/file.txt &&\n+\tgit -C server commit -a -m \"add 0\" &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n+\tgit -C client checkout origin/master &&\n+\tgit -C client checkout origin/otherbranch &&\n+\n+\t# Sanity check to ensure that the blobs from the merge base and \"add\n+\t# 11\" are missing\n+\tgit -C client rev-list --objects --all --missing=print >missing_list &&\n+\tMERGE_BASE_BLOB=$(git -C server rev-parse master^^:file.txt) &&\n+\tADD_11_BLOB=$(git -C server rev-parse master^:file.txt) &&\n+\tgrep \"\\\\?$MERGE_BASE_BLOB\" missing_list &&\n+\tgrep \"\\\\?$ADD_11_BLOB\" missing_list &&\n+\n+\tgit -C client rebase --merge --skip-cherry-pick-detection origin/master &&\n+\n+\t# The blob from the merge base had to be fetched, but not \"add 11\"\n+\tgit -C client rev-list --objects --all --missing=print >missing_list &&\n+\t! grep \"\\\\?$MERGE_BASE_BLOB\" missing_list &&\n+\tgrep \"\\\\?$ADD_11_BLOB\" missing_list\n+'\n+\n test_done\n-- \n2.25.1.481.gfbce0eb801-goog\n\n"},{"id":"393405","messageId":"xmqqpnd9fql0.fsf@gitster.c.googlers.com","threadId":"52960","inReplyTo":"20200318173051.25875-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] rebase --merge: optionally skip upstreamed commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-18T18:47:07Z","receivedAt":"2020-03-18T18:47:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> When rebasing against an upstream that has had many commits since the\n> original branch was created:\n>\n> O -- O -- ... -- O -- O (upstream)\n> \\\n> -- O (my-dev-branch)\n>\n> it must read the contents of every novel upstream commit, in addition to\n> the tip of the upstream and the merge base, because \"git rebase\"\n> attempts to exclude commits that are duplicates of upstream ones. This\n> can be a significant performance hit, especially in a partial clone,\n> wherein a read of an object may end up being a fetch.\n\nOK. I presume that we do this by comparing patch IDs?\n\nTotal disabling would of course is OK as a feature, especially for\nthe first cut, but I wonder if it would be a reasonable idea to use\nsome heuristic to keep the current \"filter the same change\" feature\nas much as possible but optimize it by filtering the novel upstream\ncommits without hitting their trees and blobs (I am assuming that\nyou at least are aware of and have the commit objects on the\nupstream side).\n\nThe most false-negative-prone approach is just to compare the\n of a candidate upstream commit with\nwhat you have---if that author does not appear on my-dev-branch, it\nis very unlikely that your change has been accepted upstream. Of\ncourse, two people who independently discover the same solution is\nnot all that rare, so it does risk false-negative to take too little\nclue from the commits to compare, but at least it is not worse than\nwhat you are proposing here ;-) And if one of your commits on\nmy-dev-branch _might_ be identical to one of the novel upstream ones,\nat that point, we could dig deeper to actually compute the patch ID\nby fetching the upstream's tree.\n\nThat's all totally outside the scope of this patch. It is just a\nrandom thought to see if anybody wants to pursue to make the topic\neven better, possible after it lands.\n\n> New in V2: changed parameter name, used Taylor's commit message\n> suggestions, and used Elijah's documentation suggestions.\n\nHmph, what was it called earlier? My gut reaction without much\nthinking finds --no-skip-* a bit confusing double-negation and\nsuspect \"--[no-]detect-cherry-pick\" (which defaults to true for\nbackward compatibility) may feel more natural, but I suspect (I do\nnot recall details of the discussion on v1) it has been already\ndiscussed and people found --no-skip-* is OK (in which case I won't\nobject)?\n\nI also wonder if --detect-cherry-pick=(yes|no|auto) may give a\nbetter end-user experience, with \"auto\" meaning \"do run patch-ID\nbased filtering, but if we know it will be expensive (e.g. the\nrepository is sparsely cloned), please skip it\". That way, there\nmay appear other reasons that makes patch-ID computation expensive\nnow or in the fiture, and the users are automatically covered.\n\n"},{"id":"393409","messageId":"20200318192821.43808-1-jonathantanmy@google.com","threadId":"52960","inReplyTo":"xmqqpnd9fql0.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] rebase --merge: optionally skip upstreamed commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-18T19:28:21Z","receivedAt":"2020-03-18T19:28:27Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > When rebasing against an upstream that has had many commits since the\n> > original branch was created:\n> >\n> > O -- O -- ... -- O -- O (upstream)\n> > \\\n> > -- O (my-dev-branch)\n> >\n> > it must read the contents of every novel upstream commit, in addition to\n> > the tip of the upstream and the merge base, because \"git rebase\"\n> > attempts to exclude commits that are duplicates of upstream ones. This\n> > can be a significant performance hit, especially in a partial clone,\n> > wherein a read of an object may end up being a fetch.\n> \n> OK. I presume that we do this by comparing patch IDs?\n\nYes.\n\n> Total disabling would of course is OK as a feature, especially for\n> the first cut, but I wonder if it would be a reasonable idea to use\n> some heuristic to keep the current \"filter the same change\" feature\n> as much as possible but optimize it by filtering the novel upstream\n> commits without hitting their trees and blobs (I am assuming that\n> you at least are aware of and have the commit objects on the\n> upstream side).\n> \n> The most false-negative-prone approach is just to compare the\n> of a candidate upstream commit with\n> what you have---if that author does not appear on my-dev-branch, it\n> is very unlikely that your change has been accepted upstream. Of\n> course, two people who independently discover the same solution is\n> not all that rare, so it does risk false-negative to take too little\n> clue from the commits to compare, but at least it is not worse than\n> what you are proposing here ;-) And if one of your commits on\n> my-dev-branch _might_ be identical to one of the novel upstream ones,\n> at that point, we could dig deeper to actually compute the patch ID\n> by fetching the upstream's tree.\n\nAs far as I know, the existing patch ID behavior is only based on the\npatch contents, so if there was any author name or time rewriting (or if\ntwo people independently discovered the same solution, as you wrote),\nthen the behavior would be different. Apart from that, this does sound\nlike a cheap thing to compare before comparing the diff.\n\nElijah Newren suggested and I investigated another approach of using a\nfilename-only diff as a first approximation. The relevant quotations and\nexplanations are in my email here [1].\n\n[1] https://lore.kernel.org/git/20200312180427.192096-1-jonathantanmy@google.com/\n\n> That's all totally outside the scope of this patch. It is just a\n> random thought to see if anybody wants to pursue to make the topic\n> even better, possible after it lands.\n\nOK.\n\n> > New in V2: changed parameter name, used Taylor's commit message\n> > suggestions, and used Elijah's documentation suggestions.\n> \n> Hmph, what was it called earlier? My gut reaction without much\n> thinking finds --no-skip-* a bit confusing double-negation and\n> suspect \"--[no-]detect-cherry-pick\" (which defaults to true for\n> backward compatibility) may feel more natural, but I suspect (I do\n> not recall details of the discussion on v1) it has been already\n> discussed and people found --no-skip-* is OK (in which case I won't\n> object)?\n\nIt was earlier called \"--{,no-}skip-already-present\" (with the opposite\nmeaning, and thus, --skip-already-present is the default), so the double\nnegative has always existed. \"--detect-cherry-pick\" might be a better\nidea...I'll wait to see if anybody else has an opinion.\n\n> I also wonder if --detect-cherry-pick=(yes|no|auto) may give a\n> better end-user experience, with \"auto\" meaning \"do run patch-ID\n> based filtering, but if we know it will be expensive (e.g. the\n> repository is sparsely cloned), please skip it\". That way, there\n> may appear other reasons that makes patch-ID computation expensive\n> now or in the fiture, and the users are automatically covered.\n\nIt might be better to have predictability, and for \"auto\", I don't know\nif we can have a simple and explainable set of rules as to when to use\npatch-ID-based filtering - for example, in a partial clone with no\nblobs, I would normally want no patch-ID-based filtering, but in a\npartial clone with only a blob size limit, I probably will still want\npatch-ID-based filtering.\n"},{"id":"393412","messageId":"xmqqd099fnfm.fsf@gitster.c.googlers.com","threadId":"52960","inReplyTo":"20200318192821.43808-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] rebase --merge: optionally skip upstreamed commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-18T19:55:09Z","receivedAt":"2020-03-18T19:55:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> Hmph, what was it called earlier? My gut reaction without much\n>> thinking finds --no-skip-* a bit confusing double-negation and\n>> suspect \"--[no-]detect-cherry-pick\" (which defaults to true for\n>> backward compatibility) may feel more natural, but I suspect (I do\n>> not recall details of the discussion on v1) it has been already\n>> discussed and people found --no-skip-* is OK (in which case I won't\n>> object)?\n>\n> It was earlier called \"--{,no-}skip-already-present\" (with the opposite\n> meaning, and thus, --skip-already-present is the default), so the double\n> negative has always existed. \"--detect-cherry-pick\" might be a better\n> idea...I'll wait to see if anybody else has an opinion.\n\nWhile \"--[no-]detect-cherry-pick\" is much better in avoiding double\nnegation, it is a horrible name---we do not tell the users what we\ndo after we detect cherry pick (\"--[no-]skip-cherry-pick-detection\"\ndoes not tell us, either). \n\nCompared to them, \"--[no-]skip-already-present\" is much better, even\nthough there is double negation. \n\nHow about a name along the lines of \"--[no-]keep-duplicate\", then?\n\n>> I also wonder if --detect-cherry-pick=(yes|no|auto) may give a\n>> better end-user experience, with \"auto\" meaning \"do run patch-ID\n>> based filtering, but if we know it will be expensive (e.g. the\n>> repository is sparsely cloned), please skip it\". That way, there\n>> may appear other reasons that makes patch-ID computation expensive\n>> now or in the fiture, and the users are automatically covered.\n>\n> It might be better to have predictability, and for \"auto\", I don't know\n> if we can have a simple and explainable set of rules as to when to use\n> patch-ID-based filtering - for example, in a partial clone with no\n> blobs, I would normally want no patch-ID-based filtering, but in a\n> partial clone with only a blob size limit, I probably will still want\n> patch-ID-based filtering.\n\nPerhaps. You could have something more specific than \"auto\". The\nmain point was instead of \"--[no-]$knob\", \"--$knob=(yes|no|...)\" is\nmuch easier to extend. I simply do not know if we will see need to\nextend the vocabulary in the near future (to which you guys who are\nmore interested in sparse clones would have much better insight than\nI do).\n\nThanks.\n"},{"id":"393414","messageId":"xmqq8sjxfm99.fsf@gitster.c.googlers.com","threadId":"52960","inReplyTo":"20200318173051.25875-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] rebase --merge: optionally skip upstreamed commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-18T20:20:34Z","receivedAt":"2020-03-18T20:20:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> @@ -1840,6 +1844,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t \"interactive or merge options\"));\n> \t}\n> \n> +\tif (options.skip_cherry_pick_detection && !is_interactive(&options))\n> +\t\tdie(_(\"--skip-cherry-pick-detection does not work with the 'apply' backend\"));\n> +\n\nI presume this is, as before, built directly on v2.25.0; thanks for\nkeeping the original base while iterating.\n\nJust a note to myself and those who are experimenting with the\npatch. When merged to the more recent codebase, is_interactive()\nhere will have to become is_merge().\n"},{"id":"393416","messageId":"CABPp-BGSvT9zu1xjHUPHBQ3jEktZ56O=m6VNH2v0E-RcfBN_tw@mail.gmail.com","threadId":"52960","inReplyTo":"xmqqd099fnfm.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] rebase --merge: optionally skip upstreamed commits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-03-18T20:41:23Z","receivedAt":"2020-03-18T20:41:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 18, 2020 at 12:55 PM Junio C Hamano wrote:\n>\n> Jonathan Tan writes:\n>\n> >> Hmph, what was it called earlier? My gut reaction without much\n> >> thinking finds --no-skip-* a bit confusing double-negation and\n> >> suspect \"--[no-]detect-cherry-pick\" (which defaults to true for\n> >> backward compatibility) may feel more natural, but I suspect (I do\n> >> not recall details of the discussion on v1) it has been already\n> >> discussed and people found --no-skip-* is OK (in which case I won't\n> >> object)?\n> >\n> > It was earlier called \"--{,no-}skip-already-present\" (with the opposite\n> > meaning, and thus, --skip-already-present is the default), so the double\n> > negative has always existed. \"--detect-cherry-pick\" might be a better\n> > idea...I'll wait to see if anybody else has an opinion.\n>\n> While \"--[no-]detect-cherry-pick\" is much better in avoiding double\n> negation, it is a horrible name---we do not tell the users what we\n> do after we detect cherry pick (\"--[no-]skip-cherry-pick-detection\"\n> does not tell us, either).\n\nI like --[no-]detect-cherry-pick. I'm on board with using \"keep\"\ninstead of \"skip\" to avoid double negation.\n\n> Compared to them, \"--[no-]skip-already-present\" is much better, even\n> though there is double negation.\n\nThis one seems especially bad from a discoverability and\nunderstandability viewpoint though. It's certainly nice if options\nare fully self-documenting, but sometimes that would require full\nparagraphs for the option name. Focusing on what is done with the\noption at the expense of discovering which options are relevant to\nyour case or at the expense of enabling users to create a mental model\nfor when options might be meaningful is something that I think is very\ndetrimental to usability. I think users who want such an option would\nhave a very hard time finding this based on its name, and people who\nwant completely unrelated features would be confused enough by it that\nthey feel compelled to read its description and attempt to parse it\nand guess how it's related. In contrast, --[no-]detect-cherry-pick is\na bit clearer to both groups of people for whether it is useful, and\nthe group who wants it can read up the description to get the details.\n\n> How about a name along the lines of \"--[no-]keep-duplicate\", then?\n\nThis name is much better than --[no-]keep-already-present would be\nbecause \"duplicate\" is a far better indicator than \"already-present\"\nof the intended meaning. But I'm still worried the name \"duplicate\"\nisn't going to be enough of a clue to individuals about whether they\nwill need this options or not. Perhaps --[no-]keep-cherry-pick?\n\n> >> I also wonder if --detect-cherry-pick=(yes|no|auto) may give a\n> >> better end-user experience, with \"auto\" meaning \"do run patch-ID\n> >> based filtering, but if we know it will be expensive (e.g. the\n> >> repository is sparsely cloned), please skip it\". That way, there\n> >> may appear other reasons that makes patch-ID computation expensive\n> >> now or in the fiture, and the users are automatically covered.\n> >\n> > It might be better to have predictability, and for \"auto\", I don't know\n> > if we can have a simple and explainable set of rules as to when to use\n> > patch-ID-based filtering - for example, in a partial clone with no\n> > blobs, I would normally want no patch-ID-based filtering, but in a\n> > partial clone with only a blob size limit, I probably will still want\n> > patch-ID-based filtering.\n>\n> Perhaps. You could have something more specific than \"auto\". The\n> main point was instead of \"--[no-]$knob\", \"--$knob=(yes|no|...)\" is\n> much easier to extend. I simply do not know if we will see need to\n> extend the vocabulary in the near future (to which you guys who are\n> more interested in sparse clones would have much better insight than\n> I do).\n\nI also struggle to understand when auto would be used. But beyond\nthat, I'm still a little uneasy with where we seem to be ending up\n(even if no fault of this patch):\n\n1) Behavior has long been --keep-cherry-pick, and in various cases\nthat behavior can reduce conflicts users have to deal with.\n2) Both Junio and I independently guessed that the cherry-pick\ndetection logic is poorly performing and could be improved; Jonathan\nconfirmed with some investigative work. We've all suggested punting\nfor now, though.\n3) I think we can make the sequencer machinery fast enough that the\ncherry-pick detection is going to be the slowest part by a good margin\neven in normal cases, not just sparse clones or the cases Taylor or I\nhad in mind. So I think it's going to stick out like a sore thumb for\na lot more people (though maybe they're all happy because it's faster\noverall?).\n4) Jonathan provided some good examples of cases where the\n--keep-cherry-pick behavior isn't just slow, but leads to actually\nwrong answers (a revert followed by an un-revert).\n\nI particularly don't like the idea of something being the default when\nit can both cause wrong behavior and present a huge performance\nproblem that folks have to learn to workaround, especially when based\nonly on the tradeoff of sometimes reducing the amount of work we push\nback on the user. Maybe that's just inevitable, but does anyone have\nany words that will make me feel better about this?\n\nElijah\n"},{"id":"393427","messageId":"xmqqsgi5dygn.fsf@gitster.c.googlers.com","threadId":"52960","inReplyTo":"CABPp-BGSvT9zu1xjHUPHBQ3jEktZ56O=m6VNH2v0E-RcfBN_tw@mail.gmail.com","subject":"Re: [PATCH v2] rebase --merge: optionally skip upstreamed commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-18T23:39:52Z","receivedAt":"2020-03-18T23:40:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> 4) Jonathan provided some good examples of cases where the\n> --keep-cherry-pick behavior isn't just slow, but leads to actually\n> wrong answers (a revert followed by an un-revert).\n\nThat one cuts both ways, doesn't it? If your change that upstream\nonce thought was good (and got accepted) turned out to be bad and\nthey reverted, you do not want to blindly reapply it to break the\ncodebase again, and with the \"drop duplicate\" logic, it would lead\nto a wrong answer silently.\n\nSo from correctness point of view, I do not think you can make any\nargument either way.\n\n"},{"id":"393430","messageId":"CABPp-BHM7_+QcpnQe1H4RGz7MmvS07r2Ks+aVzV61DGwYAPvAg@mail.gmail.com","threadId":"52960","inReplyTo":"xmqqsgi5dygn.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] rebase --merge: optionally skip upstreamed commits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-03-19T00:17:23Z","receivedAt":"2020-03-19T00:17:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 18, 2020 at 4:39 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > 4) Jonathan provided some good examples of cases where the\n> > --keep-cherry-pick behavior isn't just slow, but leads to actually\n> > wrong answers (a revert followed by an un-revert).\n>\n> That one cuts both ways, doesn't it? If your change that upstream\n> once thought was good (and got accepted) turned out to be bad and\n> they reverted, you do not want to blindly reapply it to break the\n> codebase again, and with the \"drop duplicate\" logic, it would lead\n> to a wrong answer silently.\n>\n> So from correctness point of view, I do not think you can make any\n> argument either way.\n\nGood point. Thanks, that helps.\n"},{"id":"394105","messageId":"20200326175033.35944-1-jonathantanmy@google.com","threadId":"52960","inReplyTo":"20200318173051.25875-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] rebase --merge: optionally skip upstreamed commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-26T17:50:33Z","receivedAt":"2020-03-26T17:50:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> New in V2: changed parameter name, used Taylor's commit message\n> suggestions, and used Elijah's documentation suggestions.\n\nI think the discussion has shifted away from whether this functionality\nis desirable (or desirable and we should implement this functionality\nwithout any CLI option) to the name and nature of the CLI option. Before\nI send out a new version, what do you think of using this name and\ndocumenting it this way:\n\n --keep-cherry-pick=(always|never)::\n Control rebase's behavior towards commits in the working\n branch that are already present upstream, i.e. cherry-picks.\n +\n If 'never', these commits will be dropped. Because this necessitates\n reading all upstream commits, this can be expensive in repos with a\n large number of upstream commits that need to be read.\n +\n If 'always', all commits (including these) will be re-applied. This\n allows rebase to forgo reading all upstream commits, potentially \n improving performance.\n +\n The default is 'never'.\n +\n See also INCOMPATIBLE OPTIONS below.\n\nI've tried to use everyone's suggestions: Junio's suggestions to use the\n\"keep\" name (instead of \"detect\", so that we also communicate what we do\nwith the result of our detection) and the non-boolean option (for\nextensibility later if we need it), and Elijah's suggestion to use\n\"cherry-pick\" instead of \"duplicate\". If this sounds good, I'll update\nthe patch and send out a new version.\n"},{"id":"394107","messageId":"CABPp-BHdacsWrD3Q1avCCNbt4MhwN5Tm_rY8HKHotJADJdPYxw@mail.gmail.com","threadId":"52960","inReplyTo":"20200326175033.35944-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] rebase --merge: optionally skip upstreamed commits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-03-26T19:17:32Z","receivedAt":"2020-03-26T19:17:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Mar 26, 2020 at 10:50 AM Jonathan Tan wrote:\n>\n> > New in V2: changed parameter name, used Taylor's commit message\n> > suggestions, and used Elijah's documentation suggestions.\n>\n> I think the discussion has shifted away from whether this functionality\n> is desirable (or desirable and we should implement this functionality\n> without any CLI option) to the name and nature of the CLI option. Before\n> I send out a new version, what do you think of using this name and\n> documenting it this way:\n>\n> --keep-cherry-pick=(always|never)::\n> Control rebase's behavior towards commits in the working\n> branch that are already present upstream, i.e. cherry-picks.\n> +\n> If 'never', these commits will be dropped. Because this necessitates\n> reading all upstream commits, this can be expensive in repos with a\n> large number of upstream commits that need to be read.\n> +\n> If 'always', all commits (including these) will be re-applied. This\n> allows rebase to forgo reading all upstream commits, potentially\n> improving performance.\n> +\n> The default is 'never'.\n> +\n> See also INCOMPATIBLE OPTIONS below.\n>\n> I've tried to use everyone's suggestions: Junio's suggestions to use the\n> \"keep\" name (instead of \"detect\", so that we also communicate what we do\n> with the result of our detection) and the non-boolean option (for\n> extensibility later if we need it), and Elijah's suggestion to use\n> \"cherry-pick\" instead of \"duplicate\". If this sounds good, I'll update\n> the patch and send out a new version.\n\nSounds good to me. Thanks!\n"},{"id":"394108","messageId":"xmqq4kuakjcn.fsf@gitster.c.googlers.com","threadId":"52960","inReplyTo":"20200326175033.35944-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] rebase --merge: optionally skip upstreamed commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-26T19:27:04Z","receivedAt":"2020-03-26T19:27:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> New in V2: changed parameter name, used Taylor's commit message\n>> suggestions, and used Elijah's documentation suggestions.\n>\n> I think the discussion has shifted away from whether this functionality\n> is desirable (or desirable and we should implement this functionality\n> without any CLI option) to the name and nature of the CLI option. Before\n> I send out a new version, what do you think of using this name and\n> documenting it this way:\n>\n> --keep-cherry-pick=(always|never)::\n> ...\n> The default is 'never'.\n> +\n> See also INCOMPATIBLE OPTIONS below.\n\nSounds much better to me. I do not mind --[no-]keep-cherry-pick,\neither, by the way. I know I raised the possibility of having to\nmake it non-bool later, but since then I haven't thought of a good\nthird option myself anyway, so...\n\nThanks for keeping the ball rolling.\n\n"},{"id":"394323","messageId":"20200330040621.13701-1-jonathantanmy@google.com","threadId":"52960","inReplyTo":"20200309205523.121319-1-jonathantanmy@google.com","subject":"[PATCH v3] rebase --merge: optionally skip upstreamed commits","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-30T04:06:21Z","receivedAt":"2020-03-30T04:06:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When rebasing against an upstream that has had many commits since the\noriginal branch was created:\n\n O -- O -- ... -- O -- O (upstream)\n \\\n -- O (my-dev-branch)\n\nit must read the contents of every novel upstream commit, in addition to\nthe tip of the upstream and the merge base, because \"git rebase\"\nattempts to exclude commits that are duplicates of upstream ones. This\ncan be a significant performance hit, especially in a partial clone,\nwherein a read of an object may end up being a fetch.\n\nAdd a flag to \"git rebase\" to allow suppression of this feature. This\nflag only works when using the \"merge\" backend.\n\nThis flag changes the behavior of sequencer_make_script(), called from\ndo_interactive_rebase() <- run_rebase_interactive() <-\nrun_specific_rebase() <- cmd_rebase(). With this flag, limit_list()\n(indirectly called from sequencer_make_script() through\nprepare_revision_walk()) will no longer call cherry_pick_list(), and\nthus PATCHSAME is no longer set. Refraining from setting PATCHSAME both\nmeans that the intermediate commits in upstream are no longer read (as\nshown by the test) and means that no PATCHSAME-caused skipping of\ncommits is done by sequencer_make_script(), either directly or through\nmake_script_with_merges().\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\nThis commit contains Junio's sign-off because I based it on\njt/rebase-allow-duplicate.\n\nThis does not include the fix by Đoàn Trần Công Danh. If we want all\ncommits to pass all tests (whether run by Busybox or not) it seems like\nwe should squash that patch instead of having it as a separate commit.\nIf we do squash, maybe include a \"Helped-by\" with Đoàn Trần Công Danh's\nname.\n\nJunio wrote [1]:\n\n> Sounds much better to me. I do not mind --[no-]keep-cherry-pick,\n> either, by the way. I know I raised the possibility of having to\n> make it non-bool later, but since then I haven't thought of a good\n> third option myself anyway, so...\n\nIn that case, I think it's better to stick to bool. This also means that\nthe change from the version in jt/rebase-allow-duplicate is very small,\nhopefully aiding reviewers - mostly a replacement of\n--skip-cherry-pick-detection with --keep-cherry-pick (which mean the\nsame thing).\n\n[1] https://lore.kernel.org/git/xmqq4kuakjcn.fsf@gitster.c.googlers.com/\n---\n Documentation/git-rebase.txt | 21 +++++++++-\n builtin/rebase.c | 7 ++++\n sequencer.c | 3 +-\n sequencer.h | 2 +-\n t/t3402-rebase-merge.sh | 77 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 107 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 0c4f038dd6..f4f8afeb9a 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -318,6 +318,21 @@ See also INCOMPATIBLE OPTIONS below.\n +\n See also INCOMPATIBLE OPTIONS below.\n \n+--keep-cherry-pick::\n+--no-keep-cherry-pick::\n+\tControl rebase's behavior towards commits in the working\n+\tbranch that are already present upstream, i.e. cherry-picks.\n++\n+By default, these commits will be dropped. Because this necessitates\n+reading all upstream commits, this can be expensive in repos with a\n+large number of upstream commits that need to be read.\n++\n+If `--keep-cherry-pick is given`, all commits (including these) will be\n+re-applied. This allows rebase to forgo reading all upstream commits,\n+potentially improving performance.\n++\n+See also INCOMPATIBLE OPTIONS below.\n+\n --rerere-autoupdate::\n --no-rerere-autoupdate::\n \tAllow the rerere mechanism to update the index with the\n@@ -568,6 +583,9 @@ In addition, the following pairs of options are incompatible:\n * --keep-base and --onto\n * --keep-base and --root\n \n+Also, the --keep-cherry-pick option requires the use of the merge backend\n+(e.g., through --merge).\n+\n BEHAVIORAL DIFFERENCES\n -----------------------\n \n@@ -866,7 +884,8 @@ Only works if the changes (patch IDs based on the diff contents) on\n 'subsystem' did.\n \n In that case, the fix is easy because 'git rebase' knows to skip\n-changes that are already present in the new upstream. So if you say\n+changes that are already present in the new upstream (unless\n+`--keep-cherry-pick` is given). So if you say\n (assuming you're on 'topic')\n ------------\n $ git rebase subsystem\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 8081741f8a..626549b0b2 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -88,6 +88,7 @@ struct rebase_options {\n \tstruct strbuf git_format_patch_opt;\n \tint reschedule_failed_exec;\n \tint use_legacy_rebase;\n+\tint keep_cherry_pick;\n };\n \n #define REBASE_OPTIONS_INIT {\t\t\t \t\\\n@@ -381,6 +382,7 @@ static int run_rebase_interactive(struct rebase_options *opts,\n \tflags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;\n \tflags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;\n \tflags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;\n+\tflags |= opts->keep_cherry_pick ? TODO_LIST_KEEP_CHERRY_PICK : 0;\n \n \tswitch (command) {\n \tcase ACTION_NONE: {\n@@ -1515,6 +1517,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"reschedule-failed-exec\",\n \t\t\t &reschedule_failed_exec,\n \t\t\t N_(\"automatically re-schedule any `exec` that fails\")),\n+\t\tOPT_BOOL(0, \"keep-cherry-pick\", &options.keep_cherry_pick,\n+\t\t\t N_(\"apply all changes, even those already present upstream\")),\n \t\tOPT_END(),\n \t};\n \tint i;\n@@ -1848,6 +1852,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t \"interactive or merge options\"));\n \t}\n \n+\tif (options.keep_cherry_pick && !is_interactive(&options))\n+\t\tdie(_(\"--keep-cherry-pick does not work with the 'apply' backend\"));\n+\n \tif (options.signoff) {\n \t\tif (options.type == REBASE_PRESERVE_MERGES)\n \t\t\tdie(\"cannot combine '--signoff' with \"\ndiff --git a/sequencer.c b/sequencer.c\nindex b9dbf1adb0..7bbb63f444 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4800,12 +4800,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \tint keep_empty = flags & TODO_LIST_KEEP_EMPTY;\n \tconst char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? \"p\" : \"pick\";\n \tint rebase_merges = flags & TODO_LIST_REBASE_MERGES;\n+\tint keep_cherry_pick = flags & TODO_LIST_KEEP_CHERRY_PICK;\n \n \trepo_init_revisions(r, &revs, NULL);\n \trevs.verbose_header = 1;\n \tif (!rebase_merges)\n \t\trevs.max_parents = 1;\n-\trevs.cherry_mark = 1;\n+\trevs.cherry_mark = !keep_cherry_pick;\n \trevs.limited = 1;\n \trevs.reverse = 1;\n \trevs.right_only = 1;\ndiff --git a/sequencer.h b/sequencer.h\nindex 9f9ae291e3..298b7de1c8 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -148,7 +148,7 @@ int sequencer_remove_state(struct replay_opts *opts);\n * `--onto`, we do not want to re-generate the root commits.\n */\n #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)\n-\n+#define TODO_LIST_KEEP_CHERRY_PICK (1U << 7)\n \n int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \t\t\t const char **argv, unsigned flags);\ndiff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\nindex a1ec501a87..64200c5f20 100755\n--- a/t/t3402-rebase-merge.sh\n+++ b/t/t3402-rebase-merge.sh\n@@ -162,4 +162,81 @@ test_expect_success 'rebase --skip works with two conflicts in a row' '\n \tgit rebase --skip\n '\n \n+test_expect_success '--keep-cherry-pick' '\n+\tgit init repo &&\n+\n+\t# O(1-10) -- O(1-11) -- O(0-10) master\n+\t# \\\n+\t# -- O(1-11) -- O(1-12) otherbranch\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 10) >repo/file.txt &&\n+\tgit -C repo add file.txt &&\n+\tgit -C repo commit -m \"base commit\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 11) >repo/file.txt &&\n+\tgit -C repo commit -a -m \"add 11\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 0 10) >repo/file.txt &&\n+\tgit -C repo commit -a -m \"add 0 delete 11\" &&\n+\n+\tgit -C repo checkout -b otherbranch HEAD^^ &&\n+\tprintf \"Line %d\\n\" $(test_seq 1 11) >repo/file.txt &&\n+\tgit -C repo commit -a -m \"add 11 in another branch\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 12) >repo/file.txt &&\n+\tgit -C repo commit -a -m \"add 12 in another branch\" &&\n+\n+\t# Regular rebase fails, because the 1-11 commit is deduplicated\n+\ttest_must_fail git -C repo rebase --merge master 2> err &&\n+\ttest_i18ngrep \"error: could not apply.*add 12 in another branch\" err &&\n+\tgit -C repo rebase --abort &&\n+\n+\t# With --keep-cherry-pick, it works\n+\tgit -C repo rebase --merge --keep-cherry-pick master\n+'\n+\n+test_expect_success '--keep-cherry-pick refrains from reading unneeded blobs' '\n+\tgit init server &&\n+\n+\t# O(1-10) -- O(1-11) -- O(1-12) master\n+\t# \\\n+\t# -- O(0-10) otherbranch\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 10) >server/file.txt &&\n+\tgit -C server add file.txt &&\n+\tgit -C server commit -m \"merge base\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 11) >server/file.txt &&\n+\tgit -C server commit -a -m \"add 11\" &&\n+\n+\tprintf \"Line %d\\n\" $(test_seq 1 12) >server/file.txt &&\n+\tgit -C server commit -a -m \"add 12\" &&\n+\n+\tgit -C server checkout -b otherbranch HEAD^^ &&\n+\tprintf \"Line %d\\n\" $(test_seq 0 10) >server/file.txt &&\n+\tgit -C server commit -a -m \"add 0\" &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n+\tgit -C client checkout origin/master &&\n+\tgit -C client checkout origin/otherbranch &&\n+\n+\t# Sanity check to ensure that the blobs from the merge base and \"add\n+\t# 11\" are missing\n+\tgit -C client rev-list --objects --all --missing=print >missing_list &&\n+\tMERGE_BASE_BLOB=$(git -C server rev-parse master^^:file.txt) &&\n+\tADD_11_BLOB=$(git -C server rev-parse master^:file.txt) &&\n+\tgrep \"\\\\?$MERGE_BASE_BLOB\" missing_list &&\n+\tgrep \"\\\\?$ADD_11_BLOB\" missing_list &&\n+\n+\tgit -C client rebase --merge --keep-cherry-pick origin/master &&\n+\n+\t# The blob from the merge base had to be fetched, but not \"add 11\"\n+\tgit -C client rev-list --objects --all --missing=print >missing_list &&\n+\t! grep \"\\\\?$MERGE_BASE_BLOB\" missing_list &&\n+\tgrep \"\\\\?$ADD_11_BLOB\" missing_list\n+'\n+\n test_done\n-- \n2.26.0.rc2.310.g2932bb562d-goog\n\n"},{"id":"394324","messageId":"xmqqh7y6a0pd.fsf@gitster.c.googlers.com","threadId":"52960","inReplyTo":"20200330040621.13701-1-jonathantanmy@google.com","subject":"Re: [PATCH v3] rebase --merge: optionally skip upstreamed commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-30T05:09:02Z","receivedAt":"2020-03-30T05:09:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> When rebasing against an upstream that has had many commits since the\n> original branch was created:\n>\n> O -- O -- ... -- O -- O (upstream)\n> \\\n> -- O (my-dev-branch)\n>\n> it must read the contents of every novel upstream commit, in addition to\n> the tip of the upstream and the merge base, because \"git rebase\"\n> attempts to exclude commits that are duplicates of upstream ones. This\n> can be a significant performance hit, especially in a partial clone,\n> wherein a read of an object may end up being a fetch.\n>\n> Add a flag to \"git rebase\" to allow suppression of this feature. This\n> flag only works when using the \"merge\" backend.\n>\n> This flag changes the behavior of sequencer_make_script(), called from\n> do_interactive_rebase() <- run_rebase_interactive() <-\n> run_specific_rebase() <- cmd_rebase(). With this flag, limit_list()\n> (indirectly called from sequencer_make_script() through\n> prepare_revision_walk()) will no longer call cherry_pick_list(), and\n> thus PATCHSAME is no longer set. Refraining from setting PATCHSAME both\n> means that the intermediate commits in upstream are no longer read (as\n> shown by the test) and means that no PATCHSAME-caused skipping of\n> commits is done by sequencer_make_script(), either directly or through\n> make_script_with_merges().\n>\n> Signed-off-by: Jonathan Tan \n> Signed-off-by: Junio C Hamano \n> ---\n> This commit contains Junio's sign-off because I based it on\n> jt/rebase-allow-duplicate.\n>\n> This does not include the fix by Đoàn Trần Công Danh. If we want all\n> commits to pass all tests (whether run by Busybox or not) it seems like\n> we should squash that patch instead of having it as a separate commit.\n> If we do squash, maybe include a \"Helped-by\" with Đoàn Trần Công Danh's\n> name.\n\nYup, I think Đoàn already said it is fine to squash in, so please do\nthat.\n\nThanks.\n"},{"id":"394326","messageId":"20200330052229.GA786@danh.dev","threadId":"52960","inReplyTo":"20200330040621.13701-1-jonathantanmy@google.com","subject":"Re: [PATCH v3] rebase --merge: optionally skip upstreamed commits","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-30T05:22:29Z","receivedAt":"2020-03-30T05:22:34Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-03-29 21:06:21-0700, Jonathan Tan wrote:\n> When rebasing against an upstream that has had many commits since the\n> original branch was created:\n> \n> O -- O -- ... -- O -- O (upstream)\n> \\\n> -- O (my-dev-branch)\n> \n> it must read the contents of every novel upstream commit, in addition to\n> the tip of the upstream and the merge base, because \"git rebase\"\n> attempts to exclude commits that are duplicates of upstream ones. This\n> can be a significant performance hit, especially in a partial clone,\n> wherein a read of an object may end up being a fetch.\n> \n> Add a flag to \"git rebase\" to allow suppression of this feature. This\n> flag only works when using the \"merge\" backend.\n> \n> This flag changes the behavior of sequencer_make_script(), called from\n> do_interactive_rebase() <- run_rebase_interactive() <-\n> run_specific_rebase() <- cmd_rebase(). With this flag, limit_list()\n> (indirectly called from sequencer_make_script() through\n> prepare_revision_walk()) will no longer call cherry_pick_list(), and\n> thus PATCHSAME is no longer set. Refraining from setting PATCHSAME both\n> means that the intermediate commits in upstream are no longer read (as\n> shown by the test) and means that no PATCHSAME-caused skipping of\n> commits is done by sequencer_make_script(), either directly or through\n> make_script_with_merges().\n> \n> Signed-off-by: Jonathan Tan \n> Signed-off-by: Junio C Hamano \n> ---\n> This commit contains Junio's sign-off because I based it on\n> jt/rebase-allow-duplicate.\n> \n> This does not include the fix by Đoàn Trần Công Danh. If we want all\n> commits to pass all tests (whether run by Busybox or not) it seems like\n> we should squash that patch instead of having it as a separate commit.\n> If we do squash, maybe include a \"Helped-by\" with Đoàn Trần Công Danh's\n> name.\n\nHi Jonathan,\n\nFeel free to squash it in.\n\n\n-- \nDanh\n"},{"id":"394337","messageId":"e917f451-c00a-c819-7f78-888ba6e8f5ea@gmail.com","threadId":"52960","inReplyTo":"20200330040621.13701-1-jonathantanmy@google.com","subject":"Re: [PATCH v3] rebase --merge: optionally skip upstreamed commits","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-30T12:13:32Z","receivedAt":"2020-03-30T12:13:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/30/2020 12:06 AM, Jonathan Tan wrote:\n> When rebasing against an upstream that has had many commits since the\n> original branch was created:\n> \n> O -- O -- ... -- O -- O (upstream)\n> \\\n> -- O (my-dev-branch)\n> \n> it must read the contents of every novel upstream commit, in addition to\n> the tip of the upstream and the merge base, because \"git rebase\"\n> attempts to exclude commits that are duplicates of upstream ones. This\n> can be a significant performance hit, especially in a partial clone,\n> wherein a read of an object may end up being a fetch.\n> \n> Add a flag to \"git rebase\" to allow suppression of this feature. This\n> flag only works when using the \"merge\" backend.\n\nSo this is the behavior that already exists, and you are providing a way\nto suppress it. However, you also change the default in this patch, which\nmay surprise users expecting this behavior to continue.\n\n> This flag changes the behavior of sequencer_make_script(), called from\n> do_interactive_rebase() <- run_rebase_interactive() <-\n> run_specific_rebase() <- cmd_rebase(). With this flag, limit_list()\n> (indirectly called from sequencer_make_script() through\n> prepare_revision_walk()) will no longer call cherry_pick_list(), and\n> thus PATCHSAME is no longer set. Refraining from setting PATCHSAME both\n> means that the intermediate commits in upstream are no longer read (as\n> shown by the test) and means that no PATCHSAME-caused skipping of\n> commits is done by sequencer_make_script(), either directly or through\n> make_script_with_merges().\n> \n> Signed-off-by: Jonathan Tan \n> Signed-off-by: Junio C Hamano \n> ---\n> This commit contains Junio's sign-off because I based it on\n> jt/rebase-allow-duplicate.\n> \n> This does not include the fix by Đoàn Trần Công Danh. If we want all\n> commits to pass all tests (whether run by Busybox or not) it seems like\n> we should squash that patch instead of having it as a separate commit.\n> If we do squash, maybe include a \"Helped-by\" with Đoàn Trần Công Danh's\n> name.\n> \n> Junio wrote [1]:\n> \n>> Sounds much better to me. I do not mind --[no-]keep-cherry-pick,\n>> either, by the way. I know I raised the possibility of having to\n>> make it non-bool later, but since then I haven't thought of a good\n>> third option myself anyway, so...\n> \n> In that case, I think it's better to stick to bool. This also means that\n> the change from the version in jt/rebase-allow-duplicate is very small,\n> hopefully aiding reviewers - mostly a replacement of\n> --skip-cherry-pick-detection with --keep-cherry-pick (which mean the\n> same thing).\n> \n> [1] https://lore.kernel.org/git/xmqq4kuakjcn.fsf@gitster.c.googlers.com/\n> ---\n> Documentation/git-rebase.txt | 21 +++++++++-\n> builtin/rebase.c | 7 ++++\n> sequencer.c | 3 +-\n> sequencer.h | 2 +-\n> t/t3402-rebase-merge.sh | 77 ++++++++++++++++++++++++++++++++++++\n> 5 files changed, 107 insertions(+), 3 deletions(-)\n> \n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 0c4f038dd6..f4f8afeb9a 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -318,6 +318,21 @@ See also INCOMPATIBLE OPTIONS below.\n> +\n> See also INCOMPATIBLE OPTIONS below.\n> \n> +--keep-cherry-pick::\n> +--no-keep-cherry-pick::\n\nI noticed that this _could_ have been simplified to\n\n\t--[no-]keep-cherry-pick::\n\nbut I also see several uses of either in our documentation. Do we\nhave a preference? By inspecting the lines before a \"no-\" string,\nI see that some have these two lines, some use the [no-] pattern,\nand others highlight the --no-