Re: [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.

[Johan Herland <johan@herland.net>]

On Saturday 18 June 2011, Yann Dirson wrote:
> This causes the "merge empty notes ref (z => y)" test in
> t3308-notes-merge.sh to fail - obviously, it is removing the
> functionnality that is tested for.
> 
> Is there any real use for this ?  It just seems so different from
> "git merge", which errors out in the similar situation:
> 
> $ git merge foo
> fatal: 'foo' does not point to a commit

I understand your reasoning, and I don't have a problem with changing this 
behavior to be in line with "git merge".

> Signed-off-by: Yann Dirson <ydirson@free.fr>
> ---
>  builtin/notes.c        |    3 +++
>  t/t3308-notes-merge.sh |    6 ------
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 6bff44f..058b14d 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -908,6 +908,9 @@ static int merge(int argc, const char **argv, const
> char *prefix) expand_notes_ref(&remote_ref, 1);
>  	o.remote_ref = remote_ref.buf;
> 
> +	if (!peel_to_type(o.remote_ref, 0, NULL, OBJ_COMMIT))
> +		die("'%s' does not point to a commit", o.remote_ref);

Hmm. I'm not sure requiring the remote ref to always point to a _commit_ is 
the right solution here. In previous discussions on the notes topic, some 
people (Peff?) expressed a need/interest for history-less notes refs (i.e. a 
notes tree where we don't keep track of its development, but only refer to 
the latest/current version). Obviously, there are two ways to implement 
history-less notes refs: (a) making the notes ref point to a notes commit 
without any parents (i.e. each notes commit is a root commit), or (b) making 
the notes ref point directly at the notes _tree_ object (i.e. no commit 
object at all).

I can't remember off the top of my head whether our earlier discussions on 
this topic resulted in us excluding support for option (b), but if we 
didn't, it should be possible to merge notes refs where one or both refs 
point directly at a tree object, and your above line would break this.

> [...]
> 
> diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
> index 24d82b4..2dcc1db 100755
> --- a/t/t3308-notes-merge.sh
> +++ b/t/t3308-notes-merge.sh
> @@ -104,12 +104,6 @@ test_expect_success 'merge notes into empty notes
> ref (x => y)' ' test "$(git rev-parse refs/notes/x)" = "$(git rev-parse
> refs/notes/y)" '
> 
> -test_expect_success 'merge empty notes ref (z => y)' '
> -	git notes merge z &&
> -	# y should not change (still == x)
> -	test "$(git rev-parse refs/notes/x)" = "$(git rev-parse refs/notes/y)"
> -'

Instead of removing the test, please change it into verifying the _new_ 
expected behavior (that we fail with an appropriate error message when asked 
to merge a non-existent notes ref).


...Johan

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