Re: [PATCH 6/6] Add git-rewrite-commits

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +Examples
>> +--------
>> +
>> +Suppose you want to remove a file (containing confidential information
>> +or copyright violation) from all commits:
>> +
>> +----------------------------------------------------------------------------
>> +git rewrite-commits --index-filter 'git update-index --remove filename || :'
>
> We seem to prefer "$ git" instead of just "git" in the other man pages' 
> examples.

"git update-index --remove Foo" does not remove the index entry
Foo if the file Foo still exists in the working tree (use "git
update-index --force-remove" for that).

But this leads to more fundamental issues.  It is not obvious
from the description what environment rewrite-commits runs in.
Does it run at the toplevel of the current working tree, or is
it run in a separate temporary directory like filter-branch
does?  What "index" and "HEAD" do operations done by filters
affect (I think it is safe to assume that readers familiar
enough with other parts of git would be able to guess that
filters should operate on the "HEAD" and index given by
rewrite-commits to its execution environment without mucking
with GIT_DIR nor GIT_INDEX_FILE)?  Are filters allowed to modify
files in the working tree, and if so what is the consequence of
doing so?

>> +----------------------------------------------------------------------------
>> +
>> +Now, you will get the rewritten history saved in your current branch
>> +(the old branch is saved in refs/original).
>
> 						The "|| :" construct 
> + prevents the filter to fail when the given file was not present in the 
> + index.

prevents the filter from failing?  But is that really what we
want?  Why are we ignoring the error, and if there is a valid
reason to ignore shouldn't we explain why?

>> +To move the whole tree into a subdirectory, or remove it from there:
>> +
>> +---------------------------------------------------------------
>> +git rewrite-commits --index-filter \
>> +	'git ls-files -s | sed "s-\t-&newsubdir/-" |
>> +		GIT_INDEX_FILE=$GIT_INDEX_FILE.new \
>> +			git update-index --index-info &&
>> +	 mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE'
>> +---------------------------------------------------------------

I see only one operation in the example, and "or remove it from
there" confuses the reader.

I'll refrain from comments on the code right now, until I read
the series over.