libgit2 does not work the same way as vanilla git does for adding files to index by csware · Pull Request #3285 · libgit2/libgit2

csware · 2015-07-01T16:57:26Z

This is based on issue #2798 and issue #3199.

The code of issue #2798 is rebased onto current master and uses a similar testing approach as issue #3199.
Hopefully there are better chances for getting this merged than issue #2798.

The files in "checkin_input_files" are directly based on the files used for issue #3199.

/cc @ethomson

libgit2-0001 to libgit2-0005 based on libgit2/libgit2#3285, however, w/o tests here. Signed-off-by: Sven Strickroth <email@cs-ware.de>

csware · 2015-09-29T08:53:47Z

This is an open issue for a very long time? Any comments or don't you care for broken CRLF?

carlosmn · 2015-09-30T08:40:09Z

Why are we accessing git's private data? If we need the repository, we should store it like we do in every other case (including tests/generate_crlf.sh and use .gitted/ as the git-dir. Looking into this structure is very fragile.

In my first attempt all the private data was hardcoded, then it was requested that a script should be used for generating the "known good test results". I'm accessing the private data so that I can be sure which objects (ids) were created on adding a file (after the CRLF handling).

If you have a better idea here, I'm open.

But you don't want the private data, you want to see which objects were created. Ask git for it, don't take it out of .git/.

git ls-files -s | cut -d ' ' -f 2

will give you the object names of whatever is in the index. If you're sure there's only ever one, that will give you the answer already. If you want to clear the index, use

git read-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904

where that's the empty tree. You can generate it yourself at the beginning of the script with

git hash-object -t tree --stdin </dev/null

if you'd rather not hard-code it.

@carlosmn Thanks, updated.

carlosmn · 2015-09-30T09:02:15Z

In general, the commits are missing an explanation for why the change is necessary. They don't explain why the old version was wrong or why the new version of the code fixes it. They just claim that they fix something. This makes it much harder to go through the commits and make sense of the changes. It would also help if they mentioned which failing tests they are fixing.

csware · 2015-09-30T13:59:52Z

LF -> CRLF filter now correctly honors core.safecrlf=true errors
- LF only files were accepted with core.autocrlf=true on CRLF platforms
- files containig CRLF in combination with core.autocrlf=input were accepted
- adding files containing CR and CRLF but not the same number failed
LF -> CRLF filter now handles files on adding to index the way vanilla git does
- skip mangling of files marked as "binary" (-crlf)
- especially files containing single CR chars are added as is now
- honor "text" attribute for forcing a file being interpreted as text

csware · 2016-02-22T21:32:35Z

/cc @ethomson @carlosmn
Any news on this PR? AutoCrLf i still broken in libgit2... Btw. I'm using this code for several months in TortoiseGit (also, i don't know why the appveyor tests show failures, local tests run w/o problems)

johnhaley81 · 2016-02-22T21:49:39Z

👍 auto-crlf affects us a lot over in GitKraken land as well.

csware · 2017-01-29T01:32:27Z

This PR is now updated to match with the changed CrLf filter logic of Git 2.9.

csware · 2017-01-29T02:29:46Z

I'm sorry that the tests are failing right now as this is dependant on PR #4098. When this is on top of PR #4098 all the tests succeed w/o errors:

Rebased commit: bcf497d
AppVeyor: https://ci.appveyor.com/project/libgit2/libgit2/build/3139
Travis CI: https://travis-ci.org/libgit2/libgit2/builds/196256027

novalis

Also, github makes this unreviewable because of the 3,000 file limit. I do not think that having 5,492 files to test crlf manipulation is reasonable. Nobody could possibly review these files to see if they are correct. I understand that you are starting with 1,369 and that the initial situation is not your fault. But nonetheless, I think that the moment that we bust github's ability to display code is a good moment to sit back and reflect on whether the approach makes any sense.

These files are generated by machine. Why don't we let the machine generate them and not check them in?

novalis · 2018-03-16T22:07:22Z

