On Wed, Dec 18, 2019 at 05:55:25AM -0500, Derrick Stolee wrote:
> On 12/17/2019 10:28 PM, Philippe Blain wrote:
> >
> >> Le 17 déc. 2019 à 13:16, Junio C Hamano <gitster@xxxxxxxxx> a écrit :
> >> 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.
> ...
> > 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.
>
> > 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.
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.
> 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
>
