| Message ID | f7577a0ab37988476cdb83e310f96f4841c9364a.1717402363.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 66c23e679273ac706c302902b6d8e9a6c602ff78 |
| Headers | show |
| Series | refs: ref storage migrations | expand |
On Mon, Jun 03, 2024 at 11:30:35AM +0200, Patrick Steinhardt wrote: > +static int for_each_root_ref(struct files_ref_store *refs, > + int (*cb)(const char *refname, void *cb_data), > + void *cb_data) > { > struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT; > const char *dirname = refs->loose->root->name; > struct dirent *de; > size_t dirnamelen; > + int ret; > DIR *d; Should we initialize ret to 0 here? We set it only inside the loop over dir entries: > @@ -357,14 +355,47 @@ static void add_root_refs(struct files_ref_store *refs, > strbuf_addstr(&refname, de->d_name); > > dtype = get_dtype(de, &path, 1); > - if (dtype == DT_REG && is_root_ref(de->d_name)) > - loose_fill_ref_dir_regular_file(refs, refname.buf, dir); > + if (dtype == DT_REG && is_root_ref(de->d_name)) { > + ret = cb(refname.buf, cb_data); > + if (ret) > + goto done; > + } > > strbuf_setlen(&refname, dirnamelen); > } ...but if the directory is empty (or only has "." files and ".lock" files), we won't call "cb" at all, and hence won't ever set "ret". And then at the end: > +done: > strbuf_release(&refname); > strbuf_release(&path); > closedir(d); > + return ret; > +} We return uninitialized garbage. (Sorry for the late review; this got flagged by coverity since the topic hit 'next'). -Peff
On Wed, Jun 05, 2024 at 06:07:28AM -0400, Jeff King wrote: > On Mon, Jun 03, 2024 at 11:30:35AM +0200, Patrick Steinhardt wrote: > > > +static int for_each_root_ref(struct files_ref_store *refs, > > + int (*cb)(const char *refname, void *cb_data), > > + void *cb_data) > > { > > struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT; > > const char *dirname = refs->loose->root->name; > > struct dirent *de; > > size_t dirnamelen; > > + int ret; > > DIR *d; > > Should we initialize ret to 0 here? Yeah, we should. Or rather, I'll set `ret = 0;` on the successful path. Patrick
On Thu, Jun 06, 2024 at 06:50:56AM +0200, Patrick Steinhardt wrote: > On Wed, Jun 05, 2024 at 06:07:28AM -0400, Jeff King wrote: > > On Mon, Jun 03, 2024 at 11:30:35AM +0200, Patrick Steinhardt wrote: > > > > > +static int for_each_root_ref(struct files_ref_store *refs, > > > + int (*cb)(const char *refname, void *cb_data), > > > + void *cb_data) > > > { > > > struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT; > > > const char *dirname = refs->loose->root->name; > > > struct dirent *de; > > > size_t dirnamelen; > > > + int ret; > > > DIR *d; > > > > Should we initialize ret to 0 here? > > Yeah, we should. Or rather, I'll set `ret = 0;` on the successful path. > > Patrick I was wondering why the compiler didn't flag it, because I know that GCC has `-Wmaybe-uninitialized`. Turns out that this warning only works when having optimizations enabled, but if we do then it correctly flags this use: (git) ~/Development/git:HEAD $ make refs/files-backend.o * new build flags CC refs/files-backend.o refs/files-backend.c: In function ‘for_each_root_ref’: refs/files-backend.c:371:16: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized] 371 | return ret; | ^~~ refs/files-backend.c:334:13: note: ‘ret’ was declared here 334 | int ret; | ^~~ cc1: all warnings being treated as errors I'll have a look at our CI jobs and adapt my own config.mak to include `-Og`. Patrick
On Thu, Jun 06, 2024 at 07:15:38AM +0200, Patrick Steinhardt wrote: > On Thu, Jun 06, 2024 at 06:50:56AM +0200, Patrick Steinhardt wrote: > > On Wed, Jun 05, 2024 at 06:07:28AM -0400, Jeff King wrote: > > > On Mon, Jun 03, 2024 at 11:30:35AM +0200, Patrick Steinhardt wrote: > > > > > > > +static int for_each_root_ref(struct files_ref_store *refs, > > > > + int (*cb)(const char *refname, void *cb_data), > > > > + void *cb_data) > > > > { > > > > struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT; > > > > const char *dirname = refs->loose->root->name; > > > > struct dirent *de; > > > > size_t dirnamelen; > > > > + int ret; > > > > DIR *d; > > > > > > Should we initialize ret to 0 here? > > > > Yeah, we should. Or rather, I'll set `ret = 0;` on the successful path. > > > > Patrick > > I was wondering why the compiler didn't flag it, because I know that GCC > has `-Wmaybe-uninitialized`. Turns out that this warning only works when > having optimizations enabled, but if we do then it correctly flags this > use: > > (git) ~/Development/git:HEAD $ make refs/files-backend.o > * new build flags > CC refs/files-backend.o > refs/files-backend.c: In function ‘for_each_root_ref’: > refs/files-backend.c:371:16: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized] > 371 | return ret; > | ^~~ > refs/files-backend.c:334:13: note: ‘ret’ was declared here > 334 | int ret; > | ^~~ > cc1: all warnings being treated as errors > > I'll have a look at our CI jobs and adapt my own config.mak to include > `-Og`. > > Patrick I've sent out a patch series [1] that would have made CI detect this issue before hitting any of the mainline branches. [1]: https://lore.kernel.org/git/cover.1717655210.git.ps@pks.im/ Patrick
diff --git a/refs/files-backend.c b/refs/files-backend.c index b4e5437ffe..b7268b26c8 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -323,17 +323,15 @@ static void loose_fill_ref_dir(struct ref_store *ref_store, add_per_worktree_entries_to_dir(dir, dirname); } -/* - * Add root refs to the ref dir by parsing the directory for any files which - * follow the root ref syntax. - */ -static void add_root_refs(struct files_ref_store *refs, - struct ref_dir *dir) +static int for_each_root_ref(struct files_ref_store *refs, + int (*cb)(const char *refname, void *cb_data), + void *cb_data) { struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT; const char *dirname = refs->loose->root->name; struct dirent *de; size_t dirnamelen; + int ret; DIR *d; files_ref_path(refs, &path, dirname); @@ -341,7 +339,7 @@ static void add_root_refs(struct files_ref_store *refs, d = opendir(path.buf); if (!d) { strbuf_release(&path); - return; + return -1; } strbuf_addstr(&refname, dirname); @@ -357,14 +355,47 @@ static void add_root_refs(struct files_ref_store *refs, strbuf_addstr(&refname, de->d_name); dtype = get_dtype(de, &path, 1); - if (dtype == DT_REG && is_root_ref(de->d_name)) - loose_fill_ref_dir_regular_file(refs, refname.buf, dir); + if (dtype == DT_REG && is_root_ref(de->d_name)) { + ret = cb(refname.buf, cb_data); + if (ret) + goto done; + } strbuf_setlen(&refname, dirnamelen); } + +done: strbuf_release(&refname); strbuf_release(&path); closedir(d); + return ret; +} + +struct fill_root_ref_data { + struct files_ref_store *refs; + struct ref_dir *dir; +}; + +static int fill_root_ref(const char *refname, void *cb_data) +{ + struct fill_root_ref_data *data = cb_data; + loose_fill_ref_dir_regular_file(data->refs, refname, data->dir); + return 0; +} + +/* + * Add root refs to the ref dir by parsing the directory for any files which + * follow the root ref syntax. + */ +static void add_root_refs(struct files_ref_store *refs, + struct ref_dir *dir) +{ + struct fill_root_ref_data data = { + .refs = refs, + .dir = dir, + }; + + for_each_root_ref(refs, fill_root_ref, &data); } static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs,
Extract a new function that can be used to iterate through all root refs known to the "files" backend. This will be used in the next commit, where we start to teach ref backends to remove themselves. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- refs/files-backend.c | 49 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 9 deletions(-)