git/ref-filter.c, branch v2.47.2

Merge branch 'ak/refs-symref-referent-typofix'

2024-09-25T17:37:12Z

Typofix. * ak/refs-symref-referent-typofix: ref-filter: fix a typo

Merge branch 'ps/environ-wo-the-repository'

2024-09-23T17:35:05Z

Code clean-up. * ps/environ-wo-the-repository: (21 commits) environment: stop storing "core.notesRef" globally environment: stop storing "core.warnAmbiguousRefs" globally environment: stop storing "core.preferSymlinkRefs" globally environment: stop storing "core.logAllRefUpdates" globally refs: stop modifying global `log_all_ref_updates` variable branch: stop modifying `log_all_ref_updates` variable repo-settings: track defaults close to `struct repo_settings` repo-settings: split out declarations into a standalone header environment: guard state depending on a repository environment: reorder header to split out `the_repository`-free section environment: move `set_git_dir()` and related into setup layer environment: make `get_git_namespace()` self-contained environment: move object database functions into object layer config: make dependency on repo in `read_early_config()` explicit config: document `read_early_config()` and `read_very_early_config()` environment: make `get_git_work_tree()` accept a repository environment: make `get_graft_file()` accept a repository environment: make `get_index_file()` accept a repository environment: make `get_object_directory()` accept a repository environment: make `get_git_common_dir()` accept a repository ...

ref-filter: fix a typo

2024-09-19T20:50:36Z

Fix a typo in comments. Signed-off-by: Andrew Kreimer Signed-off-by: Junio C Hamano

environment: stop storing "core.warnAmbiguousRefs" globally

2024-09-12T17:15:44Z

Same as the preceding commits, storing the "core.warnAmbiguousRefs" value globally is misdesigned as this setting may be set per repository. Move the logic into the repo-settings subsystem. The usual pattern here is that users are expected to call `prepare_repo_settings()` before they access the settings themselves. This seems somewhat fragile though, as it is easy to miss and leads to somewhat ugly code patterns at the call sites. Instead, introduce a new function that encapsulates this logic for us. This also allows us to change how exactly the lazy initialization works in the future, e.g. by only partially initializing values as requested by the caller. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

ref-filter: fix leak with unterminated %(if) atoms

2024-09-10T16:26:13Z

When parsing `%(if)` atoms we expect a few other atoms to exist to complete it, like `%(then)` and `%(end)`. Whether or not we have seen these other atoms is tracked in an allocated `if_then_else` structure, which gets free'd by the `if_then_else_handler()` once we have parsed the complete conditional expression. This results in a memory leak when the `%(if)` atom is not terminated correctly and thus incomplete. We never end up executing its handler and thus don't end up freeing the structure. Plug this memory leak by introducing a new `at_end_data_free` callback function. If set, we'll execute it in `pop_stack_element()` and pass it the `at_end_data` variable with the intent to free its state. Wire it up for the `%(if)` atom accordingly. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

ref-filter: add ref_format_clear() function

2024-09-09T23:26:11Z

After using the ref-filter API, callers should use ref_filter_clear() to free any used memory. However, there's not a matching function to clear the ref_format struct. Traditionally this did not need to be cleaned up, as it was just a way for the caller to store and pass format options as a single unit. Even though the parsing step of some placeholders may allocate data, that's usually inside their "used_atom" structs, which are part of the ref_filter itself. But a few placeholders keep data outside of there. The %(ahead-behind) and %(is-base) parsers both keep a master list of bases, because they perform a single filtering pass outside of the use of any particular atom. And since the format parser does not have access to the ref_filter struct, they store their cross-atom data in the ref_format struct itself. And thus when they are finished, the ref_format also needs to be cleaned up. So let's add a function to do so, and call it from all of the users of the ref-filter API. The %(is-base) case is found by running LSan on t6300. After this patch, the script can now be marked leak-free. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

ref-filter: fix leak when formatting %(push:remoteref)

2024-09-09T23:26:10Z

