On Sun, Oct 12, 2008 at 10:08 PM, Shawn O. Pearce <spearce@xxxxxxxxxxx> wrote:
> Tuncer Ayaz <tuncer.ayaz@xxxxxxxxx> 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. ;-)
Hi Shawn, thanks for the quick review.
This is a problem caused by my automagic editor
configuration and an oversight. ACK.
>> @@ -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.
You are right, I was not 100% sure whether the assembled
message is only displayed or possibly also stored somewhere.
What would be the right place to quieten this one?
I should have tagged the mail as an RFC :-).
>> @@ -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 (".
ACK.
>> @@ -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 (".
ACK.
>> 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?
ACK.
I will fix the obvious and see what I can come up with for the
pretty_print hunk.
--
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
