Re: [PATCH] prune: honor --expire=never

From: Adam Simpkins <simpkins@facebook.com>
Date: Feb 27, 2010, 01:21 UTC
Message-ID: <20100227012130.GA28452@facebook.com>
In-Reply-To: <7v4ol3ilri.fsf@alter.siamese.dyndns.org>

On Fri, Feb 26, 2010 at 04:07:45PM -0800, Junio C Hamano wrote:

Show 21 quoted lines

> Adam Simpkins <simpkins@facebook.com> writes:
> 
> > diff --git a/builtin-prune.c b/builtin-prune.c
> > index 4675f60..ce43271 100644
> > --- a/builtin-prune.c
> > +++ b/builtin-prune.c
> > @@ -7,6 +7,8 @@
> >  #include "parse-options.h"
> >  #include "dir.h"
> >  
> > +#define ALWAYS_EXPIRE ((unsigned int)-1)
> > ...
> > @@ -34,7 +36,7 @@ static int prune_tmp_object(const char *path, const char *filename)
> >  static int prune_object(char *path, const char *filename, const unsigned char *sha1)
> >  {
> >  	const char *fullpath = mkpath("%s/%s", path, filename);
> > -	if (expire) {
> > +	if (expire != ALWAYS_EXPIRE) {
> 
> Wouldn't it be a lot simpler to initialize expire to "now" for the default
> case, and remove all these "if (expire)"?

Sure, I can submit an updated patch to do that. It does slightly change the behavior of "git prune" with no --expire argument though:

  - Objects with an mtime in the future will no longer be pruned.
  - We'll call lstat() all of the unreachable objects, even though it
    isn't really necessary.

However, the code is indeed simpler, if you don't think either of these changes matter.

> I think that is how the logic
> to expire reflog entries work, which I think is saner.

Hmm. reflog appears to have the same bug when parsing the gc.reflogexpire and gc.reflogexpireunreachable options. Setting either of these to "never" or "false" results in the default expiration time. (However, using --expire=never on the command line works correctly.)

I'll submit a separate patch for that.

> While you are at it, you might want to think about a way to unify what
> parse_opt_approxidate_cb() and parse_expire_cfg_value() do.  The latter
> knows about "expire = false" but the former doesn't, which is a slight
> inconsistency.

Sure, I'll look into it and submit a patch.

-- 
Adam Simpkins
simpkins@facebook.com