Re: [PATCH v2 1/4] builtin-add.c: restructure the code for maintainability

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 21, 2008, 07:52 UTC
Message-ID: <7v7ibfvfmh.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <7v3am3yfph.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 21 quoted lines

> That changed late 2006 when Nico allowed git-add to stage already tracked
> files as well.  We collect the paths in the work tree that match given
> pathspec, and for the directory traverser to do that job, you would need
> an empty index.
>
> 	Side note: 366bfcb (make 'git add' a first class user friendly
> 	interface to the index, 2006-12-04) is something to marvel at.  It
> 	describes the change with its documentation update fully, changes
> 	the semantics in a drastic way, with so little change.
>
>         Documentation/git-add.txt  |   53 ++++++++++++-----------
>         Documentation/tutorial.txt |   46 ++++++++++++++++++---
>         builtin-add.c              |    6 +-
>         wt-status.c                |    2 +-
>         4 files changed, 72 insertions(+), 35 deletions(-)
>
> Perhaps we can add a bit to the dir_struct we give to the traverser to
> tell it to ignore the index even if we have already read one.  That would
> be a much cleaner API enhancement than reading the index and setting aside
> while calling read_directory() which feels like a you know what I would
> call it.

Thinking about this issue a bit more, I realize that the earlier "git add -A" change was done in a quite inefficient way (i.e. it is as unefficient as "git add -u && git add ." modulo one fork/exec and read/write index). For that matter, the original "git add ." could probably be more efficient than it currently is.

The thing is, when the user asks "git add .", we do not have to examine all paths we encounter and perform the excluded() and dir_add_name() processing, both of which are slower code and use slower data structure by git standard, especially when the index is already populated.

Instead, we should be able to implement "git add $pathspec..." as:

 - read the index;

 - read_directory() to process untracked, unignored files the current way,
   that is, recursively doing readdir(), filtering them by pathspec and
   excluded(), queueing them via dir_add_name() and finally do
   add_files(); and

 - iterate over the index, filtering them by pathspec, and update only the
   modified/type changed paths but not deleted ones.

And "git add -A" will become exactly the same as above, modulo:

 - missing $pathspec means "." instead of being an error; and

 - "interate over the index" part handles deleted ones as well,
   i.e. exactly what the current update_callback() in builtin-add.c does.

It is likely that I am too tired to do this right tonight, so I'll go to bed and expect to find a nicely done patch in my mailbox by somebody else ;-).