Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 26, 2021, 19:25 UTC
- Message-ID
- <xmqqzh0v1ox4.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20210125212525.dpnsj7ejngvpkd5y@pengutronix.de>
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
Show 9 quoted lines
>> > My POV is that if it's easy to use the same function (and so the same >> > set of range descriptors) for git log and git range-diff then do so. >> > This yields a consistent behaviour which is IMHO better than preventing >> > people to do things that are considered strange today. >> >> ... I am OK with that point of view. It certainly is simpler to >> explain to end users. > > It seems you understood my argument :-)
So it seems ;-).
Show 26 quoted lines
>> Having said that, it would make it much harder to implement >> efficiently, though. For example, when your user says >> >> git range-diff A B >> >> to compare "git log A" (all the way down to the root) and "git log >> B" (likewise), you'd certainly optimize the older common part of the >> history out, essentially turning it into >> >> git range-diff A..B B..A >> >> or its moral equivalent >> >> git range-diff A...B >> >> But you cannot apply such an optimization blindly. When the user >> gives A..B and B..A as two args, you somehow need to notice that >> you shouldn't rewrite it to "A..B...B..A", and for that, you'd still >> need some "parsing" of these args. > > I agree that for a long history > > git range-diff A B > > is an expensive request and I wouldn't invest too many cycles optimizing > it.
Well, your devil's advocate can argue that accepting an input that can easily make the tool unusable would be irresponsible, though.
And there are two possible ways out:
(1) declare that optimizing "A B" into "A...B" is way too difficult
to do in general, and find a good enough way to see if A and B
individually gives a "range" that should be manageable; or(2) invest cycles to optimize, so your users do not have to care.
I think the series takes the former approach, and I find it understandable.
It is a different matter if the way found and implemented in the patch is "good enough" to tell if a given argument names a manageable range. You said something about "rev^{/^here are two..dots}" that can be mistaken as a "good enough" range, but it actually names a revision and all the way down to the root.
> (And if I'd optimize it, it wouldn't be done using textual > combination of the two strings but by checking if the two ranges > intersect.
Yup, that is in line with what I mumbled earlier about setup_revisions() and inspecting the rev_info.pending, I think.
Show 10 quoted lines
>> So, I dunno. Limiting the second form to only forms that the >> implementation does not have to do such optimization would certainly >> make it simpler for Dscho to implement ;-) > > I don't want to make it more complicated for Dscho, I'm happy if I can > in the near future use range-diff with $rev1^! $ref2^! . So feel free to > ignore me. > > Best regards > Uwe