On Saturday 16 May 2009, Junio C Hamano wrote:
> Johan Herland <johan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html