frej / fast-export

A mercurial to git converter using git-fast-import
http://repo.or.cz/w/fast-export.git
808 stars 255 forks source link

Consistently terminate commit messages with LF #333

Closed thaliaarchi closed 1 month ago

thaliaarchi commented 2 months ago

When the length logic for fast-import 'data' commands was updated in 4c10270 (Fix data handling, 2023-03-02), one branch was missed, so commit messages now do not have a final LF appended in most cases. This changed the longtime behavior, which had been consistent since the first commit of hg2git, 9832035 (Initial import, 2007-03-06), and is expected by some applications which compare against old conversions from Mercurial[1].

thaliaarchi commented 2 months ago

This changes the expected output of the tests. Note, though, that t/ was created after this bug was introduced, so they have never reflected this behavior. I would have updated them myself here, but tests currently fail on master for reasons I don't understand.

frej commented 2 months ago

This changed the longtime behavior, which had been consistent since the first commit of hg2git

Good catch! That should be fixed, no argument there. But we should have the test cases updated in the same commit to stay bisectable.

I would have updated them myself here, but tests currently fail on master for reasons I don't understand.

I can't reproduce any breakage on master locally. I did #334 to rule out that it was just a fluke, but it's not broken in the CI either. If you are switching between branches, have you done git clean -fxd to be sure that there's not any old files laying around? Running make TEST_OPTS='--debug --verbose' -C t will give you more information. The documentation for the test harness is here.

thaliaarchi commented 2 months ago

Thanks for the pointer about --debug and --verbose. With that, this error is printed, which was hidden before:

Error: The option core.ignoreCase is set to true in the git repository. This will produce empty changesets for renames that just change the case of the file name. Use --force to skip this check or change the option with git config core.ignoreCase false

I have core.ignoreCase=true globally configured, because I unfortunately setup my filesystem as case-insensitive long ago. The problem is simply that the tests don't override this. Rather than setting it in the tests' repos as I have done in my own usage of hg-fast-export, I've pushed a patch to not rely on the user's configuration and instead run git fast-import with ignoreCase disabled inline. Now users have one less error to worry about. I don't think there's a single use case for wanting path collisions with hg-fast-export, that wouldn't be more correctly solved with a git-filter-repo pass afterwards.

I'm surprised that git fast-import has this issue in the first place, since I'm familiar with the code from contributing patches to it, but, sure enough, it uses variably case-sensitive path comparisons:

int fspathncmp(const char *a, const char *b, size_t count)
{
    return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
}
thaliaarchi commented 2 months ago

Should I have left the core.ignoreCase change out of this PR? Tests pass for me locally with those changes and should remain working with CI (though I think you need to approve the workflow, for some reason, for the tests to run).

thaliaarchi commented 1 month ago

Thanks for the review. I've reverted the changes to core.ignoreCase behavior and updated only tests.

frej commented 1 month ago

Thank you for your contribution @thaliaarchi.