threads / patch / 42937

patchpretty format string support for reflog times

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

## tl;dr

20 messages between Jul 27, 2016 and Jul 27, 2016. Diffs are folded; open one to read it.

replies: 19people: 4as markdown or json

Phil Pennock· Jul 27, 2016, 08:14 UTC · lore

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 <phil@pennock-tech.com>
---
 pretty.c                      | 36 ++++++++++++++++++++++++++++++++++++
 reflog-walk.c                 | 16 ++++++++++++++++
 reflog-walk.h                 |  3 +++
 t/t4205-log-pretty-formats.sh | 26 ++++++++++++++++++++++++++
 4 files changed, 81 insertions(+)
Show changes to 4 files +81 −0

pretty.c, reflog-walk.c, reflog-walk.h, t/t4205-log-pretty-formats.sh

diff --git a/pretty.c b/pretty.c
index 9fa42c2..955baf1 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1066,6 +1066,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	const char *msg = c->message;
 	struct commit_list *p;
 	int h1, h2;
+	unsigned long timestamp;
+	int tz;
 
 	/* these are independent of the commit */
 	switch (placeholder[0]) {
@@ -1212,6 +1214,40 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 						    placeholder[1],
 						    c->pretty_ctx->reflog_info,
 						    &c->pretty_ctx->date_mode);
+		/*
+		 * all the date options except those taken for other reflog handling
+		 * (so not 'd' or 'D').
+		 */
+		case 'r':
+			if (get_reflog_timeinfo(&timestamp, &tz, c->pretty_ctx->reflog_info)) {
+				strbuf_addstr(sb, show_date(timestamp, tz, DATE_MODE(RELATIVE)));
+			}
+			return 2;
+		case 't':
+			if (get_reflog_timeinfo(&timestamp, &tz, c->pretty_ctx->reflog_info)) {
+				strbuf_addf(sb, "%lu", timestamp);
+			}
+			return 2;
+		case 'i':
+			if (get_reflog_timeinfo(&timestamp, &tz, c->pretty_ctx->reflog_info)) {
+				strbuf_addstr(sb, show_date(timestamp, tz, DATE_MODE(ISO8601)));
+			}
+			return 2;
+		case 'I':
+			if (get_reflog_timeinfo(&timestamp, &tz, c->pretty_ctx->reflog_info)) {
+				strbuf_addstr(sb, show_date(timestamp, tz, DATE_MODE(ISO8601_STRICT)));
+			}
+			return 2;
+		/*
+		 * 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(&timestamp, &tz, c->pretty_ctx->reflog_info)) {
+				strbuf_addstr(sb, show_date(timestamp, tz, &c->pretty_ctx->date_mode));
+			}
+			return 2;
 		}
 		return 0;	/* unknown %g placeholder */
 	case 'N':
diff --git a/reflog-walk.c b/reflog-walk.c
index a246af2..bb9cbf8 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -309,6 +309,22 @@ void get_reflog_message(struct strbuf *sb,
 	strbuf_add(sb, info->message, len);
 }
 
+int get_reflog_timeinfo(unsigned long *timestamp_out,
+			 int *tz_out,
+			 struct reflog_walk_info *reflog_info)
+{
+	struct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;
+	struct reflog_info *info;
+
+	if (!commit_reflog)
+		return 0;
+
+	info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
+	*timestamp_out = info->timestamp;
+	*tz_out = info->tz;
+	return 1;
+}
+
 const char *get_reflog_ident(struct reflog_walk_info *reflog_info)
 {
 	struct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;
diff --git a/reflog-walk.h b/reflog-walk.h
index 27886f7..a368c72 100644
--- a/reflog-walk.h
+++ b/reflog-walk.h
@@ -19,5 +19,8 @@ extern void get_reflog_selector(struct strbuf *sb,
 		struct reflog_walk_info *reflog_info,
 		const struct date_mode *dmode, int force_date,
 		int shorten);
+extern int get_reflog_timeinfo(unsigned long *timestamp_out,
+		int *tz_out,
+		struct reflog_walk_info *reflog_info);
 
 #endif
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index d9f6242..9d211ab 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -533,4 +533,30 @@ EOF
 	test_cmp expected actual1
 '
 
+refhead1_short=$(git rev-parse --short --verify HEAD@{0}) &&
+refhead2_short=$(git rev-parse --short --verify HEAD@{1}) &&
+
+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
+'
+
+test_expect_success 'reflog tformat timestamps work' '
+	git log -g -n 2 \
+		--pretty=tformat:"%h %gd / %gr %gt%n %gi %gI%n %gT : %gs" > actual &&
+	cat >expected <<EOF &&
+$refhead1_short HEAD@{0} / 11 years ago 1112912173
+ 2005-04-07 15:16:13 -0700 2005-04-07T15:16:13-07:00
+ Thu Apr 7 15:16:13 2005 -0700 : commit (amend): shorter
+$refhead2_short HEAD@{1} / 11 years ago 1112912173
+ 2005-04-07 15:16:13 -0700 2005-04-07T15:16:13-07:00
+ Thu Apr 7 15:16:13 2005 -0700 : commit (amend): short
+EOF
+	test_cmp expected actual
+'
+
 test_done
-- 
2.9.2.466.g8c6d1f9.dirty
Phil Pennock· Jul 27, 2016, 12:02 UTC · re: Phil Pennock · lore

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

On 2016-07-27 at 04:14 -0400, Phil Pennock wrote:
Show 5 quoted lines
>  pretty.c                      | 36 ++++++++++++++++++++++++++++++++++++
>  reflog-walk.c                 | 16 ++++++++++++++++
>  reflog-walk.h                 |  3 +++
>  t/t4205-log-pretty-formats.sh | 26 ++++++++++++++++++++++++++
>  4 files changed, 81 insertions(+)

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
Jeff King· Jul 27, 2016, 13:58 UTC · re: Phil Pennock · lore

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

On Wed, Jul 27, 2016 at 04:14:14AM -0400, Phil Pennock wrote:
Show 5 quoted lines
> 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
Phil Pennock· Jul 27, 2016, 14:17 UTC · re: Jeff King · lore

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

On Wed, Jul 27, 2016, at 09:58, Jeff King wrote:
Show 11 quoted lines
> 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

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

Junio C Hamano· Jul 27, 2016, 17:18 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 17 quoted lines
> 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.

I forgot about that thread after it stalled without drawing conclusion, after Ted asked if anybody has a strong opinion and saw only one response to it at

  https://public-inbox.org/git/20160711164317.GB3890%40thunk.org/
So, what is the next step?  Apply
  https://public-inbox.org/git/20160710055402.32684-1-tytso%40mit.edu/
but exclude %g[iI] bits out of that patch while doing so?
Thanks.
Jeff King· Jul 27, 2016, 17:39 UTC · re: Junio C Hamano · lore

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

On Wed, Jul 27, 2016 at 10:18:11AM -0700, Junio C Hamano wrote:
Show 21 quoted lines
> > 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.
> 
> I forgot about that thread after it stalled without drawing
> conclusion, after Ted asked if anybody has a strong opinion
> and saw only one response to it at
> 
>   https://public-inbox.org/git/20160711164317.GB3890%40thunk.org/
> 
> So, what is the next step?  Apply
> 
>   https://public-inbox.org/git/20160710055402.32684-1-tytso%40mit.edu/
> 
> but exclude %g[iI] bits out of that patch while doing so?

After having thought about it, I'm inclined to leave in the "%gi". There's no real rationale for doing "%gr" and "%gt" and not "%gi" except "well, Ted didn't need it". And it does make it match the author and committer date-formatting (except for the 'd' formats, of course).

I do think with the new "unix" format in jk/reflog-date, the interface to the reflog code could be simplified; we don't need a function to pull out just the timestamp any more. Something like the diff below.

However, I see a few remaining issues with Ted's original:
  - the refactored get_reflog_message() tries to return NULL, but the
    return type of the function is void. Presumably this should just be
    "return"?
  - ditto, the new get_reflog_time_t returns NULL, but wants an unsigned
    long (though this function goes away with my squash below)
  - show_reflog_date can return NULL, but we blindly feed its return
    value to strbuf_addstr(). I'm not sure under what conditions it
    _would_ return NULL, but that would cause a segfault
  - there should probably be tests in t6006 for the new formats
  - my squash below cuts out the use of gm_time_t(). But I don't think
    it should be necessary, as the reflog timestamp should already be in
    GMT, I would think. But maybe I am missing something.

I actually think Phil's patch from today is a little cleaner for most of these, as it returns the values via out-parameters, and uses the return value for "did we get anything?".

-Peff
Junio C Hamano· Jul 27, 2016, 18:09 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
> I actually think Phil's patch from today is a little cleaner for most of
> these, as it returns the values via out-parameters, and uses the return
> value for "did we get anything?".

True. That part of the interface is indeed better done with the new one.

I am still in favor of this suggestion you earlier made:
Show 7 quoted lines
> 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})

