[PATCH 4/4] repository: delete ignore_env member

[Nguyễn Thái Ngọc Duy]

This variable was added because the repo_set_gitdir() was created to
cover both submodule and main repos, but these two are initialized a
bit differently so ignore_env == 0 means main repo, while ignore_env
!= 0 is submodules.

Since the difference part (env variables) has been moved out of
repo_set_gitdir(), this function works the same way for both repo
types and ignore_env is not needed anymore.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 repository.c | 4 +---
 repository.h | 9 -
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/repository.c b/repository.c
index 19532ec5ff..8f6386022f 100644
--- a/repository.c
+++ b/repository.c
@@ -12,7 +12,7 @@ static struct repository the_repo = {
NULL, NULL, NULL,
_index,
_algos[GIT_HASH_SHA1],
-   0, 0
+   0
 };
 struct repository *the_repository = _repo;
 
@@ -134,8 +134,6 @@ int repo_init(struct repository *repo, const char *gitdir, 
const char *worktree)
struct repository_format format;
memset(repo, 0, sizeof(*repo));
 
-   repo->ignore_env = 1;
-
INIT_LIST_HEAD(>objects.packed_git_mru);
 
if (repo_init_gitdir(repo, gitdir))
diff --git a/repository.h b/repository.h
index b1da2a6384..07e8971428 100644
--- a/repository.h
+++ b/repository.h
@@ -73,15 +73,6 @@ struct repository {
const struct git_hash_algo *hash_algo;
 
/* Configurations */
-   /*
-* Bit used during initialization to indicate if repository state (like
-* the location of the 'objectdir') should be read from the
-* environment.  By default this bit will be set at the begining of
-* 'repo_init()' so that all repositories will ignore the environment.
-* The exception to this is 'the_repository', which doesn't go through
-* the normal 'repo_init()' process.
-*/
-   unsigned ignore_env:1;
 
/* Indicate if a repository has a different 'commondir' from 'gitdir' */
unsigned different_commondir:1;
-- 
2.16.1.435.g8f24da2e1a

[PATCH 3/4] sha1_file.c: move delayed getenv(altdb) back to setup_git_env()

[Nguyễn Thái Ngọc Duy]

getenv() is supposed to work on the main repository only. This delayed
getenv() code in sha1_file.c makes it more difficult to convert
sha1_file.c to a generic object store that could be used by both
submodule and main repositories.

Move the getenv() back in setup_git_env() where other env vars are
also fetched.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 environment.c  | 1 +
 object-store.h | 5 -
 object.c   | 1 +
 repository.c   | 2 ++
 repository.h   | 1 +
 sha1_file.c| 6 +-
 6 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/environment.c b/environment.c
index 2905042b16..95de419de8 100644
--- a/environment.c
+++ b/environment.c
@@ -158,6 +158,7 @@ void setup_git_env(const char *git_dir)
args.object_dir = getenv(DB_ENVIRONMENT);
args.graft_file = getenv(GRAFT_ENVIRONMENT);
args.index_file = getenv(INDEX_ENVIRONMENT);
+   args.alternate_db = getenv(ALTERNATE_DB_ENVIRONMENT);
repo_set_gitdir(the_repository, git_dir, );
 
if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
diff --git a/object-store.h b/object-store.h
index afe2f93459..9b1303549b 100644
--- a/object-store.h
+++ b/object-store.h
@@ -87,6 +87,9 @@ struct raw_object_store {
 */
char *objectdir;
 
+   /* Path to extra alternate object database if not NULL */
+   char *alternate_db;
+
struct packed_git *packed_git;
/* A most-recently-used ordered version of the packed_git list. */
struct list_head packed_git_mru;
@@ -109,7 +112,7 @@ struct raw_object_store {
unsigned packed_git_initialized : 1;
 };
 
-#define RAW_OBJECT_STORE_INIT(o) { NULL, NULL, 
LIST_HEAD_INIT(o.packed_git_mru), NULL, NULL, 0, 0, 0 }
+#define RAW_OBJECT_STORE_INIT(o) { NULL, NULL, NULL, 
LIST_HEAD_INIT(o.packed_git_mru), NULL, NULL, 0, 0, 0 }
 
 void raw_object_store_clear(struct raw_object_store *o);
 
diff --git a/object.c b/object.c
index a7c238339b..5317cfc390 100644
--- a/object.c
+++ b/object.c
@@ -464,6 +464,7 @@ static void free_alt_odbs(struct raw_object_store *o)
 void raw_object_store_clear(struct raw_object_store *o)
 {
FREE_AND_NULL(o->objectdir);
+   FREE_AND_NULL(o->alternate_db);
 
free_alt_odbs(o);
o->alt_odb_tail = NULL;
diff --git a/repository.c b/repository.c
index be86da82cc..19532ec5ff 100644
--- a/repository.c
+++ b/repository.c
@@ -56,6 +56,8 @@ void repo_set_gitdir(struct repository *repo,
repo_set_commondir(repo, o->shared_root);
expand_base_dir(>objects.objectdir, o->object_dir,
repo->commondir, "objects");
+   free(repo->objects.alternate_db);
+   repo->objects.alternate_db = xstrdup_or_null(o->alternate_db);
expand_base_dir(>graft_file, o->graft_file,
repo->commondir, "info/grafts");
expand_base_dir(>index_file, o->index_file,
diff --git a/repository.h b/repository.h
index b5b5d138aa..b1da2a6384 100644
--- a/repository.h
+++ b/repository.h
@@ -94,6 +94,7 @@ struct set_gitdir_args {
const char *object_dir;
const char *graft_file;
const char *index_file;
+   const char *alternate_db;
 };
 
 extern void repo_set_gitdir(struct repository *repo,
diff --git a/sha1_file.c b/sha1_file.c
index dfc8deec38..ad1cd441e6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -673,15 +673,11 @@ int foreach_alt_odb(alt_odb_fn fn, void *cb)
 
 void prepare_alt_odb(struct repository *r)
 {
-   const char *alt;
-
if (r->objects.alt_odb_tail)
return;
 
-   alt = getenv(ALTERNATE_DB_ENVIRONMENT);
-
r->objects.alt_odb_tail = >objects.alt_odb_list;
-   link_alt_odb_entries(r, alt, PATH_SEP, NULL, 0);
+   link_alt_odb_entries(r, r->objects.alternate_db, PATH_SEP, NULL, 0);
 
read_info_alternates(r, r->objects.objectdir, 0);
 }
-- 
2.16.1.435.g8f24da2e1a

[PATCH 2/4] repository.c: delete dead functions

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 repository.c | 45 -
 1 file changed, 45 deletions(-)

diff --git a/repository.c b/repository.c
index 70dc8dc661..be86da82cc 100644
--- a/repository.c
+++ b/repository.c
@@ -16,51 +16,6 @@ static struct repository the_repo = {
 };
 struct repository *the_repository = _repo;
 
-static char *git_path_from_env(const char *envvar, const char *git_dir,
-  const char *path, int fromenv)
-{
-   if (fromenv) {
-   const char *value = getenv(envvar);
-   if (value)
-   return xstrdup(value);
-   }
-
-   return xstrfmt("%s/%s", git_dir, path);
-}
-
-static int find_common_dir(struct strbuf *sb, const char *gitdir, int fromenv)
-{
-   if (fromenv) {
-   const char *value = getenv(GIT_COMMON_DIR_ENVIRONMENT);
-   if (value) {
-   strbuf_addstr(sb, value);
-   return 1;
-   }
-   }
-
-   return get_common_dir_noenv(sb, gitdir);
-}
-
-static void repo_setup_env(struct repository *repo)
-{
-   struct strbuf sb = STRBUF_INIT;
-
-   repo->different_commondir = find_common_dir(, repo->gitdir,
-   !repo->ignore_env);
-   free(repo->commondir);
-   repo->commondir = strbuf_detach(, NULL);
-   raw_object_store_clear(>objects);
-   repo->objects.objectdir =
-   git_path_from_env(DB_ENVIRONMENT, repo->commondir,
- "objects", !repo->ignore_env);
-   free(repo->graft_file);
-   repo->graft_file = git_path_from_env(GRAFT_ENVIRONMENT, repo->commondir,
-"info/grafts", !repo->ignore_env);
-   free(repo->index_file);
-   repo->index_file = git_path_from_env(INDEX_ENVIRONMENT, repo->gitdir,
-"index", !repo->ignore_env);
-}
-
 static void expand_base_dir(char **out, const char *in,
const char *base_dir, const char *def_in)
 {
-- 
2.16.1.435.g8f24da2e1a

[PATCH 1/4] repository.c: move env-related setup code back to environment.c

[Nguyễn Thái Ngọc Duy]

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 
---
 cache.h   |  2 +-
 environment.c | 12 +---
 repository.c  | 48 ++--
 repository.h  | 11 ++-
 setup.c   |  3 +--
 5 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/cache.h b/cache.h
index 5717399183..b164a407eb 100644
--- a/cache.h
+++ b/cache.h
@@ -459,7 +459,7 @@ static inline enum object_type object_type(unsigned int 
mode)
  */
 extern const char * const local_repo_env[];
 
-extern void setup_git_env(void);
+extern void setup_git_env(const char *git_dir);
 
 /*
  * Returns true iff we have a configured git repository (either via
diff --git a/environment.c b/environment.c
index ec10b062e6..2905042b16 100644
--- a/environment.c
+++ b/environment.c
@@ -148,10 +148,17 @@ static char *expand_namespace(const char *raw_namespace)
return strbuf_detach(, NULL);
 }
 
-void setup_git_env(void)
+void setup_git_env(const char *git_dir)
 {
const char *shallow_file;
const char *replace_ref_base;
+   struct set_gitdir_args args = { NULL };
+
+   args.shared_root = getenv(GIT_COMMON_DIR_ENVIRONMENT);
+   args.object_dir = getenv(DB_ENVIRONMENT);
+   args.graft_file = getenv(GRAFT_ENVIRONMENT);
+   args.index_file = getenv(INDEX_ENVIRONMENT);
+   repo_set_gitdir(the_repository, git_dir, );
 
if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
check_replace_refs = 0;
@@ -301,8 +308,7 @@ int set_git_dir(const char *path)
 {
if (setenv(GIT_DIR_ENVIRONMENT, path, 1))
return error("Could not set GIT_DIR to '%s'", path);
-   repo_set_gitdir(the_repository, path);
-   setup_git_env();
+   setup_git_env(path);
return 0;
 }
 
diff --git a/repository.c b/repository.c
index a069b1b640..70dc8dc661 100644
--- a/repository.c
+++ b/repository.c
@@ -61,15 +61,50 @@ static void repo_setup_env(struct repository *repo)
 "index", !repo->ignore_env);
 }
 
-void repo_set_gitdir(struct repository *repo, const char *path)
+static void expand_base_dir(char **out, const char *in,
+   const char *base_dir, const char *def_in)
 {
-   const char *gitfile = read_gitfile(path);
-   char *old_gitdir = repo->gitdir;
+   free(*out);
+   if (in)
+   *out = xstrdup(in);
+   else
+   *out = xstrfmt("%s/%s", base_dir, def_in);
+}
+
+static void repo_set_commondir(struct repository *repo,
+  const char *shared_root)
+{
+   struct strbuf sb = STRBUF_INIT;
+
+   free(repo->commondir);
 
-   repo->gitdir = xstrdup(gitfile ? gitfile : path);
-   repo_setup_env(repo);
+   if (shared_root) {
+   repo->different_commondir = 1;
+   repo->commondir = xstrdup(shared_root);
+   return;
+   }
 
+   repo->different_commondir = get_common_dir_noenv(, repo->gitdir);
+   repo->commondir = strbuf_detach(, NULL);
+}
+
+void repo_set_gitdir(struct repository *repo,
+const char *root,
+const struct set_gitdir_args *o)
+{
+   const char *gitfile = read_gitfile(root);
+   char *old_gitdir = repo->gitdir;
+
+   repo->gitdir = xstrdup(gitfile ? gitfile : root);
free(old_gitdir);
+
+   repo_set_commondir(repo, o->shared_root);
+   expand_base_dir(>objects.objectdir, o->object_dir,
+   repo->commondir, "objects");
+   expand_base_dir(>graft_file, o->graft_file,
+   repo->commondir, "info/grafts");
+   expand_base_dir(>index_file, o->index_file,
+   repo->gitdir, "index");
 }
 
 void repo_set_hash_algo(struct repository *repo, int hash_algo)
@@ -87,6 +122,7 @@ static int repo_init_gitdir(struct repository *repo, const 
char *gitdir)
int error = 0;
char *abspath = NULL;
const char *resolved_gitdir;
+   struct set_gitdir_args args = { NULL };
 
abspath = real_pathdup(gitdir, 0);
if (!abspath) {
@@ -101,7 +137,7 @@ static int repo_init_gitdir(struct repository *repo, const 
char *gitdir)
goto out;
}
 
-   repo_set_gitdir(repo, resolved_gitdir);
+   repo_set_gitdir(repo, resolved_gitdir, );
 
 out:
free(abspath);
diff --git a/repository.h b/repository.h
index fa73ab8e93..b5b5d138aa 100644
--- a/repository.h
+++ b/repository.h
@@ -89,7 +89,16 @@ struct repository {
 
 extern 

[PATCH 0/4] Delete ignore_env member in struct repository

[Nguyễn Thái Ngọc Duy]

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-pclo...@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

Re: Use of uninitialised value of size 8 in sha1_name.c

[Christian Couder]

On Mon, Feb 26, 2018 at 10:53 AM, Jeff King  wrote:
> On Mon, Feb 26, 2018 at 10:04:22AM +0100, Christian Couder wrote:
>
>> ==21455== Use of uninitialised value of size 8
>> ==21455==at 0x2D2A73: get_hex_char_from_oid (sha1_name.c:492)
>> ==21455==by 0x2D2AFE: extend_abbrev_len (sha1_name.c:502)
>> ==21455==by 0x2D2C3D: find_abbrev_len_for_pack (sha1_name.c:551)
>> ==21455==by 0x2D2CFF: find_abbrev_len_packed (sha1_name.c:569)
>> ==21455==by 0x2D2E12: find_unique_abbrev_r (sha1_name.c:608)
>> ==21455==by 0x2DCB66: strbuf_add_unique_abbrev (strbuf.c:877)
>> ==21455==by 0x14F7CE: update_local_ref (fetch.c:700)
>> ==21455==by 0x1500CF: store_updated_refs (fetch.c:871)
>> ==21455==by 0x15035B: fetch_refs (fetch.c:932)
>> ==21455==by 0x150CF8: do_fetch (fetch.c:1146)
>> ==21455==by 0x1515AB: fetch_one (fetch.c:1370)
>> ==21455==by 0x151A1D: cmd_fetch (fetch.c:1457)
>> ==21455==  Uninitialised value was created by a stack allocation
>> ==21455==at 0x2D2B2E: find_abbrev_len_for_pack (sha1_name.c:513)
>> ==21455==
>>
>> A quick git blame seems to point to 0e87b85683 (sha1_name: minimize
>> OID comparisons during disambiguation, 2017-10-12).
>>
>> It is difficult to tell for sure though as t5616-partial-clone.sh was
>> added after that commit.
>
> I think that commit is to blame, though the error isn't exactly where
> that stack trace puts it. Try this:
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 611c7d24dd..6f7f36436f 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -547,7 +547,10 @@ static void find_abbrev_len_for_pack(struct packed_git 
> *p,
>  */
> mad->init_len = 0;
> if (!match) {
> -   nth_packed_object_oid(, p, first);
> +   warning("p->num_objects = %u, first = %u",
> +   p->num_objects, first);
> +   if (!nth_packed_object_oid(, p, first))
> +   die("oops!");
> extend_abbrev_len(, mad);
> } else if (first < num - 1) {
> nth_packed_object_oid(, p, first + 1);
>
> I get failures all over the test suite, like this:
>
>   warning: p->num_objects = 4, first = 3
>   warning: p->num_objects = 8, first = 6
>   warning: p->num_objects = 10, first = 0
>   warning: p->num_objects = 4, first = 0
>   warning: p->num_objects = 8, first = 0
>   warning: p->num_objects = 10, first = 10
>   fatal: oops!

Yeah, I tried to t5601-clone.sh with --valgrind and I also get one
error (the same "Uninitialised value" error actually).

> Any time the abbreviated hex would go after the last object in the pack,
> then first==p->num_objects, and nth_packed_object_oid() will fail. That
> leaves uninitialized data in "oid", which is what valgrind complains
> about when we examine it in extend_abbrev_len().
>
> Most of the time the code behaves correctly anyway, because the
> uninitialized bytes are unlikely to match up with our hex and extend the
> length. Probably we just need to detect that case and skip the call to
> extend_abbrev_len() altogether.

Yeah, something like:

diff --git a/sha1_name.c b/sha1_name.c
index 611c7d24dd..a041d8d24f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -547,15 +547,15 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
 */
mad->init_len = 0;
if (!match) {
-   nth_packed_object_oid(, p, first);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first))
+   extend_abbrev_len(, mad);
} else if (first < num - 1) {
-   nth_packed_object_oid(, p, first + 1);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first + 1))
+   extend_abbrev_len(, mad);
}
if (first > 0) {
-   nth_packed_object_oid(, p, first - 1);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first - 1))
+   extend_abbrev_len(, mad);
}
mad->init_len = mad->cur_len;
 }

