[Bug report] git diff stat shows unrelated diff

[Bug report] git diff stat shows unrelated diff

[Viresh Kumar]

Hello,

I am not sure if it is a bug or not, but the output I got wasn't what
I was looking for. And so looking for some help. I was looking to send
pull request [1] to PM maintainer and was generating the request
against his tree [2], which already has kernel upto v5.0-rc6 merged in
it.

These are my local branches:

Branch A:

55538fbc79e9 cpufreq: qcom: Read voltage LUT and populate OPP
f896d06665ec cpufreq: qcom-hw: Move to device_initcall
1c7fc5cbc339 (tag: v5.0-rc2) Linux 5.0-rc2

Branch B:

a4f342b9607d PM / OPP: Introduce a power estimation helper
285881b51eb5 PM / OPP: Remove unused parameter of _generic_set_opp_clk_only()
bfeffd155283 (tag: v5.0-rc1) Linux 5.0-rc1

pm/linux-next branch already has Branch B merged in it (with an
earlier pull request).

Branch C (7c139d3f0f99) is a merge of Branch A and Branch B. When I
try to generate diff-stat with:

git diff -M --stat pm/linux-next..7c139d3f0f99

It shows me the diffstat between v5.0-rc1..v5.0-rc2 as well.

If I do

git diff -M --stat v5.0-rc2..7c139d3f0f99

it shows only the files that have changed in patches in branch A and
B.

Since pm/linux-next already has Branch B and all the rcs upto rc6, I
was expecting the output of first diffstat to be similar to second one
(without branch B stuff). Is the expectation incorrect ?

-- 
viresh

[1] My tree: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git
[2] PM tree: git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git

Top commit in PM tree at the time I was trying it:

d26ff2405272 (pm/testing, pm/linux-next) Merge branch 'pm-opp' into linux-next

Re: [Bug report] git diff stat shows unrelated diff

[Johannes Sixt]

Am 14.02.19 um 09:22 schrieb Viresh Kumar:
> Hello,
> 
> I am not sure if it is a bug or not, but the output I got wasn't what
> I was looking for. And so looking for some help. I was looking to send
> pull request [1] to PM maintainer and was generating the request
> against his tree [2], which already has kernel upto v5.0-rc6 merged in
> it.
> 
> These are my local branches:
> 
> Branch A:
> 
> 55538fbc79e9 cpufreq: qcom: Read voltage LUT and populate OPP
> f896d06665ec cpufreq: qcom-hw: Move to device_initcall
> 1c7fc5cbc339 (tag: v5.0-rc2) Linux 5.0-rc2
> 
> Branch B:
> 
> a4f342b9607d PM / OPP: Introduce a power estimation helper
> 285881b51eb5 PM / OPP: Remove unused parameter of _generic_set_opp_clk_only()
> bfeffd155283 (tag: v5.0-rc1) Linux 5.0-rc1
> 
> pm/linux-next branch already has Branch B merged in it (with an
> earlier pull request).
> 
> Branch C (7c139d3f0f99) is a merge of Branch A and Branch B. When I
> try to generate diff-stat with:
> 
> git diff -M --stat pm/linux-next..7c139d3f0f99
> 
> It shows me the diffstat between v5.0-rc1..v5.0-rc2 as well.
> 
> If I do
> 
> git diff -M --stat v5.0-rc2..7c139d3f0f99
> 
> it shows only the files that have changed in patches in branch A and
> B.
> 
> Since pm/linux-next already has Branch B and all the rcs upto rc6, I
> was expecting the output of first diffstat to be similar to second one
> (without branch B stuff). Is the expectation incorrect ?

Yes, I think your expectation is incorrect.

The meaning of .. is different between diff and the log commands. For
diff it is mostly just noise, that is your first diff command is
actually equivalent to

  git diff -M --stat pm/linux-next 7c139d3f0f99

It is quite obvious IMO that this is the diff between two end points.
What you most likely wanted to see is

  git diff -M --stat pm/linux-next...7c139d3f0f99

It shows the diff between a merge base of the two revisions and the
second one, i.e. between a where Branch C branched off of pm/linux-next
and its tip.

-- Hannes

Re: [Bug report] git diff stat shows unrelated diff

[Elijah Newren]

Hi,

