git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH v3 1/2] pack-objects: move read mutex to packing_data struct
@ 2019-01-24  1:05 Patrick Hogg
  2019-01-24  1:05 ` [PATCH v3 2/2] pack-objects: merge read_lock and lock in " Patrick Hogg
  2019-01-24  8:35 ` [PATCH v3 1/2] pack-objects: move read mutex to " Duy Nguyen
  0 siblings, 2 replies; 6+ messages in thread
From: Patrick Hogg @ 2019-01-24  1:05 UTC (permalink / raw)
  To: git; +Cc: gitster, pclouds, Johannes.Schindelin, peff, newren, Patrick Hogg

ac77d0c37 ("pack-objects: shrink size field in struct object_entry",
2018-04-14) added an extra usage of read_lock/read_unlock in the newly
introduced oe_get_size_slow for thread safety in parallel calls to
try_delta(). Unfortunately oe_get_size_slow is also used in serial
code, some of which is called before the first invocation of
ll_find_deltas. As such the read mutex is not guaranteed to be
initialized.

Resolve this by moving the read mutex to packing_data and initializing
it in prepare_packing_data which is initialized in cmd_pack_objects.

Signed-off-by: Patrick Hogg <phogg@novamoon.net>
---
 builtin/pack-objects.c |  7 ++-----
 pack-objects.c         |  1 +
 pack-objects.h         | 10 ++++++++++
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 411aefd68..506061b4c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1954,9 +1954,8 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 }
 
 /* Protect access to object database */
-static pthread_mutex_t read_mutex;
-#define read_lock()		pthread_mutex_lock(&read_mutex)
-#define read_unlock()		pthread_mutex_unlock(&read_mutex)
+#define read_lock()		packing_data_read_lock(&to_pack)
+#define read_unlock()		packing_data_read_unlock(&to_pack)
 
 /* Protect delta_cache_size */
 static pthread_mutex_t cache_mutex;
@@ -2381,7 +2380,6 @@ static pthread_cond_t progress_cond;
  */
 static void init_threaded_search(void)
 {
-	init_recursive_mutex(&read_mutex);
 	pthread_mutex_init(&cache_mutex, NULL);
 	pthread_mutex_init(&progress_mutex, NULL);
 	pthread_cond_init(&progress_cond, NULL);
@@ -2392,7 +2390,6 @@ static void cleanup_threaded_search(void)
 {
 	set_try_to_free_routine(old_try_to_free_routine);
 	pthread_cond_destroy(&progress_cond);
-	pthread_mutex_destroy(&read_mutex);
 	pthread_mutex_destroy(&cache_mutex);
 	pthread_mutex_destroy(&progress_mutex);
 }
diff --git a/pack-objects.c b/pack-objects.c
index b6cdbb016..3554c43ac 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -150,6 +150,7 @@ void prepare_packing_data(struct packing_data *pdata)
 						   1UL << OE_DELTA_SIZE_BITS);
 #ifndef NO_PTHREADS
 	pthread_mutex_init(&pdata->lock, NULL);
+	init_recursive_mutex(&pdata->read_lock);
 #endif
 }
 
