Multiple line ranges and files in line level history browser

Multiple line ranges and files in line level history browser

[Bo Yang]

Hi,

    Since we want to support to browse the history of multiple line
ranges in multiple files, so we should design a workable way to let
users specify these kind of thing. Generally, git revision walking
mechanism support syntax like:

git log <revision> -- <file1> <file2>

That's that we can specify one revision and multiple files one time.
And the dashdash is optional. And I want to introduce a new way for
line ranges argument based on this, that is something like:

git log <revision> -L1,8 -L45,+6 <file1> -L/some/,/end/ -L9,29 <file2> <file3>

That's that putting the line ranges arguments just before the filename
argument.  And here, no dashdash should be provided by users, if there
is, all the following arguments are still take considered as normal
filenames. And this helps if there are some files in your working
directory called '-L1,8'... Yes, you can always try to browse the line
level history of such a strange file by 'git log -L1,8 <revision> --
-L1,8' .

And the principle for this:
1. After '--', all following arguments are treated as filenames, just
as what we do now;
2. After any <filename>, all following arguments are treated as
whether ranges or filenames, using -L to separate;
3. '-L' can be specified at any place, and all ranges will be assigned
to the first filename following the '-L';
4. If some file has no '-L', we will display the normal log for that
file just like git log -p <file>.

Since '-L' option is also used by many other command, I don't know the
exact impact of this, anyway we can use another name anytime...

If there is no opposition for this kind of option syntax, I will try
to implement it in revision.c. ;-)

Regards!
Bo
-- 
My blog: http://blog.morebits.org

Re: Multiple line ranges and files in line level history browser

[Junio C Hamano]

Bo Yang <struggleyb.nku@gmail.com> writes:

> git log <revision> -L1,8 -L45,+6 <file1> -L/some/,/end/ -L9,29 <file2> <file3>
> ...
> Since '-L' option is also used by many other command, I don't know the
> exact impact of this, anyway we can use another name anytime...
>
> If there is no opposition for this kind of option syntax, I will try
> to implement it in revision.c. ;-)

I'd rather not to see this in revision.c at all.  The revision.c parser
has always been options and then pathspecs and never takes individual
filnames, except for "--follow" that is an afterthought checkbox hack that
lets the main parser parse and then reject a generic pathspec after the
fact.

I think the right place to do this sort of thing is in the incremental
output of "blame".  The command only takes individual filenames and never
takes general pathspecs, it knows about -L as line-range (and as far as I
see, it is the only command that does so) and I think its output routine
already has the right infrastructure for doing this.  The "--incremental"
output itself is designed for porcelain scripts to interpret and do the UI
around it, but that does not mean it can have another mode that gives
human readable output.  When "blame --incremental" emits the record that
says "these lines are blamed to this path in this commit", it has already
run internally necessary "diff" between blobs to find that
information---you should be able to re-run the diff (or keep the output
from "diff" around, but I would suggest against it) and show something
like "git log -p" output but is limited to the hunks that actually touch
the areas that we are following.

Re: Multiple line ranges and files in line level history browser

[Bo Yang]

Hi,
On Mon, May 10, 2010 at 2:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> If there is no opposition for this kind of option syntax, I will try
>> to implement it in revision.c. ;-)
>
> I'd rather not to see this in revision.c at all.  The revision.c parser
> has always been options and then pathspecs and never takes individual
> filnames, except for "--follow" that is an afterthought checkbox hack that
> lets the main parser parse and then reject a generic pathspec after the
> fact.

Ok, this will be put in the builtin/log.c

> I think the right place to do this sort of thing is in the incremental
> output of "blame".  The command only takes individual filenames and never
> takes general pathspecs, it knows about -L as line-range (and as far as I
> see, it is the only command that does so) and I think its output routine
> already has the right infrastructure for doing this.  The "--incremental"
> output itself is designed for porcelain scripts to interpret and do the UI
> around it, but that does not mean it can have another mode that gives
> human readable output.  When "blame --incremental" emits the record that
> says "these lines are blamed to this path in this commit", it has already
> run internally necessary "diff" between blobs to find that
> information---you should be able to re-run the diff (or keep the output
> from "diff" around, but I would suggest against it) and show something
> like "git log -p" output but is limited to the hunks that actually touch
> the areas that we are following.
>

