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

[Junio C Hamano <gitster@pobox.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.