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

From: Philip Oakley <philipoakley@iee.org>
Date: Aug 8, 2013, 21:09 UTC
Message-ID: <A88E12BCC58A401DBDD2EB300C1AAA2D@PhilipOakley>
In-Reply-To: <20130807044248.17464.35806.chriscool@tuxfamily.org>

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