{"thread":{"id":"28020","subject":"[PATCH] rebase -i: fix has_action","startedAt":"2011-08-04T09:39:40Z","lastAt":"2011-08-05T17:01:03Z","messageCount":9,"participants":["Noe Rubinstein","Sverre Rabbelier","Junio C Hamano","Johannes Sixt","Andrew Wong","Steffen Daode Nurpmeso"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"172898","messageId":"1312450780-5021-1-git-send-email-nrubinstein@proformatique.com","threadId":"28020","inReplyTo":null,"subject":"[PATCH] rebase -i: fix has_action","fromName":"Noe Rubinstein","fromEmail":"nrubinstein@proformatique.com","sentAt":"2011-08-04T09:39:40Z","receivedAt":"2011-08-04T09:39:40Z","isPatch":true,"sender":{"key":"nrubinstein@proformatique.com","avatar":null},"body":"When doing git rebase -i, removing all actions in the todo list is\nsupposed to result in aborting the rebase. However, if there are spaces\nat the beginning of an empty line, has_action returns true and the\nrebase therefore removes all commits. This is probably not what a user\nleaving a space on an empty line expects.\n\nThis patch fixes the bug by changing has_action to grep any line\ncontaining anything that is not a space nor a #.\n\nSigned-off-by: Noe Rubinstein \n---\n git-rebase--interactive.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex c6ba7c1..bed79af 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -149,7 +149,7 @@ die_abort () {\n }\n \n has_action () {\n-\tsane_grep '^[^#]' \"$1\" >/dev/null\n+\tsane_grep '^[^#[:space:]]' \"$1\" >/dev/null\n }\n \n # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and\n-- \nNoé Rubinstein\nAvencall - XiVO IPBX OpenHardware\n10 bis, rue Lucien VOILIN - 92800 Puteaux\nTél. : +33 (0)1 41 38 99 60 ext 123\nFax. : +33 (0)1 41 38 99 70\n"},{"id":"172929","messageId":"CAGdFq_j2ZuHLHT-M_+-apv5fe8CUh9JLQ9bZvx+v=QTb8+K9Rw@mail.gmail.com","threadId":"28020","inReplyTo":"1312450780-5021-1-git-send-email-nrubinstein@proformatique.com","subject":"Re: [PATCH] rebase -i: fix has_action","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-08-04T12:15:35Z","receivedAt":"2011-08-04T12:15:35Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Thu, Aug 4, 2011 at 11:39, Noe Rubinstein\n wrote:\n> This patch fixes the bug by changing has_action to grep any line\n> containing anything that is not a space nor a #.\n\nProbably maint-worthy? Should this have a test too?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"172946","messageId":"7vliv93r9g.fsf@alter.siamese.dyndns.org","threadId":"28020","inReplyTo":"1312450780-5021-1-git-send-email-nrubinstein@proformatique.com","subject":"Re: [PATCH] rebase -i: fix has_action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-04T19:34:19Z","receivedAt":"2011-08-04T19:34:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Noe Rubinstein writes:\n\n> When doing git rebase -i, removing all actions in the todo list is\n> supposed to result in aborting the rebase.\n\nI thought it was meant to be more like \"removing all _lines_\", and the\ngrep was a half-assed attempt to ignore lines that are clearly comments.\nChecking the size of the insn sheet might be a better change in that\nsense, as that would not leave any ambiguity:\n\n\thas_action () {\n\t test -s \"$1\"\n\t}\n\n> This patch fixes the bug by changing has_action to grep any line\n> containing anything that is not a space nor a #.\n\nFirst of all, I do not think it is a \"fixes the bug\". I can buy \"makes\nthings safer by detecting user errors\", of course.\n\nMore importantly, I do not think you are grepping \"any line containing\nanything that is not a space nor a hash\". You are instead grepping lines\nthat do not begin with a hash or a whitespace, no?\n\n> has_action () {\n> -\tsane_grep '^[^#]' \"$1\" >/dev/null\n> +\tsane_grep '^[^#[:space:]]' \"$1\" >/dev/null\n> }\n\nWe tend to avoid [:character class:] to accomodate older implementations\nof grep.\n\nWe earlier asked \"do we have any line that begins with a character that is\nnot a hash '#'?\" but now we say \"do we have any line that begins with a\ncharacter that is not a hash nor a space?\".\n\nIf a user fat-fingers an unnecessary space into a blank line, that line\ncertainly will be excluded. But if the user fat-fingers ^X^I (or >> for vi\nusers), all lines begin with whitespace and they now get ignored?\n\nHow about removing the unnecessary negation from the logic and directly\nask what we really want to know?\n\nThat is, \"Do we have a line that is _not_ comment?\"\n\n\thas_action () {\n sane_grep -v -e '^#' -e '^[ ]*$' \"$1\" >/dev/null\n\t}\n\nHmm?\n"},{"id":"172973","messageId":"CAGdFq_j2wRw-gB109VypZkG1u=fm7yynkn2-Gu8AzNpVOrun8w@mail.gmail.com","threadId":"28020","inReplyTo":"7vliv93r9g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rebase -i: fix has_action","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-08-05T12:36:00Z","receivedAt":"2011-08-05T12:36:00Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Thu, Aug 4, 2011 at 21:34, Junio C Hamano wrote:\n>        has_action () {\n>          test -s \"$1\"\n>        }\n\n>        has_action () {\n>          sane_grep -v -e '^#' -e '^[   ]*$' \"$1\" >/dev/null\n>        }\n\nI think the former more correctly checks what the function name\nimplies, is there any downside to that which makes you suggest this\nsecond approach?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"172974","messageId":"4E3BE63E.8030509@viscovery.net","threadId":"28020","inReplyTo":"CAGdFq_j2wRw-gB109VypZkG1u=fm7yynkn2-Gu8AzNpVOrun8w@mail.gmail.com","subject":"Re: [PATCH] rebase -i: fix has_action","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-08-05T12:46:54Z","receivedAt":"2011-08-05T12:46:54Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 8/5/2011 14:36, schrieb Sverre Rabbelier:\n> On Thu, Aug 4, 2011 at 21:34, Junio C Hamano wrote:\n>> has_action () {\n>> test -s \"$1\"\n>> }\n> \n>> has_action () {\n>> sane_grep -v -e '^#' -e '^[ ]*$' \"$1\" >/dev/null\n>> }\n> \n> I think the former more correctly checks what the function name\n> implies, is there any downside to that which makes you suggest this\n> second approach?\n\nYes. There might be editors where it is difficult to edit a non-empty file\nso that it becomes empty. I recall this was a problem for me with vi\nbefore I got intimately acquainted with it.\n\n-- Hannes\n"},{"id":"172976","messageId":"4E3BFB86.4010408@sohovfx.com","threadId":"28020","inReplyTo":"7vliv93r9g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rebase -i: fix has_action","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2011-08-05T14:17:42Z","receivedAt":"2011-08-05T14:17:42Z","isPatch":true,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 08/04/2011 03:34 PM, Junio C Hamano wrote:\n> How about removing the unnecessary negation from the logic and directly\n> ask what we really want to know?\n>\n> That is, \"Do we have a line that is _not_ comment?\"\n>\n> \thas_action () {\n> sane_grep -v -e '^#' -e '^[ ]*$' \"$1\" >/dev/null\n> \t}\nHow about also including comments that begins with spaces? i.e.\n\n has_action () {\n sane_grep -v -e '^[ ]*#' -e '^[ ]*$' \"$1\" >/dev/null\n }\n\nAlso, is [ ] supposed to be a space and a hard tab? They just seem to\nbe three spaces in my email. We might need to watch out for the hard tab\ngetting expanded into spaces somewhere during the email process,\nespecially when applying the patch from email into code.\n\nAndrew\n"},{"id":"172977","messageId":"20110805144743.GA41203@sherwood.local","threadId":"28020","inReplyTo":"7vliv93r9g.fsf@alter.siamese.dyndns.org","subject":"What you can throw (on a Friday)","fromName":"Steffen Daode Nurpmeso","fromEmail":"sdaoden@googlemail.com","sentAt":"2011-08-05T14:47:43Z","receivedAt":"2011-08-05T14:47:43Z","isPatch":false,"sender":{"key":"sdaoden@googlemail.com","avatar":null},"body":"@ Junio C Hamano wrote (2011-08-04 21:34+0200):\n> Noe Rubinstein writes: [...]\n> If a user fat-fingers an unnecessary [...]\n\nHeh. Note that one of the first mails i've received from this\nlist was the one which transported the annoyed gasp from\nthe-one-who-was-born-right-next-door-to-where-the-elks-live.\nAbout scattered non-breaking spaces (U+A0).\n\nUnfortunately that patch series has been thrown away! And so\ni think you underestimate the problem. On my german keyboard,\nf.e., i need to press ALT for all of these: []|{} (ALT + [5-9]).\n!!! Well, there *are* days where my thumb is fast enough to leave\nALT before i hit SPC ... (But today it yet caused distress.)\n\nAfter the second scissor i'll append my current pre-commit (and\nthus pre-applypatch) (for nothing except to show that it's\na problem people really have to deal with). But i already thought\nabout resurrecting your patch-series and reduce it to only the\nwhitespace check. Because, being able to say\n\n exec git diff-index --check --cached $against --\n\ninstead would be much easier, because then you could simply state\nand request from contributors \"please adhere to the whitespace\npolicy of this project:\"\n\n whitespace = trailing-space,tabwidth=4,tab-in-indent,yell-on-nbsp\n\n> Hmm?\n\nWell and while fooling around and getting more familiar with\ngit(1) i stumbled over some things which might be caused by bad\ncontrol flow instead of being desired behaviour. After the first\nscissor there is a test shell script which reproduces them.\n\nThanks for git(1) beside that, plastic dishes also break ...\nNice weekend all of you.\n\n--Steffen\nCiao, sdaoden(*)(gmail.com)\nASCII ribbon campaign ( ) More nuclear fission plants\n against HTML e-mail X can serve more coloured\n and proprietary attachments / \\ and sounding animations\n\n-- >8 --\n#!/bin/sh\n\nerror() {\n\techo >&2 Error: $*\n}\n\nadd_file() {\n local f=$1\n echo $f > $f\n git add $f\n git commit -qm $f\n}\n\norigin() {\n rm -rf origin\n mkdir origin\n cd origin\n git init -q\n add_file eins\n add_file zwei\n add_file drei\n add_file vier\n add_file fuenf\n git checkout -qb devel\n add_file devel-one\n add_file devel-two\n git checkout -q master\n git tag -m vT1 vT1 HEAD\n cd ..\n}\n\nbadbad_tagopt() {\n echo\n echo\n echo '1. echo git fetch will --prune away branches if --tags is set'\n echo ' (even if done so through remote.XY.tagopt config).'\n echo ' But nice: it works well again if --all is also given.'\n echo\n\twork() {\n echo - Am using fetch -q --prune $1 $2\n\t\trm -rf tr1\n\t\tmkdir tr1\n\t\tcd tr1\n\t\tgit init -q\n\t\tgit remote add -t master -t devel -m master origin ../origin\n\n\t\tgit fetch -q --prune $1 $2\n\n\t\tgit branch -a | grep -F 'origin/master' || error no master branch\n\t\tgit branch -a | grep -F 'origin/devel' || error no devel branch\n\t\tcd ..\n\t\trm -rf tr1\n\t}\n\n\n\twork '' ''\n\twork '--tags' ''\n work '--tags' '--all'\n}\n\nlazy_ref() {\n echo\n echo\n echo 2. If you do drop/there is no remote.XY.fetch of a branch,\n echo ' but configure the remote.XY.push entry, then after'\n echo ' git push the local ref is not updated, even though'\n echo ' the push succeeded and correctly updated the target repo.'\n echo\n\twork() {\n echo - remote.origin.fetch will include devel branch: $#\n cp -R origin origin.save\n\t\trm -rf tr1\n\t\tmkdir tr1\n\t\tcd tr1\n\t\tgit init -q\n\t\tgit remote add -t master -m master -t devel origin ../origin\n git config --local remote.origin.push \\\n +refs/heads/master:refs/heads/master\n git config --local --add remote.origin.push \\\n +refs/heads/devel:refs/heads/devel\n\t\tgit fetch -q --prune\n git checkout -q master\n git checkout -q devel\n add_file test-repo-devel-branch-file\n\n test $# == 0 &&\n git config --local --replace-all remote.origin.fetch \\\n +refs/heads/master:refs/remotes/origin/master\n git push -q origin\n\n x=$(git show-ref --hash devel | sort -u | awk '{++l} END {print l}')\n test $x == 1 || error 'local-ref mismatch'\n\t\tcd ..\n\t\trm -rf tr1 origin\n mv origin.save origin\n\t}\n\n\twork YesPlease\n\twork\n}\n\ncd $TMPDIR\nmkdir workdir\ncd workdir\n\norigin\n\nbadbad_tagopt\nlazy_ref\n\ncd ..\nrm -rf workdir\nexit 0\n-- >8 --\n#!/bin/sh\n#@ git(1) pre-commit hook for dummies\n\n#if git rev-parse --verify HEAD >/dev/null 2>&1\n#then\n against=HEAD\n#else\n # Initial commit: diff against an empty tree object\n# against=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n#fi\n\n# Oh no, unfortunately not: exec git diff-index --check --cached $against --\ngit diff --cached $against | perl -e '\n # XXX 1st version, may not be able to swallow all possible diff output yet\n my ($estat, $l, $fname) = (0, undef, undef);\n\n for (;;) { last if stdin() =~ /^diff/o; }\n for (;;) { head(); hunk(); }\n\n sub stdin {\n $l = ;\n exit($estat) unless $l;\n chomp($l);\n return $l;\n }\n\n sub head {\n # Skip anything, including options and entire rename and delete diffs,\n # until we see the ---/+++ line pair\n for (;;) {\n last if $l =~ /^---/o;\n stdin();\n }\n\n stdin();\n die \"head, 1.: cannot parse diff!\" unless $l =~ /^\\+\\+\\+ /o;\n $fname = substr($l, 4);\n $fname = substr($fname, 2) if $fname =~ /^b\\//o;\n }\n\n sub hunk() {\n stdin();\n die \"hunk, 1.: cannot parse diff!\" unless $l =~ /^@@ /o;\nJHUNK:\n # regex shamelessly stolen from git(1), and modified\n $l =~ /^@@ -\\d+(?:,\\d+)? \\+(\\d+)(?:,\\d+)? @@/;\n my $lno = $1 - 1;\n\n for (;;) {\n stdin();\n return if $l =~ /^diff/o; # Different file?\n goto JHUNK if $l =~ /^@@ /o; # Same file, different hunk?\n next if $l =~ /^-/o; # Ignore removals\n\n ++$lno;\n next if $l =~ /^ /o;\n $l = substr($l, 1);\n\n if (index($l, \"\\xA0\") != -1) {\n $estat = 1;\n print \"$fname:$lno: non-breaking space (NBSP, U+A0).\\n\";\n }\n if ($l =~ /\\s+$/o) {\n $estat = 1;\n print \"$fname:$lno: trailing whitespace.\\n\";\n }\n if ($l =~ /^(\\s+)/o && $1 =~ /\\x09/o) {\n $estat = 1;\n print \"$fname:$lno: tabulator in indent.\\n\";\n }\n }\n }\n '\n"},{"id":"172983","messageId":"7v62mb4wwd.fsf@alter.siamese.dyndns.org","threadId":"28020","inReplyTo":"CAGdFq_j2wRw-gB109VypZkG1u=fm7yynkn2-Gu8AzNpVOrun8w@mail.gmail.com","subject":"Re: [PATCH] rebase -i: fix has_action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-05T16:59:30Z","receivedAt":"2011-08-05T16:59:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> Heya,\n>\n> On Thu, Aug 4, 2011 at 21:34, Junio C Hamano wrote:\n>> has_action () {\n>> test -s \"$1\"\n>> }\n>\n>> has_action () {\n>> sane_grep -v -e '^#' -e '^[ ]*$' \"$1\" >/dev/null\n>> }\n>\n> I think the former more correctly checks what the function name\n> implies, is there any downside to that which makes you suggest this\n> second approach?\n\nI vaguely recall the original reason we didn't do the most straightforward\nthing was something like what J6t said already.\n\nAs we are not interested in _adding_ new feature, I would say that,\nstrictly speaking, this *should* become a two-patch series whose first one\nuses\n\n\tsane_grep -v -e '^#' -e '^$' \"$1\" >/dev/null\n\nthat is, \"do we have anything aside from comments and blanks?\", which is\nthe original semantics, with Noe's \"safety\" change as the second patch in\nthe series that uses\n\n\tsane_grep -v -e '^#' -e '^[\t ]*$' \"$1\" >/dev/null\n\nto say \"let's count a line that solely consists of whitespaces also as a\nblank\".\n\nBut of course in practice it can and should be just a single patch that \nsquashes these two \"conceptually separate\" steps.\n"},{"id":"172984","messageId":"7v1uwz4wts.fsf@alter.siamese.dyndns.org","threadId":"28020","inReplyTo":"4E3BFB86.4010408@sohovfx.com","subject":"Re: [PATCH] rebase -i: fix has_action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-05T17:01:03Z","receivedAt":"2011-08-05T17:01:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Wong writes:\n\n> On 08/04/2011 03:34 PM, Junio C Hamano wrote:\n>> How about removing the unnecessary negation from the logic and directly\n>> ask what we really want to know?\n>>\n>> That is, \"Do we have a line that is _not_ comment?\"\n>>\n>> \thas_action () {\n>> sane_grep -v -e '^#' -e '^[ ]*$' \"$1\" >/dev/null\n>> \t}\n> How about also including comments that begins with spaces? i.e.\n\nNot interested.\n\nIt would be _clear_ if you inserted extra space before '#'; Noe's issue is\nthat it is not clear if you have extra space on a blank line, which I am a\nbit more sympathetic.\n"}]}