git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] pretty format now configurable
@ 2008-02-29 19:44 Denis Cheng
  2008-02-29 20:00 ` Linus Torvalds
  2008-02-29 21:51 ` [PATCH] pretty format now configurable Johannes Schindelin
  0 siblings, 2 replies; 15+ messages in thread
From: Denis Cheng @ 2008-02-29 19:44 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

 * New configuration variable "format.pretty" can be used
    in git log/show/whathappened.

Signed-off-by: Denis Cheng <crquan@gmail.com>
---
 builtin-log.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index bbadbc0..0f7ee1f 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -20,6 +20,7 @@
 
 static int default_show_root = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
+static const char *fmt_pretty;
 
 static void add_name_decoration(const char *prefix, const char *name, struct object *obj)
 {
@@ -53,7 +54,8 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	int decorate = 0;
 
 	rev->abbrev = DEFAULT_ABBREV;
-	rev->commit_format = CMIT_FMT_DEFAULT;
+	if (fmt_pretty)
+		rev->commit_format = get_commit_format(fmt_pretty);
 	rev->verbose_header = 1;
 	DIFF_OPT_SET(&rev->diffopt, RECURSIVE);
 	rev->show_root_diff = default_show_root;
@@ -221,6 +223,12 @@ static int cmd_log_walk(struct rev_info *rev)
 
 static int git_log_config(const char *var, const char *value)
 {
+	if (!strcmp(var, "format.pretty")) {
+		if (!value)
+			config_error_nonbool(var);
+		fmt_pretty = xstrdup(value);
+		return 0;
+	}
 	if (!strcmp(var, "format.subjectprefix")) {
 		if (!value)
 			config_error_nonbool(var);
-- 
1.5.4.2


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

* Re: [PATCH] pretty format now configurable
  2008-02-29 19:44 [PATCH] pretty format now configurable Denis Cheng
@ 2008-02-29 20:00 ` Linus Torvalds
  2008-03-01  6:07   ` rae l
  2008-03-01 17:15   ` [PATCH] add pretty format configuration to git log/show/whatchanged Denis Cheng
  2008-02-29 21:51 ` [PATCH] pretty format now configurable Johannes Schindelin
  1 sibling, 2 replies; 15+ messages in thread
From: Linus Torvalds @ 2008-02-29 20:00 UTC (permalink / raw)
  To: Denis Cheng; +Cc: Junio C Hamano, git



On Sat, 1 Mar 2008, Denis Cheng wrote:
>  
>  	rev->abbrev = DEFAULT_ABBREV;
> -	rev->commit_format = CMIT_FMT_DEFAULT;
> +	if (fmt_pretty)
> +		rev->commit_format = get_commit_format(fmt_pretty);

Umm. Now it looks like commit_format isn't initialized at all if 
fmt_pretty hasn't been set.

Now, it looks like it will have been initialized properly in 
"init_revisions()", but your commit log doesn't mention that, so it was 
harder to review this patch than necessary.

Also, can you describe what the background for this is? The reason I ask 
is that if anybody ever sets that default commit format to anythign else, 
it will now *seriously* confuse not just users but potentially other git 
tools too (at least gitk uses "--pretty=raw", but who knows what other 
tools/scripts are around that just expected the default format).

		Linus

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

* Re: [PATCH] pretty format now configurable
  2008-02-29 19:44 [PATCH] pretty format now configurable Denis Cheng
  2008-02-29 20:00 ` Linus Torvalds
@ 2008-02-29 21:51 ` Johannes Schindelin
  1 sibling, 0 replies; 15+ messages in thread
From: Johannes Schindelin @ 2008-02-29 21:51 UTC (permalink / raw)
  To: Denis Cheng; +Cc: Junio C Hamano, git

Hi,

On Sat, 1 Mar 2008, Denis Cheng wrote:

> Signed-off-by: Denis Cheng <crquan@gmail.com>
> ---
>  builtin-log.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/builtin-log.c b/builtin-log.c
> index bbadbc0..0f7ee1f 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -20,6 +20,7 @@
>  
>  static int default_show_root = 1;
>  static const char *fmt_patch_subject_prefix = "PATCH";
> +static const char *fmt_pretty;

Don't you want to initialise this?

Ciao,
Dscho

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

* Re: [PATCH] pretty format now configurable
  2008-02-29 20:00 ` Linus Torvalds
@ 2008-03-01  6:07   ` rae l
  2008-03-01 17:15   ` [PATCH] add pretty format configuration to git log/show/whatchanged Denis Cheng
  1 sibling, 0 replies; 15+ messages in thread
From: rae l @ 2008-03-01  6:07 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Junio C Hamano, git

On Sat, Mar 1, 2008 at 4:00 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>  On Sat, 1 Mar 2008, Denis Cheng wrote:
>  >
>  >       rev->abbrev = DEFAULT_ABBREV;
>  > -     rev->commit_format = CMIT_FMT_DEFAULT;
>  > +     if (fmt_pretty)
>  > +             rev->commit_format = get_commit_format(fmt_pretty);
>
>  Umm. Now it looks like commit_format isn't initialized at all if
>  fmt_pretty hasn't been set.
>
>  Now, it looks like it will have been initialized properly in
>  "init_revisions()", but your commit log doesn't mention that, so it was
>  harder to review this patch than necessary.
>
>  Also, can you describe what the background for this is? The reason I ask
>  is that if anybody ever sets that default commit format to anythign else,
>  it will now *seriously* confuse not just users but potentially other git
>  tools too (at least gitk uses "--pretty=raw", but who knows what other
>  tools/scripts are around that just expected the default format).
yes, rev->commit_format has been initialized to CMIT_FMT_DEFAULT in
"init_revisions()", so the code of this patch has been working well in
my local repo,

the "format.pretty" configuration's background is that I often use
"--pretty=fuller" on my command line, and different "format:..." on my
different local repos, I hope there is a configuration to store this
to gitconfig.

now git log/show/whathappened accept pretty format in the following sort:
1. rev->commit_format set to CMIT_FMT_DEFAULT in init_revisions;
2. call to gitconfig will set fmt_pretty properly;
3. set rev->commit_format according to fmt_pretty;
4. setup_revisions will accept "--pretty=" from the command line;

so the "--pretty=" precedence is:
1. the command line "--pretty=";
2. "format.pretty" from the gitconfig;
3. default CMIT_FMT_DEFAULT;

and indeed I need to  generate a new patch including comments of this
in the source and in Documentation/*.txt; I will soon send a new
patch.

>
>                 Linus
>

--
Denis Cheng

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

* [PATCH] add pretty format configuration to git log/show/whatchanged
  2008-02-29 20:00 ` Linus Torvalds
  2008-03-01  6:07   ` rae l
@ 2008-03-01 17:15   ` Denis Cheng
  2008-03-01 18:22     ` Johannes Schindelin
  1 sibling, 1 reply; 15+ messages in thread
From: Denis Cheng @ 2008-03-01 17:15 UTC (permalink / raw)
  To: Linus Torvalds, Junio C Hamano; +Cc: git

 * New configuration variable "format.pretty" can be used
    in git log/show/whatchanged.

the "format.pretty" configuration's design background is that I often use
"--pretty=fuller" on my command line, and different "--pretty=format:..."
 on my different local repos, I hope there is a configuration to store this
to git-config.

after applying the patch, rev->commit_format in `git log/show/whatchanged`
 will be initialized in the following order:
1. call to gitconfig will set static fmt_pretty according to git-config:
   if the user never config "format.pretty", fmt_pretty doesn't need to be
   initialized;
2. rev->commit_format set to CMIT_FMT_DEFAULT in init_revisions;
3. set rev->commit_format according to fmt_pretty if the user has configured
   "format.pretty" in git-config;
4. setup_revisions will accept "--pretty=" from the command line;

so the pretty format's setting precedence is:
1. the command line "--pretty=";
2. "format.pretty" from the gitconfig;
3. default CMIT_FMT_DEFAULT;

here documentation of `git config/log/show/whatchanged` also updated.

Signed-off-by: Denis Cheng <crquan@gmail.com>
---
 Documentation/config.txt          |    5 +++++
 Documentation/git-whatchanged.txt |    9 ++++-----
 Documentation/pretty-options.txt  |    8 ++++++++
 builtin-log.c                     |   10 +++++++++-
 4 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4027726..8a0dff9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -556,6 +556,11 @@ format.suffix::
 	`.patch`. Use this variable to change that suffix (make sure to
 	include the dot if you want it).
 
+format.pretty::
+	The default pretty format for log/show/whatchanged command,
+	See linkgit:git-log[1], linkgit:git-show[1],
+	linkgit:git-whatchanged[1].
+
 gc.aggressiveWindow::
 	The window size parameter used in the delta compression
 	algorithm used by 'git gc --aggressive'.  This defaults
diff --git a/Documentation/git-whatchanged.txt b/Documentation/git-whatchanged.txt
index 54947b6..a6e7bd4 100644
--- a/Documentation/git-whatchanged.txt
+++ b/Documentation/git-whatchanged.txt
@@ -38,11 +38,6 @@ OPTIONS
 	Show git internal diff output, but for the whole tree,
 	not just the top level.
 
---pretty=<format>::
-	Controls the output format for the commit logs.
-	<format> can be one of 'raw', 'medium', 'short', 'full',
-	and 'oneline'.
-
 -m::
 	By default, differences for merge commits are not shown.
 	With this flag, show differences to that commit from all
@@ -51,6 +46,10 @@ OPTIONS
 However, it is not very useful in general, although it
 *is* useful on a file-by-file basis.
 
+include::pretty-options.txt[]
+
+include::pretty-formats.txt[]
+
 Examples
 --------
 git-whatchanged -p v2.6.12.. include/scsi drivers/scsi::
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 973d8dd..15e01fa 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -4,6 +4,14 @@
 	where '<format>' can be one of 'oneline', 'short', 'medium',
 	'full', 'fuller', 'email', 'raw' and 'format:<string>'.
 	When omitted, the format defaults to 'medium'.
++
+Note: now you can specify the default pretty format in the repository
+configuration (see linkgit:git-config[1]), like this in .git/config:
++
+-----------------------
+[format]
+	pretty = fuller
+-----------------------
 
 --abbrev-commit::
 	Instead of showing the full 40-byte hexadecimal commit object
diff --git a/builtin-log.c b/builtin-log.c
index bbadbc0..0f7ee1f 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -20,6 +20,7 @@
 
 static int default_show_root = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
+static const char *fmt_pretty;
 
 static void add_name_decoration(const char *prefix, const char *name, struct object *obj)
 {
@@ -53,7 +54,8 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	int decorate = 0;
 
 	rev->abbrev = DEFAULT_ABBREV;
-	rev->commit_format = CMIT_FMT_DEFAULT;
+	if (fmt_pretty)
+		rev->commit_format = get_commit_format(fmt_pretty);
 	rev->verbose_header = 1;
 	DIFF_OPT_SET(&rev->diffopt, RECURSIVE);
 	rev->show_root_diff = default_show_root;
@@ -221,6 +223,12 @@ static int cmd_log_walk(struct rev_info *rev)
 
 static int git_log_config(const char *var, const char *value)
 {
+	if (!strcmp(var, "format.pretty")) {
+		if (!value)
+			config_error_nonbool(var);
+		fmt_pretty = xstrdup(value);
+		return 0;
+	}
 	if (!strcmp(var, "format.subjectprefix")) {
 		if (!value)
 			config_error_nonbool(var);
-- 
1.5.4.2


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

* Re: [PATCH] add pretty format configuration to git log/show/whatchanged
  2008-03-01 17:15   ` [PATCH] add pretty format configuration to git log/show/whatchanged Denis Cheng
@ 2008-03-01 18:22     ` Johannes Schindelin
  2008-03-01 19:50       ` Denis Cheng
  0 siblings, 1 reply; 15+ messages in thread
From: Johannes Schindelin @ 2008-03-01 18:22 UTC (permalink / raw)
  To: Denis Cheng; +Cc: Linus Torvalds, Junio C Hamano, git

Hi,

On Sun, 2 Mar 2008, Denis Cheng wrote:

> diff --git a/builtin-log.c b/builtin-log.c
> index bbadbc0..0f7ee1f 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -20,6 +20,7 @@
>  
>  static int default_show_root = 1;
>  static const char *fmt_patch_subject_prefix = "PATCH";
> +static const char *fmt_pretty;

I still think this should default to CMIT_FMT_DEFAULT.

Ciao,
Dscho

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

* [PATCH] add pretty format configuration to git log/show/whatchanged
  2008-03-01 18:22     ` Johannes Schindelin
@ 2008-03-01 19:50       ` Denis Cheng
  2008-03-02  3:44         ` Junio C Hamano
  0 siblings, 1 reply; 15+ messages in thread
From: Denis Cheng @ 2008-03-01 19:50 UTC (permalink / raw)
  To: Johannes Schindelin, Junio C Hamano; +Cc: git

 * New configuration variable "format.pretty" can be used
    in git log/show/whatchanged.

the "format.pretty" configuration's design background is that I often use
"--pretty=fuller" on my command line, and different "--pretty=format:..."
 on my different local repos, I hope there is a configuration to store this
to git-config, and I think many people also need this to avoid specifying
"--pretty=..." every time.

with applying the patch, code in `git log/show/whatchanged` executed in
the following order:
1. call to gitconfig will set static fmt_pretty according to user's git-config:
   if the user never config "format.pretty", fmt_pretty doesn't need to be
   initialized;
2. rev->commit_format set to CMIT_FMT_DEFAULT in init_revisions;
3. set rev->commit_format according to fmt_pretty if the user has configured
   "format.pretty" in git-config, else default to CMIT_FMT_DEFAULT;
4. setup_revisions will accept "--pretty=" from the command line;

so the pretty format's setting precedence will be:
1. the command line "--pretty=";
2. "format.pretty" from the git-config;
3. default CMIT_FMT_DEFAULT;

here documentation of `git config/log/show/whatchanged` also updated.

Signed-off-by: Denis Cheng <crquan@gmail.com>
---
to Johannes: rev->commit_format default to CMIT_FMT_DEFAULT instead of fmt_pretty
could avoid an extra call to get_commit_format.
---
 Documentation/config.txt          |    5 +++++
 Documentation/git-whatchanged.txt |    9 ++++-----
 Documentation/pretty-options.txt  |    8 ++++++++
 builtin-log.c                     |    9 +++++++++
 4 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4027726..8a0dff9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -556,6 +556,11 @@ format.suffix::
 	`.patch`. Use this variable to change that suffix (make sure to
 	include the dot if you want it).
 
+format.pretty::
+	The default pretty format for log/show/whatchanged command,
+	See linkgit:git-log[1], linkgit:git-show[1],
+	linkgit:git-whatchanged[1].
+
 gc.aggressiveWindow::
 	The window size parameter used in the delta compression
 	algorithm used by 'git gc --aggressive'.  This defaults
diff --git a/Documentation/git-whatchanged.txt b/Documentation/git-whatchanged.txt
index 54947b6..a6e7bd4 100644
--- a/Documentation/git-whatchanged.txt
+++ b/Documentation/git-whatchanged.txt
@@ -38,11 +38,6 @@ OPTIONS
 	Show git internal diff output, but for the whole tree,
 	not just the top level.
 
---pretty=<format>::
-	Controls the output format for the commit logs.
-	<format> can be one of 'raw', 'medium', 'short', 'full',
-	and 'oneline'.
-
 -m::
 	By default, differences for merge commits are not shown.
 	With this flag, show differences to that commit from all
@@ -51,6 +46,10 @@ OPTIONS
 However, it is not very useful in general, although it
 *is* useful on a file-by-file basis.
 
+include::pretty-options.txt[]
+
+include::pretty-formats.txt[]
+
 Examples
 --------
 git-whatchanged -p v2.6.12.. include/scsi drivers/scsi::
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 973d8dd..15e01fa 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -4,6 +4,14 @@
 	where '<format>' can be one of 'oneline', 'short', 'medium',
 	'full', 'fuller', 'email', 'raw' and 'format:<string>'.
 	When omitted, the format defaults to 'medium'.
++
+Note: now you can specify the default pretty format in the repository
+configuration (see linkgit:git-config[1]), like this in .git/config:
++
+-----------------------
+[format]
+	pretty = fuller
+-----------------------
 
 --abbrev-commit::
 	Instead of showing the full 40-byte hexadecimal commit object
diff --git a/builtin-log.c b/builtin-log.c
index bbadbc0..23c05bc 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -20,6 +20,7 @@
 
 static int default_show_root = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
+static const char *fmt_pretty;
 
 static void add_name_decoration(const char *prefix, const char *name, struct object *obj)
 {
@@ -54,6 +55,8 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
+	if (fmt_pretty)
+		rev->commit_format = get_commit_format(fmt_pretty);
 	rev->verbose_header = 1;
 	DIFF_OPT_SET(&rev->diffopt, RECURSIVE);
 	rev->show_root_diff = default_show_root;
@@ -221,6 +224,12 @@ static int cmd_log_walk(struct rev_info *rev)
 
 static int git_log_config(const char *var, const char *value)
 {
+	if (!strcmp(var, "format.pretty")) {
+		if (!value)
+			config_error_nonbool(var);
+		fmt_pretty = xstrdup(value);
+		return 0;
+	}
 	if (!strcmp(var, "format.subjectprefix")) {
 		if (!value)
 			config_error_nonbool(var);
-- 
1.5.4.2


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

* Re: [PATCH] add pretty format configuration to git log/show/whatchanged
  2008-03-01 19:50       ` Denis Cheng
@ 2008-03-02  3:44         ` Junio C Hamano
  2008-03-02  6:20           ` Junio C Hamano
  2008-03-02  9:05           ` [PATCH 1/3] whatchanged documentation: share description of --pretty with others Denis Cheng
  0 siblings, 2 replies; 15+ messages in thread
From: Junio C Hamano @ 2008-03-02  3:44 UTC (permalink / raw)
  To: Denis Cheng; +Cc: Johannes Schindelin, git

Denis Cheng <crquan@gmail.com> writes:

>  * New configuration variable "format.pretty" can be used
>     in git log/show/whatchanged.
>
> the "format.pretty" configuration's design background is that I often use
> "--pretty=fuller" on my command line, and different "--pretty=format:..."
>  on my different local repos, I hope there is a configuration to store this
> to git-config, and I think many people also need this to avoid specifying
> "--pretty=..." every time.

I had to wonder what foreign project this commit log format message was
borrowed from.  also, in english, each sentence begins with a capital
letter.  not only the the first sentence ;-).

> with applying the patch, code in `git log/show/whatchanged` executed in
> the following order:
> 1. call to gitconfig will set static fmt_pretty according to user's git-config:
>    if the user never config "format.pretty", fmt_pretty doesn't need to be
>    initialized;
> 2. rev->commit_format set to CMIT_FMT_DEFAULT in init_revisions;
> 3. set rev->commit_format according to fmt_pretty if the user has configured
>    "format.pretty" in git-config, else default to CMIT_FMT_DEFAULT;
> 4. setup_revisions will accept "--pretty=" from the command line;

It is good to show that you looked at the codepath, but I think this can
go after the three-dashes line.  But what this part describes.

> so the pretty format's setting precedence will be:
> 1. the command line "--pretty=";
> 2. "format.pretty" from the git-config;
> 3. default CMIT_FMT_DEFAULT;

is a must-have in the commit log message.  The precedence order looks
sane.

By the way, I also share the concern Linus raised earlier that end-user
configuration may break existing scripts.

In-tree, there are only two callers that do not use --pretty on the
command line when calling these three commands:

 * "bisect visualize" calls "git log" when gitk is not available, with the
   user supplied formatting options.  This is very much Ok --- we actively
   want your configuration feature for this caller.

 * "git merge --squash" calls "git log" to prepare the commit message
   template.  This is _not_ Ok, and will be broken if we accept your
   patch.

So you will need a preliminary patch to "git-merge" _before_ submitting
this patch to make the latter codepath use "git log --pretty" instead.
Doing so would have raised _my_ confidence level of the patch that you
made your best effort not to introduce regression.

> diff --git a/Documentation/git-whatchanged.txt b/Documentation/git-whatchanged.txt
> index 54947b6..a6e7bd4 100644
> --- a/Documentation/git-whatchanged.txt
> +++ b/Documentation/git-whatchanged.txt
> @@ -38,11 +38,6 @@ OPTIONS
>  	Show git internal diff output, but for the whole tree,
>  	not just the top level.
>  
> ---pretty=<format>::
> -	Controls the output format for the commit logs.
> -	<format> can be one of 'raw', 'medium', 'short', 'full',
> -	and 'oneline'.
> -
>  -m::
>  	By default, differences for merge commits are not shown.
>  	With this flag, show differences to that commit from all
> @@ -51,6 +46,10 @@ OPTIONS
>  However, it is not very useful in general, although it
>  *is* useful on a file-by-file basis.
>  
> +include::pretty-options.txt[]
> +
> +include::pretty-formats.txt[]
> +
>  Examples
>  --------
>  git-whatchanged -p v2.6.12.. include/scsi drivers/scsi::

While this may be a sensible clean-up (note: I didn't actually formatted
the results and proofread it), it does not belong to your topic, does it?
It is also a preliminary clean-up before your change.

> diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
> index 973d8dd..15e01fa 100644
> --- a/Documentation/pretty-options.txt
> +++ b/Documentation/pretty-options.txt
> @@ -4,6 +4,14 @@
>  	where '<format>' can be one of 'oneline', 'short', 'medium',
>  	'full', 'fuller', 'email', 'raw' and 'format:<string>'.
>  	When omitted, the format defaults to 'medium'.
> ++
> +Note: now you can specify the default pretty format in the repository
> +configuration (see linkgit:git-config[1]), like this in .git/config:

In the commit log message, it is very sane to say "earlier we couldn't but
now we can", but in the end-user documentation we should avoid that.  The
documentation does not talk only to git old timers, but should be written
for first time readers as well.  Drop "now".

> diff --git a/builtin-log.c b/builtin-log.c
> index bbadbc0..23c05bc 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -221,6 +224,12 @@ static int cmd_log_walk(struct rev_info *rev)
>  
>  static int git_log_config(const char *var, const char *value)
>  {
> +	if (!strcmp(var, "format.pretty")) {
> +		if (!value)
> +			config_error_nonbool(var);
> +		fmt_pretty = xstrdup(value);
> +		return 0;
> +	}

These days, this can be written as:

	if (!strcmp(var, "format.pretty"))
		return git_config_string(&fmt_pretty, var, value);

Other than that, I think it is reasonably well done.

My suggestion would be to (re)do this as three series of patches:

 [1/3] whatchanged documentation: share description of --pretty with others

    The documentation had its own description for --pretty and did not
    include pretty-options/formats as documentation for other commands in
    the "log" family did.

 [2/3] merge --squash: explicitly ask for --pretty when preparing the message

    "git-merge --squash" uses "git log" when preparing the commit log
    message template without passing --pretty.

    When format.pretty configuration variable is used by the end user,
    this will result in the message template to be formatted with the
    configured format, regressing the current behaviour.

    This commit makes it explicitly ask for the default pretty format by
    passing the --pretty option when running "git log".

 [3/3] log/show/whatchanged: introduce format.pretty configuration

    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.

    The earlier patch fixed the only in-tree caller that runs these
    commands for a purpose other than showing the output directly to the
    end user (the other in-tree caller is "git bisect visualize", whose
    output directly goes to the end user and should be affected by this
    patch).  Similar fixes will be needed for end-user scripts that expect
    the output from these commands to be in the default pretty format
    (i.e. for the purpose of parsing it themselves).

Thanks.

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

* Re: [PATCH] add pretty format configuration to git log/show/whatchanged
  2008-03-02  3:44         ` Junio C Hamano
@ 2008-03-02  6:20           ` Junio C Hamano
  2008-03-02  9:05           ` [PATCH 1/3] whatchanged documentation: share description of --pretty with others Denis Cheng
  1 sibling, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2008-03-02  6:20 UTC (permalink / raw)
  To: Denis Cheng; +Cc: Johannes Schindelin, git

Junio C Hamano <gitster@pobox.com> writes:

> In-tree, there are only two callers that do not use --pretty on the
> command line when calling these three commands:
>
>  * "bisect visualize" calls "git log" when gitk is not available, with the
>    user supplied formatting options.  This is very much Ok --- we actively
>    want your configuration feature for this caller.
>
>  * "git merge --squash" calls "git log" to prepare the commit message
>    template.  This is _not_ Ok, and will be broken if we accept your
>    patch.
>
> So you will need a preliminary patch to "git-merge" _before_ submitting
> this patch to make the latter codepath use "git log --pretty" instead.
> Doing so would have raised _my_ confidence level of the patch that you
> made your best effort not to introduce regression.

I should point out that I did not look at things outside shell scripts,
i.e. stuff I do not consider really the core part of the system.  I did
not look at contrib/ area either.

I suspect the following have calls to show/log/whatchanged and do expect
to read the default format out of them, and can be broken by your patch:

 * contrib/emacs/git.el (git-setup-commit-buffer)
 * git-cvsserver.perl (sub update)

There also is a call to git log, but I think it is Ok.

 * contrib/hooks/post-receive (generate_general_email)

You should not blindly trust nor take the above as an exhaustive list; it
is just from my quick survey.

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

* [PATCH 1/3] whatchanged documentation: share description of --pretty with others
  2008-03-02  3:44         ` Junio C Hamano
  2008-03-02  6:20           ` Junio C Hamano
@ 2008-03-02  9:05           ` Denis Cheng
  2008-03-02  9:05             ` [PATCH 2/3] specify explicit "--pretty=medium" with `git log/show/whatchanged` Denis Cheng
  1 sibling, 1 reply; 15+ messages in thread
From: Denis Cheng @ 2008-03-02  9:05 UTC (permalink / raw)
  To: Junio C Hamano, Linus Torvalds, Johannes Schindelin; +Cc: git

The documentation had its own description for --pretty and did not
include pretty-options/formats as documentation for other commands in
the "log" family did.

Signed-off-by: Denis Cheng <crquan@gmail.com>
---
 Documentation/git-whatchanged.txt |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-whatchanged.txt b/Documentation/git-whatchanged.txt
index 54947b6..a6e7bd4 100644
--- a/Documentation/git-whatchanged.txt
+++ b/Documentation/git-whatchanged.txt
@@ -38,11 +38,6 @@ OPTIONS
 	Show git internal diff output, but for the whole tree,
 	not just the top level.
 
---pretty=<format>::
-	Controls the output format for the commit logs.
-	<format> can be one of 'raw', 'medium', 'short', 'full',
-	and 'oneline'.

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

* [PATCH 2/3] specify explicit "--pretty=medium" with `git log/show/whatchanged`
  2008-03-02  9:05           ` [PATCH 1/3] whatchanged documentation: share description of --pretty with others Denis Cheng
@ 2008-03-02  9:05             ` Denis Cheng
  2008-03-02  9:05               ` [PATCH 3/3] log/show/whatchanged: introduce format.pretty configuration Denis Cheng
  2008-03-02 17:00               ` [PATCH 2/3] specify explicit "--pretty=medium" with `git log/show/whatchanged` Junio C Hamano
  0 siblings, 2 replies; 15+ messages in thread
From: Denis Cheng @ 2008-03-02  9:05 UTC (permalink / raw)
  To: Junio C Hamano, Linus Torvalds, Johannes Schindelin; +Cc: git

The following patch will introduce a new configuration variable,
"format.pretty", from then on the pretty format without specifying
"--pretty" might not be the default "--pretty=medium", it depends on
the user's config. So all kinds of Shell/Perl/Emacs scripts that needs
the default medium pretty format must specify it explicitly.

Signed-off-by: Denis Cheng <crquan@gmail.com>
---
 contrib/emacs/git.el             |    2 +-
 contrib/hooks/post-receive-email |    2 +-
 git-cvsserver.perl               |    2 +-
 git-merge.sh                     |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/emacs/git.el b/contrib/emacs/git.el
index c926823..4fa853f 100644
--- a/contrib/emacs/git.el
+++ b/contrib/emacs/git.el
@@ -1299,7 +1299,7 @@ Return the list of files that haven't been handled."
   (let (author-name author-email subject date msg)
     (with-temp-buffer
       (let ((coding-system (git-get-logoutput-coding-system)))
-        (git-call-process-env t nil "log" "-1" commit)
+        (git-call-process-env t nil "log" "-1" "--pretty=medium" commit)
         (goto-char (point-min))
         (when (re-search-forward "^Author: *\\(.*\\) <\\(.*\\)>$" nil t)
           (setq author-name (match-string 1))
diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index 77c88eb..62a740c 100644
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -567,7 +567,7 @@ generate_general_email()
 	echo ""
 	if [ "$newrev_type" = "commit" ]; then
 		echo $LOGBEGIN
-		git show --no-color --root -s $newrev
+		git show --no-color --root -s --pretty=medium $newrev
 		echo $LOGEND
 	else
 		# What can we do here?  The tag marks an object that is not
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index afe3d0b..7f632af 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -2556,7 +2556,7 @@ sub update
                     if ($base) {
                         my @merged;
                         # print "want to log between  $base $parent \n";
-                        open(GITLOG, '-|', 'git-log', "$base..$parent")
+                        open(GITLOG, '-|', 'git-log', '--pretty=medium', "$base..$parent")
 			  or die "Cannot call git-log: $!";
                         my $mergedhash;
                         while (<GITLOG>) {
diff --git a/git-merge.sh b/git-merge.sh
index 1c123a3..39aa5f5 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -70,7 +70,7 @@ finish_up_to_date () {
 squash_message () {
 	echo Squashed commit of the following:
 	echo
-	git log --no-merges ^"$head" $remoteheads
+	git log --no-merges --pretty=medium ^"$head" $remoteheads
 }
 
 finish () {
-- 
1.5.4.3.368.g2bb0a


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

* [PATCH 3/3] log/show/whatchanged: introduce format.pretty configuration
  2008-03-02  9:05             ` [PATCH 2/3] specify explicit "--pretty=medium" with `git log/show/whatchanged` Denis Cheng
@ 2008-03-02  9:05               ` Denis Cheng
  2008-03-02 17:00                 ` Junio C Hamano
  2008-03-02 17:00               ` [PATCH 2/3] specify explicit "--pretty=medium" with `git log/show/whatchanged` Junio C Hamano
  1 sibling, 1 reply; 15+ messages in thread
From: Denis Cheng @ 2008-03-02  9:05 UTC (permalink / raw)
  To: Junio C Hamano, Linus Torvalds, Johannes Schindelin; +Cc: git

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.

The earlier patch fixed the only in-tree caller that runs these
commands for a purpose other than showing the output directly to the
end user (the other in-tree caller is "git bisect visualize", whose
output directly goes to the end user and should be affected by this
patch).

Signed-off-by: Denis Cheng <crquan@gmail.com>
---
 Documentation/config.txt         |    5 +++++
 Documentation/pretty-options.txt |    8 ++++++++
 builtin-log.c                    |    5 +++++
 3 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4027726..8a0dff9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -556,6 +556,11 @@ format.suffix::
 	`.patch`. Use this variable to change that suffix (make sure to
 	include the dot if you want it).
 
+format.pretty::
+	The default pretty format for log/show/whatchanged command,
+	See linkgit:git-log[1], linkgit:git-show[1],
+	linkgit:git-whatchanged[1].
+
 gc.aggressiveWindow::
 	The window size parameter used in the delta compression
 	algorithm used by 'git gc --aggressive'.  This defaults
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 973d8dd..f86b0cc 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -4,6 +4,14 @@
 	where '<format>' can be one of 'oneline', 'short', 'medium',
 	'full', 'fuller', 'email', 'raw' and 'format:<string>'.
 	When omitted, the format defaults to 'medium'.
++
+Note: you can specify the default pretty format in the repository
+configuration (see linkgit:git-config[1]), like this in .git/config:
++
+-----------------------
+[format]
+	pretty = fuller
+-----------------------
 
 --abbrev-commit::
 	Instead of showing the full 40-byte hexadecimal commit object
diff --git a/builtin-log.c b/builtin-log.c
index bbadbc0..67f13ff 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -20,6 +20,7 @@
 
 static int default_show_root = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
+static const char *fmt_pretty;
 
 static void add_name_decoration(const char *prefix, const char *name, struct object *obj)
 {
@@ -54,6 +55,8 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
+	if (fmt_pretty)
+		rev->commit_format = get_commit_format(fmt_pretty);
 	rev->verbose_header = 1;
 	DIFF_OPT_SET(&rev->diffopt, RECURSIVE);
 	rev->show_root_diff = default_show_root;
@@ -221,6 +224,8 @@ static int cmd_log_walk(struct rev_info *rev)
 
 static int git_log_config(const char *var, const char *value)
 {
+	if (!strcmp(var, "format.pretty"))
+		return git_config_string(&fmt_pretty, var, value);
 	if (!strcmp(var, "format.subjectprefix")) {
 		if (!value)
 			config_error_nonbool(var);
-- 
1.5.4.3.368.g2bb0a


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

* Re: [PATCH 2/3] specify explicit "--pretty=medium" with `git log/show/whatchanged`
  2008-03-02  9:05             ` [PATCH 2/3] specify explicit "--pretty=medium" with `git log/show/whatchanged` Denis Cheng
  2008-03-02  9:05               ` [PATCH 3/3] log/show/whatchanged: introduce format.pretty configuration Denis Cheng
@ 2008-03-02 17:00               ` Junio C Hamano
  1 sibling, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2008-03-02 17:00 UTC (permalink / raw)
  To: Denis Cheng; +Cc: Linus Torvalds, Johannes Schindelin, git

Denis Cheng <crquan@gmail.com> writes:

> The following patch will introduce a new configuration variable,
> "format.pretty", from then on the pretty format without specifying
> "--pretty" might not be the default "--pretty=medium", it depends on
> the user's config. So all kinds of Shell/Perl/Emacs scripts that needs
> the default medium pretty format must specify it explicitly.
>
> Signed-off-by: Denis Cheng <crquan@gmail.com>
> ---
>  contrib/emacs/git.el             |    2 +-
>  contrib/hooks/post-receive-email |    2 +-
>  git-cvsserver.perl               |    2 +-
>  git-merge.sh                     |    2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)

I think --pretty is enough and you do not have to say --pretty=medium, but
as long as we are being explicit, we'd better be fully explicit to future
proof them.

The list of in-tree users and places match what I found with my quick
review, which hopefully means both of us did our best effort to catch
potential breakages.

Thanks.



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

* Re: [PATCH 3/3] log/show/whatchanged: introduce format.pretty configuration
  2008-03-02  9:05               ` [PATCH 3/3] log/show/whatchanged: introduce format.pretty configuration Denis Cheng
@ 2008-03-02 17:00                 ` Junio C Hamano
  2008-03-02 17:12                   ` rae l
  0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2008-03-02 17:00 UTC (permalink / raw)
  To: Denis Cheng; +Cc: Linus Torvalds, Johannes Schindelin, git

Denis Cheng <crquan@gmail.com> writes:

> 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.
>
> The earlier patch fixed the only in-tree caller that runs these
> commands for a purpose other than showing the output directly to the
> end user (the other in-tree caller is "git bisect visualize", whose
> output directly goes to the end user and should be affected by this
> patch).
>
> Signed-off-by: Denis Cheng <crquan@gmail.com>

I see you pretty much copied my suggested commit log messages except that
you dropped the warning about the need to adjust out-of-tree scripts by
end users from this one.  I however think that was the most important
part.  We need to warn our users fairly aggressively in Release Notes
about possible compatibility issues, and commit log messages are one of
the most important sources for that.

Incidentally, I noticed only one when I wrote the above but now we have
more, so "the only in-tree caller that runs" part is totally bogus.

No need to resend anything, as I can manage with these three messages.

Thanks.

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

* Re: [PATCH 3/3] log/show/whatchanged: introduce format.pretty configuration
  2008-03-02 17:00                 ` Junio C Hamano
@ 2008-03-02 17:12                   ` rae l
  0 siblings, 0 replies; 15+ messages in thread
From: rae l @ 2008-03-02 17:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Linus Torvalds, Johannes Schindelin, git

On Mon, Mar 3, 2008 at 1:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
>  I see you pretty much copied my suggested commit log messages except that
>  you dropped the warning about the need to adjust out-of-tree scripts by
>  end users from this one.  I however think that was the most important
>  part.  We need to warn our users fairly aggressively in Release Notes
>  about possible compatibility issues, and commit log messages are one of
>  the most important sources for that.
>
>  Incidentally, I noticed only one when I wrote the above but now we have
>  more, so "the only in-tree caller that runs" part is totally bogus.
>
>  No need to resend anything, as I can manage with these three messages.
Thanks. :-)

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

end of thread, other threads:[~2008-03-02 17:13 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-29 19:44 [PATCH] pretty format now configurable Denis Cheng
2008-02-29 20:00 ` Linus Torvalds
2008-03-01  6:07   ` rae l
2008-03-01 17:15   ` [PATCH] add pretty format configuration to git log/show/whatchanged Denis Cheng
2008-03-01 18:22     ` Johannes Schindelin
2008-03-01 19:50       ` Denis Cheng
2008-03-02  3:44         ` Junio C Hamano
2008-03-02  6:20           ` Junio C Hamano
2008-03-02  9:05           ` [PATCH 1/3] whatchanged documentation: share description of --pretty with others Denis Cheng
2008-03-02  9:05             ` [PATCH 2/3] specify explicit "--pretty=medium" with `git log/show/whatchanged` Denis Cheng
2008-03-02  9:05               ` [PATCH 3/3] log/show/whatchanged: introduce format.pretty configuration Denis Cheng
2008-03-02 17:00                 ` Junio C Hamano
2008-03-02 17:12                   ` rae l
2008-03-02 17:00               ` [PATCH 2/3] specify explicit "--pretty=medium" with `git log/show/whatchanged` Junio C Hamano
2008-02-29 21:51 ` [PATCH] pretty format now configurable Johannes Schindelin

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).