[PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Tuncer Ayaz]

After fixing clone -q I noticed that pull -q is does not do what
it's supposed to do and implemented --quiet/--verbose by
adding it to builtin-merge and fixing two places in builtin-fetch.

I have not touched/adjusted contrib/completion/git-completion.bash
but can take a look if wanted. I think it needs one or two adjustions
anyway caused by recent --OPTIONS changes in master.

I've tested the following invocations with the below changes applied:
$ git pull
$ git pull -q
$ git pull -v

Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>
---
 Documentation/merge-options.txt |    8 ++++++
 builtin-fetch.c                 |    5 ++-
 builtin-merge.c                 |   52 +++++++++++++++++++++++---------------
 git-pull.sh                     |   10 +++++--
 4 files changed, 49 insertions(+), 26 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 007909a..427cdef 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -1,3 +1,11 @@
+-q::
+--quiet::
+	Operate quietly.
+
+-v::
+--verbose::
+	Be verbose.
+
 --stat::
 	Show a diffstat at the end of the merge. The diffstat is also
 	controlled by the configuration option merge.stat.
diff --git a/builtin-fetch.c b/builtin-fetch.c
index ee93d3a..b5c2885 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -372,12 +372,13 @@ static int store_updated_refs(const char *url,
const char *remote_name,
 				SUMMARY_WIDTH, *kind ? kind : "branch",
 				 REFCOL_WIDTH, *what ? what : "HEAD");
 		if (*note) {
-			if (!shown_url) {
+			if ((verbose || !quiet) && !shown_url) {
 				fprintf(stderr, "From %.*s\n",
 						url_len, url);
 				shown_url = 1;
 			}
-			fprintf(stderr, " %s\n", note);
+			if(verbose || !quiet)
+				fprintf(stderr, " %s\n", note);
 		}
 	}
 	fclose(fp);
diff --git a/builtin-merge.c b/builtin-merge.c
index 38266ba..1f601d4 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -44,6 +44,7 @@ static const char * const builtin_merge_usage[] = {
 static int show_diffstat = 1, option_log, squash;
 static int option_commit = 1, allow_fast_forward = 1;
 static int allow_trivial = 1, have_message;
+static int quiet, verbose;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
 static unsigned char head[20], stash[20];
@@ -101,7 +102,7 @@ static struct strategy *get_strategy(const char *name)
 			struct cmdname *ent = main_cmds.names[i];
 			for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
 				if (!strncmp(ent->name, all_strategy[j].name, ent->len)
-						&& !all_strategy[j].name[ent->len])
+					&& !all_strategy[j].name[ent->len])
 					found = 1;
 			if (!found)
 				add_cmdname(&not_strategies, ent->name, ent->len);
@@ -152,6 +153,8 @@ static int option_parse_n(const struct option *opt,
 }

 static struct option builtin_merge_options[] = {
+	OPT__QUIET(&quiet),
+	OPT__VERBOSE(&verbose),
 	{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
 		"do not show a diffstat at the end of the merge",
 		PARSE_OPT_NOARG, option_parse_n },
@@ -250,7 +253,8 @@ static void restore_state(void)
 /* This is called when no merge was necessary. */
 static void finish_up_to_date(const char *msg)
 {
-	printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
+	if(verbose || !quiet)
+		printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
 	drop_save();
 }

@@ -262,7 +266,8 @@ static void squash_message(void)
 	struct commit_list *j;
 	int fd;

-	printf("Squash commit -- not updating HEAD\n");
+	if(verbose || !quiet)
+		printf("Squash commit -- not updating HEAD\n");
 	fd = open(git_path("SQUASH_MSG"), O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
 		die("Could not write to %s", git_path("SQUASH_MSG"));
@@ -282,18 +287,20 @@ static void squash_message(void)
 	if (prepare_revision_walk(&rev))
 		die("revision walk setup failed");

-	strbuf_init(&out, 0);
-	strbuf_addstr(&out, "Squashed commit of the following:\n");
-	while ((commit = get_revision(&rev)) != NULL) {
-		strbuf_addch(&out, '\n');
-		strbuf_addf(&out, "commit %s\n",
-			sha1_to_hex(commit->object.sha1));
-		pretty_print_commit(rev.commit_format, commit, &out, rev.abbrev,
-			NULL, NULL, rev.date_mode, 0);
+	if(verbose || !quiet) {
+		strbuf_init(&out, 0);
+		strbuf_addstr(&out, "Squashed commit of the following:\n");
+		while ((commit = get_revision(&rev)) != NULL) {
+			strbuf_addch(&out, '\n');
+			strbuf_addf(&out, "commit %s\n",
+						sha1_to_hex(commit->object.sha1));
+			pretty_print_commit(rev.commit_format, commit, &out, rev.abbrev,
+								NULL, NULL, rev.date_mode, 0);
+		}
+		write(fd, out.buf, out.len);
+		close(fd);
+		strbuf_release(&out);
 	}
-	write(fd, out.buf, out.len);
-	close(fd);
-	strbuf_release(&out);
 }

 static int run_hook(const char *name)
@@ -333,14 +340,15 @@ static void finish(const unsigned char
*new_head, const char *msg)
 	if (!msg)
 		strbuf_addstr(&reflog_message, getenv("GIT_REFLOG_ACTION"));
 	else {
-		printf("%s\n", msg);
+		if(verbose || !quiet)
+			printf("%s\n", msg);
 		strbuf_addf(&reflog_message, "%s: %s",
 			getenv("GIT_REFLOG_ACTION"), msg);
 	}
 	if (squash) {
 		squash_message();
 	} else {
-		if (!merge_msg.len)
+		if ((verbose || !quiet) && !merge_msg.len)
 			printf("No merge message -- not updating HEAD\n");
 		else {
 			const char *argv_gc_auto[] = { "gc", "--auto", NULL };
@@ -877,6 +885,8 @@ int cmd_merge(int argc, const char **argv, const
char *prefix)

 	argc = parse_options(argc, argv, builtin_merge_options,
 			builtin_merge_usage, 0);
+	if(!verbose && quiet)
+		show_diffstat = 0;

 	if (squash) {
 		if (!allow_fast_forward)
@@ -1019,11 +1029,11 @@ int cmd_merge(int argc, const char **argv,
const char *prefix)
 		char hex[41];

 		strcpy(hex, find_unique_abbrev(head, DEFAULT_ABBREV));
-
-		printf("Updating %s..%s\n",
-			hex,
-			find_unique_abbrev(remoteheads->item->object.sha1,
-			DEFAULT_ABBREV));
+		if(verbose || !quiet)
+			printf("Updating %s..%s\n",
+				   hex,
+				   find_unique_abbrev(remoteheads->item->object.sha1,
+									  DEFAULT_ABBREV));
 		strbuf_init(&msg, 0);
 		strbuf_addstr(&msg, "Fast forward");
 		if (have_message)
diff --git a/git-pull.sh b/git-pull.sh
index 75c3610..d84ceb5 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -16,13 +16,17 @@ cd_to_toplevel
 test -z "$(git ls-files -u)" ||
 	die "You are in the middle of a conflicted merge."

-strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
+quiet= verbose= strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
 curr_branch=$(git symbolic-ref -q HEAD)
 curr_branch_short=$(echo "$curr_branch" | sed "s|refs/heads/||")
 rebase=$(git config --bool branch.$curr_branch_short.rebase)
 while :
 do
 	case "$1" in
+	-q|--quiet)
+		quiet=-q ;;
+	-v|--verbose)
+		verbose=-v ;;
 	-n|--no-stat|--no-summary)
 		no_stat=-n ;;
 	--stat|--summary)
@@ -121,7 +125,7 @@ test true = "$rebase" && {
 		"refs/remotes/$origin/$reflist" 2>/dev/null)"
 }
 orig_head=$(git rev-parse --verify HEAD 2>/dev/null)
