threads / discuss / 23075

Problem with contrib/hooks/post-receive-email

Subject: Problem with contrib/hooks/post-receive-email

## tl;dr

4 messages between Mar 19, 2010 and Mar 20, 2010.

replies: 3people: 3as markdown or json

Eli Barzilay· Mar 19, 2010, 06:39 UTC · lore

The post-receive-email script goes out of its way to avoid sending commits twice by filtering out commits that are included in existing refs, but if more than one branch changes then some commits can end up not being reported. For example, I made two commits A and B, made one branch point at A and another at B, and pushed both -- neither of the resulting two emails had A.

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

Andy Parkins· Mar 19, 2010, 16:39 UTC · re: Eli Barzilay · lore

Re: Problem with contrib/hooks/post-receive-email

Eli Barzilay wrote:

Show 6 quoted lines

> The post-receive-email script goes out of its way to avoid sending
> commits twice by filtering out commits that are included in existing
> refs, but if more than one branch changes then some commits can end up
> not being reported.  For example, I made two commits A and B, made one
> branch point at A and another at B, and pushed both -- neither of the
> resulting two emails had A.

<Andy starts crying>

I can't see any way to deal with this case easily with post-receive-email as it is. It inherently processes ref-by-ref. The relevant bit of script is in generate_update_branch_email(). The comments explain how the same problem is addressed for another developer changing the same branch before post-receive-email runs, but after the update is performed. I think the same method could be applied.

  git rev-parse --not --all | grep -v $(git rev-parse $refname)

This line is where the particular branches are being included and excluded. The problem you have is that "--all" means "--all-at-the-moment", and you want "--all-as-they-were-before-the-update".

So, --all will have to go, and a manual list built instead. The supplied change list includes all the information necessary:

 ref1_oldrev ref1_newrev ref1
 ref2_oldrev ref2_newrev ref2
 ref3_oldrev ref3_newrev ref3
 ref4_oldrev ref4_newrev ref4

Let's say there is also a ref5 and ref6 in the repository. The revision list we want for (say) the ref1 call to generate_email would be:

 ref1_newrev
 ^ref2_oldrev
 ^ref3_oldrev
 ^ref4_oldrev
 ^ref5
 ^ref6

And similarly for ref2, ref3 and ref4. It seems to me that it needs a hash table keyed on the refname, but I have no idea how to do that in bash.

 %originalreftable{"ref1"} = "^ref1_oldrev"
 %originalreftable{"ref2"} = "^ref2_oldrev"
 %originalreftable{"ref3"} = "^ref3_oldrev"
 %originalreftable{"ref4"} = "^ref4_oldrev"
 %originalreftable{"ref5"} = "^ref5"
 %originalreftable{"ref6"} = "^ref6"

This table would be sufficient to create the revision list for every generate_email(), because each generate_email() knows which ref it's being updated for, so could easily do:

 %originalreftable{$myref} = "$mynewrev"

Before using the table (and restore it afterwards).

In short: yuck. It feels an awful lot like its pushing the boundaries of what is sensible to do in shell script.

Andy

-- 
Dr Andy Parkins
andyparkins@gmail.com

Brandon Casey· Mar 19, 2010, 18:44 UTC · re: Andy Parkins · lore

Re: Problem with contrib/hooks/post-receive-email

On 03/19/2010 11:39 AM, Andy Parkins wrote:

Show 62 quoted lines

> Eli Barzilay wrote:
> 
>> The post-receive-email script goes out of its way to avoid sending
>> commits twice by filtering out commits that are included in existing
>> refs, but if more than one branch changes then some commits can end up
>> not being reported.  For example, I made two commits A and B, made one
>> branch point at A and another at B, and pushed both -- neither of the
>> resulting two emails had A.
> 
> <Andy starts crying>
> 
> I can't see any way to deal with this case easily with post-receive-email as 
> it is.  It inherently processes ref-by-ref.  The relevant bit of script is 
> in generate_update_branch_email().  The comments explain how the same 
> problem is addressed for another developer changing the same branch before 
> post-receive-email runs, but after the update is performed.  I think the 
> same method could be applied.
> 
>   git rev-parse --not --all | grep -v $(git rev-parse $refname)
> 
> This line is where the particular branches are being included and excluded.  
> The problem you have is that "--all" means "--all-at-the-moment", and you 
> want "--all-as-they-were-before-the-update".
> 
> So, --all will have to go, and a manual list built instead.  The supplied 
> change list includes all the information necessary:
> 
>  ref1_oldrev ref1_newrev ref1
>  ref2_oldrev ref2_newrev ref2
>  ref3_oldrev ref3_newrev ref3
>  ref4_oldrev ref4_newrev ref4
> 
> Let's say there is also a ref5 and ref6 in the repository.  The revision 
> list we want for (say) the ref1 call to generate_email would be:
> 
>  ref1_newrev
>  ^ref2_oldrev
>  ^ref3_oldrev
>  ^ref4_oldrev
>  ^ref5
>  ^ref6
> 
> And similarly for ref2, ref3 and ref4.  It seems to me that it needs a hash 
> table keyed on the refname, but I have no idea how to do that in bash.
> 
>  %originalreftable{"ref1"} = "^ref1_oldrev"
>  %originalreftable{"ref2"} = "^ref2_oldrev"
>  %originalreftable{"ref3"} = "^ref3_oldrev"
>  %originalreftable{"ref4"} = "^ref4_oldrev"
>  %originalreftable{"ref5"} = "^ref5"
>  %originalreftable{"ref6"} = "^ref6"
> 
> This table would be sufficient to create the revision list for every 
> generate_email(), because each generate_email() knows which ref it's being 
> updated for, so could easily do:
> 
>  %originalreftable{$myref} = "$mynewrev"
> 
> Before using the table (and restore it afterwards).
> 
> In short: yuck.  It feels an awful lot like its pushing the boundaries of 
> what is sensible to do in shell script.

