git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Erik Faye-Lund <kusmabite@gmail.com>
Date
Dec 14, 2010, 22:35 UTC
Message-ID
<AANLkTi=cHb2kV2MaYu72nXVOksO7O9HhJLEo-fU0sV5N@mail.gmail.com>
In-Reply-To
<20101214222122.GD4084@sandbox>
On Tue, Dec 14, 2010 at 11:21 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
Show 55 quoted lines
> 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?
Show 32 quoted lines
> +{
> +       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?
Show 18 quoted lines
> +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.

Previous: Heiko VoigtNext: Junio C Hamano
Message 8 of 34 in “make open/unlink failures user friendly on windows using retry/abort”
  1. 0/5 make open/unlink failures user friendly on windows using retry/abortHeiko Voigt, Dec 14, 2010
  2. 1/8 mingw: move unlink wrapper to mingw.cHeiko Voigt, Dec 14, 2010
  3. 2/8 mingw: work around irregular failures of unlink on windowsHeiko Voigt, Dec 14, 2010
  4. Erik Faye-LundDec 14, 2010
  5. Heiko VoigtDec 14, 2010
  6. 2/5 mingw: work around irregular failures of unlink on windowsHeiko Voigt, Dec 14, 2010
  7. 3/8 mingw: make failures to unlink or move raise a questionHeiko Voigt, Dec 14, 2010
  8. Erik Faye-LundDec 14, 2010
  9. Junio C HamanoDec 14, 2010
  10. Heiko VoigtDec 15, 2010
  11. Johannes SchindelinDec 15, 2010
  12. Junio C HamanoDec 15, 2010
  13. Heiko VoigtDec 15, 2010
  14. Erik Faye-LundDec 15, 2010
  15. Heiko VoigtDec 15, 2010
  16. 4/5 mingw: add fallback for rmdir in case directory is in useHeiko Voigt, Dec 14, 2010
  17. 5/5 mingw_rmdir: set errno=ENOTEMPTY when appropriateHeiko Voigt, Dec 14, 2010
  18. Erik Faye-LundDec 14, 2010
  19. Johannes SchindelinDec 15, 2010
  20. Erik Faye-LundDec 15, 2010
  21. Junio C HamanoDec 15, 2010
  22. 0/5 make open/unlink failures user friendly on windows using retry/abortHeiko Voigt, Feb 7, 2011
  23. 1/5 mingw: move unlink wrapper to mingw.cHeiko Voigt, Feb 7, 2011
  24. Johannes SchindelinFeb 17, 2011
  25. 2/5 mingw: work around irregular failures of unlink on windowsHeiko Voigt, Feb 7, 2011
  26. 3/5 mingw: make failures to unlink or move raise a questionHeiko Voigt, Feb 7, 2011
  27. 4/5 mingw: add fallback for rmdir in case directory is in useHeiko Voigt, Feb 7, 2011
  28. 5/5 mingw_rmdir: set errno=ENOTEMPTY when appropriateHeiko Voigt, Feb 7, 2011
  29. Erik Faye-LundFeb 7, 2011
  30. Heiko VoigtFeb 7, 2011
  31. Erik Faye-LundFeb 7, 2011
  32. Junio C HamanoFeb 7, 2011
  33. Erik Faye-LundFeb 7, 2011
  34. Junio C HamanoFeb 8, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.