Re: [PATCH] revert: libify pick

[Christian Couder <chriscool@tuxfamily.org>]

On Friday 31 July 2009, Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> >
> > 	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?

It has it in the v2 series I will post just after sending this email.

As usual feel free to squash all or some of the patches together if you 
prefer.

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

This change is in patch 2.

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

I changed it to "pick_commit()" in patch 3.

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

I removed code that made REVERSE imply ADD_NOTE in patch 4

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

I removed the misleading part and added some of your comments in patch 5.

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

Thanks,
Christian.