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

From: Santi Béjar <sbejar@gmail.com>
Date: Dec 18, 2006, 11:06 UTC
Message-ID: <8aa486160612180306s38c83b17qc3fddf087ccd0460@mail.gmail.com>
In-Reply-To: <20061218091206.GA11284@spearce.org>

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).