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

From: Sven Verdoolaege <skimo@kotnet.org>
Date: Jul 9, 2007, 09:47 UTC
Message-ID: <20070709094703.GP1528MdfPADPa@greensroom.kotnet.org>
In-Reply-To: <Pine.LNX.4.64.0707090011070.4248@racer.site>

Thanks for the review.

On Mon, Jul 09, 2007 at 12:56:04AM +0100, Johannes Schindelin wrote:

Show 9 quoted lines

> - you use a lot of what was in cg-admin-rewritehist (including 
>   adjustments I made in the documentation for filter-branch), but you also 
>   make it more confusing for people used to that tool:
> 
> 	- instead of leaving the original branches as they are, you 
> 	  overwrite them. That's okay. But then you put the originals into 
> 	  refs/rewritten. Without cg-admin-rewritehist using that name 
> 	  for the _result_, you could explain your way out of confusion. 
> 	  As it is, you cannot.

I thought you had to specify the name of the new branch on the command line. Anyway, I don't really care about the name of this hierarchy. I just picked a name that is somewhat related to "rewrite-commits". Suggestions are welcome. I could also just not create them. The old values are also available in reflog.

Show 5 quoted lines

> 	- in spite of doing the same as cg-admin-rewritehist with filters, 
> 	  you call them maps. But they are no maps. They are manipulators, 
> 	  you can call them mutators or filters, too. Given what people 
> 	  know of cg-admin-rewritehist, you really should keep the name 
> 	  "filter".

Nonesense. They are not filters (http://en.wikipedia.org/wiki/Filter_%28higher-order_function%29). They are maps (http://en.wikipedia.org/wiki/Map_%28higher-order_function%29). (In cg-admin-rewritehist some of them are (partial) filters, but the ones I have are not filters). I could extend the commit-map to remove the commit it the output is empty, but it'd still be closer to a map than to a filter. (You can map a commit to nothing, but a filter can't alter the elements of a list, it only determines which elements are kept.)

> 	- the name "map" itself is used in cg-admin-rewritehist, to map 
> 	  commit names from old to new. By using that name differently, 
> 	  again you contribute to confusion, for no good reason.

There is a good reason to call what I call maps maps. They _are_ maps. Still, I'm open for suggestions.

> - get_one_line() is a misnomer. It wants to be named get_linelen().

Hmmm... I guess you missed Linus' mistake when he introduced it in commit.c (e3bc7a3bc7b77f44d686003f5a9346a135529f73). Do you want me to rename that one as well?

> - instead of spawning read-tree, you could use unpack_trees() to boost 
>   performance even more. But I guess it is probably left for later, to 
>   make it easier to review the patch.

Yeah, it looked a bit tricky for an initial implementation, especially where I move the HEAD forward.

> - The example you give with "git update-index --remove" can fail, right?

Yes.  Spectacularly, even.

> - The commit filter again deviates from the usage in cg-admin-rewritehist. 
>   I can see that you wanted to make it more versatile. However, it makes 
>   the tool potentially also a bit more cumbersome to use. Besides, you use 
>   a temporary file where there is no need to.

Are you saying I should use two pipes? What if the commit message is larger than the pipe buffer?

Show 7 quoted lines

> - the more fundamental problem with the missing "map", I do not see a 
>   reasonable way to get the same functionality from any of the code 
>   snippets passed to rewrite-commits. Indeed, even the workaround of 
>   cg-admin-rewritehist, to read $TEMP/map/$sha1, does not work, since you 
>   are keeping it all in memory. On IRC, gitster suggested to use a 
>   bidirectional pipe (such as stdin/stdout) to get at the new commit 
>   names, but because of buffering, I guess this is no joy.

I could add an option to write $TEMP/map/$sha1, but it's not clear to me when such a map would be useful. Please enlighten me.

Show 6 quoted lines

> As commented on IRC, the env/tree/parent/msg filters of 
> cg-admin-rewritehist could be all emulated by commit filters. However, 
> that would be really inconvenient. So at a later stage, these would have 
> to be integrated into rewrite-commits (even if it would be possible to 
> drive rewrite-commits by a shell porcelain, but I guess you are opposed to 
> that idea, since you want to do everything else in C, too).

I'm not opposed to running a few commands and connecting stuff in shell. (See git-submodule add, although I admit that I would have preferred to do all of it in C.)

Show 5 quoted lines

> However, the biggest and very real problem is that your filters do not 
> have a "map" function to get the rewritten sha1 for a given sha1. That is 
> what makes the filters so versatile, though, since you can skip revisions 
> by much more complex rules than just greps on the commit message or 
> header.

I thought your were opposed to the idea of skipping commits, since you still carry along the changes in those commits. Do you have a use case?

Show 5 quoted lines

> But hey, maybe it _is_ time to rethink the whole filter business, and 
> introduce some kind of regular expression based action language. Something 
> like
> 
> 	git rewrite-commits -e '/^author Darl McBribe/skip-commit' \

What's wrong with --author='!Darl McBribe' ?

> 		-e 'substitute/^author Joahnnes/author Johannes/header' \
> 		-e 'substitute/poreclain/porcelain/body' \
> 		-e 'rewrite-commit-names'

Hmmm... some of these would basically need a builtin sed. I was thinking about adding --remove and --rename, though.

skimo