Hi,
Michael Grubb wrote:
> Add support for branch.*.mergeoptions for setting default options for
> all branches. This new value shares semantics with the existing
> branch.<name>.mergeoptions variable. If a branch specific value is
> found, that value will be used.
So in the future one might be able to do things like
[branch "git-gui/*"]
mergeoptions = -s subtree
Interesting.
> The need for this arises from the fact that there is currently not an
> easy way to set merge options for all branches.
I'm curious: what merge options/workflows does this tend to be useful
for? The above explanation seems a bit abstract (though already
convincing).
> The approach taken is to make note of whether a branch specific
> mergeoptions key has been seen and only apply the global value if it
> hasn't.
What happens if the global value is seen first?
On to the code. Warning: nitpicks ahead.
[...]
> +++ b/builtin/merge.c
> @@ -32,6 +32,13 @@
> #define NO_FAST_FORWARD (1<<2)
> #define NO_TRIVIAL (1<<3)
>
> +#define MERGEOPTIONS_DEFAULT (1<<0)
> +#define MERGEOPTIONS_BRANCH (1<<1)
Are these bitflags?
> @@ -505,24 +512,42 @@ cleanup:
>
> static int git_merge_config(const char *k, const char *v, void *cb)
> {
> + int merge_option_mode = 0;
> + struct merge_options_cb *merge_options =
> + (struct merge_options_cb *)cb;
This cast should not needed, I'd think.
[...]
> - if (branch && !prefixcmp(k, "branch.") &&
> - !prefixcmp(k + 7, branch) &&
> - !strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
> + if (!strcmp(k, "branch.*.mergeoptions"))
> + merge_option_mode = MERGEOPTIONS_DEFAULT;
> + else if (branch && !prefixcmp(k, "branch.") &&
> + !prefixcmp(k + 7, branch) &&
> + !strcmp(k + 7 + strlen(branch), ".mergeoptions"))
> + merge_option_mode = MERGEOPTIONS_BRANCH;
> +
> + if ((merge_option_mode == MERGEOPTIONS_DEFAULT &&
> + !merge_options->override_default) ||
> + merge_option_mode == MERGEOPTIONS_BRANCH) {
> const char **argv;
It is hard to see at a glance where the "if" condition ends and
the body begins. Why not
if ((merge_option_mode == MERGEOPTIONS_DEFAULT &&
!merge_options->override_default) ||
merge_option_mode == MERGEOPTIONS_BRANCH) {
const char **argv;
...
or
if (merge_option_mode == MERGEOPTIONS_BRANCH ? 1 :
merge_option_mode == MERGEOPTIONS_DEFAULT ?
!merge_options->override_default : 0) {
const char **argv;
...
or even
if (merge_option_mode == MERGEOPTIONS_DEFAULT &&
merge_options->override_default)
merge_option_mode = 0;
if (merge_option_mode) {
const char **argv;
...
?
> int argc;
> char *buf;
>
> buf = xstrdup(v);
> argc = split_cmdline(buf, &argv);
> - if (argc < 0)
> - die(_("Bad branch.%s.mergeoptions string: %s"), branch,
> - split_cmdline_strerror(argc));
> + if (argc < 0) {
> + if (merge_option_mode == 1)
> + die(_("Bad merge.mergeoptions string: %s"),
> + split_cmdline_strerror(argc));
merge.*.mergeoptions, no?
> + else
> + die(_("Bad branch.%s.mergeoptions string: %s"), branch,
> + split_cmdline_strerror(argc));
> + }
> argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
> memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
> argc++;
> parse_options(argc, argv, NULL, builtin_merge_options,
> builtin_merge_usage, 0);
> free(buf);
> + if (merge_option_mode == MERGEOPTIONS_BRANCH)
> + merge_options->override_default = 1;
Could be clearer to put this next to the code that checks
override_default.
[...]
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -415,6 +415,33 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
>
> test_debug 'git log --graph --decorate --oneline --all'
>
> +test_expect_success 'merge c0 with c1 (global no-ff)' '
> + git reset --hard c0 &&
> + git config --unset branch.master.mergeoptions &&
Better to make that
test_might_fail git config --unset ...
so it will still work if earlier tests stop setting that
variable.
> + git config "branch.*.mergeoptions" "--no-ff" &&
> + test_tick &&
> + git merge c1 &&
> + git config --remove-section "branch.*" &&
> + verify_merge file result.1 &&
> + verify_parents $c0 $c1
> +'
> +
> +test_debug 'git log --graph --decorate --oneline --all'
Yuck. Did anything come of the idea of a --between-tests option to
use an arbitrary command here automatically? (Not your fault.)
> +
> +test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '
> + git reset --hard c0 &&
> + git config --remove-section branch.master &&
Could make sense to use test_might_fail for this one, too.
> + git config "branch.*.mergeoptions" "--no-ff" &&
> + git config branch.master.mergeoptions "--ff" &&
> + test_tick &&
> + git merge c1 &&
> + git config --remove-section "branch.*" &&
> + verify_merge file result.1 &&
> + verify_parents "$c0"
> +'
> +
> +test_debug 'git log --graph --decorate --oneline --all'
Nice, a clean patch with a few reasonable tests.
With whichever of the changes below make sense,
Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
Thanks.
---
builtin/merge.c | 37 ++++++++++++++++++++-----------------
t/t7600-merge.sh | 4 ++--
2 files changed, 22 insertions(+), 19 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 9fe129f..7156e92 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -32,8 +32,8 @@
#define NO_FAST_FORWARD (1<<2)
#define NO_TRIVIAL (1<<3)
-#define MERGEOPTIONS_DEFAULT (1<<0)
-#define MERGEOPTIONS_BRANCH (1<<1)
+#define MERGEOPTIONS_DEFAULT 1
+#define MERGEOPTIONS_BRANCH 2
struct merge_options_cb {
int override_default;
@@ -513,8 +513,7 @@ cleanup:
static int git_merge_config(const char *k, const char *v, void *cb)
{
int merge_option_mode = 0;
- struct merge_options_cb *merge_options =
- (struct merge_options_cb *)cb;
+ struct merge_options_cb *merge_options = cb;
if (!strcmp(k, "branch.*.mergeoptions"))
merge_option_mode = MERGEOPTIONS_DEFAULT;
@@ -523,31 +522,35 @@ static int git_merge_config(const char *k, const char *v, void *cb)
!strcmp(k + 7 + strlen(branch), ".mergeoptions"))
merge_option_mode = MERGEOPTIONS_BRANCH;
- if ((merge_option_mode == MERGEOPTIONS_DEFAULT &&
- !merge_options->override_default) ||
- merge_option_mode == MERGEOPTIONS_BRANCH) {
+ /*
+ * If an applicable [branch "foo"] mergeoptions setting was
+ * seen already, let it mask the [branch "*"] defaults.
+ */
+ if (merge_options->override_default &&
+ merge_option_mode == MERGEOPTIONS_DEFAULT)
+ merge_option_mode = 0;
+
+ if (merge_option_mode == MERGEOPTIONS_BRANCH)
+ merge_options->override_default = 1;
+
+ if (merge_option_mode) {
const char **argv;
int argc;
char *buf;
buf = xstrdup(v);
argc = split_cmdline(buf, &argv);
- if (argc < 0) {
- if (merge_option_mode == 1)
- die(_("Bad merge.mergeoptions string: %s"),
- split_cmdline_strerror(argc));
- else
- die(_("Bad branch.%s.mergeoptions string: %s"), branch,
- split_cmdline_strerror(argc));
- }
+ if (argc < 0)
+ die(_("Bad merge.%s.mergeoptions string: %s"),
+ merge_option_mode == MERGEOPTIONS_DEFAULT ?
+ "*" : branch,
+ split_cmdline_strerror(argc));
argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
argc++;
parse_options(argc, argv, NULL, builtin_merge_options,
builtin_merge_usage, 0);
free(buf);
- if (merge_option_mode == MERGEOPTIONS_BRANCH)
- merge_options->override_default = 1;
}
if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index cea2b31..ff807f4 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -417,7 +417,7 @@ test_debug 'git log --graph --decorate --oneline --all'
test_expect_success 'merge c0 with c1 (global no-ff)' '
git reset --hard c0 &&
- git config --unset branch.master.mergeoptions &&
+ test_might_fail git config --unset branch.master.mergeoptions &&
git config "branch.*.mergeoptions" "--no-ff" &&
test_tick &&
git merge c1 &&
@@ -430,7 +430,7 @@ test_debug 'git log --graph --decorate --oneline --all'
test_expect_success 'combine merge.mergeoptions with branch.x.mergeoptions' '
git reset --hard c0 &&
- git config --remove-section branch.master &&
+ test_might_fail git config --remove-section branch.master &&
git config "branch.*.mergeoptions" "--no-ff" &&
git config branch.master.mergeoptions "--ff" &&
test_tick &&
--
1.7.5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html