{"thread":{"id":"59566","subject":"[PATCH] date: remove approxidate_relative()","startedAt":"2023-04-08T09:35:33Z","lastAt":"2023-04-12T07:30:06Z","messageCount":9,"participants":["René Scharfe","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"475033","messageId":"f5b9a290-7cec-7a83-660b-e15494d2cdc8@web.de","threadId":"59566","inReplyTo":null,"subject":"[PATCH] date: remove approxidate_relative()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-04-08T09:35:24Z","receivedAt":"2023-04-08T09:35:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"When 29f4332e66 (Quit passing 'now' to date code, 2019-09-11) removed\nits timeval parameter, approxidate_relative() became equivalent to\napproxidate(). Convert its last two call sites and remove the redundant\nfunction.\n\nSigned-off-by: René Scharfe \n---\nFormatted with -U16 for easier review. Only useful in date.c, but\ncannot be restricted to just one file.\n\n date.c | 14 --------------\n date.h | 1 -\n t/helper/test-date.c | 4 ++--\n 3 files changed, 2 insertions(+), 17 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 1fb2cd1b53..7c8650f799 100644\n--- a/date.c\n+++ b/date.c\n@@ -1353,46 +1353,32 @@ static timestamp_t approxidate_str(const char *date,\n \t\tdate++;\n \t\tif (isdigit(c)) {\n \t\t\tpending_number(&tm, &number);\n \t\t\tdate = approxidate_digit(date-1, &tm, &number, time_sec);\n \t\t\ttouched = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (isalpha(c))\n \t\t\tdate = approxidate_alpha(date-1, &tm, &now, &number, &touched);\n \t}\n \tpending_number(&tm, &number);\n \tif (!touched)\n \t\t*error_ret = 1;\n \treturn (timestamp_t)update_tm(&tm, &now, 0);\n }\n\n-timestamp_t approxidate_relative(const char *date)\n-{\n-\tstruct timeval tv;\n-\ttimestamp_t timestamp;\n-\tint offset;\n-\tint errors = 0;\n-\n-\tif (!parse_date_basic(date, ×tamp, &offset))\n-\t\treturn timestamp;\n-\n-\tget_time(&tv);\n-\treturn approxidate_str(date, (const struct timeval *) &tv, &errors);\n-}\n-\n timestamp_t approxidate_careful(const char *date, int *error_ret)\n {\n \tstruct timeval tv;\n \ttimestamp_t timestamp;\n \tint offset;\n \tint dummy = 0;\n \tif (!error_ret)\n \t\terror_ret = &dummy;\n\n \tif (!parse_date_basic(date, ×tamp, &offset)) {\n \t\t*error_ret = 0;\n \t\treturn timestamp;\n \t}\n\n \tget_time(&tv);\n \treturn approxidate_str(date, &tv, error_ret);\ndiff --git a/date.h b/date.h\nindex 5d4eaba0a9..6136212a19 100644\n--- a/date.h\n+++ b/date.h\n@@ -55,20 +55,19 @@ const char *show_date(timestamp_t time, int timezone, const struct date_mode *mo\n */\n void parse_date_format(const char *format, struct date_mode *mode);\n\n /**\n * Release a \"struct date_mode\", currently only required if\n * parse_date_format() has parsed a \"DATE_STRFTIME\" format.\n */\n void date_mode_release(struct date_mode *mode);\n\n void show_date_relative(timestamp_t time, struct strbuf *timebuf);\n int parse_date(const char *date, struct strbuf *out);\n int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset);\n int parse_expiry_date(const char *date, timestamp_t *timestamp);\n void datestamp(struct strbuf *out);\n #define approxidate(s) approxidate_careful((s), NULL)\n timestamp_t approxidate_careful(const char *, int *);\n-timestamp_t approxidate_relative(const char *date);\n int date_overflows(timestamp_t date);\n time_t tm_to_time_t(const struct tm *tm);\n #endif\ndiff --git a/t/helper/test-date.c b/t/helper/test-date.c\nindex cd6a6df702..2d9232cc68 100644\n--- a/t/helper/test-date.c\n+++ b/t/helper/test-date.c\n@@ -68,42 +68,42 @@ static void parse_dates(const char **argv)\n\n \t\tstrbuf_reset(&result);\n \t\tparse_date(*argv, &result);\n \t\tif (sscanf(result.buf, \"%\"PRItime\" %d\", &t, &tz) == 2)\n \t\t\tprintf(\"%s -> %s\\n\",\n \t\t\t *argv, show_date(t, tz, DATE_MODE(ISO8601)));\n \t\telse\n \t\t\tprintf(\"%s -> bad\\n\", *argv);\n \t}\n \tstrbuf_release(&result);\n }\n\n static void parse_approxidate(const char **argv)\n {\n \tfor (; *argv; argv++) {\n \t\ttimestamp_t t;\n-\t\tt = approxidate_relative(*argv);\n+\t\tt = approxidate(*argv);\n \t\tprintf(\"%s -> %s\\n\", *argv, show_date(t, 0, DATE_MODE(ISO8601)));\n \t}\n }\n\n static void parse_approx_timestamp(const char **argv)\n {\n \tfor (; *argv; argv++) {\n \t\ttimestamp_t t;\n-\t\tt = approxidate_relative(*argv);\n+\t\tt = approxidate(*argv);\n \t\tprintf(\"%s -> %\"PRItime\"\\n\", *argv, t);\n \t}\n }\n\n static void getnanos(const char **argv)\n {\n \tdouble seconds = getnanotime() / 1.0e9;\n\n \tif (*argv)\n \t\tseconds -= strtod(*argv, NULL);\n \tprintf(\"%lf\\n\", seconds);\n }\n\n int cmd__date(int argc UNUSED, const char **argv)\n {\n \tconst char *x;\n--\n2.40.0\n"},{"id":"475066","messageId":"xmqqjzyjemji.fsf@gitster.g","threadId":"59566","inReplyTo":"f5b9a290-7cec-7a83-660b-e15494d2cdc8@web.de","subject":"Re: [PATCH] date: remove approxidate_relative()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-10T16:20:49Z","receivedAt":"2023-04-10T16:20:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> When 29f4332e66 (Quit passing 'now' to date code, 2019-09-11) removed\n> its timeval parameter, approxidate_relative() became equivalent to\n> approxidate(). Convert its last two call sites and remove the redundant\n> function.\n>\n> Signed-off-by: René Scharfe \n> ---\n\nWe might keep a backward compatibility definition or comment to help\nmigrating in-flight (or pre-flight) callers if this were a function\nmore often used, but seeing that the only remaining two callers are\nin the test helper, I agree the simplicity of straight removal like\nthis patch does is appropriate.\n\nThanks, will queue.\n\n> Formatted with -U16 for easier review. Only useful in date.c, but\n> cannot be restricted to just one file.\n\nPerhaps\n\n\t$ git -c format-patch.date.c.context=16 format-patch -1\n\nwould be a workable idea? I do not think it makes any sense to say\n\"when taking a diff for this path, always use 16 lines of context\"\nso it should not be stored in a file to be used every time, like\n\n\t[format-patch \"date.c\"] context = 16\n\nand it makes even less sense to say that all project participants\nshould use this context, always, so making it an attribute would be\neven less appropriate.\n\nBut this was the easiest way to prototype ;-) If this turns out to\nbe useful, the real version should probably be done by:\n\n * designing a command line option format, e.g.\n\n --per-path-context=date.c:16\n\n * instead of adding an extra \"int ctxlen\" parameter to the call\n path(s), add a ctxlen_map that maps path to ctxlen we read from\n the command line options to the diff_options structure.\n\n * In builtin_diff() where xecfg.ctxlen is assigned to, consult\n o->ctxlen_map and use o->context as a fallback.\n\nor something along that line, I guess.\n\n diff.c | 18 ++++++++++++++++--\n 1 file changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git c/diff.c w/diff.c\nindex 469e18aed2..cbfbb4af96 100644\n--- c/diff.c\n+++ w/diff.c\n@@ -3460,6 +3460,7 @@ static void builtin_diff(const char *name_a,\n \t\t\t const char *xfrm_msg,\n \t\t\t int must_show_header,\n \t\t\t struct diff_options *o,\n+\t\t\t int ctxlen,\n \t\t\t int complete_rewrite)\n {\n \tmmfile_t mf1, mf2;\n@@ -3657,7 +3658,7 @@ static void builtin_diff(const char *name_a,\n \t\txpp.ignore_regex_nr = o->ignore_regex_nr;\n \t\txpp.anchors = o->anchors;\n \t\txpp.anchors_nr = o->anchors_nr;\n-\t\txecfg.ctxlen = o->context;\n+\t\txecfg.ctxlen = (ctxlen < 0) ? o->context : ctxlen;\n \t\txecfg.interhunkctxlen = o->interhunkcontext;\n \t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n \t\tif (o->flags.funccontext)\n@@ -4443,6 +4444,17 @@ static void fill_metainfo(struct strbuf *msg,\n \t}\n }\n \n+static int configured_ctxlen(const char *path)\n+{\n+\tstruct strbuf key = STRBUF_INIT;\n+\tint ctxlen;\n+\n+\tstrbuf_addf(&key, \"format-patch.%s.context\", path);\n+\tif (!repo_config_get_int(the_repository, key.buf, &ctxlen))\n+\t\treturn ctxlen;\n+\treturn -1;\n+}\n+\n static void run_diff_cmd(const char *pgm,\n \t\t\t const char *name,\n \t\t\t const char *other,\n@@ -4480,12 +4492,14 @@ static void run_diff_cmd(const char *pgm,\n \t\treturn;\n \t}\n \tif (one && two) {\n+\t\tint ctxlen = configured_ctxlen(attr_path);\n+\n \t\tif (!o->ignore_driver_algorithm && drv && drv->algorithm)\n \t\t\tset_diff_algorithm(o, drv->algorithm);\n \n \t\tbuiltin_diff(name, other ? other : name,\n \t\t\t one, two, xfrm_msg, must_show_header,\n-\t\t\t o, complete_rewrite);\n+\t\t\t o, ctxlen, complete_rewrite);\n \t} else {\n \t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n \t}\n\n"},{"id":"475082","messageId":"20230410202536.GE104097@coredump.intra.peff.net","threadId":"59566","inReplyTo":"xmqqjzyjemji.fsf@gitster.g","subject":"Re: [PATCH] date: remove approxidate_relative()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-10T20:25:36Z","receivedAt":"2023-04-10T20:25:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 10, 2023 at 09:20:49AM -0700, Junio C Hamano wrote:\n\n> Perhaps\n> \n> \t$ git -c format-patch.date.c.context=16 format-patch -1\n> \n> would be a workable idea? I do not think it makes any sense to say\n> \"when taking a diff for this path, always use 16 lines of context\"\n> so it should not be stored in a file to be used every time, like\n> \n> \t[format-patch \"date.c\"] context = 16\n> \n> and it makes even less sense to say that all project participants\n> should use this context, always, so making it an attribute would be\n> even less appropriate.\n> \n> But this was the easiest way to prototype ;-) If this turns out to\n> be useful, the real version should probably be done by:\n\nCute. It feels like this only goes half-way, though. You really want\nper-hunk configurable context. This particular patch was just lucky that\nthere was only one hunk in the date.c file.\n\nI'm not sure of the least-confusing way to address a single hunk, though\n(by line number is one option, by hunk-number within the patch is\nanother). I suspect the best workflow for a user would be to\ninteractively say \"show me more context for this hunk\". Some viewers\nhave support for that (e.g., GitHub's web view of a diff). But handling\nthat for a one-shot CLI program is tricky, not to mention then feeding\nit back to format-patch to generate the output you want to send. :)\n\n-Peff\n"},{"id":"475083","messageId":"b17e05d5-deda-527f-1587-9f1cd7046a0b@web.de","threadId":"59566","inReplyTo":"20230410202536.GE104097@coredump.intra.peff.net","subject":"Re: [PATCH] date: remove approxidate_relative()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-04-10T20:52:01Z","receivedAt":"2023-04-10T20:52:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.04.23 um 22:25 schrieb Jeff King:\n>\n> I'm not sure of the least-confusing way to address a single hunk, though\n> (by line number is one option, by hunk-number within the patch is\n> another). I suspect the best workflow for a user would be to\n> interactively say \"show me more context for this hunk\". Some viewers\n> have support for that (e.g., GitHub's web view of a diff). But handling\n> that for a one-shot CLI program is tricky, not to mention then feeding\n> it back to format-patch to generate the output you want to send. :)\n\nSo basically you propose a format-patch --interactive mode that shows\neach hunk and allows extending its context. This could work. For hunks\nthat span multiple screens it might be a bit iffy -- or perhaps not, if\nthe scrollback buffer of the terminal or console is big enough.\n\nRené\n"},{"id":"475084","messageId":"xmqqy1mzcus6.fsf@gitster.g","threadId":"59566","inReplyTo":"20230410202536.GE104097@coredump.intra.peff.net","subject":"Re: [PATCH] date: remove approxidate_relative()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-10T21:05:45Z","receivedAt":"2023-04-10T21:05:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Cute. It feels like this only goes half-way, though. You really want\n> per-hunk configurable context. This particular patch was just lucky that\n> there was only one hunk in the date.c file.\n\n\"-U16\" extends the context lines in both directions by the same\nnumber of lines, but most likely you need to extend asymmetrically.\nIn René's patch, for example, much of the body of approxidate_str()\nis visible in the precontext of the hunk to remove\napproxidate_relative(), but the body of that function is irrelevant.\nWhat he wanted to show was the body of approxidate_careful() and the\nsize of that function is where the -U16 came from.\n\nInstead, imagine --extra-context=':' were the way to\ntell Git to include the specified range of lines in the post context\neven though they may not have been modified. Then René's patch\ncould have been produced with\n\n $ git format-patch -1 \\\n --extra-context='/^timestamp_t approxidate_careful/,/^}$/:date.c'\n\nand would have shown 3 lines of precontext before the removed\napproxidate_relative(), plus the unchanged approxidate_careful()\nfunction in full in the postcontext.\n"},{"id":"475133","messageId":"20230411092502.GA398350@coredump.intra.peff.net","threadId":"59566","inReplyTo":"b17e05d5-deda-527f-1587-9f1cd7046a0b@web.de","subject":"Re: [PATCH] date: remove approxidate_relative()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-11T09:25:02Z","receivedAt":"2023-04-11T09:25:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 10, 2023 at 10:52:01PM +0200, René Scharfe wrote:\n\n> Am 10.04.23 um 22:25 schrieb Jeff King:\n> >\n> > I'm not sure of the least-confusing way to address a single hunk, though\n> > (by line number is one option, by hunk-number within the patch is\n> > another). I suspect the best workflow for a user would be to\n> > interactively say \"show me more context for this hunk\". Some viewers\n> > have support for that (e.g., GitHub's web view of a diff). But handling\n> > that for a one-shot CLI program is tricky, not to mention then feeding\n> > it back to format-patch to generate the output you want to send. :)\n> \n> So basically you propose a format-patch --interactive mode that shows\n> each hunk and allows extending its context. This could work. For hunks\n> that span multiple screens it might be a bit iffy -- or perhaps not, if\n> the scrollback buffer of the terminal or console is big enough.\n\nWell, less proposing and more thinking out loud. :)\n\nOne thing that I think would make it awkward is that some people (well,\nme at least) tend to show diffs repeatedly. So I might look at a diff\nseveral times, and then even run format-patch multiple times as part of\nmy workflow of sending it. Interactively expanding the diff each time,\nand then losing that result, would be annoying.\n\nSo an interactive tool that somehow output \"oh, and here are some\nparameters you can feed back to get the same view\" would be my ideal.\n\n-Peff\n"},{"id":"475134","messageId":"20230411093005.GB398350@coredump.intra.peff.net","threadId":"59566","inReplyTo":"xmqqy1mzcus6.fsf@gitster.g","subject":"Re: [PATCH] date: remove approxidate_relative()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-11T09:30:05Z","receivedAt":"2023-04-11T09:31:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 10, 2023 at 02:05:45PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Cute. It feels like this only goes half-way, though. You really want\n> > per-hunk configurable context. This particular patch was just lucky that\n> > there was only one hunk in the date.c file.\n> \n> \"-U16\" extends the context lines in both directions by the same\n> number of lines, but most likely you need to extend asymmetrically.\n\nGood point. This is sort of like grep's \"-C\" along with \"-A\" and \"-B\".\nAll of which I generally find somewhat awkward. :)\n\n> In René's patch, for example, much of the body of approxidate_str()\n> is visible in the precontext of the hunk to remove\n> approxidate_relative(), but the body of that function is irrelevant.\n> What he wanted to show was the body of approxidate_careful() and the\n> size of that function is where the -U16 came from.\n> \n> Instead, imagine --extra-context=':' were the way to\n> tell Git to include the specified range of lines in the post context\n> even though they may not have been modified. Then René's patch\n> could have been produced with\n> \n> $ git format-patch -1 \\\n> --extra-context='/^timestamp_t approxidate_careful/,/^}$/:date.c'\n> \n> and would have shown 3 lines of precontext before the removed\n> approxidate_relative(), plus the unchanged approxidate_careful()\n> function in full in the postcontext.\n\nOoh, I like that very much. In that sense it really feels like an\nextension of --function-context. Would the regexes be searches starting\nfrom the edge of some context (as they more or less are under the hood\nfor function context), or would you search within the whole file for\nranges (and then presumably use them when a hunk's context is adjacent\nto or overlaps a range)?\n\nIf the latter, I guess you could also allow both absolute and relative\nline numbers, similar to how \"-L\" accepts range input.\n\n-Peff\n"},{"id":"475148","messageId":"xmqqsfd6bf0s.fsf@gitster.g","threadId":"59566","inReplyTo":"20230411093005.GB398350@coredump.intra.peff.net","subject":"Re: [PATCH] date: remove approxidate_relative()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-11T15:43:47Z","receivedAt":"2023-04-11T15:43:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Instead, imagine --extra-context=':' were the way to\n>> tell Git to include the specified range of lines in the post context\n>> even though they may not have been modified. Then René's patch\n>> could have been produced with\n>> \n>> $ git format-patch -1 \\\n>> --extra-context='/^timestamp_t approxidate_careful/,/^}$/:date.c'\n>> \n>> and would have shown 3 lines of precontext before the removed\n>> approxidate_relative(), plus the unchanged approxidate_careful()\n>> function in full in the postcontext.\n>\n> Ooh, I like that very much. In that sense it really feels like an\n> extension of --function-context. Would the regexes be searches starting\n> from the edge of some context (as they more or less are under the hood\n> for function context), or would you search within the whole file for\n> ranges (and then presumably use them when a hunk's context is adjacent\n> to or overlaps a range)?\n>\n> If the latter, I guess you could also allow both absolute and relative\n> line numbers, similar to how \"-L\" accepts range input.\n\nWe want the latter.\n\nIf we further imagine that approxidate_careful() were defined very\nfar away (in either direction) from approxidate_relative() that\n\"extending\" the patch context to show the removal of the latter to\ncover the former would show too much irrelevant information, I think\nRené would have wanted to show a normal patch plus an extra hunk\nthat contains the entirety of approxidate_careful() that shows no\nmodification (i.e. all lines are prefixed with an SP). The way I\nthink about this new \"feature\" is \"compute what hunks should be\nshown, honoring all other options. Then pretend no-op hunks to\ncover all specified lines in the postimage [*] are also in the\nresult. Combine them all, ignoring parts of the made-up no-op hunks\nwhen they contradict the real hunks.\". The end result should show\nall specified lines from the postimage plus the usual diff.\n\n\n[Footnote]\n\n * There is no need for a similar option to talk about lines in the\n preimage, because a line in the preimage would either appear in\n the postimage (in which case the range in the postimage can be\n used to show it), or otherwise it would appear as deleted line\n (in which case the reader will see it without the new feature.\n"},{"id":"475199","messageId":"20230412073001.GA1695531@coredump.intra.peff.net","threadId":"59566","inReplyTo":"xmqqsfd6bf0s.fsf@gitster.g","subject":"Re: [PATCH] date: remove approxidate_relative()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T07:30:01Z","receivedAt":"2023-04-12T07:30:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2023 at 08:43:47AM -0700, Junio C Hamano wrote:\n\n> > Ooh, I like that very much. In that sense it really feels like an\n> > extension of --function-context. Would the regexes be searches starting\n> > from the edge of some context (as they more or less are under the hood\n> > for function context), or would you search within the whole file for\n> > ranges (and then presumably use them when a hunk's context is adjacent\n> > to or overlaps a range)?\n> >\n> > If the latter, I guess you could also allow both absolute and relative\n> > line numbers, similar to how \"-L\" accepts range input.\n> \n> We want the latter.\n> \n> If we further imagine that approxidate_careful() were defined very\n> far away (in either direction) from approxidate_relative() that\n> \"extending\" the patch context to show the removal of the latter to\n> cover the former would show too much irrelevant information, I think\n> René would have wanted to show a normal patch plus an extra hunk\n> that contains the entirety of approxidate_careful() that shows no\n> modification (i.e. all lines are prefixed with an SP). The way I\n> think about this new \"feature\" is \"compute what hunks should be\n> shown, honoring all other options. Then pretend no-op hunks to\n> cover all specified lines in the postimage [*] are also in the\n> result. Combine them all, ignoring parts of the made-up no-op hunks\n> when they contradict the real hunks.\". The end result should show\n> all specified lines from the postimage plus the usual diff.\n\nRight, that makes perfect sense. Thanks for explaining.\n\nNow we just need somebody to implement it. ;)\n\n-Peff\n"}]}