threads / patch / 7048

patchFix typo on variable name $newref should be $newrev in sample update hook

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

## tl;dr

9 messages between Mar 2, 2007 and Mar 4, 2007. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Andy Parkins· Mar 2, 2007, 10:27 UTC · lore

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(-)
Show changes to templates/hooks--update +1 −1
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
Linus Torvalds· Mar 2, 2007, 16:44 UTC · re: Andy Parkins · lore

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

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
Andy Parkins· Mar 2, 2007, 19:25 UTC · re: Linus Torvalds · lore

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

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
Andy Parkins· Mar 2, 2007, 19:29 UTC · re: Linus Torvalds · lore

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

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(-)
Show changes to templates/hooks--update +3 −4
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
Linus Torvalds· Mar 2, 2007, 20:48 UTC · re: Andy Parkins · lore

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

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
Junio C Hamano· Mar 2, 2007, 23:10 UTC · re: Andy Parkins · lore

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

Andy Parkins <andyparkins@gmail.com> writes:
Show 16 quoted lines
> 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.

Andy Parkins· Mar 3, 2007, 08:25 UTC · re: Junio C Hamano · lore

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

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?
Show changes to builtin-mailinfo.c +1 −1
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
Junio C Hamano· Mar 3, 2007, 11:52 UTC · re: Andy Parkins · lore

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

Andy Parkins <andyparkins@gmail.com> writes:
Show 19 quoted lines
> 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.

Linus Torvalds· Mar 4, 2007, 21:44 UTC · re: Junio C Hamano · lore

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

On Sat, 3 Mar 2007, Junio C Hamano wrote:
Show 5 quoted lines
> 
> 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

← back to recent threads