git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] pretty: format aliases
@ 2010-04-25 15:42 Will Palmer
  2010-04-25 15:42 ` [PATCH 1/3] pretty: add conditional %C?colorname placeholders Will Palmer
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Will Palmer @ 2010-04-25 15:42 UTC (permalink / raw)
  To: git; +Cc: wmpalmer, gitster

The following patch series adds the ability to configure aliases for
user-defined formats. The first two patches define new placeholders and
modify the output of existing placeholders to allow aliases to be more
consistent with the way builtin formats are handled. The final patch
adds support for the aliases themselves.

There were a couple of places where I wasn't entirely sure about which
color setting I should be following, but I've tried to be consistent
throughout. It may be that I could have simply followed diffopt's color
option in all cases, in which case various modifications to show_log()
were entirely unnecessary. I'll await judgement at the hands of one who
groks those sections more than I do, but I think what I've done feels
correct.

My original goal was to make it possible to define all of the builtin
formats as builtin aliases to format strings, but complications
regarding how --parents and --decorate would be handled require further
thought and discussion. For example, we could simply make
"--format=%H --decorate" synonymous with "--format=%H%d", but I'm not
sure if that feels clean enough.

For now, I think this is at a point where its good-enough to submit, if
only as a starting point for some discussion as to where to head next.

Will Palmer (3):
  pretty: add conditional %C?colorname placeholders
  pretty: make %H/%h dependent on --abbrev[-commit]
  pretty: add aliases for pretty formats

 Documentation/config.txt         |    8 ++
 Documentation/pretty-formats.txt |    1 +
 builtin/log.c                    |    2 +-
 builtin/rev-list.c               |    2 +
 builtin/shortlog.c               |    7 +-
 builtin/show-branch.c            |    1 +
 combine-diff.c                   |   11 +-
 commit.h                         |    2 +
 log-tree.c                       |   11 ++-
 log-tree.h                       |    2 +-
 pretty.c                         |  248 ++++++++++++++++++++++++++++++--------
 shortlog.h                       |    2 +-
 t/t4205-log-pretty-formats.sh    |   87 +++++++++++++
 13 files changed, 321 insertions(+), 63 deletions(-)
 create mode 100755 t/t4205-log-pretty-formats.sh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/3] pretty: add conditional %C?colorname placeholders
  2010-04-25 15:42 [PATCH 0/3] pretty: format aliases Will Palmer
@ 2010-04-25 15:42 ` Will Palmer
  2010-04-26  6:30   ` Alex Riesen
  2010-04-25 15:42 ` [PATCH 2/3] pretty: make %H/%h dependent on --abbrev[-commit] Will Palmer
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Will Palmer @ 2010-04-25 15:42 UTC (permalink / raw)
  To: git; +Cc: wmpalmer, gitster

Many commands are able to colorize, or not, depending on a user's
configuration and whether output is being sent to a terminal. However,
if a user explicitly specifies a --pretty format, color will always be
output, regardless of the destination. This would generally be okay, in
a "do what I tell you, whether or not I should tell you to" sense, but
the assumption fell apart when an alias was defined which may be run in
various contexts: there was no way to specify "use this color, but only
if you normally would display color at all"

Here we add the %C?colorname placeholders which act just as the
%Ccolorname placeholders, with the exception that the pretty_context is
checked to see if color should be used according to configuration

Signed-off-by: Will Palmer <wmpalmer@gmail.com>
---
 Documentation/pretty-formats.txt |    1 +
 builtin/log.c                    |    2 +-
 builtin/rev-list.c               |    1 +
 builtin/shortlog.c               |    5 ++-
 builtin/show-branch.c            |    1 +
 combine-diff.c                   |   11 +++++---
 commit.h                         |    1 +
 log-tree.c                       |    9 ++++--
 log-tree.h                       |    2 +-
 pretty.c                         |   49 ++++++++++++++++++++++++-------------
 shortlog.h                       |    2 +-
 t/t4205-log-pretty-formats.sh    |   44 ++++++++++++++++++++++++++++++++++
 12 files changed, 99 insertions(+), 29 deletions(-)
 create mode 100755 t/t4205-log-pretty-formats.sh

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 1686a54..53eb903 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -132,6 +132,7 @@ The placeholders are:
 - '%Cblue': switch color to blue
 - '%Creset': reset color
 - '%C(...)': color specification, as described in color.branch.* config option
+- '%C?...: switch to specified color, if relevant color.* config option specifies that color is ok
 - '%m': left, right or boundary mark
 - '%n': newline
 - '%%': a raw '%'
diff --git a/builtin/log.c b/builtin/log.c
index 6208703..a9fc6ea 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -740,7 +740,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	log.in1 = 2;
 	log.in2 = 4;
 	for (i = 0; i < nr; i++)
-		shortlog_add_commit(&log, list[i]);
+		shortlog_add_commit(&log, list[i], rev);
 
 	shortlog_output(&log);
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 51ceb19..5a53862 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -99,6 +99,7 @@ static void show_commit(struct commit *commit, void *data)
 		struct pretty_print_context ctx = {0};
 		ctx.abbrev = revs->abbrev;
 		ctx.date_mode = revs->date_mode;
+		ctx.use_color = DIFF_OPT_TST(&revs->diffopt, COLOR_DIFF);
 		pretty_print_commit(revs->commit_format, commit, &buf, &ctx);
 		if (revs->graph) {
 			if (buf.len) {
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 06320f5..7aee491 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -136,13 +136,14 @@ static void read_from_stdin(struct shortlog *log)
 	}
 }
 
-void shortlog_add_commit(struct shortlog *log, struct commit *commit)
+void shortlog_add_commit(struct shortlog *log, struct commit *commit, struct rev_info *rev)
 {
 	const char *author = NULL, *buffer;
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf ufbuf = STRBUF_INIT;
 	struct pretty_print_context ctx = {0};
 
+	ctx.use_color = DIFF_OPT_TST(&rev->diffopt, COLOR_DIFF);
 	pretty_print_commit(CMIT_FMT_RAW, commit, &buf, &ctx);
 	buffer = buf.buf;
 	while (*buffer && *buffer != '\n') {
@@ -183,7 +184,7 @@ static void get_from_rev(struct rev_info *rev, struct shortlog *log)
 	if (prepare_revision_walk(rev))
 		die("revision walk setup failed");
 	while ((commit = get_revision(rev)) != NULL)
-		shortlog_add_commit(log, commit);
+		shortlog_add_commit(log, commit, rev);
 }
 
 static int parse_uint(char const **arg, int comma, int defval)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index e20fcf3..e324336 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -294,6 +294,7 @@ static void show_one_commit(struct commit *commit, int no_name)
 
 	if (commit->object.parsed) {
 		struct pretty_print_context ctx = {0};
+		ctx.use_color = showbranch_use_color;
 		pretty_print_commit(CMIT_FMT_ONELINE, commit, &pretty, &ctx);
 		pretty_str = pretty.buf;
 	}
diff --git a/combine-diff.c b/combine-diff.c
index 3480dae..5157c62 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -841,7 +841,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 		int deleted = 0;
 
 		if (rev->loginfo && !rev->no_commit_id)
-			show_log(rev);
+			show_log(rev, use_color);
 		dump_quoted_path(dense ? "diff --cc " : "diff --combined ",
 				 "", elem->path, c_meta, c_reset);
 		printf("%sindex ", c_meta);
@@ -923,8 +923,10 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 	if (!line_termination)
 		inter_name_termination = 0;
 
-	if (rev->loginfo && !rev->no_commit_id)
-		show_log(rev);
+	if (rev->loginfo && !rev->no_commit_id) {
+		int use_color = DIFF_OPT_TST(opt, COLOR_DIFF);
+		show_log(rev, use_color);
+	}
 
 	if (opt->output_format & DIFF_FORMAT_RAW) {
 		offset = strlen(COLONS) - num_parent;
@@ -1005,7 +1007,8 @@ void diff_tree_combined(const unsigned char *sha1,
 		paths = intersect_paths(paths, i, num_parent);
 
 		if (show_log_first && i == 0) {
-			show_log(rev);
+			int use_color = DIFF_OPT_TST(opt, COLOR_DIFF);
+			show_log(rev, use_color);
 			if (rev->verbose_header && opt->output_format)
 				putchar(opt->line_termination);
 		}
diff --git a/commit.h b/commit.h
index 26ec8c0..b6caf91 100644
--- a/commit.h
+++ b/commit.h
@@ -71,6 +71,7 @@ struct pretty_print_context
 	enum date_mode date_mode;
 	int need_8bit_cte;
 	int show_notes;
+	int use_color;
 	struct reflog_walk_info *reflog_info;
 };
 
diff --git a/log-tree.c b/log-tree.c
index d3ae969..b743c43 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -274,7 +274,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 	*extra_headers_p = extra_headers;
 }
 
-void show_log(struct rev_info *opt)
+void show_log(struct rev_info *opt, int use_color)
 {
 	struct strbuf msgbuf = STRBUF_INIT;
 	struct log_info *log = opt->loginfo;
@@ -282,6 +282,7 @@ void show_log(struct rev_info *opt)
 	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
 	const char *extra_headers = opt->extra_headers;
 	struct pretty_print_context ctx = {0};
+	ctx.use_color = use_color;
 
 	opt->loginfo = NULL;
 	ctx.show_notes = opt->show_notes;
@@ -457,12 +458,13 @@ int log_tree_diff_flush(struct rev_info *opt)
 	}
 
 	if (opt->loginfo && !opt->no_commit_id) {
+		int use_color = DIFF_OPT_TST(&opt->diffopt, COLOR_DIFF);
 		/* When showing a verbose header (i.e. log message),
 		 * and not in --pretty=oneline format, we would want
 		 * an extra newline between the end of log and the
 		 * output for readability.
 		 */
-		show_log(opt);
+		show_log(opt, use_color);
 		if ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&
 		    opt->verbose_header &&
 		    opt->commit_format != CMIT_FMT_ONELINE) {
@@ -559,8 +561,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 
 	shown = log_tree_diff(opt, commit, &log);
 	if (!shown && opt->loginfo && opt->always_show_header) {
+		int use_color = DIFF_OPT_TST(&opt->diffopt, COLOR_DIFF);
 		log.parent = NULL;
-		show_log(opt);
+		show_log(opt, use_color);
 		shown = 1;
 	}
 	opt->loginfo = NULL;
diff --git a/log-tree.h b/log-tree.h
index 3f7b400..09f970e 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -11,7 +11,7 @@ void init_log_tree_opt(struct rev_info *);
 int log_tree_diff_flush(struct rev_info *);
 int log_tree_commit(struct rev_info *, struct commit *);
 int log_tree_opt_parse(struct rev_info *, const char **, int);
-void show_log(struct rev_info *opt);
+void show_log(struct rev_info *opt, int use_color);
 void show_decorations(struct rev_info *opt, struct commit *commit);
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     const char **subject_p,
diff --git a/pretty.c b/pretty.c
index 7cb3a2a..fdb5e16 100644
--- a/pretty.c
+++ b/pretty.c
@@ -634,35 +634,50 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 	struct format_commit_context *c = context;
 	const struct commit *commit = c->commit;
 	const char *msg = commit->buffer;
+	const char *color_start;
 	struct commit_list *p;
+	int use_color = c->pretty_ctx->use_color;
+	int conditional_color = 0;
 	int h1, h2;
 
 	/* these are independent of the commit */
 	switch (placeholder[0]) {
 	case 'C':
-		if (placeholder[1] == '(') {
+		color_start = placeholder;
+		if (placeholder[1] == '?') {
+			color_start++;
+			conditional_color = 1;
+		}
+		if (color_start[1] == '(') {
 			const char *end = strchr(placeholder + 2, ')');
 			char color[COLOR_MAXLEN];
 			if (!end)
 				return 0;
-			color_parse_mem(placeholder + 2,
-					end - (placeholder + 2),
-					"--pretty format", color);
-			strbuf_addstr(sb, color);
+			if ( !conditional_color || use_color ) {
+				color_parse_mem(color_start + 2,
+						end - (color_start + 2),
+						"--pretty format", color);
+				strbuf_addstr(sb, color);
+			}
 			return end - placeholder + 1;
 		}
-		if (!prefixcmp(placeholder + 1, "red")) {
-			strbuf_addstr(sb, GIT_COLOR_RED);
-			return 4;
-		} else if (!prefixcmp(placeholder + 1, "green")) {
-			strbuf_addstr(sb, GIT_COLOR_GREEN);
-			return 6;
-		} else if (!prefixcmp(placeholder + 1, "blue")) {
-			strbuf_addstr(sb, GIT_COLOR_BLUE);
-			return 5;
-		} else if (!prefixcmp(placeholder + 1, "reset")) {
-			strbuf_addstr(sb, GIT_COLOR_RESET);
-			return 6;
+
+		if (!prefixcmp(color_start + 1, "red")) {
+			if ( !conditional_color || use_color )
+				strbuf_addstr(sb, GIT_COLOR_RED);
+			return 4 + (conditional_color ? 1 : 0);
+		} else if (!prefixcmp(color_start + 1, "green")) {
+			if ( !conditional_color || use_color )
+				strbuf_addstr(sb, GIT_COLOR_GREEN);
+			return 6 + (conditional_color ? 1 : 0);
+		} else if (!prefixcmp(color_start + 1, "blue")) {
+			if ( !conditional_color || use_color )
+				strbuf_addstr(sb, GIT_COLOR_BLUE);
+			return 5 + (conditional_color ? 1 : 0);
+		} else if (!prefixcmp(color_start + 1, "reset")) {
+			if ( !conditional_color || use_color )
+				strbuf_addstr(sb, GIT_COLOR_RESET);
+			return 6 + (conditional_color ? 1 : 0);
 		} else
 			return 0;
 	case 'n':		/* newline */
diff --git a/shortlog.h b/shortlog.h
index bc02cc2..43498a0 100644
--- a/shortlog.h
+++ b/shortlog.h
@@ -20,7 +20,7 @@ struct shortlog {
 
 void shortlog_init(struct shortlog *log);
 
-void shortlog_add_commit(struct shortlog *log, struct commit *commit);
+void shortlog_add_commit(struct shortlog *log, struct commit *commit, struct rev_info *rev);
 
 void shortlog_output(struct shortlog *log);
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
new file mode 100755
index 0000000..28d2948
--- /dev/null
+++ b/t/t4205-log-pretty-formats.sh
@@ -0,0 +1,44 @@
+#!/bin/sh
+#
+# Released into Public Domain by Will Palmer 2010
+#
+
+test_description='Test pretty formats'
+. ./test-lib.sh
+
+test_expect_success "set up basic repos" ">foo && git add foo && git commit -m initial"
+
+for flag in false true always; do
+for color in red green blue reset; do
+
+	make_expected="git config --get-color no.such.slot $color >expected"
+	test_expect_success "%C$color with color.ui $flag" \
+		"$make_expected &&
+		git config color.ui $flag &&
+		git log -1 --pretty=format:'%C$color' > actual &&
+		cmp expected actual"
+
+
+	test_expect_success "%C($color) with color.ui $flag" \
+		"$make_expected &&
+		git config color.ui $flag &&
+		git log -1 --pretty=format:'%C($color)' > actual &&
+		cmp expected actual"
+
+	[ ! "$flag" = "always" ] && make_expected=">expected"
+	test_expect_success "%C?$color with color.ui $flag" \
+		"$make_expected &&
+		git config color.ui $flag &&
+		git log -1 --pretty=format:'%C?$color' > actual &&
+		cmp expected actual"
+
+	test_expect_success "%C?($color) with color.ui $flag" \
+		"$make_expected &&
+		git config color.ui $flag &&
+		git log -1 --pretty=format:'%C?($color)' > actual &&
+		cmp expected actual"
+
+done
+done
+
+test_done
-- 
1.7.1.rc1.13.gbb0a0a.dirty

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/3] pretty: make %H/%h dependent on --abbrev[-commit]
  2010-04-25 15:42 [PATCH 0/3] pretty: format aliases Will Palmer
  2010-04-25 15:42 ` [PATCH 1/3] pretty: add conditional %C?colorname placeholders Will Palmer
@ 2010-04-25 15:42 ` Will Palmer
  2010-04-25 15:42 ` [PATCH 3/3] pretty: add aliases for pretty formats Will Palmer
  2010-04-25 19:48 ` [PATCH 0/3] pretty: format aliases Jeff King
  3 siblings, 0 replies; 16+ messages in thread
From: Will Palmer @ 2010-04-25 15:42 UTC (permalink / raw)
  To: git; +Cc: wmpalmer, gitster

Prior to this, the output of %H was always 40 characters long, and the
output of %h always DEFAULT_ABBREV characters long, without regard to
whether --abbrev-commit or --abbrev had been passed.

Here we make "git log --pretty=%H --abbrev-commit" synonymous with
"git log --pretty=%h", and make %h/abbreviated-%H respect the length
specified for --abbrev.

The same is applied to other commit-placeholders %P and %p, and
--abbrev is respected for %t, though %T is not changed.

Signed-off-by: Will Palmer <wmpalmer@gmail.com>
---
 builtin/rev-list.c |    1 +
 builtin/shortlog.c |    2 ++
 commit.h           |    1 +
 log-tree.c         |    2 ++
 pretty.c           |   30 +++++++++++++++++++-----------
 5 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 5a53862..1d1e59c 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -98,6 +98,7 @@ static void show_commit(struct commit *commit, void *data)
 		struct strbuf buf = STRBUF_INIT;
 		struct pretty_print_context ctx = {0};
 		ctx.abbrev = revs->abbrev;
+		ctx.abbrev_commit = revs->abbrev_commit;
 		ctx.date_mode = revs->date_mode;
 		ctx.use_color = DIFF_OPT_TST(&revs->diffopt, COLOR_DIFF);
 		pretty_print_commit(revs->commit_format, commit, &buf, &ctx);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 7aee491..5c0721c 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -143,6 +143,8 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit, struct rev
 	struct strbuf ufbuf = STRBUF_INIT;
 	struct pretty_print_context ctx = {0};
 
+	ctx.abbrev = rev->abbrev;
+	ctx.abbrev_commit = rev->abbrev_commit;
 	ctx.use_color = DIFF_OPT_TST(&rev->diffopt, COLOR_DIFF);
 	pretty_print_commit(CMIT_FMT_RAW, commit, &buf, &ctx);
 	buffer = buf.buf;
diff --git a/commit.h b/commit.h
index b6caf91..7a476a0 100644
--- a/commit.h
+++ b/commit.h
@@ -72,6 +72,7 @@ struct pretty_print_context
 	int need_8bit_cte;
 	int show_notes;
 	int use_color;
+	int abbrev_commit;
 	struct reflog_walk_info *reflog_info;
 };
 
diff --git a/log-tree.c b/log-tree.c
index b743c43..9bd4f47 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -282,6 +282,8 @@ void show_log(struct rev_info *opt, int use_color)
 	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
 	const char *extra_headers = opt->extra_headers;
 	struct pretty_print_context ctx = {0};
+	ctx.abbrev = opt->abbrev;
+	ctx.abbrev_commit = opt->abbrev_commit;
 	ctx.use_color = use_color;
 
 	opt->loginfo = NULL;
diff --git a/pretty.c b/pretty.c
index fdb5e16..f884f48 100644
--- a/pretty.c
+++ b/pretty.c
@@ -725,13 +725,16 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 
 	switch (placeholder[0]) {
 	case 'H':		/* commit hash */
-		strbuf_addstr(sb, sha1_to_hex(commit->object.sha1));
-		return 1;
 	case 'h':		/* abbreviated commit hash */
+		if (placeholder[0] != 'h' && !c->pretty_ctx->abbrev_commit) {
+			strbuf_addstr(sb, sha1_to_hex(commit->object.sha1));
+			return 1;
+		}
+
 		if (add_again(sb, &c->abbrev_commit_hash))
 			return 1;
 		strbuf_addstr(sb, find_unique_abbrev(commit->object.sha1,
-		                                     DEFAULT_ABBREV));
+						     c->pretty_ctx->abbrev));
 		c->abbrev_commit_hash.len = sb->len - c->abbrev_commit_hash.off;
 		return 1;
 	case 'T':		/* tree hash */
@@ -741,24 +744,29 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		if (add_again(sb, &c->abbrev_tree_hash))
 			return 1;
 		strbuf_addstr(sb, find_unique_abbrev(commit->tree->object.sha1,
-		                                     DEFAULT_ABBREV));
+						     c->pretty_ctx->abbrev));
 		c->abbrev_tree_hash.len = sb->len - c->abbrev_tree_hash.off;
 		return 1;
 	case 'P':		/* parent hashes */
-		for (p = commit->parents; p; p = p->next) {
-			if (p != commit->parents)
-				strbuf_addch(sb, ' ');
-			strbuf_addstr(sb, sha1_to_hex(p->item->object.sha1));
-		}
-		return 1;
 	case 'p':		/* abbreviated parent hashes */
+		if (placeholder[0] != 'p' && !c->pretty_ctx->abbrev_commit) {
+			for (p = commit->parents; p; p = p->next) {
+				if (p != commit->parents)
+					strbuf_addch(sb, ' ');
+				strbuf_addstr(sb,
+					      sha1_to_hex(p->item->object.sha1));
+			}
+			return 1;
+		}
+
 		if (add_again(sb, &c->abbrev_parent_hashes))
 			return 1;
 		for (p = commit->parents; p; p = p->next) {
 			if (p != commit->parents)
 				strbuf_addch(sb, ' ');
 			strbuf_addstr(sb, find_unique_abbrev(
-					p->item->object.sha1, DEFAULT_ABBREV));
+				      p->item->object.sha1,
+				      c->pretty_ctx->abbrev));
 		}
 		c->abbrev_parent_hashes.len = sb->len -
 		                              c->abbrev_parent_hashes.off;
