submodule: check index for path and prefix before adding submodule #4378

cjhoward92 · 2017-10-16T22:33:40Z

This is to fix an issue where libgit2 will allow you to add a submodule even if the path you are trying add is found on the index. In the CLI we get a nice error about this, but not in libgit2.

I would obviously like some pointers on how to do this, as I do not think it is 100% correct. For one, when I add the trailing slash to the path, I feel like I could clean that up or have a better implementation. I am also unsure if there is a more streamlined routine to check the index for the path.

Some advice on how I could clean this up would be awesome.

pks-t

Thanks for the PR! I think it looks good regarding what it tries to do, but I'm a bit in a hurry right now. I'll do a proper review of the logic at a later point, probably when you've got around to amending this PR :)

pks-t · 2017-10-20T13:48:16Z

src/submodule.c

@@ -688,6 +691,34 @@ int git_submodule_add_setup(
 		goto cleanup;
 	}

+	/* get the index for the repo */


It would be nice to put the following checks into a simple function. Something like can_add_submodule(repo, path).

pks-t · 2017-10-20T13:51:14Z

src/submodule.c

+	path_len = strlen(path);
+	dir = git__malloc(path_len + 1);
+	strcpy(dir, path);
+	git_path_string_to_dir(dir, path_len + 1);


You could use a buffer here with git_path_to_dir:

git_buf path = GIT_BUF_INIT; git_buf_sets(&path, path); git_path_to_dir(&path);

Error handling is obviously missing.

The code is obviously bogus, I should've chosen two different variable names. But I guess you get what I mean :P

pks-t · 2017-10-20T13:52:26Z

tests/submodule/add.c

+	/* In this repo, HEAD (master) has no remote tracking branc h*/
+	g_repo = cl_git_sandbox_init("testrepo");
+
+	git_buf_joinpath(&dirname, git_repository_workdir(g_repo), "TestGitRepository");


Please wrap every call which you expect to just return 0 for success in cl_git_pass.

pks-t · 2017-10-20T13:53:25Z

tests/submodule/add.c

+
+	cl_git_pass(
+		git_repository_index__weakptr(&index, g_repo)
+	);


There's no need for the newlines here. In most cases, we simply ignore line length limits for our tests and put those things in one line.

pks-t · 2017-10-20T13:54:04Z

tests/submodule/add.c

+
+	p_mkdir(dirname.ptr, 0700);
+	fd = fopen(filename.ptr, "w");
+	fclose(fd);


You should also assert that those three functions return expected values.

pks-t · 2017-10-20T13:54:55Z

tests/submodule/add.c

+	);
+
+	git_buf_free(&dirname);
+	git_buf_free(&filename);


You have to free the submodule, as well.

pks-t · 2017-10-20T13:56:24Z

tests/submodule/add.c

+	git_buf_joinpath(&filename, dirname.ptr, "test.txt");
+
+	p_mkdir(dirname.ptr, 0700);
+	fd = fopen(filename.ptr, "w");


We have cl_git_mkfile for that

pks-t · 2017-10-20T13:56:48Z

tests/submodule/add.c

+	/* In this repo, HEAD (master) has no remote tracking branc h*/
+	g_repo = cl_git_sandbox_init("testrepo");
+
+	git_buf_joinpath(&name, git_repository_workdir(g_repo), "TestGitRepository");


Please assert the return value here.

pks-t · 2017-10-20T13:56:58Z

tests/submodule/add.c

+	git_buf_joinpath(&name, git_repository_workdir(g_repo), "TestGitRepository");
+
+	fd = fopen(name.ptr, "w");
+	fclose(fd);


We have cl_git_mkfile for that.

pks-t · 2017-10-20T13:57:12Z

tests/submodule/add.c

+	);
+
+	git_buf_free(&name);
+}


Again, you also have to free the submodule here

cjhoward92 · 2017-10-20T14:39:34Z

@pks-t Also thank you for reviewing this. I will make these changes as soon as I can! I appreciate you taking the time to look over my work.

cjhoward92 · 2017-10-20T21:24:36Z

@pks-t Updated. Let me know what you think!

cjhoward92 · 2017-11-01T20:30:58Z

Any chance anyone will take a look at this?

pks-t

Sorry for taking that long to review, I've been quite busy recently with life :/

Anyway, thanks for your fixups, it's a lot more readable already. Besides some style-related things, I've also taken a look at it regarding functionality now. I've added some comments regarding that, would be nice if you could clarify those.

Keep going your good work and this PR will be ready soon :)

pks-t · 2017-11-06T09:08:59Z

src/submodule.c

@@ -149,6 +149,41 @@ static int find_by_path(const git_config_entry *entry, void *payload)
 	return 0;
 }