seems to prevent valgrind from complaining when running t5616-partial-clone.sh.

Re: What's cooking in git.git (Feb 2018, #03; Wed, 21)

[Duy Nguyen]

On Thu, Feb 22, 2018 at 7:31 AM, Junio C Hamano  wrote:
> * nd/worktree-move (2018-02-12) 7 commits
>  - worktree remove: allow it when $GIT_WORK_TREE is already gone
>  - worktree remove: new command
>  - worktree move: refuse to move worktrees with submodules
>  - worktree move: accept destination as directory
>  - worktree move: new command
>  - worktree.c: add update_worktree_location()
>  - worktree.c: add validate_worktree()
>
>  "git worktree" learned move and remove subcommands.
>
>  Expecting a reroll.
>  cf. <20180124095357.19645-1-pclo...@gmail.com>

I think 'pu' already has the reroll (v2). So far no new comments.
-- 
Duy

Re: Use of uninitialised value of size 8 in sha1_name.c

[Jeff King]

On Mon, Feb 26, 2018 at 10:04:22AM +0100, Christian Couder wrote:

> ==21455== Use of uninitialised value of size 8
> ==21455==at 0x2D2A73: get_hex_char_from_oid (sha1_name.c:492)
> ==21455==by 0x2D2AFE: extend_abbrev_len (sha1_name.c:502)
> ==21455==by 0x2D2C3D: find_abbrev_len_for_pack (sha1_name.c:551)
> ==21455==by 0x2D2CFF: find_abbrev_len_packed (sha1_name.c:569)
> ==21455==by 0x2D2E12: find_unique_abbrev_r (sha1_name.c:608)
> ==21455==by 0x2DCB66: strbuf_add_unique_abbrev (strbuf.c:877)
> ==21455==by 0x14F7CE: update_local_ref (fetch.c:700)
> ==21455==by 0x1500CF: store_updated_refs (fetch.c:871)
> ==21455==by 0x15035B: fetch_refs (fetch.c:932)
> ==21455==by 0x150CF8: do_fetch (fetch.c:1146)
> ==21455==by 0x1515AB: fetch_one (fetch.c:1370)
> ==21455==by 0x151A1D: cmd_fetch (fetch.c:1457)
> ==21455==  Uninitialised value was created by a stack allocation
> ==21455==at 0x2D2B2E: find_abbrev_len_for_pack (sha1_name.c:513)
> ==21455==
> 
> A quick git blame seems to point to 0e87b85683 (sha1_name: minimize
> OID comparisons during disambiguation, 2017-10-12).
> 
> It is difficult to tell for sure though as t5616-partial-clone.sh was
> added after that commit.

