{"thread":{"id":"23756","subject":"Multiple line ranges and files in line level history browser","startedAt":"2010-05-09T14:00:32Z","lastAt":"2010-05-12T07:57:10Z","messageCount":9,"participants":["Bo Yang","Junio C Hamano","Jonathan Nieder","Thomas Rast"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"141313","messageId":"p2q41f08ee11005090700xdb6070dajab0e2d77b1d0f9fc@mail.gmail.com","threadId":"23756","inReplyTo":null,"subject":"Multiple line ranges and files in line level history browser","fromName":"Bo Yang","fromEmail":"struggleyb.nku@gmail.com","sentAt":"2010-05-09T14:00:32Z","receivedAt":"2010-05-09T14:00:32Z","isPatch":false,"sender":{"key":"struggleyb.nku@gmail.com","avatar":"https://avatars.githubusercontent.com/u/233030?v=4"},"body":"Hi,\n\n Since we want to support to browse the history of multiple line\nranges in multiple files, so we should design a workable way to let\nusers specify these kind of thing. Generally, git revision walking\nmechanism support syntax like:\n\ngit log -- \n\nThat's that we can specify one revision and multiple files one time.\nAnd the dashdash is optional. And I want to introduce a new way for\nline ranges argument based on this, that is something like:\n\ngit log -L1,8 -L45,+6 -L/some/,/end/ -L9,29 \n\nThat's that putting the line ranges arguments just before the filename\nargument. And here, no dashdash should be provided by users, if there\nis, all the following arguments are still take considered as normal\nfilenames. And this helps if there are some files in your working\ndirectory called '-L1,8'... Yes, you can always try to browse the line\nlevel history of such a strange file by 'git log -L1,8 --\n-L1,8' .\n\nAnd the principle for this:\n1. After '--', all following arguments are treated as filenames, just\nas what we do now;\n2. After any , all following arguments are treated as\nwhether ranges or filenames, using -L to separate;\n3. '-L' can be specified at any place, and all ranges will be assigned\nto the first filename following the '-L';\n4. If some file has no '-L', we will display the normal log for that\nfile just like git log -p .\n\nSince '-L' option is also used by many other command, I don't know the\nexact impact of this, anyway we can use another name anytime...\n\nIf there is no opposition for this kind of option syntax, I will try\nto implement it in revision.c. ;-)\n\nRegards!\nBo\n-- \nMy blog: http://blog.morebits.org\n"},{"id":"141324","messageId":"7v1vdl9btm.fsf@alter.siamese.dyndns.org","threadId":"23756","inReplyTo":"p2q41f08ee11005090700xdb6070dajab0e2d77b1d0f9fc@mail.gmail.com","subject":"Re: Multiple line ranges and files in line level history browser","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-05-09T18:20:21Z","receivedAt":"2010-05-09T18:20:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bo Yang writes:\n\n> git log -L1,8 -L45,+6 -L/some/,/end/ -L9,29 \n> ...\n> Since '-L' option is also used by many other command, I don't know the\n> exact impact of this, anyway we can use another name anytime...\n>\n> If there is no opposition for this kind of option syntax, I will try\n> to implement it in revision.c. ;-)\n\nI'd rather not to see this in revision.c at all. The revision.c parser\nhas always been options and then pathspecs and never takes individual\nfilnames, except for \"--follow\" that is an afterthought checkbox hack that\nlets the main parser parse and then reject a generic pathspec after the\nfact.\n\nI think the right place to do this sort of thing is in the incremental\noutput of \"blame\". The command only takes individual filenames and never\ntakes general pathspecs, it knows about -L as line-range (and as far as I\nsee, it is the only command that does so) and I think its output routine\nalready has the right infrastructure for doing this. The \"--incremental\"\noutput itself is designed for porcelain scripts to interpret and do the UI\naround it, but that does not mean it can have another mode that gives\nhuman readable output. When \"blame --incremental\" emits the record that\nsays \"these lines are blamed to this path in this commit\", it has already\nrun internally necessary \"diff\" between blobs to find that\ninformation---you should be able to re-run the diff (or keep the output\nfrom \"diff\" around, but I would suggest against it) and show something\nlike \"git log -p\" output but is limited to the hunks that actually touch\nthe areas that we are following.\n"},{"id":"141334","messageId":"20100509210654.GA1637@progeny.tock","threadId":"23756","inReplyTo":"p2q41f08ee11005090700xdb6070dajab0e2d77b1d0f9fc@mail.gmail.com","subject":"Re: Multiple line ranges and files in line level history browser","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-09T21:06:54Z","receivedAt":"2010-05-09T21:06:54Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nBo Yang wrote:\n\n> git log -L1,8 -L45,+6 -L/some/,/end/ -L9,29 \n\nI like it. It looks like paranoid script authors would have to check\nfor paths like ‘--’ and ‘-L’ and quote them as ‘./--’ and ‘./-L’, a\nsmall price to pay for a nice syntax.\n\nUnfortunately, this is completely incompatible with the existing blame\noption syntax. i.e., existing scripts might do things like this:\n\n git blame -L1,8 -C \n\nor\n\n git blame -L1,8 \n\nMaybe there should be a line range required before every file\nspecifier in this syntax, to avoid trouble. Borrowing syntax from sed,\nthis makes\n\n git log -L1,8 -L45,+6 -L/some/,/end/ -L9,29 -L1,$ \n\nwhich is also a little clearer to look at, I think.\n\n> 'git log -L1,8 -- -L1,8' .\n\nThis provides a single line range specifier for all files? Sounds\nconvenient.\n\n # who wrote the opening comments?\n git blame -L '/^[/][*]/,/^ [*][/]/' -- '*.c'\n\nSumming up, with my refinement above, a human would parse args like\nthis:\n\n When an -L option is encountered, remember the current state.\n\n Try to parse the remaining arguments as ((-L range)* filespec)*,\n where filespec has some appropriately strict meaning that forbids\n ‘-L’ and ‘--’. If that succeeds, we’re done.\n\n Otherwise, rewind. Look for an upcoming ‘--’. If -- is found, any\n -L arguments before the -- apply to all files specified. Unclaimed\n arguments before the -- are revision specifiers.\n\n If no -- is found either, any -L arguments before the first\n unclaimed argument that is not unambiguously a revision apply to all\n files specified. Arguments from that point on must be unambiguously\n paths.\n\nA little hairy. Maybe you can do better, but already it seems okay.\nMaybe the last case ought to disallow multiple -L arguments and\nmultiple files, to encourage people to use the first syntax or an\nexplicit ‘--’.\n\nHope that helps,\nJonathan\n"},{"id":"141363","messageId":"20100510093120.GA4445@progeny.tock","threadId":"23756","inReplyTo":"20100509210654.GA1637@progeny.tock","subject":"Re: Multiple line ranges and files in line level history browser","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-10T09:31:20Z","receivedAt":"2010-05-10T09:31:20Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> If -- is found, any\n> -L arguments before the -- apply to all files specified. Unclaimed\n> arguments before the -- are revision specifiers.\n\nMore comments.\n\nThe -L arguments describe lines in some particular revision of the\nfiles, so how would arbitrary ‘rev-list’-style revision specifiers\nwork here? They don’t: in ‘blame’, one \"positive\" revision is allowed\nand the rest must be negative. Good.\n\nThe modified proposal was, roughly:\n\n git blame [options, no -L among them] revs ((-L range)... filespec)...\n git blame [options, -L permitted] revs -- [filespec...]\n git blame [options, -L permitted] revs [filespec...]\n\n“...” means “one or more”. How to know whether the -L or revision\narguments are encountered first? One approach is to abuse\nSTOP_AT_NON_OPTION to catch the -L, revisions, and filespecs as they\nappear. Probably better would be to make -L an unknown option and\nrely on parse_revision_opt leaving a residue of any revisions it\nfinds, so that after the first pass, the first syntax can be\ndistinguished from the others by the first argument starting with \"-L\".\n\nFeel free to do something else entirely (including another syntax) if\nyou prefer, of course.\n\nHere’s a patch that makes STOP_AT_NON_OPTION easier to abuse, without\naffecting current users. Maybe it would make it easier to play\naround.\n\nGood night,\nJonathan\n\n parse-options.c | 3 ++-\n parse-options.h | 1 +\n 2 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 8546d85..4e3532b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -372,7 +372,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tif (parse_nodash_opt(ctx, arg, options) == 0)\n \t\t\t\tcontinue;\n \t\t\tif (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)\n-\t\t\t\tbreak;\n+\t\t\t\treturn PARSE_OPT_NON_OPTION;\n \t\t\tctx->out[ctx->cpidx++] = ctx->argv[0];\n \t\t\tcontinue;\n \t\t}\n@@ -454,6 +454,7 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n \t\texit(129);\n+\tcase PARSE_OPT_NON_OPTION:\n \tcase PARSE_OPT_DONE:\n \t\tbreak;\n \tdefault: /* PARSE_OPT_UNKNOWN */\ndiff --git a/parse-options.h b/parse-options.h\nindex 7581e93..4773cf9 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -160,6 +160,7 @@ extern NORETURN void usage_msg_opt(const char *msg,\n enum {\n \tPARSE_OPT_HELP = -1,\n \tPARSE_OPT_DONE,\n+\tPARSE_OPT_NON_OPTION,\n \tPARSE_OPT_UNKNOWN,\n };\n \n-- \n1.7.1\n"},{"id":"141452","messageId":"AANLkTimCT7euE0CjGBx88MlfVJhxVo1GdOtnMhtZqBQs@mail.gmail.com","threadId":"23756","inReplyTo":"7v1vdl9btm.fsf@alter.siamese.dyndns.org","subject":"Re: Multiple line ranges and files in line level history browser","fromName":"Bo Yang","fromEmail":"struggleyb.nku@gmail.com","sentAt":"2010-05-11T05:43:27Z","receivedAt":"2010-05-11T05:43:27Z","isPatch":false,"sender":{"key":"struggleyb.nku@gmail.com","avatar":"https://avatars.githubusercontent.com/u/233030?v=4"},"body":"Hi,\nOn Mon, May 10, 2010 at 2:20 AM, Junio C Hamano wrote:\n>> If there is no opposition for this kind of option syntax, I will try\n>> to implement it in revision.c. ;-)\n>\n> I'd rather not to see this in revision.c at all.  The revision.c parser\n> has always been options and then pathspecs and never takes individual\n> filnames, except for \"--follow\" that is an afterthought checkbox hack that\n> lets the main parser parse and then reject a generic pathspec after the\n> fact.\n\nOk, this will be put in the builtin/log.c\n\n> I think the right place to do this sort of thing is in the incremental\n> output of \"blame\".  The command only takes individual filenames and never\n> takes general pathspecs, it knows about -L as line-range (and as far as I\n> see, it is the only command that does so) and I think its output routine\n> already has the right infrastructure for doing this.  The \"--incremental\"\n> output itself is designed for porcelain scripts to interpret and do the UI\n> around it, but that does not mean it can have another mode that gives\n> human readable output.  When \"blame --incremental\" emits the record that\n> says \"these lines are blamed to this path in this commit\", it has already\n> run internally necessary \"diff\" between blobs to find that\n> information---you should be able to re-run the diff (or keep the output\n> from \"diff\" around, but I would suggest against it) and show something\n> like \"git log -p\" output but is limited to the hunks that actually touch\n> the areas that we are following.\n>\n\nHmm, maybe I should say something about my general plan about how to\nimplement the line level browser here.\n\nGenerally, I plan to change two place in major to make the history of\nline ranges.\n1. The definition of TREESAME. TREESAME will be changed to consider\nthe line ranges if -L option is given;\nWith this, the history simplification can be done very well for line\nlevel history traversal. And even well for parent rewriting to support\n--graph option.\nAlso, line tracing will be done along the process of the history\nsimplification in the same time. And the tracing algorithm will be\nwritten in another new source file and get called in revision.c (maybe\nfile_change and file_add_remove)...\n\n2. The diff format, maybe a DIFF_FORMAT_LINE option will be added, and\na new function builtin_diffline() will be added to output the line\nlevel diffs.\n\nThis has some difference with my original proposal on the source files\nwhich need to modify. But I think this is the right way to do things\nafter I have digged more into git source code.\nI hope this very rough idea can help you to understand what my\nfollowing patches will look like.\n\nRegards!\nBo\n-- \nMy blog: http://blog.morebits.org\n"},{"id":"141454","messageId":"AANLkTim0MUq7JiKQavArUcxZxu7hvXUrO_AdEX6JbcVv@mail.gmail.com","threadId":"23756","inReplyTo":"20100509210654.GA1637@progeny.tock","subject":"Re: Multiple line ranges and files in line level history browser","fromName":"Bo Yang","fromEmail":"struggleyb.nku@gmail.com","sentAt":"2010-05-11T06:16:04Z","receivedAt":"2010-05-11T06:16:04Z","isPatch":false,"sender":{"key":"struggleyb.nku@gmail.com","avatar":"https://avatars.githubusercontent.com/u/233030?v=4"},"body":"Hi,\nOn Mon, May 10, 2010 at 5:06 AM, Jonathan Nieder wrote:\n> I like it.  It looks like paranoid script authors would have to check\n> for paths like ‘--’ and ‘-L’ and quote them as ‘./--’ and ‘./-L’, a\n> small price to pay for a nice syntax.\n>\n> Unfortunately, this is completely incompatible with the existing blame\n> option syntax.  i.e., existing scripts might do things like this:\n>\n>  git blame -L1,8 -C \n>\n> or\n>\n>  git blame -L1,8 \n>\n> Maybe there should be a line range required before every file\n> specifier in this syntax, to avoid trouble.  Borrowing syntax from sed,\n> this makes\n>\n>  git log -L1,8 -L45,+6 -L/some/,/end/ -L9,29 -L1,$ \n>\n> which is also a little clearer to look at, I think.\n>\n>> 'git log -L1,8 -- -L1,8' .\n>\n> This provides a single line range specifier for all files?  Sounds\n> convenient.\n\nAh, I don't mean that...\nI just want to provide a way to let the users view the line range\nhistory of files like '-L1,8'. But since you remind me users can\nalways use 'log -L1,8 ./-L1,8' to do this, I think we should abandon\nthis kind of syntax.\n\n>  # who wrote the opening comments?\n>  git blame -L '/^[/][*]/,/^ [*][/]/' -- '*.c'\n\nI think users can always use a little shell script to achieve this, we\nshould not bother to implement such a complex thing which can be\neasily done by shell script.\n\n\nRegards!\nBo\n-- \nMy blog: http://blog.morebits.org\n"},{"id":"141458","messageId":"20100511062828.GA29615@progeny.tock","threadId":"23756","inReplyTo":"AANLkTim0MUq7JiKQavArUcxZxu7hvXUrO_AdEX6JbcVv@mail.gmail.com","subject":"Re: Multiple line ranges and files in line level history browser","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-11T06:28:28Z","receivedAt":"2010-05-11T06:28:28Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nBo Yang wrote:\n> On Mon, May 10, 2010 at 5:06 AM, Jonathan Nieder wrote:\n\n>>  # who wrote the opening comments?\n>>  git blame -L '/^[/][*]/,/^ [*][/]/' -- '*.c'\n>\n> I think users can always use a little shell script to achieve this, we\n> should not bother to implement such a complex thing which can be\n> easily done by shell script.\n\nTrue enough. Thanks for the sanity.\n\nJonathan\n"},{"id":"141459","messageId":"AANLkTimhMLazMDk53fY3g3Fy0FXXHWIC9o5VST7fPQjG@mail.gmail.com","threadId":"23756","inReplyTo":"20100510093120.GA4445@progeny.tock","subject":"Re: Multiple line ranges and files in line level history browser","fromName":"Bo Yang","fromEmail":"struggleyb.nku@gmail.com","sentAt":"2010-05-11T06:28:51Z","receivedAt":"2010-05-11T06:28:51Z","isPatch":false,"sender":{"key":"struggleyb.nku@gmail.com","avatar":"https://avatars.githubusercontent.com/u/233030?v=4"},"body":"Hi Jonathan,\n Many thanks to provide so much precious advice and even a patch, I\nthink the patch is really helpful. Thanks.\n After some thought, I think we should keep the syntax as simple,\nsee my following comments.\n\nOn Mon, May 10, 2010 at 5:31 PM, Jonathan Nieder wrote:\n> Jonathan Nieder wrote:\n> The -L arguments describe lines in some particular revision of the\n> files, so how would arbitrary ‘rev-list’-style revision specifiers\n> work here?  They don’t: in ‘blame’, one \"positive\" revision is allowed\n> and the rest must be negative.  Good.\n\nA great reminder, thanks!\n\n> The modified proposal was, roughly:\n>\n>  git blame [options, no -L among them] revs ((-L range)... filespec)...\n>  git blame [options, -L permitted] revs -- [filespec...]\n>  git blame [options, -L permitted] revs [filespec...]\n>\n> “...” means “one or more”.  How to know whether the -L or revision\n> arguments are encountered first?  One approach is to abuse\n> STOP_AT_NON_OPTION to catch the -L, revisions, and filespecs as they\n> appear.  Probably better would be to make -L an unknown option and\n> rely on parse_revision_opt leaving a residue of any revisions it\n> finds, so that after the first pass, the first syntax can be\n> distinguished from the others by the first argument starting with \"-L\".\n\nHow if the user provide now argument at all. In that case, we\nmay encounter the '-L' firstly too. And then we need to check whether\nthe non option argument is revision specifier. I think this kind of\ncheck is not good to appear in builtin/log.c\n\n> Feel free to do something else entirely (including another syntax) if\n> you prefer, of course.\n\nYeah, I want to make a simpler syntax for the line level browser. It is:\n\ngit log [options without -L] revs ((-Llines)... filespec)...\n\nWe can then parse the options by:\n1. Make -L a unknown option and call parse_revision_opt to filter out\nall options and leave out revs, -L and pathspec.\n2. Parse the remain options with '-L' a known option and your\nSTOP_AT_NON_OPTION way to matching all line ranges with pathspec.\nError out when there are some pathspec which has no line ranges\nspecified. And this time, leaving out the revs and pathspec. And put a\n'--' just before the first '-L'.\n3. Call setup_revisions using the remain arguments.\n\n> Here’s a patch that makes STOP_AT_NON_OPTION easier to abuse, without\n> affecting current users.  Maybe it would make it easier to play\n> around.\n>\n> Good night,\n> Jonathan\n>\n>  parse-options.c |    3 ++-\n>  parse-options.h |    1 +\n>  2 files changed, 3 insertions(+), 1 deletions(-)\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index 8546d85..4e3532b 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -372,7 +372,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n>                        if (parse_nodash_opt(ctx, arg, options) == 0)\n>                                continue;\n>                        if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)\n> -                               break;\n> +                               return PARSE_OPT_NON_OPTION;\n>                        ctx->out[ctx->cpidx++] = ctx->argv[0];\n>                        continue;\n>                }\n> @@ -454,6 +454,7 @@ int parse_options(int argc, const char **argv, const char *prefix,\n>        switch (parse_options_step(&ctx, options, usagestr)) {\n>        case PARSE_OPT_HELP:\n>                exit(129);\n> +       case PARSE_OPT_NON_OPTION:\n>        case PARSE_OPT_DONE:\n>                break;\n>        default: /* PARSE_OPT_UNKNOWN */\n> diff --git a/parse-options.h b/parse-options.h\n> index 7581e93..4773cf9 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -160,6 +160,7 @@ extern NORETURN void usage_msg_opt(const char *msg,\n>  enum {\n>        PARSE_OPT_HELP = -1,\n>        PARSE_OPT_DONE,\n> +       PARSE_OPT_NON_OPTION,\n>        PARSE_OPT_UNKNOWN,\n>  };\n>\n> --\n> 1.7.1\n>\n>\n\nReally thanks for this patch I will use it.\n\nRegards!\nBo\n-- \nMy blog: http://blog.morebits.org\n"},{"id":"141513","messageId":"201005120957.10961.trast@student.ethz.ch","threadId":"23756","inReplyTo":"AANLkTimCT7euE0CjGBx88MlfVJhxVo1GdOtnMhtZqBQs@mail.gmail.com","subject":"Re: Multiple line ranges and files in line level history browser","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-05-12T07:57:10Z","receivedAt":"2010-05-12T07:57:10Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Bo Yang wrote:\n> On Mon, May 10, 2010 at 2:20 AM, Junio C Hamano wrote:\n> > I'd rather not to see this in revision.c at all. The revision.c parser\n> > has always been options and then pathspecs and never takes individual\n> > filnames, except for \"--follow\" that is an afterthought checkbox hack that\n> > lets the main parser parse and then reject a generic pathspec after the\n> > fact.\n> \n> Ok, this will be put in the builtin/log.c\n[...]\n> 1. The definition of TREESAME. TREESAME will be changed to consider\n> the line ranges if -L option is given;\n> With this, the history simplification can be done very well for line\n> level history traversal. And even well for parent rewriting to support\n> --graph option.\n\nDoesn't this actually mean it should go in revision.c, after all?\n\nAFAICS you make a case that this feature is an extension of the path\nfiltering and also of --follow. This is not strictly speaking true,\nsince '-- path' only cares about the path itself, '--follow -- path'\ntracks only \"sufficiently close\" renames and '-L0,inf path'[0] tracks\nthe lines across files even if it's only small chunks.\n\nBut I still think it's a good argument. And then if git-log learns\nabout it, why should this be a special case that only the log family\nunderstands, but not rev-list?\n\nAnd since we can't remove --follow for hysterical raisins, you might\nas well link the whole \"what moved where\" tracking logic to a place\nwhere it helps fixing --follow.\n\nOr am I totally on the wrong page?\n\n\n[0] making up syntax for the purposes of the example, please don't\n think of this as normative or even a good idea\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}