From: William Sprent Date: Wed, 24 Nov 2021 13:05:04 GMT Subject: Re: [PATCH] fast-export: fix surprising behavior with --first-parent Message-ID: <6c7b5e1d-77b3-8af2-78e5-3519ec6ee13d@unity3d.com> In-Reply-To: On 24/11/2021 01.41, Junio C Hamano wrote: > "William Sprent via GitGitGadget" writes: > >> From: William Sprent >> >> When invoking git-fast-export with the --first-parent flag on a branch >> with merges, fast-export would early-out on processing the first merge >> on the branch. If combined with --reverse, fast-export would instead >> output all single parent commits on the branch. > > I do not doubt we would want to make the command behave sensibly > with all options it accepts, but let me first ask a more basic and > possibly stupid question. > > What is "git fast-export --first-parent " supposed to do > differently from "git fast-export " (with the same set of > options other than "--first-parent")? Should it omit merge commits > altogether, pretending that the first single-parent ancestor it > finds on the first parent chain is a direct parent of a > single-parent descendant, e.g. if the real history were with two > single-parente commits A and B, with two merges M and N, on the > mainline, making the resulting commits into a single strand of two > pearls, with A and B before and after the rewrite to have the same > tree objects? > > ---A---M---N---B ---A---B > / / ==> > X Y > > Or should it pretend merge commits have only their first parent as > their parents, i.e. > > ---A---M---N---B ---A---M---N---B > / / ==> > X Y > > "git fast-export --help" does not even mention "--first-parent" and > pretend that any and all [...] are valid requests > to make to the command, but I am wondering if that is what we intend > to support in the first place. In builtin/fast-export.c, I do not > see any attempt to do anything differently when "--first-parent" is > requested. Perhaps we shouldn't be even taking "--first-parent" as > an option to begin with. > The "--reverse" feels even more iffy. Are we reversing the history > with such an export, i.e. pretending that parents are children and > merges are forks? > > ---A---M---N---B B---N---M---A--- > / / ==> \ \ > X Y X Y > > Or are we supposed to produce the same history in the end, just > spewing older commits first in the output stream? I am not sure > what purpose such a request serves---the "fast-import" downstream > would need the same set of objects before it can create each commit > anyway, so I am not sure what the point of giving "--reverse" is. > > If there is no sensible interpretation for some of the options that > are valid in rev-list in the context of "fast-export" command, should > we just error out when we parse the command line, instead of producing > nonsense output stream, I wonder. > I agree with the concerns. I just skimmed the list of flags that git rev-list take, and I'm pretty sure that there are both flags that don't make sense at all in the context of fast-export, and that there are flags where it is unclear what the behavior of fast-export would be when passed. However, I do think that having git fast-export support history limiting is useful. And I also think that the workflow of crafting a git rev-list command (perhaps using --graph) which outputs the part of history you want, and then applying it to git fast-export is fairly straight forward. But I also agree that "git fast-export --reverse" is nonsense. I've thought about this a bit, and I wonder if having "git fast-export" accept revisions on stdin in a similar format as "git rev-list --parents" outputs would be an API that would be flexible enough, but without the oddities of allowing all rev-list flags. Or maybe there should be a list of acceptable rev-list flags which fast-export should accept. I don't really know. For the specific question of what "--first-parent" should output, my thinking is that I would expect "git fast-export --first-parent" to output the same set of commits as "git rev-list --first-parent", which would be latter of your examples, i.e. ---A---M---N---B Similarly, I guess if the user wanted ---A---B then they could pass "--no-merges" as well, which would leave out the merge commits. With regards to this patch in particular, we now overrides any "--reverse" flag that the user passes, so I can make "git fast-export --reverse" cause an error while I'm at it, if that is desired behavior.