Re: [PATCH 0/2] "git add -A/--no-all" finishing touches

Junio C Hamano [off-list ref] writes:

Jonathan Nieder [off-list ref] writes:

quoted

Maybe the warning should happen after add_file_to_index() has run,
letting git compare the old and new index entries for that path?

Yeah, new and deleted cases we do not have to worry about, so a
no-op add_file_to_index() is the only case we have to be careful.
There is a "if verbose, say 'add %s'" logic in the funciton, so it
should be possible to enhance the API without affecting existing
callers to extract that necessary information out of it.

I've thought about this a bit more.

One possible solution would go like this:

 - Extend add_file_to_index() (the logic is add_to_index() in
   read-cache.c) so that it can return an extra boolean "I would add
   it, but that would be a no-op---the index already has that
   object" to the caller.

 - In update_callback(), when we are comparing _all_ paths due to
   "implicit-dot" logic, check if the path is outside the current
   directory, instead of unconditionally calling warn_pathless_add():

   * If fix_unmerged_status() tells us that we would go to the
     remove_file_from_index() codepath, instead of calling it, call
     warn_pathless_add() instead.

   * If we are going to call add_file_to_index(), call it with
     ADD_CACHE_PRETEND on using the extended interface to see if it
     is adding already up-to-date contents. If not, call
     warn_pathless_add().

But I think it is a much better solution to just refresh the index
like the attached patch when implicit_dot is active and we are not
at the top level directory.  The paths that are stat-dirty but have
the up-to-date contents need to be hashed at least once _anyway_ to
see if the current contents match with what is in the index.  If we
use the approach outlined above, the rehashing will be done in the
extended add_file_to_index(). If we simply refresh the entire cache,
the same check will be done there.  The only performance penalty
would be that we may end up running lstat() twice.

Incidentally, I noticed that we set implicit_dot=1 even when we are
already at the top-level directory.  I suspect the code may become
somewhat simpler if we set it only when (prefix != NULL), but it
probably would not matter.


 builtin/add.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/add.c b/builtin/add.c
index daf02c6..ec2359c 100644
--- a/builtin/add.c
+++ b/builtin/add.c

@@ -495,6 +495,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		refresh(verbose, pathspec);
 		goto finish;
 	}
+	if (implicit_dot && !prefix)
+		refresh_cache(REFRESH_QUIET);
 
 	if (pathspec) {
 		int i;

Junio C Hamano wrote:

I think you mean "if (implicit_dot && prefix)". :)

This strategy is much less invasive than the alternatives discussed,
so for what it's worth, with that correction,

Acked-by: Jonathan Nieder <redacted>

I'll try to get time to work on those promised tests this weekend.