threads / patch / 22692

patchAdd `[decorate]' configuration section.

Subject: [PATCH] Add `[decorate]' configuration section.

## tl;dr

10 messages between Feb 17, 2010 and Feb 26, 2010. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Steven Drake· Feb 17, 2010, 08:22 UTC · lore

This can be used to enable the display of ref names of commits that are shown by log commands. Each of the log commands (whatchanged, show, reflog, and log) can be enable separately.

e.g:
	[decorate]
		log
		reflog
Signed-off-by: Steven Drake <sdrake@xnet.co.nz>
---
 Documentation/config.txt |   12 ++++++++++++
 builtin-log.c            |   43 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 51 insertions(+), 4 deletions(-)
Show changes to 2 files +51 −4

Documentation/config.txt, builtin-log.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4c36aa9..dc9d6fd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -740,6 +740,18 @@ commit.template::
 	"{tilde}/" is expanded to the value of `$HOME` and "{tilde}user/" to the
 	specified user's home directory.
 
+decorate.whatchanged::
+decorate.log::
+decorate.reflog::
+decorate.show::
+	Print out the ref names of any commits that are shown by the different
+	log command.  If 'short' is specified, the ref name prefixes
+	'refs/heads/', 'refs/tags/' and 'refs/remotes/' will not be printed.
+	If 'full' is specified, the full ref name (including prefix) will be
+	printed.  May be set as a bool which will be treated as 'short'.
+	This is the same as the log commands '--decorate' option.
+
+
 diff.autorefreshindex::
 	When using 'git diff' to compare with work tree
 	files, do not consider stat-only change as changed.
diff --git a/builtin-log.c b/builtin-log.c
index 8d16832..51b5e10 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -24,6 +24,8 @@
 static const char *default_date_mode = NULL;
 
 static int default_show_root = 1;
+static struct { int whatchanged, show, reflog, log;} decorate = {0,0,0,0};
+static int decoration_style = 0;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
@@ -35,7 +37,6 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		      struct rev_info *rev)
 {
 	int i;
-	int decoration_style = 0;
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
@@ -58,9 +59,6 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, "HEAD");
 
