Re: [PATCH v4 00/10] Sequencer Foundations

Ramkumar Ramachandra [off-list ref] writes:

This is a quick re-roll after Junio's review of the previous
iteration; the previous iteration was posted less than 24 hours ago.
I'm in a hurry,...

Open source is not just about pushing new code through other people's
throats, but also about the skill to make other people around you to work
with you. They may be busy spending time on helping other topics moving
forward. Or they may simply be bone-headed ;-).

If you are in a hurry, try to spend _more_ time making sure it is easier
to review for other people. What were updated, what were kept as they were
and for what reason? The purpose of adding the cover letter to the series
is to do just that.

Without spending that effort and saying "I am in a hurry" give a wrong
impression to others: "I cannot tell if this round is better cooked than
the previous round without spending nontrivial time reading on it, but
chances are it hasn't been much improved during the 24 hours.  It may be
better use of my time to skip it and work on other topics first.  After
finishing everything else I can come back---by that time Ram may have sent
out another round and reviewing this round right now may become wasted
time".

Also the series seems to be based on a rather old codebase where we didn't
allow reverting the root commit. I tried to apply a first few to
understand where these "positive" error status could be coming from, and
was stopped by patch conflicts. In [2/10] I think you are propagating
errors from run_command() coming back through try_merge_command(), and the
change is good for that particular codepath, but I haven't followed all
the other codepaths from do_cherry_pick() to convince myself that the exit
status from the merge strategy backend are _the only_ positive returns you
can possibly get. Have you?

Hi Junio,

Junio C Hamano writes:

Without spending that effort and saying "I am in a hurry" give a wrong
impression to others: "I cannot tell if this round is better cooked than
the previous round without spending nontrivial time reading on it, but
chances are it hasn't been much improved during the 24 hours.  It may be
better use of my time to skip it and work on other topics first.  After
finishing everything else I can come back---by that time Ram may have sent
out another round and reviewing this round right now may become wasted
time".

You're right; this iteration was ill-thought-out. I'll try to put
myself in my reviewer's shoes before submitting code next time, so
that I don't waste effort re-rolling unnecessarily.

Also the series seems to be based on a rather old codebase where we didn't
allow reverting the root commit. I tried to apply a first few to
understand where these "positive" error status could be coming from, and
was stopped by patch conflicts. In [2/10] I think you are propagating
errors from run_command() coming back through try_merge_command(), and the
change is good for that particular codepath, but I haven't followed all
the other codepaths from do_cherry_pick() to convince myself that the exit
status from the merge strategy backend are _the only_ positive returns you
can possibly get. Have you?

Oops, I'll rebase.
I'll trace all the code paths carefully and include a summary of my
learnings in the next iteration.

-- Ram