Re: [PATCH 13/17] refs: convert each_reflog_ent_fn to struct object_id

[Michael Haggerty <mhagger@alum.mit.edu>]

On 01/01/2017 08:18 PM, brian m. carlson wrote:
> Make each_reflog_ent_fn take two struct object_id pointers instead of
> two pointers to unsigned char.  Convert the various callbacks to use
> struct object_id as well.  Also, rename fsck_handle_reflog_sha1 to
> fsck_handle_reflog_oid.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/fsck.c       | 16 ++++++++--------
>  builtin/merge-base.c |  6 +++---
>  builtin/reflog.c     |  2 +-
>  reflog-walk.c        |  6 +++---
>  refs.c               | 24 ++++++++++++------------
>  refs.h               |  2 +-
>  refs/files-backend.c | 24 ++++++++++++------------
>  revision.c           | 12 ++++++------
>  sha1_name.c          |  2 +-
>  wt-status.c          |  6 +++---
>  10 files changed, 50 insertions(+), 50 deletions(-)
> 
> [...]
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index f9023939d..3da3141ee 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3113,15 +3113,15 @@ static int files_delete_reflog(struct ref_store *ref_store,
>  
>  static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *cb_data)
>  {
> -	unsigned char osha1[20], nsha1[20];
> +	struct object_id ooid, noid;
>  	char *email_end, *message;
>  	unsigned long timestamp;
>  	int tz;
>  
>  	/* old SP new SP name <email> SP time TAB msg LF */
>  	if (sb->len < 83 || sb->buf[sb->len - 1] != '\n' ||
> -	    get_sha1_hex(sb->buf, osha1) || sb->buf[40] != ' ' ||
> -	    get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' ||
> +	    get_oid_hex(sb->buf, &ooid) || sb->buf[40] != ' ' ||
> +	    get_oid_hex(sb->buf + 41, &noid) || sb->buf[81] != ' ' ||

Some magic numbers above could be converted to use constants.

>  	    !(email_end = strchr(sb->buf + 82, '>')) ||
>  	    email_end[1] != ' ' ||
>  	    !(timestamp = strtoul(email_end + 2, &message, 10)) ||
> @@ -3136,7 +3136,7 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
>  		message += 6;
>  	else
>  		message += 7;
> -	return fn(osha1, nsha1, sb->buf + 82, timestamp, tz, message, cb_data);
> +	return fn(&ooid, &noid, sb->buf + 82, timestamp, tz, message, cb_data);

Here, too.

>  }
>  
>  static char *find_beginning_of_line(char *bob, char *scan)
> [...]
> @@ -4047,14 +4047,14 @@ static int files_reflog_expire(struct ref_store *ref_store,
>  		 */
>  		int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&
>  			!(type & REF_ISSYMREF) &&
> -			!is_null_sha1(cb.last_kept_sha1);
> +			!is_null_oid(&cb.last_kept_oid);
>  
>  		if (close_lock_file(&reflog_lock)) {
>  			status |= error("couldn't write %s: %s", log_file,
>  					strerror(errno));
>  		} else if (update &&
>  			   (write_in_full(get_lock_file_fd(lock->lk),
> -				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
> +				oid_to_hex(&cb.last_kept_oid), 40) != 40 ||

More magic numbers above.

>  			    write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 ||
>  			    close_ref(lock) < 0)) {
>  			status |= error("couldn't write %s",
> [...]

I thought it would make sense to convert `struct read_ref_at_cb` in
`refs.c` to use `struct object_id` at the same time, but I see that
would require the interface to `read_ref_at()` to change. I guess it's
important to pick your battles in this campaign :-)

Michael