Re: [PATCH v2 2/2] Be more careful when determining whether a remote was configured

[Jeff King]

On Thu, Jan 19, 2017 at 01:45:29PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I'm trying to figure out why "fetch --multiple" wouldn't just take a url
> > in the first place. I guess it is because multiple fetch is useless
> > without refspecs (since otherwise you're just writing to FETCH_HEAD,
> > which gets immediately overwritten).
> 
> This is probably a tangent, if FETCH_HEAD is overwritten, wouldn't
> that be a bug in the implementation of --multiple?  I somehow
> thought we had an option to tell second and subsequent "fetch" to
> append to FETCH_HEAD instead of overwriting it.

Maybe. I was just speculating on the reason.

I just disabled that check and tried it with two local repos:

  git init
  git fetch --multiple /path/to/one /path/to/two
  cat .git/FETCH_HEAD

and indeed it does seem to append.

The logic comes from 9c4a036b3 (Teach the --all option to 'git fetch',
2009-11-09), but it does not seem to give any reasoning. Nor could I
find anything on the mailing list. 

-Peff

Re: [PATCH v2 2/2] Be more careful when determining whether a remote was configured

[Junio C Hamano]

Jeff King  writes:

> I'm trying to figure out why "fetch --multiple" wouldn't just take a url
> in the first place. I guess it is because multiple fetch is useless
> without refspecs (since otherwise you're just writing to FETCH_HEAD,
> which gets immediately overwritten).

This is probably a tangent, if FETCH_HEAD is overwritten, wouldn't
that be a bug in the implementation of --multiple?  I somehow
thought we had an option to tell second and subsequent "fetch" to
append to FETCH_HEAD instead of overwriting it.

Re: [PATCH v2 2/2] Be more careful when determining whether a remote was configured

[Johannes Schindelin]

Hi Peff,

On Thu, 19 Jan 2017, Jeff King wrote:

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index c85f3471d..9024cfffa 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1014,9 +1014,9 @@ static int add_remote_or_group(const char *name, struct 
> string_list *list)
>   git_config(get_remote_group, );
>   if (list->nr == prev_nr) {
>   struct remote *remote;
> - if (!remote_is_configured(name))
> - return 0;
>   remote = remote_get(name);
> + if (!remote->fetch_refspec_nr)
> + return 0;

Please note that it is legitimate to fetch from foreign vcs, in which case
the fetch refspecs may not be required (or even make sense).

> It's outside the scope of your patches, so I think we are OK to just
> ignore it. But if you want to pursue it, it avoids having to add the
> extra parameter to remote_is_configured().

Sure, it would avoid that. But that parameter makes semantic sense: some
callers may want to have all remotes, even those configured via the
command-line, and others really are only interested in knowing whether the
current repository config already has settings for a remote of the given
name.

> > Many thanks to Jeff King whose tireless review helped with settling for
> > nothing less than the current strategy.
> 
> Just how I wanted to be immortalized in git's commit history. ;)

You are welcome ;-)

Ciao,
Johannes

Re: [PATCH v2 2/2] Be more careful when determining whether a remote was configured

[Jeff King]

On Thu, Jan 19, 2017 at 10:20:02PM +0100, Johannes Schindelin wrote:

> There is only one caller of remote_is_configured() (in `git fetch`) that
> may want to take remotes into account even if they were configured
> outside the repository config; all other callers essentially try to
> prevent the Git command from overwriting settings in the repository
> config.
> 
> To accommodate that fact, the remote_is_configured() function now
> requires a parameter that states whether the caller is interested in all
> remotes, or only in those that were configured in the repository config.

Just to make sure I understand the issue, the problem is that:

  git config --global remote.foo.url whatever
  git fetch --multiple foo bar

would not work without this part of the patch?

I'm trying to figure out why "fetch --multiple" wouldn't just take a url
in the first place. I guess it is because multiple fetch is useless
without refspecs (since otherwise you're just writing to FETCH_HEAD,
which gets immediately overwritten). I wonder if that test really should
be:

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c85f3471d..9024cfffa 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1014,9 +1014,9 @@ static int add_remote_or_group(const char *name, struct 
string_list *list)
git_config(get_remote_group, );
if (list->nr == prev_nr) {
struct remote *remote;
-   if (!remote_is_configured(name))
-   return 0;
remote = remote_get(name);
+   if (!remote->fetch_refspec_nr)
+   return 0;
string_list_append(list, remote->name);
}
return 1;

It's outside the scope of your patches, so I think we are OK to just
ignore it. But if you want to pursue it, it avoids having to add the
extra parameter to remote_is_configured().

> Many thanks to Jeff King whose tireless review helped with settling for
> nothing less than the current strategy.

Just how I wanted to be immortalized in git's commit history. ;)

>  builtin/fetch.c   |  2 +-
>  builtin/remote.c  | 14 +++---
>  remote.c  | 12 ++--
>  remote.h  |  4 ++--
>  t/t5505-remote.sh |  2 +-
>  5 files changed, 21 insertions(+), 13 deletions(-)

Patch itself looks OK to me.

-Peff