[BUG] Symbolic links break "git fast-export"?

Hi folks,

Is my understanding correct, that `git fast-export | git fast-import` should not modify the repository? If yes, then we might have a bug in `git fast-export` if symbolic directory links are removed and converted to a real directory.

Consider this test case:

    # Create test repo
    git init .
    mkdir foo
    echo "foo" >foo/baz
    git add .
    git commit -m "add foo dir"
    ln -s foo bar
    git add .
    git commit -m "add bar dir as link"
    rm bar
    mkdir bar
    echo "bar" >bar/baz
    git add .
    git commit -m "remove link and make bar dir real"

    printf "BEFORE: "
    git rev-parse HEAD

    # Fast export, import ... that should not change anything!
    git fast-export --no-data --all --signed-tags=warn-strip \
        --tag-of-filtered-object=rewrite | git fast-import --force --quiet

    printf "AFTER: "

I would assume that the BEFORE/AFTER hashes match. Unfortunately, with Git 2.22.0 they do no. The problem is this export output I think:

    remove link and make bar dir real
    from :2
    M 100644 5716ca5987cbf97d6bb54920bea6adde242d87e6 bar/baz
    D bar

The new file in the `bar` directory is added to the repo first and afterwards the path `bar` is deleted. I think that deletes the entire directory `bar`?

If you confirm that this is a bug, then I will try to provide a fix.

Thanks, Lars

On Mon, Jun 24, 2019 at 5:05 AM Lars Schneider <larsxschneider@gmail.com> wrote:

My first reaction was, "we regressed on this again?", but it looks like my original fix for directory/file changes only handled one direction. Thus, my commit 060df6242281 ("fast-export: Fix output order of D/F changes", 2010-07-09) probably *caused* this bug. We should probably just sort not based on filename, but on changetype -- send all the deletes to fast-import before we send the modifies.

We should probably also make a corresponding improvement to fast-import; it also makes some attempts to be smart about handling order of modifies and deletes, but misses this case. See commit 253fb5f8897d ("fast-import: Improve robustness when D->F changes provided in wrong order", 2010-07-09). It'd be nice if fast-import could go through the list of changes, apply the deletes first, then the modifies -- although I'm not sure where renames go in the order off the top of my head.

Thanks for flagging this and working on it.

Elijah

On Mon, Jun 24, 2019 at 06:33:38AM -0600, Elijah Newren wrote:

You'd have to split the renames into separate delete/adds, since they can have a circular dependency. E.g. renaming "foo" to "bar" and "bar" to "foo", you must remove "foo" and "bar" both, and then add them back in.

-Peff

@peff: Can you give me a hint how one would perform this circular dependency in a single commit? I try to write a test case for this.

Thank you, Lars

Am 30.06.19 um 15:05 schrieb Lars Schneider:

git mv Makefile foo git mv COPYING Makefile git mv foo COPYING git diff -B HEAD

-- Hannes

On Sun, Jun 30, 2019 at 12:28 PM Johannes Sixt <j6t@kdbg.org> wrote:

Interestingly, fast-export has special code to handle cases like this; possibly due to the understanding of how all filemodify/filedelete/filerename commands take effect immediately (see below for more on that). If I make the above changes in git.git and commit them, then:

$ git diff --name-status -B HEAD~1 HEAD R100 Makefile COPYING R100 COPYING Makefile

BUT:

$ git fast-export -B -M --no-data HEAD~2..HEAD | tail -n 10 commit refs/heads/master mark :7 author Elijah Newren <newren@gmail.com> 1562000065 -0600 committer Elijah Newren <newren@gmail.com> 1562000065 -0600 data 8 Testing from :6 R COPYING Makefile M 100644 8a7e2353520ddd7e0c8074d2b32d0441d97c1597 COPYING

I.e. fast-export breaks the rename and translates it into a modify instead. This comes from here:

        case DIFF_STATUS_COPIED:
        case DIFF_STATUS_RENAMED:
                /*
                 * If a change in the file corresponding to ospec->path
                 * has been observed, we cannot trust its contents
                 * because the diff is calculated based on the prior
                 * contents, not the current contents.  So, declare a
                 * copy or rename only if there was no change observed.
                 */
                if (!string_list_has_string(changed, ospec->path)) {
                        <snipped code for handling rename/copy>
                }
                /* fallthrough */
        case DIFF_STATUS_TYPE_CHANGED:
        case DIFF_STATUS_MODIFIED:
        case DIFF_STATUS_ADDED:

