[PATCH 0/1] Allow default value for target of git-notes copy

[PATCH 0/1] Allow default value for target of git-notes copy

[Doan Tran Cong Danh]

As an alternate for the incoming patch,
this snippet could be used to eliminate some dead code in builtin/notes.c
Since `argc' will always be 2 in the removed condition.

Also:


Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---

 builtin/notes.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 02e97f55c5..641ae66f75 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -488,7 +488,6 @@ static int copy(int argc, const char **argv, const char *prefix)
 {
 	int retval = 0, force = 0, from_stdin = 0;
 	const struct object_id *from_note, *note;
-	const char *object_ref;
 	struct object_id object, from_obj;
 	struct notes_tree *t;
 	const char *rewrite_cmd = NULL;
@@ -525,10 +524,8 @@ static int copy(int argc, const char **argv, const char *prefix)
 	if (get_oid(argv[0], &from_obj))
 		die(_("failed to resolve '%s' as a valid ref."), argv[0]);
 
-	object_ref = 1 < argc ? argv[1] : "HEAD";
-
-	if (get_oid(object_ref, &object))
-		die(_("failed to resolve '%s' as a valid ref."), object_ref);
+	if (get_oid(argv[1], &object))
+		die(_("failed to resolve '%s' as a valid ref."), argv[1]);
 
 	t = init_notes_check("copy", NOTES_INIT_WRITABLE);
 	note = get_note(t, &object);
-- 
2.23.0

[PATCH 1/1] notes: copy notes to HEAD by default

[Doan Tran Cong Danh]

The target objects for copying notes was defaulted to HEAD from very
early stage of git-notes.

However, that default was limited by commit bbb1b8a35a, ("notes: check
number of parameters to "git notes copy"", 2010-06-28).

Lift that limitation by adjust the check for numbers of arguments.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 Documentation/git-notes.txt | 6 +++---
 builtin/notes.c             | 2 +-
 t/t3301-notes.sh            | 4 ++++
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index f56a5a9197..ced2e8280e 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git notes' [list [<object>]]
 'git notes' add [-f] [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
-'git notes' copy [-f] ( --stdin | <from-object> <to-object> )
+'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] )
 'git notes' append [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
 'git notes' edit [--allow-empty] [<object>]
 'git notes' show [<object>]
@@ -68,8 +68,8 @@ add::
 	subcommand).
 
 copy::
-	Copy the notes for the first object onto the second object.
-	Abort if the second object already has notes, or if the first
+	Copy the notes for the first object onto the second object (defaults to
+	HEAD). Abort if the second object already has notes, or if the first
 	object has none (use -f to overwrite existing notes to the
 	second object). This subcommand is equivalent to:
 	`git notes add [-f] -C $(git notes list <from-object>) <to-object>`
diff --git a/builtin/notes.c b/builtin/notes.c
index 02e97f55c5..95456f3165 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -513,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (argc < 2) {
+	if (argc < 1) {
 		error(_("too few parameters"));
 		usage_with_options(git_notes_copy_usage, options);
 	}
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index d3fa298c6a..a8f9a0f36c 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -908,6 +908,10 @@ test_expect_success 'allow overwrite with "git notes copy -f"' '
 	git notes copy -f HEAD~2 HEAD &&
 	git log -1 >actual &&
 	test_cmp expect actual &&
+	test "$(git notes list HEAD)" = "$(git notes list HEAD~2)" &&
+	git notes copy -f HEAD~2 &&
+	git log -1 >actual &&
+	test_cmp expect actual &&
 	test "$(git notes list HEAD)" = "$(git notes list HEAD~2)"
 '
 
-- 
2.23.0

Re: [PATCH 1/1] notes: copy notes to HEAD by default

[Junio C Hamano]

Doan Tran Cong Danh <congdanhqx@gmail.com> writes:

> The target objects for copying notes was defaulted to HEAD from very
> early stage of git-notes.
>
> However, that default was limited by commit bbb1b8a35a, ("notes: check
> number of parameters to "git notes copy"", 2010-06-28).

Sorry, I don't quite get the above.  The said commit made sure 'git
notes copy' gets the right number of arguments, saying """Otherwise
we may segfault with too few parameters."""  I take that as a sign
that before that commit it was not defaulting to HEAD but attempting
to access the missing argv[2] (or whatever the index the <to-object>
should be at) and dereferencing a NULL?

    ... goes and digs ...

I think v1.6.6.1-458-g74884b524e is the commit that made the command
line parsing into the current shape, i.e. one parse_options() call
in each of the subcommand that gets dispatched, and you are right
that with that version a single argument given on the command line
is taken as the <from-object> and <to-object> defaults to HEAD.

So... what happend between that vesrion and v1.7.1-200-gbbb1b8a35a?

    ... goes and looks at bbb1b8a35a again ...

Ah, I think there is an off-by-one.  When not from-stdin and not
using rewrite-cmd, before that patch, we did not even check if
from-obj exists, so in that sense, the commit had a right idea that
it must check for "too few parameters", but it shouldn't have
insisted that we have at least two.  It is OK to have just one,
i.e. only the from-obj, for our purpose.

>  copy::
> -	Copy the notes for the first object onto the second object.
> -	Abort if the second object already has notes, or if the first
> +	Copy the notes for the first object onto the second object (defaults to
> +	HEAD). Abort if the second object already has notes, or if the first
>  	object has none (use -f to overwrite existing notes to the
>  	second object). This subcommand is equivalent to:
>  	`git notes add [-f] -C $(git notes list <from-object>) <to-object>`

This is OK.

> diff --git a/builtin/notes.c b/builtin/notes.c
> index 02e97f55c5..95456f3165 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -513,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>  
> -	if (argc < 2) {
> +	if (argc < 1) {
>  		error(_("too few parameters"));
>  		usage_with_options(git_notes_copy_usage, options);
>  	}

So is this.

> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index d3fa298c6a..a8f9a0f36c 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -908,6 +908,10 @@ test_expect_success 'allow overwrite with "git notes copy -f"' '
>  	git notes copy -f HEAD~2 HEAD &&
>  	git log -1 >actual &&
>  	test_cmp expect actual &&
> +	test "$(git notes list HEAD)" = "$(git notes list HEAD~2)" &&
> +	git notes copy -f HEAD~2 &&
> +	git log -1 >actual &&
> +	test_cmp expect actual &&
>  	test "$(git notes list HEAD)" = "$(git notes list HEAD~2)"
>  '

This I am not sure is a good test to add to, especially as a fix to
bbb1b8a, which added this test:

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 64f32ad94d..2d67a40fc1 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1044,4 +1044,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides config' '
 	git log -1 > output &&
 	test_cmp expect output
 '
+
+test_expect_success 'git notes copy diagnoses too many or too few parameters' '
+	test_must_fail git notes copy &&
+	test_must_fail git notes copy one two three
+'
+
 test_done

The lack of testing that "git notes copy <from-obj>" succeeding is
why the off-by-one bug was not noticed, so I think that test (which
still exists to this day) is the right place to add a test to
protect this fix.

As to the log message, here is how I would explain/justify the
change, if I were writing it.

	notes: fix minimum number of parameters to "copy" subcommand

	The builtin/notes.::copy() function is prepared to handle
	only one argument given from the command line; in such a
	case, to-obj defaults to HEAD.

	When bbb1b8a3 ("notes: check number of parameters to "git
	notes copy"", 2010-06-28) tried to make sure "git notes
	copy" (with *no* other argument) does not dereference NULL
	by making sure we did not get too few parameters, it
	incorrectly insisted that we get two arguments, instead of
	just one.  This disabled the defaulting to-obj to HEAD.
	Correct it.

Thanks.

Re: [PATCH 1/1] notes: copy notes to HEAD by default

[Danh Doan]

On 2019-10-16 11:01:34 +0900, Junio C Hamano wrote:
> Doan Tran Cong Danh <congdanhqx@gmail.com> writes:
> 
> > The target objects for copying notes was defaulted to HEAD from very
> > early stage of git-notes.
> >
> > However, that default was limited by commit bbb1b8a35a, ("notes: check
> > number of parameters to "git notes copy"", 2010-06-28).
> 
> Sorry, I don't quite get the above.  The said commit made sure 'git
> notes copy' gets the right number of arguments, saying """Otherwise
> we may segfault with too few parameters."""  I take that as a sign
> that before that commit it was not defaulting to HEAD but attempting
> to access the missing argv[2] (or whatever the index the <to-object>
> should be at) and dereferencing a NULL?
> 
>     ... goes and digs ...
> 
> I think v1.6.6.1-458-g74884b524e is the commit that made the command
> line parsing into the current shape, i.e. one parse_options() call
> in each of the subcommand that gets dispatched, and you are right
> that with that version a single argument given on the command line
> is taken as the <from-object> and <to-object> defaults to HEAD.
> 
> So... what happend between that vesrion and v1.7.1-200-gbbb1b8a35a?
> 
>     ... goes and looks at bbb1b8a35a again ...
> 
> Ah, I think there is an off-by-one.  When not from-stdin and not
> using rewrite-cmd, before that patch, we did not even check if
> from-obj exists, so in that sense, the commit had a right idea that
> it must check for "too few parameters", but it shouldn't have
> insisted that we have at least two.  It is OK to have just one,
> i.e. only the from-obj, for our purpose.

Yes, this is my intention.

> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> > index d3fa298c6a..a8f9a0f36c 100755
> > --- a/t/t3301-notes.sh
> > +++ b/t/t3301-notes.sh
> > @@ -908,6 +908,10 @@ test_expect_success 'allow overwrite with "git notes copy -f"' '
> >  	git notes copy -f HEAD~2 HEAD &&
> >  	git log -1 >actual &&
> >  	test_cmp expect actual &&
> > +	test "$(git notes list HEAD)" = "$(git notes list HEAD~2)" &&
> > +	git notes copy -f HEAD~2 &&
> > +	git log -1 >actual &&
> > +	test_cmp expect actual &&
> >  	test "$(git notes list HEAD)" = "$(git notes list HEAD~2)"
> >  '
> 
> This I am not sure is a good test to add to, especially as a fix to

I was writing this patch just before my bed time, just to get some
comments on the directions, e.g:
- loosen the argc requirements; or
- do the code cleanup

> bbb1b8a, which added this test:
> 
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 64f32ad94d..2d67a40fc1 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1044,4 +1044,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides config' '
>  	git log -1 > output &&
>  	test_cmp expect output
>  '
> +
> +test_expect_success 'git notes copy diagnoses too many or too few parameters' '
> +	test_must_fail git notes copy &&
> +	test_must_fail git notes copy one two three
> +'
> +
>  test_done
> 
> The lack of testing that "git notes copy <from-obj>" succeeding is
> why the off-by-one bug was not noticed, so I think that test (which
> still exists to this day) is the right place to add a test to
> protect this fix.

I don't think this is a good place to add this test either,
since the test description specificaly said it diagnoses too many or
too few parameters.

Anyway, the test `git notes copy one two three` still fails if we
accidentally allow 3 arguments since one two three isn't valid ref.
I'm gonna add a statement to assert the diagnose message.

Since I don't want to update the commit id (e.g. 10th, 11th, etc..)
of other test cases, I think it'd be better to modify the current test:

- for the test case without '-f' flag, remove HEAD if it's the target,
  and add a test-case to copy to somewhere else. Well, all of our current
  test cases only test with HEAD as target-object.
- for the test case with '-f' flag, I think I'll keep my current
  approach.

> 
> As to the log message, here is how I would explain/justify the
> change, if I were writing it.

I'll update the commit message.

-- 
Danh

[PATCH 1/2] t3301: test diagnose messages for too few/many paramters

[Doan Tran Cong Danh]

From commit bbb1b8a35a, ("notes: check number of parameters to "git
notes copy"", 2010-06-28), we have a test for too many or too few
of parameters provided to `git notes copy'.

