Skip to contentSkip to conversation

Improve line log documentation

PATCH Superseded Philippe Blain via GitGitGadget 5 participants 18 messages Dec 17, 2019 – Dec 26, 2019 Documentation

lore
Share & tools
lore

Philippe Blain via GitGitGadget

author
#0 lore raw

cover Improve line log documentation

These two patches add more information to the documentation for line history (git log -L) :

* Mention explicitly that only the --patch and --no-patch diff formats are supported
* Mention that any given regex must match in the starting revision

Regarding point 2 : I stumbled upon this while reading the history of 'filter_refs' in fetch-pack.c. I did

git log -s -L :filter_refs:fetch-pack.c

which shows a list of commits starting with 745f7a8cac (fetch-pack: move core code to libgit.a, 2012-10-26). So then I did

git log -s -L :filter_refs:fetch-pack.c -L :filter_refs:builtin/fetch-pack.c

which fails with

fatal: -L parameter 'filter_refs' starting at line 1: no match

Philippe Blain (2):
Documentation/git-log: document accepted line-log diff formats Documentation/git-log: mention that line-log regex must match in
starting revision

 Documentation/git-log.txt | 9 +++++++--
 Documentation/gitk.txt    | 6 +++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

base-commit: ad05a3d8e5a6a06443836b5e40434262d992889a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-494%2Fphil-blain%2Fline-log-doc-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-494/phil-blain/line-log-doc-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/494

Signature · 1 line
gitgitgadget
author
#1 lore raw

PATCH 1/2 Documentation/git-log: document accepted line-log diff formats

From: Philippe Blain <[email protected]>

Currently the line-log functionality (git log -L) only supports displaying patch output (-p, its default behavior) and suppressing it (-s). A check was added in the code to that effect in 5314efaea (line-log:
detect unsupported formats, 2019-03-10) but the documentation was not updated.

Explicitly mention that -L implies -p, that patch output can be suppressed using -s, and that all other diff formats are not allowed.

Additionnally, mention that the ':<funcname>' form implies --function-context.

Signed-off-by Philippe Blain <[email protected]>
---
 Documentation/git-log.txt | 6 +++++-
 Documentation/gitk.txt    | 6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)
Documentation/git-log.txt +5 -1
index b406bc4c48..1c52bf184d 100644
@@ -77,7 +77,11 @@ produced by `--stat`, etc.
 	not give any pathspec limiters.  This is currently limited to
 	a walk starting from a single revision, i.e., you may only
 	give zero or one positive revision arguments.
-	You can specify this option more than once.
+	You can specify this option more than once. Implies `--patch`.
+	If ``:<funcname>'' is given, implies `--function-context`.
+	Patch output can be suppressed using `-s`, but other diff formats
+	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
+	`--name-only`, `--name-status`, `--check`) are not currently implemented.
 +
 include::line-range-format.txt[]
Documentation/gitk.txt +5 -1
index 1eabb0aaf3..91993b9d4c 100644
@@ -106,7 +106,11 @@ linkgit:git-rev-list[1] for a complete list.
 	not give any pathspec limiters.  This is currently limited to
 	a walk starting from a single revision, i.e., you may only
 	give zero or one positive revision arguments.
-	You can specify this option more than once.
+	You can specify this option more than once. Implies `--patch`.
+	If ``:<funcname>'' is given, implies `--function-context`.
+	Patch output can be suppressed using `-s`, but other diff formats
+	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
+	`--name-only`, `--name-status`, `--check`) are not currently implemented.
 +
 *Note:* gitk (unlike linkgit:git-log[1]) currently only understands
 this option if you specify it "glued together" with its argument.  Do
Signature · 1 line
gitgitgadget
author
#6 lore raw

PATCH 2/2 Documentation/git-log: mention that line-log regex must match in starting revision

In reply to Philippe Blain via GitGitGadget

From: Philippe Blain <[email protected]>

When giving a regex as parameter <start> or <end> in
git log -L <start>,<end>:<file>, or a function name in
git log -L :<funcname>:<file>, the given regex must match in the starting revision, or else the command exits with a fatal error.

This is not obvious in the documentation, so add a note to that effect.

Signed-off-by Philippe Blain <[email protected]>
---
 Documentation/git-log.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Documentation/git-log.txt +2 -1
index 1c52bf184d..5c3fd39048 100644
@@ -76,7 +76,8 @@ produced by `--stat`, etc.
 	(or the function name regex <funcname>) within the <file>.  You may
 	not give any pathspec limiters.  This is currently limited to
 	a walk starting from a single revision, i.e., you may only
-	give zero or one positive revision arguments.
+	give zero or one positive revision arguments, and any given regex for
+	<start> or <end> (or <funcname>) must match in the starting revision.
 	You can specify this option more than once. Implies `--patch`.
 	If ``:<funcname>'' is given, implies `--function-context`.
 	Patch output can be suppressed using `-s`, but other diff formats
Signature · 1 line
gitgitgadget
#2 lore raw

Re: [PATCH 1/2] Documentation/git-log: document accepted line-log diff formats

In reply to Philippe Blain via GitGitGadget
Quoted 24 lines · Philippe Blain via GitGitGadget

From: Philippe Blain <[email protected]>

Currently the line-log functionality (git log -L) only supports displaying patch output (-p, its default behavior) and suppressing it (-s). A check was added in the code to that effect in 5314efaea (line-log:
detect unsupported formats, 2019-03-10) but the documentation was not updated.

Explicitly mention that -L implies -p, that patch output can be suppressed using -s, and that all other diff formats are not allowed.

Additionnally, mention that the ':<funcname>' form implies --function-context.

Signed-off-by Philippe Blain <[email protected]>
---
 Documentation/git-log.txt | 6 +++++-
 Documentation/gitk.txt    | 6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)
Documentation/git-log.txt +0 -0
index b406bc4c48..1c52bf184d 100644

Documentation/git-log.txt produced by `--stat`, etc. · line 81

Quoted 6 lines
Documentation/git-log.txt +2 -1
@@ -77,7 +77,11 @@ produced by `--stat`, etc.
 	not give any pathspec limiters.  This is currently limited to
 	a walk starting from a single revision, i.e., you may only
 	give zero or one positive revision arguments.
-	You can specify this option more than once.
+	You can specify this option more than once. Implies `--patch`.
+	If ``:<funcname>'' is given, implies `--function-context`.

':<funcname>' doesn't imply '--function-context', but defines a line range starting at the function-name-looking line that first matches "funcname" and ending just before the next function-name-looking line, and line-log will then show that line range as context for each commit. Although in many cases it shows diffs that look like as if '--function-context' were given, there are corner cases where they clearly differ, e.g. when following the history of a function that was at one point combined with the function directly below it.

Note the two commits in the middle that show two functions although only one of them was actually modified in each of those commits:

  $ git log --oneline -L:func:file.c
  04b0c16 Combine funcA() and funcB() into func()
  diff --git a/file.c b/file.c
  --- a/file.c
  +++ b/file.c
  @@ -1,9 +1,4 @@
  -int funcA()
  +int func()
   {
  -	return A;
  -}
  -
  -int funcB()
  -{
  -	return B;
  +	return A + B;
   }
  ed0d4d9 Modify funcB()
  
  diff --git a/file.c b/file.c
  --- a/file.c
  +++ b/file.c
  @@ -1,9 +1,9 @@
   int funcA()
   {
   	return A;
   }
   int funcB()
   {
  -	return b;
  +	return B;
   }
  0d4e9b5 Modify funcA()
  diff --git a/file.c b/file.c
  --- a/file.c
  +++ b/file.c
  @@ -1,9 +1,9 @@
   int funcA()
   {
  -	return a;
  +	return A;
   }
   int funcB()
   {
   	return b;
   }
  c3f8a44 Add funcA() and funcB()
  diff --git a/file.c b/file.c
  --- /dev/null
  +++ b/file.c
  @@ -0,0 +1,9 @@
  +int funcA()
  +{
  +	return a;
  +}
  +
  +int funcB()
  +{
  +	return b;
  +}

Now compare that to the same two middle commits shown with '-p --function-context', which doesn't show the unmodified function:

  $ git log --oneline -p --function-context file.c
  04b0c16 Combine funcA() and funcB() into func()
  diff --git a/file.c b/file.c
  index 89571b3..33301ea 100644
  --- a/file.c
  +++ b/file.c
  @@ -1,9 +1,4 @@
  -int funcA()
  +int func()
   {
  -	return A;
  -}
  -
  -int funcB()
  -{
  -	return B;
  +	return A + B;
   }
  ed0d4d9 Modify funcB()
  diff --git a/file.c b/file.c
  index 13592c8..89571b3 100644
  --- a/file.c
  +++ b/file.c
  @@ -5,5 +5,5 @@ int funcA()
   int funcB()
   {
  -	return b;
  +	return B;
   }
  0d4e9b5 Modify funcA()
  diff --git a/file.c b/file.c
  index 11e1e87..13592c8 100644
  --- a/file.c
  +++ b/file.c
  @@ -1,6 +1,6 @@
   int funcA()
   {
  -	return a;
  +	return A;
   }
   
   int funcB()
  c3f8a44 Add funcA() and funcB()
  diff --git a/file.c b/file.c
  new file mode 100644
  index 0000000..11e1e87
  --- /dev/null
  +++ b/file.c
  @@ -0,0 +1,9 @@
  +int funcA()
  +{
  +	return a;
  +}
  +
  +int funcB()
  +{
  +	return b;
  +}
#4 lore raw

Re: [PATCH 1/2] Documentation/git-log: document accepted line-log diff formats

In reply to Philippe Blain via GitGitGadget
Quoted 8 lines · Philippe Blain via GitGitGadget

From: Philippe Blain <[email protected]>

Currently the line-log functionality (git log -L) only supports displaying patch output (-p, its default behavior) and suppressing it (-s). A check was added in the code to that effect in 5314efaea (line-log:
detect unsupported formats, 2019-03-10) but the documentation was not updated.

Quoted 4 lines

Explicitly mention that -L implies -p, that patch output can be suppressed using -s, and that all other diff formats are not allowed.

Additionnally, mention that the ':<funcname>' form implies --function-context.

s/Additionnally/Additionally/

Quoted 11 lines
Signed-off-by Philippe Blain <[email protected]>
---
 Documentation/git-log.txt | 6 +++++-
 Documentation/gitk.txt    | 6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)
Documentation/git-log.txt +0 -0
index b406bc4c48..1c52bf184d 100644

Documentation/git-log.txt produced by `--stat`, etc. · line 82

Quoted 7 lines
Documentation/git-log.txt +3 -1
@@ -77,7 +77,11 @@ produced by `--stat`, etc.
 	not give any pathspec limiters.  This is currently limited to
 	a walk starting from a single revision, i.e., you may only
 	give zero or one positive revision arguments.
-	You can specify this option more than once.
+	You can specify this option more than once. Implies `--patch`.
+	If ``:<funcname>'' is given, implies `--function-context`.
+	Patch output can be suppressed using `-s`, but other diff formats

I would prefer the longer "--no-patch" over "-s", especially because you follow with several options in this style.

Quoted 10 lines
Documentation/git-log.txt +2 -0
+	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
+	`--name-only`, `--name-status`, `--check`) are not currently implemented.
 +
 include::line-range-format.txt[]
Documentation/gitk.txt +0 -0
index 1eabb0aaf3..91993b9d4c 100644

Documentation/gitk.txt linkgit:git-rev-list[1] for a complete list. · line 111

Quoted 7 lines
Documentation/gitk.txt +3 -1
@@ -106,7 +106,11 @@ linkgit:git-rev-list[1] for a complete list.
 	not give any pathspec limiters.  This is currently limited to
 	a walk starting from a single revision, i.e., you may only
 	give zero or one positive revision arguments.
-	You can specify this option more than once.
+	You can specify this option more than once. Implies `--patch`.
+	If ``:<funcname>'' is given, implies `--function-context`.
+	Patch output can be suppressed using `-s`, but other diff formats

(same)

Documentation/gitk.txt linkgit:git-rev-list[1] for a complete list. · line 113

Quoted 2 lines
Documentation/gitk.txt +2 -0
+	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
+	`--name-only`, `--name-status`, `--check`) are not currently implemented.

Thanks,
-Stolee

#7 lore raw

Re: [PATCH 2/2] Documentation/git-log: mention that line-log regex must match in starting revision

In reply to Philippe Blain via GitGitGadget
Quoted 9 lines · Philippe Blain via GitGitGadget

From: Philippe Blain <[email protected]>

When giving a regex as parameter <start> or <end> in
git log -L <start>,<end>:<file>, or a function name in
git log -L :<funcname>:<file>, the given regex must match in the starting revision, or else the command exits with a fatal error.

This is not obvious in the documentation, so add a note to that effect.

This seems helpful. Thanks.

-Stolee

#8 lore raw

Re: [PATCH 2/2] Documentation/git-log: mention that line-log regex must match in starting revision

Quoted 8 lines · Derrick Stolee
Quoted 7 lines · Philippe Blain via GitGitGadget

From: Philippe Blain <[email protected]>

When giving a regex as parameter <start> or <end> in
git log -L <start>,<end>:<file>, or a function name in
git log -L :<funcname>:<file>, the given regex must match in the starting revision, or else the command exits with a fatal error.

Quoted 4 lines
Quoted 2 lines

This is not obvious in the documentation, so add a note to that effect.

This seems helpful. Thanks.

Even when you specify <start> or <end> as a line number, they must exist in the starting revision or it would be a fatal error. If we are clarifying with this patch for completeness, I think we should also mention it together.

Or we could even drop the mention of regex and say <start> and <end>, regardless of how they are specified (including :<funcname> format), must exist in the starting revision.

#3 lore raw

Re: [PATCH 1/2] Documentation/git-log: document accepted line-log diff formats

