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

[Junio C Hamano <gitster@pobox.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.

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

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

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