Re: [RFC/PATCH] fetch: make --prune configurable

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

Michael Schubert <mschub@elegosoft.com> writes:

> $gmane/201715 brought up the idea to fetch --prune by default.

When you can summarize it in a few lines, e.g.

    Without "git fetch --prune", remote-tracking branches for a branch
    the other side already has removed will stay forever.  Some people
    want to always run "git fetch --prune".

please refrain from forcing people to go to the web while reading
logs.

> Since --prune is a "potentially destructive operation" (Git doesn't
> keep reflogs for deleted references yet), we don't want to prune
> without users consent.
>
> To accommodate users who want to either prune always or when fetching
> from a particular remote, add two new configuration variables
> "fetch.prune" and "remote.<name>.prune":
>
>  - "fetch.prune" allows to enable prune for all fetch operations.
>
>  - "remote.<name>.prune" allows to change the behaviour per remote.
>

Add:

    "git fetch --no-prune" from the command line will defeat the
    configured default for safety.

(I didn't check if your patch already does so, though).

Other than that, the log message looks good.

Thanks for starting to work on this.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index b4d4887..74e8026 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1067,6 +1067,10 @@ fetch.unpackLimit::
>  	especially on slow filesystems.  If not set, the value of
>  	`transfer.unpackLimit` is used instead.
>  
> +fetch.prune::
> +	If true, fetch will automatically behave as if the `--prune`
> +	option was given on the command line.
> +
>  format.attach::
>  	Enable multipart/mixed attachments as the default for
>  	'format-patch'.  The value can also be a double quoted string
> @@ -2010,6 +2014,11 @@ remote.<name>.vcs::
>  	Setting this to a value <vcs> will cause Git to interact with
>  	the remote with the git-remote-<vcs> helper.
>  
> +remote.<name>.prune::
> +	When set to true, fetching from this remote by default will also
> +	remove any remote-tracking branches which no longer exist on the
> +	remote (as if the `--prune` option was give on the command line).

We may want to say something about interaction between the two
variables.  E.g. fetch.prune=true, remote.origin.prune=false would
hopefully not to prune when you are fetching from your 'origin', and
fetch.prune=false, remote.origin.prune=true would.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index d784b2e..3953317 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -30,7 +30,14 @@ enum {
>  	TAGS_SET = 2
>  };
>  
> -static int all, append, dry_run, force, keep, multiple, prune, update_head_ok, verbosity;
> +enum {
> +	PRUNE_UNSET = 0,
> +	PRUNE_DEFAULT = 1,
> +	PRUNE_FORCE = 2
> +};
> +
> +static int prune = PRUNE_DEFAULT;

I find this unconventional in that usually _UNSET means "the user
hasn't explicitly said anything about what she wants" (hence
typically a variable is initialized to that value).  Also I am not
sure what "FORCE" means.

If this were 

enum {
	PRUNE_UNSET = -1,
	PRUNE_NO = 0,
	PRUNE_YES = 1
};

then I would understand, but at that point, that is a typical
setting for a boolean variable, so we could just use -1/0/1.

> +static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;
>  static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
>  static int tags = TAGS_DEFAULT, unshallow;
>  static const char *depth;
> @@ -54,6 +61,17 @@ static int option_parse_recurse_submodules(const struct option *opt,
>  	return 0;
>  }
>  
> +static int git_fetch_config(const char *k, const char *v, void *cb)
> +{
> +	if (!strcmp(k, "fetch.prune")) {
> +		int boolval = git_config_bool(k, v);
> +		if (boolval)
> +			prune = PRUNE_FORCE;
> +		return 0;

This is not good, is it?  Imagine fetch.prune=true in ~/.gitconfig
and fetch.prune=false in $GIT_DIR/config; I'd expect the more
specific one to set "prune" back to non-FORCE value.

As you do not have transport available before you process
parse_options(), I think you need two variables, "prune" that is
used to determine what happens (same as in the code before your
patch) and a new one "fetch_prune_config" that records what we read
from the fetch.prune configuration.

So I _suspect_ the interaction between the configuration parser and
the command line option parser should look more like this, in order
to implement the correct order of precedence:

	static int fetch_prune_config = -1; /* unspecified */
        static int prune = -1; /* unspecified */
	#define PRUNE_BY_DEFAULT 0

	...

        /* set "fetch_prune_config" */
	git_config(git_fetch_config);
	--> git_fetch_config():
		if (!strcmp(k, "fetch.prune")) {
			fetch_prune_config = git_config_bool(k, v);
			return 0;
		}

	...

        /* set "prune" */
        parse_options();

        --> fetch_one();
		transport_get();

                if (prune < 0) {
			/* no command line request; combine configs */
			if (0 <= transport->remote->prune)
				prune = transport->remote->prune;
			else if (0 <= fetch_prune_config)
                        	prune = fetch_prune_config;
			else
                        	prune = PRUNE_BY_DEFAULT;
		}

You would need to update make_remote() to initialise its .prune
member with -1 (unspecified).

You also need to change OPT_BOOLEAN() to OPT_BOOL() for the command
line processing for "prune", as the former is a "count up" that is
useful for things like "-v", "-v -v", etc.

> +	}
> +	return git_default_config(k, v, cb);

What kind of random configuration are we expecting to read from and
affect our execution by falling back to the default config?

>  static struct option builtin_fetch_options[] = {
>  	OPT__VERBOSITY(&verbosity),
>  	OPT_BOOLEAN(0, "all", &all,
> @@ -770,7 +788,7 @@ static int do_fetch(struct transport *transport,
>  		retcode = 1;
>  		goto cleanup;
>  	}
> -	if (prune) {
> +	if (prune == PRUNE_FORCE || (transport->remote->prune && prune)) {

I cannot offhand see how this is correct with your code (without the
above "you need two variables" suggestion).  It reads to me:

	If fetch.prune is set, we set it to PRUNE_FORCE in the
        configuration parser, and in that case we ignore everything
        else and go ahead to prune.

which does not sound right.  fetch.prune=yes remote.$name.prune=no
should not prune, fetch.prune=yes with --no-prune from the command
line should not prune, etc.  But I may be misreading this complex
boolean expression in the patch.

And with the suggested change, this can stay 

	if (prune)

without having to do any mental gymnastics here.

> @@ -882,8 +900,10 @@ static void add_options_to_argv(struct argv_array *argv)
>  {
>  	if (dry_run)
>  		argv_array_push(argv, "--dry-run");
> -	if (prune)
> +	if (prune == PRUNE_FORCE)
>  		argv_array_push(argv, "--prune");
> +	else if (prune == PRUNE_UNSET)
> +		argv_array_push(argv, "--no-prune");
>  	if (update_head_ok)
>  		argv_array_push(argv, "--update-head-ok");
>  	if (force)

And I think this hunk can go with the suggested change.

> @@ -1007,6 +1027,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  	for (i = 1; i < argc; i++)
>  		strbuf_addf(&default_rla, " %s", argv[i]);
>  
> +	git_config(git_fetch_config, NULL);
> +
>  	argc = parse_options(argc, argv, prefix,
>  			     builtin_fetch_options, builtin_fetch_usage, 0);
>  
> diff --git a/remote.c b/remote.c
> index 6f57830..e6f2acb 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -404,6 +404,8 @@ static int handle_config(const char *key, const char *value, void *cb)
>  		remote->skip_default_update = git_config_bool(key, value);
>  	else if (!strcmp(subkey, ".skipfetchall"))
>  		remote->skip_default_update = git_config_bool(key, value);
> +	else if (!strcmp(subkey, ".prune"))
> +		remote->prune = git_config_bool(key, value);
>  	else if (!strcmp(subkey, ".url")) {
>  		const char *v;
>  		if (git_config_string(&v, key, value))
> diff --git a/remote.h b/remote.h
> index cf56724..4db3498 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -40,6 +40,7 @@ struct remote {
>  	int fetch_tags;
>  	int skip_default_update;
>  	int mirror;
> +	int prune;
>  
>  	const char *receivepack;
>  	const char *uploadpack;
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index fde6891..f3d63ca 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -497,6 +497,44 @@ test_expect_success "should be able to fetch with duplicate refspecs" '
>  	)
>  '
>  
> +test_expect_success 'fetch should prune when fetch.prune is true' '
> +  cd "$D" &&
> +  git branch somebranch &&
> +  (
> +    cd one &&
> +    git fetch &&
> +    test -f .git/refs/remotes/origin/somebranch
> +  ) &&
> +  git branch -d somebranch &&
> +  (
> +    cd one &&
> +    git config fetch.prune true &&
> +    git fetch --no-prune &&
> +    test -f .git/refs/remotes/origin/somebranch &&
> +    git fetch &&
> +    ! test -f .git/refs/remotes/origin/somebranch
> +  )
> +'

OK, "fetch.prune < --no-prune" and  "fetch.prune alone" are tested
with this.  "fetch.prune=no" with "--prune" is not tested.

> +test_expect_success 'fetch should prune when remote.<name>.prune is true' '
> +  cd "$D" &&
> +  git branch somebranch &&
> +  (
> +    cd one &&
> +    git fetch &&
> +    test -f .git/refs/remotes/origin/somebranch

Depending on the success/failure of the previous test, we do not
know what value fetch.prune is set in this repository.  What are we
testing here?  By using "test_unconfig" to clear the variables you
care about before starting each test, you would make it clear what
exactly you are testing.

There are at least 9 combinations of settings we should be testing.
A naive matrix:

	fetch.prune		set to false or not set, set to true
        remote.origin.prune	set to false or not set, set to true
	command line		--no-prune, --prune, (neither)

will give us 12 combinations (for completeness, you may want to test
fetch.prune that is not set at all and fetch.prune explicitly set to
false separately, but let's not go overboard), but we presumably
have been testing cases where neither fetch.prune or remote.*.prune
is set, so we would need to only test cases where at least one of
the configuration variables is set (that would make 3 cases),
multiplied with the command line combinations.


> +  ) &&
> +  git branch -d somebranch &&
> +  (
> +    cd one &&
> +    git config remote.origin.prune true &&
> +    git fetch --no-prune &&
> +    test -f .git/refs/remotes/origin/somebranch &&
> +    git fetch &&
> +    ! test -f .git/refs/remotes/origin/somebranch
> +  )
> +'
> +
>  test_expect_success 'all boundary commits are excluded' '
>  	test_commit base &&
>  	test_commit oneside &&