Re: [PATCH] add --summary option to git-push and git-fetch

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 3, 2009, 09:20 UTC
Message-ID: <7viqiat965.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20090703044801.GA2072@cthulhu>

Larry D'Anna <larry@elder-gods.org> writes:

Show 13 quoted lines

> --summary will cause git-push to output a one-line of each commit pushed.
> --summary=n will display at most n commits for each ref pushed.
>
> $ git push --dry-run --summary origin :
> To /home/larry/gitsandbox/a
>    80f0e50..5593a38  master -> master
>     > 5593a38 foo
>     > 81c03f8 bar
>
> Fetch works the same way.
>
> Signed-off-by: Larry D'Anna <larry@elder-gods.org>
> ---

With this rewrite not to call cmd_log() directly, it looks much better than the previous round. It allows us more freedom to do things slightly differently than the stock cmd_log() lets us, such as giving "..." after "n" commits by default, much like fmt-merge-msg does.

Show 9 quoted lines

> diff --git a/builtin-fetch.c b/builtin-fetch.c
> index cd5eb9a..b79d870 100644
> --- a/builtin-fetch.c
> +++ b/builtin-fetch.c
> @@ -29,6 +29,7 @@ static const char *depth;
>  static const char *upload_pack;
>  static struct strbuf default_rla = STRBUF_INIT;
>  static struct transport *transport;
> +static int summary = 0;

Don't initialize statics with "= 0;".  BSS will take care of it.

Show 11 quoted lines

>  static struct option builtin_fetch_options[] = {
>  	OPT__VERBOSITY(&verbosity),
> @@ -47,6 +48,9 @@ static struct option builtin_fetch_options[] = {
>  		    "allow updating of HEAD ref"),
>  	OPT_STRING(0, "depth", &depth, "DEPTH",
>  		   "deepen history of shallow clone"),
> +	{ OPTION_INTEGER, 0, "summary", &summary, "n", "print a summary of [at most n] fetched commits",
> +	  PARSE_OPT_OPTARG, NULL, -1
> +	},
>  	OPT_END()
>  };

I think I'd prefer some reasonable default instead of making it unlimited, much like how fmt-merge-msg does. We might want to make this configurable (I think fmt-merge-msg uses a hardcoded 20), and perhaps even use them the same configuration variable (summary.length or something).

Show 6 quoted lines

> @@ -260,11 +265,12 @@ static int update_local_ref(struct ref *ref,
>  		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' : '*',
>  			SUMMARY_WIDTH, what, REFCOL_WIDTH, remote, pretty_ref,
>  			r ? "  (unable to update local ref)" : "");
> +		if (!r)
> +			strcpy (quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));

We do not leave extra whitespace between function name and open parenthesis (you have other instances of this style violation); on the other hand, we do keep one whitespace after keywords like if, while, and for (this is just fyi; I do not think I saw any violation of the latter in the patch).

Show 11 quoted lines

> diff --git a/builtin-log.c b/builtin-log.c
> index 44f9a27..cc4dc0a 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -1293,3 +1293,38 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
> ...
> +void print_summary_for_push_or_fetch (const char *quickref, int limit)
> +{
> +...
> +	temp = stdout;
> +	stdout = stderr;

Is this even a valid C? stdout and friends are described in POSIX.1 as "Normally, there are three open streams with constant pointers declared in the <stdio.h> header and associated with the standard open files."

At least Solaris 11 Clib headers does not seem to like it. I do not know about Windows.