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

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

[Dietmar Winkler]

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

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

[Jeff King]

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

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

[Dietmar Winkler]

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/

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

[Jeff King]

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.

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

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

[PATCH 1/2] pretty.c: give format_person_part the whole placeholder

[Jeff King]

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 <peff@peff.net>
---
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

[PATCH 2/2] pretty.c: allow date formats in user format strings

[Jeff King]

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 <peff@peff.net>
---
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

Fwd: [PATCH 2/2] pretty.c: allow date formats in user format strings

[Will Palmer]

(I think I accidentally hit "reply" instead of "reply all", there, so
forwarding to list)

On Sat, Mar 5, 2011 at 8:00 PM, Jeff King <peff@peff.net> wrote:
> 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.
>

The more I see long formats like this, the more I think it would make
sense to make formats %(likeThis), the way for-each-ref does.
Ideally, these formats could even be unified, at some point.

I tried this a long while ago, as part of my attempt to make all
pre-defined formats work in terms of format strings, but that turned
into too much of a bloated mess to bother submitting. I don't know
if there's enough interest in such a thing to justify trying again (or to
justify rebasing the bloated version, cleaning it up and submitting it
as-is, for that matter)

Point is: we're going to keep having more and more format options,
I think that's a given. At some point, these short mnemonics will just
stop making sense, and it makes sense to have an escape plan when
that happens.

> 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 <peff@peff.net>
> ---
> 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
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: Fwd: [PATCH 2/2] pretty.c: allow date formats in user format strings

[Jeff King]

On Sun, Mar 06, 2011 at 09:54:01PM +0000, Will Palmer wrote:

> On Sat, Mar 5, 2011 at 8:00 PM, Jeff King <peff@peff.net> wrote:
> > 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.
> >
> 
> The more I see long formats like this, the more I think it would make
> sense to make formats %(likeThis), the way for-each-ref does.
> Ideally, these formats could even be unified, at some point.

Yeah, I totally agree. One problem is that everytime an extended format
comes up it gets bikeshedded to death as everybody mentions their
favorite format and/or feature, and then nobody codes it.

> I tried this a long while ago, as part of my attempt to make all
> pre-defined formats work in terms of format strings, but that turned
> into too much of a bloated mess to bother submitting. I don't know
> if there's enough interest in such a thing to justify trying again (or to
> justify rebasing the bloated version, cleaning it up and submitting it
> as-is, for that matter)

I think there is interest. I'd be curious to see what you have. A few
days ago, when working on this series, I tried to make a
minimally-invasive change to allow "%(ad)" to work alongside "%ad", with
a generic arguments format like %(ad:flag:key=value). Which would allow
existing shorthand, for-each-ref-style %(refname:short), and leave room
for arbitrary extension of each placeholder (alongside more
human-readable placeholder names).

The problem I ran into was the internal code interface. We parse the
format string each time we expand it. This works OK for simple
printf-like stuff. But ideally we can handle something like:

  %(ad:key=embedded\:colon:key2=embedded\)paren)

It's hard to make a nice interface to that which doesn't involve copying
the quoted string out into a non-quoted version. But we don't want to be
doing a bunch of parsing and allocation per-expansion. It's slow, and
this expansion happens inside a fairly tight loop in many cases (e.g.,
during rev-list).

So I think the whole thing needs to be factored into two phases: a
parsing phase where we build some internal parse tree, and then an
expansion phase where we walk the parse tree for each commit (or ref, or
whatever is being expanded).

> Point is: we're going to keep having more and more format options,
> I think that's a given. At some point, these short mnemonics will just
> stop making sense, and it makes sense to have an escape plan when
> that happens.

Agreed. And I think it is possible to do it in a backwards-compatible
way; support %(longname:options) for everything, and keep short-hands
like %h and %ad for existing elements without options.

-Peff

Re: Fwd: [PATCH 2/2] pretty.c: allow date formats in user format strings

[Will Palmer]

On Mon, 2011-03-07 at 11:17 -0500, Jeff King wrote:
> On Sun, Mar 06, 2011 at 09:54:01PM +0000, Will Palmer wrote:
> 
> > On Sat, Mar 5, 2011 at 8:00 PM, Jeff King <peff@peff.net> wrote:
> > > 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.
> > >
> > 
> > The more I see long formats like this, the more I think it would make
> > sense to make formats %(likeThis), the way for-each-ref does.
> > Ideally, these formats could even be unified, at some point.
> 
> Yeah, I totally agree. One problem is that everytime an extended format
> comes up it gets bikeshedded to death as everybody mentions their
> favorite format and/or feature, and then nobody codes it.
> 
> > I tried this a long while ago, as part of my attempt to make all
> > pre-defined formats work in terms of format strings, but that turned
> > into too much of a bloated mess to bother submitting. I don't know
> > if there's enough interest in such a thing to justify trying again (or to
> > justify rebasing the bloated version, cleaning it up and submitting it
> > as-is, for that matter)
> 
> I think there is interest. I'd be curious to see what you have. A few
> days ago, when working on this series, I tried to make a
> minimally-invasive change to allow "%(ad)" to work alongside "%ad", with
> a generic arguments format like %(ad:flag:key=value). Which would allow
> existing shorthand, for-each-ref-style %(refname:short), and leave room
> for arbitrary extension of each placeholder (alongside more
> human-readable placeholder names).
> 
> The problem I ran into was the internal code interface. We parse the
> format string each time we expand it. This works OK for simple
> printf-like stuff. But ideally we can handle something like:
>   %(ad:key=embedded\:colon:key2=embedded\)paren)
> 
> It's hard to make a nice interface to that which doesn't involve copying
> the quoted string out into a non-quoted version. But we don't want to be
> doing a bunch of parsing and allocation per-expansion. It's slow, and
> this expansion happens inside a fairly tight loop in many cases (e.g.,
> during rev-list).

Exactly the problem I ran into.

> 
> So I think the whole thing needs to be factored into two phases: a
> parsing phase where we build some internal parse tree, and then an
> expansion phase where we walk the parse tree for each commit (or ref, or
> whatever is being expanded).

And exactly the solution I implemented.
At the time, it felt like needless bloat, but perhaps the problem has
gotten to the point where it's worth it.

I assume rebasing what I have right now would be problematic, but it
sounds like it's about time to give it another go.

The code was ever only in a "proof of concept" stage- I had it working
for single revisions, but in a way which wasn't yet compatible with any
of the other parts of log, iirc.

I'll try getting a rebase started tonight, but in the mean time
I /think/ the latest code is at 
https://github.com/wpalmer/git/tree/pretty/parse-format-poc

Warning: quite ugly.

If you have comments, I would not mind hearing them (though off-list
might be better)

> 
> > Point is: we're going to keep having more and more format options,
> > I think that's a given. At some point, these short mnemonics will just
> > stop making sense, and it makes sense to have an escape plan when
> > that happens.
> 
> Agreed. And I think it is possible to do it in a backwards-compatible
> way; support %(longname:options) for everything, and keep short-hands
> like %h and %ad for existing elements without options.
> 
> -Peff

Re: Fwd: [PATCH 2/2] pretty.c: allow date formats in user format strings

[Will Palmer]

