Re: [PATCH v3 3/8] mingw: make failures to unlink or move raise a question

[Erik Faye-Lund <kusmabite@gmail.com>]

On Tue, Dec 14, 2010 at 11:21 PM, Heiko Voigt <hvoigt@hvoigt.net> 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 <hvoigt@hvoigt.net>
> Signed-off-by: Albert Dvornik <dvornik+git@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 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 <conio.h>
>  #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.