git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johan Herland <johan@herland.net>
Date
May 16, 2009, 11:20 UTC
Message-ID
<200905161320.45426.johan@herland.net>
In-Reply-To
<7vpre9plwl.fsf@alter.siamese.dyndns.org>
On Saturday 16 May 2009, Junio C Hamano wrote:
Show 16 quoted lines
> 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').

Show 17 quoted lines
> 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.
Show 20 quoted lines
>     +	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.
Show 28 quoted lines
> 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
Previous: Junio C HamanoNext: Johan Herland
Message 8 of 19 in “RESEND: git notes”
  1. 0/5 RESEND: git notesJohan Herland, May 16, 2009
  2. 1/5 Introduce commit notesJohan Herland, May 16, 2009
  3. 2/5 Add a script to edit/inspect notesJohan Herland, May 16, 2009
  4. 3/5 Speed up git notes lookupJohan Herland, May 16, 2009
  5. 4/5 Add an expensive test for git-notesJohan Herland, May 16, 2009
  6. 5/5 Teach "-m <msg>" and "-F <file>" to "git notes edit"Johan Herland, May 16, 2009
  7. Junio C HamanoMay 16, 2009
  8. Johan HerlandMay 16, 2009
  9. 0/5 RESEND: git notesJohan Herland, May 16, 2009
  10. Johannes SchindelinMay 20, 2009
  11. Johan HerlandMay 20, 2009
  12. 1/5 Introduce commit notesJohan Herland, May 16, 2009
  13. 2/5 Add a script to edit/inspect notesJohan Herland, May 16, 2009
  14. 3/5 Speed up git notes lookupJohan Herland, May 16, 2009
  15. 4/5 Add an expensive test for git-notesJohan Herland, May 16, 2009
  16. 5/5 Teach "-m <msg>" and "-F <file>" to "git notes edit"Johan Herland, May 16, 2009
  17. git@drmicha.warpmail.netMay 16, 2009
  18. Johan HerlandMay 16, 2009
  19. Johannes SchindelinMay 17, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.