Re: [WIP v2 1/2] Adding a record-time-zone command option for commit
- From
- Shengfa Lin <shengfa@google.com>
- Date
- Oct 26, 2020, 04:14 UTC
- Message-ID
- <20201026041443.1362933-1-shengfa@google.com>
- In-Reply-To
- <xmqqd01ans4h.fsf@gitster.c.googlers.com>
Show 16 quoted lines
>> Yes, we could check it in datestamp(), but ... >> >>> Initially, I thought this would be sufficient to show "-0000" in commit log >>> message. However, I found that the show_date function is used for "decoding"; >>> converting timestamp and tz to more readable format. Then I realize the >>> function won't distinguish between +0 and -0 as it only takes in a tz as >>> argument. As a result,... >> >> ... I would have imagined that you do not have to deal with all >> those complications if you don't hook this to such a low level of >> the call graph. That is why I wondered: >> ... > > Let me answer some of my puzzlement myself; that is, I would have > understood the change well if it were explained to me this way, and > if that explanation matched what the patches did ;-)
Yes, I agree.
Show 8 quoted lines
> The topic has two major parts. > > The code that prepares the timestamp to be recorded for the current > user, who wants to record an anonymous timezone "-0000", is one (and > the easier) part. And this part could be done all inside > ident_default_date() without touching anything in date.c; when we > need to call datestamp(), we are getting the current time for the > current user, so we can mask the timezone.
So for this part, there is no need to modify datestamp in dates.c. We could modify ident_default_date buf after datestamp to set the last 5 bytes to "-0000" using strcpy.
Show 8 quoted lines
> The other part is that we need to read the timestamp from existing > records, and if we choose to distinguish between timestamp in UTC > and timestamp with anonymous timezone, we'd need to devise a way to > encode the anonymous timezone differently. It is where the extra > bit that says "this bit does not usually mean anything but only when > the offset (which is a signed integer whose valid range is set to > between -2400 to +2400 by date.c::match_tz()) is zero, and this bit > is set, the zone is anonymous" comes in.
Yes, that's correct.
Show 5 quoted lines
> Side note. I suspect the damage to the callchain can be > limited much narrower if we didn't add this bit throughout > the API. What if we instead pick a number outside the valid > range of offsets, say -10000, as a sentinel value and passed > that throughout the code when we want an anonymous zone?
Good idea.
Show 9 quoted lines
> The functions in the callchain that care about the timezone > must understand how anonymous zone is encoded anyway, so to > them it's a matter of using an int plus one bit or using an > int that can have a special value. But other functions in > the callchain whose sole purpose (with respect to the > timezone information) is to pass it between their caller and > their callee as an opaque piece of data, using just a single > integer is much less error prone---the patch does not have > to touch them at all.
That would be easier to follow and requires less changes as well.
> Thanks.
Thanks for the clarification. Now I think we have a much better understanding. I will try to do a better job describing patches next time.