diff mbox series

ds/sparse-cone, was Re: What's cooking in git.git (Oct 2019, #03; Fri, 11)

Message ID nycvar.QRO.7.76.6.1910122327250.3272@tvgsbejvaqbjf.bet (mailing list archive)
State New, archived
Headers show
Series ds/sparse-cone, was Re: What's cooking in git.git (Oct 2019, #03; Fri, 11) | expand

Commit Message

Johannes Schindelin Oct. 12, 2019, 9:31 p.m. UTC 
Hi Junio,

On Fri, 11 Oct 2019, Junio C Hamano wrote:

> * ds/sparse-cone (2019-10-08) 17 commits
>  - sparse-checkout: cone mode should not interact with .gitignore
>  - sparse-checkout: write using lockfile
>  - sparse-checkout: update working directory in-process
>  - sparse-checkout: sanitize for nested folders
>  - read-tree: show progress by default
>  - unpack-trees: add progress to clear_ce_flags()
>  - unpack-trees: hash less in cone mode
>  - sparse-checkout: init and set in cone mode
>  - sparse-checkout: use hashmaps for cone patterns
>  - sparse-checkout: add 'cone' mode
>  - trace2: add region in clear_ce_flags
>  - sparse-checkout: create 'disable' subcommand
>  - sparse-checkout: add '--stdin' option to set subcommand
>  - sparse-checkout: 'set' subcommand
>  - clone: add --sparse mode
>  - sparse-checkout: create 'init' subcommand
>  - sparse-checkout: create builtin with 'list' subcommand
>
>  Management of sparsely checked-out working tree has gained a
>  dedicated "sparse-checkout" command.
>
>  Seems not to play well with the hashmap updates.

Hrm. I had sent out links to the three fixups needed to make the build
green:

https://public-inbox.org/git/nycvar.QRO.7.76.6.1910081055210.46@tvgsbejvaqbjf.bet/

In particular, the patches to squash were:

https://github.com/git-for-windows/git/commit/f74259754971b427a14e6290681e18950824b99d
https://github.com/git-for-windows/git/commit/124c8bc08e974e76ca7d956dc07eb288e71d639e
https://github.com/git-for-windows/git/commit/45948433d1b48ff513fbd37f134c0f1491c78192

For your convenience:

-- snip --
-- snap --

Why not pick it up and squash it into the merge commit?

Ciao,
Dscho

Comments

Eric Wong Oct. 15, 2019, 1:50 a.m. UTC | #1 
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Fri, 11 Oct 2019, Junio C Hamano wrote:
> > * ds/sparse-cone (2019-10-08) 17 commits
> >  - sparse-checkout: cone mode should not interact with .gitignore
> >  - sparse-checkout: write using lockfile
> >  - sparse-checkout: update working directory in-process
> >  - sparse-checkout: sanitize for nested folders
> >  - read-tree: show progress by default
> >  - unpack-trees: add progress to clear_ce_flags()
> >  - unpack-trees: hash less in cone mode
> >  - sparse-checkout: init and set in cone mode
> >  - sparse-checkout: use hashmaps for cone patterns
> >  - sparse-checkout: add 'cone' mode
> >  - trace2: add region in clear_ce_flags
> >  - sparse-checkout: create 'disable' subcommand
> >  - sparse-checkout: add '--stdin' option to set subcommand
> >  - sparse-checkout: 'set' subcommand
> >  - clone: add --sparse mode
> >  - sparse-checkout: create 'init' subcommand
> >  - sparse-checkout: create builtin with 'list' subcommand
> >
> >  Management of sparsely checked-out working tree has gained a
> >  dedicated "sparse-checkout" command.
> >
> >  Seems not to play well with the hashmap updates.
> 
> Hrm. I had sent out links to the three fixups needed to make the build
> green:
> 
> https://public-inbox.org/git/nycvar.QRO.7.76.6.1910081055210.46@tvgsbejvaqbjf.bet/
> 
> In particular, the patches to squash were:
> 
> https://github.com/git-for-windows/git/commit/f74259754971b427a14e6290681e18950824b99d
> https://github.com/git-for-windows/git/commit/124c8bc08e974e76ca7d956dc07eb288e71d639e
> https://github.com/git-for-windows/git/commit/45948433d1b48ff513fbd37f134c0f1491c78192

