Re: [PATCH v2 09/24] refs: convert dwim_ref and expand_ref to struct object_id
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Oct 10, 2017, 02:03 UTC
- Message-ID
- <20171010020309.GQ19555@aiede.mtv.corp.google.com>
- In-Reply-To
- <20171009011132.675341-10-sandals@crustytoothpaste.net>
brian m. carlson wrote:
Show 22 quoted lines
> All of the callers of these functions just pass the hash member of a > struct object_id, so convert them to use a pointer to struct object_id > directly. > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > --- > archive.c | 2 +- > branch.c | 2 +- > builtin/fast-export.c | 2 +- > builtin/log.c | 2 +- > builtin/merge-base.c | 2 +- > builtin/merge.c | 2 +- > builtin/rev-parse.c | 2 +- > builtin/show-branch.c | 2 +- > bundle.c | 2 +- > refs.c | 14 +++++++------- > refs.h | 4 ++-- > remote.c | 3 +-- > sha1_name.c | 6 +++--- > upload-pack.c | 2 +- > wt-status.c | 2 +- > 15 files changed, 24 insertions(+), 25 deletions(-)
One worry below. I might be worrying in vain but thought I might as well ask.
> --- a/refs.c > +++ b/refs.c
[...]
Show 22 quoted lines
> -int expand_ref(const char *str, int len, unsigned char *sha1, char **ref)
> +int expand_ref(const char *str, int len, struct object_id *oid, char **ref)
> {
> const char **p, *r;
> int refs_found = 0;
> @@ -472,15 +472,15 @@ int expand_ref(const char *str, int len, unsigned char *sha1, char **ref)
>
> *ref = NULL;
> for (p = ref_rev_parse_rules; *p; p++) {
> - unsigned char sha1_from_ref[20];
> - unsigned char *this_result;
> + struct object_id oid_from_ref;
> + struct object_id *this_result;
> int flag;
>
> - this_result = refs_found ? sha1_from_ref : sha1;
> + this_result = refs_found ? &oid_from_ref : oid;
> strbuf_reset(&fullref);
> strbuf_addf(&fullref, *p, len, str);
> r = resolve_ref_unsafe(fullref.buf, RESOLVE_REF_READING,
> - this_result, &flag);
> + this_result->hash, &flag);Can this_result be NULL? Can the oid param be NULL?
Since both this and dwim_ref are non-static functions, I'd be comforted by an
if (!oid)
BUG("expand_ref: oid must be non-NULL");at the top so that mistakes in callers are caught quickly.
[...]
Show 10 quoted lines
> --- a/remote.c > +++ b/remote.c > @@ -1628,8 +1628,7 @@ static void set_merge(struct branch *ret) > if (!remote_find_tracking(remote, ret->merge[i]) || > strcmp(ret->remote_name, ".")) > continue; > - if (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]), > - oid.hash, &ref) == 1) > + if (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]), &oid, &ref) == 1) > ret->merge[i]->dst = ref;
Long line (but as discussed earlier in this series, I don't mind).
Thanks, Jonathan