[PATCH/RFC 01/10] Teach rebase interactive the mark command

[PATCH/RFC 01/10] Teach rebase interactive the mark command

[Jörg Sommer]

This new command can be used to set symbolic marks for an commit while
doing a rebase. This symbolic name can later be used for merges or
resets.
---
 git-rebase--interactive.sh    |   36 ++++++++++++++++++++++++++++++++++++
 t/t3404-rebase-interactive.sh |   21 +++++++++++++++++++++
 2 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 8aa7371..b001ddf 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -23,6 +23,7 @@ DONE="$DOTEST"/done
 MSG="$DOTEST"/message
 SQUASH_MSG="$DOTEST"/message-squash
 REWRITTEN="$DOTEST"/rewritten
+MARKS="$DOTEST"/marks
 PRESERVE_MERGES=
 STRATEGY=
 VERBOSE=
@@ -240,6 +241,23 @@ peek_next_command () {
 	sed -n "1s/ .*$//p" < "$TODO"
 }
 
+parse_mark() {
+	local tmp
+	tmp="$*"
+
+	case "$tmp" in
+	'#'[0-9]*)
+		tmp="${tmp#\#}"
+		if test "$tmp" = $(printf %d "$tmp")
+		then
+			echo $tmp
+			return 0
+		fi
+		;;
+	esac
+	return 1
+}
+
 do_next () {
 	rm -f "$DOTEST"/message "$DOTEST"/author-script \
 		"$DOTEST"/amend || exit
@@ -317,6 +335,23 @@ do_next () {
 			die_with_patch $sha1 ""
 		fi
 		;;
+	mark|a)
+		if ! mark=$(parse_mark $sha1)
+		then
+			warn "Invalid mark given: $command $sha1 $rest"
+			die_with_patch $sha1 \
+				"Please fix this in the file $TODO."
+		fi
+		mark_action_done
+
+		test -d "$MARKS" || mkdir "$MARKS"
+
+		test -e "$MARKS"/$mark && \
+			warn "mark $mark already exist; overwriting it"
+
+		git rev-parse --verify HEAD > "$MARKS"/$mark || \
+			die "HEAD is invalid"
+		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"
 		die_with_patch $sha1 "Please fix this in the file $TODO."
@@ -533,6 +568,7 @@ do
 #  pick = use commit
 #  edit = use commit, but stop for amending
 #  squash = use commit, but meld into previous commit
+#  mark #NUM = mark the current HEAD for later reference
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9cf873f..4461674 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -82,6 +82,9 @@ for line in $FAKE_LINES; do
 	case $line in
 	squash|edit)
 		action="$line";;
+	mark*)
+		echo "mark ${line#mark}"
+		echo "mark ${line#mark}" >> "$1";;
 	*)
 		echo sed -n "${line}s/^pick/$action/p"
 		sed -n "${line}p" < "$1".tmp
@@ -189,6 +192,24 @@ test_expect_success '-p handles "no changes" gracefully' '
 	test $HEAD = $(git rev-parse HEAD)
 '
 
+test_expect_success 'setting an invalid mark fails' '
+	export FAKE_LINES="mark12 1" && \
+	test_must_fail git rebase -i HEAD~1 &&
+	unset FAKE_LINES &&
+	git rebase --abort
+'
+
+test_expect_success 'setting marks works' '
+	git checkout master &&
+	FAKE_LINES="mark#0 2 1 mark#42 3 edit 4" git rebase -i HEAD~4 &&
+	marks_dir=.git/.dotest-merge/marks
+	test -d $marks_dir &&
+	test $(ls $marks_dir | wc -l) -eq 2 &&
+	test "$(git rev-parse HEAD~4)" = "$(cat $marks_dir/0)" &&
+	test "$(git rev-parse HEAD~2)" = "$(cat $marks_dir/42)" &&
+	git rebase --abort
+'
+
 test_expect_success 'preserve merges with -p' '
 	git checkout -b to-be-preserved master^ &&
 	: > unrelated-file &&
-- 
1.5.4.5

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Mike Ralphson]

On 10/04/2008, Jörg Sommer <joerg@alea.gnuu.de> wrote:
> This new command can be used to set symbolic marks for an commit while
>  doing a rebase. This symbolic name can later be used for merges or
>  resets.

What would be wrong with using the existing tag machinery for this instead?

Mike

PS: Apologies to anyone who got an html version of this mail.

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Junio C Hamano]

Jörg Sommer <joerg@alea.gnuu.de> writes:

> This new command can be used to set symbolic marks for an commit while
> doing a rebase. This symbolic name can later be used for merges or
> resets.
> ---

Comments as requested...

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 8aa7371..b001ddf 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -23,6 +23,7 @@ DONE="$DOTEST"/done
>  MSG="$DOTEST"/message
>  SQUASH_MSG="$DOTEST"/message-squash
>  REWRITTEN="$DOTEST"/rewritten
> +MARKS="$DOTEST"/marks

I wonder if this should live somewhere inside $GIT_DIR/refs namespace, so
that if any object pruning is triggered ever while rebasing interactively
the objects that marks require will be protected.

> @@ -240,6 +241,23 @@ peek_next_command () {
>  	sed -n "1s/ .*$//p" < "$TODO"
>  }
>  
> +parse_mark() {
> +	local tmp

Bashism is not appreciated here.

> +	case "$tmp" in
> +	'#'[0-9]*)
> +		tmp="${tmp#\#}"
> +		if test "$tmp" = $(printf %d "$tmp")
> +		then
> +			echo $tmp
> +			return 0
> +		fi
> +		;;
> +	esac

Wouldn't

	pick 5cc8f37 (init: show "Reinit" message even in ...)
	mark 1
	pick 18d077c (quiltimport: fix misquoting of parse...)
	mark 2
	reset 1
        merge #2

be easier for people?

When you reference a commit with mark name, it is reasonable to require it
to be prefixed with '#', which is a character that cannot be either in
refname nor hex representation of a commit object.  I think it is Ok to
accept an optional prefix when defining one, e.g. "mark #47", but I do not
think requiring '#' prefix when defining one is needed.

> @@ -317,6 +335,23 @@ do_next () {
>  			die_with_patch $sha1 ""
>  		fi
>  		;;
> +	mark|a)

I understand that the reason why you did not pick a more obvious 'm' is
because you would want to add 'merge' later and give it 'm', but we do not
have to give a single-letter equivalent to all commands especially when
there is not an appropriate one.

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Jörg Sommer]

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

Hi Junio,

Junio C Hamano schrieb am Fri 11. Apr, 16:48 (-0700):
> Jörg Sommer <joerg@alea.gnuu.de> writes:
> 
> > This new command can be used to set symbolic marks for an commit while
> > doing a rebase. This symbolic name can later be used for merges or
> > resets.
> > ---
> 
> Comments as requested...

Thanks.

> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 8aa7371..b001ddf 100755
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -23,6 +23,7 @@ DONE="$DOTEST"/done
> >  MSG="$DOTEST"/message
> >  SQUASH_MSG="$DOTEST"/message-squash
> >  REWRITTEN="$DOTEST"/rewritten
> > +MARKS="$DOTEST"/marks
> 
> I wonder if this should live somewhere inside $GIT_DIR/refs namespace, so
> that if any object pruning is triggered ever while rebasing interactively
> the objects that marks require will be protected.

Why wasn't the rewritten directory underneath refs/?

> > @@ -240,6 +241,23 @@ peek_next_command () {
> >  	sed -n "1s/ .*$//p" < "$TODO"
> >  }
> >  
> > +parse_mark() {
> > +	local tmp
> 
> Bashism is not appreciated here.

“IEEE P1003.2 Draft 11.2 - September 1991”, page 277:

 Local variables within a function were considered and included in Draft
 9 (controlled by the special built-in local), but were removed because
 they do not fit the simple model developed for the scoping of functions
 and there was some opposition to adding yet another new special built-in
 from outside existing practice.  Implementations should reserve the
 identifier local (as well as typeset, as used in the KornShell) in case
 this local variable mechanism is adopted in a future version of POSIX.2.

… but I didn't found it in “IEEE Std 1003.1, 2004 Edition”.

> > +	case "$tmp" in
> > +	'#'[0-9]*)
> > +		tmp="${tmp#\#}"
> > +		if test "$tmp" = $(printf %d "$tmp")
> > +		then
> > +			echo $tmp
> > +			return 0
> > +		fi
> > +		;;
> > +	esac
> 
> Wouldn't
> 
> 	pick 5cc8f37 (init: show "Reinit" message even in ...)
> 	mark 1
> 	pick 18d077c (quiltimport: fix misquoting of parse...)
> 	mark 2
> 	reset 1

“reset 18d077c~2” or “reset some-tag” or “reset my-branch~12”

>         merge #2
> 
> be easier for people?

I don't know. Using the special sign everywhere a mark is used looks more
consistent to me. The only case where it might be omitted is the mark
command, because it only uses marks.

> When you reference a commit with mark name, it is reasonable to require it
> to be prefixed with '#', which is a character that cannot be either in
> refname nor hex representation of a commit object.  I think it is Ok to
> accept an optional prefix when defining one, e.g. "mark #47", but I do not
> think requiring '#' prefix when defining one is needed.

That sounds useful.

BTW: Should the mark be a number or a string, e.g. 001 == 1 or 001 != 1?

> > @@ -317,6 +335,23 @@ do_next () {
> >  			die_with_patch $sha1 ""
> >  		fi
> >  		;;
> > +	mark|a)
> 
> I understand that the reason why you did not pick a more obvious 'm' is
> because you would want to add 'merge' later and give it 'm', but we do not
> have to give a single-letter equivalent to all commands especially when
> there is not an appropriate one.

That's fine. I thought it's a requirement.

Bye, Jörg.
-- 
Was der Bauer nicht kennt, das frisst er nicht. Würde der Städter kennen,
was er frisst, er würde umgehend Bauer werden.
                                                       Oliver Hassencamp

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

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Jörg Sommer]

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

Hallo Mike,

Mike Ralphson schrieb am Thu 10. Apr, 10:33 (+0100):
> On 10/04/2008, Jörg Sommer <joerg@alea.gnuu.de> wrote:
> > This new command can be used to set symbolic marks for an commit while
> >  doing a rebase. This symbolic name can later be used for merges or
> >  resets.
> 
> What would be wrong with using the existing tag machinery for this instead?

You may have to deal with conflicts if users named tags 03 or 10. But
Junio suggested to use a ref, too. I think refs/rebase-marks/ is a good
prefix.

Bye, Jörg.
-- 
Der Hase läuft schneller als der Fuchs,
denn der Hase läuft um sein Leben.

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

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Shawn O. Pearce]

Jrg Sommer <joerg@alea.gnuu.de> wrote:
> > Wouldn't
> > 
> > 	pick 5cc8f37 (init: show "Reinit" message even in ...)
> > 	mark 1
> > 	pick 18d077c (quiltimport: fix misquoting of parse...)
> > 	mark 2
> > 	reset 1
> 
> “reset 18d077c~2” or “reset some-tag” or “reset my-branch~12”
> 
> >         merge #2
> > 
> > be easier for people?
> 
> I don't know. Using the special sign everywhere a mark is used looks more
> consistent to me. The only case where it might be omitted is the mark
> command, because it only uses marks.

Why not use the mark syntax that fast-import uses?  In fast-import
we use ":n" anytime we need to refer to a mark, e.g. ":1" or ":5".
Its the same idea.  We already have a language for it.  Heck, the
commands above are bordering on a language not too far from the
one that fast-import accepts.  :-)

-- 
Shawn.

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Jörg Sommer]

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

Hi Shawn,

Shawn O. Pearce schrieb am Sat 12. Apr, 23:56 (-0400):
> Jrg Sommer <joerg@alea.gnuu.de> wrote:
> > > Wouldn't
> > > 
> > > 	pick 5cc8f37 (init: show "Reinit" message even in ...)
> > > 	mark 1
> > > 	pick 18d077c (quiltimport: fix misquoting of parse...)
> > > 	mark 2
> > > 	reset 1
> > 
> > “reset 18d077c~2” or “reset some-tag” or “reset my-branch~12”
> > 
> > >         merge #2
> > > 
> > > be easier for people?
> > 
> > I don't know. Using the special sign everywhere a mark is used looks more
> > consistent to me. The only case where it might be omitted is the mark
> > command, because it only uses marks.
> 
> Why not use the mark syntax that fast-import uses?

I didn't know it.

> In fast-import we use ":n" anytime we need to refer to a mark, e.g.
> ":1" or ":5".

Currently, I don't restrict the mark to be a number. It can anything that
is a valid ref. Should I restrict it?

And how do you handle the :/ syntax? “reset :/Bla” is than not valid.
Mmm, I'll add an exception for :/.

Except of this, I prefer to use the colon to be much closer to the syntax
of fast-import.

Bye, Jörg.
-- 
Der Wunsch, klug zu erscheinen, verhindert oft, es zu werden.
    	    	    			      (Francois de la Rochefoucauld)

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

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Paul Fredrickson]

> Jrg Sommer <joerg@alea.gnuu.de> wrote:
> > > Wouldn't
> > >
> > > pick 5cc8f37 (init: show "Reinit" message even in ...)
> > > mark 1
> > > pick 18d077c (quiltimport: fix misquoting of parse...)
> > > mark 2
> > > reset 1
> >
> > "reset 18d077c~2" or "reset some-tag" or "reset my-branch~12"
> >
> >         merge #2
> > >
> > > be easier for people?
> >
> > I don't know. Using the special sign everywhere a mark is used looks more
> > consistent to me. The only case where it might be omitted is the mark
> > command, because it only uses marks.
>
> Why not use the mark syntax that fast-import uses?  In fast-import
> we use ":n" anytime we need to refer to a mark, e.g. ":1" or ":5".
> Its the same idea.  We already have a language for it.  Heck, the
> commands above are bordering on a language not too far from the
> one that fast-import accepts.  :-)

I like the idea of adding marks to an interactive rebase in general, but instead
of adding a separate command, what if rebase *automatically* marked all the
commits in the session:

    1: pick 5cc8f37 (init: show "Reinit" message even in ...)
    2: pick 18d007c (quiltimport: fix misquoting of parse ...)
    reset 1
    merge 2

or "reset :1" and "merge :2".  Neither notation bothers me for marks.

--Paul

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Shawn O. Pearce]

Jrg Sommer <joerg@alea.gnuu.de> wrote:
> Shawn O. Pearce schrieb am Sat 12. Apr, 23:56 (-0400):
> > 
> > Why not use the mark syntax that fast-import uses?
> 
> I didn't know it.
> 
> > In fast-import we use ":n" anytime we need to refer to a mark, e.g.
> > ":1" or ":5".
> 
> Currently, I don't restrict the mark to be a number. It can anything that
> is a valid ref. Should I restrict it?

In fast-import a mark can *only* be a number.  It cannot be a ref
string or anything complex like that.  This reduces the memory load
of fast-import, but does cause a burden on the import frontend.
 
> And how do you handle the :/ syntax? “reset :/Bla” is than not valid.
> Mmm, I'll add an exception for :/.

I think the ':/' syntax came along after fast-import had already started
to use ':' as the mark syntax.  I forgot to object to this bastard form
of looking up a commit when it was introduced by Dscho and now we have
a SHA-1 expression syntax that fast-import will confuse with a mark.  I
originally had chosen to start a mark off with ':' because it is not an
allowed character in a ref, due to its use to split src:dst in a fetch
or push refspec.
 
> Except of this, I prefer to use the colon to be much closer to the syntax
> of fast-import.

Me too, but it looks like in a human edited "TODO" script we may want
to be more friendly and allow named marks.  Though I'm not sure that is
really all that useful.  If you are merging something because it used to
be merged before the rebase I doubt we'd generate a meaningful mark name
when the TODO script is initially formatted.

-- 
Shawn.

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

>> Except of this, I prefer to use the colon to be much closer to the syntax
>> of fast-import.
>
> Me too, but it looks like in a human edited "TODO" script we may want
> to be more friendly and allow named marks.  Though I'm not sure that is
> really all that useful.  If you are merging something because it used to
> be merged before the rebase I doubt we'd generate a meaningful mark name
> when the TODO script is initially formatted.

I'd say a small integer is the only thing we would need.  The TODO insn
sequence would be machine generated then manually tweaked, not the other
way around.

Regarding colon vs pound, I would say that the 'mark' insn can just say
"2" to set the second mark, and then store it in refs/marks/2; the
commands that refer to the commit later can use the usual "refs/marks/2"
or "marks/2" syntax without colon nor pound nor any other special syntax
that way.

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Jörg Sommer]

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

Hello Paul,

Paul Fredrickson schrieb am Sun 13. Apr, 13:51 (-0700):
> > Jrg Sommer <joerg@alea.gnuu.de> wrote:
> > > > Wouldn't
> > > >
> > > > pick 5cc8f37 (init: show "Reinit" message even in ...)
> > > > mark 1
> > > > pick 18d077c (quiltimport: fix misquoting of parse...)
> > > > mark 2
> > > > reset 1
> 
> I like the idea of adding marks to an interactive rebase in general, but instead
> of adding a separate command, what if rebase *automatically* marked all the
> commits in the session:
> 
>     1: pick 5cc8f37 (init: show "Reinit" message even in ...)
>     2: pick 18d007c (quiltimport: fix misquoting of parse ...)
>     reset 1
>     merge 2

This format would be incompatible with the current format and it makes
the parsing a little bit more difficult; the first column contains a mark
or a command. No, I think that's not a good idea.

Have a nice day, Jörg.
-- 
Der Hase läuft schneller als der Fuchs,
denn der Hase läuft um sein Leben.

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

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Jörg Sommer]

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

Hi Shawn,

Shawn O. Pearce schrieb am Mon 14. Apr, 02:24 (-0400):
> Jrg Sommer <joerg@alea.gnuu.de> wrote:
> > Shawn O. Pearce schrieb am Sat 12. Apr, 23:56 (-0400):
> > > 
> > > Why not use the mark syntax that fast-import uses?
> > 
> > I didn't know it.
> > 
> > > In fast-import we use ":n" anytime we need to refer to a mark, e.g.
> > > ":1" or ":5".
> > 
> > Currently, I don't restrict the mark to be a number. It can anything that
> > is a valid ref. Should I restrict it?
> 
> In fast-import a mark can *only* be a number.

Only for the record: fast-import uses numbers and not strings of
digits, i.e. 001 == 1, and it ignores stuff following digits, i.e.
"12a" == 12 and "abc" == 0.

> > Except of this, I prefer to use the colon to be much closer to the syntax
> > of fast-import.
> 
> Me too, but it looks like in a human edited "TODO" script we may want
> to be more friendly and allow named marks.

I don't think so. There shouldn't be so much marks that a user can't
remember them.

Thanks for your comments.

Jörg.
-- 
The UNIX Guru's View of Sex:
# unzip ; strip ; touch ; finger ; mount ; fsck ; more ; yes ; umount ; sleep

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

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Johannes Schindelin]

Hi,

On Sun, 13 Apr 2008, Paul Fredrickson wrote:

> > Jrg Sommer <joerg@alea.gnuu.de> wrote:
> > > > Wouldn't
> > > >
> > > > pick 5cc8f37 (init: show "Reinit" message even in ...)
> > > > mark 1
> > > > pick 18d077c (quiltimport: fix misquoting of parse...)
> > > > mark 2
> > > > reset 1
> > >
> > > "reset 18d077c~2" or "reset some-tag" or "reset my-branch~12"
> > >
> > >         merge #2
> > > >
> > > > be easier for people?

Actually, I think that this whole "mark" stuff is way too complicated, as 
can be seen by the amount of patches needed to get it somewhere usable.

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.e. like with filter-branch, and like with rebase -i -p in its current 
form, we take the _original_ names as keys as to which commits to merge, 
or where to reset to.

That would be relatively easy to implement, since the whole infrastructure 
for it is already there: whenever a commit was rewritten, the new commit 
name is saved in $DOTEST/rewritten/<original-commit-name>.

I really do not like complicating things unnecessarily.

Ciao,
Dscho

Re: [PATCH/RFC 01/10] Teach rebase interactive the mark command

[Junio C Hamano]

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

> 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.e. like with filter-branch, and like with rebase -i -p in its current 
> form, we take the _original_ names as keys as to which commits to merge, 
> or where to reset to.

While the need probably would not be felt strongly if we design this only
for rebase -i, I suspect that you would want to have two kinds of reset if
you go that route.  There might be some other insn that may have similar
issues.

For example, imagine a case where you want to create a merge with a
recontructed side branch.  First you grow the branch you would merge into,
with a sequence:

	pick A
        pick B
        pick C

Then in order to reconstruct a side branch that begins from a known point,
say the tip of "master", you would want to reset to a commit that is
outside of the scope of this rewriting.  And then you rebuild that side
branch:

	reset master
        pick D
        pick E

And finally (and this step shows the beauty of your approach), come back
to the other tip and make the merge:

	reset C
        merge E

Two resets above would have different semantics.  The former resets to
unwritten, and the latter rewritten.