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

On Saturday 16 May 2009, Junio C Hamano wrote:

Johan Herland [off-list ref] writes:

quoted

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, [off-list ref]
www.herland.net