Re: [PATCH] WIP: git notes copy --stdin

[Johan Herland <johan@herland.net>]

On Sunday 14 February 2010, Thomas Rast wrote:
> This implements a 'git notes copy --stdin' mode suitable to read the
> data fed to the post-rewrite hook.
> ---
> 
> Ok, so I changed my mind and decided to implement the "make it
> built-in and configurable" way.  This helper should work.  I spent
> some time trying to refactor cmd_notes() into something that can
> nicely handle commands that do not fit the normal scheme, but
> eventually was too tired to continue and just crafted it in the
> existing code in the right place.
> 
> If nobody beats me to it I'll pick this up later this week, but
> tomorrow will be too busy to even think about it.
> 
> 
>  builtin-notes.c |   56
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed,
> 56 insertions(+), 0 deletions(-)
> 
> diff --git a/builtin-notes.c b/builtin-notes.c
> index 123ecad..3cd9e45 100644
> --- a/builtin-notes.c
> +++ b/builtin-notes.c
> @@ -278,6 +278,52 @@ int commit_notes(struct notes_tree *t, const char
> *msg) return 0;
>  }
> 
> +int notes_copy_from_stdin(int force)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	struct notes_tree *t;
> +
> +	init_notes(NULL, NULL, NULL, 0);
> +	t = &default_notes_tree;

As you say, cmd_notes() should be refactored, so that you don't have to 
duplicate all of this (and some of the below stuff).

> +	while (strbuf_getline(&buf, stdin, '\n') != EOF) {
> +		unsigned char from_obj[20], to_obj[20];
> +		struct strbuf **split;
> +		const unsigned char *note;
> +
> +		split = strbuf_split(&buf, ' ');
> +		if (!split[0] || !split[1])
> +			die("Malformed input line: '%s'.", buf.buf);
> +		strbuf_rtrim(split[0]);
> +		strbuf_rtrim(split[1]);
> +		if (get_sha1(split[0]->buf, from_obj))
> +			die("Failed to resolve '%s' as a valid ref.", split[0]->buf);
> +		if (get_sha1(split[1]->buf, to_obj))
> +			die("Failed to resolve '%s' as a valid ref.", split[1]->buf);
> +
> +		note = get_note(t, from_obj);
> +		if (!force) {
> +			const unsigned char *existing_note = get_note(t, to_obj);
> +			if (existing_note) {
> +				error("Cannot copy notes. Found existing notes for object"
> +				      " %s. Use '-f' to overwrite existing notes",
> +				      sha1_to_hex(to_obj));
> +				return 1;
> +			}
> +		}
> +
> +		if (note)
> +			add_note(t, to_obj, note, combine_notes_overwrite);
> +		else
> +			remove_note(t, to_obj);
> +
> +		strbuf_list_free(split);
> +	}
> +
> +	commit_notes(t, "Notes added by 'git notes copy'");

We (this goes for the existing commit_notes() call in cmd_notes() as well) 
should either check the return value for commit_notes(), or (since it 
currently only returns 0) we should change commit_notes() to void instead of 
int. Actually, the correct solution is probably to "return error()" instead 
of "die()" inside commit_notes(), and then check the return value.

> +	return 0;
> +}
> +
>  int cmd_notes(int argc, const char **argv, const char *prefix)
>  {
>  	struct notes_tree *t;
> @@ -301,6 +347,7 @@ int cmd_notes(int argc, const char **argv, const char
> *prefix) OPT_CALLBACK('C', "reuse-message", &msg, "OBJECT",
>  			   "reuse specified note object", parse_reuse_arg),
>  		OPT_BOOLEAN('f', "force", &force, "replace existing notes"),
> +		OPT_BOOLEAN(0, "stdin", &force, "read objects from stdin"),

Why are you setting &force from "--stdin"?

> @@ -351,6 +398,15 @@ int cmd_notes(int argc, const char **argv, const
> char *prefix)
> 
>  	if (copy) {
>  		const char *from_ref;
> +		if (stdin) {

I don't think this is intended. Isn't stdin (the standard input FILE *) 
_always_ true?

> +			if (argc > 2) {
> +				error("too few parameters");
> +				usage_with_options(git_notes_usage, options);
> +			} else {
> +				retval = notes_copy_from_stdin(force);
> +				goto end;
> +			}
> +		}
>  		if (argc < 3) {
>  			error("too few parameters");
>  			usage_with_options(git_notes_usage, options);

The patch needs to add code that barfs if "--stdin" is used with any 
subcommand other than "copy".

Of course, the patch also needs documentation and tests, but I guess you 
knew that... ;)


I like the intent of this patch, although it needs more work before it's 
finished.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net