When we expand the %(upstream) or %(push) placeholders, we rely on remote.c's remote_ref_for_branch() to fill in the ":refname" argument. But that function has confusing memory ownership semantics: it may or may not return an allocated string, depending on whether we are in "upstream" mode or "push" mode. The caller in ref-filter.c always duplicates the result, meaning that we leak the original in the case of %(push:refname). To solve this, let's make the return value from remote_ref_for_branch() consistent, by always returning an allocated pointer. Note that the switch to returning a non-const pointer has a ripple effect inside the function, too. We were storing the "dst" result as a const pointer, too, even though it is always allocated! It is the return value from apply_refspecs(), which is always a non-const allocated string. And then on the caller side in ref-filter.c (and this is the only caller at all), we just need to avoid the extra duplication when the return value is non-NULL. This clears up one case that LSan finds in t6300, but there are more. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

ref-filter: fix leak with %(describe) arguments

2024-09-09T23:26:10Z

When we parse a %(describe) placeholder, we stuff its arguments into a strvec, which is then detached into the used_atom struct. But later, when ref_array_clear() frees the atom, we never free the memory. To solve this, we just need to add the appropriate free() calls. But it's a little awkward, since we have to free each element of the array, in addition to the array itself. Instead, let's store the actual strvec, which lets us do a simple strvec_clear(). This clears up one case that LSan finds in t6300, but there are more. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

ref-filter: fix leak of %(trailers) "argbuf"

2024-09-09T23:26:10Z

When we parse a placeholder like "%(trailers:key=foo)", our atom parsing function is passed just the argument string "key=foo". We duplicate this into its own string, but never free it, causing a leak. We do the duplication for two reasons: 1. There's a mismatch with the pretty.c trailer-formatting code that we rely on. It expects to see a closing paren, like "key=foo)". So we duplicate the argument string with that extra character to pass along. This is probably something we could fix in the long run, but it's somewhat non-trivial if we want to avoid regressing error cases for things like "git log --format='%(trailer:oops'". So let's accept it as a necessity for now. 2. The argument parser expects to store the list of "key" entries ("foo" in this case) in a string-list. It also stores the length of the string in the string-list "util" field. The original caller in pretty.c uses this with a "nodup" string list to avoid making extra copies, which creates a subtle dependency on the lifetime of the original format string. We do the same here, which creates that same dependency. So we can't simply free it as soon as the parsing is done. There are two possible solutions here. The first is to hold on to the duplicated "argbuf" string in the used_atom struct, so that it lives as long as the string_list which references it. But I think a less-subtle solution, and what this patch does, is to switch to a duplicating string_list. That makes it self-contained, and lets us free argbuf immediately. It may involve a few extra allocations, but this parsing is something that happens once per program, not once per output ref. This clears up one case that LSan finds in t6300, but there are more. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

ref-filter: store ref_trailer_buf data per-atom

2024-09-09T23:26:10Z

The trailer API takes options via a trailer_opts struct. Some of those options point to data structures which require extra storage. Those structures aren't actually embedded in the options struct, but rather we pass pointers, and the caller is responsible for managing them. This is a little convoluted, but makes sense since some of them are not even concrete (e.g., you can pass a filter function and a void data pointer, but the trailer code doesn't even know what's in the pointer). When for-each-ref, etc, parse the %(trailers) placeholder, they stuff the extra data into a ref_trailer_buf struct. But we only hold a single static global instance of this struct. So if a format string has multiple %(trailer) placeholders, they'll stomp on each other: the "key" list will end up with entries for all of them, and the separator buffers will use the values from whichever was parsed last. Instead, we should have a ref_trailer_buf for each instance of the placeholder, and store it alongside the trailer_opts in the used_atom structure. And that's what this patch does. Note that we also have to add code to clean them up in ref_array_clear(). The original code did not bother cleaning them up, but it wasn't technically a "leak" since they were still reachable from the static global instance. Reported-by: Brooke Kuhlmann Signed-off-by: Jeff King Signed-off-by: Junio C Hamano