On 12/06/2019 19:11, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
Thanks for the new version, this is looking pretty good now, just a
few comments below
I agree that this step is looking pretty good now.
I didn't check closely, but when 1/3 undergoes necessary polishing,
it may have repercussions on this step, though (I did see that the
change in 3/3 would have overlaps with what was touched by 1/3 that
needs to be done differently).
Thanks for guiding Rohit's series forward.
This is only slightly different from reset_for_rollback() if you
decide to keep a separate code path for skip vs abort then I'd be
tempted to combine the two like this.
diff --git a/sequencer.c b/sequencer.c
index ecf4be7e15..b187b4222e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2740,11 +2740,13 @@ static int reset_merge(void) {
static int reset_for_rollback(const struct object_id *oid)
{
const char *argv[4]; /* reset --merge <arg> + NULL */
+ size_t i = 0;
That size_t is, eh, "unusual". For an index into a small local
array of known size, just sticking to bog-standard-and-boring 'int'
would make it less distracting for future readers of the code.
Yes size_t is overkill here, it tends to be my default "array index"
type but there's no need for it here.
Or even better, perhaps use argv-array, so that you do not have to
worry about sizing the local array sufficiently large in the first
place.
That would be a much better way of doing it, I was so focussed on the
existing code I didn't think of that.
Best Wishes
Phillip
+ argv[i++] = "reset";
+ argv[i++] = "--merge";
+ if (oid)
+ argv[i++] = oid_to_hex(oid);
+ argv[i] = NULL;
return run_command_v_opt(argv, RUN_GIT_CMD);
}
