[PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Brian Gernhardt]

t3404-rebase-interactive used `grep -Fx 0` to match against `wc -l`
output.  This fails on OS X and any other system where wc outputs
whitespace.  Use `test 0 = `... instead, like we do in other tests.

Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
---

 Should this construct go into CodingStyle?  I seem to have to write
 patches like this every month or so.

 t/t3404-rebase-interactive.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d20ed4f..f204284 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -211,7 +211,7 @@ test_expect_success 'setting marks works' '
 	test "$(git rev-parse HEAD~2)" = \
 		"$(git rev-parse refs/rebase-marks/42)" &&
 	git rebase --abort &&
-	ls $marks_dir | wc -l | grep -Fx 0
+	test 0 = $(ls $marks_dir | wc -l)
 '
 
 test_expect_success 'reset with nonexistent mark fails' '
-- 
1.5.5.1.174.g8f57349

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Johannes Schindelin]

Hi,

On Sun, 27 Apr 2008, Brian Gernhardt wrote:

>  Should this construct go into CodingStyle?  I seem to have to write 
>  patches like this every month or so.

Yes, probably.  I am very sorry, I really should have reviewed those 
patches better (I know that ":" in expr is better than "match", "tac" is 
something to be avoided, and "wc -l" can output whitespace).  It did not 
help that I hated the fact that that series changed the original design 
without even understanding it.

Ciao,
Dscho

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Brian Gernhardt]

On Apr 27, 2008, at 11:22 AM, Johannes Schindelin wrote:

> On Sun, 27 Apr 2008, Brian Gernhardt wrote:
>
>> Should this construct go into CodingStyle?  I seem to have to write
>> patches like this every month or so.
>
> Yes, probably.  I am very sorry, I really should have reviewed those
> patches better (I know that ":" in expr is better than "match",  
> "tac" is
> something to be avoided, and "wc -l" can output whitespace).  It did  
> not
> help that I hated the fact that that series changed the original  
> design
> without even understanding it.

Eh, not everyone's perfect.  I would have used `rev` instead of `tac`  
and still been wrong for Solaris.  But it seems that the `wc -l`  
whitespace issue seems to hit nearly everyone at some point, so I  
thought it would be a good candidate for CodingStyle.

Personally, I'd love to have the time to review all the patches to  
catch these issues while still on the list instead of waiting until  
they hit next and I tried to compile it.  But I don't always notice,  
have time, or care myself.

~~ Brian

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Jeff King]

On Sun, Apr 27, 2008 at 11:32:24AM -0400, Brian Gernhardt wrote:

> Eh, not everyone's perfect.  I would have used `rev` instead of `tac` and 
> still been wrong for Solaris.  But it seems that the `wc -l` whitespace 
> issue seems to hit nearly everyone at some point, so I thought it would be 
> a good candidate for CodingStyle.
>
> Personally, I'd love to have the time to review all the patches to catch 
> these issues while still on the list instead of waiting until they hit 
> next and I tried to compile it.  But I don't always notice, have time, or 
> care myself.

BTW, how did you discover this bug? Through normal use, or was there a
failing test?

If a failing test, then I wonder if we could get a few people to set up
automated tests on alternate platforms. IIRC, Junio makes sure that
master always passes test on his Linux box and KO (Debian and Redhat, I
think?). Other platforms could "git pull && make test" daily. I could
probably do Solaris (once I get the tests to complete pass at all!) and
FreeBSD 6.

-Peff

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Mike Ralphson]

2008/4/28 Jeff King <peff@peff.net>:
> If a failing test, then I wonder if we could get a few people to set up
> automated tests on alternate platforms. IIRC, Junio makes sure that
> master always passes test on his Linux box and KO (Debian and Redhat, I
> think?). Other platforms could "git pull && make test" daily. I could
> probably do Solaris (once I get the tests to complete pass at all!) and
> FreeBSD 6.

I could run automated build / test [/ bisect?] cycles on AIX if of any interest.

Mike

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Jeff King]

On Mon, Apr 28, 2008 at 10:56:05AM +0100, Mike Ralphson wrote:

> > If a failing test, then I wonder if we could get a few people to set up
> > automated tests on alternate platforms. IIRC, Junio makes sure that
> > master always passes test on his Linux box and KO (Debian and Redhat, I
> > think?). Other platforms could "git pull && make test" daily. I could
> > probably do Solaris (once I get the tests to complete pass at all!) and
> > FreeBSD 6.
> 
> I could run automated build / test [/ bisect?] cycles on AIX if of any
> interest.

I think that would be helpful. We seem to have most Linux variants
pretty well covered. I now have a daily pull/build/test running on a
FreeBSD 6.1 box. I am going to try to get a Solaris one going, too, but
I have to first actually get the test scripts to pass _once_. :)

AIX would be nice, since it seems easy to break. ;) OS X would be nice,
too, though I suspect there are a few developers (Shawn?) who end up
running the test scripts occasionally anyway.

I am just calling the script below through cron, and it dumps a bunch of
output if any test fails (at which point I go investigate manually). The
only argument is the path to a git repo.

-- >8 --
#!/bin/sh

dir=$1; shift
log="$dir/.autotest.out"

try() {
  "$@" >"$log" 2>&1
  case "$?" in
    0) ;;
    *) echo >&2 "autotest failed: $*"
       cat >&2 "$log"
       exit 1
       ;;
  esac
}

try cd "$dir"
try git pull
try gmake
PATH=/usr/local/bin:/usr/bin:/bin; export PATH
try gmake test

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Mike Ralphson]

2008/5/13 Jeff King <peff@peff.net>:
>
> On Mon, Apr 28, 2008 at 10:56:05AM +0100, Mike Ralphson wrote:
>  > I could run automated build / test [/ bisect?] cycles on AIX if of any
>  > interest.
>
>  I think that would be helpful. We seem to have most Linux variants
>  pretty well covered. I now have a daily pull/build/test running on a
>  FreeBSD 6.1 box. I am going to try to get a Solaris one going, too, but
>  I have to first actually get the test scripts to pass _once_. :)
>
>  AIX would be nice, since it seems easy to break. ;) OS X would be nice,
>  too, though I suspect there are a few developers (Shawn?) who end up
>  running the test scripts occasionally anyway.
>
>  I am just calling the script below through cron, and it dumps a bunch of
>  output if any test fails (at which point I go investigate manually). The
>  only argument is the path to a git repo.

Thanks - that was a helpful spur to action. I'll check tomorrow how it
fairs pulling, building, running the tests etc. I've added a couple of
'try git tag -f's to it, so I have KNOWN_BUILDING and KNOWN_PASSING
points to pass quickly into bisect if necessary.

I'll shout the first time something breaks (after doing a bit of
rudimentary investigation), then maybe we can look at a way of
aggregating the build/test statuses and whether that should be pushed
to a website (or git repo, obviously) or some kind of alert.

Cheers, Mike

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Mike Ralphson]

2008/5/13 Mike Ralphson <mike.ralphson@gmail.com>:
> Thanks - that was a helpful spur to action. I'll check tomorrow how it
> fairs pulling, building, running the tests etc. I've added a couple of
> 'try git tag -f's to it, so I have KNOWN_BUILDING and KNOWN_PASSING
> points to pass quickly into bisect if necessary.

My KNOWN_BUILDING and KNOWN_PASSING tags are now happily chasing each
other up the commit log.

Which branch(es) would it be most useful on which to have this
automated build/test cycle?

Although the list of tags might get slightly unwieldy (i.e. the top
commit will gain a lot of tags if all is well), with a sensible naming
convention, these tags could be pushed to a central repo (a regularly
updated clone of git.git) allowing easy visibility of the current
state of the 'build collective'.

Something like {intials}_{uname info}_{branch}_KNOWN_{BUILDING|PASSING} ?

Mike

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Jeff King]

On Thu, May 15, 2008 at 11:16:27AM +0100, Mike Ralphson wrote:

> Which branch(es) would it be most useful on which to have this
> automated build/test cycle?

I would think maint, master, and next, but with next as the least
important. I think Junio generally tests maint and master before
publishing, but presumably not always next (as there was test breakage
in next earlier today).

