Re: [PATCH] fast-export: fix surprising behavior with --first-parent
flat view
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-11-23 13:17:40
On Tue, Nov 23 2021, William Sprent via GitGitGadget wrote:
From: William Sprent <redacted>
When invoking git-fast-export with the --first-parent flag on a branch
with merges, fast-export would early-out on processing the first merge
on the branch. If combined with --reverse, fast-export would instead
output all single parent commits on the branch.
This commit makes fast-export output the same commits as rev-list
--first-parent, and makes --reverse not have an effect on which commits
are output.
The fix involves removing logic within fast-export which was responsible
for ensuring that parents are processed before their children, which was
what was exiting early due to missing second parents. This is replaced
by setting 'reverse = 1' before revision walking, which, in conjuction
with topo_order, allows for delegating the ordering of commits to
revision.c. The reverse flag is set after parsing rev-list arguments to
avoid having it disabled.
Signed-off-by: William Sprent <redacted>
---
fast-export: fix surprising behavior with --first-parent
Hi,
This is my first time patching git, so I probably need some guidance on
my approach. :)Hi, thanks for your first contribution to git. This is a rather shallow review, a deeper one is much deserved. I notice that you're removing code in builtin/fast-export.c, presumably we have code in revision.c that does the same thing. It would really help a reviewer for you to dig a bit into the relevant commit history and note it in the commit message. I.e. could revision.c always do this, and this was always needless duplication, or at time X it was needed, but as of Y revision.c learned to do this, and callers A, B and C were adjusted, but just not this missed call D? etc.
quoted hunk ↗ jump to hunk
-static int has_unshown_parent(struct commit *commit) -{ - struct commit_list *parent; - - for (parent = commit->parents; parent; parent = parent->next) - if (!(parent->item->object.flags & SHOWN) && - !(parent->item->object.flags & UNINTERESTING)) - return 1; - return 0; -} - struct anonymized_entry { struct hashmap_entry hash; const char *anon;@@ -752,20 +740,6 @@ static char *anonymize_tag(void *data) return strbuf_detach(&out, NULL); } -static void handle_tail(struct object_array *commits, struct rev_info *revs, - struct string_list *paths_of_changed_objects) -{ - struct commit *commit; - while (commits->nr) { - commit = (struct commit *)object_array_pop(commits); - if (has_unshown_parent(commit)) { - /* Queue again, to be handled later */ - add_object_array(&commit->object, NULL, commits); - return; - } - handle_commit(commit, revs, paths_of_changed_objects); - } -}
...
quoted hunk ↗ jump to hunk
static void handle_tag(const char *name, struct tag *tag) {@@ -1185,7 +1159,6 @@ static int parse_opt_anonymize_map(const struct option *opt, int cmd_fast_export(int argc, const char **argv, const char *prefix) { struct rev_info revs; - struct object_array commits = OBJECT_ARRAY_INIT; struct commit *commit; char *export_filename = NULL, *import_filename = NULL,@@ -1281,19 +1254,14 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix) get_tags_and_duplicates(&revs.cmdline); + revs.reverse = 1;
Is the placement of revs.reverse = 1 here important, or could it go earlier after init_revision_sources() when we assign some other values ir revs?
if (prepare_revision_walk(&revs))
die("revision walk setup failed");A light reading of prepare_revision_walk() suggests it could come after, but maybe I'm entirely wrong.
quoted hunk ↗ jump to hunk
revs.diffopt.format_callback = show_filemodify; revs.diffopt.format_callback_data = &paths_of_changed_objects; revs.diffopt.flags.recursive = 1; while ((commit = get_revision(&revs))) { - if (has_unshown_parent(commit)) { - add_object_array(&commit->object, NULL, &commits); - } - else { - handle_commit(commit, &revs, &paths_of_changed_objects); - handle_tail(&commits, &revs, &paths_of_changed_objects); - } + handle_commit(commit, &revs, &paths_of_changed_objects); }
Yay code deletion, good if it works (I didn't check). Since this is just a one-statement while-loop we can also remove its braces now.
quoted hunk ↗ jump to hunk
+test_expect_success 'fast-export --first-parent outputs all revisions output by revision walk' ' + git init first-parent && + cd first-parent &&
Do any such "cd" in a sub-shell:
git init x &&
(
cd x &&
...
)
Otherwise the next test after you is going to run in anotherdirectory.
quoted hunk ↗ jump to hunk
+ test_commit init && + git checkout -b topic1 && + test_commit file2 file2.txt && + git checkout main && + git merge topic1 --no-ff && + + git checkout -b topic2 && + test_commit file3 file3.txt && + git checkout main && + git merge topic2 --no-ff &&
Just a nit. I'd use "test_commit A", "test_commit B" etc. when the filenames etc. aren't important. There's no subsequent reference here, so I assume they're not.
quoted hunk ↗ jump to hunk
+ test_commit branch-head && + + git rev-list --format="%ad%B" --first-parent --topo-order --no-commit-header main > expected &&
nit; >expected, not > expected is the usual style.
quoted hunk ↗ jump to hunk
+ + git fast-export main -- --first-parent > first-parent-export && + git fast-export main -- --first-parent --reverse > first-parent-reverse-export &&
ditto:
+ git init import && cd import &&
ditto earlier "cd" comment.
+ cat ../first-parent-export | git fast-import &&
Instead of "cat x | prog" do "prog <x".
+ git rev-list --format="%ad%B" --topo-order --all --no-commit-header > actual &&
+ test $(git rev-list --all | wc -l) -eq 4 &&
Instead:
git rev-list --all >tmp &&
test_line_count = 4 tmp
(for some value of tmp)
quoted hunk ↗ jump to hunk
+ test_cmp ../expected actual && + test_cmp ../first-parent-export ../first-parent-reverse-export +'
Maybe some of the CD-ing around here wouldu be easier by not doing that
and instead running e.g.:
git -C subdir fast-export >file-not-in-subdir &&
...