Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Jun 2, 2015, 14:56 UTC
- Message-ID
- <vpqzj4ignwe.fsf@anie.imag.fr>
- In-Reply-To
- <1433252180-25591-2-git-send-email-remi.galan-alfonso@ensimag.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".
Show 8 quoted lines
> 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.
Show 6 quoted lines
> +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."
?
Show 6 quoted lines
> +# 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/
Show 11 quoted lines
> +# $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.
Show 5 quoted lines
> +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')
Show 9 quoted lines
> + 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.
Show 10 quoted lines
> +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/