In reply to SZEDER Gábor

That’s a good point. I did not think of such corner cases. I will remove that mention then.

Quoted 174 lines · SZEDER Gábor
Quoted 24 lines · Philippe Blain via GitGitGadget

From: Philippe Blain <[email protected]>

Currently the line-log functionality (git log -L) only supports displaying patch output (-p, its default behavior) and suppressing it (-s). A check was added in the code to that effect in 5314efaea (line-log:
detect unsupported formats, 2019-03-10) but the documentation was not updated.

Explicitly mention that -L implies -p, that patch output can be suppressed using -s, and that all other diff formats are not allowed.

Additionnally, mention that the ':<funcname>' form implies --function-context.

Signed-off-by Philippe Blain <[email protected]>

---
Documentation/git-log.txt | 6 +++++-
Documentation/gitk.txt | 6 +++++-

2 files changed, 10 insertions(+), 2 deletions(-)
Documentation/git-log.txt +0 -0
index b406bc4c48..1c52bf184d 100644

Documentation/git-log.txt produced by `--stat`, etc. · line 81

Quoted 6 lines
Documentation/git-log.txt +2 -1
@@ -77,7 +77,11 @@ produced by `--stat`, etc.
 	not give any pathspec limiters.  This is currently limited to
 	a walk starting from a single revision, i.e., you may only
 	give zero or one positive revision arguments.
-	You can specify this option more than once.
+	You can specify this option more than once. Implies `--patch`.
+	If ``:<funcname>'' is given, implies `--function-context`.

':<funcname>' doesn't imply '--function-context', but defines a line range starting at the function-name-looking line that first matches "funcname" and ending just before the next function-name-looking line, and line-log will then show that line range as context for each commit. Although in many cases it shows diffs that look like as if '--function-context' were given, there are corner cases where they clearly differ, e.g. when following the history of a function that was at one point combined with the function directly below it.

Note the two commits in the middle that show two functions although only one of them was actually modified in each of those commits:

 $ git log --oneline -L:func:file.c
 04b0c16 Combine funcA() and funcB() into func()

diff --git a/file.c b/file.c
--- a/file.c
+++ b/file.c
@@ -1,9 +1,4 @@
-int funcA()
+int func()
{
- return A;
-}
-
-int funcB()
-{
- return B;
+ return A + B;
}
ed0d4d9 Modify funcB()

diff --git a/file.c b/file.c
--- a/file.c
+++ b/file.c
@@ -1,9 +1,9 @@
int funcA()
{
return A;
}

int funcB()
{
- return b;
+ return B;
}
0d4e9b5 Modify funcA()

diff --git a/file.c b/file.c
--- a/file.c
+++ b/file.c
@@ -1,9 +1,9 @@
int funcA()
{
- return a;
+ return A;
}

int funcB()
{
return b;
}
c3f8a44 Add funcA() and funcB()

diff --git a/file.c b/file.c
--- /dev/null
+++ b/file.c
@@ -0,0 +1,9 @@
+int funcA()
+{
+ return a;
+}
+
+int funcB()
+{
+ return b;
+}

Now compare that to the same two middle commits shown with '-p --function-context', which doesn't show the unmodified function:

 $ git log --oneline -p --function-context file.c
 04b0c16 Combine funcA() and funcB() into func()
 diff --git a/file.c b/file.c
 index 89571b3..33301ea 100644
 --- a/file.c
 +++ b/file.c
 @@ -1,9 +1,4 @@
 -int funcA()
 +int func()
  {
 -	return A;
 -}
 -
 -int funcB()
 -{
 -	return B;
 +	return A + B;
  }
 ed0d4d9 Modify funcB()
 diff --git a/file.c b/file.c
 index 13592c8..89571b3 100644
 --- a/file.c
 +++ b/file.c
 @@ -5,5 +5,5 @@ int funcA()

int funcB()
{
- return b;
+ return B;
}
0d4e9b5 Modify funcA()
diff --git a/file.c b/file.c
index 11e1e87..13592c8 100644
--- a/file.c
+++ b/file.c
@@ -1,6 +1,6 @@
int funcA()
{
- return a;
+ return A;
}

int funcB()
c3f8a44 Add funcA() and funcB()
diff --git a/file.c b/file.c
new file mode 100644
index 0000000..11e1e87
--- /dev/null
+++ b/file.c
@@ -0,0 +1,9 @@
+int funcA()
+{
+ return a;
+}
+
+int funcB()
+{
+ return b;
+}

#5 lore raw

Re: [PATCH 1/2] Documentation/git-log: document accepted line-log diff formats

In reply to Derrick Stolee
Quoted 5 lines · Derrick Stolee

