git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] Support copy and rename detection in fast-export.
@ 2008-07-21  8:16 Alexander Gavrilov
  2008-07-21 10:17 ` Johannes Schindelin
  0 siblings, 1 reply; 9+ messages in thread
From: Alexander Gavrilov @ 2008-07-21  8:16 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin

Although it does not matter for Git itself, tools that
export to systems that explicitly track copies and
renames can benefit from such information.

This patch makes fast-export output correct action
logs when -M or -C are enabled.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---

	I'm thinking about Git->SVN conversion, like http://repo.or.cz/w/git2svn.git

	The trouble with this patch is that old versions of fast-export
	accept -M and -C, but produce garbage if they are specified.
	So the only way for the users to ensure that it is supported is
	to check the git version (or directly test it).

	As a somewhat related question, in which order does fast-export
	output the commits, beside topological? In particular, does it order
	commits on parrallel branches (i.e. not topologically dependent) by date?
	
	-- Alexander

 builtin-fast-export.c  |   28 ++++++++++++++++++++++++++--
 t/t9301-fast-export.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 8bea269..3225e8f 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -118,10 +118,27 @@ static void show_filemodify(struct diff_queue_struct *q,
 {
 	int i;
 	for (i = 0; i < q->nr; i++) {
+		struct diff_filespec *ospec = q->queue[i]->one;
 		struct diff_filespec *spec = q->queue[i]->two;
-		if (is_null_sha1(spec->sha1))
+
+		switch (q->queue[i]->status) {
+		case DIFF_STATUS_DELETED:
 			printf("D %s\n", spec->path);
-		else {
+			break;
+
+		case DIFF_STATUS_COPIED:
+		case DIFF_STATUS_RENAMED:
+			printf("%c \"%s\" \"%s\"\n", q->queue[i]->status,
+			       ospec->path, spec->path);
+
+			if (!hashcmp(ospec->sha1, spec->sha1) &&
+			    ospec->mode == spec->mode)
+				break;
+			/* fallthrough */
+
+		case DIFF_STATUS_TYPE_CHANGED:
+		case DIFF_STATUS_MODIFIED:
+		case DIFF_STATUS_ADDED:
 			/*
 			 * Links refer to objects in another repositories;
 			 * output the SHA-1 verbatim.
@@ -134,6 +151,13 @@ static void show_filemodify(struct diff_queue_struct *q,
 				printf("M %06o :%d %s\n", spec->mode,
 				       get_object_mark(object), spec->path);
 			}
+			break;
+
+		default:
+			die("Unexpected comparison status '%c' for %s, %s",
+				q->queue[i]->status,
+				ospec->path ? ospec->path : "none",
+				spec->path ? spec->path : "none");
 		}
 	}
 }
diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh
index f18eec9..bb595b7 100755
--- a/t/t9301-fast-export.sh
+++ b/t/t9301-fast-export.sh
@@ -162,4 +162,50 @@ test_expect_success 'submodule fast-export | fast-import' '
 
 '
 
+export GIT_AUTHOR_NAME='A U Thor'
+export GIT_COMMITTER_NAME='C O Mitter'
+
+test_expect_success 'setup copies' '
+
+	git config --unset i18n.commitencoding &&
+	git checkout -b copy rein &&
+	git mv file file3 &&
+	git commit -m move1 &&
+	test_tick &&
+	cp file2 file4 &&
+	git add file4 &&
+	git mv file2 file5 &&
+	git commit -m copy1 &&
+	test_tick &&
+	cp file3 file6 &&
+	git add file6 &&
+	git commit -m copy2 &&
+	test_tick &&
+	echo more text >> file6 &&
+	echo even more text >> file6 &&
+	git add file6 &&
+	git commit -m modify &&
+	test_tick &&
+	cp file6 file7 &&
+	echo test >> file7 &&
+	git add file7 &&
+	git commit -m copy_modify
+
+'
+
+test_expect_success 'fast-export -C -C | fast-import' '
+
+	ENTRY=$(git rev-parse --verify copy) &&
+	rm -rf new &&
+	mkdir new &&
+	git --git-dir=new/.git init &&
+	git fast-export -C -C --signed-tags=strip --all > output &&
+	grep "^C \"file6\" \"file7\"\$" output &&
+	cat output |
+	(cd new &&
+	 git fast-import &&
+	 test $ENTRY = $(git rev-parse --verify refs/heads/copy))
+
+'
+
 test_done
-- 
1.5.6.3.18.gfe82

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] Support copy and rename detection in fast-export.
  2008-07-21  8:16 [RFC PATCH] Support copy and rename detection in fast-export Alexander Gavrilov
@ 2008-07-21 10:17 ` Johannes Schindelin
  2008-07-26 18:49   ` [PATCH v2] " Alexander Gavrilov
  0 siblings, 1 reply; 9+ messages in thread
From: Johannes Schindelin @ 2008-07-21 10:17 UTC (permalink / raw)
  To: Alexander Gavrilov; +Cc: git

Hi,

On Mon, 21 Jul 2008, Alexander Gavrilov wrote:

> Although it does not matter for Git itself, tools that
> export to systems that explicitly track copies and
> renames can benefit from such information.
> 
> This patch makes fast-export output correct action
> logs when -M or -C are enabled.

I like it.

> 	I'm thinking about Git->SVN conversion, like 
> 	http://repo.or.cz/w/git2svn.git
> 
> 	The trouble with this patch is that old versions of fast-export
> 	accept -M and -C, but produce garbage if they are specified.
> 	So the only way for the users to ensure that it is supported is
> 	to check the git version (or directly test it).

Unfortunately, this is so.

> 	As a somewhat related question, in which order does fast-export 
> 	output the commits, beside topological? In particular, does it order 
> 	commits on parrallel branches (i.e. not topologically dependent) by 
> 	date?

AFAIR it obeys topological order first, and then date order, by default.  
This can be changed by --topo-order.

But before you go and try to support more options with fast-export:  Some 
options do not make sense, such as --reverse, "a...b", --skip, etc.

I do not even think we need to bother about die()ing with such options: 
the user should know what she is doing with fast-export.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] Support copy and rename detection in fast-export.
  2008-07-21 10:17 ` Johannes Schindelin
