Add additional make target - make patch

[wheldom01]

Updated to add the make patch target and the Runtime method to carry out the task.

[sartak]

HI @wheldom01, thanks for your contribution. I love the idea and I'd love to get us to a place where we can use it. I have two primary concerns with your patch:

• Sometimes we ship different patches for different versions of RT. See for example https://github.com/bestpractical/rt-extension-assetsql/tree/master/patches - How could we improve make patch to handle this? We'd be happy to establish and follow a naming convention if that's required.
• sub RTxPatch has a couple small issues:
• • You concatenate together the shell command. I'm a little concerned that you're escaping shell characters, most importantly whitespace. For example if $RT::BasePath has a space in it. Without escaping, make patch may end up doing nothing (in the best case scenario), the wrong thing, or something that compromises security (very unlikely but still not worth exposing our users to). The standard way to deal with this is to use the list form of Perl's system() which avoids the shell entirely. You won't be able to specify < file.patch since that is a feature shells implement, but instead you can use the -i flag to patch to provide the file that way instead.
• • if ($_ =~ m/.patch/xms) { is perhaps more clearly and correctly written as if ($_ =~ m/\.patch$/) {. This checks if the filename ends in ".patch" rather than your version which checks if the file contains the string "patch" with any character preceding it.

Thanks very much! I look forward to hearing your feedback. :)

Best,
Shawn

[wheldom01]

Hi Shawn,

I like your idea of being able to handle different versions of RT during
the patch process.
Looking at the example you gave the, first 2 scenarios seem straight
forward as they apply to a
single rt version.

However the one below may not be quite so :(

https://github.com/bestpractical/rt-extension-assetsql/blob/master/patches/rt-4.4.2-later.patch

I'm assuming that this patch applies to versions of rt >= 4.4.2 is that
correct?

If that is the case then how about the following as a directory naming
convention:

patches/4.4/ => Apply *.patch files to all 4.4 range
patches/4.4.4/ => Apply *.patch files to specific version only
patches/4.4.1-4.4.5/ => Apply *.patch files to all versions in range
patches/>4.4.1/ => Apply *.patch files to versions greater than
4.4.1 (i.e 4.4.2 and above)
patches/<4.4.3/ => Apply *.patch files to versions less than
4.4.3 (i.e 4.4.2 and below)

As for RTxPatch and system command concatenation, your suggestion is a
much better approach,
more than happy to fix that.

• if ($_ =~ m/.patch/xms) { is perhaps more clearly and correctly
  Mmm that's a couple of typos on my part I would normally write:
  if ($_ =~ m/.patch\z/xms) {

again more than happy to change to your suggestion.

All the best

Martin

On 2016-11-16 16:12, Shawn M Moore wrote:

HI @wheldom01 [1], thanks for your contribution. I love the idea and
I'd love to get us to a place where we can use it. I have two primary
concerns with your patch:

Sometimes we ship different patches for different versions of RT. See
for example
https://github.com/bestpractical/rt-extension-assetsql/tree/master/patches

• How could we improve make patch to handle this? We'd be happy to
  establish and follow a naming convention if that's required.
  *

sub RTxPatch has a couple small issues:
*

• You concatenate together the shell command. I'm a little concerned
  that you're escaping shell characters, most importantly whitespace.
  For example if $RT::BasePath has a space in it. Without escaping, make
  patch may end up doing nothing (in the best case scenario), the wrong
  thing, or something that compromises security (very unlikely but still
  not worth exposing our users to). The standard way to deal with this
  is to use the list form of Perl's system() which avoids the shell
  entirely. You won't be able to specify < file.patch since that is a
  feature shells implement, but instead you can use the -i flag to patch
  to provide the file that way instead.

• if ($_ =~ m/.patch/xms) { is perhaps more clearly and correctly
  written as if ($_ =~ m/.patch$/) {. This checks if the filename ends
  in ".patch" rather than your version which checks if the file contains
  the string "patch" with any character preceding it.

Thanks very much! I look forward to hearing your feedback. :)

Best,
Shawn

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub [2], or mute the
thread [3].

Links:

[1] https://github.com/wheldom01
[2]
#4 (comment)
[3]
https://github.com/notifications/unsubscribe-auth/AOnrJO_hNffOanZhYtTejv7QkUmXZqu5ks5q-ytigaJpZM4Kx5Zz

[wheldom01]

Hi @sartak,

I've fixed the regex issue and converted the system command to use the list invocation method as you suggested.

I've also had a stab at the handling of patches for different RT versions. I'd be interested to hear your thoughts.

All the best

Martin

[wheldom01]

Hi @sartak,

The final patch adds the ability to remove the relevant patches.

Best Regards

Martin