Re: [PATCH 1/3] repository: create disable_replace_refs()

From: Derrick Stolee <derrickstolee@github.com>
Date: May 31, 2023, 13:37 UTC
Message-ID: <ae89feda-0a76-29d7-14ce-662214414638@github.com>
In-Reply-To: <CABPp-BFzA0yVecHK1DEGMpAhewm7oyqEim7BCw7-DTKpUzWnpw@mail.gmail.com>

On 5/31/2023 12:41 AM, Elijah Newren wrote:

> On Fri, May 26, 2023 at 11:43 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:

Show 6 quoted lines

>> We will never scope this to an in-memory repository as we want to make
>> sure that we never use replace refs throughout the life of the process
>> if this method is called.
> 
> I'm confused; doesn't the 3rd patch do exactly what this paragraph
> says you'll never do?

You mentioned in another reply that you figured it out, but for the sake
of anyone reading here: we _add_ a repo-scoped version for the config,
but we need this globally-scoped one for process-wide disabling the
feature. This could be said more clearly.

Show 22 quoted lines

>> @@ -323,13 +322,13 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
>>         struct option *options = parse_options_concat(builtin_commit_graph_options, common_opts);
>>
>>         git_config(git_default_config, NULL);
>> -
>> -       read_replace_refs = 0;
>>         save_commit_buffer = 0;
>>
>>         argc = parse_options(argc, argv, prefix, options,
>>                              builtin_commit_graph_usage, 0);
>>         FREE_AND_NULL(options);
>>
>> +       disable_replace_refs();
>> +
> 
> In this place and several others in the file, you opt to not just
> replace the assignment with a function call, but move the action line
> to later in the file.  In some cases, much later.
> 
> I don't think it hurts things, but it certainly makes me wonder why it
> was moved.  Did it break for some reason when called earlier?  (Is
> there something trickier about this patch than I expected?)

Generally, I decided to move it after option-parsing, so it wouldn't be called if we are hitting an option-parse error.

However, these moves were only important for a draft version where I had not separated the global and local scopes, so calling the method would also load config.

In this version of the patch, this is not needed at all, and I could do an in-line replacement. Thanks!

Show 12 quoted lines

>> diff --git a/repo-settings.c b/repo-settings.c
>> index 7b566d729d0..1df0320bf33 100644
>> --- a/repo-settings.c
>> +++ b/repo-settings.c
>> @@ -3,6 +3,7 @@
>>  #include "repository.h"
>>  #include "midx.h"
>>  #include "compat/fsmonitor/fsm-listen.h"
>> +#include "environment.h"
> 
> Why?  There are no other changes in this file, so I don't see why
> you'd need another include.

Thanks. This is a leftover from a previous version of the patch.

Show 8 quoted lines

>>
>>  static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
>>                           int def)
>> --
>> gitgitgadget
> 
> I think the patch is probably fine, but I saw a few things that made
> me wonder if I had missed something important, highlighted above.

Thank you for pointing them out, as the things that brought you confusion are cruft from an earlier version but are no longer valuable in this version.

Thanks, -Stolee