Hide tree heading when cmd has no stdout/stderr

[nastevens]

I have a use case that I haven't been able to figure out how to implement. I'd like to have a global command unpushed that goes through all my trees and prints out branch names for branches that aren't pushed upstream. I can list unpushed branches fine, but right now every tree gets a heading even if it doesn't have any unpushed branches. I'd like to be able to hide the headings for trees that don't have any unpushed branches. So for example:

# dev/advent-of-code
830b34d (incomplete-2021-19) Incomplete 2021-19
# dev/bitcurry-provisioning
# dev/cookiecutter-rust-bin
...

dev/advent-of-code has an unpushed branch so its heading should be listed, but dev/bitcurry-provisioning doesn't have any unpushed branches so I'd like to prevent having the heading printed.

If there's not a way to do this, no big deal, but I figured I'd ask! This is a really great tool by the way!

[davvid]

It'd be tough from the garden side because garden doesn't don't do any capturing of stdout/stderr -- the terminal stays connected so we don't touch the output currently. One upside of this approach is that tools that detect color by checking whether a terminal is attached will detect a terminal and use colored output.

One thing you can do is use the garden -q (--quiet) option to turn off the headings completely, and then the command could do something like this to add its own heading, perhaps?

commands:
  unpushed: |
    branches=$(get unpushed branches output)
    if test -n "$branches"
    then
        echo "# ${TREE_NAME}"
        echo "$branches"
    fi

[nastevens]

Thanks! That got me pointed me in the right direction:

if test -n "$(git unpushed)"; then
  echo "# ${TREE_NAME}"
  echo "  $(git unpushed --color=always)"
fi

One thing I noticed is that trying to use a variable didn't work because garden is doing variable expansion before running the command. So the following case:

branches=$(git unpushed)
echo "$branches"

won't work because echo "$branches" gets expanded to echo "" before running in the shell. Is this maybe a bug?

[davvid]

Indeed -- there is a syntax clash between the shellexpand syntax we use for expansions and the $variable syntax used in shells.

I'm not 100% sure whether this should be considered a bug although it could be better. There is a supported way to do it currently, though.

The current method to make this work is to use echo "$$branches".

Double-$ ($$) escapes $ so that a single literal $ makes its way into the evaluated output.

This does mean that the convention PID shell variable $$ would need to be written out as $$$$ in commands.

If this is not good enough, what could we do instead?

Option (X): Alternatives to this current behavior would be to change shellexpand so that we can specify the expansion syntax, eg. maybe we could use a handlebars-like {{var}} syntax instead of ${var}. This would be a breaking change, and we probably wouldn't want to switch to {{var}}, so I'm not really considering this as a viable option at this time. I also slightly suspect that shellexpand may not take such a change upstream.

Option (Y): Another alternative would be that we could change shellexpand so that it ignores the bare $var syntax and only processes the braced ${var} syntax. This is also a breaking change, but perhaps a smaller one because it would only affect commands and expressions that currently use the $var syntax.

I'd like to hear your thoughts on whether Option (Y) is worth exploring. It's slightly contingent on the upstream shellexpand project accepting an option to disable the bare $var syntax, though.

That said, even if we changed shellexpand then the syntax clash would still exist because ${var} is also a valid shell expression and we can only pass it through by using $${var}, so I wonder whether the existing behavior is good enough.

Is another Option (Z) alternative to leave undefined variables as-is and not expand them? I believe we currently expand them to empty strings, akin to what a shell does. Evaluating to an empty string has some advantages because it ensures that there's no ambiguity between garden variables and shell variables. The simple answer is, "shell variables require escaping", but I'm not sure if that's too rigid of a stance. Option (Z) seems fuzzier / less precise, so I'd like to hear your opinions.

If you're comfortable using $$ escaping then I'd be happy accepting that as the supported/official way to handle it going forward. The upside is that we won't have to make a breaking change and it is relatively simple to understand with no ambiguity about whether something is a garden variable or a shell variable.

This is definitely something we should decide before 1.0.

For now I'll document the $-escaping syntax so that it's easier to discover. Let me know what you think.

