Re: [PATCH v3 6/6] unpack-trees.[ch]: define and use a UNPACK_TREES_OPTIONS_INIT
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 1, 2021, 21:39 UTC
- Message-ID
- <xmqqk0iw4e7v.fsf@gitster.g>
- In-Reply-To
- <patch-v3-6.6-18358f5d57a-20211001T102056Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 21 quoted lines
> diff --git a/archive.c b/archive.c
> index a3bbb091256..210d7235c5a 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -269,7 +269,7 @@ int write_archive_entries(struct archiver_args *args,
> write_archive_entry_fn_t write_entry)
> {
> struct archiver_context context;
> - struct unpack_trees_options opts;
> + struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
> struct tree_desc t;
> int err;
> struct strbuf path_in_archive = STRBUF_INIT;
> @@ -300,7 +300,6 @@ int write_archive_entries(struct archiver_args *args,
> * Setup index and instruct attr to read index only
> */
> if (!args->worktree_attributes) {
> - memset(&opts, 0, sizeof(opts));
> opts.index_only = 1;
> opts.head_idx = -1;
> opts.src_index = args->repo->index;These two hunks almost makes me say "Meh" (I am adding this parenthetical comment after reading the patch thru and I am not yet convinced otherwise).
The reason why we had memset(0), getting rid of which I have no problem about, is because we didn't prepare the members of the structure using initializer in the first place, no? The "opts" is used only in this block, so instead of these lines, we could have
if (!args->worktree_attributes) {
struct unpack_trees_options opts = {
.index_only = 1,
.head_idx = -1,
...
.fn = oneway_merge;
};
init_tree_desc(...);
if (unpack_trees(1, &t, &opts))
...which would be much easier to understand. We can get rid of memset(0), having UNPACK_TREES_OPTIONS_INIT did not help us much.
Some hunks show that we have code between the location where "opts" is declared and where we know for certain that "opts" needs to be actually used and with what values in its members, like ...
Show 21 quoted lines
> diff --git a/builtin/am.c b/builtin/am.c
> index e4a0ff9cd7c..82641ce68ec 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1901,7 +1901,7 @@ static void am_resolve(struct am_state *state)
> static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
> {
> struct lock_file lock_file = LOCK_INIT;
> - struct unpack_trees_options opts;
> + struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
> struct tree_desc t[2];
>
> if (parse_tree(head) || parse_tree(remote))
> ...
>
> refresh_cache(REFRESH_QUIET);
>
> - memset(&opts, 0, sizeof(opts));
> opts.head_idx = 1;
> opts.src_index = &the_index;
> opts.dst_index = &the_index;... this one. And it is not necessarily a good idea to initialize "opts" with the actual values we'd use, and _INIT does help us getting rid of memset(0);
But many in this patch are much simpler like this one:
Show 16 quoted lines
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 8c69dcdf72a..fea4533dbec 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -639,10 +639,9 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
> int worktree, int *writeout_error,
> struct branch_info *info)
> {
> - struct unpack_trees_options opts;
> + struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
> struct tree_desc tree_desc;
>
> - memset(&opts, 0, sizeof(opts));
> opts.head_idx = -1;
> opts.update = worktree;
> opts.skip_unmerged = !worktree;which can be initialized with designated initializers in place, and having an _INIT macro would not help us very much.
Show 11 quoted lines
> @@ -719,9 +718,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
> } else {
> struct tree_desc trees[2];
> struct tree *tree;
> - struct unpack_trees_options topts;
> + struct unpack_trees_options topts = UNPACK_TREES_OPTIONS_INIT;
>
> - memset(&topts, 0, sizeof(topts));
> topts.head_idx = -1;
> topts.src_index = &the_index;
> topts.dst_index = &the_index;Likewise here.
So...