On 2/6/2022 6:30 AM, Eric Sunshine wrote:
> On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
>> + /*
>> + * If we are using worktree config, then copy all current config
>> + * values from the current worktree into the new one, that way the
>> + * new worktree behaves the same as this one.
>> + */
>
> As an aid for future readers, it might be helpful to extend this
> content to explain _why_ the new worktree should behave the same as
> the current one. Reproducing the important bits from the commit
> message here in the comment could make it more useful.
Here, I think I disagree. The comment is intentionally short to say
"we need to be careful here" but the reason behind it can be found
in the commit message from history spelunking.
>> + git_config_set_multivar_in_file_gently(
>> + to_file, "core.bare", NULL, "true", 0))
>> + error(_("failed to unset 'core.bare' in '%s'"), to_file);
>> + if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&
>
> In patch [2/5] you used git_configset_get_string_tmp() to retrieve
> this setting, but here you're using git_configset_get_value(). Is
> there a reason for the inconsistency?
I'm not sure why I chose to use one over the other, but looking at
the code, it seems that my use in patch 2 is the only use of
git_configset_get_string_tmp() that is not internal to config.c.
I should convert the one in patch 2 to use git_configset_get_value()
and then we can remove the declaration of git_configset_get_string_tmp()
in config.h.
>> + git worktree add ../there3 main &&
>> + cd ../there3 &&
>> + git status
>> + ) &&
>
> Is this some debugging code you forgot to remove or was `git status`
> failing due to the bug(s) fixed by this patch series? I'm guessing the
> latter since you also use `git status` in more tests below. Anyhow,
> it's not very clear what the `git-status` is meant to be testing. An
> in-code comment _might_ help. Even better, perhaps, would be to add a
> new single-purpose test or a well-named function which explicitly
> checks the conditions you want to test (i.e. that git-config doesn't
> report core.bare as true or core.worktree as having a value).
Basically, in the old code any Git command would immediately fail
because of the interpretation of core.bare or core.worktree. So
this check is just a check that a basic Git command doesn't fail
>> + git config --worktree bogus.key value &&
>> + git config --unset core.bare &&
>
> Why is this being unset? (Genuine question. Am I missing something obvious?)
I'm moving it out of the common config file. Earlier commands
enabled it in the config.worktree file for this working tree.
Thanks,
-Stolee
