Re: [PATCH/RFCv5 2/3] git rebase -i: warn about removed commits

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:

> Check if commits were removed (i.e. a line was deleted) and print
> warnings or stop git rebase depending on the value of the
> configuration variable rebase.missingCommitsCheck.
>
> This patch gives the user the possibility to avoid silent loss of
> information (losing a commit through deleting the line in this case)
> if he wants.
>
> Add the configuration variable rebase.missingCommitsCheck.
>     - When unset or set to "ignore", no checking is done.
>     - When set to "warn", the commits are checked, warnings are
>       displayed but git rebase still proceeds.
>     - When set to "error", the commits are checked, warnings are
>       displayed and the rebase is stopped.
>       (The user can then use 'git rebase --edit-todo' and
>       'git rebase --continue', or 'git rebase --abort')
>
> rebase.missingCommitsCheck defaults to "ignore".
>
> Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>
> ---
>  In git-rebase--interactive, in the error case of check_todo_list, I
>  added 'git checkout $onto' so that using 'die' for the error allows
>  to use 'git rebase --edit-todo' (otherwise HEAD would not have been
>  changed and it still would be placed after the commits of the
>  rebase).
>  Since now it doesn't abort the rebase, the documentation and the
>  messages in the case error have changed.
>  I moved the error case away from the initial test case for missing
>  commits as to prepare for 3/3 part of the patch. It is something that
>  was advised by Eric Sunshine when I checked both missing and
>  duplicated commits, but that I removed it when removing the checking
>  for duplicated commits since there was only one test. However I add
>  it again since 3/3 will add more checking.
>  I use the variable raiseError that I affect if the error must be
>  raised instead of testing directly because I think it makes more
>  sense with 3/3 and if we add other check in the future since it adds
>  more possible errors (the test for the error case if not something
>  like 'if (test checkLevel = error && test -s todo.miss) || test cond2
>  || test cond3 || ...').
>  I am wondering if a check_todo_list call should be added in the
>  '--continue' part of the code: with this patch, the checking is only
>  done once, if the user doesn't edit correctly with 'git rebase
>  --edit-todo', it won't be picked by this.
>  In the tests in t3404 I now also test that the warning/error messages
>  are correct.
>  I tried to not change this patch too much since it was already
>  heavily reviewed, but there are some changes (mostly the ones
>  mentionned above).
>
>  Documentation/config.txt      | 11 +++++
>  Documentation/git-rebase.txt  |  6 +++
>  git-rebase--interactive.sh    | 96 +++++++++++++++++++++++++++++++++++++++++++
>  t/t3404-rebase-interactive.sh | 66 +++++++++++++++++++++++++++++
>  4 files changed, 179 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 4d21ce1..25b2a04 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2160,6 +2160,17 @@ rebase.autoStash::
>  	successful rebase might result in non-trivial conflicts.
>  	Defaults to false.
>  
> +rebase.missingCommitsCheck::
> +	If set to "warn", git rebase -i will print a warning if some
> +	commits are removed (e.g. a line was deleted), however the
> +	rebase will still proceed. If set to "error", it will print
> +	the previous warning and stop the rebase, 'git rebase
> +	--edit-todo' can then be used to correct the error. If set to
> +	"ignore", no checking is done.
> +	To drop a commit without warning or error, use the `drop`
> +	command in the todo-list.
> +	Defaults to "ignore".
> +
>  receive.advertiseAtomic::
>  	By default, git-receive-pack will advertise the atomic push
>  	capability to its clients. If you don't want to this capability
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 34bd070..2ca3b8d 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -213,6 +213,12 @@ rebase.autoSquash::
>  rebase.autoStash::
>  	If set to true enable '--autostash' option by default.
>  
> +rebase.missingCommitsCheck::
> +	If set to "warn", print warnings about removed commits in
> +	interactive mode. If set to "error", print the warnings and
> +	stop the rebase. If set to "ignore", no checking is
> +	done. "ignore" by default.
> +
>  OPTIONS
>  -------
>  --onto <newbase>::
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 72abf90..68a71d0 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -834,6 +834,100 @@ add_exec_commands () {
>  	mv "$1.new" "$1"
>  }
>  
> +# Print the list of the SHA-1 of the commits
> +# from stdin to stdout
> +todo_list_to_sha_list () {
> +	git stripspace --strip-comments |
> +	while read -r command sha1 rest
> +	do
> +		case $command in
> +		"$comment_char"*|''|noop|x|"exec")
> +			;;
> +		*)
> +			printf "%s\n" "$sha1"
> +			;;
> +		esac
> +	done
> +}
> +
> +# Use warn for each line of a file
> +# $1: file
> +warn_file () {
> +	while read -r line
> +	do
> +		warn " - $line"
> +	done <"$1"
> +}
> +
> +# Check if the user dropped some commits by mistake
> +# Behaviour determined by rebase.missingCommitsCheck.
> +check_todo_list () {
> +	raiseError=f
> +
> +	checkLevel=$(git config --get rebase.missingCommitsCheck)
> +	checkLevel=${checkLevel:-ignore}
> +	# Don't be case sensitive
> +	checkLevel=$(echo "$checkLevel" | tr 'A-Z' 'a-z')

Avoid echo on user-supplied data. If $checkLevel starts with a "-", the
behavior is platform-dependant. Should not happen if the user is
sensible, but using

  printf '%s' "$checkLevel"

is safer.

> +			opt="--no-walk=sorted --format=oneline --abbrev-commit --stdin"
> +			git rev-list $opt <"$todo".miss >"$todo".miss+
> +			mv "$todo".miss+ "$todo".miss
> +
> +			warn "Warning: some commits may have been dropped" \
> +				"accidentally."
> +			warn "Dropped commits (newer to older):"
> +			warn_file "$todo".miss

I would find it more elegant with less intermediate files, like

git rev-list $opt <"$todo".miss | while read -r line
do
	warn " - $line"
done

> +	if test $raiseError = t
> +	then
> +		# Checkout before the first commit of the
> +		# rebase: this way git rebase --continue
> +		# will work correctly as it expects HEAD to be
> +		# placed before the commit of the next action
> +		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
> +		output git checkout $onto || die_abort "could not detach HEAD"
> +		git update-ref ORIG_HEAD $orig_head

This is cut-and-pasted from below in the same file. It would deserve a
helper function I think.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/