Re: [PATCH] notes: Disallow reusing non-blob as a note object
On Wed, Feb 12, 2014 at 4:54 AM, Johan Herland jo...@herland.net wrote: Currently git notes add -C $object will read the raw bytes from $object, and then copy those bytes into the note object, which is hardcoded to be of type blob. This means that if the given $object is a non-blob (e.g. tree or commit), the raw bytes from that object is copied into a blob object. This is probably not useful, and certainly not what any sane user would expect. So disallow it, by erroring out if the $object passed to the -C option is not a blob. The fix also applies to the -c option (in which the user is prompted to edit/verify the note contents in a text editor), and also when -c/-C is passed to git notes append (which appends the $object contents to an existing note object). In both cases, passing a non-blob $object does not make sense. Also add a couple of tests demonstrating expected behavior. Suggested-by: Junio C Hamano gits...@pobox.com Signed-off-by: Johan Herland jo...@herland.net --- builtin/notes.c | 6 +- t/t3301-notes.sh | 27 +++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/builtin/notes.c b/builtin/notes.c index 2b24d05..bb89930 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -269,7 +269,11 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset) die(_(Failed to resolve '%s' as a valid ref.), arg); if (!(buf = read_sha1_file(object, type, len)) || !len) { free(buf); - die(_(Failed to read object '%s'.), arg);; + die(_(Failed to read object '%s'.), arg); + } + if (type != OBJ_BLOB) { + free(buf); + die(_(Cannot read note data from non-blob object '%s'.), arg); The way this diagnostic is worded, it sound as if the 'read' failed rather than that the user specified an incorrect object type. Perhaps Object is not a blob '%s' or Expected blob but '%s' has type '%s' or something along those lines? } strbuf_add((msg-buf), buf, len); free(buf); -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] notes: Disallow reusing non-blob as a note object
Eric Sunshine sunsh...@sunshineco.com writes: + if (type != OBJ_BLOB) { + free(buf); + die(_(Cannot read note data from non-blob object '%s'.), arg); The way this diagnostic is worded, it sound as if the 'read' failed rather than that the user specified an incorrect object type. Perhaps Object is not a blob '%s' or Expected blob but '%s' has type '%s' or something along those lines? Yeah, sounds good. You also need to say what expects a blob, too. Perhaps something like this? builtin/notes.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/notes.c b/builtin/notes.c index c11d6e6..a16bc00 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -272,8 +272,10 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset) die(_(Failed to read object '%s'.), arg); } if (type != OBJ_BLOB) { + struct msg_arg *msg = opt-value; free(buf); - die(_(Cannot read note data from non-blob object '%s'.), arg); + die(_(The -%c option takes a blob, which '%s' is not., + msg-use_editor ? 'c' : 'C', arg)); } strbuf_add((msg-buf), buf, len); free(buf); -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] notes: Disallow reusing non-blob as a note object
Currently git notes add -C $object will read the raw bytes from $object, and then copy those bytes into the note object, which is hardcoded to be of type blob. This means that if the given $object is a non-blob (e.g. tree or commit), the raw bytes from that object is copied into a blob object. This is probably not useful, and certainly not what any sane user would expect. So disallow it, by erroring out if the $object passed to the -C option is not a blob. The fix also applies to the -c option (in which the user is prompted to edit/verify the note contents in a text editor), and also when -c/-C is passed to git notes append (which appends the $object contents to an existing note object). In both cases, passing a non-blob $object does not make sense. Also add a couple of tests demonstrating expected behavior. Suggested-by: Junio C Hamano gits...@pobox.com Signed-off-by: Johan Herland jo...@herland.net --- builtin/notes.c | 6 +- t/t3301-notes.sh | 27 +++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/builtin/notes.c b/builtin/notes.c index 2b24d05..bb89930 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -269,7 +269,11 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset) die(_(Failed to resolve '%s' as a valid ref.), arg); if (!(buf = read_sha1_file(object, type, len)) || !len) { free(buf); - die(_(Failed to read object '%s'.), arg);; + die(_(Failed to read object '%s'.), arg); + } + if (type != OBJ_BLOB) { + free(buf); + die(_(Cannot read note data from non-blob object '%s'.), arg); } strbuf_add((msg-buf), buf, len); free(buf); diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 16de05a..3bb79a4 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -812,6 +812,33 @@ test_expect_success 'create note from non-existing note with git notes add -C test_must_fail git notes list HEAD ' +test_expect_success 'create note from non-blob with git notes add -C fails' ' + commit=$(git rev-parse --verify HEAD) + tree=$(git rev-parse --verify HEAD:) + test_must_fail git notes add -C $commit + test_must_fail git notes add -C $tree + test_must_fail git notes list HEAD +' + +cat expect EOF +commit 80d796defacd5db327b7a4e50099663902fbdc5c +Author: A U Thor aut...@example.com +Date: Thu Apr 7 15:20:13 2005 -0700 + +8th + +Notes (other): +This is a blob object +EOF + +test_expect_success 'create note from blob with git notes add -C reuses blob id' ' + blob=$(echo This is a blob object | git hash-object -w --stdin) + git notes add -C $blob + git log -1 actual + test_cmp expect actual + test $(git notes list HEAD) = $blob +' + cat expect EOF commit 016e982bad97eacdbda0fcbd7ce5b0ba87c81f1b Author: A U Thor aut...@example.com -- 1.8.4.653.g2df02b3 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html