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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 19, 2008, 21:26 UTC
Message-ID: <7vy70k5las.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1224445691-1366-1-git-send-email-tuncer.ayaz@gmail.com>

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

Show 6 quoted lines

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

Show 9 quoted lines

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

Show 15 quoted lines

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

Show 12 quoted lines

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

Show 8 quoted lines

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