* [PATCH 3/3] diffstat generation in hooks--update was passing "^baserev" to git-diff-tree
@ 2007-02-13 14:24 Andy Parkins
2007-02-13 15:37 ` Johannes Sixt
2007-02-13 17:03 ` Linus Torvalds
0 siblings, 2 replies; 6+ messages in thread
From: Andy Parkins @ 2007-02-13 14:24 UTC (permalink / raw)
To: git
For generating the diffstat after a branch update, git-diff-tree is
simply comparing the new revision with the base revision. However, the
baserev was being passed with a "^" operator proceeding it - this (I
think) made git-diff-tree think that a path was being specified for
comparison rather than a second path, so only a single revision was
being summarised.
Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
templates/hooks--update | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/templates/hooks--update b/templates/hooks--update
index 7e8258a..ee3859c 100644
--- a/templates/hooks--update
+++ b/templates/hooks--update
@@ -193,8 +193,8 @@ case "$refname_type" in
git-rev-parse --not --all | git-rev-list --stdin --pretty $newrev ^$baserev
echo $LOGEND
echo ""
- echo "Diffstat:"
- git-diff-tree --no-color --stat -M -C --find-copies-harder $newrev ^$baserev
+ echo "Diffstat against $baserev:"
+ git-diff-tree --no-color --stat -M -C --find-copies-harder $newrev $baserev
fi
;;
"annotated tag")
--
1.5.0.rc4.364.g85b1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] diffstat generation in hooks--update was passing "^baserev" to git-diff-tree
2007-02-13 14:24 [PATCH 3/3] diffstat generation in hooks--update was passing "^baserev" to git-diff-tree Andy Parkins
@ 2007-02-13 15:37 ` Johannes Sixt
2007-02-13 16:32 ` Andy Parkins
2007-02-13 17:03 ` Linus Torvalds
1 sibling, 1 reply; 6+ messages in thread
From: Johannes Sixt @ 2007-02-13 15:37 UTC (permalink / raw)
To: git
Andy Parkins wrote:
> - echo "Diffstat:"
> - git-diff-tree --no-color --stat -M -C --find-copies-harder $newrev ^$baserev
> + echo "Diffstat against $baserev:"
> + git-diff-tree --no-color --stat -M -C --find-copies-harder $newrev $baserev
Shouldn't that be "... $baserev $newrev"? But read on.
I cannot entirely follow the reasoning of the comment above this part,
which says:
# Now the problem is for cases like this:
# * --- * --- * --- * (oldrev)
# \
# * --- * --- * (newrev)
# i.e. there is no guarantee that newrev is a strict subset
# of oldrev - (would have required a force, but that's allowed).
# So, we can't simply say rev-list $oldrev..$newrev. Instead
# we find the common base of the two revs and list from there
git rev-list $oldrev..$newrev
is exactly what you want in this case. The stunt with $baserev is not
necessary, and it may even be wrong if there is more than one
merge-base. $oldrev..$newrev will be correct even in this case.
You still need to derive a merge-base, but only to detect the forced
update and to format the message. Then you should use --not $baserev
instead of ^$baserev just in case there is more than one merge-base.
-- Hannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] diffstat generation in hooks--update was passing "^baserev" to git-diff-tree
2007-02-13 15:37 ` Johannes Sixt
@ 2007-02-13 16:32 ` Andy Parkins
2007-02-13 17:16 ` Johannes Sixt
0 siblings, 1 reply; 6+ messages in thread
From: Andy Parkins @ 2007-02-13 16:32 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt
On Tuesday 2007 February 13 15:37, Johannes Sixt wrote:
> Shouldn't that be "... $baserev $newrev"? But read on.
I think it should, yes. Thanks.
> # * --- * --- * --- * (oldrev)
> # \
> # * --- * --- * (newrev)
>
> git rev-list $oldrev..$newrev
>
> is exactly what you want in this case. The stunt with $baserev is not
> necessary, and it may even be wrong if there is more than one
> merge-base. $oldrev..$newrev will be correct even in this case.
I just pinched the idea from the old update hook by Junio in 4f11b84c84. I
just assumed that was the better way to do it. A quick test shows it to give
the same output - I'm easy.
> You still need to derive a merge-base, but only to detect the forced
> update and to format the message. Then you should use --not $baserev
> instead of ^$baserev just in case there is more than one merge-base.
Are you suggesting something like this?
git-rev-parse --not --all $baserev | git-rev-list --stdin --pretty $newrev
Which would start showing from $newrev but would exclude all baserevs and all
existing branches.
Andy
--
Dr Andy Parkins, M Eng (hons), MIEE
andyparkins@gmail.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] diffstat generation in hooks--update was passing "^baserev" to git-diff-tree
2007-02-13 14:24 [PATCH 3/3] diffstat generation in hooks--update was passing "^baserev" to git-diff-tree Andy Parkins
2007-02-13 15:37 ` Johannes Sixt
@ 2007-02-13 17:03 ` Linus Torvalds
2007-02-13 18:34 ` Andy Parkins
1 sibling, 1 reply; 6+ messages in thread
From: Linus Torvalds @ 2007-02-13 17:03 UTC (permalink / raw)
To: Andy Parkins; +Cc: git
On Tue, 13 Feb 2007, Andy Parkins wrote:
>
> - echo "Diffstat:"
> - git-diff-tree --no-color --stat -M -C --find-copies-harder $newrev ^$baserev
> + echo "Diffstat against $baserev:"
> + git-diff-tree --no-color --stat -M -C --find-copies-harder $newrev $baserev
This is wrong.
newrev ^baserev
is right. The "not baserev" tells diff-tree that the baserev is the
starting point, so newrev is obviously the target, and thus that will
generate a diff from baserev to newrev.
So will either of
baserev..newrev
baserev newrev
which mean _exactly_ the same thing as "newrev ^basrev" to "diff-tree",
because in all cases it's obviously "baserev" that is the old one to diff
against.
But
newrev baserev
means the diff from "new" to "base", which is exactly the wrong way
around.
Of course, since we did
baserev=$(git-merge-base $oldrev $newrev)
to generate base-rev, we could actually have done
$oldrev...$newrev
(note the _three_ dots) which means "diff from merge-base to newrev". But
since we use "baserev" multiple times, what the update hook does right now
is actually better - it avoids the cost of re-computing the merge base
that we needed for other things anyway.
Linus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] diffstat generation in hooks--update was passing "^baserev" to git-diff-tree
2007-02-13 16:32 ` Andy Parkins
@ 2007-02-13 17:16 ` Johannes Sixt
0 siblings, 0 replies; 6+ messages in thread
From: Johannes Sixt @ 2007-02-13 17:16 UTC (permalink / raw)
To: Andy Parkins; +Cc: git
Andy Parkins wrote:
> > You still need to derive a merge-base, but only to detect the forced
> > update and to format the message. Then you should use --not $baserev
> > instead of ^$baserev just in case there is more than one merge-base.
>
> Are you suggesting something like this?
>
> git-rev-parse --not --all $baserev | git-rev-list --stdin --pretty $newrev
>
> Which would start showing from $newrev but would exclude all baserevs and all
> existing branches.
You are using ^$baserev in two instances.
(1) in a for loop which obviously is intended as a table of contents
(revs plus rev-type (can this be anything else than 'commit'?)). Here
you should use
git-rev-list $newrev --not $baserev
(2) later to print the log messages for all new revs. Here you use just
this:
git-rev-list --pretty $oldrev..$newrev
Then the diffstat:
git-diff-tree --no-color --stat -M -C --find-copies-harder \
$oldrev $newrev
This reflects the modifications of the revisions that have just been
listed *only* in the fast-forward case. But even in the forced update
case it tells how the old tree transformed into the new tree, and for
this reason nothing more complicated is needed, IMO.
-- Hannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] diffstat generation in hooks--update was passing "^baserev" to git-diff-tree
2007-02-13 17:03 ` Linus Torvalds
@ 2007-02-13 18:34 ` Andy Parkins
0 siblings, 0 replies; 6+ messages in thread
From: Andy Parkins @ 2007-02-13 18:34 UTC (permalink / raw)
To: git; +Cc: Linus Torvalds
On Tuesday 2007, February 13, Linus Torvalds wrote:
> This is wrong.
>
> newrev ^baserev
>
> is right. The "not baserev" tells diff-tree that the baserev is the
Right. It's all come back to me now you've explained it; I obviously
understood it when I wrote it, but it leaked out in the interim :-). I
was surprised because I've been using the hook for ages and the output
seemed right. :-)
Junio - please ignore this patch.
Andy
--
Dr Andrew Parkins, M Eng (Hons), AMIEE
andyparkins@gmail.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-02-13 18:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-02-13 14:24 [PATCH 3/3] diffstat generation in hooks--update was passing "^baserev" to git-diff-tree Andy Parkins
2007-02-13 15:37 ` Johannes Sixt
2007-02-13 16:32 ` Andy Parkins
2007-02-13 17:16 ` Johannes Sixt
2007-02-13 17:03 ` Linus Torvalds
2007-02-13 18:34 ` Andy Parkins
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.