There is a question of whether fast-import should try to handle different exporters that aren't as careful; e.g. if one gets a stream like:

commit refs/heads/master mark :4 author Me My <self@and.eye> 110000000 -0700 committer Me My <self@and.eye> 110000000 -0700 data 11 correction R letters numbers R numbers letters

Should git-fast-import attempt to divine the user's intent to swap these two files (though it's not clear if that is the intent; see below), or would that violate the documented behavior:

           A filerename command takes effect immediately. Once the source
           location has been renamed to the destination any future commands
           applied to the source location will create new files there and not
           impact the destination of the rename.

(I'm pretty sure Shawn would have said the latter; see e.g. https://public-inbox.org/git/20100706193455.GA19476@spearce.org/ and the follow-ups.) I think the view of "immediately taking effect" implies that this is a rename of 'letters' to 'numbers' which deletes 'numbers', and that the subsequent entry just renames the file back, making it an expensive almost no-op; almost because it has the side-effect of deleting the original 'numbers' file. This is certainly what fast-import does right now.

Elijah

060df6242281 is interesting! If I revert the changes in builtin/fast-export.c, then the "t9350:directory becomes symlink" test still passes nowadays.

Plus, my my new test case passes too:

	test_expect_success 'when transforming a symlink to a directory' '
		test_create_repo src &&

	   	mkdir src/foo &&
		echo a_line >src/foo/file.txt &&
		git -C src add foo/file.txt &&
		git -C src commit -m 1st_commit &&

		ln -s src/foo src/bar &&
		git -C src add bar &&
		git -C src commit -m 2nd_commit &&

		rm src/bar &&
	   	mkdir src/bar &&
	   	echo b_line >src/bar/b_file.txt &&
		git -C src add . &&
		git -C src commit -m 3rd_commit &&

		test_create_repo dst &&
		git -C src fast-export --all &&
		git -C src fast-export --all | git -C dst fast-import &&
		git -C src show >expected &&
		git -C dst show >actual &&
		test_cmp expected actual
	'

Do you think it would make sense to revert the qsort change in fast-export? I haven't bisected yet which other change made the qsort change obsolete.

Thanks, Lars

On Sun, Jun 30, 2019 at 8:01 AM Lars Schneider <larsxschneider@gmail.com> wrote:

No need to bisect; it was the other commit I pointed out in my last email, commit 253fb5f8897d ("fast-import: Improve robustness when D->F changes provided in wrong order", 2010-07-09). The original bug was fixed/worked around in two places, because Shawn specifically said the fast-import side being more robust was okay but that fast-export was buggy and needed fixing. See https://public-inbox.org/git/20100706193455.GA19476@spearce.org/ and the thread around there. (It'd be really nice to be able to cc Shawn on this...sigh.) Reverting the fast-export change would be okay if we replaced it with a better change (something like sorting deletes before modifies, though maybe with extra work around renames as discussion elsewhere in this thread is touching on), but if we straight up revert that change it would leave fast-export in violation of the stream format.

Hope that helps, Elijah

On Mon, Jun 24, 2019 at 5:05 AM Lars Schneider <larsxschneider@gmail.com> wrote:

I forgot to respond to this part.  The answer is mostly yes, but actually no:

* fast-export strips any extended commit headers, if any (it only
looks for expected headers and handles those)
* fast-export omits any tags of trees
* fast-export sometimes rewrites tags of tags of commits to tags of
commits (depends on options passed; it either mistakenly does this, or
dies with an error)
* fast-export defaults to re-encoding commits to be in UTF-8, with no
option in git <= 2.22.0 to leave the encoding alone
* annotated tags outside of the refs/tags/ namespace will have their
location mangled
* references that include commit cycles in their history (which can be
created with git-replace(1)) will not be flagged to the user as an
error but will be silently deleted by fast-export as though the branch
or tag contained no interesting files
* fast-export or fast-import may die on some repositories (e.g. if
there are tags of blobs, if there are signed tags and no
--signed-tags= option passed, if a commit has improperly formatted
info such as a timezone with more than four digits (which exist in the
wild, e.g. in the rails repo, and google will find you more), or in
latest git master if the commit has a recorded encoding and no
--reencode option is passed)
* ...and the bug you just found.

It's a valid question whether these are bugs or not; I'd say that some definitely are, but not all. I had to document all of these up at https://github.com/newren/git-filter-repo#inherited-limitations, among one or two others based on the options I default to passing to fast-export.

Hope that helps, Elijah