Fix Regression: attr: Correctly load system attr file (on Windows) by csware · Pull Request #5152 · libgit2/libgit2

csware · 2019-06-29T14:20:34Z

Regression introduced in commit 5452e49 on PR #4967.

I don't know how to provide a test for this.

csware · 2019-07-03T11:35:56Z

@pks-t @ethomson Could you please review (and merge)?

pks-t · 2019-07-04T07:27:09Z

src/attr.c

+		error = preload_attr_file(repo, attr_session, GIT_ATTR_FILE__FROM_FILE,
+					  NULL, path.ptr);
+	if (error != GIT_ENOTFOUND)
+		goto out;


Yup, this indeed did change behaviour. Previous to my commit 5452e49, we would've returned if preload_attr_file returned any value except GIT_ENOTFOUND. Now we only return if there was an error, but will not return if we were successful.

But was previous behaviour really the correct thing to do? The intention of attr_setup is to pre-cache all attribute files that are relevant to the current repository. So we want to load the global attributes file, the repo attributes file as well as attributes files from the working directory and index. Previously, we would've returned as soon as we have successfully preloaded the system attributes file without preloading any of the others, which I think is the wrong thing to do.

You say that the fix here is that preload_attr_file should only be called if error == 0. But this is indeed not true: system_attr_file may only return either a negative value or 0, there is no codepath that returns positive. So with my refactorings, preload_attr_file is still called if and only if error == 0. So the real change you do here is the return on success.

So I'm not yet convinced that the old behaviour is the correct thing to do. Could you please provide a reproducer with the changing behaviour you have encountered? Just want to make up my own mind whether your proposed fix is the correct thing to do or if you were previously relying on libgit2's faulty behaviour.

I already posted a case where this failed: #4967 (comment)

Considering to change a behavior is fine, but it should not be called "Refactoring" and it should also not break things.

Btw in TortoiseGit I also have tests which rely on a working .gitattributes files (e.g., with a filter, cf. https://gitlab.com/tortoisegit/tortoisegit/blob/47a46485f4f9f99b17257e6692e51827fa23a0f4/test/UnitTests/GitTest.cpp#L1382) and this is working correctly with libgit2 even with the early exit here.

In my case w/o the early exit attr_setup fails here: https://github.com/libgit2/libgit2/blob/master/src/attr.c#L340 and returns -1

I just found the reason: git_buf_sets fails in git_repository_item_path if the buffer is not empty. Comming up with a new PR if my tests run w/o errors now.

@pks-t Commit pushed.

Btw. I just checked the coude again and the early exit was fine, because the "other" files are loaded again after the call to attr_setup on line https://github.com/libgit2/libgit2/blob/master/src/attr.c#L515.

I already posted a case where this failed: #4967 (comment)

Oops, sorry, didn't see that one.

Considering to change a behavior is fine, but it should not be called "Refactoring" and it should also not break things.

I fully agree with you. The change in behaviour definitely wasn't intended, even though it seems to me like the right thing to do in hindsight. But as said, I don't yet know if I'm correct about that.

I just found the reason: git_buf_sets fails in git_repository_item_path if the buffer is not empty. Comming up with a new PR if my tests run w/o errors now.

Ah, interesting.

Btw. I just checked the coude again and the early exit was fine, because the "other" files are loaded again after the call to attr_setup on line https://github.com/libgit2/libgit2/blob/master/src/attr.c#L515.

That's... weird. The current code could be improved by quite a lot, if you ask me. Either we do the loading in attr_setup, or if we load the files anyway after attr_setup then we should just inline the one file we were not loading in collect_attr_files, which is the global one. After all, attr_setup is used in this one location, only.

I'll take a look at your changes and try to dig into that code a bit more.

pks-t · 2019-07-04T08:14:29Z

src/attr.c

 				       NULL, git_repository_attr_cache(repo)->cfg_attr_file)) < 0)
 		goto out;

