Re: bug? round-trip through fast-import/fast-export loses files

From: Elijah Newren <newren@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Miguel Torroja <miguel.torroja@gmail.com>,
	"Priedhorsky, Reid" <reidpr@lanl.gov>,
	"git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: bug? round-trip through fast-import/fast-export loses files
Date: Tue, 21 Mar 2023 20:07:50 -0700	[thread overview]
Message-ID: <CABPp-BGvC9wGwUYPGLXkPZH-stg9KxwFFJSCk2dXhDitOFCWoA@mail.gmail.com> (raw)
In-Reply-To: <20230321183138.GL3119834@coredump.intra.peff.net>

On Tue, Mar 21, 2023 at 11:31 AM Jeff King <peff@peff.net> 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@gmail.com/
>
> 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.