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

From: Elijah Newren <newren@gmail.com>
Date: Jul 1, 2019, 18:02 UTC
Message-ID: <CABPp-BERu_Sd6YYtVPBrjuHz_Yc0_f3rgvYESQB8EuUs-8jCUQ@mail.gmail.com>
In-Reply-To: <3AA3CC52-84FE-4FD0-8977-D4FBCF0DCE2C@gmail.com>

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

Show 54 quoted lines

> > On Jun 24, 2019, at 5:33 AM, Elijah Newren <newren@gmail.com> wrote:
> >
> > On Mon, Jun 24, 2019 at 5:05 AM Lars Schneider <larsxschneider@gmail.com> wrote:
> >>
> >> 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.
> >>
> >> ...
> >
> > 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.
>
> 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.

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