From: Stefan Beller <hidden> Date: 2018-02-13 01:22:53
This is a real take on the first part of the recent RFC[1].
Jonathan Tan suggested[2] that "sha1_loose_object_info to handle arbitrary repositories"
might be a good breaking point for a first part at that RFC at patch 38.
This series is smaller and contains only 26 patches as the patches in the big
RFC were slightly out of order.
I developed this series partly by writing patches, but mostly by cherrypicking
from that RFC on top of current master. I noticed no external conflicts apart
from one addition to the repositories _INIT macro, which was easy to resolve.
Comments in the early range of that RFC were on 003 where Junio pointed out
that the coccinelle patch ought to be not in contrib/coccinelle, so I put it
in a sub directory there, as 'make coccicheck' doesn't traverse subdirs.
brian had a questoin on patch 25 in the RFC, but that seemed to resolve itself
without any suggestion to include into this series[3].
Duy suggested that we shall not use the repository blindly, but should carefully
examine whether to pass on an object store or the refstore or such[4], which
I agree with if it makes sense. This series unfortunately has an issue with that
as I would not want to pass down the `ignore_env` flag separately from the object
store, so I made all functions that only take the object store to have the raw
object store as the first parameter, and others using the full repository.
Eric Sunshine brought up memory leaks with the RFC, and I would think to
have plugged all holes.
[1] https://public-inbox.org/git/20180205235508.216277-1-sbeller@google.com/
[2] https://public-inbox.org/git/20180207143300.ce1c39ca07f6a0d64fe0e7ca@google.com/
[3] https://public-inbox.org/git/20180206011940.GD7904@genre.crustytoothpaste.net/
[4] https://public-inbox.org/git/CACsJy8CGgekpX4cZkyyTSPrj87uQVKZSOL7fyT__P2dh_1LmVQ@mail.gmail.com/
Thanks,
Stefan
Jonathan Nieder (8):
pack: move prepare_packed_git_run_once to object store
pack: move approximate object count to object store
sha1_file: add repository argument to sha1_file_name
sha1_file: add repository argument to map_sha1_file
sha1_file: allow stat_sha1_file to handle arbitrary repositories
sha1_file: allow open_sha1_file to handle arbitrary repositories
sha1_file: allow map_sha1_file_1 to handle arbitrary repositories
sha1_file: allow sha1_loose_object_info to handle arbitrary
repositories
Stefan Beller (18):
repository: introduce raw object store field
object-store: move alt_odb_list and alt_odb_tail to object store
object-store: free alt_odb_list
object-store: move packed_git and packed_git_mru to object store
object-store: close all packs upon clearing the object store
sha1_file: add raw_object_store argument to alt_odb_usable
sha1_file: add repository argument to link_alt_odb_entry
sha1_file: add repository argument to read_info_alternates
sha1_file: add repository argument to link_alt_odb_entries
sha1_file: add repository argument to prepare_alt_odb
sha1_file: allow link_alt_odb_entries to handle arbitrary repositories
sha1_file: allow prepare_alt_odb to handle arbitrary repositories
sha1_file: add repository argument to stat_sha1_file
sha1_file: add repository argument to open_sha1_file
sha1_file: add repository argument to map_sha1_file_1
sha1_file: add repository argument to sha1_loose_object_info
sha1_file: allow sha1_file_name to handle arbitrary repositories
sha1_file: allow map_sha1_file to handle arbitrary repositories
builtin/am.c | 2 +-
builtin/clone.c | 2 +-
builtin/count-objects.c | 6 +-
builtin/fetch.c | 2 +-
builtin/fsck.c | 13 ++-
builtin/gc.c | 4 +-
builtin/grep.c | 2 +-
builtin/index-pack.c | 1 +
builtin/merge.c | 2 +-
builtin/pack-objects.c | 21 ++--
builtin/pack-redundant.c | 6 +-
builtin/receive-pack.c | 3 +-
cache.h | 46 ++------
contrib/coccinelle/refactoring/packed_git.cocci | 7 ++
environment.c | 5 +-
fast-import.c | 6 +-
http-backend.c | 6 +-
http-push.c | 1 +
http-walker.c | 4 +-
http.c | 6 +-
mru.h | 1 +
object-store.h | 75 +++++++++++++
object.c | 26 +++++
pack-bitmap.c | 4 +-
pack-check.c | 1 +
pack-revindex.c | 1 +
packfile.c | 64 +++++------
packfile.h | 2 +-
path.c | 2 +-
reachable.c | 1 +
repository.c | 17 ++-
repository.h | 7 +-
server-info.c | 6 +-
sha1_file.c | 135 +++++++++++++-----------
sha1_name.c | 11 +-
streaming.c | 5 +-
36 files changed, 321 insertions(+), 182 deletions(-)
create mode 100644 contrib/coccinelle/refactoring/packed_git.cocci
create mode 100644 object-store.h
--
2.16.1.73.ga2c3e9663f.dirty
From: Stefan Beller <hidden> Date: 2018-02-13 01:22:55
The raw object store field will contain any objects needed for
access to objects in a given repository.
This patch introduces the raw object store and populates it with the
`objectdir`, which used to be part of the repository struct.
As the struct gains members, we'll also populate the function to clear
the memory for these members.
In a later we'll introduce a struct object_parser, that will complement
the object parsing in a repository struct: The raw object parser is the
layer that will provide access to raw object content, while the higher
level object parser code will parse raw objects and keeps track of
parenthood and other object relationships using 'struct object'.
For now only add the lower level to the repository struct.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/grep.c | 2 +-
environment.c | 5 +++--
object-store.h | 15 +++++++++++++++
object.c | 5 +++++
path.c | 2 +-
repository.c | 17 +++++++++++++----
repository.h | 7 ++++---
7 files changed, 42 insertions(+), 11 deletions(-)
create mode 100644 object-store.h
@@ -1,11 +1,18 @@#include"cache.h"#include"repository.h"+#include"object-store.h"#include"config.h"#include"submodule-config.h"/* The main repository */staticstructrepositorythe_repo={-NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,&hash_algos[GIT_HASH_SHA1],0,0+NULL,NULL,+RAW_OBJECT_STORE_INIT,+NULL,NULL,NULL,+NULL,NULL,NULL,+&the_index,+&hash_algos[GIT_HASH_SHA1],+0,0};structrepository*the_repository=&the_repo;
From: Stefan Beller <hidden> Date: 2018-02-13 01:22:57
In a process with multiple repositories open, alternates should be
associated to a single repository and not shared globally. Move
alt_odb_list and alt_odb_tail into the_repository and adjust callers
to reflect this.
Now that the alternative object data base is per repository, we're
leaking its memory upon freeing a repository. The next patch plugs
this hole.
No functional change intended.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/fsck.c | 4 +++-
cache.h | 4 ++--
object-store.h | 7 ++++++-
packfile.c | 3 ++-
sha1_file.c | 25 ++++++++++++-------------
sha1_name.c | 3 ++-
6 files changed, 27 insertions(+), 19 deletions(-)
@@ -1573,7 +1573,7 @@ extern int has_dirs_only_path(const char *name, int len, int prefix_len);externvoidschedule_dir_for_removal(constchar*name,intlen);externvoidremove_scheduled_dirs(void);-externstructalternate_object_database{+structalternate_object_database{structalternate_object_database*next;/* see alt_scratch_buf() */
@@ -563,7 +561,7 @@ void add_to_alternates_file(const char *reference)fprintf_or_die(out,"%s\n",reference);if(commit_lock_file(&lock))die_errno("unable to move new alternates file into place");-if(alt_odb_tail)+if(the_repository->objects.alt_odb_tail)link_alt_odb_entries(reference,'\n',NULL,0);}free(alts);
@@ -661,7 +659,7 @@ int foreach_alt_odb(alt_odb_fn fn, void *cb)intr=0;prepare_alt_odb();-for(ent=alt_odb_list;ent;ent=ent->next){+for(ent=the_repository->objects.alt_odb_list;ent;ent=ent->next){r=fn(ent,cb);if(r)break;
From: Stefan Beller <hidden> Date: 2018-02-13 01:23:06
From: Jonathan Nieder <redacted>
The approximate_object_count() function maintains a rough count of
objects in a repository to estimate how long object name abbreviates
should be. Object names are scoped to a repository and the
appropriate length may differ by repository, so the object count
should not be global.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
object-store.h | 10 +++++++++-
packfile.c | 11 +++++------
2 files changed, 14 insertions(+), 7 deletions(-)
@@ -684,7 +687,7 @@ void prepare_alt_odb(void)&the_repository->objects.alt_odb_list;link_alt_odb_entries(alt,PATH_SEP,NULL,0);-read_info_alternates(get_object_directory(),0);+read_info_alternates(the_repository,get_object_directory(),0);}/* Returns 1 if we have successfully freshened the file, 0 otherwise. */
From: Stefan Beller <hidden> Date: 2018-02-13 01:23:10
Add a raw_object_store to alt_odb_usable to be more specific about which
repository to act on. The choice of the repository is delegated to its
only caller link_alt_odb_entry.
Signed-off-by: Stefan Beller <redacted>
---
sha1_file.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
@@ -569,7 +573,8 @@ void add_to_alternates_file(const char *reference)if(commit_lock_file(&lock))die_errno("unable to move new alternates file into place");if(the_repository->objects.alt_odb_tail)-link_alt_odb_entries(reference,'\n',NULL,0);+link_alt_odb_entries(the_repository,reference,+'\n',NULL,0);}free(alts);}
From: Stefan Beller <hidden> Date: 2018-02-13 01:23:13
Add a repository argument to allow the link_alt_odb_entry caller to be
more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.
Since the implementation does not yet work with other repositories,
use a wrapper macro to enforce that the caller passes in
the_repository as the first argument. It would be more appealing to
use BUILD_ASSERT_OR_ZERO to enforce this, but that doesn't work
because it requires a compile-time constant and common compilers like
gcc 4.8.4 do not consider "r == the_repository" a compile-time
constant.
This and the following three patches add repository arguments to
link_alt_odb_entry, read_info_alternates, link_alt_odb_entries
and prepare_alt_odb. Three out of the four functions are found
in a recursive call chain, calling each other, and one of them
accesses the repositories `objectdir` (which was migrated; it
was an obvious choice) and `ignore_env` (which we need to keep in
the repository struct for clarify); hence we will pass through the
repository unlike just the object store object + the ignore_env flag.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
sha1_file.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Stefan Beller <hidden> Date: 2018-02-13 01:23:16
From: Jonathan Nieder <redacted>
Add a repository argument to allow sha1_file_name callers to be more
specific about which repository to handle. This is a small mechanical
change; it doesn't change the implementation to handle repositories
other than the_repository yet.
As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.
While at it, move the declaration to object-store.h, where it should
be easier to find.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
cache.h | 8 --------
http-walker.c | 3 ++-
http.c | 5 +++--
object-store.h | 9 +++++++++
sha1_file.c | 11 ++++++-----
5 files changed, 20 insertions(+), 16 deletions(-)
@@ -721,7 +721,8 @@ int check_and_freshen_file(const char *fn, int freshen)staticintcheck_and_freshen_local(constunsignedchar*sha1,intfreshen){-returncheck_and_freshen_file(sha1_file_name(sha1),freshen);+returncheck_and_freshen_file(sha1_file_name(the_repository,sha1),+freshen);}staticintcheck_and_freshen_nonlocal(constunsignedchar*sha1,intfreshen)
@@ -878,7 +879,7 @@ static int stat_sha1_file(const unsigned char *sha1, struct stat *st,{structalternate_object_database*alt;-*path=sha1_file_name(sha1);+*path=sha1_file_name(the_repository,sha1);if(!lstat(*path,st))return0;
@@ -677,21 +677,22 @@ int foreach_alt_odb(alt_odb_fn fn, void *cb)returnr;}-voidprepare_alt_odb_the_repository(void)+voidprepare_alt_odb(structrepository*r){-constchar*alt;--if(the_repository->objects.alt_odb_tail)+if(r->objects.alt_odb_tail)return;-alt=getenv(ALTERNATE_DB_ENVIRONMENT);+r->objects.alt_odb_tail=&r->objects.alt_odb_list;++if(!r->ignore_env){+constchar*alt=getenv(ALTERNATE_DB_ENVIRONMENT);+if(!alt)+alt="";-the_repository->objects.alt_odb_tail=-&the_repository->objects.alt_odb_list;-link_alt_odb_entries(the_repository,alt,-PATH_SEP,NULL,0);+link_alt_odb_entries(r,alt,PATH_SEP,NULL,0);+}-read_info_alternates(the_repository,get_object_directory(),0);+read_info_alternates(r,r->objects.objectdir,0);}/* Returns 1 if we have successfully freshened the file, 0 otherwise. */
From: Stefan Beller <hidden> Date: 2018-02-13 01:23:23
From: Jonathan Nieder <redacted>
Add a repository argument to allow map_sha1_file callers to be more
specific about which repository to handle. This is a small mechanical
change; it doesn't change the implementation to handle repositories
other than the_repository yet.
As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.
While at it, move the declaration to object-store.h, where it should
be easier to find.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
cache.h | 1 -
object-store.h | 2 ++
sha1_file.c | 4 ++--
streaming.c | 5 ++++-
4 files changed, 8 insertions(+), 4 deletions(-)
From: Stefan Beller <hidden> Date: 2018-02-13 01:23:28
Add a repository argument to allow the sha1_loose_object_info caller
to be more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.
As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
sha1_file.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
@@ -1151,9 +1151,10 @@ int parse_sha1_header(const char *hdr, unsigned long *sizep)returnparse_sha1_header_extended(hdr,&oi,0);}-staticintsha1_loose_object_info(constunsignedchar*sha1,-structobject_info*oi,-intflags)+#define sha1_loose_object_info(r, s, o, f) sha1_loose_object_info_##r(s, o, f)+staticintsha1_loose_object_info_the_repository(constunsignedchar*sha1,+structobject_info*oi,+intflags){intstatus=0;unsignedlongmapsize;
@@ -1266,7 +1267,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,if(!find_pack_entry(real,&e)){/* Most likely it's a loose object. */-if(!sha1_loose_object_info(real,oi,flags))+if(!sha1_loose_object_info(the_repository,real,oi,flags))return0;/* Not a loose object; someone else may have just packed it. */
@@ -1148,10 +1148,9 @@ int parse_sha1_header(const char *hdr, unsigned long *sizep)returnparse_sha1_header_extended(hdr,&oi,0);}-#define sha1_loose_object_info(r, s, o, f) sha1_loose_object_info_##r(s, o, f)-staticintsha1_loose_object_info_the_repository(constunsignedchar*sha1,-structobject_info*oi,-intflags)+staticintsha1_loose_object_info(structrepository*r,+constunsignedchar*sha1,+structobject_info*oi,intflags){intstatus=0;unsignedlongmapsize;
@@ -1175,14 +1174,14 @@ static int sha1_loose_object_info_the_repository(const unsigned char *sha1,if(!oi->typep&&!oi->typename&&!oi->sizep&&!oi->contentp){constchar*path;structstatst;-if(stat_sha1_file(the_repository,sha1,&st,&path)<0)+if(stat_sha1_file(r,sha1,&st,&path)<0)return-1;if(oi->disk_sizep)*oi->disk_sizep=st.st_size;return0;}-map=map_sha1_file(the_repository,sha1,&mapsize);+map=map_sha1_file(r,sha1,&mapsize);if(!map)return-1;
From: Stefan Beller <hidden> Date: 2018-02-13 01:23:46
Add a repository argument to allow the stat_sha1_file caller to be
more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.
As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
sha1_file.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
@@ -874,8 +874,9 @@ int git_open_cloexec(const char *name, int flags)*Notethatitmaypointtostaticstorageandisonlyvaliduntilanother*calltosha1_file_name(),etc.*/-staticintstat_sha1_file(constunsignedchar*sha1,structstat*st,-constchar**path)+#define stat_sha1_file(r, s, st, p) stat_sha1_file_##r(s, st, p)+staticintstat_sha1_file_the_repository(constunsignedchar*sha1,+structstat*st,constchar**path){structalternate_object_database*alt;
@@ -1173,7 +1174,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,if(!oi->typep&&!oi->typename&&!oi->sizep&&!oi->contentp){constchar*path;structstatst;-if(stat_sha1_file(sha1,&st,&path)<0)+if(stat_sha1_file(the_repository,sha1,&st,&path)<0)return-1;if(oi->disk_sizep)*oi->disk_sizep=st.st_size;
@@ -1372,7 +1373,7 @@ void *read_sha1_file_extended(const unsigned char *sha1,die("replacement %s not found for %s",sha1_to_hex(repl),sha1_to_hex(sha1));-if(!stat_sha1_file(repl,&st,&path))+if(!stat_sha1_file(the_repository,repl,&st,&path))die("loose object %s (stored in %s) is corrupt",sha1_to_hex(repl),path);
From: Stefan Beller <hidden> Date: 2018-02-13 01:23:48
Add a repository argument to allow the open_sha1_file caller to be
more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.
As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
sha1_file.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
From: Stefan Beller <hidden> Date: 2018-02-13 01:23:49
Add a repository argument to allow the map_sha1_file_1 caller to be
more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.
As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
sha1_file.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
From: Stefan Beller <hidden> Date: 2018-02-13 01:23:53
See previous patch for explanation.
While at it, move the declaration to object-store.h,
where it should be easier to find.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/fsck.c | 2 +-
cache.h | 1 -
object-store.h | 3 +++
packfile.c | 2 +-
sha1_file.c | 13 +++++++------
sha1_name.c | 3 ++-
6 files changed, 14 insertions(+), 10 deletions(-)
From: Stefan Beller <hidden> Date: 2018-02-13 01:23:56
Actually this also allows read_info_alternates and link_alt_odb_entry to
handle arbitrary repositories, but link_alt_odb_entries is the most
interesting function in this set of functions, hence the commit subject.
These functions span a strongly connected component in the function
graph, i.e. the recursive call chain might look like
-> link_alt_odb_entries
-> link_alt_odb_entry
-> read_info_alternates
-> link_alt_odb_entries
That is why we need to convert all these functions at the same time.
Signed-off-by: Jonathan Nieder <redacted>
Signed-off-by: Stefan Beller <redacted>
---
cache.h | 4 ++++
sha1_file.c | 36 ++++++++++++++++--------------------
2 files changed, 20 insertions(+), 20 deletions(-)
From: Stefan Beller <hidden> Date: 2018-02-13 01:24:00
From: Jonathan Nieder <redacted>
Each repository's object store can be initialized independently, so
they must not share a run_once variable.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
object-store.h | 8 +++++++-
packfile.c | 7 +++----
2 files changed, 10 insertions(+), 5 deletions(-)
From: Stefan Beller <hidden> Date: 2018-02-13 01:24:06
In a process with multiple repositories open, packfile accessors
should be associated to a single repository and not shared globally.
Move packed_git and packed_git_mru into the_repository and adjust
callers to reflect this.
Patch generated by
1. Moving the struct packed_git declaration to object-store.h
and packed_git, packed_git_mru globals to struct object_store.
2. Applying the semantic patch
contrib/coccinelle/refactoring/packed_git.cocci to adjust callers.
This semantic patch is placed in a sub directory of the coccinelle
contrib dir, as this semantic patch is not expected to be of general
usefulness; it is only useful during developing this series and
merging it with other topics in flight. At a later date, just
delete that semantic patch.
3. Applying line wrapping fixes from "make style" to break the
resulting long lines.
4. Adding missing #includes of repository.h and object-store.h
where needed.
5. As the packfiles are now owned by an objectstore/repository, which
is ephemeral unlike globals, we introduce memory leaks. So address
them in raw_object_store_clear().
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/count-objects.c | 6 ++--
builtin/fsck.c | 7 +++--
builtin/gc.c | 4 ++-
builtin/index-pack.c | 1 +
builtin/pack-objects.c | 21 ++++++++-----
builtin/pack-redundant.c | 6 ++--
builtin/receive-pack.c | 1 +
cache.h | 28 ------------------
contrib/coccinelle/refactoring/packed_git.cocci | 7 +++++
fast-import.c | 6 ++--
http-backend.c | 6 ++--
http-push.c | 1 +
http-walker.c | 1 +
http.c | 1 +
mru.h | 1 +
object-store.h | 32 +++++++++++++++++++-
object.c | 6 ++++
pack-bitmap.c | 4 ++-
pack-check.c | 1 +
pack-revindex.c | 1 +
packfile.c | 39 +++++++++++++------------
reachable.c | 1 +
server-info.c | 6 ++--
sha1_name.c | 5 ++--
24 files changed, 120 insertions(+), 72 deletions(-)
create mode 100644 contrib/coccinelle/refactoring/packed_git.cocci
@@ -154,8 +154,10 @@ Format of STDIN stream:#include"builtin.h"#include"cache.h"+#include"repository.h"#include"config.h"#include"lockfile.h"+#include"object-store.h"#include"object.h"#include"blob.h"#include"tree.h"
@@ -1110,7 +1112,7 @@ static int store_object(if(e->idx.offset){duplicate_count_by_type[type]++;return1;-}elseif(find_sha1_pack(oid.hash,packed_git)){+}elseif(find_sha1_pack(oid.hash,the_repository->objects.packed_git)){e->type=type;e->pack_id=MAX_PACK_ID;e->idx.offset=1;/* just not zero! */
@@ -1305,7 +1307,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)duplicate_count_by_type[OBJ_BLOB]++;truncate_pack(&checkpoint);-}elseif(find_sha1_pack(oid.hash,packed_git)){+}elseif(find_sha1_pack(oid.hash,the_repository->objects.packed_git)){e->type=OBJ_BLOB;e->pack_id=MAX_PACK_ID;e->idx.offset=1;/* just not zero! */
@@ -200,7 +202,7 @@ static void init_pack_info(const char *infofile, int force)objdirlen=strlen(objdir);prepare_packed_git();-for(p=packed_git;p;p=p->next){+for(p=the_repository->objects.packed_git;p;p=p->next){/* we ignore things on alternate path since they are*notavailabletothepullersingeneral.*/
On Mon, Feb 12, 2018 at 05:22:15PM -0800, Stefan Beller wrote:
This is a real take on the first part of the recent RFC[1].
...
Duy suggested that we shall not use the repository blindly, but
should carefully examine whether to pass on an object store or the
refstore or such[4], which I agree with if it makes sense. This
series unfortunately has an issue with that as I would not want to
pass down the `ignore_env` flag separately from the object store, so
I made all functions that only take the object store to have the raw
object store as the first parameter, and others using the full
repository.
Second proposal :) How about you store ignore_env in raw_object_store?
This would not be the first time an object has some configuration
passed in at construction time. And it has a "constructor" now,
raw_object_store_init() (I probably should merge _setup in it too)
The core changes look like this. I have a full commit on top of your
series [1] that keeps sha1_file.c functions take 'struct
raw_object_store' instead.
[1] https://github.com/pclouds/git/tree/object-store-part1
-- 8< --
On Tue, Feb 13, 2018 at 6:49 PM, Duy Nguyen [off-list ref] wrote:
On Mon, Feb 12, 2018 at 05:22:15PM -0800, Stefan Beller wrote:
quoted
This is a real take on the first part of the recent RFC[1].
...
Duy suggested that we shall not use the repository blindly, but
should carefully examine whether to pass on an object store or the
refstore or such[4], which I agree with if it makes sense. This
series unfortunately has an issue with that as I would not want to
pass down the `ignore_env` flag separately from the object store, so
I made all functions that only take the object store to have the raw
object store as the first parameter, and others using the full
repository.
Second proposal :) How about you store ignore_env in raw_object_store?
This would not be the first time an object has some configuration
passed in at construction time. And it has a "constructor" now,
raw_object_store_init() (I probably should merge _setup in it too)
A bit more on this configuration parameters. Down the road I think we
need something like this anyway to delete global config vars like
packed_git_window_size, delta_base_cache_limit... Either all these
end up in raw_object_store, or raw_object_store holds a link to
"struct config_set".
The ignore_env specifically though looks to me like a stop gap
solution until everything goes through repo_init() first. At that
point we don't have to delay getenv() anymore. We can getenv() all at
repo_init() then pass them in raw_object_store and ignore_env should
be gone. So sticking it inside raw_object_store _temporarily_ does not
sound so bad.
--
Duy
From: Brandon Williams <hidden> Date: 2018-02-13 16:52:57
On 02/13, Duy Nguyen wrote:
On Tue, Feb 13, 2018 at 6:49 PM, Duy Nguyen [off-list ref] wrote:
quoted
On Mon, Feb 12, 2018 at 05:22:15PM -0800, Stefan Beller wrote:
quoted
This is a real take on the first part of the recent RFC[1].
...
Duy suggested that we shall not use the repository blindly, but
should carefully examine whether to pass on an object store or the
refstore or such[4], which I agree with if it makes sense. This
series unfortunately has an issue with that as I would not want to
pass down the `ignore_env` flag separately from the object store, so
I made all functions that only take the object store to have the raw
object store as the first parameter, and others using the full
repository.
Second proposal :) How about you store ignore_env in raw_object_store?
This would not be the first time an object has some configuration
passed in at construction time. And it has a "constructor" now,
raw_object_store_init() (I probably should merge _setup in it too)
A bit more on this configuration parameters. Down the road I think we
need something like this anyway to delete global config vars like
packed_git_window_size, delta_base_cache_limit... Either all these
end up in raw_object_store, or raw_object_store holds a link to
"struct config_set".
The ignore_env specifically though looks to me like a stop gap
solution until everything goes through repo_init() first. At that
point we don't have to delay getenv() anymore. We can getenv() all at
repo_init() then pass them in raw_object_store and ignore_env should
be gone. So sticking it inside raw_object_store _temporarily_ does not
sound so bad.
I like this approach, I mean at the moment we are replicating a single
bit of data but that allows us to be able to limit the scope of where a
repository struct is passed, giving us a better abstraction layer.
From: Stefan Beller <hidden> Date: 2018-02-13 17:47:45
On Tue, Feb 13, 2018 at 4:13 AM, Duy Nguyen [off-list ref] wrote:
On Tue, Feb 13, 2018 at 6:49 PM, Duy Nguyen [off-list ref] wrote:
quoted
On Mon, Feb 12, 2018 at 05:22:15PM -0800, Stefan Beller wrote:
quoted
This is a real take on the first part of the recent RFC[1].
...
Duy suggested that we shall not use the repository blindly, but
should carefully examine whether to pass on an object store or the
refstore or such[4], which I agree with if it makes sense. This
series unfortunately has an issue with that as I would not want to
pass down the `ignore_env` flag separately from the object store, so
I made all functions that only take the object store to have the raw
object store as the first parameter, and others using the full
repository.
Second proposal :) How about you store ignore_env in raw_object_store?
This would not be the first time an object has some configuration
passed in at construction time. And it has a "constructor" now,
raw_object_store_init() (I probably should merge _setup in it too)
A bit more on this configuration parameters. Down the road I think we
need something like this anyway to delete global config vars like
packed_git_window_size, delta_base_cache_limit... Either all these
end up in raw_object_store, or raw_object_store holds a link to
"struct config_set".
That makes sense long term.
The ignore_env specifically though looks to me like a stop gap
solution until everything goes through repo_init() first. At that
point we don't have to delay getenv() anymore. We can getenv() all at
repo_init() then pass them in raw_object_store and ignore_env should
be gone. So sticking it inside raw_object_store _temporarily_ does not
sound so bad.
Oh, that is an interesting perspective. Here is how I arrived at the opposite
conclusion initially: Searching for 'ignore_env' shows that we care about it
as well for the index and graft paths, which are not the object store, hence
it would be better kept in the repository. (The alternative would be to
duplicate it into the raw object store, but I do not like data duplication)
But maybe it is better to duplicate this one bit instead of passing through
a larger scoped object.
I'll rework the patches.
Thanks!
Stefan
From: Brandon Williams <hidden> Date: 2018-02-13 18:51:29
On 02/12, Stefan Beller wrote:
quoted hunk
In a process with multiple repositories open, alternates should be
associated to a single repository and not shared globally. Move
alt_odb_list and alt_odb_tail into the_repository and adjust callers
to reflect this.
Now that the alternative object data base is per repository, we're
leaking its memory upon freeing a repository. The next patch plugs
this hole.
No functional change intended.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/fsck.c | 4 +++-
cache.h | 4 ++--
object-store.h | 7 ++++++-
packfile.c | 3 ++-
sha1_file.c | 25 ++++++++++++-------------
sha1_name.c | 3 ++-
6 files changed, 27 insertions(+), 19 deletions(-)
@@ -1573,7 +1573,7 @@ extern int has_dirs_only_path(const char *name, int len, int prefix_len);externvoidschedule_dir_for_removal(constchar*name,intlen);externvoidremove_scheduled_dirs(void);-externstructalternate_object_database{+structalternate_object_database{structalternate_object_database*next;/* see alt_scratch_buf() */
@@ -563,7 +561,7 @@ void add_to_alternates_file(const char *reference)fprintf_or_die(out,"%s\n",reference);if(commit_lock_file(&lock))die_errno("unable to move new alternates file into place");-if(alt_odb_tail)+if(the_repository->objects.alt_odb_tail)link_alt_odb_entries(reference,'\n',NULL,0);}free(alts);
@@ -661,7 +659,7 @@ int foreach_alt_odb(alt_odb_fn fn, void *cb)intr=0;prepare_alt_odb();-for(ent=alt_odb_list;ent;ent=ent->next){+for(ent=the_repository->objects.alt_odb_list;ent;ent=ent->next){r=fn(ent,cb);if(r)break;
From: Jonathan Nieder <hidden> Date: 2018-02-13 19:26:31
Hi,
Stefan Beller wrote:
This is a real take on the first part of the recent RFC[1].
Jonathan Tan suggested[2] that "sha1_loose_object_info to handle arbitrary repositories"
might be a good breaking point for a first part at that RFC at patch 38.
This series is smaller and contains only 26 patches as the patches in the big
RFC were slightly out of order.
Thanks. This looks like a nice reviewable series, so I'm happy to see
it broken out.
[...]
Comments in the early range of that RFC were on 003 where Junio pointed out
that the coccinelle patch ought to be not in contrib/coccinelle, so I put it
in a sub directory there, as 'make coccicheck' doesn't traverse subdirs.
Can you say a little more about this? Was the problem that the
semantic patch wasn't idempotent, that it was too slow to run, or
something else?
If we're including the semantic patch for reference but never running
it, I think I'd prefer it to go in the commit message. But if it's
useful to run then we should make it idempotent so it can go in
contrib/coccinelle.
[...]
Duy suggested that we shall not use the repository blindly, but should carefully
examine whether to pass on an object store or the refstore or such[4], which
I agree with if it makes sense. This series unfortunately has an issue with that
as I would not want to pass down the `ignore_env` flag separately from the object
store, so I made all functions that only take the object store to have the raw
object store as the first parameter, and others using the full repository.
I think I want to push back on this a little.
The advantage of a function taking e.g. an object_store as an argument
instead of a repository is that it increases its flexibility, since it
allows callers that do not have access to a repository to call it. The
disadvantage is also that it increases the flexibility without any
callers benefitting from that:
1. It ties us to assumptions from today. If e.g. an object access in
the future starts relying on some other information from the
repository (e.g. its config) then we'd have to either add a
back-pointer from the object store to its repository or add
additional arguments for that additional data at that point.
If all callers already have a repository, it is simpler to pass
that repository as context so that we have the flexibility to make
more use of it later.
2. It complicates the caller. Instead of consistently passing the
same repository argument as context to functions that access that
repository, the caller would have to pull out relevant fields like
the object store from it.
3. It prevents us from making opportunistic use of other information
from the repository, such as its name for use in error messages.
In lower-level funcitons that need to be usable by callers without a
repository (e.g. to find packfiles in an alternate) it makes sense to
not pass a repository, but without such a use case in mind I don't
think it needs to be a general goal.
To put it another way, most callers do not *care* whether they are
working with a repository's object store, ref database, or some other
aspect of the repository. They just know they want to e.g. read an
object from this repository.
It's similar to how FILE * works: some operations rely on the buffer
the FILE * manages and some other operations only rely on the
underlying file descriptor, but using the FILE * consistently provides
a clean abstraction that generally makes life easier.
Eric Sunshine brought up memory leaks with the RFC, and I would think to
have plugged all holes.
Yay, thank you!
I'll try to find time to look at the patches in detail soon, but no
promises (i.e. if someone else reviews them first, then even better
;-)).
Sincerely,
Jonathan
From: Brandon Williams <hidden> Date: 2018-02-13 19:33:47
On 02/12, Stefan Beller wrote:
This is a real take on the first part of the recent RFC[1].
Jonathan Tan suggested[2] that "sha1_loose_object_info to handle arbitrary repositories"
might be a good breaking point for a first part at that RFC at patch 38.
This series is smaller and contains only 26 patches as the patches in the big
RFC were slightly out of order.
I developed this series partly by writing patches, but mostly by cherrypicking
from that RFC on top of current master. I noticed no external conflicts apart
from one addition to the repositories _INIT macro, which was easy to resolve.
Comments in the early range of that RFC were on 003 where Junio pointed out
that the coccinelle patch ought to be not in contrib/coccinelle, so I put it
in a sub directory there, as 'make coccicheck' doesn't traverse subdirs.
brian had a questoin on patch 25 in the RFC, but that seemed to resolve itself
without any suggestion to include into this series[3].
Duy suggested that we shall not use the repository blindly, but should carefully
examine whether to pass on an object store or the refstore or such[4], which
I agree with if it makes sense. This series unfortunately has an issue with that
as I would not want to pass down the `ignore_env` flag separately from the object
store, so I made all functions that only take the object store to have the raw
object store as the first parameter, and others using the full repository.
Eric Sunshine brought up memory leaks with the RFC, and I would think to
have plugged all holes.
I've looked through the patches and I think they look good. At the end
of the series all the #define tricks have been eliminated so we don't
have to worry about them possibly being left and forgotten :)
Thanks for getting the ball rolling on this.
[1] https://public-inbox.org/git/20180205235508.216277-1-sbeller@google.com/
[2] https://public-inbox.org/git/20180207143300.ce1c39ca07f6a0d64fe0e7ca@google.com/
[3] https://public-inbox.org/git/20180206011940.GD7904@genre.crustytoothpaste.net/
[4] https://public-inbox.org/git/CACsJy8CGgekpX4cZkyyTSPrj87uQVKZSOL7fyT__P2dh_1LmVQ@mail.gmail.com/
Thanks,
Stefan
Jonathan Nieder (8):
pack: move prepare_packed_git_run_once to object store
pack: move approximate object count to object store
sha1_file: add repository argument to sha1_file_name
sha1_file: add repository argument to map_sha1_file
sha1_file: allow stat_sha1_file to handle arbitrary repositories
sha1_file: allow open_sha1_file to handle arbitrary repositories
sha1_file: allow map_sha1_file_1 to handle arbitrary repositories
sha1_file: allow sha1_loose_object_info to handle arbitrary
repositories
Stefan Beller (18):
repository: introduce raw object store field
object-store: move alt_odb_list and alt_odb_tail to object store
object-store: free alt_odb_list
object-store: move packed_git and packed_git_mru to object store
object-store: close all packs upon clearing the object store
sha1_file: add raw_object_store argument to alt_odb_usable
sha1_file: add repository argument to link_alt_odb_entry
sha1_file: add repository argument to read_info_alternates
sha1_file: add repository argument to link_alt_odb_entries
sha1_file: add repository argument to prepare_alt_odb
sha1_file: allow link_alt_odb_entries to handle arbitrary repositories
sha1_file: allow prepare_alt_odb to handle arbitrary repositories
sha1_file: add repository argument to stat_sha1_file
sha1_file: add repository argument to open_sha1_file
sha1_file: add repository argument to map_sha1_file_1
sha1_file: add repository argument to sha1_loose_object_info
sha1_file: allow sha1_file_name to handle arbitrary repositories
sha1_file: allow map_sha1_file to handle arbitrary repositories
builtin/am.c | 2 +-
builtin/clone.c | 2 +-
builtin/count-objects.c | 6 +-
builtin/fetch.c | 2 +-
builtin/fsck.c | 13 ++-
builtin/gc.c | 4 +-
builtin/grep.c | 2 +-
builtin/index-pack.c | 1 +
builtin/merge.c | 2 +-
builtin/pack-objects.c | 21 ++--
builtin/pack-redundant.c | 6 +-
builtin/receive-pack.c | 3 +-
cache.h | 46 ++------
contrib/coccinelle/refactoring/packed_git.cocci | 7 ++
environment.c | 5 +-
fast-import.c | 6 +-
http-backend.c | 6 +-
http-push.c | 1 +
http-walker.c | 4 +-
http.c | 6 +-
mru.h | 1 +
object-store.h | 75 +++++++++++++
object.c | 26 +++++
pack-bitmap.c | 4 +-
pack-check.c | 1 +
pack-revindex.c | 1 +
packfile.c | 64 +++++------
packfile.h | 2 +-
path.c | 2 +-
reachable.c | 1 +
repository.c | 17 ++-
repository.h | 7 +-
server-info.c | 6 +-
sha1_file.c | 135 +++++++++++++-----------
sha1_name.c | 11 +-
streaming.c | 5 +-
36 files changed, 321 insertions(+), 182 deletions(-)
create mode 100644 contrib/coccinelle/refactoring/packed_git.cocci
create mode 100644 object-store.h
--
2.16.1.73.ga2c3e9663f.dirty
@@ -677,21 +677,22 @@ int foreach_alt_odb(alt_odb_fn fn, void *cb)returnr;}-voidprepare_alt_odb_the_repository(void)+voidprepare_alt_odb(structrepository*r){-constchar*alt;--if(the_repository->objects.alt_odb_tail)+if(r->objects.alt_odb_tail)return;-alt=getenv(ALTERNATE_DB_ENVIRONMENT);+r->objects.alt_odb_tail=&r->objects.alt_odb_list;++if(!r->ignore_env){+constchar*alt=getenv(ALTERNATE_DB_ENVIRONMENT);
If one day the majority of git moves to use 'struct repository', then
ALTERNATE_DB_ENVIRONMENT is always ignored because ignore_env is
always true. I think if you ignore_env, then you still need to get
this "alt" from 'struct raw_object_store' (or 'struct repository').
Since you get lots of getenv() in repo_setup_env(), I think this
getenv(ALTERNATE_DB_ENVIRONMENT) belongs there too. Then here, if
ignore_env is true, you read r->objects.alt or something instead of
doing getenv().
I really want to kill this getenv() in this code, which is basically
delayed initialization because we haven't done proper init on
the_repo. I realize that it cannot be done earlier, when
prepare_alt_odb() does not even have a 'struct repository *' to work
with. Would it be ok if I contributed a patch on top of your series to
basically do repo_init(&the_repo) for all builtin/external commands
(and fix all the bugs that come with it)? Then we would not need
ignore_env here anymore.
+ if (!alt)
+ alt = "";
- the_repository->objects.alt_odb_tail =
- &the_repository->objects.alt_odb_list;
- link_alt_odb_entries(the_repository, alt,
- PATH_SEP, NULL, 0);
+ link_alt_odb_entries(r, alt, PATH_SEP, NULL, 0);
+ }
- read_info_alternates(the_repository, get_object_directory(), 0);
+ read_info_alternates(r, r->objects.objectdir, 0);
}
/* Returns 1 if we have successfully freshened the file, 0 otherwise. */
--
2.16.1.73.ga2c3e9663f.dirty
On Wed, Feb 14, 2018 at 2:26 AM, Jonathan Nieder [off-list ref] wrote:
quoted
Duy suggested that we shall not use the repository blindly, but should carefully
examine whether to pass on an object store or the refstore or such[4], which
I agree with if it makes sense. This series unfortunately has an issue with that
as I would not want to pass down the `ignore_env` flag separately from the object
store, so I made all functions that only take the object store to have the raw
object store as the first parameter, and others using the full repository.
I think I want to push back on this a little.
The advantage of a function taking e.g. an object_store as an argument
instead of a repository is that it increases its flexibility, since it
allows callers that do not have access to a repository to call it. The
disadvantage is also that it increases the flexibility without any
callers benefitting from that:
1. It ties us to assumptions from today. If e.g. an object access in
the future starts relying on some other information from the
repository (e.g. its config) then we'd have to either add a
back-pointer from the object store to its repository or add
additional arguments for that additional data at that point.
If all callers already have a repository, it is simpler to pass
that repository as context so that we have the flexibility to make
more use of it later.
It's essentially putting all global variables in the same place again.
Only this time it's not global namespace, but "struct repository".
It's no worse than the current state though.
2. It complicates the caller. Instead of consistently passing the
same repository argument as context to functions that access that
repository, the caller would have to pull out relevant fields like
the object store from it.
Well, I see that as a good point :)
3. It prevents us from making opportunistic use of other information
from the repository, such as its name for use in error messages.
It does not exactly prevent us. It's just more effort to pass this
around. The repository name for example, there's no reason we can't
have object store name (which could be initialized the same as repo
name).
In lower-level funcitons that need to be usable by callers without a
repository (e.g. to find packfiles in an alternate) it makes sense to
not pass a repository, but without such a use case in mind
I do agree with your benefit argument. But I'd like to point out
though that having all input to object store visible from something
like "struct raw_object_store" makes it easier to reason about the
code. I know how object store works, but occasionally I'm still
surprised when it getenv (or read $GIT_DIR/index, but not in object
store code) behind the scene. Imagine how hard it is for newcomers.
I would count that as benefit, even though it's not a use case per se.
Another potential benefit is writing unit tests will be much easier
(you can configure object store through struct repository too, but
setting one piece here, one piece there to control object store
behavior is not a nice experience). It's a nice thing to have, but not
a deciding factor.
I don't think it needs to be a general goal.
My stand is a bit more aggressive. We should try to achieve better
abstraction if possible. But if it makes Stefan's life hell, it's not
worth doing. Converting to 'struct repository' is already a step
forward. Actually if it discourages him from finishing this work, it's
already not worth doing.
--
Duy
@@ -677,21 +677,22 @@ int foreach_alt_odb(alt_odb_fn fn, void *cb)returnr;}-voidprepare_alt_odb_the_repository(void)+voidprepare_alt_odb(structrepository*r){-constchar*alt;--if(the_repository->objects.alt_odb_tail)+if(r->objects.alt_odb_tail)return;-alt=getenv(ALTERNATE_DB_ENVIRONMENT);+r->objects.alt_odb_tail=&r->objects.alt_odb_list;++if(!r->ignore_env){+constchar*alt=getenv(ALTERNATE_DB_ENVIRONMENT);
If one day the majority of git moves to use 'struct repository', then
ALTERNATE_DB_ENVIRONMENT is always ignored because ignore_env is
always true. I think if you ignore_env, then you still need to get
this "alt" from 'struct raw_object_store' (or 'struct repository').
Since you get lots of getenv() in repo_setup_env(), I think this
getenv(ALTERNATE_DB_ENVIRONMENT) belongs there too. Then here, if
ignore_env is true, you read r->objects.alt or something instead of
doing getenv().
I really want to kill this getenv() in this code, which is basically
delayed initialization because we haven't done proper init on
the_repo. I realize that it cannot be done earlier, when
prepare_alt_odb() does not even have a 'struct repository *' to work
with. Would it be ok if I contributed a patch on top of your series to
basically do repo_init(&the_repo) for all builtin/external commands
(and fix all the bugs that come with it)? Then we would not need
ignore_env here anymore.
At some point yes we would definitely want the setup code to fully
initialize a repository object (in this case the_repository). And I
would even like to change the function signatures of all the builtin
commands to take a repository object so that they don't implicitly rely
on the_repository at all.
When I introduced struct repository I seem to remember there being a
couple things which were different about setup that made it difficult to
simply call repo_init() on the_repository. If you can fix whatever
those issues with setup were (I can't remember all of them) then that
would be great :)
quoted
+ if (!alt)
+ alt = "";
- the_repository->objects.alt_odb_tail =
- &the_repository->objects.alt_odb_list;
- link_alt_odb_entries(the_repository, alt,
- PATH_SEP, NULL, 0);
+ link_alt_odb_entries(r, alt, PATH_SEP, NULL, 0);
+ }
- read_info_alternates(the_repository, get_object_directory(), 0);
+ read_info_alternates(r, r->objects.objectdir, 0);
}
/* Returns 1 if we have successfully freshened the file, 0 otherwise. */
--
2.16.1.73.ga2c3e9663f.dirty
On Thu, Feb 15, 2018 at 1:08 AM, Brandon Williams [off-list ref] wrote:
At some point yes we would definitely want the setup code to fully
initialize a repository object (in this case the_repository). And I
would even like to change the function signatures of all the builtin
commands to take a repository object so that they don't implicitly rely
on the_repository at all.
When I introduced struct repository I seem to remember there being a
couple things which were different about setup that made it difficult to
simply call repo_init() on the_repository. If you can fix whatever
those issues with setup were (I can't remember all of them) then that
would be great :)
I can certainly try! I start to remember all the hairy details in that
setup code.
The first step may be something like this, which identifies all the
"repo init" entry points. This is basically a revert of e26f7f19b6
(repository: pre-initialize hash algo pointer - 2018-01-19) and doing
things the proper way, hopefully.
This is on 'master', independent from Stefan's series. I have another
patch on top of that series to remove the use of ignore_env in
sha1_file.c (and things seem to work). Basically whenever you have to
initialize the hash algorithm, there's a good chance you need to
initialize object store as well. But I'll hold that off until
Stefan's and this one are both merged.
But yeah, it looks like we need some surgery in setup.c if we want
something as pretty as repo_submodule_init() but for the main repo.
Nguyễn Thái Ngọc Duy (2):
setup.c: initialize the_repository correctly in all cases
Revert "repository: pre-initialize hash algo pointer"
builtin/index-pack.c | 5 +++++
builtin/init-db.c | 3 ++-
cache.h | 3 ++-
common-main.c | 4 ++++
diff-no-index.c | 5 +++++
path.c | 2 +-
repository.c | 2 +-
setup.c | 5 ++++-
t/helper/test-dump-split-index.c | 2 ++
9 files changed, 26 insertions(+), 5 deletions(-)
--
2.16.1.435.g8f24da2e1a
There are many ways for any command to access a git repository:
- most of them will try to discover the .git dir via
setup_git_directory() and friends
- the server side programs already know where the repo is and prepare
with enter_repo()
- special commands that deal with repo creation (init/clone) use
init_db() once the new repo is ready for access.
- somebody accesses $GIT_DIR before any of above functions are called
and accidentally sets up a git repository by set_git_dir() alone
"the_repository" is partially set up via set_git_dir() at some point
in all four cases. The hash algorithm though is configured later after
.git/config is read.
So far proper repo initialization is done only for the first case [1].
The second case is not covered (but that's fine [3]). The third case
was found and worked around in [2]. The fourth case is a buggy one,
which should be fixed already by jk/no-looking-at-dotgit-outside-repo
and never happens again.
This patch makes sure all cases initialize the hash algorithm in
the_repository correctly. Both second and third cases must run
check_repo_format() before "entering" it. Eventually we probably just
rename this function to init_repo() or something.
[1] 78a6766802 (Integrate hash algorithm support with repo setup -
2017-11-12)
[2] e26f7f19b6 (repository: pre-initialize hash algo pointer -
2018-01-19)
[3] the reason server side is still running ok with no hash algo
before [2] is because the programs that use enter_repo() do very
little then spawn a new program (like pack-objects or
upload-archive) to do the heavy lifting. These programs already
use setup_git_dir..()
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
builtin/init-db.c | 3 ++-
cache.h | 3 ++-
path.c | 2 +-
setup.c | 5 ++++-
4 files changed, 9 insertions(+), 4 deletions(-)
@@ -894,6 +894,7 @@ extern int repository_format_precious_objects;externchar*repository_format_partial_clone;externconstchar*core_partial_clone_filter_default;+structrepository;structrepository_format{intversion;intprecious_objects;
@@ -926,7 +927,7 @@ int verify_repository_format(const struct repository_format *format,*set_git_dir()beforecallingthis,anduseitonlyfor"are we in a valid*repo?".*/-externvoidcheck_repository_format(void);+externvoidcheck_repository_format(structrepository*);#define MTIME_CHANGED 0x0001#define CTIME_CHANGED 0x0002
@@ -1180,11 +1180,14 @@ int git_config_perm(const char *var, const char *value)return-(i&0666);}-voidcheck_repository_format(void)+/* optionally configure "repo" to the correct format */+voidcheck_repository_format(structrepository*repo){structrepository_formatrepo_fmt;check_repository_format_gently(get_git_dir(),&repo_fmt,NULL);startup_info->have_repository=1;+if(repo)+repo_set_hash_algo(repo,repo_fmt.hash_algo);}/*
This reverts commit e26f7f19b6c7485f04234946a59ab8f4fd21d6d1. The root
problem, git clone not setting up the_hash_algo, has been fixed in the
previous patch.
As a result of the revert, some code paths that use the_hash_algo
without initialization is revealed and fixed here. It's basically
commands that are allowed to run without a repository. The fix here is
not the best. We probably could figure out the hash algorithm from input
somehow.
Since this is a dangerous move and could potentially break stuff after
release (and leads to workaround like the reverted commit), the
workaround technically remains, but is hidden behind a new environment
variable GIT_HASH_FIXUP. This should let the users continue to use git
while we fix the problem.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
builtin/index-pack.c | 5 +++++
common-main.c | 4 ++++
diff-no-index.c | 5 +++++
repository.c | 2 +-
t/helper/test-dump-split-index.c | 2 ++
5 files changed, 17 insertions(+), 1 deletion(-)
@@ -1673,6 +1673,11 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)if(prefix&&chdir(prefix))die(_("Cannot come back to cwd"));+if(!the_hash_algo){+warning(_("Running without a repository, assuming SHA-1 hash"));+repo_set_hash_algo(the_repository,GIT_HASH_SHA1);+}+for(i=1;i<argc;i++){constchar*arg=argv[i];
@@ -5,7 +5,7 @@/* The main repository */staticstructrepositorythe_repo={-NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,&hash_algos[GIT_HASH_SHA1],0,0+NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,NULL,0,0};structrepository*the_repository=&the_repo;
From: Stefan Beller <hidden> Date: 2018-02-23 18:34:05
On Fri, Feb 23, 2018 at 1:56 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:
quoted hunk
This reverts commit e26f7f19b6c7485f04234946a59ab8f4fd21d6d1. The root
problem, git clone not setting up the_hash_algo, has been fixed in the
previous patch.
As a result of the revert, some code paths that use the_hash_algo
without initialization is revealed and fixed here. It's basically
commands that are allowed to run without a repository. The fix here is
not the best. We probably could figure out the hash algorithm from input
somehow.
Since this is a dangerous move and could potentially break stuff after
release (and leads to workaround like the reverted commit), the
workaround technically remains, but is hidden behind a new environment
variable GIT_HASH_FIXUP. This should let the users continue to use git
while we fix the problem.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
builtin/index-pack.c | 5 +++++
common-main.c | 4 ++++
diff-no-index.c | 5 +++++
repository.c | 2 +-
t/helper/test-dump-split-index.c | 2 ++
5 files changed, 17 insertions(+), 1 deletion(-)
@@ -1673,6 +1673,11 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)if(prefix&&chdir(prefix))die(_("Cannot come back to cwd"));+if(!the_hash_algo){+warning(_("Running without a repository, assuming SHA-1 hash"));+repo_set_hash_algo(the_repository,GIT_HASH_SHA1);+}+for(i=1;i<argc;i++){constchar*arg=argv[i];
@@ -5,7 +5,7 @@/* The main repository */staticstructrepositorythe_repo={-NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,&hash_algos[GIT_HASH_SHA1],0,0+NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,NULL,0,0
I am not sure I agree with this defense in depth, because it would add
a lot to maintenance burden.
Specifically this part. The series that I sent out usually clashes
here as this is currently a hot area
of the code touched by many different series in flight.
However this is the long term correct thing to do? We assume no algorithm until
the repository can tell us from its config (or we default to sha1 if there is no
configuration present).
I wonder if there is yet another missing case in the enumeration of
the previous patch:
Some commands are able to operate on GIT_OBJECT_DIR instead
of GIT_DIR (git repack?), which may not even explore the full git directory,
and so doesn't know about the hash value.
In the cover letter you reference my series, but comparing the diffstats
(and looking through the patches), I would only expect this one place
to have merge conflicts, which ought to be easy to resolve.
(In my series I break the initializer into multiple lines to help the
future, too)
After some thought, I like this series.
Thanks,
Stefan
From: brian m. carlson <hidden> Date: 2018-02-23 22:18:00
On Fri, Feb 23, 2018 at 04:56:39PM +0700, Nguyễn Thái Ngọc Duy wrote:
[3] the reason server side is still running ok with no hash algo
before [2] is because the programs that use enter_repo() do very
little then spawn a new program (like pack-objects or
upload-archive) to do the heavy lifting. These programs already
use setup_git_dir..()
@@ -1673,6 +1673,11 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)if(prefix&&chdir(prefix))die(_("Cannot come back to cwd"));+if(!the_hash_algo){+warning(_("Running without a repository, assuming SHA-1 hash"));+repo_set_hash_algo(the_repository,GIT_HASH_SHA1);+}
Is this warning going to be visible to users in the normal course of
operation? If so, people are probably going to find this bothersome or
alarming.
quoted hunk
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
@@ -40,5 +41,8 @@ int main(int argc, const char **argv)restore_sigpipe_to_default();+if(getenv("GIT_HASH_FIXUP"))+repo_set_hash_algo(the_repository,GIT_HASH_SHA1);
I'm lukewarm on adding this environment variable, but considering our
history here, we had probably better. We can always remove it after a
few releases.
@@ -241,6 +241,11 @@ void diff_no_index(struct rev_info *revs,structstrbufreplacement=STRBUF_INIT;constchar*prefix=revs->prefix;+if(!the_hash_algo){+warning(_("Running without a repository, assuming SHA-1 hash"));+repo_set_hash_algo(the_repository,GIT_HASH_SHA1);+}
Again, same concern. I can imagine scripts that will blow up loudly if
git diff --no-index spews things to standard error.
I'm not opposed to making this more visible, but I wonder if maybe it
should only be visible to developers or such. The only way I can think
of doing is that is with an advice options, but maybe there's a better
way.
--
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
From: brian m. carlson <hidden> Date: 2018-02-23 22:48:02
On Fri, Feb 23, 2018 at 04:56:38PM +0700, Nguyễn Thái Ngọc Duy wrote:
I can certainly try! I start to remember all the hairy details in that
setup code.
The first step may be something like this, which identifies all the
"repo init" entry points. This is basically a revert of e26f7f19b6
(repository: pre-initialize hash algo pointer - 2018-01-19) and doing
things the proper way, hopefully.
This is on 'master', independent from Stefan's series. I have another
patch on top of that series to remove the use of ignore_env in
sha1_file.c (and things seem to work). Basically whenever you have to
initialize the hash algorithm, there's a good chance you need to
initialize object store as well. But I'll hold that off until
Stefan's and this one are both merged.
I definitely think this series is an improvement over my previous patch.
My major concern is alarming users (or breaking scripts) with the
warning message. I wonder if deferring the use of the warning until we
have multiple hash algorithms might be a better idea. At that point,
the warning would become something people could act upon.
--
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
@@ -1673,6 +1673,11 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)if(prefix&&chdir(prefix))die(_("Cannot come back to cwd"));+if(!the_hash_algo){+warning(_("Running without a repository, assuming SHA-1 hash"));+repo_set_hash_algo(the_repository,GIT_HASH_SHA1);+}
Is this warning going to be visible to users in the normal course of
operation? If so, people are probably going to find this bothersome or
alarming.
quoted
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
@@ -40,5 +41,8 @@ int main(int argc, const char **argv)restore_sigpipe_to_default();+if(getenv("GIT_HASH_FIXUP"))+repo_set_hash_algo(the_repository,GIT_HASH_SHA1);
I'm lukewarm on adding this environment variable, but considering our
history here, we had probably better. We can always remove it after a
few releases.
Yeah I don't know what the best thing to do here would be. I mean we
could always just init the hash_algo for the_repository here so that we
don't ever have to worry about it, but I don't know if even that is the
right approach.
@@ -241,6 +241,11 @@ void diff_no_index(struct rev_info *revs,structstrbufreplacement=STRBUF_INIT;constchar*prefix=revs->prefix;+if(!the_hash_algo){+warning(_("Running without a repository, assuming SHA-1 hash"));+repo_set_hash_algo(the_repository,GIT_HASH_SHA1);+}
Again, same concern. I can imagine scripts that will blow up loudly if
git diff --no-index spews things to standard error.
I'm not opposed to making this more visible, but I wonder if maybe it
should only be visible to developers or such. The only way I can think
of doing is that is with an advice options, but maybe there's a better
way.
--
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
On Sat, Feb 24, 2018 at 5:17 AM, brian m. carlson
[off-list ref] wrote:
On Fri, Feb 23, 2018 at 04:56:39PM +0700, Nguyễn Thái Ngọc Duy wrote:
quoted
[3] the reason server side is still running ok with no hash algo
before [2] is because the programs that use enter_repo() do very
little then spawn a new program (like pack-objects or
upload-archive) to do the heavy lifting. These programs already
use setup_git_dir..()
You have "..()" here. Did you want to say "()." instead?
It's because we have two functions, setup_git_directory() and
setup_git_directory_gently() and I tend to just put '*' instead of
listing both. But since it causes confusion, I'm listing both in v2.
--
Duy
Brian questioned the unnecessary alarms in v1 when "git diff" or "git
index-pack" run in no-repository mode. I had the same feeling after
sending v1. But instead of suppresing alarms, we could do better.
v2 breaks those "fall back to SHA-1" code into separate patches and
handles it properly (I hope) instead of blind fall back like v1:
- for index-pack, we can determine the needed algorithm from the pack
file. I'm making an assumption here that pack files with new hash
algo must step up file format version. But I think it's a reasonable
assumption.
- for diff --no-index, I still fall back to SHA-1 to generate the
hashes like before. We could probably introduce a new command line
option to use a different hash. But that work could be done later
when an actual new hash has come
Note, I didn't test but this series could potentially break 'pu' (in a
good way). I initially was puzzled why the test suite didn't fail when
the_repository->hash_algo was NULL (i.e. before Brian's fix). Then I
found out that more work to abstract away SHA-1 hashing functions have
been done since then. But because the_hash_algo is pre-initialized
with SHA-1, these special cases did not show up until now. If there
are more the_hash_algo conversion on 'pu', more "no-repo" cases
could be spotted by the test suite.
I think this makes pre-initializing the_hash_algo to NULL a very good
point: during the abstraction work, we at least must identify the use
cases like this (code running without repo). We don't have to fix it
right away, but we can start thinking about how to deal with it. If we
ignore it and switch to a new hash, these code will keep using SHA-1
and cause more headache in future.
Nguyễn Thái Ngọc Duy (5):
setup.c: initialize the_repository correctly in all cases
sha1_file.c: keep a good name for "unknown" hash_algos[UNKNOWN]
index-pack: check (and optionally set) hash algo based on input file
diff.c: initialize hash algo when running in --no-index mode
Revert "repository: pre-initialize hash algo pointer"
builtin/index-pack.c | 26 +++++++++++++++++++++++++-
builtin/init-db.c | 3 ++-
cache.h | 3 ++-
common-main.c | 10 ++++++++++
diff.c | 12 ++++++++++++
path.c | 2 +-
repository.c | 2 +-
setup.c | 5 ++++-
sha1_file.c | 2 +-
t/helper/test-dump-split-index.c | 2 ++
10 files changed, 60 insertions(+), 7 deletions(-)
--
2.16.1.435.g8f24da2e1a
There are many ways for any command to access a git repository:
- most of them will try to discover the .git dir via
setup_git_directory() and friends
- the server side programs already know where the repo is and prepare
with enter_repo()
- special commands that deal with repo creation (init/clone) use
init_db() once the new repo is ready for access.
- somebody accesses $GIT_DIR before any of above functions are called
and accidentally sets up a git repository by set_git_dir() alone
"the_repository" is partially set up via set_git_dir() at some point
in all four cases. The hash algorithm though is configured later after
.git/config is read.
So far proper repo initialization is done only for the first case [1].
The second case is not covered (but that's fine [3]). The third case
was found and worked around in [2]. The fourth case is a buggy one,
which should be fixed already by jk/no-looking-at-dotgit-outside-repo
and never happens again.
This patch makes sure all cases initialize the hash algorithm in
the_repository correctly. Both second and third cases must run
check_repo_format() before "entering" it. Eventually we probably just
rename this function to init_repo() or something.
[1] 78a6766802 (Integrate hash algorithm support with repo setup -
2017-11-12)
[2] e26f7f19b6 (repository: pre-initialize hash algo pointer -
2018-01-19)
[3] the reason server side is still running ok with no hash algo before
[2] is because the programs that use enter_repo() do very
little (and unlikely to hash anything) then spawn a new
program (like pack-objects or upload-archive) to do the heavy
lifting. These programs already use setup_git_directory() or the
gently version
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
builtin/init-db.c | 3 ++-
cache.h | 3 ++-
path.c | 2 +-
setup.c | 5 ++++-
4 files changed, 9 insertions(+), 4 deletions(-)
@@ -894,6 +894,7 @@ extern int repository_format_precious_objects;externchar*repository_format_partial_clone;externconstchar*core_partial_clone_filter_default;+structrepository;structrepository_format{intversion;intprecious_objects;
@@ -926,7 +927,7 @@ int verify_repository_format(const struct repository_format *format,*set_git_dir()beforecallingthis,anduseitonlyfor"are we in a valid*repo?".*/-externvoidcheck_repository_format(void);+externvoidcheck_repository_format(structrepository*);#define MTIME_CHANGED 0x0001#define CTIME_CHANGED 0x0002
@@ -1180,11 +1180,14 @@ int git_config_perm(const char *var, const char *value)return-(i&0666);}-voidcheck_repository_format(void)+/* optionally configure "repo" to the correct format */+voidcheck_repository_format(structrepository*repo){structrepository_formatrepo_fmt;check_repository_format_gently(get_git_dir(),&repo_fmt,NULL);startup_info->have_repository=1;+if(repo)+repo_set_hash_algo(repo,repo_fmt.hash_algo);}/*
This is mostly for displaying the hash algorithm name when we report
errors. Printing "unknown" with '%s' is much better than '(null)' in
glibc printf version (and probably could crash if other implementations
do not check for NULL pointer)
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
sha1_file.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
After 454253f059 (builtin/index-pack: improve hash function abstraction
- 2018-02-01), index-pack uses the_hash_algo for hashing. If "git
index-pack" is executed without a repository, we do not know what hash
algorithm to be used and the_hash_algo in theory could be undefined.
Since there should be some information about the hash algorithm in the
input pack file, we can initialize the correct hash algorithm with that
if the_hash_algo is not yet initialized. This assumes that pack files
with new hash algorithm MUST step up pack version.
While at there, make sure the hash algorithm requested by the pack file
and configured by the repository (if we're running with a repo) are
consistent.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
builtin/index-pack.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
@@ -326,10 +326,31 @@ static const char *open_pack_file(const char *pack_name)output_fd=-1;nothread_data.pack_fd=input_fd;}-the_hash_algo->init_fn(&input_ctx);returnpack_name;}+staticvoidprepare_hash_algo(uint32_tpack_version)+{+conststructgit_hash_algo*pack_algo;++switch(pack_version){+case2:+case3:+pack_algo=&hash_algos[GIT_HASH_SHA1];+break;+default:+die("BUG: how to determine hash algo for new version?");+}++if(!the_hash_algo)/* running without repo */+the_hash_algo=pack_algo;++if(the_hash_algo!=pack_algo)+die(_("incompatible hash algorithm, "+"configured for %s but the pack file needs %s"),+the_hash_algo->name,pack_algo->name);+}+staticvoidparse_pack_header(void){structpack_header*hdr=fill(sizeof(structpack_header));
@@ -341,6 +362,9 @@ static void parse_pack_header(void)die(_("pack version %"PRIu32" unsupported"),ntohl(hdr->hdr_version));+prepare_hash_algo(ntohl(hdr->hdr_version));+the_hash_algo->init_fn(&input_ctx);+nr_objects=ntohl(hdr->hdr_entries);use(sizeof(structpack_header));}
Our "git diff" command supports running as a standalone tool. In this
code path, we try to hash the file content but after
18e2588e11 (sha1_file: switch uses of SHA-1 to the_hash_algo -
2018-02-01), there is a chance that the_hash_algo (required by
index_path) may still be uninitialized if no repository is found.
Executing index_path() when the_hash_algo is NULL (or points to unknown
algo) either crashes or dies. Let's make it a bit safer by explicitly
falling back to SHA-1 (so that the diff output remains the same as
before, compared to the alternative that we simply do not hash).
dòng được
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
diff.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
This reverts commit e26f7f19b6c7485f04234946a59ab8f4fd21d6d1. The root
problem, git clone not setting up the_hash_algo, has been fixed in the
previous patch.
Since this is a dangerous move and could potentially break stuff after
release (and leads to workaround like the reverted commit), the
workaround technically remains, but is hidden behind a new environment
variable GIT_HASH_FIXUP. This should let the users continue to use git
while we fix the problem. This variable can be deleted after one or two
releases.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
common-main.c | 10 ++++++++++
repository.c | 2 +-
t/helper/test-dump-split-index.c | 2 ++
3 files changed, 13 insertions(+), 1 deletion(-)
@@ -5,7 +5,7 @@/* The main repository */staticstructrepositorythe_repo={-NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,&hash_algos[GIT_HASH_SHA1],0,0+NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,NULL,0,0};structrepository*the_repository=&the_repo;
On Sat, Feb 24, 2018 at 5:29 AM, brian m. carlson
[off-list ref] wrote:
quoted
@@ -40,5 +41,8 @@ int main(int argc, const char **argv) restore_sigpipe_to_default();+ if (getenv("GIT_HASH_FIXUP"))+ repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
I'm lukewarm on adding this environment variable, but considering our
history here, we had probably better. We can always remove it after a
few releases.
Yes that's the intention. But after writing cover letter for v2 and
sending it out, it looks to me that this thing must stay until all our
code is converted to using the_hash_algo (I don't know if there are
more to convert or it's finished already). So an alternative is we do
the opposite: default to GIT_HASH_SHA1, but when an env variable is
set, reset it back to NULL. This env variable will _always_ be set by
the test suite to help us catch problems.
--
Duy
From: Eric Sunshine <hidden> Date: 2018-02-24 08:15:47
On Fri, Feb 23, 2018 at 10:34 PM, Nguyễn Thái Ngọc Duy
[off-list ref] wrote:
Our "git diff" command supports running as a standalone tool. In this
code path, we try to hash the file content but after
18e2588e11 (sha1_file: switch uses of SHA-1 to the_hash_algo -
2018-02-01), there is a chance that the_hash_algo (required by
index_path) may still be uninitialized if no repository is found.
Executing index_path() when the_hash_algo is NULL (or points to unknown
algo) either crashes or dies. Let's make it a bit safer by explicitly
falling back to SHA-1 (so that the diff output remains the same as
before, compared to the alternative that we simply do not hash).
dòng được
@@ -3995,6 +3995,18 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)+ /*+ * NEEDSWORK: When running in no-index mode (and no repo is+ * found, thus no hash algo conifugred), fall back to SHA-1
s/conifugred/configured/
+ * hashing (which is used by diff_fill_oid_info below) to
+ * avoid regression in diff output.
+ *
+ * In future, perhaps we can allow the user to specify their
+ * hash algorithm from command line in this mode.
+ */
+ if (o->flags.no_index && !the_hash_algo)
+ the_hash_algo = &hash_algos[GIT_HASH_SHA1];
+
diff_fill_oid_info(one);
diff_fill_oid_info(two);
On Sat, Feb 24, 2018 at 10:34 AM, Nguyễn Thái Ngọc Duy
[off-list ref] wrote:
quoted hunk
@@ -3995,6 +3995,18 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o) return; }+ /*+ * NEEDSWORK: When running in no-index mode (and no repo is+ * found, thus no hash algo conifugred), fall back to SHA-1+ * hashing (which is used by diff_fill_oid_info below) to+ * avoid regression in diff output.+ *+ * In future, perhaps we can allow the user to specify their+ * hash algorithm from command line in this mode.+ */+ if (o->flags.no_index && !the_hash_algo)+ the_hash_algo = &hash_algos[GIT_HASH_SHA1];
Brian, are we supposed to use the_hash_algo this way (i.e. as a
writable var)? Or should I stick to something like
repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
which allows us to notify other parts inside struct repository about
the hash algorithm change, if we ever need to?
If the_hash_algo is supposed to be read-only, maybe I should convert
that macro to an inline function to prevent people from accidentally
reassigning it?
From: brian m. carlson <hidden> Date: 2018-02-24 22:29:14
On Sat, Feb 24, 2018 at 09:36:03PM +0700, Duy Nguyen wrote:
On Sat, Feb 24, 2018 at 10:34 AM, Nguyễn Thái Ngọc Duy
[off-list ref] wrote:
quoted
@@ -3995,6 +3995,18 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o) return; }+ /*+ * NEEDSWORK: When running in no-index mode (and no repo is+ * found, thus no hash algo conifugred), fall back to SHA-1+ * hashing (which is used by diff_fill_oid_info below) to+ * avoid regression in diff output.+ *+ * In future, perhaps we can allow the user to specify their+ * hash algorithm from command line in this mode.+ */+ if (o->flags.no_index && !the_hash_algo)+ the_hash_algo = &hash_algos[GIT_HASH_SHA1];
Brian, are we supposed to use the_hash_algo this way (i.e. as a
writable var)? Or should I stick to something like
repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
which allows us to notify other parts inside struct repository about
the hash algorithm change, if we ever need to?
I would definitely recommend using the function. As you pointed out, it
makes our code future-proof against needing to more work when setting
the value.
If the_hash_algo is supposed to be read-only, maybe I should convert
that macro to an inline function to prevent people from accidentally
reassigning it?
You could if you want to, although I don't really see a need to, since
we can just catch it in review. If you wanted to, I'd make it an inline
function for performance reasons.
--
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
From: brian m. carlson <hidden> Date: 2018-02-24 22:40:27
On Sat, Feb 24, 2018 at 10:34:26AM +0700, Nguyễn Thái Ngọc Duy wrote:
This is mostly for displaying the hash algorithm name when we report
errors. Printing "unknown" with '%s' is much better than '(null)' in
glibc printf version (and probably could crash if other implementations
do not check for NULL pointer)
This is a good change. Using a NULL pointer with %s on the BSDs
definitely did cause a crash, last I checked.
I looked at the comment I wrote in hash.h, and based on that, it is
indeed very likely that someone would accidentally write code this way.
--
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
From: brian m. carlson <hidden> Date: 2018-02-24 22:56:36
On Sat, Feb 24, 2018 at 10:34:27AM +0700, Nguyễn Thái Ngọc Duy wrote:
quoted hunk
After 454253f059 (builtin/index-pack: improve hash function abstraction
- 2018-02-01), index-pack uses the_hash_algo for hashing. If "git
index-pack" is executed without a repository, we do not know what hash
algorithm to be used and the_hash_algo in theory could be undefined.
Since there should be some information about the hash algorithm in the
input pack file, we can initialize the correct hash algorithm with that
if the_hash_algo is not yet initialized. This assumes that pack files
with new hash algorithm MUST step up pack version.
While at there, make sure the hash algorithm requested by the pack file
and configured by the repository (if we're running with a repo) are
consistent.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
builtin/index-pack.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
@@ -326,10 +326,31 @@ static const char *open_pack_file(const char *pack_name)output_fd=-1;nothread_data.pack_fd=input_fd;}-the_hash_algo->init_fn(&input_ctx);returnpack_name;}+staticvoidprepare_hash_algo(uint32_tpack_version)+{+conststructgit_hash_algo*pack_algo;++switch(pack_version){+case2:+case3:+pack_algo=&hash_algos[GIT_HASH_SHA1];+break;+default:+die("BUG: how to determine hash algo for new version?");+}++if(!the_hash_algo)/* running without repo */+the_hash_algo=pack_algo;++if(the_hash_algo!=pack_algo)+die(_("incompatible hash algorithm, "+"configured for %s but the pack file needs %s"),+the_hash_algo->name,pack_algo->name);
I like this. It's a nice improvement and it should be easy for us to
pass additional information into the function when our pack format
understands multiple algorithms.
I might have done the comparison using the format_id members instead of
the pointers themselves, but that's more a personal preference than
anything.
--
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
From: brian m. carlson <hidden> Date: 2018-02-24 22:58:50
On Sat, Feb 24, 2018 at 10:34:29AM +0700, Nguyễn Thái Ngọc Duy wrote:
quoted hunk
This reverts commit e26f7f19b6c7485f04234946a59ab8f4fd21d6d1. The root
problem, git clone not setting up the_hash_algo, has been fixed in the
previous patch.
Since this is a dangerous move and could potentially break stuff after
release (and leads to workaround like the reverted commit), the
workaround technically remains, but is hidden behind a new environment
variable GIT_HASH_FIXUP. This should let the users continue to use git
while we fix the problem. This variable can be deleted after one or two
releases.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
common-main.c | 10 ++++++++++
repository.c | 2 +-
t/helper/test-dump-split-index.c | 2 ++
3 files changed, 13 insertions(+), 1 deletion(-)
@@ -5,7 +5,7 @@/* The main repository */staticstructrepositorythe_repo={-NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,&hash_algos[GIT_HASH_SHA1],0,0+NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,NULL,0,0
I'm wondering, now that you have the name field for the unknown value,
if that might be a better choice here than NULL. I don't have a strong
preference either way, so whatever you decide here is fine.
--
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
@@ -5,7 +5,7 @@/* The main repository */staticstructrepositorythe_repo={-NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,&hash_algos[GIT_HASH_SHA1],0,0+NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,NULL,0,0
I'm wondering, now that you have the name field for the unknown value,
if that might be a better choice here than NULL. I don't have a strong
preference either way, so whatever you decide here is fine.
I did try that first, but for the purpose of catching uninitialized
algo use, NULL is better.
When I set unknown hash algo here, I think some test failed
mysteriously because it used rawsz field (which has value zero), it
didn't match some expectation, the code went to the error handling
path, which eventually failed with some error message, but it's not
obvious that the problem was rawsz being zero and back tracking that
took me some time.
With NULL hash_algo, any dereferencing fails immediately with a nice
stack trace. Another reason to push me towards NULL hash algo is, even
if we prefer nice messages over segmentation faults, we can't avoid it
completely anyway (empty_tree and empty_blob are still NULL in unknown
hash algo and will cause segfaults). Might as well make things
consistent and always segfault.
--
Duy
v3 refines v2 a bit more:
- fix configure typo (and stray words in commit message)
- use repo_set_hash_algo() instead of reassigning the_hash_algo
- compare hash algos by format_id
- catch NULL hash algo, report nicely and suggest GIT_HASH_FIXUP
The last point makes me much happier about keeping this workaround
around until we are confident we can live without it. Interdiff
@@ -331,21 +331,24 @@ static const char *open_pack_file(const char *pack_name)staticvoidprepare_hash_algo(uint32_tpack_version){+intpack_algo_id;conststructgit_hash_algo*pack_algo;switch(pack_version){case2:case3:-pack_algo=&hash_algos[GIT_HASH_SHA1];+pack_algo_id=GIT_HASH_SHA1;break;default:-die("BUG: how to determine hash algo for new version?");+die("BUG: how to determine hash algo for version %d?",+pack_version);}-if(!the_hash_algo)/* running without repo */-the_hash_algo=pack_algo;+if(!repo_has_valid_hash_algo(the_repository))/* running without repo */+repo_set_hash_algo(the_repository,pack_algo_id);-if(the_hash_algo!=pack_algo)+pack_algo=&hash_algos[pack_algo_id];+if(the_hash_algo->format_id!=pack_algo->format_id)die(_("incompatible hash algorithm, ""configured for %s but the pack file needs %s"),the_hash_algo->name,pack_algo->name);
@@ -107,4 +107,20 @@ extern void repo_clear(struct repository *repo);*/externintrepo_read_index(structrepository*repo);+staticinlineconststructgit_hash_algo*repo_get_hash_algo(+conststructrepository*repo)+{+if(!repo->hash_algo)+die("BUG: hash_algo is not initialized!\n%s",+_("You can work around this by setting environment"+" variable GIT_HASH_FIXUP=1.\n"+"Please report this to git@vger.kernel.org"));+returnrepo->hash_algo;+}++staticinlineintrepo_has_valid_hash_algo(conststructrepository*repo)+{+returnrepo->hash_algo!=NULL;+}+#endif /* REPOSITORY_H */
Nguyễn Thái Ngọc Duy (6):
setup.c: initialize the_repository correctly in all cases
sha1_file.c: keep a good name for "unknown" hash_algos[UNKNOWN]
cache.h: make the_hash_algo read-only
index-pack: check (and optionally set) hash algo based on input file
diff.c: initialize hash algo when running in --no-index mode
Revert "repository: pre-initialize hash algo pointer"
builtin/index-pack.c | 29 ++++++++++++++++++++++++++++-
builtin/init-db.c | 3 ++-
cache.h | 5 +++--
common-main.c | 10 ++++++++++
diff.c | 12 ++++++++++++
path.c | 2 +-
repository.c | 2 +-
repository.h | 16 ++++++++++++++++
setup.c | 5 ++++-
sha1_file.c | 2 +-
t/helper/test-dump-split-index.c | 2 ++
11 files changed, 80 insertions(+), 8 deletions(-)
--
2.16.1.435.g8f24da2e1a
There are many ways for any command to access a git repository:
- most of them will try to discover the .git dir via
setup_git_directory() and friends
- the server side programs already know where the repo is and prepare
with enter_repo()
- special commands that deal with repo creation (init/clone) use
init_db() once the new repo is ready for access.
- somebody accesses $GIT_DIR before any of above functions are called
and accidentally sets up a git repository by set_git_dir() alone
"the_repository" is partially set up via set_git_dir() at some point
in all four cases. The hash algorithm though is configured later after
.git/config is read.
So far proper repo initialization is done only for the first case [1].
The second case is not covered (but that's fine [3]). The third case
was found and worked around in [2]. The fourth case is a buggy one,
which should be fixed already by jk/no-looking-at-dotgit-outside-repo
and never happens again.
This patch makes sure all cases initialize the hash algorithm in
the_repository correctly. Both second and third cases must run
check_repo_format() before "entering" it. Eventually we probably just
rename this function to init_repo() or something.
[1] 78a6766802 (Integrate hash algorithm support with repo setup -
2017-11-12)
[2] e26f7f19b6 (repository: pre-initialize hash algo pointer -
2018-01-19)
[3] the reason server side is still running ok with no hash algo before
[2] is because the programs that use enter_repo() do very
little (and unlikely to hash anything) then spawn a new
program (like pack-objects or upload-archive) to do the heavy
lifting. These programs already use setup_git_directory() or the
gently version
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
builtin/init-db.c | 3 ++-
cache.h | 3 ++-
path.c | 2 +-
setup.c | 5 ++++-
4 files changed, 9 insertions(+), 4 deletions(-)
@@ -894,6 +894,7 @@ extern int repository_format_precious_objects;externchar*repository_format_partial_clone;externconstchar*core_partial_clone_filter_default;+structrepository;structrepository_format{intversion;intprecious_objects;
@@ -926,7 +927,7 @@ int verify_repository_format(const struct repository_format *format,*set_git_dir()beforecallingthis,anduseitonlyfor"are we in a valid*repo?".*/-externvoidcheck_repository_format(void);+externvoidcheck_repository_format(structrepository*);#define MTIME_CHANGED 0x0001#define CTIME_CHANGED 0x0002
@@ -1180,11 +1180,14 @@ int git_config_perm(const char *var, const char *value)return-(i&0666);}-voidcheck_repository_format(void)+/* optionally configure "repo" to the correct format */+voidcheck_repository_format(structrepository*repo){structrepository_formatrepo_fmt;check_repository_format_gently(get_git_dir(),&repo_fmt,NULL);startup_info->have_repository=1;+if(repo)+repo_set_hash_algo(repo,repo_fmt.hash_algo);}/*
This is mostly for displaying the hash algorithm name when we report
errors. Printing "unknown" with '%s' is much better than '(null)' in
glibc printf version (and probably could crash if other implementations
do not check for NULL pointer)
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
sha1_file.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
By returning an R-value in the_hash_algo we make sure people can't
accidentally change hash algorithm with
the_hash_algo = &hash_algos[something];
and go with repo_set_hash_algo() instead. Of course they can still do
the_repository->hash_algo = ...
but that is more obvious and easily caught in review.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
cache.h | 2 +-
repository.h | 6 ++++++
2 files changed, 7 insertions(+), 1 deletion(-)
After 454253f059 (builtin/index-pack: improve hash function abstraction
- 2018-02-01), index-pack uses the_hash_algo for hashing. If "git
index-pack" is executed without a repository, we do not know what hash
algorithm to be used and the_hash_algo in theory could be undefined.
Since there should be some information about the hash algorithm in the
input pack file, we can initialize the correct hash algorithm with that
if the_hash_algo is not yet initialized. This assumes that pack files
with new hash algorithm MUST step up pack version.
While at there, make sure the hash algorithm requested by the pack file
and configured by the repository (if we're running with a repo) are
consistent.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
builtin/index-pack.c | 29 ++++++++++++++++++++++++++++-
repository.h | 5 +++++
2 files changed, 33 insertions(+), 1 deletion(-)
@@ -326,10 +326,34 @@ static const char *open_pack_file(const char *pack_name)output_fd=-1;nothread_data.pack_fd=input_fd;}-the_hash_algo->init_fn(&input_ctx);returnpack_name;}+staticvoidprepare_hash_algo(uint32_tpack_version)+{+intpack_algo_id;+conststructgit_hash_algo*pack_algo;++switch(pack_version){+case2:+case3:+pack_algo_id=GIT_HASH_SHA1;+break;+default:+die("BUG: how to determine hash algo for version %d?",+pack_version);+}++if(!repo_has_valid_hash_algo(the_repository))/* running without repo */+repo_set_hash_algo(the_repository,pack_algo_id);++pack_algo=&hash_algos[pack_algo_id];+if(the_hash_algo->format_id!=pack_algo->format_id)+die(_("incompatible hash algorithm, "+"configured for %s but the pack file needs %s"),+the_hash_algo->name,pack_algo->name);+}+staticvoidparse_pack_header(void){structpack_header*hdr=fill(sizeof(structpack_header));
@@ -341,6 +365,9 @@ static void parse_pack_header(void)die(_("pack version %"PRIu32" unsupported"),ntohl(hdr->hdr_version));+prepare_hash_algo(ntohl(hdr->hdr_version));+the_hash_algo->init_fn(&input_ctx);+nr_objects=ntohl(hdr->hdr_entries);use(sizeof(structpack_header));}
Our "git diff" command supports running as a standalone tool. In this
code path, we try to hash the file content but after
18e2588e11 (sha1_file: switch uses of SHA-1 to the_hash_algo -
2018-02-01), there is a chance that the_hash_algo (required by
index_path) may still be uninitialized if no repository is found.
Executing index_path() when the_hash_algo is NULL (or points to unknown
algo) either crashes or dies. Let's make it a bit safer by explicitly
falling back to SHA-1 (so that the diff output remains the same as
before, compared to the alternative that we simply do not hash).
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
diff.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
This reverts commit e26f7f19b6c7485f04234946a59ab8f4fd21d6d1. The root
problem, git clone not setting up the_hash_algo, has been fixed in the
previous patch.
Since this is a dangerous move and could potentially break stuff after
release (and leads to workaround like the reverted commit), the
workaround technically remains, but is hidden behind a new environment
variable GIT_HASH_FIXUP. This should let the users continue to use git
while we fix the problem. This variable can be deleted after one or two
releases.
test-dump-split-index.c needs to call setup_git_directory() after this
to configure hash algorithm before parsing the index file. It's not the
best way (i.e. check index file version) but it's a test tool, this is
good enough.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
common-main.c | 10 ++++++++++
repository.c | 2 +-
repository.h | 5 +++++
t/helper/test-dump-split-index.c | 2 ++
4 files changed, 18 insertions(+), 1 deletion(-)
@@ -5,7 +5,7 @@/* The main repository */staticstructrepositorythe_repo={-NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,&hash_algos[GIT_HASH_SHA1],0,0+NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,&the_index,NULL,0,0};structrepository*the_repository=&the_repo;
@@ -110,6 +110,11 @@ extern int repo_read_index(struct repository *repo);staticinlineconststructgit_hash_algo*repo_get_hash_algo(conststructrepository*repo){+if(!repo->hash_algo)+die("BUG: hash_algo is not initialized!\n%s",+_("You can work around this by setting environment"+" variable GIT_HASH_FIXUP=1.\n"+"Please report this to git@vger.kernel.org"));returnrepo->hash_algo;}
From: brian m. carlson <hidden> Date: 2018-02-25 20:28:32
On Sun, Feb 25, 2018 at 10:29:29AM +0700, Duy Nguyen wrote:
When I set unknown hash algo here, I think some test failed
mysteriously because it used rawsz field (which has value zero), it
didn't match some expectation, the code went to the error handling
path, which eventually failed with some error message, but it's not
obvious that the problem was rawsz being zero and back tracking that
took me some time.
With NULL hash_algo, any dereferencing fails immediately with a nice
stack trace. Another reason to push me towards NULL hash algo is, even
if we prefer nice messages over segmentation faults, we can't avoid it
completely anyway (empty_tree and empty_blob are still NULL in unknown
hash algo and will cause segfaults). Might as well make things
consistent and always segfault.
From: brian m. carlson <hidden> Date: 2018-02-25 20:34:12
On Sun, Feb 25, 2018 at 06:18:34PM +0700, Nguyễn Thái Ngọc Duy wrote:
v3 refines v2 a bit more:
- fix configure typo (and stray words in commit message)
- use repo_set_hash_algo() instead of reassigning the_hash_algo
- compare hash algos by format_id
- catch NULL hash algo, report nicely and suggest GIT_HASH_FIXUP
The last point makes me much happier about keeping this workaround
around until we are confident we can live without it. Interdiff
It turns out I don't need my other series [1] in order to delete this
field. This series moves getenv() calls from
repo_set_gitdir()/repo_setup_env() and prepare_alt_odb() back in
environment.c where they belong in my opinion.
The repo_set_gitdir() now takes $GIT_DIR and optionally all other
configurable paths. If those paths are NULL, default repo layout will
be used. With getenv() no longer called inside repo_set_gitdir(),
ignore_env has no reason to stay. This is in 1/4.
The getenv() in prepare_alt_odb() is also moved back to
setup_git_env() in 3/4. It demonstrates how we could move other
getenv() back to if we want.
This series is built on top of Stefan's object-store-part1, v4. I
could rebase it on 'master' too, but then Junio may need to resolve
some conflicts.
[1] https://public-inbox.org/git/20180225111840.16421-1-pclouds@gmail.com/
Nguyễn Thái Ngọc Duy (4):
repository.c: move env-related setup code back to environment.c
repository.c: delete dead functions
sha1_file.c: move delayed getenv(altdb) back to setup_git_env()
repository: delete ignore_env member
cache.h | 2 +-
environment.c | 13 +++++++--
object-store.h | 5 +++-
object.c | 1 +
repository.c | 79 ++++++++++++++++++++++----------------------------
repository.h | 21 +++++++-------
setup.c | 3 +-
sha1_file.c | 6 +---
8 files changed, 64 insertions(+), 66 deletions(-)
--
2.16.1.435.g8f24da2e1a
It does not make sense that generic repository code contains handling
of environment variables, which are specific for the main repository
only. Refactor repo_set_gitdir() function to take $GIT_DIR and
optionally _all_ other customizable paths. These optional paths can be
NULL and will be calculated according to the default directory layout.
Note that some dead functions are left behind to reduce diff
noise. They will be deleted in the next patch.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
cache.h | 2 +-
environment.c | 12 +++++++++---
repository.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
repository.h | 11 ++++++++++-
setup.c | 3 +--
5 files changed, 63 insertions(+), 13 deletions(-)
@@ -301,8 +308,7 @@ int set_git_dir(const char *path){if(setenv(GIT_DIR_ENVIRONMENT,path,1))returnerror("Could not set GIT_DIR to '%s'",path);-repo_set_gitdir(the_repository,path);-setup_git_env();+setup_git_env(path);return0;}