# [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose 6 messages from 2008-10-13 to 2008-10-16. Participants: tuncer.ayaz@gmail.com, Junio C Hamano, Tuncer Ayaz. Thread: https://gitlist.dev/t/15884 ## tuncer.ayaz@gmail.com, 2008-10-13 21:42 Subject: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose Message-ID: <1223934148-13942-1-git-send-email-tuncer.ayaz@gmail.com> URL: https://gitlist.dev/e/1223934148-13942-1-git-send-email-tuncer.ayaz%40gmail.com ``` From: Tuncer Ayaz Updated patch to current Junio master. Signed-off-by: Tuncer Ayaz --- 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) { fprintf(stderr, "From %.*s\n", url_len, url); shown_url = 1; } - fprintf(stderr, " %s\n", note); + if (verbose || !quiet) + fprintf(stderr, " %s\n", note); } } fclose(fp); diff --git a/builtin-merge.c b/builtin-merge.c index 5e2b7f1..4f90501 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -44,6 +44,7 @@ static const char * const builtin_merge_usage[] = { static int show_diffstat = 1, option_log, squash; static int option_commit = 1, allow_fast_forward = 1; static int allow_trivial = 1, have_message; +static int quiet, verbose; static struct strbuf merge_msg; static struct commit_list *remoteheads; static unsigned char head[20], stash[20]; @@ -152,6 +153,8 @@ static int option_parse_n(const struct option *opt, } static struct option builtin_merge_options[] = { + OPT__QUIET(&quiet), + OPT__VERBOSE(&verbose), { OPTION_CALLBACK, 'n', NULL, NULL, NULL, "do not show a diffstat at the end of the merge", PARSE_OPT_NOARG, option_parse_n }, @@ -249,7 +252,8 @@ static void restore_state(void) /* This is called when no merge was necessary. */ static void finish_up_to_date(const char *msg) { - printf("%s%s\n", squash ? " (nothing to squash)" : "", msg); + if (verbose || !quiet) + printf("%s%s\n", squash ? " (nothing to squash)" : "", msg); drop_save(); } @@ -330,14 +334,15 @@ static void finish(const unsigned char *new_head, const char *msg) if (!msg) strbuf_addstr(&reflog_message, getenv("GIT_REFLOG_ACTION")); else { - printf("%s\n", msg); + if (verbose || !quiet) + printf("%s\n", msg); strbuf_addf(&reflog_message, "%s: %s", getenv("GIT_REFLOG_ACTION"), msg); } if (squash) { squash_message(); } else { - if (!merge_msg.len) + if ((verbose || !quiet) && !merge_msg.len) printf("No merge message -- not updating HEAD\n"); else { const char *argv_gc_auto[] = { "gc", "--auto", NULL }; @@ -871,6 +876,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, builtin_merge_options, builtin_merge_usage, 0); + if (!verbose && quiet) + show_diffstat = 0; if (squash) { if (!allow_fast_forward) @@ -1012,10 +1019,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix) strcpy(hex, find_unique_abbrev(head, DEFAULT_ABBREV)); - printf("Updating %s..%s\n", - hex, - find_unique_abbrev(remoteheads->item->object.sha1, - DEFAULT_ABBREV)); + if (verbose || !quiet) + printf("Updating %s..%s\n", + hex, + find_unique_abbrev(remoteheads->item->object.sha1, + DEFAULT_ABBREV)); strbuf_addstr(&msg, "Fast forward"); if (have_message) strbuf_addstr(&msg, diff --git a/git-pull.sh b/git-pull.sh index 75c3610..8e25d44 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." +quiet= verbose= 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/||") @@ -23,6 +24,10 @@ rebase=$(git config --bool branch.$curr_branch_short.rebase) while : do case "$1" in + -q|--quiet) + quiet=-q ;; + -v|--verbose) + verbose=-v ;; -n|--no-stat|--no-summary) no_stat=-n ;; --stat|--summary) @@ -121,7 +126,7 @@ test true = "$rebase" && { "refs/remotes/$origin/$reflist" 2>/dev/null)" } orig_head=$(git rev-parse --verify HEAD 2>/dev/null) -git fetch --update-head-ok "$@" || exit 1 +git fetch $verbose $quiet --update-head-ok "$@" || exit 1 curr_head=$(git rev-parse --verify HEAD 2>/dev/null) if test "$curr_head" != "$orig_head" @@ -181,5 +186,6 @@ merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit test true = "$rebase" && exec git-rebase $strategy_args --onto $merge_head \ ${oldremoteref:-$merge_head} -exec git-merge $no_stat $no_commit $squash $no_ff $log_arg $strategy_args \ +exec git-merge $quiet $verbose $no_stat $no_commit \ + $squash $no_ff $log_arg $strategy_args \ "$merge_name" HEAD $merge_head -- 1.6.0.2.GIT ``` ## Junio C Hamano, 2008-10-13 22:13 Subject: Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose Message-ID: <7vzll887ps.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzll887ps.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1223934148-13942-1-git-send-email-tuncer.ayaz@gmail.com> ``` tuncer.ayaz@gmail.com writes: > From: Tuncer Ayaz > > Updated patch to current Junio master. That's not a commit log message, is it? > Signed-off-by: Tuncer Ayaz > --- > 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) { ... } 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)? > + 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? > + if (verbose || !quiet) > + printf("Updating %s..%s\n", > + hex, > + find_unique_abbrev(remoteheads->item->object.sha1, > + DEFAULT_ABBREV)); Ditto. ``` ## Tuncer Ayaz, 2008-10-13 22:29 Subject: Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose Message-ID: <4ac8254d0810131529l37d67b61q3589f15700d38261@mail.gmail.com> URL: https://gitlist.dev/e/4ac8254d0810131529l37d67b61q3589f15700d38261%40mail.gmail.com In-Reply-To: <7vzll887ps.fsf@gitster.siamese.dyndns.org> ``` On Tue, Oct 14, 2008 at 12:13 AM, Junio C Hamano wrote: > tuncer.ayaz@gmail.com writes: > >> From: Tuncer Ayaz >> >> 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 >> --- >> 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. > ``` ## Tuncer Ayaz, 2008-10-16 05:54 Subject: Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose Message-ID: <4ac8254d0810152254i615bca9dye0aedd8689c946e7@mail.gmail.com> URL: https://gitlist.dev/e/4ac8254d0810152254i615bca9dye0aedd8689c946e7%40mail.gmail.com In-Reply-To: <7vprm1pfmd.fsf@gitster.siamese.dyndns.org> ``` On Thu, Oct 16, 2008 at 2:07 AM, Junio C Hamano wrote: > "Tuncer Ayaz" writes: > >> On Wed, Oct 15, 2008 at 9:06 PM, Junio C Hamano wrote: >>> "Tuncer Ayaz" writes: >>> >>>> Junio, what's the status here? Do you want me to rework it >>>> all with a new verbose/quiet log infrastructure or >>> >>> Not really. >> >> OK, when do you expect to cut 1.6.0.3? It's so simple >> that I'd like to have it included in that revision. > > Hmm, I did not think this was a breakage that needs to be fixed on the > maintenance track. git-pull does not know -q nor -v and teaching these > new options to the command would be a feature enhancement, which by > definition won't be in 1.6.0.3. It's no breakage as the options did not exist before :-), yes. >>> Using two variables to keep track of what is conceptually a tristate >>> (quiet, normal and verbose) is insane, and I'd like to see that insanity >>> fixed first in the patch, regardless of an elaborate "log infrastructure" >>> you mentioned. >> >> I see what you mean. What about all other modules >> with quiet/verbose doing it with two variables? > > Are they broken? If not, let's not touch them. On the other hand, let's > avoid adding more. Not really. After I fixed -q in clone/fetch Miklos Vajna came up with --verbose for clone. I just thought if we come up with a new way to handle this we should fix it everywhere -v and -q are available and make it consistent. >> I guess doing it with a single variable in pull first >> as an example is what you're after, right? > > I did not actually mind the ones in 'git-pull' that much, as eventually > the script will be rewritten in C by somebody anyway. The patch to > builtin-fetch.c/builtin-merge.c is different, as the repetition of > (verbose || !quiet) was quite noticeable. I've added -v as it was added to clone in "[PATCH] Implement git clone -v" and I wanted to be fair to the IDE developers. Would you prefer to leave -v out? > Why have we gone off-list, by the way? I was not sure this is of interest to everybody. We are now on-list again :-). ``` ## Junio C Hamano, 2008-10-16 06:15 Subject: Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose Message-ID: <7vtzbdjcb8.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtzbdjcb8.fsf%40gitster.siamese.dyndns.org In-Reply-To: <4ac8254d0810152254i615bca9dye0aedd8689c946e7@mail.gmail.com> ``` "Tuncer Ayaz" writes: > On Thu, Oct 16, 2008 at 2:07 AM, Junio C Hamano wrote: >> "Tuncer Ayaz" writes: >> >>> On Wed, Oct 15, 2008 at 9:06 PM, Junio C Hamano wrote: >>>> "Tuncer Ayaz" writes: >>>> > Would you prefer to leave -v out? Not at all. Perhaps there is a deeper misunderstanding. It makes perfect sense _at the end user interface level_ to have -v and -q as two separate options, perhaps with "later one wins" semantics. Another possible semantics is "-q and -v are mutually incompatible", but I think "later one wins" makes it much more usable from the end user's point of view. The only thing I was objecting to was your repeated (verbose || !quiet) expression in the _implementation_, which would have been much easier to read and maintain, if it were expressed as a single variable "verbosity" that can have one of three values. IOW, static enum { QUIET, NORMAL, VERBOSE } verbosity = NORMAL; ... if (!strcmp("--quiet", arg)) verbosity = QUIET; else if (!strcmp("--verbose", arg)) verbosity = VERBOSE; else ... ... if (verbosity > QUIET) print informational message; if (verbosity > NORMAL) print verbose message; See? ``` ## Tuncer Ayaz, 2008-10-16 20:08 Subject: Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose Message-ID: <4ac8254d0810161308q3d463850k69e4f5615a974367@mail.gmail.com> URL: https://gitlist.dev/e/4ac8254d0810161308q3d463850k69e4f5615a974367%40mail.gmail.com In-Reply-To: <7vtzbdjcb8.fsf@gitster.siamese.dyndns.org> ``` On Thu, Oct 16, 2008 at 8:15 AM, Junio C Hamano wrote: > "Tuncer Ayaz" writes: > >> On Thu, Oct 16, 2008 at 2:07 AM, Junio C Hamano wrote: >>> "Tuncer Ayaz" writes: >>> >>>> On Wed, Oct 15, 2008 at 9:06 PM, Junio C Hamano wrote: >>>>> "Tuncer Ayaz" writes: >>>>> >> Would you prefer to leave -v out? > > Not at all. > > Perhaps there is a deeper misunderstanding. Perhaps there was one :-) > It makes perfect sense _at the end user interface level_ to have -v and -q > as two separate options, perhaps with "later one wins" semantics. Another > possible semantics is "-q and -v are mutually incompatible", but I think > "later one wins" makes it much more usable from the end user's point of view. > > The only thing I was objecting to was your repeated (verbose || !quiet) > expression in the _implementation_, which would have been much easier to > read and maintain, if it were expressed as a single variable "verbosity" > that can have one of three values. This leaves no space for speculation and is as clear as it gets :D > IOW, > > static enum { QUIET, NORMAL, VERBOSE } verbosity = NORMAL; > ... > > if (!strcmp("--quiet", arg)) > verbosity = QUIET; > else if (!strcmp("--verbose", arg)) > verbosity = VERBOSE; > else ... > > ... > if (verbosity > QUIET) > print informational message; > if (verbosity > NORMAL) > print verbose message; > > See? Yeah, no problem with that. How do you propose we should integrate this with the existing usage of parse_options() and OPT_ macros? I want to keep using it and not redo argv handling from scratch in builtin-fetch/builtin-merge. ```