Re: [PATCH v3 00/33] Add directory rename detection to git

[Stefan Beller <sbeller@google.com>]

On Tue, Nov 21, 2017 at 5:12 PM, Elijah Newren <newren@gmail.com> wrote:
> On Tue, Nov 21, 2017 at 4:42 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Tue, Nov 21, 2017 at 12:00 AM, Elijah Newren <newren@gmail.com> wrote:
>>> This patchset introduces directory rename detection to merge-recursive; I'm
>>> resubmitting just a few hours after my PATCHv2 because I didn't know about
>>> the DEVELOPER=1 flag previously, and my code had a number of
>>> warnings/errors.  I would have just submitted fixup/squash patches, but
>>> when I checked, there sadly they cause merge conflicts when rebasing
>>>
>>> See https://public-inbox.org/git/20171110190550.27059-1-newren@gmail.com/
>>> for the first series, design considerations, etc, and
>>> https://public-inbox.org/git/20171120220209.15111-1-newren@gmail.com/ for
>>> v2.
>>
>> Thanks, I'll take a look!
>>
>> Protip: To make it easy for reviewers add an interdiff[1] between the different
>> versions of the patch series, this can be done via tbdiff[2] for example,
>> or in case you still have the old branch around or Junio has it queued already,
>> you can do a diff against that branch.
>
> Thanks!
>
> Interesting; tbdiff looks cool.  Junio hasn't queued this series yet,
> but it's easy enough to reconstruct the old one.  It does weigh in
> pretty heavy, and I'm slighly worried about gmail mangling all the
> lines, but I'm going to give it a shot anyway.  If it's too mangled,
> I'll try to repost using git-send-email.  It does weigh in at over
> 1600 lines, so it's not small.

In my first round of review I only looked over the tests to see if I'd
find the behavior intuitive, I spared the implementation, as Junio seemed
to have reviewed a couple patches of the v1 implementation.

Now I also looked over the implementation and quite like it, though
I'd be happy if others would also have a look.

All but one comment were minor style nits, which are no big deal;
the other remark that I was musing about was whether we want to use
strbufs in the new code instead of e.g. sprintfs to extend strings.
And I'd think we would want to use them unless there are compelling
reasons not to.

Thanks,
Stefan