> diff --git a/dir.c b/dir.c
> index 0135f9e2180..9efcdc9aacd 100644
> --- a/dir.c
> +++ b/dir.c

<snip>
> @@ -706,8 +710,8 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern
> 
>  clear_hashmaps:
>  	warning(_("disabling cone pattern matching"));
> -	hashmap_free(&pl->parent_hashmap, 1);
> -	hashmap_free(&pl->recursive_hashmap, 1);
> +	hashmap_free(&pl->parent_hashmap);
> +	hashmap_free(&pl->recursive_hashmap);

I just took a brief look, but that appears to leak memory.

"hashmap_free(var, 1)" should be replaced with
"hashmap_free_entries(var, struct foo, member)"

Only "hashmap_free(var, 0)" can become "hashmap_free(var)"
Junio C Hamano Oct. 15, 2019, 3:20 a.m. UTC | #2 
Eric Wong <e@80x24.org> writes:

> I just took a brief look, but that appears to leak memory.
>
> "hashmap_free(var, 1)" should be replaced with
> "hashmap_free_entries(var, struct foo, member)"
>
> Only "hashmap_free(var, 0)" can become "hashmap_free(var)"

I deliberately avoided merge-time band-aid fixups on this topic and
ew/hashmap exactly because I was sure that I'd introduce a similar
bugs by doing so myself.  Using evil merges can be a great way to
help multiple topics polished independently at the same time, but
when overused, can hide this kind of gotchas quite easily.

A reroll on top of ew/hashmap would be desirable, now that topic is
ready for 'master'.

Thanks.
Eric Wong Oct. 15, 2019, 7:11 a.m. UTC | #3 
Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
> 
> > I just took a brief look, but that appears to leak memory.
> >
> > "hashmap_free(var, 1)" should be replaced with
> > "hashmap_free_entries(var, struct foo, member)"
> >
> > Only "hashmap_free(var, 0)" can become "hashmap_free(var)"
> 
> I deliberately avoided merge-time band-aid fixups on this topic and
> ew/hashmap exactly because I was sure that I'd introduce a similar
> bugs by doing so myself.  Using evil merges can be a great way to
> help multiple topics polished independently at the same time, but
> when overused, can hide this kind of gotchas quite easily.
> 
> A reroll on top of ew/hashmap would be desirable, now that topic is
> ready for 'master'.

Just to be clear, that reroll should come from Stolee (+Cc-ed), right?
I'll be around to help answer questions, but also pretty busy
with other stuff and I think Stolee knows this API pretty well :>
Derrick Stolee Oct. 15, 2019, 12:54 p.m. UTC | #4 
On 10/15/2019 3:11 AM, Eric Wong wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Wong <e@80x24.org> writes:
>>
>>> I just took a brief look, but that appears to leak memory.
>>>
>>> "hashmap_free(var, 1)" should be replaced with
>>> "hashmap_free_entries(var, struct foo, member)"
>>>
>>> Only "hashmap_free(var, 0)" can become "hashmap_free(var)"
>>
>> I deliberately avoided merge-time band-aid fixups on this topic and
>> ew/hashmap exactly because I was sure that I'd introduce a similar
>> bugs by doing so myself.  Using evil merges can be a great way to
>> help multiple topics polished independently at the same time, but
>> when overused, can hide this kind of gotchas quite easily.
>>
>> A reroll on top of ew/hashmap would be desirable, now that topic is
>> ready for 'master'.
> 
> Just to be clear, that reroll should come from Stolee (+Cc-ed), right?
> I'll be around to help answer questions, but also pretty busy
> with other stuff and I think Stolee knows this API pretty well :>

I'm working on the re-roll, yes. I was waiting for ew/hashmap to merge
with history that included ds/include-exclude. Now the current 'master'
has both, so I can rebase and check everything carefully. v4 should
have every commit compile with the new hashmap API.

Thanks for pointing out the bugs with the suggested fixups. I'll
be careful as I adapt the new API.

-Stolee
Junio C Hamano Oct. 16, 2019, 1:21 a.m. UTC | #5 
Derrick Stolee <stolee@gmail.com> writes:

