Re: [PATCH] add pretty format configuration to git log/show/whatchanged

Denis Cheng [off-list ref] 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.

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.

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

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.

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

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

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

Denis Cheng [off-list ref] 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 <redacted>
---
 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.

Denis Cheng [off-list ref] 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 <redacted>

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.

On Mon, Mar 3, 2008 at 1:00 AM, Junio C Hamano [off-list ref] 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. :-)