# [PATCH 2/4] notes: use GIT_EDITOR and core.editor over VISUAL/EDITOR 47 messages from 2009-02-14 to 2009-02-19. Participants: Thomas Rast, Johannes Schindelin, Junio C Hamano, Jeff King, Mike Ralphson, Brandon Casey, Jay Soffian, Wincent Colaiuta. Thread: https://gitlist.dev/t/17794 ## Thomas Rast, 2009-02-14 19:15 Subject: [PATCH 1/4] notes: only clean up message file when editing Message-ID: URL: https://gitlist.dev/e/b36bbe2f7f0ede6a2caa47190ada868dd4c0099c.1234638466.git.trast%40student.ethz.ch ``` We clean up the notes file when exiting. However, actions other than 'edit' have nothing to do with the file, so they should not remove it when done. Signed-off-by: Thomas Rast --- git-notes.sh | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/git-notes.sh b/git-notes.sh index 9cbad02..246df65 100755 --- a/git-notes.sh +++ b/git-notes.sh @@ -14,13 +14,13 @@ test -z "$GIT_NOTES_REF" && GIT_NOTES_REF="refs/notes/commits" COMMIT=$(git rev-parse --verify --default HEAD "$@") || die "Invalid commit: $@" -MESSAGE="$GIT_DIR"/new-notes-$COMMIT -trap ' - test -f "$MESSAGE" && rm "$MESSAGE" -' 0 - case "$ACTION" in edit) + MESSAGE="$GIT_DIR"/new-notes-$COMMIT + trap ' + test -f "$MESSAGE" && rm "$MESSAGE" + ' 0 + GIT_NOTES_REF= git log -1 $COMMIT | sed "s/^/#/" > "$MESSAGE" GIT_INDEX_FILE="$MESSAGE".idx -- 1.6.2.rc0.288.g6852b ``` ## Thomas Rast, 2009-02-14 19:15 Subject: [PATCH 2/4] notes: use GIT_EDITOR and core.editor over VISUAL/EDITOR Message-ID: URL: https://gitlist.dev/e/d2ca964918f3f930505cd969cf8407d4a8d67fe3.1234638466.git.trast%40student.ethz.ch In-Reply-To: ``` This is the order documented in git-config(1), so we should stick to it. Signed-off-by: Thomas Rast --- It's actually only _almost_ the documented order: the manpage says that $GIT_EDITOR is used when it is set, presumably even when it is set but empty. I cannot see how that would be useful however. git-notes.sh | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/git-notes.sh b/git-notes.sh index 246df65..6859470 100755 --- a/git-notes.sh +++ b/git-notes.sh @@ -35,7 +35,8 @@ edit) git cat-file blob :$COMMIT >> "$MESSAGE" 2> /dev/null fi - ${VISUAL:-${EDITOR:-vi}} "$MESSAGE" + core_editor="$(git config core.editor)" + ${GIT_EDITOR:-${core_editor:-${VISUAL:-${EDITOR:-vi}}}} "$MESSAGE" grep -v ^# < "$MESSAGE" | git stripspace > "$MESSAGE".processed mv "$MESSAGE".processed "$MESSAGE" -- 1.6.2.rc0.288.g6852b ``` ## Thomas Rast, 2009-02-14 19:15 Subject: [PATCH 3/4] t3301: fix confusing test for valid notes ref Message-ID: <21cefcfb71849535430c75575a9decf1f9fe9c06.1234638466.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/21cefcfb71849535430c75575a9decf1f9fe9c06.1234638466.git.trast%40student.ethz.ch In-Reply-To: ``` The test used single quotes in the test code, but also single quotes to wrap the entire test snippet, thus effectively skipping _out_ of quoted mode. Since it doesn't matter here, just drop the quotes to cause less confusion. Also, the test passed a MSG variable to 'git notes show', but that never calls an editor. Signed-off-by: Thomas Rast --- As an aside, is there a specific reason why all the negative tests use '! VAR=foo bar' instead of 'VAR=foo test_must_fail bar'? I thought the latter was preferred to catch segfaults. t/t3301-notes.sh | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index ff4ea05..b99271e 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -31,8 +31,8 @@ test_expect_success setup ' ' test_expect_success 'need valid notes ref' ' - ! MSG=1 GIT_NOTES_REF='/' git notes edit && - ! MSG=2 GIT_NOTES_REF='/' git notes show + ! MSG=1 GIT_NOTES_REF=/ git notes edit && + ! GIT_NOTES_REF=/ git notes show ' # 1 indicates caught gracefully by die, 128 means git-show barked -- 1.6.2.rc0.288.g6852b ``` ## Thomas Rast, 2009-02-14 19:15 Subject: [PATCH 4/4] notes: refuse to edit notes outside refs/notes/ Message-ID: URL: https://gitlist.dev/e/ac3c2ce272e8e7f3000349f682a5a4843aea34e6.1234638466.git.trast%40student.ethz.ch In-Reply-To: ``` The current git-notes has no safety valves whatsoever, and will happily edit notes in, e.g., refs/remotes/. This is dangerous, as they will be overwritten during the next fetch of the remote they belong to. Since it is supposed to be a plumbing frontend for the notes feature, simply forbid editing notes with GIT_NOTES_REF pointing anywhere but refs/notes/*. Signed-off-by: Thomas Rast --- This is the only patch of substance in this series :-) I'm not sure if anyone uses a notes ref outside of refs/notes/ yet, so this may actually break some people's setups already. But now that we (at least me :-) offer notes for download in public repos, I can see people accidentally edit a remote notes ref. git-notes.sh | 4 ++++ t/t3301-notes.sh | 8 ++++++++ 2 files changed, 12 insertions(+), 0 deletions(-) diff --git a/git-notes.sh b/git-notes.sh index 6859470..6ec33c9 100755 --- a/git-notes.sh +++ b/git-notes.sh @@ -16,6 +16,10 @@ die "Invalid commit: $@" case "$ACTION" in edit) + if [ "${GIT_NOTES_REF#refs/notes/}" = "$GIT_NOTES_REF" ]; then + die "Refusing to edit notes in $GIT_NOTES_REF (outside of refs/notes/)" + fi + MESSAGE="$GIT_DIR"/new-notes-$COMMIT trap ' test -f "$MESSAGE" && rm "$MESSAGE" diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index b99271e..1503e79 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -35,6 +35,14 @@ test_expect_success 'need valid notes ref' ' ! GIT_NOTES_REF=/ git notes show ' +test_expect_success 'refusing to edit in refs/heads/' ' + ! MSG=1 GIT_NOTES_REF=refs/heads/bogus git notes edit +' + +test_expect_success 'refusing to edit in refs/remotes/' ' + ! MSG=1 GIT_NOTES_REF=refs/remotes/bogus git notes edit +' + # 1 indicates caught gracefully by die, 128 means git-show barked test_expect_success 'handle empty notes gracefully' ' git notes show ; test 1 = $? -- 1.6.2.rc0.288.g6852b ``` ## Johannes Schindelin, 2009-02-14 19:29 Subject: Re: [PATCH 1/4] notes: only clean up message file when editing Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902142029010.10279%40pacific.mpi-cbg.de In-Reply-To: ``` Hi, On Sat, 14 Feb 2009, Thomas Rast wrote: > We clean up the notes file when exiting. However, actions other than > 'edit' have nothing to do with the file, so they should not remove it > when done. ACK. Ciao, Dscho ``` ## Johannes Schindelin, 2009-02-14 19:29 Subject: Re: [PATCH 2/4] notes: use GIT_EDITOR and core.editor over VISUAL/EDITOR Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902142029170.10279%40pacific.mpi-cbg.de In-Reply-To: ``` Hi, On Sat, 14 Feb 2009, Thomas Rast wrote: > This is the order documented in git-config(1), so we should stick to it. ACK. Ciao, Dscho ``` ## Johannes Schindelin, 2009-02-14 19:32 Subject: Re: [PATCH 3/4] t3301: fix confusing test for valid notes ref Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902142029420.10279%40pacific.mpi-cbg.de In-Reply-To: <21cefcfb71849535430c75575a9decf1f9fe9c06.1234638466.git.trast@student.ethz.ch> ``` Hi, On Sat, 14 Feb 2009, Thomas Rast wrote: > The test used single quotes in the test code, but also single quotes > to wrap the entire test snippet, thus effectively skipping _out_ of > quoted mode. Since it doesn't matter here, just drop the quotes to > cause less confusion. ACK. > Also, the test passed a MSG variable to 'git notes show', but that > never calls an editor. That was actually what I tried to test by passing MSG=2... So I'd like to keep it. > As an aside, is there a specific reason why all the negative tests use > '! VAR=foo bar' instead of 'VAR=foo test_must_fail bar'? I thought the > latter was preferred to catch segfaults. I guess that back then when I wrote the first version of notes (which I hoped Johan Herland would take custody of), test_must_fail either did not exist or was too new for me to have picked up yet. Ciao, Dscho ``` ## Johannes Schindelin, 2009-02-14 19:33 Subject: Re: [PATCH 4/4] notes: refuse to edit notes outside refs/notes/ Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902142032210.10279%40pacific.mpi-cbg.de In-Reply-To: ``` Hi, On Sat, 14 Feb 2009, Thomas Rast wrote: > The current git-notes has no safety valves whatsoever, and will > happily edit notes in, e.g., refs/remotes/. This is dangerous, as > they will be overwritten during the next fetch of the remote they > belong to. > > Since it is supposed to be a plumbing frontend for the notes feature, > simply forbid editing notes with GIT_NOTES_REF pointing anywhere but > refs/notes/*. ACK. Except maybe for calling it "plumbing". "git notes" is meant to be _the_ porcelain of the "notes", and that is why I like the idea to restrict it to refs/notes/* for editing (not for anything else, though). Thanks, Dscho ``` ## Thomas Rast, 2009-02-14 19:56 Subject: Re: [PATCH 4/4] notes: refuse to edit notes outside refs/notes/ Message-ID: <200902142056.42198.trast@student.ethz.ch> URL: https://gitlist.dev/e/200902142056.42198.trast%40student.ethz.ch In-Reply-To: ``` Johannes Schindelin wrote: > On Sat, 14 Feb 2009, Thomas Rast wrote: > > Since it is supposed to be a plumbing frontend for the notes feature, > > simply forbid editing notes with GIT_NOTES_REF pointing anywhere but > > refs/notes/*. > > ACK. > > Except maybe for calling it "plumbing". "git notes" is meant to be _the_ > porcelain of the "notes", and that is why I like the idea to restrict it > to refs/notes/* for editing (not for anything else, though). That's what I actually wanted to write. Maybe I should go get more coffee... Thanks for the acks. Will resend with 3/4 corrected, and add a conversion to test_must_fail. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Thomas Rast, 2009-02-14 20:23 Subject: [PATCH v2 1/5] notes: only clean up message file when editing Message-ID: <4655551d70d8829a7a15219aef384deb4e63371a.1234642638.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/4655551d70d8829a7a15219aef384deb4e63371a.1234642638.git.trast%40student.ethz.ch In-Reply-To: <200902142056.42198.trast@student.ethz.ch> ``` We clean up the notes file when exiting. However, actions other than 'edit' have nothing to do with the file, so they should not remove it when done. Signed-off-by: Thomas Rast Acked-by: Johannes Schindelin --- Same as v1. git-notes.sh | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/git-notes.sh b/git-notes.sh index 9cbad02..246df65 100755 --- a/git-notes.sh +++ b/git-notes.sh @@ -14,13 +14,13 @@ test -z "$GIT_NOTES_REF" && GIT_NOTES_REF="refs/notes/commits" COMMIT=$(git rev-parse --verify --default HEAD "$@") || die "Invalid commit: $@" -MESSAGE="$GIT_DIR"/new-notes-$COMMIT -trap ' - test -f "$MESSAGE" && rm "$MESSAGE" -' 0 - case "$ACTION" in edit) + MESSAGE="$GIT_DIR"/new-notes-$COMMIT + trap ' + test -f "$MESSAGE" && rm "$MESSAGE" + ' 0 + GIT_NOTES_REF= git log -1 $COMMIT | sed "s/^/#/" > "$MESSAGE" GIT_INDEX_FILE="$MESSAGE".idx -- 1.6.2.rc0.296.ge2122 ``` ## Thomas Rast, 2009-02-14 20:23 Subject: [PATCH v2 2/5] notes: use GIT_EDITOR and core.editor over VISUAL/EDITOR Message-ID: <69c813ea5c4a3c3f43e729bd59a4064f74e72d66.1234642638.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/69c813ea5c4a3c3f43e729bd59a4064f74e72d66.1234642638.git.trast%40student.ethz.ch In-Reply-To: <200902142056.42198.trast@student.ethz.ch> ``` This is the order documented in git-config(1), so we should stick to it. Signed-off-by: Thomas Rast Acked-by: Johannes Schindelin --- Same as v1. git-notes.sh | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/git-notes.sh b/git-notes.sh index 246df65..6859470 100755 --- a/git-notes.sh +++ b/git-notes.sh @@ -35,7 +35,8 @@ edit) git cat-file blob :$COMMIT >> "$MESSAGE" 2> /dev/null fi - ${VISUAL:-${EDITOR:-vi}} "$MESSAGE" + core_editor="$(git config core.editor)" + ${GIT_EDITOR:-${core_editor:-${VISUAL:-${EDITOR:-vi}}}} "$MESSAGE" grep -v ^# < "$MESSAGE" | git stripspace > "$MESSAGE".processed mv "$MESSAGE".processed "$MESSAGE" -- 1.6.2.rc0.296.ge2122 ``` ## Thomas Rast, 2009-02-14 20:23 Subject: [PATCH v2 3/5] t3301: fix confusing quoting in test for valid notes ref Message-ID: <2d4b5321543974ec25921102faef900e42eea1d5.1234642638.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/2d4b5321543974ec25921102faef900e42eea1d5.1234642638.git.trast%40student.ethz.ch In-Reply-To: <200902142056.42198.trast@student.ethz.ch> ``` The test used single quotes in the test code, but also single quotes to wrap the entire test snippet, thus effectively skipping _out_ of quoted mode. Since it doesn't matter here, just drop the quotes to cause less confusion. Signed-off-by: Thomas Rast Acked-by: Johannes Schindelin --- Dropped the removal of MSG=2. t/t3301-notes.sh | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index ff4ea05..43d82a2 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -31,8 +31,8 @@ test_expect_success setup ' ' test_expect_success 'need valid notes ref' ' - ! MSG=1 GIT_NOTES_REF='/' git notes edit && - ! MSG=2 GIT_NOTES_REF='/' git notes show + ! MSG=1 GIT_NOTES_REF=/ git notes edit && + ! MSG=2 GIT_NOTES_REF=/ git notes show ' # 1 indicates caught gracefully by die, 128 means git-show barked -- 1.6.2.rc0.296.ge2122 ``` ## Thomas Rast, 2009-02-14 20:23 Subject: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <8c50889d27e5baec1cbbd9a5775fa80b986c6df6.1234642638.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/8c50889d27e5baec1cbbd9a5775fa80b986c6df6.1234642638.git.trast%40student.ethz.ch In-Reply-To: <200902142056.42198.trast@student.ethz.ch> ``` We should use test_must_fail to negate output status, so that segfaults can be caught. One instance of ! negation remains, but that actually tests the return code of 'grep'. Signed-off-by: Thomas Rast --- New in this series. t/t3301-notes.sh | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 43d82a2..764fad5 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -16,7 +16,7 @@ VISUAL=./fake_editor.sh export VISUAL test_expect_success 'cannot annotate non-existing HEAD' ' - ! MSG=3 git notes edit + MSG=3 test_must_fail git notes edit ' test_expect_success setup ' @@ -31,8 +31,8 @@ test_expect_success setup ' ' test_expect_success 'need valid notes ref' ' - ! MSG=1 GIT_NOTES_REF=/ git notes edit && - ! MSG=2 GIT_NOTES_REF=/ git notes show + MSG=1 GIT_NOTES_REF=/ test_must_fail git notes edit && + MSG=2 GIT_NOTES_REF=/ test_must_fail git notes show ' # 1 indicates caught gracefully by die, 128 means git-show barked @@ -47,7 +47,7 @@ test_expect_success 'create notes' ' test 1 = $(git ls-tree refs/notes/commits | wc -l) && test b1 = $(git notes show) && git show HEAD^ && - ! git notes show HEAD^ + test_must_fail git notes show HEAD^ ' cat > expect << EOF -- 1.6.2.rc0.296.ge2122 ``` ## Thomas Rast, 2009-02-14 20:23 Subject: [PATCH v2 5/5] notes: refuse to edit notes outside refs/notes/ Message-ID: URL: https://gitlist.dev/e/aecacfd80a8b68dfe3090b303c95b42fa1169e76.1234642638.git.trast%40student.ethz.ch In-Reply-To: <200902142056.42198.trast@student.ethz.ch> ``` The current git-notes has no safety valves whatsoever, and will happily edit notes in, e.g., refs/remotes/. This is dangerous, as they will be overwritten during the next fetch of the remote they belong to. Since it is supposed to be a porcelain frontend for the notes feature, simply forbid editing notes with GIT_NOTES_REF pointing anywhere but refs/notes/*. Signed-off-by: Thomas Rast Acked-by: Johannes Schindelin --- Same as v1, except for s/plumbing/porcelain/ in the commit message. git-notes.sh | 4 ++++ t/t3301-notes.sh | 8 ++++++++ 2 files changed, 12 insertions(+), 0 deletions(-) diff --git a/git-notes.sh b/git-notes.sh index 6859470..6ec33c9 100755 --- a/git-notes.sh +++ b/git-notes.sh @@ -16,6 +16,10 @@ die "Invalid commit: $@" case "$ACTION" in edit) + if [ "${GIT_NOTES_REF#refs/notes/}" = "$GIT_NOTES_REF" ]; then + die "Refusing to edit notes in $GIT_NOTES_REF (outside of refs/notes/)" + fi + MESSAGE="$GIT_DIR"/new-notes-$COMMIT trap ' test -f "$MESSAGE" && rm "$MESSAGE" diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 764fad5..f68d8ee 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -35,6 +35,14 @@ test_expect_success 'need valid notes ref' ' MSG=2 GIT_NOTES_REF=/ test_must_fail git notes show ' +test_expect_success 'refusing to edit in refs/heads/' ' + ! MSG=1 GIT_NOTES_REF=refs/heads/bogus git notes edit +' + +test_expect_success 'refusing to edit in refs/remotes/' ' + ! MSG=1 GIT_NOTES_REF=refs/remotes/bogus git notes edit +' + # 1 indicates caught gracefully by die, 128 means git-show barked test_expect_success 'handle empty notes gracefully' ' git notes show ; test 1 = $? -- 1.6.2.rc0.296.ge2122 ``` ## Johannes Schindelin, 2009-02-14 21:26 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902142226200.10279%40pacific.mpi-cbg.de In-Reply-To: <8c50889d27e5baec1cbbd9a5775fa80b986c6df6.1234642638.git.trast@student.ethz.ch> ``` Hi, On Sat, 14 Feb 2009, Thomas Rast wrote: > We should use test_must_fail to negate output status, so that > segfaults can be caught. One instance of ! negation remains, but that > actually tests the return code of 'grep'. ACK. Ciao, Dscho ``` ## Junio C Hamano, 2009-02-15 07:52 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <7vvdrcnnl0.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvdrcnnl0.fsf%40gitster.siamese.dyndns.org In-Reply-To: <8c50889d27e5baec1cbbd9a5775fa80b986c6df6.1234642638.git.trast@student.ethz.ch> ``` Thomas Rast writes: > + MSG=3 test_must_fail git notes edit test_must_fail is a shell function, and we have precedence 2d60615 (tests: Avoid single-shot environment export for shell function invocation, 2009-01-26) to avoid this construct. ``` ## Junio C Hamano, 2009-02-15 07:54 Subject: Re: [PATCH v2 1/5] notes: only clean up message file when editing Message-ID: <7vr620nnid.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr620nnid.fsf%40gitster.siamese.dyndns.org In-Reply-To: <4655551d70d8829a7a15219aef384deb4e63371a.1234642638.git.trast@student.ethz.ch> ``` Thomas Rast writes: > + MESSAGE="$GIT_DIR"/new-notes-$COMMIT > + trap ' > + test -f "$MESSAGE" && rm "$MESSAGE" > + ' 0 > + Hmph, why not unconditionally rm -f "$MESSAGE" without test? ``` ## Thomas Rast, 2009-02-15 16:11 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <200902151711.45099.trast@student.ethz.ch> URL: https://gitlist.dev/e/200902151711.45099.trast%40student.ethz.ch In-Reply-To: <7vvdrcnnl0.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > Thomas Rast writes: > > > + MSG=3 test_must_fail git notes edit > > test_must_fail is a shell function, and we have precedence 2d60615 (tests: > Avoid single-shot environment export for shell function invocation, > 2009-01-26) to avoid this construct. Bah. I really have a bad memory, don't I? Is there a "bare minimum" POSIX shell that I can run the tests under, to save myself such embarassment in the future? -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Jeff King, 2009-02-15 18:18 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <20090215181818.GA2291@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090215181818.GA2291%40coredump.intra.peff.net In-Reply-To: <200902151711.45099.trast@student.ethz.ch> ``` On Sun, Feb 15, 2009 at 05:11:40PM +0100, Thomas Rast wrote: > > > + MSG=3 test_must_fail git notes edit > > > > test_must_fail is a shell function, and we have precedence 2d60615 (tests: > > Avoid single-shot environment export for shell function invocation, > > 2009-01-26) to avoid this construct. > > Bah. I really have a bad memory, don't I? > > Is there a "bare minimum" POSIX shell that I can run the tests under, > to save myself such embarassment in the future? Using "dash" will catch bash-isms, and is pretty commonly available, I think. But this behavior, IIRC, happens on FreeBSD's /bin/sh, which is derived from "ash" (so is "dash", but I they have long since diverged). There are even more quirks on more exotic systems, I expect, though I have to admit that I gave up on Solaris and just started using bash there. :) So I don't think there is a catch-all shell that will help you, but some "ash" variant is probably your best bet. -Peff ``` ## Thomas Rast, 2009-02-15 22:07 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <200902152307.14571.trast@student.ethz.ch> URL: https://gitlist.dev/e/200902152307.14571.trast%40student.ethz.ch In-Reply-To: <20090215181818.GA2291@coredump.intra.peff.net> ``` Jeff King wrote: > Using "dash" will catch bash-isms, and is pretty commonly available, I > think. But this behavior, IIRC, happens on FreeBSD's /bin/sh, which is > derived from "ash" (so is "dash", but I they have long since diverged). Thanks, that worked well enough. It's not in the opensuse repos, but hey, they track it with git, so it must be good ;-) > So I don't think there is a catch-all shell that will help you, but some > "ash" variant is probably your best bet. I tried plain "ash" (which is in the opensuse repos) but that falls down due to lack of 'cd -P'. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Thomas Rast, 2009-02-17 08:44 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <200902170944.08827.trast@student.ethz.ch> URL: https://gitlist.dev/e/200902170944.08827.trast%40student.ethz.ch In-Reply-To: <7vvdrcnnl0.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > Thomas Rast writes: > > > + MSG=3 test_must_fail git notes edit > > test_must_fail is a shell function, and we have precedence 2d60615 (tests: > Avoid single-shot environment export for shell function invocation, > 2009-01-26) to avoid this construct. I see you took this into the pu branch (currently as 891840b). I assumed you would simply drop it, given the incompatibility? I'd rather have the slight chance of missing a segfault while git is trying to execute a shell script (what are the odds that it only does so for git-notes?) than be killed by an angry mob of ash users ;-) -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Thomas Rast, 2009-02-17 08:46 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <200902170946.52093.trast@student.ethz.ch> URL: https://gitlist.dev/e/200902170946.52093.trast%40student.ethz.ch In-Reply-To: <200902170944.08827.trast@student.ethz.ch> ``` Thomas Rast wrote: > Junio C Hamano wrote: > > Thomas Rast writes: > > > > > + MSG=3 test_must_fail git notes edit > > > > test_must_fail is a shell function, and we have precedence 2d60615 (tests: > > Avoid single-shot environment export for shell function invocation, > > 2009-01-26) to avoid this construct. > > I see you took this into the pu branch (currently as 891840b). I > assumed you would simply drop it, given the incompatibility? I'd > rather have the slight chance of missing a segfault while git is > trying to execute a shell script (what are the odds that it only does > so for git-notes?) than be killed by an angry mob of ash users ;-) Oh, you fixed it up locally, and the next one in the series too. Thanks, and sorry for the noise! -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Mike Ralphson, 2009-02-17 09:29 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: URL: https://gitlist.dev/e/e2b179460902170129s7ae613cehe237619be5e84936%40mail.gmail.com In-Reply-To: <20090215181818.GA2291@coredump.intra.peff.net> ``` 2009/2/15 Jeff King : > On Sun, Feb 15, 2009 at 05:11:40PM +0100, Thomas Rast wrote: >> Is there a "bare minimum" POSIX shell that I can run the tests under, >> to save myself such embarassment in the future? > > Using "dash" will catch bash-isms, and is pretty commonly available, I > think. But this behavior, IIRC, happens on FreeBSD's /bin/sh, which is > derived from "ash" (so is "dash", but I they have long since diverged). > > There are even more quirks on more exotic systems, I expect, though I > have to admit that I gave up on Solaris and just started using bash > there. :) > > So I don't think there is a catch-all shell that will help you, but some > "ash" variant is probably your best bet. posh? http://packages.debian.org/lenny/posh I've heard that if you unset POSIXLY_CORRECT it just sits there and hums until you set it again. 8-) Mike ``` ## Jeff King, 2009-02-17 16:34 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <20090217163413.GB31297@sigill.intra.peff.net> URL: https://gitlist.dev/e/20090217163413.GB31297%40sigill.intra.peff.net In-Reply-To: ``` On Tue, Feb 17, 2009 at 09:29:57AM +0000, Mike Ralphson wrote: > posh? http://packages.debian.org/lenny/posh > > I've heard that if you unset POSIXLY_CORRECT it just sits there and > hums until you set it again. 8-) Hmm. I tried "make SHELL_PATH=/bin/posh test", and posh segfaulted during t0005. So I don't think it's quite ready for mainstream use. ;) -Peff ``` ## Brandon Casey, 2009-02-17 16:56 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: URL: https://gitlist.dev/e/tJ6pp2ZnBjKVBLynGfYewBaGmSehbwZNLOShEvpXmtkD3IMXRugEKg%40cipher.nrlssc.navy.mil In-Reply-To: <200902170946.52093.trast@student.ethz.ch> ``` Thomas Rast wrote: > Thomas Rast wrote: >> Junio C Hamano wrote: >>> Thomas Rast writes: >>> >>>> + MSG=3 test_must_fail git notes edit >>> test_must_fail is a shell function, and we have precedence 2d60615 (tests: >>> Avoid single-shot environment export for shell function invocation, >>> 2009-01-26) to avoid this construct. >> I see you took this into the pu branch (currently as 891840b). I >> assumed you would simply drop it, given the incompatibility? I'd >> rather have the slight chance of missing a segfault while git is >> trying to execute a shell script (what are the odds that it only does >> so for git-notes?) than be killed by an angry mob of ash users ;-) > > Oh, you fixed it up locally, and the next one in the series too. > Thanks, and sorry for the noise! Yeah, but what's this, introduced by 94859732? t/t3301-notes.sh line 40: test_expect_success 'refusing to edit in refs/heads/' ' (MSG=1 GIT_NOTES_REF=refs/heads/bogus && export MSG= GIT_NOTES_REF=refs/heads/bogus && test_must_fail git notes edit) ' test_expect_success 'refusing to edit in refs/remotes/' ' (MSG=1 GIT_NOTES_REF=refs/remotes/bogus && export MSG= GIT_NOTES_REF=refs/heads/bogus && test_must_fail git notes edit) ' Notice the 'export' lines. Comparing it to Thomas's v2 5/5 patch, it looks like the first line which sets the variables is correct and the export line should just be 'export MSG GIT_NOTES_REF' in both tests. -brandon ``` ## Mike Ralphson, 2009-02-17 18:00 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: URL: https://gitlist.dev/e/e2b179460902171000s605675dct1f499a9425c3ebb2%40mail.gmail.com In-Reply-To: <20090217163413.GB31297@sigill.intra.peff.net> ``` 2009/2/17 Jeff King : > On Tue, Feb 17, 2009 at 09:29:57AM +0000, Mike Ralphson wrote: >> posh? http://packages.debian.org/lenny/posh > > Hmm. I tried "make SHELL_PATH=/bin/posh test", and posh segfaulted during > t0005. So I don't think it's quite ready for mainstream use. ;) Works ok for me as far as t3404-rebase-interactive on next, though what I actually grabbed in the end was 0.6.16[1], which claims to fix a segfault bug[2], but then it also claims to be 0.6.12 as well, so who knows? 8-) Maybe a Debian user would like to report a repeat of [3]? Mike [1] http://packages.debian.org/sid/posh [2] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=515278 [3] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=397596 ``` ## Jeff King, 2009-02-17 20:27 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <20090217202731.GA16586@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090217202731.GA16586%40coredump.intra.peff.net In-Reply-To: ``` On Tue, Feb 17, 2009 at 06:00:17PM +0000, Mike Ralphson wrote: > > Hmm. I tried "make SHELL_PATH=/bin/posh test", and posh segfaulted during > > t0005. So I don't think it's quite ready for mainstream use. ;) > > Works ok for me as far as t3404-rebase-interactive on next, though > what I actually grabbed in the end was 0.6.16[1], which claims to fix > a segfault bug[2], but then it also claims to be 0.6.12 as well, so > who knows? 8-) > > Maybe a Debian user would like to report a repeat of [3]? Hmm. It sort of fixes the segfault bug. With posh 0.6.16, I can do: $ posh -x $ ls + ls ... but: $ posh -x t0005-signals.sh Segmentation fault OK, maybe it doesn't like non-interactive shells. Let's try sourcing the script: $ posh -x $ . t0005-signals.sh + . t0005-signals.sh posh: .: t0005-signals.sh: not found OK, maybe it is looking in the PATH? $ PATH=$PATH:. Segmentation fault Oops. How about a new shell with a more exact path? $ posh -x $ . ./t0005-signals.sh Segmentation fault Oops. So I think there are some serious problems (at least with -x). Of course it's hard to diagnose the non "-x" segfault in t0005 without "-x". ;) But this is just unproductive complaining to do it on the git list (I'm just amazed at how frequently it is segfaulting). I'll go file some Debian bug reports. -Peff ``` ## Junio C Hamano, 2009-02-17 22:20 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <7vr61w3dug.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr61w3dug.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Brandon Casey writes: > Notice the 'export' lines. Comparing it to Thomas's v2 5/5 patch, it > looks like the first line which sets the variables is correct and the > export line should just be 'export MSG GIT_NOTES_REF' in both tests. Thanks for catching my late-night typos. Some shells do not like "export var=val", and the right way to write these is to do an usual assignment and then export just variable names. We need to fix the following: $ git grep -n -e 'export .*=' --and --not -e '-export' pu -- t/ pu:t/t3301-notes.sh:42: export MSG= GIT_NOTES_REF=refs/heads/bogus && pu:t/t3301-notes.sh:48: export MSG= GIT_NOTES_REF=refs/heads/bogus && pu:t/t3302-notes-index-expensive.sh:32: export GIT_INDEX_FILE=.git/temp; pu:t/t3302-notes-index-expensive.sh:66: export GIT_NOTES_REF=non-existing pu:t/t9301-fast-export.sh:188:export GIT_AUTHOR_NAME='A U Thor' pu:t/t9301-fast-export.sh:189:export GIT_COMMITTER_NAME='C O Mitter' pu:t/test-lib.sh:101: export GIT_TEST_LONG=t; shift ;; ``` ## Jay Soffian, 2009-02-17 22:53 Subject: [PATCH] test suite: correct export var=val usage Message-ID: <1234911188-36941-1-git-send-email-jaysoffian@gmail.com> URL: https://gitlist.dev/e/1234911188-36941-1-git-send-email-jaysoffian%40gmail.com In-Reply-To: <7vr61w3dug.fsf@gitster.siamese.dyndns.org> ``` Some shells do not like "export var=val", and the right way to write these is to do an usual assignment and then export just variable names. --- t/t3301-notes.sh | 6 +++--- t/t3302-notes-index-expensive.sh | 4 ++-- t/t9301-fast-export.sh | 4 ++-- t/test-lib.sh | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 2321316..7ea98f2 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -38,14 +38,14 @@ test_expect_success 'need valid notes ref' ' ' test_expect_success 'refusing to edit in refs/heads/' ' - (MSG=1 GIT_NOTES_REF=refs/heads/bogus && - export MSG= GIT_NOTES_REF=refs/heads/bogus && + (MSG=1 GIT_NOTES_REF=refs/heads/bogus + && export MSG GIT_NOTES_REF && test_must_fail git notes edit) ' test_expect_success 'refusing to edit in refs/remotes/' ' (MSG=1 GIT_NOTES_REF=refs/remotes/bogus && - export MSG= GIT_NOTES_REF=refs/heads/bogus && + export MSG GIT_NOTES_REF && test_must_fail git notes edit) ' diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh index 00d27bf..0ef3e95 100755 --- a/t/t3302-notes-index-expensive.sh +++ b/t/t3302-notes-index-expensive.sh @@ -29,7 +29,7 @@ create_repo () { done && git update-ref refs/heads/master $commit && { - export GIT_INDEX_FILE=.git/temp; + GIT_INDEX_FILE=.git/temp; export GIT_INDEX_FILE; git rev-list HEAD | cat -n | sed "s/^[ ][ ]*/ /g" | while read nr sha1; do blob=$(echo note $nr | git hash-object -w --stdin) && @@ -63,7 +63,7 @@ cat > time_notes << \EOF while [ $i -lt $2 ]; do case $1 in no-notes) - export GIT_NOTES_REF=non-existing + GIT_NOTES_REF=non-existing; export GIT_NOTES_REF ;; notes) unset GIT_NOTES_REF diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh index 9985721..86c3760 100755 --- a/t/t9301-fast-export.sh +++ b/t/t9301-fast-export.sh @@ -185,8 +185,8 @@ test_expect_success 'submodule fast-export | fast-import' ' ' -export GIT_AUTHOR_NAME='A U Thor' -export GIT_COMMITTER_NAME='C O Mitter' +GIT_AUTHOR_NAME='A U Thor'; export GIT_AUTHOR_NAME +GIT_COMMITTER_NAME='C O Mitter'; export GIT_COMMITTER_NAME test_expect_success 'setup copies' ' diff --git a/t/test-lib.sh b/t/test-lib.sh index 2021446..7a847ec 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -98,7 +98,7 @@ do -i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate) immediate=t; shift ;; -l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests) - export GIT_TEST_LONG=t; shift ;; + GIT_TEST_LONG=t; export GIT_TEST_LONG; shift ;; -h|--h|--he|--hel|--help) help=t; shift ;; -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose) -- 1.6.2.rc1.218.g1b4fab ``` ## Jay Soffian, 2009-02-17 22:54 Subject: [PATCH] test suite: correct export var=val usage Message-ID: <1234911240-37028-1-git-send-email-jaysoffian@gmail.com> URL: https://gitlist.dev/e/1234911240-37028-1-git-send-email-jaysoffian%40gmail.com In-Reply-To: <7vr61w3dug.fsf@gitster.siamese.dyndns.org> ``` Some shells do not like "export var=val", and the right way to write these is to do an usual assignment and then export just variable names. Signed-off-by: Jay Soffian --- This time with SOB. Sorry. t/t3301-notes.sh | 6 +++--- t/t3302-notes-index-expensive.sh | 4 ++-- t/t9301-fast-export.sh | 4 ++-- t/test-lib.sh | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 2321316..7ea98f2 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -38,14 +38,14 @@ test_expect_success 'need valid notes ref' ' ' test_expect_success 'refusing to edit in refs/heads/' ' - (MSG=1 GIT_NOTES_REF=refs/heads/bogus && - export MSG= GIT_NOTES_REF=refs/heads/bogus && + (MSG=1 GIT_NOTES_REF=refs/heads/bogus + && export MSG GIT_NOTES_REF && test_must_fail git notes edit) ' test_expect_success 'refusing to edit in refs/remotes/' ' (MSG=1 GIT_NOTES_REF=refs/remotes/bogus && - export MSG= GIT_NOTES_REF=refs/heads/bogus && + export MSG GIT_NOTES_REF && test_must_fail git notes edit) ' diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh index 00d27bf..0ef3e95 100755 --- a/t/t3302-notes-index-expensive.sh +++ b/t/t3302-notes-index-expensive.sh @@ -29,7 +29,7 @@ create_repo () { done && git update-ref refs/heads/master $commit && { - export GIT_INDEX_FILE=.git/temp; + GIT_INDEX_FILE=.git/temp; export GIT_INDEX_FILE; git rev-list HEAD | cat -n | sed "s/^[ ][ ]*/ /g" | while read nr sha1; do blob=$(echo note $nr | git hash-object -w --stdin) && @@ -63,7 +63,7 @@ cat > time_notes << \EOF while [ $i -lt $2 ]; do case $1 in no-notes) - export GIT_NOTES_REF=non-existing + GIT_NOTES_REF=non-existing; export GIT_NOTES_REF ;; notes) unset GIT_NOTES_REF diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh index 9985721..86c3760 100755 --- a/t/t9301-fast-export.sh +++ b/t/t9301-fast-export.sh @@ -185,8 +185,8 @@ test_expect_success 'submodule fast-export | fast-import' ' ' -export GIT_AUTHOR_NAME='A U Thor' -export GIT_COMMITTER_NAME='C O Mitter' +GIT_AUTHOR_NAME='A U Thor'; export GIT_AUTHOR_NAME +GIT_COMMITTER_NAME='C O Mitter'; export GIT_COMMITTER_NAME test_expect_success 'setup copies' ' diff --git a/t/test-lib.sh b/t/test-lib.sh index 2021446..7a847ec 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -98,7 +98,7 @@ do -i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate) immediate=t; shift ;; -l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests) - export GIT_TEST_LONG=t; shift ;; + GIT_TEST_LONG=t; export GIT_TEST_LONG; shift ;; -h|--h|--he|--hel|--help) help=t; shift ;; -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose) -- 1.6.2.rc1.218.g1b4fab ``` ## Jay Soffian, 2009-02-17 22:57 Subject: [PATCH] test suite: correct export var=val usage Message-ID: <1234911457-37188-1-git-send-email-jaysoffian@gmail.com> URL: https://gitlist.dev/e/1234911457-37188-1-git-send-email-jaysoffian%40gmail.com In-Reply-To: <7vr61w3dug.fsf@gitster.siamese.dyndns.org> ``` Some shells do not like "export var=val", and the right way to write these is to do an usual assignment and then export just variable names. Signed-off-by: Jay Soffian --- Argh. Last time. How come I only notice typos after they go to the list? The prior version had a '&&' on the wrong line. I think it wouldn't be a problem because it's in a (...) subshell but this way is better. t/t3301-notes.sh | 4 ++-- t/t3302-notes-index-expensive.sh | 4 ++-- t/t9301-fast-export.sh | 4 ++-- t/test-lib.sh | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 2321316..18c4eb6 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -39,13 +39,13 @@ test_expect_success 'need valid notes ref' ' test_expect_success 'refusing to edit in refs/heads/' ' (MSG=1 GIT_NOTES_REF=refs/heads/bogus && - export MSG= GIT_NOTES_REF=refs/heads/bogus && + export MSG GIT_NOTES_REF && test_must_fail git notes edit) ' test_expect_success 'refusing to edit in refs/remotes/' ' (MSG=1 GIT_NOTES_REF=refs/remotes/bogus && - export MSG= GIT_NOTES_REF=refs/heads/bogus && + export MSG GIT_NOTES_REF && test_must_fail git notes edit) ' diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh index 00d27bf..0ef3e95 100755 --- a/t/t3302-notes-index-expensive.sh +++ b/t/t3302-notes-index-expensive.sh @@ -29,7 +29,7 @@ create_repo () { done && git update-ref refs/heads/master $commit && { - export GIT_INDEX_FILE=.git/temp; + GIT_INDEX_FILE=.git/temp; export GIT_INDEX_FILE; git rev-list HEAD | cat -n | sed "s/^[ ][ ]*/ /g" | while read nr sha1; do blob=$(echo note $nr | git hash-object -w --stdin) && @@ -63,7 +63,7 @@ cat > time_notes << \EOF while [ $i -lt $2 ]; do case $1 in no-notes) - export GIT_NOTES_REF=non-existing + GIT_NOTES_REF=non-existing; export GIT_NOTES_REF ;; notes) unset GIT_NOTES_REF diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh index 9985721..86c3760 100755 --- a/t/t9301-fast-export.sh +++ b/t/t9301-fast-export.sh @@ -185,8 +185,8 @@ test_expect_success 'submodule fast-export | fast-import' ' ' -export GIT_AUTHOR_NAME='A U Thor' -export GIT_COMMITTER_NAME='C O Mitter' +GIT_AUTHOR_NAME='A U Thor'; export GIT_AUTHOR_NAME +GIT_COMMITTER_NAME='C O Mitter'; export GIT_COMMITTER_NAME test_expect_success 'setup copies' ' diff --git a/t/test-lib.sh b/t/test-lib.sh index 2021446..7a847ec 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -98,7 +98,7 @@ do -i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate) immediate=t; shift ;; -l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests) - export GIT_TEST_LONG=t; shift ;; + GIT_TEST_LONG=t; export GIT_TEST_LONG; shift ;; -h|--h|--he|--hel|--help) help=t; shift ;; -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose) -- 1.6.2.rc1.218.g1b4fab ``` ## Johannes Schindelin, 2009-02-17 23:47 Subject: Re: [PATCH] test suite: correct export var=val usage Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902180046290.10279%40pacific.mpi-cbg.de In-Reply-To: <1234911188-36941-1-git-send-email-jaysoffian@gmail.com> ``` Hi, On Tue, 17 Feb 2009, Jay Soffian wrote: > + (MSG=1 GIT_NOTES_REF=refs/heads/bogus > + && export MSG GIT_NOTES_REF && Hmm. Why does the "&&" at the start of a line strike me as odd. Ah, yes, it is because no code around it uses that style. *grins* Ciao, Dscho ``` ## Jay Soffian, 2009-02-18 00:37 Subject: Re: [PATCH] test suite: correct export var=val usage Message-ID: <76718490902171637j55f97a7fg2a0ead846b88d3e6@mail.gmail.com> URL: https://gitlist.dev/e/76718490902171637j55f97a7fg2a0ead846b88d3e6%40mail.gmail.com In-Reply-To: ``` On Tue, Feb 17, 2009 at 6:47 PM, Johannes Schindelin wrote: > Hi, > > On Tue, 17 Feb 2009, Jay Soffian wrote: > >> + (MSG=1 GIT_NOTES_REF=refs/heads/bogus >> + && export MSG GIT_NOTES_REF && > > Hmm. Why does the "&&" at the start of a line strike me as odd. Ah, yes, > it is because no code around it uses that style. > > *grins* Sometimes authors catch their own mistakes, but you have to slow down on the reply button to notice. It's not like I didn't mail out the correction less than a minute afterwards. *double-grins* And in case your curious how that happened, it was because I had a single line, realized it was too long, and hit return in the wrong place. Unfortunately I didn't catch my mistake till after I mailed it out. So there. (sticking tongue out at Dscho) j. ``` ## Jeff King, 2009-02-18 06:41 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <20090218064121.GA16611@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090218064121.GA16611%40coredump.intra.peff.net In-Reply-To: <20090217202731.GA16586@coredump.intra.peff.net> ``` On Tue, Feb 17, 2009 at 03:27:31PM -0500, Jeff King wrote: > But this is just unproductive complaining to do it on the git list (I'm > just amazed at how frequently it is segfaulting). I'll go file some > Debian bug reports. The maintainer was very responsive, fixing my bugs in about an hour after they were reported. I can now get as far as t3404, which seems to have problems due to rebase-interactive using parentheses, "-a", and "-o" with "test". I'm not sure how much time it is worth spending on this; I'm not sure anyone actually uses a shell as featureless as posh in real life (though I suppose it could eventually become the Debian shell). -Peff ``` ## Wincent Colaiuta, 2009-02-18 10:06 Subject: Re: [PATCH] test suite: correct export var=val usage Message-ID: <0E74A2BF-1E59-4D82-A750-380697667F10@wincent.com> URL: https://gitlist.dev/e/0E74A2BF-1E59-4D82-A750-380697667F10%40wincent.com In-Reply-To: <1234911457-37188-1-git-send-email-jaysoffian@gmail.com> ``` El 17/2/2009, a las 23:57, Jay Soffian escribió: > test_expect_success 'refusing to edit in refs/heads/' ' > (MSG=1 GIT_NOTES_REF=refs/heads/bogus && > - export MSG= GIT_NOTES_REF=refs/heads/bogus && > + export MSG GIT_NOTES_REF && > test_must_fail git notes edit) > ' > > test_expect_success 'refusing to edit in refs/remotes/' ' > (MSG=1 GIT_NOTES_REF=refs/remotes/bogus && > - export MSG= GIT_NOTES_REF=refs/heads/bogus && > + export MSG GIT_NOTES_REF && > test_must_fail git notes edit) > ' Perhaps my eyes are playing tricks on me but I see the original version setting MSG to an empty string and exporting it, and your version setting MSG to "1" and exporting it. So which one is wrong? The original or yours? Cheers, Wincent ``` ## Johannes Schindelin, 2009-02-18 10:14 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902181113210.6274%40intel-tinevez-2-302 In-Reply-To: <20090218064121.GA16611@coredump.intra.peff.net> ``` Hi, On Wed, 18 Feb 2009, Jeff King wrote: > I can now get as far as t3404, which seems to have problems due to > rebase-interactive using parentheses, "-a", and "-o" with "test". The parentheses will be fixed soon, but "-a" and "-o" really are necessary. Ciao, Dscho ``` ## Jeff King, 2009-02-18 10:16 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <20090218101615.GA23035@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090218101615.GA23035%40coredump.intra.peff.net In-Reply-To: ``` On Wed, Feb 18, 2009 at 11:14:10AM +0100, Johannes Schindelin wrote: > > I can now get as far as t3404, which seems to have problems due to > > rebase-interactive using parentheses, "-a", and "-o" with "test". > > The parentheses will be fixed soon, but "-a" and "-o" really are > necessary. Really? Are they not easily replaced with -test cond1 -a cond2 -o cond3 +test cond1 && test cond2 || test cond3 ? The trickier part is the grouping, but that is what the parentheses are doing. -Peff ``` ## Johannes Schindelin, 2009-02-18 11:53 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902181251160.6274%40intel-tinevez-2-302 In-Reply-To: <20090218101615.GA23035@coredump.intra.peff.net> ``` Hi, On Wed, 18 Feb 2009, Jeff King wrote: > On Wed, Feb 18, 2009 at 11:14:10AM +0100, Johannes Schindelin wrote: > > > > I can now get as far as t3404, which seems to have problems due to > > > rebase-interactive using parentheses, "-a", and "-o" with "test". > > > > The parentheses will be fixed soon, but "-a" and "-o" really are > > necessary. > > Really? Are they not easily replaced with > > -test cond1 -a cond2 -o cond3 > +test cond1 && test cond2 || test cond3 ...which is substantially harder to read. > ? The trickier part is the grouping, but that is what the parentheses > are doing. I got rid of that code about two weeks ago. The upcoming rebase -i -p series is unfortunately not in a good shape at the moment; intermediate stages fail the tests, so I will have to clean up a bit. But at the moment, real world is taking my time, reducing my Git time budget substantially. On the positive side (well, half of it), I fiddle with msysGit these days, as have to use Windows again. Ciao, Dscho ``` ## Jay Soffian, 2009-02-18 13:19 Subject: Re: [PATCH] test suite: correct export var=val usage Message-ID: <76718490902180519s3b5f4469m391aa34456059d37@mail.gmail.com> URL: https://gitlist.dev/e/76718490902180519s3b5f4469m391aa34456059d37%40mail.gmail.com In-Reply-To: <0E74A2BF-1E59-4D82-A750-380697667F10@wincent.com> ``` On Wed, Feb 18, 2009 at 5:06 AM, Wincent Colaiuta wrote: > Perhaps my eyes are playing tricks on me but I see the original version > setting MSG to an empty string and exporting it, and your version setting > MSG to "1" and exporting it. So which one is wrong? The original or yours? http://thread.gmane.org/gmane.comp.version-control.git/109897/focus=110462 j. ``` ## Jay Soffian, 2009-02-18 13:29 Subject: Re: [PATCH] test suite: correct export var=val usage Message-ID: <76718490902180529w7520ba64kb7d0d6b284b406bd@mail.gmail.com> URL: https://gitlist.dev/e/76718490902180529w7520ba64kb7d0d6b284b406bd%40mail.gmail.com In-Reply-To: <76718490902180519s3b5f4469m391aa34456059d37@mail.gmail.com> ``` On Wed, Feb 18, 2009 at 8:19 AM, Jay Soffian wrote: > On Wed, Feb 18, 2009 at 5:06 AM, Wincent Colaiuta wrote: >> Perhaps my eyes are playing tricks on me but I see the original version >> setting MSG to an empty string and exporting it, and your version setting >> MSG to "1" and exporting it. So which one is wrong? The original or yours? > > http://thread.gmane.org/gmane.comp.version-control.git/109897/focus=110462 Rather, http://thread.gmane.org/gmane.comp.version-control.git/109897/focus=110457 (Cursed frame interface.) (The original is wrong.) j. ``` ## Johannes Schindelin, 2009-02-18 13:34 Subject: Re: [PATCH] test suite: correct export var=val usage Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902181433550.6274%40intel-tinevez-2-302 In-Reply-To: <76718490902180529w7520ba64kb7d0d6b284b406bd@mail.gmail.com> ``` Hi, On Wed, 18 Feb 2009, Jay Soffian wrote: > On Wed, Feb 18, 2009 at 8:19 AM, Jay Soffian wrote: > > On Wed, Feb 18, 2009 at 5:06 AM, Wincent Colaiuta wrote: > >> Perhaps my eyes are playing tricks on me but I see the original version > >> setting MSG to an empty string and exporting it, and your version setting > >> MSG to "1" and exporting it. So which one is wrong? The original or yours? > > > > http://thread.gmane.org/gmane.comp.version-control.git/109897/focus=110462 > > Rather, http://thread.gmane.org/gmane.comp.version-control.git/109897/focus=110457 > > (Cursed frame interface.) > > (The original is wrong.) Maybe the time would be better spent paraphrasing the gist of the thread, instead of cursing? Besides, where du you find a curses interface to gmane? :-) Ciao, Dscho ``` ## Wincent Colaiuta, 2009-02-18 16:56 Subject: Re: [PATCH] test suite: correct export var=val usage Message-ID: URL: https://gitlist.dev/e/F21AE3A2-480B-4311-81E7-32B75B49A356%40wincent.com In-Reply-To: <76718490902180529w7520ba64kb7d0d6b284b406bd@mail.gmail.com> ``` El 18/2/2009, a las 14:29, Jay Soffian escribió: > On Wed, Feb 18, 2009 at 8:19 AM, Jay Soffian > wrote: >> On Wed, Feb 18, 2009 at 5:06 AM, Wincent Colaiuta >> wrote: >>> Perhaps my eyes are playing tricks on me but I see the original >>> version >>> setting MSG to an empty string and exporting it, and your version >>> setting >>> MSG to "1" and exporting it. So which one is wrong? The original >>> or yours? >> >> http://thread.gmane.org/gmane.comp.version-control.git/109897/focus=110462 > > Rather, http://thread.gmane.org/gmane.comp.version-control.git/109897/focus=110457 > > (Cursed frame interface.) > > (The original is wrong.) I'm definitely blind then, or perhaps I didn't explain myself. At least, I saw nothing in the message you linked that answers my question. Let me try again. - Prior to your patch, in the two hunks I quoted we set MSG to an empty string and exported it - After your patch, the hunks now set MSG to "1" (not the same string) and export it In other words, you not only changed the _style_ from "assign and export in a single step" to "assign and then export as two separate steps"; you also changed _what_ gets exported in two of those hunks. So my question really was, was this intended? Cheers, Wincent ``` ## Thomas Rast, 2009-02-18 17:05 Subject: Re: [PATCH] test suite: correct export var=val usage Message-ID: <200902181805.44145.trast@student.ethz.ch> URL: https://gitlist.dev/e/200902181805.44145.trast%40student.ethz.ch In-Reply-To: ``` Wincent Colaiuta wrote: > I'm definitely blind then, or perhaps I didn't explain myself. At > least, I saw nothing in the message you linked that answers my > question. Let me try again. > > - Prior to your patch, in the two hunks I quoted we set MSG to an > empty string and exported it > > - After your patch, the hunks now set MSG to "1" (not the same string) > and export it > > In other words, you not only changed the _style_ from "assign and > export in a single step" to "assign and then export as two separate > steps"; you also changed _what_ gets exported in two of those hunks. Actually the original version was http://article.gmane.org/gmane.comp.version-control.git/109920 which was subsequently fixed by Junio to not use a single-shot export. Then Brandon noticed that during the fix-up, the export line suddenly started exporting values itself, and different ones at that. So while Jay's patch indeed changes the semantics, the net result is what was intended all along, except it's now (hopefully) compatible with more shells. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Jay Soffian, 2009-02-18 20:55 Subject: [PATCH v2] test suite: correct export var=val usage Message-ID: <1234990515-1636-1-git-send-email-jaysoffian@gmail.com> URL: https://gitlist.dev/e/1234990515-1636-1-git-send-email-jaysoffian%40gmail.com In-Reply-To: <7vr61w3dug.fsf@gitster.siamese.dyndns.org> ``` Some shells do not like "export var=val", and the right way to write these is to do an usual assignment and then export just variable names. Also corrects a typo in the tests added by 94859732 and noticed by Brandon Casey (gmane 110412): test_expect_success 'refusing to edit in refs/heads/' ' (MSG=1 GIT_NOTES_REF=refs/heads/bogus && export MSG= GIT_NOTES_REF=refs/heads/bogus && test_must_fail git notes edit) ' test_expect_success 'refusing to edit in refs/remotes/' ' (MSG=1 GIT_NOTES_REF=refs/remotes/bogus && export MSG= GIT_NOTES_REF=refs/heads/bogus && test_must_fail git notes edit) ' Notice the 'export' lines. Comparing it to Thomas's v2 5/5 patch, it looks like the first line which sets the variables is correct and the export line should just be 'export MSG GIT_NOTES_REF' in both tests. Signed-off-by: Jay Soffian --- This time with a better (I hope...) commit message to clarify that we are fixing a typo and not introducing one. t/t3301-notes.sh | 4 ++-- t/t3302-notes-index-expensive.sh | 4 ++-- t/t9301-fast-export.sh | 4 ++-- t/test-lib.sh | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 2321316..18c4eb6 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -39,13 +39,13 @@ test_expect_success 'need valid notes ref' ' test_expect_success 'refusing to edit in refs/heads/' ' (MSG=1 GIT_NOTES_REF=refs/heads/bogus && - export MSG= GIT_NOTES_REF=refs/heads/bogus && + export MSG GIT_NOTES_REF && test_must_fail git notes edit) ' test_expect_success 'refusing to edit in refs/remotes/' ' (MSG=1 GIT_NOTES_REF=refs/remotes/bogus && - export MSG= GIT_NOTES_REF=refs/heads/bogus && + export MSG GIT_NOTES_REF && test_must_fail git notes edit) ' diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh index 00d27bf..0ef3e95 100755 --- a/t/t3302-notes-index-expensive.sh +++ b/t/t3302-notes-index-expensive.sh @@ -29,7 +29,7 @@ create_repo () { done && git update-ref refs/heads/master $commit && { - export GIT_INDEX_FILE=.git/temp; + GIT_INDEX_FILE=.git/temp; export GIT_INDEX_FILE; git rev-list HEAD | cat -n | sed "s/^[ ][ ]*/ /g" | while read nr sha1; do blob=$(echo note $nr | git hash-object -w --stdin) && @@ -63,7 +63,7 @@ cat > time_notes << \EOF while [ $i -lt $2 ]; do case $1 in no-notes) - export GIT_NOTES_REF=non-existing + GIT_NOTES_REF=non-existing; export GIT_NOTES_REF ;; notes) unset GIT_NOTES_REF diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh index 9985721..86c3760 100755 --- a/t/t9301-fast-export.sh +++ b/t/t9301-fast-export.sh @@ -185,8 +185,8 @@ test_expect_success 'submodule fast-export | fast-import' ' ' -export GIT_AUTHOR_NAME='A U Thor' -export GIT_COMMITTER_NAME='C O Mitter' +GIT_AUTHOR_NAME='A U Thor'; export GIT_AUTHOR_NAME +GIT_COMMITTER_NAME='C O Mitter'; export GIT_COMMITTER_NAME test_expect_success 'setup copies' ' diff --git a/t/test-lib.sh b/t/test-lib.sh index 2021446..7a847ec 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -98,7 +98,7 @@ do -i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate) immediate=t; shift ;; -l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests) - export GIT_TEST_LONG=t; shift ;; + GIT_TEST_LONG=t; export GIT_TEST_LONG; shift ;; -h|--h|--he|--hel|--help) help=t; shift ;; -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose) -- 1.6.2.rc1.218.g1b4fab ``` ## Jeff King, 2009-02-19 00:37 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <20090219003727.GA25808@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090219003727.GA25808%40coredump.intra.peff.net In-Reply-To: ``` On Wed, Feb 18, 2009 at 12:53:37PM +0100, Johannes Schindelin wrote: > > Really? Are they not easily replaced with > > > > -test cond1 -a cond2 -o cond3 > > +test cond1 && test cond2 || test cond3 > > ...which is substantially harder to read. I don't agree that it is harder to read, but that is beside the point. What is important is whether or not the construct is portable enough to meet git's standards. "-a" and "-o" are XSI extensions to POSIX, which is usually a sign that there may be problems. However, besides posh (which at this point I think can be considered a compliance-testing shell and not an actual shell in use), I haven't heard of problems in practice. Even FreeBSD's ash derivative supports it (along with parentheses). So I don't think it needs to be changed (which is what I said in my original message). But I also think saying "-a and -o are necessary" is not true; they can be replaced if it turns out to be a problem. -Peff ``` ## Johannes Schindelin, 2009-02-19 00:46 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902190142440.10279%40pacific.mpi-cbg.de In-Reply-To: <20090219003727.GA25808@coredump.intra.peff.net> ``` Hi, On Wed, 18 Feb 2009, Jeff King wrote: > On Wed, Feb 18, 2009 at 12:53:37PM +0100, Johannes Schindelin wrote: > > > > Really? Are they not easily replaced with > > > > > > -test cond1 -a cond2 -o cond3 > > > +test cond1 && test cond2 || test cond3 > > > > ...which is substantially harder to read. > > I don't agree that it is harder to read, but that is beside the point. > What is important is whether or not the construct is portable enough to > meet git's standards. > > "-a" and "-o" are XSI extensions to POSIX, which is usually a sign that > there may be problems. However, besides posh (which at this point I > think can be considered a compliance-testing shell and not an actual > shell in use), I haven't heard of problems in practice. Even FreeBSD's > ash derivative supports it (along with parentheses). > > So I don't think it needs to be changed (which is what I said in my > original message). But I also think saying "-a and -o are necessary" is > not true; they can be replaced if it turns out to be a problem. Even if they were in POSIX, I'd suggest not to change the constructs. rebase is _bound_ to be built in. Even if Stephan is not a fan of my recent changes to rebase -i -p (which I will present on this list once they all work as I want them to), I think I can talk him into continuing the sequencer effort: he was not discussing the design in the open, so he should have expected the process to be dragged out. Therefore, any changes to git-rebase--interactive (or for that matter, git-rebase) before the sequencer would be wasted work. Ciao, Dscho ``` ## Jeff King, 2009-02-19 00:46 Subject: Re: [PATCH v2 4/5] t3301: use test_must_fail instead of ! Message-ID: <20090219004633.GC25808@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090219004633.GC25808%40coredump.intra.peff.net In-Reply-To: ``` On Thu, Feb 19, 2009 at 01:46:28AM +0100, Johannes Schindelin wrote: > Even if they were in POSIX, I'd suggest not to change the constructs. > rebase is _bound_ to be built in. > > Even if Stephan is not a fan of my recent changes to rebase -i -p (which I > will present on this list once they all work as I want them to), I think I > can talk him into continuing the sequencer effort: he was not discussing > the design in the open, so he should have expected the process to be > dragged out. Having a sequencer in C would make me even happier. ;) -Peff ```