though. After all we only need three short ones while we migrate away to a longer %(reflog:<what>) format, right?

As to the unfortunate %gd that squats on the "date" other specifiers use, I do not see a good/quick approach to migrate it. If our ideal short-term endgame before the longer format were to use %gd, %g# and %gg for the above three, we first start warning people who use %gd for the historical mistaken "reflog selector", while telling people to use "%gg@{%g#}" instead if they truly want "reflog selector", and then switch its meaning to "reflog date". That would take a long time.

As %r prefix is not taken, we can immediately deprecate %g-anything format as a historical mistake and make sure we do not repeat the mistake of giving "d" to "reflog selector", perhaps?

Jeff King· Jul 27, 2016, 18:17 UTC · re: Junio C Hamano · lore

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

On Wed, Jul 27, 2016 at 11:09:30AM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> I am still in favor of this suggestion you earlier made:
> 
> > 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})
> 
> though.  After all we only need three short ones while we migrate
> away to a longer %(reflog:<what>) format, right?
Yes, I think those three would be sufficient to allow something like:
  HEAD@{0} (1 month ago) ...

where the relative time would come from "%gT" mixed with "--date=relative". What that doesn't allow is showing the time in multiple formats with different placeholders, like:

  HEAD@{0} (2016-06-25T01:23:45 -- 1 month ago) ...

For that you need either format-specific placeholders, or a generic date placeholder which can specify the format, like:

  %(reflog-date:relative)

So if you mean doing those on _top_ of what Phil and Ted are proposing, I think it is pretty flexible, but just a bit ugly. But doing it _instead_ would not allow the format Ted wanted.

Show 8 quoted lines
> As to the unfortunate %gd that squats on the "date" other specifiers
> use, I do not see a good/quick approach to migrate it.  If our ideal
> short-term endgame before the longer format were to use %gd, %g# and
> %gg for the above three, we first start warning people who use %gd
> for the historical mistaken "reflog selector", while telling people
> to use "%gg@{%g#}" instead if they truly want "reflog selector", and
> then switch its meaning to "reflog date".  That would take a long
> time.
I don't think it's worth the deprecation effort and confusion.
> As %r prefix is not taken, we can immediately deprecate %g-anything
> format as a historical mistake and make sure we do not repeat the
> mistake of giving "d" to "reflog selector", perhaps?

Moving to "%r" as a more sensible prefix for "reflog", while cleaning up historical mistakes, is more appealing. But if we really are planning to move to "%(reflog-*)", then I think we can just forget about "%r" entirely.

I just don't think anybody has volunteered to work on %(reflog).
-Peff
Junio C Hamano· Jul 27, 2016, 17:46 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 21 quoted lines
>> 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.
>
> I forgot about that thread after it stalled without drawing
> conclusion, after Ted asked if anybody has a strong opinion
> and saw only one response to it at
>
>   https://public-inbox.org/git/20160711164317.GB3890%40thunk.org/
>
> So, what is the next step?  Apply
>
>   https://public-inbox.org/git/20160710055402.32684-1-tytso%40mit.edu/
>
> but exclude %g[iI] bits out of that patch while doing so?

