git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "René Scharfe" <rene.scharfe@lsrfire.ath.cx>
To: Junio C Hamano <gitster@pobox.com>
Cc: Francis Moreau <francis.moro@gmail.com>,
	git@vger.kernel.org, Johannes Sixt <j6t@kdbg.org>
Subject: Re: Can't find the revelant commit with git-log
Date: Sat, 29 Jan 2011 21:26:12 +0100	[thread overview]
Message-ID: <4D4477E4.6020006@lsrfire.ath.cx> (raw)
In-Reply-To: <7vsjwcb6rh.fsf@alter.siamese.dyndns.org>

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

  reply	other threads:[~2011-01-29 20:26 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-01-25  9:01 Can't find the revelant commit with git-log Francis Moreau
2011-01-25 16:12 ` René Scharfe
2011-01-25 17:44   ` Francis Moreau
2011-01-26  8:36     ` Francis Moreau
2011-01-26 10:44       ` Johannes Sixt
2011-01-26 20:56         ` Francis Moreau
2011-01-26 21:03           ` Sverre Rabbelier
2011-01-26 21:08             ` Francis Moreau
2011-01-26 21:14               ` Sverre Rabbelier
2011-01-26 21:31                 ` Francis Moreau
2011-01-26 21:24               ` Junio C Hamano
2011-01-26 21:32                 ` Francis Moreau
2011-01-26 18:11       ` René Scharfe
2011-01-28 20:29         ` René Scharfe
2011-01-29  0:02           ` Junio C Hamano
2011-01-29  2:34             ` René Scharfe
2011-01-29  5:47               ` Junio C Hamano
2011-01-29 20:26                 ` René Scharfe [this message]
2011-02-01 21:28                   ` Junio C Hamano
2011-02-07 22:51                   ` Junio C Hamano
2011-02-10 18:50                     ` René Scharfe
2011-01-29 20:26               ` René Scharfe
2011-01-28 22:01         ` René Scharfe
2011-01-29 12:52           ` Francis Moreau
2011-01-29 13:02             ` René Scharfe
2011-01-29 13:57               ` Francis Moreau
2011-01-29 15:17                 ` René Scharfe
2011-01-26  9:01   ` Francis Moreau
2011-01-26 18:39     ` René Scharfe
2011-01-26 19:50       ` Francis Moreau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4D4477E4.6020006@lsrfire.ath.cx \
    --to=rene.scharfe@lsrfire.ath.cx \
    --cc=francis.moro@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).