> Although the list of tags might get slightly unwieldy (i.e. the top
> commit will gain a lot of tags if all is well), with a sensible naming
> convention, these tags could be pushed to a central repo (a regularly
> updated clone of git.git) allowing easy visibility of the current
> state of the 'build collective'.
> 
> Something like {intials}_{uname info}_{branch}_KNOWN_{BUILDING|PASSING} ?

I have started tagging my auto-builds as you suggest. It should be easy
enough to push to a repo.or.cz repository. Although I'm not sure of the
utility of auto-publishing this information. Who is going to look at it?

I had assumed a workflow more like "it passes 99% of the time; in the
remaining 1%, the cron job kicks off a message to the owning user, who
then investigates and/or writes a bug report to the list."

That implies a little bit of expertise and work from the user owning the
build, but:

  - presumably it won't happen very frequently

  - they are probably the only person with the resources to diganose and
    fix, anyway, since they are the ones with access to the platform.

-Peff

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Jeff King]

On Thu, May 15, 2008 at 07:20:30AM -0400, Jeff King wrote:

> I have started tagging my auto-builds as you suggest. It should be easy
> enough to push to a repo.or.cz repository. Although I'm not sure of the
> utility of auto-publishing this information. Who is going to look at it?

Also, if there is interest in an automated "this is now broken on
platform X", I think the interesting thing is not "what was the last
passing state" but rather "what is the output of 'make test' for the
failing state." So:

> I had assumed a workflow more like "it passes 99% of the time; in the
> remaining 1%, the cron job kicks off a message to the owning user, who
> then investigates and/or writes a bug report to the list."

In that case, I think the interesting automation is making a problem
report from a failed case.

-Peff

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, May 15, 2008 at 11:16:27AM +0100, Mike Ralphson wrote:
>
>> Which branch(es) would it be most useful on which to have this
>> automated build/test cycle?
>
> I would think maint, master, and next, but with next as the least
> important. I think Junio generally tests maint and master before
> publishing, but presumably not always next (as there was test breakage
> in next earlier today).

I'd prefer heterogeneous automated test coverage to be on 'next' and
'master'.  If the coverage extends to 'maint' that would be nicer, but on
the other hand, I rarely apply anything remotely questionable directly on
top of maint (instead, I'd fork from maint and merge the result first to
next or master), so if we can catch master and next, we should be Ok.

Before any push-out, I ran tests on all four integration branches on
Debian (etch) and FC (I think it is FC5), both x86-64.  But sometimes 'pu'
is shipped with known breakage in tests.  I can not push out with broken
tests in 'maint', 'master' or 'next' (automated procedure on my end
prevents me from doing so).

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Mike Ralphson]

2008/5/15 Junio C Hamano <gitster@pobox.com>:
> I'd prefer heterogeneous automated test coverage to be on 'next' and
> 'master'.  If the coverage extends to 'maint' that would be nicer...
>

Do you have any interest in seeing the results of these automated
builds? I'm thinking specifically of the case where you're about to
tag a final release...

Obviously I'll shout on the list if/when we get a breakage.

An example of a tracking fork with the build/pass tags pushed to it is
at http://repo.or.cz/w/git/gitbuild.git - unfortunately as the
branches are significant, the mob approach doesn't really work, so if
anyone else would like to push tags to this repo, please just ask.

Mike

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Brian Gernhardt]

On Apr 28, 2008, at 5:41 AM, Jeff King <peff@peff.net> wrote:
> BTW, how did you discover this bug? Through normal use, or was there a
> failing test?

My small series of one-line patches were from trying to get t3404  
(IIRC) to work. So our tests do help. ;-)

> If a failing test, then I wonder if we could get a few people to set  
> up
> automated tests on alternate platforms. IIRC, Junio makes sure that
> master always passes test on his Linux box and KO (Debian and  
> Redhat, I
> think?). Other platforms could "git pull && make test" daily. I could
> probably do Solaris (once I get the tests to complete pass at all!)  
> and
> FreeBSD 6.

I have a script that boils down to 'make && make test && make  
install'.  I pull, check changes, and then run it once or twice a  
week. I can't dedicate my laptop to automated testing, but I do raise  
a fuss when it fails.