-git fetch --update-head-ok "$@" || exit 1
+git fetch $verbose $quiet --update-head-ok "$@" || exit 1

 curr_head=$(git rev-parse --verify HEAD 2>/dev/null)
 if test "$curr_head" != "$orig_head"
@@ -181,5 +185,5 @@ merge_name=$(git fmt-merge-msg $log_arg
<"$GIT_DIR/FETCH_HEAD") || exit
 test true = "$rebase" &&
 	exec git-rebase $strategy_args --onto $merge_head \
 	${oldremoteref:-$merge_head}
-exec git-merge $no_stat $no_commit $squash $no_ff $log_arg $strategy_args \
+exec git-merge $quiet $verbose $no_stat $no_commit $squash $no_ff
$log_arg $strategy_args \
 	"$merge_name" HEAD $merge_head
-- 
1.6.0.2.GIT

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Shawn O. Pearce]

Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:
> After fixing clone -q I noticed that pull -q is does not do what
> it's supposed to do and implemented --quiet/--verbose by
> adding it to builtin-merge and fixing two places in builtin-fetch.
 
> diff --git a/builtin-merge.c b/builtin-merge.c
> index 38266ba..1f601d4 100644
> --- a/builtin-merge.c
> +++ b/builtin-merge.c
> @@ -101,7 +102,7 @@ static struct strategy *get_strategy(const char *name)
>  			struct cmdname *ent = main_cmds.names[i];
>  			for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
>  				if (!strncmp(ent->name, all_strategy[j].name, ent->len)
> -						&& !all_strategy[j].name[ent->len])
> +					&& !all_strategy[j].name[ent->len])

This hunk seems to just be whitespace formatting.  I'd rather not
see it in a patch that is otherwise about --quiet/--verbose changes.
One change per patch, please.  ;-)

> @@ -282,18 +287,20 @@ static void squash_message(void)
>  	if (prepare_revision_walk(&rev))
>  		die("revision walk setup failed");
> 
> -	strbuf_init(&out, 0);
> -	strbuf_addstr(&out, "Squashed commit of the following:\n");
> -	while ((commit = get_revision(&rev)) != NULL) {
> -		strbuf_addch(&out, '\n');
> -		strbuf_addf(&out, "commit %s\n",
> -			sha1_to_hex(commit->object.sha1));
> -		pretty_print_commit(rev.commit_format, commit, &out, rev.abbrev,
> -			NULL, NULL, rev.date_mode, 0);
> +	if(verbose || !quiet) {
> +		strbuf_init(&out, 0);
> +		strbuf_addstr(&out, "Squashed commit of the following:\n");
> +		while ((commit = get_revision(&rev)) != NULL) {
> +			strbuf_addch(&out, '\n');
> +			strbuf_addf(&out, "commit %s\n",
> +						sha1_to_hex(commit->object.sha1));
> +			pretty_print_commit(rev.commit_format, commit, &out, rev.abbrev,
> +								NULL, NULL, rev.date_mode, 0);
> +		}
> +		write(fd, out.buf, out.len);
> +		close(fd);
> +		strbuf_release(&out);
>  	}
> -	write(fd, out.buf, out.len);
> -	close(fd);
> -	strbuf_release(&out);

This entire hunk strikes me as being completely wrong.  The fd
we are writing to is SQUASH_MSG.  It was opened earlier in the
function and should be closed, even if we put nothing into the
file.  Your change causes --quiet to leak the file descriptor.

But even worse, I think your change causes SQUASH_MSG to lose its
entire content, which makes "git merge --quiet --squash" behave
very differently from what it does today, where it at least gives
you a summary of the commits in the SQUASH_MSG file.

IMHO this hunk shouldn't be here.

> @@ -877,6 +885,8 @@ int cmd_merge(int argc, const char **argv, const
> char *prefix)
> 
>  	argc = parse_options(argc, argv, builtin_merge_options,
>  			builtin_merge_usage, 0);
> +	if(!verbose && quiet)
> +		show_diffstat = 0;

Formatting nit, use "if (".

> @@ -1019,11 +1029,11 @@ int cmd_merge(int argc, const char **argv,
> const char *prefix)
>  		char hex[41];
> 
>  		strcpy(hex, find_unique_abbrev(head, DEFAULT_ABBREV));
> -
> -		printf("Updating %s..%s\n",
> -			hex,
> -			find_unique_abbrev(remoteheads->item->object.sha1,
> -			DEFAULT_ABBREV));
> +		if(verbose || !quiet)

Formatting nit, use "if (".

> diff --git a/git-pull.sh b/git-pull.sh
> index 75c3610..d84ceb5 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -16,13 +16,17 @@ cd_to_toplevel
>  test -z "$(git ls-files -u)" ||
>  	die "You are in the middle of a conflicted merge."
> 
> -strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
> +quiet= verbose= strategy_args= no_stat= no_commit= squash= no_ff= log_arg=

This line got a little long, maybe put the two new ones on a new
line so we don't overrun the 80 column margin and there's an easier
to read diff?

-- 
Shawn.

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Tuncer Ayaz]

On Sun, Oct 12, 2008 at 10:08 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:
>> After fixing clone -q I noticed that pull -q is does not do what
>> it's supposed to do and implemented --quiet/--verbose by
>> adding it to builtin-merge and fixing two places in builtin-fetch.
>
>> diff --git a/builtin-merge.c b/builtin-merge.c
>> index 38266ba..1f601d4 100644
>> --- a/builtin-merge.c
>> +++ b/builtin-merge.c
>> @@ -101,7 +102,7 @@ static struct strategy *get_strategy(const char *name)
>>                       struct cmdname *ent = main_cmds.names[i];
>>                       for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
>>                               if (!strncmp(ent->name, all_strategy[j].name, ent->len)
>> -                                             && !all_strategy[j].name[ent->len])
>> +                                     && !all_strategy[j].name[ent->len])
>
> This hunk seems to just be whitespace formatting.  I'd rather not
> see it in a patch that is otherwise about --quiet/--verbose changes.
> One change per patch, please.  ;-)

Hi Shawn, thanks for the quick review.

This is a problem caused by my automagic editor
configuration and an oversight. ACK.

>> @@ -282,18 +287,20 @@ static void squash_message(void)
>>       if (prepare_revision_walk(&rev))
>>               die("revision walk setup failed");
>>
>> -     strbuf_init(&out, 0);
>> -     strbuf_addstr(&out, "Squashed commit of the following:\n");
>> -     while ((commit = get_revision(&rev)) != NULL) {
>> -             strbuf_addch(&out, '\n');
>> -             strbuf_addf(&out, "commit %s\n",
>> -                     sha1_to_hex(commit->object.sha1));
>> -             pretty_print_commit(rev.commit_format, commit, &out, rev.abbrev,
>> -                     NULL, NULL, rev.date_mode, 0);
>> +     if(verbose || !quiet) {
>> +             strbuf_init(&out, 0);
>> +             strbuf_addstr(&out, "Squashed commit of the following:\n");
>> +             while ((commit = get_revision(&rev)) != NULL) {
>> +                     strbuf_addch(&out, '\n');
>> +                     strbuf_addf(&out, "commit %s\n",
>> +                                             sha1_to_hex(commit->object.sha1));
>> +                     pretty_print_commit(rev.commit_format, commit, &out, rev.abbrev,
>> +                                                             NULL, NULL, rev.date_mode, 0);
>> +             }
>> +             write(fd, out.buf, out.len);
>> +             close(fd);
>> +             strbuf_release(&out);
>>       }
>> -     write(fd, out.buf, out.len);
>> -     close(fd);
>> -     strbuf_release(&out);
>
> This entire hunk strikes me as being completely wrong.  The fd
> we are writing to is SQUASH_MSG.  It was opened earlier in the
> function and should be closed, even if we put nothing into the
> file.  Your change causes --quiet to leak the file descriptor.
>
> But even worse, I think your change causes SQUASH_MSG to lose its
> entire content, which makes "git merge --quiet --squash" behave
> very differently from what it does today, where it at least gives
> you a summary of the commits in the SQUASH_MSG file.
>
> IMHO this hunk shouldn't be here.

You are right, I was not 100% sure whether the assembled
message is only displayed or possibly also stored somewhere.
What would be the right place to quieten this one?
I should have tagged the mail as an RFC :-).

>> @@ -877,6 +885,8 @@ int cmd_merge(int argc, const char **argv, const
>> char *prefix)
>>
>>       argc = parse_options(argc, argv, builtin_merge_options,
>>                       builtin_merge_usage, 0);
>> +     if(!verbose && quiet)
>> +             show_diffstat = 0;
>
> Formatting nit, use "if (".

ACK.

>> @@ -1019,11 +1029,11 @@ int cmd_merge(int argc, const char **argv,
>> const char *prefix)
>>               char hex[41];
>>
>>               strcpy(hex, find_unique_abbrev(head, DEFAULT_ABBREV));
>> -
>> -             printf("Updating %s..%s\n",
>> -                     hex,
>> -                     find_unique_abbrev(remoteheads->item->object.sha1,
>> -                     DEFAULT_ABBREV));
>> +             if(verbose || !quiet)
>
> Formatting nit, use "if (".

ACK.

>> diff --git a/git-pull.sh b/git-pull.sh
>> index 75c3610..d84ceb5 100755
>> --- a/git-pull.sh
>> +++ b/git-pull.sh
>> @@ -16,13 +16,17 @@ cd_to_toplevel
>>  test -z "$(git ls-files -u)" ||
>>       die "You are in the middle of a conflicted merge."
>>
>> -strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
>> +quiet= verbose= strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
>
> This line got a little long, maybe put the two new ones on a new
> line so we don't overrun the 80 column margin and there's an easier
> to read diff?

ACK.

I will fix the obvious and see what I can come up with for the
pretty_print hunk.

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Tuncer Ayaz]

On Sun, Oct 12, 2008 at 10:08 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:

<snip>

I've incorporated all defects as noticed by Shawn and
thanks to him found out that the changes in
squash_message() are definitely not needed and are
more of a left-over of my tryout/devel session.
Moreover I've avoided further whitespace changes.

---

>From 31fd16eccf2df334aef8fb59d4e48cf97cca93eb Mon Sep 17 00:00:00 2001
From: Tuncer Ayaz <tuncer.ayaz@gmail.com>
Date: Sun, 12 Oct 2008 23:27:14 +0200
Subject: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>
---
 Documentation/merge-options.txt |    8 ++++++++
 builtin-fetch.c                 |    5 +++--
 builtin-merge.c                 |   22 +++++++++++++++-------
 git-pull.sh                     |   10 ++++++++--
 4 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 007909a..427cdef 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -1,3 +1,11 @@
+-q::
+--quiet::
+	Operate quietly.
+
+-v::
+--verbose::
+	Be verbose.
+
 --stat::
 	Show a diffstat at the end of the merge. The diffstat is also
 	controlled by the configuration option merge.stat.
