{"thread":{"id":"26220","subject":"[PATCH/RFC] alias: use run_command api to execute aliases","startedAt":"2011-01-06T19:13:07Z","lastAt":"2011-01-07T19:21:45Z","messageCount":7,"participants":["Erik Faye-Lund","Jonathan Nieder","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"159052","messageId":"1294341187-3956-1-git-send-email-kusmabite@gmail.com","threadId":"26220","inReplyTo":null,"subject":"[PATCH/RFC] alias: use run_command api to execute aliases","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-01-06T19:13:07Z","receivedAt":"2011-01-06T19:13:07Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Windows, system() executes with cmd.exe instead of /bin/sh. This\nmeans that aliases currently has to be batch-scripts instead of\nbourne-scripts. On top of that, cmd.exe does not handle single quotes,\nwhich is what the code-path currently uses to handle arguments with\nspaces.\n\nTo solve both problems in one go, use run_command_v_opt() to execute\nthe alias. It already does the right thing prepend \"sh -c \" to the\nalias.\n\nSigned-off-by: Erik Faye-Lund \n---\n\nThis fixes issue 553 in the msysGit issue tracker.\n\n git.c | 25 +++++++++++++------------\n 1 files changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 68334f6..5b0b9d8 100644\n--- a/git.c\n+++ b/git.c\n@@ -177,19 +177,20 @@ static int handle_alias(int *argcp, const char ***argv)\n \talias_string = alias_lookup(alias_command);\n \tif (alias_string) {\n \t\tif (alias_string[0] == '!') {\n+\t\t\tconst char **alias_argv;\n+\t\t\tint i;\n+\n \t\t\tcommit_pager_choice();\n-\t\t\tif (*argcp > 1) {\n-\t\t\t\tstruct strbuf buf;\n-\n-\t\t\t\tstrbuf_init(&buf, PATH_MAX);\n-\t\t\t\tstrbuf_addstr(&buf, alias_string);\n-\t\t\t\tsq_quote_argv(&buf, (*argv) + 1, PATH_MAX);\n-\t\t\t\tfree(alias_string);\n-\t\t\t\talias_string = buf.buf;\n-\t\t\t}\n-\t\t\ttrace_printf(\"trace: alias to shell cmd: %s => %s\\n\",\n-\t\t\t\t alias_command, alias_string + 1);\n-\t\t\tret = system(alias_string + 1);\n+\n+\t\t\t/* build alias_argv */\n+\t\t\talias_argv = malloc(sizeof(char *) * *argcp + 1);\n+\t\t\talias_argv[0] = alias_string + 1;\n+\t\t\tfor (i = 1; i < *argcp; ++i)\n+\t\t\t\talias_argv[i] = (*argv)[i];\n+\t\t\talias_argv[*argcp] = NULL;\n+\n+\t\t\tret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n+\n \t\t\tif (ret >= 0 && WIFEXITED(ret) &&\n \t\t\t WEXITSTATUS(ret) != 127)\n \t\t\t\texit(WEXITSTATUS(ret));\n-- \n1.7.3.3.585.g74f6e\n"},{"id":"159055","messageId":"20110106194101.GA14750@burratino","threadId":"26220","inReplyTo":"1294341187-3956-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH/RFC] alias: use run_command api to execute aliases","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-01-06T19:41:01Z","receivedAt":"2011-01-06T19:41:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Erik Faye-Lund wrote:\n\n> --- a/git.c\n> +++ b/git.c\n> @@ -177,19 +177,20 @@ static int handle_alias(int *argcp, const char ***argv)\n[...]\n> -\t\t\ttrace_printf(\"trace: alias to shell cmd: %s => %s\\n\",\n> -\t\t\t\t alias_command, alias_string + 1);\n\nReplaced by\n\n\ttrace: run_command: ...\n\n(followed by \"trace: exec: ...\" on non-Windows (execv_shell_cmd)).\nOk.\n\n> -\t\t\tret = system(alias_string + 1);\n> +\n> +\t\t\t/* build alias_argv */\n> +\t\t\talias_argv = malloc(sizeof(char *) * *argcp + 1);\n\nThis seems to be missing parentheses, so valgrind will complain\nexcept on 8-bit systems. ;-)\n\nWhat if malloc fails?\n\n> +\t\t\talias_argv[0] = alias_string + 1;\n> +\t\t\tfor (i = 1; i < *argcp; ++i)\n> +\t\t\t\talias_argv[i] = (*argv)[i];\n> +\t\t\talias_argv[*argcp] = NULL;\n\nNit: all these *argcp are noisy.\n\n> +\n> +\t\t\tret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n> +\n> \t\t\tif (ret >= 0 && WIFEXITED(ret) &&\n\nThe return value from run_command and from system do not mean\nthe same thing.\n\n> \t\t\tdie(\"Failed to run '%s' when expanding alias '%s'\",\n> \t\t\t alias_string + 1, alias_command);\n\nrun_command already prints an error message, but this one still\nseems useful since it mentions the alias.\n\nExcept as noted above,\nReviewed-by: Jonathan Nieder \n\nThanks.\n\ndiff --git a/git.c b/git.c\nindex 5b0b9d8..dbf061d 100644\n--- a/git.c\n+++ b/git.c\n@@ -178,22 +178,20 @@ static int handle_alias(int *argcp, const char ***argv)\n \tif (alias_string) {\n \t\tif (alias_string[0] == '!') {\n \t\t\tconst char **alias_argv;\n-\t\t\tint i;\n+\t\t\tint argc = *argcp, i;\n \n \t\t\tcommit_pager_choice();\n \n-\t\t\t/* build alias_argv */\n-\t\t\talias_argv = malloc(sizeof(char *) * *argcp + 1);\n+\t\t\talias_argv = xmalloc(sizeof(*alias_argv) * (argc + 1));\n \t\t\talias_argv[0] = alias_string + 1;\n-\t\t\tfor (i = 1; i < *argcp; ++i)\n+\t\t\tfor (i = 1; i < argc; ++i)\n \t\t\t\talias_argv[i] = (*argv)[i];\n-\t\t\talias_argv[*argcp] = NULL;\n+\t\t\talias_argv[argc] = NULL;\n \n \t\t\tret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n+\t\t\tif (ret >= 0)\t/* normal exit */\n+\t\t\t\texit(ret);\n \n-\t\t\tif (ret >= 0 && WIFEXITED(ret) &&\n-\t\t\t WEXITSTATUS(ret) != 127)\n-\t\t\t\texit(WEXITSTATUS(ret));\n \t\t\tdie(\"Failed to run '%s' when expanding alias '%s'\",\n \t\t\t alias_string + 1, alias_command);\n \t\t}\n"},{"id":"159058","messageId":"AANLkTiniRYH2jz69vhuSCzPMmAg4KQ5nowd7NqrmMYVw@mail.gmail.com","threadId":"26220","inReplyTo":"20110106194101.GA14750@burratino","subject":"Re: [PATCH/RFC] alias: use run_command api to execute aliases","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-01-06T19:52:04Z","receivedAt":"2011-01-06T19:52:04Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Jan 6, 2011 at 8:41 PM, Jonathan Nieder wrote:\n> Erik Faye-Lund wrote:\n>\n>> --- a/git.c\n>> +++ b/git.c\n>> @@ -177,19 +177,20 @@ static int handle_alias(int *argcp, const char ***argv)\n> [...]\n>> -                     trace_printf(\"trace: alias to shell cmd: %s => %s\\n\",\n>> -                                  alias_command, alias_string + 1);\n>\n> Replaced by\n>\n>        trace: run_command: ...\n>\n> (followed by \"trace: exec: ...\" on non-Windows (execv_shell_cmd)).\n> Ok.\n>\n>> -                     ret = system(alias_string + 1);\n>> +\n>> +                     /* build alias_argv */\n>> +                     alias_argv = malloc(sizeof(char *) * *argcp + 1);\n>\n> This seems to be missing parentheses, so valgrind will complain\n> except on 8-bit systems. ;-)\n>\n> What if malloc fails?\n>\n\n2x whoops :)\n\n>> +                     alias_argv[0] = alias_string + 1;\n>> +                     for (i = 1; i < *argcp; ++i)\n>> +                             alias_argv[i] = (*argv)[i];\n>> +                     alias_argv[*argcp] = NULL;\n>\n> Nit: all these *argcp are noisy.\n>\n\nYes. Fetching argc once is cleaner, thanks.\n\n>> +\n>> +                     ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n>> +\n>>                       if (ret >= 0 && WIFEXITED(ret) &&\n>\n> The return value from run_command and from system do not mean\n> the same thing.\n>\n\nYet another \"whoops\" :)\n\n>>                       die(\"Failed to run '%s' when expanding alias '%s'\",\n>>                           alias_string + 1, alias_command);\n>\n> run_command already prints an error message, but this one still\n> seems useful since it mentions the alias.\n>\n> Except as noted above,\n> Reviewed-by: Jonathan Nieder \n>\n\nThanks, I agree with all your comments. But why did you remove the \"/*\nbuild alias_argv */\"-comment? :)\n\nv2 coming up!\n"},{"id":"159087","messageId":"alpine.DEB.1.00.1101070216390.1542@bonsai2","threadId":"26220","inReplyTo":"1294341187-3956-1-git-send-email-kusmabite@gmail.com","subject":"Re: [msysGit] [PATCH/RFC] alias: use run_command api to execute aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-01-07T01:17:19Z","receivedAt":"2011-01-07T01:17:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 6 Jan 2011, Erik Faye-Lund wrote:\n\n> On Windows, system() executes with cmd.exe instead of /bin/sh. This\n> means that aliases currently has to be batch-scripts instead of\n> bourne-scripts. On top of that, cmd.exe does not handle single quotes,\n> which is what the code-path currently uses to handle arguments with\n> spaces.\n> \n> To solve both problems in one go, use run_command_v_opt() to execute\n> the alias. It already does the right thing prepend \"sh -c \" to the\n> alias.\n\nWould this not break setups where aliases were defined to execute batch \nscripts?\n\nIf this is true, I'm of two minds here.\n\nCiao,\nDscho\n"},{"id":"159108","messageId":"AANLkTi=6wG6khBAqLA8nki5-wbxQB-oYUAgMSqT-egpt@mail.gmail.com","threadId":"26220","inReplyTo":"alpine.DEB.1.00.1101070216390.1542@bonsai2","subject":"Re: [msysGit] [PATCH/RFC] alias: use run_command api to execute aliases","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-01-07T14:24:25Z","receivedAt":"2011-01-07T14:24:25Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Jan 7, 2011 at 2:17 AM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Thu, 6 Jan 2011, Erik Faye-Lund wrote:\n>\n>> On Windows, system() executes with cmd.exe instead of /bin/sh. This\n>> means that aliases currently has to be batch-scripts instead of\n>> bourne-scripts. On top of that, cmd.exe does not handle single quotes,\n>> which is what the code-path currently uses to handle arguments with\n>> spaces.\n>>\n>> To solve both problems in one go, use run_command_v_opt() to execute\n>> the alias. It already does the right thing prepend \"sh -c \" to the\n>> alias.\n>\n> Would this not break setups where aliases were defined to execute batch\n> scripts?\n>\n> If this is true, I'm of two minds here.\n>\n\nIt would indeed, but I wouldn't worry TOO much about it. We've clearly\ntold the users that Git for Windows is a tool that you have to be\nwilling to work on to use.\n\nBut I'm kind of of two minds here myself, but for a slightly different\nreason: I think Git for Windows SHOULD use cmd.exe to execute scripts.\nWe should be able to lose the msys-environment and still have the\nbasic functionality working. In that sense, this is a step in the\nwrong direction. But I'd rather have all code use the same code-path\nto execute scripts, and make a bit switch to cmd.exe together with\nporting all supplied scripts to batch-files some time in the future.\n"},{"id":"159111","messageId":"alpine.DEB.1.00.1101071551190.15247@pacific.mpi-cbg.de","threadId":"26220","inReplyTo":"AANLkTi=6wG6khBAqLA8nki5-wbxQB-oYUAgMSqT-egpt@mail.gmail.com","subject":"Re: [msysGit] [PATCH/RFC] alias: use run_command api to execute aliases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-01-07T14:51:45Z","receivedAt":"2011-01-07T14:51:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 7 Jan 2011, Erik Faye-Lund wrote:\n\n> On Fri, Jan 7, 2011 at 2:17 AM, Johannes Schindelin\n> wrote:\n>\n> > On Thu, 6 Jan 2011, Erik Faye-Lund wrote:\n> >\n> >> On Windows, system() executes with cmd.exe instead of /bin/sh. This \n> >> means that aliases currently has to be batch-scripts instead of \n> >> bourne-scripts. On top of that, cmd.exe does not handle single \n> >> quotes, which is what the code-path currently uses to handle \n> >> arguments with spaces.\n> >>\n> >> To solve both problems in one go, use run_command_v_opt() to execute \n> >> the alias. It already does the right thing prepend \"sh -c \" to the \n> >> alias.\n> >\n> > Would this not break setups where aliases were defined to execute \n> > batch scripts?\n> >\n> > If this is true, I'm of two minds here.\n> \n> It would indeed, but I wouldn't worry TOO much about it. We've clearly \n> told the users that Git for Windows is a tool that you have to be \n> willing to work on to use.\n> \n> But I'm kind of of two minds here myself, but for a slightly different \n> reason: I think Git for Windows SHOULD use cmd.exe to execute scripts. \n> We should be able to lose the msys-environment and still have the basic \n> functionality working. In that sense, this is a step in the wrong \n> direction. But I'd rather have all code use the same code-path to \n> execute scripts, and make a bit switch to cmd.exe together with porting \n> all supplied scripts to batch-files some time in the future.\n\nOkay, strike my objections, I agree now. Feel free to apply!\n\nThanks,\nDscho\n"},{"id":"159139","messageId":"7v8vyw7c52.fsf@alter.siamese.dyndns.org","threadId":"26220","inReplyTo":"alpine.DEB.1.00.1101071551190.15247@pacific.mpi-cbg.de","subject":"Re: [msysGit] [PATCH/RFC] alias: use run_command api to execute aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-07T19:21:45Z","receivedAt":"2011-01-07T19:21:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> But I'm kind of of two minds here myself, but for a slightly different \n>> reason: I think Git for Windows SHOULD use cmd.exe to execute scripts. \n>> We should be able to lose the msys-environment and still have the basic \n>> functionality working. In that sense, this is a step in the wrong \n>> direction. But I'd rather have all code use the same code-path to \n>> execute scripts, and make a bit switch to cmd.exe together with porting \n>> all supplied scripts to batch-files some time in the future.\n>\n> Okay, strike my objections, I agree now. Feel free to apply!\n\nFWIW, I agree with Erik's reasoning, too.\nThanks, both.\n"}]}