Re: [PATCH v3 5/5] remote: announce removal of "branches/" and "remotes/"

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> +repositories at all and most users aren't even aware of these mechanisms. They
> +have been deprecated for almost 20 years and 14 years respectively, and we are
> +not aware of any reason why anybody would want to use these mechanisms.

I am aware of one reason why some folks may prefer being able to say

    $ ls .git/branches/*pattern*
    $ echo "$URL#branch" >".git/branches/$shortname"
    $ git fetch $shortname

over the configuration file based mechanism, especially when they
have to deal with dozens of remotes that change the branch name to
be pulled from.  And as I already said the above while reviewing the
previous round of this series, _we_ are now aware of it.

I however am in favor of deprecating and removing the support, but
that is not because I am not aware how useful they could be.  I am
and we are aware, but we haven't heard anybody jumping up and down
to advocate for its undeprecation for a long time, and that is why
I am personally OK with this removal.

>  branches::
> -	A slightly deprecated way to store shorthands to be used
> +	A deprecated way to store shorthands to be used
>  	to specify a URL to 'git fetch', 'git pull' and 'git push'.
>  	A file can be stored as `branches/<name>` and then
>  	'name' can be given to these commands in place of
> @@ -162,7 +162,8 @@ branches::
>  	and not likely to be found in modern repositories. This
>  	directory is ignored if $GIT_COMMON_DIR is set and
>  	"$GIT_COMMON_DIR/branches" will be used instead.
> -
> ++
> +Git will stop reading remotes from this directory in Git 3.0.
>  
>  hooks::
>  	Hooks are customization scripts used by various Git
> @@ -238,6 +239,8 @@ remotes::
>  	and not likely to be found in modern repositories. This
>  	directory is ignored if $GIT_COMMON_DIR is set and
>  	"$GIT_COMMON_DIR/remotes" will be used instead.
> ++
> +Git will stop reading remotes from this directory in Git 3.0.

OK.

> diff --git a/builtin/remote.c b/builtin/remote.c
> index 1ad3e70a6b..e565b2b3fe 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -640,10 +640,12 @@ static int migrate_file(struct remote *remote)
>  	strbuf_addf(&buf, "remote.%s.fetch", remote->name);
>  	for (i = 0; i < remote->fetch.nr; i++)
>  		git_config_set_multivar(buf.buf, remote->fetch.items[i].raw, "^$", 0);
> +#ifndef WITH_BREAKING_CHANGES
>  	if (remote->origin == REMOTE_REMOTES)
>  		unlink_or_warn(git_path("remotes/%s", remote->name));
>  	else if (remote->origin == REMOTE_BRANCHES)
>  		unlink_or_warn(git_path("branches/%s", remote->name));
> +#endif /* WITH_BREAKING_CHANGES */
>  	strbuf_release(&buf);

Interesting.  I wonder if our new warning should talk about whatever
end-user facing interface that triggers this code path.  It would
help them wean themselves away from the old interface, no?

> diff --git a/remote.c b/remote.c
> index 10104d11e3..5feb0ae886 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -293,6 +293,7 @@ static void add_instead_of(struct rewrite *rewrite, const char *instead_of)
>  	rewrite->instead_of_nr++;
>  }
>  
> +#ifndef WITH_BREAKING_CHANGES
>  static const char *skip_spaces(const char *s)
>  {
>  	while (isspace(*s))
> @@ -308,6 +309,13 @@ static void read_remotes_file(struct remote_state *remote_state,
>  
>  	if (!f)
>  		return;
> +
> +	warning(_("Reading remote from \"remotes/%s\", which is nominated\n"
> +		  "for removal. If you still use the \"remotes/\" directory\n"
> +		  "it is recommended to migrate to config-based remotes. If\n"

Do we have a way to concisely say "how" to do this?  If I am reading
the caller of migrate_file() in builtin/remote.c, it would be

    $ git remote mv foo foo

for any foo in .git/remotes/* or .git/branches/* hierarchy?

Of course they may be an ancient leftover file that the user even no
longer is aware of having, in which case

    $ rm .git/remotes/foo

might be an OK answer, but even then

    $ git remote rm foo

would probably be more appropriate.

> +		  "you cannot, please let us know you still use it by sending\n"

I do not think we care to receive a piece of e-mail that only says
"I still use it".  We may want to learn _why_ they cannot switch
away, though.

The same comment applies to the other side.

Everything else in this patch looked superb.

Thanks.