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

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

[Thomas Rast]

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(-)

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

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

[Thomas Rast]

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(-)

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

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

[Johannes Schindelin]

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

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

[Thomas Rast]

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(-)

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

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

[Johannes Schindelin]

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

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

[Thomas Rast]

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(-)

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

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

[Johannes Schindelin]

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

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

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Thomas Rast]

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(-)

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

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

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> +	MESSAGE="$GIT_DIR"/new-notes-$COMMIT
> +	trap '
> +		test -f "$MESSAGE" && rm "$MESSAGE"
> +	' 0
> +

Hmph, why not unconditionally rm -f "$MESSAGE" without test?

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

[Thomas Rast]

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(-)

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

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

[Thomas Rast]

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(-)

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

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

[Thomas Rast]

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(-)

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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.

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

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

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.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Jeff King]

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

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

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 640 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Mike Ralphson]

2009/2/15 Jeff King <peff@peff.net>:
> 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

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

[Jeff King]

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

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

[Mike Ralphson]

2009/2/17 Jeff King <peff@peff.net>:
> 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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Johannes Schindelin]

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

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

[Jeff King]

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

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

[Johannes Schindelin]

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

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

[Jeff King]

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

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

[Johannes Schindelin]

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

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

[Jeff King]

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

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

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 700 bytes --]

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 ;-)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

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!

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Brandon Casey]

Thomas Rast wrote:
> 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

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

[Junio C Hamano]

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 ;;

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

[Jay Soffian]

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

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

[Johannes Schindelin]

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

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

[Jay Soffian]

On Tue, Feb 17, 2009 at 6:47 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> 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.

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

[Jay Soffian]

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(-)

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

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

[Jay Soffian]

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(-)

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

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

[Wincent Colaiuta]

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

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

[Jay Soffian]

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.

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

[Jay Soffian]

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.)

j.

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

[Johannes Schindelin]

Hi,

On Wed, 18 Feb 2009, Jay Soffian wrote:

> 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

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

[Wincent Colaiuta]

El 18/2/2009, a las 14:29, Jay Soffian escribió:

> 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

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

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Jay Soffian]

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(-)

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

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

[Thomas Rast]

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(-)

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

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

[Johannes Schindelin]

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