Comments (7)
The problem is that git credential approve
and reject
don't have any output. When we're executing the commands, we're giving it buffers for the process stdout and stderr, so cmd.Wait()
will wait for those to be filled. Since they'll never be filled, it waits forever.
We'll need to modify the command execution to not provide buffers for approve
.
from git-lfs.
This is actually a little bit more involved. This is only triggered when the credential cache daemon is not already running. If it is not running, git credential
operations will start it. When it is started, one of the child processes is probably not closing the stderr descriptor. This means that the git media client never receives EOF on the stderr pipe that it's waiting on, essentially blocking forever. It appears that the child process properly closes its stdout descriptor, because you can actually leave the buffer on stdout and everything is fine. This is likely a bug somewhere down the chain of git commands, but it's easy enough to handle here by just not waiting for any output on the commands we know don't have any output.
from git-lfs.
With a little more investigation, this seems like it might be a bug in git itself. Things work like this:
The git-media
client [A] launches git
[B] which, upon detecting the credential cache daemon is not running, launches git-credential-cache--daemon
[C]. The process [C] closes its stdout descriptor, but it does not close its stderr descriptor. Therefore, Process [A] will read EOF on the stdout pipe, but it will never read EOF on the stderr pipe, which will cause [A] to wait forever (or until [C] dies or is killed, even though [B] is long gone).
In git's credential-cache.c where it launches the credential cache daemon, you can see that it requests to not keep stdout open. I think it also should ask to not keep stderr open, as such:
diff --git a/credential-cache.c b/credential-cache.c
index 9a03792..9abce0b 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -47,7 +47,9 @@ static void spawn_daemon(const char *socket)
argv[1] = socket;
daemon.argv = argv;
daemon.no_stdin = 1;
+ daemon.no_stderr = 1;
daemon.out = -1;
if (start_command(&daemon))
die_errno("unable to start cache daemon");
If I do this, the problem goes away. @github/git - does this seem like a legit git bug to you?
We'll have to work around this anyway by just never waiting on the subprocess's stderr, but if it is a legit git bug we might want to send patches.
Edit the code should not set daemon.err = -1
, because it never gets set to a valid file descriptor, and thus the program will try to close(-1)
. Only daemon.no_stderr
should be set.
from git-lfs.
Hrm. I'd question a bit whether it is actually worth capturing stderr from a git credential
call in the first place. Normally it's stderr would just go the user via the terminal (or captured in a file or whatever). Yes, it's a little weird that we have a persistent process that may keep writing to stderr. But then, it may still have things to say to stderr (i.e., errors to report while the daemon runs).
But assuming we did want to close stderr, this is definitely the wrong place to do it. Most of the interesting errors would happen while the daemon is starting up, and they would all go to /dev/null
with your patch. I'd much prefer to close it in the daemon after it has successfully started up but right before it goes into its event loop:
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 3b370ca..1a5572c 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -211,6 +211,7 @@ static void serve_cache(const char *socket_path)
printf("ok\n");
fclose(stdout);
+ fclose(stderr);
while (serve_cache_loop(fd))
; /* nothing */
That at least allows through messages related to opening the socket. We'd still miss errors and warnings that happen when serving individual clients, though. Two things that would make it a little better:
- Teach the daemon a command-line argument to optionally close stderr (after successfully starting). Use it when auto-starting the daemon from credential-cache. This makes it easy to debug issues by running the daemon manually without that option.
- Detect pipes via fstat() and only close stderr if it goes to a pipe. This would make things work as now in most cases. It does feel a little dirty, though (e.g., if you piped your stderr to "tee" it would magically drop some messages).
Of the two, I think I prefer the first. But I'd still be curious to hear why you want to pipe stderr in the first place.
from git-lfs.
Thanks for taking a look at this @peff, I think the reason we were capturing stderr is because the git media client may not necessarily be started from a terminal, and we'd want to bubble those errors up to whatever is controlling it. However, the stderr that we're really expecting to read from I think would come from the git process itself, not the cache daemon. This problem only shows up in the case where this git credential
process is the one that kicks off the cache daemon. If the cache daemon is already running then everything is OK.
The reason I think the code in git has a problem is that I think a daemon should not assume that it has a terminal to write to. Given how this daemon is started from the git process, it could be safe-ish to assume that there is a terminal to write startup errors to, but once it's launched that terminal can go away at any time and the output won't go anywhere (or if some later terminal opens up on the same device it could get the output, and that'd be weird). I suppose the credential cache daemon is not your typical daemon in that it's usually started by the git process, is generally short lived, and probably has a terminal to write to.
With regard to start up errors, you're correct, my patch does prevent them from showing while your patch lets them through. I verified this by chmod'ing ~/.git-credential-cache
to 777, which will trigger some output on stderr down in the cache daemon code. If anything, allowing those errors to come through is more useful than not.
I think for now, we will work around the problem by not piping stderr, as you suggest. I think we should be OK with that given the types of errors that might happen at that point. It may be that in the typical use case for the git / cache daemon interaction, allowing the daemon to assume it can write to the terminal is OK, I just wanted to check with some git internals experts to make sure that was the case 😄
from git-lfs.
Yeah, the "bubble up to GUI" thing makes sense. Git-media aside, this could be a problem even for something like GitHub for Mac making a call to git fetch
. I think we probably didn't notice it there because they typically use the osxkeychain
helper rather than the caching helper.
I just sent a patch to the list.
from git-lfs.
@peff Awesome, thanks!
Closing this now, #118 just doesn't hook up stderr.
from git-lfs.
Related Issues (20)
- git clone --mirror: remote missing object HOT 5
- Bundle support for air-gap sync HOT 1
- Client certificates silently ignored if paths specified with "~/" HOT 2
- Choose to be successful in the world wide web site for the main curl https://www.geniustechmedia.com redirect=1-3834482702&p=appgame_ratings&rd=web@geniustechmedia. HOT 1
- Client version passed to the server? HOT 2
- Some tests are functionally disabled due to `if $TRAVIS` HOT 4
- Error When Attempting to Merge Branch Into Original HOT 11
- Wrong PUT Host IP HOT 2
- Any news on ssh/sftp support ? HOT 23
- `git lfs` does not honor `git -c url.<url>.insteadOf` when inside a submodule HOT 2
- Is what Tutorial says about SSH correct? HOT 4
- `checkout` does not respect files which have been staged for removal with `git rm` HOT 2
- [Bug] UI Scrolling HOT 1
- Windows: runtime VirtualAlloc failed with Out Of Memory despite 20GB+ free commit & Physical RAM HOT 3
- Error: "stale NFS file handle" with lfs.storage HOT 2
- [macOS 14+] Unable to clone a repository that uses `git-lfs` HOT 7
- Install on Windows fails due to "unexpected" git executable HOT 7
- In repos which using 'repo sync' downloaded, all branches can not be recognized HOT 1
- INSTALLED LFS BUT STILL NOT ABLE TO PUSH MY ZIP FILE TO MY GITHUB REPOSITORY HOT 2
- Retrieve files contents from remote - how? HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from git-lfs.