[PATCH 16/19] reset [--mixed] --quiet: don't refresh index
git reset [--mixed] without --quiet refreshes the index in order to
display the Unstaged changes after reset. When --quiet is given,
that output is suppressed, removing the need to refresh the index.
Other porcelain commands that care about a refreshed index should
already be refreshing it, so running e.g. git reset -q git diff
is still safe.
This commit together with 686b2de (oneway_merge(): only lstat() when
told to update worktree, 2012-12-20) removes all calls to lstat() the
worktree from the command.
This speeds up git reset -q a little on the linux-2.6 repo (best
of five, warm cache):
Before After
real0m0.215s0m0.176s
user0m0.150s0m0.130s
sys 0m0.060s0m0.040s
And with cold cache (best of five):
Before After
real0m11.351s 0m8.420s
user0m0.230s0m0.220s
sys 0m0.270s0m0.060s
---
There is a test case in t7102 called '--mixed refreshes the index',
but it only checks that right output it printed. Is the test case not
testing right or not named right? As you can see, I suspect it's the
name/description that should change.
builtin/reset.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 9bcad29..a2e69eb 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -109,12 +109,6 @@ static void print_new_head_line(struct commit *commit)
printf(\n);
}
-static void update_index_refresh(int flags)
-{
- refresh_index(the_index, (flags), NULL, NULL,
- _(Unstaged changes after reset:));
-}
-
static void update_index_from_diff(struct diff_queue_struct *q,
struct diff_options *opt, void *data)
{
@@ -328,9 +322,9 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
die(_(Could not reset index file to revision
'%s'.), rev);
}
- if (reset_type == MIXED) /* Report what has not been updated. */
- update_index_refresh(
- quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ if (reset_type == MIXED !quiet) /* Report what has not been
updated. */
+ refresh_index(the_index, REFRESH_IN_PORCELAIN, NULL,
NULL,
+ _(Unstaged changes after reset:));
if (write_cache(newfd, active_cache, active_nr) ||
commit_locked_index(lock))
--
1.8.1.rc3.331.g1ef2165
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/19] reset [--mixed] --quiet: don't refresh index
On Wed, Jan 09, 2013 at 12:16:13AM -0800, Martin von Zweigbergk wrote: git reset [--mixed] without --quiet refreshes the index in order to display the Unstaged changes after reset. When --quiet is given, that output is suppressed, removing the need to refresh the index. Other porcelain commands that care about a refreshed index should already be refreshing it, so running e.g. git reset -q git diff is still safe. Hmm. But git reset -q git diff-files would not be? We have never been very clear about which commands refresh the index. Since reset is about manipulating the index, I'd expect it to be refreshed afterwards. On the other hand, since we have never guaranteed anything, perhaps a careful script should always use git update-index --refresh. I would not be too surprised if some of our own scripts are not that careful, though. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/19] reset [--mixed] --quiet: don't refresh index
On Wed, Jan 9, 2013 at 9:01 AM, Jeff King p...@peff.net wrote: On Wed, Jan 09, 2013 at 12:16:13AM -0800, Martin von Zweigbergk wrote: git reset [--mixed] without --quiet refreshes the index in order to display the Unstaged changes after reset. When --quiet is given, that output is suppressed, removing the need to refresh the index. Other porcelain commands that care about a refreshed index should already be refreshing it, so running e.g. git reset -q git diff is still safe. Hmm. But git reset -q git diff-files would not be? Right. Actually, git reset -q git diff was perhaps not a good example, because its analogous plumbing command would be git reset -q git diff-files -p, which is also safe. But, as you say, git reset -q git diff-files (without -p) might list files for which only the stat information has changed. We have never been very clear about which commands refresh the index. Yes, git-reset's documentation doesn't mention it. Since reset is about manipulating the index, I'd expect it to be refreshed afterwards. On the other hand, since we have never guaranteed anything, perhaps a careful script should always use git update-index --refresh. Since git diff-files is a plumbing command, users of it to a hopefully a bit more careful than regular users, but you never know. I would not be too surprised if some of our own scripts are not that careful, though. I didn't find any, but I might have missed something. Regardless, this patch was tangential. The goal of this series can be achieved independently of this patch, so if it's too risky, we can drop easily drop it. Also, even though it does make git reset -q faster, I'm not sure how important that is in practice. Most use cases would probably refresh the index afterwards anyway. In such cases, the improvement on warm cache would still be there, but the relative improvement in the cold cache case would be pretty much gone (since the entire tree would be stat'ed by the following refresh anyway). Martin -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/19] reset [--mixed] --quiet: don't refresh index
On Wed, Jan 9, 2013 at 11:12 AM, Junio C Hamano gits...@pobox.com wrote: Martin von Zweigbergk martinv...@gmail.com writes: And as a Porcelain, I would rather expect it to leave the resulting index refreshed. Yeah, I guess you're right. Regular users (those using only porcelain) shouldn't notice, but it does make sense to think that the index would be refreshed after running a porcelain. And the risk of breaking people's scripts seems real too. I'll drop patch this from the re-roll (which I'll also make sure I'll sign off) (FYI, the reason I wrote this patch was because I was surprised that git reset did anything with the worktree at all.) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/19] reset [--mixed] --quiet: don't refresh index
Martin von Zweigbergk martinv...@gmail.com writes:
There is a test case in t7102 called '--mixed refreshes the index',
but it only checks that right output it printed.
I think that comes from 620a6cd (builtin-reset: avoid forking
update-index --refresh, 2007-11-03). Before that commit, we
refreshed the index with --mixed, and the test tries to make sure we
continue to do so after the change. Even though it is not testing
if the index has stat only changes (which is rather cumbersome to
write---you need to futz with timestamp or something) and using the
output from refresh machinery as a substitute, I think the intent of
that commit is fairly clear.
builtin/reset.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 9bcad29..a2e69eb 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -109,12 +109,6 @@ static void print_new_head_line(struct commit *commit)
printf(\n);
}
-static void update_index_refresh(int flags)
-{
- refresh_index(the_index, (flags), NULL, NULL,
- _(Unstaged changes after reset:));
-}
-
static void update_index_from_diff(struct diff_queue_struct *q,
struct diff_options *opt, void *data)
{
@@ -328,9 +322,9 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
die(_(Could not reset index file to revision
'%s'.), rev);
}
- if (reset_type == MIXED) /* Report what has not been updated. */
- update_index_refresh(
- quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
+ if (reset_type == MIXED !quiet) /* Report what has not been
updated. */
+ refresh_index(the_index, REFRESH_IN_PORCELAIN, NULL,
NULL,
+ _(Unstaged changes after reset:));
if (write_cache(newfd, active_cache, active_nr) ||
commit_locked_index(lock))
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