> On 10/15/2019 3:11 AM, Eric Wong wrote:
>> Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Wong <e@80x24.org> writes:
>>>
>>>> I just took a brief look, but that appears to leak memory.
>>>>
>>>> "hashmap_free(var, 1)" should be replaced with
>>>> "hashmap_free_entries(var, struct foo, member)"
>>>>
>>>> Only "hashmap_free(var, 0)" can become "hashmap_free(var)"
>>>
>>> I deliberately avoided merge-time band-aid fixups on this topic and
>>> ew/hashmap exactly because I was sure that I'd introduce a similar
>>> bugs by doing so myself.  Using evil merges can be a great way to
>>> help multiple topics polished independently at the same time, but
>>> when overused, can hide this kind of gotchas quite easily.
>>>
>>> A reroll on top of ew/hashmap would be desirable, now that topic is
>>> ready for 'master'.
>> 
>> Just to be clear, that reroll should come from Stolee (+Cc-ed), right?
>> I'll be around to help answer questions, but also pretty busy
>> with other stuff and I think Stolee knows this API pretty well :>
>
> I'm working on the re-roll, yes. I was waiting for ew/hashmap to merge
> with history that included ds/include-exclude. Now the current 'master'
> has both, so I can rebase and check everything carefully. v4 should
> have every commit compile with the new hashmap API.

Thanks, both.
diff mbox series

Patch

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 160afb2fd7f..fb21d6f8780 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -229,9 +229,9 @@  static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat

 	e->patternlen = path->len;
 	e->pattern = strbuf_detach(path, NULL);
-	hashmap_entry_init(e, memhash(e->pattern, e->patternlen));
+	hashmap_entry_init(&e->ent, memhash(e->pattern, e->patternlen));

