Re: [PATCH 3/3] git-commit.sh: convert run_status to a C builtin

From: Jeff King <peff@peff.net>
Date: Sep 8, 2006, 05:42 UTC
Message-ID: <20060908054226.GA19537@coredump.intra.peff.net>
In-Reply-To: <7vzmdbqke3.fsf@assigned-by-dhcp.cox.net>

On Thu, Sep 07, 2006 at 05:20:20PM -0700, Junio C Hamano wrote:

> "status.h" and "struct status" somehow sounds too broad.
> Granted, "object.h" is also broad, but in git context "object"
> has a specific meaning.

I agree it is quite broad (as is git-runstatus). Conceptually it's another type of diff format, but making it a diff argument doesn't really makes much sense. We're at least not introducing any broadness, since there is already git-status; are we interested in fixing that name?

Names for similar concepts from other systems: tree-lint (arch), inventory (arch), whatsnew (darcs, perhaps a little too close to whatchanged), status (svn, hg), update -n (cvs, ugh!).

> Having said that I cannot come up with a good alternative name.
> It is not "project status" nor "repository status".  It is
> "working tree status", but that sounds very loooooooooooooooong.

wt_status? ucu_status (updated, changed, untracked)?

> Very nicely done.  Especially I liked that you are careful not
> to paint leading '#\t' (which is noticeable when you use reverse
> as an attribute).

Yes. I seem to recall some issues raised about color attributes persisting over a newline, but I can't find any reference to it now. Using color_printf makes sure every color is 'closed' but it sometimes includes the newline in the colorized portion. Does anybody object to that?

> Very nice code reuse.  I do not mean sarcasm -- the part copied
> and pasted from ls-files is almost trivial to bother factoring

Yes, I don't like cutting and pasting, but it seemed more confusing to try factoring it out. It's not a lot of *code*, it's just that remembering to pick out unmerged entries (and the weird -pos bit) is confusing.

> out.  What's nice is read_directory() does all what is needed to
> deal with .gitignore files, which I forgot almost all about.

I just had to write a file_exists to check for .git/gitignore, which was a little clumsy.

Show 5 quoted lines

> > +int status_foreach_cached(status_cb cb);
> > +int status_foreach_updated(status_cb cb);
> > +int status_foreach_changed(status_cb cb);
> > +int status_foreach_untracked(status_cb cb);
> I do not see them defined nor used...

My fault, they were left in from a previous iteration.

> I'll take only [1/3] for now but I am interested in 2 and 3.

OK. Besides the things you mentioned, what improvements would you like to see?

-Peff