-- 
1.7.1.rc1.13.gbb0a0a.dirty

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/3] pretty: add aliases for pretty formats
  2010-04-25 15:42 [PATCH 0/3] pretty: format aliases Will Palmer
  2010-04-25 15:42 ` [PATCH 1/3] pretty: add conditional %C?colorname placeholders Will Palmer
  2010-04-25 15:42 ` [PATCH 2/3] pretty: make %H/%h dependent on --abbrev[-commit] Will Palmer
@ 2010-04-25 15:42 ` Will Palmer
  2010-04-25 19:48 ` [PATCH 0/3] pretty: format aliases Jeff King
  3 siblings, 0 replies; 16+ messages in thread
From: Will Palmer @ 2010-04-25 15:42 UTC (permalink / raw)
  To: git; +Cc: wmpalmer, gitster

previously the only ways to alias a --pretty format within git were
either to set the format as your default format (via the format.pretty
configuration variable), or by using a regular git alias. This left the
definition of more complicated formats to the realm of "builtin or
nothing", with user-defined formats usually being reserved for quick
one-offs.

Here we allow user-defined formats to enjoy more or less the same
benefits of builtins. By defining format.pretty.myalias, "myalias" can
be used in place of whatever would normally come after --pretty=. This
can be a format:, tformat:, raw (ie, defaulting to tformat), or the name
of another builtin or user-defined pretty format.

