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

[Stephan Beyer <s-beyer@gmx.net>]

Hi,

On Fri, Jul 04, 2008 at 01:53:21AM +0200, Johannes Schindelin wrote:
> On Thu, 3 Jul 2008, Stephan Beyer wrote:
> > 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).

> > Johannes Schindelin wrote:
> > > > > +# 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.
> 
> > 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 ;)


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

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

> > Johannes Schindelin wrote:
> > > 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 <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F