{"thread":{"id":"56674","subject":"[PATCH 2/2] commit: fix duplication regression in permission error output","startedAt":"2021-10-09T13:34:16Z","lastAt":"2021-10-12T14:31:01Z","messageCount":7,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"438405","messageId":"patch-2.2-56b20f6024b-20211009T133354Z-avarab@gmail.com","threadId":"56674","inReplyTo":"patch-1.2-a5ef8ea47f4-20211009T133354Z-avarab@gmail.com","subject":"[PATCH 2/2] commit: fix duplication regression in permission error output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-09T13:34:03Z","receivedAt":"2021-10-09T13:34:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a regression in the error output emitted when .git/objects can't\nbe written to. Before 9c4d6c0297b (cache-tree: Write updated\ncache-tree after commit, 2014-07-13) we'd emit only one \"insufficient\npermission\" error, now we'll do so again.\n\nThe cause is rather straightforward, we've got WRITE_TREE_SILENT for\nthe use-case of wanting to prepare an index silently, quieting any\npermission etc. error output. Then when we attempt to update to\nthat (possibly broken) index we'll run into the same errors again.\n\nBut with 9c4d6c0297b the gap between the cache-tree API and the object\nstore wasn't closed in terms of asking write_object_file() to be\nsilent. I.e. post-9c4d6c0297b the first call is to prepare_index(),\nand after that we'll call prepare_to_commit(). We only want verbose\nerror output from the latter.\n\nSo let's add and use that facility with a corresponding HASH_SILENT\nflag, its only user is cache-tree.c's update_one(), which will set it\nif its \"WRITE_TREE_SILENT\" flag is set.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n cache-tree.c          |  5 +++--\n cache.h               |  1 +\n object-file.c         | 20 ++++++++++++--------\n object-store.h        | 10 ++++++++--\n t/t0004-unwritable.sh |  1 -\n 5 files changed, 24 insertions(+), 13 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 90919f9e345..f21bfc631c6 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -440,8 +440,9 @@ static int update_one(struct cache_tree *it,\n \t} else if (dryrun) {\n \t\thash_object_file(the_hash_algo, buffer.buf, buffer.len,\n \t\t\t\t tree_type, &it->oid);\n-\t} else if (write_object_file(buffer.buf, buffer.len, tree_type,\n-\t\t\t\t     &it->oid)) {\n+\t} else if (write_object_file_flags(buffer.buf, buffer.len, tree_type,\n+\t\t\t\t\t   &it->oid, flags & WRITE_TREE_SILENT\n+\t\t\t\t\t   ? HASH_SILENT : 0)) {\n \t\tstrbuf_release(&buffer);\n \t\treturn -1;\n \t}\ndiff --git a/cache.h b/cache.h\nindex 3e5658c6dda..088f274fa17 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -887,6 +887,7 @@ int ie_modified(struct index_state *, const struct cache_entry *, struct stat *,\n #define HASH_WRITE_OBJECT 1\n #define HASH_FORMAT_CHECK 2\n #define HASH_RENORMALIZE  4\n+#define HASH_SILENT 8\n 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);\n int index_path(struct index_state *istate, struct object_id *oid, const char *path, struct stat *st, unsigned flags);\n \ndiff --git a/object-file.c b/object-file.c\nindex 112d9b4badc..8fd67b07e45 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1873,7 +1873,7 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)\n \n static int write_loose_object(const struct object_id *oid, char *hdr,\n \t\t\t      int hdrlen, const void *buf, unsigned long len,\n-\t\t\t      time_t mtime)\n+\t\t\t      time_t mtime, unsigned flags)\n {\n \tint fd, ret;\n \tunsigned char compressed[4096];\n@@ -1887,7 +1887,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,\n \n \tfd = create_tmpfile(&tmp_file, filename.buf);\n \tif (fd < 0) {\n-\t\tif (errno == EACCES)\n+\t\tif (flags & HASH_SILENT)\n+\t\t\treturn -1;\n+\t\telse if (errno == EACCES)\n \t\t\treturn error(_(\"insufficient permission for adding an object to repository database %s\"), get_object_directory());\n \t\telse\n \t\t\treturn error_errno(_(\"unable to create temporary file\"));\n@@ -1937,7 +1939,8 @@ static int write_loose_object(const struct object_id *oid, char *hdr,\n \t\tstruct utimbuf utb;\n \t\tutb.actime = mtime;\n \t\tutb.modtime = mtime;\n-\t\tif (utime(tmp_file.buf, &utb) < 0)\n+\t\tif (utime(tmp_file.buf, &utb) < 0 &&\n+\t\t    !(flags & HASH_SILENT))\n \t\t\twarning_errno(_(\"failed utime() on %s\"), tmp_file.buf);\n \t}\n \n@@ -1962,8 +1965,9 @@ static int freshen_packed_object(const struct object_id *oid)\n \treturn 1;\n }\n \n-int write_object_file(const void *buf, unsigned long len, const char *type,\n-\t\t      struct object_id *oid)\n+int write_object_file_flags(const void *buf, unsigned long len,\n+\t\t\t    const char *type, struct object_id *oid,\n+\t\t\t    unsigned flags)\n {\n \tchar hdr[MAX_HEADER_LEN];\n \tint hdrlen = sizeof(hdr);\n@@ -1975,7 +1979,7 @@ int write_object_file(const void *buf, unsigned long len, const char *type,\n \t\t\t\t  &hdrlen);\n \tif (freshen_packed_object(oid) || freshen_loose_object(oid))\n \t\treturn 0;\n-\treturn write_loose_object(oid, hdr, hdrlen, buf, len, 0);\n+\treturn write_loose_object(oid, hdr, hdrlen, buf, len, 0, flags);\n }\n \n int hash_object_file_literally(const void *buf, unsigned long len,\n@@ -1995,7 +1999,7 @@ int hash_object_file_literally(const void *buf, unsigned long len,\n \t\tgoto cleanup;\n \tif (freshen_packed_object(oid) || freshen_loose_object(oid))\n \t\tgoto cleanup;\n-\tstatus = write_loose_object(oid, header, hdrlen, buf, len, 0);\n+\tstatus = write_loose_object(oid, header, hdrlen, buf, len, 0, 0);\n \n cleanup:\n \tfree(header);\n@@ -2017,7 +2021,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \tif (!buf)\n \t\treturn error(_(\"cannot read object for %s\"), oid_to_hex(oid));\n \thdrlen = xsnprintf(hdr, sizeof(hdr), \"%s %\"PRIuMAX , type_name(type), (uintmax_t)len) + 1;\n-\tret = write_loose_object(oid, hdr, hdrlen, buf, len, mtime);\n+\tret = write_loose_object(oid, hdr, hdrlen, buf, len, mtime, 0);\n \tfree(buf);\n \n \treturn ret;\ndiff --git a/object-store.h b/object-store.h\nindex c5130d8baea..f5b9e016107 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -223,8 +223,14 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,\n \t\t     unsigned long len, const char *type,\n \t\t     struct object_id *oid);\n \n-int write_object_file(const void *buf, unsigned long len,\n-\t\t      const char *type, struct object_id *oid);\n+int write_object_file_flags(const void *buf, unsigned long len,\n+\t\t\t    const char *type, struct object_id *oid,\n+\t\t\t    unsigned flags);\n+static inline int write_object_file(const void *buf, unsigned long len,\n+\t\t\t\t    const char *type, struct object_id *oid)\n+{\n+\treturn write_object_file_flags(buf, len, type, oid, 0);\n+}\n \n int hash_object_file_literally(const void *buf, unsigned long len,\n \t\t\t       const char *type, struct object_id *oid,\ndiff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh\nindex 998c9d1be69..75c6d5c8b49 100755\n--- a/t/t0004-unwritable.sh\n+++ b/t/t0004-unwritable.sh\n@@ -33,7 +33,6 @@ test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository\n \n \tcat >expect <<-\\EOF &&\n \terror: insufficient permission for adding an object to repository database .git/objects\n-\terror: insufficient permission for adding an object to repository database .git/objects\n \terror: Error building trees\n \tEOF\n \ttest_must_fail git commit -m second 2>actual &&\n-- \n2.33.0.1492.gcc3b81fc59a\n\n"},{"id":"438406","messageId":"patch-1.2-a5ef8ea47f4-20211009T133354Z-avarab@gmail.com","threadId":"56674","inReplyTo":null,"subject":"[PATCH 1/2] unwritable tests: assert exact error output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-09T13:34:02Z","receivedAt":"2021-10-09T13:34:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preparation for fixing a regression where we started emitting some\nof these error messages twice, let's assert what the output from \"git\ncommit\" and friends is now in the case of permission errors.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t0004-unwritable.sh | 36 ++++++++++++++++++++++++++++++++----\n 1 file changed, 32 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh\nindex e3137d638ee..998c9d1be69 100755\n--- a/t/t0004-unwritable.sh\n+++ b/t/t0004-unwritable.sh\n@@ -18,27 +18,55 @@ test_expect_success setup '\n test_expect_success POSIXPERM,SANITY 'write-tree should notice unwritable repository' '\n \ttest_when_finished \"chmod 775 .git/objects .git/objects/??\" &&\n \tchmod a-w .git/objects .git/objects/?? &&\n-\ttest_must_fail git write-tree\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: insufficient permission for adding an object to repository database .git/objects\n+\tfatal: git-write-tree: error building trees\n+\tEOF\n+\ttest_must_fail git write-tree 2>actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository' '\n \ttest_when_finished \"chmod 775 .git/objects .git/objects/??\" &&\n \tchmod a-w .git/objects .git/objects/?? &&\n-\ttest_must_fail git commit -m second\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: insufficient permission for adding an object to repository database .git/objects\n+\terror: insufficient permission for adding an object to repository database .git/objects\n+\terror: Error building trees\n+\tEOF\n+\ttest_must_fail git commit -m second 2>actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success POSIXPERM,SANITY 'update-index should notice unwritable repository' '\n \ttest_when_finished \"chmod 775 .git/objects .git/objects/??\" &&\n \techo 6O >file &&\n \tchmod a-w .git/objects .git/objects/?? &&\n-\ttest_must_fail git update-index file\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: insufficient permission for adding an object to repository database .git/objects\n+\terror: file: failed to insert into database\n+\tfatal: Unable to process path file\n+\tEOF\n+\ttest_must_fail git update-index file 2>actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success POSIXPERM,SANITY 'add should notice unwritable repository' '\n \ttest_when_finished \"chmod 775 .git/objects .git/objects/??\" &&\n \techo b >file &&\n \tchmod a-w .git/objects .git/objects/?? &&\n-\ttest_must_fail git add file\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: insufficient permission for adding an object to repository database .git/objects\n+\terror: file: failed to insert into database\n+\terror: unable to index file '\\''file'\\''\n+\tfatal: updating files failed\n+\tEOF\n+\ttest_must_fail git add file 2>actual &&\n+\ttest_cmp expect actual\n '\n \n test_done\n-- \n2.33.0.1492.gcc3b81fc59a\n\n"},{"id":"438504","messageId":"xmqq5yu3b80j.fsf@gitster.g","threadId":"56674","inReplyTo":"patch-1.2-a5ef8ea47f4-20211009T133354Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] unwritable tests: assert exact error output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-11T20:50:52Z","receivedAt":"2021-10-11T20:50:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> In preparation for fixing a regression where we started emitting some\n> of these error messages twice, let's assert what the output from \"git\n> commit\" and friends is now in the case of permission errors.\n\nUsually we frown upon expecting an exact error message, but I guess\nthat the nature of the bug this series tries to address justifies\nit.\n\n> -\ttest_must_fail git write-tree\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\terror: insufficient permission for adding an object to repository database .git/objects\n> +\tfatal: git-write-tree: error building trees\n> +\tEOF\n> +\ttest_must_fail git write-tree 2>actual &&\n> +\ttest_cmp expect actual\n>  '\n\nOK.\n\n>  test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository' '\n>  \ttest_when_finished \"chmod 775 .git/objects .git/objects/??\" &&\n>  \tchmod a-w .git/objects .git/objects/?? &&\n> -\ttest_must_fail git commit -m second\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\terror: insufficient permission for adding an object to repository database .git/objects\n> +\terror: insufficient permission for adding an object to repository database .git/objects\n> +\terror: Error building trees\n> +\tEOF\n\nThis is odd.  Shouldn't the test expect one message from write-tree\nand be marked as expecting a failure until the bug gets fixed?\n\n> +\ttest_must_fail git commit -m second 2>actual &&\n> +\ttest_cmp expect actual\n>  '\n"},{"id":"438505","messageId":"xmqq1r4rb7rh.fsf@gitster.g","threadId":"56674","inReplyTo":"patch-2.2-56b20f6024b-20211009T133354Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] commit: fix duplication regression in permission error output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-11T20:56:18Z","receivedAt":"2021-10-11T20:56:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> So let's add and use that facility with a corresponding HASH_SILENT\n> flag, its only user is cache-tree.c's update_one(), which will set it\n> if its \"WRITE_TREE_SILENT\" flag is set.\n\nOK.\n\n> diff --git a/cache.h b/cache.h\n> index 3e5658c6dda..088f274fa17 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -887,6 +887,7 @@ int ie_modified(struct index_state *, const struct cache_entry *, struct stat *,\n>  #define HASH_WRITE_OBJECT 1\n>  #define HASH_FORMAT_CHECK 2\n>  #define HASH_RENORMALIZE  4\n> +#define HASH_SILENT 8\n\nNice to see this done in a straight-forward way, instead of being a\nsingle step buried in a multi-step series whose early part converts\npreprocessor constants into enum and other \"clean-ups\" that are not\nessential (and can be done as a separate series when the codebase\naround the area is quiescent).\n\n>  static int write_loose_object(const struct object_id *oid, char *hdr,\n>  \t\t\t      int hdrlen, const void *buf, unsigned long len,\n> -\t\t\t      time_t mtime)\n> +\t\t\t      time_t mtime, unsigned flags)\n>  {\n>  \tint fd, ret;\n>  \tunsigned char compressed[4096];\n> @@ -1887,7 +1887,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,\n>  \n>  \tfd = create_tmpfile(&tmp_file, filename.buf);\n>  \tif (fd < 0) {\n> -\t\tif (errno == EACCES)\n> +\t\tif (flags & HASH_SILENT)\n> +\t\t\treturn -1;\n\nOK.\n\n> diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh\n> index 998c9d1be69..75c6d5c8b49 100755\n> --- a/t/t0004-unwritable.sh\n> +++ b/t/t0004-unwritable.sh\n> @@ -33,7 +33,6 @@ test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository\n>  \n>  \tcat >expect <<-\\EOF &&\n>  \terror: insufficient permission for adding an object to repository database .git/objects\n> -\terror: insufficient permission for adding an object to repository database .git/objects\n>  \terror: Error building trees\n>  \tEOF\n\nAgain, I think it makes it more clear if the test starts out as\nexpecting a failure and gets turned into expecting a success as a\npart of this fix.\n\nThanks.\n"},{"id":"438563","messageId":"cover-v2-0.2-00000000000-20211012T142950Z-avarab@gmail.com","threadId":"56674","inReplyTo":"patch-2.2-56b20f6024b-20211009T133354Z-avarab@gmail.com","subject":"[PATCH v2 0/2] commit: fix duplication regression in permission error output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-12T14:30:47Z","receivedAt":"2021-10-12T14:30:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A v2 of a series that fixes duplicate permission output whe we write\ntrees. Before:\n\n    $ git commit -m\"add another one\"\n    error: insufficient permission for adding an object to repository database .git/objects\n    error: insufficient permission for adding an object to repository database .git/objects\n    error: Error building trees\n\nAfter we only emit that \"insufficient permission\" line once.\n\nThe only change since v1 is to make 1/2 and 2/2 flip a\n\"test_expect_failure\" to a \"test_expect_success\".\n\nÆvar Arnfjörð Bjarmason (2):\n  unwritable tests: assert exact error output\n  commit: fix duplication regression in permission error output\n\n cache-tree.c          |  5 +++--\n cache.h               |  1 +\n object-file.c         | 20 ++++++++++--------\n object-store.h        | 10 +++++++--\n t/t0004-unwritable.sh | 47 +++++++++++++++++++++++++++++++++++++++----\n 5 files changed, 67 insertions(+), 16 deletions(-)\n\nRange-diff against v1:\n1:  a5ef8ea47f4 ! 1:  74bc5568c88 unwritable tests: assert exact error output\n    @@ Commit message\n         of these error messages twice, let's assert what the output from \"git\n         commit\" and friends is now in the case of permission errors.\n     \n    +    As noted in [1] using test_expect_failure to mark up a TODO test has\n    +    some unexpected edge cases, e.g. we don't want to break --run=3 by\n    +    skipping the \"test_lazy_prereq\" here. This pattern allows us to test\n    +    just the test_cmp (and the \"cat\", which shouldn't fail) with the added\n    +    \"test_expect_failure\", we'll flip that to a \"test_expect_success\" in\n    +    the next commit.\n    +\n    +    1. https://lore.kernel.org/git/87tuhmk19c.fsf@evledraar.gmail.com/T/#u\n    +\n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## t/t0004-unwritable.sh ##\n    @@ t/t0004-unwritable.sh: test_expect_success setup '\n      \ttest_when_finished \"chmod 775 .git/objects .git/objects/??\" &&\n      \tchmod a-w .git/objects .git/objects/?? &&\n     -\ttest_must_fail git write-tree\n    ++\ttest_must_fail git write-tree 2>out.write-tree\n    ++'\n     +\n    ++test_lazy_prereq WRITE_TREE_OUT 'test -e \"$TRASH_DIRECTORY\"/out.write-tree'\n    ++test_expect_success WRITE_TREE_OUT 'write-tree output on unwritable repository' '\n     +\tcat >expect <<-\\EOF &&\n     +\terror: insufficient permission for adding an object to repository database .git/objects\n     +\tfatal: git-write-tree: error building trees\n     +\tEOF\n    -+\ttest_must_fail git write-tree 2>actual &&\n    -+\ttest_cmp expect actual\n    ++\ttest_cmp expect out.write-tree\n      '\n      \n    - test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository' '\n    + test_expect_success POSIXPERM,SANITY,!SANITIZE_LEAK 'commit should notice unwritable repository' '\n      \ttest_when_finished \"chmod 775 .git/objects .git/objects/??\" &&\n      \tchmod a-w .git/objects .git/objects/?? &&\n     -\ttest_must_fail git commit -m second\n    ++\ttest_must_fail git commit -m second 2>out.commit\n    ++'\n     +\n    ++test_lazy_prereq COMMIT_OUT 'test -e \"$TRASH_DIRECTORY\"/out.commit'\n    ++test_expect_failure COMMIT_OUT 'commit output on unwritable repository' '\n     +\tcat >expect <<-\\EOF &&\n     +\terror: insufficient permission for adding an object to repository database .git/objects\n    -+\terror: insufficient permission for adding an object to repository database .git/objects\n     +\terror: Error building trees\n     +\tEOF\n    -+\ttest_must_fail git commit -m second 2>actual &&\n    -+\ttest_cmp expect actual\n    ++\ttest_cmp expect out.commit\n      '\n      \n      test_expect_success POSIXPERM,SANITY 'update-index should notice unwritable repository' '\n    @@ t/t0004-unwritable.sh: test_expect_success setup '\n      \techo 6O >file &&\n      \tchmod a-w .git/objects .git/objects/?? &&\n     -\ttest_must_fail git update-index file\n    ++\ttest_must_fail git update-index file 2>out.update-index\n    ++'\n     +\n    ++test_lazy_prereq UPDATE_INDEX_OUT 'test -e \"$TRASH_DIRECTORY\"/out.update-index'\n    ++test_expect_success UPDATE_INDEX_OUT 'update-index output on unwritable repository' '\n     +\tcat >expect <<-\\EOF &&\n     +\terror: insufficient permission for adding an object to repository database .git/objects\n     +\terror: file: failed to insert into database\n     +\tfatal: Unable to process path file\n     +\tEOF\n    -+\ttest_must_fail git update-index file 2>actual &&\n    -+\ttest_cmp expect actual\n    ++\ttest_cmp expect out.update-index\n      '\n      \n      test_expect_success POSIXPERM,SANITY 'add should notice unwritable repository' '\n    @@ t/t0004-unwritable.sh: test_expect_success setup '\n      \techo b >file &&\n      \tchmod a-w .git/objects .git/objects/?? &&\n     -\ttest_must_fail git add file\n    ++\ttest_must_fail git add file 2>out.add\n    ++'\n     +\n    ++test_lazy_prereq ADD_OUT 'test -e \"$TRASH_DIRECTORY\"/out.add'\n    ++test_expect_success ADD_OUT 'add output on unwritable repository' '\n     +\tcat >expect <<-\\EOF &&\n     +\terror: insufficient permission for adding an object to repository database .git/objects\n     +\terror: file: failed to insert into database\n     +\terror: unable to index file '\\''file'\\''\n     +\tfatal: updating files failed\n     +\tEOF\n    -+\ttest_must_fail git add file 2>actual &&\n    -+\ttest_cmp expect actual\n    ++\ttest_cmp expect out.add\n      '\n      \n      test_done\n2:  56b20f6024b ! 2:  e6cd47355d5 commit: fix duplication regression in permission error output\n    @@ object-store.h: int hash_object_file(const struct git_hash_algo *algo, const voi\n      \t\t\t       const char *type, struct object_id *oid,\n     \n      ## t/t0004-unwritable.sh ##\n    -@@ t/t0004-unwritable.sh: test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository\n    +@@ t/t0004-unwritable.sh: test_expect_success POSIXPERM,SANITY,!SANITIZE_LEAK 'commit should notice unwrit\n    + '\n      \n    + test_lazy_prereq COMMIT_OUT 'test -e \"$TRASH_DIRECTORY\"/out.commit'\n    +-test_expect_failure COMMIT_OUT 'commit output on unwritable repository' '\n    ++test_expect_success COMMIT_OUT 'commit output on unwritable repository' '\n      \tcat >expect <<-\\EOF &&\n      \terror: insufficient permission for adding an object to repository database .git/objects\n    --\terror: insufficient permission for adding an object to repository database .git/objects\n      \terror: Error building trees\n    - \tEOF\n    - \ttest_must_fail git commit -m second 2>actual &&\n-- \n2.33.0.1567.g7b23ce7ed9e\n\n"},{"id":"438564","messageId":"patch-v2-1.2-74bc5568c88-20211012T142950Z-avarab@gmail.com","threadId":"56674","inReplyTo":"cover-v2-0.2-00000000000-20211012T142950Z-avarab@gmail.com","subject":"[PATCH v2 1/2] unwritable tests: assert exact error output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-12T14:30:48Z","receivedAt":"2021-10-12T14:30:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preparation for fixing a regression where we started emitting some\nof these error messages twice, let's assert what the output from \"git\ncommit\" and friends is now in the case of permission errors.\n\nAs noted in [1] using test_expect_failure to mark up a TODO test has\nsome unexpected edge cases, e.g. we don't want to break --run=3 by\nskipping the \"test_lazy_prereq\" here. This pattern allows us to test\njust the test_cmp (and the \"cat\", which shouldn't fail) with the added\n\"test_expect_failure\", we'll flip that to a \"test_expect_success\" in\nthe next commit.\n\n1. https://lore.kernel.org/git/87tuhmk19c.fsf@evledraar.gmail.com/T/#u\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t0004-unwritable.sh | 47 +++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 43 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh\nindex 37d68ef03be..3627ce367c0 100755\n--- a/t/t0004-unwritable.sh\n+++ b/t/t0004-unwritable.sh\n@@ -19,27 +19,66 @@ test_expect_success setup '\n test_expect_success POSIXPERM,SANITY 'write-tree should notice unwritable repository' '\n \ttest_when_finished \"chmod 775 .git/objects .git/objects/??\" &&\n \tchmod a-w .git/objects .git/objects/?? &&\n-\ttest_must_fail git write-tree\n+\ttest_must_fail git write-tree 2>out.write-tree\n+'\n+\n+test_lazy_prereq WRITE_TREE_OUT 'test -e \"$TRASH_DIRECTORY\"/out.write-tree'\n+test_expect_success WRITE_TREE_OUT 'write-tree output on unwritable repository' '\n+\tcat >expect <<-\\EOF &&\n+\terror: insufficient permission for adding an object to repository database .git/objects\n+\tfatal: git-write-tree: error building trees\n+\tEOF\n+\ttest_cmp expect out.write-tree\n '\n \n test_expect_success POSIXPERM,SANITY,!SANITIZE_LEAK 'commit should notice unwritable repository' '\n \ttest_when_finished \"chmod 775 .git/objects .git/objects/??\" &&\n \tchmod a-w .git/objects .git/objects/?? &&\n-\ttest_must_fail git commit -m second\n+\ttest_must_fail git commit -m second 2>out.commit\n+'\n+\n+test_lazy_prereq COMMIT_OUT 'test -e \"$TRASH_DIRECTORY\"/out.commit'\n+test_expect_failure COMMIT_OUT 'commit output on unwritable repository' '\n+\tcat >expect <<-\\EOF &&\n+\terror: insufficient permission for adding an object to repository database .git/objects\n+\terror: Error building trees\n+\tEOF\n+\ttest_cmp expect out.commit\n '\n \n test_expect_success POSIXPERM,SANITY 'update-index should notice unwritable repository' '\n \ttest_when_finished \"chmod 775 .git/objects .git/objects/??\" &&\n \techo 6O >file &&\n \tchmod a-w .git/objects .git/objects/?? &&\n-\ttest_must_fail git update-index file\n+\ttest_must_fail git update-index file 2>out.update-index\n+'\n+\n+test_lazy_prereq UPDATE_INDEX_OUT 'test -e \"$TRASH_DIRECTORY\"/out.update-index'\n+test_expect_success UPDATE_INDEX_OUT 'update-index output on unwritable repository' '\n+\tcat >expect <<-\\EOF &&\n+\terror: insufficient permission for adding an object to repository database .git/objects\n+\terror: file: failed to insert into database\n+\tfatal: Unable to process path file\n+\tEOF\n+\ttest_cmp expect out.update-index\n '\n \n test_expect_success POSIXPERM,SANITY 'add should notice unwritable repository' '\n \ttest_when_finished \"chmod 775 .git/objects .git/objects/??\" &&\n \techo b >file &&\n \tchmod a-w .git/objects .git/objects/?? &&\n-\ttest_must_fail git add file\n+\ttest_must_fail git add file 2>out.add\n+'\n+\n+test_lazy_prereq ADD_OUT 'test -e \"$TRASH_DIRECTORY\"/out.add'\n+test_expect_success ADD_OUT 'add output on unwritable repository' '\n+\tcat >expect <<-\\EOF &&\n+\terror: insufficient permission for adding an object to repository database .git/objects\n+\terror: file: failed to insert into database\n+\terror: unable to index file '\\''file'\\''\n+\tfatal: updating files failed\n+\tEOF\n+\ttest_cmp expect out.add\n '\n \n test_done\n-- \n2.33.0.1567.g7b23ce7ed9e\n\n"},{"id":"438565","messageId":"patch-v2-2.2-e6cd47355d5-20211012T142950Z-avarab@gmail.com","threadId":"56674","inReplyTo":"cover-v2-0.2-00000000000-20211012T142950Z-avarab@gmail.com","subject":"[PATCH v2 2/2] commit: fix duplication regression in permission error output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-12T14:30:49Z","receivedAt":"2021-10-12T14:31:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a regression in the error output emitted when .git/objects can't\nbe written to. Before 9c4d6c0297b (cache-tree: Write updated\ncache-tree after commit, 2014-07-13) we'd emit only one \"insufficient\npermission\" error, now we'll do so again.\n\nThe cause is rather straightforward, we've got WRITE_TREE_SILENT for\nthe use-case of wanting to prepare an index silently, quieting any\npermission etc. error output. Then when we attempt to update to\nthat (possibly broken) index we'll run into the same errors again.\n\nBut with 9c4d6c0297b the gap between the cache-tree API and the object\nstore wasn't closed in terms of asking write_object_file() to be\nsilent. I.e. post-9c4d6c0297b the first call is to prepare_index(),\nand after that we'll call prepare_to_commit(). We only want verbose\nerror output from the latter.\n\nSo let's add and use that facility with a corresponding HASH_SILENT\nflag, its only user is cache-tree.c's update_one(), which will set it\nif its \"WRITE_TREE_SILENT\" flag is set.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n cache-tree.c          |  5 +++--\n cache.h               |  1 +\n object-file.c         | 20 ++++++++++++--------\n object-store.h        | 10 ++++++++--\n t/t0004-unwritable.sh |  2 +-\n 5 files changed, 25 insertions(+), 13 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 90919f9e345..f21bfc631c6 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -440,8 +440,9 @@ static int update_one(struct cache_tree *it,\n \t} else if (dryrun) {\n \t\thash_object_file(the_hash_algo, buffer.buf, buffer.len,\n \t\t\t\t tree_type, &it->oid);\n-\t} else if (write_object_file(buffer.buf, buffer.len, tree_type,\n-\t\t\t\t     &it->oid)) {\n+\t} else if (write_object_file_flags(buffer.buf, buffer.len, tree_type,\n+\t\t\t\t\t   &it->oid, flags & WRITE_TREE_SILENT\n+\t\t\t\t\t   ? HASH_SILENT : 0)) {\n \t\tstrbuf_release(&buffer);\n \t\treturn -1;\n \t}\ndiff --git a/cache.h b/cache.h\nindex d092820c943..dc58bfa48a0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -887,6 +887,7 @@ int ie_modified(struct index_state *, const struct cache_entry *, struct stat *,\n #define HASH_WRITE_OBJECT 1\n #define HASH_FORMAT_CHECK 2\n #define HASH_RENORMALIZE  4\n+#define HASH_SILENT 8\n 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);\n int index_path(struct index_state *istate, struct object_id *oid, const char *path, struct stat *st, unsigned flags);\n \ndiff --git a/object-file.c b/object-file.c\nindex 112d9b4badc..8fd67b07e45 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1873,7 +1873,7 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)\n \n static int write_loose_object(const struct object_id *oid, char *hdr,\n \t\t\t      int hdrlen, const void *buf, unsigned long len,\n-\t\t\t      time_t mtime)\n+\t\t\t      time_t mtime, unsigned flags)\n {\n \tint fd, ret;\n \tunsigned char compressed[4096];\n@@ -1887,7 +1887,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,\n \n \tfd = create_tmpfile(&tmp_file, filename.buf);\n \tif (fd < 0) {\n-\t\tif (errno == EACCES)\n+\t\tif (flags & HASH_SILENT)\n+\t\t\treturn -1;\n+\t\telse if (errno == EACCES)\n \t\t\treturn error(_(\"insufficient permission for adding an object to repository database %s\"), get_object_directory());\n \t\telse\n \t\t\treturn error_errno(_(\"unable to create temporary file\"));\n@@ -1937,7 +1939,8 @@ static int write_loose_object(const struct object_id *oid, char *hdr,\n \t\tstruct utimbuf utb;\n \t\tutb.actime = mtime;\n \t\tutb.modtime = mtime;\n-\t\tif (utime(tmp_file.buf, &utb) < 0)\n+\t\tif (utime(tmp_file.buf, &utb) < 0 &&\n+\t\t    !(flags & HASH_SILENT))\n \t\t\twarning_errno(_(\"failed utime() on %s\"), tmp_file.buf);\n \t}\n \n@@ -1962,8 +1965,9 @@ static int freshen_packed_object(const struct object_id *oid)\n \treturn 1;\n }\n \n-int write_object_file(const void *buf, unsigned long len, const char *type,\n-\t\t      struct object_id *oid)\n+int write_object_file_flags(const void *buf, unsigned long len,\n+\t\t\t    const char *type, struct object_id *oid,\n+\t\t\t    unsigned flags)\n {\n \tchar hdr[MAX_HEADER_LEN];\n \tint hdrlen = sizeof(hdr);\n@@ -1975,7 +1979,7 @@ int write_object_file(const void *buf, unsigned long len, const char *type,\n \t\t\t\t  &hdrlen);\n \tif (freshen_packed_object(oid) || freshen_loose_object(oid))\n \t\treturn 0;\n-\treturn write_loose_object(oid, hdr, hdrlen, buf, len, 0);\n+\treturn write_loose_object(oid, hdr, hdrlen, buf, len, 0, flags);\n }\n \n int hash_object_file_literally(const void *buf, unsigned long len,\n@@ -1995,7 +1999,7 @@ int hash_object_file_literally(const void *buf, unsigned long len,\n \t\tgoto cleanup;\n \tif (freshen_packed_object(oid) || freshen_loose_object(oid))\n \t\tgoto cleanup;\n-\tstatus = write_loose_object(oid, header, hdrlen, buf, len, 0);\n+\tstatus = write_loose_object(oid, header, hdrlen, buf, len, 0, 0);\n \n cleanup:\n \tfree(header);\n@@ -2017,7 +2021,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \tif (!buf)\n \t\treturn error(_(\"cannot read object for %s\"), oid_to_hex(oid));\n \thdrlen = xsnprintf(hdr, sizeof(hdr), \"%s %\"PRIuMAX , type_name(type), (uintmax_t)len) + 1;\n-\tret = write_loose_object(oid, hdr, hdrlen, buf, len, mtime);\n+\tret = write_loose_object(oid, hdr, hdrlen, buf, len, mtime, 0);\n \tfree(buf);\n \n \treturn ret;\ndiff --git a/object-store.h b/object-store.h\nindex 1e647a5be30..cea8d38bbd0 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -223,8 +223,14 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,\n \t\t     unsigned long len, const char *type,\n \t\t     struct object_id *oid);\n \n-int write_object_file(const void *buf, unsigned long len,\n-\t\t      const char *type, struct object_id *oid);\n+int write_object_file_flags(const void *buf, unsigned long len,\n+\t\t\t    const char *type, struct object_id *oid,\n+\t\t\t    unsigned flags);\n+static inline int write_object_file(const void *buf, unsigned long len,\n+\t\t\t\t    const char *type, struct object_id *oid)\n+{\n+\treturn write_object_file_flags(buf, len, type, oid, 0);\n+}\n \n int hash_object_file_literally(const void *buf, unsigned long len,\n \t\t\t       const char *type, struct object_id *oid,\ndiff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh\nindex 3627ce367c0..2e9d652d826 100755\n--- a/t/t0004-unwritable.sh\n+++ b/t/t0004-unwritable.sh\n@@ -38,7 +38,7 @@ test_expect_success POSIXPERM,SANITY,!SANITIZE_LEAK 'commit should notice unwrit\n '\n \n test_lazy_prereq COMMIT_OUT 'test -e \"$TRASH_DIRECTORY\"/out.commit'\n-test_expect_failure COMMIT_OUT 'commit output on unwritable repository' '\n+test_expect_success COMMIT_OUT 'commit output on unwritable repository' '\n \tcat >expect <<-\\EOF &&\n \terror: insufficient permission for adding an object to repository database .git/objects\n \terror: Error building trees\n-- \n2.33.0.1567.g7b23ce7ed9e\n\n"}]}