Thanks for all the review.
Yes I agree it does brings more complexity with a small gain in feature.
So please disregard this patch (also please forgive my missing signed-off as it's my first patch), as I'm gonna move on to work on some fixes on submodules as I originally intended.
Thanks!
Tim
On Apr 3, 2011, at 3:22 PM, Jonathan Nieder wrote:
> Hi,
>
> Timothy Chen wrote:
>
>> builtin/merge.c | 57 +++++++++++++++++++++++++++++-------------------------
>> 1 files changed, 31 insertions(+), 26 deletions(-)
>
> Now for mechanics.
>
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
> [...]
>> @@ -1101,36 +1098,44 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>> remote_head = peel_to_type(argv[0], 0, NULL, OBJ_COMMIT);
>> if (!remote_head)
>> die(_("%s - not something we can merge"), argv[0]);
>> - read_empty(remote_head->sha1, 0);
>> update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
>> DIE_ON_ERR);
>> - return 0;
>> +
>> + if (argc < 2)
>> + return 0;
>
> When argc == 1, this means read_empty never gets called. Is that
> intended?
>
> It breaks 7607.13. Running "make test" is a good way to find some
> breakages.
>
>> +
>> + hashcpy(head, remote_head->sha1);
>> + read_empty(remote_head->sha1, 0);
>> + head_arg = argv[0];
>> + argc--;
>> + argv++;
>
> As always when pretending something, I think a comment would be
> helpful. Something to the effect of:
>
> /*
> * We were called as "git merge <branch1> <branch2> <branch3>...".
> *
> * Now HEAD has advanced to <branch1>, and we can pretend
> * we were called as "git merge <branch2> <branch3>...".
> */
>
> Though I think I prefer the more explicit comment I suggested last
> time[1].
>
>> + }
>> +
>> + struct strbuf merge_names = STRBUF_INIT;
>> +
>> - } else {
>> - struct strbuf merge_names = STRBUF_INIT;
>> -
>> - /* We are invoked directly as the first-class UI. */
>> + /* We are invoked directly as the first-class UI. */
>
> Won't this break
>
> git merge "message here" $(git rev-parse HEAD) foo bar
>
> ? Previously this code was in an "else" block so it wasn't reached in
> the is_old_style_invocation case.
>
>> - if (head_invalid || !argc)
>> + if (!argc)
>> usage_with_options(builtin_merge_usage,
>> builtin_merge_options);
>
> What happens with
>
> git merge "message here" HEAD foo bar
>
> from an unborn branch?
>
> Hope that helps.
> Jonathan
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/170456
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html