Skip to content

DRY commit parsing #4445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pks-t merged 3 commits into from
Oct 3, 2019
Merged

DRY commit parsing #4445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c847a6
commit: generic parse mechanism
tiennou Oct 25, 2018
5988cf3
commit_list: unify commit information parsing
tiennou Dec 15, 2017
5cf17e0
commit_list: store in/out-degrees as uint16_t
pks-t Oct 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 33 additions & 11 deletions src/commit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "message.h"

@pks-t pks-t Dec 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tiennou tiennou Dec 15, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#include "refs.h"
#include "object.h"
#include "array.h"
#include "oidarray.h"

void git_commit__free(void *_commit)
Expand Down Expand Up @@ -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 commit_parse(git_commit *commit, const char *data, size_t size, unsigned int flags)
{
git_commit *commit = _commit;
const char *buffer_start = data, *buffer;
const char *buffer_end = buffer_start + size;
git_oid parent_id;
size_t header_len;
git_signature dummy_sig;

assert(commit && data);

buffer = buffer_start;

/* Allocate for one, which will allow not to realloc 90% of the time */
git_array_init_to_size(commit->parent_ids, 1);
GIT_ERROR_CHECK_ARRAY(commit->parent_ids);

/* The tree is always the first field */
if (git_oid__parse(&commit->tree_id, &buffer, buffer_end, "tree ") < 0)
goto bad_buffer;
if (!(flags & GIT_COMMIT_PARSE_QUICK)) {
if (git_oid__parse(&commit->tree_id, &buffer, buffer_end, "tree ") < 0)
goto bad_buffer;
} else {
size_t tree_len = strlen("tree ") + GIT_OID_HEXSZ + 1;
if (buffer + tree_len > buffer_end)
goto bad_buffer;
buffer += tree_len;
}

/*
* TODO: commit grafts!
Expand All @@ -413,11 +422,13 @@ int git_commit__parse_raw(void *_commit, const char *data, size_t size)
git_oid_cpy(new_id, &parent_id);
}

commit->author = git__malloc(sizeof(git_signature));
GIT_ERROR_CHECK_ALLOC(commit->author);
if (!(flags & GIT_COMMIT_PARSE_QUICK)) {
commit->author = git__malloc(sizeof(git_signature));
GIT_ERROR_CHECK_ALLOC(commit->author);

if (git_signature__parse(commit->author, &buffer, buffer_end, "author ", '\n') < 0)
return -1;
if (git_signature__parse(commit->author, &buffer, buffer_end, "author ", '\n') < 0)
return -1;
}

/* Some tools create multiple author fields, ignore the extra ones */
while (!git__prefixncmp(buffer, buffer_end - buffer, "author ")) {
Expand All @@ -435,6 +446,9 @@ int git_commit__parse_raw(void *_commit, const char *data, size_t size)
if (git_signature__parse(commit->committer, &buffer, buffer_end, "committer ", '\n') < 0)
return -1;

if (flags & GIT_COMMIT_PARSE_QUICK)
return 0;

/* Parse add'l header entries */
while (buffer < buffer_end) {
const char *eoln = buffer;
Expand Down Expand Up @@ -477,11 +491,19 @@ int git_commit__parse_raw(void *_commit, const char *data, size_t size)
return -1;
}

int git_commit__parse_raw(void *commit, const char *data, size_t size)
{
return commit_parse(commit, data, size, 0);
}

int git_commit__parse_ext(git_commit *commit, git_odb_object *odb_obj, unsigned int flags)
{
return commit_parse(commit, git_odb_object_data(odb_obj), git_odb_object_size(odb_obj), flags);
}

int git_commit__parse(void *_commit, git_odb_object *odb_obj)
{
return git_commit__parse_raw(_commit,
git_odb_object_data(odb_obj),
git_odb_object_size(odb_obj));
return git_commit__parse_ext(_commit, odb_obj, 0);
}
Comment thread
pks-t marked this conversation as resolved.

#define GIT_COMMIT_GETTER(_rvalue, _name, _return) \
Expand Down
6 changes: 6 additions & 0 deletions src/commit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,10 @@ 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_flags;

int git_commit__parse_ext(git_commit *commit, git_odb_object *odb_obj, unsigned int flags);

#endif
104 changes: 28 additions & 76 deletions src/commit_list.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "revwalk.h"
#include "pool.h"
#include "odb.h"
#include "commit.h"

int git_commit_list_time_cmp(const void *a, const void *b)
{
Expand Down Expand Up @@ -55,17 +56,6 @@ git_commit_list_node *git_commit_list_alloc_node(git_revwalk *walk)
return (git_commit_list_node *)git_pool_mallocz(&walk->commit_pool, 1);
}

static int commit_error(git_commit_list_node *commit, const char *msg)
{
char commit_oid[GIT_OID_HEXSZ + 1];
git_oid_fmt(commit_oid, &commit->oid);
commit_oid[GIT_OID_HEXSZ] = '\0';

git_error_set(GIT_ERROR_ODB, "failed to parse commit %s - %s", commit_oid, msg);

return -1;
}

static git_commit_list_node **alloc_parents(
git_revwalk *walk, git_commit_list_node *commit, size_t n_parents)
{
Expand Down Expand Up @@ -107,77 +97,42 @@ git_commit_list_node *git_commit_list_pop(git_commit_list **stack)

static int commit_quick_parse(
git_revwalk *walk,
git_commit_list_node *commit,
const uint8_t *buffer,
size_t buffer_len)
git_commit_list_node *node,
git_odb_object *obj)
{
const size_t parent_len = strlen("parent ") + GIT_OID_HEXSZ + 1;
const uint8_t *buffer_end = buffer + buffer_len;
const uint8_t *parents_start, *committer_start;
int i, parents = 0;
int64_t commit_time;

buffer += strlen("tree ") + GIT_OID_HEXSZ + 1;

parents_start = buffer;
while (buffer + parent_len < buffer_end && memcmp(buffer, "parent ", strlen("parent ")) == 0) {
parents++;
buffer += parent_len;
}

commit->parents = alloc_parents(walk, commit, parents);
GIT_ERROR_CHECK_ALLOC(commit->parents);

buffer = parents_start;
for (i = 0; i < parents; ++i) {
git_oid oid;

if (git_oid_fromstr(&oid, (const char *)buffer + strlen("parent ")) < 0)
return -1;
git_oid *parent_oid;
git_commit *commit;
int error;
size_t i;

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

@tiennou tiennou Oct 3, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😆 ?

@pks-t pks-t Oct 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

GIT_ERROR_CHECK_ALLOC(commit);
commit->object.repo = walk->repo;

buffer += parent_len;
if ((error = git_commit__parse_ext(commit, obj, GIT_COMMIT_PARSE_QUICK)) < 0) {
git__free(commit);
return error;
}

commit->out_degree = (unsigned short)parents;

if ((committer_start = buffer = memchr(buffer, '\n', buffer_end - buffer)) == NULL)
return commit_error(commit, "object is corrupted");

buffer++;

if ((buffer = memchr(buffer, '\n', buffer_end - buffer)) == NULL)
return commit_error(commit, "object is corrupted");

/* Skip trailing spaces */
while (buffer > committer_start && git__isspace(*buffer))
buffer--;

/* Seek for the beginning of the pack of digits */
while (buffer > committer_start && git__isdigit(*buffer))
buffer--;

/* Skip potential timezone offset */
if ((buffer > committer_start) && (*buffer == '+' || *buffer == '-')) {
buffer--;
if (!git__is_uint16(git_array_size(commit->parent_ids))) {
git__free(commit);
git_error_set(GIT_ERROR_INVALID, "commit has more than 2^16 parents");
return -1;
}

while (buffer > committer_start && git__isspace(*buffer))
buffer--;
node->time = commit->committer->when.time;
node->out_degree = (uint16_t) git_array_size(commit->parent_ids);
node->parents = alloc_parents(walk, node, node->out_degree);
GIT_ERROR_CHECK_ALLOC(node->parents);

while (buffer > committer_start && git__isdigit(*buffer))
buffer--;
git_array_foreach(commit->parent_ids, i, parent_oid) {
node->parents[i] = git_revwalk__commit_lookup(walk, parent_oid);
}

if ((buffer == committer_start) ||
(git__strntol64(&commit_time, (char *)(buffer + 1),
buffer_end - buffer + 1, NULL, 10) < 0))
return commit_error(commit, "cannot parse commit time");
git_commit__free(commit);

node->parsed = 1;

commit->time = commit_time;
commit->parsed = 1;
return 0;
}

Expand All @@ -196,10 +151,7 @@ int git_commit_list_parse(git_revwalk *walk, git_commit_list_node *commit)
git_error_set(GIT_ERROR_INVALID, "object is no commit object");
error = -1;
} else
error = commit_quick_parse(
walk, commit,
(const uint8_t *)git_odb_object_data(obj),
git_odb_object_size(obj));
error = commit_quick_parse(walk, commit, obj);

git_odb_object_free(obj);
return error;
Expand Down
4 changes: 2 additions & 2 deletions src/commit_list.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ typedef struct git_commit_list_node {
added:1,
flags : FLAG_BITS;

unsigned short in_degree;
unsigned short out_degree;
uint16_t in_degree;
uint16_t out_degree;

struct git_commit_list_node **parents;
} git_commit_list_node;
Expand Down
7 changes: 7 additions & 0 deletions src/integer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ GIT_INLINE(int) git__is_ssizet(size_t p)
return p == (size_t)r;
}

/** @return true if p fits into the range of a uint16_t */
GIT_INLINE(int) git__is_uint16(size_t p)
{
uint16_t r = (uint16_t)p;
return p == (size_t)r;
}

/** @return true if p fits into the range of a uint32_t */
GIT_INLINE(int) git__is_uint32(size_t p)
{
Expand Down