On Thu, Apr 27, 2023 at 09:20:53AM -0700, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@xxxxxxxxx> writes: > > >> I did also allow "-" here, which may be controversial, as we don't > >> currently support negative timestamps. My reasoning was two-fold. One, > >> the design of parse_timestamp() is such that we should be able to easily > >> switch it to handling signed values, and this otherwise creates a > >> hard-to-find gotcha that anybody doing that work would get tripped up > >> on. And two, the status quo is that we currently parse them, though the > >> result of course ends up as a very large unsigned value (which is likely > >> to just get clamped to "0" for display anyway, since our date routines > >> can't handle it). > > > > I think this makes a good case for accepting '-'. The commit message > > is well explained as always :-) This all looks good to me apart from a > > query about one of the tests. > > I agree. I was somewhat surprised that the big comment before that > code did not mention it, but hopefully those who would be tempted to > remove the check for '-' would either be careful enough themselves > or be stopped by reviewers who are careful enough to go back to the > log message of the commit that added the check in the first place, > so it is OK. Hmph, I thought I did write something in that comment. But I think in the end I just migrated it to the commit message (and skimming my reflog I don't think it even made it as far as a commit). I think it's OK. I was mostly trying to help out Future Peff, who has a series almost completed to handle negative timestamps. But I think the worst case is that the tests in that new series would fail, and I'd figure it out. :) -Peff
