Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Brandon Williams  writes:

>> I actually think it would make more sense to add
>> 
>> lf_to_nul () {
>> perl -pe 'y/\012/\000/'
>> }
>> 
>> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
>> this route.
>
> Turns out this function already exists in test-lib-functions.sh

;-)

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> > In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
> > we seem to avoid using "tr", even though q_to_cr and others do use
> > it.  I wonder if we had some portability issues with passing NUL
> > through tr or something?
> >
> > ... digs and finds e85fe4d8 ("more tr portability test script
> > fixes", 2008-03-12)
> >
> > So use something like
> >
> > perl -pe 'y/\012/\000/' <<\-EOF
> > ...
> > EOF
> >
> > instead, perhaps?
> 
> I actually think it would make more sense to add
> 
> lf_to_nul () {
> perl -pe 'y/\012/\000/'
> }
> 
> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
> this route.

Turns out this function already exists in test-lib-functions.sh

-- 
Brandon Williams

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Brandon Williams  writes:

> Oh there is a separate if gaurd for pathspecs which is introduced in 2/4
> and then removed once pathspec support has been added in 4/4.

Thanks; I missed to spot that when I wrote the message you are
responding to, but it indeed is there ;-)

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Stefan Beller]

On Tue, Sep 27, 2016 at 1:52 PM, Brandon Williams  wrote:
> On 09/27, Junio C Hamano wrote:
>> Junio C Hamano  writes:
>>
>> > In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
>> > we seem to avoid using "tr", even though q_to_cr and others do use
>> > it.  I wonder if we had some portability issues with passing NUL
>> > through tr or something?
>> >
>> > ... digs and finds e85fe4d8 ("more tr portability test script
>> > fixes", 2008-03-12)
>> >
>> > So use something like
>> >
>> > perl -pe 'y/\012/\000/' <<\-EOF
>> > ...
>> > EOF
>> >
>> > instead, perhaps?
>>
>> I actually think it would make more sense to add
>>
>> lf_to_nul () {
>> perl -pe 'y/\012/\000/'
>> }
>>
>> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
>> this route.
>
> my mind is drawing a blank, what does the 'lf' in 'lf_to_nul' stand for?
> line feed?
>

Yes, line feed. (Note that Git has to deal with this cross platform new lines
e.g. CRLF is common on Windows, CR was common on MAC, and LF is
Windows, so naming the new line as they are, makes sense here.)

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Brandon Williams  writes:

> my mind is drawing a blank, what does the 'lf' in 'lf_to_nul' stand for?
> line feed?

Yup.  "man 7 ascii" ;-)

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams  writes:
> > @@ -170,6 +171,27 @@ static void show_killed_files(struct dir_struct *dir)
> > }
> >  }
> >  
> > +/*
> > + * Compile an argv_array with all of the options supported by 
> > --recurse_submodules
> > + */
> > +static void compile_submodule_options(int show_tag)
> > +{
> > +   if (show_cached)
> > +   argv_array_push(_submodules_opts, "--cached");
> > +   if (show_stage)
> > +   argv_array_push(_submodules_opts, "--stage");
> > +   if (show_valid_bit)
> > +   argv_array_push(_submodules_opts, "-v");
> > +   if (show_tag)
> > +   argv_array_push(_submodules_opts, "-t");
> > +   if (line_terminator == '\0')
> > +   argv_array_push(_submodules_opts, "-z");
> > +   if (debug_mode)
> > +   argv_array_push(_submodules_opts, "--debug");
> > +   if (show_eol)
> > +   argv_array_push(_submodules_opts, "--eol");
> > +}
> 
> OK.  These are only the safe ones to pass through?  "compile" or
> "assemble" is much less important thing to say than how these are
> chosen.  "pass_supported_options()" or something?  I dunno.

Alternatively we can change this to compile all potential options (not
just the ones that are supported now) and then just error the caller
out, as you've suggested, if an unsupported option used.  'pass' may not
be the most descriptive word for this function as it isn't actually
doing the passing but rather generating an argv of options that will be
passed in the event a submodule is found and we need to kick off a child
process for it.

-- 
Brandon Williams

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> > In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
> > we seem to avoid using "tr", even though q_to_cr and others do use
> > it.  I wonder if we had some portability issues with passing NUL
> > through tr or something?
> >
> > ... digs and finds e85fe4d8 ("more tr portability test script
> > fixes", 2008-03-12)
> >
> > So use something like
> >
> > perl -pe 'y/\012/\000/' <<\-EOF
> > ...
> > EOF
> >
> > instead, perhaps?
> 
> I actually think it would make more sense to add
> 
> lf_to_nul () {
> perl -pe 'y/\012/\000/'
> }
> 
> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
> this route.

my mind is drawing a blank, what does the 'lf' in 'lf_to_nul' stand for?
line feed?

-- 
Brandon Williams

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > if (recurse_submodules &&
> > -   (show_stage || show_deleted || show_others || show_unmerged ||
> > +   (show_deleted || show_others || show_unmerged ||
> >  show_killed || show_modified || show_resolve_undo ||
> > -show_valid_bit || show_tag || show_eol || with_tree ||
> > -(line_terminator == '\0')))
> > +with_tree))
> > die("ls-files --recurse-submodules unsupported mode");
> 
> Ahh, one more thing, and this comment probably applies to 2/4 not
> this one, but if the intention is to shrink this "not supported yet"
> check as the series progresses, in the earlier step the check would
> need to make sure no pathspec is given, which is first supported in
> 4/4, I think.  It is not a big deal to require rerolling by itself,
> bit if you are rerolling 2/4 for other reasons, then...

