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

René Scharfe [off-list ref] 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.

Am 29.01.2011 06:47, schrieb Junio C Hamano:

René Scharfe [off-list ref] writes:

quoted

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 <redacted>
Signed-off-by: Rene Scharfe <redacted>
---
 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

René Scharfe [off-list ref] 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 <redacted>
Signed-off-by: Rene Scharfe <redacted>

Thanks.