RE: [BUG] Git fast-export with import marks file omits merge commits
Hi Martin,
No problem. I was thinking of the peek/pop pattern as well. :) If you don't
mind, can you please go ahead and submit a patch for this? Thanks so much.
Isaac
-Original Message-
From: Martin Ågren [mailto:martin.ag...@gmail.com]
Sent: Friday, April 20, 2018 1:08 AM
To: Junio C Hamano
Cc: Isaac Chou ; Johannes Schindelin
; Jonathan Tan ;
git@vger.kernel.org
Subject: Re: [BUG] Git fast-export with import marks file omits merge commits
On 20 April 2018 at 00:48, Junio C Hamano wrote:
> Isaac Chou writes:
>
>> I inspected the source code (builtin/fast-export.c) for the
>> fast-export issue I encountered, and it looks like the merge commit
>> is discarded too early by the call to object_array_pop() after only
>> one of the two UNSHOWN parents is processed in the method
>> handle_tail(). The poped merge commit still has one UNSHOWN parent,
>> therefore it is not processed and is lost in the output. Can someone
>> advise me on how to submit a code change or bug report in order to
>> get the fix into the code base?
>
> There indeed are some differences between v2.14 and v2.15 around the
> code that returns early when has_unshown_parent() says "yes" [*1*],
> but the decision to return early when the function says "yes" hasn't
> changed between that timeperiod---it dates back to f2dc849e ("Add 'git
> fast-export', the sister of 'git fast-import'", 2007-12-02), i.e. the
> very beginning of the program's life.
>
> I'll CC those who wrote the original and b3e8ca89 ("fast-export: do
> not copy from modified file", 2017-09-20) and 71992039
> ("object_array: add and use `object_array_pop()`", 2017-09-23), which
> are the only two commits that touch the surrounding area during that
> timeperiod, to ask if something jumps at them.
>
> Thanks.
>
>
> [Footnotes]
>
> *1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c'
> reads like so:
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c index
> d412c0a8f3..2fb60d6d48 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> ...
> @@ -630,15 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len)
> return strbuf_detach(&out, len); }
>
> -static void handle_tail(struct object_array *commits, struct rev_info
> *revs)
> +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 *)commits->objects[commits->nr -
> 1].item;
> + commit = (struct commit *)object_array_pop(commits);
> if (has_unshown_parent(commit))
> return;
> - handle_commit(commit, revs);
> - commits->nr--;
> + handle_commit(commit, revs, paths_of_changed_objects);
> }
> }
Indeed. This looks wrong and the guilty person would be me.
If my 71992039 ("object_array: add and use `object_array_pop()`",
2017-09-23) would instead have done something like
s/commits->nr--/(void)object_array_pop(commits)/ it would not have screwed up
as much. It could also use a peek+pop-pattern.
Isaac, are you up for submitting a patch? Just let me know if you encounter any
issues. Otherwise, I can submit a patch shortly since I was the one who dropped
the ball originally.
Thanks for diagnosing this.
Martin
Re: [BUG] Git fast-export with import marks file omits merge commits
On 20 April 2018 at 00:48, Junio C Hamano wrote:
> Isaac Chou writes:
>
>> I inspected the source code (builtin/fast-export.c) for the
>> fast-export issue I encountered, and it looks like the merge
>> commit is discarded too early by the call to object_array_pop()
>> after only one of the two UNSHOWN parents is processed in the
>> method handle_tail(). The poped merge commit still has one
>> UNSHOWN parent, therefore it is not processed and is lost in the
>> output. Can someone advise me on how to submit a code change or
>> bug report in order to get the fix into the code base?
>
> There indeed are some differences between v2.14 and v2.15 around the
> code that returns early when has_unshown_parent() says "yes" [*1*],
> but the decision to return early when the function says "yes" hasn't
> changed between that timeperiod---it dates back to f2dc849e ("Add
> 'git fast-export', the sister of 'git fast-import'", 2007-12-02),
> i.e. the very beginning of the program's life.
>
> I'll CC those who wrote the original and b3e8ca89 ("fast-export: do
> not copy from modified file", 2017-09-20) and 71992039
> ("object_array: add and use `object_array_pop()`", 2017-09-23),
> which are the only two commits that touch the surrounding area
> during that timeperiod, to ask if something jumps at them.
>
> Thanks.
>
>
> [Footnotes]
>
> *1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c'
> reads like so:
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index d412c0a8f3..2fb60d6d48 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> ...
> @@ -630,15 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len)
> return strbuf_detach(&out, len);
> }
>
> -static void handle_tail(struct object_array *commits, struct rev_info *revs)
> +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 *)commits->objects[commits->nr -
> 1].item;
> + commit = (struct commit *)object_array_pop(commits);
> if (has_unshown_parent(commit))
> return;
> - handle_commit(commit, revs);
> - commits->nr--;
> + handle_commit(commit, revs, paths_of_changed_objects);
> }
> }
Indeed. This looks wrong and the guilty person would be me.
If my 71992039 ("object_array: add and use `object_array_pop()`",
2017-09-23) would instead have done something like
s/commits->nr--/(void)object_array_pop(commits)/ it would not have
screwed up as much. It could also use a peek+pop-pattern.
Isaac, are you up for submitting a patch? Just let me know if you
encounter any issues. Otherwise, I can submit a patch shortly since I
was the one who dropped the ball originally.
Thanks for diagnosing this.
Martin
Re: [BUG] Git fast-export with import marks file omits merge commits
Isaac Chou writes:
> I inspected the source code (builtin/fast-export.c) for the
> fast-export issue I encountered, and it looks like the merge
> commit is discarded too early by the call to object_array_pop()
> after only one of the two UNSHOWN parents is processed in the
> method handle_tail(). The poped merge commit still has one
> UNSHOWN parent, therefore it is not processed and is lost in the
> output. Can someone advise me on how to submit a code change or
> bug report in order to get the fix into the code base?
>
> Thanks,
>
> Isaac
>
> -Original Message-
> From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf
> Of Isaac Chou
> Sent: Monday, April 16, 2018 3:58 PM
> To: git@vger.kernel.org
> Subject: Git fast-export with import marks file omits merge commits
>
> Hello,
>
> I came across a change of behavior with Git version 2.15 and later
> where the fast-export command would omit the merge commits. The
> same use case works correctly with Git version 2.14 and older.
> ...
There indeed are some differences between v2.14 and v2.15 around the
code that returns early when has_unshown_parent() says "yes" [*1*],
but the decision to return early when the function says "yes" hasn't
changed between that timeperiod---it dates back to f2dc849e ("Add
'git fast-export', the sister of 'git fast-import'", 2007-12-02),
i.e. the very beginning of the program's life.
I'll CC those who wrote the original and b3e8ca89 ("fast-export: do
not copy from modified file", 2017-09-20) and 71992039
("object_array: add and use `object_array_pop()`", 2017-09-23),
which are the only two commits that touch the surrounding area
during that timeperiod, to ask if something jumps at them.
Thanks.
[Footnotes]
*1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c'
reads like so:
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d412c0a8f3..2fb60d6d48 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
...
@@ -630,15 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len)
return strbuf_detach(&out, len);
}
-static void handle_tail(struct object_array *commits, struct rev_info *revs)
+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 *)commits->objects[commits->nr -
1].item;
+ commit = (struct commit *)object_array_pop(commits);
if (has_unshown_parent(commit))
return;
- handle_commit(commit, revs);
- commits->nr--;
+ handle_commit(commit, revs, paths_of_changed_objects);
}
}
Re: [BUG] Git fast-export with import marks file omits merge commits
Hi Isaac, On Thu, Apr 19, 2018 at 2:46 PM, Isaac Chou wrote: > I inspected the source code (builtin/fast-export.c) for the fast-export issue > I encountered, and it looks like the merge commit is discarded too early by > the call to object_array_pop() after only one of the two UNSHOWN parents is > processed in the method handle_tail(). The poped merge commit still has one > UNSHOWN parent, therefore it is not processed and is lost in the output. Can > someone advise me on how to submit a code change or bug report in order to > get the fix into the code base? Careful now, fast-export was also the location of my first patch. It's easy to get addicted to contributing changes to git. :-) Inside the git.git repository, there are two files that explain the basic process -- Documentation/SubmittingPatches and Documentation/CodingGuidelines. If they don't cover the process well, that's probably a bug itself, but feel free to ask on the list if you still run into questions. Those documents can be slighly overwhelming at first glance, but they've got good information. Elijah
RE: [BUG] Git fast-export with import marks file omits merge commits
I inspected the source code (builtin/fast-export.c) for the fast-export issue I encountered, and it looks like the merge commit is discarded too early by the call to object_array_pop() after only one of the two UNSHOWN parents is processed in the method handle_tail(). The poped merge commit still has one UNSHOWN parent, therefore it is not processed and is lost in the output. Can someone advise me on how to submit a code change or bug report in order to get the fix into the code base? Thanks, Isaac -Original Message- From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf Of Isaac Chou Sent: Monday, April 16, 2018 3:58 PM To: git@vger.kernel.org Subject: Git fast-export with import marks file omits merge commits Hello, I came across a change of behavior with Git version 2.15 and later where the fast-export command would omit the merge commits. The same use case works correctly with Git version 2.14 and older. Here is the detail of the use case: 0> git --version git version 2.16.2.windows.1 1> git init Initialized empty Git repository in c:/./.git/ 2> echo >> file.txt 3> git add file.txt 4> git commit -m "first commit" [master (root-commit) 711d4d5] first commit 1 file changed, 1 insertion(+) create mode 100644 file.txt 5> git checkout -b test Switched to a new branch 'test' 6> echo >> file.txt 7> git add file.txt 8> git commit -m "commit on test branch" [test 76d231c] commit on test branch 1 file changed, 1 insertion(+) 9> git checkout master Switched to branch 'master' 10> echo >> file.txt 11> git add file.txt 12> git commit -m "commit on master branch" [master 61c55fd] commit on master branch 1 file changed, 1 insertion(+) 13> git merge test Auto-merging file.txt CONFLICT (content): Merge conflict in file.txt Automatic merge failed; fix conflicts and then commit the result. 14> notepad file.txt 15> git add file.txt 16> git commit -m "merged with test branch" [master 1442e0e] merged with test branch 17> git log commit 1442e0ee728c831e74550329e39d27d4188b4422 (HEAD -> master) Merge: 61c55fd 76d231c Author: isaac <...> Date: Mon Apr 16 15:08:39 2018 -0400 merged with test branch commit 61c55fdb883fc403e63c91b49bc11bdade62b3e8 Author: isaac <...> Date: Mon Apr 16 15:07:41 2018 -0400 commit on master branch commit 76d231cdb12eb84f45abdebede06a56f912613d3 (test) Author: isaac <...> Date: Mon Apr 16 15:07:07 2018 -0400 commit on test branch commit 711d4d5781df41924421f8629af040e7c91a8d2e Author: isaac <...> Date: Mon Apr 16 15:06:07 2018 -0400 first commit 18> echo :1 711d4d5781df41924421f8629af040e7c91a8d2e > git-marks 19> cat git-marks :1 711d4d5781df41924421f8629af040e7c91a8d2e 20> git fast-export --use-done-feature --import-marks=git-marks refs/heads/master -- feature done blob mark :2 data 12 commit refs/heads/master mark :3 author isaac <...> 1523905627 -0400 committer isaac <...> 1523905627 -0400 data 22 commit on test branch from :1 M 100644 :2 file.txt blob mark :4 data 12 commit refs/heads/master mark :5 author isaac <...> 1523905661 -0400 committer isaac <...> 1523905661 -0400 data 24 commit on master branch from :1 M 100644 :4 file.txt done Thanks, Isaac
Git fast-export with import marks file omits merge commits
Hello, I came across a change of behavior with Git version 2.15 and later where the fast-export command would omit the merge commits. The same use case works correctly with Git version 2.14 and older. Here is the detail of the use case: 0> git --version git version 2.16.2.windows.1 1> git init Initialized empty Git repository in c:/./.git/ 2> echo >> file.txt 3> git add file.txt 4> git commit -m "first commit" [master (root-commit) 711d4d5] first commit 1 file changed, 1 insertion(+) create mode 100644 file.txt 5> git checkout -b test Switched to a new branch 'test' 6> echo >> file.txt 7> git add file.txt 8> git commit -m "commit on test branch" [test 76d231c] commit on test branch 1 file changed, 1 insertion(+) 9> git checkout master Switched to branch 'master' 10> echo >> file.txt 11> git add file.txt 12> git commit -m "commit on master branch" [master 61c55fd] commit on master branch 1 file changed, 1 insertion(+) 13> git merge test Auto-merging file.txt CONFLICT (content): Merge conflict in file.txt Automatic merge failed; fix conflicts and then commit the result. 14> notepad file.txt 15> git add file.txt 16> git commit -m "merged with test branch" [master 1442e0e] merged with test branch 17> git log commit 1442e0ee728c831e74550329e39d27d4188b4422 (HEAD -> master) Merge: 61c55fd 76d231c Author: isaac <...> Date: Mon Apr 16 15:08:39 2018 -0400 merged with test branch commit 61c55fdb883fc403e63c91b49bc11bdade62b3e8 Author: isaac <...> Date: Mon Apr 16 15:07:41 2018 -0400 commit on master branch commit 76d231cdb12eb84f45abdebede06a56f912613d3 (test) Author: isaac <...> Date: Mon Apr 16 15:07:07 2018 -0400 commit on test branch commit 711d4d5781df41924421f8629af040e7c91a8d2e Author: isaac <...> Date: Mon Apr 16 15:06:07 2018 -0400 first commit 18> echo :1 711d4d5781df41924421f8629af040e7c91a8d2e > git-marks 19> cat git-marks :1 711d4d5781df41924421f8629af040e7c91a8d2e 20> git fast-export --use-done-feature --import-marks=git-marks refs/heads/master -- feature done blob mark :2 data 12 commit refs/heads/master mark :3 author isaac <...> 1523905627 -0400 committer isaac <...> 1523905627 -0400 data 22 commit on test branch from :1 M 100644 :2 file.txt blob mark :4 data 12 commit refs/heads/master mark :5 author isaac <...> 1523905661 -0400 committer isaac <...> 1523905661 -0400 data 24 commit on master branch from :1 M 100644 :4 file.txt done Thanks, Isaac