On Thu, Feb 14, 2019 at 11:35 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Hello,
>
> I am not sure if it is a bug or not, but the output I got wasn't what
> I was looking for. And so looking for some help. I was looking to send
> pull request [1] to PM maintainer and was generating the request
> against his tree [2], which already has kernel upto v5.0-rc6 merged in
> it.
>
> These are my local branches:
>
> Branch A:
>
> 55538fbc79e9 cpufreq: qcom: Read voltage LUT and populate OPP
> f896d06665ec cpufreq: qcom-hw: Move to device_initcall
> 1c7fc5cbc339 (tag: v5.0-rc2) Linux 5.0-rc2
>
> Branch B:
>
> a4f342b9607d PM / OPP: Introduce a power estimation helper
> 285881b51eb5 PM / OPP: Remove unused parameter of _generic_set_opp_clk_only()
> bfeffd155283 (tag: v5.0-rc1) Linux 5.0-rc1
>
> pm/linux-next branch already has Branch B merged in it (with an
> earlier pull request).
>
> Branch C (7c139d3f0f99) is a merge of Branch A and Branch B. When I
> try to generate diff-stat with:
>
> git diff -M --stat pm/linux-next..7c139d3f0f99
>
> It shows me the diffstat between v5.0-rc1..v5.0-rc2 as well.
>
> If I do
>
> git diff -M --stat v5.0-rc2..7c139d3f0f99
>
> it shows only the files that have changed in patches in branch A and
> B.
>
> Since pm/linux-next already has Branch B and all the rcs upto rc6, I
> was expecting the output of first diffstat to be similar to second one
> (without branch B stuff). Is the expectation incorrect ?

I think you're getting tripped up by double-dot vs triple-dot with
diff being different than log:

`git diff D..E` means the same thing as `git diff D E`, i.e. diff the
two commits D and E.

`git diff D...E` means the same thing as `git diff $(git merge-base D E) E`

There are some people for whom this state of affairs makes sense.  I
am not one of them, and I suspect you aren't either.  The arguments
made by those who feel this makes sense seem reasonable to me in the
moment when they present them, but I have never been able to remember
these arguments longer than briefly.  It just doesn't stick with me.
The only thing I seem to be able to retain is the following:  "git
diff D..E is totally useless and should be an error because (1) it
doesn't do what I expect and (2) for folks that want the behavior
currently gotten with that syntax can instead just use a space instead
of a double dot."


Hope that helps,
Elijah

Re: [Bug report] git diff stat shows unrelated diff

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> The only thing I seem to be able to retain is the following:  "git
> diff D..E is totally useless and should be an error because (1) it
> doesn't do what I expect and (2) for folks that want the behavior
> currently gotten with that syntax can instead just use a space instead
> of a double dot."

That sums up pretty nicely.  diff is fundamentally an operation
between two endpoints, so the range notation a..b does not work
nicely with it at the conceptual level.

When we realized that we can take advantage of the above fact, and
reuse a range notation to mean something that is generally useful in
the context of diff, such as 'one end of the comparison is the merge
base between a and b, and the other end is b', it was too late to
use "a..b", as an early adopters of Git was already used to the fact
that "a..b" happened to mean the same thing as "comparison of one
end is a, the other end is b", pretty much implemented without much
thought.

It might be _possible_ to spend a year (i.e. 4 cycles) to start
warning when two-dot notation is used for "git diff" (only, not any
plumbing like "git diff-files") and tell the user to use the more
logical two-end notation "git diff A B" and then eventually error
out when two-dot notation is used, while retaining the three-dot
notation throughout and to the eternity.  I am not sure if it is
worth the deprecation cost, though.

Re: [Bug report] git diff stat shows unrelated diff

[Denton Liu]

