[Bug] %[a|c]d placeholder does not respect --date= option in combination with git archive

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

It seems like that the place holders %ad and %cd do not respect the
- --date= option when used as part of the export substitution.

In my file I have the place holder $Format:%ad$ and in .git/config the
setting log.date = short is present.

I can very this the date setting by running
	git log --pretty=format:%ad
and I get:
2011-03-03
2011-02-28
2011-02-28
2011-02-28
2011-02-28


Now if I run on the same repo
	git archive --format=zip HEAD -o out.zip
and check the place holder in the exported zip file it is actually
replaced with:

Thu, 3 Mar 2011 10:06:43 +0100

and not

2011-03-03

The same happens with the place holder %cd

/Dietmar/


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iJwEAQECAAYFAk1vX6gACgkQCXG8gXafJGFfHwQApYJ/y0bS0D97fMKGjjBjjZQv
sOrPbwxIaPII2RGeRqxlQLjDL2kYnXHObTor+3rLWbNHLXPjjPw/2r4YIeCxz/f+
vA8ro9o4dTAyvGiUc/xUhu/U4XaVuV4Rl3QX83oaGmuefHRGc5/esex4R4mnzVdW
QBPVvqqs25gyiu7zV6s=
=lVtM
-----END PGP SIGNATURE-----

On Thu, Mar 03, 2011 at 10:30:16AM +0100, Dietmar Winkler wrote:

In my file I have the place holder $Format:%ad$ and in .git/config the
setting log.date = short is present.
[...]
Now if I run on the same repo
	git archive --format=zip HEAD -o out.zip
and check the place holder in the exported zip file it is actually
replaced with:

Thu, 3 Mar 2011 10:06:43 +0100

and not

2011-03-03

I am not sure that this is a bug. The log.date parameter is about the
log command, not necessarily other format substitutions. If it were any
other date format, I would say the right answer is that you should be
using one of the format-specific date specifiers. But annoyingly, there
is no such specifier for "short". Which means there is no way to
actually get the output that you want.

I remember at some point discussing extending the specifier syntax to
allow things like "%(ad,date=short)", but it was never implemented. I
think that would be the cleanest way to do what you want.

The second cleanest would be adding an archive.date variable. Which is
much simpler, obviously. But I think making "log.date" start applying to
archive substitutions is going to surprise some people and possibly
break their setups.

-Peff

Jeff and list,

Den 03. mars 2011 16:10, skrev Jeff King:

I am not sure that this is a bug. The log.date parameter is about the
log command, not necessarily other format substitutions. 

Well in
http://www.kernel.org/pub/software/scm/git/docs/gitattributes.html it says:

  "The placeholders are the same as those for the option
--pretty=format: of git-log(1), except that they need to be wrapped like
this: $Format:PLACEHOLDERS$ in the file."

And in git log the list includes (besides the various date formats) also

 %ad: author date (format respects --date= option)
  ...
 %cd: committer date *

*) actually here the string "(format respects --date= option)" is
missing. Otherwise what committer date format are we speaking about ;)

So either the documentation should make clear that the substitution will
*not* work or (and this would be preferable) fix the substitution so
that it works as documented.

I remember at some point discussing extending the specifier syntax to
allow things like "%(ad,date=short)", but it was never implemented. I
think that would be the cleanest way to do what you want.

Yes that would be even better since it would give one the freedom of
defining different format for the subsitutions  in different places in a
project. Shame it was not accepted.

The second cleanest would be adding an archive.date variable. Which is
much simpler, obviously. But I think making "log.date" start applying to
archive substitutions is going to surprise some people and possibly
break their setups.

How should this surprise people? If the used %ad they would have
expected a configuration depended substitution to start with. If they
wanted a log.date *independent* substitution they should have (according
to the documentation) some of the other formats (e.g., %ar, %ai, ...).
So I don't really see this as a reason for not fixing this bug.


/Dietmar/

On Fri, Mar 04, 2011 at 11:10:36AM +0100, Dietmar Winkler wrote:

Well in
http://www.kernel.org/pub/software/scm/git/docs/gitattributes.html it says:

  "The placeholders are the same as those for the option
