Can't find the revelant commit with git-log

Can't find the revelant commit with git-log

[Francis Moreau]

Hello,

I'm trying to find out a commit which removed a function inside a file.

The project is the Linux kernel, and I'm trying to look for changes
which happened between v2.6.27 and v2.6.28. The changes happened in the
following file: drivers/pci/intel-iommu.c where a function has been
removed:

   $ git --version
   git version 1.7.4.rc3

   $ cd ~/linux-2.6/drivers/pci/
   $ git grep blacklist v2.6.27 -- drivers/pci/intel-iommu.c
   $

No ouput... hmm, I know it's in... oh maybe the path is incorrect

   $ git git grep blacklist v2.6.27 -- intel-iommu.c
   v2.6.27:intel-iommu.c:static int blacklist_iommu(const struct dmi_system_id *id)
   v2.6.27:intel-iommu.c:          .callback = blacklist_iommu,

ah, so Git failed previously without any comments on the wrong
path... maybe it should ?

So at that point I know that in the revision v2.6.27, there's a function
called "blacklist_iommu" in drivers/pci/intel-iommu.c

Now take a look if it's still there in v2.6.28:

   $ git git grep blacklist v2.6.28 -- intel-iommu.c
   $

This time nothing is printed but I know that the command is correct.

So now I'm interested in looking for the commit which removed this
function. Fo this I'm trying to use git-log(1):

   $ git log --full-history --follow -S'static int blacklist_iommu(const struct dmi_system_id *id)' v2.6.27..v2.6.28 -- intel-iommu.c
   $ echo $?
   0

I tried different options but it fails.

Also I found that passing the exact string to '-S' is annoying, are
there any way to just pass a part of the string such as
"-Sblacklist_iommu" ?

Sorry if I miss the revelant git-log's option, but there're so many...
-- 
Francis

Re: Can't find the revelant commit with git-log

[René Scharfe]

Am 25.01.2011 10:01, schrieb Francis Moreau:
> Hello,
> 
> I'm trying to find out a commit which removed a function inside a file.
> 
> The project is the Linux kernel, and I'm trying to look for changes
> which happened between v2.6.27 and v2.6.28. The changes happened in the
> following file: drivers/pci/intel-iommu.c where a function has been
> removed:
> 
>    $ git --version
>    git version 1.7.4.rc3
> 
>    $ cd ~/linux-2.6/drivers/pci/
>    $ git grep blacklist v2.6.27 -- drivers/pci/intel-iommu.c
>    $
> 
> No ouput... hmm, I know it's in... oh maybe the path is incorrect
> 
>    $ git git grep blacklist v2.6.27 -- intel-iommu.c
>    v2.6.27:intel-iommu.c:static int blacklist_iommu(const struct dmi_system_id *id)
>    v2.6.27:intel-iommu.c:          .callback = blacklist_iommu,
> 
> ah, so Git failed previously without any comments on the wrong
> path... maybe it should ?

Good idea.

> So at that point I know that in the revision v2.6.27, there's a function
> called "blacklist_iommu" in drivers/pci/intel-iommu.c
> 
> Now take a look if it's still there in v2.6.28:
> 
>    $ git git grep blacklist v2.6.28 -- intel-iommu.c
>    $
> 
> This time nothing is printed but I know that the command is correct.
> 
> So now I'm interested in looking for the commit which removed this
> function. Fo this I'm trying to use git-log(1):
> 
>    $ git log --full-history --follow -S'static int blacklist_iommu(const struct dmi_system_id *id)' v2.6.27..v2.6.28 -- intel-iommu.c
>    $ echo $?
>    0
> 
> I tried different options but it fails.
> 
> Also I found that passing the exact string to '-S' is annoying, are
> there any way to just pass a part of the string such as
> "-Sblacklist_iommu" ?

This (-Sblacklist_iommu) works for me.

> Sorry if I miss the revelant git-log's option, but there're so many...

Try -m (show full diff for merge commits), as the change you're looking
for seems to have been introduced by a merge, not a regular commit.

	$ opts="--stat -Sblacklist_iommu -m --oneline"
	$ revs="v2.6.27..v2.6.28"

	$ git log $opts $revs -- drivers/pci/intel-iommu.c

This returns nothing.  Hmm.  Let's try this instead:

	$ git log $opts $revs -- drivers/pci/
	057316c (from 3e2dab9) Merge branch 'linus' into test
	 drivers/pci/intel-iommu.c |  307 ++++++++++++++++++++------------------------
	 1 files changed, 140 insertions(+), 167 deletions(-)
	6b2ada8 (from 3b7ecb5) Merge branches 'core/softlockup', 'core/softirq', 'core/resources', 'core/printk' and 'core/misc' into core-v28-for-linus
	 drivers/pci/intel-iommu.c |  187 ++++++--------------------------------------
	 1 files changed, 26 insertions(+), 161 deletions(-)
	d847059 (from 725c258) Merge branch 'x86/apic' into x86-v28-for-linus-phase4-B
	 drivers/pci/intel-iommu.c |  185 ++++++---------------------------------------
	 1 files changed, 25 insertions(+), 160 deletions(-)
	725c258 (from 129d6ab) Merge branches 'core/iommu', 'x86/amd-iommu' and 'x86/iommu' into x86-v28-for-linus-phase3-B
	 drivers/pci/intel-iommu.c |   25 ++++++++++++++++++++++++-
	 1 files changed, 24 insertions(+), 1 deletions(-)
	6e03f99 (from 9821626) Merge branch 'linus' into x86/iommu
	 drivers/pci/intel-iommu.c |   23 +++++++++++++++++++++++
	 1 files changed, 23 insertions(+), 0 deletions(-)

Strange, why did we need to remove the last path component to get these
results which say that exactly the file we previously specified was changed?

Also interesting, and matching the above results in that we see the need for
the -m flag confirmed:

	$ for merge in 057316c 6b2ada8 d847059 725c258 6e03f99
	do
		a=$(git show $merge | grep -c blacklist_iommu)
		b=$(git show -m $merge | grep -c blacklist_iommu)
		echo $merge $a $b
	done
	057316c 0 2
	6b2ada8 0 2
	d847059 0 2
	725c258 0 2
	6e03f99 0 2

IAW: the combined diff for the five found merges doesn't show any changes to
a line containing the string "blacklist_iommu", but the full diffs do.

Let's check for the presence of the string in the merge results and their
parents:

	$ for merge in 057316c 6b2ada8 d847059 725c258 6e03f99
	  do
		for rev in $(git show $merge | grep ^Merge:)
		do
			t=parent
			case $rev in
			Merge:)
				echo
				rev=$merge
				t=merge
				;;
			esac
			if git grep -q blacklist_iommu $rev -- drivers/pci/
			then
				echo "$t $rev: found"
			else
				echo "$t $rev: not found"
			fi
		done
	done

	merge 057316c: not found
	parent 3e2dab9: found
	parent 2515ddc: not found
	
	merge 6b2ada8: not found
	parent 278429c: not found
	parent 3b7ecb5: found
	parent 77af7e3: not found
	parent 1516071: not found
	parent 1fa63a8: not found
	parent 8546232: not found
	
	merge d847059: not found
	parent 725c258: found
	parent 11494547: not found
	
	merge 725c258: found
	parent 3dd392a: found
	parent 72d3105: found
	parent 129d6ab: not found
	parent 1e19b16: found
	
	merge 6e03f99: found
	parent 9821626: not found
	parent 6bfb09a: found

Hmm, seems like the function is gone since d847059.  Does all of this help
you in any way?

René

Re: Can't find the revelant commit with git-log

[Francis Moreau]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Am 25.01.2011 10:01, schrieb Francis Moreau:
>>
>> The project is the Linux kernel, and I'm trying to look for changes
>> which happened between v2.6.27 and v2.6.28. The changes happened in the
>> following file: drivers/pci/intel-iommu.c where a function has been
>> removed:
>> 
>>    $ git --version
>>    git version 1.7.4.rc3
>> 
>>    $ cd ~/linux-2.6/drivers/pci/
>>    $ git grep blacklist v2.6.27 -- drivers/pci/intel-iommu.c
>>    $
>> 
>> No ouput... hmm, I know it's in... oh maybe the path is incorrect
>> 
>>    $ git git grep blacklist v2.6.27 -- intel-iommu.c
>>    v2.6.27:intel-iommu.c:static int blacklist_iommu(const struct dmi_system_id *id)
>>    v2.6.27:intel-iommu.c:          .callback = blacklist_iommu,
>> 
>> ah, so Git failed previously without any comments on the wrong
>> path... maybe it should ?
>
> Good idea.
>