+	git_buf_clear(&path); /* git_repository_item_path expects an empty buffer, becasue it uses git_buf_set */


Yup, this fix definitely looks correct to me. Previously, we would've usually never encountered this code path due to the wrong early exit.

Anyway. I'll have to investigate why we have so much duplicated code in attr_setup and collect_attr_files.

First of all I'd like to have this regression fixed.

Cannot argue with that. Does your proposed change fix the issues you're seeing? If so, then we may just merge this quickly and be done with it. I'm currently writing a testcase that I'd like to include here, shouldn't take me too long.

Anyway, why we have two passes: we need to preload gitattributes files as they may contain macros that are used in other files. So the first pass is only to make macros available, the second pass is to re-parse files to make use of macros. But in fact, we do this the wrong way. gitattributes(5) specifies that macros may only be parsed from either global files, the repo's file in ".git/info/attributes" or the top-level working directory only. It explicitly says that macros defined in subdirectories shall not be honored, but we fail to respect this. I'll create a PR to fix this.

Regression introduced in commit 5452e49 on PR #4967. Signed-off-by: Sven Strickroth <[email protected]>

The function `git_attr_session__init` is currently only initializing setting up the attribute's session key by incrementing the repo-global key by one. Most notably, all other members of the `git_attr_session` struct are not getting initialized at all. So if one is to allocate a session on the stack and then calls `git_attr_session__init`, the session will still not be fully initialized. We have fared just fine with that until now as all users of the function have allocated the session structure as part of bigger structs with `calloc`, and thus its contents have been zero-initialized implicitly already. Fix this by explicitly zeroing out the session to enable allocation of sessions on the stack.

There were no tests that verified that system-level gitattributes files get handled correctly. In fact, we have recently introduced a regression that caused us to abort if there was a system-level gitattributes file available. Add two tests that verify that we're able to handle system-level gitattributes files. The test attr::repo::sysdir_with_session would've failed without the fix to the described regression.

pks-t · 2019-07-04T09:50:10Z

I've added two tests to this PR. attr::repo::sysdir_with_session would've failed without your fix.

pks-t · 2019-07-04T10:14:44Z

Will merge if you give me green light @csware

csware · 2019-07-04T10:16:52Z

Gogogo! ;)

pks-t · 2019-07-04T10:20:10Z

Thanks a lot, @csware!

csware mentioned this pull request Jun 29, 2019

Incomplete commondir support #4967

Merged

4 tasks

pks-t reviewed Jul 4, 2019

View reviewed changes

csware and others added 3 commits July 4, 2019 10:28

attr: Don't fail in attr_setup if there exists a system attributes file

18a6d9f

Regression introduced in commit 5452e49 on PR #4967. Signed-off-by: Sven Strickroth <[email protected]>

pks-t approved these changes Jul 4, 2019

View reviewed changes

pks-t merged commit 5c87b5a into libgit2:master Jul 4, 2019

implausible mentioned this pull request Jul 23, 2019

Bump libgit2 nodegit/nodegit#1705

Merged

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Conversation

csware commented Jun 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csware commented Jul 3, 2019

Uh oh!

pks-t Jul 4, 2019 • 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!

csware Jul 4, 2019 • 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!

csware Jul 4, 2019 • 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!

csware Jul 4, 2019 • 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!

csware Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

csware Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

csware Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

csware Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

pks-t Jul 4, 2019 • 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!

pks-t Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

csware Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

pks-t Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

pks-t commented Jul 4, 2019

Uh oh!

pks-t commented Jul 4, 2019

Uh oh!

csware commented Jul 4, 2019

Uh oh!

pks-t commented Jul 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

csware commented Jun 29, 2019 •

edited

Loading

pks-t Jul 4, 2019 •

edited

Loading

csware Jul 4, 2019 •

edited

Loading

csware Jul 4, 2019 •

edited

Loading

csware Jul 4, 2019 •

edited

Loading

pks-t Jul 4, 2019 •

edited

Loading