I think that commit is to blame, though the error isn't exactly where
that stack trace puts it. Try this:

diff --git a/sha1_name.c b/sha1_name.c
index 611c7d24dd..6f7f36436f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -547,7 +547,10 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
 */
mad->init_len = 0;
if (!match) {
-   nth_packed_object_oid(, p, first);
+   warning("p->num_objects = %u, first = %u",
+   p->num_objects, first);
+   if (!nth_packed_object_oid(, p, first))
+   die("oops!");
extend_abbrev_len(, mad);
} else if (first < num - 1) {
nth_packed_object_oid(, p, first + 1);

I get failures all over the test suite, like this:

  warning: p->num_objects = 4, first = 3
  warning: p->num_objects = 8, first = 6
  warning: p->num_objects = 10, first = 0
  warning: p->num_objects = 4, first = 0
  warning: p->num_objects = 8, first = 0
  warning: p->num_objects = 10, first = 10
  fatal: oops!

Any time the abbreviated hex would go after the last object in the pack,
then first==p->num_objects, and nth_packed_object_oid() will fail. That
leaves uninitialized data in "oid", which is what valgrind complains
about when we examine it in extend_abbrev_len().

Most of the time the code behaves correctly anyway, because the
uninitialized bytes are unlikely to match up with our hex and extend the
length. Probably we just need to detect that case and skip the call to
extend_abbrev_len() altogether.

-Peff

Re: [PATCHv4 01/27] repository: introduce raw object store field

[Duy Nguyen]

On Fri, Feb 23, 2018 at 04:47:28PM -0800, Stefan Beller wrote:
>  /* The main repository */
>  static struct repository the_repo = {
> - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, _index, 
> _algos[GIT_HASH_SHA1], 0, 0
> + NULL, NULL,
> + RAW_OBJECT_STORE_INIT,
> + NULL, NULL, NULL,
> + NULL, NULL, NULL,
> + _index,
> + _algos[GIT_HASH_SHA1],
> + 0, 0
>  };
>  struct repository *the_repository = _repo;

I wonder if we should do something like this. It makes the_repo
initialization easier to read and it helps unify the init code with
submodule.

No don't reroll. If you think it's a good idea, you can do something
like this in the next series instead.

-- 8< --
diff --git a/check-racy.c b/check-racy.c
index 24b6542352..47cbb4eb6d 100644
--- a/check-racy.c
+++ b/check-racy.c
@@ -1,10 +1,12 @@
 #include "cache.h"
+#include "repository.h"
 
 int main(int ac, char **av)
 {
int i;
int dirty, clean, racy;
 
+   init_the_repository();
dirty = clean = racy = 0;
read_cache();
for (i = 0; i < active_nr; i++) {
diff --git a/common-main.c b/common-main.c
index 6a689007e7..a13ab981aa 100644
--- a/common-main.c
+++ b/common-main.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "attr.h"
+#include "repository.h"
 
 /*
  * Many parts of Git have subprograms communicate via pipe, expect the
@@ -32,6 +33,8 @@ int main(int argc, const char **argv)
 */
sanitize_stdfds();
 
+   init_the_repository();
+
git_setup_gettext();
 
attr_start();
diff --git a/object-store.h b/object-store.h
index cf35760ceb..c3253ebc59 100644
--- a/object-store.h
+++ b/object-store.h
@@ -8,8 +8,8 @@ struct raw_object_store {
 */
char *objectdir;
 };
-#define RAW_OBJECT_STORE_INIT { NULL }
 
+void raw_object_store_init(struct raw_object_store *o);
 void raw_object_store_clear(struct raw_object_store *o);
 
 #endif /* OBJECT_STORE_H */
diff --git a/object.c b/object.c
index 11d904c033..8a4d01dd5f 100644
--- a/object.c
+++ b/object.c
@@ -446,6 +446,11 @@ void clear_commit_marks_all(unsigned int flags)
}
 }
 
+void raw_object_store_init(struct raw_object_store *o)
+{
+   memset(o, 0, sizeof(*o));
+}
+
 void raw_object_store_clear(struct raw_object_store *o)
 {
free(o->objectdir);
diff --git a/repository.c b/repository.c
index 2255ff657e..0ebcca8539 100644
--- a/repository.c
+++ b/repository.c
@@ -5,16 +5,22 @@
 #include "submodule-config.h"
 
 /* The main repository */
-static struct repository the_repo = {
-   NULL, NULL,
-   RAW_OBJECT_STORE_INIT,
-   NULL, NULL, NULL,
-   NULL, NULL, NULL,
-   _index,
-   _algos[GIT_HASH_SHA1],
-   0, 0
-};
-struct repository *the_repository = _repo;
+static struct repository the_repo;
+struct repository *the_repository;
+
+static void repo_pre_init(struct repository *repo)
+{
+   memset(repo, 0, sizeof(*repo));
+   raw_object_store_init(>objects);
+}
+
+void init_the_repository(void)
+{
+   the_repository = _repo;
+   repo_pre_init(the_repository);
+   the_repository->index = _index;
+   repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+}
 
 static char *git_path_from_env(const char *envvar, const char *git_dir,
   const char *path, int fromenv)
@@ -138,7 +144,8 @@ static int read_and_verify_repository_format(struct 
repository_format *format,
 int repo_init(struct repository *repo, const char *gitdir, const char 
*worktree)
 {
struct repository_format format;
-   memset(repo, 0, sizeof(*repo));
+
+   repo_pre_init(repo);
 
repo->ignore_env = 1;
 
diff --git a/repository.h b/repository.h
index 1f8bc7a7cf..da6a8f9af9 100644
--- a/repository.h
+++ b/repository.h
@@ -93,6 +93,7 @@ extern void repo_set_gitdir(struct repository *repo, const 
char *path);
 extern void repo_set_worktree(struct repository *repo, const char *path);
 extern void repo_set_hash_algo(struct repository *repo, int algo);
 extern int repo_init(struct repository *repo, const char *gitdir, const char 
*worktree);
+extern void init_the_repository(void);
 extern int repo_submodule_init(struct repository *submodule,
   struct repository *superproject,
   const char *path);
-- 8< --

Use of uninitialised value of size 8 in sha1_name.c

[Christian Couder]

Hi Derrick,

These days when running:

./t5616-partial-clone.sh --valgrind

on master, I get a bunch of:

==21455== Use of uninitialised value of size 8
==21455==at 0x2D2A73: get_hex_char_from_oid (sha1_name.c:492)
==21455==by 0x2D2AFE: extend_abbrev_len (sha1_name.c:502)
==21455==by 0x2D2C3D: find_abbrev_len_for_pack (sha1_name.c:551)
==21455==by 0x2D2CFF: find_abbrev_len_packed (sha1_name.c:569)
==21455==by 0x2D2E12: find_unique_abbrev_r (sha1_name.c:608)
==21455==by 0x2DCB66: strbuf_add_unique_abbrev (strbuf.c:877)
==21455==by 0x14F7CE: update_local_ref (fetch.c:700)
==21455==by 0x1500CF: store_updated_refs (fetch.c:871)
==21455==by 0x15035B: fetch_refs (fetch.c:932)
==21455==by 0x150CF8: do_fetch (fetch.c:1146)
==21455==by 0x1515AB: fetch_one (fetch.c:1370)
==21455==by 0x151A1D: cmd_fetch (fetch.c:1457)
==21455==  Uninitialised value was created by a stack allocation
==21455==at 0x2D2B2E: find_abbrev_len_for_pack (sha1_name.c:513)
==21455==

A quick git blame seems to point to 0e87b85683 (sha1_name: minimize
OID comparisons during disambiguation, 2017-10-12).

It is difficult to tell for sure though as t5616-partial-clone.sh was
added after that commit.

Best,
Christian.

Re: [PATCH] strbuf_read_file(): preserve errno across close() call

[Jeff King]

On Fri, Feb 23, 2018 at 10:00:24PM +0100, René Scharfe wrote:

> How about adding a stealthy close_no_errno(), or do something like the
> following to get shorter and more readable code?  (We could also keep
> a single close() call, but would then set errno even on success.)
> [...]
> @@ -391,7 +393,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t 
> hint)
>  
>   if (got < 0) {
>   if (oldalloc == 0)
> - strbuf_release(sb);
> + IGNORE_ERROR(strbuf_release(sb));
>   else
>   strbuf_setlen(sb, oldlen);
>   return -1;

I dunno, that may be crossing the line of "too magical".

I had envisioned something like:

diff --git a/strbuf.c b/strbuf.c
index 5f138ed3c8..0790dd7bcb 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -365,6 +365,14 @@ void strbuf_addbuf_percentquote(struct strbuf *dst, const 
struct strbuf *src)
}
 }
 
+/* release, but preserve errno */
+static void strbuf_release_careful(struct strbuf *sb)
+{
+   int saved_errno = errno;
+   strbuf_release(sb);
+   errno = saved_errno;
+}
+
 size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 {
size_t res;
@@ -375,7 +383,7 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
if (res > 0)
strbuf_setlen(sb, sb->len + res);
else if (oldalloc == 0)
-   strbuf_release(sb);
+   strbuf_release_careful(sb);
return res;
 }
 
@@ -391,7 +399,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
if (got < 0) {
if (oldalloc == 0)
-   strbuf_release(sb);
+   strbuf_release_careful(sb);
else
strbuf_setlen(sb, oldlen);
return -1;
@@ -416,7 +424,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t 
hint)
if (cnt > 0)
strbuf_setlen(sb, sb->len + cnt);
else if (oldalloc == 0)
-   strbuf_release(sb);
+   strbuf_release_careful(sb);
return cnt;
 }
 
@@ -482,7 +490,7 @@ int strbuf_getcwd(struct strbuf *sb)
break;
}
if (oldalloc == 0)
-   strbuf_release(sb);
+   strbuf_release_careful(sb);
else
strbuf_reset(sb);
return -1;


