Re: [RFC/PATCH] shortstatus v1
- From
- Tuncer Ayaz <tuncer.ayaz@gmail.com>
- Date
- Feb 10, 2009, 10:11 UTC
- Message-ID
- <4ac8254d0902100211p6a52e040je10e11c4f79ea488@mail.gmail.com>
- In-Reply-To
- <7vr627qd4p.fsf@gitster.siamese.dyndns.org>
On Tue, Feb 10, 2009 at 2:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> Tuncer Ayaz <tuncer.ayaz@gmail.com> writes: > >> Adding git 'shortstatus --mini' to PS1 is not noticeable or 1sec >> maximum in my tree. As a worst case it takes 10secs in a clone >> of WebKit.git.
Junio, if I leave out my --mini experiment would you be interested in merging shortstatus without any additions except maybe removing the index_score? Is it useful enough in your eyes? If yes I will resubmit it and decouple the --mini case completely as possible future work.
> Frankly, I think having to spend one second to add only one or two bits to > PS1 is simply spending one second too much.
ACK. it will get worse with time.
Show 47 quoted lines
>> diff --git a/builtin-commit.c b/builtin-commit.c
>> index d6a3a62..9267d26 100644
>> --- a/builtin-commit.c
>> +++ b/builtin-commit.c
>> @@ -821,6 +827,88 @@ static int parse_and_validate_options(int argc, const char *argv[],
>> return argc;
>> }
>>
>> +int cmd_shortstatus(int argc, const char **argv, const char *prefix)
>> +{
>> + struct wt_status s;
>> + int i;
>> + int c, a, u;
>> +
>> + c = a = u = 0;
>> +
>> + argc = parse_and_validate_options(argc, argv, builtin_shortstatus_usage, prefix);
>> + read_cache();
>> + refresh_cache(REFRESH_QUIET);
>> + wt_status_prepare(&s);
>> + wt_status_collect_changes(&s);
>> + if (mini) {
>> + for (i = 0; i < s.change.nr; i++) {
>> + struct wt_status_change_data *d;
>> + struct string_list_item *it;
>> +
>> + it = &(s.change.items[i]);
>> + d = it->util;
>> + switch (d->index_status) {
>> + case DIFF_STATUS_ADDED:
>> + a = 1;
>> + break;
>> + case 0:
>> + case DIFF_STATUS_COPIED:
>> + case DIFF_STATUS_DELETED:
>> + case DIFF_STATUS_MODIFIED:
>> + case DIFF_STATUS_RENAMED:
>> + case DIFF_STATUS_TYPE_CHANGED:
>> + c = 1;
>> + break;
>
> If you at the end discard information by squashing renamed, copied,
> deleted and modified into a single "changed" category, I do not think you
> would want wt_status_collect_changes() to spend the cost of rename
> detection in the first place. Sure, you can tell between "git mv old new"
> and "git add new", because you won't show "+" for "new" if you run rename
> detection, but that is about the only thing I think you are getting.actually I can leave out all but case 0 to get the current behavior. I am not sure but (presumably) have the suspicion from what I have read that these extra cases are irrelevant in this case. I may err.
> Is it worth extra 1 second (or 10 seconds)?
1 second is noticeable and therefore bad but it is a definite improvement compared to what I had before with 'git status|grep' calls. it is slow for PS1, yes.
Show 10 quoted lines
> What are you really trying to achieve? Do you want to see if you have any > change to the index since you checked out? Do you want to further tell > the user that the work tree has more changes that are not staged yet > (which --mini does not seem to do)? > > Do you really need more than "diff-index --cached --exit-code" in your > $PS1 code, and so why? Does the added feature your "shortstatus --mini" > offers over "diff-index --cached --exit-code" justify the latency penalty > to the user? >
What I and others need - based on the fact that the PS1 enhancement was inspired by someone else's PS1 - is not diff-index --cached. It should include changes in the index plus those not. The feature is there to display that a repo is dirty and if possible in an instant way also display that there are not only untrackeds but also modifications and/or additions not yet committed with separate symbols (+,*,?). If this is not currently implementable fast enough let's forget about it for now and tackle it once the future unfolds and shows us a better path or someone comes up with a bright idea :).