Bug in "git diff --quiet" handling.

Bug in "git diff --quiet" handling.

[Paul Gortmaker]

I came across this when I was mistakenly thinking I could use "--quiet" to
stop the output of git format-patch giving the 0001-somecommit.patch names as
it generated them.

It was then when I read the man page and found it is passed into git diff and
is supposed to disable all output.  The interesting thing, is that it only
seems to do this on every other commit it format-patches, as shown
below.

I'm assuming this is a bug, since I can't imagine what use case would have
every alternate patch being output as useful.  If I get a chance, I'll take
a look at the code and see if I can figure out what is going on, but I
figured I'd mention it 1st in case it triggered an "Oh yeah it is XYZ"
in someones memory.

Paul.

---

Script started on Mon 11 Apr 2011 04:50:22 PM EDT
paul@dv2000:~/git/git$ git --version
git version 1.7.4.4
paul@dv2000:~/git/git$ git format-patch --quiet -o foo HEAD~10..HEAD
paul@dv2000:~/git/git$ ls -l foo
total 60
-rw-r--r-- 1 paul paul 2960 2011-04-11 16:50 0001-fetch-pull-recurse-into-submodules-when-necessary.patch
-rw-r--r-- 1 paul paul    0 2011-04-11 16:50 0002-fetch-pull-Add-the-on-demand-value-to-the-recurse-su.patch
-rw-r--r-- 1 paul paul 1514 2011-04-11 16:50 0003-config-teach-the-fetch.recurseSubmodules-option-the-.patch
-rw-r--r-- 1 paul paul    0 2011-04-11 16:50 0004-Submodules-Add-on-demand-value-for-the-fetchRecurseS.patch
-rw-r--r-- 1 paul paul 1391 2011-04-11 16:50 0005-fetch-pull-Don-t-recurse-into-a-submodule-when-commi.patch
-rw-r--r-- 1 paul paul    0 2011-04-11 16:50 0006-submodule-update-Don-t-fetch-when-the-submodule-comm.patch
-rw-r--r-- 1 paul paul 1381 2011-04-11 16:50 0007-fetch-pull-Describe-recurse-submodule-restrictions-i.patch
-rw-r--r-- 1 paul paul    0 2011-04-11 16:50 0008-remote-disallow-some-nonsensical-option-combinations.patch
-rw-r--r-- 1 paul paul 3801 2011-04-11 16:50 0009-remote-separate-the-concept-of-push-and-fetch-mirror.patch
-rw-r--r-- 1 paul paul    0 2011-04-11 16:50 0010-remote-deprecate-mirror.patch
-rw-r--r-- 1 paul paul 3172 2011-04-11 16:50 0011-submodule-process-conflicting-submodules-only-once.patch
-rw-r--r-- 1 paul paul    0 2011-04-11 16:50 0012-revisions.txt-consistent-use-of-quotes.patch
-rw-r--r-- 1 paul paul 9679 2011-04-11 16:50 0013-revisions.txt-structure-with-a-labelled-list.patch
-rw-r--r-- 1 paul paul    0 2011-04-11 16:50 0014-log-cherry-pick-documentation-regression-fix.patch
-rw-r--r-- 1 paul paul 1826 2011-04-11 16:50 0015-compat-add-missing-include-sys-resource.h.patch
-rw-r--r-- 1 paul paul    0 2011-04-11 16:50 0016-pull-do-not-clobber-untracked-files-on-initial-pull.patch
-rw-r--r-- 1 paul paul 1743 2011-04-11 16:50 0017-Start-preparing-for-1.7.4.4.patch
-rw-r--r-- 1 paul paul    0 2011-04-11 16:50 0018-gitweb-Fix-parsing-of-negative-fractional-timezones-.patch
-rw-r--r-- 1 paul paul 1089 2011-04-11 16:50 0019-Documentation-trivial-grammar-fix-in-core.worktree-d.patch
-rw-r--r-- 1 paul paul    0 2011-04-11 16:50 0020-revisions.txt-language-improvements.patch
-rw-r--r-- 1 paul paul 1114 2011-04-11 16:50 0021-Git-1.7.4.4.patch
-rw-r--r-- 1 paul paul    0 2011-04-11 16:50 0022-Git-1.7.5-rc1.patch
-rw-r--r-- 1 paul paul 4996 2011-04-11 16:50 0023-git-p4-replace-each-tab-with-8-spaces-for-consistenc.patch
paul@dv2000:~/git/git$ exit
exit
Script done on Mon 11 Apr 2011 04:51:34 PM EDT

Re: Bug in "git diff --quiet" handling.

[Junio C Hamano]

Paul Gortmaker <paul.gortmaker@windriver.com> writes:

> I'm assuming this is a bug,...

Yeah, it sounds like you found an interesting one.

