Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

> Tuncer Ayaz <tuncer.ayaz@gmail.com> wrote:
>> After fixing clone -q I noticed that pull -q is 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.
>
>> diff --git a/builtin-merge.c b/builtin-merge.c
>> index 38266ba..1f601d4 100644
>> --- a/builtin-merge.c
>> +++ b/builtin-merge.c
>> @@ -101,7 +102,7 @@ static struct strategy *get_strategy(const char *name)
>>                       struct cmdname *ent = main_cmds.names[i];
>>                       for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
>>                               if (!strncmp(ent->name, all_strategy[j].name, ent->len)
>> -                                             && !all_strategy[j].name[ent->len])
>> +                                     && !all_strategy[j].name[ent->len])
>
> This hunk seems to just be whitespace formatting.  I'd rather not
> see it in a patch that is otherwise about --quiet/--verbose changes.
> One change per patch, please.  ;-)

>> @@ -282,18 +287,20 @@ static void squash_message(void)
>>       if (prepare_revision_walk(&rev))
>>               die("revision walk setup failed");
>>
>> -     strbuf_init(&out, 0);
>> -     strbuf_addstr(&out, "Squashed commit of the following:\n");
>> -     while ((commit = get_revision(&rev)) != NULL) {
>> -             strbuf_addch(&out, '\n');
>> -             strbuf_addf(&out, "commit %s\n",
>> -                     sha1_to_hex(commit->object.sha1));
>> -             pretty_print_commit(rev.commit_format, commit, &out, rev.abbrev,
>> -                     NULL, NULL, rev.date_mode, 0);
>> +     if(verbose || !quiet) {
>> +             strbuf_init(&out, 0);
>> +             strbuf_addstr(&out, "Squashed commit of the following:\n");
>> +             while ((commit = get_revision(&rev)) != NULL) {
>> +                     strbuf_addch(&out, '\n');
>> +                     strbuf_addf(&out, "commit %s\n",
>> +                                             sha1_to_hex(commit->object.sha1));
>> +                     pretty_print_commit(rev.commit_format, commit, &out, rev.abbrev,
>> +                                                             NULL, NULL, rev.date_mode, 0);
>> +             }
>> +             write(fd, out.buf, out.len);
>> +             close(fd);
>> +             strbuf_release(&out);
>>       }
>> -     write(fd, out.buf, out.len);
>> -     close(fd);
>> -     strbuf_release(&out);
>
> This entire hunk strikes me as being completely wrong.  The fd
> we are writing to is SQUASH_MSG.  It was opened earlier in the
> function and should be closed, even if we put nothing into the
> file.  Your change causes --quiet to leak the file descriptor.
>
> But even worse, I think your change causes SQUASH_MSG to lose its
> entire content, which makes "git merge --quiet --squash" behave
> very differently from what it does today, where it at least gives
> you a summary of the commits in the SQUASH_MSG file.
>
> IMHO this hunk shouldn't be here.

>> @@ -877,6 +885,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;
>
> Formatting nit, use "if (".

>> @@ -1019,11 +1029,11 @@ int cmd_merge(int argc, const char **argv,
>> const char *prefix)
>>               char hex[41];
>>
>>               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)
>
> Formatting nit, use "if (".

>> diff --git a/git-pull.sh b/git-pull.sh
>> index 75c3610..d84ceb5 100755
>> --- a/git-pull.sh
>> +++ b/git-pull.sh
>> @@ -16,13 +16,17 @@ cd_to_toplevel
>>  test -z "$(git ls-files -u)" ||
>>       die "You are in the middle of a conflicted merge."
>>
>> -strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
>> +quiet= verbose= strategy_args= no_stat= no_commit= squash= no_ff= log_arg=
>
> This line got a little long, maybe put the two new ones on a new
> line so we don't overrun the 80 column margin and there's an easier
> to read diff?