Re: [PATCH 1/2] Update git to use "git diff-index" instead of "git status"

[Kai Willadsen <kai willadsen gmail com>]

On 2 March 2010 14:46, Peter Tyser <ptyser gmail com> wrote:
> On Mon, Mar 1, 2010 at 1:41 AM, Peter Tyser <ptyser gmail com> wrote:
>> On Mon, Mar 1, 2010 at 12:38 AM, Kai Willadsen <kai willadsen gmail com> wrote:
>>> So this won't handle renames or copies. The old code treated rename as
>>> new/deleted, and a copy as a straight new. I guess adding --no-renames
>>> to the command line would give us the same result for the rename case?
>>> I couldn't find an equivalent for no copies, but I'm sure it's around
>>> somewhere.
>>
>> Ahh, good point.  The patch handles renames and copies, but just as
>> the addition/deletion of files.  It'd be much nicer to detect the
>> rename/copy and create a relevant diff as you suggest.  I'm pretty
>> sure "git diff-index -M" should do the trick.  This would have the
>> same functionality as the previous code where renames, but not copies
>> are detected.
>>
>> It could be argued that detecting copies would be nice too (eg add -C
>> to "git diff-index").  Git can detect copied files with changes, so
>> the diff might actually be usefully when copying and slightly
>> modifying a file from the original.  Let me know if you have any
>> issues with this operation as it is slightly different from the
>> original code.
>>
>> I'll rework the patch and resubmit.  Thanks for the comments,
>
> I just looked into it again, and I think this patch should not change
> the functionality of the old code.  My patch also treats a rename as a
> new/deleted, and a copy as a straight new.  For example:
<snip>

Yes, I see the same behaviour. I was worried, since git-diff-index
*can* return R and C in addition to the statuses that you handle. The
git-diff-index manpage talks about the option:
    --no-renames
           Turn off rename detection, even when the configuration file gives
           the default to do so.
which suggested to me that there was a way to turn on rename detection
through git config. I can't find a definitive answer, but from my
experiments the git config diff.renames setting doesn't affect
git-diff-index. So I guess it's okay to not handle R and C statuses,
as long as we never ask for them...

> Last night I was under the impression that it would be easy to add the
> ability to detect a rename/copy, and then provide a diff of any
> changes to the renamed/copied file.  This looks like it would be more
> difficult than I thought since now there is a 1-to-1 correspondence
> between a file and state.  In order to detect and display modified
> renames/copies you'd need to have 1 state correspond to 2 files - the
> original plus the modified renamed/copied file.  I can look into how
> hard this would be, but I'd prefer to do it in a follow-up patch if at
> all.

I didn't actually mean to suggest this. While it would potentially be
very cool, I think our version control backend really isn't up to it
at the moment.

> Also, using git diff-index resolves another bug with the current code.
<snip>
>  The meld output would be incorrect in this instance.  "git
> diff-index" doesn't have this issue:

Nice.

> Sorry for the long-winded email.  Long story short, I think the
> current patch is an improvement as is.  I won't resubmit unless
> someone sees a flaw in the logic above.

While I'd like a definitive answer on the rename/copy detection, I'm
inclined to agree. I'll probably poke it a little bit more and then
push. Thanks.

cheers,
Kai