[PATCH] Fix typo on variable name $newref should be $newrev in sample update hook

[PATCH] Fix typo on variable name $newref should be $newrev in sample update hook

[Andy Parkins]

The log message would be empty for new branches because git-rev-list was
being passed $newref instead of the correct variable $newrev.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 templates/hooks--update |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/templates/hooks--update b/templates/hooks--update
index fd1f73d..2a707e5 100644
--- a/templates/hooks--update
+++ b/templates/hooks--update
@@ -148,7 +148,7 @@ case "$refname_type" in
 			# This shows all log entries that are not already covered by
 			# another ref - i.e. commits that are now accessible from this
 			# ref that were previously not accessible
-			git-rev-parse --not --all | git-rev-list --stdin --pretty $newref
+			git-rev-parse --not --all | git-rev-list --stdin --pretty $newrev
 			echo $LOGEND
 		else
 			# oldrev is valid
-- 
1.5.0.2.205.g74e20

Re: [PATCH] Fix typo on variable name $newref should be $newrev in sample update hook

[Linus Torvalds]

On Fri, 2 Mar 2007, Andy Parkins wrote:
>  			# ref that were previously not accessible
> -			git-rev-parse --not --all | git-rev-list --stdin --pretty $newref
> +			git-rev-parse --not --all | git-rev-list --stdin --pretty $newrev

Well, it should just avoid git-rev-parse these days and do

	git rev-list --pretty $newrev --not --all

instead. And quite frankly, rather than "git rev-list --pretty", there's 
no real reason not to do

	git log $newrev --not --all

unless there is some subtle issue with pagers (but none of this goes to a 
terminal?) in which case you'd have to disable that (and then rev-list is 
probably simpler and more readable than adding a "GIT_PAGER=" to the 
front).

		Linus

Re: [PATCH] Fix typo on variable name $newref should be $newrev in sample update hook

[Andy Parkins]

On Friday 2007, March 02, Linus Torvalds wrote:

> Well, it should just avoid git-rev-parse these days and do
>
> 	git rev-list --pretty $newrev --not --all

Ah - I didn't know that was possible.  I thought that the --not switch 
would invert all the arguments not just those following it.

Patch to follow.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

[PATCH] Remove use of git-rev-parse and replace git-rev-list --pretty with git-log

[Andy Parkins]

Linus noticed:

--- >8 ---
it should just avoid git-rev-parse these days and do

  git rev-list --pretty $newrev --not --all

instead. And quite frankly, rather than "git rev-list --pretty", there's
no real reason not to do

  git log $newrev --not --all
--- 8< ---

This patch makes those changes.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 templates/hooks--update |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/templates/hooks--update b/templates/hooks--update
index 2a707e5..f580360 100644
--- a/templates/hooks--update
+++ b/templates/hooks--update
@@ -148,7 +148,7 @@ case "$refname_type" in
 			# This shows all log entries that are not already covered by
 			# another ref - i.e. commits that are now accessible from this
 			# ref that were previously not accessible
-			git-rev-parse --not --all | git-rev-list --stdin --pretty $newrev
+			git log $newrev --not --all
 			echo $LOGEND
 		else
 			# oldrev is valid
@@ -165,7 +165,7 @@ case "$refname_type" in
 			baserev=$(git-merge-base $oldrev $newrev)
 
 			# Commit with a parent
-			for rev in $(git-rev-parse --not --all | git-rev-list --stdin $newrev ^$baserev)
+			for rev in $(git-rev-list $newrev ^$baserev --not --all)
 			do
 				revtype=$(git-cat-file -t "$rev")
 				echo "       via  $rev ($revtype)"
@@ -190,8 +190,7 @@ case "$refname_type" in
 			fi
 			echo ""
 			echo $LOGBEGIN
-			git-rev-parse --not --all |
-			git-rev-list --stdin --pretty $newrev ^$baserev
+			git log $newrev ^$baserev --not --all
 			echo $LOGEND
 			echo ""
 			echo "Diffstat:"
-- 
1.5.0.rc4.gb4d2

Re: [PATCH] Remove use of git-rev-parse and replace git-rev-list --pretty with git-log

[Linus Torvalds]

On Fri, 2 Mar 2007, Andy Parkins wrote:
>  			# Commit with a parent
> -			for rev in $(git-rev-parse --not --all | git-rev-list --stdin $newrev ^$baserev)
> +			for rev in $(git-rev-list $newrev ^$baserev --not --all)

Looks fine, and I don't think it's worth changing, but you can avoid the 
extra caret by just moving "$baserev" to after the "--not", ie writing the 
thing as

	git-rev-list $newrev --not $baserev --all

instead if you want to.

> -			git-rev-parse --not --all |
> -			git-rev-list --stdin --pretty $newrev ^$baserev
> +			git log $newrev ^$baserev --not --all

And that's true here too.

		Linus

