From: Erik Faye-Lund Date: Tue, 14 Dec 2010 22:35:26 GMT Subject: Re: [PATCH v3 3/8] mingw: make failures to unlink or move raise a question Message-ID: In-Reply-To: <20101214222122.GD4084@sandbox> On Tue, Dec 14, 2010 at 11:21 PM, Heiko Voigt wrote: > On Windows in case a program is accessing a file unlink or > move operations may fail. To give the user a chance to correct > this we simply wait until the user asks us to retry or fail. > > This is useful because of the following use case which seem > to happen rarely but when it does it is a mess: > > After making some changes the user realizes that he was on the > incorrect branch. When trying to change the branch some file > is still in use by some other process and git stops in the > middle of changing branches. Now the user has lots of files > with changes mixed with his own. This is especially confusing > on repositories that contain lots of files. > > Although the recent implementation of automatic retry makes > this scenario much more unlikely lets provide a fallback as > a last resort. > > Thanks to Albert Dvornik for disabling the question if users can't see it. > > If the stdout of the command is connected to a terminal but the stderr > has been redirected, the odds are good that the user can't see any > question we print out to stderr.  This will result in a "mysterious > hang" while the app is waiting for user input. > > It seems better to be conservative, and avoid asking for input > whenever the stderr is not a terminal, just like we do for stdin. > > Signed-off-by: Heiko Voigt > Signed-off-by: Albert Dvornik > Signed-off-by: Johannes Schindelin > --- > I have added the sign-off from the squashed commit of Albert and > Johannes. I hope its ok this way. > >  compat/mingw.c |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >  1 files changed, 82 insertions(+), 0 deletions(-) > > diff --git a/compat/mingw.c b/compat/mingw.c > index 52183a7..ac9fb4a 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -2,6 +2,7 @@ >  #include "win32.h" >  #include >  #include "../strbuf.h" > +#include "../run-command.h" > >  static const int delay[] = { 0, 1, 10, 20, 40 }; > > @@ -129,6 +130,78 @@ static inline int is_file_in_use_error(DWORD errcode) >        return 0; >  } > > +static int read_yes_no_answer() Perhaps "static int read_yes_no_answer(void)" for portability? > +{ > +       char answer[1024]; > + > +       if (fgets(answer, sizeof(answer), stdin)) { > +               size_t answer_len = strlen(answer); > +               int got_full_line = 0, c; > + > +               /* remove the newline */ > +               if (answer_len >= 2 && answer[answer_len-2] == '\r') { > +                       answer[answer_len-2] = '\0'; > +                       got_full_line = 1; > +               } > +               else if (answer_len >= 1 && answer[answer_len-1] == '\n') { > +                       answer[answer_len-1] = '\0'; > +                       got_full_line = 1; > +               } > +               /* flush the buffer in case we did not get the full line */ > +               if (!got_full_line) > +                       while((c = getchar()) != EOF && c != '\n'); > +       } else > +               /* we could not read, return the > +                * default answer which is no */ > +               return 0; > + > +       if (answer[0] == 'y' && strlen(answer) == 1) > +               return 1; > +       if (!strncasecmp(answer, "yes", sizeof(answer))) > +               return 1; > +       if (answer[0] == 'n' && strlen(answer) == 1) > +               return 0; > +       if (!strncasecmp(answer, "no", sizeof(answer))) > +               return 0; Since you're doing case insensitive checks for "yes" and "no", perhaps it'd make sense to allow upper case 'Y' and 'N' also? Something like: -       if (answer[0] == 'n' && strlen(answer) == 1) +       if (tolower(answer[0]) == 'n' && strlen(answer) == 1) hm? > +static int ask_user_yes_no(const char *format, ...) > +{ > +       char question[4096]; > +       const char *retry_hook[] = { NULL, NULL, NULL }; > +       va_list args; > + > +       if ((retry_hook[0] = getenv("GIT_ASK_YESNO"))) { > + > +               va_start(args, format); > +               vsnprintf(question, sizeof(question), format, args); > +               va_end(args); > + > +               retry_hook[1] = question; > +               return !run_command_v_opt(retry_hook, 0); > +       } > + > +       if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr))) > +               return 0; I'm wondering, doesn't this make the semantics a bit wrong? The function is called "ask_user_yes_no", but it might end up not asking after all. Perhaps it should be called something that reflects this? "maybe_ask_yes_no", "ask_yes_no_if_tty", "should_retry"? I don't have a non-ugly suggestion, but I suspect something like that might leave other people less puzzled when reading the code.