FWIW, a squash to do so is trivial, but the original had two compiler breakers.

I am not sure about the latter, i.e. the return value from the get_reflog_time_t() function (which I think is misnamed, in that the function returns "unsigned long", not "time_t", and should be renamed to get_reflog_time()) uses the epoch as a fallback value.

 reflog-walk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to reflog-walk.c +2 −2
diff --git a/reflog-walk.c b/reflog-walk.c
index d0aa2d0..51bfe29 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -309,7 +309,7 @@ void get_reflog_message(struct strbuf *sb,
 	size_t len;
 
 	if (!info)
-		return NULL;
+		return;
 	len = strlen(info->message);
 	if (len > 0)
 		len--; /* strip away trailing newline */
@@ -330,7 +330,7 @@ unsigned long get_reflog_time_t(struct reflog_walk_info *reflog_info)
 	struct reflog_info *info = get_reflog_info(reflog_info);
 
 	if (!info)
-		return NULL;
+		return 0;
 	return gm_time_t(info->timestamp, info->tz);
 }
 
Jeff King· Jul 27, 2016, 17:58 UTC · re: Junio C Hamano · lore

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

On Wed, Jul 27, 2016 at 10:46:48AM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> I am not sure about the latter, i.e. the return value from the
> get_reflog_time_t() function (which I think is misnamed, in that the
> function returns "unsigned long", not "time_t", and should be
> renamed to get_reflog_time()) uses the epoch as a fallback value.
> [...]
> @@ -330,7 +330,7 @@ unsigned long get_reflog_time_t(struct reflog_walk_info *reflog_info)
>  	struct reflog_info *info = get_reflog_info(reflog_info);
>  
>  	if (!info)
> -		return NULL;
> +		return 0;
>  	return gm_time_t(info->timestamp, info->tz);
>  }

Our mails just crossed, but I think returning even "0" is not ideal. We should probably write nothing to match things like "%gd" and "%gs", which just eat the placeholder and show nothing when they cannot access the reflog.

-Peff
Jeff King· Jul 27, 2016, 17:56 UTC · re: Phil Pennock · lore

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

On Wed, Jul 27, 2016 at 04:14:14AM -0400, Phil Pennock wrote:
Show 5 quoted lines
> 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.

Show 8 quoted lines
> 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).

