From: Jonathan Tan <jonathantanmy@google.com>
To: stolee@gmail.com
Cc: jonathantanmy@google.com, git@vger.kernel.org,
congdanhqx@gmail.com, newren@gmail.com, gitster@pobox.com
Subject: Re: [PATCH v3] rebase --merge: optionally skip upstreamed commits
Date: Mon, 30 Mar 2020 09:57:05 -0700 [thread overview]
Message-ID: <20200330165705.134447-1-jonathantanmy@google.com> (raw)
In-Reply-To: <e917f451-c00a-c819-7f78-888ba6e8f5ea@gmail.com>
> > Add a flag to "git rebase" to allow suppression of this feature. This
> > flag only works when using the "merge" backend.
>
> So this is the behavior that already exists, and you are providing a way
> to suppress it. However, you also change the default in this patch, which
> may surprise users expecting this behavior to continue.
First of all, thanks for looking at this.
I'm not changing the default - was there anything in the commit message
that made you believe it? If yes, I could change it.
Looking back to the title, maybe it should be "rebase --merge: make
skipping of upstreamed commits optional" (although that would exceed 50
characters, so I would have to think of a way to shorten it).
> > +--keep-cherry-pick::
> > +--no-keep-cherry-pick::
>
> I noticed that this _could_ have been simplified to
>
> --[no-]keep-cherry-pick::
>
> but I also see several uses of either in our documentation. Do we
> have a preference? By inspecting the lines before a "no-" string,
> I see that some have these two lines, some use the [no-] pattern,
> and others highlight the --no-<option> flag completely separately.
I was following the existing options in this file.
> > + Control rebase's behavior towards commits in the working
> > + branch that are already present upstream, i.e. cherry-picks.
>
> I think the "already present upstream" is misleading. We don't rebase
> things that are _reachable_ already, but this is probably better as
>
> Specify if rebase should include commits in the working branch
> that have diffs equivalent to other commits upstream. For example,
> a cherry-picked commit has an equivalent diff.
OK, I'll use this.
> > +By default, these commits will be dropped. Because this necessitates
> > +reading all upstream commits, this can be expensive in repos with a
> > +large number of upstream commits that need to be read.
>
> Now I'm confused. Are they dropped by default? Which option does what?
> --keep-cherry-pick makes me think that cherry-picked commits will come
> along for the rebase, so we will not check for them. But you have documented
> that --no-keep-cherry-pick is the default.
This part is followed by "If --keep-cherry-pick is given", so I thought
it would be clear that this is the "--no-keep-cherry-pick" part (or if
nothing is given), but I'll s/By default/By default, or if
--no-keep-cherry-pick is given/.
> (Also, I keep writing "--[no-]keep-cherry-picks" (plural) because that
> seems more natural to me. Then I go back and fix it when I notice.)
OK, let's see if others have opinions on this. Admittedly,
--keep-cherry-picks with the "s" at the end now sounds more natural to
me.
> > +If `--keep-cherry-pick is given`, all commits (including these) will be
>
> Bad tick marks: "`--keep-cherry-pick` is given"
Thanks.
> > +re-applied. This allows rebase to forgo reading all upstream commits,
> > +potentially improving performance.
>
> This reasoning is good. Could you also introduce a config option to make
> --keep-cherry-pick the default? I would like to enable that option by
> default in Scalar, but could also see partial clones wanting to enable that
> by default, too.
Maybe this could be done in another patch. This sounds like a good idea.
> > +See also INCOMPATIBLE OPTIONS below.
> > +
>
> Could we just say that his only applies with the --merge option? Why require
> the jump to the end of the options section? (After writing this, I go look
> at the rest of the doc file and see this is a common pattern.)
Yes, I'm following the pattern.
> > +Also, the --keep-cherry-pick option requires the use of the merge backend
> > +(e.g., through --merge).
> > +
>
> Will the command _fail_ if someone says --keep-cherry-pick without the merge
> backend, or just have no effect? Also, specify the option with ticks and
>
> `--[no-]keep-cherry-pick`
>
> It seems that none of the options in this section are back-ticked, which I think
> is a doc bug.
It will fail. I'll figure out how to add a test for that (which might be
difficult since the default merge backend is changing).
I'll add the ticks. (The "no-" is fine with either backend, since it
just invokes the current behavior.)
> > @@ -381,6 +382,7 @@ static int run_rebase_interactive(struct rebase_options *opts,
> > flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
> > flags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;
> > flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
> > + flags |= opts->keep_cherry_pick ? TODO_LIST_KEEP_CHERRY_PICK : 0;
>
> Since opts->keep_cherry_pick is initialized as zero, did you change the default
> behavior?
I did not change it - keep_cherry_pick being 0 means that we do not keep
any cherry-picks, so we have to read every upstream commit in order to
know which are cherry-picks (which is the current behavior).
> Do we not have a test that verifies this behavior when using the merge
> backend an no "--keep-cherry-pick" option?
Yes, there are existing tests that already check the deduplicating
behavior of "git rebase --merge".
> > + if (options.keep_cherry_pick && !is_interactive(&options))
> > + die(_("--keep-cherry-pick does not work with the 'apply' backend"));
> > +
>
> I see you are failing here. Is this the right decision?
>
> The apply backend will "keep" cherry-picks because it will not look for them upstream.
> If anything, shouldn't it be that "--no-keep-cherry-pick" is incompatible?
I haven't delved deeply into the "apply" backend, but it seems to me
that it still performs some sort of cherry-pick detection (that is, it
does not keep cherry-picks, thus --no-keep-cherry-pick). In this patch,
I have a test with the lines:
# Regular rebase fails, because the 1-11 commit is deduplicated
test_must_fail git -C repo rebase --merge master 2> err &&
When I remove "--merge" from this line, the rebase still fails (with a
different error message, so indeed another backend is used).
> > +test_expect_success '--keep-cherry-pick' '
> > + git init repo &&
> > +
> > + # O(1-10) -- O(1-11) -- O(0-10) master
> > + # \
> > + # -- O(1-11) -- O(1-12) otherbranch
> > +
> > + printf "Line %d\n" $(test_seq 1 10) >repo/file.txt &&
> > + git -C repo add file.txt &&
> > + git -C repo commit -m "base commit" &&
> > +
> > + printf "Line %d\n" $(test_seq 1 11) >repo/file.txt &&
> > + git -C repo commit -a -m "add 11" &&
> > +
> > + printf "Line %d\n" $(test_seq 0 10) >repo/file.txt &&
> > + git -C repo commit -a -m "add 0 delete 11" &&
> > +
> > + git -C repo checkout -b otherbranch HEAD^^ &&
> > + printf "Line %d\n" $(test_seq 1 11) >repo/file.txt &&
> > + git -C repo commit -a -m "add 11 in another branch" &&
> > +
> > + printf "Line %d\n" $(test_seq 1 12) >repo/file.txt &&
> > + git -C repo commit -a -m "add 12 in another branch" &&
> > +
> > + # Regular rebase fails, because the 1-11 commit is deduplicated
> > + test_must_fail git -C repo rebase --merge master 2> err &&
> > + test_i18ngrep "error: could not apply.*add 12 in another branch" err &&
> > + git -C repo rebase --abort &&
>
> OK. So here you are demonstrating that the --no-keep-cherry-pick is the
> new default. Just trying to be sure that this was intended.
Yes, --no-keep-cherry-pick is the default, but it has the same behavior
as if the flag was omitted. (The existing tests that test the
cherry-pick deduplication behavior all still work.)
next prev parent reply other threads:[~2020-03-30 16:57 UTC|newest]
Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-09 20:55 [PATCH] rebase --merge: optionally skip upstreamed commits Jonathan Tan
2020-03-10 2:10 ` Taylor Blau
2020-03-10 15:51 ` Jonathan Tan
2020-03-10 12:17 ` Johannes Schindelin
2020-03-10 16:00 ` Jonathan Tan
2020-03-10 18:56 ` Elijah Newren
2020-03-10 22:56 ` Jonathan Tan
2020-03-12 18:04 ` Jonathan Tan
2020-03-12 22:40 ` Elijah Newren
2020-03-14 8:04 ` Elijah Newren
2020-03-17 3:03 ` Jonathan Tan
2020-03-18 17:30 ` [PATCH v2] " Jonathan Tan
2020-03-18 18:47 ` Junio C Hamano
2020-03-18 19:28 ` Jonathan Tan
2020-03-18 19:55 ` Junio C Hamano
2020-03-18 20:41 ` Elijah Newren
2020-03-18 23:39 ` Junio C Hamano
2020-03-19 0:17 ` Elijah Newren
2020-03-18 20:20 ` Junio C Hamano
2020-03-26 17:50 ` Jonathan Tan
2020-03-26 19:17 ` Elijah Newren
2020-03-26 19:27 ` Junio C Hamano
2020-03-29 10:12 ` [PATCH v2 4/4] t3402: use POSIX compliant regex(7) Đoàn Trần Công Danh
2020-03-30 4:06 ` [PATCH v3] rebase --merge: optionally skip upstreamed commits Jonathan Tan
2020-03-30 5:09 ` Junio C Hamano
2020-03-30 5:22 ` Danh Doan
2020-03-30 12:13 ` Derrick Stolee
2020-03-30 16:49 ` Junio C Hamano
2020-03-30 16:57 ` Jonathan Tan [this message]
2020-03-31 11:55 ` Derrick Stolee
2020-03-31 16:27 ` Elijah Newren
2020-03-31 18:34 ` Junio C Hamano
2020-03-31 18:43 ` Junio C Hamano
2020-04-10 22:27 ` Jonathan Tan
2020-04-11 0:06 ` Elijah Newren
2020-04-11 1:11 ` Jonathan Tan
2020-04-11 2:46 ` Elijah Newren
-- strict thread matches above, loose matches on Subject: below --
2020-03-26 7:35 [PATCH 0/3] add travis job for linux with musl libc Đoàn Trần Công Danh
2020-03-26 7:35 ` [PATCH 1/3] ci: libify logic for usage and checking CI_USER Đoàn Trần Công Danh
2020-03-26 7:35 ` [PATCH 2/3] ci: refactor docker runner script Đoàn Trần Công Danh
2020-03-26 16:06 ` Eric Sunshine
2020-03-28 17:53 ` SZEDER Gábor
2020-03-29 6:36 ` Danh Doan
2020-03-26 7:35 ` [PATCH 3/3] travis: build and test on Linux with musl libc and busybox Đoàn Trần Công Danh
2020-03-29 5:49 ` [PATCH 0/3] add travis job for linux with musl libc Junio C Hamano
2020-03-29 10:12 ` [PATCH v2 0/4] Travis + Azure jobs " Đoàn Trần Công Danh
2020-03-29 10:12 ` [PATCH v2 1/4] ci: libify logic for usage and checking CI_USER Đoàn Trần Công Danh
2020-03-29 10:12 ` [PATCH v2 2/4] ci: refactor docker runner script Đoàn Trần Công Danh
2020-04-01 21:51 ` SZEDER Gábor
2020-03-29 10:12 ` [PATCH v2 3/4] travis: build and test on Linux with musl libc and busybox Đoàn Trần Công Danh
2020-04-01 22:18 ` SZEDER Gábor
2020-04-02 1:42 ` Danh Doan
2020-04-07 14:53 ` Johannes Schindelin
2020-04-07 21:35 ` Junio C Hamano
2020-04-10 13:38 ` Johannes Schindelin
2020-03-29 16:23 ` [PATCH v2 0/4] Travis + Azure jobs for linux with musl libc Junio C Hamano
2020-04-02 13:03 ` [PATCH v3 0/6] " Đoàn Trần Công Danh
2020-04-02 13:04 ` [PATCH v3 1/6] ci: make MAKEFLAGS available inside the Docker container in the Linux32 job Đoàn Trần Công Danh
2020-04-02 13:04 ` [PATCH v3 2/6] ci/lib-docker: preserve required environment variables Đoàn Trần Công Danh
2020-04-03 8:22 ` SZEDER Gábor
2020-04-03 10:09 ` Danh Doan
2020-04-03 19:55 ` SZEDER Gábor
2020-04-02 13:04 ` [PATCH v3 3/6] ci/linux32: parameterise command to switch arch Đoàn Trần Công Danh
2020-04-02 13:04 ` [PATCH v3 4/6] ci: refactor docker runner script Đoàn Trần Công Danh
2020-04-02 13:04 ` [PATCH v3 5/6] ci/linux32: libify install-dependencies step Đoàn Trần Công Danh
2020-04-02 13:04 ` [PATCH v3 6/6] travis: build and test on Linux with musl libc and busybox Đoàn Trần Công Danh
2020-04-02 17:53 ` [PATCH v3 0/6] Travis + Azure jobs for linux with musl libc Junio C Hamano
2020-04-03 0:23 ` Danh Doan
2020-04-04 1:08 ` [PATCH v4 0/6] Travis " Đoàn Trần Công Danh
2020-04-04 1:08 ` [PATCH v4 1/6] ci: make MAKEFLAGS available inside the Docker container in the Linux32 job Đoàn Trần Công Danh
2020-04-04 1:08 ` [PATCH v4 2/6] ci/lib-docker: preserve required environment variables Đoàn Trần Công Danh
2020-04-04 1:08 ` [PATCH v4 3/6] ci/linux32: parameterise command to switch arch Đoàn Trần Công Danh
2020-04-04 1:08 ` [PATCH v4 4/6] ci: refactor docker runner script Đoàn Trần Công Danh
2020-04-04 1:08 ` [PATCH v4 5/6] ci/linux32: libify install-dependencies step Đoàn Trần Công Danh
2020-04-04 1:08 ` [PATCH v4 6/6] travis: build and test on Linux with musl libc and busybox Đoàn Trần Công Danh
2020-04-05 20:39 ` [PATCH v4 0/6] Travis jobs for linux with musl libc Junio C Hamano
2020-04-07 14:55 ` Johannes Schindelin
2020-04-07 19:25 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200330165705.134447-1-jonathantanmy@google.com \
--to=jonathantanmy@google.com \
--cc=congdanhqx@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=newren@gmail.com \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).