| Message ID | e4c0047a17ca1b5f824acfb209884a59a93ea523.1645209647.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libify reflog | expand |
On Fri, Feb 18 2022, John Cai via GitGitGadget wrote: > From: John Cai <johncai86@gmail.com> > > Now that reflog is libified into reflog.c, we can call reflog_delete > from the reflog.c library. > > Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > Signed-off-by: John Cai <johncai86@gmail.com> > --- > builtin/reflog.c | 42 ++---------------------------------------- > 1 file changed, 2 insertions(+), 40 deletions(-) > > diff --git a/builtin/reflog.c b/builtin/reflog.c > index 65198320cd2..03d347e5832 100644 > --- a/builtin/reflog.c > +++ b/builtin/reflog.c > @@ -316,12 +316,10 @@ static const char * reflog_delete_usage[] = { > > static int cmd_reflog_delete(int argc, const char **argv, const char *prefix) > { > - struct cmd_reflog_expire_cb cmd = { 0 }; > int i, status = 0; > unsigned int flags = 0; > int verbose = 0; > > - reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent; > const struct option options[] = { > OPT_BIT(0, "dry-run", &flags, N_("do not actually prune any entries"), > EXPIRE_REFLOGS_DRY_RUN), > @@ -337,48 +335,12 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix) > > argc = parse_options(argc, argv, prefix, options, reflog_delete_usage, 0); > > - if (verbose) > - should_prune_fn = should_expire_reflog_ent_verbose; > - > if (argc < 1) > return error(_("no reflog specified to delete")); > > - for (i = 0; i < argc; i++) { > - const char *spec = strstr(argv[i], "@{"); > - char *ep, *ref; > - int recno; > - struct expire_reflog_policy_cb cb = { > - .dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN), > - }; > - > - if (!spec) { > - status |= error(_("not a reflog: %s"), argv[i]); > - continue; > - } > - > - if (!dwim_log(argv[i], spec - argv[i], NULL, &ref)) { > - status |= error(_("no reflog for '%s'"), argv[i]); > - continue; > - } > - > - recno = strtoul(spec + 2, &ep, 10); > - if (*ep == '}') { > - cmd.recno = -recno; > - for_each_reflog_ent(ref, count_reflog_ent, &cmd); > - } else { > - cmd.expire_total = approxidate(spec + 2); > - for_each_reflog_ent(ref, count_reflog_ent, &cmd); > - cmd.expire_total = 0; > - } > + for (i = 0; i < argc; i++) > + status |= reflog_delete(argv[i], flags, verbose); > > - cb.cmd = cmd; > - status |= reflog_expire(ref, flags, > - reflog_expiry_prepare, > - should_prune_fn, > - reflog_expiry_cleanup, > - &cb); > - free(ref); > - } > return status; > } Maybe others will disagree, but per my comment on 1/2 I found reviewing this locally much easier with this squashed into 1/2 (without the {} changes I suggested). I.e. the diff move/rename detection eats up this change & shows that the combinatino of 1/3 and 2/3 is almost entirely just moving around existing code (good!). But without this squashed 1/3 has a reflog_delete() "addition", that we later can see is mostly just moving things around. I'll leave it to you to decide what you want to do there, just suggestion on an otherwise very trivial-to-review change :)
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Maybe others will disagree, but per my comment on 1/2 I found reviewing > this locally much easier with this squashed into 1/2 (without the {} > changes I suggested). Oh, I am pretty much on the same page. The if() block has to retain {} after all but not for the reason you cite (i.e. help "diff --color-moved"), but for correctness reasons it has to have some "early return to avoid using NULL" to replace "continue", which means the body of the if() statement needs to stay a two statement block.
diff --git a/builtin/reflog.c b/builtin/reflog.c index 65198320cd2..03d347e5832 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -316,12 +316,10 @@ static const char * reflog_delete_usage[] = { static int cmd_reflog_delete(int argc, const char **argv, const char *prefix) { - struct cmd_reflog_expire_cb cmd = { 0 }; int i, status = 0; unsigned int flags = 0; int verbose = 0; - reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent; const struct option options[] = { OPT_BIT(0, "dry-run", &flags, N_("do not actually prune any entries"), EXPIRE_REFLOGS_DRY_RUN), @@ -337,48 +335,12 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, reflog_delete_usage, 0); - if (verbose) - should_prune_fn = should_expire_reflog_ent_verbose; - if (argc < 1) return error(_("no reflog specified to delete")); - for (i = 0; i < argc; i++) { - const char *spec = strstr(argv[i], "@{"); - char *ep, *ref; - int recno; - struct expire_reflog_policy_cb cb = { - .dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN), - }; - - if (!spec) { - status |= error(_("not a reflog: %s"), argv[i]); - continue; - } - - if (!dwim_log(argv[i], spec - argv[i], NULL, &ref)) { - status |= error(_("no reflog for '%s'"), argv[i]); - continue; - } - - recno = strtoul(spec + 2, &ep, 10); - if (*ep == '}') { - cmd.recno = -recno; - for_each_reflog_ent(ref, count_reflog_ent, &cmd); - } else { - cmd.expire_total = approxidate(spec + 2); - for_each_reflog_ent(ref, count_reflog_ent, &cmd); - cmd.expire_total = 0; - } + for (i = 0; i < argc; i++) + status |= reflog_delete(argv[i], flags, verbose); - cb.cmd = cmd; - status |= reflog_expire(ref, flags, - reflog_expiry_prepare, - should_prune_fn, - reflog_expiry_cleanup, - &cb); - free(ref); - } return status; }