A build farm would help, especially with simple portability errors  
like this. Would be good to get OS X, Solaris (our old nemesis), and a  
few variations for /bin/sh. But I don't really have a good machine to  
volunteer for the effort myself.

~~ Brian G.

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> ...  It did not 
> help that I hated the fact that that series changed the original design 
> without even understanding it.

Care to elaborate on this point further?  I do not get it.

Do you mean to say "I hate it because it does things differently from how
I did it originally", or "Because it does things differently from how the
original did, it breaks this and that cases"?

If the latter, "this and that" part is especially useful.  A solution to
fix that may end up to be closer to the original implementation.

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Johannes Schindelin]

Hi,

On Sun, 27 Apr 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > ...  It did not help that I hated the fact that that series changed 
> > the original design without even understanding it.
> 
> Care to elaborate on this point further?  I do not get it.

The original implementation of -p was modeled closely after filter-branch, 
in that it created a subdirectory (dotest/rewritten) containing the new 
commit names for those commits that were rewritten.

Now, whenever a commit was picked, the parents would be looked up in 
dotest/rewritten, and replaced with the rewritten name (or left unchanged 
if they were not rewritten).

In that manner, every commit is identified by the (original) commit name.  
<irony>Surprisingly, this is the way Git was meant to operate</irony>

Now, a mark command has been introduced which is totally unnecessary.  
Commits can _still_ be identified by their (original) commit name.  That's 
the whole assumption rebase -i relies on.

Basically, the output of rebase -i -p is ugly now, because you have _two_ 
ways of specifying things, and frankly, I would have to read documentation 
to find out when to use what.  And I maintain that this was not necessary 
with the old way rebase -i operated.

So I am really unhappy that this patch series made it in, and I am even 
more unhappy that my suggestions (which I made, in spite of moving between 
two countries, and in spite of spending a lot of time with someone very 
special, and therefore having less time for Git than I would have liked 
to) were blatantly ignored.

It would have been easier for me if I would not be so utterly convinced 
that the "new" way is so much more complicated and unintuitive than what I 
suggested.

And now it is already in "next", which does not help me at all (me being 
very busy at the moment to find a job).  I am also slightly uneasy about 
the fact that a few obvious mistakes had to be fixed in the last days.

Formulations such as "deliberately leaves $DOTEST directory behind if 
clean-up fails" make me wonder, too: I sincerely hope that I misunderstand 
the intention of this message.

I have the feeling that I have to repeat my point again, so that it is not 
ignored -- again.  Maybe an example would help:

-- snip --
pick abcdefg This is the first commit to be picked
reset cdefghij
pick zyxwvux A commit in a side-branch
merge recursive abcdefg
-- snap --

I am convinced that this syntax does not need much explanation.

A patch implementing a syntax like this would have won my unilateral 
approval (modulo expr/tac quirks, but that would have been easy to fix).

Ciao,
Dscho who does not like complicator's gloves

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Jörg Sommer]

Hi,

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Sun, 27 Apr 2008, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > ...  It did not help that I hated the fact that that series changed 
>> > the original design without even understanding it.
>> 
>> Care to elaborate on this point further?  I do not get it.
>
> The original implementation of -p was modeled closely after filter-branch, 
> in that it created a subdirectory (dotest/rewritten) containing the new 
> commit names for those commits that were rewritten.

But that wasn't the way rebase -i works. You had to jump in before
pick_one does anything which clearly shows you did something different
from the default way.

> Now, whenever a commit was picked, the parents would be looked up in 
> dotest/rewritten, and replaced with the rewritten name (or left unchanged 
> if they were not rewritten).

This approach doesn't work when you change the order of commits.
Take the commit A, B and C in this order and reorder them to A C B:
1. pick A, A^ was not rewritten, nothing changed, A stays the same
2. pick C, C^ was not rewritten, nothing changed, C stays the same
3. pick B, B^ was not rewritten, nothing changed, B stays the same

Depending on your handling of the new tip of the branch you loose C or,
as your code did, nothing changed, because you made the assumption the
new HEAD is the rewritten old HEAD.

> Basically, the output of rebase -i -p is ugly now, because you have _two_ 
> ways of specifying things,