--pretty=format: of git-log(1), except that they need to be wrapped like
this: $Format:PLACEHOLDERS$ in the file."

And in git log the list includes (besides the various date formats) also

 %ad: author date (format respects --date= option)
  ...
 %cd: committer date *

*) actually here the string "(format respects --date= option)" is
missing. Otherwise what committer date format are we speaking about ;)

So either the documentation should make clear that the substitution will
*not* work or (and this would be preferable) fix the substitution so
that it works as documented.

Yeah, the documentation is misleadingly vague there. I've improved it in
the patch series below.

quoted

I remember at some point discussing extending the specifier syntax to
allow things like "%(ad,date=short)", but it was never implemented. I
think that would be the cleanest way to do what you want.

Yes that would be even better since it would give one the freedom of
defining different format for the subsitutions  in different places in a
project. Shame it was not accepted.

I think we got bogged down in what exactly the extended format should
look like and then nothing got done. I spent a few hours yesterday
looking again at how bad it would be to extend the syntax to handle both
the traditional format and '%(foo,arg=value)' but there are lot of
corner cases.

So this morning I scrapped that and just added "%ad(mode)" which was
much simpler, and matches syntactically with some of our other commands.
It's in the series below.

quoted

The second cleanest would be adding an archive.date variable. Which is
much simpler, obviously. But I think making "log.date" start applying to
archive substitutions is going to surprise some people and possibly
break their setups.

How should this surprise people? If the used %ad they would have
expected a configuration depended substitution to start with. If they
wanted a log.date *independent* substitution they should have (according
to the documentation) some of the other formats (e.g., %ar, %ai, ...).
So I don't really see this as a reason for not fixing this bug.

Imagine a project which uses "git archive" as part of its scripts for
building a distribution tarball. I.e., you run "make dist" or similar,
and it produces the tarball. The gitattributes and $Format:%ad$
placeholders are contained in the upstream repository. So anybody who
clones it can run "make dist" and get the identical tarball.

Now imagine as a developer on the project, you prefer to see your logs
with a different date format. So you set log.date to "short". But if
git-archive behaves as you want it to, then your "make dist" is now
broken. It generates different results to everyone else's.

Anyway, hopefully the point becomes moot with this patch series, which
lets you do %ad(short) in your format strings:

  [1/2]: pretty.c: give format_person_part the whole placeholder
  [2/2]: pretty.c: allow date formats in user format strings

-Peff

Until now it only got to see the next character. Giving it
the whole string will make it possible to add longer
placeholders in a future patch.

Signed-off-by: Jeff King <redacted>
---
I split this out because the patch ends up so noisy.

 pretty.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/pretty.c b/pretty.c
index 8549934..00bcf83 100644
--- a/pretty.c
+++ b/pretty.c

@@ -440,7 +440,7 @@ static int mailmap_name(char *email, int email_len, char *name, int name_len)
 	return mail_map->nr && map_user(mail_map, email, email_len, name, name_len);
 }
 
