[Possible bug] diff-tree --stat info does not count copies

[Possible bug] diff-tree --stat info does not count copies

[Marco Costalba]

While testing qgit with the new rename/copy support  I found this
(possible) bug playing on git tree.

$ git-diff-tree -r --stat 6973dca
6973dcaee76ef7b7bfcabd2f26e76205aae07858
  Makefile                      |    2
 diff-files.c                  |  212 +----
 diff-lib.c                    | 1862 ++---------------------------------------
 diff.c                        | 1795 ++++++++++++++++++++++++++++++++++++++++
 diff.h                        |    7
 t/t1001-read-tree-m-2way.sh   |    2
 t/t1002-read-tree-m-u-2way.sh |    2
 7 files changed, 1929 insertions(+), 1953 deletions(-)

$ git-diff-tree -r --stat -C 6973dca
6973dcaee76ef7b7bfcabd2f26e76205aae07858
  Makefile                      |    2
 diff-files.c                  |  212 +----
 diff-lib.c                    | 1862 ++---------------------------------------
 diff-lib.c => diff.c          |    0
 diff.h                        |    7
 t/t1001-read-tree-m-2way.sh   |    2
 t/t1002-read-tree-m-u-2way.sh |    2
 7 files changed, 134 insertions(+), 1953 deletions(-)

IMHO the bug is

   "diff-lib.c => diff.c          |    0"

instead of

   "diff-lib.c => diff.c          |    1795"

because, after the patch applied, in the repository we have
1953-1929=24 lines of code more, not 1953-134= 1819 less.

Thanks
Marco

Re: [Possible bug] diff-tree --stat info does not count copies

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> While testing qgit with the new rename/copy support  I found this
> (possible) bug playing on git tree.
>
> $ git-diff-tree -r --stat 6973dca
> 6973dcaee76ef7b7bfcabd2f26e76205aae07858
>  Makefile                      |    2
> diff-files.c                  |  212 +----
> diff-lib.c                    | 1862 ++---------------------------------------
> diff.c                        | 1795 ++++++++++++++++++++++++++++++++++++++++
> diff.h                        |    7
> t/t1001-read-tree-m-2way.sh   |    2
> t/t1002-read-tree-m-u-2way.sh |    2
> 7 files changed, 1929 insertions(+), 1953 deletions(-)
>
> $ git-diff-tree -r --stat -C 6973dca
> 6973dcaee76ef7b7bfcabd2f26e76205aae07858
>  Makefile                      |    2
> diff-files.c                  |  212 +----
> diff-lib.c                    | 1862 ++---------------------------------------
> diff-lib.c => diff.c          |    0
> diff.h                        |    7
> t/t1001-read-tree-m-2way.sh   |    2
> t/t1002-read-tree-m-u-2way.sh |    2
> 7 files changed, 134 insertions(+), 1953 deletions(-)
>
> IMHO the bug is
>
>   "diff-lib.c => diff.c          |    0"
>
> instead of
>
>   "diff-lib.c => diff.c          |    1795"
>
> because, after the patch applied, in the repository we have
> 1953-1929=24 lines of code more, not 1953-134= 1819 less.

Interesting.  That's really a matter of taste and interpretation.

If it were a straight rename without changing a single line,
then would you say 1795 lines were removed (from the LHS file)
and 1795 lines were added (to the RHS file)?

I personally find that output would be useless and would prefer
it to say "I renamed file A to file B.  Content-wise, there were
N lines added and M lines removed, compared to the straight
rename case, by the way".

And that is what the current output does.  I do not see why it
should be different in the case of a copy instead of a rename.

Re: [Possible bug] diff-tree --stat info does not count copies

[Marco Costalba]

On 8/17/06, Junio C Hamano <junkio@cox.net> wrote:
> "Marco Costalba" <mcostalba@gmail.com> writes:
>
> > While testing qgit with the new rename/copy support  I found this
> > (possible) bug playing on git tree.
> >
> > $ git-diff-tree -r --stat 6973dca
> > 6973dcaee76ef7b7bfcabd2f26e76205aae07858
> >  Makefile                      |    2
> > diff-files.c                  |  212 +----
> > diff-lib.c                    | 1862 ++---------------------------------------
> > diff.c                        | 1795 ++++++++++++++++++++++++++++++++++++++++
> > diff.h                        |    7
> > t/t1001-read-tree-m-2way.sh   |    2
> > t/t1002-read-tree-m-u-2way.sh |    2
> > 7 files changed, 1929 insertions(+), 1953 deletions(-)
> >
> > $ git-diff-tree -r --stat -C 6973dca
> > 6973dcaee76ef7b7bfcabd2f26e76205aae07858
> >  Makefile                      |    2
> > diff-files.c                  |  212 +----
> > diff-lib.c                    | 1862 ++---------------------------------------
> > diff-lib.c => diff.c          |    0
> > diff.h                        |    7
> > t/t1001-read-tree-m-2way.sh   |    2
> > t/t1002-read-tree-m-u-2way.sh |    2
> > 7 files changed, 134 insertions(+), 1953 deletions(-)
> >
> > IMHO the bug is
> >
> >   "diff-lib.c => diff.c          |    0"
> >
> > instead of
> >
> >   "diff-lib.c => diff.c          |    1795"
> >
> > because, after the patch applied, in the repository we have
> > 1953-1929=24 lines of code more, not 1953-134= 1819 less.
>
> Interesting.  That's really a matter of taste and interpretation.
>
> If it were a straight rename without changing a single line,
> then would you say 1795 lines were removed (from the LHS file)
> and 1795 lines were added (to the RHS file)?
>
> I personally find that output would be useless and would prefer
> it to say "I renamed file A to file B.  Content-wise, there were
> N lines added and M lines removed, compared to the straight
> rename case, by the way".
>
> And that is what the current output does.  I do not see why it
> should be different in the case of a copy instead of a rename.
>

