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

From: Junio C Hamano <junio@pobox.com>
Date: Apr 11, 2008, 23:48 UTC
Message-ID: <7vskxsneau.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1207785521-27742-2-git-send-email-joerg@alea.gnuu.de>

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

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

Show 6 quoted lines

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

Bashism is not appreciated here.

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

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