Re: [PATCH 09/23] expire_reflog(): extract two policy-related functions

[Stefan Beller <sbeller@google.com>]

On Fri, Dec 05, 2014 at 12:08:21AM +0100, Michael Haggerty wrote:
> 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>
Reviewed-by: Stefan Beller <sbeller@google.com>

> ---
> 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
>