patch, 23 partsrefs.c: make ref_transaction_create a wrapper for ref_transaction_update

> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -349,7 +349,7 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
>  	return 0;
>  }
>  
> -static int expire_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)
> +static int expire_reflog(const char *ref, const unsigned char *sha1, void *cb_data)
>  {
>  	struct cmd_reflog_expire_cb *cmd = cb_data;

>> It was called "unused", so at least it was self-consistent.
>
> The missing context is that this was a callback function that had to
> match the each_ref_fn signature [...]
>
> With or without a note in the commit message explaining that,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

>>  {
>>  	struct cmd_reflog_expire_cb *cmd = cb_data;
>
> On second thought: why not update the last parameter to be a 'struct
> cmd_reflog_expire_cb *' instead of 'void *' while at it, like this?
> [...]

> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -368,9 +368,11 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>  	lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
>  	if (!lock)
>  		return error("cannot lock ref '%s'", refname);
> +	if (!reflog_exists(refname)) {
> +		unlock_ref(lock);
> +		return 0;
> +	}
>  	log_file = git_pathdup("logs/%s", refname);
> -	if (!reflog_exists(refname))
> -		goto finish;

> Michael Haggerty wrote:
> 
>> [Subject: expire_reflog(): exit early if the reference has no reflog]
> 
> The caller moves on to expire other reflogs, so it's not exiting.
> "return early", maybe?
> 
> Except the function already returned early.  I think the purpose of
> this patch is to simplify the no-reflog case by handling it separately.
> 
> Anyway, that's just nitpicking about the subject line.  With
> s/exit/return/ it should be clear that this is a refactoring change,
> which for someone looking at the shortlog is the important thing.

> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
>  	return 0;
>  }
>  
> +static struct lock_file reflog_lock;

> +
>  static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
>  {
>  	struct cmd_reflog_expire_cb *cmd = cb_data;
>  	struct expire_reflog_cb cb;
>  	struct ref_lock *lock;
> -	char *log_file, *newlog_path = NULL;
> +	char *log_file;
>  	struct commit *tip_commit;
>  	struct commit_list *tips;
>  	int status = 0;
> @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>  		unlock_ref(lock);
>  		return 0;
>  	}
> +
>  	log_file = git_pathdup("logs/%s", refname);
>  	if (!cmd->dry_run) {
> -		newlog_path = git_pathdup("logs/%s.lock", refname);
> -		cb.newlog = fopen(newlog_path, "w");
> +		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
> +			goto failure;

> @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>  	}
>  
>  	if (cb.newlog) {
> -		if (fclose(cb.newlog)) {
> -			status |= error("%s: %s", strerror(errno),
> -					newlog_path);
> -			unlink(newlog_path);
> +		if (close_lock_file(&reflog_lock)) {
> +			status |= error("Couldn't write %s: %s", log_file,
> +					strerror(errno));

> @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>  			 close_ref(lock) < 0)) {
>  			status |= error("Couldn't write %s",
>  					lock->lk->filename.buf);
> -			unlink(newlog_path);
> -		} else if (rename(newlog_path, log_file)) {
> -			status |= error("cannot rename %s to %s",
> -					newlog_path, log_file);
> -			unlink(newlog_path);
> +			rollback_lock_file(&reflog_lock);
> +		} else if (commit_lock_file(&reflog_lock)) {
> +			status |= error("cannot rename %s.lock to %s",
> +					log_file, log_file);

> Michael Haggerty wrote:
> 
> > We don't actually need the locking functionality, because we already
> > hold the lock on the reference itself, which is how the reflog file is
> > locked. But the lock_file code still does some of the bookkeeping for
> > us and is more careful than the old code here was.
> 
> As you say, the ref lock takes care of mutual exclusion, so we do not
> have to be too careful about compatibility with other tools that might
> not know to lock the reflog.  And this is not tying our hands for a
> future when I might want to lock logs/refs/heads/topic/1 while
> logs/refs/heads/topic still exists as part of the implementation of
> "git mv topic/1 topic".
> 
> Stefan and I had forgotten about that guarantee when looking at that
> kind of operation --- thanks for the reminder.

> On Thu, Dec 04, 2014 at 04:23:31PM -0800, Jonathan Nieder wrote:
>> Michael Haggerty wrote:
>>
>>> We don't actually need the locking functionality, because we already
>>> hold the lock on the reference itself, which is how the reflog file is
>>> locked. But the lock_file code still does some of the bookkeeping for
>>> us and is more careful than the old code here was.
>>
>> As you say, the ref lock takes care of mutual exclusion, so we do not
>> have to be too careful about compatibility with other tools that might
>> not know to lock the reflog.  And this is not tying our hands for a
>> future when I might want to lock logs/refs/heads/topic/1 while
>> logs/refs/heads/topic still exists as part of the implementation of
>> "git mv topic/1 topic".
>>
>> Stefan and I had forgotten about that guarantee when looking at that
>> kind of operation --- thanks for the reminder.
> 
> I did not forget about it, I did not know about that in the first hand.
> We don't seem to have documentation on it?
> 
> So sorry for heading in a direction, which would have been avoidable.

> This isn't documented very well. I thought I saw a comment somewhere in
> the code that stated it explicitly, but I can't find it now. In any
> case, my understanding of the locking protocol for reflogs is:
>
>     The reflog for "$refname", which is stored at
>     "$GIT_DIR/logs/$refname", is locked by holding
>     "$GIT_DIR/refs/$refname.lock", *even if the corresponding
>     reference is packed*.
>
> This implies that readers, who don't pay attention to locks, have to be
> prepared for the possibility that the reflog is in the middle of an
> update and that the last line is incomplete. This is handled by
> show_one_reflog_ent(), which discards incomplete lines.

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
> > This isn't documented very well. I thought I saw a comment somewhere in
> > the code that stated it explicitly, but I can't find it now. In any
> > case, my understanding of the locking protocol for reflogs is:
> >
> >     The reflog for "$refname", which is stored at
> >     "$GIT_DIR/logs/$refname", is locked by holding
> >     "$GIT_DIR/refs/$refname.lock", *even if the corresponding
> >     reference is packed*.
> >
> > This implies that readers, who don't pay attention to locks, have to be
> > prepared for the possibility that the reflog is in the middle of an
> > update and that the last line is incomplete. This is handled by
> > show_one_reflog_ent(), which discards incomplete lines.
> 
> Interesting, as I think I saw Peff did something around that area
> recently.

> Michael Haggerty wrote:
> 
> > We don't actually need the locking functionality, because we already
> > hold the lock on the reference itself, which is how the reflog file is
> > locked. But the lock_file code still does some of the bookkeeping for
> > us and is more careful than the old code here was.
> 
> As you say, the ref lock takes care of mutual exclusion, so we do not
> have to be too careful about compatibility with other tools that might
> not know to lock the reflog.  And this is not tying our hands for a
> future when I might want to lock logs/refs/heads/topic/1 while
> logs/refs/heads/topic still exists as part of the implementation of
> "git mv topic/1 topic".
> 
> Stefan and I had forgotten about that guarantee when looking at that
> kind of operation --- thanks for the reminder.
> 
> Should updates to the HEAD reflog acquire HEAD.lock?  (They don't
> currently.)
> 
> [...]
> > --- a/builtin/reflog.c
> > +++ b/builtin/reflog.c
> > @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
> >  	return 0;
> >  }
> >  
> > +static struct lock_file reflog_lock;
> 
> If this lockfile is only used in that one function, it can be declared
> inside the function.
> 
> If it is meant to be used throughout the 'git reflog' command, then it
> can go near the top of the file.
> 

> > +
> >  static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
> >  {
> >  	struct cmd_reflog_expire_cb *cmd = cb_data;
> >  	struct expire_reflog_cb cb;
> >  	struct ref_lock *lock;
> > -	char *log_file, *newlog_path = NULL;
> > +	char *log_file;
> >  	struct commit *tip_commit;
> >  	struct commit_list *tips;
> >  	int status = 0;
> > @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
> >  		unlock_ref(lock);
> >  		return 0;
> >  	}
> > +
> >  	log_file = git_pathdup("logs/%s", refname);
> >  	if (!cmd->dry_run) {
> > -		newlog_path = git_pathdup("logs/%s.lock", refname);
> > -		cb.newlog = fopen(newlog_path, "w");
> > +		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
> > +			goto failure;
> 
> hold_lock_file_for_update doesn't print a message.  Code to print one
> looks like
> 
> 	if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
> 		unable_to_lock_message(log_file, errno, &err);
> 		error("%s", err.buf);
> 		goto failure;
> 	}
> 
> (A patch in flight changes that to
> 
> 	if (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {
> 		error("%s", err.buf);
> 		goto failure;
> 	}
> 
> )
> 
> > +		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> > +		if (!cb.newlog)
> > +			goto failure;
> 
> Hm.  lockfile.c::fdopen_lock_file ought to use xfdopen to make this
> case impossible.  And xfdopen should use try_to_free_routine() and
> try again on failure.
> 
> [...]
> > @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
> >  	}
> >  
> >  	if (cb.newlog) {
> > -		if (fclose(cb.newlog)) {
> > -			status |= error("%s: %s", strerror(errno),
> > -					newlog_path);
> > -			unlink(newlog_path);
> > +		if (close_lock_file(&reflog_lock)) {
> > +			status |= error("Couldn't write %s: %s", log_file,
> > +					strerror(errno));
> 
> Style nit: error messages usually start with a lowercase letter
> (though I realize nearby examples are already inconsistent).
> 
> commit_lock_file() can take care of the close_lock_file automatically.
> 
> [...]
> > @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
> >  			 close_ref(lock) < 0)) {
> >  			status |= error("Couldn't write %s",
> >  					lock->lk->filename.buf);
> > -			unlink(newlog_path);
> > -		} else if (rename(newlog_path, log_file)) {
> > -			status |= error("cannot rename %s to %s",
> > -					newlog_path, log_file);
> > -			unlink(newlog_path);
> > +			rollback_lock_file(&reflog_lock);
> > +		} else if (commit_lock_file(&reflog_lock)) {
> > +			status |= error("cannot rename %s.lock to %s",
> > +					log_file, log_file);
> 
> Most callers say "unable to commit reflog '%s'", log_file to hedge their
> bets in case the close failed (which may be what you were avoiding
> above.
> 
> errno is meaningful when commit_lock_file fails, making a more
> detailed diagnosis from strerror(errno) possible.
> 
> Thanks,
> Jonathan

>> > +static struct lock_file reflog_lock;
>> 
>> If this lockfile is only used in that one function, it can be declared
>> inside the function.
>> 
>> If it is meant to be used throughout the 'git reflog' command, then it
>> can go near the top of the file.
>> 
>
> After the series completes, this lock is only used in reflog_expire.
> So I'd rather move it inside the function? Then we could run the reflog_expire
> function in parallel for different locks in theory?

> Stefan Beller <sbeller@google.com> writes:
>
>>> > +static struct lock_file reflog_lock;
>>>
>>> If this lockfile is only used in that one function, it can be declared
>>> inside the function.
>>>
>>> If it is meant to be used throughout the 'git reflog' command, then it
>>> can go near the top of the file.
>>>
>>
>> After the series completes, this lock is only used in reflog_expire.
>> So I'd rather move it inside the function? Then we could run the reflog_expire
>> function in parallel for different locks in theory?
>
> I am not sure about the "parallel" part, but I would imagine that it
> is an essential prerequisite to move this outside the "client" code
> if we want to later replace the backing storage of refs and reflogs
> outside the filesystem, so from that point of view,  I think the
> suggestion makes sense.
>
> Thanks.
>

>>> After the series completes, this lock is only used in reflog_expire.
>>> So I'd rather move it inside the function? Then we could run the reflog_expire
>>> function in parallel for different locks in theory?
>>
>> I am not sure about the "parallel" part, but I would imagine that it
>> is an essential prerequisite to move this outside the "client" code
>> if we want to later replace the backing storage of refs and reflogs
>> outside the filesystem, so from that point of view,  I think the
>> suggestion makes sense.
>>
>> Thanks.
>>
>
> Sorry for the confusion. With parallel I mean,...

> Michael Haggerty wrote:
> 
>> We don't actually need the locking functionality, because we already
>> hold the lock on the reference itself, which is how the reflog file is
>> locked. But the lock_file code still does some of the bookkeeping for
>> us and is more careful than the old code here was.
> 
> As you say, the ref lock takes care of mutual exclusion, so we do not
> have to be too careful about compatibility with other tools that might
> not know to lock the reflog.  And this is not tying our hands for a
> future when I might want to lock logs/refs/heads/topic/1 while
> logs/refs/heads/topic still exists as part of the implementation of
> "git mv topic/1 topic".
> 
> Stefan and I had forgotten about that guarantee when looking at that
> kind of operation --- thanks for the reminder.

> [...]
>> --- a/builtin/reflog.c
>> +++ b/builtin/reflog.c
>> @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
>>  	return 0;
>>  }
>>  
>> +static struct lock_file reflog_lock;
> 
> If this lockfile is only used in that one function, it can be declared
> inside the function.
> 
> If it is meant to be used throughout the 'git reflog' command, then it
> can go near the top of the file.

>> +
>>  static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
>>  {
>>  	struct cmd_reflog_expire_cb *cmd = cb_data;
>>  	struct expire_reflog_cb cb;
>>  	struct ref_lock *lock;
>> -	char *log_file, *newlog_path = NULL;
>> +	char *log_file;
>>  	struct commit *tip_commit;
>>  	struct commit_list *tips;
>>  	int status = 0;
>> @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>>  		unlock_ref(lock);
>>  		return 0;
>>  	}
>> +
>>  	log_file = git_pathdup("logs/%s", refname);
>>  	if (!cmd->dry_run) {
>> -		newlog_path = git_pathdup("logs/%s.lock", refname);
>> -		cb.newlog = fopen(newlog_path, "w");
>> +		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
>> +			goto failure;
> 
> hold_lock_file_for_update doesn't print a message.  Code to print one
> looks like
> 
> 	if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
> 		unable_to_lock_message(log_file, errno, &err);
> 		error("%s", err.buf);
> 		goto failure;
> 	}

> (A patch in flight changes that to
> 
> 	if (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {
> 		error("%s", err.buf);
> 		goto failure;
> 	}
> 
> )

>> +		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
>> +		if (!cb.newlog)
>> +			goto failure;
> 
> Hm.  lockfile.c::fdopen_lock_file ought to use xfdopen to make this
> case impossible.  And xfdopen should use try_to_free_routine() and
> try again on failure.

> [...]
>> @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>>  	}
>>  
>>  	if (cb.newlog) {
>> -		if (fclose(cb.newlog)) {
>> -			status |= error("%s: %s", strerror(errno),
>> -					newlog_path);
>> -			unlink(newlog_path);
>> +		if (close_lock_file(&reflog_lock)) {
>> +			status |= error("Couldn't write %s: %s", log_file,
>> +					strerror(errno));
> 
> Style nit: error messages usually start with a lowercase letter
> (though I realize nearby examples are already inconsistent).

> [...]
>> @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>>  			 close_ref(lock) < 0)) {
>>  			status |= error("Couldn't write %s",
>>  					lock->lk->filename.buf);
>> -			unlink(newlog_path);
>> -		} else if (rename(newlog_path, log_file)) {
>> -			status |= error("cannot rename %s to %s",
>> -					newlog_path, log_file);
>> -			unlink(newlog_path);
>> +			rollback_lock_file(&reflog_lock);
>> +		} else if (commit_lock_file(&reflog_lock)) {
>> +			status |= error("cannot rename %s.lock to %s",
>> +					log_file, log_file);
> 
> Most callers say "unable to commit reflog '%s'", log_file to hedge their
> bets in case the close failed (which may be what you were avoiding
> above.
> 
> errno is meaningful when commit_lock_file fails, making a more
> detailed diagnosis from strerror(errno) possible.

> which is how the reflog file is
> locked. But the lock_file code still does some of the bookkeeping for
> us and is more careful than the old code here was.
>
> For example:
>
> * Correctly handle the case that the reflog lock file already exists
>   for some reason or cannot be opened.
>
> * Correctly clean up the lockfile if the program dies.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/reflog.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index a282e60..d344d45 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
>         return 0;
>  }
>
> +static struct lock_file reflog_lock;
> +
>  static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
>  {
>         struct cmd_reflog_expire_cb *cmd = cb_data;
>         struct expire_reflog_cb cb;
>         struct ref_lock *lock;
> -       char *log_file, *newlog_path = NULL;
> +       char *log_file;
>         struct commit *tip_commit;
>         struct commit_list *tips;
>         int status = 0;
> @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>                 unlock_ref(lock);
>                 return 0;
>         }
> +
>         log_file = git_pathdup("logs/%s", refname);
>         if (!cmd->dry_run) {
> -               newlog_path = git_pathdup("logs/%s.lock", refname);
> -               cb.newlog = fopen(newlog_path, "w");
> +               if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
> +                       goto failure;
> +               cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> +               if (!cb.newlog)
> +                       goto failure;
>         }
>
>         cb.cmd = cmd;
> @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>         }
>
>         if (cb.newlog) {
> -               if (fclose(cb.newlog)) {
> -                       status |= error("%s: %s", strerror(errno),
> -                                       newlog_path);
> -                       unlink(newlog_path);
> +               if (close_lock_file(&reflog_lock)) {
> +                       status |= error("Couldn't write %s: %s", log_file,
> +                                       strerror(errno));
>                 } else if (cmd->updateref &&
>                         (write_in_full(lock->lock_fd,
>                                 sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
> @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>                          close_ref(lock) < 0)) {
>                         status |= error("Couldn't write %s",
>                                         lock->lk->filename.buf);
> -                       unlink(newlog_path);
> -               } else if (rename(newlog_path, log_file)) {
> -                       status |= error("cannot rename %s to %s",
> -                                       newlog_path, log_file);
> -                       unlink(newlog_path);
> +                       rollback_lock_file(&reflog_lock);
> +               } else if (commit_lock_file(&reflog_lock)) {
> +                       status |= error("cannot rename %s.lock to %s",
> +                                       log_file, log_file);
>                 } else if (cmd->updateref && commit_ref(lock)) {
>                         status |= error("Couldn't set %s", lock->ref_name);
> -               } else {
> -                       adjust_shared_perm(log_file);
>                 }
>         }
> -       free(newlog_path);
>         free(log_file);
>         unlock_ref(lock);
>         return status;
> +
> + failure:
> +       rollback_lock_file(&reflog_lock);
> +       free(log_file);
> +       unlock_ref(lock);
> +       return -1;
>  }
>
>  static int collect_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)
> --
> 2.1.3
>

> On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> We don't actually need the locking functionality, because we already
>> hold the lock on the reference itself,
> 
> No. You do need the lock.
> The ref is locked only during transaction_commit()
> 
> If you don't want to lock the reflog file and instead rely on the lock
> on the ref itself you will need to
> rework your patches so that the lock on the ref is taken already
> during, for example, transaction_update_ref() instead.
> 
> But without doing those changes and moving the ref locking from
> _commit() to _update_ref() you will risk reflog corruption/surprises
> if two operations collide and both rewrite the reflog without any lock held.

> Extracted from expire_reflog_ent() a function that is solely
> responsible for deciding whether a reflog entry should be expired. By
> separating this "business logic" from the mechanics of actually
> expiring entries, we are working towards the goal of encapsulating
> reflog expiry within the refs API, with policy decided by a callback
> function passed to it by its caller.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

> +	if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
> +				     message, cb_data)) {
> +		if (!cb->newlog)
> +			printf("would prune %s", message);
> +		else if (cb->cmd->verbose)
> +			printf("prune %s", message);

> +			char sign = (tz < 0) ? '-' : '+';
> +			int zone = (tz < 0) ? (-tz) : tz;
> +			fprintf(cb->newlog, "%s %s %s %lu %c%04d\t%s",
> +				sha1_to_hex(osha1), sha1_to_hex(nsha1),
> +				email, timestamp, sign, zone,
> +				message);

> Extract two functions, reflog_expiry_prepare() and
> reflog_expiry_cleanup(), from expire_reflog(). This is a further step
> towards separating the code for deciding on expiration policy from the
> code that manages the physical expiration.
> 
> This change requires a couple of local variables from expire_reflog()
> to be turned into fields of "struct expire_reflog_cb". More
> reorganization of the callback data will follow in later commits.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

> ---
> In fact, the work done in reflog_expire_cleanup() doesn't really need
> to be done via a callback, because it doesn't need to be done while
> the reference lock is held. But the symmetry between prepare and
> cleanup is kindof nice. Perhaps some future policy decision will want
> to do some final work under the reference lock?
> 
> But it would be easy to get rid of this third callback function and
> have the callers do the work themselves after calling expire_reflog().
> I don't have a string feeling either way.
> 
>  builtin/reflog.c | 94 +++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 52 insertions(+), 42 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 7bc6e0f..ebfa635 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -43,6 +43,8 @@ struct expire_reflog_cb {
>  	unsigned long mark_limit;
>  	struct cmd_reflog_expire_cb *cmd;
>  	unsigned char last_kept_sha1[20];
> +	struct commit *tip_commit;
> +	struct commit_list *tips;
>  };
>  
>  struct collected_reflog {
> @@ -363,6 +365,54 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
>  	return 0;
>  }
>  
> +static void reflog_expiry_prepare(const char *refname,
> +				  const unsigned char *sha1,
> +				  struct expire_reflog_cb *cb)
> +{
> +	if (!cb->cmd->expire_unreachable || !strcmp(refname, "HEAD")) {
> +		cb->tip_commit = NULL;
> +		cb->unreachable_expire_kind = UE_HEAD;
> +	} else {
> +		cb->tip_commit = lookup_commit_reference_gently(sha1, 1);
> +		if (!cb->tip_commit)
> +			cb->unreachable_expire_kind = UE_ALWAYS;
> +		else
> +			cb->unreachable_expire_kind = UE_NORMAL;
> +	}
> +
> +	if (cb->cmd->expire_unreachable <= cb->cmd->expire_total)
> +		cb->unreachable_expire_kind = UE_ALWAYS;
> +
> +	cb->mark_list = NULL;
> +	cb->tips = NULL;
> +	if (cb->unreachable_expire_kind != UE_ALWAYS) {
> +		if (cb->unreachable_expire_kind == UE_HEAD) {
> +			struct commit_list *elem;
> +			for_each_ref(push_tip_to_list, &cb->tips);
> +			for (elem = cb->tips; elem; elem = elem->next)
> +				commit_list_insert(elem->item, &cb->mark_list);
> +		} else {
> +			commit_list_insert(cb->tip_commit, &cb->mark_list);
> +		}
> +		cb->mark_limit = cb->cmd->expire_total;
> +		mark_reachable(cb);
> +	}
> +}
> +
> +static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
> +{
> +	if (cb->unreachable_expire_kind != UE_ALWAYS) {
> +		if (cb->unreachable_expire_kind == UE_HEAD) {
> +			struct commit_list *elem;
> +			for (elem = cb->tips; elem; elem = elem->next)
> +				clear_commit_marks(elem->item, REACHABLE);
> +			free_commit_list(cb->tips);
> +		} else {
> +			clear_commit_marks(cb->tip_commit, REACHABLE);
> +		}
> +	}
> +}
> +
>  static struct lock_file reflog_lock;
>  
>  static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
> @@ -371,8 +421,6 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>  	struct expire_reflog_cb cb;
>  	struct ref_lock *lock;
>  	char *log_file;
> -	struct commit *tip_commit;
> -	struct commit_list *tips;
>  	int status = 0;
>  
>  	memset(&cb, 0, sizeof(cb));
> @@ -400,47 +448,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
>  
>  	cb.cmd = cmd;
>  
> -	if (!cmd->expire_unreachable || !strcmp(refname, "HEAD")) {
> -		tip_commit = NULL;
> -		cb.unreachable_expire_kind = UE_HEAD;
> -	} else {
> -		tip_commit = lookup_commit_reference_gently(sha1, 1);
> -		if (!tip_commit)
> -			cb.unreachable_expire_kind = UE_ALWAYS;
> -		else
> -			cb.unreachable_expire_kind = UE_NORMAL;
> -	}
> -
> -	if (cmd->expire_unreachable <= cmd->expire_total)
> -		cb.unreachable_expire_kind = UE_ALWAYS;
> -
> -	cb.mark_list = NULL;
> -	tips = NULL;
> -	if (cb.unreachable_expire_kind != UE_ALWAYS) {
> -		if (cb.unreachable_expire_kind == UE_HEAD) {
> -			struct commit_list *elem;
> -			for_each_ref(push_tip_to_list, &tips);
> -			for (elem = tips; elem; elem = elem->next)
> -				commit_list_insert(elem->item, &cb.mark_list);
> -		} else {
> -			commit_list_insert(tip_commit, &cb.mark_list);
> -		}
> -		cb.mark_limit = cmd->expire_total;
> -		mark_reachable(&cb);
> -	}
> -
> +	reflog_expiry_prepare(refname, sha1, &cb);
>  	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
> -
> -	if (cb.unreachable_expire_kind != UE_ALWAYS) {
> -		if (cb.unreachable_expire_kind == UE_HEAD) {
> -			struct commit_list *elem;
> -			for (elem = tips; elem; elem = elem->next)
> -				clear_commit_marks(elem->item, REACHABLE);
> -			free_commit_list(tips);
> -		} else {
> -			clear_commit_marks(tip_commit, REACHABLE);
> -		}
> -	}
> +	reflog_expiry_cleanup(&cb);
>  
>  	if (cb.newlog) {
>  		if (close_lock_file(&reflog_lock)) {
> -- 
> 2.1.3
> 

> We want to separate the options relevant to the expiry machinery from
> the options affecting the expiration policy. So add a "flags" argument
> to expire_reflog() to hold the former.
> 
> The argument doesn't yet do anything.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

> ---
>  builtin/reflog.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index ebfa635..319f0d2 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -415,7 +415,8 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
>  
>  static struct lock_file reflog_lock;
>  
> -static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
> +static int expire_reflog(const char *refname, const unsigned char *sha1,
> +			 unsigned int flags, void *cb_data)
>  {
>  	struct cmd_reflog_expire_cb *cmd = cb_data;
>  	struct expire_reflog_cb cb;
> @@ -627,6 +628,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  	unsigned long now = time(NULL);
>  	int i, status, do_all;
>  	int explicit_expiry = 0;
> +	unsigned int flags = 0;
>  
>  	default_reflog_expire_unreachable = now - 30 * 24 * 3600;
>  	default_reflog_expire = now - 90 * 24 * 3600;
> @@ -696,7 +698,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  		for (i = 0; i < collected.nr; i++) {
>  			struct collected_reflog *e = collected.e[i];
>  			set_reflog_expiry_param(&cb, explicit_expiry, e->reflog);
> -			status |= expire_reflog(e->reflog, e->sha1, &cb);
> +			status |= expire_reflog(e->reflog, e->sha1, flags, &cb);
>  			free(e);
>  		}
>  		free(collected.e);
> @@ -710,7 +712,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  			continue;
>  		}
>  		set_reflog_expiry_param(&cb, explicit_expiry, ref);
> -		status |= expire_reflog(ref, sha1, &cb);
> +		status |= expire_reflog(ref, sha1, flags, &cb);
>  	}
>  	return status;
>  }
> @@ -729,6 +731,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  {
>  	struct cmd_reflog_expire_cb cb;
>  	int i, status = 0;
> +	unsigned int flags = 0;
>  
>  	memset(&cb, 0, sizeof(cb));
>  
> @@ -781,7 +784,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  			cb.expire_total = 0;
>  		}
>  
> -		status |= expire_reflog(ref, sha1, &cb);
> +		status |= expire_reflog(ref, sha1, flags, &cb);
>  		free(ref);
>  	}
>  	return status;
> -- 
> 2.1.3
> 

> ---
>  builtin/reflog.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 319f0d2..a490193 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -22,7 +22,6 @@ static unsigned long default_reflog_expire_unreachable;
>  
>  struct cmd_reflog_expire_cb {
>  	struct rev_info revs;
> -	int dry_run;
>  	int stalefix;
>  	int rewrite;
>  	int updateref;
> @@ -415,6 +414,10 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
>  
>  static struct lock_file reflog_lock;
>  
> +enum expire_reflog_flags {
> +	EXPIRE_REFLOGS_DRY_RUN = 1 << 0
> +};
> +
>  static int expire_reflog(const char *refname, const unsigned char *sha1,
>  			 unsigned int flags, void *cb_data)
>  {
> @@ -439,7 +442,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  	}
>  
>  	log_file = git_pathdup("logs/%s", refname);
> -	if (!cmd->dry_run) {
> +	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
>  		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
>  			goto failure;
>  		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> @@ -453,7 +456,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
>  	reflog_expiry_cleanup(&cb);
>  
> -	if (cb.newlog) {
> +	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
>  		if (close_lock_file(&reflog_lock)) {
>  			status |= error("Couldn't write %s: %s", log_file,
>  					strerror(errno));
> @@ -644,7 +647,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  	for (i = 1; i < argc; i++) {
>  		const char *arg = argv[i];
>  		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
> -			cb.dry_run = 1;
> +			flags |= EXPIRE_REFLOGS_DRY_RUN;
>  		else if (starts_with(arg, "--expire=")) {
>  			if (parse_expiry_date(arg + 9, &cb.expire_total))
>  				die(_("'%s' is not a valid timestamp"), arg);
> @@ -738,7 +741,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  	for (i = 1; i < argc; i++) {
>  		const char *arg = argv[i];
>  		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
> -			cb.dry_run = 1;
> +			flags |= EXPIRE_REFLOGS_DRY_RUN;
>  		else if (!strcmp(arg, "--rewrite"))
>  			cb.rewrite = 1;
>  		else if (!strcmp(arg, "--updateref"))
> -- 
> 2.1.3
> 

> ---
>  builtin/reflog.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index a490193..597c547 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -24,7 +24,6 @@ struct cmd_reflog_expire_cb {
>  	struct rev_info revs;
>  	int stalefix;
>  	int rewrite;
> -	int updateref;
>  	int verbose;
>  	unsigned long expire_total;
>  	unsigned long expire_unreachable;
> @@ -415,7 +414,8 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
>  static struct lock_file reflog_lock;
>  
>  enum expire_reflog_flags {
> -	EXPIRE_REFLOGS_DRY_RUN = 1 << 0
> +	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
> +	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1
>  };
>  
>  static int expire_reflog(const char *refname, const unsigned char *sha1,
> @@ -460,7 +460,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  		if (close_lock_file(&reflog_lock)) {
>  			status |= error("Couldn't write %s: %s", log_file,
>  					strerror(errno));
> -		} else if (cmd->updateref &&
> +		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
>  			(write_in_full(lock->lock_fd,
>  				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
>  			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
> @@ -471,7 +471,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  		} else if (commit_lock_file(&reflog_lock)) {
>  			status |= error("cannot rename %s.lock to %s",
>  					log_file, log_file);
> -		} else if (cmd->updateref && commit_ref(lock)) {
> +		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
>  			status |= error("Couldn't set %s", lock->ref_name);
>  		}
>  	}
> @@ -663,7 +663,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  		else if (!strcmp(arg, "--rewrite"))
>  			cb.rewrite = 1;
>  		else if (!strcmp(arg, "--updateref"))
> -			cb.updateref = 1;
> +			flags |= EXPIRE_REFLOGS_UPDATE_REF;
>  		else if (!strcmp(arg, "--all"))
>  			do_all = 1;
>  		else if (!strcmp(arg, "--verbose"))
> @@ -745,7 +745,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  		else if (!strcmp(arg, "--rewrite"))
>  			cb.rewrite = 1;
>  		else if (!strcmp(arg, "--updateref"))
> -			cb.updateref = 1;
> +			flags |= EXPIRE_REFLOGS_UPDATE_REF;
>  		else if (!strcmp(arg, "--verbose"))
>  			cb.verbose = 1;
>  		else if (!strcmp(arg, "--")) {
> -- 
> 2.1.3
> 

> ---
>  builtin/reflog.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 597c547..3538e4b 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -30,7 +30,7 @@ struct cmd_reflog_expire_cb {
>  	int recno;
>  };
>  
> -struct expire_reflog_cb {
> +struct expire_reflog_policy_cb {
>  	FILE *newlog;
>  	enum {
>  		UE_NORMAL,
> @@ -220,7 +220,7 @@ static int keep_entry(struct commit **it, unsigned char *sha1)
>   * the expire_limit and queue them back, so that the caller can call
>   * us again to restart the traversal with longer expire_limit.
>   */
> -static void mark_reachable(struct expire_reflog_cb *cb)
> +static void mark_reachable(struct expire_reflog_policy_cb *cb)
>  {
>  	struct commit *commit;
>  	struct commit_list *pending;
> @@ -259,7 +259,7 @@ static void mark_reachable(struct expire_reflog_cb *cb)
>  	cb->mark_list = leftover;
>  }
>  
> -static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, unsigned char *sha1)
> +static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit, unsigned char *sha1)
>  {
>  	/*
>  	 * We may or may not have the commit yet - if not, look it
> @@ -295,7 +295,7 @@ static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
>  				    const char *email, unsigned long timestamp, int tz,
>  				    const char *message, void *cb_data)
>  {
> -	struct expire_reflog_cb *cb = cb_data;
> +	struct expire_reflog_policy_cb *cb = cb_data;
>  	struct commit *old, *new;
>  
>  	if (timestamp < cb->cmd->expire_total)
> @@ -323,7 +323,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
>  		const char *email, unsigned long timestamp, int tz,
>  		const char *message, void *cb_data)
>  {
> -	struct expire_reflog_cb *cb = cb_data;
> +	struct expire_reflog_policy_cb *cb = cb_data;
>  
>  	if (cb->cmd->rewrite)
>  		osha1 = cb->last_kept_sha1;
> @@ -350,7 +350,8 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
>  	return 0;
>  }
>  
> -static int push_tip_to_list(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
> +static int push_tip_to_list(const char *refname, const unsigned char *sha1,
> +			    int flags, void *cb_data)
>  {
>  	struct commit_list **list = cb_data;
>  	struct commit *tip_commit;
> @@ -365,7 +366,7 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
>  
>  static void reflog_expiry_prepare(const char *refname,
>  				  const unsigned char *sha1,
> -				  struct expire_reflog_cb *cb)
> +				  struct expire_reflog_policy_cb *cb)
>  {
>  	if (!cb->cmd->expire_unreachable || !strcmp(refname, "HEAD")) {
>  		cb->tip_commit = NULL;
> @@ -397,7 +398,7 @@ static void reflog_expiry_prepare(const char *refname,
>  	}
>  }
>  
> -static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
> +static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
>  {
>  	if (cb->unreachable_expire_kind != UE_ALWAYS) {
>  		if (cb->unreachable_expire_kind == UE_HEAD) {
> @@ -422,7 +423,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  			 unsigned int flags, void *cb_data)
>  {
>  	struct cmd_reflog_expire_cb *cmd = cb_data;
> -	struct expire_reflog_cb cb;
> +	struct expire_reflog_policy_cb cb;
>  	struct ref_lock *lock;
>  	char *log_file;
>  	int status = 0;
> -- 
> 2.1.3
> 

> Add a new data type, "struct expire_reflog_cb", for holding the data
> that expire_reflog() passes to expire_reflog_ent() via
> for_each_reflog_ent(). For now it only holds a pointer to "struct
> expire_reflog_policy_cb". In future commits we will move some data
> from the latter to the former.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

> ---
>  builtin/reflog.c | 43 ++++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 3538e4b..5dfa53a 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -45,10 +45,15 @@ struct expire_reflog_policy_cb {
>  	struct commit_list *tips;
>  };
>  
> +struct expire_reflog_cb {
> +	void *policy_cb;
> +};
> +
>  struct collected_reflog {
>  	unsigned char sha1[20];
>  	char reflog[FLEX_ARRAY];
>  };
> +
>  struct collect_reflog_cb {
>  	struct collected_reflog **e;
>  	int alloc;
> @@ -323,28 +328,29 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
>  		const char *email, unsigned long timestamp, int tz,
>  		const char *message, void *cb_data)
>  {
> -	struct expire_reflog_policy_cb *cb = cb_data;
> +	struct expire_reflog_cb *cb = cb_data;
> +	struct expire_reflog_policy_cb *policy_cb = cb->policy_cb;
>  
> -	if (cb->cmd->rewrite)
> -		osha1 = cb->last_kept_sha1;
> +	if (policy_cb->cmd->rewrite)
> +		osha1 = policy_cb->last_kept_sha1;
>  
>  	if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
> -				     message, cb_data)) {
> -		if (!cb->newlog)
> +				     message, policy_cb)) {
> +		if (!policy_cb->newlog)
>  			printf("would prune %s", message);
> -		else if (cb->cmd->verbose)
> +		else if (policy_cb->cmd->verbose)
>  			printf("prune %s", message);
>  	} else {
> -		if (cb->newlog) {
> +		if (policy_cb->newlog) {
>  			char sign = (tz < 0) ? '-' : '+';
>  			int zone = (tz < 0) ? (-tz) : tz;
> -			fprintf(cb->newlog, "%s %s %s %lu %c%04d\t%s",
> +			fprintf(policy_cb->newlog, "%s %s %s %lu %c%04d\t%s",
>  				sha1_to_hex(osha1), sha1_to_hex(nsha1),
>  				email, timestamp, sign, zone,
>  				message);
> -			hashcpy(cb->last_kept_sha1, nsha1);
> +			hashcpy(policy_cb->last_kept_sha1, nsha1);
>  		}
> -		if (cb->cmd->verbose)
> +		if (policy_cb->cmd->verbose)
>  			printf("keep %s", message);
>  	}
>  	return 0;
> @@ -423,12 +429,15 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  			 unsigned int flags, void *cb_data)
>  {
>  	struct cmd_reflog_expire_cb *cmd = cb_data;
> -	struct expire_reflog_policy_cb cb;
> +	struct expire_reflog_cb cb;
> +	struct expire_reflog_policy_cb policy_cb;
>  	struct ref_lock *lock;
>  	char *log_file;
>  	int status = 0;
>  
>  	memset(&cb, 0, sizeof(cb));
> +	memset(&policy_cb, 0, sizeof(policy_cb));
> +	cb.policy_cb = &policy_cb;
>  
>  	/*
>  	 * we take the lock for the ref itself to prevent it from
> @@ -446,16 +455,16 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
>  		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
>  			goto failure;
> -		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> -		if (!cb.newlog)
> +		policy_cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> +		if (!policy_cb.newlog)
>  			goto failure;
>  	}
>  
> -	cb.cmd = cmd;
> +	policy_cb.cmd = cmd;
>  
> -	reflog_expiry_prepare(refname, sha1, &cb);
> +	reflog_expiry_prepare(refname, sha1, &policy_cb);
>  	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
> -	reflog_expiry_cleanup(&cb);
> +	reflog_expiry_cleanup(&policy_cb);
>  
>  	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
>  		if (close_lock_file(&reflog_lock)) {
> @@ -463,7 +472,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  					strerror(errno));
>  		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
>  			(write_in_full(lock->lock_fd,
> -				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
> +				sha1_to_hex(policy_cb.last_kept_sha1), 40) != 40 ||
>  			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
>  			 close_ref(lock) < 0)) {
>  			status |= error("Couldn't write %s",
> -- 
> 2.1.3
> 

> Add a flags field to "struct expire_reflog_cb", and pass the flags
> argument through to expire_reflog_ent(). In a moment we will start
> using it to pass through flags that expire_reflog_ent() needs.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

> ---
>  builtin/reflog.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 5dfa53a..1512b67 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -46,6 +46,7 @@ struct expire_reflog_policy_cb {
>  };
>  
>  struct expire_reflog_cb {
> +	unsigned int flags;
>  	void *policy_cb;
>  };
>  
> @@ -437,6 +438,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  
>  	memset(&cb, 0, sizeof(cb));
>  	memset(&policy_cb, 0, sizeof(policy_cb));
> +	cb.flags = flags;
>  	cb.policy_cb = &policy_cb;
>  
>  	/*
> -- 
> 2.1.3
> 

> ---
>  builtin/reflog.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 1512b67..cc7a220 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -20,11 +20,16 @@ static const char reflog_delete_usage[] =
>  static unsigned long default_reflog_expire;
>  static unsigned long default_reflog_expire_unreachable;
>  
> +enum expire_reflog_flags {
> +	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
> +	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
> +	EXPIRE_REFLOGS_VERBOSE = 1 << 2
> +};
> +
>  struct cmd_reflog_expire_cb {
>  	struct rev_info revs;
>  	int stalefix;
>  	int rewrite;
> -	int verbose;
>  	unsigned long expire_total;
>  	unsigned long expire_unreachable;
>  	int recno;
> @@ -339,7 +344,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
>  				     message, policy_cb)) {
>  		if (!policy_cb->newlog)
>  			printf("would prune %s", message);
> -		else if (policy_cb->cmd->verbose)
> +		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
>  			printf("prune %s", message);
>  	} else {
>  		if (policy_cb->newlog) {
> @@ -351,7 +356,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
>  				message);
>  			hashcpy(policy_cb->last_kept_sha1, nsha1);
>  		}
> -		if (policy_cb->cmd->verbose)
> +		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
>  			printf("keep %s", message);
>  	}
>  	return 0;
> @@ -421,11 +426,6 @@ static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
>  
>  static struct lock_file reflog_lock;
>  
> -enum expire_reflog_flags {
> -	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
> -	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1
> -};
> -
>  static int expire_reflog(const char *refname, const unsigned char *sha1,
>  			 unsigned int flags, void *cb_data)
>  {
> @@ -679,7 +679,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  		else if (!strcmp(arg, "--all"))
>  			do_all = 1;
>  		else if (!strcmp(arg, "--verbose"))
> -			cb.verbose = 1;
> +			flags |= EXPIRE_REFLOGS_VERBOSE;
>  		else if (!strcmp(arg, "--")) {
>  			i++;
>  			break;
> @@ -697,10 +697,10 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  	 */
>  	if (cb.stalefix) {
>  		init_revisions(&cb.revs, prefix);
> -		if (cb.verbose)
> +		if (flags & EXPIRE_REFLOGS_VERBOSE)
>  			printf("Marking reachable objects...");
>  		mark_reachable_objects(&cb.revs, 0, 0, NULL);
> -		if (cb.verbose)
> +		if (flags & EXPIRE_REFLOGS_VERBOSE)
>  			putchar('\n');
>  	}
>  
> @@ -759,7 +759,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  		else if (!strcmp(arg, "--updateref"))
>  			flags |= EXPIRE_REFLOGS_UPDATE_REF;
>  		else if (!strcmp(arg, "--verbose"))
> -			cb.verbose = 1;
> +			flags |= EXPIRE_REFLOGS_VERBOSE;
>  		else if (!strcmp(arg, "--")) {
>  			i++;
>  			break;
> -- 
> 2.1.3
> 

> ---
>  builtin/reflog.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index cc7a220..6294406 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -23,13 +23,13 @@ static unsigned long default_reflog_expire_unreachable;
>  enum expire_reflog_flags {
>  	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
>  	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
> -	EXPIRE_REFLOGS_VERBOSE = 1 << 2
> +	EXPIRE_REFLOGS_VERBOSE = 1 << 2,
> +	EXPIRE_REFLOGS_REWRITE = 1 << 3
>  };
>  
>  struct cmd_reflog_expire_cb {
>  	struct rev_info revs;
>  	int stalefix;
> -	int rewrite;
>  	unsigned long expire_total;
>  	unsigned long expire_unreachable;
>  	int recno;
> @@ -337,7 +337,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
>  	struct expire_reflog_cb *cb = cb_data;
>  	struct expire_reflog_policy_cb *policy_cb = cb->policy_cb;
>  
> -	if (policy_cb->cmd->rewrite)
> +	if (cb->flags & EXPIRE_REFLOGS_REWRITE)
>  		osha1 = policy_cb->last_kept_sha1;
>  
>  	if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
> @@ -673,7 +673,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  		else if (!strcmp(arg, "--stale-fix"))
>  			cb.stalefix = 1;
>  		else if (!strcmp(arg, "--rewrite"))
> -			cb.rewrite = 1;
> +			flags |= EXPIRE_REFLOGS_REWRITE;
>  		else if (!strcmp(arg, "--updateref"))
>  			flags |= EXPIRE_REFLOGS_UPDATE_REF;
>  		else if (!strcmp(arg, "--all"))
> @@ -755,7 +755,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
>  			flags |= EXPIRE_REFLOGS_DRY_RUN;
>  		else if (!strcmp(arg, "--rewrite"))
> -			cb.rewrite = 1;
> +			flags |= EXPIRE_REFLOGS_REWRITE;
>  		else if (!strcmp(arg, "--updateref"))
>  			flags |= EXPIRE_REFLOGS_UPDATE_REF;
>  		else if (!strcmp(arg, "--verbose"))
> -- 
> 2.1.3
> 

> ---
>  builtin/reflog.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 6294406..01b76d0 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -36,7 +36,6 @@ struct cmd_reflog_expire_cb {
>  };
>  
>  struct expire_reflog_policy_cb {
> -	FILE *newlog;
>  	enum {
>  		UE_NORMAL,
>  		UE_ALWAYS,
> @@ -45,7 +44,6 @@ struct expire_reflog_policy_cb {
>  	struct commit_list *mark_list;
>  	unsigned long mark_limit;
>  	struct cmd_reflog_expire_cb *cmd;
> -	unsigned char last_kept_sha1[20];
>  	struct commit *tip_commit;
>  	struct commit_list *tips;
>  };
> @@ -53,6 +51,8 @@ struct expire_reflog_policy_cb {
>  struct expire_reflog_cb {
>  	unsigned int flags;
>  	void *policy_cb;
> +	FILE *newlog;
> +	unsigned char last_kept_sha1[20];
>  };
>  
>  struct collected_reflog {
> @@ -338,23 +338,23 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
>  	struct expire_reflog_policy_cb *policy_cb = cb->policy_cb;
>  
>  	if (cb->flags & EXPIRE_REFLOGS_REWRITE)
> -		osha1 = policy_cb->last_kept_sha1;
> +		osha1 = cb->last_kept_sha1;
>  
>  	if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
>  				     message, policy_cb)) {
> -		if (!policy_cb->newlog)
> +		if (!cb->newlog)
>  			printf("would prune %s", message);
>  		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
>  			printf("prune %s", message);
>  	} else {
> -		if (policy_cb->newlog) {
> +		if (cb->newlog) {
>  			char sign = (tz < 0) ? '-' : '+';
>  			int zone = (tz < 0) ? (-tz) : tz;
> -			fprintf(policy_cb->newlog, "%s %s %s %lu %c%04d\t%s",
> +			fprintf(cb->newlog, "%s %s %s %lu %c%04d\t%s",
>  				sha1_to_hex(osha1), sha1_to_hex(nsha1),
>  				email, timestamp, sign, zone,
>  				message);
> -			hashcpy(policy_cb->last_kept_sha1, nsha1);
> +			hashcpy(cb->last_kept_sha1, nsha1);
>  		}
>  		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
>  			printf("keep %s", message);
> @@ -457,8 +457,8 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
>  		if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
>  			goto failure;
> -		policy_cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> -		if (!policy_cb.newlog)
> +		cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> +		if (!cb.newlog)
>  			goto failure;
>  	}
>  
> @@ -474,7 +474,7 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  					strerror(errno));
>  		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
>  			(write_in_full(lock->lock_fd,
> -				sha1_to_hex(policy_cb.last_kept_sha1), 40) != 40 ||
> +				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
>  			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
>  			 close_ref(lock) < 0)) {
>  			status |= error("Couldn't write %s",
> -- 
> 2.1.3
> 

> Now that expire_reflog() doesn't actually look in the
> expire_reflog_policy_cb data structure, we can make it opaque:
> 
> * Change its callers to pass it a pointer to an entire "struct
>   expire_reflog_policy_cb".
> 
> * Change it to pass the pointer through as a "void *".
> 
> * Change the policy functions, reflog_expiry_prepare(),
>   reflog_expiry_cleanup(), and should_expire_reflog_ent(), to accept
>   "void *cb_data" arguments and cast them to "struct
>   expire_reflog_policy_cb" internally.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

> ---
>  builtin/reflog.c | 73 ++++++++++++++++++++++++++++----------------------------
>  1 file changed, 36 insertions(+), 37 deletions(-)
> 
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 01b76d0..c30936bb 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -43,7 +43,7 @@ struct expire_reflog_policy_cb {
>  	} unreachable_expire_kind;
>  	struct commit_list *mark_list;
>  	unsigned long mark_limit;
> -	struct cmd_reflog_expire_cb *cmd;
> +	struct cmd_reflog_expire_cb cmd;
>  	struct commit *tip_commit;
>  	struct commit_list *tips;
>  };
> @@ -309,22 +309,22 @@ static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
>  	struct expire_reflog_policy_cb *cb = cb_data;
>  	struct commit *old, *new;
>  
> -	if (timestamp < cb->cmd->expire_total)
> +	if (timestamp < cb->cmd.expire_total)
>  		return 1;
>  
>  	old = new = NULL;
> -	if (cb->cmd->stalefix &&
> +	if (cb->cmd.stalefix &&
>  	    (!keep_entry(&old, osha1) || !keep_entry(&new, nsha1)))
>  		return 1;
>  
> -	if (timestamp < cb->cmd->expire_unreachable) {
> +	if (timestamp < cb->cmd.expire_unreachable) {
>  		if (cb->unreachable_expire_kind == UE_ALWAYS)
>  			return 1;
>  		if (unreachable(cb, old, osha1) || unreachable(cb, new, nsha1))
>  			return 1;
>  	}
>  
> -	if (cb->cmd->recno && --(cb->cmd->recno) == 0)
> +	if (cb->cmd.recno && --(cb->cmd.recno) == 0)
>  		return 1;
>  
>  	return 0;
> @@ -378,9 +378,11 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1,
>  
>  static void reflog_expiry_prepare(const char *refname,
>  				  const unsigned char *sha1,
> -				  struct expire_reflog_policy_cb *cb)
> +				  void *cb_data)
>  {
> -	if (!cb->cmd->expire_unreachable || !strcmp(refname, "HEAD")) {
> +	struct expire_reflog_policy_cb *cb = cb_data;
> +
> +	if (!cb->cmd.expire_unreachable || !strcmp(refname, "HEAD")) {
>  		cb->tip_commit = NULL;
>  		cb->unreachable_expire_kind = UE_HEAD;
>  	} else {
> @@ -391,7 +393,7 @@ static void reflog_expiry_prepare(const char *refname,
>  			cb->unreachable_expire_kind = UE_NORMAL;
>  	}
>  
> -	if (cb->cmd->expire_unreachable <= cb->cmd->expire_total)
> +	if (cb->cmd.expire_unreachable <= cb->cmd.expire_total)
>  		cb->unreachable_expire_kind = UE_ALWAYS;
>  
>  	cb->mark_list = NULL;
> @@ -405,13 +407,15 @@ static void reflog_expiry_prepare(const char *refname,
>  		} else {
>  			commit_list_insert(cb->tip_commit, &cb->mark_list);
>  		}
> -		cb->mark_limit = cb->cmd->expire_total;
> +		cb->mark_limit = cb->cmd.expire_total;
>  		mark_reachable(cb);
>  	}
>  }
>  
> -static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
> +static void reflog_expiry_cleanup(void *cb_data)
>  {
> +	struct expire_reflog_policy_cb *cb = cb_data;
> +
>  	if (cb->unreachable_expire_kind != UE_ALWAYS) {
>  		if (cb->unreachable_expire_kind == UE_HEAD) {
>  			struct commit_list *elem;
> @@ -427,19 +431,16 @@ static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
>  static struct lock_file reflog_lock;
>  
>  static int expire_reflog(const char *refname, const unsigned char *sha1,
> -			 unsigned int flags, void *cb_data)
> +			 unsigned int flags, void *policy_cb_data)
>  {
> -	struct cmd_reflog_expire_cb *cmd = cb_data;
>  	struct expire_reflog_cb cb;
> -	struct expire_reflog_policy_cb policy_cb;
>  	struct ref_lock *lock;
>  	char *log_file;
>  	int status = 0;
>  
>  	memset(&cb, 0, sizeof(cb));
> -	memset(&policy_cb, 0, sizeof(policy_cb));
>  	cb.flags = flags;
> -	cb.policy_cb = &policy_cb;
> +	cb.policy_cb = policy_cb_data;
>  
>  	/*
>  	 * we take the lock for the ref itself to prevent it from
> @@ -462,11 +463,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>  			goto failure;
>  	}
>  
> -	policy_cb.cmd = cmd;
> -
> -	reflog_expiry_prepare(refname, sha1, &policy_cb);
> +	reflog_expiry_prepare(refname, sha1, cb.policy_cb);
>  	for_each_reflog_ent(refname, expire_reflog_ent, &cb);
> -	reflog_expiry_cleanup(&policy_cb);
> +	reflog_expiry_cleanup(cb.policy_cb);
>  
>  	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
>  		if (close_lock_file(&reflog_lock)) {
> @@ -639,7 +638,7 @@ static void set_reflog_expiry_param(struct cmd_reflog_expire_cb *cb, int slot, c
>  
>  static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  {
> -	struct cmd_reflog_expire_cb cb;
> +	struct expire_reflog_policy_cb cb;
>  	unsigned long now = time(NULL);
>  	int i, status, do_all;
>  	int explicit_expiry = 0;
> @@ -653,25 +652,25 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  	do_all = status = 0;
>  	memset(&cb, 0, sizeof(cb));
>  
> -	cb.expire_total = default_reflog_expire;
> -	cb.expire_unreachable = default_reflog_expire_unreachable;
> +	cb.cmd.expire_total = default_reflog_expire;
> +	cb.cmd.expire_unreachable = default_reflog_expire_unreachable;
>  
>  	for (i = 1; i < argc; i++) {
>  		const char *arg = argv[i];
>  		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
>  			flags |= EXPIRE_REFLOGS_DRY_RUN;
>  		else if (starts_with(arg, "--expire=")) {
> -			if (parse_expiry_date(arg + 9, &cb.expire_total))
> +			if (parse_expiry_date(arg + 9, &cb.cmd.expire_total))
>  				die(_("'%s' is not a valid timestamp"), arg);
>  			explicit_expiry |= EXPIRE_TOTAL;
>  		}
>  		else if (starts_with(arg, "--expire-unreachable=")) {
> -			if (parse_expiry_date(arg + 21, &cb.expire_unreachable))
> +			if (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))
>  				die(_("'%s' is not a valid timestamp"), arg);
>  			explicit_expiry |= EXPIRE_UNREACH;
>  		}
>  		else if (!strcmp(arg, "--stale-fix"))
> -			cb.stalefix = 1;
> +			cb.cmd.stalefix = 1;
>  		else if (!strcmp(arg, "--rewrite"))
>  			flags |= EXPIRE_REFLOGS_REWRITE;
>  		else if (!strcmp(arg, "--updateref"))
> @@ -695,11 +694,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  	 * even in older repository.  We cannot trust what's reachable
>  	 * from reflog if the repository was pruned with older git.
>  	 */
> -	if (cb.stalefix) {
> -		init_revisions(&cb.revs, prefix);
> +	if (cb.cmd.stalefix) {
> +		init_revisions(&cb.cmd.revs, prefix);
>  		if (flags & EXPIRE_REFLOGS_VERBOSE)
>  			printf("Marking reachable objects...");
> -		mark_reachable_objects(&cb.revs, 0, 0, NULL);
> +		mark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);
>  		if (flags & EXPIRE_REFLOGS_VERBOSE)
>  			putchar('\n');
>  	}
> @@ -712,7 +711,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  		for_each_reflog(collect_reflog, &collected);
>  		for (i = 0; i < collected.nr; i++) {
>  			struct collected_reflog *e = collected.e[i];
> -			set_reflog_expiry_param(&cb, explicit_expiry, e->reflog);
> +			set_reflog_expiry_param(&cb.cmd, explicit_expiry, e->reflog);
>  			status |= expire_reflog(e->reflog, e->sha1, flags, &cb);
>  			free(e);
>  		}
> @@ -726,7 +725,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>  			status |= error("%s points nowhere!", argv[i]);
>  			continue;
>  		}
> -		set_reflog_expiry_param(&cb, explicit_expiry, ref);
> +		set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
>  		status |= expire_reflog(ref, sha1, flags, &cb);
>  	}
>  	return status;
> @@ -736,15 +735,15 @@ static int count_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
>  		const char *email, unsigned long timestamp, int tz,
>  		const char *message, void *cb_data)
>  {
> -	struct cmd_reflog_expire_cb *cb = cb_data;
> -	if (!cb->expire_total || timestamp < cb->expire_total)
> -		cb->recno++;
> +	struct expire_reflog_policy_cb *cb = cb_data;
> +	if (!cb->cmd.expire_total || timestamp < cb->cmd.expire_total)
> +		cb->cmd.recno++;
>  	return 0;
>  }
>  
>  static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  {
> -	struct cmd_reflog_expire_cb cb;
> +	struct expire_reflog_policy_cb cb;
>  	int i, status = 0;
>  	unsigned int flags = 0;
>  
> @@ -791,12 +790,12 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  
>  		recno = strtoul(spec + 2, &ep, 10);
>  		if (*ep == '}') {
> -			cb.recno = -recno;
> +			cb.cmd.recno = -recno;
>  			for_each_reflog_ent(ref, count_reflog_ent, &cb);
>  		} else {
> -			cb.expire_total = approxidate(spec + 2);
> +			cb.cmd.expire_total = approxidate(spec + 2);
>  			for_each_reflog_ent(ref, count_reflog_ent, &cb);
> -			cb.expire_total = 0;
> +			cb.cmd.expire_total = 0;
>  		}
>  
>  		status |= expire_reflog(ref, sha1, flags, &cb);
> -- 
> 2.1.3
> 

> Move expire_reflog() into refs.c and rename it to reflog_expire().
> Turn the three policy functions into function pointers that are passed
> into reflog_expire(). Add function prototypes and documentation to
> refs.h.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

> ---
>  builtin/reflog.c | 133 +++++++------------------------------------------------
>  refs.c           | 114 +++++++++++++++++++++++++++++++++++++++++++++++
>  refs.h           |  45 +++++++++++++++++++
>  3 files changed, 174 insertions(+), 118 deletions(-)
> 

> +enum expire_reflog_flags {
> +	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
> +	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
> +	EXPIRE_REFLOGS_VERBOSE = 1 << 2,
> +	EXPIRE_REFLOGS_REWRITE = 1 << 3
> +};

> On Fri, Dec 05, 2014 at 12:08:32AM +0100, Michael Haggerty wrote:
>> Move expire_reflog() into refs.c and rename it to reflog_expire().
>> Turn the three policy functions into function pointers that are passed
>> into reflog_expire(). Add function prototypes and documentation to
>> refs.h.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> 
> With or without the nits fixed
> Reviewed-by: Stefan Beller <sbeller@google.com>
> as the nits are not degrading functionality.
> 
>> ---
>>  builtin/reflog.c | 133 +++++++------------------------------------------------
>>  refs.c           | 114 +++++++++++++++++++++++++++++++++++++++++++++++
>>  refs.h           |  45 +++++++++++++++++++
>>  3 files changed, 174 insertions(+), 118 deletions(-)
>>
> 
> 
> 
>> +static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
>> +		const char *email, unsigned long timestamp, int tz,
>> +		const char *message, void *cb_data)
> 
> Nit: According to our Codingguidelines we want to indent it further, so it aligns with
> the arguments from the first line.

> +static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
> +                             const char *email, unsigned long timestamp, int tz,
> +                             const char *message, void *cb_data)
> 
>> +	}
>> +	return 0;
> 
> Why do we need the return value for expire_reflog_ent?
> The "return 0:" at the very end of the function is the only return I see here.

>> +enum expire_reflog_flags {
>> +	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
>> +	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
>> +	EXPIRE_REFLOGS_VERBOSE = 1 << 2,
>> +	EXPIRE_REFLOGS_REWRITE = 1 << 3
>> +};
> 
> Sometimes we align the assigned numbers and sometimes we don't in git, so an alternative would be
> 
> enum expire_reflog_flags {
>      EXPIRE_REFLOGS_DRY_RUN    = 1 << 0,
>      EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
>      EXPIRE_REFLOGS_VERBOSE    = 1 << 2,
>      EXPIRE_REFLOGS_REWRITE    = 1 << 3
> }
> 
> Do we have a preference in the coding style on this one?

>> + *
>> + * reflog_expiry_select_fn -- Called once for each entry in the
>> + *     existing reflog. It should return true iff that entry should be
>> + *     pruned.
> 
> Also I know how we got here, I wonder if we should inverse the logic here
> (in a later patch). "select" sounds to me as if the line is selected to keep it.
> However the opposite is true. To actually select (keep) the line we need to return
> 0. Would it make sense to rename this to reflog_expiry_should_prune_fn ?

> On 12/09/2014 12:32 AM, Stefan Beller wrote:
> >> +enum expire_reflog_flags {
> >> +	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
> >> +	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
> >> +	EXPIRE_REFLOGS_VERBOSE = 1 << 2,
> >> +	EXPIRE_REFLOGS_REWRITE = 1 << 3
> >> +};
> > 
> > Sometimes we align the assigned numbers and sometimes we don't in git, so an alternative would be
> > 
> > enum expire_reflog_flags {
> >      EXPIRE_REFLOGS_DRY_RUN    = 1 << 0,
> >      EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
> >      EXPIRE_REFLOGS_VERBOSE    = 1 << 2,
> >      EXPIRE_REFLOGS_REWRITE    = 1 << 3
> > }
> > 
> > Do we have a preference in the coding style on this one?

> Both styles are used in our codebase, and I don't think the style guide
> says anything about it. My practice in such cases is:
> 
> * If I'm modifying existing code, preserve the existing style (to avoid
> unnecessary churn)
> * If most of our code uses one style, then use that style
> * If our code uses both styles frequently, just use whatever style looks
> better to me

>> > enum expire_reflog_flags {
>> >      EXPIRE_REFLOGS_DRY_RUN    = 1 << 0,
>> >      EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
>> >      EXPIRE_REFLOGS_VERBOSE    = 1 << 2,
>> >      EXPIRE_REFLOGS_REWRITE    = 1 << 3
>> > }
>> > 
>> > Do we have a preference in the coding style on this one?
>
> I think vertically aligned lists look really nice. But they often wreak
> havoc with diffs, because introducing one longer line means re-aligning
> the whole thing. IMHO, it's not worth it (but if you're going to do it,
> leave lots of extra room for expansion).
>
> Just my two cents, of course. I don't recall this particular style point
> coming up before.
>
>> Both styles are used in our codebase, and I don't think the style guide
>> says anything about it. My practice in such cases is:
>> 
>> * If I'm modifying existing code, preserve the existing style (to avoid
>> unnecessary churn)
>> * If most of our code uses one style, then use that style
>> * If our code uses both styles frequently, just use whatever style looks
>> better to me
>
> I think that is a very good philosophy in general.

> From: Ronnie Sahlberg <sahlberg@google.com>
> 
> Inline the function at its one remaining caller (which is within
> refs.c) and remove it.
> 

> ---
>  refs.c | 9 +--------
>  refs.h | 9 +--------
>  2 files changed, 2 insertions(+), 16 deletions(-)
> 

> On Fri, Dec 05, 2014 at 12:08:34AM +0100, Michael Haggerty wrote:
>> From: Ronnie Sahlberg <sahlberg@google.com>
>>
>> Inline the function at its one remaining caller (which is within
>> refs.c) and remove it.
>>
> 
> 
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> 
> It's originally from Ronnie, but his sign off is missing?
> 
> If that sign off is found again, 
> Reviewed-by: Stefan Beller <sbeller@google.com>

> On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty <mhagger@alum.mit.edu>
> wrote:
> 
> > We don't actually need the locking functionality, because we already
> > hold the lock on the reference itself,
> 
> 
> No. You do need the lock.
> The ref is locked only during transaction_commit()
> 

> If you don't want to lock the reflog file and instead rely on the lock on
> the ref itself you will need to
> rework your patches so that the lock on the ref is taken already during,
> for example, transaction_update_ref() instead.
> 
> But without doing those changes and moving the ref locking from _commit()
> to _update_ref() you will risk reflog corruption/surprises
> if two operations collide and both rewrite the reflog without any lock held.
> 
> 
> 
> 
> 
> > which is how the reflog file is
> > locked. But the lock_file code still does some of the bookkeeping for
> > us and is more careful than the old code here was.
> >
> > For example:
> >
> > * Correctly handle the case that the reflog lock file already exists
> >   for some reason or cannot be opened.
> >
> > * Correctly clean up the lockfile if the program dies.
> >
> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> > ---
> >  builtin/reflog.c | 37 ++++++++++++++++++++++---------------
> >  1 file changed, 22 insertions(+), 15 deletions(-)
> >
> > diff --git a/builtin/reflog.c b/builtin/reflog.c
> > index a282e60..d344d45 100644
> > --- a/builtin/reflog.c
> > +++ b/builtin/reflog.c
> > @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname,
> > const unsigned char *sha1, int
> >         return 0;
> >  }
> >
> > +static struct lock_file reflog_lock;
> > +
> >  static int expire_reflog(const char *refname, const unsigned char *sha1,
> > void *cb_data)
> >  {
> >         struct cmd_reflog_expire_cb *cmd = cb_data;
> >         struct expire_reflog_cb cb;
> >         struct ref_lock *lock;
> > -       char *log_file, *newlog_path = NULL;
> > +       char *log_file;
> >         struct commit *tip_commit;
> >         struct commit_list *tips;
> >         int status = 0;
> > @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const
> > unsigned char *sha1, void *c
> >                 unlock_ref(lock);
> >                 return 0;
> >         }
> > +
> >         log_file = git_pathdup("logs/%s", refname);
> >         if (!cmd->dry_run) {
> > -               newlog_path = git_pathdup("logs/%s.lock", refname);
> > -               cb.newlog = fopen(newlog_path, "w");
> > +               if (hold_lock_file_for_update(&reflog_lock, log_file, 0) <
> > 0)
> > +                       goto failure;
> > +               cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> > +               if (!cb.newlog)
> > +                       goto failure;
> >         }
> >
> >         cb.cmd = cmd;
> > @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const
> > unsigned char *sha1, void *c
> >         }
> >
> >         if (cb.newlog) {
> > -               if (fclose(cb.newlog)) {
> > -                       status |= error("%s: %s", strerror(errno),
> > -                                       newlog_path);
> > -                       unlink(newlog_path);
> > +               if (close_lock_file(&reflog_lock)) {
> > +                       status |= error("Couldn't write %s: %s", log_file,
> > +                                       strerror(errno));
> >                 } else if (cmd->updateref &&
> >                         (write_in_full(lock->lock_fd,
> >                                 sha1_to_hex(cb.last_kept_sha1), 40) != 40
> > ||
> > @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const
> > unsigned char *sha1, void *c
> >                          close_ref(lock) < 0)) {
> >                         status |= error("Couldn't write %s",
> >                                         lock->lk->filename.buf);
> > -                       unlink(newlog_path);
> > -               } else if (rename(newlog_path, log_file)) {
> > -                       status |= error("cannot rename %s to %s",
> > -                                       newlog_path, log_file);
> > -                       unlink(newlog_path);
> > +                       rollback_lock_file(&reflog_lock);
> > +               } else if (commit_lock_file(&reflog_lock)) {
> > +                       status |= error("cannot rename %s.lock to %s",
> > +                                       log_file, log_file);
> >                 } else if (cmd->updateref && commit_ref(lock)) {
> >                         status |= error("Couldn't set %s", lock->ref_name);
> > -               } else {
> > -                       adjust_shared_perm(log_file);
> >                 }
> >         }
> > -       free(newlog_path);
> >         free(log_file);
> >         unlock_ref(lock);
> >         return status;
> > +
> > + failure:
> > +       rollback_lock_file(&reflog_lock);
> > +       free(log_file);
> > +       unlock_ref(lock);
> > +       return -1;
> >  }
> >
> >  static int collect_reflog(const char *ref, const unsigned char *sha1, int
> > unused, void *cb_data)
> > --
> > 2.1.3
> >
> >