| Message ID | f5b9a290-7cec-7a83-660b-e15494d2cdc8@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | date: remove approxidate_relative() | expand |
René Scharfe <l.s.r@web.de> writes: > When 29f4332e66 (Quit passing 'now' to date code, 2019-09-11) removed > its timeval parameter, approxidate_relative() became equivalent to > approxidate(). Convert its last two call sites and remove the redundant > function. > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- We might keep a backward compatibility definition or comment to help migrating in-flight (or pre-flight) callers if this were a function more often used, but seeing that the only remaining two callers are in the test helper, I agree the simplicity of straight removal like this patch does is appropriate. Thanks, will queue. > Formatted with -U16 for easier review. Only useful in date.c, but > cannot be restricted to just one file. Perhaps $ git -c format-patch.date.c.context=16 format-patch -1 would be a workable idea? I do not think it makes any sense to say "when taking a diff for this path, always use 16 lines of context" so it should not be stored in a file to be used every time, like [format-patch "date.c"] context = 16 and it makes even less sense to say that all project participants should use this context, always, so making it an attribute would be even less appropriate. But this was the easiest way to prototype ;-) If this turns out to be useful, the real version should probably be done by: * designing a command line option format, e.g. --per-path-context=date.c:16 * instead of adding an extra "int ctxlen" parameter to the call path(s), add a ctxlen_map that maps path to ctxlen we read from the command line options to the diff_options structure. * In builtin_diff() where xecfg.ctxlen is assigned to, consult o->ctxlen_map and use o->context as a fallback. or something along that line, I guess. diff.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git c/diff.c w/diff.c index 469e18aed2..cbfbb4af96 100644 --- c/diff.c +++ w/diff.c @@ -3460,6 +3460,7 @@ static void builtin_diff(const char *name_a, const char *xfrm_msg, int must_show_header, struct diff_options *o, + int ctxlen, int complete_rewrite) { mmfile_t mf1, mf2; @@ -3657,7 +3658,7 @@ static void builtin_diff(const char *name_a, xpp.ignore_regex_nr = o->ignore_regex_nr; xpp.anchors = o->anchors; xpp.anchors_nr = o->anchors_nr; - xecfg.ctxlen = o->context; + xecfg.ctxlen = (ctxlen < 0) ? o->context : ctxlen; xecfg.interhunkctxlen = o->interhunkcontext; xecfg.flags = XDL_EMIT_FUNCNAMES; if (o->flags.funccontext) @@ -4443,6 +4444,17 @@ static void fill_metainfo(struct strbuf *msg, } } +static int configured_ctxlen(const char *path) +{ + struct strbuf key = STRBUF_INIT; + int ctxlen; + + strbuf_addf(&key, "format-patch.%s.context", path); + if (!repo_config_get_int(the_repository, key.buf, &ctxlen)) + return ctxlen; + return -1; +} + static void run_diff_cmd(const char *pgm, const char *name, const char *other, @@ -4480,12 +4492,14 @@ static void run_diff_cmd(const char *pgm, return; } if (one && two) { + int ctxlen = configured_ctxlen(attr_path); + if (!o->ignore_driver_algorithm && drv && drv->algorithm) set_diff_algorithm(o, drv->algorithm); builtin_diff(name, other ? other : name, one, two, xfrm_msg, must_show_header, - o, complete_rewrite); + o, ctxlen, complete_rewrite); } else { fprintf(o->file, "* Unmerged path %s\n", name); }
On Mon, Apr 10, 2023 at 09:20:49AM -0700, Junio C Hamano wrote: > Perhaps > > $ git -c format-patch.date.c.context=16 format-patch -1 > > would be a workable idea? I do not think it makes any sense to say > "when taking a diff for this path, always use 16 lines of context" > so it should not be stored in a file to be used every time, like > > [format-patch "date.c"] context = 16 > > and it makes even less sense to say that all project participants > should use this context, always, so making it an attribute would be > even less appropriate. > > But this was the easiest way to prototype ;-) If this turns out to > be useful, the real version should probably be done by: Cute. It feels like this only goes half-way, though. You really want per-hunk configurable context. This particular patch was just lucky that there was only one hunk in the date.c file. I'm not sure of the least-confusing way to address a single hunk, though (by line number is one option, by hunk-number within the patch is another). I suspect the best workflow for a user would be to interactively say "show me more context for this hunk". Some viewers have support for that (e.g., GitHub's web view of a diff). But handling that for a one-shot CLI program is tricky, not to mention then feeding it back to format-patch to generate the output you want to send. :) -Peff
Am 10.04.23 um 22:25 schrieb Jeff King: > > I'm not sure of the least-confusing way to address a single hunk, though > (by line number is one option, by hunk-number within the patch is > another). I suspect the best workflow for a user would be to > interactively say "show me more context for this hunk". Some viewers > have support for that (e.g., GitHub's web view of a diff). But handling > that for a one-shot CLI program is tricky, not to mention then feeding > it back to format-patch to generate the output you want to send. :) So basically you propose a format-patch --interactive mode that shows each hunk and allows extending its context. This could work. For hunks that span multiple screens it might be a bit iffy -- or perhaps not, if the scrollback buffer of the terminal or console is big enough. René
Jeff King <peff@peff.net> writes: > Cute. It feels like this only goes half-way, though. You really want > per-hunk configurable context. This particular patch was just lucky that > there was only one hunk in the date.c file. "-U16" extends the context lines in both directions by the same number of lines, but most likely you need to extend asymmetrically. In René's patch, for example, much of the body of approxidate_str() is visible in the precontext of the hunk to remove approxidate_relative(), but the body of that function is irrelevant. What he wanted to show was the body of approxidate_careful() and the size of that function is where the -U16 came from. Instead, imagine --extra-context='<range>:<path>' were the way to tell Git to include the specified range of lines in the post context even though they may not have been modified. Then René's patch could have been produced with $ git format-patch -1 \ --extra-context='/^timestamp_t approxidate_careful/,/^}$/:date.c' and would have shown 3 lines of precontext before the removed approxidate_relative(), plus the unchanged approxidate_careful() function in full in the postcontext.
On Mon, Apr 10, 2023 at 10:52:01PM +0200, René Scharfe wrote: > Am 10.04.23 um 22:25 schrieb Jeff King: > > > > I'm not sure of the least-confusing way to address a single hunk, though > > (by line number is one option, by hunk-number within the patch is > > another). I suspect the best workflow for a user would be to > > interactively say "show me more context for this hunk". Some viewers > > have support for that (e.g., GitHub's web view of a diff). But handling > > that for a one-shot CLI program is tricky, not to mention then feeding > > it back to format-patch to generate the output you want to send. :) > > So basically you propose a format-patch --interactive mode that shows > each hunk and allows extending its context. This could work. For hunks > that span multiple screens it might be a bit iffy -- or perhaps not, if > the scrollback buffer of the terminal or console is big enough. Well, less proposing and more thinking out loud. :) One thing that I think would make it awkward is that some people (well, me at least) tend to show diffs repeatedly. So I might look at a diff several times, and then even run format-patch multiple times as part of my workflow of sending it. Interactively expanding the diff each time, and then losing that result, would be annoying. So an interactive tool that somehow output "oh, and here are some parameters you can feed back to get the same view" would be my ideal. -Peff
On Mon, Apr 10, 2023 at 02:05:45PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Cute. It feels like this only goes half-way, though. You really want > > per-hunk configurable context. This particular patch was just lucky that > > there was only one hunk in the date.c file. > > "-U16" extends the context lines in both directions by the same > number of lines, but most likely you need to extend asymmetrically. Good point. This is sort of like grep's "-C" along with "-A" and "-B". All of which I generally find somewhat awkward. :) > In René's patch, for example, much of the body of approxidate_str() > is visible in the precontext of the hunk to remove > approxidate_relative(), but the body of that function is irrelevant. > What he wanted to show was the body of approxidate_careful() and the > size of that function is where the -U16 came from. > > Instead, imagine --extra-context='<range>:<path>' were the way to > tell Git to include the specified range of lines in the post context > even though they may not have been modified. Then René's patch > could have been produced with > > $ git format-patch -1 \ > --extra-context='/^timestamp_t approxidate_careful/,/^}$/:date.c' > > and would have shown 3 lines of precontext before the removed > approxidate_relative(), plus the unchanged approxidate_careful() > function in full in the postcontext. Ooh, I like that very much. In that sense it really feels like an extension of --function-context. Would the regexes be searches starting from the edge of some context (as they more or less are under the hood for function context), or would you search within the whole file for ranges (and then presumably use them when a hunk's context is adjacent to or overlaps a range)? If the latter, I guess you could also allow both absolute and relative line numbers, similar to how "-L" accepts range input. -Peff
Jeff King <peff@peff.net> writes: >> Instead, imagine --extra-context='<range>:<path>' were the way to >> tell Git to include the specified range of lines in the post context >> even though they may not have been modified. Then René's patch >> could have been produced with >> >> $ git format-patch -1 \ >> --extra-context='/^timestamp_t approxidate_careful/,/^}$/:date.c' >> >> and would have shown 3 lines of precontext before the removed >> approxidate_relative(), plus the unchanged approxidate_careful() >> function in full in the postcontext. > > Ooh, I like that very much. In that sense it really feels like an > extension of --function-context. Would the regexes be searches starting > from the edge of some context (as they more or less are under the hood > for function context), or would you search within the whole file for > ranges (and then presumably use them when a hunk's context is adjacent > to or overlaps a range)? > > If the latter, I guess you could also allow both absolute and relative > line numbers, similar to how "-L" accepts range input. We want the latter. If we further imagine that approxidate_careful() were defined very far away (in either direction) from approxidate_relative() that "extending" the patch context to show the removal of the latter to cover the former would show too much irrelevant information, I think René would have wanted to show a normal patch plus an extra hunk that contains the entirety of approxidate_careful() that shows no modification (i.e. all lines are prefixed with an SP). The way I think about this new "feature" is "compute what hunks should be shown, honoring all other options. Then pretend no-op hunks to cover all specified lines in the postimage [*] are also in the result. Combine them all, ignoring parts of the made-up no-op hunks when they contradict the real hunks.". The end result should show all specified lines from the postimage plus the usual diff. [Footnote] * There is no need for a similar option to talk about lines in the preimage, because a line in the preimage would either appear in the postimage (in which case the range in the postimage can be used to show it), or otherwise it would appear as deleted line (in which case the reader will see it without the new feature.
On Tue, Apr 11, 2023 at 08:43:47AM -0700, Junio C Hamano wrote: > > Ooh, I like that very much. In that sense it really feels like an > > extension of --function-context. Would the regexes be searches starting > > from the edge of some context (as they more or less are under the hood > > for function context), or would you search within the whole file for > > ranges (and then presumably use them when a hunk's context is adjacent > > to or overlaps a range)? > > > > If the latter, I guess you could also allow both absolute and relative > > line numbers, similar to how "-L" accepts range input. > > We want the latter. > > If we further imagine that approxidate_careful() were defined very > far away (in either direction) from approxidate_relative() that > "extending" the patch context to show the removal of the latter to > cover the former would show too much irrelevant information, I think > René would have wanted to show a normal patch plus an extra hunk > that contains the entirety of approxidate_careful() that shows no > modification (i.e. all lines are prefixed with an SP). The way I > think about this new "feature" is "compute what hunks should be > shown, honoring all other options. Then pretend no-op hunks to > cover all specified lines in the postimage [*] are also in the > result. Combine them all, ignoring parts of the made-up no-op hunks > when they contradict the real hunks.". The end result should show > all specified lines from the postimage plus the usual diff. Right, that makes perfect sense. Thanks for explaining. Now we just need somebody to implement it. ;) -Peff
diff --git a/date.c b/date.c index 1fb2cd1b53..7c8650f799 100644 --- a/date.c +++ b/date.c @@ -1353,46 +1353,32 @@ static timestamp_t approxidate_str(const char *date, date++; if (isdigit(c)) { pending_number(&tm, &number); date = approxidate_digit(date-1, &tm, &number, time_sec); touched = 1; continue; } if (isalpha(c)) date = approxidate_alpha(date-1, &tm, &now, &number, &touched); } pending_number(&tm, &number); if (!touched) *error_ret = 1; return (timestamp_t)update_tm(&tm, &now, 0); } -timestamp_t approxidate_relative(const char *date) -{ - struct timeval tv; - timestamp_t timestamp; - int offset; - int errors = 0; - - if (!parse_date_basic(date, ×tamp, &offset)) - return timestamp; - - get_time(&tv); - return approxidate_str(date, (const struct timeval *) &tv, &errors); -} - timestamp_t approxidate_careful(const char *date, int *error_ret) { struct timeval tv; timestamp_t timestamp; int offset; int dummy = 0; if (!error_ret) error_ret = &dummy; if (!parse_date_basic(date, ×tamp, &offset)) { *error_ret = 0; return timestamp; } get_time(&tv); return approxidate_str(date, &tv, error_ret); diff --git a/date.h b/date.h index 5d4eaba0a9..6136212a19 100644 --- a/date.h +++ b/date.h @@ -55,20 +55,19 @@ const char *show_date(timestamp_t time, int timezone, const struct date_mode *mo */ void parse_date_format(const char *format, struct date_mode *mode); /** * Release a "struct date_mode", currently only required if * parse_date_format() has parsed a "DATE_STRFTIME" format. */ void date_mode_release(struct date_mode *mode); void show_date_relative(timestamp_t time, struct strbuf *timebuf); int parse_date(const char *date, struct strbuf *out); int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset); int parse_expiry_date(const char *date, timestamp_t *timestamp); void datestamp(struct strbuf *out); #define approxidate(s) approxidate_careful((s), NULL) timestamp_t approxidate_careful(const char *, int *); -timestamp_t approxidate_relative(const char *date); int date_overflows(timestamp_t date); time_t tm_to_time_t(const struct tm *tm); #endif diff --git a/t/helper/test-date.c b/t/helper/test-date.c index cd6a6df702..2d9232cc68 100644 --- a/t/helper/test-date.c +++ b/t/helper/test-date.c @@ -68,42 +68,42 @@ static void parse_dates(const char **argv) strbuf_reset(&result); parse_date(*argv, &result); if (sscanf(result.buf, "%"PRItime" %d", &t, &tz) == 2) printf("%s -> %s\n", *argv, show_date(t, tz, DATE_MODE(ISO8601))); else printf("%s -> bad\n", *argv); } strbuf_release(&result); } static void parse_approxidate(const char **argv) { for (; *argv; argv++) { timestamp_t t; - t = approxidate_relative(*argv); + t = approxidate(*argv); printf("%s -> %s\n", *argv, show_date(t, 0, DATE_MODE(ISO8601))); } } static void parse_approx_timestamp(const char **argv) { for (; *argv; argv++) { timestamp_t t; - t = approxidate_relative(*argv); + t = approxidate(*argv); printf("%s -> %"PRItime"\n", *argv, t); } } static void getnanos(const char **argv) { double seconds = getnanotime() / 1.0e9; if (*argv) seconds -= strtod(*argv, NULL); printf("%lf\n", seconds); } int cmd__date(int argc UNUSED, const char **argv) { const char *x;
When 29f4332e66 (Quit passing 'now' to date code, 2019-09-11) removed its timeval parameter, approxidate_relative() became equivalent to approxidate(). Convert its last two call sites and remove the redundant function. Signed-off-by: René Scharfe <l.s.r@web.de> --- Formatted with -U16 for easier review. Only useful in date.c, but cannot be restricted to just one file. date.c | 14 -------------- date.h | 1 - t/helper/test-date.c | 4 ++-- 3 files changed, 2 insertions(+), 17 deletions(-) -- 2.40.0