Re: [PATCH] Remove use of git-rev-parse and replace git-rev-list --pretty with git-log

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> Linus noticed:
>
> --- >8 ---
> it should just avoid git-rev-parse these days and do
>
>   git rev-list --pretty $newrev --not --all
>
> instead. And quite frankly, rather than "git rev-list --pretty", there's
> no real reason not to do
>
>   git log $newrev --not --all
> --- 8< ---
>
> This patch makes those changes.
>
> Signed-off-by: Andy Parkins <andyparkins@gmail.com>

Please look at your Subject: line above and ponder how it would
look in the next issue of "What's cooking in git.git".  Do I
have to spend extra brain cycles to go back to "git log --stat"
and realize that this one only updates a sample hook script?

Also, please don't do "--- >8 ---".  If you want to, please use
two dashes; this is purely for technical reasons.

I'll massage the log message and move "^$base --not" around as
Linus suggested, but next time please be a bit more careful.

Re: [PATCH] Remove use of git-rev-parse and replace git-rev-list --pretty with git-log

[Andy Parkins]

On Friday 2007, March 02, Junio C Hamano wrote:

> Please look at your Subject: line above and ponder how it would
> look in the next issue of "What's cooking in git.git".  Do I
> have to spend extra brain cycles to go back to "git log --stat"
> and realize that this one only updates a sample hook script?

Apologies.  It's easy to drop into focusing narrowly on on your own 
patch and forget about the larger picture.

> Also, please don't do "--- >8 ---".  If you want to, please use
> two dashes; this is purely for technical reasons.

I'm happy to comply of course.  However, this seems like a bug in git to 
me.  This makes it so that some content is not allowed in a log 
message, which seems very much out of keeping with git's normal "I can 
handle anything" stance.

Finding the "---" separator between diff and log message could at least 
rely on finding "---" alone on a line so that "--- something else" 
wouldn't trigger the end of log?

I assume this is too simple?

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 766a37e..4e0795a 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -670,7 +670,7 @@ static int handle_commit_msg(int *seen)
        return 0;
    do {
        if (!memcmp("diff -", line, 6) ||
-           !memcmp("---", line, 3) ||
+           !memcmp("---\n", line, 4) ||
            !memcmp("Index: ", line, 7))
            break;
        if ((multipart_boundary[0] && is_multipart_boundary(line))) {

> I'll massage the log message and move "^$base --not" around as
> Linus suggested, but next time please be a bit more careful.

I will try.  Once again, my apologies.



Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: [PATCH] Remove use of git-rev-parse and replace git-rev-list --pretty with git-log

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> Finding the "---" separator between diff and log message could at least 
> rely on finding "---" alone on a line so that "--- something else" 
> wouldn't trigger the end of log?
>
> I assume this is too simple?
>
> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
> index 766a37e..4e0795a 100644
> --- a/builtin-mailinfo.c
> +++ b/builtin-mailinfo.c
> @@ -670,7 +670,7 @@ static int handle_commit_msg(int *seen)
>         return 0;
>     do {
>         if (!memcmp("diff -", line, 6) ||
> -           !memcmp("---", line, 3) ||
> +           !memcmp("---\n", line, 4) ||
>             !memcmp("Index: ", line, 7))
>             break;
>         if ((multipart_boundary[0] && is_multipart_boundary(line))) {

I think that would make us reject patches that we currently
accept.  Today, you can submit a patch created this way:

	$ cp Makefile Makefile.orig
        $ edit Makefile

	$ cat body-of-message
        $ echo "Signed-off-by: $me"
        $ diff -u Makefile.orig Makefile

The first line of the diff part would be "--- Makefile.orig"
(and the second one begins with "+++ Makefile").

I do not think I've asked Linus about how he originally came up
with "three dashes at the beginning of line" convention in the
kernel circle, but my guess always have been that it would
automatically allow us to accept patches prepared like this.

Re: [PATCH] Remove use of git-rev-parse and replace git-rev-list --pretty with git-log

[Linus Torvalds]

On Sat, 3 Mar 2007, Junio C Hamano wrote:
> 
> I do not think I've asked Linus about how he originally came up
> with "three dashes at the beginning of line" convention in the
> kernel circle, but my guess always have been that it would
> automatically allow us to accept patches prepared like this.

Indeed. The three dashes are just two different usage cases:

 - bare patches. So we stop on 

	--- Makefile.orig
	+++ Makefile

   exactly the same way we stop of "^diff -" and "Index: "

 - that then meant that I also extended it to ask people to add comments 
   after a "---" section.

That said, we could certainly require that it be:

 - either just three dashes on the line
OR
 - three dashes, exactly one whitespace, and what looks like a filename 
   (which in turn can have a date after it, that's what many versions of 
   GNU patch do)

but it won't remove the case where if somebody writes some text message 
that really *looks* like the beginning of a patch, it's what git 
"mailinfo" will use to split the patch from the top of the message. So we 
can tighten it up a bit, but people still need to be aware of this!

		Linus