{"thread":{"id":"48196","subject":"Need help debugging issue in git","startedAt":"2018-04-02T00:36:32Z","lastAt":"2018-04-14T16:39:14Z","messageCount":3,"participants":["Robert Dailey","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"343561","messageId":"CAHd499C39sBTJy_AoCJxE2ZS4ECJju2jroY6_0E1MssCBtmC=A@mail.gmail.com","threadId":"48196","inReplyTo":null,"subject":"Need help debugging issue in git","fromName":"Robert Dailey","fromEmail":"rcdailey.lists@gmail.com","sentAt":"2018-04-02T00:36:24Z","receivedAt":"2018-04-02T00:36:32Z","isPatch":false,"sender":{"key":"rcdailey.lists@gmail.com","avatar":null},"body":"I'm struggling with a bug that I found introduced in git v2.13.2. The\nbug was not reproducible in v2.13.1. The issue is that using arguments\nlike \"@{-1}\" to aliases causes those curly braces to be removed, so\nonce the command is executed after alias processing the argument looks\nlike \"@-1\". This breaks any aliases you have that wrap `git log` and\nsuch. I originally opened the bug on the Git for Windows project\n(since I use Git mostly on Windows):\n\nhttps://github.com/git-for-windows/git/issues/1220\n\nAfter digging into the code of run-command.c some more, I have a\nchange that fixes the issue but it doesn't feel like the right\nsolution. Furthermore, the function I'm modifying doesn't seem to have\nbeen changed in the diff between v2.13.1 and v2.13.2. I'll provide my\ndiff at the bottom of this email for those curious as to what I've\nmodified.\n\nI was not able to reproduce this bug on Linux. However, the way\naliases get expanded seems extremely wasteful. Essentially if an alias\nalready has $@ in it, Git shouldn't try to insert another one. This\nresults in the alias being repeated multiple times. I'll give you an\nexample (Note there's a ton of detail in the github post linked\nearlier, and most of this is already there).\n\nHere is the alias being used for a test:\n\n[alias]\n    lgtest = !git log --oneline \\\"$@\\\"\n\nAnd here is the command I invoke for the test:\n\n$ git lgtest @{-1}\n\nI should get logs for the previously-checked-out branch.\n\nWhen `prepare_shell_cmd()` is called in run-command.c, it gets expanded like so:\n\n+ [0] \"sh\" const char *\n+ [1] \"-c\" const char *\n+ [2] \"git log --oneline \\\"$@\\\" \\\"$@\\\"\" const char *\n+ [3] \"git log --oneline \\\"$@\\\"\" const char *\n+ [4] \"@{-1}\" const char *\n\nWith my modifications (again, patch inline below) I get this:\n\n+ [0] \"sh\" const char *\n+ [1] \"-c\" const char *\n+ [2] \"git log --oneline \\\"$@\\\"\" const char *\n+ [3] \"@{-1}\" const char *\n\nThe second version looks much better. I think the constant nesting of\ncommands inside each other that the first version does is somehow\ncausing curly braces to be removed. I don't understand enough about\nshell processing to know why it would do this.\n\nEssentially I don't feel like I'm addressing the root cause here. I\nhope that someone has the time to take a peek and point me in the\nright direction. If I can make sense of this maybe I can make a proper\nfix somewhere.\n\nHere is the patch:\n\ndiff --git a/run-command.c b/run-command.c\nindex 31fc5ea86e..39bab7f5b2 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -261,10 +261,10 @@ static const char **prepare_shell_cmd(struct\nargv_array *out, const char **argv)\n  if (!argv[1])\n  argv_array_push(out, argv[0]);\n  else\n- argv_array_pushf(out, \"%s \\\"$@\\\"\", argv[0]);\n+ argv_array_pushf(out, \"%s\", argv[0]);\n  }\n\n- argv_array_pushv(out, argv);\n+ argv_array_pushv(out, argv+1);\n  return out->argv;\n }\n"},{"id":"343571","messageId":"3152c739-5c80-99b3-ac16-31fe348b7657@kdbg.org","threadId":"48196","inReplyTo":"CAHd499C39sBTJy_AoCJxE2ZS4ECJju2jroY6_0E1MssCBtmC=A@mail.gmail.com","subject":"Re: Need help debugging issue in git","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-04-02T06:53:35Z","receivedAt":"2018-04-02T06:53:43Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.04.2018 um 02:36 schrieb Robert Dailey:\n> I'm struggling with a bug that I found introduced in git v2.13.2. The\n> bug was not reproducible in v2.13.1. The issue is that using arguments\n> like \"@{-1}\" to aliases causes those curly braces to be removed, so\n> once the command is executed after alias processing the argument looks\n> like \"@-1\". This breaks any aliases you have that wrap `git log` and\n> such. I originally opened the bug on the Git for Windows project\n> (since I use Git mostly on Windows):\n> \n> https://github.com/git-for-windows/git/issues/1220\n...\n> Here is the alias being used for a test:\n> \n> [alias]\n>      lgtest = !git log --oneline \\\"$@\\\"\n> \n> And here is the command I invoke for the test:\n> \n> $ git lgtest @{-1}\n> \n> I should get logs for the previously-checked-out branch.\n> \n> When `prepare_shell_cmd()` is called in run-command.c, it gets expanded like so:\n> \n> + [0] \"sh\" const char *\n> + [1] \"-c\" const char *\n> + [2] \"git log --oneline \\\"$@\\\" \\\"$@\\\"\" const char *\n> + [3] \"git log --oneline \\\"$@\\\"\" const char *\n> + [4] \"@{-1}\" const char *\n> \n> With my modifications (again, patch inline below) I get this:\n> \n> + [0] \"sh\" const char *\n> + [1] \"-c\" const char *\n> + [2] \"git log --oneline \\\"$@\\\"\" const char *\n> + [3] \"@{-1}\" const char *\n> \n> The second version looks much better.\n\nBut this is wrong. Try this on the command line:\n\n   sh -c 'echo \"$@\"' a b c\n\nNotice how this prints only 'b c', not 'a b c'. The reason is that the \nargument 'a' is treated like a \"script\" name, i.e. what you get for \n\"$0\", and 'b' and 'c' as the actual arguments to the \"script\".\n\nThat is, you must fill in some dummy \"script\" name at slot [3], and \nrun_command chooses to put the alias text there.\n\n> I think the constant nesting of\n> commands inside each other that the first version does is somehow\n> causing curly braces to be removed. I don't understand enough about\n> shell processing to know why it would do this.\n\nSome shells expand the curly braces. They must get lost somewhere by one \nof the two shell invocations that happen on the way.\n\nBTW, you don't happen to have a file named '@-1' in your directory, most \nlikely by accident?\n\n-- Hannes\n"},{"id":"344689","messageId":"CAHd499ApQV-juNU4tZ7XgDx3y4hi8u2-dwwa=eytFxrwVaHRmA@mail.gmail.com","threadId":"48196","inReplyTo":"3152c739-5c80-99b3-ac16-31fe348b7657@kdbg.org","subject":"Re: Need help debugging issue in git","fromName":"Robert Dailey","fromEmail":"rcdailey.lists@gmail.com","sentAt":"2018-04-14T16:39:07Z","receivedAt":"2018-04-14T16:39:14Z","isPatch":false,"sender":{"key":"rcdailey.lists@gmail.com","avatar":null},"body":"On Mon, Apr 2, 2018 at 1:53 AM, Johannes Sixt <j6t@kdbg.org> wrote:\n> Am 02.04.2018 um 02:36 schrieb Robert Dailey:\n>>\n>> I'm struggling with a bug that I found introduced in git v2.13.2. The\n>> bug was not reproducible in v2.13.1. The issue is that using arguments\n>> like \"@{-1}\" to aliases causes those curly braces to be removed, so\n>> once the command is executed after alias processing the argument looks\n>> like \"@-1\". This breaks any aliases you have that wrap `git log` and\n>> such. I originally opened the bug on the Git for Windows project\n>> (since I use Git mostly on Windows):\n>>\n>> https://github.com/git-for-windows/git/issues/1220\n>\n> ...\n>>\n>> Here is the alias being used for a test:\n>>\n>> [alias]\n>>      lgtest = !git log --oneline \\\"$@\\\"\n>>\n>> And here is the command I invoke for the test:\n>>\n>> $ git lgtest @{-1}\n>>\n>> I should get logs for the previously-checked-out branch.\n>>\n>> When `prepare_shell_cmd()` is called in run-command.c, it gets expanded\n>> like so:\n>>\n>> + [0] \"sh\" const char *\n>> + [1] \"-c\" const char *\n>> + [2] \"git log --oneline \\\"$@\\\" \\\"$@\\\"\" const char *\n>> + [3] \"git log --oneline \\\"$@\\\"\" const char *\n>> + [4] \"@{-1}\" const char *\n>>\n>> With my modifications (again, patch inline below) I get this:\n>>\n>> + [0] \"sh\" const char *\n>> + [1] \"-c\" const char *\n>> + [2] \"git log --oneline \\\"$@\\\"\" const char *\n>> + [3] \"@{-1}\" const char *\n>>\n>> The second version looks much better.\n>\n>\n> But this is wrong. Try this on the command line:\n>\n>   sh -c 'echo \"$@\"' a b c\n>\n> Notice how this prints only 'b c', not 'a b c'. The reason is that the\n> argument 'a' is treated like a \"script\" name, i.e. what you get for \"$0\",\n> and 'b' and 'c' as the actual arguments to the \"script\".\n>\n> That is, you must fill in some dummy \"script\" name at slot [3], and\n> run_command chooses to put the alias text there.\n>\n>> I think the constant nesting of\n>> commands inside each other that the first version does is somehow\n>> causing curly braces to be removed. I don't understand enough about\n>> shell processing to know why it would do this.\n>\n>\n> Some shells expand the curly braces. They must get lost somewhere by one of\n> the two shell invocations that happen on the way.\n>\n> BTW, you don't happen to have a file named '@-1' in your directory, most\n> likely by accident?\n\nThanks for your help. I checked for @-1 but I do not have a file with\nthat name (good catch though). I contacted the MinGW mailing list and\nthey seem to indicate that {-1} is a valid brace expansion. I was able\nto verify the git.exe code itself is not causing this problem. It\nseems to be GNU bash doing it. But oddly enough, the Ubuntu version of\nBash for example does not process {-1} as a brace expansion. It seems\nweird to me that Git uses a syntax for a portion of its revision\nspecification that could be ambiguously treated as syntax processed by\nBash. In other words, I feel like this would have been designed into\nGit years ago, so I'm not sure why this is a problem now all of a\nsudden.\n\nTheir suggested solution was to start quoting items in the list or\nescaping the braces, but that will make git revisions less intuitive\nto use and make commands more tedious to type. I am still discussing\nthings over there but for the purposes of the Git mailing list, I\nthink it's clear at this point this is not an issue with Git itself.\nHaving said that, thanks again for the help!!\n"}]}