From: Jonathan Nieder <jrnieder@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Michael Grubb <devel@dailyvoid.com>, git@vger.kernel.org
Subject: Re: [PATCH v5] Add default merge options for all branches
Date: Fri, 6 May 2011 16:32:57 -0500 [thread overview]
Message-ID: <20110506213257.GD20182@elie> (raw)
In-Reply-To: <7vsjsuc704.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Subject: [PATCH] merge: fix branch.<name>.mergeoptions
To be more precise, if I understand correctly:
merge: allow branch.<name>.mergeoptions to override merge.*
> This patch should fix it, even though I now strongly suspect that
> branch.<name>.mergeoptions that gives a single command line that
> needs to be parsed was likely to be an ill-conceived idea to begin
> with. Sigh...
Yes, and introducing branch.<name>.ff and branch.<name>.log might
still be a good idea.
The patch looks mostly good. I see only one actual problem, marked
with [*] below.
> --- a/builtin-merge.c
> +++ b/builtin-merge.c
> @@ -54,6 +54,7 @@ static size_t use_strategies_nr, use_strategies_alloc;
> static const char **xopts;
> static size_t xopts_nr, xopts_alloc;
> static const char *branch;
> +static char *branch_mergeoptions;
> static int verbosity;
> static int allow_rerere_auto;
>
> @@ -474,25 +475,33 @@ cleanup:
> strbuf_release(&bname);
> }
>
> +static void parse_branch_merge_options(char *bmo)
> +{
> + const char **argv;
> + int argc;
> + char *buf;
> +
> + if (!bmo)
> + return;
> + argc = split_cmdline(bmo, &argv);
> + if (argc < 0)
> + die("Bad branch.%s.mergeoptions string", branch);
> + argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
> + memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
This is not new code, but it might make sense to do
argv[0] = "merge.*.options";
for a saner error message when someone tries
[branch "master"]
mergeoptions = --nonsense
> + argc++;
> + parse_options(argc, argv, NULL, builtin_merge_options,
> + builtin_merge_usage, 0);
> + free(buf);
[*]
This buf seems to be left over. (I don't think the intent is to
call free on an uninitialized pointer. ;-))
[...]
> - free(buf);
> + free(branch_mergeoptions);
> + branch_mergeoptions = xstrdup(v);
It is tempting to do
size_t len;
len = strlen(v);
branch_mergeoptions = xrealloc(branch_mergeoptions, len + 1);
memcpy(branch_mergeoptions, v, len + 1);
but free + xstrdup is simpler and clearer. Makes sense.
> +test_expect_success 'merge c1 with c2 (log in config gets overridden)' '
> + (
> + git config --remove-section branch.master
> + git config --remove-section merge
> + )
Since this patch is meant to apply to a very old git, we cannot use
test_might_fail. Makes sense: it can be fixed up later to use
&&-friendly syntax as part of a series introducing checks to make
sure we don't regress in that.
Thanks and hope that helps,
Jonathan
next prev parent reply other threads:[~2011-05-06 21:33 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-05-02 19:23 [PATCH 2] Add default merge options for all branches Michael Grubb
2011-05-02 22:47 ` Miklos Vajna
2011-05-02 23:36 ` Junio C Hamano
2011-05-03 5:35 ` Michael Grubb
2011-05-03 5:38 ` [PATCH v3] " Michael Grubb
2011-05-03 9:03 ` Jonathan Nieder
2011-05-03 9:49 ` Jonathan Nieder
2011-05-03 16:46 ` Michael Grubb
2011-05-03 18:16 ` Junio C Hamano
2011-05-03 20:22 ` Michael Grubb
2011-05-03 20:50 ` Jonathan Nieder
2011-05-03 20:37 ` Jens Lehmann
2011-05-03 20:07 ` [PATCH v4] " Michael Grubb
2011-05-03 20:36 ` Michael Grubb
2011-05-03 20:44 ` Jonathan Nieder
2011-05-03 22:51 ` Junio C Hamano
2011-05-04 4:25 ` Junio C Hamano
2011-05-04 4:28 ` Michael Grubb
2011-05-04 4:58 ` Jonathan Nieder
2011-05-04 18:58 ` Michael Grubb
2011-05-04 21:35 ` Junio C Hamano
2011-05-04 10:58 ` John Szakmeister
2011-05-03 22:03 ` Junio C Hamano
2011-05-03 20:37 ` [PATCH v4.1] " Michael Grubb
2011-05-04 22:07 ` [PATCH v5] " Michael Grubb
2011-05-05 0:42 ` Junio C Hamano
2011-05-06 20:36 ` Junio C Hamano
2011-05-06 21:59 ` Jonathan Nieder
2011-05-06 20:54 ` [PATCH 0/2] tests: make verify_merge check that the number of parents is right Jonathan Nieder
2011-05-06 20:58 ` [PATCH 1/2] tests: eliminate unnecessary setup test assertions Jonathan Nieder
2011-05-06 21:48 ` Jeff King
2011-05-06 22:13 ` Jeff King
2011-05-06 22:27 ` Junio C Hamano
2011-05-06 22:29 ` Jeff King
2011-05-07 22:05 ` Junio C Hamano
2011-05-09 13:31 ` [PATCH 0/3] blame --line-porcelain Jeff King
2011-05-09 13:33 ` [PATCH 1/3] add tests for various blame formats Jeff King
2011-05-09 13:34 ` [PATCH 2/3] blame: refactor porcelain output Jeff King
2011-05-09 15:39 ` Thiago Farina
2011-05-09 13:34 ` [PATCH 3/3] blame: add --line-porcelain output format Jeff King
2011-05-06 22:26 ` [PATCH 1/2] tests: eliminate unnecessary setup test assertions Jonathan Nieder
2011-05-06 21:00 ` [PATCH 2/2] tests: teach verify_parents to check for extra parents Jonathan Nieder
2011-05-06 21:34 ` Junio C Hamano
2011-05-06 21:42 ` Jonathan Nieder
2011-05-06 21:32 ` Jonathan Nieder [this message]
2011-05-06 22:01 ` [PATCH v5] Add default merge options for all branches Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20110506213257.GD20182@elie \
--to=jrnieder@gmail.com \
--cc=devel@dailyvoid.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).