[davvid]

tangent: another way to make this work would be to move the command expansion into a shell expression variable:

variables:
  branches: $ git unpushed

and then "${branches}" can be used in the shell snippet without having to worry about the syntax. This isn't the answer since it side-steps the issue but I'm mentioning it in case it's helpful.

[davvid]

I think I've come up with a variant on Option (Y) that seems really simple and won't require changing shellexpand at all. This is currently my personal favorite, but I don't know if it's too subtle.

• Option (Y2): Make it so that $var is treated like a shell variable and only the braced ${var} variables are subject to expansion by pre-processing the values we send to shellexpand.

The difference between Option (Y) and Option (Y2) is that we don't need to change shellexpand to do it. We only need to perform a small string transformation before sending the values through the shellexpand machinery.

The transformation is to do the $$ escaping automatically for bare $var values. echo ${foo} $bar would become echo ${foo} $$bar automatically before handing it over to the shellexpand machinery.

A naive (slow) approach is to first replace all occurrences of $ with $$ so that we get an intermediate echo $${foo} $$foo value and then replace $${ with ${ in a 2nd pass resulting in echo ${foo} $$bar.

A more direct transformation would be do a regex(?) replacement that transforms echo ${foo} $bar into echo ${foo} $$bar in a single pass. This should be pretty straightforward.

The reason this seems better is that it lets us have the best of both worlds. We can write $shell scripts the same way we'd naturally write them and garden will only be hijacking the ${var} syntax and nothing more. Users won't have to remember to $$escape things, either.

I kinda like this idea the best. It is a breaking change, though, even though we never documented that $$ can escape things.

If we think this is a good idea I can make that change and then consider this the final behavior that we'll support going forward. It feels like the right answer to me, at least.

@grymoire7 I'd like to get your thoughts on this topic as well. Definitely let me know if this is something that would break any of your existing garden files or if you think the current behavior is better, for example.

If it would break your existing files, but we still like the idea of changing the behavior, then I can build in a configuration option to restore the current behavior. Hopefully no one's run into this yet and we can just make the change without needing a config setting, though.

[grymoire7]

@davvid I haven't rolled out garden to our entire team yet (still beta testing with the deploy coordinator on our initial use case), so we can easily pivot syntax at this point if needed.

One thing that concerns me about Y2 are cases where the braces are required to avoid ambiguity. For example, with "${foo}bar" it's not possible to use $foo and get the expected results. This and things like array expansion (${array[37]}), etc. might be an issue if we don't want to break shell syntax, yes?

If that's correct, I think some kind of escaping solution might make more sense. Also, if a user wants to use $$ to get the process id, would that just be an unsupported bit of shell syntax (seems like a minor loss, if so)?

[davvid]

Good point about ${array[...]} syntax.

As a proof of concept I implemented Y2 in #12. The braced ${var} is already reserved for garden variables so it's a pretty straightforward incremental improvement, but it might make the $ escaping more difficult for the less common${array[1]} kind of syntax.

I'll try testing out that syntax to see if there's a simple escape hatch we can use for ${array[...]} as well.

[davvid]

Testing a bit with #12 I'm going to try and extend it so that we can use $${var} so that the shell sees ${var} as as an escape hatch. It doesn't handle that currently, but the the point about the ${array[@]:0:1} syntax makes me think that we should provide for it by allowing $${array[@]:0:1}, for example, as the escaping mechanism.

[davvid]

That was pretty easy -- thanks for pointing that out. #12 now has support for using $${var} as an escape mechanism. The double-$ braced variable makes it so that the literal ${var} makes its way into the shell command.

This is now supported in #12

commands:
    example: |
        values=(one two three)
        echo $${values[@]:0:1}

Thanks for your quick feedback. I think that might be the last thing we tweak before tagging the final v0.5.0. Good stuff.

[nastevens]

Wow, thanks for the fast turnaround on this! I think what you came up with is a good compromise and your doc updates in #12 make things very clear :+1:.

[davvid]

Thanks Nick, I think what we've got in #12 is just about ready to ship, so I'll plan to tag v0.5.0 final in the next day or so.