From: Stefan Beller <sbeller@google.com>
Date: Mon, 08 Dec 2014 22:33:30 GMT
Subject: Re: [PATCH 08/23] Extract function should_expire_reflog_ent()
Message-ID: <20141208223330.GA25562@google.com>
In-Reply-To: <1417734515-11812-9-git-send-email-mhagger@alum.mit.edu>

On Fri, Dec 05, 2014 at 12:08:20AM +0100, Michael Haggerty wrote:
> 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>

Reviewed-by: Stefan Beller <sbeller@google.com>

The comments below are just thoughts, which don't need to be
included into this commit.


> +	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);

While this commit is just shoveling code around, we don't want to introduce
changes here. So a question for a possible later follow up:
"git reflog" is listed as an ancillary manipulator, which still is porcelain.
So we maybe want to translate "[would] prune"?


> +			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);

This is fine for just moving code around and reviewing.
I send a patch on top of this one to remove the manual calculation of the 
sign and zone and let the fprintf function figure it out.