Re: [PATCH v2] git-add--interactive pathspec and patch additions

[Junio C Hamano <gitster@pobox.com>]

Wincent Colaiuta <win@wincent.com> writes:

> The series implements these changes in seven steps that apply on top of
> "master"; these patches are rebased/squashed ones which *replace* the
> ones sent the other day:

That's very unfortunate, as some already usable bits from the
series are already in 'next'.

>     1. Add -q/--quiet switch to git-ls-files
>
> Needed because run_cmd_pipe() doesn't propagate the child exit status
> and system() likes to be chatty on the standard out. Of the possible
> workarounds adding this switch seems to be the cleanest and most
> portable.

I do not like this very much.  If it is a problem that
run_cmd_pipe() does not properly signal you an error, wouldn't
it be much better to fix _that_ problem?  That way we do not
have to add kludge to all commands we need to run and get the
exit status out of.

>     2. Rename patch_update_file function to patch_update_pathspec
>
> Merely cosmetic.
>
>     3. Add path-limiting to git-add--interactive
>     4. Bail if user supplies an invalid pathspec

On the first read, I did not quite like 4, but I'd agree it is
probably the cleanest implementation for 3 to reject a wrong
invocation early.

>     5. Teach builtin-add to pass path arguments to git-add--interactive

I think this is already in 'next'.

>     6. Add "--patch" option to git-add--interactive
>     7. Teach builtin-add to handle "--patch" option

These should be straightforward, but use of Getopt::Long feels
way overkill for an internal command like add--interactive which
is called by only a very limited known callers (exactly one).

If we assume "a single caller", we probably can do without 1 and
4, by making the caller in builtin-add to validate the list of
pathspecs, reusing the code for "ls-files --error-unmatch",
before calling the external helper "add--interactive".

There are functions refactored as part of the builtin-commit
series to be usable from outside "ls-files", and you can build a
imple function called from interactive_add(ac, av) using them:

static int validate_pathspec(const char *prefix, const char **pattern)
{
	int i, ret;
	char *m;

	if (!pattern || !*pattern)
        	return 0;

	for (i = 0; pattern[i]; i++)
		;
	m = xcalloc(1, i);

	for (i = 0; i < active_nr; i++) {
		struct cache_entry *ce = active_cache[i];
                (void) pathspec_match(pattern, m, ce->name, 0);
	}
        ret = report_path_error(m, pattern, prefix ? strlen(prefix) : 0);
        free(m);
        return ret;
}