[PATCH] refresh-index: fix bitmask assignment

[PATCH] refresh-index: fix bitmask assignment

[Junio C Hamano]

5fdeacb (Teach update-index about --ignore-submodules, 2008-05-14) added a
new refresh option flag but did not assign a unique bit for it correctly,
and broke "update-index --ignore-missing".

This should fix it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * The fact that it took this long for anybody to notice the breakage
   probably means that the "--ignore-missing" option in particular but the
   ability for plumbing to allow scripting in general is not utilized by
   as many people as the design initially envisioned.

 cache.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cache.h b/cache.h
index ca382d4..9735b66 100644
--- a/cache.h
+++ b/cache.h
@@ -396,7 +396,7 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
 #define REFRESH_UNMERGED	0x0002	/* allow unmerged */
 #define REFRESH_QUIET		0x0004	/* be quiet about it */
 #define REFRESH_IGNORE_MISSING	0x0008	/* ignore non-existent */
-#define REFRESH_IGNORE_SUBMODULES	0x0008	/* ignore submodules */
+#define REFRESH_IGNORE_SUBMODULES	0x0010	/* ignore submodules */
 extern int refresh_index(struct index_state *, unsigned int flags, const char **pathspec, char *seen);
 
 struct lock_file {
-- 
1.5.6.4.570.g052e6

Re: [PATCH] refresh-index: fix bitmask assignment

[Junio C Hamano]

This hopefully protects the previous fix (and other --refresh related
options) from future  breakages.

I'll squash it in to the previous one.
---
 t/t2103-update-index-ignore-missing.sh |   89 ++++++++++++++++++++++++++++++++
 1 files changed, 89 insertions(+), 0 deletions(-)
 create mode 100755 t/t2103-update-index-ignore-missing.sh

diff --git a/t/t2103-update-index-ignore-missing.sh b/t/t2103-update-index-ignore-missing.sh
new file mode 100755
index 0000000..332694e
--- /dev/null
+++ b/t/t2103-update-index-ignore-missing.sh
@@ -0,0 +1,89 @@
+#!/bin/sh
+
+test_description='update-index with options'
+
+. ./test-lib.sh
+
+test_expect_success basics '
+	>one &&
+	>two &&
+	>three &&
+
+	# need --add when adding
+	test_must_fail git update-index one &&
+	test -z "$(git ls-files)" &&
+	git update-index --add one &&
+	test zone = "z$(git ls-files)" &&
+
+	# update-index is atomic
+	echo 1 >one &&
+	test_must_fail git update-index one two &&
+	echo "M	one" >expect &&
+	git diff-files --name-status >actual &&
+	test_cmp expect actual &&
+
+	git update-index --add one two three &&
+	for i in one three two; do echo $i; done >expect &&
+	git ls-files >actual &&
+	test_cmp expect actual &&
+
+	test_tick &&
+	(
+		test_create_repo xyzzy &&
+		cd xyzzy &&
+		>file &&
+		git add file
+		git commit -m "sub initial"
+	) &&
+	git add xyzzy &&
+
+	test_tick &&
+	git commit -m initial &&
+	git tag initial
+'
+
+test_expect_success '--ignore-missing --refresh' '
+	git reset --hard initial &&
+	echo 2 >one &&
+	test_must_fail git update-index --refresh &&
+	echo 1 >one &&
+	git update-index --refresh &&
+	rm -f two &&
+	test_must_fail git update-index --refresh &&
+	git update-index --ignore-missing --refresh
+
+'
+
+test_expect_success '--unmerged --refresh' '
+	git reset --hard initial &&
+	info=$(git ls-files -s one | sed -e "s/ 0	/ 1	/") &&
+	git rm --cached one &&
+	echo "$info" | git update-index --index-info &&
+	test_must_fail git update-index --refresh &&
+	git update-index --unmerged --refresh &&
+	echo 2 >two &&
+	test_must_fail git update-index --unmerged --refresh >actual &&
+	grep two actual &&
+	! grep one actual &&
+	! grep three actual
+'
+
+test_expect_success '--ignore-submodules --refresh (1)' '
+	git reset --hard initial &&
+	rm -f two &&
+	test_must_fail git update-index --ignore-submodules --refresh
+'
+
+test_expect_success '--ignore-submodules --refresh (2)' '
+	git reset --hard initial &&
+	test_tick &&
+	(
+		cd xyzzy &&
+		git commit -m "sub second" --allow-empty
+	) &&
+	test_must_fail git update-index --refresh &&
+	test_must_fail git update-index --ignore-missing --refresh &&
+	git update-index --ignore-submodules --refresh
+'
+
+test_done

Re: [PATCH] refresh-index: fix bitmask assignment

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> This hopefully protects the previous fix (and other --refresh related
> options) from future  breakages.
>
> I'll squash it in to the previous one.

Actually, I'll squash this further on top, for a reason that will become
clear with the next series...

---
 t/t2103-update-index-ignore-missing.sh |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/t/t2103-update-index-ignore-missing.sh b/t/t2103-update-index-ignore-missing.sh
index 332694e..4fbf855 100755
--- a/t/t2103-update-index-ignore-missing.sh
+++ b/t/t2103-update-index-ignore-missing.sh
@@ -62,10 +62,9 @@ test_expect_success '--unmerged --refresh' '
 	test_must_fail git update-index --refresh &&
 	git update-index --unmerged --refresh &&
 	echo 2 >two &&
+	echo "two: needs update" >expect &&
 	test_must_fail git update-index --unmerged --refresh >actual &&
-	grep two actual &&
-	! grep one actual &&
-	! grep three actual
+	test_cmp expect actual
 '
 
 test_expect_success '--ignore-submodules --refresh (1)' '

[RFC variant 1 of 2] "needs update" considered harmful

[Junio C Hamano]

"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>
---
 builtin-add.c    |    3 ++-
 builtin-reset.c  |    2 +-
 cache.h          |    1 +
 read-cache.c     |    5 ++++-
 t/t7102-reset.sh |    2 +-
 5 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 9930cf5..5ffe4da 100644
--- 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");
-	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]);
diff --git a/builtin-reset.c b/builtin-reset.c
index a032169..98dbe1c 100644
--- 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");
-	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");
diff --git a/cache.h b/cache.h
index 9735b66..da87c72 100644
--- a/cache.h
+++ b/cache.h
@@ -397,6 +397,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_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 {
diff --git a/read-cache.c b/read-cache.c
index f83de8c..9839362 100644
--- 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;
+	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;
@@ -1019,7 +1022,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			}
 			if (quiet)
 				continue;
