threads / patch / 17794

patch, 4 partsnotes: use GIT_EDITOR and core.editor over VISUAL/EDITOR

Subject: [PATCH 2/4] notes: use GIT_EDITOR and core.editor over VISUAL/EDITOR

## tl;dr

47 messages between Feb 14, 2009 and Feb 19, 2009. Diffs are folded; open one to read it.

replies: 46people: 8as markdown or json

Thomas Rast· Feb 14, 2009, 19:15 UTC · lore

[PATCH 1/4] notes: only clean up message file when editing

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 <trast@student.ethz.ch>
---
 git-notes.sh |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
Show changes to git-notes.sh +5 −5
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· Feb 14, 2009, 19:15 UTC · re: Thomas Rast · lore

This is the order documented in git-config(1), so we should stick to it.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

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(-)
Show changes to git-notes.sh +2 −1
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· Feb 14, 2009, 19:15 UTC · re: Thomas Rast · lore

[PATCH 3/4] t3301: fix confusing test for valid notes ref

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 <trast@student.ethz.ch>
---

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(-)
Show changes to t/t3301-notes.sh +2 −2
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
Johannes Schindelin· Feb 14, 2009, 19:32 UTC · re: Thomas Rast · lore

Re: [PATCH 3/4] t3301: fix confusing test for valid notes ref

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

Thomas Rast· Feb 14, 2009, 19:15 UTC · re: Thomas Rast · lore

[PATCH 4/4] notes: refuse to edit notes outside refs/notes/

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 <trast@student.ethz.ch>
---
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(-)
Show changes to 2 files +12 −0

git-notes.sh, t/t3301-notes.sh

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· Feb 14, 2009, 19:33 UTC · re: Thomas Rast · lore

Re: [PATCH 4/4] notes: refuse to edit notes outside refs/notes/

Hi,
On Sat, 14 Feb 2009, Thomas Rast wrote:
Show 8 quoted lines
> 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· Feb 14, 2009, 19:56 UTC · re: Johannes Schindelin · lore

Re: [PATCH 4/4] notes: refuse to edit notes outside refs/notes/

Johannes Schindelin wrote:
Show 10 quoted lines
> 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· Feb 14, 2009, 20:23 UTC · re: Thomas Rast · lore

[PATCH v2 1/5] notes: only clean up message file when editing

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 <trast@student.ethz.ch>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
Same as v1.
 git-notes.sh |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
Show changes to git-notes.sh +5 −5
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
Junio C Hamano· Feb 15, 2009, 07:54 UTC · re: Thomas Rast · lore

Re: [PATCH v2 1/5] notes: only clean up message file when editing

Thomas Rast <trast@student.ethz.ch> writes:
Show 5 quoted lines
> +	MESSAGE="$GIT_DIR"/new-notes-$COMMIT
> +	trap '
> +		test -f "$MESSAGE" && rm "$MESSAGE"
> +	' 0
> +
Hmph, why not unconditionally rm -f "$MESSAGE" without test?
Thomas Rast· Feb 14, 2009, 20:23 UTC · re: Thomas Rast · lore

[PATCH v2 2/5] notes: use GIT_EDITOR and core.editor over VISUAL/EDITOR

This is the order documented in git-config(1), so we should stick to it.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
Same as v1.
 git-notes.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to git-notes.sh +2 −1
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· Feb 14, 2009, 20:23 UTC · re: Thomas Rast · lore

[PATCH v2 3/5] t3301: fix confusing quoting in test for valid notes ref

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 <trast@student.ethz.ch>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
Dropped the removal of MSG=2.
 t/t3301-notes.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t3301-notes.sh +2 −2
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· Feb 14, 2009, 20:23 UTC · re: Thomas Rast · lore

[PATCH v2 4/5] t3301: use test_must_fail instead of !

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 <trast@student.ethz.ch>
---
New in this series.
 t/t3301-notes.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
