On 3/30/2020 12:57 PM, Jonathan Tan wrote: >>> 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: > 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. > 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