+static int can_add_submodule(git_repository *repo, const char *path) {


Please move the opening curly brace to a new line.

pks-t · 2017-11-06T09:10:53Z

src/submodule.c

+	if ((error = git_path_to_dir(&dir)) < 0)
+		return error;
+
+	/* get the index for the repo */


Would be nice if you removed those comments. You shouldn't describe what you are doing with comments, as most often that is obvious from the code itself. Instead, you should try to document why you are doing that. E.g. a useful comment here would be one preceding the function which explains exactly when you can add a submodule and when you can't.

pks-t · 2017-11-06T09:12:40Z

src/submodule.c

+	if ((error = git_buf_sets(&dir, path)) < 0)
+		return error;
+
+	if ((error = git_path_to_dir(&dir)) < 0)


You start to leak memory as soon as one of those functions returns an error. Instead of "return error;", we usually have e.g. a "goto error;" where all memory is free'd. E.g.:

static int can_add_submodule(git_repository *repo, const char *path) { ... if ((error = git_path_to_dir(&dir)) < 0) goto out; ... out: git_buf_free(&dir); return error; }

pks-t · 2017-11-06T09:17:01Z

src/submodule.c

+
+	/* see if the submodule name exists as a file on the index */
+
+	if ((error = git_index_find(NULL, index, path)) == 0) {


I think there's something off here. You first normalize path to a directory path by using git_path_to_dir and now you try to check if there's a file at that location matching the path. I guess what you wanted to do instead is first using the "normal" path value and only normalize it to a directory name after you've used git_index_find here to check whether there's a file there.

pks-t · 2017-11-06T09:17:31Z

src/submodule.c

+
+	if ((error = git_index_find_prefix(NULL, index, dir.ptr)) == 0) {
+		giterr_set(GITERR_SUBMODULE,
+			"'%s' already exists in the index", path);


Would be cool if you disambiguated those two error messages based on their type (that is directory vs file already existing).

pks-t · 2017-11-06T09:20:08Z

tests/submodule/add.c

+	git_buf dirname = GIT_BUF_INIT;
+	git_buf filename = GIT_BUF_INIT;
+
+	/* In this repo, HEAD (master) has no remote tracking branc h*/


There's a typo. Also: why does this matter for our particular unit test here? I don't want to propose it doesn't, but for myself I don't get why this is interesting. Please either extend that comment and explain a bit more why this is important here or just remove it :)

pks-t · 2017-11-06T09:20:44Z

tests/submodule/add.c

+	git_submodule *sm;
+	git_buf name = GIT_BUF_INIT;
+
+	/* In this repo, HEAD (master) has no remote tracking branc h*/


Same as above

pks-t · 2017-11-06T09:27:30Z

tests/submodule/add.c

+	/* In this repo, HEAD (master) has no remote tracking branc h*/
+	g_repo = cl_git_sandbox_init("testrepo");
+
+	cl_git_pass(git_buf_joinpath(&dirname, git_repository_workdir(g_repo), "TestGitRepository"));


I'd rename "TestGitRepository" to e.g. just "subdirectory" or "directory". Otherwise it's a bit confusing, as that path should not represent a real git repository but should only contain a file later on.

pks-t · 2017-11-06T09:32:30Z

tests/submodule/add.c

@@ -128,3 +128,51 @@ void test_submodule_add__url_relative_to_workdir(void)

 	assert_submodule_url("TestGitRepository", git_repository_workdir(g_repo));
 }
+
+void test_submodule_add__path_exists_in_index(void)


There's one problem with that test. You want to verify that creation of a submodule fails if the path exists in the index, but actually when you create your submodule it exists both inside the index and also inside of the working directory. So us failing here might not be the result from the entry existing in the index, but instead that file existing in the working directory.

You could either just remove the directory and file after you've added them to the index or, which is the preferred method if you ask me, instead add the index entry directly by using git_index_add.

I get the problem you are describing, but I am unsure how to use git_index_add. How can I create a git_index_entry properly? I s there something I am missing? Other than index_create_entry, which is not exposed outside of index.c I do not know how to create an entry. For now, I am updating the routine to just delete the file and directory after I add to the index.

Take a look at for example "tests/index/add.c", test_add_entry(). You can create an index entry by simply assigning the structure fields.

pks-t · 2017-11-06T09:35:17Z

tests/submodule/add.c

+
+	git_submodule_free(sm);
+	git_buf_free(&name);
+}


Right now you're checking two out of four cases:

an index entry exists inside of the directory you want to add

a file exists in the working directory

But there are two cases missing:

an index entry exists directly where you want to create the submodule

a directory exists where you want to create the submodule

Would it make sense to add tests for those two cases?

I am not sure exactly what your test cases mean. I was trying to test two cases:

A file path exists in the index with the same name as the submodule