On Thu, Feb 14, 2019 at 02:10:53PM -0800, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
> 
> > The only thing I seem to be able to retain is the following:  "git
> > diff D..E is totally useless and should be an error because (1) it
> > doesn't do what I expect and (2) for folks that want the behavior
> > currently gotten with that syntax can instead just use a space instead
> > of a double dot."
> 
> That sums up pretty nicely.  diff is fundamentally an operation
> between two endpoints, so the range notation a..b does not work
> nicely with it at the conceptual level.
> 
> When we realized that we can take advantage of the above fact, and
> reuse a range notation to mean something that is generally useful in
> the context of diff, such as 'one end of the comparison is the merge
> base between a and b, and the other end is b', it was too late to
> use "a..b", as an early adopters of Git was already used to the fact
> that "a..b" happened to mean the same thing as "comparison of one
> end is a, the other end is b", pretty much implemented without much
> thought.
> 
> It might be _possible_ to spend a year (i.e. 4 cycles) to start
> warning when two-dot notation is used for "git diff" (only, not any
> plumbing like "git diff-files") and tell the user to use the more
> logical two-end notation "git diff A B" and then eventually error
> out when two-dot notation is used, while retaining the three-dot
> notation throughout and to the eternity.  I am not sure if it is
> worth the deprecation cost, though.
> 
Instead of outright deprecating it, would it make sense to include a
configuration option, say "diff.sensibleDots", that would enable a user
to set the dots to the other form if they desire?

This has bugged me for long enough that if there's a desire for this
from others, I'm willing take on the effort of making this change.

Thanks,

Denton

Re: [Bug report] git diff stat shows unrelated diff

[Elijah Newren]

On Fri, Feb 15, 2019 at 10:52 AM Denton Liu <liu.denton@gmail.com> wrote:
>
> On Thu, Feb 14, 2019 at 02:10:53PM -0800, Junio C Hamano wrote:
> > Elijah Newren <newren@gmail.com> writes:
> >
> > > The only thing I seem to be able to retain is the following:  "git
> > > diff D..E is totally useless and should be an error because (1) it
> > > doesn't do what I expect and (2) for folks that want the behavior
> > > currently gotten with that syntax can instead just use a space instead
> > > of a double dot."
> >
> > That sums up pretty nicely.  diff is fundamentally an operation
> > between two endpoints, so the range notation a..b does not work
> > nicely with it at the conceptual level.
> >
> > When we realized that we can take advantage of the above fact, and
> > reuse a range notation to mean something that is generally useful in
> > the context of diff, such as 'one end of the comparison is the merge
> > base between a and b, and the other end is b', it was too late to
> > use "a..b", as an early adopters of Git was already used to the fact
> > that "a..b" happened to mean the same thing as "comparison of one
> > end is a, the other end is b", pretty much implemented without much
> > thought.
> >
> > It might be _possible_ to spend a year (i.e. 4 cycles) to start
> > warning when two-dot notation is used for "git diff" (only, not any
> > plumbing like "git diff-files") and tell the user to use the more
> > logical two-end notation "git diff A B" and then eventually error
> > out when two-dot notation is used, while retaining the three-dot
> > notation throughout and to the eternity.  I am not sure if it is
> > worth the deprecation cost, though.
> >
> Instead of outright deprecating it, would it make sense to include a
> configuration option, say "diff.sensibleDots", that would enable a user
> to set the dots to the other form if they desire?

I think Junio's suggested steps would still have to come first, and
there may also need to be a time period after two-dot notation is an
error before we were to try to repurpose it for something else (e.g.
to make it behave the same as triple-dot).  Adding a config to change
things now without both a deprecation and error period would invite
horrible surprises and bug reports; people need time and warning to
change workflows and fix existing scripts that might be out there.

> This has bugged me for long enough that if there's a desire for this
> from others, I'm willing take on the effort of making this change.

I was going to put the deprecation and erroring steps on my (always
growing) TODO list to eventually get to it, but if you're motivated,
go for it; I wouldn't get to it soon anyway.

Re: [Bug report] git diff stat shows unrelated diff

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> Instead of outright deprecating it, would it make sense to include a
>> configuration option, say "diff.sensibleDots", that would enable a user
>> to set the dots to the other form if they desire?
>
> I think Junio's suggested steps would still have to come first, and
> there may also need to be a time period after two-dot notation is an
> error before we were to try to repurpose it for something else (e.g.
> to make it behave the same as triple-dot).  Adding a config to change
> things now without both a deprecation and error period would invite
> horrible surprises and bug reports; people need time and warning to
> change workflows and fix existing scripts that might be out there.

Historically, it was a mistake to allow A..B to be used for two
endpoints, which was made back when we haven't thought things
through.  That is why I stopped "warn to deprecate and then
completely remove", as I do not think it would help people very much
if "git diff A B" can be spelled with two-dots.

