@matheusbsbs / matheus-bagliano / commits / 726a228dfb

fast-export: fix surprising behavior with --first-parent

The revision traversal machinery typically processes and returns all children before any parent. fast-export needs to operate in the reverse fashion, handling parents before any of their children in order to build up the history starting from the root commit(s). This would be a clear case where we could just use the revision traversal machinery's "reverse" option to achieve this desired affect. However, this wasn't what the code did. It added its own array for queuing. The obvious hand-rolled solution would be to just push all the commits into the array and then traverse afterwards, but it didn't quite do that either. It instead attempted to process anything it could as soon as it could, and once it could, check whether it could process anything that had been queued. As far as I can tell, this was an effort to save a little memory in the case of multiple root commits since it could process some commits before queueing all of them. This involved some helper functions named has_unshown_parent() and handle_tail(). For typical invocations of fast-export, this alternative essentially amounted to a hand-rolled method of reversing the commits -- it was a bunch of work to duplicate the revision traversal machinery's "reverse" option. This hand-rolled reversing mechanism is actually somewhat difficult to reason about. It takes some time to figure out how it ensures in normal cases that it will actually process all traversed commits (rather than just dropping some and not printing anything for them). And it turns out there are some cases where the code does drop commits without handling them, and not even printing an error or warning for the user. Due to the has_unshown_parent() checks, some commits could be left in the array at the end of the "while...get_revision()" loop which would be unprocessed. This could be triggered for example with git fast-export main -- --first-parent or non-sensical traversal rules such as git fast-export main -- --grep=Merge --invert-grep While most traversals that don't include all parents should likely trigger errors in fast-export (or at least require being used in combination with --reference-excluded-parents), the --first-parent traversal is at least reasonable and it'd be nice if it didn't just drop commits. It'd also be nice for future readers of the code to have a simpler "reverse traversal" mechanism. Use the "reverse" option of the revision traversal machinery to achieve both. Even for the non-sensical traversal flags like the --grep one above, this would be an improvement. For example, in that case, the code previously would have silently truncated history to only those commits that do not have an ancestor containing "Merge" in their commit message. After this code change, that case would include all commits without "Merge" in their commit message -- but any commit that previously had a "Merge"-mentioning parent would lose that parent (likely resulting in many new root commits). While the new behavior is still odd, it is at least understandable given that --reference-excluded-parents is not the default. Helped-by: Elijah Newren <newren@gmail.com> Signed-off-by: William Sprent <williams@unity3d.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

William Sprent committed Dec 16, 2021 at 16:23 UTC 726a228dfb19fae8befa0f209436f8fae7919a58

2 files changed +36 -36

builtin/fast-export.c

+4 -36

@@ -107,18 +107,6 @@ static int parse_opt_reencode_mode(const struct option *opt,

107

108 static struct decoration idnums;

109 static uint32_t last_idnum;

110 -

111 -static int has_unshown_parent(struct commit *commit)

112 -{

113 - struct commit_list *parent;

114 -

115 - for (parent = commit->parents; parent; parent = parent->next)

116 - if (!(parent->item->object.flags & SHOWN) &&

117 - !(parent->item->object.flags & UNINTERESTING))

118 - return 1;

119 - return 0;

120 -}

121 -

110 struct anonymized_entry {

111 struct hashmap_entry hash;

112 const char *anon;

@@ -752,20 +740,6 @@ static char *anonymize_tag(void *data)

740 return strbuf_detach(&out, NULL);

741 }

742

755 -static void handle_tail(struct object_array *commits, struct rev_info *revs,

756 - struct string_list *paths_of_changed_objects)

757 -{

758 - struct commit *commit;

759 - while (commits->nr) {

760 - commit = (struct commit *)object_array_pop(commits);

761 - if (has_unshown_parent(commit)) {

762 - /* Queue again, to be handled later */

763 - add_object_array(&commit->object, NULL, commits);

764 - return;

765 - }

766 - handle_commit(commit, revs, paths_of_changed_objects);

767 - }

768 -}

743

744 static void handle_tag(const char *name, struct tag *tag)

745 {

@@ -1185,7 +1159,6 @@ static int parse_opt_anonymize_map(const struct option *opt,

1159 int cmd_fast_export(int argc, const char **argv, const char *prefix)

1160 {

1161 struct rev_info revs;

1188 - struct object_array commits = OBJECT_ARRAY_INIT;

1162 struct commit *commit;

1163 char *export_filename = NULL,

1164 *import_filename = NULL,

@@ -1283,18 +1256,13 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)

1256

1257 if (prepare_revision_walk(&revs))

1258 die("revision walk setup failed");

1259 +

1260 + revs.reverse = 1;

1261 revs.diffopt.format_callback = show_filemodify;

1262 revs.diffopt.format_callback_data = &paths_of_changed_objects;

1263 revs.diffopt.flags.recursive = 1;

1289 - while ((commit = get_revision(&revs))) {

1290 - if (has_unshown_parent(commit)) {

1291 - add_object_array(&commit->object, NULL, &commits);

1292 - }

1293 - else {

1294 - handle_commit(commit, &revs, &paths_of_changed_objects);

1295 - handle_tail(&commits, &revs, &paths_of_changed_objects);

1296 - }

1297 - }

1264 + while ((commit = get_revision(&revs)))

1265 + handle_commit(commit, &revs, &paths_of_changed_objects);

1266

1267 handle_tags_and_duplicates(&extra_refs);

1268 handle_tags_and_duplicates(&tag_refs);

t/t9350-fast-export.sh

+32

@@ -750,4 +750,36 @@ test_expect_success 'merge commit gets exported with --import-marks' '

750 )

751 '

752

753 +

754 +test_expect_success 'fast-export --first-parent outputs all revisions output by revision walk' '

755 + git init first-parent &&

756 + (

757 + cd first-parent &&

758 + test_commit A &&

759 + git checkout -b topic1 &&

760 + test_commit B &&

761 + git checkout main &&

762 + git merge --no-ff topic1 &&

763 +

764 + git checkout -b topic2 &&

765 + test_commit C &&

766 + git checkout main &&

767 + git merge --no-ff topic2 &&

768 +

769 + test_commit D &&

770 +

771 + git fast-export main -- --first-parent >first-parent-export &&

772 + git fast-export main -- --first-parent --reverse >first-parent-reverse-export &&

773 + test_cmp first-parent-export first-parent-reverse-export &&

774 +

775 + git init import &&

776 + git -C import fast-import <first-parent-export &&

777 +

778 + git log --format="%ad %s" --first-parent main >expected &&

779 + git -C import log --format="%ad %s" --all >actual &&

780 + test_cmp expected actual &&

781 + test_line_count = 4 actual

782 + )

783 +'

784 +

785 test_done