A directory exists in the index with the same name as the submodule

If you could offer some clarification I would be more than happy to add extra test cases :)

Do you mean we should test against the workdir as well? I am not sure that my added code does that, it is an explicit check against the index but it makes sense that we should check untracked files in the workdir too. Probably a good check to have in can_add_submodule. Thoughts?

cjhoward92 · 2017-11-14T04:18:31Z

I janked this up bad. I will recover the remote from my other machine :(

cjhoward92 · 2017-12-18T18:58:29Z

@pks-t I finally got around to making all of the changes you requested, I think. Let me know if there is anything else I need to do. Sorry for the wait, been a busy few months!

cjhoward92 · 2018-01-04T19:25:52Z

bump

pks-t

To be honest, I'm being a bit nitpicky with the can_add_submodule function, but we do usually return -1 codes in case of an unexpected error only. It's also a bit weird to see something like if (can_add_submodule() < 0), because that doesn't explain the semantics immediately. On the other hand, if (is_path_occupied(&occupied)) < 0) return -1; if (occupied) ... makes the intentions totally clear.

The tests now look perfect, thanks a lot for improving them!

pks-t · 2018-01-05T08:06:47Z

src/submodule.c

+		error = GIT_EEXISTS;
+		goto out;
+	}
+	error = 0;


This is not needed here, as the next line will already unconditionally set error

pks-t · 2018-01-05T08:08:44Z

src/submodule.c

@@ -688,6 +723,9 @@ int git_submodule_add_setup(
 		goto cleanup;
 	}

+	if ((error = can_add_submodule(repo, path)) < 0)


Huh. Looks alright to me, too. Dunno either :P

pks-t · 2018-01-05T08:14:23Z

src/submodule.c

+ * Checks to see if the submodule shares its name with a file or directory that
+ * already exists on the index. If so, the submodule cannot be added.
+ */
+static int can_add_submodule(git_repository *repo, const char *path)


I think this is a bit misnamed. You do not check if we can add the submodule at a given path, as that would involve other checks as well. Instead, you only check whether the given path is already occupied by something else. How about is_path_occupied instead?

One more thing is that functions which should answer a certain question are expected to return a bool, not an int. So I'd propose a signature like static int is_path_occupied(bool *occupied, git_repository *repo, const char *path). Like this, a real unexpected error can be returned by returing a negative value. If no error happens we return 0 and occupied will then be set to true if the path is occupied and false otherwise.

pks-t · 2018-01-05T08:17:06Z

tests/submodule/add.c

+
+	cl_git_pass(git_repository_index(&index, g_repo));
+
+	test_add_entry(index, valid_blob_id, filename.ptr, GIT_FILEMODE_BLOB);


Perfect, these tests look very nice now! :)

cjhoward92 · 2018-01-06T18:08:56Z

@pks-t Should be updated with is_path_occupied. Let me know what you think!

pks-t · 2018-01-09T14:40:27Z

src/submodule.c

