uniform error messages for index read and write
cover uniform error messages for index read and write
Currently we use different messages for the same error. Make them uniform.
Michael J Gruber (2):
show-index: uniform error messages for index read
messages: uniform error messages for index write
builtin/add.c | 2 +- builtin/apply.c | 2 +- builtin/checkout-index.c | 2 +- builtin/checkout.c | 4 ++-- builtin/clone.c | 2 +- builtin/commit.c | 17 ++++++++++------- builtin/merge.c | 2 +- builtin/mv.c | 2 +- builtin/read-tree.c | 2 +- builtin/rm.c | 2 +- builtin/update-index.c | 2 +- merge-recursive.c | 2 +- merge.c | 2 +- rerere.c | 2 +- sequencer.c | 5 +++-- show-index.c | 4 ++-- 16 files changed, 29 insertions(+), 25 deletions(-)
Signature · 1 line
2.4.2.548.g1e81565
PATCH 1/2 show-index: uniform error messages for index read
Currently, we have different wordings for the same index read error message, which may be confusing to users and increase3s the risk of more severely different translated messages:
builtin/apply.c: die(_("unable to read index file"));show-index.c: die("unable to read index");show-index.c: die("unable to read index");test-dump-cache-tree.c: die("unable to read index file");test-dump-untracked-cache.c: die("unable to read index file");test-scrap-cache-tree.c: die("unable to read index file");builtin/commit.c: die(_("Cannot read index"));rerere.c: return error("Could not read index");rerere.c: return error("Could not read index");rerere.c: return error("Could not read index");
Turn all of them into "unable to read index file" except for the rerere
messages: They appear on a higher level (index file access + parsing) and are worded similarly to other rerere messages.
--- builtin/commit.c | 2 +- show-index.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
builtin/commit.c +1 -1
@@ -878,7 +878,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,const char *parent ="HEAD" ;if (!active_nr &&read_cache () <0 ) -die (_ ("Cannot read index" )); +die (_ ("unable to read index file" ));if (amend) parent ="HEAD^1" ;
show-index.c +2 -2
@@ -22,11 +22,11 @@ int main(int argc, char **argv)if (version <2 || version >2 )die ("unknown index version" );if (fread (top_index,256 *4 ,1 , stdin) !=1 ) -die ("unable to read index" ); +die ("unable to read index file" ); }else { version =1 ;if (fread (&top_index[2 ],254 *4 ,1 , stdin) !=1 ) -die ("unable to read index" ); +die ("unable to read index file" ); } nr =0 ;for (i =0 ; i <256 ; i++) {
Signature · 1 line
2.4.2.548.g1e81565
PATCH 2/2 messages: uniform error messages for index write
In reply to Michael J GruberCurrently, we have different wordings for the same index write error message, which may be confusing to users and increases the risk of more severely different translated messages:
builtin/add.c: die(_("Unable to write new index file"));builtin/apply.c: die(_("Unable to write new index file"));builtin/checkout.c: die(_("unable to write new index file"));builtin/checkout-index.c: die("Unable to write new index file");builtin/clone.c: die(_("unable to write new index file"));builtin/commit.c: die(_("unable to create temporary index"));builtin/commit.c: die(_("unable to update temporary index file"));builtin/commit.c: die(_("unable to write index file"));builtin/commit.c: die(_("unable to write new_index file"));builtin/commit.c: die(_("unable to write new_index file"));builtin/commit.c: die(_("unable to write new_index file"));builtin/commit.c: die(_("unable to write temporary index file"));builtin/commit.c: "new_index file. Check that disk is not full and quota is\n"builtin/merge.c: return error(_("Unable to write index."));builtin/mv.c: die(_("Unable to write new index file"));builtin/read-tree.c: die("unable to write new index file");builtin/rm.c: die(_("Unable to write new index file"));builtin/update-index.c: die("Unable to write new index file");merge.c: die(_("unable to write new index file"));merge-recursive.c: return error(_("Unable to write index."));pack-write.c: die_errno("unable to create '%s'", index_name);rerere.c: die("Unable to write new index file");sequencer.c: die(_("%s: Unable to write new index file"), action_name(opts));test-scrap-cache-tree.c: die("unable to write index file");
Except for "checkout", "new index" always refers to the new state of the index file, which is extra confusing. "checkout" does not talk about "new work tree" (but "work tree") either. Therefore, word all of these as "unable to write index file" (by possibly omitting "new" or "new_", adjusting capitalization and punctuation, turning "create" into "write"), except for:
* Leave "temporary" in place and add TRANSLATORS note.
It may happen that the index file is writable but not the temporary one.
* Leave pack-write.c alone.
It is low level without l10n.
---
I am actually a bit torn on the "temporary index" messages. On the one hand we should hide technicalities from the user; on the other hand, we should give helpful information for debugging.
Maybe "new index" and "temporary index" are really in the same camp? I would omit "temporary" in that case, too.
builtin/add.c | 2 +- builtin/apply.c | 2 +- builtin/checkout-index.c | 2 +- builtin/checkout.c | 4 ++-- builtin/clone.c | 2 +- builtin/commit.c | 15 +++++++++------ builtin/merge.c | 2 +- builtin/mv.c | 2 +- builtin/read-tree.c | 2 +- builtin/rm.c | 2 +- builtin/update-index.c | 2 +- merge-recursive.c | 2 +- merge.c | 2 +- rerere.c | 2 +- sequencer.c | 5 +++-- 15 files changed, 26 insertions(+), 22 deletions(-)
builtin/add.c +1 -1
@@ -445,7 +445,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) finish:if (active_cache_changed) {if (write_locked_index (&the_index, &lock_file,COMMIT_LOCK )) -die (_ ("Unable to write new index file" )); +die (_ ("unable to write index file" )); }return exit_status;
builtin/apply.c +1 -1
@@ -4672,7 +4672,7 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)if (update_index) {if (write_locked_index (&the_index, &lock_file,COMMIT_LOCK )) -die (_ ("Unable to write new index file" )); +die (_ ("unable to write index file" )); }return !!errs;
builtin/checkout-index.c +1 -1
@@ -279,6 +279,6 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)if (0 <= newfd &&write_locked_index (&the_index, &lock_file,COMMIT_LOCK )) -die ("Unable to write new index file" ); +die ("unable to write index file" );return 0 ; }
builtin/checkout.c +2 -2
@@ -380,7 +380,7 @@ static int checkout_paths(const struct checkout_opts *opts, }if (write_locked_index (&the_index, lock_file,COMMIT_LOCK )) -die (_ ("unable to write new index file" )); +die (_ ("unable to write index file" ));read_ref_full ("HEAD" ,0 , rev, &flag); head =lookup_commit_reference_gently (rev,1 );
@@ -591,7 +591,7 @@ static int merge_working_tree(const struct checkout_opts *opts,cache_tree_update (&the_index,WRITE_TREE_SILENT |WRITE_TREE_REPAIR );if (write_locked_index (&the_index, lock_file,COMMIT_LOCK )) -die (_ ("unable to write new index file" )); +die (_ ("unable to write index file" ));if (!opts->force && !opts->quiet)show_local_changes (&new->commit->object, &opts->diff_options);
builtin/clone.c +1 -1
@@ -660,7 +660,7 @@ static int checkout(void)die (_ ("unable to checkout working tree" ));if (write_locked_index (&the_index, lock_file,COMMIT_LOCK )) -die (_ ("unable to write new index file" )); +die (_ ("unable to write index file" )); err |=run_hook_le (NULL ,"post-checkout" ,sha1_to_hex (null_sha1),sha1_to_hex (sha1),"1" ,NULL );
builtin/commit.c +9 -6
@@ -341,7 +341,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefixrefresh_cache_or_die (refresh_flags);if (write_locked_index (&the_index, &index_lock,CLOSE_LOCK )) -die (_ ("unable to create temporary index" )); +/* TRANSLATORS: match with "unable to write index file" */ +die (_ ("unable to write temporary index file" )); old_index_env =getenv (INDEX_ENVIRONMENT );setenv (INDEX_ENVIRONMENT , index_lock.filename.buf,1 );
@@ -360,7 +361,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefixif (reopen_lock_file (&index_lock) <0 )die (_ ("unable to write index file" ));if (write_locked_index (&the_index, &index_lock,CLOSE_LOCK )) -die (_ ("unable to update temporary index" )); +die (_ ("unable to write temporary index file" )); }else warning (_ ("Failed to update main cache tree" ));
@@ -386,7 +387,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefixrefresh_cache_or_die (refresh_flags);update_main_cache_tree (WRITE_TREE_SILENT );if (write_locked_index (&the_index, &index_lock,CLOSE_LOCK )) -die (_ ("unable to write new_index file" )); +die (_ ("unable to write index file" )); commit_style =COMMIT_NORMAL ;return index_lock.filename.buf; }
@@ -411,7 +412,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefixif (active_cache_changed) {if (write_locked_index (&the_index, &index_lock,COMMIT_LOCK )) -die (_ ("unable to write new_index file" )); +die (_ ("unable to write index file" )); }else {rollback_lock_file (&index_lock); }
@@ -460,7 +461,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefixrefresh_cache (REFRESH_QUIET );update_main_cache_tree (WRITE_TREE_SILENT );if (write_locked_index (&the_index, &index_lock,CLOSE_LOCK )) -die (_ ("unable to write new_index file" )); +die (_ ("unable to write index file" )); hold_lock_file_for_update(&false_lock, git_path("next-index-%"PRIuMAX,
@@ -472,6 +473,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefixrefresh_cache (REFRESH_QUIET );if (write_locked_index (&the_index, &false_lock,CLOSE_LOCK )) +/* TRANSLATORS: match with "unable to write index file" */ die (_ ("unable to write temporary index file" ));discard_cache ();
@@ -1783,8 +1785,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)unlink (git_path ("SQUASH_MSG" ));if (commit_index_files ()) +/* TRANSLATORS: match with "unable to write index file" */ die (_ ("Repository has been updated, but unable to write\n" -"new_index file. Check that disk is not full and quota is\n" +"index file. Check that disk is not full and quota is\n" "not exceeded, and then \"git reset HEAD\" to recover." ));rerere (0 );
builtin/merge.c +1 -1
@@ -642,7 +642,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,refresh_cache (REFRESH_QUIET );if (active_cache_changed &&write_locked_index (&the_index, &lock,COMMIT_LOCK )) -return error (_ ("Unable to write index." )); +return error (_ ("unable to write index file" ));rollback_lock_file (&lock);if (!strcmp (strategy,"recursive" ) || !strcmp (strategy,"subtree" )) {
builtin/mv.c +1 -1
@@ -276,7 +276,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)if (active_cache_changed &&write_locked_index (&the_index, &lock_file,COMMIT_LOCK )) -die (_ ("Unable to write new index file" )); +die (_ ("unable to write index file" ));return 0 ; }
builtin/read-tree.c +1 -1
@@ -244,6 +244,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)prime_cache_tree (&the_index, trees[0 ]);if (write_locked_index (&the_index, &lock_file,COMMIT_LOCK )) -die ("unable to write new index file" ); +die ("unable to write index file" );return 0 ; }
builtin/rm.c +1 -1
@@ -427,7 +427,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)if (active_cache_changed) {if (write_locked_index (&the_index, &lock_file,COMMIT_LOCK )) -die (_ ("Unable to write new index file" )); +die (_ ("unable to write index file" )); }return 0 ;
builtin/update-index.c +1 -1
@@ -1134,7 +1134,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)unable_to_lock_die (get_index_file (), lock_error); }if (write_locked_index (&the_index, lock_file,COMMIT_LOCK )) -die ("Unable to write new index file" ); +die ("unable to write index file" ); }rollback_lock_file (lock_file);
merge-recursive.c +1 -1
@@ -2015,7 +2015,7 @@ int merge_recursive_generic(struct merge_options *o, result);if (active_cache_changed &&write_locked_index (&the_index, lock,COMMIT_LOCK )) -return error (_ ("Unable to write index." )); +return error (_ ("unable to write index file" ));return clean ?0 :1 ; }
merge.c +1 -1
@@ -91,6 +91,6 @@ int checkout_fast_forward(const unsigned char *head,if (unpack_trees (nr_trees, t, &opts))return -1 ;if (write_locked_index (&the_index, lock_file,COMMIT_LOCK )) -die (_ ("unable to write new index file" )); +die (_ ("unable to write index file" ));return 0 ; }
rerere.c +1 -1
@@ -491,7 +491,7 @@ static void update_paths(struct string_list *update)if (active_cache_changed) {if (write_locked_index (&the_index, &index_lock,COMMIT_LOCK )) -die ("Unable to write new index file" ); +die ("unable to write index file" ); }else rollback_lock_file (&index_lock); }
sequencer.c +3 -2
@@ -317,8 +317,9 @@ static int do_recursive_merge(struct commit *base, struct commit *next,if (active_cache_changed &&write_locked_index (&the_index, &index_lock,COMMIT_LOCK )) -/* TRANSLATORS: %s will be "revert" or "cherry-pick" */ -die (_ ("%s: Unable to write new index file" ),action_name (opts)); +/* TRANSLATORS: %s will be "revert" or "cherry-pick" +match this with "unable to write index file" */ +die (_ ("%s: unable to write index file" ),action_name (opts));rollback_lock_file (&index_lock);if (opts->signoff)
Signature · 1 line
2.4.2.548.g1e81565
Re: [RFC/PATCH 2/2] messages: uniform error messages for index write
Quoted 7 lines · Michael J Gruber
* Leave "temporary" in place and add TRANSLATORS note.
It may happen that the index file is writable but not the temporary one.* Leave
pack-write.calone.
It is low level without l10n.
Quoted 8 lines
Signed-off-by Michael J Gruber <[email protected]>---
I am actually a bit torn on the "temporary index" messages. On the one hand we should hide technicalities from the user; on the other hand, we should give helpful information for debugging.Maybe "new index" and "temporary index" are really in the same camp? I would omit "temporary" in that case, too.
It is true that we never overwrite the file in-place, but instead create a new file and then rename it to the destiation. So in that sense, we are always writing "new index file".
There however indeed is a different kind of "temporary" index involved, when you are doing "git commit paths" to create a partial commit. We populate a pristine (relative to HEAD) index, update the contents of it with the working tree contents at specified paths in order to write the tree out, but do not use that index after that. The real index is updated the same way for the specified paths to match what was committed, but what was added to the real index outside the specified paths before the partial commit was made is all kept in the real index. The presense of this "temporary" index is something the end users may have to be aware of, if they want to peek into the state of the index from inside their hooks, so in that sense, it may make sense to include the word "temporary" in the error message in that codepath.