patch, 3 partsdiffstat generation in hooks--update was passing "^baserev" to git-diff-tree

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(-)

Andy Parkins wrote:

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

On Tuesday 2007 February 13 15:37, Johannes Sixt wrote:

I think it should, yes.  Thanks.

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.

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

Andy Parkins wrote:

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

On Tue, 13 Feb 2007, Andy Parkins wrote:

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

On Tuesday 2007, February 13, Linus Torvalds wrote:

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