Check for CI=true, not CI=1 for cherry-picking

[huonw]

This fixes #19127, which accidentally checked for CI=1 to determine if it's running on CI, but CI services (and GHA in particular) actually set CI=true, and so the script was operating in (unchanged) local mode on CI 💥

This will fix a bug with #19125 if running on the second commit on main (i.e. HEAD^), and so adjusts the fetch-depth comment to explain that. This patch also ensures the shallow-clone optimisations apply on CI.

[thejcannon]

Alternatively, what if we removed the workflow dispatch and had the checkout action use the event ref with fetch-depth of 2?

Same result and guaranteed to succeed

[huonw]

I think it'd be good to still fetch the milestone branch with --depth=1, but an optimisation rather than correctness 🤷‍♂️

My preference would also be to make the script more resilient internally, not just rely on calling it in precisely the right manner/in a repo with precisely the correct set-up, but I can be convinced otherwise.

[thejcannon]

Really the intended usage is for the action to handle things. The only reason this isn't only in an action is because of merge conflicts.

So I err on the side of simpler, especially considering bash is an unintelligible spaghetti to some 😅

[huonw]

Yeah, that's fair. Having to use arrays here is particularly annoying. Would you feel happier with it I flipped the condition to make it simpler:

if [[ $CI = true ]]; then
   function git_fetch_with_depth() {
     git fetch "--depth=$1" https://github.com/pantsbuild/pants "$2"
   }
else
   # ... comment ...
   function git_fetch_with_depth() {
     git fetch https://github.com/pantsbuild/pants "$2"
   }
fi

[thejcannon]

Honestly at some inflection point the convenience of Bash is outweighed by all the nuance, and you start scripting in Python.

I measure a language by the ease of understanding the intent of the code. Bash is great for capturing intent of very command-heavy scripts, but terrible of the intent of logic. Python is (depending on the author) much better at representing intent, but you have to tradeoff that you need an ecosystem for it to run/work.

[thejcannon]

...that didn't answer your question.

Honestly, like I said the script is only around for merge conflicts. So maybe we have it do less (just assume the runner already has the correct commit locally, by pulling main).

Im noodling completely different ways of accomplishing the goal. Like, what if we mixed the script (to some degree).

We probably should comment on the original PR if there's a merge conflict, sot he author knows. Maybe we just leave some steps there on manually cherry picking?

[thejcannon]

I shouldn't block this specific change though, since it is a good bugfix to the script. So...