Re: [PATCH] A utility to perform merges between Subversion branches using git

From: Junio C Hamano <junkio@cox.net>
Date: Apr 27, 2007, 04:30 UTC
Message-ID: <7vy7kephg8.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20070427005528.GA12688@midwinter.com>

Steven Grimm <koreth@midwinter.com> writes:

Show 26 quoted lines

> diff --git a/contrib/svn/git-svnmerge b/contrib/svn/git-svnmerge
> new file mode 100755
> index 0000000..e7ed45d
> --- /dev/null
> +++ b/contrib/svn/git-svnmerge
> @@ -0,0 +1,139 @@
> +#!/bin/sh
> +#
> +# Handles svn merges using git-svn.
> +#
> +# Usage: git-svnmerge [-m commit-message] other-branch-name
> +#
> +# This script pulls the changes from another svn branch into the current branch
> +# and checks the result into svn. Then it updates the grafts file so that the
> +# merge is known to git. That means that subsequent runs of the script will
> +# automatically know which changes to apply.
> +#
> +# The user is assumed to have already updated the repository using git-svn.
> +#
> +# Exit codes:
> +#   0 Merge successfully committed to svn.
> +#   1 Merge has conflicts that need to be resolved.
> +#   2 Encountered an error.
> +#
> +# Author: Steven Grimm <koreth@midwinter.com>
> +#

This is contrib stuff, so I'll limit the comments mostly to style.

> +
> +die() {
> +	echo "$@" 1>&2

Many of our scripts do redirection at the beginning, like:

	echo >&2 ...

Also omitting 1 when redirecting to stderr is an idiom.

> +	exit 2
> +}

Does exit value 2 have any significance, or just any non-zero value would do?

> +
> +store_status() {
> +	echo $Status $OldHead $RevToMerge $CommitMessage > "$StatusFile"
> +}

We do not seem to do CamelCase on shell variable names. This is minor, as it is mostly taste.

> +
> +while test $# -gt 1; do

        while <<command>>
        do

without semicolon.

> +	case "$1" in
> +		-m)

Align case arms with "case/esac", just like you would do "switch" and "case" in C.

> +		*)
> +			die "Unknown argument $1"
> +	esac

... and do not omit ';;' just like you do not omit "break" in the last case arm in switch in C.

> +done
> +
> +if test $# -lt 1; then

	if <<command>>
        then

without semicolon.

> +	die "Usage: $0 [-m commit-message] other-branch-name"
> +fi
> +
> +GitDir="`git rev-parse --git-dir`"

GIT_DIR?

Show 20 quoted lines

> +if test -z "$GitDir"; then
> +	die "Not a git repository."
> +fi
> +StatusFile="$GitDir/svnmerge-status"
> +
> +# We might be resuming a previous run, so get the old state if any.
> +Status="start"
> +if test -f "$StatusFile"; then
> +	read Status OldHead RevToMerge CommitMessage < "$StatusFile"
> +fi
> +
> +if test "$Status" = "start"; then
> +	# Make sure there aren't uncommitted (to svn) changes here.
> +	if test -n "`git svn dcommit -n`"; then
> +		die "Can't merge to dirty branch"
> +	fi
> +
> +	# Record the revisions we're merging. We'll use them in the
> +	# grafts file later.
> +	OldHead="`git rev-list --max-count=1 HEAD`"

OldHead=`git rev-parse --verify HEAD`

> +	RevToMerge="`git rev-list --max-count=1 $1`"
> +	if test -z "$RevToMerge"; then
> +		die "Can't merge nonexistent branch."
> +	fi

Wouldn't it possible that $1 might have $IFS in it here, would it?

	RevToMerge=`git rev-parse --verify "$1"`

If you are expecting "$1" to be only an existing branch, not just an arbitrary rev, you might want to be more strict by saying:

	RevToMerge=`git show-ref -s refs/heads/"$1"` ||
        die "Can't merge nonexistent branch."

Show 32 quoted lines

> +
> +	# Do the actual merge.
> +	git merge --squash "$RevToMerge" || \
> +		MergeFailed=1
> +
> +	# Did we actually merge anything?
> +	if git status > /dev/null; then
> +		:
> +	else
> +		echo "No changes to merge. Have you fetched from svn?"
> +		exit 0
> +	fi
> +
> +	Status=merged
> +	store_status
> +
> +	# If there are conflicts, bail out.
> +	if test -n "$MergeFailed`git ls-files -u`"; then
> +		echo "Please resolve conflicts and run again to continue."
> +		exit 1
> +	fi
> +fi
> +
> +if test "$Status" = "merged"; then
> +	if test -n "`git ls-files -u`"; then
> +		echo "There are still conflicts; can't continue."
> +		exit 1
> +	fi
> +
> +	SvnRevision="`git log --pretty=format:%b -n 1 $RevToMerge | \
> +			egrep '^git-svn-id:' | \
> +			sed 's:.*/\([^/]*\)@\([0-9]*\) [0-9a-f].*:\1 revision \2:'`"

Piping grep to sed often makes your script look amateurish. How about...

        script='s|^git-svn-id:.*/\([^/]*\)@\([0-9]*\) [0-9a-f].*|\1 revision \2|p'
        git show -s --pretty=format:%b $RevToMerge |
        sed -n -e "$script"

Show 24 quoted lines

> +	echo "$SvnRevision merged. Committing."
> +
> +	if test -z "$CommitMessage"; then
> +		CommitMessage="Merge from $SvnRevision"
> +	fi
> +
> +	git commit -m "$CommitMessage" || \
> +		die "Commit failed"
> +
> +	Status=committed
> +	store_status
> +fi
> +
> +# Since git-svn dcommit can fail for reasons having nothing to do with the
> +# local repository (e.g. the svn server is down), we allow the user to retry
> +# the dcommit by running this command again.
> +if test "$Status" = "committed"; then
> +	git svn dcommit || \
> +		die "Can't commit to Subversion"
> +
> +	# dcommit will update our current HEAD to point to the newly committed
> +	# svn revision. Update grafts file to tell git that it's a merge.
> +	NewRevision="`git rev-list --max-count=1 HEAD`"
> +	echo "$NewRevision $OldHead $RevToMerge" >> $GitDir/info/grafts

This graft look very yucky, and would not scale.

Presumably, the tree in HEAD commit before dcommit match the one in NewRevision, right? I wonder if there is a way to convince "git-svn" after dcommit that the commit you made yourself above is where it should stay.

Show 14 quoted lines

> +
> +	rm "$StatusFile"
> +	echo "Successfully merged $SvnRevision."
> +
> +	Status=""
> +fi
> +
> +if test -n "$Status"; then
> +	die "Unknown status $Status in status file!"
> +fi
> +
> +exit 0
> -- 
> 1.5.2.rc0.35.gf41c8