But in a distant future long after that happens, by the time nobody
remembers what A..B meant for "git diff", I do not think I'd
strongly be opposed to reusing it to mean something different.

Re: [Bug report] git diff stat shows unrelated diff

[Philip Oakley]

On 15/02/2019 20:12, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>>> Instead of outright deprecating it, would it make sense to include a
>>> configuration option, say "diff.sensibleDots", that would enable a user
>>> to set the dots to the other form if they desire?
>> I think Junio's suggested steps would still have to come first, and
>> there may also need to be a time period after two-dot notation is an
>> error before we were to try to repurpose it for something else (e.g.
>> to make it behave the same as triple-dot).  Adding a config to change
>> things now without both a deprecation and error period would invite
>> horrible surprises and bug reports; people need time and warning to
>> change workflows and fix existing scripts that might be out there.
> Historically, it was a mistake to allow A..B to be used for two
> endpoints, which was made back when we haven't thought things
> through.  That is why I stopped "warn to deprecate and then
> completely remove", as I do not think it would help people very much
> if "git diff A B" can be spelled with two-dots.
>
> But in a distant future long after that happens, by the time nobody
> remembers what A..B meant for "git diff", I do not think I'd
> strongly be opposed to reusing it to mean something different.

Would an option be to add a opt-in config to do the warning, rather than 
start immediately at a deprecation warning?

It would give users the chance to test out their usage early should the 
so wish/desire/notice.

I'd agree that the deprecation period would need to be quite a few 
cycles before the default (unset) would be flipped to warn if not set, 
yet still allow the warnings to be switched off for this case.

(obviously after the current release..)

Re: [Bug report] git diff stat shows unrelated diff

[Junio C Hamano]

Philip Oakley <philipoakley@iee.org> writes:

> On 15/02/2019 20:12, Junio C Hamano wrote:
>>
>> Historically, it was a mistake to allow A..B to be used for two
>> endpoints, which was made back when we haven't thought things
>> through.  That is why I stopped "warn to deprecate and then
>> completely remove", as I do not think it would help people very much
>> if "git diff A B" can be spelled with two-dots.
>>
>> But in a distant future long after that happens, by the time nobody
>> remembers what A..B meant for "git diff", I do not think I'd
>> strongly be opposed to reusing it to mean something different.
>
> Would an option be to add a opt-in config to do the warning, rather
> than start immediately at a deprecation warning?

Well, anything would be "an option".  I am not sure it would be
particularly a good option to allow people to "opt" into getting
warned, only to get a chance to train their fingers not to type
double-dot instead of a SP, earlier than other people, though.

> It would give users the chance to test out their usage early should
> the so wish/desire/notice.

I am somewhat puzzled.  What are you trying to achieve by that?

Those who do *not* opt into that "early warning" configuration dance
would eventually be warned whenever they type "diff A..B", and the
timing for that eventuality is not under their control, so quite
honestly, I do not see much point in "giving users the chance".

Re: [Bug report] git diff stat shows unrelated diff

[Philip Oakley]

On 15/02/2019 23:32, Junio C Hamano wrote:
> Philip Oakley <philipoakley@iee.org> writes:
>
>> On 15/02/2019 20:12, Junio C Hamano wrote:
>>> Historically, it was a mistake to allow A..B to be used for two
>>> endpoints, which was made back when we haven't thought things
>>> through.  That is why I stopped "warn to deprecate and then
>>> completely remove", as I do not think it would help people very much
>>> if "git diff A B" can be spelled with two-dots.
>>>
>>> But in a distant future long after that happens, by the time nobody
>>> remembers what A..B meant for "git diff", I do not think I'd
>>> strongly be opposed to reusing it to mean something different.
>> Would an option be to add a opt-in config to do the warning, rather
>> than start immediately at a deprecation warning?
> Well, anything would be "an option".  I am not sure it would be
> particularly a good option to allow people to "opt" into getting
> warned, only to get a chance to train their fingers not to type
> double-dot instead of a SP, earlier than other people, though.
The point of the suggestion was very much to provide users with the 
choice of finger training at their own time and pace, rather than being 
forced by the deprecation sequence to be surprised at an inconvenient 
time into having to respond.
>
>> It would give users the chance to test out their usage early should
>> the so wish/desire/notice.
> I am somewhat puzzled.  What are you trying to achieve by that?
>
> Those who do *not* opt into that "early warning" configuration dance
> would eventually be warned whenever they type "diff A..B", and the
> timing for that eventuality is not under their control, so quite
> honestly, I do not see much point in "giving users the chance".

