Re: [PATCH] revert: libify pick

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 31, 2009, 07:15 UTC
Message-ID: <7v8wi52uig.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20090731032548.4169.16389.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

Show 27 quoted lines

> From: Stephan Beyer <s-beyer@gmx.net>
>
> This commit is made of code from the sequencer GSoC project:
>
> git://repo.or.cz/git/sbeyer.git
>
> (commit e7b8dab0c2a73ade92017a52bb1405ea1534ef20)
>
> The goal of this commit is to abstract out pick functionnality
> into a new pick() function made of code from "builtin-revert.c".
>
> The new pick() function is in a new "pick.c" file with an
> associated "pick.h".
>
> "pick.h" and "pick.c" are strictly the same as on the sequencer repo,
> but a few changes were needed on "builtin-revert.c" to keep it up to
> date with changes on git.git.
>
> Mentored-by: Daniel Barkalow <barkalow@iabervon.org>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>
> ---
>
> 	This patch is part of trying to port git-rebase--interactive.sh
> 	to C using code from the sequencer GSoC project. But maybe it can
> 	be seen as a clean up too.

Thanks.  Why doesn't this have Stephan's sign-off?

The new "pick.c" file seems to be a nicer implementation of the main logic of builtin-revert.c and its primary niceness comes from the use of strbuf.

A few minor points and comments.

 * error() returns -1.

        error("message");     =>        return error("message");
        return -1;

 * pick() might be a bit too short and abstract name for a generic library
   function.

 * REVERSE is made to imply ADD_NOTE but the codepath that acts on
   ADD_NOTE never seems to be reached if REVERSE is set.

The intent of pick() funtion looks like it starts from the current index (not HEAD), and allow the effect of one commit replayed (either forward or backward) to that state, leaving the result in the index.

You do not have to start from a commit, so you can replay many commits to the index in sequence without commiting in between to squash multiple steps if you wanted to. I think that makes sense as a nice general interface.

The "if (no_commit)" codepath in the original code did things very differently from the usual "start from HEAD and replay the effect" codepath and it warranted the big "We do not intend to commit immediately" comment. For pick() function, however, the "start from index" is the normal and only mode of operation. Keeping the big comment is misleading.

When it replays another commit on HEAD, the new code does not read "HEAD" by hand into head anymore, but it still has the check between the index and "HEAD" and refuses to run if the index is dirty, which means the tree you get from write_cache_as_tree() is guaranteed to be the same as "HEAD", so this conversion looks correct.