[PATCH] Only warn about missing branch.<n>.merge in pull.

[PATCH] Only warn about missing branch.<n>.merge in pull.

[Shawn O. Pearce]

Commit 62b339a5 added a warning for git-pull to notify the user when
they have not configured the setting 'branch.<n>.merge' (where <n>
is the current branch) and no arguments were given to git-pull to
specify the branches to merge.

Unfortunately this warning also appears in git-fetch when no
arguments were supplied, as the warning is being output at the
same time that the contents of FETCH_HEAD is being determined.
This causes users who fetch into local tracking branches prior
to merging to receive unexpected/unnecessary warnings:

  $ git fetch
  Warning: No merge candidate found because value of config option
           "branch.sp/topic.merge" does not match any remote branch fetched.

This warning may also cause problems for other Porcelain that use
git-fetch as "plumbing", as the other Porcelain may not actually
use (or honor) the branch.<n>.merge configuration option.

Instead we should delay the warning about no matching branches until
we are actually in git-pull and are trying to setup the call to
git-merge to actually carry out the merge.  This way direct users
of git-fetch do not receive these warnings.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 git-parse-remote.sh |   10 ----------
 git-pull.sh         |   16 +++++++++++++++-
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index f27c3c2..7a1cf5c 100755
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -134,7 +134,6 @@ canon_refs_list_for_fetch () {
 	# or the first one otherwise; add prefix . to the rest
 	# to prevent the secondary branches to be merged by default.
 	merge_branches=
-	found_mergeref=
 	curr_branch=
 	if test "$1" = "-d"
 	then
@@ -174,10 +173,6 @@ canon_refs_list_for_fetch () {
 			    dot_prefix= && break
 			done
 		fi
-		if test -z $dot_prefix
-		then
-			found_mergeref=true
-		fi
 		case "$remote" in
 		'') remote=HEAD ;;
 		refs/heads/* | refs/tags/* | refs/remotes/*) ;;
@@ -198,11 +193,6 @@ canon_refs_list_for_fetch () {
 		fi
 		echo "${dot_prefix}${force}${remote}:${local}"
 	done
-	if test -z "$found_mergeref" -a "$curr_branch"
-	then
-		echo >&2 "Warning: No merge candidate found because value of config option
-         \"branch.${curr_branch}.merge\" does not match any remote branch fetched."
-	fi
 }
 
 # Returns list of src: (no store), or src:dst (store)
diff --git a/git-pull.sh b/git-pull.sh
index e23beb6..d43a565 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -76,7 +76,21 @@ merge_head=$(sed -e '/	not-for-merge	/d' \
 
 case "$merge_head" in
 '')
-	echo >&2 "No changes."
+	echo >&2 "warning: No branches were selected for merge."
+	if test $# = 0
+	then
+		branch=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')
+		remote=$(git-repo-config --get-all "branch.$branch.remote")
+		if test -z "$remote"
+		then
+			echo >&2 "warning: (Config option 'branch.$branch.remote' not set.)"
+		fi
+		merge=$(git-repo-config --get-all "branch.$branch.merge")
+		if test -z "$merge"
+		then
+			echo >&2 "warning: (Config option 'branch.$branch.merge' not set.)"
+		fi
+	fi
 	exit 0
 	;;
 ?*' '?*)
-- 

Re: [PATCH] Only warn about missing branch.<n>.merge in pull.

[Santi Béjar]

On 12/18/06, Shawn O. Pearce <spearce@spearce.org> wrote:
> Commit 62b339a5 added a warning for git-pull to notify the user when
> they have not configured the setting 'branch.<n>.merge' (where <n>
> is the current branch) and no arguments were given to git-pull to
> specify the branches to merge.
>
> Unfortunately this warning also appears in git-fetch when no
> arguments were supplied, as the warning is being output at the
> same time that the contents of FETCH_HEAD is being determined.
> This causes users who fetch into local tracking branches prior
> to merging to receive unexpected/unnecessary warnings:
>
>   $ git fetch
>   Warning: No merge candidate found because value of config option
>            "branch.sp/topic.merge" does not match any remote branch fetched.
>
> This warning may also cause problems for other Porcelain that use
> git-fetch as "plumbing", as the other Porcelain may not actually
> use (or honor) the branch.<n>.merge configuration option.
>
> Instead we should delay the warning about no matching branches until
> we are actually in git-pull and are trying to setup the call to
> git-merge to actually carry out the merge.  This way direct users
> of git-fetch do not receive these warnings.
>

I think it is a sensible thing to do, but:

[...]

> diff --git a/git-pull.sh b/git-pull.sh
> index e23beb6..d43a565 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -76,7 +76,21 @@ merge_head=$(sed -e '/       not-for-merge   /d' \
>
>  case "$merge_head" in
>  '')
> -       echo >&2 "No changes."
> +       echo >&2 "warning: No branches were selected for merge."
> +       if test $# = 0
> +       then
> +               branch=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')
> +               remote=$(git-repo-config --get-all "branch.$branch.remote")

There is only support for one branch.<n>.remote.

> +               if test -z "$remote"
> +               then
> +                       echo >&2 "warning: (Config option 'branch.$branch.remote' not set.)"

It is OK not to have a branch.<n>.remote, it defaults to origin.

> +               fi
> +               merge=$(git-repo-config --get-all "branch.$branch.merge")
> +               if test -z "$merge"
> +               then
> +                       echo >&2 "warning: (Config option 'branch.$branch.merge' not set.)"
> +               fi
> +       fi

I don't like the (), and it's missing the other possibility:

else
   echo >&2 "Warning: config option 'branch.$branch.merge' does not
match any remote branch fetched."

Also we could check that the number of to be merge branches equals to
the number of branch.<n>.merge (I'll do it).

Re: [PATCH] Only warn about missing branch.<n>.merge in pull.

[Josef Weidendorfer]

On Monday 18 December 2006 21:43, Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 18 Dec 2006, Junio C Hamano wrote:
> 
> > 	$ git pull second
> > ...
> >  (3) branch.$current.merge was a mistake.  It should have been
> >      branch.$current.merge.$remote.  In other words, the
> >      configuration should have been about the current branch and
> >      the remote repository pair.
> > 
> >  (4) the current configuration mechanism is fine, but the code
> >      is not.  We should forbid "the first branch listed" rule
> >      from being applied for "git pull second", and require the
> >      users to explicitly say which branch(es) to merge.

> I fetch/merge criss-crossed over my machines, so this affects me. Until 
> the recent changes, I _always_ fetched/merged with explicit remote and 
> branch. This keeps me unconfused about what I actually do.
> 
> With the options you list, I'd say (3) with (4) as a fallback is the way 
> to go.

I agree.
Despite of this, I just sent out the quick fix.

> However, I would actually reuse our versatile (often hated?) config  
> handling:
> 
> [branch "xyz"]
> 	remote = blabla # this is the default remote
> 	merge = master # this is the default branch for the default remote
> 	merge = pu for remote second # merge 'pu' if pulling from second

Looks a little bit confusing, but is fine with me.
I even would remove the need for the word "remote" in the second merge line.
Anybody using this has to look it up in the documentation, anyway.
Because these options are not really self-describing.

Josef

Re: [PATCH] Only warn about missing branch.<n>.merge in pull.

[Junio C Hamano]

Josef Weidendorfer <Josef.Weidendorfer@gmx.de> writes:

> On Monday 18 December 2006 21:43, Johannes Schindelin wrote:
>> 
>> However, I would actually reuse our versatile (often hated?) config  
>> handling:
>> 
>> [branch "xyz"]
>> 	remote = blabla # this is the default remote
>> 	merge = master # this is the default branch for the default remote
>> 	merge = pu for remote second # merge 'pu' if pulling from second
>
> Looks a little bit confusing, but is fine with me.
> I even would remove the need for the word "remote" in the second merge line.
> Anybody using this has to look it up in the documentation, anyway.
> Because these options are not really self-describing.

I actually am in favor of Johannes's one, except that it does
look like you are always making a pentapus merge of pu, for,
remote and second branches into xyz ;-).

Re: [PATCH] Only warn about missing branch.<n>.merge in pull.

[Shawn Pearce]

"Michael S. Tsirkin" <mst@mellanox.co.il> wrote:
> > Quoting r. Junio C Hamano <junkio@cox.net>:
> > I can see a few possibilities:
> > 
> >  (1) people do not interact with multiple remote repositories
> >      regularly, so this is not a problem in practice.
> > 
> >  (2) people do, but "the first branch listed" rule is good
> >      enough in practice.  Because they would always say "git
> >      pull second which-branch" instead if they want something
> >      different, this is a non-issue.
> > 
> >  (3) branch.$current.merge was a mistake.  It should have been
> >      branch.$current.merge.$remote.  In other words, the
> >      configuration should have been about the current branch and
> >      the remote repository pair.
> > 
> >  (4) the current configuration mechanism is fine, but the code
> >      is not.  We should forbid "the first branch listed" rule
> >      from being applied for "git pull second", and require the
> >      users to explicitly say which branch(es) to merge.
> > 
> > I am inclined to say that (1) is possible, (2) is implausible
> > (otherwise we would not have done 62b339a5 for the same reason),
> > (3) is confused, and probably (4) is what we need.
> 
> As a person who tracks multiple remotes in one repository,
> I would say (4) best matches what I do.
> 
> So I currently always do git fetch <remote> to download changes,
> and always use git pull . <branch> to merge a specific branch.

Agreed; #4 best matches what I (and those I work with on that ugly
repository of mine) do.

My git.git repository currently just fetches all of Junio's branches
right into my local refs/heads.  I fetch every day or so, but never
merge from next into any branch.  Though I create branches off next,
master, or some select commit based on whatever topic I'm hacking.

In my other repositories I tend to fetch before merging, for a
number of reasons:

 a) Its distributed backup; if I fetch the branch that's one more copy.

 b) I can easily view a branch's recent changes, but not affect my
    own topics until I'm ready to take them in.  People often ask
	me to look at their changes, or wonder why something is suddenly
	acting odd - looking at the commits in gitk usually tells me
	quite quickly who did what.  :-)

 c) I can easily start a new topic off any recent change.

 d) I can easily merge topics once they are local and reviewed.

a,b,d are not quite as necessary in an email-patch based group such
as git itself, as the mailing list offers most of the reasons I
fetch first.  b and c are just convience there.

-- 

Re: [PATCH] Only warn about missing branch.<n>.merge in pull.

[Johannes Schindelin]

Hi,

On Mon, 18 Dec 2006, Junio C Hamano wrote:

> Josef Weidendorfer <Josef.Weidendorfer@gmx.de> writes:
> 
> > On Monday 18 December 2006 21:43, Johannes Schindelin wrote:
> >> 
> >> However, I would actually reuse our versatile (often hated?) config  
> >> handling:
> >> 
> >> [branch "xyz"]
> >> 	remote = blabla # this is the default remote
> >> 	merge = master # this is the default branch for the default remote
> >> 	merge = pu for remote second # merge 'pu' if pulling from second
> >
> > Looks a little bit confusing, but is fine with me.
> > I even would remove the need for the word "remote" in the second merge line.
> > Anybody using this has to look it up in the documentation, anyway.
> > Because these options are not really self-describing.
> 
> I actually am in favor of Johannes's one, except that it does
> look like you are always making a pentapus merge of pu, for,
> remote and second branches into xyz ;-).


Would

	merge = pu, when pulling second

be better? ;-)

Ciao,
Dscho

Re: [PATCH] Only warn about missing branch.<n>.merge in pull.

[Johannes Schindelin]

Hi,

On Tue, 19 Dec 2006, Josef Weidendorfer wrote:

> On Monday 18 December 2006 21:43, Johannes Schindelin wrote:
>
> > However, I would actually reuse our versatile (often hated?) config  
> > handling:
> > 
> > [branch "xyz"]
> > 	remote = blabla # this is the default remote
> > 	merge = master # this is the default branch for the default remote
> > 	merge = pu for remote second # merge 'pu' if pulling from second
> 
> Looks a little bit confusing, but is fine with me.

Granted. Suggestions?

> I even would remove the need for the word "remote" in the second merge 
> line.

No. In Git, a line like

	blabla

would turn into a boolean named "blabla" being true.

Ciao,
Dscho

Re: [PATCH] Only warn about missing branch.<n>.merge in pull.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> >> 	merge = pu for remote second # merge 'pu' if pulling from second
>>
>> I actually am in favor of Johannes's one, except that it does
>> look like you are always making a pentapus merge of pu, for,
>> remote and second branches into xyz ;-).
>
>
> Would
>
> 	merge = pu, when pulling second
>
> be better? ;-)

The users can work it around by writing:

	merge pu remote second for

Just document it clearly and say "if you are going to make an
pentapus (or more) with remote branches that include ones named
'for' and 'remote', do not place them as third and second
(respectively) word from the end on the line" ;-).

Your "noise word" remote is actually useful.