Re: [PATCH 6/6] RFC - Notes merge: die when asked to merge a non-existent ref.
- From
Johan Herland <johan@herland.net>
- Date
- Jun 19, 2011, 22:03 UTC
- Message-ID
- <201106200003.46490.johan@herland.net>
- In-Reply-To
- <1308431208-13353-7-git-send-email-ydirson@free.fr>
On Saturday 18 June 2011, Yann Dirson wrote:
Show 9 quoted lines
> 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".
Show 16 quoted lines
> 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.
Show 15 quoted lines
> [...] > > 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