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

Hi,

On Fri, 4 Jul 2008, Stephan Beyer wrote:

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

quoted

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

Well, logically, it should come _before_ you use it in sequencer.  And you 
should use it in sequencer.

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

The thing is, sequencer and cherry-pick _both_ use the same underlying 
functionality: cherry-picking a single commit.

So, logically, that part should be moved into libgit.a, and cherry-pick 
should use the to-be-introduced sequencer() function.

quoted

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.

Ooops.  I think I was talking about a later comment of Junio's.

The thing is, if you try to pick a commit, and the current revision is 
already the parent of that commit, I think it would be wrong to redo the 
commit, pretending that the current time and committer were applying that 
commit.

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

IMO the --signoff should check if the sign off is present (must be the 
last one, as if we redid that commit), and if it is, fast-forward.

If it is missing, we need to redo the commit.

quoted

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.

Wow.  Is that the CVS/SVN merge strategy?

Do not care too much about the rare use cases.  Only experts will use -s.

But experts are known to shoot themselves, or inflict other pain on their 
bodies, so it is okay to let them suffer from a typo.

Ciao,
Dscho