Show changes to t/t3301-notes.sh +4 −4
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
Johannes Schindelin· Feb 14, 2009, 21:26 UTC · re: Thomas Rast · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

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· Feb 15, 2009, 07:52 UTC · re: Thomas Rast · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

Thomas Rast <trast@student.ethz.ch> 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.

Thomas Rast· Feb 15, 2009, 16:11 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

Junio C Hamano wrote:
Show 7 quoted lines
> Thomas Rast <trast@student.ethz.ch> 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· Feb 15, 2009, 18:18 UTC · re: Thomas Rast · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

On Sun, Feb 15, 2009 at 05:11:40PM +0100, Thomas Rast wrote:
Show 10 quoted lines
> > > +	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· Feb 15, 2009, 22:07 UTC · re: Jeff King · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

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
Mike Ralphson· Feb 17, 2009, 09:29 UTC · re: Jeff King · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

2009/2/15 Jeff King <peff@peff.net>:
Show 14 quoted lines
> 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· Feb 17, 2009, 16:34 UTC · re: Mike Ralphson · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

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
Mike Ralphson· Feb 17, 2009, 18:00 UTC · re: Jeff King · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

2009/2/17 Jeff King <peff@peff.net>:
Show 5 quoted lines
> 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· Feb 17, 2009, 20:27 UTC · re: Mike Ralphson · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

On Tue, Feb 17, 2009 at 06:00:17PM +0000, Mike Ralphson wrote:
Show 9 quoted lines
> > 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
Jeff King· Feb 18, 2009, 06:41 UTC · re: Jeff King · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

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
Johannes Schindelin· Feb 18, 2009, 10:14 UTC · re: Jeff King · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

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· Feb 18, 2009, 10:16 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

On Wed, Feb 18, 2009 at 11:14:10AM +0100, Johannes Schindelin wrote:
Show 5 quoted lines
> > 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· Feb 18, 2009, 11:53 UTC · re: Jeff King · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

Hi,
On Wed, 18 Feb 2009, Jeff King wrote:
Show 12 quoted lines
> 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

Jeff King· Feb 19, 2009, 00:37 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

On Wed, Feb 18, 2009 at 12:53:37PM +0100, Johannes Schindelin wrote:
Show 6 quoted lines
> > 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· Feb 19, 2009, 00:46 UTC · re: Jeff King · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

Hi,
On Wed, 18 Feb 2009, Jeff King wrote:
Show 22 quoted lines
> 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· Feb 19, 2009, 00:46 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

On Thu, Feb 19, 2009 at 01:46:28AM +0100, Johannes Schindelin wrote:
Show 8 quoted lines
> 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
Thomas Rast· Feb 17, 2009, 08:44 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

Junio C Hamano wrote:
Show 7 quoted lines
> Thomas Rast <trast@student.ethz.ch> 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· Feb 17, 2009, 08:46 UTC · re: Thomas Rast · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

Thomas Rast wrote:
Show 14 quoted lines
> Junio C Hamano wrote:
> > Thomas Rast <trast@student.ethz.ch> 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
Brandon Casey· Feb 17, 2009, 16:56 UTC · re: Thomas Rast · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

Thomas Rast wrote:
Show 16 quoted lines
> Thomas Rast wrote:
>> Junio C Hamano wrote:
>>> Thomas Rast <trast@student.ethz.ch> 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
Junio C Hamano· Feb 17, 2009, 22:20 UTC · re: Brandon Casey · lore

Re: [PATCH v2 4/5] t3301: use test_must_fail instead of !

Brandon Casey <casey@nrlssc.navy.mil> 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· Feb 17, 2009, 22:53 UTC · re: Junio C Hamano · lore

[PATCH] test suite: correct export var=val usage

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(-)
Show changes to 4 files +8 −8

t/t3301-notes.sh, t/t3302-notes-index-expensive.sh, t/t9301-fast-export.sh, t/test-lib.sh

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
Johannes Schindelin· Feb 17, 2009, 23:47 UTC · re: Jay Soffian · lore