diff --git a/builtin-fetch.c b/builtin-fetch.c
index ee93d3a..287ce33 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -372,12 +372,13 @@ static int store_updated_refs(const char *url,
const char *remote_name,
 				SUMMARY_WIDTH, *kind ? kind : "branch",
 				 REFCOL_WIDTH, *what ? what : "HEAD");
 		if (*note) {
-			if (!shown_url) {
+			if ((verbose || !quiet) && !shown_url) {
 				fprintf(stderr, "From %.*s\n",
 						url_len, url);
 				shown_url = 1;
 			}
-			fprintf(stderr, " %s\n", note);
+			if (verbose || !quiet)
+				fprintf(stderr, " %s\n", note);
 		}
 	}
 	fclose(fp);
diff --git a/builtin-merge.c b/builtin-merge.c
index 38266ba..dc12a2a 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -44,6 +44,7 @@ static const char * const builtin_merge_usage[] = {
 static int show_diffstat = 1, option_log, squash;
 static int option_commit = 1, allow_fast_forward = 1;
 static int allow_trivial = 1, have_message;
+static int quiet, verbose;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
 static unsigned char head[20], stash[20];
@@ -152,6 +153,8 @@ static int option_parse_n(const struct option *opt,
 }

 static struct option builtin_merge_options[] = {
+	OPT__QUIET(&quiet),
+	OPT__VERBOSE(&verbose),
 	{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
 		"do not show a diffstat at the end of the merge",
 		PARSE_OPT_NOARG, option_parse_n },
@@ -250,7 +253,8 @@ static void restore_state(void)
 /* This is called when no merge was necessary. */
 static void finish_up_to_date(const char *msg)
 {
-	printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
+	if (verbose || !quiet)
+		printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
 	drop_save();
 }

@@ -333,14 +337,15 @@ static void finish(const unsigned char
*new_head, const char *msg)
 	if (!msg)
 		strbuf_addstr(&reflog_message, getenv("GIT_REFLOG_ACTION"));
 	else {
-		printf("%s\n", msg);
+		if (verbose || !quiet)
+			printf("%s\n", msg);
 		strbuf_addf(&reflog_message, "%s: %s",
 			getenv("GIT_REFLOG_ACTION"), msg);
 	}
 	if (squash) {
 		squash_message();
 	} else {
-		if (!merge_msg.len)
+		if ((verbose || !quiet) && !merge_msg.len)
 			printf("No merge message -- not updating HEAD\n");
 		else {
 			const char *argv_gc_auto[] = { "gc", "--auto", NULL };
@@ -877,6 +882,8 @@ int cmd_merge(int argc, const char **argv, const
char *prefix)

 	argc = parse_options(argc, argv, builtin_merge_options,
 			builtin_merge_usage, 0);
+	if (!verbose && quiet)
+		show_diffstat = 0;

 	if (squash) {
 		if (!allow_fast_forward)
@@ -1020,10 +1027,11 @@ int cmd_merge(int argc, const char **argv,
const char *prefix)

 		strcpy(hex, find_unique_abbrev(head, DEFAULT_ABBREV));

-		printf("Updating %s..%s\n",
-			hex,
-			find_unique_abbrev(remoteheads->item->object.sha1,
-			DEFAULT_ABBREV));
+		if (verbose || !quiet)
+			printf("Updating %s..%s\n",
+				hex,
+				find_unique_abbrev(remoteheads->item->object.sha1,
+				DEFAULT_ABBREV));
 		strbuf_init(&msg, 0);
 		strbuf_addstr(&msg, "Fast forward");
 		if (have_message)
diff --git a/git-pull.sh b/git-pull.sh
index 75c3610..8e25d44 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -16,6 +16,7 @@ cd_to_toplevel
 test -z "$(git ls-files -u)" ||
 	die "You are in the middle of a conflicted merge."

+quiet= verbose=
 strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
 curr_branch=$(git symbolic-ref -q HEAD)
 curr_branch_short=$(echo "$curr_branch" | sed "s|refs/heads/||")
@@ -23,6 +24,10 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase)
 while :
 do
 	case "$1" in
+	-q|--quiet)
+		quiet=-q ;;
+	-v|--verbose)
+		verbose=-v ;;
 	-n|--no-stat|--no-summary)
 		no_stat=-n ;;
 	--stat|--summary)
@@ -121,7 +126,7 @@ test true = "$rebase" && {
 		"refs/remotes/$origin/$reflist" 2>/dev/null)"
 }
 orig_head=$(git rev-parse --verify HEAD 2>/dev/null)
-git fetch --update-head-ok "$@" || exit 1
+git fetch $verbose $quiet --update-head-ok "$@" || exit 1

 curr_head=$(git rev-parse --verify HEAD 2>/dev/null)
 if test "$curr_head" != "$orig_head"
@@ -181,5 +186,6 @@ merge_name=$(git fmt-merge-msg $log_arg
<"$GIT_DIR/FETCH_HEAD") || exit
 test true = "$rebase" &&
 	exec git-rebase $strategy_args --onto $merge_head \
 	${oldremoteref:-$merge_head}
-exec git-merge $no_stat $no_commit $squash $no_ff $log_arg $strategy_args \
+exec git-merge $quiet $verbose $no_stat $no_commit \
+	$squash $no_ff $log_arg $strategy_args \
 	"$merge_name" HEAD $merge_head
-- 
1.6.0.2.GIT

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Tuncer Ayaz]

On Sun, Oct 12, 2008 at 11:31 PM, Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:
> On Sun, Oct 12, 2008 at 10:08 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
>> Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:
>
> <snip>
>
> I've incorporated all defects as noticed by Shawn and

I did of course intend to write
s/incorporated/repaired/ :-)

> thanks to him found out that the changes in
> squash_message() are definitely not needed and are
> more of a left-over of my tryout/devel session.
> Moreover I've avoided further whitespace changes.

<snip>

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Tuncer Ayaz]

Hi Shawn and list,

I've updated the patch to current Junio master.

Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>
---
 Documentation/merge-options.txt |    8 ++++++++
 builtin-fetch.c                 |    5 +++--
 builtin-merge.c                 |   22 +++++++++++++++-------
 git-pull.sh                     |   10 ++++++++--
 4 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 007909a..427cdef 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -1,3 +1,11 @@
