{"thread":{"id":"6799","subject":"[PATCH 3/3] diffstat generation in hooks--update was passing \"^baserev\" to git-diff-tree","startedAt":"2007-02-13T14:24:21Z","lastAt":"2007-02-13T18:34:34Z","messageCount":6,"participants":["Andy Parkins","Johannes Sixt","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"34406","messageId":"200702131424.21665.andyparkins@gmail.com","threadId":"6799","inReplyTo":null,"subject":"[PATCH 3/3] diffstat generation in hooks--update was passing \"^baserev\" to git-diff-tree","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-02-13T14:24:21Z","receivedAt":"2007-02-13T14:24:21Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"For generating the diffstat after a branch update, git-diff-tree is\nsimply comparing the new revision with the base revision.  However, the\nbaserev was being passed with a \"^\" operator proceeding it - this (I\nthink) made git-diff-tree think that a path was being specified for\ncomparison rather than a second path, so only a single revision was\nbeing summarised.\n\nSigned-off-by: Andy Parkins <andyparkins@gmail.com>\n---\n templates/hooks--update |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/templates/hooks--update b/templates/hooks--update\nindex 7e8258a..ee3859c 100644\n--- a/templates/hooks--update\n+++ b/templates/hooks--update\n@@ -193,8 +193,8 @@ case \"$refname_type\" in\n \t\t\tgit-rev-parse --not --all | git-rev-list --stdin --pretty $newrev ^$baserev\n \t\t\techo $LOGEND\n \t\t\techo \"\"\n-\t\t\techo \"Diffstat:\"\n-\t\t\tgit-diff-tree --no-color --stat -M -C --find-copies-harder $newrev ^$baserev\n+\t\t\techo \"Diffstat against $baserev:\"\n+\t\t\tgit-diff-tree --no-color --stat -M -C --find-copies-harder $newrev $baserev\n \t\tfi\n \t\t;;\n \t\"annotated tag\")\n-- \n1.5.0.rc4.364.g85b1\n"},{"id":"34416","messageId":"45D1DB57.65433549@eudaptics.com","threadId":"6799","inReplyTo":"200702131424.21665.andyparkins@gmail.com","subject":"Re: [PATCH 3/3] diffstat generation in hooks--update was passing \"^baserev\" to git-diff-tree","fromName":"Johannes Sixt","fromEmail":"j.sixt@eudaptics.com","sentAt":"2007-02-13T15:37:59Z","receivedAt":"2007-02-13T15:37:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Andy Parkins wrote:\n> -                       echo \"Diffstat:\"\n> -                       git-diff-tree --no-color --stat -M -C --find-copies-harder $newrev ^$baserev\n> +                       echo \"Diffstat against $baserev:\"\n> +                       git-diff-tree --no-color --stat -M -C --find-copies-harder $newrev $baserev\n\nShouldn't that be \"... $baserev $newrev\"? But read on.\n\nI cannot entirely follow the reasoning of the comment above this part,\nwhich says:\n\n# Now the problem is for cases like this:\n#   * --- * --- * --- * (oldrev)\n#          \\\n#           * --- * --- * (newrev)\n# i.e. there is no guarantee that newrev is a strict subset\n# of oldrev - (would have required a force, but that's allowed).\n# So, we can't simply say rev-list $oldrev..$newrev.  Instead\n# we find the common base of the two revs and list from there\n\ngit rev-list $oldrev..$newrev\n\nis exactly what you want in this case. The stunt with $baserev is not\nnecessary, and it may even be wrong if there is more than one\nmerge-base. $oldrev..$newrev will be correct even in this case.\n\nYou still need to derive a merge-base, but only to detect the forced\nupdate and to format the message. Then you should use --not $baserev\ninstead of ^$baserev just in case there is more than one merge-base.\n\n-- Hannes\n"},{"id":"34420","messageId":"200702131632.37401.andyparkins@gmail.com","threadId":"6799","inReplyTo":"45D1DB57.65433549@eudaptics.com","subject":"Re: [PATCH 3/3] diffstat generation in hooks--update was passing \"^baserev\" to git-diff-tree","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-02-13T16:32:35Z","receivedAt":"2007-02-13T16:32:35Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Tuesday 2007 February 13 15:37, Johannes Sixt wrote:\n\n> Shouldn't that be \"... $baserev $newrev\"? But read on.\n\nI think it should, yes.  Thanks.\n\n> #   * --- * --- * --- * (oldrev)\n> #          \\\n> #           * --- * --- * (newrev)\n>\n> git rev-list $oldrev..$newrev\n>\n> is exactly what you want in this case. The stunt with $baserev is not\n> necessary, and it may even be wrong if there is more than one\n> merge-base. $oldrev..$newrev will be correct even in this case.\n\nI just pinched the idea from the old update hook by Junio in 4f11b84c84.  I \njust assumed that was the better way to do it.  A quick test shows it to give \nthe same output - I'm easy.\n\n> You still need to derive a merge-base, but only to detect the forced\n> update and to format the message. Then you should use --not $baserev\n> instead of ^$baserev just in case there is more than one merge-base.\n\nAre you suggesting something like this?\n\ngit-rev-parse --not --all $baserev | git-rev-list --stdin --pretty $newrev\n\nWhich would start showing from $newrev but would exclude all baserevs and all \nexisting branches.\n\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIEE\nandyparkins@gmail.com\n"},{"id":"34425","messageId":"Pine.LNX.4.64.0702130856580.8424@woody.linux-foundation.org","threadId":"6799","inReplyTo":"200702131424.21665.andyparkins@gmail.com","subject":"Re: [PATCH 3/3] diffstat generation in hooks--update was passing \"^baserev\" to git-diff-tree","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-02-13T17:03:44Z","receivedAt":"2007-02-13T17:03:44Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 13 Feb 2007, Andy Parkins wrote:\n>\n> -\t\t\techo \"Diffstat:\"\n> -\t\t\tgit-diff-tree --no-color --stat -M -C --find-copies-harder $newrev ^$baserev\n> +\t\t\techo \"Diffstat against $baserev:\"\n> +\t\t\tgit-diff-tree --no-color --stat -M -C --find-copies-harder $newrev $baserev\n\nThis is wrong.\n\n\tnewrev ^baserev\n\nis right. The \"not baserev\" tells diff-tree that the baserev is the \nstarting point, so newrev is obviously the target, and thus that will \ngenerate a diff from baserev to newrev.\n\nSo will either of\n\n\tbaserev..newrev\n\tbaserev newrev\n\nwhich mean _exactly_ the same thing as \"newrev ^basrev\" to \"diff-tree\", \nbecause in all cases it's obviously \"baserev\" that is the old one to diff \nagainst.\n\nBut\n\n\tnewrev baserev\n\nmeans the diff from \"new\" to \"base\", which is exactly the wrong way \naround.\n\nOf course, since we did\n\n\tbaserev=$(git-merge-base $oldrev $newrev)\n\nto generate base-rev, we could actually have done\n\n\t$oldrev...$newrev\n\n(note the _three_ dots) which means \"diff from merge-base to newrev\". But \nsince we use \"baserev\" multiple times, what the update hook does right now \nis actually better - it avoids the cost of re-computing the merge base \nthat we needed for other things anyway.\n\n\t\tLinus\n"},{"id":"34426","messageId":"45D1F26A.28FEADD8@eudaptics.com","threadId":"6799","inReplyTo":"200702131632.37401.andyparkins@gmail.com","subject":"Re: [PATCH 3/3] diffstat generation in hooks--update was passing \"^baserev\" to git-diff-tree","fromName":"Johannes Sixt","fromEmail":"j.sixt@eudaptics.com","sentAt":"2007-02-13T17:16:26Z","receivedAt":"2007-02-13T17:16:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Andy Parkins wrote:\n> > You still need to derive a merge-base, but only to detect the forced\n> > update and to format the message. Then you should use --not $baserev\n> > instead of ^$baserev just in case there is more than one merge-base.\n> \n> Are you suggesting something like this?\n> \n> git-rev-parse --not --all $baserev | git-rev-list --stdin --pretty $newrev\n> \n> Which would start showing from $newrev but would exclude all baserevs and all\n> existing branches.\n\nYou are using ^$baserev in two instances.\n\n(1) in a for loop which obviously is intended as a table of contents\n(revs plus rev-type (can this be anything else than 'commit'?)). Here\nyou should use\n\ngit-rev-list $newrev --not $baserev\n\n(2) later to print the log messages for all new revs. Here you use just\nthis:\n\ngit-rev-list --pretty $oldrev..$newrev\n\n\nThen the diffstat:\n\ngit-diff-tree --no-color --stat -M -C --find-copies-harder \\\n  $oldrev $newrev\n\nThis reflects the modifications of the revisions that have just been\nlisted *only* in the fast-forward case. But even in the forced update\ncase it tells how the old tree transformed into the new tree, and for\nthis reason nothing more complicated is needed, IMO.\n\n-- Hannes\n"},{"id":"34458","messageId":"200702131834.36001.andyparkins@gmail.com","threadId":"6799","inReplyTo":"Pine.LNX.4.64.0702130856580.8424@woody.linux-foundation.org","subject":"Re: [PATCH 3/3] diffstat generation in hooks--update was passing \"^baserev\" to git-diff-tree","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-02-13T18:34:34Z","receivedAt":"2007-02-13T18:34:34Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Tuesday 2007, February 13, Linus Torvalds wrote:\n\n> This is wrong.\n>\n> \tnewrev ^baserev\n>\n> is right. The \"not baserev\" tells diff-tree that the baserev is the\n\nRight.  It's all come back to me now you've explained it; I obviously \nunderstood it when I wrote it, but it leaked out in the interim :-).  I \nwas surprised because I've been using the hook for ages and the output \nseemed right. :-)\n\nJunio - please ignore this patch.\n\n\nAndy\n-- \nDr Andrew Parkins, M Eng (Hons), AMIEE\nandyparkins@gmail.com\n"}]}