Re: [RFC/PATCH] patch-ids: check modified paths before calculating diff

[Jonathan Nieder <jrnieder@gmail.com>]

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