Re: [PATCH 3/8] Convert struct ref to use object_id.

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

I visually inspected patches 1 and 2 without finding any problems.

Regarding this patch, I saw a few functions where you could convert
local variables to "struct object_id" and then change function calls
like hashcpy() to oidcpy(). See below. I'm not sure if it makes sense to
do that in this same patch.

For that matter, it seems to me that it should be possible to change
*all* local

    unsigned char $variable[20];

to

    struct object_id $variable;

without having to change any external interfaces. I wonder whether it
would be advisable to make that change early in this transition? It
would have the advantage that during later refactorings, where, e.g., a
function needs a "struct object_id *" parameter, you would often already
have one handy.

Michael

On 06/09/2015 06:28 PM, brian m. carlson wrote:
> Use struct object_id in three fields in struct ref and convert all the
> necessary places that use it.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/clone.c        | 16 +++++++-------
>  builtin/fetch-pack.c   |  4 ++--
>  builtin/fetch.c        | 50 +++++++++++++++++++++----------------------
>  builtin/ls-remote.c    |  2 +-
>  builtin/receive-pack.c |  2 +-
>  builtin/remote.c       | 12 +++++------
>  connect.c              |  2 +-
>  fetch-pack.c           | 18 ++++++++--------
>  http-push.c            | 46 +++++++++++++++++++--------------------
>  http.c                 |  2 +-
>  remote-curl.c          | 10 ++++-----
>  remote.c               | 58 +++++++++++++++++++++++++-------------------------
>  remote.h               |  6 +++---
>  send-pack.c            | 16 +++++++-------
>  transport-helper.c     | 18 ++++++++--------
>  transport.c            | 32 ++++++++++++++--------------
>  transport.h            |  8 +++----
>  walker.c               |  2 +-
>  18 files changed, 152 insertions(+), 152 deletions(-)
> 
> [...]
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 4a6b340..19215b3 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -17,7 +17,7 @@ static void add_sought_entry_mem(struct ref ***sought, int *nr, int *alloc,
>  	unsigned char sha1[20];
>  
>  	if (namelen > 41 && name[40] == ' ' && !get_sha1_hex(name, sha1)) {
> -		hashcpy(ref->old_sha1, sha1);
> +		hashcpy(ref->old_oid.hash, sha1);
>  		name += 41;
>  		namelen -= 41;
>  	}

Variable "sha1" in this function could become "struct object_id".

> [...]
> diff --git a/connect.c b/connect.c
> index c0144d8..f8b10eb 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -165,7 +165,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  		if (!check_ref(name, flags))
>  			continue;
>  		ref = alloc_ref(buffer + 41);
> -		hashcpy(ref->old_sha1, old_sha1);
> +		hashcpy(ref->old_oid.hash, old_sha1);
>  		*list = ref;
>  		list = &ref->next;
>  		got_at_least_one_head = 1;

old_sha1 in this function could become "struct object_id". Also, this
function has a few "20" and "41" and "42" hard-coded literals.

> [...]
> diff --git a/remote-curl.c b/remote-curl.c
> index af7b678..80cb4c7 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> [...]
> @@ -814,7 +814,7 @@ static void parse_fetch(struct strbuf *buf)
>  				die("protocol error: expected sha/ref, got %s'", p);
>  
>  			ref = alloc_ref(name);
> -			hashcpy(ref->old_sha1, old_sha1);
> +			hashcpy(ref->old_oid.hash, old_sha1);
>  
>  			*list = ref;
>  			list = &ref->next;

old_sha1 in this code block could be converted to "struct object_id".

> diff --git a/remote.c b/remote.c
> index 26504b7..706d2fb 100644
> --- a/remote.c
> +++ b/remote.c
> [...]
> @@ -1131,7 +1131,7 @@ static int try_explicit_object_name(const char *name,
>  
>  	if (match) {
>  		*match = alloc_ref(name);
> -		hashcpy((*match)->new_sha1, sha1);
> +		hashcpy((*match)->new_oid.hash, sha1);
>  	}
>  	return 0;
>  }

The "sha1" variable in this function could become a "struct object_id".

> [...]
> @@ -2181,7 +2181,7 @@ static int one_local_ref(const char *refname, const struct object_id *oid,
>  
>  	len = strlen(refname) + 1;
>  	ref = xcalloc(1, sizeof(*ref) + len);
> -	hashcpy(ref->new_sha1, oid->hash);
> +	hashcpy(ref->new_oid.hash, oid->hash);

This could become oidcopy().

>  	memcpy(ref->name, refname, len);
>  	**local_tail = ref;
>  	*local_tail = &ref->next;
> [...]
> diff --git a/transport-helper.c b/transport-helper.c
> index 5d99a6b..4ca3e80 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> [...]
> @@ -883,7 +883,7 @@ static int push_refs_with_export(struct transport *transport,
>  		if (private && !get_sha1(private, sha1)) {
>  			strbuf_addf(&buf, "^%s", private);
>  			string_list_append(&revlist_args, strbuf_detach(&buf, NULL));
> -			hashcpy(ref->old_sha1, sha1);
> +			hashcpy(ref->old_oid.hash, sha1);
>  		}
>  		free(private);
>  

sha1 in this function could become "struct object_id".

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu