Submodules-API should report .gitmodules parse errors instead of ignoring them by csware · Pull Request #4522 · libgit2/libgit2

csware · 2018-02-08T11:53:39Z

Issue mentioned in issue #4517 and issue #4516.

Git for Windows does report parse errors in .gitmodules, so should libgit2.

pks-t · 2018-02-08T12:06:04Z

 static int submodule_alloc(git_submodule **out, git_repository *repo, const char *name);
 static git_config_backend *open_gitmodules(git_repository *repo, int gitmod);
-static git_config *gitmodules_snapshot(git_repository *repo);
+static int gitmodules_snapshot(git_repository *repo, git_config **snap);


The out parameter is typically first

pks-t · 2018-02-08T12:10:48Z

 		data.repo = repo;

-		if ((mods = gitmodules_snapshot(repo)) == NULL)
+		if ((error = gitmodules_snapshot(repo, &mods)) < 0 || mods == NULL)


I feel like this interface has become a bit more brittle by having to check for mods == NULL, as well. What about returning for example ENOTFOUND in case there are no submodules?

I haven't done that in the first place as I then have to reset errors... First lemme see whether this approach will work at all...

pks-t · 2018-02-08T12:12:10Z

-		if (git_config_open_ondisk(&mods, path.ptr) < 0)
-			mods = NULL;
-	}
+		int error = git_config_open_ondisk(&mods, path.ptr);


Might be I'm misreading the diff, but isn't this introducing a declaration after instructions?

pks-t · 2018-02-08T12:12:43Z

+	cl_git_rewritefile("submod2/.gitmodules",
+			   "[submodule \"Test_App\"\n"
+			   "    path = Test_App\n"
+			   "    url = ../Test_App\n");


pks-t · 2018-02-08T12:13:10Z

 	cl_git_fail(git_submodule_foreach(g_repo, foreach_cb, NULL));
 }
+
+void test_submodule_lookup__fail_invalid_gitignores(void)


gitignores? Don't you mean gitmodules?

Yeah a type, this is still WIP as I can't test locally ATM...

csware · 2018-02-09T08:59:15Z

@pks-t All change requests should be addressed now.

pks-t

Thanks for the fixups. I think the interface is a lot easier to use like this, as you can assume that mods will never be NULL in case the function returns success. The only non-stylistic thing that bothers me is the git_buf_joinpath error being ignored, which should definitly be fixed (even though you obviously just go with the previous behavior).

pks-t · 2018-02-09T09:52:30Z