Here's how I did it. I did this so quickly and so long ago that I forgot about giving it a final think-through and submitting it. Now that I look at it again, and the fact that it has been working for a good long while, it doesn't look as ugly as I remember it.

What do you think? Sane enough?

Warning: copy/pasted so beware white space corruption

diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index 58a35c8..42dc4e7 100755
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -619,9 +619,8 @@ show_new_revisions()
                revspec=$oldrev..$newrev
        fi

-       other_branches=$(git for-each-ref --format='%(refname)' refs/heads/ |
-           grep -F -v $refname)
-       git rev-parse --not $other_branches |
+       git rev-parse --not --branches | grep -v $(git rev-parse $refname) |
+               sed $sed_rev_script |
        if [ -z "$custom_showrev" ]
        then
                git rev-list --pretty --stdin $revspec
@@ -680,10 +679,22 @@ if [ -n "$1" -a -n "$2" -a -n "$3" ]; then
        # Output to the terminal in command line mode - if someone wanted to
        # resend an email; they could redirect the output to sendmail
        # themselves
+       sed_rev_script="-e s/$3/$2/"
        PAGER= generate_email $2 $3 $1
 else
+       numrevs=0
        while read oldrev newrev refname
        do
-               generate_email $oldrev $newrev $refname | send_mail
+               sed_rev_script="$sed_rev_script -e s/$newrev/$oldrev/"
+               oldrevs[$numrevs]=$oldrev
+               newrevs[$numrevs]=$newrev
+               refnames[$numrevs]=$refname
+               numrevs=$(($numrevs + 1))
+       done
+       i=0
+       while [ $i -lt $numrevs ]; do
+               generate_email ${oldrevs[$i]} ${newrevs[$i]} ${refnames[$i]} |
+                       send_mail
+               i=$(($i + 1))
        done
 fi

Eli Barzilay· Mar 20, 2010, 03:21 UTC · re: Andy Parkins · lore

Re: Problem with contrib/hooks/post-receive-email

Andy Parkins <andyparkins@gmail.com> writes:

Show 13 quoted lines

> Eli Barzilay wrote:
>
>> The post-receive-email script goes out of its way to avoid sending
>> commits twice by filtering out commits that are included in
>> existing refs, but if more than one branch changes then some
>> commits can end up not being reported.  For example, I made two
>> commits A and B, made one branch point at A and another at B, and
>> pushed both -- neither of the resulting two emails had A.
>
> <Andy starts crying>
>
> I can't see any way to deal with this case easily with
> post-receive-email as it is.  It inherently processes ref-by-ref.

Well, one thing that sounded obvious to me is to expicitly say something about it. The "danger" that I see in this is a central repository setup and people relying on knowing everything that happens by reading through these emails -- then get a bad surprise when something sneaks in past those emails.

As for a proper solution, I first thought along the lines of what Brandon suggested -- but I considered doing that with a hash table instead of accumulating a sed script.

> And similarly for ref2, ref3 and ref4.  It seems to me that it needs
> a hash table keyed on the refname, but I have no idea how to do that
> in bash.
> [...]

(Isn't that just associative arrays?)

> In short: yuck.  It feels an awful lot like its pushing the
> boundaries of what is sensible to do in shell script.

Yeah, my conclusion was similar... But after considering it for a while, I think that a saner approach is to choose a main branch (eg, `master' or `devel') where all commits are always reported, then for branches show only commits that are not on this main branch. This means that if you only read the emails on this branch you know practically everything that happens, and if you're interested in what happens on a branch you will see some commits again in the future when they're added to the main branch -- but that seems even better to me: even I read some commit when it happened on a branch, I'd still want to know when it's added to the main branch.

This seems to me both more predictable in the sense of the notifications and the code. But I'm in the process of converting a project to git so I might not be experienced enough...

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!