With the opposite hat on, not giving users the choice does seem unfair 
to those that are trying to keep up. If we are warning (in the release 
notes) of an upcoming deprecation (in the code) then it does seem 
helpful that users could buy into the deprecation early, and at their 
convenience, to assist in the unlearning of an old habit, which can be 
much harder than learning a new habit, hence my comment.

You are right that those who neither notice nor care will be surprised 
later, but we shouldn't let that limit others.

--

Philip

Re: [Bug report] git diff stat shows unrelated diff

[Junio C Hamano]

Philip Oakley <philipoakley@iee.org> writes:

>> Those who do *not* opt into that "early warning" configuration dance
>> would eventually be warned whenever they type "diff A..B", and the
>> timing for that eventuality is not under their control, so quite
>> honestly, I do not see much point in "giving users the chance".
>
> With the opposite hat on, not giving users the choice does seem unfair
> to those that are trying to keep up. If we are warning (in the release
> notes) of an upcoming deprecation (in the code) then it does seem
> helpful that users could buy into the deprecation early, and at their
> convenience, to assist in the unlearning of an old habit, which can be
> much harder than learning a new habit, hence my comment.
>
> You are right that those who neither notice nor care will be surprised
> later, but we shouldn't let that limit others.

I still do not quite get where you are coming from.  Are you saying
that those who do not opt into the early warning may get 2 cycles
(just picked out of thin-air) of deprecation period, and with an
optional early warning feature, those who feel that 2 cycles is not
long enough to train their fingers would spend 3 cycles and they
will be helped than without?

That line of thinking sounds somewhat ridiculous---where does it
end?  If those who opt into would find it sufficient to have 3
cycles to train, there may still be people who feel 3 is not enough
and want to have 4.  As we make it longer, we'd cover more people
and at some point we'd reach the point of diminishing returns.

Wouldn't it be even better, and far simper, to just extend the
deprecation period to that many cycles to make it long enough for
majority of users' needs, without any early warning option?  

The thing is, once you train your fingers, it does not matter to you
if the deprecation warning is still there, as you'd not be typing
"diff A..B" at that point.  So I am not sure who you are trying to
help by the early warning option.

Thanks.

Re: [Bug report] git diff stat shows unrelated diff

[Philip Oakley]

Hi Junio,

On 17/02/2019 03:34, Junio C Hamano wrote:
> Philip Oakley <philipoakley@iee.org> writes:
>
>>> Those who do *not* opt into that "early warning" configuration dance
>>> would eventually be warned whenever they type "diff A..B", and the
>>> timing for that eventuality is not under their control, so quite
>>> honestly, I do not see much point in "giving users the chance".
>> With the opposite hat on, not giving users the choice does seem unfair
>> to those that are trying to keep up. If we are warning (in the release
>> notes) of an upcoming deprecation (in the code) then it does seem
>> helpful that users could buy into the deprecation early, and at their
>> convenience, to assist in the unlearning of an old habit, which can be
>> much harder than learning a new habit, hence my comment.
>>
>> You are right that those who neither notice nor care will be surprised
>> later, but we shouldn't let that limit others.
> I still do not quite get where you are coming from.  Are you saying
> that those who do not opt into the early warning may get 2 cycles
> (just picked out of thin-air) of deprecation period, and with an
> optional early warning feature, those who feel that 2 cycles is not
> long enough to train their fingers would spend 3 cycles and they
> will be helped than without?

It was my understanding that the end point would be total removal of any 
options and the typing of the double dot would be an error. Given that 
hard end point I was looking to ensure that users of double dots have a 
manageable route to unlearning old bad habits. Thus the first phase 
would be opt-in, the second phase opt-out, and on the third final phase 
it would be a non-optional error (assuming your first comment in [1]).

