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

From: Alexander Gavrilov <angavrilov@gmail.com>
Date: Jul 30, 2008, 09:10 UTC
Message-ID: <bb6f213e0807300210r205846b4qffa9fc71d4aa6533@mail.gmail.com>
In-Reply-To: <7v7ib4hdpu.fsf@gitster.siamese.dyndns.org>

On Tue, Jul 29, 2008 at 8:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

> 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.

Show 6 quoted lines

> 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.

Show 18 quoted lines

>> 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.