"needs update" considered harmful

"git update-index --refresh", "git reset" and "git add --refresh" have
reported paths that have local modifications as "needs update" since the
beginning of git.

Although this is logically correct in that you need to update the index at
that path before you can commit that change, it is now becoming more and
more clear, especially with the continuous push for user friendliness
since 1.5.0 series, that the message is suboptimal.  After all, the change
may be something the user might want to get rid of, and "updating" would
be absolutely a wrong thing to do if that is the case.

I prepared two alternatives to solve this.  Both aim to reword the message
to more neutral "locally modified".

This patch is a more intrusive variant that changes the message for only
Porcelain commands ("add" and "reset") while keeping the plumbing
"update-index" intact.

Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin-add.c b/builtin-add.c
index bf13aa3ad6e55075af4f727f73b8c41861b8693e..0800b14ac6b8bcc439f1c51c97f1fbc3ad61b79d 100644 (file)
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -142,7 +142,8 @@ static void refresh(int verbose, const char **pathspec)
        seen = xcalloc(specs, 1);
        if (read_cache() < 0)
                die("index file corrupt");
        seen = xcalloc(specs, 1);
        if (read_cache() < 0)
                die("index file corrupt");

-       refresh_index(&the_index, verbose ? 0 : REFRESH_QUIET, pathspec, seen);
+       refresh_index(&the_index, verbose ? REFRESH_SAY_CHANGED : REFRESH_QUIET,
+                     pathspec, seen);

        for (i = 0; i < specs; i++) {
                if (!seen[i])
                        die("pathspec '%s' did not match any files", pathspec[i]);
        for (i = 0; i < specs; i++) {
                if (!seen[i])
                        die("pathspec '%s' did not match any files", pathspec[i]);

diff --git a/builtin-reset.c b/builtin-reset.c
index 2e5a8867ac74fd1bc2ab8d5121baa7971b3b69d0..4d246c31b19381778848024ff49aebf3e77fdbbc 100644 (file)
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -96,7 +96,7 @@ static int update_index_refresh(int fd, struct lock_file *index_lock)
 
        if (read_cache() < 0)
                return error("Could not read index");
 
        if (read_cache() < 0)
                return error("Could not read index");

-       result = refresh_cache(0) ? 1 : 0;
+       result = refresh_cache(REFRESH_SAY_CHANGED) ? 1 : 0;

        if (write_cache(fd, active_cache, active_nr) ||
                        commit_locked_index(index_lock))
                return error ("Could not refresh index");
        if (write_cache(fd, active_cache, active_nr) ||
                        commit_locked_index(index_lock))
                return error ("Could not refresh index");

diff --git a/cache.h b/cache.h
index 305e2f701bd4fe3cbfb5a7cb37e75b73b67ee222..38985aa63eaa41ee7ddc322c682a7482a8204cbe 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -398,6 +398,7 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
 #define REFRESH_QUIET          0x0004  /* be quiet about it */
 #define REFRESH_IGNORE_MISSING 0x0008  /* ignore non-existent */
 #define REFRESH_IGNORE_SUBMODULES      0x0010  /* ignore submodules */
 #define REFRESH_QUIET          0x0004  /* be quiet about it */
 #define REFRESH_IGNORE_MISSING 0x0008  /* ignore non-existent */
 #define REFRESH_IGNORE_SUBMODULES      0x0010  /* ignore submodules */

+#define REFRESH_SAY_CHANGED    0x0020  /* say "changed" not "needs update" */

 extern int refresh_index(struct index_state *, unsigned int flags, const char **pathspec, char *seen);
 
 struct lock_file {
 extern int refresh_index(struct index_state *, unsigned int flags, const char **pathspec, char *seen);
 
 struct lock_file {

diff --git a/read-cache.c b/read-cache.c
index 16484281f7ea051a41e4352a3e6a1c0fdc746a9a..a50a851125db765f06f3869873623bf7592de405 100644 (file)
--- a/read-cache.c
+++ b/read-cache.c
@@ -980,7 +980,10 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
        int not_new = (flags & REFRESH_IGNORE_MISSING) != 0;
        int ignore_submodules = (flags & REFRESH_IGNORE_SUBMODULES) != 0;
        unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
        int not_new = (flags & REFRESH_IGNORE_MISSING) != 0;
        int ignore_submodules = (flags & REFRESH_IGNORE_SUBMODULES) != 0;
        unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;

+       const char *needs_update_message;

 
 

+       needs_update_message = ((flags & REFRESH_SAY_CHANGED)
+                               ? "locally modified" : "needs update");

        for (i = 0; i < istate->cache_nr; i++) {
                struct cache_entry *ce, *new;
                int cache_errno = 0;
        for (i = 0; i < istate->cache_nr; i++) {
                struct cache_entry *ce, *new;
                int cache_errno = 0;


@@ -1019,7 +1022,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
                        }
                        if (quiet)
                                continue;
                        }
                        if (quiet)
                                continue;

-                       printf("%s: needs update\n", ce->name);
+                       printf("%s: %s\n", ce->name, needs_update_message);

                        has_errors = 1;
                        continue;
                }
                        has_errors = 1;
                        continue;
                }

diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 0bfc1e9be49110ac0e8130fcdef440eecbaff2c9..29f5678b4c93485ad492fa865a5da58a3cc05b7c 100755 (executable)
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -419,7 +419,7 @@ test_expect_success 'resetting an unmodified path is a no-op' '
 '
 
 cat > expect << EOF
 '
 
 cat > expect << EOF

-file2: needs update
+file2: locally modified

 EOF
 
 test_expect_success '--mixed refreshes the index' '
 EOF
 
 test_expect_success '--mixed refreshes the index' '