{"thread":{"id":"29510","subject":"[PATCH v3 0/4] completion: couple of cleanups","startedAt":"2012-02-02T01:15:16Z","lastAt":"2012-02-06T23:11:01Z","messageCount":38,"participants":["Felipe Contreras","Jonathan Nieder","Thomas Rast","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"183527","messageId":"1328145320-14071-1-git-send-email-felipe.contreras@gmail.com","threadId":"29510","inReplyTo":null,"subject":"[PATCH v3 0/4] completion: couple of cleanups","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-02T01:15:16Z","receivedAt":"2012-02-02T01:15:16Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"And an improvement for zsh.\n\nJunio: I see you already picked most of them for 'pu', but I've made further changes based on the feedback:\n\n * completion: be nicer with zsh\n\tImproved the code-style\n\n  * completion: simplify __gitcomp*\n\tFix\n\tImproved commit message\n\nCheers.\n\nFelipe Contreras (4):\n  completion: be nicer with zsh\n  completion: simplify __git_remotes\n  completion: remove unused code\n  completion: simplify __gitcomp*\n\n contrib/completion/git-completion.bash |   66 +++++---------------------------\n 1 files changed, 10 insertions(+), 56 deletions(-)\n\n-- \n1.7.9\n"},{"id":"183549","messageId":"20120202081622.GB3823@burratino","threadId":"29510","inReplyTo":"1328145320-14071-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T08:16:22Z","receivedAt":"2012-02-02T08:16:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> And yet another bug in zsh[1] causes a mismatch; zsh seems to have\n> problem emulating wordspliting, but only on the command substitution.\n\nPatches didn't hit the list again.  Any idea why?\n"},{"id":"183562","messageId":"CAMP44s3FxUmnpQevoV2ARJpWK9CJ16zXDmpJRDOLHNW6RdSc5Q@mail.gmail.com","threadId":"29510","inReplyTo":"20120202081622.GB3823@burratino","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-02T08:34:06Z","receivedAt":"2012-02-02T08:34:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Felipe Contreras wrote:\n>\n>> And yet another bug in zsh[1] causes a mismatch; zsh seems to have\n>> problem emulating wordspliting, but only on the command substitution.\n>\n> Patches didn't hit the list again.  Any idea why?\n\nNo. A bug in list software?\n\nI didn't get any warning or error.\n\n-- \nFelipe Contreras\n"},{"id":"183563","messageId":"20120202084859.GC3823@burratino","threadId":"29510","inReplyTo":"1328145320-14071-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T08:48:59Z","receivedAt":"2012-02-02T08:48:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nFelipe Contreras wrote:\n\n> Felipe Contreras (4):\n>   completion: be nicer with zsh\n\nSince I can't find this patch in the mail archive, I'll reply here.\nLuckily the most important bit is above already.\n\nI think I mentioned before that this subject line is what will appear\nin the shortlog and the shortlog is all that some people will see of\nthe changelog, so it should include a self-contained description of\nthe impact of the patch.\n\nHowever, clearly I did not say it clearly enough. :)  I guess it's\nbetter to take a cue from storytellers and show rather than tell.\n(Please don't take this as a precedent --- I will not always be doing\nthe style fixes myself, and sometimes will consider a patch to scratch\nsomeone else's itch not worth the trouble and work on something else.)\n\n-- >8 --\nFrom: Felipe Contreras <felipe.contreras@gmail.com>\nDate: Thu, 2 Feb 2012 03:15:17 +0200\nSubject: completion: avoid default value assignment on : true command\n\nzsh versions from 4.3.0 to present (4.3.15) do not correctly propagate\nthe SH_WORD_SPLIT option into the subshell in ${foo:=$(bar)}\nexpressions.  For example, after running\n\n\temulate sh\n\tfn () {\n\t\tvar='one two'\n\t\tprintf '%s\\n' $var\n\t}\n\tx=$(fn)\n\t: ${y=$(fn)}\n\nprinting \"$x\" results in two lines as expected, but printing \"$y\"\nresults in a single line because $var is expanded as a single word\nwhen evaluating fn to compute y.\n\nSo avoid the construct, and use an explicit 'test -n \"$foo\" ||\nfoo=$(bar)' instead.  This fixes a bug tht caused all commands to be\ntreated as porcelain and show up in \"git <TAB><TAB>\" completion,\nbecause the list of all commands was treated as a single word in\n__git_list_porcelain_commands and did not match any of the patterns\nthat would usually cause plumbing to be excluded.\n\n[jn: clarified commit message, indentation style fix]\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n contrib/completion/git-completion.bash |    9 ++++++---\n 1 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 78be1958..d7965daf 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -676,7 +676,8 @@ __git_merge_strategies=\n # is needed.\n __git_compute_merge_strategies ()\n {\n-\t: ${__git_merge_strategies:=$(__git_list_merge_strategies)}\n+\t[ -n \"$__git_merge_strategies\" ] ||\n+\t__git_merge_strategies=$(__git_list_merge_strategies)\n }\n \n __git_complete_revlist_file ()\n@@ -854,7 +855,8 @@ __git_list_all_commands ()\n __git_all_commands=\n __git_compute_all_commands ()\n {\n-\t: ${__git_all_commands:=$(__git_list_all_commands)}\n+\t[ -n \"$__git_all_commands\" ] ||\n+\t__git_all_commands=$(__git_list_all_commands)\n }\n \n __git_list_porcelain_commands ()\n@@ -947,7 +949,8 @@ __git_porcelain_commands=\n __git_compute_porcelain_commands ()\n {\n \t__git_compute_all_commands\n-\t: ${__git_porcelain_commands:=$(__git_list_porcelain_commands)}\n+\t[ -n \"$__git_porcelain_commands\" ] ||\n+\t__git_porcelain_commands=$(__git_list_porcelain_commands)\n }\n \n __git_pretty_aliases ()\n-- \n1.7.9\n"},{"id":"183565","messageId":"20120202090529.GD3823@burratino","threadId":"29510","inReplyTo":"1328145320-14071-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3 2/4] completion: simplify __git_remotes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T09:05:29Z","receivedAt":"2012-02-02T09:05:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n>   completion: simplify __git_remotes\n>   completion: remove unused code\n\nIn the same spirit of stealing potential shortlog lines from Junio:\n\n-- >8 --\nFrom: Felipe Contreras <felipe.contreras@gmail.com>\nSubject: completion: use ls -1 instead of rolling a loop to do that ourselves\n\nThis simplifies the code a great deal.  In particular, it allows us to\nget rid of __git_shopt, which is used only in this fuction to enable\n'nullglob' in zsh.\n\n[jn: squashed with a patch that actually gets rid of __git_shopt]\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nThe diffstat says it all.\n\n contrib/completion/git-completion.bash |   37 +-------------------------------\n 1 files changed, 1 insertions(+), 36 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 78be1958..4612dde9 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -644,12 +644,7 @@ __git_refs_remotes ()\n __git_remotes ()\n {\n \tlocal i ngoff IFS=$'\\n' d=\"$(__gitdir)\"\n-\t__git_shopt -q nullglob || ngoff=1\n-\t__git_shopt -s nullglob\n-\tfor i in \"$d/remotes\"/*; do\n-\t\techo ${i#$d/remotes/}\n-\tdone\n-\t[ \"$ngoff\" ] && __git_shopt -u nullglob\n+\ttest -d \"$d/remotes\" && ls -1 \"$d/remotes\"\n \tfor i in $(git --git-dir=\"$d\" config --get-regexp 'remote\\..*\\.url' 2>/dev/null); do\n \t\ti=\"${i#remote.}\"\n \t\techo \"${i/.url*/}\"\n@@ -2733,33 +2728,3 @@ if [ Cygwin = \"$(uname -o 2>/dev/null)\" ]; then\n complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \\\n \t|| complete -o default -o nospace -F _git git.exe\n fi\n-\n-if [[ -n ${ZSH_VERSION-} ]]; then\n-\t__git_shopt () {\n-\t\tlocal option\n-\t\tif [ $# -ne 2 ]; then\n-\t\t\techo \"USAGE: $0 (-q|-s|-u) <option>\" >&2\n-\t\t\treturn 1\n-\t\tfi\n-\t\tcase \"$2\" in\n-\t\tnullglob)\n-\t\t\toption=\"$2\"\n-\t\t\t;;\n-\t\t*)\n-\t\t\techo \"$0: invalid option: $2\" >&2\n-\t\t\treturn 1\n-\t\tesac\n-\t\tcase \"$1\" in\n-\t\t-q)\tsetopt | grep -q \"$option\" ;;\n-\t\t-u)\tunsetopt \"$option\" ;;\n-\t\t-s)\tsetopt \"$option\" ;;\n-\t\t*)\n-\t\t\techo \"$0: invalid flag: $1\" >&2\n-\t\t\treturn 1\n-\t\tesac\n-\t}\n-else\n-\t__git_shopt () {\n-\t\tshopt \"$@\"\n-\t}\n-fi\n-- \n1.7.9\n"},{"id":"183566","messageId":"20120202091059.GE3823@burratino","threadId":"29510","inReplyTo":"CAMP44s3FxUmnpQevoV2ARJpWK9CJ16zXDmpJRDOLHNW6RdSc5Q@mail.gmail.com","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T09:10:59Z","receivedAt":"2012-02-02T09:10:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(dropping Shawn from cc list, since I don't think he's touched the\n completion code for years)\nFelipe Contreras wrote:\n> On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n>> Patches didn't hit the list again.  Any idea why?\n>\n> No. A bug in list software?\n>\n> I didn't get any warning or error.\n\nExcept in response to HTML attachments, I've never seen vger return any\nexplanation when it decides a message is spam.  It just discards them.\n\nSee [1] for details.  If there's no obvious explanation, I'd suggest\ncontacting the postmaster.\n\nHope that helps, and sorry for the fuss,\nJonathan\n\n[1] http://vger.kernel.org/majordomo-info.html\n"},{"id":"183567","messageId":"CAMP44s0nD4p9=fwpLwchmGJ123onLmRaSPmOL+cYpTFCJ-jwXw@mail.gmail.com","threadId":"29510","inReplyTo":"20120202091059.GE3823@burratino","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-02T09:38:49Z","receivedAt":"2012-02-02T09:38:49Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> (dropping Shawn from cc list, since I don't think he's touched the\n>  completion code for years)\n> Felipe Contreras wrote:\n>> On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n>>> Patches didn't hit the list again.  Any idea why?\n>>\n>> No. A bug in list software?\n>>\n>> I didn't get any warning or error.\n>\n> Except in response to HTML attachments, I've never seen vger return any\n> explanation when it decides a message is spam.  It just discards them.\n\nWhich is wrong, because when it discards them wrongly nobody knows why.\n\n> See [1] for details.  If there's no obvious explanation, I'd suggest\n> contacting the postmaster.\n\nBut there's nothing like the taboo words in the mail:\nhttp://vger.kernel.org/majordomo-taboos.txt\n\n-- \nFelipe Contreras\n"},{"id":"183569","messageId":"20120202094624.GF3823@burratino","threadId":"29510","inReplyTo":"CAMP44s0nD4p9=fwpLwchmGJ123onLmRaSPmOL+cYpTFCJ-jwXw@mail.gmail.com","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T09:46:24Z","receivedAt":"2012-02-02T09:46:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n> On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n>> See [1] for details.  If there's no obvious explanation, I'd suggest\n>> contacting the postmaster.\n>\n> But there's nothing like the taboo words in the mail:\n> http://vger.kernel.org/majordomo-taboos.txt\n\nWhy are you telling me?  I am not the postmaster.  I can't do anything\nto investigate it or fix it.\n"},{"id":"183573","messageId":"CAMP44s0w1eXWWaMT3WAfLxFHPQvc9dp33cyJ=T2im6g7rsrKhw@mail.gmail.com","threadId":"29510","inReplyTo":"20120202084859.GC3823@burratino","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-02T10:12:35Z","receivedAt":"2012-02-02T10:12:35Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Feb 2, 2012 at 10:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Felipe Contreras wrote:\n>\n>> Felipe Contreras (4):\n>>   completion: be nicer with zsh\n>\n> Since I can't find this patch in the mail archive, I'll reply here.\n> Luckily the most important bit is above already.\n>\n> I think I mentioned before that this subject line is what will appear\n> in the shortlog and the shortlog is all that some people will see of\n> the changelog, so it should include a self-contained description of\n> the impact of the patch.\n\nExactly, and \"completion: avoid default value assignment on : true\ncommand\" tells *nothing* to most people. Why is this patch needed? Do\nI care about it?\n\nOTOH \"completion: be nicer with zsh\" explains the purpose of the patch\nand people that don't care about zsh can happily ignore it if they\nwant, and the ones that care about zsh might want to back port it, or\nwhatever.\n\nAlso, if you are looking at a shortlog and looking for patches related\nto zsh, you would easily see this.\n\n\"completion: avoid default value assignment on : true command\" almost\nensures that the people reading that summary would need to read\nfurther to see *why*.\n\n> -- >8 --\n> From: Felipe Contreras <felipe.contreras@gmail.com>\n> Date: Thu, 2 Feb 2012 03:15:17 +0200\n> Subject: completion: avoid default value assignment on : true command\n\nNo useful information provided yet. Should I care about this? I guess\nI need to open the mail and see.\n\n> zsh versions from 4.3.0 to present (4.3.15) do not correctly propagate\n> the SH_WORD_SPLIT option into the subshell in ${foo:=$(bar)}\n> expressions.  For example, after running\n\nOk, a bug in zsh, I can skip this. Or if I care about zsh, then I\nstill have to read further, because I still don't know what's the\nproblem.\n\nAnd BTW, this is extremely detailed. Where's the evidence? Is there a\nbug report? How can I follow this to find out when it's fixed. (hint:\nmissing link)\n\n>        emulate sh\n>        fn () {\n>                var='one two'\n>                printf '%s\\n' $var\n>        }\n>        x=$(fn)\n>        : ${y=$(fn)}\n>\n> printing \"$x\" results in two lines as expected, but printing \"$y\"\n> results in a single line because $var is expanded as a single word\n> when evaluating fn to compute y.\n\nOk, still reading.\n\n> So avoid the construct, and use an explicit 'test -n \"$foo\" ||\n> foo=$(bar)' instead.\n\nYeah...\n\n> This fixes a bug tht caused all commands to be\n> treated as porcelain and show up in \"git <TAB><TAB>\" completion,\n> because the list of all commands was treated as a single word in\n> __git_list_porcelain_commands and did not match any of the patterns\n> that would usually cause plumbing to be excluded.\n\nAha! Finally we arrive to the important part.\n\nI guess we have different styles of writing. Personally, I don't see\nthe point of forcing the readers to go through the whole thing.\nCompare this to mine:\n\n| Subject: [PATCH v3 1/4] completion: be nicer with zsh\n\nAt this point most people can skip this.\n\n| And yet another bug in zsh[1] causes a mismatch; zsh seems to have\n| problem emulating wordspliting, but only on the command substitution.\n|\n| Let's avoid it.\n\nA bug in zsh in certain situations... got it.\n\n| I found this issue because __git_compute_porcelain_commands listed all\n| commands (not only porcelain).\n\nAt this point it might have been better to mention \"git <TAB><TAB>\"\ninstead, as you did. But not a big deal, the problem is clear; all\ncommands are listed.\n\nAlmost all people would stop at this point; either they don't care\nabout zsh, or they care, and they want the patch.\n\n| This is because in zsh the following code:\n|\n|  for i in $__git_all_commands\n|\n| would evaluate $__git_all_commands as a single word (with spaces),\n| ${=__git_all_commands} should be used to do word splitting expansion\n| (unless SH_WORD_SPLIT is used).\n|\n| sh emulation should take care of that, but the command expantion is\n| messing up with that.\n\nAnd more details for the skeptics, or the ones that want to learn more.\n\n| tl;dr: $__git_porcelain_commands = $__git_all_commands\n\nWrapping it up, to make clear what happens.\n\n| [1] http://article.gmane.org/gmane.comp.shells.zsh.devel/24296\n\nAnd then a link to further details, discussion, and possible fixes.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"183574","messageId":"CAMP44s058VWdjJW-pUH5V4WJ4B3r6Oq9STqGHVREuD5rtP1V8Q@mail.gmail.com","threadId":"29510","inReplyTo":"20120202094624.GF3823@burratino","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-02T10:18:54Z","receivedAt":"2012-02-02T10:18:54Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Feb 2, 2012 at 11:46 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Felipe Contreras wrote:\n>> On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n>>> See [1] for details.  If there's no obvious explanation, I'd suggest\n>>> contacting the postmaster.\n>>\n>> But there's nothing like the taboo words in the mail:\n>> http://vger.kernel.org/majordomo-taboos.txt\n>\n> Why are you telling me?  I am not the postmaster.  I can't do anything\n> to investigate it or fix it.\n\nI'm not telling you, this is a conversation on a public mailing list.\nOther people might know something about, or they might hit the same\nissue in the future. It's good to state things on record.\n\nAnd FTR, I had already contacted the postmaster, which has been not\nexactly helpful in previous occasions, but lets see.\n\n-- \nFelipe Contreras\n"},{"id":"183576","messageId":"8739at8qw6.fsf@thomas.inf.ethz.ch","threadId":"29510","inReplyTo":"CAMP44s0w1eXWWaMT3WAfLxFHPQvc9dp33cyJ=T2im6g7rsrKhw@mail.gmail.com","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-02-02T10:35:05Z","receivedAt":"2012-02-02T10:35:05Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> On Thu, Feb 2, 2012 at 10:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n> Exactly, and \"completion: avoid default value assignment on : true\n> command\" tells *nothing* to most people. Why is this patch needed? Do\n> I care about it?\n>\n> OTOH \"completion: be nicer with zsh\" explains the purpose of the patch\n> and people that don't care about zsh can happily ignore it if they\n> want, and the ones that care about zsh might want to back port it, or\n> whatever.\n\nPerhaps you could compromise on\n\n  completion: work around zsh word splitting bug in : ${foo:=$(bar)}\n\n?\n\n\n> | tl;dr: $__git_porcelain_commands = $__git_all_commands\n>\n> Wrapping it up, to make clear what happens.\n\nI think this is not good style for a commit message.  Apart from the\nvery trendy use of tl;dr, it doesn't even properly summarize the cause\n*or* the user-visible symptom.  It just states how the confusion\npropagates somewhere in the middle of the code.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"183578","messageId":"20120202105006.GH3823@burratino","threadId":"29510","inReplyTo":"8739at8qw6.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T10:50:06Z","receivedAt":"2012-02-02T10:50:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Rast wrote:\n\n> Perhaps you could compromise on\n>\n>   completion: work around zsh word splitting bug in : ${foo:=$(bar)}\n\nThanks, that looks like a good subject line to me.  It gives a hint of\nwhat the patch is trying to do, and it does not try to fool me into\nthinking that if I use bash the patch does not affect me.\n\nFelipe, I'm not going to respond to the rest of your message.  Perhaps\nsomeone more patient than I am will, or if someone has specific\nquestions for me, I'll be glad to help.\n"},{"id":"183579","messageId":"20120202105527.GI3823@burratino","threadId":"29510","inReplyTo":"20120202105006.GH3823@burratino","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T10:55:27Z","receivedAt":"2012-02-02T10:55:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Thomas Rast wrote:\n\n>> Perhaps you could compromise on\n>>\n>>   completion: work around zsh word splitting bug in : ${foo:=$(bar)}\n>\n> Thanks, that looks like a good subject line to me.\n\nAh, except it's not a word splitting bug.  It's an option propagation\nbug.\n"},{"id":"183581","messageId":"CAMP44s3BcSGPCKPr_2EpGQRAv_kDM2_OQiyKDT7qY=QR0cHYpQ@mail.gmail.com","threadId":"29510","inReplyTo":"8739at8qw6.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-02T11:00:27Z","receivedAt":"2012-02-02T11:00:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Feb 2, 2012 at 12:35 PM, Thomas Rast <trast@inf.ethz.ch> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> On Thu, Feb 2, 2012 at 10:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>>\n>> Exactly, and \"completion: avoid default value assignment on : true\n>> command\" tells *nothing* to most people. Why is this patch needed? Do\n>> I care about it?\n>>\n>> OTOH \"completion: be nicer with zsh\" explains the purpose of the patch\n>> and people that don't care about zsh can happily ignore it if they\n>> want, and the ones that care about zsh might want to back port it, or\n>> whatever.\n>\n> Perhaps you could compromise on\n>\n>  completion: work around zsh word splitting bug in : ${foo:=$(bar)}\n\nYes, that sounds better, perhaps:\n\ncompletion: work around zsh option propagation bug\n\n>> | tl;dr: $__git_porcelain_commands = $__git_all_commands\n>>\n>> Wrapping it up, to make clear what happens.\n>\n> I think this is not good style for a commit message.  Apart from the\n> very trendy use of tl;dr, it doesn't even properly summarize the cause\n> *or* the user-visible symptom.  It just states how the confusion\n> propagates somewhere in the middle of the code.\n\nI don't think the cause or the user-visible symptom need any summary.\n\nI was trying to summarize this:\n---\nThis is because in zsh the following code:\n\n for i in $__git_all_commands\n\nwould evaluate $__git_all_commands as a single word (with spaces),\n${=__git_all_commands} should be used to do word splitting expansion\n(unless SH_WORD_SPLIT is used).\n\nsh emulation should take care of that, but the command expantion is\nmessing up with that.\n---\n\n-- \nFelipe Contreras\n"},{"id":"183637","messageId":"7v8vklvxwh.fsf@alter.siamese.dyndns.org","threadId":"29510","inReplyTo":"20120202084859.GC3823@burratino","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-02T19:27:26Z","receivedAt":"2012-02-02T19:27:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Felipe Contreras wrote:\n>\n>> Felipe Contreras (4):\n>>   completion: be nicer with zsh\n>\n> Since I can't find this patch in the mail archive, I'll reply here.\n> Luckily the most important bit is above already.\n\nThanks for playing a mail-relay.  Except for the much more readable log\nmessage you have here, the result matches what I have at 06357ef (modulo\ntest vs '[').  I'll replace what I queued.\n\n> I think I mentioned before that this subject line is what will appear\n> in the shortlog and the shortlog is all that some people will see of\n> the changelog, so it should include a self-contained description of\n> the impact of the patch.\n>\n> However, clearly I did not say it clearly enough. :)  I guess it's\n> better to take a cue from storytellers and show rather than tell.\n\nVery big thanks for this ;-)\n"},{"id":"183641","messageId":"7v39atvwxz.fsf@alter.siamese.dyndns.org","threadId":"29510","inReplyTo":"1328145320-14071-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3 0/4] completion: couple of cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-02T19:48:08Z","receivedAt":"2012-02-02T19:48:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Felipe Contreras (4):\n>   completion: be nicer with zsh\n>   completion: simplify __git_remotes\n>   completion: remove unused code\n>   completion: simplify __gitcomp*\n\nMy understanding is that the two patches relayed by Jonathan cover the\nfirst three entries, so here is my attempt to reconstruct the last one\nwithout seeing a single bit from the v3 series ;-)\n\n\n-- >8 --\nFrom: Felipe Contreras <felipe.contreras@gmail.com>\nSubject: [PATCH] completion: simplify __gitcomp and __gitcomp_nl implementations\n\nThese shell functions are written in an unnecessarily verbose way;\nsimplify their \"conditionally use $<number> after checking $# against\n<number>\" logic by using shell's built-in conditional substitution\nfacilities.\n\nAlso remove the first of the two assignments to IFS in __gitcomp_nl\nthat does not have any effect.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n contrib/completion/git-completion.bash |   19 +++----------------\n 1 files changed, 3 insertions(+), 16 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 8dd4e44..1ce1d6e 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -495,11 +495,8 @@ fi\n # 4: A suffix to be appended to each possible completion word (optional).\n __gitcomp ()\n {\n-\tlocal cur_=\"$cur\"\n+\tlocal cur_=\"${3-$cur}\"\n \n-\tif [ $# -gt 2 ]; then\n-\t\tcur_=\"$3\"\n-\tfi\n \tcase \"$cur_\" in\n \t--*=)\n \t\tCOMPREPLY=()\n@@ -524,18 +521,8 @@ __gitcomp ()\n #    appended.\n __gitcomp_nl ()\n {\n-\tlocal s=$'\\n' IFS=' '$'\\t'$'\\n'\n-\tlocal cur_=\"$cur\" suffix=\" \"\n-\n-\tif [ $# -gt 2 ]; then\n-\t\tcur_=\"$3\"\n-\t\tif [ $# -gt 3 ]; then\n-\t\t\tsuffix=\"$4\"\n-\t\tfi\n-\tfi\n-\n-\tIFS=$s\n-\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"$suffix\" -W \"$1\" -- \"$cur_\"))\n+\tlocal IFS=$'\\n'\n+\tCOMPREPLY=($(compgen -P \"${2-}\" -S \"${4- }\" -W \"$1\" -- \"${3-$cur}\"))\n }\n \n __git_heads ()\n-- \n1.7.9.172.ge26ae\n"},{"id":"183663","messageId":"CAMP44s1gWNG+jJ6M7OnLS-1VA5YPa07LJfnrvdbsQb0MOJB7PA@mail.gmail.com","threadId":"29510","inReplyTo":"7v8vklvxwh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-02T20:45:39Z","receivedAt":"2012-02-02T20:45:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Feb 2, 2012 at 9:27 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n>> However, clearly I did not say it clearly enough. :)  I guess it's\n>> better to take a cue from storytellers and show rather than tell.\n>\n> Very big thanks for this ;-)\n\nNot a single comment regarding what I said? I don't see what's not\nsensible about a commit message with this order:\n\n 1) short description of the *purpose* of the patch\n 2) Summary of the problem.\n 3) Proposed solution\n 4) Extra information, useful for future references, etc.\n\nThis ensures the most relevant information for most people is at the top.\n\n-- \nFelipe Contreras\n"},{"id":"183680","messageId":"7vhaz8vkhd.fsf@alter.siamese.dyndns.org","threadId":"29510","inReplyTo":"CAMP44s1gWNG+jJ6M7OnLS-1VA5YPa07LJfnrvdbsQb0MOJB7PA@mail.gmail.com","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-03T00:17:18Z","receivedAt":"2012-02-03T00:17:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> On Thu, Feb 2, 2012 at 9:27 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Jonathan Nieder <jrnieder@gmail.com> writes:\n>>> However, clearly I did not say it clearly enough. :) I guess it's\n>>> better to take a cue from storytellers and show rather than tell.\n>>\n>> Very big thanks for this ;-)\n>\n> Not a single comment regarding what I said?\n\nWhat entitles you to force me to refraining from commenting at all until I\nread everything in my mailbox and after waiting for a while to make sure\nthere is no more to come to the thread?\n\nIn any case, \"be nicer with zsh\" conveys no more meaningful information\nthan \"this is some patch about zsh\".  Let's try to avoid warm and fuzzy\nwords that imply \"goodness\", e.g. \"improve\" and \"be nicer with\" because\nnobody sends a patch to purposefully make Git worse and expects it to be\napplied.\n\nI found Jonathan's alternative \"avoid default value assignment on : true\ncommand\" at least a bit better for the purpose of jogging the short-term\nmemory in the \"'git shortlog v1.7.9.. contrib/completion/' tells us that\nwe have applied several patches, and I remember that : ${var=word} one!\"\nsense.  It is not super-useful for the longer term, though.\n\nHere is what I ended up in preparation for queuing the series.  I still\nhaven't seen any version of 4/4, but please check $gmane/189683 and see if\nthat matches what you intended.  Also I am assuming $gmane/189606 relayed\nby Jonathan is a squash between your 2 and 3 (which didn't reach me), so\nplease advise if that does not match what you want to have.\n\nThanks.\n\n-- >8 --\nFrom: Felipe Contreras <felipe.contreras@gmail.com>\nSubject: [PATCH] completion: work around zsh option propagation bug\n\nzsh versions from 4.3.0 to present (4.3.15) do not correctly propagate the\nSH_WORD_SPLIT option into the subshell in ${foo:=$(bar)} expressions.  For\nexample, after running\n\n\temulate sh\n\tfn () {\n\t\tvar='one two'\n\t\tprintf '%s\\n' $var\n\t}\n\tx=$(fn)\n\t: ${y=$(fn)}\n\nprinting \"$x\" results in two lines as expected, but printing \"$y\" results\nin a single line because $var is expanded as a single word when evaluating\nfn to compute y.\n\nSo avoid the construct, and use an explicit 'test -n \"$foo\" || foo=$(bar)'\ninstead.  This fixes a bug tht caused all commands to be treated as\nporcelain and show up in \"git <TAB><TAB>\" completion, because the list of\nall commands was treated as a single word in __git_list_porcelain_commands\nand did not match any of the patterns that would usually cause plumbing to\nbe excluded.\n\n[jn: clarified commit message, indentation style fix]\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n contrib/completion/git-completion.bash |    9 ++++++---\n 1 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 1496c6d..c636166 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -676,7 +676,8 @@ __git_merge_strategies=\n # is needed.\n __git_compute_merge_strategies ()\n {\n-\t: ${__git_merge_strategies:=$(__git_list_merge_strategies)}\n+\ttest -n \"$__git_merge_strategies\" ||\n+\t__git_merge_strategies=$(__git_list_merge_strategies)\n }\n \n __git_complete_revlist_file ()\n@@ -854,7 +855,8 @@ __git_list_all_commands ()\n __git_all_commands=\n __git_compute_all_commands ()\n {\n-\t: ${__git_all_commands:=$(__git_list_all_commands)}\n+\ttest -n \"$__git_all_commands\" ||\n+\t__git_all_commands=$(__git_list_all_commands)\n }\n \n __git_list_porcelain_commands ()\n@@ -947,7 +949,8 @@ __git_porcelain_commands=\n __git_compute_porcelain_commands ()\n {\n \t__git_compute_all_commands\n-\t: ${__git_porcelain_commands:=$(__git_list_porcelain_commands)}\n+\ttest -n \"$__git_porcelain_commands\" ||\n+\t__git_porcelain_commands=$(__git_list_porcelain_commands)\n }\n \n __git_pretty_aliases ()\n-- \n1.7.9.172.ge26ae\n"},{"id":"183705","messageId":"CAMP44s0Fq_BGwcmDM5E1kWNiyoJw6e6Hr=8XaNF6tmQAcdnUmw@mail.gmail.com","threadId":"29510","inReplyTo":"7vhaz8vkhd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-03T10:38:55Z","receivedAt":"2012-02-03T10:38:55Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Feb 3, 2012 at 2:17 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> On Thu, Feb 2, 2012 at 9:27 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Jonathan Nieder <jrnieder@gmail.com> writes:\n>>>> However, clearly I did not say it clearly enough. :) I guess it's\n>>>> better to take a cue from storytellers and show rather than tell.\n>>>\n>>> Very big thanks for this ;-)\n>>\n>> Not a single comment regarding what I said?\n>\n> What entitles you to force me to refraining from commenting at all until I\n> read everything in my mailbox and after waiting for a while to make sure\n> there is no more to come to the thread?\n\nFair enough. Just wondering.\n\n> In any case, \"be nicer with zsh\" conveys no more meaningful information\n> than \"this is some patch about zsh\".\n\nAnd that already tells you a lot more than other alternatives.\n\n> Let's try to avoid warm and fuzzy\n> words that imply \"goodness\", e.g. \"improve\" and \"be nicer with\" because\n> nobody sends a patch to purposefully make Git worse and expects it to be\n> applied.\n\nTrue. Which why I listened to the suggestion from Thomas Rast and\ndidn't use that, but \"completion: work around zsh option propagation\nbug\" instead.\n\n> I found Jonathan's alternative \"avoid default value assignment on : true\n> command\" at least a bit better for the purpose of jogging the short-term\n> memory in the \"'git shortlog v1.7.9.. contrib/completion/' tells us that\n> we have applied several patches, and I remember that : ${var=word} one!\"\n> sense.  It is not super-useful for the longer term, though.\n>\n> Here is what I ended up in preparation for queuing the series.  I still\n> haven't seen any version of 4/4, but please check $gmane/189683 and see if\n> that matches what you intended.  Also I am assuming $gmane/189606 relayed\n> by Jonathan is a squash between your 2 and 3 (which didn't reach me), so\n> please advise if that does not match what you want to have.\n\nThis is getting ridiculous, now I sent the patches directly to you, is\nyour pobox.com server also silently dropping them for no reason? I\nthink this is totally counter-productive. I haven't received any reply\nfrom the vger postmaster, but I guess you should be able to find out\nwhy your host is dropping mails. Am I the only one that has such\nissues?\n\nAnyway. I have uploaded all the mails to here:\n\nhttp://people.freedesktop.org/~felipec/git-patches/\n\nAs for $gmane/189683, the changes seem to be correct, but I still\nprefer my commit message[1]--which I have written and rewritten many\ntimes now to improve it.\n\nRegarding $gmane/189606, I still prefer my commit message[2], because\nit starts with the *purpose* of the patch. As for the changes, they\nare correct, and I don't mind squashing them, but they are *two*\nlogically independent changes; imagine in the future somebody\nwants/need to re-enable __git_shopt, well, all they have to do is\nrevert the second patch. But that's up to you.\n\nCheers.\n\n[1] http://people.freedesktop.org/~felipec/git-patches/4\n[2] http://people.freedesktop.org/~felipec/git-patches/2\n\n-- \nFelipe Contreras\n"},{"id":"183757","messageId":"7vehuboe5g.fsf@alter.siamese.dyndns.org","threadId":"29510","inReplyTo":"CAMP44s0Fq_BGwcmDM5E1kWNiyoJw6e6Hr=8XaNF6tmQAcdnUmw@mail.gmail.com","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-03T20:28:11Z","receivedAt":"2012-02-03T20:28:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> On Fri, Feb 3, 2012 at 2:17 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> ...\n>> Here is what I ended up in preparation for queuing the series. I still\n>> haven't seen any version of 4/4, but please check $gmane/189683 and see if\n>> that matches what you intended. Also I am assuming $gmane/189606 relayed\n>> by Jonathan is a squash between your 2 and 3 (which didn't reach me), so\n>> please advise if that does not match what you want to have.\n>\n> This is getting ridiculous, now I sent the patches directly to you, is\n> your pobox.com server also silently dropping them for no reason?\n\nDo not blame pobox.com; they have nothing to do with the corruption of\nyour headers.\n\nThe volume of messages on the git mailing list I read on gmane's nntp\ninterface is much larger than that of my personal git-mail mailbox.\nPatches and ideas in their early rounds do not come to my personal\ngit-mail mailbox (because I asked people not to cc: me such messages\nunless I am an area expert, and they have been good to me), but I try to\npoint people not to go in a wrong direction as early as possible to avoid\nwasting time for contributors and reviewers when I can.\n\nSo I almost always am on the mailing list \"newsgroup\" when dealing with\ngit related mail traffic (which is not 100% of my git time to begin with),\nand only after I ran out of messages to process I check my personal\ngit-mail mailbox \"newsgroup\". I usually only find \"help me\" messages and\nquestions that are addressed directly to me in private, and my standard\nresponse to them is to ask the sender to conduct the business in public on\nthe list instead. In other words, my personal git-mail mailbox \"newsgroup\"\nis primariliy a back-up mailbox for my purpose.  I do not even run\nfetchmail that often to poll it, because the latency for it is large due\nto the way I work (described above).\n\nYou just caught me at the wrong moment when there were much more important\nmessages on the list (more refers to the volume, not all of them are more\nimportant) and I was working on them (not limited to your issue) from top\nto bottom in the mailing list \"newsgroup\".  I however wanted to get the\nzsh issue resolved sooner, and because you seemed to have been having so\nmuch trouble with your MUA (I only so 0/4 even for v4), I tried to help\nout by sending what I thought is already good, hoping that a message that\nonly has to say \"that looks good, thanks\" would be easier to make it to\nthe list.\n\nAll that happened before I got to the back-up git mailbox where you sent\nthe rest of your v4.\n\nPeople say \"Oops, our mails crossed.\" and go on without making too much\nfuss about it.  E-mail communications are asynchronous.  Get used to it.\n\nI think your mail breakage, from looking at your mail header, is this:\n\n  From: Felipe Contreras <felipe.contreras@gmail.com>\n  To: git@vger.kernel.org\n  Cc: Junio C Hamano <gitster@pobox.com>, SZEDER Gábor <szeder@ira.uka.de>, Jonathan Nieder <jrnieder@gmail.com>, Thomas Rast <trast@inf.ethz.ch>, Felipe Contreras <felipe.contreras@gmail.com>, \"Shawn O. Pearce\" <spearce@spearce.org>>\n  Subject: [PATCH v4 1/4] completion: work around zsh option propagation bug\n\nNotice the excess '>' after the last address on Cc:?\n\nIt's not like this is your first serious submission to the list, so it is\ncurious why only this time you have been having so much trouble. Perhaps\nyou have changed your mail set-up lately?\n"},{"id":"183829","messageId":"CAMP44s2QdJ4+qgg4fF5-DOWHx3Btd0pTivTT9s_E=qqxg16YLQ@mail.gmail.com","threadId":"29510","inReplyTo":"7vehuboe5g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-04T15:46:05Z","receivedAt":"2012-02-04T15:46:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Feb 3, 2012 at 10:28 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> On Fri, Feb 3, 2012 at 2:17 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> ...\n>>> Here is what I ended up in preparation for queuing the series. I still\n>>> haven't seen any version of 4/4, but please check $gmane/189683 and see if\n>>> that matches what you intended. Also I am assuming $gmane/189606 relayed\n>>> by Jonathan is a squash between your 2 and 3 (which didn't reach me), so\n>>> please advise if that does not match what you want to have.\n>>\n>> This is getting ridiculous, now I sent the patches directly to you, is\n>> your pobox.com server also silently dropping them for no reason?\n>\n> Do not blame pobox.com; they have nothing to do with the corruption of\n> your headers.\n\nNo, but they have everything to do with *silently* dropping it. Why\ncouldn't they _at least_ return an error saying that the headers are\nwrong? Note that other servers didn't even complain, they processed\nthe mail happily.\n\nIn any case, the one to blame for the header corruption is git:\n\n% git blame -e -L 947,+7 contrib/completion/git-completion.bash v1.7.9\neaa4e6ee (<jrnieder@gmail.com>   2009-11-17 18:49:10 -0600 947)\n__git_compute_porcelain_commands ()\neaa4e6ee (<jrnieder@gmail.com>   2009-11-17 18:49:10 -0600 948) {\neaa4e6ee (<jrnieder@gmail.com>   2009-11-17 18:49:10 -0600 949)\n __git_compute_all_commands\neaa4e6ee (<jrnieder@gmail.com>   2009-11-17 18:49:10 -0600 950)\n : ${__git_porcelain_commands:=$(__git_list_porcelain_commands)}\neaa4e6ee (<jrnieder@gmail.com>   2009-11-17 18:49:10 -0600 951) }\nf2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)\nc3898111 (<szeder@ira.uka.de>    2010-10-11 00:06:22 +0200 953)\n__git_pretty_aliases ()\n\nNotice the mail is wrong.\n\nAnd then, 'git send-email' is happy to send such headers. I sent a\npatch to the list to improve that situation, but looks like\nsanitize_address() could be improved a lot.\n\nYou can blame it on 'git send-email', or 'git blame', or my cccmd\nscript[1], but as I discussed before, this is *precisely* the reason\nwhy it would be nice to have an official cccmd script.\n\n> You just caught me at the wrong moment when there were much more important\n> messages on the list (more refers to the volume, not all of them are more\n> important) and I was working on them (not limited to your issue) from top\n> to bottom in the mailing list \"newsgroup\".  I however wanted to get the\n> zsh issue resolved sooner, and because you seemed to have been having so\n> much trouble with your MUA (I only so 0/4 even for v4), I tried to help\n> out by sending what I thought is already good, hoping that a message that\n> only has to say \"that looks good, thanks\" would be easier to make it to\n> the list.\n\nI was not nor am I blaming you, I am just saying the situation is ridiculous.\n\nAnd my MUA is 'git send-email'.\n\n> People say \"Oops, our mails crossed.\" and go on without making too much\n> fuss about it.  E-mail communications are asynchronous.  Get used to it.\n\nThat's not the problem, but problem is these sanctimonious mail\nservers that drop mails without warning. And sure, also the code that\nsends malformed mail.\n\nIn any case, fixing 'git blame', and/or 'git send-email', and/or have\nan official cccmd script, would solve the problem.\n\n> I think your mail breakage, from looking at your mail header, is this:\n>\n>  From: Felipe Contreras <felipe.contreras@gmail.com>\n>  To: git@vger.kernel.org\n>  Cc: Junio C Hamano <gitster@pobox.com>, SZEDER Gábor <szeder@ira.uka.de>, Jonathan Nieder <jrnieder@gmail.com>, Thomas Rast <trast@inf.ethz.ch>, Felipe Contreras <felipe.contreras@gmail.com>, \"Shawn O. Pearce\" <spearce@spearce.org>>\n>  Subject: [PATCH v4 1/4] completion: work around zsh option propagation bug\n>\n> Notice the excess '>' after the last address on Cc:?\n\nThanks for pointing that out :)\n\n> It's not like this is your first serious submission to the list, so it is\n> curious why only this time you have been having so much trouble. Perhaps\n> you have changed your mail set-up lately?\n\nYes, people asked me to CC more relevant people, so I enabled by\ncc-cmd script that I sent to the list long time ago, but didn't\nreceive any more replies, and I also asked if it wouldn't make sense\nto have an official one [1] -- no replies.\n\nCheers.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/189360\n\n-- \nFelipe Contreras\n"},{"id":"183848","messageId":"20120204182611.GA31091@sigill.intra.peff.net","threadId":"29510","inReplyTo":"CAMP44s2QdJ4+qgg4fF5-DOWHx3Btd0pTivTT9s_E=qqxg16YLQ@mail.gmail.com","subject":"[bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-04T18:26:11Z","receivedAt":"2012-02-04T18:26:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 04, 2012 at 05:46:05PM +0200, Felipe Contreras wrote:\n\n> In any case, the one to blame for the header corruption is git:\n> [...]\n> f2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)\n> \n> Notice the mail is wrong.\n\nUgh. The fault lies in this code:\n\n  $ sed -n 1405,1414p builtin/blame.c \n          if (map_user(&mailmap, mail+1, mail_len-1, person, tmp-person-1)) {\n                  /* Add a trailing '>' to email, since map_user returns plain emails\n                     Note: It already has '<', since we replace from mail+1 */\n                  mailpos = memchr(mail, '\\0', mail_len);\n                  if (mailpos && mailpos-mail < mail_len - 1) {\n                          *mailpos = '>';\n                          *(mailpos+1) = '\\0';\n                  }\n          }\n  }\n\nBut that comment is wrong. If there's no email mapping needed, map_user\nwill leave the \"mail\" buffer intact, in which case it will have the\ntrailing \">\" (because we feed the address with enclosing angle\nbrackets).  So while map_user tries to accept either \"foo@example.com\\0\"\nand \"foo@example.com>\", it is up to the contents of the mailmap whether\nyou get back something with the closing angle bracket or not. Which is a\npretty error-prone interface.\n\nYou can fix it with this:\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 5a67c20..9b886fa 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1406,7 +1406,8 @@ static void get_ac_line(const char *inbuf, const char *what,\n \t\t/* Add a trailing '>' to email, since map_user returns plain emails\n \t\t   Note: It already has '<', since we replace from mail+1 */\n \t\tmailpos = memchr(mail, '\\0', mail_len);\n-\t\tif (mailpos && mailpos-mail < mail_len - 1) {\n+\t\tif (mailpos && mailpos-mail < mail_len - 1 &&\n+\t\t    mailpos > mail && *(mailpos-1) != '>') {\n \t\t\t*mailpos = '>';\n \t\t\t*(mailpos+1) = '\\0';\n \t\t}\n\nbut it feels like the fix should go into map_user.  I tried a few things,\nlike \"git log -1 --format=%aE\", and couldn't find other code paths with\nthis problem. So presumably they are all feeding email addresses without\nthe closing \">\" (so one option is to just say \"map_user needs to get\nNUL-terminated strings).\n\n-Peff\n"},{"id":"183851","messageId":"CAMP44s2r-fcnfpdT4u5U7TwF1z6Abp+J1U7oqfsSrYMuD6weOQ@mail.gmail.com","threadId":"29510","inReplyTo":"20120204182611.GA31091@sigill.intra.peff.net","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-04T19:30:42Z","receivedAt":"2012-02-04T19:30:42Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 4, 2012 at 8:26 PM, Jeff King <peff@peff.net> wrote:\n> On Sat, Feb 04, 2012 at 05:46:05PM +0200, Felipe Contreras wrote:\n>\n>> In any case, the one to blame for the header corruption is git:\n>> [...]\n>> f2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)\n>>\n>> Notice the mail is wrong.\n>\n> Ugh. The fault lies in this code:\n\nYes, I found that as well.\n\n> But that comment is wrong. If there's no email mapping needed, map_user\n> will leave the \"mail\" buffer intact, in which case it will have the\n> trailing \">\" (because we feed the address with enclosing angle\n> brackets).  So while map_user tries to accept either \"foo@example.com\\0\"\n> and \"foo@example.com>\", it is up to the contents of the mailmap whether\n> you get back something with the closing angle bracket or not. Which is a\n> pretty error-prone interface.\n>\n> You can fix it with this:\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 5a67c20..9b886fa 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1406,7 +1406,8 @@ static void get_ac_line(const char *inbuf, const char *what,\n>                /* Add a trailing '>' to email, since map_user returns plain emails\n>                   Note: It already has '<', since we replace from mail+1 */\n>                mailpos = memchr(mail, '\\0', mail_len);\n> -               if (mailpos && mailpos-mail < mail_len - 1) {\n> +               if (mailpos && mailpos-mail < mail_len - 1 &&\n> +                   mailpos > mail && *(mailpos-1) != '>') {\n>                        *mailpos = '>';\n>                        *(mailpos+1) = '\\0';\n>                }\n\nYes, I already did this. I'm writing tests for this right now, but I\nthink I found yet another bug...\n\n> but it feels like the fix should go into map_user.  I tried a few things,\n> like \"git log -1 --format=%aE\", and couldn't find other code paths with\n> this problem. So presumably they are all feeding email addresses without\n> the closing \">\" (so one option is to just say \"map_user needs to get\n> NUL-terminated strings).\n\nPerhaps, but I though the idea was to make it efficient. I think the\nabove fix should be ok.\n\nWe should have tests for this though, to make sure it doesn't get\nbroken again. I'm on that.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"183876","messageId":"20120204232015.GB1170@sigill.intra.peff.net","threadId":"29510","inReplyTo":"CAMP44s2r-fcnfpdT4u5U7TwF1z6Abp+J1U7oqfsSrYMuD6weOQ@mail.gmail.com","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-04T23:20:15Z","receivedAt":"2012-02-04T23:20:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 04, 2012 at 09:30:42PM +0200, Felipe Contreras wrote:\n\n> > but it feels like the fix should go into map_user.  I tried a few things,\n> > like \"git log -1 --format=%aE\", and couldn't find other code paths with\n> > this problem. So presumably they are all feeding email addresses without\n> > the closing \">\" (so one option is to just say \"map_user needs to get\n> > NUL-terminated strings).\n> \n> Perhaps, but I though the idea was to make it efficient. I think the\n> above fix should be ok.\n\nBecause of the calling convention of map_user, the buffer with the input\nmust also be writable (since it holds the result). So there should be no\nloss of efficiency to convert the \">\" into a \"\\0\" (and in fact, the\nsimplest fix is probably to just have map_user \"tie off\" any \">\" it\ndetects).\n\n> We should have tests for this though, to make sure it doesn't get\n> broken again. I'm on that.\n\nDefinitely. Thanks for working on it.\n\n-Peff\n"},{"id":"183908","messageId":"7v39aphw85.fsf@alter.siamese.dyndns.org","threadId":"29510","inReplyTo":"20120204182611.GA31091@sigill.intra.peff.net","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-05T20:16:26Z","receivedAt":"2012-02-05T20:16:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Feb 04, 2012 at 05:46:05PM +0200, Felipe Contreras wrote:\n>\n>> In any case, the one to blame for the header corruption is git:\n>> [...]\n>> f2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)\n>> \n>> Notice the mail is wrong.\n> ...\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 5a67c20..9b886fa 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1406,7 +1406,8 @@ static void get_ac_line(const char *inbuf, const char *what,\n>  \t\t/* Add a trailing '>' to email, since map_user returns plain emails\n>  \t\t   Note: It already has '<', since we replace from mail+1 */\n>  \t\tmailpos = memchr(mail, '\\0', mail_len);\n> -\t\tif (mailpos && mailpos-mail < mail_len - 1) {\n> +\t\tif (mailpos && mailpos-mail < mail_len - 1 &&\n> +\t\t    mailpos > mail && *(mailpos-1) != '>') {\n>  \t\t\t*mailpos = '>';\n>  \t\t\t*(mailpos+1) = '\\0';\n>  \t\t}\n>\n> but it feels like the fix should go into map_user.\n\nThanks.\n\nThe map_user() API takes an email address that is terminated by either NUL\nor '>' to allow the caller to learn the corresponding up-to-date email\naddress that is NUL terminated, while indicating with its return value\nthat if the caller even needs to replace what it already has.  But the\nfunction does not properly terminate email when it only touched the name\npart. And I think that is the real bug.\n\nSo I agree that the real fix should go to map_user() so that when it says\n\"I've updated something, so pick up the updated result from the i/o\narguments you gave me, i.e. email and name\", it makes sure what it claims\nto be an e-mail address does not have the extra '>' in it.\n\nWorking around the current behaviour by forcing all callers that pass '>'\nterminated e-mail address to have the code like the above quoted patch \ndoes not feel right.\n"},{"id":"183914","messageId":"7vipjlgg4j.fsf@alter.siamese.dyndns.org","threadId":"29510","inReplyTo":"CAMP44s2QdJ4+qgg4fF5-DOWHx3Btd0pTivTT9s_E=qqxg16YLQ@mail.gmail.com","subject":"Re: [PATCH v3 1/4] completion: be nicer with zsh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-05T20:49:32Z","receivedAt":"2012-02-05T20:49:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>> Do not blame pobox.com; they have nothing to do with the corruption of\n>> your headers.\n>\n> No, but they have everything to do with *silently* dropping it. Why\n> couldn't they _at least_ return an error saying that the headers are\n> wrong? Note that other servers didn't even complain, they processed\n> the mail happily.\n\nAgain, please do not blame pobox.com; they fall into your \"other servers\"\ncategory.  You are probably talking about vger.kernel.org that is extra\npicky and wants to avoid wasting their outgoing bandwidth because they get\nso much spams.\n\n> % git blame -e -L 947,+7 contrib/completion/git-completion.bash v1.7.9\n> ...\n> f2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)\n\nI am glad to see that something useful came out from your digging, and a\nfix is being worked on it, while I was away from my machines.  Thanks for\ngetting the ball rolling.\n"},{"id":"183918","messageId":"CAMP44s0xmvjxsE6AYrA5qTZuDfWq8vPDPOo69hiRS+xWbsbS7g@mail.gmail.com","threadId":"29510","inReplyTo":"20120204232015.GB1170@sigill.intra.peff.net","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-05T21:11:20Z","receivedAt":"2012-02-05T21:11:20Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Feb 5, 2012 at 1:20 AM, Jeff King <peff@peff.net> wrote:\n> On Sat, Feb 04, 2012 at 09:30:42PM +0200, Felipe Contreras wrote:\n>\n>> > but it feels like the fix should go into map_user.  I tried a few things,\n>> > like \"git log -1 --format=%aE\", and couldn't find other code paths with\n>> > this problem. So presumably they are all feeding email addresses without\n>> > the closing \">\" (so one option is to just say \"map_user needs to get\n>> > NUL-terminated strings).\n>>\n>> Perhaps, but I though the idea was to make it efficient. I think the\n>> above fix should be ok.\n>\n> Because of the calling convention of map_user, the buffer with the input\n> must also be writable (since it holds the result). So there should be no\n> loss of efficiency to convert the \">\" into a \"\\0\" (and in fact, the\n> simplest fix is probably to just have map_user \"tie off\" any \">\" it\n> detects).\n\nYes, but then the caller (git blame) would need to _always_ do that\nconversion before (\">\" -> \"\\0\"), and after (\"\\0\" -> \">\"), as opposed\nto now, that it does the conversion only when map_user succeeds (or\nchecks if it has to do it).\n\n-- \nFelipe Contreras\n"},{"id":"183925","messageId":"7vipjlezas.fsf@alter.siamese.dyndns.org","threadId":"29510","inReplyTo":"7v39aphw85.fsf@alter.siamese.dyndns.org","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-05T21:38:19Z","receivedAt":"2012-02-05T21:38:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n> ...\n>> but it feels like the fix should go into map_user.\n>\n> Thanks.\n>\n> The map_user() API takes an email address that is terminated by either NUL\n> or '>' to allow the caller to learn the corresponding up-to-date email\n> address that is NUL terminated, while indicating with its return value\n> that if the caller even needs to replace what it already has.  But the\n> function does not properly terminate email when it only touched the name\n> part. And I think that is the real bug.\n\nAnd the gist of the patch to fix the bug would look like this two liner.\nIn the real fix, \"p\" should be renamed to \"end_of_email\" or something\ndescriptive like that.\n\nI only made sure that this fixes the original case of the email address of\nShawn reported by Felipe, but other than that like everything else I send\nhere with \"... would look like this\", not tested beyond \"it compiles\".\n\nBut conceptually it looks correct (famous last words ;-).\n\nFelipe, does it pass your test cases?\n\n\n mailmap.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex 8c3196c..ce805fa 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -236,6 +236,8 @@ int map_user(struct string_list *map,\n \t\t}\n \t\tif (maxlen_email && mi->email)\n \t\t\tstrlcpy(email, mi->email, maxlen_email);\n+\t\telse\n+\t\t\t*p = '\\0';\n \t\tif (maxlen_name && mi->name)\n \t\t\tstrlcpy(name, mi->name, maxlen_name);\n \t\tdebug_mm(\"map_user:  to '%s' <%s>\\n\", name, mi->email ? mi->email : \"\");\n"},{"id":"183942","messageId":"20120205234750.GA28735@sigill.intra.peff.net","threadId":"29510","inReplyTo":"7vipjlezas.fsf@alter.siamese.dyndns.org","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-05T23:47:50Z","receivedAt":"2012-02-05T23:47:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 05, 2012 at 01:38:19PM -0800, Junio C Hamano wrote:\n\n> > The map_user() API takes an email address that is terminated by either NUL\n> > or '>' to allow the caller to learn the corresponding up-to-date email\n> > address that is NUL terminated, while indicating with its return value\n> > that if the caller even needs to replace what it already has.  But the\n> > function does not properly terminate email when it only touched the name\n> > part. And I think that is the real bug.\n> \n> And the gist of the patch to fix the bug would look like this two liner.\n> In the real fix, \"p\" should be renamed to \"end_of_email\" or something\n> descriptive like that.\n\nExactly; this is much better.\n\nWe could also go as far as saying that map_user would _always_ terminate\nin this way (i.e., the caller gets a munged result, whether we found\nanything or not). Then internally, map_user could be simplified to stop\nworrying about making a temporary copy in mailbuf. And callers could\nsimply call map_user without worrying about branching on whether it\nfound anything or not.\n\nBut maybe it is not worth that level of refactoring. From my reading,\nyour patch fixes the problem just fine.\n\n-Peff\n"},{"id":"183943","messageId":"20120205235044.GB28735@sigill.intra.peff.net","threadId":"29510","inReplyTo":"CAMP44s0xmvjxsE6AYrA5qTZuDfWq8vPDPOo69hiRS+xWbsbS7g@mail.gmail.com","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-05T23:50:44Z","receivedAt":"2012-02-05T23:50:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 05, 2012 at 11:11:20PM +0200, Felipe Contreras wrote:\n\n> > Because of the calling convention of map_user, the buffer with the input\n> > must also be writable (since it holds the result). So there should be no\n> > loss of efficiency to convert the \">\" into a \"\\0\" (and in fact, the\n> > simplest fix is probably to just have map_user \"tie off\" any \">\" it\n> > detects).\n> \n> Yes, but then the caller (git blame) would need to _always_ do that\n> conversion before (\">\" -> \"\\0\"), and after (\"\\0\" -> \">\"), as opposed\n> to now, that it does the conversion only when map_user succeeds (or\n> checks if it has to do it).\n\nYes, I'm talking about changing the calling and return conventions of\nmap_user. I think the efficiency change is negligible, though, as we\nare talking about character assignments (and in fact, it would probably\nend up more efficient, as we could eliminate some copying inside\nmap_user). But Junio's patch is simple, and fixes the problem without\ncreating any complexity for the callers. So I think it's a good fix.\n\n-Peff\n"},{"id":"183946","messageId":"7vehu8dcc8.fsf@alter.siamese.dyndns.org","threadId":"29510","inReplyTo":"20120205234750.GA28735@sigill.intra.peff.net","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T00:39:35Z","receivedAt":"2012-02-06T00:39:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We could also go as far as saying that map_user would _always_ terminate\n> in this way (i.e., the caller gets a munged result, whether we found\n> anything or not). Then internally, map_user could be simplified to stop\n> worrying about making a temporary copy in mailbuf. And callers could\n> simply call map_user without worrying about branching on whether it\n> found anything or not.\n\nI thought about it, but such a change needs to audit all the call sites\nthat assumes the promise original map_user() used to make before it was\nbroken. If we return 0 to the caller, the caller does not have to worry\nabout map_user() munging the buffer it lent to it.\n\nIt might be a worthwhile thing to do. I dunno; I didn't look into it.\n"},{"id":"183956","messageId":"20120206030339.GA29123@sigill.intra.peff.net","threadId":"29510","inReplyTo":"7vehu8dcc8.fsf@alter.siamese.dyndns.org","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T03:03:39Z","receivedAt":"2012-02-06T03:03:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 05, 2012 at 04:39:35PM -0800, Junio C Hamano wrote:\n\n> > We could also go as far as saying that map_user would _always_ terminate\n> > in this way (i.e., the caller gets a munged result, whether we found\n> > anything or not). Then internally, map_user could be simplified to stop\n> > worrying about making a temporary copy in mailbuf. And callers could\n> > simply call map_user without worrying about branching on whether it\n> > found anything or not.\n> \n> I thought about it, but such a change needs to audit all the call sites\n> that assumes the promise original map_user() used to make before it was\n> broken. If we return 0 to the caller, the caller does not have to worry\n> about map_user() munging the buffer it lent to it.\n> \n> It might be a worthwhile thing to do. I dunno; I didn't look into it.\n\nUgh, yeah. I was thinking about how it would improve this call site, but\nI don't want to get into auditing the others. Let's drop it and go with\nyour patch.\n\n-Peff\n"},{"id":"183957","messageId":"7v39aobqmp.fsf@alter.siamese.dyndns.org","threadId":"29510","inReplyTo":"20120206030339.GA29123@sigill.intra.peff.net","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T03:13:50Z","receivedAt":"2012-02-06T03:13:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> It might be a worthwhile thing to do. I dunno; I didn't look into it.\n>\n> Ugh, yeah. I was thinking about how it would improve this call site, but\n> I don't want to get into auditing the others.\n\nThere aren't that many, though.  shortlog has one, pretty has another and\nthat is about it.\n\nBut both seems to care that map_user() is not a function that returns void,\nso...\n"},{"id":"183958","messageId":"7vy5sgaby1.fsf@alter.siamese.dyndns.org","threadId":"29510","inReplyTo":"20120206030339.GA29123@sigill.intra.peff.net","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T03:16:22Z","receivedAt":"2012-02-06T03:16:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Ugh, yeah. I was thinking about how it would improve this call site, but\n> I don't want to get into auditing the others. Let's drop it and go with\n> your patch.\n\nIn any case, here is what I queued for tonight.\n\n-- >8 --\nSubject: [PATCH] mailmap: do not leave '>' in the output when answering \"we did something\"\n\nThe callers of map_user() give email and name to it, and expect to get an\nup-to-date versions of email and/or name to be used in their output. The\nfunction rewrites the given buffers in place. To optimize the majority of\ncases, the function returns 0 when it did not do anything, and it returns\n1 when the caller should use the updated contents.\n\nThe 'email' input to the function is terminated by '>' or a NUL (whichever\ncomes first) for historical reasons, but when a rewrite happens, the value\nis replaced with the mailbox inside the <> pair.  However, it failed to\nmeet this expectation when it only rewrote the name part without rewriting\nthe email part, and the email in the input was terminated by '>'.\n\nThis causes an extra '>' to appear in the output of \"blame -e\", because the\ncaller does send in '>'-terminated email, and when the function returned 1\nto tell it that rewriting happened, it appends '>' that is necessary when\nthe email part was rewritten.\n\nThe patch looks bigger than it actually is, because this change makes a\nvariable that points at the end of the email part in the input 'p' live\nmuch longer than it used to, deserving a more descriptive name.\n\nNoticed and diagnosed by Felipe Contreras and Jeff King.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n mailmap.c |   18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex 8c3196c..47aa419 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -190,27 +190,27 @@ void clear_mailmap(struct string_list *map)\n int map_user(struct string_list *map,\n \t     char *email, int maxlen_email, char *name, int maxlen_name)\n {\n-\tchar *p;\n+\tchar *end_of_email;\n \tstruct string_list_item *item;\n \tstruct mailmap_entry *me;\n \tchar buf[1024], *mailbuf;\n \tint i;\n \n \t/* figure out space requirement for email */\n-\tp = strchr(email, '>');\n-\tif (!p) {\n+\tend_of_email = strchr(email, '>');\n+\tif (!end_of_email) {\n \t\t/* email passed in might not be wrapped in <>, but end with a \\0 */\n-\t\tp = memchr(email, '\\0', maxlen_email);\n-\t\tif (!p)\n+\t\tend_of_email = memchr(email, '\\0', maxlen_email);\n+\t\tif (!end_of_email)\n \t\t\treturn 0;\n \t}\n-\tif (p - email + 1 < sizeof(buf))\n+\tif (end_of_email - email + 1 < sizeof(buf))\n \t\tmailbuf = buf;\n \telse\n-\t\tmailbuf = xmalloc(p - email + 1);\n+\t\tmailbuf = xmalloc(end_of_email - email + 1);\n \n \t/* downcase the email address */\n-\tfor (i = 0; i < p - email; i++)\n+\tfor (i = 0; i < end_of_email - email; i++)\n \t\tmailbuf[i] = tolower(email[i]);\n \tmailbuf[i] = 0;\n \n@@ -236,6 +236,8 @@ int map_user(struct string_list *map,\n \t\t}\n \t\tif (maxlen_email && mi->email)\n \t\t\tstrlcpy(email, mi->email, maxlen_email);\n+\t\telse\n+\t\t\t*end_of_email = '\\0';\n \t\tif (maxlen_name && mi->name)\n \t\t\tstrlcpy(name, mi->name, maxlen_name);\n \t\tdebug_mm(\"map_user:  to '%s' <%s>\\n\", name, mi->email ? mi->email : \"\");\n-- \n1.7.9.204.gdf845\n"},{"id":"183959","messageId":"20120206040145.GA29214@sigill.intra.peff.net","threadId":"29510","inReplyTo":"7vy5sgaby1.fsf@alter.siamese.dyndns.org","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T04:01:45Z","receivedAt":"2012-02-06T04:01:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 05, 2012 at 07:16:22PM -0800, Junio C Hamano wrote:\n\n> In any case, here is what I queued for tonight.\n> \n> -- >8 --\n> Subject: [PATCH] mailmap: do not leave '>' in the output when answering \"we did something\"\n\nLooks good to me.\n\n> The callers of map_user() give email and name to it, and expect to get an\n> up-to-date versions of email and/or name to be used in their output. The\n\nMinor grammar error.\n\n-Peff\n"},{"id":"184023","messageId":"CAMP44s1RN+_UK9rAk_m9Z=YaJJtwHLyiCu2stMMDEWqZN9260g@mail.gmail.com","threadId":"29510","inReplyTo":"7vy5sgaby1.fsf@alter.siamese.dyndns.org","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-06T12:14:56Z","receivedAt":"2012-02-06T12:14:56Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Feb 6, 2012 at 5:16 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> Ugh, yeah. I was thinking about how it would improve this call site, but\n>> I don't want to get into auditing the others. Let's drop it and go with\n>> your patch.\n>\n> In any case, here is what I queued for tonight.\n>\n> -- >8 --\n> Subject: [PATCH] mailmap: do not leave '>' in the output when answering \"we did something\"\n\nThis subject doesn't explain the *purpose* of the patch: always return\na plain mail address from map_user()\n\nThat would be enough for most readers.\n\nI think the immediate problem should be here:\n\nCurrently 'git blame -e' would add an extra '>' if map_user() returns\ntrue, which would end up as '<foo@bar.com>>'. This is because\nmap_user() sometimes modifies, the mail string, but sometimes not. So\nlet's always modify it.\n\nAt this point a lot of readers can skip the rest of the explanation.\n\n> The callers of map_user() give email and name to it, and expect to get an\n> up-to-date versions of email and/or name to be used in their output. The\n> function rewrites the given buffers in place. To optimize the majority of\n> cases, the function returns 0 when it did not do anything, and it returns\n> 1 when the caller should use the updated contents.\n>\n> The 'email' input to the function is terminated by '>' or a NUL (whichever\n> comes first) for historical reasons, but when a rewrite happens, the value\n> is replaced with the mailbox inside the <> pair.  However, it failed to\n> meet this expectation when it only rewrote the name part without rewriting\n> the email part, and the email in the input was terminated by '>'.\n\nWith the above explanation I suggested, I think this can be summarized as:\n\nAs of right now most of the callers of map_user() give a plan address,\nand expect a plain address, however, 'git blame -e' passes along a\nmail address that ends with >, and uses the return value to determine\nif a transformation was made, and adds the missing '>'. This is\nbecause when map_user() does the transformation, it returns a plan\naddress.\n\nThis might have worked in previous versions of map_user(), but now not\nonly does it transforms mail addresses, but also the name (phrase). So\nnow callers can't use the return value to know if they need to add an\nextra '>', or not. So lets always remove the '>', so they can.\n\n> This causes an extra '>' to appear in the output of \"blame -e\", because the\n> caller does send in '>'-terminated email, and when the function returned 1\n> to tell it that rewriting happened, it appends '>' that is necessary when\n> the email part was rewritten.\n\nIt took too long to reach the problem. As a reader, that's the first\nthing I would expect.\n\n> The patch looks bigger than it actually is, because this change makes a\n> variable that points at the end of the email part in the input 'p' live\n> much longer than it used to, deserving a more descriptive name.\n\nThis is a *separate* logically independent patch. And you yourself\nmention, makes the review of the patch more difficult, as of right now\nit's difficult to spot the functional change, because it's mixed with\nnon-functional changes.\n\nI find it peculiar how patches in the Linux kernel are truly logically\nindependent, but Git has a tendency to squash many things: cleanups,\nfixes, documentation, tests, extra tests, remove unused code, etc.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"184067","messageId":"7vliof62ko.fsf@alter.siamese.dyndns.org","threadId":"29510","inReplyTo":"CAMP44s1RN+_UK9rAk_m9Z=YaJJtwHLyiCu2stMMDEWqZN9260g@mail.gmail.com","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T22:04:39Z","receivedAt":"2012-02-06T22:04:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> This subject doesn't explain the *purpose* of the patch: always return\n> a plain mail address from map_user()\n\nThat would be a much better subject.\n\n> I think the immediate problem should be here:\n>\n> Currently 'git blame -e' would add an extra '>' if map_user() returns\n> true, which would end up as '<foo@bar.com>>'. This is because\n> map_user() sometimes modifies, the mail string, but sometimes not. So\n> let's always modify it.\n\nThat is just a symptom.  People who reached this commit by digging the\nhistory of mailmap.c would need to see the *cause* of the symptom\ndescribed in the light of how the API is designed to be used.  In other\nwords, \"the code after the update has to be this way because these are the\ni/o constraints this API has\".  \"Otherwise you would see this breakage for\nexample\" is merely a supporting material.\n"},{"id":"184074","messageId":"CAMP44s3i3eE8mVGHBB5zUdcX95+KEz9xcC1B3B6qON1StD=FXQ@mail.gmail.com","threadId":"29510","inReplyTo":"7vliof62ko.fsf@alter.siamese.dyndns.org","subject":"Re: [bug] blame duplicates trailing \">\" in mailmapped emails","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-06T23:11:01Z","receivedAt":"2012-02-06T23:11:01Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Feb 7, 2012 at 12:04 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> This subject doesn't explain the *purpose* of the patch: always return\n>> a plain mail address from map_user()\n>\n> That would be a much better subject.\n>\n>> I think the immediate problem should be here:\n>>\n>> Currently 'git blame -e' would add an extra '>' if map_user() returns\n>> true, which would end up as '<foo@bar.com>>'. This is because\n>> map_user() sometimes modifies, the mail string, but sometimes not. So\n>> let's always modify it.\n>\n> That is just a symptom.\n\nThat's a matter of semantics. Is the API broken? Then the API has a\nproblem, and you are fixing it, otherwise you are merely *improving*\nit.\n\n> People who reached this commit by digging the\n> history of mailmap.c would need to see the *cause* of the symptom\n> described in the light of how the API is designed to be used.\n\nWell, when you have a summary as \"always return a plain mail address\nfrom map_user()\", I think it's pretty clear what is the \"problem\"; the\nAPI does not always return a plain mail address. But any case, that is\ndescribed below, in my suggestion.\n\nBesides, even people digging the history would benefit from seeing the\n\"symptom\" first.\n\n> In other\n> words, \"the code after the update has to be this way because these are the\n> i/o constraints this API has\".  \"Otherwise you would see this breakage for\n> example\" is merely a supporting material.\n\nI disagree. The reader of the current commit message will keep in\nhis/her head the question \"Why?\", and it would be answered only at the\nvery end. Besides, what more succinct way to describe a problem with\nthe API than with an example (that happens to be real).\n\n-- \nFelipe Contreras\n"}]}