Jared Hance wrote:
> Adds the option merge.defaultupstream to add support for merging from the
> upstream branch by default.
Could you give an example of breakage this configurability is designed
to prevent?
Not that it is a bad idea to be careful anyway, mind you --- just
looking for a clear answer for when people ask "and why would I ever
want to set merge.defaultToUpstream to false?"
> reduce reduncancy and impove clarity.
redundancy, improve :)
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1389,6 +1389,11 @@ man.<tool>.path::
>
> include::merge-config.txt[]
>
> +merge.defaultUpstream::
> + If merge is called without any ref arguments, merge from the branch
> + specified in branch.<current branch>.merge, which is considered to be
> + the upstream branch for the current branch.
I'd prefer to say "merge from the branch configured with --track or
--set-upstream" (but that's just me).
> +++ b/builtin/merge.c
> @@ -37,7 +37,7 @@ struct strategy {
> };
>
> static const char * const builtin_merge_usage[] = {
> - "git merge [options] <remote>...",
> + "git merge [options] [<remote>...]",
> "git merge [options] <msg> HEAD <remote>",
> NULL
Side note: these should probably say "<commit>" or "<branch>" rather
than "<remote>". I'm guessing the usage string comes from the days
before the separate-remotes ref layout...
> @@ -58,6 +58,8 @@ static int option_renormalize;
[...]
> -static int git_merge_config(const char *k, const char *v, void *cb)
> +static int per_branch_config(const char *k, const char *v, void *cb)
> {
> - if (branch && !prefixcmp(k, "branch.") &&
> - !prefixcmp(k + 7, branch) &&
> - !strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
> + const char *variable;
> + if(!branch || prefixcmp(k, "branch.")
> + || prefixcmp(k + 7, branch))
> + return 1; /* ignore me */
Style: missing space after "if" keyword.
Clarity: we are not supposed to _ignore_ the non-branch.*
configuration, just leave it for other functions to handle, no?
Maybe the comment should say "let others handle it" or something?
> +
> + variable = k + 7 + strlen(branch);
'7' can be written in a more self-explanatory way as 'strlen("branch.")'
and the optimizer will take care of translating it to 7. Don't worry
about it if that makes the diff or code harder to read, though; I'm
just mentioning the trick for future reference.
> + if(!strcmp(variable, ".mergeoptions")) {
Style: missing space after "if" keyword.
> @@ -518,9 +524,26 @@ static int git_merge_config(const char *k, const char *v, void *cb)
> parse_options(argc, argv, NULL, builtin_merge_options,
> builtin_merge_usage, 0);
> free(buf);
> +
> + return 0;
I'd group the cleanup with the return.
parse_options(...);
free(buf);
return 0;
> }
> + else if(strcmp(variable, ".merge")) {
Style: missing space after "if". Uncuddled brace.
[...]
> @@ -911,6 +934,24 @@ static int evaluate_result(void)
> return cnt;
> }
>
> +static void setup_merge_commit(struct strbuf *buf,
> + struct commit_list ***remotes, const char *s)
> +{
> + struct object *o;
> + struct commit *commit;
> +
> + o = peel_to_type(s, 0, NULL, OBJ_COMMIT);
> + if (!o)
> + die("%s - not something we can merge", s);
> + commit = lookup_commit(o->sha1);
> + commit->util = (void *)s;
> + *remotes = &commit_list_insert(commit, *remotes)->next;
> +
> + strbuf_addf(buf, "GITHEAD_%s", sha1_to_hex(o->sha1));
> + setenv(buf->buf, s, 1);
> + strbuf_reset(buf);
> +}
Would be easier to review if this code movement were in a separate
patch (separating cleanup from semantic changes).
> @@ -983,9 +1024,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> if (!allow_fast_forward && fast_forward_only)
> die("You cannot combine --no-ff with --ff-only.");
>
> - if (!argc)
> - usage_with_options(builtin_merge_usage,
> - builtin_merge_options);
> + if (!argc) {
> + if(default_upstream && upstream_branch) {
Style: missing space after "if". Unnecessary braces. To avoid
keeping the reader in suspense, it's usually best to handle the
(simple) error case first, like so:
if (!default_upstream || !upstream_branch)
usage_with_options(...);
setup_merge_commit(...);
[...]
> @@ -1048,7 +1095,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> }
> }
>
> - if (head_invalid || !argc)
> + if (head_invalid || (!argc && !(default_upstream && upstream_branch)))
Might be clearer to split the line?
if (head_invalid
|| (!argc && (!default_upstream || !upstream_branch)))
usage_with_options(...);
Even better would be to use descriptive messages, like so:
if (head_invalid)
usage_msg_opt("cannot use old-style invocation from an unborn branch",
...);
if (!argc && ...)
usage_msg_opt("no commit to merge specified", ...);
Thanks for making this happen. :)
Jonathan
--
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