On Thu, Jul 13, 2017 at 11:40:32AM -0700, Stefan Beller wrote:
> On Thu, Jul 13, 2017 at 7:58 AM, Jeff King <peff@xxxxxxxx> wrote:
>
> > I really only need t6300 and t6006 converted to build on for the rest of
> > the series. But t4207 was easy to do. t4026 still uses raw codes, but
> > converting it would be a pretty big job, so I punted.
>
> I think it is good to have raw codes in at least one place to test
> that raw codes work, but then again it could be specific test calling
> out that this is tested.
Yeah, that thought crossed my mind, too. t4026 would definitely be the
place for it, as it is about exhaustively testing the various colors.
The others are just about feeding color codes through config and
user-formats.
> > @@ -59,7 +54,8 @@ EOF
> > # to this test since it does not contain any decoration, hence --first-parent
> > test_expect_success 'Commit Decorations Colored Correctly' '
> > git log --first-parent --abbrev=10 --all --decorate --oneline --color=always |
> > - sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" >out &&
> > + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" |
> > + test_decode_color >out &&
>
> Just some thoughts:
>
> This extension of the pipe-chain is not making it worse as gits exit code
> is already hidden.
Yes, I noticed the existing pipe-chain. We can fix it while we're here,
though I think it's not a big deal in practice.
> The sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" is sort of funny, because
> I would have expected it to break in the future with e.g. the sha1 to longer
> hash conversion. But as we specify --abbrev=10, this seems future proof.
> In an ideal world this would be encapsulated in a function (c.f. t/diff-lib.sh).
I agree it's a bit gross. Possibly:
git log --format='%C(auto)%d %s'
would be easier for the test to parse (I'm pretty sure that didn't exist
back when this test was written).
-Peff
