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

Hi,

On Wed, 2 Jul 2008, Junio C Hamano wrote:

Stephan Beyer [off-list ref] writes:

quoted

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.

...

quoted

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

My fault.  I like "perform".

quoted

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

Submodules are not updated by checkout.  Indeed, the _only_ Git command 
that actually changes the state of a submodule is "git submodule update".

Therefore, it is wrong to assume that rebase/am/whatever works with 
submodules as far as the working directory is concerned.  Updating 
submodules with any Git command other than "git submodule update" is a 
_pure_ index operation.

Of course, that means that if you use rebase -i's "edit" command to go 
back to a certain revision, edit that, and want to test, it is _your_ 
responsibility to make sure that the submodules are at their correct 
revision.

Are there cases where submodules should not be ignored?

With above reasoning, it would always be wrong for sequencer to require 
the submodules to be up-to-date.

quoted

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

As "count" and "total" are only used for the progress output, anything 
else would not make sense.

quoted

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

Yes.  I had exactly that need a few days ago, where I wanted to import a 
few zips, and rebase them on top of an existing branch (which was 
generated in the same manner).

I worked around that limitation of rebase (actually, I only tried rebase 
-i, come to think of it), by rewriting the first commit object, inserting 
the "parent" line.  "fast-export > a1; vi a1; fast-import < a1" can be so 
much fun!

quoted

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

Again, my fault (as this was most likely copied from rebase -i).  All the 
files written to $DOTEST in rebase -i should be $IFS safe.

quoted

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

I think this just mimicks "git commit".

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.

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

Yes.

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

I think it should, again for the same reason why a merge can result in a 
fast-forward.

quoted

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

Right, again my fault.

quoted

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

Yes, this is wrong.  it must be

	sed -n -e '/^#/d' -e '1s .*$//p' < "$TODO"

quoted

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

I'd not check in sequencer for the strategy.  Especially given that we 
want to support user-written strategies in the future.

Ciao,
Dscho