Skip to content

Commit

Permalink
send-pack: do not check for sha1 file when GVFS_MISSING_OK set
Browse files Browse the repository at this point in the history
  • Loading branch information
@dscho
Kevin Willford authored and dscho committed Dec 15, 2018
1 parent e21465f commit c9ceb5a
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion send-pack.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "sha1-array.h"
#include "gpg-interface.h"
#include "cache.h"
#include "gvfs.h"

int option_parse_push_signed(const struct option *opt,
const char *arg, int unset)
Expand Down Expand Up @@ -50,7 +51,7 @@ static int send_pack_config(const char *var, const char *value, void *unused)

static void feed_object(const struct object_id *oid, FILE *fh, int negative)
{
if (negative && !has_sha1_file(oid->hash))
if (negative && !gvfs_config_is_set(GVFS_MISSING_OK) && !has_sha1_file(oid->hash))
return;

if (negative)
Expand Down

10 comments on commit c9ceb5a

@Ignas
Copy link

@Ignas Ignas commented on c9ceb5a Feb 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to figure out what is causing microsoft/VFSForGit#830 and this particular change seems to be breaking one of the assumptions made by git.

The flow usually is:

  1. Get a list of remote refs
  2. Get a list of local refs we will be sending
  3. Filter out remote refs that we do not have in the repository
  4. Generate a list of the remaining refs and pass it to pack-objects (^ for refs that the remote has, no ^ for refs that we are trying to send over)

In this case if a remote has a branch that has never existed locally - instead of skipping it, it gets added in with ^ in front of it, which is not what would have happened. @kewillford could you elaborate on what the semantics of this particular change are as I find it difficult to understand what issue you were working around by introducing it.

@dscho
Copy link
Member

@dscho dscho commented on c9ceb5a Feb 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kewillford do you remember what this change was about?
@Ignas I don't think that VFSforGit currently supports multiple remotes...

@kewillford
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #68 for the PR as well as this VFSForGit issue for some additional information. Basically on Mac this was causing git to hang because the read-object hook would be spawned here and because of how handles are inherited on Mac, stdin and stdout would not be correct when it came back to wait on pack-objects stdout because it was really waiting on the read-object's stdout.

@Ignas
Copy link

@Ignas Ignas commented on c9ceb5a Mar 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, found the PR. So it seems that assumptions that this would not break git were not correct. I will look into fixing the hang so this could be reverted. The case only surfaces if you are pushing to a remote that you don't have access to via VFS protocol (upstream for example).

@kewillford
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are a couple other PRs that I put up that would fix the issue.
#79 - fixes it by closing all the file descriptors when a process is launched. Not sure if this is a safe change though.
#69 - fixes it by loading the list of objects before passing the objects to the pack-object process. Probably our best option at this point.

We went with the smallest code change to begin with but if that is causing issues we can revert that and switch to one of these others.

@Ignas
Copy link

@Ignas Ignas commented on c9ceb5a Mar 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#69 looks way better as far as I am concerned as #68 changes git semantics and while I understand that multiple remote support is not there, #69 would at least make them work when pushing. And the less changes to git semantics the better :)

@Ignas
Copy link

@Ignas Ignas commented on c9ceb5a Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything I can do to help move this forward? Should I open a pull request that replaces #68 with #69 to fix microsoft/VFSForGit#830 or will you do that instead as you are the author of both? Though I still think it's kind of weird that git code can't handle running 2 sub-processes in parallel, and feel that it might be something fixable. Have not managed to figure it out yet though.

@Ignas
Copy link

@Ignas Ignas commented on c9ceb5a Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so I looked really hard, and I think I figured it out #128 is I think the correct way to fix it. It might make sense to contribute changes to run-command.c to Git upstream, but I wanted to make this available so we could continue the discussion on how to fix my issue with some actual code present.

@kewillford
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened a new PR #127 that reverts the old code and replaces it. Have you tried to push with GVFS on Mac with #128? I can see if I will have some time to test it because you have to push and during the push GVFS will need to download some objects so that the read-object hook will start after the pack-objects process was started.

@Ignas
Copy link

@Ignas Ignas commented on c9ceb5a Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that was my test case during debugging of the issue on my macbook for the microsoft/VFSForGit#830 bug. And both the hang and the crash are gone.

As my remote is ahead, read-object is invoked to check if it exists (has_object) which triggers read-object concurrently with pack-objects call.

Basically to figure this out I reverted your workaround and did inspecting with "lsof -p" on all 3 objects, to notice that the pipe used for pack-objects stdin was closed on git side, but still open on read-objects side. Which is expected, read-objects inherits all fds, but has no idea about "fd20" and should not know about it.

The solution to this is to basically set the bit closeonexec on the fd before you start read-objects (or do any other fork + exec for that matter).

Please sign in to comment.