bug#27953: vc-region-history doesn't follow renames

bug#27953: vc-region-history doesn't follow renames

[Clément Pit--Claudel]

[-- Attachment #1.1.1: Type: text/plain, Size: 607 bytes --]

X-Debbugs-CC: monnier@iro.umontreal.ca

Hi bug-gnu-emacs,

vc-git-region-history currently stops at the first rename, instead of continuing across renames.  This is because it doesn't pass --follow to git.  The original author put the required option "--follow" in a comment, adding "FIXME: not supported?".

Indeed, `git log -p --follow -L5,25:file.c' doesn't work.  But `git log -p --follow -L5,25:file.c file.c' does work (note the extra mention of the file name).

Any arguments against the attached patch?

(CCing Stefan, since you're the last one who modified this function)

Clément.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: 0001-Follow-renames-in-vc-git-region-history.patch --]
[-- Type: text/x-patch; name="0001-Follow-renames-in-vc-git-region-history.patch", Size: 1243 bytes --]

From 7a1c945645e6ea3c995ba21b4288b7a048340e67 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Pit--Claudel?= <clement.pitclaudel@live.com>
Date: Fri, 4 Aug 2017 17:03:20 +0200
Subject: [PATCH] Follow renames in vc-git-region-history

* lisp/vc/vc-git.el (vc-git-region-history): Pass --follow.  This
requires a path spec, so pass that too.
---
 lisp/vc/vc-git.el | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index cc3e295..6830951 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1159,8 +1159,10 @@ vc-git-region-history
                 (- (+ curno curcnt) (+ headno headcnt)))))
       (setq lto (- lto (or to-offset last-offset)))
       (setq lfrom (- lfrom (or to-offset last-offset)))))
-  (vc-git-command buffer 'async nil "log" "-p" ;"--follow" ;FIXME: not supported?
-                  (format "-L%d,%d:%s" lfrom lto (file-relative-name file))))
+  (vc-git-command buffer 'async nil "log" "-p" "--follow"
+            (format "-L%d,%d:%s" lfrom lto (file-relative-name file))
+            ;; Must include one path spec for --follow to work
+            (file-relative-name file)))
 
 (require 'diff-mode)
 
-- 
2.7.4


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#27953: vc-region-history doesn't follow renames

[Stefan Monnier]

> Indeed, `git log -p --follow -L5,25:file.c' doesn't work.  But `git
> log -p --follow -L5,25:file.c file.c' does work (note the extra mention of
> the file name).

Hmm... what's the difference between

    git log -p -L5,25:file.c
and
    git log -p -L5,25:file.c file.c

?

Would this issue deserve a bug-report to Git?

> Any arguments against the attached patch?

I'd suggest to compute (file-relative-name file) only once, and to add
bug#27953 to the comment, but otherwise, looks good.


        Stefan

bug#27953: vc-region-history doesn't follow renames

[Andreas Schwab]

On Aug 04 2017, Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:

>> Indeed, `git log -p --follow -L5,25:file.c' doesn't work.  But `git
>> log -p --follow -L5,25:file.c file.c' does work (note the extra mention of
>> the file name).
>
> Hmm... what's the difference between
>
>     git log -p -L5,25:file.c
> and
>     git log -p -L5,25:file.c file.c

The latter is supposed to be invalid:

       -L <start>,<end>:<file>, -L :<funcname>:<file>
           Trace the evolution of the line range given by "<start>,<end>" (or
           the function name regex <funcname>) within the <file>. You may not
                                                                  ^^^^^^^^^^^
           give any pathspec limiters.
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^

> Would this issue deserve a bug-report to Git?

It does.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#27953: vc-region-history doesn't follow renames

[Clément Pit--Claudel]

[-- Attachment #1.1.1: Type: text/plain, Size: 775 bytes --]

On 2017-08-04 19:21, Stefan Monnier wrote:
>> Indeed, `git log -p --follow -L5,25:file.c' doesn't work.  But `git
>> log -p --follow -L5,25:file.c file.c' does work (note the extra mention of
>> the file name).
> 
> Hmm... what's the difference between
> 
>     git log -p -L5,25:file.c
> and
>     git log -p -L5,25:file.c file.c

