* [PATCHv4 01/21] notes.c: Hexify SHA1 in die() message from init_notes()
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 02/21] (trivial) notes.h: Minor documentation fixes to copy_notes() Johan Herland
` (20 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
Signed-off-by: Johan Herland <johan@herland.net>
---
notes.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/notes.c b/notes.c
index 1978244..bb03eb0 100644
--- a/notes.c
+++ b/notes.c
@@ -940,7 +940,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
return;
if (get_tree_entry(object_sha1, "", sha1, &mode))
die("Failed to read notes tree referenced by %s (%s)",
- notes_ref, object_sha1);
+ notes_ref, sha1_to_hex(object_sha1));
hashclr(root_tree.key_sha1);
hashcpy(root_tree.val_sha1, sha1);
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 02/21] (trivial) notes.h: Minor documentation fixes to copy_notes()
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
2010-10-21 2:08 ` [PATCHv4 01/21] notes.c: Hexify SHA1 in die() message from init_notes() Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 03/21] notes.h: Make default_notes_ref() available in notes API Johan Herland
` (19 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
Signed-off-by: Johan Herland <johan@herland.net>
---
notes.h | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/notes.h b/notes.h
index 65fc3a6..c0288b0 100644
--- a/notes.h
+++ b/notes.h
@@ -104,6 +104,10 @@ const unsigned char *get_note(struct notes_tree *t,
* Copy a note from one object to another in the given notes_tree.
*
* Fails if the to_obj already has a note unless 'force' is true.
+ *
+ * IMPORTANT: The changes made by copy_note() to the given notes_tree structure
+ * are not persistent until a subsequent call to write_notes_tree() returns
+ * zero.
*/
int copy_note(struct notes_tree *t,
const unsigned char *from_obj, const unsigned char *to_obj,
@@ -149,6 +153,7 @@ int copy_note(struct notes_tree *t,
* notes tree) from within the callback:
* - add_note()
* - remove_note()
+ * - copy_note()
* - free_notes()
*/
typedef int each_note_fn(const unsigned char *object_sha1,
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 03/21] notes.h: Make default_notes_ref() available in notes API
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
2010-10-21 2:08 ` [PATCHv4 01/21] notes.c: Hexify SHA1 in die() message from init_notes() Johan Herland
2010-10-21 2:08 ` [PATCHv4 02/21] (trivial) notes.h: Minor documentation fixes to copy_notes() Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 04/21] notes.c: Reorder functions in preparation for next commit Johan Herland
` (18 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
Signed-off-by: Johan Herland <johan@herland.net>
---
notes.c | 2 +-
notes.h | 14 ++++++++++++++
2 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/notes.c b/notes.c
index bb03eb0..d71c0a3 100644
--- a/notes.c
+++ b/notes.c
@@ -898,7 +898,7 @@ static int notes_display_config(const char *k, const char *v, void *cb)
return 0;
}
-static const char *default_notes_ref(void)
+const char *default_notes_ref(void)
{
const char *notes_ref = NULL;
if (!notes_ref)
diff --git a/notes.h b/notes.h
index c0288b0..20db42f 100644
--- a/notes.h
+++ b/notes.h
@@ -44,6 +44,20 @@ extern struct notes_tree {
} default_notes_tree;
/*
+ * Return the default notes ref.
+ *
+ * The default notes ref is the notes ref that is used when notes_ref == NULL
+ * is passed to init_notes().
+ *
+ * This the first of the following to be defined:
+ * 1. The '--ref' option to 'git notes', if given
+ * 2. The $GIT_NOTES_REF environment variable, if set
+ * 3. The value of the core.notesRef config variable, if set
+ * 4. GIT_NOTES_DEFAULT_REF (i.e. "refs/notes/commits")
+ */
+const char *default_notes_ref(void);
+
+/*
* Flags controlling behaviour of notes tree initialization
*
* Default behaviour is to initialize the notes tree from the tree object
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 04/21] notes.c: Reorder functions in preparation for next commit
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (2 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 03/21] notes.h: Make default_notes_ref() available in notes API Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 05/21] notes.h/c: Clarify the handling of notes objects that are == null_sha1 Johan Herland
` (17 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
This patch introduces no functional change. It consists solely of reordering
functions in notes.c to avoid use-before-declaration errors after applying
the next commit in this series.
Signed-off-by: Johan Herland <johan@herland.net>
---
notes.c | 146 +++++++++++++++++++++++++++++++-------------------------------
1 files changed, 73 insertions(+), 73 deletions(-)
diff --git a/notes.c b/notes.c
index d71c0a3..bfb3ea5 100644
--- a/notes.c
+++ b/notes.c
@@ -150,6 +150,79 @@ static struct leaf_node *note_tree_find(struct notes_tree *t,
}
/*
+ * How to consolidate an int_node:
+ * If there are > 1 non-NULL entries, give up and return non-zero.
+ * Otherwise replace the int_node at the given index in the given parent node
+ * with the only entry (or a NULL entry if no entries) from the given tree,
+ * and return 0.
+ */
+static int note_tree_consolidate(struct int_node *tree,
+ struct int_node *parent, unsigned char index)
+{
+ unsigned int i;
+ void *p = NULL;
+
+ assert(tree && parent);
+ assert(CLR_PTR_TYPE(parent->a[index]) == tree);
+
+ for (i = 0; i < 16; i++) {
+ if (GET_PTR_TYPE(tree->a[i]) != PTR_TYPE_NULL) {
+ if (p) /* more than one entry */
+ return -2;
+ p = tree->a[i];
+ }
+ }
+
+ /* replace tree with p in parent[index] */
+ parent->a[index] = p;
+ free(tree);
+ return 0;
+}
+
+/*
+ * To remove a leaf_node:
+ * Search to the tree location appropriate for the given leaf_node's key:
+ * - If location does not hold a matching entry, abort and do nothing.
+ * - Replace the matching leaf_node with a NULL entry (and free the leaf_node).
+ * - Consolidate int_nodes repeatedly, while walking up the tree towards root.
+ */
+static void note_tree_remove(struct notes_tree *t, struct int_node *tree,
+ unsigned char n, struct leaf_node *entry)
+{
+ struct leaf_node *l;
+ struct int_node *parent_stack[20];
+ unsigned char i, j;
+ void **p = note_tree_search(t, &tree, &n, entry->key_sha1);
+
+ assert(GET_PTR_TYPE(entry) == 0); /* no type bits set */
+ if (GET_PTR_TYPE(*p) != PTR_TYPE_NOTE)
+ return; /* type mismatch, nothing to remove */
+ l = (struct leaf_node *) CLR_PTR_TYPE(*p);
+ if (hashcmp(l->key_sha1, entry->key_sha1))
+ return; /* key mismatch, nothing to remove */
+
+ /* we have found a matching entry */
+ free(l);
+ *p = SET_PTR_TYPE(NULL, PTR_TYPE_NULL);
+
+ /* consolidate this tree level, and parent levels, if possible */
+ if (!n)
+ return; /* cannot consolidate top level */
+ /* first, build stack of ancestors between root and current node */
+ parent_stack[0] = t->root;
+ for (i = 0; i < n; i++) {
+ j = GET_NIBBLE(i, entry->key_sha1);
+ parent_stack[i + 1] = CLR_PTR_TYPE(parent_stack[i]->a[j]);
+ }
+ assert(i == n && parent_stack[i] == tree);
+ /* next, unwind stack until note_tree_consolidate() is done */
+ while (i > 0 &&
+ !note_tree_consolidate(parent_stack[i], parent_stack[i - 1],
+ GET_NIBBLE(i - 1, entry->key_sha1)))
+ i--;
+}
+
+/*
* To insert a leaf_node:
* Search to the tree location appropriate for the given leaf_node's key:
* - If location is unused (NULL), store the tweaked pointer directly there
@@ -229,79 +302,6 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
note_tree_insert(t, new_node, n + 1, entry, type, combine_notes);
}
-/*
- * How to consolidate an int_node:
- * If there are > 1 non-NULL entries, give up and return non-zero.
- * Otherwise replace the int_node at the given index in the given parent node
- * with the only entry (or a NULL entry if no entries) from the given tree,
- * and return 0.
- */
-static int note_tree_consolidate(struct int_node *tree,
- struct int_node *parent, unsigned char index)
-{
- unsigned int i;
- void *p = NULL;
-
- assert(tree && parent);
- assert(CLR_PTR_TYPE(parent->a[index]) == tree);
-
- for (i = 0; i < 16; i++) {
- if (GET_PTR_TYPE(tree->a[i]) != PTR_TYPE_NULL) {
- if (p) /* more than one entry */
- return -2;
- p = tree->a[i];
- }
- }
-
- /* replace tree with p in parent[index] */
- parent->a[index] = p;
- free(tree);
- return 0;
-}
-
-/*
- * To remove a leaf_node:
- * Search to the tree location appropriate for the given leaf_node's key:
- * - If location does not hold a matching entry, abort and do nothing.
- * - Replace the matching leaf_node with a NULL entry (and free the leaf_node).
- * - Consolidate int_nodes repeatedly, while walking up the tree towards root.
- */
-static void note_tree_remove(struct notes_tree *t, struct int_node *tree,
- unsigned char n, struct leaf_node *entry)
-{
- struct leaf_node *l;
- struct int_node *parent_stack[20];
- unsigned char i, j;
- void **p = note_tree_search(t, &tree, &n, entry->key_sha1);
-
- assert(GET_PTR_TYPE(entry) == 0); /* no type bits set */
- if (GET_PTR_TYPE(*p) != PTR_TYPE_NOTE)
- return; /* type mismatch, nothing to remove */
- l = (struct leaf_node *) CLR_PTR_TYPE(*p);
- if (hashcmp(l->key_sha1, entry->key_sha1))
- return; /* key mismatch, nothing to remove */
-
- /* we have found a matching entry */
- free(l);
- *p = SET_PTR_TYPE(NULL, PTR_TYPE_NULL);
-
- /* consolidate this tree level, and parent levels, if possible */
- if (!n)
- return; /* cannot consolidate top level */
- /* first, build stack of ancestors between root and current node */
- parent_stack[0] = t->root;
- for (i = 0; i < n; i++) {
- j = GET_NIBBLE(i, entry->key_sha1);
- parent_stack[i + 1] = CLR_PTR_TYPE(parent_stack[i]->a[j]);
- }
- assert(i == n && parent_stack[i] == tree);
- /* next, unwind stack until note_tree_consolidate() is done */
- while (i > 0 &&
- !note_tree_consolidate(parent_stack[i], parent_stack[i - 1],
- GET_NIBBLE(i - 1, entry->key_sha1)))
- i--;
-}
-
/* Free the entire notes data contained in the given tree */
static void note_tree_free(struct int_node *tree)
{
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 05/21] notes.h/c: Clarify the handling of notes objects that are == null_sha1
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (3 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 04/21] notes.c: Reorder functions in preparation for next commit Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 5:12 ` Jonathan Nieder
2010-10-21 2:08 ` [PATCHv4 06/21] notes.h/c: Propagate combine_notes_fn return value to add_note() and beyond Johan Herland
` (16 subsequent siblings)
21 siblings, 1 reply; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
Clearly specify how combine_notes functions are expected to handle null_sha1
in input. Also specify (and implement) that returning null_sha1 from a
combine_notes function will cause the note in question to be removed.
Furthermore, document that passing note_sha1 == null_sha1 to add_note() is
usually a no-op, except in cases where combining it with an existing note
yields a new/changed result.
Signed-off-by: Johan Herland <johan@herland.net>
---
notes.c | 12 +++++++++++-
notes.h | 16 +++++++++++++++-
2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/notes.c b/notes.c
index bfb3ea5..0c13a36 100644
--- a/notes.c
+++ b/notes.c
@@ -248,7 +248,10 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
switch (GET_PTR_TYPE(*p)) {
case PTR_TYPE_NULL:
assert(!*p);
- *p = SET_PTR_TYPE(entry, type);
+ if (is_null_sha1(entry->val_sha1))
+ free(entry);
+ else
+ *p = SET_PTR_TYPE(entry, type);
return;
case PTR_TYPE_NOTE:
switch (type) {
@@ -264,6 +267,9 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
sha1_to_hex(l->val_sha1),
sha1_to_hex(entry->val_sha1),
sha1_to_hex(l->key_sha1));
+
+ if (is_null_sha1(l->val_sha1))
+ note_tree_remove(t, tree, n, entry);
free(entry);
return;
}
@@ -295,6 +301,10 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
/* non-matching leaf_node */
assert(GET_PTR_TYPE(*p) == PTR_TYPE_NOTE ||
GET_PTR_TYPE(*p) == PTR_TYPE_SUBTREE);
+ if (is_null_sha1(entry->val_sha1)) { /* skip insertion of empty note */
+ free(entry);
+ return;
+ }
new_node = (struct int_node *) xcalloc(sizeof(struct int_node), 1);
note_tree_insert(t, new_node, n + 1, l, GET_PTR_TYPE(*p),
combine_notes);
diff --git a/notes.h b/notes.h
index 20db42f..79ea797 100644
--- a/notes.h
+++ b/notes.h
@@ -12,7 +12,10 @@
* resulting SHA1 into the first SHA1 argument (cur_sha1). A non-zero return
* value indicates failure.
*
- * The two given SHA1s must both be non-NULL and different from each other.
+ * The two given SHA1s shall both be non-NULL and different from each other.
+ * Either of them (but not both) may be == null_sha1, which indicates an
+ * empty/non-existent note. If the resulting SHA1 (cur_sha1) is == null_sha1,
+ * the note will be removed from the notes tree.
*
* The default combine_notes function (you get this when passing NULL) is
* combine_notes_concatenate(), which appends the contents of the new note to
@@ -90,6 +93,17 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
/*
* Add the given note object to the given notes_tree structure
*
+ * If there already exists a note for the given object_sha1, the given
+ * combine_notes function is invoked to break the tie. If not given (i.e.
+ * combine_notes == NULL), the default combine_notes function for the given
+ * notes_tree is used.
+ *
+ * Passing note_sha1 == null_sha1 indicates the addition of an
+ * empty/non-existent note. This is a (potentially expensive) no-op unless
+ * there already exists a note for the given object_sha1, AND combining that
+ * note with the empty note (using the given combine_notes function) results
+ * in a new/changed note.
+ *
* IMPORTANT: The changes made by add_note() to the given notes_tree structure
* are not persistent until a subsequent call to write_notes_tree() returns
* zero.
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCHv4 05/21] notes.h/c: Clarify the handling of notes objects that are == null_sha1
2010-10-21 2:08 ` [PATCHv4 05/21] notes.h/c: Clarify the handling of notes objects that are == null_sha1 Johan Herland
@ 2010-10-21 5:12 ` Jonathan Nieder
2010-10-21 13:13 ` Johan Herland
0 siblings, 1 reply; 43+ messages in thread
From: Jonathan Nieder @ 2010-10-21 5:12 UTC (permalink / raw)
To: Johan Herland; +Cc: git, bebarino, avarab, gitster
Johan Herland wrote:
> Clearly specify how combine_notes functions are expected to handle null_sha1
> in input. Also specify (and implement) that returning null_sha1 from a
> combine_notes function will cause the note in question to be removed.
Ack again on patches 1-4. As for this one, I still think the log message
does not make the goal obvious.
1. Clearly specify how combine_notes functions are expected to
handle null_sha1 in input.
Wasn't it already clear? I guess you mean that the documentation was
updated. But surely that is less important than:
2. Also specify (and implement) that returning null_sha1 from a
combine_notes function will ...
A person reading this for the first time could be forgiven for thinking
this is like (1), i.e., documenting an edge case. But actually it's
the main point, and the part I omitted with "..." is the important
part!
Why not say something like:
Allow combine_notes functions to request that a note be
removed, by returning the object id of the empty blob.
For consistency, also teach note_tree_insert() to skip
insertion of an empty note when there is no note to
combine it with.
In general, an empty note is treated identically to no
note at all: for example, when merging two notes trees,
one of which does not have a certain note, combine_notes()
will be called as though that tree had an empty note
instead. Document this.
The above includes guesses, so please do not use it verbatim
unless it's true. :)
Of course these are minor nitpicks as compared to the content of
the patch itself. The patch still looks good.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCHv4 05/21] notes.h/c: Clarify the handling of notes objects that are == null_sha1
2010-10-21 5:12 ` Jonathan Nieder
@ 2010-10-21 13:13 ` Johan Herland
2010-10-21 17:54 ` Jonathan Nieder
0 siblings, 1 reply; 43+ messages in thread
From: Johan Herland @ 2010-10-21 13:13 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, bebarino, avarab, gitster
On Thursday 21 October 2010, Jonathan Nieder wrote:
> Johan Herland wrote:
> > Clearly specify how combine_notes functions are expected to handle
> > null_sha1 in input. Also specify (and implement) that returning
> > null_sha1 from a combine_notes function will cause the note in
> > question to be removed.
>
> Ack again on patches 1-4. As for this one, I still think the log
> message does not make the goal obvious.
>
> [...]
Ok, I liked your alternative message much better. The next iteration
will have this commit message:
notes.h/c: Allow combine_notes functions to remove notes
Allow combine_notes functions to request that a note be removed,
by setting the resulting note SHA1 to null_sha1 (0000000...).
For consistency, also teach note_tree_insert() to skip insertion
of an empty note when there is no note to combine it with.
In general, an empty note is treated identically to no note at
all, but when adding an empty note where there already exists a
non-empty note, we allow the combine_notes function to potentially
record a new/changed note. Document this behaviour, and clearly
specify how combine_notes functions are expected to handle null_sha1
in input.
Thanks again, for your superhuman patience in reviewing.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCHv4 05/21] notes.h/c: Clarify the handling of notes objects that are == null_sha1
2010-10-21 13:13 ` Johan Herland
@ 2010-10-21 17:54 ` Jonathan Nieder
2010-10-22 13:32 ` Johan Herland
0 siblings, 1 reply; 43+ messages in thread
From: Jonathan Nieder @ 2010-10-21 17:54 UTC (permalink / raw)
To: Johan Herland; +Cc: git, bebarino, avarab, gitster
Johan Herland wrote:
> Allow combine_notes functions to request that a note be removed,
> by setting the resulting note SHA1 to null_sha1 (0000000...).
>
> For consistency, also teach note_tree_insert() to skip insertion
> of an empty note when there is no note to combine it with.
I guess it's probably also worth clarifying that the term "empty note"
means null_sha1 rather than empty_blob_sha1.
i.e. something like:
For consistency, also teach note_tree_insert() to skip insertion
of an empty note (a note with val_sha1 equal to null_sha1)
when there is no note to combine it with.
The previous behavior iiuc would have been for write_notes_tree() to
write invalid trees after such an operation.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCHv4 05/21] notes.h/c: Clarify the handling of notes objects that are == null_sha1
2010-10-21 17:54 ` Jonathan Nieder
@ 2010-10-22 13:32 ` Johan Herland
0 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-22 13:32 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, bebarino, avarab, gitster
On Thursday 21 October 2010, Jonathan Nieder wrote:
> Johan Herland wrote:
> > Allow combine_notes functions to request that a note be removed,
> > by setting the resulting note SHA1 to null_sha1 (0000000...).
> >
> > For consistency, also teach note_tree_insert() to skip insertion
> > of an empty note when there is no note to combine it with.
>
> I guess it's probably also worth clarifying that the term "empty
> note" means null_sha1 rather than empty_blob_sha1.
>
> i.e. something like:
>
> For consistency, also teach note_tree_insert() to skip insertion
> of an empty note (a note with val_sha1 equal to null_sha1)
> when there is no note to combine it with.
Agreed. Incorporated.
> The previous behavior iiuc would have been for write_notes_tree() to
> write invalid trees after such an operation.
Indeed, you are correct. I've added a note about this to the commit
message.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv4 06/21] notes.h/c: Propagate combine_notes_fn return value to add_note() and beyond
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (4 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 05/21] notes.h/c: Clarify the handling of notes objects that are == null_sha1 Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 5:21 ` Jonathan Nieder
2010-10-21 2:08 ` [PATCHv4 07/21] (trivial) t3303: Indent with tabs instead of spaces for consistency Johan Herland
` (15 subsequent siblings)
21 siblings, 1 reply; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
The combine_notes_fn functions uses a non-zero return value to indicate
failure. However, this return value was converted to a call to die()
in note_tree_insert().
Instead, propagate this return value out to add_note(), and return it
from there to enable the caller to handle errors appropriately.
Existing add_note() callers are updated to die() upon failure, thus
preserving the current behaviour. The only exceptions are copy_note()
and notes_cache_put() where we are able to propagate the add_note()
return value instead.
This patch has been improved by the following contributions:
- Jonathan Nieder: Future-proof by always checking add_note() return value
Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin/notes.c | 11 +++++----
notes-cache.c | 3 +-
notes.c | 59 ++++++++++++++++++++++++++++--------------------------
notes.h | 11 +++++++--
4 files changed, 46 insertions(+), 38 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index fbc347c..35f6eb6 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -573,8 +573,8 @@ static int add(int argc, const char **argv, const char *prefix)
if (is_null_sha1(new_note))
remove_note(t, object);
- else
- add_note(t, object, new_note, combine_notes_overwrite);
+ else if (add_note(t, object, new_note, combine_notes_overwrite))
+ die("confused: combine_notes_overwrite failed");
snprintf(logmsg, sizeof(logmsg), "Notes %s by 'git notes %s'",
is_null_sha1(new_note) ? "removed" : "added", "add");
@@ -653,7 +653,8 @@ static int copy(int argc, const char **argv, const char *prefix)
goto out;
}
- add_note(t, object, from_note, combine_notes_overwrite);
+ if (add_note(t, object, from_note, combine_notes_overwrite))
+ die("confused: combine_notes_overwrite failed");
commit_notes(t, "Notes added by 'git notes copy'");
out:
free_notes(t);
@@ -712,8 +713,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
if (is_null_sha1(new_note))
remove_note(t, object);
- else
- add_note(t, object, new_note, combine_notes_overwrite);
+ else if (add_note(t, object, new_note, combine_notes_overwrite))
+ die("confused: combine_notes_overwrite failed");
snprintf(logmsg, sizeof(logmsg), "Notes %s by 'git notes %s'",
is_null_sha1(new_note) ? "removed" : "added", argv[0]);
diff --git a/notes-cache.c b/notes-cache.c
index dee6d62..4c8984e 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -89,6 +89,5 @@ int notes_cache_put(struct notes_cache *c, unsigned char key_sha1[20],
if (write_sha1_file(data, size, "blob", value_sha1) < 0)
return -1;
- add_note(&c->tree, key_sha1, value_sha1, NULL);
- return 0;
+ return add_note(&c->tree, key_sha1, value_sha1, NULL);
}
diff --git a/notes.c b/notes.c
index 0c13a36..c4203ce 100644
--- a/notes.c
+++ b/notes.c
@@ -235,13 +235,14 @@ static void note_tree_remove(struct notes_tree *t, struct int_node *tree,
* - Else, create a new int_node, holding both the node-at-location and the
* node-to-be-inserted, and store the new int_node into the location.
*/
-static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
+static int note_tree_insert(struct notes_tree *t, struct int_node *tree,
unsigned char n, struct leaf_node *entry, unsigned char type,
combine_notes_fn combine_notes)
{
struct int_node *new_node;
struct leaf_node *l;
void **p = note_tree_search(t, &tree, &n, entry->key_sha1);
+ int ret = 0;
assert(GET_PTR_TYPE(entry) == 0); /* no type bits set */
l = (struct leaf_node *) CLR_PTR_TYPE(*p);
@@ -252,26 +253,21 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
free(entry);
else
*p = SET_PTR_TYPE(entry, type);
- return;
+ return 0;
case PTR_TYPE_NOTE:
switch (type) {
case PTR_TYPE_NOTE:
if (!hashcmp(l->key_sha1, entry->key_sha1)) {
/* skip concatenation if l == entry */
if (!hashcmp(l->val_sha1, entry->val_sha1))
- return;
+ return 0;
- if (combine_notes(l->val_sha1, entry->val_sha1))
- die("failed to combine notes %s and %s"
- " for object %s",
- sha1_to_hex(l->val_sha1),
- sha1_to_hex(entry->val_sha1),
- sha1_to_hex(l->key_sha1));
-
- if (is_null_sha1(l->val_sha1))
+ ret = combine_notes(l->val_sha1,
+ entry->val_sha1);
+ if (!ret && is_null_sha1(l->val_sha1))
note_tree_remove(t, tree, n, entry);
free(entry);
- return;
+ return ret;
}
break;
case PTR_TYPE_SUBTREE:
@@ -280,7 +276,7 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
/* unpack 'entry' */
load_subtree(t, entry, tree, n);
free(entry);
- return;
+ return 0;
}
break;
}
@@ -291,9 +287,8 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
*p = NULL;
load_subtree(t, l, tree, n);
free(l);
- note_tree_insert(t, tree, n, entry, type,
- combine_notes);
- return;
+ return note_tree_insert(t, tree, n, entry, type,
+ combine_notes);
}
break;
}
@@ -303,13 +298,17 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
GET_PTR_TYPE(*p) == PTR_TYPE_SUBTREE);
if (is_null_sha1(entry->val_sha1)) { /* skip insertion of empty note */
free(entry);
- return;
+ return 0;
}
new_node = (struct int_node *) xcalloc(sizeof(struct int_node), 1);
- note_tree_insert(t, new_node, n + 1, l, GET_PTR_TYPE(*p),
- combine_notes);
- *p = SET_PTR_TYPE(new_node, PTR_TYPE_INTERNAL);
- note_tree_insert(t, new_node, n + 1, entry, type, combine_notes);
+ ret = note_tree_insert(t, new_node, n + 1, l, GET_PTR_TYPE(*p),
+ combine_notes);
+ if (!ret) {
+ *p = SET_PTR_TYPE(new_node, PTR_TYPE_INTERNAL);
+ ret = note_tree_insert(t, new_node, n + 1, entry, type,
+ combine_notes);
+ }
+ return ret;
}
/* Free the entire notes data contained in the given tree */
@@ -452,8 +451,12 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
l->key_sha1[19] = (unsigned char) len;
type = PTR_TYPE_SUBTREE;
}
- note_tree_insert(t, node, n, l, type,
- combine_notes_concatenate);
+ if (note_tree_insert(t, node, n, l, type,
+ combine_notes_concatenate))
+ die("Failed to load %s %s into notes tree "
+ "from %s",
+ type == PTR_TYPE_NOTE ? "note" : "subtree",
+ sha1_to_hex(l->key_sha1), t->ref);
}
continue;
@@ -1014,7 +1017,7 @@ void init_display_notes(struct display_notes_opt *opt)
string_list_clear(&display_notes_refs, 0);
}
-void add_note(struct notes_tree *t, const unsigned char *object_sha1,
+int add_note(struct notes_tree *t, const unsigned char *object_sha1,
const unsigned char *note_sha1, combine_notes_fn combine_notes)
{
struct leaf_node *l;
@@ -1028,7 +1031,7 @@ void add_note(struct notes_tree *t, const unsigned char *object_sha1,
l = (struct leaf_node *) xmalloc(sizeof(struct leaf_node));
hashcpy(l->key_sha1, object_sha1);
hashcpy(l->val_sha1, note_sha1);
- note_tree_insert(t, t->root, 0, l, PTR_TYPE_NOTE, combine_notes);
+ return note_tree_insert(t, t->root, 0, l, PTR_TYPE_NOTE, combine_notes);
}
void remove_note(struct notes_tree *t, const unsigned char *object_sha1)
@@ -1204,7 +1207,7 @@ void format_display_notes(const unsigned char *object_sha1,
int copy_note(struct notes_tree *t,
const unsigned char *from_obj, const unsigned char *to_obj,
- int force, combine_notes_fn combine_fn)
+ int force, combine_notes_fn combine_notes)
{
const unsigned char *note = get_note(t, from_obj);
const unsigned char *existing_note = get_note(t, to_obj);
@@ -1213,9 +1216,9 @@ int copy_note(struct notes_tree *t,
return 1;
if (note)
- add_note(t, to_obj, note, combine_fn);
+ return add_note(t, to_obj, note, combine_notes);
else if (existing_note)
- add_note(t, to_obj, null_sha1, combine_fn);
+ return add_note(t, to_obj, null_sha1, combine_notes);
return 0;
}
diff --git a/notes.h b/notes.h
index 79ea797..b372575 100644
--- a/notes.h
+++ b/notes.h
@@ -104,11 +104,13 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
* note with the empty note (using the given combine_notes function) results
* in a new/changed note.
*
+ * Returns zero on success; non-zero means combine_notes failed.
+ *
* IMPORTANT: The changes made by add_note() to the given notes_tree structure
* are not persistent until a subsequent call to write_notes_tree() returns
* zero.
*/
-void add_note(struct notes_tree *t, const unsigned char *object_sha1,
+int add_note(struct notes_tree *t, const unsigned char *object_sha1,
const unsigned char *note_sha1, combine_notes_fn combine_notes);
/*
@@ -131,7 +133,10 @@ const unsigned char *get_note(struct notes_tree *t,
/*
* Copy a note from one object to another in the given notes_tree.
*
- * Fails if the to_obj already has a note unless 'force' is true.
+ * Returns 1 if the to_obj already has a note and 'force' is false. Otherwise,
+ * returns non-zero if 'force' is true, but the given combine_notes function
+ * failed to combine from_obj's note with to_obj's existing note.
+ * Returns zero on success.
*
* IMPORTANT: The changes made by copy_note() to the given notes_tree structure
* are not persistent until a subsequent call to write_notes_tree() returns
@@ -139,7 +144,7 @@ const unsigned char *get_note(struct notes_tree *t,
*/
int copy_note(struct notes_tree *t,
const unsigned char *from_obj, const unsigned char *to_obj,
- int force, combine_notes_fn combine_fn);
+ int force, combine_notes_fn combine_notes);
/*
* Flags controlling behaviour of for_each_note()
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCHv4 06/21] notes.h/c: Propagate combine_notes_fn return value to add_note() and beyond
2010-10-21 2:08 ` [PATCHv4 06/21] notes.h/c: Propagate combine_notes_fn return value to add_note() and beyond Johan Herland
@ 2010-10-21 5:21 ` Jonathan Nieder
2010-10-21 13:16 ` Johan Herland
0 siblings, 1 reply; 43+ messages in thread
From: Jonathan Nieder @ 2010-10-21 5:21 UTC (permalink / raw)
To: Johan Herland; +Cc: git, bebarino, avarab, gitster
Johan Herland wrote:
> --- a/notes.c
> +++ b/notes.c
> @@ -303,13 +298,17 @@ static void note_tree_insert(struct notes_tree *t, struct int_node *tree,
> GET_PTR_TYPE(*p) == PTR_TYPE_SUBTREE);
> if (is_null_sha1(entry->val_sha1)) { /* skip insertion of empty note */
> free(entry);
> - return;
> + return 0;
> }
> new_node = (struct int_node *) xcalloc(sizeof(struct int_node), 1);
> - note_tree_insert(t, new_node, n + 1, l, GET_PTR_TYPE(*p),
> - combine_notes);
> - *p = SET_PTR_TYPE(new_node, PTR_TYPE_INTERNAL);
> - note_tree_insert(t, new_node, n + 1, entry, type, combine_notes);
> + ret = note_tree_insert(t, new_node, n + 1, l, GET_PTR_TYPE(*p),
> + combine_notes);
> + if (!ret) {
> + *p = SET_PTR_TYPE(new_node, PTR_TYPE_INTERNAL);
> + ret = note_tree_insert(t, new_node, n + 1, entry, type,
> + combine_notes);
> + }
> + return ret;
Micronit: it would probably be clearer to write
if (ret)
return ret;
*p = SET_PTR_TYPE(...
return note_tree_insert(...
to avoid a little nesting.
The patch still looks good to me.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCHv4 06/21] notes.h/c: Propagate combine_notes_fn return value to add_note() and beyond
2010-10-21 5:21 ` Jonathan Nieder
@ 2010-10-21 13:16 ` Johan Herland
0 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 13:16 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, bebarino, avarab, gitster
On Thursday 21 October 2010, Jonathan Nieder wrote:
> Johan Herland wrote:
> > --- a/notes.c
> > +++ b/notes.c
> > @@ -303,13 +298,17 @@ static void note_tree_insert(struct
> > notes_tree *t, struct int_node *tree, GET_PTR_TYPE(*p) ==
> > PTR_TYPE_SUBTREE);
> > if (is_null_sha1(entry->val_sha1)) { /* skip insertion of empty
> > note */ free(entry);
> > - return;
> > + return 0;
> > }
> > new_node = (struct int_node *) xcalloc(sizeof(struct int_node),
> > 1); - note_tree_insert(t, new_node, n + 1, l, GET_PTR_TYPE(*p),
> > - combine_notes);
> > - *p = SET_PTR_TYPE(new_node, PTR_TYPE_INTERNAL);
> > - note_tree_insert(t, new_node, n + 1, entry, type, combine_notes);
> > + ret = note_tree_insert(t, new_node, n + 1, l, GET_PTR_TYPE(*p),
> > + combine_notes);
> > + if (!ret) {
> > + *p = SET_PTR_TYPE(new_node, PTR_TYPE_INTERNAL);
> > + ret = note_tree_insert(t, new_node, n + 1, entry, type,
> > + combine_notes);
> > + }
> > + return ret;
>
> Micronit: it would probably be clearer to write
>
> if (ret)
> return ret;
> *p = SET_PTR_TYPE(...
> return note_tree_insert(...
>
> to avoid a little nesting.
Agreed. Will be fixed in the next iteration.
Thanks,
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv4 07/21] (trivial) t3303: Indent with tabs instead of spaces for consistency
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (5 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 06/21] notes.h/c: Propagate combine_notes_fn return value to add_note() and beyond Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 08/21] notes.c: Use two newlines (instead of one) when concatenating notes Johan Herland
` (14 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
The rest of the file uses tabs for indenting. Fix the one function
that doesn't.
Signed-off-by: Johan Herland <johan@herland.net>
---
t/t3303-notes-subtrees.sh | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/t/t3303-notes-subtrees.sh b/t/t3303-notes-subtrees.sh
index 75ec187..d571708 100755
--- a/t/t3303-notes-subtrees.sh
+++ b/t/t3303-notes-subtrees.sh
@@ -168,15 +168,15 @@ INPUT_END
}
verify_concatenated_notes () {
- git log | grep "^ " > output &&
- i=$number_of_commits &&
- while [ $i -gt 0 ]; do
- echo " commit #$i" &&
- echo " first note for commit #$i" &&
- echo " second note for commit #$i" &&
- i=$(($i-1));
- done > expect &&
- test_cmp expect output
+ git log | grep "^ " > output &&
+ i=$number_of_commits &&
+ while [ $i -gt 0 ]; do
+ echo " commit #$i" &&
+ echo " first note for commit #$i" &&
+ echo " second note for commit #$i" &&
+ i=$(($i-1));
+ done > expect &&
+ test_cmp expect output
}
test_expect_success 'test notes in no fanout concatenated with 2/38-fanout' 'test_concatenated_notes "s|^..|&/|" ""'
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 08/21] notes.c: Use two newlines (instead of one) when concatenating notes
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (6 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 07/21] (trivial) t3303: Indent with tabs instead of spaces for consistency Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 09/21] builtin/notes.c: Split notes ref DWIMmery into a separate function Johan Herland
` (13 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
When using combine_notes_concatenate() to concatenate notes, it currently
ensures exactly one newline character between the given notes. However,
when using builtin/notes.c:create_note() to concatenate notes (e.g. by
'git notes append'), it adds a newline character to the trailing newline
of the preceding notes object, thus resulting in _two_ newlines (aka. a
blank line) separating contents of the two notes.
This patch brings combine_notes_concatenate() into consistency with
builtin/notes.c:create_note(), by ensuring exactly _two_ newline characters
between concatenated notes.
The patch also changes a few notes-related selftests accordingly.
Signed-off-by: Johan Herland <johan@herland.net>
---
notes.c | 7 ++++---
t/t3301-notes.sh | 4 ++++
t/t3303-notes-subtrees.sh | 1 +
t/t3404-rebase-interactive.sh | 1 +
t/t9301-fast-import-notes.sh | 5 +++++
5 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/notes.c b/notes.c
index c4203ce..a7a901a 100644
--- a/notes.c
+++ b/notes.c
@@ -814,16 +814,17 @@ int combine_notes_concatenate(unsigned char *cur_sha1,
return 0;
}
- /* we will separate the notes by a newline anyway */
+ /* we will separate the notes by two newlines anyway */
if (cur_msg[cur_len - 1] == '\n')
cur_len--;
/* concatenate cur_msg and new_msg into buf */
- buf_len = cur_len + 1 + new_len;
+ buf_len = cur_len + 2 + new_len;
buf = (char *) xmalloc(buf_len);
memcpy(buf, cur_msg, cur_len);
buf[cur_len] = '\n';
- memcpy(buf + cur_len + 1, new_msg, new_len);
+ buf[cur_len + 1] = '\n';
+ memcpy(buf + cur_len + 2, new_msg, new_len);
free(cur_msg);
free(new_msg);
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 1d82f79..4bf4e52 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -955,6 +955,7 @@ Date: Thu Apr 7 15:27:13 2005 -0700
Notes (other):
a fresh note
+$whitespace
another fresh note
EOF
@@ -976,8 +977,11 @@ Date: Thu Apr 7 15:27:13 2005 -0700
Notes (other):
a fresh note
+$whitespace
another fresh note
+$whitespace
append 1
+$whitespace
append 2
EOF
diff --git a/t/t3303-notes-subtrees.sh b/t/t3303-notes-subtrees.sh
index d571708..704aee8 100755
--- a/t/t3303-notes-subtrees.sh
+++ b/t/t3303-notes-subtrees.sh
@@ -173,6 +173,7 @@ verify_concatenated_notes () {
while [ $i -gt 0 ]; do
echo " commit #$i" &&
echo " first note for commit #$i" &&
+ echo " " &&
echo " second note for commit #$i" &&
i=$(($i-1));
done > expect &&
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9f03ce6..9ed70dc 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -593,6 +593,7 @@ test_expect_success 'rebase -i can copy notes' '
cat >expect <<EOF
an earlier note
+
a note
EOF
diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index a5c99d8..7cf8cd8 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -255,13 +255,18 @@ EOF
INPUT_END
+whitespace=" "
+
cat >expect <<EXPECT_END
fourth commit
pre-prefix of note for fourth commit
+$whitespace
prefix of note for fourth commit
+$whitespace
third note for fourth commit
third commit
prefix of note for third commit
+$whitespace
third note for third commit
second commit
third note for second commit
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 09/21] builtin/notes.c: Split notes ref DWIMmery into a separate function
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (7 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 08/21] notes.c: Use two newlines (instead of one) when concatenating notes Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 10/21] git notes merge: Initial implementation handling trivial merges only Johan Herland
` (12 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
expand_notes_ref() is a new function that performs the DWIM transformation
of "foo" -> "refs/notes/foo" where notes refs are expected.
This is done in preparation for future patches which will also need this
DWIM functionality.
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin/notes.c | 17 +++++++++++------
1 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 35f6eb6..9c91c59 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -83,6 +83,16 @@ struct msg_arg {
struct strbuf buf;
};
+static void expand_notes_ref(struct strbuf *sb)
+{
+ if (!prefixcmp(sb->buf, "refs/notes/"))
+ return; /* we're happy */
+ else if (!prefixcmp(sb->buf, "notes/"))
+ strbuf_insert(sb, 0, "refs/", 5);
+ else
+ strbuf_insert(sb, 0, "refs/notes/", 11);
+}
+
static int list_each_note(const unsigned char *object_sha1,
const unsigned char *note_sha1, char *note_path,
void *cb_data)
@@ -839,13 +849,8 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
if (override_notes_ref) {
struct strbuf sb = STRBUF_INIT;
- if (!prefixcmp(override_notes_ref, "refs/notes/"))
- /* we're happy */;
- else if (!prefixcmp(override_notes_ref, "notes/"))
- strbuf_addstr(&sb, "refs/");
- else
- strbuf_addstr(&sb, "refs/notes/");
strbuf_addstr(&sb, override_notes_ref);
+ expand_notes_ref(&sb);
setenv("GIT_NOTES_REF", sb.buf, 1);
strbuf_release(&sb);
}
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 10/21] git notes merge: Initial implementation handling trivial merges only
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (8 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 09/21] builtin/notes.c: Split notes ref DWIMmery into a separate function Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 11/21] builtin/notes.c: Refactor creation of notes commits Johan Herland
` (11 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
This initial implementation of 'git notes merge' only handles the trivial
merge cases (i.e. where the merge is either a no-op, or a fast-forward).
The patch includes testcases for these trivial merge cases.
Future patches will extend the functionality of 'git notes merge'.
This patch has been improved by the following contributions:
- Stephen Boyd: Simplify argc logic
- Stephen Boyd: Use test_commit
- Ævar Arnfjörð Bjarmason: Don't use C99 comments.
- Jonathan Nieder: Add constants for common verbosity values
- Jonathan Nieder: Use trace_printf(...) instead of OUTPUT(o, 5, ...)
- Jonathan Nieder: Remove extraneous show() function
- Jonathan Nieder: Clarify handling of empty/missing notes ref in notes_merge()
Thanks-to: Stephen Boyd <bebarino@gmail.com>
Thanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
Makefile | 2 +
builtin/notes.c | 54 ++++++++++++++
notes-merge.c | 120 ++++++++++++++++++++++++++++++++
notes-merge.h | 36 ++++++++++
t/t3308-notes-merge.sh | 180 ++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 392 insertions(+), 0 deletions(-)
create mode 100644 notes-merge.c
create mode 100644 notes-merge.h
create mode 100755 t/t3308-notes-merge.sh
diff --git a/Makefile b/Makefile
index f33648d..14c0ff1 100644
--- a/Makefile
+++ b/Makefile
@@ -503,6 +503,7 @@ LIB_H += mailmap.h
LIB_H += merge-recursive.h
LIB_H += notes.h
LIB_H += notes-cache.h
+LIB_H += notes-merge.h
LIB_H += object.h
LIB_H += pack.h
LIB_H += pack-refs.h
@@ -593,6 +594,7 @@ LIB_OBJS += merge-recursive.o
LIB_OBJS += name-hash.o
LIB_OBJS += notes.o
LIB_OBJS += notes-cache.o
+LIB_OBJS += notes-merge.o
LIB_OBJS += object.o
LIB_OBJS += pack-check.o
LIB_OBJS += pack-refs.o
diff --git a/builtin/notes.c b/builtin/notes.c
index 9c91c59..fbabdc7 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -17,6 +17,7 @@
#include "run-command.h"
#include "parse-options.h"
#include "string-list.h"
+#include "notes-merge.h"
static const char * const git_notes_usage[] = {
"git notes [--ref <notes_ref>] [list [<object>]]",
@@ -25,6 +26,7 @@ static const char * const git_notes_usage[] = {
"git notes [--ref <notes_ref>] append [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]",
"git notes [--ref <notes_ref>] edit [<object>]",
"git notes [--ref <notes_ref>] show [<object>]",
+ "git notes [--ref <notes_ref>] merge [-v | -q] <notes_ref>",
"git notes [--ref <notes_ref>] remove [<object>]",
"git notes [--ref <notes_ref>] prune [-n | -v]",
NULL
@@ -61,6 +63,11 @@ static const char * const git_notes_show_usage[] = {
NULL
};
+static const char * const git_notes_merge_usage[] = {
+ "git notes merge [<options>] <notes_ref>",
+ NULL
+};
+
static const char * const git_notes_remove_usage[] = {
"git notes remove [<object>]",
NULL
@@ -772,6 +779,51 @@ static int show(int argc, const char **argv, const char *prefix)
return retval;
}
+static int merge(int argc, const char **argv, const char *prefix)
+{
+ struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
+ unsigned char result_sha1[20];
+ struct notes_merge_options o;
+ int verbosity = 0, result;
+ struct option options[] = {
+ OPT__VERBOSITY(&verbosity),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+ git_notes_merge_usage, 0);
+
+ if (argc != 1) {
+ error("Must specify a notes ref to merge");
+ usage_with_options(git_notes_merge_usage, options);
+ }
+
+ init_notes_merge_options(&o);
+ o.verbosity = verbosity + NOTES_MERGE_VERBOSITY_DEFAULT;
+
+ o.local_ref = default_notes_ref();
+ strbuf_addstr(&remote_ref, argv[0]);
+ expand_notes_ref(&remote_ref);
+ o.remote_ref = remote_ref.buf;
+
+ result = notes_merge(&o, result_sha1);
+
+ strbuf_addf(&msg, "notes: Merged notes from %s into %s",
+ remote_ref.buf, default_notes_ref());
+ if (result == 0) { /* Merge resulted (trivially) in result_sha1 */
+ /* Update default notes ref with new commit */
+ update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,
+ 0, DIE_ON_ERR);
+ } else {
+ /* TODO: */
+ die("'git notes merge' cannot yet handle non-trivial merges!");
+ }
+
+ strbuf_release(&remote_ref);
+ strbuf_release(&msg);
+ return 0;
+}
+
static int remove_cmd(int argc, const char **argv, const char *prefix)
{
struct option options[] = {
@@ -865,6 +917,8 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
result = append_edit(argc, argv, prefix);
else if (!strcmp(argv[0], "show"))
result = show(argc, argv, prefix);
+ else if (!strcmp(argv[0], "merge"))
+ result = merge(argc, argv, prefix);
else if (!strcmp(argv[0], "remove"))
result = remove_cmd(argc, argv, prefix);
else if (!strcmp(argv[0], "prune"))
diff --git a/notes-merge.c b/notes-merge.c
new file mode 100644
index 0000000..cd917f9
--- /dev/null
+++ b/notes-merge.c
@@ -0,0 +1,120 @@
+#include "cache.h"
+#include "commit.h"
+#include "refs.h"
+#include "notes-merge.h"
+
+void init_notes_merge_options(struct notes_merge_options *o)
+{
+ memset(o, 0, sizeof(struct notes_merge_options));
+ o->verbosity = NOTES_MERGE_VERBOSITY_DEFAULT;
+}
+
+#define OUTPUT(o, v, ...) \
+ do { \
+ if ((o)->verbosity >= (v)) { \
+ printf(__VA_ARGS__); \
+ puts(""); \
+ } \
+ } while (0)
+
+int notes_merge(struct notes_merge_options *o,
+ unsigned char *result_sha1)
+{
+ unsigned char local_sha1[20], remote_sha1[20];
+ struct commit *local, *remote;
+ struct commit_list *bases = NULL;
+ const unsigned char *base_sha1;
+ int result = 0;
+
+ assert(o->local_ref && o->remote_ref);
+ hashclr(result_sha1);
+
+ trace_printf("notes_merge(o->local_ref = %s, o->remote_ref = %s)\n",
+ o->local_ref, o->remote_ref);
+
+ /* Dereference o->local_ref into local_sha1 */
+ if (!resolve_ref(o->local_ref, local_sha1, 0, NULL))
+ die("Failed to resolve local notes ref '%s'", o->local_ref);
+ else if (!check_ref_format(o->local_ref) && is_null_sha1(local_sha1))
+ local = NULL; /* local_sha1 == null_sha1 indicates unborn ref */
+ else if (!(local = lookup_commit_reference(local_sha1)))
+ die("Could not parse local commit %s (%s)",
+ sha1_to_hex(local_sha1), o->local_ref);
+ trace_printf("\tlocal commit: %.7s\n", sha1_to_hex(local_sha1));
+
+ /* Dereference o->remote_ref into remote_sha1 */
+ if (get_sha1(o->remote_ref, remote_sha1)) {
+ /*
+ * Failed to get remote_sha1. If o->remote_ref looks like an
+ * unborn ref, perform the merge using an empty notes tree.
+ */
+ if (!check_ref_format(o->remote_ref)) {
+ hashclr(remote_sha1);
+ remote = NULL;
+ }
+ else
+ die("Failed to resolve remote notes ref '%s'",
+ o->remote_ref);
+ }
+ else if (!(remote = lookup_commit_reference(remote_sha1)))
+ die("Could not parse remote commit %s (%s)",
+ sha1_to_hex(remote_sha1), o->remote_ref);
+ trace_printf("\tremote commit: %.7s\n", sha1_to_hex(remote_sha1));
+
+ if (!local && !remote)
+ die("Cannot merge empty notes ref (%s) into empty notes ref "
+ "(%s)", o->remote_ref, o->local_ref);
+ if (!local) {
+ /* result == remote commit */
+ hashcpy(result_sha1, remote_sha1);
+ goto found_result;
+ }
+ if (!remote) {
+ /* result == local commit */
+ hashcpy(result_sha1, local_sha1);
+ goto found_result;
+ }
+ assert(local && remote);
+
+ /* Find merge bases */
+ bases = get_merge_bases(local, remote, 1);
+ if (!bases) {
+ base_sha1 = null_sha1;
+ OUTPUT(o, 4, "No merge base found; doing history-less merge");
+ } else if (!bases->next) {
+ base_sha1 = bases->item->object.sha1;
+ OUTPUT(o, 4, "One merge base found (%.7s)",
+ sha1_to_hex(base_sha1));
+ } else {
+ /* TODO: How to handle multiple merge-bases? */
+ base_sha1 = bases->item->object.sha1;
+ OUTPUT(o, 3, "Multiple merge bases found. Using the first "
+ "(%.7s)", sha1_to_hex(base_sha1));
+ }
+
+ OUTPUT(o, 4, "Merging remote commit %.7s into local commit %.7s with "
+ "merge-base %.7s", sha1_to_hex(remote->object.sha1),
+ sha1_to_hex(local->object.sha1), sha1_to_hex(base_sha1));
+
+ if (!hashcmp(remote->object.sha1, base_sha1)) {
+ /* Already merged; result == local commit */
+ OUTPUT(o, 2, "Already up-to-date!");
+ hashcpy(result_sha1, local->object.sha1);
+ goto found_result;
+ }
+ if (!hashcmp(local->object.sha1, base_sha1)) {
+ /* Fast-forward; result == remote commit */
+ OUTPUT(o, 2, "Fast-forward");
+ hashcpy(result_sha1, remote->object.sha1);
+ goto found_result;
+ }
+
+ /* TODO: */
+ result = error("notes_merge() cannot yet handle real merges.");
+
+found_result:
+ free_commit_list(bases);
+ trace_printf("notes_merge(): result = %i, result_sha1 = %.7s\n",
+ result, sha1_to_hex(result_sha1));
+ return result;
+}
diff --git a/notes-merge.h b/notes-merge.h
new file mode 100644
index 0000000..fd572ac
--- /dev/null
+++ b/notes-merge.h
@@ -0,0 +1,36 @@
+#ifndef NOTES_MERGE_H
+#define NOTES_MERGE_H
+
+enum notes_merge_verbosity {
+ NOTES_MERGE_VERBOSITY_DEFAULT = 2,
+ NOTES_MERGE_VERBOSITY_MAX = 5
+};
+
+struct notes_merge_options {
+ const char *local_ref;
+ const char *remote_ref;
+ int verbosity;
+};
+
+void init_notes_merge_options(struct notes_merge_options *o);
+
+/*
+ * Merge notes from o->remote_ref into o->local_ref
+ *
+ * The commits given by the two refs are merged, producing one of the following
+ * outcomes:
+ *
+ * 1. The merge trivially results in an existing commit (e.g. fast-forward or
+ * already-up-to-date). The SHA1 of the result is written into 'result_sha1'
+ * and 0 is returned.
+ * 2. The merge fails. result_sha1 is set to null_sha1, and non-zero returned.
+ *
+ * Both o->local_ref and o->remote_ref must be given (non-NULL), but either ref
+ * (although not both) may refer to a non-existing notes ref, in which case
+ * that notes ref is interpreted as an empty notes tree, and the merge
+ * trivially results in what the other ref points to.
+ */
+int notes_merge(struct notes_merge_options *o,
+ unsigned char *result_sha1);
+
+#endif
diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
new file mode 100755
index 0000000..9acb684
--- /dev/null
+++ b/t/t3308-notes-merge.sh
@@ -0,0 +1,180 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Johan Herland
+#
+
+test_description='Test merging of notes trees'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ test_commit 1st &&
+ test_commit 2nd &&
+ test_commit 3rd &&
+ test_commit 4th &&
+ test_commit 5th &&
+ # Create notes on 4 first commits
+ git config core.notesRef refs/notes/x &&
+ 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
+'
+
+commit_sha1=$(git rev-parse 1st^{commit})
+commit_sha2=$(git rev-parse 2nd^{commit})
+commit_sha3=$(git rev-parse 3rd^{commit})
+commit_sha4=$(git rev-parse 4th^{commit})
+commit_sha5=$(git rev-parse 5th^{commit})
+
+verify_notes () {
+ notes_ref="$1"
+ git -c core.notesRef="refs/notes/$notes_ref" notes |
+ sort >"output_notes_$notes_ref" &&
+ test_cmp "expect_notes_$notes_ref" "output_notes_$notes_ref" &&
+ git -c core.notesRef="refs/notes/$notes_ref" log --format="%H %s%n%N" \
+ >"output_log_$notes_ref" &&
+ test_cmp "expect_log_$notes_ref" "output_log_$notes_ref"
+}
+
+cat <<EOF | sort >expect_notes_x
+5e93d24084d32e1cb61f7070505b9d2530cca987 $commit_sha4
+8366731eeee53787d2bdf8fc1eff7d94757e8da0 $commit_sha3
+eede89064cd42441590d6afec6c37b321ada3389 $commit_sha2
+daa55ffad6cb99bf64226532147ffcaf5ce8bdd1 $commit_sha1
+EOF
+
+cat >expect_log_x <<EOF
+$commit_sha5 5th
+
+$commit_sha4 4th
+Notes on 4th commit
+
+$commit_sha3 3rd
+Notes on 3rd commit
+
+$commit_sha2 2nd
+Notes on 2nd commit
+
+$commit_sha1 1st
+Notes on 1st commit
+
+EOF
+
+test_expect_success 'verify initial notes (x)' '
+ verify_notes x
+'
+
+cp expect_notes_x expect_notes_y
+cp expect_log_x expect_log_y
+
+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
+'
+
+test_expect_success 'fail to merge into various non-notes refs' '
+ test_must_fail git -c "core.notesRef=refs/notes" notes merge x &&
+ test_must_fail git -c "core.notesRef=refs/notes/" notes merge x &&
+ mkdir -p .git/refs/notes/dir &&
+ test_must_fail git -c "core.notesRef=refs/notes/dir" notes merge x &&
+ test_must_fail git -c "core.notesRef=refs/notes/dir/" notes merge x &&
+ test_must_fail git -c "core.notesRef=refs/heads/master" notes merge x &&
+ test_must_fail git -c "core.notesRef=refs/notes/y:" notes merge x &&
+ test_must_fail git -c "core.notesRef=refs/notes/y:foo" notes merge x &&
+ 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 notes into empty notes ref (x => y)' '
+ git config core.notesRef refs/notes/y &&
+ git notes merge x &&
+ verify_notes y &&
+ # x and y should point to the same notes commit
+ 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)"
+'
+
+test_expect_success 'change notes on other notes ref (y)' '
+ # Not touching notes to 1st commit
+ git notes remove 2nd &&
+ git notes append -m "More notes on 3rd commit" 3rd &&
+ git notes add -f -m "New notes on 4th commit" 4th &&
+ git notes add -m "Notes on 5th commit" 5th
+'
+
+test_expect_success 'merge previous notes commit (y^ => y) => No-op' '
+ pre_state="$(git rev-parse refs/notes/y)" &&
+ git notes merge y^ &&
+ # y should not move
+ test "$pre_state" = "$(git rev-parse refs/notes/y)"
+'
+
+cat <<EOF | sort >expect_notes_y
+0f2efbd00262f2fd41dfae33df8765618eeacd99 $commit_sha5
+dec2502dac3ea161543f71930044deff93fa945c $commit_sha4
+4069cdb399fd45463ec6eef8e051a16a03592d91 $commit_sha3
+daa55ffad6cb99bf64226532147ffcaf5ce8bdd1 $commit_sha1
+EOF
+
+cat >expect_log_y <<EOF
+$commit_sha5 5th
+Notes on 5th commit
+
+$commit_sha4 4th
+New notes on 4th commit
+
+$commit_sha3 3rd
+Notes on 3rd commit
+
+More notes on 3rd commit
+
+$commit_sha2 2nd
+
+$commit_sha1 1st
+Notes on 1st commit
+
+EOF
+
+test_expect_success 'verify changed notes on other notes ref (y)' '
+ verify_notes y
+'
+
+test_expect_success 'verify unchanged notes on original notes ref (x)' '
+ verify_notes x
+'
+
+test_expect_success 'merge original notes (x) into changed notes (y) => No-op' '
+ git notes merge -vvv x &&
+ verify_notes y &&
+ verify_notes x
+'
+
+cp expect_notes_y expect_notes_x
+cp expect_log_y expect_log_x
+
+test_expect_success 'merge changed (y) into original (x) => Fast-forward' '
+ git config core.notesRef refs/notes/x &&
+ git notes merge y &&
+ verify_notes x &&
+ verify_notes y &&
+ # x and y should point to same the notes commit
+ test "$(git rev-parse refs/notes/x)" = "$(git rev-parse refs/notes/y)"
+'
+
+test_done
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 11/21] builtin/notes.c: Refactor creation of notes commits.
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (9 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 10/21] git notes merge: Initial implementation handling trivial merges only Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 12/21] git notes merge: Handle real, non-conflicting notes merges Johan Herland
` (10 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
Create new function create_notes_commit() which is slightly more general than
commit_notes() (accepts multiple commit parents and does not auto-update the
notes ref). This function will be used by the notes-merge functionality in
future patches.
Also rewrite builtin/notes.c:commit_notes() to reuse this new function.
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin.h | 2 +-
builtin/notes.c | 28 +++++-----------------------
notes-merge.c | 27 +++++++++++++++++++++++++++
notes-merge.h | 14 ++++++++++++++
4 files changed, 47 insertions(+), 24 deletions(-)
diff --git a/builtin.h b/builtin.h
index ed6ee26..908d850 100644
--- a/builtin.h
+++ b/builtin.h
@@ -17,7 +17,7 @@ extern void prune_packed_objects(int);
extern int fmt_merge_msg(int merge_summary, struct strbuf *in,
struct strbuf *out);
extern int fmt_merge_msg_shortlog(struct strbuf *in, struct strbuf *out);
-extern int commit_notes(struct notes_tree *t, const char *msg);
+extern void commit_notes(struct notes_tree *t, const char *msg);
struct notes_rewrite_cfg {
struct notes_tree **trees;
diff --git a/builtin/notes.c b/builtin/notes.c
index fbabdc7..97d8baa 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -288,18 +288,17 @@ static int parse_reedit_arg(const struct option *opt, const char *arg, int unset
return parse_reuse_arg(opt, arg, unset);
}
-int commit_notes(struct notes_tree *t, const char *msg)
+void commit_notes(struct notes_tree *t, const char *msg)
{
- struct commit_list *parent;
- unsigned char tree_sha1[20], prev_commit[20], new_commit[20];
struct strbuf buf = STRBUF_INIT;
+ unsigned char commit_sha1[20];
if (!t)
t = &default_notes_tree;
if (!t->initialized || !t->ref || !*t->ref)
die("Cannot commit uninitialized/unreferenced notes tree");
if (!t->dirty)
- return 0; /* don't have to commit an unchanged tree */
+ return; /* don't have to commit an unchanged tree */
/* Prepare commit message and reflog message */
strbuf_addstr(&buf, "notes: "); /* commit message starts at index 7 */
@@ -307,27 +306,10 @@ int commit_notes(struct notes_tree *t, const char *msg)
if (buf.buf[buf.len - 1] != '\n')
strbuf_addch(&buf, '\n'); /* Make sure msg ends with newline */
- /* Convert notes tree to tree object */
- if (write_notes_tree(t, tree_sha1))
- die("Failed to write current notes tree to database");
-
- /* Create new commit for the tree object */
- if (!read_ref(t->ref, prev_commit)) { /* retrieve parent commit */
- parent = xmalloc(sizeof(*parent));
- parent->item = lookup_commit(prev_commit);
- parent->next = NULL;
- } else {
- hashclr(prev_commit);
- parent = NULL;
- }
- if (commit_tree(buf.buf + 7, tree_sha1, parent, new_commit, NULL))
- die("Failed to commit notes tree to database");
-
- /* Update notes ref with new commit */
- update_ref(buf.buf, t->ref, new_commit, prev_commit, 0, DIE_ON_ERR);
+ create_notes_commit(t, NULL, buf.buf + 7, commit_sha1);
+ update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, DIE_ON_ERR);
strbuf_release(&buf);
- return 0;
}
combine_notes_fn parse_combine_notes_fn(const char *v)
diff --git a/notes-merge.c b/notes-merge.c
index cd917f9..6ffa6e7 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -1,6 +1,7 @@
#include "cache.h"
#include "commit.h"
#include "refs.h"
+#include "notes.h"
#include "notes-merge.h"
void init_notes_merge_options(struct notes_merge_options *o)
@@ -17,6 +18,32 @@ void init_notes_merge_options(struct notes_merge_options *o)
} \
} while (0)
+void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
+ const char *msg, unsigned char *result_sha1)
+{
+ unsigned char tree_sha1[20];
+
+ assert(t->initialized);
+
+ if (write_notes_tree(t, tree_sha1))
+ die("Failed to write notes tree to database");
+
+ if (!parents) {
+ /* Deduce parent commit from t->ref */
+ unsigned char parent_sha1[20];
+ if (!read_ref(t->ref, parent_sha1)) {
+ struct commit *parent = lookup_commit(parent_sha1);
+ if (!parent || parse_commit(parent))
+ die("Failed to find/parse commit %s", t->ref);
+ commit_list_insert(parent, &parents);
+ }
+ /* else: t->ref points to nothing, assume root/orphan commit */
+ }
+
+ if (commit_tree(msg, tree_sha1, parents, result_sha1, NULL))
+ die("Failed to commit notes tree to database");
+}
+
int notes_merge(struct notes_merge_options *o,
unsigned char *result_sha1)
{
diff --git a/notes-merge.h b/notes-merge.h
index fd572ac..49e1b3a 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -15,6 +15,20 @@ struct notes_merge_options {
void init_notes_merge_options(struct notes_merge_options *o);
/*
+ * Create new notes commit from the given notes tree
+ *
+ * Properties of the created commit:
+ * - tree: the result of converting t to a tree object with write_notes_tree().
+ * - parents: the given parents OR (if NULL) the commit referenced by t->ref.
+ * - author/committer: the default determined by commmit_tree().
+ * - commit message: msg
+ *
+ * The resulting commit SHA1 is stored in result_sha1.
+ */
+void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
+ const char *msg, unsigned char *result_sha1);
+
+/*
* Merge notes from o->remote_ref into o->local_ref
*
* The commits given by the two refs are merged, producing one of the following
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 12/21] git notes merge: Handle real, non-conflicting notes merges
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (10 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 11/21] builtin/notes.c: Refactor creation of notes commits Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 13/21] git notes merge: Add automatic conflict resolvers (ours, theirs, union) Johan Herland
` (9 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
This continuation of the 'git notes merge' implementation teaches notes-merge
to properly do real merges between notes trees: Two diffs are performed, one
from $base to $remote, and another from $base to $local. The paths in each
diff are normalized to SHA1 object names. The two diffs are then consolidated
into a single list of change pairs to be evaluated. Each change pair consist
of:
- The annotated object's SHA1
- The $base SHA1 (i.e. the common ancestor notes for this object)
- The $local SHA1 (i.e. the current notes for this object)
- The $remote SHA1 (i.e. the to-be-merged notes for this object)
>From the pair ($base -> $local, $base -> $remote), we can determine the merge
result using regular 3-way rules. If conflicts are encountered in this
process, we fail loudly and exit (conflict handling to be added in a future
patch), If we can complete the merge without conflicts, the resulting
notes tree is committed, and the current notes ref updated.
The patch includes added testcases verifying that we can successfully do real
conflict-less merges.
This patch has been improved by the following contributions:
- Jonathan Nieder: Future-proof by always checking add_note() return value
- Stephen Boyd: Use test_commit
- Jonathan Nieder: Use trace_printf(...) instead of OUTPUT(o, 5, ...)
Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
Thanks-to: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin/notes.c | 15 ++-
notes-merge.c | 325 +++++++++++++++++++++++++++++++++++++++++++++++-
notes-merge.h | 15 ++-
t/t3308-notes-merge.sh | 188 ++++++++++++++++++++++++++++
4 files changed, 532 insertions(+), 11 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 97d8baa..c967b23 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -765,6 +765,7 @@ static int merge(int argc, const char **argv, const char *prefix)
{
struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
unsigned char result_sha1[20];
+ struct notes_tree *t;
struct notes_merge_options o;
int verbosity = 0, result;
struct option options[] = {
@@ -788,19 +789,23 @@ static int merge(int argc, const char **argv, const char *prefix)
expand_notes_ref(&remote_ref);
o.remote_ref = remote_ref.buf;
- result = notes_merge(&o, result_sha1);
+ t = init_notes_check("merge");
strbuf_addf(&msg, "notes: Merged notes from %s into %s",
remote_ref.buf, default_notes_ref());
- if (result == 0) { /* Merge resulted (trivially) in result_sha1 */
+ o.commit_msg = msg.buf + 7; // skip "notes: " prefix
+
+ result = notes_merge(&o, t, result_sha1);
+
+ if (result >= 0) /* Merge resulted (trivially) in result_sha1 */
/* Update default notes ref with new commit */
update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,
0, DIE_ON_ERR);
- } else {
+ else
/* TODO: */
- die("'git notes merge' cannot yet handle non-trivial merges!");
- }
+ die("'git notes merge' cannot yet handle conflicts!");
+ free_notes(t);
strbuf_release(&remote_ref);
strbuf_release(&msg);
return 0;
diff --git a/notes-merge.c b/notes-merge.c
index 6ffa6e7..20db0b6 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -1,9 +1,15 @@
#include "cache.h"
#include "commit.h"
#include "refs.h"
+#include "diff.h"
+#include "diffcore.h"
#include "notes.h"
#include "notes-merge.h"
+struct notes_merge_pair {
+ unsigned char obj[20], base[20], local[20], remote[20];
+};
+
void init_notes_merge_options(struct notes_merge_options *o)
{
memset(o, 0, sizeof(struct notes_merge_options));
@@ -18,6 +24,305 @@ void init_notes_merge_options(struct notes_merge_options *o)
} \
} while (0)
+static int path_to_sha1(const char *path, unsigned char *sha1)
+{
+ char hex_sha1[40];
+ int i = 0;
+ while (*path && i < 40) {
+ if (*path != '/')
+ hex_sha1[i++] = *path;
+ path++;
+ }
+ if (*path || i != 40)
+ return -1;
+ return get_sha1_hex(hex_sha1, sha1);
+}
+
+static int verify_notes_filepair(struct diff_filepair *p, unsigned char *sha1)
+{
+ switch (p->status) {
+ case DIFF_STATUS_MODIFIED:
+ assert(p->one->mode == p->two->mode);
+ assert(!is_null_sha1(p->one->sha1));
+ assert(!is_null_sha1(p->two->sha1));
+ break;
+ case DIFF_STATUS_ADDED:
+ assert(is_null_sha1(p->one->sha1));
+ break;
+ case DIFF_STATUS_DELETED:
+ assert(is_null_sha1(p->two->sha1));
+ break;
+ default:
+ return -1;
+ }
+ assert(!strcmp(p->one->path, p->two->path));
+ return path_to_sha1(p->one->path, sha1);
+}
+
+static struct notes_merge_pair *find_notes_merge_pair_pos(
+ struct notes_merge_pair *list, int len, unsigned char *obj,
+ int insert_new, int *occupied)
+{
+ /*
+ * Both diff_tree_remote() and diff_tree_local() tend to process
+ * merge_pairs in ascending order. Therefore, cache last returned
+ * index, and search sequentially from there until the appropriate
+ * position is found.
+ *
+ * Since inserts only happen from diff_tree_remote() (which mainly
+ * _appends_), we don't care that inserting into the middle of the
+ * list is expensive (using memmove()).
+ */
+ static int last_index = 0;
+ int i = last_index < len ? last_index : len - 1;
+ int prev_cmp = 0, cmp = -1;
+ while (i >= 0 && i < len) {
+ cmp = hashcmp(obj, list[i].obj);
+ if (!cmp) /* obj belongs @ i */
+ break;
+ else if (cmp < 0 && prev_cmp <= 0) /* obj belongs < i */
+ i--;
+ else if (cmp < 0) /* obj belongs between i-1 and i */
+ break;
+ else if (cmp > 0 && prev_cmp >= 0) /* obj belongs > i */
+ i++;
+ else /* if (cmp > 0) */ { /* obj belongs between i and i+1 */
+ i++;
+ break;
+ }
+ prev_cmp = cmp;
+ }
+ if (i < 0)
+ i = 0;
+ /* obj belongs at, or immediately preceding, index i (0 <= i <= len) */
+
+ if (!cmp)
+ *occupied = 1;
+ else {
+ *occupied = 0;
+ if (insert_new && i < len) {
+ memmove(list + i + 1, list + i,
+ (len - i) * sizeof(struct notes_merge_pair));
+ memset(list + i, 0, sizeof(struct notes_merge_pair));
+ }
+ }
+ last_index = i;
+ return list + i;
+}
+
+static unsigned char uninitialized[20] =
+ "\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff" \
+ "\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff";
+
+static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,
+ const unsigned char *base,
+ const unsigned char *remote,
+ int *num_changes)
+{
+ struct diff_options opt;
+ struct notes_merge_pair *changes;
+ int i, len = 0;
+
+ trace_printf("\tdiff_tree_remote(base = %.7s, remote = %.7s)\n",
+ sha1_to_hex(base), sha1_to_hex(remote));
+
+ diff_setup(&opt);
+ DIFF_OPT_SET(&opt, RECURSIVE);
+ opt.output_format = DIFF_FORMAT_NO_OUTPUT;
+ if (diff_setup_done(&opt) < 0)
+ die("diff_setup_done failed");
+ diff_tree_sha1(base, remote, "", &opt);
+ diffcore_std(&opt);
+
+ changes = xcalloc(diff_queued_diff.nr, sizeof(struct notes_merge_pair));
+
+ for (i = 0; i < diff_queued_diff.nr; i++) {
+ struct diff_filepair *p = diff_queued_diff.queue[i];
+ struct notes_merge_pair *mp;
+ int occupied;
+ unsigned char obj[20];
+
+ if (verify_notes_filepair(p, obj)) {
+ trace_printf("\t\tCannot merge entry '%s' (%c): "
+ "%.7s -> %.7s. Skipping!\n", p->one->path,
+ p->status, sha1_to_hex(p->one->sha1),
+ sha1_to_hex(p->two->sha1));
+ continue;
+ }
+ mp = find_notes_merge_pair_pos(changes, len, obj, 1, &occupied);
+ if (occupied) {
+ /* We've found an addition/deletion pair */
+ assert(!hashcmp(mp->obj, obj));
+ if (is_null_sha1(p->one->sha1)) { /* addition */
+ assert(is_null_sha1(mp->remote));
+ hashcpy(mp->remote, p->two->sha1);
+ } else if (is_null_sha1(p->two->sha1)) { /* deletion */
+ assert(is_null_sha1(mp->base));
+ hashcpy(mp->base, p->one->sha1);
+ } else
+ assert(!"Invalid existing change recorded");
+ } else {
+ hashcpy(mp->obj, obj);
+ hashcpy(mp->base, p->one->sha1);
+ hashcpy(mp->local, uninitialized);
+ hashcpy(mp->remote, p->two->sha1);
+ len++;
+ }
+ trace_printf("\t\tStored remote change for %s: %.7s -> %.7s\n",
+ sha1_to_hex(mp->obj), sha1_to_hex(mp->base),
+ sha1_to_hex(mp->remote));
+ }
+ diff_flush(&opt);
+ diff_tree_release_paths(&opt);
+
+ *num_changes = len;
+ return changes;
+}
+
+static void diff_tree_local(struct notes_merge_options *o,
+ struct notes_merge_pair *changes, int len,
+ const unsigned char *base,
+ const unsigned char *local)
+{
+ struct diff_options opt;
+ int i;
+
+ trace_printf("\tdiff_tree_local(len = %i, base = %.7s, local = %.7s)\n",
+ len, sha1_to_hex(base), sha1_to_hex(local));
+
+ diff_setup(&opt);
+ DIFF_OPT_SET(&opt, RECURSIVE);
+ opt.output_format = DIFF_FORMAT_NO_OUTPUT;
+ if (diff_setup_done(&opt) < 0)
+ die("diff_setup_done failed");
+ diff_tree_sha1(base, local, "", &opt);
+ diffcore_std(&opt);
+
+ for (i = 0; i < diff_queued_diff.nr; i++) {
+ struct diff_filepair *p = diff_queued_diff.queue[i];
+ struct notes_merge_pair *mp;
+ int match;
+ unsigned char obj[20];
+
+ if (verify_notes_filepair(p, obj)) {
+ trace_printf("\t\tCannot merge entry '%s' (%c): "
+ "%.7s -> %.7s. Skipping!\n", p->one->path,
+ p->status, sha1_to_hex(p->one->sha1),
+ sha1_to_hex(p->two->sha1));
+ continue;
+ }
+ mp = find_notes_merge_pair_pos(changes, len, obj, 0, &match);
+ if (!match) {
+ trace_printf("\t\tIgnoring local-only change for %s: "
+ "%.7s -> %.7s\n", sha1_to_hex(obj),
+ sha1_to_hex(p->one->sha1),
+ sha1_to_hex(p->two->sha1));
+ continue;
+ }
+
+ assert(!hashcmp(mp->obj, obj));
+ if (is_null_sha1(p->two->sha1)) { /* deletion */
+ /*
+ * Either this is a true deletion (1), or it is part
+ * of an A/D pair (2), or D/A pair (3):
+ *
+ * (1) mp->local is uninitialized; set it to null_sha1
+ * (2) mp->local is not uninitialized; don't touch it
+ * (3) mp->local is uninitialized; set it to null_sha1
+ * (will be overwritten by following addition)
+ */
+ if (!hashcmp(mp->local, uninitialized))
+ hashclr(mp->local);
+ } else if (is_null_sha1(p->one->sha1)) { /* addition */
+ /*
+ * Either this is a true addition (1), or it is part
+ * of an A/D pair (2), or D/A pair (3):
+ *
+ * (1) mp->local is uninitialized; set to p->two->sha1
+ * (2) mp->local is uninitialized; set to p->two->sha1
+ * (3) mp->local is null_sha1; set to p->two->sha1
+ */
+ assert(is_null_sha1(mp->local) ||
+ !hashcmp(mp->local, uninitialized));
+ hashcpy(mp->local, p->two->sha1);
+ } else { /* modification */
+ /*
+ * This is a true modification. p->one->sha1 shall
+ * match mp->base, and mp->local shall be uninitialized.
+ * Set mp->local to p->two->sha1.
+ */
+ assert(!hashcmp(p->one->sha1, mp->base));
+ assert(!hashcmp(mp->local, uninitialized));
+ hashcpy(mp->local, p->two->sha1);
+ }
+ trace_printf("\t\tStored local change for %s: %.7s -> %.7s\n",
+ sha1_to_hex(mp->obj), sha1_to_hex(mp->base),
+ sha1_to_hex(mp->local));
+ }
+ diff_flush(&opt);
+ diff_tree_release_paths(&opt);
+}
+
+static int merge_changes(struct notes_merge_options *o,
+ struct notes_merge_pair *changes, int *num_changes,
+ struct notes_tree *t)
+{
+ int i, conflicts = 0;
+
+ trace_printf("\tmerge_changes(num_changes = %i)\n", *num_changes);
+ for (i = 0; i < *num_changes; i++) {
+ struct notes_merge_pair *p = changes + i;
+ trace_printf("\t\t%.7s: %.7s -> %.7s/%.7s\n",
+ sha1_to_hex(p->obj), sha1_to_hex(p->base),
+ sha1_to_hex(p->local), sha1_to_hex(p->remote));
+
+ if (!hashcmp(p->base, p->remote)) {
+ /* no remote change; nothing to do */
+ trace_printf("\t\t\tskipping (no remote change)\n");
+ } else if (!hashcmp(p->local, p->remote)) {
+ /* same change in local and remote; nothing to do */
+ trace_printf("\t\t\tskipping (local == remote)\n");
+ } else if (!hashcmp(p->local, uninitialized) ||
+ !hashcmp(p->local, p->base)) {
+ /* no local change; adopt remote change */
+ trace_printf("\t\t\tno local change, adopted remote\n");
+ if (add_note(t, p->obj, p->remote,
+ combine_notes_overwrite))
+ die("confused: combine_notes_overwrite failed");
+ } else {
+ /* need file-level merge between local and remote */
+ trace_printf("\t\t\tneed content-level merge\n");
+ conflicts += 1; /* TODO */
+ }
+ }
+
+ return conflicts;
+}
+
+static int merge_from_diffs(struct notes_merge_options *o,
+ const unsigned char *base,
+ const unsigned char *local,
+ const unsigned char *remote, struct notes_tree *t)
+{
+ struct notes_merge_pair *changes;
+ int num_changes, conflicts;
+
+ trace_printf("\tmerge_from_diffs(base = %.7s, local = %.7s, "
+ "remote = %.7s)\n", sha1_to_hex(base), sha1_to_hex(local),
+ sha1_to_hex(remote));
+
+ changes = diff_tree_remote(o, base, remote, &num_changes);
+ diff_tree_local(o, changes, num_changes, base, local);
+
+ conflicts = merge_changes(o, changes, &num_changes, t);
+ free(changes);
+
+ OUTPUT(o, 4, "Merge result: %i unmerged notes and a %s notes tree",
+ conflicts, t->dirty ? "dirty" : "clean");
+
+ return conflicts ? -1 : 1;
+}
+
void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
const char *msg, unsigned char *result_sha1)
{
@@ -45,15 +350,17 @@ void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
}
int notes_merge(struct notes_merge_options *o,
+ struct notes_tree *local_tree,
unsigned char *result_sha1)
{
unsigned char local_sha1[20], remote_sha1[20];
struct commit *local, *remote;
struct commit_list *bases = NULL;
- const unsigned char *base_sha1;
+ const unsigned char *base_sha1, *base_tree_sha1;
int result = 0;
assert(o->local_ref && o->remote_ref);
+ assert(!strcmp(o->local_ref, local_tree->ref));
hashclr(result_sha1);
trace_printf("notes_merge(o->local_ref = %s, o->remote_ref = %s)\n",
@@ -107,14 +414,17 @@ int notes_merge(struct notes_merge_options *o,
bases = get_merge_bases(local, remote, 1);
if (!bases) {
base_sha1 = null_sha1;
+ base_tree_sha1 = (unsigned char *)EMPTY_TREE_SHA1_BIN;
OUTPUT(o, 4, "No merge base found; doing history-less merge");
} else if (!bases->next) {
base_sha1 = bases->item->object.sha1;
+ base_tree_sha1 = bases->item->tree->object.sha1;
OUTPUT(o, 4, "One merge base found (%.7s)",
sha1_to_hex(base_sha1));
} else {
/* TODO: How to handle multiple merge-bases? */
base_sha1 = bases->item->object.sha1;
+ base_tree_sha1 = bases->item->tree->object.sha1;
OUTPUT(o, 3, "Multiple merge bases found. Using the first "
"(%.7s)", sha1_to_hex(base_sha1));
}
@@ -136,8 +446,17 @@ int notes_merge(struct notes_merge_options *o,
goto found_result;
}
- /* TODO: */
- result = error("notes_merge() cannot yet handle real merges.");
+ result = merge_from_diffs(o, base_tree_sha1, local->tree->object.sha1,
+ remote->tree->object.sha1, local_tree);
+
+ if (result > 0) { /* successful non-trivial merge */
+ /* Commit result */
+ struct commit_list *parents = NULL;
+ commit_list_insert(remote, &parents); /* LIFO order */
+ commit_list_insert(local, &parents);
+ create_notes_commit(local_tree, parents, o->commit_msg,
+ result_sha1);
+ }
found_result:
free_commit_list(bases);
diff --git a/notes-merge.h b/notes-merge.h
index 49e1b3a..577cfb3 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -9,6 +9,7 @@ enum notes_merge_verbosity {
struct notes_merge_options {
const char *local_ref;
const char *remote_ref;
+ const char *commit_msg;
int verbosity;
};
@@ -31,13 +32,20 @@ void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
/*
* Merge notes from o->remote_ref into o->local_ref
*
+ * The given notes_tree 'local_tree' must be the notes_tree referenced by the
+ * o->local_ref. This is the notes_tree in which the object-level merge is
+ * performed.
+ *
* The commits given by the two refs are merged, producing one of the following
* outcomes:
*
* 1. The merge trivially results in an existing commit (e.g. fast-forward or
- * already-up-to-date). The SHA1 of the result is written into 'result_sha1'
- * and 0 is returned.
- * 2. The merge fails. result_sha1 is set to null_sha1, and non-zero returned.
+ * already-up-to-date). 'local_tree' is untouched, the SHA1 of the result
+ * is written into 'result_sha1' and 0 is returned.
+ * 2. The merge successfully completes, producing a merge commit. local_tree
+ * contains the updated notes tree, the SHA1 of the resulting commit is
+ * written into 'result_sha1', and 1 is returned.
+ * 3. The merge fails. result_sha1 is set to null_sha1, and -1 is returned.
*
* Both o->local_ref and o->remote_ref must be given (non-NULL), but either ref
* (although not both) may refer to a non-existing notes ref, in which case
@@ -45,6 +53,7 @@ void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
* trivially results in what the other ref points to.
*/
int notes_merge(struct notes_merge_options *o,
+ struct notes_tree *local_tree,
unsigned char *result_sha1);
#endif
diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
index 9acb684..24d82b4 100755
--- a/t/t3308-notes-merge.sh
+++ b/t/t3308-notes-merge.sh
@@ -177,4 +177,192 @@ test_expect_success 'merge changed (y) into original (x) => Fast-forward' '
test "$(git rev-parse refs/notes/x)" = "$(git rev-parse refs/notes/y)"
'
+test_expect_success 'merge empty notes ref (z => y)' '
+ # Prepare empty (but valid) notes ref (z)
+ git config core.notesRef refs/notes/z &&
+ git notes add -m "foo" &&
+ git notes remove &&
+ git notes >output_notes_z &&
+ test_cmp /dev/null output_notes_z &&
+ # Do the merge (z => y)
+ git config core.notesRef refs/notes/y &&
+ git notes merge z &&
+ verify_notes y &&
+ # y should no longer point to the same notes commit as x
+ test "$(git rev-parse refs/notes/x)" != "$(git rev-parse refs/notes/y)"
+'
+
+cat <<EOF | sort >expect_notes_y
+0f2efbd00262f2fd41dfae33df8765618eeacd99 $commit_sha5
+dec2502dac3ea161543f71930044deff93fa945c $commit_sha4
+4069cdb399fd45463ec6eef8e051a16a03592d91 $commit_sha3
+d000d30e6ddcfce3a8122c403226a2ce2fd04d9d $commit_sha2
+43add6bd0c8c0bc871ac7991e0f5573cfba27804 $commit_sha1
+EOF
+
+cat >expect_log_y <<EOF
+$commit_sha5 5th
+Notes on 5th commit
+
+$commit_sha4 4th
+New notes on 4th commit
+
+$commit_sha3 3rd
+Notes on 3rd commit
+
+More notes on 3rd commit
+
+$commit_sha2 2nd
+New notes on 2nd commit
+
+$commit_sha1 1st
+Notes on 1st commit
+
+More notes on 1st commit
+
+EOF
+
+test_expect_success 'change notes on other notes ref (y)' '
+ # Append to 1st commit notes
+ git notes append -m "More notes on 1st commit" 1st &&
+ # Add new notes to 2nd commit
+ git notes add -m "New notes on 2nd commit" 2nd &&
+ verify_notes y
+'
+
+cat <<EOF | sort >expect_notes_x
+0f2efbd00262f2fd41dfae33df8765618eeacd99 $commit_sha5
+1f257a3a90328557c452f0817d6cc50c89d315d4 $commit_sha4
+daa55ffad6cb99bf64226532147ffcaf5ce8bdd1 $commit_sha1
+EOF
+
+cat >expect_log_x <<EOF
+$commit_sha5 5th
+Notes on 5th commit
+
+$commit_sha4 4th
+New notes on 4th commit
+
+More notes on 4th commit
+
+$commit_sha3 3rd
+
+$commit_sha2 2nd
+
+$commit_sha1 1st
+Notes on 1st commit
+
+EOF
+
+test_expect_success 'change notes on notes ref (x)' '
+ git config core.notesRef refs/notes/x &&
+ git notes remove 3rd &&
+ git notes append -m "More notes on 4th commit" 4th &&
+ verify_notes x
+'
+
+cat <<EOF | sort >expect_notes_x
+0f2efbd00262f2fd41dfae33df8765618eeacd99 $commit_sha5
+1f257a3a90328557c452f0817d6cc50c89d315d4 $commit_sha4
+d000d30e6ddcfce3a8122c403226a2ce2fd04d9d $commit_sha2
+43add6bd0c8c0bc871ac7991e0f5573cfba27804 $commit_sha1
+EOF
+
+cat >expect_log_x <<EOF
+$commit_sha5 5th
+Notes on 5th commit
+
+$commit_sha4 4th
+New notes on 4th commit
+
+More notes on 4th commit
+
+$commit_sha3 3rd
+
+$commit_sha2 2nd
+New notes on 2nd commit
+
+$commit_sha1 1st
+Notes on 1st commit
+
+More notes on 1st commit
+
+EOF
+
+test_expect_success 'merge y into x => Non-conflicting 3-way merge' '
+ git notes merge y &&
+ verify_notes x &&
+ verify_notes y
+'
+
+cat <<EOF | sort >expect_notes_w
+05a4927951bcef347f51486575b878b2b60137f2 $commit_sha3
+d000d30e6ddcfce3a8122c403226a2ce2fd04d9d $commit_sha2
+EOF
+
+cat >expect_log_w <<EOF
+$commit_sha5 5th
+
+$commit_sha4 4th
+
+$commit_sha3 3rd
+New notes on 3rd commit
+
+$commit_sha2 2nd
+New notes on 2nd commit
+
+$commit_sha1 1st
+
+EOF
+
+test_expect_success 'create notes on new, separate notes ref (w)' '
+ git config core.notesRef refs/notes/w &&
+ # Add same note as refs/notes/y on 2nd commit
+ git notes add -m "New notes on 2nd commit" 2nd &&
+ # Add new note on 3rd commit (non-conflicting)
+ git notes add -m "New notes on 3rd commit" 3rd &&
+ # Verify state of notes on new, separate notes ref (w)
+ verify_notes w
+'
+
+cat <<EOF | sort >expect_notes_x
+0f2efbd00262f2fd41dfae33df8765618eeacd99 $commit_sha5
+1f257a3a90328557c452f0817d6cc50c89d315d4 $commit_sha4
+05a4927951bcef347f51486575b878b2b60137f2 $commit_sha3
+d000d30e6ddcfce3a8122c403226a2ce2fd04d9d $commit_sha2
+43add6bd0c8c0bc871ac7991e0f5573cfba27804 $commit_sha1
+EOF
+
+cat >expect_log_x <<EOF
+$commit_sha5 5th
+Notes on 5th commit
+
+$commit_sha4 4th
+New notes on 4th commit
+
+More notes on 4th commit
+
+$commit_sha3 3rd
+New notes on 3rd commit
+
+$commit_sha2 2nd
+New notes on 2nd commit
+
+$commit_sha1 1st
+Notes on 1st commit
+
+More notes on 1st commit
+
+EOF
+
+test_expect_success 'merge w into x => Non-conflicting history-less merge' '
+ git config core.notesRef refs/notes/x &&
+ git notes merge w &&
+ # Verify new state of notes on other notes ref (x)
+ verify_notes x &&
+ # Also verify that nothing changed on other notes refs (y and w)
+ verify_notes y &&
+ verify_notes w
+'
+
test_done
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 13/21] git notes merge: Add automatic conflict resolvers (ours, theirs, union)
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (11 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 12/21] git notes merge: Handle real, non-conflicting notes merges Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 14/21] Documentation: Preliminary docs on 'git notes merge' Johan Herland
` (8 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
The new -s/--strategy command-line option to 'git notes merge' allow the user
to choose how notes merge conflicts should be resolved. There are four valid
strategies to choose from:
1. "manual" (the default): This will let the user manually resolve conflicts.
This option currently fails with an error message. It will be implemented
properly in future patches.
2. "ours": This automatically chooses the local version of a conflict, and
discards the remote version.
3. "theirs": This automatically chooses the remote version of a conflict, and
discards the local version.
4. "union": This automatically resolves the conflict by appending the remote
version to the local version.
The strategies are implemented using the combine_notes_* functions from the
notes.h API.
The patch also includes testcases verifying the correct implementation of
these strategies.
This patch has been improved by the following contributions:
- Jonathan Nieder: Future-proof by always checking add_note() return value
- Stephen Boyd: Use test_commit
- Stephen Boyd: Use correct option name
Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
Thanks-to: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin/notes.c | 21 ++-
notes-merge.c | 31 ++-
notes-merge.h | 6 +
t/t3309-notes-merge-auto-resolve.sh | 502 +++++++++++++++++++++++++++++++++++
4 files changed, 558 insertions(+), 2 deletions(-)
create mode 100755 t/t3309-notes-merge-auto-resolve.sh
diff --git a/builtin/notes.c b/builtin/notes.c
index c967b23..a249f19 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -26,7 +26,7 @@ static const char * const git_notes_usage[] = {
"git notes [--ref <notes_ref>] append [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]",
"git notes [--ref <notes_ref>] edit [<object>]",
"git notes [--ref <notes_ref>] show [<object>]",
- "git notes [--ref <notes_ref>] merge [-v | -q] <notes_ref>",
+ "git notes [--ref <notes_ref>] merge [-v | -q] [-s <strategy> ] <notes_ref>",
"git notes [--ref <notes_ref>] remove [<object>]",
"git notes [--ref <notes_ref>] prune [-n | -v]",
NULL
@@ -768,8 +768,12 @@ static int merge(int argc, const char **argv, const char *prefix)
struct notes_tree *t;
struct notes_merge_options o;
int verbosity = 0, result;
+ const char *strategy = NULL;
struct option options[] = {
OPT__VERBOSITY(&verbosity),
+ OPT_STRING('s', "strategy", &strategy, "strategy",
+ "resolve notes conflicts using the given "
+ "strategy (manual/ours/theirs/union)"),
OPT_END()
};
@@ -789,6 +793,21 @@ static int merge(int argc, const char **argv, const char *prefix)
expand_notes_ref(&remote_ref);
o.remote_ref = remote_ref.buf;
+ if (strategy) {
+ if (!strcmp(strategy, "manual"))
+ o.strategy = NOTES_MERGE_RESOLVE_MANUAL;
+ else if (!strcmp(strategy, "ours"))
+ o.strategy = NOTES_MERGE_RESOLVE_OURS;
+ else if (!strcmp(strategy, "theirs"))
+ o.strategy = NOTES_MERGE_RESOLVE_THEIRS;
+ else if (!strcmp(strategy, "union"))
+ o.strategy = NOTES_MERGE_RESOLVE_UNION;
+ else {
+ error("Unknown -s/--strategy: %s", strategy);
+ usage_with_options(git_notes_merge_usage, options);
+ }
+ }
+
t = init_notes_check("merge");
strbuf_addf(&msg, "notes: Merged notes from %s into %s",
diff --git a/notes-merge.c b/notes-merge.c
index 20db0b6..f2fa12d 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -263,6 +263,35 @@ static void diff_tree_local(struct notes_merge_options *o,
diff_tree_release_paths(&opt);
}
+static int merge_one_change(struct notes_merge_options *o,
+ struct notes_merge_pair *p, struct notes_tree *t)
+{
+ /*
+ * Return 0 if change was resolved (and added to notes_tree),
+ * 1 if conflict
+ */
+ switch (o->strategy) {
+ case NOTES_MERGE_RESOLVE_MANUAL:
+ return 1;
+ case NOTES_MERGE_RESOLVE_OURS:
+ OUTPUT(o, 2, "Using local notes for %s", sha1_to_hex(p->obj));
+ /* nothing to do */
+ return 0;
+ case NOTES_MERGE_RESOLVE_THEIRS:
+ OUTPUT(o, 2, "Using remote notes for %s", sha1_to_hex(p->obj));
+ if (add_note(t, p->obj, p->remote, combine_notes_overwrite))
+ die("confused: combine_notes_overwrite failed");
+ return 0;
+ case NOTES_MERGE_RESOLVE_UNION:
+ OUTPUT(o, 2, "Concatenating local and remote notes for %s",
+ sha1_to_hex(p->obj));
+ if (add_note(t, p->obj, p->remote, combine_notes_concatenate))
+ die("confused: combine_notes_concatenate failed");
+ return 0;
+ }
+ die("Unknown strategy (%i).", o->strategy);
+}
+
static int merge_changes(struct notes_merge_options *o,
struct notes_merge_pair *changes, int *num_changes,
struct notes_tree *t)
@@ -292,7 +321,7 @@ static int merge_changes(struct notes_merge_options *o,
} else {
/* need file-level merge between local and remote */
trace_printf("\t\t\tneed content-level merge\n");
- conflicts += 1; /* TODO */
+ conflicts += merge_one_change(o, p, t);
}
}
diff --git a/notes-merge.h b/notes-merge.h
index 577cfb3..ed356ae 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -11,6 +11,12 @@ struct notes_merge_options {
const char *remote_ref;
const char *commit_msg;
int verbosity;
+ enum {
+ NOTES_MERGE_RESOLVE_MANUAL = 0,
+ NOTES_MERGE_RESOLVE_OURS,
+ NOTES_MERGE_RESOLVE_THEIRS,
+ NOTES_MERGE_RESOLVE_UNION
+ } strategy;
};
void init_notes_merge_options(struct notes_merge_options *o);
diff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh
new file mode 100755
index 0000000..1a1fc7e
--- /dev/null
+++ b/t/t3309-notes-merge-auto-resolve.sh
@@ -0,0 +1,502 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Johan Herland
+#
+
+test_description='Test notes merging with auto-resolving strategies'
+
+. ./test-lib.sh
+
+# Set up a notes merge scenario with all kinds of potential conflicts
+test_expect_success 'setup commits' '
+ test_commit 1st &&
+ test_commit 2nd &&
+ test_commit 3rd &&
+ test_commit 4th &&
+ test_commit 5th &&
+ test_commit 6th &&
+ test_commit 7th &&
+ test_commit 8th &&
+ test_commit 9th &&
+ test_commit 10th &&
+ test_commit 11th &&
+ test_commit 12th &&
+ test_commit 13th &&
+ test_commit 14th &&
+ test_commit 15th
+'
+
+commit_sha1=$(git rev-parse 1st^{commit})
+commit_sha2=$(git rev-parse 2nd^{commit})
+commit_sha3=$(git rev-parse 3rd^{commit})
+commit_sha4=$(git rev-parse 4th^{commit})
+commit_sha5=$(git rev-parse 5th^{commit})
+commit_sha6=$(git rev-parse 6th^{commit})
+commit_sha7=$(git rev-parse 7th^{commit})
+commit_sha8=$(git rev-parse 8th^{commit})
+commit_sha9=$(git rev-parse 9th^{commit})
+commit_sha10=$(git rev-parse 10th^{commit})
+commit_sha11=$(git rev-parse 11th^{commit})
+commit_sha12=$(git rev-parse 12th^{commit})
+commit_sha13=$(git rev-parse 13th^{commit})
+commit_sha14=$(git rev-parse 14th^{commit})
+commit_sha15=$(git rev-parse 15th^{commit})
+
+verify_notes () {
+ notes_ref="$1"
+ suffix="$2"
+ git -c core.notesRef="refs/notes/$notes_ref" notes |
+ sort >"output_notes_$suffix" &&
+ test_cmp "expect_notes_$suffix" "output_notes_$suffix" &&
+ git -c core.notesRef="refs/notes/$notes_ref" log --format="%H %s%n%N" \
+ >"output_log_$suffix" &&
+ test_cmp "expect_log_$suffix" "output_log_$suffix"
+}
+
+test_expect_success 'setup merge base (x)' '
+ git config core.notesRef refs/notes/x &&
+ git notes add -m "x notes on 6th commit" 6th &&
+ git notes add -m "x notes on 7th commit" 7th &&
+ git notes add -m "x notes on 8th commit" 8th &&
+ git notes add -m "x notes on 9th commit" 9th &&
+ git notes add -m "x notes on 10th commit" 10th &&
+ git notes add -m "x notes on 11th commit" 11th &&
+ git notes add -m "x notes on 12th commit" 12th &&
+ git notes add -m "x notes on 13th commit" 13th &&
+ git notes add -m "x notes on 14th commit" 14th &&
+ git notes add -m "x notes on 15th commit" 15th
+'
+
+cat <<EOF | sort >expect_notes_x
+457a85d6c814ea208550f15fcc48f804ac8dc023 $commit_sha15
+b0c95b954301d69da2bc3723f4cb1680d355937c $commit_sha14
+5d30216a129eeffa97d9694ffe8c74317a560315 $commit_sha13
+dd161bc149470fd890dd4ab52a4cbd79bbd18c36 $commit_sha12
+7abbc45126d680336fb24294f013a7cdfa3ed545 $commit_sha11
+b8d03e173f67f6505a76f6e00cf93440200dd9be $commit_sha10
+20c613c835011c48a5abe29170a2402ca6354910 $commit_sha9
+a3daf8a1e4e5dc3409a303ad8481d57bfea7f5d6 $commit_sha8
+897003322b53bc6ca098e9324ee508362347e734 $commit_sha7
+11d97fdebfa5ceee540a3da07bce6fa0222bc082 $commit_sha6
+EOF
+
+cat >expect_log_x <<EOF
+$commit_sha15 15th
+x notes on 15th commit
+
+$commit_sha14 14th
+x notes on 14th commit
+
+$commit_sha13 13th
+x notes on 13th commit
+
+$commit_sha12 12th
+x notes on 12th commit
+
+$commit_sha11 11th
+x notes on 11th commit
+
+$commit_sha10 10th
+x notes on 10th commit
+
+$commit_sha9 9th
+x notes on 9th commit
+
+$commit_sha8 8th
+x notes on 8th commit
+
+$commit_sha7 7th
+x notes on 7th commit
+
+$commit_sha6 6th
+x notes on 6th commit
+
+$commit_sha5 5th
+
+$commit_sha4 4th
+
+$commit_sha3 3rd
+
+$commit_sha2 2nd
+
+$commit_sha1 1st
+
+EOF
+
+test_expect_success 'verify state of merge base (x)' 'verify_notes x x'
+
+test_expect_success 'setup local branch (y)' '
+ git update-ref refs/notes/y refs/notes/x &&
+ git config core.notesRef refs/notes/y &&
+ git notes add -f -m "y notes on 3rd commit" 3rd &&
+ git notes add -f -m "y notes on 4th commit" 4th &&
+ git notes add -f -m "y notes on 5th commit" 5th &&
+ git notes remove 6th &&
+ git notes remove 7th &&
+ git notes remove 8th &&
+ git notes add -f -m "y notes on 12th commit" 12th &&
+ git notes add -f -m "y notes on 13th commit" 13th &&
+ git notes add -f -m "y notes on 14th commit" 14th &&
+ git notes add -f -m "y notes on 15th commit" 15th
+'
+
+cat <<EOF | sort >expect_notes_y
+68b8630d25516028bed862719855b3d6768d7833 $commit_sha15
+5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
+3a631fdb6f41b05b55d8f4baf20728ba8f6fccbc $commit_sha13
+a66055fa82f7a03fe0c02a6aba3287a85abf7c62 $commit_sha12
+7abbc45126d680336fb24294f013a7cdfa3ed545 $commit_sha11
+b8d03e173f67f6505a76f6e00cf93440200dd9be $commit_sha10
+20c613c835011c48a5abe29170a2402ca6354910 $commit_sha9
+154508c7a0bcad82b6fe4b472bc4c26b3bf0825b $commit_sha5
+e2bfd06a37dd2031684a59a6e2b033e212239c78 $commit_sha4
+5772f42408c0dd6f097a7ca2d24de0e78d1c46b1 $commit_sha3
+EOF
+
+cat >expect_log_y <<EOF
+$commit_sha15 15th
+y notes on 15th commit
+
+$commit_sha14 14th
+y notes on 14th commit
+
+$commit_sha13 13th
+y notes on 13th commit
+
+$commit_sha12 12th
+y notes on 12th commit
+
+$commit_sha11 11th
+x notes on 11th commit
+
+$commit_sha10 10th
+x notes on 10th commit
+
+$commit_sha9 9th
+x notes on 9th commit
+
+$commit_sha8 8th
+
+$commit_sha7 7th
+
+$commit_sha6 6th
+
+$commit_sha5 5th
+y notes on 5th commit
+
+$commit_sha4 4th
+y notes on 4th commit
+
+$commit_sha3 3rd
+y notes on 3rd commit
+
+$commit_sha2 2nd
+
+$commit_sha1 1st
+
+EOF
+
+test_expect_success 'verify state of local branch (y)' 'verify_notes y y'
+
+test_expect_success 'setup remote branch (z)' '
+ git update-ref refs/notes/z refs/notes/x &&
+ git config core.notesRef refs/notes/z &&
+ git notes add -f -m "z notes on 2nd commit" 2nd &&
+ git notes add -f -m "y notes on 4th commit" 4th &&
+ git notes add -f -m "z notes on 5th commit" 5th &&
+ git notes remove 6th &&
+ git notes add -f -m "z notes on 8th commit" 8th &&
+ git notes remove 9th &&
+ git notes add -f -m "z notes on 11th commit" 11th &&
+ git notes remove 12th &&
+ git notes add -f -m "y notes on 14th commit" 14th &&
+ git notes add -f -m "z notes on 15th commit" 15th
+'
+
+cat <<EOF | sort >expect_notes_z
+9b4b2c61f0615412da3c10f98ff85b57c04ec765 $commit_sha15
+5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
+5d30216a129eeffa97d9694ffe8c74317a560315 $commit_sha13
+7e3c53503a3db8dd996cb62e37c66e070b44b54d $commit_sha11
+b8d03e173f67f6505a76f6e00cf93440200dd9be $commit_sha10
+851e1638784a884c7dd26c5d41f3340f6387413a $commit_sha8
+897003322b53bc6ca098e9324ee508362347e734 $commit_sha7
+99fc34adfc400b95c67b013115e37e31aa9a6d23 $commit_sha5
+e2bfd06a37dd2031684a59a6e2b033e212239c78 $commit_sha4
+283b48219aee9a4105f6cab337e789065c82c2b9 $commit_sha2
+EOF
+
+cat >expect_log_z <<EOF
+$commit_sha15 15th
+z notes on 15th commit
+
+$commit_sha14 14th
+y notes on 14th commit
+
+$commit_sha13 13th
+x notes on 13th commit
+
+$commit_sha12 12th
+
+$commit_sha11 11th
+z notes on 11th commit
+
+$commit_sha10 10th
+x notes on 10th commit
+
+$commit_sha9 9th
+
+$commit_sha8 8th
+z notes on 8th commit
+
+$commit_sha7 7th
+x notes on 7th commit
+
+$commit_sha6 6th
+
+$commit_sha5 5th
+z notes on 5th commit
+
+$commit_sha4 4th
+y notes on 4th commit
+
+$commit_sha3 3rd
+
+$commit_sha2 2nd
+z notes on 2nd commit
+
+$commit_sha1 1st
+
+EOF
+
+test_expect_success 'verify state of remote branch (z)' 'verify_notes z z'
+
+# At this point, before merging z into y, we have the following status:
+#
+# commit | base/x | local/y | remote/z | diff from x to y/z | result
+# -------|---------|---------|----------|----------------------------|-------
+# 1st | [none] | [none] | [none] | unchanged / unchanged | [none]
+# 2nd | [none] | [none] | 283b482 | unchanged / added | 283b482
+# 3rd | [none] | 5772f42 | [none] | added / unchanged | 5772f42
+# 4th | [none] | e2bfd06 | e2bfd06 | added / added (same) | e2bfd06
+# 5th | [none] | 154508c | 99fc34a | added / added (diff) | ???
+# 6th | 11d97fd | [none] | [none] | removed / removed | [none]
+# 7th | 8970033 | [none] | 8970033 | removed / unchanged | [none]
+# 8th | a3daf8a | [none] | 851e163 | removed / changed | ???
+# 9th | 20c613c | 20c613c | [none] | unchanged / removed | [none]
+# 10th | b8d03e1 | b8d03e1 | b8d03e1 | unchanged / unchanged | b8d03e1
+# 11th | 7abbc45 | 7abbc45 | 7e3c535 | unchanged / changed | 7e3c535
+# 12th | dd161bc | a66055f | [none] | changed / removed | ???
+# 13th | 5d30216 | 3a631fd | 5d30216 | changed / unchanged | 3a631fd
+# 14th | b0c95b9 | 5de7ea7 | 5de7ea7 | changed / changed (same) | 5de7ea7
+# 15th | 457a85d | 68b8630 | 9b4b2c6 | changed / changed (diff) | ???
+
+test_expect_success 'merge z into y with invalid strategy => Fail/No changes' '
+ git config core.notesRef refs/notes/y &&
+ test_must_fail git notes merge --strategy=foo z &&
+ # Verify no changes (y)
+ verify_notes y y
+'
+
+cat <<EOF | sort >expect_notes_ours
+68b8630d25516028bed862719855b3d6768d7833 $commit_sha15
+5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
+3a631fdb6f41b05b55d8f4baf20728ba8f6fccbc $commit_sha13
+a66055fa82f7a03fe0c02a6aba3287a85abf7c62 $commit_sha12
+7e3c53503a3db8dd996cb62e37c66e070b44b54d $commit_sha11
+b8d03e173f67f6505a76f6e00cf93440200dd9be $commit_sha10
+154508c7a0bcad82b6fe4b472bc4c26b3bf0825b $commit_sha5
+e2bfd06a37dd2031684a59a6e2b033e212239c78 $commit_sha4
+5772f42408c0dd6f097a7ca2d24de0e78d1c46b1 $commit_sha3
+283b48219aee9a4105f6cab337e789065c82c2b9 $commit_sha2
+EOF
+
+cat >expect_log_ours <<EOF
+$commit_sha15 15th
+y notes on 15th commit
+
+$commit_sha14 14th
+y notes on 14th commit
+
+$commit_sha13 13th
+y notes on 13th commit
+
+$commit_sha12 12th
+y notes on 12th commit
+
+$commit_sha11 11th
+z notes on 11th commit
+
+$commit_sha10 10th
+x notes on 10th commit
+
+$commit_sha9 9th
+
+$commit_sha8 8th
+
+$commit_sha7 7th
+
+$commit_sha6 6th
+
+$commit_sha5 5th
+y notes on 5th commit
+
+$commit_sha4 4th
+y notes on 4th commit
+
+$commit_sha3 3rd
+y notes on 3rd commit
+
+$commit_sha2 2nd
+z notes on 2nd commit
+
+$commit_sha1 1st
+
+EOF
+
+test_expect_success 'merge z into y with "ours" strategy => Non-conflicting 3-way merge' '
+ git notes merge --strategy=ours z &&
+ verify_notes y ours
+'
+
+test_expect_success 'reset to pre-merge state (y)' '
+ git update-ref refs/notes/y refs/notes/y^1 &&
+ # Verify pre-merge state
+ verify_notes y y
+'
+
+cat <<EOF | sort >expect_notes_theirs
+9b4b2c61f0615412da3c10f98ff85b57c04ec765 $commit_sha15
+5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
+3a631fdb6f41b05b55d8f4baf20728ba8f6fccbc $commit_sha13
+7e3c53503a3db8dd996cb62e37c66e070b44b54d $commit_sha11
+b8d03e173f67f6505a76f6e00cf93440200dd9be $commit_sha10
+851e1638784a884c7dd26c5d41f3340f6387413a $commit_sha8
+99fc34adfc400b95c67b013115e37e31aa9a6d23 $commit_sha5
+e2bfd06a37dd2031684a59a6e2b033e212239c78 $commit_sha4
+5772f42408c0dd6f097a7ca2d24de0e78d1c46b1 $commit_sha3
+283b48219aee9a4105f6cab337e789065c82c2b9 $commit_sha2
+EOF
+
+cat >expect_log_theirs <<EOF
+$commit_sha15 15th
+z notes on 15th commit
+
+$commit_sha14 14th
+y notes on 14th commit
+
+$commit_sha13 13th
+y notes on 13th commit
+
+$commit_sha12 12th
+
+$commit_sha11 11th
+z notes on 11th commit
+
+$commit_sha10 10th
+x notes on 10th commit
+
+$commit_sha9 9th
+
+$commit_sha8 8th
+z notes on 8th commit
+
+$commit_sha7 7th
+
+$commit_sha6 6th
+
+$commit_sha5 5th
+z notes on 5th commit
+
+$commit_sha4 4th
+y notes on 4th commit
+
+$commit_sha3 3rd
+y notes on 3rd commit
+
+$commit_sha2 2nd
+z notes on 2nd commit
+
+$commit_sha1 1st
+
+EOF
+
+test_expect_success 'merge z into y with "theirs" strategy => Non-conflicting 3-way merge' '
+ git notes merge --strategy=theirs z &&
+ verify_notes y theirs
+'
+
+test_expect_success 'reset to pre-merge state (y)' '
+ git update-ref refs/notes/y refs/notes/y^1 &&
+ # Verify pre-merge state
+ verify_notes y y
+'
+
+cat <<EOF | sort >expect_notes_union
+7c4e546efd0fe939f876beb262ece02797880b54 $commit_sha15
+5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
+3a631fdb6f41b05b55d8f4baf20728ba8f6fccbc $commit_sha13
+a66055fa82f7a03fe0c02a6aba3287a85abf7c62 $commit_sha12
+7e3c53503a3db8dd996cb62e37c66e070b44b54d $commit_sha11
+b8d03e173f67f6505a76f6e00cf93440200dd9be $commit_sha10
+851e1638784a884c7dd26c5d41f3340f6387413a $commit_sha8
+6c841cc36ea496027290967ca96bd2bef54dbb47 $commit_sha5
+e2bfd06a37dd2031684a59a6e2b033e212239c78 $commit_sha4
+5772f42408c0dd6f097a7ca2d24de0e78d1c46b1 $commit_sha3
+283b48219aee9a4105f6cab337e789065c82c2b9 $commit_sha2
+EOF
+
+cat >expect_log_union <<EOF
+$commit_sha15 15th
+y notes on 15th commit
+
+z notes on 15th commit
+
+$commit_sha14 14th
+y notes on 14th commit
+
+$commit_sha13 13th
+y notes on 13th commit
+
+$commit_sha12 12th
+y notes on 12th commit
+
+$commit_sha11 11th
+z notes on 11th commit
+
+$commit_sha10 10th
+x notes on 10th commit
+
+$commit_sha9 9th
+
+$commit_sha8 8th
+z notes on 8th commit
+
+$commit_sha7 7th
+
+$commit_sha6 6th
+
+$commit_sha5 5th
+y notes on 5th commit
+
+z notes on 5th commit
+
+$commit_sha4 4th
+y notes on 4th commit
+
+$commit_sha3 3rd
+y notes on 3rd commit
+
+$commit_sha2 2nd
+z notes on 2nd commit
+
+$commit_sha1 1st
+
+EOF
+
+test_expect_success 'merge z into y with "union" strategy => Non-conflicting 3-way merge' '
+ git notes merge --strategy=union z &&
+ verify_notes y union
+'
+
+test_done
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 14/21] Documentation: Preliminary docs on 'git notes merge'
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (12 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 13/21] git notes merge: Add automatic conflict resolvers (ours, theirs, union) Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 15/21] git notes merge: Manual conflict resolution, part 1/2 Johan Herland
` (7 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
This patch has been improved by the following contributions:
- Stephen Boyd: Use "automatically resolves" instead of "auto-resolves"
- Stephen Boyd: Remove unbalanced '('
Thanks-to: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
Documentation/git-notes.txt | 44 ++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 43 insertions(+), 1 deletions(-)
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 2981d8c..07a5042 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -14,6 +14,7 @@ SYNOPSIS
'git notes' append [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
'git notes' edit [<object>]
'git notes' show [<object>]
+'git notes' merge [-v | -q] [-s <strategy> ] <notes_ref>
'git notes' remove [<object>]
'git notes' prune [-n | -v]
@@ -83,6 +84,16 @@ edit::
show::
Show the notes for a given object (defaults to HEAD).
+merge::
+ Merge the given notes ref into the current notes ref.
+ This will try to merge the changes made by the given
+ notes ref (called "remote") since the merge-base (if
+ any) into the current notes ref (called "local").
++
+If conflicts arise and a strategy for automatically resolving
+conflicting notes (see the -s/--strategy option) is not given,
+the merge fails (TODO).
+
remove::
Remove the notes for a given object (defaults to HEAD).
This is equivalent to specifying an empty note message to
@@ -133,9 +144,23 @@ OPTIONS
Do not remove anything; just report the object names whose notes
would be removed.
+-s <strategy>::
+--strategy=<strategy>::
+ When merging notes, resolve notes conflicts using the given
+ strategy. The following strategies are recognized: "manual"
+ (default), "ours", "theirs" and "union".
+ See the "NOTES MERGE STRATEGIES" section below for more
+ information on each notes merge strategy.
+
+-q::
+--quiet::
+ When merging notes, operate quietly.
+
-v::
--verbose::
- Report all object names whose notes are removed.
+ When merging notes, be more verbose.
+ When pruning notes, report all object names whose notes are
+ removed.
DISCUSSION
@@ -163,6 +188,23 @@ object, in which case the history of the notes can be read with
`git log -p -g <refname>`.
+NOTES MERGE STRATEGIES
+----------------------
+
+The default notes merge strategy is "manual", which is not yet
+implemented (TODO).
+
+"ours" automatically resolves conflicting notes in favor of the local
+version (i.e. the current notes ref).
+
+"theirs" automatically resolves notes conflicts in favor of the remote
+version (i.e. the given notes ref being merged into the current notes
+ref).
+
+"union" automatically resolves notes conflicts by concatenating the
+local and remote versions.
+
+
EXAMPLES
--------
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 15/21] git notes merge: Manual conflict resolution, part 1/2
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (13 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 14/21] Documentation: Preliminary docs on 'git notes merge' Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 16/21] git notes merge: Manual conflict resolution, part 2/2 Johan Herland
` (6 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
Conflicts (that are to be resolved manually) are written into a special-
purpose working tree, located at .git/NOTES_MERGE_WORKTREE. Within this
directory, conflicting notes entries are stored (with conflict markers
produced by ll_merge()) using the SHA1 of the annotated object. The
.git/NOTES_MERGE_WORKTREE directory will only contain the _conflicting_
note entries. The non-conflicting note entries (aka. the partial merge
result) are stored in 'local_tree', and the SHA1 of the resulting commit
is written to 'result_sha1'. The return value from notes_merge() is -1.
The user is told to edit the files within the .git/NOTES_MERGE_WORKTREE
directory in order to resolve the conflicts.
The patch also contains documentation and testcases for the correct setup
of .git/NOTES_MERGE_WORKTREE.
The next part will recombine the partial notes merge result with the
resolved conflicts in .git/NOTES_MERGE_WORKTREE to produce the complete
merge result.
This patch has been improved by the following contributions:
- Jonathan Nieder: Use trace_printf(...) instead of OUTPUT(o, 5, ...)
Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
Documentation/git-notes.txt | 10 +-
builtin/notes.c | 8 +-
notes-merge.c | 166 ++++++++++++++++++-
notes-merge.h | 11 +-
t/t3310-notes-merge-manual-resolve.sh | 292 +++++++++++++++++++++++++++++++++
5 files changed, 474 insertions(+), 13 deletions(-)
create mode 100755 t/t3310-notes-merge-manual-resolve.sh
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 07a5042..f003b7c 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -92,7 +92,9 @@ merge::
+
If conflicts arise and a strategy for automatically resolving
conflicting notes (see the -s/--strategy option) is not given,
-the merge fails (TODO).
+the "manual" resolver is used. This resolver checks out the
+conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
+and instructs the user to manually resolve the conflicts there.
remove::
Remove the notes for a given object (defaults to HEAD).
@@ -191,8 +193,10 @@ object, in which case the history of the notes can be read with
NOTES MERGE STRATEGIES
----------------------
-The default notes merge strategy is "manual", which is not yet
-implemented (TODO).
+The default notes merge strategy is "manual", which checks out
+conflicting notes in a special work tree for resolving notes conflicts
+(`.git/NOTES_MERGE_WORKTREE`), and instructs the user to resolve the
+conflicts in that work tree.
"ours" automatically resolves conflicting notes in favor of the local
version (i.e. the current notes ref).
diff --git a/builtin/notes.c b/builtin/notes.c
index a249f19..7a2a288 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -820,14 +820,14 @@ static int merge(int argc, const char **argv, const char *prefix)
/* Update default notes ref with new commit */
update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,
0, DIE_ON_ERR);
- else
- /* TODO: */
- die("'git notes merge' cannot yet handle conflicts!");
+ else /* Merge has unresolved conflicts */
+ printf("Automatic notes merge failed. Fix conflicts in %s.\n",
+ git_path(NOTES_MERGE_WORKTREE));
free_notes(t);
strbuf_release(&remote_ref);
strbuf_release(&msg);
- return 0;
+ return result < 0; /* return non-zero on conflicts */
}
static int remove_cmd(int argc, const char **argv, const char *prefix)
diff --git a/notes-merge.c b/notes-merge.c
index f2fa12d..5734d0b 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -3,6 +3,9 @@
#include "refs.h"
#include "diff.h"
#include "diffcore.h"
+#include "xdiff-interface.h"
+#include "ll-merge.h"
+#include "dir.h"
#include "notes.h"
#include "notes-merge.h"
@@ -263,16 +266,169 @@ static void diff_tree_local(struct notes_merge_options *o,
diff_tree_release_paths(&opt);
}
+static void check_notes_merge_worktree(struct notes_merge_options *o)
+{
+ if (!o->has_worktree) {
+ /*
+ * Must establish NOTES_MERGE_WORKTREE.
+ * Abort if NOTES_MERGE_WORKTREE already exists
+ */
+ if (file_exists(git_path(NOTES_MERGE_WORKTREE))) {
+ if (advice_resolve_conflict)
+ die("You have not concluded your previous "
+ "notes merge (%s exists).\nPlease, use "
+ "'git notes merge --commit' or 'git notes "
+ "merge --reset' to commit/abort the "
+ "previous merge before you start a new "
+ "notes merge.", git_path("NOTES_MERGE_*"));
+ else
+ die("You have not concluded your notes merge "
+ "(%s exists).", git_path("NOTES_MERGE_*"));
+ }
+
+ if (safe_create_leading_directories(git_path(
+ NOTES_MERGE_WORKTREE "/.test")))
+ die_errno("unable to create directory %s",
+ git_path(NOTES_MERGE_WORKTREE));
+ o->has_worktree = 1;
+ } else if (!file_exists(git_path(NOTES_MERGE_WORKTREE)))
+ /* NOTES_MERGE_WORKTREE should already be established */
+ die("missing '%s'. This should not happen",
+ git_path(NOTES_MERGE_WORKTREE));
+}
+
+static void write_buf_to_worktree(const unsigned char *obj,
+ const char *buf, unsigned long size)
+{
+ int fd;
+ char *path = git_path(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
+ if (safe_create_leading_directories(path))
+ die_errno("unable to create directory for '%s'", path);
+ if (file_exists(path))
+ die("found existing file at '%s'", path);
+
+ fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, 0666);
+ if (fd < 0)
+ die_errno("failed to open '%s'", path);
+
+ while (size > 0) {
+ long ret = write_in_full(fd, buf, size);
+ if (ret < 0) {
+ /* Ignore epipe */
+ if (errno == EPIPE)
+ break;
+ die_errno("notes-merge");
+ } else if (!ret) {
+ die("notes-merge: disk full?");
+ }
+ size -= ret;
+ buf += ret;
+ }
+
+ close(fd);
+}
+
+static void write_note_to_worktree(const unsigned char *obj,
+ const unsigned char *note)
+{
+ enum object_type type;
+ unsigned long size;
+ void *buf = read_sha1_file(note, &type, &size);
+
+ if (!buf)
+ die("cannot read note %s for object %s",
+ sha1_to_hex(note), sha1_to_hex(obj));
+ if (type != OBJ_BLOB)
+ die("blob expected in note %s for object %s",
+ sha1_to_hex(note), sha1_to_hex(obj));
+ write_buf_to_worktree(obj, buf, size);
+ free(buf);
+}
+
+static int ll_merge_in_worktree(struct notes_merge_options *o,
+ struct notes_merge_pair *p)
+{
+ mmbuffer_t result_buf;
+ mmfile_t base, local, remote;
+ int status;
+
+ read_mmblob(&base, p->base);
+ read_mmblob(&local, p->local);
+ read_mmblob(&remote, p->remote);
+
+ status = ll_merge(&result_buf, sha1_to_hex(p->obj), &base, NULL,
+ &local, o->local_ref, &remote, o->remote_ref, 0);
+
+ free(base.ptr);
+ free(local.ptr);
+ free(remote.ptr);
+
+ if ((status < 0) || !result_buf.ptr)
+ die("Failed to execute internal merge");
+
+ write_buf_to_worktree(p->obj, result_buf.ptr, result_buf.size);
+ free(result_buf.ptr);
+
+ return status;
+}
+
+static int merge_one_change_manual(struct notes_merge_options *o,
+ struct notes_merge_pair *p,
+ struct notes_tree *t)
+{
+ const char *lref = o->local_ref ? o->local_ref : "local version";
+ const char *rref = o->remote_ref ? o->remote_ref : "remote version";
+
+ trace_printf("\t\t\tmerge_one_change_manual(obj = %.7s, base = %.7s, "
+ "local = %.7s, remote = %.7s)\n",
+ sha1_to_hex(p->obj), sha1_to_hex(p->base),
+ sha1_to_hex(p->local), sha1_to_hex(p->remote));
+
+ OUTPUT(o, 2, "Auto-merging notes for %s", sha1_to_hex(p->obj));
+ check_notes_merge_worktree(o);
+ if (is_null_sha1(p->local)) {
+ /* D/F conflict, checkout p->remote */
+ assert(!is_null_sha1(p->remote));
+ OUTPUT(o, 1, "CONFLICT (delete/modify): Notes for object %s "
+ "deleted in %s and modified in %s. Version from %s "
+ "left in tree.", sha1_to_hex(p->obj), lref, rref, rref);
+ write_note_to_worktree(p->obj, p->remote);
+ } else if (is_null_sha1(p->remote)) {
+ /* D/F conflict, checkout p->local */
+ assert(!is_null_sha1(p->local));
+ OUTPUT(o, 1, "CONFLICT (delete/modify): Notes for object %s "
+ "deleted in %s and modified in %s. Version from %s "
+ "left in tree.", sha1_to_hex(p->obj), rref, lref, lref);
+ write_note_to_worktree(p->obj, p->local);
+ } else {
+ /* "regular" conflict, checkout result of ll_merge() */
+ const char *reason = "content";
+ if (is_null_sha1(p->base))
+ reason = "add/add";
+ assert(!is_null_sha1(p->local));
+ assert(!is_null_sha1(p->remote));
+ OUTPUT(o, 1, "CONFLICT (%s): Merge conflict in notes for "
+ "object %s", reason, sha1_to_hex(p->obj));
+ ll_merge_in_worktree(o, p);
+ }
+
+ trace_printf("\t\t\tremoving from partial merge result\n");
+ remove_note(t, p->obj);
+
+ return 1;
+}
+
static int merge_one_change(struct notes_merge_options *o,
struct notes_merge_pair *p, struct notes_tree *t)
{
/*
- * Return 0 if change was resolved (and added to notes_tree),
- * 1 if conflict
+ * Return 0 if change is successfully resolved (stored in notes_tree).
+ * Return 1 is change results in a conflict (NOT stored in notes_tree,
+ * but instead written to NOTES_MERGE_WORKTREE with conflict markers).
*/
switch (o->strategy) {
case NOTES_MERGE_RESOLVE_MANUAL:
- return 1;
+ return merge_one_change_manual(o, p, t);
case NOTES_MERGE_RESOLVE_OURS:
OUTPUT(o, 2, "Using local notes for %s", sha1_to_hex(p->obj));
/* nothing to do */
@@ -478,8 +634,8 @@ int notes_merge(struct notes_merge_options *o,
result = merge_from_diffs(o, base_tree_sha1, local->tree->object.sha1,
remote->tree->object.sha1, local_tree);
- if (result > 0) { /* successful non-trivial merge */
- /* Commit result */
+ if (result != 0) { /* non-trivial merge (with or without conflicts) */
+ /* Commit (partial) result */
struct commit_list *parents = NULL;
commit_list_insert(remote, &parents); /* LIFO order */
commit_list_insert(local, &parents);
diff --git a/notes-merge.h b/notes-merge.h
index ed356ae..55ef3d9 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -1,6 +1,8 @@
#ifndef NOTES_MERGE_H
#define NOTES_MERGE_H
+#define NOTES_MERGE_WORKTREE "NOTES_MERGE_WORKTREE"
+
enum notes_merge_verbosity {
NOTES_MERGE_VERBOSITY_DEFAULT = 2,
NOTES_MERGE_VERBOSITY_MAX = 5
@@ -17,6 +19,7 @@ struct notes_merge_options {
NOTES_MERGE_RESOLVE_THEIRS,
NOTES_MERGE_RESOLVE_UNION
} strategy;
+ unsigned has_worktree:1;
};
void init_notes_merge_options(struct notes_merge_options *o);
@@ -51,7 +54,13 @@ void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
* 2. The merge successfully completes, producing a merge commit. local_tree
* contains the updated notes tree, the SHA1 of the resulting commit is
* written into 'result_sha1', and 1 is returned.
- * 3. The merge fails. result_sha1 is set to null_sha1, and -1 is returned.
+ * 3. The merge results in conflicts. This is similar to #2 in that the
+ * partial merge result (i.e. merge result minus the unmerged entries)
+ * are stored in 'local_tree', and the SHA1 or the resulting commit
+ * (to be amended when the conflicts have been resolved) is written into
+ * 'result_sha1'. The unmerged entries are written into the
+ * .git/NOTES_MERGE_WORKTREE directory with conflict markers.
+ * -1 is returned.
*
* Both o->local_ref and o->remote_ref must be given (non-NULL), but either ref
* (although not both) may refer to a non-existing notes ref, in which case
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
new file mode 100755
index 0000000..eadb6b7
--- /dev/null
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -0,0 +1,292 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Johan Herland
+#
+
+test_description='Test notes merging with manual conflict resolution'
+
+. ./test-lib.sh
+
+# Set up a notes merge scenario with different kinds of conflicts
+test_expect_success 'setup commits' '
+ test_commit 1st &&
+ test_commit 2nd &&
+ test_commit 3rd &&
+ test_commit 4th &&
+ test_commit 5th
+'
+
+commit_sha1=$(git rev-parse 1st^{commit})
+commit_sha2=$(git rev-parse 2nd^{commit})
+commit_sha3=$(git rev-parse 3rd^{commit})
+commit_sha4=$(git rev-parse 4th^{commit})
+commit_sha5=$(git rev-parse 5th^{commit})
+
+verify_notes () {
+ notes_ref="$1"
+ git -c core.notesRef="refs/notes/$notes_ref" notes |
+ sort >"output_notes_$notes_ref" &&
+ test_cmp "expect_notes_$notes_ref" "output_notes_$notes_ref" &&
+ git -c core.notesRef="refs/notes/$notes_ref" log --format="%H %s%n%N" \
+ >"output_log_$notes_ref" &&
+ test_cmp "expect_log_$notes_ref" "output_log_$notes_ref"
+}
+
+cat <<EOF | sort >expect_notes_x
+6e8e3febca3c2bb896704335cc4d0c34cb2f8715 $commit_sha4
+e5388c10860456ee60673025345fe2e153eb8cf8 $commit_sha3
+ceefa674873670e7ecd131814d909723cce2b669 $commit_sha2
+EOF
+
+cat >expect_log_x <<EOF
+$commit_sha5 5th
+
+$commit_sha4 4th
+x notes on 4th commit
+
+$commit_sha3 3rd
+x notes on 3rd commit
+
+$commit_sha2 2nd
+x notes on 2nd commit
+
+$commit_sha1 1st
+
+EOF
+
+test_expect_success 'setup merge base (x)' '
+ git config core.notesRef refs/notes/x &&
+ git notes add -m "x notes on 2nd commit" 2nd &&
+ git notes add -m "x notes on 3rd commit" 3rd &&
+ git notes add -m "x notes on 4th commit" 4th &&
+ verify_notes x
+'
+
+cat <<EOF | sort >expect_notes_y
+e2bfd06a37dd2031684a59a6e2b033e212239c78 $commit_sha4
+5772f42408c0dd6f097a7ca2d24de0e78d1c46b1 $commit_sha3
+b0a6021ec006d07e80e9b20ec9b444cbd9d560d3 $commit_sha1
+EOF
+
+cat >expect_log_y <<EOF
+$commit_sha5 5th
+
+$commit_sha4 4th
+y notes on 4th commit
+
+$commit_sha3 3rd
+y notes on 3rd commit
+
+$commit_sha2 2nd
+
+$commit_sha1 1st
+y notes on 1st commit
+
+EOF
+
+test_expect_success 'setup local branch (y)' '
+ git update-ref refs/notes/y refs/notes/x &&
+ git config core.notesRef refs/notes/y &&
+ git notes add -f -m "y notes on 1st commit" 1st &&
+ git notes remove 2nd &&
+ git notes add -f -m "y notes on 3rd commit" 3rd &&
+ git notes add -f -m "y notes on 4th commit" 4th &&
+ verify_notes y
+'
+
+cat <<EOF | sort >expect_notes_z
+cff59c793c20bb49a4e01bc06fb06bad642e0d54 $commit_sha4
+283b48219aee9a4105f6cab337e789065c82c2b9 $commit_sha2
+0a81da8956346e19bcb27a906f04af327e03e31b $commit_sha1
+EOF
+
+cat >expect_log_z <<EOF
+$commit_sha5 5th
+
+$commit_sha4 4th
+z notes on 4th commit
+
+$commit_sha3 3rd
+
+$commit_sha2 2nd
+z notes on 2nd commit
+
+$commit_sha1 1st
+z notes on 1st commit
+
+EOF
+
+test_expect_success 'setup remote branch (z)' '
+ git update-ref refs/notes/z refs/notes/x &&
+ git config core.notesRef refs/notes/z &&
+ git notes add -f -m "z notes on 1st commit" 1st &&
+ git notes add -f -m "z notes on 2nd commit" 2nd &&
+ git notes remove 3rd &&
+ git notes add -f -m "z notes on 4th commit" 4th &&
+ verify_notes z
+'
+
+# At this point, before merging z into y, we have the following status:
+#
+# commit | base/x | local/y | remote/z | diff from x to y/z
+# -------|---------|---------|----------|---------------------------
+# 1st | [none] | b0a6021 | 0a81da8 | added / added (diff)
+# 2nd | ceefa67 | [none] | 283b482 | removed / changed
+# 3rd | e5388c1 | 5772f42 | [none] | changed / removed
+# 4th | 6e8e3fe | e2bfd06 | cff59c7 | changed / changed (diff)
+# 5th | [none] | [none] | [none] | [none]
+
+cat <<EOF | sort >expect_conflicts
+$commit_sha1
+$commit_sha2
+$commit_sha3
+$commit_sha4
+EOF
+
+cat >expect_conflict_$commit_sha1 <<EOF
+<<<<<<< refs/notes/m
+y notes on 1st commit
+=======
+z notes on 1st commit
+>>>>>>> refs/notes/z
+EOF
+
+cat >expect_conflict_$commit_sha2 <<EOF
+z notes on 2nd commit
+EOF
+
+cat >expect_conflict_$commit_sha3 <<EOF
+y notes on 3rd commit
+EOF
+
+cat >expect_conflict_$commit_sha4 <<EOF
+<<<<<<< refs/notes/m
+y notes on 4th commit
+=======
+z notes on 4th commit
+>>>>>>> refs/notes/z
+EOF
+
+cp expect_notes_y expect_notes_m
+cp expect_log_y expect_log_m
+
+git rev-parse refs/notes/y > pre_merge_y
+
+test_expect_success 'merge z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
+ git update-ref refs/notes/m refs/notes/y &&
+ git config core.notesRef refs/notes/m &&
+ test_must_fail git notes merge z >output &&
+ # Output should point to where to resolve conflicts
+ grep -q "\\.git/NOTES_MERGE_WORKTREE" output &&
+ # Inspect merge conflicts
+ ls .git/NOTES_MERGE_WORKTREE >output_conflicts &&
+ test_cmp expect_conflicts output_conflicts &&
+ ( for f in $(cat expect_conflicts); do
+ test_cmp "expect_conflict_$f" ".git/NOTES_MERGE_WORKTREE/$f" ||
+ exit 1
+ done ) &&
+ # Verify that current notes tree (pre-merge) has not changed (m == y)
+ verify_notes y &&
+ verify_notes m &&
+ test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+'
+
+cat <<EOF | sort >expect_notes_z
+00494adecf2d9635a02fa431308d67993f853968 $commit_sha4
+283b48219aee9a4105f6cab337e789065c82c2b9 $commit_sha2
+0a81da8956346e19bcb27a906f04af327e03e31b $commit_sha1
+EOF
+
+cat >expect_log_z <<EOF
+$commit_sha5 5th
+
+$commit_sha4 4th
+z notes on 4th commit
+
+More z notes on 4th commit
+
+$commit_sha3 3rd
+
+$commit_sha2 2nd
+z notes on 2nd commit
+
+$commit_sha1 1st
+z notes on 1st commit
+
+EOF
+
+test_expect_success 'change notes in z' '
+ git notes --ref z append -m "More z notes on 4th commit" 4th &&
+ verify_notes z
+'
+
+test_expect_success 'cannot do merge w/conflicts when previous merge is unfinished' '
+ test -d .git/NOTES_MERGE_WORKTREE &&
+ test_must_fail git notes merge z >output 2>&1 &&
+ # Output should indicate what is wrong
+ grep -q "\\.git/NOTES_MERGE_\\* exists" output
+'
+
+# Setup non-conflicting merge between x and new notes ref w
+
+cat <<EOF | sort >expect_notes_w
+ceefa674873670e7ecd131814d909723cce2b669 $commit_sha2
+f75d1df88cbfe4258d49852f26cfc83f2ad4494b $commit_sha1
+EOF
+
+cat >expect_log_w <<EOF
+$commit_sha5 5th
+
+$commit_sha4 4th
+
+$commit_sha3 3rd
+
+$commit_sha2 2nd
+x notes on 2nd commit
+
+$commit_sha1 1st
+w notes on 1st commit
+
+EOF
+
+test_expect_success 'setup unrelated notes ref (w)' '
+ git config core.notesRef refs/notes/w &&
+ git notes add -m "w notes on 1st commit" 1st &&
+ git notes add -m "x notes on 2nd commit" 2nd &&
+ verify_notes w
+'
+
+cat <<EOF | sort >expect_notes_w
+6e8e3febca3c2bb896704335cc4d0c34cb2f8715 $commit_sha4
+e5388c10860456ee60673025345fe2e153eb8cf8 $commit_sha3
+ceefa674873670e7ecd131814d909723cce2b669 $commit_sha2
+f75d1df88cbfe4258d49852f26cfc83f2ad4494b $commit_sha1
+EOF
+
+cat >expect_log_w <<EOF
+$commit_sha5 5th
+
+$commit_sha4 4th
+x notes on 4th commit
+
+$commit_sha3 3rd
+x notes on 3rd commit
+
+$commit_sha2 2nd
+x notes on 2nd commit
+
+$commit_sha1 1st
+w notes on 1st commit
+
+EOF
+
+test_expect_success 'can do merge without conflicts even if previous merge is unfinished (x => w)' '
+ test -d .git/NOTES_MERGE_WORKTREE &&
+ git notes merge x &&
+ verify_notes w &&
+ # Verify that other notes refs has not changed (x and y)
+ verify_notes x &&
+ verify_notes y
+'
+
+test_done
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 16/21] git notes merge: Manual conflict resolution, part 2/2
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (14 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 15/21] git notes merge: Manual conflict resolution, part 1/2 Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 17/21] git notes merge: List conflicting notes in notes merge commit message Johan Herland
` (5 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
When the notes merge conflicts in .git/NOTES_MERGE_WORKTREE have been
resolved, we need to record a new notes commit on the appropriate notes
ref with the resolved notes.
This patch implements 'git notes merge --commit' which the user should
run after resolving conflicts in the notes merge worktree. This command
finalizes the notes merge by recombining the partial notes tree from
part 1 with the now-resolved conflicts in the notes merge worktree in a
merge commit, and updating the appropriate ref to this merge commit.
In order to correctly finalize the merge, we need to keep track of three
things:
- The partial merge result from part 1, containing the auto-merged notes.
This is now stored into a ref called .git/NOTES_MERGE_PARTIAL.
- The unmerged notes. These are already stored in
.git/NOTES_MERGE_WORKTREE, thanks to part 1.
- The notes ref to be updated by the finalized merge result. This is now
stored in a symref called .git/NOTES_MERGE_REF.
In addition to "git notes merge --commit", which uses the above details
to create the finalized notes merge commit, this patch also implements
"git notes merge --reset", which aborts the ongoing notes merge by simply
removing the files/directory described above.
FTR, "git notes merge --commit" reuses "git notes merge --reset" to remove
the information described above (.git/NOTES_MERGE_*) after the notes merge
have been successfully finalized.
The patch also contains documentation and testcases for the two new options.
This patch has been improved by the following contributions:
- Ævar Arnfjörð Bjarmason: Fix nonsense sentence in --commit description
Thanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
Documentation/git-notes.txt | 22 ++++
builtin/notes.c | 106 +++++++++++++++++++-
notes-merge.c | 69 +++++++++++++
notes-merge.h | 23 +++++
t/t3310-notes-merge-manual-resolve.sh | 176 +++++++++++++++++++++++++++++++++
5 files changed, 393 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index f003b7c..5bb9784 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -15,6 +15,8 @@ SYNOPSIS
'git notes' edit [<object>]
'git notes' show [<object>]
'git notes' merge [-v | -q] [-s <strategy> ] <notes_ref>
+'git notes' merge --commit [-v | -q]
+'git notes' merge --reset [-v | -q]
'git notes' remove [<object>]
'git notes' prune [-n | -v]
@@ -95,6 +97,9 @@ conflicting notes (see the -s/--strategy option) is not given,
the "manual" resolver is used. This resolver checks out the
conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
and instructs the user to manually resolve the conflicts there.
+When done, the user can either finalize the merge with
+'git notes merge --commit', or abort the merge with
+'git notes merge --reset'.
remove::
Remove the notes for a given object (defaults to HEAD).
@@ -154,6 +159,20 @@ OPTIONS
See the "NOTES MERGE STRATEGIES" section below for more
information on each notes merge strategy.
+--commit::
+ Finalize an in-progress 'git notes merge'. Use this option
+ when you have resolved the conflicts that 'git notes merge'
+ stored in .git/NOTES_MERGE_WORKTREE. This amends the partial
+ merge commit created by 'git notes merge' (stored in
+ .git/NOTES_MERGE_PARTIAL) by adding the notes in
+ .git/NOTES_MERGE_WORKTREE. The notes ref stored in the
+ .git/NOTES_MERGE_REF symref is updated to the resulting commit.
+
+--reset::
+ Reset/abort a in-progress 'git notes merge', i.e. a notes merge
+ with conflicts. This simply removes all files related to the
+ notes merge.
+
-q::
--quiet::
When merging notes, operate quietly.
@@ -197,6 +216,9 @@ The default notes merge strategy is "manual", which checks out
conflicting notes in a special work tree for resolving notes conflicts
(`.git/NOTES_MERGE_WORKTREE`), and instructs the user to resolve the
conflicts in that work tree.
+When done, the user can either finalize the merge with
+'git notes merge --commit', or abort the merge with
+'git notes merge --reset'.
"ours" automatically resolves conflicting notes in favor of the local
version (i.e. the current notes ref).
diff --git a/builtin/notes.c b/builtin/notes.c
index 7a2a288..f604eec 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -27,6 +27,8 @@ static const char * const git_notes_usage[] = {
"git notes [--ref <notes_ref>] edit [<object>]",
"git notes [--ref <notes_ref>] show [<object>]",
"git notes [--ref <notes_ref>] merge [-v | -q] [-s <strategy> ] <notes_ref>",
+ "git notes merge --commit [-v | -q]",
+ "git notes merge --reset [-v | -q]",
"git notes [--ref <notes_ref>] remove [<object>]",
"git notes [--ref <notes_ref>] prune [-n | -v]",
NULL
@@ -65,6 +67,8 @@ static const char * const git_notes_show_usage[] = {
static const char * const git_notes_merge_usage[] = {
"git notes merge [<options>] <notes_ref>",
+ "git notes merge --commit [<options>]",
+ "git notes merge --reset [<options>]",
NULL
};
@@ -761,33 +765,119 @@ static int show(int argc, const char **argv, const char *prefix)
return retval;
}
+static int merge_reset(struct notes_merge_options *o)
+{
+ int ret = 0;
+
+ /*
+ * Remove .git/NOTES_MERGE_PARTIAL and .git/NOTES_MERGE_REF, and call
+ * notes_merge_reset() to remove .git/NOTES_MERGE_WORKTREE.
+ */
+
+ if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0))
+ ret += error("Failed to delete ref NOTES_MERGE_PARTIAL");
+ if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF))
+ ret += error("Failed to delete ref NOTES_MERGE_REF");
+ if (notes_merge_reset(o))
+ ret += error("Failed to remove 'git notes merge' worktree");
+ return ret;
+}
+
+static int merge_commit(struct notes_merge_options *o)
+{
+ struct strbuf msg = STRBUF_INIT;
+ unsigned char sha1[20];
+ struct notes_tree *t;
+ struct commit *partial;
+ struct pretty_print_context pretty_ctx;
+
+ /*
+ * Read partial merge result from .git/NOTES_MERGE_PARTIAL,
+ * and target notes ref from .git/NOTES_MERGE_REF.
+ */
+
+ if (get_sha1("NOTES_MERGE_PARTIAL", sha1))
+ die("Failed to read ref NOTES_MERGE_PARTIAL");
+ else if (!(partial = lookup_commit_reference(sha1)))
+ die("Could not find commit from NOTES_MERGE_PARTIAL.");
+ else if (parse_commit(partial))
+ die("Could not parse commit from NOTES_MERGE_PARTIAL.");
+
+ t = xcalloc(1, sizeof(struct notes_tree));
+ init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0);
+
+ o->local_ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, 0);
+ if (!o->local_ref)
+ die("Failed to resolve NOTES_MERGE_REF");
+
+ if (notes_merge_commit(o, t, partial, sha1))
+ die("Failed to finalize notes merge");
+
+ /* Reuse existing commit message in reflog message */
+ memset(&pretty_ctx, 0, sizeof(pretty_ctx));
+ format_commit_message(partial, "%s", &msg, &pretty_ctx);
+ strbuf_trim(&msg);
+ strbuf_insert(&msg, 0, "notes: ", 7);
+ update_ref(msg.buf, o->local_ref, sha1, NULL, 0, DIE_ON_ERR);
+
+ free_notes(t);
+ strbuf_release(&msg);
+ return merge_reset(o);
+}
+
static int merge(int argc, const char **argv, const char *prefix)
{
struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
unsigned char result_sha1[20];
struct notes_tree *t;
struct notes_merge_options o;
+ int do_merge = 0, do_commit = 0, do_reset = 0;
int verbosity = 0, result;
const char *strategy = NULL;
struct option options[] = {
+ OPT_GROUP("General options"),
OPT__VERBOSITY(&verbosity),
+ OPT_GROUP("Merge options"),
OPT_STRING('s', "strategy", &strategy, "strategy",
"resolve notes conflicts using the given "
"strategy (manual/ours/theirs/union)"),
+ OPT_GROUP("Committing unmerged notes"),
+ { OPTION_BOOLEAN, 0, "commit", &do_commit, NULL,
+ "finalize notes merge by committing unmerged notes",
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG },
+ OPT_GROUP("Aborting notes merge resolution"),
+ { OPTION_BOOLEAN, 0, "reset", &do_reset, NULL,
+ "abort notes merge",
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG },
OPT_END()
};
argc = parse_options(argc, argv, prefix, options,
git_notes_merge_usage, 0);
- if (argc != 1) {
+ if (strategy || do_commit + do_reset == 0)
+ do_merge = 1;
+ if (do_commit + do_reset + do_merge != 1) {
+ error("cannot mix --commit, --reset or -s/--strategy");
+ usage_with_options(git_notes_merge_usage, options);
+ }
+
+ if (do_merge && argc != 1) {
error("Must specify a notes ref to merge");
usage_with_options(git_notes_merge_usage, options);
+ } else if (!do_merge && argc) {
+ error("too many parameters");
+ usage_with_options(git_notes_merge_usage, options);
}
init_notes_merge_options(&o);
o.verbosity = verbosity + NOTES_MERGE_VERBOSITY_DEFAULT;
+ if (do_reset)
+ return merge_reset(&o);
+ if (do_commit)
+ return merge_commit(&o);
+
o.local_ref = default_notes_ref();
strbuf_addstr(&remote_ref, argv[0]);
expand_notes_ref(&remote_ref);
@@ -820,9 +910,19 @@ static int merge(int argc, const char **argv, const char *prefix)
/* Update default notes ref with new commit */
update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,
0, DIE_ON_ERR);
- else /* Merge has unresolved conflicts */
- printf("Automatic notes merge failed. Fix conflicts in %s.\n",
+ else { /* Merge has unresolved conflicts */
+ /* Update .git/NOTES_MERGE_PARTIAL with partial merge result */
+ update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL,
+ 0, DIE_ON_ERR);
+ /* Store ref-to-be-updated into .git/NOTES_MERGE_REF */
+ if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL))
+ die("Failed to store link to current notes ref (%s)",
+ default_notes_ref());
+ printf("Automatic notes merge failed. Fix conflicts in %s and "
+ "commit the result with 'git notes merge --commit', or "
+ "abort the merge with 'git notes merge --reset'.\n",
git_path(NOTES_MERGE_WORKTREE));
+ }
free_notes(t);
strbuf_release(&remote_ref);
diff --git a/notes-merge.c b/notes-merge.c
index 5734d0b..8ae63b1 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -649,3 +649,72 @@ found_result:
result, sha1_to_hex(result_sha1));
return result;
}
+
+int notes_merge_commit(struct notes_merge_options *o,
+ struct notes_tree *partial_tree,
+ struct commit *partial_commit,
+ unsigned char *result_sha1)
+{
+ /*
+ * Iterate through files in .git/NOTES_MERGE_WORKTREE and add all
+ * found notes to 'partial_tree'. Write the updates notes tree to
+ * the DB, and commit the resulting tree object while reusing the
+ * commit message and parents from 'partial_commit'.
+ * Finally store the new commit object SHA1 into 'result_sha1'.
+ */
+ struct dir_struct dir;
+ const char *path = git_path(NOTES_MERGE_WORKTREE "/");
+ int path_len = strlen(path), i;
+ const char *msg = strstr(partial_commit->buffer, "\n\n");
+
+ OUTPUT(o, 3, "Committing notes in notes merge worktree at %.*s",
+ path_len - 1, path);
+
+ if (!msg || msg[2] == '\0')
+ die("partial notes commit has empty message");
+ msg += 2;
+
+ memset(&dir, 0, sizeof(dir));
+ read_directory(&dir, path, path_len, NULL);
+ for (i = 0; i < dir.nr; i++) {
+ struct dir_entry *ent = dir.entries[i];
+ struct stat st;
+ const char *relpath = ent->name + path_len;
+ unsigned char obj_sha1[20], blob_sha1[20];
+
+ if (ent->len - path_len != 40 || get_sha1_hex(relpath, obj_sha1)) {
+ OUTPUT(o, 3, "Skipping non-SHA1 entry '%s'", ent->name);
+ continue;
+ }
+
+ /* write file as blob, and add to partial_tree */
+ if (stat(ent->name, &st))
+ die_errno("Failed to stat '%s'", ent->name);
+ if (index_path(blob_sha1, ent->name, &st, 1))
+ die("Failed to write blob object from '%s'", ent->name);
+ if (add_note(partial_tree, obj_sha1, blob_sha1, NULL))
+ die("Failed to add resolved note '%s' to notes tree",
+ ent->name);
+ OUTPUT(o, 4, "Added resolved note for object %s: %s",
+ sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
+ }
+
+ create_notes_commit(partial_tree, partial_commit->parents, msg,
+ result_sha1);
+ OUTPUT(o, 4, "Finalized notes merge commit: %s",
+ sha1_to_hex(result_sha1));
+ return 0;
+}
+
+int notes_merge_reset(struct notes_merge_options *o)
+{
+ /* Remove .git/NOTES_MERGE_WORKTREE directory and all files within */
+ struct strbuf buf = STRBUF_INIT;
+ int ret;
+
+ strbuf_addstr(&buf, git_path(NOTES_MERGE_WORKTREE));
+ OUTPUT(o, 3, "Removing notes merge worktree at %s", buf.buf);
+ ret = remove_dir_recursively(&buf, 0);
+ strbuf_release(&buf);
+ return ret;
+}
diff --git a/notes-merge.h b/notes-merge.h
index 55ef3d9..b8f20aa 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -71,4 +71,27 @@ int notes_merge(struct notes_merge_options *o,
struct notes_tree *local_tree,
unsigned char *result_sha1);
+/*
+ * Finalize conflict resolution from an earlier notes_merge()
+ *
+ * The given notes tree 'partial_tree' must be the notes_tree corresponding to
+ * the given 'partial_commit', the partial result commit created by a previous
+ * call to notes_merge().
+ *
+ * This function will add the (now resolved) notes in .git/NOTES_MERGE_WORKTREE
+ * to 'partial_tree', and create a final notes merge commit, the SHA1 of which
+ * will be stored in 'result_sha1'.
+ */
+int notes_merge_commit(struct notes_merge_options *o,
+ struct notes_tree *partial_tree,
+ struct commit *partial_commit,
+ unsigned char *result_sha1);
+
+/*
+ * Abort conflict resolution from an earlier notes_merge()
+ *
+ * Removes the notes merge worktree in .git/NOTES_MERGE_WORKTREE.
+ */
+int notes_merge_reset(struct notes_merge_options *o);
+
#endif
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index eadb6b7..1db0649 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -171,6 +171,7 @@ cp expect_notes_y expect_notes_m
cp expect_log_y expect_log_m
git rev-parse refs/notes/y > pre_merge_y
+git rev-parse refs/notes/z > pre_merge_z
test_expect_success 'merge z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
git update-ref refs/notes/m refs/notes/y &&
@@ -289,4 +290,179 @@ test_expect_success 'can do merge without conflicts even if previous merge is un
verify_notes y
'
+cat <<EOF | sort >expect_notes_m
+021faa20e931fb48986ffc6282b4bb05553ac946 $commit_sha4
+5772f42408c0dd6f097a7ca2d24de0e78d1c46b1 $commit_sha3
+283b48219aee9a4105f6cab337e789065c82c2b9 $commit_sha2
+0a59e787e6d688aa6309e56e8c1b89431a0fc1c1 $commit_sha1
+EOF
+
+cat >expect_log_m <<EOF
+$commit_sha5 5th
+
+$commit_sha4 4th
+y and z notes on 4th commit
+
+$commit_sha3 3rd
+y notes on 3rd commit
+
+$commit_sha2 2nd
+z notes on 2nd commit
+
+$commit_sha1 1st
+y and z notes on 1st commit
+
+EOF
+
+test_expect_success 'finalize conflicting merge (z => m)' '
+ # Resolve conflicts and finalize merge
+ cat >.git/NOTES_MERGE_WORKTREE/$commit_sha1 <<EOF &&
+y and z notes on 1st commit
+EOF
+ cat >.git/NOTES_MERGE_WORKTREE/$commit_sha4 <<EOF &&
+y and z notes on 4th commit
+EOF
+ git notes merge --commit &&
+ # No .git/NOTES_MERGE_* files left
+ test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+ test_cmp /dev/null output &&
+ # Merge commit has pre-merge y and pre-merge z as parents
+ test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
+ test "$(git rev-parse refs/notes/m^2)" = "$(cat pre_merge_z)" &&
+ # Merge commit mentions the notes refs merged
+ git log -1 --format=%B refs/notes/m > merge_commit_msg &&
+ grep -q refs/notes/m merge_commit_msg &&
+ grep -q refs/notes/z merge_commit_msg &&
+ # Verify contents of merge result
+ verify_notes m &&
+ # Verify that other notes refs has not changed (w, x, y and z)
+ verify_notes w &&
+ verify_notes x &&
+ verify_notes y &&
+ verify_notes z
+'
+
+cat >expect_conflict_$commit_sha4 <<EOF
+<<<<<<< refs/notes/m
+y notes on 4th commit
+=======
+z notes on 4th commit
+
+More z notes on 4th commit
+>>>>>>> refs/notes/z
+EOF
+
+cp expect_notes_y expect_notes_m
+cp expect_log_y expect_log_m
+
+git rev-parse refs/notes/y > pre_merge_y
+git rev-parse refs/notes/z > pre_merge_z
+
+test_expect_success 'redo merge of z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
+ git update-ref refs/notes/m refs/notes/y &&
+ git config core.notesRef refs/notes/m &&
+ test_must_fail git notes merge z >output &&
+ # Output should point to where to resolve conflicts
+ grep -q "\\.git/NOTES_MERGE_WORKTREE" output &&
+ # Inspect merge conflicts
+ ls .git/NOTES_MERGE_WORKTREE >output_conflicts &&
+ test_cmp expect_conflicts output_conflicts &&
+ ( for f in $(cat expect_conflicts); do
+ test_cmp "expect_conflict_$f" ".git/NOTES_MERGE_WORKTREE/$f" ||
+ exit 1
+ done ) &&
+ # Verify that current notes tree (pre-merge) has not changed (m == y)
+ verify_notes y &&
+ verify_notes m &&
+ test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+'
+
+test_expect_success 'abort notes merge' '
+ git notes merge --reset &&
+ # No .git/NOTES_MERGE_* files left
+ test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+ test_cmp /dev/null output &&
+ # m has not moved (still == y)
+ test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+ # Verify that other notes refs has not changed (w, x, y and z)
+ verify_notes w &&
+ verify_notes x &&
+ verify_notes y &&
+ verify_notes z
+'
+
+git rev-parse refs/notes/y > pre_merge_y
+git rev-parse refs/notes/z > pre_merge_z
+
+test_expect_success 'redo merge of z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
+ test_must_fail git notes merge z >output &&
+ # Output should point to where to resolve conflicts
+ grep -q "\\.git/NOTES_MERGE_WORKTREE" output &&
+ # Inspect merge conflicts
+ ls .git/NOTES_MERGE_WORKTREE >output_conflicts &&
+ test_cmp expect_conflicts output_conflicts &&
+ ( for f in $(cat expect_conflicts); do
+ test_cmp "expect_conflict_$f" ".git/NOTES_MERGE_WORKTREE/$f" ||
+ exit 1
+ done ) &&
+ # Verify that current notes tree (pre-merge) has not changed (m == y)
+ verify_notes y &&
+ verify_notes m &&
+ test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+'
+
+cat <<EOF | sort >expect_notes_m
+304dfb4325cf243025b9957486eb605a9b51c199 $commit_sha5
+283b48219aee9a4105f6cab337e789065c82c2b9 $commit_sha2
+0a59e787e6d688aa6309e56e8c1b89431a0fc1c1 $commit_sha1
+EOF
+
+cat >expect_log_m <<EOF
+$commit_sha5 5th
+new note on 5th commit
+
+$commit_sha4 4th
+
+$commit_sha3 3rd
+
+$commit_sha2 2nd
+z notes on 2nd commit
+
+$commit_sha1 1st
+y and z notes on 1st commit
+
+EOF
+
+test_expect_success 'add + remove notes in finalized merge (z => m)' '
+ # Resolve one conflict
+ cat >.git/NOTES_MERGE_WORKTREE/$commit_sha1 <<EOF &&
+y and z notes on 1st commit
+EOF
+ # Remove another conflict
+ rm .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
+ # Remove a D/F conflict
+ rm .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
+ # Add a new note
+ echo "new note on 5th commit" > .git/NOTES_MERGE_WORKTREE/$commit_sha5 &&
+ # Finalize merge
+ git notes merge --commit &&
+ # No .git/NOTES_MERGE_* files left
+ test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+ test_cmp /dev/null output &&
+ # Merge commit has pre-merge y and pre-merge z as parents
+ test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
+ test "$(git rev-parse refs/notes/m^2)" = "$(cat pre_merge_z)" &&
+ # Merge commit mentions the notes refs merged
+ git log -1 --format=%B refs/notes/m > merge_commit_msg &&
+ grep -q refs/notes/m merge_commit_msg &&
+ grep -q refs/notes/z merge_commit_msg &&
+ # Verify contents of merge result
+ verify_notes m &&
+ # Verify that other notes refs has not changed (w, x, y and z)
+ verify_notes w &&
+ verify_notes x &&
+ verify_notes y &&
+ verify_notes z
+'
+
test_done
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 17/21] git notes merge: List conflicting notes in notes merge commit message
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (15 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 16/21] git notes merge: Manual conflict resolution, part 2/2 Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 18/21] git notes merge: --commit should fail if underlying notes ref has moved Johan Herland
` (4 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
This brings notes merge in line with regular merge's behaviour.
This patch has been improved by the following contributions:
- Ævar Arnfjörð Bjarmason: Don't use C99 comments.
Thanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin/notes.c | 2 +-
notes-merge.c | 11 ++++++++++-
notes-merge.h | 2 +-
t/t3310-notes-merge-manual-resolve.sh | 12 ++++++++++++
4 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index f604eec..3c373ec 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -902,7 +902,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());
- o.commit_msg = msg.buf + 7; // skip "notes: " prefix
+ strbuf_add(&(o.commit_msg), msg.buf + 7, msg.len - 7); /* skip "notes: " */
result = notes_merge(&o, t, result_sha1);
diff --git a/notes-merge.c b/notes-merge.c
index 8ae63b1..c7659ce 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -8,6 +8,7 @@
#include "dir.h"
#include "notes.h"
#include "notes-merge.h"
+#include "strbuf.h"
struct notes_merge_pair {
unsigned char obj[20], base[20], local[20], remote[20];
@@ -16,6 +17,7 @@ struct notes_merge_pair {
void init_notes_merge_options(struct notes_merge_options *o)
{
memset(o, 0, sizeof(struct notes_merge_options));
+ strbuf_init(&(o->commit_msg), 0);
o->verbosity = NOTES_MERGE_VERBOSITY_DEFAULT;
}
@@ -384,6 +386,12 @@ static int merge_one_change_manual(struct notes_merge_options *o,
sha1_to_hex(p->obj), sha1_to_hex(p->base),
sha1_to_hex(p->local), sha1_to_hex(p->remote));
+ /* add "Conflicts:" section to commit message first time through */
+ if (!o->has_worktree)
+ strbuf_addstr(&(o->commit_msg), "\n\nConflicts:\n");
+
+ strbuf_addf(&(o->commit_msg), "\t%s\n", sha1_to_hex(p->obj));
+
OUTPUT(o, 2, "Auto-merging notes for %s", sha1_to_hex(p->obj));
check_notes_merge_worktree(o);
if (is_null_sha1(p->local)) {
@@ -639,12 +647,13 @@ int notes_merge(struct notes_merge_options *o,
struct commit_list *parents = NULL;
commit_list_insert(remote, &parents); /* LIFO order */
commit_list_insert(local, &parents);
- create_notes_commit(local_tree, parents, o->commit_msg,
+ create_notes_commit(local_tree, parents, o->commit_msg.buf,
result_sha1);
}
found_result:
free_commit_list(bases);
+ strbuf_release(&(o->commit_msg));
trace_printf("notes_merge(): result = %i, result_sha1 = %.7s\n",
result, sha1_to_hex(result_sha1));
return result;
diff --git a/notes-merge.h b/notes-merge.h
index b8f20aa..f9988cd 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -11,7 +11,7 @@ enum notes_merge_verbosity {
struct notes_merge_options {
const char *local_ref;
const char *remote_ref;
- const char *commit_msg;
+ struct strbuf commit_msg;
int verbosity;
enum {
NOTES_MERGE_RESOLVE_MANUAL = 0,
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 1db0649..8568307 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -333,6 +333,12 @@ EOF
git log -1 --format=%B refs/notes/m > merge_commit_msg &&
grep -q refs/notes/m merge_commit_msg &&
grep -q refs/notes/z merge_commit_msg &&
+ # Merge commit mentions conflicting notes
+ grep -q "Conflicts" merge_commit_msg &&
+ ( for sha1 in $(cat expect_conflicts); do
+ grep -q "$sha1" merge_commit_msg ||
+ exit 1
+ done ) &&
# Verify contents of merge result
verify_notes m &&
# Verify that other notes refs has not changed (w, x, y and z)
@@ -456,6 +462,12 @@ EOF
git log -1 --format=%B refs/notes/m > merge_commit_msg &&
grep -q refs/notes/m merge_commit_msg &&
grep -q refs/notes/z merge_commit_msg &&
+ # Merge commit mentions conflicting notes
+ grep -q "Conflicts" merge_commit_msg &&
+ ( for sha1 in $(cat expect_conflicts); do
+ grep -q "$sha1" merge_commit_msg ||
+ exit 1
+ done ) &&
# Verify contents of merge result
verify_notes m &&
# Verify that other notes refs has not changed (w, x, y and z)
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 18/21] git notes merge: --commit should fail if underlying notes ref has moved
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (16 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 17/21] git notes merge: List conflicting notes in notes merge commit message Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 19/21] git notes merge: Add another auto-resolving strategy: "cat_sort_uniq" Johan Herland
` (3 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
When manually resolving a notes merge, if the merging ref has moved since
the merge started, we should fail to complete the merge, and alert the user
to what's going on.
This situation may arise if you start a 'git notes merge' which results in
conflicts, and you then update the current notes ref (using for example
'git notes add/copy/amend/edit/remove/prune', 'git update-ref', etc.),
before you get around to resolving the notes conflicts and calling
'git notes merge --commit'.
We detect this situation by comparing the first parent of the partial merge
commit (which was created when the merge started) to the current value of the
merging notes ref (pointed to by the .git/NOTES_MERGE_REF symref).
If we don't fail in this situation, the notes merge commit would overwrite
the updated notes ref, thus losing the changes that happened in the meantime.
The patch includes a testcase verifying that we fail correctly in this
situation.
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin/notes.c | 11 ++++-
t/t3310-notes-merge-manual-resolve.sh | 76 +++++++++++++++++++++++++++++++++
2 files changed, 85 insertions(+), 2 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 3c373ec..d3754ab 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -786,7 +786,7 @@ static int merge_reset(struct notes_merge_options *o)
static int merge_commit(struct notes_merge_options *o)
{
struct strbuf msg = STRBUF_INIT;
- unsigned char sha1[20];
+ unsigned char sha1[20], parent_sha1[20];
struct notes_tree *t;
struct commit *partial;
struct pretty_print_context pretty_ctx;
@@ -803,6 +803,11 @@ static int merge_commit(struct notes_merge_options *o)
else if (parse_commit(partial))
die("Could not parse commit from NOTES_MERGE_PARTIAL.");
+ if (partial->parents)
+ hashcpy(parent_sha1, partial->parents->item->object.sha1);
+ else
+ hashclr(parent_sha1);
+
t = xcalloc(1, sizeof(struct notes_tree));
init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0);
@@ -818,7 +823,9 @@ static int merge_commit(struct notes_merge_options *o)
format_commit_message(partial, "%s", &msg, &pretty_ctx);
strbuf_trim(&msg);
strbuf_insert(&msg, 0, "notes: ", 7);
- update_ref(msg.buf, o->local_ref, sha1, NULL, 0, DIE_ON_ERR);
+ update_ref(msg.buf, o->local_ref, sha1,
+ is_null_sha1(parent_sha1) ? NULL : parent_sha1,
+ 0, DIE_ON_ERR);
free_notes(t);
strbuf_release(&msg);
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 8568307..dfd4954 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -477,4 +477,80 @@ EOF
verify_notes z
'
+cp expect_notes_y expect_notes_m
+cp expect_log_y expect_log_m
+
+test_expect_success 'redo merge of z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
+ git update-ref refs/notes/m refs/notes/y &&
+ test_must_fail git notes merge z >output &&
+ # Output should point to where to resolve conflicts
+ grep -q "\\.git/NOTES_MERGE_WORKTREE" output &&
+ # Inspect merge conflicts
+ ls .git/NOTES_MERGE_WORKTREE >output_conflicts &&
+ test_cmp expect_conflicts output_conflicts &&
+ ( for f in $(cat expect_conflicts); do
+ test_cmp "expect_conflict_$f" ".git/NOTES_MERGE_WORKTREE/$f" ||
+ exit 1
+ done ) &&
+ # Verify that current notes tree (pre-merge) has not changed (m == y)
+ verify_notes y &&
+ verify_notes m &&
+ test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+'
+
+cp expect_notes_w expect_notes_m
+cp expect_log_w expect_log_m
+
+test_expect_success 'reset notes ref m to somewhere else (w)' '
+ git update-ref refs/notes/m refs/notes/w &&
+ verify_notes m &&
+ test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
+'
+
+test_expect_success 'fail to finalize conflicting merge if underlying ref has moved in the meantime (m != NOTES_MERGE_PARTIAL^1)' '
+ # Resolve conflicts
+ cat >.git/NOTES_MERGE_WORKTREE/$commit_sha1 <<EOF &&
+y and z notes on 1st commit
+EOF
+ cat >.git/NOTES_MERGE_WORKTREE/$commit_sha4 <<EOF &&
+y and z notes on 4th commit
+EOF
+ # Fail to finalize merge
+ test_must_fail git notes merge --commit >output 2>&1 &&
+ # .git/NOTES_MERGE_* must remain
+ test -f .git/NOTES_MERGE_PARTIAL &&
+ test -f .git/NOTES_MERGE_REF &&
+ test -f .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
+ test -f .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
+ test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
+ test -f .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
+ # Refs are unchanged
+ test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
+ test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)"
+ test "$(git rev-parse refs/notes/m)" != "$(git rev-parse NOTES_MERGE_PARTIAL^1)"
+ # Mention refs/notes/m, and its current and expected value in output
+ grep -q "refs/notes/m" output &&
+ grep -q "$(git rev-parse refs/notes/m)" output &&
+ grep -q "$(git rev-parse NOTES_MERGE_PARTIAL^1)" output &&
+ # Verify that other notes refs has not changed (w, x, y and z)
+ verify_notes w &&
+ verify_notes x &&
+ verify_notes y &&
+ verify_notes z
+'
+
+test_expect_success 'resolve situation by aborting the notes merge' '
+ git notes merge --reset &&
+ # No .git/NOTES_MERGE_* files left
+ test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+ test_cmp /dev/null output &&
+ # m has not moved (still == w)
+ test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
+ # Verify that other notes refs has not changed (w, x, y and z)
+ verify_notes w &&
+ verify_notes x &&
+ verify_notes y &&
+ verify_notes z
+'
+
test_done
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 19/21] git notes merge: Add another auto-resolving strategy: "cat_sort_uniq"
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (17 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 18/21] git notes merge: --commit should fail if underlying notes ref has moved Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 20/21] git notes merge: Add testcases for merging notes trees at different fanouts Johan Herland
` (2 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
This new strategy is similar to "concatenate", but in addition to
concatenating the two note candidates, this strategy sorts the resulting
lines, and removes duplicate lines from the result. This is equivalent to
applying the "cat | sort | uniq" shell pipeline to the two note candidates.
This strategy is useful if the notes follow a line-based format where one
wants to avoid duplicate lines in the merge result.
Note that if either of the note candidates contain duplicate lines _prior_
to the merge, these will also be removed by this merge strategy.
The patch also contains tests and documentation for the new strategy.
Signed-off-by: Johan Herland <johan@herland.net>
---
Documentation/git-notes.txt | 12 +++-
builtin/notes.c | 8 ++-
notes-merge.c | 6 ++
notes-merge.h | 3 +-
notes.c | 76 ++++++++++++++++++
notes.h | 1 +
t/t3309-notes-merge-auto-resolve.sh | 145 +++++++++++++++++++++++++++++++++++
7 files changed, 247 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 5bb9784..466885a 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -155,7 +155,7 @@ OPTIONS
--strategy=<strategy>::
When merging notes, resolve notes conflicts using the given
strategy. The following strategies are recognized: "manual"
- (default), "ours", "theirs" and "union".
+ (default), "ours", "theirs", "union" and "cat_sort_uniq".
See the "NOTES MERGE STRATEGIES" section below for more
information on each notes merge strategy.
@@ -230,6 +230,16 @@ ref).
"union" automatically resolves notes conflicts by concatenating the
local and remote versions.
+"cat_sort_uniq" is similar to "union", but in addition to concatenating
+the local and remote versions, this strategy also sorts the resulting
+lines, and removes duplicate lines from the result. This is equivalent
+to applying the "cat | sort | uniq" shell pipeline to the local and
+remote versions. This strategy is useful if the notes follow a line-based
+format where one wants to avoid duplicated lines in the merge result.
+Note that if either the local or remote version contain duplicate lines
+prior to the merge, these will also be removed by this notes merge
+strategy.
+
EXAMPLES
--------
diff --git a/builtin/notes.c b/builtin/notes.c
index d3754ab..abf26e5 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -324,6 +324,8 @@ combine_notes_fn parse_combine_notes_fn(const char *v)
return combine_notes_ignore;
else if (!strcasecmp(v, "concatenate"))
return combine_notes_concatenate;
+ else if (!strcasecmp(v, "cat_sort_uniq"))
+ return combine_notes_cat_sort_uniq;
else
return NULL;
}
@@ -846,8 +848,8 @@ static int merge(int argc, const char **argv, const char *prefix)
OPT__VERBOSITY(&verbosity),
OPT_GROUP("Merge options"),
OPT_STRING('s', "strategy", &strategy, "strategy",
- "resolve notes conflicts using the given "
- "strategy (manual/ours/theirs/union)"),
+ "resolve notes conflicts using the given strategy "
+ "(manual/ours/theirs/union/cat_sort_uniq)"),
OPT_GROUP("Committing unmerged notes"),
{ OPTION_BOOLEAN, 0, "commit", &do_commit, NULL,
"finalize notes merge by committing unmerged notes",
@@ -899,6 +901,8 @@ static int merge(int argc, const char **argv, const char *prefix)
o.strategy = NOTES_MERGE_RESOLVE_THEIRS;
else if (!strcmp(strategy, "union"))
o.strategy = NOTES_MERGE_RESOLVE_UNION;
+ else if (!strcmp(strategy, "cat_sort_uniq"))
+ o.strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
else {
error("Unknown -s/--strategy: %s", strategy);
usage_with_options(git_notes_merge_usage, options);
diff --git a/notes-merge.c b/notes-merge.c
index c7659ce..c32979e 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -452,6 +452,12 @@ static int merge_one_change(struct notes_merge_options *o,
if (add_note(t, p->obj, p->remote, combine_notes_concatenate))
die("confused: combine_notes_concatenate failed");
return 0;
+ case NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ:
+ OUTPUT(o, 2, "Concatenating unique lines in local and remote "
+ "notes for %s", sha1_to_hex(p->obj));
+ if (add_note(t, p->obj, p->remote, combine_notes_cat_sort_uniq))
+ die("confused: combine_notes_cat_sort_uniq failed");
+ return 0;
}
die("Unknown strategy (%i).", o->strategy);
}
diff --git a/notes-merge.h b/notes-merge.h
index f9988cd..1bb9cb7 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -17,7 +17,8 @@ struct notes_merge_options {
NOTES_MERGE_RESOLVE_MANUAL = 0,
NOTES_MERGE_RESOLVE_OURS,
NOTES_MERGE_RESOLVE_THEIRS,
- NOTES_MERGE_RESOLVE_UNION
+ NOTES_MERGE_RESOLVE_UNION,
+ NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ
} strategy;
unsigned has_worktree:1;
};
diff --git a/notes.c b/notes.c
index a7a901a..521f5d0 100644
--- a/notes.c
+++ b/notes.c
@@ -847,6 +847,82 @@ int combine_notes_ignore(unsigned char *cur_sha1,
return 0;
}
+static int string_list_add_note_lines(struct string_list *sort_uniq_list,
+ const unsigned char *sha1)
+{
+ char *data;
+ unsigned long len;
+ enum object_type t;
+ struct strbuf buf = STRBUF_INIT;
+ struct strbuf **lines = NULL;
+ int i, list_index;
+
+ if (is_null_sha1(sha1))
+ return 0;
+
+ /* read_sha1_file NUL-terminates */
+ data = read_sha1_file(sha1, &t, &len);
+ if (t != OBJ_BLOB || !data || !len) {
+ free(data);
+ return t != OBJ_BLOB || !data;
+ }
+
+ strbuf_attach(&buf, data, len, len + 1);
+ lines = strbuf_split(&buf, '\n');
+
+ for (i = 0; lines[i]; i++) {
+ if (lines[i]->buf[lines[i]->len - 1] == '\n')
+ strbuf_setlen(lines[i], lines[i]->len - 1);
+ if (!lines[i]->len)
+ continue; /* skip empty lines */
+ list_index = string_list_find_insert_index(sort_uniq_list,
+ lines[i]->buf, 0);
+ if (list_index < 0)
+ continue; /* skip duplicate lines */
+ string_list_insert_at_index(sort_uniq_list, list_index,
+ lines[i]->buf);
+ }
+
+ strbuf_list_free(lines);
+ strbuf_release(&buf);
+ return 0;
+}
+
+static int string_list_join_lines_helper(struct string_list_item *item,
+ void *cb_data)
+{
+ struct strbuf *buf = cb_data;
+ strbuf_addstr(buf, item->string);
+ strbuf_addch(buf, '\n');
+ return 0;
+}
+
+int combine_notes_cat_sort_uniq(unsigned char *cur_sha1,
+ const unsigned char *new_sha1)
+{
+ struct string_list sort_uniq_list = { NULL, 0, 0, 1 };
+ struct strbuf buf = STRBUF_INIT;
+ int ret = 1;
+
+ /* read both note blob objects into unique_lines */
+ if (string_list_add_note_lines(&sort_uniq_list, cur_sha1))
+ goto out;
+ if (string_list_add_note_lines(&sort_uniq_list, new_sha1))
+ goto out;
+
+ /* create a new blob object from sort_uniq_list */
+ if (for_each_string_list(&sort_uniq_list,
+ string_list_join_lines_helper, &buf))
+ goto out;
+
+ ret = write_sha1_file(buf.buf, buf.len, blob_type, cur_sha1);
+
+out:
+ strbuf_release(&buf);
+ string_list_clear(&sort_uniq_list, 0);
+ return ret;
+}
+
static int string_list_add_one_ref(const char *path, const unsigned char *sha1,
int flag, void *cb)
{
diff --git a/notes.h b/notes.h
index b372575..93a7365 100644
--- a/notes.h
+++ b/notes.h
@@ -27,6 +27,7 @@ typedef int (*combine_notes_fn)(unsigned char *cur_sha1, const unsigned char *ne
int combine_notes_concatenate(unsigned char *cur_sha1, const unsigned char *new_sha1);
int combine_notes_overwrite(unsigned char *cur_sha1, const unsigned char *new_sha1);
int combine_notes_ignore(unsigned char *cur_sha1, const unsigned char *new_sha1);
+int combine_notes_cat_sort_uniq(unsigned char *cur_sha1, const unsigned char *new_sha1);
/*
* Notes tree object
diff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh
index 1a1fc7e..461fd84 100755
--- a/t/t3309-notes-merge-auto-resolve.sh
+++ b/t/t3309-notes-merge-auto-resolve.sh
@@ -499,4 +499,149 @@ test_expect_success 'merge z into y with "union" strategy => Non-conflicting 3-w
verify_notes y union
'
+test_expect_success 'reset to pre-merge state (y)' '
+ git update-ref refs/notes/y refs/notes/y^1 &&
+ # Verify pre-merge state
+ verify_notes y y
+'
+
+cat <<EOF | sort >expect_notes_union2
+d682107b8bf7a7aea1e537a8d5cb6a12b60135f1 $commit_sha15
+5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
+3a631fdb6f41b05b55d8f4baf20728ba8f6fccbc $commit_sha13
+a66055fa82f7a03fe0c02a6aba3287a85abf7c62 $commit_sha12
+7e3c53503a3db8dd996cb62e37c66e070b44b54d $commit_sha11
+b8d03e173f67f6505a76f6e00cf93440200dd9be $commit_sha10
+851e1638784a884c7dd26c5d41f3340f6387413a $commit_sha8
+357b6ca14c7afd59b7f8b8aaaa6b8b723771135b $commit_sha5
+e2bfd06a37dd2031684a59a6e2b033e212239c78 $commit_sha4
+5772f42408c0dd6f097a7ca2d24de0e78d1c46b1 $commit_sha3
+283b48219aee9a4105f6cab337e789065c82c2b9 $commit_sha2
+EOF
+
+cat >expect_log_union2 <<EOF
+$commit_sha15 15th
+z notes on 15th commit
+
+y notes on 15th commit
+
+$commit_sha14 14th
+y notes on 14th commit
+
+$commit_sha13 13th
+y notes on 13th commit
+
+$commit_sha12 12th
+y notes on 12th commit
+
+$commit_sha11 11th
+z notes on 11th commit
+
+$commit_sha10 10th
+x notes on 10th commit
+
+$commit_sha9 9th
+
+$commit_sha8 8th
+z notes on 8th commit
+
+$commit_sha7 7th
+
+$commit_sha6 6th
+
+$commit_sha5 5th
+z notes on 5th commit
+
+y notes on 5th commit
+
+$commit_sha4 4th
+y notes on 4th commit
+
+$commit_sha3 3rd
+y notes on 3rd commit
+
+$commit_sha2 2nd
+z notes on 2nd commit
+
+$commit_sha1 1st
+
+EOF
+
+test_expect_success 'merge y into z with "union" strategy => Non-conflicting 3-way merge' '
+ git config core.notesRef refs/notes/z &&
+ git notes merge --strategy=union y &&
+ verify_notes z union2
+'
+
+test_expect_success 'reset to pre-merge state (z)' '
+ git update-ref refs/notes/z refs/notes/z^1 &&
+ # Verify pre-merge state
+ verify_notes z z
+'
+
+cat <<EOF | sort >expect_notes_cat_sort_uniq
+6be90240b5f54594203e25d9f2f64b7567175aee $commit_sha15
+5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
+3a631fdb6f41b05b55d8f4baf20728ba8f6fccbc $commit_sha13
+a66055fa82f7a03fe0c02a6aba3287a85abf7c62 $commit_sha12
+7e3c53503a3db8dd996cb62e37c66e070b44b54d $commit_sha11
+b8d03e173f67f6505a76f6e00cf93440200dd9be $commit_sha10
+851e1638784a884c7dd26c5d41f3340f6387413a $commit_sha8
+660311d7f78dc53db12ac373a43fca7465381a7e $commit_sha5
+e2bfd06a37dd2031684a59a6e2b033e212239c78 $commit_sha4
+5772f42408c0dd6f097a7ca2d24de0e78d1c46b1 $commit_sha3
+283b48219aee9a4105f6cab337e789065c82c2b9 $commit_sha2
+EOF
+
+cat >expect_log_cat_sort_uniq <<EOF
+$commit_sha15 15th
+y notes on 15th commit
+z notes on 15th commit
+
+$commit_sha14 14th
+y notes on 14th commit
+
+$commit_sha13 13th
+y notes on 13th commit
+
+$commit_sha12 12th
+y notes on 12th commit
+
+$commit_sha11 11th
+z notes on 11th commit
+
+$commit_sha10 10th
+x notes on 10th commit
+
+$commit_sha9 9th
+
+$commit_sha8 8th
+z notes on 8th commit
+
+$commit_sha7 7th
+
+$commit_sha6 6th
+
+$commit_sha5 5th
+y notes on 5th commit
+z notes on 5th commit
+
+$commit_sha4 4th
+y notes on 4th commit
+
+$commit_sha3 3rd
+y notes on 3rd commit
+
+$commit_sha2 2nd
+z notes on 2nd commit
+
+$commit_sha1 1st
+
+EOF
+
+test_expect_success 'merge y into z with "cat_sort_uniq" strategy => Non-conflicting 3-way merge' '
+ git notes merge --strategy=cat_sort_uniq y &&
+ verify_notes z cat_sort_uniq
+'
+
test_done
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 20/21] git notes merge: Add testcases for merging notes trees at different fanouts
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (18 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 19/21] git notes merge: Add another auto-resolving strategy: "cat_sort_uniq" Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 2:08 ` [PATCHv4 21/21] Provide 'git notes get-ref' to easily retrieve current notes ref Johan Herland
2010-10-21 21:00 ` [PATCHv4 00/21] git notes merge Sverre Rabbelier
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
Notes trees may exist at different fanout levels internally. This
implementation detail should not be visible to the user, and it should
certainly not affect the merging of notes tree.
This patch adds testcases verifying the correctness of 'git notes merge'
when merging notes trees at different fanout levels.
Signed-off-by: Johan Herland <johan@herland.net>
---
t/t3311-notes-merge-fanout.sh | 436 +++++++++++++++++++++++++++++++++++++++++
1 files changed, 436 insertions(+), 0 deletions(-)
create mode 100755 t/t3311-notes-merge-fanout.sh
diff --git a/t/t3311-notes-merge-fanout.sh b/t/t3311-notes-merge-fanout.sh
new file mode 100755
index 0000000..d1c7b69
--- /dev/null
+++ b/t/t3311-notes-merge-fanout.sh
@@ -0,0 +1,436 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Johan Herland
+#
+
+test_description='Test notes merging at various fanout levels'
+
+. ./test-lib.sh
+
+verify_notes () {
+ notes_ref="$1"
+ commit="$2"
+ if test -f "expect_notes_$notes_ref"
+ then
+ git -c core.notesRef="refs/notes/$notes_ref" notes |
+ sort >"output_notes_$notes_ref" &&
+ test_cmp "expect_notes_$notes_ref" "output_notes_$notes_ref" ||
+ return 1
+ fi &&
+ git -c core.notesRef="refs/notes/$notes_ref" log --format="%H %s%n%N" \
+ "$commit" >"output_log_$notes_ref" &&
+ test_cmp "expect_log_$notes_ref" "output_log_$notes_ref"
+}
+
+verify_fanout () {
+ notes_ref="$1"
+ # Expect entire notes tree to have a fanout == 1
+ git rev-parse --quiet --verify "refs/notes/$notes_ref" >/dev/null &&
+ git ls-tree -r --name-only "refs/notes/$notes_ref" |
+ while read path
+ do
+ case "$path" in
+ ??/??????????????????????????????????????)
+ : true
+ ;;
+ *)
+ echo "Invalid path \"$path\"" &&
+ return 1
+ ;;
+ esac
+ done
+}
+
+verify_no_fanout () {
+ notes_ref="$1"
+ # Expect entire notes tree to have a fanout == 0
+ git rev-parse --quiet --verify "refs/notes/$notes_ref" >/dev/null &&
+ git ls-tree -r --name-only "refs/notes/$notes_ref" |
+ while read path
+ do
+ case "$path" in
+ ????????????????????????????????????????)
+ : true
+ ;;
+ *)
+ echo "Invalid path \"$path\"" &&
+ return 1
+ ;;
+ esac
+ done
+}
+
+# Set up a notes merge scenario with different kinds of conflicts
+test_expect_success 'setup a few initial commits with notes (notes ref: x)' '
+ git config core.notesRef refs/notes/x &&
+ for i in 1 2 3 4 5
+ do
+ test_commit "commit$i" >/dev/null &&
+ git notes add -m "notes for commit$i" || return 1
+ done
+'
+
+commit_sha1=$(git rev-parse commit1^{commit})
+commit_sha2=$(git rev-parse commit2^{commit})
+commit_sha3=$(git rev-parse commit3^{commit})
+commit_sha4=$(git rev-parse commit4^{commit})
+commit_sha5=$(git rev-parse commit5^{commit})
+
+cat <<EOF | sort >expect_notes_x
+aed91155c7a72c2188e781fdf40e0f3761b299db $commit_sha5
+99fab268f9d7ee7b011e091a436c78def8eeee69 $commit_sha4
+953c20ae26c7aa0b428c20693fe38bc687f9d1a9 $commit_sha3
+6358796131b8916eaa2dde6902642942a1cb37e1 $commit_sha2
+b02d459c32f0e68f2fe0981033bb34f38776ba47 $commit_sha1
+EOF
+
+cat >expect_log_x <<EOF
+$commit_sha5 commit5
+notes for commit5
+
+$commit_sha4 commit4
+notes for commit4
+
+$commit_sha3 commit3
+notes for commit3
+
+$commit_sha2 commit2
+notes for commit2
+
+$commit_sha1 commit1
+notes for commit1
+
+EOF
+
+test_expect_success 'sanity check (x)' '
+ verify_notes x commit5 &&
+ verify_no_fanout x
+'
+
+num=300
+
+cp expect_log_x expect_log_y
+
+test_expect_success 'Add a few hundred commits w/notes to trigger fanout (x -> y)' '
+ git update-ref refs/notes/y refs/notes/x &&
+ git config core.notesRef refs/notes/y &&
+ i=5 &&
+ while test $i -lt $num
+ do
+ i=$(($i + 1)) &&
+ test_commit "commit$i" >/dev/null &&
+ git notes add -m "notes for commit$i" || return 1
+ done &&
+ test "$(git rev-parse refs/notes/y)" != "$(git rev-parse refs/notes/x)" &&
+ # Expected number of commits and notes
+ test "$(git rev-list HEAD | wc -l)" = "$num" &&
+ test "$(git notes list | wc -l)" = "$num" &&
+ # 5 first notes unchanged
+ verify_notes y commit5
+'
+
+test_expect_success 'notes tree has fanout (y)' 'verify_fanout y'
+
+test_expect_success 'No-op merge (already included) (x => y)' '
+ git update-ref refs/notes/m refs/notes/y &&
+ git config core.notesRef refs/notes/m &&
+ git notes merge x &&
+ test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/y)"
+'
+
+test_expect_success 'Fast-forward merge (y => x)' '
+ git update-ref refs/notes/m refs/notes/x &&
+ git notes merge y &&
+ test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/y)"
+'
+
+cat <<EOF | sort >expect_notes_z
+9f506ee70e20379d7f78204c77b334f43d77410d $commit_sha3
+23a47d6ea7d589895faf800752054818e1e7627b $commit_sha2
+b02d459c32f0e68f2fe0981033bb34f38776ba47 $commit_sha1
+EOF
+
+cat >expect_log_z <<EOF
+$commit_sha5 commit5
+
+$commit_sha4 commit4
+
+$commit_sha3 commit3
+notes for commit3
+
+appended notes for commit3
+
+$commit_sha2 commit2
+new notes for commit2
+
+$commit_sha1 commit1
+notes for commit1
+
+EOF
+
+test_expect_success 'change some of the initial 5 notes (x -> z)' '
+ git update-ref refs/notes/z refs/notes/x &&
+ git config core.notesRef refs/notes/z &&
+ git notes add -f -m "new notes for commit2" commit2 &&
+ git notes append -m "appended notes for commit3" commit3 &&
+ git notes remove commit4 &&
+ git notes remove commit5 &&
+ verify_notes z commit5
+'
+
+test_expect_success 'notes tree has no fanout (z)' 'verify_no_fanout z'
+
+cp expect_log_z expect_log_m
+
+test_expect_success 'successful merge without conflicts (y => z)' '
+ git update-ref refs/notes/m refs/notes/z &&
+ git config core.notesRef refs/notes/m &&
+ git notes merge y &&
+ verify_notes m commit5 &&
+ # x/y/z unchanged
+ verify_notes x commit5 &&
+ verify_notes y commit5 &&
+ verify_notes z commit5
+'
+
+test_expect_success 'notes tree still has fanout after merge (m)' 'verify_fanout m'
+
+cat >expect_log_w <<EOF
+$commit_sha5 commit5
+
+$commit_sha4 commit4
+other notes for commit4
+
+$commit_sha3 commit3
+other notes for commit3
+
+$commit_sha2 commit2
+notes for commit2
+
+$commit_sha1 commit1
+other notes for commit1
+
+EOF
+
+test_expect_success 'introduce conflicting changes (y -> w)' '
+ git update-ref refs/notes/w refs/notes/y &&
+ git config core.notesRef refs/notes/w &&
+ git notes add -f -m "other notes for commit1" commit1 &&
+ git notes add -f -m "other notes for commit3" commit3 &&
+ git notes add -f -m "other notes for commit4" commit4 &&
+ git notes remove commit5 &&
+ verify_notes w commit5
+'
+
+cat >expect_log_m <<EOF
+$commit_sha5 commit5
+
+$commit_sha4 commit4
+other notes for commit4
+
+$commit_sha3 commit3
+other notes for commit3
+
+$commit_sha2 commit2
+new notes for commit2
+
+$commit_sha1 commit1
+other notes for commit1
+
+EOF
+
+test_expect_success 'successful merge using "ours" strategy (z => w)' '
+ git update-ref refs/notes/m refs/notes/w &&
+ git config core.notesRef refs/notes/m &&
+ git notes merge -s ours z &&
+ verify_notes m commit5 &&
+ # w/x/y/z unchanged
+ verify_notes w commit5 &&
+ verify_notes x commit5 &&
+ verify_notes y commit5 &&
+ verify_notes z commit5
+'
+
+test_expect_success 'notes tree still has fanout after merge (m)' 'verify_fanout m'
+
+cat >expect_log_m <<EOF
+$commit_sha5 commit5
+
+$commit_sha4 commit4
+
+$commit_sha3 commit3
+notes for commit3
+
+appended notes for commit3
+
+$commit_sha2 commit2
+new notes for commit2
+
+$commit_sha1 commit1
+other notes for commit1
+
+EOF
+
+test_expect_success 'successful merge using "theirs" strategy (z => w)' '
+ git update-ref refs/notes/m refs/notes/w &&
+ git notes merge -s theirs z &&
+ verify_notes m commit5 &&
+ # w/x/y/z unchanged
+ verify_notes w commit5 &&
+ verify_notes x commit5 &&
+ verify_notes y commit5 &&
+ verify_notes z commit5
+'
+
+test_expect_success 'notes tree still has fanout after merge (m)' 'verify_fanout m'
+
+cat >expect_log_m <<EOF
+$commit_sha5 commit5
+
+$commit_sha4 commit4
+other notes for commit4
+
+$commit_sha3 commit3
+other notes for commit3
+
+notes for commit3
+
+appended notes for commit3
+
+$commit_sha2 commit2
+new notes for commit2
+
+$commit_sha1 commit1
+other notes for commit1
+
+EOF
+
+test_expect_success 'successful merge using "union" strategy (z => w)' '
+ git update-ref refs/notes/m refs/notes/w &&
+ git notes merge -s union z &&
+ verify_notes m commit5 &&
+ # w/x/y/z unchanged
+ verify_notes w commit5 &&
+ verify_notes x commit5 &&
+ verify_notes y commit5 &&
+ verify_notes z commit5
+'
+
+test_expect_success 'notes tree still has fanout after merge (m)' 'verify_fanout m'
+
+cat >expect_log_m <<EOF
+$commit_sha5 commit5
+
+$commit_sha4 commit4
+other notes for commit4
+
+$commit_sha3 commit3
+appended notes for commit3
+notes for commit3
+other notes for commit3
+
+$commit_sha2 commit2
+new notes for commit2
+
+$commit_sha1 commit1
+other notes for commit1
+
+EOF
+
+test_expect_success 'successful merge using "cat_sort_uniq" strategy (z => w)' '
+ git update-ref refs/notes/m refs/notes/w &&
+ git notes merge -s cat_sort_uniq z &&
+ verify_notes m commit5 &&
+ # w/x/y/z unchanged
+ verify_notes w commit5 &&
+ verify_notes x commit5 &&
+ verify_notes y commit5 &&
+ verify_notes z commit5
+'
+
+test_expect_success 'notes tree still has fanout after merge (m)' 'verify_fanout m'
+
+# We're merging z into w. Here are the conflicts we expect:
+#
+# commit | x -> w | x -> z | conflict?
+# -------|-----------|-----------|----------
+# 1 | changed | unchanged | no, use w
+# 2 | unchanged | changed | no, use z
+# 3 | changed | changed | yes (w, then z in conflict markers)
+# 4 | changed | deleted | yes (w)
+# 5 | deleted | deleted | no, deleted
+
+test_expect_success 'fails to merge using "manual" strategy (z => w)' '
+ git update-ref refs/notes/m refs/notes/w &&
+ test_must_fail git notes merge z
+'
+
+test_expect_success 'notes tree still has fanout after merge (m)' 'verify_fanout m'
+
+cat <<EOF | sort >expect_conflicts
+$commit_sha3
+$commit_sha4
+EOF
+
+cat >expect_conflict_$commit_sha3 <<EOF
+<<<<<<< refs/notes/m
+other notes for commit3
+=======
+notes for commit3
+
+appended notes for commit3
+>>>>>>> refs/notes/z
+EOF
+
+cat >expect_conflict_$commit_sha4 <<EOF
+other notes for commit4
+EOF
+
+test_expect_success 'verify conflict entries (with no fanout)' '
+ ls .git/NOTES_MERGE_WORKTREE >output_conflicts &&
+ test_cmp expect_conflicts output_conflicts &&
+ ( for f in $(cat expect_conflicts); do
+ test_cmp "expect_conflict_$f" ".git/NOTES_MERGE_WORKTREE/$f" ||
+ exit 1
+ done ) &&
+ # Verify that current notes tree (pre-merge) has not changed (m == w)
+ test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
+'
+
+cat >expect_log_m <<EOF
+$commit_sha5 commit5
+
+$commit_sha4 commit4
+other notes for commit4
+
+$commit_sha3 commit3
+other notes for commit3
+
+appended notes for commit3
+
+$commit_sha2 commit2
+new notes for commit2
+
+$commit_sha1 commit1
+other notes for commit1
+
+EOF
+
+test_expect_success 'resolve and finalize merge (z => w)' '
+ cat >.git/NOTES_MERGE_WORKTREE/$commit_sha3 <<EOF &&
+other notes for commit3
+
+appended notes for commit3
+EOF
+ git notes merge --commit &&
+ verify_notes m commit5 &&
+ # w/x/y/z unchanged
+ verify_notes w commit5 &&
+ verify_notes x commit5 &&
+ verify_notes y commit5 &&
+ verify_notes z commit5
+'
+
+test_expect_success 'notes tree still has fanout after merge (m)' 'verify_fanout m'
+
+test_done
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCHv4 21/21] Provide 'git notes get-ref' to easily retrieve current notes ref
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (19 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 20/21] git notes merge: Add testcases for merging notes trees at different fanouts Johan Herland
@ 2010-10-21 2:08 ` Johan Herland
2010-10-21 21:00 ` [PATCHv4 00/21] git notes merge Sverre Rabbelier
21 siblings, 0 replies; 43+ messages in thread
From: Johan Herland @ 2010-10-21 2:08 UTC (permalink / raw)
To: git; +Cc: johan, jrnieder, bebarino, avarab, gitster
Script may use 'git notes get-ref' to easily retrieve the current notes ref.
Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
Documentation/git-notes.txt | 5 +++++
builtin/notes.c | 23 +++++++++++++++++++++++
t/t3301-notes.sh | 19 +++++++++++++++++++
3 files changed, 47 insertions(+), 0 deletions(-)
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 466885a..58dc378 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -19,6 +19,7 @@ SYNOPSIS
'git notes' merge --reset [-v | -q]
'git notes' remove [<object>]
'git notes' prune [-n | -v]
+'git notes' get-ref
DESCRIPTION
@@ -109,6 +110,10 @@ remove::
prune::
Remove all notes for non-existing/unreachable objects.
+get-ref::
+ Print the current notes ref. This provides an easy way to
+ retrieve the current notes ref (e.g. from scripts).
+
OPTIONS
-------
-f::
diff --git a/builtin/notes.c b/builtin/notes.c
index abf26e5..5d8c8e8 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -31,6 +31,7 @@ static const char * const git_notes_usage[] = {
"git notes merge --reset [-v | -q]",
"git notes [--ref <notes_ref>] remove [<object>]",
"git notes [--ref <notes_ref>] prune [-n | -v]",
+ "git notes [--ref <notes_ref>] get-ref",
NULL
};
@@ -82,6 +83,11 @@ static const char * const git_notes_prune_usage[] = {
NULL
};
+static const char * const git_notes_get_ref_usage[] = {
+ "git notes get-ref",
+ NULL
+};
+
static const char note_template[] =
"\n"
"#\n"
@@ -1002,6 +1008,21 @@ static int prune(int argc, const char **argv, const char *prefix)
return 0;
}
+static int get_ref(int argc, const char **argv, const char *prefix)
+{
+ struct option options[] = { OPT_END() };
+ argc = parse_options(argc, argv, prefix, options,
+ git_notes_get_ref_usage, 0);
+
+ if (argc) {
+ error("too many parameters");
+ usage_with_options(git_notes_get_ref_usage, options);
+ }
+
+ puts(default_notes_ref());
+ return 0;
+}
+
int cmd_notes(int argc, const char **argv, const char *prefix)
{
int result;
@@ -1040,6 +1061,8 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
result = remove_cmd(argc, argv, prefix);
else if (!strcmp(argv[0], "prune"))
result = prune(argc, argv, prefix);
+ else if (!strcmp(argv[0], "get-ref"))
+ result = get_ref(argc, argv, prefix);
else {
result = error("Unknown subcommand: %s", argv[0]);
usage_with_options(git_notes_usage, options);
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 4bf4e52..f5b72c7 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1058,4 +1058,23 @@ test_expect_success 'git notes copy diagnoses too many or too few parameters' '
test_must_fail git notes copy one two three
'
+test_expect_success 'git notes get-ref (no overrides)' '
+ git config --unset core.notesRef &&
+ unset GIT_NOTES_REF &&
+ test "$(git notes get-ref)" = "refs/notes/commits"
+'
+
+test_expect_success 'git notes get-ref (core.notesRef)' '
+ git config core.notesRef refs/notes/foo &&
+ test "$(git notes get-ref)" = "refs/notes/foo"
+'
+
+test_expect_success 'git notes get-ref (GIT_NOTES_REF)' '
+ test "$(GIT_NOTES_REF=refs/notes/bar git notes get-ref)" = "refs/notes/bar"
+'
+
+test_expect_success 'git notes get-ref (--ref)' '
+ test "$(GIT_NOTES_REF=refs/notes/bar git notes --ref=baz get-ref)" = "refs/notes/baz"
+'
+
test_done
--
1.7.3.98.g5ad7d9
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCHv4 00/21] git notes merge
2010-10-21 2:08 [PATCHv4 00/21] git notes merge Johan Herland
` (20 preceding siblings ...)
2010-10-21 2:08 ` [PATCHv4 21/21] Provide 'git notes get-ref' to easily retrieve current notes ref Johan Herland
@ 2010-10-21 21:00 ` Sverre Rabbelier
2010-10-21 23:20 ` Junio C Hamano
` (2 more replies)
21 siblings, 3 replies; 43+ messages in thread
From: Sverre Rabbelier @ 2010-10-21 21:00 UTC (permalink / raw)
To: Johan Herland; +Cc: git, jrnieder, bebarino, avarab, gitster
Heya,
On Wed, Oct 20, 2010 at 21:08, Johan Herland <johan@herland.net> wrote:
> - Sverre Rabbelier suggests renaming 'git notes merge --reset' to
> 'git notes merge --abort'. I sort of agree, but would want some
> consistency with 'git merge', e.g. by providing 'git merge --abort'
> as an alias to 'git reset --merge'.
Can someone comment on whether "git reset --merge" is a good
implementation of "git merge --abort"? If we can get an Ack from
someone I'd love to see this implemented.
> - When resolving notes merge conflicts, you can add/remove files/notes
> in .git/NOTES_MERGE_WORKTREE; 'git notes merge --commit' does not
> check that the notes have any relationship to the notes originally
> put there by 'git notes merge'. Should we warn about removed and
> added notes in .git/NOTES_MERGE_WORKTREE? Currently we don't, and
> I'm not sure it's worth it. Users can always review the merge commit
> afterwards.
If it's easy to do, it would be useful.
> - Fetching and pushing note refs:
> - Add refs/notes/* to default fetch refspec?
> - A way to specify (at clone time) which refspec(s) to set up?
> - A way for the remote repo to hint at which refspecs you might want
> to set up (by default?)
Didn't we already discuss this earlier? Can you summarize (or at least
link to) that discussion?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCHv4 00/21] git notes merge
2010-10-21 21:00 ` [PATCHv4 00/21] git notes merge Sverre Rabbelier
@ 2010-10-21 23:20 ` Junio C Hamano
2010-10-21 23:30 ` Jonathan Nieder
2010-10-22 14:11 ` git merge --abort? [was: Re: [PATCHv4 00/21] git notes merge] Johan Herland
2010-10-22 15:41 ` [PATCHv4 00/21] git notes merge Johan Herland
2010-10-22 22:28 ` Johan Herland
2 siblings, 2 replies; 43+ messages in thread
From: Junio C Hamano @ 2010-10-21 23:20 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Johan Herland, git, jrnieder, bebarino, avarab, gitster
Sverre Rabbelier <srabbelier@gmail.com> writes:
> Can someone comment on whether "git reset --merge" is a good
> implementation of "git merge --abort"? If we can get an Ack from
> someone I'd love to see this implemented.
I think it depends on in what state you started from and how the merge
operation stopped. If your index was clean, probably yes, but "git reset
--merge" may not be something you may want to do if the merge stopped
because you had previously added something.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCHv4 00/21] git notes merge
2010-10-21 23:20 ` Junio C Hamano
@ 2010-10-21 23:30 ` Jonathan Nieder
2010-10-22 14:11 ` git merge --abort? [was: Re: [PATCHv4 00/21] git notes merge] Johan Herland
1 sibling, 0 replies; 43+ messages in thread
From: Jonathan Nieder @ 2010-10-21 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sverre Rabbelier, Johan Herland, git, bebarino, avarab
Junio C Hamano wrote:
> Sverre Rabbelier <srabbelier@gmail.com> writes:
>> Can someone comment on whether "git reset --merge" is a good
>> implementation of "git merge --abort"?
[...]
> I think it depends on in what state you started from and how the merge
> operation stopped. If your index was clean, probably yes, but "git reset
> --merge" may not be something you may want to do if the merge stopped
> because you had previously added something.
I suppose it would be safe if one checks that MERGE_HEAD is present
first?
It would apply to failed cherry-picks, too. Not sure if that's a
good or a bad thing.
^ permalink raw reply [flat|nested] 43+ messages in thread
* git merge --abort? [was: Re: [PATCHv4 00/21] git notes merge]
2010-10-21 23:20 ` Junio C Hamano
2010-10-21 23:30 ` Jonathan Nieder
@ 2010-10-22 14:11 ` Johan Herland
2010-10-22 14:55 ` Jonathan Nieder
1 sibling, 1 reply; 43+ messages in thread
From: Johan Herland @ 2010-10-22 14:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Sverre Rabbelier, jrnieder, bebarino, avarab
On Friday 22 October 2010, Junio C Hamano wrote:
> Sverre Rabbelier <srabbelier@gmail.com> writes:
> > Can someone comment on whether "git reset --merge" is a good
> > implementation of "git merge --abort"? If we can get an Ack from
> > someone I'd love to see this implemented.
>
> I think it depends on in what state you started from and how the
> merge operation stopped. If your index was clean, probably yes, but
> "git reset --merge" may not be something you may want to do if the
> merge stopped because you had previously added something.
Does this mean that there are situations where you simply _cannot_ get
back to the pre-merge state (using 'git reset --merge' or otherwise)?
Is this something we should detect and warn about when starting the
merge? Something like:
$ git merge bar
I'm sorry, Dave. I'm afraid I can't merge with and unclean index.
Use -f to force the merge anyway, but then 'git merge --abort' will
lose your staged changes.
Or could we solve it simply by making a backup of the pre-merge index
that can later be restored by 'git merge --abort'?
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: git merge --abort? [was: Re: [PATCHv4 00/21] git notes merge]
2010-10-22 14:11 ` git merge --abort? [was: Re: [PATCHv4 00/21] git notes merge] Johan Herland
@ 2010-10-22 14:55 ` Jonathan Nieder
2010-10-22 15:12 ` Johan Herland
0 siblings, 1 reply; 43+ messages in thread
From: Jonathan Nieder @ 2010-10-22 14:55 UTC (permalink / raw)
To: Johan Herland; +Cc: git, Junio C Hamano, Sverre Rabbelier, bebarino, avarab
Johan Herland wrote:
> Does this mean that there are situations where you simply _cannot_ get
> back to the pre-merge state (using 'git reset --merge' or otherwise)?
Technically yes, there is such an edge case, but I don't think that
was what he was talking about.
> Is this something we should detect and warn about when starting the
> merge? Something like:
>
> $ git merge bar
> I'm sorry, Dave. I'm afraid I can't merge with and unclean index.
> Use -f to force the merge anyway, but then 'git merge --abort' will
> lose your staged changes.
"Use -f to force the merge anyway" does not make sense to me.
git merge does not work with an index that does not match HEAD (except
in the aforementioned edge case where the content in the edge already
matches the merged content). So 'git merge' bails out in this case,
leaving the index as-is; if a person doesn't notice that and tries
'git reset --merge', her staged changes may be clobbered.
> Or could we solve it simply by making a backup of the pre-merge index
> that can later be restored by 'git merge --abort'?
Yes, that's one way. I think it might be better for 'git reset
--merge' to check for MERGE_HEAD and do nothing if it is absent if we
want it to be closer to an inverse to failed 'git merge'.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: git merge --abort? [was: Re: [PATCHv4 00/21] git notes merge]
2010-10-22 14:55 ` Jonathan Nieder
@ 2010-10-22 15:12 ` Johan Herland
2010-10-22 15:20 ` Sverre Rabbelier
0 siblings, 1 reply; 43+ messages in thread
From: Johan Herland @ 2010-10-22 15:12 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Junio C Hamano, Sverre Rabbelier, bebarino, avarab
On Friday 22 October 2010, Jonathan Nieder wrote:
> Johan Herland wrote:
> > Does this mean that there are situations where you simply _cannot_
> > get back to the pre-merge state (using 'git reset --merge' or
> > otherwise)?
>
> Technically yes, there is such an edge case, but I don't think that
> was what he was talking about.
Ah, sorry for the misunderstanding.
> > Is this something we should detect and warn about when starting the
> > merge? Something like:
> >
> > $ git merge bar
> > I'm sorry, Dave. I'm afraid I can't merge with and unclean index.
> > Use -f to force the merge anyway, but then 'git merge --abort'
> > will lose your staged changes.
>
> "Use -f to force the merge anyway" does not make sense to me.
> git merge does not work with an index that does not match HEAD
> (except in the aforementioned edge case where the content in the edge
> already matches the merged content). So 'git merge' bails out in
> this case, leaving the index as-is; if a person doesn't notice that
> and tries 'git reset --merge', her staged changes may be clobbered.
>
> > Or could we solve it simply by making a backup of the pre-merge
> > index that can later be restored by 'git merge --abort'?
>
> Yes, that's one way. I think it might be better for 'git reset
> --merge' to check for MERGE_HEAD and do nothing if it is absent if we
> want it to be closer to an inverse to failed 'git merge'.
Yes, that makes sense to me, especially if we make a 'git merge --abort'
synonym. Alternatively, we can make 'git merge --abort' check for
MERGE_HEAD, and then defer to 'git reset --merge', while leaving 'git
reset --merge' as-is.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: git merge --abort? [was: Re: [PATCHv4 00/21] git notes merge]
2010-10-22 15:12 ` Johan Herland
@ 2010-10-22 15:20 ` Sverre Rabbelier
2010-10-22 15:48 ` Jonathan Nieder
0 siblings, 1 reply; 43+ messages in thread
From: Sverre Rabbelier @ 2010-10-22 15:20 UTC (permalink / raw)
To: Johan Herland; +Cc: Jonathan Nieder, git, Junio C Hamano, bebarino, avarab
Heya,
On Fri, Oct 22, 2010 at 10:12, Johan Herland <johan@herland.net> wrote:
> Yes, that makes sense to me, especially if we make a 'git merge --abort'
> synonym. Alternatively, we can make 'git merge --abort' check for
> MERGE_HEAD, and then defer to 'git reset --merge', while leaving 'git
> reset --merge' as-is.
ACK to that. I think people using 'git reset --merge' know what
they're doing, and should be able to clobber their changes (viz. `git
reset --hard`), while 'git merge --abort' should do more handholding
and make sure you don't lose any data. (If I had more time I'd
implement that 'core.nodataloss' flag...)
--
Cheers,
Sverre Rabbelier
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: git merge --abort? [was: Re: [PATCHv4 00/21] git notes merge]
2010-10-22 15:20 ` Sverre Rabbelier
@ 2010-10-22 15:48 ` Jonathan Nieder
2010-10-22 15:57 ` Sverre Rabbelier
0 siblings, 1 reply; 43+ messages in thread
From: Jonathan Nieder @ 2010-10-22 15:48 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Johan Herland, git, Junio C Hamano, bebarino, avarab
Sverre Rabbelier wrote:
> On Fri, Oct 22, 2010 at 10:12, Johan Herland <johan@herland.net> wrote:
>> Yes, that makes sense to me, especially if we make a 'git merge --abort'
>> synonym. Alternatively, we can make 'git merge --abort' check for
>> MERGE_HEAD, and then defer to 'git reset --merge', while leaving 'git
>> reset --merge' as-is.
>
> ACK to that. I think people using 'git reset --merge' know what
> they're doing, and should be able to clobber their changes (viz. `git
> reset --hard`),
Wait a second: isn't that exactly what 'git reset --hard' is for?
I doubt[*] 'git reset --merge' could be anything but a mistake when used
outside the context of a merge (remember that the plumbing is called
"read-tree", not "reset").
[*] This is disingenuous of me. I used to use "git reset --merge <commit>"
with the intent of "git reset --keep <commit>", since the latter did
not exist yet. But I was wrong. :)
Here's a start for the interested:
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
diff --git a/builtin/reset.c b/builtin/reset.c
index 0037be4..a4fc7b3 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -335,6 +335,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
die("%s reset is not allowed in a bare repository",
reset_type_names[reset_type]);
+ if (reset_type == MERGE && !file_exists(git_path("MERGE_HEAD")))
+ die("You are not in the middle of a merge (MERGE_HEAD does not exist).");
+
/* Soft reset does not touch the index file nor the working tree
* at all, but requires them in a good order. Other resets reset
* the index file to the tree object we are switching to. */
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: git merge --abort? [was: Re: [PATCHv4 00/21] git notes merge]
2010-10-22 15:48 ` Jonathan Nieder
@ 2010-10-22 15:57 ` Sverre Rabbelier
0 siblings, 0 replies; 43+ messages in thread
From: Sverre Rabbelier @ 2010-10-22 15:57 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Johan Herland, git, Junio C Hamano, bebarino, avarab
Heya,
On Fri, Oct 22, 2010 at 10:48, Jonathan Nieder <jrnieder@gmail.com> wrote:
> I doubt[*] 'git reset --merge' could be anything but a mistake when used
> outside the context of a merge (remember that the plumbing is called
> "read-tree", not "reset").
In that case, we should perhaps deprecate 'git reset --merge' and
encourage users to use 'git merge --abort' instead. (I was thinking
first that it doesn't make sense conceptually to have a command under
'git reset' that only works for merges, but we have the same with 'git
checkout' already in the form of 'git checkout --ours', so perhaps
it's ok to keep 'git reset --merge' then.)
--
Cheers,
Sverre Rabbelier
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCHv4 00/21] git notes merge
2010-10-21 21:00 ` [PATCHv4 00/21] git notes merge Sverre Rabbelier
2010-10-21 23:20 ` Junio C Hamano
@ 2010-10-22 15:41 ` Johan Herland
2010-10-22 15:54 ` Sverre Rabbelier
2010-10-22 22:28 ` Johan Herland
2 siblings, 1 reply; 43+ messages in thread
From: Johan Herland @ 2010-10-22 15:41 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: git, jrnieder, bebarino, avarab, gitster
On Thursday 21 October 2010, Sverre Rabbelier wrote:
> On Wed, Oct 20, 2010 at 21:08, Johan Herland wrote:
> > - Fetching and pushing note refs:
> > - Add refs/notes/* to default fetch refspec?
> > - A way to specify (at clone time) which refspec(s) to set up?
> > - A way for the remote repo to hint at which refspecs you might
> > want to set up (by default?)
>
> Didn't we already discuss this earlier? Can you summarize (or at
> least link to) that discussion?
Yes, sorry for not answering you earlier. Here's what you wrote in the
previous thread:
On Saturday 09 October 2010, Sverre Rabbelier wrote:
> Heya,
>
> On Sat, Oct 9, 2010 at 03:08, Johan Herland <johan@herland.net> wrote:
> > - Fetching and pushing note refs:
> > - Add refs/notes/* to default fetch refspec?
>
> Or at least add a '--notes[=<notes namespace>]' to fetch, pull, and
> push.
Agreed, at least that.
In order to promote sharing of notes, though, I'd like for it to be
possible to configure the repo so that a vanilla 'git fetch' also
updates your notes. In fact, I wonder if this should even be made the
default.
> > - A way to specify (at clone time) which refspec(s) to set up?
>
> How would that look like?
Maybe add an option to 'git clone' (and 'git remote add') that specifies
the refspec you want to use in your config for that remote. Something
like:
git clone --fetch="+refs/heads/*:refs/remotes/origin/*" \
--fetch="+refs/notes/*:refs/remotes/origin/notes/*" \
<source_url> ...
...will set up the following config:
[remote "origin"]
url = <source_url>
fetch = +refs/heads/*:refs/remotes/origin/*
fetch = +refs/notes/*:refs/remotes/origin/notes/*
Obviously, we would probably want to provide shorthands for the most
common refspecs, like:
git clone --fetch=default,notes <source_url> ...
or
git clone --fetch-heads --fetch-notes <source_url> ...
> > - A way for the remote repo to hint at which refspecs you might
> > want to set up (by default?)
>
> I assume this would be a generic mechanism of sorts? Are there any
> other use cases for this other than notes?
Yes, I believe so (although I haven't thought much about this, yet).
There's been earlier discussions on hiding certain branches from view.
This could maybe be solved by the server suggesting a refspec that
excludes the stuff you don't want to share (by default). Similary, the
refspec could _include_ notes namespaces that you do want to share.
Of course (as today) the client should be free to demand a different
refspec, e.g. if it wants access to everything, or if it's only
interested in a subset of the "default" refs.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCHv4 00/21] git notes merge
2010-10-22 15:41 ` [PATCHv4 00/21] git notes merge Johan Herland
@ 2010-10-22 15:54 ` Sverre Rabbelier
2010-10-23 0:47 ` Johan Herland
0 siblings, 1 reply; 43+ messages in thread
From: Sverre Rabbelier @ 2010-10-22 15:54 UTC (permalink / raw)
To: Johan Herland; +Cc: git, jrnieder, bebarino, avarab, gitster
Heya,
On Fri, Oct 22, 2010 at 10:41, Johan Herland <johan@herland.net> wrote:
> Yes, sorry for not answering you earlier. Here's what you wrote in the
> previous thread:
No problem :).
> On Saturday 09 October 2010, Sverre Rabbelier wrote:
>> On Sat, Oct 9, 2010 at 03:08, Johan Herland <johan@herland.net> wrote:
>> > - Fetching and pushing note refs:
>> > - Add refs/notes/* to default fetch refspec?
>>
>> Or at least add a '--notes[=<notes namespace>]' to fetch, pull, and
>> push.
>
> Agreed, at least that.
>
> In order to promote sharing of notes, though, I'd like for it to be
> possible to configure the repo so that a vanilla 'git fetch' also
> updates your notes. In fact, I wonder if this should even be made the
> default.
I think notes directly under /refs/notes/ should be shared by default,
but those in sub-refs (such as the /refs/notes/am/ that's been
mentioned before) should not.
>> > - A way to specify (at clone time) which refspec(s) to set up?
>>
>> How would that look like?
>
> Maybe add an option to 'git clone' (and 'git remote add') that specifies
> the refspec you want to use in your config for that remote. Something
> like:
>
> git clone --fetch="+refs/heads/*:refs/remotes/origin/*" \
> --fetch="+refs/notes/*:refs/remotes/origin/notes/*" \
> <source_url> ...
Who's going to type that out though? The only use case I can think of
is if you want to be able to give someone a line they can paste and be
set right away, but I don't see why in that case pasting multiple
commands (i.e., calling 'git config' a few times) wouldn't suffice.
> Obviously, we would probably want to provide shorthands for the most
> common refspecs, like:
>
> git clone --fetch=default,notes <source_url> ...
> git clone --fetch-heads --fetch-notes <source_url> ...
Adding refspec shorthands _does_ make sense. However, it might make
more sense to put those under 'git remote' instead?
>> > - A way for the remote repo to hint at which refspecs you might
>> > want to set up (by default?)
>>
>> I assume this would be a generic mechanism of sorts? Are there any
>> other use cases for this other than notes?
>
> Yes, I believe so (although I haven't thought much about this, yet).
> There's been earlier discussions on hiding certain branches from view.
> This could maybe be solved by the server suggesting a refspec that
> excludes the stuff you don't want to share (by default). Similary, the
> refspec could _include_ notes namespaces that you do want to share.
That sounds like a good use case.
> Of course (as today) the client should be free to demand a different
> refspec, e.g. if it wants access to everything, or if it's only
> interested in a subset of the "default" refs.
Of course, I reckon it would just set up their refspecs, and the user
would be free to change it. Especially if we inform the user that the
refspec was set to something other than the default refspec.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCHv4 00/21] git notes merge
2010-10-22 15:54 ` Sverre Rabbelier
@ 2010-10-23 0:47 ` Johan Herland
2010-10-23 1:44 ` Sverre Rabbelier
0 siblings, 1 reply; 43+ messages in thread
From: Johan Herland @ 2010-10-23 0:47 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: git, jrnieder, bebarino, avarab, gitster
On Friday 22 October 2010, Sverre Rabbelier wrote:
> On Fri, Oct 22, 2010 at 10:41, Johan Herland wrote:
> > On Saturday 09 October 2010, Sverre Rabbelier wrote:
> >> On Sat, Oct 9, 2010 at 03:08, Johan Herland wrote:
> >> > - Fetching and pushing note refs:
> >> > - Add refs/notes/* to default fetch refspec?
> >>
> >> Or at least add a '--notes[=<notes namespace>]' to fetch, pull, and
> >> push.
> >
> > Agreed, at least that.
> >
> > In order to promote sharing of notes, though, I'd like for it to be
> > possible to configure the repo so that a vanilla 'git fetch' also
> > updates your notes. In fact, I wonder if this should even be made the
> > default.
>
> I think notes directly under /refs/notes/ should be shared by default,
> but those in sub-refs (such as the /refs/notes/am/ that's been
> mentioned before) should not.
In theory that would make sense, but how you write that as a refspec? AFAIK,
refspecs don't (yet) support a syntax for (a) _excluding_ something from a
glob, and certainly not (b) excluding all "sub-refs" from a glob.
Alternatively, we could share everything under refs/notes/, except
/refs/notes/local/ (or refs/notes/private/ or whatever). That would just
require support for (a)...
> >> > - A way to specify (at clone time) which refspec(s) to set up?
> >>
> >> How would that look like?
> >
> > Maybe add an option to 'git clone' (and 'git remote add') that
> > specifies the refspec you want to use in your config for that remote.
> > Something like:
> >
> > git clone --fetch="+refs/heads/*:refs/remotes/origin/*" \
> > --fetch="+refs/notes/*:refs/remotes/origin/notes/*" \
> > <source_url> ...
>
> Who's going to type that out though? The only use case I can think of
> is if you want to be able to give someone a line they can paste and be
> set right away, but I don't see why in that case pasting multiple
> commands (i.e., calling 'git config' a few times) wouldn't suffice.
You might be right. I was thinking that before we provide shorthands, we
should provide an option that let you specify _any_ refspec. Sort of as a
fallback if somebody's scenario doesn't fit into any of the shorthands.
But as you say, this can also be done with a few calls to git config.
> > Obviously, we would probably want to provide shorthands for the most
> > common refspecs, like:
> >
> > git clone --fetch=default,notes <source_url> ...
> > git clone --fetch-heads --fetch-notes <source_url> ...
>
> Adding refspec shorthands _does_ make sense. However, it might make
> more sense to put those under 'git remote' instead?
I'd say _both_ 'git remote' and 'git clone'.
> >> > - A way for the remote repo to hint at which refspecs you might
> >> > want to set up (by default?)
> >>
> >> I assume this would be a generic mechanism of sorts? Are there any
> >> other use cases for this other than notes?
> >
> > Yes, I believe so (although I haven't thought much about this, yet).
> > There's been earlier discussions on hiding certain branches from view.
> > This could maybe be solved by the server suggesting a refspec that
> > excludes the stuff you don't want to share (by default). Similary, the
> > refspec could _include_ notes namespaces that you do want to share.
>
> That sounds like a good use case.
But I believe we need to extend the refspec syntax to support at least (a)
from above (excluding named entries from a refspec glob) in order to support
hiding of certain refs. Still, I think it would be a useful addition.
> > Of course (as today) the client should be free to demand a different
> > refspec, e.g. if it wants access to everything, or if it's only
> > interested in a subset of the "default" refs.
>
> Of course, I reckon it would just set up their refspecs, and the user
> would be free to change it. Especially if we inform the user that the
> refspec was set to something other than the default refspec.
Agreed.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCHv4 00/21] git notes merge
2010-10-23 0:47 ` Johan Herland
@ 2010-10-23 1:44 ` Sverre Rabbelier
0 siblings, 0 replies; 43+ messages in thread
From: Sverre Rabbelier @ 2010-10-23 1:44 UTC (permalink / raw)
To: Johan Herland; +Cc: git, jrnieder, bebarino, avarab, gitster
Heya,
On Fri, Oct 22, 2010 at 17:47, Johan Herland <johan@herland.net> wrote:
> On Friday 22 October 2010, Sverre Rabbelier wrote:
>> I think notes directly under /refs/notes/ should be shared by default,
>> but those in sub-refs (such as the /refs/notes/am/ that's been
>> mentioned before) should not.
>
> In theory that would make sense, but how you write that as a refspec? AFAIK,
> refspecs don't (yet) support a syntax for (a) _excluding_ something from a
> glob, and certainly not (b) excluding all "sub-refs" from a glob.
Indeed, which I think is a shame. For that matter, I'd love to be able
to have negative globs for branches in general (e.g., 'git log
--branches :-heads/hidden/**).
> Alternatively, we could share everything under refs/notes/, except
> /refs/notes/local/ (or refs/notes/private/ or whatever). That would just
> require support for (a)...
That goes against how notes are used though. The refs/notes/* space is
the one that is shown by default, not refs/notes/local/*.
>> Who's going to type that out though? The only use case I can think of
>> is if you want to be able to give someone a line they can paste and be
>> set right away, but I don't see why in that case pasting multiple
>> commands (i.e., calling 'git config' a few times) wouldn't suffice.
>
> You might be right. I was thinking that before we provide shorthands, we
> should provide an option that let you specify _any_ refspec. Sort of as a
> fallback if somebody's scenario doesn't fit into any of the shorthands.
>
> But as you say, this can also be done with a few calls to git config.
[...]
>> Adding refspec shorthands _does_ make sense. However, it might make
>> more sense to put those under 'git remote' instead?
>
> I'd say _both_ 'git remote' and 'git clone'.
I think getting those shorthands in git config/git remote is step one,
if we do want that. Then, if desired, we can add that to git clone.
>> That sounds like a good use case.
>
> But I believe we need to extend the refspec syntax to support at least (a)
> from above (excluding named entries from a refspec glob) in order to support
> hiding of certain refs. Still, I think it would be a useful addition.
Agreed.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCHv4 00/21] git notes merge
2010-10-21 21:00 ` [PATCHv4 00/21] git notes merge Sverre Rabbelier
2010-10-21 23:20 ` Junio C Hamano
2010-10-22 15:41 ` [PATCHv4 00/21] git notes merge Johan Herland
@ 2010-10-22 22:28 ` Johan Herland
2010-10-23 1:38 ` Sverre Rabbelier
2 siblings, 1 reply; 43+ messages in thread
From: Johan Herland @ 2010-10-22 22:28 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: git, jrnieder, bebarino, avarab, gitster
(fragmenting this thread for the last time, I promise...)
On Thursday 21 October 2010, Sverre Rabbelier wrote:
> On Wed, Oct 20, 2010 at 21:08, Johan Herland wrote:
> > - When resolving notes merge conflicts, you can add/remove files/notes
> > in .git/NOTES_MERGE_WORKTREE; 'git notes merge --commit' does not
> > check that the notes have any relationship to the notes originally
> > put there by 'git notes merge'. Should we warn about removed and
> > added notes in .git/NOTES_MERGE_WORKTREE? Currently we don't, and
> > I'm not sure it's worth it. Users can always review the merge commit
> > afterwards.
>
> If it's easy to do, it would be useful.
Well, currently I don't store a separate list of conflicts anywhere, so I
would need to add that in order to have 'git notes merge --commit' warn
about deletions/additions in .git/NOTES_MERGE_WORKTREE.
Also, for conflicts where a note is modified in one branch and deleted in
the other, I don't think it makes sense to warn about their deletion (since
that in many cases is a perfectly valid resolution of those conflicts).
Which leaves warning about "other" additions/deletions. We could certainly
do this, but I'd frankly rather get the current patch series through without
broadening its scope. I've been sitting on this far too long as it is. I'll
keep it on my TODO list for a future series (if nobody else beats me to it).
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCHv4 00/21] git notes merge
2010-10-22 22:28 ` Johan Herland
@ 2010-10-23 1:38 ` Sverre Rabbelier
0 siblings, 0 replies; 43+ messages in thread
From: Sverre Rabbelier @ 2010-10-23 1:38 UTC (permalink / raw)
To: Johan Herland; +Cc: git, jrnieder, bebarino, avarab, gitster
Heya,
On Fri, Oct 22, 2010 at 15:28, Johan Herland <johan@herland.net> wrote:
> Which leaves warning about "other" additions/deletions. We could certainly
> do this, but I'd frankly rather get the current patch series through without
> broadening its scope. I've been sitting on this far too long as it is. I'll
> keep it on my TODO list for a future series (if nobody else beats me to it).
Fair enough, I don't think anyone will be seriously hurt by not having
this (especially since git notes is still more or less a 'beta
feature'), so I'm in favor of getting this merged now, and polishing
it more later if we decide it needs more polishing.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply [flat|nested] 43+ messages in thread