Re: [RFC/PATCH] shortstatus v1

[Tuncer Ayaz <tuncer.ayaz@gmail.com>]

On Tue, Feb 10, 2009 at 2:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>> 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.

> 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 :).