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