-	if (!rev->show_notes_given && !rev->pretty_given)
-		rev->show_notes = 1;
-
 	if (rev->diffopt.pickaxe || rev->diffopt.filter)
 		rev->always_show_header = 0;
 	if (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {
@@ -244,8 +242,41 @@ static int cmd_log_walk(struct rev_info *rev)
 	return diff_result_code(&rev->diffopt, 0);
 }
 
+static int set_decoration_style(const char *var, const char *value)
+{
+	switch (git_config_maybe_bool(var, value)) {
+	case 0:
+		return 0;
+	case 1:
+		return DECORATE_SHORT_REFS;
+	default:
+		break;
+	}
+	if (!strcmp(value, "full"))
+		return DECORATE_FULL_REFS;
+	else if (!strcmp(value, "short"))
+		return DECORATE_SHORT_REFS;
+	return 0;
+}
+
 static int git_log_config(const char *var, const char *value, void *cb)
 {
+	if (!strcmp(var, "decorate.log")) {
+		decorate.log = set_decoration_style(var, value);
+		return 0;
+	}
+	if (!strcmp(var, "decorate.reflog")) {
+		decorate.reflog = set_decoration_style(var, value);
+		return 0;
+	}
+	if (!strcmp(var, "decorate.show")) {
+		decorate.show = set_decoration_style(var, value);
+		return 0;
+	}
+	if (!strcmp(var, "decorate.whatchanged")) {
+		decorate.whatchanged = set_decoration_style(var, value);
+		return 0;
+	}
 	if (!strcmp(var, "format.pretty"))
 		return git_config_string(&fmt_pretty, var, value);
 	if (!strcmp(var, "format.subjectprefix"))
@@ -264,6 +295,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 
 	git_config(git_log_config, NULL);
+	decoration_style = decorate.whatchanged;
 
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
@@ -331,6 +363,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	int i, count, ret = 0;
 
 	git_config(git_log_config, NULL);
+	decoration_style = decorate.show;
 
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
@@ -407,6 +440,7 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 
 	git_config(git_log_config, NULL);
+	decoration_style = decorate.reflog;
 
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
@@ -440,6 +474,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 
 	git_config(git_log_config, NULL);
+	decoration_style = decorate.log;
 
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
-- 
1.6.6
Junio C Hamano· Feb 17, 2010, 17:50 UTC · re: Steven Drake · lore

Re: [PATCH] Add `[decorate]' configuration section.

Steven Drake <sdrake@xnet.co.nz> writes:
Show 8 quoted lines
> This can be used to enable the display of ref names of commits that are
> shown by log commands.  Each of the log commands (whatchanged, show,
> reflog, and log) can be enable separately.
>
> e.g:
> 	[decorate]
> 		log
> 		reflog

What is the reason to set these configuration differently except "because we can"? Also, if it is a good idea to allow setting them independently, you would need to make it easier for people who want to set them all to the same value, e.g. "[decorate] logfamily = short" or something.

I somehow find the older "log.decorate" much more natural and the changes from it to this version not worth it, but maybe it is just me.

Junio C Hamano· Feb 17, 2010, 18:28 UTC · re: Junio C Hamano · lore

Re* [PATCH] Add `[decorate]' configuration section.

Junio C Hamano <gitster@pobox.com> writes:
> I somehow find the older "log.decorate" much more natural and the changes
> from it to this version not worth it, but maybe it is just me.

I was looking at your previous one. Here is a bit of clean-up on top of yours.

-- >8 --
Subject: [PATCH] log.decorate: usability fixes

The configuration is meant to suppliment --decorate command line option that can be used as a boolean to turn the feature on, so it is natural to expect

	[log]
		decorate
		decorate = yes

to work. The original commit would segfault with the first one, and would not understand the second one.

Once a user has this configuration in ~/.gitconfig, there needs to be a way to override it from the command line. Add --no-decorate option to log family and also allow --decorate=no to mean the same thing. Since we allow setting log.decorate to 'true', the command line also should accept --decorate=yes and behave accordingly.

New tests in t4202 are designed to exercise the interaction between the configuration variable and the command line option that overrides it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-log.txt |    3 +-
 builtin-log.c             |   35 +++++++++++++++++++++++---------
 t/t4202-log.sh            |   49 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 11 deletions(-)
Show changes to 3 files +76 −10

Documentation/git-log.txt, builtin-log.c, t/t4202-log.sh

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 0e39bb6..64bb879 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -37,7 +37,8 @@ include::diff-options.txt[]
 	and <until>, see "SPECIFYING REVISIONS" section in
 	linkgit:git-rev-parse[1].
 
---decorate[=short|full]::
+--no-decorate::
+--decorate[=short|full|no]::
 	Print out the ref names of any commits that are shown. If 'short' is
 	specified, the ref name prefixes 'refs/heads/', 'refs/tags/' and
 	'refs/remotes/' will not be printed. If 'full' is specified, the
diff --git a/builtin-log.c b/builtin-log.c
index 3100dc0..0afba31 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -24,7 +24,7 @@
 static const char *default_date_mode = NULL;
 
 static int default_show_root = 1;
-static int decoration_style = 0;
+static int decoration_style;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
@@ -32,6 +32,23 @@ static const char * const builtin_log_usage =
 	"git log [<options>] [<since>..<until>] [[--] <path>...]\n"
 	"   or: git show [options] <object>...";
 
+static int parse_decoration_style(const char *var, const char *value)
+{
+	switch (git_config_maybe_bool(var, value)) {
+	case 1:
+		return DECORATE_SHORT_REFS;
+	case 0:
+		return 0;
+	default:
+		break;
+	}
+	if (!strcmp(value, "full"))
+		return DECORATE_FULL_REFS;
+	else if (!strcmp(value, "short"))
+		return DECORATE_SHORT_REFS;
+	return -1;
+}
+
 static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		      struct rev_info *rev)
 {
@@ -74,12 +91,11 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 			decoration_style = DECORATE_SHORT_REFS;
 		} else if (!prefixcmp(arg, "--decorate=")) {
 			const char *v = skip_prefix(arg, "--decorate=");
-			if (!strcmp(v, "full"))
-				decoration_style = DECORATE_FULL_REFS;
-			else if (!strcmp(v, "short"))
-				decoration_style = DECORATE_SHORT_REFS;
-			else
+			decoration_style = parse_decoration_style(arg, v);
+			if (decoration_style < 0)
 				die("invalid --decorate option: %s", arg);
+		} else if (!strcmp(arg, "--no-decorate")) {
+			decoration_style = 0;
 		} else if (!strcmp(arg, "--source")) {
 			rev->show_source = 1;
 		} else if (!strcmp(arg, "-h")) {
@@ -253,10 +269,9 @@ static int git_log_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "log.date"))
 		return git_config_string(&default_date_mode, var, value);
 	if (!strcmp(var, "log.decorate")) {
-		if (!strcmp(value, "full"))
-			decoration_style = DECORATE_FULL_REFS;
-		else if (!strcmp(value, "short"))
-			decoration_style = DECORATE_SHORT_REFS;
+		decoration_style = parse_decoration_style(var, value);
+		if (decoration_style < 0)
+			decoration_style = 0; /* maybe warn? */
 		return 0;
 	}
 	if (!strcmp(var, "log.showroot")) {
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 1dc224f..2230e60 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -387,5 +387,54 @@ test_expect_success 'log --graph with merge' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log.decorate configuration' '
+	git config --unset-all log.decorate || :
+
+	git log --oneline >expect.none &&
+	git log --oneline --decorate >expect.short &&
+	git log --oneline --decorate=full >expect.full &&
+
+	echo "[log] decorate" >>.git/config &&
+	git log --oneline >actual &&
+	test_cmp expect.short actual &&
+
+	git config --unset-all log.decorate &&
+	git config log.decorate true &&
+	git log --oneline >actual &&
+	test_cmp expect.short actual &&
+	git log --oneline --decorate=full >actual &&
+	test_cmp expect.full actual &&
+	git log --oneline --decorate=no >actual &&
+	test_cmp expect.none actual &&
+
+	git config --unset-all log.decorate &&
+	git config log.decorate no &&
+	git log --oneline >actual &&
+	test_cmp expect.none actual &&
+	git log --oneline --decorate >actual &&
+	test_cmp expect.short actual &&
+	git log --oneline --decorate=full >actual &&
+	test_cmp expect.full actual &&
+
+	git config --unset-all log.decorate &&
+	git config log.decorate short &&
+	git log --oneline >actual &&
+	test_cmp expect.short actual &&
+	git log --oneline --no-decorate >actual &&
+	test_cmp expect.none actual &&
+	git log --oneline --decorate=full >actual &&
+	test_cmp expect.full actual &&
+
+	git config --unset-all log.decorate &&
+	git config log.decorate full &&
+	git log --oneline >actual &&
+	test_cmp expect.full actual &&
+	git log --oneline --no-decorate >actual &&
+	test_cmp expect.none actual &&
+	git log --oneline --decorate >actual &&
+	test_cmp expect.short actual
+
+'
+
 test_done
 
-- 
1.7.0.54.gbe03aa
Heiko Voigt· Feb 20, 2010, 17:17 UTC · re: Junio C Hamano · lore

Re: Re* [PATCH] Add `[decorate]' configuration section.

On Wed, Feb 17, 2010 at 10:28:28AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
I have just seen this in Junios tree:
commit 8a3d203bd02bec48a02557961899d81da172fa23 writes:
Show 21 quoted lines
> Author: Junio C Hamano <gitster@pobox.com>
> Date:   Wed Feb 17 10:20:49 2010 -0800
> 
>     log.decorate: usability fixes
>     
>     The configuration is meant to suppliment --decorate command line option
>     that can be used as a boolean to turn the feature on, so it is natural
>     to expect
>     
>         [log]
>                 decorate
>                 decorate = yes
>     
>     to work.  The original commit would segfault with the first one, and
>     would not understand the second one.
>     
>     Once a user has this configuration in ~/.gitconfig, there needs to be a
>     way to override it from the command line.  Add --no-decorate option to
>     log family and also allow --decorate=no to mean the same thing.  Since
>     we allow setting log.decorate to 'true', the command line also should
>     accept --decorate=yes and behave accordingly.

How about calling that option --plain, --porcelain or similar and have it disable all configs which change the current output? Because if we can go this way I would actually like to add more useful configurations. For example I always find me typing:

git log --decorate --abbrev-commit -p
which in most situations just gives you a nicer overview.
cheers Heiko
Thomas Rast· Feb 20, 2010, 17:49 UTC · re: Heiko Voigt · lore

Re: Re* [PATCH] Add `[decorate]' configuration section.

On Saturday 20 February 2010 18:17:44 Heiko Voigt wrote:
Show 7 quoted lines
> On Wed, Feb 17, 2010 at 10:28:28AM -0800, Junio C Hamano wrote:
> > Junio C Hamano <gitster@pobox.com> writes:
> 
> I have just seen this in Junios tree:
> 
> commit 8a3d203bd02bec48a02557961899d81da172fa23 writes:
> > Author: Junio C Hamano <gitster@pobox.com>
[...]
Show 14 quoted lines
> >     Once a user has this configuration in ~/.gitconfig, there needs to be a
> >     way to override it from the command line.  Add --no-decorate option to
> >     log family and also allow --decorate=no to mean the same thing.  Since
> >     we allow setting log.decorate to 'true', the command line also should
> >     accept --decorate=yes and behave accordingly.
> 
> How about calling that option --plain, --porcelain or similar and have it
> disable all configs which change the current output? Because if we can go this
> way I would actually like to add more useful configurations. For example I
> always find me typing:
> 
> git log --decorate --abbrev-commit -p
> 
> which in most situations just gives you a nicer overview.

Since 66b2ed0 (Fix "log" family not to be too agressive about showing notes, 2010-01-20), git log --pretty=... doesn't show notes. Perhaps at least the 'raw' format should also implicitly disable other user configuration, including this?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Feb 22, 2010, 05:52 UTC · re: Thomas Rast · lore

Re: Re* [PATCH] Add `[decorate]' configuration section.

Thomas Rast <trast@student.ethz.ch> writes:
> Since 66b2ed0 (Fix "log" family not to be too agressive about showing
> notes, 2010-01-20), git log --pretty=... doesn't show notes.  Perhaps
> at least the 'raw' format should also implicitly disable other user
> configuration, including this?
Sounds like a reasonable suggestion.
Steven Drake· Feb 25, 2010, 22:44 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add `[decorate]' configuration section.

Sorry for taking so  long to reply to your email, got destracted.
 
On Wed, 17 Feb 2010, Junio C Hamano wrote:
Show 13 quoted lines
> Steven Drake <sdrake@xnet.co.nz> writes:
> 
> > This can be used to enable the display of ref names of commits that are
> > shown by log commands.  Each of the log commands (whatchanged, show,
> > reflog, and log) can be enable separately.
> >
> > e.g:
> > 	[decorate]
> > 		log
> > 		reflog
> 
> What is the reason to set these configuration differently except "because
> we can"? 

Basicly yes, but I did this when I was looking at what you said in a reply about the "log.decorate" changes, I.E.

  On Tue, 16 Feb 2010, Junio C Hamano wrote:
  > I was not worried about what your change does.  I am worried about
  > protecting what the code after your change currently does from future
  > changes done by other people while you are not actively watching the
  > patches in flight on this list.
But that can easly be with the patch below.
> Also, if it is a good idea to allow setting them independently,
> you would need to make it easier for people who want to set them all to
> the same value, e.g. "[decorate] logfamily = short" or something.
Good Idea!
> I somehow find the older "log.decorate" much more natural and the changes
> from it to this version not worth it, but maybe it is just me.

I agree with you, I just thought having a single "[decorate]" section in the config file was cleaner. But nomater which way around it was done someone would want t done the other way around.

--- >8 ---
Subject: [PATCH 1/1] log.decorate: future proofing against builtin-log.c changes

Instead of directly setting 'decoration_style' in git_log_config() we set an intermediate variable, then for each of the commands we want 'log.decorate' to have an affect 'decoration_style' is set from the intermediate.

This is to protect against future change to cmd_log_init() and how it is used or the commands 'log.decorate' affects.

Signed-off-by: Steven Drake <sdrake@xnet.co.nz>
---
 builtin-log.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)
