{"thread":{"id":"40499","subject":"[PATCH] pretend_sha1_file(): Change return type from int to void","startedAt":"2015-10-06T12:15:04Z","lastAt":"2015-10-08T07:45:26Z","messageCount":12,"participants":["Tobias Klauser","Johannes Schindelin","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"271213","messageId":"1444133704-29571-1-git-send-email-tklauser@distanz.ch","threadId":"40499","inReplyTo":null,"subject":"[PATCH] pretend_sha1_file(): Change return type from int to void","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-06T12:15:04Z","receivedAt":"2015-10-06T12:15:04Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"prented_sha1_file() always returns 0 and its only callsite in\nbuiltin/blame.c doesn't use the return value, so change the return type\nto void.\n\nSigned-off-by: Tobias Klauser \n---\n cache.h | 2 +-\n sha1_file.c | 5 ++---\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 752031e..445853b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -970,7 +970,7 @@ extern int sha1_object_info(const unsigned char *, unsigned long *);\n extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);\n extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);\n-extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n+extern void pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n extern int git_open_noatime(const char *name);\n extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d295a32..d76b723 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2789,14 +2789,14 @@ static void *read_packed_sha1(const unsigned char *sha1,\n \treturn data;\n }\n \n-int pretend_sha1_file(void *buf, unsigned long len, enum object_type type,\n+void pretend_sha1_file(void *buf, unsigned long len, enum object_type type,\n \t\t unsigned char *sha1)\n {\n \tstruct cached_object *co;\n \n \thash_sha1_file(buf, len, typename(type), sha1);\n \tif (has_sha1_file(sha1) || find_cached_object(sha1))\n-\t\treturn 0;\n+\t\treturn;\n \tALLOC_GROW(cached_objects, cached_object_nr + 1, cached_object_alloc);\n \tco = &cached_objects[cached_object_nr++];\n \tco->size = len;\n@@ -2804,7 +2804,6 @@ int pretend_sha1_file(void *buf, unsigned long len, enum object_type type,\n \tco->buf = xmalloc(len);\n \tmemcpy(co->buf, buf, len);\n \thashcpy(co->sha1, sha1);\n-\treturn 0;\n }\n \n static void *read_object(const unsigned char *sha1, enum object_type *type,\n-- \n2.6.0\n"},{"id":"271217","messageId":"632cbcf1dc9fa45ce71693a2cfae73e4@dscho.org","threadId":"40499","inReplyTo":"1444133704-29571-1-git-send-email-tklauser@distanz.ch","subject":"Re: [PATCH] pretend_sha1_file(): Change return type from int to void","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-06T13:16:12Z","receivedAt":"2015-10-06T13:16:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Tobias,\n\nOn 2015-10-06 14:15, Tobias Klauser wrote:\n> prented_sha1_file() always returns 0 and its only callsite in\n> builtin/blame.c doesn't use the return value, so change the return type\n> to void.\n\nWhile this commit message is technically correct, it would appear that there are some things left unsaid.\n\nIs there a problem with the current code that is solved by not returning 0? If so, could you add it to the commit message? And in particular, change the oneline appropriately?\n\nCiao,\nJohannes\n"},{"id":"271229","messageId":"20151006135101.GA11304@distanz.ch","threadId":"40499","inReplyTo":"632cbcf1dc9fa45ce71693a2cfae73e4@dscho.org","subject":"Re: [PATCH] pretend_sha1_file(): Change return type from int to void","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-06T13:51:01Z","receivedAt":"2015-10-06T13:51:01Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"Hi Johannes\n\nThanks for your feedback.\n\nOn 2015-10-06 at 15:16:12 +0200, Johannes Schindelin wrote:\n> Hi Tobias,\n> \n> On 2015-10-06 14:15, Tobias Klauser wrote:\n> > prented_sha1_file() always returns 0 and its only callsite in\n> > builtin/blame.c doesn't use the return value, so change the return type\n> > to void.\n> \n> While this commit message is technically correct, it would appear that there are some things left unsaid.\n> \n> Is there a problem with the current code that is solved by not returning 0? If so, could you add it to the commit message? And in particular, change the oneline appropriately?\n\nThere's no problem with the current code other than that the return\nvalue is unused and thus unnecessary for correct funcionality. So it's\ncertainly not a functional problem but rather a cosmetic change.\n\nDoes such a change even make sense (it's one of my first patch to git,\nso I'm not really sure what your criteria in this respect are)?\n\nIf yes, would something like the following bring across the intention\nmore clearly?\n\n pretend_sha1_file() always returns 0 and its only user in\n builtin/blame.c doesn't use the returned value. Thus, the return value\n is unnecessary and the return type of pretend_sha1_file() can be\n changed to void.\n\nCheers\nTobias\n"},{"id":"271232","messageId":"ef5b20ed42ea20b2891fc3998a81f339@dscho.org","threadId":"40499","inReplyTo":"20151006135101.GA11304@distanz.ch","subject":"Re: [PATCH] pretend_sha1_file(): Change return type from int to void","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-06T14:30:36Z","receivedAt":"2015-10-06T14:30:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Tobias,\n\nOn 2015-10-06 15:51, Tobias Klauser wrote:\n\n> On 2015-10-06 at 15:16:12 +0200, Johannes Schindelin\n> wrote:\n>>\n>> On 2015-10-06 14:15, Tobias Klauser wrote:\n>> > prented_sha1_file() always returns 0 and its only callsite in\n>> > builtin/blame.c doesn't use the return value, so change the return type\n>> > to void.\n>>\n>> While this commit message is technically correct, it would appear that there are some things left unsaid.\n>>\n>> Is there a problem with the current code that is solved by not returning 0? If so, could you add it to the commit message? And in particular, change the oneline appropriately?\n> \n> There's no problem with the current code other than that the return\n> value is unused and thus unnecessary for correct funcionality. So it's\n> certainly not a functional problem but rather a cosmetic change.\n\nOkay.\n\n> Does such a change even make sense (it's one of my first patch to git,\n> so I'm not really sure what your criteria in this respect are)?\n\nWelcome!\n\nAs to the patch, I cannot speak for Junio, of course, but my preference would be to keep the return type. Traditionally, functions that can fail either die() or return an int; non-zero indicates an error. In this case, it seems that we do not have any condition (yet...) under which an error could occur. It does not seem very unlikely that we may eventually have such conditions, though, hence my preference.\n\nCiao,\nJohannes\n"},{"id":"271254","messageId":"20151007081344.GC11304@distanz.ch","threadId":"40499","inReplyTo":"ef5b20ed42ea20b2891fc3998a81f339@dscho.org","subject":"Re: [PATCH] pretend_sha1_file(): Change return type from int to void","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-07T08:13:48Z","receivedAt":"2015-10-07T08:13:48Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"Hi Johannes\n\nOn 2015-10-06 at 16:30:36 +0200, Johannes Schindelin wrote:\n> On 2015-10-06 15:51, Tobias Klauser wrote:\n> \n> > On 2015-10-06 at 15:16:12 +0200, Johannes Schindelin\n> > wrote:\n> >>\n> >> On 2015-10-06 14:15, Tobias Klauser wrote:\n> >> > prented_sha1_file() always returns 0 and its only callsite in\n> >> > builtin/blame.c doesn't use the return value, so change the return type\n> >> > to void.\n> >>\n> >> While this commit message is technically correct, it would appear that there are some things left unsaid.\n> >>\n> >> Is there a problem with the current code that is solved by not returning 0? If so, could you add it to the commit message? And in particular, change the oneline appropriately?\n> > \n> > There's no problem with the current code other than that the return\n> > value is unused and thus unnecessary for correct funcionality. So it's\n> > certainly not a functional problem but rather a cosmetic change.\n> \n> Okay.\n> \n> > Does such a change even make sense (it's one of my first patch to git,\n> > so I'm not really sure what your criteria in this respect are)?\n> \n> Welcome!\n> \n> As to the patch, I cannot speak for Junio, of course, but my preference would be to keep the return type. Traditionally, functions that can fail either die() or return an int; non-zero indicates an error. In this case, it seems that we do not have any condition (yet...) under which an error could occur. It does not seem very unlikely that we may eventually have such conditions, though, hence my preference.\n\nOk, I see. Thank you for your explanation. I'll wait for Junio's decision\nthen :)\n\nCheers\nTobias\n"},{"id":"271265","messageId":"xmqqfv1mvawu.fsf@gitster.mtv.corp.google.com","threadId":"40499","inReplyTo":"ef5b20ed42ea20b2891fc3998a81f339@dscho.org","subject":"Re: [PATCH] pretend_sha1_file(): Change return type from int to void","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-07T17:36:01Z","receivedAt":"2015-10-07T17:36:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> As to the patch, I cannot speak for Junio, of course, but my\n> preference would be to keep the return type. Traditionally, functions\n> that can fail either die() or return an int; non-zero indicates an\n> error. In this case, it seems that we do not have any condition\n> (yet...) under which an error could occur. It does not seem very\n> unlikely that we may eventually have such conditions, though, hence my\n> preference.\n\nAccepting Tobias's patch may have a documentation value to let the\ncallers know that the function does not give the caller any error\ndiagnosis, and it may matter a lot if this were a very frequently\nused function, but that is not exactly the case here.\n\nI do not care too deeply.\n"},{"id":"271274","messageId":"CAGZ79kZmon6xwDE2reSOjM87HfG_dqc6-Rk2KzxnePLAN=BiQw@mail.gmail.com","threadId":"40499","inReplyTo":"xmqqfv1mvawu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pretend_sha1_file(): Change return type from int to void","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-07T20:42:30Z","receivedAt":"2015-10-07T20:42:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Compare to a patch[1] I sent a while back and the discussion on it.\n\n[1] https://www.mail-archive.com/git@vger.kernel.org/msg70474.html\n"},{"id":"271276","messageId":"xmqqh9m2tm8v.fsf@gitster.mtv.corp.google.com","threadId":"40499","inReplyTo":"CAGZ79kZmon6xwDE2reSOjM87HfG_dqc6-Rk2KzxnePLAN=BiQw@mail.gmail.com","subject":"Re: [PATCH] pretend_sha1_file(): Change return type from int to void","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-07T21:14:08Z","receivedAt":"2015-10-07T21:14:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Compare to a patch[1] I sent a while back and the discussion on it.\n>\n> [1] https://www.mail-archive.com/git@vger.kernel.org/msg70474.html\n\nIt is not clear what conclusion you want others to draw from the\ncomparison, I am afraid.\n\nI am guessing that you are in favor of dropping this patch, because\n'int' that signals success or error is the most natural return type\nand meanint for this function if its callers ever start using the\nvalue as the indication of an error, just like in the old thread,\nthe return value from get_remote_heads() had the most useful type\nand the meaning for its callers if they wanted to use it.\n\nAnd if that is what you wanted to say, I fully agree with the\nconclusion.\n\nBy the way, it is not a very good comparison, though. The patch in\nthe old thread deliberately attempted to discard a useful piece of\ninformation. The information the patch in this thread attempts to\ndiscard is not so useful, as there currently is nobody that returns\nan error in the codepath. So in that sense, the patch in this\nthread to change the return value to void is a bit more justifiable\nthan the one in the old thread, I think.\n\nThanks.\n"},{"id":"271277","messageId":"xmqqa8rutlu4.fsf@gitster.mtv.corp.google.com","threadId":"40499","inReplyTo":"ef5b20ed42ea20b2891fc3998a81f339@dscho.org","subject":"Re: [PATCH] pretend_sha1_file(): Change return type from int to void","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-07T21:22:59Z","receivedAt":"2015-10-07T21:22:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> As to the patch, I cannot speak for Junio, of course, but my\n> preference would be to keep the return type. Traditionally, functions\n> that can fail either die() or return an int; non-zero indicates an\n> error. In this case, it seems that we do not have any condition\n> (yet...) under which an error could occur. It does not seem very\n> unlikely that we may eventually have such conditions, though, hence my\n> preference.\n\nPerhaps the attached is a better approach.\n\nEven though the current implementation of \"pretend\" implementation\ndoes not, future generations are allowed to make pretend_sha1_file()\nreturn failure when appropriate.\n\n builtin/blame.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 203a981..fa24f8f 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2362,7 +2362,8 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \tconvert_to_git(path, buf.buf, buf.len, &buf, 0);\n \torigin->file.ptr = buf.buf;\n \torigin->file.size = buf.len;\n-\tpretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);\n+\tif (pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1))\n+\t\tdie(\"failed to create a fake commit for the working tree version.\");\n \n \t/*\n \t * Read the current index, replace the path entry with\n"},{"id":"271278","messageId":"CAGZ79kYfcA4Atsx9zy+DNA_uhW-f91c5dLMGqhwSpEV7tPE5dA@mail.gmail.com","threadId":"40499","inReplyTo":"xmqqh9m2tm8v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pretend_sha1_file(): Change return type from int to void","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-07T21:24:24Z","receivedAt":"2015-10-07T21:24:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 7, 2015 at 2:14 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Compare to a patch[1] I sent a while back and the discussion on it.\n>>\n>> [1] https://www.mail-archive.com/git@vger.kernel.org/msg70474.html\n>\n> It is not clear what conclusion you want others to draw from the\n> comparison, I am afraid.\n\nI did not draw a conclusion. All I wanted is to point out, we've had similar\npatches before. Maybe I wanted to point out, you had a different opinion\nabout the patch I linked to than you seem to have now about this patch.\n\n>\n> I am guessing that you are in favor of dropping this patch, because\n> 'int' that signals success or error is the most natural return type\n> and meanint for this function if its callers ever start using the\n> value as the indication of an error, just like in the old thread,\n> the return value from get_remote_heads() had the most useful type\n> and the meaning for its callers if they wanted to use it.\n>\n> And if that is what you wanted to say, I fully agree with the\n> conclusion.\n\nI really did not want to say anything except for pointing out how similar\ncases were dealt with in the past. So I guess for a good comparision\nwe'd need to asses how similar the patches are. If they are similar\nit's easier to link to the old discussion instead of retyping the same\nreasons.\n\n>\n> By the way, it is not a very good comparison, though. The patch in\n> the old thread deliberately attempted to discard a useful piece of\n> information. The information the patch in this thread attempts to\n> discard is not so useful, as there currently is nobody that returns\n> an error in the codepath.\n\nIsn't that a bit picky? (old thread: the information is useful, but\nnobody uses it,\nthis thread: information is useless, and nobody uses it)\n\nSo the similarity is nobody is using the result, the difference is the\nusefulness of\nthe information provided.\n\n> So in that sense, the patch in this\n> thread to change the return value to void is a bit more justifiable\n> than the one in the old thread, I think.\n\nThat makes sense to me.\n\n>\n> Thanks.\n"},{"id":"271279","messageId":"xmqq612itlj8.fsf@gitster.mtv.corp.google.com","threadId":"40499","inReplyTo":"CAGZ79kYfcA4Atsx9zy+DNA_uhW-f91c5dLMGqhwSpEV7tPE5dA@mail.gmail.com","subject":"Re: [PATCH] pretend_sha1_file(): Change return type from int to void","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-07T21:29:31Z","receivedAt":"2015-10-07T21:29:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> By the way, it is not a very good comparison, though. The patch in\n>> the old thread deliberately attempted to discard a useful piece of\n>> information. The information the patch in this thread attempts to\n>> discard is not so useful, as there currently is nobody that returns\n>> an error in the codepath.\n>\n> Isn't that a bit picky? (old thread: the information is useful, but\n> nobody uses it,\n> this thread: information is useless, and nobody uses it)\n>\n> So the similarity is nobody is using the result, the difference is the\n> usefulness of\n> the information provided.\n\nExactly. Why is it picky?\n\nThe amount of work in the existing code that is discarded is the\namount of work it will take when somebody wants to resurrect the\ncompuation of that useful information. When you judge pros and cons\nfor a patch that discards existing code, you would need to take both\ninto account---the cost of carrying it and the future cost of having\nto resurrect it.\n"},{"id":"271297","messageId":"20151008074526.GD11304@distanz.ch","threadId":"40499","inReplyTo":"xmqqa8rutlu4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pretend_sha1_file(): Change return type from int to void","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2015-10-08T07:45:26Z","receivedAt":"2015-10-08T07:45:26Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2015-10-07 at 23:22:59 +0200, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n> > As to the patch, I cannot speak for Junio, of course, but my\n> > preference would be to keep the return type. Traditionally, functions\n> > that can fail either die() or return an int; non-zero indicates an\n> > error. In this case, it seems that we do not have any condition\n> > (yet...) under which an error could occur. It does not seem very\n> > unlikely that we may eventually have such conditions, though, hence my\n> > preference.\n> \n> Perhaps the attached is a better approach.\n> \n> Even though the current implementation of \"pretend\" implementation\n> does not, future generations are allowed to make pretend_sha1_file()\n> return failure when appropriate.\n\nFor my original patch I didn't consider that pretend_sha1_file() might\nreturn failure in the future. I was just confused by the fact that the\nreturn value was seemingly useless (but now I realize that unused !=\nuseless ;-), sorry for the noise.\n\nPlease disregard my patch and apply yours instead, if you see fit.\n\n> \n> builtin/blame.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 203a981..fa24f8f 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2362,7 +2362,8 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n> \tconvert_to_git(path, buf.buf, buf.len, &buf, 0);\n> \torigin->file.ptr = buf.buf;\n> \torigin->file.size = buf.len;\n> -\tpretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);\n> +\tif (pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1))\n> +\t\tdie(\"failed to create a fake commit for the working tree version.\");\n> \n> \t/*\n> \t * Read the current index, replace the path entry with\n> \n"}]}