Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose
- From
Shawn O. Pearce <spearce@spearce.org>
- Date
- Oct 12, 2008, 20:08 UTC
- Message-ID
- <20081012200833.GC5255@spearce.org>
- In-Reply-To
- <4ac8254d0810120954x2364054ahf2d49d6fbb7b0bb1@mail.gmail.com>
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.
Show 10 quoted lines
> 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. ;-)
Show 29 quoted lines
> @@ -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.
Show 7 quoted lines
> @@ -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 (".Show 11 quoted lines
> @@ -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 (".Show 10 quoted lines
> 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?
-- Shawn.