send-pack: do not check for sha1 file when GVFS_MISSING_OK set #68
Conversation
| @@ -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)) | |||
There was a problem hiding this comment.
@kewillford We should be a bit careful here. Specifically, git push starts with an info/refs call to find the current branch tips. If those tips have moved ahead of our prefetch packfiles, then those commits will require the read-object hook. This logic will prevent those commits from being sent to send-pack and hence will not be considered as --not commits in the revision walk. This can lead to over-pushing objects.
In a particularly egregious case, think about a fresh clone with master as the only branch favorite. I clone, make a simple change, and push. The master branch has moved ahead, but now we tell pack-objects to pack our commit and sends zero --not commits. We will pack the entire repo and try to push it.
Let me know if I'm misunderstanding the effect of this change.
There was a problem hiding this comment.
It's more likely that I am missing something, as if master moves ahead in a vanilla git repo, we don't have the object, so we continue without marking that as a --not commit. Instead, we probably use our refs/remote/origin/... refs. I'm going to do some digging and come back to this soon.
There was a problem hiding this comment.
I talked with @kewillford offline and we determined that I was wrong, for multiple reasons. I was misreading some things and it took a while to shake them all out. Here is my new understanding of what's going on:
git pushcallsgit remote-httpswhich callsgit send-pack.- Before,
git send-packwould trigger theread-objecthook and somehow this was causing timing issues. - The change here is to remove the
read-objectcalls from thesend-packlogic by understanding we are in a virtualized environment andhas_sha1_file(hash)will always return true. - This change does not alter the set of
--notcommits passed togit pack-objects. - The
read-objecthook will still be run bygit pack-objects, but somehow this timing change prevents the failure.
| @@ -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)) | |||
There was a problem hiding this comment.
I talked with @kewillford offline and we determined that I was wrong, for multiple reasons. I was misreading some things and it took a while to shake them all out. Here is my new understanding of what's going on:
git pushcallsgit remote-httpswhich callsgit send-pack.- Before,
git send-packwould trigger theread-objecthook and somehow this was causing timing issues. - The change here is to remove the
read-objectcalls from thesend-packlogic by understanding we are in a virtualized environment andhas_sha1_file(hash)will always return true. - This change does not alter the set of
--notcommits passed togit pack-objects. - The
read-objecthook will still be run bygit pack-objects, but somehow this timing change prevents the failure.
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
…_MISSING_OK set send-pack: do not check for sha1 file when GVFS_MISSING_OK set
This is to fix the issue on Mac where the push will hang because the read-object hook process gets spawned and the pack-object process has also been spawned and something, maybe stdin/stdout, get in a deadlock. Still need to find the smoking gun.
For this fix, it will check the GVFS flag before checking that an object doesn't exist so that it will not download the object. @derrickstolee I'm not sure how this will affect push performance since it will be sending more negative oids to the pack-object process. Still need to dig into the pack-object code to see determine if this is the right change.