[v3,4/8] evolve: Add support for parsing metacommits

Commit Message

Stefan Xenos Jan. 27, 2019, 7:44 p.m. UTC

From: Stefan Xenos <[e-mail]>

This patch adds the get_metacommit_content method, which can classify
commits as either metacommits or normal commits, determine whether they
are abandoned, and extract the content commit's object id from the
metacommit.

Signed-off-by: Stefan Xenos <[e-mail]>
---
 Makefile            |  1 +
 metacommit-parser.c | 87 +++++++++++++++++++++++++++++++++++++++++++++
 metacommit-parser.h | 16 +++++++++
 3 files changed, 104 insertions(+)
 create mode 100644 metacommit-parser.c
 create mode 100644 metacommit-parser.h

Comments

Junio C Hamano Jan. 28, 2019, 2:05 a.m. UTC | #1

[e-mail] writes:

> +/*
> + * Search the commit buffer for a line starting with the given key. Unlike
> + * find_commit_header, this also searches the commit message body.
> + */
> +static const char *find_key(const char *msg, const char *key, size_t *out_len)
> +{
> +	int key_len = strlen(key);
> +	const char *line = msg;
> +
> +	while (line) {
> +		const char *eol = strchrnul(line, '\n');
> +
> +		if (eol - line > key_len &&
> +				!strncmp(line, key, key_len) &&

Use of strncmp() here forces readers to wonder if and why we are
preparing the code to allow NUL in key[0..key_len] (as line..eol
would not, because we just used strchrnul()).  But because key_len
was computed using strlen(key), there is no valid reason to do so.
If the code used memcmp(), it won't waste readers' time.

> +				line[key_len] == ' ') {
> +			*out_len = eol - line - key_len - 1;
> +			return line + key_len + 1;
> +		}
> +		line = *eol ? eol + 1 : NULL;
> +	}
> +	return NULL;
> +}
> +
> +static struct commit *get_commit_by_index(struct commit_list *to_search, int index)
> +{
> +	while (to_search && index) {
> +		to_search = to_search->next;
> +		--index;

Style: unlike some C++ shop, we tend to use post-increment/decrement
when we do not use the value.

> +	}
> +
> +	return to_search->item;
> +}

If a maliciously crafted parent-type field has excess ' ' to make
index larger than the number of "parent" field in the commit object,
the while() loop terminates upon noticing that to_search has become
NULL.  And then this return statement dereferences that NULL
pointer.

> +/*
> + * Writes the content parent's object id to "content".
> + * Returns the metacommit type. See the METACOMMIT_TYPE_* constants.
> + */
> +int get_metacommit_content(
> +	struct commit *commit, struct object_id *content)
> +{
> +	const char *buffer = get_commit_buffer(commit, NULL);
> +	size_t parent_types_size;
> +	const char *parent_types = find_key(buffer, "parent-type",
> +		&parent_types_size);
> +	const char *end;
> +	int index = 0;
> +	int ret;
> +	struct commit *content_parent;
> +
> +	if (!parent_types) {
> +		return METACOMMIT_TYPE_NONE;

Unnecessary brace?

> +	}
> +
> +	end = &(parent_types[parent_types_size]);

Unnecessary parenthesis?

> +	while (1) {
> +		char next = *parent_types;
> +		if (next == ' ') {
> +			index++;
> +		}
> +		if (next == 'c') {
> +			ret = METACOMMIT_TYPE_NORMAL;
> +			break;
> +		}
> +		if (next == 'a') {
> +			ret = METACOMMIT_TYPE_ABANDONED;
> +			break;
> +		}

The parsing of this seems somewhat loose.  If there is 'x' on the
line, this loop spins and consumes it without doing anything, which
means that the same commit with a parent-type field can be encoded
in different ways by adding arbitrary number of 'x' just after SP
after the "parent-type" keyword, no?

> +		parent_types++;
> +		if (parent_types >= end) {
> +			return METACOMMIT_TYPE_NONE;
> +		}
> +	}
> +
> +	content_parent = get_commit_by_index(commit->parents, index);
> +
> +	if (!content_parent) {
> +		return METACOMMIT_TYPE_NONE;
> +	}
> +
> +	oidcpy(content, &(content_parent->object.oid));
> +	return ret;
> +}
> diff --git a/metacommit-parser.h b/metacommit-parser.h
> new file mode 100644
> index 0000000000..e546f5a7e7
> --- /dev/null
> +++ b/metacommit-parser.h
> @@ -0,0 +1,16 @@
> +#ifndef METACOMMIT_PARSER_H
> +#define METACOMMIT_PARSER_H
> +
> +// Indicates a normal commit (non-metacommit)

