On Tue, Jul 31, 2018 at 6:01 AM Phillip Wood <phillip.wood@xxxxxxxxxxxx> 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.