Hmm, maybe I should say something about my general plan about how to
implement the line level browser here.

Generally, I plan to change two place in major to make the history of
line ranges.
1. The definition of TREESAME. TREESAME will be changed to consider
the line ranges if -L option is given;
With this, the history simplification can be done very well for line
level history traversal. And even well for parent rewriting to support
--graph option.
Also, line tracing will be done along the process of the history
simplification in the same time. And the tracing algorithm will be
written in another new source file and get called in revision.c (maybe
file_change and file_add_remove)...

2. The diff format, maybe a DIFF_FORMAT_LINE option will be added, and
a new function builtin_diffline() will be added to output the line
level diffs.

This has some difference with my original proposal on the source files
which need to modify. But I think this is the right way to do things
after I have digged more into git source code.
I hope this very rough idea can help you to understand what my
following patches will look like.

Regards!
Bo
-- 
My blog: http://blog.morebits.org

Re: Multiple line ranges and files in line level history browser

[Thomas Rast]

Bo Yang wrote:
> On Mon, May 10, 2010 at 2:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > I'd rather not to see this in revision.c at all.  The revision.c parser
> > has always been options and then pathspecs and never takes individual
> > filnames, except for "--follow" that is an afterthought checkbox hack that
> > lets the main parser parse and then reject a generic pathspec after the
> > fact.
> 
> Ok, this will be put in the builtin/log.c
[...]
> 1. The definition of TREESAME. TREESAME will be changed to consider
> the line ranges if -L option is given;
> With this, the history simplification can be done very well for line
> level history traversal. And even well for parent rewriting to support
> --graph option.

Doesn't this actually mean it should go in revision.c, after all?

AFAICS you make a case that this feature is an extension of the path
filtering and also of --follow.  This is not strictly speaking true,
since '-- path' only cares about the path itself, '--follow -- path'
tracks only "sufficiently close" renames and '-L0,inf path'[0] tracks
the lines across files even if it's only small chunks.

But I still think it's a good argument.  And then if git-log learns
about it, why should this be a special case that only the log family
understands, but not rev-list?

And since we can't remove --follow for hysterical raisins, you might
as well link the whole "what moved where" tracking logic to a place
where it helps fixing --follow.

Or am I totally on the wrong page?


[0] making up syntax for the purposes of the example, please don't
    think of this as normative or even a good idea

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: Multiple line ranges and files in line level history browser

[Jonathan Nieder]

Hi,

Bo Yang wrote:

> git log <revision> -L1,8 -L45,+6 <file1> -L/some/,/end/ -L9,29 <file2> <file3>

I like it.  It looks like paranoid script authors would have to check
for paths like ‘--’ and ‘-L’ and quote them as ‘./--’ and ‘./-L’, a
small price to pay for a nice syntax.

Unfortunately, this is completely incompatible with the existing blame
option syntax.  i.e., existing scripts might do things like this:

  git blame -L1,8 -C <file>

or

  git blame -L1,8 <rev> <file>

Maybe there should be a line range required before every file
specifier in this syntax, to avoid trouble.  Borrowing syntax from sed,
this makes

 git log <rev> -L1,8 -L45,+6 <file1> -L/some/,/end/ -L9,29 <file2> -L1,$ <file3>

which is also a little clearer to look at, I think.

> 'git log -L1,8 <revision> -- -L1,8' .

This provides a single line range specifier for all files?  Sounds
convenient.

  # who wrote the opening comments?
  git blame -L '/^[/][*]/,/^ [*][/]/' -- '*.c'

Summing up, with my refinement above, a human would parse args like
this:

  When an -L option is encountered, remember the current state.

  Try to parse the remaining arguments as ((-L range)* filespec)*,
  where filespec has some appropriately strict meaning that forbids
  ‘-L’ and ‘--’.  If that succeeds, we’re done.

  Otherwise, rewind.  Look for an upcoming ‘--’.  If -- is found, any
  -L arguments before the -- apply to all files specified.  Unclaimed
  arguments before the -- are revision specifiers.

  If no -- is found either, any -L arguments before the first
  unclaimed argument that is not unambiguously a revision apply to all
  files specified.  Arguments from that point on must be unambiguously
  paths.

