WIP: ENV-friendly libgit2 #4856

tiennou · 2018-10-21T22:37:17Z

This is a work-in-progress implementation of the design I proposed in #4815. For consistency, here's a summary :

The current git_repository_open_ext lacks the flexibility we need to protect users from changes to the API w.r.t environment variable we might start supporting. So, this adds git_repository_open_with_opts, which supersedes it, and adds a user-configurable whitelist of what we're allowed to customize using the environment. This also modifies how we load values from the environment, so we can apply any of those allowed envvars while the repository is opened (mostly to support GIT_CONFIG, as the previous implementation would have already loaded and used the config before we could override it). Opening bare repositories while using the environment is now available.

I'm labeling it as WIP because I didn't write tests for the new behavior yet, as I have a few points worth discussing:

as per the last commit in the PR, I propose removing the git_strarray ceiling_dirs in favor of a git_strarray. Since its value depends on the platform-specific path separator, I feel like we're doing a disservice to users by not handling that more transparently. Since git_repository_open_with_opts is brand new, now's the time to do it.
I've discovered that GIT_REPOSITORY_OPEN_FROM_ENV's documentation said this :

In the future, this flag will also cause git_repository_open_ext to
respect $GIT_WORK_TREE and $GIT_COMMON_DIR; currently,
git_repository_open_ext with this flag will error out if either
$GIT_WORK_TREE or $GIT_COMMON_DIR is set.

This is now supported, which means I've changed how git_repository_from_ext behaves for unsuspecting users, which was the whole point of the implementation (😁). I have no strong opinion on the matter, we just need to choose whether those envvars are used or not w.r.t to backward-compat.

I tried to minimize changes to the "find_repo" function, but I haven't checked for work that might be done for nothing, in case the environment specifies everything. IMHO it would not be worthwhile.
Documentation-checking. From the top of my head, the semantics of what should happen when ceiling_dirs is specified as both a parameter and its envvar form. I'm leaning toward using the more explicit value (ie. listening to the code vs. the environment) in that case.

src/repository.c

pks-t · 2018-12-19T11:47:05Z

include/git2/repository.h

+ * `git_repository_open_with_opts`:
+ *
+ * - If `GIT_REPOSITORY_OPEN_FROM_ENV` is set, all other flags as well as the
+ * `ceiling_dirs` argument will be ignored.


One might argue that in the future, we could potentially grow additional flags that will be honored even if FROM_ENV is set. In that case, we should explicitly list all flags that do get ignored. But yeah, this future is not there yet so I'd be fine to just keep and document behaviour like you have it right here

include/git2/repository.h

pks-t · 2018-12-19T11:50:23Z

include/git2/repository.h

+	 * When GIT_REPOSITORY_OPEN_FROM_ENV is set, this can be used to
+	 * whitelist the environment variables we're allowed to access.
+	 *
+	 * An empty list means that we can use any of them.


I'd rather say "we can use all of them if the above flag is set." to make it a tad clearer. Because I'm already confused whether any means "all" or "none" -- code below definitely says "none".

Sounds like I've confused myself too : a NULL whitelist means "any", an empty one means "none". I fail to see the use case for "empty" so I'm not sure if it makes sense. I'll fix & rephrase though.

pks-t · 2018-12-19T11:51:55Z

src/repository.c


 	if (!start_path) {
-		error = git__getenv(&dir_buf, "GIT_DIR");
+		error = git__getenv_wl(&dir_buf, "GIT_DIR", &opts->allowed_env);


Doesn't this behave opposite to what the documentation says? This pointer will never be a NULL pointer, and in case where the user has not added any whitelisted entries then it will always return GIT_ENOTFOUND.

allowed_env is initialized by either the backward-compatibility code in git_repository_open_ext, or explicitly by the user via the new options struct, which defaults to NULL. So unless I'm missing something this should behave as expected.

Disregard that, I mistook pointers for non-pointers 🙄.

src/repository.c

pks-t · 2018-12-19T12:06:34Z

src/repository.c

-			flags |= GIT_REPOSITORY_OPEN_NO_SEARCH;
-			flags |= GIT_REPOSITORY_OPEN_NO_DOTGIT;
+			opts->flags |= GIT_REPOSITORY_OPEN_NO_SEARCH;
+			opts->flags |= GIT_REPOSITORY_OPEN_NO_DOTGIT;


Honestly, it's a bit weird to modify the flags passed in by callers

src/repository.c

pks-t · 2018-12-19T12:10:10Z

include/git2/repository.h

 	 * the search for a containing repository should terminate.
+	 *
+	 * FIXME: this depends on the value of GIT_PATH_LIST_SEPARATOR, which is
+	 * platform-specific. Make that a git_strarray ?


I think it's perfectly fine to fix this API while we're at it. The old function could simply split the string it got into the string array

Previously, when `GIT_REPOSITORY_OPEN_FROM_ENV` was used, we'd end up calling `git_repository_open_ext` recursively with another set of flags and ceiling_dirs. This prevents us from supporting some environment variables (eg. GIT_CONFIG) because part of `git_repository_open_ext` would touch the config before we'd get a chance to tweak it. AFAICT `GIT_WORK_TREE` and `GIT_COMMON_DIR` might have encountered the same problem. Values from the environment are now stored in a struct, so those can applied as soon as possible while the repository is being setup.

The goal is to make the list of allowed envvars user-specified, so users can specify what we're allowed to use from the environment. This also freezes the current list uses when opening in `_open_ext`, for backward-compatibility reasons.

It seems the find-workdir-from-gitdir was broken. The first find_env will setup something, which the rest of the code will trample over.

LIBRARIES is the (absolute?) path to the library. LDFLAGS is the full linker stanza to correctly link with this lib. By passing LIBRARIES as LIBGIT_LIBS, the linker ends up with the absolute path for the SDK'ed version of CoreFoundation (which doesn't exist), instead of the familiar `-framework CoreFoundation`.