+-q::
+--quiet::
+	Operate quietly.
+
+-v::
+--verbose::
+	Be verbose.
+
 --stat::
 	Show a diffstat at the end of the merge. The diffstat is also
 	controlled by the configuration option merge.stat.
diff --git a/builtin-fetch.c b/builtin-fetch.c
index ee93d3a..287ce33 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -372,12 +372,13 @@ static int store_updated_refs(const char *url,
const char *remote_name,
 				SUMMARY_WIDTH, *kind ? kind : "branch",
 				 REFCOL_WIDTH, *what ? what : "HEAD");
 		if (*note) {
-			if (!shown_url) {
+			if ((verbose || !quiet) && !shown_url) {
 				fprintf(stderr, "From %.*s\n",
 						url_len, url);
 				shown_url = 1;
 			}
-			fprintf(stderr, " %s\n", note);
+			if (verbose || !quiet)
+				fprintf(stderr, " %s\n", note);
 		}
 	}
 	fclose(fp);
diff --git a/builtin-merge.c b/builtin-merge.c
index 5e2b7f1..4f90501 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -44,6 +44,7 @@ static const char * const builtin_merge_usage[] = {
 static int show_diffstat = 1, option_log, squash;
 static int option_commit = 1, allow_fast_forward = 1;
 static int allow_trivial = 1, have_message;
+static int quiet, verbose;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
 static unsigned char head[20], stash[20];
@@ -152,6 +153,8 @@ static int option_parse_n(const struct option *opt,
 }

 static struct option builtin_merge_options[] = {
+	OPT__QUIET(&quiet),
+	OPT__VERBOSE(&verbose),
 	{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
 		"do not show a diffstat at the end of the merge",
 		PARSE_OPT_NOARG, option_parse_n },
@@ -249,7 +252,8 @@ static void restore_state(void)
 /* This is called when no merge was necessary. */
 static void finish_up_to_date(const char *msg)
 {
-	printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
+	if (verbose || !quiet)
+		printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
 	drop_save();
 }

@@ -330,14 +334,15 @@ static void finish(const unsigned char
*new_head, const char *msg)
 	if (!msg)
 		strbuf_addstr(&reflog_message, getenv("GIT_REFLOG_ACTION"));
 	else {
-		printf("%s\n", msg);
+		if (verbose || !quiet)
+			printf("%s\n", msg);
 		strbuf_addf(&reflog_message, "%s: %s",
 			getenv("GIT_REFLOG_ACTION"), msg);
 	}
 	if (squash) {
 		squash_message();
 	} else {
-		if (!merge_msg.len)
+		if ((verbose || !quiet) && !merge_msg.len)
 			printf("No merge message -- not updating HEAD\n");
 		else {
 			const char *argv_gc_auto[] = { "gc", "--auto", NULL };
@@ -871,6 +876,8 @@ int cmd_merge(int argc, const char **argv, const
char *prefix)

 	argc = parse_options(argc, argv, builtin_merge_options,
 			builtin_merge_usage, 0);
+	if (!verbose && quiet)
+		show_diffstat = 0;

 	if (squash) {
 		if (!allow_fast_forward)
@@ -1012,10 +1019,11 @@ int cmd_merge(int argc, const char **argv,
const char *prefix)

 		strcpy(hex, find_unique_abbrev(head, DEFAULT_ABBREV));

-		printf("Updating %s..%s\n",
-			hex,
-			find_unique_abbrev(remoteheads->item->object.sha1,
-			DEFAULT_ABBREV));
+		if (verbose || !quiet)
+			printf("Updating %s..%s\n",
+				hex,
+				find_unique_abbrev(remoteheads->item->object.sha1,
+				DEFAULT_ABBREV));
 		strbuf_addstr(&msg, "Fast forward");
 		if (have_message)
 			strbuf_addstr(&msg,
diff --git a/git-pull.sh b/git-pull.sh
index 75c3610..8e25d44 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -16,6 +16,7 @@ cd_to_toplevel
 test -z "$(git ls-files -u)" ||
 	die "You are in the middle of a conflicted merge."

+quiet= verbose=
 strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
 curr_branch=$(git symbolic-ref -q HEAD)
 curr_branch_short=$(echo "$curr_branch" | sed "s|refs/heads/||")
@@ -23,6 +24,10 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase)
 while :
 do
 	case "$1" in
+	-q|--quiet)
+		quiet=-q ;;
+	-v|--verbose)
+		verbose=-v ;;
 	-n|--no-stat|--no-summary)
 		no_stat=-n ;;
 	--stat|--summary)
@@ -121,7 +126,7 @@ test true = "$rebase" && {
 		"refs/remotes/$origin/$reflist" 2>/dev/null)"
 }
 orig_head=$(git rev-parse --verify HEAD 2>/dev/null)
-git fetch --update-head-ok "$@" || exit 1
+git fetch $verbose $quiet --update-head-ok "$@" || exit 1

 curr_head=$(git rev-parse --verify HEAD 2>/dev/null)
 if test "$curr_head" != "$orig_head"
@@ -181,5 +186,6 @@ merge_name=$(git fmt-merge-msg $log_arg
<"$GIT_DIR/FETCH_HEAD") || exit
 test true = "$rebase" &&
 	exec git-rebase $strategy_args --onto $merge_head \
 	${oldremoteref:-$merge_head}
-exec git-merge $no_stat $no_commit $squash $no_ff $log_arg $strategy_args \
+exec git-merge $quiet $verbose $no_stat $no_commit \
+	$squash $no_ff $log_arg $strategy_args \
 	"$merge_name" HEAD $merge_head
-- 
1.6.0.2.GIT

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Shawn O. Pearce]

Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:
> Hi Shawn and list,
> 
> I've updated the patch to current Junio master.
> 
> Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>

Looks good to me.

-- 
Shawn.

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Tuncer Ayaz]

On Mon, Oct 13, 2008 at 11:06 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:
>> Hi Shawn and list,
>>
>> I've updated the patch to current Junio master.
>>
>> Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>
>
> Looks good to me.

