> Hi Jonathan, > > This patch makes good sense to me. I left a few notes below, but they > are relatively minor, and this seems to be all in a good direction. > > As a (somewhat) interesting aside, this feature would be useful to me > outside of partial clones, since I often have this workflow in my local > development wherein 'git rebase' spends quite a bit of time comparing > patches on my branch to everything new upstream. Thanks for your review, and it's great to know of a use case that this helps. > This sentence is a little confusing if you skip over the graph, since it > reads: "When rebasing against an because ... because ...". It may be > clearer if you swap the order of the last two clauses to instead be: > > 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. Sounds good; will do. > > +--skip-already-present:: > > +--no-skip-already-present:: > > + Skip commits that are already present in the new upstream. > > + This is the default. > > I believe that you mean '--skip-already-present' is the default, here, > but the placement makes it ambiguous, since it is in a paragraph with a > header that contains both the positive and negated version of this flag. > > Maybe this could changed to: s/This/--skip-already-present/'. Will do. > > In that case, the fix is easy because 'git rebase' knows to skip > > -changes that are already present in the new upstream. So if you say > > +changes that are already present in the new upstream (unless > > +`--no-skip-already-present` is given). So if you say > > Extremely minor nit: there is a whitespace change on this line where the > original has two spaces between the '.' and 'So', and the new version > has only one. OK - I'll change it to 2 spaces. > > diff --git a/sequencer.h b/sequencer.h > > index 393571e89a..39bb12f624 100644 > > --- a/sequencer.h > > +++ b/sequencer.h > > @@ -149,7 +149,7 @@ int sequencer_remove_state(struct replay_opts *opts); > > * `--onto`, we do not want to re-generate the root commits. > > */ > > #define TODO_LIST_ROOT_WITH_ONTO (1U << 6) > > - > > +#define TODO_LIST_SKIP_ALREADY_PRESENT (1U << 7) > > This was another spot that I thought could maybe be turned into an enum, > but it's clearly not the fault of your patch, and could easily be turned > into #leftoverbits. There was a recent discussion on the list [1] about whether bitsets should be enums, and we decided against it. But anyway we can revisit this later if need be. [1] https://lore.kernel.org/git/20191016193750.258148-1-jonathantanmy@xxxxxxxxxx/ The changes Taylor suggested were minor, so I'll hold off sending another version until there are more substantial changes requested.
