{"thread":{"id":"22600","subject":"[PATCH] blame: allow -L n,m to have an m bigger than the file's line count","startedAt":"2010-02-10T07:27:44Z","lastAt":"2010-02-12T00:25:24Z","messageCount":9,"participants":["Stephen Boyd","SZEDER Gábor","Jay Soffian","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"134131","messageId":"1265786864-5460-1-git-send-email-bebarino@gmail.com","threadId":"22600","inReplyTo":null,"subject":"[PATCH] blame: allow -L n,m to have an m bigger than the file's line count","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-02-10T07:27:44Z","receivedAt":"2010-02-10T07:27:44Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Sometimes I want to blame a file starting at some point and ending at\nthe end of the file. In my haste I'll write something like this:\n\n$ git blame -L5,2342343 -- builtin-blame.c\n\nand be greeted by a die message telling me that my end range is greater\nthan the number of lines in the file. Obviously I can do:\n\n$ git blame -L5, -- builtin-blame.c\n\nand get what I want but that isn't very discoverable. If the range is\ngreater than the number of lines just truncate the range to go up to\nthe end of the file.\n\nUpdate the docs to more accurately reflect the defaults for n and m too.\n\nSigned-off-by: Stephen Boyd \n---\n\nI realize this is late in the game for 1.7.0 so I'll resend if this\nisn't picked up.\n\n Documentation/blame-options.txt | 4 +++-\n builtin-blame.c | 4 +++-\n t/t8003-blame.sh | 4 ++--\n 3 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 4833cac..620660d 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -9,7 +9,7 @@\n --show-stats::\n \tInclude additional statistics at the end of blame output.\n \n--L ,::\n+-L [],[]::\n \tAnnotate only the given line range. and can take\n \tone of these forms:\n \n@@ -31,6 +31,8 @@ starting at the line given by .\n This is only valid for and will specify a number\n of lines before or after the line given by .\n +\n+Note: if is not given it defaults to 1 and if is not given it\n+defaults to the number of lines in the file.\n \n -l::\n \tShow long rev (Default: off).\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 10f7eac..77b7323 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1962,6 +1962,8 @@ static void prepare_blame_range(struct scoreboard *sb,\n \t\tterm = parse_loc(term + 1, sb, lno, *bottom + 1, top);\n \t\tif (*term)\n \t\t\tusage(blame_usage);\n+\t\tif (lno < *top)\n+\t\t\t*top = lno;\n \t}\n \tif (*term)\n \t\tusage(blame_usage);\n@@ -2238,7 +2240,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"contents\", &contents_from, \"file\", \"Use 's contents as the final image\"),\n \t\t{ OPTION_CALLBACK, 'C', NULL, &opt, \"score\", \"Find line copies within and across files\", PARSE_OPT_OPTARG, blame_copy_callback },\n \t\t{ OPTION_CALLBACK, 'M', NULL, &opt, \"score\", \"Find line movements within and across files\", PARSE_OPT_OPTARG, blame_move_callback },\n-\t\tOPT_CALLBACK('L', NULL, &bottomtop, \"n,m\", \"Process only line range n,m, counting from 1\", blame_bottomtop_callback),\n+\t\tOPT_CALLBACK('L', NULL, &bottomtop, \"[n],[m]\", \"Process only line range n,m, counting from 1\", blame_bottomtop_callback),\n \t\tOPT_END()\n \t};\n \ndiff --git a/t/t8003-blame.sh b/t/t8003-blame.sh\nindex 4a8db74..0ba150e 100755\n--- a/t/t8003-blame.sh\n+++ b/t/t8003-blame.sh\n@@ -161,8 +161,8 @@ test_expect_success 'blame -L with invalid start' '\n \ttest_must_fail git blame -L5 tres 2>&1 | grep \"has only 2 lines\"\n '\n \n-test_expect_success 'blame -L with invalid end' '\n-\tgit blame -L1,5 tres 2>&1 | grep \"has only 2 lines\"\n+test_expect_success 'blame -L with invalid end truncates automatically' '\n+\tgit blame -L1,5 tres\n '\n \n test_done\n-- \n1.7.0.rc2.13.g8b233\n"},{"id":"134142","messageId":"20100210124238.GA31978@neumann","threadId":"22600","inReplyTo":"1265786864-5460-1-git-send-email-bebarino@gmail.com","subject":"Re: [PATCH] blame: allow -L n,m to have an m bigger than the file's line count","fromName":"SZEDER Gábor","fromEmail":"szeder@fzi.de","sentAt":"2010-02-10T12:42:38Z","receivedAt":"2010-02-10T12:42:38Z","isPatch":true,"sender":{"key":"szeder@fzi.de","avatar":null},"body":"Hi Stephen,\n\n\nOn Tue, Feb 09, 2010 at 11:27:44PM -0800, Stephen Boyd wrote:\n> Sometimes I want to blame a file starting at some point and ending at\n> the end of the file. In my haste I'll write something like this:\n> \n> $ git blame -L5,2342343 -- builtin-blame.c\n> \n> and be greeted by a die message telling me that my end range is greater\n> than the number of lines in the file. Obviously I can do:\n> \n> $ git blame -L5, -- builtin-blame.c\n> \n> and get what I want but that isn't very discoverable. If the range is\n> greater than the number of lines just truncate the range to go up to\n> the end of the file.\n> \n> Update the docs to more accurately reflect the defaults for n and m too.\n> \n> Signed-off-by: Stephen Boyd \n> ---\n> \n> I realize this is late in the game for 1.7.0 so I'll resend if this\n> isn't picked up.\n> \n> Documentation/blame-options.txt | 4 +++-\n> builtin-blame.c | 4 +++-\n> t/t8003-blame.sh | 4 ++--\n> 3 files changed, 8 insertions(+), 4 deletions(-)\n> \n> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\n> index 4833cac..620660d 100644\n> --- a/Documentation/blame-options.txt\n> +++ b/Documentation/blame-options.txt\n> @@ -9,7 +9,7 @@\n> --show-stats::\n> \tInclude additional statistics at the end of blame output.\n> \n> --L ,::\n> +-L [],[]::\n> \tAnnotate only the given line range. and can take\n> \tone of these forms:\n> \n> @@ -31,6 +31,8 @@ starting at the line given by .\n> This is only valid for and will specify a number\n> of lines before or after the line given by .\n> +\n> +Note: if is not given it defaults to 1 and if is not given it\n> +defaults to the number of lines in the file.\n> \n> -l::\n> \tShow long rev (Default: off).\n\nI agree that its too late for the behavioral change, but IMHO the\ndocumentation update part can be considered as a bugfix, and as such\nit could perhaps be included in 1.7.0. (I never knew that or\n can be omitted... so thanks for the hint anyway)\n\n\nBest,\nGábor\n"},{"id":"134144","messageId":"76718491002100537h521fcc26gb267ed7cd2b8db6f@mail.gmail.com","threadId":"22600","inReplyTo":"1265786864-5460-1-git-send-email-bebarino@gmail.com","subject":"Re: [PATCH] blame: allow -L n,m to have an m bigger than the file's line count","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-02-10T13:37:36Z","receivedAt":"2010-02-10T13:37:36Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Feb 10, 2010 at 2:27 AM, Stephen Boyd wrote:\n> and get what I want but that isn't very discoverable. If the range is\n> greater than the number of lines just truncate the range to go up to\n> the end of the file.\n\nI agree this is the right thing to do. I'm working on a patch to\nsupport matching multiple times when given a regex range and made just\nthat change as well. :-)\n\nj.\n"},{"id":"134147","messageId":"4B72DDFE.7090400@gmail.com","threadId":"22600","inReplyTo":"76718491002100537h521fcc26gb267ed7cd2b8db6f@mail.gmail.com","subject":"Re: [PATCH] blame: allow -L n,m to have an m bigger than the file's line count","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-02-10T16:25:34Z","receivedAt":"2010-02-10T16:25:34Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On 02/10/2010 05:37 AM, Jay Soffian wrote:\n> I agree this is the right thing to do. I'm working on a patch to\n> support matching multiple times when given a regex range and made just\n> that change as well. :-)\n> \n\nGreat! I'll split the patch into a documentation patch (for 1.7.0) and a \nbehavioral patch (for post 1.7.0)? Or perhaps you can take care of the \nbehavioral change in your upcoming series?\n"},{"id":"134164","messageId":"7vwrykapfp.fsf@alter.siamese.dyndns.org","threadId":"22600","inReplyTo":"76718491002100537h521fcc26gb267ed7cd2b8db6f@mail.gmail.com","subject":"Re: [PATCH] blame: allow -L n,m to have an m bigger than the file's line count","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-10T18:58:02Z","receivedAt":"2010-02-10T18:58:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> On Wed, Feb 10, 2010 at 2:27 AM, Stephen Boyd wrote:\n>> and get what I want but that isn't very discoverable. If the range is\n>> greater than the number of lines just truncate the range to go up to\n>> the end of the file.\n>\n> I agree this is the right thing to do. I'm working on a patch to\n> support matching multiple times when given a regex range and made just\n> that change as well. :-)\n\nI would mildly suggest against going in that direction.\n\nThis is merely \"mildly\", because truncating 99999 in \"-L200,99999\" to the\nnumber of lines in the target blob would _not_ hurt. But it is an ugly\nhack. I would be Ok with coding that special case, but I do not want to\nsee it advertised, especially if we are making \"omission defaults to the\nend\" the documented way to explicitly say \"I don't care to count, just do\nit til the end\".\n\nWhile we are talking about touching the vicinity, I think we should\ntighten the -L s,e parsing rules a bit further.\n\nThere is an undocumented code that swaps start and end if the given end is\nsmaller than the start. This triggers even when \"-L280,300\" is mis-typed\nas \"-L280,30\". I was bitten by this more than once---when the input does\nnot make sense, we should actively error out, instead of doing a wrong\nthing. I suspect that I coded it that way _only_ to support this pattern:\n\n\t-L'/^#endif \\/\\* !WINDOWS \\/\\*/,-30'\n\ni.e. \"blame 30 lines before the '#endif' line\". But the code also\ninternally turns \"-L50,-20\" into \"-L 50,30\" and then swaps them to may\nmake it \"-L30,50\"; this was merely an unintended side effect.\n\nI do want to see -L'/regexp/,-offset' keep working, I do not mind if we\nkeep taking \"-L50,-20\" as an unintuitive way to spell \"-L30,50\", or reject\n\"-L50,-20\" as a nonsense. But I do want to see us reject \"-L280,30\" as a\ntypo.\n\nAs to use of more than one -L option, especially when the start (or end\nfor that matter) is specified with an regexp, I am of two minds.\n\nWhen annotating the body of two functions, frotz and nitfol, I might\nexpect this to work:\n\n -L'/^int frotz(/,/^}/' -L'/^int nitfol(/,/^}/'\n\nregardless of the order these functions appear in the blob (i.e. nitfol\nmay be defined first). This requires that parsing of \"regexp\" needs to\nreset to the beginning of blob for each -L option (iow, multiple -L are\nparsed independently from each other).\n\nBut at the same time, if I am actually looking at the blob contents in one\nterminal while spelling the blame command line in another, it would be\nnicer if the multiple -L looked for patterns incrementally. I may\nappreciate if I can write the above command line as:\n\n -L'/^int frotz(/,/^}/' -L'/nitfol/,/^}/'\n\nwhen I can see in my \"less\" of the blob contents in the other terminal\nthat the first line that has string \"nitfol\" after the end of the\ndefinition of \"frotz\" is the beginning of function \"nitfol\".\n\nAnother thing we _might_ want to consider doing is something like:\n\n -L'*/^#ifdef WINDOWS/,/^#endif \\/\\* WINDOWS \\/\\*/'\n\nto tell it \"I don't care to count how many WINDOWS ifdef blocks there are;\ngrab all of them\".\n\nRegardless of how parsing of multiple -L goes, you need to be careful to\nsort the resulting line ranges and possibly coalesce them when there are\noverlaps (e.g. \"-L12,+7 -L10,+5\" should become \"-L10,17\"). And be careful\nabout refcounting of origin. You'll be making multiple blame_ent and\nqueuing them to the scoreboard when starting, all pointing at the blame\ntarget blob; the origin blob needs to start with the right number of\nreferences to keep origin_decref() discarding it.\n"},{"id":"134170","messageId":"76718491002101139m4061fb90qcee7d34fca9f242f@mail.gmail.com","threadId":"22600","inReplyTo":"7vwrykapfp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame: allow -L n,m to have an m bigger than the file's line count","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-02-10T19:39:45Z","receivedAt":"2010-02-10T19:39:45Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Feb 10, 2010 at 1:58 PM, Junio C Hamano wrote:\n> While we are talking about touching the vicinity, I think we should\n> tighten the -L s,e parsing rules a bit further.\n>\n> There is an undocumented code that swaps start and end if the given end is\n> smaller than the start.  This triggers even when \"-L280,300\" is mis-typed\n> as \"-L280,30\".  I was bitten by this more than once---when the input does\n> not make sense, we should actively error out, instead of doing a wrong\n> thing.  I suspect that I coded it that way _only_ to support this pattern:\n>\n>        -L'/^#endif \\/\\* !WINDOWS \\/\\*/,-30'\n>\n> i.e. \"blame 30 lines before the '#endif' line\".  But the code also\n> internally turns \"-L50,-20\" into \"-L 50,30\" and then swaps them to may\n> make it \"-L30,50\"; this was merely an unintended side effect.\n\nI was curious what sed does. At least on my system, sed -n -e '10,1p'\nprints just line 1. Seems a bit odd. sed -n -e '1,10000p' just prints\nto the end, and doesn't error out if there are less than 10k lines.\n\n> I do want to see -L'/regexp/,-offset' keep working, I do not mind if we\n> keep taking \"-L50,-20\" as an unintuitive way to spell \"-L30,50\", or reject\n> \"-L50,-20\" as a nonsense.  But I do want to see us reject \"-L280,30\" as a\n> typo.\n>\n> As to use of more than one -L option, especially when the start (or end\n> for that matter) is specified with an regexp, I am of two minds.\n\nActually, I was not planning on supporting multiple -L options, but rather...\n\n> When annotating the body of two functions, frotz and nitfol, I might\n> expect this to work:\n>\n>    -L'/^int frotz(/,/^}/'  -L'/^int nitfol(/,/^}/'\n>\n> regardless of the order these functions appear in the blob (i.e. nitfol\n> may be defined first).  This requires that parsing of \"regexp\" needs to\n> reset to the beginning of blob for each -L option (iow, multiple -L are\n> parsed independently from each other).\n>\n> But at the same time, if I am actually looking at the blob contents in one\n> terminal while spelling the blame command line in another, it would be\n> nicer if the multiple -L looked for patterns incrementally.  I may\n> appreciate if I can write the above command line as:\n>\n>    -L'/^int frotz(/,/^}/'  -L'/nitfol/,/^}/'\n>\n> when I can see in my \"less\" of the blob contents in the other terminal\n> that the first line that has string \"nitfol\" after the end of the\n> definition of \"frotz\" is the beginning of function \"nitfol\".\n>\n> Another thing we _might_ want to consider doing is something like:\n>\n>    -L'*/^#ifdef WINDOWS/,/^#endif \\/\\* WINDOWS \\/\\*/'\n>\n> to tell it \"I don't care to count how many WINDOWS ifdef blocks there are;\n> grab all of them\".\n\nThat was my aim, but the syntax I'd settled on was to use\n-L/pattern/..END where END is either a numerical argument or another\npattern. IOW, \"..\" instead of \",\".\n\n> Regardless of how parsing of multiple -L goes, you need to be careful to\n> sort the resulting line ranges and possibly coalesce them when there are\n> overlaps (e.g. \"-L12,+7 -L10,+5\" should become \"-L10,17\").  And be careful\n> about refcounting of origin.  You'll be making multiple blame_ent and\n> queuing them to the scoreboard when starting, all pointing at the blame\n> target blob; the origin blob needs to start with the right number of\n> references to keep origin_decref() discarding it.\n\nUnderstood.\n\nj.\n"},{"id":"134172","messageId":"7v8wb098kv.fsf@alter.siamese.dyndns.org","threadId":"22600","inReplyTo":"76718491002101139m4061fb90qcee7d34fca9f242f@mail.gmail.com","subject":"Re: [PATCH] blame: allow -L n,m to have an m bigger than the file's line count","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-10T19:47:28Z","receivedAt":"2010-02-10T19:47:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n>> As to use of more than one -L option, especially when the start (or end\n>> for that matter) is specified with an regexp, I am of two minds.\n>\n> Actually, I was not planning on supporting multiple -L options, but rather...\n\nThat is extremely sad.\n"},{"id":"134173","messageId":"76718491002101151x20ff88aev8a5d2de806f71a73@mail.gmail.com","threadId":"22600","inReplyTo":"7v8wb098kv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame: allow -L n,m to have an m bigger than the file's line count","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-02-10T19:51:58Z","receivedAt":"2010-02-10T19:51:58Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Feb 10, 2010 at 2:47 PM, Junio C Hamano wrote:\n> That is extremely sad.\n\nFor you, I'll see what I can do. I don't like the maintainer to be grumpy. :-)\n\nj.\n"},{"id":"134280","messageId":"7vfx57mhaj.fsf@alter.siamese.dyndns.org","threadId":"22600","inReplyTo":"76718491002101139m4061fb90qcee7d34fca9f242f@mail.gmail.com","subject":"Re: [PATCH] blame: allow -L n,m to have an m bigger than the file's line count","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-12T00:25:24Z","receivedAt":"2010-02-12T00:25:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n>> Another thing we _might_ want to consider doing is something like:\n>>\n>>    -L'*/^#ifdef WINDOWS/,/^#endif \\/\\* WINDOWS \\/\\*/'\n>>\n>> to tell it \"I don't care to count how many WINDOWS ifdef blocks there are;\n>> grab all of them\".\n>\n> That was my aim, but the syntax I'd settled on was to use\n> -L/pattern/..END where END is either a numerical argument or another\n> pattern. IOW, \"..\" instead of \",\".\n\nI would suggest against using that syntax.\n\nUsers of different systems use A,B or A..B as range notations, but there\nisn't anything that helps the unsuspecting learners to learn and memorize\nthat double-dot variant A..B has a repeating semantics and comma A,B does\nnot.\n"}]}