GMail borked it by inserting some linebreaks :(
is that a problem for this single diff or should
I first resend it properly via SMTP?

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Shawn O. Pearce]

Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:
> On Mon, Oct 13, 2008 at 11:06 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> > Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:
> >> Hi Shawn and list,
> >>
> >> I've updated the patch to current Junio master.
> >>
> >> Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>
> >
> > Looks good to me.
> 
> GMail borked it by inserting some linebreaks :(
> is that a problem for this single diff or should
> I first resend it properly via SMTP?

I would resend it.  For a one line diff its easy to hand apply,
but with multiple hunks like this you don't to waste Junio's time
by trying to fix up the patch by hand.

-- 
Shawn.

[PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[tuncer.ayaz]

From: Tuncer Ayaz <tuncer.ayaz@gmail.com>

Updated patch to current Junio master.

Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>
---
 Documentation/merge-options.txt |    8 ++++++++
 builtin-fetch.c                 |    5 +++--
 builtin-merge.c                 |   22 +++++++++++++++-------
 git-pull.sh                     |   10 ++++++++--
 4 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 007909a..427cdef 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -1,3 +1,11 @@
+-q::
+--quiet::
+	Operate quietly.
+
+-v::
+--verbose::
+	Be verbose.
+
 --stat::
 	Show a diffstat at the end of the merge. The diffstat is also
 	controlled by the configuration option merge.stat.
diff --git a/builtin-fetch.c b/builtin-fetch.c
index ee93d3a..287ce33 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -372,12 +372,13 @@ static int store_updated_refs(const char *url, const char *remote_name,
 				SUMMARY_WIDTH, *kind ? kind : "branch",
 				 REFCOL_WIDTH, *what ? what : "HEAD");
 		if (*note) {
-			if (!shown_url) {
+			if ((verbose || !quiet) && !shown_url) {
 				fprintf(stderr, "From %.*s\n",
 						url_len, url);
 				shown_url = 1;
 			}
-			fprintf(stderr, " %s\n", note);
+			if (verbose || !quiet)
+				fprintf(stderr, " %s\n", note);
 		}
 	}
 	fclose(fp);
diff --git a/builtin-merge.c b/builtin-merge.c
index 5e2b7f1..4f90501 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -44,6 +44,7 @@ static const char * const builtin_merge_usage[] = {
 static int show_diffstat = 1, option_log, squash;
 static int option_commit = 1, allow_fast_forward = 1;
 static int allow_trivial = 1, have_message;
+static int quiet, verbose;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
 static unsigned char head[20], stash[20];
@@ -152,6 +153,8 @@ static int option_parse_n(const struct option *opt,
 }
 
 static struct option builtin_merge_options[] = {
+	OPT__QUIET(&quiet),
+	OPT__VERBOSE(&verbose),
 	{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
 		"do not show a diffstat at the end of the merge",
 		PARSE_OPT_NOARG, option_parse_n },
@@ -249,7 +252,8 @@ static void restore_state(void)
 /* This is called when no merge was necessary. */
 static void finish_up_to_date(const char *msg)
 {
-	printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
+	if (verbose || !quiet)
+		printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
 	drop_save();
 }
 
@@ -330,14 +334,15 @@ static void finish(const unsigned char *new_head, const char *msg)
 	if (!msg)
 		strbuf_addstr(&reflog_message, getenv("GIT_REFLOG_ACTION"));
 	else {
-		printf("%s\n", msg);
+		if (verbose || !quiet)
+			printf("%s\n", msg);
 		strbuf_addf(&reflog_message, "%s: %s",
 			getenv("GIT_REFLOG_ACTION"), msg);
 	}
 	if (squash) {
 		squash_message();
 	} else {
-		if (!merge_msg.len)
+		if ((verbose || !quiet) && !merge_msg.len)
 			printf("No merge message -- not updating HEAD\n");
 		else {
 			const char *argv_gc_auto[] = { "gc", "--auto", NULL };
@@ -871,6 +876,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, builtin_merge_options,
 			builtin_merge_usage, 0);
+	if (!verbose && quiet)
+		show_diffstat = 0;
 
 	if (squash) {
 		if (!allow_fast_forward)
@@ -1012,10 +1019,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 		strcpy(hex, find_unique_abbrev(head, DEFAULT_ABBREV));
 
-		printf("Updating %s..%s\n",
-			hex,
-			find_unique_abbrev(remoteheads->item->object.sha1,
-			DEFAULT_ABBREV));
+		if (verbose || !quiet)
+			printf("Updating %s..%s\n",
+				hex,
+				find_unique_abbrev(remoteheads->item->object.sha1,
+				DEFAULT_ABBREV));
 		strbuf_addstr(&msg, "Fast forward");
 		if (have_message)
 			strbuf_addstr(&msg,
diff --git a/git-pull.sh b/git-pull.sh
index 75c3610..8e25d44 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -16,6 +16,7 @@ cd_to_toplevel
 test -z "$(git ls-files -u)" ||
 	die "You are in the middle of a conflicted merge."
 
+quiet= verbose=
 strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
 curr_branch=$(git symbolic-ref -q HEAD)
 curr_branch_short=$(echo "$curr_branch" | sed "s|refs/heads/||")
@@ -23,6 +24,10 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase)
 while :
 do
 	case "$1" in
+	-q|--quiet)
+		quiet=-q ;;
+	-v|--verbose)
+		verbose=-v ;;
 	-n|--no-stat|--no-summary)
 		no_stat=-n ;;
 	--stat|--summary)
@@ -121,7 +126,7 @@ test true = "$rebase" && {
 		"refs/remotes/$origin/$reflist" 2>/dev/null)"
 }
 orig_head=$(git rev-parse --verify HEAD 2>/dev/null)
-git fetch --update-head-ok "$@" || exit 1
+git fetch $verbose $quiet --update-head-ok "$@" || exit 1
 
 curr_head=$(git rev-parse --verify HEAD 2>/dev/null)
 if test "$curr_head" != "$orig_head"
@@ -181,5 +186,6 @@ merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
 test true = "$rebase" &&
 	exec git-rebase $strategy_args --onto $merge_head \
 	${oldremoteref:-$merge_head}
-exec git-merge $no_stat $no_commit $squash $no_ff $log_arg $strategy_args \
+exec git-merge $quiet $verbose $no_stat $no_commit \
+	$squash $no_ff $log_arg $strategy_args \
 	"$merge_name" HEAD $merge_head
-- 
1.6.0.2.GIT

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Tuncer Ayaz]

