This is a follow-up topic I had for my "errno" refs API changes that
just landed[1]. I initially thought I'd split this into multiple
submissions, but I think it's probably better to consider it in
unison.
Comments on individual patches below:
1. https://lore.kernel.org/git/cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (12):
reflog delete: narrow scope of "cmd" passed to count_reflog_ent()
reflog expire: narrow scope of "cb" in cmd_reflog_expire()
reflog: change one->many worktree->refnames to use a string_list
reflog expire: don't do negative comparison on enum values
reflog expire: refactor & use "tip_commit" only for UE_NORMAL
reflog expire: don't use lookup_commit_reference_gently()
reflog: reduce scope of "struct rev_info"
refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN
This is all refactorings to make the reflog-related code smaller,
easier to read, using string-list instead of a custom home-grown flex
array API etc, and to make it clear what variables and data is used
where. This really helps for subsequent steps.
reflog + refs-backend: move "verbose" out of the backend
As noted in
https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/
this makes the "verbose" API independent, so the reftable backend
we'll have soon won't need to do anything special to support it.
It used to conflict with Han-Wen's topic, but in his v2 he picked
another approach.
reflog expire: add progress output on --stale-fix
gc + reflog: emit progress output from "reflog expire --all"
gc + reflog: don't stall before initial "git gc" progress output
These are all progress additions to "reflog expire" including fixing
the long-standing oddity that on large repos "git gc"'s progress seems
to simply hang before reaching "Enumerating objects".
As noted in 12/12 this is partly a bit of a hack, but I've got other
in-flight progress.c API changes that'll eventually lead to supporting
"nested" progress bars, which this code will be one of the first
consumers of.
Documentation/git-reflog.txt | 8 ++
builtin/gc.c | 4 +-
builtin/reflog.c | 260 ++++++++++++++++++++++-------------
refs.h | 3 +-
refs/files-backend.c | 44 +++---
5 files changed, 194 insertions(+), 125 deletions(-)
--
2.34.1.877.g7d5b0a3b8a6
Change the "cb_data" we pass to the count_reflog_ent() to be the
&cb.cmd itself, instead of passing &cb and having the callback lookup
cb->cmd.
This makes it clear that the "cb" itself is the same memzero'd
structure on each iteration of the for-loop that uses &cb, except for
the "cmd" member.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
As with the preceding change for "reflog delete", change the "cb_data"
we pass to callbacks to be &cb.cmd itself, instead of passing &cb and
having the callback lookup cb->cmd.
This makes it clear that the "cb" itself is the same memzero'd
structure on each iteration of the for-loops that use &cb, except for
the "cmd" member.
The "struct expire_reflog_policy_cb" we pass to reflog_expire() will
have the members that aren't "cmd" modified by the callbacks, but
before we invoke them everything except "cmd" is zero'd out.
This included the "tip_commit", "mark_list" and "tips". It might have
looked as though we were re-using those between iterations, but the
first thing we did in reflog_expiry_prepare() was to either NULL them,
or clobber them with another value.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
@@ -564,17 +560,17 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)if(!strcmp(arg,"--dry-run")||!strcmp(arg,"-n"))flags|=EXPIRE_REFLOGS_DRY_RUN;elseif(skip_prefix(arg,"--expire=",&arg)){-if(parse_expiry_date(arg,&cb.cmd.expire_total))+if(parse_expiry_date(arg,&cmd.expire_total))die(_("'%s' is not a valid timestamp"),arg);explicit_expiry|=EXPIRE_TOTAL;}elseif(skip_prefix(arg,"--expire-unreachable=",&arg)){-if(parse_expiry_date(arg,&cb.cmd.expire_unreachable))+if(parse_expiry_date(arg,&cmd.expire_unreachable))die(_("'%s' is not a valid timestamp"),arg);explicit_expiry|=EXPIRE_UNREACH;}elseif(!strcmp(arg,"--stale-fix"))-cb.cmd.stalefix=1;+cmd.stalefix=1;elseif(!strcmp(arg,"--rewrite"))flags|=EXPIRE_REFLOGS_REWRITE;elseif(!strcmp(arg,"--updateref"))
Change the FLEX_ARRAY pattern added in bda3a31cc79 (reflog-expire:
Avoid creating new files in a directory inside readdir(3) loop,
2008-01-25) the string-list API instead.
This does not change any behavior, allows us to delete much of this
code as it's replaced by things we get from the string-list API for
free, as a result we need just one struct to keep track of this data,
instead of two.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 48 +++++++++++++++++++-----------------------------
1 file changed, 19 insertions(+), 29 deletions(-)
Change code added in 03cb91b18cc (reflog --expire-unreachable: special
case entries in "HEAD" reflog, 2010-04-09) to not do positive instead
of negative comparisons on enum values, i.e. not to assume that "x !=
UE_ALWAYS" means "(x == UE_HEAD || x || UE_NORMAL)".
That assumption is true now, but we'd introduce subtle bugs here if
that were to change, now the compiler will notice and error out on
such errors.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 57 ++++++++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 24 deletions(-)
Add an intermediate variable for "tip_commit" in
reflog_expiry_prepare(), and only add it to the struct if we're
handling the UE_NORMAL case.
The code behaves the same way as before, but this makes the control
flow clearer, and the shorter name allows us to fold a 4-line i/else
int a one-line terany instead.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
In the initial implementation of "git reflog" in 4264dc15e19 (git
reflog expire, 2006-12-19) we had this
lookup_commit_reference_gently().
I don't think we've ever found tags that we need to recursively
dereference in reflogs, so this should at least be changed to a
"lookup commit" as I'm doing here, although I can't think of a way
where it mattered in practice.
I also think we'd probably like to just die here if we have a NULL
object, but as this code needs to handle potentially broken
repositories let's just show an "error" but continue, the non-quiet
lookup_commit() will do for us. None of our tests cover the case where
"commit" is NULL after this lookup.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Move the handling of the "verbose" flag entirely out of
"refs/files-backend.c" and into "builtin/reflog.c". This allows the
backend to stop knowing about the EXPIRE_REFLOGS_VERBOSE flag.
The expire_reflog_ent() function shouldn't need to deal with the
implementation detail of whether or not we're emitting verbose output,
by doing this the --verbose output becomes backend-agnostic, so
reftable will get the same output.
I think the output is rather bad currently, and should e.g. be
implemented with some better future mode of progress.[ch], but that's
a topic for another improvement.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 42 +++++++++++++++++++++++++++++++-----------
refs.h | 3 +--
refs/files-backend.c | 44 ++++++++++++++++++++------------------------
3 files changed, 52 insertions(+), 37 deletions(-)
Emit progress output on "git reflog expire --all", which is what "git
gc" runs. On my git.git checkout I'll now get:
$ time GIT_PROGRESS_DELAY=0 ~/g/git/git reflog expire --all --dry-run
Enumerating reflogs: 23782, done.
Expiring reflogs: 100% (23782/23782), done.
real 0m3.264s
user 0m2.308s
sys 0m0.941s
The "Enumerating reflogs" is too fast to appear for me except with
GIT_PROGRESS_DELAY=0. We'll also emit this at the top of "git gc"
output:
$ ~/g/git/git --exec-path=$PWD gc
Expiring reflogs: 100% (23782/23782), done.
Enumerating objects: [...]
This goes a long way (but not quite, a subsequent commit will) to
addressing the seeming halting of "git gc" on startup. That usually
happens because of the "HEAD" case in "reflog expire --all" and
unreachable().
Note that this code isn't going to be affected by the sort of bug we
had to fix in 6b89a34c89f (gc: fix regression in 7b0f229222 impacting
--quiet, 2018-09-19).
This is because "git gc" even with "--auto" won't detach until after
it runs "git reflog -expire", so whatever output we emit will never
end up in the gc.log. We should still obey its --quiet to mean our
--no-progress, but we don't need a special-case for "daemonized" as
write_commit_graph_reachable() does.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/gc.c | 4 +++-
builtin/reflog.c | 19 ++++++++++++++++++-
2 files changed, 21 insertions(+), 2 deletions(-)
Add progress output when the "git reflog expire --stale-fix" option is
used. This output was previously only emitted under --verbose, but we
shouldn't treat it the same way as the actually verbose "--verbose"
output emitted in should_expire_reflog_ent().
Note that this code isn't going to be affected by the sort of bug we
had to fix in 6b89a34c89f (gc: fix regression in 7b0f229222 impacting
--quiet, 2018-09-19). I.e. "git gc" won't call it with the
"--stale-fix" flag, that option is purely used as a one-off.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Documentation/git-reflog.txt | 8 ++++++++
builtin/reflog.c | 18 +++++++++++++-----
2 files changed, 21 insertions(+), 5 deletions(-)
@@ -69,6 +69,14 @@ Options for `show` Options for `expire` ~~~~~~~~~~~~~~~~~~~~+--progress::+--no-progress::+ Progress status is reported on the standard error stream by+ default when it is attached to a terminal. The `--progress+ flag enables progress reporting even if not attached to a+ terminal. Supplying `--no-progress` will suppress all progress+ output.+ --all:: Process the reflogs of all references.
@@ -11,6 +11,7 @@#include"revision.h"#include"reachable.h"#include"worktree.h"+#include"progress.h"/* NEEDSWORK: switch to using parse_options */staticconstcharreflog_expire_usage[]=
It's not possible for "cb->newlog" to be NULL if
!EXPIRE_REFLOGS_DRY_RUN, since files_reflog_expire() would have
error()'d and taken the "goto failure" branch if it couldn't open the
file. By not using the "newlog" field private to "file-backend.c"'s
"struct expire_reflog_cb", we can move this verbosity logging to
"builtin/reflog.c" in a subsequent commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
refs/files-backend.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Change the "git reflog expire --all" progress output to show progress
on mark_reachable() in addition to the "Expiring reflogs" output added
in the preceding commit.
This is a major UI improvement to the "git gc" progress output, which
in large repositories would seem to hang before the "initial"
"Enumerating objects" phase. E.g. on a linux.git checkout I've got it
previously took me around 10 seconds before I saw any output.
With the change to add the "Expiring reflogs" output in the preceding
commit that ~10 second delay was perhaps ~8-9, now it's pretty much
guaranteed to show output within the first couple of seconds (our
default progress delay).
Why? Because as we iterate through our reflogs to expire in
reflog_exire() we'd always check the reflog for "HEAD" first, and as
we hadn't previously marked anything REACHABLE in mark_reachable()
would almost certainly end up needing to walk a substantial amount of
history to mark commits. In linux.git north of 1 million commits.
Now we'll instead show:
$ git gc
Iterating (un)reachable HEAD: 1137354, done.
Expiring reflogs: 100% (45/45), done.
^Enmerating objects: 662499^C
[...]
The implementation is a bit of a hack. Ideally we'd support nested
progress bars, and the ability to "attach" a sub-progress bar to a
running one. I.e. to optimistically show either:
Expiring reflogs: X (Y/Z)
Or:
Expiring reflogs: X (Y/Z) iterating unreachable HEAD: 123456
In this case the problem is that if we have done our initial big
"REACHABLE" iteration we're usually going to process the reflog quite
quickly, but if we haven't we'd stall.
So as a hack pass down a "show_progress" to reflog_expiry_prepare(),
which will only be shown if we haven't processed the UE_HEAD
case. We'll then start a progress bar, which when passed through to
mark_reachable() will almost certainly result in a substantial initial
iteration.
Then when we've done that initial walk we'll stop that progress bar in
our main loop, and start the "Expiring reflogs" progress bar. We'll
usually start it at 2/X, since our "HEAD" was the 1/X usurped by the
"Iterating (un)reachable HEAD" progress bar.
The "usually" would assume that we wouldn't hit the
"cb->no_reachable_progress = 1" case being added here. I.e. if our
configuration is such that we're not going to do the UE_HEAD walk
we'll just fall back on only using "Expiring reflogs". We'll still
start the count at the 2nd reflog, but it shouldn't matter, on e.g. my
linux.git checkout it doesn't, the progress bar goes by too fast to
notice.
It would have been nicer to be able to compute the
"cb->unreachable_expire_kind" before we call reflog_expire(), but to
do that we'd need the "oid", which we'll only know once we lock it.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 53 +++++++++++++++++++++++++++++++++---------------
1 file changed, 37 insertions(+), 16 deletions(-)
This series refactors various small bits of builtin/reflog.c
(e.g. using a "struct string_list" now), and finally makes it handle
the "--verbose" output, instead of telling the files backend to emit
that same verbose output.
This means that when we start to integrate "reftable" the new backend
won't need to implement verbose reflog output, it will just work.
This is a sort-of v2[1]. I ejected the changes at the end to add
better --progress output to "git reflog". Those fixes are worthwhile,
but hopefully this smaller & easier to review series can be queued up
first, we can do those UX improvements later.
1. https://lore.kernel.org/git/cover-00.12-00000000000-20211130T213319Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (9):
reflog delete: narrow scope of "cmd" passed to count_reflog_ent()
reflog expire: narrow scope of "cb" in cmd_reflog_expire()
reflog: change one->many worktree->refnames to use a string_list
reflog expire: don't do negative comparison on enum values
reflog expire: refactor & use "tip_commit" only for UE_NORMAL
reflog expire: don't use lookup_commit_reference_gently()
reflog: reduce scope of "struct rev_info"
refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN
reflog + refs-backend: move "verbose" out of the backend
builtin/reflog.c | 208 +++++++++++++++++++++++--------------------
refs.h | 3 +-
refs/files-backend.c | 44 +++++----
3 files changed, 134 insertions(+), 121 deletions(-)
Range-diff against v1:
1: 99e8a639163 = 1: 22c8119640c reflog delete: narrow scope of "cmd" passed to count_reflog_ent()
2: c424b26b4fe = 2: b8e84538427 reflog expire: narrow scope of "cb" in cmd_reflog_expire()
3: 5a54b04a13e = 3: c0e190e46cf reflog: change one->many worktree->refnames to use a string_list
4: a7a2dfd1406 = 4: e42fac1b518 reflog expire: don't do negative comparison on enum values
5: de162a476c1 = 5: 39263cd00ae reflog expire: refactor & use "tip_commit" only for UE_NORMAL
6: eb3dd3fa8b9 = 6: c71aab5845e reflog expire: don't use lookup_commit_reference_gently()
7: 3aab4a4a436 = 7: 2fb33ef2546 reflog: reduce scope of "struct rev_info"
8: adbec242a7a = 8: f9fe6a2cfb0 refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN
9: 6a8f3915898 = 9: 28aa0aa6e30 reflog + refs-backend: move "verbose" out of the backend
10: f54dee1f1cc < -: ----------- reflog expire: add progress output on --stale-fix
11: 794e6e677a8 < -: ----------- gc + reflog: emit progress output from "reflog expire --all"
12: fc2b15d0abe < -: ----------- gc + reflog: don't stall before initial "git gc" progress output
--
2.34.1.1020.gc80c40b6642
Change the "cb_data" we pass to the count_reflog_ent() to be the
&cb.cmd itself, instead of passing &cb and having the callback lookup
cb->cmd.
This makes it clear that the "cb" itself is the same memzero'd
structure on each iteration of the for-loop that uses &cb, except for
the "cmd" member.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
As with the preceding change for "reflog delete", change the "cb_data"
we pass to callbacks to be &cb.cmd itself, instead of passing &cb and
having the callback lookup cb->cmd.
This makes it clear that the "cb" itself is the same memzero'd
structure on each iteration of the for-loops that use &cb, except for
the "cmd" member.
The "struct expire_reflog_policy_cb" we pass to reflog_expire() will
have the members that aren't "cmd" modified by the callbacks, but
before we invoke them everything except "cmd" is zero'd out.
This included the "tip_commit", "mark_list" and "tips". It might have
looked as though we were re-using those between iterations, but the
first thing we did in reflog_expiry_prepare() was to either NULL them,
or clobber them with another value.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
@@ -564,17 +560,17 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)if(!strcmp(arg,"--dry-run")||!strcmp(arg,"-n"))flags|=EXPIRE_REFLOGS_DRY_RUN;elseif(skip_prefix(arg,"--expire=",&arg)){-if(parse_expiry_date(arg,&cb.cmd.expire_total))+if(parse_expiry_date(arg,&cmd.expire_total))die(_("'%s' is not a valid timestamp"),arg);explicit_expiry|=EXPIRE_TOTAL;}elseif(skip_prefix(arg,"--expire-unreachable=",&arg)){-if(parse_expiry_date(arg,&cb.cmd.expire_unreachable))+if(parse_expiry_date(arg,&cmd.expire_unreachable))die(_("'%s' is not a valid timestamp"),arg);explicit_expiry|=EXPIRE_UNREACH;}elseif(!strcmp(arg,"--stale-fix"))-cb.cmd.stalefix=1;+cmd.stalefix=1;elseif(!strcmp(arg,"--rewrite"))flags|=EXPIRE_REFLOGS_REWRITE;elseif(!strcmp(arg,"--updateref"))
Change the FLEX_ARRAY pattern added in bda3a31cc79 (reflog-expire:
Avoid creating new files in a directory inside readdir(3) loop,
2008-01-25) the string-list API instead.
This does not change any behavior, allows us to delete much of this
code as it's replaced by things we get from the string-list API for
free, as a result we need just one struct to keep track of this data,
instead of two.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 48 +++++++++++++++++++-----------------------------
1 file changed, 19 insertions(+), 29 deletions(-)
Change code added in 03cb91b18cc (reflog --expire-unreachable: special
case entries in "HEAD" reflog, 2010-04-09) to not do positive instead
of negative comparisons on enum values, i.e. not to assume that "x !=
UE_ALWAYS" means "(x == UE_HEAD || x || UE_NORMAL)".
That assumption is true now, but we'd introduce subtle bugs here if
that were to change, now the compiler will notice and error out on
such errors.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 57 ++++++++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 24 deletions(-)
Add an intermediate variable for "tip_commit" in
reflog_expiry_prepare(), and only add it to the struct if we're
handling the UE_NORMAL case.
The code behaves the same way as before, but this makes the control
flow clearer, and the shorter name allows us to fold a 4-line i/else
int a one-line terany instead.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
In the initial implementation of "git reflog" in 4264dc15e19 (git
reflog expire, 2006-12-19) we had this
lookup_commit_reference_gently().
I don't think we've ever found tags that we need to recursively
dereference in reflogs, so this should at least be changed to a
"lookup commit" as I'm doing here, although I can't think of a way
where it mattered in practice.
I also think we'd probably like to just die here if we have a NULL
object, but as this code needs to handle potentially broken
repositories let's just show an "error" but continue, the non-quiet
lookup_commit() will do for us. None of our tests cover the case where
"commit" is NULL after this lookup.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
It's not possible for "cb->newlog" to be NULL if
!EXPIRE_REFLOGS_DRY_RUN, since files_reflog_expire() would have
error()'d and taken the "goto failure" branch if it couldn't open the
file. By not using the "newlog" field private to "file-backend.c"'s
"struct expire_reflog_cb", we can move this verbosity logging to
"builtin/reflog.c" in a subsequent commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
refs/files-backend.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Move the handling of the "verbose" flag entirely out of
"refs/files-backend.c" and into "builtin/reflog.c". This allows the
backend to stop knowing about the EXPIRE_REFLOGS_VERBOSE flag.
The expire_reflog_ent() function shouldn't need to deal with the
implementation detail of whether or not we're emitting verbose output,
by doing this the --verbose output becomes backend-agnostic, so
reftable will get the same output.
I think the output is rather bad currently, and should e.g. be
implemented with some better future mode of progress.[ch], but that's
a topic for another improvement.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 42 +++++++++++++++++++++++++++++++-----------
refs.h | 3 +--
refs/files-backend.c | 44 ++++++++++++++++++++------------------------
3 files changed, 52 insertions(+), 37 deletions(-)
On Thu, Dec 16, 2021 at 2:45 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
This series refactors various small bits of builtin/reflog.c
(e.g. using a "struct string_list" now), and finally makes it handle
the "--verbose" output, instead of telling the files backend to emit
that same verbose output.
This means that when we start to integrate "reftable" the new backend
won't need to implement verbose reflog output, it will just work.
This is a sort-of v2[1]. I ejected the changes at the end to add
better --progress output to "git reflog". Those fixes are worthwhile,
but hopefully this smaller & easier to review series can be queued up
first, we can do those UX improvements later.
Thanks for sending this.
I looked over all patches separately. Overall, the series looks good to me.
int a one-line terany instead.
ternary.
"don't do negative comparison on enum values"
I would describe it as "use switch over enum values", as this doesn't
involve negative numbers.
collected.reflogs.strdup_strings = 1;
This puzzled me. Why isn't the init done as _DUP ? Warrants a comment
at the least.
.. goto expire
..
return 0;
expire:
(personal opinion) this is going overboard with gotos and labels. Either
if ( .. ) {
expire = 1;
goto done;
}
done:
if (expire) { print stuff }
return expire
Or wrap the existing function (without changes) in a callback that
does the print for you.
your call.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
On Thu, Dec 16, 2021 at 2:45 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
This series refactors various small bits of builtin/reflog.c
(e.g. using a "struct string_list" now), and finally makes it handle
the "--verbose" output, instead of telling the files backend to emit
that same verbose output.
This means that when we start to integrate "reftable" the new backend
won't need to implement verbose reflog output, it will just work.
This is a sort-of v2[1]. I ejected the changes at the end to add
better --progress output to "git reflog". Those fixes are worthwhile,
but hopefully this smaller & easier to review series can be queued up
first, we can do those UX improvements later.
Thanks for sending this.
I looked over all patches separately. Overall, the series looks good to me.
quoted
int a one-line terany instead.
ternary.
quoted
"don't do negative comparison on enum values"
Will fix.
I would describe it as "use switch over enum values", as this doesn't
involve negative numbers.
*nod*
quoted
collected.reflogs.strdup_strings = 1;
This puzzled me. Why isn't the init done as _DUP ? Warrants a comment
at the least.
Will comment on it. FWWI this is a common pattern with the string_list
API.
If you declare it "dup" and push into it you'll end up double-duping,
but if you don't declare it dup'd and free it you'll leak memory. It
won't free() a non-duped list.
The other option is to declare it "dup" and then
"string_list_append_nodup", will try and see...
quoted
.. goto expire
..
return 0;
expire:
(personal opinion) this is going overboard with gotos and labels. Either
if ( .. ) {
expire = 1;
goto done;
}
done:
if (expire) { print stuff }
return expire
Or wrap the existing function (without changes) in a callback that
does the print for you.
your call.
Will experiment & see, looks like a wrapper might be easiest here.
Thanks!
This series refactors various small bits of builtin/reflog.c
(e.g. using a "struct string_list" now), and finally makes it handle
the "--verbose" output, instead of telling the files backend to emit
that same verbose output.
This means that when we start to integrate "reftable" the new backend
won't need to implement verbose reflog output, it will just work.
This v3 addresses comments Han-Wen had on the v2[1] in [2]. Those are
all addressed here.
I thought that eliminating the "goto" as he suggested might lead to
needlessly verbose code, but I think this new approach is much
better. We now have a "verbose" callback that's a wrapper around the
non-verbose version. For the common case of non-verbose we won't
execute any "verbose" code at all, which makes this code easier to
follow.
1. https://lore.kernel.org/git/cover-v2-0.9-00000000000-20211216T134028Z-avarab@gmail.com/
2. https://lore.kernel.org/git/CAFQ2z_McOfm545Xd8hF7YDgzyOjDmcGxpWZ6pQ-yaKAEWMMbgg@mail.gmail.com/
Ævar Arnfjörð Bjarmason (9):
reflog delete: narrow scope of "cmd" passed to count_reflog_ent()
reflog expire: narrow scope of "cb" in cmd_reflog_expire()
reflog: change one->many worktree->refnames to use a string_list
reflog expire: use "switch" over enum values
reflog expire: refactor & use "tip_commit" only for UE_NORMAL
reflog expire: don't use lookup_commit_reference_gently()
reflog: reduce scope of "struct rev_info"
refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN
reflog + refs-backend: move "verbose" out of the backend
builtin/reflog.c | 223 +++++++++++++++++++++++++------------------
refs.h | 3 +-
refs/files-backend.c | 44 ++++-----
3 files changed, 150 insertions(+), 120 deletions(-)
Range-diff against v2:
1: 22c8119640c = 1: 7fac198f485 reflog delete: narrow scope of "cmd" passed to count_reflog_ent()
2: b8e84538427 = 2: 1ffc8ef8a8b reflog expire: narrow scope of "cb" in cmd_reflog_expire()
3: c0e190e46cf ! 3: ba7679e6fc0 reflog: change one->many worktree->refnames to use a string_list
@@ Commit message
free, as a result we need just one struct to keep track of this data,
instead of two.
+ The "DUP" -> "string_list_append_nodup(..., strbuf_detach(...))"
+ pattern here is the same as that used in a recent memory leak fix in
+ b202e51b154 (grep: fix a "path_list" memory leak, 2021-10-22).
+
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
## builtin/reflog.c ##
@@ builtin/reflog.c: static void reflog_expiry_cleanup(void *cb_data)
- FLEX_ALLOC_STR(e, reflog, newref.buf);
- strbuf_release(&newref);
+ strbuf_worktree_ref(worktree, &newref, ref);
-+ string_list_append(&cb->reflogs, strbuf_detach(&newref, NULL));
++ string_list_append_nodup(&cb->reflogs, strbuf_detach(&newref, NULL));
- oidcpy(&e->oid, oid);
- ALLOC_GROW(cb->e, cb->nr + 1, cb->alloc);
@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, cons
if (do_all) {
- struct collect_reflog_cb collected;
+ struct worktree_reflogs collected = {
-+ .reflogs = STRING_LIST_INIT_NODUP,
++ .reflogs = STRING_LIST_INIT_DUP,
+ };
+ struct string_list_item *item;
struct worktree **worktrees, **p;
@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, cons
- free(e);
}
- free(collected.e);
-+ collected.reflogs.strdup_strings = 1;
+ string_list_clear(&collected.reflogs, 0);
}
4: e42fac1b518 ! 4: 74447de0413 reflog expire: don't do negative comparison on enum values
@@ Metadata
Author: Ævar Arnfjörð Bjarmason [off-list ref]
## Commit message ##
- reflog expire: don't do negative comparison on enum values
+ reflog expire: use "switch" over enum values
Change code added in 03cb91b18cc (reflog --expire-unreachable: special
- case entries in "HEAD" reflog, 2010-04-09) to not do positive instead
- of negative comparisons on enum values, i.e. not to assume that "x !=
- UE_ALWAYS" means "(x == UE_HEAD || x || UE_NORMAL)".
+ case entries in "HEAD" reflog, 2010-04-09) to use a "switch" statement
+ with an exhaustive list of "case" statements instead of doing numeric
+ comparisons against the enum labels.
- That assumption is true now, but we'd introduce subtle bugs here if
- that were to change, now the compiler will notice and error out on
- such errors.
+ Now we won't assume that "x != UE_ALWAYS" means "(x == UE_HEAD || x ||
+ UE_NORMAL)". That assumption is true now, but we'd introduce subtle
+ bugs here if that were to change, now the compiler will notice and
+ error out on such errors.
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
5: 39263cd00ae ! 5: 896ae9f73eb reflog expire: refactor & use "tip_commit" only for UE_NORMAL
@@ Commit message
The code behaves the same way as before, but this makes the control
flow clearer, and the shorter name allows us to fold a 4-line i/else
- int a one-line terany instead.
+ into a one-line ternary instead.
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
6: c71aab5845e = 6: cfa80e84c6d reflog expire: don't use lookup_commit_reference_gently()
7: 2fb33ef2546 = 7: b3a62b9b177 reflog: reduce scope of "struct rev_info"
8: f9fe6a2cfb0 = 8: 6748298a782 refs files-backend: assume cb->newlog if !EXPIRE_REFLOGS_DRY_RUN
9: 28aa0aa6e30 ! 9: 2fb9de8ae51 reflog + refs-backend: move "verbose" out of the backend
@@ builtin/reflog.c: struct expire_reflog_policy_cb {
struct cmd_reflog_expire_cb cmd;
struct commit *tip_commit;
struct commit_list *tips;
-+ unsigned int dry_run:1,
-+ verbose:1;
++ unsigned int dry_run:1;
};
struct worktree_reflogs {
@@ builtin/reflog.c: static int should_expire_reflog_ent(struct object_id *ooid, struct object_id *no
- struct commit *old_commit, *new_commit;
-
- if (timestamp < cb->cmd.expire_total)
-- return 1;
-+ goto expire;
-
- old_commit = new_commit = NULL;
- if (cb->cmd.stalefix &&
- (!keep_entry(&old_commit, ooid) || !keep_entry(&new_commit, noid)))
-- return 1;
-+ goto expire;
-
- if (timestamp < cb->cmd.expire_unreachable) {
- switch (cb->unreachable_expire_kind) {
- case UE_ALWAYS:
-- return 1;
-+ goto expire;
- case UE_NORMAL:
- case UE_HEAD:
- if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid))
-- return 1;
-+ goto expire;
- break;
- }
- }
+ return 0;
+ }
- if (cb->cmd.recno && --(cb->cmd.recno) == 0)
-- return 1;
-+ goto expire;
++static int should_expire_reflog_ent_verbose(struct object_id *ooid,
++ struct object_id *noid,
++ const char *email,
++ timestamp_t timestamp, int tz,
++ const char *message, void *cb_data)
++{
++ struct expire_reflog_policy_cb *cb = cb_data;
++ int expire;
+
-+ if (cb->verbose)
++ expire = should_expire_reflog_ent(ooid, noid, email, timestamp, tz,
++ message, cb);
++
++ if (!expire)
+ printf("keep %s", message);
-
- return 0;
-+expire:
-+ if (cb->dry_run)
++ else if (cb->dry_run)
+ printf("would prune %s", message);
-+ else if (cb->verbose)
++ else
+ printf("prune %s", message);
-+ return 1;
- }
-
++
++ return expire;
++}
++
static int push_tip_to_list(const char *refname, const struct object_id *oid,
+ int flags, void *cb_data)
+ {
@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
int i, status, do_all, all_worktrees = 1;
int explicit_expiry = 0;
unsigned int flags = 0;
+ int verbose = 0;
++ reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
default_reflog_expire_unreachable = now - 30 * 24 * 3600;
default_reflog_expire = now - 90 * 24 * 3600;
@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, cons
else if (!strcmp(arg, "--")) {
i++;
break;
+@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
+ break;
+ }
+
++ if (verbose)
++ should_prune_fn = should_expire_reflog_ent_verbose;
++
+ /*
+ * We can trust the commits and objects reachable from refs
+ * even in older repository. We cannot trust what's reachable
@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
revs.do_not_die_on_missing_tree = 1;
revs.ignore_missing = 1;
@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, cons
+ struct expire_reflog_policy_cb cb = {
+ .cmd = cmd,
+ .dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
-+ .verbose = verbose,
+ };
set_reflog_expiry_param(&cb.cmd, explicit_expiry, item->string);
status |= reflog_expire(item->string, flags,
+ reflog_expiry_prepare,
+- should_expire_reflog_ent,
++ should_prune_fn,
+ reflog_expiry_cleanup,
+ &cb);
+ }
+@@ builtin/reflog.c: static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
+ set_reflog_expiry_param(&cb.cmd, explicit_expiry, ref);
+ status |= reflog_expire(ref, flags,
+ reflog_expiry_prepare,
+- should_expire_reflog_ent,
++ should_prune_fn,
+ reflog_expiry_cleanup,
+ &cb);
+ free(ref);
@@ builtin/reflog.c: 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;
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
@@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, cons
else if (!strcmp(arg, "--")) {
i++;
break;
+@@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
+ break;
+ }
+
++ if (verbose)
++ should_prune_fn = should_expire_reflog_ent_verbose;
++
+ if (argc - i < 1)
+ return error(_("no reflog specified to delete"));
+
@@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
const char *spec = strstr(argv[i], "@{");
char *ep, *ref;
int recno;
- struct expire_reflog_policy_cb cb = { 0 };
+ struct expire_reflog_policy_cb cb = {
-+ .verbose = verbose,
+ .dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+ };
if (!spec) {
status |= error(_("not a reflog: %s"), argv[i]);
+@@ builtin/reflog.c: static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
+ cb.cmd = cmd;
+ status |= reflog_expire(ref, flags,
+ reflog_expiry_prepare,
+- should_expire_reflog_ent,
++ should_prune_fn,
+ reflog_expiry_cleanup,
+ &cb);
+ free(ref);
## refs.h ##
@@ refs.h: enum ref_type ref_type(const char *refname);
--
2.34.1.1146.gb52885e7c44
Change the "cb_data" we pass to the count_reflog_ent() to be the
&cb.cmd itself, instead of passing &cb and having the callback lookup
cb->cmd.
This makes it clear that the "cb" itself is the same memzero'd
structure on each iteration of the for-loop that uses &cb, except for
the "cmd" member.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
As with the preceding change for "reflog delete", change the "cb_data"
we pass to callbacks to be &cb.cmd itself, instead of passing &cb and
having the callback lookup cb->cmd.
This makes it clear that the "cb" itself is the same memzero'd
structure on each iteration of the for-loops that use &cb, except for
the "cmd" member.
The "struct expire_reflog_policy_cb" we pass to reflog_expire() will
have the members that aren't "cmd" modified by the callbacks, but
before we invoke them everything except "cmd" is zero'd out.
This included the "tip_commit", "mark_list" and "tips". It might have
looked as though we were re-using those between iterations, but the
first thing we did in reflog_expiry_prepare() was to either NULL them,
or clobber them with another value.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
@@ -564,17 +560,17 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)if(!strcmp(arg,"--dry-run")||!strcmp(arg,"-n"))flags|=EXPIRE_REFLOGS_DRY_RUN;elseif(skip_prefix(arg,"--expire=",&arg)){-if(parse_expiry_date(arg,&cb.cmd.expire_total))+if(parse_expiry_date(arg,&cmd.expire_total))die(_("'%s' is not a valid timestamp"),arg);explicit_expiry|=EXPIRE_TOTAL;}elseif(skip_prefix(arg,"--expire-unreachable=",&arg)){-if(parse_expiry_date(arg,&cb.cmd.expire_unreachable))+if(parse_expiry_date(arg,&cmd.expire_unreachable))die(_("'%s' is not a valid timestamp"),arg);explicit_expiry|=EXPIRE_UNREACH;}elseif(!strcmp(arg,"--stale-fix"))-cb.cmd.stalefix=1;+cmd.stalefix=1;elseif(!strcmp(arg,"--rewrite"))flags|=EXPIRE_REFLOGS_REWRITE;elseif(!strcmp(arg,"--updateref"))
Change code added in 03cb91b18cc (reflog --expire-unreachable: special
case entries in "HEAD" reflog, 2010-04-09) to use a "switch" statement
with an exhaustive list of "case" statements instead of doing numeric
comparisons against the enum labels.
Now we won't assume that "x != UE_ALWAYS" means "(x == UE_HEAD || x ||
UE_NORMAL)". That assumption is true now, but we'd introduce subtle
bugs here if that were to change, now the compiler will notice and
error out on such errors.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 57 ++++++++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 24 deletions(-)
Change the FLEX_ARRAY pattern added in bda3a31cc79 (reflog-expire:
Avoid creating new files in a directory inside readdir(3) loop,
2008-01-25) the string-list API instead.
This does not change any behavior, allows us to delete much of this
code as it's replaced by things we get from the string-list API for
free, as a result we need just one struct to keep track of this data,
instead of two.
The "DUP" -> "string_list_append_nodup(..., strbuf_detach(...))"
pattern here is the same as that used in a recent memory leak fix in
b202e51b154 (grep: fix a "path_list" memory leak, 2021-10-22).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 47 ++++++++++++++++++-----------------------------
1 file changed, 18 insertions(+), 29 deletions(-)
In the initial implementation of "git reflog" in 4264dc15e19 (git
reflog expire, 2006-12-19) we had this
lookup_commit_reference_gently().
I don't think we've ever found tags that we need to recursively
dereference in reflogs, so this should at least be changed to a
"lookup commit" as I'm doing here, although I can't think of a way
where it mattered in practice.
I also think we'd probably like to just die here if we have a NULL
object, but as this code needs to handle potentially broken
repositories let's just show an "error" but continue, the non-quiet
lookup_commit() will do for us. None of our tests cover the case where
"commit" is NULL after this lookup.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Add an intermediate variable for "tip_commit" in
reflog_expiry_prepare(), and only add it to the struct if we're
handling the UE_NORMAL case.
The code behaves the same way as before, but this makes the control
flow clearer, and the shorter name allows us to fold a 4-line i/else
into a one-line ternary instead.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
It's not possible for "cb->newlog" to be NULL if
!EXPIRE_REFLOGS_DRY_RUN, since files_reflog_expire() would have
error()'d and taken the "goto failure" branch if it couldn't open the
file. By not using the "newlog" field private to "file-backend.c"'s
"struct expire_reflog_cb", we can move this verbosity logging to
"builtin/reflog.c" in a subsequent commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
refs/files-backend.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Move the handling of the "verbose" flag entirely out of
"refs/files-backend.c" and into "builtin/reflog.c". This allows the
backend to stop knowing about the EXPIRE_REFLOGS_VERBOSE flag.
The expire_reflog_ent() function shouldn't need to deal with the
implementation detail of whether or not we're emitting verbose output,
by doing this the --verbose output becomes backend-agnostic, so
reftable will get the same output.
I think the output is rather bad currently, and should e.g. be
implemented with some better future mode of progress.[ch], but that's
a topic for another improvement.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/reflog.c | 56 +++++++++++++++++++++++++++++++++++++-------
refs.h | 3 +--
refs/files-backend.c | 44 ++++++++++++++++------------------
3 files changed, 68 insertions(+), 35 deletions(-)