patchdoc: fix quoting bug in credential cache example

Unquoted semicolons are considered shell argument separators, quote them so the example works correctly.

Signed-off-by: Douglas Fuller <douglas.fuller@gmail.com>
---
 Documentation/gitcredentials.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

douglas.fuller@gmail.com writes:

I think what you wanted to do might make sense, but the above justification is totally incorrect.

This is one of the examples shown, each shows possible value that can be given to credential.helper variable. Reproducing them fully:

    # run "git credential-foo"
    foo

    # same as above, but pass an argument to the helper
    foo --bar=baz

    # the arguments are parsed by the shell, so use shell
    # quoting if necessary
    foo --bar="whitespace arg"

    # you can also use an absolute path, which will not use the git wrapper
    /path/to/my/helper --with-arguments

    # or you can specify your own shell snippet
    !f() { echo "password=`cat $HOME/.secret`"; }; f

These are examples of values, and how they may have to be quoted in various environments is not discussed here.

We will not want a patch that says that the second example is wrong because "spaces separate arguments in shell and a string with a space in it must be quoted", i.e.

    $ git -c credential.helper="foo --bar=baz" frotz

and does this

     # same as above, but pass an argument to the helper
    -foo --bar=baz
    +"foo --bar=baz"

because the quoting convention would be different depending on where it appears. In a .git/config file, i.e.

    [credential]
	helper = foo --bar=baz

is perfectly fine without quoting.

    $ git -c credential.helper='!f() { echo "password=`cat ...`"; }; f' frotz

would be how you would pass a one-shot config from shell.

Now, the reason why I said what you did is correct but the justification is wrong is because the semicolon does pose a problem in the .git/config file. In fact

    [credential]
	helper = !f() { echo "password=`cat ...`"; }; f

would *NOT* work, because semicolon introduces a comment in the configuration file.

For this particular case, you can just do

    [credential]
	helper = !echo password=`cat $HOME/.secret`

without any quoting issues, though.

Having said all that, I think we should clarify what these sample strings are in the introductory text in the examples.

I've always thought that they are illustrating possible values and how to express that value in the context the values appear in is up to the readers who learn what values to write in this document (and they learn from manual for shell to learn the shell quoting convention and manual for 'git config' to learn the config quoting convention). Hence my initial reaction to your patch was "shell? Quoting for shell is outside the scope of the explanation here".

On the other hand, for anybody who assumes that these examples are literally showing what you write after "[credential] helper = " in the configuration file, the example clearly is wrong and dq may be needed (but yours is also wrong, in that it loses double quotes around the argument to echo; if ~/.secret file had a tab in it, the helper will now yield a wrong password and you won't be able to log in).

On Thu, Apr 30, 2020 at 11:09:02AM -0700, Junio C Hamano wrote:

You already said everything I was going to. :)

Yes, they were definitely meant as: here are the raw values you would want to use, and it is up to you to figure out how to get that into a config file (whether on the cmdline via "git config" or editing the file yourself).

I think we can either clarify that with a note at the beginning of the list, or we can just present it as config, like:

On Fri, May 01, 2020 at 01:57:38AM -0400, Jeff King wrote:

So here's a patch that does that. I also noticed a few other deficiencies in that final example, which are fixed in the second patch.

I hope I'm not stealing Douglas's thunder. :)

  [1/2]: gitcredentials(7): clarify quoting of helper examples
  [2/2]: gitcredentials(7): make shell-snippet example more realistic

 Documentation/gitcredentials.txt | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

-Peff

We give several helper config examples, but don't make clear that these are raw values. It's up to the user to add the appropriate quoting to put them into a config file (either by running with "git config" and quoting against the shell, or by adding double-quotes as appropriate within the git-config file).

Let's flesh them out as full config blocks, which makes the syntax more clear (and makes it possible for people to just cut-and-paste them as a starting point). I added double-quotes to any values larger than a single word. That isn't strictly necessary in all cases, but it sidesteps explaining the rules about exactly when you need to quote a value.

The existing quotes can be converted to single-quotes in one instance, and simply omitted in another for simplicity (unless you have odd whitespace in your password, the shell whitespace-splitting behavior is fine). I also swapped out backticks for our preferred $().

Reported-by: douglas.fuller@gmail.com
Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/gitcredentials.txt | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

On Mai 01 2020, Jeff King wrote:

That now lacks a pair of quotes around the argument of echo.

Andreas.

On Fri, May 01, 2020 at 09:19:18AM +0200, Andreas Schwab wrote:

Right, as discussed in the commit message.

-Peff

There's an example of using your own bit of shell to act as a credential helper, but it's not very realistic:

 - It's stupid to hand out your secret password to _every_ host. In the
   real world you'd use the config-matcher to limit it to a particular
   host.

 - We never provided a username. We can easily do that in another config
   option (you can do it in the helper, too, but this is much more
   readable).

 - We were sending the secret even for store/erase operations. This
   is OK because Git would just ignore it, but a real system would
   probably be unlocking a password store, which you wouldn't want to do
   more than necessary.

Signed-off-by: Jeff King <peff@peff.net>
---
This is in fact very close to what's in my own ~/.gitconfig, except that
I swap out "cat" for the "pass" tool.

 Documentation/gitcredentials.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Jeff King <peff@peff.net> writes:

All of them make sense, but I do not think we want to encourage that loose style of passing unquoted argument to echo to lose embedded $IFS spaces that is not a SP.

On Thu, Apr 30, 2020 at 11:26:39PM -0700, Junio C Hamano wrote:

You mean dropping the quotes in the first patch?

Doing:

  echo "password=$(cat $HOME/.secret)"

already eats some trailing whitespace, though I guess if you have newlines in your password you are beyond help anyway.

I can add back in the quoted \", though it does make the code slightly harder to read.

-Peff

On Fri, May 01, 2020 at 02:32:07AM -0400, Jeff King wrote:

Or did you mean passing $1 in the test call? It definitely isn't good shell practice, but we know that we're getting a single-word action from Git, per the protocol.

Fully quoting, it looks like this:

  helper = "!f() { test \"$1\" = get && echo \"password=$(cat $HOME/.secret)\"; }; f"

which IMHO is getting a little hard to read. I think that's part of why I gave such an unfinished example in the first place. :)

-Peff

On Fri, May 01, 2020 at 02:35:13AM -0400, Jeff King wrote:

So I dunno. That's ugly, but I don't think is worth nitpicking over more. So here it is with indentation and full-on quoting.

  [1/2]: gitcredentials(7): clarify quoting of helper examples
  [2/2]: gitcredentials(7): make shell-snippet example more realistic

 Documentation/gitcredentials.txt | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

-Peff

We give several helper config examples, but don't make clear that these are raw values. It's up to the user to add the appropriate quoting to put them into a config file (either by running with "git config" and quoting against the shell, or by adding double-quotes as appropriate within the git-config file).

Let's flesh them out as full config blocks, which makes the syntax more clear (and makes it possible for people to just cut-and-paste them as a starting point). I added double-quotes to any values larger than a single word. That isn't strictly necessary in all cases, but it sidesteps explaining the rules about exactly when you need to quote a value.

The existing quotes can be converted to single-quotes in one instance, and backslash-esccaped in the other. I also swapped out backticks for our preferred $().

Reported-by: douglas.fuller@gmail.com
Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/gitcredentials.txt | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

There's an example of using your own bit of shell to act as a credential helper, but it's not very realistic:

 - It's stupid to hand out your secret password to _every_ host. In the
   real world you'd use the config-matcher to limit it to a particular
   host.

 - We never provided a username. We can easily do that in another config
   option (you can do it in the helper, too, but this is much more
   readable).

 - We were sending the secret even for store/erase operations. This
   is OK because Git would just ignore it, but a real system would
   probably be unlocking a password store, which you wouldn't want to do
   more than necessary.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/gitcredentials.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Jeff King <peff@peff.net> writes:

I meant former.  I actually am having a second thought.

I do not think this page should be the canonical place for config and shell syntax lessons, but the value we have been using as an example is a good candidate that an end user would encounter in the real life and has need to do so.

Perhaps it deserves some comment next to it?

; For the specified site, use "your_user" as the username, and ; invoke the helper, which is a short shell script. Note that the ; value of the helper variable is enclosed in a double-quote pair, ; because it has a semicolon, which will cause the rest of the line ; discarded as a comment unless quoted. The shell script in turn ; needs to quote various pieces of it in double quotes, each of ; which needs to be escaped with a backslash.

[credential "https://example.com"]
    username = your_user
    helper = "!f() { test \"$1\" = get && echo \"password=$(cat $HOME/.secret)\"; }; f"

The more I think about it, the worse it becomes X-<

Do we expect that most of our users are comfortable editing ~/.gitconfig in their editor, or do they mostly work with the "git config --global" command?

I have a feeling that my wishful-thinking answer, which is "former", is not true in the real world. Which means they not only need to quote for the parser of the configuration file, but they then need to quote that for the shell X-<.

    $ git config --global credential.https://example.com.helper \
	'"!f() { test \"$1\" = get && echo \"password=$(cat $HOME/.secret)\"; }; f"'

I wonder if it helps the users to have something that guides them to figure out how they do the above.

Yes, "these are values, go quote them as necessary" is certainly a lot more attractive position to take. But apparently that wasn't all that helpful.

Thanks.

On Fri, 2020-05-01 at 02:19 -0400, Jeff King wrote:

My one-line patch definitely wasn't thunderous -- I didn't even know how to write a comment in a config file. As posited upthread, I did interpret the example as presenting config file syntax, so I got tripped up by the semicolon there. This patch makes it more clear.

I use git (thanks!) without having looked at the code, so I figured I was a good example of the target audience for this doc. Thanks for making it more clear.

Cheers, --Doug

Jeff King <peff@peff.net> writes:

I like this style of "clarification"; it makes it clear that we are explaining the value in the config-file syntax.

But I do not think the "tentative shell function" trick is necessary. I personally think it is oversold ;-) For this particular one,

    [credential]
        helper = !echo \"password=`cat $HOME/.secret`\"

should be sufficient, perhaps? You do not even need to understand how shell functions work to understand it.

Also, "git config" indents the "var = value" lines, so it would look more natural if we indented our examples in a similar way.

Thanks.

On Thu, Apr 30, 2020 at 11:20:52PM -0700, Junio C Hamano wrote:

That will print:

  password=your-password get

See the patches I just sent for a better example. :)

We do earlier, too.

I can re-indent the patches I just set, but I'll hold off on sending in case you have any other comment.

-Peff