Actually I _think_ I encoutered this behaviour for other commands as well,
like git-show or git-log, I don't remember.

>
>> So at that point I know that in the revision v2.6.27, there's a function
>> called "blacklist_iommu" in drivers/pci/intel-iommu.c
>> 
>> Now take a look if it's still there in v2.6.28:
>> 
>>    $ git git grep blacklist v2.6.28 -- intel-iommu.c
>>    $
>> 
>> This time nothing is printed but I know that the command is correct.
>> 
>> So now I'm interested in looking for the commit which removed this
>> function. Fo this I'm trying to use git-log(1):
>> 
>>    $ git log --full-history --follow -S'static int blacklist_iommu(const struct dmi_system_id *id)' v2.6.27..v2.6.28 -- intel-iommu.c
>>    $ echo $?
>>    0
>> 
>> I tried different options but it fails.
>> 
>> Also I found that passing the exact string to '-S' is annoying, are
>> there any way to just pass a part of the string such as
>> "-Sblacklist_iommu" ?
>
> This (-Sblacklist_iommu) works for me.
>

Hmm, I thought I tried this and it didn't work, but that's obviously
wrong.

>
>> Sorry if I miss the revelant git-log's option, but there're so many...
>
> Try -m (show full diff for merge commits), as the change you're looking
> for seems to have been introduced by a merge, not a regular commit.
>
> 	$ opts="--stat -Sblacklist_iommu -m --oneline"
> 	$ revs="v2.6.27..v2.6.28"
>
> 	$ git log $opts $revs -- drivers/pci/intel-iommu.c
>
> This returns nothing.  Hmm.  Let's try this instead:
>
> 	$ git log $opts $revs -- drivers/pci/
> 	057316c (from 3e2dab9) Merge branch 'linus' into test
> 	 drivers/pci/intel-iommu.c |  307 ++++++++++++++++++++------------------------
> 	 1 files changed, 140 insertions(+), 167 deletions(-)
> 	6b2ada8 (from 3b7ecb5) Merge branches 'core/softlockup', 'core/softirq', 'core/resources', 'core/printk' and 'core/misc' into core-v28-for-linus
> 	 drivers/pci/intel-iommu.c |  187 ++++++--------------------------------------
> 	 1 files changed, 26 insertions(+), 161 deletions(-)
> 	d847059 (from 725c258) Merge branch 'x86/apic' into x86-v28-for-linus-phase4-B
> 	 drivers/pci/intel-iommu.c |  185 ++++++---------------------------------------
> 	 1 files changed, 25 insertions(+), 160 deletions(-)
> 	725c258 (from 129d6ab) Merge branches 'core/iommu', 'x86/amd-iommu' and 'x86/iommu' into x86-v28-for-linus-phase3-B
> 	 drivers/pci/intel-iommu.c |   25 ++++++++++++++++++++++++-
> 	 1 files changed, 24 insertions(+), 1 deletions(-)
> 	6e03f99 (from 9821626) Merge branch 'linus' into x86/iommu
> 	 drivers/pci/intel-iommu.c |   23 +++++++++++++++++++++++
> 	 1 files changed, 23 insertions(+), 0 deletions(-)
>
> Strange, why did we need to remove the last path component to get these
> results which say that exactly the file we previously specified was changed?

ah... I think I've been hit by this, since I tried '-m' too but see
nothing and was not smart enough to remove the filename from the path.

> Also interesting, and matching the above results in that we see the need for
> the -m flag confirmed:
>
> 	$ for merge in 057316c 6b2ada8 d847059 725c258 6e03f99
> 	do
> 		a=$(git show $merge | grep -c blacklist_iommu)
> 		b=$(git show -m $merge | grep -c blacklist_iommu)
> 		echo $merge $a $b
> 	done
> 	057316c 0 2
> 	6b2ada8 0 2
> 	d847059 0 2
> 	725c258 0 2
> 	6e03f99 0 2
>
> IAW: the combined diff for the five found merges doesn't show any changes to
> a line containing the string "blacklist_iommu", but the full diffs do.
>
> Let's check for the presence of the string in the merge results and their
> parents:
>
> 	$ for merge in 057316c 6b2ada8 d847059 725c258 6e03f99
> 	  do
> 		for rev in $(git show $merge | grep ^Merge:)
> 		do
> 			t=parent
> 			case $rev in
> 			Merge:)
> 				echo
> 				rev=$merge
> 				t=merge
> 				;;
> 			esac
> 			if git grep -q blacklist_iommu $rev -- drivers/pci/
> 			then
> 				echo "$t $rev: found"
> 			else
> 				echo "$t $rev: not found"
> 			fi
> 		done
> 	done
>
> 	merge 057316c: not found
> 	parent 3e2dab9: found
> 	parent 2515ddc: not found
> 	
> 	merge 6b2ada8: not found
> 	parent 278429c: not found
> 	parent 3b7ecb5: found
> 	parent 77af7e3: not found
> 	parent 1516071: not found
> 	parent 1fa63a8: not found
> 	parent 8546232: not found
> 	
> 	merge d847059: not found
> 	parent 725c258: found
> 	parent 11494547: not found
> 	
> 	merge 725c258: found
> 	parent 3dd392a: found
> 	parent 72d3105: found
> 	parent 129d6ab: not found
> 	parent 1e19b16: found
> 	
> 	merge 6e03f99: found
> 	parent 9821626: not found
> 	parent 6bfb09a: found
>
> Hmm, seems like the function is gone since d847059.  Does all of this help
> you in any way?

Yes it does, but one question I'm wondering is: is it possible to do
this more user friendly ? ;)

Thanks
-- 
Francis

Re: Can't find the revelant commit with git-log

[Francis Moreau]

Francis Moreau <francis.moro@gmail.com> writes:

> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>
>> Am 25.01.2011 10:01, schrieb Francis Moreau:
>>>
>>> The project is the Linux kernel, and I'm trying to look for changes
>>> which happened between v2.6.27 and v2.6.28. The changes happened in the
>>> following file: drivers/pci/intel-iommu.c where a function has been
>>> removed:
>>> 
>>>    $ git --version
>>>    git version 1.7.4.rc3
>>> 
>>>    $ cd ~/linux-2.6/drivers/pci/
>>>    $ git grep blacklist v2.6.27 -- drivers/pci/intel-iommu.c
>>>    $
>>> 
>>> No ouput... hmm, I know it's in... oh maybe the path is incorrect
>>> 
>>>    $ git git grep blacklist v2.6.27 -- intel-iommu.c
>>>    v2.6.27:intel-iommu.c:static int blacklist_iommu(const struct dmi_system_id *id)
>>>    v2.6.27:intel-iommu.c:          .callback = blacklist_iommu,
>>> 
>>> ah, so Git failed previously without any comments on the wrong
>>> path... maybe it should ?
>>
>> Good idea.
>>
>
> Actually I _think_ I encoutered this behaviour for other commands as well,
> like git-show or git-log, I don't remember.
>
>>
>>> So at that point I know that in the revision v2.6.27, there's a function
>>> called "blacklist_iommu" in drivers/pci/intel-iommu.c
>>> 
>>> Now take a look if it's still there in v2.6.28:
>>> 
>>>    $ git git grep blacklist v2.6.28 -- intel-iommu.c
>>>    $
>>> 
>>> This time nothing is printed but I know that the command is correct.
>>> 
>>> So now I'm interested in looking for the commit which removed this
>>> function. Fo this I'm trying to use git-log(1):
>>> 
>>>    $ git log --full-history --follow -S'static int blacklist_iommu(const struct dmi_system_id *id)' v2.6.27..v2.6.28 -- intel-iommu.c
>>>    $ echo $?
>>>    0
>>> 
>>> I tried different options but it fails.
>>> 
>>> Also I found that passing the exact string to '-S' is annoying, are
>>> there any way to just pass a part of the string such as
>>> "-Sblacklist_iommu" ?
>>
>> This (-Sblacklist_iommu) works for me.
>>
>
> Hmm, I thought I tried this and it didn't work, but that's obviously
> wrong.
>
>>
>>> Sorry if I miss the revelant git-log's option, but there're so many...
>>
>> Try -m (show full diff for merge commits), as the change you're looking
>> for seems to have been introduced by a merge, not a regular commit.
>>
>> 	$ opts="--stat -Sblacklist_iommu -m --oneline"
>> 	$ revs="v2.6.27..v2.6.28"
>>
>> 	$ git log $opts $revs -- drivers/pci/intel-iommu.c
>>
>> This returns nothing.  Hmm.  Let's try this instead:
>>
>> 	$ git log $opts $revs -- drivers/pci/
>> 	057316c (from 3e2dab9) Merge branch 'linus' into test
>> 	 drivers/pci/intel-iommu.c |  307 ++++++++++++++++++++------------------------
>> 	 1 files changed, 140 insertions(+), 167 deletions(-)
>> 	6b2ada8 (from 3b7ecb5) Merge branches 'core/softlockup', 'core/softirq', 'core/resources', 'core/printk' and 'core/misc' into core-v28-for-linus
>> 	 drivers/pci/intel-iommu.c |  187 ++++++--------------------------------------
>> 	 1 files changed, 26 insertions(+), 161 deletions(-)
>> 	d847059 (from 725c258) Merge branch 'x86/apic' into x86-v28-for-linus-phase4-B
>> 	 drivers/pci/intel-iommu.c |  185 ++++++---------------------------------------
>> 	 1 files changed, 25 insertions(+), 160 deletions(-)
>> 	725c258 (from 129d6ab) Merge branches 'core/iommu', 'x86/amd-iommu' and 'x86/iommu' into x86-v28-for-linus-phase3-B
>> 	 drivers/pci/intel-iommu.c |   25 ++++++++++++++++++++++++-
>> 	 1 files changed, 24 insertions(+), 1 deletions(-)
>> 	6e03f99 (from 9821626) Merge branch 'linus' into x86/iommu
>> 	 drivers/pci/intel-iommu.c |   23 +++++++++++++++++++++++
>> 	 1 files changed, 23 insertions(+), 0 deletions(-)
>>
>> Strange, why did we need to remove the last path component to get these
>> results which say that exactly the file we previously specified was changed?
>
> ah... I think I've been hit by this, since I tried '-m' too but see
> nothing and was not smart enough to remove the filename from the path.
>
>> Also interesting, and matching the above results in that we see the need for
>> the -m flag confirmed:

BTW, couldn't '-m' automatically set when '-S' is given ?

>> 
>>
>> 	$ for merge in 057316c 6b2ada8 d847059 725c258 6e03f99
>> 	do
>> 		a=$(git show $merge | grep -c blacklist_iommu)
>> 		b=$(git show -m $merge | grep -c blacklist_iommu)
>> 		echo $merge $a $b
>> 	done
>> 	057316c 0 2
>> 	6b2ada8 0 2
>> 	d847059 0 2
>> 	725c258 0 2
>> 	6e03f99 0 2
>>
>> IAW: the combined diff for the five found merges doesn't show any changes to
>> a line containing the string "blacklist_iommu", but the full diffs do.
>>
>> Let's check for the presence of the string in the merge results and their
>> parents:
>>
>> 	$ for merge in 057316c 6b2ada8 d847059 725c258 6e03f99
>> 	  do
>> 		for rev in $(git show $merge | grep ^Merge:)
>> 		do
>> 			t=parent
>> 			case $rev in
>> 			Merge:)
>> 				echo
>> 				rev=$merge
>> 				t=merge
>> 				;;
>> 			esac
>> 			if git grep -q blacklist_iommu $rev -- drivers/pci/
>> 			then
>> 				echo "$t $rev: found"
>> 			else
>> 				echo "$t $rev: not found"
>> 			fi
>> 		done
>> 	done
>>
>> 	merge 057316c: not found
>> 	parent 3e2dab9: found
>> 	parent 2515ddc: not found
>> 	
>> 	merge 6b2ada8: not found
>> 	parent 278429c: not found
>> 	parent 3b7ecb5: found
>> 	parent 77af7e3: not found
>> 	parent 1516071: not found
>> 	parent 1fa63a8: not found
>> 	parent 8546232: not found
>> 	
>> 	merge d847059: not found
>> 	parent 725c258: found
>> 	parent 11494547: not found
>> 	
>> 	merge 725c258: found
>> 	parent 3dd392a: found
>> 	parent 72d3105: found
>> 	parent 129d6ab: not found
>> 	parent 1e19b16: found
>> 	
>> 	merge 6e03f99: found
>> 	parent 9821626: not found
>> 	parent 6bfb09a: found
>>
>> Hmm, seems like the function is gone since d847059.  Does all of this help
>> you in any way?
>
> Yes it does, but one question I'm wondering is: is it possible to do
> this more user friendly ? ;)

I tried to reproduce something similar but with a far more simple repo:


<v2.6.28> 1.f o
              |
          1.e o (merge)
              | \
          1.d o  o 2.c (merge)
              |  | \
          1.c o  o  o 3.a "Remove blacklist_iommu()"
              |  | /
              |  o 2.a
              | /
          1.b o
              |
<v2.6.27> 1.a o "Introduce blacklist_iommu()"
              |
              o Init

Basically this repo 3 branches: master, 2, 3. Master branch introduces
the "blacklist_iommu()" function with commit 1.a, and branch "3" removes
it at commit 3.a.

So now:

   $ git log --oneline -S"blacklist_iommu"
   ea1ddb0 3.a
   8053460 1.a


   $ git show :/3.a
   commit ea1ddb023833470be173363a71a3a055ae8acb85
   Author: Francis Moreau <fmoreau@spoutnik.(none)>
   Date:   Wed Jan 26 09:17:58 2011 +0100
   
       3.a
       
       Remove blacklist_iommy()
   
   diff --git a/intel-iommu.c b/intel-iommu.c
   index 2c9539d..f19c1cc 100644
   --- a/intel-iommu.c
   +++ b/intel-iommu.c
   @@ -2,12 +2,4 @@ Wed Jan 26 09:10:15 CET 2011
    Wed Jan 26 09:16:23 CET 2011
    
    
   -static int blacklist_iommu(const struct dmi_system_id *id)
   -{
   -  printk(KERN_INFO "%s detected; disabling IOMMU\n",
   -	 id->ident);
   -  dmar_disabled = 1;
   -  return 0;
   -}
   -
    Wed Jan 26 09:14:40 CET 2011

So in this case there's no need to pass the '-m' flag and git-log(1), by
default walks through all the commits:

   $ git log --oneline v2.6.27..v2.6.28
   e2a5af3 1.e
   c9fbd3a Merge branch '2'
   a8207d9 Merge branch '3' into 2
   ea1ddb0 3.a
   3a86ada 2.b
   767186a 2.a
   45beaac 1.d
   1af781c 1.c
   4f9b5c1 1.b
   8053460 1.a

whereas in my previous example, it doesn't seem to be true.

Could you shed some light ?

Thanks
-- 
Francis

Re: Can't find the revelant commit with git-log

[Francis Moreau]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

[...]

> Hmm, seems like the function is gone since d847059.  Does all of this help
> you in any way?

Sigh... one more try, one more failure...

This time I tried to use git-bisect(1) to see if I couldn't track the
change more easily.

So, here's what I did:

    $ cat ./bisect.sh
    #! /bin/sh
    
    if ! git grep -q -e blacklist_iommu -- drivers/pci/intel-iommu.c
    then
    	exit 1
    fi

    $ git bisect start v2.6.28 v2.6.27 -- drivers/pci
    Bisecting: 70 revisions left to test after this (roughly 6 steps)
    [a0bfb673dca8a2b4324fe11e678ec6d6a9ad67e0] Merge branch 'linux-next' of
    git://git.kernel.org/pub/scm/linux/kernel/git/jbarnes/pci-2.6

    $ git bisect run ./bisect.sh 
    running /dev/shm/bisect.sh
    Bisecting: 42 revisions left to test after this (roughly 5 steps)
    [0235c4fc7fc6f621dc0dd89eba102ad5aa373390] PCI PM: Introduce function
    pci_wake_from_d3
    running /dev/shm/bisect.sh
    error: The following untracked working tree files would be overwritten
    by checkout:
       arch/x86/es7000/Makefile
       arch/x86/es7000/es7000.h
       arch/x86/es7000/es7000plat.c
       drivers/pci/dma_remapping.h
    Please move or remove them before you can switch branches.
    Bisecting: 18 revisions left to test after this (roughly 4 steps)
    Aborting
    bisect run failed:
    'bisect_state bad' exited with error code 1

Do you know what's going wrong ?

Thanks
-- 
Francis

Re: Can't find the revelant commit with git-log

[Johannes Sixt]

Please don't set Mail-Followup-To; it's disliked on this list.

Am 1/26/2011 9:36, schrieb Francis Moreau:
> I tried to reproduce something similar but with a far more simple repo:
> 
> 
> <v2.6.28> 1.f o
>               |
>           1.e o (merge)
>               | \
>           1.d o  o 2.c (merge)
>               |  | \
>           1.c o  o  o 3.a "Remove blacklist_iommu()"
>               |  | /
>               |  o 2.a
>               | /
>           1.b o
>               |
> <v2.6.27> 1.a o "Introduce blacklist_iommu()"
>               |
>               o Init
> 
> Basically this repo 3 branches: master, 2, 3. Master branch introduces
> the "blacklist_iommu()" function with commit 1.a, and branch "3" removes
> it at commit 3.a.
> ...
> So in this case there's no need to pass the '-m' flag and git-log(1), by
> default walks through all the commits:

To reproduce the real history, you have to modify your example in three ways:

1. 2.a must be forked off of Init, not 1.b; i.e., this commit does not
contain "blacklist_iommu".

2. Drop the side branch that removes the word. (Drop at least the commit.)

3. The merge 1.e (which resembles d847059) must be modified such that it
takes the contents of 2.c rather than 1.d.

IOW, "blacklist_iommu" is not removed explicitly by a commit, but rather
by a merge of one branch that has it and another one that doesn't have it.

Look closely at d847059: The commit message hints at a conflict in
intel_iommu.c, and Ingo resolved the conflict by taking the contents of
the file of one branch, namely the branch that does *not* contain
"blacklist_iommu", ignoring entirely what happend on the other branch
(that had added "blacklist_iommu" somewhere at or before v2.6.27).

-- Hannes

Re: Can't find the revelant commit with git-log

[René Scharfe]

Am 26.01.2011 09:36, schrieb Francis Moreau:
> Francis Moreau <francis.moro@gmail.com> writes:
> 
>> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>>
>>> Try -m (show full diff for merge commits), as the change you're looking
>>> for seems to have been introduced by a merge, not a regular commit.
>>>
>>> 	$ opts="--stat -Sblacklist_iommu -m --oneline"
>>> 	$ revs="v2.6.27..v2.6.28"
>>>
>>> 	$ git log $opts $revs -- drivers/pci/intel-iommu.c
>>>
>>> This returns nothing.  Hmm.  Let's try this instead:
>>>
>>> 	$ git log $opts $revs -- drivers/pci/
>>> 	057316c (from 3e2dab9) Merge branch 'linus' into test
>>> 	 drivers/pci/intel-iommu.c |  307 ++++++++++++++++++++------------------------
>>> 	 1 files changed, 140 insertions(+), 167 deletions(-)
>>> 	6b2ada8 (from 3b7ecb5) Merge branches 'core/softlockup', 'core/softirq', 'core/resources', 'core/printk' and 'core/misc' into core-v28-for-linus
>>> 	 drivers/pci/intel-iommu.c |  187 ++++++--------------------------------------
>>> 	 1 files changed, 26 insertions(+), 161 deletions(-)
>>> 	d847059 (from 725c258) Merge branch 'x86/apic' into x86-v28-for-linus-phase4-B
>>> 	 drivers/pci/intel-iommu.c |  185 ++++++---------------------------------------
>>> 	 1 files changed, 25 insertions(+), 160 deletions(-)
>>> 	725c258 (from 129d6ab) Merge branches 'core/iommu', 'x86/amd-iommu' and 'x86/iommu' into x86-v28-for-linus-phase3-B
>>> 	 drivers/pci/intel-iommu.c |   25 ++++++++++++++++++++++++-
>>> 	 1 files changed, 24 insertions(+), 1 deletions(-)
>>> 	6e03f99 (from 9821626) Merge branch 'linus' into x86/iommu
>>> 	 drivers/pci/intel-iommu.c |   23 +++++++++++++++++++++++
>>> 	 1 files changed, 23 insertions(+), 0 deletions(-)
>>>
>>> Strange, why did we need to remove the last path component to get these
>>> results which say that exactly the file we previously specified was changed?
>>
>> ah... I think I've been hit by this, since I tried '-m' too but see
>> nothing and was not smart enough to remove the filename from the path.
>>
>>> Also interesting, and matching the above results in that we see the need for
>>> the -m flag confirmed:
> 
> BTW, couldn't '-m' automatically set when '-S' is given ?

I can't see a connection between the two options.  Merges are ignored by
default (without -m) because they shouldn't contain any changes that
aren't already present in one of the merged branches (by convention),
and diffs with a single parent are easier to read.  This is true with or
without -S.

So far we have two action items, I think:

	- Make git grep report non-matching path specs (new feature).

	- Find out why removing the last path component made a
	  difference in the case above (looks like a bug, but I don't
	  understand what's going on).

Taking into account what Johannes said regarding the disappearance of
the function during a merge instead of as part of a regular commit, I
don't think these changes would help you much with your use case,
though.  You would still be looking at a complicated net of commits,
with the action happening in non-obvious places (merges).

Perhaps --graph can help a bit, see for yourself:

	$ git log --graph -Sblacklist_iommu -m --oneline \
	v2.6.27..v2.6.28 -- drivers/pci/

If you start at v.2.6.26 instead -- e.g. to catch the commit that
introduced the function -- then the graph gets a lot more colourful and
too complicated, at least for me, though.

René

Re: Can't find the revelant commit with git-log

[René Scharfe]

Am 26.01.2011 10:01, schrieb Francis Moreau:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
> [...]
> 
>> Hmm, seems like the function is gone since d847059.  Does all of this help
>> you in any way?
> 
> Sigh... one more try, one more failure...
> 
> This time I tried to use git-bisect(1) to see if I couldn't track the
> change more easily.
> 
> So, here's what I did:
> 
>     $ cat ./bisect.sh
>     #! /bin/sh
>     
>     if ! git grep -q -e blacklist_iommu -- drivers/pci/intel-iommu.c
>     then
>     	exit 1
>     fi
> 
>     $ git bisect start v2.6.28 v2.6.27 -- drivers/pci
>     Bisecting: 70 revisions left to test after this (roughly 6 steps)
>     [a0bfb673dca8a2b4324fe11e678ec6d6a9ad67e0] Merge branch 'linux-next' of
>     git://git.kernel.org/pub/scm/linux/kernel/git/jbarnes/pci-2.6
> 
>     $ git bisect run ./bisect.sh 
>     running /dev/shm/bisect.sh
>     Bisecting: 42 revisions left to test after this (roughly 5 steps)
>     [0235c4fc7fc6f621dc0dd89eba102ad5aa373390] PCI PM: Introduce function
>     pci_wake_from_d3
>     running /dev/shm/bisect.sh
>     error: The following untracked working tree files would be overwritten
>     by checkout:
>        arch/x86/es7000/Makefile
>        arch/x86/es7000/es7000.h
>        arch/x86/es7000/es7000plat.c
>        drivers/pci/dma_remapping.h
>     Please move or remove them before you can switch branches.

These are untracked files; your tree is not clean.  Either commit them,
stash them away or delete the files.  Or make a local clone just for
bisecting purposes.

>     Bisecting: 18 revisions left to test after this (roughly 4 steps)
>     Aborting
>     bisect run failed:
>     'bisect_state bad' exited with error code 1
> 
> Do you know what's going wrong ?

I tried the same and the result was:

	e61d98d8dad0048619bb138b0ff996422ffae53b is the first bad commit

But this probably won't help you much, either, as it is a regular commit
which doesn't touch the function (it doesn't exist before and after it).

I can only guess that bisect gets confused by the merges just like log
(and me).

René

Re: Can't find the revelant commit with git-log

[Francis Moreau]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Am 26.01.2011 10:01, schrieb Francis Moreau:

[...]

>>     error: The following untracked working tree files would be overwritten
>>     by checkout:
>>        arch/x86/es7000/Makefile
>>        arch/x86/es7000/es7000.h
>>        arch/x86/es7000/es7000plat.c
>>        drivers/pci/dma_remapping.h
>>     Please move or remove them before you can switch branches.
>
> These are untracked files; your tree is not clean.  Either commit them,
> stash them away or delete the files.  Or make a local clone just for
> bisecting purposes.
>

Hmm, I'm wondering how those files get there...

Maybe they are some left from some git commands which failed, I dunno.

-- 
Francis

Re: Can't find the revelant commit with git-log

[Francis Moreau]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Please don't set Mail-Followup-To; it's disliked on this list.

Just out of curiosity, I'd like to know why, since it has no annoyance
for those who are replying to my emails.