Signed-off-by: Will Palmer <wmpalmer@gmail.com>
---
 Documentation/config.txt      |    8 ++
 pretty.c                      |  169 +++++++++++++++++++++++++++++++++++------
 t/t4205-log-pretty-formats.sh |   53 ++++++++++++-
 3 files changed, 202 insertions(+), 28 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 92f851e..6573d18 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -894,6 +894,14 @@ format.pretty::
 	See linkgit:git-log[1], linkgit:git-show[1],
 	linkgit:git-whatchanged[1].
 
+format.pretty.<name>::
+	Alias for a --pretty= format string, as specified in
+	linkgit:git-log[1]. Any aliases defined here can be used just
+	as the builtin pretty formats could. For example, defining
+	"format.pretty.hash = format:%H" would cause the invocation
+	"git log --pretty=hash" to be equivalent to running
+	"git log --pretty=format:%H".
+
 format.thread::
 	The default threading style for 'git format-patch'.  Can be
 	a boolean value, or `shallow` or `deep`.  `shallow` threading
diff --git a/pretty.c b/pretty.c
index f884f48..d49fec7 100644
--- a/pretty.c
+++ b/pretty.c
@@ -11,6 +11,16 @@
 #include "reflog-walk.h"
 
 static char *user_format;
+static struct cmt_fmt_map {
+	const char *name;
+	enum cmit_fmt format;
+	const char *user_format;
+	int is_tformat;
+	int is_alias;
+} *commit_formats = NULL;
+static size_t commit_formats_len = 0;
+static size_t commit_formats_alloc = 0;
+static struct cmt_fmt_map *find_commit_format(const char *sought);
 
 static void save_user_format(struct rev_info *rev, const char *cp, int is_tformat)
 {
@@ -21,22 +31,134 @@ static void save_user_format(struct rev_info *rev, const char *cp, int is_tforma
 	rev->commit_format = CMIT_FMT_USERFORMAT;
 }
 
-void get_commit_format(const char *arg, struct rev_info *rev)
+static int git_pretty_formats_config(const char *var, const char *value, void *cb)
+{
+	if (!prefixcmp(var, "format.pretty.")) {
+		struct cmt_fmt_map user_format = {0};
+		const char *fmt;
+
+		user_format.name = xstrdup(&var[14]);
+		user_format.format = CMIT_FMT_USERFORMAT;
+		git_config_string(&fmt, var, value);
+		if (!prefixcmp(fmt, "format:") || !prefixcmp(fmt, "tformat:")) {
+			user_format.is_tformat = fmt[0] == 't';
+			fmt = strchr(fmt, ':') + 1;
+		} else if (strchr(fmt, '%'))
+			user_format.is_tformat = 1;
+		else
+			user_format.is_alias = 1;
+		user_format.user_format = fmt;
+
+		ALLOC_GROW(commit_formats, commit_formats_len+1,
+			   commit_formats_alloc);
+		memcpy(&commit_formats[ commit_formats_len ], &user_format,
+		       sizeof(user_format));
+		commit_formats_len++;
+	}
+	return 0;
+}
+
+static void setup_commit_formats(void)
 {
 	int i;
-	static struct cmt_fmt_map {
-		const char *n;
-		size_t cmp_len;
-		enum cmit_fmt v;
-	} cmt_fmts[] = {
-		{ "raw",	1,	CMIT_FMT_RAW },
-		{ "medium",	1,	CMIT_FMT_MEDIUM },
-		{ "short",	1,	CMIT_FMT_SHORT },
-		{ "email",	1,	CMIT_FMT_EMAIL },
-		{ "full",	5,	CMIT_FMT_FULL },
-		{ "fuller",	5,	CMIT_FMT_FULLER },
-		{ "oneline",	1,	CMIT_FMT_ONELINE },
+	const char **attempted_aliases = NULL;
+	size_t attempted_aliases_alloc = 0;
+	size_t attempted_aliases_len;
+	struct cmt_fmt_map builtin_formats[] = {
+		{ "raw",	CMIT_FMT_RAW,		NULL,	0 },
+		{ "medium",	CMIT_FMT_MEDIUM,	NULL,	0 },
+		{ "short",	CMIT_FMT_SHORT,		NULL,	0 },
+		{ "email",	CMIT_FMT_EMAIL,		NULL,	0 },
+		{ "full",	CMIT_FMT_FULL,		NULL,	0 },
+		{ "fuller",	CMIT_FMT_FULLER,	NULL,	0 },
+		{ "oneline",	CMIT_FMT_ONELINE,	NULL,	1 }
 	};
+	commit_formats_len = ARRAY_SIZE(builtin_formats);
+	ALLOC_GROW(commit_formats, commit_formats_len, commit_formats_alloc);
+	memcpy(commit_formats, builtin_formats,
+	       sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));
+
+	git_config(git_pretty_formats_config, NULL);
+
+	for (i = ARRAY_SIZE(builtin_formats); i < commit_formats_len; i++) {
+		attempted_aliases_len = 0;
+		struct cmt_fmt_map *aliased_format = &commit_formats[i];
+		const char *fmt = commit_formats[i].user_format;
+		int j;
+
+		if (!commit_formats[i].is_alias)
+			continue;
+
+		while ((aliased_format = find_commit_format(fmt))) {
+			if (!aliased_format->is_alias)
+				break;
+
+			fmt = aliased_format->user_format;
+			for (j=0; j<attempted_aliases_len; j++) {
+				if (!strcmp(fmt, attempted_aliases[j])) {
+					aliased_format = NULL;
+					break;
+				}
+			}
+			if (!aliased_format)
+				break;
+
+			ALLOC_GROW(attempted_aliases, attempted_aliases_len+1,
+				   attempted_aliases_alloc);
+			attempted_aliases[attempted_aliases_len] = fmt;
+			attempted_aliases_len++;
+		}
+		if (aliased_format) {
+			commit_formats[i].format = aliased_format->format;
+			commit_formats[i].user_format = aliased_format->user_format;
+			commit_formats[i].is_tformat = aliased_format->is_tformat;
+			commit_formats[i].is_alias = 0;
+		} else
+			commit_formats[i].format = CMIT_FMT_UNSPECIFIED;
+	}
+}
+
+static struct cmt_fmt_map *find_commit_format(const char *sought)
+{
+	struct cmt_fmt_map *found = NULL;
+	size_t found_match_len = 0;
+
+	if (!commit_formats)
+		setup_commit_formats();
+
+	int i;
+	for (i = 0; i < commit_formats_len; i++) {
+		const char *candidate = commit_formats[i].name;
+		const char *s = sought;
+		size_t match_len = 0;
+
+		if (commit_formats[i].format == CMIT_FMT_UNSPECIFIED)
+			continue;
+
+		for (; s++, candidate++;) {
+			if (!*candidate && *s) {
+				match_len = 0;
+				break;
+			}
+			if (!*candidate || !*s) {
+				match_len = s - sought;
+				break;
+			}
+			if (*s != *candidate) {
+				match_len = 0;
+				break;
+			}
+		}
+		if (match_len > found_match_len) {
+			found = &commit_formats[i];
+		}
+	}
+	return found;
+}
+
+void get_commit_format(const char *arg, struct rev_info *rev)
+{
+	struct cmt_fmt_map *commit_format;
 
 	rev->use_terminator = 0;
 	if (!arg || !*arg) {
@@ -47,21 +169,22 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 		save_user_format(rev, strchr(arg, ':') + 1, arg[0] == 't');
 		return;
 	}
-	for (i = 0; i < ARRAY_SIZE(cmt_fmts); i++) {
-		if (!strncmp(arg, cmt_fmts[i].n, cmt_fmts[i].cmp_len) &&
-		    !strncmp(arg, cmt_fmts[i].n, strlen(arg))) {
-			if (cmt_fmts[i].v == CMIT_FMT_ONELINE)
-				rev->use_terminator = 1;
-			rev->commit_format = cmt_fmts[i].v;
-			return;
-		}
-	}
+
 	if (strchr(arg, '%')) {
 		save_user_format(rev, arg, 1);
 		return;
 	}
 
-	die("invalid --pretty format: %s", arg);
+	commit_format = find_commit_format(arg);
+	if( !commit_format )
+		die("invalid --pretty format: %s", arg);
+
+	rev->commit_format = commit_format->format;
+	rev->use_terminator = commit_format->is_tformat;
+	if( commit_format->format == CMIT_FMT_USERFORMAT ){
+		save_user_format(rev, commit_format->user_format,
+				 commit_format->is_tformat);
+	}
 }
 
 /*
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 28d2948..ee3e934 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -6,7 +6,15 @@
 test_description='Test pretty formats'
 . ./test-lib.sh
 
-test_expect_success "set up basic repos" ">foo && git add foo && git commit -m initial"
+test_expect_success "set up basic repos" \
+	">foo &&
+	>bar &&
+	git add foo &&
+	test_tick &&
+	git commit -m initial &&
+	git add bar &&
+	test_tick &&
+	git commit -m 'add bar'"
 
 for flag in false true always; do
 for color in red green blue reset; do
@@ -16,29 +24,64 @@ for color in red green blue reset; do
 		"$make_expected &&
 		git config color.ui $flag &&
 		git log -1 --pretty=format:'%C$color' > actual &&
-		cmp expected actual"
+		test_cmp expected actual"
 
 
 	test_expect_success "%C($color) with color.ui $flag" \
 		"$make_expected &&
 		git config color.ui $flag &&
 		git log -1 --pretty=format:'%C($color)' > actual &&
-		cmp expected actual"
+		test_cmp expected actual"
 
 	[ ! "$flag" = "always" ] && make_expected=">expected"
 	test_expect_success "%C?$color with color.ui $flag" \
 		"$make_expected &&
 		git config color.ui $flag &&
 		git log -1 --pretty=format:'%C?$color' > actual &&
-		cmp expected actual"
+		test_cmp expected actual"
 
 	test_expect_success "%C?($color) with color.ui $flag" \
 		"$make_expected &&
 		git config color.ui $flag &&
 		git log -1 --pretty=format:'%C?($color)' > actual &&
-		cmp expected actual"
+		test_cmp expected actual"
 
 done
 done
+test_expect_success "reset color flags" "git config --unset color.ui"
+
+test_expect_success "alias builtin format" \
+	"git log --pretty=oneline >expected &&
+	git config format.pretty.test-alias oneline &&
+	git log --pretty=test-alias >actual &&
+	test_cmp expected actual"
+
+test_expect_success "alias user-defined format" \
+	"git log --pretty='format:%h' >expected &&
+	git config format.pretty.test-alias 'format:%h' &&
+	git log --pretty=test-alias >actual &&
+	test_cmp expected actual"
+
+test_expect_success "alias user-defined tformat" \
+	"git log --pretty='tformat:%h' >expected &&
+	git config format.pretty.test-alias 'tformat:%h' &&
+	git log --pretty=test-alias >actual &&
+	test_cmp expected actual"
+
+test_expect_code 128 "alias non-existant format" \
+	"git config format.pretty.test-alias format-that-will-never-exist &&
+	git log --pretty=test-alias"
+
+test_expect_success "alias of an alias" \
+	"git log --pretty='tformat:%h' >expected &&
+	git config format.pretty.test-foo 'tformat:%h' &&
+	git config format.pretty.test-bar test-foo &&
+	git log --pretty=test-bar >actual &&
+	test_cmp expected actual"
+
+test_expect_code 128 "alias loop" \
+	"git config format.pretty.test-foo test-bar &&
+	git config format.pretty.test-bar test-foo &&
+	git log --pretty=test-foo"
 
 test_done
-- 
1.7.1.rc1.13.gbb0a0a.dirty

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/3] pretty: format aliases
  2010-04-25 15:42 [PATCH 0/3] pretty: format aliases Will Palmer
                   ` (2 preceding siblings ...)
  2010-04-25 15:42 ` [PATCH 3/3] pretty: add aliases for pretty formats Will Palmer
@ 2010-04-25 19:48 ` Jeff King
  2010-04-25 20:40   ` Will Palmer
  2010-04-25 22:09   ` Jonathan Nieder
  3 siblings, 2 replies; 16+ messages in thread
From: Jeff King @ 2010-04-25 19:48 UTC (permalink / raw)
  To: Will Palmer; +Cc: git, gitster

On Sun, Apr 25, 2010 at 04:42:52PM +0100, Will Palmer wrote:

> The following patch series adds the ability to configure aliases for
> user-defined formats. The first two patches define new placeholders and
> modify the output of existing placeholders to allow aliases to be more
> consistent with the way builtin formats are handled. The final patch
> adds support for the aliases themselves.

Interesting idea. I think right now most people just want to use their
format with "log", so they would do something like:

  git config alias.mylog "log --format='...'"

Your method allows the same format to be used with multiple commands,
which is more flexible. But I wonder how many people would find it
useful in practice. I can think of only "log" and "show" that I would
really want to share between.

I skimmed your patches and have included a few comments below, as I
don't have time at the moment to do a thorough review.

> There were a couple of places where I wasn't entirely sure about which
> color setting I should be following, but I've tried to be consistent
> throughout. It may be that I could have simply followed diffopt's color
> option in all cases, in which case various modifications to show_log()
> were entirely unnecessary. I'll await judgement at the hands of one who
> groks those sections more than I do, but I think what I've done feels
> correct.

I think that makes some sense. The "diff" color option was the first,
and so the commit-coloring in "git log" follows that already. So short
of making a new "color.log" variable, that makes the most sense.

> My original goal was to make it possible to define all of the builtin
> formats as builtin aliases to format strings, but complications

I like that goal. As you found out, it may be harder than you would
like. But the pretty-print code is IMHO a bit messy with conditionals,
and most of it can now be expressed as much simpler user-formats.

>   pretty: add conditional %C?colorname placeholders

This one seems reasonable overall.

>   pretty: make %H/%h dependent on --abbrev[-commit]

This is really a variation on the first one. And there are more
variations, like %T/%t, %P/%p, etc. I'm a little hesitant to just change
the meaning of "%H", which has always explicitly meant the full sha1.
Should we perhaps introduce some universal syntax for "abbreviate if
--abbrev was given, otherwise full sha1". Like "%?H" as you did above,
except that "?" doesn't really make sense.

>   pretty: add aliases for pretty formats

The config variables format.* are traditionally about format-patch. I
see we have format.pretty these days, too. I'm not sure if that is a
deliberate attempt to make format.* more inclusive, or simply an error.
If the latter, we should probably not make it worse with
format.pretty.name.

-Peff

PS Welcome to the git list. It is nice to see a submission from a
newcomer who has clearly read SubmittingPatches, and that even has
appropriate documentation and test updates. :)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/3] pretty: format aliases
  2010-04-25 19:48 ` [PATCH 0/3] pretty: format aliases Jeff King
@ 2010-04-25 20:40   ` Will Palmer
  2010-04-25 22:09   ` Jonathan Nieder
  1 sibling, 0 replies; 16+ messages in thread
From: Will Palmer @ 2010-04-25 20:40 UTC (permalink / raw)
  To: Jeff King; +Cc: git

On Sun, Apr 25, 2010 at 8:48 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Apr 25, 2010 at 04:42:52PM +0100, Will Palmer wrote:
> Interesting idea. I think right now most people just want to use their
> format with "log", so they would do something like:
>
>  git config alias.mylog "log --format='...'"
>
> Your method allows the same format to be used with multiple commands,
> which is more flexible. But I wonder how many people would find it
> useful in practice. I can think of only "log" and "show" that I would
> really want to share between.

Indeed, this is what I do now, and I really wasn't expecting anyone to
use it for a command other than log. There are three main
justifications:
 1) I hope to eventually have all builtin formats converted to use
this mechanism, and this is a good stepping-stone.
 2) defining a git alias to "log" when I really just want a shorter
way to type a format has always felt horribly clunky to me.
 3) it's so easy to add it's essentially free.

as you might guess, #2 is what really started the itch, and #3 is what
encouraged me to scratch it. #1 was more a side-effect when I realized
(when first trying to implement things) that it's not how things are
currently done.

>
> I skimmed your patches and have included a few comments below, as I
> don't have time at the moment to do a thorough review.
>
Thank you, any input is greatly appreciated.

> I think that makes some sense. The "diff" color option was the first,
> and so the commit-coloring in "git log" follows that already. So short
> of making a new "color.log" variable, that makes the most sense.
>
following the "diff" color seemed to be the right thing in most
places, but for some things it looked as if there may be more-specific
not-diff-related color options being passed int, which is why I
changed the show_log() calls. Looking back over the patch now, it
seems that the only place left which doesn't reference a rev_info
struct is show-branch.c, which doesn't even have a --format option. So
I suppose that can all get ripped out and replaced with a single check
of the diff options in  the next version of the patch.

> This is really a variation on the first one. And there are more
> variations, like %T/%t, %P/%p, etc. I'm a little hesitant to just change
> the meaning of "%H", which has always explicitly meant the full sha1.
> Should we perhaps introduce some universal syntax for "abbreviate if
> --abbrev was given, otherwise full sha1". Like "%?H" as you did above,
> except that "?" doesn't really make sense.
>
This one I was also a bit iffy on, but in my mind the fact that %H
currently completely ignores the --abbrev-commit option feels like a
bug to me. One of those "if it has no effect, there should be a
warning", things.
We can at least be assured that as --abbrev[-commit] previously had no
effect on --format options, that the only people who would have used
them together have probably been disappointed at the results, and did
not continue to do so.
There /are/ a couple of places in-code which use %H to fetch a
commit-hash-string, and I made sure to avoid modification of rev_info
when I spotted that, but really that feels like something that should
be using a wrapper get_commit_hash_string() function.

> The config variables format.* are traditionally about format-patch. I
> see we have format.pretty these days, too. I'm not sure if that is a
> deliberate attempt to make format.* more inclusive, or simply an error.
> If the latter, we should probably not make it worse with
> format.pretty.name.

I was writing them as pretty.<name>, until I saw that format.pretty
was already there. I'm not sure which is the weirder of the two.

>
> -Peff
>
> PS Welcome to the git list. It is nice to see a submission from a
> newcomer who has clearly read SubmittingPatches, and that even has
> appropriate documentation and test updates. :)
>

Nice to be welcome. You may have seen a test-run I did to get my
confidence up by submitting a two-line documentation change a week or
so ago ;)
-- 
Will Palmer
wmpalmer@gmail.com

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/3] pretty: format aliases
  2010-04-25 19:48 ` [PATCH 0/3] pretty: format aliases Jeff King
  2010-04-25 20:40   ` Will Palmer
@ 2010-04-25 22:09   ` Jonathan Nieder
  2010-04-26 17:22     ` Jeff King
  1 sibling, 1 reply; 16+ messages in thread
From: Jonathan Nieder @ 2010-04-25 22:09 UTC (permalink / raw)
  To: Jeff King; +Cc: Will Palmer, git, gitster, Cheng Renquan

Jeff King wrote:

> The config variables format.* are traditionally about format-patch. I
> see we have format.pretty these days, too.

Yes, it as added in commit v1.5.5-rc0~57^2 (log/show/whatchanged:
introduce format.pretty configuration, 2008-03-02):

    When running log/show/whatchanged from the command line, the user may want
    to use a preferred format without having to pass --pretty=<fmt> option
    every time from the command line.  This teaches these three commands to
    honor a new configuration variable, format.pretty.

    The --pretty option given from the command line will override the 
    configured format.

I use it to mess with line wrapping; that it’s possible is very nice.
Thanks, Renquan.

Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] pretty: add conditional %C?colorname placeholders
  2010-04-25 15:42 ` [PATCH 1/3] pretty: add conditional %C?colorname placeholders Will Palmer
@ 2010-04-26  6:30   ` Alex Riesen
       [not found]     ` <r2o5b9751661004260113m7b6c387bm8467a063c13e5a0f@mail.gmail.com>
  0 siblings, 1 reply; 16+ messages in thread
From: Alex Riesen @ 2010-04-26  6:30 UTC (permalink / raw)
  To: Will Palmer; +Cc: git, gitster

On Sun, Apr 25, 2010 at 17:42, Will Palmer <wmpalmer@gmail.com> wrote:
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 1686a54..53eb903 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -132,6 +132,7 @@ The placeholders are:
>  - '%Cblue': switch color to blue
>  - '%Creset': reset color
>  - '%C(...)': color specification, as described in color.branch.* config option
> +- '%C?...: switch to specified color, if relevant color.* config option specifies that color is ok

You missed the closing quote character. Besides, how do you think to
distinguish between "%C?diff.color" and "%C?diff.colorcontinuation text"?
The "%C(...)" has a placeholde termination rule (the closing bracket), yours
does not seem to have one (unless something is missing in the documentation).

Ah... I see. Definitely something missing. The spec should be: '%C?(...)'.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] pretty: add conditional %C?colorname placeholders
       [not found]     ` <r2o5b9751661004260113m7b6c387bm8467a063c13e5a0f@mail.gmail.com>
@ 2010-04-26  8:20       ` Will Palmer
  0 siblings, 0 replies; 16+ messages in thread
From: Will Palmer @ 2010-04-26  8:20 UTC (permalink / raw)
  To: git

On Mon, Apr 26, 2010 at 7:30 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
> On Sun, Apr 25, 2010 at 17:42, Will Palmer <wmpalmer@gmail.com> wrote:
>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
>> index 1686a54..53eb903 100644
>> --- a/Documentation/pretty-formats.txt
>> +++ b/Documentation/pretty-formats.txt
>> @@ -132,6 +132,7 @@ The placeholders are:
>>  - '%Cblue': switch color to blue
>>  - '%Creset': reset color
>>  - '%C(...)': color specification, as described in color.branch.* config option
>> +- '%C?...: switch to specified color, if relevant color.* config option specifies that color is ok
>
> You missed the closing quote character. Besides, how do you think to
> distinguish between "%C?diff.color" and "%C?diff.colorcontinuation text"?
> The "%C(...)" has a placeholde termination rule (the closing bracket), yours
> does not seem to have one (unless something is missing in the documentation).
>
> Ah... I see. Definitely something missing. The spec should be: '%C?(...)'.
>

It's meant to handle both %C?green and %C?(green), perhaps I should
list those out on separate lines?
Missing quote noted, thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/3] pretty: format aliases
  2010-04-25 22:09   ` Jonathan Nieder
@ 2010-04-26 17:22     ` Jeff King
  2010-04-26 17:57       ` Jonathan Nieder
  0 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2010-04-26 17:22 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Will Palmer, git, gitster, Cheng Renquan

On Sun, Apr 25, 2010 at 05:09:55PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > The config variables format.* are traditionally about format-patch. I
> > see we have format.pretty these days, too.
> 
> Yes, it as added in commit v1.5.5-rc0~57^2 (log/show/whatchanged:
> introduce format.pretty configuration, 2008-03-02):
> 
>     When running log/show/whatchanged from the command line, the user may want
>     to use a preferred format without having to pass --pretty=<fmt> option
>     every time from the command line.  This teaches these three commands to
>     honor a new configuration variable, format.pretty.
> 
>     The --pretty option given from the command line will override the 
>     configured format.
> 
> I use it to mess with line wrapping; that it’s possible is very nice.
> Thanks, Renquan.

To be clear, I don't have any problem with the _functionality_. I just
think it probably should have been log.pretty, or pretty.default or
something. Too late now, though.

-Peff

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/3] pretty: format aliases
  2010-04-26 17:22     ` Jeff King
@ 2010-04-26 17:57       ` Jonathan Nieder
  2010-04-26 18:07         ` Jeff King
  0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Nieder @ 2010-04-26 17:57 UTC (permalink / raw)
  To: Jeff King; +Cc: Will Palmer, git, gitster, Cheng Renquan

Jeff King wrote:

> To be clear, I don't have any problem with the _functionality_. I just
> think it probably should have been log.pretty, or pretty.default or
> something. Too late now, though.

Ah, sorry to be dense.  Maybe something like the following would work:

 [log "format"]
	changelog = "* [%h] %s"

This is rev-list’s code, and log is its most noticeable manifestation.

By the way: I just noticed today that rev-list is not very friendly
when given a --pretty=format: specification:

  $ git rev-list -1 --oneline HEAD
  651b91e log --format=email: fix confusing variable name
  $ git rev-list -1 --format='%h %s' HEAD
  commit 651b91ef7a15a46145d65193c8709670b969161f
  651b91e log --format=email: fix confusing variable name

Is this deliberate?  I mention it because I seem to remember from the
discussion of --decorate configuration that scripts should be using
rev-list in preference to log even when a --format option is involved.

Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/3] pretty: format aliases
  2010-04-26 17:57       ` Jonathan Nieder
@ 2010-04-26 18:07         ` Jeff King
  2010-04-26 18:37           ` Jonathan Nieder
  0 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2010-04-26 18:07 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Will Palmer, git, gitster, Cheng Renquan

On Mon, Apr 26, 2010 at 12:57:08PM -0500, Jonathan Nieder wrote:

> > To be clear, I don't have any problem with the _functionality_. I just
> > think it probably should have been log.pretty, or pretty.default or
> > something. Too late now, though.
> 
> Ah, sorry to be dense.  Maybe something like the following would work:
> 
>  [log "format"]
> 	changelog = "* [%h] %s"

We usually reserve the [log "format"] for when "format" can be an
arbitrary string. I'm not sure if we can use some nicer syntax like
[log.format]; I'd have to experiment.

> This is rev-list’s code, and log is its most noticeable manifestation.

We also tend to refer to the log/show/whatchanged as "the log family of
commands". I think whatchanged is pretty much dead and historical at
this point, though.

> By the way: I just noticed today that rev-list is not very friendly
> when given a --pretty=format: specification:
> 
>   $ git rev-list -1 --oneline HEAD
>   651b91e log --format=email: fix confusing variable name
>   $ git rev-list -1 --format='%h %s' HEAD
>   commit 651b91ef7a15a46145d65193c8709670b969161f
>   651b91e log --format=email: fix confusing variable name
> 
> Is this deliberate?  I mention it because I seem to remember from the
> discussion of --decorate configuration that scripts should be using
> rev-list in preference to log even when a --format option is involved.

I'm not sure it's deliberate, as in somebody thought it was a good idea.
But it has always been that way, and I think it has been deliberate that
we not change it. I agree it is unfriendly.

I think we have accepted that scripts will use log because of its
enhanced features (gitk does), but we generally suppress extra output
(like decorations or notes) for at least --pretty=raw and
--pretty=format.

-Peff

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/3] pretty: format aliases
  2010-04-26 18:07         ` Jeff King
@ 2010-04-26 18:37           ` Jonathan Nieder
  2010-04-26 21:14             ` Will Palmer
  0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Nieder @ 2010-04-26 18:37 UTC (permalink / raw)
  To: Jeff King; +Cc: Will Palmer, git, gitster, Cheng Renquan

Jeff King wrote:
> On Mon, Apr 26, 2010 at 12:57:08PM -0500, Jonathan Nieder wrote:

>>  [log "format"]
>> 	changelog = "* [%h] %s"
>
> We usually reserve the [log "format"] for when "format" can be an
> arbitrary string. I'm not sure if we can use some nicer syntax like
> [log.format]; I'd have to experiment.

Alternatively, we could use

 [logformat]
	changelog = ...

which is arguably just as clear and may be closer to existing
practice.  But really, I am happy as long as the configuration exists.

Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/3] pretty: format aliases
  2010-04-26 18:37           ` Jonathan Nieder
@ 2010-04-26 21:14             ` Will Palmer
  2010-04-26 22:00               ` Jonathan Nieder
  0 siblings, 1 reply; 16+ messages in thread
From: Will Palmer @ 2010-04-26 21:14 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Jeff King, git, gitster, Cheng Renquan

On Mon, 2010-04-26 at 13:37 -0500, Jonathan Nieder wrote:
> Alternatively, we could use
> 
>  [logformat]
> 	changelog = ...
> 
> which is arguably just as clear and may be closer to existing
> practice.  But really, I am happy as long as the configuration exists.
> 
> Jonathan

I like the one-level-of-hierarchy thing ([format "pretty"] is ugly for a
number of reasons), but two problems with that specific name:
 - it says "log", while this effects more than the "log" command
 - it says "format", while this doesn't effect --format.

[pretty]
    changelog = ...

is not taken, and my only hesitation there is that some other config
option may want to be there. Still, it's what I'll call it in the next
version of the patch, unless there are specific objections. (which I
certainly may have already missed, as I've just switched mail clients
twice today)

-- Will

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/3] pretty: format aliases
  2010-04-26 21:14             ` Will Palmer
@ 2010-04-26 22:00               ` Jonathan Nieder
  2010-04-27  1:25                 ` Jeff King
  0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Nieder @ 2010-04-26 22:00 UTC (permalink / raw)
  To: Will Palmer; +Cc: Jeff King, git, gitster, Cheng Renquan, Nanako Shiraishi

Will Palmer wrote:

> [pretty]
>     changelog = ...

Fine by me. :)

> On Mon, 2010-04-26 at 13:37 -0500, Jonathan Nieder wrote:

>>  [logformat]
>> 	changelog = ...
[...]
> I like the one-level-of-hierarchy thing ([format "pretty"] is ugly for a
> number of reasons), but two problems with that specific name:
>  - it says "log", while this effects more than the "log" command
>  - it says "format", while this doesn't effect --format.

Ah, but it does affect --format.  When Nanako wrote commit 3a4c1a5
(Add --format that is a synonym to --pretty, 2009-02-24), the new
option made me very happy.  And since then it has remained a synonym.

Cheers,
Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/3] pretty: format aliases
  2010-04-26 22:00               ` Jonathan Nieder
@ 2010-04-27  1:25                 ` Jeff King
  0 siblings, 0 replies; 16+ messages in thread
From: Jeff King @ 2010-04-27  1:25 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Will Palmer, git, gitster, Cheng Renquan, Nanako Shiraishi

On Mon, Apr 26, 2010 at 05:00:49PM -0500, Jonathan Nieder wrote:

> Will Palmer wrote:
> 
> > [pretty]
> >     changelog = ...
> 
> Fine by me. :)

Also my favorite of the suggestions.

-Peff

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2010-04-27  1:26 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-04-25 15:42 [PATCH 0/3] pretty: format aliases Will Palmer
2010-04-25 15:42 ` [PATCH 1/3] pretty: add conditional %C?colorname placeholders Will Palmer
2010-04-26  6:30   ` Alex Riesen
     [not found]     ` <r2o5b9751661004260113m7b6c387bm8467a063c13e5a0f@mail.gmail.com>
2010-04-26  8:20       ` Will Palmer
2010-04-25 15:42 ` [PATCH 2/3] pretty: make %H/%h dependent on --abbrev[-commit] Will Palmer
2010-04-25 15:42 ` [PATCH 3/3] pretty: add aliases for pretty formats Will Palmer
2010-04-25 19:48 ` [PATCH 0/3] pretty: format aliases Jeff King
2010-04-25 20:40   ` Will Palmer
2010-04-25 22:09   ` Jonathan Nieder
2010-04-26 17:22     ` Jeff King
2010-04-26 17:57       ` Jonathan Nieder
2010-04-26 18:07         ` Jeff King
2010-04-26 18:37           ` Jonathan Nieder
2010-04-26 21:14             ` Will Palmer
2010-04-26 22:00               ` Jonathan Nieder
2010-04-27  1:25                 ` Jeff King

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).