Re: [PATCH 04/10] evolve: add support for parsing metacommits

>>> 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.
>>> diff --git a/Makefile b/Makefile
>>> index cac3452edb9..b2bcc00c289 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -999,6 +999,7 @@ LIB_OBJS += merge-ort.o
>>>    LIB_OBJS += merge-ort-wrappers.o
>>>    LIB_OBJS += merge-recursive.o
>>>    LIB_OBJS += merge.o
>>> +LIB_OBJS += metacommit-parser.o
>>
>> There seems to be a problem with the indent here
> 
> I'm not sure I follow, there's not indentation on that line?

>>> diff --git a/metacommit-parser.c b/metacommit-parser.c
>>> new file mode 100644
>>> index 00000000000..70c1428bfc6
>>> --- /dev/null
>>> +++ b/metacommit-parser.c
>>> @@ -0,0 +1,110 @@
>>> +#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.
>>> + */
>>
>> There is no explanation in the code or commit message as to why this
>> function is needed. The documentation added in the first commit says
>> that "parent-type" header is a commit header. I think the answer is that
>> this series does not implement that header but uses the commit message
>> instead. That's perfectly fine for a proof of concept but it is
>> precisely the sort of detail that should be described it the commit
>> message and probably flagged up in the cover letter.
> 
> I admit I thought I thought this was part of the header because it
> shows up before
> the blank line before the commit title.

>>> +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 && !memcmp(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--;
>>> +     }
>>> +
>>> +     if (!to_search)
>>> +             return NULL;
>>> +
>>> +     return to_search->item;
>>> +}
>>
>> This function is a useful utility for struct commit_list and should live
>> in commit.c. It could be used to simplify object-name.c:get_parent() for
>> example.
> 
> Done.  I'll defer cleaning up get_parent to a potentially later change to avoid
> muddying up this change too much.