>
> That line of thinking sounds somewhat ridiculous---where does it
> end?  If those who opt into would find it sufficient to have 3
> cycles to train, there may still be people who feel 3 is not enough
> and want to have 4.  As we make it longer, we'd cover more people
> and at some point we'd reach the point of diminishing returns.
The length of the phase 1 is your choice, but having a zero length (as 
some discussions implied) felt too short. For me, one cycle of users 
'opting-in' to do their testing and training given a deprecation 
notification would be sufficient.
>
> Wouldn't it be even better, and far simper, to just extend the
> deprecation period to that many cycles to make it long enough for
> majority of users' needs, without any early warning option?
>
> The thing is, once you train your fingers,
To train the fingers, and to check local scripts and aliases, the user 
needs feedback, preferably at a time of their convenience (as opposed to 
being a time of inconvenience), so assuming they have been paying 
moderate attention to the release notes, providing the opt-in phase 
gives them that.
>   it does not matter to you
> if the deprecation warning is still there, as you'd not be typing
> "diff A..B" at that point.  So I am not sure who you are trying to
> help by the early warning option.
>
> Thanks.
I do note that you had indicated at the end of [1]: "I am not sure if it 
is worth the deprecation cost, though.", so this may be a bit of a mute 
point anyway.

Philip

[1] <xmqqmumy6mxe.fsf@gitster-ct.c.googlers.com> "then eventually error 
out when two-dot notation is used"

Re: [Bug report] git diff stat shows unrelated diff

[Junio C Hamano]

Philip Oakley <philipoakley@iee.org> writes:

> It was my understanding that the end point would be total removal of
> any options and the typing of the double dot would be an error. Given
> that hard end point I was looking to ensure that users of double dots
> have a manageable route to unlearning old bad habits. Thus the first
> phase would be opt-in,

Sorry, but I do not see any logical connection in this "Thus".  If
we are still undecided if deprecating double-dot is a good idea and
trying to gauge the impact, then perhaps an early "opt-in" to leave
the door open for aborting the transition plan might make sense (as
an escape hatch for _us_ the project developers to make excuse to
the end users).  But I am getting an impression that it is not the
plan you have in mind.

> To train the fingers, and to check local scripts and aliases, the user
> needs feedback, preferably at a time of their convenience (as opposed
> to being a time of inconvenience), so assuming they have been paying
> moderate attention to the release notes, providing the opt-in phase
> gives them that.

And to those who haven't been paying attention, what happens when
your "first phase" period expires?

I would be a lot more sympathetic if your argument were "some people
will not be ready to start training, and they will be helped if we
had an opt-out knob early in the long deprecation period".  Even
those who have not been paying attention at all _will_ be hit by
deprecation warning, and that is when they can decide if they want
to start training or they are not ready and want to postpone, so in
that sense, "initial opt-out" may make sense, but I do not see how
"initial opt-in" can be a viable thing.

Re: [Bug report] git diff stat shows unrelated diff

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> On Thu, Feb 14, 2019 at 02:10:53PM -0800, Junio C Hamano wrote:
>
>> It might be _possible_ to spend a year (i.e. 4 cycles) to start
>> warning when two-dot notation is used for "git diff" (only, not any
>> plumbing like "git diff-files") and tell the user to use the more
>> logical two-end notation "git diff A B" and then eventually error
>> out when two-dot notation is used, while retaining the three-dot
>> notation throughout and to the eternity.  I am not sure if it is
>> worth the deprecation cost, though.
>> 
> Instead of outright deprecating it, would it make sense to include a
> configuration option, say "diff.sensibleDots", that would enable a user
> to set the dots to the other form if they desire?

I doubt that such a change helps anybody more than it hurts.
Defining A..B to mean something totally different based on a hidden
config will hurt those who want to help others.  It would even hurt
those who are new to Git and blindly set such configuration because
their local guru tells them to and then goes on to read code
snippets and docs on the web outside our control that have been
written with the established meaning of what A..B and A...B mean to
"git diff".

There is nothing sensible in using dots, i.e. range notation,
between A and B _if_ you are trying to compare A and B, i.e. two
endpoints.  Just use "diff A B" and you are OK.

Re: [Bug report] git diff stat shows unrelated diff

[Viresh Kumar]

On 14-02-19, 13:23, Elijah Newren wrote:
> I think you're getting tripped up by double-dot vs triple-dot with
> diff being different than log:
> 
> `git diff D..E` means the same thing as `git diff D E`, i.e. diff the
> two commits D and E.

Right, so both the branches have at least until rc2 (though
pm/linux-next had until rc6), why will the diff D..E show the diff
between rc1 and rc2 ?

> `git diff D...E` means the same thing as `git diff $(git merge-base D E) E`

I get exactly the same result with both .. and ... in this particular
case and that's why I wonder if everything is okay or not.

The problem in this case is:
- PM tree had rc1,2,3,4,5,6 merged earlier into it.
- Then I got merged one of my branches which was based of rc1 into
  pm/linux-next.
- And now I am trying to send pull request for another branch which is
  a merge of the earlier branch (which got merged, based of rc1) and
  second branch that has more stuff over rc2.
- The most recent merge commit common between my branch and
  pm/linux-next becomes the earlier branch which was based of rc1.
- Now I expect ... to return the diff between rc1 and rc2 as it will
  diff against the most recent merge.
- But I expected ... to not include rc1..rc2 diff.

> There are some people for whom this state of affairs makes sense.  I
> am not one of them, and I suspect you aren't either.  The arguments
> made by those who feel this makes sense seem reasonable to me in the
> moment when they present them, but I have never been able to remember
> these arguments longer than briefly.  It just doesn't stick with me.
> The only thing I seem to be able to retain is the following:  "git
> diff D..E is totally useless and should be an error because (1) it
> doesn't do what I expect and (2) for folks that want the behavior
> currently gotten with that syntax can instead just use a space instead
> of a double dot."

Okay but git request-pull uses .. and not ... and that's where I saw
the issue in the first place.

-- 
viresh

Re: [Bug report] git diff stat shows unrelated diff

[Elijah Newren]

Hi Viresh,

On Thu, Feb 14, 2019 at 10:40 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 14-02-19, 13:23, Elijah Newren wrote:
> > I think you're getting tripped up by double-dot vs triple-dot with
> > diff being different than log:
> >
> > `git diff D..E` means the same thing as `git diff D E`, i.e. diff the
> > two commits D and E.
>
> Right, so both the branches have at least until rc2 (though
> pm/linux-next had until rc6), why will the diff D..E show the diff
> between rc1 and rc2 ?

I don't think it does; it includes the diff between rc2 and rc6.
First, `git diff D..E` will show you roughly the same thing as if you
have two clones of linux, one with D checked out, one with E checked
out, and then you run 'diff -ru linux-copy1/ linux-copy2/'.

But let's go back to your example, you had

Branch A: commit 55538fbc79e9
Branch B: commit a4f342b9607d
Branch C: commit 7c139d3f0f99
Branch pm/linux-next: unspecified, but I'll guess commit a06639e89e4

Now, using git describe on each of those commits in order shows us
where in history they are:

A: v5.0-rc2-2-g55538fbc79e9
B: v5.0-rc1-2-ga4f342b9607d
C: v5.0-rc2-5-g7c139d3f0f99
D: v5.0-rc6-84-ga06639e89e47

Now, you said you ran
   git diff -M --stat pm/linux-next..7c139d3f0f99
and that it included the diff between rc1 and rc2.  I think you
actually saw pieces of the diff from rc2 to rc6 and assumed it was the
diff from rc1 to rc2.  Besides the fact that pm/linux-next is based on
rc6 and 7c139d3f0f99 is based on rc2, here's another way to verify
that:

$ git diff --name-only v5.0-rc1 v5.0-rc2 | sort > early
$ git diff --name-only v5.0-rc2 v5.0-rc6 | sort > late
$ comm -23 early late > early-only
$ comm -13 early late > late-only
$ wc -l *-only
  407 early-only
 1149 late-only
 1556 total

So, here "early-only" is a list of files that changed between rc1 and
rc2 that did not change between rc2 and rc6.  "late-only" is a list of
files that did not change between rc1 and rc2, but did change between
rc2 and rc6.  No, let's compare that to your diff:

$ git diff --name-only a06639e89e4..7c139d3f0f99 | sort > changes
$ comm -12 changes early-only | wc -l
6
$ comm -12 changes late-only | wc -l
1148

So, the files listed in your diff only included 6 files that were
unique to early-only, and included 1148 files that were unique to
late-only.  So, your diff looks an awful lot like the diff between rc2
and rc6, and not much at all like the diff between rc1 and rc2.

