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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
