{"thread":{"id":"38316","subject":"[PATCH] cat-file: Remove unused includes","startedAt":"2015-01-08T18:56:06Z","lastAt":"2015-01-09T19:15:27Z","messageCount":6,"participants":["Alexander Kuleshov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"254474","messageId":"1420743366-14926-1-git-send-email-kuleshovmail@gmail.com","threadId":"38316","inReplyTo":null,"subject":"[PATCH] cat-file: Remove unused includes","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-01-08T18:56:06Z","receivedAt":"2015-01-08T18:56:06Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Signed-off-by: Alexander Kuleshov \n---\n builtin/cat-file.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex f8d8129..750b5a2 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -4,12 +4,8 @@\n * Copyright (C) Linus Torvalds, 2005\n */\n #include \"cache.h\"\n-#include \"exec_cmd.h\"\n-#include \"tag.h\"\n-#include \"tree.h\"\n #include \"builtin.h\"\n #include \"parse-options.h\"\n-#include \"diff.h\"\n #include \"userdiff.h\"\n #include \"streaming.h\"\n \n-- \n2.2.1.269.g787aadb.dirty\n"},{"id":"254481","messageId":"xmqq1tn5f441.fsf@gitster.dls.corp.google.com","threadId":"38316","inReplyTo":"1420743366-14926-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: [PATCH] cat-file: Remove unused includes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-08T19:45:50Z","receivedAt":"2015-01-08T19:45:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Kuleshov writes:\n\n> Signed-off-by: Alexander Kuleshov \n> ---\n> builtin/cat-file.c | 4 ----\n> 1 file changed, 4 deletions(-)\n>\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index f8d8129..750b5a2 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -4,12 +4,8 @@\n> * Copyright (C) Linus Torvalds, 2005\n> */\n> #include \"cache.h\"\n> -#include \"exec_cmd.h\"\n> -#include \"tag.h\"\n> -#include \"tree.h\"\n> #include \"builtin.h\"\n> #include \"parse-options.h\"\n> -#include \"diff.h\"\n> #include \"userdiff.h\"\n> #include \"streaming.h\"\n\nInteresting.\n\n - \"exec_cmd.h\" became unnecessary at b931aa5a (Call builtin ls-tree\n in git-cat-file -p, 2006-05-26).\n\n - \"tag.h\" and \"tree.h\" became unnecessary at 21666f1a (convert\n object type handling from a string to a number, 2007-02-26).\n\n - \"diff.h\" was added at e5fba602 (textconv: support for cat_file,\n 2010-06-15), together with \"userdiff.h\". Was it unnecessary from\n the beginning?\n\nI didn't dig further to find out the answer to the last question,\nbut a patch to remove these include should explain these in its log\nmessage, I would think.\n\nThanks.\n"},{"id":"254488","messageId":"CANCZXo7_yUT4x5fBvwNdc7d+ew5M7yaayGE_yXaeLxiPHzK9GA@mail.gmail.com","threadId":"38316","inReplyTo":"xmqq1tn5f441.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] cat-file: Remove unused includes","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-01-08T20:21:22Z","receivedAt":"2015-01-08T20:21:22Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Will research this question and resend the patch.\n\nThank you.\n\n2015-01-09 1:45 GMT+06:00 Junio C Hamano :\n> Alexander Kuleshov writes:\n>\n>> Signed-off-by: Alexander Kuleshov \n>> ---\n>> builtin/cat-file.c | 4 ----\n>> 1 file changed, 4 deletions(-)\n>>\n>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n>> index f8d8129..750b5a2 100644\n>> --- a/builtin/cat-file.c\n>> +++ b/builtin/cat-file.c\n>> @@ -4,12 +4,8 @@\n>> * Copyright (C) Linus Torvalds, 2005\n>> */\n>> #include \"cache.h\"\n>> -#include \"exec_cmd.h\"\n>> -#include \"tag.h\"\n>> -#include \"tree.h\"\n>> #include \"builtin.h\"\n>> #include \"parse-options.h\"\n>> -#include \"diff.h\"\n>> #include \"userdiff.h\"\n>> #include \"streaming.h\"\n>\n> Interesting.\n>\n> - \"exec_cmd.h\" became unnecessary at b931aa5a (Call builtin ls-tree\n> in git-cat-file -p, 2006-05-26).\n>\n> - \"tag.h\" and \"tree.h\" became unnecessary at 21666f1a (convert\n> object type handling from a string to a number, 2007-02-26).\n>\n> - \"diff.h\" was added at e5fba602 (textconv: support for cat_file,\n> 2010-06-15), together with \"userdiff.h\". Was it unnecessary from\n> the beginning?\n>\n> I didn't dig further to find out the answer to the last question,\n> but a patch to remove these include should explain these in its log\n> message, I would think.\n>\n> Thanks.\n>\n>\n\n\n\n-- \n_________________________\n0xAX\n"},{"id":"254500","messageId":"CANCZXo4=TydW=DH9pU4aTOCZRgMnZr1inhmwNPUaDQZMMYt4Ww@mail.gmail.com","threadId":"38316","inReplyTo":"CANCZXo7_yUT4x5fBvwNdc7d+ew5M7yaayGE_yXaeLxiPHzK9GA@mail.gmail.com","subject":"Re: [PATCH] cat-file: Remove unused includes","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-01-09T08:21:30Z","receivedAt":"2015-01-09T08:21:30Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"userdiff.h used in git_cat_file_config for getting textconv driver\n\n2015-01-09 2:21 GMT+06:00 Alexander Kuleshov :\n> Will research this question and resend the patch.\n>\n> Thank you.\n>\n> 2015-01-09 1:45 GMT+06:00 Junio C Hamano :\n>> Alexander Kuleshov writes:\n>>\n>>> Signed-off-by: Alexander Kuleshov \n>>> ---\n>>> builtin/cat-file.c | 4 ----\n>>> 1 file changed, 4 deletions(-)\n>>>\n>>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n>>> index f8d8129..750b5a2 100644\n>>> --- a/builtin/cat-file.c\n>>> +++ b/builtin/cat-file.c\n>>> @@ -4,12 +4,8 @@\n>>> * Copyright (C) Linus Torvalds, 2005\n>>> */\n>>> #include \"cache.h\"\n>>> -#include \"exec_cmd.h\"\n>>> -#include \"tag.h\"\n>>> -#include \"tree.h\"\n>>> #include \"builtin.h\"\n>>> #include \"parse-options.h\"\n>>> -#include \"diff.h\"\n>>> #include \"userdiff.h\"\n>>> #include \"streaming.h\"\n>>\n>> Interesting.\n>>\n>> - \"exec_cmd.h\" became unnecessary at b931aa5a (Call builtin ls-tree\n>> in git-cat-file -p, 2006-05-26).\n>>\n>> - \"tag.h\" and \"tree.h\" became unnecessary at 21666f1a (convert\n>> object type handling from a string to a number, 2007-02-26).\n>>\n>> - \"diff.h\" was added at e5fba602 (textconv: support for cat_file,\n>> 2010-06-15), together with \"userdiff.h\". Was it unnecessary from\n>> the beginning?\n>>\n>> I didn't dig further to find out the answer to the last question,\n>> but a patch to remove these include should explain these in its log\n>> message, I would think.\n>>\n>> Thanks.\n>>\n>>\n>\n>\n>\n> --\n> _________________________\n> 0xAX\n\n\n\n-- \n_________________________\n0xAX\n"},{"id":"254507","messageId":"xmqqiogfdb8p.fsf@gitster.dls.corp.google.com","threadId":"38316","inReplyTo":"CANCZXo4=TydW=DH9pU4aTOCZRgMnZr1inhmwNPUaDQZMMYt4Ww@mail.gmail.com","subject":"Re: [PATCH] cat-file: Remove unused includes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-09T19:07:02Z","receivedAt":"2015-01-09T19:07:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Kuleshov writes:\n\n> userdiff.h used in git_cat_file_config for getting textconv driver\n\nYeah, but you know that I already know that when I pointed out about\ne5fba602 in the message you are responding to. And your patch does\nnot remove it, we still need to include it; we do not need to dig\nthat part of the change.\n\nTwo corrections to my message you are responding to are in order.\n\nI said:\n\n>>> I didn't dig further to find out the answer to the last question,\n>>> but a patch to remove these include should explain these in its log\n>>> message, I would think.\n\nbut I think \"should\" was a bit too strong, especially without\nexplaining why. \"It would have been nicer with such explanation\"\nis what I should have said.\n\nAnd while on the topic, I should explain why.\n\nMost of the time, removal of \"#include\" is done because we used to\ninclude the header for a good reason (i.e. the source used to need\nsomething that is declared in it) but with a code change to remove\nthe last such reference we no longer need to include it. Commit\n6d63baa4 (prio-queue: factor out compare and swap operations,\n2014-07-14) [*1*] is an example. We used to mention 'struct commit'\nin the implementation of prio-queue, but the commit realizes that\nthe use of prio-queue does not have to be limited to queuing\ncommits, and removes the need to include \"commit.h\".\n\nBut this clean-up patch removes #includes without doing anything\nelse. It is clear we _can_ remove them; the submitter of such a\npatch would have made sure that the code compiles and links fine\nwithout these includes. So \"Why can we remove them?\" is not a very\ninteresting question.\n\nThe interesting question is \"Why remove them *now*?\" Why do we have\nthese unused #includes? Were they unnecessary from the start? Were\nthey necessary but during the course of the development, we did\nsomething else that made them unnecessary and forgot to remove them?\n\nThese are the natural questions that somebody reading a clean-up\npatch like this one may ask, and that is why I think it would have\nbeen nice if the proposed log message answered them before being\nasked.\n\nSo here is an update after I dug a bit more.\n\n - \"exec_cmd.h\" became unnecessary at b931aa5a (Call builtin ls-tree\n in git-cat-file -p, 2006-05-26), when it changed an earlier code\n that used to delegate tree display to \"ls-tree\" via the\n run_command() interface (hence needing \"exec_cmd.h\") to a direct\n call to cmd_ls_tree(). We should have removed the include in the\n same commit, but we forgot to do so.\n\n - \"diff.h\" was added at e5fba602 (textconv: support for cat_file,\n 2010-06-15), together with \"userdiff.h\", but \"userdiff.h\" can be\n included without including \"diff.h\"; the header was unnecessary\n from the start.\n\n - \"tag.h\" and \"tree.h\" was necessary since 8e440259 (Use blob_,\n commit_, tag_, and tree_type throughout., 2006-04-02) as the code\n used to check the type of object by comparing typename with\n tree_type and tag_type (pointers to extern strings).\n\n 21666f1a (convert object type handling from a string to a number,\n 2007-02-26) made these _type strings unnecessary, and it\n could have switched to include \"object.h\" to use typename(), but\n it forgot to do so. Because \"tag.h\" and \"tree.h\" include\n \"object.h\", it did not need to include \"object.h\" in order to\n start using typename().\n\n In today's code, we do not even have to include \"object.h\" after\n removing these two #includes, because \"builtin.h\" includes\n \"commit.h\" which in turn includes \"object.h\" these days. This\n happened at 7b9c0a69 (git-commit-tree: make it usable from other\n builtins, 2008-07-01).\n\n\nHaving said all that, what the above satisifies is mostly curiosity,\nand gives whatever value Postmortems have by analysing what we could\nhave done better.\n\nIt is OK to omit the postmortem and instead just say \"These are no\nlonger used; remove them.\", which was your original. So I shouldn't\nhave said \"*should* explain\".\n\n\n[Footnote]\n\n*1* I pulled this randomly from \"git log -Sinclude --grep=include\"\n output.\n"},{"id":"254508","messageId":"CANCZXo4x_xv8qA0FLmakVHj7p4zP9f8jQ5iSKx4-EZgoAmTsAg@mail.gmail.com","threadId":"38316","inReplyTo":"xmqqiogfdb8p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] cat-file: Remove unused includes","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2015-01-09T19:15:27Z","receivedAt":"2015-01-09T19:15:27Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Hello Junio,\n\nThanks for such great explanation, i understood you. I will update\nthis patch and resend it tomorrow.\n\nThank you.\n\n2015-01-10 1:07 GMT+06:00 Junio C Hamano :\n> Alexander Kuleshov writes:\n>\n>> userdiff.h used in git_cat_file_config for getting textconv driver\n>\n> Yeah, but you know that I already know that when I pointed out about\n> e5fba602 in the message you are responding to. And your patch does\n> not remove it, we still need to include it; we do not need to dig\n> that part of the change.\n>\n> Two corrections to my message you are responding to are in order.\n>\n> I said:\n>\n>>>> I didn't dig further to find out the answer to the last question,\n>>>> but a patch to remove these include should explain these in its log\n>>>> message, I would think.\n>\n> but I think \"should\" was a bit too strong, especially without\n> explaining why. \"It would have been nicer with such explanation\"\n> is what I should have said.\n>\n> And while on the topic, I should explain why.\n>\n> Most of the time, removal of \"#include\" is done because we used to\n> include the header for a good reason (i.e. the source used to need\n> something that is declared in it) but with a code change to remove\n> the last such reference we no longer need to include it. Commit\n> 6d63baa4 (prio-queue: factor out compare and swap operations,\n> 2014-07-14) [*1*] is an example. We used to mention 'struct commit'\n> in the implementation of prio-queue, but the commit realizes that\n> the use of prio-queue does not have to be limited to queuing\n> commits, and removes the need to include \"commit.h\".\n>\n> But this clean-up patch removes #includes without doing anything\n> else. It is clear we _can_ remove them; the submitter of such a\n> patch would have made sure that the code compiles and links fine\n> without these includes. So \"Why can we remove them?\" is not a very\n> interesting question.\n>\n> The interesting question is \"Why remove them *now*?\" Why do we have\n> these unused #includes? Were they unnecessary from the start? Were\n> they necessary but during the course of the development, we did\n> something else that made them unnecessary and forgot to remove them?\n>\n> These are the natural questions that somebody reading a clean-up\n> patch like this one may ask, and that is why I think it would have\n> been nice if the proposed log message answered them before being\n> asked.\n>\n> So here is an update after I dug a bit more.\n>\n> - \"exec_cmd.h\" became unnecessary at b931aa5a (Call builtin ls-tree\n> in git-cat-file -p, 2006-05-26), when it changed an earlier code\n> that used to delegate tree display to \"ls-tree\" via the\n> run_command() interface (hence needing \"exec_cmd.h\") to a direct\n> call to cmd_ls_tree(). We should have removed the include in the\n> same commit, but we forgot to do so.\n>\n> - \"diff.h\" was added at e5fba602 (textconv: support for cat_file,\n> 2010-06-15), together with \"userdiff.h\", but \"userdiff.h\" can be\n> included without including \"diff.h\"; the header was unnecessary\n> from the start.\n>\n> - \"tag.h\" and \"tree.h\" was necessary since 8e440259 (Use blob_,\n> commit_, tag_, and tree_type throughout., 2006-04-02) as the code\n> used to check the type of object by comparing typename with\n> tree_type and tag_type (pointers to extern strings).\n>\n> 21666f1a (convert object type handling from a string to a number,\n> 2007-02-26) made these _type strings unnecessary, and it\n> could have switched to include \"object.h\" to use typename(), but\n> it forgot to do so. Because \"tag.h\" and \"tree.h\" include\n> \"object.h\", it did not need to include \"object.h\" in order to\n> start using typename().\n>\n> In today's code, we do not even have to include \"object.h\" after\n> removing these two #includes, because \"builtin.h\" includes\n> \"commit.h\" which in turn includes \"object.h\" these days. This\n> happened at 7b9c0a69 (git-commit-tree: make it usable from other\n> builtins, 2008-07-01).\n>\n>\n> Having said all that, what the above satisifies is mostly curiosity,\n> and gives whatever value Postmortems have by analysing what we could\n> have done better.\n>\n> It is OK to omit the postmortem and instead just say \"These are no\n> longer used; remove them.\", which was your original. So I shouldn't\n> have said \"*should* explain\".\n>\n>\n> [Footnote]\n>\n> *1* I pulled this randomly from \"git log -Sinclude --grep=include\"\n> output.\n\n\n\n-- \n_________________________\n0xAX\n"}]}