This should probably be done after they are properly escaped: #1311092: Branch names are escaped multiple times, incorrectly displaying special characters.

CommentFileSizeAuthor
#5 1316824-escape-branch-name-5.patch530 bytesNiklas Fiekas
#2 1316824-escape-branch-names.patch804 bytesNiklas Fiekas

Comments

helmo’s picture

helmo commented

It was decided to leave these out initially for simplicity, but I like to have them when a shell needs them.

It's unfortunately a bit more complex then filling the @branch to include quote chars.
As the same token is also used in the form origin/@branch, in which case 'origin/' should be included in the quotes.

Niklas Fiekas’s picture

Niklas Fiekas commented
Status: Active » Needs review
StatusFileSize
new 1316824-escape-branch-names.patch804 bytes

Fortunately single quotes work in origin/'branchname'. Please review this patch, that works for me.

helmo’s picture

helmo commented

Nice find, have to give it a try soonish...

helmo’s picture

helmo commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs review » Needs work

Hmm soonish can be stretched too far ;)

I've tried with a branch called: test<branch in http://drupal.org/sandbox/helmo/1104672

escapeshellarg("test<branch");

only adds single quotes around that, which would result in no quotes at all following your patch.

Niklas Fiekas’s picture

Niklas Fiekas commented
Status: Needs work » Needs review
StatusFileSize
new 1316824-escape-branch-name-5.patch530 bytes

escapeshellcmd() works on all the examples. But I am not sure if that is luck, or if it is correct in general.

eliza411’s picture

eliza411 commented
Issue tags: +needs testing on git-dev

tagging

helmo’s picture

helmo commented
Status: Needs review » Fixed

I've done some further testing.

The patch from #2 which I committed last year does not suffice.

For example a string containing less then (<) didn't get quotes.

I've now updated the condition to 

if (preg_match('/^[\w-]+/i', $version)) {

Testable on: http://git-drupal_7.redesign.devdrupal.org/node/1104672/git-instructions...

helmo’s picture

helmo commented

Darn it needed more work... now it's:

if (!preg_match('/^[\w\.-]+$/i', $version)) {

Automatically closed -- issue fixed for 2 weeks with no activity.

  • Commit e30142b on 7.x-1.x, 7.x-1.x-dev by helmo: 
    Issue #1316824 by Niklas Fiekas: Fixed Branch names should be in quotes...
  • Commit 7b59871 on 7.x-1.x, 7.x-1.x-dev by helmo:
    Issue #1316824 by Niklas Fiekas, helmo: Broaden the range of strings to...
fit_hunk’s picture

fit_hunk commented
Issue summary: View changes

branchname in double quotes as "" worked for me