No C99 // comments please.  Not in the header, and not in the code.

> +#define METACOMMIT_TYPE_NONE 0
> +// Indicates a metacommit with normal content (non-abandoned)
> +#define METACOMMIT_TYPE_NORMAL 1
> +// Indicates a metacommit with abandoned content
> +#define METACOMMIT_TYPE_ABANDONED 2
> +
> +struct commit;
> +
> +extern int get_metacommit_content(
> +	struct commit *commit, struct object_id *content);
> +
> +#endif

Patch

diff --git a/Makefile b/Makefile
index 1a44c811aa..7ffc383f2b 100644
--- a/Makefile
+++ b/Makefile
@@ -919,6 +919,7 @@  LIB_OBJS += merge.o
 LIB_OBJS += merge-blobs.o
 LIB_OBJS += merge-recursive.o
 LIB_OBJS += mergesort.o
+LIB_OBJS += metacommit-parser.o
 LIB_OBJS += midx.o
 LIB_OBJS += name-hash.o
 LIB_OBJS += negotiator/default.o
diff --git a/metacommit-parser.c b/metacommit-parser.c
new file mode 100644
index 0000000000..5013a108a3
--- /dev/null
+++ b/metacommit-parser.c
@@ -0,0 +1,87 @@ 
+#include "cache.h"
+#include "metacommit-parser.h"
+#include "commit.h"
+
+/*
+ * Search the commit buffer for a line starting with the given key. Unlike
+ * find_commit_header, this also searches the commit message body.
+ */
+static const char *find_key(const char *msg, const char *key, size_t *out_len)
+{
+	int key_len = strlen(key);
+	const char *line = msg;
+
+	while (line) {
+		const char *eol = strchrnul(line, '\n');
+
+		if (eol - line > key_len &&
+				!strncmp(line, key, key_len) &&
+				line[key_len] == ' ') {
+			*out_len = eol - line - key_len - 1;
+			return line + key_len + 1;
+		}
+		line = *eol ? eol + 1 : NULL;
+	}
+	return NULL;
+}
+
+static struct commit *get_commit_by_index(struct commit_list *to_search, int index)
+{
+	while (to_search && index) {
+		to_search = to_search->next;
+		--index;
+	}
+
+	return to_search->item;
+}
+
+/*
+ * Writes the content parent's object id to "content".
+ * Returns the metacommit type. See the METACOMMIT_TYPE_* constants.
+ */
+int get_metacommit_content(
+	struct commit *commit, struct object_id *content)
+{
+	const char *buffer = get_commit_buffer(commit, NULL);
+	size_t parent_types_size;
+	const char *parent_types = find_key(buffer, "parent-type",
+		&parent_types_size);
+	const char *end;
+	int index = 0;
+	int ret;
+	struct commit *content_parent;
+
+	if (!parent_types) {
+		return METACOMMIT_TYPE_NONE;
+	}
+
+	end = &(parent_types[parent_types_size]);
+
+	while (1) {
+		char next = *parent_types;
+		if (next == ' ') {
+			index++;
+		}
+		if (next == 'c') {
+			ret = METACOMMIT_TYPE_NORMAL;
+			break;
+		}
+		if (next == 'a') {
+			ret = METACOMMIT_TYPE_ABANDONED;
+			break;
+		}
+		parent_types++;
+		if (parent_types >= end) {
+			return METACOMMIT_TYPE_NONE;
+		}
+	}
+
+	content_parent = get_commit_by_index(commit->parents, index);
+
+	if (!content_parent) {
+		return METACOMMIT_TYPE_NONE;
+	}
+
+	oidcpy(content, &(content_parent->object.oid));
+	return ret;
+}
diff --git a/metacommit-parser.h b/metacommit-parser.h
new file mode 100644
index 0000000000..e546f5a7e7
--- /dev/null
+++ b/metacommit-parser.h
@@ -0,0 +1,16 @@ 
+#ifndef METACOMMIT_PARSER_H
+#define METACOMMIT_PARSER_H
+
+// Indicates a normal commit (non-metacommit)
+#define METACOMMIT_TYPE_NONE 0
+// Indicates a metacommit with normal content (non-abandoned)
+#define METACOMMIT_TYPE_NORMAL 1
+// Indicates a metacommit with abandoned content
+#define METACOMMIT_TYPE_ABANDONED 2
+
+struct commit;
+
+extern int get_metacommit_content(
+	struct commit *commit, struct object_id *content);
+
+#endif