[0/3] sha256 fixes for filter-branch

On Wed, Mar 10, 2021 at 01:04:37PM +0100, Ævar Arnfjörð Bjarmason wrote:

> I found a missing spot that wasn't trivial to fix, so sending an E-Mail:
> 
> In git-filter-branch.sh we have:
> 
> _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
> _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
> 
> Then later we have a case condition based on matching a SHA-1:
> 
>         $_x40)
>                 echo "Ref '$ref' was rewritten"
>                 if ! git update-ref -m "filter-branch: rewrite" \
> 
> Just deleting that case arm has filter-branch tests passing, so whatever
> it's meant to do it has zero coverage, which explains why it hasn't
> broken with our tests.

It actually does get covered. Dropping that case-arm means we'll fall
through to the one below, which does _roughly_ the same thing with a
bunch of extra warnings. But none of the tests actually check the error
messages, so they don't notice.

Here's a series which fixes it, plus extra tests to notice the
distinction.

  [1/3]: t7003: test ref rewriting explicitly
  [2/3]: filter-branch: drop multiple-ancestor warning
  [3/3]: filter-branch: drop $_x40 glob

 git-filter-branch.sh     | 16 ++--------------
 t/t7003-filter-branch.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 14 deletions(-)

-Peff

On Wed, Mar 10, 2021 at 9:06 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Mar 10, 2021 at 01:04:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> > I found a missing spot that wasn't trivial to fix, so sending an E-Mail:
> >
> > In git-filter-branch.sh we have:
> >
> > _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
> > _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
> >
> > Then later we have a case condition based on matching a SHA-1:
> >
> >         $_x40)
> >                 echo "Ref '$ref' was rewritten"
> >                 if ! git update-ref -m "filter-branch: rewrite" \
> >
> > Just deleting that case arm has filter-branch tests passing, so whatever
> > it's meant to do it has zero coverage, which explains why it hasn't
> > broken with our tests.
>
> It actually does get covered. Dropping that case-arm means we'll fall
> through to the one below, which does _roughly_ the same thing with a
> bunch of extra warnings. But none of the tests actually check the error
> messages, so they don't notice.
>
> Here's a series which fixes it, plus extra tests to notice the
> distinction.
>
>   [1/3]: t7003: test ref rewriting explicitly
>   [2/3]: filter-branch: drop multiple-ancestor warning
>   [3/3]: filter-branch: drop $_x40 glob
>
>  git-filter-branch.sh     | 16 ++--------------
>  t/t7003-filter-branch.sh | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 14 deletions(-)
>
> -Peff

Ævar successfully nerd sniped you.

...and sucked me in too, since I went through and read your patches.
:-)  They look good to me.

Elijah Newren <newren@gmail.com> writes:

> Ævar successfully nerd sniped you.
>
> ...and sucked me in too, since I went through and read your patches.
> :-)  They look good to me.

Yup, thanks, both.

On 2021-03-10 at 17:06:33, Jeff King wrote:
> It actually does get covered. Dropping that case-arm means we'll fall
> through to the one below, which does _roughly_ the same thing with a
> bunch of extra warnings. But none of the tests actually check the error
> messages, so they don't notice.
> 
> Here's a series which fixes it, plus extra tests to notice the
> distinction.
> 
>   [1/3]: t7003: test ref rewriting explicitly
>   [2/3]: filter-branch: drop multiple-ancestor warning
>   [3/3]: filter-branch: drop $_x40 glob

This series looked good to me.  Thanks for sending it.