|
|
Created:
5 years, 6 months ago by szager1 Modified:
5 years, 6 months ago Reviewers:
agable, hinoka CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionSpecify GIT_DIR when running git-config in a mirror.
BUG=497894
R=hinoka@chromium.org,agable@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=295587
Patch Set 1 #Patch Set 2 : set env correctly #Patch Set 3 : Missed one call to RunGit #
Total comments: 2
Patch Set 4 : fix whitespace #
Created: 5 years, 6 months ago
Messages
Total messages: 16 (2 generated)
Doesn't GIT_DIR need to point directly at a .git directory, not at a checkout containing one?
I think this removes all other environment variables ($HOME, $PATH, etc) from the execution environment.
On 2015/06/08 19:54:04, hinoka wrote: > I think this removes all other environment variables ($HOME, $PATH, etc) from > the execution environment. If you look at the definition of RunGit(), it already adds an env parameter. This just adds one more entry to that.
On 2015/06/08 19:52:33, agable wrote: > Doesn't GIT_DIR need to point directly at a .git directory, not at a checkout > containing one? Either a .git directory, or the root of a bare repo.
On 2015/06/08 20:27:22, szager1 wrote: > On 2015/06/08 19:52:33, agable wrote: > > Doesn't GIT_DIR need to point directly at a .git directory, not at a checkout > > containing one? > > Either a .git directory, or the root of a bare repo. To clarify: config() is only ever called to set up a mirror, which is a bare git repo. So in every case, GIT_DIR=cwd is correct.
RunGit() does: env = kwargs.get('env') or kwargs.setdefault('env', os.environ.copy()) if you don't specify an env in kwargs it's set to os.environ.copy() If you do specify an env then it takes it directly from kwargs and os.environ.copy() never gets called. On Mon, Jun 8, 2015 at 1:28 PM, <szager@chromium.org> wrote: > On 2015/06/08 20:27:22, szager1 wrote: > >> On 2015/06/08 19:52:33, agable wrote: >> > Doesn't GIT_DIR need to point directly at a .git directory, not at a >> > checkout > >> > containing one? >> > > Either a .git directory, or the root of a bare repo. >> > > To clarify: config() is only ever called to set up a mirror, which is a > bare git > repo. So in every case, GIT_DIR=cwd is correct. > > https://codereview.chromium.org/1167193002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/06/08 20:34:13, chromium-reviews wrote: > RunGit() does: > env = kwargs.get('env') or kwargs.setdefault('env', os.environ.copy()) > > if you don't specify an env in kwargs it's set to os.environ.copy() > > If you do specify an env then it takes it directly from kwargs and > os.environ.copy() never gets called. > > On Mon, Jun 8, 2015 at 1:28 PM, <mailto:szager@chromium.org> wrote: > > > On 2015/06/08 20:27:22, szager1 wrote: > > > >> On 2015/06/08 19:52:33, agable wrote: > >> > Doesn't GIT_DIR need to point directly at a .git directory, not at a > >> > > checkout > > > >> > containing one? > >> > > > > Either a .git directory, or the root of a bare repo. > >> > > > > To clarify: config() is only ever called to set up a mirror, which is a > > bare git > > repo. So in every case, GIT_DIR=cwd is correct. > > > > https://codereview.chromium.org/1167193002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Ah, indeed; fixed now, thanks.
ping
lgtm + nit https://codereview.chromium.org/1167193002/diff/40001/git_cache.py File git_cache.py (right): https://codereview.chromium.org/1167193002/diff/40001/git_cache.py#newcode241 git_cache.py:241: cwd=cwd, env=env) that looks like a tab
https://codereview.chromium.org/1167193002/diff/40001/git_cache.py File git_cache.py (right): https://codereview.chromium.org/1167193002/diff/40001/git_cache.py#newcode241 git_cache.py:241: cwd=cwd, env=env) On 2015/06/09 18:10:34, hinoka wrote: > that looks like a tab Done.
The CQ bit was checked by szager@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@chromium.org Link to the patchset: https://codereview.chromium.org/1167193002/#ps60001 (title: "fix whitespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167193002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=295587
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1179593002/ by hinoka@chromium.org. The reason for reverting is: Potentially broke the NaCl toolchain builders: crbug.com/498942 Speculative revert.. |