Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, May 26, 2015 at 5:38 PM, Galan Rémi
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> git rebase -i: Warn removed or dupplicated commits

s/dupplicated/duplicated/

Also, drop capitalization, and insert "about":

    git rebase -i: warn about removed or duplicated commits

> Check if commits were removed (i.e. a line was deleted) or dupplicated

s/dupplicated/duplicated/

> (e.g. the same commit is picked twice), can print warnings or abort

s/can/and/, I think.

> git rebase according to the value of the configuration variable
> rebase.checkLevel.
>
> Add the configuration variable rebase.checkLevel.
>     - When unset or set to "IGNORED", no checking is done.

s/IGNORED/IGNORE/

>     - 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.

Why uppercase for these names? Is there precedence for that? I think
lowercase is more common.

> Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>
> ---
>  This part of the patch has no test yet, it is more for rfc.
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d44bc85..2152e27 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2204,6 +2204,14 @@ rebase.autoStash::
>         successful rebase might result in non-trivial conflicts.
>         Defaults to false.
>
> +rebase.checkLevel::
> +       If set to "warn", git rebase -i will print a warning if some
> +       commits are removed (i.e. a line was deleted) or if some
> +       commits appear more than one time (e.g. the same commit is
> +       picked twice), however the rebase will still proceed. If set
> +       to "error", it will print the previous warnings and abort the
> +       rebase.

The commit message talks about "ignore", but there is no mention here.

Also, what is the default behavior if not specified? That should be documented.

Finally, this talks about lowercase "warn" and "error", whereas the
commit message uses upper case "WARN" and "ERROR", as does the code.
Why the inconsistency?

>  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 3cd2ef2..cb05cbb 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -213,6 +213,11 @@ rebase.autoSquash::
>  rebase.autoStash::
>         If set to true enable '--autostash' option by default.
>
> +rebase.checkLevel::
> +       If set to "warn" print warnings about removed commits and
> +       duplicated commits in interactive mode. If set to "error"
> +       print the warnings and abort the rebase. No check by default.

Ditto: Fails to mention "ignore".

>  OPTIONS
>  -------
>  --onto <newbase>::
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index cb749e8..8a837ca 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -837,6 +837,80 @@ add_exec_commands () {
>         mv "$1.new" "$1"
>  }
>
> +# 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)
> +       echo "$todo_list" > "$temp_file"
> +       while read -r command sha1 rest < "$temp_file"

On this project it is typical to drop the space after redirection
operators (<, >, >>), however, git-rebase--interactive.sh is filled
with both styles (space and no space after redirection). New code
probably ought to drop the space.

> +       do
> +               case "$command" in
> +               x|"exec")
> +                       ;;
> +               *)
> +                       echo "$sha1" >> "$2"
> +                       ;;
> +               esac
> +               sed -i '1d' "$temp_file"
> +       done
> +       rm "$temp_file"
> +}
> +
> +# Check if the user dropped some commits by mistake
> +# or if there are two identical commits.
> +# Behaviour determined by .gitconfig.
> +check_commits () {
> +       checkLevel=$(git config --get rebase.checkLevel)
> +       checkLevel=${checkLevel:-"IGNORE"}

Minor aside: Unnecessary quoting increases the noise level, thus
making the code slightly more difficult to read. This could just as
well have been:

    checkLevel=${checkLevel:-IGNORE}

There are plenty of other places throughout this patch which exhibit
the same shortcoming, but I won't point them out individually.

> +       # To uppercase
> +       checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')

Is there precedence elsewhere for recognizing uppercase and lowercase
variants of config values?

> +       case "$checkLevel" in
> +       "WARN"|"ERROR")
> +               todo_list_to_sha_list "$todo".backup "$todo".oldsha1
> +               todo_list_to_sha_list "$todo" "$todo".newsha1
> +
> +               duplicates=$(sort "$todo".newsha1 | uniq -d)
> +
> +               echo "$(sort -u "$todo".oldsha1)" > "$todo".oldsha1
> +               echo "$(sort -u "$todo".newsha1)" > "$todo".newsha1
> +               missing=$(comm -2 -3 "$todo".oldsha1 "$todo".newsha1)
> +
> +               # check missing commits
> +               if ! test -z "$missing"

Isn't "! test -z" just a verbose way of saying "test -n"?

> +               then
> +                       warn "Warning : some commits may have been dropped accidentally."
> +                       warn "Dropped commits:"
> +                       warn "$missing"
> +                       warn "To avoid this message, use \"drop\" to explicitely remove a commit."

s/explicitely/explicitly/

> +                       warn "Use git --config rebase.checkLevel to change"
> +                       warn "the level of warnings (ignore,warn,error)."
> +                       warn ""
> +
> +                       if test "$checkLevel" = "ERROR"
> +                       then
> +                               die_abort "Rebase aborted due to dropped commits."
> +                       fi
> +               fi
> +
> +               # check duplicate commits
> +               if ! test -z "$duplicates"
> +               then
> +                       warn "Warning : some commits have been used twice:"
> +                       warn "$duplicates"
> +                       warn ""
> +               fi

Shouldn't this case also 'die' when rebase.checkLevel is "error"? And,
why doesn't the user get advice about configuring rebase.checkLevel in
this case?

In fact, the current logic flow seems a bit borked. I would have
expected it to be more like this:

    if test -n "$missing"
    then
        ...warn about accidental drops...
    fi

    if test -n "$duplicates"
    then
        ...warn about accidental duplicates...
    fi

    if test -n "$missing$duplicates"
    then
        ...show advice about configuring rebase.checkLevel...

        if test $checkLevel = ERROR
        then
            die_abort "..."
        fi
    fi

> +               ;;
> +       "IGNORE")
> +               ;;
> +       *)
> +               warn "Unrecognized setting for option rebase.checkLevel in git rebase -i"

This message might be more useful if it mentioned the actual unrecognized value.

> +               ;;
> +       esac
> +}
> +
>  # The whole contents of this file is run by dot-sourcing it from
>  # inside a shell function.  It used to be that "return"s we see
>  # below were not inside any function, and expected to return
> @@ -1082,6 +1156,8 @@ has_action "$todo" ||
>
>  expand_todo_ids
>
> +check_commits
> +
>  test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
>
>  GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
> --
> 2.4.1.174.g28bfe8e