Hello Junio, Thanks for such great explanation, i understood you. I will update this patch and resend it tomorrow. Thank you. 2015-01-10 1:07 GMT+06:00 Junio C Hamano <gitster@xxxxxxxxx>: > Alexander Kuleshov <kuleshovmail@xxxxxxxxx> writes: > >> userdiff.h used in git_cat_file_config for getting textconv driver > > Yeah, but you know that I already know that when I pointed out about > e5fba602 in the message you are responding to. And your patch does > not remove it, we still need to include it; we do not need to dig > that part of the change. > > Two corrections to my message you are responding to are in order. > > I said: > >>>> I didn't dig further to find out the answer to the last question, >>>> but a patch to remove these include should explain these in its log >>>> message, I would think. > > but I think "should" was a bit too strong, especially without > explaining why. "It would have been nicer with such explanation" > is what I should have said. > > And while on the topic, I should explain why. > > Most of the time, removal of "#include" is done because we used to > include the header for a good reason (i.e. the source used to need > something that is declared in it) but with a code change to remove > the last such reference we no longer need to include it. Commit > 6d63baa4 (prio-queue: factor out compare and swap operations, > 2014-07-14) [*1*] is an example. We used to mention 'struct commit' > in the implementation of prio-queue, but the commit realizes that > the use of prio-queue does not have to be limited to queuing > commits, and removes the need to include "commit.h". > > But this clean-up patch removes #includes without doing anything > else. It is clear we _can_ remove them; the submitter of such a > patch would have made sure that the code compiles and links fine > without these includes. So "Why can we remove them?" is not a very > interesting question. > > The interesting question is "Why remove them *now*?" Why do we have > these unused #includes? Were they unnecessary from the start? Were > they necessary but during the course of the development, we did > something else that made them unnecessary and forgot to remove them? > > These are the natural questions that somebody reading a clean-up > patch like this one may ask, and that is why I think it would have > been nice if the proposed log message answered them before being > asked. > > So here is an update after I dug a bit more. > > - "exec_cmd.h" became unnecessary at b931aa5a (Call builtin ls-tree > in git-cat-file -p, 2006-05-26), when it changed an earlier code > that used to delegate tree display to "ls-tree" via the > run_command() interface (hence needing "exec_cmd.h") to a direct > call to cmd_ls_tree(). We should have removed the include in the > same commit, but we forgot to do so. > > - "diff.h" was added at e5fba602 (textconv: support for cat_file, > 2010-06-15), together with "userdiff.h", but "userdiff.h" can be > included without including "diff.h"; the header was unnecessary > from the start. > > - "tag.h" and "tree.h" was necessary since 8e440259 (Use blob_, > commit_, tag_, and tree_type throughout., 2006-04-02) as the code > used to check the type of object by comparing typename with > tree_type and tag_type (pointers to extern strings). > > 21666f1a (convert object type handling from a string to a number, > 2007-02-26) made these <type>_type strings unnecessary, and it > could have switched to include "object.h" to use typename(), but > it forgot to do so. Because "tag.h" and "tree.h" include > "object.h", it did not need to include "object.h" in order to > start using typename(). > > In today's code, we do not even have to include "object.h" after > removing these two #includes, because "builtin.h" includes > "commit.h" which in turn includes "object.h" these days. This > happened at 7b9c0a69 (git-commit-tree: make it usable from other > builtins, 2008-07-01). > > > Having said all that, what the above satisifies is mostly curiosity, > and gives whatever value Postmortems have by analysing what we could > have done better. > > It is OK to omit the postmortem and instead just say "These are no > longer used; remove them.", which was your original. So I shouldn't > have said "*should* explain". > > > [Footnote] > > *1* I pulled this randomly from "git log -Sinclude --grep=include" > output. -- _________________________ 0xAX -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html