diff --git a/pack-objects.h b/pack-objects.h
index dc869f26c..0a038e3bc 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -146,6 +146,7 @@ struct packing_data {
 	struct packed_git **in_pack;
 
 	pthread_mutex_t lock;
+	pthread_mutex_t read_lock;
 
 	/*
 	 * This list contains entries for bases which we know the other side
@@ -174,6 +175,15 @@ static inline void packing_data_unlock(struct packing_data *pdata)
 	pthread_mutex_unlock(&pdata->lock);
 }
 
+static inline void packing_data_read_lock(struct packing_data *pdata)
+{
+	pthread_mutex_lock(&pdata->read_lock);
+}
+static inline void packing_data_read_unlock(struct packing_data *pdata)
+{
+	pthread_mutex_unlock(&pdata->read_lock);
+}
+
 struct object_entry *packlist_alloc(struct packing_data *pdata,
 				    const unsigned char *sha1,
 				    uint32_t index_pos);
-- 
2.20.1.windows.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/2] pack-objects: merge read_lock and lock in packing_data struct
  2019-01-24  1:05 [PATCH v3 1/2] pack-objects: move read mutex to packing_data struct Patrick Hogg
@ 2019-01-24  1:05 ` Patrick Hogg
  2019-01-24  8:33   ` Duy Nguyen
  2019-01-24  8:35 ` [PATCH v3 1/2] pack-objects: move read mutex to " Duy Nguyen
  1 sibling, 1 reply; 6+ messages in thread
From: Patrick Hogg @ 2019-01-24  1:05 UTC (permalink / raw)
  To: git; +Cc: gitster, pclouds, Johannes.Schindelin, peff, newren, Patrick Hogg

Upgrade the packing_data lock to a recursive mutex to make it suitable
for current read_lock usages. Additionally remove the superfluous
#ifndef NO_PTHREADS guard around mutex initialization in
prepare_packing_data as the mutex functions themselves are already
protected.

Signed-off-by: Patrick Hogg <phogg@novamoon.net>
---
 builtin/pack-objects.c | 24 ++++++++++++------------
 pack-objects.c         |  5 +----
 pack-objects.h         | 10 ----------
 3 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 506061b4c..5439b434c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1954,8 +1954,8 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 }
 
 /* Protect access to object database */
-#define read_lock()		packing_data_read_lock(&to_pack)
-#define read_unlock()		packing_data_read_unlock(&to_pack)
+#define pack_lock()		packing_data_lock(&to_pack)
+#define pack_unlock()		packing_data_unlock(&to_pack)
 
 /* Protect delta_cache_size */
 static pthread_mutex_t cache_mutex;
@@ -1992,11 +1992,11 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
 	unsigned long used, avail, size;
 
 	if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
-		read_lock();
+		pack_lock();
 		if (oid_object_info(the_repository, &e->idx.oid, &size) < 0)
 			die(_("unable to get size of %s"),
 			    oid_to_hex(&e->idx.oid));
-		read_unlock();
+		pack_unlock();
 		return size;
 	}
 
@@ -2004,7 +2004,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
 	if (!p)
 		BUG("when e->type is a delta, it must belong to a pack");
 
-	read_lock();
+	pack_lock();
 	w_curs = NULL;
 	buf = use_pack(p, &w_curs, e->in_pack_offset, &avail);
 	used = unpack_object_header_buffer(buf, avail, &type, &size);
@@ -2013,7 +2013,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
 		    oid_to_hex(&e->idx.oid));
 
 	unuse_pack(&w_curs);
-	read_unlock();
+	pack_unlock();
 	return size;
 }
 
