1

Re: [PATCH] Add command -v builtin.

Details
Message ID
<20190409165645.GD21799@homura.localdomain>
DKIM signature
pass
Download raw message
On 2019-04-09  2:53 PM, dragnel wrote:
> +++ b/builtin/command.c
> -%<-
> +int builtin_command(struct mrsh_state *state, int argc, char *argv[]) {
> -%<-
> +                if(look_alias != NULL){

style: space between if and (

> +                if(path == NULL) {
> +                    fprintf(stderr, "command: $PATH doesn't exists, searching in /bin:/usr/bin\n");

style: 80 cols maximum

> +                    used_path = (char *)default_path;
> +                }
> +                else
> +                {

style: { on the same line as else

> +                    used_path = malloc(strlen(path));
> +                    strncpy(used_path,path,strlen(path)+1);

style: spaces around operators

> +                }
> +
> +                char *full_path = NULL;
> +                char *p = NULL;
> +                for (;;used_path = NULL) {

style: for (;; used_path = NULL)

However, I'm -1 to using a for loop this way. How about while (!p)

> +                    if(full_path == NULL)
> +                    {
> +                        fprintf(stderr,"PANIC out of memory\n");
> +                        return 666;
> +                    }

Where'd this come from? 666 is outside of the valid range for exit
statuses. We can probably just return 1 here.

> +                return 0x42;

??

This project is not a joke.

> +            case 'V':
> +                fprintf(stderr, "command: `-p` and no arg not yet implemented\n");
> +                return 1;
> +            case 'p': // use getconf or not ?
> +                // not implemented

Print a warning and return nonzero here? Like you do for -V. Could
fallthrough if you want.

> +                break;
> +            default:
> +                fprintf(stderr, "command: unknown option -- %c\n", mrsh_optopt);
> +                fprintf(stderr, command_usage);
> +                return 1;

getopt should handle this for us.

Re: [PATCH] Add command -v builtin.

Details
Message ID
<4qs7lGnQDAedgarFKt60vRZcKE652LwguvyRbG8V5D0KR2Zz5PIPX--5fc2X51bUfzkGR67G8YA3R4wShFcHgVtil2oXPBYHMPxA1rhm7lo=@protonmail.com>
In-Reply-To
<20190409165645.GD21799@homura.localdomain> (view parent)
DKIM signature
pass
Download raw message
Hi,

Ok for the style.
Sorry about returns code, I forgot to change it before sending the patch.

> On Tuesday 9 April 2019 18:56, Drew DeVault <sir@cmpwn.com> wrote:
> style: for (;; used_path = NULL) However, I'm -1 to using a for loop
> this way. How about while (!p)

I took that from man strtok but you're right It's cleaner with a while

> On 2019-04-09 2:53 PM, dragnel wrote:
> > -              default:
> > -                  fprintf(stderr, "command: unknown option -- %c\\n", mrsh_optopt);
> > -                  fprintf(stderr, command_usage);
> > -                  return 1;
> >
>
> getopt should handle this for us.

I'm confused as I took that from builtin/cd.c and others builtins looks the same. What is wrong ?

I'm adding the search in keyword and builtins, and may send a v2 soon.

P.S : It's my first time dealing with this workflow and contributing, I'm trying my best but my setup may be flawed. Thanks for your time.