@ 2008-07-26 18:49   ` Alexander Gavrilov
  2008-07-26 20:21     ` Shawn O. Pearce
  0 siblings, 1 reply; 9+ messages in thread
From: Alexander Gavrilov @ 2008-07-26 18:49 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Junio C Hamano

Although it does not matter for Git itself, tools that
export to systems that explicitly track copies and
renames can benefit from such information.

This patch makes fast-export output correct action
logs when -M or -C are enabled.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---

	Added a note to the fast-export documentation. When this patch
	is merged, it probably should be updated with the exact version.

	-- Alexander 

 Documentation/git-fast-export.txt |    9 +++++++
 builtin-fast-export.c             |   28 +++++++++++++++++++++-
 t/t9301-fast-export.sh            |   46 +++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 332346c..699b69e 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -36,6 +36,15 @@ when encountering a signed tag.  With 'strip', the tags will be made
 unsigned, with 'verbatim', they will be silently exported
 and with 'warn', they will be exported, but you will see a warning.
 
+-M, -C::
+	Perform move and/or copy detection, as described in the
+	linkgit:git-diff[1] manual page, and use it to generate
+	rename and copy commands in the output dump.
++
+Note that these options are always accepted by git-fast-import,
+but before a certain version it silently produced wrong results.
+You should always check the git version before using them.
+
 
 EXAMPLES
 --------
diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 8bea269..3225e8f 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -118,10 +118,27 @@ static void show_filemodify(struct diff_queue_struct *q,
 {
 	int i;
 	for (i = 0; i < q->nr; i++) {
+		struct diff_filespec *ospec = q->queue[i]->one;
 		struct diff_filespec *spec = q->queue[i]->two;
-		if (is_null_sha1(spec->sha1))
+
+		switch (q->queue[i]->status) {
+		case DIFF_STATUS_DELETED:
 			printf("D %s\n", spec->path);
-		else {
+			break;
+
+		case DIFF_STATUS_COPIED:
+		case DIFF_STATUS_RENAMED:
+			printf("%c \"%s\" \"%s\"\n", q->queue[i]->status,
+			       ospec->path, spec->path);
+
+			if (!hashcmp(ospec->sha1, spec->sha1) &&
+			    ospec->mode == spec->mode)
+				break;
+			/* fallthrough */
+
+		case DIFF_STATUS_TYPE_CHANGED:
+		case DIFF_STATUS_MODIFIED:
+		case DIFF_STATUS_ADDED:
 			/*
 			 * Links refer to objects in another repositories;
 			 * output the SHA-1 verbatim.
@@ -134,6 +151,13 @@ static void show_filemodify(struct diff_queue_struct *q,
 				printf("M %06o :%d %s\n", spec->mode,
 				       get_object_mark(object), spec->path);
 			}
+			break;
+
+		default:
+			die("Unexpected comparison status '%c' for %s, %s",
+				q->queue[i]->status,
+				ospec->path ? ospec->path : "none",
+				spec->path ? spec->path : "none");
 		}
 	}
 }
diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh
index f18eec9..bb595b7 100755
--- a/t/t9301-fast-export.sh
+++ b/t/t9301-fast-export.sh
@@ -162,4 +162,50 @@ test_expect_success 'submodule fast-export | fast-import' '
 
 '
 
+export GIT_AUTHOR_NAME='A U Thor'
+export GIT_COMMITTER_NAME='C O Mitter'
+
+test_expect_success 'setup copies' '
+
+	git config --unset i18n.commitencoding &&
+	git checkout -b copy rein &&
+	git mv file file3 &&
+	git commit -m move1 &&
+	test_tick &&
+	cp file2 file4 &&
+	git add file4 &&
+	git mv file2 file5 &&
+	git commit -m copy1 &&
+	test_tick &&
+	cp file3 file6 &&
+	git add file6 &&
+	git commit -m copy2 &&
+	test_tick &&
+	echo more text >> file6 &&
+	echo even more text >> file6 &&
+	git add file6 &&
+	git commit -m modify &&
+	test_tick &&
+	cp file6 file7 &&
+	echo test >> file7 &&
+	git add file7 &&
+	git commit -m copy_modify
+
+'
+
+test_expect_success 'fast-export -C -C | fast-import' '
+
+	ENTRY=$(git rev-parse --verify copy) &&
+	rm -rf new &&
+	mkdir new &&
+	git --git-dir=new/.git init &&
+	git fast-export -C -C --signed-tags=strip --all > output &&
+	grep "^C \"file6\" \"file7\"\$" output &&
+	cat output |
+	(cd new &&
+	 git fast-import &&
+	 test $ENTRY = $(git rev-parse --verify refs/heads/copy))
+
+'
+
 test_done
-- 
1.5.6.3.18.gfe82

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Support copy and rename detection in fast-export.
  2008-07-26 18:49   ` [PATCH v2] " Alexander Gavrilov
@ 2008-07-26 20:21     ` Shawn O. Pearce
  2008-07-26 20:52       ` Alexander Gavrilov
  0 siblings, 1 reply; 9+ messages in thread
From: Shawn O. Pearce @ 2008-07-26 20:21 UTC (permalink / raw)
  To: Alexander Gavrilov; +Cc: Johannes Schindelin, git, Junio C Hamano

Alexander Gavrilov <angavrilov@gmail.com> wrote:
> This patch makes fast-export output correct action
> logs when -M or -C are enabled.
...
> +-M, -C::
> +	Perform move and/or copy detection, as described in the
> +	linkgit:git-diff[1] manual page, and use it to generate
> +	rename and copy commands in the output dump.
> ++
> +Note that these options are always accepted by git-fast-import,
> +but before a certain version it silently produced wrong results.
> +You should always check the git version before using them.

Do you mean to say git-fast-export in the end of the first line of
that last paragraph?

-- 
Shawn.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Support copy and rename detection in fast-export.
  2008-07-26 20:21     ` Shawn O. Pearce
@ 2008-07-26 20:52       ` Alexander Gavrilov
  2008-07-29 16:11         ` Junio C Hamano
  0 siblings, 1 reply; 9+ messages in thread
From: Alexander Gavrilov @ 2008-07-26 20:52 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Johannes Schindelin, git, Junio C Hamano

Although it does not matter for Git itself, tools that
export to systems that explicitly track copies and
renames can benefit from such information.

This patch makes fast-export output correct action
logs when -M or -C are enabled.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---

	On Sunday 27 July 2008 00:21:03 Shawn O. Pearce wrote:
	> Do you mean to say git-fast-export in the end of the first line of
	> that last paragraph?

	Yes, of course. Thank you.

	By the way, I see that http://git.kernel.org/?p=gitk/gitk.git;a=summary
	hasn't been updated for 2 months. Did the main gitk repository move
	to some other place?

	-- Alexander

 Documentation/git-fast-export.txt |    9 +++++++
 builtin-fast-export.c             |   28 +++++++++++++++++++++-
 t/t9301-fast-export.sh            |   46 +++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 332346c..bb2f9a8 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -36,6 +36,15 @@ when encountering a signed tag.  With 'strip', the tags will be made
 unsigned, with 'verbatim', they will be silently exported
 and with 'warn', they will be exported, but you will see a warning.
 
+-M, -C::
+	Perform move and/or copy detection, as described in the
+	linkgit:git-diff[1] manual page, and use it to generate
+	rename and copy commands in the output dump.
++
+Note that these options were always accepted by git-fast-export,
+but before a certain version it silently produced wrong results.
+You should always check the git version before using them.
+
 
 EXAMPLES
 --------
diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 8bea269..3225e8f 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -118,10 +118,27 @@ static void show_filemodify(struct diff_queue_struct *q,
 {
 	int i;
 	for (i = 0; i < q->nr; i++) {
+		struct diff_filespec *ospec = q->queue[i]->one;
 		struct diff_filespec *spec = q->queue[i]->two;
-		if (is_null_sha1(spec->sha1))
+
+		switch (q->queue[i]->status) {
+		case DIFF_STATUS_DELETED:
 			printf("D %s\n", spec->path);
-		else {
+			break;
+
+		case DIFF_STATUS_COPIED:
+		case DIFF_STATUS_RENAMED:
+			printf("%c \"%s\" \"%s\"\n", q->queue[i]->status,
+			       ospec->path, spec->path);
+
+			if (!hashcmp(ospec->sha1, spec->sha1) &&
+			    ospec->mode == spec->mode)
+				break;
+			/* fallthrough */
+
+		case DIFF_STATUS_TYPE_CHANGED:
+		case DIFF_STATUS_MODIFIED:
+		case DIFF_STATUS_ADDED:
 			/*
 			 * Links refer to objects in another repositories;
 			 * output the SHA-1 verbatim.
@@ -134,6 +151,13 @@ static void show_filemodify(struct diff_queue_struct *q,
 				printf("M %06o :%d %s\n", spec->mode,
 				       get_object_mark(object), spec->path);
 			}
+			break;
+
+		default:
+			die("Unexpected comparison status '%c' for %s, %s",
+				q->queue[i]->status,
+				ospec->path ? ospec->path : "none",
+				spec->path ? spec->path : "none");
 		}
 	}
 }
diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh
index f18eec9..bb595b7 100755
--- a/t/t9301-fast-export.sh
+++ b/t/t9301-fast-export.sh
@@ -162,4 +162,50 @@ test_expect_success 'submodule fast-export | fast-import' '
 
 '
 
+export GIT_AUTHOR_NAME='A U Thor'
+export GIT_COMMITTER_NAME='C O Mitter'
+
+test_expect_success 'setup copies' '
+
+	git config --unset i18n.commitencoding &&
+	git checkout -b copy rein &&
+	git mv file file3 &&
+	git commit -m move1 &&
+	test_tick &&
+	cp file2 file4 &&
+	git add file4 &&
+	git mv file2 file5 &&
+	git commit -m copy1 &&
+	test_tick &&
+	cp file3 file6 &&
+	git add file6 &&
+	git commit -m copy2 &&
+	test_tick &&
+	echo more text >> file6 &&
+	echo even more text >> file6 &&
+	git add file6 &&
+	git commit -m modify &&
+	test_tick &&
+	cp file6 file7 &&
+	echo test >> file7 &&
+	git add file7 &&
+	git commit -m copy_modify
+
+'
+
+test_expect_success 'fast-export -C -C | fast-import' '
+
+	ENTRY=$(git rev-parse --verify copy) &&
+	rm -rf new &&
+	mkdir new &&
+	git --git-dir=new/.git init &&
+	git fast-export -C -C --signed-tags=strip --all > output &&
+	grep "^C \"file6\" \"file7\"\$" output &&
+	cat output |
+	(cd new &&
+	 git fast-import &&
+	 test $ENTRY = $(git rev-parse --verify refs/heads/copy))
+
+'
+
 test_done
-- 
1.5.6.3.18.gfe82

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Support copy and rename detection in fast-export.
  2008-07-26 20:52       ` Alexander Gavrilov
@ 2008-07-29 16:11         ` Junio C Hamano
  2008-07-29 16:45           ` Shawn O. Pearce
  2008-07-30  9:10           ` Alexander Gavrilov
  0 siblings, 2 replies; 9+ messages in thread
From: Junio C Hamano @ 2008-07-29 16:11 UTC (permalink / raw)
  To: Alexander Gavrilov; +Cc: Shawn O. Pearce, Johannes Schindelin, git

Alexander Gavrilov <angavrilov@gmail.com> writes:

> Although it does not matter for Git itself, tools that
> export to systems that explicitly track copies and
> renames can benefit from such information.
>
> This patch makes fast-export output correct action
> logs when -M or -C are enabled.
>
> Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
> ---
>
> 	On Sunday 27 July 2008 00:21:03 Shawn O. Pearce wrote:
> 	> Do you mean to say git-fast-export in the end of the first line of
> 	> that last paragraph?
>
> 	Yes, of course. Thank you.

Alexander, Shawn, what is the status of this patch?  Has it been reviewed
sufficiently and is ready for application?

>  Documentation/git-fast-export.txt |    9 +++++++
>  builtin-fast-export.c             |   28 +++++++++++++++++++++-
>  t/t9301-fast-export.sh            |   46 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 81 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
> index 332346c..bb2f9a8 100644
> --- a/Documentation/git-fast-export.txt
> +++ b/Documentation/git-fast-export.txt
> @@ -36,6 +36,15 @@ when encountering a signed tag.  With 'strip', the tags will be made
>  unsigned, with 'verbatim', they will be silently exported
>  and with 'warn', they will be exported, but you will see a warning.
>  
> +-M, -C::
> +	Perform move and/or copy detection, as described in the
> +	linkgit:git-diff[1] manual page, and use it to generate
> +	rename and copy commands in the output dump.

I think it is more fashionable to do what 3240240 (Docs: Use
"-l::\n--long\n" format in OPTIONS sections, 2008-06-08) did these days,
i.e.:

	-M::
        -C::
        	Description...

> ++
> +Note that these options were always accepted by git-fast-export,
> +but before a certain version it silently produced wrong results.
> +You should always check the git version before using them.
> +

I do not quite follow the mention of "before a certain version", but I
think it is talking about the earlier "fast-export" that took any diff
options but did not act differently upon renamed/copied entries.  If that
is the case, I think we can say something like this instead:

	Note that earlier versions of this command did not complain and
	produced incorrect results if you gave these options.

because docs always talk about the current version.  My reading of Dscho's
original 'show_filemodify' suggests me that "wrong results" does not just
mean missing rename/copy information but a renamed old entity did not get
removed correctly, resulting in an incorrect tree in the commit, right?
Maybe we would want to be a bit more explicit about what kind of breakage
you are talking about here.

> diff --git a/builtin-fast-export.c b/builtin-fast-export.c
> index 8bea269..3225e8f 100644
> --- a/builtin-fast-export.c
> +++ b/builtin-fast-export.c
> @@ -118,10 +118,27 @@ static void show_filemodify(struct diff_queue_struct *q,
>  {
>  	int i;
>  	for (i = 0; i < q->nr; i++) {
> +		struct diff_filespec *ospec = q->queue[i]->one;
>  		struct diff_filespec *spec = q->queue[i]->two;
> -		if (is_null_sha1(spec->sha1))
> +
> +		switch (q->queue[i]->status) {
> +		case DIFF_STATUS_DELETED:
>  			printf("D %s\n", spec->path);
> -		else {
> +			break;
> +
> +		case DIFF_STATUS_COPIED:
> +		case DIFF_STATUS_RENAMED:
> +			printf("%c \"%s\" \"%s\"\n", q->queue[i]->status,
> +			       ospec->path, spec->path);
> +
> +			if (!hashcmp(ospec->sha1, spec->sha1) &&
> +			    ospec->mode == spec->mode)
> +				break;
> +			/* fallthrough */

If you see a copied or renamed entry, you emit "this old path to that old
path" first, and then say that same path got modified.  It appears from my
quick glance of fast-import that touching the same path more than once in
a same commit like this sequence does would work fine (it will involve two
calls to tree_content_set() for the same path but that is not something it
has to forbid, and the function doesn't).

> diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh
> index f18eec9..bb595b7 100755
> --- a/t/t9301-fast-export.sh
> +++ b/t/t9301-fast-export.sh
> @@ -162,4 +162,50 @@ test_expect_success 'submodule fast-export | fast-import' '
>  
>  '
>  
> +export GIT_AUTHOR_NAME='A U Thor'
> +export GIT_COMMITTER_NAME='C O Mitter'
> +
> +test_expect_success 'setup copies' '
> +
> +	git config --unset i18n.commitencoding &&

These are somewhat unusual.  Was there any reason for these exports and
config?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Support copy and rename detection in fast-export.
  2008-07-29 16:11         ` Junio C Hamano
@ 2008-07-29 16:45           ` Shawn O. Pearce
  2008-07-30  9:10           ` Alexander Gavrilov
  1 sibling, 0 replies; 9+ messages in thread
From: Shawn O. Pearce @ 2008-07-29 16:45 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Alexander Gavrilov, Johannes Schindelin, git

Junio C Hamano <gitster@pobox.com> wrote:
> Alexander Gavrilov <angavrilov@gmail.com> writes:
> 
> > Although it does not matter for Git itself, tools that
> > export to systems that explicitly track copies and
> > renames can benefit from such information.
> >
> > This patch makes fast-export output correct action
> > logs when -M or -C are enabled.
> >
> > Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
> > ---
> >
> > 	On Sunday 27 July 2008 00:21:03 Shawn O. Pearce wrote:
> > 	> Do you mean to say git-fast-export in the end of the first line of
> > 	> that last paragraph?
> >
> > 	Yes, of course. Thank you.
> 
> Alexander, Shawn, what is the status of this patch?  Has it been reviewed
> sufficiently and is ready for application?

It looked OK to me on the surface, except the one minor remark in
the documentation I noted above.  Other than that I am not very good
with the internal diff machinary so my ACK/NACK on such matters is
worth very little, if anything.
 
-- 
Shawn.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Support copy and rename detection in fast-export.
  2008-07-29 16:11         ` Junio C Hamano
  2008-07-29 16:45           ` Shawn O. Pearce
@ 2008-07-30  9:10           ` Alexander Gavrilov
  2008-07-30 18:35             ` Junio C Hamano
  1 sibling, 1 reply; 9+ messages in thread
From: Alexander Gavrilov @ 2008-07-30  9:10 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Shawn O. Pearce, Johannes Schindelin, git

On Tue, Jul 29, 2008 at 8:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Alexander Gavrilov <angavrilov@gmail.com> writes:
>> ++
>> +Note that these options were always accepted by git-fast-export,
>> +but before a certain version it silently produced wrong results.
>> +You should always check the git version before using them.
>> +
>
> I do not quite follow the mention of "before a certain version", but I
> think it is talking about the earlier "fast-export" that took any diff
> options but did not act differently upon renamed/copied entries.  If that
> is the case, I think we can say something like this instead:
>
>        Note that earlier versions of this command did not complain and
>        produced incorrect results if you gave these options.
>
> because docs always talk about the current version.  My reading of Dscho's
> original 'show_filemodify' suggests me that "wrong results" does not just
> mean missing rename/copy information but a renamed old entity did not get
> removed correctly, resulting in an incorrect tree in the commit, right?
> Maybe we would want to be a bit more explicit about what kind of breakage
> you are talking about here.

Yes, broken renames is what I've been thinking of when I wrote that.

As fast-export is mainly meant to be used by third-party conversion
scripts, which are not bundled together with git, unsuspecting users
might try to run them using an old git version. The main point of my
note is that scripts should always check the version if they want to
use these options. It probably should also specify the exact value to
compare to, e.g:

        Note that before git 1.6 this command did not complain and produced
        incorrect results if you gave these options. Your scripts should always
        check the version before using them.


> If you see a copied or renamed entry, you emit "this old path to that old
> path" first, and then say that same path got modified.  It appears from my
> quick glance of fast-import that touching the same path more than once in
> a same commit like this sequence does would work fine (it will involve two
> calls to tree_content_set() for the same path but that is not something it
> has to forbid, and the function doesn't).

I'm sorry, but I don't quite understand what are you suggesting by
this paragraph. Yes, fast-import understands double modification, and
my test includes a check for this case. Of course, conversion scripts
for dumber targets might need to do one-line lookahead to eliminate
non-100% copies.


>> diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh
>> index f18eec9..bb595b7 100755
>> --- a/t/t9301-fast-export.sh
>> +++ b/t/t9301-fast-export.sh
>> @@ -162,4 +162,50 @@ test_expect_success 'submodule fast-export | fast-import' '
>>
>>  '
>>
>> +export GIT_AUTHOR_NAME='A U Thor'
>> +export GIT_COMMITTER_NAME='C O Mitter'
>> +
>> +test_expect_success 'setup copies' '
>> +
>> +     git config --unset i18n.commitencoding &&
>
> These are somewhat unusual.  Was there any reason for these exports and
> config?
>

t9301-fast-export.sh earlier changes these parameters to test
automatic conversion to utf8. I reset them back before my test, in
order to be able to test the import by comparing SHA-1 (encoding
conversion changes it). There may be a better way to do it, though.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] Support copy and rename detection in fast-export.
  2008-07-30  9:10           ` Alexander Gavrilov
@ 2008-07-30 18:35             ` Junio C Hamano
  0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2008-07-30 18:35 UTC (permalink / raw)
  To: Alexander Gavrilov; +Cc: Shawn O. Pearce, Johannes Schindelin, git

"Alexander Gavrilov" <angavrilov@gmail.com> writes:

>> If you see a copied or renamed entry, you emit "this old path to that old
>> path" first, and then say that same path got modified.  It appears from my
>> quick glance of fast-import that touching the same path more than once in
>> a same commit like this sequence does would work fine (it will involve two
>> calls to tree_content_set() for the same path but that is not something it
>> has to forbid, and the function doesn't).
>
> I'm sorry, but I don't quite understand what are you suggesting by
> this paragraph.

Sorry for the unmarked remark --- this was not a question, suggestion nor
complaint, but just a plain observation.  I should have ended the
paragraph with "--- I think it's all good." to avoid confusion.

>>> +export GIT_AUTHOR_NAME='A U Thor'
>>> +export GIT_COMMITTER_NAME='C O Mitter'
>>> +
>>> +test_expect_success 'setup copies' '
>>> +
>>> +     git config --unset i18n.commitencoding &&
>>
>> These are somewhat unusual.  Was there any reason for these exports and
>> config?
>>
>
> t9301-fast-export.sh earlier changes these parameters to test
> automatic conversion to utf8.

Yeah, I noticed that while looking at wider context, after I sent the
message you are responding to.

Anyway, thanks for the patch.  Applied and pushed out.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2008-07-30 18:36 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-07-21  8:16 [RFC PATCH] Support copy and rename detection in fast-export Alexander Gavrilov
2008-07-21 10:17 ` Johannes Schindelin
2008-07-26 18:49   ` [PATCH v2] " Alexander Gavrilov
2008-07-26 20:21     ` Shawn O. Pearce
2008-07-26 20:52       ` Alexander Gavrilov
2008-07-29 16:11         ` Junio C Hamano
2008-07-29 16:45           ` Shawn O. Pearce
2008-07-30  9:10           ` Alexander Gavrilov
2008-07-30 18:35             ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).