Re: [PATCH] pretty format string support for reflog times

From: Phil Pennock <phil@pennock-tech.com>
Date: Jul 27, 2016, 18:13 UTC
Message-ID: <20160727181334.GA24224@tower.spodhuis.org>
In-Reply-To: <20160727175622.GA9961@sigill.intra.peff.net>

On 2016-07-27 at 13:56 -0400, Jeff King wrote:

Show 12 quoted lines

> On Wed, Jul 27, 2016 at 04:14:14AM -0400, Phil Pennock wrote:
> > +		 */
> > +		case 'T':
> > +			if (get_reflog_timeinfo(&timestamp, &tz, c->pretty_ctx->reflog_info)) {
> > +				strbuf_addstr(sb, show_date(timestamp, tz, &c->pretty_ctx->date_mode));
> > +			}
> > +			return 2;
> >  		}
> 
> I think we can drop this one. It's a nice direction to go eventually,
> but without the extra placeholders I mentioned elsewhere, it doesn't buy
> us much.

Will do.

> Our usual error-return is "0 is success", "-1 is error".

The idea was "boolean function" and adding more negations elsewhere just made things ugly. I can change if you really want, as consistency wins, but I'll be holding my nose as the invoker flow becomes:

    if (! function_call(out_params)) {
      use(out_params);
    }

which is counter-intuitive (but then, I do much less C these days and have been corrupted). So I'll hold off for now, until told otherwise.

> Though we don't _always_ adhere to that, and I won't be surprised if you
> simply copied this from other nearby code.

Not _this_ one ..

Show 10 quoted lines

> > +refhead1_short=$(git rev-parse --short --verify HEAD@{0}) &&
> > +refhead2_short=$(git rev-parse --short --verify HEAD@{1}) &&
> 
> We try to push as much as possible into a test_expect_success block,
> since it handles things like unexpected output to stderr. I guess you
> put these outside because they are used in multiple tests. You don't
> have to do that, because tests can affect the greater environment. But
> if you do keep something outside of a test, you _don't_ want to use
> &&-chaining, as it means that the lines below it (i.e., the next test!)
> would simply not be run at all.

This however was matching existing style for `head1` and `head2` a little above. I was somewhat surprised.

Show 12 quoted lines

> > +test_expect_success 'can access the reflog' '
> > +	git reflog -n 2 >actual &&
> > +	cat >expected <<EOF &&
> > +$refhead1_short HEAD@{0}: commit (amend): shorter
> > +$refhead2_short HEAD@{1}: commit (amend): short
> > +EOF
> > +	test_cmp expected actual
> > +'
> 
> I'm not sure what this is testing. Just that we have reflogs turned on
> at all? I think we can skip this, as it's implicit in the
> reflog-formatting test below.

Disagree: I could see no existing tests for reflog content matching an
expected layout (but could have missed one; I see some _using_ reflog).
If adding a test for minutiae of how tuning options adjust the output,
and something changes which breaks the output more widely, the person
investigating can spend a lot of time investigating a red herring,
looking to see what they broke in the `--pretty` handling.

First test the basics, then test the specifics, so that if the basics break too then the developer is naturally led to the correct thing to investigate instead of their only clue being that specifics broke.

> You can use "<<-" to ask the shell to strip leading tabs from the
> here-doc. And then you can indent the contents to match the rest of the
> test.

You can, but it's fragile if tabs become spaces and it isn't consistent with the existing tests above.

> I kind of wonder if it would be better to drop "%h" from your format,
> too. You're just testing the timestamps, and handling the shortening is
> cluttering up the test. So maybe just:

This makes a lot of sense.

> (I did a few more tweaks to the format to hopefully make it easier to
> read). It would probably be a more interesting test if the two reflogs
> actually had different timestamps, though.

Is there a way to force that, through the normalizations?

> Also, come to think of it, that "%gr" test is going to break in about
> year. :-/

Oh crap, I saw the normalization and thought everything was being set to specifics, but of course the relative is against now. Good catch, thanks. How about:

    | sed "s/[1-9][0-9]* years/N years/"

and then test against "N years" in expected?

-Phil