Because after a copy you have 2 files, not still one. And the 'after
copied' file cannot be the original, but a new file, because the
original is still alive.

Perhaps I can explain better myself taking in account the _couple_ of
files involved in both cases:

Without -C we have

diff-lib.c                    | 1862 ++---------------------------------------
diff.c                        | 1795 ++++++++++++++++++++++++++++++++++++++++

With -C option is

 diff-lib.c                    | 1862 ++---------------------------------------
 diff-lib.c => diff.c          |    0

If it was a rename we had something like:

diff-lib.c                  | 1795 -----------------------------------------
diff.c                        | 1795 ++++++++++++++++++++++++++++++++++++++++

and, with -C

diff-lib.c => diff.c          |    0

and _this_ is correct. But with copy  diff-lib.c => diff.c  should not
stay at zero lines changed because diff.c is not the same of
diff-lib.c, but it's a _new_ file created with the same content of
diff-lib.c and _then_ the original and only diff-lib.c file is further
modified on his own (in our case changing 1862 lines).

Please tell me where I get wrong.

Thanks
Marco

Re: [Possible bug] diff-tree --stat info does not count copies

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> On 8/17/06, Junio C Hamano <junkio@cox.net> wrote:
>
>> Interesting.  That's really a matter of taste and interpretation.
>>...
>
> If it was a rename we had something like:
>
> diff-lib.c                  | 1795 -----------------------------------------
> diff.c                        | 1795 ++++++++++++++++++++++++++++++++++++++++
>
> and, with -C
>
> diff-lib.c => diff.c          |    0
>
> and _this_ is correct. But with copy  diff-lib.c => diff.c  should not
> stay at zero lines changed because diff.c is not the same of
> diff-lib.c, but it's a _new_ file created with the same content of
> diff-lib.c and _then_ the original and only diff-lib.c file is further
> modified on his own (in our case changing 1862 lines).
>
> Please tell me where I get wrong.

That's why I said this is a matter of taste and interpretation.
Our differences lie in what we expect from these numbers.  I do
not think your interpretation is wrong.  It is just different
from mine, and I happen to think my interpretation is more
useful for my purposes.

The numbers currently shown give "how big a patch do I have to
go through if I were to review this change in a patch form,
assuming I am reasonably familiar with the current, pre-image,
code?"  So straight copy does not count -- if the patch tells me
that new file diff.c has the same contents as old diff-lib.c,
then I know what the resulting diff.c would contain without
looking at the patch text to judge what its implications are
(including things like 'if file A.c is moved to sub/B.c, I know
A.c includes "C.h" and it needs to be adjusted to include
"../C.h").  In "copy plus edit" case, the change shown in the
diff text plus the fact that the file was copied from something
I know makes the patch part the only thing that needs to be
reviewed.  The same logic applies to rename with or without
editing.

What you expect seems to be different.  If I understand you
correctly, you are asking for the number of lines that need to
be touched (inserted and removed) in order to make the original
into the patched, when the source tree is taken as a whole.  I
do not think it is wrong to want to know that number, which is
pretty much the same number the command gives without -M/-C
[*1*].

To make this topic even more interesting, we can compare these
three commands:

    $ paths='diff-lib.c diff.c'
    $ git diff-tree -r --stat --summary 6973dca -- $paths
    $ git diff-tree -r --stat --summary -C 6973dca -- $paths
    $ git diff-tree -r --stat --summary -B -C 6973dca -- $paths

The last one says:

6973dcaee76ef7b7bfcabd2f26e76205aae07858
 diff-lib.c           | 1928 +++-----------------------------------------------
 diff-lib.c => diff.c |    0 
 2 files changed, 133 insertions(+), 1795 deletions(-)
 rewrite diff-lib.c (99%)
 rename diff-lib.c => diff.c (100%)

which, personally, I think tells the story closest to what
really happened for this change.  What was known to be
diff-lib.c was renamed to diff.c without any change, while
diff-lib.c was replaced by completely new contents, losing all
1795 lines that were there and acquiring 133 brand new lines.

In this case, I do not _need_ to look at the 1795 lines that
were lost from diff-lib.c to review the new implementation of
diff-lib.c, assuming that I am familiar with the pre-image code
(so what I said earlier about the number of lines I have to
review is not quite true, but is close enough).

[Footnote]

*1* But if that is the number you are after, then you should
count rename and copy the same way by saying rename removes N
lines and adds N lines (that happen to be the same) elsewhere,
so you would not say 0 is the right answer.