-			printf("%s: needs update\n", ce->name);
+			printf("%s: %s\n", ce->name, needs_update_message);
 			has_errors = 1;
 			continue;
 		}
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 96d1508..da4b142 100755
--- 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
-file2: needs update
+file2: locally modified
 EOF
 
 test_expect_success '--mixed refreshes the index' '

[RFC variant 2 of 2] "needs update" considered harmful

[Junio C Hamano]

"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 straightforward variant that changes the message not
only for Porcelain commands ("add" and "reset") but also changes the
output from the plumbing command "update-index".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c                           |    2 +-
 t/t2103-update-index-ignore-missing.sh |    2 +-
 t/t7102-reset.sh                       |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index f83de8c..d37aec0 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1019,7 +1019,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			}
 			if (quiet)
 				continue;
-			printf("%s: needs update\n", ce->name);
+			printf("%s: locally modified\n", ce->name);
 			has_errors = 1;
 			continue;
 		}
diff --git a/t/t2103-update-index-ignore-missing.sh b/t/t2103-update-index-ignore-missing.sh
index 4fbf855..f775acb 100755
--- a/t/t2103-update-index-ignore-missing.sh
+++ b/t/t2103-update-index-ignore-missing.sh
@@ -62,7 +62,7 @@ test_expect_success '--unmerged --refresh' '
 	test_must_fail git update-index --refresh &&
 	git update-index --unmerged --refresh &&
 	echo 2 >two &&
-	echo "two: needs update" >expect &&
+	echo "two: locally modified" >expect &&
 	test_must_fail git update-index --unmerged --refresh >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 96d1508..da4b142 100755
--- 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
-file2: needs update
+file2: locally modified
 EOF
 
 test_expect_success '--mixed refreshes the index' '
-- 
1.5.6.4.570.g052e6

[RFC] "needs update" considered harmful

[Junio C Hamano]

The previous two variants both aim to reword the somewhat unpopular "needs
update" message to easier "locally modified".

"Politically correct" thing to do would be to keep the output from the
update-index (plumbing) intact and update only output from "reset" and
"add --refresh -v" which are Porcelain.  This has smaller chance of
breaking people's existing scripts, but some people may find the two
messages that say the same thing inconsistent.

Rewording to "locally modified" even at the plumbing level is a simpler
patch, keeps the API to refresh_index() intact, and probably is a better
approach in the longer term, especially if we can ignore people's existing
scripts.

Re: [RFC variant 2 of 2] "needs update" considered harmful

