# Re: [PATCH] Teach/Fix pull/fetch -q/-v options

5 messages from 2008-10-19 to 2008-10-21. Participants: Junio C Hamano, Tuncer Ayaz.
Thread: https://gitlist.dev/t/15969

## Tuncer Ayaz, 2008-10-19 19:48

Subject: [PATCH] Teach/Fix pull/fetch -q/-v options
Message-ID: <1224445691-1366-1-git-send-email-tuncer.ayaz@gmail.com>
URL: https://gitlist.dev/e/1224445691-1366-1-git-send-email-tuncer.ayaz%40gmail.com

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

This is the next attempt trying to incorporate Junio's
suggestions and I'm not sure about the following:
1) having the same option callback function in both modules
2) my adaption of the following two lines from
builtin-fetch.c to the new verbosity option:
    if (verbosity == VERBOSE)
        transport->verbose = 1;
    if (verbosity == QUIET)
        transport->verbose = -1;
3) my usage of OPTION_CALLBACK
4) my support for overriding -q or -v
as implemented in git-pull.sh by appending
-q or -v as it is passed to the script

therefore please correct me if I did it wrong or my
cb fun has an obvious defect. it may very well
have one :)

This revision does actually override verbosity
based on what was supplied later in argv (-q or -v).

Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>
---
 Documentation/merge-options.txt |    8 +++++++
 builtin-fetch.c                 |   39 ++++++++++++++++++++++++++++---------
 builtin-merge.c                 |   40 ++++++++++++++++++++++++++++++++------
 git-pull.sh                     |   10 +++++++-
 4 files changed, 78 insertions(+), 19 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..2596aee 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -22,16 +22,35 @@ enum {
 	TAGS_SET = 2
 };
 
-static int append, force, keep, update_head_ok, verbose, quiet;
+static int append, force, keep, update_head_ok;
 static int tags = TAGS_DEFAULT;
 static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
 static struct transport *transport;
