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

From: Wincent Colaiuta <win@wincent.com>
Date: Nov 23, 2007, 22:20 UTC
Message-ID: <499E75F1-88A3-411C-8B38-68D43307794D@wincent.com>
In-Reply-To: <7vk5o8smaf.fsf@gitster.siamese.dyndns.org>

El 23/11/2007, a las 22:07, Junio C Hamano escribió:

Show 9 quoted lines

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

Well, no problem, I'll redo this tomorrow if I can get some free time. Otherwise it will have to wait to Monday.

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

The problem is that run_cmd_pipe has two code paths, one for Windows and one for all the rest.

I could "fix" the non-Windows path by reading in all the command input at once and explicitly doing a close() before returning. In that case I believe that the exit status would be correctly set before the function returns. The Windows code path, however, I am reluctant to touch as I don't know its behaviour and can't test it.

The other alternative was to use qx() or backticks, but then I would have to start worrying about sanitizing paths and once again because of the dual-platform issues I was hesitant to do that. And how do you redirect to /dev/null on Windows?

In any case, I actually *like* the addition of -q to git-ls-files. I can see some places in the codebase (look in git-commit.sh, for instance) where git-ls-files is run and the output is thrown away (redirected to /dev/null) because it's not at all interesting to the caller. It seems that there is a real use case there which amounts to "just tell me whether these pathspecs are valid or not, I don't care about any of the rest".

Show 11 quoted lines

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

Well, you're probably right. I was thinking, "just in case we ever need to add more options"; but it may be that we never do, and perhaps git-add--interactive will become builtin one day anyway.

Show 5 quoted lines

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

Ok, thanks for the tip, Junio. Like I said, will look at this either tomorrow or Monday and get something posted that applies on top of "next".

Cheers, Wincent