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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 20, 2013, 06:36 UTC
Message-ID: <20130520063623.GB2919@elie.Belkin>
In-Reply-To: <20130519220206.GK27005@serenity.lan>

John Keeping wrote:

> On Sun, May 19, 2013 at 12:36:12PM -0700, Jonathan Nieder wrote:

Show 9 quoted lines

>>                                        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.

[...]

Show 6 quoted lines

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