[PATCH] merge: default to @{upstream}

[PATCH] merge: default to @{upstream}

[Felipe Contreras]

So 'git merge' is 'git merge @{upstream}' instead of 'git merge -h';
it's better to do something useful.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/merge.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 42fff38..f23d669 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -983,9 +983,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (!allow_fast_forward && fast_forward_only)
 		die("You cannot combine --no-ff with --ff-only.");
 
-	if (!argc)
-		usage_with_options(builtin_merge_usage,
-			builtin_merge_options);
+	if (!argc) {
+		/* argv[argc] should be NULL, so we can hijack it */
+		argv[0] = "@{u}";
+		argc = 1;
+	}
 
 	/*
 	 * This could be traditional "merge <msg> HEAD <commit>..."  and
-- 
1.7.4.rc3

Re: [PATCH] merge: default to @{upstream}

[Drew Northup]

On Fri, 2011-01-28 at 18:17 +0200, Felipe Contreras wrote:
> So 'git merge' is 'git merge @{upstream}' instead of 'git merge -h';
> it's better to do something useful.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/merge.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 42fff38..f23d669 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -983,9 +983,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  	if (!allow_fast_forward && fast_forward_only)
>  		die("You cannot combine --no-ff with --ff-only.");
>  
> -	if (!argc)
> -		usage_with_options(builtin_merge_usage,
> -			builtin_merge_options);
> +	if (!argc) {
> +		/* argv[argc] should be NULL, so we can hijack it */
> +		argv[0] = "@{u}";
> +		argc = 1;
> +	}
>  
>  	/*
>  	 * This could be traditional "merge <msg> HEAD <commit>..."  and

Honestly, I'd prefer that this NOT be merged in. When I mess up the
command line I am typing I don't want some sort of hidden magic to kick
in--I want it to tell me that I did something stupid by printing out the
help message. This is standard to a large number of commands that by
default expect a certain number of operands and I don't see any good
reason why git merge should be any different.

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [PATCH] merge: default to @{upstream}

[Felipe Contreras]

On Fri, Jan 28, 2011 at 6:44 PM, Drew Northup <drew.northup@maine.edu> wrote:
> Honestly, I'd prefer that this NOT be merged in. When I mess up the
> command line I am typing I don't want some sort of hidden magic to kick
> in--I want it to tell me that I did something stupid by printing out the
> help message. This is standard to a large number of commands that by
> default expect a certain number of operands and I don't see any good
> reason why git merge should be any different.

git checkout (defaults to HEAD)
git diff (defaults to HEAD)
git fetch (defaults to origin)
git format-patch (defaults to HEAD)
git log (defaults to HEAD)
git pull (defaults to origin)
git show (defaults to HEAD)

How is this different from 'git pull'? If you are not sure about the
'git merge' command, then type 'git help merge' instead. Just like if
you are not sure about the 'git pull' command. If you type any of
these two, a merge would happen, and you can revert it easily with
'git reset --hard HEAD^'.

-- 
Felipe Contreras

Re: [PATCH] merge: default to @{upstream}

[Jonathan Nieder]

Drew Northup wrote:
> On Fri, 2011-01-28 at 18:17 +0200, Felipe Contreras wrote:

>> So 'git merge' is 'git merge @{upstream}' instead of 'git merge -h';
>> it's better to do something useful.
[...]
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
>> @@ -983,9 +983,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>>  	if (!allow_fast_forward && fast_forward_only)
>>  		die("You cannot combine --no-ff with --ff-only.");
>>  
>> -	if (!argc)
>> -		usage_with_options(builtin_merge_usage,
>> -			builtin_merge_options);
>> +	if (!argc) {
>> +		/* argv[argc] should be NULL, so we can hijack it */
>> +		argv[0] = "@{u}";
[...]
> Honestly, I'd prefer that this NOT be merged in. When I mess up the
> command line I am typing I don't want some sort of hidden magic to kick
> in--I want it to tell me that I did something stupid by printing out the
> help message.

