Re: [PATCH v2 3/4] Recommend git-filter-repo instead of git-filter-branch

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 28, 2019, 06:17 UTC
Message-ID: <CAPig+cSydZnie3CSwPgfSaNcGxr1jj94YBXAL47vEXrtxaQXUw@mail.gmail.com>
In-Reply-To: <20190828002210.8862-4-newren@gmail.com>

On Tue, Aug 27, 2019 at 8:22 PM Elijah Newren <newren@gmail.com> wrote:

Show 20 quoted lines

> filter-branch suffers from a deluge of disguised dangers that disfigure
> history rewrites (i.e. deviate from the deliberate changes). [...]
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt
> @@ -16,6 +16,20 @@ SYNOPSIS
> +WARNING
> +-------
> +'git filter-branch' has a plethora of pitfalls that can produce non-obvious
> +manglings of the intended history rewrite (and can leave you with little
> +time to investigate such problems since it has such abysmal performance).
> +These safety and performance issues cannot be backward compatibly fixed and
> +as such, its use is not recommended.  Please use an alternative history
> +filtering tool such as https://github.com/newren/git-filter-repo/[git
> +filter-repo].  If you still need to use 'git filter-branch', please
> +carefully read the "Safety" section of the message on the Git mailing list
> +https://public-inbox.org/git/CABPp-BEDOH-row-hxY4u_cP30ptqOpcCvPibwyZ2wBu142qUbA@mail.gmail.com/[detailing
> +the land mines of filter-branch] and vigilantly avoid as many of the
> +hazards listed there as reasonably possible.

Is there a good reason to not simply copy the "Safety" section from that email directly into this document so that readers don't have to go chasing down the link (especially those who are reading documentation offline)?

Show 7 quoted lines

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> @@ -832,7 +832,7 @@ Hard case: The changes are not the same.::
>         This happens if the 'subsystem' rebase had conflicts, or used
>         `--interactive` to omit, edit, squash, or fixup commits; or
>         if the upstream used one of `commit --amend`, `reset`, or
> -       `filter-branch`.
> +       a full history rewriting command like `filter-repo`.

Do we want a clickable link to `filter-repo` here?

Show 9 quoted lines

> diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
> @@ -123,10 +123,10 @@ The following format are available:
> +linkgit:git-hash-object[1], linkgit:git-rebase[1], and
> +linkgit:git-filter-repo[1], among other git commands, can be used to
> [...]
> @@ -148,8 +148,8 @@ pending objects.
>  linkgit:git-hash-object[1]
>  linkgit:git-rebase[1]
> +linkgit:git-filter-repo[1]

Are these 'linkgit:' references to `filter-repo` going to be meaningful if that tool is not incorporated into the Git project proper? Perhaps use a generic clickable link instead.

Same comment applies to other 'linkgit:' invocations in the remainder of the patch.

> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> old mode 100755
> new mode 100644

Why lose the executable bit?

> @@ -83,6 +83,19 @@ set_ident () {
> +if [ -z "$FILTER_BRANCH_SQUELCH_WARNING" -a \
> +     -z "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" ]; then

If this script didn't already have a mix of styles, I'd say something about modern style being:

    if test -z "$FILTER_BRANCH_SQUELCH_WARNING" &&
        test -z "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS"
    then
        ...
    fi

Show 6 quoted lines

> +       cat <<EOF
> +WARNING: git-filter-branch has a glut of gotchas generating mangled history
> +         rewrites.  Please use an alternative filtering tool such as 'git
> +         filter-repo' (https://github.com/newren/git-filter-repo/) instead.
> +         See the filter-branch manual page for more details; to squelch
> +         this warning and pause, set FILTER_BRANCH_SQUELCH_WARNING=1.

The "and pause" threw me. There's more than a bit of ambiguity surrounding it. Perhaps drop it?