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

Make ZED_WORKTREE_ROOT always point to a directory or is not set #23150

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

aborg-dev
Copy link
Contributor

@aborg-dev aborg-dev commented Jan 14, 2025

See: #22912

When the out-of-tree file like tasks.json is selected, the ZED_WORKTREE_ROOT is now unset.

This fixes the immediate problem with ZED_WORKTREE_ROOT pointing to the file, but the desired long-term behavior is still under discussion.

At the moment, BasicContextProvider::build_context does not have any information about the active project, so there is no way to get the root directory of the active project without plumbing in more context.

Release Notes:

  • Fixed ZED_WORKTREE_ROOT incorrectly pointing to a file. Now points to a directory when current file is a project or unset when in out-of-project files (settings.json, tasks.json, etc).

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 14, 2025
@maxdeviant maxdeviant changed the title ZED_WORKTREE_ROOT now always points to a workspace root directory Make ZED_WORKTREE_ROOT always point to a workspace root directory Jan 14, 2025
@aborg-dev aborg-dev marked this pull request as ready for review January 15, 2025 20:30
@aborg-dev
Copy link
Contributor Author

@osiewicz , I've updated the PR with the behavior that you suggested. Will look into writing a test.

@aborg-dev aborg-dev changed the title Make ZED_WORKTREE_ROOT always point to a workspace root directory Make ZED_WORKTREE_ROOT always point to adirectory Jan 15, 2025
@aborg-dev aborg-dev changed the title Make ZED_WORKTREE_ROOT always point to adirectory Make ZED_WORKTREE_ROOT always point to a directory Jan 15, 2025
@aborg-dev
Copy link
Contributor Author

Now that I think about it, maybe when root_dir() is not well-defined, the ZED_WORKTREE_ROOT should be empty to be consistent with cwd?
I think this might be less confusing option for the user, at least until we don't align the behavior again to point to the active project root.

@aborg-dev
Copy link
Contributor Author

aborg-dev commented Jan 17, 2025

Now that I think about it, maybe when root_dir() is not well-defined, the ZED_WORKTREE_ROOT should be empty to be consistent with cwd? I think this might be less confusing option for the user, at least until we don't align the behavior again to point to the active project root.

I think this is the right approach, I've double-checked that on the detached files, the behavior is identical to the cwd, it is empty:

[crates/terminal_view/src/terminal_panel.rs:451:9] &spawn_in_terminal = SpawnInTerminal {
    id: TaskId(
        "oneshot_207ea101dedafe545a2e9eb5b80020e6b71a5b0d29fec56e4dd4488822939b51_1072e6503c4689620973fb491aee92e007207cde652ea674817aef523004b243",
    ),
    full_label: "echo $(pwd) && sleep 2",
    label: "echo $(pwd) && sleep 2",
    command: "echo $(pwd) && sleep 2",
    args: [],
    command_label: "echo $(pwd) && sleep 2",
    cwd: None,
    env: {
        "ZED_FILE": "/home/aborg/.config/zed/settings.json",
        "ZED_SYMBOL": "ui_font_size",
        "ZED_FILENAME": "settings.json",
        "ZED_ROW": "20",
        "ZED_COLUMN": "21",
        "ZED_DIRNAME": "/home/aborg/.config/zed",
        "ZED_STEM": "settings",
    },
    use_new_terminal: false,
    allow_concurrent_runs: false,
    reveal: Always,
    reveal_target: Dock,
    hide: Never,
    shell: System,
    show_summary: false,
    show_command: false,
}

I think any future improvements should change cwd and ZED_WORKTREE_ROOT in tandem.
Proving another placeholder value for ZED_WORKTREE_ROOT (e.g. the parent directory of ZED_FILE) would be creating a precedent for people to start relying on the temporary behavior.

@notpeter , does this make sense? If so, what are the next steps for landing this change?

@aborg-dev aborg-dev changed the title Make ZED_WORKTREE_ROOT always point to a directory Make ZED_WORKTREE_ROOT always point to a directory or be empty Jan 18, 2025
@aborg-dev aborg-dev changed the title Make ZED_WORKTREE_ROOT always point to a directory or be empty Make ZED_WORKTREE_ROOT always point to a directory or is not set Jan 18, 2025
@osiewicz
Copy link
Contributor

I think you're right in that cwd and ZED_WORKTREE_ROOT should be fixed in tandem. Let's land it as is.

@aborg-dev
Copy link
Contributor Author

Rebased the PR on the latest main.

@notpeter , can we land this PR now? Or is there anything else you would like to add?

@notpeter notpeter self-assigned this Jan 27, 2025
@zed-industries-bot
Copy link

Messages
📖

This PR includes links to the following GitHub Issues: #22912
If this PR aims to close an issue, please include a Closes #ISSUE line at the top of the PR body.

Generated by 🚫 dangerJS against f703e60

@notpeter notpeter merged commit 9f3dbd6 into zed-industries:main Jan 27, 2025
15 checks passed
@notpeter
Copy link
Member

Merged. Additional context to #22912 to reflect the updated state (unset var). Thanks @aborg-dev for the bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants