Fallout from azure-pipelines-msvc

We have just introduced the helper index_pos_to_insert_pos() to help avoiding underflows when returning -1 - pos for cases where we want to return an insert position, using the ones' complement (as int).

We do not want to have it for *all* cases, where we return -1 - pos, but only for those cases, where the result was actually encoded by index_pos_to_insert_pos().

That excludes all cases where the argument is
derived from index_name_pos(), and leaves just...

rerere.c static struct rerere_dir *find_rerere_dir(const char *hex) · line 157

Quoted 8 lines

rerere.c +1 -1

@@ -154,7 +154,7 @@ static struct rerere_dir *find_rerere_dir(const char *hex)
 		rr_dir->status = NULL;
 		rr_dir->status_nr = 0;
 		rr_dir->status_alloc = 0;
-		pos = -1 - pos;
+		pos = insert_pos_to_index_pos(pos);

... this one...

sha1-name.c static void find_short_object_filename(struct disambiguate_state *ds) · line 101

Quoted 13 lines

rerere.c +0 -0

 		/* Make sure the array is big enough ... */
 		ALLOC_GROW(rerere_dir, rerere_dir_nr + 1, rerere_dir_alloc);

sha1-name.c +1 -1

index 49855ad24f..bee7ce39ee 100644
@@ -98,7 +98,7 @@ static void find_short_object_filename(struct disambiguate_state *ds)
 		loose_objects = odb_loose_cache(odb, &ds->bin_pfx);
 		pos = oid_array_lookup(loose_objects, &ds->bin_pfx);
 		if (pos < 0)
-			pos = -1 - pos;
+			pos = insert_pos_to_index_pos(pos);

... and this one.

sha1-name.c static void find_short_object_filename(struct disambiguate_state *ds) · line 104

Quoted 3 lines

sha1-name.c +0 -0

 		while (!ds->ambiguous && pos < loose_objects->nr) {
 			const struct object_id *oid;
 			oid = loose_objects->oid + pos;

-- Hannes

Quoted 3 lines · Johannes Sixt

We do not want to have it for *all* cases, where we return -1 - pos, but only for those cases, where the result was actually encoded by index_pos_to_insert_pos().

Yup, I agree with you that decoder should be fed only the data emitted by the encoder.

But shouldn't the code that yielded 'pos' that later gets decoded by computing "-1 -pos" without using the encoding helper be corrected to use the encoder instead?

After all, the primary purpose of
inventing the encoder was to catch the arith overflow, wasn't it?

Am 09.10.19 um 03:19 schrieb Junio C Hamano:

Quoted 4 lines

Johannes Sixt <[email protected]> writes:

After all, the primary purpose of
inventing the encoder was to catch the arith overflow, wasn't it?

That was *your* motivation for the helper function. But IMO it is a wrong design decision.

Whether or not the index calculation overflows is a matter of the type that is used for the index, and that in turn is dicated by the possible sizes of the collections that are indexed. IOW, the index overflow check is (*if* it is necessary) a policy decision that must be made at a higher level and must not be hidden away in a helper function whose purpose (as suggested by its name) is something entirely different.

Unless, of course, we declare "all our indexes are of type int". But that ship has sailed long ago, because there are too many cases where we are forced to use size_t as index (strlen, sizeof...).

Meta note: We know that we are painting a tiny shed here (Replacing a one-liner by a one-liner, huh?) If anyone of you has better things to do, please move on. My interest in this discussion are just the design decisions that are made, not the actual outcome of this particular case.

-- Hannes

FWIW I actually agree with Junio about the helper, but in hindsight I could have used a better name (not one that is tied to the "index"). Something like unsigned_one_complement(). But of course, that would say _what_ it does, not _why_.

Quoted 4 lines · Johannes Schindelin

FWIW I actually agree with Junio about the helper, but in hindsight I could have used a better name (not one that is tied to the "index"). Something like unsigned_one_complement(). But of course, that would say _what_ it does, not _why_.

I personally feel that the particular name is on the better side of the borderline. "st_add3(a, b, c)" says it is about adding three size_t quantities, without saying why it exists and should be used over a+b+c. Existence of the helper and calling it alone should be a good enough sign that we somehow feel a+b+c is not sufficient [ly safe], so we do not call it st_add3_safe() or st_add3_wo_overflow().

Your unsigned-one-complement would fall into the same category, no?

Quoted 14 lines · Junio C Hamano

Quoted 4 lines · Johannes Schindelin

FWIW I actually agree with Junio about the helper, but in hindsight I could have used a better name (not one that is tied to the "index"). Something like unsigned_one_complement(). But of course, that would say _what_ it does, not _why_.

I personally feel that the particular name is on the better side of the borderline. "st_add3(a, b, c)" says it is about adding three size_t quantities, without saying why it exists and should be used over a+b+c. Existence of the helper and calling it alone should be a good enough sign that we somehow feel a+b+c is not sufficient [ly safe], so we do not call it st_add3_safe() or st_add3_wo_overflow().

Quoted 1 line

Your unsigned-one-complement would fall into the same category, no?

Yes. That's what I meant to say with the "what vs why" argument.

Quoted 3 lines · Johannes Sixt

We do not want to have it for *all* cases, where we return -1 - pos, but only for those cases, where the result was actually encoded by index_pos_to_insert_pos().

Yup, I agree with you that decoder should be fed only the data emitted by the encoder.

But shouldn't the code that yielded 'pos' that later gets decoded by computing "-1 -pos" without using the encoding helper be corrected to use the encoder instead? After all, the primary purpose of inventing the encoder was to catch the arith overflow, wasn't it?

Quoted 36 lines

That excludes all cases where the argument is
derived from index_name_pos(), and leaves just...

rerere.c static struct rerere_dir *find_rerere_dir(const char *hex) · line 157

Quoted 8 lines

rerere.c +1 -1

@@ -154,7 +154,7 @@ static struct rerere_dir *find_rerere_dir(const char *hex)
 		rr_dir->status = NULL;
 		rr_dir->status_nr = 0;
 		rr_dir->status_alloc = 0;
-		pos = -1 - pos;
+		pos = insert_pos_to_index_pos(pos);

... this one...

sha1-name.c static void find_short_object_filename(struct disambiguate_state *ds) · line 101

Quoted 13 lines

rerere.c +0 -0

 		/* Make sure the array is big enough ... */
 		ALLOC_GROW(rerere_dir, rerere_dir_nr + 1, rerere_dir_alloc);

sha1-name.c +1 -1

index 49855ad24f..bee7ce39ee 100644
@@ -98,7 +98,7 @@ static void find_short_object_filename(struct disambiguate_state *ds)
 		loose_objects = odb_loose_cache(odb, &ds->bin_pfx);
 		pos = oid_array_lookup(loose_objects, &ds->bin_pfx);
 		if (pos < 0)
-			pos = -1 - pos;
+			pos = insert_pos_to_index_pos(pos);

... and this one.

sha1-name.c static void find_short_object_filename(struct disambiguate_state *ds) · line 104

Quoted 3 lines

sha1-name.c +0 -0

 		while (!ds->ambiguous && pos < loose_objects->nr) {
 			const struct object_id *oid;
 			oid = loose_objects->oid + pos;

-- Hannes

Am 08.10.19 um 08:48 schrieb Johannes Schindelin via GitGitGadget:

Quoted 3 lines

We have just introduced the helper index_pos_to_insert_pos() to help avoiding underflows when returning -1 - pos for cases where we want to return an insert position, using the ones' complement (as int).

We do not want to have it for *all* cases, where we return -1 - pos, but only for those cases, where the result was actually encoded by index_pos_to_insert_pos(). That excludes all cases where the argument is derived from index_name_pos(), and leaves just...

rerere.c static struct rerere_dir *find_rerere_dir(const char *hex) · line 157

Quoted 8 lines

rerere.c +1 -1

@@ -154,7 +154,7 @@ static struct rerere_dir *find_rerere_dir(const char *hex)
 		rr_dir->status = NULL;
 		rr_dir->status_nr = 0;
 		rr_dir->status_alloc = 0;
-		pos = -1 - pos;
+		pos = insert_pos_to_index_pos(pos);

... this one...

Quoted 9 lines

rerere.c +0 -0

 		/* Make sure the array is big enough ... */
 		ALLOC_GROW(rerere_dir, rerere_dir_nr + 1, rerere_dir_alloc);

sha1-name.c +0 -0

index 49855ad24f..bee7ce39ee 100644

sha1-name.c static void find_short_object_filename(struct disambiguate_state *ds) · line 101

Quoted 5 lines

sha1-name.c +1 -1

@@ -98,7 +98,7 @@ static void find_short_object_filename(struct disambiguate_state *ds)
 		loose_objects = odb_loose_cache(odb, &ds->bin_pfx);
 		pos = oid_array_lookup(loose_objects, &ds->bin_pfx);
 		if (pos < 0)
-			pos = -1 - pos;
+			pos = insert_pos_to_index_pos(pos);

... and this one.

sha1-name.c static void find_short_object_filename(struct disambiguate_state *ds) · line 104

Quoted 4 lines

sha1-name.c +0 -0

 		while (!ds->ambiguous && pos < loose_objects->nr) {
 			const struct object_id *oid;
 			oid = loose_objects->oid + pos;

-- Hannes