{"thread":{"id":"56856","subject":"[PATCH] stash: show error message when lockfile is present","startedAt":"2021-11-07T21:30:57Z","lastAt":"2021-11-08T20:36:21Z","messageCount":8,"participants":["Birk Tjelmeland","Taylor Blau","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"440644","messageId":"20211107213012.6978-1-git@birktj.no","threadId":"56856","inReplyTo":null,"subject":"[PATCH] stash: show error message when lockfile is present","fromName":"Birk Tjelmeland","fromEmail":"git@birktj.no","sentAt":"2021-11-07T21:30:12Z","receivedAt":"2021-11-07T21:30:57Z","isPatch":true,"sender":{"key":"git@birktj.no","avatar":null},"body":"Multiple git-stash commands silently fail when index.lock is present,\nincluding git stash push and git stash apply. This is somewhat confusing\nand a better behaviour would probably be to exit with a meaningful error\nmessage like most other git commands do.\n\nThis patch updates repo_refresh_and_write_index to accept another\nparameter lock_flags and updates some callsites of this function to call\nit with LOCK_REPORT_ON_ERROR resulting a suitable error message when the\nrelevant git-stash commands used on a repo with an index.lock file.\n\nThis patch only adds the described error message to git-stash commands,\nhowever the diff highlights other uses of repo_refresh_and_write_index\nwhich could also benefit from the changes. On the other hand these\ncallsites already have some limited error messages.\n\nSigned-off-by: Birk Tjelmeland <git@birktj.no>\n---\n add-interactive.c | 4 ++--\n add-patch.c       | 4 ++--\n builtin/am.c      | 2 +-\n builtin/merge.c   | 4 ++--\n builtin/stash.c   | 6 +++---\n cache.h           | 4 ++--\n read-cache.c      | 3 ++-\n 7 files changed, 14 insertions(+), 13 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 6498ae196f..e51d19e1b5 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -810,7 +810,7 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps,\n \t\t\t\t       COMMIT_LOCK) < 0)\n \t\tres = -1;\n \telse\n-\t\tres = repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0, 1,\n+\t\tres = repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0, 0, 1,\n \t\t\t\t\t\t   NULL, NULL, NULL);\n \n \tif (!res)\n@@ -1150,7 +1150,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)\n \n \tif (discard_index(r->index) < 0 ||\n \t    repo_read_index(r) < 0 ||\n-\t    repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 1,\n+\t    repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 0, 1,\n \t\t\t\t\t NULL, NULL, NULL) < 0)\n \t\twarning(_(\"could not refresh index\"));\n \ndiff --git a/add-patch.c b/add-patch.c\nindex 8c41cdfe39..d8000f02d4 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1680,7 +1680,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\terror(_(\"'git apply' failed\"));\n \t\t}\n \t\tif (repo_read_index(s->s.r) >= 0)\n-\t\t\trepo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,\n+\t\t\trepo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0, 0,\n \t\t\t\t\t\t     1, NULL, NULL, NULL);\n \t}\n \n@@ -1733,7 +1733,7 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \n \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0 ||\n \t    (!s.mode->index_only &&\n-\t     repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 1,\n+\t     repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 0, 1,\n \t\t\t\t\t  NULL, NULL, NULL) < 0) ||\n \t    parse_diff(&s, ps) < 0) {\n \t\tadd_p_state_clear(&s);\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 8677ea2348..a0dead658c 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1750,7 +1750,7 @@ static void am_run(struct am_state *state, int resume)\n \n \tunlink(am_path(state, \"dirtyindex\"));\n \n-\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0) < 0)\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0, 0) < 0)\n \t\tdie(_(\"unable to write index file\"));\n \n \tif (repo_index_has_changes(the_repository, NULL, &sb)) {\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex ea3112e0c0..9d168ee965 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -714,7 +714,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n {\n \tconst char *head_arg = \"HEAD\";\n \n-\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)\n+\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0, 0) < 0)\n \t\treturn error(_(\"Unable to write index.\"));\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\") ||\n@@ -900,7 +900,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n \tstruct object_id result_tree, result_commit;\n \tstruct commit_list *parents, **pptr = &parents;\n \n-\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)\n+\tif (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0, 0) < 0)\n \t\treturn error(_(\"Unable to write index.\"));\n \n \twrite_tree_trivial(&result_tree);\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex a0ccc8654d..977fcc4e40 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -501,7 +501,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \tconst struct object_id *bases[1];\n \n \tread_cache_preload(NULL);\n-\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0, LOCK_REPORT_ON_ERROR, 0))\n \t\treturn -1;\n \n \tif (write_cache_as_tree(&c_tree, 0, NULL))\n@@ -1277,7 +1277,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n \tprepare_fallback_ident(\"git stash\", \"git@stash\");\n \n \tread_cache_preload(NULL);\n-\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0) < 0) {\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0, LOCK_REPORT_ON_ERROR, 0) < 0) {\n \t\tret = -1;\n \t\tgoto done;\n \t}\n@@ -1443,7 +1443,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\tfree(ps_matched);\n \t}\n \n-\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) {\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0, LOCK_REPORT_ON_ERROR, 0)) {\n \t\tret = -1;\n \t\tgoto done;\n \t}\ndiff --git a/cache.h b/cache.h\nindex eba12487b9..fee557f409 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -444,7 +444,7 @@ extern struct index_state the_index;\n #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))\n #define chmod_cache_entry(ce, flip) chmod_index_entry(&the_index, (ce), (flip))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n-#define refresh_and_write_cache(refresh_flags, write_flags, gentle) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_flags), (gentle), NULL, NULL, NULL)\n+#define refresh_and_write_cache(refresh_flags, write_flags, lock_flags, gentle) repo_refresh_and_write_index(the_repository, (refresh_flags), (write_flags), (lock_flags), (gentle), NULL, NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n #define cache_dir_exists(name, namelen) index_dir_exists(&the_index, (name), (namelen))\n@@ -933,7 +933,7 @@ int refresh_index(struct index_state *, unsigned int flags, const struct pathspe\n  * Note that if refreshing the index returns an error, we still write\n  * out the index (unless locking fails).\n  */\n-int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, int gentle, const struct pathspec *, char *seen, const char *header_msg);\n+int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, int lock_flags, int gentle, const struct pathspec *, char *seen, const char *header_msg);\n \n struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex f398659662..01f2a2d470 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1540,6 +1540,7 @@ static void show_file(const char * fmt, const char * name, int in_porcelain,\n int repo_refresh_and_write_index(struct repository *repo,\n \t\t\t\t unsigned int refresh_flags,\n \t\t\t\t unsigned int write_flags,\n+\t\t\t\t int lock_flags,\n \t\t\t\t int gentle,\n \t\t\t\t const struct pathspec *pathspec,\n \t\t\t\t char *seen, const char *header_msg)\n@@ -1547,7 +1548,7 @@ int repo_refresh_and_write_index(struct repository *repo,\n \tstruct lock_file lock_file = LOCK_INIT;\n \tint fd, ret = 0;\n \n-\tfd = repo_hold_locked_index(repo, &lock_file, 0);\n+\tfd = repo_hold_locked_index(repo, &lock_file, lock_flags);\n \tif (!gentle && fd < 0)\n \t\treturn -1;\n \tif (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))\n-- \n2.33.1\n\n"},{"id":"440660","messageId":"YYiXw41upJfPS7l0@nand.local","threadId":"56856","inReplyTo":"20211107213012.6978-1-git@birktj.no","subject":"Re: [PATCH] stash: show error message when lockfile is present","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-08T03:21:39Z","receivedAt":"2021-11-08T03:21:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Nov 07, 2021 at 10:30:12PM +0100, Birk Tjelmeland wrote:\n> Multiple git-stash commands silently fail when index.lock is present,\n> including git stash push and git stash apply. This is somewhat confusing\n> and a better behaviour would probably be to exit with a meaningful error\n> message like most other git commands do.\n\nWe do terminate with non-zero exit code when trying to, for e.g., 'git\nstash push' when $GIT_DIR/index.lock already exists. That is reflected\nin your patch by not adding any new paths which we return, which makes\nsense.\n\n> This patch updates repo_refresh_and_write_index to accept another\n> parameter lock_flags and updates some callsites of this function to call\n> it with LOCK_REPORT_ON_ERROR resulting a suitable error message when the\n> relevant git-stash commands used on a repo with an index.lock file.\n>\n> This patch only adds the described error message to git-stash commands,\n> however the diff highlights other uses of repo_refresh_and_write_index\n> which could also benefit from the changes. On the other hand these\n> callsites already have some limited error messages.\n\nI wonder if there are callers of repo_refresh_and_write_index() that\ndon't want any errors reported. Not having thought about it too hard\n(much less looked through any of these callers), I would expect that\nhaving the choice to either error() or die() is something worth keeping.\nBut I do not know if there are callers which want neither.\n\n> Signed-off-by: Birk Tjelmeland <git@birktj.no>\n> ---\n>  add-interactive.c | 4 ++--\n>  add-patch.c       | 4 ++--\n>  builtin/am.c      | 2 +-\n>  builtin/merge.c   | 4 ++--\n>  builtin/stash.c   | 6 +++---\n>  cache.h           | 4 ++--\n>  read-cache.c      | 3 ++-\n>  7 files changed, 14 insertions(+), 13 deletions(-)\n\nIt looks like the sum-total of this patch are a few things:\n\n  - repo_refresh_and_write_index() gets a new lock_flags parameter which\n    is passed down to repo_hold_locked_index()\n\n  - refresh_and_write_cache() which is a thin wrapper around\n    repo_refresh_and_write_index() also learned the new parameter\n\n  - do_apply_stash(), do_create_stash(), do_push_stash() all pass\n    LOCK_REPORT_ON_ERROR via the new lock_flags parameter\n\nThat all makes sense to me. It results in us printing a helpful error\nmessage when we couldn't acquire an exclusive lock on\n$GIT_DIR/index.lock where before we would have silently failed and\nexited non-zero (which is not exactly a *silent* failure, but it is\nclose).\n\nThis patch does not include any tests, which I think that you should add\nin another revision before we consider queuing this.\n\nThanks,\nTaylor\n"},{"id":"440662","messageId":"YYike53eJEjSX3er@nand.local","threadId":"56856","inReplyTo":"YYiXw41upJfPS7l0@nand.local","subject":"Re: [PATCH] stash: show error message when lockfile is present","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-08T04:15:55Z","receivedAt":"2021-11-08T04:16:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"I forgot to mention in my earlier email, but in this hunk:\n\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index a0ccc8654d..977fcc4e40 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -501,7 +501,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>         const struct object_id *bases[1];\n>\n>         read_cache_preload(NULL);\n> -       if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n> +       if (refresh_and_write_cache(REFRESH_QUIET, 0, LOCK_REPORT_ON_ERROR, 0))\n>                 return -1;\n>\n>         if (write_cache_as_tree(&c_tree, 0, NULL))\n\nNot the fault of your patch, but this hunk is unlike the others in that\nit only checks the return value of refresh_and_write_cache() is\nnon-zero, not non-negative.\n\nLooking through refresh_and_write_cache(), we can return a non-zero\nvalue in any one of three cases:\n\n  - We could not acquire the index.lock file with\n    repo_hold_locked_index(), or\n\n  - We failed to write the index (indicated by write_locked_index()\n    failing), or\n\n  - refresh_index() returned a non-*zero* value, which happens when it\n    sets its `has_errors` variable to 1.\n\nSo because even non-zero positive return values from this function\nindicate an error, this is OK. In other words, the current\nimplementation of refresh_and_write_cache() (and the functions that it\ncalls) make it so that it doesn't matter if you check whether the return\nvalue is negative, or non-zero.\n\nBut at least for consistency with the other callers (not to mention\nsaving future readers in this area the same thought process I just wrote\ndown here) it may be worth changing this to:\n\n    if (refresh_and_write_cache(...) < 0)\n      return -1;\n\nThanks,\nTaylor\n"},{"id":"440663","messageId":"xmqqlf1zunqe.fsf@gitster.g","threadId":"56856","inReplyTo":"YYiXw41upJfPS7l0@nand.local","subject":"Re: [PATCH] stash: show error message when lockfile is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-08T07:10:17Z","receivedAt":"2021-11-08T07:10:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> I wonder if there are callers of repo_refresh_and_write_index() that\n> don't want any errors reported. Not having thought about it too hard\n> (much less looked through any of these callers), I would expect that\n> having the choice to either error() or die() is something worth keeping.\n> But I do not know if there are callers which want neither.\n> ...\n>>  add-interactive.c | 4 ++--\n>>  add-patch.c       | 4 ++--\n>>  builtin/am.c      | 2 +-\n>>  builtin/merge.c   | 4 ++--\n>>  builtin/stash.c   | 6 +++---\n>>  cache.h           | 4 ++--\n>>  read-cache.c      | 3 ++-\n>>  7 files changed, 14 insertions(+), 13 deletions(-)\n\nI think most of the changes in this patch, other than the ones to\nbuiltin/stash.c, are unwanted, and I suspect what you wondered above\nmay be the same thing.  Take for example this hunk:\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex a0ccc8654d..977fcc4e40 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -501,7 +501,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \tconst struct object_id *bases[1];\n \n \tread_cache_preload(NULL);\n-\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n+\tif (refresh_and_write_cache(REFRESH_QUIET, 0, LOCK_REPORT_ON_ERROR, 0))\n \t\treturn -1;\n \n \tif (write_cache_as_tree(&c_tree, 0, NULL))\n\nTelling the function to be quiet and at the same time be noisy on\nonly one particular kind of error sounds somewhat strange.  I do not\nthink of any reason why we should believe that failing to lock will\nbe the only special kind of failure to be of interest to the users.\n\nI would think the \"fix\" should look more like this:\n\n \tread_cache_preload(NULL);\n\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n- \t\treturn -1;\n+ \t\treturn error(_(\"failed to refresh the index\"));\n\nThat is, tell the function that the caller will do the error\nreporting (i.e. \"QUIET\") and do so.\n\nThanks.\n"},{"id":"440668","messageId":"YYlkumkOWotx6umn@nand.local","threadId":"56856","inReplyTo":"xmqqlf1zunqe.fsf@gitster.g","subject":"Re: [PATCH] stash: show error message when lockfile is present","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-08T17:56:10Z","receivedAt":"2021-11-08T17:56:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Nov 07, 2021 at 11:10:17PM -0800, Junio C Hamano wrote:\n> I would think the \"fix\" should look more like this:\n>\n>  \tread_cache_preload(NULL);\n> \tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n> - \t\treturn -1;\n> + \t\treturn error(_(\"failed to refresh the index\"));\n>\n> That is, tell the function that the caller will do the error\n> reporting (i.e. \"QUIET\") and do so.\n\nThanks; this is a much better approach. It makes Birk's change much more\ndirect, and does not require plumbing a new parameter through many\nfunction calls.\n\nDoing something like this (with appropriate tests) would look good to\nme.\n\nThanks,\nTaylor\n"},{"id":"440671","messageId":"211108.868rxyfote.gmgdl@evledraar.gmail.com","threadId":"56856","inReplyTo":"xmqqlf1zunqe.fsf@gitster.g","subject":"Re: [PATCH] stash: show error message when lockfile is present","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-08T19:05:47Z","receivedAt":"2021-11-08T19:08:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Nov 07 2021, Junio C Hamano wrote:\n\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n>> I wonder if there are callers of repo_refresh_and_write_index() that\n>> don't want any errors reported. Not having thought about it too hard\n>> (much less looked through any of these callers), I would expect that\n>> having the choice to either error() or die() is something worth keeping.\n>> But I do not know if there are callers which want neither.\n>> ...\n>>>  add-interactive.c | 4 ++--\n>>>  add-patch.c       | 4 ++--\n>>>  builtin/am.c      | 2 +-\n>>>  builtin/merge.c   | 4 ++--\n>>>  builtin/stash.c   | 6 +++---\n>>>  cache.h           | 4 ++--\n>>>  read-cache.c      | 3 ++-\n>>>  7 files changed, 14 insertions(+), 13 deletions(-)\n>\n> I think most of the changes in this patch, other than the ones to\n> builtin/stash.c, are unwanted, and I suspect what you wondered above\n> may be the same thing.  Take for example this hunk:\n>\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index a0ccc8654d..977fcc4e40 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -501,7 +501,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>  \tconst struct object_id *bases[1];\n>  \n>  \tread_cache_preload(NULL);\n> -\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n> +\tif (refresh_and_write_cache(REFRESH_QUIET, 0, LOCK_REPORT_ON_ERROR, 0))\n>  \t\treturn -1;\n>  \n>  \tif (write_cache_as_tree(&c_tree, 0, NULL))\n>\n> Telling the function to be quiet and at the same time be noisy on\n> only one particular kind of error sounds somewhat strange.  I do not\n> think of any reason why we should believe that failing to lock will\n> be the only special kind of failure to be of interest to the users.\n>\n> I would think the \"fix\" should look more like this:\n>\n>  \tread_cache_preload(NULL);\n> \tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n> - \t\treturn -1;\n> + \t\treturn error(_(\"failed to refresh the index\"));\n>\n> That is, tell the function that the caller will do the error\n> reporting (i.e. \"QUIET\") and do so.\n>\n> Thanks.\n\nWe shouldn't be doing that because we won't get an error that's as\nmeaningful as what we'll get from unable_to_lock_message().\n\nI think the patch as-is is taking the right approach. It would be nice\nto see a re-indentation of the argument list, and perhaps we should\nprovide another macro name for this one caller, but those are all nits.\n\nThe \"quiet\" here is orthagonal, it's to disable the chatty output from\nread-cache.c.\n"},{"id":"440678","messageId":"xmqqsfw6tnu7.fsf@gitster.g","threadId":"56856","inReplyTo":"211108.868rxyfote.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] stash: show error message when lockfile is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-08T20:05:36Z","receivedAt":"2021-11-08T20:05:40Z","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> The \"quiet\" here is orthagonal, it's to disable the chatty output from\n> read-cache.c.\n\nIt shouldn't be.  read-cache.c may want to report errors and\nprogresses, and among the errors it may not include lock-related\nthings.  If the change were to (1) extend the flag word to make it\nmore uniform, so that it does not special case ONLY the lock-related\nerrors, and to (2) figure out how to grab richer diagnoses out of\nthe lower level API functions, and to (3) reduce the messages issued\nby the lower-level API functions and have it callers' responsibility\nto give the messages, that would have been palatable, but the patch\nas posted goes totally the opposite direction on all these points.\n\nYuck.\n"},{"id":"440686","messageId":"0777e271-d5cc-45b2-97b2-7597cf75ad86@www.fastmail.com","threadId":"56856","inReplyTo":"xmqqsfw6tnu7.fsf@gitster.g","subject":"Re: [PATCH] stash: show error message when lockfile is present","fromName":"Birk Tjelmeland","fromEmail":"git@birktj.no","sentAt":"2021-11-08T20:35:01Z","receivedAt":"2021-11-08T20:36:21Z","isPatch":true,"sender":{"key":"git@birktj.no","avatar":null},"body":"> I would think the \"fix\" should look more like this:\n>\n>  \tread_cache_preload(NULL);\n> \tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n> - \t\treturn -1;\n> + \t\treturn error(_(\"failed to refresh the index\"));\n\nThis is how the other callsites of refresh_and_write_cache (the ones\noutside of stash.c) solve this problem, however I agree with Ævar that\nthis error is not as useful as the one given by\nLOCK_REPORT_ON_ERROR (which is why I wrote the patch this way\nto begin with, although I do see that it is not exactly the most elegant\nsolution to the problem)\n\n> If the change were to (1) extend the flag word to make it more uniform,\n> so that it does not special case ONLY the lock-related errors\n\nWould adding a REFRESH_REPORT_ERRORS flag which could then be\nused with repo_refresh_and_write_index be a more acceptable solution? \nThen various functions in read-cache.c could check for this flag and\noutput errors but not progress when it is present.\n\nRegards,\nBirk\n"}]}