Re: [PATCH] notes: allow merging from arbitrary references

From: Jacob Keller <jacob.keller@gmail.com>
Date: Oct 1, 2015, 22:51 UTC
Message-ID: <CA+P7+xp6XtsZ3jCN91+2Mt4Ad0VxRvd3Ra3yAWHktpYW2Ho7uw@mail.gmail.com>
In-Reply-To: <1443739165-17526-1-git-send-email-jacob.e.keller@intel.com>

On Thu, Oct 1, 2015 at 3:39 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:

Show 15 quoted lines

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Create a new expansion function, expand_loose_notes_ref which will
> expand any ref using get_sha1, but falls back to expand_notes_ref if
> this fails. The contents of the strbuf will be either the hex string of
> the sha1, or the expanded notes ref. It is expected to be re-expanded
> using get_sha1 inside the notes merge machinery, and there is no real
> error checking provided at this layer.
>
> Since we now support merging from non-notes refs, remove the test case
> associated with that behavior. Add a test case for merging from a
> non-notes ref.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---

I haven't yet figured out how to handle showing of notes on non-notes refs, since the core notes command uses expand_notes_ref very early on in the flow before we even know what kind of command we're using. I think this will take a larger structural change to get right.

For merges it's pretty straight forward. This patch may seem weird, but it's expected that the internal notes machinery will (re)expand the remote_ref again and properly error out if it doesn't exist. We need this since merging from a notes ref that doesn't exist isn't considered an error today (it just falls back to using an empty ref).

Note that this does have some weird consequences, in that if you typo a reference name such as refs/foo/bar which doesn't exist, then it will be expanded into refs/notes/refs/foo/bar, which I am not sure is the best approach, but otherwise things such as "x" don't properly expand... I think in practice this will always result in the "empty" notes ref being used for the source of the merge, so it may not be a big deal.

Regards, Jake