{"thread":{"id":"56964","subject":"[PATCH] fast-export: fix surprising behavior with --first-parent","startedAt":"2021-11-23T11:28:52Z","lastAt":"2021-12-22T08:38:21Z","messageCount":18,"participants":["William Sprent via GitGitGadget","Ævar Arnfjörð Bjarmason","Elijah Newren","Junio C Hamano","William Sprent"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"442108","messageId":"pull.1084.git.1637666927224.gitgitgadget@gmail.com","threadId":"56964","inReplyTo":null,"subject":"[PATCH] fast-export: fix surprising behavior with --first-parent","fromName":"William Sprent via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-23T11:28:47Z","receivedAt":"2021-11-23T11:28:52Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"From: William Sprent \n\nWhen invoking git-fast-export with the --first-parent flag on a branch\nwith merges, fast-export would early-out on processing the first merge\non the branch. If combined with --reverse, fast-export would instead\noutput all single parent commits on the branch.\n\nThis commit makes fast-export output the same commits as rev-list\n--first-parent, and makes --reverse not have an effect on which commits\nare output.\n\nThe fix involves removing logic within fast-export which was responsible\nfor ensuring that parents are processed before their children, which was\nwhat was exiting early due to missing second parents. This is replaced\nby setting 'reverse = 1' before revision walking, which, in conjuction\nwith topo_order, allows for delegating the ordering of commits to\nrevision.c. The reverse flag is set after parsing rev-list arguments to\navoid having it disabled.\n\nSigned-off-by: William Sprent \n---\n fast-export: fix surprising behavior with --first-parent\n \n Hi,\n \n This is my first time patching git, so I probably need some guidance on\n my approach. :)\n \n I've noticed that git fast-export exhibits some odd behavior when passed\n the --first-parent flag. In the repository I was working on, it would\n only output the initial commit before exiting. Moreover, adding the\n --reverse flag causes it to behave differently and instead output all\n commits in the first parent line that only have one parent. My\n expectation is more or less that git fast-export should output the same\n set of commits as git rev-list would output given the same arguments,\n which matches how it acts when given revision ranges.\n \n It seems like this behavior comes from the fact that has_unshown_parents\n will never be false for any merge commits encountered when fast-export\n is called with --first-parent. This causes the while loop to follow the\n pattern of pushing all commits into the \"commits\" queue until the\n initial commit is encountered, at which point it calls handle_tail which\n falls through on the first merge commit, causing most of the commits to\n be unhandled.\n \n My impression is that this logic only serves to ensure that parents are\n processed before their children, so in my patch I've opted to fix the\n issue by delegating that responsibility to revision.c by adding the\n reverse flag before performing the revision walk. From what I can see,\n this should be equivalent to what the previous logic is trying to\n achieve, but I can also see that there could be risk in these changes.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1084%2Fwilliams-unity%2Ffast-export%2Ffirst-parent-issues-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1084/williams-unity/fast-export/first-parent-issues-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1084\n\n builtin/fast-export.c | 36 ++----------------------------------\n t/t9350-fast-export.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 8e2caf72819..50f8c224b6e 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -107,18 +107,6 @@ static int parse_opt_reencode_mode(const struct option *opt,\n \n static struct decoration idnums;\n static uint32_t last_idnum;\n-\n-static int has_unshown_parent(struct commit *commit)\n-{\n-\tstruct commit_list *parent;\n-\n-\tfor (parent = commit->parents; parent; parent = parent->next)\n-\t\tif (!(parent->item->object.flags & SHOWN) &&\n-\t\t !(parent->item->object.flags & UNINTERESTING))\n-\t\t\treturn 1;\n-\treturn 0;\n-}\n-\n struct anonymized_entry {\n \tstruct hashmap_entry hash;\n \tconst char *anon;\n@@ -752,20 +740,6 @@ static char *anonymize_tag(void *data)\n \treturn strbuf_detach(&out, NULL);\n }\n \n-static void handle_tail(struct object_array *commits, struct rev_info *revs,\n-\t\t\tstruct string_list *paths_of_changed_objects)\n-{\n-\tstruct commit *commit;\n-\twhile (commits->nr) {\n-\t\tcommit = (struct commit *)object_array_pop(commits);\n-\t\tif (has_unshown_parent(commit)) {\n-\t\t\t/* Queue again, to be handled later */\n-\t\t\tadd_object_array(&commit->object, NULL, commits);\n-\t\t\treturn;\n-\t\t}\n-\t\thandle_commit(commit, revs, paths_of_changed_objects);\n-\t}\n-}\n \n static void handle_tag(const char *name, struct tag *tag)\n {\n@@ -1185,7 +1159,6 @@ static int parse_opt_anonymize_map(const struct option *opt,\n int cmd_fast_export(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n-\tstruct object_array commits = OBJECT_ARRAY_INIT;\n \tstruct commit *commit;\n \tchar *export_filename = NULL,\n \t *import_filename = NULL,\n@@ -1281,19 +1254,14 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \n \tget_tags_and_duplicates(&revs.cmdline);\n \n+\trevs.reverse = 1;\n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n \trevs.diffopt.format_callback = show_filemodify;\n \trevs.diffopt.format_callback_data = &paths_of_changed_objects;\n \trevs.diffopt.flags.recursive = 1;\n \twhile ((commit = get_revision(&revs))) {\n-\t\tif (has_unshown_parent(commit)) {\n-\t\t\tadd_object_array(&commit->object, NULL, &commits);\n-\t\t}\n-\t\telse {\n-\t\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n-\t\t\thandle_tail(&commits, &revs, &paths_of_changed_objects);\n-\t\t}\n+\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n \t}\n \n \thandle_tags_and_duplicates(&extra_refs);\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 409b48e2442..bd08101b81d 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -750,4 +750,34 @@ test_expect_success 'merge commit gets exported with --import-marks' '\n \t)\n '\n \n+\n+test_expect_success 'fast-export --first-parent outputs all revisions output by revision walk' '\n+\tgit init first-parent &&\n+\tcd first-parent &&\n+\ttest_commit init &&\n+\tgit checkout -b topic1 &&\n+\ttest_commit file2 file2.txt &&\n+\tgit checkout main &&\n+\tgit merge topic1 --no-ff &&\n+\n+\tgit checkout -b topic2 &&\n+\ttest_commit file3 file3.txt &&\n+\tgit checkout main &&\n+\tgit merge topic2 --no-ff &&\n+\n+\ttest_commit branch-head &&\n+\n+\tgit rev-list --format=\"%ad%B\" --first-parent --topo-order --no-commit-header main > expected &&\n+\n+\tgit fast-export main -- --first-parent > first-parent-export &&\n+\tgit fast-export main -- --first-parent --reverse > first-parent-reverse-export &&\n+\tgit init import && cd import &&\n+\tcat ../first-parent-export | git fast-import &&\n+\n+\tgit rev-list --format=\"%ad%B\" --topo-order --all --no-commit-header > actual &&\n+\ttest $(git rev-list --all | wc -l) -eq 4 &&\n+\ttest_cmp ../expected actual &&\n+\ttest_cmp ../first-parent-export ../first-parent-reverse-export\n+'\n+\n test_done\n\nbase-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc\n-- \ngitgitgadget\n"},{"id":"442149","messageId":"211123.865ysjui34.gmgdl@evledraar.gmail.com","threadId":"56964","inReplyTo":"pull.1084.git.1637666927224.gitgitgadget@gmail.com","subject":"Re: [PATCH] fast-export: fix surprising behavior with --first-parent","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-23T13:07:26Z","receivedAt":"2021-11-23T13:17:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 23 2021, William Sprent via GitGitGadget wrote:\n\n> From: William Sprent \n>\n> When invoking git-fast-export with the --first-parent flag on a branch\n> with merges, fast-export would early-out on processing the first merge\n> on the branch. If combined with --reverse, fast-export would instead\n> output all single parent commits on the branch.\n>\n> This commit makes fast-export output the same commits as rev-list\n> --first-parent, and makes --reverse not have an effect on which commits\n> are output.\n>\n> The fix involves removing logic within fast-export which was responsible\n> for ensuring that parents are processed before their children, which was\n> what was exiting early due to missing second parents. This is replaced\n> by setting 'reverse = 1' before revision walking, which, in conjuction\n> with topo_order, allows for delegating the ordering of commits to\n> revision.c. The reverse flag is set after parsing rev-list arguments to\n> avoid having it disabled.\n>\n> Signed-off-by: William Sprent \n> ---\n> fast-export: fix surprising behavior with --first-parent\n> \n> Hi,\n> \n> This is my first time patching git, so I probably need some guidance on\n> my approach. :)\n\nHi, thanks for your first contribution to git. This is a rather shallow\nreview, a deeper one is much deserved.\n\nI notice that you're removing code in builtin/fast-export.c, presumably\nwe have code in revision.c that does the same thing. It would really\nhelp a reviewer for you to dig a bit into the relevant commit history\nand note it in the commit message.\n\nI.e. could revision.c always do this, and this was always needless\nduplication, or at time X it was needed, but as of Y revision.c learned\nto do this, and callers A, B and C were adjusted, but just not this\nmissed call D? etc.\n\n> -static int has_unshown_parent(struct commit *commit)\n> -{\n> -\tstruct commit_list *parent;\n> -\n> -\tfor (parent = commit->parents; parent; parent = parent->next)\n> -\t\tif (!(parent->item->object.flags & SHOWN) &&\n> -\t\t !(parent->item->object.flags & UNINTERESTING))\n> -\t\t\treturn 1;\n> -\treturn 0;\n> -}\n> -\n> struct anonymized_entry {\n> \tstruct hashmap_entry hash;\n> \tconst char *anon;\n> @@ -752,20 +740,6 @@ static char *anonymize_tag(void *data)\n> \treturn strbuf_detach(&out, NULL);\n> }\n> \n> -static void handle_tail(struct object_array *commits, struct rev_info *revs,\n> -\t\t\tstruct string_list *paths_of_changed_objects)\n> -{\n> -\tstruct commit *commit;\n> -\twhile (commits->nr) {\n> -\t\tcommit = (struct commit *)object_array_pop(commits);\n> -\t\tif (has_unshown_parent(commit)) {\n> -\t\t\t/* Queue again, to be handled later */\n> -\t\t\tadd_object_array(&commit->object, NULL, commits);\n> -\t\t\treturn;\n> -\t\t}\n> -\t\thandle_commit(commit, revs, paths_of_changed_objects);\n> -\t}\n> -}\n\n...\n\n> static void handle_tag(const char *name, struct tag *tag)\n> {\n> @@ -1185,7 +1159,6 @@ static int parse_opt_anonymize_map(const struct option *opt,\n> int cmd_fast_export(int argc, const char **argv, const char *prefix)\n> {\n> \tstruct rev_info revs;\n> -\tstruct object_array commits = OBJECT_ARRAY_INIT;\n> \tstruct commit *commit;\n> \tchar *export_filename = NULL,\n> \t *import_filename = NULL,\n> @@ -1281,19 +1254,14 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n> \n> \tget_tags_and_duplicates(&revs.cmdline);\n> \n> +\trevs.reverse = 1;\n\nIs the placement of revs.reverse = 1 here important, or could it go\nearlier after init_revision_sources() when we assign some other values\nir revs?\n\n> \tif (prepare_revision_walk(&revs))\n> \t\tdie(\"revision walk setup failed\");\n\nA light reading of prepare_revision_walk() suggests it could come after,\nbut maybe I'm entirely wrong.\n\n> \trevs.diffopt.format_callback = show_filemodify;\n> \trevs.diffopt.format_callback_data = &paths_of_changed_objects;\n> \trevs.diffopt.flags.recursive = 1;\n> \twhile ((commit = get_revision(&revs))) {\n> -\t\tif (has_unshown_parent(commit)) {\n> -\t\t\tadd_object_array(&commit->object, NULL, &commits);\n> -\t\t}\n> -\t\telse {\n> -\t\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n> -\t\t\thandle_tail(&commits, &revs, &paths_of_changed_objects);\n> -\t\t}\n> +\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n> \t}\n> \n\nYay code deletion, good if it works (I didn't check).\n\nSince this is just a one-statement while-loop we can also remove its\nbraces now.\n\n> +test_expect_success 'fast-export --first-parent outputs all revisions output by revision walk' '\n> +\tgit init first-parent &&\n> +\tcd first-parent &&\n\nDo any such \"cd\" in a sub-shell:\n\n\tgit init x &&\n\t(\n \t\tcd x &&\n ...\n\t)\nOtherwise the next test after you is going to run in anotherdirectory.\n\n> +\ttest_commit init &&\n> +\tgit checkout -b topic1 &&\n> +\ttest_commit file2 file2.txt &&\n> +\tgit checkout main &&\n> +\tgit merge topic1 --no-ff &&\n> +\n> +\tgit checkout -b topic2 &&\n> +\ttest_commit file3 file3.txt &&\n> +\tgit checkout main &&\n> +\tgit merge topic2 --no-ff &&\n\nJust a nit. I'd use \"test_commit A\", \"test_commit B\" etc. when the\nfilenames etc. aren't important. There's no subsequent reference here,\nso I assume they're not.\n\n> +\ttest_commit branch-head &&\n> +\n> +\tgit rev-list --format=\"%ad%B\" --first-parent --topo-order --no-commit-header main > expected &&\n\nnit; >expected, not > expected is the usual style.\n> +\n> +\tgit fast-export main -- --first-parent > first-parent-export &&\n> +\tgit fast-export main -- --first-parent --reverse > first-parent-reverse-export &&\n\nditto:\n\n> +\tgit init import && cd import &&\n\nditto earlier \"cd\" comment.\n\n> +\tcat ../first-parent-export | git fast-import &&\n\nInstead of \"cat x | prog\" do \"prog +\tgit rev-list --format=\"%ad%B\" --topo-order --all --no-commit-header > actual &&\n\n> +\ttest $(git rev-list --all | wc -l) -eq 4 &&\n\nInstead:\n\n git rev-list --all >tmp &&\n test_line_count = 4 tmp\n\n(for some value of tmp)\n\n> +\ttest_cmp ../expected actual &&\n> +\ttest_cmp ../first-parent-export ../first-parent-reverse-export\n> +'\n\nMaybe some of the CD-ing around here wouldu be easier by not doing that\nand instead running e.g.:\n\n git -C subdir fast-export >file-not-in-subdir &&\n ...\n"},{"id":"442191","messageId":"CABPp-BGm0VtU=z0r0oGc48J020Meh_xdibnagNeDmBGQ-RVu+g@mail.gmail.com","threadId":"56964","inReplyTo":"pull.1084.git.1637666927224.gitgitgadget@gmail.com","subject":"Re: [PATCH] fast-export: fix surprising behavior with --first-parent","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-11-23T19:14:26Z","receivedAt":"2021-11-23T19:14:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Nov 23, 2021 at 5:25 AM William Sprent via GitGitGadget\n wrote:\n>\n> From: William Sprent \n>\n> When invoking git-fast-export with the --first-parent flag on a branch\n> with merges, fast-export would early-out on processing the first merge\n> on the branch. If combined with --reverse, fast-export would instead\n> output all single parent commits on the branch.\n>\n> This commit makes fast-export output the same commits as rev-list\n> --first-parent, and makes --reverse not have an effect on which commits\n> are output.\n>\n> The fix involves removing logic within fast-export which was responsible\n> for ensuring that parents are processed before their children, which was\n> what was exiting early due to missing second parents. This is replaced\n> by setting 'reverse = 1' before revision walking, which, in conjuction\n> with topo_order, allows for delegating the ordering of commits to\n> revision.c. The reverse flag is set after parsing rev-list arguments to\n> avoid having it disabled.\n>\n> Signed-off-by: William Sprent \n> ---\n> fast-export: fix surprising behavior with --first-parent\n>\n> Hi,\n>\n> This is my first time patching git, so I probably need some guidance on\n> my approach. :)\n>\n> I've noticed that git fast-export exhibits some odd behavior when passed\n> the --first-parent flag. In the repository I was working on, it would\n> only output the initial commit before exiting. Moreover, adding the\n> --reverse flag causes it to behave differently and instead output all\n> commits in the first parent line that only have one parent. My\n> expectation is more or less that git fast-export should output the same\n> set of commits as git rev-list would output given the same arguments,\n> which matches how it acts when given revision ranges.\n>\n> It seems like this behavior comes from the fact that has_unshown_parents\n> will never be false for any merge commits encountered when fast-export\n> is called with --first-parent. This causes the while loop to follow the\n> pattern of pushing all commits into the \"commits\" queue until the\n> initial commit is encountered, at which point it calls handle_tail which\n> falls through on the first merge commit, causing most of the commits to\n> be unhandled.\n>\n> My impression is that this logic only serves to ensure that parents are\n> processed before their children, so in my patch I've opted to fix the\n> issue by delegating that responsibility to revision.c by adding the\n> reverse flag before performing the revision walk. From what I can see,\n> this should be equivalent to what the previous logic is trying to\n> achieve, but I can also see that there could be risk in these changes.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1084%2Fwilliams-unity%2Ffast-export%2Ffirst-parent-issues-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1084/williams-unity/fast-export/first-parent-issues-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1084\n>\n> builtin/fast-export.c | 36 ++----------------------------------\n> t/t9350-fast-export.sh | 30 ++++++++++++++++++++++++++++++\n> 2 files changed, 32 insertions(+), 34 deletions(-)\n>\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 8e2caf72819..50f8c224b6e 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -107,18 +107,6 @@ static int parse_opt_reencode_mode(const struct option *opt,\n>\n> static struct decoration idnums;\n> static uint32_t last_idnum;\n> -\n> -static int has_unshown_parent(struct commit *commit)\n> -{\n> - struct commit_list *parent;\n> -\n> - for (parent = commit->parents; parent; parent = parent->next)\n> - if (!(parent->item->object.flags & SHOWN) &&\n> - !(parent->item->object.flags & UNINTERESTING))\n> - return 1;\n> - return 0;\n> -}\n> -\n> struct anonymized_entry {\n> struct hashmap_entry hash;\n> const char *anon;\n> @@ -752,20 +740,6 @@ static char *anonymize_tag(void *data)\n> return strbuf_detach(&out, NULL);\n> }\n>\n> -static void handle_tail(struct object_array *commits, struct rev_info *revs,\n> - struct string_list *paths_of_changed_objects)\n> -{\n> - struct commit *commit;\n> - while (commits->nr) {\n> - commit = (struct commit *)object_array_pop(commits);\n> - if (has_unshown_parent(commit)) {\n> - /* Queue again, to be handled later */\n> - add_object_array(&commit->object, NULL, commits);\n> - return;\n> - }\n> - handle_commit(commit, revs, paths_of_changed_objects);\n> - }\n> -}\n>\n> static void handle_tag(const char *name, struct tag *tag)\n> {\n> @@ -1185,7 +1159,6 @@ static int parse_opt_anonymize_map(const struct option *opt,\n> int cmd_fast_export(int argc, const char **argv, const char *prefix)\n> {\n> struct rev_info revs;\n> - struct object_array commits = OBJECT_ARRAY_INIT;\n> struct commit *commit;\n> char *export_filename = NULL,\n> *import_filename = NULL,\n> @@ -1281,19 +1254,14 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n>\n> get_tags_and_duplicates(&revs.cmdline);\n>\n> + revs.reverse = 1;\n> if (prepare_revision_walk(&revs))\n> die(\"revision walk setup failed\");\n> revs.diffopt.format_callback = show_filemodify;\n> revs.diffopt.format_callback_data = &paths_of_changed_objects;\n> revs.diffopt.flags.recursive = 1;\n> while ((commit = get_revision(&revs))) {\n> - if (has_unshown_parent(commit)) {\n> - add_object_array(&commit->object, NULL, &commits);\n> - }\n> - else {\n> - handle_commit(commit, &revs, &paths_of_changed_objects);\n> - handle_tail(&commits, &revs, &paths_of_changed_objects);\n> - }\n> + handle_commit(commit, &revs, &paths_of_changed_objects);\n\nWow, interesting. I did some related work that I never submitted at\nhttps://github.com/newren/git/commit/08f799b4667de1c755c78e1ea1657f946b588556\n-- in that commit, I also noticed that fast-export did not seem\ncareful enough about making sure to process all commits, and came up\nwith a much different fix. However, in my case, I didn't know how to\ntrigger the problems in fast-export without some additional changes I\nwas trying to make, and since I never got those other changes working,\nI didn't think it was worth submitting my fix. My solution is more\ncomplex, in part because it was designed to handle ordering\nrequirements & recommendations other than just ancestry. However, we\ndon't currently have any additional ordering requirements (the current\ncode only considers ancestry), so your solution is much simpler. If I\ndo at some point get my other changes working, I'd need to\nre-introduce the queue and handle_tail() and my changes to these, but\nthat can always be done later if and when I get those other changes\nworking.\n\nInterestingly, Dscho added both the --reverse ability to revision.c\nand the commits object_array and the handle_tail() stuff in\nfast-export.c. Both were done in the same year, and --reverse came\nfirst. See 9c5e66e97da8 (Teach revision machinery about --reverse,\n2007-01-20) and f2dc849e9c5f (Add 'git fast-export', the sister of\n'git fast-import', 2007-12-02). It's not clear why revs.reverse = 1\nwasn't used previously, although it certainly didn't occur to me\neither and I think it would have been an alternative solution to my\nfirst ever git.git contribution -- 784f8affe4df (fast-export: ensure\nwe traverse commits in topological order, 2009-02-10). (Though\nrev.reverse = 1 wouldn't have provided the same speedups that\nrev.topo_order=1 provided.)\n\nI can't think of any cases where this would cause any problems, and it\nseems like using rev.reverse is a pretty clean solution. Just in case\nI'm missing something, though, it would be nice to get Dscho to also\ncomment on this. I'm cc'ing him.\n\n(Also, I see that Ævar has already provided some good feedback on the\ntestcase, so I'll stop here.)\n\n> }\n>\n> handle_tags_and_duplicates(&extra_refs);\n> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\n> index 409b48e2442..bd08101b81d 100755\n> --- a/t/t9350-fast-export.sh\n> +++ b/t/t9350-fast-export.sh\n> @@ -750,4 +750,34 @@ test_expect_success 'merge commit gets exported with --import-marks' '\n> )\n> '\n>\n> +\n> +test_expect_success 'fast-export --first-parent outputs all revisions output by revision walk' '\n> + git init first-parent &&\n> + cd first-parent &&\n> + test_commit init &&\n> + git checkout -b topic1 &&\n> + test_commit file2 file2.txt &&\n> + git checkout main &&\n> + git merge topic1 --no-ff &&\n> +\n> + git checkout -b topic2 &&\n> + test_commit file3 file3.txt &&\n> + git checkout main &&\n> + git merge topic2 --no-ff &&\n> +\n> + test_commit branch-head &&\n> +\n> + git rev-list --format=\"%ad%B\" --first-parent --topo-order --no-commit-header main > expected &&\n> +\n> + git fast-export main -- --first-parent > first-parent-export &&\n> + git fast-export main -- --first-parent --reverse > first-parent-reverse-export &&\n> + git init import && cd import &&\n> + cat ../first-parent-export | git fast-import &&\n> +\n> + git rev-list --format=\"%ad%B\" --topo-order --all --no-commit-header > actual &&\n> + test $(git rev-list --all | wc -l) -eq 4 &&\n> + test_cmp ../expected actual &&\n> + test_cmp ../first-parent-export ../first-parent-reverse-export\n> +'\n> +\n> test_done\n>\n> base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc\n> --\n> gitgitgadget\n"},{"id":"442216","messageId":"xmqqh7c25qsj.fsf@gitster.g","threadId":"56964","inReplyTo":"pull.1084.git.1637666927224.gitgitgadget@gmail.com","subject":"Re: [PATCH] fast-export: fix surprising behavior with --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-24T00:41:00Z","receivedAt":"2021-11-24T00:41:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"William Sprent via GitGitGadget\" writes:\n\n> From: William Sprent \n>\n> When invoking git-fast-export with the --first-parent flag on a branch\n> with merges, fast-export would early-out on processing the first merge\n> on the branch. If combined with --reverse, fast-export would instead\n> output all single parent commits on the branch.\n\nI do not doubt we would want to make the command behave sensibly\nwith all options it accepts, but let me first ask a more basic and\npossibly stupid question.\n\nWhat is \"git fast-export --first-parent \" supposed to do\ndifferently from \"git fast-export \" (with the same set of\noptions other than \"--first-parent\")? Should it omit merge commits\naltogether, pretending that the first single-parent ancestor it\nfinds on the first parent chain is a direct parent of a\nsingle-parent descendant, e.g. if the real history were with two\nsingle-parente commits A and B, with two merges M and N, on the\nmainline, making the resulting commits into a single strand of two\npearls, with A and B before and after the rewrite to have the same\ntree objects?\n\n ---A---M---N---B ---A---B\n / / ==>\n X Y\n\nOr should it pretend merge commits have only their first parent as\ntheir parents, i.e.\n\n ---A---M---N---B ---A---M---N---B\n / / ==>\n X Y\n\n\"git fast-export --help\" does not even mention \"--first-parent\" and\npretend that any and all [...] are valid requests\nto make to the command, but I am wondering if that is what we intend\nto support in the first place. In builtin/fast-export.c, I do not\nsee any attempt to do anything differently when \"--first-parent\" is\nrequested. Perhaps we shouldn't be even taking \"--first-parent\" as\nan option to begin with.\n\nThe \"--reverse\" feels even more iffy. Are we reversing the history\nwith such an export, i.e. pretending that parents are children and\nmerges are forks?\n\n ---A---M---N---B B---N---M---A---\n / / ==> \\ \\\n X Y X Y\n\nOr are we supposed to produce the same history in the end, just\nspewing older commits first in the output stream? I am not sure\nwhat purpose such a request serves---the \"fast-import\" downstream\nwould need the same set of objects before it can create each commit\nanyway, so I am not sure what the point of giving \"--reverse\" is.\n\nIf there is no sensible interpretation for some of the options that\nare valid in rev-list in the context of \"fast-export\" command, should\nwe just error out when we parse the command line, instead of producing\nnonsense output stream, I wonder.\n"},{"id":"442238","messageId":"7c2a01ec-8a69-b7af-cabb-c6a6ef7483a9@unity3d.com","threadId":"56964","inReplyTo":"211123.865ysjui34.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] fast-export: fix surprising behavior with --first-parent","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2021-11-24T11:57:03Z","receivedAt":"2021-11-24T11:57:09Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 23/11/2021 14.07, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Nov 23 2021, William Sprent via GitGitGadget wrote:\n> \n>> From: William Sprent \n>>\n>> When invoking git-fast-export with the --first-parent flag on a branch\n>> with merges, fast-export would early-out on processing the first merge\n>> on the branch. If combined with --reverse, fast-export would instead\n>> output all single parent commits on the branch.\n>>\n>> This commit makes fast-export output the same commits as rev-list\n>> --first-parent, and makes --reverse not have an effect on which commits\n>> are output.\n>>\n>> The fix involves removing logic within fast-export which was responsible\n>> for ensuring that parents are processed before their children, which was\n>> what was exiting early due to missing second parents. This is replaced\n>> by setting 'reverse = 1' before revision walking, which, in conjuction\n>> with topo_order, allows for delegating the ordering of commits to\n>> revision.c. The reverse flag is set after parsing rev-list arguments to\n>> avoid having it disabled.\n>>\n>> Signed-off-by: William Sprent \n>> ---\n>> fast-export: fix surprising behavior with --first-parent\n>> \n>> Hi,\n>> \n>> This is my first time patching git, so I probably need some guidance on\n>> my approach. :)\n> \n> Hi, thanks for your first contribution to git. This is a rather shallow\n> review, a deeper one is much deserved.\n> > I notice that you're removing code in builtin/fast-export.c, presumably\n> we have code in revision.c that does the same thing. It would really\n> help a reviewer for you to dig a bit into the relevant commit history\n> and note it in the commit message.\n> \n> I.e. could revision.c always do this, and this was always needless\n> duplication, or at time X it was needed, but as of Y revision.c learned\n> to do this, and callers A, B and C were adjusted, but just not this\n> missed call D? etc.\n> \n\nThat's a really good suggestion. I should've done that. I did dig just \nenough to see that the logic has been around since fast-export was \nintroduced, but I didn't check whether the 'reverse' option was part of \nrevision.c at that point. I see that Elijah has done that homework for \nme later in this thread and discovered that --reverse was introduce a \nyear or so before fast-export though.\n\n>> -static int has_unshown_parent(struct commit *commit)\n>> -{\n>> -\tstruct commit_list *parent;\n>> -\n>> -\tfor (parent = commit->parents; parent; parent = parent->next)\n>> -\t\tif (!(parent->item->object.flags & SHOWN) &&\n>> -\t\t !(parent->item->object.flags & UNINTERESTING))\n>> -\t\t\treturn 1;\n>> -\treturn 0;\n>> -}\n>> -\n>> struct anonymized_entry {\n>> \tstruct hashmap_entry hash;\n>> \tconst char *anon;\n>> @@ -752,20 +740,6 @@ static char *anonymize_tag(void *data)\n>> \treturn strbuf_detach(&out, NULL);\n>> }\n>> \n>> -static void handle_tail(struct object_array *commits, struct rev_info *revs,\n>> -\t\t\tstruct string_list *paths_of_changed_objects)\n>> -{\n>> -\tstruct commit *commit;\n>> -\twhile (commits->nr) {\n>> -\t\tcommit = (struct commit *)object_array_pop(commits);\n>> -\t\tif (has_unshown_parent(commit)) {\n>> -\t\t\t/* Queue again, to be handled later */\n>> -\t\t\tadd_object_array(&commit->object, NULL, commits);\n>> -\t\t\treturn;\n>> -\t\t}\n>> -\t\thandle_commit(commit, revs, paths_of_changed_objects);\n>> -\t}\n>> -}\n> \n> ...\n> \n>> static void handle_tag(const char *name, struct tag *tag)\n>> {\n>> @@ -1185,7 +1159,6 @@ static int parse_opt_anonymize_map(const struct option *opt,\n>> int cmd_fast_export(int argc, const char **argv, const char *prefix)\n>> {\n>> \tstruct rev_info revs;\n>> -\tstruct object_array commits = OBJECT_ARRAY_INIT;\n>> \tstruct commit *commit;\n>> \tchar *export_filename = NULL,\n>> \t *import_filename = NULL,\n>> @@ -1281,19 +1254,14 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n>> \n>> \tget_tags_and_duplicates(&revs.cmdline);\n>> \n>> +\trevs.reverse = 1;\n> \n> Is the placement of revs.reverse = 1 here important, or could it go\n> earlier after init_revision_sources() when we assign some other values\n> ir revs?\n> \n>> \tif (prepare_revision_walk(&revs))\n>> \t\tdie(\"revision walk setup failed\");\n> \n> A light reading of prepare_revision_walk() suggests it could come after,\n> but maybe I'm entirely wrong.\n\nIt needs to go after the setup_revisions() call as otherwise a --reverse \npassed to fast-export will toggle the option off again. You are right in \nthat it can be moved down. I've done that in my working directory for \nthe next patch.\n\nAnother option would be to move the revs.reverse up as you suggest and \nthen then call die() if it was toggled off by setup_revisions(). The \nonly downside I can think of is that it would make any current usage of \n'fast-export --reverse' go from a no-op to an error.\n\n>> \trevs.diffopt.format_callback = show_filemodify;\n>> \trevs.diffopt.format_callback_data = &paths_of_changed_objects;\n>> \trevs.diffopt.flags.recursive = 1;\n>> \twhile ((commit = get_revision(&revs))) {\n>> -\t\tif (has_unshown_parent(commit)) {\n>> -\t\t\tadd_object_array(&commit->object, NULL, &commits);\n>> -\t\t}\n>> -\t\telse {\n>> -\t\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n>> -\t\t\thandle_tail(&commits, &revs, &paths_of_changed_objects);\n>> -\t\t}\n>> +\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n>> \t}\n>> \n> \n> Yay code deletion, good if it works (I didn't check).\n> \n> Since this is just a one-statement while-loop we can also remove its\n> braces now.\n> >> +test_expect_success 'fast-export --first-parent outputs all \nrevisions output by revision walk' '\n>> +\tgit init first-parent &&\n>> +\tcd first-parent &&\n> \n> Do any such \"cd\" in a sub-shell:\n> \n> \tgit init x &&\n> \t(\n> \t\tcd x &&\n> ...\n> \t)\n> Otherwise the next test after you is going to run in anotherdirectory.\n> \n>> +\ttest_commit init &&\n>> +\tgit checkout -b topic1 &&\n>> +\ttest_commit file2 file2.txt &&\n>> +\tgit checkout main &&\n>> +\tgit merge topic1 --no-ff &&\n>> +\n>> +\tgit checkout -b topic2 &&\n>> +\ttest_commit file3 file3.txt &&\n>> +\tgit checkout main &&\n>> +\tgit merge topic2 --no-ff &&\n> \n> Just a nit. I'd use \"test_commit A\", \"test_commit B\" etc. when the\n> filenames etc. aren't important. There's no subsequent reference here,\n> so I assume they're not.\n> \n>> +\ttest_commit branch-head &&\n>> +\n>> +\tgit rev-list --format=\"%ad%B\" --first-parent --topo-order --no-commit-header main > expected &&\n> \n> nit; >expected, not > expected is the usual style.\n>> +\n>> +\tgit fast-export main -- --first-parent > first-parent-export &&\n>> +\tgit fast-export main -- --first-parent --reverse > first-parent-reverse-export &&\n> \n> ditto:\n> \n>> +\tgit init import && cd import &&\n> \n> ditto earlier \"cd\" comment.\n> \n>> +\tcat ../first-parent-export | git fast-import &&\n> \n> Instead of \"cat x | prog\" do \"prog \n>> +\tgit rev-list --format=\"%ad%B\" --topo-order --all --no-commit-header > actual &&\n> \n>> +\ttest $(git rev-list --all | wc -l) -eq 4 &&\n> \n> Instead:\n> \n> git rev-list --all >tmp &&\n> test_line_count = 4 tmp\n> \n> (for some value of tmp)\n> \n>> +\ttest_cmp ../expected actual &&\n>> +\ttest_cmp ../first-parent-export ../first-parent-reverse-export\n>> +'\n> \n> Maybe some of the CD-ing around here wouldu be easier by not doing that\n> and instead running e.g.:\n> \n> git -C subdir fast-export >file-not-in-subdir &&\n> ...\n> \n\nThanks for taking the time to give your feedback. :) I'll remove those \nbraces from the while loop and incorporate your comments about the test \nfor v2.\n"},{"id":"442240","messageId":"6c7b5e1d-77b3-8af2-78e5-3519ec6ee13d@unity3d.com","threadId":"56964","inReplyTo":"xmqqh7c25qsj.fsf@gitster.g","subject":"Re: [PATCH] fast-export: fix surprising behavior with --first-parent","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2021-11-24T13:05:04Z","receivedAt":"2021-11-24T14:29:51Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 24/11/2021 01.41, Junio C Hamano wrote:\n> \"William Sprent via GitGitGadget\" writes:\n> \n>> From: William Sprent \n>>\n>> When invoking git-fast-export with the --first-parent flag on a branch\n>> with merges, fast-export would early-out on processing the first merge\n>> on the branch. If combined with --reverse, fast-export would instead\n>> output all single parent commits on the branch.\n> \n> I do not doubt we would want to make the command behave sensibly\n> with all options it accepts, but let me first ask a more basic and\n> possibly stupid question.\n> \n> What is \"git fast-export --first-parent \" supposed to do\n> differently from \"git fast-export \" (with the same set of\n> options other than \"--first-parent\")? Should it omit merge commits\n> altogether, pretending that the first single-parent ancestor it\n> finds on the first parent chain is a direct parent of a\n> single-parent descendant, e.g. if the real history were with two\n> single-parente commits A and B, with two merges M and N, on the\n> mainline, making the resulting commits into a single strand of two\n> pearls, with A and B before and after the rewrite to have the same\n> tree objects?\n> \n> ---A---M---N---B ---A---B\n> / / ==>\n> X Y\n> \n> Or should it pretend merge commits have only their first parent as\n> their parents, i.e.\n> \n> ---A---M---N---B ---A---M---N---B\n> / / ==>\n> X Y\n> \n> \"git fast-export --help\" does not even mention \"--first-parent\" and\n> pretend that any and all [...] are valid requests\n> to make to the command, but I am wondering if that is what we intend\n> to support in the first place. In builtin/fast-export.c, I do not\n> see any attempt to do anything differently when \"--first-parent\" is\n> requested. Perhaps we shouldn't be even taking \"--first-parent\" as\n> an option to begin with.\n> The \"--reverse\" feels even more iffy. Are we reversing the history\n> with such an export, i.e. pretending that parents are children and\n> merges are forks?\n> \n> ---A---M---N---B B---N---M---A---\n> / / ==> \\ \\\n> X Y X Y\n> \n> Or are we supposed to produce the same history in the end, just\n> spewing older commits first in the output stream? I am not sure\n> what purpose such a request serves---the \"fast-import\" downstream\n> would need the same set of objects before it can create each commit\n> anyway, so I am not sure what the point of giving \"--reverse\" is.\n> \n> If there is no sensible interpretation for some of the options that\n> are valid in rev-list in the context of \"fast-export\" command, should\n> we just error out when we parse the command line, instead of producing\n> nonsense output stream, I wonder.\n> \n\n\nI agree with the concerns. I just skimmed the list of flags that git \nrev-list take, and I'm pretty sure that there are both flags that don't \nmake sense at all in the context of fast-export, and that there are \nflags where it is unclear what the behavior of fast-export would be when \npassed.\n\nHowever, I do think that having git fast-export support history limiting \nis useful. And I also think that the workflow of crafting a git rev-list \ncommand (perhaps using --graph) which outputs the part of history you \nwant, and then applying it to git fast-export is fairly straight \nforward. But I also agree that \"git fast-export --reverse\" is nonsense.\n\nI've thought about this a bit, and I wonder if having \"git fast-export\" \naccept revisions on stdin in a similar format as \"git rev-list \n--parents\" outputs would be an API that would be flexible enough, but \nwithout the oddities of allowing all rev-list flags. Or maybe there \nshould be a list of acceptable rev-list flags which fast-export should \naccept. I don't really know.\n\nFor the specific question of what \"--first-parent\" should output, my \nthinking is that I would expect \"git fast-export --first-parent\" to \noutput the same set of commits as \"git rev-list --first-parent\", which \nwould be latter of your examples, i.e.\n\n ---A---M---N---B\n\nSimilarly, I guess if the user wanted\n\n ---A---B\n\nthen they could pass \"--no-merges\" as well, which would leave out the \nmerge commits.\n\nWith regards to this patch in particular, we now overrides any \n\"--reverse\" flag that the user passes, so I can make \"git fast-export \n--reverse\" cause an error while I'm at it, if that is desired behavior.\n"},{"id":"442241","messageId":"96bba3dd-589c-dc37-012a-6c827cafacb8@unity3d.com","threadId":"56964","inReplyTo":"CABPp-BGm0VtU=z0r0oGc48J020Meh_xdibnagNeDmBGQ-RVu+g@mail.gmail.com","subject":"Re: [PATCH] fast-export: fix surprising behavior with --first-parent","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2021-11-24T13:05:28Z","receivedAt":"2021-11-24T14:29:53Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 23/11/2021 20.14, Elijah Newren wrote:\n> On Tue, Nov 23, 2021 at 5:25 AM William Sprent via GitGitGadget\n> wrote:\n>>\n>> From: William Sprent \n>> ...\n> \n> Wow, interesting. I did some related work that I never submitted at\n> https://github.com/newren/git/commit/08f799b4667de1c755c78e1ea1657f946b588556\n> -- in that commit, I also noticed that fast-export did not seem\n> careful enough about making sure to process all commits, and came up\n> with a much different fix. However, in my case, I didn't know how to\n> trigger the problems in fast-export without some additional changes I\n> was trying to make, and since I never got those other changes working,\n> I didn't think it was worth submitting my fix. My solution is more\n> complex, in part because it was designed to handle ordering\n> requirements & recommendations other than just ancestry. However, we\n> don't currently have any additional ordering requirements (the current\n> code only considers ancestry), so your solution is much simpler. If I\n> do at some point get my other changes working, I'd need to\n> re-introduce the queue and handle_tail() and my changes to these, but\n> that can always be done later if and when I get those other changes\n> working.\n> \n\nAh that is interesting. Good to know. I was happy that I could make \nfast-export rely on revision.c for ordering. At surface level, with the \nminimal amount of insight I have, that seems like an alright separation \nof concerns.\n\n> Interestingly, Dscho added both the --reverse ability to revision.c\n> and the commits object_array and the handle_tail() stuff in\n> fast-export.c. Both were done in the same year, and --reverse came\n> first. See 9c5e66e97da8 (Teach revision machinery about --reverse,\n> 2007-01-20) and f2dc849e9c5f (Add 'git fast-export', the sister of\n> 'git fast-import', 2007-12-02). It's not clear why revs.reverse = 1\n> wasn't used previously, although it certainly didn't occur to me\n> either and I think it would have been an alternative solution to my\n> first ever git.git contribution -- 784f8affe4df (fast-export: ensure\n> we traverse commits in topological order, 2009-02-10). (Though\n> rev.reverse = 1 wouldn't have provided the same speedups that\n> rev.topo_order=1 provided.)\n\nAh. Thanks for the insight.\n\n> I can't think of any cases where this would cause any problems, and it\n> seems like using rev.reverse is a pretty clean solution. Just in case\n> I'm missing something, though, it would be nice to get Dscho to also\n> comment on this. I'm cc'ing him.\n\nThanks. Good idea.\n\n"},{"id":"443612","messageId":"pull.1084.v2.git.1639037637231.gitgitgadget@gmail.com","threadId":"56964","inReplyTo":"pull.1084.git.1637666927224.gitgitgadget@gmail.com","subject":"[PATCH v2] fast-export: fix surprising behavior with --first-parent","fromName":"William Sprent via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-09T08:13:57Z","receivedAt":"2021-12-09T08:14:01Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"From: William Sprent \n\nWhen invoking git-fast-export with the --first-parent flag on a branch\nwith merges, fast-export would early-out on processing the first merge\non the branch. If combined with --reverse, fast-export would instead\noutput all single parent commits on the branch.\n\nThis commit makes fast-export output the same commits as rev-list\n--first-parent, and makes --reverse not have an effect on which commits\nare output.\n\nThe fix involves removing logic within fast-export which was responsible\nfor ensuring that parents are processed before their children, which was\nwhat was exiting early due to missing second parents. This is replaced\nby setting 'reverse = 1' before revision walking, which, in conjuction\nwith topo_order, allows for delegating the ordering of commits to\nrevision.c. The reverse flag is set after parsing rev-list arguments to\navoid having it disabled.\n\nSigned-off-by: William Sprent \n---\n fast-export: fix surprising behavior with --first-parent\n \n Hi,\n \n I've noticed that git fast-export exhibits some odd behavior when passed\n the --first-parent flag. In the repository I was working on, it would\n only output the initial commit before exiting. Moreover, adding the\n --reverse flag causes it to behave differently and instead output all\n commits in the first parent line that only have one parent. My\n expectation is more or less that git fast-export should output the same\n set of commits as git rev-list would output given the same arguments,\n which matches how it acts when given revision ranges.\n \n It seems like this behavior comes from the fact that has_unshown_parents\n will never be false for any merge commits encountered when fast-export\n is called with --first-parent. This causes the while loop to follow the\n pattern of pushing all commits into the \"commits\" queue until the\n initial commit is encountered, at which point it calls handle_tail which\n falls through on the first merge commit, causing most of the commits to\n be unhandled.\n \n My impression is that this logic only serves to ensure that parents are\n processed before their children, so in my patch I've opted to fix the\n issue by delegating that responsibility to revision.c by adding the\n reverse flag before performing the revision walk. From what I can see,\n this should be equivalent to what the previous logic is trying to\n achieve, but I can also see that there could be risk in these changes.\n \n Changes since v1:\n \n * Moved revs.reverse assignment down to similar assignments below.\n * Removed braces around single statement while loop.\n * The test now only changes directory inside a sub-shell.\n * Applied stylistic feedback on test such as: making redirections be on\n the form >FILE etc.\n \n There were questions raised about whether it makes sense at all for\n fast-export to simply accept all git rev-list arguments whether they\n have an effect or not - in particular for a flag like --reverse. I think\n I agree that it is questionable behavior, or at least questionably\n documented, but I also think it is out of scope for this change.\n \n I did consider teaching fast-export to complain if given --reverse, but\n it seemed inconsistent to me as it will gladly accept every other\n rev-list argument (for example, \"git fast-export HEAD --show-signature\n --graph\" works just fine).\n \n cc: Ævar Arnfjörð Bjarmason avarab@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1084%2Fwilliams-unity%2Ffast-export%2Ffirst-parent-issues-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1084/williams-unity/fast-export/first-parent-issues-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1084\n\nRange-diff vs v1:\n\n 1: e2824193a8f ! 1: af552f37dbe fast-export: fix surprising behavior with --first-parent\n @@ builtin/fast-export.c: static int parse_opt_anonymize_map(const struct option *o\n \t *import_filename = NULL,\n @@ builtin/fast-export.c: int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \n - \tget_tags_and_duplicates(&revs.cmdline);\n - \n -+\trevs.reverse = 1;\n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n ++\n ++\trevs.reverse = 1;\n \trevs.diffopt.format_callback = show_filemodify;\n \trevs.diffopt.format_callback_data = &paths_of_changed_objects;\n \trevs.diffopt.flags.recursive = 1;\n - \twhile ((commit = get_revision(&revs))) {\n +-\twhile ((commit = get_revision(&revs))) {\n -\t\tif (has_unshown_parent(commit)) {\n -\t\t\tadd_object_array(&commit->object, NULL, &commits);\n -\t\t}\n @@ builtin/fast-export.c: int cmd_fast_export(int argc, const char **argv, const ch\n -\t\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n -\t\t\thandle_tail(&commits, &revs, &paths_of_changed_objects);\n -\t\t}\n +-\t}\n ++\twhile ((commit = get_revision(&revs)))\n +\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n - \t}\n \n \thandle_tags_and_duplicates(&extra_refs);\n + \thandle_tags_and_duplicates(&tag_refs);\n \n ## t/t9350-fast-export.sh ##\n @@ t/t9350-fast-export.sh: test_expect_success 'merge commit gets exported with --import-marks' '\n @@ t/t9350-fast-export.sh: test_expect_success 'merge commit gets exported with --i\n +\n +test_expect_success 'fast-export --first-parent outputs all revisions output by revision walk' '\n +\tgit init first-parent &&\n -+\tcd first-parent &&\n -+\ttest_commit init &&\n -+\tgit checkout -b topic1 &&\n -+\ttest_commit file2 file2.txt &&\n -+\tgit checkout main &&\n -+\tgit merge topic1 --no-ff &&\n ++\t(\n ++\t\tcd first-parent &&\n ++\t\ttest_commit A &&\n ++\t\tgit checkout -b topic1 &&\n ++\t\ttest_commit B &&\n ++\t\tgit checkout main &&\n ++\t\tgit merge topic1 --no-ff &&\n ++\n ++\t\tgit checkout -b topic2 &&\n ++\t\ttest_commit C &&\n ++\t\tgit checkout main &&\n ++\t\tgit merge topic2 --no-ff &&\n ++\n ++\t\ttest_commit D &&\n +\n -+\tgit checkout -b topic2 &&\n -+\ttest_commit file3 file3.txt &&\n -+\tgit checkout main &&\n -+\tgit merge topic2 --no-ff &&\n ++\t\tgit rev-list --format=\"%ad%B\" --first-parent --topo-order --no-commit-header main >expected &&\n +\n -+\ttest_commit branch-head &&\n ++\t\tgit fast-export main -- --first-parent >first-parent-export &&\n ++\t\tgit fast-export main -- --first-parent --reverse >first-parent-reverse-export &&\n +\n -+\tgit rev-list --format=\"%ad%B\" --first-parent --topo-order --no-commit-header main > expected &&\n ++\t\tgit init import &&\n ++\t\tgit -C import fast-import first-parent-export &&\n -+\tgit fast-export main -- --first-parent --reverse > first-parent-reverse-export &&\n -+\tgit init import && cd import &&\n -+\tcat ../first-parent-export | git fast-import &&\n ++\t\tgit -C import rev-list --format=\"%ad%B\" --topo-order --all --no-commit-header >actual &&\n ++\t\tgit -C import rev-list --all >tmp &&\n +\n -+\tgit rev-list --format=\"%ad%B\" --topo-order --all --no-commit-header > actual &&\n -+\ttest $(git rev-list --all | wc -l) -eq 4 &&\n -+\ttest_cmp ../expected actual &&\n -+\ttest_cmp ../first-parent-export ../first-parent-reverse-export\n ++\t\ttest_line_count = 4 tmp &&\n ++\t\ttest_cmp expected actual &&\n ++\t\ttest_cmp first-parent-export first-parent-reverse-export\n ++\t)\n +'\n +\n test_done\n\n\n builtin/fast-export.c | 40 ++++------------------------------------\n t/t9350-fast-export.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 39 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 8e2caf72819..cb1d6473f12 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -107,18 +107,6 @@ static int parse_opt_reencode_mode(const struct option *opt,\n \n static struct decoration idnums;\n static uint32_t last_idnum;\n-\n-static int has_unshown_parent(struct commit *commit)\n-{\n-\tstruct commit_list *parent;\n-\n-\tfor (parent = commit->parents; parent; parent = parent->next)\n-\t\tif (!(parent->item->object.flags & SHOWN) &&\n-\t\t !(parent->item->object.flags & UNINTERESTING))\n-\t\t\treturn 1;\n-\treturn 0;\n-}\n-\n struct anonymized_entry {\n \tstruct hashmap_entry hash;\n \tconst char *anon;\n@@ -752,20 +740,6 @@ static char *anonymize_tag(void *data)\n \treturn strbuf_detach(&out, NULL);\n }\n \n-static void handle_tail(struct object_array *commits, struct rev_info *revs,\n-\t\t\tstruct string_list *paths_of_changed_objects)\n-{\n-\tstruct commit *commit;\n-\twhile (commits->nr) {\n-\t\tcommit = (struct commit *)object_array_pop(commits);\n-\t\tif (has_unshown_parent(commit)) {\n-\t\t\t/* Queue again, to be handled later */\n-\t\t\tadd_object_array(&commit->object, NULL, commits);\n-\t\t\treturn;\n-\t\t}\n-\t\thandle_commit(commit, revs, paths_of_changed_objects);\n-\t}\n-}\n \n static void handle_tag(const char *name, struct tag *tag)\n {\n@@ -1185,7 +1159,6 @@ static int parse_opt_anonymize_map(const struct option *opt,\n int cmd_fast_export(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n-\tstruct object_array commits = OBJECT_ARRAY_INIT;\n \tstruct commit *commit;\n \tchar *export_filename = NULL,\n \t *import_filename = NULL,\n@@ -1283,18 +1256,13 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n+\n+\trevs.reverse = 1;\n \trevs.diffopt.format_callback = show_filemodify;\n \trevs.diffopt.format_callback_data = &paths_of_changed_objects;\n \trevs.diffopt.flags.recursive = 1;\n-\twhile ((commit = get_revision(&revs))) {\n-\t\tif (has_unshown_parent(commit)) {\n-\t\t\tadd_object_array(&commit->object, NULL, &commits);\n-\t\t}\n-\t\telse {\n-\t\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n-\t\t\thandle_tail(&commits, &revs, &paths_of_changed_objects);\n-\t\t}\n-\t}\n+\twhile ((commit = get_revision(&revs)))\n+\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n \n \thandle_tags_and_duplicates(&extra_refs);\n \thandle_tags_and_duplicates(&tag_refs);\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 409b48e2442..df1a5b1013a 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -750,4 +750,39 @@ test_expect_success 'merge commit gets exported with --import-marks' '\n \t)\n '\n \n+\n+test_expect_success 'fast-export --first-parent outputs all revisions output by revision walk' '\n+\tgit init first-parent &&\n+\t(\n+\t\tcd first-parent &&\n+\t\ttest_commit A &&\n+\t\tgit checkout -b topic1 &&\n+\t\ttest_commit B &&\n+\t\tgit checkout main &&\n+\t\tgit merge topic1 --no-ff &&\n+\n+\t\tgit checkout -b topic2 &&\n+\t\ttest_commit C &&\n+\t\tgit checkout main &&\n+\t\tgit merge topic2 --no-ff &&\n+\n+\t\ttest_commit D &&\n+\n+\t\tgit rev-list --format=\"%ad%B\" --first-parent --topo-order --no-commit-header main >expected &&\n+\n+\t\tgit fast-export main -- --first-parent >first-parent-export &&\n+\t\tgit fast-export main -- --first-parent --reverse >first-parent-reverse-export &&\n+\n+\t\tgit init import &&\n+\t\tgit -C import fast-import actual &&\n+\t\tgit -C import rev-list --all >tmp &&\n+\n+\t\ttest_line_count = 4 tmp &&\n+\t\ttest_cmp expected actual &&\n+\t\ttest_cmp first-parent-export first-parent-reverse-export\n+\t)\n+'\n+\n test_done\n\nbase-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc\n-- \ngitgitgadget\n"},{"id":"443729","messageId":"CABPp-BGdCizEGcwPS+0VB_vvYLpGCWKLqx-nbZtJ16QkVxzbGQ@mail.gmail.com","threadId":"56964","inReplyTo":"pull.1084.v2.git.1639037637231.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] fast-export: fix surprising behavior with --first-parent","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-12-10T03:48:20Z","receivedAt":"2021-12-10T03:48:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":" On Thu, Dec 9, 2021 at 12:13 AM William Sprent via GitGitGadget\n wrote:\n>\n> From: William Sprent \n>\n> When invoking git-fast-export with the --first-parent flag on a branch\n> with merges, fast-export would early-out on processing the first merge\n> on the branch. If combined with --reverse, fast-export would instead\n> output all single parent commits on the branch.\n>\n> This commit makes fast-export output the same commits as rev-list\n> --first-parent, and makes --reverse not have an effect on which commits\n> are output.\n>\n> The fix involves removing logic within fast-export which was responsible\n> for ensuring that parents are processed before their children, which was\n> what was exiting early due to missing second parents. This is replaced\n> by setting 'reverse = 1' before revision walking, which, in conjuction\n> with topo_order, allows for delegating the ordering of commits to\n> revision.c. The reverse flag is set after parsing rev-list arguments to\n> avoid having it disabled.\n\nThis explains how you discovered the issue, but seems to focus on\nthese particular flags, leaving the reader wondering whether there are\nother flags that are also mishandled that need additional care, and\nwhether this fix might break things for other sets of flags. It took\nme quite a bit of reasoning to satisfy myself on those points; it's\nactually somewhat complex. May I suggest an alternative commit\nmessage based on that? Here's what I think are the relevant points\n(and yeah, it's lengthy):\n\n\nThe revision traversal machinery typically processes and returns all\nchildren before any parent. fast-export needs to operate in the\nreverse fashion, handling parents before any of their children in\norder to build up the history starting from the root commit(s). This\nwould be a clear case where we could just use the revision traversal\nmachinery's \"reverse\" option to achieve this desired affect.\n\nHowever, this wasn't what the code did. It added its own array for\nqueuing. The obvious hand-rolled solution would be to just push all\nthe commits into the array and then traverse afterwards, but it didn't\nquite do that either. It instead attempted to process anything it\ncould as soon as it could, and once it could, check whether it could\nprocess anything that had been queued. As far as I can tell, this was\nan effort to save a little memory in the case of multiple root commits\nsince it could process some commits before queueing all of them. This\ninvolved some helper functions named has_unshown_parent() and\nhandle_tail(). For typical invocations of fast-export, this\nalternative essentially amounted to a hand-rolled method of reversing\nthe commits -- it was a bunch of work to duplicate the revision\ntraversal machinery's \"reverse\" option.\n\nThis hand-rolled reversing mechanism is actually somewhat difficult to\nreason about. It takes some time to figure out how it ensures in\nnormal cases that it will actually process all traversed commits\n(rather than just dropping some and not printing anything for them).\n\nAnd it turns out there are some cases where the code does drop commits\nwithout handling them, and not even printing an error or warning for\nthe user. Due to the has_unshown_parent() checks, some commits could\nbe left in the array at the end of the \"while...get_revision()\" loop\nwhich would be unprocessed. This could be triggered for example with\n git fast-export main -- --first-parent\nor non-sensical traversal rules such as\n git fast-export main -- --grep=Merge --invert-grep\n\nWhile most traversals that don't include all parents should likely\ntrigger errors in fast-export (or at least require being used in\ncombination with --reference-excluded-parents), the --first-parent\ntraversal is at least reasonable and it'd be nice if it didn't just\ndrop commits. It'd also be nice to have a simpler \"reverse traversal\"\nmechanism. Use the \"reverse\" option of the revision traversal\nmachinery to achieve both.\n\nEven for the non-sensical traversal flags like the --grep one above,\nthis would be an improvement. For example, in that case, the code\npreviously would have silently truncated history to only those commits\nthat do not have an ancestor containing \"Merge\" in their commit\nmessage. After this code change, that case would would include all\ncommits without \"Merge\" in their commit message -- but any commit that\npreviously had a \"Merge\"-mentioning parent would lose that parent\n(likely resulting in many new root commits). While the new behavior\nis still odd, it is at least understandable given that\n--reference-excluded-parents is not the default.\n\n\n>\n> Signed-off-by: William Sprent \n> ---\n> fast-export: fix surprising behavior with --first-parent\n>\n> Hi,\n>\n> I've noticed that git fast-export exhibits some odd behavior when passed\n> the --first-parent flag. In the repository I was working on, it would\n> only output the initial commit before exiting. Moreover, adding the\n> --reverse flag causes it to behave differently and instead output all\n> commits in the first parent line that only have one parent. My\n> expectation is more or less that git fast-export should output the same\n> set of commits as git rev-list would output given the same arguments,\n> which matches how it acts when given revision ranges.\n>\n> It seems like this behavior comes from the fact that has_unshown_parents\n> will never be false for any merge commits encountered when fast-export\n> is called with --first-parent. This causes the while loop to follow the\n> pattern of pushing all commits into the \"commits\" queue until the\n> initial commit is encountered, at which point it calls handle_tail which\n> falls through on the first merge commit, causing most of the commits to\n> be unhandled.\n>\n> My impression is that this logic only serves to ensure that parents are\n> processed before their children, so in my patch I've opted to fix the\n> issue by delegating that responsibility to revision.c by adding the\n> reverse flag before performing the revision walk. From what I can see,\n> this should be equivalent to what the previous logic is trying to\n> achieve, but I can also see that there could be risk in these changes.\n>\n> Changes since v1:\n>\n> * Moved revs.reverse assignment down to similar assignments below.\n> * Removed braces around single statement while loop.\n> * The test now only changes directory inside a sub-shell.\n> * Applied stylistic feedback on test such as: making redirections be on\n> the form >FILE etc.\n>\n> There were questions raised about whether it makes sense at all for\n> fast-export to simply accept all git rev-list arguments whether they\n> have an effect or not - in particular for a flag like --reverse. I think\n> I agree that it is questionable behavior, or at least questionably\n> documented, but I also think it is out of scope for this change.\n>\n> I did consider teaching fast-export to complain if given --reverse, but\n> it seemed inconsistent to me as it will gladly accept every other\n> rev-list argument (for example, \"git fast-export HEAD --show-signature\n> --graph\" works just fine).\n>\n> cc: Ævar Arnfjörð Bjarmason avarab@gmail.com\n>\n...\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 8e2caf72819..cb1d6473f12 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -107,18 +107,6 @@ static int parse_opt_reencode_mode(const struct option *opt,\n>\n> static struct decoration idnums;\n> static uint32_t last_idnum;\n> -\n> -static int has_unshown_parent(struct commit *commit)\n> -{\n> - struct commit_list *parent;\n> -\n> - for (parent = commit->parents; parent; parent = parent->next)\n> - if (!(parent->item->object.flags & SHOWN) &&\n> - !(parent->item->object.flags & UNINTERESTING))\n> - return 1;\n> - return 0;\n> -}\n> -\n> struct anonymized_entry {\n> struct hashmap_entry hash;\n> const char *anon;\n> @@ -752,20 +740,6 @@ static char *anonymize_tag(void *data)\n> return strbuf_detach(&out, NULL);\n> }\n>\n> -static void handle_tail(struct object_array *commits, struct rev_info *revs,\n> - struct string_list *paths_of_changed_objects)\n> -{\n> - struct commit *commit;\n> - while (commits->nr) {\n> - commit = (struct commit *)object_array_pop(commits);\n> - if (has_unshown_parent(commit)) {\n> - /* Queue again, to be handled later */\n> - add_object_array(&commit->object, NULL, commits);\n> - return;\n> - }\n> - handle_commit(commit, revs, paths_of_changed_objects);\n> - }\n> -}\n\nDeleted code is debugged code! :-)\n\n>\n> static void handle_tag(const char *name, struct tag *tag)\n> {\n> @@ -1185,7 +1159,6 @@ static int parse_opt_anonymize_map(const struct option *opt,\n> int cmd_fast_export(int argc, const char **argv, const char *prefix)\n> {\n> struct rev_info revs;\n> - struct object_array commits = OBJECT_ARRAY_INIT;\n> struct commit *commit;\n> char *export_filename = NULL,\n> *import_filename = NULL,\n> @@ -1283,18 +1256,13 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n>\n> if (prepare_revision_walk(&revs))\n> die(\"revision walk setup failed\");\n> +\n> + revs.reverse = 1;\n\nI really wanted to see this next to the\n rev.topo_order = 1\nstatement elsewhere, but as you alluded to in the commit message, this\npart of revision.c makes that unsafe:\n\n\"\"\"\n} else if (!strcmp(arg, \"--reverse\")) {\n revs->reverse ^= 1;\n\"\"\"\n\nHowever, given that it's unsafe to set revs.reverse=1 earlier, now\nthat I think about it, isn't it also unsafe to set revs.topo_order\nwhere it is? Someone could override it by passing --date-order to\nfast-export. (It's okay if you want to leave fixing that to someone\nelse, just thought I'd mention it while reviewing.)\n\n> revs.diffopt.format_callback = show_filemodify;\n> revs.diffopt.format_callback_data = &paths_of_changed_objects;\n> revs.diffopt.flags.recursive = 1;\n> - while ((commit = get_revision(&revs))) {\n> - if (has_unshown_parent(commit)) {\n> - add_object_array(&commit->object, NULL, &commits);\n> - }\n> - else {\n> - handle_commit(commit, &revs, &paths_of_changed_objects);\n> - handle_tail(&commits, &revs, &paths_of_changed_objects);\n> - }\n> - }\n> + while ((commit = get_revision(&revs)))\n> + handle_commit(commit, &revs, &paths_of_changed_objects);\n\nLooks good. Nice work on finding this.\n\n>\n> handle_tags_and_duplicates(&extra_refs);\n> handle_tags_and_duplicates(&tag_refs);\n> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\n> index 409b48e2442..df1a5b1013a 100755\n> --- a/t/t9350-fast-export.sh\n> +++ b/t/t9350-fast-export.sh\n> @@ -750,4 +750,39 @@ test_expect_success 'merge commit gets exported with --import-marks' '\n> )\n> '\n>\n> +\n> +test_expect_success 'fast-export --first-parent outputs all revisions output by revision walk' '\n> + git init first-parent &&\n> + (\n> + cd first-parent &&\n> + test_commit A &&\n> + git checkout -b topic1 &&\n> + test_commit B &&\n> + git checkout main &&\n> + git merge topic1 --no-ff &&\n> +\n> + git checkout -b topic2 &&\n> + test_commit C &&\n> + git checkout main &&\n> + git merge topic2 --no-ff &&\n\nmicro nit: I'd really prefer the --no-ff before the \"topic1\" and \"topic2\".\n\n> +\n> + test_commit D &&\n> +\n> + git rev-list --format=\"%ad%B\" --first-parent --topo-order --no-commit-header main >expected &&\n\nI'd much prefer this were changed to\n git log --format=\"%ad %s\" --first-parent >expected &&\nbecause:\n * --topo-order is irrelevant when you have a linear history (which\n--first-parent gives you)\n * --no-commit-header can be tossed by using log instead of rev-list\n * %B provides the entire (multi-line) commit message body but you\nclearly care about how many commits you saw below and were assuming\njust one line per commit, so %s is better.\n * The format looks weird when inspecting as a human; it's much nicer\nwith a space between the %ad and %s.\n\n> +\n> + git fast-export main -- --first-parent >first-parent-export &&\n> + git fast-export main -- --first-parent --reverse >first-parent-reverse-export &&\n> +\n> + git init import &&\n> + git -C import fast-import +\n> + git -C import rev-list --format=\"%ad%B\" --topo-order --all --no-commit-header >actual &&\n\nSame simplifications as above here:\n git -C import log --format=\"%ad %s\" --topo-order --all >actual &&\n\nHowever, is there a reason you're using \"--all\" instead of \"main\"?\nAlthough main is the only branch, which makes either output the same\nthing, it'd be easier for folks reading to catch the equivalence if it\nwas clear you were now listing information about the same branch.\n\n> + git -C import rev-list --all >tmp &&\n> +\n> + test_line_count = 4 tmp &&\n\nI don't understand why you need tmp. Why not just use actual on the\ntest_line_count line?\n\n> + test_cmp expected actual &&\n> + test_cmp first-parent-export first-parent-reverse-export\n\nThis last line would be much nicer to include right after\nfirst-parent-reverse-export is created. Or else move the creation of\nfirst-parent-reverse-export down to just above this line.\n\n> + )\n> +'\n> +\n> test_done\n"},{"id":"443839","messageId":"xmqqee6km8ez.fsf@gitster.g","threadId":"56964","inReplyTo":"CABPp-BGdCizEGcwPS+0VB_vvYLpGCWKLqx-nbZtJ16QkVxzbGQ@mail.gmail.com","subject":"Re: [PATCH v2] fast-export: fix surprising behavior with --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-10T21:55:48Z","receivedAt":"2021-12-10T21:55:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> ... Here's what I think are the relevant points\n> (and yeah, it's lengthy):\n>\n>\n> The revision traversal machinery typically processes and returns all\n> children before any parent. fast-export needs to operate in the\n> reverse fashion, handling parents before any of their children in\n> order to build up the history starting from the root commit(s). This\n> would be a clear case where we could just use the revision traversal\n> machinery's \"reverse\" option to achieve this desired affect.\n>\n> However, this wasn't what the code did. It added its own array for\n> queuing. The obvious hand-rolled solution would be to just push all\n> the commits into the array and then traverse afterwards, but it didn't\n> quite do that either. It instead attempted to process anything it\n> could as soon as it could, and once it could, check whether it could\n> process anything that had been queued. As far as I can tell, this was\n> an effort to save a little memory in the case of multiple root commits\n> since it could process some commits before queueing all of them. This\n> involved some helper functions named has_unshown_parent() and\n> handle_tail(). For typical invocations of fast-export, this\n> alternative essentially amounted to a hand-rolled method of reversing\n> the commits -- it was a bunch of work to duplicate the revision\n> traversal machinery's \"reverse\" option.\n>\n> This hand-rolled reversing mechanism is actually somewhat difficult to\n> reason about. It takes some time to figure out how it ensures in\n> normal cases that it will actually process all traversed commits\n> (rather than just dropping some and not printing anything for them).\n>\n> And it turns out there are some cases where the code does drop commits\n> without handling them, and not even printing an error or warning for\n> the user. Due to the has_unshown_parent() checks, some commits could\n> be left in the array at the end of the \"while...get_revision()\" loop\n> which would be unprocessed. This could be triggered for example with\n> git fast-export main -- --first-parent\n> or non-sensical traversal rules such as\n> git fast-export main -- --grep=Merge --invert-grep\n>\n> While most traversals that don't include all parents should likely\n> trigger errors in fast-export (or at least require being used in\n> combination with --reference-excluded-parents), the --first-parent\n> traversal is at least reasonable and it'd be nice if it didn't just\n> drop commits. It'd also be nice to have a simpler \"reverse traversal\"\n> mechanism. Use the \"reverse\" option of the revision traversal\n> machinery to achieve both.\n\nThe above is a very helpful and understandable explanation of what\nis going on. I am a bit puzzled by the very last part, though. By\n\"It'd also be nice to have a simpler 'reverse traversal' mechanism\",\ndo you mean that the end users have need to control the direction\nthe traversal goes (in other words, they use \"git fast-export\" for\nsome thing, and \"git fast-export --reverse\" to achieve some other\nthings)? Or do you just mean that we need to do a reverse traversal\nbut that is already available in the revision traversal machinery,\nand not using it and rolling our own does not make sense?\n\n> Even for the non-sensical traversal flags like the --grep one above,\n> this would be an improvement. For example, in that case, the code\n> previously would have silently truncated history to only those commits\n> that do not have an ancestor containing \"Merge\" in their commit\n> message. After this code change, that case would would include all\n\n\"would would\" -> \"would\"\n\n> commits without \"Merge\" in their commit message -- but any commit that\n> previously had a \"Merge\"-mentioning parent would lose that parent\n> (likely resulting in many new root commits). While the new behavior\n> is still odd, it is at least understandable given that\n> --reference-excluded-parents is not the default.\n\nNicely written.\n"},{"id":"443841","messageId":"CABPp-BEU2pbfa6CSSMe9Dw7YQCaw+uU1rNMJn1YRraHKJ5D_8g@mail.gmail.com","threadId":"56964","inReplyTo":"xmqqee6km8ez.fsf@gitster.g","subject":"Re: [PATCH v2] fast-export: fix surprising behavior with --first-parent","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-12-10T22:02:33Z","receivedAt":"2021-12-10T22:02:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Dec 10, 2021 at 1:55 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > ... Here's what I think are the relevant points\n> > (and yeah, it's lengthy):\n> >\n> >\n> > The revision traversal machinery typically processes and returns all\n> > children before any parent. fast-export needs to operate in the\n> > reverse fashion, handling parents before any of their children in\n> > order to build up the history starting from the root commit(s). This\n> > would be a clear case where we could just use the revision traversal\n> > machinery's \"reverse\" option to achieve this desired affect.\n> >\n> > However, this wasn't what the code did. It added its own array for\n> > queuing. The obvious hand-rolled solution would be to just push all\n> > the commits into the array and then traverse afterwards, but it didn't\n> > quite do that either. It instead attempted to process anything it\n> > could as soon as it could, and once it could, check whether it could\n> > process anything that had been queued. As far as I can tell, this was\n> > an effort to save a little memory in the case of multiple root commits\n> > since it could process some commits before queueing all of them. This\n> > involved some helper functions named has_unshown_parent() and\n> > handle_tail(). For typical invocations of fast-export, this\n> > alternative essentially amounted to a hand-rolled method of reversing\n> > the commits -- it was a bunch of work to duplicate the revision\n> > traversal machinery's \"reverse\" option.\n> >\n> > This hand-rolled reversing mechanism is actually somewhat difficult to\n> > reason about. It takes some time to figure out how it ensures in\n> > normal cases that it will actually process all traversed commits\n> > (rather than just dropping some and not printing anything for them).\n> >\n> > And it turns out there are some cases where the code does drop commits\n> > without handling them, and not even printing an error or warning for\n> > the user. Due to the has_unshown_parent() checks, some commits could\n> > be left in the array at the end of the \"while...get_revision()\" loop\n> > which would be unprocessed. This could be triggered for example with\n> > git fast-export main -- --first-parent\n> > or non-sensical traversal rules such as\n> > git fast-export main -- --grep=Merge --invert-grep\n> >\n> > While most traversals that don't include all parents should likely\n> > trigger errors in fast-export (or at least require being used in\n> > combination with --reference-excluded-parents), the --first-parent\n> > traversal is at least reasonable and it'd be nice if it didn't just\n> > drop commits. It'd also be nice to have a simpler \"reverse traversal\"\n> > mechanism. Use the \"reverse\" option of the revision traversal\n> > machinery to achieve both.\n>\n> The above is a very helpful and understandable explanation of what\n> is going on. I am a bit puzzled by the very last part, though. By\n> \"It'd also be nice to have a simpler 'reverse traversal' mechanism\",\n> do you mean that the end users have need to control the direction\n> the traversal goes (in other words, they use \"git fast-export\" for\n> some thing, and \"git fast-export --reverse\" to achieve some other\n> things)? Or do you just mean that we need to do a reverse traversal\n> but that is already available in the revision traversal machinery,\n> and not using it and rolling our own does not make sense?\n\nSorry, yeah, I meant the latter. I do not think end users should have\ncontrol of the direction. Perhaps if that was reworded to \"...It'd\nalso be nice for future readers of the code to have a simpler...\" it'd\nbe clearer?\n\n> > Even for the non-sensical traversal flags like the --grep one above,\n> > this would be an improvement. For example, in that case, the code\n> > previously would have silently truncated history to only those commits\n> > that do not have an ancestor containing \"Merge\" in their commit\n> > message. After this code change, that case would would include all\n>\n> \"would would\" -> \"would\"\n\nGood catch.\n\n> > commits without \"Merge\" in their commit message -- but any commit that\n> > previously had a \"Merge\"-mentioning parent would lose that parent\n> > (likely resulting in many new root commits). While the new behavior\n> > is still odd, it is at least understandable given that\n> > --reference-excluded-parents is not the default.\n>\n> Nicely written.\n\nThanks. :-)\n"},{"id":"443983","messageId":"b87ec8f9-dd0d-c7b8-1c2a-edfd3d015930@unity3d.com","threadId":"56964","inReplyTo":"CABPp-BGdCizEGcwPS+0VB_vvYLpGCWKLqx-nbZtJ16QkVxzbGQ@mail.gmail.com","subject":"Re: [PATCH v2] fast-export: fix surprising behavior with --first-parent","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2021-12-13T15:09:48Z","receivedAt":"2021-12-13T15:09:54Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 10/12/2021 04.48, Elijah Newren wrote:\n> On Thu, Dec 9, 2021 at 12:13 AM William Sprent via GitGitGadget\n> wrote:\n>>\n>> From: William Sprent \n>>\n>> When invoking git-fast-export with the --first-parent flag on a branch\n>> with merges, fast-export would early-out on processing the first merge\n>> on the branch. If combined with --reverse, fast-export would instead\n>> output all single parent commits on the branch.\n>>\n>> This commit makes fast-export output the same commits as rev-list\n>> --first-parent, and makes --reverse not have an effect on which commits\n>> are output.\n>>\n>> The fix involves removing logic within fast-export which was responsible\n>> for ensuring that parents are processed before their children, which was\n>> what was exiting early due to missing second parents. This is replaced\n>> by setting 'reverse = 1' before revision walking, which, in conjuction\n>> with topo_order, allows for delegating the ordering of commits to\n>> revision.c. The reverse flag is set after parsing rev-list arguments to\n>> avoid having it disabled.\n> \n> This explains how you discovered the issue, but seems to focus on\n> these particular flags, leaving the reader wondering whether there are\n> other flags that are also mishandled that need additional care, and\n> whether this fix might break things for other sets of flags. It took\n> me quite a bit of reasoning to satisfy myself on those points; it's\n> actually somewhat complex. May I suggest an alternative commit\n> message based on that? Here's what I think are the relevant points\n> (and yeah, it's lengthy):\n> \n> \n> The revision traversal machinery typically processes and returns all\n> children before any parent. fast-export needs to operate in the\n> reverse fashion, handling parents before any of their children in\n> order to build up the history starting from the root commit(s). This\n> would be a clear case where we could just use the revision traversal\n> machinery's \"reverse\" option to achieve this desired affect.\n> \n> However, this wasn't what the code did. It added its own array for\n> queuing. The obvious hand-rolled solution would be to just push all\n> the commits into the array and then traverse afterwards, but it didn't\n> quite do that either. It instead attempted to process anything it\n> could as soon as it could, and once it could, check whether it could\n> process anything that had been queued. As far as I can tell, this was\n> an effort to save a little memory in the case of multiple root commits\n> since it could process some commits before queueing all of them. This\n> involved some helper functions named has_unshown_parent() and\n> handle_tail(). For typical invocations of fast-export, this\n> alternative essentially amounted to a hand-rolled method of reversing\n> the commits -- it was a bunch of work to duplicate the revision\n> traversal machinery's \"reverse\" option.\n> \n> This hand-rolled reversing mechanism is actually somewhat difficult to\n> reason about. It takes some time to figure out how it ensures in\n> normal cases that it will actually process all traversed commits\n> (rather than just dropping some and not printing anything for them).\n> \n> And it turns out there are some cases where the code does drop commits\n> without handling them, and not even printing an error or warning for\n> the user. Due to the has_unshown_parent() checks, some commits could\n> be left in the array at the end of the \"while...get_revision()\" loop\n> which would be unprocessed. This could be triggered for example with\n> git fast-export main -- --first-parent\n> or non-sensical traversal rules such as\n> git fast-export main -- --grep=Merge --invert-grep\n> \n> While most traversals that don't include all parents should likely\n> trigger errors in fast-export (or at least require being used in\n> combination with --reference-excluded-parents), the --first-parent\n> traversal is at least reasonable and it'd be nice if it didn't just\n> drop commits. It'd also be nice to have a simpler \"reverse traversal\"\n> mechanism. Use the \"reverse\" option of the revision traversal\n> machinery to achieve both.\n> \n> Even for the non-sensical traversal flags like the --grep one above,\n> this would be an improvement. For example, in that case, the code\n> previously would have silently truncated history to only those commits\n> that do not have an ancestor containing \"Merge\" in their commit\n> message. After this code change, that case would would include all\n> commits without \"Merge\" in their commit message -- but any commit that\n> previously had a \"Merge\"-mentioning parent would lose that parent\n> (likely resulting in many new root commits). While the new behavior\n> is still odd, it is at least understandable given that\n> --reference-excluded-parents is not the default.\n> \n> \n\nThanks for taking the time to write such a thorough explanation. I'll\napply Junio's comments and use it as the commit message for the next\nversion.\n\n>>\n>> Signed-off-by: William Sprent \n>> ---\n>> fast-export: fix surprising behavior with --first-parent\n>>\n>> Hi,\n>>\n>> I've noticed that git fast-export exhibits some odd behavior when passed\n>> the --first-parent flag. In the repository I was working on, it would\n>> only output the initial commit before exiting. Moreover, adding the\n>> --reverse flag causes it to behave differently and instead output all\n>> commits in the first parent line that only have one parent. My\n>> expectation is more or less that git fast-export should output the same\n>> set of commits as git rev-list would output given the same arguments,\n>> which matches how it acts when given revision ranges.\n>>\n>> It seems like this behavior comes from the fact that has_unshown_parents\n>> will never be false for any merge commits encountered when fast-export\n>> is called with --first-parent. This causes the while loop to follow the\n>> pattern of pushing all commits into the \"commits\" queue until the\n>> initial commit is encountered, at which point it calls handle_tail which\n>> falls through on the first merge commit, causing most of the commits to\n>> be unhandled.\n>>\n>> My impression is that this logic only serves to ensure that parents are\n>> processed before their children, so in my patch I've opted to fix the\n>> issue by delegating that responsibility to revision.c by adding the\n>> reverse flag before performing the revision walk. From what I can see,\n>> this should be equivalent to what the previous logic is trying to\n>> achieve, but I can also see that there could be risk in these changes.\n>>\n>> Changes since v1:\n>>\n>> * Moved revs.reverse assignment down to similar assignments below.\n>> * Removed braces around single statement while loop.\n>> * The test now only changes directory inside a sub-shell.\n>> * Applied stylistic feedback on test such as: making redirections be on\n>> the form >FILE etc.\n>>\n>> There were questions raised about whether it makes sense at all for\n>> fast-export to simply accept all git rev-list arguments whether they\n>> have an effect or not - in particular for a flag like --reverse. I think\n>> I agree that it is questionable behavior, or at least questionably\n>> documented, but I also think it is out of scope for this change.\n>>\n>> I did consider teaching fast-export to complain if given --reverse, but\n>> it seemed inconsistent to me as it will gladly accept every other\n>> rev-list argument (for example, \"git fast-export HEAD --show-signature\n>> --graph\" works just fine).\n>>\n>> cc: Ævar Arnfjörð Bjarmason avarab@gmail.com\n>>\n> ...\n>> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n>> index 8e2caf72819..cb1d6473f12 100644\n>> --- a/builtin/fast-export.c\n>> +++ b/builtin/fast-export.c\n>> @@ -107,18 +107,6 @@ static int parse_opt_reencode_mode(const struct option *opt,\n>>\n>> static struct decoration idnums;\n>> static uint32_t last_idnum;\n>> -\n>> -static int has_unshown_parent(struct commit *commit)\n>> -{\n>> - struct commit_list *parent;\n>> -\n>> - for (parent = commit->parents; parent; parent = parent->next)\n>> - if (!(parent->item->object.flags & SHOWN) &&\n>> - !(parent->item->object.flags & UNINTERESTING))\n>> - return 1;\n>> - return 0;\n>> -}\n>> -\n>> struct anonymized_entry {\n>> struct hashmap_entry hash;\n>> const char *anon;\n>> @@ -752,20 +740,6 @@ static char *anonymize_tag(void *data)\n>> return strbuf_detach(&out, NULL);\n>> }\n>>\n>> -static void handle_tail(struct object_array *commits, struct rev_info *revs,\n>> - struct string_list *paths_of_changed_objects)\n>> -{\n>> - struct commit *commit;\n>> - while (commits->nr) {\n>> - commit = (struct commit *)object_array_pop(commits);\n>> - if (has_unshown_parent(commit)) {\n>> - /* Queue again, to be handled later */\n>> - add_object_array(&commit->object, NULL, commits);\n>> - return;\n>> - }\n>> - handle_commit(commit, revs, paths_of_changed_objects);\n>> - }\n>> -}\n> \n> Deleted code is debugged code! :-)\n> \n>>\n>> static void handle_tag(const char *name, struct tag *tag)\n>> {\n>> @@ -1185,7 +1159,6 @@ static int parse_opt_anonymize_map(const struct option *opt,\n>> int cmd_fast_export(int argc, const char **argv, const char *prefix)\n>> {\n>> struct rev_info revs;\n>> - struct object_array commits = OBJECT_ARRAY_INIT;\n>> struct commit *commit;\n>> char *export_filename = NULL,\n>> *import_filename = NULL,\n>> @@ -1283,18 +1256,13 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n>>\n>> if (prepare_revision_walk(&revs))\n>> die(\"revision walk setup failed\");\n>> +\n>> + revs.reverse = 1;\n> \n> I really wanted to see this next to the\n> rev.topo_order = 1\n> statement elsewhere, but as you alluded to in the commit message, this\n> part of revision.c makes that unsafe:\n> \n> \"\"\"\n> } else if (!strcmp(arg, \"--reverse\")) {\n> revs->reverse ^= 1;\n> \"\"\"\n> \n> However, given that it's unsafe to set revs.reverse=1 earlier, now\n> that I think about it, isn't it also unsafe to set revs.topo_order\n> where it is? Someone could override it by passing --date-order to\n> fast-export. (It's okay if you want to leave fixing that to someone\n> else, just thought I'd mention it while reviewing.)\n> \n\nI couldn't tell you for sure if the topo_order placement is safe. I at\nleast don't see any place where topo_order itself can be toggled off in\nrevision.c. I'm sure there exists at least one rev-list argument which\nwill cause unexpected behaviour, though.\n\nI agree that it would be nice to have the traversal order options be\nassigned in the same place. I guess we have three options:\n\n\n 1. Put the reverse assignment to the top (together with topo_order),\nallowing the user to disable it with --reverse, which will cause odd\nbehaviour.\n\n 2. Put the reverse assignment to the top and throw an error if the\nuser passes the --reverse option.\n\n 3. Keep the reverse assignment at the bottom, silently ignoring any\n--reverse option.\n\n\nI don't think any of the three options are particularly good. The first\none for obvious reasons. The second seems inconsistent to me as we would\nonly error on --reverse but not any of the other \"nonsensical\" rev-list\nargs. However, silently ignoring certain arguments does also not make\nfor a good user experience.\n\nI think that it might be a good idea to move up the 'reverse' assignment\nand then add a paragraph to the man page for git-fast-export explaining\nthat some arguments, in particular the ones that change the ordering of\ncommits and the ones that change how commits are displayed (such as \n--graph), may have no or unexpected effects.\n\nI've tried writing a snippet in git-fast-export.txt, which I could include\nin the next version, if you think it seems like a reasonable approach:\n\ndiff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt\nindex 1978dbdc6a..34875ef01d 100644\n--- a/Documentation/git-fast-export.txt\n+++ b/Documentation/git-fast-export.txt\n@@ -157,16 +157,21 @@ by keeping the marks the same across runs.\n [...]::\n \tA list of arguments, acceptable to 'git rev-parse' and\n \t'git rev-list', that specifies the specific objects and references\n \tto export. For example, `master~10..master` causes the\n \tcurrent master reference to be exported along with all objects\n \tadded since its 10th ancestor commit and (unless the\n \t--reference-excluded-parents option is specified) all files\n \tcommon to master{tilde}9 and master{tilde}10.\n++\n+Arguments to `git rev-list` which change the _order_ in which commits are\n+traversed, such as '--reverse', as well as arguments which control how commits\n+are displayed, such as '--graph', may either have no effect or have an\n+unexpected effect on which commits are exported.\n \n EXAMPLES\n --------\n\n\n>> revs.diffopt.format_callback = show_filemodify;\n>> revs.diffopt.format_callback_data = &paths_of_changed_objects;\n>> revs.diffopt.flags.recursive = 1;\n>> - while ((commit = get_revision(&revs))) {\n>> - if (has_unshown_parent(commit)) {\n>> - add_object_array(&commit->object, NULL, &commits);\n>> - }\n>> - else {\n>> - handle_commit(commit, &revs, &paths_of_changed_objects);\n>> - handle_tail(&commits, &revs, &paths_of_changed_objects);\n>> - }\n>> - }\n>> + while ((commit = get_revision(&revs)))\n>> + handle_commit(commit, &revs, &paths_of_changed_objects);\n> \n> Looks good. Nice work on finding this.\n> \n>>\n>> handle_tags_and_duplicates(&extra_refs);\n>> handle_tags_and_duplicates(&tag_refs);\n>> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\n>> index 409b48e2442..df1a5b1013a 100755\n>> --- a/t/t9350-fast-export.sh\n>> +++ b/t/t9350-fast-export.sh\n>> @@ -750,4 +750,39 @@ test_expect_success 'merge commit gets exported with --import-marks' '\n>> )\n>> '\n>>\n>> +\n>> +test_expect_success 'fast-export --first-parent outputs all revisions output by revision walk' '\n>> + git init first-parent &&\n>> + (\n>> + cd first-parent &&\n>> + test_commit A &&\n>> + git checkout -b topic1 &&\n>> + test_commit B &&\n>> + git checkout main &&\n>> + git merge topic1 --no-ff &&\n>> +\n>> + git checkout -b topic2 &&\n>> + test_commit C &&\n>> + git checkout main &&\n>> + git merge topic2 --no-ff &&\n> \n> micro nit: I'd really prefer the --no-ff before the \"topic1\" and \"topic2\".\n> \n\nI agree. I'll move it forward.\n\n>> +\n>> + test_commit D &&\n>> +\n>> + git rev-list --format=\"%ad%B\" --first-parent --topo-order --no-commit-header main >expected &&\n> \n> I'd much prefer this were changed to\n> git log --format=\"%ad %s\" --first-parent >expected &&\n> because:\n> * --topo-order is irrelevant when you have a linear history (which\n> --first-parent gives you)\n> * --no-commit-header can be tossed by using log instead of rev-list\n> * %B provides the entire (multi-line) commit message body but you\n> clearly care about how many commits you saw below and were assuming\n> just one line per commit, so %s is better.\n> * The format looks weird when inspecting as a human; it's much nicer\n> with a space between the %ad and %s.\n>\n\nI think that makes sense. That would also allow me to use 'actual' in\nthe line count test below.\n\n>> +\n>> + git fast-export main -- --first-parent >first-parent-export &&\n>> + git fast-export main -- --first-parent --reverse >first-parent-reverse-export &&\n>> +\n>> + git init import &&\n>> + git -C import fast-import > +\n>> + git -C import rev-list --format=\"%ad%B\" --topo-order --all --no-commit-header >actual &&\n> \n> Same simplifications as above here:\n> git -C import log --format=\"%ad %s\" --topo-order --all >actual &&\n> \n> However, is there a reason you're using \"--all\" instead of \"main\"?\n> Although main is the only branch, which makes either output the same\n> thing, it'd be easier for folks reading to catch the equivalence if it\n> was clear you were now listing information about the same branch.\n> \n\nI guess the intent is to be completely sure that only four commits were\nexported, and no other refs made it into the new repository. I don't feel\ntoo strongly about it, but I think it is a slightly stronger test than\nleaving out the '--all'.\n\n>> + git -C import rev-list --all >tmp &&\n>> +\n>> + test_line_count = 4 tmp &&\n> \n> I don't understand why you need tmp. Why not just use actual on the\n> test_line_count line?\n> \n\nAs you noted above, I used '%B' for writing into 'actual', so I don't think\nI can use that to verify that the correct number of commits have been\nexported. I guess I can use your suggestion above and then save one check.\n\n>> + test_cmp expected actual &&\n>> + test_cmp first-parent-export first-parent-reverse-export\n> \n> This last line would be much nicer to include right after\n> first-parent-reverse-export is created. Or else move the creation of\n> first-parent-reverse-export down to just above this line.\n> \n\nI agree. I will move it up. Except if we decide on moving the revs.reverse\nassignment to the topo_order assignment, then passing '--reverse' won't be\na no-op and I'll remove the check instead.\n\n>> + )\n>> +'\n>> +\n>> test_done\n\n"},{"id":"444056","messageId":"CABPp-BHwk25XK7qT4C8KELWXObc_W2DCXusPfLSMUuUKmeCUxw@mail.gmail.com","threadId":"56964","inReplyTo":"b87ec8f9-dd0d-c7b8-1c2a-edfd3d015930@unity3d.com","subject":"Re: [PATCH v2] fast-export: fix surprising behavior with --first-parent","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-12-14T00:31:12Z","receivedAt":"2021-12-14T00:31:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Dec 13, 2021 at 7:09 AM William Sprent wrote:\n>\n> > However, given that it's unsafe to set revs.reverse=1 earlier, now\n> > that I think about it, isn't it also unsafe to set revs.topo_order\n> > where it is? Someone could override it by passing --date-order to\n> > fast-export. (It's okay if you want to leave fixing that to someone\n> > else, just thought I'd mention it while reviewing.)\n> >\n>\n> I couldn't tell you for sure if the topo_order placement is safe. I at\n> least don't see any place where topo_order itself can be toggled off in\n> revision.c. I'm sure there exists at least one rev-list argument which\n> will cause unexpected behaviour, though.\n>\n> I agree that it would be nice to have the traversal order options be\n> assigned in the same place. I guess we have three options:\n>\n>\n> 1. Put the reverse assignment to the top (together with topo_order),\n> allowing the user to disable it with --reverse, which will cause odd\n> behaviour.\n\nI'd call it broken rather than merely odd; more on this below.\n\n> 2. Put the reverse assignment to the top and throw an error if the\n> user passes the --reverse option.\n\nMight be a reasonable longer term solution if someone wants to dive\ninto all the non-sensical options and mark them as such. But I agree\nthat it's slightly odd only picking one specific one when we know\nthere's likely a pile of them here.\n\n> 3. Keep the reverse assignment at the bottom, silently ignoring any\n> --reverse option.\n\n\"silently ignored\" or \"dismissed with prejudice\"? :-)\n\n> I don't think any of the three options are particularly good. The first\n> one for obvious reasons. The second seems inconsistent to me as we would\n> only error on --reverse but not any of the other \"nonsensical\" rev-list\n> args. However, silently ignoring certain arguments does also not make\n> for a good user experience.\n>\n> I think that it might be a good idea to move up the 'reverse' assignment\n> and then add a paragraph to the man page for git-fast-export explaining\n> that some arguments, in particular the ones that change the ordering of\n> commits and the ones that change how commits are displayed (such as\n> --graph), may have no or unexpected effects.\n\nI'd rather choose option #3, like builtin/add.c does with max_count.\nIn part this is because...\n\n> I've tried writing a snippet in git-fast-export.txt, which I could include\n> in the next version, if you think it seems like a reasonable approach:\n>\n> diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt\n> index 1978dbdc6a..34875ef01d 100644\n> --- a/Documentation/git-fast-export.txt\n> +++ b/Documentation/git-fast-export.txt\n> @@ -157,16 +157,21 @@ by keeping the marks the same across runs.\n> [...]::\n> A list of arguments, acceptable to 'git rev-parse' and\n> 'git rev-list', that specifies the specific objects and references\n> to export. For example, `master~10..master` causes the\n> current master reference to be exported along with all objects\n> added since its 10th ancestor commit and (unless the\n> --reference-excluded-parents option is specified) all files\n> common to master{tilde}9 and master{tilde}10.\n> ++\n> +Arguments to `git rev-list` which change the _order_ in which commits are\n> +traversed, such as '--reverse', as well as arguments which control how commits\n> +are displayed, such as '--graph', may either have no effect or have an\n> +unexpected effect on which commits are exported.\n\nAfter your patch, --reverse won't have an unexpected effect on _which_\ncommits are exported, it would instead have an unexpected effect on\n_how_ commits are exported (turning _every_ commit into a root\ncommit). I'd rather just go with your option #3.\n\n> >> +\n> >> + git fast-export main -- --first-parent >first-parent-export &&\n> >> + git fast-export main -- --first-parent --reverse >first-parent-reverse-export &&\n> >> +\n> >> + git init import &&\n> >> + git -C import fast-import >> +\n> >> + git -C import rev-list --format=\"%ad%B\" --topo-order --all --no-commit-header >actual &&\n> >\n> > Same simplifications as above here:\n> > git -C import log --format=\"%ad %s\" --topo-order --all >actual &&\n> >\n> > However, is there a reason you're using \"--all\" instead of \"main\"?\n> > Although main is the only branch, which makes either output the same\n> > thing, it'd be easier for folks reading to catch the equivalence if it\n> > was clear you were now listing information about the same branch.\n> >\n>\n> I guess the intent is to be completely sure that only four commits were\n> exported, and no other refs made it into the new repository. I don't feel\n> too strongly about it, but I think it is a slightly stronger test than\n> leaving out the '--all'.\n\nFair enough, '--all' works for me with that explanation.\n"},{"id":"444100","messageId":"8f1c66f7-f0d8-bd95-dbc1-f0f3e25b3dff@unity3d.com","threadId":"56964","inReplyTo":"CABPp-BHwk25XK7qT4C8KELWXObc_W2DCXusPfLSMUuUKmeCUxw@mail.gmail.com","subject":"Re: [PATCH v2] fast-export: fix surprising behavior with --first-parent","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2021-12-14T13:11:48Z","receivedAt":"2021-12-14T13:11:53Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 14/12/2021 01.31, Elijah Newren wrote:\n> On Mon, Dec 13, 2021 at 7:09 AM William Sprent wrote:\n>>\n>>> However, given that it's unsafe to set revs.reverse=1 earlier, now\n>>> that I think about it, isn't it also unsafe to set revs.topo_order\n>>> where it is? Someone could override it by passing --date-order to\n>>> fast-export. (It's okay if you want to leave fixing that to someone\n>>> else, just thought I'd mention it while reviewing.)\n>>>\n>>\n>> I couldn't tell you for sure if the topo_order placement is safe. I at\n>> least don't see any place where topo_order itself can be toggled off in\n>> revision.c. I'm sure there exists at least one rev-list argument which\n>> will cause unexpected behaviour, though.\n>>\n>> I agree that it would be nice to have the traversal order options be\n>> assigned in the same place. I guess we have three options:\n>>\n>>\n>> 1. Put the reverse assignment to the top (together with topo_order),\n>> allowing the user to disable it with --reverse, which will cause odd\n>> behaviour.\n> \n> I'd call it broken rather than merely odd; more on this below.\n> \n>> 2. Put the reverse assignment to the top and throw an error if the\n>> user passes the --reverse option.\n> \n> Might be a reasonable longer term solution if someone wants to dive\n> into all the non-sensical options and mark them as such. But I agree\n> that it's slightly odd only picking one specific one when we know\n> there's likely a pile of them here.\n> \n>> 3. Keep the reverse assignment at the bottom, silently ignoring any\n>> --reverse option.\n> \n> \"silently ignored\" or \"dismissed with prejudice\"? :-)\n> \n\nHeh. :) It would definitely an \"interesting\" option to be able to \nreverse the commit graph, if it worked..\n\n>> I don't think any of the three options are particularly good. The first\n>> one for obvious reasons. The second seems inconsistent to me as we would\n>> only error on --reverse but not any of the other \"nonsensical\" rev-list\n>> args. However, silently ignoring certain arguments does also not make\n>> for a good user experience.\n>>\n>> I think that it might be a good idea to move up the 'reverse' assignment\n>> and then add a paragraph to the man page for git-fast-export explaining\n>> that some arguments, in particular the ones that change the ordering of\n>> commits and the ones that change how commits are displayed (such as\n>> --graph), may have no or unexpected effects.\n> \n> I'd rather choose option #3, like builtin/add.c does with max_count.\n> In part this is because...\n> \n>> I've tried writing a snippet in git-fast-export.txt, which I could include\n>> in the next version, if you think it seems like a reasonable approach:\n>>\n>> diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt\n>> index 1978dbdc6a..34875ef01d 100644\n>> --- a/Documentation/git-fast-export.txt\n>> +++ b/Documentation/git-fast-export.txt\n>> @@ -157,16 +157,21 @@ by keeping the marks the same across runs.\n>> [...]::\n>> A list of arguments, acceptable to 'git rev-parse' and\n>> 'git rev-list', that specifies the specific objects and references\n>> to export. For example, `master~10..master` causes the\n>> current master reference to be exported along with all objects\n>> added since its 10th ancestor commit and (unless the\n>> --reference-excluded-parents option is specified) all files\n>> common to master{tilde}9 and master{tilde}10.\n>> ++\n>> +Arguments to `git rev-list` which change the _order_ in which commits are\n>> +traversed, such as '--reverse', as well as arguments which control how commits\n>> +are displayed, such as '--graph', may either have no effect or have an\n>> +unexpected effect on which commits are exported.\n> \n> After your patch, --reverse won't have an unexpected effect on _which_\n> commits are exported, it would instead have an unexpected effect on\n> _how_ commits are exported (turning _every_ commit into a root\n> commit). I'd rather just go with your option #3.\n> \n\nAlright. I guess another solution would have been to move topo_order \ndown, but that seems unsafe as well. According to your commit, \n668f3aa776 (fast-export: Set revs.topo_order before calling \nsetup_revisions, 2009-06-25).\n\nI'll leave the revs.reverse assignment where it is for now.\n\n>>>> +\n>>>> + git fast-export main -- --first-parent >first-parent-export &&\n>>>> + git fast-export main -- --first-parent --reverse >first-parent-reverse-export &&\n>>>> +\n>>>> + git init import &&\n>>>> + git -C import fast-import >>> +\n>>>> + git -C import rev-list --format=\"%ad%B\" --topo-order --all --no-commit-header >actual &&\n>>>\n>>> Same simplifications as above here:\n>>> git -C import log --format=\"%ad %s\" --topo-order --all >actual &&\n>>>\n>>> However, is there a reason you're using \"--all\" instead of \"main\"?\n>>> Although main is the only branch, which makes either output the same\n>>> thing, it'd be easier for folks reading to catch the equivalence if it\n>>> was clear you were now listing information about the same branch.\n>>>\n>>\n>> I guess the intent is to be completely sure that only four commits were\n>> exported, and no other refs made it into the new repository. I don't feel\n>> too strongly about it, but I think it is a slightly stronger test than\n>> leaving out the '--all'.\n> \n> Fair enough, '--all' works for me with that explanation.\n> \n\nOk. In summary I'll use the commit message you wrote and apply the rest \nof your feedback for the test for the next version.\n"},{"id":"444239","messageId":"pull.1084.v3.git.1639671789279.gitgitgadget@gmail.com","threadId":"56964","inReplyTo":"pull.1084.v2.git.1639037637231.gitgitgadget@gmail.com","subject":"[PATCH v3] fast-export: fix surprising behavior with --first-parent","fromName":"William Sprent via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-16T16:23:09Z","receivedAt":"2021-12-16T16:23:16Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"From: William Sprent \n\nThe revision traversal machinery typically processes and returns all\nchildren before any parent. fast-export needs to operate in the\nreverse fashion, handling parents before any of their children in\norder to build up the history starting from the root commit(s). This\nwould be a clear case where we could just use the revision traversal\nmachinery's \"reverse\" option to achieve this desired affect.\n\nHowever, this wasn't what the code did. It added its own array for\nqueuing. The obvious hand-rolled solution would be to just push all\nthe commits into the array and then traverse afterwards, but it didn't\nquite do that either. It instead attempted to process anything it\ncould as soon as it could, and once it could, check whether it could\nprocess anything that had been queued. As far as I can tell, this was\nan effort to save a little memory in the case of multiple root commits\nsince it could process some commits before queueing all of them. This\ninvolved some helper functions named has_unshown_parent() and\nhandle_tail(). For typical invocations of fast-export, this\nalternative essentially amounted to a hand-rolled method of reversing\nthe commits -- it was a bunch of work to duplicate the revision\ntraversal machinery's \"reverse\" option.\n\nThis hand-rolled reversing mechanism is actually somewhat difficult to\nreason about. It takes some time to figure out how it ensures in\nnormal cases that it will actually process all traversed commits\n(rather than just dropping some and not printing anything for them).\n\nAnd it turns out there are some cases where the code does drop commits\nwithout handling them, and not even printing an error or warning for\nthe user. Due to the has_unshown_parent() checks, some commits could\nbe left in the array at the end of the \"while...get_revision()\" loop\nwhich would be unprocessed. This could be triggered for example with\n git fast-export main -- --first-parent\nor non-sensical traversal rules such as\n git fast-export main -- --grep=Merge --invert-grep\n\nWhile most traversals that don't include all parents should likely\ntrigger errors in fast-export (or at least require being used in\ncombination with --reference-excluded-parents), the --first-parent\ntraversal is at least reasonable and it'd be nice if it didn't just drop\ncommits. It'd also be nice for future readers of the code to have a\nsimpler \"reverse traversal\" mechanism. Use the \"reverse\" option of the\nrevision traversal machinery to achieve both.\n\nEven for the non-sensical traversal flags like the --grep one above,\nthis would be an improvement. For example, in that case, the code\npreviously would have silently truncated history to only those commits\nthat do not have an ancestor containing \"Merge\" in their commit message.\nAfter this code change, that case would include all commits without\n\"Merge\" in their commit message -- but any commit that previously had a\n\"Merge\"-mentioning parent would lose that parent\n(likely resulting in many new root commits). While the new behavior is\nstill odd, it is at least understandable given that\n--reference-excluded-parents is not the default.\n\nSigned-off-by: William Sprent \n---\n fast-export: fix surprising behavior with --first-parent\n \n Hi,\n \n Here's a revised version of my previously submitted patch regarding\n fast-export and how it reacts to the --first-parent flag.\n \n Changes since v2:\n \n * Changed commit message to include the detailed description of the\n problem area as suggested by Elijah. I went back and forth with my\n self about whether the message needs some \"lead in\", but it ended up\n getting long without adding much.\n * Cleaned up test such that the comparison with the --reverse output is\n just below where it is generated.\n * In test, moved all usages of --no-ff to just after 'git merge' .\n * In test, replaced long 'git rev-list' incantation with simpler 'git\n log' format.\n \n Changes since v1:\n \n * Moved revs.reverse assignment down to similar assignments below.\n * Removed braces around single statement while loop.\n * The test now only changes directory inside a sub-shell.\n * Applied stylistic feedback on test such as: making redirections be on\n the form >FILE etc.\n \n There were questions raised about whether it makes sense at all for\n fast-export to simply accept all git rev-list arguments whether they\n have an effect or not - in particular for a flag like --reverse. I think\n I agree that it is questionable behavior, or at least questionably\n documented, but I also think it is out of scope for this change.\n \n I did consider teaching fast-export to complain if given --reverse, but\n it seemed inconsistent to me as it will gladly accept every other\n rev-list argument (for example, \"git fast-export HEAD --show-signature\n --graph\" works just fine).\n \n Original Message: I've noticed that git fast-export exhibits some odd\n behavior when passed the --first-parent flag. In the repository I was\n working on, it would only output the initial commit before exiting.\n Moreover, adding the --reverse flag causes it to behave differently and\n instead output all commits in the first parent line that only have one\n parent. My expectation is more or less that git fast-export should\n output the same set of commits as git rev-list would output given the\n same arguments, which matches how it acts when given revision ranges.\n \n It seems like this behavior comes from the fact that has_unshown_parents\n will never be false for any merge commits encountered when fast-export\n is called with --first-parent. This causes the while loop to follow the\n pattern of pushing all commits into the \"commits\" queue until the\n initial commit is encountered, at which point it calls handle_tail which\n falls through on the first merge commit, causing most of the commits to\n be unhandled.\n \n My impression is that this logic only serves to ensure that parents are\n processed before their children, so in my patch I've opted to fix the\n issue by delegating that responsibility to revision.c by adding the\n reverse flag before performing the revision walk. From what I can see,\n this should be equivalent to what the previous logic is trying to\n achieve, but I can also see that there could be risk in these changes.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1084%2Fwilliams-unity%2Ffast-export%2Ffirst-parent-issues-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1084/williams-unity/fast-export/first-parent-issues-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1084\n\nRange-diff vs v2:\n\n 1: af552f37dbe ! 1: 312255260b1 fast-export: fix surprising behavior with --first-parent\n @@ Metadata\n ## Commit message ##\n fast-export: fix surprising behavior with --first-parent\n \n - When invoking git-fast-export with the --first-parent flag on a branch\n - with merges, fast-export would early-out on processing the first merge\n - on the branch. If combined with --reverse, fast-export would instead\n - output all single parent commits on the branch.\n + The revision traversal machinery typically processes and returns all\n + children before any parent. fast-export needs to operate in the\n + reverse fashion, handling parents before any of their children in\n + order to build up the history starting from the root commit(s). This\n + would be a clear case where we could just use the revision traversal\n + machinery's \"reverse\" option to achieve this desired affect.\n \n - This commit makes fast-export output the same commits as rev-list\n - --first-parent, and makes --reverse not have an effect on which commits\n - are output.\n + However, this wasn't what the code did. It added its own array for\n + queuing. The obvious hand-rolled solution would be to just push all\n + the commits into the array and then traverse afterwards, but it didn't\n + quite do that either. It instead attempted to process anything it\n + could as soon as it could, and once it could, check whether it could\n + process anything that had been queued. As far as I can tell, this was\n + an effort to save a little memory in the case of multiple root commits\n + since it could process some commits before queueing all of them. This\n + involved some helper functions named has_unshown_parent() and\n + handle_tail(). For typical invocations of fast-export, this\n + alternative essentially amounted to a hand-rolled method of reversing\n + the commits -- it was a bunch of work to duplicate the revision\n + traversal machinery's \"reverse\" option.\n \n - The fix involves removing logic within fast-export which was responsible\n - for ensuring that parents are processed before their children, which was\n - what was exiting early due to missing second parents. This is replaced\n - by setting 'reverse = 1' before revision walking, which, in conjuction\n - with topo_order, allows for delegating the ordering of commits to\n - revision.c. The reverse flag is set after parsing rev-list arguments to\n - avoid having it disabled.\n + This hand-rolled reversing mechanism is actually somewhat difficult to\n + reason about. It takes some time to figure out how it ensures in\n + normal cases that it will actually process all traversed commits\n + (rather than just dropping some and not printing anything for them).\n +\n + And it turns out there are some cases where the code does drop commits\n + without handling them, and not even printing an error or warning for\n + the user. Due to the has_unshown_parent() checks, some commits could\n + be left in the array at the end of the \"while...get_revision()\" loop\n + which would be unprocessed. This could be triggered for example with\n + git fast-export main -- --first-parent\n + or non-sensical traversal rules such as\n + git fast-export main -- --grep=Merge --invert-grep\n +\n + While most traversals that don't include all parents should likely\n + trigger errors in fast-export (or at least require being used in\n + combination with --reference-excluded-parents), the --first-parent\n + traversal is at least reasonable and it'd be nice if it didn't just drop\n + commits. It'd also be nice for future readers of the code to have a\n + simpler \"reverse traversal\" mechanism. Use the \"reverse\" option of the\n + revision traversal machinery to achieve both.\n +\n + Even for the non-sensical traversal flags like the --grep one above,\n + this would be an improvement. For example, in that case, the code\n + previously would have silently truncated history to only those commits\n + that do not have an ancestor containing \"Merge\" in their commit message.\n + After this code change, that case would include all commits without\n + \"Merge\" in their commit message -- but any commit that previously had a\n + \"Merge\"-mentioning parent would lose that parent\n + (likely resulting in many new root commits). While the new behavior is\n + still odd, it is at least understandable given that\n + --reference-excluded-parents is not the default.\n \n Signed-off-by: William Sprent \n \n @@ t/t9350-fast-export.sh: test_expect_success 'merge commit gets exported with --i\n +\t\tgit checkout -b topic1 &&\n +\t\ttest_commit B &&\n +\t\tgit checkout main &&\n -+\t\tgit merge topic1 --no-ff &&\n ++\t\tgit merge --no-ff topic1 &&\n +\n +\t\tgit checkout -b topic2 &&\n +\t\ttest_commit C &&\n +\t\tgit checkout main &&\n -+\t\tgit merge topic2 --no-ff &&\n ++\t\tgit merge --no-ff topic2 &&\n +\n +\t\ttest_commit D &&\n +\n -+\t\tgit rev-list --format=\"%ad%B\" --first-parent --topo-order --no-commit-header main >expected &&\n -+\n +\t\tgit fast-export main -- --first-parent >first-parent-export &&\n +\t\tgit fast-export main -- --first-parent --reverse >first-parent-reverse-export &&\n ++\t\ttest_cmp first-parent-export first-parent-reverse-export &&\n +\n +\t\tgit init import &&\n +\t\tgit -C import fast-import actual &&\n -+\t\tgit -C import rev-list --all >tmp &&\n -+\n -+\t\ttest_line_count = 4 tmp &&\n ++\t\tgit log --format=\"%ad %s\" --first-parent main >expected &&\n ++\t\tgit -C import log --format=\"%ad %s\" --all >actual &&\n +\t\ttest_cmp expected actual &&\n -+\t\ttest_cmp first-parent-export first-parent-reverse-export\n ++\t\ttest_line_count = 4 actual\n +\t)\n +'\n +\n\n\n builtin/fast-export.c | 40 ++++------------------------------------\n t/t9350-fast-export.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 8e2caf72819..cb1d6473f12 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -107,18 +107,6 @@ static int parse_opt_reencode_mode(const struct option *opt,\n \n static struct decoration idnums;\n static uint32_t last_idnum;\n-\n-static int has_unshown_parent(struct commit *commit)\n-{\n-\tstruct commit_list *parent;\n-\n-\tfor (parent = commit->parents; parent; parent = parent->next)\n-\t\tif (!(parent->item->object.flags & SHOWN) &&\n-\t\t !(parent->item->object.flags & UNINTERESTING))\n-\t\t\treturn 1;\n-\treturn 0;\n-}\n-\n struct anonymized_entry {\n \tstruct hashmap_entry hash;\n \tconst char *anon;\n@@ -752,20 +740,6 @@ static char *anonymize_tag(void *data)\n \treturn strbuf_detach(&out, NULL);\n }\n \n-static void handle_tail(struct object_array *commits, struct rev_info *revs,\n-\t\t\tstruct string_list *paths_of_changed_objects)\n-{\n-\tstruct commit *commit;\n-\twhile (commits->nr) {\n-\t\tcommit = (struct commit *)object_array_pop(commits);\n-\t\tif (has_unshown_parent(commit)) {\n-\t\t\t/* Queue again, to be handled later */\n-\t\t\tadd_object_array(&commit->object, NULL, commits);\n-\t\t\treturn;\n-\t\t}\n-\t\thandle_commit(commit, revs, paths_of_changed_objects);\n-\t}\n-}\n \n static void handle_tag(const char *name, struct tag *tag)\n {\n@@ -1185,7 +1159,6 @@ static int parse_opt_anonymize_map(const struct option *opt,\n int cmd_fast_export(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n-\tstruct object_array commits = OBJECT_ARRAY_INIT;\n \tstruct commit *commit;\n \tchar *export_filename = NULL,\n \t *import_filename = NULL,\n@@ -1283,18 +1256,13 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n+\n+\trevs.reverse = 1;\n \trevs.diffopt.format_callback = show_filemodify;\n \trevs.diffopt.format_callback_data = &paths_of_changed_objects;\n \trevs.diffopt.flags.recursive = 1;\n-\twhile ((commit = get_revision(&revs))) {\n-\t\tif (has_unshown_parent(commit)) {\n-\t\t\tadd_object_array(&commit->object, NULL, &commits);\n-\t\t}\n-\t\telse {\n-\t\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n-\t\t\thandle_tail(&commits, &revs, &paths_of_changed_objects);\n-\t\t}\n-\t}\n+\twhile ((commit = get_revision(&revs)))\n+\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n \n \thandle_tags_and_duplicates(&extra_refs);\n \thandle_tags_and_duplicates(&tag_refs);\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 409b48e2442..7b7a18dd2c1 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -750,4 +750,36 @@ test_expect_success 'merge commit gets exported with --import-marks' '\n \t)\n '\n \n+\n+test_expect_success 'fast-export --first-parent outputs all revisions output by revision walk' '\n+\tgit init first-parent &&\n+\t(\n+\t\tcd first-parent &&\n+\t\ttest_commit A &&\n+\t\tgit checkout -b topic1 &&\n+\t\ttest_commit B &&\n+\t\tgit checkout main &&\n+\t\tgit merge --no-ff topic1 &&\n+\n+\t\tgit checkout -b topic2 &&\n+\t\ttest_commit C &&\n+\t\tgit checkout main &&\n+\t\tgit merge --no-ff topic2 &&\n+\n+\t\ttest_commit D &&\n+\n+\t\tgit fast-export main -- --first-parent >first-parent-export &&\n+\t\tgit fast-export main -- --first-parent --reverse >first-parent-reverse-export &&\n+\t\ttest_cmp first-parent-export first-parent-reverse-export &&\n+\n+\t\tgit init import &&\n+\t\tgit -C import fast-import expected &&\n+\t\tgit -C import log --format=\"%ad %s\" --all >actual &&\n+\t\ttest_cmp expected actual &&\n+\t\ttest_line_count = 4 actual\n+\t)\n+'\n+\n test_done\n\nbase-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc\n-- \ngitgitgadget\n"},{"id":"444650","messageId":"CABPp-BF7Rg3NH3UBnyzD6c1vuFh+FNrNc8eRvUffqmaLppUPkw@mail.gmail.com","threadId":"56964","inReplyTo":"pull.1084.v3.git.1639671789279.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] fast-export: fix surprising behavior with --first-parent","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-12-21T18:47:22Z","receivedAt":"2021-12-21T18:47:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Dec 16, 2021 at 8:23 AM William Sprent via GitGitGadget\n wrote:\n>\n> From: William Sprent \n>\n> The revision traversal machinery typically processes and returns all\n> children before any parent. fast-export needs to operate in the\n> reverse fashion, handling parents before any of their children in\n> order to build up the history starting from the root commit(s). This\n> would be a clear case where we could just use the revision traversal\n> machinery's \"reverse\" option to achieve this desired affect.\n>\n> However, this wasn't what the code did. It added its own array for\n> queuing. The obvious hand-rolled solution would be to just push all\n> the commits into the array and then traverse afterwards, but it didn't\n> quite do that either. It instead attempted to process anything it\n> could as soon as it could, and once it could, check whether it could\n> process anything that had been queued. As far as I can tell, this was\n> an effort to save a little memory in the case of multiple root commits\n> since it could process some commits before queueing all of them. This\n> involved some helper functions named has_unshown_parent() and\n> handle_tail(). For typical invocations of fast-export, this\n> alternative essentially amounted to a hand-rolled method of reversing\n> the commits -- it was a bunch of work to duplicate the revision\n> traversal machinery's \"reverse\" option.\n>\n> This hand-rolled reversing mechanism is actually somewhat difficult to\n> reason about. It takes some time to figure out how it ensures in\n> normal cases that it will actually process all traversed commits\n> (rather than just dropping some and not printing anything for them).\n>\n> And it turns out there are some cases where the code does drop commits\n> without handling them, and not even printing an error or warning for\n> the user. Due to the has_unshown_parent() checks, some commits could\n> be left in the array at the end of the \"while...get_revision()\" loop\n> which would be unprocessed. This could be triggered for example with\n> git fast-export main -- --first-parent\n> or non-sensical traversal rules such as\n> git fast-export main -- --grep=Merge --invert-grep\n>\n> While most traversals that don't include all parents should likely\n> trigger errors in fast-export (or at least require being used in\n> combination with --reference-excluded-parents), the --first-parent\n> traversal is at least reasonable and it'd be nice if it didn't just drop\n> commits. It'd also be nice for future readers of the code to have a\n> simpler \"reverse traversal\" mechanism. Use the \"reverse\" option of the\n> revision traversal machinery to achieve both.\n>\n> Even for the non-sensical traversal flags like the --grep one above,\n> this would be an improvement. For example, in that case, the code\n> previously would have silently truncated history to only those commits\n> that do not have an ancestor containing \"Merge\" in their commit message.\n> After this code change, that case would include all commits without\n> \"Merge\" in their commit message -- but any commit that previously had a\n> \"Merge\"-mentioning parent would lose that parent\n> (likely resulting in many new root commits). While the new behavior is\n> still odd, it is at least understandable given that\n> --reference-excluded-parents is not the default.\n>\n> Signed-off-by: William Sprent \n> ---\n> fast-export: fix surprising behavior with --first-parent\n>\n> Hi,\n>\n> Here's a revised version of my previously submitted patch regarding\n> fast-export and how it reacts to the --first-parent flag.\n>\n> Changes since v2:\n>\n> * Changed commit message to include the detailed description of the\n> problem area as suggested by Elijah. I went back and forth with my\n> self about whether the message needs some \"lead in\", but it ended up\n> getting long without adding much.\n\nIf it feels weird quoting someone else so extensively and just using\ntheir words for the commit message, we've used a \"Commit-message-by:\"\ntrailer in the past; that could be useful here.\n\n> * Cleaned up test such that the comparison with the --reverse output is\n> just below where it is generated.\n> * In test, moved all usages of --no-ff to just after 'git merge' .\n> * In test, replaced long 'git rev-list' incantation with simpler 'git\n> log' format.\n>\n> Changes since v1:\n>\n> * Moved revs.reverse assignment down to similar assignments below.\n> * Removed braces around single statement while loop.\n> * The test now only changes directory inside a sub-shell.\n> * Applied stylistic feedback on test such as: making redirections be on\n> the form >FILE etc.\n>\n> There were questions raised about whether it makes sense at all for\n> fast-export to simply accept all git rev-list arguments whether they\n> have an effect or not - in particular for a flag like --reverse. I think\n> I agree that it is questionable behavior, or at least questionably\n> documented, but I also think it is out of scope for this change.\n>\n> I did consider teaching fast-export to complain if given --reverse, but\n> it seemed inconsistent to me as it will gladly accept every other\n> rev-list argument (for example, \"git fast-export HEAD --show-signature\n> --graph\" works just fine).\n>\n> Original Message: I've noticed that git fast-export exhibits some odd\n> behavior when passed the --first-parent flag. In the repository I was\n> working on, it would only output the initial commit before exiting.\n> Moreover, adding the --reverse flag causes it to behave differently and\n> instead output all commits in the first parent line that only have one\n> parent. My expectation is more or less that git fast-export should\n> output the same set of commits as git rev-list would output given the\n> same arguments, which matches how it acts when given revision ranges.\n>\n> It seems like this behavior comes from the fact that has_unshown_parents\n> will never be false for any merge commits encountered when fast-export\n> is called with --first-parent. This causes the while loop to follow the\n> pattern of pushing all commits into the \"commits\" queue until the\n> initial commit is encountered, at which point it calls handle_tail which\n> falls through on the first merge commit, causing most of the commits to\n> be unhandled.\n>\n> My impression is that this logic only serves to ensure that parents are\n> processed before their children, so in my patch I've opted to fix the\n> issue by delegating that responsibility to revision.c by adding the\n> reverse flag before performing the revision walk. From what I can see,\n> this should be equivalent to what the previous logic is trying to\n> achieve, but I can also see that there could be risk in these changes.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1084%2Fwilliams-unity%2Ffast-export%2Ffirst-parent-issues-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1084/williams-unity/fast-export/first-parent-issues-v3\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1084\n>\n> Range-diff vs v2:\n>\n> 1: af552f37dbe ! 1: 312255260b1 fast-export: fix surprising behavior with --first-parent\n> @@ Metadata\n> ## Commit message ##\n> fast-export: fix surprising behavior with --first-parent\n>\n> - When invoking git-fast-export with the --first-parent flag on a branch\n> - with merges, fast-export would early-out on processing the first merge\n> - on the branch. If combined with --reverse, fast-export would instead\n> - output all single parent commits on the branch.\n> + The revision traversal machinery typically processes and returns all\n> + children before any parent. fast-export needs to operate in the\n> + reverse fashion, handling parents before any of their children in\n> + order to build up the history starting from the root commit(s). This\n> + would be a clear case where we could just use the revision traversal\n> + machinery's \"reverse\" option to achieve this desired affect.\n>\n> - This commit makes fast-export output the same commits as rev-list\n> - --first-parent, and makes --reverse not have an effect on which commits\n> - are output.\n> + However, this wasn't what the code did. It added its own array for\n> + queuing. The obvious hand-rolled solution would be to just push all\n> + the commits into the array and then traverse afterwards, but it didn't\n> + quite do that either. It instead attempted to process anything it\n> + could as soon as it could, and once it could, check whether it could\n> + process anything that had been queued. As far as I can tell, this was\n> + an effort to save a little memory in the case of multiple root commits\n> + since it could process some commits before queueing all of them. This\n> + involved some helper functions named has_unshown_parent() and\n> + handle_tail(). For typical invocations of fast-export, this\n> + alternative essentially amounted to a hand-rolled method of reversing\n> + the commits -- it was a bunch of work to duplicate the revision\n> + traversal machinery's \"reverse\" option.\n>\n> - The fix involves removing logic within fast-export which was responsible\n> - for ensuring that parents are processed before their children, which was\n> - what was exiting early due to missing second parents. This is replaced\n> - by setting 'reverse = 1' before revision walking, which, in conjuction\n> - with topo_order, allows for delegating the ordering of commits to\n> - revision.c. The reverse flag is set after parsing rev-list arguments to\n> - avoid having it disabled.\n> + This hand-rolled reversing mechanism is actually somewhat difficult to\n> + reason about. It takes some time to figure out how it ensures in\n> + normal cases that it will actually process all traversed commits\n> + (rather than just dropping some and not printing anything for them).\n> +\n> + And it turns out there are some cases where the code does drop commits\n> + without handling them, and not even printing an error or warning for\n> + the user. Due to the has_unshown_parent() checks, some commits could\n> + be left in the array at the end of the \"while...get_revision()\" loop\n> + which would be unprocessed. This could be triggered for example with\n> + git fast-export main -- --first-parent\n> + or non-sensical traversal rules such as\n> + git fast-export main -- --grep=Merge --invert-grep\n> +\n> + While most traversals that don't include all parents should likely\n> + trigger errors in fast-export (or at least require being used in\n> + combination with --reference-excluded-parents), the --first-parent\n> + traversal is at least reasonable and it'd be nice if it didn't just drop\n> + commits. It'd also be nice for future readers of the code to have a\n> + simpler \"reverse traversal\" mechanism. Use the \"reverse\" option of the\n> + revision traversal machinery to achieve both.\n> +\n> + Even for the non-sensical traversal flags like the --grep one above,\n> + this would be an improvement. For example, in that case, the code\n> + previously would have silently truncated history to only those commits\n> + that do not have an ancestor containing \"Merge\" in their commit message.\n> + After this code change, that case would include all commits without\n> + \"Merge\" in their commit message -- but any commit that previously had a\n> + \"Merge\"-mentioning parent would lose that parent\n> + (likely resulting in many new root commits). While the new behavior is\n> + still odd, it is at least understandable given that\n> + --reference-excluded-parents is not the default.\n>\n> Signed-off-by: William Sprent \n>\n> @@ t/t9350-fast-export.sh: test_expect_success 'merge commit gets exported with --i\n> + git checkout -b topic1 &&\n> + test_commit B &&\n> + git checkout main &&\n> -+ git merge topic1 --no-ff &&\n> ++ git merge --no-ff topic1 &&\n> +\n> + git checkout -b topic2 &&\n> + test_commit C &&\n> + git checkout main &&\n> -+ git merge topic2 --no-ff &&\n> ++ git merge --no-ff topic2 &&\n> +\n> + test_commit D &&\n> +\n> -+ git rev-list --format=\"%ad%B\" --first-parent --topo-order --no-commit-header main >expected &&\n> -+\n> + git fast-export main -- --first-parent >first-parent-export &&\n> + git fast-export main -- --first-parent --reverse >first-parent-reverse-export &&\n> ++ test_cmp first-parent-export first-parent-reverse-export &&\n> +\n> + git init import &&\n> + git -C import fast-import +\n> -+ git -C import rev-list --format=\"%ad%B\" --topo-order --all --no-commit-header >actual &&\n> -+ git -C import rev-list --all >tmp &&\n> -+\n> -+ test_line_count = 4 tmp &&\n> ++ git log --format=\"%ad %s\" --first-parent main >expected &&\n> ++ git -C import log --format=\"%ad %s\" --all >actual &&\n> + test_cmp expected actual &&\n> -+ test_cmp first-parent-export first-parent-reverse-export\n> ++ test_line_count = 4 actual\n> + )\n> +'\n> +\n>\n>\n> builtin/fast-export.c | 40 ++++------------------------------------\n> t/t9350-fast-export.sh | 32 ++++++++++++++++++++++++++++++++\n> 2 files changed, 36 insertions(+), 36 deletions(-)\n>\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 8e2caf72819..cb1d6473f12 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -107,18 +107,6 @@ static int parse_opt_reencode_mode(const struct option *opt,\n>\n> static struct decoration idnums;\n> static uint32_t last_idnum;\n> -\n> -static int has_unshown_parent(struct commit *commit)\n> -{\n> - struct commit_list *parent;\n> -\n> - for (parent = commit->parents; parent; parent = parent->next)\n> - if (!(parent->item->object.flags & SHOWN) &&\n> - !(parent->item->object.flags & UNINTERESTING))\n> - return 1;\n> - return 0;\n> -}\n> -\n> struct anonymized_entry {\n> struct hashmap_entry hash;\n> const char *anon;\n> @@ -752,20 +740,6 @@ static char *anonymize_tag(void *data)\n> return strbuf_detach(&out, NULL);\n> }\n>\n> -static void handle_tail(struct object_array *commits, struct rev_info *revs,\n> - struct string_list *paths_of_changed_objects)\n> -{\n> - struct commit *commit;\n> - while (commits->nr) {\n> - commit = (struct commit *)object_array_pop(commits);\n> - if (has_unshown_parent(commit)) {\n> - /* Queue again, to be handled later */\n> - add_object_array(&commit->object, NULL, commits);\n> - return;\n> - }\n> - handle_commit(commit, revs, paths_of_changed_objects);\n> - }\n> -}\n>\n> static void handle_tag(const char *name, struct tag *tag)\n> {\n> @@ -1185,7 +1159,6 @@ static int parse_opt_anonymize_map(const struct option *opt,\n> int cmd_fast_export(int argc, const char **argv, const char *prefix)\n> {\n> struct rev_info revs;\n> - struct object_array commits = OBJECT_ARRAY_INIT;\n> struct commit *commit;\n> char *export_filename = NULL,\n> *import_filename = NULL,\n> @@ -1283,18 +1256,13 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n>\n> if (prepare_revision_walk(&revs))\n> die(\"revision walk setup failed\");\n> +\n> + revs.reverse = 1;\n> revs.diffopt.format_callback = show_filemodify;\n> revs.diffopt.format_callback_data = &paths_of_changed_objects;\n> revs.diffopt.flags.recursive = 1;\n> - while ((commit = get_revision(&revs))) {\n> - if (has_unshown_parent(commit)) {\n> - add_object_array(&commit->object, NULL, &commits);\n> - }\n> - else {\n> - handle_commit(commit, &revs, &paths_of_changed_objects);\n> - handle_tail(&commits, &revs, &paths_of_changed_objects);\n> - }\n> - }\n> + while ((commit = get_revision(&revs)))\n> + handle_commit(commit, &revs, &paths_of_changed_objects);\n>\n> handle_tags_and_duplicates(&extra_refs);\n> handle_tags_and_duplicates(&tag_refs);\n> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\n> index 409b48e2442..7b7a18dd2c1 100755\n> --- a/t/t9350-fast-export.sh\n> +++ b/t/t9350-fast-export.sh\n> @@ -750,4 +750,36 @@ test_expect_success 'merge commit gets exported with --import-marks' '\n> )\n> '\n>\n> +\n> +test_expect_success 'fast-export --first-parent outputs all revisions output by revision walk' '\n> + git init first-parent &&\n> + (\n> + cd first-parent &&\n> + test_commit A &&\n> + git checkout -b topic1 &&\n> + test_commit B &&\n> + git checkout main &&\n> + git merge --no-ff topic1 &&\n> +\n> + git checkout -b topic2 &&\n> + test_commit C &&\n> + git checkout main &&\n> + git merge --no-ff topic2 &&\n> +\n> + test_commit D &&\n> +\n> + git fast-export main -- --first-parent >first-parent-export &&\n> + git fast-export main -- --first-parent --reverse >first-parent-reverse-export &&\n> + test_cmp first-parent-export first-parent-reverse-export &&\n> +\n> + git init import &&\n> + git -C import fast-import +\n> + git log --format=\"%ad %s\" --first-parent main >expected &&\n> + git -C import log --format=\"%ad %s\" --all >actual &&\n> + test_cmp expected actual &&\n> + test_line_count = 4 actual\n> + )\n> +'\n> +\n> test_done\n>\n> base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc\n\nThis version looks good to me:\n\nReviewed-by: Elijah Newren \n"},{"id":"444660","messageId":"xmqq5yrhptqk.fsf@gitster.g","threadId":"56964","inReplyTo":"CABPp-BF7Rg3NH3UBnyzD6c1vuFh+FNrNc8eRvUffqmaLppUPkw@mail.gmail.com","subject":"Re: [PATCH v3] fast-export: fix surprising behavior with --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-21T20:50:59Z","receivedAt":"2021-12-21T20:51:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> * Changed commit message to include the detailed description of the\n>> problem area as suggested by Elijah. I went back and forth with my\n>> self about whether the message needs some \"lead in\", but it ended up\n>> getting long without adding much.\n>\n> If it feels weird quoting someone else so extensively and just using\n> their words for the commit message, we've used a \"Commit-message-by:\"\n> trailer in the past; that could be useful here.\n\nI do not mind tweaking the copy I already have with \"Helped-by\"\nmyself, while ...\n\n> This version looks good to me:\n>\n> Reviewed-by: Elijah Newren \n\n... adding this one before merging the result to 'next'.\n\nThanks, both.\n"},{"id":"444756","messageId":"26316444-9fd2-8f69-10a4-2753c5455e53@unity3d.com","threadId":"56964","inReplyTo":"xmqq5yrhptqk.fsf@gitster.g","subject":"Re: [PATCH v3] fast-export: fix surprising behavior with --first-parent","fromName":"William Sprent","fromEmail":"williams@unity3d.com","sentAt":"2021-12-22T08:38:15Z","receivedAt":"2021-12-22T08:38:21Z","isPatch":true,"sender":{"key":"williams@unity3d.com","avatar":"https://avatars.githubusercontent.com/u/77490534?v=4"},"body":"On 21/12/2021 21.50, Junio C Hamano wrote:\n> Elijah Newren writes:\n> \n>>> * Changed commit message to include the detailed description of the\n>>> problem area as suggested by Elijah. I went back and forth with my\n>>> self about whether the message needs some \"lead in\", but it ended up\n>>> getting long without adding much.\n>>\n>> If it feels weird quoting someone else so extensively and just using\n>> their words for the commit message, we've used a \"Commit-message-by:\"\n>> trailer in the past; that could be useful here.\n> \n> I do not mind tweaking the copy I already have with \"Helped-by\"\n> myself, while ...\n> \n>> This version looks good to me:\n>>\n>> Reviewed-by: Elijah Newren \n> \n> ... adding this one before merging the result to 'next'.\n> \n> Thanks, both.\n\nEither way works for me. I'm not too familiar with conventions around \ntrailers. Both seem very reasonable to me.\n\nThanks a bunch for the help. :)\n"}]}