but that solution is definitely very specific to these cases. I also had
a feeling I should be able to shove the "oldalloc" logic into the
helper, too, but there are too many different behaviors in the "else"
block.

-Peff

Re: [PATCH 08/27] pack: move approximate object count to object store

[Jeff King]

On Fri, Feb 23, 2018 at 02:22:14PM -0800, Stefan Beller wrote:

> >> + /*
> >> +  * A fast, rough count of the number of objects in the repository.
> >> +  * These two fields are not meant for direct access. Use
> >> +  * approximate_object_count() instead.
> >> +  */
> >> + unsigned long approximate_object_count;
> >> + unsigned approximate_object_count_valid : 1;
> >
> > Patch looks fine and is effectively a no-op, though what is the need for
> > both of these variables?  Maybe it can be simplified down to just use
> > one?  Just musing as its out of the scope of this patch and we probably
> > shouldn't try to change that in this series.
> 
> I agree we should. It was introduced in e323de4ad7f (sha1_file:
> allow sha1_loose_object_info to handle arbitrary repositories, 2017-08-30)
> and I think it was seen as a clever optimization trick back then?

I think you meant 8e3f52d778 (find_unique_abbrev: move logic out of
get_short_sha1(), 2016-10-03)?

Yes, it was just to avoid the dual-meaning of "0" for "not set" and a
repository with no packfiles.  It would probably be fine to get rid of
it. If you have no packfiles then you probably don't have enough objects
to worry about micro-optimizing. And anyway, the "wasted" case wouldn't
even make any syscalls (it would do a noop prepare_packed_git() and then
realize the packed_git list is empty).

-Peff