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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Dec 18, 2019, 11:49 UTC
Message-ID: <20191218114954.GC8609@szeder.dev>
In-Reply-To: <d8e753ec-2835-331f-99de-35a4a5369b48@gmail.com>

On Wed, Dec 18, 2019 at 05:55:25AM -0500, Derrick Stolee wrote:

Show 40 quoted lines

> On 12/17/2019 10:28 PM, Philippe Blain wrote:
> > 
> >> Le 17 déc. 2019 à 13:16, Junio C Hamano <gitster@pobox.com> 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.

Show 10 quoted 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
>