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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 14, 2007, 19:26 UTC
Message-ID: <7v7ip2hjna.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0707141140510.14781@racer.site>

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

Show 11 quoted lines

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

Show 8 quoted lines

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

Show 9 quoted lines

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