On Mon, Feb 12, 2018 at 2:37 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: >> Fix this by changing to the new worktree's directory before running >> the hook, and adjust the tests to verify that the hook is indeed run >> within the correct directory. > > I like the approach taken by this replacement better. Just to make > sure I understand the basic idea, let me rephrase what these two > patches are doing: > > - "path" that is made absolute in this step is where the new > worktree is created, i.e. the top-level of the working tree in > the new worktree. We chdir there and then run the hook script. Sorry for misleading. The "absolute path" stuff in this patch is unnecessary; it's probably just left-over from Lars's proposal which did need to make it absolute when setting GIT_WORK_TREE, and I likely didn't think hard enough to realize that it doesn't need to be absolute just for chdir(). I'll drop the unnecessary absolute_pathdup() in the re-roll. (The hook path in patch 1/2, on the other hand, does need to be made absolute since find_hook() returns a relative path before we've chdir()'d into the new worktree.) > - Even though we often see hooks executed inside .git/ directory, > for post-checkout, the top-level of the working tree is the right > place, as that is where the hook is run by "git checkout" [...] Patch 1/2's commit message is a bit sloppy in its description of this. I'll tighten it up in the re-roll. I'm also not fully convinced that these new overloads of run_hook_*() are warranted since it's hard to imagine any other case when they would be useful. It may make sense just to have builtin/worktree.c run the hook itself for this one-off case. > I wonder if we need to clear existing GIT_DIR/GIT_WORK_TREE from the > environment, though. When a user with a funny configuration (where > these two environment variables are pointing at unusual places) uses > "git worktree add" to create another worktree for the repository, it > would not be sufficient to chdir to defeat them that are appropriate > for the original, and not for the new, worktree, would it? Good point. I'll look into sanitizing the environment.
