From: Sven Verdoolaege <skimo@kotnet.org>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 4/4] Add git-rewrite-commits
Date: Mon, 09 Jul 2007 11:47:03 +0200 [thread overview]
Message-ID: <20070709094703.GP1528MdfPADPa@greensroom.kotnet.org> (raw)
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:
> - 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.
> - 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?
> - 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.
> 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.)
> 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?
> 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
next prev parent reply other threads:[~2007-07-09 9:47 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-07-08 16:23 [PATCH 0/4] Add git-rewrite-commits skimo
2007-07-08 16:23 ` [PATCH 1/4] export get_short_sha1 skimo
2007-07-08 16:23 ` [PATCH 2/4] export add_ref_decoration skimo
2007-07-08 16:23 ` [PATCH 3/4] revision: mark commits that didn't match a pattern for later use skimo
2007-07-08 16:23 ` [PATCH 4/4] Add git-rewrite-commits skimo
2007-07-08 16:37 ` Johannes Schindelin
2007-07-08 17:30 ` Sven Verdoolaege
2007-07-08 18:17 ` Johannes Schindelin
2007-07-08 19:11 ` Sven Verdoolaege
2007-07-08 18:04 ` Steven Grimm
2007-07-08 18:15 ` Sven Verdoolaege
2007-07-08 18:41 ` Johannes Schindelin
2007-07-08 21:10 ` Sven Verdoolaege
2007-07-09 9:01 ` Johannes Sixt
2007-07-09 9:48 ` Sven Verdoolaege
2007-07-09 11:57 ` Johannes Schindelin
2007-07-08 23:56 ` Johannes Schindelin
2007-07-09 9:47 ` Sven Verdoolaege [this message]
2007-07-09 12:57 ` Johannes Schindelin
2007-07-09 13:49 ` Sven Verdoolaege
2007-07-09 14:11 ` Johannes Schindelin
2007-07-09 14:42 ` Sven Verdoolaege
2007-07-09 14:59 ` Johannes Schindelin
2007-07-09 12:36 ` Jeff King
2007-07-09 13:16 ` Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20070709094703.GP1528MdfPADPa@greensroom.kotnet.org \
--to=skimo@kotnet.org \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=skimo@liacs.nl \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).