> I have the feeling that I have to repeat my point again, so that it is not 
> ignored -- again.  Maybe an example would help:
>
> -- snip --
> pick abcdefg This is the first commit to be picked
> reset cdefghij
> pick zyxwvux A commit in a side-branch
> merge recursive abcdefg
> -- snap --
>
> I am convinced that this syntax does not need much explanation.

But above you said this syntax + mark is “ugly”. Strange.

> A patch implementing a syntax like this would have won my unilateral 
> approval

I doubt this. You refused any changes to your idea and your code from the
beginning. You didn't answer questions and doesn't take part on the
discussion [1] about the new syntax.

Bye, Jörg.

[1] <7vabkoufzq.fsf@gitster.siamese.dyndns.org>
-- 
Es gibt nichts schöneres als dem Schweigen eines Dummkopfes zuzuhören.
   	       		      	  	        (Helmut Quatlinger)

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3313 bytes --]

Hi,

don't cull me from the Cc: list.  This has been mentioned on this list so 
often, it is not even funny any more.

On Mon, 28 Apr 2008, Jörg Sommer wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > On Sun, 27 Apr 2008, Junio C Hamano wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> 
> >> > ...  It did not help that I hated the fact that that series changed 
> >> > the original design without even understanding it.
> >> 
> >> Care to elaborate on this point further?  I do not get it.
> >
> > The original implementation of -p was modeled closely after 
> > filter-branch, in that it created a subdirectory (dotest/rewritten) 
> > containing the new commit names for those commits that were rewritten.
> 
> But that wasn't the way rebase -i works.

I know exactly how it works. D'oh.

> You had to jump in before pick_one does anything which clearly shows you 
> did something different from the default way.

That is bullshit.  I did not do anything "different from the default way".  
I carefully designed an interface that was easy to understand, because it 
mimicked how you would do the same _by hand_, but without the hassle to 
actually having to do everything by hand.

In other words, rebase -i is just a cherry-pick in a loop.

And _exactly_ the same should have been done for -p.  Namely, _not_ 
introduce some artificial marks, but use the _commit names_!

> > Now, whenever a commit was picked, the parents would be looked up in 
> > dotest/rewritten, and replaced with the rewritten name (or left 
> > unchanged if they were not rewritten).
> 
> This approach doesn't work when you change the order of commits.
> Take the commit A, B and C in this order and reorder them to A C B:
> 1. pick A, A^ was not rewritten, nothing changed, A stays the same
> 2. pick C, C^ was not rewritten, nothing changed, C stays the same
> 3. pick B, B^ was not rewritten, nothing changed, B stays the same

You carefully ignored how I intended the parents to be used: only for 
merges.

> > Basically, the output of rebase -i -p is ugly now, because you have 
> > _two_ ways of specifying things,
> 
> > I have the feeling that I have to repeat my point again, so that it is not 
> > ignored -- again.  Maybe an example would help:
> >
> > -- snip --
> > pick abcdefg This is the first commit to be picked
> > reset cdefghij
> > pick zyxwvux A commit in a side-branch
> > merge recursive abcdefg
> > -- snap --
> >
> > I am convinced that this syntax does not need much explanation.
> 
> But above you said this syntax + mark is “ugly”. Strange.

You know, I find it strange how you try to make a _point_ in 
misunderstanding me.  Did I not mention that the way to have _two_ ways to 
reference commits was ugly?  You did not even bother to remove that part 
from what you quoted.

> > A patch implementing a syntax like this would have won my unilateral 
> > approval
> 
> I doubt this. You refused any changes to your idea and your code from 
> the beginning. You didn't answer questions and doesn't take part on the 
> discussion [1] about the new syntax.

Well, you carefully ignored (but removed from the quoted text) my 
explanation.  Nevertheless, I did participate in the discussion, and 
mentioned my preferred way of doing things.

Sheesh,
Dscho

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Jörg Sommer]

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

Hi,

