On Sun, Oct 19, 2008 at 11:26 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Tuncer Ayaz <tuncer.ayaz@xxxxxxxxx> 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
?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
