Re: rev-parse --show-toplevel broken during exec'ed rebase?
SZEDER Gábor writes: >> Forgetting the code in git-sh-setup, are we? >> >> git_dir_init() rather specifically set GIT_DIR to the absolute path, and >> since that variable is already exported, the `exec` commands launched via >> `git-rebase--interactive` all saw it. >> >> That is the reason why the sequencer.c was taught to set GIT_DIR to an >> absolute path rathern than not setting it: for backwards compatibility. > > GIT_DIR was not exported to 'exec' commands during an interactive > rebase prior to 18633e1a22 (rebase -i: use the rebase--helper builtin, > 2017-02-09) (nor was GIT_PREFIX): > > $ git log -Sgit_dir_init master git-rebase*.sh > # Nothing. > $ git checkout 18633e1a22a6^ && make -j4 prefix=/tmp/BEFORE install > <> > $ git checkout 18633e1a22a6 && make -j4 prefix=/tmp/AFTER install > <> > $ GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"' /tmp/BEFORE/bin/git rebase -i > HEAD^ > Executing: set |grep ^GIT > GIT_CHERRY_PICK_HELP=$'\nWhen you have resolved this problem, run "git > rebase --continue".\nIf you prefer to skip this patch, run "git rebase > --skip" instead.\nTo check out the original branch and stop rebasing, run > "git rebase --abort".\n' > GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"' > GIT_INTERNAL_GETTEXT_SH_SCHEME=gnu > GIT_REFLOG_ACTION='rebase -i (start): checkout HEAD^' > warning: notes ref refs/notes/commits is invalid > Successfully rebased and updated refs/heads/master. > $ GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"' /tmp/AFTER/bin/git rebase -i > HEAD^ > Executing: set |grep ^GIT > GIT_CHERRY_PICK_HELP=' > GIT_DIR='.git' > GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"' > GIT_INTERNAL_GETTEXT_SH_SCHEME='gnu' > GIT_PREFIX='' > GIT_REFLOG_ACTION='rebase -i (start): checkout HEAD^' > warning: notes ref refs/notes/commits is invalid > Successfully rebased and updated refs/heads/master. > > And then recently came 226c0ddd0d (exec_cmd: RUNTIME_PREFIX on some > POSIX systems, 2018-04-10), which then started to export GIT_EXEC_PATH > to 'exec' commands as well... Correct. git-sh-setup does assign to GIT_DIR, but unless the end-user has it exported, it does not export, which has long been very deliberate. But it does not really matter, as exporting BOTH GIT_DIR and GIT_WORK_TREE is a lot easier to understand and probably safe (even though technically a regressing) solution, and fewer and fewer things will be relying on git-sh-setup in the future anyway.
Re: rev-parse --show-toplevel broken during exec'ed rebase?
> On Mon, 16 Jul 2018, Junio C Hamano wrote: > > > Jeff King writes: > > > > > None of which is too surprising. The root of the bug is in the > > > conversion to rebase--helper, I think, when presumably we started > > > setting GIT_DIR at all (but I didn't dig further). Then 09d7b6c6fa fixed > > > _one_ fallout of that, which was relative paths, but didn't help the > > > subdirectory case. > > > > > > Just reading over this thread, I suspect the simplest fix is to pass > > > GIT_DIR and GIT_WORK_TREE together, which is almost always the right > > > thing to do. > > > > Perhaps. Not exporting GIT_DIR (unless the end-user already did to > > the environment before starting "git rebase"---it would be a bad > > change to unexport it unconditionally) may probably be a way to make > > rebase--helper conversion more faithful to the original scripted > > Porcelain, but I suspect in practice always giving GIT_DIR and > > GIT_WORK_TREE would work well for many existing hooks. > > Forgetting the code in git-sh-setup, are we? > > git_dir_init() rather specifically set GIT_DIR to the absolute path, and > since that variable is already exported, the `exec` commands launched via > `git-rebase--interactive` all saw it. > > That is the reason why the sequencer.c was taught to set GIT_DIR to an > absolute path rathern than not setting it: for backwards compatibility. GIT_DIR was not exported to 'exec' commands during an interactive rebase prior to 18633e1a22 (rebase -i: use the rebase--helper builtin, 2017-02-09) (nor was GIT_PREFIX): $ git log -Sgit_dir_init master git-rebase*.sh # Nothing. $ git checkout 18633e1a22a6^ && make -j4 prefix=/tmp/BEFORE install <> $ git checkout 18633e1a22a6 && make -j4 prefix=/tmp/AFTER install <> $ GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"' /tmp/BEFORE/bin/git rebase -i HEAD^ Executing: set |grep ^GIT GIT_CHERRY_PICK_HELP=$'\nWhen you have resolved this problem, run "git rebase --continue".\nIf you prefer to skip this patch, run "git rebase --skip" instead.\nTo check out the original branch and stop rebasing, run "git rebase --abort".\n' GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"' GIT_INTERNAL_GETTEXT_SH_SCHEME=gnu GIT_REFLOG_ACTION='rebase -i (start): checkout HEAD^' warning: notes ref refs/notes/commits is invalid Successfully rebased and updated refs/heads/master. $ GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"' /tmp/AFTER/bin/git rebase -i HEAD^ Executing: set |grep ^GIT GIT_CHERRY_PICK_HELP=' GIT_DIR='.git' GIT_EDITOR='sed -i -e "1ix set |grep ^GIT"' GIT_INTERNAL_GETTEXT_SH_SCHEME='gnu' GIT_PREFIX='' GIT_REFLOG_ACTION='rebase -i (start): checkout HEAD^' warning: notes ref refs/notes/commits is invalid Successfully rebased and updated refs/heads/master. And then recently came 226c0ddd0d (exec_cmd: RUNTIME_PREFIX on some POSIX systems, 2018-04-10), which then started to export GIT_EXEC_PATH to 'exec' commands as well...
Re: rev-parse --show-toplevel broken during exec'ed rebase?
Hi Junio, On Mon, 16 Jul 2018, Junio C Hamano wrote: > Jeff King writes: > > > None of which is too surprising. The root of the bug is in the > > conversion to rebase--helper, I think, when presumably we started > > setting GIT_DIR at all (but I didn't dig further). Then 09d7b6c6fa fixed > > _one_ fallout of that, which was relative paths, but didn't help the > > subdirectory case. > > > > Just reading over this thread, I suspect the simplest fix is to pass > > GIT_DIR and GIT_WORK_TREE together, which is almost always the right > > thing to do. > > Perhaps. Not exporting GIT_DIR (unless the end-user already did to > the environment before starting "git rebase"---it would be a bad > change to unexport it unconditionally) may probably be a way to make > rebase--helper conversion more faithful to the original scripted > Porcelain, but I suspect in practice always giving GIT_DIR and > GIT_WORK_TREE would work well for many existing hooks. Forgetting the code in git-sh-setup, are we? git_dir_init() rather specifically set GIT_DIR to the absolute path, and since that variable is already exported, the `exec` commands launched via `git-rebase--interactive` all saw it. That is the reason why the sequencer.c was taught to set GIT_DIR to an absolute path rathern than not setting it: for backwards compatibility. Ciao, Dscho
Re: rev-parse --show-toplevel broken during exec'ed rebase?
Jeff King writes: > On Mon, Jul 16, 2018 at 11:14:51AM -0700, Junio C Hamano wrote: > >> Porcelain, but I suspect in practice always giving GIT_DIR and >> GIT_WORK_TREE would work well for many existing hooks. > > Yeah, that may be an option. I don't remember if this was discussed in > this thread or elsewhere, but setting GIT_DIR is a regression for hooks, > etc, which do: > > git -C /some/other/repo log > > or similar. I'm not sure if that falls under "actual regression" or > "just how it happened to work in the past". I'm not even sure it worked > that way _consistently_ in the past. > > The best practice if you're switching directories is to do: > > unset $(git rev-parse --local-env-vars) > > though I highly doubt that most people bother. Yeah, that is exactly where my "in practice" comes from. I think I caught and killed another potential regression last week while it is still in the draft status during my review ;-)
Re: rev-parse --show-toplevel broken during exec'ed rebase?
On Mon, Jul 16, 2018 at 11:14:51AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > None of which is too surprising. The root of the bug is in the > > conversion to rebase--helper, I think, when presumably we started > > setting GIT_DIR at all (but I didn't dig further). Then 09d7b6c6fa fixed > > _one_ fallout of that, which was relative paths, but didn't help the > > subdirectory case. > > > > Just reading over this thread, I suspect the simplest fix is to pass > > GIT_DIR and GIT_WORK_TREE together, which is almost always the right > > thing to do. > > Perhaps. Not exporting GIT_DIR (unless the end-user already did to > the environment before starting "git rebase"---it would be a bad > change to unexport it unconditionally) may probably be a way to make > rebase--helper conversion more faithful to the original scripted > Porcelain, but I suspect in practice always giving GIT_DIR and > GIT_WORK_TREE would work well for many existing hooks. Yeah, that may be an option. I don't remember if this was discussed in this thread or elsewhere, but setting GIT_DIR is a regression for hooks, etc, which do: git -C /some/other/repo log or similar. I'm not sure if that falls under "actual regression" or "just how it happened to work in the past". I'm not even sure it worked that way _consistently_ in the past. The best practice if you're switching directories is to do: unset $(git rev-parse --local-env-vars) though I highly doubt that most people bother. -Peff
Re: rev-parse --show-toplevel broken during exec'ed rebase?
Jeff King writes: > None of which is too surprising. The root of the bug is in the > conversion to rebase--helper, I think, when presumably we started > setting GIT_DIR at all (but I didn't dig further). Then 09d7b6c6fa fixed > _one_ fallout of that, which was relative paths, but didn't help the > subdirectory case. > > Just reading over this thread, I suspect the simplest fix is to pass > GIT_DIR and GIT_WORK_TREE together, which is almost always the right > thing to do. Perhaps. Not exporting GIT_DIR (unless the end-user already did to the environment before starting "git rebase"---it would be a bad change to unexport it unconditionally) may probably be a way to make rebase--helper conversion more faithful to the original scripted Porcelain, but I suspect in practice always giving GIT_DIR and GIT_WORK_TREE would work well for many existing hooks.
Re: rev-parse --show-toplevel broken during exec'ed rebase?
On Fri, Jul 13, 2018 at 04:19:49PM -0400, Jeff King wrote: > Just reading over this thread, I suspect the simplest fix is to pass > GIT_DIR and GIT_WORK_TREE together, which is almost always the right > thing to do. I agree. I'll write up a patch. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: rev-parse --show-toplevel broken during exec'ed rebase?
On Fri, Jul 13, 2018 at 06:47:32PM +, brian m. carlson wrote: > > >> git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git > > >> rev-parse --show-toplevel)" > > > ... path to git repository/xdiff !!! > > > > > > This seems like incorrect behaviour to me since it's a weird > > > inconsistency (even with other rebase contexts) & the documentation > > > says "Show the absolute path of the top-level directory." with no > > > caveats. > > > > Does it reproduce with older rebase (say from v2.10 days), too? > > It appears to work correctly on the CentOS SCL Git 2.9.3. We print the > top level of the repository there. Bisecting is tricky, because there are actually three distinct behaviors. The command above prints the correct top-level until 18633e1a22 (rebase -i: use the rebase--helper builtin, 2017-02-09). After that, rev-parse prints "Not a git repository". That goes on until 09d7b6c6fa (sequencer: pass absolute GIT_DIR to exec commands, 2017-10-31). None of which is too surprising. The root of the bug is in the conversion to rebase--helper, I think, when presumably we started setting GIT_DIR at all (but I didn't dig further). Then 09d7b6c6fa fixed _one_ fallout of that, which was relative paths, but didn't help the subdirectory case. Just reading over this thread, I suspect the simplest fix is to pass GIT_DIR and GIT_WORK_TREE together, which is almost always the right thing to do. -Peff
Re: rev-parse --show-toplevel broken during exec'ed rebase?
On Thu, Jul 12, 2018 at 08:23:02AM -0700, Junio C Hamano wrote: > Vitali Lovich writes: > > > Repro (starting with cwd within git project): > >> (cd xdiff; git rev-parse --show-toplevel) > > ... path to git repository > >> git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd > > # Stop at some commit for editing > >> (cd xdiff; git rev-parse --show-toplevel) > > ... path to git repository > >> git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git > >> rev-parse --show-toplevel)" > > ... path to git repository/xdiff !!! > > > > This seems like incorrect behaviour to me since it's a weird > > inconsistency (even with other rebase contexts) & the documentation > > says "Show the absolute path of the top-level directory." with no > > caveats. > > Does it reproduce with older rebase (say from v2.10 days), too? It appears to work correctly on the CentOS SCL Git 2.9.3. We print the top level of the repository there. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: rev-parse --show-toplevel broken during exec'ed rebase?
On Thu, Jul 12, 2018 at 8:23 AM Junio C Hamano wrote: > > Vitali Lovich writes: > > > Repro (starting with cwd within git project): > >> (cd xdiff; git rev-parse --show-toplevel) > > ... path to git repository > >> git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd > > # Stop at some commit for editing > >> (cd xdiff; git rev-parse --show-toplevel) > > ... path to git repository > >> git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git > >> rev-parse --show-toplevel)" > > ... path to git repository/xdiff !!! > > > > This seems like incorrect behaviour to me since it's a weird > > inconsistency (even with other rebase contexts) & the documentation > > says "Show the absolute path of the top-level directory." with no > > caveats. > > Does it reproduce with older rebase (say from v2.10 days), too? > > I suspect that the above is because "git rebase" is exporting > GIT_DIR without exporting GIT_WORK_TREE. When the former is given > without the latter, that tells Git that you are at the top-level of > the working tree (if that is not the case, you also export the > latter to tell Git where the top-level is). > > I suspect that "git rebase" before the ongoing piecemeal rewrite to > C started (or scripted Porcelain in general) refrained from > exporting GIT_DIR to the environment, and if my suspicion is correct, > older "git rebase" would behave differently to your test case. Unfortunately I don't have an older git version handy to test this out.
Re: rev-parse --show-toplevel broken during exec'ed rebase?
Vitali Lovich writes: > Repro (starting with cwd within git project): >> (cd xdiff; git rev-parse --show-toplevel) > ... path to git repository >> git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd > # Stop at some commit for editing >> (cd xdiff; git rev-parse --show-toplevel) > ... path to git repository >> git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git >> rev-parse --show-toplevel)" > ... path to git repository/xdiff !!! > > This seems like incorrect behaviour to me since it's a weird > inconsistency (even with other rebase contexts) & the documentation > says "Show the absolute path of the top-level directory." with no > caveats. Does it reproduce with older rebase (say from v2.10 days), too? I suspect that the above is because "git rebase" is exporting GIT_DIR without exporting GIT_WORK_TREE. When the former is given without the latter, that tells Git that you are at the top-level of the working tree (if that is not the case, you also export the latter to tell Git where the top-level is). I suspect that "git rebase" before the ongoing piecemeal rewrite to C started (or scripted Porcelain in general) refrained from exporting GIT_DIR to the environment, and if my suspicion is correct, older "git rebase" would behave differently to your test case.
Re: rev-parse --show-toplevel broken during exec'ed rebase?
Hi, On Thu, 12 Jul 2018, Johannes Schindelin wrote: > On Wed, 11 Jul 2018, Vitali Lovich wrote: > > > On Wed, Jul 11, 2018 at 7:50 PM Vitali Lovich wrote: > > > > > > Typically git rev-parse --show-toplevel prints the folder containing > > > the .git folder regardless what subdirectory one is in. One exception > > > I have found is that if one is within the context of git rebase --exec > > > then show-toplevel always just prints the current directory it's > > > running from. > > > > > > Repro (starting with cwd within git project): > > > > (cd xdiff; git rev-parse --show-toplevel) > > > ... path to git repository > > > > git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd > > > # Stop at some commit for editing > > > > (cd xdiff; git rev-parse --show-toplevel) > > > ... path to git repository > > > > git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git > > > > rev-parse --show-toplevel)" > > > ... path to git repository/xdiff !!! > > > > > > This seems like incorrect behaviour to me since it's a weird > > > inconsistency (even with other rebase contexts) & the documentation > > > says "Show the absolute path of the top-level directory." with no > > > caveats. > > > > Sorry. Forgot to include the git versions I tested with (2.13.1, > > 2.17.0, 2.18.0) > > This is actually not so much a bug in `rebase` as in `rev-parse > --show-top-level`: > > $ GIT_DIR=$PWD/.git git -C xdiff rev-parse --show-toplevel > C:/git-sdk-64/usr/src/git/xdiff And the reason for this behavior is that setting `GIT_DIR` explicitly makes Git *always* consider the current working directory to be the top-level directory: https://github.com/git/git/blob/v2.18.0/setup.c#L1061-L1063 I wonder whether changing the line (https://github.com/git/git/blob/v2.18.0/sequencer.c#L2635) argv_array_pushf(_env, "GIT_DIR=%s", absolute_path(get_git_dir())); to argv_array_push(_env, "GIT_DIR"); breaks anything (and whether it fixes the bug you demonstrated via `rev-parse --show-toplevel`). Ciao, Johannes
Re: rev-parse --show-toplevel broken during exec'ed rebase?
Hi Vitali, [please avoid top-posting on this mailing list] On Wed, 11 Jul 2018, Vitali Lovich wrote: > On Wed, Jul 11, 2018 at 7:50 PM Vitali Lovich wrote: > > > > Typically git rev-parse --show-toplevel prints the folder containing > > the .git folder regardless what subdirectory one is in. One exception > > I have found is that if one is within the context of git rebase --exec > > then show-toplevel always just prints the current directory it's > > running from. > > > > Repro (starting with cwd within git project): > > > (cd xdiff; git rev-parse --show-toplevel) > > ... path to git repository > > > git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd > > # Stop at some commit for editing > > > (cd xdiff; git rev-parse --show-toplevel) > > ... path to git repository > > > git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git > > > rev-parse --show-toplevel)" > > ... path to git repository/xdiff !!! > > > > This seems like incorrect behaviour to me since it's a weird > > inconsistency (even with other rebase contexts) & the documentation > > says "Show the absolute path of the top-level directory." with no > > caveats. > > Sorry. Forgot to include the git versions I tested with (2.13.1, > 2.17.0, 2.18.0) This is actually not so much a bug in `rebase` as in `rev-parse --show-top-level`: $ GIT_DIR=$PWD/.git git -C xdiff rev-parse --show-toplevel C:/git-sdk-64/usr/src/git/xdiff Ciao, Johannes
Re: rev-parse --show-toplevel broken during exec'ed rebase?
Sorry. Forgot to include the git versions I tested with (2.13.1, 2.17.0, 2.18.0) On Wed, Jul 11, 2018 at 7:50 PM Vitali Lovich wrote: > > Typically git rev-parse --show-toplevel prints the folder containing > the .git folder regardless what subdirectory one is in. One exception > I have found is that if one is within the context of git rebase --exec > then show-toplevel always just prints the current directory it's > running from. > > Repro (starting with cwd within git project): > > (cd xdiff; git rev-parse --show-toplevel) > ... path to git repository > > git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd > # Stop at some commit for editing > > (cd xdiff; git rev-parse --show-toplevel) > ... path to git repository > > git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git > > rev-parse --show-toplevel)" > ... path to git repository/xdiff !!! > > This seems like incorrect behaviour to me since it's a weird > inconsistency (even with other rebase contexts) & the documentation > says "Show the absolute path of the top-level directory." with no > caveats. > > Thanks, > Vital