-	hashmap_add(&pl->recursive_hashmap, e);
+	hashmap_add(&pl->recursive_hashmap, &e->ent);

 	while (e->patternlen) {
 		char *slash = strrchr(e->pattern, '/');
@@ -245,24 +245,26 @@  static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat
 		e = xmalloc(sizeof(struct pattern_entry));
 		e->patternlen = newlen;
 		e->pattern = xstrndup(oldpattern, newlen);
-		hashmap_entry_init(e, memhash(e->pattern, e->patternlen));
+		hashmap_entry_init(&e->ent, memhash(e->pattern, e->patternlen));

-		if (!hashmap_get(&pl->parent_hashmap, e, NULL))
-			hashmap_add(&pl->parent_hashmap, e);
+		if (!hashmap_get_entry(&pl->parent_hashmap, e, ent, NULL))
+			hashmap_add(&pl->parent_hashmap, &e->ent);
 	}
 }

 static void write_cone_to_file(FILE *fp, struct pattern_list *pl)
 {
 	int i;
-	struct pattern_entry *entry;
+	struct hashmap_entry *e;
 	struct hashmap_iter iter;
 	struct string_list sl = STRING_LIST_INIT_DUP;
 	struct strbuf parent_pattern = STRBUF_INIT;

 	hashmap_iter_init(&pl->parent_hashmap, &iter);
-	while ((entry = hashmap_iter_next(&iter))) {
-		if (hashmap_get(&pl->recursive_hashmap, entry, NULL))
+	while ((e = hashmap_iter_next(&iter))) {
+		struct pattern_entry *entry =
+			container_of(e, struct pattern_entry, ent);
+		if (hashmap_get_entry(&pl->recursive_hashmap, entry, ent, NULL))
 			continue;

 		if (!hashmap_contains_parent(&pl->recursive_hashmap,
@@ -286,7 +288,9 @@  static void write_cone_to_file(FILE *fp, struct pattern_list *pl)
 	string_list_clear(&sl, 0);

 	hashmap_iter_init(&pl->recursive_hashmap, &iter);
-	while ((entry = hashmap_iter_next(&iter))) {
+	while ((e = hashmap_iter_next(&iter))) {
+		struct pattern_entry *entry =
+			container_of(e, struct pattern_entry, ent);
 		if (!hashmap_contains_parent(&pl->recursive_hashmap,
 					     entry->pattern,
 					     &parent_pattern))
diff --git a/dir.c b/dir.c
index 0135f9e2180..9efcdc9aacd 100644
--- a/dir.c
+++ b/dir.c
@@ -612,10 +612,13 @@  void parse_path_pattern(const char **pattern,
 }

 int pl_hashmap_cmp(const void *unused_cmp_data,
-		   const void *a, const void *b, const void *key)
+		   const struct hashmap_entry *a, const struct hashmap_entry *b,
+		   const void *key)
 {
-	const struct pattern_entry *ee1 = (const struct pattern_entry *)a;
-	const struct pattern_entry *ee2 = (const struct pattern_entry *)b;
+	const struct pattern_entry *ee1 =
+		container_of(a, struct pattern_entry, ent);
+	const struct pattern_entry *ee2 =
+		container_of(b, struct pattern_entry, ent);

 	size_t min_len = ee1->patternlen <= ee2->patternlen
 			 ? ee1->patternlen
@@ -660,10 +663,11 @@  static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern
 		translated = xmalloc(sizeof(struct pattern_entry));
 		translated->pattern = truncated;
 		translated->patternlen = given->patternlen - 2;
-		hashmap_entry_init(translated,
+		hashmap_entry_init(&translated->ent,
 				   memhash(translated->pattern, translated->patternlen));

-		if (!hashmap_get(&pl->recursive_hashmap, translated, NULL)) {
+		if (!hashmap_get_entry(&pl->recursive_hashmap,
+				       translated, ent, NULL)) {
 			/* We did not see the "parent" included */
 			warning(_("unrecognized negative pattern: '%s'"),
 				given->pattern);
@@ -672,8 +676,8 @@  static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern
 			goto clear_hashmaps;
 		}

-		hashmap_add(&pl->parent_hashmap, translated);
-		hashmap_remove(&pl->recursive_hashmap, translated, &data);
+		hashmap_add(&pl->parent_hashmap, &translated->ent);
+		hashmap_remove(&pl->recursive_hashmap, &translated->ent, &data);
 		free(data);
 		return;
 	}
@@ -688,16 +692,16 @@  static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern

 	translated->pattern = xstrdup(given->pattern);
 	translated->patternlen = given->patternlen;
-	hashmap_entry_init(translated,
+	hashmap_entry_init(&translated->ent,
 			   memhash(translated->pattern, translated->patternlen));

-	hashmap_add(&pl->recursive_hashmap, translated);
+	hashmap_add(&pl->recursive_hashmap, &translated->ent);

-	if (hashmap_get(&pl->parent_hashmap, translated, NULL)) {
+	if (hashmap_get_entry(&pl->parent_hashmap, translated, ent, NULL)) {
 		/* we already included this at the parent level */
 		warning(_("your sparse-checkout file may have issues: pattern '%s' is repeated"),
 			given->pattern);
-		hashmap_remove(&pl->parent_hashmap, translated, &data);
+		hashmap_remove(&pl->parent_hashmap, &translated->ent, &data);
 		free(data);
 		free(translated);
 	}
@@ -706,8 +710,8 @@  static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern

 clear_hashmaps:
 	warning(_("disabling cone pattern matching"));
-	hashmap_free(&pl->parent_hashmap, 1);
-	hashmap_free(&pl->recursive_hashmap, 1);
+	hashmap_free(&pl->parent_hashmap);
+	hashmap_free(&pl->recursive_hashmap);
 	pl->use_cone_patterns = 0;
 }

@@ -719,8 +723,8 @@  static int hashmap_contains_path(struct hashmap *map,
 	/* Check straight mapping */
 	p.pattern = pattern->buf;
 	p.patternlen = pattern->len;
-	hashmap_entry_init(&p, memhash(p.pattern, p.patternlen));
-	return !!hashmap_get(map, &p, NULL);
+	hashmap_entry_init(&p.ent, memhash(p.pattern, p.patternlen));
+	return !!hashmap_get_entry(map, &p, ent, NULL);
 }

 int hashmap_contains_parent(struct hashmap *map,
diff --git a/dir.h b/dir.h
index 09b7c4c44be..81d9f1b0a4e 100644
--- a/dir.h
+++ b/dir.h
@@ -301,7 +301,8 @@  int is_excluded(struct dir_struct *dir,
 		const char *name, int *dtype);

 int pl_hashmap_cmp(const void *unused_cmp_data,
-		   const void *a, const void *b, const void *key);
+		   const struct hashmap_entry *a, const struct hashmap_entry *b,
+		   const void *key);
 int hashmap_contains_parent(struct hashmap *map,
 			    const char *path,
 			    struct strbuf *buffer);