Update Windows helper script to handle /c properly.

[maksz42]

A new version of apktool.bat is causing my scripts to fail. For some reason, a .bat ending with for /f "tokens=x" ..., when the actual number of tokens is less than expected sets %errorlevel% to '1'. What's funny is that echo %errorlevel% prints '0', and even putting rem at the end of the file works. Only calling a .bat whose last line is for /f "tokens=x" ... from another .bat causes the issue. I had a hard time debugging 😫.
Apart from that, it's just an incorrect way to check for '/c' argument.

[iBotPeaches]

I don't think I have the Windows knowledge to +1/-1 this. So I'll just ping some Windows folks to take a look.

[iBotPeaches]

@maksz42 - I haven't found anyone to review this, so this will probably sit awhile unless you have any links to reference these changes so I can learn how to properly review this.

Way too many regressions in Windows lately that I'm no longer just merging Windows script changes without a good deal of understanding of it.

[trivalik]

The original code checks hard whether the second token (1 based) is "/c". But it does this not, because "for /f" wants to have a real file! Means it could not work.

The changed code checks whether any token is a match. It could be possible that multiple "/c" occur and then multiple pause would happen, maybe not really a problem. First I thought that pull request creator called the batch file like this "cmd /A /C" then /C could also not found if the second token would work :-).

Here my version base on https://stackoverflow.com/questions/7005951/batch-file-find-if-substring-is-in-string-not-in-a-file
which would solve the possible multiple pause executions.

echo.%cmdcmdline%| findstr /R /C:"/[cC]">NUL && pause

If findstr return 1 means it finds something and so logical AND will be evaluated and pause executed.

or without regex:

echo.%cmdcmdline%| find /I "/c">NUL && pause

[maksz42]

@trivalik
for /f takes strings just fine
https://ss64.com/nt/for_f.html

I didn't think about multiple /c but why not just pause && exit /b?

First I thought that pull request creator called the batch file like this "cmd /A /C" then /C could also not found if the second token would work :-).

My problem was the script was falsely returning non zero errorcodes. I think that the original code apart from that it isn't fulfilling its job, is also triggering batch interpreter bugs.

[trivalik]

Thanks for clarifing for /f, in my inital test it was not working, but now it does.

pause && exit /b would also pause on manually executing the batch from an open cmd.

[maksz42]

I meant for %%i in (%cmdcmdline%) do if /i "%%~i"=="/c" pause && exit /b of course.
But a single & is probably better here due to no errorlevel handling so
for %%i in (%cmdcmdline%) do if /i "%%~i"=="/c" pause & exit /b.

[trivalik]

I see. From readability and short code standpoint I would still prefer echo.%cmdcmdline%| find /I "/c">NUL && pause (two && necessary to work). I read that for loop will always finish all loops, no goto/exit could interrupt them. Just nested calls could be exited see https://ss64.com/nt/exit.html.

[maksz42]

As far as I understand, loops are fully expanded but not executed. If performance is a big concern you could also make use of shift. As for the find approach, what if any parameter contains /c like --output dist/centurion.apk? I'd stick to checking every parameter individually.

[trivalik]

Nice catch. Alternative would add whitespace in front and end of regex of the findstr example workarround this. But somehow returns it on not found also errorcode 1, so your solution won.

Since you mentioned the last line determine the return value, maybe we should forward the return value of apktool?
Since apktool itself returns void a possible java failure would return an errorcode.
If so it would not really matter which version would win :-).

"%java_exe%" -jar -Duser.language=en -Dfile.encoding=UTF8 "%~dp0%BASENAME%%max%.jar" %fastCommand% %*
set ERRORCODE=%ERRORLEVEL%
rem Pause when ran non interactively
for %%i in (%cmdcmdline%) do if /i "%%~i"=="/c" pause & exit /b
exit /b %ERRORCODE%

@iBotPeaches What do you think about forwarding java error code?

[iBotPeaches]

@iBotPeaches What do you think about forwarding java error code?

I'm a bit confused what you meant by Apktool returning void. All the CLI entry points should have standard numeric exit codes.

If by forwarding you mean capturing the code from cli and passing it on. Yes that makes sense.

[maksz42]

I switched to Kubuntu and don't have a Windows machine to check this at the moment but I'm pretty sure these are equivalent:

"%java_exe%" -jar -Duser.language=en -Dfile.encoding=UTF8 "%~dp0%BASENAME%%max%.jar" %fastCommand% %*
set ERRORCODE=%ERRORLEVEL%
rem Pause when ran non interactively
for %%i in (%cmdcmdline%) do if /i "%%~i"=="/c" pause & exit /b
exit /b %ERRORCODE%

and

"%java_exe%" -jar -Duser.language=en -Dfile.encoding=UTF8 "%~dp0%BASENAME%%max%.jar" %fastCommand% %*
rem Pause when ran non interactively
for %%i in (%cmdcmdline%) do if /i "%%~i"=="/c" pause & exit /b

This is because for, pause and exit /b don't touch errorlevel. I'm sure the code in my PR was returning correct exit codes from apktool. That's why I created it.

Since apktool itself returns void

There is also System.exit()

[trivalik]

You are right they behave same, but it is anyway weird that only if you call apktool.bat return 1 happens, in case not enough token in command line are found.

@iBotPeaches We conclude both that the change of pull request is basically ok and resolves a small bug in combination with call.
The part with & exit /b is nice to have to prevent a possible second pause call.

The final version of the last line would be:

for %%i in (%cmdcmdline%) do if /i "%%~i"=="/c" pause & exit /b

[iBotPeaches]

So you are both happy as-is to merge this PR as-is?

[trivalik]

The part with & exit /b I would suggest to add, this prevents a duplicate pause call, in case on a later part the command line /c repeats.

[iBotPeaches]

Okay thanks - when I see that change in this diff. I'll merge this.

[trivalik]

After looking again on it I found right now the tilde is used to remove the quotes. This lead to that the command would also pause with:

cmd /K apktool.bat "/c"

But the tilde character has here not an effect since:

cmd /K apktool.bat "/c

fails with or without the tilde character and the batch would just exit on this line because it is not a valid syntax. This behavior is already like that in original implementation.

Since this is the last line, we could leave it like it is since the batch exits, but if you would call it from another batch it would just exit as well. To fixing this we could replace all quotes with just o before.

for %%i in (%cmdcmdline:"=o%) do if /i "%%i"=="/c" pause & exit /b

The only complain I have is, in case apktool would support the flag /c, it would be also a false positive.

To go back to the inital problem that this happens only on call from another batch. Maybe we should just add at the end an errorcode check (tested):

for /f "tokens=2" %%# in ("%cmdcmdline:"=o%") do if /i "%%#" equ "/c" pause
if errorlevel 1 exit /b 0

This includes also the replacement to fix crashs with /c", but would swallow the return code.

That lead to directly remember the return code of java and just return it.

"%java_exe%" -jar -Duser.language=en -Dfile.encoding=UTF8 "%~dp0%BASENAME%%max%.jar" %fastCommand% %*
set "JAVA_ERROR=%errorlevel%"

rem Pause when ran non interactively
for /f "tokens=2" %%# in ("%cmdcmdline:"=o%") do if /i "%%#" == "/c" pause
exit /b %JAVA_ERROR%

BTW why EQU to == is changed: https://stackoverflow.com/a/70775763/7174191

[maksz42]

After looking again on it I found right now the tilde is used to remove the quotes. This lead to that the command would also pause with:

cmd /K apktool.bat "/c"

This is by design.
Anyway, this is not the point of this PR nor is the /c switch expected to be used directly by the user.

[iBotPeaches]

thanks!

[trivalik]

@maksz42 Sure if you use the tilde, it will remove quotes. What do you mean?

Without tilde character it would be more correct and also one character shorter :-)

[maksz42]

It is valid to set switches enclosed with double quotes.
cmd "/c" echo 42 is a correct command.

[trivalik]

Wow, thanks I could not believe that!