Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits
> Matthieu Moy writes: > > Ideally, you would check the list of commits displayed too. If the > > commits sha1 are stable, this should be easy to do. If it's too hard to > > test, I'd say its not worth the trouble, but others may disagree. > > Originally I chose not to check if the SHA-1 were corrects since > check_commits was called right after expand_todo_ids and I thought > that expand_todo_ids checked them, but from what I understand, it > doesn't seem to check if the SHA-1 are commits, I could be wrong > though. Ignore this email, I completely misunderstood the email I was responding to. (Mailer that doesn't show the quotes by default) Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits
Matthieu Moy writes: > Ideally, you would check the list of commits displayed too. If the > commits sha1 are stable, this should be easy to do. If it's too hard to > test, I'd say its not worth the trouble, but others may disagree. Originally I chose not to check if the SHA-1 were corrects since check_commits was called right after expand_todo_ids and I thought that expand_todo_ids checked them, but from what I understand, it doesn't seem to check if the SHA-1 are commits, I could be wrong though. Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits
Remi Galan Alfonso writes: > Checking that the warning was correctly displayed like in the test for > "warn" if I understood correctly. About that, is checking that the > first line is "Warning: some commits may have been dropped > accidentally." (like in the test for "warn") enough, or should I check > that the commit displayed as removed is the correct one? Ideally, you would check the list of commits displayed too. If the commits sha1 are stable, this should be easy to do. If it's too hard to test, I'd say its not worth the trouble, but others may disagree. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits
Remi Galan Alfonso writes: > In this case it is not true, because of the infile and outfile being > identical. However sort does have a -o (-output) that I missed that > allows avoiding using echo or writing in another file; I'm correcting > with this. Even though it is in POSIX, some implementation may lack it, so our code tend to avoid "sort -o". "echo $(sort)" is also ugly and inefficient. The obvious and old-fashioned would be sufficient here: sort -u "$todo".oldsha1 >"$todo".oldsha1+ && mv "$todo".oldsha1+ "$todo".oldsha1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits
Matthieu Moy writes: > > Galan Rémi writes: > > +# 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 In this case it is not true, because of the infile and outfile being identical. However sort does have a -o (-output) that I missed that allows avoiding using echo or writing in another file; I'm correcting with this. > 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. In this patch, the error is dealt with a die_abort and not die, thus there is no rebase --edit-todo or rebase --continue possible afterward. > You should check the output of git rebase -i too. Checking that the warning was correctly displayed like in the test for "warn" if I understood correctly. About that, is checking that the first line is "Warning: some commits may have been dropped accidentally." (like in the test for "warn") enough, or should I check that the commit displayed as removed is the correct one? The other points have been corrected, or their correction is in progress. Thanks, Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits
Galan Rémi 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" \
> +