On Mon, Oct 13, 2008 at 11:13 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:
>> On Mon, Oct 13, 2008 at 11:06 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
>> > Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:
>> >> Hi Shawn and list,
>> >>
>> >> I've updated the patch to current Junio master.
>> >>
>> >> Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>
>> >
>> > Looks good to me.
>>
>> GMail borked it by inserting some linebreaks :(
>> is that a problem for this single diff or should
>> I first resend it properly via SMTP?
>
> I would resend it.  For a one line diff its easy to hand apply,
> but with multiple hunks like this you don't to waste Junio's time
> by trying to fix up the patch by hand.

OK, configured my local setup for send-email to work
and used that.

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Junio C Hamano]

tuncer.ayaz@gmail.com writes:

> From: Tuncer Ayaz <tuncer.ayaz@gmail.com>
>
> Updated patch to current Junio master.

That's not a commit log message, is it?

> Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>
> ---
>  Documentation/merge-options.txt |    8 ++++++++
>  builtin-fetch.c                 |    5 +++--
>  builtin-merge.c                 |   22 +++++++++++++++-------
>  git-pull.sh                     |   10 ++++++++--
>  4 files changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 007909a..427cdef 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -1,3 +1,11 @@
> +-q::
> +--quiet::
> +	Operate quietly.
> +
> +-v::
> +--verbose::
> +	Be verbose.
> +
>  --stat::
>  	Show a diffstat at the end of the merge. The diffstat is also
>  	controlled by the configuration option merge.stat.
> diff --git a/builtin-fetch.c b/builtin-fetch.c
> index ee93d3a..287ce33 100644
> --- a/builtin-fetch.c
> +++ b/builtin-fetch.c
> @@ -372,12 +372,13 @@ static int store_updated_refs(const char *url, const char *remote_name,
>  				SUMMARY_WIDTH, *kind ? kind : "branch",
>  				 REFCOL_WIDTH, *what ? what : "HEAD");
>  		if (*note) {
> -			if (!shown_url) {
> +			if ((verbose || !quiet) && !shown_url) {

A pair of external verbosity flag -q and -v may be acceptable, but is it
sane to have a pair of variables in code always used like this?  In other
words, this makes me wonder if a single "verbosity level" variable that
can be set to quiet, normal and verbose would make it more readable.  For
example, this one would say:

	if (verbosity >= VERBOSITY_NORMAL && !shown_url) {
        	...
	}

Also what does your command line parsing code do when the user gives -q
and -v at the same time?  Does the last one on the command line win?
Shouldn't you instead get an error message (which of course would mean you
would need to fix the caller in git-pull.sh)?

> +			if (verbose || !quiet)
> +				fprintf(stderr, " %s\n", note);

Ditto.

> +	if (verbose || !quiet)
> +		printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);

Ditto.

> +		if (verbose || !quiet)
> +			printf("%s\n", msg);
> +		if ((verbose || !quiet) && !merge_msg.len)

Ditto.

> +	if (!verbose && quiet)
> +		show_diffstat = 0;

Hmph, ah, that's (!(verbose || !quiet)).  See the readability issue?

> +		if (verbose || !quiet)
> +			printf("Updating %s..%s\n",
> +				hex,
> +				find_unique_abbrev(remoteheads->item->object.sha1,
> +				DEFAULT_ABBREV));

Ditto.

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Tuncer Ayaz]

On Tue, Oct 14, 2008 at 12:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> tuncer.ayaz@gmail.com writes:
>
>> From: Tuncer Ayaz <tuncer.ayaz@gmail.com>
>>
>> Updated patch to current Junio master.
>
> That's not a commit log message, is it?

Sorry, I was referring to my previous post and
this was my first post via send-email.

Is it ok for me to include the log message here?

-->
After fixing clone -q I noticed that pull -q does not do what
it's supposed to do and implemented --quiet/--verbose by
adding it to builtin-merge and fixing two places in builtin-fetch.

I have not touched/adjusted contrib/completion/git-completion.bash
but can take a look if wanted. I think it already needs one or two
adjustments caused by recent --OPTIONS changes in master.

I've tested the following invocations with the below changes applied:
$ git pull
$ git pull -q
$ git pull -v
<--

is that good enough or did I miss something?

>> Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>
>> ---
>>  Documentation/merge-options.txt |    8 ++++++++
>>  builtin-fetch.c                 |    5 +++--
>>  builtin-merge.c                 |   22 +++++++++++++++-------
>>  git-pull.sh                     |   10 ++++++++--
>>  4 files changed, 34 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
>> index 007909a..427cdef 100644
>> --- a/Documentation/merge-options.txt
>> +++ b/Documentation/merge-options.txt
>> @@ -1,3 +1,11 @@
>> +-q::
>> +--quiet::
>> +     Operate quietly.
>> +
>> +-v::
>> +--verbose::
>> +     Be verbose.
>> +
>>  --stat::
>>       Show a diffstat at the end of the merge. The diffstat is also
>>       controlled by the configuration option merge.stat.
>> diff --git a/builtin-fetch.c b/builtin-fetch.c
>> index ee93d3a..287ce33 100644
>> --- a/builtin-fetch.c
>> +++ b/builtin-fetch.c
>> @@ -372,12 +372,13 @@ static int store_updated_refs(const char *url, const char *remote_name,
>>                               SUMMARY_WIDTH, *kind ? kind : "branch",
>>                                REFCOL_WIDTH, *what ? what : "HEAD");
>>               if (*note) {
>> -                     if (!shown_url) {
>> +                     if ((verbose || !quiet) && !shown_url) {
>
> A pair of external verbosity flag -q and -v may be acceptable, but is it
> sane to have a pair of variables in code always used like this?  In other
> words, this makes me wonder if a single "verbosity level" variable that
> can be set to quiet, normal and verbose would make it more readable.  For
> example, this one would say:
>
>        if (verbosity >= VERBOSITY_NORMAL && !shown_url) {
>                ...
>        }

what I would actually prefer to implement are separate
printf functions for verbose, info and error messages
and display them according to:
info: sent to ouput if verbose is set or quiet is not set
error: always sent to ouput
verbose: only sent to output if verbose is set

you could get that with your "verbosity level" solution.
to keep it simple I would avoid adding any more
levels or topics to logging and if someone really wants
to either declare trace_printf to be debug_printf
or rename it :).

if that make sense I would like to teach this to
git as a general option as far as possible and get
rid of all the if clauses in front of printf calls.

> Also what does your command line parsing code do when the user gives -q
> and -v at the same time?  Does the last one on the command line win?
> Shouldn't you instead get an error message (which of course would mean you
> would need to fix the caller in git-pull.sh)?

I thought about that also and at least in this patch
tried to handle it by ignoring quiet if verbose is set.
this may not be the logic everyone wants to have
and exclusively allowing either -q or -v makes
more sense.

>> +                     if (verbose || !quiet)
>> +                             fprintf(stderr, " %s\n", note);
>
> Ditto.
>
>> +     if (verbose || !quiet)
>> +             printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
>
> Ditto.
>
>> +             if (verbose || !quiet)
>> +                     printf("%s\n", msg);
>> +             if ((verbose || !quiet) && !merge_msg.len)
>
> Ditto.
>
>> +     if (!verbose && quiet)
>> +             show_diffstat = 0;
>
> Hmph, ah, that's (!(verbose || !quiet)).  See the readability issue?

your version is more readable. the human mind seems to
have a problem with double or triple negations :).