Re: [PATCH] test suite: correct export var=val usage

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· Feb 18, 2009, 00:37 UTC · re: Johannes Schindelin · lore

Re: [PATCH] test suite: correct export var=val usage

On Tue, Feb 17, 2009 at 6:47 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 11 quoted lines
> 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.
Jay Soffian· Feb 17, 2009, 22:54 UTC · re: Junio C Hamano · lore

[PATCH] test suite: correct export var=val usage

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 <jaysoffian@gmail.com>
---
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(-)
Show changes to 4 files +8 −8

t/t3301-notes.sh, t/t3302-notes-index-expensive.sh, t/t9301-fast-export.sh, t/test-lib.sh

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· Feb 17, 2009, 22:57 UTC · re: Junio C Hamano · lore

[PATCH] test suite: correct export var=val usage

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 <jaysoffian@gmail.com>
---
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(-)
Show changes to 4 files +7 −7

t/t3301-notes.sh, t/t3302-notes-index-expensive.sh, t/t9301-fast-export.sh, t/test-lib.sh

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
Wincent Colaiuta· Feb 18, 2009, 10:06 UTC · re: Jay Soffian · lore

Re: [PATCH] test suite: correct export var=val usage

El 17/2/2009, a las 23:57, Jay Soffian escribió:
Show 13 quoted lines
> 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

Jay Soffian· Feb 18, 2009, 13:19 UTC · re: Wincent Colaiuta · lore

Re: [PATCH] test suite: correct export var=val usage

On Wed, Feb 18, 2009 at 5:06 AM, Wincent Colaiuta <win@wincent.com> 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· Feb 18, 2009, 13:29 UTC · re: Jay Soffian · lore

Re: [PATCH] test suite: correct export var=val usage

On Wed, Feb 18, 2009 at 8:19 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
Show 6 quoted lines
> On Wed, Feb 18, 2009 at 5:06 AM, Wincent Colaiuta <win@wincent.com> 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· Feb 18, 2009, 13:34 UTC · re: Jay Soffian · lore

Re: [PATCH] test suite: correct export var=val usage

Hi,
On Wed, 18 Feb 2009, Jay Soffian wrote:
Show 13 quoted lines
> On Wed, Feb 18, 2009 at 8:19 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> > On Wed, Feb 18, 2009 at 5:06 AM, Wincent Colaiuta <win@wincent.com> 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· Feb 18, 2009, 16:56 UTC · re: Jay Soffian · lore

Re: [PATCH] test suite: correct export var=val usage

El 18/2/2009, a las 14:29, Jay Soffian escribió:
Show 18 quoted lines
> On Wed, Feb 18, 2009 at 8:19 AM, Jay Soffian <jaysoffian@gmail.com>  
> wrote:
>> On Wed, Feb 18, 2009 at 5:06 AM, Wincent Colaiuta <win@wincent.com>  
>> 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· Feb 18, 2009, 17:05 UTC · re: Wincent Colaiuta · lore

Re: [PATCH] test suite: correct export var=val usage

Wincent Colaiuta wrote:
Show 13 quoted lines
> 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· Feb 18, 2009, 20:55 UTC · re: Junio C Hamano · lore

[PATCH v2] test suite: correct export var=val usage

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 <jaysoffian@gmail.com>
---
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(-)
Show changes to 4 files +7 −7

t/t3301-notes.sh, t/t3302-notes-index-expensive.sh, t/t9301-fast-export.sh, t/test-lib.sh

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
Thomas Rast· Feb 14, 2009, 20:23 UTC · re: Thomas Rast · lore

[PATCH v2 5/5] notes: refuse to edit notes outside refs/notes/

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 <trast@student.ethz.ch>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
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(-)
Show changes to 2 files +12 −0

git-notes.sh, t/t3301-notes.sh

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· Feb 14, 2009, 19:29 UTC · re: Thomas Rast · lore

Re: [PATCH 1/4] notes: only clean up message file when editing

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

← back to recent threads