Re: [PATCH 1/6] t3404 & t3411: undo copy&paste

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Tue, 27 Jan 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Rather than copying and pasting, which is prone to lead to fixes
> > missing in one version, move the fake-editor generator to t/t3404/.
> >
> > While at it, fix a typo that causes head-scratching: use
> > ${SHELL_PATH-/bin/sh} instead of $SHELL_PATH.
> 
> I've learned to be cautious whenever I see "while at it".

Heh.

> > diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> > new file mode 100644
> > index 0000000..8c8caab
> > --- /dev/null
> > +++ b/t/lib-rebase.sh
> > @@ -0,0 +1,36 @@
> > +#!/bin/sh
> > +
> > +set_fake_editor () {
> > +	echo "#!${SHELL_PATH-/bin_sh}" >fake-editor.sh
> 
> It is unclear why you would want to do this.  It was unclear what "typo"
> you were referring to in your commit log message, either.
> 
> The tests are supposed to run under the shell the user specified, so if
> there is a case you found that $SHELL_PATH is unset, that is a bug we
> would want to fix, and ${SHELL_PATH-/bin/sh} is sweeping the problem under
> the rug to make it harder to fix, isn't it?

I call the scripts directly, and I do not think it would be a good idea to 
force the user to use GIT_TEST_OPTS and make when calling the script 
directly is so much easier.  Plus, this way I can pass "sh -x $SCRIPT" 
easily.

I am really puzzled that it works, BTW.  With an empty SHELL_PATH, 
apparently.

> Besides, it's /bin/sh, not /bin_sh ;-)

Right.  The commit message was right, at least!

> > +	cat >> fake-editor.sh <<\EOF
> > +case "$1" in
> > +*/COMMIT_EDITMSG)
> > +	test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
> > +	test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
> > +	exit
> > +	;;
> > +esac
> > +test -z "$EXPECT_COUNT" ||
> > +	test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
> > +	exit
> > +test -z "$FAKE_LINES" && exit
> > +grep -v '^#' < "$1" > "$1".tmp
> > +rm -f "$1"
> > +cat "$1".tmp
> > +action=pick
> > +for line in $FAKE_LINES; do
> > +	case $line in
> > +	squash|edit)
> > +		action="$line";;
> > +	*)
> > +		echo sed -n "${line}s/^pick/$action/p"
> > +		sed -n "${line}p" < "$1".tmp
> > +		sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
> 
> I looked at the output from this and wondered what these "sed -n" shown 
> in the "-v" output were about last night.  I do think it is a good idea 
> to show what edit was done to the insn stream, but I suspect it may be 
> easier to read the output if you did this instead:
> 
> > +		sed -n "${line}p" < "$1".tmp
> > +		sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
> > +		sed -n "${line}s/^pick/$action/p" < "$1".tmp


Probably.  It is for debugging, anyway.  As everything you only see with 
-v.

Ciao,
Dscho