Re: [PATCH 16/19] reset [--mixed] --quiet: don't refresh index
Martin von Zweigbergk 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
Re: [PATCH 16/19] reset [--mixed] --quiet: don't refresh index
On Wed, Jan 9, 2013 at 11:12 AM, Junio C Hamano wrote: > Martin von Zweigbergk 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 writes: >> 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. contrib/examples/ have some, but looking at it makes me realize that we have been fairly careful to avoid using "git reset" which is a Porcelain. And as a Porcelain, I would rather expect it to leave the resulting index refreshed. -- 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 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 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
