Re: [PATCH 3/6] Add git-notes

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 16, 2007, 05:11 UTC
Message-ID: <7v8x9h6igv.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0707160023360.14781@racer.site>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 26 quoted lines

> This script allows you to edit and show commit notes easily.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  .gitignore   |    1 +
>  Makefile     |    2 +-
>  git-notes.sh |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 1 deletions(-)
>  create mode 100755 git-notes.sh
>
> diff --git a/git-notes.sh b/git-notes.sh
> new file mode 100755
> index 0000000..e0ad0b9
> --- /dev/null
> +++ b/git-notes.sh
> @@ -0,0 +1,61 @@
> +#!/bin/sh
> +
> +USAGE="(edit | show) [commit]"
> +. git-sh-setup
> +
> +test -n "$3" && usage
> +
> +test -z "$GIT_NOTES_REF" && GIT_NOTES_REF="$(git config core.notesref)"
> +test -z "$GIT_NOTES_REF" &&
> +	die "No notes ref set."

	test -n "${GIT_NOTES_REF=$(git config core.notesref)}" || die

> +COMMIT=$(git rev-parse --verify --default HEAD "$2")

This silently annotates the HEAD commit if $2 is misspelled, I suspect. Also if HEAD does not exist, COMMIT will be empty and this whole command will exit with non-zero status, which you would want to catch here...

> +NAME=$(echo $COMMIT | sed "s/^../&\//")

... or here.

> +case "$1" in
> +edit)
> +	MESSAGE="$GIT_DIR"/new-notes
> +	GIT_NOTES_REF= git log -1 $COMMIT | sed "s/^/#/" > "$MESSAGE"

$MESSAGE and its associated temporary file needs to be cleaned up upon command exit; perhaps a trap is in order.

Show 11 quoted lines

> +	GIT_INDEX_FILE="$MESSAGE".idx
> +	export GIT_INDEX_FILE
> +
> +	CURRENT_HEAD=$(git show-ref $GIT_NOTES_REF | cut -f 1 -d ' ')
> +	if [ -z "$CURRENT_HEAD" ]; then
> +		PARENT=
> +	else
> +		PARENT="-p $OLDTIP"
> +		git read-tree $GIT_NOTES_REF || die "Could not read index"
> +		git cat-file blob :$NAME >> "$MESSAGE" 2> /dev/null
> +	fi

I take that OLDTIP is a typo.

	if CURRENT_HEAD=$(git show-ref -s "$GIT_NOTES_REF")
        then
		PARENT="-p $CURRENT_HEAD"
                ...
	else
        	PARENT=
	fi

> +
> +	${VISUAL:-${EDITOR:-vi}} "$MESSAGE"
> +
> +	grep -v ^# < "$MESSAGE" | git stripspace > "$MESSAGE".processed

Makes us wonder if we would want to teach hash-stripping to git-stripspace, doesn't it?

> +	mv "$MESSAGE".processed "$MESSAGE"
> +	if [ -z "$(cat "$MESSAGE")" ]; then

Make this 'if test -s "$MESSAGE"' and swap then/else clause around; no reason to slurp the value into your shell.

Show 14 quoted lines

> +		test -z "$CURRENT_HEAD" &&
> +			die "Will not initialise with empty tree"
> +		git update-index --force-remove $NAME ||
> +			die "Could not update index"
> +	else
> +		BLOB=$(git hash-object -w "$MESSAGE") ||
> +			die "Could not write into object database"
> +		git update-index --add --cacheinfo 0644 $BLOB $NAME ||
> +			die "Could not write index"
> +	fi
> +
> +	TREE=$(git write-tree) || die "Could not write tree"
> +	NEW_HEAD=$(: | git commit-tree $TREE $PARENT) ||
> +		die "Could not annotate"

Hmph.  How about "echo Annotate $COMMIT | git commit-tree..."?

Show 5 quoted lines

> +	case "$CURRENT_HEAD" in
> +	'') git update-ref $GIT_NOTES_REF $NEW_HEAD ;;
> +	*) git update-ref $GIT_NOTES_REF $NEW_HEAD $CURRENT_HEAD;;
> +	esac
> +;;

There are some places that have "$GIT_NOTES_REF" in dq and some places you don't. I think GIT_NOTES_REF begins with refs/ and consists only of valid refname characters, so unless the user wants to shoot himself in the foot it should be Ok, but we probably would want to quote it.

Also, as unquoted $CURRENT_HEAD will not even count as a parameter to update-ref, you do not have to do that case/esac, but simply do:

	git update-ref "$GIT_NOTES_REF" $NEW_HEAD $CURRENT_HEAD

Would we have reflog for this ref? What would we want to see as the message if we do?