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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

I am way too tired to comment in detail, but here are some preliminary 
findings:

- your command line interface is very nice and elegant.

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

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

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

- neat idea to abuse the decorator for rewriting purposes.

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

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

- your memspn() and memcspn() functions are very inefficient. Better walk 
  the memory, introduce a GIT_HEXCHAR into ctype.c, ishexchar() into 
  git-compat-util.h, and use that.

- The example you give with "git update-index --remove" can fail, right? 
  Tell the user about that.

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

- "map" is missing. This is a function that you can use in all filters in 
  cg-admin-rewritehist, except (unfortunately) in the commit filter (for 
  technical reasons). Alas, these technical reasons also prevent such a 
  function to be usable by any of the filters ("maps", as you call them).

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

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

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.

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' \
		-e 'substitute/^author Joahnnes/author Johannes/header' \
		-e 'substitute/poreclain/porcelain/body' \
		-e 'rewrite-commit-names'

Hmm?

Ciao,
Dscho