On Mon, 2011-03-07 at 17:28 +0000, Will Palmer wrote:
> On Mon, 2011-03-07 at 11:17 -0500, Jeff King wrote:
> > On Sun, Mar 06, 2011 at 09:54:01PM +0000, Will Palmer wrote:
> > 
> > > On Sat, Mar 5, 2011 at 8:00 PM, Jeff King <peff@peff.net> wrote:
> > > > 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.
> > > >
> > > 
> > > The more I see long formats like this, the more I think it would make
> > > sense to make formats %(likeThis), the way for-each-ref does.
> > > Ideally, these formats could even be unified, at some point.
> > 
> > Yeah, I totally agree. One problem is that everytime an extended format
> > comes up it gets bikeshedded to death as everybody mentions their
> > favorite format and/or feature, and then nobody codes it.
> > 
> > > I tried this a long while ago, as part of my attempt to make all
> > > pre-defined formats work in terms of format strings, but that turned
> > > into too much of a bloated mess to bother submitting. I don't know
> > > if there's enough interest in such a thing to justify trying again (or to
> > > justify rebasing the bloated version, cleaning it up and submitting it
> > > as-is, for that matter)
> > 
> > I think there is interest. I'd be curious to see what you have. A few
> > days ago, when working on this series, I tried to make a
> > minimally-invasive change to allow "%(ad)" to work alongside "%ad", with
> > a generic arguments format like %(ad:flag:key=value). Which would allow
> > existing shorthand, for-each-ref-style %(refname:short), and leave room
> > for arbitrary extension of each placeholder (alongside more
> > human-readable placeholder names).
> > 
> > The problem I ran into was the internal code interface. We parse the
> > format string each time we expand it. This works OK for simple
> > printf-like stuff. But ideally we can handle something like:
> >   %(ad:key=embedded\:colon:key2=embedded\)paren)
> > 
> > It's hard to make a nice interface to that which doesn't involve copying
> > the quoted string out into a non-quoted version. But we don't want to be
> > doing a bunch of parsing and allocation per-expansion. It's slow, and
> > this expansion happens inside a fairly tight loop in many cases (e.g.,
> > during rev-list).
> 
> Exactly the problem I ran into.
> 
> > 
> > So I think the whole thing needs to be factored into two phases: a
> > parsing phase where we build some internal parse tree, and then an
> > expansion phase where we walk the parse tree for each commit (or ref, or
> > whatever is being expanded).
> 
> And exactly the solution I implemented.
> At the time, it felt like needless bloat, but perhaps the problem has
> gotten to the point where it's worth it.
> 
> I assume rebasing what I have right now would be problematic, but it
> sounds like it's about time to give it another go.
> 
> The code was ever only in a "proof of concept" stage- I had it working
> for single revisions, but in a way which wasn't yet compatible with any
> of the other parts of log, iirc.
> 
> I'll try getting a rebase started tonight, but in the mean time
> I /think/ the latest code is at 
> https://github.com/wpalmer/git/tree/pretty/parse-format-poc
> 

I'm home now, and apparently that should have been:
https://github.com/wpalmer/git/tree/pretty/parse-format

I assume the code is very hard to follow, as it was pretty much written
with the mindset of "get it done now, fix it later". Looking into it
again, I see that part of the reason I abandoned it was not being able
to determine a good way to split things into logical commits. It's
almost entirely an "everything works or nothing works" change.

There was of course one section of it that I managed to split out, which
is the "format aliases" code, already merged. I assume that this code
has absolutely never been used since inclusion, as what it was actually
intended to support was never finished.

To see it in action, try:
 ./git log --pretty='%h%(opt-color ? %Cred) foo'

uncommenting the //parts_debug(parsed, 0);    line in pretty.c will show
off the built format tree.

> Warning: quite ugly.
> 
> If you have comments, I would not mind hearing them (though off-list
> might be better)
> 
> > 
> > > Point is: we're going to keep having more and more format options,
> > > I think that's a given. At some point, these short mnemonics will just
> > > stop making sense, and it makes sense to have an escape plan when
> > > that happens.
> > 
> > Agreed. And I think it is possible to do it in a backwards-compatible
> > way; support %(longname:options) for everything, and keep short-hands
> > like %h and %ad for existing elements without options.
> > 
> > -Peff
> 
> 

Re: Fwd: [PATCH 2/2] pretty.c: allow date formats in user format strings

[Jeff King]

On Mon, Mar 07, 2011 at 06:50:34PM +0000, Will Palmer wrote:

> I'm home now, and apparently that should have been:
> https://github.com/wpalmer/git/tree/pretty/parse-format
> 
> I assume the code is very hard to follow, as it was pretty much written
> with the mindset of "get it done now, fix it later". Looking into it
> again, I see that part of the reason I abandoned it was not being able
> to determine a good way to split things into logical commits. It's
> almost entirely an "everything works or nothing works" change.

I haven't looked at your code yet, but the breakdown of patches I would
expect / hope for is something like:

  1. introduce infrastructure for creating parse-tree from strbuf_expand
     format, with some tests

  2. port format_commit_* over to new system; I would expect that the
     caller code will have to be part of both the parsing and the
     expansion, since the generic code can't know that "%ad" is
     meaningful (and we want to keep it for backwards compatibility).
     Leave format_commit_message as a parse + expand wrapper for simple
     callers who don't care about speed.

  3. Add generic "%(key:option)" support to the new infrastructure,
     forward-porting format_commit_* as necessary (and hopefully the
     change are minimal...).

So those are all big commits, obviously, but hopefully it lets us review
in three stages: does the new infrastructure look good, does porting an
existing caller (and probably the most complex caller) clean up the
caller code, and then finally, does the new syntax look good?

But of course the devil is in the details, so probably that breakdown
has some flaw in it. :) I'll see when I look at your code how close to
reality I came.

-Peff

Re: Fwd: [PATCH 2/2] pretty.c: allow date formats in user format strings

[Will Palmer]

On Mon, 2011-03-07 at 14:26 -0500, Jeff King wrote:
> On Mon, Mar 07, 2011 at 06:50:34PM +0000, Will Palmer wrote:
> 
> > I'm home now, and apparently that should have been:
> > https://github.com/wpalmer/git/tree/pretty/parse-format
> > 
> > I assume the code is very hard to follow, as it was pretty much written
> > with the mindset of "get it done now, fix it later". Looking into it
> > again, I see that part of the reason I abandoned it was not being able
> > to determine a good way to split things into logical commits. It's
> > almost entirely an "everything works or nothing works" change.
> 
> I haven't looked at your code yet, but the breakdown of patches I would
> expect / hope for is something like:
> 
>   1. introduce infrastructure for creating parse-tree from strbuf_expand
>      format, with some tests
> 
>   2. port format_commit_* over to new system; I would expect that the
>      caller code will have to be part of both the parsing and the
>      expansion, since the generic code can't know that "%ad" is
>      meaningful (and we want to keep it for backwards compatibility).
>      Leave format_commit_message as a parse + expand wrapper for simple
>      callers who don't care about speed.
> 
>   3. Add generic "%(key:option)" support to the new infrastructure,
>      forward-porting format_commit_* as necessary (and hopefully the
>      change are minimal...).
> 
> So those are all big commits, obviously,

Yeah, looks about right. It's mostly the "those commits will still be
pretty big" that I was concerned with. There's also the question of:
as my end-goal is conditional formatting, should these "smaller, but
still big" commits try to make sense independently, or, for example,
should I lay out a basic structure in the earlier commits, filled-out
with a relatively simple loop, and only later expand that into the
recursive function / parse-tree structure; or, should I start with the
"fancy" structures, even before they have a justification?

 - the "simple first" way sounds tempting, but it has the result of
   pretty much "inventing" in-between code which is never intended to
   actually be used. (even if it is intended to compile and work just 
   fine)
 - the "write it as it will be", however, is going to result in commits
   which may not make any sense one after another, and really only make
   sense in the end. I don't know if that's okay.

Neither of these options sound fun for bisecting, and yet it's such a
big change (in terms of "everyone uses log, so every user is effected")
that ease of bisectability seems like a very important consideration.

What I don't want to do is start the patch over from scratch, with only
the "long formats"/"unification with for-each-ref" in mind, only to
submit another patch following up later on that needs to completely
change the structures again to fit with the "parse tree" idea. Given
that the basic %(opt-color?...) test works, I expect that the current
state of the tree-structure is at least fairly close to what it should
be, though I also expect that someone with more experience writing
parsers may want to slap me for the way that structure is built.
Criticism is anticipated and appreciated.