As far as I know, whatever "format-patch" does in response to "--quiet"
option is not a deliberate and designed behaviour, as squelching the patch
output in the context of the command does not make much sense [*1*]; the
current implementation simply writes anything off as an user error when
"format-patch --quiet" did anything "interesting" ;-).

A patch to make --quiet not to squelch the patch output, and instead
silence any progress output would be a good addition.

Thanks.

[Footnote]

*1* Also note that at least in the original design, the standard output
from "format-patch" was never meant to be squelched.  It was the only way
the calling scripts (and humans) can learn under what filenames the
patches were output, so that the command line to fire them off as e-mails
can be programatically formed without running "ls" and filtering non-patch
files manually (if you use "format-patch -o newdir" and newdir did not
have anythning in it before running the command, of course you can rely on
the output from "ls").

[PATCH] format-patch: document --quiet option

[Carlos Martín Nieto]

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---

I guess this should be squashed into the previous one. I forgot it
wasn't documented, partly because reading the commit log for
ec2956df59 (Nate Case, format-patch: Respect --quiet option) says the
man page suggests this should work.

 Documentation/git-format-patch.txt |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 9dcafc6..616726b 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -20,7 +20,7 @@ SYNOPSIS
 		   [--ignore-if-in-upstream]
 		   [--subject-prefix=Subject-Prefix]
 		   [--to=<email>] [--cc=<email>]
-		   [--cover-letter]
+		   [--cover-letter] [--quiet]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
 
@@ -192,6 +192,9 @@ will want to ensure that threading is disabled for `git send-email`.
 	filenames, use specified suffix.  A common alternative is
 	`--suffix=.txt`.  Leaving this empty will remove the `.patch`
 	suffix.
+
+--quiet::
+	Do not print the patch names to standard output.
 +
 Note that the leading character does not have to be a dot; for example,
 you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
-- 
1.7.4.2.437.g4fc7e.dirty

Re: [PATCH] format-patch: document --quiet option

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---
>
> I guess this should be squashed into the previous one. I forgot it
> wasn't documented, partly because reading the commit log for
> ec2956df59 (Nate Case, format-patch: Respect --quiet option) says the
> man page suggests this should work.

I don't think the manual page ever meant to say anything like that.  It
used to include generic "diff" options from manual pages from diff-tree
and friends, but the "output" the description refers to is the diff
output and later dropped by protecting the description in diff-options.txt
with "ifndef::git-format-patch[]" in the Documentation/ sources.

> @@ -192,6 +192,9 @@ will want to ensure that threading is disabled for `git send-email`.
>  	filenames, use specified suffix.  A common alternative is
>  	`--suffix=.txt`.  Leaving this empty will remove the `.patch`
>  	suffix.
> +
> +--quiet::
> +	Do not print the patch names to standard output.

I see "filenames" in the context and that is "generated filenames".

Be consistent and don't introduce an undefined term "patch names" here; it
will lead to confusing readers to think as if the "generated filenames"
and "patch names" are different things.

Re: [PATCH] format-patch: document --quiet option

[Junio C Hamano]

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

>> @@ -192,6 +192,9 @@ will want to ensure that threading is disabled for `git send-email`.
>>  	filenames, use specified suffix.  A common alternative is
>>  	`--suffix=.txt`.  Leaving this empty will remove the `.patch`
>>  	suffix.
>> +
>> +--quiet::
>> +	Do not print the patch names to standard output.
>
> I see "filenames" in the context and that is "generated filenames".
> ...

Also the existing "Note that ..." that followed your added lines actually
belong to the description of the previous item.  Thusly....

-- >8 --
From: Carlos Martín Nieto <cmn@elego.de>
Subject: [PATCH] format-patch: document --quiet option

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-format-patch.txt |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index a5525e9..f4e959d 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -20,7 +20,7 @@ SYNOPSIS
 		   [--ignore-if-in-upstream]
 		   [--subject-prefix=Subject-Prefix]
 		   [--to=<email>] [--cc=<email>]
-		   [--cover-letter]
+		   [--cover-letter] [--quiet]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
 
@@ -196,6 +196,9 @@ will want to ensure that threading is disabled for `git send-email`.
 Note that the leading character does not have to be a dot; for example,
 you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
 
+--quiet::
+	Do not print the names of the generated files to standard output.
+
 --no-binary::
 	Do not output contents of changes in binary files, instead
 	display a notice that those files changed.  Patches generated
-- 
1.7.5.rc1.16.g9db19

Re: [PATCH] format-patch: document --quiet option

[Carlos Martín Nieto]

On Tue, Apr 12, 2011 at 12:52:22PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> @@ -192,6 +192,9 @@ will want to ensure that threading is disabled for `git send-email`.
> >>  	filenames, use specified suffix.  A common alternative is
> >>  	`--suffix=.txt`.  Leaving this empty will remove the `.patch`
> >>  	suffix.
> >> +
> >> +--quiet::
> >> +	Do not print the patch names to standard output.
> >
> > I see "filenames" in the context and that is "generated filenames".
> > ...
> 
> Also the existing "Note that ..." that followed your added lines actually
> belong to the description of the previous item.  Thusly....
> 

 Oops, sorry. I blindly took the indentation change to mean it was
 something unrelated. Thanks for fixing it.

   cmn

[PATCH] format-patch: don't pass on the --quiet flag

[Carlos Martín Nieto]

The --quiet flag is not meant to be passed on to the diff, as the user
always wants the patches to be produced so catch it and pass it to
reopen_stdout which decides whether to print the filename or not.

Noticed by Paul Gortmaker

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
> A patch to make --quiet not to squelch the patch output, and instead
> silence any progress output would be a good addition.

Something like this? I guess the only use case would be together with
-o.

 builtin/log.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 9a15d69..1ce00ba 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -623,7 +623,7 @@ static FILE *realstdout = NULL;
 static const char *output_directory = NULL;
 static int outdir_offset;
 
-static int reopen_stdout(struct commit *commit, struct rev_info *rev)
+static int reopen_stdout(struct commit *commit, struct rev_info *rev, int quiet)
 {
 	struct strbuf filename = STRBUF_INIT;
 	int suffix_len = strlen(fmt_patch_suffix) + 1;
@@ -639,7 +639,7 @@ static int reopen_stdout(struct commit *commit, struct rev_info *rev)
 
 	get_patch_filename(commit, rev->nr, fmt_patch_suffix, &filename);
 
-	if (!DIFF_OPT_TST(&rev->diffopt, QUICK))
+	if (!quiet)
 		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
 
 	if (freopen(filename.buf, "w", stdout) == NULL)
@@ -718,7 +718,8 @@ static void print_signature(void)
 static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			      int numbered, int numbered_files,
 			      struct commit *origin,
-			      int nr, struct commit **list, struct commit *head)
+			      int nr, struct commit **list, struct commit *head,
+			      int quiet)
 {
 	const char *committer;
 	const char *subject_start = NULL;
@@ -754,7 +755,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			sha1_to_hex(head->object.sha1), committer, committer);
 	}
 
-	if (!use_stdout && reopen_stdout(commit, rev))
+	if (!use_stdout && reopen_stdout(commit, rev, quiet))
 		return;
 
 	if (commit) {
@@ -995,6 +996,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	char *add_signoff = NULL;
 	struct strbuf buf = STRBUF_INIT;
 	int use_patch_format = 0;
+	int quiet = 0;
 	const struct option builtin_format_patch_options[] = {
 		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
 			    "use [PATCH n/m] even with a single patch",
@@ -1050,6 +1052,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    PARSE_OPT_OPTARG, thread_callback },
 		OPT_STRING(0, "signature", &signature, "signature",
 			    "add a signature"),
+		OPT_BOOLEAN(0, "quiet", &quiet,
+			    "don't print the patch filenames"),
 		OPT_END()
 	};
 
@@ -1259,7 +1263,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		if (thread)
 			gen_message_id(&rev, "cover");
 		make_cover_letter(&rev, use_stdout, numbered, numbered_files,
-				  origin, nr, list, head);
+				  origin, nr, list, head, quiet);
 		total++;
 		start_number--;
 	}
@@ -1305,7 +1309,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!use_stdout && reopen_stdout(numbered_files ? NULL : commit,
-						 &rev))
+						 &rev, quiet))
 			die("Failed to create output files");
 		shown = log_tree_commit(&rev, commit);
 		free(commit->buffer);
-- 
1.7.4.2.437.g4fc7e.dirty

Re: [PATCH] format-patch: don't pass on the --quiet flag

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

>> A patch to make --quiet not to squelch the patch output, and instead
>> silence any progress output would be a good addition.
>
> Something like this? I guess the only use case would be together with
> -o.

When the user gives -q without giving -o to a new or an empty directory,
the user deserves to get what was asked on the command line, so I wouldn't
worry about this particular case.  For a casual user, it is perfectly a
sensible thing to say "I'll eyeball; I don't have other files whose names
begin with [0-9]{4}- in my working tree" and I don't think we need safety
against doing that.

I however wonder if we should audit other commands in the "log" family to
see what they do when "--quiet" is given.  I know what they do currently
is whatever they happen to do for a nonsense request, and in no way is a
designed behaviour.  We simply did never think about that case.

For example, what should "git show master^2 next^2" do with "--quiet"?  Of
course the standard way to squelch diff output in the output from "show"
is to use "-s" (coming from "git diff-tree"), but giving "--quiet" should
at least be a no-op.

Re: [PATCH] format-patch: don't pass on the --quiet flag

[Carlos Martín Nieto]