Show changes to builtin-log.c +8 −4
diff --git a/builtin-log.c b/builtin-log.c
index 0afba31..91712e6 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -24,7 +24,7 @@
 static const char *default_date_mode = NULL;
 
 static int default_show_root = 1;
-static int decoration_style;
+static int default_decoration_style, decoration_style;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
@@ -269,9 +269,9 @@ static int git_log_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "log.date"))
 		return git_config_string(&default_date_mode, var, value);
 	if (!strcmp(var, "log.decorate")) {
-		decoration_style = parse_decoration_style(var, value);
-		if (decoration_style < 0)
-			decoration_style = 0; /* maybe warn? */
+		default_decoration_style = parse_decoration_style(var, value);
+		if (default_decoration_style < 0)
+			default_decoration_style = 0; /* maybe warn? */
 		return 0;
 	}
 	if (!strcmp(var, "log.showroot")) {
@@ -286,6 +286,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 
 	git_config(git_log_config, NULL);
+	decoration_style = default_decoration_style;
 
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
@@ -353,6 +354,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	int i, count, ret = 0;
 
 	git_config(git_log_config, NULL);
+	decoration_style = default_decoration_style;
 
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
@@ -429,6 +431,7 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 
 	git_config(git_log_config, NULL);
+	decoration_style = default_decoration_style;
 
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
@@ -462,6 +465,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 
 	git_config(git_log_config, NULL);
+	decoration_style = default_decoration_style;
 
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
-- 
1.6.6
Junio C Hamano· Feb 25, 2010, 23:12 UTC · re: Steven Drake · lore

Re: [PATCH] Add `[decorate]' configuration section.

Steven Drake <sdrake@xnet.co.nz> writes:
Show 7 quoted lines
>   On Tue, 16 Feb 2010, Junio C Hamano wrote:
>   > I was not worried about what your change does.  I am worried about
>   > protecting what the code after your change currently does from future
>   > changes done by other people while you are not actively watching the
>   > patches in flight on this list.
>
> But that can easly be with the patch below.

What does that buy us? "future changes" can revert your patch to builtin-log.c easily.

I was talking about having tests in the test suite; any "future changes" that breaks the output your series has established would not pass if you specify what the expected output should be there. Of course they can change the test pattern in their patches, but then we will immediately know they are changing the output.

Steven Drake· Feb 26, 2010, 00:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add `[decorate]' configuration section.

On Thu, 25 Feb 2010, Junio C Hamano wrote:
Show 18 quoted lines
> Steven Drake <sdrake@xnet.co.nz> writes:
> 
> >   On Tue, 16 Feb 2010, Junio C Hamano wrote:
> >   > I was not worried about what your change does.  I am worried about
> >   > protecting what the code after your change currently does from future
> >   > changes done by other people while you are not actively watching the
> >   > patches in flight on this list.
> >
> > But that can easly be with the patch below.
> 
> What does that buy us?  "future changes" can revert your patch to
> builtin-log.c easily.
> 
> I was talking about having tests in the test suite; any "future changes"
> that breaks the output your series has established would not pass if you
> specify what the expected output should be there.  Of course they can
> change the test pattern in their patches, but then we will immediately
> know they are changing the output.
Sorry I didn't get what you meant!  I'll try writing a test.
-- 
Steven
Steven Drake· Feb 26, 2010, 03:57 UTC · re: Steven Drake · lore

Re: [PATCH] Add `[decorate]' configuration section.

On Fri, 26 Feb 2010, Steven Drake wrote:
> I'll try writing a test.
Opps, noted that you did that in the "log.decorate: usability fixes" commit.
-- 
Steven

← back to recent threads