[pull] master from git:master#71
Merged
pull[bot] merged 25 commits intoturkdevops:masterfrom Jun 26, 2025
Merged
Conversation
Commands slated for removal like "git pack-redundant" now require an explicit "--i-still-use-this" option to run. This is to discourage casual use and surface their pending deprecation to users. The warning message is long, so factor it into a helper function you_still_use_that() to simplify reuse by other commands. Also add a missing test to ensure this enforcement works for "pack-redundant". Helped-by: Elijah Newren <newren@gmail.com> [en: log message] Signed-off-by: Junio C Hamano <gitster@pobox.com>
Some documentation examples reference "whatchanged", either as a placeholder command or an example of source structure. To reduce the need for future edits when `whatchanged` is removed, replace these references with alternatives: - In `MyFirstObjectWalk.adoc`, use `version` as the nearby anchor point for `walken`, instead of `whatchanged`. - In `user-manual.adoc`, cite `show` instead of `whatchanged` as a command whose source lives in the same file as `log`. Helped-by: Elijah Newren <newren@gmail.com> [en: log message] Signed-off-by: Junio C Hamano <gitster@pobox.com>
Some tests on fast-import run "git whatchanged" without even checking the output from the command. It is tempting to remove the calls altogether since they are not doing anything useful, but they presumably were added there while the tests were developed to manually sanity check which paths were touched. Replace these calls with "git log --raw", which is a rough equivalent in the more modern Git. This does not remove "git whatchanged", but we no longer have to worry about adjusting these places when we eventually do. Helped-by: Elijah Newren <newren@gmail.com> [en: log message] Signed-off-by: Junio C Hamano <gitster@pobox.com>
The documentation of "git whatchanged" is pretty explicit that the command was retained for historical reasons to help those whose fingers cannot be retrained. Let's see if they still are finding it hard to type "git log --raw" instead of "git whatchanged" by marking the command as "nominated for removal", and require "--i-still-use-this" on the command line. Adjust the tests so that the option is passed when we invoke the command. In addition, we test that the command fails when "--i-still-use-this" is not given. Signed-off-by: Junio C Hamano <gitster@pobox.com>
As we made "git whatchanged" require "--i-still-use-this" and asked the users to report if they still want to use it, the logical next step is to allow us build Git without "whatchanged" to prepare for its eventual removal. If we were to follow the pattern established in 8ccc75c (remote: announce removal of "branches/" and "remotes/", 2025-01-22), we can do this together with the documentation update to officially list that the command will be removed in the BreakingChanges document, but let's just keep the changes separate just in case we want to proceed a bit slower. Signed-off-by: Junio C Hamano <gitster@pobox.com>
This can be squashed into the previous step. That is how our "git pack-redundant" conversion did. Theoretically, however, those who want to gauge the need to keep the command by exposing their users to patches before this one may want to wait until their experiment finishes before they formally say "this will go away". This change is made into a separate patch from the previous step precisely to help those folks. While at it, update the documentation page to use the new [synopsis] facility to mark-up the SYNOPSIS part. Helped-by: Elijah Newren <newren@gmail.com> [en: typofix] Signed-off-by: Junio C Hamano <gitster@pobox.com>
Convert the array of maintenance tasks to use designated field initializers. This makes it easier to add more fields to the struct without having to modify all tasks. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have two different variables that track the quietness for git-gc(1): - The local variable `quiet`, which we wire up. - The `quiet` field of `struct maintenance_run_opts`. This leads to confusion which of these variables should be used and what the respective effect is. Simplify this logic by dropping the local variable in favor of the options field. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Users of git-maintenance(1) can explicitly ask it to run specific tasks
by passing the `--task=` command line option. This option can be passed
multiple times, which causes us to execute tasks in the same order as
the tasks have been provided by the user.
The order in which tasks are run is computed in `task_option_parse()`:
every time we parse such a command line argument, we modify the global
array of tasks by seting the selected index for that specific task.
This has two downsides:
- We modify global state, which makes it hard to follow the logic.
- The configuration of tasks is split across multiple different
functions, so it is not easy to figure out the different factors
that play a role in selecting tasks.
Refactor the logic so that `task_option_parse()` does not modify global
state anymore. Instead, this function now only collects the list of
configured tasks. The logic to configure ordering of the respective
tasks is then deferred to `initialize_task_config()`.
This refactoring solves the second problem, that the configuration of
tasks is spread across multiple different locations. The first problem,
that we modify global state, will be fixed in a subsequent commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "--task=" option explicitly allows the user to say which maintenance tasks should be run, whereas "--schedule=" only respects the maintenance strategy configured for a specific repository. As such, it is not sensible to accept both options at the same time. Mark them as incompatible with one another. While at it, also convert the existing logic that marks "--auto" and "--schedule=" as incompatible to use `die_for_incompatible_opt2()`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When configuring maintenance tasks run by git-maintenance(1) we do so by
modifying the global array of tasks directly. This is already quite bad
on its own, as global state makes for logic that is hard to follow.
Even more importantly though we use multiple different fields to track
whether or not a task should be run:
- "enabled" tracks the "maintenance.*.enabled" config key. This field
disables execution of a task, unless the user has explicitly asked
for the task.
- "selected_order" tracks the order in which jobs have been asked for
by the user via the "--task=" command line option. It overrides
everything else, but only has an effect if at least one job has been
selected.
- "schedule" tracks the schedule priority for a job, that is how often
it should run. This field only plays a role when the user has passed
the "--schedule=" command line option.
All of this makes it non-trivial to figure out which job really should
be running right now. The logic to configure these fields and the logic
that interprets them is distributed across multiple functions, making it
even harder to follow it.
Refactor the logic so that we stop modifying global state. Instead, we
now compute which jobs should be run in `initialize_task_config()`,
represented as an array of jobs to run that is stored in the options
structure. Like this, all logic becomes self-contained and any users of
this array only need to iterate through the tasks and execute them one
by one.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Extract the function to run maintenance tasks. This function will be reused in a subsequent commit where we introduce a split between maintenance tasks that run before and after daemonizing the process. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The typedefs for `maintenance_task_fn` and `maintenance_auto_fn` are somewhat confusingly not true function pointers. As such, any user of those typedefs needs to manually add the pointer to make use of them. Fix this by making these true function pointers. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Both git-gc(1) and git-maintenance(1) have logic to daemonize so that the maintenance tasks are performed in the background. git-gc(1) has some special logic though to not perform _all_ housekeeping tasks in the background: both references and reflogs are still handled synchronously in the foreground. This split exists because otherwise it may easily happen that git-gc(1) keeps the "packed-refs" file locked for an extended amount of time, where the next Git command that wants to modify any reference could now fail. This was especially important in the past, where git-gc(1) was still executed directly as part of our automatic maintenance: git-gc(1) was invoked via `git gc --auto --detach`, so we knew to handle most of the maintenance tasks in the background while doing those parts that may cause locking issues in the foreground. We have since moved to git-maintenance(1), which is a more flexible replacement for git-gc(1). By default this command runs git-gc(1), only, but it can be configured to run different tasks, as well. This command does not know about the split between maintenance tasks that should run before and after detach though, and this has led to several bug reports about spurious locking errors for the "packed-refs" file. Prepare for a fix by introducing this split for maintenance tasks. Note that this commit does not yet change any of the tasks, so there should not (yet) be a change in behaviour. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
As explained in the preceding commit, git-gc(1) knows to detach only after it has already packed references and expired reflogs. This is done to avoid racing around their respective lockfiles. Adapt git-maintenance(1) accordingly and run the "pack-refs" and "reflog-expire" tasks in the foreground. Note that the "gc" task has the same issue, but the fix is a bit more involved there and will thus be done in a subsequent commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Sometimes code wants to die in a situation where it already has written an error message. To use the same error code as `die()` we have to use `exit(128)`, which is easy to get wrong and leaves magic numbers all over our codebase. Teach `die_message_builtin()` to not print any error when passed a `NULL` pointer as error string. Like this, such users can now call `die(NULL)` to achieve the same result without any hardcoded error codes. Adapt a couple of builtins to use this new pattern to demonstrate that there is a need for such a helper. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `gc_before_repack()` should only ever run once in git-gc(1), but we may end up calling it twice when the "--detach" flag is passed. The duplicated call is avoided though via a static flag in this function. This pattern is somewhat unintuitive though. Refactor it to drop the static flag and instead guard the second call of `gc_before_repack()` via `opts.detach`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "gc" task has a similar locking race as the one that we have fixed
for the "pack-refs" and "reflog-expire" tasks in preceding commits. Fix
this by splitting up the logic of the "gc" task:
- We execute `gc_before_repack()` in the foreground, which contains
the logic that git-gc(1) itself would execute in the foreground, as
well.
- We spawn git-gc(1) after detaching, but with a new hidden flag that
suppresses calling `gc_before_repack()`.
Like this we have roughly the same logic as git-gc(1) itself and know to
repack refs and reflogs before detaching, thus fixing the race.
Note that `gc_before_repack()` is renamed to `gc_foreground_tasks()` to
better reflect what this function does.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The function handle_content_type allocates memory for boundary using xmalloc(sizeof(struct strbuf)). If (++mi->content_top >= &mi->content[MAX_BOUNDARIES]) is true, the function returns without freeing boundary. Signed-off-by: Jinyao Guo <guo846@purdue.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Some platforms like AIX lack .d_type member in "struct dirent"; use the DTYPE(e) macro instead of a direct reference to e->d_type and when it yields DT_UNKNOWN, find the real type with get_dtype(). Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git whatchanged" that is longer to type than "git log --raw" which is its modern rough equivalent has outlived its usefulness more than 10 years ago. Plan to deprecate and remove it. * jc/you-still-use-whatchanged: whatschanged: list it in BreakingChanges document whatchanged: remove when built with WITH_BREAKING_CHANGES whatchanged: require --i-still-use-this tests: prepare for a world without whatchanged doc: prepare for a world without whatchanged you-still-use-that??: help deprecating commands for removal
"git maintenance" lacked the care "git gc" had to avoid holding onto the repository lock for too long during packing refs, which has been remedied. * ps/maintenance-ref-lock: builtin/maintenance: fix locking race when handling "gc" task builtin/gc: avoid global state in `gc_before_repack()` usage: allow dying without writing an error message builtin/maintenance: fix locking race with refs and reflogs tasks builtin/maintenance: split into foreground and background tasks builtin/maintenance: fix typedef for function pointers builtin/maintenance: extract function to run tasks builtin/maintenance: stop modifying global array of tasks builtin/maintenance: mark "--task=" and "--schedule=" as incompatible builtin/maintenance: centralize configuration of explicit tasks builtin/gc: drop redundant local variable builtin/gc: use designated field initializers for maintenance tasks
Recent code added a direct access to the d_type member in "struct dirent", but some platforms lack it, which has been corrected. * jc/diff-no-index-with-pathspec-fix: diff-no-index: do not reference .d_type member of struct dirent
Leakfix. * jg/mailinfo-leakfix: mailinfo.c: fix memory leak in function handle_content_type()
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
Mode: paranoid | Total findings: 1 | Considered vulnerability: 1 Insecure Processing of Data (1)
More info on how to fix Insecure Processing of Data in C/C++. 👉 Go to the dashboard for detailed results. 📥 Happy? Share your feedback with us. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.1)
Can you help keep this open source service alive? 💖 Please sponsor : )