* [PATCH] Make git log work for git CWD outside of work tree @ 2017-04-09 2:21 Danny Sauer 2017-04-09 10:54 ` Johannes Schindelin 0 siblings, 1 reply; 15+ messages in thread From: Danny Sauer @ 2017-04-09 2:21 UTC (permalink / raw) To: git, danny Make git log's `--use-mailmap` argument works if the GIT_DIR & GIT_WORK_TREE env vars are set and git is run from outside of work tree. Without the NEED_WORK_TREE set on the log subcommand, .mailmap is silently not found. Signed-off-by: Danny Sauer <danny@dannysauer.com> --- Notes: I'm not entirely sure if this is the best way to fix it, as my git internals knowledge is pretty weak. But this /seems/ reasonable to me, and passes all of the current test cases. If there's a more appropriate way to make `--use-mailmap` work properly when `git log` is run outside of the tree, I'd be excited about the opportunity to learn. :) git.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git.c b/git.c index 8ff44f0..e147f01 100644 --- a/git.c +++ b/git.c @@ -440,7 +440,7 @@ static struct cmd_struct commands[] = { { "init", cmd_init_db }, { "init-db", cmd_init_db }, { "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY }, - { "log", cmd_log, RUN_SETUP }, + { "log", cmd_log, RUN_SETUP | NEED_WORK_TREE }, { "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX }, { "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY }, { "ls-tree", cmd_ls_tree, RUN_SETUP }, -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-09 2:21 [PATCH] Make git log work for git CWD outside of work tree Danny Sauer @ 2017-04-09 10:54 ` Johannes Schindelin 2017-04-09 14:15 ` Danny Sauer 0 siblings, 1 reply; 15+ messages in thread From: Johannes Schindelin @ 2017-04-09 10:54 UTC (permalink / raw) To: Danny Sauer; +Cc: git Hi Danny, On Sat, 8 Apr 2017, Danny Sauer wrote: > Make git log's `--use-mailmap` argument works if the GIT_DIR & > GIT_WORK_TREE env vars are set and git is run from outside of work tree. > Without the NEED_WORK_TREE set on the log subcommand, .mailmap is > silently not found. A laudable goal. How about adding a test case, say, to t/t4203-mailmap.sh, to ensure that Git won't regress on your fix? > diff --git a/git.c b/git.c > index 8ff44f0..e147f01 100644 > --- a/git.c > +++ b/git.c > @@ -440,7 +440,7 @@ static struct cmd_struct commands[] = { > { "init", cmd_init_db }, > { "init-db", cmd_init_db }, > { "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY }, > - { "log", cmd_log, RUN_SETUP }, > + { "log", cmd_log, RUN_SETUP | NEED_WORK_TREE }, > { "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX }, > { "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY }, > { "ls-tree", cmd_ls_tree, RUN_SETUP }, This may work for you, but it does not work for me, as I often call `git log` in a bare repository. And that call works, and it should keep working. Instead, I think, you need to figure out why the .mailmap file is not read correctly when you use the GIT_DIR & GIT_WORK_TREE approach. My vague hunch is that you need to replace the ".mailmap" in read_mailmap() (defined in mailmap.c): err |= read_mailmap_file(map, ".mailmap", repo_abbrev); by something like mkpath("%s/%s", get_git_work_tree(), ".mailmap"), but probably only after testing that we're not in a bare repository (which would also fix a bug, I suspect, as `git log` in a bare repository probably heeds a .mailmap file in the current directory, which is incorrect). I.e. something like: if (!is_bare_repository()) { const char *path = mkpath("%s/%s", get_git_work_tree(), ".mailmap") err |= read_mailmap_file(map, path, repo_abbrev); } But you really want to add the test case first, with `test_expect_failure`, to demonstrate what is currently broken, and then triumphantly setting it to `test_expect_success` with your patch. Ciao, Johannes ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-09 10:54 ` Johannes Schindelin @ 2017-04-09 14:15 ` Danny Sauer 2017-04-10 0:21 ` Junio C Hamano 0 siblings, 1 reply; 15+ messages in thread From: Danny Sauer @ 2017-04-09 14:15 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin On 4/9/2017 5:54 AM, Johannes Schindelin wrote: > Hi Danny, > > On Sat, 8 Apr 2017, Danny Sauer wrote: >> diff --git a/git.c b/git.c >> index 8ff44f0..e147f01 100644 >> --- a/git.c >> +++ b/git.c >> @@ -440,7 +440,7 @@ static struct cmd_struct commands[] = { >> { "init", cmd_init_db }, >> { "init-db", cmd_init_db }, >> { "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY }, >> - { "log", cmd_log, RUN_SETUP }, >> + { "log", cmd_log, RUN_SETUP | NEED_WORK_TREE }, >> { "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX }, >> { "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY }, >> { "ls-tree", cmd_ls_tree, RUN_SETUP }, > This may work for you, but it does not work for me, as I often call `git > log` in a bare repository. And that call works, and it should keep > working. I was wondering about that situation, but honestly I wasn't positive how to test it and sort of just went on the hope that the read_mailmap_blob mechanism would continue handling it. So, I guess there's another test case which would be useful, as passing the test cases is what gave me that undeserved confidence. :) And I'm going to need to read a bit more to figure out what a bare repository actually is... > Instead, I think, you need to figure out why the .mailmap file is not read > correctly when you use the GIT_DIR & GIT_WORK_TREE approach. My vague > hunch is that you need to replace the ".mailmap" in read_mailmap() > (defined in mailmap.c): > > err |= read_mailmap_file(map, ".mailmap", repo_abbrev); > > by something like mkpath("%s/%s", get_git_work_tree(), ".mailmap"), but > probably only after testing that we're not in a bare repository (which > would also fix a bug, I suspect, as `git log` in a bare repository > probably heeds a .mailmap file in the current directory, which is > incorrect). I.e. something like: > > if (!is_bare_repository()) { > const char *path = mkpath("%s/%s", > get_git_work_tree(), ".mailmap") > err |= read_mailmap_file(map, path, repo_abbrev); > } That's feedback I was looking for - what's the "right" way to get the path to the file. The call as-is works in any subdirectory of a repository, because there's some logic buried along the way which puts you in the top level of the repo if you started underneath. But if you're outside, it does the same chdir to verify that the top level is a real repo and then returns you to where you started (which is perfectly reasonable). In comparing how shortlog works v/s log, the main difference I could see is the use of RUN_SETUP_GENTLY instead of RUN_SETUP as the flag, which itself seems to only differ by passing in a true value for nongit_ok in run_setup, ultimately leading to "some different directory checks" - which I somewhat got lost in due partially to not fully knowing what a bare repository is. :) But changing that didn't make it work in my (poorly-formalized) testing. I'm on-board with a proper test case being the first situation which needs rectifying. It's off to learn stuff for me, then. Thanks! --Danny ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-09 14:15 ` Danny Sauer @ 2017-04-10 0:21 ` Junio C Hamano 2017-04-10 12:01 ` Duy Nguyen 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2017-04-10 0:21 UTC (permalink / raw) To: Danny Sauer; +Cc: git, Johannes Schindelin Danny Sauer <danny@dannysauer.com> writes: > That's feedback I was looking for - what's the "right" way to get the > path to the file. My knee-jerk reaction is that you SHOULDN'T have to do anything special to "get the path". I do not have the code in front of me (this is my "down" week after all), so let me give the general outline of how to reason about this, with the knowledge of code evolution of the system. In the beginning, Git worked only from the top level of the working tree. If you have your project in ~/myproject with a file "f" in a directory "d", that file is ~/myproject/d/f", and you would do $ cd ~/myproject $ git add d/f to add it. If Git wanted to access the index, it just accessed ".git/index" as a relative path. If Git wanted to access a file at the top of the working tree, say ".gitignore", it just accessed ".gitignore" as a relatlvei path. Because it only worked from the top. Then we added a support for running Git from a subdirectory, so that you can say "cd ~/myproject/d && git add f". In order to keep the existing code that wants to access the index as ".git/index" and wants to access the top-level ".gitignore" as ".gitignore" working, the support to run Git from a subdirectory is designed this way: - Each main program of subcommand (e.g. cmd_log()) receives a parameter "prefix", which tells what subdirectory you were in when you started Git. - Before running the main program of subcommand, Git chdir(2)s up to the top level of the working tree. - The main program of subcommand receives the command line from the user intact. It is responsible for prepending the prefix to the path the user gave it from the command line. So if you did "cd ~/myproject/d && git add f", Git goes up to "~/myproject", passes argv=("f", NULL) and prefix="d/" to cmd_add(). Adding to the index wants to read and update the index, which is still done by opening ".git/index" (relative to the toplevel as before), and inspecting the top-level .gitigore file is done by opening ".gitignore" (relative to the toplevel as before). And the cmd_add() forms the path "d/f" by using the prefix "d/" and the user-supplied pathname "f". When we first added a support for having the (equivalent of) ".git/" directory outside the working tree by setting GIT_DIR environment variable, again, you can only use Git from the top-level of the working tree. Instead of "~/myproject/.git", you can keep your repository metainformation in say "~/mypro.git/" and point at it with GIT_DIR environment variable, and said $ export GIT_DIR=~/mypro.git $ cd ~/myproject $ git add d/f Later we also added GIT_WORK_TREE environment variable to be used together with GIT_DIR so that you can start from ~/myproject/d, very similarly to how you worked from a subdirectory without these environment variables, i.e. $ export GIT_DIR=~/mypro.git GIT_WORK_TREE=~/myproject $ cd ~/myproject/d $ git add f The way this support was added was to go to the top-level of the working tree (i.e. "~/myproject" in this example) and passing the prefix (i.e. "d" in this example). Notice that in all of the above configurations, if a Git command knows a path to something that is relative to the top of the working tree (e.g. ".git/index" in the ancient Git, ".gitignore" at the top-level that governs the entire working tree, or ".mailmap"), it should just be able to open that path without asking "where is the top of the working tree?". So if your directory arrangement is a variation of these basic configurations supported, e.g. if your git directory is ~/myproject/.git and your working tree is ~/myproject, and you use the GIT_DIR and GIT_WORK_TREE to point at them, regardless of which subdirectory of $GIT_WORK_TREE you started with, Git should already have done chdir(2) to ~/myproject/ before it starts cmd_log(), and it should be able to just open and read ".mailmap" (of course, it needs to limit itself to do so only when it is working with a repository with a working tree). If your arrangement is even more exotic, e.g. you have these two variables set, and then are running from OUTSIDE the working tree, my knee-jerk reaction is that you should get your head examined, as it is totally unclear what "git add foo" would mean if you did this: $ export GIT_DIR=~/myproject/.git GIT_WORK_TREE=~/myproject $ cd ~/myproject/../somewhere/else $ git add foo But it should still "work" in the sense that the above command should notice that you are trying to add "../somewhere/else/foo" to the index, which is obviously nonsense, and die with a message. Similarly, if you replace "git add foo" with "git log", it still should work in the above, i.e. $ export GIT_DIR=~/myproject/.git GIT_WORK_TREE=~/myproject $ cd ~/myproject/../somewhere/else $ git log If Git is not chdir(2)ing to ~/myproject before calling cmd_log() in the above (again, this is my down week so I didn't and will not check with the code myself), we may want to call that a bug and fix it, so that you do not have to do anything special to get to the path of ".mailmap" that is at the top-level. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-10 0:21 ` Junio C Hamano @ 2017-04-10 12:01 ` Duy Nguyen 2017-04-10 17:13 ` Jeff King 0 siblings, 1 reply; 15+ messages in thread From: Duy Nguyen @ 2017-04-10 12:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: Danny Sauer, Git Mailing List, Johannes Schindelin On Mon, Apr 10, 2017 at 7:21 AM, Junio C Hamano <gitster@pobox.com> wrote: > If your arrangement is even more exotic, e.g. you have these two > variables set, and then are running from OUTSIDE the working tree, > my knee-jerk reaction is that you should get your head examined, as > it is totally unclear what "git add foo" would mean if you did this: > > $ export GIT_DIR=~/myproject/.git GIT_WORK_TREE=~/myproject > $ cd ~/myproject/../somewhere/else > $ git add foo > > But it should still "work" in the sense that the above command > should notice that you are trying to add "../somewhere/else/foo" to > the index, which is obviously nonsense, and die with a message. > > Similarly, if you replace "git add foo" with "git log", it still > should work in the above, i.e. > > $ export GIT_DIR=~/myproject/.git GIT_WORK_TREE=~/myproject > $ cd ~/myproject/../somewhere/else > $ git log > > If Git is not chdir(2)ing to ~/myproject before calling cmd_log() > in the above (again, this is my down week so I didn't and will not > check with the code myself), we may want to call that a bug and fix > it, so that you do not have to do anything special to get to the > path of ".mailmap" that is at the top-level. The behavior is "documented" in t1510 since f3bb8b4b84 (Merge branch 'nd/setup' - 2010-12-28) "11. When user's cwd is outside worktree, cwd remains unchanged, prefix is NULL." This behavior probably started long before my topic though, mine was more of documentation, making worktree detection more consistent. It's the same case with defining GIT_DIR without GIT_WORK_TREE, I think: scripts started to depend on a behavior that we did not clearly define, by the time we knew what we wanted and we kept the old behavior forever. I think it's just safer to go with Johannes' suggestion. An alternative is, when you have found out you need to read .mailmap, you call setup_work_tree() then, which prepares the worktree for you (including moving back to cwd) or dies if worktree does not exist, or no-op if worktree has already been asked by somebody. Many commands do lazy worktree initialization this way. -- Duy ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-10 12:01 ` Duy Nguyen @ 2017-04-10 17:13 ` Jeff King 2017-04-12 6:30 ` Duy Nguyen 0 siblings, 1 reply; 15+ messages in thread From: Jeff King @ 2017-04-10 17:13 UTC (permalink / raw) To: Duy Nguyen Cc: Junio C Hamano, Danny Sauer, Git Mailing List, Johannes Schindelin On Mon, Apr 10, 2017 at 07:01:00PM +0700, Duy Nguyen wrote: > > Similarly, if you replace "git add foo" with "git log", it still > > should work in the above, i.e. > > > > $ export GIT_DIR=~/myproject/.git GIT_WORK_TREE=~/myproject > > $ cd ~/myproject/../somewhere/else > > $ git log > > > > If Git is not chdir(2)ing to ~/myproject before calling cmd_log() > > in the above (again, this is my down week so I didn't and will not > > check with the code myself), we may want to call that a bug and fix > > it, so that you do not have to do anything special to get to the > > path of ".mailmap" that is at the top-level. > > The behavior is "documented" in t1510 since f3bb8b4b84 (Merge branch > 'nd/setup' - 2010-12-28) > > "11. When user's cwd is outside worktree, cwd remains unchanged, > prefix is NULL." > > This behavior probably started long before my topic though, mine was > more of documentation, making worktree detection more consistent. It's > the same case with defining GIT_DIR without GIT_WORK_TREE, I think: > scripts started to depend on a behavior that we did not clearly > define, by the time we knew what we wanted and we kept the old > behavior forever. > > I think it's just safer to go with Johannes' suggestion. > > An alternative is, when you have found out you need to read .mailmap, > you call setup_work_tree() then, which prepares the worktree for you > (including moving back to cwd) or dies if worktree does not exist, or > no-op if worktree has already been asked by somebody. Many commands do > lazy worktree initialization this way. I think this is much more than just .mailmap, though. For instance, I have noticed a similar problem with .gitattributes: # set up a repo with an attribute git init repo ( cd repo && echo content >file && echo "file diff=sed" >.gitattributes && git add . && git commit -m foo && git config diff.sed.textconv 'sed s/^/foo:/' ) If I run: (cd repo && git log -p) the diff for "file" shows: +foo:content as expected. But: GIT_WORK_TREE=$PWD/repo GIT_DIR=$PWD/repo/.git git log -p doesn't (while writing this I also noticed that "git log -p file" doesn't work because of the DWIM lookup. That one is more debatable, but I still think it makes more sense to move to $GIT_WORK_TREE). -Peff ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-10 17:13 ` Jeff King @ 2017-04-12 6:30 ` Duy Nguyen 2017-04-12 8:41 ` Junio C Hamano 2017-04-12 12:53 ` Jeff King 0 siblings, 2 replies; 15+ messages in thread From: Duy Nguyen @ 2017-04-12 6:30 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Danny Sauer, Git Mailing List, Johannes Schindelin On Tue, Apr 11, 2017 at 12:13 AM, Jeff King <peff@peff.net> wrote: > On Mon, Apr 10, 2017 at 07:01:00PM +0700, Duy Nguyen wrote: >> An alternative is, when you have found out you need to read .mailmap, >> you call setup_work_tree() then, which prepares the worktree for you >> (including moving back to cwd) or dies if worktree does not exist, or >> no-op if worktree has already been asked by somebody. Many commands do >> lazy worktree initialization this way. > > I think this is much more than just .mailmap, though. For instance, I > have noticed a similar problem with .gitattributes: Urgh. assuming that we should not read .gitattributes if there's no worktree to read from (similar to the "defaults to .git" situation), how about - if mailmap stuff is requested, setup worktree, or die trying - if worktree is detected, but setup code does not jump to it, do it - if no worktree is detected, tell git-log to stop reading .gitattributes We probablly want some "if no wotktree then die()" in .gitattributes and .gitignore code, just in case it's incorrectly and accidentally executed in exotic setup -- Duy ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-12 6:30 ` Duy Nguyen @ 2017-04-12 8:41 ` Junio C Hamano 2017-04-12 11:13 ` Duy Nguyen 2017-04-12 12:53 ` Jeff King 1 sibling, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2017-04-12 8:41 UTC (permalink / raw) To: Duy Nguyen; +Cc: Jeff King, Danny Sauer, Git Mailing List, Johannes Schindelin Duy Nguyen <pclouds@gmail.com> writes: >> I think this is much more than just .mailmap, though. For instance, I >> have noticed a similar problem with .gitattributes: > > Urgh. assuming that we should not read .gitattributes if there's no > worktree to read from (similar to the "defaults to .git" situation), > how about > > - if mailmap stuff is requested, setup worktree, or die trying > - if worktree is detected, but setup code does not jump to it, do it > - if no worktree is detected, tell git-log to stop reading .gitattributes My gut reaction is that we are doing something wrong once we start saying "if mailmap stuff is requested". This is not about .mailmap but is about how sanely the paths relative to the root of the working tree (which includes a path in the index, or comparing $commit:$path with $path in the working tree) can be named by any subcommand of Git. Can't we model this after how setup_git_directory_gently() gives the subcommands a choice? While the majority of subcommands do not call the _gently() variant and die when we are not in a repository, the rest do use it and continue after learning that they are outside a repository. Perhaps we want a new bit GOTO_WORK_TREE_ROOT that is orthogonal to NEED_WORK_TREE to tell the codepath that calls cmd_foo() to always move to the root of the working tree (if there is one), regaredless of the behaviour f3bb8b4b84 documents. I have a strong suspicion that we didn't _care_ about a silly use case where GIT_WORK_TREE is specified and the command is started from somewhere completely unrelated to that location, and the users with such a silly use case didn't care either what Git does to the files in the working tree, i.e. what you quoted in your previous message "11. When user's cwd is outside worktree, cwd remains unchanged, prefix is NULL." This behavior probably started long before my topic though, mine was more of documentation, making worktree detection more consistent. It's was not describing the design, but just describing whatever random thing the code happened to be doing. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-12 8:41 ` Junio C Hamano @ 2017-04-12 11:13 ` Duy Nguyen 2017-04-12 13:01 ` Jeff King 0 siblings, 1 reply; 15+ messages in thread From: Duy Nguyen @ 2017-04-12 11:13 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Danny Sauer, Git Mailing List, Johannes Schindelin On Wed, Apr 12, 2017 at 3:41 PM, Junio C Hamano <gitster@pobox.com> wrote: > Duy Nguyen <pclouds@gmail.com> writes: > >>> I think this is much more than just .mailmap, though. For instance, I >>> have noticed a similar problem with .gitattributes: >> >> Urgh. assuming that we should not read .gitattributes if there's no >> worktree to read from (similar to the "defaults to .git" situation), >> how about >> >> - if mailmap stuff is requested, setup worktree, or die trying >> - if worktree is detected, but setup code does not jump to it, do it >> - if no worktree is detected, tell git-log to stop reading .gitattributes > > My gut reaction is that we are doing something wrong once we start > saying "if mailmap stuff is requested". This is not about .mailmap > but is about how sanely the paths relative to the root of the > working tree (which includes a path in the index, or comparing > $commit:$path with $path in the working tree) can be named by any > subcommand of Git. I probably should phrased that as "if any worktree stuff is requested". I was under an impression that you need to pass a command line option to use mailmap, but I dind't check the code > Can't we model this after how setup_git_directory_gently() gives the > subcommands a choice? While the majority of subcommands do not call > the _gently() variant and die when we are not in a repository, the > rest do use it and continue after learning that they are outside a > repository. It may work, but we need to be careful because paths as command line arguments will not be relative to cwd anymore. If some code assumes cwd unchanged, they're in trouble. I guess they're in trouble either way because of the "sometimes chdir, sometimes not" current behavior. > Perhaps we want a new bit GOTO_WORK_TREE_ROOT that is orthogonal to > NEED_WORK_TREE to tell the codepath that calls cmd_foo() to always > move to the root of the working tree (if there is one), regaredless > of the behaviour f3bb8b4b84 documents. I have a strong suspicion > that we didn't _care_ about a silly use case where GIT_WORK_TREE is > specified and the command is started from somewhere completely > unrelated to that location, and the users with such a silly use case > didn't care either what Git does to the files in the working tree, > i.e. what you quoted in your previous message > > "11. When user's cwd is outside worktree, cwd remains unchanged, > prefix is NULL." > > This behavior probably started long before my topic though, mine was > more of documentation, making worktree detection more consistent. It's > > was not describing the design, but just describing whatever random > thing the code happened to be doing. I never said otherwise :) The only thing I was worried was how long it had behaved that way, long enough that scripts started to depend on it? If we can get rid of special cases in setup code, I will be the first one to be happier. -- Duy ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-12 11:13 ` Duy Nguyen @ 2017-04-12 13:01 ` Jeff King 2017-04-12 13:11 ` Duy Nguyen 0 siblings, 1 reply; 15+ messages in thread From: Jeff King @ 2017-04-12 13:01 UTC (permalink / raw) To: Duy Nguyen Cc: Junio C Hamano, Danny Sauer, Git Mailing List, Johannes Schindelin On Wed, Apr 12, 2017 at 06:13:49PM +0700, Duy Nguyen wrote: > > Can't we model this after how setup_git_directory_gently() gives the > > subcommands a choice? While the majority of subcommands do not call > > the _gently() variant and die when we are not in a repository, the > > rest do use it and continue after learning that they are outside a > > repository. > > It may work, but we need to be careful because paths as command line > arguments will not be relative to cwd anymore. If some code assumes > cwd unchanged, they're in trouble. I guess they're in trouble either > way because of the "sometimes chdir, sometimes not" current behavior. Exactly. The code itself must respect "prefix", and if it doesn't, it's broken. So I don't think a code switch makes any sense here. It's not the code which needs to care, it's the user who might be relying on the externally visible behavior. I can't think of a case where the existing behavior actually makes sense for the user, though. They've provided a $GIT_WORK_TREE, but somehow want their out-of-worktree relative paths to still work? What are they using the paths for? Surely: cd /out-of-tree echo content >foo GIT_DIR=/repo/.git GIT_WORK_TREE=/repo git add foo shouldn't work? Likewise reading .mailmap and .gitattributes from /out-of-tree is simply a bug. I dunno. Maybe I am missing some subtle case, but it's not clear to me what the user would be trying to do by having git stay in the original directory. -Peff ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-12 13:01 ` Jeff King @ 2017-04-12 13:11 ` Duy Nguyen 2017-04-13 21:29 ` Jeff King 0 siblings, 1 reply; 15+ messages in thread From: Duy Nguyen @ 2017-04-12 13:11 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Danny Sauer, Git Mailing List, Johannes Schindelin On Wed, Apr 12, 2017 at 8:01 PM, Jeff King <peff@peff.net> wrote: > I dunno. Maybe I am missing some subtle case, but it's not clear to me > what the user would be trying to do by having git stay in the original > directory. Not sure if it really happens. But if we jump from worktree is inside original cwd and we have to jump to worktree, we set empty prefix, not "../../../" one. So we can't refer back to files relative to original cwd with prefix. I was kinda hoping "super prefix" would solve it, but that one seems designed specifically for submodules. -- Duy ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-12 13:11 ` Duy Nguyen @ 2017-04-13 21:29 ` Jeff King 2017-04-17 0:41 ` Junio C Hamano 2017-04-17 10:29 ` Duy Nguyen 0 siblings, 2 replies; 15+ messages in thread From: Jeff King @ 2017-04-13 21:29 UTC (permalink / raw) To: Duy Nguyen Cc: Junio C Hamano, Danny Sauer, Git Mailing List, Johannes Schindelin On Wed, Apr 12, 2017 at 08:11:22PM +0700, Duy Nguyen wrote: > On Wed, Apr 12, 2017 at 8:01 PM, Jeff King <peff@peff.net> wrote: > > I dunno. Maybe I am missing some subtle case, but it's not clear to me > > what the user would be trying to do by having git stay in the original > > directory. > > Not sure if it really happens. But if we jump from worktree is inside > original cwd and we have to jump to worktree, we set empty prefix, not > "../../../" one. So we can't refer back to files relative to original > cwd with prefix. I was kinda hoping "super prefix" would solve it, but > that one seems designed specifically for submodules. Ah, right. I think the issue is that "prefix" really serves two uses. For things like command-line arguments, we use to find the original path after we've moved. But we also use it for "where in the working tree are we". So with an out-of-tree cwd, we'd want to set the first one to the real path ("../../whatever" or even just an absolute path), but the second one would probably be empty (i.e., just pretend that we started from the top-level). But that would require first refactoring all of the cmd_* functions to handle those prefixes separately. -Peff ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-13 21:29 ` Jeff King @ 2017-04-17 0:41 ` Junio C Hamano 2017-04-17 10:29 ` Duy Nguyen 1 sibling, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2017-04-17 0:41 UTC (permalink / raw) To: Jeff King; +Cc: Duy Nguyen, Danny Sauer, Git Mailing List, Johannes Schindelin Jeff King <peff@peff.net> writes: >> cwd with prefix. I was kinda hoping "super prefix" would solve it, but >> that one seems designed specifically for submodules. > > Ah, right. I think the issue is that "prefix" really serves two uses. > For things like command-line arguments, we use to find the original path > after we've moved. But we also use it for "where in the working tree > are we". > > So with an out-of-tree cwd, we'd want to set the first one to the real > path ("../../whatever" or even just an absolute path), but the second > one would probably be empty (i.e., just pretend that we started from the > top-level). > > But that would require first refactoring all of the cmd_* functions to > handle those prefixes separately. Yes, I am still kinda hoping that the "super prefix" thing will address the separation of the two uses, though. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-13 21:29 ` Jeff King 2017-04-17 0:41 ` Junio C Hamano @ 2017-04-17 10:29 ` Duy Nguyen 1 sibling, 0 replies; 15+ messages in thread From: Duy Nguyen @ 2017-04-17 10:29 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Danny Sauer, Git Mailing List, Johannes Schindelin On Fri, Apr 14, 2017 at 4:29 AM, Jeff King <peff@peff.net> wrote: > On Wed, Apr 12, 2017 at 08:11:22PM +0700, Duy Nguyen wrote: > >> On Wed, Apr 12, 2017 at 8:01 PM, Jeff King <peff@peff.net> wrote: >> > I dunno. Maybe I am missing some subtle case, but it's not clear to me >> > what the user would be trying to do by having git stay in the original >> > directory. >> >> Not sure if it really happens. But if we jump from worktree is inside >> original cwd and we have to jump to worktree, we set empty prefix, not >> "../../../" one. So we can't refer back to files relative to original >> cwd with prefix. I was kinda hoping "super prefix" would solve it, but >> that one seems designed specifically for submodules. > > Ah, right. I think the issue is that "prefix" really serves two uses. > For things like command-line arguments, we use to find the original path > after we've moved. But we also use it for "where in the working tree > are we". > > So with an out-of-tree cwd, we'd want to set the first one to the real > path ("../../whatever" or even just an absolute path), but the second > one would probably be empty (i.e., just pretend that we started from the > top-level). > > But that would require first refactoring all of the cmd_* functions to > handle those prefixes separately. Yeah. I probably shouldn't start another series until all others of mine settle. But if anyone is changing cmd_*, I suggest we take this opportunity to pass "struct struct startup_info *" in the as the only option. argv/argv could be stored there as well. And you can add "cwd_prefix" to present that "../../whatever" (I would avoid the name "super prefix" until things settle), or just keep the original "cwd" in there and let people do whatever they want with it. -- Duy ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Make git log work for git CWD outside of work tree 2017-04-12 6:30 ` Duy Nguyen 2017-04-12 8:41 ` Junio C Hamano @ 2017-04-12 12:53 ` Jeff King 1 sibling, 0 replies; 15+ messages in thread From: Jeff King @ 2017-04-12 12:53 UTC (permalink / raw) To: Duy Nguyen Cc: Junio C Hamano, Danny Sauer, Git Mailing List, Johannes Schindelin On Wed, Apr 12, 2017 at 01:30:31PM +0700, Duy Nguyen wrote: > On Tue, Apr 11, 2017 at 12:13 AM, Jeff King <peff@peff.net> wrote: > > On Mon, Apr 10, 2017 at 07:01:00PM +0700, Duy Nguyen wrote: > >> An alternative is, when you have found out you need to read .mailmap, > >> you call setup_work_tree() then, which prepares the worktree for you > >> (including moving back to cwd) or dies if worktree does not exist, or > >> no-op if worktree has already been asked by somebody. Many commands do > >> lazy worktree initialization this way. > > > > I think this is much more than just .mailmap, though. For instance, I > > have noticed a similar problem with .gitattributes: > > Urgh. assuming that we should not read .gitattributes if there's no > worktree to read from (similar to the "defaults to .git" situation), > how about > > - if mailmap stuff is requested, setup worktree, or die trying > - if worktree is detected, but setup code does not jump to it, do it > - if no worktree is detected, tell git-log to stop reading .gitattributes > > We probablly want some "if no wotktree then die()" in .gitattributes > and .gitignore code, just in case it's incorrectly and accidentally > executed in exotic setup I didn't check what we do with attributes when there is no worktree. The behavior you describe above sounds right. But note that in my test we _do_ have a worktree, but just look in the wrong location. So doing a chdir to $GIT_WORK_TREE would just work. -Peff ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-04-17 10:30 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-09 2:21 [PATCH] Make git log work for git CWD outside of work tree Danny Sauer 2017-04-09 10:54 ` Johannes Schindelin 2017-04-09 14:15 ` Danny Sauer 2017-04-10 0:21 ` Junio C Hamano 2017-04-10 12:01 ` Duy Nguyen 2017-04-10 17:13 ` Jeff King 2017-04-12 6:30 ` Duy Nguyen 2017-04-12 8:41 ` Junio C Hamano 2017-04-12 11:13 ` Duy Nguyen 2017-04-12 13:01 ` Jeff King 2017-04-12 13:11 ` Duy Nguyen 2017-04-13 21:29 ` Jeff King 2017-04-17 0:41 ` Junio C Hamano 2017-04-17 10:29 ` Duy Nguyen 2017-04-12 12:53 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.