>
> ...................................... but hopefully it lets us review
> in three stages: does the new infrastructure look good, does porting an
> existing caller (and probably the most complex caller) clean up the
> caller code, and then finally, does the new syntax look good?
> 
> But of course the devil is in the details, so probably that breakdown
> has some flaw in it. :) I'll see when I look at your code how close to
> reality I came.
> 
> -Peff

Er, good luck :)
As a side-note: It turns out that rebasing to the current "next" was not
too difficult. The result hasn't been pushed yet (I need to do a little
be of forensic work to make sure a behaviour I'm seeing isn't a
rebase-induced regression), but it does imply that at least pretty.c
should still make a fair amount of sense, even though it's about a year
old. Most of the problems I expected of the rebase, it turns out would
have been in sections which I hadn't actually done yet.

Re: Fwd: [PATCH 2/2] pretty.c: allow date formats in user format strings

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I think the whole thing needs to be factored into two phases: a
> parsing phase where we build some internal parse tree, and then an
> expansion phase where we walk the parse tree for each commit (or ref, or
> whatever is being expanded).

You are right.  I think for-each-ref expander has an attempt for
optimization of this exact kind.

>> Point is: we're going to keep having more and more format options,
>> I think that's a given. At some point, these short mnemonics will just
>> stop making sense, and it makes sense to have an escape plan when
>> that happens.
>
> Agreed. And I think it is possible to do it in a backwards-compatible
> way; support %(longname:options) for everything, and keep short-hands
> like %h and %ad for existing elements without options.

Yes, I think %( is not taken in the pretty-format language, so we should
be able to do this.

I wanted to take your earlier "'%ad' or '%ad(format)'" patch but refrained
from doing so.  The above line of reasoning is much better for the long
term health of the project.

Re: Fwd: [PATCH 2/2] pretty.c: allow date formats in user format strings

[Jeff King]

On Wed, Mar 09, 2011 at 01:06:17PM -0800, Junio C Hamano wrote:

> > Agreed. And I think it is possible to do it in a backwards-compatible
> > way; support %(longname:options) for everything, and keep short-hands
> > like %h and %ad for existing elements without options.
> 
> Yes, I think %( is not taken in the pretty-format language, so we should
> be able to do this.
> 
> I wanted to take your earlier "'%ad' or '%ad(format)'" patch but refrained
> from doing so.  The above line of reasoning is much better for the long
> term health of the project.

OK. Do you want me to throw away the %ad(format) patch for now, then, in
favor of building it on top of a more sane syntax?

I had originally planned to do %ad(format) for now, and then worry about
syntax later. Since we already have a variety of of other placeholders
with similar syntax (e.g., %w(), %C()). But I don't care too much either
way; it is not a feature I personally wanted, so delay doesn't bother
me. Dietmar (the original requestor) may feel differently, of course. :)

-Peff

Re: Fwd: [PATCH 2/2] pretty.c: allow date formats in user format strings

[Dietmar Winkler]

Den 10. mars 2011 23:31, skrev Jeff King:
> OK. Do you want me to throw away the %ad(format) patch for now, then, in
> favor of building it on top of a more sane syntax?
> 
> I had originally planned to do %ad(format) for now, and then worry about
> syntax later. Since we already have a variety of of other placeholders
> with similar syntax (e.g., %w(), %C()). But I don't care too much either
> way; it is not a feature I personally wanted, so delay doesn't bother
> me. Dietmar (the original requestor) may feel differently, of course. :)

As much as I would like to have such a feature (and a documentation that
is in synch with the implementation ;) I don't heavily rely on it. I'm
happy to wait a bit longer in favour of a more complete and clean
implementation which also opens up for other %(longname:options) support.
-- 
Dietmar.