Re: Add retry logic to rebaseall

[Jason Tishler <jason@tishler.net>]

David,

On Fri, Jan 17, 2014 at 11:30:54AM -0500, David Boyce wrote:
> On Fri, Jan 17, 2014 at 11:13 AM, Jason Tishler <jason@tishler.net> wrote:
> > AFAICT, there is a race condition issue with the proposed
> > functionality.  David's build servers could be quiescent when the
> > check for running processes is performed, but they could restart
> > before the rebase is finished.  I realize the window is very small,
> > but it is nevertheless nonzero, so the rebase could still fail.
> 
> I don't think race condition is the right phrase here. This is the
> exact same situation faced by the existing rebaseall functionality; it
> knows there are no Cygwin processes running at start time but any
> process could start between then and end time.

Yes, but rebaseall it typically called manually, so the user wouldn't do
this or would have to deal with the consequences if they did.

> I agree there's a window where things could go wrong, but this feature
> doesn't worsen it. Closing that window is an orthogonal effort, IMHO;
> the new flag isn't called --make-sure-the-rebase-works-dammit.

IMO, if a feature adds automation (i.e., no human intervention required),
then it should try to work under all conditions, if feasible without too
much effort.  Your wait feature could also detect rebase failures and
retry until it succeeds (maybe giving up after N attempts).

> > There are also formatting issues with the patch.  For example, the
> > addition of the while loop requires lines to be shifted to the right.
> > I know rebaseall unfortunately has a mixture of tabs and spaces, but
> > after applying the patch some lines are not indented correctly.
> 
> I had a paragraph about that in the original email which got rejected
> due to "spam score too high" so I cut the text down for the second try
> and ended up losing that part. Yes, the original has a mix of tabs and
> spaces and my editor might be configured differently from yours, so I
> made the patch using "diff -u -w". That may have been a mistake; I'm
> happy to clean it up if asked.

I agree that the formatting issue is minor, but a patch should be
generated to minimize a maintainer's efforts.

> > IMO, the proposed functionality is very specialized and doesn't seem
> > to be generally applicable.  This functionality could also be
> > implemented (by the few who need it) as a very simple wrapper script
> > that calls rebaseall until is succeeds.  This approach would also
> > workaround the race condition.
> 
> How so? The behavior and risk factors would be identical as far as I
> can see.

If you wrap rebaseall in its entirety and retry on failure until
successful, then that will eliminate the "race condition" issue.

If consensus thinks this change is generally useful, then rebaseall
should be changed accordingly.  Otherwise, you can write a few line
wrapper script to meet your special needs.

Jason

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple