Re: [PATCH 0/5] RESEND: git notes

[Johan Herland <johan@herland.net>]

On Saturday 16 May 2009, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > In sum, these 5 patches produce the exact same result as the original
> > js/notes series (plus my 2 patches).
>
> Thanks, but you did no such thing, actually.
>
> The result from this sequence
>
> 	git checkout -b jh/notes v1.6.3.1^0
>         git am ./+jh-notes.mbox ;# these patches
> 	git checkout v1.6.3.1^0
>         git merge js/notes ;# allow rerere to reapply the resolution
>         git diff -R jh/notes^
>
> should be empty, or should show your improvements over what has been
> queued in 'pu'.

Sorry for the screwup. The diff should in any case not be empty, since
the first patch (the cleanup/bugfix part) of my original 2 patches
(which are NOT in js/notes on 'pu') has been squashed into the first 4
patches of this new series.

Therefore, the diff you quote below is simply this first patch from my
original two-part series (the second patch of that series is now the
5th patch in this series - adding '-m' and '-F' support to 'git notes
edit').

> Here is what I see:
>
>     diff --git b/git-notes.sh a/git-notes.sh
>     index 6ec33c9..7c3b8b9 100755
>     --- b/git-notes.sh
>     +++ a/git-notes.sh
>     @@ -20,15 +20,16 @@ edit)
>                     die "Refusing to edit notes in $GIT_NOTES_REF (outside of refs/notes/)" fi
>
>     -	MESSAGE="$GIT_DIR"/new-notes-$COMMIT
>     +	MSG_FILE="$GIT_DIR/new-notes-$COMMIT"
>     +	GIT_INDEX_FILE="MSG_FILE.idx"
>
> Renaming of the variable MESSAGE to MSG_FILE may be an improvement, as
> the former could have puzzled the reader if $MESSAGE contains the message
> itself, or it has the name of a file that stores the message (the answer
> is latter).

Indeed. If you look at the next patch (adding support for '-m' and
'-F'), you will see that I reuse $MESSAGE to hold the _actual_ message
(given by '-m' or '-F').

> But I think there is a regression with a missing '$' here.

Yes. Thanks for catching.

>     +	export GIT_INDEX_FILE
>     +
>             trap '
>     -		test -f "$MESSAGE" && rm "$MESSAGE"
>     +		test -f "$MSG_FILE" && rm "$MSG_FILE"
>     +		test -f "$GIT_INDEX_FILE" && rm "$GIT_INDEX_FILE"
>             ' 0
>
> Another improvement is that the variable definitions were moved up before
> the trap that uses these definitions.  This is a good change.
>
>     -	GIT_NOTES_REF= git log -1 $COMMIT | sed "s/^/#/" > "$MESSAGE"
>     -
>     -	GIT_INDEX_FILE="$MESSAGE".idx
>     -	export GIT_INDEX_FILE
>     +	git log -1 $COMMIT | sed "s/^/#/" > "$MSG_FILE"
>
> Dscho's version exports an empty GIT_NOTES_REF, presumably to avoid
> triggering the notes mechanism, while running this "git log -1", but
> yours don't.  Is there a reason behind this change?

Oh, so _that's_ what he did... I was puzzled by that "stray"
'GIT_NOTES_REF= '...

Yet another screwup on my part.

> The rest are fallouts from s/MESSAGE/MSG_FILE/ renaming.
>
>     @@ -36,16 +37,16 @@ edit)
>             else
>                     PARENT="-p $CURRENT_HEAD"
>                     git read-tree "$GIT_NOTES_REF" || die "Could not read index"
>     -		git cat-file blob :$COMMIT >> "$MESSAGE" 2> /dev/null
>     +		git cat-file blob :$COMMIT >> "$MSG_FILE" 2> /dev/null
>             fi
>
>             core_editor="$(git config core.editor)"
>     -	${GIT_EDITOR:-${core_editor:-${VISUAL:-${EDITOR:-vi}}}} "$MESSAGE"
>     +	${GIT_EDITOR:-${core_editor:-${VISUAL:-${EDITOR:-vi}}}} "$MSG_FILE"
>
>     -	grep -v ^# < "$MESSAGE" | git stripspace > "$MESSAGE".processed
>     -	mv "$MESSAGE".processed "$MESSAGE"
>     -	if [ -s "$MESSAGE" ]; then
>     -		BLOB=$(git hash-object -w "$MESSAGE") ||
>     +	grep -v ^# < "$MSG_FILE" | git stripspace > "$MSG_FILE".processed
>     +	mv "$MSG_FILE".processed "$MSG_FILE"
>     +	if [ -s "$MSG_FILE" ]; then
>     +		BLOB=$(git hash-object -w "$MSG_FILE") ||
>       		die "Could not write into object database"
>       	git update-index --add --cacheinfo 0644 $BLOB $COMMIT ||
>       		die "Could not write index"
>
> I am a bit reluctant to take this to replace js/notes as-is until I hear
> answers to these points (and others may have comments too).

Thanks for the review.
I will send an updated series with the above fixes.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net