* Re: [PATCH v3] Add default merge options for all branches
2011-05-03 9:03 ` Jonathan Nieder
@ 2011-05-03 9:49 ` Jonathan Nieder
2011-05-03 16:46 ` Michael Grubb
` (4 subsequent siblings)
5 siblings, 0 replies; 46+ messages in thread
From: Jonathan Nieder @ 2011-05-03 9:49 UTC (permalink / raw)
To: Michael Grubb; +Cc: git, Junio C Hamano, vmiklos, deskinm
Jonathan Nieder wrote:
> Michael Grubb wrote:
>> 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?
[...]
> With whichever of the changes below make sense,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Agh, what I meant is "except as noted above". I am still worried
about (and have not carefully reviewed) the following case:
[branch "*"]
mergeoptions = --foo
[branch "topic"]
mergeoptions = --bar
Does it produce different behavior from
[branch "topic"]
mergeoptions = --bar
[branch "*"]
mergeoptions = --foo
? If so, is that a good thing, and is it documented?
Sorry for the lack of clarity.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v3] Add default merge options for all branches
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
` (3 subsequent siblings)
5 siblings, 0 replies; 46+ messages in thread
From: Michael Grubb @ 2011-05-03 16:46 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Junio C Hamano, vmiklos, deskinm
On 5/3/11 4:03 AM, Jonathan Nieder wrote:
> 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).
For myself I've adopted Vincent Driessen's branching model
(http://nvie.com/posts/a-successful-git-branching-model/)
Which specifically recommends using --no-ff when merging to preserve
the history of the existence of a topic branch once those branches are
removed. But it would equally be beneficial for folks that want the
--log, etc option turned on for every merge.
>
>> 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?
I will code a test for this, but if the global option is seen first then
those options are used, unless/until a branch specific option is seen which will
override the globals. If the branch specific option is seen first then the global
options are ignored. (I did test this btw, but I didn't add it in the unit tests, bad me).
>
> 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?
I had been using literals in the git_merge_config function but thought better of it and added
these defines. I'm not really using them as bitflags right now. Perhaps an enum, or just plain
literals are better. I'm not clear what the best practice is in this case, so I'm certainly open
to a better/more clear way of doing this.
>
>> @@ -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.
Indeed. It has been removed.
>
> [...]
>> - 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
I completely agree, though believe it or not, this was what the original code looked like
so I was following it's lead. Here is what the original looks like:
if (branch && !prefixcmp(k, "branch.") &&
!prefixcmp(k + 7, branch) &&
!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
const char **argv;
int argc;
char *buf;
I would, of course, be happy to clear this up.
>
> 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.
Yes. I had put this there originally so that it would only be set if there were no errors in parsing the options
but after chasing the rabbit down the whole, if there are errors parse_options won't return, so I'll move this
up closer to the override_default check.
>
> [...]
>> --- 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.
Thanks for all the constructive criticism, Jonathan.
I will make the recommended changes and post a new patch.
>
> With whichever of the changes below make sense,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> 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 &&
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v3] Add default merge options for all branches
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:37 ` Jens Lehmann
2011-05-03 20:07 ` [PATCH v4] " Michael Grubb
` (2 subsequent siblings)
5 siblings, 2 replies; 46+ messages in thread
From: Junio C Hamano @ 2011-05-03 18:16 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Michael Grubb, git, Junio C Hamano, vmiklos, deskinm
Jonathan Nieder <jrnieder@gmail.com> writes:
> 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?
I actually am curious myself, too. I want to see a real-life example.
The "git-gui/*" example you gave is unfortunately not it. It specifies the
branch at the wrong end. Whether I am merging into "master" or "next", I
would want "-s subtree" when I am merging "git-gui/*" project, but all my
merges to "master" or "next" do not necessarily want to use "-s subtree".
It might turn out to be that "branch.<name>.mergeoptions" is a
ill-conceived idea to begin with.
>> 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?
If implemented correctly, it should use the specific one and fall back to
the wildcard one. Another issue is if the values should be cumulative or
overriding, but in the remainder I'd assume we want overriding.
>> @@ -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.
Correct. That is the whole point of using (void *) as a parameter, so
that it can be assigned to the real expected type easily without cast.
>> + 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
> ...
> or even
> ...
> ?
Why not have two string pointers in merge_options_cb structure that are
initialized to NULL and holds the matched config key and the value?
When we are looking at a (k, v) pair in this function, if there is no
previous key in cb, we store (k, v) in cb and return. If there already is
a previous key, we see if k is more specific than that key, and replace
(k, v) in cb with what we currently have. Otherwise we do not do
anything.
When git_config() returns, the caller will find the final value in cb.
>> + 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.)
I actually think test_debug should go inside the previous test. Why not
have it immediately after "git merge c1" above?
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v3] Add default merge options for all branches
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
1 sibling, 1 reply; 46+ messages in thread
From: Michael Grubb @ 2011-05-03 20:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, vmiklos, deskinm
On 5/3/11 1:16 PM, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> 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?
>
> I actually am curious myself, too. I want to see a real-life example.
>
In my reply to Jonathan I gave the example of turning off fast forward merges
globally for all branches (and then perhaps turning them back on for specific branches).
The same could be done with --log or any other option that might make since to turn on
for all branches without having to specifically name each branch in the config file.
Also as previously cited in my earlier reply to Jonathan, a good real life example can be
found in Vincent Driessen's branching model where he creates ephemeral feature/topic branches
for development then deletes them when they get merged back into the mainline development tree.
However, he wants to keep the history that there was a merge from a branch instead of it just looking
like a straight line. The link to his work is here: http://nvie.com/posts/a-successful-git-branching-model/
> The "git-gui/*" example you gave is unfortunately not it. It specifies the
> branch at the wrong end. Whether I am merging into "master" or "next", I
> would want "-s subtree" when I am merging "git-gui/*" project, but all my
> merges to "master" or "next" do not necessarily want to use "-s subtree".
>
> It might turn out to be that "branch.<name>.mergeoptions" is a
> ill-conceived idea to begin with.
>
>>> 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?
>
> If implemented correctly, it should use the specific one and fall back to
> the wildcard one. Another issue is if the values should be cumulative or
> overriding, but in the remainder I'd assume we want overriding.
>
There is now a unit test for this scenario.
>>> @@ -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.
>
> Correct. That is the whole point of using (void *) as a parameter, so
> that it can be assigned to the real expected type easily without cast.
>
This has been corrected in the latest version of the patch.
>>> + 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
>> ...
>> or even
>> ...
>> ?
>
> Why not have two string pointers in merge_options_cb structure that are
> initialized to NULL and holds the matched config key and the value?
>
That was a great idea. I've reworked things around this and also moved
to a voting type method for determining priority of the keys. So no more
defines needed. I think keeping all that state in the struct is much better
and scales better as well.
> When we are looking at a (k, v) pair in this function, if there is no
> previous key in cb, we store (k, v) in cb and return. If there already is
> a previous key, we see if k is more specific than that key, and replace
> (k, v) in cb with what we currently have. Otherwise we do not do
> anything.
>
> When git_config() returns, the caller will find the final value in cb.
>
>>> + 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.)
>
> I actually think test_debug should go inside the previous test. Why not
> have it immediately after "git merge c1" above?
Again I followed the existing pattern here. I didn't want to be the odd man out.
Do you want me to make that change?
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v3] Add default merge options for all branches
2011-05-03 20:22 ` Michael Grubb
@ 2011-05-03 20:50 ` Jonathan Nieder
0 siblings, 0 replies; 46+ messages in thread
From: Jonathan Nieder @ 2011-05-03 20:50 UTC (permalink / raw)
To: Michael Grubb; +Cc: Junio C Hamano, git, vmiklos, deskinm
Michael Grubb wrote:
> On 5/3/11 1:16 PM, Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>> Yuck. Did anything come of the idea of a --between-tests option to
>>> use an arbitrary command here automatically? (Not your fault.)
>>
>> I actually think test_debug should go inside the previous test. Why not
>> have it immediately after "git merge c1" above?
>
> Again I followed the existing pattern here. I didn't want to be the odd man out.
> Do you want me to make that change?
No, I was just reminding the list at large about how annoying it is.
You did right to mimic the style of surrounding tests. Thank you.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Add default merge options for all branches
2011-05-03 18:16 ` Junio C Hamano
2011-05-03 20:22 ` Michael Grubb
@ 2011-05-03 20:37 ` Jens Lehmann
1 sibling, 0 replies; 46+ messages in thread
From: Jens Lehmann @ 2011-05-03 20:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Michael Grubb, git, vmiklos, deskinm
Am 03.05.2011 20:16, schrieb Junio C Hamano:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>>> 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?
>
> I actually am curious myself, too. I want to see a real-life example.
At my dayjob the reviewer of a branch does the merge of a topic into
master. And to always record who did the review even if the merge is
a fast forward we use the "branch.master.mergeoptions=--no-ff" config
in all our repos, set by our installer.
But that doesn't work out of the box for our release branches, we have
to manually set stuff like "branch.v1.23-4.mergeoptions=--no-ff" for
every release branch in every clone where merges are done. This is easy
to forget and as we use git gui for our daily work, we can't simply use
the command line option --no-ff.
So we would greatly benefit from this patch (and actually I looked into
doing it myself some time ago but postponed it to do some submodule
stuff first ;-)
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4] Add default merge options for all branches
2011-05-03 9:03 ` Jonathan Nieder
` (2 preceding siblings ...)
2011-05-03 18:16 ` Junio C Hamano
@ 2011-05-03 20:07 ` Michael Grubb
2011-05-03 20:36 ` Michael Grubb
` (2 more replies)
2011-05-03 20:37 ` [PATCH v4.1] " Michael Grubb
2011-05-04 22:07 ` [PATCH v5] " Michael Grubb
5 siblings, 3 replies; 46+ messages in thread
From: Michael Grubb @ 2011-05-03 20:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, vmiklos
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.
The need for this arises from the fact that there is currently not an
easy way to set merge options for all branches. Instead of having to
specify merge options for each individual branch there should be a way
to set defaults for all branches and then override a specific branch's
options.
In order to facilitate future features centered around this new
"globlike" syntax a new struct has been created to keep track of the
branch.*.* options. Currently it only supports branch.*.mergeoptions,
but can be easily modified in the future to support other branch
specific options as well. The mechanism will "vote" on specific-"ness"
of the configuration key and ultimately only use the most specific
options. This is not cumulative but overriding.
Signed-off-by: Michael Grubb <devel@dailyvoid.com>
---
Documentation/git-merge.txt | 3 +
builtin/merge.c | 90 +++++++++++++++++++++++++++++++++---------
t/t7600-merge.sh | 39 ++++++++++++++++++
3 files changed, 112 insertions(+), 20 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index e2e6aba..eaab3e4 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -307,6 +307,9 @@ branch.<name>.mergeoptions::
Sets default options for merging into branch <name>. The syntax and
supported options are the same as those of 'git merge', but option
values containing whitespace characters are currently not supported.
+ The special value '*' for <name> may be used to configure default
+ options for all branches. Values for specific branch names will
+ override the this default.
SEE ALSO
--------
diff --git a/builtin/merge.c b/builtin/merge.c
index d171c63..d6ce85e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -32,6 +32,21 @@
#define NO_FAST_FORWARD (1<<2)
#define NO_TRIVIAL (1<<3)
+/* This is for branch.<foo>. blocks
+ * the vote member holds a value between
+ * 0.0 and 1.0 which measures how closely
+ * a branch name matches the key member.
+ * where branch.*.mergeoptions would be 0.1 and
+ * branch.<name>.mergeoptions would be 1.0
+ * Also it is called vote because I couldn't come
+ * up with a better name.
+ */
+struct merge_options_cb {
+ char *key;
+ char *value;
+ float vote;
+};
+
struct strategy {
const char *name;
unsigned attr;
@@ -503,28 +518,60 @@ cleanup:
strbuf_release(&bname);
}
-static int git_merge_config(const char *k, const char *v, void *cb)
+static void parse_git_merge_options(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 **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));
- 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);
+ struct merge_options_cb *merge_options = cb;
+ int changed = 0;
+
+ /* We only handle mergeoptions for now */
+ if (suffixcmp(k, ".mergeoptions"))
+ return;
+
+ if (!prefixcmp(k, "branch.*") && merge_options->vote <= 0.1 ) {
+ merge_options->vote = 0.1;
+ changed = 1;
+ } else if (branch && !prefixcmp(k, "branch.") &&
+ !prefixcmp(k + 7, branch) &&
+ merge_options->vote < 1.0) {
+ merge_options->vote = 1.0;
+ changed = 1;
}
+ if (changed) {
+ merge_options->key = (char *)k;
+ merge_options->value = (char *)v;
+ }
+}
+
+static void apply_merge_options(struct merge_options_cb *opts)
+{
+ const char **argv;
+ int argc;
+ char *buf;
+
+ if ( opts == NULL )
+ return;
+
+ buf = xstrdup(opts->value);
+ argc = split_cmdline(buf, &argv);
+ if (argc < 0)
+ die(_("Bad %s string: %s"),
+ opts->key, 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);
+}
+
+static int git_merge_config(const char *k, const char *v, void *cb)
+{
+ if (cb != NULL && branch && !prefixcmp(k, "branch."))
+ parse_git_merge_options(k, v, cb);
+
if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
show_diffstat = git_config_bool(k, v);
else if (!strcmp(k, "pull.twohead"))
@@ -987,6 +1034,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
const char *head_arg;
int flag, head_invalid = 0, i;
int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
+ struct merge_options_cb merge_options = {NULL, NULL, 0.0};
struct commit_list *common = NULL;
const char *best_strategy = NULL, *wt_strategy = NULL;
struct commit_list **remotes = &remoteheads;
@@ -1004,7 +1052,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (is_null_sha1(head))
head_invalid = 1;
- git_config(git_merge_config, NULL);
+ git_config(git_merge_config, &merge_options);
+ if (merge_options.key != NULL && merge_options.value != NULL)
+ apply_merge_options(&merge_options);
/* for color.ui */
if (diff_use_color_default == -1)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index e84e822..5b1f8e1 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
test_debug 'git log --graph --decorate --oneline --all'
+test_expect_success 'merge c0 with c1 (default no-ff)' '
+ git reset --hard c0 &&
+ test_might_fail git config --unset branch.master.mergeoptions &&
+ 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'
+
+test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '
+ git reset --hard c0 &&
+ test_might_fail git config --remove-section branch.master &&
+ 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_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '
+ git reset --hard c0 &&
+ test_might_fail git config --remove-section branch.master &&
+ git config branch.master.mergeoptions "--ff" &&
+ git config "branch.*.mergeoptions" "--no-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'
+
test_expect_success 'combining --squash and --no-ff is refused' '
test_must_fail git merge --squash --no-ff c1 &&
test_must_fail git merge --no-ff --squash c1
--
1.7.5
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4] Add default merge options for all branches
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:03 ` Junio C Hamano
2 siblings, 0 replies; 46+ messages in thread
From: Michael Grubb @ 2011-05-03 20:36 UTC (permalink / raw)
Cc: git, Junio C Hamano, Jonathan Nieder, vmiklos
This patch has a trailing whitespace on one line.
I'm resubmitting to fix it.
On 5/3/11 3:07 PM, 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.
>
> The need for this arises from the fact that there is currently not an
> easy way to set merge options for all branches. Instead of having to
> specify merge options for each individual branch there should be a way
> to set defaults for all branches and then override a specific branch's
> options.
>
> In order to facilitate future features centered around this new
> "globlike" syntax a new struct has been created to keep track of the
> branch.*.* options. Currently it only supports branch.*.mergeoptions,
> but can be easily modified in the future to support other branch
> specific options as well. The mechanism will "vote" on specific-"ness"
> of the configuration key and ultimately only use the most specific
> options. This is not cumulative but overriding.
>
> Signed-off-by: Michael Grubb <devel@dailyvoid.com>
> ---
> Documentation/git-merge.txt | 3 +
> builtin/merge.c | 90 +++++++++++++++++++++++++++++++++---------
> t/t7600-merge.sh | 39 ++++++++++++++++++
> 3 files changed, 112 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index e2e6aba..eaab3e4 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -307,6 +307,9 @@ branch.<name>.mergeoptions::
> Sets default options for merging into branch <name>. The syntax and
> supported options are the same as those of 'git merge', but option
> values containing whitespace characters are currently not supported.
> + The special value '*' for <name> may be used to configure default
> + options for all branches. Values for specific branch names will
> + override the this default.
>
> SEE ALSO
> --------
> diff --git a/builtin/merge.c b/builtin/merge.c
> index d171c63..d6ce85e 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -32,6 +32,21 @@
> #define NO_FAST_FORWARD (1<<2)
> #define NO_TRIVIAL (1<<3)
>
> +/* This is for branch.<foo>. blocks
> + * the vote member holds a value between
> + * 0.0 and 1.0 which measures how closely
> + * a branch name matches the key member.
> + * where branch.*.mergeoptions would be 0.1 and
> + * branch.<name>.mergeoptions would be 1.0
> + * Also it is called vote because I couldn't come
> + * up with a better name.
> + */
> +struct merge_options_cb {
> + char *key;
> + char *value;
> + float vote;
> +};
> +
> struct strategy {
> const char *name;
> unsigned attr;
> @@ -503,28 +518,60 @@ cleanup:
> strbuf_release(&bname);
> }
>
> -static int git_merge_config(const char *k, const char *v, void *cb)
> +static void parse_git_merge_options(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 **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));
> - 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);
> + struct merge_options_cb *merge_options = cb;
> + int changed = 0;
> +
> + /* We only handle mergeoptions for now */
> + if (suffixcmp(k, ".mergeoptions"))
> + return;
> +
> + if (!prefixcmp(k, "branch.*") && merge_options->vote <= 0.1 ) {
> + merge_options->vote = 0.1;
> + changed = 1;
> + } else if (branch && !prefixcmp(k, "branch.") &&
> + !prefixcmp(k + 7, branch) &&
> + merge_options->vote < 1.0) {
> + merge_options->vote = 1.0;
> + changed = 1;
> }
>
> + if (changed) {
> + merge_options->key = (char *)k;
> + merge_options->value = (char *)v;
> + }
> +}
> +
> +static void apply_merge_options(struct merge_options_cb *opts)
> +{
> + const char **argv;
> + int argc;
> + char *buf;
> +
> + if ( opts == NULL )
> + return;
> +
> + buf = xstrdup(opts->value);
> + argc = split_cmdline(buf, &argv);
> + if (argc < 0)
> + die(_("Bad %s string: %s"),
> + opts->key, 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);
> +}
> +
> +static int git_merge_config(const char *k, const char *v, void *cb)
> +{
> + if (cb != NULL && branch && !prefixcmp(k, "branch."))
> + parse_git_merge_options(k, v, cb);
> +
> if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
> show_diffstat = git_config_bool(k, v);
> else if (!strcmp(k, "pull.twohead"))
> @@ -987,6 +1034,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> const char *head_arg;
> int flag, head_invalid = 0, i;
> int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
> + struct merge_options_cb merge_options = {NULL, NULL, 0.0};
> struct commit_list *common = NULL;
> const char *best_strategy = NULL, *wt_strategy = NULL;
> struct commit_list **remotes = &remoteheads;
> @@ -1004,7 +1052,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> if (is_null_sha1(head))
> head_invalid = 1;
>
> - git_config(git_merge_config, NULL);
> + git_config(git_merge_config, &merge_options);
> + if (merge_options.key != NULL && merge_options.value != NULL)
> + apply_merge_options(&merge_options);
>
> /* for color.ui */
> if (diff_use_color_default == -1)
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index e84e822..5b1f8e1 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
>
> test_debug 'git log --graph --decorate --oneline --all'
>
> +test_expect_success 'merge c0 with c1 (default no-ff)' '
> + git reset --hard c0 &&
> + test_might_fail git config --unset branch.master.mergeoptions &&
> + 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'
> +
> +test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '
> + git reset --hard c0 &&
> + test_might_fail git config --remove-section branch.master &&
> + 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_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '
> + git reset --hard c0 &&
> + test_might_fail git config --remove-section branch.master &&
> + git config branch.master.mergeoptions "--ff" &&
> + git config "branch.*.mergeoptions" "--no-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'
> +
> test_expect_success 'combining --squash and --no-ff is refused' '
> test_must_fail git merge --squash --no-ff c1 &&
> test_must_fail git merge --no-ff --squash c1
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4] Add default merge options for all branches
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-03 22:03 ` Junio C Hamano
2 siblings, 1 reply; 46+ messages in thread
From: Jonathan Nieder @ 2011-05-03 20:44 UTC (permalink / raw)
To: Michael Grubb; +Cc: git, Junio C Hamano, vmiklos
Michael Grubb wrote:
> In order to facilitate future features centered around this new
> "globlike" syntax a new struct has been created to keep track of the
> branch.*.* options. Currently it only supports branch.*.mergeoptions,
> but can be easily modified in the future to support other branch
> specific options as well. The mechanism will "vote" on specific-"ness"
> of the configuration key and ultimately only use the most specific
> options.
My immediate reactions:
- Is this really needed? If we need some priority field when other
branch globs are being supported, can't we add it then?
- Floating point? *turns to flee*
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -307,6 +307,9 @@ branch.<name>.mergeoptions::
> Sets default options for merging into branch <name>. The syntax and
> supported options are the same as those of 'git merge', but option
> values containing whitespace characters are currently not supported.
> + The special value '*' for <name> may be used to configure default
> + options for all branches. Values for specific branch names will
> + override the this default.
In what sense are they overridden? For example, if I write
[branch "*"]
mergeoptions = --no-ff
[branch "master"]
mergeoptions = --log=5
and merge another branch into master, will the effect be as though I
wrote --no-ff --log=5 or just --log=5?
I'm starting to suspect it might be simpler to add a new "[merge] no-ff"
configuration item, like the existing "[merge] log".
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -32,6 +32,21 @@
> #define NO_FAST_FORWARD (1<<2)
> #define NO_TRIVIAL (1<<3)
>
> +/* This is for branch.<foo>. blocks
> + * the vote member holds a value between
> + * 0.0 and 1.0 which measures how closely
> + * a branch name matches the key member.
> + * where branch.*.mergeoptions would be 0.1 and
> + * branch.<name>.mergeoptions would be 1.0
> + * Also it is called vote because I couldn't come
> + * up with a better name.
> + */
Style: comments should look like this:
/*
* Explanation comes here, with each line being
* approximately the same length as the next one.
*/
"It is called vote because I couldn't come up with a better name" does
not belong in a comment unless you are asking the reader to fix it.
In that case, I would write something like
float vote; /* NEEDSWORK: give this a better name */
or
float priority;
> +struct merge_options_cb {
> + char *key;
> + char *value;
> + float vote;
> +};
Since I didn't read the comment, I'm not sure what these represent.
Who is responsible for freeing them? Is the key a glob?
> @@ -503,28 +518,60 @@ cleanup:
> strbuf_release(&bname);
> }
>
> -static int git_merge_config(const char *k, const char *v, void *cb)
> +static void parse_git_merge_options(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 **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));
> - 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);
> + struct merge_options_cb *merge_options = cb;
> + int changed = 0;
> +
> + /* We only handle mergeoptions for now */
> + if (suffixcmp(k, ".mergeoptions"))
> + return;
The function is called parse_git_merge_options; I wonder if this "for
now" is an old comment from when it had a different name.
> +
> + if (!prefixcmp(k, "branch.*") && merge_options->vote <= 0.1 ) {
> + merge_options->vote = 0.1;
> + changed = 1;
Style: parens. What happens if I use
[branch "*aster"]
mergeoptions = ...
or
[branch "*.c"]
mergeoptions = ...
? Where does 0.1 come from?
> + } else if (branch && !prefixcmp(k, "branch.") &&
> + !prefixcmp(k + 7, branch) &&
> + merge_options->vote < 1.0) {
> + merge_options->vote = 1.0;
> + changed = 1;
What does changed mean?
> }
>
> + if (changed) {
> + merge_options->key = (char *)k;
> + merge_options->value = (char *)v;
Why not make the struct fields const?
> + }
> +}
> +
> +static void apply_merge_options(struct merge_options_cb *opts)
> +{
> + const char **argv;
> + int argc;
> + char *buf;
> +
> + if ( opts == NULL )
Style: parens. It would be more idiomatic to say
if (!opts)
> + return;
> +
> + buf = xstrdup(opts->value);
> + argc = split_cmdline(buf, &argv);
> + if (argc < 0)
> + die(_("Bad %s string: %s"),
> + opts->key, 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);
> +}
Idea: the series could be made easier to read if splitting this off
into its own function were a separate patch. Anyway, splitting it out
was a good idea; thanks for doing that.
It seems you changed the whitespace while unindenting? The whitespace
on the continuation line for parse_options(...) arguments is
especially confusing (I suspect you are using a tabwidth of 6, but
that doesn't make sense, either, since opts->key is not an argument to
_. The official rendering uses a tabwidth of 8).
> +
> +static int git_merge_config(const char *k, const char *v, void *cb)
> +{
> + if (cb != NULL && branch && !prefixcmp(k, "branch."))
> + parse_git_merge_options(k, v, cb);
More idiomatic to say
if (!cb && branch && !prefixcmp(k, "branch."))
...
Changing behavior when cb is NULL seems odd to me. Would it ever
actually be NULL?
> @@ -1004,7 +1052,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> if (is_null_sha1(head))
> head_invalid = 1;
>
> - git_config(git_merge_config, NULL);
> + git_config(git_merge_config, &merge_options);
> + if (merge_options.key != NULL && merge_options.value != NULL)
> + apply_merge_options(&merge_options);
More idiomatic to say
if (merge_options.key && merge_options.value)
But this seems odd, too --- when would "key" be non-null without
"value" being so?
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
[...]
> +test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '
> + git reset --hard c0 &&
> + test_might_fail git config --remove-section branch.master &&
> + 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_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '
> + git reset --hard c0 &&
> + test_might_fail git config --remove-section branch.master &&
> + git config branch.master.mergeoptions "--ff" &&
> + git config "branch.*.mergeoptions" "--no-ff" &&
> + test_tick &&
> + git merge c1 &&
Thanks. This might pass by mistake with a "git config" implementation
that sorts its entries when writing them; since I think git is
intended to allow users writing to the config file directly, too,
maybe something along the lines of
cat >>.git/config <<-\EOF &&
[branch "master"]
mergeoptions = --ff
[branch "*"]
mergeoptions = --no-ff
EOF
could be interesting.
Also --ff and --no-ff do not have orthogonal effect. What happens
when the merge options do (e.g., --ff and --log)?
> + git config --remove-section "branch.*" &&
> + verify_merge file result.1 &&
> + verify_parents "$c0"
> +'
Thanks again.
Hope that helps,
Jonathan
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4] Add default merge options for all branches
2011-05-03 20:44 ` Jonathan Nieder
@ 2011-05-03 22:51 ` Junio C Hamano
2011-05-04 4:25 ` Junio C Hamano
0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2011-05-03 22:51 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Michael Grubb, git, vmiklos
Jonathan Nieder <jrnieder@gmail.com> writes:
> In what sense are they overridden? For example, if I write
>
> [branch "*"]
> mergeoptions = --no-ff
>
> [branch "master"]
> mergeoptions = --log=5
>
> and merge another branch into master, will the effect be as though I
> wrote --no-ff --log=5 or just --log=5?
I think the latter is overriding, and the former is cumulative.
> I'm starting to suspect it might be simpler to add a new "[merge] no-ff"
> configuration item, like the existing "[merge] log".
Surely
[merge]
log = false
ff = false
would be a lot simpler and probably far easier to explain.
Does
[merge]
log = false
[branch "master"]
mergeoptions = --log=5
do the right thing with the current codebase?
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4] Add default merge options for all branches
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 10:58 ` John Szakmeister
0 siblings, 2 replies; 46+ messages in thread
From: Junio C Hamano @ 2011-05-04 4:25 UTC (permalink / raw)
To: Jonathan Nieder, Michael Grubb; +Cc: git, vmiklos
Junio C Hamano <gitster@pobox.com> writes:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> I'm starting to suspect it might be simpler to add a new "[merge] no-ff"
>> configuration item, like the existing "[merge] log".
>
> Surely
>
> [merge]
> log = false
> ff = false
>
> would be a lot simpler and probably far easier to explain.
Yes, it is far simpler and easier to explain. I'll leave the tests and
the commit log message to people who are more interested in this topic
than I am ;-)
Documentation/merge-config.txt | 6 ++++++
builtin/merge.c | 3 +++
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 8920258..2aa4408 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -16,6 +16,12 @@ merge.defaultToUpstream::
to their corresponding remote tracking branches, and the tips of
these tracking branches are merged.
+merge.ff::
+ Do not generate a merge commit if the merge resolved as a
+ fast-forward; only update the branch pointer instead. Setting
+ this to `false` would be equivalent to giving `--no-ff` from
+ the command line.
+
merge.log::
In addition to branch names, populate the log message with at
most the specified number of one-line descriptions from the
diff --git a/builtin/merge.c b/builtin/merge.c
index d171c63..5194f04 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -541,6 +541,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
if (is_bool && shortlog_len)
shortlog_len = DEFAULT_MERGE_LOG_LEN;
return 0;
+ } else if (!strcmp(k, "merge.ff")) {
+ allow_fast_forward = git_config_bool(k, v);
+ return 0;
} else if (!strcmp(k, "merge.defaulttoupstream")) {
default_to_upstream = git_config_bool(k, v);
return 0;
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4] Add default merge options for all branches
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 10:58 ` John Szakmeister
1 sibling, 1 reply; 46+ messages in thread
From: Michael Grubb @ 2011-05-04 4:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, vmiklos
I take this to mean that my patch is no longer needed/wanted?
On 5/3/11 11:25 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>> I'm starting to suspect it might be simpler to add a new "[merge] no-ff"
>>> configuration item, like the existing "[merge] log".
>>
>> Surely
>>
>> [merge]
>> log = false
>> ff = false
>>
>> would be a lot simpler and probably far easier to explain.
>
> Yes, it is far simpler and easier to explain. I'll leave the tests and
> the commit log message to people who are more interested in this topic
> than I am ;-)
>
> Documentation/merge-config.txt | 6 ++++++
> builtin/merge.c | 3 +++
> 2 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
> index 8920258..2aa4408 100644
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -16,6 +16,12 @@ merge.defaultToUpstream::
> to their corresponding remote tracking branches, and the tips of
> these tracking branches are merged.
>
> +merge.ff::
> + Do not generate a merge commit if the merge resolved as a
> + fast-forward; only update the branch pointer instead. Setting
> + this to `false` would be equivalent to giving `--no-ff` from
> + the command line.
> +
> merge.log::
> In addition to branch names, populate the log message with at
> most the specified number of one-line descriptions from the
> diff --git a/builtin/merge.c b/builtin/merge.c
> index d171c63..5194f04 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -541,6 +541,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
> if (is_bool && shortlog_len)
> shortlog_len = DEFAULT_MERGE_LOG_LEN;
> return 0;
> + } else if (!strcmp(k, "merge.ff")) {
> + allow_fast_forward = git_config_bool(k, v);
> + return 0;
> } else if (!strcmp(k, "merge.defaulttoupstream")) {
> default_to_upstream = git_config_bool(k, v);
> return 0;
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4] Add default merge options for all branches
2011-05-04 4:28 ` Michael Grubb
@ 2011-05-04 4:58 ` Jonathan Nieder
2011-05-04 18:58 ` Michael Grubb
0 siblings, 1 reply; 46+ messages in thread
From: Jonathan Nieder @ 2011-05-04 4:58 UTC (permalink / raw)
To: Michael Grubb; +Cc: Junio C Hamano, git, vmiklos
Michael Grubb wrote:
> I take this to mean that my patch is no longer needed/wanted?
No, as usual it means that Junio was thinking and sent us his
thoughts.
Do you like it? If so, the way to move it forward would be to add
tests and write a commit log message. If not, the way to move things
forward would be to explain what use case it misses and work on a
better patch that takes care of them.
Hope that helps,
Jonathan
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] Add default merge options for all branches
2011-05-04 4:58 ` Jonathan Nieder
@ 2011-05-04 18:58 ` Michael Grubb
2011-05-04 21:35 ` Junio C Hamano
0 siblings, 1 reply; 46+ messages in thread
From: Michael Grubb @ 2011-05-04 18:58 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git, vmiklos
Well, it certainly serves *my* immediate needs and addresses the specific use case that I was originally working on. Though I think that what we've come up with would benefit the codebase in general if for no other reason than it lays some ground work for future features and is a bit more generic. I also wouldn't mind going a step further and working on the globbing feature in a separate series.
Here are some corrections to the previous patch:
This patch makes the branch specific options more flexible by laying
the ground work for supporting globs for the branch names.
At this time only the value '*' is supported which will set default
values for all branches. In the future this may be expanded to support
true globbing.
Signed-off-by: Michael Grubb <devel@dailyvoid.com>
---
builtin/merge.c | 120 ++++++++++++++++++++++++++++++++++-------------------
t/t7600-merge.sh | 27 ++++++++----
2 files changed, 95 insertions(+), 52 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index fa56205..9e6ec5f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -31,20 +31,12 @@
#define DEFAULT_OCTOPUS (1<<1)
#define NO_FAST_FORWARD (1<<2)
#define NO_TRIVIAL (1<<3)
+#define MERGE_OPTIONS_INIT {NULL, NULL, NULL}
-/* This is for branch.<foo>. blocks
- * the vote member holds a value between
- * 0.0 and 1.0 which measures how closely
- * a branch name matches the key member.
- * where branch.*.mergeoptions would be 0.1 and
- * branch.<name>.mergeoptions would be 1.0
- * Also it is called vote because I couldn't come
- * up with a better name.
- */
struct merge_options_cb {
- char *key;
- char *value;
- float vote;
+ char *option;
+ char *spec;
+ char *name;
};
struct strategy {
@@ -518,30 +510,11 @@ cleanup:
strbuf_release(&bname);
}
-static void parse_git_merge_options(const char *k, const char *v,
- void *cb)
+static void free_merge_options_cb(struct merge_options_cb *data)
{
- struct merge_options_cb *merge_options = cb;
- int changed = 0;
-
- /* We only handle mergeoptions for now */
- if (suffixcmp(k, ".mergeoptions"))
- return;
-
- if (!prefixcmp(k, "branch.*") && merge_options->vote <= 0.1 ) {
- merge_options->vote = 0.1;
- changed = 1;
- } else if (branch && !prefixcmp(k, "branch.") &&
- !prefixcmp(k + 7, branch) &&
- merge_options->vote < 1.0) {
- merge_options->vote = 1.0;
- changed = 1;
- }
-
- if (changed) {
- merge_options->key = (char *)k;
- merge_options->value = (char *)v;
- }
+ free(data->option);
+ free(data->spec);
+ free(data->name);
}
static void apply_merge_options(struct merge_options_cb *opts)
@@ -550,26 +523,85 @@ static void apply_merge_options(struct merge_options_cb *opts)
int argc;
char *buf;
- if ( opts == NULL )
+ if (!opts)
return;
- buf = xstrdup(opts->value);
+ buf = xstrdup(opts->option);
argc = split_cmdline(buf, &argv);
if (argc < 0)
- die(_("Bad %s string: %s"),
- opts->key, split_cmdline_strerror(argc));
+ die(_("Bad branch.%s.%s string: %s"),
+ opts->spec, opts->name, 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);
+ builtin_merge_usage, 0);
free(buf);
}
+/*
+ * This function returns -1 if the first argument is less specific
+ * than the second, and 1 vice versa. Returns -1 if both arguments
+ * are the same.
+ */
+static int cmp_specificity(const char *a, const char *b)
+{
+ switch((!strcmp(a, "*") ? 2 : 0) |
+ (!strcmp(b, "*") ? 1 : 0)) {
+ case 3:
+ case 0:
+ return -1; /* later one wins if they are the same */
+ case 1:
+ return 1;
+ case 2:
+ return -1;
+ }
+ return 0; /* should never happen */
+}
+
+static void parse_git_merge_options(const char *k, const char *v,
+ void *cb)
+{
+ struct merge_options_cb *merge_options = cb;
+ char *spec, *eon; /* end-of-name */
+
+ k += 7; /* not interested in the leading "branch." */
+ eon = strrchr(k, '.');
+ if (!eon || strcmp(eon, ".mergeoptions"))
+ return;
+
+ /* k through eon is name or glob */
+ spec = xmemdupz(k, eon - k);
+ /*
+ * NEEDSWORK: for now we say "*" matches; we would need
+ * to turn the following into something like:
+ * if (has_wildcard(spec)
+ * ? !glob_matches(spec, branch)
+ * : strcmp(spec, branch)) {
+ * free(spec);
+ * return;
+ * }
+ */
+ if (strcmp(spec, "*") && strcmp(spec, branch)) {
+ free(spec);
+ return;
+ }
+
+ if (!merge_options->option ||
+ cmp_specificity(merge_options->spec, spec) < 0) {
+ free_merge_options_cb(merge_options);
+ merge_options->option = xstrdup(v);
+ merge_options->name = xstrdup(eon + 1);
+ merge_options->spec = spec;
+ return;
+ }
+ free(spec);
+}
+
static int git_merge_config(const char *k, const char *v, void *cb)
{
- if (cb != NULL && branch && !prefixcmp(k, "branch."))
+ if (branch && !prefixcmp(k, "branch."))
parse_git_merge_options(k, v, cb);
if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
@@ -1034,7 +1066,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
const char *head_arg;
int flag, head_invalid = 0, i;
int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
- struct merge_options_cb merge_options = {NULL, NULL, 0.0};
+ struct merge_options_cb merge_options = MERGE_OPTIONS_INIT;
struct commit_list *common = NULL;
const char *best_strategy = NULL, *wt_strategy = NULL;
struct commit_list **remotes = &remoteheads;
@@ -1053,8 +1085,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
head_invalid = 1;
git_config(git_merge_config, &merge_options);
- if (merge_options.key != NULL && merge_options.value != NULL)
+ if (merge_options.option) {
apply_merge_options(&merge_options);
+ free_merge_options_cb(&merge_options);
+ }
/* for color.ui */
if (diff_use_color_default == -1)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 5b1f8e1..d7a60b2 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -417,37 +417,46 @@ test_debug 'git log --graph --decorate --oneline --all'
test_expect_success 'merge c0 with c1 (default no-ff)' '
git reset --hard c0 &&
- test_might_fail git config --unset branch.master.mergeoptions &&
- git config "branch.*.mergeoptions" "--no-ff" &&
+ cat >.git/config <<-\EOF &&
+ [branch "*"]
+ mergeoptions = --no-ff
+ EOF
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'
test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '
git reset --hard c0 &&
- test_might_fail git config --remove-section branch.master &&
- git config "branch.*.mergeoptions" "--no-ff" &&
- git config branch.master.mergeoptions "--ff" &&
+ cat >.git/config <<-\EOF &&
+ [branch "*"]
+ mergeoptions = --no-ff
+ [branch "master"]
+ mergeoptions = --ff
+ EOF
test_tick &&
git merge c1 &&
git config --remove-section "branch.*" &&
+ git config --remove-section "branch.master" &&
verify_merge file result.1 &&
verify_parents "$c0"
'
test_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '
git reset --hard c0 &&
- test_might_fail git config --remove-section branch.master &&
- git config branch.master.mergeoptions "--ff" &&
- git config "branch.*.mergeoptions" "--no-ff" &&
+ cat >.git/config <<-\EOF &&
+ [branch "master"]
+ mergeoptions = --ff
+ [branch "*"]
+ mergeoptions = --no-ff
+ EOF
test_tick &&
git merge c1 &&
git config --remove-section "branch.*" &&
+ git config --remove-section "branch.master" &&
verify_merge file result.1 &&
verify_parents "$c0"
'
--
1.7.5
On 5/3/11 11:58 PM, Jonathan Nieder wrote:
> Michael Grubb wrote:
>
>> I take this to mean that my patch is no longer needed/wanted?
>
> No, as usual it means that Junio was thinking and sent us his
> thoughts.
>
> Do you like it? If so, the way to move it forward would be to add
> tests and write a commit log message. If not, the way to move things
> forward would be to explain what use case it misses and work on a
> better patch that takes care of them.
>
> Hope that helps,
> Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4] Add default merge options for all branches
2011-05-04 18:58 ` Michael Grubb
@ 2011-05-04 21:35 ` Junio C Hamano
0 siblings, 0 replies; 46+ messages in thread
From: Junio C Hamano @ 2011-05-04 21:35 UTC (permalink / raw)
To: Michael Grubb; +Cc: Jonathan Nieder, git, vmiklos
Michael Grubb <devel@dailyvoid.com> writes:
> Well, it certainly serves *my* immediate needs and addresses the
> specific use case that I was originally working on. Though I think that
> what we've come up with would benefit the codebase in general if for no
> other reason than it lays some ground work for future features...
The discussion was fruitful, I think, and it may have laid the groundwork
at the _conceptual level_. I however do not think that the approach the
patch takes is generic enough, even if we add globbing of the branch name,
to build on other things that can come out of the future directions the
discussion suggested.
For example, why does "merge.c" hold the parser for "branch.*.*", when
there are a lot more configuration variables that apply per branch that
are totally unrelated to merge? Don't these branch.*.* variables benefit
from having a wildcard support as well? If we wanted to add such support,
which configuration parser should be called by many pgorams that deal with
branches (e.g. "checkout -b", "branch", "fetch", ...), and where that
parser should be defined in? I suspect the answer might be "branch.c",
but I do not htink we explored the uses widely enough to make that
decision yet.
Even if I limit the discussion to "mergeoptions" [*1*], why can I specify
the merge options based on what branch I am currently on, but not based on
what branch I am merging to my current branch? When on master branch,
should branch.*.mo augument what I have in branch.master.mo, or should it
overwrite it? I may want to say "all my merges should use --log" and
"when on master I want --no-ff". Should branch.master.mo repeat "--log",
or should the values on the two variables automatically combine? If so in
what order? Am I allowed to say "Use 5 lines of --log" for generic one,
and then "Use 10 more lines of --log than generic" for branch.master.mo,
so that later I can easily change my mind and update branch.*.mo to use 10
lines, and I get automatically 20 lines for the master branch? If not why
not?
etc.etc.etc....
I am not saying some of these issues are unsolvable, nor we should solve
all these problems right now, but I do not think these issues are
something you are trying to solve with this patch, nor the approach this
patch happens to use was designed with these problems in mind (I certainly
didn't, when I made many suggestions I see in this round of your patch) to
become a foundation to solve them in the future.
The suggestion by Jonathan does not have such a design issue that requires
us to open a huge can of worms right now and potentially result in a
solution that is overengineered to address a wrong problem [*2*].
If it solves the original issue without such downsides, that would be more
preferrable, I think; no?
[Footnote]
*1* I personally think "branch.<name>.mergeoptions" was a mistake. If it
were separate "branch.<name>.merge-ff", "branch.<name>.merge-log", ... it
might have been more clear what the combining semantics should be. But
that is not going to change, so I'd rather not to extend the support for
it, until we come up with something more sensible.
*2* For example, globbing to match the current branch name could become an
overengineered solution to a wrong problem, if it turns out that it is not
useful to decide things based solely on the current branch name.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] Add default merge options for all branches
2011-05-04 4:25 ` Junio C Hamano
2011-05-04 4:28 ` Michael Grubb
@ 2011-05-04 10:58 ` John Szakmeister
1 sibling, 0 replies; 46+ messages in thread
From: John Szakmeister @ 2011-05-04 10:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Michael Grubb, git, vmiklos
On Wed, May 4, 2011 at 12:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>> I'm starting to suspect it might be simpler to add a new "[merge] no-ff"
>>> configuration item, like the existing "[merge] log".
>>
>> Surely
>>
>> [merge]
>> log = false
>> ff = false
>>
>> would be a lot simpler and probably far easier to explain.
>
> Yes, it is far simpler and easier to explain. I'll leave the tests and
> the commit log message to people who are more interested in this topic
> than I am ;-)
I like the idea. I assume it can be overridden on the command line too?
-John
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4] Add default merge options for all branches
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:03 ` Junio C Hamano
2 siblings, 0 replies; 46+ messages in thread
From: Junio C Hamano @ 2011-05-03 22:03 UTC (permalink / raw)
To: Michael Grubb; +Cc: git, Jonathan Nieder, vmiklos
Michael Grubb <devel@dailyvoid.com> writes:
/*
* Our Multi-line comments begin with a line with
* slash asterisk then newline.
*/
> +/* This is for branch.<foo>. blocks
> + * the vote member holds a value between
> + * 0.0 and 1.0 which measures how closely
> + * a branch name matches the key member.
> + * where branch.*.mergeoptions would be 0.1 and
> + * branch.<name>.mergeoptions would be 1.0
> + * Also it is called vote because I couldn't come
> + * up with a better name.
> + */
How about simply dropping that "vote" thing? I do not want to see
unnecessary float creeping into our codebase.
The k and v parameters are volatile from the point of view of this
function. You need to xstrdup() them to keep a copy.
There is no need to store "branch." part in cb->key, as it is common
across the variables.
The logic would probably look like this:
if (prefixcmp(k, "branch."))
return;
k += 7; /* past "branch." part */
eon = strrchr(k, '.'); /* end-of-name 8/
if (!eon || strcmp(eon, ".mergeoptions"))
return;
/* k thru eon is the name or wildcard */
spec = xmemdupz(k, eon - k);
/*
* NEEDSWORK: for now we say "*" matches; we would need
* to turn the following into something like:
* if (has_wildcard(spec)
* ? !glob_matches(spec, branch)
* : strcmp(spec, branch)) {
* free(spec);
* return;
* }
*/
if (strcmp(spec, "*") && strcmp(spec, branch)) {
free(spec);
return;
}
if (!merge_options->option ||
cmp_specificity(merge_options->spec, spec) < 0) {
/* use this one */
free(merge_options->spec);
free(merge_options->option);
merge_options->option = xstrdup(v);
merge_options->spec = spec;
return;
}
free(spec);
And then cmp_specificity() would say something like:
static int cmp_specificity(const char *a, const char *b)
{
switch ((!strcmp(a, "*") ? 2 : 0) |
(!strcmp(b, "*") ? 1 : 0)) {
case 3:
/*
* NEEDSWORK: when we start truly globbing,
* we need to decide "foo/*" is more specific than
* "*" and the like. But for now we do not have to
* worry about that case.
*/
case 0:
return -1; /* later one wins if they are the same */
case 1:
return 1;
case 2:
return -1;
}
}
meaning, the ones with wildcard are weaker than the ones without.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4.1] Add default merge options for all branches
2011-05-03 9:03 ` Jonathan Nieder
` (3 preceding siblings ...)
2011-05-03 20:07 ` [PATCH v4] " Michael Grubb
@ 2011-05-03 20:37 ` Michael Grubb
2011-05-04 22:07 ` [PATCH v5] " Michael Grubb
5 siblings, 0 replies; 46+ messages in thread
From: Michael Grubb @ 2011-05-03 20:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, vmiklos
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.
The need for this arises from the fact that there is currently not an
easy way to set merge options for all branches. Instead of having to
specify merge options for each individual branch there should be a way
to set defaults for all branches and then override a specific branch's
options.
In order to facilitate future features centered around this new
"globlike" syntax a new struct has been created to keep track of the
branch.*.* options. Currently it only supports branch.*.mergeoptions,
but can be easily modified in the future to support other branch
specific options as well. The mechanism will "vote" on specific-"ness"
of the configuration key and ultimately only use the most specific
options. This is not cumulative but overriding.
Signed-off-by: Michael Grubb <devel@dailyvoid.com>
---
Documentation/git-merge.txt | 3 +
builtin/merge.c | 90 +++++++++++++++++++++++++++++++++---------
t/t7600-merge.sh | 39 ++++++++++++++++++
3 files changed, 112 insertions(+), 20 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index e2e6aba..eaab3e4 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -307,6 +307,9 @@ branch.<name>.mergeoptions::
Sets default options for merging into branch <name>. The syntax and
supported options are the same as those of 'git merge', but option
values containing whitespace characters are currently not supported.
+ The special value '*' for <name> may be used to configure default
+ options for all branches. Values for specific branch names will
+ override the this default.
SEE ALSO
--------
diff --git a/builtin/merge.c b/builtin/merge.c
index d171c63..fa56205 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -32,6 +32,21 @@
#define NO_FAST_FORWARD (1<<2)
#define NO_TRIVIAL (1<<3)
+/* This is for branch.<foo>. blocks
+ * the vote member holds a value between
+ * 0.0 and 1.0 which measures how closely
+ * a branch name matches the key member.
+ * where branch.*.mergeoptions would be 0.1 and
+ * branch.<name>.mergeoptions would be 1.0
+ * Also it is called vote because I couldn't come
+ * up with a better name.
+ */
+struct merge_options_cb {
+ char *key;
+ char *value;
+ float vote;
+};
+
struct strategy {
const char *name;
unsigned attr;
@@ -503,28 +518,60 @@ cleanup:
strbuf_release(&bname);
}
-static int git_merge_config(const char *k, const char *v, void *cb)
+static void parse_git_merge_options(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 **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));
- 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);
+ struct merge_options_cb *merge_options = cb;
+ int changed = 0;
+
+ /* We only handle mergeoptions for now */
+ if (suffixcmp(k, ".mergeoptions"))
+ return;
+
+ if (!prefixcmp(k, "branch.*") && merge_options->vote <= 0.1 ) {
+ merge_options->vote = 0.1;
+ changed = 1;
+ } else if (branch && !prefixcmp(k, "branch.") &&
+ !prefixcmp(k + 7, branch) &&
+ merge_options->vote < 1.0) {
+ merge_options->vote = 1.0;
+ changed = 1;
}
+ if (changed) {
+ merge_options->key = (char *)k;
+ merge_options->value = (char *)v;
+ }
+}
+
+static void apply_merge_options(struct merge_options_cb *opts)
+{
+ const char **argv;
+ int argc;
+ char *buf;
+
+ if ( opts == NULL )
+ return;
+
+ buf = xstrdup(opts->value);
+ argc = split_cmdline(buf, &argv);
+ if (argc < 0)
+ die(_("Bad %s string: %s"),
+ opts->key, 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);
+}
+
+static int git_merge_config(const char *k, const char *v, void *cb)
+{
+ if (cb != NULL && branch && !prefixcmp(k, "branch."))
+ parse_git_merge_options(k, v, cb);
+
if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
show_diffstat = git_config_bool(k, v);
else if (!strcmp(k, "pull.twohead"))
@@ -987,6 +1034,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
const char *head_arg;
int flag, head_invalid = 0, i;
int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
+ struct merge_options_cb merge_options = {NULL, NULL, 0.0};
struct commit_list *common = NULL;
const char *best_strategy = NULL, *wt_strategy = NULL;
struct commit_list **remotes = &remoteheads;
@@ -1004,7 +1052,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (is_null_sha1(head))
head_invalid = 1;
- git_config(git_merge_config, NULL);
+ git_config(git_merge_config, &merge_options);
+ if (merge_options.key != NULL && merge_options.value != NULL)
+ apply_merge_options(&merge_options);
/* for color.ui */
if (diff_use_color_default == -1)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index e84e822..5b1f8e1 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -415,6 +415,45 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
test_debug 'git log --graph --decorate --oneline --all'
+test_expect_success 'merge c0 with c1 (default no-ff)' '
+ git reset --hard c0 &&
+ test_might_fail git config --unset branch.master.mergeoptions &&
+ 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'
+
+test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '
+ git reset --hard c0 &&
+ test_might_fail git config --remove-section branch.master &&
+ 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_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '
+ git reset --hard c0 &&
+ test_might_fail git config --remove-section branch.master &&
+ git config branch.master.mergeoptions "--ff" &&
+ git config "branch.*.mergeoptions" "--no-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'
+
test_expect_success 'combining --squash and --no-ff is refused' '
test_must_fail git merge --squash --no-ff c1 &&
test_must_fail git merge --no-ff --squash c1
--
1.7.5
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v5] Add default merge options for all branches
2011-05-03 9:03 ` Jonathan Nieder
` (4 preceding siblings ...)
2011-05-03 20:37 ` [PATCH v4.1] " Michael Grubb
@ 2011-05-04 22:07 ` Michael Grubb
2011-05-05 0:42 ` Junio C Hamano
5 siblings, 1 reply; 46+ messages in thread
From: Michael Grubb @ 2011-05-04 22:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder
Add merge.ff flag to support turning off fast-forward merges by default.
This flag is overridden if any branch.x.mergeoptions turn it back on.
Documentation/merge-config.txt | 6 ++++++
builtin/merge.c | 3 +++
2 files changed, 9 insertions(+), 0 deletions(-)
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Reported-by: Michael Grubb <devel@dailyvoid.com>
---
Documentation/merge-config.txt | 6 ++++++
builtin/merge.c | 3 +++
t/t7600-merge.sh | 23 +++++++++++++++++++++++
3 files changed, 32 insertions(+), 0 deletions(-)
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 8920258..2aa4408 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -16,6 +16,12 @@ merge.defaultToUpstream::
to their corresponding remote tracking branches, and the tips of
these tracking branches are merged.
+merge.ff::
+ Do not generate a merge commit if the merge resolved as a
+ fast-forward; only update the branch pointer instead. Setting
+ this to `false` would be equivalent to giving `--no-ff` from
+ the command line.
+
merge.log::
In addition to branch names, populate the log message with at
most the specified number of one-line descriptions from the
diff --git a/builtin/merge.c b/builtin/merge.c
index d171c63..5194f04 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -541,6 +541,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
if (is_bool && shortlog_len)
shortlog_len = DEFAULT_MERGE_LOG_LEN;
return 0;
+ } else if (!strcmp(k, "merge.ff")) {
+ allow_fast_forward = git_config_bool(k, v);
+ return 0;
} else if (!strcmp(k, "merge.defaulttoupstream")) {
default_to_upstream = git_config_bool(k, v);
return 0;
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index e84e822..21c25d4 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -415,6 +415,29 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
test_debug 'git log --graph --decorate --oneline --all'
+test_expect_success 'merge c0 with c1 (merge.ff=false)' '
+ git reset --hard c0 &&
+ git config merge.ff false &&
+ test_tick &&
+ git merge c1 &&
+ git config --remove-section merge &&
+ verify_merge file result.1 &&
+ verify_parents $c0 $c1
+'
+test_debug 'git log --graph --decorate --oneline --all'
+
+test_expect_success 'combine branch.master.mergeoptions with merge.ff' '
+ git reset --hard c0 &&
+ git config branch.master.mergeoptions --ff
+ git config merge.ff false
+ test_tick &&
+ git merge c1 &&
+ git config --remove-section "branch.master" &&
+ git config --remove-section "merge" &&
+ verify_merge file result.1 &&
+ verify_parents "$c0"
+'
+
test_expect_success 'combining --squash and --no-ff is refused' '
test_must_fail git merge --squash --no-ff c1 &&
test_must_fail git merge --no-ff --squash c1
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v5] Add default merge options for all branches
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
` (2 more replies)
0 siblings, 3 replies; 46+ messages in thread
From: Junio C Hamano @ 2011-05-05 0:42 UTC (permalink / raw)
To: Michael Grubb; +Cc: git, Jonathan Nieder
Thanks.
I think we still need to work on this a bit more, but I found an unrelated
and nastier issue before this patch can sanely be applied.
> +test_expect_success 'combine branch.master.mergeoptions with merge.ff' '
> + git reset --hard c0 &&
> + git config branch.master.mergeoptions --ff
> + git config merge.ff false
> + test_tick &&
> + git merge c1 &&
> + git config --remove-section "branch.master" &&
> + git config --remove-section "merge" &&
> + verify_merge file result.1 &&
> + verify_parents "$c0"
> +'
If you insert an "exit" after this test and inspect the resulting commit,
you will see that it created a merge.
Side note: I think verify_parents is buggy. It only makes sure
that the earlier parents of HEAD match the commits given, and does
not care if there actually are more parents.
In any case, your test exposed an ancient breakage ever since the
per-branch mergeoptions was introduced back when git-merge was a shell
script (aec7b36 (git-merge: add support for branch.<name>.mergeoptions,
2007-09-24).
I am not going to fix verify_parents tonight, as I have other git things
to do.
-- >8 --
Subject: [PATCH] merge: fix branch.<name>.mergeoptions
The parsing of the additional command line parameters supplied to
the branch.<name>.mergeoptions configuration variable was implemented
at the wrong stage. If any merge-related variable came after we read
branch.<name>.mergeoptions, the earlier value was overwritten.
We should first read all the merge.* configuration, override them by
reading from branch.<name>.mergeoptions and then finally read from
the command line.
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...
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin-merge.c | 39 +++++++++++++++++++++++++--------------
t/t7600-merge.sh | 32 ++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+), 14 deletions(-)
diff --git a/builtin-merge.c b/builtin-merge.c
index 3aaec7b..01389a3 100644
--- 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));
+ argc++;
+ parse_options(argc, argv, NULL, builtin_merge_options,
+ builtin_merge_usage, 0);
+ free(buf);
+}
+
static int git_merge_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 **argv;
- int argc;
- char *buf;
-
- buf = xstrdup(v);
- argc = split_cmdline(buf, &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));
- argc++;
- parse_options(argc, argv, NULL, builtin_merge_options,
- builtin_merge_usage, 0);
- free(buf);
+ free(branch_mergeoptions);
+ branch_mergeoptions = xstrdup(v);
+ return 0;
}
if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
@@ -918,6 +927,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (diff_use_color_default == -1)
diff_use_color_default = git_use_color_default;
+ if (branch_mergeoptions)
+ parse_branch_merge_options(branch_mergeoptions);
argc = parse_options(argc, argv, prefix, builtin_merge_options,
builtin_merge_usage, 0);
if (verbosity < 0)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 57f6d2b..56c653d 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -372,6 +372,38 @@ test_expect_success 'merge c1 with c2 (no-commit in config)' '
test_debug 'gitk --all'
+test_expect_success 'merge c1 with c2 (log in config)' '
+ git config branch.master.mergeoptions "" &&
+ git reset --hard c1 &&
+ git merge --log c2 &&
+ git show -s --pretty=tformat:%s%n%b >expect &&
+
+ git config branch.master.mergeoptions --log &&
+ git reset --hard c1 &&
+ git merge c2 &&
+ git show -s --pretty=tformat:%s%n%b >actual &&
+
+ test_cmp expect actual
+'
+
+test_expect_success 'merge c1 with c2 (log in config gets overridden)' '
+ (
+ git config --remove-section branch.master
+ git config --remove-section merge
+ )
+ git reset --hard c1 &&
+ git merge c2 &&
+ git show -s --pretty=tformat:%s%n%b >expect &&
+
+ git config branch.master.mergeoptions "--no-log" &&
+ git config merge.log true &&
+ git reset --hard c1 &&
+ git merge c2 &&
+ git show -s --pretty=tformat:%s%n%b >actual &&
+
+ test_cmp expect actual
+'
+
test_expect_success 'merge c1 with c2 (squash in config)' '
git reset --hard c1 &&
git config branch.master.mergeoptions "--squash" &&
--
1.7.5.284.g84c3a8
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v5] Add default merge options for all branches
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 21:32 ` [PATCH v5] Add default merge options for all branches Jonathan Nieder
2 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2011-05-06 20:36 UTC (permalink / raw)
To: Michael Grubb; +Cc: git, Jonathan Nieder
Junio C Hamano <gitster@pobox.com> writes:
> In any case, your test exposed an ancient breakage ever since the
> per-branch mergeoptions was introduced back when git-merge was a shell
> script (aec7b36 (git-merge: add support for branch.<name>.mergeoptions,
> 2007-09-24).
>
> -- >8 --
> Subject: [PATCH] merge: fix branch.<name>.mergeoptions
> ...
And then on top of that fix, we can do this.
I have a seemingly unrelated change to the existing test but that was
because it only made sure that the --ff-only option made the command fail
when it should fail, without making sure that it does not interfere when
it should succeed. A typical symptom of "showing off shiny new toy
because I am too excited" developer disease, I would guess. I didn't want
to forget to fix it.
-- >8 --
Subject: [PATCH] merge: introduce merge.ff configuration variable
This variable gives the default setting for --ff, --no-ff or --ff-only
options of "git merge" command.
Helped-by: Michael Grubb <devel@dailyvoid.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* If we were doing the command line option of "merge" from scratch today,
we probably would have done --ff, --ff=no, and --ff=only, instead of a
separate --ff-only. We could still add the latter two as a consistency
synonyms without deprecating anything, though.
Documentation/merge-config.txt | 10 +++++++++
builtin/merge.c | 9 ++++++++
t/t7600-merge.sh | 43 ++++++++++++++++++++++++++++++++++++++-
3 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 8920258..861bd6f 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -16,6 +16,16 @@ merge.defaultToUpstream::
to their corresponding remote tracking branches, and the tips of
these tracking branches are merged.
+merge.ff::
+ By default, git does not create an extra merge commit when merging
+ a commit that is a descendant of the current commit. Instead, the
+ tip of the current branch is fast-forwarded. When set to `false`,
+ this variable tells git to create an extra merge commit in such
+ a case (equivalent to giving the `--no-ff` option from the command
+ line). When set to `only`, only such fast-forward merges are
+ allowed (equivalent to giving the `--ff-only` option from the
+ command line).
+
merge.log::
In addition to branch names, populate the log message with at
most the specified number of one-line descriptions from the
diff --git a/builtin/merge.c b/builtin/merge.c
index 4fa789a..1c3ff13 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -550,6 +550,15 @@ static int git_merge_config(const char *k, const char *v, void *cb)
if (is_bool && shortlog_len)
shortlog_len = DEFAULT_MERGE_LOG_LEN;
return 0;
+ } else if (!strcmp(k, "merge.ff")) {
+ int boolval = git_config_maybe_bool(k, v);
+ if (0 <= boolval) {
+ allow_fast_forward = boolval;
+ } else if (v && !strcmp(v, "only")) {
+ allow_fast_forward = 1;
+ fast_forward_only = 1;
+ } /* do not barf on values from future versions of git */
+ return 0;
} else if (!strcmp(k, "merge.defaulttoupstream")) {
default_to_upstream = git_config_bool(k, v);
return 0;
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 5463f87..4f1d4eb 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -225,12 +225,28 @@ test_expect_success 'merge c1 with c2 and c3' '
test_debug 'git log --graph --decorate --oneline --all'
-test_expect_success 'failing merges with --ff-only' '
+test_expect_success 'merges with --ff-only' '
git reset --hard c1 &&
test_tick &&
test_must_fail git merge --ff-only c2 &&
test_must_fail git merge --ff-only c3 &&
- test_must_fail git merge --ff-only c2 c3
+ test_must_fail git merge --ff-only c2 c3 &&
+ git reset --hard c0 &&
+ git merge c3 &&
+ verify_head $c3
+'
+
+test_expect_success 'merges with merge.ff=only' '
+ git reset --hard c1 &&
+ test_tick &&
+ test_when_finished "git config --unset merge.ff" &&
+ git config merge.ff only &&
+ test_must_fail git merge c2 &&
+ test_must_fail git merge c3 &&
+ test_must_fail git merge c2 c3 &&
+ git reset --hard c0 &&
+ git merge c3 &&
+ verify_head $c3
'
test_expect_success 'merge c0 with c1 (no-commit)' '
@@ -447,6 +463,29 @@ test_expect_success 'merge c0 with c1 (no-ff)' '
test_debug 'git log --graph --decorate --oneline --all'
+test_expect_success 'merge c0 with c1 (merge.ff=false)' '
+ git reset --hard c0 &&
+ git config merge.ff false &&
+ test_tick &&
+ git merge c1 &&
+ git config --remove-section merge &&
+ verify_merge file result.1 &&
+ verify_parents $c0 $c1
+'
+test_debug 'git log --graph --decorate --oneline --all'
+
+test_expect_success 'combine branch.master.mergeoptions with merge.ff' '
+ git reset --hard c0 &&
+ git config branch.master.mergeoptions --ff
+ git config merge.ff false
+ test_tick &&
+ git merge c1 &&
+ git config --remove-section "branch.master" &&
+ git config --remove-section "merge" &&
+ verify_merge file result.1 &&
+ verify_parents "$c0"
+'
+
test_expect_success 'combining --squash and --no-ff is refused' '
test_must_fail git merge --squash --no-ff c1 &&
test_must_fail git merge --no-ff --squash c1
--
1.7.5.1.268.gce5bd
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v5] Add default merge options for all branches
2011-05-06 20:36 ` Junio C Hamano
@ 2011-05-06 21:59 ` Jonathan Nieder
0 siblings, 0 replies; 46+ messages in thread
From: Jonathan Nieder @ 2011-05-06 21:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Grubb, git
Junio C Hamano wrote:
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -16,6 +16,16 @@ merge.defaultToUpstream::
> to their corresponding remote tracking branches, and the tips of
> these tracking branches are merged.
>
> +merge.ff::
[...]
> + line). When set to `only`, only such fast-forward merges are
> + allowed (equivalent to giving the `--ff-only` option from the
> + command line).
A habitual "git pull" user that uses git in a read-only fashion to get
the latest development snapshot of a project's code would probably
find this handy.
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -550,6 +550,15 @@ static int git_merge_config(const char *k, const char *v, void *cb)
> if (is_bool && shortlog_len)
> shortlog_len = DEFAULT_MERGE_LOG_LEN;
> return 0;
> + } else if (!strcmp(k, "merge.ff")) {
> + int boolval = git_config_maybe_bool(k, v);
> + if (0 <= boolval) {
> + allow_fast_forward = boolval;
> + } else if (v && !strcmp(v, "only")) {
> + allow_fast_forward = 1;
> + fast_forward_only = 1;
> + } /* do not barf on values from future versions of git */
Perhaps deserves a test, like so (meant for squashing)?
-- >8 --
Subject: tests: check git does not barf on merge.ff values for future versions of git
Maybe some day in the future we will want to support a syntax
like
[merge]
ff = branch1
ff = branch2
ff = branch3
in addition to the currently permitted "true", "false", and "only"
values. So make sure we continue to treat such configurations as
though an unknown variable had been defined rather than erroring out,
until it is time to implement such a thing, so configuration files
using such a facility can be shared between present and future git.
While at it, add a few missing && and start the "combining --squash
and --no-ff" test with a known state so we can be sure it does not
succeed or fail for the wrong reason.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
t/t7600-merge.sh | 16 ++++++++++++++--
1 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 89e0b77..1e20f2e 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -35,6 +35,7 @@ printf '%s\n' 1 2 3 4 5 6 7 8 '9 X' >file.9
printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >result.1
printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 9 >result.1-5
printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 '9 X' >result.1-5-9
+>empty
create_merge_msgs () {
echo "Merge commit 'c2'" >msg.1-5 &&
@@ -475,8 +476,8 @@ test_debug 'git log --graph --decorate --oneline --all'
test_expect_success 'combine branch.master.mergeoptions with merge.ff' '
git reset --hard c0 &&
- git config branch.master.mergeoptions --ff
- git config merge.ff false
+ git config branch.master.mergeoptions --ff &&
+ git config merge.ff false &&
test_tick &&
git merge c1 &&
git config --remove-section "branch.master" &&
@@ -485,7 +486,18 @@ test_expect_success 'combine branch.master.mergeoptions with merge.ff' '
verify_parents "$c0"
'
+test_expect_success 'tolerate unknown values for merge.ff' '
+ git reset --hard c0 &&
+ git config merge.ff something-new &&
+ test_tick &&
+ git merge c1 2>message &&
+ git config --remove-section "merge" &&
+ verify_head "$c1" &&
+ test_cmp empty message
+'
+
test_expect_success 'combining --squash and --no-ff is refused' '
+ git reset --hard c0 &&
test_must_fail git merge --squash --no-ff c1 &&
test_must_fail git merge --no-ff --squash c1
'
--
1.7.5.1
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 0/2] tests: make verify_merge check that the number of parents is right
2011-05-05 0:42 ` Junio C Hamano
2011-05-06 20:36 ` Junio C Hamano
@ 2011-05-06 20:54 ` Jonathan Nieder
2011-05-06 20:58 ` [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:32 ` [PATCH v5] Add default merge options for all branches Jonathan Nieder
2 siblings, 2 replies; 46+ messages in thread
From: Jonathan Nieder @ 2011-05-06 20:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Grubb, git
Junio C Hamano wrote:
> Side note: I think verify_parents is buggy. It only makes sure
> that the earlier parents of HEAD match the commits given, and does
> not care if there actually are more parents.
How about this?
Jonathan Nieder (2):
tests: eliminate unnecessary setup test assertions
tests: teach verify_parents to check for extra parents
t/t6010-merge-base.sh | 62 +++++++++++-----------
t/t7600-merge.sh | 135 ++++++++++++++++++++++++-------------------------
2 files changed, 98 insertions(+), 99 deletions(-)
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 1/2] tests: eliminate unnecessary setup test assertions
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 ` Jonathan Nieder
2011-05-06 21:48 ` Jeff King
2011-05-06 21:00 ` [PATCH 2/2] tests: teach verify_parents to check for extra parents Jonathan Nieder
1 sibling, 1 reply; 46+ messages in thread
From: Jonathan Nieder @ 2011-05-06 20:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Grubb, git, Jeff King
Most of git's tests write files and define shell functions and
variables that will last throughout a test script at the top of
the script, before all test assertions:
. ./test-lib.sh
VAR='some value'
export VAR
>empty
fn () {
do something
}
test_expect_success 'setup' '
... nontrivial commands go here ...
'
Two scripts use a different style with this kind of trivial code
enclosed by a test assertion; fix them. The usual style is easier to
read since there is less indentation to keep track of and no need to
worry about nested quotes; and on the other hand, because the commands
in question are trivial, it should not make the test suite any worse
at catching future bugs in git.
While at it, make some other small tweaks:
- spell function definitions with a space before () for consistency
with other scripts;
- use the self-contained command "git mktree </dev/null" in
preference to "git write-tree" which looks at the index when
writing an empty tree.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
I should have done this long ago. Sorry for the eyesore.
t/t6010-merge-base.sh | 62 +++++++++++-----------
t/t7600-merge.sh | 134 ++++++++++++++++++++++++-------------------------
2 files changed, 97 insertions(+), 99 deletions(-)
diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
index 082032e..f80bba8 100755
--- a/t/t6010-merge-base.sh
+++ b/t/t6010-merge-base.sh
@@ -8,38 +8,38 @@ test_description='Merge base and parent list computation.
. ./test-lib.sh
+M=1130000000
+Z=+0000
+
+GIT_COMMITTER_EMAIL=git@comm.iter.xz
+GIT_COMMITTER_NAME='C O Mmiter'
+GIT_AUTHOR_NAME='A U Thor'
+GIT_AUTHOR_EMAIL=git@au.thor.xz
+export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
+
+doit () {
+ OFFSET=$1 &&
+ NAME=$2 &&
+ shift 2 &&
+
+ PARENTS= &&
+ for P
+ do
+ PARENTS="${PARENTS}-p $P "
+ done &&
+
+ GIT_COMMITTER_DATE="$(($M + $OFFSET)) $Z" &&
+ GIT_AUTHOR_DATE=$GIT_COMMITTER_DATE &&
+ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE &&
+
+ commit=$(echo $NAME | git commit-tree $T $PARENTS) &&
+
+ echo $commit >.git/refs/tags/$NAME &&
+ echo $commit
+}
+
test_expect_success 'setup' '
- T=$(git write-tree) &&
-
- M=1130000000 &&
- Z=+0000 &&
-
- GIT_COMMITTER_EMAIL=git@comm.iter.xz &&
- GIT_COMMITTER_NAME="C O Mmiter" &&
- GIT_AUTHOR_NAME="A U Thor" &&
- GIT_AUTHOR_EMAIL=git@au.thor.xz &&
- export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
-
- doit() {
- OFFSET=$1 &&
- NAME=$2 &&
- shift 2 &&
-
- PARENTS= &&
- for P
- do
- PARENTS="${PARENTS}-p $P "
- done &&
-
- GIT_COMMITTER_DATE="$(($M + $OFFSET)) $Z" &&
- GIT_AUTHOR_DATE=$GIT_COMMITTER_DATE &&
- export GIT_COMMITTER_DATE GIT_AUTHOR_DATE &&
-
- commit=$(echo $NAME | git commit-tree $T $PARENTS) &&
-
- echo $commit >.git/refs/tags/$NAME &&
- echo $commit
- }
+ T=$(git mktree </dev/null)
'
test_expect_success 'set up G and H' '
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 87d5d78..c665acd 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -28,80 +28,78 @@ Testing basic merge operations/option parsing.
. ./test-lib.sh
-test_expect_success 'set up test data and helpers' '
- printf "%s\n" 1 2 3 4 5 6 7 8 9 >file &&
- printf "%s\n" "1 X" 2 3 4 5 6 7 8 9 >file.1 &&
- printf "%s\n" 1 2 3 4 "5 X" 6 7 8 9 >file.5 &&
- printf "%s\n" 1 2 3 4 5 6 7 8 "9 X" >file.9 &&
- printf "%s\n" "1 X" 2 3 4 5 6 7 8 9 >result.1 &&
- printf "%s\n" "1 X" 2 3 4 "5 X" 6 7 8 9 >result.1-5 &&
- printf "%s\n" "1 X" 2 3 4 "5 X" 6 7 8 "9 X" >result.1-5-9 &&
+printf '%s\n' 1 2 3 4 5 6 7 8 9 >file
+printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >file.1
+printf '%s\n' 1 2 3 4 '5 X' 6 7 8 9 >file.5
+printf '%s\n' 1 2 3 4 5 6 7 8 '9 X' >file.9
+printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >result.1
+printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 9 >result.1-5
+printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 '9 X' >result.1-5-9
- create_merge_msgs() {
- echo "Merge commit '\''c2'\''" >msg.1-5 &&
- echo "Merge commit '\''c2'\''; commit '\''c3'\''" >msg.1-5-9 &&
- {
- echo "Squashed commit of the following:" &&
- echo &&
- git log --no-merges ^HEAD c1
- } >squash.1 &&
- {
- echo "Squashed commit of the following:" &&
- echo &&
- git log --no-merges ^HEAD c2
- } >squash.1-5 &&
- {
- echo "Squashed commit of the following:" &&
- echo &&
- git log --no-merges ^HEAD c2 c3
- } >squash.1-5-9 &&
- echo >msg.nolog &&
- {
- echo "* commit '\''c3'\'':" &&
- echo " commit 3" &&
- echo
- } >msg.log
- } &&
+create_merge_msgs () {
+ echo "Merge commit 'c2'" >msg.1-5 &&
+ echo "Merge commit 'c2'; commit 'c3'" >msg.1-5-9 &&
+ {
+ echo "Squashed commit of the following:" &&
+ echo &&
+ git log --no-merges ^HEAD c1
+ } >squash.1 &&
+ {
+ echo "Squashed commit of the following:" &&
+ echo &&
+ git log --no-merges ^HEAD c2
+ } >squash.1-5 &&
+ {
+ echo "Squashed commit of the following:" &&
+ echo &&
+ git log --no-merges ^HEAD c2 c3
+ } >squash.1-5-9 &&
+ echo >msg.nolog &&
+ {
+ echo "* commit 'c3':" &&
+ echo " commit 3" &&
+ echo
+ } >msg.log
+}
- verify_merge() {
- test_cmp "$2" "$1" &&
- git update-index --refresh &&
- git diff --exit-code &&
- if test -n "$3"
- then
- git show -s --pretty=format:%s HEAD >msg.act &&
- test_cmp "$3" msg.act
- fi
- } &&
+verify_merge () {
+ test_cmp "$2" "$1" &&
+ git update-index --refresh &&
+ git diff --exit-code &&
+ if test -n "$3"
+ then
+ git show -s --pretty=format:%s HEAD >msg.act &&
+ test_cmp "$3" msg.act
+ fi
+}
- verify_head() {
- echo "$1" >head.expected &&
- git rev-parse HEAD >head.actual &&
- test_cmp head.expected head.actual
- } &&
+verify_head () {
+ echo "$1" >head.expected &&
+ git rev-parse HEAD >head.actual &&
+ test_cmp head.expected head.actual
+}
- verify_parents() {
- printf "%s\n" "$@" >parents.expected &&
- >parents.actual &&
- i=1 &&
- while test $i -le $#
- do
- git rev-parse HEAD^$i >>parents.actual &&
- i=$(expr $i + 1) ||
- return 1
- done &&
- test_cmp parents.expected parents.actual
- } &&
+verify_parents () {
+ printf '%s\n' "$@" >parents.expected &&
+ >parents.actual &&
+ i=1 &&
+ while test $i -le $#
+ do
+ git rev-parse HEAD^$i >>parents.actual &&
+ i=$(expr $i + 1) ||
+ return 1
+ done &&
+ test_cmp parents.expected parents.actual
+}
- verify_mergeheads() {
- printf "%s\n" "$@" >mergehead.expected &&
- test_cmp mergehead.expected .git/MERGE_HEAD
- } &&
+verify_mergeheads () {
+ printf '%s\n' "$@" >mergehead.expected &&
+ test_cmp mergehead.expected .git/MERGE_HEAD
+}
- verify_no_mergehead() {
- ! test -e .git/MERGE_HEAD
- }
-'
+verify_no_mergehead () {
+ ! test -e .git/MERGE_HEAD
+}
test_expect_success 'setup' '
git add file &&
--
1.7.5.1
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions
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:26 ` [PATCH 1/2] tests: eliminate unnecessary setup test assertions Jonathan Nieder
0 siblings, 2 replies; 46+ messages in thread
From: Jeff King @ 2011-05-06 21:48 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Michael Grubb, git
On Fri, May 06, 2011 at 03:58:52PM -0500, Jonathan Nieder wrote:
> Two scripts use a different style with this kind of trivial code
> enclosed by a test assertion; fix them. The usual style is easier to
> read since there is less indentation to keep track of and no need to
> worry about nested quotes; and on the other hand, because the commands
> in question are trivial, it should not make the test suite any worse
> at catching future bugs in git.
Thanks. Glancing at the first few hunks, this change didn't seem like a
big cleanup, but then I got to the hunk with all the ugly '\'' bits. :)
The patch looks correct to me (reviewing with "git diff -b" was a big
help).
> While at it, make some other small tweaks:
>
> - spell function definitions with a space before () for consistency
> with other scripts;
Hmm.
$ cd t
$ git grep ' ()' *.sh | wc -l
271
$ git grep '[^ ]()' *.sh | wc -l
247
I'm not sure you are making things any more consistent. But I don't
really care either way.
Just for giggles, I was curious who introduced the styles:
pattern_authors() {
git grep -n "$@" |
while IFS=: read file line match; do
git blame -L "$line,$line" "$file"
done |
perl -lpe '/\((.*?) \d+-\d+-\d+/; $_=$1'
}
$ pattern_authors ' ()' t/*.sh | sort | uniq -c | sort -rn
84 Junio C Hamano
32 Jonathan Nieder
16 Thomas Rast
16 Johan Herland
12 Johannes Sixt
12 Johannes Schindelin
...
$ pattern_authors '[^ ]()' t/*.sh | sort | uniq -c | sort -rn
52 Jeff King
26 Jonathan Nieder
18 Nguyễn Thái Ngọc Duy
16 Wincent Colaiuta
14 Jon Seymour
10 Tay Ray Chuan
...
So there are definitely particular people who prefer different styles
(and I recalled that Junio and I differed on this style point, which is
confirmed here). Interestingly, you are the only person to fall right in
the middle. I guess that means you are good at emulating surrounding
code. :)
-Peff
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions
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:26 ` [PATCH 1/2] tests: eliminate unnecessary setup test assertions Jonathan Nieder
1 sibling, 1 reply; 46+ messages in thread
From: Jeff King @ 2011-05-06 22:13 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Michael Grubb, git
On Fri, May 06, 2011 at 05:48:01PM -0400, Jeff King wrote:
> pattern_authors() {
> git grep -n "$@" |
> while IFS=: read file line match; do
> git blame -L "$line,$line" "$file"
> done |
> perl -lpe '/\((.*?) \d+-\d+-\d+/; $_=$1'
> }
Two minor complaints on git-blame; maybe somebody can point out
something clever I've missed.
1. blame's "-L" understands patterns already. So in theory I could
tell it "blame all lines that match pattern X". But I don't think
there is a way to do that (it tries looking to for _one_ range to
blame for each -L, not a set of ranges).
2. Parsing the human-readable output blame output sucks. But parsing
--porcelain is annoyingly complex for quick-and-dirty things like
this. It doesn't repeat the commit information per-line.
I guess we could have an inefficient --line-porcelain format that
breaks down ranges into single lines and repeats the commit info
for each one.
The clever among you may notice that in this particular case,
though, I could have gotten away with regular --porcelain as I
blame a single line at a time.
-Peff
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions
2011-05-06 22:13 ` Jeff King
@ 2011-05-06 22:27 ` Junio C Hamano
2011-05-06 22:29 ` Jeff King
0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2011-05-06 22:27 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, Michael Grubb, git
Jeff King <peff@peff.net> writes:
> Two minor complaints on git-blame; maybe somebody can point out
> something clever I've missed.
> 1. blame's "-L" understands patterns already.
Teaching blame to take multiple -L options has been one of many
longstanding todo item for me. Someday.
> 2. Parsing the human-readable output blame output sucks. But parsing
> --porcelain is annoyingly complex for quick-and-dirty things like
> this. It doesn't repeat the commit information per-line.
Non-repetition was quite deliberate, as the reader was expected to have
memory proportional to the number of lines in the range, but I agree it is
not friendly for quick and dirty hack.
You should be able to add a command line option that disables the early
return at the beginning of emit_one_suspect_detail() with a 5-6 lines of
patch.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions
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
0 siblings, 2 replies; 46+ messages in thread
From: Jeff King @ 2011-05-06 22:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Michael Grubb, git
On Fri, May 06, 2011 at 03:27:08PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Two minor complaints on git-blame; maybe somebody can point out
> > something clever I've missed.
>
> > 1. blame's "-L" understands patterns already.
>
> Teaching blame to take multiple -L options has been one of many
> longstanding todo item for me. Someday.
I think multiple -L is not quite enough. I want a single "-L" that
matches every instance of a pattern, like:
-L "/ ()/,+0"
> > 2. Parsing the human-readable output blame output sucks. But parsing
> > --porcelain is annoyingly complex for quick-and-dirty things like
> > this. It doesn't repeat the commit information per-line.
>
> Non-repetition was quite deliberate, as the reader was expected to have
> memory proportional to the number of lines in the range, but I agree it is
> not friendly for quick and dirty hack.
>
> You should be able to add a command line option that disables the early
> return at the beginning of emit_one_suspect_detail() with a 5-6 lines of
> patch.
I tried that, and it is slightly more involved. You also need to break a
multi-line run of lines that blame to a single suspect into its
constituent lines. I am 75% of the way to such a patch if you are
interested. It's not a lot of code, but it takes some refactoring of
emit_porcelain.
-Peff
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions
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
1 sibling, 0 replies; 46+ messages in thread
From: Junio C Hamano @ 2011-05-07 22:05 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, Michael Grubb, git
Jeff King <peff@peff.net> writes:
> I think multiple -L is not quite enough. I want a single "-L" that
> matches every instance of a pattern, like:
>
> -L "/ ()/,+0"
Of course I am aware that needs to be a part of the multiple -L topic;
after all I was the one who invented -L "$regexp" support ;-)
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 0/3] blame --line-porcelain
2011-05-06 22:29 ` Jeff King
2011-05-07 22:05 ` Junio C Hamano
@ 2011-05-09 13:31 ` Jeff King
2011-05-09 13:33 ` [PATCH 1/3] add tests for various blame formats Jeff King
` (2 more replies)
1 sibling, 3 replies; 46+ messages in thread
From: Jeff King @ 2011-05-09 13:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Fri, May 06, 2011 at 06:29:51PM -0400, Jeff King wrote:
> > Non-repetition was quite deliberate, as the reader was expected to have
> > memory proportional to the number of lines in the range, but I agree it is
> > not friendly for quick and dirty hack.
> >
> > You should be able to add a command line option that disables the early
> > return at the beginning of emit_one_suspect_detail() with a 5-6 lines of
> > patch.
>
> I tried that, and it is slightly more involved. You also need to break a
> multi-line run of lines that blame to a single suspect into its
> constituent lines. I am 75% of the way to such a patch if you are
> interested. It's not a lot of code, but it takes some refactoring of
> emit_porcelain.
It turned out to not be too bad. Here's the series.
[1/3]: add tests for various blame formats
[2/3]: blame: refactor porcelain output
[3/3]: blame: add --line-porcelain output format
-Peff
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH 1/3] add tests for various blame formats
2011-05-09 13:31 ` [PATCH 0/3] blame --line-porcelain Jeff King
@ 2011-05-09 13:33 ` Jeff King
2011-05-09 13:34 ` [PATCH 2/3] blame: refactor porcelain output Jeff King
2011-05-09 13:34 ` [PATCH 3/3] blame: add --line-porcelain output format Jeff King
2 siblings, 0 replies; 46+ messages in thread
From: Jeff King @ 2011-05-09 13:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
We don't seem to have any tests for "blame --porcelain".
Let's at least do a trivial test on a simple example.
Signed-off-by: Jeff King <peff@peff.net>
---
I always feel funny putting human-readable output in a test. Maybe it is
not worth including the "normal" output test below, but I assume it's
going to remain pretty stable.
t/t8008-blame-formats.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 71 insertions(+), 0 deletions(-)
create mode 100755 t/t8008-blame-formats.sh
diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
new file mode 100755
index 0000000..387d1a6
--- /dev/null
+++ b/t/t8008-blame-formats.sh
@@ -0,0 +1,71 @@
+#!/bin/sh
+
+test_description='blame output in various formats on a simple case'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ echo a >file &&
+ git add file
+ test_tick &&
+ git commit -m one &&
+ echo b >>file &&
+ echo c >>file &&
+ echo d >>file &&
+ test_tick &&
+ git commit -a -m two
+'
+
+cat >expect <<'EOF'
+^baf5e0b (A U Thor 2005-04-07 15:13:13 -0700 1) a
+8825379d (A U Thor 2005-04-07 15:14:13 -0700 2) b
+8825379d (A U Thor 2005-04-07 15:14:13 -0700 3) c
+8825379d (A U Thor 2005-04-07 15:14:13 -0700 4) d
+EOF
+test_expect_success 'normal blame output' '
+ git blame file >actual &&
+ test_cmp expect actual
+'
+
+ID1=baf5e0b3869e0b2b2beb395a3720c7b51eac94fc
+COMMIT1='author A U Thor
+author-mail <author@example.com>
+author-time 1112911993
+author-tz -0700
+committer C O Mitter
+committer-mail <committer@example.com>
+committer-time 1112911993
+committer-tz -0700
+summary one
+boundary
+filename file'
+ID2=8825379dfb8a1267b58e8e5bcf69eec838f685ec
+COMMIT2='author A U Thor
+author-mail <author@example.com>
+author-time 1112912053
+author-tz -0700
+committer C O Mitter
+committer-mail <committer@example.com>
+committer-time 1112912053
+committer-tz -0700
+summary two
+previous baf5e0b3869e0b2b2beb395a3720c7b51eac94fc file
+filename file'
+
+cat >expect <<EOF
+$ID1 1 1 1
+$COMMIT1
+ a
+$ID2 2 2 3
+$COMMIT2
+ b
+$ID2 3 3
+ c
+$ID2 4 4
+ d
+EOF
+test_expect_success 'blame --porcelain output' '
+ git blame --porcelain file >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
1.7.5.rc2.8.gc085
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 2/3] blame: refactor porcelain output
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 ` 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
2 siblings, 1 reply; 46+ messages in thread
From: Jeff King @ 2011-05-09 13:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
This is in preparation for adding more porcelain output
options. The three changes are:
1. emit_porcelain now receives the format option flags
2. emit_one_suspect_detail takes an optional "repeat"
parameter to suppress the "show only once" behavior
3. The code for emitting porcelain suspect is factored
into its own function for repeatability.
There should be no functional changes.
Signed-off-by: Jeff King <peff@peff.net>
---
I broke this out for readability. I can break each of the 3 out into a
separate patch if that helps, but it seemed excessive.
builtin/blame.c | 25 ++++++++++++++++---------
1 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 4242e4b..d74e18f 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1484,13 +1484,14 @@ static void write_filename_info(const char *path)
/*
* Porcelain/Incremental format wants to show a lot of details per
* commit. Instead of repeating this every line, emit it only once,
- * the first time each commit appears in the output.
+ * the first time each commit appears in the output (unless the
+ * user has specifically asked for us to repeat).
*/
-static int emit_one_suspect_detail(struct origin *suspect)
+static int emit_one_suspect_detail(struct origin *suspect, int repeat)
{
struct commit_info ci;
- if (suspect->commit->object.flags & METAINFO_SHOWN)
+ if (!repeat && suspect->commit->object.flags & METAINFO_SHOWN)
return 0;
suspect->commit->object.flags |= METAINFO_SHOWN;
@@ -1529,7 +1530,7 @@ static void found_guilty_entry(struct blame_entry *ent)
printf("%s %d %d %d\n",
sha1_to_hex(suspect->commit->object.sha1),
ent->s_lno + 1, ent->lno + 1, ent->num_lines);
- emit_one_suspect_detail(suspect);
+ emit_one_suspect_detail(suspect, 0);
write_filename_info(suspect->path);
maybe_flush_or_die(stdout, "stdout");
}
@@ -1619,7 +1620,15 @@ static const char *format_time(unsigned long time, const char *tz_str,
#define OUTPUT_NO_AUTHOR 0200
#define OUTPUT_SHOW_EMAIL 0400
-static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent)
+static void emit_porcelain_details(struct origin *suspect, int repeat)
+{
+ if (emit_one_suspect_detail(suspect, repeat) ||
+ (suspect->commit->object.flags & MORE_THAN_ONE_PATH))
+ write_filename_info(suspect->path);
+}
+
+static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
+ int opt)
{
int cnt;
const char *cp;
@@ -1633,9 +1642,7 @@ static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent)
ent->s_lno + 1,
ent->lno + 1,
ent->num_lines);
- if (emit_one_suspect_detail(suspect) ||
- (suspect->commit->object.flags & MORE_THAN_ONE_PATH))
- write_filename_info(suspect->path);
+ emit_porcelain_details(suspect, 0);
cp = nth_line(sb, ent->lno);
for (cnt = 0; cnt < ent->num_lines; cnt++) {
@@ -1756,7 +1763,7 @@ static void output(struct scoreboard *sb, int option)
for (ent = sb->ent; ent; ent = ent->next) {
if (option & OUTPUT_PORCELAIN)
- emit_porcelain(sb, ent);
+ emit_porcelain(sb, ent, option);
else {
emit_other(sb, ent, option);
}
--
1.7.5.rc2.8.gc085
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 2/3] blame: refactor porcelain output
2011-05-09 13:34 ` [PATCH 2/3] blame: refactor porcelain output Jeff King
@ 2011-05-09 15:39 ` Thiago Farina
0 siblings, 0 replies; 46+ messages in thread
From: Thiago Farina @ 2011-05-09 15:39 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
On Mon, May 9, 2011 at 10:34 AM, Jeff King <peff@peff.net> wrote:
> This is in preparation for adding more porcelain output
> options. The three changes are:
>
> 1. emit_porcelain now receives the format option flags
>
> 2. emit_one_suspect_detail takes an optional "repeat"
> parameter to suppress the "show only once" behavior
>
> 3. The code for emitting porcelain suspect is factored
> into its own function for repeatability.
>
> There should be no functional changes.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I broke this out for readability. I can break each of the 3 out into a
> separate patch if that helps, but it seemed excessive.
>
> builtin/blame.c | 25 ++++++++++++++++---------
> 1 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4242e4b..d74e18f 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1484,13 +1484,14 @@ static void write_filename_info(const char *path)
> /*
> * Porcelain/Incremental format wants to show a lot of details per
> * commit. Instead of repeating this every line, emit it only once,
> - * the first time each commit appears in the output.
> + * the first time each commit appears in the output (unless the
> + * user has specifically asked for us to repeat).
> */
> -static int emit_one_suspect_detail(struct origin *suspect)
> +static int emit_one_suspect_detail(struct origin *suspect, int repeat)
> {
> struct commit_info ci;
>
> - if (suspect->commit->object.flags & METAINFO_SHOWN)
> + if (!repeat && suspect->commit->object.flags & METAINFO_SHOWN)
Maybe would be worth adding parentheses here:
if (!repeat && (...))
return 0;
?
Probably is fine as is though.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 3/3] blame: add --line-porcelain output format
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 13:34 ` Jeff King
2 siblings, 0 replies; 46+ messages in thread
From: Jeff King @ 2011-05-09 13:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
This is just like --porcelain, except that we always output
the commit information for each line, not just the first
time it is referenced. This can make quick and dirty scripts
much easier to write; see the example added to the blame
documentation.
Signed-off-by: Jeff King <peff@peff.net>
---
I'm not 100% happy with the name, but I couldn't think of anything
better. Something like --verbose-porcelain works, but is a little too
vague. Suggestions welcome.
Documentation/blame-options.txt | 5 +++++
Documentation/git-blame.txt | 13 +++++++++++++
builtin/blame.c | 10 ++++++++--
t/t8008-blame-formats.sh | 19 +++++++++++++++++++
4 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 16e3c68..e76195a 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -52,6 +52,11 @@ of lines before or after the line given by <start>.
--porcelain::
Show in a format designed for machine consumption.
+--line-porcelain::
+ Show the porcelain format, but output commit information for
+ each line, not just the first time a commit is referenced.
+ Implies --porcelain.
+
--incremental::
Show the result incrementally in a format designed for
machine consumption.
diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index bb8edb4..9516914 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -105,6 +105,19 @@ The contents of the actual line is output after the above
header, prefixed by a TAB. This is to allow adding more
header elements later.
+The porcelain format generally suppresses commit information that has
+already been seen. For example, two lines that are blamed to the same
+commit will both be shown, but the details for that commit will be shown
+only once. This is more efficient, but may require more state be kept by
+the reader. The `--line-porcelain` option can be used to output full
+commit information for each line, allowing simpler (but less efficient)
+usage like:
+
+ # count the number of lines attributed to each author
+ git blame --line-porcelain file |
+ sed -n 's/^author //p' |
+ sort | uniq -c | sort -rn
+
SPECIFYING RANGES
-----------------
diff --git a/builtin/blame.c b/builtin/blame.c
index d74e18f..6c26672 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1619,6 +1619,7 @@ static const char *format_time(unsigned long time, const char *tz_str,
#define OUTPUT_SHOW_SCORE 0100
#define OUTPUT_NO_AUTHOR 0200
#define OUTPUT_SHOW_EMAIL 0400
+#define OUTPUT_LINE_PORCELAIN 01000
static void emit_porcelain_details(struct origin *suspect, int repeat)
{
@@ -1630,6 +1631,7 @@ static void emit_porcelain_details(struct origin *suspect, int repeat)
static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
int opt)
{
+ int repeat = opt & OUTPUT_LINE_PORCELAIN;
int cnt;
const char *cp;
struct origin *suspect = ent->suspect;
@@ -1642,15 +1644,18 @@ static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
ent->s_lno + 1,
ent->lno + 1,
ent->num_lines);
- emit_porcelain_details(suspect, 0);
+ emit_porcelain_details(suspect, repeat);
cp = nth_line(sb, ent->lno);
for (cnt = 0; cnt < ent->num_lines; cnt++) {
char ch;
- if (cnt)
+ if (cnt) {
printf("%s %d %d\n", hex,
ent->s_lno + 1 + cnt,
ent->lno + 1 + cnt);
+ if (repeat)
+ emit_porcelain_details(suspect, 1);
+ }
putchar('\t');
do {
ch = *cp++;
@@ -2307,6 +2312,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
OPT_BIT('f', "show-name", &output_option, "Show original filename (Default: auto)", OUTPUT_SHOW_NAME),
OPT_BIT('n', "show-number", &output_option, "Show original linenumber (Default: off)", OUTPUT_SHOW_NUMBER),
OPT_BIT('p', "porcelain", &output_option, "Show in a format designed for machine consumption", OUTPUT_PORCELAIN),
+ OPT_BIT(0, "line-porcelain", &output_option, "Show porcelain format with per-line commit information", OUTPUT_PORCELAIN|OUTPUT_LINE_PORCELAIN),
OPT_BIT('c', NULL, &output_option, "Use the same output mode as git-annotate (Default: off)", OUTPUT_ANNOTATE_COMPAT),
OPT_BIT('t', NULL, &output_option, "Show raw timestamp (Default: off)", OUTPUT_RAW_TIMESTAMP),
OPT_BIT('l', NULL, &output_option, "Show long commit SHA1 (Default: off)", OUTPUT_LONG_OBJECT_NAME),
diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
index 387d1a6..d15f8b3 100755
--- a/t/t8008-blame-formats.sh
+++ b/t/t8008-blame-formats.sh
@@ -68,4 +68,23 @@ test_expect_success 'blame --porcelain output' '
test_cmp expect actual
'
+cat >expect <<EOF
+$ID1 1 1 1
+$COMMIT1
+ a
+$ID2 2 2 3
+$COMMIT2
+ b
+$ID2 3 3
+$COMMIT2
+ c
+$ID2 4 4
+$COMMIT2
+ d
+EOF
+test_expect_success 'blame --line-porcelain output' '
+ git blame --line-porcelain file >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.5.rc2.8.gc085
^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions
2011-05-06 21:48 ` Jeff King
2011-05-06 22:13 ` Jeff King
@ 2011-05-06 22:26 ` Jonathan Nieder
1 sibling, 0 replies; 46+ messages in thread
From: Jonathan Nieder @ 2011-05-06 22:26 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Michael Grubb, git
Jeff King wrote:
> So there are definitely particular people who prefer different styles
> (and I recalled that Junio and I differed on this style point, which is
> confirmed here). Interestingly, you are the only person to fall right in
> the middle. I guess that means you are good at emulating surrounding
> code. :)
Probably my older code leaves out the space more often, and newer code
includes it. There is an odd kind of logic that can be used to
justify including or not including the space, namely:
In C, a function definition starts with an expression that looks
something like a function call, as in "double sin(double x);". So
when you want to know everything there is to know about the sine
function, you can do a "git grep -F -e 'sin('", and it will return to
you its definition and a list of callers.
The shell function definition syntax looks oddly like an old-style C
prototype "f()". But do not be misled: to duplicate the above
property familiar from C, one needs to include a space before the
parentheses, so "git grep -F -e 'f '" will return its definition and a
list of callers.
Of course the same argument works backwards: if you want the
definition without the callers, then only the spaceless syntax will
allow you to grep for 'f()'.
Unlike brace placement, this seems to be a question of style with no
right answer. :)
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 2/2] tests: teach verify_parents to check for extra parents
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:00 ` Jonathan Nieder
2011-05-06 21:34 ` Junio C Hamano
1 sibling, 1 reply; 46+ messages in thread
From: Jonathan Nieder @ 2011-05-06 21:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Grubb, git
Currently verify_parents only makes sure that the earlier parents of
HEAD match the commits given, and does not care if there are more
parents. This makes it harder than one would like to check that, for
example, parent reduction works correctly when making an octopus.
Fix it by checking that HEAD^(n+1) is not a valid commit name.
Noticed while working on a new test that was supposed to create a
fast-forward one commit ahead but actually created a merge.
Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
t/t7600-merge.sh | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index c665acd..9af748a 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -89,6 +89,7 @@ verify_parents () {
i=$(expr $i + 1) ||
return 1
done &&
+ test_must_fail git rev-parse --verify HEAD^$(($# + 1)) &&
test_cmp parents.expected parents.actual
}
--
1.7.5.1
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 2/2] tests: teach verify_parents to check for extra parents
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
0 siblings, 1 reply; 46+ messages in thread
From: Junio C Hamano @ 2011-05-06 21:34 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Michael Grubb, git
Jonathan Nieder <jrnieder@gmail.com> writes:
> Currently verify_parents only makes sure that the earlier parents of
> HEAD match the commits given, and does not care if there are more
> parents. This makes it harder than one would like to check that, for
> example, parent reduction works correctly when making an octopus.
>
> Fix it by checking that HEAD^(n+1) is not a valid commit name.
> Noticed while working on a new test that was supposed to create a
> fast-forward one commit ahead but actually created a merge.
>
> Reported-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> t/t7600-merge.sh | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index c665acd..9af748a 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -89,6 +89,7 @@ verify_parents () {
> i=$(expr $i + 1) ||
> return 1
> done &&
> + test_must_fail git rev-parse --verify HEAD^$(($# + 1)) &&
Isn't $i at this point the same as that complex $(()) line noise?
> test_cmp parents.expected parents.actual
> }
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/2] tests: teach verify_parents to check for extra parents
2011-05-06 21:34 ` Junio C Hamano
@ 2011-05-06 21:42 ` Jonathan Nieder
0 siblings, 0 replies; 46+ messages in thread
From: Jonathan Nieder @ 2011-05-06 21:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Grubb, git
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> --- a/t/t7600-merge.sh
>> +++ b/t/t7600-merge.sh
>> @@ -89,6 +89,7 @@ verify_parents () {
>> i=$(expr $i + 1) ||
>> return 1
>> done &&
>> + test_must_fail git rev-parse --verify HEAD^$(($# + 1)) &&
>
> Isn't $i at this point the same as that complex $(()) line noise?
Yes, that sounds better.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v5] Add default merge options for all branches
2011-05-05 0:42 ` Junio C Hamano
2011-05-06 20:36 ` Junio C Hamano
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 21:32 ` Jonathan Nieder
2011-05-06 22:01 ` Junio C Hamano
2 siblings, 1 reply; 46+ messages in thread
From: Jonathan Nieder @ 2011-05-06 21:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Grubb, git
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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v5] Add default merge options for all branches
2011-05-06 21:32 ` [PATCH v5] Add default merge options for all branches Jonathan Nieder
@ 2011-05-06 22:01 ` Junio C Hamano
0 siblings, 0 replies; 46+ messages in thread
From: Junio C Hamano @ 2011-05-06 22:01 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Michael Grubb, git
Jonathan Nieder <jrnieder@gmail.com> writes:
>> +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
Yes, either we stuff a fixed string "branch.*.mergeoptions" to argv[0], or
use another static to recall which variable gave us that value and use
it. The former is of course easier and less nicer.
>> + 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. ;-))
I also forgot to free argv[].
^ permalink raw reply [flat|nested] 46+ messages in thread