Re: [PATCH 09/17] pathspec: always show mnemonic and name in unsupported_magic

2016-12-07 Thread Brandon Williams 
On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > @@ -413,10 +411,9 @@ void parse_pathspec(struct pathspec *pathspec,
> > prefixlen = prefix ? strlen(prefix) : 0;
> >
> > for (i = 0; i < n; i++) {
> > -   unsigned short_magic;
> > entry = argv[i];
> >
> > -   item[i].magic = prefix_pathspec(item + i, _magic,
> > +   item[i].magic = prefix_pathspec(item + i,
> > flags,
> > prefix, prefixlen, entry);
> 
> The final output looks a bit ...um.. strangely tall, with the first
> two lines that have one argument each, then the last line comes with
> three arguments. Maybe put 'flags' in the same line as 'item + i'?

Yep you're right, it does look a bit funny.

-- 
Brandon Williams

Re: [PATCH 09/17] pathspec: always show mnemonic and name in unsupported_magic

2016-12-07 Thread Brandon Williams 
On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > @@ -426,8 +423,7 @@ void parse_pathspec(struct pathspec *pathspec,
> > nr_exclude++;
> > if (item[i].magic & magic_mask)
> > unsupported_magic(entry,
> > - item[i].magic & magic_mask,
> > - short_magic);
> > + item[i].magic & magic_mask);
> 
> Same here. Maybe put both arguments in the same line. It looks a bit
> better. (sorry for two mails on the same patch, I'm reading the final
> output first before going through individual patches that breaks this
> function down)

All good.  Sometimes its easier to parse comments if they are in
multiple small emails.  I don't mind getting lots of mail :)

> 
> >
> > if ((flags & PATHSPEC_SYMLINK_LEADING_PATH) &&
> > has_symlink_leading_path(item[i].match, item[i].len)) {
> -- 
> Duy

-- 
Brandon Williams

Re: [PATCH 09/17] pathspec: always show mnemonic and name in unsupported_magic

2016-12-07 Thread Duy Nguyen 
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> @@ -426,8 +423,7 @@ void parse_pathspec(struct pathspec *pathspec,
> nr_exclude++;
> if (item[i].magic & magic_mask)
> unsupported_magic(entry,
> - item[i].magic & magic_mask,
> - short_magic);
> + item[i].magic & magic_mask);

Same here. Maybe put both arguments in the same line. It looks a bit
better. (sorry for two mails on the same patch, I'm reading the final
output first before going through individual patches that breaks this
function down)

>
> if ((flags & PATHSPEC_SYMLINK_LEADING_PATH) &&
> has_symlink_leading_path(item[i].match, item[i].len)) {
-- 
Duy

Re: [PATCH 09/17] pathspec: always show mnemonic and name in unsupported_magic

2016-12-07 Thread Duy Nguyen 
On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> @@ -413,10 +411,9 @@ void parse_pathspec(struct pathspec *pathspec,
> prefixlen = prefix ? strlen(prefix) : 0;
>
> for (i = 0; i < n; i++) {
> -   unsigned short_magic;
> entry = argv[i];
>
> -   item[i].magic = prefix_pathspec(item + i, _magic,
> +   item[i].magic = prefix_pathspec(item + i,
> flags,
> prefix, prefixlen, entry);

The final output looks a bit ...um.. strangely tall, with the first
two lines that have one argument each, then the last line comes with
three arguments. Maybe put 'flags' in the same line as 'item + i'?

> if ((flags & PATHSPEC_LITERAL_PATH) &&
>



-- 
Duy