Re: [RFC/PATCH 1/4] Add git-sequencer shell prototype

Hi,

On Fri, Jul 04, 2008 at 01:53:21AM +0200, Johannes Schindelin wrote:

On Thu, 3 Jul 2008, Stephan Beyer wrote:

quoted

Btw, another root commit problem is btw that it's not possible to 
cherry-pick root commits.

That is a problem to be fixed in cherry-pick, not in sequencer.  Care to 
take care of that?

Not at the moment but that's one of the things I note down for later ;-)

And btw, somehow it is still open for me if builtin sequencer should be
a git-cherry-pick user (for pick) or if git-cherry-pick should be a
sequencer user (which would result in a change of usage on cherry-pick
conflicts).

quoted

Johannes Schindelin wrote:

quoted

quoted

quoted

+# Usage: pick_one (cherry-pick|revert) [-*|--edit] sha1
+pick_one () {
+	what="$1"
+	# we just assume that this is either cherry-pick or revert
+	shift
+
+	# check for fast-forward if no options are given
+	if expr "x$1" : 'x[^-]' >/dev/null
+	then
+		test "$(git rev-parse --verify "$1^")" = \
+			"$(git rev-parse --verify HEAD)" &&
+			output git reset --hard "$1" &&
+			return
+	fi
+	test "$1" != '--edit' -a "$what" = 'revert' &&
+		what='revert --no-edit'

This looks somewhat wrong.

When the history looks like ---A---B and we are at A, cherry-picking B can
be optimized to just advancing to B, but that optimization has a slight
difference (or two) in the semantics.

 (1) The committer information would not record the user and time of the
     sequencer operation, which actually may be a good thing.

This is debatable.  But I think you are correct, for all the same reasons 
why a merge can result in a fast-forward.

Dscho, you mean me by referring to 'you' here, right?

Nope.

quoted

Otherwise I'm a bit confused: "For the same reasons why a merge can 
result in a fast-forward we should not do fast forward here" ;-)

What I meant: there is no use here to redo it.  It has already be done, 
and redoing just pretends that the girl calling sequencer tried to pretend 
that she did it.

If the merge has been done already, it should not be redone.

Only if the user _explicitely_ specified a merge strategy, there _might_ 
be a reason to redo the merge, but I still doubt it.

I don't get the light bulb.  You're talking about "the merge", I am
talking about fast-forward on picks.
Perhaps I got Junio wrong, too.

I try a simple example just to go sure that we're talking about the
same.

We have commits

  A ---- B ---- C ---- D
       HEAD

A is parent of B, B of C, C of D.

Now we do:
	pick C
	pick --signoff D
(Assume that the Signed-off-by: line is missing on D)

Without fast-forward, we get

  A ---- B ---- C ---- D
          \
           `--- C'---- D'
                     HEAD

C' differs in C only in the committer data, perhaps only committer date.

With fast-forward, we get:

  A ---- B ---- C ---- D
                 \
                  `--- D'
                     HEAD

If Junio meant with

 (1) The committer information would not record the user and time of the
     sequencer operation, which actually may be a good thing.

that he thinks the first variant is the way to go, I strongly disagree.
But perhaps I'm getting everyone wrong these days ;)

quoted

quoted

quoted

 (2) When $what is revert, this codepath shouldn't be exercised, 
 should it?

Yes.

I haven't done a check intentionally, but there was a stupid thinko.
So you're right.

But: this will only be a bug if the commit that _comes next in the
original history_ is to be reverted.

Does not matter.  It's a bug.

A bug is almost always in the details, a corner-case, but it almost always 
needs fixing nevertheless.

Of course ;)

quoted

Nonetheless, purely tested:

"Nevertheless", maybe?  "untested", maybe?

No, I tested it once. ;-)
(For the new single-quoted variant I've changed the author name in
t3350).

quoted

Johannes Schindelin wrote:

quoted

I'd not check in sequencer for the strategy.  Especially given that we 
want to support user-written strategies in the future.

I don't know how this is planned to look like, but perhaps 
--list-strategies may make sense here, too.

No.  You just do not check for strategies.  Period.  git-merge does that, 
and you can easily abort a rebase if you explicitely asked for an invalid 
strategy.

Hmm, my dream of the "robust sequencing after sanity check passed" is
dead with your "period".
So I'll have to check what happens, when e.g. "--strategy=hours" is used.
(I mean, you should be in a safe state to do git sequencer --edit and
correct "hours" to "ours'.)

Regards,
  Stephan

-- 
Stephan Beyer [off-list ref], PGP 0x6EDDD207FCC5040F