Edgewall Software
Modify

Opened 5 years ago

Closed 5 years ago

#12763 closed defect (duplicate)

Example post-receive git hook is dangerous

Reported by: strk@… Owned by:
Priority: normal Milestone:
Component: plugin/git Version:
Severity: normal Keywords: hook
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

The example post-receive hook for explicit sync of git repository found on https://trac.edgewall.org/wiki/TracRepositoryAdmin#Git is dangerous because it invokes trac-admin for ALL the commits upon pushing a new branch, even if the new branch has exactly the same history of an existing one, and is broken in that *dropping* a branch results in an error.

The current example code is as follows:

#!/bin/sh
tracenv=/path/to/env     # change with your Trac environment's path
repos=                   # change with your repository's name
while read oldrev newrev refname; do
    if [ "$oldrev" = 0000000000000000000000000000000000000000 ]; then
        git rev-list --reverse "$newrev" --
    else
        git rev-list --reverse "$newrev" "^$oldrev" --
    fi | xargs trac-admin "$tracenv" changeset added "$repos"
done

A saner version would be (tested):

#!/bin/sh
tracenv=/path/to/env     # change with your Trac environment's path
repos=                   # change with your repository's name
while read oldrev newrev refname; do
    if expr "$oldrev" : '0*$' >/dev/null; then
        branches=`git for-each-ref --format="%(refname)" | grep -vw "$refname"`
        git rev-list --reverse "$newrev" --not $branches --
    elif expr "$newrev" : '0*$' >/dev/null; then
        : # do nothing upon removing a branch
    else
        git rev-list --reverse "$newrev" "^$oldrev" --
    fi | xargs --no-run-if-empty trac-admin "$tracenv" changeset added "$repos"

done

Using the current example in production resulted in adding ~4000 commits at once, and for each commit containing a ticket reference sending out email notifications to all ticket subscribers. Pretty huge flood…

Attachments (0)

Change History (7)

comment:1 by Ryan J Ollos, 5 years ago

Component: projectplugin/git
Keywords: hook added; website removed

See also #10730. If the script in comment:11:ticket:10730 also fixes the behavior we should just add that script to /contrib and update the documentation.

comment:2 by strk@…, 5 years ago

In our case we also want to send an email for every changeset, so using an external script like the one you reference won't work as it would consume the standard input.

comment:3 by Jun Omae, 5 years ago

The proposed script has another issue, which is ignoring new commits in the new branch.

comment:4 by anonymous, 5 years ago

It does not, was tested

comment:5 by Jun Omae, 5 years ago

Ah, sorry. I've misunderstood.

Another issue in the example and the proposed script: It wouldn't work as expected when multiple branches are pushed.

comment:6 by strk@…, 5 years ago

I did not test pushing multiple branches but I believe in that case the hook would get multiple lines on stdin (one per pushed branch). For each line, only the commits that are exclusively in the pushed branch are added to trac, which I guess would mean that pushing 2 branches with the same new commits would get no trac-admin call at all. If that's confirmed the only way out of it would be reading all of stdin before starting the work.

comment:7 by Ryan J Ollos, 5 years ago

Resolution: duplicate
Status: newclosed

Closing as a duplicate of #10730.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The ticket will remain with no owner.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none) to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.