DRY commit parsing #4445

tiennou · 2017-12-13T00:35:52Z

Splitted from #4331. See this PR for context.

This adds a "quick" mode to our commit parsing machinery, and makes the revwalk code instead of going with its own. The outcome is a nice, single place where grafts can be handled.

pks-t

I like the code deduplication here, thanks for that! Despite my comments, there seems to be a reproducible segfault on Win64 platforms. At least both W64 builds have segfaulted at the exact same point.

pks-t · 2017-12-15T12:44:20Z

src/commit_list.c

 #include "odb.h"
+#include "commit.h"
+#include "signature.h"


I couldn't really see why repository.h and signature.h are required now. At least that's not obvious from the diff.

Leftover from a previous attempt, where I tried to make that parser use already existing functions, before I noticed the "main" parse code. Will remove if that's unwarranted (I don't think it is).

pks-t · 2017-12-15T12:47:22Z

src/commit.c

@@ -19,6 +19,7 @@
 #include "message.h"


I think the commits are out of order. You first use the new generic mechanism and introduce it only in the next commit

Yes, I caused some historic wreckage when doing some rebases, so when date-sorting, the commits are reverted. I already tried to fix it but failed, so I think I'll end up extracting patches and reapplying manually.

pks-t · 2017-12-15T12:50:43Z

src/commit.c

+		buffer += strlen("tree ") + GIT_OID_HEXSZ + 1;
+	}
+
+	if (flags & (GIT_COMMIT_PARSE_ALL|GIT_COMMIT_PARSE_QUICK)) {


Can't this condition just be removed? You're doing it for all flags anyway, so no real need to check for those flags.

I did that mostly for completeness — that makes it easier to spot the differences between ALL and QUICK, and also (somewhat) future-proof. I can remove though, if that's preferred.

pks-t · 2017-12-15T12:52:21Z

src/commit.c

@@ -428,11 +441,16 @@ int git_commit__parse(void *_commit, git_odb_object *odb_obj)
 	}

 	/* Always parse the committer; we need the commit time */