+	if ((error = git_path_to_dir(&dir)) < 0)
+		goto out;
+
+	if ((error = git_index_find_prefix(NULL, index, dir.ptr)) == 0) {


You're ignoring any potential error returned by git_index_find_prefix other than GIT_ENOTFOUND. Granted, right now it will not return any other error, but you never know how it will change in the future. Maybe something like this:

if ((error = git_index_find_prefix(NULL, index, dir.ptr)) < 0 && error != GIT_ENOTFOUND) goto out; if (!error) { giterr_set(GITERR_SUBMODULE, ...); *occupied = true; goto out; } error = 0;

submodule: check path and prefix before adding submodule submodule: fix test errors

…tests

cjhoward92 · 2018-03-27T14:37:57Z

This is also updated! Thanks again!

pks-t

Should be the last iteration, I guess. Looks very clean already

pks-t · 2018-03-28T10:16:30Z

src/submodule.c

+	if ((error = git_repository_index__weakptr(&index, repo)) < 0)
+		goto out;
+
+	if ((error = git_index_find(NULL, index, path)) == 0) {


I'd say it'd be a bit less fragile if the check was if ((error = git_index_find(NULL, index, path)) != GIT_ENOTFOUND). Otherwise, we might just ignore all errors by git_index_find (even though it currently does not return any error different than GIT_ENOTFOUND.

pks-t · 2018-03-28T10:16:54Z

src/submodule.c

+		goto out;
+
+	if ((error = git_index_find_prefix(NULL, index, dir.ptr)) < 0 && error != GIT_ENOTFOUND)
+    goto out;


This uses spaces instead of tabs.

pks-t · 2018-03-28T10:22:36Z

src/submodule.c

+	if ((error = git_path_to_dir(&dir)) < 0)
+		goto out;
+
+	if ((error = git_index_find_prefix(NULL, index, dir.ptr)) < 0 && error != GIT_ENOTFOUND)


You could probably do the same as above:

if ((error = git_index_find_prefix(NULL, index, dir.ptr)) != GIT_ENOTFOUND) { if (!error) { giterr_set(GITERR_SUBMODULE, "Already exists"); *occupied = true; error = -1; } goto out; }

pks-t · 2018-03-28T10:24:39Z

tests/submodule/add.c

+
+	cl_git_pass(git_buf_joinpath(&filename, "subdirectory", "test.txt"));
+
+	cl_git_pass(git_repository_index(&index, g_repo));


You should use git_repository_index__weakptr, otherwise you'd have to free index

pks-t · 2018-03-28T10:24:56Z

tests/submodule/add.c

+
+	g_repo = cl_git_sandbox_init("testrepo");
+
+	cl_git_pass(git_repository_index(&index, g_repo));


…und on add

cjhoward92 · 2018-03-28T13:51:31Z

Thanks for all of the hard work. I have no problem making as many iterations as possible to get this to be perfect!

pks-t · 2018-03-28T14:04:44Z

Looks good to me, thanks! Waiting for CI to catch up and will give others a chance to review.

pks-t · 2018-03-30T12:12:39Z

Thanks again for this PR!

pks-t requested changes Oct 20, 2017

View reviewed changes

cjhoward92 changed the title ~~WIP: submodule: check index for path and prefix before adding submodule~~ submodule: check index for path and prefix before adding submodule Nov 1, 2017

pks-t requested changes Nov 6, 2017

View reviewed changes

cjhoward92 force-pushed the fix/submodule-add-check-index branch from 78a4c02 to 2953339 Compare November 14, 2017 04:11

cjhoward92 force-pushed the fix/submodule-add-check-index branch 3 times, most recently from 12c939c to 420ffbd Compare November 20, 2017 04:00

cjhoward92 force-pushed the fix/submodule-add-check-index branch from 420ffbd to afd09ba Compare December 5, 2017 18:41

cjhoward92 force-pushed the fix/submodule-add-check-index branch 3 times, most recently from 5d7f680 to bea77bb Compare December 18, 2017 17:50

pks-t requested changes Jan 5, 2018

View reviewed changes

pks-t requested changes Jan 9, 2018

View reviewed changes

Carson Howard and others added 8 commits March 27, 2018 07:29

submodule: check index for prefix before adding submodule

ad1c435

submodule: check path and prefix before adding submodule submodule: fix test errors

test: submodule: add: use p_mkdir to create directories

0a74f39

test: submodule: add: join path without slashes

3e500fc

submodule: fix styling errors

9371149

submodule: update index check to check path before directory and fix …

ef9a774

…tests

tests: submodule: insert index entries directly into index

677d393

submodule: change can_add_submodule to is_path_occupied

b282ca7

submodule: add better error handling to is_path_occupied

c07abd6

cjhoward92 force-pushed the fix/submodule-add-check-index branch from d89af2c to c07abd6 Compare March 27, 2018 14:37

pks-t requested changes Mar 28, 2018

View reviewed changes

submodule: add more robust error handling when a submodule path is fo…

69a282d

…und on add

pks-t approved these changes Mar 28, 2018

View reviewed changes

pks-t merged commit dc27772 into libgit2:master Mar 30, 2018

cjhoward92 deleted the fix/submodule-add-check-index branch March 30, 2018 15:27

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


		/* see if the submodule name exists as a file on the index */

		if ((error = git_index_find(NULL, index, path)) == 0) {


		cl_git_pass(git_repository_index(&index, g_repo));

		test_add_entry(index, valid_blob_id, filename.ptr, GIT_FILEMODE_BLOB);


		cl_git_pass(git_buf_joinpath(&filename, "subdirectory", "test.txt"));

		cl_git_pass(git_repository_index(&index, g_repo));


		g_repo = cl_git_sandbox_init("testrepo");

		cl_git_pass(git_repository_index(&index, g_repo));

submodule: check index for path and prefix before adding submodule #4378

submodule: check index for path and prefix before adding submodule #4378

Conversation

cjhoward92 commented Oct 16, 2017

pks-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjhoward92 commented Oct 20, 2017

cjhoward92 commented Oct 20, 2017

cjhoward92 commented Nov 1, 2017

pks-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjhoward92 commented Nov 14, 2017

cjhoward92 commented Dec 18, 2017

cjhoward92 commented Jan 4, 2018

pks-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjhoward92 commented Jan 6, 2018

Choose a reason for hiding this comment

cjhoward92 commented Mar 27, 2018

pks-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjhoward92 commented Mar 28, 2018

pks-t commented Mar 28, 2018

pks-t commented Mar 30, 2018