-static size_t format_person_part(struct strbuf *sb, char part,
+static size_t format_person_part(struct strbuf *sb, const char *part,
 				 const char *msg, int len, enum date_mode dmode)
 {
 	/* currently all placeholders have same length */

@@ -477,7 +477,7 @@ static size_t format_person_part(struct strbuf *sb, char part,
 		goto skip;
 	end = mail_end-msg;
 
-	if (part == 'N' || part == 'E') { /* mailmap lookup */
+	if (*part == 'N' || *part == 'E') { /* mailmap lookup */
 		strlcpy(person_name, name_start, name_end-name_start+1);
 		strlcpy(person_mail, mail_start, mail_end-mail_start+1);
 		mailmap_name(person_mail, sizeof(person_mail), person_name, sizeof(person_name));

@@ -486,11 +486,11 @@ static size_t format_person_part(struct strbuf *sb, char part,
 		mail_start = person_mail;
 		mail_end = mail_start +  strlen(person_mail);
 	}
-	if (part == 'n' || part == 'N') {	/* name */
+	if (*part == 'n' || *part == 'N') {	/* name */
 		strbuf_add(sb, name_start, name_end-name_start);
 		return placeholder_len;
 	}
-	if (part == 'e' || part == 'E') {	/* email */
+	if (*part == 'e' || *part == 'E') {	/* email */
 		strbuf_add(sb, mail_start, mail_end-mail_start);
 		return placeholder_len;
 	}

@@ -504,7 +504,7 @@ static size_t format_person_part(struct strbuf *sb, char part,
 	if (msg + start == ep)
 		goto skip;
 
-	if (part == 't') {	/* date, UNIX timestamp */
+	if (*part == 't') {	/* date, UNIX timestamp */
 		strbuf_add(sb, msg + start, ep - (msg + start));
 		return placeholder_len;
 	}

@@ -518,7 +518,7 @@ static size_t format_person_part(struct strbuf *sb, char part,
 			tz = -tz;
 	}
 
-	switch (part) {
+	switch (*part) {
 	case 'd':	/* date */
 		strbuf_addstr(sb, show_date(date, tz, dmode));
 		return placeholder_len;

@@ -538,8 +538,8 @@ skip:
 	 * bogus commit, 'sb' cannot be updated, but we still need to
 	 * compute a valid return value.
 	 */
-	if (part == 'n' || part == 'e' || part == 't' || part == 'd'
-	    || part == 'D' || part == 'r' || part == 'i')
+	if (*part == 'n' || *part == 'e' || *part == 't' || *part == 'd'
+	    || *part == 'D' || *part == 'r' || *part == 'i')
 		return placeholder_len;
 
 	return 0; /* unknown placeholder */

@@ -899,11 +899,11 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 
 	switch (placeholder[0]) {
 	case 'a':	/* author ... */
-		return format_person_part(sb, placeholder[1],
+		return format_person_part(sb, placeholder + 1,
 				   msg + c->author.off, c->author.len,
 				   c->pretty_ctx->date_mode);
 	case 'c':	/* committer ... */
-		return format_person_part(sb, placeholder[1],
+		return format_person_part(sb, placeholder + 1,
 				   msg + c->committer.off, c->committer.len,
 				   c->pretty_ctx->date_mode);
 	case 'e':	/* encoding */

-- 
1.7.4.rc1.24.g38985d

You can now do "%ad(short)" or similar (using any format
that works for --date). This makes some formats like %aD
redundant (since you can do "%ad(rfc)"), but of course we
keep them for compatibility.

While we're updating the docs, let's explain in more detail
how the placeholder mode, the --date= option, and the
log.date config all interact.

Signed-off-by: Jeff King <redacted>
---
My only reservation here is the strdup() we need to call
parse_date_format(). We usually try to keep the formatting parsing
lightweight since it gets re-parsed for each commit.

My timings for logging all of git.git showed that the slowdown is lost
in the noise, so it's probably not worth caring about.

 Documentation/pretty-formats.txt |   21 +++++++++++++++++++--
 pretty.c                         |   33 +++++++++++++++++++++++++++++----
 t/t6006-rev-list-format.sh       |   12 ++++++++++++
 3 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 561cc9f..a73a9ac 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt

@@ -109,7 +109,7 @@ The placeholders are:
 - '%aN': author name (respecting .mailmap, see linkgit:git-shortlog[1] or linkgit:git-blame[1])
 - '%ae': author email
 - '%aE': author email (respecting .mailmap, see linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%ad': author date (format respects --date= option)
+- '%ad': author date (see below for format information)
 - '%aD': author date, RFC2822 style
 - '%ar': author date, relative
 - '%at': author date, UNIX timestamp

@@ -118,7 +118,7 @@ The placeholders are:
 - '%cN': committer name (respecting .mailmap, see linkgit:git-shortlog[1] or linkgit:git-blame[1])
 - '%ce': committer email
 - '%cE': committer email (respecting .mailmap, see linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%cd': committer date
+- '%cd': committer date (see below for format information)
 - '%cD': committer date, RFC2822 style
 - '%cr': committer date, relative
 - '%ct': committer date, UNIX timestamp

@@ -151,6 +151,23 @@ insert an empty string unless we are traversing reflog entries (e.g., by
 `git log -g`). The `%d` placeholder will use the "short" decoration
 format if `--decorate` was not already provided on the command line.
 
+Dates given by `%ad` and `%cd` are formatted according to the following
+rules:
+
+  1. A date mode in parentheses may follow the placeholder. For example,
+     `%ad(iso8601)` will format the author date in the ISO8601 format.
+     You may specify any mode valid for the `--date=` option of
+     linkgit:git-log[1].
+
+  2. If no date mode is specified, and the command respects the
+     `--date=` option, the mode specified by that option is used.
+
+  3. Otherwise, if the format is used by the log family of commands and
+     the `log.date` config option is set, the mode specified by that
+     option is used.
+
+  4. Otherwise, the format is equivalent to that of --date=default.
+
 If you add a `{plus}` (plus sign) after '%' of a placeholder, a line-feed
 is inserted immediately before the expansion if and only if the
 placeholder expands to a non-empty string.

diff --git a/pretty.c b/pretty.c
index 00bcf83..d0bf2a0 100644
--- a/pretty.c
+++ b/pretty.c

@@ -440,6 +440,27 @@ static int mailmap_name(char *email, int email_len, char *name, int name_len)
 	return mail_map->nr && map_user(mail_map, email, email_len, name, name_len);
 }
 
+static size_t format_date(struct strbuf *sb, const char *part,
+			  unsigned long date, int tz, enum date_mode dmode)
+{
+	int consumed = 0;
+	if (*part == '(') {
+		char *v;
+		consumed++;
+		while (part[consumed] && part[consumed] != ')')
+			consumed++;
+		/* yuck, we do this malloc for every commit */
+		v = xstrndup(part + 1, consumed - 1);
+		dmode = parse_date_format(v);
+		free(v);
+		if (part[consumed] == ')')
+			consumed++;
+	}
+	if (sb)
+		strbuf_addstr(sb, show_date(date, tz, dmode));
+	return consumed;
+}
+
 static size_t format_person_part(struct strbuf *sb, const char *part,
 				 const char *msg, int len, enum date_mode dmode)
 {

@@ -519,9 +540,9 @@ static size_t format_person_part(struct strbuf *sb, const char *part,
 	}
 
 	switch (*part) {
-	case 'd':	/* date */
-		strbuf_addstr(sb, show_date(date, tz, dmode));
-		return placeholder_len;
+	case 'd':	/* date, possibly with format */
+		return placeholder_len +
+			format_date(sb, part + 1, date, tz, dmode);
 	case 'D':	/* date, RFC2822 style */
 		strbuf_addstr(sb, show_date(date, tz, DATE_RFC2822));
 		return placeholder_len;

@@ -538,9 +559,13 @@ skip:
 	 * bogus commit, 'sb' cannot be updated, but we still need to
 	 * compute a valid return value.
 	 */
-	if (*part == 'n' || *part == 'e' || *part == 't' || *part == 'd'
+	if (*part == 'n' || *part == 'e' || *part == 't'
 	    || *part == 'D' || *part == 'r' || *part == 'i')
 		return placeholder_len;
+	/* handle 'd' separately, as it is variable length */
+	if (*part == 'd')
+		return placeholder_len +
+			format_date(NULL, part + 1, 0, 0, 0);
 
 	return 0; /* unknown placeholder */
 }

diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index d918cc0..b9cef1f 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh

@@ -176,6 +176,18 @@ test_expect_success '%ad respects --date=' '
 	test_cmp expect.ad-short output.ad-short
 '
 
+test_format 'date-with-mode' '%ad(short)%n%ad(iso)' <<'EOF'
+commit f58db70b055c5718631e5c61528b28b12090cdea
+2005-04-07
+2005-04-07 15:13:13 -0700
+commit 131a310eb913d107dd3c09a65d1651175898735d
+2005-04-07
+2005-04-07 15:13:13 -0700
+commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
+2005-04-07
+2005-04-07 15:13:13 -0700
+EOF
+
 test_expect_success 'empty email' '
 	test_tick &&
 	C=$(GIT_AUTHOR_EMAIL= git commit-tree HEAD^{tree} </dev/null) &&

-- 
1.7.4.rc1.24.g38985d