threads / patch / 17697

patchFix contrib/hooks/post-receive-email for new branch with no new commits

Subject: [PATCH] Fix contrib/hooks/post-receive-email for new branch with no new commits

## tl;dr

5 messages between Feb 10, 2009 and Feb 10, 2009. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

Pat Notz· Feb 10, 2009, 13:48 UTC · lore
In the show_new_revisions function, the original code:
   git rev-parse --not --branches | grep -v $(git rev-parse $refname) |

isn't quite right since one can create a new branch and push it without any new commits. In that case, two refs will have the same sha1 but both would get filtered by the 'grep'. In the end, we'll show ALL the history which is not what we want. Instead, we should list the branches by name and remove the branch being updated and THEN pass that list through rev-parse.

Signed-off-by: Pat Notz <pknotz@sandia.gov>
---
 contrib/hooks/post-receive-email |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Show changes to contrib/hooks/post-receive-email +3 −1
diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index 28a3c0e..116f89c 100644
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -615,7 +615,9 @@ show_new_revisions()
 		revspec=$oldrev..$newrev
 	fi
 
-	git rev-parse --not --branches | grep -v $(git rev-parse $refname) |
+	this_branch=$(echo $refname | sed 's@refs/heads/@@')
+	other_branches=$(git branch | sed 's/\*//g' | grep -v $this_branch)
+	git rev-parse --not $other_branches |
 	if [ -z "$custom_showrev" ]
 	then
 		git rev-list --pretty --stdin $revspec

-- 
1.6.1.2
Jakub Narebski· Feb 10, 2009, 15:46 UTC · re: Pat Notz · lore

Re: [PATCH] Fix contrib/hooks/post-receive-email for new branch with no new commits

"Pat Notz" <pknotz@sandia.gov> writes:
Show 10 quoted lines
> In the show_new_revisions function, the original code:
> 
>    git rev-parse --not --branches | grep -v $(git rev-parse $refname) |
> 
> isn't quite right since one can create a new branch and push it without
> any new commits.  In that case, two refs will have the same sha1 but
> both would get filtered by the 'grep'.  In the end, we'll show ALL the
> history which is not what we want.  Instead, we should list the branches
> by name and remove the branch being updated and THEN pass that list
> through rev-parse.
Good idea, bad execution.
Show 17 quoted lines
> 
> Signed-off-by: Pat Notz <pknotz@sandia.gov>
> ---
>  contrib/hooks/post-receive-email |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
> index 28a3c0e..116f89c 100644
> --- a/contrib/hooks/post-receive-email
> +++ b/contrib/hooks/post-receive-email
> @@ -615,7 +615,9 @@ show_new_revisions()
>  		revspec=$oldrev..$newrev
>  	fi
>  
> -	git rev-parse --not --branches | grep -v $(git rev-parse $refname) |
> +	this_branch=$(echo $refname | sed 's@refs/heads/@@')
> +	other_branches=$(git branch | sed 's/\*//g' | grep -v $this_branch)

git-branch is porcelain, git-branch is porcelain, git-branch is porcelain, git-branch is porcelain, git-branch is porcelain, git-branch is porcelain, git-branch is porcelain, git-branch is porcelain, ...

Don't use sed if shell will suffice...
Either:
+	this_branch=$refname
+	other_branches=$(git for-each-ref --format='%(refname)' refs/heads/ |
+               grep -v $this_branch)
or

+ this_branch=${refname#refs/heads/} ...

Show 8 quoted lines
> +	git rev-parse --not $other_branches |
>  	if [ -z "$custom_showrev" ]
>  	then
>  		git rev-list --pretty --stdin $revspec
> -- 
> 1.6.1.2
> 
> 
-- 
Jakub Narebski
Poland
ShadeHawk on #git
Johannes Schindelin· Feb 10, 2009, 15:59 UTC · re: Jakub Narebski · lore

Re: [PATCH] Fix contrib/hooks/post-receive-email for new branch with no new commits

Hi,
On Tue, 10 Feb 2009, Jakub Narebski wrote:
Show 14 quoted lines
> "Pat Notz" <pknotz@sandia.gov> writes:
> 
> > In the show_new_revisions function, the original code:
> > 
> >    git rev-parse --not --branches | grep -v $(git rev-parse $refname) |
> > 
> > isn't quite right since one can create a new branch and push it without
> > any new commits.  In that case, two refs will have the same sha1 but
> > both would get filtered by the 'grep'.  In the end, we'll show ALL the
> > history which is not what we want.  Instead, we should list the branches
> > by name and remove the branch being updated and THEN pass that list
> > through rev-parse.
> 
> Good idea, bad execution.
And I thought that I hold the patent for grumpy comments on this list :-)

As for your suggestions, I think they are valid. We try to keep the interface of certain commands (so called "plumbing") stable, for script consumption. "git for-each-ref" is such a command.

However, "git branch" is meant for human consumption, and a pretty recent patch wants to change the interface to make it even friendlier -- but breaking scripts' assumption in the process, should they use "git branch".

Ciao, Dscho

Junio C Hamano· Feb 10, 2009, 16:30 UTC · re: Jakub Narebski · lore

Re: [PATCH] Fix contrib/hooks/post-receive-email for new branch with no new commits

Jakub Narebski <jnareb@gmail.com> writes:
> +	this_branch=$refname
> +	other_branches=$(git for-each-ref --format='%(refname)' refs/heads/ |
> +               grep -v $this_branch)
This is still not quite right.  grep -F -v "$this_branch" perhaps?
Pat Notz· Feb 10, 2009, 16:43 UTC · re: Junio C Hamano · lore

[PATCH] Fix contrib/hooks/post-receive-email for new duplicate branch

In the show_new_revisions function, the original code:
  git rev-parse --not --branches | grep -v $(git rev-parse $refname) |

isn't quite right since one can create a new branch and push it without any new commits. In that case, two refs will have the same sha1 but both would get filtered by the 'grep'. In the end, we'll show ALL the history which is not what we want. Instead, we should list the branches by name and remove the branch being updated and THEN pass that list through rev-parse.

Revised as suggested by Jakub Narebski and Junio C Hamano to use git-for-each-ref instead of git-branch. (Thanks!)

Signed-off-by: Pat Notz <pknotz@sandia.gov>
---
 contrib/hooks/post-receive-email |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Show changes to contrib/hooks/post-receive-email +3 −1
diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index 28a3c0e..60cbab6 100644
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -615,7 +615,9 @@ show_new_revisions()
 		revspec=$oldrev..$newrev
 	fi
 
-	git rev-parse --not --branches | grep -v $(git rev-parse $refname) |
+	other_branches=$(git for-each-ref --format='%(refname)' refs/heads/ |
+	    grep -F -v $refname)
+	git rev-parse --not $other_branches |
 	if [ -z "$custom_showrev" ]
 	then
 		git rev-list --pretty --stdin $revspec

-- 
1.6.1.2

← back to recent threads