{"thread":{"id":"34629","subject":"[PATCH] replace: forbid replacing an object with one of a different type","startedAt":"2013-08-07T04:42:47Z","lastAt":"2013-08-08T21:38:31Z","messageCount":4,"participants":["Christian Couder","Thomas Rast","Philip Oakley","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"224731","messageId":"20130807044248.17464.35806.chriscool@tuxfamily.org","threadId":"34629","inReplyTo":null,"subject":"[PATCH] replace: forbid replacing an object with one of a different type","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-08-07T04:42:47Z","receivedAt":"2013-08-07T04:42:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Users replacing an object with one of a different type were not\nprevented to do so, even if it was obvious, and stated in the doc,\nthat bad things would result from doing that.\n\nTo avoid mistakes, it is better to just forbid that though.\n\nThe doc will be updated in a later patch.\n\nSigned-off-by: Christian Couder \n---\nIf this patch is considered useful, I will update the doc and\nmaybe add tests.\n\n builtin/replace.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 59d3115..0246ab3 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -85,6 +85,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n \t\t\t int force)\n {\n \tunsigned char object[20], prev[20], repl[20];\n+\tenum object_type obj_type, repl_type;\n \tchar ref[PATH_MAX];\n \tstruct ref_lock *lock;\n \n@@ -100,6 +101,14 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n \tif (check_refname_format(ref, 0))\n \t\tdie(\"'%s' is not a valid ref name.\", ref);\n \n+\tobj_type = sha1_object_info(object, NULL);\n+\trepl_type = sha1_object_info(repl, NULL);\n+\tif (obj_type != repl_type)\n+\t\tdie(\"Object ref '%s' is of type '%s'\\n\"\n+\t\t \"while replace ref '%s' is of type '%s'.\",\n+\t\t object_ref, typename(obj_type),\n+\t\t replace_ref, typename(repl_type));\n+\n \tif (read_ref(ref, prev))\n \t\thashclr(prev);\n \telse if (!force)\n-- \n1.8.4.rc1.22.g132b1a0\n"},{"id":"224736","messageId":"87iozhyjki.fsf@linux-k42r.v.cablecom.net","threadId":"34629","inReplyTo":"20130807044248.17464.35806.chriscool@tuxfamily.org","subject":"Re: [PATCH] replace: forbid replacing an object with one of a different type","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-08-07T08:46:37Z","receivedAt":"2013-08-07T08:46:37Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Christian Couder writes:\n\n> Users replacing an object with one of a different type were not\n> prevented to do so, even if it was obvious, and stated in the doc,\n> that bad things would result from doing that.\n>\n> To avoid mistakes, it is better to just forbid that though.\n>\n> The doc will be updated in a later patch.\n>\n> Signed-off-by: Christian Couder \n\nFeel free to steal some of my other email for the commit message, to\nwrite down for posterity that reverting would not really be a useful\nstep.\n\nThe patch looks good to me.\n\n> If this patch is considered useful, I will update the doc and\n> maybe add tests.\n>\n> builtin/replace.c | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index 59d3115..0246ab3 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -85,6 +85,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n> \t\t\t int force)\n> {\n> \tunsigned char object[20], prev[20], repl[20];\n> +\tenum object_type obj_type, repl_type;\n> \tchar ref[PATH_MAX];\n> \tstruct ref_lock *lock;\n> \n> @@ -100,6 +101,14 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n> \tif (check_refname_format(ref, 0))\n> \t\tdie(\"'%s' is not a valid ref name.\", ref);\n> \n> +\tobj_type = sha1_object_info(object, NULL);\n> +\trepl_type = sha1_object_info(repl, NULL);\n> +\tif (obj_type != repl_type)\n> +\t\tdie(\"Object ref '%s' is of type '%s'\\n\"\n> +\t\t \"while replace ref '%s' is of type '%s'.\",\n> +\t\t object_ref, typename(obj_type),\n> +\t\t replace_ref, typename(repl_type));\n> +\n> \tif (read_ref(ref, prev))\n> \t\thashclr(prev);\n> \telse if (!force)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"224855","messageId":"A88E12BCC58A401DBDD2EB300C1AAA2D@PhilipOakley","threadId":"34629","inReplyTo":"20130807044248.17464.35806.chriscool@tuxfamily.org","subject":"Re: [PATCH] replace: forbid replacing an object with one of a different type","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-08-08T21:09:31Z","receivedAt":"2013-08-08T21:09:31Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Christian Couder\" \nSent: Wednesday, August 07, 2013 5:42 AM\n> Users replacing an object with one of a different type were not\n> prevented to do so, even if it was obvious, and stated in the doc,\n> that bad things would result from doing that.\n>\n> To avoid mistakes, it is better to just forbid that though.\n>\n> The doc will be updated in a later patch.\n>\n> Signed-off-by: Christian Couder \n> ---\n> If this patch is considered useful, I will update the doc and\n> maybe add tests.\n\nAcked-by: Philip Oakley \nImproved documentation would always be useful.\n\n>\n> builtin/replace.c | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index 59d3115..0246ab3 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -85,6 +85,7 @@ static int replace_object(const char *object_ref,\n> const char *replace_ref,\n> int force)\n> {\n> unsigned char object[20], prev[20], repl[20];\n> + enum object_type obj_type, repl_type;\n> char ref[PATH_MAX];\n> struct ref_lock *lock;\n>\n> @@ -100,6 +101,14 @@ static int replace_object(const char *object_ref,\n> const char *replace_ref,\n> if (check_refname_format(ref, 0))\n> die(\"'%s' is not a valid ref name.\", ref);\n>\n> + obj_type = sha1_object_info(object, NULL);\n> + repl_type = sha1_object_info(repl, NULL);\n\nDo (very) large blobs have any issues here? As I understand it, such\nblobs, as with other smaller objects, need to be expanded to determine\nthe type. Is there a heuristic (and is it worth it) to do a 'large\nobject == blob' initial check to avoid such an expansion? Small objects\nshouldn't be an overhead.\n\nJust a thought.\n\n> + if (obj_type != repl_type)\n> + die(\"Object ref '%s' is of type '%s'\\n\"\n> + \"while replace ref '%s' is of type '%s'.\",\n> + object_ref, typename(obj_type),\n> + replace_ref, typename(repl_type));\n\nPerhaps start with \"Objects must be of the same type.\\n\"\n\n> +\n> if (read_ref(ref, prev))\n> hashclr(prev);\n> else if (!force)\n> -- \n> 1.8.4.rc1.22.g132b1a0\n"},{"id":"224864","messageId":"7vsiyj982w.fsf@alter.siamese.dyndns.org","threadId":"34629","inReplyTo":"A88E12BCC58A401DBDD2EB300C1AAA2D@PhilipOakley","subject":"Re: [PATCH] replace: forbid replacing an object with one of a different type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-08T21:38:31Z","receivedAt":"2013-08-08T21:38:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" writes:\n\n> From: \"Christian Couder\" \n> Sent: Wednesday, August 07, 2013 5:42 AM\n>> Users replacing an object with one of a different type were not\n>> prevented to do so, even if it was obvious, and stated in the doc,\n>> that bad things would result from doing that.\n>>\n>> To avoid mistakes, it is better to just forbid that though.\n>>\n>> The doc will be updated in a later patch.\n>>\n>> Signed-off-by: Christian Couder \n>> ---\n>> If this patch is considered useful, I will update the doc and\n>> maybe add tests.\n>\n> Acked-by: Philip Oakley \n> Improved documentation would always be useful.\n\nWill wait for a reroll with tests and doc updates, then.\n\n>> @@ -100,6 +101,14 @@ static int replace_object(const char *object_ref,\n>> const char *replace_ref,\n>> if (check_refname_format(ref, 0))\n>> die(\"'%s' is not a valid ref name.\", ref);\n>>\n>> + obj_type = sha1_object_info(object, NULL);\n>> + repl_type = sha1_object_info(repl, NULL);\n>\n> Do (very) large blobs have any issues here? As I understand it, such\n> blobs, as with other smaller objects, need to be expanded to determine\n> the type.\n\nsha1_object_info() is coded to peek into only the early part of a\nloose object or the object header part of a packed object. Loose\nones we do mmap(), but we only inflate a tiny bit (the first 32\nbytes).\n\n>> + if (obj_type != repl_type)\n>> + die(\"Object ref '%s' is of type '%s'\\n\"\n>> + \"while replace ref '%s' is of type '%s'.\",\n>> + object_ref, typename(obj_type),\n>> + replace_ref, typename(repl_type));\n>\n> Perhaps start with \"Objects must be of the same type.\\n\"\n\nYes.\n"}]}