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

[Junio C Hamano <gitster@xxxxxxxxx>]

Larry D'Anna <larry@xxxxxxxxxxxxxx> writes:

> --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@xxxxxxxxxxxxxx>
> ---

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.

> 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.

>  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).

> @@ -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).

> 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.

--
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