exec command: Only switch branch when cloning

[ekohl]

When using msync exec, I was surprised to learn it switched branches. My immediate impression was that it was simply a wrapper that executes commands on checkouts.

This changes the commend to only switch branch when cloning the repository or when it's explicitly passed via the --branch option.

Right now there are no tests (I did verify it manually) because I first want to figure out if this is a good idea. @smortex would you mind having a look?

[codecov[bot]]

Codecov Report

Merging #251 (39b3169) into master (39863fc) will increase coverage by 0.02%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
+ Coverage   87.20%   87.22%   +0.02%     
==========================================
  Files          30       30              
  Lines        1016     1018       +2     
==========================================
+ Hits          886      888       +2     
  Misses        130      130              

Impacted Files	Coverage Δ
lib/modulesync.rb	98.59% <100.00%> (+0.02%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 39863fc...39b3169. Read the comment docs.

[bittner]

Heads-up: Please make sure you don't break the behavior fixed via #192 and #242.

[ekohl]

Note that this is the exec command. I'm not sure that even takes .sync.yml into account. I thought that file is only needed for rendering templates.

[smortex]

:thinking: quite unexpected as far as I think about it, yes. I had a similar issue but not exactly the same I think and @neomilium demonstrated me I was inconsistent and changing branch was better… Maybe it's not applicable here and maybe it's not applicable anymore anywhere after the recent refactoring, so let's let him shine and enlighten us: is this expected :grin: ?

[neomilium]

exec is a new feature without explicit behavior expected, but the most important thing to me during development was to keep some kind of idempotency / reproductibility. ie. Running the same command at different times and different contexts should produce the same result

My first idea was to set --branch as required option, then I add some kind of flexibility to allow "default" branch to be selected when branch wasn't supplied... I agree with you: it could be unexpected to run it without --branch option but do switch branch.

I'm not so comfortable with the supplied patch as it produce a different behavior if cloned? or not.

Maybe it could be an idea, to implement something like this:

• check if each repo have been cloned before running exec
• raise unless all repo are cloned (and notice user about msync clone)
• turn --branch into required option unless --keep-current-checkout is set

@ekohl It seems we use a different workflow to use msync, if you have some contextualized use case where its relevant to keep current checkout, I do want to ear it.

Nevertheless, I'm OK to merge this if main contributors too, maybe some behavior tests could help to keep the feature when the expected behavior were found.

[ekohl]

@ekohl It seems we use a different workflow to use msync, if you have some contextualized use case where its relevant to keep current checkout, I do want to ear it.

I'll share my previous example. I started work on something that wasn't ready, but I wanted to see the impact. So I started work on a change:

$ msync update --noop -b my-change

Then wanted to figure out what changed, so I ran

$ msync exec -- git diff

This led to unexpected results.

Other actions I can imagine are msync exec -- bundle update && bundle exec rubocop -a && git commit -am 'Rubocop autofix'.

Maybe my core issue is that I like having control over branches. Perhaps similar to msync update, it should have --offline to not perform any git operations. I've already guaranteed that everything is in order with msync update in a previous step.

Nevertheless, I'm OK to merge this if main contributors too, maybe some behavior tests could help to keep the feature when the expected behavior were found.

Any thoughts from @bastelfreak or @alexjfisher? I think you're semi-regular users.