[PATCH 0/2] Use %as and %cs as pretty format flags

[PATCH 0/2] Use %as and %cs as pretty format flags

[Nathan Panike]

The patch series below allows git to use %as and %cs as flags for
pretty-printing the format of a date.

Nathan W. Panike (2):
  Document %as and %cs as a pretty format flag
  Give %as and %cs meaning for pretty format

 Documentation/pretty-formats.txt |    2 ++
 pretty.c                         |    3 +++
 2 files changed, 5 insertions(+), 0 deletions(-)

Re: [PATCH 0/2] Use %as and %cs as pretty format flags

[Jeff King]

On Thu, Aug 28, 2008 at 06:09:37AM -0500, Nathan Panike wrote:

> The patch series below allows git to use %as and %cs as flags for
> pretty-printing the format of a date.

Your description leaves a little to be desired (here and in the patches
themselves). I had to read the patch to figure out that these are
formatting specifiers for the date format "short".

That being said, I think this is probably reasonable just for the sake
of completeness (and I doubt we are wasting a useful formatting combo,
since %a* is likely to remain dedicated to author information). I wonder
if there should be "%al" for "local".

However, it makes me wonder even more if '%ad' should simply respect the
--date= parameter (this wouldn't allow you to mix and match dates in a
single format, but I don't think that is what is desired). Or whether we
should have some syntax for "%ad(short)" or something, where the
argument would be handed off to the date format parser. But that is
probably overengineering.

-Peff

PS Junio, I think this will make your "git one" alias much shorter. :)

