Am 29.01.2011 06:47, schrieb Junio C Hamano: > Renà Scharfe <rene.scharfe@xxxxxxxxxxxxxx> 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@xxxxxxxxx> Signed-off-by: Rene Scharfe <rene.scharfe@xxxxxxxxxxxxxx> --- 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html