On Tue, Apr 12, 2011 at 11:56:20AM -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> >> A patch to make --quiet not to squelch the patch output, and instead
> >> silence any progress output would be a good addition.
> >
> > Something like this? I guess the only use case would be together with
> > -o.
> 
> When the user gives -q without giving -o to a new or an empty directory,
> the user deserves to get what was asked on the command line, so I wouldn't
> worry about this particular case.  For a casual user, it is perfectly a
> sensible thing to say "I'll eyeball; I don't have other files whose names
> begin with [0-9]{4}- in my working tree" and I don't think we need safety
> against doing that.
> 

 Agreed.

> I however wonder if we should audit other commands in the "log" family to
> see what they do when "--quiet" is given.  I know what they do currently
> is whatever they happen to do for a nonsense request, and in no way is a
> designed behaviour.  We simply did never think about that case.
> 
> For example, what should "git show master^2 next^2" do with "--quiet"?  Of
> course the standard way to squelch diff output in the output from "show"
> is to use "-s" (coming from "git diff-tree"), but giving "--quiet" should
> at least be a no-op.
> 

 We get a similar effect to format-patch, and a line disappears

    carlos@bee:~/apps/git$ git show --oneline origin/master^2 origin/next^2
    9973d93 t2021: mark a test as fixed
    diff --git a/t/t2021-checkout-overwrite.sh b/t/t2021-checkout-overwrite.sh
    index 27db2ad..5da63e9 100755
    --- a/t/t2021-checkout-overwrite.sh
    +++ b/t/t2021-checkout-overwrite.sh
    @@ -39,7 +39,7 @@ test_expect_success SYMLINKS 'create a commit where dir a/b changed to symlink'
	[...] Diff here
    9db1941 Merge branch 'js/checkout-untracked-symlink'

and

    carlos@bee:~/apps/git$ git show --oneline --quiet origin/master^2 origin/next^2
    9973d93 t2021: mark a test as fixed

so we certainly should catch it.

I'm not so sure what we should do with it, though. We shouldn't
squelch all the output, because then it just makes the command useless
(though I guess the user asked for it in that case). Maybe making it
behave like a --no-diff option would make sense (i.e. pretend the user
passed -s) in order to make it behave like a prettier version of
rev-parse.

Looking at the other cmd_ functions in builtin/log.c I see:
 - reflog ignores it
 - cherry complains that it doesn't know about the option
 - log -p --quiet is the same as log
 - whatchanged --quiet shows the same as log but skips every second
   commit
 - show --quiet skips one of the commits

and I don't see any others, so whatchanged should be tought not to
skip the commits. I'll what cmd_log does differently from
cmd_whatchanged when passing options.

   cmn

[PATCH] whatchanged: always show the header

[Carlos Martín Nieto]

If --quiet is passed and there is no patch output, log_tree_commit
will not print the log which is certainly not wanted.

Set the always_show_header option to fix this.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---

With this, "--quiet" just means the same as "-s" by telling
log_tree_commit to output it. I still haven't completely understood
what the relationship between log_tree_commit, log_tree_diff and
log_tree_diff_flush is but AFAICS sometimes one function shows the log
and sometimes the other one shows it, which I guess has to do with the
QUICK option to diff.

I'm sending this now because it's a one-liner and is probably the
correct behaviour anyway, but a more general solution would be to
convert cmd_log_init to use the option parser and catch --quiet there,
maybe even making it mean the same as -s.

 builtin/log.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 1ce00ba..b24ca8a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -322,6 +322,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.simplify_history = 0;
+	rev.always_show_header = 1;
 	memset(&opt, 0, sizeof(opt));
 	opt.def = "HEAD";
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
-- 
1.7.4.2.437.g4fc7e.dirty

Re: [PATCH] whatchanged: always show the header

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> Set the always_show_header option to fix this.

I don't think that is correct.  The command should skip empty commits, no?

I'll take a look at this later when I have time.  I plan to tag -rc2
today.

Re: [PATCH] whatchanged: always show the header

[Carlos Martín Nieto]

On Wed, Apr 13, 2011 at 08:58:58AM -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > Set the always_show_header option to fix this.
> 
> I don't think that is correct.  The command should skip empty commits, no?

Yes, from the man page, they should be skipped unless "-m" is passed.

> 
> I'll take a look at this later when I have time.  I plan to tag -rc2
> today.

Maybe unconditionally unsetting QUICK would give the desired
result. Tomorrow I should be done changing cmd_log_init to use the
parse options API so it's easier to catch "--quiet".

   cmn
 

[PATCH] log: convert to parse-options

[Carlos Martín Nieto]

Use parse-options in cmd_log_init instead of manually iterating
through them. This makes the code a bit cleaner but more importantly
allows us to catch the "--quiet" option which causes some of the
log-related commands to misbehave as it would otherwise get passed on
to the diff.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---

This "fixes" the previous --quiet effects by not letting it
through. Later we can decide to make it mean the same as -s or just
leave it like that.

 builtin/log.c |   77 ++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 9a15d69..5316be3 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -25,6 +25,7 @@ static const char *default_date_mode = NULL;
 
 static int default_show_root = 1;
 static int decoration_style;
