Alexander Kuleshov <[email protected]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
