Re: [PATCH v2 3/4] sequencer: fix "rebase -i --root" corrupting author header timestamp

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Jul 31, 2018 at 6:01 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
> Now that the author is correct, can we test_cmp() it against its
> expected value to make sure there are no hidden surprises in the name
> and email in the future. (It would be reassuring to test an author with
> "'" in the name as well but that is out of scope for this series.)
>
> +       git cat-file commit HEAD^ |grep ^author >expected &&
> >       set_fake_editor &&
> >       FAKE_LINES="2 1" git rebase -i --root &&
> >       git cat-file commit HEAD^ >out &&
> -       git cat-file commit HEAD^ >out &&
> > -     grep "^author ..*> @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out
> +       git cat-file commit HEAD^ |grep ^author >out &&
> +       test_cmp expected out
> > +     grep "^author ..*> [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out
> >   ' >   test_done

I deliberately avoided test_cmp() in favor of 'grep' specifically
because I didn't want to hard-code the timestamp into the 'expect'
file since future changes to tests preceding this one could
potentially outdate the hard-coded value, and setting up my own commit
to ensure a consistent timestamp would have made this test longer and
less "obvious".

However, your approach sidesteps those concerns nicely. That said, I
think such a simplification could be done on top of this patch since
the current change to the test in this patch makes it very clear (to
the reader) that the "@" problem has been corrected, whereas it would
not be at all obvious if this patch incorporated your simplification.

While your idea is nice, I'd rather not re-roll this series just for
that. (I'd really like to see these fixes for this critical commit
object corruption land as soon as possible without re-rolling
repeatedly for "optional" or less important changes.) Perhaps such a
simplification can be done in the series you're working on(?).

Thanks for the review.