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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2007, 21:07 UTC
Message-ID: <7vk5o8smaf.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1195845650-85962-1-git-send-email-win@wincent.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'.

Show 6 quoted lines

>     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.

Show 6 quoted lines

>     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;
}