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

[Elijah Newren <newren@gmail.com>]

On Tue, Aug 27, 2019 at 11:17 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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)?

Makes sense, I can include it.  However, saying e.g. "the
git-filter-branch manpage is missing..." or "the git-filter-branch
manpage actually documents <crazy buggy behavior> as expected" feels
really weird to include on the git-filter-branch manpage.  I'll try to
touch it up.

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

I guess it can't hurt.

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

I'm fixing them up.

> > diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> > old mode 100755
> > new mode 100644
>
> Why lose the executable bit?

Whoops.  Did some rebasing and fixups, then continued editing my
buffer of the file after one of the rebases, realized the file was
deleted (because of the final patch in the series), moved the file out
of the way and rebased again and copied the file back into place, and
forgot to check the filemode.

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

Sure, will do.