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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 26, 2015, 23:27 UTC
Message-ID: <CAPig+cQJMSjS=fiwMHE93efSsa2QYQ8TphyyfcLg7kAXRi_+cw@mail.gmail.com>
In-Reply-To: <1432676318-22852-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr>

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.

Show 5 quoted lines

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

Show 19 quoted lines

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

Show 15 quoted lines

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

Show 19 quoted lines

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

Show 19 quoted lines

> +       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?

Show 13 quoted lines

> +       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"?

Show 5 quoted lines

> +               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/

Show 17 quoted lines

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

Show 5 quoted lines

> +               ;;
> +       "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.

Show 18 quoted lines

> +               ;;
> +       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