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

From: Tuncer Ayaz <tuncer.ayaz@gmail.com>
Date: Oct 20, 2008, 16:35 UTC
Message-ID: <4ac8254d0810200935sf7ad873tea53c0fb53bbe1c0@mail.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:

Show 10 quoted lines

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

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

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

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

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

Show 34 quoted lines

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