3 ms·

Shouldn't `safe_directory_cb` be checking the key parameter? It's ignoring it completely. So any unrelated config that has a directory in its value will also ma

Shouldn't `safe_directory_cb` be checking the key parameter? It's ignoring it completely. So any unrelated config that has a directory in its value will also mark it as safe. Unless I'm misunderstanding something?

db48x 5y ago
That does seem like a mistake, upon a cursory examination.
- Matheus28 5y ago
  I submitted a PR on github https://github.com/git/git/pull/1235 https://github.com/git/git/pull/1235. Supposedly there's a bot who will send an email, but I don't have permissions to use it... mhm...
  - db48x 5y ago
    Fun :) Going to send an email the old–fashioned way?
    - Matheus28 5y ago
      Gonna beg in the irc channel for git so they give me access to that bot. God forbid I have to format a patch the way they want me to
      - db48x 5y ago
        lol :)
      - vtbassmatt 5y ago
        You probably know this, but for anyone else following this thread: the bot is https://gitgitgadget.github.io/ https://gitgitgadget.github.io/.
waynecochran 5y ago
It's using it here on line 1042? git_config_pathname(&interpolated, key, value)
- Matheus28 5y ago
  Yes but that's a general use function, it won't check for safe.directory inside of it
  - db48x 5y ago
    Right, all that does is turn paths like ~/foo into /home/<user>/foo. I’ve no idea why it even takes the key as an argument.
    - Matheus28 5y ago
      It's so it can print an error referring to the key if there was a problem parsing.
      - db48x 5y ago
        Oh, that makes sense.
cmeacham98 5y ago
Maybe it's intended? If you specify a directory for something in your git config it sounds reasonable to assume you trust it. That said, if it is intended, I'm surprised there isn't a comment mentioning that because it certainly looks like a bug.
- Matheus28 5y ago
  I considered that too... but not sure. There's also the fact that it'll reset is_safe to 0 on each config line... which is likely not intended. Seems like a rushed patch. Unless I'm seriously misunderstanding how that read_very_early_config function works (it calls the cb for each key-value pair in the config, I'm assuming).
  - db48x 5y ago
    It does. In fact every time that function is called it completely reparses all the config files. That seems like a really weird choice to me, since there are dozens of functions that do this to check individual settings, but I guess in practice it’s not really that slow.
deleted 5y ago
[deleted]

db48x 5y ago

That does seem like a mistake, upon a cursory examination.

Matheus28 5y ago
I submitted a PR on github https://github.com/git/git/pull/1235 https://github.com/git/git/pull/1235. Supposedly there's a bot who will send an email, but I don't have permissions to use it... mhm...
- db48x 5y ago
  Fun :) Going to send an email the old–fashioned way?
  - Matheus28 5y ago
    Gonna beg in the irc channel for git so they give me access to that bot. God forbid I have to format a patch the way they want me to
    - db48x 5y ago
      lol :)
    - vtbassmatt 5y ago
      You probably know this, but for anyone else following this thread: the bot is https://gitgitgadget.github.io/ https://gitgitgadget.github.io/.

waynecochran 5y ago

It's using it here on line 1042? git_config_pathname(&interpolated, key, value)

Matheus28 5y ago
Yes but that's a general use function, it won't check for safe.directory inside of it
- db48x 5y ago
  Right, all that does is turn paths like ~/foo into /home/<user>/foo. I’ve no idea why it even takes the key as an argument.
  - Matheus28 5y ago
    It's so it can print an error referring to the key if there was a problem parsing.
    - db48x 5y ago
      Oh, that makes sense.

cmeacham98 5y ago

Maybe it's intended? If you specify a directory for something in your git config it sounds reasonable to assume you trust it. That said, if it is intended, I'm surprised there isn't a comment mentioning that because it certainly looks like a bug.

Matheus28 5y ago
I considered that too... but not sure. There's also the fact that it'll reset is_safe to 0 on each config line... which is likely not intended. Seems like a rushed patch. Unless I'm seriously misunderstanding how that read_very_early_config function works (it calls the cb for each key-value pair in the config, I'm assuming).
- db48x 5y ago
  It does. In fact every time that function is called it completely reparses all the config files. That seems like a really weird choice to me, since there are dozens of functions that do this to check individual settings, but I guess in practice it’s not really that slow.

deleted 5y ago

[deleted]