@@ -2075,9 +2075,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 
 	/* Load data if not already done */
 	if (!trg->data) {
-		read_lock();
+		pack_lock();
 		trg->data = read_object_file(&trg_entry->idx.oid, &type, &sz);
-		read_unlock();
+		pack_unlock();
 		if (!trg->data)
 			die(_("object %s cannot be read"),
 			    oid_to_hex(&trg_entry->idx.oid));
@@ -2088,9 +2088,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 		*mem_usage += sz;
 	}
 	if (!src->data) {
-		read_lock();
+		pack_lock();
 		src->data = read_object_file(&src_entry->idx.oid, &type, &sz);
-		read_unlock();
+		pack_unlock();
 		if (!src->data) {
 			if (src_entry->preferred_base) {
 				static int warned = 0;
@@ -2336,9 +2336,9 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
 
 static void try_to_free_from_threads(size_t size)
 {
-	read_lock();
+	pack_lock();
 	release_pack_memory(size);
-	read_unlock();
+	pack_unlock();
 }
 
 static try_to_free_t old_try_to_free_routine;
diff --git a/pack-objects.c b/pack-objects.c
index 3554c43ac..6f32a7ba0 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -148,10 +148,7 @@ void prepare_packing_data(struct packing_data *pdata)
 					     1U << OE_SIZE_BITS);
 	pdata->oe_delta_size_limit = git_env_ulong("GIT_TEST_OE_DELTA_SIZE",
 						   1UL << OE_DELTA_SIZE_BITS);
-#ifndef NO_PTHREADS
-	pthread_mutex_init(&pdata->lock, NULL);
-	init_recursive_mutex(&pdata->read_lock);
-#endif
+	init_recursive_mutex(&pdata->lock);
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,
diff --git a/pack-objects.h b/pack-objects.h
index 0a038e3bc..dc869f26c 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -146,7 +146,6 @@ struct packing_data {
 	struct packed_git **in_pack;
 
 	pthread_mutex_t lock;
-	pthread_mutex_t read_lock;
 
 	/*
 	 * This list contains entries for bases which we know the other side
@@ -175,15 +174,6 @@ static inline void packing_data_unlock(struct packing_data *pdata)
 	pthread_mutex_unlock(&pdata->lock);
 }
 
-static inline void packing_data_read_lock(struct packing_data *pdata)
-{
-	pthread_mutex_lock(&pdata->read_lock);
-}
-static inline void packing_data_read_unlock(struct packing_data *pdata)
-{
-	pthread_mutex_unlock(&pdata->read_lock);
-}
-
 struct object_entry *packlist_alloc(struct packing_data *pdata,
 				    const unsigned char *sha1,
 				    uint32_t index_pos);
-- 
2.20.1.windows.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] pack-objects: merge read_lock and lock in packing_data struct
  2019-01-24  1:05 ` [PATCH v3 2/2] pack-objects: merge read_lock and lock in " Patrick Hogg
@ 2019-01-24  8:33   ` Duy Nguyen
  2019-01-24 19:48     ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Duy Nguyen @ 2019-01-24  8:33 UTC (permalink / raw)
  To: Patrick Hogg
  Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin, Jeff King,
	Elijah Newren

On Thu, Jan 24, 2019 at 8:06 AM Patrick Hogg <phogg@novamoon.net> wrote:
> diff --git a/pack-objects.h b/pack-objects.h
> index 0a038e3bc..dc869f26c 100644
> --- a/pack-objects.h
> +++ b/pack-objects.h
> @@ -146,7 +146,6 @@ struct packing_data {
>         struct packed_git **in_pack;
>
>         pthread_mutex_t lock;
> -       pthread_mutex_t read_lock;

"lock" without any comments in this struct, to me, implies that it
protects access to this struct alone. But since you're using it as
"read lock" (aka access to object database), I think you should add a
comment here clarify the new role of "lock" variable.
-- 
Duy

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] pack-objects: move read mutex to packing_data struct
  2019-01-24  1:05 [PATCH v3 1/2] pack-objects: move read mutex to packing_data struct Patrick Hogg
  2019-01-24  1:05 ` [PATCH v3 2/2] pack-objects: merge read_lock and lock in " Patrick Hogg
@ 2019-01-24  8:35 ` Duy Nguyen
  1 sibling, 0 replies; 6+ messages in thread
From: Duy Nguyen @ 2019-01-24  8:35 UTC (permalink / raw)
  To: Patrick Hogg
  Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin, Jeff King,
	Elijah Newren

On Thu, Jan 24, 2019 at 8:06 AM Patrick Hogg <phogg@novamoon.net> wrote:
>
> ac77d0c37 ("pack-objects: shrink size field in struct object_entry",
> 2018-04-14) added an extra usage of read_lock/read_unlock in the newly
> introduced oe_get_size_slow for thread safety in parallel calls to
> try_delta(). Unfortunately oe_get_size_slow is also used in serial
> code, some of which is called before the first invocation of
> ll_find_deltas. As such the read mutex is not guaranteed to be
> initialized.
>
> Resolve this by moving the read mutex to packing_data and initializing
> it in prepare_packing_data which is initialized in cmd_pack_objects.

Obviously correct.

Reviewed-by: Duy Nguyen <pclouds@gmail.com>

>
> Signed-off-by: Patrick Hogg <phogg@novamoon.net>
> ---
>  builtin/pack-objects.c |  7 ++-----
>  pack-objects.c         |  1 +
>  pack-objects.h         | 10 ++++++++++
>  3 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 411aefd68..506061b4c 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1954,9 +1954,8 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
>  }
>
>  /* Protect access to object database */
> -static pthread_mutex_t read_mutex;
> -#define read_lock()            pthread_mutex_lock(&read_mutex)
> -#define read_unlock()          pthread_mutex_unlock(&read_mutex)
> +#define read_lock()            packing_data_read_lock(&to_pack)
> +#define read_unlock()          packing_data_read_unlock(&to_pack)
>
>  /* Protect delta_cache_size */
>  static pthread_mutex_t cache_mutex;
> @@ -2381,7 +2380,6 @@ static pthread_cond_t progress_cond;
>   */
>  static void init_threaded_search(void)
>  {
> -       init_recursive_mutex(&read_mutex);
>         pthread_mutex_init(&cache_mutex, NULL);
>         pthread_mutex_init(&progress_mutex, NULL);
>         pthread_cond_init(&progress_cond, NULL);
> @@ -2392,7 +2390,6 @@ static void cleanup_threaded_search(void)
>  {
>         set_try_to_free_routine(old_try_to_free_routine);
>         pthread_cond_destroy(&progress_cond);
> -       pthread_mutex_destroy(&read_mutex);
>         pthread_mutex_destroy(&cache_mutex);
>         pthread_mutex_destroy(&progress_mutex);
>  }
> diff --git a/pack-objects.c b/pack-objects.c
> index b6cdbb016..3554c43ac 100644
> --- a/pack-objects.c
> +++ b/pack-objects.c
> @@ -150,6 +150,7 @@ void prepare_packing_data(struct packing_data *pdata)
>                                                    1UL << OE_DELTA_SIZE_BITS);
>  #ifndef NO_PTHREADS
>         pthread_mutex_init(&pdata->lock, NULL);
> +       init_recursive_mutex(&pdata->read_lock);
>  #endif
>  }
>
> diff --git a/pack-objects.h b/pack-objects.h
> index dc869f26c..0a038e3bc 100644
> --- a/pack-objects.h
> +++ b/pack-objects.h
> @@ -146,6 +146,7 @@ struct packing_data {
>         struct packed_git **in_pack;
>
>         pthread_mutex_t lock;
> +       pthread_mutex_t read_lock;
>
>         /*
>          * This list contains entries for bases which we know the other side
> @@ -174,6 +175,15 @@ static inline void packing_data_unlock(struct packing_data *pdata)
>         pthread_mutex_unlock(&pdata->lock);
>  }
>
> +static inline void packing_data_read_lock(struct packing_data *pdata)
> +{
> +       pthread_mutex_lock(&pdata->read_lock);
> +}
> +static inline void packing_data_read_unlock(struct packing_data *pdata)
> +{
> +       pthread_mutex_unlock(&pdata->read_lock);
> +}
> +
>  struct object_entry *packlist_alloc(struct packing_data *pdata,
>                                     const unsigned char *sha1,
>                                     uint32_t index_pos);
> --
> 2.20.1.windows.1
>


-- 
Duy

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] pack-objects: merge read_lock and lock in packing_data struct
  2019-01-24  8:33   ` Duy Nguyen
@ 2019-01-24 19:48     ` Junio C Hamano
  2019-01-25  0:20       ` Patrick Hogg
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2019-01-24 19:48 UTC (permalink / raw)
  To: Duy Nguyen
  Cc: Patrick Hogg, Git Mailing List, Johannes Schindelin, Jeff King,
	Elijah Newren

Duy Nguyen <pclouds@gmail.com> writes:

> On Thu, Jan 24, 2019 at 8:06 AM Patrick Hogg <phogg@novamoon.net> wrote:
>> diff --git a/pack-objects.h b/pack-objects.h
>> index 0a038e3bc..dc869f26c 100644
>> --- a/pack-objects.h
>> +++ b/pack-objects.h
>> @@ -146,7 +146,6 @@ struct packing_data {
>>         struct packed_git **in_pack;
>>
>>         pthread_mutex_t lock;
>> -       pthread_mutex_t read_lock;
>
> "lock" without any comments in this struct, to me, implies that it
> protects access to this struct alone. But since you're using it as
> "read lock" (aka access to object database), I think you should add a
> comment here clarify the new role of "lock" variable.

Sounds sensible.  How about squashing something like this in?

Some older part of this file still tries to hide the reliance on the
global variable "to_pack", but newer code refers to it already, and
I think it no longer is buying us much.

 builtin/pack-objects.c | 24 ++++++++++--------------
 pack-objects.c         |  2 +-
 pack-objects.h         | 11 ++++++++---
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 5439b434c6..cfef48217f 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1953,10 +1953,6 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,
 	return 0;
 }
 
-/* Protect access to object database */
-#define pack_lock()		packing_data_lock(&to_pack)
-#define pack_unlock()		packing_data_unlock(&to_pack)
-
 /* Protect delta_cache_size */
 static pthread_mutex_t cache_mutex;
 #define cache_lock()		pthread_mutex_lock(&cache_mutex)
@@ -1992,11 +1988,11 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
 	unsigned long used, avail, size;
 
 	if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
-		pack_lock();
+		packing_data_lock(&to_pack);
 		if (oid_object_info(the_repository, &e->idx.oid, &size) < 0)
 			die(_("unable to get size of %s"),
 			    oid_to_hex(&e->idx.oid));
-		pack_unlock();
+		packing_data_unlock(&to_pack);
 		return size;
 	}
 
@@ -2004,7 +2000,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
 	if (!p)
 		BUG("when e->type is a delta, it must belong to a pack");
 
-	pack_lock();
+	packing_data_lock(&to_pack);
 	w_curs = NULL;
 	buf = use_pack(p, &w_curs, e->in_pack_offset, &avail);
 	used = unpack_object_header_buffer(buf, avail, &type, &size);
@@ -2013,7 +2009,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
 		    oid_to_hex(&e->idx.oid));
 
 	unuse_pack(&w_curs);
-	pack_unlock();
+	packing_data_unlock(&to_pack);
 	return size;
 }
 