A little hairy.  Maybe you can do better, but already it seems okay.
Maybe the last case ought to disallow multiple -L arguments and
multiple files, to encourage people to use the first syntax or an
explicit ‘--’.

Hope that helps,
Jonathan

Re: Multiple line ranges and files in line level history browser

[Jonathan Nieder]

Jonathan Nieder wrote:

>   If -- is found, any
>   -L arguments before the -- apply to all files specified.  Unclaimed
>   arguments before the -- are revision specifiers.

More comments.

The -L arguments describe lines in some particular revision of the
files, so how would arbitrary ‘rev-list’-style revision specifiers
work here?  They don’t: in ‘blame’, one "positive" revision is allowed
and the rest must be negative.  Good.

The modified proposal was, roughly:

  git blame [options, no -L among them] revs ((-L range)... filespec)...
  git blame [options, -L permitted] revs -- [filespec...]
  git blame [options, -L permitted] revs [filespec...]

“...” means “one or more”.  How to know whether the -L or revision
arguments are encountered first?  One approach is to abuse
STOP_AT_NON_OPTION to catch the -L, revisions, and filespecs as they
appear.  Probably better would be to make -L an unknown option and
rely on parse_revision_opt leaving a residue of any revisions it
finds, so that after the first pass, the first syntax can be
distinguished from the others by the first argument starting with "-L".

Feel free to do something else entirely (including another syntax) if
you prefer, of course.

Here’s a patch that makes STOP_AT_NON_OPTION easier to abuse, without
affecting current users.  Maybe it would make it easier to play
around.

Good night,
Jonathan

 parse-options.c |    3 ++-
 parse-options.h |    1 +
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 8546d85..4e3532b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -372,7 +372,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			if (parse_nodash_opt(ctx, arg, options) == 0)
 				continue;
 			if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
-				break;
+				return PARSE_OPT_NON_OPTION;
 			ctx->out[ctx->cpidx++] = ctx->argv[0];
 			continue;
 		}
@@ -454,6 +454,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 		exit(129);
+	case PARSE_OPT_NON_OPTION:
 	case PARSE_OPT_DONE:
 		break;
 	default: /* PARSE_OPT_UNKNOWN */
diff --git a/parse-options.h b/parse-options.h
index 7581e93..4773cf9 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -160,6 +160,7 @@ extern NORETURN void usage_msg_opt(const char *msg,
 enum {
 	PARSE_OPT_HELP = -1,
 	PARSE_OPT_DONE,
+	PARSE_OPT_NON_OPTION,
 	PARSE_OPT_UNKNOWN,
 };
 
-- 
1.7.1

Re: Multiple line ranges and files in line level history browser

[Bo Yang]

Hi Jonathan,
    Many thanks to provide so much precious advice and even a patch, I
think the patch is really helpful. Thanks.
    After some thought, I think we should keep the syntax as simple,
see my following comments.

On Mon, May 10, 2010 at 5:31 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
> The -L arguments describe lines in some particular revision of the
> files, so how would arbitrary ‘rev-list’-style revision specifiers
> work here?  They don’t: in ‘blame’, one "positive" revision is allowed
> and the rest must be negative.  Good.

A great reminder, thanks!

> The modified proposal was, roughly:
>
>  git blame [options, no -L among them] revs ((-L range)... filespec)...
>  git blame [options, -L permitted] revs -- [filespec...]
>  git blame [options, -L permitted] revs [filespec...]
>
> “...” means “one or more”.  How to know whether the -L or revision
> arguments are encountered first?  One approach is to abuse
> STOP_AT_NON_OPTION to catch the -L, revisions, and filespecs as they
> appear.  Probably better would be to make -L an unknown option and
> rely on parse_revision_opt leaving a residue of any revisions it
> finds, so that after the first pass, the first syntax can be
> distinguished from the others by the first argument starting with "-L".

How if the user provide now <revs> argument at all. In that case, we
may encounter the '-L' firstly too. And then we need to check whether
the non option argument is revision specifier. I think this kind of
check is not good to appear in builtin/log.c

> Feel free to do something else entirely (including another syntax) if
> you prefer, of course.

Yeah, I want to make a simpler syntax for the line level browser. It is:

git log [options without -L] revs ((-Llines)... filespec)...

We can then parse the options by:
1. Make -L a unknown option and call parse_revision_opt to filter out
all options and leave out  revs, -L and pathspec.
2. Parse the remain options with '-L' a known option and your
STOP_AT_NON_OPTION way to matching all line ranges with pathspec.
Error out when there are some pathspec which has no line ranges
specified. And this time, leaving out the revs and pathspec. And put a
'--' just before the first '-L'.
3. Call setup_revisions using the remain arguments.

> Here’s a patch that makes STOP_AT_NON_OPTION easier to abuse, without
> affecting current users.  Maybe it would make it easier to play
> around.
>
> Good night,
> Jonathan
>
>  parse-options.c |    3 ++-
>  parse-options.h |    1 +
>  2 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 8546d85..4e3532b 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -372,7 +372,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>                        if (parse_nodash_opt(ctx, arg, options) == 0)
>                                continue;
>                        if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
> -                               break;
> +                               return PARSE_OPT_NON_OPTION;
>                        ctx->out[ctx->cpidx++] = ctx->argv[0];
>                        continue;
>                }
> @@ -454,6 +454,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
>        switch (parse_options_step(&ctx, options, usagestr)) {
>        case PARSE_OPT_HELP:
>                exit(129);
> +       case PARSE_OPT_NON_OPTION:
>        case PARSE_OPT_DONE:
>                break;
>        default: /* PARSE_OPT_UNKNOWN */
> diff --git a/parse-options.h b/parse-options.h
> index 7581e93..4773cf9 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -160,6 +160,7 @@ extern NORETURN void usage_msg_opt(const char *msg,
>  enum {
>        PARSE_OPT_HELP = -1,
>        PARSE_OPT_DONE,
> +       PARSE_OPT_NON_OPTION,
>        PARSE_OPT_UNKNOWN,
>  };
>
> --
> 1.7.1
>
>

Really thanks for this patch I will use it.

Regards!
Bo
-- 
My blog: http://blog.morebits.org

Re: Multiple line ranges and files in line level history browser

[Bo Yang]

Hi,
On Mon, May 10, 2010 at 5:06 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> I like it.  It looks like paranoid script authors would have to check
> for paths like ‘--’ and ‘-L’ and quote them as ‘./--’ and ‘./-L’, a
> small price to pay for a nice syntax.
>
> Unfortunately, this is completely incompatible with the existing blame
> option syntax.  i.e., existing scripts might do things like this:
>
>  git blame -L1,8 -C <file>
>
> or
>
>  git blame -L1,8 <rev> <file>
>
> Maybe there should be a line range required before every file
> specifier in this syntax, to avoid trouble.  Borrowing syntax from sed,
> this makes
>
>  git log <rev> -L1,8 -L45,+6 <file1> -L/some/,/end/ -L9,29 <file2> -L1,$ <file3>
>
> which is also a little clearer to look at, I think.
>
>> 'git log -L1,8 <revision> -- -L1,8' .
>
> This provides a single line range specifier for all files?  Sounds
> convenient.

Ah, I don't mean that...
I just want to provide a way to let the users view the line range
history of files like '-L1,8'. But since you remind me users can
always use 'log -L1,8 ./-L1,8' to do this, I think we should abandon
this kind of syntax.

>  # who wrote the opening comments?
>  git blame -L '/^[/][*]/,/^ [*][/]/' -- '*.c'

I think users can always use a little shell script to achieve this, we
should not bother to implement such a complex thing which can be
easily done by shell script.


Regards!
Bo
-- 
My blog: http://blog.morebits.org

Re: Multiple line ranges and files in line level history browser

[Jonathan Nieder]

Hi,

Bo Yang wrote:
> On Mon, May 10, 2010 at 5:06 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>  # who wrote the opening comments?
>>  git blame -L '/^[/][*]/,/^ [*][/]/' -- '*.c'
>
> I think users can always use a little shell script to achieve this, we
> should not bother to implement such a complex thing which can be
> easily done by shell script.

True enough.  Thanks for the sanity.

Jonathan