Re: PATCH] remove duplicate #include directives

[Junio C Hamano <gitster@pobox.com>]

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