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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 31, 2018, 10:30 UTC
Message-ID: <CAPig+cQQvZgFFWfn=OaJ969YKhGx5PQ3r6iSMBpqpjDZGBuViA@mail.gmail.com>
In-Reply-To: <31870bca-329f-2451-750a-56d917153844@talktalk.net>

On Tue, Jul 31, 2018 at 6:01 AM Phillip Wood <phillip.wood@talktalk.net> wrote:

Show 15 quoted lines

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