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

Johannes Schindelin [off-list ref] 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.

Hi,

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

Johannes Schindelin [off-list ref] writes:

quoted

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

Hi,

Johannes Schindelin [off-list ref] wrote:

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

quoted

Johannes Schindelin [off-list ref] writes:

quoted

...  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] [off-list ref]
-- 
Es gibt nichts schöneres als dem Schweigen eines Dummkopfes zuzuhören.
   	       		      	  	        (Helmut Quatlinger)

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 [off-list ref] wrote:

quoted

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

quoted

Johannes Schindelin [off-list ref] writes:

quoted

...  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_!

quoted

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.

quoted

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

quoted

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.

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

Hi,

Johannes Schindelin schrieb am Mon 28. Apr, 14:42 (+0100):

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

quoted

Johannes Schindelin [off-list ref] wrote:

quoted

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

quoted

Johannes Schindelin [off-list ref] writes:

quoted

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

quoted

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.

quoted

quoted

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

quoted

quoted

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

quoted

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?

quoted

quoted

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

quoted

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.

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)

Hi,

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

Johannes Schindelin schrieb am Mon 28. Apr, 14:42 (+0100):

quoted

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

quoted

Johannes Schindelin [off-list ref] wrote:

quoted

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

quoted

Johannes Schindelin [off-list ref] writes:

quoted

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

quoted

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.

quoted

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.

quoted

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