Re: [PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin

From: Jeff King <peff@peff.net>
Date: Mar 25, 2012, 01:19 UTC
Message-ID: <20120325011948.GC27651@sigill.intra.peff.net>
In-Reply-To: <4F6E3446.9080001@gmx.net>

On Sat, Mar 24, 2012 at 09:53:26PM +0100, Ivan Todoroski wrote:

> From c4bb55f9f27569faa368d823ca6fe4b236e37cd6 Mon Sep 17 00:00:00 2001
> From: Ivan Todoroski <grnch@gmx.net>
> Date: Sat, 24 Mar 2012 15:13:05 +0100
> Subject: [PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin

You can drop these lines; they are redundant with the actual email headers.

Show 5 quoted lines

> ---
>  Documentation/git-fetch-pack.txt |    9 ++++++++
>  builtin/fetch-pack.c             |   44 ++++++++++++++++++++++++++++++++++++--
>  fetch-pack.h                     |    3 ++-
>  3 files changed, 53 insertions(+), 3 deletions(-)

Give more of a commit message. Why is this option useful (I know, of course, from our previous discussion. But keep in mind the audience of developers reading "git log" a year from now).

Show 8 quoted lines

> +--stdin::
> +	Take the list of refs from stdin, one per line. If there
> +	are refs specified on the command line in addition to this
> +	option, then the refs from stdin are processed after those
> +	on the command line.
> +	If '--stateless-rpc' is specified together with this option
> +	then the list of refs must be in packet format (pkt-line)
> +	with a flush packet terminating the list.

Nice. Thanks for taking the time not just to solve your problem, but to give sane semantics in the non-stateless-rpc case.

I think there is a minor formatting bug in the above. Asciidoc will make your two paragraphs into a single one, won't it? I think you need to do the (horribly ugly):

  --stdin::
      First paragraph.
  +
  Second paragraph.

to appease asciidoc.

Show 8 quoted lines

> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -23,7 +23,7 @@ static struct fetch_pack_args args = {
>  };
>  
>  static const char fetch_pack_usage[] =
> -"git fetch-pack [--all] [--quiet|-q] [--keep|-k] [--thin] [--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] [--no-progress] [-v] [<host>:]<directory> [<refs>...]";
> +"git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] [--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] [--no-progress] [-v] [<host>:]<directory> [<refs>...]";

Not a problem you introduced, but maybe it is time for us to cut down this gigantic line (it's 180 characters even before your patch!). Even breaking it across lines at 80 columns would help.

Show 6 quoted lines

> @@ -972,6 +976,42 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  	if (!dest)
>  		usage(fetch_pack_usage);
>  
> +	if (args.refs_from_stdin) {
> +		char ref[1000];

Ick. Is there any reason not to use a strbuf here? 1000 is probably plenty, but we are generally moving towards removing such limits where possible.

You'd also get to use strbuf_getline and strbuf_trim in the newline-delimited case.

-Peff