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.

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

FSMonitor fixes #3263

Merged
merged 5 commits into from
Jun 9, 2021
Merged

FSMonitor fixes #3263

merged 5 commits into from
Jun 9, 2021

Conversation

dscho
Copy link
Member

@dscho dscho commented Jun 9, 2021

While Git's test suite did not report any regressions, I had to hunt for a few days to figure out why Scalar's Functional Tests were still unhappy with the new FSMonitor patch series.

This PR is the culmination of those efforts, and a sibling to microsoft#371 (which not only integrates these patches, but makes sure that Scalar's Functional Tests are run in microsoft/git as part of the CI).

dscho added 5 commits June 9, 2021 11:30
…emon via IPC

In FSMonitor v1, we made sure to only use a valid `since_token` when
querying the FSMonitor. This condition was accidentally lost in v2, and
caused segmentation faults uncovered by Scalar's Functional Tests.

I had tried to fix this in https://github.com/git-for-windows/pull/3241,
but the fix was incomplete, and I had to follow up with
https://github.com/git-for-windows/pull/3258. However, it turns out that
both of them were actually only work-arounds; I should have dug deeper
to figure out _why_ the `since_token` was no longer guaranteed not to be
`NULL`, and I finally did.

Signed-off-by: Johannes Schindelin <[email protected]>
Now that we have a correct fix where we guarantee again (just like
v1 of the built-in FSMonitor) that `since_token` is not `NULL`, we can
revert the work-arounds introduced by these two PRs:

- https://github.com/git-for-windows/pull/3241

- https://github.com/git-for-windows/pull/3258

Signed-off-by: Johannes Schindelin <[email protected]>
When flushing the FSMonitor data, we do not actually need to wait for a
cookie file. In the worst case, we will over-report a bit.

If we _do_ wait for a cookie file, in the worst case we cause a hang
because the FSMonitor daemon will wait and wait even though the `.git/`
directory might be gone already.

Signed-off-by: Johannes Schindelin <[email protected]>
When the built-in FSMonitor receives an unexpected token, we do not
actually need to wait for a cookie file. There simply is no use for
waiting in this instance. It's not like the client will all of a sudden
realize that it sent an incorrect token and somehow make up a correct
token from thin air in a follow-up query.

Signed-off-by: Johannes Schindelin <[email protected]>
When the built-in FSMonitor receives an invalid v2 token, we do not
actually need to wait for a cookie file. There simply is no use for
waiting in this instance. It's not like the client will all of a sudden
realize that it sent an incorrect token and somehow make up a correct
token from thin air in a follow-up query.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho requested a review from jeffhostetler June 9, 2021 13:50
@dscho dscho added this to the Next release milestone Jun 9, 2021
Copy link

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

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

LGTM. Checks out as the same commits from microsoft/git.

@dscho dscho merged commit b312fa4 into git-for-windows:main Jun 9, 2021
@dscho dscho deleted the fix-built-in-fsmonitor branch June 9, 2021 16:05
dscho added a commit to git-for-windows/build-extra that referenced this pull request Jun 9, 2021
The built-in file system watcher could hang in some scenarios. [This
was fixed](git-for-windows/git#3263).

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho mentioned this pull request Jun 21, 2021
@kashi1729
Copy link

when i start my "bash" it show cd: too many arguments

@kashi1729
Copy link

how to solve this

@derrickstolee
Copy link

when i start my "bash" it show cd: too many arguments

@jeffhostetler sounds like maybe an issue with spaces in arguments. While I don't think that is from this PR, perhaps you know where to look for such a possibility?

@dscho
Copy link
Member Author

dscho commented Jul 19, 2021

@kashi1729 while it would be better to open a new ticket rather than tacking on a question to an unrelated Pull Request, I will give you this pointer: you can trace the startup commands as described here.

@jeffhostetler
Copy link

Are you getting the error opening a new terminal window or when running a command within it?

If it is when open a terminal window, please look and see if you have a bad command line or something in your startup ~/.profile or ~/.bashrc that might be causing the error.

@kashi1729
Copy link

Are you getting the error opening a new terminal window or when running a command within it?

If it is when open a terminal window, please look and see if you have a bad command line or something in your startup ~/.profile or ~/.bashrc that might be causing the error.

no there is no bad command line and ~/.bashrc not working

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants