On Friday 31 July 2009, Junio C Hamano wrote:
> Christian Couder <chriscool@xxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html