threads / patch / 34629

patchreplace: forbid replacing an object with one of a different type

Subject: [PATCH] replace: forbid replacing an object with one of a different type

## tl;dr

4 messages between Aug 7, 2013 and Aug 8, 2013. Diffs are folded; open one to read it.

replies: 3people: 4as markdown or json

Christian Couder· Aug 7, 2013, 04:42 UTC · lore

Users replacing an object with one of a different type were not prevented to do so, even if it was obvious, and stated in the doc, that bad things would result from doing that.

To avoid mistakes, it is better to just forbid that though.
The doc will be updated in a later patch.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
If this patch is considered useful, I will update the doc and
maybe add tests.
 builtin/replace.c | 9 +++++++++
 1 file changed, 9 insertions(+)
Show changes to builtin/replace.c +9 −0
diff --git a/builtin/replace.c b/builtin/replace.c
index 59d3115..0246ab3 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -85,6 +85,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 			  int force)
 {
 	unsigned char object[20], prev[20], repl[20];
+	enum object_type obj_type, repl_type;
 	char ref[PATH_MAX];
 	struct ref_lock *lock;
 
@@ -100,6 +101,14 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 	if (check_refname_format(ref, 0))
 		die("'%s' is not a valid ref name.", ref);
 
+	obj_type = sha1_object_info(object, NULL);
+	repl_type = sha1_object_info(repl, NULL);
+	if (obj_type != repl_type)
+		die("Object ref '%s' is of type '%s'\n"
+		    "while replace ref '%s' is of type '%s'.",
+		    object_ref, typename(obj_type),
+		    replace_ref, typename(repl_type));
+
 	if (read_ref(ref, prev))
 		hashclr(prev);
 	else if (!force)

-- 
1.8.4.rc1.22.g132b1a0
Thomas Rast· Aug 7, 2013, 08:46 UTC · re: Christian Couder · lore

Re: [PATCH] replace: forbid replacing an object with one of a different type

Christian Couder <chriscool@tuxfamily.org> writes:
Show 9 quoted lines
> Users replacing an object with one of a different type were not
> prevented to do so, even if it was obvious, and stated in the doc,
> that bad things would result from doing that.
>
> To avoid mistakes, it is better to just forbid that though.
>
> The doc will be updated in a later patch.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

Feel free to steal some of my other email for the commit message, to write down for posterity that reverting would not really be a useful step.

The patch looks good to me.
Show 33 quoted lines
> If this patch is considered useful, I will update the doc and
> maybe add tests.
>
>  builtin/replace.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 59d3115..0246ab3 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -85,6 +85,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
>  			  int force)
>  {
>  	unsigned char object[20], prev[20], repl[20];
> +	enum object_type obj_type, repl_type;
>  	char ref[PATH_MAX];
>  	struct ref_lock *lock;
>  
> @@ -100,6 +101,14 @@ static int replace_object(const char *object_ref, const char *replace_ref,
>  	if (check_refname_format(ref, 0))
>  		die("'%s' is not a valid ref name.", ref);
>  
> +	obj_type = sha1_object_info(object, NULL);
> +	repl_type = sha1_object_info(repl, NULL);
> +	if (obj_type != repl_type)
> +		die("Object ref '%s' is of type '%s'\n"
> +		    "while replace ref '%s' is of type '%s'.",
> +		    object_ref, typename(obj_type),
> +		    replace_ref, typename(repl_type));
> +
>  	if (read_ref(ref, prev))
>  		hashclr(prev);
>  	else if (!force)
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Philip Oakley· Aug 8, 2013, 21:09 UTC · re: Christian Couder · lore

Re: [PATCH] replace: forbid replacing an object with one of a different type

From: "Christian Couder" <chriscool@tuxfamily.org>
Sent: Wednesday, August 07, 2013 5:42 AM
Show 12 quoted lines
> Users replacing an object with one of a different type were not
> prevented to do so, even if it was obvious, and stated in the doc,
> that bad things would result from doing that.
>
> To avoid mistakes, it is better to just forbid that though.
>
> The doc will be updated in a later patch.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> If this patch is considered useful, I will update the doc and
> maybe add tests.
Acked-by: Philip Oakley <philipoakley@iee.org>
Improved documentation would always be useful.
Show 24 quoted lines
>
> builtin/replace.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 59d3115..0246ab3 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -85,6 +85,7 @@ static int replace_object(const char *object_ref,
> const char *replace_ref,
>    int force)
> {
>  unsigned char object[20], prev[20], repl[20];
> + enum object_type obj_type, repl_type;
>  char ref[PATH_MAX];
>  struct ref_lock *lock;
>
> @@ -100,6 +101,14 @@ static int replace_object(const char *object_ref,
> const char *replace_ref,
>  if (check_refname_format(ref, 0))
>  die("'%s' is not a valid ref name.", ref);
>
> + obj_type = sha1_object_info(object, NULL);
> + repl_type = sha1_object_info(repl, NULL);

Do (very) large blobs have any issues here? As I understand it, such blobs, as with other smaller objects, need to be expanded to determine the type. Is there a heuristic (and is it worth it) to do a 'large object == blob' initial check to avoid such an expansion? Small objects shouldn't be an overhead.

Just a thought.
Show 5 quoted lines
> + if (obj_type != repl_type)
> + die("Object ref '%s' is of type '%s'\n"
> +     "while replace ref '%s' is of type '%s'.",
> +     object_ref, typename(obj_type),
> +     replace_ref, typename(repl_type));
Perhaps start with "Objects must be of the same type.\n"
Show 6 quoted lines
> +
>  if (read_ref(ref, prev))
>  hashclr(prev);
>  else if (!force)
> -- 
> 1.8.4.rc1.22.g132b1a0
Junio C Hamano· Aug 8, 2013, 21:38 UTC · re: Philip Oakley · lore

Re: [PATCH] replace: forbid replacing an object with one of a different type

"Philip Oakley" <philipoakley@iee.org> writes:
Show 17 quoted lines
> From: "Christian Couder" <chriscool@tuxfamily.org>
> Sent: Wednesday, August 07, 2013 5:42 AM
>> Users replacing an object with one of a different type were not
>> prevented to do so, even if it was obvious, and stated in the doc,
>> that bad things would result from doing that.
>>
>> To avoid mistakes, it is better to just forbid that though.
>>
>> The doc will be updated in a later patch.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>> If this patch is considered useful, I will update the doc and
>> maybe add tests.
>
> Acked-by: Philip Oakley <philipoakley@iee.org>
> Improved documentation would always be useful.
Will wait for a reroll with tests and doc updates, then.
Show 11 quoted lines
>> @@ -100,6 +101,14 @@ static int replace_object(const char *object_ref,
>> const char *replace_ref,
>>  if (check_refname_format(ref, 0))
>>  die("'%s' is not a valid ref name.", ref);
>>
>> + obj_type = sha1_object_info(object, NULL);
>> + repl_type = sha1_object_info(repl, NULL);
>
> Do (very) large blobs have any issues here?  As I understand it, such
> blobs, as with other smaller objects, need to be expanded to determine
> the type.

sha1_object_info() is coded to peek into only the early part of a loose object or the object header part of a packed object. Loose ones we do mmap(), but we only inflate a tiny bit (the first 32 bytes).

Show 7 quoted lines
>> + if (obj_type != repl_type)
>> + die("Object ref '%s' is of type '%s'\n"
>> +     "while replace ref '%s' is of type '%s'.",
>> +     object_ref, typename(obj_type),
>> +     replace_ref, typename(repl_type));
>
> Perhaps start with "Objects must be of the same type.\n"
Yes.

← back to recent threads