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

From: Jörg Sommer <joerg@alea.gnuu.de>
Date: Apr 12, 2008, 10:11 UTC
Message-ID: <20080412101110.GD31356@alea.gnuu.de>
In-Reply-To: <7vskxsneau.fsf@gitster.siamese.dyndns.org>

Hi Junio,

Junio C Hamano schrieb am Fri 11. Apr, 16:48 (-0700):

Show 8 quoted lines

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

Show 13 quoted lines

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

Show 8 quoted lines

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

Show 18 quoted lines

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

Show 5 quoted lines

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

Show 10 quoted lines

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