>
>
> Am 1/26/2011 9:36, schrieb Francis Moreau:
>> I tried to reproduce something similar but with a far more simple repo:
>> 
>> 
>> <v2.6.28> 1.f o
>>               |
>>           1.e o (merge)
>>               | \
>>           1.d o  o 2.c (merge)
>>               |  | \
>>           1.c o  o  o 3.a "Remove blacklist_iommu()"
>>               |  | /
>>               |  o 2.a
>>               | /
>>           1.b o
>>               |
>> <v2.6.27> 1.a o "Introduce blacklist_iommu()"
>>               |
>>               o Init
>> 
>> Basically this repo 3 branches: master, 2, 3. Master branch introduces
>> the "blacklist_iommu()" function with commit 1.a, and branch "3" removes
>> it at commit 3.a.
>> ...
>> So in this case there's no need to pass the '-m' flag and git-log(1), by
>> default walks through all the commits:
>
> To reproduce the real history, you have to modify your example in three ways:
>
> 1. 2.a must be forked off of Init, not 1.b; i.e., this commit does not
> contain "blacklist_iommu".
>
> 2. Drop the side branch that removes the word. (Drop at least the commit.)
>
> 3. The merge 1.e (which resembles d847059) must be modified such that it
> takes the contents of 2.c rather than 1.d.
>
> IOW, "blacklist_iommu" is not removed explicitly by a commit, but rather
> by a merge of one branch that has it and another one that doesn't have it.
>
> Look closely at d847059: The commit message hints at a conflict in
> intel_iommu.c,

But how did you find out d847059 ?

Did you use René's method ?

Thanks
-- 
Francis

Re: Can't find the revelant commit with git-log

[Sverre Rabbelier]

Heya,

On Wed, Jan 26, 2011 at 21:56, Francis Moreau <francis.moro@gmail.com> wrote:
> Just out of curiosity, I'd like to know why, since it has no annoyance
> for those who are replying to my emails.

We keep everybody on CC who's interested in the thread here, and
usually reply directly to the person whose email we respond to. In
this case, I had to manually edit the TO line to be just you, and move
everybody else to the CC.

-- 
Cheers,

Sverre Rabbelier

Re: Can't find the revelant commit with git-log

[Francis Moreau]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Heya,
>
> On Wed, Jan 26, 2011 at 21:56, Francis Moreau <francis.moro@gmail.com> wrote:
>> Just out of curiosity, I'd like to know why, since it has no annoyance
>> for those who are replying to my emails.
>
> We keep everybody on CC who's interested in the thread here, and
> usually reply directly to the person whose email we respond to. In
> this case, I had to manually edit the TO line to be just you, and move
> everybody else to the CC.

Well, if I decided to set Mail-Followup-To, that do mean that I don't
want you to do that !

-- 
Francis

Re: Can't find the revelant commit with git-log

[Sverre Rabbelier]

Heya,

On Wed, Jan 26, 2011 at 22:08, Francis Moreau <francis.moro@gmail.com> wrote:
> Well, if I decided to set Mail-Followup-To, that do mean that I don't
> want you to do that !

Yes, but it makes it harder for us to do that. For example, now I had
to remove myself from the to line, add you, and move everybody else to
CC. In other words, please don't do it. Thanks :)

-- 
Cheers,

Sverre Rabbelier

Re: Can't find the revelant commit with git-log

[Junio C Hamano]

Francis Moreau <francis.moro@gmail.com> writes:

>> On Wed, Jan 26, 2011 at 21:56, Francis Moreau <francis.moro@gmail.com> wrote:
>>> Just out of curiosity, I'd like to know why, since it has no annoyance
>>> for those who are replying to my emails.
>>
>> We keep everybody on CC who's interested in the thread here, and
>> usually reply directly to the person whose email we respond to. In
>> this case, I had to manually edit the TO line to be just you, and move
>> everybody else to the CC.
>
> Well, if I decided to set Mail-Followup-To, that do mean that I don't
> want you to do that !

That is _not_ something for _you_ to unilaterally decide.

I am responding to _you_ with this message to tell _you_ something, while
keeping Sverre, J6t and Rene and the list in the loop.

If I followedyour M-F-T on the message I am responding to, I would have
ended up placing these "secondary" (for the purpose of my message) on my
"To:" line.  That would make it impossible for them to prioritize the
messages addressed directly to them (which would have their address on
"To:") over other messages that they are merely in the loop (which would
not have them on "To:"---their address may be on "Cc:" or they may be
getting the message from the list).

So please don't.

Thanks.

Re: Can't find the revelant commit with git-log

[Francis Moreau]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Heya,
>
> On Wed, Jan 26, 2011 at 22:08, Francis Moreau <francis.moro@gmail.com> wrote:
>> Well, if I decided to set Mail-Followup-To, that do mean that I don't
>> want you to do that !
>
> Yes, but it makes it harder for us to do that. For example, now I had
> to remove myself from the to line, add you, and move everybody else to
> CC. In other words, please don't do it. Thanks :)

It should be fixed by now, please let me know it isn't !

I didn't know it works like this: I thought it was set so replies just
replace my email address with the mailing list one.

Thanks
-- 
Francis

Re: Can't find the revelant commit with git-log

[Francis Moreau]

Junio C Hamano <gitster@pobox.com> writes:

> Francis Moreau <francis.moro@gmail.com> writes:
>
>>> On Wed, Jan 26, 2011 at 21:56, Francis Moreau <francis.moro@gmail.com> wrote:
>>>> Just out of curiosity, I'd like to know why, since it has no annoyance
>>>> for those who are replying to my emails.
>>>
>>> We keep everybody on CC who's interested in the thread here, and
>>> usually reply directly to the person whose email we respond to. In
>>> this case, I had to manually edit the TO line to be just you, and move
>>> everybody else to the CC.
>>
>> Well, if I decided to set Mail-Followup-To, that do mean that I don't
>> want you to do that !
>
> That is _not_ something for _you_ to unilaterally decide.
>
> I am responding to _you_ with this message to tell _you_ something, while
> keeping Sverre, J6t and Rene and the list in the loop.
>
> If I followedyour M-F-T on the message I am responding to, I would have
> ended up placing these "secondary" (for the purpose of my message) on my
> "To:" line.  That would make it impossible for them to prioritize the
> messages addressed directly to them (which would have their address on
> "To:") over other messages that they are merely in the loop (which would
> not have them on "To:"---their address may be on "Cc:" or they may be
> getting the message from the list).
>
> So please don't.

Agreed.

Should be fixed now, if not, please let me know.

Thanks
-- 
Francis

Re: Can't find the revelant commit with git-log

[René Scharfe]

Am 26.01.2011 19:11, schrieb René Scharfe:
> So far we have two action items, I think:
> 
> - Make git grep report non-matching path specs (new feature).
> 
> - Find out why removing the last path component made a difference in
> the case above (looks like a bug, but I don't understand what's going
> on).

OK, regarding the second point:

Merges that have at least one parent without changes in the selected
subset of files won't be displayed, not even with --full-history.  That
explains why removing the last path component made a difference: all the
merges ended up with a version of the file that matched one of their
parents, but there were other changes in the directory.

This is a feature: since the version of the file picked by the merge
must have been introduced by an earlier commit (a regular one,
presumably), you'll find it there anyway.

And this history simplification takes precedence over pickaxe (-S).

The patch below turns down its aggressiveness when the pickaxe is swung
at the same time.  Here's what it does to your use case:

	$ revs="v2.6.26..v2.6.29"
	$ opts="-Sblacklist_iommu --oneline -m --full-history"

	# This takes quite a while...
	$ git log $opts $revs | wc -l
	160

	# Without the patch:
	$ git log $opts $revs -- drivers/pci/intel-iommu.c | wc -l
	2

	# With the patch (really just its first hunk):
	$ git log $opts $revs -- drivers/pci/intel-iommu.c | wc -l
	160

	$ opts="-Sblacklist_iommu --oneline -m"

	# This takes quite a while...
	$ git log $opts $revs | wc -l
	160

	# Without the patch:
	$ git log $opts $revs -- drivers/pci/intel-iommu.c | wc -l
	0

	# With the patch:
	$ git log $opts $revs -- drivers/pci/intel-iommu.c | wc -l
	160

	$ opts="-Sblacklist_iommu --oneline"

	# This takes a bit, but not too long.
	$ git log $opts $revs | wc -l
	1

	# Without the patch:
	$ git log $opts $revs -- drivers/pci/intel-iommu.c | wc -l
	0

	# With the patch:
	$ git log $opts $revs -- drivers/pci/intel-iommu.c | wc -l
	1

The full output matches exactly if the number of lines match.  That's to
be expected, as the string "blacklist_iommu" only ever appears in the
file drivers/pci/intel-iommu.c.

It wasn't mentioned before v2.6.26 or after v2.6.29.

There is only one regular commit, namely the initial one that introduced
the function.  Some merges are reported more than once, each for every
parent where -S hit.  135 unique commits are reported.

-- >8 --
Subject: pickaxe: don't simplify history too much

If pickaxe is used, turn off history simplification and make sure to keep
merges with at least one interesting parent.

If path specs are used, merges that have at least one parent whose files
match those in the specified subset are edited out.  This is good in
general, but leads to unexpectedly few results if used together with
pickaxe.  Merges that also have an interesting parent (in terms of -S or
-G) are dropped, too.