-		if ((mods = gitmodules_snapshot(repo)) == NULL)
+		if ((error = gitmodules_snapshot(&mods, repo)) < 0)
+		{


The opening "{" is on the same line for single-line if-statements.

pks-t · 2018-02-09T09:53:19Z

+		if ((error = gitmodules_snapshot(&mods, repo)) < 0)
+		{
+			if (error == GIT_ENOTFOUND)
+				error = 0;


In my opinion, it looks much more readable like this

pks-t · 2018-02-09T09:54:38Z

@@ -1916,31 +1921,32 @@ static int submodule_load_from_wd_lite(git_submodule *sm)

 /**
 * Returns a snapshot of $WORK_TREE/.gitmodules.


I'd like to see a short comment about GIT_ENOTFOUND here such that people know to expect it in case there are no submodules.

pks-t · 2018-02-09T09:57:02Z

+		git_config *mods = NULL;
 		if (git_buf_joinpath(&path, workdir, GIT_MODULES_FILE) != 0)
-			return NULL;
+			return 0;


Hum. Shouldn't this bubble up the error? git_buf_joinpath failing indicates that there was a memory allocation error, which definitly shouldn't be ignored.

if (git_buf_joinpath(&path, workdir, GIT_MODULES_FILE) < 0) return -1;

pks-t · 2018-02-09T09:58:28Z

-	git_config *mods = NULL, *snap = NULL;
-	git_buf path = GIT_BUF_INIT;

 	if (workdir != NULL) {


The whole indentation could be avoided if you return early in case workdir == NULL. That's a matter of taste, though -- your call if you want to do that instead.

Yeah I also thought that, however, I did not want to make too big changes...

csware · 2018-02-09T11:09:38Z

@pks-t All change requests should be addressed now.

pks-t · 2018-02-16T09:26:07Z


 	if (mods) {
-		git_config_snapshot(&snap, mods);
+		git_config_snapshot(snap, mods);


Sorry for not catching that earlier, but now that you report errors you should definitly also check the return value of git_config_snapshot.

pks-t · 2018-02-16T09:29:14Z

-		git_config_snapshot(&snap, mods);
+		git_config_snapshot(snap, mods);
 		git_config_free(mods);
+		return 0;


We usually try to avoid returning early for non-trivial code. I'd rather go with something like

... *snap = NULL; ... if ((error = git_config_open_ondisk(...)) < 0) goto out; if (!mods) { error = -1; goto out; } if ((error = git_config_snapshot(mods, config)) < 0) goto out; out: git_config_free(mods); if (error) git_config_free(*snap); return error;

goto is better? - Shall I change all the code for this? You asked to exit early before for !worktree.

Sorry 😅 Seeing that your code already improves the function by quite a lot, I'm satisfied if you check the return code of git_config_snapshot, without converting the function to goto style.

csware · 2018-03-11T15:43:11Z

@pks-t Any reason why lots of PRs are not merged (such as this one)?

ethomson · 2018-03-11T16:05:27Z

@csware because we’re busy. We appreciate your contributions but we are all volunteers and do not always have as much time to review pull requests as we would like.

We appreciate your patience.

ethomson · 2018-03-11T16:19:58Z

Also, we’ve locked down for 0.27. This is eligible for merging to master after that release, but this is not a change we’d take before then.

pks-t

v0.27.0 is finally out the door, so we're back to our usual business again.

pks-t · 2018-03-26T09:51:16Z

-			mods = NULL;
-	}
+	if (git_buf_joinpath(&path, workdir, GIT_MODULES_FILE) != 0)
+		return -1;


This should probably be if ((error = git_buf_joinpath(&path, workdir, GIT_MODULES_FILE)) < 0) return error;

pks-t · 2018-03-26T09:53:39Z

+	if (git_buf_joinpath(&path, workdir, GIT_MODULES_FILE) != 0)
+		return -1;
+
+	error = git_config_open_ondisk(&mods, path.ptr);


You should probably also check the return value here. While an error will cause mods to be NULL, which is catched in a few lines, it feels a bit more robust to check directly. You could avoid multiple free's by adding a goto out. Additional kudos if you also convert the other error handling branches to goto out with a unified section for freeing up memory in case error != 0.

pks-t · 2018-03-26T09:53:58Z

+
+	cl_git_fail(git_submodule_foreach(g_repo, sm_lookup_cb, &data));
+}
+


There's a trailing newline here

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

ethomson · 2018-04-17T13:32:27Z

Thanks for the contribution, @csware!

pks-t requested changes Feb 8, 2018

View reviewed changes

csware changed the title ~~Submodules should report parse errors (WIP)~~ Submodules should report parse errors Feb 8, 2018

csware changed the title ~~Submodules should report parse errors~~ Submodules should report .gitmodules parse errors Feb 9, 2018

csware changed the title ~~Submodules should report .gitmodules parse errors~~ Submodules-API should report .gitmodules parse errors instead of ignoring them Feb 9, 2018

pks-t requested changes Feb 9, 2018

View reviewed changes

pks-t requested changes Feb 16, 2018

View reviewed changes

pks-t requested changes Mar 26, 2018

View reviewed changes

pks-t added the feedback provided label Mar 26, 2018

Submodule API should report .gitmodules parse errors

e55b537

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

ethomson merged commit 286a676 into libgit2:master Apr 17, 2018

csware deleted the submodules-should-report-parse-errors branch April 17, 2018 13:48

pks-t added backport-0.27.1 and removed feedback provided labels Apr 20, 2018

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

		@@ -1916,31 +1921,32 @@ static int submodule_load_from_wd_lite(git_submodule *sm)

		/**
		* Returns a snapshot of $WORK_TREE/.gitmodules.


		cl_git_fail(git_submodule_foreach(g_repo, sm_lookup_cb, &data));
		}

Uh oh!

Conversation

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csware commented Feb 9, 2018

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!

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 Feb 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csware Feb 16, 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!

csware commented Mar 11, 2018

Uh oh!

ethomson commented Mar 11, 2018

Uh oh!

ethomson commented Mar 11, 2018

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!

Choose a reason for hiding this comment

Uh oh!

ethomson commented Apr 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

csware commented Feb 8, 2018 •

edited

Loading

csware Feb 16, 2018 •

edited

Loading