I would prefer the longer "--no-patch" over "-s", especially because you follow with several options in this style.

That’s a good idea. Thanks for the review!

#9 lore raw

Re: [PATCH 2/2] Documentation/git-log: mention that line-log regex must match in starting revision

In reply to Junio C Hamano
Quoted 6 lines · Junio C Hamano

Even when you specify <start> or <end> as a line number, they must exist in the starting revision or it would be a fatal error. If we are clarifying with this patch for completeness, I think we should also mention it together.

Thanks for the feedback. I did some tests :

    git log -L 73,2000000085:Documentation/git-log.txt
does not error and shows the history from line 73 to the end of the file.
    git log -L 300,2000000085:Documentation/git-log.txt 
errors out:
    fatal: file Documentation/git-log.txt has only 239 lines
But 
    git log -L 300,-2000000085:Documentation/git-log.txt
does not error out and shows the history for the whole file. Also,
    git log -L 1,-2000000085:Documentation/git-log.txt

does not error out and gives the history for the first line.

So I think that it’s really only regex that must match...

#10 lore raw

Re: [PATCH 2/2] Documentation/git-log: mention that line-log regex must match in starting revision

Quoted 6 lines · Philippe Blain
Quoted 6 lines · Junio C Hamano

Even when you specify <start> or <end> as a line number, they must exist in the starting revision or it would be a fatal error. If we are clarifying with this patch for completeness, I think we should also mention it together.

...

Quoted 3 lines
    git log -L 300,2000000085:Documentation/git-log.txt

errors out:
fatal: file Documentation/git-log.txt has only 239 lines

This case seems important enough to include what it means to "match".

Specifically, the range must match at least one line in the file.

Quoted 5 lines

But

    git log -L 300,-2000000085:Documentation/git-log.txt
does not error out and shows the history for the whole file. Also,
    git log -L 1,-2000000085:Documentation/git-log.txt

does not error out and gives the history for the first line.

Negative numbers in the ranges are a bit strange to include, and the large magnitude you include here seems unnecessary for the test. However, it appears that we do store signed values in the line-log machinery:

/* A range [start,end]. Lines are numbered starting at 0, and the
* ranges include start but exclude end. */

struct range {
	long start, end;
};

Perhaps these should be replaced with an unsigned value instead, just to be clear that negative values do not make sense in this context?

Or rather, do we want to enforce start < end if they are to be valuable? We apparently do assert() this in range_set_check_invariants() for all that does.

The current behavior of showing only the first line is a strange byproduct of allowing these odd ranges, but we may want to keep it for consistency. That would imply we allow start <= end, and auto-correct these negative values to have end = start. (This does not fix the 300,-1 case, but that is strange enough to be considered a bug, right?)

Thanks,
-Stolee

#14 lore raw

Re: [PATCH 2/2] Documentation/git-log: mention that line-log regex must match in starting revision

In reply to Philippe Blain
Quoted 7 lines · Philippe Blain
Quoted 6 lines · Junio C Hamano

Even when you specify <start> or <end> as a line number, they must exist in the starting revision or it would be a fatal error. If we are clarifying with this patch for completeness, I think we should also mention it together.

Thanks for the feedback. I did some tests :

I'll swap the order of your first two tests:

Quoted 3 lines
    git log -L 300,2000000085:Documentation/git-log.txt

errors out:
fatal: file Documentation/git-log.txt has only 239 lines

Here the entire line range is outside of the file, so there is not much we can do about it, but error out.

An alternative would be to treat it as an empty line range and then don't show any commits but exit silently, but I think that would be confusing ("why didn't I get any output?!"), and telling the user what's wrong is better ("Ah, ok, I mistyped 192 as 912").

Quoted 2 lines
    git log -L 73,2000000085:Documentation/git-log.txt

does not error and shows the history from line 73 to the end of the file.

Here there is an overlap between the given line range and the file (lines 73-239), so we have two possibilities:

- be strict and error out saying that the <end> doesn't make sense.

- be lax about it, and interpret the <end> as the end of file. This allows for cases like "I want line log from here to the end of file, but instead of finding out the exact number of lines in the file I'll just say 999999 that is surely larger than the file, and you shall do what I mean".

Those who implemented the line-log feature chose the latter. I think it's the better choice.

Quoted 5 lines

But

    git log -L 300,-2000000085:Documentation/git-log.txt
does not error out and shows the history for the whole file. Also,
    git log -L 1,-2000000085:Documentation/git-log.txt

does not error out and gives the history for the first line.

These are a variant of the previous case, with the difference that because of the '-' in <end> it is not an absolute line number but relative, and is interpreted as that many lines before <start> (i.e. in this case <start> actually means the end of the line range).

I think the same argument can be made about <end> pointing past the beginning of the file, though, arguably, it's not as useful, because the first line as always 1, and '-L 123,-99999' is more keystrokes than '-L 1,123'.

Quoted 1 line

So I think that it’s really only regex that must match...

#11 lore raw

Re: [PATCH 2/2] Documentation/git-log: mention that line-log regex must match in starting revision

In reply to Derrick Stolee
Quoted 33 lines · Derrick Stolee
Quoted 6 lines · Philippe Blain
Quoted 6 lines · Junio C Hamano

Even when you specify <start> or <end> as a line number, they must exist in the starting revision or it would be a fatal error. If we are clarifying with this patch for completeness, I think we should also mention it together.

...

Quoted 3 lines
    git log -L 300,2000000085:Documentation/git-log.txt

errors out:
fatal: file Documentation/git-log.txt has only 239 lines

This case seems important enough to include what it means to "match".

Specifically, the range must match at least one line in the file.

Quoted 5 lines

But

    git log -L 300,-2000000085:Documentation/git-log.txt
does not error out and shows the history for the whole file. Also,
    git log -L 1,-2000000085:Documentation/git-log.txt

does not error out and gives the history for the first line.

Negative numbers in the ranges are a bit strange to include, and the large magnitude you include here seems unnecessary for the test. However, it appears that we do store signed values in the line-log machinery:

/* A range [start,end]. Lines are numbered starting at 0, and the
* ranges include start but exclude end. */

struct range {
	long start, end;
};
Quoted 7 lines

Perhaps these should be replaced with an unsigned value instead, just to be clear that negative values do not make sense in this context?

Or rather, do we want to enforce start < end if they are to be valuable? We apparently do assert() this in range_set_check_invariants() for all that does.

We have different contexts for start,end here: the '-L <start>,<end>' specified on the command line and the 'start' and 'end' fields of the internal 'struct range' are not the same and different constraints apply to them.

'-L X,+Y' and '-L X,-Y' are explicitly allowed, and mean that Y is interpreted relative to X, the former meaning Y lines starting at X, while the latter meaning Y lines ending at X. These are interpreted while parsing the '-L ...' options, during which we do pass around negative values in (signed) long parameters, see line-range.c:parse_loc(). (though, arguably, having unsigned types and a separate flag parameter might make the code more clear)

The purpose of range_set_check_invariants() is different: as the file's content changes over its history the internal line ranges must be adjusted (grown, shrunk, shifted around or sometimes even merged) accordingly, and this function makes sure that the resulting 'struct range' instances after these adjustments still make sense, because the rest of the line log machinery expects sorted and non-overlapping line ranges.

Quoted 10 lines

The current behavior of showing only the first line is a strange byproduct of allowing these odd ranges, but we may want to keep it for consistency. That would imply we allow start <= end, and auto-correct these negative values to have end = start. (This does not fix the 300,-1 case, but that is strange enough to be considered a bug, right?)

Thanks,
-Stolee

#12 lore raw

Re: [PATCH 2/2] Documentation/git-log: mention that line-log regex must match in starting revision

In reply to Derrick Stolee
Quoted 7 lines · Derrick Stolee
Quoted 6 lines · Philippe Blain
Quoted 6 lines · Junio C Hamano

Even when you specify <start> or <end> as a line number, they must exist in the starting revision or it would be a fatal error. If we are clarifying with this patch for completeness, I think we should also mention it together.

Quoted 6 lines

...

Quoted 3 lines
    git log -L 300,2000000085:Documentation/git-log.txt

errors out:
fatal: file Documentation/git-log.txt has only 239 lines

This case seems important enough to include what it means to "match".

Yup, that is why I suggested not to focus too much on regexp and "match"-ing, but express it in terms of the lines that are specified by given arguments to "exist". The word "match" gives an incorrect connotation that the description in the paragraph that uses the verb applies only to regexp.

author
#15 lore raw

v2 cover Improve line log documentation

In reply to Philippe Blain via GitGitGadget

These two patches add more information to the documentation for line history (git log -L) :

* Mention explicitly that only the --patch and --no-patch diff formats are supported
* Mention that parameters <start>, <end> and <funcname> must exist in the starting revision

Regarding point 2 : I stumbled upon this while reading the history of 'filter_refs' in fetch-pack.c. I did

git log -s -L :filter_refs:fetch-pack.c

which shows a list of commits starting with 745f7a8cac (fetch-pack: move core code to libgit.a, 2012-10-26). So then I did

git log -s -L :filter_refs:fetch-pack.c -L :filter_refs:builtin/fetch-pack.c

which fails with

fatal: -L parameter 'filter_refs' starting at line 1: no match

Changes since v1:

* Shorten the commit titles
* Use long options names for --patch and --no-patch
* Remove inexact mention of --function-context
* Reword the clarification regarding <start>, <end> and <funcname> so that it doesn't apply only to regex parameters
* Also add this clarification to Documentation/gitk.txt since it applies there as well

Philippe Blain (2):
doc: log, gitk: document accepted line-log diff formats
doc: log, gitk: line-log arguments must exist in starting revision

 Documentation/git-log.txt | 8 ++++++--
 Documentation/gitk.txt    | 8 ++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

base-commit: ad05a3d8e5a6a06443836b5e40434262d992889a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-494%2Fphil-blain%2Fline-log-doc-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-494/phil-blain/line-log-doc-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/494

Range-diff vs v1:

1: 0ed04a8629 ! 1: e68ef0ced3 Documentation/git-log: document accepted line-log diff formats
@@ -1,18 +1,16 @@
Author: Philippe Blain <[email protected]>

- Documentation/git-log: document accepted line-log diff formats
+ doc: log, gitk: document accepted line-log diff formats

Currently the line-log functionality (git log -L) only supports
- displaying patch output (-p, its default behavior) and suppressing it
- (-s). A check was added in the code to that effect in 5314efaea (line-log:
- detect unsupported formats, 2019-03-10) but the documentation was not
+ displaying patch output (-p | --patch, its default behavior) and suppressing it
+ (-s | --no-patch). A check was added in the code to that effect in 5314efaea
+ (line-log: detect unsupported formats, 2019-03-10) but the documentation was not
updated.

Explicitly mention that -L implies -p, that patch output can be suppressed using -s, and that all other diff formats are not allowed.

- Additionnally, mention that the ':<funcname>' form implies --function-context.
-

Signed-off-by Philippe Blain <[email protected]>

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
@@ -24,8 +22,7 @@
give zero or one positive revision arguments.
- You can specify this option more than once.
+ You can specify this option more than once. Implies --patch.
-+ If ``:<funcname>'' is given, implies --function-context.
-+ Patch output can be suppressed using -s, but other diff formats
++ Patch output can be suppressed using --no-patch, but other diff formats
+ (namely --raw, --numstat, --shortstat, --dirstat, --summary,
+ --name-only, --name-status, --check) are not currently implemented.
+
@@ -41,8 +38,7 @@
give zero or one positive revision arguments.
- You can specify this option more than once.
+ You can specify this option more than once. Implies --patch.
-+ If ``:<funcname>'' is given, implies --function-context.
-+ Patch output can be suppressed using -s, but other diff formats
++ Patch output can be suppressed using --no-patch, but other diff formats
+ (namely --raw, --numstat, --shortstat, --dirstat, --summary,
+ --name-only, --name-status, --check) are not currently implemented.
+
2: 4ea4eeae0c ! 2: c7d489794c Documentation/git-log: mention that line-log regex must match in starting revision
@@ -1,10 +1,10 @@
Author: Philippe Blain <[email protected]>

- Documentation/git-log: mention that line-log regex must match in starting revision
+ doc: log, gitk: line-log arguments must exist in starting revision

- When giving a regex as parameter <start> or <end> in
- git log -L <start>,<end>:<file>, or a function name in
- git log -L :<funcname>:<file>, the given regex must match in the starting
+ The line number, regex or offset parameters <start> and <end> in
+ git log -L <start>,<end>:<file>, or the function name regex in
+ git log -L :<funcname>:<file> must exist in the starting
revision, or else the command exits with a fatal error.

