Incomplete commondir support by tiennou · Pull Request #4967 · libgit2/libgit2

tiennou · 2019-01-30T15:00:06Z

This comes from #4671, but I'd like to discuss the problem because I think it's a shortcoming of our "item locator" (and/or repository initialization), thus my initial fix is only papering over the underlying issue.

What happens is that you can (easily) get yourself a repository (using git_repository_new) that has an unset commondir member, which would cause failures in the locator, as it has no fallback mechanism if this member is not set (it reports GIT_ENOTFOUND).

AFAICS there's a bunch of things we can do:

add a fallback system to the locator.
As per gitrepository-layout, $GIT_DIR/info is shadowed by $GIT_COMMON_DIR/info if it's set (set meaning envvar, or a $GIT_DIR/commondir "gitlink".
add some API to change the commondir.
Right now there's no way to do this, it's initialized on new or open, and set in stone at that time. I'm not sure if that's really needed, but to me it seems like some kind of shortcoming (since we allow swapping out ODBs and things, even if it's kinda crazy).
make the relevant users of the item locator ignore issues.
This is the approach I started here, before I took a look at the layout document and noticed the discrepancy. The layout doc seems to imply that missing those directories is fine, thus we're just being overzealous.
introduce some layout-validity tests

@pks-t I'd like your opinion on these points before tackling them, as my gut feeling is that it's some kind of regression introduced by the worktree support (at least users that use git_repository_new now have an unfixable half-working repository, as per #4671).

Helpful audit regex:

/GIT_REPOSITORY_ITEM_(OBJECTS|(PACKED_)REFS|REMOTES|CONFIG|INFO|HOOKS|LOGS|WORKTREE)/

pks-t · 2019-02-20T11:29:42Z

Oops, should've checked whether the #4671 is being referenced already by a PR. I've started running down the rabbit hole with our attrs code to fix this exact issue. Anyway, I'll upload it later and then we can see how to proceed -- at least we ran into the different directions after doing the obvious immediate fix to the attrs code :)

As per gitrepository-layout, $GIT_DIR/info is shadowed by $GIT_COMMON_DIR/info if it's set (set meaning envvar, or a $GIT_DIR/commondir "gitlink".

Well, right now our support for GIT_COMMON_DIR environment variable doesn't exist at all:

	error = git__getenv(&work_tree_buf, "GIT_COMMON_DIR");
	if (error == GIT_ENOTFOUND)
		git_error_clear();
	else if (error < 0)
		goto error;
	else {
		git_error_set(GIT_ERROR_INVALID, "GIT_COMMON_DIR unimplemented");
		error = GIT_ERROR;
		goto error;
	}

So first step here would be to actually implement it at all, and then afterwards we may implement the fallback mechanism. I don't remember right now whether this was part of #4856 or not.

add some API to change the commondir.
Right now there's no way to do this, it's initialized on new or open, and set in stone at that time. I'm not sure if that's really needed, but to me it seems like some kind of shortcoming (since we allow swapping out ODBs and things, even if it's kinda crazy).

Personally, I'd wait until we see somebody requesting it. Until now, nobody has complained about our lacking API here. I just fear that as soon as we do that, people try to change those variables half-way through the repo's lifetime, which may lead to inconsistencies.

That being said, I could easily see us adding an extended API to create repos. As you say, git_repository_new doesn't help too much. It might make sense to create a new API here like e.g. git_repository_new_with_opts (name is subject to change), where you can pass additional information. This would be stuff like commondir, gitdir, ODB, refdb, etc.

make the relevant users of the item locator ignore issues.
This is the approach I started here, before I took a look at the layout document and noticed the discrepancy. The layout doc seems to imply that missing those directories is fine, thus we're just being overzealous.

Yup, definitely agreed. I've also fixed this in a local branch already.

introduce some layout-validity tests

That'd be rad. Git has grown increasingly complex over time, making the precedence and lookup rules non-trivial. So I'd be very eager to see some tests, even though I can imagine it being quite hard for some parts of libgit2.

ethomson · 2019-02-20T11:50:28Z

Personally, I'd wait until we see somebody requesting it.

Strongly agree here, I think that there's vast opportunity to increase our complexity and potentially do this "wrong" from the standpoint of a consumer if we eagerly try to implement it without a use case.

As a concrete example of the existing complexity: I was just creating an inmemory repository over the weekend and got caught up in a few issues that are not obvious and make assumptions about gitdir/commondir existing. (I'll open up a PR when I have a moment).

tiennou · 2019-02-20T15:03:39Z

or a $GIT_DIR/commondir "gitlink".

Note that I was referring to this kinda-gitlink way, which we're already supporting : https://github.com/libgit2/libgit2/blob/master/src/repository.c#L193

I'm completely okay with not adding custom commondir support, because it might not make any sense. @pks-t suggestion of finding an API to provide those attributes is arguably more in-memory-ish. I was going the way that creating the file would make it pick it up, but it would act sanely if it's missing.

pks-t · 2019-02-22T11:23:58Z

I was going the way that creating the file would make it pick it up, but it would act sanely if it's missing.

That's definitely the right thing to do and is completely independent of improved commondir or in-memory attributes support. It's also the same thing that I've been doing in #4995 (which crossed work, as I initially didn't spot this PR as fixing the same issue due to the different title). I'm mighty annoyed though that it's impossible to write a test without either in-memory gitattributes or commondir mangling. But yeah, that's the way it is.

tiennou · 2019-02-22T16:38:49Z

I've pushed my current WIPs on top of that branch, which should have a working commondir => gitdir override if there's no commondir. I'm not sure I understand what is wrong inside the attribute loading, so I'd like to defer more in-depth changes to your other PR and keep the commondir/git_repository_new fixes (for limited functionality).

tiennou

Rebased, I've pulled in #4995 for DRY reasons (apart for its failing test, which I'm tracking a rebase of in my fork), but kept piling on the new issue. I've commented on the WIP parts I'd like some insight on.

Note that I'm refraining from adding a git_repository_new(&repo, path), or an _opts variant, because all the "low-level" setters are already present, and right now works in an in-memory repository can be quite surprising (but that's #4995 + its ignore counterpart, I think).

@kcsaul Do you have a moment to give a spin to that (OP is #4671), see if the behavior is at least equivalent to v27 ?

tiennou · 2019-05-01T18:58:45Z

+	 * sense, and will thus be disabled. Passing "objects" here is thus
+	 * reserved to backends that can make sense of its contents.
+	 */
+	const char *location;


Do not mind this too much. I have a hunch it would be nice to have (as in, allows us to tell in-memory from not, while providing them with a dedicated location). But it's only used as a somewhat-lock to also identify if the odb stack is only default backends. Useful, but unused 😉.

I feel like this is assuming just too much about possible ODB backend implementations. The location belongs into loose/packed objects for sure, but it doesn't make any sense for e.g. in-memory or database-backed backends. As such I'd vote to not include such a field, as most backends except for Git's own cannot make any sense of it.

I was attempting something with that, but I'm not quite sure now. I felt that knowing that we're using our own "standard" backends was something generally useful. And I felt that knowing if a backend has or has no filesystem location was as well, and that this aspect could be used as part of the definition of what is an in-memory repository. You could well imagine a backend that uses the filesystem, but it's not ".git/objects"-compatible: how can you signal that to the repository, so that it knows to report that its GIT_REPOSITORY_ITEM_OBJECTS contents is non-standard ? Not sure where I was going with this though…

I agree that this would be useful. But that'd require a new callback in the backends, as we generally shouldn't assume anything of backends. Might very well be (well, probably not) that somebody has implemented his own implementation of a ".git/objects" backend that is uber fast due to it using platform-specific things.

tiennou · 2019-05-01T19:02:19Z

+ *
+ * @return 0 on success, or an error code
+ */
+GIT_EXTERN(int) git_repository_set_path(git_repository *repo, const char *path);


That allows giving a repository a filesystem location. It's not quite clear what will happen to it, but at least that should fix the "cannot exist in repository", if a user choses to do so.

I hate that we started using path in the past. There's so many different paths, and this name doesn't tell at all which one we're referring to. Gitdir? Commondir? Working dir? Anyway, we're stuck with it and use it in different places already, so let's bite the bullet and stick with it :/ Or is it another API for deprecation, @ethomson?

That being said, the comment should say which of these paths we're actually referring to.

👍 on the naming issue, and I can take care of the deprecation here if we decide for it.

Mh. Dunno -- I think it would be easier to do in another PR to keep the code churn low and keep this reviewable. The deprecation things always require to make a sweep through all of our code, and I bet that git_repository_path is used in quite a lot of places.

tiennou · 2019-05-01T19:06:02Z

+
+	*out = NULL;
+
+	if ((error = git_repository_item_path(&objects_dir, repo,


This function got extracted because it's the interface point between them, and normally the place where the odb would initialize its backends' filesystem locations (hence it was more logical in odb). That's still in flux though.

kcsaul · 2019-05-10T01:31:19Z

@tiennou I'm getting some build errors in regards the new constant, do I need to change a compiler setting? (using MSVC).

C2099 initializer is not a constant \libgit2\src\repository.c Line 50
C2440 'initializing': cannot convert from 'void *' to 'git_repository_item_t' \libgit2\src\repository.c Line 50

tiennou · 2019-05-10T11:59:46Z

Hmm, I'm confused as well, and can't take a look right now. Maybe it doesn't see a definition for git_repository_item_t, uses * whatever, and then complains about it not being const-compatible ? I think that approach was actually reverted because of MSVC…

kcsaul · 2019-05-11T00:00:02Z

@tiennou For now, I've been able to fix the build error by moving the 'none' constant to the enumeration in the header.

typedef enum {
	GIT_REPOSITORY_ITEM__NONE = -1,
	GIT_REPOSITORY_ITEM_GITDIR,
	GIT_REPOSITORY_ITEM_WORKDIR,
	GIT_REPOSITORY_ITEM_COMMONDIR,
	...
} git_repository_item_t;

Happy to confirm I'm no longer getting the 'path cannot exist in repository' error (#4671) and things seem to be working as they were previously now 😄

tiennou · 2019-06-19T18:39:37Z

Rebased, I've made the fallback value public since it can't be "hidden".

Note that I've seen to have messed up my brain when writing the PR, so let's recap the issues:

As there can be accesses to things inside the gitdir itself, for which we have no "repository backend" (lots of direct file accesses, actually), users going through git_repository_new would have no way to set a location. With no gitdir, some systems would fail to work for those users. Those are the first few fixes from the PR.
We were missing the fallback via the commondir mechanism, used for some items. This has been implemented.
Since being able to set the location of a repository seemed generally useful (to cater for the users of 1) that do want to make use of those systems "correctly"), so git_repository_set_path was added.

I think not much is missing from it, it just needs some thought about whether we should do 3), or if the first two are enough, and 3) can wait for a "repository backend" interface. In other words, I'm happy to drop the test and the setter for now (or make it private and keep them).

pks-t

Sorry for taking so long to review. Most of the PR looks very good to me, especially all those refactorings make the code a lot easier to read.

I'm not really convinced of having location and the is_default things. It's a heavy layering violation and is really only applicable to the loose/packed ODB backends, which is why we should avoid adding such fields.

pks-t · 2019-06-21T07:12:41Z

+	 * sense, and will thus be disabled. Passing "objects" here is thus
+	 * reserved to backends that can make sense of its contents.
+	 */
+	const char *location;


I feel like this is assuming just too much about possible ODB backend implementations. The location belongs into loose/packed objects for sure, but it doesn't make any sense for e.g. in-memory or database-backed backends. As such I'd vote to not include such a field, as most backends except for Git's own cannot make any sense of it.

pks-t · 2019-06-21T07:15:28Z

+ *
+ * @return 0 on success, or an error code
+ */
+GIT_EXTERN(int) git_repository_set_path(git_repository *repo, const char *path);


I hate that we started using path in the past. There's so many different paths, and this name doesn't tell at all which one we're referring to. Gitdir? Commondir? Working dir? Anyway, we're stuck with it and use it in different places already, so let's bite the bullet and stick with it :/ Or is it another API for deprecation, @ethomson?

That being said, the comment should say which of these paths we're actually referring to.

pks-t · 2019-06-21T07:18:33Z

-	if ((error = git_repository_item_path(&path,
-			repo, GIT_REPOSITORY_ITEM_INFO)) < 0)
+	if ((error = preload_attr_file(repo, attr_session, GIT_ATTR_FILE__FROM_FILE,
+				       NULL, git_repository_attr_cache(repo)->cfg_attr_file)) < 0)


I know we previously didn't check for it, but what about GIT_ENOTFOUND here? Just wondering aloud

That's true. I have not really read that code, but I'll check what it expects.

preload_attr_file calls git_attr_cache__get, which swallows GIT_ENOTFOUND already, so this looks fine:

libgit2/src/attrcache.c

Lines 236 to 250 in c544850

/* if file could not be loaded */

if (error < 0) {

/* remove existing entry */

if (file) {

attr_cache_remove(cache, file);

git_attr_file__free(file); /* offset incref from lookup */

file = NULL;

}

/* no error if file simply doesn't exist */

if (error == GIT_ENOTFOUND) {

git_error_clear();

error = 0;

}

}

Huh, confusing. But well, fair enough, thanks for digging in

The code in the `attr_setup` function is not really matching our current coding style. Besides alignment issues, it's also hard to see what functions calls depend on one another because they're split up over multiple conditional statements. Fix these issues by grouping together dependent function calls and adjusting the alignment.

If creating a repository without a common directory (e.g. by using `git_repository_new`), then `git_repository_item_path` will return `GIT_ENOTFOUND` for every file that's usually located in this directory. While we do not care for this case when looking up the "info/attributes" file, we fail to properly ignore these errors when setting up or collecting attributes files. Thus, the gitattributes lookup is broken and will only ever return `GIT_ENOTFOUND`. Fix this issue by properly ignoring `GIT_ENOTFOUND` returned by `git_repository_item_path`.

As with the preceding commit, the ignore code tries to load code from info/exclude, and we fail to ignore a non-existent file here.

For example, https://git-scm.com/docs/gitrepository-layout says: info Additional information about the repository is recorded in this directory. This directory is ignored if $GIT_COMMON_DIR is set and "$GIT_COMMON_DIR/info" will be used instead. So when looking for `info/attributes`, we need to check the commondir first, or fallback to "our" `info/attributes`.

tiennou · 2019-06-26T13:11:52Z

Rebased, I've separated out all my experiments to feature/odb-fs-location, so this fixes the following issues only :

as custom backends have no way to store files on-disk (they can, but cannot inform us that they support those accesses, and some of those files aren't actually under the ODB's purview), any higher-level system that tries to access a "standard" git file will get back GIT_ENOTFOUND. Some systems didn't respond well to that (diff in git_diff_foreach on bare repository fails with 'path cannot exist in repository' #4671, attr in Fix attribute lookup without common dir #4995). IIRC I've audited all calls to git_repository_item_path at the time, so there should be no other fixes.
additionally, we were missing some of the fallbacks done by git. I don't think there's an issue for that though, as it's worktree-related; it's not clear to me what kind of weirdness would have resulted.

I'm kinda bummed that we don't have that many tests for those kind of ODB backend issues… I've looked at our libgit2-backend repository but it's not in a good shape. Would integrating at least one of the simpler ones as part of our CI "examples" runs or something would be good ? SQLite backend maybe, as it still needs a "locally discoverable" file — say .git/sqlite — and could thus in theory still support the item_path API, but cannot because of the aforementioned issue. Ideally that'd provide some guidance as to what we functionally expect from an ODB backend, be sufficiently un-git-like that it would exercise the API in novel ways, helping to find and correct those "missing parts" ?

pks-t · 2019-06-27T08:06:45Z

-	if ((error = git_repository_item_path(&path,
-			repo, GIT_REPOSITORY_ITEM_INFO)) < 0)
+	if ((error = preload_attr_file(repo, attr_session, GIT_ATTR_FILE__FROM_FILE,
+				       NULL, git_repository_attr_cache(repo)->cfg_attr_file)) < 0)


Huh, confusing. But well, fair enough, thanks for digging in

pks-t · 2019-06-27T08:11:36Z

Thanks @tiennou!

csware · 2019-06-29T14:24:26Z

This PR breaks various tests in TortoiseGit and changes the output of libgit2 (cf. https://ci.appveyor.com/project/TortoiseGit/tortoisegit-06nst/builds/25633104?fullLog=true).

I bisected the libgit2 commit history and the first faulty commit it 5452e49.

One case is:
Diffing two trees a call to git_diff_get_stats fail with -1 and git_error_last returns cannot grow a borrowed buffer (comparing b02add66f48814a73aa2f0876d6bbc8662d6a9a8 with b9ef30183497cdad5c30b88d32dc1bed7951dfeb in the repository https://gitlab.com/tortoisegit/tortoisegit/tree/master/test/UnitTests/resources/git-repo1, test: https://gitlab.com/tortoisegit/tortoisegit/blob/47a46485f4f9f99b17257e6692e51827fa23a0f4/test/UnitTests/GitTest.cpp#L3009).

Fix is in PR #5152.

pks-t mentioned this pull request Feb 20, 2019

Fix attribute lookup without common dir #4995

Closed

tiennou force-pushed the fix/4671 branch from f509f50 to 6d0c600 Compare May 1, 2019 18:53

tiennou commented May 1, 2019

View reviewed changes

tiennou force-pushed the fix/4671 branch from 6d0c600 to d9fc2a7 Compare May 1, 2019 19:39

tiennou force-pushed the fix/4671 branch 2 times, most recently from f1e285a to 5d1ab21 Compare June 19, 2019 18:37

tiennou changed the title ~~WIP: Incomplete commondir support~~ Incomplete commondir support Jun 19, 2019

pks-t requested changes Jun 21, 2019

View reviewed changes

tiennou force-pushed the fix/4671 branch from 5d1ab21 to f06e32e Compare June 26, 2019 12:48

pks-t and others added 5 commits June 26, 2019 14:49

ignore: fix a missing commondir causing failures

b883d37

As with the preceding commit, the ignore code tries to load code from info/exclude, and we fail to ignore a non-existent file here.

docs: fixups

9f723c9

tiennou force-pushed the fix/4671 branch from f06e32e to 501c51b Compare June 26, 2019 12:50

pks-t approved these changes Jun 27, 2019

View reviewed changes

pks-t merged commit 3d22394 into libgit2:master Jun 27, 2019

tiennou deleted the fix/4671 branch June 27, 2019 08:45

csware mentioned this pull request Jun 29, 2019

Fix Regression: attr: Correctly load system attr file (on Windows) #5152

Merged

implausible mentioned this pull request Jul 23, 2019

Bump libgit2 nodegit/nodegit#1705

Merged

kcsaul mentioned this pull request Sep 28, 2019

git_diff_foreach on bare repository fails with 'path cannot exist in repository' #4671

Closed

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

Vesemir mentioned this pull request Apr 29, 2021

"Assertion failed path && path_out" in src/path.c while trying to rename reference on custom refdb repo without commonpath set #5851

Closed

kcsaul mentioned this pull request Jul 11, 2021

Fix worktree iteration when repository has no common directory #5943

Merged


		*out = NULL;

		if ((error = git_repository_item_path(&objects_dir, repo,

	/* if file could not be loaded */
	if (error < 0) {
	/* remove existing entry */
	if (file) {
	attr_cache_remove(cache, file);
	git_attr_file__free(file); /* offset incref from lookup */
	file = NULL;
	}
	/* no error if file simply doesn't exist */
	if (error == GIT_ENOTFOUND) {
	git_error_clear();
	error = 0;
	}
	}

Uh oh!

Conversation

tiennou commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pks-t commented Feb 20, 2019

Uh oh!

ethomson commented Feb 20, 2019

Uh oh!

tiennou commented Feb 20, 2019

Uh oh!

pks-t commented Feb 22, 2019

Uh oh!

tiennou commented Feb 22, 2019

Uh oh!

tiennou left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

kcsaul commented May 10, 2019

Uh oh!

tiennou commented May 10, 2019

Uh oh!

kcsaul commented May 11, 2019

Uh oh!

tiennou commented Jun 19, 2019

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

tiennou commented Jun 26, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t commented Jun 27, 2019

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

tiennou commented Jan 30, 2019 •

edited

Loading

tiennou left a comment •

edited

Loading

csware commented Jun 29, 2019 •

edited

Loading