Oh there is a separate if gaurd for pathspecs which is introduced in 2/4
and then removed once pathspec support has been added in 4/4.

-- 
Brandon Williams

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Junio C Hamano  writes:

> In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
> we seem to avoid using "tr", even though q_to_cr and others do use
> it.  I wonder if we had some portability issues with passing NUL
> through tr or something?
>
> ... digs and finds e85fe4d8 ("more tr portability test script
> fixes", 2008-03-12)
>
> So use something like
>
>   perl -pe 'y/\012/\000/' <<\-EOF
> ...
> EOF
>
> instead, perhaps?

I actually think it would make more sense to add

lf_to_nul () {
perl -pe 'y/\012/\000/'
}

to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
this route.

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Brandon Williams  writes:

>   if (recurse_submodules &&
> - (show_stage || show_deleted || show_others || show_unmerged ||
> + (show_deleted || show_others || show_unmerged ||
>show_killed || show_modified || show_resolve_undo ||
> -  show_valid_bit || show_tag || show_eol || with_tree ||
> -  (line_terminator == '\0')))
> +  with_tree))
>   die("ls-files --recurse-submodules unsupported mode");

Ahh, one more thing, and this comment probably applies to 2/4 not
this one, but if the intention is to shrink this "not supported yet"
check as the series progresses, in the earlier step the check would
need to make sure no pathspec is given, which is first supported in
4/4, I think.  It is not a big deal to require rerolling by itself,
bit if you are rerolling 2/4 for other reasons, then...

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Brandon Williams  writes:

> Pass through some known-safe options when recursing into submodules.
> (--cached, --stage, -v, -t, -z, --debug, --eol)
>
> Signed-off-by: Brandon Williams 
> ---
>  builtin/ls-files.c | 34 
> ++
>  t/t3007-ls-files-recurse-submodules.sh | 17 -
>  2 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index d4bfc60..a39367f 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -31,6 +31,7 @@ static int debug_mode;
>  static int show_eol;
>  static int recurse_submodules;
>  static const char *submodule_prefix;
> +static struct argv_array recurse_submodules_opts = ARGV_ARRAY_INIT;

I'd imagine that this is also thread-unsafe, but we do not have to
comment it ;-)

> @@ -170,6 +171,27 @@ static void show_killed_files(struct dir_struct *dir)
>   }
>  }
>  
> +/*
> + * Compile an argv_array with all of the options supported by 
> --recurse_submodules
> + */
> +static void compile_submodule_options(int show_tag)
> +{
> + if (show_cached)
> + argv_array_push(_submodules_opts, "--cached");
> + if (show_stage)
> + argv_array_push(_submodules_opts, "--stage");
> + if (show_valid_bit)
> + argv_array_push(_submodules_opts, "-v");
> + if (show_tag)
> + argv_array_push(_submodules_opts, "-t");
> + if (line_terminator == '\0')
> + argv_array_push(_submodules_opts, "-z");
> + if (debug_mode)
> + argv_array_push(_submodules_opts, "--debug");
> + if (show_eol)
> + argv_array_push(_submodules_opts, "--eol");
> +}

OK.  These are only the safe ones to pass through?  "compile" or
"assemble" is much less important thing to say than how these are
chosen.  "pass_supported_options()" or something?  I dunno.

>   if (recurse_submodules &&
> - (show_stage || show_deleted || show_others || show_unmerged ||
> + (show_deleted || show_others || show_unmerged ||
>show_killed || show_modified || show_resolve_undo ||
> -  show_valid_bit || show_tag || show_eol || with_tree ||
> -  (line_terminator == '\0')))
> +  with_tree))
>   die("ls-files --recurse-submodules unsupported mode");

Makes sense.

> +test_expect_success 'ls-files correctly outputs files in submodule with -z' '
> + cat | tr "\n" "\0" >expect <<-\EOF &&
> + .gitmodules
> + a
> + b/b
> + submodule/c
> + EOF

Hmm, what do you need "cat" for here?

In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
we seem to avoid using "tr", even though q_to_cr and others do use
it.  I wonder if we had some portability issues with passing NUL
through tr or something?

... digs and finds e85fe4d8 ("more tr portability test script
fixes", 2008-03-12)

So use something like

perl -pe 'y/\012/\000/' <<\-EOF
...
EOF

instead, perhaps?

> + git ls-files --recurse-submodules -z >actual &&
> + test_cmp expect actual
> +'