Skip to content

Add guidelines on submitting batch commits #55

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
IwoHerka wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add guidelines on submitting batch commits #55

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions git.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,26 @@ Fix nasty little bug ISSUE-51 #resolve #time 2h30m
* Keep the history *clean* and *simple*. Before pushing any branch to the
public always ensure that it conforms to the style-guide. If it doesn't:
squash, rebase, reword commits as necessary.

* When merging multiple small commits into a single change, make sure to
Copy link
Member

@maticomp maticomp May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the word "merging", which for git has a very precise meaning. Do you mean "squashing", or do you mean "posting them for review in a single pull request containing multiple commits"?

Copy link
Member Author

@IwoHerka IwoHerka May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second one. One a technical level, they can be merged, squashed, amended of rebased. I will change the wording.

Copy link
Contributor

@rkowalski4 rkowalski4 May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't merge/squash multiple small commits into one if they correspond to different tickets. imo, every commit should correspond to one ticket if possible. We could add summary as shown below in pull request overview instead.

Copy link
Contributor

@rkowalski4 rkowalski4 May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use approach you suggested but we should change merge strategy.

Copy link
Member Author

@IwoHerka IwoHerka May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that anyone will have the self-discipline to do so. This proposition relaxes git guidelines a bit to compensate for really small changes. Making separate commits in those cases is cumbersome and not really helpful anyway. Right now, for example, we have a rule to amend pull request instead of adding "Apply review changes" commit. Do you obey this rule? Adding more and more strict rules is not realistic.

Copy link
Contributor

@rkowalski4 rkowalski4 May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that overview filling is more strict than commit squashing.

To be clear: I am not against you approach, I just think than pr overview is good enough.

keep high locality of the changes. In another words, if you're changing,
Copy link

@Struchu Struchu May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "group changes in small vicinity of each other" mean in git commit context?

Copy link
Member Author

@IwoHerka IwoHerka May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means changes in the same function or same class or same logical unit of the code. The goal here is to disqualify groupings which include changes happening all over the place.

Copy link

@Struchu Struchu May 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I would use a real life example here as it is not as clear in the text.

let's say, some elements of the UI, group changes
that occur in small vicinity of each other.

* When merging multiple changes into one, make sure to _do not exceed
Copy link
Member

@maticomp maticomp May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue with wording here - do you mean "When commiting multiple changes at once, make sure..."?

Copy link
Member

@dragonee dragonee May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Myślałem że mamy po jednej zmianie zazwyczaj na commit?

Copy link
Member Author

@IwoHerka IwoHerka May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mamy, ale batch commity zdarzaja sie i beda sie zdarzac. Realistycznie, nie wymagalabym od nikogo osobno commitowania zmiany paddingu. To by wrecz zasmiecalo repo. Jezeli ktos ma pare drobnych poprawek ktore sa ze soba logicznie powiazane, to chcialbym batch commit.

Copy link
Member

@dragonee dragonee May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To dodałbym "do that only with changes that do not exceed a few lines"

Copy link
Contributor

@BarryOgg BarryOgg May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "make sure to do not" is not gramatically correct. Should rather be "make sure not to".

limit of 10 changes in one commit_. Additionally, always include
ticket/issue numbers in the commit log. For example:

```git
Improve user-experience in subscription views PROJ-10
Copy link

@Struchu Struchu May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I'm not in favour of creating commits that relate to several JIRA issues. For me it's fogging the boundaries dividing changes that are related to specific tasks/bugs etc.

Copy link
Member Author

@IwoHerka IwoHerka May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See me response to @dragonee above.

Copy link

@Struchu Struchu May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the point, but I would emphasize that this is not recommended and should be done as a last resort: otherwise people will abuse this rule.

Copy link
Contributor

@BarryOgg BarryOgg May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand - sometimes tickets are created in a very granular way (e.g. separate ticket for each letter of CRUD), and so there no logical way to separate them, and so a single commit is related to at least four tickets, in a way that doesn't seem last resort-ish to me

Copy link
Member Author

@IwoHerka IwoHerka May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"separate ticket for each letter of CRUD" - This sounds to me like 4 pull requests for 4 features.

Copy link
Member Author

@IwoHerka IwoHerka May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be completely clear, this proposition is about really small commits.


This batch includes:
* Tweak padding between buttons PROJ-13
* Ensure proper scrolling of the list-view PROJ-22
* Fix alpha layer on the toolbar PROJ-19
```

where `PROJ-10` is a super-ticket for all subsequent issues.
Copy link
Member

@maticomp maticomp May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this imply that it's valid only when PROJ-10 is a task, and PROJ-13/22/19 are subtasks, or can these issues be logically related, but not related in JIRA?

What I'm trying to say is that I feel the guide applies well to any batches of multiple changes, not necessarily strictly related by JIRA issues.

Copy link
Member Author

@IwoHerka IwoHerka May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaving here a little wiggle room. Sometimes there will be a supertask, and sometimes not (depends on the QA most of the time). The implicature of the guide is that if there are tickets, include them, and if not, consider making them. Ultimately, the choice is of the developer.


### 4. Rewriting history

Expand Down