Johannes Schindelin schrieb am Mon 28. Apr, 14:42 (+0100):
> On Mon, 28 Apr 2008, Jörg Sommer wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > On Sun, 27 Apr 2008, Junio C Hamano wrote:
> > >
> > >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > >> 
> > >> > ...  It did not help that I hated the fact that that series changed 
> > >> > the original design without even understanding it.
> > >> 
> > >> Care to elaborate on this point further?  I do not get it.
> > >
> > > The original implementation of -p was modeled closely after 
> > > filter-branch, in that it created a subdirectory (dotest/rewritten) 
> > > containing the new commit names for those commits that were rewritten.
> > 
> > But that wasn't the way rebase -i works.
> 
> I know exactly how it works. D'oh.
> 
> > You had to jump in before pick_one does anything which clearly shows you 
> > did something different from the default way.
> 
> That is bullshit.  I did not do anything "different from the default way".  
> I carefully designed an interface that was easy to understand, because it 
> mimicked how you would do the same _by hand_, but without the hassle to 
> actually having to do everything by hand.
> 
> In other words, rebase -i is just a cherry-pick in a loop.

But not rebase -i -p.

> And _exactly_ the same should have been done for -p.

But you didn't do it.

> Namely, _not_ introduce some artificial marks, but use the _commit
> names_!

I don't buy, you don't use marks (notes on paper or git tags) when you rebase
a branch with at least 8 commits and 2 merges.

And Junio discribed how he would do such a rebase and it included marks.
And I follow how. So no, they aren't artificial.

> > > Now, whenever a commit was picked, the parents would be looked up in 
> > > dotest/rewritten, and replaced with the rewritten name (or left 
> > > unchanged if they were not rewritten).
> > 
> > This approach doesn't work when you change the order of commits.
> > Take the commit A, B and C in this order and reorder them to A C B:
> > 1. pick A, A^ was not rewritten, nothing changed, A stays the same
> > 2. pick C, C^ was not rewritten, nothing changed, C stays the same
> > 3. pick B, B^ was not rewritten, nothing changed, B stays the same
> 
> You carefully ignored how I intended the parents to be used: only for 
> merges.

And why does this test fail? Please tell me, as you “know exactly how it
works.”

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9cf873f..83c2964 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -361,4 +361,10 @@ test_expect_success 'rebase with a file named HEAD in worktree' '
 
 '
 
+test_expect_success 'rebase with a file named HEAD in worktree' '
+       head=$(git rev-parse HEAD) &&
+       FAKE_LINES="1 3 2" git rebase -i -p HEAD~3 &&
+       test $(git rev-parse HEAD) != $head
+'
+
 test_done

* FAIL 25: rebase with a file named HEAD in worktree

                head=$(git rev-parse HEAD) &&
                FAKE_LINES="1 3 2" git rebase -i -p HEAD~3 &&
                test $(git rev-parse HEAD) != $head

> > > Basically, the output of rebase -i -p is ugly now, because you have 
> > > _two_ ways of specifying things,
> > 
> > > I have the feeling that I have to repeat my point again, so that it is not 
> > > ignored -- again.  Maybe an example would help:
> > >
> > > -- snip --
> > > pick abcdefg This is the first commit to be picked
> > > reset cdefghij
> > > pick zyxwvux A commit in a side-branch
> > > merge recursive abcdefg

Where do you tell which merge should be redone?

> > > -- snap --
> > >
> > > I am convinced that this syntax does not need much explanation.
> > 
> > But above you said this syntax + mark is “ugly”. Strange.
> 
> You know, I find it strange how you try to make a _point_ in 
> misunderstanding me.  Did I not mention that the way to have _two_ ways to 
> reference commits was ugly?  You did not even bother to remove that part 
> from what you quoted.

Because Junio told you about it. And I don't find your suggestion in
<alpine.DEB.1.00.0804141506270.28504@racer> very readable:

| I would like it much better, if there was something like
|
| pick 5cc8f37 (init: show "Reinit" message even in ...)
| pick 18d077c (quiltimport: fix misquoting of parse...)
| merge 9876543:5cc8f37,18d077c (Merge blub)
        ^^^^^^^^^^^^^^^
| reset 5cc8f37
| ...

I don't see why you complain about the marks and suggest to use
9876543:5cc8f37,18d077c. In a short example like yours it doesn't hurd,
but put 10 line between the merge an the pick and maybe change move one
of the merged commits behind the merge.

