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

From: Philip Oakley <philipoakley@iee.org>
Date: Aug 31, 2013, 22:11 UTC
Message-ID: <7D7C5D53BA544AAD92B05DF9635926C8@PhilipOakley>
In-Reply-To: <20130831191215.26699.18957.chriscool@tuxfamily.org>

Sorry for not replying earlier in the series.

From: "Christian Couder" <chriscool@tuxfamily.org>

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.
>
> If one object is replaced with one of a different type, the only way
> to keep the history valid is to also replace all the other objects
> that point to the replaced object.

Isn't this a recursion problem? Taken in that order one unravels the whole DAG.

However if considered in the reverse direction, one can replace an existing object within the DAG with a carefully crafted alternative of the same type, but which then wrongly references other dangling objects which are then replaced by objects which have the right type (this last replacement requires -f force).

Show 58 quoted lines

> That's because:
>
> * Annotated tags contain the type of the tagged object.
>
> * The tree/parent lines in commits must be a tree and commits, resp.
>
> * The object types referred to by trees are specified in the 'mode'
>  field:
>    100644 and 100755    blob
>    160000               commit
>    040000               tree
>  (these are the only valid modes)
>
> * Blobs don't point at anything.
>
> The doc will be updated in a later patch.
>
> Acked-by: Philip Oakley <philipoakley@iee.org>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> builtin/replace.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 59d3115..9a94769 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,15 @@ 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("Objects must be of the same type.\n"
> +     "'%s' points to a replaced object of type '%s'\n"
> +     "while '%s' points to a replacement object 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.31.g530f5ce.dirty
>
>
>