+static enum { QUIET, NORMAL, VERBOSE } verbosity = NORMAL;
+
+static int option_parse_quiet(const struct option *opt,
+			  const char *arg, int unset)
+{
+	verbosity = QUIET;
+	return 0;
+}
+
+static int option_parse_verbose(const struct option *opt,
+			  const char *arg, int unset)
+{
+	verbosity = VERBOSE;
+	return 0;
+}
 
 static struct option builtin_fetch_options[] = {
-	OPT__QUIET(&quiet),
-	OPT__VERBOSE(&verbose),
+	{ OPTION_CALLBACK, 'q', "quiet", NULL, NULL,
+		"operate quietly",
+		PARSE_OPT_NOARG, option_parse_quiet },
+	{ OPTION_CALLBACK, 'v', "verbose", NULL, NULL,
+		"be verbose",
+		PARSE_OPT_NOARG, option_parse_verbose },
 	OPT_BOOLEAN('a', "append", &append,
 		    "append to .git/FETCH_HEAD instead of overwriting"),
 	OPT_STRING(0, "upload-pack", &upload_pack, "PATH",
@@ -192,7 +211,6 @@ static int s_update_ref(const char *action,
 
 static int update_local_ref(struct ref *ref,
 			    const char *remote,
-			    int verbose,
 			    char *display)
 {
 	struct commit *current = NULL, *updated;
@@ -210,7 +228,7 @@ static int update_local_ref(struct ref *ref,
 		die("object %s not found", sha1_to_hex(ref->new_sha1));
 
 	if (!hashcmp(ref->old_sha1, ref->new_sha1)) {
-		if (verbose)
+		if (verbosity == VERBOSE)
 			sprintf(display, "= %-*s %-*s -> %s", SUMMARY_WIDTH,
 				"[up to date]", REFCOL_WIDTH, remote,
 				pretty_ref);
@@ -366,18 +384,19 @@ static int store_updated_refs(const char *url, const char *remote_name,
 			note);
 
 		if (ref)
-			rc |= update_local_ref(ref, what, verbose, note);
+			rc |= update_local_ref(ref, what, note);
 		else
 			sprintf(note, "* %-*s %-*s -> FETCH_HEAD",
 				SUMMARY_WIDTH, *kind ? kind : "branch",
 				 REFCOL_WIDTH, *what ? what : "HEAD");
 		if (*note) {
-			if (!shown_url) {
+			if (verbosity > QUIET && !shown_url) {
 				fprintf(stderr, "From %.*s\n",
 						url_len, url);
 				shown_url = 1;
 			}
-			fprintf(stderr, " %s\n", note);
+			if (verbosity > QUIET)
+				fprintf(stderr, " %s\n", note);
 		}
 	}
 	fclose(fp);
@@ -622,9 +641,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		remote = remote_get(argv[0]);
 
 	transport = transport_get(remote, remote->url[0]);
-	if (verbose >= 2)
+	if (verbosity == VERBOSE)
 		transport->verbose = 1;
-	if (quiet)
+	if (verbosity == QUIET)
 		transport->verbose = -1;
 	if (upload_pack)
 		set_option(TRANS_OPT_UPLOADPACK, upload_pack);
diff --git a/builtin-merge.c b/builtin-merge.c
index 5e2b7f1..8259acb 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -50,6 +50,7 @@ static unsigned char head[20], stash[20];
 static struct strategy **use_strategies;
 static size_t use_strategies_nr, use_strategies_alloc;
 static const char *branch;
+static enum { QUIET, NORMAL, VERBOSE } verbosity = NORMAL;
 
 static struct strategy all_strategy[] = {
 	{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -151,7 +152,27 @@ static int option_parse_n(const struct option *opt,
 	return 0;
 }
 
+static int option_parse_quiet(const struct option *opt,
+			  const char *arg, int unset)
+{
+	verbosity = QUIET;
+	return 0;
+}
+
+static int option_parse_verbose(const struct option *opt,
+			  const char *arg, int unset)
+{
+	verbosity = VERBOSE;
+	return 0;
+}
+
 static struct option builtin_merge_options[] = {
+	{ OPTION_CALLBACK, 'q', "quiet", NULL, NULL,
+		"operate quietly",
+		PARSE_OPT_NOARG, option_parse_quiet },
+	{ OPTION_CALLBACK, 'v', "verbose", NULL, NULL,
+		"be verbose",
+		PARSE_OPT_NOARG, option_parse_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 +270,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 (verbosity > QUIET)
+		printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
 	drop_save();
 }
 
@@ -330,14 +352,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 (verbosity > 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 (verbosity > QUIET && !merge_msg.len)
 			printf("No merge message -- not updating HEAD\n");
 		else {
 			const char *argv_gc_auto[] = { "gc", "--auto", NULL };
@@ -871,6 +894,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 (verbosity > QUIET)
+		show_diffstat = 0;
 
 	if (squash) {
 		if (!allow_fast_forward)
@@ -1012,10 +1037,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 (verbosity > 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..dc613db 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."
 
+verbosity=
 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)
+		verbosity="$verbosity -q" ;;
+	-v|--verbose)
+		verbosity="$verbosity -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 $verbosity --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 $verbosity $no_stat $no_commit \
+	$squash $no_ff $log_arg $strategy_args \
 	"$merge_name" HEAD $merge_head
-- 
1.6.0.2.GIT

```

## Junio C Hamano, 2008-10-19 21:26

Subject: Re: [PATCH] Teach/Fix pull/fetch -q/-v options
Message-ID: <7vy70k5las.fsf@gitster.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vy70k5las.fsf%40gitster.siamese.dyndns.org
In-Reply-To: <1224445691-1366-1-git-send-email-tuncer.ayaz@gmail.com>

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

> 2) my adaption of the following two lines from
> builtin-fetch.c to the new verbosity option:
>     if (verbosity == VERBOSE)
>         transport->verbose = 1;
>     if (verbosity == QUIET)
>         transport->verbose = -1;

Hmm, what's wrong with it?  Looks Ok to me...

>  static struct option builtin_fetch_options[] = {
> -	OPT__QUIET(&quiet),
> -	OPT__VERBOSE(&verbose),
> +	{ OPTION_CALLBACK, 'q', "quiet", NULL, NULL,
> +		"operate quietly",
> +		PARSE_OPT_NOARG, option_parse_quiet },
> +	{ OPTION_CALLBACK, 'v', "verbose", NULL, NULL,
> +		"be verbose",
> +		PARSE_OPT_NOARG, option_parse_verbose },

Isn't there a OPTION_FOO that assigns a constant to the given variable?

> @@ -192,7 +211,6 @@ static int s_update_ref(const char *action,
>  
>  static int update_local_ref(struct ref *ref,
>  			    const char *remote,
> -			    int verbose,
>  			    char *display)
>  {
>  	struct commit *current = NULL, *updated;
> ...
> @@ -366,18 +384,19 @@ static int store_updated_refs(const char *url, const char *remote_name,
>  			note);
>  
>  		if (ref)
> -			rc |= update_local_ref(ref, what, verbose, note);
> +			rc |= update_local_ref(ref, what, note);

Hmph, in the existing code, do_fetch()->fetch_refs()->store_updated_refs()
callchain relies on the "verbose" to be global anyway, so losing the
ability to call update_local_ref() with verbosity as parameter is not a
huge deal.

I however think it would be more beneficial in the longer term to keep
"verbosity" as parameter so the caller can tweak what the callee does, and
making large part of what cmd_fetch() does callable from outside.  That
would involve making the builtin_fetch_options[] on-stack, and passing
verbosity (and possibly other variables currently used as file-scope
global) as parameters, which is outside of the scope of your patch, but it
is something to keep in mind.

> diff --git a/git-pull.sh b/git-pull.sh
> index 75c3610..dc613db 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."
>  
> +verbosity=
>  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/||")

It would fit at the end of the next line just fine, wouldn't it?

> @@ -23,6 +24,10 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase)
>  while :
>  do
>  	case "$1" in
> +	-q|--quiet)
> +		verbosity="$verbosity -q" ;;
> +	-v|--verbose)
> +		verbosity="$verbosity -v" ;;

You know verbosity flags (-q and -v) are "the last one wins", so I do not
see much point in this concatenation.

```

## Tuncer Ayaz, 2008-10-20 16:35

Subject: Re: [PATCH] Teach/Fix pull/fetch -q/-v options
Message-ID: <4ac8254d0810200935sf7ad873tea53c0fb53bbe1c0@mail.gmail.com>
URL: https://gitlist.dev/e/4ac8254d0810200935sf7ad873tea53c0fb53bbe1c0%40mail.gmail.com
In-Reply-To: <7vy70k5las.fsf@gitster.siamese.dyndns.org>

```
On Sun, Oct 19, 2008 at 11:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tuncer Ayaz <tuncer.ayaz@gmail.com> writes:
>
>> 2) my adaption of the following two lines from
>> builtin-fetch.c to the new verbosity option:
>>     if (verbosity == VERBOSE)
>>         transport->verbose = 1;
>>     if (verbosity == QUIET)
>>         transport->verbose = -1;
>
> Hmm, what's wrong with it?  Looks Ok to me...

Just wanted to be sure it's correct, that's all.

Actually I think the old code:
if (verbose >= 2)
    transport->verbose = 1;
is wrong and probably a leftover from old days
as Shawn confirmed.

>>  static struct option builtin_fetch_options[] = {
>> -     OPT__QUIET(&quiet),
>> -     OPT__VERBOSE(&verbose),
>> +     { OPTION_CALLBACK, 'q', "quiet", NULL, NULL,
>> +             "operate quietly",
>> +             PARSE_OPT_NOARG, option_parse_quiet },
>> +     { OPTION_CALLBACK, 'v', "verbose", NULL, NULL,
>> +             "be verbose",
>> +             PARSE_OPT_NOARG, option_parse_verbose },
>
> Isn't there a OPTION_FOO that assigns a constant to the given variable?

Yes, there is - OPT_SET_INT and I've used that in my next patch.

>> @@ -192,7 +211,6 @@ static int s_update_ref(const char *action,
>>
>>  static int update_local_ref(struct ref *ref,
>>                           const char *remote,
>> -                         int verbose,
>>                           char *display)
>>  {
>>       struct commit *current = NULL, *updated;
>> ...
>> @@ -366,18 +384,19 @@ static int store_updated_refs(const char *url, const char
>> *remote_name,
>>                       note);
>>
>>               if (ref)
>> -                     rc |= update_local_ref(ref, what, verbose, note);
>> +                     rc |= update_local_ref(ref, what, note);
>
> Hmph, in the existing code, do_fetch()->fetch_refs()->store_updated_refs()
> callchain relies on the "verbose" to be global anyway, so losing the
> ability to call update_local_ref() with verbosity as parameter is not a
> huge deal.

OK, I've kept the removal of the verbose param in the next patch.

> I however think it would be more beneficial in the longer term to keep
> "verbosity" as parameter so the caller can tweak what the callee does, and
> making large part of what cmd_fetch() does callable from outside.  That
> would involve making the builtin_fetch_options[] on-stack, and passing
> verbosity (and possibly other variables currently used as file-scope
> global) as parameters, which is outside of the scope of your patch, but it
> is something to keep in mind.
>
>> diff --git a/git-pull.sh b/git-pull.sh
>> index 75c3610..dc613db 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."
>>
>> +verbosity=
>>  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/||")
>
> It would fit at the end of the next line just fine, wouldn't it?
>
>> @@ -23,6 +24,10 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase)
>>  while :
>>  do
>>       case "$1" in
>> +     -q|--quiet)
>> +             verbosity="$verbosity -q" ;;
>> +     -v|--verbose)
>> +             verbosity="$verbosity -v" ;;
>
> You know verbosity flags (-q and -v) are "the last one wins", so I do not
> see much point in this concatenation.

Without concatenation I would need to analyze the content
of the variable each time the option is passed to the shell
script. Do you know of a simpler/better way still keeping the
functionality that
$ git pull -q -v --quiet --verbose --quiet gives verbosity=QUIET
and
$ git pull -q -v --quiet --verbose --quiet -v yields verbosity=VERBOSE
?

```

## Junio C Hamano, 2008-10-20 23:54

Subject: Re: [PATCH] Teach/Fix pull/fetch -q/-v options
Message-ID: <7vwsg22568.fsf@gitster.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vwsg22568.fsf%40gitster.siamese.dyndns.org
In-Reply-To: <4ac8254d0810200935sf7ad873tea53c0fb53bbe1c0@mail.gmail.com>

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

> On Sun, Oct 19, 2008 at 11:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>>> @@ -23,6 +24,10 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase)
>>>  while :
>>>  do
>>>       case "$1" in
>>> +     -q|--quiet)
>>> +             verbosity="$verbosity -q" ;;
>>> +     -v|--verbose)
>>> +             verbosity="$verbosity -v" ;;
>>
>> You know verbosity flags (-q and -v) are "the last one wins", so I do not
>> see much point in this concatenation.
>
> Without concatenation I would need to analyze the content
> of the variable each time the option is passed to the shell
> script. Do you know of a simpler/better way still keeping the
> functionality that
> $ git pull -q -v --quiet --verbose --quiet gives verbosity=QUIET
> and
> $ git pull -q -v --quiet --verbose --quiet -v yields verbosity=VERBOSE
> ?

Wouldn't

	verbosity=
	while :
        do
        	case "$1" in
                -q|--quiet) verbosity=-q ;;
                -v|--verbose) verbosity=-v ;;
		... others ...
                esac
                shift
	done
        git pull $verbosity other options

give the -q for the former and -v for the latter to "git pull"?

```

## Tuncer Ayaz, 2008-10-21 16:25

Subject: Re: [PATCH] Teach/Fix pull/fetch -q/-v options
Message-ID: <4ac8254d0810210925l11b8b471p27cc06206c7b24ca@mail.gmail.com>
URL: https://gitlist.dev/e/4ac8254d0810210925l11b8b471p27cc06206c7b24ca%40mail.gmail.com
In-Reply-To: <7vwsg22568.fsf@gitster.siamese.dyndns.org>

```
On Tue, Oct 21, 2008 at 1:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:
>
>> On Sun, Oct 19, 2008 at 11:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>>> @@ -23,6 +24,10 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase)
>>>>  while :
>>>>  do
>>>>       case "$1" in
>>>> +     -q|--quiet)
>>>> +             verbosity="$verbosity -q" ;;
>>>> +     -v|--verbose)
>>>> +             verbosity="$verbosity -v" ;;
>>>
>>> You know verbosity flags (-q and -v) are "the last one wins", so I do not
>>> see much point in this concatenation.
>>
>> Without concatenation I would need to analyze the content
>> of the variable each time the option is passed to the shell
>> script. Do you know of a simpler/better way still keeping the
>> functionality that
>> $ git pull -q -v --quiet --verbose --quiet gives verbosity=QUIET
>> and
>> $ git pull -q -v --quiet --verbose --quiet -v yields verbosity=VERBOSE
>> ?
>
> Wouldn't
>
>        verbosity=
>        while :
>        do
>                case "$1" in
>                -q|--quiet) verbosity=-q ;;
>                -v|--verbose) verbosity=-v ;;
>                ... others ...
>                esac
>                shift
>        done
>        git pull $verbosity other options
>
> give the -q for the former and -v for the latter to "git pull"?

Yes that is much simpler and works :). Thanks.
Please see my next patch in a few minutes.
I might not reply before the weekend as I'm pretty busy, btw.

```