Re: [PATCH v3 6/6] unpack-trees.[ch]: define and use a UNPACK_TREES_OPTIONS_INIT

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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 ...

> 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:

> 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.

> @@ -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...