* [RFC/PATCH] shortstatus v1
@ 2009-02-10 0:51 Tuncer Ayaz
2009-02-10 1:44 ` Junio C Hamano
2009-02-10 11:03 ` Jeff King
0 siblings, 2 replies; 26+ messages in thread
From: Tuncer Ayaz @ 2009-02-10 0:51 UTC (permalink / raw)
To: git; +Cc: gitster
As discussed recently I started taking Junio's shortstatus patch
from October 25th 2008 and integrated it into current master.
This revision does work as advertised by Junio and v0 also did.
This revision also implements an experimental --mini param to
shortstatus which prints the following:
anything modified -> * (working)
anything added -> + (working)
anything untracked/unknown -> ? (not yet implemented)
So if you have a repo where one file is modified,
a new file is added and an unknown file exists and
is not ignored 'shortstatus --mini' prints:
+*?.
This is really useful for enhancing a Git enabled
shell prompt with small but important information.
Right now this is basically Junio's shortstatus
from Oct 25th 2008 with no substantial change
except a line or two.
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.
TODO:
- print ? if untracked/unknown found
- maybe implement git-ministatus instead of git-shortstatus --mini
- as Junio mentioned maybe we should not print the index_score
- peer review (mini clause/mode, especially the switch-case)
- peer review rest of the patch
Signed-off-by: Tuncer Ayaz <tuncer.ayaz@gmail.com>
---
.gitignore | 1
Makefile | 1
builtin-commit.c | 92 +++++++++++++++++++++++
builtin-revert.c | 1
builtin.h | 1
git.c | 1
wt-status.c | 213 +++++++++++++++++++++++++++++++++++++++++++------------
wt-status.h | 9 ++
8 files changed, 273 insertions(+), 46 deletions(-)
diff --git a/.gitignore b/.gitignore
index 13311f1..055eb54 100644
--- a/.gitignore
+++ b/.gitignore
@@ -115,6 +115,7 @@ git-send-pack
git-sh-setup
git-shell
git-shortlog
+git-shortstatus
git-show
git-show-branch
git-show-index
diff --git a/Makefile b/Makefile
index 27b9569..a0ca137 100644
--- a/Makefile
+++ b/Makefile
@@ -330,6 +330,7 @@ BUILT_INS += git-repo-config$X
BUILT_INS += git-show$X
BUILT_INS += git-stage$X
BUILT_INS += git-status$X
+BUILT_INS += git-shortstatus$X
BUILT_INS += git-whatchanged$X
# what 'all' will build and 'install' will install, in gitexecdir
diff --git a/builtin-commit.c b/builtin-commit.c
index d6a3a62..9267d26 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -14,6 +14,7 @@
#include "diffcore.h"
#include "commit.h"
#include "revision.h"
+#include "string-list.h"
#include "wt-status.h"
#include "run-command.h"
#include "refs.h"
@@ -21,7 +22,6 @@
#include "strbuf.h"
#include "utf8.h"
#include "parse-options.h"
-#include "string-list.h"
#include "rerere.h"
#include "unpack-trees.h"
@@ -35,6 +35,11 @@ static const char * const builtin_status_usage[] = {
NULL
};
+static const char * const builtin_shortstatus_usage[] = {
+ "git shortstatus [options] [--] <filepattern>...",
+ NULL
+};
+
static unsigned char head_sha1[20], merge_head_sha1[20];
static char *use_message_buffer;
static const char commit_editmsg[] = "COMMIT_EDITMSG";
@@ -51,7 +56,7 @@ static const char *template_file;
static char *edit_message, *use_message;
static char *author_name, *author_email, *author_date;
static int all, edit_flag, also, interactive, only, amend, signoff;
-static int quiet, verbose, no_verify, allow_empty;
+static int quiet, verbose, no_verify, allow_empty, mini;
static char *untracked_files_arg;
/*
* The default commit message cleanup mode will remove the lines
@@ -107,6 +112,7 @@ static struct option builtin_commit_options[] = {
{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, "mode", "show untracked files, optional modes: all, normal, no. (Default: all)", PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
OPT_BOOLEAN(0, "allow-empty", &allow_empty, "ok to record an empty change"),
OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
+ OPT_BOOLEAN(0, "mini", &mini, "print mini shortstatus"),
OPT_END()
};
@@ -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;
+ default:
+ case DIFF_STATUS_UNKNOWN:
+ case DIFF_STATUS_UNMERGED:
+ u = 1;
+ break;
+ }
+ }
+ if (c)
+ printf("*");
+ if (a)
+ printf("+");
+ if (u)
+ printf("?");
+ } else {
+ for (i = 0; i < s.change.nr; i++) {
+ struct wt_status_change_data *d;
+ struct string_list_item *it;
+ char pfx[1 + 3 + 1 + 1];
+
+ it = &(s.change.items[i]);
+ d = it->util;
+ switch (d->index_status) {
+ case DIFF_STATUS_COPIED:
+ case DIFF_STATUS_RENAMED:
+ sprintf(pfx, "%c%3d",
+ d->index_status,
+ (int)(d->index_score * 100 / MAX_SCORE));
+ break;
+ case 0:
+ memcpy(pfx, " ", 4);
+ break;
+ default:
+ sprintf(pfx, "%c ", d->index_status);
+ break;
+ }
+ if (!d->worktree_status)
+ pfx[4] = ' ';
+ else
+ pfx[4] = d->worktree_status;
+ pfx[5] = '\0';
+ printf("%s ", pfx);
+ if (d->head_path)
+ printf("%s -> ", d->head_path);
+ printf("%s\n", it->string);
+ }
+
+ }
+ return 0;
+}
+
int cmd_status(int argc, const char **argv, const char *prefix)
{
const char *index_file;
diff --git a/builtin-revert.c b/builtin-revert.c
index d48313c..7dd7646 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -3,6 +3,7 @@
#include "object.h"
#include "commit.h"
#include "tag.h"
+#include "string-list.h"
#include "wt-status.h"
#include "run-command.h"
#include "exec_cmd.h"
diff --git a/builtin.h b/builtin.h
index 1495cf6..f054fc7 100644
--- a/builtin.h
+++ b/builtin.h
@@ -94,6 +94,7 @@ extern int cmd_shortlog(int argc, const char **argv, const char *prefix);
extern int cmd_show(int argc, const char **argv, const char *prefix);
extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_shortstatus(int argc, const char **argv, const char *prefix);
extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
extern int cmd_tag(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index c2b181e..4c0fa44 100644
--- a/git.c
+++ b/git.c
@@ -344,6 +344,7 @@ static void handle_internal_command(int argc, const char **argv)
{ "rm", cmd_rm, RUN_SETUP },
{ "send-pack", cmd_send_pack, RUN_SETUP },
{ "shortlog", cmd_shortlog, USE_PAGER },
+ { "shortstatus", cmd_shortstatus, RUN_SETUP | NEED_WORK_TREE },
{ "show-branch", cmd_show_branch, RUN_SETUP },
{ "show", cmd_show, RUN_SETUP | USE_PAGER },
{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
diff --git a/wt-status.c b/wt-status.c
index 96ff2f8..18042dc 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1,4 +1,5 @@
#include "cache.h"
+#include "string-list.h"
#include "wt-status.h"
#include "color.h"
#include "object.h"
@@ -56,6 +57,7 @@ void wt_status_prepare(struct wt_status *s)
s->reference = "HEAD";
s->fp = stdout;
s->index_file = get_index_file();
+ s->change.strdup_strings = 1;
}
static void wt_status_print_cached_header(struct wt_status *s)
@@ -98,18 +100,23 @@ static void wt_status_print_trailer(struct wt_status *s)
#define quote_path quote_path_relative
-static void wt_status_print_filepair(struct wt_status *s,
- int t, struct diff_filepair *p)
+static void wt_status_print_change_data(struct wt_status *s,
+ int t,
+ int status,
+ char *one_name,
+ char *two_name,
+ int score)
{
const char *c = color(t);
const char *one, *two;
struct strbuf onebuf = STRBUF_INIT, twobuf = STRBUF_INIT;
- one = quote_path(p->one->path, -1, &onebuf, s->prefix);
- two = quote_path(p->two->path, -1, &twobuf, s->prefix);
+ one = quote_path(one_name, -1, &onebuf, s->prefix);
+ two = quote_path(two_name, -1, &twobuf, s->prefix);
+
color_fprintf(s->fp, color(WT_STATUS_HEADER), "#\t");
- switch (p->status) {
+ switch (status) {
case DIFF_STATUS_ADDED:
color_fprintf(s->fp, c, "new file: %s", one);
break;
@@ -135,64 +142,88 @@ static void wt_status_print_filepair(struct wt_status *s,
color_fprintf(s->fp, c, "unmerged: %s", one);
break;
default:
- die("bug: unhandled diff status %c", p->status);
+ die("bug: unhandled diff status %c", status);
}
fprintf(s->fp, "\n");
strbuf_release(&onebuf);
strbuf_release(&twobuf);
}
-static void wt_status_print_updated_cb(struct diff_queue_struct *q,
- struct diff_options *options,
- void *data)
+static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
+ struct diff_options *options,
+ void *data)
{
struct wt_status *s = data;
- int shown_header = 0;
int i;
+
+ if (!q->nr)
+ return;
+ s->workdir_dirty = 1;
for (i = 0; i < q->nr; i++) {
- if (q->queue[i]->status == 'U')
- continue;
- if (!shown_header) {
- wt_status_print_cached_header(s);
- s->commitable = 1;
- shown_header = 1;
- }
- wt_status_print_filepair(s, WT_STATUS_UPDATED, q->queue[i]);
+ struct diff_filepair *p;
+ struct string_list_item *it;
+ struct wt_status_change_data *d;
+
+ p = q->queue[i];
+
+ d = xcalloc(1, sizeof(*d));
+ d->worktree_status = p->status;
+ it = string_list_insert(p->one->path, &s->change);
+ it->util = d;
}
- if (shown_header)
- wt_status_print_trailer(s);
}
-static void wt_status_print_changed_cb(struct diff_queue_struct *q,
- struct diff_options *options,
- void *data)
+static void wt_status_collect_updated_cb(struct diff_queue_struct *q,
+ struct diff_options *options,
+ void *data)
{
struct wt_status *s = data;
int i;
- if (q->nr) {
- int has_deleted = 0;
- s->workdir_dirty = 1;
- for (i = 0; i < q->nr; i++)
- if (q->queue[i]->status == DIFF_STATUS_DELETED) {
- has_deleted = 1;
+
+ for (i = 0; i < q->nr; i++) {
+ struct diff_filepair *p;
+ struct string_list_item *it;
+ struct wt_status_change_data *d;
+
+ p = q->queue[i];
+ it = string_list_insert(p->two->path, &s->change);
+ d = it->util;
+ if (!d) {
+ d = xcalloc(1, sizeof(*d));
+ it->util = d;
+ }
+ d->index_status = p->status;
+ switch (p->status) {
+ case DIFF_STATUS_COPIED:
+ case DIFF_STATUS_RENAMED:
+ d->head_path = xstrdup(p->one->path);
+ d->index_score = p->score;
break;
- }
- wt_status_print_dirty_header(s, has_deleted);
+ }
}
- for (i = 0; i < q->nr; i++)
- wt_status_print_filepair(s, WT_STATUS_CHANGED, q->queue[i]);
- if (q->nr)
- wt_status_print_trailer(s);
}
-static void wt_status_print_updated(struct wt_status *s)
+static void wt_status_collect_changes_worktree(struct wt_status *s)
{
struct rev_info rev;
+
+ init_revisions(&rev, NULL);
+ setup_revisions(0, NULL, &rev, NULL);
+ rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+ rev.diffopt.format_callback = wt_status_collect_changed_cb;
+ rev.diffopt.format_callback_data = s;
+ run_diff_files(&rev, 0);
+}
+
+static void wt_status_collect_changes_index(struct wt_status *s)
+{
+ struct rev_info rev;
+
init_revisions(&rev, NULL);
setup_revisions(0, NULL, &rev,
s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference);
rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
- rev.diffopt.format_callback = wt_status_print_updated_cb;
+ rev.diffopt.format_callback = wt_status_collect_updated_cb;
rev.diffopt.format_callback_data = s;
rev.diffopt.detect_rename = 1;
rev.diffopt.rename_limit = 200;
@@ -200,15 +231,107 @@ static void wt_status_print_updated(struct wt_status *s)
run_diff_index(&rev, 1);
}
+static void wt_status_collect_changes_initial(struct wt_status *s)
+{
+ int i;
+
+ for (i = 0; i < active_nr; i++) {
+ struct string_list_item *it;
+ struct wt_status_change_data *d;
+
+ it = string_list_insert(active_cache[i]->name, &s->change);
+ d = it->util;
+ if (!d) {
+ d = xcalloc(1, sizeof(*d));
+ it->util = d;
+ }
+ d->index_status = DIFF_STATUS_ADDED;
+ }
+}
+
+void wt_status_collect_changes(struct wt_status *s)
+{
+ wt_status_collect_changes_worktree(s);
+
+ if (s->is_initial)
+ wt_status_collect_changes_initial(s);
+ else
+ wt_status_collect_changes_index(s);
+}
+
+static void wt_status_print_updated(struct wt_status *s)
+{
+ int shown_header = 0;
+ int i;
+
+ 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;
+ if (!d->index_status)
+ continue;
+ if (!shown_header) {
+ wt_status_print_cached_header(s);
+ s->commitable = 1;
+ shown_header = 1;
+ }
+ wt_status_print_change_data(s, WT_STATUS_UPDATED,
+ d->index_status,
+ d->head_path ? d->head_path : it->string,
+ it->string,
+ d->index_score);
+ }
+ if (shown_header)
+ wt_status_print_trailer(s);
+}
+
+/*
+ * -1 : has delete
+ * 0 : no change
+ * 1 : some change but no delete
+ */
+static int wt_status_check_worktree_changes(struct wt_status *s)
+{
+ int i;
+ int changes = 0;
+
+ for (i = 0; i < s->change.nr; i++) {
+ struct wt_status_change_data *d;
+ d = s->change.items[i].util;
+ if (!d->worktree_status)
+ continue;
+ changes = 1;
+ if (d->worktree_status == DIFF_STATUS_DELETED)
+ return -1;
+ }
+ return changes;
+}
+
static void wt_status_print_changed(struct wt_status *s)
{
- struct rev_info rev;
- init_revisions(&rev, "");
- setup_revisions(0, NULL, &rev, NULL);
- rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
- rev.diffopt.format_callback = wt_status_print_changed_cb;
- rev.diffopt.format_callback_data = s;
- run_diff_files(&rev, 0);
+ int i;
+ int worktree_changes = wt_status_check_worktree_changes(s);
+
+ if (!worktree_changes)
+ return;
+
+ wt_status_print_dirty_header(s, worktree_changes < 0);
+
+ 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;
+ if (!d->worktree_status)
+ continue;
+ wt_status_print_change_data(s, WT_STATUS_CHANGED,
+ d->worktree_status,
+ it->string,
+ it->string,
+ 0);
+ }
+ wt_status_print_trailer(s);
}
static void wt_status_print_submodule_summary(struct wt_status *s)
@@ -338,6 +461,8 @@ void wt_status_print(struct wt_status *s)
wt_status_print_tracking(s);
}
+ wt_status_collect_changes(s);
+
if (s->is_initial) {
color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "#");
color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "# Initial commit");
diff --git a/wt-status.h b/wt-status.h
index 78add09..00508c3 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -18,6 +18,13 @@ enum untracked_status_type {
};
extern enum untracked_status_type show_untracked_files;
+struct wt_status_change_data {
+ int worktree_status;
+ int index_status;
+ int index_score;
+ char *head_path;
+};
+
struct wt_status {
int is_initial;
char *branch;
@@ -33,6 +40,7 @@ struct wt_status {
const char *index_file;
FILE *fp;
const char *prefix;
+ struct string_list change;
};
int git_status_config(const char *var, const char *value, void *cb);
@@ -40,5 +48,6 @@ extern int wt_status_use_color;
extern int wt_status_relative_paths;
void wt_status_prepare(struct wt_status *s);
void wt_status_print(struct wt_status *s);
+void wt_status_collect_changes(struct wt_status *s);
#endif /* STATUS_H */
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 0:51 [RFC/PATCH] shortstatus v1 Tuncer Ayaz
@ 2009-02-10 1:44 ` Junio C Hamano
2009-02-10 3:46 ` Sitaram Chamarty
2009-02-10 10:11 ` Tuncer Ayaz
2009-02-10 11:03 ` Jeff King
1 sibling, 2 replies; 26+ messages in thread
From: Junio C Hamano @ 2009-02-10 1:44 UTC (permalink / raw)
To: Tuncer Ayaz; +Cc: git
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.
Frankly, I think having to spend one second to add only one or two bits to
PS1 is simply spending one second too much.
> 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.
Is it worth extra 1 second (or 10 seconds)?
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?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 1:44 ` Junio C Hamano
@ 2009-02-10 3:46 ` Sitaram Chamarty
2009-02-10 10:22 ` Spending time in PS1, was " Johannes Schindelin
2009-02-10 10:11 ` Tuncer Ayaz
1 sibling, 1 reply; 26+ messages in thread
From: Sitaram Chamarty @ 2009-02-10 3:46 UTC (permalink / raw)
To: git
On 2009-02-10, 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.
>
> Frankly, I think having to spend one second to add only one or two bits to
> PS1 is simply spending one second too much.
[snip]
> 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?
I wonder if I could ask people opinions on a trick I pulled,
which is basically maintain a state of the value of $SECONDS
each time the user is shown a bash prompt. If the value is
the same as last time (meaning he hit enter twice in a row
very quickly), it runs the extra stuff.
It sounds like a dirty trick, but seems to work fine and
give you the best of both worlds.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 1:44 ` Junio C Hamano
2009-02-10 3:46 ` Sitaram Chamarty
@ 2009-02-10 10:11 ` Tuncer Ayaz
1 sibling, 0 replies; 26+ messages in thread
From: Tuncer Ayaz @ 2009-02-10 10:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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 :).
^ permalink raw reply [flat|nested] 26+ messages in thread
* Spending time in PS1, was Re: [RFC/PATCH] shortstatus v1
2009-02-10 3:46 ` Sitaram Chamarty
@ 2009-02-10 10:22 ` Johannes Schindelin
2009-02-10 17:31 ` Sitaram Chamarty
0 siblings, 1 reply; 26+ messages in thread
From: Johannes Schindelin @ 2009-02-10 10:22 UTC (permalink / raw)
To: Sitaram Chamarty; +Cc: git
Hi,
On Tue, 10 Feb 2009, Sitaram Chamarty wrote:
> I wonder if I could ask people opinions on a trick I pulled, which is
> basically maintain a state of the value of $SECONDS each time the user
> is shown a bash prompt. If the value is the same as last time (meaning
> he hit enter twice in a row very quickly), it runs the extra stuff.
As you know, I am a big fan of consistency. In this light, I do not like
it when I am shown something at times, and at other times not.
Besides, I agree with Junio that even a single second spent to calculate
PS1 is too much. I use my command line extensively (some claim I live in
it). An slow PS1 would drive me to madness.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 0:51 [RFC/PATCH] shortstatus v1 Tuncer Ayaz
2009-02-10 1:44 ` Junio C Hamano
@ 2009-02-10 11:03 ` Jeff King
2009-02-10 11:29 ` Michael J Gruber
2009-02-10 15:58 ` Junio C Hamano
1 sibling, 2 replies; 26+ messages in thread
From: Jeff King @ 2009-02-10 11:03 UTC (permalink / raw)
To: Tuncer Ayaz; +Cc: git, gitster
On Tue, Feb 10, 2009 at 01:51:07AM +0100, Tuncer Ayaz wrote:
> As discussed recently I started taking Junio's shortstatus patch
> from October 25th 2008 and integrated it into current master.
>
> This revision does work as advertised by Junio and v0 also did.
I did a simple test with this:
mkdir repo && cd repo && git init &&
touch unchanged changed changed-staged deleted deleted-staged &&
git add . && git commit -m one &&
echo changes >changed &&
echo changes >changed-staged && git add changed-staged &&
rm deleted &&
git rm deleted-staged &&
git shortstatus
The output is:
changed
M changed-staged
deleted
D deleted-staged
Some comments:
1. Is the staggered indentation intentional? It looks awful, and the
only use I can think of is to separate unstaged from staged
changes. But surely there must be a more obvious way of doing so.
2. Why do staged changes get a letter marking what happened, but
unstaged changes do not?
3. What advantage does this have over just doing:
(git diff --name-status;
git diff --cached --name-status) | sort -k2
> Right now this is basically Junio's shortstatus
> from Oct 25th 2008 with no substantial change
> except a line or two.
This is not a very helpful commit message. What is it supposed to do?
What does the output look like? Why is it implemented this way? If Junio
sent a patch in October and it isn't substantially changed, why wasn't
it accepted then?
> +static const char * const builtin_shortstatus_usage[] = {
> + "git shortstatus [options] [--] <filepattern>...",
> + NULL
> +};
Really? Doing "git shortstatus subdir" seems not to affect the output,
nor does "git shortstatus I totally made up these command line
arguments".
What options are available? It looks like this is intimately tied with
"commit", which I think is one of the _shortcomings_ of the current
status. It means the command line options are non-intuitive for what
people generally want to say: "what is changed, possibly limiting to
some path".
> + OPT_BOOLEAN(0, "mini", &mini, "print mini shortstatus"),
So now "git status --mini" doesn't complain, but it doesn't seem to
actually do anything.
> + argc = parse_and_validate_options(argc, argv, builtin_shortstatus_usage, prefix);
Ah, I see the source of the option issues. You parse with the commit
options, but then you don't actually respect any of them. You would want
a totally separate set of options for shortstatus. In fact, I really
don't see what point there is in putting it with the 'commit' code at
all.
> + 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;
> + default:
> + case DIFF_STATUS_UNKNOWN:
> + case DIFF_STATUS_UNMERGED:
> + u = 1;
> + break;
> + }
> + }
> + if (c)
> + printf("*");
> + if (a)
> + printf("+");
> + if (u)
> + printf("?");
Isn't this a bit heavy-handed? If you really just want to know "are
there any changes", can't you run a custom diff with EXIT_CODE and QUIET
set, which will bail when it sees the first change, saving you a lot of
useless computation?
> + } else {
> + for (i = 0; i < s.change.nr; i++) {
> + struct wt_status_change_data *d;
> + struct string_list_item *it;
> + char pfx[1 + 3 + 1 + 1];
Holy magic numbers, Batman.
-Peff
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 11:03 ` Jeff King
@ 2009-02-10 11:29 ` Michael J Gruber
2009-02-10 11:31 ` Tuncer Ayaz
2009-02-10 11:45 ` Jeff King
2009-02-10 15:58 ` Junio C Hamano
1 sibling, 2 replies; 26+ messages in thread
From: Michael J Gruber @ 2009-02-10 11:29 UTC (permalink / raw)
To: Jeff King; +Cc: Tuncer Ayaz, git, gitster
Jeff King venit, vidit, dixit 10.02.2009 12:03:
> On Tue, Feb 10, 2009 at 01:51:07AM +0100, Tuncer Ayaz wrote:
...
> 3. What advantage does this have over just doing:
>
> (git diff --name-status;
> git diff --cached --name-status) | sort -k2
That is fine, except that it can't list untracked files.
> What options are available? It looks like this is intimately tied with
> "commit", which I think is one of the _shortcomings_ of the current
> status. It means the command line options are non-intuitive for what
> people generally want to say: "what is changed, possibly limiting to
> some path".
Right now, "git status" is basically "git commit --dry-run", which may
or may not be good, but certainly is not what people coming from other
vcs expect. I would suggest having "git commit -n" replace "git status"
if I hadn't done so already or if I dared to (I can't remember ;) ).
The softer approach was naming "shortstatus" what those people would
expect for "status".
The "git diff" based solution does almost everything, but back then it
wasn't clear how to get at the untracked and ignored files. In fact,
that would have the benefit that output from "git diff --name-status
commitA commitB" is guaranteed to stay consistent with "git diff
--name-status HEAD WORKTREE", "git diff --name-status INDEX WORKTREE"
and the three-way diff between HEAD, INDEX and WORKTREE which
shortstatus really is (WORKTREE meaning full wt with untrcaked/ignored
files).
"git ls-files" may do but has a different set of mode characters. I
think that sums up what preceeded Junio's patch from October.
Michael
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 11:29 ` Michael J Gruber
@ 2009-02-10 11:31 ` Tuncer Ayaz
2009-02-10 11:45 ` Jeff King
1 sibling, 0 replies; 26+ messages in thread
From: Tuncer Ayaz @ 2009-02-10 11:31 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Jeff King, git, gitster
On Tue, Feb 10, 2009 at 12:29 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Jeff King venit, vidit, dixit 10.02.2009 12:03:
>> On Tue, Feb 10, 2009 at 01:51:07AM +0100, Tuncer Ayaz wrote:
> ...
>> 3. What advantage does this have over just doing:
>>
>> (git diff --name-status;
>> git diff --cached --name-status) | sort -k2
>
> That is fine, except that it can't list untracked files.
>
>> What options are available? It looks like this is intimately tied with
>> "commit", which I think is one of the _shortcomings_ of the current
>> status. It means the command line options are non-intuitive for what
>> people generally want to say: "what is changed, possibly limiting to
>> some path".
>
> Right now, "git status" is basically "git commit --dry-run", which may
> or may not be good, but certainly is not what people coming from other
> vcs expect. I would suggest having "git commit -n" replace "git status"
> if I hadn't done so already or if I dared to (I can't remember ;) ).
>
> The softer approach was naming "shortstatus" what those people would
> expect for "status".
>
> The "git diff" based solution does almost everything, but back then it
> wasn't clear how to get at the untracked and ignored files. In fact,
> that would have the benefit that output from "git diff --name-status
> commitA commitB" is guaranteed to stay consistent with "git diff
> --name-status HEAD WORKTREE", "git diff --name-status INDEX WORKTREE"
> and the three-way diff between HEAD, INDEX and WORKTREE which
> shortstatus really is (WORKTREE meaning full wt with untrcaked/ignored
> files).
>
> "git ls-files" may do but has a different set of mode characters. I
> think that sums up what preceeded Junio's patch from October.
For reference:
http://markmail.org/message/tqvshvcj2ybgj6ea
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 11:29 ` Michael J Gruber
2009-02-10 11:31 ` Tuncer Ayaz
@ 2009-02-10 11:45 ` Jeff King
2009-02-10 12:36 ` Michael J Gruber
1 sibling, 1 reply; 26+ messages in thread
From: Jeff King @ 2009-02-10 11:45 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Tuncer Ayaz, git, gitster
On Tue, Feb 10, 2009 at 12:29:40PM +0100, Michael J Gruber wrote:
> > 3. What advantage does this have over just doing:
> >
> > (git diff --name-status;
> > git diff --cached --name-status) | sort -k2
>
> That is fine, except that it can't list untracked files.
Well, neither does this patch:
$ echo content >tracked &&
> echo content >untracked &&
> git add tracked &&
> git shortstatus
A tracked
but you could easily include that:
(git diff --name-status;
git diff --cached --name-status;
git ls-files --exclude-standard -o | sed 's/^/? /') | sort -k2
which is really more or less what the wt-status code does.
Note that I am not _against_ a convenient command for doing this. But I
have to wonder why such a large patch is necessary when I can do it in
three lines. I don't mind the C version being a little longer, but I
wonder what advantage there is in using wt_status for this.
> Right now, "git status" is basically "git commit --dry-run", which may
> or may not be good, but certainly is not what people coming from other
> vcs expect. I would suggest having "git commit -n" replace "git status"
> if I hadn't done so already or if I dared to (I can't remember ;) ).
I would much prefer that, if it had been done that way from the
beginning. But I think we are stuck with "git status" due to hysterical
raisins.
> "git ls-files" may do but has a different set of mode characters. I
> think that sums up what preceeded Junio's patch from October.
But you only need to use it here to get the untracked files, so it
doesn't matter what it says about modified files.
The big downside with the snippet I posted above is that it runs three
separate commands that go through the index. In theory, you could do it
in one pass. But wt-status _doesn't_ do that, since the diff
infrastructure isn't there (a long time ago, Junio had an experimental
parallel diff walker patch, but it never made it out of next).
-Peff
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 11:45 ` Jeff King
@ 2009-02-10 12:36 ` Michael J Gruber
2009-02-10 13:01 ` Jeff King
0 siblings, 1 reply; 26+ messages in thread
From: Michael J Gruber @ 2009-02-10 12:36 UTC (permalink / raw)
To: Jeff King; +Cc: Tuncer Ayaz, git, gitster
Jeff King venit, vidit, dixit 10.02.2009 12:45:
...
>> Right now, "git status" is basically "git commit --dry-run", which may
>> or may not be good, but certainly is not what people coming from other
>> vcs expect. I would suggest having "git commit -n" replace "git status"
>> if I hadn't done so already or if I dared to (I can't remember ;) ).
>
> I would much prefer that, if it had been done that way from the
> beginning. But I think we are stuck with "git status" due to hysterical
> raisins.
ROTFTCOOTF!
Now I know why I never liked those caricatures of grapes...
>> "git ls-files" may do but has a different set of mode characters. I
>> think that sums up what preceeded Junio's patch from October.
>
> But you only need to use it here to get the untracked files, so it
> doesn't matter what it says about modified files.
>
> The big downside with the snippet I posted above is that it runs three
> separate commands that go through the index. In theory, you could do it
> in one pass. But wt-status _doesn't_ do that, since the diff
> infrastructure isn't there (a long time ago, Junio had an experimental
> parallel diff walker patch, but it never made it out of next).
We completely agree. How do you suggest to progress? Go for the diff
walker? For a (porc.) command like shortstatus I think going through the
index 3 times isn't that bad, all disk access should be cached after the
first run.
Michael
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 12:36 ` Michael J Gruber
@ 2009-02-10 13:01 ` Jeff King
0 siblings, 0 replies; 26+ messages in thread
From: Jeff King @ 2009-02-10 13:01 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Tuncer Ayaz, git, gitster
On Tue, Feb 10, 2009 at 01:36:56PM +0100, Michael J Gruber wrote:
> > I would much prefer that, if it had been done that way from the
> > beginning. But I think we are stuck with "git status" due to hysterical
> > raisins.
>
> ROTFTCOOTF!
I have to admit, this is a new acronym for me...I get the first half,
but not the second.
> > The big downside with the snippet I posted above is that it runs three
> > separate commands that go through the index. In theory, you could do it
> > in one pass. But wt-status _doesn't_ do that, since the diff
> > infrastructure isn't there (a long time ago, Junio had an experimental
> > parallel diff walker patch, but it never made it out of next).
>
> We completely agree. How do you suggest to progress? Go for the diff
> walker? For a (porc.) command like shortstatus I think going through the
> index 3 times isn't that bad, all disk access should be cached after the
> first run.
I don't know if resurrecting the parallel diff walker is worth the
trouble. I guess if somebody cares enough about the performance they can
find the old patch and try benchmarking it.
Making a C command rather than a shell script is probably reasonable if
this is performance critical (and there seems to be talk of putting it
into a prompt). What I really object to in the patch is:
- sticking this in builtin-commit.c. It really has _nothing_ to do
with commit or the existing status. Especially using the same
option parser is just nonsensical.
- I'm not sure bolting this onto wt-status really makes much sense.
Especially the performance-critical --mini prompt mode _doesn't_
want to do the string collection because it wastes a lot of cycles
figuring out things that we are just going to throw away.
However, for the "regular" mode, I don't think it is too big a
problem. wt_status does a few extra things that shortstatus won't
care about, but I don't think they are too performance critical
(e.g., I believe it will find out the "your branch is N commits
ahead of the remote" information).
-Peff
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 11:03 ` Jeff King
2009-02-10 11:29 ` Michael J Gruber
@ 2009-02-10 15:58 ` Junio C Hamano
2009-02-10 18:10 ` Jeff King
2009-02-10 23:52 ` Nanako Shiraishi
1 sibling, 2 replies; 26+ messages in thread
From: Junio C Hamano @ 2009-02-10 15:58 UTC (permalink / raw)
To: Jeff King; +Cc: Tuncer Ayaz, git
Jeff King <peff@peff.net> writes:
> Some comments:
>
> 1. Is the staggered indentation intentional? It looks awful, and the
> only use I can think of is to separate unstaged from staged
> changes. But surely there must be a more obvious way of doing so.
Probably not.
> 2. Why do staged changes get a letter marking what happened, but
> unstaged changes do not?
Bug? FWIW, the original patch from October shows:
M changed
M M changed-again
M changed-staged
D deleted
D deleted-staged
(where changed-again has both staged changes and further changes in the
work tree).
The gap between these two are to show the rename similarity index, which
we could do without.
> 3. What advantage does this have over just doing:
>
> (git diff --name-status;
> git diff --cached --name-status) | sort -k2
>
>> Right now this is basically Junio's shortstatus
>> from Oct 25th 2008 with no substantial change
>> except a line or two.
>
> This is not a very helpful commit message. What is it supposed to do?
> What does the output look like? Why is it implemented this way? If Junio
> sent a patch in October and it isn't substantially changed, why wasn't
> it accepted then?
The output mimicked what was in Shawn's "repo" tool announcement IIRC.
My patch was supposed to give interested parties hint to base a patch like
Tuncer's on (I think this answers your last question, too).
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Spending time in PS1, was Re: [RFC/PATCH] shortstatus v1
2009-02-10 10:22 ` Spending time in PS1, was " Johannes Schindelin
@ 2009-02-10 17:31 ` Sitaram Chamarty
0 siblings, 0 replies; 26+ messages in thread
From: Sitaram Chamarty @ 2009-02-10 17:31 UTC (permalink / raw)
To: git
On 2009-02-10, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 10 Feb 2009, Sitaram Chamarty wrote:
>
>> I wonder if I could ask people opinions on a trick I pulled, which is
>> basically maintain a state of the value of $SECONDS each time the user
>> is shown a bash prompt. If the value is the same as last time (meaning
>> he hit enter twice in a row very quickly), it runs the extra stuff.
>
> As you know, I am a big fan of consistency. In this light, I do not like
Actually no; I haven't been here long enough yet.
> it when I am shown something at times, and at other times not.
However, it seems to me that this discussion is about
reconciling two conflicting needs:
- some people (not all) want certain info in the prompt
- but getting that info is expensive so it shouldn't be in
the prompt *all* the time
Such a conflict might well be served by showing something
sometimes, and at other times not. A little bit of
DWIMmery, if you will...
In any case, one can always alias something to a single
letter (say "s") if one needs quick but not "in your face"
access to this info, making this whole discussion moot if it
should not be *in* the prompt.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 15:58 ` Junio C Hamano
@ 2009-02-10 18:10 ` Jeff King
2009-02-10 18:22 ` Jeff King
2009-02-10 19:11 ` Jeff King
2009-02-10 23:52 ` Nanako Shiraishi
1 sibling, 2 replies; 26+ messages in thread
From: Jeff King @ 2009-02-10 18:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tuncer Ayaz, git
On Tue, Feb 10, 2009 at 07:58:47AM -0800, Junio C Hamano wrote:
> > 2. Why do staged changes get a letter marking what happened, but
> > unstaged changes do not?
>
> Bug? FWIW, the original patch from October shows:
>
> M changed
> M M changed-again
> M changed-staged
> D deleted
> D deleted-staged
>
> (where changed-again has both staged changes and further changes in the
> work tree).
>
> The gap between these two are to show the rename similarity index, which
> we could do without.
OK, that makes more sense. And your example shows a good answer to my
earlier question: why is just sorting the output of the the three
commands (diff, diff --cached, and ls-files -o) not as nice. The answer
is that we need to actually combine lines when files are appear in
multiple places.
> The output mimicked what was in Shawn's "repo" tool announcement IIRC.
>
> My patch was supposed to give interested parties hint to base a patch like
> Tuncer's on (I think this answers your last question, too).
I went back and read some of the background. I think having this work
with the wt-status machinery is reasonable, then. My concerns with
Tuncer's patch are still:
- this should not be part of builtin-commit.c; it doesn't use any of
the same code except that which has already been lib-ified in
wt-status.[ch].
- I don't think the "mini" status is really related to this. The novel
thing here is collating the outputs into a single sorted list. But
the "mini" output is not about that at all:
1. It doesn't care about full output, so it should be able to exit
early from the diff, avoid rename detection, etc, so that it is
as quick as possible.
2. It doesn't collate the output at all. It is about three
separate symbols for the three separate lists.
-Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 18:10 ` Jeff King
@ 2009-02-10 18:22 ` Jeff King
2009-02-10 19:11 ` Jeff King
1 sibling, 0 replies; 26+ messages in thread
From: Jeff King @ 2009-02-10 18:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tuncer Ayaz, git
On Tue, Feb 10, 2009 at 01:10:52PM -0500, Jeff King wrote:
> - I don't think the "mini" status is really related to this. The novel
> thing here is collating the outputs into a single sorted list. But
> the "mini" output is not about that at all:
>
> 1. It doesn't care about full output, so it should be able to exit
> early from the diff, avoid rename detection, etc, so that it is
> as quick as possible.
>
> 2. It doesn't collate the output at all. It is about three
> separate symbols for the three separate lists.
Oh, sorry, I was misreading the "mini" output. I thought the three flags
corresponded to the staged, unstaged, and untracked changes. But they
are "unstaged or staged but added", "unstaged or staged but changed", or
"untracked" (although right now the last is triggered by unmerged
entries?).
I honestly don't see much point in differentiating added versus changed
files. Splitting it into "some things are staged" and "some things are
not staged" makes more sense to me. But if you do want that distinction
then an early exit from the diff is more complicated (since you might
have to keep going to see if there are any of _either_ type).
-Peff
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 18:10 ` Jeff King
2009-02-10 18:22 ` Jeff King
@ 2009-02-10 19:11 ` Jeff King
2009-02-10 21:21 ` Tuncer Ayaz
2009-02-10 22:25 ` Junio C Hamano
1 sibling, 2 replies; 26+ messages in thread
From: Jeff King @ 2009-02-10 19:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Tuncer Ayaz
On Tue, Feb 10, 2009 at 01:10:52PM -0500, Jeff King wrote:
> - I don't think the "mini" status is really related to this. The novel
> thing here is collating the outputs into a single sorted list. But
> the "mini" output is not about that at all:
>
> 1. It doesn't care about full output, so it should be able to exit
> early from the diff, avoid rename detection, etc, so that it is
> as quick as possible.
>
> 2. It doesn't collate the output at all. It is about three
> separate symbols for the three separate lists.
OK, I realize this is not exactly what the proposed --mini does. But
here is more along the lines of what I was thinking.
Warm cache, it runs in .042s on my git repo, about half of which is the
untracked files check. It takes about .49s on the kernel repo. The
read_directory() bit is not optimized at all, and could probably benefit
from an early return (OTOH, the worst case is still going to need to
look at every path).
I am not particularly interested in a fancy prompt myself, but maybe
this will help somebody else.
The patch relies on the index_differs_from() patch that Stephan
posted earlier today.
---
.gitignore | 1 +
Makefile | 1 +
builtin-ministatus.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
builtin.h | 1 +
git.c | 1 +
5 files changed, 56 insertions(+), 0 deletions(-)
diff --git a/.gitignore b/.gitignore
index 055eb54..de2249b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -81,6 +81,7 @@ git-mergetool
git-mktag
git-mktree
git-name-rev
+git-ministatus
git-mv
git-notes
git-pack-redundant
diff --git a/Makefile b/Makefile
index a0ca137..9145c7b 100644
--- a/Makefile
+++ b/Makefile
@@ -559,6 +559,7 @@ BUILTIN_OBJS += builtin-merge-base.o
BUILTIN_OBJS += builtin-merge-file.o
BUILTIN_OBJS += builtin-merge-ours.o
BUILTIN_OBJS += builtin-merge-recursive.o
+BUILTIN_OBJS += builtin-ministatus.o
BUILTIN_OBJS += builtin-mv.o
BUILTIN_OBJS += builtin-name-rev.o
BUILTIN_OBJS += builtin-pack-objects.o
diff --git a/builtin-ministatus.c b/builtin-ministatus.c
new file mode 100644
index 0000000..c9f8e7f
--- /dev/null
+++ b/builtin-ministatus.c
@@ -0,0 +1,52 @@
+#include "cache.h"
+#include "diff.h"
+#include "commit.h"
+#include "revision.h"
+#include "dir.h"
+
+static int worktree_is_dirty(void)
+{
+ struct rev_info rev;
+ init_revisions(&rev, "");
+ setup_revisions(0, NULL, &rev, NULL);
+ DIFF_OPT_SET(&rev.diffopt, QUIET);
+ DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
+ run_diff_files(&rev, 0);
+ return DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
+}
+
+static int have_untracked(void)
+{
+ struct dir_struct dir;
+ int i;
+
+ memset(&dir, 0, sizeof dir);
+ setup_standard_excludes(&dir);
+
+ read_directory(&dir, ".", "", 0, NULL);
+ /* XXX we are probably leaking memory from dir */
+ for (i = 0; i < dir.nr; i++)
+ struct dir_entry *ent = dir.entries[i];
+ if (cache_name_is_other(ent->name, ent->len))
+ return 1;
+ }
+ return 0;
+}
+
+int cmd_ministatus(int argc, const char **argv, const char *prefix)
+{
+ if (argc != 1)
+ die("Sorry, I don't understand any command line options.");
+
+ read_cache();
+ refresh_cache(REFRESH_QUIET);
+
+ if (index_differs_from("HEAD", 0))
+ putchar('+');
+ if (worktree_is_dirty())
+ putchar('*');
+ if (have_untracked())
+ putchar('?');
+
+ return 0;
+}
diff --git a/builtin.h b/builtin.h
index f054fc7..03e6a88 100644
--- a/builtin.h
+++ b/builtin.h
@@ -71,6 +71,7 @@ extern int cmd_merge_base(int argc, const char **argv, const char *prefix);
extern int cmd_merge_ours(int argc, const char **argv, const char *prefix);
extern int cmd_merge_file(int argc, const char **argv, const char *prefix);
extern int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
+extern int cmd_ministatus(int argc, const char **argv, const char *prefix);
extern int cmd_mv(int argc, const char **argv, const char *prefix);
extern int cmd_name_rev(int argc, const char **argv, const char *prefix);
extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index 4c0fa44..8bf7e78 100644
--- a/git.c
+++ b/git.c
@@ -323,6 +323,7 @@ static void handle_internal_command(int argc, const char **argv)
{ "merge-ours", cmd_merge_ours, RUN_SETUP },
{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
+ { "ministatus", cmd_ministatus, RUN_SETUP | NEED_WORK_TREE },
{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
{ "name-rev", cmd_name_rev, RUN_SETUP },
{ "pack-objects", cmd_pack_objects, RUN_SETUP },
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 19:11 ` Jeff King
@ 2009-02-10 21:21 ` Tuncer Ayaz
2009-02-10 21:36 ` Jeff King
2009-02-10 22:25 ` Junio C Hamano
1 sibling, 1 reply; 26+ messages in thread
From: Tuncer Ayaz @ 2009-02-10 21:21 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
On Tue, Feb 10, 2009 at 8:11 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 10, 2009 at 01:10:52PM -0500, Jeff King wrote:
>
>> - I don't think the "mini" status is really related to this. The novel
>> thing here is collating the outputs into a single sorted list. But
>> the "mini" output is not about that at all:
>>
>> 1. It doesn't care about full output, so it should be able to exit
>> early from the diff, avoid rename detection, etc, so that it is
>> as quick as possible.
>>
>> 2. It doesn't collate the output at all. It is about three
>> separate symbols for the three separate lists.
>
> OK, I realize this is not exactly what the proposed --mini does. But
> here is more along the lines of what I was thinking.
>
> Warm cache, it runs in .042s on my git repo, about half of which is the
> untracked files check. It takes about .49s on the kernel repo. The
> read_directory() bit is not optimized at all, and could probably benefit
> from an early return (OTOH, the worst case is still going to need to
> look at every path).
>
> I am not particularly interested in a fancy prompt myself, but maybe
> this will help somebody else.
I tried this and it did not run faster than my experiment.
I had to add a missing opening curly brace in
have_untracked() before it compiled.
As we haven't found a fast way yet I can live without it.
> The patch relies on the index_differs_from() patch that Stephan
> posted earlier today.
>
> ---
> .gitignore | 1 +
> Makefile | 1 +
> builtin-ministatus.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
> builtin.h | 1 +
> git.c | 1 +
> 5 files changed, 56 insertions(+), 0 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index 055eb54..de2249b 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -81,6 +81,7 @@ git-mergetool
> git-mktag
> git-mktree
> git-name-rev
> +git-ministatus
> git-mv
> git-notes
> git-pack-redundant
> diff --git a/Makefile b/Makefile
> index a0ca137..9145c7b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -559,6 +559,7 @@ BUILTIN_OBJS += builtin-merge-base.o
> BUILTIN_OBJS += builtin-merge-file.o
> BUILTIN_OBJS += builtin-merge-ours.o
> BUILTIN_OBJS += builtin-merge-recursive.o
> +BUILTIN_OBJS += builtin-ministatus.o
> BUILTIN_OBJS += builtin-mv.o
> BUILTIN_OBJS += builtin-name-rev.o
> BUILTIN_OBJS += builtin-pack-objects.o
> diff --git a/builtin-ministatus.c b/builtin-ministatus.c
> new file mode 100644
> index 0000000..c9f8e7f
> --- /dev/null
> +++ b/builtin-ministatus.c
> @@ -0,0 +1,52 @@
> +#include "cache.h"
> +#include "diff.h"
> +#include "commit.h"
> +#include "revision.h"
> +#include "dir.h"
> +
> +static int worktree_is_dirty(void)
> +{
> + struct rev_info rev;
> + init_revisions(&rev, "");
> + setup_revisions(0, NULL, &rev, NULL);
> + DIFF_OPT_SET(&rev.diffopt, QUIET);
> + DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
> + run_diff_files(&rev, 0);
> + return DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
> +}
> +
> +static int have_untracked(void)
> +{
> + struct dir_struct dir;
> + int i;
> +
> + memset(&dir, 0, sizeof dir);
> + setup_standard_excludes(&dir);
> +
> + read_directory(&dir, ".", "", 0, NULL);
> + /* XXX we are probably leaking memory from dir */
> + for (i = 0; i < dir.nr; i++)
> + struct dir_entry *ent = dir.entries[i];
> + if (cache_name_is_other(ent->name, ent->len))
> + return 1;
> + }
> + return 0;
> +}
> +
> +int cmd_ministatus(int argc, const char **argv, const char *prefix)
> +{
> + if (argc != 1)
> + die("Sorry, I don't understand any command line options.");
> +
> + read_cache();
> + refresh_cache(REFRESH_QUIET);
> +
> + if (index_differs_from("HEAD", 0))
> + putchar('+');
> + if (worktree_is_dirty())
> + putchar('*');
> + if (have_untracked())
> + putchar('?');
> +
> + return 0;
> +}
> diff --git a/builtin.h b/builtin.h
> index f054fc7..03e6a88 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -71,6 +71,7 @@ extern int cmd_merge_base(int argc, const char **argv, const char *prefix);
> extern int cmd_merge_ours(int argc, const char **argv, const char *prefix);
> extern int cmd_merge_file(int argc, const char **argv, const char *prefix);
> extern int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
> +extern int cmd_ministatus(int argc, const char **argv, const char *prefix);
> extern int cmd_mv(int argc, const char **argv, const char *prefix);
> extern int cmd_name_rev(int argc, const char **argv, const char *prefix);
> extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);
> diff --git a/git.c b/git.c
> index 4c0fa44..8bf7e78 100644
> --- a/git.c
> +++ b/git.c
> @@ -323,6 +323,7 @@ static void handle_internal_command(int argc, const char **argv)
> { "merge-ours", cmd_merge_ours, RUN_SETUP },
> { "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
> { "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
> + { "ministatus", cmd_ministatus, RUN_SETUP | NEED_WORK_TREE },
> { "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
> { "name-rev", cmd_name_rev, RUN_SETUP },
> { "pack-objects", cmd_pack_objects, RUN_SETUP },
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 21:21 ` Tuncer Ayaz
@ 2009-02-10 21:36 ` Jeff King
0 siblings, 0 replies; 26+ messages in thread
From: Jeff King @ 2009-02-10 21:36 UTC (permalink / raw)
To: Tuncer Ayaz; +Cc: git, Junio C Hamano
On Tue, Feb 10, 2009 at 10:21:16PM +0100, Tuncer Ayaz wrote:
> I tried this and it did not run faster than my experiment.
I'm surprised, based on the numbers you gave before. I guess your
machine is just a lot slower than what I am experimenting with. I think
you will have to profile to see what is taking so long, then, if you
want to speed it up.
> I had to add a missing opening curly brace in
> have_untracked() before it compiled.
Sorry about that. I added the comment just above directly into the
patch, and obviously managed to butcher the brace while I was doing it.
-Peff
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 19:11 ` Jeff King
2009-02-10 21:21 ` Tuncer Ayaz
@ 2009-02-10 22:25 ` Junio C Hamano
2009-02-10 22:52 ` Tuncer Ayaz
2009-02-10 22:55 ` Jeff King
1 sibling, 2 replies; 26+ messages in thread
From: Junio C Hamano @ 2009-02-10 22:25 UTC (permalink / raw)
To: Jeff King; +Cc: git, Tuncer Ayaz
Jeff King <peff@peff.net> writes:
> Warm cache, it runs in .042s on my git repo, about half of which is the
> untracked files check. It takes about .49s on the kernel repo. The
> read_directory() bit is not optimized at all, and could probably benefit
> from an early return (OTOH, the worst case is still going to need to
> look at every path).
I suspect that with a large tree your have_untracked() would show
unnecessary overhead from dir_add_name(), because you only want one bit of
information but there is no way to stop with "ok, we know enough". This
toy patch adds a trivial "early return" to read_directory() codepath, but
there are two sad things about it.
* In order to cheaply run "is there a single other file", you really
should scan the level you have already opened first before digging
deeper. I didn't bother because the primary use of read_directory is
the depth first traversal.
* In a cloned work tree, the tracked files and directories come early in
the physical directory and then crufts you created yourself comes at
the end in readdir() order. We tend to read a lot of tracked ones
first and the finally hit other files.
builtin-ministatus.c | 4 +++-
dir.c | 32 +++++++++++++++++++++++++++-----
dir.h | 2 ++
3 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/builtin-ministatus.c b/builtin-ministatus.c
index aff9e5a..4b5a191 100644
--- a/builtin-ministatus.c
+++ b/builtin-ministatus.c
@@ -25,7 +25,7 @@ static int have_untracked(void)
read_directory(&dir, ".", "", 0, NULL);
/* XXX we are probably leaking memory from dir */
- for (i = 0; i < dir.nr; i++)
+ for (i = 0; i < dir.nr; i++) {
struct dir_entry *ent = dir.entries[i];
if (cache_name_is_other(ent->name, ent->len))
return 1;
@@ -47,6 +47,8 @@ int cmd_ministatus(int argc, const char **argv, const char *prefix)
putchar('*');
if (have_untracked())
putchar('?');
+ if (untracked_files_exist())
+ putchar('%');
return 0;
}
diff --git a/dir.c b/dir.c
index cfd1ea5..8d4fcdd 100644
--- a/dir.c
+++ b/dir.c
@@ -16,7 +16,10 @@ struct path_simplify {
static int read_directory_recursive(struct dir_struct *dir,
const char *path, const char *base, int baselen,
- int check_only, const struct path_simplify *simplify);
+ int mode, const struct path_simplify *simplify);
+#define READ_DIRECTORY_EMPTY_CHECK 1
+#define READ_DIRECTORY_OTHER_CHECK 2
+
static int get_dtype(struct dirent *de, const char *path);
int common_prefix(const char **pathspec)
@@ -505,7 +508,8 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
/* This is the "show_other_directories" case */
if (!dir->hide_empty_directories)
return show_directory;
- if (!read_directory_recursive(dir, dirname, dirname, len, 1, simplify))
+ if (!read_directory_recursive(dir, dirname, dirname, len,
+ READ_DIRECTORY_EMPTY_CHECK, simplify))
return ignore_directory;
return show_directory;
}
@@ -574,10 +578,12 @@ static int get_dtype(struct dirent *de, const char *path)
* Also, we ignore the name ".git" (even if it is not a directory).
* That likely will not change.
*/
-static int read_directory_recursive(struct dir_struct *dir, const char *path, const char *base, int baselen, int check_only, const struct path_simplify *simplify)
+static int read_directory_recursive(struct dir_struct *dir, const char *path, const char *base, int baselen, int mode, const struct path_simplify *simplify)
{
DIR *fdir = opendir(path);
int contents = 0;
+ int empty_check_only = (mode == READ_DIRECTORY_EMPTY_CHECK);
+ int other_check_only = (mode == READ_DIRECTORY_OTHER_CHECK);
if (fdir) {
struct dirent *de;
@@ -639,7 +645,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
break;
case recurse_into_directory:
contents += read_directory_recursive(dir,
- fullname, fullname, baselen + len, 0, simplify);
+ fullname, fullname, baselen + len, mode, simplify);
continue;
case ignore_directory:
continue;
@@ -650,10 +656,12 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
break;
}
contents++;
- if (check_only)
+ if (empty_check_only)
goto exit_early;
else
dir_add_name(dir, fullname, baselen + len);
+ if (other_check_only && dir->nr)
+ goto exit_early;
}
exit_early:
closedir(fdir);
@@ -731,6 +739,20 @@ int read_directory(struct dir_struct *dir, const char *path, const char *base, i
return dir->nr;
}
+int untracked_files_exist(void)
+{
+ struct dir_struct dir;
+ int i;
+
+ memset(&dir, 0, sizeof(dir));
+ setup_standard_excludes(&dir);
+ read_directory_recursive(&dir, ".", "", 0, READ_DIRECTORY_OTHER_CHECK,
+ NULL);
+ for (i = 0; i < dir.nr; i++)
+ free(dir.entries[i]);
+ return !!dir.nr;
+}
+
int file_exists(const char *f)
{
struct stat sb;
diff --git a/dir.h b/dir.h
index bdc2d47..1f8b575 100644
--- a/dir.h
+++ b/dir.h
@@ -92,4 +92,6 @@ extern int remove_dir_recursively(struct strbuf *path, int only_empty);
/* tries to remove the path with empty directories along it, ignores ENOENT */
extern int remove_path(const char *path);
+extern int untracked_files_exist(void);
+
#endif
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 22:25 ` Junio C Hamano
@ 2009-02-10 22:52 ` Tuncer Ayaz
2009-02-10 22:55 ` Jeff King
1 sibling, 0 replies; 26+ messages in thread
From: Tuncer Ayaz @ 2009-02-10 22:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
On Tue, Feb 10, 2009 at 11:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> Warm cache, it runs in .042s on my git repo, about half of which is the
>> untracked files check. It takes about .49s on the kernel repo. The
>> read_directory() bit is not optimized at all, and could probably benefit
>> from an early return (OTOH, the worst case is still going to need to
>> look at every path).
>
> I suspect that with a large tree your have_untracked() would show
> unnecessary overhead from dir_add_name(), because you only want one bit of
> information but there is no way to stop with "ok, we know enough". This
> toy patch adds a trivial "early return" to read_directory() codepath, but
> there are two sad things about it.
>
> * In order to cheaply run "is there a single other file", you really
> should scan the level you have already opened first before digging
> deeper. I didn't bother because the primary use of read_directory is
> the depth first traversal.
>
> * In a cloned work tree, the tracked files and directories come early in
> the physical directory and then crufts you created yourself comes at
> the end in readdir() order. We tend to read a lot of tracked ones
> first and the finally hit other files.
I've done some measurements from within .bashrc via a
custom timer() bash function called before and after git ministatus call.
The box I'm testing on is a Core2Duo with 1.8GHz and 2GB of RAM.
I have faster machines I can test on but do Git coding on this one.
For WebKit.git this is similar to the previous patch. It does also take
at least 8 or 9 seconds. That's an improvement over 8-11secs.
Is it good to test against WebKit.git? I mean it's apparently big.
> builtin-ministatus.c | 4 +++-
> dir.c | 32 +++++++++++++++++++++++++++-----
> dir.h | 2 ++
> 3 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/builtin-ministatus.c b/builtin-ministatus.c
> index aff9e5a..4b5a191 100644
> --- a/builtin-ministatus.c
> +++ b/builtin-ministatus.c
> @@ -25,7 +25,7 @@ static int have_untracked(void)
>
> read_directory(&dir, ".", "", 0, NULL);
> /* XXX we are probably leaking memory from dir */
> - for (i = 0; i < dir.nr; i++)
> + for (i = 0; i < dir.nr; i++) {
> struct dir_entry *ent = dir.entries[i];
> if (cache_name_is_other(ent->name, ent->len))
> return 1;
> @@ -47,6 +47,8 @@ int cmd_ministatus(int argc, const char **argv, const char *prefix)
> putchar('*');
> if (have_untracked())
> putchar('?');
> + if (untracked_files_exist())
> + putchar('%');
>
> return 0;
> }
> diff --git a/dir.c b/dir.c
> index cfd1ea5..8d4fcdd 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -16,7 +16,10 @@ struct path_simplify {
>
> static int read_directory_recursive(struct dir_struct *dir,
> const char *path, const char *base, int baselen,
> - int check_only, const struct path_simplify *simplify);
> + int mode, const struct path_simplify *simplify);
> +#define READ_DIRECTORY_EMPTY_CHECK 1
> +#define READ_DIRECTORY_OTHER_CHECK 2
> +
> static int get_dtype(struct dirent *de, const char *path);
>
> int common_prefix(const char **pathspec)
> @@ -505,7 +508,8 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
> /* This is the "show_other_directories" case */
> if (!dir->hide_empty_directories)
> return show_directory;
> - if (!read_directory_recursive(dir, dirname, dirname, len, 1, simplify))
> + if (!read_directory_recursive(dir, dirname, dirname, len,
> + READ_DIRECTORY_EMPTY_CHECK, simplify))
> return ignore_directory;
> return show_directory;
> }
> @@ -574,10 +578,12 @@ static int get_dtype(struct dirent *de, const char *path)
> * Also, we ignore the name ".git" (even if it is not a directory).
> * That likely will not change.
> */
> -static int read_directory_recursive(struct dir_struct *dir, const char *path, const char *base, int baselen, int check_only, const struct path_simplify *simplify)
> +static int read_directory_recursive(struct dir_struct *dir, const char *path, const char *base, int baselen, int mode, const struct path_simplify *simplify)
> {
> DIR *fdir = opendir(path);
> int contents = 0;
> + int empty_check_only = (mode == READ_DIRECTORY_EMPTY_CHECK);
> + int other_check_only = (mode == READ_DIRECTORY_OTHER_CHECK);
>
> if (fdir) {
> struct dirent *de;
> @@ -639,7 +645,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
> break;
> case recurse_into_directory:
> contents += read_directory_recursive(dir,
> - fullname, fullname, baselen + len, 0, simplify);
> + fullname, fullname, baselen + len, mode, simplify);
> continue;
> case ignore_directory:
> continue;
> @@ -650,10 +656,12 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
> break;
> }
> contents++;
> - if (check_only)
> + if (empty_check_only)
> goto exit_early;
> else
> dir_add_name(dir, fullname, baselen + len);
> + if (other_check_only && dir->nr)
> + goto exit_early;
> }
> exit_early:
> closedir(fdir);
> @@ -731,6 +739,20 @@ int read_directory(struct dir_struct *dir, const char *path, const char *base, i
> return dir->nr;
> }
>
> +int untracked_files_exist(void)
> +{
> + struct dir_struct dir;
> + int i;
> +
> + memset(&dir, 0, sizeof(dir));
> + setup_standard_excludes(&dir);
> + read_directory_recursive(&dir, ".", "", 0, READ_DIRECTORY_OTHER_CHECK,
> + NULL);
> + for (i = 0; i < dir.nr; i++)
> + free(dir.entries[i]);
> + return !!dir.nr;
> +}
> +
> int file_exists(const char *f)
> {
> struct stat sb;
> diff --git a/dir.h b/dir.h
> index bdc2d47..1f8b575 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -92,4 +92,6 @@ extern int remove_dir_recursively(struct strbuf *path, int only_empty);
> /* tries to remove the path with empty directories along it, ignores ENOENT */
> extern int remove_path(const char *path);
>
> +extern int untracked_files_exist(void);
> +
> #endif
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 22:25 ` Junio C Hamano
2009-02-10 22:52 ` Tuncer Ayaz
@ 2009-02-10 22:55 ` Jeff King
2009-02-10 23:05 ` Junio C Hamano
1 sibling, 1 reply; 26+ messages in thread
From: Jeff King @ 2009-02-10 22:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Tuncer Ayaz
On Tue, Feb 10, 2009 at 02:25:39PM -0800, Junio C Hamano wrote:
> > Warm cache, it runs in .042s on my git repo, about half of which is the
> > untracked files check. It takes about .49s on the kernel repo. The
> > read_directory() bit is not optimized at all, and could probably benefit
> > from an early return (OTOH, the worst case is still going to need to
> > look at every path).
>
> I suspect that with a large tree your have_untracked() would show
> unnecessary overhead from dir_add_name(), because you only want one bit of
> information but there is no way to stop with "ok, we know enough". This
> toy patch adds a trivial "early return" to read_directory() codepath, but
> there are two sad things about it.
My timings are exactly the same (with my have_untracked removed,
of course). Due most likely to the fact that I keep my repos clean, so
we have to look at every directory to realize there are no files.
Here are a few timings:
For the relative times of each action, I get (in the linux-2.6 tree):
- nothing (just read_cache / refresh_cache)
real 0m0.178s
user 0m0.060s
sys 0m0.116s
- just index_differs_from (minus nothing = 0.107s)
real 0m0.285s
user 0m0.140s
sys 0m0.140s
- just worktree_is_dirty (minus nothing = 0.0s)
real 0m0.178s
user 0m0.048s
sys 0m0.132s
- just untracked_files_exist (minus nothing = 0.184s)
real 0m0.362s
user 0m0.188s
sys 0m0.176s
So untracked_files is definitely the worst, but I was surprised that
the index to HEAD diff takes so long.
For just index_differs_from, gprof claims we spend most of our time in
unpack-trees stuff:
33.33 0.01 0.01 28301 0.00 0.00 unpack_callback
33.33 0.02 0.01 28301 0.00 0.00 unpack_nondirectories
33.33 0.03 0.01 26627 0.00 0.00 convert_from_disk
For just untracked_files_exist, gprof claims we spend most of our time
dealing with the index name hash (and obviously a bit of time in
excluded_1):
27.27 0.03 0.03 1479931 0.00 0.00 icase_hash
18.18 0.05 0.02 74142 0.00 0.00 excluded_1
9.09 0.06 0.01 120178 0.00 0.00 lookup_hash_entry
9.09 0.07 0.01 49855 0.00 0.00 hash_name
9.09 0.08 0.01 26627 0.00 0.00 convert_from_disk
9.09 0.09 0.01 24739 0.00 0.00 excluded
9.09 0.10 0.01 18 0.56 0.88 grow_hash_table
So maybe there are speedups to be had there. ISTR the icase_hash stuff
came about to support case-challenged filesystems. I haven't looked to
see if it could be turned into a ALL_OF_MY_FILESYSTEMS_ARE_SANE
compile-time option to get some speedup.
-Peff
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 22:55 ` Jeff King
@ 2009-02-10 23:05 ` Junio C Hamano
2009-02-12 0:49 ` Jeff King
0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2009-02-10 23:05 UTC (permalink / raw)
To: Jeff King; +Cc: git, Tuncer Ayaz
Jeff King <peff@peff.net> writes:
> So untracked_files is definitely the worst, but I was surprised that
> the index to HEAD diff takes so long.
There is an obvious optimization you can do to "diff-index --cached" using
cache-tree. If your index is really clean, computing the tree object the
index would represent (without writing the tree object out) and comparing
it against HEAD^{tree} may be a tad faster.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 15:58 ` Junio C Hamano
2009-02-10 18:10 ` Jeff King
@ 2009-02-10 23:52 ` Nanako Shiraishi
2009-02-11 21:24 ` Junio C Hamano
1 sibling, 1 reply; 26+ messages in thread
From: Nanako Shiraishi @ 2009-02-10 23:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Tuncer Ayaz, git
Quoting Junio C Hamano <gitster@pobox.com>:
> Jeff King <peff@peff.net> writes:
>
>> Some comments:
>>
>> 1. Is the staggered indentation intentional? It looks awful, and the
>> only use I can think of is to separate unstaged from staged
>> changes. But surely there must be a more obvious way of doing so.
>
> Probably not.
>
>> 2. Why do staged changes get a letter marking what happened, but
>> unstaged changes do not?
>
> Bug? FWIW, the original patch from October shows:
>
> M changed
> M M changed-again
> M changed-staged
> D deleted
> D deleted-staged
>
> (where changed-again has both staged changes and further changes in the
> work tree).
>
> The gap between these two are to show the rename similarity index, which
> we could do without.
I have a question. Why do you have the gap for the rename similarity between the two but not between the second status and the filename?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 23:58 [RFC] New command: 'git snapshot' Ulrik Sverdrup
@ 2009-02-11 0:08 ` Nanako Shiraishi
0 siblings, 0 replies; 26+ messages in thread
From: Nanako Shiraishi @ 2009-02-11 0:08 UTC (permalink / raw)
To: Ulrik Sverdrup; +Cc: git, Geoffrey Lee
Quoting Ulrik Sverdrup <ulrik.sverdrup@gmail.com>:
>> On Tue, Feb 10, 2009 at 3:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > Jeff King <peff@peff.net> writes:
>> > How is it different from "git stash create"?
>>
>> Git stash doesn't touch untracked files, whereas git snapshot would.
>> Take another closer look at the table in the original post titled
>> "What are the differences between 'git stash' and 'git snapshot'?"
>>
>> -Geoffrey Lee
>
> I'm understanding this just as I read this, but it seems that implementing a
> git snapshot (I'm myself interested), could be done quickly with a new git.
> (When was git stash create introduced? I don't know it?)
I did the initial git-stash at the end of June 2007; git-stash create was done by Junio about a week later and was released as a part of v1.5.4.
> Something like this:
> cp .git/index .git/tmp-index
> GIT_INDEX_FILE=.git/tmp-index
> git add -N .
> git stash create
>
> So we use add -N to put all files into tracked but unstaged by default, but we
> keep our old index. Now stash is ready to save off the working directory, and
> further logic has to be applied on the returned commit to save it off..
git-add -N came much later, late August 2008, and is available only in v1.6.1 and later.
I think you want to export GIT_INDEX_FILE you set to the temporary file before running these two commands.
--
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 23:52 ` Nanako Shiraishi
@ 2009-02-11 21:24 ` Junio C Hamano
0 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2009-02-11 21:24 UTC (permalink / raw)
To: Nanako Shiraishi; +Cc: Jeff King, Tuncer Ayaz, git
Nanako Shiraishi <nanako3@lavabit.com> writes:
> Quoting Junio C Hamano <gitster@pobox.com>:
>
>> Bug? FWIW, the original patch from October shows:
>>
>> M changed
>> M M changed-again
>> M changed-staged
>> D deleted
>> D deleted-staged
>>
>> (where changed-again has both staged changes and further changes in the
>> work tree).
>>
>> The gap between these two are to show the rename similarity index, which
>> we could do without.
>
> I have a question. Why do you have the gap for the rename similarity
> between the two but not between the second status and the filename?
There can be renames between the HEAD and the index, but by definition
there can never be renames between the index and the work tree, because
we do not use untracked files in the work tree for comparison, which means
there is no "new" files when comparing the index and the work tree.
For this reason, there is need for similarity indices for the second one.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [RFC/PATCH] shortstatus v1
2009-02-10 23:05 ` Junio C Hamano
@ 2009-02-12 0:49 ` Jeff King
0 siblings, 0 replies; 26+ messages in thread
From: Jeff King @ 2009-02-12 0:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Tuncer Ayaz
On Tue, Feb 10, 2009 at 03:05:57PM -0800, Junio C Hamano wrote:
> There is an obvious optimization you can do to "diff-index --cached" using
> cache-tree. If your index is really clean, computing the tree object the
> index would represent (without writing the tree object out) and comparing
> it against HEAD^{tree} may be a tad faster.
Clever, but I think you may just be trading one scenario for "worst
case" versus another (i.e., now when you _do_ have a difference to do an
early return, you still have to touch everything in the cache).
Just for fun, I timed a quick and dirty implementation. It looks like
generating the tree actually ends up taking just a little bit longer,
even with an unchanged index (which should be the case it speeds up).
But maybe I just did it wrong. My implementation was basically just:
t = cache_tree();
if (!cache_tree_fully_valid(t))
cache_tree_update(t, active_cache, active_nr, 0, 0);
get_sha1("HEAD", sha1);
return hashcmp(t->sha1, sha1);
-Peff
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2009-02-12 0:50 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-02-10 0:51 [RFC/PATCH] shortstatus v1 Tuncer Ayaz
2009-02-10 1:44 ` Junio C Hamano
2009-02-10 3:46 ` Sitaram Chamarty
2009-02-10 10:22 ` Spending time in PS1, was " Johannes Schindelin
2009-02-10 17:31 ` Sitaram Chamarty
2009-02-10 10:11 ` Tuncer Ayaz
2009-02-10 11:03 ` Jeff King
2009-02-10 11:29 ` Michael J Gruber
2009-02-10 11:31 ` Tuncer Ayaz
2009-02-10 11:45 ` Jeff King
2009-02-10 12:36 ` Michael J Gruber
2009-02-10 13:01 ` Jeff King
2009-02-10 15:58 ` Junio C Hamano
2009-02-10 18:10 ` Jeff King
2009-02-10 18:22 ` Jeff King
2009-02-10 19:11 ` Jeff King
2009-02-10 21:21 ` Tuncer Ayaz
2009-02-10 21:36 ` Jeff King
2009-02-10 22:25 ` Junio C Hamano
2009-02-10 22:52 ` Tuncer Ayaz
2009-02-10 22:55 ` Jeff King
2009-02-10 23:05 ` Junio C Hamano
2009-02-12 0:49 ` Jeff King
2009-02-10 23:52 ` Nanako Shiraishi
2009-02-11 21:24 ` Junio C Hamano
-- strict thread matches above, loose matches on Subject: below --
2009-02-10 23:58 [RFC] New command: 'git snapshot' Ulrik Sverdrup
2009-02-11 0:08 ` [RFC/PATCH] shortstatus v1 Nanako Shiraishi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).