diff mbox series

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

Message ID 1fd58520253420fbe870a8528540dbc9e2178e3f.1665650564.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point | expand

Commit Message

Phillip Wood Oct. 13, 2022, 8:42 a.m. UTC 
From: Phillip Wood <phillip.wood@dunelm.org.uk>

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.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Oct. 13, 2022, 4:34 p.m. UTC | #1 
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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.

OK.

> -	if (get_oid(buf.buf, &oid))
> +	if (get_oid_hex(buf.buf, &oid))
> ...
>  		return -1;
> -	if (get_oid(buf.buf, &opts->orig_head))
> +	if (get_oid_hex(buf.buf, &opts->orig_head))
Ævar Arnfjörð Bjarmason Oct. 13, 2022, 7:10 p.m. UTC | #2 
On Thu, Oct 13 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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.

This seems sensible, but isn't "full hex oid" the tip of the iceberg in
get_oid() v.s. get_oid_hex() differences? The former allowing
e.g. :-syntax, ^-syntax etc.

> @@ -431,7 +431,7 @@ 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))
> +	if (get_oid_hex(buf.buf, &oid))

The change looks sensible, maybe a commit message update + assuring
ourselves that we don't care about anyone manually manipulating these
files (e.g. a script with OID abbreviation that would "echo" to these.

>  		return error(_("could not get 'onto': '%s'"), buf.buf);
>  	opts->onto = lookup_commit_or_die(&oid, buf.buf);
>  
> @@ -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);

Not a new issue, but this error() is much more sensible than the above,
we could get "onto", we just didn't like its contents on error(), oh
well...
Junio C Hamano Oct. 13, 2022, 8:13 p.m. UTC | #3 
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Oct 13 2022, Phillip Wood via GitGitGadget wrote:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> 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.
> ...
>> @@ -431,7 +431,7 @@ 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))
>> +	if (get_oid_hex(buf.buf, &oid))
>
> The change looks sensible, maybe a commit message update + assuring
> ourselves that we don't care about anyone manually manipulating these
> files (e.g. a script with OID abbreviation that would "echo" to these.

"should contain" sufficiently conveys that already, but I do not
mind being extra clear, either.

You do not want 6c1221c99975ad3216d82de51ed980fbf327d7f8 to be
interpreted as a branch whose name is the 40-hex that points at a
wrong commit, when it appears on the state file and I think that the
proposed log message gives a reasonable explanation why we do not
want to use get_oid() and instead use get_oid_hex().  If the code
made sure that 40-hex is followed by end-of-line, that would even be
better, but it is not required.
diff mbox series

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 56e4214b441..76f83a42f49 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -431,7 +431,7 @@  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))
+	if (get_oid_hex(buf.buf, &oid))
 		return error(_("could not get 'onto': '%s'"), buf.buf);
 	opts->onto = lookup_commit_or_die(&oid, buf.buf);
 
@@ -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)))