Re: [PATCH v5 4/4] merge: add support for merging from upstream by default
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:50:32
Jared Hance [off-list ref] writes:
Add the option merge.defaultupstream to add support for merging from the upstream branch by default. The upstream branch is found using branch.[name].merge. Signed-off-by: Jared Hance <redacted> ---
Thanks; the first three in the series looks right.
quoted hunk
diff --git a/Documentation/config.txt b/Documentation/config.txt index c5e1835..4415691 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt@@ -1389,6 +1389,12 @@ man.<tool>.path:: include::merge-config.txt[] +merge.defaultUpstream::
I somehow had an impression that majority of others convinced you to rename this to default-to-upstream, but I may be mistaken. I think somebody who does not know the history of the development and discussion of this feature would think that this specifies the default upstream remote (i.e. not a boolean variable, but a string) given this name.
quoted hunk
@@ -536,7 +539,9 @@ static int git_merge_config(const char *k, const char *v, void *cb) if (status <= 0) return status; - if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat")) + if (!strcmp(k, "merge.defaultupstream")) + default_upstream = git_config_bool(k, v); + else if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat")) show_diffstat = git_config_bool(k, v);
It is somewhat rude to reviewers' eyes to turn an existing "if" into "else if" and place new stuff at the beginning; unless there is a good reason that the new stuff has to be at the beginning, that is. This is not a new issue, but a callback function to git_config() should return once it recognized and handled the variable, instead of cascading the controll out. The "diffstat/stat" code shows a bad example and you inherited the badness from there.