Aarni Koskela <aarni.koskela@xxxxxxxxxxxxxxxxxxxxx> writes:
> Hello!
>
> This patch makes it possible to select or unselect files in `git add -i`
> by regular expression instead of unique prefix only.
>
> The command syntax is `/foo` for selection and `-/foo` for unselection.
> I don't think the syntax will conflict with any existing use cases, but feel
> free to prove me wrong.
Usually the responsibility to ensure correctness lies on the person
who proposes a change, not those who relies on the continued correct
operation of the existing code.
> I'm not a Perl programmer, but I've tried to follow the style of the
> existing code as much as possible. :)
>
> Note I'm currently not on the mailing list, so please cc.
>
> Best regards,
>
> Aarni Koskela
All of the above do not belong to the commit log message. Please
have these commentaries after the three-dash line you have under
your Signed-off-by line.
>
> From 53c12d9c9928dc93a57595e92d785ecc0b245390 Mon Sep 17 00:00:00 2001
> From: Aarni Koskela <akx@xxxxxx>
> Date: Mon, 1 Dec 2014 11:06:10 +0200
> Subject: [PATCH] git-add--interactive: allow list (un)selection by regular
> expression
Remove these four lines when sending out a patch in the e-mail.
> Teach `list_and_choose` to allow `/regexp` and `-/regexp` syntax to
> select items based on regular expression match.
>
> For instance, `/jpg$` will select all options whose display name ends with
> `jpg`.
It has been a long time since I wrote this code, but is this about
the selection that happens after showing you a list of filenames to
choose from? "all options" together with "jpg" made me go "Huh?
Does any of our command have jpeg related options? We are not in
the image processing business".
Perhaps s/all options/all files/ or something.
> Signed-off-by: Aarni Koskela <akx@xxxxxx>
> ---
> git-add--interactive.perl | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 1fadd69..34cc603 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -483,6 +483,8 @@ sub is_valid_prefix {
> !($prefix =~ /[\s,]/) && # separators
> !($prefix =~ /^-/) && # deselection
> !($prefix =~ /^\d+/) && # selection
> + !($prefix =~ /^\//) && # regexp selection
> + !($prefix =~ /^-\//) && # regexp unselection
> ($prefix ne '*') && # "all" wildcard
> ($prefix ne '?'); # prompt help
> }
> @@ -585,6 +587,28 @@ sub list_and_choose {
> prompt_help_cmd();
> next TOPLOOP;
> }
> + if ($line =~ /^(-)*\/(.+)$/) {
You want zero or one '-', not zero or arbitrary large number of '-',
as a sign to unchoose. (-)? instead of (-)*, perhaps?
> + my $choose = !($1 && $1 eq '-');
The first $1 is an attempt to catch non-negative case where it is
"undef"; it might be more explicit this way?
my $choose = !(defined $1);
> + my $re = $2;
Mental note. $re is an end-user supplied random string that may or
may not be a valid regular expression.
> + my $found = 0;
> + for ($i = 0; $i < @stuff; $i++) {
> + my $val = $stuff[$i];
> + my $ref = ref $val;
> + if ($ref eq 'ARRAY') {
> + $val = $val->[0];
> + }
> + elsif ($ref eq 'HASH') {
> + $val = $val->{VALUE};
> + }
> + if ($val =~ /$re/) {
... which makes me wonder what happens when $re is a bogus regular
expression here. Does the program die? Does the user get an error
message about bad regexp and the condition to this if expression is
false? Something else?
Perhaps validating and catching regexp errors early like this might
be sufficient:
my $re = eval { qr/$2/ };
if (!$re) {
print STDERR "$@\n";
next TOPLOOP;
}
Thanks.
--
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