John Keeping wrote:
> On Sun, May 19, 2013 at 12:36:12PM -0700, Jonathan Nieder wrote:
>> Who is responsible for freeing
>> "path"? Would it make sense to use a strbuf here?
>>
>> strbuf_setlen(&buf, traverse_path_len(info, n));
>> make_traverse_path(&buf.buf, info, n);
>
> Perhaps alloc_traverse_path? I'm not sure the strbuf buys us much for
> this case, since we only use the result for a few lines before freeing
> it.
Good idea. Generally strbufs are nice for this kind of thing because
they make it easy to reuse buffers, but in this case there's no
opportunity for that.
[...]
> then this is "process_changed_path".
Sounds good.
[...]
>> What should happen for commits with more than 1 parent? If they're
>> supposed to not enter this codepath because of a previous check, a
>> die("BUG: ...") could be a good way to make reading easier.
>
> Currently the patch ID code compares the commit with its first parent,
> so I think this should do the same.
Ok. I guess a comment nearby would help future readers avoid the same
question.
I don't know what it should mean to try to use --cherry without
--no-merges or --first-parent, so I guess this is harmless.
[...]
> Thanks for the review.
No problem. Thanks for a pleasant read.
Ciao,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html