This change makes sure pickaxe takes precedence over history
simplification.  This means path specs won't change the results as long
as they contain all the files that pickaxe turns up.  E.g. these two
commands now report the same single commit that added the function
blacklist_iommu to the specified file in the Linux kernel repo:

   $ git log -Sblacklist_iommu v2.6.26..v2.6.29 --
   $ git log -Sblacklist_iommu v2.6.26..v2.6.29 -- drivers/pci/intel-iommu.c

Previously the second one came up empty.

Reported-by: Francis Moreau <francis.moro@gmail.com>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 revision.c                   |    5 +++++
 t/t6012-rev-list-simplify.sh |    2 ++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/revision.c b/revision.c
index 7b9eaef..cacf60c 100644
--- a/revision.c
+++ b/revision.c
@@ -441,6 +441,8 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
 	}
 	if (tree_changed && !tree_same)
 		return;
+	if (tree_changed && revs->diffopt.pickaxe)
+		return;
 	commit->object.flags |= TREESAME;
 }
 
@@ -1647,6 +1649,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	    revs->diffopt.filter ||
 	    DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES))
 		revs->diff = 1;
+
+	if (revs->diffopt.pickaxe)
+		revs->simplify_history = 0;
 
 	if (revs->topo_order)
 		revs->limited = 1;
diff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh
index af34a1e..b4fb8d0 100755
--- a/t/t6012-rev-list-simplify.sh
+++ b/t/t6012-rev-list-simplify.sh
@@ -86,5 +86,7 @@ check_result 'I H E C B A' --full-history --date-order -- file
 check_result 'I E C B A' --simplify-merges -- file
 check_result 'I B A' -- file
 check_result 'I B A' --topo-order -- file
+check_result 'I C B' -SHello
+check_result 'I C B' -SHello -- file
 
 test_done
-- 
1.7.3.4

Re: Can't find the revelant commit with git-log

[René Scharfe]

Am 26.01.2011 19:11, schrieb René Scharfe:
> - Make git grep report non-matching path specs (new feature).

This is a bit complicated because grep can work on files, index entries
as well as versioned objects and supports wildcards, so it's not that
easy to tell if a path spec matches something or is a rather typo.  But
it's not impossible either, of course.

What you can do until someone implements it is to simply omit the double
dash.  Path specs are then looked up as revs and files and you'll get an
error if they can't be found:

	# In the Linux kernel repo; we enter the wrong directory:
	$ cd drivers
	$ git grep blacklist_iommu v2.6.27 intel-iommu.c
	fatal: ambiguous argument 'intel-iommu.c': unknown revision or path not in the working tree.
	Use '--' to separate paths from revisions

	# Now we enter the right one and try again:
	$ cd pci
	$ git grep blacklist_iommu v2.6.27 intel-iommu.c
	v2.6.27:intel-iommu.c:static int blacklist_iommu(const struct dmi_system_id *id)
	v2.6.27:intel-iommu.c:		.callback = blacklist_iommu,

This won't work in bare repos or with wildcards, but it's better than
nothing.  And it saves you a few keystrokes.

René

Re: Can't find the revelant commit with git-log

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Subject: pickaxe: don't simplify history too much
>
> If pickaxe is used, turn off history simplification and make sure to keep
> merges with at least one interesting parent.
>
> If path specs are used, merges that have at least one parent whose files
> match those in the specified subset are edited out.  This is good in
> general, but leads to unexpectedly few results if used together with
> pickaxe.  Merges that also have an interesting parent (in terms of -S or
> -G) are dropped, too.
>
> This change makes sure pickaxe takes precedence over history
> simplification.

Hmmm, I understand the _motivation_ behind the change in the second hunk,
in that you _might_ want to dig the side branch that did not contribute
anything to the end result when looking for a needle with either -S or -G,
but doesn't the same logic apply to things like --grep?

I do not think it is a good idea to unconditionally disable simplification
for -S/G without a way for the user to countermand (even though I could be
persuaded to say that the flipping the default for -S/-G/--grep might have
been a better alternative in hindsight).

The user can control this behaviour by giving or not giving --simplify
from the command line anyway, no?

As to the first hunk, I have no idea why this is a good change.

It feels as if you are fighting against what this part of the code does in
try_to_simplify_commit():

	switch (rev_compare_tree(revs, p, commit)) {
	case REV_TREE_SAME:
		tree_same = 1;
		if (!revs->simplify_history || (p->object.flags & UNINTERESTING)) {
			/* ... */
			pp = &parent->next;
			continue;
		}
		parent->next = NULL;
		commit->parents = parent;
		commit->object.flags |= TREESAME;
		return;
	...

When we see a _single_ parent that has the same tree, we set tree_same and
also cull the parent list to contain only that parent.  When we are not
simplifying the history, we do not cull the parent list and will inspect
other parents as well, but still we set tree_same to 1 here.  When some
other parent is found to be different, we set tree_changed to 1.  So we
have four states (same = (0, 1) x changed = (0, 1)).

The code before your addition in the first hunk says that we keep the
commit if there is no parent with the same contents (i.e. !tree_same) and
there is at least one parent with different contents (i.e. tree_changed).
I suspect that this logic may not be working well when we do not simplify
the merge.

Let's look at the original code before your patch again.

 1. If all the parents of a commit are the same, we will see (tree_same &&
    !tree_changed), so we get TREESAME.

 2. If some but not all of the parents are the same, we will see (tree_same
    && tree_changed), and we end up getting TREESAME.

 3. If none of the parents is the same, (!tree_same && tree_changed) holds
    true, and we do not get TREESAME.

Perhaps the second condition needs to be tweaked for the "do not simplify
merges" case?  That is, we split 2. into two cases:

 2a. When simplifying the merges, if any of the parents is the same as the
     commit, we say TREESAME (the same as before);

 2b. When not simplifying, we say TREESAME only when all the parents are
     the same as the commit.  Otherwise the merge commit itself is worth
     showing, i.e. !TREESAME.

But I probably am missing some corner cases you saw in your analysis...

diff --git a/revision.c b/revision.c
index 7b9eaef..0147124 100644
--- a/revision.c
+++ b/revision.c
@@ -439,7 +439,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
 		}
 		die("bad tree compare for commit %s", sha1_to_hex(commit->object.sha1));
 	}
-	if (tree_changed && !tree_same)
+	if ((!revs->simplify_history && tree_changed) || !tree_same)
 		return;
 	commit->object.flags |= TREESAME;
 }

Re: Can't find the revelant commit with git-log

[René Scharfe]

Am 29.01.2011 01:02, schrieb Junio C Hamano:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> Subject: pickaxe: don't simplify history too much
>>
>> If pickaxe is used, turn off history simplification and make sure to keep
>> merges with at least one interesting parent.
>>
>> If path specs are used, merges that have at least one parent whose files
>> match those in the specified subset are edited out.  This is good in
>> general, but leads to unexpectedly few results if used together with
>> pickaxe.  Merges that also have an interesting parent (in terms of -S or
>> -G) are dropped, too.
>>
>> This change makes sure pickaxe takes precedence over history
>> simplification.
> 
> Hmmm, I understand the _motivation_ behind the change in the second hunk,
> in that you _might_ want to dig the side branch that did not contribute
> anything to the end result when looking for a needle with either -S or -G,
> but doesn't the same logic apply to things like --grep?

Yes, that's true.  I have to admit that I'm mostly reacting to the
unintuitive output given in the specific case ("test driven") and
probably don't fully understand the underlying problem and all its
implications.

> I do not think it is a good idea to unconditionally disable simplification
> for -S/G without a way for the user to countermand (even though I could be
> persuaded to say that the flipping the default for -S/-G/--grep might have
> been a better alternative in hindsight).

Currently there is no way to turn simplification off, resulting in
certain commits to become invisible when using e.g. -S in combination
with path specs.

> The user can control this behaviour by giving or not giving --simplify
> from the command line anyway, no?

Yes, but that goes only so far (see the examples in the parent post
which use --full-history; --simplify-merges gives 3 more results with
-m, but still not the full 160).

And as a user I don't want to have to add another option in order to use
pickaxe with path specs.  My expectation is that my search has
precedence over any history simplification, which is a nice and
necessary optimization, but shouldn't hide any search results that I
would have got if I had used no path specs.

However, it definitely looks like a corner case and I still don't know
what happened with all these merges.

> As to the first hunk, I have no idea why this is a good change.

I didn't see any other way to fix the example given in the commit message..

