On Tue, Mar 21, 2023 at 11:31 AM Jeff King <peff@xxxxxxxx> wrote: > > On Mon, Mar 20, 2023 at 06:57:21PM -0700, Elijah Newren wrote: > > > Honestly, looking back at those two patches of mine, I think both were > > rather suboptimal. A better solution that would handle both F->D and > > D->F would be having fast-export sort the diff_filepairs such that it > > processes the deletes before the modifies. Another improved solution > > would be having fast-import sort the files given to it and handling > > deletes first. Either should fix this. > > > > Might be a good task for a new contributor. Any takers? (Tagging as > > #leftoverbits.) > > There was a patch a while ago, but it didn't get applied: > > https://lore.kernel.org/git/1493079137-1838-1-git-send-email-miguel.torroja@xxxxxxxxx/ > > It got hung up on the fact that fast-export can also generate renames, > and ordering there is tricky. I stand by the sentiment from back then > that it is still worth it to order things to make the no-rename case > work, even if there are still corner cases with renames (since you can > have a cycle of renames, you should not use them if you want to be > robust against this kind of ordering dependency). Ah, thanks for the heads up. Stinkin' renames. ;-) I tend to forget those in the fast-export/fast-import context since fast-export doesn't detect renames by default (and e.g. filter-repo would turn them off if they were on by default). So, I think we probably need to have the fix in fast-import. Currently, parse_new_commit() calls the various file_change_*() functions immediately, which in turn immediately call the various tree_content_*() functions (e.g. file_change_cr() for a rename will call both tree_content_remove() and tree_content_set()). Instead of immediately handling all these, I think that queueing all the file_change_*() calls up, splitting the renames into delete + modify, and sorting all the deletes first should fix things for both F->D and D->F, even when cycles of renames are present as per your example in that thread.
