[HN Gopher] Git Things
___________________________________________________________________
Git Things
Author : nalgeon
Score : 171 points
Date : 2024-01-01 07:10 UTC (15 hours ago)
(HTM) web link (matklad.github.io)
(TXT) w3m dump (matklad.github.io)
| cesnja wrote:
| Due to (async) code reviews slowing down the development, we've
| adopted some workflows described at
| https://martinfowler.com/articles/ship-show-ask.html and it's
| working out pretty great. The committer decides how many
| approvals they need to merge a pull request, based on how
| confident they are about the code they wrote. This allows us to
| quickly merge small incremental improvements that would otherwise
| be bundled in a crowded feature pull request.
| kageiit wrote:
| I feel like the number of reviewers should be predicated more
| on how well the reviewers understand the relevant part of the
| code review.
|
| Most code review tooling is too coarse in the sense that it
| expects a small number of reviewers to know the full context of
| the change or has too many reviewers that slow down things.
|
| Tools to show relevant parts of the change to specific
| reviewers and potentially break apart a large change into
| smaller ones automatically will go a long way especially in
| large scale codebases or when team sizes are large enough that
| the full context is not understood by everyone on the team
| ozim wrote:
| This is wrong direction IMO.
|
| That is a technical solution to something that could be
| handled by the team. While tooling might help people in the
| team still have to make same and sound decisions like
| unblocking team member that needs to do a quick fix.
| blep_ wrote:
| The 50-character summary thing is bizarre. As far as I can tell,
| what happened is someone did statistics on summary lines, found
| that the average was around 50, and then louder people with
| strong opinions missed the word "average" and declared 50 as a
| maximum.
|
| My hot take is that you should use exactly as many characters as
| you need to write a meaningful summary, and people who insist on
| using tiny terminal windows can deal with the consequences of
| their actions.
| antonvs wrote:
| For a summary line, I don't see big problem with recommending
| they be kept that short. The git commit docs say this:
|
| > Though not required, it's a good idea to begin the commit
| message with a single short (no more than 50 characters) line
| summarizing the change, followed by a blank line and then a
| more thorough description.
|
| It's like the headline of an article, it doesn't need to tell
| you everything. That's the job of the "more thorough
| description". And if you omit the summary line, then you're
| just writing a description, and the limit no longer applies.
| It's not a hard limit anyway.
|
| I'm not sure it has to do with tiny terminal windows so much as
| encouraging people to be concise. It allows you to skim commits
| without reading essays, for example.
| ufo wrote:
| To add insult to injury, that 50 char maximum sneaked its way
| into several syntax highlighting and linting tools, so now my
| computer is also complaining about my commit messages.
| cerved wrote:
| That's not what happened
| dclowd9901 wrote:
| > When fixing a bug, add a failing test first, as a separate
| commit. That way it becomes easy to verify for anyone that the
| test indeed fails without the follow up fix.
|
| I would genuinely like to know in what world this is useful
| advice. Maybe in personal projects or small teams or companies
| with a maniacal lead?
|
| I just can't imagine someone in a sufficiently large organization
| (which most companies hope to be someday) goes sifting through
| feature commits to get to the bottom of a failing test.
| pjbster wrote:
| I think the failing test is meant to help with code review. The
| commit would be rebased out prior to merging into main.
| akoboldfrying wrote:
| I don't really understand your reaction. Adding a test that
| fails before and passes after the fix is good practice (do you
| disagree?), and putting that in a separate commit is a trivial
| effort (do you disagree?)
|
| It's basically the lowest-friction way I know to do the part of
| TDD that counts.
| chris_wot wrote:
| Yeah, but it kills a git bisect run
| akoboldfrying wrote:
| How so? Adding 1 commit adds at most 1 (but usually 0) git
| bisect steps.
| F-W-M wrote:
| In one of the best teams I worked with we started doing that on
| our own. I still do it (not always, but often) on my projects.
| hmeh wrote:
| I don't think it is. By all means add a failing test, but I'd
| tend to commit it along with the code that makes it pass. This
| makes the commit cohesive. If I must commit it as work in
| progress I would, but I'd probably amend it once I had the test
| passing.
| computerfriend wrote:
| I sift through commits to get to the bottom of failing tests.
| barbegal wrote:
| >if you want to reliably record file moves during refactors in
| git, you should do two commits: the first commit just moves the
| file without any changes, the second commit applies all the
| required fixups.
|
| Yes this will record a file move in one of the commits but if you
| diff between before and after the two commits a file move might
| not be shown. When thinking about file moves in git it is worth
| remembering this comes from the diff tool not from the commits
| since the commits just show the state of the file system at the
| point it is committed.
| forrestthewoods wrote:
| The fact that Git is incapable of effectively tracking files
| across moves is such a failure of Git. It's critical
| information that ought to be robustly tracked.
| Izkata wrote:
| The flipside is when two files are combined, git blame can
| track history across both sources. Can't do that when
| explicit moves are recorded, one of the source files would be
| recorded and the others treated as new lines.
| forrestthewoods wrote:
| One of these is waaaaaay more common of an operation than
| the other.
| vinnymac wrote:
| The fact that it was brought up early in the article,
| tells us which one is a more common operation as well.
| Izkata wrote:
| Moving single functions or groups of lines (like
| extracting something common into a function) I'd say is
| more common than even that, and this same functionality
| tracks those changes across files too.
| mnsc wrote:
| Does this really work consistently? Like merging in a small
| java method into the middle of an existing class with a
| couple of methods.
| cerved wrote:
| Pretty sure the answer is no
| Izkata wrote:
| That's what the -C flag is for, to make it more/less
| sensitive. So answer is "yes, if you use it", and this is
| what it looks like: $ git blame file3 -C1
| 9afddd5e file3 (Izkata 2024-01-01 11:40:51 -0600 1)
| 9c8635a7 file2 (Izkata 2024-01-01 11:40:27 -0600 2) 4
| 9c8635a7 file1 (Izkata 2024-01-01 11:40:27 -0600 3) e
| 9afddd5e file3 (Izkata 2024-01-01 11:40:51 -0600 4)
|
| Also fun fact, -C can be given up to 3 times:
|
| 1 - Look for the source in files modified in the same
| commit
|
| 2 - Look for the source in any file that existed as of
| that same commit
|
| 3 - Look for the source in any commit
|
| Great for if you suspect the original committer didn't do
| the delete/add (move) in one commit.
| IshKebab wrote:
| I dunno, it kind of makes sense. If you do just rename a file
| it _can_ track it. If you do more than renaming, it
| immediately becomes ambiguous as to whether you actually
| renamed the file or just deleted it and made a new very
| similar file. Or multiple similar files.
|
| I think the real failing is that it isn't very good at
| handling the "rename and slightly modify" case, even when it
| theoretically could. Of course it's non-trivial to detect
| that case, and there are flags you can use to improve the
| detection but it's still not great in my experience.
|
| It might make sense to allow adding hints to the git commit
| message to help it. Dunno if anyone has tried implementing
| that.
| Karellen wrote:
| I think GP's point was that there is an argument that `git`
| should be able to track "real" metadata about what changed.
| e.g. `git mv foo bar` should be able to record that "foo"
| was definitely renamed to "bar" - even in the presence of
| massive changes. Whereas `git rm foo; git add bar` should
| be able to record that "foo" was deleted and "bar" was
| added, even if the files are substantially similar. And
| that this should be more meaningful to `git` itself than
| just hints in the commit message.
|
| (Personally, I've not come across the need for such a
| feature. But I can understand why people might want to have
| it be available.)
| IshKebab wrote:
| Yeah it sounds nice in theory but the number of edge
| cases and complications it adds are crazy.
|
| * You need to add `git cp`.
|
| * You'll mess things up if you accidentally `mv` instead
| of `git mv`.
|
| * All IDEs have to add support for this.
|
| * Have fun resolving metadata merge conflicts!
|
| That's just the things I thought if in a few seconds.
|
| IMO the sensible way to improve this is to have a place
| for Git to add hints, so that it's automatic rename
| detection algorithms work better.
| forrestthewoods wrote:
| I think you're overcomplicating it.
|
| Files just need to know where they moved from, if
| anywhere. Copy, modify, delete operations can be auto-
| detected almost all of the time. Lineage can be edited
| after the fact if needed.
| erik_seaberg wrote:
| If you use "svn mv" inconsistently, you rapidly land in
| conflict resolution hell (this was a nearly weekly ordeal
| on a former team). If you forget to use "git mv," git
| still works.
| kageiit wrote:
| I feel this advice has some merit when dealing with oss projects,
| systems/backend or small teams.
|
| It tends to break apart when working with large teams or when
| working on mobile apps where one can't just rollback a change
| easily after it's shipped. The descriptions need to be more
| detailed and capture lot more information.
|
| For example, our team would require all mobile devs to add
| information about feature flags for each change to turn the
| feature off if things go wrong. This also places additional
| review burden to make sure the flag covers all the new code
| introduced and does not interact in a bad way with other existing
| feature flags
| mfrw wrote:
| > Second, not every change needs a great commit message. If a
| change is really minor, I would say minor is an okay commit
| message!
|
| I want to slightly disagree on this for folks who are early on in
| career e.g me; I think there is a lot of value in learning to
| communicate precisely and having a habit of writing good commit
| message has certainly improved my skills, both verbal and
| written.
|
| Even though, this activity appears futile for a minor change; it
| might help to improve other skills :)
| karlshea wrote:
| You're totally right, if it's that minor then you can type in a
| word or two to describe it like "spacing".
| Izkata wrote:
| Even something that simple falls into what vs why: You can
| tell it was spacing from the diff, so the commit message
| should be "linting" or "alignment" or something similarly
| more meaningful.
| quickthrower2 wrote:
| The diff may not be visible. The person might be doing a
| complicated cherry pick or merge and scrolling through
| hundreds of commits. If the message explains it, it saves
| them digging in.
|
| My analogy is your bookshelf books still need jackets!
| Vinnl wrote:
| First line is for what, the rest of the commit message is
| for the why - if needed.
|
| As for "spacing", you can probably tell the why from the
| diff as well. The "what", however, tells you whether it's
| even worth opening the diff.
| anordal wrote:
| I also want to slightly disagree, or rather state a minimum:
|
| _The most important part of a commit message is the context._
|
| Good enough commit log: utils: Minor
| drivers: Fix assets: Update docs: Typo
|
| Hopeless commit log: Minor Fix
| Update Typo
|
| Context should answer the "what". Think of it as the one word
| you tell a new conversation participant to let them realize
| what you are talking about.
| mcluck wrote:
| Totally disagree on drivers: Fix
|
| Let's say your fix causes issues later. When I read the
| commit message, I want to understand what you were trying to
| fix so I don't accidentally re-introduce the problem when I
| fix your fix
| quickthrower2 wrote:
| For that you have external bug tracking systems. Although I
| tend to have a short summary and mention the ticket number.
| Therefore the comment stands alone but also has that extra
| detail outside.
| chris_wot wrote:
| Yeah, don't do that. The LibreOffice code is littered
| with commits from bug tracking systems we no longer have
| access to or don't exist any more.
|
| Explain the issue, and by all means mention the bug
| number, but you must explain the issue you are dealing
| with.
| mcny wrote:
| At $work, I noticed someone was regularly committing with
| just a dot . as a commit message. The problem was that
| this repo does not have automatic scss => css for
| whatever reason and there's a designated person who
| untangles the mess and fixes people's bad scss and
| regenerates the css files (among other duties). After
| about a few dozen times, they just started putting a dot
| as a commit message.
|
| Point is bad commit messages are sometimes a symptom to a
| larger problem and as with this vendor it is often a
| process problem, not an individual developer problem. In
| my opinion, the main problem here is that management
| simply doesn't care she it doesn't empower leads and
| developers who should care.
| ZealousIdeal wrote:
| I agree such patterns are indicative of systemic issues.
| In my experiences, those process issues emit their own
| structured behavioral patterns. In your example, the "."
| commit is now a convention that indicates a specific
| process happened. I am not certain what the costs of
| fixing the root problem are, though I'd anticipate they
| could be expensive and difficult at this point given the
| time commitment to restructure the repo and automate the
| scss=>css. Whereas the expense of an esoteric "." commit
| appears relatively cheap given the need of a human to
| untangle the mess which cannot be automated away (without
| a redesign).
| anordal wrote:
| Do you totally disagree that this is the minimum? I'm only
| saying that the context is the minimum. I refuse to
| disagree with you about things I'm not saying.
| mcluck wrote:
| I'm disagreeing to both points. That's not an acceptable
| commit message at all. I don't need to know you were
| working on drivers, that's obvious from the files you
| were working on. The minimum for a commit message should
| be the why, not the what
| hmeh wrote:
| There's another way that feels totally unnatural at first,
| but puts the subject (the most important part) of the commit
| message first. It makes the commit about the change and the
| code, rather than the author. This is how we do it in all of
| our projects. I fought it at first, but I was wrong and it's
| a lot better.
|
| Example:
|
| EntityStore build method records the specifier argument when
| given
|
| Article about it: https://github.com/aaronjensen/software-
| development/blob/mas...
| mytailorisrich wrote:
| Usually, in more structured professional environments every
| commit must link back to a change request, which them provides
| context.
|
| But yes, even minor changes should have a descriptive commit
| message that summarises the change in order to make commit
| history useful.
|
| Now, when the code itself needs an explanation then that should
| be in the form of a comment next to the code itself.
| 32bitkid wrote:
| I'd take it one step further: if `git log`--a hierarchical view
| of the connection of commits--is the only way you (and your
| team) interact with commit messages, then yea it's probably
| fine.
|
| But, I think the value of commit messages become really
| apparent once you (or your team) are relying on other
| visualizations of commit messages like `blame` and `bisect`
|
| Tracking down the introduction of a bug with bisect and
| resolving to a commit whose message in "minor", because the
| person in the past didn't think this change was important
| enough to describe _intent_ can quite the temporal foot-gun.
| forgotmypw17 wrote:
| I would strongly disagree with it, because it misses the
| opportunity for using the commit message as a sanity check.
| Accidents happen, sometimes I select the wrong file for the
| commit, or include changes I didn't intend.
|
| Even for the most minor commits, including "improve comment
| formatting ; utils.pl" allows skimming that message and
| comparing it to the actual commit.
|
| It also helps me be aware of what I'm doing, pointing and
| calling style.
|
| https://en.wikipedia.org/wiki/Pointing_and_calling
| Cthulhu_ wrote:
| To nitpick, the git commit already contains information about
| which files are affected; adding it explicitly to the commit
| message does not add value and, like comments, may be
| incorrect. I like the angular style guide's concept of
| "scope", like, what category or module of the application
| does it apply to.
|
| That said, an "utils" file is often a smell in code
| organization.
| forrestthewoods wrote:
| I can't help but feel that the fact that rebase and merge are
| treated as different commands is broken. This topic keeps coming
| up on HN lately and it feels like the framing is all wrong. I
| dunno.
|
| At the end of the day it's all just a sequence of commits and
| some commit labels.
| pjbster wrote:
| It feels like there's a tiny bit of conflicting advice in this.
| There's the "not rocket science" rule which results in "no CI-
| failing commits on main" and then there's this:
|
| > Third, our review process is backwards. Review is done before
| code gets into main, but that's inefficient for most of the non-
| mission critical projects out there. A better approach is to
| optimistically merge most changes as soon as not-rocket-science
| allows it, and then later review the code in situ, in the main
| branch.
|
| But the tip about adding a failing test as a separate commit on
| the feature branch wouldn't survive a merge and it wouldn't live
| long enough to be reviewed either.
|
| I like most of the advice in this article but this review one is
| giving me pause.
| epage wrote:
| The idea is one commit adds the test and another fixes it.
| 90319080 wrote:
| Wax
| weavejester wrote:
| Why bother keeping minor commits in feature branches? They just
| clutter the main branch without adding any useful information
| that a future maintainer could use.
| kvdveer wrote:
| If the minor commits are related to the feature branch, by all
| means squash it into the feature commits if you feel that makes
| your feature branch easier to review.
|
| However,should you, while developing your feature, find some
| minor thing that needs improving (e.g. adding comments,
| spelling fixes or tests to some existing API you encountered
| while developing your feature), it can be better to keep it
| separate, so there's fewer red herring in both the history and
| the review.
| cerved wrote:
| Because it's much easier to rebase ontop of and it declutters
| the important commits.
|
| Typically, you git log main --first-parent
| IshKebab wrote:
| The sensible alternative is squashing the commit, not doing a
| rebase merge.
| cerved wrote:
| no, that's the not sensible alternative
| IshKebab wrote:
| Why not?
| gverrilla wrote:
| I'm a beginner dev and I find it very odd both that vscode tries
| to limit my commit messages to 50 or 55 characters, and also that
| python linters want to make my lines of code so small. What's the
| reason behind this? Old small monitors of the past?
|
| I also don't get why fediverse, mastodon seem to limit characters
| on a post, mimicking the stupid twitter, which is a niche social
| network. I way be wrong about the limit (never tried), but these
| platforms feel for ignorant outsiders like twitter-clones with a
| different infrastructure.
| wodenokoto wrote:
| Yes, old monitors are the reason why we have line lengths as
| they are.
|
| The reason why we still have them is a mix between inertia,
| it's hard to read long lines and it's nice to be able to have
| two files side by side.
|
| Mastodon is a twitter clone, with different infrastructure.
| gverrilla wrote:
| I'm only a begginer, of course, but it seems to be long lines
| are better than broken ones.
|
| `MyDict[family][subfamily][element][property_a][value]`
|
| is much better imo than:
|
| ```
|
| MyDict[family][subfamily][element][property_a][
| value
|
| ]
|
| ```
|
| Probably the data format is what's wrong here in the first
| place, lol, but anyhow.
| cerved wrote:
| If a long line can't be hardwrapped nicely at 80 chars, it
| typically means there's room for improvement in the actual
| code.
| yungporko wrote:
| i'd say that this is much nicer than both. there is always
| a better option than an abnormally long line. personally i
| limit my lines to 120 characters
|
| MyDict [family] [subfamily]
| [element] [property_a] [value]
| gverrilla wrote:
| it's very clear indeed, but makes the code much harder to
| navigate in my opinion, and makes it look more complex
| than it really is, particularly for absolute beginners.
| In python, specifically, where indents matter, these line
| breaks seem very confusing for those starting to learn.
| Brian_K_White wrote:
| Significant whitespace is a python problem not a
| whitespace problem.
|
| I am not one of the ones who cry about Make, but many do
| and for that reason primarily.
| broscillator wrote:
| I don't think the first example is a long line. Imagine
| opening a book, and to have to read sentences that span
| _both_ pages rather than just one. It 's uncomfortable for
| the eye and it makes it harder to keep track of the motion
| of finishing one line on the right, and starting the next
| one on the left.
|
| Similar to how websites employ a lot of white space,
| imagine if instead the text went from the first pixel on
| the left to the last one on the right, it feels awkward.
| addicted wrote:
| The solution to long lines isn't to break up the line.
|
| The solution would have been (let's say JS since that's the
| most "beginner"ish language where this could even have a
| perf impact"):
|
| const myElement = myDict[family][subfamily][element];
|
| const propValue = myElement[property][value];
| dsego wrote:
| Ideally, you would never access deep props like that. First
| it's easy to make a mistake, and secondly each of those
| keys can fail if the dict doesn't contain that prop. You
| probably want to de-structure it first, depending on the
| language, or use a path getter that has a fallback value
| (or even better, use strictly defined object types). Long
| lines are usually a code smell and harder to visually parse
| and understand. I don't adhere to 80 chars, but somewhere
| around 100-120 is comfortable.
| _ZeD_ wrote:
| both of the lines you've showed are poorly readable. not
| for the length itself, but for the attached mental needs
| needed to understand the `MyDict` data structure (that is
| the only variable here in your expression).
|
| that said, having a max-length, apart from the eyesight
| reasons, have this nice side effect that make it blatantly
| obvious that one expression is definitely too complex, as
| you've shown in your second example
| kingkongjaffa wrote:
| Its mostly tribal knowledge at this point built on the echo of
| "studies show 80 is the best for comprehension and
| understanding"
|
| 80 should be fine for most single lines of good code in most
| languages.
|
| Comments can usually be longer so I have my linter ignore
| comment length.
|
| Some discussion here:
| https://stackoverflow.com/questions/578059/studies-on-optima...
|
| For me it's one of those things without a "scientific" answer,
| but the status quo is good enough that it's easier to just go
| with it and never think about it again. Like linting and
| autoformatting, tabs vs spaces, all trivial stuff that detracts
| energy from building a product for customers.
| KronisLV wrote:
| > 80 should be fine for most single lines of good code in
| most languages.
|
| It feels like what's doable in one language, might be a bit
| more restrictive and annoying to follow in another one. For
| example, Java, when written in an idiomatic way, with verbose
| variable names etc.
| DrBazza wrote:
| > 80 should be fine for most single lines of good code in
| most languages.
|
| C++ with even a modest template will flow over 80 without
| much effort.
|
| I'm now using the condensed width font Iosevka font [1] with
| 160 chars as my max width in clang-format and indents at 1.
|
| After a few days of using it, I'm converted. It was a bit odd
| looking at first, but I guess that's brain plasticity at
| work.
|
| 1. https://github.com/be5invis/Iosevka/releases
| JimBlackwood wrote:
| Hah, this reminds me of my C++ professor requiring a max line
| width of 80 characters. My first question was "why not more!?"
| aswell.
|
| He would have the homework submissions compiled to LaTeX and
| printed out - then he would write comments by hand. 80
| characters was what would fit on a line. If you had more
| characters, lines wrapped wrongly and you'd get incorrect
| looking code - so he refused to grade.
|
| I've since warmed up to the max line width idea. It's nice to
| be able to read code on a small width window (I'll rarely have
| a single full screen window with just code) and ensure each
| line is a valid syntax (and not unreadable due to softwrap in
| the editor). But I still think 80 chars is a bit extreme. 120
| is nicer in my opinion.
| sgarland wrote:
| On my docked laptop with a quad-split 43" 4K, I am definitely
| in the camp of wider is better.
|
| Then I pull out my 13" Air and remind myself that there are
| reasons for limits. For Python, I just default to black,
| which is 88 chars. Easier to not argue with people that way.
| Plus, I'm free to write it with super wide columns if I want,
| and then let my precommit hook fix it.
| i80and wrote:
| Opinionated formatters are the way out of this. Black, as
| you mention, is a godsend.
|
| I'm really disappointed by how weak the dotnet formatter is
| in comparison.
| dahart wrote:
| 80 char limits used to be very common in school and business,
| I'm old enough that _most_ of my professors preferred it, and
| most students actually did too. My first programming jobs
| required it as well. There were a few students who felt
| limited, or had fun intentionally acting flippant or
| incredulous about these limits when their screens were more
| than 80 chars wide.
|
| The rule is slightly anachronistic now, and originally came
| from the time when CRT terminals (and printers) were 80
| columns of text, and there was no graphical window manager.
| The rule was primarily about making code comfortable to read
| for everyone involved, to meet the minimum display with of
| anyone in the group. If you have a wide display but I don't
| then it sucks for me. Plus I'm pretty sure that when I first
| started programming, a lot of printers and text editors and a
| lot of software wasn't very good at handling long lines and
| characters would get mugged or discarded. Almost everything
| today will just wrap and display reasonably, but that wasn't
| always the case back in the days of ed/edlin and earlier.
|
| These days I hardly ever hear about width limits anymore,
| though it does come up every once in a while when people have
| to work a lot in terminal or ssh sessions. I do have to drop
| into Linux non-graphical mode all the time, and even edits
| files and build code sometimes, and code that's wider than
| the display is less fun to wade through. People still like
| having a reasonable limit, like we'd definitely get
| complaints if we start writing code that's 500 characters on
| a line, right? :P
| thejosh wrote:
| Not sure if you mean the git commit "title" and the "body", but
| the way git works is that the first line is the summary/title
| and then a blank line, then the body/description.
|
| I really like this approach, a nice short message with a
| summary, followed by a longer description in the "body".
|
| One feature I really like from GitHub is you can set an option
| when merging a Pull Requests that when merging the commit title
| will be the Pull Request title, and the body of the PR as the
| message.
| zaptheimpaler wrote:
| Yeah the git thing is just a dumb hangover from very old times
| that really should not exist anymore. The lines should be as
| long as they need to be, and the frontend like the git CLI or
| GitHub can wrap them if needed. Thats how we do it in literally
| any other sane context because the frontend has the information
| on the available display size. "ls" doesn't randomly force you
| into 80char columns it adjusts. I don't like break this comment
| every 50 chars either. Only in git world everyone accepts this
| insanity.
|
| Join me, ignore the dumb red lines and make your commit message
| as long as you like. Linters are usually easy to configure and
| set to wrap at e.g 120 as well.
| kissgyorgy wrote:
| The first line of the commit (the subject) should be short,
| succinct, but descriptive, because this way it will fit in all
| kinds of GUIs, CLI whatnot. There are a lot of interfaces for
| Git you doesn't even know about.
| kzrdude wrote:
| mastodon instances can choose their own limit
| heads wrote:
| There are plenty of good reasons for short line lengths. Two
| factors constrain the max line length down to something not
| much more than 80 columns: side-by-side diffs and waning
| eyesight.
|
| In-line diffs mix the flow of change in with the flow of
| execution, both going down the page. Side-by-side diffs makes
| those two axes orthogonal. With decorations (either in a
| terminal or a code review tool) even 80 column code requires a
| fairly small font. Editing two panes of code only needs 161
| columns but a decorated patch might be more like 170.
|
| The point font size is its height in increments of 1/72", and
| most fonts are half as wide as their point height. The largest
| font one can use on a 27" monitor (24" wide) is 20pt. My eyes
| get knackered at anything smaller than 18pt.
| IshKebab wrote:
| Yeah it made sense for old monitors and then a lot of people
| got into their minds that it was some kind of rule that could
| never be changed.
|
| The limits you should observe today are more like:
|
| * Code line length: ~120-140 characters. Longer than that and
| it gets unwieldy especially in side-by-side views. But you
| don't need to be strict about it. It doesn't matter if the
| occasional line overflows. The best code formatting algorithm I
| know (the Prettier algorithm) doesn't have a strict limit.
|
| * Commit subject length: 72, because Github will overflow
| anything longer into an ellipsis.
|
| * Commit body line lines: No limit (unless you are developing
| the Linux kernel). I don't know of an interface other than
| email that doesn't wrap them.
|
| (You're going to get a lot of old farts disagreeing with this
| advice, but don't listen to them.)
| _ZeD_ wrote:
| >>> (You're going to get a lot of old farts disagreeing with
| this advice, but don't listen to them.) "I
| used to be with 'it', but then they changed what 'it' was.
| Now what I'm with isn't 'it' anymore and what's 'it' seems
| weird and scary. It'll happen to you!"
|
| seriously, it's not "being old farts", it's being reasonable
| with your eyes and your tools: while you may have 20/20 eyes,
| you will agree with me that viewing a diff of two columns of
| code at 10px fonts is not an happy thing to do, as the
| difference may be just a comma or a dot, and those on a 15"
| 4k monitor are about 0.3mm. And you cannot really view the
| whole columns without scrollbars or overflows if you set the
| font much bigger.
| matklad wrote:
| >I don't know of an interface other than email that doesn't
| wrap them.
|
| Here are some common examples:
|
| 1. GitHub's single commit view word-wraps at monitor width,
| not at a reasonable line length:
|
| https://github.com/matklad/repros/commit/9351f5c91bd1d80dbcc.
| ..
|
| The result is completely unreadable.
|
| 2. `git log` in a full-screen terminal wraps at window width,
| not at a readable length.
|
| 3. In VS Code in Magit, lines are not wrapped at all by
| default. It is possible to enable wrapping, but then they are
| wrapped at the screen edge, not at a readable line length. I
| _think_ VS Code actually has a setting to visually wrap at
| column, but I wasn't able to make that work.
|
| Additionally, if you look at that commit message, you'll see
| that, to properly soft-wrap it, you'll need a markdown parser
| to recognize an indented bullet list. VS Code actually wraps
| that list correctly, but I expect very few other tools to do
| that.
| matklad wrote:
| For comparison, this is how the message looks when wrapped
| manually:
|
| https://github.com/matklad/repros/commit/7d7dc3e3539e8cc68e
| 5...
|
| (Every time I see someone advocating for _not_ wrapping
| plain text formats, I can't get an image of a hammerhead
| shark sitting in front of a 16:9 display out of my mind :-)
| IshKebab wrote:
| Yeah that's definitely not idea, but neither is manually
| wrapping. Look what happens when your manual wrapping gets
| wrapped again!
|
| https://i.postimg.cc/x1KFkyMZ/image.png
|
| I think I'd take stupidly long lines over that tbh.
| Arch-TK wrote:
| > I'm a beginner dev and I find it very odd both that vscode
| tries to limit my commit messages to 50 or 55 characters, and
| also that python linters want to make my lines of code so
| small. What's the reason behind this? Old small monitors of the
| past?
|
| On my laptop with a 13.3" 1080p display I can fit two side-by-
| side terminals at 87 characters wide (two of these are used up
| by vim to display git symbols and linter markers, I don't use a
| number line).
|
| This is not because I can't afford a laptop with a bigger
| screen, but rather that I value portability, lightness and
| thinness over screen size. That being said, on larger desktop
| monitors I also value easy readability meaning that the font
| size is usually so large that I don't get much more than 90
| characters per column.
|
| This means I can have a man-page or other form of documentation
| as well as an interpreter window on the right side of my screen
| (in two stacked windows) and the code on the left side of my
| screen.
|
| I really don't even quite understand how people handle the
| defaults of VSCode with its enormous directory tree column and
| the minimap. about 40% of horizontal screen space is wasted in
| the default VSCode configuration. I mean, to some extent,
| multi-monitor setups help solve this, but again, I value
| portability and while I do have a multi-monitor setup, I don't
| want to be reliant on it to be able to comfortably get work
| done.
|
| So, at least for me, there's an enormous amount of value in
| keeping things to 80 characters in width. This doesn't mean I
| never go over that limit, it just means that most of my code is
| comfortably readable both when using a small laptop and when
| using a larger desktop setup.
|
| The other main reason is making it easier to read. It's not
| that helpful or easy to keep track of a line which is 120
| characters long. Not least of all because in my setup it would
| either end up off-screen or badly hard-wrapped. There's a
| reason newspapers still use columns and it's so it's easier to
| keep track of where you are in a paragraph. Now in code, it's
| not usually 10 120 character lines one after another, but
| usually it's still harder to make sense of what you're looking
| at when it's one really long line.
| Vinnl wrote:
| It's useful in situations where you don't have the entire
| commit message expanded on screen. For example, when running
| `git blame` on a file, or the same functionality in a web UI
| like here: https://vincenttunru.com/img/Commits-are-
| documentation/annot...
|
| As for regular code, similarly, there are situations where the
| code doesn't take up the full screen, e.g. in split-screen
| mode, or viewing a side-by-side diff.
|
| That said, there's a balance to be struck between facilitating
| those use cases and facilitating the common use case of reading
| (or even writing) the code, and there's no clear "right"
| answer. Which is probably why there are so many debates on
| this.
| nemoniac wrote:
| You could break this up into two steps.
|
| The first is agreeing on _a_ width limit, whatever that width
| might be. Such a limit might already be controversial but it
| seems to make sense.
|
| The second is the question of what that width limit should be.
|
| There's a fun story about the with of our railway gauges and
| roads and Roman chariots that has the lovely quote that if
| you"...wonder what horse's ass came up with it, you may be
| exactly right."
|
| https://www.snopes.com/fact-check/railroad-gauge-chariots/
|
| When it comes to an 80(ish) character limit, that does indeed
| come from old monitors, which took it from older line printers
| before them.
|
| https://en.wikipedia.org/wiki/ADM-3A
|
| But it all goes back to Hollerith cards, which were invented in
| the late 19th century and used in a US census at around that
| time.
|
| https://en.wikipedia.org/wiki/Punched_card
| kingkongjaffa wrote:
| > That's why for typical project it is useful to merge pull
| requests into the main branch -- the linear sequence of merge
| commits is a record of successful CI runs, and is a set of
| commits you want to git bisect over.
|
| This is kind of obvious after reading but no one ever explained
| this to me until now
| kissgyorgy wrote:
| Instead, adjust the asserts such that they lock down the current
| (wrong) behavior, and add a clear // TODO: comment explaining
| what would be the correct result. This prevents such tests from
| rotting and also catches cases where the behavior is fixed by an
| unrelated change.
|
| Don't do that, because they are still comments and even worse,
| the tests are lying. In pytest, there is a way to tell that those
| tests are expected to fail: xfail, so it will show in the test
| results every time and it's clear it's just a temporary things
| and should be fixed.
|
| https://docs.pytest.org/en/7.1.x/how-to/skipping.html#xfail-...
| lexicality wrote:
| > Second, not every change needs a great commit message. If a
| change is really minor, I would say minor is an okay commit
| message!
|
| It's very hard to rank them, but very high on my list of things
| that make me want to send people actionable threats is when I'm
| searching down the history of a bug to figure out if it was a
| deliberate change that had unexpected consequences or just a
| mistake is when the git blame path ends on a commit like this.
|
| I don't care how minor you think your change is, if you're
| working on something collaboratively you need to express intent
| in your commit messages!
| bad_alloc wrote:
| > if you're working on something collaboratively you need to
| express intent in your commit messages!
|
| 100%! And if somebody cannot communicate the intent, there is a
| high chance they don't understand fully what they just did.
| cerved wrote:
| I think the authors point is that some changes are so minor
| that elaboration is not required, like changing indentation.
|
| Elaborate commit messages are great, and most people write
| commit messages that could be more elaborate.
|
| But most people also tend to squash several changes together,
| because it's easier.
|
| Being overzealous about writing elaborate commit messages for
| minor cookies is IMHO probably counter productive.
| lexicality wrote:
| No change is minor enough that you don't need to explain what
| it does.
|
| If the commit message is "fix indentation" then I will ignore
| it and skip over it while searching the commit history. If
| it's "minor" or "fix" then I have to open the commit and look
| to see what it actually is.
|
| These "minor cookies" add up rapidly.
|
| (side note, people that squash important changes together are
| also on The List)
| cerved wrote:
| Let's not forget people that commit trailing whitespace, or
| different line-endings.
|
| Can we put those on a separate List?
| Vinnl wrote:
| To be fair, we've got computers (CI, pre-commit hooks)
| that can stop that.
| sgarland wrote:
| To be pragmatic, the number of times I've had to fix or
| outright create CI for other teams because they didn't
| bother to add these basics is too damn high.
|
| For anything with my team (or personal projects), where I
| can be guaranteed everyone understands exactly what I've
| set up, I add a pre-commit hook to fix minor linting
| issues and then add the change to the commit. If black,
| tf fmt, shfmt, etc. can just fix the problem, then do so,
| and don't bother asking me if I'm sure.
| Aeolun wrote:
| I love the 'merge commit for my feature branch with 100
| commits, but squash for merging release branch to master'
| type.
|
| It's as if a thousand commits cried out in anguish and were
| suddenly silenced.
| Arch-TK wrote:
| > I think the authors point is that some changes are so minor
| that elaboration is not required, like changing indentation.
|
| Is the change _just_ changing indentation or is it changing
| something else which is hidden by a bunch of indentation
| changes?
|
| The difference between a commit message like "reindent" and
| "." is that the former makes it clear that the change is
| intended to be completely superficial and (unless you're
| writing python) should have no impact on anything.
|
| Now the person who wrote that commit message may be lying, or
| may have made a mistake, but, with even a commit message such
| as "reindent", it's much easier/faster to approach the issue
| if your bisect lands on such a commit message as you can go
| straight for "well let's normalize both versions to see if
| there's an accidental change hidden in here" rather than
| getting frustrated trying to figure out what the commit was
| about.
|
| > But most people also tend to squash several changes
| together, because it's easier.
|
| Don't.
|
| I also disagree with the author of the article about having
| an unclean history, it's not that hard to keep a clean
| history and, combined with actually properly splitting and
| isolating changes, makes it much easier to review the code.
| Proper use of git isn't solely centred around making bisect
| work.
| smcameron wrote:
| > If a change is really minor, I would say minor is an okay
| commit message!
|
| This has got to be the author fishing for reactions, right? My
| reaction, were I on his team, would be to revoke commit
| privileges.
| forgotmypw17 wrote:
| > if you're working on something collaboratively
|
| If I'm working on something "by myself", I'm really
| collaborating with my future selves, who will appreciate the
| information that I'll forget after the next context switch.
| erik_seaberg wrote:
| This. If it looks like my change is blowing up in prod at 4 AM,
| and I didn't write down what I was trying to do, you are
| absolutely entitled to wake me up and demand an explanation.
| quickthrower2 wrote:
| I am a merger not a rebaser but it is probably a tab/space level
| personal quirks distinction!
| hmeh wrote:
| It's not. They are different things and have different
| purposes. Saying you're a merger is like saying you're a left
| turner. You can be an ambiturner.
|
| Here's a quick guide:
|
| Working on a feature branch with someone else and you have
| local commits and they have remote commits? Rebase
|
| Need to rewrite history? Rebase
|
| For just about everything else, there's merge.
| aliceryhl wrote:
| Regarding the review process ... one thing that I find
| challenging and don't know a good solution to is documentation.
| I've received many PRs where the change itself is fine, but the
| PR is dragging out because the documentation is lacking, and
| getting the PR author to improve it sometimes takes a lot of
| review rounds.
|
| What would you do to avoid this?
|
| Sometimes the same situation comes up with tests, but it is not
| as common in my experience.
| seb1204 wrote:
| Make it clear that documentation is part of the code. Missing
| or poor docu = code not acceptable.
| FPGAhacker wrote:
| Get people to write the docs first. Not many people like
| writing docs after the fact, and much of the value of working
| documentation is lost if you do it after the implementation.
|
| Assuming we're not taking about user guide kind of docs, then a
| major benefit of writing docs first is to clarify your
| thinking. Being able to explain your intent in the written word
| is valuable because you will often uncover gaps in your
| thinking. This applies to a specification, or to acknowledging
| problem reports and updating with theories on what the cause of
| said problem is and an approach to confirming or fixing it. You
| can even reference that problem report in commits and merge
| requests. It pretty beneficial all around.
|
| And docs don't have to me masterpiece works of art. Just
| getting people to clarify intent is a huge win. Peer reviewers
| don't have time to do a super deep dive into code. If they know
| what you intended code to do, that's something many reviewers
| can check pretty quickly without having to know much context.
|
| It's selfish and naive to disregard basic documentation of
| intent.
| Vinnl wrote:
| One option would be to take an initial stab at the
| documentation yourself - that makes it clear to the submitter
| where things are unclear, because you made mistakes or omitted
| things, and they can just correct that, which is a lot more
| feasible to do than figuring out what's important while your
| head is in the code.
| Aeolun wrote:
| > Instead, adjust the asserts such that they lock down the
| current (wrong) behavior, and add a clear // TODO: comment
| explaining what would be the correct result. This prevents such
| tests from rotting and also catches cases where the behavior is
| fixed by an unrelated change.
|
| Yuck! Tests asserting wrong behavior?! Then you might as well not
| have the thing. Just remove it entirely.
| IshKebab wrote:
| Not a good idea. He explained why in the bit you quoted.
| taf2 wrote:
| I really like:
|
| [diff] colormoved = "default" colormovedws = "allow-indentation-
| change"
|
| That is great for understanding the actual code changes wish it
| was on by default.
| epage wrote:
| > Within a feature branch, not every commit necessary passes the
| tests (or even builds), and that is a useful property! Here's
| some ways this can be exploited:
|
| > When fixing a bug, add a failing test first, as a separate
| commit. That way it becomes easy to verify for anyone that the
| test indeed fails without the follow up fix.
|
| As a reviewer, I want the first commit to be passing and then the
| bugfix commit update the test to still pass
|
| - Makes change of behavior obvious to reviewer
|
| - Assumeing all commits are tested, ensures the test is testing
| the right thing
| hmeh wrote:
| > However, in a typical project, landing a trivial change is
| slow. How long would it take you to fix it's/its typo in a
| comment? Probably 30 seconds to push the actual change, 30
| minutes to get the CI results, and 3 hours for a review
| roundtrip.
|
| It doesn't have to be like this. This is a choice. There's no
| reason a team cannot work towards a world where they can push a
| small fix to master directly after running their 5 second test
| suite on their local machine.
|
| We are used to squalor, but it's not necessary. It's the result
| of a series of choices. There are other ways to do our work that
| maintain continuity of productivity. Old projects can feel like
| new projects.
|
| Any time someone writes something like this, they are normalizing
| slights against our fellow developers. They may not recognize it
| as such because it's all they've ever known, but we should
| (collectively) know that better is possible and strive for it.
| hmeh wrote:
| What does it say about us as an industry when someone gets
| downvoted for saying our work lives can be better? I always
| picture lobsters pulling each other back into boiling water.
| It's sick, honestly.
| bornfreddy wrote:
| > Instead, adjust the asserts such that they lock down the
| current (wrong) behavior, and add a clear // TODO: comment
| explaining what would be the correct result.
|
| This is awful advice. The test is there to prevent regression.
| Why would you change it to not only allow the wrong behavior, but
| to enforce it? Either fix the test or decide that you won't, in
| which case remove it. If neither option appeals to you then mark
| it as optional and let CI succeed with a warning. But changing
| the test to lock the wrong behavior? No, just no.
| ender7 wrote:
| > A better approach is to optimistically merge most changes as
| soon as not-rocket-science allows it, and then later review the
| code in situ, in the main branch. And instead of adding comments
| in web ui, just changint the code in-place, sending a new PR
| ccing the original author.
|
| This may work for small projects or open source projects that
| generally receive high-quality PRs, but this sounds truly
| infeasible for large teams or organizations.
|
| There are a couple reasons why the code review process occurs
| before merging. First, it helps keep the canonical version of the
| codebase in a "correct" state. This is, ironically, an extension
| of the "not rocket science" principle that the article mentions.
| Without this invariant, any checkout of the codebase might
| contain what is essentially "WIP" code that will waste other
| engineers' time if they have to interact with it. Second, the
| social pressure of blocking someone else's work is an important
| and necessary force for motivating code review. The idea that
| folks will go back and review code that's already been merged is
| akin to "will add tests in a followup PR". It's a pleasant lie we
| tell ourselves, but it rarely comes true.
|
| Relatedly, the article also seems to suggest that the code
| REVIEWER should be providing the changes necessary after code
| review. This is problematic for so many reasons: first, it places
| an even higher burden on code reviewers' time which no one wants;
| second, it encourages poor code hygiene if someone else is just
| going to fix up your crappy work later on; third, it robs junior
| engineers of what is perhaps their most valuable learning
| experience: getting feedback from more experienced engineers and
| acting upon it.
|
| (there are other issues here; the general thesis of the article
| leans heavily towards making code easier to _write_ rather than
| _read_, which again I think it just not appropriate for a
| codebase with a significant lifetime or number of contributors)
___________________________________________________________________
(page generated 2024-01-01 23:02 UTC)