Re: [PATCH v2] Support copy and rename detection in fast-export.

[Junio C Hamano <gitster@pobox.com>]

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?