Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix global repository field not being cleared #4083

Merged
merged 1 commit into from
Dec 20, 2022
Merged

Fix global repository field not being cleared #4083

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion repository.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ static void repo_set_commondir(struct repository *repo,
{
struct strbuf sb = STRBUF_INIT;

free(repo->commondir);
FREE_AND_NULL(repo->commondir);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, didn't you say that the commit that introduced the problem was not yet upstreamed? But I see gitgitgadget@b0f1a41 being the identical patch, but targeting upstream's default branch...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, actually I first thought that it will be applicable there, but then started to investigate further and came to conclusion that it quite probably would not be relevant there and closed my PR.

Anyway, I've seen that you hinted in that PR something like that it still can be important in upstream, so I'll read your comments more thoroughly and then decide whether to continue working with upstream instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug described is not present in upstream (at least in the exact same scenario).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am actually rather convinced that the upstream contribution is the way we should go forward with: the patch is clearly fixing a bug that is caused by the combination of two upstream patches, and even if the current symptom cannot be seen with upstream's version, it is still a bug in upstream.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate, please? I'm not aware of implicit assumptions taken in the codebase, and for me it's quite likely that they are implying that the code path in question should not request for config values of the global repository object... If that's not the case, I would be happy to make an upstream contribution with corresponding reasoning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate, please?

In gitgitgadget@f9b7573#diff-8107da631aae41d1abfe44a6e2ee403d79b2bdf07398eb1f7737fefeb073c0acR43, code was added that releases the memory that repo->commondir points to, but it does not set that pointer to NULL because the very next line sets it to something different.

However, in gitgitgadget@357a03e#diff-8107da631aae41d1abfe44a6e2ee403d79b2bdf07398eb1f7737fefeb073c0acR59-R66, quite a bit of code (and a call to another function) was introduced between releasing that memory and setting it again.

This introduced the fragility in the code that eventually lead to the code path (currently only a problem in Git for Windows' fork).

One might argue that the bug is in the windows.appendAtomically code, but this would be an incorrect assessment: the fragile code was introduced by 357a03e and it was only a matter of time until a patch would trigger this fragility, in this instance 61cbb76.

What 61cbb76 does is totally legitimate: it asks whether there is a repo->commondir to work with, and if there is, uses it. It's the fault of 357a03e that there is an unusable, non-NULL repo->commondir.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, exactly what I thought (actually). I'll prepare the PR to upstream and get this discussion linked in it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll prepare the PR to upstream

@zwerdna you could simply reopen gitgitgadget#1402 :-)


if (commondir) {
repo->different_commondir = 1;
Expand Down