+		(ca->crlf_action == GIT_CRLF_GUESS || ca->crlf_action == GIT_CRLF_AUTO || ca->crlf_action == GIT_CRLF_TEXT) && ca->eol == GIT_EOL_LF ||
+		(ca->auto_crlf == GIT_AUTO_CRLF_INPUT && (ca->crlf_action == GIT_CRLF_GUESS)) ||
+		(ca->auto_crlf == GIT_AUTO_CRLF_INPUT && (ca->crlf_action == GIT_CRLF_AUTO || ca->crlf_action == GIT_CRLF_TEXT) && ca->eol == GIT_EOL_UNSET) ||
+		(GIT_EOL_NATIVE != GIT_EOL_CRLF && ca->auto_crlf == GIT_AUTO_CRLF_FALSE && (ca->crlf_action == GIT_CRLF_AUTO || ca->crlf_action == GIT_CRLF_TEXT) && ca->eol == GIT_EOL_UNSET)) {


This seems overcomplicated to me. For one thing, you sometimes have the action test before the auto_crlf test and sometimes after. This means that you miss some commonalities like the duplicated ca->crlf_action == auto || ca->crlf_action == text. We could also make it easier to read by breaking out sa->action, sa->eol, and sa->auto_crlf into separate variables. And we could define a helper GIT_EOF_NATIVE_IS_LF = GIT_EOL_NATIVE != GIT_EOL_CRLF, just to make that bit a bit shorter and clearer. I made these changes and I was still unable to make heads or tails of it. So then I thought: what if we make a function?

static int is_crlf_reasonable(sa) { switch (sa->crlf_action) { case GIT_CRLF_INPUT: return 1; break; case GIT_CRLF_GUESS: return sa->eol == GIT_EOL_LF || sa->auto_crlf == GIT_AUTO_CRLF_INPUT; case GIT_CRLF_AUTO: /* FALL-THROUGH */ case GIT_CRLF_TEXT: switch (sa->eol) { case GIT_EOL_LF: return 1; case GIT_EOL_UNSET: return sa->auto_crlf == GIT_AUTO_CRLF_INPUT; case GIT_EOL_UNSET: return GIT_EOL_NATIVE_IS_LF && sa->auto_crlf == GIT_AUTO_CRLF_FALSE; } } return 0; }

I have not yet actually tested this to ensure that it is identical, but I think the overall approach is much easier to read. Plus it's clear from the function name that success means that the check passed. Perhaps we need a better function name -- I don't really understand what this code is doing, but I can tell that I would be unable to debug it.

@novalis

These files are generated by machine. Why don't we let the machine generate them and not check them in?

In a previous commit (4ebe320) @ethomson established this test approach for checking out files, I just adjusted this approach for checking files into the index.

novalis · 2018-03-16T22:17:38Z

 	cl_git_pass(git_index_clear(g_index));

-	git_buf_joinpath(&expected_dirname, "crlf_data", "checkin_results");
+	git_buf_joinpath(&expected_dirname, "crlf_data/checkin_results", systype);


If you're doing joinpath anyway, don't use a slash. add another joinpath.

novalis · 2018-03-16T22:24:28Z

+	if (ca->crlf_action == GIT_CRLF_INPUT ||
+		(ca->auto_crlf == GIT_AUTO_CRLF_INPUT &&
+		(ca->crlf_action == GIT_CRLF_GUESS || ca->crlf_action == GIT_CRLF_AUTO ||
+		(ca->crlf_action == GIT_CRLF_TEXT && ca->eol==GIT_EOL_UNSET)))) {


nit: missing spaces around the last ==.

But more importantly, this is another case of "sanity-checking" that doesn't make clear what exactly is being checked and which is an if statement with a thousand clauses. This one is somewhat less heinous than the other one, but it does seem to cover a lot of the same ground, so I wonder if you missed some cases here, or if we could extract the common logic between the two, or something.

csware · 2018-04-17T20:50:50Z

@novalis @pks-t @ethomson I refactored this (again and rebased), so hopefully the code is better readable now.

novalis · 2018-04-18T04:17:27Z

Thanks, that refactor looks much cleaner.

ethomson · 2018-04-18T08:16:06Z

Thanks for the work on this @csware - and thanks much for the review @novalis. I'll take a 👀 as soon as I have a little extra time. Sorry it's taken so long already, I hope to get to this promptly.

ethomson · 2018-04-18T08:19:47Z

@novalis asked:

These files are generated by machine. Why don't we let the machine generate them and not check them in?

I agree that this is unfortunate, but we don't want to add dependencies to the build or test process. We would need a git installation - the actual git itself that we're trying to emulate - in order to test against it. That means that it needs to be the latest git, since its CR/LF handling changes from time-to-time. It's pretty hard to rely on people having that.

I don't see a path to success that doesn't involve us checking in the test resources.

pks-t

I've just had a very quick go at this PR. Are you still interested in maintaining it? If not that's fine, it's our fault for not reviewing it sooner. In that case I'll adopt it and move it forward.

pks-t · 2018-08-31T08:48:54Z

+	git_oid oid;
+	git_error_state error = { 0 };
+
+	basename = git_path_basename(actual_path->ptr);


This may fail due to OOM conditions, so we have to check its return value

pks-t · 2018-08-31T08:50:35Z

+	cmp_git = strcmp(basename, ".git");
+	cmp_gitattributes = strcmp(basename, ".gitattributes");
+
+	if (cmp_git == 0 || cmp_gitattributes == 0) {


Stylistic nit: as I can't see these values being used anywhere else, I'd just avoid the use of variables and simply inline the two strcmp calls

I just copied the coe from checkout/cflf.c: compare_file, there are these two variables and also no OOM check

pks-t · 2018-08-31T08:51:18Z

+	cl_git_pass(git_buf_joinpath(&expected_path_oid, cd->dirname, basename));
+	cl_git_pass(git_buf_joinpath(&expected_path_fail, cd->dirname, basename));
+	git_buf_puts(&expected_path_oid, ".obj");
+	git_buf_puts(&expected_path_fail, ".fail");


These should be wrapped in cl_git_pass, as well

pks-t · 2018-08-31T08:53:08Z

+	git_buf_puts(&sandboxname, safecrlf);
+
+	git_buf_puts(&sandboxname, ",autocrlf_");
+	git_buf_puts(&sandboxname, autocrlf);


All these buffer operations need to be checked via cl_git_pass

pks-t · 2018-08-31T08:53:18Z

+	cl_git_pass(git_index_clear(g_index));
+
+	git_buf_joinpath(&expected_dirname, "crlf_data", "checkin_results");
+	git_buf_joinpath(&expected_fixture, expected_dirname.ptr, sandboxname.ptr);


And these as well

csware · 2018-08-31T11:54:41Z

@pks-t Updated.

Include a shell script that will generate the expected data of OIDs and failures for calling git.git to capture its output as a test resource. Right now, there is no need to differentiate different systems as git behaves the same on all systems IIRC. Signed-off-by: Sven Strickroth <email@cs-ware.de>

A corpus of files containing the OIDs after adding files to index or errors based on SafeCRLF settings with Git for Windows (2.4.4.windows.1) to ensure that we produce identical data when adding to index using a CRLF filter. Signed-off-by: Sven Strickroth <email@cs-ware.de>

Given a variety of combinations of core.autocrlf, core.safecrlf settings and attributes settings, test that we add files to index the same way (regarding OIDs and fatal errors) as a known-good test resource created by git.git. Signed-off-by: Sven Strickroth <email@cs-ware.de>

…e as vanilla git does Reported-by: Yue Lin Ho <b8732003@student.nsysu.edu.tw> Signed-off-by: Sven Strickroth <email@cs-ware.de>

… file exists and matches Knowing the proper core.autocrlf value is need for deciding whether the CRLF filter needs to be executed... Signed-off-by: Sven Strickroth <email@cs-ware.de>

…es not fail as with vanilla git Signed-off-by: Sven Strickroth <email@cs-ware.de>

Signed-off-by: Sven Strickroth <email@cs-ware.de>

…ion with attributes Based on lots of manual testing (even with "malformed" files and binary) with vanilla git executable... Signed-off-by: Sven Strickroth <email@cs-ware.de>

Update with "git version 2.11.0.windows.3". Signed-off-by: Sven Strickroth <email@cs-ware.de>

Update with vanilla git version 2.11.0 on Debian. Signed-off-by: Sven Strickroth <email@cs-ware.de>

The logic differs on *nix and Windows now. Signed-off-by: Sven Strickroth <email@cs-ware.de>

Signed-off-by: Sven Strickroth <email@cs-ware.de>

csware · 2018-10-05T09:53:02Z

@pks-t I addressed all points. Any chance to get a new review?

pks-t · 2018-10-05T11:08:02Z

I already had a look a few days ago and didn't find anything noteworthy. I didn't yet state that as I am still thinking about these 4000 new files, which honestly put me a bit off. But I still ain't got a better idea how to properly do this, at least nothing that's as comprehensive as what you propose.

@ethomson, @carlosmn: I'd appreciate if you could have another look at this

ethomson · 2018-12-02T03:41:47Z

I’ve been looking at this as time allows. I have some changes to propose but in the meantime I appreciate all the hard work here.

csware · 2019-01-03T12:58:07Z

It took long, but finally fixed with issue #4904.

csware added a commit to TortoiseGit/TortoiseGit that referenced this pull request Jul 2, 2015

Update libgit2 patches

5af3213

libgit2-0001 to libgit2-0005 based on libgit2/libgit2#3285, however, w/o tests here. Signed-off-by: Sven Strickroth <email@cs-ware.de>

carlosmn reviewed Sep 30, 2015
View reviewed changes

csware changed the title ~~libgit2 does not work the same way as vanilla git does for adding files to index~~ WIP: libgit2 does not work the same way as vanilla git does for adding files to index Jan 28, 2017

csware changed the title ~~WIP: libgit2 does not work the same way as vanilla git does for adding files to index~~ libgit2 does not work the same way as vanilla git does for adding files to index Jan 29, 2017

csware mentioned this pull request Jan 29, 2017

Update index_to_workdir CRLF conversion to match Git >= 2.9 logic #4098

Merged

tboby mentioned this pull request Feb 9, 2018

CRLF in repository and * text = auto results in incorrect modified state #4526

Closed

novalis suggested changes Mar 16, 2018

View reviewed changes

pks-t reviewed Aug 31, 2018

View reviewed changes

csware added 9 commits September 15, 2018 10:19

core.autocrlf=true and core.safecrlf=true did not fail on LF-only fil…

0349d71

…e as vanilla git does Reported-by: Yue Lin Ho <b8732003@student.nsysu.edu.tw> Signed-off-by: Sven Strickroth <email@cs-ware.de>

core.autocrlf is not correctly initialized on Win32 if .gitattributes…

0c3e960

… file exists and matches Knowing the proper core.autocrlf value is need for deciding whether the CRLF filter needs to be executed... Signed-off-by: Sven Strickroth <email@cs-ware.de>

Adding a CRLF-file with core.autocrlf=input and core.safecrlf=true do…

56d2fe0

…es not fail as with vanilla git Signed-off-by: Sven Strickroth <email@cs-ware.de>

Make files with #CR!=#CRLF not fail with core.safecrlf=true

407addf

Signed-off-by: Sven Strickroth <email@cs-ware.de>

Make CRLF filter behave like vanilla git for adding files in combinat…

8653cb1

…ion with attributes Based on lots of manual testing (even with "malformed" files and binary) with vanilla git executable... Signed-off-by: Sven Strickroth <email@cs-ware.de>

tests: crlf: update Windows test data to reflect Git 2.9+ behavior

8cde415

Update with "git version 2.11.0.windows.3". Signed-off-by: Sven Strickroth <email@cs-ware.de>

csware added 4 commits September 15, 2018 10:21

tests: crlf: update POSIX test data to reflect Git 2.9+ behavior

38eabd1

Update with vanilla git version 2.11.0 on Debian. Signed-off-by: Sven Strickroth <email@cs-ware.de>

crlf: update add to index logic to reflect Git 2.9+ behavior

35e50e0

The logic differs on *nix and Windows now. Signed-off-by: Sven Strickroth <email@cs-ware.de>

crlf: Refactor if-conditions to make them better readable

86b78a3

Signed-off-by: Sven Strickroth <email@cs-ware.de>

Optimize tests

9abc15f

Signed-off-by: Sven Strickroth <email@cs-ware.de>

ethomson mentioned this pull request Dec 3, 2018

Update CRLF filtering to match modern git #4904

Merged

csware closed this Jan 3, 2019

csware deleted the another-crlf-approach branch January 3, 2019 12:58

Uh oh!

Conversation

csware commented Jul 1, 2015

Uh oh!

csware commented Sep 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosmn commented Sep 30, 2015

Uh oh!

csware commented Sep 30, 2015

Uh oh!

csware commented Feb 22, 2016

Uh oh!

johnhaley81 commented Feb 22, 2016

Uh oh!

csware commented Jan 29, 2017

Uh oh!

csware commented Jan 29, 2017

Uh oh!

novalis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csware commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

novalis commented Apr 18, 2018

Uh oh!

ethomson commented Apr 18, 2018

Uh oh!

ethomson commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csware Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csware commented Aug 31, 2018

Uh oh!

csware commented Oct 5, 2018

Uh oh!

pks-t commented Oct 5, 2018

Uh oh!

ethomson commented Dec 2, 2018

Uh oh!

csware commented Jan 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

csware commented Apr 17, 2018 •

edited

Loading

ethomson commented Apr 18, 2018 •

edited

Loading

csware Aug 31, 2018 •

edited

Loading