> It feels as if you are fighting against what this part of the code does in
> try_to_simplify_commit():
> 
> 	switch (rev_compare_tree(revs, p, commit)) {
> 	case REV_TREE_SAME:
> 		tree_same = 1;
> 		if (!revs->simplify_history || (p->object.flags & UNINTERESTING)) {
> 			/* ... */
> 			pp = &parent->next;
> 			continue;
> 		}
> 		parent->next = NULL;
> 		commit->parents = parent;
> 		commit->object.flags |= TREESAME;
> 		return;
> 	...
> 
> When we see a _single_ parent that has the same tree, we set tree_same and
> also cull the parent list to contain only that parent.  When we are not
> simplifying the history, we do not cull the parent list and will inspect
> other parents as well, but still we set tree_same to 1 here.  When some
> other parent is found to be different, we set tree_changed to 1.  So we
> have four states (same = (0, 1) x changed = (0, 1)).
> 
> The code before your addition in the first hunk says that we keep the
> commit if there is no parent with the same contents (i.e. !tree_same) and
> there is at least one parent with different contents (i.e. tree_changed).
> I suspect that this logic may not be working well when we do not simplify
> the merge.
> 
> Let's look at the original code before your patch again.
> 
>  1. If all the parents of a commit are the same, we will see (tree_same &&
>     !tree_changed), so we get TREESAME.
> 
>  2. If some but not all of the parents are the same, we will see (tree_same
>     && tree_changed), and we end up getting TREESAME.
> 
>  3. If none of the parents is the same, (!tree_same && tree_changed) holds
>     true, and we do not get TREESAME.

For completeness, a fourth case (!tree_same && !tree_changed), which
would be triggered by commits whose parents are all classified as
REV_TREE_NEW.  That's another corner case for sure, but the old code
would mark it TREESAME and your patch changes that.

> Perhaps the second condition needs to be tweaked for the "do not simplify
> merges" case?  That is, we split 2. into two cases:
> 
>  2a. When simplifying the merges, if any of the parents is the same as the
>      commit, we say TREESAME (the same as before);
> 
>  2b. When not simplifying, we say TREESAME only when all the parents are
>      the same as the commit.  Otherwise the merge commit itself is worth
>      showing, i.e. !TREESAME.
> 
> But I probably am missing some corner cases you saw in your analysis...
> 
> diff --git a/revision.c b/revision.c
> index 7b9eaef..0147124 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -439,7 +439,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
>  		}
>  		die("bad tree compare for commit %s", sha1_to_hex(commit->object.sha1));
>  	}
> -	if (tree_changed && !tree_same)
> +	if ((!revs->simplify_history && tree_changed) || !tree_same)
>  		return;
>  	commit->object.flags |= TREESAME;
>  }

The patch lists the right commits in the test case, but requires the
option --full-history to be given.  Without it no output is given if the
full file name is specified, as in master.

Perhaps we should check my underlying assumption first: is it reasonable
to expect a git log command to show the same commits with and without a
path spec that covers all changed files?

René

Re: Can't find the revelant commit with git-log

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Perhaps we should check my underlying assumption first: is it reasonable
> to expect a git log command to show the same commits with and without a
> path spec that covers all changed files?

The simplest case would be "git log ." vs "git log" from the root level of
the repository, right?  Traditionally, the former is "please show _one_
simplest history that can explain how the current commit came to be"
(i.e. with merge simplification), while the latter is "please list
everything that is behind the current commit" (i.e. without), I think.

It feels unintuitive, but my understanding of the rationale behind the
design is that, the expectation Linus had when he first did the pathspec
limited traversal was that most of the time "git log $path" is used to get
an explanation.  It follows that having to say "git log --simplify $path"
would have been a nuisance, so "with pathspec, we simplify" was thought to
be a reasonable default.

Re: Can't find the revelant commit with git-log

[Francis Moreau]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Am 26.01.2011 19:11, schrieb René Scharfe:
>> - Make git grep report non-matching path specs (new feature).
>
> This is a bit complicated because grep can work on files, index entries
> as well as versioned objects and supports wildcards,
> so it's not that easy to tell if a path spec matches something or is a
> rather typo.  But it's not impossible either, of course.

I don't understand this for the following use case:

   $ cd ~/linux-2.6/drivers/pci/
   $ git grep blacklist v2.6.27 -- drivers/pci/intel-iommu.c

From what you said, it sounds that git grep is actually searching the
string 'somewhere'. But where ?

Thanks
-- 
Francis

Re: Can't find the revelant commit with git-log

[René Scharfe]

Am 29.01.2011 13:52, schrieb Francis Moreau:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> Am 26.01.2011 19:11, schrieb René Scharfe:
>>> - Make git grep report non-matching path specs (new feature).
>>
>> This is a bit complicated because grep can work on files, index entries
>> as well as versioned objects and supports wildcards,
>> so it's not that easy to tell if a path spec matches something or is a
>> rather typo.  But it's not impossible either, of course.
> 
> I don't understand this for the following use case:
> 
>    $ cd ~/linux-2.6/drivers/pci/
>    $ git grep blacklist v2.6.27 -- drivers/pci/intel-iommu.c
> 
> From what you said, it sounds that git grep is actually searching the
> string 'somewhere'. But where ?

All files in the directory are looked at and checked if they match the
given path spec first.  Since none of them do, no actual text search has
to take place.

René

Re: Can't find the revelant commit with git-log

[Francis Moreau]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Am 29.01.2011 13:52, schrieb Francis Moreau:
>> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>> 
>>> Am 26.01.2011 19:11, schrieb René Scharfe:
>
>>>> - Make git grep report non-matching path specs (new feature).
>>>
>>> This is a bit complicated because grep can work on files, index entries
>>> as well as versioned objects and supports wildcards,
>>> so it's not that easy to tell if a path spec matches something or is a
>>> rather typo.  But it's not impossible either, of course.
>> 
>> I don't understand this for the following use case:
>> 
>>    $ cd ~/linux-2.6/drivers/pci/
>>    $ git grep blacklist v2.6.27 -- drivers/pci/intel-iommu.c
>> 
>> From what you said, it sounds that git grep is actually searching the
>> string 'somewhere'. But where ?
>
> All files in the directory are looked at and checked if they match the
> given path spec first.  Since none of them do, no actual text search has
> to take place.

and in this case, it is complicated to tell that the given path spec
match nothing. right ?

-- 
Francis

Re: Can't find the revelant commit with git-log

[René Scharfe]

Am 29.01.2011 14:57, schrieb Francis Moreau:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> Am 29.01.2011 13:52, schrieb Francis Moreau:
>>> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>>>
>>>> Am 26.01.2011 19:11, schrieb René Scharfe:
>>
>>>>> - Make git grep report non-matching path specs (new feature).
>>>>
>>>> This is a bit complicated because grep can work on files, index entries
>>>> as well as versioned objects and supports wildcards,
>>>> so it's not that easy to tell if a path spec matches something or is a
>>>> rather typo.  But it's not impossible either, of course.
>>>
>>> I don't understand this for the following use case:
>>>
>>>    $ cd ~/linux-2.6/drivers/pci/
>>>    $ git grep blacklist v2.6.27 -- drivers/pci/intel-iommu.c
>>>
>>> From what you said, it sounds that git grep is actually searching the
>>> string 'somewhere'. But where ?
>>
>> All files in the directory are looked at and checked if they match the
>> given path spec first.  Since none of them do, no actual text search has
>> to take place.
> 
> and in this case, it is complicated to tell that the given path spec
> match nothing. right ?

The specific case is easy to handle, but a complete solution would have
to address files, index entries as well as versioned objects and support
wildcards.  Presumably it would come on top of Duy's pathspec work
that's in next now.

René

Re: Can't find the revelant commit with git-log

[René Scharfe]

Am 29.01.2011 06:47, schrieb Junio C Hamano:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> Perhaps we should check my underlying assumption first: is it
>> reasonable to expect a git log command to show the same commits
>> with and without a path spec that covers all changed files?
> 
> The simplest case would be "git log ." vs "git log" from the root
> level of the repository, right?  Traditionally, the former is "please
> show _one_ simplest history that can explain how the current commit
> came to be" (i.e. with merge simplification), while the latter is
> "please list everything that is behind the current commit" (i.e.
> without), I think.
> 
> It feels unintuitive, but my understanding of the rationale behind
> the design is that, the expectation Linus had when he first did the
> pathspec limited traversal was that most of the time "git log $path"
> is used to get an explanation.  It follows that having to say "git
> log --simplify $path" would have been a nuisance, so "with pathspec,
> we simplify" was thought to be a reasonable default.

I think simplifying the history whenever a pathspec restricts the set of
interesting commits makes sense.