The second one allows --follow; the first one doesn't.  I don't know what differences they may be beyond that :/

> Would this issue deserve a bug-report to Git?

Possibly; it's a bit intimidating.

>> Any arguments against the attached patch?
> 
> I'd suggest to compute (file-relative-name file) only once, and to add
> bug#27953 to the comment, but otherwise, looks good.

Attached an updated version :)

Clément.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: 0001-Follow-renames-in-vc-git-region-history.patch --]
[-- Type: text/x-patch; name="0001-Follow-renames-in-vc-git-region-history.patch", Size: 1288 bytes --]

From 78959b7c2bb7a13c4ab965e2617e3e9eec127b35 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Pit--Claudel?= <clement.pitclaudel@live.com>
Date: Fri, 4 Aug 2017 17:03:20 +0200
Subject: [PATCH] Follow renames in vc-git-region-history

* lisp/vc/vc-git.el (vc-git-region-history): Pass --follow.  This
requires a path spec, so pass that too.
---
 lisp/vc/vc-git.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index cc3e295..7d13dfe 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1159,8 +1159,11 @@ vc-git-region-history
                 (- (+ curno curcnt) (+ headno headcnt)))))
       (setq lto (- lto (or to-offset last-offset)))
       (setq lfrom (- lfrom (or to-offset last-offset)))))
-  (vc-git-command buffer 'async nil "log" "-p" ;"--follow" ;FIXME: not supported?
-                  (format "-L%d,%d:%s" lfrom lto (file-relative-name file))))
+  (let ((file-relname (file-relative-name file)))
+    (vc-git-command buffer 'async nil "log" "-p" "--follow"
+              (format "-L%d,%d:%s" lfrom lto file-relname)
+              ;; Must include one path spec for --follow to work (#27953)
+              file-relname)))
 
 (require 'diff-mode)
 
-- 
2.7.4


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#27953: vc-region-history doesn't follow renames

[Kyle Meyer]

Clément Pit--Claudel <clement.pitclaudel@live.com> writes:

> On 2017-08-04 19:21, Stefan Monnier wrote:
>>> Indeed, `git log -p --follow -L5,25:file.c' doesn't work.  But `git
>>> log -p --follow -L5,25:file.c file.c' does work (note the extra mention of
>>> the file name).
>> 
>> Hmm... what's the difference between
>> 
>>     git log -p -L5,25:file.c
>> and
>>     git log -p -L5,25:file.c file.c
>
> The second one allows --follow; the first one doesn't.  I don't know what differences they may be beyond that :/

As Andreas mentioned, even though git doesn't throw an error, giving a
pathspec with -L is documented as invalid.

But you shouldn't need to add --follow here.  Renames should be followed
by default when -L is used.  (At least, that's what I see using Git
v2.13.3.)

-- 
Kyle

bug#27953: vc-region-history doesn't follow renames

[Eli Zaretskii]

> From: Kyle Meyer <kyle@kyleam.com>
> Date: Sat, 05 Aug 2017 13:05:52 -0400
> Cc: Andreas Schwab <schwab@linux-m68k.org>,
> 	Stefan Monnier <monnier@IRO.UMontreal.CA>, 27953@debbugs.gnu.org
> 
> But you shouldn't need to add --follow here.  Renames should be followed
> by default when -L is used.  (At least, that's what I see using Git
> v2.13.3.)

I have 2.10 here, so maybe this was fixed later, but I don't see
renames followed when -L is used.  E.g., try this:

  git log -L77,83:admin/charsets/Makefile.in

You will see Git claiming the only version which changed that is the
one in which Makefile.in was created from Makefile.  Am I doing
something wrong, or expecting too much?

bug#27953: vc-region-history doesn't follow renames

[Andreas Schwab]

On Aug 05 2017, Eli Zaretskii <eliz@gnu.org> wrote:

> I have 2.10 here, so maybe this was fixed later, but I don't see
> renames followed when -L is used.  E.g., try this:
>
>   git log -L77,83:admin/charsets/Makefile.in
>
> You will see Git claiming the only version which changed that is the
> one in which Makefile.in was created from Makefile.  Am I doing
> something wrong, or expecting too much?

