El 23/11/2007, a las 22:07, Junio C Hamano escribió:
Wincent Colaiuta <win@xxxxxxxxxxx> 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.
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".
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.
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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
