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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Mar 12, 2020, 18:04 UTC
Message-ID: <20200312180427.192096-1-jonathantanmy@google.com>
In-Reply-To: <20200310225641.96556-1-jonathantanmy@google.com>

Show 13 quoted lines

> > Does this suggest that the cherry-pick detection is suboptimal and
> > needs to be improved?  When rebasing, it is typical that you are just
> > rebasing a small number of patches compared to how many exist
> > upstream.  As such, any upstream patch modifying files outside the set
> > of files modified on the rebased side is known to not be PATCHSAME
> > without looking at those new files.
> 
> That's true - and this would drastically reduce the fetches necessary in
> partial clone, perhaps enough that we no longer need this check.
> 
> In the absence of partial clone, this also might improve performance
> sufficiently, such that we no longer need my new option. (Or it might
> not.)

I took a further look at this. patch-ids.c and its caller (cherry_pick_list() in revision.c) implement duplicate checking by first generating full diff outputs for the commits in the shorter side, putting them in a hashmap keyed by the SHA-1 of the diff output (and values being the commit itself), and then generating full diff outputs for the commits in the longer side and checking them against the hashmap. When processing the shorter side, we could also generate filename-only diffs and put their hashes into a hashset; so when processing the longer side, we could generate the filename-only diff first (without reading any blobs) and checking them against our new hashset, and only if it appears in our new hashset, then do we generate the full diff (thus reading blobs).

One issue with this is unpredictability to the user (since which blobs get read depend on which side is longer), but that seems resolvable by not doing any length checks but always reading the blobs on the right side (that is, the non-upstream side).

So I would say that yes, the cherry-pick detection is suboptimal and could be improved. So the question is...what to do with my patch? An argument could be made that my patch should be dropped because an improvement in cherry-pick detection would eliminate the need for the option I'm introducing in my patch, but after some thought, I think that this option will still be useful even with cherry-pick detection. If we move in a direction where not only blobs but also trees (or even commits) are omitted, we'll definitely want this new option. And even if a user is not using partial clone at all, I think it is still useful to suppress both the filtering of commits (e.g. when upstream has a commit then revert, it would be reasonable to cherry-pick the same commit on top) and reduce disk reads (although I don't know if this will be the case in practice).