-	commit->committer = git__malloc(sizeof(git_signature));
-	GITERR_CHECK_ALLOC(commit->committer);
+	if (flags & (GIT_COMMIT_PARSE_ALL|GIT_COMMIT_PARSE_QUICK)) {


Same here -- no need for the conditional if you do so for all available flags anyway.

pks-t · 2017-12-15T12:53:21Z

src/commit.h

@@ -36,4 +36,11 @@ struct git_commit {
 void git_commit__free(void *commit);
 int git_commit__parse(void *commit, git_odb_object *obj);

+typedef enum {
+	GIT_COMMIT_PARSE_QUICK = (1 << 0),


You should add some documentation here stating what those two flags mean and what information exactly is not going to get parsed for QUICK.

Right, will do.

tiennou · 2017-12-15T17:20:58Z

git rebase master --ignore-date did the trick w.r.t ordering. I'll investigate the W64 crash, but I'm a slight disadvantage here…

pks-t · 2018-01-12T15:46:58Z

I've debugged and fixed the issue. You were underallocating the commit object due to using git_odb_object_size instead of git_object__size.

tiennou · 2018-01-17T00:21:08Z

@pks-t Many thanks for the fix ! 🌟

src/commit.c

tiennou · 2018-02-12T23:50:14Z

There you go !

pks-t

Sorry, this was lingering a lot longer than intended. Hope the additional remark isn't too frustrating after such a long time.

pks-t · 2019-09-27T09:24:10Z

src/commit.h

@@ -37,4 +37,11 @@ void git_commit__free(void *commit);
 int git_commit__parse(void *commit, git_odb_object *obj);
 int git_commit__parse_raw(void *commit, const char *data, size_t size);

+typedef enum {
+	GIT_COMMIT_PARSE_QUICK = (1 << 0), /**< Only parse parents and committer info */
+	GIT_COMMIT_PARSE_ALL   = (1 << 1), /**< Parse all commit data */


So... do these two flags really make sense? After all, you're not using PARSE_QUICK anywhere in git_commit__parse_internal and in fact PARSE_QUICK and PARSE_ALL do not make any sense together, right? I guess having only one of them would be more sensible, and I lean towards retaining PARSE_QUICK. In that case, default behaviour would be to parse everything and by passing the optional PARSE_QUICK flag we would skip certain information

pks-t · 2019-09-27T09:28:13Z

src/commit.c

@@ -383,24 +384,32 @@ int git_commit_amend(
 	return error;
 }

-int git_commit__parse_raw(void *_commit, const char *data, size_t size)
+static int git_commit__parse_internal(git_commit *commit, const char *data, size_t size, git_commit__parse_flags flags)


Well, I now realize that this really accepts an enum, only. I first thought that this was a bitfield due to its naming (flags instead of flag). So another route to make this a tad easier to grasp might be to just rename it to git_commit__parse_mode mode, as it's not really about flags at all

This allows us to pick which data from a commit we're interested in. This will be used by the revwalk code, which is only interested in parents' and committer data.

pks-t

I've changed the flags parameter to be an unsigned int to fix problems with or'ing multiple enums with C++ compilers, removed the GIT_COMMIT_PARSE_ALL enum which was redundant and fixed two memory allocation issues. This is good to go now, will merge as soon as CI is green

pks-t · 2019-10-03T07:29:25Z

Meh, there's some fallout from recent changes (e.g. changing to Werror on Win32, deprecation of git_object__size). I'll follow up shortly to fix these issues.

pks-t · 2019-10-03T08:11:39Z

@ethomson: want to give this another round of reviews or may I just proceed and merge?

ethomson · 2019-10-03T09:16:03Z

src/commit_list.c


-		buffer += parent_len;
+	if ((error = git_commit__parse_ext(commit, obj, GIT_COMMIT_PARSE_QUICK)) < 0 ||
+	    !git__is_uint16(git_array_size(commit->parent_ids))) {


We should set an error message if git__is_uint16 is false.

I’d recommend restructuring this a bit to preserve the parse error in case we want to start using more exotic error codes, but that’s rather optional.

Yup, thanks for catching! I now set an error message and also restructured to retain the error code.

ethomson · 2019-10-03T09:18:04Z

Thanks for the ping. One minor request but otherwise 👍

tiennou · 2019-10-03T09:58:06Z

src/commit_list.c

-		commit->parents[i] = git_revwalk__commit_lookup(walk, &oid);
-		if (commit->parents[i] == NULL)
-			return -1;
+	commit = git__calloc(1, sizeof(*commit));


I have a few "I don't remember" commits related to that, as fixups in the actual shallow implementation, here and here. AFAIR, I was getting asserts/crashes in cache.c, until I made the initialization like this. But it's fuzzy now, and I think I wasn't able to extract a testcase for it anyways. Now that I'm thinking about it, maybe that's because the ODB memory semantics being weird 😆 ?

They are! That's why we've deprecated the git_object__size thingy, because nobody was able to remember what it actually does. And in most cases, it did not do what was intended.

The commit list's in- and out-degrees are currently stored as `unsigned short`. When assigning it the value of `git_array_size`, which returns an `size_t`, this generates a warning on some Win32 platforms due to loosing precision. We could just cast the returned value of `git_array_size`, which would work fine for 99.99% of all cases as commits typically have less than 2^16 parents. For crafted commits though we might end up with a wrong value, and thus we should definitely check whether the array size actually fits into the field. To ease the check, let's convert the fields to store the degrees as `uint16_t`. We shouldn't rely on such unspecific types anyway, as it may lead to different behaviour across platforms. Furthermore, this commit introduces a new `git__is_uint16` function to check whether it actually fits -- if not, we return an error.

pks-t · 2019-10-03T10:26:25Z

@ethomson: amended to fix your remarks, thanks!

ethomson · 2019-10-03T10:53:36Z

:squirrel:

pks-t · 2019-10-03T10:56:10Z

Thanks a lot, both to you @tiennou for working on this and @ethomson for the review!

tiennou · 2019-10-03T10:59:24Z

Thanks for last-mile touch ups @pks-t ! Glad to see duplicated code go away 😉.

This was referenced Dec 13, 2017

Graft support #4446

Closed

Shallow support #4331

Open

pks-t requested changes Dec 15, 2017

View reviewed changes

tiennou force-pushed the shallow/dry-commit-parsing branch from a77f9b3 to 80d0eca Compare December 15, 2017 17:13

tiennou force-pushed the shallow/dry-commit-parsing branch from 80d0eca to 22aafe3 Compare December 15, 2017 17:27

pks-t approved these changes Jan 25, 2018

View reviewed changes

ethomson requested changes Feb 2, 2018

View reviewed changes

src/commit.c Outdated Show resolved Hide resolved

src/commit.c Outdated Show resolved Hide resolved

tiennou force-pushed the shallow/dry-commit-parsing branch from ad936af to 2a88f8c Compare February 2, 2018 23:50

ethomson reviewed Feb 9, 2018

View reviewed changes

src/commit.c Show resolved Hide resolved

tiennou force-pushed the shallow/dry-commit-parsing branch from 5ea2118 to a6aac31 Compare April 22, 2018 13:30

tiennou force-pushed the shallow/dry-commit-parsing branch from a6aac31 to 2c70a29 Compare July 1, 2018 10:28

tiennou force-pushed the shallow/dry-commit-parsing branch from 2c70a29 to 3f6ce50 Compare August 2, 2018 17:14

tiennou force-pushed the shallow/dry-commit-parsing branch 4 times, most recently from ce657ea to cd6f417 Compare October 25, 2018 21:44

tiennou force-pushed the shallow/dry-commit-parsing branch from cd6f417 to 2bb3111 Compare February 3, 2019 09:22

tiennou force-pushed the shallow/dry-commit-parsing branch from 2bb3111 to e675aef Compare May 21, 2019 23:05

pks-t requested changes Sep 27, 2019

View reviewed changes

pks-t reviewed Sep 27, 2019

View reviewed changes

commit: generic parse mechanism

1c847a6

This allows us to pick which data from a commit we're interested in. This will be used by the revwalk code, which is only interested in parents' and committer data.

pks-t force-pushed the shallow/dry-commit-parsing branch 2 times, most recently from e153232 to 93288fb Compare October 3, 2019 06:45

pks-t approved these changes Oct 3, 2019

View reviewed changes

commit_list: unify commit information parsing

5988cf3

pks-t force-pushed the shallow/dry-commit-parsing branch from 93288fb to 5988cf3 Compare October 3, 2019 06:59

ethomson reviewed Oct 3, 2019

View reviewed changes

tiennou commented Oct 3, 2019

View reviewed changes

pks-t force-pushed the shallow/dry-commit-parsing branch from a46ad20 to 5cf17e0 Compare October 3, 2019 10:24

ethomson approved these changes Oct 3, 2019

View reviewed changes

pks-t merged commit f04a58b into libgit2:master Oct 3, 2019

tiennou deleted the shallow/dry-commit-parsing branch October 3, 2019 10:59

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 Mar 12, 2020

[Snyk] Upgrade nodegit from 0.26.0 to 0.26.4 thisconnect/nodegit-kit#95

Merged

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

DRY commit parsing #4445

DRY commit parsing #4445

Uh oh!

Conversation

tiennou commented Dec 13, 2017

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!

tiennou Dec 15, 2017 • 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!

tiennou commented Dec 15, 2017

Uh oh!

pks-t commented Jan 12, 2018

Uh oh!

tiennou commented Jan 17, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tiennou commented Feb 12, 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!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

pks-t commented Oct 3, 2019

Uh oh!

pks-t commented Oct 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethomson commented Oct 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t commented Oct 3, 2019

Uh oh!

ethomson commented Oct 3, 2019

Uh oh!

pks-t commented Oct 3, 2019

Uh oh!

tiennou commented Oct 3, 2019

Uh oh!

Uh oh!

tiennou Dec 15, 2017 •

edited

Loading