{"thread":{"id":"36238","subject":"[PATCH v3][GSOC] fsck: use bitwise-or assignment operator to set flag","startedAt":"2014-03-19T23:02:04Z","lastAt":"2014-03-20T23:27:51Z","messageCount":4,"participants":["Hiroyuki Sano","Junio C Hamano","Hiroyuki"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"237130","messageId":"1395270124-2242-1-git-send-email-sh19910711@gmail.com","threadId":"36238","inReplyTo":null,"subject":"[PATCH v3][GSOC] fsck: use bitwise-or assignment operator to set flag","fromName":"Hiroyuki Sano","fromEmail":"sh19910711@gmail.com","sentAt":"2014-03-19T23:02:04Z","receivedAt":"2014-03-19T23:02:04Z","isPatch":true,"sender":{"key":"sh19910711@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1015222?v=4"},"body":"fsck_tree() has two different ways to set a flag,\nwhich are the followings:\n\n 1. Using a if-statement that guards assignment.\n\n 2. Using a bitwise-or assignment operator.\n\nCurrently, many with the former way,\nand one with the latter way.\n\nIn this patch, unify them to the latter way,\nbecause it makes the code shorter and easier to read,\nand it is brief and to the point.\n\nSigned-off-by: Hiroyuki Sano \n---\n fsck.c | 18 ++++++------------\n 1 file changed, 6 insertions(+), 12 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex b3022ad..abed62b 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -165,18 +165,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \n \t\tsha1 = tree_entry_extract(&desc, &name, &mode);\n \n-\t\tif (is_null_sha1(sha1))\n-\t\t\thas_null_sha1 = 1;\n-\t\tif (strchr(name, '/'))\n-\t\t\thas_full_path = 1;\n-\t\tif (!*name)\n-\t\t\thas_empty_name = 1;\n-\t\tif (!strcmp(name, \".\"))\n-\t\t\thas_dot = 1;\n-\t\tif (!strcmp(name, \"..\"))\n-\t\t\thas_dotdot = 1;\n-\t\tif (!strcmp(name, \".git\"))\n-\t\t\thas_dotgit = 1;\n+\t\thas_null_sha1 |= is_null_sha1(sha1);\n+\t\thas_full_path |= !!strchr(name, '/');\n+\t\thas_empty_name |= !*name;\n+\t\thas_dot |= !strcmp(name, \".\");\n+\t\thas_dotdot |= !strcmp(name, \"..\");\n+\t\thas_dotgit |= !strcmp(name, \".git\");\n \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n \t\tupdate_tree_entry(&desc);\n \n-- \n1.9.0\n"},{"id":"237190","messageId":"xmqqtxasybnk.fsf@gitster.dls.corp.google.com","threadId":"36238","inReplyTo":"1395270124-2242-1-git-send-email-sh19910711@gmail.com","subject":"Re: [PATCH v3][GSOC] fsck: use bitwise-or assignment operator to set flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-20T18:20:47Z","receivedAt":"2014-03-20T18:20:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hiroyuki Sano writes:\n\n> fsck_tree() has two different ways to set a flag,\n> which are the followings:\n>\n> 1. Using a if-statement that guards assignment.\n>\n> 2. Using a bitwise-or assignment operator.\n>\n> Currently, many with the former way,\n> and one with the latter way.\n>\n> In this patch, unify them to the latter way,\n> because it makes the code shorter and easier to read,\n> and it is brief and to the point.\n\nTwo issues:\n\n * \"In this patch,\" is redundant.\n\n * \"it is brief and to the point\" are equally applicable to both\n styles, so that is not a *reason* to choose one over the other.\n\nIf a condition were *not* brief and to the point, then a rewrite to\nthe latter style will make the resulting code worse:\n\n\tif (a very complex condition\n that potentially have to consume a\n lot of brain-cycles to understand) {\n\t\thas_that_condition = 1;\n\t}\n\nis a lot easier to extend than\n\n\thas_that_condition = (a very complex condition\n that potentially have to consume a\n lot of brain-cycles to understand);\n\nbecause it is a lot more likely that we would need to later extend\nsuch a complex condition is more likely than a simple singleton\ncondition, and we could end up with\n\n\tif (a very complex condition\n that potentially have to consume a\n lot of brain-cycles to understand) {\n\t\tfuther computation to check if\n the condition really holds\n will be added here later\n if (does that condition really hold true?)\n\t\t\thas_that_condition = 1;\n\t}\n\n\nwhich may be harder to express in the latter form.\n\nIn other words, \"it is brief and to the point\" merely _allows_ these\nstatements to be expressed in the latter form; it does not say\nanything about which is better between the former and the latter.\n\n> Signed-off-by: Hiroyuki Sano \n> ---\n> fsck.c | 18 ++++++------------\n> 1 file changed, 6 insertions(+), 12 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index b3022ad..abed62b 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -165,18 +165,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n> \n> \t\tsha1 = tree_entry_extract(&desc, &name, &mode);\n> \n> -\t\tif (is_null_sha1(sha1))\n> -\t\t\thas_null_sha1 = 1;\n> -\t\tif (strchr(name, '/'))\n> -\t\t\thas_full_path = 1;\n> -\t\tif (!*name)\n> -\t\t\thas_empty_name = 1;\n> -\t\tif (!strcmp(name, \".\"))\n> -\t\t\thas_dot = 1;\n> -\t\tif (!strcmp(name, \"..\"))\n> -\t\t\thas_dotdot = 1;\n> -\t\tif (!strcmp(name, \".git\"))\n> -\t\t\thas_dotgit = 1;\n> +\t\thas_null_sha1 |= is_null_sha1(sha1);\n> +\t\thas_full_path |= !!strchr(name, '/');\n> +\t\thas_empty_name |= !*name;\n> +\t\thas_dot |= !strcmp(name, \".\");\n> +\t\thas_dotdot |= !strcmp(name, \"..\");\n> +\t\thas_dotgit |= !strcmp(name, \".git\");\n> \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n> \t\tupdate_tree_entry(&desc);\n"},{"id":"237193","messageId":"xmqqlhw4yaj0.fsf@gitster.dls.corp.google.com","threadId":"36238","inReplyTo":"xmqqtxasybnk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3][GSOC] fsck: use bitwise-or assignment operator to set flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-20T18:45:07Z","receivedAt":"2014-03-20T18:45:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> In other words, \"it is brief and to the point\" merely _allows_ these\n> statements to be expressed in the latter form; it does not say\n> anything about which is better between the former and the latter.\n\nIn any case, that is a minor point. I'll queue the patch on 'pu',\nwith a tweaked log message.\n\nThanks.\n"},{"id":"237239","messageId":"CA+gKU0fZ=Le3iLhyqX2s0rf-292yh7mA-_Zo6AkJBrzN1=XakQ@mail.gmail.com","threadId":"36238","inReplyTo":"xmqqtxasybnk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3][GSOC] fsck: use bitwise-or assignment operator to set flag","fromName":"Hiroyuki","fromEmail":"sh19910711@gmail.com","sentAt":"2014-03-20T23:27:51Z","receivedAt":"2014-03-20T23:27:51Z","isPatch":true,"sender":{"key":"sh19910711@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1015222?v=4"},"body":"Junio and Eric,\n\nThank you for the reviews and helpful advice.\nI should have read more past commit messages before send patch.\n\n\nRegards,\n\n\n2014-03-21 3:20 GMT+09:00 Junio C Hamano :\n> Hiroyuki Sano writes:\n>\n>> fsck_tree() has two different ways to set a flag,\n>> which are the followings:\n>>\n>> 1. Using a if-statement that guards assignment.\n>>\n>> 2. Using a bitwise-or assignment operator.\n>>\n>> Currently, many with the former way,\n>> and one with the latter way.\n>>\n>> In this patch, unify them to the latter way,\n>> because it makes the code shorter and easier to read,\n>> and it is brief and to the point.\n>\n> Two issues:\n>\n> * \"In this patch,\" is redundant.\n>\n> * \"it is brief and to the point\" are equally applicable to both\n> styles, so that is not a *reason* to choose one over the other.\n>\n> If a condition were *not* brief and to the point, then a rewrite to\n> the latter style will make the resulting code worse:\n>\n> if (a very complex condition\n> that potentially have to consume a\n> lot of brain-cycles to understand) {\n> has_that_condition = 1;\n> }\n>\n> is a lot easier to extend than\n>\n> has_that_condition = (a very complex condition\n> that potentially have to consume a\n> lot of brain-cycles to understand);\n>\n> because it is a lot more likely that we would need to later extend\n> such a complex condition is more likely than a simple singleton\n> condition, and we could end up with\n>\n> if (a very complex condition\n> that potentially have to consume a\n> lot of brain-cycles to understand) {\n> futher computation to check if\n> the condition really holds\n> will be added here later\n> if (does that condition really hold true?)\n> has_that_condition = 1;\n> }\n>\n>\n> which may be harder to express in the latter form.\n>\n> In other words, \"it is brief and to the point\" merely _allows_ these\n> statements to be expressed in the latter form; it does not say\n> anything about which is better between the former and the latter.\n>\n>> Signed-off-by: Hiroyuki Sano \n>> ---\n>> fsck.c | 18 ++++++------------\n>> 1 file changed, 6 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/fsck.c b/fsck.c\n>> index b3022ad..abed62b 100644\n>> --- a/fsck.c\n>> +++ b/fsck.c\n>> @@ -165,18 +165,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n>>\n>> sha1 = tree_entry_extract(&desc, &name, &mode);\n>>\n>> - if (is_null_sha1(sha1))\n>> - has_null_sha1 = 1;\n>> - if (strchr(name, '/'))\n>> - has_full_path = 1;\n>> - if (!*name)\n>> - has_empty_name = 1;\n>> - if (!strcmp(name, \".\"))\n>> - has_dot = 1;\n>> - if (!strcmp(name, \"..\"))\n>> - has_dotdot = 1;\n>> - if (!strcmp(name, \".git\"))\n>> - has_dotgit = 1;\n>> + has_null_sha1 |= is_null_sha1(sha1);\n>> + has_full_path |= !!strchr(name, '/');\n>> + has_empty_name |= !*name;\n>> + has_dot |= !strcmp(name, \".\");\n>> + has_dotdot |= !strcmp(name, \"..\");\n>> + has_dotgit |= !strcmp(name, \".git\");\n>> has_zero_pad |= *(char *)desc.buffer == '0';\n>> update_tree_entry(&desc);\n\n\n\n-- \nHiroyuki Sano\nsh19910711@gmail.com\n"}]}