Suggestion: git stash before comparing branches?

[denny]

Hi; I'm not sure if this is possible or not, but I think Overcommit does something along these lines; when you run it, it git stashes any uncommitted code first, so that it only runs your commit/push hooks against the relevant code.

Could RubyCritic do something similar, to allow checking uncommitted code against another branch? Currently you have to commit first (as far as I'm aware).

I think the sequence currently goes:

1. git checkout [branch to compare against] (fails here if there is uncommitted code)
2. rubycritic 1st run
3. git checkout [branch you ran rubycritic in]
4. rubycritic 2nd run

If instead it went:

1. git stash
2. git checkout [branch to compare against]
3. rubycritic 1st run
4. git checkout [branch you ran rubycritic in]
5. git stash pop
6. rubycritic 2nd run

... would that work? And if so, would it be a sensible default behaviour? Or is there a reason this might be a bad idea? (Technical or philosophical) 🙂

[denny]

I'm even less sure of myself on this point, but; might this also enable comparing your uncommitted code against your committed code in the same branch?

[denny]

... I guess this could get Very Weird if people are committing their tmp/rubycritic directory 😜

Would it be possible to detect if that is the case and retain/trigger the 'fail at first step' behaviour for those people?

[etagwerker]

@denny This is an interesting idea. I tend to prefer things to work according to the principle of least astonishment.

Right now I think it is pretty clear, RubyCritic fails loudly if you have uncommitted changes.

As a maintainer I find that very annoying when I'm trying to hack a patch to a bug (because I have to commit multiple times which is not natural to my workflow), but as a user I think that is useful because RubyCritic uses git to calculate the churn of a module and later calculate the cost of the module.

As a user, I have the option to git stash my changes and then run rubycritic -b which can be annoying. But what does the user expect when running rubycritic -b with uncommitted changes in their repo?

An alternative that I have considered in the past is this: If I'm running rubycritic -b with uncommitted changes, I want rubycritic to "act as if I had committed my uncommitted changes" calculate scores, then git stash, then switch to the other branch, calculate scores, and finally show me the comparison.

In an example: I have a repo with one file (app.rb, churn = 7, complexity = 10, cost = 70)

1. I make changes to app.rb in my feature/foo branch -- I don't commit them
2. I call rubycritic -b main
3. RubyCritic calculates scores for feature/foo, considering uncommitted changes, increasing churn to 8 (even though there is no git commit)
4. Then it switches to the main branch
5. Then it calculates scores
6. Then it shows me the comparison

Wouldn't that be least astonishing if someone were running rubycritic -b with uncommitted changes?

[denny]

Yeah, the churn/cost thing is one of the reasons I'm not keen on the current behaviour; I have to commit more often than I might want to, if I want to check on the quality of the changes I've made so far, which artificially increases my churn.

I don't understand the philosophical difference between the flow you've laid out and the one I laid out, other than incrementing the cost score for purposes of the calculations in the feature branch run, which sounds sensible. You're checking the branches in the opposite order to how it currently happens, but I'm not sure if that makes any difference overall?

Either way you have to check both, which means you'll have to stash at some point (which you have), and if you don't want to surprise people (which I definitely agree with - funnily enough I sent Joel's blog post about said principle to a colleague about half an hour ago!) then surely you need to put them back in the branch they started in and pop the stash back out, otherwise they'll have a heart attack about their work in progress disappearing 🙂

But anyway, I'm not too picky about the details. As you say, as a developer the need to commit before I can compare doesn't fit my normal practices - if there's a way to make that not necessary then that's the thing I care about, not so much the details of how it's done.

[denny]

(Tangentially related; I committed a bunch of changes directly to main on a work project a few weeks ago because rubycritic died on the first half of the compare run, and I stupidly didn't realise that had left me in main rather than where I had started. I only realised when I was trying to work out why the first commit had disappeared.) 🤦🏼

[etagwerker]

You're checking the branches in the opposite order to how it currently happens, but I'm not sure if that makes any difference overall?

I'm still pretty new as a maintainer to the project, but I don't think that makes a big difference.

I think there is an issue with your proposed workflow:

If instead it went:

git stash
git checkout [branch to compare against]
rubycritic 1st run
git checkout [branch you ran rubycritic in]
git stash pop
rubycritic 2nd run

If I'm not mistaken here, I think after git stash pop RubyCritic's 2nd run will fail complaining that you have uncommitted changes.

I don't understand the philosophical difference between the flow you've laid out and the one I laid out, other than incrementing the cost score for purposes of the calculations in the feature branch run, which sounds sensible.

I think that will give the most accurate information. After all, RC should consider "uncommitted changes" as a +1 in the churn for the modules that have been modified, right?

I think maybe I sidetracked your issue because I'm interested in addressing the issue when trying to run RC with unstashed, uncommitted changes.

I wonder if we can add an experimental option which does NOT complain about uncommitted changes and it actually considers these changes to tell you how your branch compares to the other branch.

I think it would look like this:

rubycritic -b main --wip

When passing the --wip (or --development?) option, RC would do these things:

1. It won't fail because there are uncommitted changes
2. It will bump the churn by one for each of these "dirty modules"
3. It will use this "fictitious churn" in the totals calculation
4. It will loudly stash your changes before switching to the compared branch. Loudly because we want to avoid a case where RC fails in the other branch and then your changes "appear to have disappeared"

How does that sound?

[denny]

That sounds perfect! (I like --wip)