This is not obvious in the documentation, so add a note to that
@@ -20,8 +20,22 @@
not give any pathspec limiters. This is currently limited to a walk starting from a single revision, i.e., you may only
- give zero or one positive revision arguments.
-+ give zero or one positive revision arguments, and any given regex for
-+ <start> or <end> (or <funcname>) must match in the starting revision.
++ give zero or one positive revision arguments, and
++ <start> and <end> (or <funcname>) must exist in the starting revision.
You can specify this option more than once. Implies --patch.
- If ``:<funcname>'' is given, implies --function-context.
- Patch output can be suppressed using -s, but other diff formats
+ Patch output can be suppressed using --no-patch, but other diff formats
+ (namely --raw, --numstat, --shortstat, --dirstat, --summary,
+
+ diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
+ --- a/Documentation/gitk.txt
+ +++ b/Documentation/gitk.txt
+@@
+ (or the function name regex <funcname>) within the <file>. You may
+ not give any pathspec limiters. This is currently limited to
+ a walk starting from a single revision, i.e., you may only
+- give zero or one positive revision arguments.
++ give zero or one positive revision arguments, and
++ <start> and <end> (or <funcname>) must exist in the starting revision.
+ You can specify this option more than once. Implies --patch.
+ Patch output can be suppressed using --no-patch, but other diff formats
+ (namely --raw, --numstat, --shortstat, --dirstat, --summary,

Signature · 1 line
gitgitgadget
author
#16 lore raw

PATCH v2 1/2 doc: log, gitk: document accepted line-log diff formats

From: Philippe Blain <[email protected]>

Currently the line-log functionality (git log -L) only supports displaying patch output (-p | --patch, its default behavior) and suppressing it (-s | --no-patch). A check was added in the code to that effect in 5314efaea (line-log: detect unsupported formats, 2019-03-10) but the documentation was not updated.

Explicitly mention that -L implies -p, that patch output can be suppressed using -s, and that all other diff formats are not allowed.

Signed-off-by Philippe Blain <[email protected]>
---
 Documentation/git-log.txt | 5 ++++-
 Documentation/gitk.txt    | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)
Documentation/git-log.txt +4 -1
index b406bc4c48..6574c0d91f 100644
@@ -77,7 +77,10 @@ produced by `--stat`, etc.
 	not give any pathspec limiters.  This is currently limited to
 	a walk starting from a single revision, i.e., you may only
 	give zero or one positive revision arguments.
-	You can specify this option more than once.
+	You can specify this option more than once. Implies `--patch`.
+	Patch output can be suppressed using `--no-patch`, but other diff formats
+	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
+	`--name-only`, `--name-status`, `--check`) are not currently implemented.
 +
 include::line-range-format.txt[]
Documentation/gitk.txt +4 -1
index 1eabb0aaf3..c487af5b74 100644
@@ -106,7 +106,10 @@ linkgit:git-rev-list[1] for a complete list.
 	not give any pathspec limiters.  This is currently limited to
 	a walk starting from a single revision, i.e., you may only
 	give zero or one positive revision arguments.
-	You can specify this option more than once.
+	You can specify this option more than once. Implies `--patch`.
+	Patch output can be suppressed using `--no-patch`, but other diff formats
+	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
+	`--name-only`, `--name-status`, `--check`) are not currently implemented.
 +
 *Note:* gitk (unlike linkgit:git-log[1]) currently only understands
 this option if you specify it "glued together" with its argument.  Do
Signature · 1 line
gitgitgadget
author
#17 lore raw

PATCH v2 2/2 doc: log, gitk: line-log arguments must exist in starting revision

In reply to Philippe Blain via GitGitGadget

From: Philippe Blain <[email protected]>

The line number, regex or offset parameters <start> and <end> in git log -L <start>,<end>:<file>, or the function name regex in git log -L :<funcname>:<file> must exist in the starting revision, or else the command exits with a fatal error.

This is not obvious in the documentation, so add a note to that effect.

Signed-off-by Philippe Blain <[email protected]>
---
 Documentation/git-log.txt | 3 ++-
 Documentation/gitk.txt    | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)
Documentation/git-log.txt +2 -1
index 6574c0d91f..bed09bb09e 100644
@@ -76,7 +76,8 @@ produced by `--stat`, etc.
 	(or the function name regex <funcname>) within the <file>.  You may
 	not give any pathspec limiters.  This is currently limited to
 	a walk starting from a single revision, i.e., you may only
-	give zero or one positive revision arguments.
+	give zero or one positive revision arguments, and
+	<start> and <end> (or <funcname>) must exist in the starting revision.
 	You can specify this option more than once. Implies `--patch`.
 	Patch output can be suppressed using `--no-patch`, but other diff formats
 	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
Documentation/gitk.txt +2 -1
index c487af5b74..c653ebb6a8 100644
@@ -105,7 +105,8 @@ linkgit:git-rev-list[1] for a complete list.
 	(or the function name regex <funcname>) within the <file>.  You may
 	not give any pathspec limiters.  This is currently limited to
 	a walk starting from a single revision, i.e., you may only
-	give zero or one positive revision arguments.
+	give zero or one positive revision arguments, and
+	<start> and <end> (or <funcname>) must exist in the starting revision.
 	You can specify this option more than once. Implies `--patch`.
 	Patch output can be suppressed using `--no-patch`, but other diff formats
 	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
Signature · 1 line
gitgitgadget
#13 lore raw

Re: [PATCH 2/2] Documentation/git-log: mention that line-log regex must match in starting revision

In reply to Junio C Hamano
Quoted 8 lines · Junio C Hamano

...
Yup, that is why I suggested not to focus too much on regexp and "match"-ing, but express it in terms of the lines that are specified by given arguments to "exist". The word "match" gives an incorrect connotation that the description in the paragraph that uses the verb applies only to regexp.

I agree. Thanks for the suggestion. I changed it to « exist » in v2.