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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Aug 27, 2019 at 8:22 PM Elijah Newren <newren@gmail.com> wrote:
> 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)?

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

> 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

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