Galan Rémi <remi.galan-alfonso@xxxxxxxxxxxxxxxxxxxxxxx> 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/
--
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