* [PATCH v3 0/4] notes: add support for notes.merge option
@ 2015-08-02 10:10 Jacob Keller
2015-08-02 10:10 ` [PATCH v3 1/4] notes: document cat_sort_uniq rewriteMode Jacob Keller
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Jacob Keller @ 2015-08-02 10:10 UTC (permalink / raw)
To: git; +Cc: Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
This series incorporates the feedback from both Johan and Eric. In
addition, I included an RFC implementing suggestion from Johan regarding
per-ref merge strategies.
I split the tests for --merge/--commit/--strategy out into their own
patch to help review and keep the "one commit = one change" logic.
I am not certain the RFC is ready yet, but the first 3 patches (now
based on next) should be ready to go.
Jacob Keller (4):
notes: document cat_sort_uniq rewriteMode
notes: add tests for --commit/--abort/--strategy exclusivity
notes: add notes.merge option to select default strategy
notes: add per-ref configuration of merge strategy
Documentation/config.txt | 18 +++-
Documentation/git-notes.txt | 23 ++++-
builtin/notes.c | 167 ++++++++++++++++++++++++++++++----
notes-merge.h | 16 ++--
t/t3309-notes-merge-auto-resolve.sh | 79 ++++++++++++++++
t/t3310-notes-merge-manual-resolve.sh | 12 +++
6 files changed, 288 insertions(+), 27 deletions(-)
--
2.5.0.482.gfcd5645
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/4] notes: document cat_sort_uniq rewriteMode
2015-08-02 10:10 [PATCH v3 0/4] notes: add support for notes.merge option Jacob Keller
@ 2015-08-02 10:10 ` Jacob Keller
2015-08-02 10:10 ` [PATCH v3 2/4] notes: add tests for --commit/--abort/--strategy exclusivity Jacob Keller
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Jacob Keller @ 2015-08-02 10:10 UTC (permalink / raw)
To: git; +Cc: Jacob Keller, Johan Herland, Michael Haggerty, Eric Sunshine
From: Jacob Keller <jacob.keller@gmail.com>
Teach documentation about the cat_sort_uniq rewriteMode that got added
at the same time as the equivalent merge strategy.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
Cc: Johan Herland <johan@herland.net>
Cc: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Eric Sunshine <sunshine@sunshineco.com>
---
Documentation/config.txt | 4 ++--
Documentation/git-notes.txt | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3f1c17814ceb..4ce5c553e50f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1939,8 +1939,8 @@ notes.rewriteMode::
When copying notes during a rewrite (see the
"notes.rewrite.<command>" option), determines what to do if
the target commit already has a note. Must be one of
- `overwrite`, `concatenate`, or `ignore`. Defaults to
- `concatenate`.
+ `overwrite`, `concatenate`, `cat_sort_uniq`, or `ignore`.
+ Defaults to `concatenate`.
+
This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`
environment variable.
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 851518d531b5..674682b34b83 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -331,7 +331,8 @@ environment variable.
notes.rewriteMode::
When copying notes during a rewrite, what to do if the target
commit already has a note. Must be one of `overwrite`,
- `concatenate`, and `ignore`. Defaults to `concatenate`.
+ `concatenate`, `cat_sort_uniq`, or `ignore`. Defaults to
+ `concatenate`.
+
This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`
environment variable.
--
2.5.0.482.gfcd5645
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 2/4] notes: add tests for --commit/--abort/--strategy exclusivity
2015-08-02 10:10 [PATCH v3 0/4] notes: add support for notes.merge option Jacob Keller
2015-08-02 10:10 ` [PATCH v3 1/4] notes: document cat_sort_uniq rewriteMode Jacob Keller
@ 2015-08-02 10:10 ` Jacob Keller
2015-08-02 10:10 ` [PATCH v3 3/4] notes: add notes.merge option to select default strategy Jacob Keller
2015-08-02 10:10 ` [PATCH RFC 4/4] notes: add per-ref configuration of merge strategy Jacob Keller
3 siblings, 0 replies; 8+ messages in thread
From: Jacob Keller @ 2015-08-02 10:10 UTC (permalink / raw)
To: git; +Cc: Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
Add new tests to ensure that --commit, --abort, and --strategy are
mutually exclusive.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
t/t3310-notes-merge-manual-resolve.sh | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 195bb97f859d..d5572121da69 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -314,6 +314,18 @@ y and z notes on 1st commit
EOF
+test_expect_success 'do not allow mixing --commit and --abort' '
+ test_must_fail git notes merge --commit --abort
+'
+
+test_expect_success 'do not allow mixing --commit and --strategy' '
+ test_must_fail git notes merge --commit --strategy theirs
+'
+
+test_expect_success 'do not allow mixing --abort and --strategy' '
+ test_must_fail git notes merge --abort --strategy theirs
+'
+
test_expect_success 'finalize conflicting merge (z => m)' '
# Resolve conflicts and finalize merge
cat >.git/NOTES_MERGE_WORKTREE/$commit_sha1 <<EOF &&
--
2.5.0.482.gfcd5645
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 3/4] notes: add notes.merge option to select default strategy
2015-08-02 10:10 [PATCH v3 0/4] notes: add support for notes.merge option Jacob Keller
2015-08-02 10:10 ` [PATCH v3 1/4] notes: document cat_sort_uniq rewriteMode Jacob Keller
2015-08-02 10:10 ` [PATCH v3 2/4] notes: add tests for --commit/--abort/--strategy exclusivity Jacob Keller
@ 2015-08-02 10:10 ` Jacob Keller
2015-08-05 20:47 ` Junio C Hamano
2015-08-06 22:37 ` Eric Sunshine
2015-08-02 10:10 ` [PATCH RFC 4/4] notes: add per-ref configuration of merge strategy Jacob Keller
3 siblings, 2 replies; 8+ messages in thread
From: Jacob Keller @ 2015-08-02 10:10 UTC (permalink / raw)
To: git; +Cc: Jacob Keller, Johan Herland, Michael Haggerty, Eric Sunshine
From: Jacob Keller <jacob.keller@gmail.com>
Teach git-notes about a new configuration option "notes.merge" for
selecting the default notes merge strategy. Document the option in
config.txt and git-notes.txt
Add tests for use of the configuration option. Include a test to ensure
that --strategy correctly overrides the configured setting.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
Cc: Johan Herland <johan@herland.net>
Cc: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Eric Sunshine <sunshine@sunshineco.com>
---
Documentation/config.txt | 7 +++++
Documentation/git-notes.txt | 14 +++++++++-
builtin/notes.c | 56 ++++++++++++++++++++++++++-----------
notes-merge.h | 16 ++++++-----
t/t3309-notes-merge-auto-resolve.sh | 29 +++++++++++++++++++
5 files changed, 98 insertions(+), 24 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4ce5c553e50f..0fa88a29dd65 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1911,6 +1911,13 @@ mergetool.writeToTemp::
mergetool.prompt::
Prompt before each invocation of the merge resolution program.
+notes.merge::
+ Which merge strategy to choose by default when resolving notes
+ conflicts. Must be one of `manual`, `ours`, `theirs`, `union`,
+ or `cat_sort_uniq`. Defaults to `manual`. See "NOTES MERGE
+ STRATEGIES" section of linkgit:git-notes[1] for more information
+ on each strategy.
+
notes.displayRef::
The (fully qualified) refname from which to show notes when
showing commit messages. The value of this variable can be set
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 674682b34b83..9c4f8536182f 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -101,7 +101,7 @@ merge::
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,
+conflicting notes (see the "NOTES MERGE STRATEGIES" section) 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.
@@ -183,6 +183,7 @@ OPTIONS
When merging notes, resolve notes conflicts using the given
strategy. The following strategies are recognized: "manual"
(default), "ours", "theirs", "union" and "cat_sort_uniq".
+ This option overrides the "notes.merge" configuration setting.
See the "NOTES MERGE STRATEGIES" section below for more
information on each notes merge strategy.
@@ -247,6 +248,9 @@ When done, the user can either finalize the merge with
'git notes merge --commit', or abort the merge with
'git notes merge --abort'.
+Users may select an automated merge strategy from among the following using
+either -s/--strategy option or configuring notes.merge accordingly:
+
"ours" automatically resolves conflicting notes in favor of the local
version (i.e. the current notes ref).
@@ -310,6 +314,14 @@ core.notesRef::
This setting can be overridden through the environment and
command line.
+notes.merge::
+ Which merge strategy to choose by default when resolving notes
+ conflicts. Must be one of `manual`, `ours`, `theirs`, `union`,
+ or `cat_sort_uniq`. Defaults to `manual`. See "NOTES MERGE
+ STRATEGIES" section above for more information on each strategy.
++
+This setting can be overridden by passing the `--strategy` option.
+
notes.displayRef::
Which ref (or refs, if a glob or specified more than once), in
addition to the default set by `core.notesRef` or
diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc55439..de0caa00df1b 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -92,6 +92,8 @@ static const char * const git_notes_get_ref_usage[] = {
static const char note_template[] =
"\nWrite/edit the notes for the following object:\n";
+static enum notes_merge_strategy merge_strategy;
+
struct note_data {
int given;
int use_editor;
@@ -737,6 +739,24 @@ static int merge_commit(struct notes_merge_options *o)
return ret;
}
+static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *strategy)
+{
+ if (!strcmp(arg, "manual"))
+ *strategy = NOTES_MERGE_RESOLVE_MANUAL;
+ else if (!strcmp(arg, "ours"))
+ *strategy = NOTES_MERGE_RESOLVE_OURS;
+ else if (!strcmp(arg, "theirs"))
+ *strategy = NOTES_MERGE_RESOLVE_THEIRS;
+ else if (!strcmp(arg, "union"))
+ *strategy = NOTES_MERGE_RESOLVE_UNION;
+ else if (!strcmp(arg, "cat_sort_uniq"))
+ *strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
+ else
+ return -1;
+
+ return 0;
+}
+
static int merge(int argc, const char **argv, const char *prefix)
{
struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
@@ -795,23 +815,13 @@ 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 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);
- }
+ if (strategy && parse_notes_strategy(strategy, &merge_strategy)) {
+ error("Unknown -s/--strategy: %s", strategy);
+ usage_with_options(git_notes_merge_usage, options);
}
+ o.strategy = merge_strategy;
+
t = init_notes_check("merge");
strbuf_addf(&msg, "notes: Merged notes from %s into %s",
@@ -945,6 +955,20 @@ static int get_ref(int argc, const char **argv, const char *prefix)
return 0;
}
+static int git_notes_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, "notes.merge")) {
+ if (!value)
+ return config_error_nonbool(var);
+ if (parse_notes_strategy(value, &merge_strategy))
+ return error("Unknown notes merge strategy: %s", value);
+ else
+ return 0;
+ }
+
+ return git_default_config(var, value, cb);
+}
+
int cmd_notes(int argc, const char **argv, const char *prefix)
{
int result;
@@ -955,7 +979,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
OPT_END()
};
- git_config(git_default_config, NULL);
+ git_config(git_notes_config, NULL);
argc = parse_options(argc, argv, prefix, options, git_notes_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
diff --git a/notes-merge.h b/notes-merge.h
index 1d01f6aacf54..bda8c0c8d348 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -8,18 +8,20 @@ enum notes_merge_verbosity {
NOTES_MERGE_VERBOSITY_MAX = 5
};
+enum notes_merge_strategy {
+ NOTES_MERGE_RESOLVE_MANUAL = 0,
+ NOTES_MERGE_RESOLVE_OURS,
+ NOTES_MERGE_RESOLVE_THEIRS,
+ NOTES_MERGE_RESOLVE_UNION,
+ NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ
+};
+
struct notes_merge_options {
const char *local_ref;
const char *remote_ref;
struct strbuf commit_msg;
int verbosity;
- enum {
- NOTES_MERGE_RESOLVE_MANUAL = 0,
- NOTES_MERGE_RESOLVE_OURS,
- NOTES_MERGE_RESOLVE_THEIRS,
- NOTES_MERGE_RESOLVE_UNION,
- NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ
- } strategy;
+ enum notes_merge_strategy strategy;
unsigned has_worktree:1;
};
diff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh
index 461fd84755d7..a773b01b73db 100755
--- a/t/t3309-notes-merge-auto-resolve.sh
+++ b/t/t3309-notes-merge-auto-resolve.sh
@@ -298,6 +298,13 @@ test_expect_success 'merge z into y with invalid strategy => Fail/No changes' '
verify_notes y y
'
+test_expect_success 'merge z into y with invalid configuration option => Fail/No changes' '
+ git config core.notesRef refs/notes/y &&
+ test_must_fail git -c notes.merge="foo" notes merge z &&
+ # Verify no changes (y)
+ verify_notes y y
+'
+
cat <<EOF | sort >expect_notes_ours
68b8630d25516028bed862719855b3d6768d7833 $commit_sha15
5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
@@ -365,6 +372,17 @@ test_expect_success 'reset to pre-merge state (y)' '
verify_notes y y
'
+test_expect_success 'merge z into y with "ours" configuration option => Non-conflicting 3-way merge' '
+ git -c notes.merge="ours" notes merge 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
@@ -432,6 +450,17 @@ test_expect_success 'reset to pre-merge state (y)' '
verify_notes y y
'
+test_expect_success 'merge z into y with "theirs" strategy overriding configuration option "ours" => Non-conflicting 3-way merge' '
+ git -c notes.merge="ours" 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
--
2.5.0.482.gfcd5645
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH RFC 4/4] notes: add per-ref configuration of merge strategy
2015-08-02 10:10 [PATCH v3 0/4] notes: add support for notes.merge option Jacob Keller
` (2 preceding siblings ...)
2015-08-02 10:10 ` [PATCH v3 3/4] notes: add notes.merge option to select default strategy Jacob Keller
@ 2015-08-02 10:10 ` Jacob Keller
2015-08-05 21:10 ` Junio C Hamano
3 siblings, 1 reply; 8+ messages in thread
From: Jacob Keller @ 2015-08-02 10:10 UTC (permalink / raw)
To: git; +Cc: Jacob Keller, Johan Herland, Michael Haggerty, Eric Sunshine
From: Jacob Keller <jacob.keller@gmail.com>
Teach git-notes about a new configuration option
"notes.<localref>.merge" which selects the merge strategy for a
particular ref. This allows selection of merge strategy different for
each note reference, in addition to the default strategy for all
references.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
Cc: Johan Herland <johan@herland.net>
Cc: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Eric Sunshine <sunshine@sunshineco.com>
---
Documentation/config.txt | 7 +++
Documentation/git-notes.txt | 6 ++
builtin/notes.c | 115 +++++++++++++++++++++++++++++++++++-
t/t3309-notes-merge-auto-resolve.sh | 50 ++++++++++++++++
4 files changed, 176 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0fa88a29dd65..c53ec4538cd3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1918,6 +1918,13 @@ notes.merge::
STRATEGIES" section of linkgit:git-notes[1] for more information
on each strategy.
+notes.<localref>.merge::
+ Which merge strategy to choose if the local ref for a notes merge
+ matches <localref>. Is overridden by notes.merge and takes the same
+ values. <localref> may be fully qualified or just under refs/notes/.
+ See "NOTES MERGE STRATEGIES" section in linkgit:git-notes[1] for more
+ information on each strategy.
+
notes.displayRef::
The (fully qualified) refname from which to show notes when
showing commit messages. The value of this variable can be set
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 9c4f8536182f..1e001807f9d9 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -322,6 +322,12 @@ notes.merge::
+
This setting can be overridden by passing the `--strategy` option.
+notes.<localref>.merge::
+ Which strategy to choose when merging into <localref>. Uses the same
+ values as notes.merge. <localref> may be either a fully qualified ref
+ or the shortname under "refs/notes/". See "NOTES MERGE STRATEGIES"
+ section above for more information about each strategy.
+
notes.displayRef::
Which ref (or refs, if a glob or specified more than once), in
addition to the default set by `core.notesRef` or
diff --git a/builtin/notes.c b/builtin/notes.c
index de0caa00df1b..b0174d1024dc 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -92,6 +92,10 @@ static const char * const git_notes_get_ref_usage[] = {
static const char note_template[] =
"\nWrite/edit the notes for the following object:\n";
+static struct note_ref **note_refs;
+static int note_refs_alloc;
+static int note_refs_nr;
+static struct hashmap note_refs_hash;
static enum notes_merge_strategy merge_strategy;
struct note_data {
@@ -757,12 +761,87 @@ static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *stra
return 0;
}
+struct note_refs_hash_key {
+ const char *str;
+ int len;
+};
+
+struct note_ref {
+ struct hashmap_entry ent; /* must be first */
+
+ const char *name;
+ enum notes_merge_strategy merge_strategy;
+};
+
+static int note_refs_hash_cmp(const struct note_ref *a, const struct note_ref *b, const struct note_refs_hash_key *key)
+{
+ if (key)
+ return strncmp(a->name, key->str, key->len) || a->name[key->len];
+ else
+ return strcmp(a->name, b->name);
+}
+
+static inline void init_note_refs_hash(void)
+{
+ if (!note_refs_hash.cmpfn)
+ hashmap_init(¬e_refs_hash, (hashmap_cmp_fn)note_refs_hash_cmp, 0);
+}
+
+static struct note_ref *make_note_ref(const char *name, int len)
+{
+ struct note_ref *ret, *replaced;
+ struct note_refs_hash_key lookup;
+ struct hashmap_entry lookup_entry;
+
+ if (!len)
+ len = strlen(name);
+
+ init_note_refs_hash();
+ lookup.str = name;
+ lookup.len = len;
+ hashmap_entry_init(&lookup_entry, memhash(name, len));
+
+ if ((ret = hashmap_get(¬e_refs_hash, &lookup_entry, &lookup)) != NULL)
+ return ret;
+
+ ret = xcalloc(1, sizeof(struct note_ref));
+ ALLOC_GROW(note_refs, note_refs_nr + 1, note_refs_alloc);
+ note_refs[note_refs_nr++] = ret;
+ ret->name = xstrndup(name, len);
+
+ hashmap_entry_init(ret, lookup_entry.hash);
+ replaced = hashmap_put(¬e_refs_hash, ret);
+ assert(replaced == NULL); /* no previous entry overwritten */
+ return ret;
+}
+
+static void set_strategy_for_ref(const char *ref)
+{
+ const char *name = ref;
+ struct note_refs_hash_key lookup;
+ struct hashmap_entry lookup_entry;
+ struct note_ref *note;
+
+ skip_prefix(ref, "refs/notes/", &name);
+
+ init_note_refs_hash();
+ lookup.str = name;
+ lookup.len = strlen(name);
+
+ hashmap_entry_init(&lookup_entry, memhash(name, lookup.len));
+
+ note = hashmap_get(¬e_refs_hash, &lookup_entry, &lookup);
+ if (note != NULL)
+ merge_strategy = note->merge_strategy;
+}
+
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;
+ struct note_ref;
int do_merge = 0, do_commit = 0, do_abort = 0;
int verbosity = 0, result;
const char *strategy = NULL;
@@ -815,6 +894,8 @@ static int merge(int argc, const char **argv, const char *prefix)
expand_notes_ref(&remote_ref);
o.remote_ref = remote_ref.buf;
+ set_strategy_for_ref(o.local_ref);
+
if (strategy && parse_notes_strategy(strategy, &merge_strategy)) {
error("Unknown -s/--strategy: %s", strategy);
usage_with_options(git_notes_merge_usage, options);
@@ -957,7 +1038,16 @@ static int get_ref(int argc, const char **argv, const char *prefix)
static int git_notes_config(const char *var, const char *value, void *cb)
{
- if (!strcmp(var, "notes.merge")) {
+ const char *ref;
+ const char *subkey;
+ struct note_ref *note;
+
+ if (!starts_with(var, "notes."))
+ return git_default_config(var, value, cb);
+ ref = var + 6;
+
+ /* Handle notes.* variables */
+ if (!strcmp(ref, "merge")) {
if (!value)
return config_error_nonbool(var);
if (parse_notes_strategy(value, &merge_strategy))
@@ -966,7 +1056,28 @@ static int git_notes_config(const char *var, const char *value, void *cb)
return 0;
}
- return git_default_config(var, value, cb);
+ if (*ref == '/') {
+ warning("Config notes ref cannot begin with '/': %s", ref);
+ return 0;
+ }
+ subkey = strrchr(ref, '.');
+ if (!subkey)
+ return 0;
+
+ /* skip refs/notes prefix if provided */
+ skip_prefix(ref, "refs/notes/", &ref);
+
+ note = make_note_ref(ref, subkey - ref);
+ if (!strcmp(subkey, ".merge")) {
+ if (!value)
+ return config_error_nonbool(var);
+ if (parse_notes_strategy(value, ¬e->merge_strategy))
+ return error("Unknown notes merge strategy: %s", value);
+ else
+ return 0;
+ }
+
+ return 0;
}
int cmd_notes(int argc, const char **argv, const char *prefix)
diff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh
index a773b01b73db..15dd539cd141 100755
--- a/t/t3309-notes-merge-auto-resolve.sh
+++ b/t/t3309-notes-merge-auto-resolve.sh
@@ -383,6 +383,28 @@ test_expect_success 'reset to pre-merge state (y)' '
verify_notes y y
'
+test_expect_success 'merge z into y with "ours" per-ref configuration option => Non-conflicting 3-way merge' '
+ git -c notes.y.merge="ours" notes merge 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
+'
+
+test_expect_success 'merge z into y with "ours" per-ref configuration option prefixed with "refs/notes" => Non-conflicting 3-way merge' '
+ git -c notes.refs/notes/y.merge="ours" notes merge 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
@@ -534,6 +556,34 @@ test_expect_success 'reset to pre-merge state (y)' '
verify_notes y y
'
+test_expect_success 'merge z into y with "union" strategy overriding per-ref configuration => Non-conflicting 3-way merge' '
+ git -c notes.refs/notes/y.merge="theirs" notes merge --strategy=union z &&
+ 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
+'
+
+test_expect_success 'merge z into y with "union" per-ref overriding general configuration => Non-conflicting 3-way merge' '
+ git -c notes.refs/notes/y.merge="union" -c notes.merge="theirs" notes merge z &&
+ 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
+'
+
+test_expect_success 'merge z into y with "manual" per-ref only checks specific ref configuration => Conflicting 3-way merge' '
+ test_must_fail git -c notes.refs/notes/z.merge="union" notes merge z &&
+ git notes merge --abort &&
+ verify_notes y y
+'
+
cat <<EOF | sort >expect_notes_union2
d682107b8bf7a7aea1e537a8d5cb6a12b60135f1 $commit_sha15
5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
--
2.5.0.482.gfcd5645
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 3/4] notes: add notes.merge option to select default strategy
2015-08-02 10:10 ` [PATCH v3 3/4] notes: add notes.merge option to select default strategy Jacob Keller
@ 2015-08-05 20:47 ` Junio C Hamano
2015-08-06 22:37 ` Eric Sunshine
1 sibling, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2015-08-05 20:47 UTC (permalink / raw)
To: Jacob Keller
Cc: git, Jacob Keller, Johan Herland, Michael Haggerty, Eric Sunshine
Jacob Keller <jacob.e.keller@intel.com> writes:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Teach git-notes about a new configuration option "notes.merge" for
> selecting the default notes merge strategy. Document the option in
> config.txt and git-notes.txt
>
> Add tests for use of the configuration option. Include a test to ensure
> that --strategy correctly overrides the configured setting.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> Cc: Johan Herland <johan@herland.net>
> Cc: Michael Haggerty <mhagger@alum.mit.edu>
> Cc: Eric Sunshine <sunshine@sunshineco.com>
> ---
Perhaps I am biased because we do not usually use Cc: around here,
but the above looks in a somewhat strange order. Shouldn't your
sign-off be at the end?
> +static enum notes_merge_strategy merge_strategy;
OK.
> +static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *strategy)
> +{
> + if (!strcmp(arg, "manual"))
> + *strategy = NOTES_MERGE_RESOLVE_MANUAL;
> + else if (!strcmp(arg, "ours"))
> + *strategy = NOTES_MERGE_RESOLVE_OURS;
> + else if (!strcmp(arg, "theirs"))
> + *strategy = NOTES_MERGE_RESOLVE_THEIRS;
> + else if (!strcmp(arg, "union"))
> + *strategy = NOTES_MERGE_RESOLVE_UNION;
> + else if (!strcmp(arg, "cat_sort_uniq"))
> + *strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
> + else
> + return -1;
> +
> + return 0;
> +}
OK.
> static int merge(int argc, const char **argv, const char *prefix)
> {
> struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
> @@ -795,23 +815,13 @@ 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 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);
> - }
> + if (strategy && parse_notes_strategy(strategy, &merge_strategy)) {
> + error("Unknown -s/--strategy: %s", strategy);
> + usage_with_options(git_notes_merge_usage, options);
> }
>
> + o.strategy = merge_strategy;
> +
This may be a minor taste thing, but initializing "o.strategy" with
merge_strategy at the same place as "o.remote_ref" is initialized,
and then passing &o.merge_strategy to parse_notes_strategy(), may be
easier to follow.
Renaming the global "merge_strategy" to "configured_merge_strategy"
might make it even easier to follow. If anybody uses the variable
instead of o.strategy after this point, it would be immediately
obvious that it is either a bug or it is deliberately using the
value from the configuration file, not command line.
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RFC 4/4] notes: add per-ref configuration of merge strategy
2015-08-02 10:10 ` [PATCH RFC 4/4] notes: add per-ref configuration of merge strategy Jacob Keller
@ 2015-08-05 21:10 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2015-08-05 21:10 UTC (permalink / raw)
To: Jacob Keller
Cc: git, Jacob Keller, Johan Herland, Michael Haggerty, Eric Sunshine
Jacob Keller <jacob.e.keller@intel.com> writes:
> +notes.<localref>.merge::
> + Which merge strategy to choose if the local ref for a notes merge
> + matches <localref>. Is overridden by notes.merge and takes the same
> + values. <localref> may be fully qualified or just under refs/notes/.
> + See "NOTES MERGE STRATEGIES" section in linkgit:git-notes[1] for more
> + information on each strategy.
If I have notes.refs/notes/commit.merge and notes.merge specified,
I'd expect the former overrides the latter. The second sentence may
need correcting.
I think it is a mistake to allow both notes.refs/notes/commit.merge
and notes.commit.merge. You'd end up needing to implement quite a
complicated "the last one wins" rule if you did so.
> +notes.<localref>.merge::
> + Which strategy to choose when merging into <localref>. Uses the same
> + values as notes.merge. <localref> may be either a fully qualified ref
> + or the shortname under "refs/notes/". See "NOTES MERGE STRATEGIES"
> + section above for more information about each strategy.
As a reviewer, I can tell that "Uses the same values" wants to say
that the set of allowed values is the same, but a casual reader is
bound to read it as "notes.commit.merge must be set to the same
value as the value set to notes.merge".
> diff --git a/builtin/notes.c b/builtin/notes.c
> index de0caa00df1b..b0174d1024dc 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -92,6 +92,10 @@ static const char * const git_notes_get_ref_usage[] = {
> static const char note_template[] =
> "\nWrite/edit the notes for the following object:\n";
>
> +static struct note_ref **note_refs;
> +static int note_refs_alloc;
> +static int note_refs_nr;
> +static struct hashmap note_refs_hash;
> static enum notes_merge_strategy merge_strategy;
>
> struct note_data {
> @@ -757,12 +761,87 @@ static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *stra
> return 0;
> }
> ...
> +struct note_refs_hash_key {
> + const char *str;
> + int len;
> +};
> + ...
> +static void set_strategy_for_ref(const char *ref)
> +{
> + ...
> +}
Hmmm, I do not see why you need all the complexity above.
When you come to merge(), after calling default_notes_ref(), you
know exactly which notes ref you are merging into, no? Shouldn't
then the change required for this feature just the matter of asking
the configuration system values for notes.$remote_ref.merge and
notes.merge?
IOW,
struct strbuf key = STRBUF_INIT;
char *value = NULL;
strbuf_addf(&key, "notes.%s.merge", remote_ref.buf);
git_config_get_string(key.buf, &value) ||
git_config_get_string_const("notes.merge", &value));
if (value)
parse_notes_strategy(value, &configured_merge_strategy);
...
parse_options();
if (strategy)
parse_notes_strategy(value, &configured_merge_strategy);
or something?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 3/4] notes: add notes.merge option to select default strategy
2015-08-02 10:10 ` [PATCH v3 3/4] notes: add notes.merge option to select default strategy Jacob Keller
2015-08-05 20:47 ` Junio C Hamano
@ 2015-08-06 22:37 ` Eric Sunshine
1 sibling, 0 replies; 8+ messages in thread
From: Eric Sunshine @ 2015-08-06 22:37 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git List, Jacob Keller, Johan Herland, Michael Haggerty
On Sun, Aug 2, 2015 at 6:10 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> Teach git-notes about a new configuration option "notes.merge" for
> selecting the default notes merge strategy. Document the option in
> config.txt and git-notes.txt
>
> Add tests for use of the configuration option. Include a test to ensure
> that --strategy correctly overrides the configured setting.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index 674682b34b83..9c4f8536182f 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -101,7 +101,7 @@ merge::
> If conflicts arise and a strategy for automatically resolving
> -conflicting notes (see the -s/--strategy option) is not given,
> +conflicting notes (see the "NOTES MERGE STRATEGIES" section) 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.
> @@ -183,6 +183,7 @@ OPTIONS
> When merging notes, resolve notes conflicts using the given
> strategy. The following strategies are recognized: "manual"
> (default), "ours", "theirs", "union" and "cat_sort_uniq".
> + This option overrides the "notes.merge" configuration setting.
> See the "NOTES MERGE STRATEGIES" section below for more
> information on each notes merge strategy.
These two documentation updates are much easier to digest than the
noisy-diff versions of the previous attempt; and the patch overall is
a more pleasant read than v1.
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 63f95fc55439..de0caa00df1b 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -945,6 +955,20 @@ static int get_ref(int argc, const char **argv, const char *prefix)
> return 0;
> }
>
> +static int git_notes_config(const char *var, const char *value, void *cb)
> +{
> + if (!strcmp(var, "notes.merge")) {
> + if (!value)
> + return config_error_nonbool(var);
> + if (parse_notes_strategy(value, &merge_strategy))
> + return error("Unknown notes merge strategy: %s", value);
> + else
> + return 0;
A purely subjective stylistic suggestion, which you can freely ignore
if your preference differs:
if (!value)
return ...;
if (parse_notes_strategy(...))
return ...;
return 0;
> + }
> +
> + return git_default_config(var, value, cb);
> +}
> +
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-08-06 22:37 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-02 10:10 [PATCH v3 0/4] notes: add support for notes.merge option Jacob Keller
2015-08-02 10:10 ` [PATCH v3 1/4] notes: document cat_sort_uniq rewriteMode Jacob Keller
2015-08-02 10:10 ` [PATCH v3 2/4] notes: add tests for --commit/--abort/--strategy exclusivity Jacob Keller
2015-08-02 10:10 ` [PATCH v3 3/4] notes: add notes.merge option to select default strategy Jacob Keller
2015-08-05 20:47 ` Junio C Hamano
2015-08-06 22:37 ` Eric Sunshine
2015-08-02 10:10 ` [PATCH RFC 4/4] notes: add per-ref configuration of merge strategy Jacob Keller
2015-08-05 21:10 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).