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

[Junio C Hamano <gitster@xxxxxxxxx>]

Stephan Beyer <s-beyer@xxxxxxx> writes:

> git sequencer is planned as a backend for user scripts
> that execute a sequence of git instructions and perhaps
> need manual intervention, for example git-rebase or git-am.

...
> +output () {
> +	case "$VERBOSE" in
> +	0)
> +		"$@" >/dev/null
> +		;;
> +	1)
> +		output=$("$@" 2>&1 )
> +		status=$?
> +		test $status -ne 0 && printf '%s\n' "$output"
> +		return $status
> +		;;
> +	2)
> +		"$@"
> +		;;
> +	esac
> +}

Perhaps misnamed?  This feels more like "do" or "perform" or "run".

> +require_clean_work_tree () {
> +	# test if working tree is dirty
> +	git rev-parse --verify HEAD >/dev/null &&
> +	git update-index --ignore-submodules --refresh &&
> +	git diff-files --quiet --ignore-submodules &&
> +	git diff-index --cached --quiet HEAD --ignore-submodules -- ||
> +	die 'Working tree is dirty'
> +}

When is it necessary to ignore submodules and why?  Are there cases where
submodules should not be ignored?

> +LAST_COUNT=
> +mark_action_done () {
> +	sed -e 1q <"$TODO" >>"$DONE"
> +	sed -e 1d <"$TODO" >"$TODO.new"
> +	mv -f "$TODO.new" "$TODO"
> +	if test "$VERBOSE" -gt 0
> +	then
> +		count=$(grep -c '^[^#]' <"$DONE")
> +		total=$(expr "$count" + "$(grep -c '^[^#]' <"$TODO")")

Here we are not counting lines that are comments as insns (I am not
complaining; just making a mental note).

> +		if test "$LAST_COUNT" != "$count"
> +		then
> +			LAST_COUNT="$count"
> +			test "$VERBOSE" -lt 1 ||
> +				printf 'Sequencing (%d/%d)\r' "$count" "$total"
> +			test "$VERBOSE" -lt 2 || echo
> +		fi
> +	fi
> +}
> +
> +# Generate message, patch and author script files
> +make_patch () {
> +	parent_sha1=$(git rev-parse --verify "$1"^) ||
> +		die "Cannot get patch for $1^"
> +	git diff-tree -p "$parent_sha1..$1" >"$PATCH"

Could there be a case where we need/want to deal with a root commit
without parents?

> +	test -f "$MSG" ||
> +		commit_message "$1" >"$MSG"
> +	test -f "$AUTHOR_SCRIPT" ||
> +		get_author_ident_from_commit "$1" >"$AUTHOR_SCRIPT"
> +}
> +
> +# Generate a patch and die with "conflict" status code
> +die_with_patch () {
> +	make_patch "$1"
> +	git rerere
> +	die_to_continue "$2"
> +}
> +
> +restore () {
> +	git rerere clear
> +
> +	HEADNAME=$(cat "$SEQ_DIR/head-name")
> +	HEAD=$(cat "$SEQ_DIR/head")

Perhaps

	read HEADNAME <"$SEQ_DIR/head-name"

provided if these values are $IFS safe?

> +	case $HEADNAME in
> +	refs/*)
> +		git symbolic-ref HEAD "$HEADNAME"
> +		;;
> +	esac &&
> +	output git reset --hard "$HEAD"
> +}
> +
> +has_action () {
> +	grep '^[^#]' "$1" >/dev/null
> +}
> +
> +# Check if text file $1 contains a commit message
> +has_message () {
> +	test -n "$(sed -n -e '/^Signed-off-by:/d;/^[^#]/p' <"$1")"
> +}

Makes one wonder if we would want to special case other kinds like
"Acked-by:" as well...

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

 (2) When $what is revert, this codepath shouldn't be exercised, should it?

 (3) If B is a merge, even if $what is pick, this codepath shouldn't be
     exercised, should it?

As to the syntax I tend to prefer

	case "$1" in
        -*)	... do option thing ... ;;
        *)	... do other thing... ;;
        esac

So how about...

	case "$what,$1" in
        revert,--edit)
        	what='revert --no-edit' ;;
        revert,* | cherry-pick,-* )
        	;;
        *)
		if ! git rev-parse --verify "$1^2" &&
	                test "$(git rev-parse --verify "$1^") = \
                	"$(git rev-parse --verify HEAD)"
		then
                	output git reset --hard "$1"
			return
		fi
		;;
	esac

> +make_squash_message () {
> +	if test -f "$squash_msg"
> +	then
> +		count=$(($(sed -n -e 's/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p' \
> +			<"$squash_msg" | sed -n -e '$p')+1))
> +		echo "# This is a combination of $count commits."
> +		sed -e '1d' -e '2,/^./{
> +			/^$/d
> +		}' <"$squash_msg"
> +	else
> +		count=2
> +		echo '# This is a combination of 2 commits.'
> +		echo '# The first commit message is:'
> +		echo
> +		commit_message HEAD
> +	fi
> +	echo
> +	echo "# This is the $(nth_string "$count") commit message:"
> +	echo
> +	commit_message "$1"
> +}
> +
> +make_squash_message_multiple () {
> +	echo '# This is a dummy to get the 0.' >"$squash_msg"
> +	for cur_sha1 in $(git rev-list --reverse "$sha1..HEAD")
> +	do
> +		make_squash_message "$cur_sha1" >"$MSG"
> +		cp "$MSG" "$squash_msg"
> +	done
> +}

Hmm, I know this is how rebase-i is written, but we should be able to do
better than writing and flipping temporary times N times, shouldn't we?

> +peek_next_command () {
> +	sed -n -e '1s/ .*$//p' <"$TODO"
> +}

... which could respond "the next command is '#' (comment)", so we are
actively counting a comment as a step here.  Does this contradict with the
mental note we made earlier, and if so, does the discrepancy hurt us
somewhere in this program?

> +# If $1 is a mark, make a ref from it; otherwise keep it
> +mark_to_ref () {
> +	arg="$1"
> +	ref=$(expr "x$arg" : 'x:0*\([0-9][0-9]*\)$')

You might want to leave comments to describe constraints that led to this
slightly awkward regexp:

 * :0 is allowed
 * :01 is the same as :1

> +strategy_check () {
> +	case "$1" in
> +	resolve|recursive|octopus|ours|subtree|theirs)
> +		return
> +		;;
> +	esac
> +	todo_warn "Strategy '$1' not known."
> +}

Hmm.  Do we need to maintain list of available strategies here and then in
git-merge separately?

> +### Author script functions
> +
> +clean_author_script () {
> +	cat "$ORIG_AUTHOR_SCRIPT" >"$AUTHOR_SCRIPT"
> +}
> +
> +# Take "Name <e-mail>" in stdin and outputs author script
> +make_author_script_from_string () {
> +	sed -e 's/^\(.*\) <\(.*\)>.*$/GIT_AUTHOR_NAME="\1"\
> +GIT_AUTHOR_EMAIL="\2"\
> +GIT_AUTHOR_DATE=/'
> +}

If you are going to "."-source or eval the output from this, you would
need to quote the values a lot more robustly, wouldn't you?  Is this safe
against shell metacharacters in names, mails and/or space between unixtime
and the timezone information?

> +	if test -z "$AUTHOR"
> +	then
> +		sed -n -e '
> +			s/^Author: \(.*\)$/GIT_AUTHOR_NAME="\1"/p;
> +			s/^Email: \(.*\)$/GIT_AUTHOR_EMAIL="\1"/p;
> +			s/^Date: \(.*\)$/GIT_AUTHOR_DATE="\1"/p
> +		' <"$infofile" >>"$AUTHOR_SCRIPT"

The same comment on quoting applies here, I think.

> +		# If sed's result is empty, we keep the original
> +		# author script by appending.
> +	fi
> ...
> +	failed=
> +	with_author git apply $apply_opts --index "$PATCH" || failed=t
> +
> +	if test -n "$failed" -a -n "$threeway" && (with_author fallback_3way)
> +	then
> +		# Applying the patch to an earlier tree and merging the
> +		# result may have produced the same tree as ours.
> +		git diff-index --quiet --cached HEAD -- && {
> +			echo 'No changes -- Patch already applied.'
> +			return 0
> +			# XXX: do we want that?
> +		}
> +		# clear apply_status -- we have successfully merged.
> +		failed=
> +	fi
> +
> +	if test -n "$failed"
> +	then
> +		# XXX: This is just a stupid hack:
> +		with_author git apply $apply_opts --reject --index "$PATCH"

Please don't do this without being asked, if you are planning to use this
in "am" when 3-way fallback was not asked.  It _may_ make sense to give an
option to the users to ask for .rej if they prefer to work that way better
than working with 3-way merge fallback, but doing this without being asked
is not acceptable.

> +		die_to_continue 'Patch failed. See the .rej files.'
> +		# XXX: We actually needed a git-apply flag that creates
> +		# conflict markers and sets the DIFF_STATUS_UNMERGED flag.

That is what -3way is all about, and this codepath is when the user did
not ask for it, isn't it?

> +# Check the "pick" instruction
> +check_pick () {
> +	revert=
> +	mainline=
> +	while test $# -gt 1
> +	do
> ...
> +	done
> +
> +	if test -n "$mainline"
> +	then
> +		test -z "$revert" ||
> +			todo_error "Cannot use $revert together with --mainline."

Why not?  If you have this...

	---A---C---D
              /
          ---B

and you are at D, you may want to undo the merge you made at C and go back
to either A or B, which essentially is same as cherry-picking diff between
C and D on top of either A or B.  Both are valid operations aren't they?

The remainder of the review will have to be in a separate message..
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html