Re: [PATCH/v3] bundle.c: added --stdin option to git-bundle

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 5, 2008, 18:15 UTC
Message-ID: <7vod5crydx.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <c376da900807050930i6d1da898s624be58adc6f1751@mail.gmail.com>

"Adam Brewster" <adambrewster@gmail.com> writes:

> Subject: Re: [PATCH/v3] bundle.c: added --stdin option to git-bundle

When the change is not about implementation detail (in which case you do want to name the source file and perhaps even a function name), but about a new feature that is visible to the end-users of a command, we'd want the message talk in terms of what the new feature does, not how the new feature is invoked nor where it is implemented. In other words, something like these are preferred:

	git-bundle: add --stdin
        Teach git-bundle to read tips and basis from standard input

and don't say "You did" in past tense --- say things in imperative mood instead, as if you are commanding the person who applies the patch to make it happen. Older log entries in our history (e.g. "git log -n 20 v0.99") may give you a better feel.

And give a few lines of obvious justfication in the body of the commit log message, e.g.

	This patch allows the caller to feed the revision parameters to
	git-bundle from its standard input.  This way, a script do not
	have to worry about limitation of the length of command line.

to explain why this is good. In order to explain that you may have to talk about other things (like what it does and how it does it), but keep in mind that the primary thing you should talk about is _why_.

> ... because it already implies that this option is available.

If that is the case, please mention in the commit log message something like "Even though the documentation said "bundle --stdin" is accepted it didn't. This patch teaches the option to the command".

But I do not think there is no such implication. "bundle create" may take list of positive and negative refs as arguments or --branches, but it does not take (and it shouldn't -- I do not think it should take --bisect option, for example) artibrary options that rev-list command accepts.

Show 9 quoted lines

>  bundle.c |   22 ++++++++++++++++++++--
>  1 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 0ba5df1..b44a4af 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -227,8 +227,26 @@ int create_bundle(struct bundle_header *header,
> const char *path,

Wrapped.

Show 7 quoted lines

>         /* write references */
>         argc = setup_revisions(argc, argv, &revs, NULL);
> -       if (argc > 1)
> -               return error("unrecognized argument: %s'", argv[1]);
> +
> +       for (i = 1; i < argc; i++) {
> +               if ( !strcmp(argv[i], "--stdin") ) {

Style.

> +                       char line[1000];
> +                               while (fgets(line, sizeof(line),
> stdin) != NULL) {

Too deep indentation.  Wrapped.

Show 16 quoted lines

> +                               int len = strlen(line);
> +                               if (len && line[len - 1] == '\n')
> +                                       line[--len] = '\0';
> +                               if (!len)
> +                                       break;
> +                               if (line[0] == '-')
> +                                       die("options not supported in
> --stdin mode");
> +                               if (handle_revision_arg(line, &revs, 0, 1))
> +                                       die("bad revision '%s'", line);
> +                       }
> +                       continue;
> +               }
> +
> +               return error("unrecognized argument: %s'", argv[i]);
> +       }

Having said that, I think copying and pasting read_revisions_from_stdin() in bundle.c is a wrong approach to take. Probably the function can easily be split out of builtin-rev-list.c and moved to revision.c or somewhere (which will be the first patch), and then a separate patch can add a few lines to call it from bundle.c.