From: Junio C Hamano <hidden> Date: 2016-06-15 22:58:25
Martin Fick [off-list ref] writes:
On Wednesday, August 14, 2013 04:53:36 pm Junio C Hamano
wrote:
quoted
Martin Fick [off-list ref] writes:
quoted
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
propose wouldn't cause any problems? But if others
agree it is a good idea, perhaps it is something a
beginner could do?
I would not be surprised if that change breaks some other
people's reimplementation. I know we do not validate
the pack name with the hash of the contents in the
current code, but at the same time I do remember that
was one of the planned things to be done while I and
Linus were working on the original pack design, which
was the last task we did together before he retired from
the maintainership of this project.
Perhaps a config option? One that becomes standard for git
2.0?
Anything new is too late for Git 2.0, as we do not want to hold the
switching of push.default to "simple" too long. End of this year
might be a bit too soon, but I want 2.0 to happen by the next
spring.
You can discuss, design the new naming and necessary transition plan
for existing repositories, reach a concensus and declare the name
switch in the future, and then schedule that for the next major
version bump after 2.0 happens.
From: Stefan Beller <hidden> Date: 2016-06-15 22:58:26
This is the beginning of the rewrite of the repacking.
* Removed unneeded system header files
* corrected remove_pack to really remove any pack files with the given
sha1
* fail if pack-objects fails
* Only test t7701 (2nd) fails now with this patch.
Signed-off-by: Stefan Beller <redacted>
---
Makefile | 2 +-
builtin.h | 1 +
builtin/repack.c | 411 ++++++++++++++++++++++++
git-repack.sh => contrib/examples/git-repack.sh | 0
git.c | 1 +
5 files changed, 414 insertions(+), 1 deletion(-)
create mode 100644 builtin/repack.c
rename git-repack.sh => contrib/examples/git-repack.sh (100%)
@@ -0,0 +1,411 @@+/*+*TheshellversionwaswrittenbyLinusTorvalds(2005)andmanyothers.+*ThisisatranslationintoCbyStefanBeller(2013)+*/++#include"builtin.h"+#include"cache.h"+#include"dir.h"+#include"parse-options.h"+#include"run-command.h"+#include"sigchain.h"+#include"strbuf.h"+#include"string-list.h"++staticconstchar*constgit_repack_usage[]={+N_("git repack [options]"),+NULL+};++/* enabled by default since 22c79eab (2008-06-25) */+staticintdelta_base_offset=1;++staticintrepack_config(constchar*var,constchar*value,void*cb)+{+if(!strcmp(var,"repack.usedeltabaseoffset")){+delta_base_offset=git_config_bool(var,value);+return0;+}+returngit_default_config(var,value,cb);+}++staticvoidremove_temporary_files(){+DIR*dir;+structdirent*e;+char*prefix,*path,*fname;++prefix=xmalloc(strlen(".tmp-10000-pack")+1);+sprintf(prefix,".tmp-%d-pack",getpid());++path=xmalloc(strlen(get_object_directory())+strlen("/pack")+1);+sprintf(path,"%s/pack",get_object_directory());++fname=xmalloc(strlen(path)+strlen("/")++strlen(prefix)+strlen("/")++40+strlen(".pack")+1);++dir=opendir(path);+while((e=readdir(dir))!=NULL){+if(!prefixcmp(e->d_name,prefix)){+sprintf(fname,"%s/%s",path,e->d_name);+unlink(fname);+}+}+free(fname);+free(prefix);+free(path);+closedir(dir);+}++staticvoidremove_pack_on_signal(intsigno)+{+remove_temporary_files();+sigchain_pop(signo);+raise(signo);+}++voidget_pack_sha1_list(char*packdir,structstring_list*sha1_list)+{+DIR*dir;+structdirent*e;+char*path,*suffix;++path=xmalloc(strlen(get_object_directory())+strlen("/pack")+1);+sprintf(path,"%s/pack",get_object_directory());++suffix=".pack";++dir=opendir(path);+while((e=readdir(dir))!=NULL){+if(!suffixcmp(e->d_name,suffix)){+charbuf[255],*sha1;+strcpy(buf,e->d_name);+buf[strlen(e->d_name)-strlen(suffix)]='\0';+sha1=&buf[strlen(e->d_name)-strlen(suffix)-40];+string_list_append(sha1_list,sha1);+}+}+free(path);+closedir(dir);+}++/*+*remove_packwillremoveanyfilesfollowingthepattern*${SHA1}.{EXT}+*whereEXTisoneof{pack,idx,keep}.TheSHA1consistsof40charsand+*isspecifiedbythesha1parameter.+*pathisspecifyingthedirectoryinwhichallfoundfileswillbedeleted.+*/+voidremove_pack(char*path,char*sha1)+{+DIR*dir;+structdirent*e;++dir=opendir(path);+while((e=readdir(dir))!=NULL){++char*sha_begin,*sha_end;+sha_end=e->d_name+strlen(e->d_name);+while(sha_end>e->d_name&&*sha_end!='.')+sha_end--;++/* do not touch files not ending in .pack, .idx or .keep */+if(strcmp(sha_end,".pack")&&+strcmp(sha_end,".idx")&&+strcmp(sha_end,".keep"))+continue;++sha_begin=sha_end-40;++if(sha_begin>=e->d_name&&!strncmp(sha_begin,sha1,40)){+char*fname;+fname=xmalloc(strlen(path)+1+strlen(e->d_name));+sprintf(fname,"%s/%s",path,e->d_name);+unlink(fname);+/*TODO: free(fname); fails here sometimes, needs investigation*/+}+}+closedir(dir);+}++intcmd_repack(intargc,constchar**argv,constchar*prefix){++intpack_everything=0;+intpack_everything_but_loose=0;+intdelete_redundant=0;+unsignedlongunpack_unreachable=0;+intwindow=0,window_memory=0;+intdepth=0;+intmax_pack_size=0;+intno_reuse_delta=0,no_reuse_object=0;+intno_update_server_info=0;+intquiet=0;+intlocal=0;+char*packdir,*packtmp;+constchar*cmd_args[20];+intcmd_i=0;+structchild_processcmd;+structstring_list_item*item;+structstring_listexisting_packs=STRING_LIST_INIT_DUP;+structstatstatbuffer;+charwindow_str[64],window_mem_str[64],depth_str[64],max_pack_size_str[64];++structoptionbuiltin_repack_options[]={+OPT_BOOL('a',"all",&pack_everything,+N_("pack everything in a single pack")),+OPT_BOOL('A',"all-but-loose",&pack_everything_but_loose,+N_("same as -a, and turn unreachable objects loose")),+OPT_BOOL('d',"delete-redundant",&delete_redundant,+N_("remove redundant packs, and run git-prune-packed")),+OPT_BOOL('f',"no-reuse-delta",&no_reuse_delta,+N_("pass --no-reuse-delta to git-pack-objects")),+OPT_BOOL('F',"no-reuse-object",&no_reuse_object,+N_("pass --no-reuse-object to git-pack-objects")),+OPT_BOOL('n',NULL,&no_update_server_info,+N_("do not run git-update-server-info")),+OPT__QUIET(&quiet,N_("be quiet")),+OPT_BOOL('l',"local",&local,+N_("pass --local to git-pack-objects")),+OPT_DATE(0,"unpack-unreachable",&unpack_unreachable,+N_("with -A, do not loosen objects older than this Packing constraints")),+OPT_INTEGER(0,"window",&window,+N_("size of the window used for delta compression")),+OPT_INTEGER(0,"window-memory",&window_memory,+N_("same as the above, but limit memory size instead of entries count")),+OPT_INTEGER(0,"depth",&depth,+N_("limits the maximum delta depth")),+OPT_INTEGER(0,"max-pack-size",&max_pack_size,+N_("maximum size of each packfile")),+OPT_END()+};++git_config(repack_config,NULL);++argc=parse_options(argc,argv,prefix,builtin_repack_options,+git_repack_usage,0);++sigchain_push_common(remove_pack_on_signal);++packdir=mkpath("%s/pack",get_object_directory());+packtmp=xmalloc(strlen(packdir)+strlen("/.tmp-10000-pack")+1);+sprintf(packtmp,"%s/.tmp-%d-pack",packdir,getpid());++remove_temporary_files();++cmd_args[cmd_i++]="pack-objects";+cmd_args[cmd_i++]="--keep-true-parents";+cmd_args[cmd_i++]="--honor-pack-keep";+cmd_args[cmd_i++]="--non-empty";+cmd_args[cmd_i++]="--all";+cmd_args[cmd_i++]="--reflog";++if(window){+sprintf(window_str,"--window=%u",window);+cmd_args[cmd_i++]=window_str;+}+if(window_memory){+sprintf(window_mem_str,"--window-memory=%u",window_memory);+cmd_args[cmd_i++]=window_str;+}+if(depth){+sprintf(depth_str,"--depth=%u",depth);+cmd_args[cmd_i++]=depth_str;+}+if(max_pack_size){+sprintf(max_pack_size_str,"--max_pack_size=%u",max_pack_size);+cmd_args[cmd_i++]=max_pack_size_str;+}++if(pack_everything+pack_everything_but_loose==0){+cmd_args[cmd_i++]="--unpacked";+cmd_args[cmd_i++]="--incremental";+}else{+if(pack_everything_but_loose)+cmd_args[cmd_i++]="--unpack-unreachable";++structstring_listsha1_list=STRING_LIST_INIT_DUP;+get_pack_sha1_list(packdir,&sha1_list);+for_each_string_list_item(item,&sha1_list){+char*fname;+fname=xmalloc(strlen(packdir)+strlen("/")+40+strlen(".keep"));+sprintf(fname,"%s/%s.keep",packdir,item->string);+if(stat(fname,&statbuffer)&&S_ISREG(statbuffer.st_mode)){+/* when the keep file is there, we're ignoring that pack */+}else{+string_list_append(&existing_packs,item->string);+}+}++if(existing_packs.nr&&unpack_unreachable&&delete_redundant){+/*+*TODO:convertunpack_unreachable(beingtimesinceepoch)+*toanaproxidateagain+*/+cmd_args[cmd_i++]="--unpack-unreachable=$DATE";+}+}++if(local)+cmd_args[cmd_i++]="--local";++if(delta_base_offset)+cmd_args[cmd_i++]="--delta-base-offset";++cmd_args[cmd_i++]=packtmp;+cmd_args[cmd_i]=NULL;++memset(&cmd,0,sizeof(cmd));+cmd.argv=cmd_args;+cmd.git_cmd=1;+cmd.out=-1;+cmd.no_stdin=1;++if(run_command(&cmd))+return1;++structstring_listnames=STRING_LIST_INIT_DUP;+structstring_listrollback=STRING_LIST_INIT_DUP;++charline[1024];+intcounter=0;+FILE*out=xfdopen(cmd.out,"r");+while(fgets(line,sizeof(line),out)){+/* a line consists of 40 hex chars + '\n' */+assert(strlen(line)==41);+line[40]='\0';+string_list_append(&names,line);+counter++;+}+if(!counter)+printf("Nothing new to pack.\n");+fclose(out);++char*fname,*fname_old;+fname=xmalloc(strlen(packdir)+strlen("/old-pack-")+40+strlen(".pack")+1);+strcpy(fname,packdir);+strcpy(fname+strlen(packdir),"/");++fname_old=xmalloc(strlen(packdir)+strlen("/old-pack-")+40+strlen(".pack")+1);+strcpy(fname_old,packdir);+strcpy(fname_old+strlen(packdir),"/");+char*exts[2]={".idx",".pack"};++intfailed=0;++for_each_string_list_item(item,&names){+intext;+for(ext=0;ext<1;ext++){+strcpy(fname+strlen(packdir)+1,item->string);+strcpy(fname+strlen(packdir)+41,exts[ext]);+if(!file_exists(fname))+continue;++strcpy(fname_old,packdir);+strcpy(fname_old+strlen(packdir),"/old-");+strcpy(fname_old+strlen(packdir)+5,item->string);+strcpy(fname_old+strlen(packdir)+45,exts[ext]);+if(file_exists(fname_old))+unlink(fname_old);++if(rename(fname,fname_old)){+failed=1;+break;+}+string_list_append(&rollback,fname);+}+if(failed)+/* set to last element to break while loop */+item=names.items+names.nr;+}+if(failed){+structstring_listrollback_failure;++for_each_string_list_item(item,&rollback){+sprintf(fname,"%s/%s",packdir,item->string);+sprintf(fname_old,"%s/old-%s",packdir,item->string);+if(rename(fname_old,fname))+string_list_append(&rollback_failure,fname);+}++if(rollback.nr){+inti;+fprintf(stderr,+"WARNING: Some packs in use have been renamed by\n"+"WARNING: prefixing old- to their name, in order to\n"+"WARNING: replace them with the new version of the\n"+"WARNING: file. But the operation failed, and\n"+"WARNING: attempt to rename them back to their\n"+"WARNING: original names also failed.\n"+"WARNING: Please rename them in $PACKDIR manually:\n");+for(i=0;i<rollback.nr;i++)+fprintf(stderr,"WARNING: old-%s -> %s\n",+rollback.items[i].string,+rollback.items[i].string);+}+exit(1);+}++/* Now the ones with the same name are out of the way... */+structstring_listfullbases=STRING_LIST_INIT_DUP;+for_each_string_list_item(item,&names){+string_list_append(&fullbases,item->string);++sprintf(fname,"%s/pack-%s.pack",packdir,item->string);+sprintf(fname_old,"%s-%s.pack",packtmp,item->string);+stat(fname_old,&statbuffer);+statbuffer.st_mode&=~S_IWOTH;+chmod(fname_old,statbuffer.st_mode);+if(rename(fname_old,fname))+die("Could not rename packfile: %s -> %s",fname_old,fname);++sprintf(fname,"%s/pack-%s.idx",packdir,item->string);+sprintf(fname_old,"%s-%s.idx",packtmp,item->string);+stat(fname_old,&statbuffer);+statbuffer.st_mode&=~S_IWOTH;+chmod(fname_old,statbuffer.st_mode);+if(rename(fname_old,fname))+die("Could not rename packfile: %s -> %s",fname_old,fname);+}++/* Remove the "old-" files */+for_each_string_list_item(item,&names){+sprintf(fname,"%s/old-pack-%s.idx",packdir,item->string);+if(remove_path(fname))+die("Could not remove file: %s",fname);++sprintf(fname,"%s/old-pack-%s.pack",packdir,item->string);+if(remove_path(fname))+die("Could not remove file: %s",fname);+}++/* End of pack replacement. */+if(delete_redundant){+sort_string_list(&fullbases);+fname=xmalloc(strlen(packtmp)+strlen("/")+40+strlen(".pack")+1);+for_each_string_list_item(item,&existing_packs){+if(!string_list_has_string(&fullbases,item->string))+remove_pack(packdir,item->string);+}+free(fname);+cmd_i=0;+cmd_args[cmd_i++]="prune-packed";+cmd_args[cmd_i++]=NULL;+/* TODO: pass argument: ${GIT_QUIET:+-q} */+memset(&cmd,0,sizeof(cmd));+cmd.argv=cmd_args;+cmd.git_cmd=1;+run_command(&cmd);+}++if(!no_update_server_info){+cmd_i=0;+cmd_args[cmd_i++]="update-server-info";+cmd_args[cmd_i++]=NULL;++memset(&cmd,0,sizeof(cmd));+cmd.argv=cmd_args;+cmd.git_cmd=1;+run_command(&cmd);+}+return0;+}+
diff --git a/git-repack.sh b/contrib/examples/git-repack.shsimilarity index 100%rename from git-repack.shrename to contrib/examples/git-repack.shdiff --git a/git.c b/git.cindex 2025f77..03510be 100644--- a/git.c+++ b/git.c
From: René Scharfe <hidden> Date: 2016-06-15 22:58:26
quoted hunk
This is the beginning of the rewrite of the repacking.
* Removed unneeded system header files
* corrected remove_pack to really remove any pack files with the given
sha1
* fail if pack-objects fails
* Only test t7701 (2nd) fails now with this patch.
Signed-off-by: Stefan Beller <redacted>
---
Makefile | 2 +-
builtin.h | 1 +
builtin/repack.c | 411 ++++++++++++++++++++++++
git-repack.sh => contrib/examples/git-repack.sh | 0
git.c | 1 +
5 files changed, 414 insertions(+), 1 deletion(-)
create mode 100644 builtin/repack.c
rename git-repack.sh => contrib/examples/git-repack.sh (100%)
@@ -0,0 +1,411 @@+/*+*TheshellversionwaswrittenbyLinusTorvalds(2005)andmanyothers.+*ThisisatranslationintoCbyStefanBeller(2013)+*/++#include"builtin.h"+#include"cache.h"+#include"dir.h"+#include"parse-options.h"+#include"run-command.h"+#include"sigchain.h"+#include"strbuf.h"+#include"string-list.h"++staticconstchar*constgit_repack_usage[]={+N_("git repack [options]"),+NULL+};++/* enabled by default since 22c79eab (2008-06-25) */+staticintdelta_base_offset=1;++staticintrepack_config(constchar*var,constchar*value,void*cb)+{+if(!strcmp(var,"repack.usedeltabaseoffset")){+delta_base_offset=git_config_bool(var,value);+return0;+}+returngit_default_config(var,value,cb);+}++staticvoidremove_temporary_files(){+DIR*dir;+structdirent*e;+char*prefix,*path,*fname;++prefix=xmalloc(strlen(".tmp-10000-pack")+1);+sprintf(prefix,".tmp-%d-pack",getpid());
This will overflow for PIDs with more than five digits. Better use a
strbuf and build the string with strbuf_addf. Or better, use mkpathdup,
which does that under the hood.
If someone has a directory entry that begins with 'prefix' but is longer
than expected this will overflow. That's unlikely, but lets avoid it
outright. You could use a strbuf instead and reset it to the length of
the prefix at the start of the loop (with strbuf_setlen), before adding
the entry's name.
mkpathdup again.
Or would it make sense to cd into the pack directory and avoid these
string manipulations outright? Probably not if we need to call other
git functions later on.
You could avoid the need for a temporary buffer by using xmemdupz and
string_list_append_nodup instead.
+ }
+ }
+ free(path);
+ closedir(dir);
+}
+
+/*
+ * remove_pack will remove any files following the pattern *${SHA1}.{EXT}
+ * where EXT is one of {pack, idx, keep}. The SHA1 consists of 40 chars and
+ * is specified by the sha1 parameter.
+ * path is specifying the directory in which all found files will be deleted.
+ */
+void remove_pack(char *path, char* sha1)
+{
+ DIR *dir;
+ struct dirent *e;
+
+ dir = opendir(path);
+ while ((e = readdir(dir)) != NULL) {
+
+ unlink(fname);
+ /*TODO: free(fname); fails here sometimes, needs investigation*/
Strange. Perhaps valgrind can tell you what's wrong.
+ }
+ }
+ closedir(dir);
+}
Hmm, stepping back a bit, why not just build the paths and call unlink
for them right away, without readdir? The shell version only ever
deletes existing .pack files (those in $existing alias existing_packs)
as well as their .idx and .keep files, if present. It doesn't use a
glob pattern, unlike remove_pack here.
+
+int cmd_repack(int argc, const char **argv, const char *prefix) {
+
+ int pack_everything = 0;
+ int pack_everything_but_loose = 0;
+ int delete_redundant = 0;
+ unsigned long unpack_unreachable = 0;
+ int window = 0, window_memory = 0;
+ int depth = 0;
+ int max_pack_size = 0;
+ int no_reuse_delta = 0, no_reuse_object = 0;
+ int no_update_server_info = 0;
+ int quiet = 0;
+ int local = 0;
+ char *packdir, *packtmp;
+ const char *cmd_args[20];
+ int cmd_i = 0;
+ struct child_process cmd;
+ struct string_list_item *item;
+ struct string_list existing_packs = STRING_LIST_INIT_DUP;
+ struct stat statbuffer;
+ char window_str[64], window_mem_str[64], depth_str[64], max_pack_size_str[64];
+
+ struct option builtin_repack_options[] = {
+ OPT_BOOL('a', "all", &pack_everything,
+ N_("pack everything in a single pack")),
+ OPT_BOOL('A', "all-but-loose", &pack_everything_but_loose,
+ N_("same as -a, and turn unreachable objects loose")),
+ OPT_BOOL('d', "delete-redundant", &delete_redundant,
+ N_("remove redundant packs, and run git-prune-packed")),
+ OPT_BOOL('f', "no-reuse-delta", &no_reuse_delta,
+ N_("pass --no-reuse-delta to git-pack-objects")),
+ OPT_BOOL('F', "no-reuse-object", &no_reuse_object,
+ N_("pass --no-reuse-object to git-pack-objects")),
+ OPT_BOOL('n', NULL, &no_update_server_info,
+ N_("do not run git-update-server-info")),
+ OPT__QUIET(&quiet, N_("be quiet")),
+ OPT_BOOL('l', "local", &local,
+ N_("pass --local to git-pack-objects")),
+ OPT_DATE(0, "unpack-unreachable", &unpack_unreachable,
+ N_("with -A, do not loosen objects older than this Packing constraints")),
+ OPT_INTEGER(0, "window", &window,
+ N_("size of the window used for delta compression")),
+ OPT_INTEGER(0, "window-memory", &window_memory,
+ N_("same as the above, but limit memory size instead of entries count")),
+ OPT_INTEGER(0, "depth", &depth,
+ N_("limits the maximum delta depth")),
+ OPT_INTEGER(0, "max-pack-size", &max_pack_size,
+ N_("maximum size of each packfile")),
+ OPT_END()
+ };
+
+ git_config(repack_config, NULL);
+
+ argc = parse_options(argc, argv, prefix, builtin_repack_options,
+ git_repack_usage, 0);
+
+ sigchain_push_common(remove_pack_on_signal);
+
+ packdir = mkpath("%s/pack", get_object_directory());
Ah, mkpath is already used. That function is a bit tricky because you
can only use it with four paths concurrently and some internal functions
might already need one (or more) of the slots for itself. It's better
to consume its output on the spot (like in printf("my path is %s\n",
mkpath(...));) or use mkpathdup.
mkpathdup..
Or maybe go through the entries in the pack directory once, like already
done in get_pack_sha1_list, and instead of just making a list of .pack
files, make a list of .keep files as well. Then work with those lists
instead of accessing the directory again with stat.
+ }
+ if (failed)
+ /* set to last element to break while loop */
+ item = names.items + names.nr;
+ }
+ if (failed) {
+ struct string_list rollback_failure;
+
+ for_each_string_list_item(item, &rollback) {
+ sprintf(fname, "%s/%s", packdir, item->string);
+ sprintf(fname_old, "%s/old-%s", packdir, item->string);
+ if (rename(fname_old, fname))
+ string_list_append(&rollback_failure, fname);
Dito.
+ }
+
+ if (rollback.nr) {
+ int i;
+ fprintf(stderr,
+ "WARNING: Some packs in use have been renamed by\n"
+ "WARNING: prefixing old- to their name, in order to\n"
+ "WARNING: replace them with the new version of the\n"
+ "WARNING: file. But the operation failed, and\n"
+ "WARNING: attempt to rename them back to their\n"
+ "WARNING: original names also failed.\n"
+ "WARNING: Please rename them in $PACKDIR manually:\n");
+ for (i = 0; i < rollback.nr; i++)
+ fprintf(stderr, "WARNING: old-%s -> %s\n",
+ rollback.items[i].string,
+ rollback.items[i].string);
+ }
+ exit(1);
+ }
+
+ /* Now the ones with the same name are out of the way... */
+ struct string_list fullbases = STRING_LIST_INIT_DUP;
+ for_each_string_list_item(item, &names) {
+ string_list_append(&fullbases, item->string);
Why make a copy of 'names'? Can't you use it directly instead of
'fullbases'? Ah, the Shell version adds a "pack-" at the beginning of
each string. We don't need to do that and thus can get rid of that
extra list, no?
From: Stefan Beller <hidden> Date: 2016-06-15 22:58:26
On 08/17/2013 03:34 PM, René Scharfe wrote:
Hmm, stepping back a bit, why not just build the paths and call unlink
for them right away, without readdir? The shell version only ever
deletes existing .pack files (those in $existing alias existing_packs)
as well as their .idx and .keep files, if present. It doesn't use a
glob pattern, unlike remove_pack here.
I'll meditate on that.
Thanks for all the other remarks. Now the code looks much more
git-ish, similar to other commands.
The lines of code went down from 411 to 385, I guess we can cut off
more inefficiencies there.
As you suggested, maybe we should juts have one helper function to
read in the pack directory and keeping all the information (complete filename),
so we do not need to find the exact filename later again by looping over
the directory again.
Stefan
From: Stefan Beller <hidden> Date: 2016-06-15 22:58:26
This is the beginning of the rewrite of the repacking.
* replace all plain string handling functions
by git helper functions, most often mkpathdup
* use argv-array structs to pass arguments to
other git invocations.
Only test t7701 (2nd) fails now with this patch.
Signed-off-by: Stefan Beller <redacted>
---
Makefile | 2 +-
builtin.h | 1 +
builtin/repack.c | 385 ++++++++++++++++++++++++
git-repack.sh => contrib/examples/git-repack.sh | 0
git.c | 1 +
5 files changed, 388 insertions(+), 1 deletion(-)
create mode 100644 builtin/repack.c
rename git-repack.sh => contrib/examples/git-repack.sh (100%)
@@ -0,0 +1,385 @@+/*+*TheshellversionwaswrittenbyLinusTorvalds(2005)andmanyothers.+*ThisisatranslationintoCbyStefanBeller(2013)+*/++#include"builtin.h"+#include"cache.h"+#include"dir.h"+#include"parse-options.h"+#include"run-command.h"+#include"sigchain.h"+#include"strbuf.h"+#include"string-list.h"++#include"argv-array.h"++staticconstchar*constgit_repack_usage[]={+N_("git repack [options]"),+NULL+};++/* enabled by default since 22c79eab (2008-06-25) */+staticintdelta_base_offset=1;++staticintrepack_config(constchar*var,constchar*value,void*cb)+{+if(!strcmp(var,"repack.usedeltabaseoffset")){+delta_base_offset=git_config_bool(var,value);+return0;+}+returngit_default_config(var,value,cb);+}++staticvoidremove_temporary_files(){+DIR*dir;+structdirent*e;+char*prefix,*path;++prefix=mkpathdup(".tmp-%d-pack",getpid());+path=mkpathdup("%s/pack",get_object_directory());++dir=opendir(path);+while((e=readdir(dir))!=NULL){+if(!prefixcmp(e->d_name,prefix)){+structstrbuffname=STRBUF_INIT;+strbuf_addf(&fname,"%s/%s",path,e->d_name);+unlink(strbuf_detach(&fname,NULL));+}+}+free(prefix);+free(path);+closedir(dir);+}++staticvoidremove_pack_on_signal(intsigno)+{+remove_temporary_files();+sigchain_pop(signo);+raise(signo);+}++voidget_pack_sha1_list(char*packdir,structstring_list*sha1_list)+{+DIR*dir;+structdirent*e;+char*path,*suffix;++path=mkpathdup("%s/pack",get_object_directory());+suffix=".pack";++dir=opendir(path);+while((e=readdir(dir))!=NULL){+if(!suffixcmp(e->d_name,suffix)){+char*buf,*sha1;+buf=xmemdupz(e->d_name,strlen(e->d_name));+buf[strlen(e->d_name)-strlen(suffix)]='\0';+if(strlen(e->d_name)-strlen(suffix)>40){+sha1=&buf[strlen(e->d_name)-strlen(suffix)-40];+string_list_append_nodup(sha1_list,sha1);+}else{+/*TODO: what should happen to pack files having no 40 char sha1 specifier?*/+}+}+}+free(path);+closedir(dir);+}++/*+*remove_packwillremoveanyfilesfollowingthepattern*${SHA1}.{EXT}+*whereEXTisoneof{pack,idx,keep}.TheSHA1consistsof40charsand+*isspecifiedbythesha1parameter.+*pathisspecifyingthedirectoryinwhichallfoundfileswillbedeleted.+*/+voidremove_pack(char*path,char*sha1)+{+DIR*dir;+structdirent*e;++dir=opendir(path);+while((e=readdir(dir))!=NULL){+char*sha_begin,*sha_end;+sha_end=e->d_name+strlen(e->d_name);+while(sha_end>e->d_name&&*sha_end!='.')+sha_end--;++/* do not touch files not ending in .pack, .idx or .keep */+if(strcmp(sha_end,".pack")&&+strcmp(sha_end,".idx")&&+strcmp(sha_end,".keep"))+continue;++sha_begin=sha_end-40;++if(sha_begin>=e->d_name&&!strncmp(sha_begin,sha1,40)){+char*fname;+fname=mkpathdup("%s/%s",path,e->d_name);+unlink(fname);+free(fname);+}+}+closedir(dir);+}++intcmd_repack(intargc,constchar**argv,constchar*prefix){++intpack_everything=0;+intpack_everything_but_loose=0;+intdelete_redundant=0;+char*unpack_unreachable=NULL;+intwindow=0,window_memory=0;+intdepth=0;+intmax_pack_size=0;+intno_reuse_delta=0,no_reuse_object=0;+intno_update_server_info=0;+intquiet=0;+intlocal=0;+char*packdir,*packtmp;+structchild_processcmd;+structstring_list_item*item;+structstring_listexisting_packs=STRING_LIST_INIT_DUP;+structstatstatbuffer;++structoptionbuiltin_repack_options[]={+OPT_BOOL('a',"all",&pack_everything,+N_("pack everything in a single pack")),+OPT_BOOL('A',"all-but-loose",&pack_everything_but_loose,+N_("same as -a, and turn unreachable objects loose")),+OPT_BOOL('d',"delete-redundant",&delete_redundant,+N_("remove redundant packs, and run git-prune-packed")),+OPT_BOOL('f',"no-reuse-delta",&no_reuse_delta,+N_("pass --no-reuse-delta to git-pack-objects")),+OPT_BOOL('F',"no-reuse-object",&no_reuse_object,+N_("pass --no-reuse-object to git-pack-objects")),+OPT_BOOL('n',NULL,&no_update_server_info,+N_("do not run git-update-server-info")),+OPT__QUIET(&quiet,N_("be quiet")),+OPT_BOOL('l',"local",&local,+N_("pass --local to git-pack-objects")),+OPT_STRING(0,"unpack-unreachable",&unpack_unreachable,N_("approxidate"),+N_("with -A, do not loosen objects older than this Packing constraints")),+OPT_INTEGER(0,"window",&window,+N_("size of the window used for delta compression")),+OPT_INTEGER(0,"window-memory",&window_memory,+N_("same as the above, but limit memory size instead of entries count")),+OPT_INTEGER(0,"depth",&depth,+N_("limits the maximum delta depth")),+OPT_INTEGER(0,"max-pack-size",&max_pack_size,+N_("maximum size of each packfile")),+OPT_END()+};++git_config(repack_config,NULL);++argc=parse_options(argc,argv,prefix,builtin_repack_options,+git_repack_usage,0);++sigchain_push_common(remove_pack_on_signal);++packdir=mkpathdup("%s/pack",get_object_directory());+packtmp=mkpathdup("%s/.tmp-%d-pack",packdir,getpid());++remove_temporary_files();++structargv_arraycmd_args=ARGV_ARRAY_INIT;+argv_array_push(&cmd_args,"pack-objects");+argv_array_push(&cmd_args,"--keep-true-parents");+argv_array_push(&cmd_args,"--honor-pack-keep");+argv_array_push(&cmd_args,"--non-empty");+argv_array_push(&cmd_args,"--all");+argv_array_push(&cmd_args,"--reflog");++if(window)+argv_array_pushf(&cmd_args,"--window=%u",window);++if(window_memory)+argv_array_pushf(&cmd_args,"--window-memory=%u",window_memory);++if(depth)+argv_array_pushf(&cmd_args,"--depth=%u",depth);++if(max_pack_size)+argv_array_pushf(&cmd_args,"--max_pack_size=%u",max_pack_size);++if(pack_everything+pack_everything_but_loose==0){+argv_array_push(&cmd_args,"--unpacked");+argv_array_push(&cmd_args,"--incremental");+}else{+if(pack_everything_but_loose)+argv_array_push(&cmd_args,"--unpack-unreachable");++structstring_listsha1_list=STRING_LIST_INIT_DUP;+get_pack_sha1_list(packdir,&sha1_list);+for_each_string_list_item(item,&sha1_list){+char*fname;+fname=mkpathdup("%s/%s.keep",packdir,item->string);+if(stat(fname,&statbuffer)&&S_ISREG(statbuffer.st_mode)){+/* when the keep file is there, we're ignoring that pack */+}else{+string_list_append(&existing_packs,item->string);+}+free(fname);+}++if(existing_packs.nr&&unpack_unreachable&&delete_redundant){+argv_array_pushf(&cmd_args,"--unpack-unreachable=%s",unpack_unreachable);+}+}++if(local)+argv_array_push(&cmd_args,"--local");++if(delta_base_offset)+argv_array_push(&cmd_args,"--delta-base-offset");++argv_array_push(&cmd_args,packtmp);++memset(&cmd,0,sizeof(cmd));+cmd.argv=argv_array_detach(&cmd_args,NULL);+cmd.git_cmd=1;+cmd.out=-1;+cmd.no_stdin=1;++if(run_command(&cmd))+return1;++structstring_listnames=STRING_LIST_INIT_DUP;+structstring_listrollback=STRING_LIST_INIT_DUP;++charline[1024];+intcounter=0;+FILE*out=xfdopen(cmd.out,"r");+while(fgets(line,sizeof(line),out)){+/* a line consists of 40 hex chars + '\n' */+assert(strlen(line)==41);+line[40]='\0';+string_list_append(&names,line);+counter++;+}+if(!counter)+printf("Nothing new to pack.\n");+fclose(out);++char*exts[2]={".idx",".pack"};+intfailed=0;+for_each_string_list_item(item,&names){+intext;+for(ext=0;ext<1;ext++){+char*fname,*fname_old;+fname=mkpathdup("%s/%s%s",packdir,item->string,exts[ext]);+if(!file_exists(fname)){+free(fname);+continue;+}++fname_old=mkpathdup("%s/old-%s%s",packdir,item->string,exts[ext]);+if(file_exists(fname_old))+unlink(fname_old);++if(rename(fname,fname_old)){+failed=1;+break;+}+string_list_append_nodup(&rollback,fname);+}+if(failed)+/* set to last element to break for_each loop */+item=names.items+names.nr;+}+if(failed){+structstring_listrollback_failure;+for_each_string_list_item(item,&rollback){+char*fname,*fname_old;+fname=mkpathdup("%s/%s",packdir,item->string);+fname_old=mkpathdup("%s/old-%s",packdir,item->string);+if(rename(fname_old,fname))+string_list_append(&rollback_failure,fname);+free(fname);+free(fname_old);+}++if(rollback.nr){+inti;+fprintf(stderr,+"WARNING: Some packs in use have been renamed by\n"+"WARNING: prefixing old- to their name, in order to\n"+"WARNING: replace them with the new version of the\n"+"WARNING: file. But the operation failed, and\n"+"WARNING: attempt to rename them back to their\n"+"WARNING: original names also failed.\n"+"WARNING: Please rename them in $PACKDIR manually:\n");+for(i=0;i<rollback.nr;i++)+fprintf(stderr,"WARNING: old-%s -> %s\n",+rollback.items[i].string,+rollback.items[i].string);+}+exit(1);+}++/* Now the ones with the same name are out of the way... */+for_each_string_list_item(item,&names){+char*fname,*fname_old;+fname=mkpathdup("%s/pack-%s.pack",packdir,item->string);+fname_old=mkpathdup("%s-%s.pack",packtmp,item->string);+stat(fname_old,&statbuffer);+statbuffer.st_mode&=~S_IWOTH;+chmod(fname_old,statbuffer.st_mode);+if(rename(fname_old,fname))+die("Could not rename packfile: %s -> %s",fname_old,fname);+free(fname);+free(fname_old);++fname=mkpathdup("%s/pack-%s.idx",packdir,item->string);+fname_old=mkpathdup("%s-%s.idx",packtmp,item->string);+stat(fname_old,&statbuffer);+statbuffer.st_mode&=~S_IWOTH;+chmod(fname_old,statbuffer.st_mode);+if(rename(fname_old,fname))+die("Could not rename packfile: %s -> %s",fname_old,fname);+free(fname);+free(fname_old);+}++/* Remove the "old-" files */+for_each_string_list_item(item,&names){+char*fname;+fname=mkpathdup("%s/old-pack-%s.idx",packdir,item->string);+if(remove_path(fname))+die("Could not remove file: %s",fname);+free(fname);++fname=mkpathdup("%s/old-pack-%s.pack",packdir,item->string);+if(remove_path(fname))+die("Could not remove file: %s",fname);+free(fname);+}++/* End of pack replacement. */+if(delete_redundant){+sort_string_list(&names);+for_each_string_list_item(item,&existing_packs){+if(!string_list_has_string(&names,item->string))+remove_pack(packdir,item->string);+}+argv_array_clear(&cmd_args);+argv_array_push(&cmd_args,"prune-packed");+/* TODO: pass argument: ${GIT_QUIET:+-q} */+memset(&cmd,0,sizeof(cmd));+cmd.argv=argv_array_detach(&cmd_args,NULL);+cmd.git_cmd=1;+run_command(&cmd);+}++if(!no_update_server_info){+argv_array_clear(&cmd_args);+argv_array_push(&cmd_args,"update-server-info");++memset(&cmd,0,sizeof(cmd));+cmd.argv=argv_array_detach(&cmd_args,NULL);+cmd.git_cmd=1;+run_command(&cmd);+}+return0;+}+
diff --git a/git-repack.sh b/contrib/examples/git-repack.shsimilarity index 100%rename from git-repack.shrename to contrib/examples/git-repack.shdiff --git a/git.c b/git.cindex 2025f77..03510be 100644--- a/git.c+++ b/git.c
From: Kyle J. McKay <hidden> Date: 2016-06-15 22:58:26
On Aug 18, 2013, at 07:36, Stefan Beller wrote:
+ fprintf(stderr,
+ "WARNING: Some packs in use have been renamed by\n"
+ "WARNING: prefixing old- to their name, in order to\n"
+ "WARNING: replace them with the new version of the\n"
+ "WARNING: file. But the operation failed, and\n"
+ "WARNING: attempt to rename them back to their\n"
+ "WARNING: original names also failed.\n"
Bad grammar "But the operation failed, and attempt to rename them ...".
How about "But the operation failed, and the attempt to rename
them ..." instead.
Unless sha1 == buf, this will crash when that string_list is freed
because sha1 was not returned by malloc. If it doesn't crash for
you then I guess sha1_list is never freed. :) How about just
taking the part of d_name we need, like this?
size_t len = strlen(e->d_name) - strlen(suffix);
if (len > 40) {
char *sha1 = xmemdupz(e->d_name + len - 40, 40);
string_list_append_nodup(sha1_list, sha1);
}
+ } else {
+ /*TODO: what should happen to pack files having no 40 char sha1 specifier?*/
What does the current code do with them? From a quick glance it
looks like it deletes them in the end, right?
René
From: Stefan Beller <hidden> Date: 2016-06-15 22:58:27
This is the beginning of the rewrite of the repacking.
* rename get_pack_sha1_list to get_pack_filename_list, which
* reads the pack directory only once as suggested by Rene.
* fix the grammar as suggested by Kyle.
All tests have been positive at least once now.
However there is still one non-deterministic error occuring,
I am tired to search for it now, I'll get it working tommorow.
Signed-off-by: Stefan Beller <redacted>
---
Makefile | 2 +-
builtin.h | 1 +
builtin/repack.c | 372 ++++++++++++++++++++++++
git-repack.sh => contrib/examples/git-repack.sh | 0
git.c | 1 +
5 files changed, 375 insertions(+), 1 deletion(-)
create mode 100644 builtin/repack.c
rename git-repack.sh => contrib/examples/git-repack.sh (100%)
@@ -0,0 +1,372 @@+/*+*TheshellversionwaswrittenbyLinusTorvalds(2005)andmanyothers.+*ThisisatranslationintoCbyStefanBeller(2013)+*/++#include"builtin.h"+#include"cache.h"+#include"dir.h"+#include"parse-options.h"+#include"run-command.h"+#include"sigchain.h"+#include"strbuf.h"+#include"string-list.h"++#include"argv-array.h"++staticconstchar*constgit_repack_usage[]={+N_("git repack [options]"),+NULL+};++/* enabled by default since 22c79eab (2008-06-25) */+staticintdelta_base_offset=1;++staticintrepack_config(constchar*var,constchar*value,void*cb)+{+if(!strcmp(var,"repack.usedeltabaseoffset")){+delta_base_offset=git_config_bool(var,value);+return0;+}+returngit_default_config(var,value,cb);+}++staticvoidremove_temporary_files(){+DIR*dir;+structdirent*e;+char*prefix,*path;++prefix=mkpathdup(".tmp-%d-pack",getpid());+path=mkpathdup("%s/pack",get_object_directory());++dir=opendir(path);+while((e=readdir(dir))!=NULL){+if(!prefixcmp(e->d_name,prefix)){+structstrbuffname=STRBUF_INIT;+strbuf_addf(&fname,"%s/%s",path,e->d_name);+unlink(strbuf_detach(&fname,NULL));+}+}+free(prefix);+free(path);+closedir(dir);+}++staticvoidremove_pack_on_signal(intsigno)+{+remove_temporary_files();+sigchain_pop(signo);+raise(signo);+}++/*+*Fillsthefilenamelistwithallthefilesfoundinthepackdirectory+*endingwith.pack,withoutthatextension.+*/+voidget_pack_filenames(char*packdir,structstring_list*fname_list)+{+DIR*dir;+structdirent*e;+char*path,*suffix,*fname;++path=mkpathdup("%s/pack",get_object_directory());+suffix=".pack";++dir=opendir(path);+while((e=readdir(dir))!=NULL){+if(!suffixcmp(e->d_name,suffix)){+size_tlen=strlen(e->d_name)-strlen(suffix);+fname=xmemdupz(e->d_name,len);+string_list_append_nodup(fname_list,fname);+}+}+free(path);+closedir(dir);+}++/*+*remove_packwillremoveanyfilesfollowingthepattern*${SHA1}.{EXT}+*whereEXTisoneof{pack,idx,keep}.TheSHA1consistsof40charsand+*isspecifiedbythesha1parameter.+*pathisspecifyingthedirectoryinwhichallfoundfileswillbedeleted.+*/+voidremove_pack(char*path,char*sha1)+{+char*exts[]={".pack",".idx",".keep"};+char*fname;+intext=0;+for(ext=0;ext<3;ext++){+fname=mkpathdup("%s/%s%s",path,sha1,exts[ext]);+unlink(fname);+free(fname);+}+}++intcmd_repack(intargc,constchar**argv,constchar*prefix){++intpack_everything=0;+intpack_everything_but_loose=0;+intdelete_redundant=0;+char*unpack_unreachable=NULL;+intwindow=0,window_memory=0;+intdepth=0;+intmax_pack_size=0;+intno_reuse_delta=0,no_reuse_object=0;+intno_update_server_info=0;+intquiet=0;+intlocal=0;+char*packdir,*packtmp;+structchild_processcmd;+structstring_list_item*item;+structstring_listexisting_packs=STRING_LIST_INIT_DUP;+structstatstatbuffer;++structoptionbuiltin_repack_options[]={+OPT_BOOL('a',"all",&pack_everything,+N_("pack everything in a single pack")),+OPT_BOOL('A',"all-but-loose",&pack_everything_but_loose,+N_("same as -a, and turn unreachable objects loose")),+OPT_BOOL('d',"delete-redundant",&delete_redundant,+N_("remove redundant packs, and run git-prune-packed")),+OPT_BOOL('f',"no-reuse-delta",&no_reuse_delta,+N_("pass --no-reuse-delta to git-pack-objects")),+OPT_BOOL('F',"no-reuse-object",&no_reuse_object,+N_("pass --no-reuse-object to git-pack-objects")),+OPT_BOOL('n',NULL,&no_update_server_info,+N_("do not run git-update-server-info")),+OPT__QUIET(&quiet,N_("be quiet")),+OPT_BOOL('l',"local",&local,+N_("pass --local to git-pack-objects")),+OPT_STRING(0,"unpack-unreachable",&unpack_unreachable,N_("approxidate"),+N_("with -A, do not loosen objects older than this Packing constraints")),+OPT_INTEGER(0,"window",&window,+N_("size of the window used for delta compression")),+OPT_INTEGER(0,"window-memory",&window_memory,+N_("same as the above, but limit memory size instead of entries count")),+OPT_INTEGER(0,"depth",&depth,+N_("limits the maximum delta depth")),+OPT_INTEGER(0,"max-pack-size",&max_pack_size,+N_("maximum size of each packfile")),+OPT_END()+};++git_config(repack_config,NULL);++argc=parse_options(argc,argv,prefix,builtin_repack_options,+git_repack_usage,0);++sigchain_push_common(remove_pack_on_signal);++packdir=mkpathdup("%s/pack",get_object_directory());+packtmp=mkpathdup("%s/.tmp-%d-pack",packdir,getpid());++remove_temporary_files();++structargv_arraycmd_args=ARGV_ARRAY_INIT;+argv_array_push(&cmd_args,"pack-objects");+argv_array_push(&cmd_args,"--keep-true-parents");+argv_array_push(&cmd_args,"--honor-pack-keep");+argv_array_push(&cmd_args,"--non-empty");+argv_array_push(&cmd_args,"--all");+argv_array_push(&cmd_args,"--reflog");++if(window)+argv_array_pushf(&cmd_args,"--window=%u",window);++if(window_memory)+argv_array_pushf(&cmd_args,"--window-memory=%u",window_memory);++if(depth)+argv_array_pushf(&cmd_args,"--depth=%u",depth);++if(max_pack_size)+argv_array_pushf(&cmd_args,"--max_pack_size=%u",max_pack_size);++if(pack_everything+pack_everything_but_loose==0){+argv_array_push(&cmd_args,"--unpacked");+argv_array_push(&cmd_args,"--incremental");+}else{+if(pack_everything_but_loose&&delete_redundant)+argv_array_push(&cmd_args,"--unpack-unreachable");++structstring_listfname_list=STRING_LIST_INIT_DUP;+get_pack_filenames(packdir,&fname_list);+for_each_string_list_item(item,&fname_list){+char*fname;+fname=mkpathdup("%s/%s.keep",packdir,item->string);+if(stat(fname,&statbuffer)&&S_ISREG(statbuffer.st_mode)){+/* when the keep file is there, we're ignoring that pack */+}else{+string_list_append(&existing_packs,item->string);+}+free(fname);+}++if(existing_packs.nr&&unpack_unreachable&&delete_redundant){+argv_array_pushf(&cmd_args,"--unpack-unreachable=%s",unpack_unreachable);+}+}++if(local)+argv_array_push(&cmd_args,"--local");++if(delta_base_offset)+argv_array_push(&cmd_args,"--delta-base-offset");++argv_array_push(&cmd_args,packtmp);++memset(&cmd,0,sizeof(cmd));+cmd.argv=argv_array_detach(&cmd_args,NULL);+cmd.git_cmd=1;+cmd.out=-1;+cmd.no_stdin=1;++if(run_command(&cmd))+return1;++structstring_listnames=STRING_LIST_INIT_DUP;+structstring_listrollback=STRING_LIST_INIT_DUP;++charline[1024];+intcounter=0;+FILE*out=xfdopen(cmd.out,"r");+while(fgets(line,sizeof(line),out)){+/* a line consists of 40 hex chars + '\n' */+assert(strlen(line)==41);+line[40]='\0';+string_list_append(&names,line);+counter++;+}+if(!counter)+printf("Nothing new to pack.\n");+fclose(out);++char*exts[2]={".idx",".pack"};+intfailed=0;+for_each_string_list_item(item,&names){+intext;+for(ext=0;ext<1;ext++){+char*fname,*fname_old;+fname=mkpathdup("%s/%s%s",packdir,item->string,exts[ext]);+if(!file_exists(fname)){+free(fname);+continue;+}++fname_old=mkpathdup("%s/old-%s%s",packdir,item->string,exts[ext]);+if(file_exists(fname_old))+unlink(fname_old);++if(rename(fname,fname_old)){+failed=1;+break;+}+string_list_append_nodup(&rollback,fname);+}+if(failed)+/* set to last element to break for_each loop */+item=names.items+names.nr;+}+if(failed){+structstring_listrollback_failure;+for_each_string_list_item(item,&rollback){+char*fname,*fname_old;+fname=mkpathdup("%s/%s",packdir,item->string);+fname_old=mkpathdup("%s/old-%s",packdir,item->string);+if(rename(fname_old,fname))+string_list_append(&rollback_failure,fname);+free(fname);+free(fname_old);+}++if(rollback.nr){+inti;+fprintf(stderr,+"WARNING: Some packs in use have been renamed by\n"+"WARNING: prefixing old- to their name, in order to\n"+"WARNING: replace them with the new version of the\n"+"WARNING: file. But the operation failed, and the\n"+"WARNING: attempt to rename them back to their\n"+"WARNING: original names also failed.\n"+"WARNING: Please rename them in $PACKDIR manually:\n");+for(i=0;i<rollback.nr;i++)+fprintf(stderr,"WARNING: old-%s -> %s\n",+rollback.items[i].string,+rollback.items[i].string);+}+exit(1);+}++/* Now the ones with the same name are out of the way... */+for_each_string_list_item(item,&names){+char*fname,*fname_old;+fname=mkpathdup("%s/pack-%s.pack",packdir,item->string);+fname_old=mkpathdup("%s-%s.pack",packtmp,item->string);+stat(fname_old,&statbuffer);+statbuffer.st_mode&=~S_IWOTH;+chmod(fname_old,statbuffer.st_mode);+if(rename(fname_old,fname))+die("Could not rename packfile: %s -> %s",fname_old,fname);+free(fname);+free(fname_old);++fname=mkpathdup("%s/pack-%s.idx",packdir,item->string);+fname_old=mkpathdup("%s-%s.idx",packtmp,item->string);+stat(fname_old,&statbuffer);+statbuffer.st_mode&=~S_IWOTH;+chmod(fname_old,statbuffer.st_mode);+if(rename(fname_old,fname))+die("Could not rename packfile: %s -> %s",fname_old,fname);+free(fname);+free(fname_old);+}++/* Remove the "old-" files */+for_each_string_list_item(item,&names){+char*fname;+fname=mkpathdup("%s/old-pack-%s.idx",packdir,item->string);+if(remove_path(fname))+die("Could not remove file: %s",fname);+free(fname);++fname=mkpathdup("%s/old-pack-%s.pack",packdir,item->string);+if(remove_path(fname))+die("Could not remove file: %s",fname);+free(fname);+}++/* End of pack replacement. */+if(delete_redundant){+sort_string_list(&names);+for_each_string_list_item(item,&existing_packs){+char*sha1;+size_tlen=strlen(item->string);+if(len<40)+continue;+sha1=item->string+len-40;+if(!string_list_has_string(&names,sha1))+remove_pack(packdir,item->string);+}+argv_array_clear(&cmd_args);+argv_array_push(&cmd_args,"prune-packed");+if(quiet)+argv_array_push(&cmd_args,"--quiet");++memset(&cmd,0,sizeof(cmd));+cmd.argv=argv_array_detach(&cmd_args,NULL);+cmd.git_cmd=1;+run_command(&cmd);+}++if(!no_update_server_info){+argv_array_clear(&cmd_args);+argv_array_push(&cmd_args,"update-server-info");++memset(&cmd,0,sizeof(cmd));+cmd.argv=argv_array_detach(&cmd_args,NULL);+cmd.git_cmd=1;+run_command(&cmd);+}+return0;+}+
diff --git a/git-repack.sh b/contrib/examples/git-repack.shsimilarity index 100%rename from git-repack.shrename to contrib/examples/git-repack.shdiff --git a/git.c b/git.cindex 2025f77..03510be 100644--- a/git.c+++ b/git.c
From: Stefan Beller <hidden> Date: 2016-06-15 22:58:27
Hi,
so today I compared the argument lists of the repack shell script with the
C rewrite passed on to the pack-objects command and fixed some corner
cases (-A -d --unpack-unreachable=%s should only pass
--unpack-unreachable=%s once to pack-objects)
Also I fixed some missing smaller options (--quiet, --no-reuse-delta, --no-reuse-object).
I have run the test suite several times successfully now,
trying to find a pattern for the non-deterministically bug, which appears to
only occur in 1 out of 4 test suite runs.
The test suite has around 40 calls to repack and 35 calls to gc, which calls
repack internally. That alone covers quite a lot of the repack options,
but the debugging for the test cases is no fun, as the the calls to repack
are usually not the main concern of the respective tests.
There are however
t7700-repack.sh
t7701-repack-unpack-unreachable.sh
It was suggested earlier, and I think it's a good idea to enhance those
tests.
Anyway, here is an updated version of the repack rewrite.
Stefan
--8<--
From f6da16ac3ca71aa746fe6d9224b06e6cc4e7a104 Mon Sep 17 00:00:00 2001
From: Stefan Beller <redacted>
Date: Fri, 16 Aug 2013 02:08:47 +0200
Subject: [RFC PATCHv4] repack: rewrite the shell script in C.
This is the beginning of the rewrite of the repacking.
All tests have been positive at least once now.
However there is still a non-deterministic error occuring in
about 1 out of 4 test suite runs (usually in 7701 or 9301,
but could also occur in 5501 or 3306 iirc)
Signed-off-by: Stefan Beller <redacted>
---
Makefile | 2 +-
builtin.h | 1 +
builtin/repack.c | 363 ++++++++++++++++++++++++
git-repack.sh => contrib/examples/git-repack.sh | 0
git.c | 1 +
5 files changed, 366 insertions(+), 1 deletion(-)
create mode 100644 builtin/repack.c
rename git-repack.sh => contrib/examples/git-repack.sh (100%)
@@ -0,0 +1,363 @@+/*+*TheshellversionwaswrittenbyLinusTorvalds(2005)andmanyothers.+*ThisisatranslationintoCbyStefanBeller(2013)+*/++#include"builtin.h"+#include"cache.h"+#include"dir.h"+#include"parse-options.h"+#include"run-command.h"+#include"sigchain.h"+#include"strbuf.h"+#include"string-list.h"+#include"argv-array.h"++staticintdelta_base_offset=0;++staticconstchar*constgit_repack_usage[]={+N_("git repack [options]"),+NULL+};++staticintrepack_config(constchar*var,constchar*value,void*cb)+{+if(!strcmp(var,"repack.usedeltabaseoffset")){+delta_base_offset=git_config_bool(var,value);+return0;+}+returngit_default_config(var,value,cb);+}++staticvoidremove_temporary_files(){+DIR*dir;+structdirent*e;+char*prefix,*path;++prefix=mkpathdup(".tmp-%d-pack",getpid());+path=mkpathdup("%s/pack",get_object_directory());++dir=opendir(path);+while((e=readdir(dir))!=NULL){+if(!prefixcmp(e->d_name,prefix)){+structstrbuffname=STRBUF_INIT;+strbuf_addf(&fname,"%s/%s",path,e->d_name);+unlink(strbuf_detach(&fname,NULL));+}+}+free(prefix);+free(path);+closedir(dir);+}++staticvoidremove_pack_on_signal(intsigno)+{+remove_temporary_files();+sigchain_pop(signo);+raise(signo);+}++/*+*Fillsthefilenamelistwithallthefilesfoundinthepackdirectory+*endingwith.pack,withoutthatextension.+*/+voidget_pack_filenames(char*packdir,structstring_list*fname_list)+{+DIR*dir;+structdirent*e;+char*path,*suffix,*fname;++path=mkpathdup("%s/pack",get_object_directory());+suffix=".pack";++dir=opendir(path);+while((e=readdir(dir))!=NULL){+if(!suffixcmp(e->d_name,suffix)){+size_tlen=strlen(e->d_name)-strlen(suffix);+fname=xmemdupz(e->d_name,len);+string_list_append_nodup(fname_list,fname);+}+}+free(path);+closedir(dir);+}++voidremove_pack(char*path,char*sha1)+{+char*exts[]={".pack",".idx",".keep"};+intext=0;+for(ext=0;ext<3;ext++){+char*fname;+fname=mkpathdup("%s/%s%s",path,sha1,exts[ext]);+unlink(fname);+free(fname);+}+}++intcmd_repack(intargc,constchar**argv,constchar*prefix){++intpack_everything=0;+intpack_everything_but_loose=0;+intdelete_redundant=0;+char*unpack_unreachable=NULL;+intwindow=0,window_memory=0;+intdepth=0;+intmax_pack_size=0;+intno_reuse_delta=0,no_reuse_object=0;+intno_update_server_info=0;+intquiet=0;+intlocal=0;+char*packdir,*packtmp;+structchild_processcmd;+structstring_list_item*item;+structstring_listexisting_packs=STRING_LIST_INIT_DUP;+structstatstatbuffer;+intext;+char*exts[2]={".idx",".pack"};++structoptionbuiltin_repack_options[]={+OPT_BOOL('a',"all",&pack_everything,+N_("pack everything in a single pack")),+OPT_BOOL('A',"all-but-loose",&pack_everything_but_loose,+N_("same as -a, and turn unreachable objects loose")),+OPT_BOOL('d',"delete-redundant",&delete_redundant,+N_("remove redundant packs, and run git-prune-packed")),+OPT_BOOL('f',"no-reuse-delta",&no_reuse_delta,+N_("pass --no-reuse-delta to git-pack-objects")),+OPT_BOOL('F',"no-reuse-object",&no_reuse_object,+N_("pass --no-reuse-object to git-pack-objects")),+OPT_BOOL('n',NULL,&no_update_server_info,+N_("do not run git-update-server-info")),+OPT__QUIET(&quiet,N_("be quiet")),+OPT_BOOL('l',"local",&local,+N_("pass --local to git-pack-objects")),+OPT_STRING(0,"unpack-unreachable",&unpack_unreachable,N_("approxidate"),+N_("with -A, do not loosen objects older than this Packing constraints")),+OPT_INTEGER(0,"window",&window,+N_("size of the window used for delta compression")),+OPT_INTEGER(0,"window-memory",&window_memory,+N_("same as the above, but limit memory size instead of entries count")),+OPT_INTEGER(0,"depth",&depth,+N_("limits the maximum delta depth")),+OPT_INTEGER(0,"max-pack-size",&max_pack_size,+N_("maximum size of each packfile")),+OPT_END()+};++git_config(repack_config,NULL);++argc=parse_options(argc,argv,prefix,builtin_repack_options,+git_repack_usage,0);++sigchain_push_common(remove_pack_on_signal);++packdir=mkpathdup("%s/pack",get_object_directory());+packtmp=mkpathdup("%s/.tmp-%d-pack",packdir,getpid());++remove_temporary_files();++structargv_arraycmd_args=ARGV_ARRAY_INIT;+argv_array_push(&cmd_args,"pack-objects");+argv_array_push(&cmd_args,"--keep-true-parents");+argv_array_push(&cmd_args,"--honor-pack-keep");+argv_array_push(&cmd_args,"--non-empty");+argv_array_push(&cmd_args,"--all");+argv_array_push(&cmd_args,"--reflog");++if(window)+argv_array_pushf(&cmd_args,"--window=%u",window);++if(window_memory)+argv_array_pushf(&cmd_args,"--window-memory=%u",window_memory);++if(depth)+argv_array_pushf(&cmd_args,"--depth=%u",depth);++if(max_pack_size)+argv_array_pushf(&cmd_args,"--max_pack_size=%u",max_pack_size);++if(no_reuse_delta)+argv_array_pushf(&cmd_args,"--no-reuse-delta");++if(no_reuse_object)+argv_array_pushf(&cmd_args,"--no-reuse-object");++if(pack_everything+pack_everything_but_loose==0){+argv_array_push(&cmd_args,"--unpacked");+argv_array_push(&cmd_args,"--incremental");+}else{+structstring_listfname_list=STRING_LIST_INIT_DUP;+get_pack_filenames(packdir,&fname_list);+for_each_string_list_item(item,&fname_list){+char*fname;+fname=mkpathdup("%s/%s.keep",packdir,item->string);+if(stat(fname,&statbuffer)&&S_ISREG(statbuffer.st_mode)){+/* when the keep file is there, we're ignoring that pack */+}else{+string_list_append(&existing_packs,item->string);+}+free(fname);+}++if(existing_packs.nr&&delete_redundant){+if(unpack_unreachable)+argv_array_pushf(&cmd_args,"--unpack-unreachable=%s",unpack_unreachable);+elseif(pack_everything_but_loose)+argv_array_push(&cmd_args,"--unpack-unreachable");+}+}++if(local)+argv_array_push(&cmd_args,"--local");+if(quiet)+argv_array_push(&cmd_args,"--quiet");+if(delta_base_offset)+argv_array_push(&cmd_args,"--delta-base-offset");++argv_array_push(&cmd_args,packtmp);++memset(&cmd,0,sizeof(cmd));+cmd.argv=argv_array_detach(&cmd_args,NULL);+cmd.git_cmd=1;+cmd.out=-1;+cmd.no_stdin=1;++if(run_command(&cmd))+return1;++structstring_listnames=STRING_LIST_INIT_DUP;+structstring_listrollback=STRING_LIST_INIT_DUP;++charline[1024];+intcounter=0;+FILE*out=xfdopen(cmd.out,"r");+while(fgets(line,sizeof(line),out)){+/* a line consists of 40 hex chars + '\n' */+assert(strlen(line)==41);+line[40]='\0';+string_list_append(&names,line);+counter++;+}+if(!counter)+printf("Nothing new to pack.\n");+fclose(out);++intfailed=0;+for_each_string_list_item(item,&names){+for(ext=0;ext<1;ext++){+char*fname,*fname_old;+fname=mkpathdup("%s/%s%s",packdir,item->string,exts[ext]);+if(!file_exists(fname)){+free(fname);+continue;+}++fname_old=mkpathdup("%s/old-%s%s",packdir,item->string,exts[ext]);+if(file_exists(fname_old))+unlink(fname_old);++if(rename(fname,fname_old)){+failed=1;+break;+}+free(fname_old);+string_list_append_nodup(&rollback,fname);+}+if(failed)+/* set to last element to break for_each loop */+item=names.items+names.nr;+}+if(failed){+structstring_listrollback_failure;+for_each_string_list_item(item,&rollback){+char*fname,*fname_old;+fname=mkpathdup("%s/%s",packdir,item->string);+fname_old=mkpathdup("%s/old-%s",packdir,item->string);+if(rename(fname_old,fname))+string_list_append(&rollback_failure,fname);+free(fname);+free(fname_old);+}++if(rollback.nr){+inti;+fprintf(stderr,+"WARNING: Some packs in use have been renamed by\n"+"WARNING: prefixing old- to their name, in order to\n"+"WARNING: replace them with the new version of the\n"+"WARNING: file. But the operation failed, and the\n"+"WARNING: attempt to rename them back to their\n"+"WARNING: original names also failed.\n"+"WARNING: Please rename them in $PACKDIR manually:\n");+for(i=0;i<rollback.nr;i++)+fprintf(stderr,"WARNING: old-%s -> %s\n",+rollback.items[i].string,+rollback.items[i].string);+}+exit(1);+}++/* Now the ones with the same name are out of the way... */+for_each_string_list_item(item,&names){+for(ext=0;ext<2;ext++){+char*fname,*fname_old;+fname=mkpathdup("%s/pack-%s%s",packdir,item->string,exts[ext]);+fname_old=mkpathdup("%s-%s%s",packtmp,item->string,exts[ext]);+stat(fname_old,&statbuffer);+statbuffer.st_mode&=~S_IWUSR|~S_IWGRP|~S_IWOTH;+chmod(fname_old,statbuffer.st_mode);+if(rename(fname_old,fname))+die("Could not rename packfile: %s -> %s",fname_old,fname);+free(fname);+free(fname_old);+}+}++/* Remove the "old-" files */+for_each_string_list_item(item,&names){+char*fname;+fname=mkpathdup("%s/old-pack-%s.idx",packdir,item->string);+if(remove_path(fname))+die("Could not remove file: %s",fname);+free(fname);++fname=mkpathdup("%s/old-pack-%s.pack",packdir,item->string);+if(remove_path(fname))+die("Could not remove file: %s",fname);+free(fname);+}++/* End of pack replacement. */+if(delete_redundant){+sort_string_list(&names);+for_each_string_list_item(item,&existing_packs){+char*sha1;+size_tlen=strlen(item->string);+if(len<40)+continue;+sha1=item->string+len-40;+if(!string_list_has_string(&names,sha1))+remove_pack(packdir,item->string);+}+argv_array_clear(&cmd_args);+argv_array_push(&cmd_args,"prune-packed");+if(quiet)+argv_array_push(&cmd_args,"--quiet");++memset(&cmd,0,sizeof(cmd));+cmd.argv=argv_array_detach(&cmd_args,NULL);+cmd.git_cmd=1;+run_command(&cmd);+}++if(!no_update_server_info){+argv_array_clear(&cmd_args);+argv_array_push(&cmd_args,"update-server-info");++memset(&cmd,0,sizeof(cmd));+cmd.argv=argv_array_detach(&cmd_args,NULL);+cmd.git_cmd=1;+run_command(&cmd);+}+return0;+}
diff --git a/git-repack.sh b/contrib/examples/git-repack.shsimilarity index 100%rename from git-repack.shrename to contrib/examples/git-repack.shdiff --git a/git.c b/git.cindex 2025f77..03510be 100644--- a/git.c+++ b/git.c
From: Johannes Sixt <hidden> Date: 2016-06-15 22:58:27
I didn't look at functions above cmd_repack.
Am 20.08.2013 01:23, schrieb Stefan Beller:
+int cmd_repack(int argc, const char **argv, const char *prefix) {
+
+ int pack_everything = 0;
+ int pack_everything_but_loose = 0;
+ int delete_redundant = 0;
+ char *unpack_unreachable = NULL;
+ int window = 0, window_memory = 0;
+ int depth = 0;
+ int max_pack_size = 0;
+ int no_reuse_delta = 0, no_reuse_object = 0;
+ int no_update_server_info = 0;
+ int quiet = 0;
+ int local = 0;
+ char *packdir, *packtmp;
+ struct child_process cmd;
+ struct string_list_item *item;
+ struct string_list existing_packs = STRING_LIST_INIT_DUP;
+ struct stat statbuffer;
+ int ext;
+ char *exts[2] = {".idx", ".pack"};
+
+ struct option builtin_repack_options[] = {
Are the long forms of options your invention?
+ OPT_BOOL('a', "all", &pack_everything,
+ N_("pack everything in a single pack")),
+ OPT_BOOL('A', "all-but-loose", &pack_everything_but_loose,
+ N_("same as -a, and turn unreachable objects loose")),
--all-but-loose does not express what the help text says. The long form of
-A is --all --unpack-unreachable, so it is really just a short option for
convenience. It does not need its own long form.
+ OPT_BOOL('d', "delete-redundant", &delete_redundant,
+ N_("remove redundant packs, and run git-prune-packed")),
+ OPT_BOOL('f', "no-reuse-delta", &no_reuse_delta,
+ N_("pass --no-reuse-delta to git-pack-objects")),
+ OPT_BOOL('F', "no-reuse-object", &no_reuse_object,
+ N_("pass --no-reuse-object to git-pack-objects")),
Do we want to allow --no-no-reuse-delta and --no-no-reuse-object?
+ OPT_BOOL('n', NULL, &no_update_server_info,
+ N_("do not run git-update-server-info")),
+ OPT_STRING(0, "unpack-unreachable", &unpack_unreachable, N_("approxidate"),
+ N_("with -A, do not loosen objects older than this Packing constraints")),
"Packing constraints" is a section heading, not a continuation of the
previous help text.
+ OPT_INTEGER(0, "window", &window,
+ N_("size of the window used for delta compression")),
This help text is suboptimal as the option is a count, not a "size" in the
narrow sense. But that can be changed later (as it would affect other
tools as well, I guess).
+ OPT_INTEGER(0, "window-memory", &window_memory,
+ N_("same as the above, but limit memory size instead of entries count")),
+ OPT_INTEGER(0, "depth", &depth,
+ N_("limits the maximum delta depth")),
+ OPT_INTEGER(0, "max-pack-size", &max_pack_size,
+ N_("maximum size of each packfile")),
+ OPT_END()
+ };
Should this not be
packdir = xstrdup(git_path("pack"));
packtmp = xstrdup(git_path("pack/.tmp-%d-pack", getpid()));
Perhaps make packdir and packtmp global so that the strings need not be
duplicated in get_pack_filenames and remove_temporary_files?
+
+ remove_temporary_files();
Yes, the shell script had this. But is it really necessary?
+
+ char line[1024];
+ int counter = 0;
+ FILE *out = xfdopen(cmd.out, "r");
+ while (fgets(line, sizeof(line), out)) {
+ /* a line consists of 40 hex chars + '\n' */
+ assert(strlen(line) == 41);
You cannot make assertions about input that you read from an external
command! You can die() if the expectation is not met. But I think that in
this case the only necessary expectation is that a line is not empty.
BTW, don't we have strbuf functions to read from an fd linewise?
+ line[40] = '\0';
+ string_list_append(&names, line);
+ counter++;
+ }
+ if (!counter)
+ printf("Nothing new to pack.\n");
This was 'say Nothing new to pack.'. say obeys --quiet, IIRC.
I think it's possible to attach arbitrary data to each string_list item.
We could attach the "%s/old-%s" name to the item name, then we wouldn't
need to re-construct the names here.
+ if (rename(fname_old, fname))
+ string_list_append(&rollback_failure, fname);
+ free(fname);
+ free(fname_old);
+ }
+
+ if (rollback.nr) {
+ int i;
+ fprintf(stderr,
+ "WARNING: Some packs in use have been renamed by\n"
+ "WARNING: prefixing old- to their name, in order to\n"
+ "WARNING: replace them with the new version of the\n"
+ "WARNING: file. But the operation failed, and the\n"
+ "WARNING: attempt to rename them back to their\n"
+ "WARNING: original names also failed.\n"
+ "WARNING: Please rename them in $PACKDIR manually:\n");
+ for (i = 0; i < rollback.nr; i++)
+ fprintf(stderr, "WARNING: old-%s -> %s\n",
+ rollback.items[i].string,
+ rollback.items[i].string);
+ }
+ exit(1);
+ }
+
+ /* Now the ones with the same name are out of the way... */
+ for_each_string_list_item(item, &names) {
+ for (ext = 0; ext < 2; ext++) {
+ char *fname, *fname_old;
+ fname = mkpathdup("%s/pack-%s%s", packdir, item->string, exts[ext]);
+ fname_old = mkpathdup("%s-%s%s", packtmp, item->string, exts[ext]);
Same here: git_path()?
+ stat(fname_old, &statbuffer);
We ignore errors during chmod in the shell script. But this doesn't give
you license to ignore stat() errors completely: If stat() fails, then
don't chmod() below, either.
In my opinion, it is good that you keep a large function that resembles
the structure of the shell script because it is easier to review. But
ultimately, it should be factored into smaller functions.
-- Hannes
From: Stefan Beller <hidden> Date: 2016-06-15 22:58:27
On 08/20/2013 03:31 PM, Johannes Sixt wrote:
Are the long forms of options your invention?
I tried to keep strong similarity with the shell script for
ease of review. In the shellscript the options where
put in variables having these names, so for example there was:
-f) no_reuse=--no-reuse-delta ;;
-F) no_reuse=--no-reuse-object ;;
So I used these variable names as well in here. And as I assumed
the variables are meaningful in itself.
In the shell script they may be meaningful, but with the option
parser in the C version, I overlooked the possibility for
--no-<option> being possible as you noted below.
Maybe we should inverse the logic and have the variables and options
called reuse-delta and being enabled by default.
quoted
+ OPT_BOOL('a', "all", &pack_everything,
+ N_("pack everything in a single pack")),
+ OPT_BOOL('A', "all-but-loose", &pack_everything_but_loose,
+ N_("same as -a, and turn unreachable objects loose")),
--all-but-loose does not express what the help text says. The long form
of -A is --all --unpack-unreachable, so it is really just a short option
for convenience. It does not need its own long form.
Ok, I'll keep that in mind, and will only use the varialbe tied to -A
to set the -a and --unpack-unreachable variable.
quoted
+ OPT_BOOL('d', "delete-redundant", &delete_redundant,
+ N_("remove redundant packs, and run git-prune-packed")),
+ OPT_BOOL('f', "no-reuse-delta", &no_reuse_delta,
+ N_("pass --no-reuse-delta to git-pack-objects")),
+ OPT_BOOL('F', "no-reuse-object", &no_reuse_object,
+ N_("pass --no-reuse-object to git-pack-objects")),
Do we want to allow --no-no-reuse-delta and --no-no-reuse-object?
see above, I'd try not to.
quoted
+ OPT_BOOL('n', NULL, &no_update_server_info,
+ N_("do not run git-update-server-info")),
No long option name?
This is also a negated option, so as above, maybe
we could have --update_server_info and --no-update_server_info
respectively. Talking about the shortform then: Is it possible to
negate the shortform?
+ OPT_STRING(0, "unpack-unreachable", &unpack_unreachable,
N_("approxidate"),
+ N_("with -A, do not loosen objects older than this
Packing constraints")),
"Packing constraints" is a section heading, not a continuation of the
previous help text.
quoted
+ OPT_INTEGER(0, "window", &window,
+ N_("size of the window used for delta compression")),
This help text is suboptimal as the option is a count, not a "size" in
the narrow sense. But that can be changed later (as it would affect
other tools as well, I guess).
quoted
+ OPT_INTEGER(0, "window-memory", &window_memory,
+ N_("same as the above, but limit memory size instead
of entries count")),
+ OPT_INTEGER(0, "depth", &depth,
+ N_("limits the maximum delta depth")),
+ OPT_INTEGER(0, "max-pack-size", &max_pack_size,
+ N_("maximum size of each packfile")),
+ OPT_END()
+ };
Should this not be
packdir = xstrdup(git_path("pack"));
packtmp = xstrdup(git_path("pack/.tmp-%d-pack", getpid()));
Perhaps make packdir and packtmp global so that the strings need not be
duplicated in get_pack_filenames and remove_temporary_files?
ok
quoted
+
+ remove_temporary_files();
Yes, the shell script had this. But is it really necessary?
Well I can drop it if it's not needed.
It actually should implement
rm -f "$PACKTMP"-*
and then the trap 'rm -f "$PACKTMP"-*' 0 1 2 3 15
as well.
+
+ char line[1024];
+ int counter = 0;
+ FILE *out = xfdopen(cmd.out, "r");
+ while (fgets(line, sizeof(line), out)) {
+ /* a line consists of 40 hex chars + '\n' */
+ assert(strlen(line) == 41);
You cannot make assertions about input that you read from an external
command! You can die() if the expectation is not met. But I think that
in this case the only necessary expectation is that a line is not empty.
BTW, don't we have strbuf functions to read from an fd linewise?
I'll check.
quoted
+ line[40] = '\0';
+ string_list_append(&names, line);
+ counter++;
+ }
+ if (!counter)
+ printf("Nothing new to pack.\n");
This was 'say Nothing new to pack.'. say obeys --quiet, IIRC.
I think it's possible to attach arbitrary data to each string_list item.
We could attach the "%s/old-%s" name to the item name, then we wouldn't
need to re-construct the names here.
handy! I'll try to do that.
quoted
+ if (rename(fname_old, fname))
+ string_list_append(&rollback_failure, fname);
+ free(fname);
+ free(fname_old);
+ }
+
+ if (rollback.nr) {
+ int i;
+ fprintf(stderr,
+ "WARNING: Some packs in use have been renamed by\n"
+ "WARNING: prefixing old- to their name, in order to\n"
+ "WARNING: replace them with the new version of the\n"
+ "WARNING: file. But the operation failed, and the\n"
+ "WARNING: attempt to rename them back to their\n"
+ "WARNING: original names also failed.\n"
+ "WARNING: Please rename them in $PACKDIR manually:\n");
+ for (i = 0; i < rollback.nr; i++)
+ fprintf(stderr, "WARNING: old-%s -> %s\n",
+ rollback.items[i].string,
+ rollback.items[i].string);
+ }
+ exit(1);
+ }
+
+ /* Now the ones with the same name are out of the way... */
+ for_each_string_list_item(item, &names) {
+ for (ext = 0; ext < 2; ext++) {
+ char *fname, *fname_old;
+ fname = mkpathdup("%s/pack-%s%s", packdir, item->string,
exts[ext]);
+ fname_old = mkpathdup("%s-%s%s", packtmp, item->string,
exts[ext]);
Same here: git_path()?
quoted
+ stat(fname_old, &statbuffer);
We ignore errors during chmod in the shell script. But this doesn't give
you license to ignore stat() errors completely: If stat() fails, then
don't chmod() below, either.
In my opinion, it is good that you keep a large function that resembles
the structure of the shell script because it is easier to review. But
ultimately, it should be factored into smaller functions.
-- Hannes
Hannes,
thank you very much for the review. I'll follow your suggestions and dive
deeper into the API to change your annotated lines.
Thanks,
Stefan
From: René Scharfe <hidden> Date: 2016-06-15 22:58:28
Am 20.08.2013 17:08, schrieb Stefan Beller:
On 08/20/2013 03:31 PM, Johannes Sixt wrote:
quoted
Are the long forms of options your invention?
I tried to keep strong similarity with the shell script for
ease of review. In the shellscript the options where
put in variables having these names, so for example there was:
-f) no_reuse=--no-reuse-delta ;;
-F) no_reuse=--no-reuse-object ;;
So I used these variable names as well in here. And as I assumed
the variables are meaningful in itself.
In the shell script they may be meaningful, but with the option
parser in the C version, I overlooked the possibility for
--no-<option> being possible as you noted below.
Maybe we should inverse the logic and have the variables and options
called reuse-delta and being enabled by default.
That's what git repack-objects does, which gets it passed to eventually.
But I think Johannes also wanted to point out that the git-repack.sh
doesn't recognize --no-reuse-delta, --all etc.. I think it's better to
introduce new long options in a separate patch. Switching the
programming language is big enough of a change already. :)
quoted
quoted
+ OPT_BOOL('f', "no-reuse-delta", &no_reuse_delta,
+ N_("pass --no-reuse-delta to git-pack-objects")),
+ OPT_BOOL('F', "no-reuse-object", &no_reuse_object,
+ N_("pass --no-reuse-object to git-pack-objects")),
Do we want to allow --no-no-reuse-delta and --no-no-reuse-object?
see above, I'd try not to.
The declaration above allows --reuse-delta, --no-reuse-delta and
--no-no-reuse-delta to be used. The latter looks funny, but I don't
think we need to forbid it. That said, dropping the no- and thus
declaring them the same way as repack-objects is a good idea.
quoted
quoted
+ OPT_BOOL('n', NULL, &no_update_server_info,
+ N_("do not run git-update-server-info")),
No long option name?
This is also a negated option, so as above, maybe
we could have --update_server_info and --no-update_server_info
respectively. Talking about the shortform then: Is it possible to
negate the shortform?
Words in long options are separated by dashes, so --update-server-info.
The no- prefix is provided for free by parseopt, unless the flag
PARSE_OPT_NONEG is given.
There is no automatic way to provide a short option that negates another
short option. You can build such a pair explicitly using OPTION_BIT and
OPTION_NEGBIT or with OPTION_SET_INT and different values.
Should this not be
packdir = xstrdup(git_path("pack"));
packtmp = xstrdup(git_path("pack/.tmp-%d-pack", getpid()));
Just a question for documentational purpose. ;)
Am I right suggesting the following:
`mkpathdup`::
Use parameters to build the path on the filesystem,
i.e. create required folders and then return a duplicate
of that path. The caller is responsible to free the memory
`xstrdup`::
Duplicates the given string, making the caller responsible
to free the return value. (No side effects to fs,
other global memory). Basically the same as man 2 strdup
with errorhandling.
`git_path`::
Returns a pointer to a static string buffer, so it can just
be used once or must be duplicated using xstrdup. The path
given is relative and is inside the repository.
Stefan
Should this not be
packdir = xstrdup(git_path("pack"));
packtmp = xstrdup(git_path("pack/.tmp-%d-pack", getpid()));
Just a question for documentational purpose. ;)
Am I right suggesting the following:
`mkpathdup`::
Use parameters to build the path on the filesystem,
i.e. create required folders and then return a duplicate
of that path. The caller is responsible to free the memory
Right. mkpathdup is basically just mkpath composed with xstrdup,
except that it avoids stomping on mkpath's buffers.
The corresponding almost-shortcut for xstrdup(git_path(s)) is
git_pathdup(s). But that's a minor detail.
Maybe a new Documentation/technical/api-paths.txt is in order.
Thanks,
Jonathan
Should this not be
packdir = xstrdup(git_path("pack"));
packtmp = xstrdup(git_path("pack/.tmp-%d-pack", getpid()));
Just a question for documentational purpose. ;)
Am I right suggesting the following:
`mkpathdup`::
Use parameters to build the path on the filesystem,
i.e. create required folders and then return a duplicate
of that path. The caller is responsible to free the memory
Right. mkpathdup is basically just mkpath composed with xstrdup,
except that it avoids stomping on mkpath's buffers.
The corresponding almost-shortcut for xstrdup(git_path(s)) is
git_pathdup(s). But that's a minor detail.
Maybe a new Documentation/technical/api-paths.txt is in order.
Thanks,
Jonathan
Is there a way to create a path, without being using git_path?
git_path seems to imply adding .git.
So if I have
packdir = xstrdup(git_path("pack"));
...
path = git_path("%s/%s", packdir, filename)
This produces something as:
.git/.git/objects/pack/.tmp-13199-pack-c59c5758ef159b272f6ab10cb9fadee443966e71.idx
definitely having one .git too much.
Also interesting to add would be that git_path operates in the
.git/objects directory?
Thanks,
Stefan
Should this not be
packdir = xstrdup(git_path("pack"));
packtmp = xstrdup(git_path("pack/.tmp-%d-pack", getpid()));
[...]
So if I have
packdir = xstrdup(git_path("pack"));
...
path = git_path("%s/%s", packdir, filename)
This produces something as:
.git/.git/objects/pack/.tmp-13199-pack-c59c5758ef159b272f6ab10cb9fadee443966e71.idx
definitely having one .git too much.
The version with get_object_directory() was right. The object
directory is not even necessarily under .git/, since it can be
overridden using the GIT_OBJECT_DIRECTORY envvar.
Also interesting to add would be that git_path operates in the
.git/objects directory?
git_path is for resolving paths within GIT_DIR, such as
git_path("config") and git_path("COMMIT_EDITMSG").
Jonathan
From: Stefan Beller <hidden> Date: 2016-06-15 22:58:28
So here is an update of git-repack
Thanks for all the reviews and annotations!
I think I got all the suggestions except the
use of git_path/mkpathdup.
I replaced mkpathdup by mkpath where possible,
but it's still not perfect.
I'll wait for the dokumentation patch of Jonathan,
before changing all these occurences forth and back
again.
What would be perfect here would be a function
which just does string processing and returning,
so
fname = create_string(fmt, ...);
or with duplication:
fname = create_string_dup(fmt, ...);
Ah wait! There are struct str_buf, but these
would require more lines (init, add to buffer,
get as char*)
Below there is just the diff against RFC PATCHv4,
however I'll send the whole patch as well.
Thanks,
Stefan
--8<--
From e544eb9b7bdea6c2000c5f0d3043845fb901e90b Mon Sep 17 00:00:00 2001
From: Stefan Beller <redacted>
Date: Wed, 21 Aug 2013 00:35:18 +0200
Subject: [PATCH] Suggestions of reviewers
---
builtin/repack.c | 104 +++++++++++++++++++++++++++----------------------------
1 file changed, 51 insertions(+), 53 deletions(-)
@@ -88,14 +87,25 @@ void remove_pack(char *path, char* sha1)intext=0;for(ext=0;ext<3;ext++){char*fname;-fname=mkpathdup("%s/%s%s",path,sha1,exts[ext]);+fname=mkpath("%s/%s%s",path,sha1,exts[ext]);unlink(fname);-free(fname);}}intcmd_repack(intargc,constchar**argv,constchar*prefix){+char*exts[2]={".idx",".pack"};+char*packdir,*packtmp,line[1024];+structchild_processcmd;+structstring_list_item*item;+structargv_arraycmd_args=ARGV_ARRAY_INIT;+structstring_listnames=STRING_LIST_INIT_DUP;+structstring_listrollback=STRING_LIST_INIT_DUP;+structstring_listexisting_packs=STRING_LIST_INIT_DUP;+intcount_packs,ext;+FILE*out;++/* variables to be filled by option parsing */intpack_everything=0;intpack_everything_but_loose=0;intdelete_redundant=0;
@@ -107,24 +117,17 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {intno_update_server_info=0;intquiet=0;intlocal=0;-char*packdir,*packtmp;-structchild_processcmd;-structstring_list_item*item;-structstring_listexisting_packs=STRING_LIST_INIT_DUP;-structstatstatbuffer;-intext;-char*exts[2]={".idx",".pack"};structoptionbuiltin_repack_options[]={-OPT_BOOL('a',"all",&pack_everything,+OPT_BOOL('a',NULL,&pack_everything,N_("pack everything in a single pack")),-OPT_BOOL('A',"all-but-loose",&pack_everything_but_loose,+OPT_BOOL('A',NULL,&pack_everything_but_loose,N_("same as -a, and turn unreachable objects loose")),-OPT_BOOL('d',"delete-redundant",&delete_redundant,+OPT_BOOL('d',NULL,&delete_redundant,N_("remove redundant packs, and run git-prune-packed")),-OPT_BOOL('f',"no-reuse-delta",&no_reuse_delta,+OPT_BOOL('f',NULL,&no_reuse_delta,N_("pass --no-reuse-delta to git-pack-objects")),-OPT_BOOL('F',"no-reuse-object",&no_reuse_object,+OPT_BOOL('F',NULL,&no_reuse_object,N_("pass --no-reuse-object to git-pack-objects")),OPT_BOOL('n',NULL,&no_update_server_info,N_("do not run git-update-server-info")),
@@ -191,7 +191,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {for_each_string_list_item(item,&fname_list){char*fname;fname=mkpathdup("%s/%s.keep",packdir,item->string);-if(stat(fname,&statbuffer)&&S_ISREG(statbuffer.st_mode)){+if(file_exists(fname)){/* when the keep file is there, we're ignoring that pack */}else{string_list_append(&existing_packs,item->string);
@@ -217,34 +217,34 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {argv_array_push(&cmd_args,packtmp);memset(&cmd,0,sizeof(cmd));-cmd.argv=argv_array_detach(&cmd_args,NULL);+cmd.argv=cmd_args.argv;cmd.git_cmd=1;cmd.out=-1;cmd.no_stdin=1;-if(run_command(&cmd))+if(start_command(&cmd))return1;-structstring_listnames=STRING_LIST_INIT_DUP;-structstring_listrollback=STRING_LIST_INIT_DUP;--charline[1024];-intcounter=0;-FILE*out=xfdopen(cmd.out,"r");+count_packs=0;+out=xfdopen(cmd.out,"r");while(fgets(line,sizeof(line),out)){/* a line consists of 40 hex chars + '\n' */-assert(strlen(line)==41);+if(strlen(line)!=41)+die("repack: Expecting 40 character sha1 lines only from pack-objects.");line[40]='\0';string_list_append(&names,line);-counter++;+count_packs++;}-if(!counter)-printf("Nothing new to pack.\n");+if(finish_command(&cmd))+return1;fclose(out);+if(!count_packs&&!quiet)+printf("Nothing new to pack.\n");+intfailed=0;for_each_string_list_item(item,&names){-for(ext=0;ext<1;ext++){+for(ext=0;ext<2;ext++){char*fname,*fname_old;fname=mkpathdup("%s/%s%s",packdir,item->string,exts[ext]);if(!file_exists(fname)){
@@ -260,23 +260,21 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {failed=1;break;}-free(fname_old);string_list_append_nodup(&rollback,fname);+free(fname);}if(failed)-/* set to last element to break for_each loop */-item=names.items+names.nr;+break;}if(failed){structstring_listrollback_failure;for_each_string_list_item(item,&rollback){char*fname,*fname_old;fname=mkpathdup("%s/%s",packdir,item->string);-fname_old=mkpathdup("%s/old-%s",packdir,item->string);+fname_old=mkpath("%s/old-%s",packdir,item->string);if(rename(fname_old,fname))string_list_append(&rollback_failure,fname);free(fname);-free(fname_old);}if(rollback.nr){
@@ -301,33 +299,33 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {for_each_string_list_item(item,&names){for(ext=0;ext<2;ext++){char*fname,*fname_old;+structstatstatbuffer;fname=mkpathdup("%s/pack-%s%s",packdir,item->string,exts[ext]);-fname_old=mkpathdup("%s-%s%s",packtmp,item->string,exts[ext]);-stat(fname_old,&statbuffer);-statbuffer.st_mode&=~S_IWUSR|~S_IWGRP|~S_IWOTH;-chmod(fname_old,statbuffer.st_mode);+fname_old=mkpath("%s-%s%s",packtmp,item->string,exts[ext]);+if(!stat(fname_old,&statbuffer)){+statbuffer.st_mode&=~S_IWUSR|~S_IWGRP|~S_IWOTH;+chmod(fname_old,statbuffer.st_mode);+}if(rename(fname_old,fname))-die("Could not rename packfile: %s -> %s",fname_old,fname);+die_errno(_("renaming '%s' failed"),fname_old);free(fname);-free(fname_old);}}/* Remove the "old-" files */for_each_string_list_item(item,&names){char*fname;-fname=mkpathdup("%s/old-pack-%s.idx",packdir,item->string);+fname=mkpath("%s/old-pack-%s.idx",packdir,item->string);if(remove_path(fname))-die("Could not remove file: %s",fname);-free(fname);+die_errno(_("removing '%s' failed"),fname);-fname=mkpathdup("%s/old-pack-%s.pack",packdir,item->string);+fname=mkpath("%s/old-pack-%s.pack",packdir,item->string);if(remove_path(fname))-die("Could not remove file: %s",fname);-free(fname);+die_errno(_("removing '%s' failed"),fname);}/* End of pack replacement. */+if(delete_redundant){sort_string_list(&names);for_each_string_list_item(item,&existing_packs){
@@ -0,0 +1,361 @@+/*+*TheshellversionwaswrittenbyLinusTorvalds(2005)andmanyothers.+*ThisisatranslationintoCbyStefanBeller(2013)+*/++#include"builtin.h"+#include"cache.h"+#include"dir.h"+#include"parse-options.h"+#include"run-command.h"+#include"sigchain.h"+#include"strbuf.h"+#include"string-list.h"+#include"argv-array.h"++staticintdelta_base_offset=0;++staticconstchar*constgit_repack_usage[]={+N_("git repack [options]"),+NULL+};++staticintrepack_config(constchar*var,constchar*value,void*cb)+{+if(!strcmp(var,"repack.usedeltabaseoffset")){+delta_base_offset=git_config_bool(var,value);+return0;+}+returngit_default_config(var,value,cb);+}++staticvoidremove_temporary_files(){+DIR*dir;+structdirent*e;+char*prefix,*path;++prefix=mkpathdup(".tmp-%d-pack",getpid());+path=mkpathdup("%s/pack",get_object_directory());++dir=opendir(path);+while((e=readdir(dir))!=NULL){+if(!prefixcmp(e->d_name,prefix)){+structstrbuffname=STRBUF_INIT;+strbuf_addf(&fname,"%s/%s",path,e->d_name);+unlink(strbuf_detach(&fname,NULL));+}+}+free(prefix);+free(path);+closedir(dir);+}++staticvoidremove_pack_on_signal(intsigno)+{+remove_temporary_files();+sigchain_pop(signo);+raise(signo);+}++/*+*Fillsthefilenamelistwithallthefilesfoundinthepackdirectory+*endingwith.pack,withoutthatextension.+*/+voidget_pack_filenames(char*packdir,structstring_list*fname_list)+{+DIR*dir;+structdirent*e;+char*path,*suffix,*fname;++path=mkpath("%s/pack",get_object_directory());+suffix=".pack";++dir=opendir(path);+while((e=readdir(dir))!=NULL){+if(!suffixcmp(e->d_name,suffix)){+size_tlen=strlen(e->d_name)-strlen(suffix);+fname=xmemdupz(e->d_name,len);+string_list_append_nodup(fname_list,fname);+}+}+closedir(dir);+}++voidremove_pack(char*path,char*sha1)+{+char*exts[]={".pack",".idx",".keep"};+intext=0;+for(ext=0;ext<3;ext++){+char*fname;+fname=mkpath("%s/%s%s",path,sha1,exts[ext]);+unlink(fname);+}+}++intcmd_repack(intargc,constchar**argv,constchar*prefix){++char*exts[2]={".idx",".pack"};+char*packdir,*packtmp,line[1024];+structchild_processcmd;+structstring_list_item*item;+structargv_arraycmd_args=ARGV_ARRAY_INIT;+structstring_listnames=STRING_LIST_INIT_DUP;+structstring_listrollback=STRING_LIST_INIT_DUP;+structstring_listexisting_packs=STRING_LIST_INIT_DUP;+intcount_packs,ext;+FILE*out;++/* variables to be filled by option parsing */+intpack_everything=0;+intpack_everything_but_loose=0;+intdelete_redundant=0;+char*unpack_unreachable=NULL;+intwindow=0,window_memory=0;+intdepth=0;+intmax_pack_size=0;+intno_reuse_delta=0,no_reuse_object=0;+intno_update_server_info=0;+intquiet=0;+intlocal=0;++structoptionbuiltin_repack_options[]={+OPT_BOOL('a',NULL,&pack_everything,+N_("pack everything in a single pack")),+OPT_BOOL('A',NULL,&pack_everything_but_loose,+N_("same as -a, and turn unreachable objects loose")),+OPT_BOOL('d',NULL,&delete_redundant,+N_("remove redundant packs, and run git-prune-packed")),+OPT_BOOL('f',NULL,&no_reuse_delta,+N_("pass --no-reuse-delta to git-pack-objects")),+OPT_BOOL('F',NULL,&no_reuse_object,+N_("pass --no-reuse-object to git-pack-objects")),+OPT_BOOL('n',NULL,&no_update_server_info,+N_("do not run git-update-server-info")),+OPT__QUIET(&quiet,N_("be quiet")),+OPT_BOOL('l',"local",&local,+N_("pass --local to git-pack-objects")),+OPT_STRING(0,"unpack-unreachable",&unpack_unreachable,N_("approxidate"),+N_("with -A, do not loosen objects older than this Packing constraints")),+OPT_INTEGER(0,"window",&window,+N_("size of the window used for delta compression")),+OPT_INTEGER(0,"window-memory",&window_memory,+N_("same as the above, but limit memory size instead of entries count")),+OPT_INTEGER(0,"depth",&depth,+N_("limits the maximum delta depth")),+OPT_INTEGER(0,"max-pack-size",&max_pack_size,+N_("maximum size of each packfile")),+OPT_END()+};++git_config(repack_config,NULL);++argc=parse_options(argc,argv,prefix,builtin_repack_options,+git_repack_usage,0);++sigchain_push_common(remove_pack_on_signal);++packdir=mkpathdup("%s/pack",get_object_directory());+packtmp=mkpathdup("%s/.tmp-%d-pack",packdir,getpid());++argv_array_push(&cmd_args,"pack-objects");+argv_array_push(&cmd_args,"--keep-true-parents");+argv_array_push(&cmd_args,"--honor-pack-keep");+argv_array_push(&cmd_args,"--non-empty");+argv_array_push(&cmd_args,"--all");+argv_array_push(&cmd_args,"--reflog");++if(window)+argv_array_pushf(&cmd_args,"--window=%u",window);++if(window_memory)+argv_array_pushf(&cmd_args,"--window-memory=%u",window_memory);++if(depth)+argv_array_pushf(&cmd_args,"--depth=%u",depth);++if(max_pack_size)+argv_array_pushf(&cmd_args,"--max_pack_size=%u",max_pack_size);++if(no_reuse_delta)+argv_array_pushf(&cmd_args,"--no-reuse-delta");++if(no_reuse_object)+argv_array_pushf(&cmd_args,"--no-reuse-object");++if(pack_everything+pack_everything_but_loose==0){+argv_array_push(&cmd_args,"--unpacked");+argv_array_push(&cmd_args,"--incremental");+}else{+structstring_listfname_list=STRING_LIST_INIT_DUP;+get_pack_filenames(packdir,&fname_list);+for_each_string_list_item(item,&fname_list){+char*fname;+fname=mkpathdup("%s/%s.keep",packdir,item->string);+if(file_exists(fname)){+/* when the keep file is there, we're ignoring that pack */+}else{+string_list_append(&existing_packs,item->string);+}+free(fname);+}++if(existing_packs.nr&&delete_redundant){+if(unpack_unreachable)+argv_array_pushf(&cmd_args,"--unpack-unreachable=%s",unpack_unreachable);+elseif(pack_everything_but_loose)+argv_array_push(&cmd_args,"--unpack-unreachable");+}+}++if(local)+argv_array_push(&cmd_args,"--local");+if(quiet)+argv_array_push(&cmd_args,"--quiet");+if(delta_base_offset)+argv_array_push(&cmd_args,"--delta-base-offset");++argv_array_push(&cmd_args,packtmp);++memset(&cmd,0,sizeof(cmd));+cmd.argv=cmd_args.argv;+cmd.git_cmd=1;+cmd.out=-1;+cmd.no_stdin=1;++if(start_command(&cmd))+return1;++count_packs=0;+out=xfdopen(cmd.out,"r");+while(fgets(line,sizeof(line),out)){+/* a line consists of 40 hex chars + '\n' */+if(strlen(line)!=41)+die("repack: Expecting 40 character sha1 lines only from pack-objects.");+line[40]='\0';+string_list_append(&names,line);+count_packs++;+}+if(finish_command(&cmd))+return1;+fclose(out);++if(!count_packs&&!quiet)+printf("Nothing new to pack.\n");++intfailed=0;+for_each_string_list_item(item,&names){+for(ext=0;ext<2;ext++){+char*fname,*fname_old;+fname=mkpathdup("%s/%s%s",packdir,item->string,exts[ext]);+if(!file_exists(fname)){+free(fname);+continue;+}++fname_old=mkpath("%s/old-%s%s",packdir,item->string,exts[ext]);+if(file_exists(fname_old))+unlink(fname_old);++if(rename(fname,fname_old)){+failed=1;+break;+}+string_list_append_nodup(&rollback,fname);+free(fname);+}+if(failed)+break;+}+if(failed){+structstring_listrollback_failure;+for_each_string_list_item(item,&rollback){+char*fname,*fname_old;+fname=mkpathdup("%s/%s",packdir,item->string);+fname_old=mkpath("%s/old-%s",packdir,item->string);+if(rename(fname_old,fname))+string_list_append(&rollback_failure,fname);+free(fname);+}++if(rollback.nr){+inti;+fprintf(stderr,+"WARNING: Some packs in use have been renamed by\n"+"WARNING: prefixing old- to their name, in order to\n"+"WARNING: replace them with the new version of the\n"+"WARNING: file. But the operation failed, and the\n"+"WARNING: attempt to rename them back to their\n"+"WARNING: original names also failed.\n"+"WARNING: Please rename them in $PACKDIR manually:\n");+for(i=0;i<rollback.nr;i++)+fprintf(stderr,"WARNING: old-%s -> %s\n",+rollback.items[i].string,+rollback.items[i].string);+}+exit(1);+}++/* Now the ones with the same name are out of the way... */+for_each_string_list_item(item,&names){+for(ext=0;ext<2;ext++){+char*fname,*fname_old;+structstatstatbuffer;+fname=mkpathdup("%s/pack-%s%s",packdir,item->string,exts[ext]);+fname_old=mkpath("%s-%s%s",packtmp,item->string,exts[ext]);+if(!stat(fname_old,&statbuffer)){+statbuffer.st_mode&=~S_IWUSR|~S_IWGRP|~S_IWOTH;+chmod(fname_old,statbuffer.st_mode);+}+if(rename(fname_old,fname))+die_errno(_("renaming '%s' failed"),fname_old);+free(fname);+}+}++/* Remove the "old-" files */+for_each_string_list_item(item,&names){+char*fname;+fname=mkpath("%s/old-pack-%s.idx",packdir,item->string);+if(remove_path(fname))+die_errno(_("removing '%s' failed"),fname);++fname=mkpath("%s/old-pack-%s.pack",packdir,item->string);+if(remove_path(fname))+die_errno(_("removing '%s' failed"),fname);+}++/* End of pack replacement. */++if(delete_redundant){+sort_string_list(&names);+for_each_string_list_item(item,&existing_packs){+char*sha1;+size_tlen=strlen(item->string);+if(len<40)+continue;+sha1=item->string+len-40;+if(!string_list_has_string(&names,sha1))+remove_pack(packdir,item->string);+}+argv_array_clear(&cmd_args);+argv_array_push(&cmd_args,"prune-packed");+if(quiet)+argv_array_push(&cmd_args,"--quiet");++memset(&cmd,0,sizeof(cmd));+cmd.argv=cmd_args.argv;+cmd.git_cmd=1;+run_command(&cmd);+}++if(!no_update_server_info){+argv_array_clear(&cmd_args);+argv_array_push(&cmd_args,"update-server-info");++memset(&cmd,0,sizeof(cmd));+cmd.argv=cmd_args.argv;+cmd.git_cmd=1;+run_command(&cmd);+}+return0;+}
diff --git a/git-repack.sh b/contrib/examples/git-repack.shsimilarity index 100%rename from git-repack.shrename to contrib/examples/git-repack.shdiff --git a/git.c b/git.cindex 2025f77..03510be 100644--- a/git.c+++ b/git.c
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:58:28
Stefan Beller wrote:
I think I got all the suggestions except the
use of git_path/mkpathdup.
I replaced mkpathdup by mkpath where possible,
but it's still not perfect.
No, mkpathdup is generally better unless you know what you're doing.
I'll wait for the dokumentation patch of Jonathan,
I never promised to write one. :) I would have preferred to have a
rough draft with the results of your investigations so far to start
from.
Oh well. I'll look into it tonight.
Thanks,
Jonathan
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:58:28
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
is not to end with a period. The above subject line is otherwise good
(a nice summary that quickly explains the effect, which is handy in
e.g. abbreviated changelogs from release announcements).
This is the beginning of the rewrite of the repacking.
This is a place to explain
- the motivation / intended positive effect of the patch
- any noticeable behavior changes
- complications and other hints for people looking back and trying
to understand this code
Based on the discussion before, I think the motivation is to get
closer to a goal of being able to have a core subset of git
functionality built in to git. That would mean
* people on Windows could get a copy of at least the core parts
of Git without having to install a Unix-style shell
* people deploying to servers don't have to rewrite the #! line
or worry about the PATH and quality of installed POSIX
utilities, if they are only using the built-in part written
in C
This patch is meant to be mostly a literal translation of the
git-repack script; the intent is that later patches would start using
more library facilities, but this patch is meant to be as close to a
no-op as possible so it doesn't do that kind of thing.
All tests are constantly positive now.
This kind of changes-since-the-previous-iteration information that
doesn't need to be recorded in the commit log for posterity goes
after the "---" marker.
Signed-off-by: Stefan Beller <redacted>
---
Makefile | 2 +-
[...]
quoted hunk
--- /dev/null+++ b/builtin/repack.c
@@ -0,0 +1,361 @@
[...]
+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"?
[...]
+static void remove_temporary_files() {
Style: argument list should have "void". (In C89 and C99, an empty
argument list means "having unspecified arguments" instead of "having
no arguments" as in C++.)
The names "prefix" and "path" are quite generic. What does this
function do? A comment could help, e.g.:
/*
* Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.
*/
+
+ dir = opendir(path);
+ while ((e = readdir(dir)) != NULL) {
What happens if the directory does not exist?
+ if (!prefixcmp(e->d_name, prefix)) {
The git-repack script removes $PACKTMP-*, but this code matches $PACKTMP*
instead. Intentional?
I wonder if it would make sense for buffers to share space here.
E.g. something like
{
/*
* Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.
*/
struct strbuf buf = STRBUF_INIT;
size_t dirlen, prefixlen;
DIR *dir;
struct dirent *e;
/* .git/objects/pack */
strbuf_addstr(&buf, get_object_directory());
strbuf_addstr(&buf, "/pack");
dir = opendir(buf.buf);
if (!dir)
... handle error ...
/* .git/objects/pack/.tmp-$$-pack-* */
dirlen = buf.len + 1;
strbuf_addf(&buf, "/.tmp-%d-pack-", getpid());
prefixlen = buf.len - dirlen;
while ((e = readdir(dir))) {
if (strncmp(e->d_name, buf.buf + dirlen, prefixlen))
continue;
strbuf_setlen(&buf, dirlen);
strbuf_addstr(&buf, e->d_name);
unlink(buf.buf);
}
if (closedir(dir))
... handle error ...
strbuf_release(&buf);
}
I dunno.
[...]
+/*
+ * Fills the filename list with all the files found in the pack directory
+ * ending with .pack, without that extension.
+ */
Ideally a comment opening a function will save lazy readers the
trouble of reading the body of the function, by explaining what the
function is for and giving them some reliable summary of what its
effect will be.
The above comment doesn't do either: it doesn't make it clear why
the function exists, and it doesn't make the semantics precise:
should fname_list be empty before this function is called? Are the
filenames filling it absolute or relative? What happens if packdir
is unreadable or doesn't exist? What happens to files without a .pack
extension?
Also, the above comment is in the wrong part of the file to maximally
help a lazy reader: it should be at the call site, so the reader
doesn't have to look for the function's definition at all.
Whenever I see the result of "mkpath" stored for more than a couple
of lines, I fret a little (since it's easy to scribble over the
rotating list of 4 get_pathname() buffers). Would doing
dir = opendir(mkpath(...))
directly work? By the way, why does this function both compute
packdir and take it as an argument?
+ suffix = ".pack";
Why not pass the string directly to suffixcmp and strlen?
+ dir = opendir(path);
What happens if the packdir does not exist or cannot be read, or
another error occurs?
+ while ((e = readdir(dir)) != NULL) {
+ if (!suffixcmp(e->d_name, suffix)) {
Can decrease the indent and deal with the boring case early
by reversing the test:
if (suffixcmp(...))
continue;
[...]
+void remove_pack(char *path, char* sha1)
Missing "const" on path. The * in pointers sticks to the variable
name instead of its type.
String constants are allowed in C to be assigned to a char * for
historical reasons, but it's never a good idea :), since they're
not mutable.
Array index is being assigned twice. ARRAY_SIZE could make this
clearer:
{
const char *exts[] = {".pack", ... };
int i;
for (i = 0; i < ARRAY_SIZE(exts); i++)
unlink(mkpath("%s/pack-%s%s", packdir, sha1, exts[i]));
}
Is the sha1 parameter actually a sha1?
It wasn't obvious to me at first what this function is for. Maybe
a name like remove_redundant_pack() would work. E.g.:
static void remove_redundant_pack(const char *pack_sha1)
{
const char *exts[] = {".pack", ".idx", ".keep"};
struct strbuf buf = STRBUF_INIT;
size_t plen;
int i;
strbuf_addf(&buf, "%s/pack-%s", get_object_directory(), pack_sha1);
plen = buf.len;
for (i = 0; i < ARRAY_SIZE(exts); i++) {
strbuf_setlen(&buf, plen);
strbuf_addstr(&buf, exts[i]);
unlink(buf.buf);
}
}
[...]
This filename pattern also appears higher in the file. Maybe it's
possible for them to share a constant or something? (If it's too
much fuss, no need to bother.)
Style: I'd leave out these blank lines, so the reader can see more
of the arguments in one screenful.
Are these signed or unsigned integers? What happens if I pass
a value < 0 or > INT_MAX?
[...]
+ if (pack_everything + pack_everything_but_loose == 0) {
Probably easier to read as
if (!pack_everything && !pack_everything_but_loose) {
The git-repack script uses "find", which scans the directory
recursively, though I'm not sure why. (Probably not important?)
Instead of building a temporary and potentially long string_list
and then pruning it to build another list, why not build the list
of packs without a corresponding .keep file in a single pass?
+ char *fname;
+ fname = mkpathdup("%s/%s.keep", packdir, item->string);
+ if (file_exists(fname)) {
+ /* when the keep file is there, we're ignoring that pack */
+ } else {
+ string_list_append(&existing_packs, item->string);
+ }
Simplifying, and avoiding braces around a single-line "if" body:
if (!file_exists(mkpath(...)))
string_list_append(...);
[...]
+ if (existing_packs.nr && delete_redundant) {
+ if (unpack_unreachable)
+ argv_array_pushf(&cmd_args, "--unpack-unreachable=%s", unpack_unreachable);
Long line.
[...]
+ count_packs = 0;
+ out = xfdopen(cmd.out, "r");
+ while (fgets(line, sizeof(line), out)) {
+ /* a line consists of 40 hex chars + '\n' */
Time to sleep. Stopping here for this round.
Hope that helps,
Jonathan
From: Johannes Sixt <hidden> Date: 2016-06-15 22:58:28
Am 21.08.2013 00:36, schrieb Stefan Beller:
I think I got all the suggestions except the
use of git_path/mkpathdup.
I replaced mkpathdup by mkpath where possible,
but it's still not perfect.
I'll wait for the dokumentation patch of Jonathan,
before changing all these occurences forth and back
again.
I trust Jonathan's judgement of how to use git_path, mkpath, and mkpathdup
more than my own. So, please take my earlier comments in this regard with
an appropriately large grain of salt.
Below there is just the diff against RFC PATCHv4,
however I'll send the whole patch as well.
Thanks, that is VERY helpful!
I'll comment here and have a look at the full patch later.
You should have an int ret here and use it like
ret = start_command(&cmd);
if (ret)
return ret;
to retain any exit codes from the sub-process. I know, the script didn't
preserve it:
names=$(git pack-objects ...) || exit 1
but that was not idiomatic as it should have been written as
names=$(git pack-objects ...) || exit
to forward the failure exit code.
Nice! I missed these decl-after-stmt in my earlier review.
+ count_packs = 0;
+ out = xfdopen(cmd.out, "r");
while (fgets(line, sizeof(line), out)) {
/* a line consists of 40 hex chars + '\n' */
- assert(strlen(line) == 41);
+ if (strlen(line) != 41)
+ die("repack: Expecting 40 character sha1 lines only from pack-objects.");
I agree with Jonathan that you should use strbuf_getline() here.
line[40] = '\0';
string_list_append(&names, line);
- counter++;
+ count_packs++;
}
- if (!counter)
- printf("Nothing new to pack.\n");
+ if (finish_command(&cmd))
+ return 1;
Same as above here:
ret = finish_command(&cmd);
if (ret)
return ret;
I would prefer to see
argv_array_clear(&cmd_args);
here, i.e., at the end of the current use rather than later at the
beginning of the next use. (Ditto for the other uses of cmd_args.)
fclose(out);
This should happen before finish_command(). It doesn't matter if there are
no errors, but if things go awry, closing the channel before
finish_command() avoids deadlocks.
quoted hunk
+ if (!count_packs && !quiet)
+ printf("Nothing new to pack.\n");
+
...
From: Stefan Beller <hidden> Date: 2016-06-15 22:58:28
On 08/21/2013 10:25 AM, Jonathan Nieder wrote:
quoted
+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 shell version and
of the C version and tried to match the arguments.
I must have missconfigured the test repository where
I run these differential tests.
Now that I test again, the --delta-base-offset option
shows up as default as it is documented.
Now fixing the rest of your annotations.
Stefan
From: Stefan Beller <hidden> Date: 2016-06-15 22:58:28
On 08/21/2013 10:25 AM, Jonathan Nieder wrote:
Hi,
Stefan Beller wrote:
quoted
[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 the reviews. I hope to have included every suggestion
so far or have send out mail discussing why not.
There have been quite a few changes since last round
because of so many reviews.
Here is a diff to the last sent patch, I'll also send
the whole patch on its one again.
Last time I forgot to label correctly with [RFC PATCHv5],
so the next patch should be v6.
Stefan
Changes since "[PATCH] repack: rewrite the shell script in C.":
--8<--
From 3cda569cdcd1312679c0035d151515cba7dacc59 Mon Sep 17 00:00:00 2001
From: Stefan Beller <redacted>
Date: Wed, 21 Aug 2013 12:33:13 +0200
Subject: [PATCH 2/3] Changes to last round.
* get_pack_filenames: directly check for .keep files
* packdir is a global variable now
* fix help string for parsing options.
* reenable the delta-base-offset being turned on by default
* rewrite remove_temporary_files(void), remove_redundant_pack(fname)
to use more strbuf instead of using mkpath(dup)
* beautifying the code (line length, empty lines)
Still on the todo list for this patch:
* Inspect the code for unlink, rename and see if we
need to deal with their return codes.
* Check for datatypes (--window-memory could use ulong?)
Later:
* Move parts of cmd_repack to extra functions
* check if subprocesses are needed (update-server-info,
prune-packed)
---
builtin/repack.c | 191
++++++++++++++++++++++++++++++-------------------------
1 file changed, 103 insertions(+), 88 deletions(-)
char *prefix) {
OPT_BOOL('l', "local", &local,
N_("pass --local to git-pack-objects")),
OPT_STRING(0, "unpack-unreachable", &unpack_unreachable,
N_("approxidate"),
- N_("with -A, do not loosen objects older than this Packing
constraints")),
+ N_("with -A, do not loosen objects older than this")),
OPT_INTEGER(0, "window", &window,
N_("size of the window used for delta compression")),
OPT_INTEGER(0, "window-memory", &window_memory,
char *prefix) {
"WARNING: file. But the operation failed, and the\n"
"WARNING: attempt to rename them back to their\n"
"WARNING: original names also failed.\n"
- "WARNING: Please rename them in $PACKDIR manually:\n");
+ "WARNING: Please rename them in %s manually:\n", packdir);
for (i = 0; i < rollback.nr; i++)
fprintf(stderr, "WARNING: old-%s -> %s\n",
rollback.items[i].string,
From: Stefan Beller <hidden> Date: 2016-06-15 22:58:28
The motivation of this patch is to get closer to a goal of being
able to have a core subset of git functionality built in to git.
That would mean
* people on Windows could get a copy of at least the core parts
of Git without having to install a Unix-style shell
* people deploying to servers don't have to rewrite the #! line
or worry about the PATH and quality of installed POSIX
utilities, if they are only using the built-in part written
in C
This patch is meant to be mostly a literal translation of the
git-repack script; the intent is that later patches would start using
more library facilities, but this patch is meant to be as close to a
no-op as possible so it doesn't do that kind of thing.
Signed-off-by: Stefan Beller <redacted>
---
Makefile | 2 +-
builtin.h | 1 +
builtin/repack.c | 376 ++++++++++++++++++++++++
git-repack.sh => contrib/examples/git-repack.sh | 0
git.c | 1 +
5 files changed, 379 insertions(+), 1 deletion(-)
create mode 100644 builtin/repack.c
rename git-repack.sh => contrib/examples/git-repack.sh (100%)
@@ -0,0 +1,376 @@+/*+*TheshellversionwaswrittenbyLinusTorvalds(2005)andmanyothers.+*ThisisatranslationintoCbyStefanBeller(2013)+*/++#include"builtin.h"+#include"cache.h"+#include"dir.h"+#include"parse-options.h"+#include"run-command.h"+#include"sigchain.h"+#include"strbuf.h"+#include"string-list.h"+#include"argv-array.h"++/* enabled by default since 22c79eab (2008-06-25) */+staticintdelta_base_offset=1;+char*packdir;++staticconstchar*constgit_repack_usage[]={+N_("git repack [options]"),+NULL+};++staticintrepack_config(constchar*var,constchar*value,void*cb)+{+if(!strcmp(var,"repack.usedeltabaseoffset")){+delta_base_offset=git_config_bool(var,value);+return0;+}+returngit_default_config(var,value,cb);+}++/*+*Removetemporary$GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-*files.+*/+staticvoidremove_temporary_files(void)+{+structstrbufbuf=STRBUF_INIT;+size_tdirlen,prefixlen;+DIR*dir;+structdirent*e;++/* .git/objects/pack */+strbuf_addstr(&buf,get_object_directory());+strbuf_addstr(&buf,"/pack");+dir=opendir(buf.buf);+if(!dir){+strbuf_release(&buf);+return;+}++/* .git/objects/pack/.tmp-$$-pack-* */+dirlen=buf.len+1;+strbuf_addf(&buf,"/.tmp-%d-pack-",(int)getpid());+prefixlen=buf.len-dirlen;++while((e=readdir(dir))){+if(strncmp(e->d_name,buf.buf+dirlen,prefixlen))+continue;+strbuf_setlen(&buf,dirlen);+strbuf_addstr(&buf,e->d_name);+unlink(buf.buf);+}+closedir(dir);+strbuf_release(&buf);+}++staticvoidremove_pack_on_signal(intsigno)+{+remove_temporary_files();+sigchain_pop(signo);+raise(signo);+}++staticvoidget_pack_filenames(structstring_list*fname_list)+{+DIR*dir;+structdirent*e;+char*fname;++if(!(dir=opendir(packdir)))+return;++while((e=readdir(dir))!=NULL){+if(suffixcmp(e->d_name,".pack"))+continue;++size_tlen=strlen(e->d_name)-strlen(".pack");+fname=xmemdupz(e->d_name,len);++if(!file_exists(mkpath("%s/%s.keep",packdir,fname)))+string_list_append_nodup(fname_list,fname);+}+closedir(dir);+}++staticvoidremove_redundant_pack(constchar*path,constchar*sha1)+{+constchar*exts[]={".pack",".idx",".keep"};+inti;+structstrbufbuf=STRBUF_INIT;+size_tplen;++strbuf_addf(&buf,"%s/%s",path,sha1);+plen=buf.len;++for(i=0;i<ARRAY_SIZE(exts);i++){+strbuf_setlen(&buf,plen);+strbuf_addstr(&buf,exts[i]);+unlink(buf.buf);+}+}++intcmd_repack(intargc,constchar**argv,constchar*prefix)+{+constchar*exts[2]={".idx",".pack"};+char*packtmp;+structchild_processcmd;+structstring_list_item*item;+structargv_arraycmd_args=ARGV_ARRAY_INIT;+structstring_listnames=STRING_LIST_INIT_DUP;+structstring_listrollback=STRING_LIST_INIT_DUP;+structstring_listexisting_packs=STRING_LIST_INIT_DUP;+structstrbufline=STRBUF_INIT;+intcount_packs,ext,ret;+FILE*out;++/* variables to be filled by option parsing */+intpack_everything=0;+intpack_everything_but_loose=0;+intdelete_redundant=0;+char*unpack_unreachable=NULL;+intwindow=0,window_memory=0;+intdepth=0;+intmax_pack_size=0;+intno_reuse_delta=0,no_reuse_object=0;+intno_update_server_info=0;+intquiet=0;+intlocal=0;++structoptionbuiltin_repack_options[]={+OPT_BOOL('a',NULL,&pack_everything,+N_("pack everything in a single pack")),+OPT_BOOL('A',NULL,&pack_everything_but_loose,+N_("same as -a, and turn unreachable objects loose")),+OPT_BOOL('d',NULL,&delete_redundant,+N_("remove redundant packs, and run git-prune-packed")),+OPT_BOOL('f',NULL,&no_reuse_delta,+N_("pass --no-reuse-delta to git-pack-objects")),+OPT_BOOL('F',NULL,&no_reuse_object,+N_("pass --no-reuse-object to git-pack-objects")),+OPT_BOOL('n',NULL,&no_update_server_info,+N_("do not run git-update-server-info")),+OPT__QUIET(&quiet,N_("be quiet")),+OPT_BOOL('l',"local",&local,+N_("pass --local to git-pack-objects")),+OPT_STRING(0,"unpack-unreachable",&unpack_unreachable,N_("approxidate"),+N_("with -A, do not loosen objects older than this")),+OPT_INTEGER(0,"window",&window,+N_("size of the window used for delta compression")),+OPT_INTEGER(0,"window-memory",&window_memory,+N_("same as the above, but limit memory size instead of entries count")),+OPT_INTEGER(0,"depth",&depth,+N_("limits the maximum delta depth")),+OPT_INTEGER(0,"max-pack-size",&max_pack_size,+N_("maximum size of each packfile")),+OPT_END()+};++git_config(repack_config,NULL);++argc=parse_options(argc,argv,prefix,builtin_repack_options,+git_repack_usage,0);++sigchain_push_common(remove_pack_on_signal);++packdir=mkpathdup("%s/pack",get_object_directory());+packtmp=mkpathdup("%s/.tmp-%d-pack",packdir,(int)getpid());++argv_array_push(&cmd_args,"pack-objects");+argv_array_push(&cmd_args,"--keep-true-parents");+argv_array_push(&cmd_args,"--honor-pack-keep");+argv_array_push(&cmd_args,"--non-empty");+argv_array_push(&cmd_args,"--all");+argv_array_push(&cmd_args,"--reflog");+if(window)+argv_array_pushf(&cmd_args,"--window=%u",window);+if(window_memory)+argv_array_pushf(&cmd_args,"--window-memory=%u",window_memory);+if(depth)+argv_array_pushf(&cmd_args,"--depth=%u",depth);+if(max_pack_size)+argv_array_pushf(&cmd_args,"--max_pack_size=%u",max_pack_size);+if(no_reuse_delta)+argv_array_pushf(&cmd_args,"--no-reuse-delta");+if(no_reuse_object)+argv_array_pushf(&cmd_args,"--no-reuse-object");++if(!pack_everything&&!pack_everything_but_loose){+argv_array_push(&cmd_args,"--unpacked");+argv_array_push(&cmd_args,"--incremental");+}else{+get_pack_filenames(&existing_packs);++if(existing_packs.nr&&delete_redundant){+if(unpack_unreachable)+argv_array_pushf(&cmd_args,+"--unpack-unreachable=%s",+unpack_unreachable);+elseif(pack_everything_but_loose)+argv_array_push(&cmd_args,+"--unpack-unreachable");+}+}++if(local)+argv_array_push(&cmd_args,"--local");+if(quiet)+argv_array_push(&cmd_args,"--quiet");+if(delta_base_offset)+argv_array_push(&cmd_args,"--delta-base-offset");++argv_array_push(&cmd_args,packtmp);++memset(&cmd,0,sizeof(cmd));+cmd.argv=cmd_args.argv;+cmd.git_cmd=1;+cmd.out=-1;+cmd.no_stdin=1;++ret=start_command(&cmd);+if(ret)+return1;++count_packs=0;+out=xfdopen(cmd.out,"r");+while(strbuf_getline(&line,out,'\n')!=EOF){+if(line.len!=40)+die("repack: Expecting 40 character sha1 lines only from pack-objects.");+strbuf_addstr(&line,"");+string_list_append(&names,line.buf);+count_packs++;+}+fclose(out);+ret=finish_command(&cmd);+if(ret)+return1;+argv_array_clear(&cmd_args);++if(!count_packs&&!quiet)+printf("Nothing new to pack.\n");++intfailed=0;+for_each_string_list_item(item,&names){+for(ext=0;ext<2;ext++){+char*fname,*fname_old;+fname=mkpathdup("%s/%s%s",packdir,+item->string,exts[ext]);+if(!file_exists(fname)){+free(fname);+continue;+}++fname_old=mkpathdup("%s/old-%s%s",packdir,+item->string,exts[ext]);+if(file_exists(fname_old))+unlink(fname_old);++if(rename(fname,fname_old)){+failed=1;+break;+}+string_list_append_nodup(&rollback,fname);+free(fname);+free(fname_old);+}+if(failed)+break;+}+if(failed){+structstring_listrollback_failure;+for_each_string_list_item(item,&rollback){+char*fname,*fname_old;+fname=mkpathdup("%s/%s",packdir,item->string);+fname_old=mkpath("%s/old-%s",packdir,item->string);+if(rename(fname_old,fname))+string_list_append(&rollback_failure,fname);+free(fname);+}++if(rollback.nr){+inti;+fprintf(stderr,+"WARNING: Some packs in use have been renamed by\n"+"WARNING: prefixing old- to their name, in order to\n"+"WARNING: replace them with the new version of the\n"+"WARNING: file. But the operation failed, and the\n"+"WARNING: attempt to rename them back to their\n"+"WARNING: original names also failed.\n"+"WARNING: Please rename them in %s manually:\n",packdir);+for(i=0;i<rollback.nr;i++)+fprintf(stderr,"WARNING: old-%s -> %s\n",+rollback.items[i].string,+rollback.items[i].string);+}+exit(1);+}++/* Now the ones with the same name are out of the way... */+for_each_string_list_item(item,&names){+for(ext=0;ext<2;ext++){+char*fname,*fname_old;+structstatstatbuffer;+fname=mkpathdup("%s/pack-%s%s",+packdir,item->string,exts[ext]);+fname_old=mkpathdup("%s-%s%s",+packtmp,item->string,exts[ext]);+if(!stat(fname_old,&statbuffer)){+statbuffer.st_mode&=~(S_IWUSR|S_IWGRP|S_IWOTH);+chmod(fname_old,statbuffer.st_mode);+}+if(rename(fname_old,fname))+die_errno(_("renaming '%s' failed"),fname_old);+free(fname);+free(fname_old);+}+}++/* Remove the "old-" files */+for_each_string_list_item(item,&names){+for(ext=0;ext<2;ext++){+char*fname;+fname=mkpath("%s/old-pack-%s%s",+packdir,+item->string,+exts[ext]);+if(remove_path(fname))+warning(_("removing '%s' failed"),fname);+}+}++/* End of pack replacement. */++if(delete_redundant){+sort_string_list(&names);+for_each_string_list_item(item,&existing_packs){+char*sha1;+size_tlen=strlen(item->string);+if(len<40)+continue;+sha1=item->string+len-40;+if(!string_list_has_string(&names,sha1))+remove_redundant_pack(packdir,item->string);+}+argv_array_push(&cmd_args,"prune-packed");+if(quiet)+argv_array_push(&cmd_args,"--quiet");++memset(&cmd,0,sizeof(cmd));+cmd.argv=cmd_args.argv;+cmd.git_cmd=1;+run_command(&cmd);+argv_array_clear(&cmd_args);+}++if(!no_update_server_info){+argv_array_push(&cmd_args,"update-server-info");+memset(&cmd,0,sizeof(cmd));+cmd.argv=cmd_args.argv;+cmd.git_cmd=1;+run_command(&cmd);+argv_array_clear(&cmd_args);+}+return0;+}
diff --git a/git-repack.sh b/contrib/examples/git-repack.shsimilarity index 100%rename from git-repack.shrename to contrib/examples/git-repack.shdiff --git a/git.c b/git.cindex 2025f77..03510be 100644--- a/git.c+++ b/git.c
From: Stefan Beller <hidden> Date: 2016-06-15 22:58:28
During the review process of the previous commit (repack: rewrite the
shell script in C), Johannes Sixt proposed to retain any exit codes from
the sub-process, which makes it probably more obvious in case of failure.
As the commit before should behave as close to the original shell
script, the proposed change is put in this extra commit.
The infrastructure however was already setup in the previous commit.
(Having a local 'ret' variable)
Signed-off-by: Stefan Beller <redacted>
---
builtin/repack.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Should this not be
packdir = xstrdup(git_path("pack"));
packtmp = xstrdup(git_path("pack/.tmp-%d-pack", getpid()));
[...]
quoted
So if I have
packdir = xstrdup(git_path("pack"));
...
path = git_path("%s/%s", packdir, filename)
This produces something as:
.git/.git/objects/pack/.tmp-13199-pack-c59c5758ef159b272f6ab10cb9fadee443966e71.idx
definitely having one .git too much.
The version with get_object_directory() was right. The object
directory is not even necessarily under .git/, since it can be
overridden using the GIT_OBJECT_DIRECTORY envvar.
quoted
Also interesting to add would be that git_path operates in the
.git/objects directory?
git_path is for resolving paths within GIT_DIR, such as
git_path("config") and git_path("COMMIT_EDITMSG").
Jonathan
Before we're doing double work, I just wrote down my understanding
so far. Feel free to tweak it, or remove obvious parts.
Thanks,
Stefan
---
path API
========
The functions described in this document are meant to be
used when dealing with pathes in the filesystem. The functions
are just for the string manipulations of the pathes, none of
the functions touches the actual filesystem.
`mkpath`::
The parameters are in printf format. This function can be
used to construct short-lived filename strings. It is meant
to be used for direct use in system functions such as
dir(mkpath("%s/pack", get_objects_directory())).
The return value is a pointer to such a sanitized filename
string, but it resides in a static buffer, so it will
be overwritten by the next call to mkpath (or other functions?)
This function only does string handling. It doesn't actually
change anything on the filesystem. (This is not Gits mkdir -p)
`mkpathdup`::
The same as mkpath, but the memory is duplicated into a new
buffer, so it is not short-lived, but stays as long as the
caller doesn't free the memory, which the caller is supposed
to do.
`xstrdup`::
Duplicates the given string, making the caller responsible
to free the return value. Basically the same as strdup(2)
with errorhandling.
I am not sure if this belongs into the path api documentation,
but it's not documented anywhere else.
`git_path`::
git_path is for resolving paths within GIT_DIR, such as
git_path("config") and git_path("COMMIT_EDITMSG").
This is similar to mkpath, returning a pointer to a static
buffer, which may be overwritten soon.
`git_pathdup`::
The same as git_path, but creating a new buffer. The caller
is responsible to free the returned buffer.
`git_path_submodule`::
`mksnpath`::
`git_snpath`::
`sha1_file_name`::
Returns the filename to a given sha1 value within
the objects directory.
`sha1_pack_name`::
`sha1_pack_index_name`::