[v4,3/8] rebase: be stricter when reading state files containing oids

Commit Message

Phillip Wood Oct. 17, 2022, 1:17 p.m. UTC

From: Phillip Wood <[e-mail]>

The state files for 'onto' and 'orig_head' should contain a full hex
oid, change the reading functions from get_oid() to get_oid_hex() to
reflect this. They should also name commits and not tags so add and use
a function that looks up a commit from an oid like
lookup_commit_reference() but without dereferencing tags.

Suggested-by: Junio C Hamano <[e-mail]>
Signed-off-by: Phillip Wood <[e-mail]>
---
 builtin/rebase.c |  8 ++++----
 commit.c         |  8 ++++++++
 commit.h         | 13 +++++++++++++
 3 files changed, 25 insertions(+), 4 deletions(-)

Comments

Junio C Hamano Oct. 17, 2022, 6:51 p.m. UTC | #1

> diff --git a/commit.c b/commit.c
> index 0db461f9735..de30b098dd3 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -59,6 +59,14 @@ struct commit *lookup_commit_or_die(const struct object_id *oid, const char *ref
>  	return c;
>  }
>  
> +struct commit *lookup_commit_object (struct repository *r,
> +				     const struct object_id *oid)

Let's lose the SP before opening parenthesis (I'll do that while
queuing unless I forget ;-).

> +{
> +	struct object *obj = parse_object(r, oid);
> +	return obj ? object_as_type(obj, OBJ_COMMIT, 0) : NULL;
> +
> +}
> +
>  struct commit *lookup_commit(struct repository *r, const struct object_id *oid)
>  {
>  	struct object *obj = lookup_object(r, oid);
> diff --git a/commit.h b/commit.h
> index 21e4d25ce78..fa39202fa6b 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -64,6 +64,19 @@ enum decoration_type {
>  void add_name_decoration(enum decoration_type type, const char *name, struct object *obj);
>  const struct name_decoration *get_name_decoration(const struct object *obj);
>  
> +/*
> + * Look up commit named by "oid" respecting replacement objects.
> + * Returns NULL if "oid" is not a commit or does not exist.
> + */
> +struct commit *lookup_commit_object(struct repository *r, const struct object_id *oid);
> +
> +/*
> + * Look up commit named by "oid" without replacement objects or
> + * checking for object existence. Returns the requested commit if it
> + * is found in the object cache, NULL if "oid" is in the object cache
> + * but is not a commit and a newly allocated unparsed commit object if
> + * "oid" is not in the object cache.
> + */
>  struct commit *lookup_commit(struct repository *r, const struct object_id *oid);

I was going to ask documenting the differences of the two API
functions, which is done here.  Good.

>  struct commit *lookup_commit_reference(struct repository *r,
>  				       const struct object_id *oid);

Phillip Wood Oct. 19, 2022, 3:32 p.m. UTC | #2

On 17/10/2022 19:51, Junio C Hamano wrote:
>> diff --git a/commit.c b/commit.c
>> index 0db461f9735..de30b098dd3 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -59,6 +59,14 @@ struct commit *lookup_commit_or_die(const struct object_id *oid, const char *ref
>>   	return c;
>>   }
>>   
>> +struct commit *lookup_commit_object (struct repository *r,
>> +				     const struct object_id *oid)
> 
> Let's lose the SP before opening parenthesis (I'll do that while
> queuing unless I forget ;-).

Sorry, thanks for fixing it up

Phillip

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 56e4214b441..06903eb6d4d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -431,9 +431,9 @@  static int read_basic_state(struct rebase_options *opts)
 	opts->head_name = starts_with(head_name.buf, "refs/") ?
 		xstrdup(head_name.buf) : NULL;
 	strbuf_release(&head_name);
-	if (get_oid(buf.buf, &oid))
-		return error(_("could not get 'onto': '%s'"), buf.buf);
-	opts->onto = lookup_commit_or_die(&oid, buf.buf);
+	if (get_oid_hex(buf.buf, &oid) ||
+	    !(opts->onto = lookup_commit_object(the_repository, &oid)))
+		return error(_("invalid onto: '%s'"), buf.buf);
 
 	/*
 	 * We always write to orig-head, but interactive rebase used to write to
@@ -448,7 +448,7 @@  static int read_basic_state(struct rebase_options *opts)
 	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
 				  READ_ONELINER_WARN_MISSING))
 		return -1;
-	if (get_oid(buf.buf, &opts->orig_head))
+	if (get_oid_hex(buf.buf, &opts->orig_head))
 		return error(_("invalid orig-head: '%s'"), buf.buf);
 
 	if (file_exists(state_dir_path("quiet", opts)))
diff --git a/commit.c b/commit.c
index 0db461f9735..de30b098dd3 100644
--- a/commit.c
+++ b/commit.c
@@ -59,6 +59,14 @@  struct commit *lookup_commit_or_die(const struct object_id *oid, const char *ref
 	return c;
 }
 
+struct commit *lookup_commit_object (struct repository *r,
+				     const struct object_id *oid)
+{
+	struct object *obj = parse_object(r, oid);
+	return obj ? object_as_type(obj, OBJ_COMMIT, 0) : NULL;
+
+}
+
 struct commit *lookup_commit(struct repository *r, const struct object_id *oid)
 {
 	struct object *obj = lookup_object(r, oid);
diff --git a/commit.h b/commit.h
index 21e4d25ce78..fa39202fa6b 100644
--- a/commit.h
+++ b/commit.h
@@ -64,6 +64,19 @@  enum decoration_type {
 void add_name_decoration(enum decoration_type type, const char *name, struct object *obj);
 const struct name_decoration *get_name_decoration(const struct object *obj);
 
+/*
+ * Look up commit named by "oid" respecting replacement objects.
+ * Returns NULL if "oid" is not a commit or does not exist.
+ */
+struct commit *lookup_commit_object(struct repository *r, const struct object_id *oid);
+
+/*
+ * Look up commit named by "oid" without replacement objects or
+ * checking for object existence. Returns the requested commit if it
+ * is found in the object cache, NULL if "oid" is in the object cache
+ * but is not a commit and a newly allocated unparsed commit object if
+ * "oid" is not in the object cache.
+ */
 struct commit *lookup_commit(struct repository *r, const struct object_id *oid);
 struct commit *lookup_commit_reference(struct repository *r,
 				       const struct object_id *oid);