On 11/29/2021 2:22 PM, Elijah Newren wrote: > On Mon, Nov 29, 2021 at 9:50 AM Derrick Stolee <stolee@xxxxxxxxx> wrote: >> >> On 11/26/2021 5:40 PM, Elijah Newren via GitGitGadget wrote: >>> cmd.git_cmd = 1; >>> >>> + if (startup_info->original_cwd && >>> + !is_absolute_path(startup_info->original_cwd)) >>> + cmd.dir = startup_info->original_cwd; >> >> I was initially confused by the "!is_absolute_path()" because >> it seemed to me like it would be natural to store an absolute >> path there, but I see this comment in patch 2: >> >> + * For convience, we would like to have the path relative to the >> + * worktree instead of an absolute path. >> >> So it seems that we won't store it as an absolute path. Is >> there any value in this condition, then? > > Good catch. This is leftover from an earlier round; I'll remove it. > From stash too. > >> This assignment of cmd.dir to the relative path has a lot >> of baked-in knowledge of this variable _and_ the current >> state (Git chdir()'d to the root of the worktree). If the >> path is always relative, then it should be a BUG() if we >> see an absolute path. Also, it seems like we would want >> cmd.dir to be a concatenation of the worktree root and the >> original_cwd. >> >> Or perhaps I'm being overly cautious and this could all be >> resolved with a comment about the expected state of the >> working directory and original_cwd. The tests will catch if >> any of those expectations change. > > Yeah, with the extra condition in there I should have added a good > explanation about why I had it in there. But I think a simple > > if (startup_info->original_cwd) > cmd.dir = startup_info->original_cwd; > > should be good enough? Or do you still want a comment for that? This is the code I would expect to see. No comment needed. Thanks, -Stolee
