Re: [PATCHv2 2/2] read-tree: migrate to parse-options

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Jun 25, 2009, 06:55 UTC
Message-ID: <4A431F5E.6070109@viscovery.net>
In-Reply-To: <1245906361-20644-1-git-send-email-bebarino@gmail.com>

Stephen Boyd schrieb:

Show 9 quoted lines

> @@ -8,7 +8,10 @@ git-read-tree - Reads tree information into the index
>  
>  SYNOPSIS
>  --------
> -'git read-tree' (<tree-ish> | [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u | -i]] [--exclude-per-directory=<gitignore>] [--index-output=<file>] <tree-ish1> [<tree-ish2> [<tree-ish3>]])
> +'git read-tree' [--index-output=<file>] <treeish>
> +'git read-tree' [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>]
> +		[-u [--exclude-per-directory=<gitignore>] | -i]]
> +		[--index-output=<file>] <tree-ish1> [<tree-ish2> [<tree-ish3>]]

Multi-line synopsis must begin with [verse].

If you write more than one mode of operation, the subsequent text should better reference them, but the current text does not do that. I think it is OK if you leave only the second, particularly because the first is only a subset of the second.

Show 7 quoted lines

> +	opts.update = update ? 1 : 0;
> +	opts.index_only = index_only ? 1 : 0;
> +	opts.trivial_merges_only = trivial_merges_only ? 1 : 0;
> +	opts.aggressive = aggressive ? 1 : 0;
> +	opts.verbose_update = verbose ? 1 : 0;
> +	opts.reset = reset ? 1 : 0;
> +	stage = opts.merge = (reset || merge || prefix_set) ? 1 : 0;

I don't think that the bitfields of struct unpack_trees_options are cast in stone. IMHO it is fine to make them regular struct members, so that you can take their address for read_tree_options and these foo ? 1 : 0 become unnecessary.

-- Hannes