Re: [PATCH/RFCv3 2/2] 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 abort git rebase according to the value of the

s/according to/depending on/

(although both translate to the same "selon" in french ;-))

> configuration variable rebase.missingCommitsCheckLevel.

Now I'm wondering whether rebase.missingCommits would be sufficient. The
full config would be

[rebase]
	missingCommits = warn

which reads like "in rebase, on missing commit, warn me".

> Add the configuration variable rebase.missingCommitsCheckLevel.
>     - 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 aborted.
>
> rebase.missingCommitsCheckLevel defaults to "ignore".

This all describes well *what* the commit is doing (which is essentially
rendundant with the documentation actually), but fails to really explain
*why*, which is the most important question to adress in a commit
message.

I'm convinced that this is a good idea (partly because I'm the one who
suggested ^^), but not everybody is.

> +rebase.missingCommitsCheckLevel::
> +	If set to "warn", git rebase -i will print a warning if some
> +	commits are removed (i.e. a line was deleted), however the
> +	rebase will still proceed. If set to "error", it will print
> +	the previous warning and abort the rebase. If set to
> +	"ignore", no checking is done.  Defaults to "ignore".

I think the relationship with 'drop' should be clarified here.

"To drop a commit without warning or error, use the `drop` command in the
todo-list."

?

> +# Print the list of the SHA-1 of the commits
> +# from a todo list in a file.
> +# $1: todo-file, $2: outfile
> +todo_list_to_sha_list () {
> +	todo_list=$(git stripspace --strip-comments <"$1")
> +	temp_file=$(mktemp)

c5770f7 (contrib/diffall: create tmp dirs without mktemp, 2012-03-14)
says "mktemp is not available on all platforms." ...

> +	echo "$todo_list" >$temp_file

Don't use echo on user-supplied data. It's not portable (think what
happens if $todo_list starts with a -).

printf '%s' "$todo_list"

You don't need all these intermediate variables/files. It looks like you
want

git stripspace --strip-comments | while read -r command sha1 rest
do
...

> +# Transforms SHA-1 list in argument
> +# to a list of commits (in place)
> +# Doesn't check if the SHA-1 are commits.

s/if/whether/

> +# $1: file with long SHA-1 list
> +long_sha_to_commit_list () {
> +	short_missing=""
> +	git_command="git show --oneline"
> +	get_line_command="head -n 1"
> +	temp_file=$(mktemp)
> +	while read -r sha
> +	do
> +		if test -n "$sha"
> +		then
> +			commit=$($git_command $sha | $get_line_command)

Why not git show --oneline $sha without using this $git_command?

To me

  git show --oneline $sha | head -n 1

says all that has to be said, while

  $git_command $sha | $get_line_command

does not say anything (although it uses more characters).

But actually, computing the diff with `git show` and eliminating it with
`head` seems backwards.

I guess you want something like:

  git rev-list --pretty=oneline -1 $sha

And the whole long_sha_to_commit_list is more or less equivalent to

  git rev-list --stdin --no-walk --format=oneline --abbrev-commit

(Yes, git was _meant_ to be scriptable, and it really is)

> +# Use warn for each line of a file
> +# $1: file to warn

I don't parse "file to warn". I'd rather warn the user than a file ;-).

> +# Check if the user dropped some commits by mistake
> +# Behaviour determined by .gitconfig.

not necessarily .gitconfig, there are other names. Say
rebase.missingCommitsCheckLevel if you want to be technically accurate.

> +check_commits () {
> +	checkLevel=$(git config --get rebase.missingCommitsCheckLevel)
> +	checkLevel=${checkLevel:-ignore}
> +	# To lowercase
> +	checkLevel=$(echo "$checkLevel" | tr 'A-Z' 'a-z')

Good comments insist on _why_ not _what_. I'd write:

	# Don't be case sensitive
	checkLevel=$(echo "$checkLevel" | tr 'A-Z' 'a-z')

> +	case "$checkLevel" in
> +	warn|error)
> +		# Get the SHA-1 of the commits
> +		todo_list_to_sha_list "$todo".backup "$todo".oldsha1
> +		todo_list_to_sha_list "$todo" "$todo".newsha1
> +
> +		# Sort the SHA-1 and compare them
> +		echo "$(sort -u "$todo".oldsha1)" >"$todo".oldsha1
> +		echo "$(sort -u "$todo".newsha1)" >"$todo".newsha1

Useless uses of echo.

echo $(foo) -> foo

> +			warn "Warning : some commits may have been dropped" \

No space before : in english (hmm, didn't I already notice this one?)

> +			warn "Use git --config rebase.missingCommitsCheckLevel to change" \
> +				"the level of warnings (ignore,warn,error)."

Spaces after commas, or (ignore/warn/error).

> +		warn "Unrecognized setting $checkLevel for option" \
> +			"rebase.missingCommitsCheckLevel in git rebase -i."

I'd drop the "in git rebase -i" part. The user may have run "git rebase
--interactive" and not know -i, and normally already knows which command
is executed.

> +test_expect_success 'rebase -i respects rebase.missingCommitsCheckLevel=error' '
> +	test_config rebase.missingCommitsCheckLevel error &&
> +	test_when_finished "git checkout master &&
> +		git branch -D tmp2" &&
> +	git checkout -b tmp2 master &&
> +	set_fake_editor &&
> +	test_must_fail env FAKE_LINES="1 2 3 4" \
> +		git rebase -i --root &&
> +	test E = $(git cat-file commit HEAD | sed -ne \$p)
> +'

You should test also that

git rebase --edit-todo # playing with $EDITOR to restore the original lines.
git rebase --continue

actually continues. You did have problems with this in early
implementations, so it's not straightforward, so it deserves a test.

You should check the output of git rebase -i too.

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