@@ -2075,9 +2071,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 
 	/* Load data if not already done */
 	if (!trg->data) {
-		pack_lock();
+		packing_data_lock(&to_pack);
 		trg->data = read_object_file(&trg_entry->idx.oid, &type, &sz);
-		pack_unlock();
+		packing_data_unlock(&to_pack);
 		if (!trg->data)
 			die(_("object %s cannot be read"),
 			    oid_to_hex(&trg_entry->idx.oid));
@@ -2088,9 +2084,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 		*mem_usage += sz;
 	}
 	if (!src->data) {
-		pack_lock();
+		packing_data_lock(&to_pack);
 		src->data = read_object_file(&src_entry->idx.oid, &type, &sz);
-		pack_unlock();
+		packing_data_unlock(&to_pack);
 		if (!src->data) {
 			if (src_entry->preferred_base) {
 				static int warned = 0;
@@ -2336,9 +2332,9 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,
 
 static void try_to_free_from_threads(size_t size)
 {
-	pack_lock();
+	packing_data_lock(&to_pack);
 	release_pack_memory(size);
-	pack_unlock();
+	packing_data_unlock(&to_pack);
 }
 
 static try_to_free_t old_try_to_free_routine;
diff --git a/pack-objects.c b/pack-objects.c
index 6f32a7ba04..a1dc5eb726 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -148,7 +148,7 @@ void prepare_packing_data(struct packing_data *pdata)
 					     1U << OE_SIZE_BITS);
 	pdata->oe_delta_size_limit = git_env_ulong("GIT_TEST_OE_DELTA_SIZE",
 						   1UL << OE_DELTA_SIZE_BITS);
-	init_recursive_mutex(&pdata->lock);
+	init_recursive_mutex(&pdata->odb_lock);
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,
diff --git a/pack-objects.h b/pack-objects.h
index dc869f26c2..a0c21959b2 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -145,7 +145,11 @@ struct packing_data {
 	struct packed_git **in_pack_by_idx;
 	struct packed_git **in_pack;
 
-	pthread_mutex_t lock;
+	/* 
+	 * During packing with multiple threads, protect the in-core
+	 * object database from concurrent accesses.
+	 */
+	pthread_mutex_t odb_lock;
 
 	/*
 	 * This list contains entries for bases which we know the other side
@@ -165,13 +169,14 @@ struct packing_data {
 
 void prepare_packing_data(struct packing_data *pdata);
 
+/* Protect access to object database */
 static inline void packing_data_lock(struct packing_data *pdata)
 {
-	pthread_mutex_lock(&pdata->lock);
+	pthread_mutex_lock(&pdata->odb_lock);
 }
 static inline void packing_data_unlock(struct packing_data *pdata)
 {
-	pthread_mutex_unlock(&pdata->lock);
+	pthread_mutex_unlock(&pdata->odb_lock);
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] pack-objects: merge read_lock and lock in packing_data struct
  2019-01-24 19:48     ` Junio C Hamano
@ 2019-01-25  0:20       ` Patrick Hogg
  0 siblings, 0 replies; 6+ messages in thread
From: Patrick Hogg @ 2019-01-25  0:20 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Duy Nguyen, Git Mailing List, Johannes Schindelin, Jeff King,
	Elijah Newren

On Thu, Jan 24, 2019 at 2:49 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > On Thu, Jan 24, 2019 at 8:06 AM Patrick Hogg <phogg@novamoon.net> wrote:
> >> diff --git a/pack-objects.h b/pack-objects.h
> >> index 0a038e3bc..dc869f26c 100644
> >> --- a/pack-objects.h
> >> +++ b/pack-objects.h
> >> @@ -146,7 +146,6 @@ struct packing_data {
> >>         struct packed_git **in_pack;
> >>
> >>         pthread_mutex_t lock;
> >> -       pthread_mutex_t read_lock;
> >
> > "lock" without any comments in this struct, to me, implies that it
> > protects access to this struct alone. But since you're using it as
> > "read lock" (aka access to object database), I think you should add a
> > comment here clarify the new role of "lock" variable.
>
> Sounds sensible.  How about squashing something like this in?
>
> Some older part of this file still tries to hide the reliance on the
> global variable "to_pack", but newer code refers to it already, and
> I think it no longer is buying us much.
>
>  [snip]

Sounds good to me, I'm going to shamelessly use exactly that
(with a Thanks-to:) as I have no better suggestions for comments.

Thanks all for the support in my first ever git.git patch!

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-01-25  0:21 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-24  1:05 [PATCH v3 1/2] pack-objects: move read mutex to packing_data struct Patrick Hogg
2019-01-24  1:05 ` [PATCH v3 2/2] pack-objects: merge read_lock and lock in " Patrick Hogg
2019-01-24  8:33   ` Duy Nguyen
2019-01-24 19:48     ` Junio C Hamano
2019-01-25  0:20       ` Patrick Hogg
2019-01-24  8:35 ` [PATCH v3 1/2] pack-objects: move read mutex to " Duy Nguyen

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).