Re: [PATCH v3] rebase --merge: optionally skip upstreamed commits

From: Derrick Stolee <stolee@gmail.com>
Date: Mar 31, 2020, 11:55 UTC
Message-ID: <f6f93f52-cb7a-f2cb-aa21-51b98044b654@gmail.com>
In-Reply-To: <20200330165705.134447-1-jonathantanmy@google.com>

On 3/30/2020 12:57 PM, Jonathan Tan wrote:

Show 11 quoted lines

>>> 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.

It's not your fault. My confusion is all. You make it very clear, but I got flipped around several times while reading the patch. Here is your message again:

Show 12 quoted lines

> When rebasing against an upstream that has had many commits since the
> original branch was created:
> 
>  O -- O -- ... -- O -- O (upstream)
>   \
>    -- O (my-dev-branch)
> 
> it must read the contents of every novel upstream commit, in addition to
> the tip of the upstream and the merge base, because "git rebase"
> attempts to exclude commits that are duplicates of upstream ones. This
> can be a significant performance hit, especially in a partial clone,
> wherein a read of an object may end up being a fetch.

So by default, it "attempts to exclude commits that are duplicates of upstream ones." So that's the --no-keep-cherry-pick option, which is the default.

> Add a flag to "git rebase" to allow suppression of this feature. This
> flag only works when using the "merge" backend.

Perhaps this is how I got confused. "suppression of this feature" probably got associated with the "--no-" version of the flag in my head. But that's not your fault. I'm probably biased from my experience with the --no-show-forced-updates flag in "git fetch". There, the "--no-" option disables the default behavior.

Maybe I wouldn't be as confused if the flag was reversed and called "--no-skip-cherry-picks" or something. That direction would make it more clear that this is a performance optimization with a possible behavior side-effect. I doubt users will be lining up to "keep cherry-picks." There is a reason we remove them by default, but it is also atypical for the check to actually change the outcome.

But if we have a config option to change the default (as a follow-up) then all of my complaints are reduced, because users will not need to think about this very often.

Show 10 quoted lines

> This flag changes the behavior of sequencer_make_script(), called from
> do_interactive_rebase() <- run_rebase_interactive() <-
> run_specific_rebase() <- cmd_rebase(). With this flag, limit_list()
> (indirectly called from sequencer_make_script() through
> prepare_revision_walk()) will no longer call cherry_pick_list(), and
> thus PATCHSAME is no longer set. Refraining from setting PATCHSAME both
> means that the intermediate commits in upstream are no longer read (as
> shown by the test) and means that no PATCHSAME-caused skipping of
> commits is done by sequencer_make_script(), either directly or through
> make_script_with_merges().

Perhaps the only change I would recommend for the commit message is to be very clear about what "this flag" means. You are talking about the "--keep-cherry-pick(s)" flag in this paragraph.

Thanks, -Stolee