Re: [RFC/PATCH] shortstatus v1

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