[Petr Baudis]

On Sun, Jul 20, 2008 at 12:48:21AM -0700, Junio C Hamano wrote:
> "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 straightforward variant that changes the message not
> only for Porcelain commands ("add" and "reset") but also changes the
> output from the plumbing command "update-index".
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

I believe this is a good thing. Scripts need to be modified for the
reorganization anyway, and I'm not sure if there are any actually
depening on this particular string. I think having inconsistent error
messaging is worse in long term.

FWIW, looking at Cogito,

	cg-Xlib: git-update-index --refresh | sed 's/needs update$/locally modified/'

is the only reference to this. ;-)

-- 
				Petr "Pasky" Baudis
As in certain cults it is possible to kill a process if you know
its true name.  -- Ken Thompson and Dennis M. Ritchie

Re: [RFC variant 2 of 2] "needs update" considered harmful

[Johannes Schindelin]

Hi,

On Sun, 20 Jul 2008, Petr Baudis wrote:

> Scripts need to be modified for the reorganization anyway,

No.  They do not, if the 1st variant is applied.

> and I'm not sure if there are any actually depening on this particular 
> string.

That is the question, isn't it?  You would never know.  Many people, 
strange as it sounds, do not write to this list when they encounter 
problems with Git.  They vent on their blogs, not giving us a chance, and 
other people comment "Me too"s.

One of the most important features of Git used to be its scriptability, 
and I think that many hackers just love Git for it, even new hackers.

So it is probably not unheard of that someone wrote an update hook or a 
cronjob using the output of "update-index --refresh".

But those people are probably not on this list, or not following every 
thread, or they even forgot how/that they implemented such a hook/cronjob.

Ergo: we would not know if scripts break.  Until it is too late.

Ciao,
Dscho

Re: [RFC variant 2 of 2] "needs update" considered harmful

[André Goddard Rosa]

On Sun, Jul 20, 2008 at 8:29 AM, Petr Baudis <pasky@suse.cz> wrote:
> On Sun, Jul 20, 2008 at 12:48:21AM -0700, Junio C Hamano wrote:
>> "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 straightforward variant that changes the message not
>> only for Porcelain commands ("add" and "reset") but also changes the
>> output from the plumbing command "update-index".
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> I believe this is a good thing. Scripts need to be modified for the

Thanks for doing that. I hope this goes mainline soon.

-- 
[]s,
André Goddard

Re: [RFC variant 2 of 2] "needs update" considered harmful

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sun, 20 Jul 2008, Petr Baudis wrote:
>
>> Scripts need to be modified for the reorganization anyway,
>
> No.  They do not, if the 1st variant is applied.

I think Pasky's point is that people who did not bother updating their
scripts with PATH=$(git --exec-path):$PATH as described in the deprecation
notice when we went 1.5.4 now have to do so.

But if they did, they have to update again.  The more elaborate variant
avoids that, at the expense of different wordings between Porcelain and
plumbing.

I personally think there is nothing wrong if Porcelain and plumbing use
different languages, by the way.  It seems that the general concensus will
be to split the Porcelain and the plumbing manuals into separate volumes
targetted for different audiences, and it is more important to keep the
plumbing output stable as part of an established API than making the same
thing called using the same wording in different languages.

Re: [RFC variant 2 of 2] "needs update" considered harmful

[Jay Soffian]

On Sun, Jul 20, 2008 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I personally think there is nothing wrong if Porcelain and plumbing use
> different languages, by the way.

Do you expect such a clean separation? That a given command is either
plumbing or porcelain but never both? If not, then a command might
want to emit different output depending upon context. Perhaps an
environment variable, along the lines of how one sets locale on Unix,
could indicate to a git command whether it running as plumbing or not.

Just a thought.

j.

Re: [RFC] "needs update" considered harmful

[Jay Soffian]

On Sun, Jul 20, 2008 at 3:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> The previous two variants both aim to reword the somewhat unpopular "needs
> update" message to easier "locally modified".

As a git beginner I would still be a confused. I think git beginners
need to understand three concepts very early:

- committed changes
- staged changes
- unstaged changes

I think saying "unstaged changes" instead of "locally modified" would
be consistent with the git add and rm man pages and less confusing.

On a related note, I don't understand the need for two different messages here:

$ echo stuff >> foo
$ git co master
error: You have local changes to 'foo'; cannot switch branches.
$ git add foo
$ git co master
error: Entry 'foo' would be overwritten by merge. Cannot merge.

A single message "You have uncommitted changes to 'foo'; cannot switch
branches." would suffice, no?

$0.02.

j.