pick 8a785dc Add tests to catch problems with un-unlinkable symlinks
pick 8d14ac9 Test: catch if trash cannot be removed
pick 29dc133 git-merge-one-file: fix longstanding stupid thinko
pick deda26b Merge branch 'jc/makefile'
pick 7f8ab8d Don't update unchanged merge entries
pick 198724a fast-import: Allow "reset" to delete a new branch without error
pick 20fd60b t1000: use "test_must_fail git frotz", not "! git frotz"
pick 7092882 Update draft release notes for 1.5.5
pick c817faa Resurrect git-rerere to contrib/examples
pick 1eaa541 Merge branch 'maint'
pick 81d6650 Start draft ReleaseNotes for 1.5.4.5
merge 9876543:81d6650,198724a (Merge blub)
pick e637122 rebase -m: do not trigger pre-commit verification

pick 8a785dc Add tests to catch problems with un-unlinkable symlinks
pick 8d14ac9 Test: catch if trash cannot be removed
pick 29dc133 git-merge-one-file: fix longstanding stupid thinko
pick deda26b Merge branch 'jc/makefile'
pick 7f8ab8d Don't update unchanged merge entries
pick 198724a fast-import: Allow "reset" to delete a new branch without error
mark #1
pick 20fd60b t1000: use "test_must_fail git frotz", not "! git frotz"
pick 7092882 Update draft release notes for 1.5.5
pick c817faa Resurrect git-rerere to contrib/examples
pick 1eaa541 Merge branch 'maint'
pick 81d6650 Start draft ReleaseNotes for 1.5.4.5
merge 9876543:81d6650,#1 (Merge blub)
pick e637122 rebase -m: do not trigger pre-commit verification

> > > A patch implementing a syntax like this would have won my unilateral 
> > > approval
> > 
> > I doubt this. You refused any changes to your idea and your code from 
> > the beginning. You didn't answer questions and doesn't take part on the 
> > discussion [1] about the new syntax.
> 
> Well, you carefully ignored (but removed from the quoted text) my 
> explanation.

Sorry no, Junio made his proposal on Mar 24 and merge the code on Apr 25.
I treat this as a adequate time window to propose a _better_ idea.

Bye, Jörg.
-- 
“Unfortunately, the current generation of mail programs do not have
 checkers to see if the sender knows what he is talking about”
            (Andrew S. Tanenbaum)

[-- Attachment #2: Digital signature http://en.wikipedia.org/wiki/OpenPGP --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2512 bytes --]

Hi,

On Mon, 28 Apr 2008, Jörg Sommer wrote:

> Johannes Schindelin schrieb am Mon 28. Apr, 14:42 (+0100):
> > On Mon, 28 Apr 2008, Jörg Sommer wrote:
> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > > On Sun, 27 Apr 2008, Junio C Hamano wrote:
> > > >
> > > >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > > >> 
> > > >> > ...  It did not help that I hated the fact that that series changed 
> > > >> > the original design without even understanding it.
> > > >> 
> > > >> Care to elaborate on this point further?  I do not get it.
> > > >
> > > > The original implementation of -p was modeled closely after 
> > > > filter-branch, in that it created a subdirectory (dotest/rewritten) 
> > > > containing the new commit names for those commits that were rewritten.
> > > 
> > > But that wasn't the way rebase -i works.
> > 
> > I know exactly how it works. D'oh.
> > 
> > > You had to jump in before pick_one does anything which clearly shows you 
> > > did something different from the default way.
> > 
> > That is bullshit.  I did not do anything "different from the default way".  
> > I carefully designed an interface that was easy to understand, because it 
> > mimicked how you would do the same _by hand_, but without the hassle to 
> > actually having to do everything by hand.
> > 
> > In other words, rebase -i is just a cherry-pick in a loop.
> 
> But not rebase -i -p.

Yes.  With the exception that you have to checkout and merge in the loop, 
too.

> > And _exactly_ the same should have been done for -p.
> 
> But you didn't do it.

Very well done.  If your intention is to piss me off: you succeeded.

_OF COURSE_ I did not do it.  That is why it was not working.

But you could have fixed that.

Instead, you chose to complicate things.

> > Namely, _not_ introduce some artificial marks, but use the _commit 
> > names_!
> 
> I don't buy, you don't use marks (notes on paper or git tags) when you 
> rebase a branch with at least 8 commits and 2 merges.
> 
> And Junio discribed how he would do such a rebase and it included marks. 
> And I follow how. So no, they aren't artificial.

So you again ignored completely the argument I made.

Brilliant.

The same issue is _totally_ the same _without_ -p!

And you cannot fix the problem by introducing another one.

You can try to complicate things even further, sure, but you will not 
change the fact that this is no solution at all.

Well, I refuse to let you insult my intelligence any more.

Ciao,
Dscho

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Maybe an example would help:
>
> -- snip --
> pick abcdefg This is the first commit to be picked
> reset cdefghij
> pick zyxwvux A commit in a side-branch
> merge recursive abcdefg
> -- snap --

Indeed it does.  "reset cdefghij" --- does it reset to the exact cdefghij
commit, or cdefghij commit after rewriting?

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Johannes Schindelin]