+static int decoration_given = 0;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
@@ -49,12 +50,51 @@ static int parse_decoration_style(const char *var, const char *value)
 	return -1;
 }
 
+static int decorate_callback(const struct option *opt, const char *arg, int unset)
+{
+	if (unset) {
+		decoration_style = 0;
+		return 0;
+	}
+
+	if (arg == NULL) {
+		decoration_style = DECORATE_SHORT_REFS;
+		decoration_given = 1;
+		return 0;
+	}
+
+	/* First arg is irrelevant, as it just tries to parse arg */
+	decoration_style = parse_decoration_style("decorate", arg);
+	if (decoration_style < 0)
+		die("invalid --decorate option: %s", arg);
+
+	decoration_given = 1;
+
+	return 0;
+}
+
 static void cmd_log_init(int argc, const char **argv, const char *prefix,
 			 struct rev_info *rev, struct setup_revision_opt *opt)
 {
-	int i;
-	int decoration_given = 0;
 	struct userformat_want w;
+	int help, quiet, source;
+
+	const struct option builtin_log_options[] = {
+		OPT_BOOLEAN(0, "h", &help, "show help"),
+		OPT_BOOLEAN(0, "quiet", &quiet, "supress diff output"),
+		OPT_BOOLEAN(0, "source", &source, "show source"),
+		{ OPTION_CALLBACK, 0, "decorate", NULL, NULL, "decorate options",
+		  PARSE_OPT_OPTARG, decorate_callback},
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, builtin_log_options,
+						 builtin_log_usage,
+						 PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
+						 PARSE_OPT_KEEP_DASHDASH);
+
+	if (help)
+		usage(builtin_log_usage);
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
@@ -69,14 +109,12 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	if (default_date_mode)
 		rev->date_mode = parse_date_format(default_date_mode);
 
-	/*
-	 * Check for -h before setup_revisions(), or "git log -h" will
-	 * fail when run without a git directory.
-	 */
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
+	/* Any arguments at this point are not recognized */
+	if (argc > 1)
+		die("unrecognized argument: %s", argv[1]);
+
 	memset(&w, 0, sizeof(w));
 	userformat_find_requirements(NULL, &w);
 
@@ -92,26 +130,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		if (rev->diffopt.nr_paths != 1)
 			usage("git logs can only follow renames on one pathname at a time");
 	}
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-		if (!strcmp(arg, "--decorate")) {
-			decoration_style = DECORATE_SHORT_REFS;
-			decoration_given = 1;
-		} else if (!prefixcmp(arg, "--decorate=")) {
-			const char *v = skip_prefix(arg, "--decorate=");
-			decoration_style = parse_decoration_style(arg, v);
-			if (decoration_style < 0)
-				die("invalid --decorate option: %s", arg);
-			decoration_given = 1;
-		} else if (!strcmp(arg, "--no-decorate")) {
-			decoration_style = 0;
-		} else if (!strcmp(arg, "--source")) {
-			rev->show_source = 1;
-		} else if (!strcmp(arg, "-h")) {
-			usage(builtin_log_usage);
-		} else
-			die("unrecognized argument: %s", arg);
-	}
+
+	if (source)
+		rev->show_source = 1;
 
 	/*
 	 * defeat log.decorate configuration interacting with --pretty=raw
-- 
1.7.4.2.437.g4fc7e.dirty

Re: [PATCH] log: convert to parse-options

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> diff --git a/builtin/log.c b/builtin/log.c
> index 9a15d69..5316be3 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -25,6 +25,7 @@ static const char *default_date_mode = NULL;
>  
>  static int default_show_root = 1;
>  static int decoration_style;
> +static int decoration_given = 0;

We prefer to leave zero-initialization of statics to the linker, similarly
to how we initialize decoration_style to zero in the line above.

> +static int decorate_callback(const struct option *opt, const char *arg, int unset)
> +{
> +	if (unset) {
> +		decoration_style = 0;
> +		return 0;
> +	}

This is not a new issue, but I do not think the original code did not mean
to keep decoration_given unmodified when --no-decorate was given from the
command line.  The variable is about "did we get any --decorate related
options from the command line to override whatever log.decorate variable
says?", and when we saw --no-decorate, we did get such an override from
the command line.  We should consistently set _given variable to 1 here.

It is immaterial that it happens not to matter to the current user of the
variable that sets decoration_style to zero.  The next user of _given may
want to do other things.

> +	if (arg == NULL) {
> +		decoration_style = DECORATE_SHORT_REFS;
> +		decoration_given = 1;
> +		return 0;
> +	}
> +
> +	/* First arg is irrelevant, as it just tries to parse arg */
> +	decoration_style = parse_decoration_style("decorate", arg);

It is used in the error message in git_config_long() in the callchain from
here, primarily meant to report which configuration variable had a bad
value, so it is in no way irrelevant.  We need to say that a bad value
comes not from a configuration but from the command line; get_color() in
builtin/config.c passes "command line" for this exact reason.

>  static void cmd_log_init(int argc, const char **argv, const char *prefix,
>  			 struct rev_info *rev, struct setup_revision_opt *opt)
>  {
> -	int i;
> -	int decoration_given = 0;
>  	struct userformat_want w;
> +	int help, quiet, source;
> +
> +	const struct option builtin_log_options[] = {
> +		OPT_BOOLEAN(0, "h", &help, "show help"),
> +		OPT_BOOLEAN(0, "quiet", &quiet, "supress diff output"),
> +		OPT_BOOLEAN(0, "source", &source, "show source"),
> +		{ OPTION_CALLBACK, 0, "decorate", NULL, NULL, "decorate options",
> +		  PARSE_OPT_OPTARG, decorate_callback},
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, builtin_log_options,
> +						 builtin_log_usage,
> +						 PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
> +						 PARSE_OPT_KEEP_DASHDASH);

The 5th parameter is an array of strings terminated with a NULL element.

> +	if (help)
> +		usage(builtin_log_usage);

I think parse_options() handles -h and --help itself, so there is no
longer need for this.

How about this fix-up patch on top of your version?

 builtin/log.c |   36 ++++++++++++++----------------------
 1 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 5316be3..80766a9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -25,13 +25,15 @@ static const char *default_date_mode = NULL;
 
 static int default_show_root = 1;
 static int decoration_style;
-static int decoration_given = 0;
+static int decoration_given;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
-static const char * const builtin_log_usage =
+static const char * const builtin_log_usage[] = {
 	"git log [<options>] [<since>..<until>] [[--] <path>...]\n"
-	"   or: git show [options] <object>...";
+	"   or: git show [options] <object>...",
+	NULL
+};
 
 static int parse_decoration_style(const char *var, const char *value)
 {
@@ -52,19 +54,13 @@ static int parse_decoration_style(const char *var, const char *value)
 
 static int decorate_callback(const struct option *opt, const char *arg, int unset)
 {
-	if (unset) {
+	if (unset)
 		decoration_style = 0;
-		return 0;
-	}
-
-	if (arg == NULL) {
+	else if (arg)
+		decoration_style = parse_decoration_style("command line", arg);
+	else
 		decoration_style = DECORATE_SHORT_REFS;
-		decoration_given = 1;
-		return 0;
-	}
 
-	/* First arg is irrelevant, as it just tries to parse arg */
-	decoration_style = parse_decoration_style("decorate", arg);
 	if (decoration_style < 0)
 		die("invalid --decorate option: %s", arg);
 
@@ -77,10 +73,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 			 struct rev_info *rev, struct setup_revision_opt *opt)
 {
 	struct userformat_want w;
-	int help, quiet, source;
+	int quiet, source;
 
 	const struct option builtin_log_options[] = {
-		OPT_BOOLEAN(0, "h", &help, "show help"),
 		OPT_BOOLEAN(0, "quiet", &quiet, "supress diff output"),
 		OPT_BOOLEAN(0, "source", &source, "show source"),
 		{ OPTION_CALLBACK, 0, "decorate", NULL, NULL, "decorate options",
@@ -88,13 +83,10 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		OPT_END()
 	};
 
-	argc = parse_options(argc, argv, prefix, builtin_log_options,
-						 builtin_log_usage,
-						 PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
-						 PARSE_OPT_KEEP_DASHDASH);
-
-	if (help)
-		usage(builtin_log_usage);
+	argc = parse_options(argc, argv, prefix,
+			     builtin_log_options, builtin_log_usage,
+			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
+			     PARSE_OPT_KEEP_DASHDASH);
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;

[PATCH] log: convert to parse-options

[=?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?=]

Use parse-options in cmd_log_init instead of manually iterating
through them. This makes the code a bit cleaner but more importantly
allows us to catch the "--quiet" option which causes some of the
log-related commands to misbehave as it would otherwise get passed on
to the diff.

Also take this opportinity to add 'whatchanged' to the help output.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---

Carlos Martín Nieto <cmn@elego.de> writes:

>> diff --git a/builtin/log.c b/builtin/log.c
>> index 9a15d69..5316be3 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -25,6 +25,7 @@ static const char *default_date_mode =3D3D NULL;
>>
>>  static int default_show_root =3D3D 1;
>>  static int decoration_style;
>> +static int decoration_given =3D3D 0;
>
>We prefer to leave zero-initialization of statics to the linker, similarly
>to how we initialize decoration_style to zero in the line above.
>
>> +static int decorate_callback(const struct option *opt, const char *arg, int unset)
>> +{
>> +     if (unset) {
>> +             decoration_style = 0;
>> +             return 0;
>>+     }

> This is not a new issue, but I do not think the original code did not
> mean to keep decoration_given unmodified when --no-decorate was given
> from the command line.  The variable is about "did we get any
> --decorate related options from the command line to override whatever
> log.decorate variable says?", and when we saw --no-decorate, we did
> get such an override from the command line.  We should consistently
> set _given variable to 1 here.

> It is immaterial that it happens not to matter to the current user of the
> variable that sets decoration_style to zero.  The next user of _given may
> want to do other things.

Ok, I've changed this to use the version in your fixup patch.

> +     if (arg == NULL) {
> +             decoration_style = DECORATE_SHORT_REFS;
> +             decoration_given = 1;
> +             return 0;
> +     }
> +
> +     /* First arg is irrelevant, as it just tries to parse arg */
> +     decoration_style = parse_decoration_style("decorate", arg);

> It is used in the error message in git_config_long() in the callchain from
> here, primarily meant to report which configuration variable had a bad
> value, so it is in no way irrelevant.  We need to say that a bad value
> comes not from a configuration but from the command line; get_color() in
> builtin/config.c passes "command line" for this exact reason.
> 

git_config_long doesn't die with an error (though git_config_int does,
if git_config_long isn't successful) but returns 0 if it can't parse
the value. git_config_maybe_bool notices this and returns -1, which
parse_decoration_style interprets as "was not boolean" and tries to
match "short" or "full". If it can't, then it returns -1 and
decoration_callback dies with the error message.

Be it as it may, gt_config_long doesn't output any error message at
all, so what we pass is irrelevant, unless we want to future-proof it
because we don't trust the git_config_maybe_bool call to let us handle
the error ourselves in the future.

>>  static void cmd_log_init(int argc, const char **argv, const char *prefix,
>>                        struct rev_info *rev, struct setup_revision_opt *opt)
>>  {
>> -     int i;
>> -     int decoration_given = 0;
>>       struct userformat_want w;
>> +     int help, quiet, source;
>> +
>> +     const struct option builtin_log_options[] = {
>> +             OPT_BOOLEAN(0, "h", &help, "show help"),
>> +             OPT_BOOLEAN(0, "quiet", &quiet, "supress diff output"),
>> +             OPT_BOOLEAN(0, "source", &source, "show source"),
>> +             { OPTION_CALLBACK, 0, "decorate", NULL, NULL, "decorate options",
>> +               PARSE_OPT_OPTARG, decorate_callback},
>> +             OPT_END()
>> +     };
>> +
>> +     argc = parse_options(argc, argv, prefix, builtin_log_options,
>> +                                              builtin_log_usage,
>> +                                              PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
>> +                                              PARSE_OPT_KEEP_DASHDASH);

> The 5th parameter is an array of strings terminated with a NULL element.

Done.

>> +     if (help)
>> +             usage(builtin_log_usage);

> I think parse_options() handles -h and --help itself, so there is no
> longer need for this.

Indeed, removed.

As the help is generated from the option struct, I've changed the
comment a bit to me more helpful and I've added 'whatchanged' to the
usage message, as it looks bad if 'git whatchanged -h' doesn't tell
you about itself.

I'm not completely sure what "show source" is meant to be, I think
it's the source of merges, which could be added to that explanation, I guess.

 Documentation/git-format-patch.txt |    5 ++-
 builtin/log.c                      |   77 ++++++++++++++++++++++--------------
 2 files changed, 51 insertions(+), 31 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 9a15d69..d1b0861 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -25,12 +25,16 @@ static const char *default_date_mode = NULL;
 
 static int default_show_root = 1;
 static int decoration_style;
+static int decoration_given;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
-static const char * const builtin_log_usage =
+static const char * const builtin_log_usage[] = {
 	"git log [<options>] [<since>..<until>] [[--] <path>...]\n"
-	"   or: git show [options] <object>...";
+	"   or: git show [options] <object>...\n"
+	"   or: git whatchanged [options] <object>...",
+	NULL
+};
 
 static int parse_decoration_style(const char *var, const char *value)
 {
@@ -49,12 +53,44 @@ static int parse_decoration_style(const char *var, const char *value)
 	return -1;
 }
 
+static int decorate_callback(const struct option *opt, const char *arg, int unset)
+{
+	if (unset)
+		decoration_style = 0;
+	else if (arg)
+		decoration_style = parse_decoration_style("decorate", arg);
+	else
+		decoration_style = DECORATE_SHORT_REFS;
+
+	if (decoration_style < 0)
+		die("invalid --decorate option: %s", arg);
+
+	decoration_given = 1;
+
+	return 0;
+}
+
 static void cmd_log_init(int argc, const char **argv, const char *prefix,
 			 struct rev_info *rev, struct setup_revision_opt *opt)
 {
-	int i;
-	int decoration_given = 0;
 	struct userformat_want w;
+	int dummy, source;
+
+	/*
+	 * The 'quiet' option is a dummy no-op to stop it from propagating
+	 * to the diff option parsing.
+	 */
+	const struct option builtin_log_options[] = {
+		OPT_BOOLEAN(0, "quiet", &dummy, "no-op, provided for compatability"),
+		OPT_BOOLEAN(0, "source", &source, "show source"),
+		{ OPTION_CALLBACK, 0, "decorate", NULL, NULL, "decoration options (default: short)",
+		  PARSE_OPT_OPTARG, decorate_callback},
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, builtin_log_options, builtin_log_usage,
+						 PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
+						 PARSE_OPT_KEEP_DASHDASH);
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
@@ -69,14 +105,12 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	if (default_date_mode)
 		rev->date_mode = parse_date_format(default_date_mode);
 
-	/*
-	 * Check for -h before setup_revisions(), or "git log -h" will
-	 * fail when run without a git directory.
-	 */
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
+	/* Any arguments at this point are not recognized */
+	if (argc > 1)
+		die("unrecognized argument: %s", argv[1]);
+
 	memset(&w, 0, sizeof(w));
 	userformat_find_requirements(NULL, &w);
 
@@ -92,26 +126,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		if (rev->diffopt.nr_paths != 1)
 			usage("git logs can only follow renames on one pathname at a time");
 	}
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-		if (!strcmp(arg, "--decorate")) {
-			decoration_style = DECORATE_SHORT_REFS;
-			decoration_given = 1;
-		} else if (!prefixcmp(arg, "--decorate=")) {
-			const char *v = skip_prefix(arg, "--decorate=");
-			decoration_style = parse_decoration_style(arg, v);
-			if (decoration_style < 0)
-				die("invalid --decorate option: %s", arg);
-			decoration_given = 1;
-		} else if (!strcmp(arg, "--no-decorate")) {
-			decoration_style = 0;
-		} else if (!strcmp(arg, "--source")) {
-			rev->show_source = 1;
-		} else if (!strcmp(arg, "-h")) {
-			usage(builtin_log_usage);
-		} else
-			die("unrecognized argument: %s", arg);
-	}
+
+	if (source)
+		rev->show_source = 1;
 
 	/*
 	 * defeat log.decorate configuration interacting with --pretty=raw
-- 
1.7.4.2.437.g4fc7e.dirty

Re: [PATCH] log: convert to parse-options

[Jeff King]

On Tue, Apr 19, 2011 at 02:33:31PM +0200, =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= wrote:

> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>

This is not about your patch at all, but rather that I notice in your
"From" header that your name is doubly rfc2047-encoded. It looks like
this:

  From: =?us-ascii?B?PT9VVEYtOD9xP0Nhcmxvcz0yME1hcnQ9QzM9QURuPTIwTmlldG8/?=
    =?us-ascii?Q?=3D?= <cmn@elego.de>

which decodes to the literal string:

  =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?=

which in turn decodes again to your proper name.

We made some changes to format-patch's quoting recently, and I want to
make sure this is not a regression. Can you describe your workflow for
sending these patches? What I think probably happened is:

  1. format-patch encoded your name because of the non-ascii characters

  2. the result was fed literally into mutt via cut-and-paste or
     otherwise pulled into the editor, rather than "mutt -f patch-file".

Which is not a regression, but just an annoying behavior that has been
there for a while[1]. But I wanted to double-check.

-Peff

[1] Probably the solution is to let people with a workflow like that
tell format-patch to give them the literal utf8 instead of encoding the
header.

Re: [PATCH] log: convert to parse-options

[Carlos Martín Nieto]

On Tue, Apr 19, 2011 at 10:38:17PM -0400, Jeff King wrote:
> On Tue, Apr 19, 2011 at 02:33:31PM +0200, =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= wrote:
> 
> > Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> 
> This is not about your patch at all, but rather that I notice in your
> "From" header that your name is doubly rfc2047-encoded. It looks like
> this:
> 
>   From: =?us-ascii?B?PT9VVEYtOD9xP0Nhcmxvcz0yME1hcnQ9QzM9QURuPTIwTmlldG8/?=
>     =?us-ascii?Q?=3D?= <cmn@elego.de>
> 
> which decodes to the literal string:
> 
>   =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?=
> 
> which in turn decodes again to your proper name.
> 

Ah, so that's what been going on.

> We made some changes to format-patch's quoting recently, and I want to
> make sure this is not a regression. Can you describe your workflow for
> sending these patches? What I think probably happened is:
> 
>   1. format-patch encoded your name because of the non-ascii characters
> 
>   2. the result was fed literally into mutt via cut-and-paste or
>      otherwise pulled into the editor, rather than "mutt -f patch-file".
> 
> Which is not a regression, but just an annoying behavior that has been
> there for a while[1]. But I wanted to double-check.

As I was only sending one patch, I did what the intertubes suggested
and used "mutt -H patch-file" which I guess that's the problem. I only
noticed this because someone mentioned it, after I sent another patch.

So no regression, and a --leave-utf8-alone option would be useful here.

Cheers,
   cmn