From: Phil Pennock <hidden> Date: 2016-07-27 08:23:10
The reflog contains timestamp information, but these were not exposed to
`--pretty`. Four of the six author/committer format string
second-letters were still available and copied, but `d`/`D` are taken
for reflog selector formatting. So use `%gT` for "time" instead of
"date" mnemonic for using `--date=...` formatting.
Implementation trade-off: could use `format_person_part()` if bounce
information out of the reflog as a buffer-constructed string, then parse
it back again, _and_ then use a hacky letter override for 'T' becoming
'd'. That would avoid reimplementing the specification of the five
added timestamp rendering cases. Instead, we allow exporting the
timestamp information from the reflog, and if we get it, we add the same
data. No memory alloc and string construction/parsing, a small amount
of duplication of meaning.
Signed-off-by: Phil Pennock <redacted>
---
pretty.c | 36 ++++++++++++++++++++++++++++++++++++
reflog-walk.c | 16 ++++++++++++++++
reflog-walk.h | 3 +++
t/t4205-log-pretty-formats.sh | 26 ++++++++++++++++++++++++++
4 files changed, 81 insertions(+)
@@ -1066,6 +1066,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */constchar*msg=c->message;structcommit_list*p;inth1,h2;+unsignedlongtimestamp;+inttz;/* these are independent of the commit */switch(placeholder[0]){
After sleep, I realized that while I included tests I forgot to update
the docs. Those will go into a v2 of the patch; sorry about that.
Feedback sought upon everything _else_ wrong with this which should be
addressed in v2.
[apologies to explicit CC'd who may see this twice; apparently the
mobile client for this mail-provider infests messages with HTML with no
option to disable]
-Phil Pennock
From: Jeff King <hidden> Date: 2016-07-27 13:58:29
On Wed, Jul 27, 2016 at 04:14:14AM -0400, Phil Pennock wrote:
The reflog contains timestamp information, but these were not exposed to
`--pretty`. Four of the six author/committer format string
second-letters were still available and copied, but `d`/`D` are taken
for reflog selector formatting. So use `%gT` for "time" instead of
"date" mnemonic for using `--date=...` formatting.
Hrm. Since Ted was not cc'd, it is not clear to me whether this is
coincidental or in response to the thread over in
http://thread.gmane.org/gmane.comp.version-control.git/299201
To summarize, I think the conclusion there was that we would go with at
least the 't' and 'r' formatters in the short term. The 'i/I' ones were
not something Ted cared about that much, I think, but they do make
things orthogonal with the other ident dates.
Your '%gT' seems like a nice idea in principle, though I'm not sure it
is all that useful due to the way that --date impacts %gd. In other
words:
git log -g --format=%gd --date=rfc
will already respect --date (albeit with "ref@{...}" wrapped around it).
But even with your patch, you cannot do:
git log -g --format="%gd %gT" --date=rfc
to get "HEAD@{0} ...rfc-date...", because the presence of "--date" is
the trigger to switch the meaning of "%gd".
So the final solution is more like:
- a formatter for just the reflog time, respecting date
- a formatter for just the reflog index (the "0" in HEAD@{0})
- a formatter for the ref name (just the "HEAD" in HEAD@{0})
And I'm not sure if we want to do that now with a bunch of arcane
two-letter placeholders, or wait for a refactoring where they can get
more readable names like %(reflog-index).
-Peff
From: Phil Pennock <hidden> Date: 2016-07-27 14:17:46
On Wed, Jul 27, 2016, at 09:58, Jeff King wrote:
On Wed, Jul 27, 2016 at 04:14:14AM -0400, Phil Pennock wrote:
quoted
The reflog contains timestamp information, but these were not exposed to
`--pretty`. Four of the six author/committer format string
second-letters were still available and copied, but `d`/`D` are taken
for reflog selector formatting. So use `%gT` for "time" instead of
"date" mnemonic for using `--date=...` formatting.
Coincidental, I was unaware of that. Sorry for the duplication. Please
ignore my patch, I'm not going to even try to compete with Mr Ts'o.
I asked a friend who is a git developer why this wasn't available, he
said it would be fairly easy to do, I asked if he was trying to suck me
into contributing patches to git, he said yes. I neglected to search
archives for myself.
Besides "two independent submissions in short space of time" as evidence
that this functionality is wanted, my real-world motivator was: `go get
$something` failed, until I told the Golang tooling to update
dependencies too; after that I wanted to easily see when the dependency
was last updated on my local system and ended up having to look at the
reflog on disk and manually convert timestamps.
To summarize, I think the conclusion there was that we would go with at
least the 't' and 'r' formatters in the short term.
'r' and one of the non-relative renderers are the ones I put into a
`[pretty]` item for my use.
to get "HEAD@{0} ...rfc-date...", because the presence of "--date" is
the trigger to switch the meaning of "%gd".
Yes, I went for completeness even though it led to some duplication, and
tested them all. Though it looks like I didn't include the --date
modifications into the test-suite example. Oops.
Thanks for the review and the cluebat.
-Phil
From: Jeff King <hidden> Date: 2016-07-27 17:56:31
On Wed, Jul 27, 2016 at 04:14:14AM -0400, Phil Pennock wrote:
The reflog contains timestamp information, but these were not exposed to
`--pretty`. Four of the six author/committer format string
second-letters were still available and copied, but `d`/`D` are taken
for reflog selector formatting. So use `%gT` for "time" instead of
"date" mnemonic for using `--date=...` formatting.
So as I mentioned before, this is similar to another recent patch. But I
kind of like this one better, so let me give this one a more thorough
review.
Implementation trade-off: could use `format_person_part()` if bounce
information out of the reflog as a buffer-constructed string, then parse
it back again, _and_ then use a hacky letter override for 'T' becoming
'd'. That would avoid reimplementing the specification of the five
added timestamp rendering cases. Instead, we allow exporting the
timestamp information from the reflog, and if we get it, we add the same
data. No memory alloc and string construction/parsing, a small amount
of duplication of meaning.
I think this is a good tradeoff. Even if you handled 'd' in a hacky way
now, you _don't_ want to "%g*" to magically grow to learn new placeholders
that "%a*" does, as they might conflict with other "%g" formats in the
same way that "%gd" (i.e., we would need to do a hacky workaround for
_those_ too, but nothing would warn the person adding the new
placeholders).
+ /*
+ * all the date options except those taken for other reflog handling
+ * (so not 'd' or 'D').
+ */
+ case 'r':
+ if (get_reflog_timeinfo(×tamp, &tz, c->pretty_ctx->reflog_info)) {
+ strbuf_addstr(sb, show_date(timestamp, tz, DATE_MODE(RELATIVE)));
+ }
+ return 2;
+ case 't':
+ if (get_reflog_timeinfo(×tamp, &tz, c->pretty_ctx->reflog_info)) {
+ strbuf_addf(sb, "%lu", timestamp);
+ }
+ return 2;
+ case 'i':
+ if (get_reflog_timeinfo(×tamp, &tz, c->pretty_ctx->reflog_info)) {
+ strbuf_addstr(sb, show_date(timestamp, tz, DATE_MODE(ISO8601)));
+ }
+ return 2;
+ case 'I':
+ if (get_reflog_timeinfo(×tamp, &tz, c->pretty_ctx->reflog_info)) {
+ strbuf_addstr(sb, show_date(timestamp, tz, DATE_MODE(ISO8601_STRICT)));
+ }
+ return 2;
These all look good. If for whatever reason we don't have a reflog
entry, we write nothing (but still eat the placeholder), which seems
sensible.
One alternative interface would be more like get_reflog_message(), where
it either puts something in a strbuf or not. And then your calls would
look like:
get_reflog_date(sb, c->pretty_cx->reflog_info, DATE_MODE(ISO8601));
or whatever. Your way here is more flexible, though, if we ever add more
callers.
+ /*
+ * reflog d/D are taken, so we can't use those for dates
+ * but we do want to support using --date= format overrides
+ * so we steal 'T' for those ('time' instead of 'date')
+ */
+ case 'T':
+ if (get_reflog_timeinfo(×tamp, &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.
Our usual error-return is "0 is success", "-1 is error".
Though we don't _always_ adhere to that, and I won't be surprised if you
simply copied this from other nearby code.
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.
But...
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.
And then you can move your environment variables down into that test
(though see below).
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.
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:
test_expect_success 'reflog tformat timestamps work' '
cat >expected <<-\EOF &&
HEAD@{0}
11 years ago
1112912173
2005-04-07 15:16:13 -0700
2005-04-07T15:16:13-07:00
HEAD@{1}
11 years ago
1112912173
2005-04-07 15:16:13 -0700
2005-04-07T15:16:13-07:00
EOF
git log -g -2 --format="%gd%n%gr%n%gt%n%gi%n%gI%n" >actual &&
test_cmp expected actual
'
(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.
Also, come to think of it, that "%gr" test is going to break in about
year. :-/
We do test relative date formatting via test-date, which can take a
special value for "when is now", but you can't do so via regular
git-log. So the best we could do is check "%gr" separately and just make
sure that it output _something_.
-Peff
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.
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.
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
From: Jeff King <hidden> Date: 2016-07-27 18:32:13
On Wed, Jul 27, 2016 at 06:13:34PM +0000, Phil Pennock wrote:
quoted
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.
Yeah, I agree the "!" test for "did it work" is counter-intuitive if you're
coming from other languages, but it's pretty normal for C code bases
(especially ours).
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.
Ah. Yeah, those are wrong and bad. We should fix that.
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).
I think because t4205 is mostly about testing user-formats. t1410 and
t1411 are where I'd expect to see tests of normal output for "git
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.
Oh, I agree. I just think the basics are tested elsewhere, and this new
test is redundant and out-of-place. The numeric progression of the test
scripts is supposed to follow this basic-first ordering, though it's
often hard in practice (e.g., I think some "git log --format" stuff has
crept into the t14xx series, just because it's so darned convenient to
use).
quoted
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.
Yeah, looking at the other "&&-" cases you mentioned, I see the whole
script does not use "<<-". That is against our "usual" style, though
there are many inconsistent sins in the history of the scripts. IMHO is
worth modernizing the whole thing.
I don't buy the tabs-become-spaces argument. We use tabs for indentation
in Git, and that's extremely unlikely to change. If your patch gets
munged in transit or by your editor, then the maintainer is going to
complain when applying your patch.
quoted
(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?
The magic function you are looking for is "test_tick()", which advances
GIT_COMMITTER_DATE, etc. It's called automatically from test_commit(),
so seeing two commits with identical timestamps is an indication that
an earlier test did a manual commit without a matching tick (probably
because it did not care about the timestamps itself).
It's OK to just make your own new commits, like:
test_commit reflog-test-one &&
test_commit reflog-test-two &&
... check the output of log -g -2 ...
and test those, and then you aren't as dependent on what happened before
(though of course if you are hard-coding the dates, somebody inserting a
new tick in between will screw you up; we tend to add new tests at the
end of scripts for reasons like that).
quoted
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?
I can't think of any reason that would fail, unless somebody travels
back in time to run the tests in 2005 (or their system clock is screwed
up).
-Peff
From: Jeff King <hidden> Date: 2016-07-27 18:53:05
On Wed, Jul 27, 2016 at 02:32:03PM -0400, Jeff King wrote:
quoted
This however was matching existing style for `head1` and `head2` a
little above. I was somewhat surprised.
Ah. Yeah, those are wrong and bad. We should fix that.
So here's that fix, along with a style cleanup for the here-docs.
[1/2]: t4205: drop top-level &&-chaining
[2/2]: t4205: indent here documents
Hopefully that provides a nicer base for building your patch on top of.
-Peff
From: Jeff King <hidden> Date: 2016-07-27 18:55:12
The test currently does something like:
do_one() &&
do_two() &&
test_expect_success ...
We generally avoid performing actions at the top-level of
the script (outside of a test_expect block) for two reasons:
1. The test harness is not checking and reporting if they
fail.
2. Their output is not handled correctly (not hidden by
default, nor shown with "-v").
Using &&-chains seems like it should help with (1), but it
doesn't. If either of the commands fails, we simply skip
running the follow-on test entirely, and the test harness
has no idea.
We can fix this by pushing that setup into its own block.
It _could_ go into the following test block, but since the
result in this case is used by multiple tests, it's more
clear to mark it explicitly as a distinct setup step.
Signed-off-by: Jeff King <redacted>
---
t/t4205-log-pretty-formats.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
@@ -504,8 +504,10 @@ test_expect_success 'ISO and ISO-strict date formats display the same values' ''# get new digests (with no abbreviations)-head1=$(gitrev-parse--verifyHEAD~0)&&-head2=$(gitrev-parse--verifyHEAD~1)&&+test_expect_success'set up log decoration tests''+head1=$(gitrev-parse--verifyHEAD~0)&&+head2=$(gitrev-parse--verifyHEAD~1)+' test_expect_success'log decoration properly follows tag chain''gittag-atag1-mtag1&&
From: Jeff King <hidden> Date: 2016-07-27 18:55:32
Our usual style in the test scripts is to indent here
documents with tabs, and use "<<-" to strip the tabs. The
result is easier to read.
This old test script did not do so in its inception, and
further tests added onto it followed the local style. Let's
bring it in line with our usual style.
Some of the tests actually care quite a bit about
whitespace, but none of them do so at the beginning of the
line (because they things like qz_to_tab_space to avoid
depending on the literal whitespace), so we can do a fairly
mechanical conversion.
Most of the here-docs also use interpolation, so they have
been left as "<<-EOF". In a few cases, though, where
interpolation was not in use, I've converted them to
"<<-\EOF" to match our usual "don't interpolate unless you
need to" style.
Signed-off-by: Jeff King <redacted>
---
t/t4205-log-pretty-formats.sh | 392 +++++++++++++++++++++---------------------
1 file changed, 196 insertions(+), 196 deletions(-)
@@ -145,199 +145,199 @@ test_expect_success 'setup more commits' ' test_expect_success'left alignment formatting''gitlog--pretty="tformat:%<(40)%s">actual&&-qz_to_tab_space<<EOF>expected&&-messagetwoZ-messageoneZ-addbarZ-$(commit_msg)Z-EOF+qz_to_tab_space<<-EOF>expected&&+messagetwoZ+messageoneZ+addbarZ+$(commit_msg)Z+EOFtest_cmpexpectedactual' test_expect_success'left alignment formatting. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%<(40)%s">actual&&-qz_to_tab_space<<EOF|iconv-futf-8-t$test_encoding>expected&&-messagetwoZ-messageoneZ-addbarZ-$(commit_msg)Z-EOF+qz_to_tab_space<<-EOF|iconv-futf-8-t$test_encoding>expected&&+messagetwoZ+messageoneZ+addbarZ+$(commit_msg)Z+EOFtest_cmpexpectedactual' test_expect_success'left alignment formatting at the nth column''gitlog--pretty="tformat:%h %<|(40)%s">actual&&-qz_to_tab_space<<EOF>expected&&-$head1messagetwoZ-$head2messageoneZ-$head3addbarZ-$head4$(commit_msg)Z-EOF+qz_to_tab_space<<-EOF>expected&&+$head1messagetwoZ+$head2messageoneZ+$head3addbarZ+$head4$(commit_msg)Z+EOFtest_cmpexpectedactual' test_expect_success'left alignment formatting at the nth column''COLUMNS=50gitlog--pretty="tformat:%h %<|(-10)%s">actual&&-qz_to_tab_space<<EOF>expected&&-$head1messagetwoZ-$head2messageoneZ-$head3addbarZ-$head4$(commit_msg)Z-EOF+qz_to_tab_space<<-EOF>expected&&+$head1messagetwoZ+$head2messageoneZ+$head3addbarZ+$head4$(commit_msg)Z+EOFtest_cmpexpectedactual' test_expect_success'left alignment formatting at the nth column. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%h %<|(40)%s">actual&&-qz_to_tab_space<<EOF|iconv-futf-8-t$test_encoding>expected&&-$head1messagetwoZ-$head2messageoneZ-$head3addbarZ-$head4$(commit_msg)Z-EOF+qz_to_tab_space<<-EOF|iconv-futf-8-t$test_encoding>expected&&+$head1messagetwoZ+$head2messageoneZ+$head3addbarZ+$head4$(commit_msg)Z+EOFtest_cmpexpectedactual' test_expect_success'left alignment formatting with no padding''gitlog--pretty="tformat:%<(1)%s">actual&&-cat<<EOF>expected&&-messagetwo-messageone-addbar-$(commit_msg)-EOF+cat<<-EOF>expected&&+messagetwo+messageone+addbar+$(commit_msg)+EOFtest_cmpexpectedactual' test_expect_success'left alignment formatting with no padding. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%<(1)%s">actual&&-cat<<EOF|iconv-futf-8-t$test_encoding>expected&&-messagetwo-messageone-addbar-$(commit_msg)-EOF+cat<<-EOF|iconv-futf-8-t$test_encoding>expected&&+messagetwo+messageone+addbar+$(commit_msg)+EOFtest_cmpexpectedactual' test_expect_success'left alignment formatting with trunc''gitlog--pretty="tformat:%<(10,trunc)%s">actual&&-qz_to_tab_space<<EOF>expected&&-message..-message..-addbarZ-initial...-EOF+qz_to_tab_space<<-\EOF>expected&&+message..+message..+addbarZ+initial...+EOFtest_cmpexpectedactual' test_expect_success'left alignment formatting with trunc. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%<(10,trunc)%s">actual&&-qz_to_tab_space<<EOF|iconv-futf-8-t$test_encoding>expected&&-message..-message..-addbarZ-initial...-EOF+qz_to_tab_space<<-\EOF|iconv-futf-8-t$test_encoding>expected&&+message..+message..+addbarZ+initial...+EOFtest_cmpexpectedactual' test_expect_success'left alignment formatting with ltrunc''gitlog--pretty="tformat:%<(10,ltrunc)%s">actual&&-qz_to_tab_space<<EOF>expected&&-..sagetwo-..sageone-addbarZ-..${sample_utf8_part}lich-EOF+qz_to_tab_space<<-EOF>expected&&+..sagetwo+..sageone+addbarZ+..${sample_utf8_part}lich+EOFtest_cmpexpectedactual' test_expect_success'left alignment formatting with ltrunc. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%<(10,ltrunc)%s">actual&&-qz_to_tab_space<<EOF|iconv-futf-8-t$test_encoding>expected&&-..sagetwo-..sageone-addbarZ-..${sample_utf8_part}lich-EOF+qz_to_tab_space<<-EOF|iconv-futf-8-t$test_encoding>expected&&+..sagetwo+..sageone+addbarZ+..${sample_utf8_part}lich+EOFtest_cmpexpectedactual' test_expect_success'left alignment formatting with mtrunc''gitlog--pretty="tformat:%<(10,mtrunc)%s">actual&&-qz_to_tab_space<<EOF>expected&&-mess..two-mess..one-addbarZ-init..lich-EOF+qz_to_tab_space<<-\EOF>expected&&+mess..two+mess..one+addbarZ+init..lich+EOFtest_cmpexpectedactual' test_expect_success'left alignment formatting with mtrunc. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%<(10,mtrunc)%s">actual&&-qz_to_tab_space<<EOF|iconv-futf-8-t$test_encoding>expected&&-mess..two-mess..one-addbarZ-init..lich-EOF+qz_to_tab_space<<-\EOF|iconv-futf-8-t$test_encoding>expected&&+mess..two+mess..one+addbarZ+init..lich+EOFtest_cmpexpectedactual' test_expect_success'right alignment formatting''gitlog--pretty="tformat:%>(40)%s">actual&&-qz_to_tab_space<<EOF>expected&&-Zmessagetwo-Zmessageone-Zaddbar-Z$(commit_msg)-EOF+qz_to_tab_space<<-EOF>expected&&+Zmessagetwo+Zmessageone+Zaddbar+Z$(commit_msg)+EOFtest_cmpexpectedactual' test_expect_success'right alignment formatting. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%>(40)%s">actual&&-qz_to_tab_space<<EOF|iconv-futf-8-t$test_encoding>expected&&-Zmessagetwo-Zmessageone-Zaddbar-Z$(commit_msg)-EOF+qz_to_tab_space<<-EOF|iconv-futf-8-t$test_encoding>expected&&+Zmessagetwo+Zmessageone+Zaddbar+Z$(commit_msg)+EOFtest_cmpexpectedactual' test_expect_success'right alignment formatting at the nth column''gitlog--pretty="tformat:%h %>|(40)%s">actual&&-qz_to_tab_space<<EOF>expected&&-$head1messagetwo-$head2messageone-$head3addbar-$head4$(commit_msg)-EOF+qz_to_tab_space<<-EOF>expected&&+$head1messagetwo+$head2messageone+$head3addbar+$head4$(commit_msg)+EOFtest_cmpexpectedactual' test_expect_success'right alignment formatting at the nth column''COLUMNS=50gitlog--pretty="tformat:%h %>|(-10)%s">actual&&-qz_to_tab_space<<EOF>expected&&-$head1messagetwo-$head2messageone-$head3addbar-$head4$(commit_msg)-EOF+qz_to_tab_space<<-EOF>expected&&+$head1messagetwo+$head2messageone+$head3addbar+$head4$(commit_msg)+EOFtest_cmpexpectedactual' test_expect_success'right alignment formatting at the nth column. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%h %>|(40)%s">actual&&-qz_to_tab_space<<EOF|iconv-futf-8-t$test_encoding>expected&&-$head1messagetwo-$head2messageone-$head3addbar-$head4$(commit_msg)-EOF+qz_to_tab_space<<-EOF|iconv-futf-8-t$test_encoding>expected&&+$head1messagetwo+$head2messageone+$head3addbar+$head4$(commit_msg)+EOFtest_cmpexpectedactual'
@@ -345,110 +345,110 @@ EOF# as in previous test. test_expect_success'right alignment formatting at the nth column with --graph. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--graph--pretty="tformat:%h %>|(40)%s">actual&&-iconv-futf-8-t$test_encoding>expected<<EOF&&-*$head1messagetwo-*$head2messageone-*$head3addbar-*$head4$(commit_msg)-EOF+iconv-futf-8-t$test_encoding>expected<<-EOF&&+*$head1messagetwo+*$head2messageone+*$head3addbar+*$head4$(commit_msg)+EOFtest_cmpexpectedactual' test_expect_success'right alignment formatting with no padding''gitlog--pretty="tformat:%>(1)%s">actual&&-cat<<EOF>expected&&-messagetwo-messageone-addbar-$(commit_msg)-EOF+cat<<-EOF>expected&&+messagetwo+messageone+addbar+$(commit_msg)+EOFtest_cmpexpectedactual' test_expect_success'right alignment formatting with no padding and with --graph''gitlog--graph--pretty="tformat:%>(1)%s">actual&&-cat<<EOF>expected&&-*messagetwo-*messageone-*addbar-*$(commit_msg)-EOF+cat<<-EOF>expected&&+*messagetwo+*messageone+*addbar+*$(commit_msg)+EOFtest_cmpexpectedactual' test_expect_success'right alignment formatting with no padding. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%>(1)%s">actual&&-cat<<EOF|iconv-futf-8-t$test_encoding>expected&&-messagetwo-messageone-addbar-$(commit_msg)-EOF+cat<<-EOF|iconv-futf-8-t$test_encoding>expected&&+messagetwo+messageone+addbar+$(commit_msg)+EOFtest_cmpexpectedactual' test_expect_success'center alignment formatting''gitlog--pretty="tformat:%><(40)%s">actual&&-qz_to_tab_space<<EOF>expected&&-ZmessagetwoZ-ZmessageoneZ-ZaddbarZ-Z$(commit_msg)Z-EOF+qz_to_tab_space<<-EOF>expected&&+ZmessagetwoZ+ZmessageoneZ+ZaddbarZ+Z$(commit_msg)Z+EOFtest_cmpexpectedactual' test_expect_success'center alignment formatting. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%><(40)%s">actual&&-qz_to_tab_space<<EOF|iconv-futf-8-t$test_encoding>expected&&-ZmessagetwoZ-ZmessageoneZ-ZaddbarZ-Z$(commit_msg)Z-EOF+qz_to_tab_space<<-EOF|iconv-futf-8-t$test_encoding>expected&&+ZmessagetwoZ+ZmessageoneZ+ZaddbarZ+Z$(commit_msg)Z+EOFtest_cmpexpectedactual' test_expect_success'center alignment formatting at the nth column''gitlog--pretty="tformat:%h %><|(40)%s">actual&&-qz_to_tab_space<<EOF>expected&&-$head1messagetwoZ-$head2messageoneZ-$head3addbarZ-$head4$(commit_msg)Z-EOF+qz_to_tab_space<<-EOF>expected&&+$head1messagetwoZ+$head2messageoneZ+$head3addbarZ+$head4$(commit_msg)Z+EOFtest_cmpexpectedactual' test_expect_success'center alignment formatting at the nth column''COLUMNS=70gitlog--pretty="tformat:%h %><|(-30)%s">actual&&-qz_to_tab_space<<EOF>expected&&-$head1messagetwoZ-$head2messageoneZ-$head3addbarZ-$head4$(commit_msg)Z-EOF+qz_to_tab_space<<-EOF>expected&&+$head1messagetwoZ+$head2messageoneZ+$head3addbarZ+$head4$(commit_msg)Z+EOFtest_cmpexpectedactual' test_expect_success'center alignment formatting at the nth column. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%h %><|(40)%s">actual&&-qz_to_tab_space<<EOF|iconv-futf-8-t$test_encoding>expected&&-$head1messagetwoZ-$head2messageoneZ-$head3addbarZ-$head4$(commit_msg)Z-EOF+qz_to_tab_space<<-EOF|iconv-futf-8-t$test_encoding>expected&&+$head1messagetwoZ+$head2messageoneZ+$head3addbarZ+$head4$(commit_msg)Z+EOFtest_cmpexpectedactual' test_expect_success'center alignment formatting with no padding''gitlog--pretty="tformat:%><(1)%s">actual&&-cat<<EOF>expected&&-messagetwo-messageone-addbar-$(commit_msg)-EOF+cat<<-EOF>expected&&+messagetwo+messageone+addbar+$(commit_msg)+EOFtest_cmpexpectedactual'
@@ -457,34 +457,34 @@ EOFold_head1=$(gitrev-parse--verifyHEAD~0) test_expect_success'center alignment formatting with no padding. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%><(1)%s">actual&&-cat<<EOF|iconv-futf-8-t$test_encoding>expected&&-messagetwo-messageone-addbar-$(commit_msg)-EOF+cat<<-EOF|iconv-futf-8-t$test_encoding>expected&&+messagetwo+messageone+addbar+$(commit_msg)+EOFtest_cmpexpectedactual' test_expect_success'left/right alignment formatting with stealing''gitcommit--amend-mshort--author"long long long <long@me.com>"&&gitlog--pretty="tformat:%<(10,trunc)%s%>>(10,ltrunc)% an">actual&&-cat<<EOF>expected&&-shortlonglonglong-message..AUThor-addbarAUThor-initial...AUThor-EOF+cat<<-\EOF>expected&&+shortlonglonglong+message..AUThor+addbarAUThor+initial...AUThor+EOFtest_cmpexpectedactual' test_expect_success'left/right alignment formatting with stealing. i18n.logOutputEncoding''git-ci18n.logOutputEncoding=$test_encodinglog--pretty="tformat:%<(10,trunc)%s%>>(10,ltrunc)% an">actual&&-cat<<EOF|iconv-futf-8-t$test_encoding>expected&&-shortlonglonglong-message..AUThor-addbarAUThor-initial...AUThor-EOF+cat<<-\EOF|iconv-futf-8-t$test_encoding>expected&&+shortlonglonglong+message..AUThor+addbarAUThor+initial...AUThor+EOFtest_cmpexpectedactual'
From: Phil Pennock <hidden> Date: 2016-07-27 18:58:44
On 2016-07-27 at 14:32 -0400, Jeff King wrote:
Yeah, I agree the "!" test for "did it work" is counter-intuitive if you're
coming from other languages, but it's pretty normal for C code bases
(especially ours).
For stuff returning pointers, sure.
Ah. Yeah, those are wrong and bad. We should fix that.
K, noted, will go back over this thread before I touch code and pick
these all up.
I don't buy the tabs-become-spaces argument. We use tabs for indentation
in Git, and that's extremely unlikely to change. If your patch gets
munged in transit or by your editor, then the maintainer is going to
complain when applying your patch.
Okay. (I happen to think that robustness against a cycle of developers
discussing why whitespace broke patches in transit is good, but I'll
change this).
The magic function you are looking for is "test_tick()", which advances
Ah. Too tired last night, didn't realize that when I switched from
constructing things on branches to have branch action in the reflog, to
"just need commits really", to "previous tests did this" I lost the
`test_tick()` invocation and for some reason when I looked at the
results, decided I must be missing something about resolution instead of
realizing that I was no longer calling `test_tick()`.
Which leads to a final point:
I'm not going to write any more code today, for reasons of "weak human
needs sleep and I'll make more stupid mistakes if I continue". So I'm
going silent on this thread for the rest of today. Not ignoring. If
folks want fast progress (I haven't looked at release cycle status) I
won't mind cutting me out of the loop :-D but otherwise I'll look
Thu/Fri this week for any remedial work and offer a fresh patch with
fixes from this thread, and documentation.
I'm tempted to just steal the docs from Ted's patch, unless that's
considered bad form?
-Phil
From: Jeff King <hidden> Date: 2016-07-27 19:16:37
On Wed, Jul 27, 2016 at 06:41:41PM +0000, Phil Pennock wrote:
On 2016-07-27 at 14:32 -0400, Jeff King wrote:
quoted
Yeah, I agree the "!" test for "did it work" is counter-intuitive if you're
coming from other languages, but it's pretty normal for C code bases
(especially ours).
For stuff returning pointers, sure.
I think the pattern comes more from syscalls.
quoted
I don't buy the tabs-become-spaces argument. We use tabs for indentation
in Git, and that's extremely unlikely to change. If your patch gets
munged in transit or by your editor, then the maintainer is going to
complain when applying your patch.
Okay. (I happen to think that robustness against a cycle of developers
discussing why whitespace broke patches in transit is good, but I'll
change this).
What we've found is that if your whitespace breaks in transit, the
patches don't apply anyway (because of wrapping, or because tab/space
conversion breaks the context lines). So we mostly just accept the risk
and stay militant about it.
I'm not going to write any more code today, for reasons of "weak human
needs sleep and I'll make more stupid mistakes if I continue". So I'm
going silent on this thread for the rest of today. Not ignoring. If
folks want fast progress (I haven't looked at release cycle status) I
won't mind cutting me out of the loop :-D but otherwise I'll look
Thu/Fri this week for any remedial work and offer a fresh patch with
fixes from this thread, and documentation.
Sure, take your time. It's open source, so movement is generally
measured in days, and sometimes weeks. Thank you for working on it!
I'm tempted to just steal the docs from Ted's patch, unless that's
considered bad form?
I think that's fine, especially considering how short the snippet is.
If you pulled content from somewhere, it's normal to acknowledge it in
the commit message.
-Peff