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

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

Hi,

On Mon, 9 Jul 2007, Sven Verdoolaege wrote:

> On Mon, Jul 09, 2007 at 12:56:04AM +0100, Johannes Schindelin wrote:
>
> > - 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.

How much more explicit should I formulate my suggestion?

> I could also just not create them.  The old values are also available in 
> reflog.

There are two types of convenience.  One for the programmer, and one for 
the user.  As a user I would definitely like to see which branches were 
actually changed.  So even if you _have_ to keep your current behaviour, 
namely that the results are stored under the original names, you should 
store the original refs in something like refs/original (overridable by a 
command line option), but _only if_ they actually changed.

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

Quite a strong word.  Let's see if you can defend it:

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

Aha.  Functional programming.  Since when do we do functional programming 
here?  C++ is much closer to what we do.  So let's see:

http://en.wikipedia.org/wiki/Map_%28C%2B%2B_container%29

Does not apply at all here.  But does something else apply?  Ah, there is 
something about mathematics.  The basis of programming.  Maybe it is 
helpful:

http://en.wikipedia.org/wiki/Map_%28mathematics%29

Ah, no!  It only says that it is a function.  But there is a mention about 
partial maps!  And you said something about that:

> (In cg-admin-rewritehist some of them are (partial) filters,
> but the ones I have are not filters).

Alas, Wikipedia says about a partial function that it is a function which 
maps from one space into another space (not necessarily different spaces, 
though), but does not necessarily have a result for every input.

But in that sense, everything is a map.  Even rewrite-commits.  And 
commits. (Maybe you should rename that to map-maps?)

So maybe more luck with "filter"?  Since we still do not write in Haskell 
or something similar obscure, let's see if Wikipedia has something which 
is more likely applying to us.  Yes.  Software.  That surely 
must apply:

http://en.wikipedia.org/wiki/Filter_%28software%29

and even more to the point: UNIX filters:

http://en.wikipedia.org/wiki/Filter_%28Unix%29

You will see that both definitions apply _perfectly_ to what you do here.  
You pipe the commit into the "map" and write it out as a new commit!  
The _perfect_ example for a UNIX filter.

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

This is not what the commit-filter of cg-admin-rewritehist does.  If at 
all, your commit-map is a better example for a filter than rewritehist's 
commit-filter is.

So, you cannot defend that bold statement.

Enough ranted about an unmerited rant, let's get back to the interesting 
stuff:

All you demonstrated here is that the way "commit-map"s work right now is 
not sufficient to make more complicated rewrite rules.  You cannot split 
one commit in two, and you cannot skip it.

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

Are you?  Well, my suggestion still stands...  I mean, the suggestion I 
already gave in the post you quoted.  You know, to keep the name as it was 
in the program you based your work on?  Okay, I'll be explicit for once: 
Filter.

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

Yes.  But finger pointing will not help you here.  I reviewed _your_ code.

> Do you want me to rename that one as well?

Be my guest.  But let's do that independently, will ya?

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

Later.

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

Does that mean you acknowledge that the man page should tell about this?  
And probably also reveal the "|| :" remedy?

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

Umm.  Why not?

> What if the commit message is larger than the pipe buffer?

You start_command().  Then you write() until it is all written, or the 
pipe is broken.  Then you get the output via index_pipe().  Which is a 
single sha1.  I do not understand your question.

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

To be flexible, you have to have a way to ask "what was this commit 
rewritten to?" from within a filter.

Example: maybe you expect the rewritten commit to have a tree identical to 
commits in a certain branch, which was _also_ possibly rewritten.  Maybe 
you want to mark these pairs, for example by a tag.  Then you need to get 
the sha1 of the rewritten commits.

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

Yes, but your original version was very intrusive and hard to review.  In 
contrast, the simple shell script we have now is hackable by many, and 
easily reviewed.  Indeed, it takes but a couple of minutes to verify that 
it does what it should do and has no side effects.

But I agree, it is easy enough to run the commands from rewrite-commits.c.  
The harder part is to provide an infrastructure which makes it useful.

To enhance on the above example: you're rewriting the commit messages so 
that commit names are rewritten to match the rewritten commits.  That is 
possible by a message filter in cg-admin-rewritehist.

But now somebody comes along, and says "I have a history I need to 
rewrite.  All bug fixes.  The commit names were all abbreviated in the 
commit messages, but they always had 'commit ' in front of them.  I want 
to rewrite them, too."

No chance without a "map" function.  Except to bug you to change the C 
code for this very special need.

By contrast, filter-branch does not need to be changed for that at all.

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

Are you trying to misunderstand me?

You can tell the user how to skip single commits.  But you have to warn 
them that they might be wanting something different, because the word 
"commit" not only implies a "revision", which you would skip in this 
context, but also a "commit diff", which you would _not_.

The "much more complex" rules clearly are for other use cases, which 
brings me to your next question:

> Do you have a use case?

_You_ already mentioned it.  Subdirectory filters.

And if you think a little further, you can easily see that for this to 
become useful, you _have_ to have a "map" function, too.  Because you are 
likely wanting to use rewrite-commits, or filter-branch, to transform big 
repositories into super/subprojects.

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

It is a very special use case.  Not always will you be able to get all the 
information from the commit object you need for conditional operations.  
My example only showed that you can do the same with that syntax.  But 
imagine what you could do if we just added a small syntactical sugar:

	-e '?has-path:README?substitute/v2/v3/'

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

IMHO you will not get happy by introducing ever more, but still restricted 
functionality.

At the moment, cg-admin-rewritehist can do much more than what 
rewrite-commits can do, because the eval'ed filters give you a much 
greater freedom in what you can do (since "map" is available, and you can 
even modify the commit name mapping by the filters on the go: they can 
edit .git-rewrite/!), and how you can do it (no need to work around 
certain limitations by using temporary files).

A lot of the speed of rewrite-commits stems from the fact that it is 
limited in that way.

Don't get me wrong.  I _want_ rewrite-commits to replace filter-branch.  
But there are fundamental problems with filter-branch, and there are 
fundamental problems with having the same functionality from within C.  
And it appears to me that you do not even try to address the latter 
fundamental problems.

Ciao,
Dscho