Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy
On Sun, Aug 2, 2015 at 1:01 AM, Eric Sunshine wrote: > Don't worry too much about it. Consider it something to keep in mind for > future patches. I reviewed the change and it seemed okay. I mentioned it > because one of the goals of patch submission, in addition to making an > actual change, is to ease the review process. If Junio is okay with > accepting it as is, then it's probably not worth spending more time > trying to refine it. > > Having said that, I came up with the following for those two paragraphs, > which gives a much less noisy diff and doesn't look too jagged: > Yea, I actually have re-worked it as well to something more suitable. I'm re-sending anyways as I fixed some of the other style issues and the one bug about not returning 0 on good read of the notes.merge value. Regards Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy
On Sun, Aug 02, 2015 at 12:41:08AM -0700, Jacob Keller wrote: > On Sat, Aug 1, 2015 at 7:46 PM, Eric Sunshine wrote: > >> If conflicts arise and a strategy for automatically resolving > >> -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 --abort'. > >> +conflicting notes (see the -s/--strategy option or notes.merge > >> +config 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 --abort'. > > > > When you re-wrap the text like this, it's difficult to spot your one > > little change in all the diff noise. For the sake of review, it's > > often better to minimize the change, even if it leaves the text more > > jagged than you'd like. > > This results in something incredibly jagged. I can't find a good way > to split which minimizes the change. Would a 3rd patch which just > re-flows this be an acceptable alternative > > ie: add the words in one patch then re-flow afterwards in a second > patch with no changes? > > There is no good alternative as other re-flows I tried end up looking > way too jagged, as compared to surrounding documentation. Don't worry too much about it. Consider it something to keep in mind for future patches. I reviewed the change and it seemed okay. I mentioned it because one of the goals of patch submission, in addition to making an actual change, is to ease the review process. If Junio is okay with accepting it as is, then it's probably not worth spending more time trying to refine it. Having said that, I came up with the following for those two paragraphs, which gives a much less noisy diff and doesn't look too jagged: --- 8< --- If conflicts arise and a strategy for automatically resolving -conflicting notes (see the -s/--strategy option) is not given, -the "manual" resolver is used. This resolver checks out the +conflicting notes (see -s/--strategy or notes.merge configuration) 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 --- 8< --- ...and... --- 8< --- When merging notes, resolve notes conflicts using the given - strategy. The following strategies are recognized: "manual" + strategy. Overrides the "notes.merge" configuration variable. + The following strategies are recognized: "manual" (default), "ours", "theirs", "union" and "cat_sort_uniq". See the "NOTES MERGE STRATEGIES" section below for more information on each notes merge strategy. --- 8< --- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy
On Sat, Aug 1, 2015 at 7:46 PM, Eric Sunshine wrote: >> If conflicts arise and a strategy for automatically resolving >> -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 --abort'. >> +conflicting notes (see the -s/--strategy option or notes.merge >> +config 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 --abort'. > > When you re-wrap the text like this, it's difficult to spot your one > little change in all the diff noise. For the sake of review, it's > often better to minimize the change, even if it leaves the text more > jagged than you'd like. This results in something incredibly jagged. I can't find a good way to split which minimizes the change. Would a 3rd patch which just re-flows this be an acceptable alternative ie: add the words in one patch then re-flow afterwards in a second patch with no changes? There is no good alternative as other re-flows I tried end up looking way too jagged, as compared to surrounding documentation. Regards, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy
On Sat, Aug 1, 2015 at 7:46 PM, Eric Sunshine wrote:
> On Fri, Jul 31, 2015 at 7:12 PM, Jacob Keller
> 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 the configuration option. Ensure that command line
>> --strategy option overrides the configured value. Ensure that -s can't
>> be passed with --commit or --abort. Ensure that --abort and --commit
>> can't be used together.
>>
>> Signed-off-by: Jacob Keller
>> Cc: Johan Herland
>> Cc: Michael Haggerty
>> Cc: Eric Sunshine
>
> Thanks, this looks better than the previous round. A few comments below...
>
>> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
>> index 674682b34b83..d8944f5aec60 100644
>> --- a/Documentation/git-notes.txt
>> +++ b/Documentation/git-notes.txt
>> @@ -101,13 +101,13 @@ 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,
>> -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 --abort'.
>> +conflicting notes (see the -s/--strategy option or notes.merge
>> +config 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 --abort'.
>
> When you re-wrap the text like this, it's difficult to spot your one
> little change in all the diff noise. For the sake of review, it's
> often better to minimize the change, even if it leaves the text more
> jagged than you'd like.
>
Even if it leaves it incredibly jagged? That is one of the downsides
with diff of flowed text like this :(
>> remove::
>> Remove the notes for given objects (defaults to HEAD). When
>> @@ -181,10 +181,10 @@ OPTIONS
>> -s ::
>> --strategy=::
>> When merging notes, resolve notes conflicts using the given
>> - strategy. The following strategies are recognized: "manual"
>> - (default), "ours", "theirs", "union" and "cat_sort_uniq".
>> - See the "NOTES MERGE STRATEGIES" section below for more
>> - information on each notes merge strategy.
>> + strategy. Overrides "notes.merge". The following strategies are
>> + recognized: `manual`, `ours`, `theirs`, `union` and
>> + `cat_sort_uniq`. See the `NOTES MERGE STRATEGIES` section below
>> + for more information on each notes merge strategy.
>
> Ditto.
>
>> --commit::
>> Finalize an in-progress 'git notes merge'. Use this option
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index 88fdafb32bc0..728980bc79bf 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> @@ -741,6 +743,25 @@ 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;
>
> In this codebase, it's customary to return 0 on success and -1 on error
>
Fair enough.
>> + }
>
> Style: drop unnecessary braces
>
>> +
>> + return 0;
>> +}
>> +
>> static int merge(int argc, const char **argv, const char *prefix)
>> {
>> struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
>> @@ -950,6 +961,18 @@ 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);
>
> For the non-error case, don't you want to 'return 0' here to indicate
> that you handled the config variabl
Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy
On Fri, Jul 31, 2015 at 7:12 PM, Jacob Keller 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 the configuration option. Ensure that command line
> --strategy option overrides the configured value. Ensure that -s can't
> be passed with --commit or --abort. Ensure that --abort and --commit
> can't be used together.
>
> Signed-off-by: Jacob Keller
> Cc: Johan Herland
> Cc: Michael Haggerty
> Cc: Eric Sunshine
Thanks, this looks better than the previous round. A few comments below...
> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index 674682b34b83..d8944f5aec60 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -101,13 +101,13 @@ 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,
> -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 --abort'.
> +conflicting notes (see the -s/--strategy option or notes.merge
> +config 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 --abort'.
When you re-wrap the text like this, it's difficult to spot your one
little change in all the diff noise. For the sake of review, it's
often better to minimize the change, even if it leaves the text more
jagged than you'd like.
> remove::
> Remove the notes for given objects (defaults to HEAD). When
> @@ -181,10 +181,10 @@ OPTIONS
> -s ::
> --strategy=::
> When merging notes, resolve notes conflicts using the given
> - strategy. The following strategies are recognized: "manual"
> - (default), "ours", "theirs", "union" and "cat_sort_uniq".
> - See the "NOTES MERGE STRATEGIES" section below for more
> - information on each notes merge strategy.
> + strategy. Overrides "notes.merge". The following strategies are
> + recognized: `manual`, `ours`, `theirs`, `union` and
> + `cat_sort_uniq`. See the `NOTES MERGE STRATEGIES` section below
> + for more information on each notes merge strategy.
Ditto.
> --commit::
> Finalize an in-progress 'git notes merge'. Use this option
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 88fdafb32bc0..728980bc79bf 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -741,6 +743,25 @@ 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;
In this codebase, it's customary to return 0 on success and -1 on error
> + }
Style: drop unnecessary braces
> +
> + return 0;
> +}
> +
> static int merge(int argc, const char **argv, const char *prefix)
> {
> struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
> @@ -950,6 +961,18 @@ 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);
For the non-error case, don't you want to 'return 0' here to indicate
that you handled the config variable rather than letting it fall
through to git_default_config() below?
> + }
> +
> + return git_default_config(var, value, cb);
> +}
> +
> int cmd_notes(int argc, const char **argv, const char *prefix)
> {
> int result;
--
To unsubscribe from this list: send the line "unsubscribe git" in
[PATCH v2 2/2] notes: add notes.merge option to select default strategy
From: Jacob Keller
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 the configuration option. Ensure that command line
--strategy option overrides the configured value. Ensure that -s can't
be passed with --commit or --abort. Ensure that --abort and --commit
can't be used together.
Signed-off-by: Jacob Keller
Cc: Johan Herland
Cc: Michael Haggerty
Cc: Eric Sunshine
---
Documentation/config.txt | 7 +
Documentation/git-notes.txt | 30 ---
builtin/notes.c | 55 +--
notes-merge.h | 16 +-
t/t3309-notes-merge-auto-resolve.sh | 29 ++
t/t3310-notes-merge-manual-resolve.sh | 12
6 files changed, 115 insertions(+), 34 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3c1e4df09beb..85c15126e4ce 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1937,6 +1937,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..d8944f5aec60 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -101,13 +101,13 @@ 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,
-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 --abort'.
+conflicting notes (see the -s/--strategy option or notes.merge
+config 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 --abort'.
remove::
Remove the notes for given objects (defaults to HEAD). When
@@ -181,10 +181,10 @@ OPTIONS
-s ::
--strategy=::
When merging notes, resolve notes conflicts using the given
- strategy. The following strategies are recognized: "manual"
- (default), "ours", "theirs", "union" and "cat_sort_uniq".
- See the "NOTES MERGE STRATEGIES" section below for more
- information on each notes merge strategy.
+ strategy. Overrides "notes.merge". The following strategies are
+ recognized: `manual`, `ours`, `theirs`, `union` and
+ `cat_sort_uniq`. 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
@@ -310,6 +310,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 88fdafb32bc0..728980bc79bf 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -93,6 +93,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;
@@ -741,6 +743,25 @@ 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 = NOTE
