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

[Peter Tyser <ptyser gmail com>]

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:
>> Patches! that look good! and have commit messages! You're my hero for the day.
>
> I aim to please!:)
>
>> On 1 March 2010 06:42, Peter Tyser <ptyser gmail com> wrote:
>> <snip>
>>> @@ -39,12 +39,12 @@ class Vc(_vc.CachedVc):
>>>     PATCH_STRIP_NUM = 1
>>>     PATCH_INDEX_RE = "^diff --git a/(.*) b/.*$"
>>>     state_map = {
>>> -        "unknown":    _vc.STATE_NONE,
>>> -        "new file":   _vc.STATE_NEW,
>>> -        "deleted":    _vc.STATE_REMOVED,
>>> -        "modified":   _vc.STATE_MODIFIED,
>>> -        "typechange": _vc.STATE_MODIFIED,
>>> -        "unmerged":   _vc.STATE_CONFLICT,
>>> +        "X": _vc.STATE_NONE,     # Unknown
>>> +        "A": _vc.STATE_NEW,      # New
>>> +        "D": _vc.STATE_REMOVED,  # Deleted
>>> +        "M": _vc.STATE_MODIFIED, # Modified
>>> +        "T": _vc.STATE_MODIFIED, # Type-changed
>>> +        "U": _vc.STATE_CONFLICT, # Unmerged
>>>     }
>>
>> 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:

ptyser ptyser-laptop u-boot $ git mv Makefile Makefile.rename
ptyser ptyser-laptop u-boot $ cp MAINTAINERS MAINTAINERS.copy
ptyser ptyser-laptop u-boot $ git add MAINTAINERS.copy
ptyser ptyser-laptop u-boot $ echo "asdf" >> README
ptyser ptyser-laptop u-boot $ git st
# On branch master
# Your branch is ahead of 'origin/master' by 32 commits.
#
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	new file:   MAINTAINERS.copy
#	renamed:    Makefile -> Makefile.rename
#
# Changed but not updated:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   README

ptyser ptyser-laptop u-boot $ git diff-index --name-status HEAD
A	MAINTAINERS.copy
D	Makefile
A	Makefile.rename
M	README

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.

Also, using git diff-index resolves another bug with the current code.
 Take the following example:
ptyser ptyser-laptop u-boot $ git mv Makefile Makefile.move
ptyser ptyser-laptop u-boot $ echo "asdf" >> Makefile.move
ptyser ptyser-laptop u-boot $ git st
# On branch master
# Your branch is ahead of 'origin/master' by 32 commits.
#
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	renamed:    Makefile -> Makefile.move
#
# Changed but not updated:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   Makefile.move

The old code would parse this output from top to bottom.  Initially it
would correctly think Makefile.move was a new file.  But later it
would see "modified: Makefile.move" and relabel it as a modified file.
 The meld output would be incorrect in this instance.  "git
diff-index" doesn't have this issue:

ptyser ptyser-laptop u-boot $ git diff-index --name-status HEAD
D	Makefile
A	Makefile.move

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.

Best,
Peter