Show 24 quoted lines
> +		/*
> +		 * all the date options except those taken for other reflog handling
> +		 * (so not 'd' or 'D').
> +		 */
> +		case 'r':
> +			if (get_reflog_timeinfo(&timestamp, &tz, c->pretty_ctx->reflog_info)) {
> +				strbuf_addstr(sb, show_date(timestamp, tz, DATE_MODE(RELATIVE)));
> +			}
> +			return 2;
> +		case 't':
> +			if (get_reflog_timeinfo(&timestamp, &tz, c->pretty_ctx->reflog_info)) {
> +				strbuf_addf(sb, "%lu", timestamp);
> +			}
> +			return 2;
> +		case 'i':
> +			if (get_reflog_timeinfo(&timestamp, &tz, c->pretty_ctx->reflog_info)) {
> +				strbuf_addstr(sb, show_date(timestamp, tz, DATE_MODE(ISO8601)));
> +			}
> +			return 2;
> +		case 'I':
> +			if (get_reflog_timeinfo(&timestamp, &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.

Show 11 quoted lines
> +		/*
> +		 * 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(&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.

Show 15 quoted lines
> +int get_reflog_timeinfo(unsigned long *timestamp_out,
> +			 int *tz_out,
> +			 struct reflog_walk_info *reflog_info)
> +{
> +	struct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;
> +	struct reflog_info *info;
> +
> +	if (!commit_reflog)
> +		return 0;
> +
> +	info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
> +	*timestamp_out = info->timestamp;
> +	*tz_out = info->tz;
> +	return 1;
> +}
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.

Show 10 quoted lines
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index d9f6242..9d211ab 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -533,4 +533,30 @@ EOF
>  	test_cmp expected actual1
>  '
>  
> +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.

But...
Show 8 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.

And then you can move your environment variables down into that test (though see below).

Show 13 quoted lines
> +test_expect_success 'reflog tformat timestamps work' '
> +	git log -g -n 2 \
> +		--pretty=tformat:"%h %gd / %gr %gt%n %gi %gI%n %gT : %gs" > actual &&
> +	cat >expected <<EOF &&
> +$refhead1_short HEAD@{0} / 11 years ago 1112912173
> + 2005-04-07 15:16:13 -0700 2005-04-07T15:16:13-07:00
> + Thu Apr 7 15:16:13 2005 -0700 : commit (amend): shorter
> +$refhead2_short HEAD@{1} / 11 years ago 1112912173
> + 2005-04-07 15:16:13 -0700 2005-04-07T15:16:13-07:00
> + Thu Apr 7 15:16:13 2005 -0700 : commit (amend): short
> +EOF
> +	test_cmp expected actual
> +'

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
Phil Pennock· Jul 27, 2016, 18:13 UTC · re: Jeff King · lore

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

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
Jeff King· Jul 27, 2016, 18:32 UTC · re: Phil Pennock · lore

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

On Wed, Jul 27, 2016 at 06:13:34PM +0000, Phil Pennock wrote:
Show 12 quoted lines
> > 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).

Show 13 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.
Ah. Yeah, those are wrong and bad. We should fix that.
Show 15 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).

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

Show 8 quoted lines
> 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).

Show 6 quoted lines
> > 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.

Show 5 quoted lines
> > (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).

Show 10 quoted lines
> > 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
Jeff King· Jul 27, 2016, 18:52 UTC · re: Jeff King · lore

[PATCH 0/2] t4205 style fixes

On Wed, Jul 27, 2016 at 02:32:03PM -0400, Jeff King wrote:
> > 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
Jeff King· Jul 27, 2016, 18:55 UTC · re: Jeff King · lore

[PATCH 1/2] t4205: drop top-level &&-chaining

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 <peff@peff.net>
---
 t/t4205-log-pretty-formats.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Show changes to t/t4205-log-pretty-formats.sh +4 −2
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index d9f6242..d6518fa 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -504,8 +504,10 @@ test_expect_success 'ISO and ISO-strict date formats display the same values' '
 '
 
 # get new digests (with no abbreviations)
-head1=$(git rev-parse --verify HEAD~0) &&
-head2=$(git rev-parse --verify HEAD~1) &&
+test_expect_success 'set up log decoration tests' '
+	head1=$(git rev-parse --verify HEAD~0) &&
+	head2=$(git rev-parse --verify HEAD~1)
+'
 
 test_expect_success 'log decoration properly follows tag chain' '
 	git tag -a tag1 -m tag1 &&
-- 
2.9.2.607.g98dce7b
Jeff King· Jul 27, 2016, 18:55 UTC · re: Jeff King · lore

[PATCH 2/2] t4205: indent here documents

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 <peff@peff.net>
---
 t/t4205-log-pretty-formats.sh | 392 +++++++++++++++++++++---------------------
 1 file changed, 196 insertions(+), 196 deletions(-)
Show changes to t/t4205-log-pretty-formats.sh +196 −196
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index d6518fa..f5435fd 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -145,199 +145,199 @@ test_expect_success 'setup more commits' '
 
 test_expect_success 'left alignment formatting' '
 	git log --pretty="tformat:%<(40)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
-message two                            Z
-message one                            Z
-add bar                                Z
-$(commit_msg)                    Z
-EOF
+	qz_to_tab_space <<-EOF >expected &&
+	message two                            Z
+	message one                            Z
+	add bar                                Z
+	$(commit_msg)                    Z
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left alignment formatting. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%<(40)%s" >actual &&
-	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-message two                            Z
-message one                            Z
-add bar                                Z
-$(commit_msg)                    Z
-EOF
+	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	message two                            Z
+	message one                            Z
+	add bar                                Z
+	$(commit_msg)                    Z
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left alignment formatting at the nth column' '
 	git log --pretty="tformat:%h %<|(40)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
-$head1 message two                    Z
-$head2 message one                    Z
-$head3 add bar                        Z
-$head4 $(commit_msg)            Z
-EOF
+	qz_to_tab_space <<-EOF >expected &&
+	$head1 message two                    Z
+	$head2 message one                    Z
+	$head3 add bar                        Z
+	$head4 $(commit_msg)            Z
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left alignment formatting at the nth column' '
 	COLUMNS=50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
-$head1 message two                    Z
-$head2 message one                    Z
-$head3 add bar                        Z
-$head4 $(commit_msg)            Z
-EOF
+	qz_to_tab_space <<-EOF >expected &&
+	$head1 message two                    Z
+	$head2 message one                    Z
+	$head3 add bar                        Z
+	$head4 $(commit_msg)            Z
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left alignment formatting at the nth column. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%h %<|(40)%s" >actual &&
-	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-$head1 message two                    Z
-$head2 message one                    Z
-$head3 add bar                        Z
-$head4 $(commit_msg)            Z
-EOF
+	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	$head1 message two                    Z
+	$head2 message one                    Z
+	$head3 add bar                        Z
+	$head4 $(commit_msg)            Z
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left alignment formatting with no padding' '
 	git log --pretty="tformat:%<(1)%s" >actual &&
-	cat <<EOF >expected &&
-message two
-message one
-add bar
-$(commit_msg)
-EOF
+	cat <<-EOF >expected &&
+	message two
+	message one
+	add bar
+	$(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left alignment formatting with no padding. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%<(1)%s" >actual &&
-	cat <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-message two
-message one
-add bar
-$(commit_msg)
-EOF
+	cat <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	message two
+	message one
+	add bar
+	$(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left alignment formatting with trunc' '
 	git log --pretty="tformat:%<(10,trunc)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
-message ..
-message ..
-add bar  Z
-initial...
-EOF
+	qz_to_tab_space <<-\EOF >expected &&
+	message ..
+	message ..
+	add bar  Z
+	initial...
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left alignment formatting with trunc. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%<(10,trunc)%s" >actual &&
-	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-message ..
-message ..
-add bar  Z
-initial...
-EOF
+	qz_to_tab_space <<-\EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	message ..
+	message ..
+	add bar  Z
+	initial...
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left alignment formatting with ltrunc' '
 	git log --pretty="tformat:%<(10,ltrunc)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
-..sage two
-..sage one
-add bar  Z
-..${sample_utf8_part}lich
-EOF
+	qz_to_tab_space <<-EOF >expected &&
+	..sage two
+	..sage one
+	add bar  Z
+	..${sample_utf8_part}lich
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left alignment formatting with ltrunc. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%<(10,ltrunc)%s" >actual &&
-	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-..sage two
-..sage one
-add bar  Z
-..${sample_utf8_part}lich
-EOF
+	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	..sage two
+	..sage one
+	add bar  Z
+	..${sample_utf8_part}lich
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left alignment formatting with mtrunc' '
 	git log --pretty="tformat:%<(10,mtrunc)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
-mess.. two
-mess.. one
-add bar  Z
-init..lich
-EOF
+	qz_to_tab_space <<-\EOF >expected &&
+	mess.. two
+	mess.. one
+	add bar  Z
+	init..lich
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left alignment formatting with mtrunc. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%<(10,mtrunc)%s" >actual &&
-	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-mess.. two
-mess.. one
-add bar  Z
-init..lich
-EOF
+	qz_to_tab_space <<-\EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	mess.. two
+	mess.. one
+	add bar  Z
+	init..lich
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'right alignment formatting' '
 	git log --pretty="tformat:%>(40)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
-Z                            message two
-Z                            message one
-Z                                add bar
-Z                    $(commit_msg)
-EOF
+	qz_to_tab_space <<-EOF >expected &&
+	Z                            message two
+	Z                            message one
+	Z                                add bar
+	Z                    $(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'right alignment formatting. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%>(40)%s" >actual &&
-	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-Z                            message two
-Z                            message one
-Z                                add bar
-Z                    $(commit_msg)
-EOF
+	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	Z                            message two
+	Z                            message one
+	Z                                add bar
+	Z                    $(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'right alignment formatting at the nth column' '
 	git log --pretty="tformat:%h %>|(40)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
-$head1                      message two
-$head2                      message one
-$head3                          add bar
-$head4              $(commit_msg)
-EOF
+	qz_to_tab_space <<-EOF >expected &&
+	$head1                      message two
+	$head2                      message one
+	$head3                          add bar
+	$head4              $(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'right alignment formatting at the nth column' '
 	COLUMNS=50 git log --pretty="tformat:%h %>|(-10)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
-$head1                      message two
-$head2                      message one
-$head3                          add bar
-$head4              $(commit_msg)
-EOF
+	qz_to_tab_space <<-EOF >expected &&
+	$head1                      message two
+	$head2                      message one
+	$head3                          add bar
+	$head4              $(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'right alignment formatting at the nth column. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%h %>|(40)%s" >actual &&
-	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-$head1                      message two
-$head2                      message one
-$head3                          add bar
-$head4              $(commit_msg)
-EOF
+	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	$head1                      message two
+	$head2                      message one
+	$head3                          add bar
+	$head4              $(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
@@ -345,110 +345,110 @@ EOF
 # as in previous test.
 test_expect_success 'right alignment formatting at the nth column with --graph. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --graph --pretty="tformat:%h %>|(40)%s" >actual &&
-	iconv -f utf-8 -t $test_encoding >expected <<EOF&&
-* $head1                    message two
-* $head2                    message one
-* $head3                        add bar
-* $head4            $(commit_msg)
-EOF
+	iconv -f utf-8 -t $test_encoding >expected <<-EOF &&
+	* $head1                    message two
+	* $head2                    message one
+	* $head3                        add bar
+	* $head4            $(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'right alignment formatting with no padding' '
 	git log --pretty="tformat:%>(1)%s" >actual &&
-	cat <<EOF >expected &&
-message two
-message one
-add bar
-$(commit_msg)
-EOF
+	cat <<-EOF >expected &&
+	message two
+	message one
+	add bar
+	$(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'right alignment formatting with no padding and with --graph' '
 	git log --graph --pretty="tformat:%>(1)%s" >actual &&
-	cat <<EOF >expected &&
-* message two
-* message one
-* add bar
-* $(commit_msg)
-EOF
+	cat <<-EOF >expected &&
+	* message two
+	* message one
+	* add bar
+	* $(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'right alignment formatting with no padding. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%>(1)%s" >actual &&
-	cat <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-message two
-message one
-add bar
-$(commit_msg)
-EOF
+	cat <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	message two
+	message one
+	add bar
+	$(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'center alignment formatting' '
 	git log --pretty="tformat:%><(40)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
-Z             message two              Z
-Z             message one              Z
-Z               add bar                Z
-Z         $(commit_msg)          Z
-EOF
+	qz_to_tab_space <<-EOF >expected &&
+	Z             message two              Z
+	Z             message one              Z
+	Z               add bar                Z
+	Z         $(commit_msg)          Z
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'center alignment formatting. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%><(40)%s" >actual &&
-	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-Z             message two              Z
-Z             message one              Z
-Z               add bar                Z
-Z         $(commit_msg)          Z
-EOF
+	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	Z             message two              Z
+	Z             message one              Z
+	Z               add bar                Z
+	Z         $(commit_msg)          Z
+	EOF
 	test_cmp expected actual
 '
 test_expect_success 'center alignment formatting at the nth column' '
 	git log --pretty="tformat:%h %><|(40)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
-$head1           message two          Z
-$head2           message one          Z
-$head3             add bar            Z
-$head4       $(commit_msg)      Z
-EOF
+	qz_to_tab_space <<-EOF >expected &&
+	$head1           message two          Z
+	$head2           message one          Z
+	$head3             add bar            Z
+	$head4       $(commit_msg)      Z
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'center alignment formatting at the nth column' '
 	COLUMNS=70 git log --pretty="tformat:%h %><|(-30)%s" >actual &&
-	qz_to_tab_space <<EOF >expected &&
-$head1           message two          Z
-$head2           message one          Z
-$head3             add bar            Z
-$head4       $(commit_msg)      Z
-EOF
+	qz_to_tab_space <<-EOF >expected &&
+	$head1           message two          Z
+	$head2           message one          Z
+	$head3             add bar            Z
+	$head4       $(commit_msg)      Z
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'center alignment formatting at the nth column. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%h %><|(40)%s" >actual &&
-	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-$head1           message two          Z
-$head2           message one          Z
-$head3             add bar            Z
-$head4       $(commit_msg)      Z
-EOF
+	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	$head1           message two          Z
+	$head2           message one          Z
+	$head3             add bar            Z
+	$head4       $(commit_msg)      Z
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'center alignment formatting with no padding' '
 	git log --pretty="tformat:%><(1)%s" >actual &&
-	cat <<EOF >expected &&
-message two
-message one
-add bar
-$(commit_msg)
-EOF
+	cat <<-EOF >expected &&
+	message two
+	message one
+	add bar
+	$(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
@@ -457,34 +457,34 @@ EOF
 old_head1=$(git rev-parse --verify HEAD~0)
 test_expect_success 'center alignment formatting with no padding. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%><(1)%s" >actual &&
-	cat <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-message two
-message one
-add bar
-$(commit_msg)
-EOF
+	cat <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	message two
+	message one
+	add bar
+	$(commit_msg)
+	EOF
 	test_cmp expected actual
 '
 
 test_expect_success 'left/right alignment formatting with stealing' '
 	git commit --amend -m short --author "long long long <long@me.com>" &&
 	git log --pretty="tformat:%<(10,trunc)%s%>>(10,ltrunc)% an" >actual &&
-	cat <<EOF >expected &&
-short long  long long
-message ..   A U Thor
-add bar      A U Thor
-initial...   A U Thor
-EOF
+	cat <<-\EOF >expected &&
+	short long  long long
+	message ..   A U Thor
+	add bar      A U Thor
+	initial...   A U Thor
+	EOF
 	test_cmp expected actual
 '
 test_expect_success 'left/right alignment formatting with stealing. i18n.logOutputEncoding' '
 	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%<(10,trunc)%s%>>(10,ltrunc)% an" >actual &&
-	cat <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
-short long  long long
-message ..   A U Thor
-add bar      A U Thor
-initial...   A U Thor
-EOF
+	cat <<-\EOF | iconv -f utf-8 -t $test_encoding >expected &&
+	short long  long long
+	message ..   A U Thor
+	add bar      A U Thor
+	initial...   A U Thor
+	EOF
 	test_cmp expected actual
 '
 
@@ -515,22 +515,22 @@ test_expect_success 'log decoration properly follows tag chain' '
 	git tag -d tag1 &&
 	git commit --amend -m shorter &&
 	git log --no-walk --tags --pretty="%H %d" --decorate=full >actual &&
-	cat <<EOF >expected &&
-$head1  (tag: refs/tags/tag2)
-$head2  (tag: refs/tags/message-one)
-$old_head1  (tag: refs/tags/message-two)
-EOF
+	cat <<-EOF >expected &&
+	$head1  (tag: refs/tags/tag2)
+	$head2  (tag: refs/tags/message-one)
+	$old_head1  (tag: refs/tags/message-two)
+	EOF
 	sort actual >actual1 &&
 	test_cmp expected actual1
 '
 
 test_expect_success 'clean log decoration' '
 	git log --no-walk --tags --pretty="%H %D" --decorate=full >actual &&
-	cat >expected <<EOF &&
-$head1 tag: refs/tags/tag2
-$head2 tag: refs/tags/message-one
-$old_head1 tag: refs/tags/message-two
-EOF
+	cat >expected <<-EOF &&
+	$head1 tag: refs/tags/tag2
+	$head2 tag: refs/tags/message-one
+	$old_head1 tag: refs/tags/message-two
+	EOF
 	sort actual >actual1 &&
 	test_cmp expected actual1
 '
-- 
2.9.2.607.g98dce7b
Junio C Hamano· Jul 27, 2016, 19:28 UTC · re: Jeff King · lore

Re: [PATCH 2/2] t4205: indent here documents

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> 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
I'll do s/they things/they use things/ here while queuing.
Thanks.
Show 583 quoted lines
> 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 <peff@peff.net>
> ---
>  t/t4205-log-pretty-formats.sh | 392 +++++++++++++++++++++---------------------
>  1 file changed, 196 insertions(+), 196 deletions(-)
>
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index d6518fa..f5435fd 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -145,199 +145,199 @@ test_expect_success 'setup more commits' '
>  
>  test_expect_success 'left alignment formatting' '
>  	git log --pretty="tformat:%<(40)%s" >actual &&
> -	qz_to_tab_space <<EOF >expected &&
> -message two                            Z
> -message one                            Z
> -add bar                                Z
> -$(commit_msg)                    Z
> -EOF
> +	qz_to_tab_space <<-EOF >expected &&
> +	message two                            Z
> +	message one                            Z
> +	add bar                                Z
> +	$(commit_msg)                    Z
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left alignment formatting. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%<(40)%s" >actual &&
> -	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -message two                            Z
> -message one                            Z
> -add bar                                Z
> -$(commit_msg)                    Z
> -EOF
> +	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	message two                            Z
> +	message one                            Z
> +	add bar                                Z
> +	$(commit_msg)                    Z
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left alignment formatting at the nth column' '
>  	git log --pretty="tformat:%h %<|(40)%s" >actual &&
> -	qz_to_tab_space <<EOF >expected &&
> -$head1 message two                    Z
> -$head2 message one                    Z
> -$head3 add bar                        Z
> -$head4 $(commit_msg)            Z
> -EOF
> +	qz_to_tab_space <<-EOF >expected &&
> +	$head1 message two                    Z
> +	$head2 message one                    Z
> +	$head3 add bar                        Z
> +	$head4 $(commit_msg)            Z
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left alignment formatting at the nth column' '
>  	COLUMNS=50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
> -	qz_to_tab_space <<EOF >expected &&
> -$head1 message two                    Z
> -$head2 message one                    Z
> -$head3 add bar                        Z
> -$head4 $(commit_msg)            Z
> -EOF
> +	qz_to_tab_space <<-EOF >expected &&
> +	$head1 message two                    Z
> +	$head2 message one                    Z
> +	$head3 add bar                        Z
> +	$head4 $(commit_msg)            Z
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left alignment formatting at the nth column. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%h %<|(40)%s" >actual &&
> -	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -$head1 message two                    Z
> -$head2 message one                    Z
> -$head3 add bar                        Z
> -$head4 $(commit_msg)            Z
> -EOF
> +	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	$head1 message two                    Z
> +	$head2 message one                    Z
> +	$head3 add bar                        Z
> +	$head4 $(commit_msg)            Z
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left alignment formatting with no padding' '
>  	git log --pretty="tformat:%<(1)%s" >actual &&
> -	cat <<EOF >expected &&
> -message two
> -message one
> -add bar
> -$(commit_msg)
> -EOF
> +	cat <<-EOF >expected &&
> +	message two
> +	message one
> +	add bar
> +	$(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left alignment formatting with no padding. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%<(1)%s" >actual &&
> -	cat <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -message two
> -message one
> -add bar
> -$(commit_msg)
> -EOF
> +	cat <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	message two
> +	message one
> +	add bar
> +	$(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left alignment formatting with trunc' '
>  	git log --pretty="tformat:%<(10,trunc)%s" >actual &&
> -	qz_to_tab_space <<EOF >expected &&
> -message ..
> -message ..
> -add bar  Z
> -initial...
> -EOF
> +	qz_to_tab_space <<-\EOF >expected &&
> +	message ..
> +	message ..
> +	add bar  Z
> +	initial...
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left alignment formatting with trunc. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%<(10,trunc)%s" >actual &&
> -	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -message ..
> -message ..
> -add bar  Z
> -initial...
> -EOF
> +	qz_to_tab_space <<-\EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	message ..
> +	message ..
> +	add bar  Z
> +	initial...
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left alignment formatting with ltrunc' '
>  	git log --pretty="tformat:%<(10,ltrunc)%s" >actual &&
> -	qz_to_tab_space <<EOF >expected &&
> -..sage two
> -..sage one
> -add bar  Z
> -..${sample_utf8_part}lich
> -EOF
> +	qz_to_tab_space <<-EOF >expected &&
> +	..sage two
> +	..sage one
> +	add bar  Z
> +	..${sample_utf8_part}lich
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left alignment formatting with ltrunc. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%<(10,ltrunc)%s" >actual &&
> -	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -..sage two
> -..sage one
> -add bar  Z
> -..${sample_utf8_part}lich
> -EOF
> +	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	..sage two
> +	..sage one
> +	add bar  Z
> +	..${sample_utf8_part}lich
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left alignment formatting with mtrunc' '
>  	git log --pretty="tformat:%<(10,mtrunc)%s" >actual &&
> -	qz_to_tab_space <<EOF >expected &&
> -mess.. two
> -mess.. one
> -add bar  Z
> -init..lich
> -EOF
> +	qz_to_tab_space <<-\EOF >expected &&
> +	mess.. two
> +	mess.. one
> +	add bar  Z
> +	init..lich
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left alignment formatting with mtrunc. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%<(10,mtrunc)%s" >actual &&
> -	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -mess.. two
> -mess.. one
> -add bar  Z
> -init..lich
> -EOF
> +	qz_to_tab_space <<-\EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	mess.. two
> +	mess.. one
> +	add bar  Z
> +	init..lich
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'right alignment formatting' '
>  	git log --pretty="tformat:%>(40)%s" >actual &&
> -	qz_to_tab_space <<EOF >expected &&
> -Z                            message two
> -Z                            message one
> -Z                                add bar
> -Z                    $(commit_msg)
> -EOF
> +	qz_to_tab_space <<-EOF >expected &&
> +	Z                            message two
> +	Z                            message one
> +	Z                                add bar
> +	Z                    $(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'right alignment formatting. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%>(40)%s" >actual &&
> -	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -Z                            message two
> -Z                            message one
> -Z                                add bar
> -Z                    $(commit_msg)
> -EOF
> +	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	Z                            message two
> +	Z                            message one
> +	Z                                add bar
> +	Z                    $(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'right alignment formatting at the nth column' '
>  	git log --pretty="tformat:%h %>|(40)%s" >actual &&
> -	qz_to_tab_space <<EOF >expected &&
> -$head1                      message two
> -$head2                      message one
> -$head3                          add bar
> -$head4              $(commit_msg)
> -EOF
> +	qz_to_tab_space <<-EOF >expected &&
> +	$head1                      message two
> +	$head2                      message one
> +	$head3                          add bar
> +	$head4              $(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'right alignment formatting at the nth column' '
>  	COLUMNS=50 git log --pretty="tformat:%h %>|(-10)%s" >actual &&
> -	qz_to_tab_space <<EOF >expected &&
> -$head1                      message two
> -$head2                      message one
> -$head3                          add bar
> -$head4              $(commit_msg)
> -EOF
> +	qz_to_tab_space <<-EOF >expected &&
> +	$head1                      message two
> +	$head2                      message one
> +	$head3                          add bar
> +	$head4              $(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'right alignment formatting at the nth column. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%h %>|(40)%s" >actual &&
> -	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -$head1                      message two
> -$head2                      message one
> -$head3                          add bar
> -$head4              $(commit_msg)
> -EOF
> +	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	$head1                      message two
> +	$head2                      message one
> +	$head3                          add bar
> +	$head4              $(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
> @@ -345,110 +345,110 @@ EOF
>  # as in previous test.
>  test_expect_success 'right alignment formatting at the nth column with --graph. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --graph --pretty="tformat:%h %>|(40)%s" >actual &&
> -	iconv -f utf-8 -t $test_encoding >expected <<EOF&&
> -* $head1                    message two
> -* $head2                    message one
> -* $head3                        add bar
> -* $head4            $(commit_msg)
> -EOF
> +	iconv -f utf-8 -t $test_encoding >expected <<-EOF &&
> +	* $head1                    message two
> +	* $head2                    message one
> +	* $head3                        add bar
> +	* $head4            $(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'right alignment formatting with no padding' '
>  	git log --pretty="tformat:%>(1)%s" >actual &&
> -	cat <<EOF >expected &&
> -message two
> -message one
> -add bar
> -$(commit_msg)
> -EOF
> +	cat <<-EOF >expected &&
> +	message two
> +	message one
> +	add bar
> +	$(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'right alignment formatting with no padding and with --graph' '
>  	git log --graph --pretty="tformat:%>(1)%s" >actual &&
> -	cat <<EOF >expected &&
> -* message two
> -* message one
> -* add bar
> -* $(commit_msg)
> -EOF
> +	cat <<-EOF >expected &&
> +	* message two
> +	* message one
> +	* add bar
> +	* $(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'right alignment formatting with no padding. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%>(1)%s" >actual &&
> -	cat <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -message two
> -message one
> -add bar
> -$(commit_msg)
> -EOF
> +	cat <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	message two
> +	message one
> +	add bar
> +	$(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'center alignment formatting' '
>  	git log --pretty="tformat:%><(40)%s" >actual &&
> -	qz_to_tab_space <<EOF >expected &&
> -Z             message two              Z
> -Z             message one              Z
> -Z               add bar                Z
> -Z         $(commit_msg)          Z
> -EOF
> +	qz_to_tab_space <<-EOF >expected &&
> +	Z             message two              Z
> +	Z             message one              Z
> +	Z               add bar                Z
> +	Z         $(commit_msg)          Z
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'center alignment formatting. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%><(40)%s" >actual &&
> -	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -Z             message two              Z
> -Z             message one              Z
> -Z               add bar                Z
> -Z         $(commit_msg)          Z
> -EOF
> +	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	Z             message two              Z
> +	Z             message one              Z
> +	Z               add bar                Z
> +	Z         $(commit_msg)          Z
> +	EOF
>  	test_cmp expected actual
>  '
>  test_expect_success 'center alignment formatting at the nth column' '
>  	git log --pretty="tformat:%h %><|(40)%s" >actual &&
> -	qz_to_tab_space <<EOF >expected &&
> -$head1           message two          Z
> -$head2           message one          Z
> -$head3             add bar            Z
> -$head4       $(commit_msg)      Z
> -EOF
> +	qz_to_tab_space <<-EOF >expected &&
> +	$head1           message two          Z
> +	$head2           message one          Z
> +	$head3             add bar            Z
> +	$head4       $(commit_msg)      Z
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'center alignment formatting at the nth column' '
>  	COLUMNS=70 git log --pretty="tformat:%h %><|(-30)%s" >actual &&
> -	qz_to_tab_space <<EOF >expected &&
> -$head1           message two          Z
> -$head2           message one          Z
> -$head3             add bar            Z
> -$head4       $(commit_msg)      Z
> -EOF
> +	qz_to_tab_space <<-EOF >expected &&
> +	$head1           message two          Z
> +	$head2           message one          Z
> +	$head3             add bar            Z
> +	$head4       $(commit_msg)      Z
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'center alignment formatting at the nth column. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%h %><|(40)%s" >actual &&
> -	qz_to_tab_space <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -$head1           message two          Z
> -$head2           message one          Z
> -$head3             add bar            Z
> -$head4       $(commit_msg)      Z
> -EOF
> +	qz_to_tab_space <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	$head1           message two          Z
> +	$head2           message one          Z
> +	$head3             add bar            Z
> +	$head4       $(commit_msg)      Z
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'center alignment formatting with no padding' '
>  	git log --pretty="tformat:%><(1)%s" >actual &&
> -	cat <<EOF >expected &&
> -message two
> -message one
> -add bar
> -$(commit_msg)
> -EOF
> +	cat <<-EOF >expected &&
> +	message two
> +	message one
> +	add bar
> +	$(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
> @@ -457,34 +457,34 @@ EOF
>  old_head1=$(git rev-parse --verify HEAD~0)
>  test_expect_success 'center alignment formatting with no padding. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%><(1)%s" >actual &&
> -	cat <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -message two
> -message one
> -add bar
> -$(commit_msg)
> -EOF
> +	cat <<-EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	message two
> +	message one
> +	add bar
> +	$(commit_msg)
> +	EOF
>  	test_cmp expected actual
>  '
>  
>  test_expect_success 'left/right alignment formatting with stealing' '
>  	git commit --amend -m short --author "long long long <long@me.com>" &&
>  	git log --pretty="tformat:%<(10,trunc)%s%>>(10,ltrunc)% an" >actual &&
> -	cat <<EOF >expected &&
> -short long  long long
> -message ..   A U Thor
> -add bar      A U Thor
> -initial...   A U Thor
> -EOF
> +	cat <<-\EOF >expected &&
> +	short long  long long
> +	message ..   A U Thor
> +	add bar      A U Thor
> +	initial...   A U Thor
> +	EOF
>  	test_cmp expected actual
>  '
>  test_expect_success 'left/right alignment formatting with stealing. i18n.logOutputEncoding' '
>  	git -c i18n.logOutputEncoding=$test_encoding log --pretty="tformat:%<(10,trunc)%s%>>(10,ltrunc)% an" >actual &&
> -	cat <<EOF | iconv -f utf-8 -t $test_encoding >expected &&
> -short long  long long
> -message ..   A U Thor
> -add bar      A U Thor
> -initial...   A U Thor
> -EOF
> +	cat <<-\EOF | iconv -f utf-8 -t $test_encoding >expected &&
> +	short long  long long
> +	message ..   A U Thor
> +	add bar      A U Thor
> +	initial...   A U Thor
> +	EOF
>  	test_cmp expected actual
>  '
>  
> @@ -515,22 +515,22 @@ test_expect_success 'log decoration properly follows tag chain' '
>  	git tag -d tag1 &&
>  	git commit --amend -m shorter &&
>  	git log --no-walk --tags --pretty="%H %d" --decorate=full >actual &&
> -	cat <<EOF >expected &&
> -$head1  (tag: refs/tags/tag2)
> -$head2  (tag: refs/tags/message-one)
> -$old_head1  (tag: refs/tags/message-two)
> -EOF
> +	cat <<-EOF >expected &&
> +	$head1  (tag: refs/tags/tag2)
> +	$head2  (tag: refs/tags/message-one)
> +	$old_head1  (tag: refs/tags/message-two)
> +	EOF
>  	sort actual >actual1 &&
>  	test_cmp expected actual1
>  '
>  
>  test_expect_success 'clean log decoration' '
>  	git log --no-walk --tags --pretty="%H %D" --decorate=full >actual &&
> -	cat >expected <<EOF &&
> -$head1 tag: refs/tags/tag2
> -$head2 tag: refs/tags/message-one
> -$old_head1 tag: refs/tags/message-two
> -EOF
> +	cat >expected <<-EOF &&
> +	$head1 tag: refs/tags/tag2
> +	$head2 tag: refs/tags/message-one
> +	$old_head1 tag: refs/tags/message-two
> +	EOF
>  	sort actual >actual1 &&
>  	test_cmp expected actual1
>  '
Jeff King· Jul 27, 2016, 19:57 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] t4205: indent here documents

On Wed, Jul 27, 2016 at 12:28:14PM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> > 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
> 
> I'll do s/they things/they use things/ here while queuing.
Whoops, thanks.
-Peff
Phil Pennock· Jul 27, 2016, 18:41 UTC · re: Jeff King · lore

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

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
Jeff King· Jul 27, 2016, 19:16 UTC · re: Phil Pennock · lore

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

On Wed, Jul 27, 2016 at 06:41:41PM +0000, Phil Pennock wrote:
Show 6 quoted lines
> 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.
I think the pattern comes more from syscalls.
Show 8 quoted lines
> > 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.

Show 7 quoted lines
> 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

← back to recent threads