Re: [PATCH 3/3] notes remove: --stdin reads from the standard input

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Wed, May 18, 2011 at 05:14:21PM -0700, Junio C Hamano wrote:
>
>> +	if (from_stdin) {
>> +		struct strbuf sb = STRBUF_INIT;
>> +		while (strbuf_getwholeline(&sb, stdin, '\n') != EOF) {
>> +			int len = sb.len;
>> +			if (len && sb.buf[len - 1] == '\n')
>> +				sb.buf[--len] = '\0';
>> +			retval |= remove_one_note(t, sb.buf, flag);
>> +		}
>> +	}
>
> Wouldn't strbuf_rtrim (or even strbuf_trim) be useful here?

Thanks for noticing.

I just mimicked what was done to the result of strbuf_getwholeline() in
other places (I think from revision.c but I am not sure).

An incremental correction, relative to what I had in 'pu' overnight, looks
like this.

 builtin/notes.c  |    5 ++---
 t/t3301-notes.sh |   26 ++++++++++++++------------
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 164d605..f8e437d 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -1000,11 +1000,10 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)
 	if (from_stdin) {
 		struct strbuf sb = STRBUF_INIT;
 		while (strbuf_getwholeline(&sb, stdin, '\n') != EOF) {
-			int len = sb.len;
-			if (len && sb.buf[len - 1] == '\n')
-				sb.buf[--len] = '\0';
+			strbuf_rtrim(&sb);
 			retval |= remove_one_note(t, sb.buf, flag);
 		}
+		strbuf_release(&sb);
 	}
 	if (!retval)
 		commit_notes(t, "Notes removed by 'git notes remove'");
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index e1b5619..16de05a 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -478,13 +478,14 @@ test_expect_success 'removing with --ignore-missing but bogus ref' '
 test_expect_success 'remove reads from --stdin' '
 	before=$(git rev-parse --verify refs/notes/commits) &&
 	test_when_finished "git update-ref refs/notes/commits $before" &&
+
+	# We have only two -- add another and make sure it stays
+	git notes add -m "extra" &&
+	git notes list HEAD >after-removal-expect &&
 	git rev-parse HEAD^^ HEAD^^^ >input &&
 	git notes remove --stdin <input &&
-	git diff --name-only refs/notes/commits^ refs/notes/commits >actual &&
-	test 2 = $(wc -l <actual) &&
-	git ls-tree -r --name-only refs/notes/commits >actual &&
-	>empty &&
-	test_cmp empty actual
+	git notes list | sed -e "s/ .*//" >actual &&
+	test_cmp after-removal-expect actual
 '
 
 test_expect_success 'remove --stdin is also atomic' '
@@ -496,16 +497,17 @@ test_expect_success 'remove --stdin is also atomic' '
 	test "$before" = "$after"
 '
 
-test_expect_success 'removing with --stdin --missing-ok' '
+test_expect_success 'removing with --stdin --ignore-missing' '
 	before=$(git rev-parse --verify refs/notes/commits) &&
 	test_when_finished "git update-ref refs/notes/commits $before" &&
+
+	# We have only two -- add another and make sure it stays
+	git notes add -m "extra" &&
+	git notes list HEAD >after-removal-expect &&
 	git rev-parse HEAD^^ HEAD^^^ HEAD^ >input &&
-	git notes remove --missing-ok --stdin <input &&
-	git diff --name-only refs/notes/commits^ refs/notes/commits >actual &&
-	test 2 = $(wc -l <actual) &&
-	git ls-tree -r --name-only refs/notes/commits >actual &&
-	>empty &&
-	test_cmp empty actual
+	git notes remove --ignore-missing --stdin <input &&
+	git notes list | sed -e "s/ .*//" >actual &&
+	test_cmp after-removal-expect actual
 '
 
 test_expect_success 'list notes with "git notes list"' '