> > `git diff D...E` means the same thing as `git diff $(git merge-base D E) E`
>
> I get exactly the same result with both .. and ... in this particular
> case and that's why I wonder if everything is okay or not.

With `git diff D..E` it doesn't matter much which order you put D and
E in, other than flipping '-' lines for '+' lines.  With `git diff
D...E` it makes a huge difference.  Compare:

$ git diff --shortstat 7c139d3f0f99..a06639e89e4
 1466 files changed, 15417 insertions(+), 7313 deletions(-)
$ git diff --shortstat a06639e89e4..7c139d3f0f99
 1466 files changed, 7313 insertions(+), 15417 deletions(-)

So, as I said, swapping with double-dot only changes '-' and '+'
lines.  In contrast:

$ git diff --shortstat 7c139d3f0f99...a06639e89e4
 1463 files changed, 15401 insertions(+), 7165 deletions(-)
$ git diff --shortstat a06639e89e4...7c139d3f0f99
 4 files changed, 148 insertions(+), 16 deletions(-)

You get a really small diff in one direction, but huge in the other.
The reason for this is that commit C (7c139d3f0f99) is really close to
the merge base, but pm/linux-next (a06639e89e4) is really far from it
-- it includes rc6.

> The problem in this case is:
> - PM tree had rc1,2,3,4,5,6 merged earlier into it.
> - Then I got merged one of my branches which was based of rc1 into
>   pm/linux-next.
> - And now I am trying to send pull request for another branch which is
>   a merge of the earlier branch (which got merged, based of rc1) and
>   second branch that has more stuff over rc2.
> - The most recent merge commit common between my branch and
>   pm/linux-next becomes the earlier branch which was based of rc1.
> - Now I expect ... to return the diff between rc1 and rc2 as it will
>   diff against the most recent merge.
> - But I expected ... to not include rc1..rc2 diff.
>
> > There are some people for whom this state of affairs makes sense.  I
> > am not one of them, and I suspect you aren't either.  The arguments
> > made by those who feel this makes sense seem reasonable to me in the
> > moment when they present them, but I have never been able to remember
> > these arguments longer than briefly.  It just doesn't stick with me.
> > The only thing I seem to be able to retain is the following:  "git
> > diff D..E is totally useless and should be an error because (1) it
> > doesn't do what I expect and (2) for folks that want the behavior
> > currently gotten with that syntax can instead just use a space instead
> > of a double dot."
>
> Okay but git request-pull uses .. and not ... and that's where I saw
> the issue in the first place.

I haven't used request-pull myself and I'm not familiar with the code,
so I don't know if you might have just passed it the wrong arguments
or if it has a bug.  I'll have to leave it to someone else to comment
on that (though they may need to know what arguments you passed to
that command).


Hope that helps,
Elijah

Re: [Bug report] git diff stat shows unrelated diff

[Viresh Kumar]

On 15-02-19, 08:09, Elijah Newren wrote:
> Hi Viresh,
> 
> On Thu, Feb 14, 2019 at 10:40 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 14-02-19, 13:23, Elijah Newren wrote:
> > > I think you're getting tripped up by double-dot vs triple-dot with
> > > diff being different than log:
> > >
> > > `git diff D..E` means the same thing as `git diff D E`, i.e. diff the
> > > two commits D and E.
> >
> > Right, so both the branches have at least until rc2 (though
> > pm/linux-next had until rc6), why will the diff D..E show the diff
> > between rc1 and rc2 ?
> 
> I don't think it does; it includes the diff between rc2 and rc6.
> First, `git diff D..E` will show you roughly the same thing as if you
> have two clones of linux, one with D checked out, one with E checked
> out, and then you run 'diff -ru linux-copy1/ linux-copy2/'.
> 
> But let's go back to your example, you had
> 
> Branch A: commit 55538fbc79e9
> Branch B: commit a4f342b9607d
> Branch C: commit 7c139d3f0f99
> Branch pm/linux-next: unspecified, but I'll guess commit a06639e89e4

I mentioned that at the bottom of the email, d26ff2405272 :)

And yes I got confused somehow earlier, ran all the commands again to
verify the diffs, sorry about that.

-- 
viresh