Re: [PATCH 0/2] Use %as and %cs as pretty format flags

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 28, 2008 at 06:09:37AM -0500, Nathan Panike wrote:
>
>> The patch series below allows git to use %as and %cs as flags for
>> pretty-printing the format of a date.
>
> Your description leaves a little to be desired (here and in the patches
> themselves). I had to read the patch to figure out that these are
> formatting specifiers for the date format "short".
>
> That being said, I think this is probably reasonable just for the sake
> of completeness (and I doubt we are wasting a useful formatting combo,
> since %a* is likely to remain dedicated to author information). I wonder
> if there should be "%al" for "local".
>
> However, it makes me wonder even more if '%ad' should simply respect the
> --date= parameter (this wouldn't allow you to mix and match dates in a
> single format, but I don't think that is what is desired). Or whether we
> should have some syntax for "%ad(short)" or something, where the
> argument would be handed off to the date format parser. But that is
> probably overengineering.

I was actually thinking about rejecting this, asking for something that
allows to express all the other %[ai][dDri] format can express, and
perhaps more.  So I think "%ad(short)" is a good direction to go, except
that 'd' is already taken.  Perhaps %a(date), %a(shortdate,local),...?

Oh, and before anybody asks, even if we do %a(specifier), you can keep
writing "%ad" if you are used to it.  I am not talking about deprecating
the existing ones, but making future extensions easier without forcing
people to remember cryptic one-letter format specifiers.

Re: [PATCH 0/2] Use %as and %cs as pretty format flags

[Jeff King]

On Thu, Aug 28, 2008 at 04:36:51PM -0700, Junio C Hamano wrote:

> I was actually thinking about rejecting this, asking for something that
> allows to express all the other %[ai][dDri] format can express, and
> perhaps more.  So I think "%ad(short)" is a good direction to go, except
> that 'd' is already taken.  Perhaps %a(date), %a(shortdate,local),...?

I was thinking we could accept %ad _or_ %ad(short), but of course
introducing the latter can break existing "%ad(my other random text)"
which is a bad idea.

I really think some consideration should be given to introducing
arbitrary "arguments" to formatting specifiers, of which this is one
example. Another that has been mentioned is pulling an arbitrary element
from a list.

How do you feel about a brand new syntax (and supporting the old, of
course) that is syntactically a little easier to extend. Like:

  %(macro, key=val, key=val)

e.g.

  %(authordate, format=short, tz=local)

where the syntax can be easily parsed without understanding what
"authordate" means.  Jakub already suggested something akin to RPM's
macro expansion, though I haven't looked too closely at it.

> Oh, and before anybody asks, even if we do %a(specifier), you can keep
> writing "%ad" if you are used to it.  I am not talking about deprecating
> the existing ones, but making future extensions easier without forcing
> people to remember cryptic one-letter format specifiers.

Yes, I don't see a need to get rid of the existing ones. Introducing a
new syntax does have the possibility of breaking existing scripts, since
we just leave unrecognized expansions in place (in fact, just adding %as
has the potential to break scripts!).

Worst case, we can introduce a --pretty=otherformat:, but I don't know
if people are really expecting "%a(blah)" to be left untouched
currently. I don't think we ever claimed that % was magical.

-Peff

Re: [PATCH 0/2] Use %as and %cs as pretty format flags

[Nathan W. Panike]

On Thu, Aug 28, 2008 at 6:54 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Aug 28, 2008 at 04:36:51PM -0700, Junio C Hamano wrote:
>
>> I was actually thinking about rejecting this, asking for something that
>> allows to express all the other %[ai][dDri] format can express, and
>> perhaps more.  So I think "%ad(short)" is a good direction to go, except
>> that 'd' is already taken.  Perhaps %a(date), %a(shortdate,local),...?
>
> I was thinking we could accept %ad _or_ %ad(short), but of course
> introducing the latter can break existing "%ad(my other random text)"
> which is a bad idea.
>
> I really think some consideration should be given to introducing
> arbitrary "arguments" to formatting specifiers, of which this is one
> example. Another that has been mentioned is pulling an arbitrary element
> from a list.
>
> How do you feel about a brand new syntax (and supporting the old, of
> course) that is syntactically a little easier to extend. Like:
>
>  %(macro, key=val, key=val)
>
> e.g.
>
>  %(authordate, format=short, tz=local)

The genesis of this patch was the documentation says that %ad
represents 'author date' without any more specific information.  I
thought that I could do

git show --date=short --pretty=format:"%ad %h"

and get the date in short form---but alas, this did not work.  So I
wrote up the patch to get a short date format.  Your ideas are much
more powerful.  I like them.

Thanks for git.

Nathan Panike

Re: [PATCH 0/2] Use %as and %cs as pretty format flags

[Jeff King]

On Thu, Aug 28, 2008 at 07:10:45PM -0500, Nathan W. Panike wrote:

> The genesis of this patch was the documentation says that %ad
> represents 'author date' without any more specific information.  I
> thought that I could do
> 
> git show --date=short --pretty=format:"%ad %h"
> 
> and get the date in short form---but alas, this did not work.  So I

Regardless of expansions to --pretty=format:, that is something we
should probably be doing _anyway_. The current behavior is just
confusing, and %ad is simply documented as "date", not in any particular
format (and I think anyone relying on it _ignoring_ --date= is insane,
since that --date is otherwise not doing anything).

So how about this?

-- >8 --
pretty=format: respect date format options

When running a command like:

  git log --pretty=format:%ad --date=short

the date option was ignored. This patch causes it to use
whatever format was specified by --date (or by
--relative-date, etc), just as the non-user formats would
do.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/pretty-formats.txt |    2 +-
 archive.c                        |    2 +-
 builtin-commit.c                 |    2 +-
 commit.h                         |    3 ++-
 pretty.c                         |   17 +++++++++++------
 t/t6006-rev-list-format.sh       |    6 ++++++
 6 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index c11d495..388d492 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -103,7 +103,7 @@ The placeholders are:
 - '%an': author name
 - '%aN': author name (respecting .mailmap)
 - '%ae': author email
-- '%ad': author date
+- '%ad': author date (format respects --date= option)
 - '%aD': author date, RFC2822 style
 - '%ar': author date, relative
 - '%at': author date, UNIX timestamp
diff --git a/archive.c b/archive.c
index 5b40e26..e2280df 100644
--- a/archive.c
+++ b/archive.c
@@ -48,7 +48,7 @@ static void format_subst(const struct commit *commit,
 		strbuf_add(&fmt, b + 8, c - b - 8);
 
 		strbuf_add(buf, src, b - src);
-		format_commit_message(commit, fmt.buf, buf);
+		format_commit_message(commit, fmt.buf, buf, DATE_NORMAL);
 		len -= c + 1 - src;
 		src  = c + 1;
 	}
diff --git a/builtin-commit.c b/builtin-commit.c
index 649c8be..c870037 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -882,7 +882,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 
 	if (!log_tree_commit(&rev, commit)) {
 		struct strbuf buf = STRBUF_INIT;
-		format_commit_message(commit, "%h: %s", &buf);
+		format_commit_message(commit, "%h: %s", &buf, DATE_NORMAL);
 		printf("%s\n", buf.buf);
 		strbuf_release(&buf);
 	}
diff --git a/commit.h b/commit.h
index d163c74..de15f4d 100644
--- a/commit.h
+++ b/commit.h
@@ -67,7 +67,8 @@ extern int non_ascii(int);
 struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
 extern void get_commit_format(const char *arg, struct rev_info *);
 extern void format_commit_message(const struct commit *commit,
-                                  const void *format, struct strbuf *sb);
+				  const void *format, struct strbuf *sb,
+				  enum date_mode dmode);
 extern void pretty_print_commit(enum cmit_fmt fmt, const struct commit*,
                                 struct strbuf *,
                                 int abbrev, const char *subject,
diff --git a/pretty.c b/pretty.c
index 33ef34a..a29c290 100644
--- a/pretty.c
+++ b/pretty.c
@@ -310,7 +310,7 @@ static int mailmap_name(struct strbuf *sb, const char *email)
 }
 
 static size_t format_person_part(struct strbuf *sb, char part,
-                               const char *msg, int len)
+				 const char *msg, int len, enum date_mode dmode)
 {
 	/* currently all placeholders have same length */
 	const int placeholder_len = 2;
@@ -377,7 +377,7 @@ static size_t format_person_part(struct strbuf *sb, char part,
 
 	switch (part) {
 	case 'd':	/* date */
-		strbuf_addstr(sb, show_date(date, tz, DATE_NORMAL));
+		strbuf_addstr(sb, show_date(date, tz, dmode));
 		return placeholder_len;
 	case 'D':	/* date, RFC2822 style */
 		strbuf_addstr(sb, show_date(date, tz, DATE_RFC2822));
@@ -409,6 +409,7 @@ struct chunk {
 
 struct format_commit_context {
 	const struct commit *commit;
+	enum date_mode dmode;
 
 	/* These offsets are relative to the start of the commit message. */
 	int commit_header_parsed;
@@ -584,10 +585,12 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 		return 1;
 	case 'a':	/* author ... */
 		return format_person_part(sb, placeholder[1],
-		                   msg + c->author.off, c->author.len);
+				   msg + c->author.off, c->author.len,
+				   c->dmode);
 	case 'c':	/* committer ... */
 		return format_person_part(sb, placeholder[1],
-		                   msg + c->committer.off, c->committer.len);
+				   msg + c->committer.off, c->committer.len,
+				   c->dmode);
 	case 'e':	/* encoding */
 		strbuf_add(sb, msg + c->encoding.off, c->encoding.len);
 		return 1;
@@ -599,12 +602,14 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 }
 
 void format_commit_message(const struct commit *commit,
-                           const void *format, struct strbuf *sb)
+			   const void *format, struct strbuf *sb,
+			   enum date_mode dmode)
 {
 	struct format_commit_context context;
 
 	memset(&context, 0, sizeof(context));
 	context.commit = commit;
+	context.dmode = dmode;
 	strbuf_expand(sb, format, format_commit_item, &context);
 }
 
@@ -770,7 +775,7 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 	const char *encoding;
 
 	if (fmt == CMIT_FMT_USERFORMAT) {
-		format_commit_message(commit, user_format, sb);
+		format_commit_message(commit, user_format, sb, dmode);
 		return;
 	}
 
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 9176484..485ad4d 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -139,6 +139,12 @@ commit 131a310eb913d107dd3c09a65d1651175898735d
 commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
 EOF
 
+test_expect_success '%ad respects --date=' '
+	echo 2005-04-07 >expect.ad-short &&
+	git log -1 --date=short --pretty=tformat:%ad >output.ad-short master &&
+	test_cmp expect.ad-short output.ad-short
+'
+
 test_expect_success 'empty email' '
 	test_tick &&
 	C=$(GIT_AUTHOR_EMAIL= git commit-tree HEAD^{tree} </dev/null) &&
-- 
1.6.0.1.173.g2bf39.dirty

Re: [PATCH 0/2] Use %as and %cs as pretty format flags

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 28, 2008 at 07:10:45PM -0500, Nathan W. Panike wrote:
>
>> The genesis of this patch was the documentation says that %ad
>> represents 'author date' without any more specific information.  I
>> thought that I could do
>> 
>> git show --date=short --pretty=format:"%ad %h"
>> 
>> and get the date in short form---but alas, this did not work.  So I
>
> Regardless of expansions to --pretty=format:, that is something we
> should probably be doing _anyway_. The current behavior is just
> confusing, and %ad is simply documented as "date", not in any particular
> format (and I think anyone relying on it _ignoring_ --date= is insane,
> since that --date is otherwise not doing anything).
>
> So how about this?
>
> -- >8 --
> pretty=format: respect date format options
>
> When running a command like:
>
>   git log --pretty=format:%ad --date=short
>
> the date option was ignored. This patch causes it to use
> whatever format was specified by --date (or by
> --relative-date, etc), just as the non-user formats would
> do.
>
> Signed-off-by: Jeff King <peff@peff.net>

Like the idea; haven't looked at the patch yet, though.

Re: [PATCH 0/2] Use %as and %cs as pretty format flags

[Avery Pennarun]

On Thu, Aug 28, 2008 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I was actually thinking about rejecting this, asking for something that
> allows to express all the other %[ai][dDri] format can express, and
> perhaps more.  So I think "%ad(short)" is a good direction to go, except
> that 'd' is already taken.  Perhaps %a(date), %a(shortdate,local),...?

These format strings are getting increasingly complicated without
really gaining much in generality.  Why not
"%ad(strftime-compatible-format-string)" ?  You could differentiate
localtime vs. gmtime with %ad vs. %aD, or something like that.

Have fun,

Avery

Re: [PATCH 0/2] Use %as and %cs as pretty format flags

[Jakub Narebski]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 28, 2008 at 04:36:51PM -0700, Junio C Hamano wrote:
> 
> > I was actually thinking about rejecting this, asking for something that
> > allows to express all the other %[ai][dDri] format can express, and
> > perhaps more.  So I think "%ad(short)" is a good direction to go, except
> > that 'd' is already taken.  Perhaps %a(date), %a(shortdate,local),...?
> 
> I was thinking we could accept %ad _or_ %ad(short), but of course
> introducing the latter can break existing "%ad(my other random text)"
> which is a bad idea.
> 
> I really think some consideration should be given to introducing
> arbitrary "arguments" to formatting specifiers, of which this is one
> example. Another that has been mentioned is pulling an arbitrary element
> from a list.
> 
> How do you feel about a brand new syntax (and supporting the old, of
> course) that is syntactically a little easier to extend. Like:
> 
>   %(macro, key=val, key=val)
> 
> e.g.
> 
>   %(authordate, format=short, tz=local)
> 
> where the syntax can be easily parsed without understanding what
> "authordate" means.  Jakub already suggested something akin to RPM's
> macro expansion, though I haven't looked too closely at it.

I'd rather we do not introduce yet another pretty-printing /
formatting syntax, and use the same syntax as git-for-each-ref uses
for extra formatting, 

  As a special case for the date-type fields, you may specify a format for
  the date by adding one of `:default`, `:relative`, `:short`, `:local`,
  `:iso8601` or `:rfc2822` to the end of the fieldname; e.g.
  `%(taggerdate:relative)`.

which incindentally (or not) is the same as rpm uses

  Alternate output formats may be requested by following the tag with  :type-
  tag.  Currently, the following types are supported:

  :armor  Wrap a public key in ASCII armor.

  :base64
         Encode binary data using base64.

  :date  Use strftime(3) "%c" format.

  :day   Use strftime(3) "%a %b %d %Y" format.

  :depflags
         Format dependency flags.

  :fflags
         Format file flags.

  :hex   Format in hexadecimal.

  :octal Format in octal.

  :perms Format file permissions.

  :shescape
         Escape single quotes for use in a script.

  :triggertype
         Display trigger suffix.

P.S. I Agree with %ad and the like respecting --date=<type>
-- 
Jakub Narebski
Poland
ShadeHawk on #git