Re: [PATCH] fast-export: fix surprising behavior with --first-parent
- From
William Sprent <williams@unity3d.com>
- Date
- Nov 24, 2021, 13:05 UTC
- Message-ID
- <6c7b5e1d-77b3-8af2-78e5-3519ec6ee13d@unity3d.com>
- In-Reply-To
- <xmqqh7c25qsj.fsf@gitster.g>
On 24/11/2021 01.41, Junio C Hamano wrote:
Show 61 quoted lines
> "William Sprent via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: William Sprent <williams@unity3d.com> >> >> 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 <options>" supposed to do > differently from "git fast-export <options>" (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 [<git-rev-list-args>...] 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.