Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects
Duy Nguyen pclo...@gmail.com writes: On Sat, Mar 8, 2014 at 1:27 AM, Junio C Hamano gits...@pobox.com wrote: On the receive-pack side, the comment at the bottom of preprare_shallow_update() makes it clear that, if we wanted to use hooks, we cannot avoid having the proposed new shallow-file in a temporary file, which is unfortunate. Do we have a similar issue on hooks on the upload-pack side? No. I don't think we have hooks on upload-pack. The question was not only about do we happen to work OK with the current code? but about are we closing the door for the future? If we ever need to add hooks to upload-pack, with the updated approach, its operation will not be affected by the temporary shallow file tailored for this specific customer. Which I think is a good thing in general. But at the same time, it means that its operation cannot be customized for the specific customer, taking into account what they lack (which can be gleaned by looking at the temporary shallow information). I do think that it is not a big downside, but that is merely my knee-jerk reaction. When upload-pack learns about hooks, I think we'll need to go back with --shallow-file, perhaps we a secure temporary place to write in. I don't see another way out. Not really sure why upload-pack needs customization though. The only case I can think of is to prevent most users from fetching certain objects, but that does not sound realistic.. I was more thinking along the lines of logging. But you are right that we can easily revisit --shallow-file interface later. Of course.. So what should we do with this? Go with this no temp file approach and deal with hooks when they come, or prepare now and introduce a secure temp. area? I was rather hoping that we did not have to worry about temporary files. This patch solves the issue for the service people would expect to be read-only the most, and it is a good step in the right direction. So let's follow through with the approach and not worry about secure temporary for now. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects
Duy Nguyen pclo...@gmail.com writes: On Fri, Mar 7, 2014 at 1:37 AM, Junio C Hamano gits...@pobox.com wrote: I like what I see in this patch, but I wonder if we can essentially revert that temporary shallow file patch and replace it with the same (or a similar) mechanism uniformly? Using --shallow-file is uniform. The only downside is temporary file. Without it you'll need to think of a way (probably different way for each command) to feed shallow info to. Yes, that is what I meant to say by the we need a way to tell hooks in some cases below; we are in agreement. On the receive-pack side, the comment at the bottom of preprare_shallow_update() makes it clear that, if we wanted to use hooks, we cannot avoid having the proposed new shallow-file in a temporary file, which is unfortunate. Do we have a similar issue on hooks on the upload-pack side? No. I don't think we have hooks on upload-pack. The question was not only about do we happen to work OK with the current code? but about are we closing the door for the future? If we ever need to add hooks to upload-pack, with the updated approach, its operation will not be affected by the temporary shallow file tailored for this specific customer. Which I think is a good thing in general. But at the same time, it means that its operation cannot be customized for the specific customer, taking into account what they lack (which can be gleaned by looking at the temporary shallow information). I do think that it is not a big downside, but that is merely my knee-jerk reaction. Nothing for Documentation/ anywhere? Heh git-pack-objects.txt never described stdin format. At least I searched for --not in it and found none. So I gladly accepted the situation and skipped doc update :D To pile new technical debt on top of existing ones is to make things worse, which we would rather not to see. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects
Duy Nguyen pclo...@gmail.com writes: On Thu, Mar 6, 2014 at 3:49 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index 3ae9092..a980574 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -173,4 +173,17 @@ EOF ) ' +test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' ' + cp -R .git read-only.git + find read-only.git -print | xargs chmod -w + test_when_finished find read-only.git -type d -print | xargs chmod +w + git clone --no-local --depth=2 read-only.git from-read-only + git --git-dir=from-read-only/.git log --format=%s actual + cat expect EOF +add-1-back +4 +EOF + test_cmp expect actual +' + test_done It's a separate issue, but maybe we should add a similar test case for non-shallow clone from a read-only repo too. Are there any other operations that should work well on read-only repos? Any read-only operation ;-)? On the object-transfer front, that would mean fetching from, archive-r from, and perhaps creating bundle from. Locally, log, grep, blame, gitk (but only the browsing part), etc. If a read-write borrower borrows from a read-only location via the objects/info/alternates mechanism, anying operation to the borrower should also work without modifying the borrowee. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects
On Sat, Mar 8, 2014 at 1:27 AM, Junio C Hamano gits...@pobox.com wrote: On the receive-pack side, the comment at the bottom of preprare_shallow_update() makes it clear that, if we wanted to use hooks, we cannot avoid having the proposed new shallow-file in a temporary file, which is unfortunate. Do we have a similar issue on hooks on the upload-pack side? No. I don't think we have hooks on upload-pack. The question was not only about do we happen to work OK with the current code? but about are we closing the door for the future? If we ever need to add hooks to upload-pack, with the updated approach, its operation will not be affected by the temporary shallow file tailored for this specific customer. Which I think is a good thing in general. But at the same time, it means that its operation cannot be customized for the specific customer, taking into account what they lack (which can be gleaned by looking at the temporary shallow information). I do think that it is not a big downside, but that is merely my knee-jerk reaction. When upload-pack learns about hooks, I think we'll need to go back with --shallow-file, perhaps we a secure temporary place to write in. I don't see another way out. Not really sure why upload-pack needs customization though. The only case I can think of is to prevent most users from fetching certain objects, but that does not sound realistic.. Nothing for Documentation/ anywhere? Heh git-pack-objects.txt never described stdin format. At least I searched for --not in it and found none. So I gladly accepted the situation and skipped doc update :D To pile new technical debt on top of existing ones is to make things worse, which we would rather not to see. Of course.. So what should we do with this? Go with this no temp file approach and deal with hooks when they come, or prepare now and introduce a secure temp. area? -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] upload-pack: send shallow info over stdin to pack-objects
From: Duy Nguyen pclo...@gmail.com
Before cdab485 (upload-pack: delegate rev walking in shallow fetch to
pack-objects - 2013-08-16) upload-pack does not write to the source
repository. cdab485 starts to write $GIT_DIR/shallow_XX if it's a
shallow fetch, so the source repo must be writable.
git:// servers do not need write access to repos and usually don't
have it, which means cdab485 breaks shallow clone over git://
Instead of using a temporary file as the media for shallow points, we
can send them over stdin to pack-objects as well. Prepend shallow
SHA-1 with --shallow so pack-objects knows what is
what.
read_object_list_from_stdin() does not accept --shallow lines because
upload-pack never uses that code. When upload-pack does, then it can
learn about --shallow lines.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
OK new approach, stop creating shallow_XX in upload-pack.
builtin/pack-objects.c | 7 +++
shallow.c| 2 ++
t/t5537-fetch-shallow.sh | 13 +
upload-pack.c| 21 -
4 files changed, 34 insertions(+), 9 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..79e848e 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2467,6 +2467,13 @@ static void get_object_list(int ac, const char **av)
write_bitmap_index = 0;
continue;
}
+ if (starts_with(line, --shallow )) {
+ unsigned char sha1[20];
+ if (get_sha1_hex(line + 10, sha1))
+ die(not an SHA-1 '%s', line + 10);
+ register_shallow(sha1);
+ continue;
+ }
die(not a rev '%s', line);
}
if (handle_revision_arg(line, revs, flags,
REVARG_CANNOT_BE_FILENAME))
diff --git a/shallow.c b/shallow.c
index bbc98b5..41ff4a0 100644
--- a/shallow.c
+++ b/shallow.c
@@ -33,6 +33,8 @@ int register_shallow(const unsigned char *sha1)
graft-nr_parent = -1;
if (commit commit-object.parsed)
commit-parents = NULL;
+ if (is_shallow == -1)
+ is_shallow = 1;
return register_commit_graft(graft, 0);
}
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 3ae9092..a980574 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -173,4 +173,17 @@ EOF
)
'
+test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
+ cp -R .git read-only.git
+ find read-only.git -print | xargs chmod -w
+ test_when_finished find read-only.git -type d -print | xargs chmod +w
+ git clone --no-local --depth=2 read-only.git from-read-only
+ git --git-dir=from-read-only/.git log --format=%s actual
+ cat expect EOF
+add-1-back
+4
+EOF
+ test_cmp expect actual
+'
+
test_done
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..a5c50e4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -70,6 +70,14 @@ static ssize_t send_client_data(int fd, const char *data,
ssize_t sz)
return sz;
}
+static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
+{
+ FILE *fp = cb_data;
+ if (graft-nr_parent == -1)
+ fprintf(fp, --shallow %s\n, sha1_to_hex(graft-sha1));
+ return 0;
+}
+
static void create_pack_file(void)
{
struct child_process pack_objects;
@@ -81,12 +89,10 @@ static void create_pack_file(void)
const char *argv[12];
int i, arg = 0;
FILE *pipe_fd;
- char *shallow_file = NULL;
if (shallow_nr) {
- shallow_file = setup_temporary_shallow(NULL);
argv[arg++] = --shallow-file;
- argv[arg++] = shallow_file;
+ argv[arg++] = ;
}
argv[arg++] = pack-objects;
argv[arg++] = --revs;
@@ -114,6 +120,9 @@ static void create_pack_file(void)
pipe_fd = xfdopen(pack_objects.in, w);
+ if (shallow_nr)
+ for_each_commit_graft(write_one_shallow, pipe_fd);
+
for (i = 0; i want_obj.nr; i++)
fprintf(pipe_fd, %s\n,
sha1_to_hex(want_obj.objects[i].item-sha1));
@@ -242,12 +251,6 @@ static void create_pack_file(void)
error(git upload-pack: git-pack-objects died with error.);
goto fail;
}
- if (shallow_file) {
- if (*shallow_file)
- unlink(shallow_file);
- free(shallow_file);
- }
-
/* flush the data */
if (0 = buffered) {
data[0] = buffered;
--
1.9.0.66.g14f785a
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More
Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
From: Duy Nguyen pclo...@gmail.com
Before cdab485 (upload-pack: delegate rev walking in shallow fetch to
pack-objects - 2013-08-16) upload-pack does not write to the source
repository. cdab485 starts to write $GIT_DIR/shallow_XX if it's a
shallow fetch, so the source repo must be writable.
git:// servers do not need write access to repos and usually don't
have it, which means cdab485 breaks shallow clone over git://
Instead of using a temporary file as the media for shallow points, we
can send them over stdin to pack-objects as well. Prepend shallow
SHA-1 with --shallow so pack-objects knows what is
what.
read_object_list_from_stdin() does not accept --shallow lines because
upload-pack never uses that code. When upload-pack does, then it can
learn about --shallow lines.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
OK new approach, stop creating shallow_XX in upload-pack.
Thanks.
I like what I see in this patch, but I wonder if we can essentially
revert that temporary shallow file patch and replace it with the
same (or a similar) mechanism uniformly?
On the receive-pack side, the comment at the bottom of
preprare_shallow_update() makes it clear that, if we wanted to use
hooks, we cannot avoid having the proposed new shallow-file in a
temporary file, which is unfortunate. Do we have a similar issue on
hooks on the upload-pack side?
builtin/pack-objects.c | 7 +++
shallow.c| 2 ++
t/t5537-fetch-shallow.sh | 13 +
upload-pack.c| 21 -
4 files changed, 34 insertions(+), 9 deletions(-)
Nothing for Documentation/ anywhere?
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..79e848e 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2467,6 +2467,13 @@ static void get_object_list(int ac, const char **av)
write_bitmap_index = 0;
continue;
}
+ if (starts_with(line, --shallow )) {
+ unsigned char sha1[20];
+ if (get_sha1_hex(line + 10, sha1))
+ die(not an SHA-1 '%s', line + 10);
+ register_shallow(sha1);
+ continue;
+ }
die(not a rev '%s', line);
}
if (handle_revision_arg(line, revs, flags,
REVARG_CANNOT_BE_FILENAME))
diff --git a/shallow.c b/shallow.c
index bbc98b5..41ff4a0 100644
--- a/shallow.c
+++ b/shallow.c
@@ -33,6 +33,8 @@ int register_shallow(const unsigned char *sha1)
graft-nr_parent = -1;
if (commit commit-object.parsed)
commit-parents = NULL;
+ if (is_shallow == -1)
+ is_shallow = 1;
return register_commit_graft(graft, 0);
}
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 3ae9092..a980574 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -173,4 +173,17 @@ EOF
)
'
+test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
+ cp -R .git read-only.git
+ find read-only.git -print | xargs chmod -w
+ test_when_finished find read-only.git -type d -print | xargs chmod +w
+ git clone --no-local --depth=2 read-only.git from-read-only
+ git --git-dir=from-read-only/.git log --format=%s actual
+ cat expect EOF
+add-1-back
+4
+EOF
+ test_cmp expect actual
+'
+
test_done
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..a5c50e4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -70,6 +70,14 @@ static ssize_t send_client_data(int fd, const char *data,
ssize_t sz)
return sz;
}
+static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
+{
+ FILE *fp = cb_data;
+ if (graft-nr_parent == -1)
+ fprintf(fp, --shallow %s\n, sha1_to_hex(graft-sha1));
+ return 0;
+}
+
static void create_pack_file(void)
{
struct child_process pack_objects;
@@ -81,12 +89,10 @@ static void create_pack_file(void)
const char *argv[12];
int i, arg = 0;
FILE *pipe_fd;
- char *shallow_file = NULL;
if (shallow_nr) {
- shallow_file = setup_temporary_shallow(NULL);
argv[arg++] = --shallow-file;
- argv[arg++] = shallow_file;
+ argv[arg++] = ;
}
argv[arg++] = pack-objects;
argv[arg++] = --revs;
@@ -114,6 +120,9 @@ static void create_pack_file(void)
pipe_fd = xfdopen(pack_objects.in, w);
+ if (shallow_nr)
+ for_each_commit_graft(write_one_shallow, pipe_fd);
+
for (i = 0; i want_obj.nr; i++)
fprintf(pipe_fd, %s\n,
Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects
On Fri, Mar 7, 2014 at 1:37 AM, Junio C Hamano gits...@pobox.com wrote: I like what I see in this patch, but I wonder if we can essentially revert that temporary shallow file patch and replace it with the same (or a similar) mechanism uniformly? Using --shallow-file is uniform.The only downside is temporary file. Without it you'll need to think of a way (probably different way for each command) to feed shallow info to. On the receive-pack side, the comment at the bottom of preprare_shallow_update() makes it clear that, if we wanted to use hooks, we cannot avoid having the proposed new shallow-file in a temporary file, which is unfortunate. Do we have a similar issue on hooks on the upload-pack side? No. I don't think we have hooks on upload-pack. builtin/pack-objects.c | 7 +++ shallow.c| 2 ++ t/t5537-fetch-shallow.sh | 13 + upload-pack.c| 21 - 4 files changed, 34 insertions(+), 9 deletions(-) Nothing for Documentation/ anywhere? Heh git-pack-objects.txt never described stdin format. At least I searched for --not in it and found none. So I gladly accepted the situation and skipped doc update :D -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects
On Thu, Mar 6, 2014 at 3:49 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index 3ae9092..a980574 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -173,4 +173,17 @@ EOF ) ' +test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' ' + cp -R .git read-only.git + find read-only.git -print | xargs chmod -w + test_when_finished find read-only.git -type d -print | xargs chmod +w + git clone --no-local --depth=2 read-only.git from-read-only + git --git-dir=from-read-only/.git log --format=%s actual + cat expect EOF +add-1-back +4 +EOF + test_cmp expect actual +' + test_done It's a separate issue, but maybe we should add a similar test case for non-shallow clone from a read-only repo too. Are there any other operations that should work well on read-only repos? -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
