Am 21.08.2013 10:49, schrieb Matthieu Moy:
Stefan Beller stefanbel...@googlemail.com writes:
+ for_each_string_list_item(item, names) {
+ for (ext = 0; ext 2; ext++) {
+ char *fname, *fname_old;
+ fname = mkpathdup(%s/%s%s,
Am 21.08.2013 15:07, schrieb Matthieu Moy:
Stefan Beller stefanbel...@googlemail.com writes:
But as these follow up changes heavily rely on the very first patch
I will first try to get that right, meaning accepted into pu.
Then I can send patches with these proposals such as making more
This patch is meant to be squashed into bb4335a21441a0
(repack: rewrite the shell script in C), I'll do so when rerolling
the series. For reviewing I'll just send this patch.
* Remove comments, which likely get out of date (authorship is kept in
git anyway)
* rename get_pack_filenames to
Stefan Beller stefanbel...@googlemail.com writes:
@@ -41,18 +35,16 @@ static void remove_temporary_files(void)
DIR *dir;
struct dirent *e;
+ dir = opendir(packdir);
+ if (!dir)
return;
+ strbuf_addstr(buf, packdir);
+
+ /* dirlen holds the
Hi,
Stefan Beller wrote:
[PATCH] repack: rewrite the shell script in C.
Thanks for your work so far. This review will have mostly cosmetic
notes. Hopefully others can try it out to see if the actual behavior
is good.
As a first nit: in git, as usual in emails, the style in subject lines
Stefan Beller stefanbel...@googlemail.com writes:
All tests are constantly positive now.
Cool!
+/*
+ * Fills the filename list with all the files found in the pack directory
Detail: filename list could be fname_list to match the actual
argument below.
+ * ending with .pack, without that
On 08/21/2013 10:25 AM, Jonathan Nieder wrote:
+static int delta_base_offset = 0;
The = 0 is automatic for statics without an initializer. The
prevailing style in git is to leave it out.
Behavior change: in the script, wasn't the default true?
Yes, I was printing out the arguments of
On 08/21/2013 10:49 AM, Matthieu Moy wrote:
+if (start_command(cmd))
+ return 1;
A warning message would be welcome in addition to returning 1.
Johannes Sixt proposes to retain the return value of
the sub process, which I'd agree on.
However why do we need a warning message
On 08/21/2013 10:49 AM, Matthieu Moy wrote:
I tend to dislike these set a variable and break twice to exit nested
loops. Using an auxiliary function, you could just do
int f()
{
for_each {
for () {
...
if ()
Stefan Beller stefanbel...@googlemail.com writes:
On 08/21/2013 10:49 AM, Matthieu Moy wrote:
+ if (start_command(cmd))
+ return 1;
A warning message would be welcome in addition to returning 1.
Johannes Sixt proposes to retain the return value of
the sub process, which I'd
Stefan Beller stefanbel...@googlemail.com writes:
But as these follow up changes heavily rely on the very first patch
I will first try to get that right, meaning accepted into pu.
Then I can send patches with these proposals such as making more
functions.
I think it's better to get the style
On 08/21/2013 10:25 AM, Jonathan Nieder wrote:
Hi,
Stefan Beller wrote:
[PATCH] repack: rewrite the shell script in C.
Thanks for your work so far. This review will have mostly cosmetic
notes. Hopefully others can try it out to see if the actual behavior
is good.
Thanks for all
This is the beginning of the rewrite of the repacking.
All tests are constantly positive now.
Signed-off-by: Stefan Beller stefanbel...@googlemail.com
---
Makefile| 2 +-
builtin.h | 1 +
builtin/repack.c
On 08/15/2013 01:25 AM, Martin Fick wrote:
On Wednesday, August 14, 2013 04:51:14 pm Matthieu Moy
wrote:
Antoine Pelisse apeli...@gmail.com writes:
On Wed, Aug 14, 2013 at 6:27 PM, Stefan Beller
stefanbel...@googlemail.com wrote:
builtin/repack.c | 410
On 08/15/2013 12:59 AM, Matthieu Moy wrote:
Junio C Hamano gits...@pobox.com writes:
Stefan Beller stefanbel...@googlemail.com writes:
I asked for a todo wish list a few weeks ago, but got no real answer,
but rather: Pick your choice and try to come up with good patches.
Hmph, I hope that
On 08/14/2013 07:04 PM, Junio C Hamano wrote:
Stefan Beller stefanbel...@googlemail.com writes:
diff --git a/builtin/repack.c b/builtin/repack.c
new file mode 100644
index 000..d39c34e
--- /dev/null
+++ b/builtin/repack.c
@@ -0,0 +1,410 @@
+/*
+ * The shell version was written by
On Thursday, August 15, 2013 01:46:02 am Stefan Beller
wrote:
On 08/15/2013 01:25 AM, Martin Fick wrote:
On Wednesday, August 14, 2013 04:51:14 pm Matthieu Moy
wrote:
Antoine Pelisse apeli...@gmail.com writes:
On Wed, Aug 14, 2013 at 6:27 PM, Stefan Beller
Martin Fick mf...@codeaurora.org writes:
On Wednesday, August 14, 2013 04:53:36 pm Junio C Hamano
wrote:
Martin Fick mf...@codeaurora.org writes:
One suggestion would be to change the repack code to
create pack filenames based on the sha1 of the
contents of the pack file instead of on
Stefan Beller stefanbel...@googlemail.com writes:
This is the beginning of the rewrite of the repacking.
(please, mark your patch as RFC/PATCH in the subject in this case)
A few quick comments on style below.
Makefile | 2 +-
builtin.h | 1 +
On 08/14/2013 09:26 AM, Matthieu Moy wrote:
I suggested that you first enrich the test suite if needed. Did you
check that the testsuite had good enough coverage for git-repack?
There are already quite some tests using git-repack for testing other
purposes. Also git repack seems to be called
* Suggestions by Matthieu Moy have been included.
* I think it is completed apart from small todos and bugfixes.
* breaks the test suite, first breakage is t5301 (gc, sliding window)
Signed-off-by: Stefan Beller stefanbel...@googlemail.com
---
Makefile | 2 +-
builtin.h
On Wed, Aug 14, 2013 at 6:27 PM, Stefan Beller
stefanbel...@googlemail.com wrote:
builtin/repack.c | 410
+
contrib/examples/git-repack.sh | 194 +++
git-repack.sh | 194 ---
I'm still not
Stefan Beller stefanbel...@googlemail.com writes:
diff --git a/builtin/repack.c b/builtin/repack.c
new file mode 100644
index 000..d39c34e
--- /dev/null
+++ b/builtin/repack.c
@@ -0,0 +1,410 @@
+/*
+ * The shell version was written by Linus Torvalds (2005) and many others.
+ * This
On 08/14/2013 06:49 PM, Antoine Pelisse wrote:
On Wed, Aug 14, 2013 at 6:27 PM, Stefan Beller
stefanbel...@googlemail.com wrote:
builtin/repack.c | 410
+
contrib/examples/git-repack.sh | 194 +++
git-repack.sh
On Wed, Aug 14, 2013 at 07:04:37PM +0200, Stefan Beller wrote:
But apart from my blabbering, I think ivegy made a good point:
The C parts just don't rely on external things, but only libc and
kernel, so it may be nicer than a shell script. Also as it is used
serversided, the performance
On Wednesday, August 14, 2013 10:49:58 am Antoine Pelisse
wrote:
On Wed, Aug 14, 2013 at 6:27 PM, Stefan Beller
stefanbel...@googlemail.com wrote:
builtin/repack.c | 410
+
contrib/examples/git-repack.sh | 194
Antoine Pelisse apeli...@gmail.com writes:
On Wed, Aug 14, 2013 at 6:27 PM, Stefan Beller
stefanbel...@googlemail.com wrote:
builtin/repack.c | 410
+
contrib/examples/git-repack.sh | 194 +++
git-repack.sh
On 08/14/2013 07:25 PM, Martin Fick wrote:
I have been holding off a bit on expressing this opinion too
because I don't want to squash someone's energy to improve
things, and because I am not yet a git dev, but since it was
brought up anyway...
It's ok, if you knew a better topic to work
On Wednesday, August 14, 2013 04:16:35 pm Stefan Beller
wrote:
On 08/14/2013 07:25 PM, Martin Fick wrote:
I have been holding off a bit on expressing this
opinion too because I don't want to squash someone's
energy to improve things, and because I am not yet a
git dev, but since it was
Antoine Pelisse apeli...@gmail.com writes:
On Wed, Aug 14, 2013 at 6:27 PM, Stefan Beller
stefanbel...@googlemail.com wrote:
builtin/repack.c | 410
+
contrib/examples/git-repack.sh | 194 +++
git-repack.sh
Stefan Beller stefanbel...@googlemail.com writes:
I asked for a todo wish list a few weeks ago, but got no real answer,
but rather: Pick your choice and try to come up with good patches.
Hmph, I hope that wasn't me.
There are some good ones here;
Martin Fick mf...@codeaurora.org writes:
One suggestion would be to change the repack code to create
pack filenames based on the sha1 of the contents of the pack
file instead of on the sha1 of the objects in the packfile.
...
I am not 100% sure if the change in naming convention I
Junio C Hamano gits...@pobox.com writes:
Stefan Beller stefanbel...@googlemail.com writes:
I asked for a todo wish list a few weeks ago, but got no real answer,
but rather: Pick your choice and try to come up with good patches.
Hmph, I hope that wasn't me.
There are some good ones here;
On Wednesday, August 14, 2013 04:51:14 pm Matthieu Moy
wrote:
Antoine Pelisse apeli...@gmail.com writes:
On Wed, Aug 14, 2013 at 6:27 PM, Stefan Beller
stefanbel...@googlemail.com wrote:
builtin/repack.c | 410
+
On Wednesday, August 14, 2013 04:53:36 pm Junio C Hamano
wrote:
Martin Fick mf...@codeaurora.org writes:
One suggestion would be to change the repack code to
create pack filenames based on the sha1 of the
contents of the pack file instead of on the sha1 of
the objects in the packfile.
On Wednesday, August 14, 2013 05:25:42 pm Martin Fick wrote:
On Wednesday, August 14, 2013 04:51:14 pm Matthieu Moy
wrote:
Antoine Pelisse apeli...@gmail.com writes:
On Wed, Aug 14, 2013 at 6:27 PM, Stefan Beller
stefanbel...@googlemail.com wrote:
builtin/repack.c
On Thu, Aug 15, 2013 at 12:25 AM, Martin Fick mf...@codeaurora.org wrote:
The script really is mostly a policy script, and with the
discussions happening in other threads about how to improve
git gc, I think it is helpful to potentially be able to
quickly modify the policies in this script, it
On Thu, Aug 15, 2013 at 5:51 AM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
There's a real problem with git-repack being shell (I already mentionned
it in the previous thread about the rewrite): it creates dependencies on
a few external binaries, and a restricted server may not have them.
This is the beginning of the rewrite of the repacking.
Signed-off-by: Stefan Beller stefanbel...@googlemail.com
---
Makefile | 2 +-
builtin.h | 1 +
builtin/repack.c | 313 +
39 matches
Mail list logo
