11
\$\begingroup\$

This is actually a function from the first commit of the git source code, for the purpose stated in the title:

static int verify_path(char *path)
{
  char c;

  goto inside;
  for (;;) {
      if (!c)
          return 1;
      if (c == '/') {
inside:
          c = *path++;
          if (c != '/' && c != '.' && c != '\0')
              continue;
          return 0;
      }
      c = *path++;
  }
}

I was checking if there was an alternate way to implement this without the goto statement and ended up with this function:

static int verify_path(char *path)
{
    char c = '/';

    do {
        if (c == '/') {
            c = *path++;
            if (c == '/' || c == '.' || c == '\0')
                return 0;
        }
    } while ((c = *path++));

    return 1;
}

Do you see any problems with the new function? Is it missing anything from the original?

Let us please avoid discussing about goto.


Sample list of invalid path names:

  • /path/to/file
  • ../path/to/file
  • path//to/file
  • path/../to/file
  • path/./to/file
  • path/to/.file
  • path/to/file/
\$\endgroup\$
2
  • \$\begingroup\$ I don't see why "/path/to/file" should be invalid. The original function does reject it, but the description says it should not. \$\endgroup\$ Aug 5, 2015 at 22:20
  • \$\begingroup\$ @WilliamMorris: In the context of git, the function should allow only files within the folder that is being tracked so absolute paths should be rejected. \$\endgroup\$
    – vijairaj
    Aug 6, 2015 at 3:30

2 Answers 2

4
\$\begingroup\$

Much better

I like your version much better than the original. I don't see any cases you missed - it appears to work the same as the original function. The only comment I have is that the path argument should be marked const. But maybe you aren't allowed to change that.

\$\endgroup\$
3
\$\begingroup\$

Basically like @JS1 stated in his answer your version is better, but can still be improved.

IMHO you should distinguish between an invalid char at the start of the path and the occurance of /., // and \0.

By using a while instead of a do..while loop you can use a guard clause to continue if c != '/' which reduces the horizontal spacing.

static int verify_path(char *path)
{
    char c = *path++;

    if (c == '/' || c == '.' || c == '\0') {
        return 0;
    }

    while ((c = *path++)) {

        if (c != '/') {
            continue;
        }

        c = *path++;
        if (c == '/' || c == '.' || c == '\0') {
            return 0;
        }
    }

    return 1;
}

by extracing the check for the invalid chars to a separate method this will remove the code duplication

static int is_invalid_char(char c)
{
    return (c == '/' || c == '.' || c == '\0');
}

which then leads to

static int verify_path(char *path)
{
    char c = *path++;

    if (is_invalid_char(c)) {
        return 0;
    }

    while ((c = *path++)) {

        if (c != '/') {
            continue;
        }

        c = *path++;
        if (is_invalid_char(c)) {
            return 0;
        }
    }

    return 1;
}  

I also have added braces {} arround single statements of the if's, because it makes the code less error prone.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.