threads / patch / 40791

patchnotes: allow merging from arbitrary references

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

## tl;dr

12 messages between Nov 13, 2015 and Dec 11, 2015. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

Jacob Keller· Nov 13, 2015, 16:34 UTC · lore
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 do not remember what version this was since it has been an age ago
that I sent the previous code. This is mostly just a rebase onto current
next. I believe I have covered everything previous reviewers noted.

I'm interested in whether this is the right direction, as my longterm goal is to be able to push/pull notes to a specific namespace (probably refs/remote-notes/*, since actually modifying to use refs/remotes/notes/* is difficult to send to users, and remote-notes makes the most useful sense). The first part of this is allowing merge to come from an arbitrary reference, as currently it is not really possible to merge from refs/remote-notes as we'd need it to be.

 builtin/notes.c        |  4 ++--
 notes.c                | 14 ++++++++++++++
 notes.h                |  8 ++++++++
 t/t3308-notes-merge.sh | 22 +++++++++++-----------
 4 files changed, 35 insertions(+), 13 deletions(-)
Show changes to 4 files +35 −13

builtin/notes.c, notes.c, notes.h, t/t3308-notes-merge.sh

diff --git a/builtin/notes.c b/builtin/notes.c
index e0f5d308d206..4a86cc90ee92 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -809,7 +809,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 
 	o.local_ref = default_notes_ref();
 	strbuf_addstr(&remote_ref, argv[0]);
-	expand_notes_ref(&remote_ref);
+	expand_loose_notes_ref(&remote_ref);
 	o.remote_ref = remote_ref.buf;
 
 	t = init_notes_check("merge", NOTES_INIT_WRITABLE);
@@ -836,7 +836,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 	}
 
 	strbuf_addf(&msg, "notes: Merged notes from %s into %s",
-		    remote_ref.buf, default_notes_ref());
+		    argv[0], default_notes_ref());
 	strbuf_add(&(o.commit_msg), msg.buf + 7, msg.len - 7); /* skip "notes: " */
 
 	result = notes_merge(&o, t, result_sha1);
diff --git a/notes.c b/notes.c
index 358e2fdb74eb..c92c22aa217a 100644
--- a/notes.c
+++ b/notes.c
@@ -1306,3 +1306,17 @@ void expand_notes_ref(struct strbuf *sb)
 	else
 		strbuf_insert(sb, 0, "refs/notes/", 11);
 }
+
+void expand_loose_notes_ref(struct strbuf *sb)
+{
+	unsigned char object[20];
+
+	if (get_sha1(sb->buf, object)) {
+		/* fallback to expand_notes_ref */
+		expand_notes_ref(sb);
+	} else {
+		/* we got an object, so replace the strbuf with the hex string */
+		strbuf_reset(sb);
+		strbuf_addstr(sb, sha1_to_hex(object));
+	}
+}
diff --git a/notes.h b/notes.h
index e5d67fd3754a..658caf7d6e99 100644
--- a/notes.h
+++ b/notes.h
@@ -302,4 +302,12 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
 /* Expand inplace a note ref like "foo" or "notes/foo" into "refs/notes/foo" */
 void expand_notes_ref(struct strbuf *sb);
 
+/*
+ * Similar to expand_notes_ref, but allows arbitrary refs to be expanded via
+ * get_sha1 first. If get_sha1 fails to find a ref, fall back to traditional
+ * expand_notes_ref. The contents of the strbuf will be suitable to attempt
+ * passing to get_sha1 again inside the notes machinery.
+ */
+void expand_loose_notes_ref(struct strbuf *sb);
+
 #endif
diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
index 24d82b49bbea..19aed7ec953b 100755
--- a/t/t3308-notes-merge.sh
+++ b/t/t3308-notes-merge.sh
@@ -18,7 +18,9 @@ test_expect_success setup '
 	git notes add -m "Notes on 1st commit" 1st &&
 	git notes add -m "Notes on 2nd commit" 2nd &&
 	git notes add -m "Notes on 3rd commit" 3rd &&
