* [PATCH 1/2] unwritable tests: assert exact error output @ 2021-10-09 13:34 Ævar Arnfjörð Bjarmason 2021-10-09 13:34 ` [PATCH 2/2] commit: fix duplication regression in permission " Ævar Arnfjörð Bjarmason 2021-10-11 20:50 ` [PATCH 1/2] unwritable tests: assert exact " Junio C Hamano 0 siblings, 2 replies; 7+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-09 13:34 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, David Turner, Ævar Arnfjörð Bjarmason In preparation for fixing a regression where we started emitting some of these error messages twice, let's assert what the output from "git commit" and friends is now in the case of permission errors. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t0004-unwritable.sh | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh index e3137d638ee..998c9d1be69 100755 --- a/t/t0004-unwritable.sh +++ b/t/t0004-unwritable.sh @@ -18,27 +18,55 @@ test_expect_success setup ' test_expect_success POSIXPERM,SANITY 'write-tree should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && chmod a-w .git/objects .git/objects/?? && - test_must_fail git write-tree + + cat >expect <<-\EOF && + error: insufficient permission for adding an object to repository database .git/objects + fatal: git-write-tree: error building trees + EOF + test_must_fail git write-tree 2>actual && + test_cmp expect actual ' test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && chmod a-w .git/objects .git/objects/?? && - test_must_fail git commit -m second + + cat >expect <<-\EOF && + error: insufficient permission for adding an object to repository database .git/objects + error: insufficient permission for adding an object to repository database .git/objects + error: Error building trees + EOF + test_must_fail git commit -m second 2>actual && + test_cmp expect actual ' test_expect_success POSIXPERM,SANITY 'update-index should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && echo 6O >file && chmod a-w .git/objects .git/objects/?? && - test_must_fail git update-index file + + cat >expect <<-\EOF && + error: insufficient permission for adding an object to repository database .git/objects + error: file: failed to insert into database + fatal: Unable to process path file + EOF + test_must_fail git update-index file 2>actual && + test_cmp expect actual ' test_expect_success POSIXPERM,SANITY 'add should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && echo b >file && chmod a-w .git/objects .git/objects/?? && - test_must_fail git add file + + cat >expect <<-\EOF && + error: insufficient permission for adding an object to repository database .git/objects + error: file: failed to insert into database + error: unable to index file '\''file'\'' + fatal: updating files failed + EOF + test_must_fail git add file 2>actual && + test_cmp expect actual ' test_done -- 2.33.0.1492.gcc3b81fc59a ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] commit: fix duplication regression in permission error output 2021-10-09 13:34 [PATCH 1/2] unwritable tests: assert exact error output Ævar Arnfjörð Bjarmason @ 2021-10-09 13:34 ` Ævar Arnfjörð Bjarmason 2021-10-11 20:56 ` Junio C Hamano 2021-10-12 14:30 ` [PATCH v2 0/2] " Ævar Arnfjörð Bjarmason 2021-10-11 20:50 ` [PATCH 1/2] unwritable tests: assert exact " Junio C Hamano 1 sibling, 2 replies; 7+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-09 13:34 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, David Turner, Ævar Arnfjörð Bjarmason Fix a regression in the error output emitted when .git/objects can't be written to. Before 9c4d6c0297b (cache-tree: Write updated cache-tree after commit, 2014-07-13) we'd emit only one "insufficient permission" error, now we'll do so again. The cause is rather straightforward, we've got WRITE_TREE_SILENT for the use-case of wanting to prepare an index silently, quieting any permission etc. error output. Then when we attempt to update to that (possibly broken) index we'll run into the same errors again. But with 9c4d6c0297b the gap between the cache-tree API and the object store wasn't closed in terms of asking write_object_file() to be silent. I.e. post-9c4d6c0297b the first call is to prepare_index(), and after that we'll call prepare_to_commit(). We only want verbose error output from the latter. So let's add and use that facility with a corresponding HASH_SILENT flag, its only user is cache-tree.c's update_one(), which will set it if its "WRITE_TREE_SILENT" flag is set. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- cache-tree.c | 5 +++-- cache.h | 1 + object-file.c | 20 ++++++++++++-------- object-store.h | 10 ++++++++-- t/t0004-unwritable.sh | 1 - 5 files changed, 24 insertions(+), 13 deletions(-) diff --git a/cache-tree.c b/cache-tree.c index 90919f9e345..f21bfc631c6 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -440,8 +440,9 @@ static int update_one(struct cache_tree *it, } else if (dryrun) { hash_object_file(the_hash_algo, buffer.buf, buffer.len, tree_type, &it->oid); - } else if (write_object_file(buffer.buf, buffer.len, tree_type, - &it->oid)) { + } else if (write_object_file_flags(buffer.buf, buffer.len, tree_type, + &it->oid, flags & WRITE_TREE_SILENT + ? HASH_SILENT : 0)) { strbuf_release(&buffer); return -1; } diff --git a/cache.h b/cache.h index 3e5658c6dda..088f274fa17 100644 --- a/cache.h +++ b/cache.h @@ -887,6 +887,7 @@ int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, #define HASH_WRITE_OBJECT 1 #define HASH_FORMAT_CHECK 2 #define HASH_RENORMALIZE 4 +#define HASH_SILENT 8 int index_fd(struct index_state *istate, struct object_id *oid, int fd, struct stat *st, enum object_type type, const char *path, unsigned flags); int index_path(struct index_state *istate, struct object_id *oid, const char *path, struct stat *st, unsigned flags); diff --git a/object-file.c b/object-file.c index 112d9b4badc..8fd67b07e45 100644 --- a/object-file.c +++ b/object-file.c @@ -1873,7 +1873,7 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename) static int write_loose_object(const struct object_id *oid, char *hdr, int hdrlen, const void *buf, unsigned long len, - time_t mtime) + time_t mtime, unsigned flags) { int fd, ret; unsigned char compressed[4096]; @@ -1887,7 +1887,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr, fd = create_tmpfile(&tmp_file, filename.buf); if (fd < 0) { - if (errno == EACCES) + if (flags & HASH_SILENT) + return -1; + else if (errno == EACCES) return error(_("insufficient permission for adding an object to repository database %s"), get_object_directory()); else return error_errno(_("unable to create temporary file")); @@ -1937,7 +1939,8 @@ static int write_loose_object(const struct object_id *oid, char *hdr, struct utimbuf utb; utb.actime = mtime; utb.modtime = mtime; - if (utime(tmp_file.buf, &utb) < 0) + if (utime(tmp_file.buf, &utb) < 0 && + !(flags & HASH_SILENT)) warning_errno(_("failed utime() on %s"), tmp_file.buf); } @@ -1962,8 +1965,9 @@ static int freshen_packed_object(const struct object_id *oid) return 1; } -int write_object_file(const void *buf, unsigned long len, const char *type, - struct object_id *oid) +int write_object_file_flags(const void *buf, unsigned long len, + const char *type, struct object_id *oid, + unsigned flags) { char hdr[MAX_HEADER_LEN]; int hdrlen = sizeof(hdr); @@ -1975,7 +1979,7 @@ int write_object_file(const void *buf, unsigned long len, const char *type, &hdrlen); if (freshen_packed_object(oid) || freshen_loose_object(oid)) return 0; - return write_loose_object(oid, hdr, hdrlen, buf, len, 0); + return write_loose_object(oid, hdr, hdrlen, buf, len, 0, flags); } int hash_object_file_literally(const void *buf, unsigned long len, @@ -1995,7 +1999,7 @@ int hash_object_file_literally(const void *buf, unsigned long len, goto cleanup; if (freshen_packed_object(oid) || freshen_loose_object(oid)) goto cleanup; - status = write_loose_object(oid, header, hdrlen, buf, len, 0); + status = write_loose_object(oid, header, hdrlen, buf, len, 0, 0); cleanup: free(header); @@ -2017,7 +2021,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime) if (!buf) return error(_("cannot read object for %s"), oid_to_hex(oid)); hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %"PRIuMAX , type_name(type), (uintmax_t)len) + 1; - ret = write_loose_object(oid, hdr, hdrlen, buf, len, mtime); + ret = write_loose_object(oid, hdr, hdrlen, buf, len, mtime, 0); free(buf); return ret; diff --git a/object-store.h b/object-store.h index c5130d8baea..f5b9e016107 100644 --- a/object-store.h +++ b/object-store.h @@ -223,8 +223,14 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf, unsigned long len, const char *type, struct object_id *oid); -int write_object_file(const void *buf, unsigned long len, - const char *type, struct object_id *oid); +int write_object_file_flags(const void *buf, unsigned long len, + const char *type, struct object_id *oid, + unsigned flags); +static inline int write_object_file(const void *buf, unsigned long len, + const char *type, struct object_id *oid) +{ + return write_object_file_flags(buf, len, type, oid, 0); +} int hash_object_file_literally(const void *buf, unsigned long len, const char *type, struct object_id *oid, diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh index 998c9d1be69..75c6d5c8b49 100755 --- a/t/t0004-unwritable.sh +++ b/t/t0004-unwritable.sh @@ -33,7 +33,6 @@ test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository cat >expect <<-\EOF && error: insufficient permission for adding an object to repository database .git/objects - error: insufficient permission for adding an object to repository database .git/objects error: Error building trees EOF test_must_fail git commit -m second 2>actual && -- 2.33.0.1492.gcc3b81fc59a ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] commit: fix duplication regression in permission error output 2021-10-09 13:34 ` [PATCH 2/2] commit: fix duplication regression in permission " Ævar Arnfjörð Bjarmason @ 2021-10-11 20:56 ` Junio C Hamano 2021-10-12 14:30 ` [PATCH v2 0/2] " Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2021-10-11 20:56 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, David Turner Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > So let's add and use that facility with a corresponding HASH_SILENT > flag, its only user is cache-tree.c's update_one(), which will set it > if its "WRITE_TREE_SILENT" flag is set. OK. > diff --git a/cache.h b/cache.h > index 3e5658c6dda..088f274fa17 100644 > --- a/cache.h > +++ b/cache.h > @@ -887,6 +887,7 @@ int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, > #define HASH_WRITE_OBJECT 1 > #define HASH_FORMAT_CHECK 2 > #define HASH_RENORMALIZE 4 > +#define HASH_SILENT 8 Nice to see this done in a straight-forward way, instead of being a single step buried in a multi-step series whose early part converts preprocessor constants into enum and other "clean-ups" that are not essential (and can be done as a separate series when the codebase around the area is quiescent). > static int write_loose_object(const struct object_id *oid, char *hdr, > int hdrlen, const void *buf, unsigned long len, > - time_t mtime) > + time_t mtime, unsigned flags) > { > int fd, ret; > unsigned char compressed[4096]; > @@ -1887,7 +1887,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr, > > fd = create_tmpfile(&tmp_file, filename.buf); > if (fd < 0) { > - if (errno == EACCES) > + if (flags & HASH_SILENT) > + return -1; OK. > diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh > index 998c9d1be69..75c6d5c8b49 100755 > --- a/t/t0004-unwritable.sh > +++ b/t/t0004-unwritable.sh > @@ -33,7 +33,6 @@ test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository > > cat >expect <<-\EOF && > error: insufficient permission for adding an object to repository database .git/objects > - error: insufficient permission for adding an object to repository database .git/objects > error: Error building trees > EOF Again, I think it makes it more clear if the test starts out as expecting a failure and gets turned into expecting a success as a part of this fix. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 0/2] commit: fix duplication regression in permission error output 2021-10-09 13:34 ` [PATCH 2/2] commit: fix duplication regression in permission " Ævar Arnfjörð Bjarmason 2021-10-11 20:56 ` Junio C Hamano @ 2021-10-12 14:30 ` Ævar Arnfjörð Bjarmason 2021-10-12 14:30 ` [PATCH v2 1/2] unwritable tests: assert exact " Ævar Arnfjörð Bjarmason 2021-10-12 14:30 ` [PATCH v2 2/2] commit: fix duplication regression in permission " Ævar Arnfjörð Bjarmason 1 sibling, 2 replies; 7+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-12 14:30 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, David Turner, Ævar Arnfjörð Bjarmason A v2 of a series that fixes duplicate permission output whe we write trees. Before: $ git commit -m"add another one" error: insufficient permission for adding an object to repository database .git/objects error: insufficient permission for adding an object to repository database .git/objects error: Error building trees After we only emit that "insufficient permission" line once. The only change since v1 is to make 1/2 and 2/2 flip a "test_expect_failure" to a "test_expect_success". Ævar Arnfjörð Bjarmason (2): unwritable tests: assert exact error output commit: fix duplication regression in permission error output cache-tree.c | 5 +++-- cache.h | 1 + object-file.c | 20 ++++++++++-------- object-store.h | 10 +++++++-- t/t0004-unwritable.sh | 47 +++++++++++++++++++++++++++++++++++++++---- 5 files changed, 67 insertions(+), 16 deletions(-) Range-diff against v1: 1: a5ef8ea47f4 ! 1: 74bc5568c88 unwritable tests: assert exact error output @@ Commit message of these error messages twice, let's assert what the output from "git commit" and friends is now in the case of permission errors. + As noted in [1] using test_expect_failure to mark up a TODO test has + some unexpected edge cases, e.g. we don't want to break --run=3 by + skipping the "test_lazy_prereq" here. This pattern allows us to test + just the test_cmp (and the "cat", which shouldn't fail) with the added + "test_expect_failure", we'll flip that to a "test_expect_success" in + the next commit. + + 1. https://lore.kernel.org/git/87tuhmk19c.fsf@evledraar.gmail.com/T/#u + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## t/t0004-unwritable.sh ## @@ t/t0004-unwritable.sh: test_expect_success setup ' test_when_finished "chmod 775 .git/objects .git/objects/??" && chmod a-w .git/objects .git/objects/?? && - test_must_fail git write-tree ++ test_must_fail git write-tree 2>out.write-tree ++' + ++test_lazy_prereq WRITE_TREE_OUT 'test -e "$TRASH_DIRECTORY"/out.write-tree' ++test_expect_success WRITE_TREE_OUT 'write-tree output on unwritable repository' ' + cat >expect <<-\EOF && + error: insufficient permission for adding an object to repository database .git/objects + fatal: git-write-tree: error building trees + EOF -+ test_must_fail git write-tree 2>actual && -+ test_cmp expect actual ++ test_cmp expect out.write-tree ' - test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository' ' + test_expect_success POSIXPERM,SANITY,!SANITIZE_LEAK 'commit should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && chmod a-w .git/objects .git/objects/?? && - test_must_fail git commit -m second ++ test_must_fail git commit -m second 2>out.commit ++' + ++test_lazy_prereq COMMIT_OUT 'test -e "$TRASH_DIRECTORY"/out.commit' ++test_expect_failure COMMIT_OUT 'commit output on unwritable repository' ' + cat >expect <<-\EOF && + error: insufficient permission for adding an object to repository database .git/objects -+ error: insufficient permission for adding an object to repository database .git/objects + error: Error building trees + EOF -+ test_must_fail git commit -m second 2>actual && -+ test_cmp expect actual ++ test_cmp expect out.commit ' test_expect_success POSIXPERM,SANITY 'update-index should notice unwritable repository' ' @@ t/t0004-unwritable.sh: test_expect_success setup ' echo 6O >file && chmod a-w .git/objects .git/objects/?? && - test_must_fail git update-index file ++ test_must_fail git update-index file 2>out.update-index ++' + ++test_lazy_prereq UPDATE_INDEX_OUT 'test -e "$TRASH_DIRECTORY"/out.update-index' ++test_expect_success UPDATE_INDEX_OUT 'update-index output on unwritable repository' ' + cat >expect <<-\EOF && + error: insufficient permission for adding an object to repository database .git/objects + error: file: failed to insert into database + fatal: Unable to process path file + EOF -+ test_must_fail git update-index file 2>actual && -+ test_cmp expect actual ++ test_cmp expect out.update-index ' test_expect_success POSIXPERM,SANITY 'add should notice unwritable repository' ' @@ t/t0004-unwritable.sh: test_expect_success setup ' echo b >file && chmod a-w .git/objects .git/objects/?? && - test_must_fail git add file ++ test_must_fail git add file 2>out.add ++' + ++test_lazy_prereq ADD_OUT 'test -e "$TRASH_DIRECTORY"/out.add' ++test_expect_success ADD_OUT 'add output on unwritable repository' ' + cat >expect <<-\EOF && + error: insufficient permission for adding an object to repository database .git/objects + error: file: failed to insert into database + error: unable to index file '\''file'\'' + fatal: updating files failed + EOF -+ test_must_fail git add file 2>actual && -+ test_cmp expect actual ++ test_cmp expect out.add ' test_done 2: 56b20f6024b ! 2: e6cd47355d5 commit: fix duplication regression in permission error output @@ object-store.h: int hash_object_file(const struct git_hash_algo *algo, const voi const char *type, struct object_id *oid, ## t/t0004-unwritable.sh ## -@@ t/t0004-unwritable.sh: test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository +@@ t/t0004-unwritable.sh: test_expect_success POSIXPERM,SANITY,!SANITIZE_LEAK 'commit should notice unwrit + ' + test_lazy_prereq COMMIT_OUT 'test -e "$TRASH_DIRECTORY"/out.commit' +-test_expect_failure COMMIT_OUT 'commit output on unwritable repository' ' ++test_expect_success COMMIT_OUT 'commit output on unwritable repository' ' cat >expect <<-\EOF && error: insufficient permission for adding an object to repository database .git/objects -- error: insufficient permission for adding an object to repository database .git/objects error: Error building trees - EOF - test_must_fail git commit -m second 2>actual && -- 2.33.0.1567.g7b23ce7ed9e ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] unwritable tests: assert exact error output 2021-10-12 14:30 ` [PATCH v2 0/2] " Ævar Arnfjörð Bjarmason @ 2021-10-12 14:30 ` Ævar Arnfjörð Bjarmason 2021-10-12 14:30 ` [PATCH v2 2/2] commit: fix duplication regression in permission " Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 7+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-12 14:30 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, David Turner, Ævar Arnfjörð Bjarmason In preparation for fixing a regression where we started emitting some of these error messages twice, let's assert what the output from "git commit" and friends is now in the case of permission errors. As noted in [1] using test_expect_failure to mark up a TODO test has some unexpected edge cases, e.g. we don't want to break --run=3 by skipping the "test_lazy_prereq" here. This pattern allows us to test just the test_cmp (and the "cat", which shouldn't fail) with the added "test_expect_failure", we'll flip that to a "test_expect_success" in the next commit. 1. https://lore.kernel.org/git/87tuhmk19c.fsf@evledraar.gmail.com/T/#u Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t0004-unwritable.sh | 47 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh index 37d68ef03be..3627ce367c0 100755 --- a/t/t0004-unwritable.sh +++ b/t/t0004-unwritable.sh @@ -19,27 +19,66 @@ test_expect_success setup ' test_expect_success POSIXPERM,SANITY 'write-tree should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && chmod a-w .git/objects .git/objects/?? && - test_must_fail git write-tree + test_must_fail git write-tree 2>out.write-tree +' + +test_lazy_prereq WRITE_TREE_OUT 'test -e "$TRASH_DIRECTORY"/out.write-tree' +test_expect_success WRITE_TREE_OUT 'write-tree output on unwritable repository' ' + cat >expect <<-\EOF && + error: insufficient permission for adding an object to repository database .git/objects + fatal: git-write-tree: error building trees + EOF + test_cmp expect out.write-tree ' test_expect_success POSIXPERM,SANITY,!SANITIZE_LEAK 'commit should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && chmod a-w .git/objects .git/objects/?? && - test_must_fail git commit -m second + test_must_fail git commit -m second 2>out.commit +' + +test_lazy_prereq COMMIT_OUT 'test -e "$TRASH_DIRECTORY"/out.commit' +test_expect_failure COMMIT_OUT 'commit output on unwritable repository' ' + cat >expect <<-\EOF && + error: insufficient permission for adding an object to repository database .git/objects + error: Error building trees + EOF + test_cmp expect out.commit ' test_expect_success POSIXPERM,SANITY 'update-index should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && echo 6O >file && chmod a-w .git/objects .git/objects/?? && - test_must_fail git update-index file + test_must_fail git update-index file 2>out.update-index +' + +test_lazy_prereq UPDATE_INDEX_OUT 'test -e "$TRASH_DIRECTORY"/out.update-index' +test_expect_success UPDATE_INDEX_OUT 'update-index output on unwritable repository' ' + cat >expect <<-\EOF && + error: insufficient permission for adding an object to repository database .git/objects + error: file: failed to insert into database + fatal: Unable to process path file + EOF + test_cmp expect out.update-index ' test_expect_success POSIXPERM,SANITY 'add should notice unwritable repository' ' test_when_finished "chmod 775 .git/objects .git/objects/??" && echo b >file && chmod a-w .git/objects .git/objects/?? && - test_must_fail git add file + test_must_fail git add file 2>out.add +' + +test_lazy_prereq ADD_OUT 'test -e "$TRASH_DIRECTORY"/out.add' +test_expect_success ADD_OUT 'add output on unwritable repository' ' + cat >expect <<-\EOF && + error: insufficient permission for adding an object to repository database .git/objects + error: file: failed to insert into database + error: unable to index file '\''file'\'' + fatal: updating files failed + EOF + test_cmp expect out.add ' test_done -- 2.33.0.1567.g7b23ce7ed9e ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] commit: fix duplication regression in permission error output 2021-10-12 14:30 ` [PATCH v2 0/2] " Ævar Arnfjörð Bjarmason 2021-10-12 14:30 ` [PATCH v2 1/2] unwritable tests: assert exact " Ævar Arnfjörð Bjarmason @ 2021-10-12 14:30 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 7+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-12 14:30 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, David Turner, Ævar Arnfjörð Bjarmason Fix a regression in the error output emitted when .git/objects can't be written to. Before 9c4d6c0297b (cache-tree: Write updated cache-tree after commit, 2014-07-13) we'd emit only one "insufficient permission" error, now we'll do so again. The cause is rather straightforward, we've got WRITE_TREE_SILENT for the use-case of wanting to prepare an index silently, quieting any permission etc. error output. Then when we attempt to update to that (possibly broken) index we'll run into the same errors again. But with 9c4d6c0297b the gap between the cache-tree API and the object store wasn't closed in terms of asking write_object_file() to be silent. I.e. post-9c4d6c0297b the first call is to prepare_index(), and after that we'll call prepare_to_commit(). We only want verbose error output from the latter. So let's add and use that facility with a corresponding HASH_SILENT flag, its only user is cache-tree.c's update_one(), which will set it if its "WRITE_TREE_SILENT" flag is set. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- cache-tree.c | 5 +++-- cache.h | 1 + object-file.c | 20 ++++++++++++-------- object-store.h | 10 ++++++++-- t/t0004-unwritable.sh | 2 +- 5 files changed, 25 insertions(+), 13 deletions(-) diff --git a/cache-tree.c b/cache-tree.c index 90919f9e345..f21bfc631c6 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -440,8 +440,9 @@ static int update_one(struct cache_tree *it, } else if (dryrun) { hash_object_file(the_hash_algo, buffer.buf, buffer.len, tree_type, &it->oid); - } else if (write_object_file(buffer.buf, buffer.len, tree_type, - &it->oid)) { + } else if (write_object_file_flags(buffer.buf, buffer.len, tree_type, + &it->oid, flags & WRITE_TREE_SILENT + ? HASH_SILENT : 0)) { strbuf_release(&buffer); return -1; } diff --git a/cache.h b/cache.h index d092820c943..dc58bfa48a0 100644 --- a/cache.h +++ b/cache.h @@ -887,6 +887,7 @@ int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, #define HASH_WRITE_OBJECT 1 #define HASH_FORMAT_CHECK 2 #define HASH_RENORMALIZE 4 +#define HASH_SILENT 8 int index_fd(struct index_state *istate, struct object_id *oid, int fd, struct stat *st, enum object_type type, const char *path, unsigned flags); int index_path(struct index_state *istate, struct object_id *oid, const char *path, struct stat *st, unsigned flags); diff --git a/object-file.c b/object-file.c index 112d9b4badc..8fd67b07e45 100644 --- a/object-file.c +++ b/object-file.c @@ -1873,7 +1873,7 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename) static int write_loose_object(const struct object_id *oid, char *hdr, int hdrlen, const void *buf, unsigned long len, - time_t mtime) + time_t mtime, unsigned flags) { int fd, ret; unsigned char compressed[4096]; @@ -1887,7 +1887,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr, fd = create_tmpfile(&tmp_file, filename.buf); if (fd < 0) { - if (errno == EACCES) + if (flags & HASH_SILENT) + return -1; + else if (errno == EACCES) return error(_("insufficient permission for adding an object to repository database %s"), get_object_directory()); else return error_errno(_("unable to create temporary file")); @@ -1937,7 +1939,8 @@ static int write_loose_object(const struct object_id *oid, char *hdr, struct utimbuf utb; utb.actime = mtime; utb.modtime = mtime; - if (utime(tmp_file.buf, &utb) < 0) + if (utime(tmp_file.buf, &utb) < 0 && + !(flags & HASH_SILENT)) warning_errno(_("failed utime() on %s"), tmp_file.buf); } @@ -1962,8 +1965,9 @@ static int freshen_packed_object(const struct object_id *oid) return 1; } -int write_object_file(const void *buf, unsigned long len, const char *type, - struct object_id *oid) +int write_object_file_flags(const void *buf, unsigned long len, + const char *type, struct object_id *oid, + unsigned flags) { char hdr[MAX_HEADER_LEN]; int hdrlen = sizeof(hdr); @@ -1975,7 +1979,7 @@ int write_object_file(const void *buf, unsigned long len, const char *type, &hdrlen); if (freshen_packed_object(oid) || freshen_loose_object(oid)) return 0; - return write_loose_object(oid, hdr, hdrlen, buf, len, 0); + return write_loose_object(oid, hdr, hdrlen, buf, len, 0, flags); } int hash_object_file_literally(const void *buf, unsigned long len, @@ -1995,7 +1999,7 @@ int hash_object_file_literally(const void *buf, unsigned long len, goto cleanup; if (freshen_packed_object(oid) || freshen_loose_object(oid)) goto cleanup; - status = write_loose_object(oid, header, hdrlen, buf, len, 0); + status = write_loose_object(oid, header, hdrlen, buf, len, 0, 0); cleanup: free(header); @@ -2017,7 +2021,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime) if (!buf) return error(_("cannot read object for %s"), oid_to_hex(oid)); hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %"PRIuMAX , type_name(type), (uintmax_t)len) + 1; - ret = write_loose_object(oid, hdr, hdrlen, buf, len, mtime); + ret = write_loose_object(oid, hdr, hdrlen, buf, len, mtime, 0); free(buf); return ret; diff --git a/object-store.h b/object-store.h index 1e647a5be30..cea8d38bbd0 100644 --- a/object-store.h +++ b/object-store.h @@ -223,8 +223,14 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf, unsigned long len, const char *type, struct object_id *oid); -int write_object_file(const void *buf, unsigned long len, - const char *type, struct object_id *oid); +int write_object_file_flags(const void *buf, unsigned long len, + const char *type, struct object_id *oid, + unsigned flags); +static inline int write_object_file(const void *buf, unsigned long len, + const char *type, struct object_id *oid) +{ + return write_object_file_flags(buf, len, type, oid, 0); +} int hash_object_file_literally(const void *buf, unsigned long len, const char *type, struct object_id *oid, diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh index 3627ce367c0..2e9d652d826 100755 --- a/t/t0004-unwritable.sh +++ b/t/t0004-unwritable.sh @@ -38,7 +38,7 @@ test_expect_success POSIXPERM,SANITY,!SANITIZE_LEAK 'commit should notice unwrit ' test_lazy_prereq COMMIT_OUT 'test -e "$TRASH_DIRECTORY"/out.commit' -test_expect_failure COMMIT_OUT 'commit output on unwritable repository' ' +test_expect_success COMMIT_OUT 'commit output on unwritable repository' ' cat >expect <<-\EOF && error: insufficient permission for adding an object to repository database .git/objects error: Error building trees -- 2.33.0.1567.g7b23ce7ed9e ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] unwritable tests: assert exact error output 2021-10-09 13:34 [PATCH 1/2] unwritable tests: assert exact error output Ævar Arnfjörð Bjarmason 2021-10-09 13:34 ` [PATCH 2/2] commit: fix duplication regression in permission " Ævar Arnfjörð Bjarmason @ 2021-10-11 20:50 ` Junio C Hamano 1 sibling, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2021-10-11 20:50 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, David Turner Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > In preparation for fixing a regression where we started emitting some > of these error messages twice, let's assert what the output from "git > commit" and friends is now in the case of permission errors. Usually we frown upon expecting an exact error message, but I guess that the nature of the bug this series tries to address justifies it. > - test_must_fail git write-tree > + > + cat >expect <<-\EOF && > + error: insufficient permission for adding an object to repository database .git/objects > + fatal: git-write-tree: error building trees > + EOF > + test_must_fail git write-tree 2>actual && > + test_cmp expect actual > ' OK. > test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository' ' > test_when_finished "chmod 775 .git/objects .git/objects/??" && > chmod a-w .git/objects .git/objects/?? && > - test_must_fail git commit -m second > + > + cat >expect <<-\EOF && > + error: insufficient permission for adding an object to repository database .git/objects > + error: insufficient permission for adding an object to repository database .git/objects > + error: Error building trees > + EOF This is odd. Shouldn't the test expect one message from write-tree and be marked as expecting a failure until the bug gets fixed? > + test_must_fail git commit -m second 2>actual && > + test_cmp expect actual > ' ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-12 14:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-09 13:34 [PATCH 1/2] unwritable tests: assert exact error output Ævar Arnfjörð Bjarmason 2021-10-09 13:34 ` [PATCH 2/2] commit: fix duplication regression in permission " Ævar Arnfjörð Bjarmason 2021-10-11 20:56 ` Junio C Hamano 2021-10-12 14:30 ` [PATCH v2 0/2] " Ævar Arnfjörð Bjarmason 2021-10-12 14:30 ` [PATCH v2 1/2] unwritable tests: assert exact " Ævar Arnfjörð Bjarmason 2021-10-12 14:30 ` [PATCH v2 2/2] commit: fix duplication regression in permission " Ævar Arnfjörð Bjarmason 2021-10-11 20:50 ` [PATCH 1/2] unwritable tests: assert exact " Junio C Hamano
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).