I'm not so sure about "." vs. none, and it feels odd that the only way
to turn off simplification is to not use pathspecs, as --full-history
will still remove merges if tree_same in revision.c is true.
Simplification by default (even without a pathspec) and --full-history
reporting, well, the full history seems more intuitive to me.

So currently pickaxe can't be used reliable to search for strings that
have been removed: either one has to refrain from using pathspecs, which
is prohibitively slow in the kernel repo, or git is free to take a
short-cut through a branch of history that never contained the string at
all.

Your patch extends --full-history coverage sufficiently to make pickaxe
work in Francis' use case.  One just has to remember to specify this
option if one hunts for removed strings using -S.  Below is an equivalent
patch, only with the simplify_history test moved into the loop and the
needed test script modification.

-- >8 --
Subject: revision: let --full-history keep half-interesting merges

Don't simplify merges away that have at least one parent with changes in
the interesting subset of files if --full-history has been specified.
The previous behaviour hid merges with one uninteresting parent, which
could lead to history that removed code to become undetectable.

E.g., this command run against the Linux kernel repo only found one
merge that brought the specified function in (and the regular commit
which added it in the first place), but missed the 92 merges that
removed it, as well as 67 other merges that added it back:

	$ git log -m --full-history -Sblacklist_iommu \
		v2.6.26..v2.6.29 -- drivers/pci/intel-iommu.c

Reported-by: Francis Moreau <francis.moro@gmail.com>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 revision.c                                 |    2 ++
 t/t6016-rev-list-graph-simplify-history.sh |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/revision.c b/revision.c
index 7b9eaef..84c231b 100644
--- a/revision.c
+++ b/revision.c
@@ -434,6 +434,8 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
 		case REV_TREE_OLD:
 		case REV_TREE_DIFFERENT:
 			tree_changed = 1;
+			if (!revs->simplify_history)
+				return;
 			pp = &parent->next;
 			continue;
 		}
diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh
index f7181d1..50ffcf4 100755
--- a/t/t6016-rev-list-graph-simplify-history.sh
+++ b/t/t6016-rev-list-graph-simplify-history.sh
@@ -168,6 +168,7 @@ test_expect_success '--graph --full-history --simplify-merges -- bar.txt' '
 	echo "|\\  " >> expected &&
 	echo "| * $C4" >> expected &&
 	echo "* | $A5" >> expected &&
+	echo "* | $A4" >> expected &&
 	echo "* | $A3" >> expected &&
 	echo "|/  " >> expected &&
 	echo "* $A2" >> expected &&
-- 
1.7.3.4

Re: Can't find the revelant commit with git-log

[René Scharfe]

Am 29.01.2011 03:34, schrieb René Scharfe:
> Am 29.01.2011 01:02, schrieb Junio C Hamano:
>> Let's look at the original code before your patch again.
>>
>>  1. If all the parents of a commit are the same, we will see (tree_same &&
>>     !tree_changed), so we get TREESAME.
>>
>>  2. If some but not all of the parents are the same, we will see (tree_same
>>     && tree_changed), and we end up getting TREESAME.
>>
>>  3. If none of the parents is the same, (!tree_same && tree_changed) holds
>>     true, and we do not get TREESAME.
> 
> For completeness, a fourth case (!tree_same && !tree_changed), which
> would be triggered by commits whose parents are all classified as
> REV_TREE_NEW.  That's another corner case for sure, but the old code
> would mark it TREESAME and your patch changes that.

Ugh, forget this part, I failed to notice the /* fallthrough */ at the
end of the REV_TREE_NEW case..

René

Re: Can't find the revelant commit with git-log

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Subject: revision: let --full-history keep half-interesting merges
>
> Don't simplify merges away that have at least one parent with changes in
> the interesting subset of files if --full-history has been specified.
> The previous behaviour hid merges with one uninteresting parent, which
> could lead to history that removed code to become undetectable.

I think this patch makes a lot more sense than what I've seen in this
thread (including mine).

> E.g., this command run against the Linux kernel repo only found one
> merge that brought the specified function in (and the regular commit
> which added it in the first place), but missed the 92 merges that
> removed it, as well as 67 other merges that added it back:
>
> 	$ git log -m --full-history -Sblacklist_iommu \
> 		v2.6.26..v2.6.29 -- drivers/pci/intel-iommu.c
>
> Reported-by: Francis Moreau <francis.moro@gmail.com>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>

Thanks.

> ---
>  revision.c                                 |    2 ++
>  t/t6016-rev-list-graph-simplify-history.sh |    1 +
>  2 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 7b9eaef..84c231b 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -434,6 +434,8 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
>  		case REV_TREE_OLD:
>  		case REV_TREE_DIFFERENT:
>  			tree_changed = 1;
> +			if (!revs->simplify_history)
> +				return;
>  			pp = &parent->next;
>  			continue;
>  		}
> diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh
> index f7181d1..50ffcf4 100755
> --- a/t/t6016-rev-list-graph-simplify-history.sh
> +++ b/t/t6016-rev-list-graph-simplify-history.sh
> @@ -168,6 +168,7 @@ test_expect_success '--graph --full-history --simplify-merges -- bar.txt' '
>  	echo "|\\  " >> expected &&
>  	echo "| * $C4" >> expected &&
>  	echo "* | $A5" >> expected &&
> +	echo "* | $A4" >> expected &&
>  	echo "* | $A3" >> expected &&
>  	echo "|/  " >> expected &&
>  	echo "* $A2" >> expected &&
> -- 
> 1.7.3.4

Re: Can't find the revelant commit with git-log

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh
> index f7181d1..50ffcf4 100755
> --- a/t/t6016-rev-list-graph-simplify-history.sh
> +++ b/t/t6016-rev-list-graph-simplify-history.sh
> @@ -168,6 +168,7 @@ test_expect_success '--graph --full-history --simplify-merges -- bar.txt' '
>  	echo "|\\  " >> expected &&
>  	echo "| * $C4" >> expected &&
>  	echo "* | $A5" >> expected &&
> +	echo "* | $A4" >> expected &&
>  	echo "* | $A3" >> expected &&
>  	echo "|/  " >> expected &&
>  	echo "* $A2" >> expected &&

Thanks for a patch with a test; I am not sure if this is quite correct,
though.

A4 has three parents, C2, A3 and B2, and does not introduce any change
with respect to bar.txt.  A6 has bar.txt identical to that of A5, but we
cannot omit it because we are showing its two parents (A5 and C4), and
that is why we show it.  A4 isn't even gets shown as a merge, so I don't
understand why we need to show it?

Don't we need to also adjust the simplify-merges codepath to take this
change into account, or something?

Re: Can't find the revelant commit with git-log

[René Scharfe]

Am 07.02.2011 23:51, schrieb Junio C Hamano:
> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:
>
>> diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh
>> index f7181d1..50ffcf4 100755
>> --- a/t/t6016-rev-list-graph-simplify-history.sh
>> +++ b/t/t6016-rev-list-graph-simplify-history.sh
>> @@ -168,6 +168,7 @@ test_expect_success '--graph --full-history --simplify-merges -- bar.txt' '
>>   	echo "|\\  ">>  expected&&
>>   	echo "| * $C4">>  expected&&
>>   	echo "* | $A5">>  expected&&
>> +	echo "* | $A4">>  expected&&
>>   	echo "* | $A3">>  expected&&
>>   	echo "|/  ">>  expected&&
>>   	echo "* $A2">>  expected&&
>
> Thanks for a patch with a test; I am not sure if this is quite correct,
> though.
>
> A4 has three parents, C2, A3 and B2, and does not introduce any change
> with respect to bar.txt.  A6 has bar.txt identical to that of A5, but we
> cannot omit it because we are showing its two parents (A5 and C4), and
> that is why we show it.  A4 isn't even gets shown as a merge, so I don't
> understand why we need to show it?

Yes, this looks a bit silly on closer look.  I thought that it matches 
Francis' use case, but that's wrong -- having --simplify-merges instead 
of -Sstring makes a difference, obviously.

After looking at the case again, I think I have a simpler solution: no 
code change, just add --sparse (include all walked commits).  This gives 
the same results as the patched version:

	$ git log --oneline -m --sparse --full-history \
		-Sblacklist_iommu v2.6.26..v2.6.29 -- \
		drivers/pci/intel-iommu.c | wc -l
	    160

Sorry for the noise.

So, the lesson would be: If you want to find commits that removed a 
certain string in a certain set of files, add --full-history, -m and 
--sparse to your "git log -Sstring -- files" command.  This allows you 
to catch merges that reverted those files to a state before the string 
was introduced in the first place, otherwise history simplification can 
hide them.

I'm not sure if there's a way to make the flags and their interactions 
more intituitive.

René