-	git notes add -m "Notes on 4th commit" 4th
+	git notes add -m "Notes on 4th commit" 4th &&
+	# Copy notes to remote-notes
+	git fetch . refs/notes/*:refs/remote-notes/origin/*
 '
 
 commit_sha1=$(git rev-parse 1st^{commit})
@@ -66,7 +68,9 @@ test_expect_success 'verify initial notes (x)' '
 '
 
 cp expect_notes_x expect_notes_y
+cp expect_notes_x expect_notes_v
 cp expect_log_x expect_log_y
+cp expect_log_x expect_log_v
 
 test_expect_success 'fail to merge empty notes ref into empty notes ref (z => y)' '
 	test_must_fail git -c "core.notesRef=refs/notes/y" notes merge z
@@ -84,16 +88,12 @@ test_expect_success 'fail to merge into various non-notes refs' '
 	test_must_fail git -c "core.notesRef=refs/notes/foo^{bar" notes merge x
 '
 
-test_expect_success 'fail to merge various non-note-trees' '
-	git config core.notesRef refs/notes/y &&
-	test_must_fail git notes merge refs/notes &&
-	test_must_fail git notes merge refs/notes/ &&
-	test_must_fail git notes merge refs/notes/dir &&
-	test_must_fail git notes merge refs/notes/dir/ &&
-	test_must_fail git notes merge refs/heads/master &&
-	test_must_fail git notes merge x: &&
-	test_must_fail git notes merge x:foo &&
-	test_must_fail git notes merge foo^{bar
+test_expect_success 'merge non-notes ref into empty notes ref (remote-notes/origin/x => v)' '
+	git config core.notesRef refs/notes/v &&
+	git notes merge refs/remote-notes/origin/x &&
+	verify_notes v &&
+	# refs/remote-notes/origin/x and v should point to the same notes commit
+	test "$(git rev-parse refs/remote-notes/origin/x)" = "$(git rev-parse refs/notes/v)"
 '
 
 test_expect_success 'merge notes into empty notes ref (x => y)' '
-- 
2.6.1.264.gbab76a9
Johan Herland· Nov 15, 2015, 22:14 UTC · re: Jacob Keller · lore

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

On Fri, Nov 13, 2015 at 5:34 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
Show 18 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 do not remember what version this was since it has been an age ago
> that I sent the previous code. This is mostly just a rebase onto current
> next. I believe I have covered everything previous reviewers noted.
Looks good to me.
Show 7 quoted lines
> I'm interested in whether this is the right direction, as my longterm
> goal is to be able to push/pull notes to a specific namespace (probably
> refs/remote-notes/*, since actually modifying to use
> refs/remotes/notes/* is difficult to send to users, and remote-notes
> makes the most useful sense). The first part of this is allowing merge
> to come from an arbitrary reference, as currently it is not really
> possible to merge from refs/remote-notes as we'd need it to be.
Yes, I agree that merging from refs outside refs/notes/ should become possible.
A related topic that has been discussed (although I cannot remember if
any conclusion was reached) is whether to allow more notes operations
- specifically _read-only_ operations - on notes trees outside
refs/notes/. I believe this should also become possible, although I
haven't thoroughly examined all implications.
...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Jacob Keller· Nov 15, 2015, 23:23 UTC · re: Johan Herland · lore

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

On Sun, Nov 15, 2015 at 2:14 PM, Johan Herland <johan@herland.net> wrote:
Show 17 quoted lines
>> ---
>> I do not remember what version this was since it has been an age ago
>> that I sent the previous code. This is mostly just a rebase onto current
>> next. I believe I have covered everything previous reviewers noted.
>
> Looks good to me.
>
>> I'm interested in whether this is the right direction, as my longterm
>> goal is to be able to push/pull notes to a specific namespace (probably
>> refs/remote-notes/*, since actually modifying to use
>> refs/remotes/notes/* is difficult to send to users, and remote-notes
>> makes the most useful sense). The first part of this is allowing merge
>> to come from an arbitrary reference, as currently it is not really
>> possible to merge from refs/remote-notes as we'd need it to be.
>
> Yes, I agree that merging from refs outside refs/notes/ should become possible.
>
Thanks.
Show 9 quoted lines
> A related topic that has been discussed (although I cannot remember if
> any conclusion was reached) is whether to allow more notes operations
> - specifically _read-only_ operations - on notes trees outside
> refs/notes/. I believe this should also become possible, although I
> haven't thoroughly examined all implications.
>
> ...Johan
>
>

This was discussed at some point on one of the versions of my patch. The tricky part is in how to get it implemented correctly.

We need to be able to correctly handle DWIM logic for things, and ensure that what we're operating on actually looks "note-like" since we don't really want to perform read-only ops on refs that don't hold notes like objects.

Regards, Jake

Johan Herland· Nov 16, 2015, 07:55 UTC · re: Jacob Keller · lore

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

On Mon, Nov 16, 2015 at 12:23 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
Show 14 quoted lines
> On Sun, Nov 15, 2015 at 2:14 PM, Johan Herland <johan@herland.net> wrote:
>> A related topic that has been discussed (although I cannot remember if
>> any conclusion was reached) is whether to allow more notes operations
>> - specifically _read-only_ operations - on notes trees outside
>> refs/notes/. I believe this should also become possible, although I
>> haven't thoroughly examined all implications.
>
> This was discussed at some point on one of the versions of my patch.
> The tricky part is in how to get it implemented correctly.
>
> We need to be able to correctly handle DWIM logic for things, and
> ensure that what we're operating on actually looks "note-like" since
> we don't really want to perform read-only ops on refs that don't hold
> notes like objects.

I believe read-only operations on non-notes trees is harmless (although suboptimal). When reading in a notes tree, the notes code maintains non-note entries in a sorted linked list. Only paths that contain exactly 40 hex characters (modulo '/') ends up as "notes" (i.e. false positives). The rest ends up in the non-notes list. The overwhelming majority of non-notes trees will have no "notes" in them (zero false positives).

For those few trees that do contain note-like paths: since we never write out the tree again, we don't end up corrupting the non-notes tree itself (which would typically look like changing the "fanout" of note-like paths, e.g. moving 'de/adbeef...' to 'deadbeef...'). Hence, the only damage we can get from reading in a non-notes tree depend on what we subsequently do with the "notes" information read from that tree.

Again, since the number of "notes" read from a non-notes tree is typically zero, the subsequent damage is typically, also, zero.

For "git notes merge", false positives from a non-notes tree are merged into the first (proper) notes tree.

For "git log --notes", false positives end up being displayed as part of the output. Note that here, a false positive must not only match the above criteria (40 hex chars, modulo '/'), but must also correctly name a commit that occurs in the log.

Are there other cases where a false positive would wreak considerable havoc?

Additionally, if we suspect that passing non-notes trees to read-only operations will be a common error, we could add a simple heuristic to the notes code, to warn (or even abort) if we strongly suspect that we are reading in a non-notes tree. For example, if the ratio of non-notes to notes entries goes above, say, 1:1 (or even 10:1), then what we're reading is probably not a proper notes tree...

...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Jacob Keller· Nov 16, 2015, 19:41 UTC · re: Johan Herland · lore

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

On Sun, Nov 15, 2015 at 11:55 PM, Johan Herland <johan@herland.net> wrote:
Show 7 quoted lines
> Additionally, if we suspect that passing non-notes trees to read-only
> operations will be a common error, we could add a simple heuristic to
> the notes code, to warn (or even abort) if we strongly suspect that we
> are reading in a non-notes tree. For example, if the ratio of
> non-notes to notes entries goes above, say, 1:1 (or even 10:1), then
> what we're reading is probably not a proper notes tree...
>

I agree here for this part, a possible heuristic check would maybe be valuable.. but not sure it's super worth the effort. I doubt it would be a common error, and I don't think the issues above would actually cause too many problems.

The main other issue is how to get notes DWIM things to work for all cases where we want to use notes refs, since right now the DWIM is basically done at the top level and only handles notes like things. The problem with it is that if you specify a full ref that *isn't* refs/notes, you will always prefix it with refs/notes, like so:

refs/remote-notes/origin => refs/notes/refs/remote-notes/origin,

This makes it really difficult to expand a ref. However, Julio seemed to think this was a possibly valuable expansion under normal circumstances. The current solution is to try to do a normal lookup first and only use the notes DWIM after we fail a lookup, which I think is what the above patch attempts to do. This seems ok enough to me.

Regards, Jake

>
> ...Johan
>
Johan Herland· Nov 18, 2015, 22:29 UTC · re: Jacob Keller · lore

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

On Mon, Nov 16, 2015 at 8:41 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
Show 7 quoted lines
> The main other issue is how to get notes DWIM things to work for all
> cases where we want to use notes refs, since right now the DWIM is
> basically done at the top level and only handles notes like things.
> The problem with it is that if you specify a full ref that *isn't*
> refs/notes, you will always prefix it with refs/notes, like so:
>
> refs/remote-notes/origin => refs/notes/refs/remote-notes/origin,

I am becoming convinced that this is a bug. I don't know anywhere else in Git, where a fully qualified ref name (i.e. anything starting with "refs/") is not interpreted verbatim. For the notes code to do just that adds unnecessary confusion.

> This makes it really difficult to expand a ref. However, Junio seemed
> to think this was a possibly valuable expansion under normal
> circumstances.

I doubt it. It carries its own set of problems in that refs/foo/bar (=> refs/notes/refs/foo/bar) is treated differently from refs/notes/bar (=> refs/notes/bar). If users _really_ want to create refs/notes/refs/$whatever, they should have to be explicit about that (i.e. we should require them to say refs/notes/refs/$whatever instead of allowing them to lazily say refs/$whatever). (It even saves them from a potential bug if their $whatever happens to start with "notes/", in which case the current code already forces them to fully qualify...)

I realize this is a backwards-incompatible change in behavior, but I don't think it'll matter much in practice. Given e.g.

  git notes --ref refs/foo list
when refs/foo and refs/notes/refs/foo is both missing:
  Current behavior: refs/notes/refs/foo lookup fails.
    Treat like empty notes tree; no output, exit code 0
  Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo
    lookup fails. Same behavior as current.
when refs/notes/refs/foo exists:
  Current behavior: refs/notes/refs/foo lookup succeeds.
    Shows notes in that tree
  Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo
    lookup succeeds. Same as current.
when refs/foo exists:
  Current behavior: refs/notes/refs/foo lookup fails. Treat like empty
    notes tree; no output, exit code 0
  Proposed behavior: refs/foo lookup succeeds. Load as notes tree,
    probably empty, hence no output, exit code 0
when both refs/foo and refs/notes/refs/foo exist:
  Current behavior: refs/notes/refs/foo lookup succeeds. Shows notes
    in that tree
  Proposed behavior: refs/foo lookup succeeds. Load as notes tree,
    probably empty, hence no output, exit code 0

In other words, this change requires both refs/foo and refs/notes/refs/foo to be present in order to cause any real confusion. And in that case, the proposed behavior forces you to use fully- qualified refs (which will be interpreted as such) whereas the current behavior takes what looks like a fully-qualified ref (refs/foo) and interprets it like a notes-shorthand (-> refs/notes/refs/foo), which I argue is probably more confusing to most users.

> The current solution is to try to do a normal lookup
> first and only use the notes DWIM after we fail a lookup, which I
> think is what the above patch attempts to do. This seems ok enough to
> me.

Yes, given $whatever, we should first lookup $whatever, and only failing that, we should try refs/notes/$whatever. Maybe it's also worth trying refs/$whatever (before refs/notes/$whatever), since that would be consistent with what's currently done for other refs (e.g. try "git log heads/master" or "git log tags/v2.6.3" in git.git).

...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Jacob Keller· Nov 18, 2015, 22:38 UTC · re: Johan Herland · lore

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

On Wed, Nov 18, 2015 at 2:29 PM, Johan Herland <johan@herland.net> wrote:
Show 14 quoted lines
> On Mon, Nov 16, 2015 at 8:41 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> The main other issue is how to get notes DWIM things to work for all
>> cases where we want to use notes refs, since right now the DWIM is
>> basically done at the top level and only handles notes like things.
>> The problem with it is that if you specify a full ref that *isn't*
>> refs/notes, you will always prefix it with refs/notes, like so:
>>
>> refs/remote-notes/origin => refs/notes/refs/remote-notes/origin,
>
> I am becoming convinced that this is a bug. I don't know anywhere else
> in Git, where a fully qualified ref name (i.e. anything starting with
> "refs/") is not interpreted verbatim. For the notes code to do just
> that adds unnecessary confusion.
>

I agree, and I had a patch to change this behavior, but the main issue being I think it didn't fallback to the suggested proposal below.

Show 13 quoted lines
>> This makes it really difficult to expand a ref. However, Junio seemed
>> to think this was a possibly valuable expansion under normal
>> circumstances.
>
> I doubt it. It carries its own set of problems in that refs/foo/bar
> (=> refs/notes/refs/foo/bar) is treated differently from refs/notes/bar
> (=> refs/notes/bar). If users _really_ want to create
> refs/notes/refs/$whatever, they should have to be explicit about that
> (i.e. we should require them to say refs/notes/refs/$whatever instead
> of allowing them to lazily say refs/$whatever). (It even saves them
> from a potential bug if their $whatever happens to start with "notes/",
> in which case the current code already forces them to fully qualify...)
>
The question is whether we do:
a) check for refs/abc/xyz and fail if we can't find it or

b) check for refs/abc/xyz and then expand to refs/notes/refs/abc/xyz if we can't?

I think that the first is generally preferable but with read-only ops it's not a big deal since we won't be writing to notes trees.

Show 49 quoted lines
> I realize this is a backwards-incompatible change in behavior, but I
> don't think it'll matter much in practice. Given e.g.
>
>   git notes --ref refs/foo list
>
> when refs/foo and refs/notes/refs/foo is both missing:
>   Current behavior: refs/notes/refs/foo lookup fails.
>     Treat like empty notes tree; no output, exit code 0
>   Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo
>     lookup fails. Same behavior as current.
>
> when refs/notes/refs/foo exists:
>   Current behavior: refs/notes/refs/foo lookup succeeds.
>     Shows notes in that tree
>   Proposed behavior: refs/foo lookup fails -> refs/notes/refs/foo
>     lookup succeeds. Same as current.
>
> when refs/foo exists:
>   Current behavior: refs/notes/refs/foo lookup fails. Treat like empty
>     notes tree; no output, exit code 0
>   Proposed behavior: refs/foo lookup succeeds. Load as notes tree,
>     probably empty, hence no output, exit code 0
>
> when both refs/foo and refs/notes/refs/foo exist:
>   Current behavior: refs/notes/refs/foo lookup succeeds. Shows notes
>     in that tree
>   Proposed behavior: refs/foo lookup succeeds. Load as notes tree,
>     probably empty, hence no output, exit code 0
>
> In other words, this change requires both refs/foo and
> refs/notes/refs/foo to be present in order to cause any real confusion.
> And in that case, the proposed behavior forces you to use fully-
> qualified refs (which will be interpreted as such) whereas the current
> behavior takes what looks like a fully-qualified ref (refs/foo) and
> interprets it like a notes-shorthand (-> refs/notes/refs/foo), which
> I argue is probably more confusing to most users.
>
>> The current solution is to try to do a normal lookup
>> first and only use the notes DWIM after we fail a lookup, which I
>> think is what the above patch attempts to do. This seems ok enough to
>> me.
>
> Yes, given $whatever, we should first lookup $whatever, and only
> failing that, we should try refs/notes/$whatever. Maybe it's also
> worth trying refs/$whatever (before refs/notes/$whatever), since that
> would be consistent with what's currently done for other refs (e.g.
> try "git log heads/master" or "git log tags/v2.6.3" in git.git).
>
>

The biggest implementation issue here is that the notes code that does DWIM happens before lookup of whether the ref exists, and the code that does lookup of refs in the notes.c code won't fallback and try a different expansion.

I think that is why my proposed change was dropped, if I remember correctly.

I am in agreement with you, and think we should use the proposed behavior above, as it is very unlikely to cause any issues with current cases, especially since we already try not to allow operation on refs outside of the notes tree today.

Regards, Jake

Show 5 quoted lines
> ...Johan
>
> --
> Johan Herland, <johan@herland.net>
> www.herland.net
Jeff King· Nov 24, 2015, 22:47 UTC · re: Jacob Keller · lore

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

On Fri, Nov 13, 2015 at 08:34:22AM -0800, Jacob Keller wrote:
> ---
> I do not remember what version this was since it has been an age ago
> that I sent the previous code. This is mostly just a rebase onto current
> next. I believe I have covered everything previous reviewers noted.

Please keep topics branched from master where possible. And if not possible, please indicate which topic in 'next' is required to build on.

We never merge 'next' itself, only individual topics from it. So I can't just apply your patch on top of 'next'.

I did get it to apply on the current master with "am -3", but some tests in t3310 seem to fail. Can you take a look?

I skimmed the discussion with Johan that followed this. Are we happy with this as a first step, or would people rather look at re-working the notes-ref lookups everywhere?

-Peff
Jeff King· Nov 24, 2015, 23:42 UTC · re: Jeff King · lore

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

On Tue, Nov 24, 2015 at 05:47:09PM -0500, Jeff King wrote:
Show 15 quoted lines
> On Fri, Nov 13, 2015 at 08:34:22AM -0800, Jacob Keller wrote:
> 
> > ---
> > I do not remember what version this was since it has been an age ago
> > that I sent the previous code. This is mostly just a rebase onto current
> > next. I believe I have covered everything previous reviewers noted.
> 
> Please keep topics branched from master where possible. And if not
> possible, please indicate which topic in 'next' is required to build on.
> 
> We never merge 'next' itself, only individual topics from it. So I can't
> just apply your patch on top of 'next'.
> 
> I did get it to apply on the current master with "am -3", but some tests
> in t3310 seem to fail. Can you take a look?

I just noticed v2, which I missed earlier. But the same complaints apply. :)

-Peff
Jacob Keller· Nov 26, 2015, 06:20 UTC · re: Jeff King · lore

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

On Tue, Nov 24, 2015 at 3:42 PM, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> On Tue, Nov 24, 2015 at 05:47:09PM -0500, Jeff King wrote:
>
>> On Fri, Nov 13, 2015 at 08:34:22AM -0800, Jacob Keller wrote:
>>
>> > ---
>> > I do not remember what version this was since it has been an age ago
>> > that I sent the previous code. This is mostly just a rebase onto current
>> > next. I believe I have covered everything previous reviewers noted.
>>
>> Please keep topics branched from master where possible. And if not
>> possible, please indicate which topic in 'next' is required to build on.
>>
>> We never merge 'next' itself, only individual topics from it. So I can't
>> just apply your patch on top of 'next'.
>>
>> I did get it to apply on the current master with "am -3", but some tests
>> in t3310 seem to fail. Can you take a look?
>
> I just noticed v2, which I missed earlier. But the same complaints
> apply. :)
>
> -Peff

Yea.. sorry about that. I normally work off next since this is what I use day to day for general git use, as I like to run the bleedy edge.

I can respin these on master, but it may take a bit of time as I am on vacation at the moment.

I'm also curious if people would rather go the more difficult route first or not.

Regards, Jake

Junio C Hamano· Dec 11, 2015, 19:47 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> On Tue, Nov 24, 2015 at 05:47:09PM -0500, Jeff King wrote:
>
>> On Fri, Nov 13, 2015 at 08:34:22AM -0800, Jacob Keller wrote:
>> 
>> > ---
>> > I do not remember what version this was since it has been an age ago
>> > that I sent the previous code. This is mostly just a rebase onto current
>> > next. I believe I have covered everything previous reviewers noted.
>> 
>> Please keep topics branched from master where possible. And if not
>> possible, please indicate which topic in 'next' is required to build on.
>> 
>> We never merge 'next' itself, only individual topics from it. So I can't
>> just apply your patch on top of 'next'.
>> 
>> I did get it to apply on the current master with "am -3", but some tests
>> in t3310 seem to fail. Can you take a look?
>
> I just noticed v2, which I missed earlier. But the same complaints
> apply. :)

I tried to queue <1447432462-21192-1-git-send-email-jacob.e.keller@intel.com> from Nov 13 directly on top of 'master', and t3310 does seem to fail.

I'll discard the topic for now, expecting the resurrection sometime later.

Jacob Keller· Dec 11, 2015, 19:58 UTC · re: Junio C Hamano · lore

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

On Fri, Dec 11, 2015 at 11:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I'll discard the topic for now, expecting the resurrection sometime
> later.
>
Hi,

Yes I fully intend to work this against master once I have some time again. It may be a few weeks as I am extra busy for the holidays, and the next posting will be fresh on the tip of the future-master :)

Regards, Jake

← back to recent threads