From: Johannes Sixt <j.sixt@viscovery.net>
Date: Thu, 25 Jun 2009 06:55:26 GMT
Subject: Re: [PATCHv2 2/2] read-tree: migrate to parse-options
Message-ID: <4A431F5E.6070109@viscovery.net>
In-Reply-To: <1245906361-20644-1-git-send-email-bebarino@gmail.com>

Stephen Boyd schrieb:
> @@ -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.

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