| Message ID | 1133ae3b-81e8-4ec1-c2d7-d071e7e65ec1@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PATCH] remove duplicate #include directives | expand |
René Scharfe <l.s.r@web.de> writes: > Found with "git grep '^#include ' '*.c' | sort | uniq -d". > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > Patch formatted with --function-context for easier review. I have a mixed feelings about that. The only audience benefitted by --function-context patch are those who read the patch only in MUA without looking at anything outside and declare they are done with reviewing the patch. For something tricky, wider context does help to see what is going on, but then I'd feel uncomfortable to hear "looks good to me" from those who did not even apply the patch to their trees and looked at the changes after "reviewing" tricky stuff that requires wider context to properly review. If there were topics in flight that touch any of these include blocks, the patch would not apply and a reviewer who is interested in these fixes ends up needing to wiggle them in somehow. If a reviewer does not trust you enough to feel the need to double check, the result after applying the patches would be checked by running "diff --function-context" by the reviewer (if it is tricky and benefits from wider context) anyway. If you've earned enough trust that such a verification is not needed, the reviewer may not need to see --function-context output. So a patch that has less chance of unnecessary conflict would be easier to handle in either case. Having said all that, for _this_ particular case, I do not see a reason why a review needs to look at anywhere outside the context presented in this patch, so I'd say it is a narrow case that -W is an appropriate thing to use. I just do not want to see contributors less experienced than you (hence cannot make good judgement on when to and not to use the option) get in the habit of using -W all the time. And needless to say, the changes in the patch look good. Thanks. > diff --git a/builtin/am.c b/builtin/am.c > index ee7305eaa6..b015e1d7d1 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -1,40 +1,39 @@ > /* > * Builtin "git am" > * > * Based on git-am.sh by Junio C Hamano. > */ > #define USE_THE_INDEX_COMPATIBILITY_MACROS > #include "cache.h" > #include "config.h" > #include "builtin.h" > #include "exec-cmd.h" > #include "parse-options.h" > #include "dir.h" > #include "run-command.h" > #include "quote.h" > #include "tempfile.h" > #include "lockfile.h" > #include "cache-tree.h" > #include "refs.h" > #include "commit.h" > #include "diff.h" > #include "diffcore.h" > #include "unpack-trees.h" > #include "branch.h" > #include "sequencer.h" > #include "revision.h" > #include "merge-recursive.h" > -#include "revision.h" > #include "log-tree.h" > #include "notes-utils.h" > #include "rerere.h" > #include "prompt.h" > #include "mailinfo.h" > #include "apply.h" > #include "string-list.h" > #include "packfile.h" > #include "repository.h" > > /** > * Returns the length of the first line of msg. > */ > diff --git a/builtin/blame.c b/builtin/blame.c > index bfd537b344..9858d6b269 100644 > --- a/builtin/blame.c > +++ b/builtin/blame.c > @@ -1,32 +1,31 @@ > /* > * Blame > * > * Copyright (c) 2006, 2014 by its authors > * See COPYING for licensing conditions > */ > > #include "cache.h" > #include "config.h" > #include "color.h" > #include "builtin.h" > #include "repository.h" > #include "commit.h" > #include "diff.h" > #include "revision.h" > #include "quote.h" > #include "string-list.h" > #include "mailmap.h" > #include "parse-options.h" > #include "prio-queue.h" > #include "utf8.h" > #include "userdiff.h" > #include "line-range.h" > #include "line-log.h" > #include "dir.h" > #include "progress.h" > #include "object-store.h" > #include "blame.h" > -#include "string-list.h" > #include "refs.h" > > static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>"); > diff --git a/builtin/clone.c b/builtin/clone.c > index 2048b6760a..9d73102c42 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -1,44 +1,43 @@ > /* > * Builtin "git clone" > * > * Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>, > * 2008 Daniel Barkalow <barkalow@iabervon.org> > * Based on git-commit.sh by Junio C Hamano and Linus Torvalds > * > * Clone a repository into a different directory that does not yet exist. > */ > > #define USE_THE_INDEX_COMPATIBILITY_MACROS > #include "builtin.h" > #include "config.h" > #include "lockfile.h" > #include "parse-options.h" > #include "fetch-pack.h" > #include "refs.h" > #include "refspec.h" > #include "object-store.h" > #include "tree.h" > #include "tree-walk.h" > #include "unpack-trees.h" > #include "transport.h" > #include "strbuf.h" > #include "dir.h" > #include "dir-iterator.h" > #include "iterator.h" > #include "sigchain.h" > #include "branch.h" > #include "remote.h" > #include "run-command.h" > #include "connected.h" > #include "packfile.h" > #include "list-objects-filter-options.h" > -#include "object-store.h" > > /* > * Overall FIXMEs: > * - respect DB_ENVIRONMENT for .git/objects. > * > * Implementation notes: > * - dropping use-separate-remote and no-separate-remote compatibility > * > */ > diff --git a/builtin/describe.c b/builtin/describe.c > index e048f85484..90feab1120 100644 > --- a/builtin/describe.c > +++ b/builtin/describe.c > @@ -1,22 +1,21 @@ > #define USE_THE_INDEX_COMPATIBILITY_MACROS > #include "cache.h" > #include "config.h" > #include "lockfile.h" > #include "commit.h" > #include "tag.h" > #include "blob.h" > #include "refs.h" > #include "builtin.h" > #include "exec-cmd.h" > #include "parse-options.h" > #include "revision.h" > #include "diff.h" > #include "hashmap.h" > #include "argv-array.h" > #include "run-command.h" > #include "object-store.h" > -#include "revision.h" > #include "list-objects.h" > #include "commit-slab.h" > > #define MAX_TAGS (FLAG_BITS - 1) > diff --git a/builtin/rev-list.c b/builtin/rev-list.c > index b8dc2e1fba..fb8187fba5 100644 > --- a/builtin/rev-list.c > +++ b/builtin/rev-list.c > @@ -1,24 +1,23 @@ > #include "cache.h" > #include "config.h" > #include "commit.h" > #include "diff.h" > #include "revision.h" > #include "list-objects.h" > #include "list-objects-filter.h" > #include "list-objects-filter-options.h" > #include "object.h" > #include "object-store.h" > #include "pack.h" > #include "pack-bitmap.h" > #include "builtin.h" > #include "log-tree.h" > #include "graph.h" > #include "bisect.h" > #include "progress.h" > #include "reflog-walk.h" > #include "oidset.h" > #include "packfile.h" > -#include "object-store.h" > > static const char rev_list_usage[] = > "git rev-list [OPTION] <commit-id>... [ -- paths... ]\n" > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 7f094f8170..0a53788151 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -1,16 +1,15 @@ > #include "cache.h" > #include "checkout.h" > #include "config.h" > #include "builtin.h" > #include "dir.h" > #include "parse-options.h" > #include "argv-array.h" > #include "branch.h" > #include "refs.h" > #include "run-command.h" > #include "sigchain.h" > #include "submodule.h" > -#include "refs.h" > #include "utf8.h" > #include "worktree.h" > > diff --git a/object.c b/object.c > index 07bdd5b26e..3b8b8c55c9 100644 > --- a/object.c > +++ b/object.c > @@ -1,13 +1,12 @@ > #include "cache.h" > #include "object.h" > #include "replace-object.h" > #include "object-store.h" > #include "blob.h" > #include "tree.h" > #include "commit.h" > #include "tag.h" > #include "alloc.h" > -#include "object-store.h" > #include "packfile.h" > #include "commit-graph.h" > > diff --git a/packfile.c b/packfile.c > index f3f962af4c..87512540f8 100644 > --- a/packfile.c > +++ b/packfile.c > @@ -1,20 +1,19 @@ > #include "cache.h" > #include "list.h" > #include "pack.h" > #include "repository.h" > #include "dir.h" > #include "mergesort.h" > #include "packfile.h" > #include "delta.h" > -#include "list.h" > #include "streaming.h" > #include "sha1-lookup.h" > #include "commit.h" > #include "object.h" > #include "tag.h" > #include "tree-walk.h" > #include "tree.h" > #include "object-store.h" > #include "midx.h" > #include "commit-graph.h" > #include "promisor-remote.h" > diff --git a/shallow.c b/shallow.c > index 5fa2b15d37..ae813658fb 100644 > --- a/shallow.c > +++ b/shallow.c > @@ -1,21 +1,18 @@ > #include "cache.h" > #include "repository.h" > #include "tempfile.h" > #include "lockfile.h" > #include "object-store.h" > #include "commit.h" > #include "tag.h" > #include "pkt-line.h" > #include "remote.h" > #include "refs.h" > #include "sha1-array.h" > #include "diff.h" > #include "revision.h" > #include "commit-slab.h" > -#include "revision.h" > #include "list-objects.h" > -#include "commit-slab.h" > -#include "repository.h" > #include "commit-reach.h" > > void set_alternate_shallow_file(struct repository *r, const char *path, int override) > diff --git a/unpack-trees.c b/unpack-trees.c > index f0f56d40ac..33ea7810d8 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -1,27 +1,26 @@ > #include "cache.h" > #include "argv-array.h" > #include "repository.h" > #include "config.h" > #include "dir.h" > #include "tree.h" > #include "tree-walk.h" > #include "cache-tree.h" > #include "unpack-trees.h" > #include "progress.h" > #include "refs.h" > #include "attr.h" > #include "split-index.h" > -#include "dir.h" > #include "submodule.h" > #include "submodule-config.h" > #include "fsmonitor.h" > #include "object-store.h" > #include "promisor-remote.h" > > /* > * Error messages expected by scripts out of plumbing commands such as > * read-tree. Non-scripted Porcelain is not required to use these messages > * and in fact are encouraged to reword them to better suit their particular > * situation better. See how "git checkout" and "git merge" replaces > * them using setup_unpack_trees_porcelain(), for example. > */ > -- > 2.23.0
Am 04.10.19 um 01:15 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> Found with "git grep '^#include ' '*.c' | sort | uniq -d". >> >> Signed-off-by: René Scharfe <l.s.r@web.de> >> --- >> Patch formatted with --function-context for easier review. > > I have a mixed feelings about that. > > The only audience benefitted by --function-context patch are those > who read the patch only in MUA without looking at anything outside > and declare they are done with reviewing the patch. For something > tricky, wider context does help to see what is going on, but then > I'd feel uncomfortable to hear "looks good to me" from those who did > not even apply the patch to their trees and looked at the changes > after "reviewing" tricky stuff that requires wider context to > properly review. Shallow reviews can happen with any form of patch. The intent of --function-context is to provide meaningful context along with the patch, as the basis for a discussion on the mailing list. It works best for changes whose effects are constrained to within the affected functions, but have crucial information located outside the three default lines of context. An example would be a change at the end of a function for which a reviewer might need to know the type of some variables declared at the top. The price for that is that patches get longer, which eats up more precious reviewer bandwidth. That shouldn't affect those who apply the patch before review, though -- they can ignore any extra lines and have git am deal with them. > If there were topics in flight that touch any of these include > blocks, the patch would not apply and a reviewer who is interested > in these fixes ends up needing to wiggle them in somehow. Instructing git am or apply to ignore extra context lines using -C3 or similar would help in such a case. > Having said all that, for _this_ particular case, I do not see a > reason why a review needs to look at anywhere outside the context > presented in this patch, so I'd say it is a narrow case that -W is > an appropriate thing to use. Right, sometimes the context in a patch is sufficient to understand the contained change completely. This one here requires one more piece of information, though, namely our convention of wrapping header files in guard defines to make repeated includes of them no-ops. We do that for those removed by the patch, but we have a few exceptions to that rule in our repo (at least command-list.h, kwset.h, sha1dc_git.h, tar.h, unicode-width.h). So in that sense it's not such a good example of a self-sufficient patch. :) > I just do not want to see contributors > less experienced than you (hence cannot make good judgement on when > to and not to use the option) get in the habit of using -W all the > time. Providing full context (all the code, all dependencies) is impractical. Three-line context is an arbitrary amount that happens to work well surprisingly often. No context (as in the original diff format) can suffice sometimes, e.g. for typo fixes. Function context is a different point on the spectrum that has its own use cases. Patches of long functions would become tedious with -W, not sure if I'd be ready for that. A MUA with syntax highlighting for diffs would help a bit, I guess. René
René Scharfe <l.s.r@web.de> writes: > It works best for changes whose effects are constrained to within the > affected functions, but have crucial information located outside the > three default lines of context. An example would be a change at the end > of a function for which a reviewer might need to know the type of some > variables declared at the top. Yup. >> If there were topics in flight that touch any of these include >> blocks, the patch would not apply and a reviewer who is interested >> in these fixes ends up needing to wiggle them in somehow. > > Instructing git am or apply to ignore extra context lines using -C3 > or similar would help in such a case. It may, but that is still extra work ;-) > This one here requires one more piece of information, though, namely our > convention of wrapping header files in guard defines to make repeated > includes of them no-ops. We do that for those removed by the patch, but > we have a few exceptions to that rule in our repo (at least > command-list.h, kwset.h, sha1dc_git.h, tar.h, unicode-width.h). So in > that sense it's not such a good example of a self-sufficient patch. :) Not really. "We use header guards" is an argument that demotes this cleanup from "must have" to "nice to have". If a project did not use header guards or including the same header twice were an error, the patch in question would have been more necessary, but that wouldn't have changed the correctness of the patch, I think.
Am 06.10.19 um 01:41 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: >> This one here requires one more piece of information, though, namely our >> convention of wrapping header files in guard defines to make repeated >> includes of them no-ops. We do that for those removed by the patch, but >> we have a few exceptions to that rule in our repo (at least >> command-list.h, kwset.h, sha1dc_git.h, tar.h, unicode-width.h). So in >> that sense it's not such a good example of a self-sufficient patch. :) > > Not really. "We use header guards" is an argument that demotes this > cleanup from "must have" to "nice to have". If a project did not > use header guards or including the same header twice were an error, > the patch in question would have been more necessary, but that > wouldn't have changed the correctness of the patch, I think. You start with "No", but make my point -- a reader would need more information than the content of the patch itself to classify it as a trivial cleanup, namely knowledge of our use of include guards. Here is an example of a non-idempotent header: #define NDEBUG ... #include <assert.h> ... #undef NDEBUG ... #include <assert.h> (That's the only one we use that I'm aware of.) René
diff --git a/builtin/am.c b/builtin/am.c index ee7305eaa6..b015e1d7d1 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1,40 +1,39 @@ /* * Builtin "git am" * * Based on git-am.sh by Junio C Hamano. */ #define USE_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" #include "config.h" #include "builtin.h" #include "exec-cmd.h" #include "parse-options.h" #include "dir.h" #include "run-command.h" #include "quote.h" #include "tempfile.h" #include "lockfile.h" #include "cache-tree.h" #include "refs.h" #include "commit.h" #include "diff.h" #include "diffcore.h" #include "unpack-trees.h" #include "branch.h" #include "sequencer.h" #include "revision.h" #include "merge-recursive.h" -#include "revision.h" #include "log-tree.h" #include "notes-utils.h" #include "rerere.h" #include "prompt.h" #include "mailinfo.h" #include "apply.h" #include "string-list.h" #include "packfile.h" #include "repository.h" /** * Returns the length of the first line of msg. */ diff --git a/builtin/blame.c b/builtin/blame.c index bfd537b344..9858d6b269 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -1,32 +1,31 @@ /* * Blame * * Copyright (c) 2006, 2014 by its authors * See COPYING for licensing conditions */ #include "cache.h" #include "config.h" #include "color.h" #include "builtin.h" #include "repository.h" #include "commit.h" #include "diff.h" #include "revision.h" #include "quote.h" #include "string-list.h" #include "mailmap.h" #include "parse-options.h" #include "prio-queue.h" #include "utf8.h" #include "userdiff.h" #include "line-range.h" #include "line-log.h" #include "dir.h" #include "progress.h" #include "object-store.h" #include "blame.h" -#include "string-list.h" #include "refs.h" static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>"); diff --git a/builtin/clone.c b/builtin/clone.c index 2048b6760a..9d73102c42 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1,44 +1,43 @@ /* * Builtin "git clone" * * Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>, * 2008 Daniel Barkalow <barkalow@iabervon.org> * Based on git-commit.sh by Junio C Hamano and Linus Torvalds * * Clone a repository into a different directory that does not yet exist. */ #define USE_THE_INDEX_COMPATIBILITY_MACROS #include "builtin.h" #include "config.h" #include "lockfile.h" #include "parse-options.h" #include "fetch-pack.h" #include "refs.h" #include "refspec.h" #include "object-store.h" #include "tree.h" #include "tree-walk.h" #include "unpack-trees.h" #include "transport.h" #include "strbuf.h" #include "dir.h" #include "dir-iterator.h" #include "iterator.h" #include "sigchain.h" #include "branch.h" #include "remote.h" #include "run-command.h" #include "connected.h" #include "packfile.h" #include "list-objects-filter-options.h" -#include "object-store.h" /* * Overall FIXMEs: * - respect DB_ENVIRONMENT for .git/objects. * * Implementation notes: * - dropping use-separate-remote and no-separate-remote compatibility * */ diff --git a/builtin/describe.c b/builtin/describe.c index e048f85484..90feab1120 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -1,22 +1,21 @@ #define USE_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" #include "config.h" #include "lockfile.h" #include "commit.h" #include "tag.h" #include "blob.h" #include "refs.h" #include "builtin.h" #include "exec-cmd.h" #include "parse-options.h" #include "revision.h" #include "diff.h" #include "hashmap.h" #include "argv-array.h" #include "run-command.h" #include "object-store.h" -#include "revision.h" #include "list-objects.h" #include "commit-slab.h" #define MAX_TAGS (FLAG_BITS - 1) diff --git a/builtin/rev-list.c b/builtin/rev-list.c index b8dc2e1fba..fb8187fba5 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -1,24 +1,23 @@ #include "cache.h" #include "config.h" #include "commit.h" #include "diff.h" #include "revision.h" #include "list-objects.h" #include "list-objects-filter.h" #include "list-objects-filter-options.h" #include "object.h" #include "object-store.h" #include "pack.h" #include "pack-bitmap.h" #include "builtin.h" #include "log-tree.h" #include "graph.h" #include "bisect.h" #include "progress.h" #include "reflog-walk.h" #include "oidset.h" #include "packfile.h" -#include "object-store.h" static const char rev_list_usage[] = "git rev-list [OPTION] <commit-id>... [ -- paths... ]\n" diff --git a/builtin/worktree.c b/builtin/worktree.c index 7f094f8170..0a53788151 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -1,16 +1,15 @@ #include "cache.h" #include "checkout.h" #include "config.h" #include "builtin.h" #include "dir.h" #include "parse-options.h" #include "argv-array.h" #include "branch.h" #include "refs.h" #include "run-command.h" #include "sigchain.h" #include "submodule.h" -#include "refs.h" #include "utf8.h" #include "worktree.h" diff --git a/object.c b/object.c index 07bdd5b26e..3b8b8c55c9 100644 --- a/object.c +++ b/object.c @@ -1,13 +1,12 @@ #include "cache.h" #include "object.h" #include "replace-object.h" #include "object-store.h" #include "blob.h" #include "tree.h" #include "commit.h" #include "tag.h" #include "alloc.h" -#include "object-store.h" #include "packfile.h" #include "commit-graph.h" diff --git a/packfile.c b/packfile.c index f3f962af4c..87512540f8 100644 --- a/packfile.c +++ b/packfile.c @@ -1,20 +1,19 @@ #include "cache.h" #include "list.h" #include "pack.h" #include "repository.h" #include "dir.h" #include "mergesort.h" #include "packfile.h" #include "delta.h" -#include "list.h" #include "streaming.h" #include "sha1-lookup.h" #include "commit.h" #include "object.h" #include "tag.h" #include "tree-walk.h" #include "tree.h" #include "object-store.h" #include "midx.h" #include "commit-graph.h" #include "promisor-remote.h" diff --git a/shallow.c b/shallow.c index 5fa2b15d37..ae813658fb 100644 --- a/shallow.c +++ b/shallow.c @@ -1,21 +1,18 @@ #include "cache.h" #include "repository.h" #include "tempfile.h" #include "lockfile.h" #include "object-store.h" #include "commit.h" #include "tag.h" #include "pkt-line.h" #include "remote.h" #include "refs.h" #include "sha1-array.h" #include "diff.h" #include "revision.h" #include "commit-slab.h" -#include "revision.h" #include "list-objects.h" -#include "commit-slab.h" -#include "repository.h" #include "commit-reach.h" void set_alternate_shallow_file(struct repository *r, const char *path, int override) diff --git a/unpack-trees.c b/unpack-trees.c index f0f56d40ac..33ea7810d8 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1,27 +1,26 @@ #include "cache.h" #include "argv-array.h" #include "repository.h" #include "config.h" #include "dir.h" #include "tree.h" #include "tree-walk.h" #include "cache-tree.h" #include "unpack-trees.h" #include "progress.h" #include "refs.h" #include "attr.h" #include "split-index.h" -#include "dir.h" #include "submodule.h" #include "submodule-config.h" #include "fsmonitor.h" #include "object-store.h" #include "promisor-remote.h" /* * Error messages expected by scripts out of plumbing commands such as * read-tree. Non-scripted Porcelain is not required to use these messages * and in fact are encouraged to reword them to better suit their particular * situation better. See how "git checkout" and "git merge" replaces * them using setup_unpack_trees_porcelain(), for example. */
Found with "git grep '^#include ' '*.c' | sort | uniq -d". Signed-off-by: René Scharfe <l.s.r@web.de> --- Patch formatted with --function-context for easier review. builtin/am.c | 1 - builtin/blame.c | 1 - builtin/clone.c | 1 - builtin/describe.c | 1 - builtin/rev-list.c | 1 - builtin/worktree.c | 1 - object.c | 1 - packfile.c | 1 - shallow.c | 3 --- unpack-trees.c | 1 - 10 files changed, 12 deletions(-) -- 2.23.0