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. ;-)
> @@ -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?
--
Shawn.
--
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
