Re: [PATCH] Improve description of git filter-branch.

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 20, 2008, 19:31 UTC
Message-ID: <7v8x0dw5sx.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080315103900.GC14769@ins.uni-bonn.de>

Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:

Show 12 quoted lines

> diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt
> index 543a1cf..3f7df04 100644
> --- a/Documentation/git-filter-branch.txt
> +++ b/Documentation/git-filter-branch.txt
> @@ -20,12 +20,12 @@ DESCRIPTION
>  Lets you rewrite git revision history by rewriting the branches mentioned
>  in the <rev-list options>, applying custom filters on each revision.
>  Those filters can modify each tree (e.g. removing a file or running
> -a perl rewrite on all files) or information about each commit.
> +a perl script on all files) or information about each commit.
>  Otherwise, all information (including original commit times or merge
>  information) will be preserved.

We could go either way, methinks. IOW, your update is not worse, but not a huge improvement either.

>  The command will only rewrite the _positive_ refs mentioned in the
> -command line (i.e. if you pass 'a..b', only 'b' will be rewritten).
> +command line (i.e. if you pass 'a..b', 'a' will not be rewritten).

This one talks about which refs (i.e. pointers to commit DAG) are rewritten, not about which commits are rewritten. Because there are only two things, 'a' and 'b', are involved (remember, the statement does not talk anything about the commits in between), "only 'b' will" and "'a' will not" are the same thing.

Saying "only 'b'" would actually make it more consistent with the body of the description: "We only rewrite _positive_ ones" (i.e. it does not say "We do not rewrite negative ones").

By the way, that "i.e." should probably be "e.g.".

Show 8 quoted lines

> @@ -42,7 +42,7 @@ Always verify that the rewritten version is correct: The original refs,
>  if different from the rewritten ones, will be stored in the namespace
>  'refs/original/'.
>  
> -Note that since this operation is extensively I/O expensive, it might
> +Note that since this operation is extremely I/O expensive, it might
>  be a good idea to redirect the temporary directory off-disk with the
>  '-d' option, e.g. on tmpfs.  Reportedly the speedup is very noticeable.

Can we say "very"? extremely/extensively expensive is using ex'es excessively to express what can be said in much easier way, I think.

Show 8 quoted lines

> @@ -51,14 +51,15 @@ Filters
>  ~~~~~~~
>  
>  The filters are applied in the order as listed below.  The <command>
> -argument is always evaluated in shell using the 'eval' command (with the
> -notable exception of the commit filter, for technical reasons).
> +argument is always evaluated in the shell context using the 'eval' command
> +(with the notable exception of the commit filter, for technical reasons).

Ok.

Show 10 quoted lines

>  Prior to that, the $GIT_COMMIT environment variable will be set to contain
>  the id of the commit being rewritten.  Also, GIT_AUTHOR_NAME,
>  GIT_AUTHOR_EMAIL, GIT_AUTHOR_DATE, GIT_COMMITTER_NAME, GIT_COMMITTER_EMAIL,
> -and GIT_COMMITTER_DATE are set according to the current commit. If any
> -evaluation of <command> returns a non-zero exit status, the whole operation
> -will be aborted.
> +and GIT_COMMITTER_DATE are set according to the current commit.  The values
> +of these variables after the filters have run, are used for the new commit.
> +If any evaluation of <command> returns a non-zero exit status, the whole
> +operation will be aborted.

Probably a good clarification, but I am currently too busy to check if the additional sentence is correct.

Show 10 quoted lines

> @@ -71,9 +72,9 @@ OPTIONS
>  -------
>  
>  --env-filter <command>::
> -	This is the filter for modifying the environment in which
> -	the commit will be performed.  Specifically, you might want
> -	to rewrite the author/committer name/email/time environment
> +	This filter may be used if you only need to modify the environment
> +	in which the commit will be performed.  Specifically, you might
> +	want to rewrite the author/committer name/email/time environment

Ok. Without needless line rewrapping this would have been much less time consuming to review, though.

Show 6 quoted lines

> @@ -149,7 +150,7 @@ definition impossible to preserve signatures at any rate.)
>  -d <directory>::
>  	Use this option to set the path to the temporary directory used for
>  	rewriting.  When applying a tree filter, the command needs to
> -	temporary checkout the tree to some directory, which may consume
> +	temporarily checkout the tree to some directory, which may consume

Ok, but isn't the verb to "check out" two words?