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@xxxxxxxxxxx>
www.herland.net
--
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