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@xxxxxxx>
> ---
> 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@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