I generally have some sympathy for that point of view (especially
given the "because we can" commit message).  In this case, can you
think of an example where one would type "git merge" without a
branchname argument and expect it to do something else?

 - Never used "git merge" before, trying it for the first time.
   In this case, I think merging from upstream is a good behavior,
   relatively consistent with "git pull".

 - In a script trying to do an octopus, in the special case of no
   extra parents.  Rough plumbing equivalent:

	set --
	git merge-recursive HEAD -- HEAD "$@"
	tree=$(git write-tree)
	git fetch . "$@" 2>/dev/null
	git fmt-merge-msg <.git/FETCH_HEAD |
	git commit-tree $tree \
		$(git merge-base --independent HEAD "$@" | sed 's/^/-p ')

   The porcelain "git fetch" is used to populate .git/FETCH_HEAD (the
   fmt-merge-msg manual doesn't explain any other way).  Maybe it
   would be better to write the log message by hand.  In any case, the
   porcelain "git fetch" defeats us --- "git fetch ." means to fetch
   the default refspec instead of no branches.

   It might be nice to have a better way to format merge messages
   (like "git fmt-merge-msg --refspec "$@"?) so we can give better
   advice to the author of the script that uses "git merge $@" and
   already breaks in the no-heads case.

 - Started typing a "git merge" command with lots of switches and
   forgot to type the branch name.  It might help to print some output
   'defaulting to branch <foo>' so the operator can notice the
   mistake.

Other nits: documentation?  tests?  The rest of cmd_merge does not
rely on argv[argc] being NULL, but it might make sense to set argv[1]
to NULL anyway for futureproofing.

Thanks.
Jonathan

Re: [PATCH] merge: default to @{upstream}

[Felipe Contreras]

On Fri, Jan 28, 2011 at 7:56 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Other nits: documentation?  tests?  The rest of cmd_merge does not
> rely on argv[argc] being NULL, but it might make sense to set argv[1]
> to NULL anyway for futureproofing.

Sure, I need to add documentation and tests. I should probably have
sent this as 'RFC'.

Anyway, I don't think we can set argv[1] to NULL, because it's
possible that this is "char *argv[1]", so that would crash. The only
thing the standard ensures, is that the last one would be NULL, so
argv[argc] = NULL, and therefore we can override it, as long as the
rest of the code checks for argc instead of NULL, which AFAIK in the
whole git code it is the case, and certainly in builtin_merge.c
AFAICS.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] merge: default to @{upstream}

[Bert Wesarg]

On Fri, Jan 28, 2011 at 17:17, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> So 'git merge' is 'git merge @{upstream}' instead of 'git merge -h';
> it's better to do something useful.

Nice idea. Could you have a look into git rebase, I think this could
be applied there too.

Anyway, I think some high level sanity check won't harm. Ie. check if
there is an upstream configured.

Thanks.
Bert

Re: [PATCH] merge: default to @{upstream}

[Martin von Zweigbergk]

On Fri, 28 Jan 2011, Bert Wesarg wrote:

> On Fri, Jan 28, 2011 at 17:17, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> > So 'git merge' is 'git merge @{upstream}' instead of 'git merge -h';
> > it's better to do something useful.
> 
> Nice idea. Could you have a look into git rebase, I think this could
> be applied there too.

I submitted an RFC patch for that a while ago [1]. I will soon send a
re-roll of some rebase refactoring patches I have been working on (I
have been busy at work and also waiting for 1.7.4 to be finished). I
will then send an updated "default upstream" patch again on top of the
refactoring patches.

And thanks for taking care of the merge case, Felipe. I'm still
struggling with the part of Git written in C, so I'm glad you took
that part.

> 
> Anyway, I think some high level sanity check won't harm. Ie. check if
> there is an upstream configured.

Will be done in the case of rebase at least (stolen from the
implementation in git pull).


[1] http://thread.gmane.org/gmane.comp.version-control.git/161382/

Re: [PATCH] merge: default to @{upstream}

[Jonathan Nieder]

Hi again,

Felipe Contreras wrote:
[out of order for convenience]

> Anyway, I don't think we can set argv[1] to NULL, because it's
> possible that this is "char *argv[1]", so that would crash.

The original argc is at least 1 (to include "merge" as argv[0]) before
the args are shifted by parse_options_end.  But probably that is too
tricky to rely on.

> Sure, I need to add documentation and tests. I should probably have
> sent this as 'RFC'.

No problem.  Thanks for keeping the topic alive.

Re: [PATCH] merge: default to @{upstream}

[Miles Bader]

Felipe Contreras <felipe.contreras@gmail.com> writes:
> So 'git merge' is 'git merge @{upstream}' instead of 'git merge -h';
> it's better to do something useful.

I totally agree -- this would be a good change[*] -- but this suggestion
has been made before, and always gets shot down for vaguely silly
reasons...

I often do "git fetch; <see what changed upstream>; git pull".  I think
this is probably not a rare pattern, when one is working with other
people via a shared upstream.

The git-pull is obviously slightly risky because another change could
have happened after the fetch, but I use that instead of "git-merge"
because git-pull's defaults make it very convenient ... and _usually_
there's no issue...  If git-merge had proper defaults (as you suggest),
it would be exactly as convenient as git-pull (and _less_ dangerous), so
I'd use that instead.

[and, no, saying "you can add an alias!" is not a reasonable answer --
git should be convenient by default!]

-Miles

-- 
Hers, pron. His.