threads / patch / 18548

patchfast-export: Avoid dropping files from commits

Subject: [PATCH] fast-export: Avoid dropping files from commits

## tl;dr

7 messages between Mar 25, 2009 and Mar 26, 2009. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

newren@gmail.com· Mar 25, 2009, 20:55 UTC · lore
From: Elijah Newren <newren@gmail.com>

When exporting a subset of commits on a branch that do not go back to a root commit (e.g. master~2..master), we still want each exported commit to have the same files in the exported tree as in the original tree.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin-fast-export.c  |    3 ++-
 t/t9301-fast-export.sh |    7 +++++--
 2 files changed, 7 insertions(+), 3 deletions(-)
Show changes to 2 files +7 −3

builtin-fast-export.c, t/t9301-fast-export.sh

diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index fdf4ae9..34a419c 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -221,7 +221,8 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)
 	if (message)
 		message += 2;
 
-	if (commit->parents) {
+	if (commit->parents &&
+	    get_object_mark(&commit->parents->item->object) != 0) {
 		parse_commit(commit->parents->item);
 		diff_tree_sha1(commit->parents->item->tree->object.sha1,
 			       commit->tree->object.sha1, "", &rev->diffopt);
diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh
index 86c3760..b860626 100755
--- a/t/t9301-fast-export.sh
+++ b/t/t9301-fast-export.sh
@@ -8,6 +8,9 @@ test_description='git fast-export'
 
 test_expect_success 'setup' '
 
+	echo break it > file0 &&
+	git add file0 &&
+	test_tick &&
 	echo Wohlauf > file &&
 	git add file &&
 	test_tick &&
@@ -57,8 +60,8 @@ test_expect_success 'fast-export master~2..master' '
 		(cd new &&
 		 git fast-import &&
 		 test $MASTER != $(git rev-parse --verify refs/heads/partial) &&
-		 git diff master..partial &&
-		 git diff master^..partial^ &&
+		 git diff --exit-code master partial &&
+		 git diff --exit-code master^ partial^ &&
 		 test_must_fail git rev-parse partial~2)
 
 '

-- 
1.6.0.6
Johannes Schindelin· Mar 25, 2009, 22:13 UTC · re: newren@gmail.com · lore

Re: [PATCH] fast-export: Avoid dropping files from commits

Hi,
On Wed, 25 Mar 2009, newren@gmail.com wrote:
Show 8 quoted lines
> From: Elijah Newren <newren@gmail.com>
> 
> When exporting a subset of commits on a branch that do not go back to a
> root commit (e.g. master~2..master), we still want each exported commit to
> have the same files in the exported tree as in the original tree.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
Makes sense.
Show 18 quoted lines
>  builtin-fast-export.c  |    3 ++-
>  t/t9301-fast-export.sh |    7 +++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin-fast-export.c b/builtin-fast-export.c
> index fdf4ae9..34a419c 100644
> --- a/builtin-fast-export.c
> +++ b/builtin-fast-export.c
> @@ -221,7 +221,8 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)
>  	if (message)
>  		message += 2;
>  
> -	if (commit->parents) {
> +	if (commit->parents &&
> +	    get_object_mark(&commit->parents->item->object) != 0) {
>  		parse_commit(commit->parents->item);
>  		diff_tree_sha1(commit->parents->item->tree->object.sha1,
>  			       commit->tree->object.sha1, "", &rev->diffopt);
I do not understand that change.
A good explanation in the commit message might help this stupid developer.

Ciao, Dscho

newren@gmail.com· Mar 25, 2009, 23:53 UTC · re: Johannes Schindelin · lore

Re: [PATCH] fast-export: Avoid dropping files from commits

From: Elijah Newren <newren@gmail.com>

When exporting a subset of commits on a branch that do not go back to a root commit (e.g. master~2..master), we still want each exported commit to have the same files in the exported tree as in the original tree.

Previously, when given such a range, we would omit master~2 as a parent of master~1, but we would still diff against master~2 when selecting the list of files to include in master~1. This would result in only files that had changed in the given range showing up in the resulting export. In such cases, we should diff master~1 against the root instead (i.e. use diff_root_tree_sha1 instead of diff_tree_sha1).

There's a special case to consider here: incremental exports (i.e. exports where the --import-marks flag is specified). If master~2 is an imported mark, then we still want to diff master~1 against master~2 when selecting the list of files to include.

We can handle all cases, including the special case, by just checking whether master~2 corresponds to a known object mark when deciding what to diff against.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin-fast-export.c  |    3 ++-
 t/t9301-fast-export.sh |    7 +++++--
 2 files changed, 7 insertions(+), 3 deletions(-)
Show changes to 2 files +7 −3

builtin-fast-export.c, t/t9301-fast-export.sh

diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index fdf4ae9..34a419c 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -221,7 +221,8 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)
 	if (message)
 		message += 2;
 
-	if (commit->parents) {
+	if (commit->parents &&
+	    get_object_mark(&commit->parents->item->object) != 0) {
 		parse_commit(commit->parents->item);
 		diff_tree_sha1(commit->parents->item->tree->object.sha1,
 			       commit->tree->object.sha1, "", &rev->diffopt);
diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh
index 86c3760..b860626 100755
--- a/t/t9301-fast-export.sh
+++ b/t/t9301-fast-export.sh
@@ -8,6 +8,9 @@ test_description='git fast-export'
 
 test_expect_success 'setup' '
 
+	echo break it > file0 &&
+	git add file0 &&
+	test_tick &&
 	echo Wohlauf > file &&
 	git add file &&
 	test_tick &&
@@ -57,8 +60,8 @@ test_expect_success 'fast-export master~2..master' '
 		(cd new &&
 		 git fast-import &&
 		 test $MASTER != $(git rev-parse --verify refs/heads/partial) &&
-		 git diff master..partial &&
-		 git diff master^..partial^ &&
+		 git diff --exit-code master partial &&
+		 git diff --exit-code master^ partial^ &&
 		 test_must_fail git rev-parse partial~2)
 
 '

-- 
1.6.0.6
Elijah Newren· Mar 26, 2009, 00:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH] fast-export: Avoid dropping files from commits

Hi,

On Wed, Mar 25, 2009 at 4:13 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 10 quoted lines
>> -     if (commit->parents) {
>> +     if (commit->parents &&
>> +         get_object_mark(&commit->parents->item->object) != 0) {
>>               parse_commit(commit->parents->item);
>>               diff_tree_sha1(commit->parents->item->tree->object.sha1,
>>                              commit->tree->object.sha1, "", &rev->diffopt);
>
> I do not understand that change.
>
> A good explanation in the commit message might help this stupid developer.

I resent the patch in another email (sorry for the duplication, but I don't trust gmail to preserve patches, and responding inline to comments via git-send-email isn't so great either). Let me know if the explanation is missing anything, is too detailed, or is using incorrect terminology. In two cases I was sufficiently unsure about my wording that I provided extra wording to try to make it clear what I was talking about.

Thanks, Elijah

Johannes Schindelin· Mar 26, 2009, 00:40 UTC · re: Elijah Newren · lore

Re: [PATCH] fast-export: Avoid dropping files from commits

Hi,
On Wed, 25 Mar 2009, Elijah Newren wrote:
Show 21 quoted lines
> On Wed, Mar 25, 2009 at 4:13 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >> -     if (commit->parents) {
> >> +     if (commit->parents &&
> >> +         get_object_mark(&commit->parents->item->object) != 0) {
> >>               parse_commit(commit->parents->item);
> >>               diff_tree_sha1(commit->parents->item->tree->object.sha1,
> >>                              commit->tree->object.sha1, "", &rev->diffopt);
> >
> > I do not understand that change.
> >
> > A good explanation in the commit message might help this stupid 
> > developer.
> 
> I resent the patch in another email (sorry for the duplication, but I 
> don't trust gmail to preserve patches, and responding inline to comments 
> via git-send-email isn't so great either).  Let me know if the 
> explanation is missing anything, is too detailed, or is using incorrect 
> terminology.  In two cases I was sufficiently unsure about my wording 
> that I provided extra wording to try to make it clear what I was talking 
> about.

I am pretty tired, but I still have the impression that I understood it, so yes, I like it.

You might want to skip the != 0, though, as we avoid that in the rest of Git's source code, too.

Thanks, Dscho

Junio C Hamano· Mar 26, 2009, 03:36 UTC · re: Johannes Schindelin · lore

Re: [PATCH] fast-export: Avoid dropping files from commits

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 14 quoted lines
> Hi,
>
> On Wed, 25 Mar 2009, newren@gmail.com wrote:
>
>> From: Elijah Newren <newren@gmail.com>
>> 
>> When exporting a subset of commits on a branch that do not go back to a
>> root commit (e.g. master~2..master), we still want each exported commit to
>> have the same files in the exported tree as in the original tree.
>> 
>> Signed-off-by: Elijah Newren <newren@gmail.com>
>> ---
>
> Makes sense.
Hmm, does it?

Shouldn't an export with a bottom commit be always considered an incremental? Why special case --import-marks?

When you say "I want to export master~2..master", isn't the intention (unstated, because it is too obvious) that follows it "... because I do have master~2 already and I would want to replay the export on top of that state"? If all of master~2, master~1 and master have a file "frotz" with exactly the same contents, I thought you wouldn't even have to have that same contents repeated in the export datastream.

Or am I (again) entirely misunderstanding the intended use case?
Elijah Newren· Mar 26, 2009, 04:02 UTC · re: Junio C Hamano · lore

Re: [PATCH] fast-export: Avoid dropping files from commits

Hi,
On Wed, Mar 25, 2009 at 9:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Hmm, does it?
>
> Shouldn't an export with a bottom commit be always considered an
> incremental?  Why special case --import-marks?
>
> When you say "I want to export master~2..master", isn't the intention
> (unstated, because it is too obvious) that follows it "... because I do
> have master~2 already and I would want to replay the export on top of that
> state"?  If all of master~2, master~1 and master have a file "frotz" with
> exactly the same contents, I thought you wouldn't even have to have that
> same contents repeated in the export datastream.

No, without the --import-marks it doesn't make sense to make it an incremental. Think of it this way: git-fast-export converts all sha1's into integer identifiers (marks). When you export master~2..master, then when fast-export comes across master~1, it notices that there's nothing to convert it's parent sha1sum into since no information was output for master~2 in the export stream. The way around it would be to simply export master~2 anyway, and master~3, and... (An alternate way around it, I guess, would be using the sha1sum in the export stream instead of a mark...but do you have any way of knowing whether the destination repository you import into will have that commit? Using --import-marks seems like a nice way to specify whether you have that information or not.)

It's perhaps a gotcha, as I know I had the same initial assumption even after reading the manpage. Perhaps Documentation/git-fast-export.txt could be made a bit clearer on this point.

> Or am I (again) entirely misunderstanding the intended use case?

Exporting master~2..master without specifying --import-marks is a way of squashing history, essentially. Perhaps you don't like it serving this purpose, but it was intentional; looking at the master~2..master testcase in t/t9301-fast-export.sh, you'll see

   git fast-export master~2..master | sed "s/master/partial/" |
   <snip some other boilerplate> &&  test_must_fail git rev-parse partial~2

← back to recent threads