The problem is that admin/charsets/Makefile.in is only similar to
admin/charsets/Makefile by 32%, much below the default threshold.  Try
lisp/obsolete/html2text.el instead, for example.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#27953: vc-region-history doesn't follow renames

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Kyle Meyer <kyle@kyleam.com>,  clement.pitclaudel@live.com,  monnier@IRO.UMontreal.CA,  27953@debbugs.gnu.org
> Date: Sat, 05 Aug 2017 20:03:15 +0200
> 
> >   git log -L77,83:admin/charsets/Makefile.in
> >
> > You will see Git claiming the only version which changed that is the
> > one in which Makefile.in was created from Makefile.  Am I doing
> > something wrong, or expecting too much?
> 
> The problem is that admin/charsets/Makefile.in is only similar to
> admin/charsets/Makefile by 32%, much below the default threshold.

They look very similar to me...

What about doc/emacs/Makefile.in?  That should be almost 100% similar
to its origin in man/Makefile.in, when it was moved from there, but
"git -L" still cannot cross the boundary.

bug#27953: vc-region-history doesn't follow renames

[Kyle Meyer]

Eli Zaretskii <eliz@gnu.org> writes:

> What about doc/emacs/Makefile.in?  That should be almost 100% similar
> to its origin in man/Makefile.in, when it was moved from there, but
> "git -L" still cannot cross the boundary.

I believe the issue here is that the file was deleted in one commit
(19e364e29) and then added in another (8cf51b2c2).

-- 
Kyle

bug#27953: vc-region-history doesn't follow renames

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 1033 bytes --]

On 2017-08-05 19:05, Kyle Meyer wrote:
> Clément Pit--Claudel <clement.pitclaudel@live.com> writes:
>> On 2017-08-04 19:21, Stefan Monnier wrote:
>>>> Indeed, `git log -p --follow -L5,25:file.c' doesn't work.  But `git
>>>> log -p --follow -L5,25:file.c file.c' does work (note the extra mention of
>>>> the file name).
>>>
>>> Hmm... what's the difference between
>>>
>>>     git log -p -L5,25:file.c
>>> and
>>>     git log -p -L5,25:file.c file.c
>>
>> The second one allows --follow; the first one doesn't.  I don't know what differences they may be beyond that :/
> 
> As Andreas mentioned, even though git doesn't throw an error, giving a
> pathspec with -L is documented as invalid.

Thanks. I'm glad I sent the patch for review.

> But you shouldn't need to add --follow here.  Renames should be followed
> by default when -L is used.  (At least, that's what I see using Git
> v2.13.3.)

This seems to be new. Adding --follow appears to make git follow renames here (2.7.4).

Thanks.
Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#27953: vc-region-history doesn't follow renames

[Kyle Meyer]

Clément Pit--Claudel <clement.pitclaudel@live.com> writes:

> On 2017-08-05 19:05, Kyle Meyer wrote:

[...]

>> But you shouldn't need to add --follow here.  Renames should be followed
>> by default when -L is used.  (At least, that's what I see using Git
>> v2.13.3.)
>
> This seems to be new. Adding --follow appears to make git follow renames here (2.7.4).

OK, just to check with a concrete example: When you run

  git log -L37,57:test/lisp/ls-lisp-tests.el 9df49cddae

565cfd9f6 isn't listed as the earliest commit?

When I run that with Git 2.8.5 (and 2.13.3), I see 565cfd9f6 as the
earliest commit, indicating that Git does detect the rename
(test/lisp/ls-lisp.el => test/lisp/ls-lisp-tests.el).  I wasn't able to
try with your version, 2.7.4, because I couldn't get it to build.

-- 
Kyle

bug#27953: vc-region-history doesn't follow renames

[Andreas Schwab]

On Aug 05 2017, Kyle Meyer <kyle@kyleam.com> wrote:

> But you shouldn't need to add --follow here.  Renames should be followed
> by default when -L is used.  (At least, that's what I see using Git
> v2.13.3.)

Note that the default for diff.renames has changed in 2.9.  Earlier
versions may need to add -M.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."