Hi,

On Mon, 28 Apr 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Maybe an example would help:
> >
> > -- snip --
> > pick abcdefg This is the first commit to be picked
> > reset cdefghij
> > pick zyxwvux A commit in a side-branch
> > merge recursive abcdefg
> > -- snap --
> 
> Indeed it does.  "reset cdefghij" --- does it reset to the exact cdefghij
> commit, or cdefghij commit after rewriting?

In the example, it would be the original commit.  However, a "reset 
abcdefg" _after_ the "pick abcdefg" line would refer to the _rewritten_ 
commit.

The rationale: you are most likely not wanting to reference _both_ the 
original _and_ the rewritten commit.

Ciao,
Dscho

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Mon, 28 Apr 2008, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > Maybe an example would help:
>> >
>> > -- snip --
>> > pick abcdefg This is the first commit to be picked
>> > reset cdefghij
>> > pick zyxwvux A commit in a side-branch
>> > merge recursive abcdefg
>> > -- snap --
>> 
>> Indeed it does.  "reset cdefghij" --- does it reset to the exact cdefghij
>> commit, or cdefghij commit after rewriting?
>
> In the example, it would be the original commit.  However, a "reset 
> abcdefg" _after_ the "pick abcdefg" line would refer to the _rewritten_ 
> commit.
>
> The rationale: you are most likely not wanting to reference _both_ the 
> original _and_ the rewritten commit.

That means moving the lines around inside the todo insn list makes the
same "reset cdefghij" mean different things.

That's insanity.

At least, if you use marks, you could detect a user error that references
a mark before it is defined.

Re: [PATCH 2/2] Fix t3404 assumption that `wc -l` does not use whitespace.

[Johannes Schindelin]

Hi,

On Mon, 28 Apr 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Mon, 28 Apr 2008, Junio C Hamano wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> 
> >> > Maybe an example would help:
> >> >
> >> > -- snip --
> >> > pick abcdefg This is the first commit to be picked
> >> > reset cdefghij
> >> > pick zyxwvux A commit in a side-branch
> >> > merge recursive abcdefg
> >> > -- snap --
> >> 
> >> Indeed it does.  "reset cdefghij" --- does it reset to the exact cdefghij
> >> commit, or cdefghij commit after rewriting?
> >
> > In the example, it would be the original commit.  However, a "reset 
> > abcdefg" _after_ the "pick abcdefg" line would refer to the _rewritten_ 
> > commit.
> >
> > The rationale: you are most likely not wanting to reference _both_ the 
> > original _and_ the rewritten commit.
> 
> That means moving the lines around inside the todo insn list makes the
> same "reset cdefghij" mean different things.
> 
> That's insanity.

I beg you pardon?

Consider this history:

A - B - C - D
      \
        E - F

Now, depending if B was rewritten or not, would you not want the reset 
before E to mean _different_ things?

I.e. _if_ B is rewritten, E should branch off of the _rewritten_ B, but if 
B is _not_ rewritten, E should branch off of the original B?

Ciao,
Dscho