On Tue, Oct 14, 2008 at 12:13 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> tuncer.ayaz@xxxxxxxxx writes:
>
>> From: Tuncer Ayaz <tuncer.ayaz@xxxxxxxxx>
>>
>> Updated patch to current Junio master.
>
> That's not a commit log message, is it?
Sorry, I was referring to my previous post and
this was my first post via send-email.
Is it ok for me to include the log message here?
-->
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
<--
is that good enough or did I miss something?
>> Signed-off-by: Tuncer Ayaz <tuncer.ayaz@xxxxxxxxx>
>> ---
>> Documentation/merge-options.txt | 8 ++++++++
>> builtin-fetch.c | 5 +++--
>> builtin-merge.c | 22 +++++++++++++++-------
>> git-pull.sh | 10 ++++++++--
>> 4 files changed, 34 insertions(+), 11 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..287ce33 100644
>> --- a/builtin-fetch.c
>> +++ b/builtin-fetch.c
>> @@ -372,12 +372,13 @@ static int store_updated_refs(const char *url, const char *remote_name,
>> SUMMARY_WIDTH, *kind ? kind : "branch",
>> REFCOL_WIDTH, *what ? what : "HEAD");
>> if (*note) {
>> - if (!shown_url) {
>> + if ((verbose || !quiet) && !shown_url) {
>
> A pair of external verbosity flag -q and -v may be acceptable, but is it
> sane to have a pair of variables in code always used like this? In other
> words, this makes me wonder if a single "verbosity level" variable that
> can be set to quiet, normal and verbose would make it more readable. For
> example, this one would say:
>
> if (verbosity >= VERBOSITY_NORMAL && !shown_url) {
> ...
> }
what I would actually prefer to implement are separate
printf functions for verbose, info and error messages
and display them according to:
info: sent to ouput if verbose is set or quiet is not set
error: always sent to ouput
verbose: only sent to output if verbose is set
you could get that with your "verbosity level" solution.
to keep it simple I would avoid adding any more
levels or topics to logging and if someone really wants
to either declare trace_printf to be debug_printf
or rename it :).
if that make sense I would like to teach this to
git as a general option as far as possible and get
rid of all the if clauses in front of printf calls.
> Also what does your command line parsing code do when the user gives -q
> and -v at the same time? Does the last one on the command line win?
> Shouldn't you instead get an error message (which of course would mean you
> would need to fix the caller in git-pull.sh)?
I thought about that also and at least in this patch
tried to handle it by ignoring quiet if verbose is set.
this may not be the logic everyone wants to have
and exclusively allowing either -q or -v makes
more sense.
>> + if (verbose || !quiet)
>> + fprintf(stderr, " %s\n", note);
>
> Ditto.
>
>> + if (verbose || !quiet)
>> + printf("%s%s\n", squash ? " (nothing to squash)" : "", msg);
>
> Ditto.
>
>> + if (verbose || !quiet)
>> + printf("%s\n", msg);
>> + if ((verbose || !quiet) && !merge_msg.len)
>
> Ditto.
>
>> + if (!verbose && quiet)
>> + show_diffstat = 0;
>
> Hmph, ah, that's (!(verbose || !quiet)). See the readability issue?
your version is more readable. the human mind seems to
have a problem with double or triple negations :).
>> + if (verbose || !quiet)
>> + printf("Updating %s..%s\n",
>> + hex,
>> + find_unique_abbrev(remoteheads->item->object.sha1,
>> + DEFAULT_ABBREV));
>
> Ditto.
>
--
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
