@matheusbsbs / matheus-bagliano / commits / df3458e957

refs API: make parse_loose_ref_contents() not set errno

Change the parse_loose_ref_contents() function to stop setting "errno" and failure, and to instead pass up a "failure_errno" via a parameter. This requires changing its callers to do the same. The EINVAL error from parse_loose_ref_contents is used in files-backend to create a custom error message. In untangling this we discovered a tricky edge case. The refs_read_special_head() function was relying on parse_loose_ref_contents() setting EINVAL. By converting it to use "saved_errno" we can migrate away from "errno" in this part of the code entirely, and do away with an existing "save_errno" pattern, its only purpose was to not clobber the "errno" we previously needed at the end of files_read_raw_ref(). Let's assert that we can do that by not having files_read_raw_ref() itself operate on *failure_errno in addition to passing it on. Instead we'll assert that if we return non-zero we actually do set errno, thus assuring ourselves and callers that they can trust the resulting "failure_errno". Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Han-Wen Nienhuys committed Oct 16, 2021 at 11:39 UTC df3458e95710b14bff1e5acb830ad77b2a4b0c73

3 files changed +29 -16

refs.c

+5 -3

@@ -1648,7 +1648,8 @@ int for_each_fullref_in_prefixes(const char *namespace,

1648

1649 static int refs_read_special_head(struct ref_store *ref_store,

1650 const char *refname, struct object_id *oid,

1651 - struct strbuf *referent, unsigned int *type)

1651 + struct strbuf *referent, unsigned int *type,

1652 + int *failure_errno)

1653 {

1654 struct strbuf full_path = STRBUF_INIT;

1655 struct strbuf content = STRBUF_INIT;

@@ -1658,7 +1659,8 @@ static int refs_read_special_head(struct ref_store *ref_store,

1659 if (strbuf_read_file(&content, full_path.buf, 0) < 0)

1660 goto done;

1661

1661 - result = parse_loose_ref_contents(content.buf, oid, referent, type);

1662 + result = parse_loose_ref_contents(content.buf, oid, referent, type,

1663 + failure_errno);

1664

1665 done:

1666 strbuf_release(&full_path);

@@ -1673,7 +1675,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,

1675 assert(failure_errno);

1676 if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {

1677 return refs_read_special_head(ref_store, refname, oid, referent,

1676 - type);

1678 + type, failure_errno);

1679 }

1680

1681 return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,

refs/files-backend.c

+20 -11

@@ -355,6 +355,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,

355 int fd;

356 int ret = -1;

357 int remaining_retries = 3;

358 + int myerr = 0;

359

360 *type = 0;

361 strbuf_reset(&sb_path);

@@ -382,11 +383,13 @@ stat_ref:

383

384 if (lstat(path, &st) < 0) {

385 int ignore_errno;

385 - if (errno != ENOENT)

386 + myerr = errno;

387 + errno = 0;

388 + if (myerr != ENOENT)

389 goto out;

390 if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,

391 referent, type, &ignore_errno)) {

389 - errno = ENOENT;

392 + myerr = ENOENT;

393 goto out;

394 }

395 ret = 0;

@@ -397,7 +400,9 @@ stat_ref:

400 if (S_ISLNK(st.st_mode)) {

401 strbuf_reset(&sb_contents);

402 if (strbuf_readlink(&sb_contents, path, st.st_size) < 0) {

400 - if (errno == ENOENT || errno == EINVAL)

403 + myerr = errno;

404 + errno = 0;

405 + if (myerr == ENOENT || myerr == EINVAL)

406 /* inconsistent with lstat; retry */

407 goto stat_ref;

408 else

@@ -427,7 +432,7 @@ stat_ref:

432 */

433 if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,

434 referent, type, &ignore_errno)) {

430 - errno = EISDIR;

435 + myerr = EISDIR;

436 goto out;

437 }

438 ret = 0;

@@ -440,7 +445,8 @@ stat_ref:

445 */

446 fd = open(path, O_RDONLY);

447 if (fd < 0) {

443 - if (errno == ENOENT && !S_ISLNK(st.st_mode))

448 + myerr = errno;

449 + if (myerr == ENOENT && !S_ISLNK(st.st_mode))

450 /* inconsistent with lstat; retry */

451 goto stat_ref;

452 else

@@ -448,26 +454,29 @@ stat_ref:

454 }

455 strbuf_reset(&sb_contents);

456 if (strbuf_read(&sb_contents, fd, 256) < 0) {

451 - int save_errno = errno;

457 + myerr = errno;

458 close(fd);

453 - errno = save_errno;

459 goto out;

460 }

461 close(fd);

462 strbuf_rtrim(&sb_contents);

463 buf = sb_contents.buf;

464

460 - ret = parse_loose_ref_contents(buf, oid, referent, type);

465 + ret = parse_loose_ref_contents(buf, oid, referent, type, &myerr);

466

467 out:

463 - *failure_errno = errno;

468 + if (ret && !myerr)

469 + BUG("returning non-zero %d, should have set myerr!", ret);

470 + *failure_errno = myerr;

471 +

472 strbuf_release(&sb_path);

473 strbuf_release(&sb_contents);

474 return ret;

475 }

476

477 int parse_loose_ref_contents(const char *buf, struct object_id *oid,

470 - struct strbuf *referent, unsigned int *type)

478 + struct strbuf *referent, unsigned int *type,

479 + int *failure_errno)

480 {

481 const char *p;

482 if (skip_prefix(buf, "ref:", &buf)) {

@@ -486,7 +495,7 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,

495 if (parse_oid_hex(buf, oid, &p) ||

496 (*p != '\0' && !isspace(*p))) {

497 *type |= REF_ISBROKEN;

489 - errno = EINVAL;

498 + *failure_errno = EINVAL;

499 return -1;

500 }

501 return 0;

refs/refs-internal.h

+4 -2

@@ -706,10 +706,12 @@ struct ref_store {

706 };

707

708 /*

709 - * Parse contents of a loose ref file.

709 + * Parse contents of a loose ref file. *failure_errno maybe be set to EINVAL for

710 + * invalid contents.

711 */

712 int parse_loose_ref_contents(const char *buf, struct object_id *oid,

712 - struct strbuf *referent, unsigned int *type);

713 + struct strbuf *referent, unsigned int *type,

714 + int *failure_errno);

715

716 /*

717 * Fill in the generic part of refs and add it to our collection of