threads / patch / 43521

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

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

## tl;dr

8 messages between Dec 18, 2006 and Dec 19, 2006. Diffs are folded; open one to read it.

replies: 7people: 5as markdown or json

Shawn O. Pearce· Dec 18, 2006, 09:12 UTC · lore

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

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(-)
Show changes to 2 files +15 −11

git-parse-remote.sh, git-pull.sh

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
 	;;
 ?*' '?*)
Santi Béjar· Dec 18, 2006, 11:06 UTC · re: Shawn O. Pearce · lore
On 12/18/06, Shawn O. Pearce <spearce@spearce.org> wrote:
Show 24 quoted lines
> 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:
[...]
Show 14 quoted lines
> 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.
Show 7 quoted lines
> +               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).

Josef Weidendorfer· Dec 19, 2006, 00:59 UTC · lore
On Monday 18 December 2006 21:43, Johannes Schindelin wrote:
Show 15 quoted lines
> 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.
Show 6 quoted lines
> 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.

Show 7 quoted lines
> 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
Junio C Hamano· Dec 19, 2006, 01:14 UTC · re: Josef Weidendorfer · lore
Josef Weidendorfer <Josef.Weidendorfer@gmx.de> writes:
Show 14 quoted lines
> 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 ;-).

Johannes Schindelin· Dec 19, 2006, 10:28 UTC · re: Junio C Hamano · lore
Hi,
On Mon, 18 Dec 2006, Junio C Hamano wrote:
Show 20 quoted lines
> 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

Junio C Hamano· Dec 19, 2006, 10:37 UTC · re: Johannes Schindelin · lore
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 12 quoted lines
>> >> 	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.
Johannes Schindelin· Dec 19, 2006, 10:30 UTC · re: Josef Weidendorfer · lore
Hi,
On Tue, 19 Dec 2006, Josef Weidendorfer wrote:
Show 11 quoted lines
> 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

Shawn Pearce· Dec 19, 2006, 06:54 UTC · lore
"Michael S. Tsirkin" <mst@mellanox.co.il> wrote:
Show 30 quoted lines
> > 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.

← back to recent threads