tiennou

Rebased, as I've wanted to try to polish it a little, and hit a snag. This is still WIP, but the current code really needs a refactor, and I don't think I can handle it without re-reading a whole lot of git manpages, so…

This should be green, but as I had to comment "innocuous looking" code to fix it, I had to look more closely at what was going on. I've left review comments about those issues, in case someone else has any insight. Note that there are a bunch of missing oom checks in there, which I didn't attempt to fix yet.

tiennou · 2019-10-14T09:09:12Z

src/repository.c

+					git_buf_set(&env->gitdir_path, path.ptr, path.size);
+					GIT_ERROR_CHECK_ALLOC_BUF(&env->gitdir_path);
+
+					git_buf_attach(&env->gitlink_path,


This reads a (potentially non-existing) $GIT_DIR/gitdir file in gitlink.

tiennou · 2019-10-14T09:23:21Z

src/repository.c

+			git_path_dirname_r(&env->workdir_path, path.ptr);
+			git_path_to_dir(&env->workdir_path);
+		}
+		if (git_buf_oom(&env->workdir_path)) {


Here, we set workdir_path as whatever dirname(path) is.

tiennou · 2019-10-14T09:24:30Z

src/repository.c

 		GIT_ERROR_CHECK_ALLOC(repo->commondir);
 	}

+//	if (git_buf_len(&env.workdir_path) > 0) {


I've added this, since it seemed to be provided/detected. This causes a crash further down, hence the commenting.

tiennou · 2019-10-14T09:26:34Z

src/repository.c

+		GIT_ERROR_CHECK_ALLOC(repo->configfile);
+	}
+
 	if ((error = repo_is_worktree(&is_worktree, repo)) < 0)


Function.

This could have easily been part of repo_env_find, since it only look at the filesystem.

tiennou · 2019-10-14T09:28:08Z

src/repository.c

 		if (config &&
 		    ((error = load_config_data(repo, config)) < 0 ||
-		     (error = load_workdir(repo, config, &workdir)) < 0))
+		     (error = load_workdir(repo, config, &env.workdir_path)) < 0))


We move on to actually setup the working directory, at https://github.com/libgit2/libgit2/pull/4856/files#diff-7cbc97c802e21b199f176b493cec8e9cR300, and we pass the workdir identified by the "find" step. This is the part where the crash I'm getting comes from, and I'm starting to get completely lost (and I can't inline-review that code anyway, so there goes) :

core.worktree is loaded from the config, as it can now be made use of.

if repo->is_worktree is true (it only performed filesystem-checks), we read repo->gitdir + "gitdir" (already loaded while finding in env->gitlink), dirname() that, and use the result as our workdir.

if core.worktree is set, we check its value and use that as our workdir.

if the parent_path is passed, we check that it's a directory and use that.

finally, we dirname(repo->gitdir) and use that.

It seems that core.worktree gets a say iif the repository structure is slightly "wrong" — a $GIT_DIR/gitdir file that doesn't point to a valid repo. The fact that it is passing as a parameter the very thing this code should be setting up doesn't help 😉.

tiennou force-pushed the feature/repo-open-opts branch 6 times, most recently from 89ef767 to b0e7e75 Compare October 22, 2018 23:24

pks-t reviewed Dec 19, 2018

View reviewed changes

tiennou force-pushed the feature/repo-open-opts branch 2 times, most recently from 0dc1bec to 318b6c6 Compare January 3, 2019 22:41

pks-t mentioned this pull request Feb 20, 2019

Incomplete commondir support #4967

Merged

4 tasks

tiennou force-pushed the feature/repo-open-opts branch from 318b6c6 to c755ad7 Compare May 2, 2019 00:14

tiennou force-pushed the feature/repo-open-opts branch from c755ad7 to f8a4e2e Compare October 3, 2019 10:26

mechanicker mentioned this pull request Oct 3, 2019

Provide flags to limit config file lookup when open a repository #5244

Closed

tiennou force-pushed the feature/repo-open-opts branch from f8a4e2e to 18f308a Compare October 14, 2019 08:59

tiennou added 14 commits October 14, 2019 10:59

buf: add git_buf_slice

1b1f90e

strarray: add a splitter for PATH_LIST-separated strings

612a0fe

repo: make ceiling_dirs a strarray

aaa90f9

repo: ODB alternates are also a PATHLIST_SEPARATED list

936c355

repo: introduce git_repository_open_with_opts

2370a6d

repo: ensure the current _open_ext semantics are preserved

39fd2d6

util: add a whitelistable getenv helper

9fdad06

repo: obey GIT_WORK_TREE/GIT_COMMON_DIR envvars

ca80576

repo: support GIT_CONFIG config file override

98b1c0b

WIP: investigate

dec455e

It seems the find-workdir-from-gitdir was broken. The first find_env will setup something, which the rest of the code will trample over.

WIP: debugging

19bcdec

tiennou force-pushed the feature/repo-open-opts branch from 18f308a to 19bcdec Compare October 14, 2019 08:59

tiennou commented Oct 14, 2019

View reviewed changes

tiennou mentioned this pull request Nov 5, 2019

"Generic" repository layout #5296

Open

tiennou mentioned this pull request Feb 10, 2020

Handle repository format v1 #5388

Merged

Base automatically changed from master to main January 7, 2021 10:09

WIP: ENV-friendly libgit2 #4856

Are you sure you want to change the base?

WIP: ENV-friendly libgit2 #4856

Uh oh!

Conversation

tiennou commented Oct 21, 2018

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants