git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jul 3, 2008, 01:45 UTC
Message-ID
<7vbq1f68rh.fsf@gitster.siamese.dyndns.org>
In-Reply-To
<1214879914-17866-2-git-send-email-s-beyer@gmx.net>
Stephan Beyer <s-beyer@gmx.net> 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.
...
Show 16 quoted lines
> +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".
Show 8 quoted lines
> +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?

Show 9 quoted lines
> +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).

Show 15 quoted lines
> +		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?

Show 18 quoted lines
> +	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?
Show 16 quoted lines
> +	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...

Show 16 quoted lines
> +# 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
Show 30 quoted lines
> +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
Show 8 quoted lines
> +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?

Show 12 quoted lines
> +### 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?

Show 7 quoted lines
> +	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.
Show 24 quoted lines
> +		# 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?

Show 13 quoted lines
> +# 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..
Previous: Karl HasselströmNext: Johannes Schindelin
Message 22 of 52 in “git sequencer prototype”
  1. Stephan BeyerJul 1, 2008
  2. 1/4 Add git-sequencer shell prototypeStephan Beyer, Jul 1, 2008
  3. 2/4 Add git-sequencer prototype documentationStephan Beyer, Jul 1, 2008
  4. 3/4 Add git-sequencer test suite (t3350)Stephan Beyer, Jul 1, 2008
  5. 4/4 Migrate git-am to use git-sequencerStephan Beyer, Jul 1, 2008
  6. git-rebase-i migration to sequencerStephan Beyer, Jul 1, 2008
  7. 1/2 Make rebase--interactive use OPTIONS_SPECStephan Beyer, Jul 1, 2008
  8. 2/2 Migrate git-rebase--i to use git-sequencerStephan Beyer, Jul 1, 2008
  9. Stephan BeyerJul 5, 2008
  10. Junio C HamanoJul 5, 2008
  11. Jakub NarebskiJul 1, 2008
  12. Stephan BeyerJul 1, 2008
  13. Jakub NarebskiJul 1, 2008
  14. Stephan BeyerJul 1, 2008
  15. Jakub NarebskiJul 2, 2008
  16. Junio C HamanoJul 2, 2008
  17. Stephan BeyerJul 2, 2008
  18. 2/4 Add git-sequencer prototype documentationStephan Beyer, Jul 5, 2008
  19. Jakub NarebskiJul 8, 2008
  20. Stephan BeyerJul 8, 2008
  21. Karl HasselströmJul 9, 2008
  22. Junio C HamanoJul 3, 2008
  23. Johannes SchindelinJul 3, 2008
  24. Stephan BeyerJul 3, 2008
  25. Junio C HamanoJul 3, 2008
  26. Stephan BeyerJul 3, 2008
  27. Stephan BeyerJul 3, 2008
  28. Johannes SchindelinJul 3, 2008
  29. Stephan BeyerJul 4, 2008
  30. Johannes SchindelinJul 4, 2008
  31. Stephan BeyerJul 4, 2008
  32. Allow cherry-picking root commitsJohannes Schindelin, Jul 4, 2008
  33. Stephan BeyerJul 4, 2008
  34. Junio C HamanoJul 6, 2008
  35. Johannes SchindelinJul 6, 2008
  36. t3503: Add test case for identical filesStephan Beyer, Jul 6, 2008
  37. Stephan BeyerJul 6, 2008
  38. Johannes SchindelinJul 6, 2008
  39. Stephan BeyerJul 4, 2008
  40. Johannes SchindelinJul 4, 2008
  41. Junio C HamanoJul 3, 2008
  42. Jakub NarebskiJul 3, 2008
  43. Stephan BeyerJul 3, 2008
  44. 1/4 Add git-sequencer shell prototypeStephan Beyer, Jul 5, 2008
  45. Junio C HamanoJul 1, 2008
  46. Stephan BeyerJul 1, 2008
  47. Alex RiesenJul 4, 2008
  48. Junio C HamanoJul 4, 2008
  49. Stephan BeyerJul 4, 2008
  50. Alex RiesenJul 5, 2008
  51. Thomas AdamJul 5, 2008
  52. Johannes SchindelinJul 5, 2008

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.