top | item 41551137

(no title)

DavidWoof | 1 year ago

One of my biggest pet peeves about modern development is just how many people don't know jack about git even though they use it every day. It really annoys me when I see a pull request with 20 random commits (with messages like "temp" or "checkpoint") or when people merge main into their personal feature branch instead of just rebasing their branch (yes, sometimes that's not right, but I'm not talking about the corner cases).

I always think about using "clean up a pull request" as a fizzbuzz-ish screen in interviews. It just seems like a decent proxy for "do you care at all?".

discuss

order

actinium226|1 year ago

Just to be clear, and you probably won't disagree with this, there's nothing wrong with commits like "temp" or "checkpoint" or "WIP" (Work In Progress). I often make these sorts of commits as I'm working on stuff.

The issue is in submitting an MR/PR with those commits. There's an expectation among professionals that you make your work presentable before submitting it for review, although those who are new to the profession don't realize that this cleanup step is necessary (how could they? the intro courses don't teach this and they're usually struggling enough with the code).

I just wanted to throw this comment in here in case some newbie sees this and comes away thinking "oh, I can't have 'temp' or 'checkpoint' in my commit messages"

taberiand|1 year ago

I encourage developers to clean up their commits but so often they're either unduly nervous about breaking things or have a fascination with "preserving the history" (of their branch, which is not yet merged in anywhere).

I partly blame the excessive fear mongering around rebasing, where the strict Never Rebase a Pushed Branch rule is drilled into them and they never learn why or when they can break the rule safely.

So it's an uphill fight but I just try to teach by demonstrating, frequently, exactly how they can tidy up for the merge request.

hyperbolablabla|1 year ago

I couldn't care less about the commit messages on a PR, I care about the diff. as long as the messages are cleaned up in the squash and merge with main

tornadofart|1 year ago

Git rebase aka People rewriting history to make it LOOK like the commit was a compilable, working work result at a point in time even though the real history was something completely different, then f*cking up the rewriting of said history, then whining "please mighty Senior SWE heeeelp I don't know what happened please help my work is gone".

It all has trade-offs.I prefer seeing the dirty laundry.

Cthulhu_|1 year ago

I don't; once the feature has been merged, how you got to that point is no longer relevant; it's noise. Git is a log of code changes, if you use it as a work log you're using it wrong.

keybored|1 year ago

That the code is “compilable, runnable” is never a given even if you never use rebase.

We can sling around stereotypes of people failing and doing a bad job with their strategy. Then going to cry to someone (presumably you?). But I don’t think that advances the conversation.

Cthulhu_|1 year ago

It's a mindset problem; people use git commits as the equivalent of hitting save in their editor, and they feel like they should use it as a work log, justifying their time spent or feeling like they have to demonstrate how they got to a certain solution. It's not relevant. Ego is not important. You don't need to justify your time.

Blackthorn|1 year ago

I agree with you in spirit (people need to learn their tools more) but your examples...not so much. Merging main into the feature branch was the original intent on how to do it. PRs are sent the way you describe because GitHub literally offers the maintainer a squash option and it's a lot easier to review a pull request when history is unedited.

aulin|1 year ago

PR should be sent to review in a state that author considers ready to merge. Assuming it will be squashed and taking that as a reason for submitting dirty history is just sloppy and unrespectful.

Merging is for keeping track of a group of commits that has been taken from a feature branch and included in the mainline. Why would you clutter the feature branch with periodic main merges when you can cleanly rebase it and keep it tidy?

7bit|1 year ago

> it's a lot easier to review a pull request when history is unedited.

Ist it really? If you would See my uncleaned history, it would take you days to understand what I was even trying to do.

Your statement seems based on the assumption that someone knows how to achieve a particular thing right from the start. But that isn't always the case and there might be a lot of different approaches until the correct or best one is found.

Do you really want to review dead code that's somewhere in the commits of a feature branch?

CRConrad|1 year ago

> PRs are sent the way you describe because GitHub...

Oh, and here I though this discussion was about git. But you're talking GitHub.

keybored|1 year ago

> Merging main into the feature branch was the original intent on how to do it.

Based on?

mettamage|1 year ago

It seems there are no best practices here. I remember senior devs working with a gui for git that didn’t have rebase in it. I worked on the cli, whenever I mentioned that I rebased people looked at me with raised eyebrows since I was not a senior and new.

scott_w|1 year ago

> It just seems like a decent proxy for "do you care at all?"

Because the reality is, when it comes to Git history, no, I don't care in the slightest. I get all the information I need by:

- Reading previous PRs (the final diff)

- Checking the name on a git diff of a line

- The ticket reference

Git commits are a tool to help me write code and reverting to a "known-good" state. Once it's merged into master/main, I don't care how messy it is because 99.999999% of the time, I'll just go back to the merge commit.

keybored|1 year ago

One of the nice things about Git is that it is a fast (not just local but fast—these people care about speed) lookup program for all things relating to the code. I want all immediately useful code information inside Git. Because then I look it up quickly. Unlike having to go to at least two different web applications (PRs and issue tracker) and find the info there, often in an inferior and more convoluted format.

But it takes surprisingly little to sell back centralization and lock-in to developers, even when working on top of a decentralized tool.

mandeepj|1 year ago

> It really annoys me

Have you set those expectations with your team or the people you are working with? Your ‘style’ shouldn’t just show up in reviews

McBun|1 year ago

From my experience, nobody teaches you Git properly in school. And once you get your first job, it is too late.

We had various lectures on languages models, math, algorithms, networking... absolutely nothing on git (I did my classes between 2008 and 2013, things might have changed now)

lerax|1 year ago

same case here. almost new guy on my team we need to train them because basically just know to use what vs code exposes as git interface and some of them can replicate the same flow on terminal. rebase? that's a forbidden command for them, while our entire company use this as daily basis.

that just kinda sad. hopeless.

Cthulhu_|1 year ago

It's so frustrating. We had a reasonably okay process going on, but then another team started working in our repository on their feature completely ignoring the fairly lightweight commit message format we ask them to uphold, and they were like "doesn't matter, we'll squash merge it". But it's not even about the commit messages; it's about the workflow they have behind it, where they commit and push as often as they hit ctrl+s, which to me communicates they're just hacking around until it works.

A commit should be atomic; it should be a complete change with code and tests all adjusted, and ideally a message explaining what and why it changes. (In practice / in my line of work this doesn't happen because it's all front-end code implementing some poorly documented user story in jira, but hey).

If I'm ever involved in hiring again I'll add git usage to my list of criteria. Along with whether they can actually touch type. I can't believe that the standards have dropped so far that basic computer skills are no longer necessary apparently.

keybored|1 year ago

I share your frustration, personally. But unfortunately Git is more user-unfriendly than it needs to be, and doing things the proper way tends to get you into a rabbithole.

Like merging main back into your feature branch: just rebase. But then you often need to re-do conflict resolution. You have git-rerere but, eh, it’s not discoverable at all. But let’s say you get over that hurdle. Now the next obstacle is the “never rewrite shared history”. And if you’re in a “corporate” environment chances are that you publicize your branch when you make a PR. And it can take a few days to get approval.

Now I care. But sometimes I have doubts about whether the caring is well-founded. Exactly because sometimes people around me seem to care not one bit.