However, the test is only ensure the command will fail but it doesn't
really check if it fails because of number of parameters.

If we accidentally lifted the check inside our code base, the test may
still failed because the provided parameter is not a valid ref.

Correct it.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 t/t3301-notes.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index d3fa298c6a..d7767e4438 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1167,8 +1167,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides config' '
 '
 
 test_expect_success 'git notes copy diagnoses too many or too few parameters' '
-	test_must_fail git notes copy &&
-	test_must_fail git notes copy one two three
+	test_must_fail git notes copy 2>error &&
+	test_i18ngrep "too few parameters" error &&
+	test_must_fail git notes copy one two three 2>error &&
+	test_i18ngrep "too many parameters" error
 '
 
 test_expect_success 'git notes get-ref expands refs/heads/master to refs/notes/refs/heads/master' '
-- 
2.23.0

[PATCH 2/2] notes: fix minimum number of parameters to "copy" subcommand

[Doan Tran Cong Danh]

The builtin/notes.c::copy() function is prepared to handle either
one or two arguments given from the command line;
in case of one argument is given, to-obj defaults to HEAD.

When bbb1b8a3 ("notes: check number of parameters to "git notes copy"",
2010-06-28) tried to make sure "git notes copy" (with *no* other
argument) does not dereference NULL by making sure we did not get
too few parameters, it incorrectly insisted that we need two arguments,
instead of either one or two.
This disabled the defaulting to-obj to HEAD.
Correct it.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 Documentation/git-notes.txt |  6 +++---
 builtin/notes.c             |  2 +-
 t/t3301-notes.sh            | 40 +++++++++++++++++++++++++++++++++++--
 3 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index f56a5a9197..ced2e8280e 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git notes' [list [<object>]]
 'git notes' add [-f] [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
-'git notes' copy [-f] ( --stdin | <from-object> <to-object> )
+'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] )
 'git notes' append [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
 'git notes' edit [--allow-empty] [<object>]
 'git notes' show [<object>]
@@ -68,8 +68,8 @@ add::
 	subcommand).
 
 copy::
-	Copy the notes for the first object onto the second object.
-	Abort if the second object already has notes, or if the first
+	Copy the notes for the first object onto the second object (defaults to
+	HEAD). Abort if the second object already has notes, or if the first
 	object has none (use -f to overwrite existing notes to the
 	second object). This subcommand is equivalent to:
 	`git notes add [-f] -C $(git notes list <from-object>) <to-object>`
diff --git a/builtin/notes.c b/builtin/notes.c
index 02e97f55c5..95456f3165 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -513,7 +513,7 @@ static int copy(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (argc < 2) {
+	if (argc < 1) {
 		error(_("too few parameters"));
 		usage_with_options(git_notes_copy_usage, options);
 	}
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index d7767e4438..d66a5f6faa 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -864,6 +864,24 @@ test_expect_success 'append to note from other note with "git notes append -c"'
 '
 
 test_expect_success 'copy note with "git notes copy"' '
+	commit=$(git rev-parse 4th) &&
+	cat >expect <<-EOF &&
+		commit $commit
+		Author: A U Thor <author@example.com>
+		Date:   Thu Apr 7 15:16:13 2005 -0700
+
+		${indent}4th
+
+		Notes:
+		${indent}This is a blob object
+	EOF
+	git notes copy 8th 4th &&
+	git log 3rd..4th >actual &&
+	test_cmp expect actual &&
+	test "$(git note list 4th)" = "$(git note list 8th)"
+'
+
+test_expect_success 'copy note with "git notes copy" with default' '
 	test_commit 11th &&
 	commit=$(git rev-parse HEAD) &&
 	cat >expect <<-EOF &&
@@ -878,7 +896,7 @@ test_expect_success 'copy note with "git notes copy"' '
 		${indent}
 		${indent}yet another note
 	EOF
-	git notes copy HEAD^ HEAD &&
+	git notes copy HEAD^ &&
 	git log -1 >actual &&
 	test_cmp expect actual &&
 	test "$(git notes list HEAD)" = "$(git notes list HEAD^)"
@@ -892,6 +910,24 @@ test_expect_success 'prevent overwrite with "git notes copy"' '
 '
 
 test_expect_success 'allow overwrite with "git notes copy -f"' '
+	commit=$(git rev-parse HEAD) &&
+	cat >expect <<-EOF &&
+		commit $commit
+		Author: A U Thor <author@example.com>
+		Date:   Thu Apr 7 15:23:13 2005 -0700
+
+		${indent}11th
+
+		Notes:
+		${indent}This is a blob object
+	EOF
+	git notes copy -f HEAD~3 HEAD &&
+	git log -1 >actual &&
+	test_cmp expect actual &&
+	test "$(git notes list HEAD)" = "$(git notes list HEAD~3)"
+'
+
+test_expect_success 'allow overwrite with "git notes copy -f" with default' '
 	commit=$(git rev-parse HEAD) &&
 	cat >expect <<-EOF &&
 		commit $commit
@@ -905,7 +941,7 @@ test_expect_success 'allow overwrite with "git notes copy -f"' '
 		${indent}
 		${indent}yet another note
 	EOF
-	git notes copy -f HEAD~2 HEAD &&
+	git notes copy -f HEAD~2 &&
 	git log -1 >actual &&
 	test_cmp expect actual &&
 	test "$(git notes list HEAD)" = "$(git notes list HEAD~2)"
-- 
2.23.0

Re: [PATCH 1/2] t3301: test diagnose messages for too few/many paramters

[Junio C Hamano]

Doan Tran Cong Danh <congdanhqx@gmail.com> writes:

> If we accidentally lifted the check inside our code base, the test may
> still failed because the provided parameter is not a valid ref.

Makes sense.  Another option would be to use a valid ref to make
sure there are no other possible reason for the command to fail,
which would make the test robust against us accidentally switching
the order of the check to see if they are refs first and then
complain about too many or too few arguments ;-)  But I think what
we have here is fine as-is (I'll copy-edit the proposed log message
for grammos, though).

Thanks.

>
> Correct it.
>
> Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
> ---
>  t/t3301-notes.sh | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index d3fa298c6a..d7767e4438 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1167,8 +1167,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides config' '
>  '
>  
>  test_expect_success 'git notes copy diagnoses too many or too few parameters' '
> -	test_must_fail git notes copy &&
> -	test_must_fail git notes copy one two three
> +	test_must_fail git notes copy 2>error &&
> +	test_i18ngrep "too few parameters" error &&
> +	test_must_fail git notes copy one two three 2>error &&
> +	test_i18ngrep "too many parameters" error
>  '
>  
>  test_expect_success 'git notes get-ref expands refs/heads/master to refs/notes/refs/heads/master' '

Re: [PATCH 2/2] notes: fix minimum number of parameters to "copy" subcommand

[Junio C Hamano]

Doan Tran Cong Danh <congdanhqx@gmail.com> writes:

>  Documentation/git-notes.txt |  6 +++---
>  builtin/notes.c             |  2 +-
>  t/t3301-notes.sh            | 40 +++++++++++++++++++++++++++++++++++--
>  3 files changed, 42 insertions(+), 6 deletions(-)

Thanks, makes sense.