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

[Junio C Hamano]

Here is my previous review comments in a squashable patch form.  The
result seems to pass all 27 combinations (fetch.prune, remote.*.prune
and command line all are tristate yes/no/unspecified).

Without the fix-up in *.c files, three combinations seem to fail.

 Documentation/config.txt |   3 +-
 builtin/fetch.c  |  41 +---
 remote.c |   1 +
 t/t5510-fetch.sh | 118 ---
 4 files changed, 108 insertions(+), 55 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fc39f3a..e4ce7c4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1051,7 +1051,7 @@ fetch.unpackLimit::
 
 fetch.prune::
If true, fetch will automatically behave as if the `--prune`
-   option was given on the command line.
+   option was given on the command line.  See also `remote.name.prune`.
 
 format.attach::
Enable multipart/mixed attachments as the default for
@@ -1992,6 +1992,7 @@ 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).
+   Overrides `fetch.prune` settings, if any.
 
 remotes.group::
The list of remotes which are fetched by git remote update
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 082450b..08ab948 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -30,13 +30,10 @@ enum {
TAGS_SET = 2
 };
 
-enum {
-   PRUNE_UNSET = 0,
-   PRUNE_DEFAULT = 1,
-   PRUNE_FORCE = 2
-};
+static int fetch_prune_config = -1; /* unspecified */
+static int prune = -1; /* unspecified */
+#define PRUNE_BY_DEFAULT 0 /* do we prune by default? */
 
-static int prune = PRUNE_DEFAULT;
 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;
@@ -64,12 +61,10 @@ static int option_parse_recurse_submodules(const struct 
option *opt,
 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;
+   fetch_prune_config = git_config_bool(k, v);
return 0;
}
-   return git_default_config(k, v, cb);
+   return 0;
 }
 
 static struct option builtin_fetch_options[] = {
@@ -87,8 +82,8 @@ static struct option builtin_fetch_options[] = {
N_(fetch all tags and associated objects), TAGS_SET),
OPT_SET_INT('n', NULL, tags,
N_(do not fetch all tags (--no-tags)), TAGS_UNSET),
-   OPT_BOOLEAN('p', prune, prune,
-   N_(prune remote-tracking branches no longer on remote)),
+   OPT_BOOL('p', prune, prune,
+N_(prune remote-tracking branches no longer on remote)),
{ OPTION_CALLBACK, 0, recurse-submodules, NULL, N_(on-demand),
N_(control recursive fetching of submodules),
PARSE_OPT_OPTARG, option_parse_recurse_submodules },
@@ -756,8 +751,11 @@ static int do_fetch(struct transport *transport,
free_refs(ref_map);
return 1;
}
-   if (prune == PRUNE_FORCE || (transport-remote-prune  prune)) {
-   /* If --tags was specified, pretend the user gave us the 
canonical tags refspec */
+   if (prune) {
+   /*
+* If --tags was specified, pretend that the user gave us
+* the canonical tags refspec
+*/
if (tags == TAGS_SET) {
const char *tags_str = refs/tags/*:refs/tags/*;
struct refspec *tags_refspec, *refspec;
@@ -866,10 +864,8 @@ static void add_options_to_argv(struct argv_array *argv)
 {
if (dry_run)
argv_array_push(argv, --dry-run);
-   if (prune == PRUNE_FORCE)
+   if (prune  0)
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)
@@ -936,6 +932,17 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv)
remote name from which new revisions should be fetched.));
 
transport = transport_get(remote, NULL);
+
+   if (prune  0) {
+   /* no command line request */
+   if (0 = transport-remote-prune)
+   prune = transport-remote-prune;
+   else if (0 = fetch_prune_config)
+   prune = fetch_prune_config;
+   else
+   prune = PRUNE_BY_DEFAULT;
+   }
+
  

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

[Junio C Hamano]

Michael Schubert msc...@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();

-- 

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

[John Keeping]

On Mon, Jul 08, 2013 at 02:56:57PM +0200, Michael Schubert wrote:
 $gmane/201715 brought up the idea to fetch --prune by default.
 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.

Should this be remote.name.pruneFetch?  I'd quite like to be able to
configure --prune for git-push as well (I just haven't got around to
actually doing anything about it yet...) and it might be better to be
explicit in the remote.name section from the start.

I'm not sure it's necessary since we already have remote and
pushremote so we could have prune and pushprune but perhaps it's
worth considering.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Mon, Jul 08, 2013 at 02:56:57PM +0200, Michael Schubert wrote:

 $gmane/201715 brought up the idea to fetch --prune by default.
 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.

Thanks. As the person who brought up the destructive nature of --prune
in the thread you mentioned, I have no problem at all with doing
something like this, where the user gets to make the choice. And it is
even a good building block if we later do have deleted-branch reflogs;
we can just flip the default from off to on.

In the meantime, I don't know if it is worth mentioning in the
documentation that the remote branches are hard to get back. On the one
hand, it is the (or at least a) reason why the default is not on. But
it is also far from the only place refs get deleted, so I don't know if
it is worth calling attention to it specifically.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html