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

2013-09-01 Thread Philip Oakley

From: "Christian Couder" 

From: "Philip Oakley" 


Sorry for not replying earlier in the series.

From: "Christian Couder" 

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).


I am not sure I understand what you are saying.

Anyway in a previous version of this patch I tried to be more explicit
about this, but Junio basically said that he found no value in
discussing this more explicitely...


I would agree that it's not worth discussing it more explicitly.

My comment was more about the direction of the line of reasoning which I 
felt was a bit Catch 22 when starting from an existing complete DAG (no 
garbage) and attempting to replace an object with another of a different 
type and still have a valid DAG.  The construction of the replacement 
items needs to be in the right order if one of the replacements is of 
the 'wrong' type (such a construction requires the creation or uses, and 
ultimately replacement of, extraneous objects that aren't (yet) in the 
DAG).


But as already been said that's a problem for the user of the --force 
option ;-)


Philip



Thanks,
Christian.



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-09-01 Thread Christian Couder
From: "Philip Oakley" 
>
> Sorry for not replying earlier in the series.
> 
> From: "Christian Couder" 
>> 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).

I am not sure I understand what you are saying.

Anyway in a previous version of this patch I tried to be more explicit
about this, but Junio basically said that he found no value in
discussing this more explicitely...

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-08-31 Thread Philip Oakley

Sorry for not replying earlier in the series.

From: "Christian Couder" 

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).



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 100755blob
   16   commit
   04   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 
Signed-off-by: Christian Couder 
---
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





--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html