>> +             if (verbose || !quiet)
>> +                     printf("Updating %s..%s\n",
>> +                             hex,
>> +                             find_unique_abbrev(remoteheads->item->object.sha1,
>> +                             DEFAULT_ABBREV));
>
> Ditto.
>

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Tuncer Ayaz]

On Thu, Oct 16, 2008 at 2:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:
>
>> On Wed, Oct 15, 2008 at 9:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> "Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:
>>>
>>>> Junio, what's the status here? Do you want me to rework it
>>>> all with a new verbose/quiet log infrastructure or
>>>
>>> Not really.
>>
>> OK, when do you expect to cut 1.6.0.3? It's so simple
>> that I'd like to have it included in that revision.
>
> Hmm, I did not think this was a breakage that needs to be fixed on the
> maintenance track.  git-pull does not know -q nor -v and teaching these
> new options to the command would be a feature enhancement, which by
> definition won't be in 1.6.0.3.

It's no breakage as the options did not exist before :-), yes.

>>> Using two variables to keep track of what is conceptually a tristate
>>> (quiet, normal and verbose) is insane, and I'd like to see that insanity
>>> fixed first in the patch, regardless of an elaborate "log infrastructure"
>>> you mentioned.
>>
>> I see what you mean. What about all other modules
>> with quiet/verbose doing it with two variables?
>
> Are they broken?  If not, let's not touch them.  On the other hand, let's
> avoid adding more.

Not really. After I fixed -q in clone/fetch Miklos Vajna came up
with --verbose for clone. I just thought if we come up with a
new way to handle this we should fix it everywhere -v and
-q are available and make it consistent.

>> I guess doing it with a single variable in pull first
>> as an example is what you're after, right?
>
> I did not actually mind the ones in 'git-pull' that much, as eventually
> the script will be rewritten in C by somebody anyway.  The patch to
> builtin-fetch.c/builtin-merge.c is different, as the repetition of
> (verbose || !quiet) was quite noticeable.

I've added -v as it was added to clone in "[PATCH] Implement
git clone -v" and I wanted to be fair to the IDE developers.

Would you prefer to leave -v out?

> Why have we gone off-list, by the way?

I was not sure this is of interest to everybody.
We are now on-list again :-).

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Junio C Hamano]

"Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:

> On Thu, Oct 16, 2008 at 2:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> "Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:
>>
>>> On Wed, Oct 15, 2008 at 9:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> "Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:
>>>>
> Would you prefer to leave -v out?

Not at all.

Perhaps there is a deeper misunderstanding.

It makes perfect sense _at the end user interface level_ to have -v and -q
as two separate options, perhaps with "later one wins" semantics.  Another
possible semantics is "-q and -v are mutually incompatible", but I think
"later one wins" makes it much more usable from the end user's point of view.

The only thing I was objecting to was your repeated (verbose || !quiet)
expression in the _implementation_, which would have been much easier to
read and maintain, if it were expressed as a single variable "verbosity"
that can have one of three values.

IOW,

	static enum { QUIET, NORMAL, VERBOSE } verbosity = NORMAL;
        ...

        	if (!strcmp("--quiet", arg))
                	verbosity = QUIET;
		else if (!strcmp("--verbose", arg))
                	verbosity = VERBOSE;
		else ...

	...
                if (verbosity > QUIET)
                	print informational message;
		if (verbosity > NORMAL)
                	print verbose message;

See?

Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

[Tuncer Ayaz]

On Thu, Oct 16, 2008 at 8:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:
>
>> On Thu, Oct 16, 2008 at 2:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> "Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:
>>>
>>>> On Wed, Oct 15, 2008 at 9:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> "Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:
>>>>>
>> Would you prefer to leave -v out?
>
> Not at all.
>
> Perhaps there is a deeper misunderstanding.

Perhaps there was one :-)

> It makes perfect sense _at the end user interface level_ to have -v and -q
> as two separate options, perhaps with "later one wins" semantics.  Another
> possible semantics is "-q and -v are mutually incompatible", but I think
> "later one wins" makes it much more usable from the end user's point of view.
>
> The only thing I was objecting to was your repeated (verbose || !quiet)
> expression in the _implementation_, which would have been much easier to
> read and maintain, if it were expressed as a single variable "verbosity"
> that can have one of three values.

This leaves no space for speculation and is as clear as it gets :D

> IOW,
>
>        static enum { QUIET, NORMAL, VERBOSE } verbosity = NORMAL;
>        ...
>
>                if (!strcmp("--quiet", arg))
>                        verbosity = QUIET;
>                else if (!strcmp("--verbose", arg))
>                        verbosity = VERBOSE;
>                else ...
>
>        ...
>                if (verbosity > QUIET)
>                        print informational message;
>                if (verbosity > NORMAL)
>                        print verbose message;
>
> See?

Yeah, no problem with that.
How do you propose we should integrate this with the
existing usage of parse_options() and OPT_ macros?
I want to keep using it and not redo argv handling
from scratch in builtin-fetch/builtin-merge.