search

OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
869 stars 436 forks source link

[Suggestion] Add Changelog? #155

Closed sreichel closed 2 years ago

sreichel commented 7 years ago

Well ... why?

In my opinion the actual description does not point out what the advantages of Magento-LTS vs Vanilla Magento are. Uhhm ... OK ..."They accecpt bug fixes". Sounds not so bad, but it's not clear whats already in ...

There are

Why dont tell that - to make this project more attractive?

Cause i'm lazy i wouldn't add a CHANGLOG.md ... i would add a link to README.md that points to https://github.com/OpenMage/magento-lts/pulls?page=1&q=is%3Apr+is%3Aclosed+is%3Amerged&utf8=%E2%9C%93 and add some labels for all merged PR (bug, enhancement, patch ...)

colinmollenhour commented 7 years ago

Definitely a good idea although I think CHANGELOG.md would be more user-friendly it would also be more annoying to maintain so I'm open to either proposed method.

Flyingmana commented 7 years ago

I support this idea, as long as we dont require it for PRs. Going from time to time trough contributions to add missing entries is not this hard.

The more problematic part is removing stuff which got fixed upstream by magento

sreichel commented 7 years ago

Agreed, it shouln't be a requirement . I'd prefer the "easier to maintain" solution. Add a first label for kind of this PR (bug/patch/...).

I also wouldn't remove entries that got fixed in later Magento version. It should be enough to add a second "branch-label" that match magentos versions, where the issue was fixed.

sreichel commented 7 years ago

Don't know where to ask ...

... is there any "schedule" for merging approved PRs? I don't want to stress someone, but there are some PRs that should go live ASAP .... (i know i could use a/my fork, but i definitly want to rely on "official" LTS)

tmotyl commented 7 years ago

+1 for making human readable changelog. Unfortunately neither git history nor list of closed PR's is useful here.

Git history is hard to read because we use non linear history (merge commits) and commit descriptions/titles are also not too nice.

List of merged PR's is also not a solution (in the form it is right now) as you don't know which PR was applied on the current branch.

I think we should require every PR to add appropriate changelog entry.

Flyingmana commented 7 years ago

git log is perfectly able to produce a linear representation even with merge commits. Require changelog entries on PRs could(depending on format, config, activity and time to merge) lead to regular merge conflicts, especially if people want to backport them.

But I would like to see references to Issues in commit messages

tmotyl commented 7 years ago

@Flyingmana can you please share the command you will use to generate a changelog based on the git log? It should present in readable from all changes which are added to Magento LTS and are not part of the Vanilla Magento.

sreichel commented 7 years ago

I "slightly" improved $ git log --format="| %d% | %an | %ar | %s |" > CHANGELOG.md could possibly do it.

sreichel commented 7 years ago

+1 for the one who added some labels ;)

tmotyl commented 7 years ago

See below for example changelog generated form git history: From my point of view it's not very useful, because it doesn't answer important questions:

In my opinion we should either improve pull requests:

Or maintain the changelog manually.

merge Author Age Title
(HEAD -> 1.9.3.x, origin/1.9.3.x) Colin Mollenhour 6 days ago Merge pull request #241 from sreichel/hotfix/reports/model/resource/quote/collection-duplicate-cust_mname
Sven Reichel 6 days ago Fixed joinLeft / joinInner
Sven Reichel 7 days ago Fixed abandoned cart report
Tymoteusz Motylewski 10 days ago Merge pull request #187 from bob2021/safe-db-transactions
Rob Wigginton 10 days ago ensure all db transactions are either committed or rolled back
Colin Mollenhour 12 days ago Merge pull request #200 from OpenMage/1.9.3.0
(origin/1.9.3.0) Colin Mollenhour 13 days ago Merge branch '1.9.3.x' into 1.9.3.0
Tymoteusz Motylewski 2 weeks ago Merge pull request #144 from OpenMage/patch-2
Colin Mollenhour 2 weeks ago Merge pull request #127 from colinmollenhour/file-storage
Colin Mollenhour 2 weeks ago Merge pull request #183 from blopa/patch-1
Colin Mollenhour 2 weeks ago Merge pull request #192 from bob2021/cron-fix
Colin Mollenhour 2 weeks ago Merge pull request #181 from OpenMage/1.9.3.1
Rob Wigginton 2 weeks ago don't escape console redirection in cron.php
Pablo Benmaman 3 weeks ago fix unserialized data clitch
(origin/1.9.3.1) Colin Mollenhour 3 weeks ago Merge pull request #151 from seansan/patch-20
Daniel Fahlke 4 weeks ago Merge pull request #161 from sreichel/hotfix/catalog/etc/config/product-collection-attributes-image
Daniel Fahlke 4 weeks ago Merge pull request #176 from alissonjr/patch-1
Daniel Fahlke 4 weeks ago Merge pull request #165 from seansan/patch-21
Daniel Fahlke 4 weeks ago Merge pull request #163 from sreichel/hotfix/customer/model/observer/deleteCustomerFlowPassword
Alisson Júnior 4 weeks ago change variable name
David Robinson 4 weeks ago Merge pull request #172 from drobinson/1.9.3.2
David Robinson 5 weeks ago Imported 1.9.3.2 sources
David Robinson 5 weeks ago Merge pull request #171 from drobinson/SUPEE-9652
David Robinson 5 weeks ago Updated readme patch table
David Robinson 5 weeks ago Applied SUPEE-9652
Colin Mollenhour 5 weeks ago Merge pull request #169 from royduin/patch-1
Roy Duineveld 5 weeks ago Fixed the CSS merging data uri bug
Tymoteusz Motylewski 5 weeks ago Unify string concatenation
seansan 5 weeks ago Simple improve of Not valid template file debugging
Sven Reichel 6 weeks ago Added missing table prefix, fixes #162
Sven Reichel 8 weeks ago Added "image" attribute to flat tables
seansan 8 months ago If add qty field is empty set a default vallue of 1
seansan 10 months ago Replace font LinLibertineFont with FONT_HELVETICA: reduces PDf filesizes almost 20x fold
David Robinson 5 months ago Fix cms breadcrumb issue
Daniel Fahlke 10 weeks ago Merge pull request #119 from drobinson/1.9.3.0
Flyingmana 10 weeks ago Merge branch '1.9.3.1' of https://github.com/sreichel/magento-lts into 1.9.3.1
Flyingmana 10 weeks ago remove priority change
seansan 5 months ago fix tabs
shirtsofholland 6 months ago remove url from homepage in sitemap
Flyingmana 10 weeks ago remove priority change
Flyingmana 10 weeks ago Merge branch 'patch-17' of https://github.com/seansan/magento-lts into 1.9.3.0
seansan 2 months ago Fixes 1px line so is whole block
Sven Reichel 3 months ago Added event for sitemap.xml CMS page generation
(origin/patch-2) Daniel Fahlke 3 months ago add Flyingmana in Maintainer info
Flyingmana 4 months ago Merge tag '1.9.3.1' into 1.9.3.1
David Robinson 4 months ago Import Magento Release 1.9.3.1
Colin Mollenhour 5 months ago Merge pull request #121 from ambimax/1.9.3.0
seansan 5 months ago fix tabs
Colin Mollenhour 5 months ago Merge remote-tracking branch 'OpenMage/1.9.1' into 1.9.3.0
Colin Mollenhour 5 months ago Merge remote-tracking branch 'OpenMage/1.9.2.4' into 1.9.3.0
Colin Mollenhour 5 months ago Merge remote-tracking branch 'OpenMage/1.9.2.3' into 1.9.3.0
(origin/1.9.2.3) Colin Mollenhour 5 months ago Merge pull request #64 from seansan/patch-3
(origin/1.9.1) Colin Mollenhour 5 months ago Merge pull request #23 from Flyingmana/Flyingmana-patch-1
Colin Mollenhour 5 months ago Merge pull request #104 from colinmollenhour/improve-layout-cache
Colin Mollenhour 5 months ago Merge pull request #118 from bastienlm/fix-shipping-label
Colin Mollenhour 5 months ago Merge pull request #120 from drobinson/remove-extra-packages
Daniel Fahlke 5 months ago Merge pull request #123 from seansan/patch-16
Colin Mollenhour 5 months ago Revert return value to false when file already exists and overwrite is not forced.
Colin Mollenhour 5 months ago Remove pointless use of fgets.
Colin Mollenhour 5 months ago Use different locking techniques for overwrite flag in file storage resource.
Colin Mollenhour 5 months ago Improvements to file storage.
seansan 5 months ago Fixes Magento bug 648
Tobias Schifftner 5 months ago flex and uploader swf files were removed in patch SUPEE-8788 and therefore also in Magento 1.9.3.0
David Robinson 5 months ago Removed old package xml files
David Robinson 5 months ago Fix cms breadcrumb issue
Lamamy Bastien 5 months ago Fix shipping street and vat label
David Robinson 5 months ago Import Magento Release 1.9.3.0
David Robinson 5 months ago Import Magento Release 1.9.3
David Robinson 5 months ago Fix configurable product attribute sorting
seansan 5 months ago Spaces
Colin Mollenhour 5 months ago Merge pull request #111 from colinmollenhour/fix-pr103-regression
Alex Kirsch 5 months ago Use older array syntax
Colin Mollenhour 5 months ago Fix regression from PR #103. Forgot to use array access by reference...
tmotyl commented 7 years ago

I've just merged a pull request in a slightly different way then before (https://github.com/OpenMage/magento-lts/commit/204cb68c788fbb0f9ccb937c720ae807e047526d) I've squashed commits form the pull request branch and improved the commit message. This way we have just 1 commit with this change instead of 2 commits from the change and 1 merge commit = 3 commits.

How do you like it?

colinmollenhour commented 7 years ago

So if we are going to use this method to maintain a changelog going forward should we just always squash PRs when merging and clean up the commit message to be descriptive but succinct at the time of merge? I've not tried this but it seems it could be easy and still yield good results.

colinmollenhour commented 7 years ago

I just tested the "Squash and Merge" method on #242 and here is what the result looks like on the commit history with yesterday's merge for a comparison. It automatically added the PR # to the commit message which wasn't there before and you can modify the message and the longer description. Seems pretty solid to me but this is just one single-commit PR so I don't know what other issues there could be. It does seem to maintain the original committer "credit" though which is definitely important.

LeeSaferite commented 7 years ago

Ignoring all other discussion in this thread, I'm 100% against squashed merges. I think if we or the contributor prefer less commits in the log, then that should be done prior to issuing the PR.

infabo commented 7 years ago

maybe there should be contributing guidelines. only one commit by squashing before the PR. the commit should describe what has changed/improved/fixed and why. a signature line - who is the author of the commit (name and email) and maybe a line with tested magento version (for later reference) and maybe a list of people having approved the commit.

sreichel commented 7 years ago

Dislike for squashed commits ... (cant see that selected branch is already merged)

lts-commits

How about ...

Should be enough to keep a CHANGELOG.md manually up2date.

colinmollenhour commented 7 years ago

I'm not seeing that requiring squashing before PR is any better than "Squash and Merge".. If the submitter provides a single clean commit with a good commit message then there is no reason to use Squash and Merge, but for multi-commit PRs or PRs with not-great commit messages (and then there is the chicken-and-egg scenario making it hard to add the PR # to the commit message) the end result is the same with less hassle. During development I think it is useful to see multiple commits as requested changes are incrementally committed because it makes approval process more clear. Requiring that submitters squash and force-push between each update is just going to result in a lot of "Looks good but please squash your commits" messages and unnecessary delays when there is a "Squash and Merge" button which does almost the same thing.

If the goals are:

  1. Maintain contributor credit in commit log
  2. Have clean commit log messages for easy generation of changelog
  3. Minimize friction for both contributors and maintainers

then from what I've seen "Squash and Merge" is a useful tool which meets all of the requirements in cases where the contributor provides a poor commit messages or multiple-commit history.

@sreichel That is only an issue for the submitter.. However, is there not a button on the PR page for you that says something like "Delete branch"?

@LeeSaferite Can you explain why you're 100% against "Squash and Merge"?

All of that said, I'm fine with adopting whatever strategy as long as it isn't like Magento 2's method which is apparently to rewrite contributors commits fresh with no mention of the original contribution... (does this look familiar?)

Flyingmana commented 7 years ago

Alternative suggestion. generate the changelog only from the merge commits. You can add all PR and Issue IDs to the commit message of the merge, all merges start with the same content. (so filtering should be easy?) and its a lot less questioned then squashing

sreichel commented 7 years ago

@colinmollenhour of course I have a "Delete Branch" button, but ... just have to delete local branches too in time, to keep overview whats already merged.

@Flyingmana "Alternative solutions" .... https://gist.github.com/sreichel/215a05b524ff2201e896e3273922d6bf?

Took some initial time, but maintaining it should be manageable. (???)

inluxc commented 7 years ago

I agree with @sreichel.

Like this, we have all information needed to do a Changelog.

edannenberg commented 6 years ago

Agreeing with a lot of the points made here. :+1: for sqashing all PRs, makes cherry-picking commits much easier when backporting, etc. I think given this repo's context/usage the commit message's header should, if possible, describe the problem instead of what changed. Makes the history much easier to digest IMO, specially for potentially new users.

We are currently considering this project for one of our sites, sadly after reviewing the current git history of the 1.9.3.x branch I didn't feel very confident, to say the least. I realize the maintainers are unpaid and free time is precious, but git discipline should be sorted out as soon as possible, the longer this ticket slumbers the bigger the mess is gonna get. Personally I'm not a big fan of the current branching model, having huge import/merge commits between version bumps is error prone, but then again there probably won't be many upstream Magento 1.x releases in the future.

To verify everything is in order I did some house cleaning:

After that I did a directory compare for the 1.9.3.x and the fresh 1.9.3.6 branch. Looks pretty good except for a few inconsistencies in the 1.9.3.x branch:

Not sure if I missed some commits for the last two bullet points, maybe someone else can shed some light on this.

Pushed the cleaned up branch to my fork, sadly the tagging format [bug#$PRNR] didn't work out as expected so another rebase to fix that is needed.

colinmollenhour commented 6 years ago

Thanks for the input and the code review, @edannenberg. Your exposure of some inconsistencies is definitely an eye-opener but as you said I think there won't be many more 1.x releases in the future but we obviously need to take more time reviewing these PRs.

Do you have any tips on applying the upstream patches? I'm not sure what method is being used currently but it probably gets more difficult the further we diverge. Perhaps it should be divided into multiple PRs? For example one for /app/code, one for /app/design and one for everything else (this could vary based on where most of the changes were)? These three PRs could be against a new temporary branch like 1.9.3.x-merge-1.9.3.7 and then once all of those PRs were reviewed and merged the temp branch could be merged. Basic divide and conquer.

I agree that for this project squash and merge makes sense even though it goes against the concept most of us have that git commits should be preserved like ancient relics. If we could come to a consensus on this I propose that we just start using squash and merge going forward and leave the existing history intact for people that are already using branches even though a clean rebase like your 1.9.3.6 branch presents a much cleaner history.

Regarding commit messages if we are using Squash and Merge then it can simply be the responsibility of the person who does the merge to clean up the commit message to meet whatever guidelines we decide upon. I think it is not realistic to expect every contributor to write perfect commit messages and it is more hassle to ask them to correct them than it is to change them during the Squash and Merge operation.

So, is anyone still opposed to using squash and merge? Please state your case.

@LeeSaferite has that 100% opposition dropped any since your last input? :)

LeeSaferite commented 6 years ago

@colinmollenhour I still don't like squashes, but I'm just one voice in the crowd. You and @drobinson and @Flyingmana have an equal say in the matter. I've been happy that you guys are stepping up as I've been unable to work on the project much lately due to a job change. I'll leave the ultimate decision on what path we take to a consensus vote among you three.

edannenberg commented 6 years ago

@colinmollenhour said: Do you have any tips on applying the upstream patches?

This repo is a bit of a special case as the goal is essentially to provide a set of patches on top of a vanilla Magento, so personally I would start with an empty branch for every Magento version and rebase --onto the patches on top of the fresh vanilla import, fixing any patch that breaks one at a time. With a clean and mostly atomic git history this should be much easier to handle and would also preserve atomicity for every bug/improvement in the future. Every upstream merge commit on top of an old Magento version increases noise and (potentially) decreases atomicity, avoiding that is key for a project like this in terms of maintainability.

This approach assumes that the Magento core is just a runtime dep and probably won't fly well for people that use this repo as upstream for their actual project repos. Coming from the Java world this always felt horribly wrong to me. Alternatively I would recommend to at least do a full directory compare as I outlined above before merging any big upstream PR. This again requires a clean git history to be feasable.

Perhaps it should be divided into multiple PRs? For example one for /app/code, one for /app/design and one for everything else (this could vary based on where most of the changes were)? These three PRs could be against a new temporary branch like 1.9.3.x-merge-1.9.3.7 and then once all of those PRs were reviewed and merged the temp branch could be merged. Basic divide and conquer.

I'm not sure breaking down the PR solves the underlying issue, it may help a bit in terms of reviewing but in the end it's still fairly large commits where you have to take extra care to not break any existing patches.

I agree that for this project squash and merge makes sense even though it goes against the concept most of us have that git commits should be preserved like ancient relics.

I can't remember a single time where not squashing a PR's commits before merging brought me any value, scattered feature commits on the other hand.. For example when a PR is reviewed and changes are committed weeks later (which is fine), sprinkle in some other PR merged at the same time and its already starting to get messy for other people to see what is what.

If we could come to a consensus on this I propose that we just start using squash and merge going forward and leave the existing history intact for people that are already using branches even though a clean rebase like your 1.9.3.6 branch presents a much cleaner history.

I would favor a clean cut: new branch, cleaned up history according to new commit guidelines. Leaving the old messy history may safe some users a bit of an annoyance, but on the other hand it makes getting an overview that much harder then it should be for any new user in the future. As I have just been there, it wasn't much fun to unravel the current history. Most of the work required for a clean history is already done in my branch, just needs a bit more work on the commit messages.

Regarding commit messages if we are using Squash and Merge then it can simply be the responsibility of the person who does the merge to clean up the commit message to meet whatever guidelines we decide upon. I think it is not realistic to expect every contributor to write perfect commit messages and it is more hassle to ask them to correct them than it is to change them during the Squash and Merge operation.

I fully agree.

Flyingmana commented 6 years ago

Iam also against squashing in general (it may be usefull in some cases, and I just did it today at my work project). Expecting squashing in general puts the contributor into the place to defend, why it may be usefull to not squash it in this case. For example because the commits are logically grouped in sub changes. There is also no extra need for the usecase of @edannenberg as github provides all pullrequests also as single patch files like https://patch-diff.githubusercontent.com/raw/OpenMage/magento-lts/pull/403.patch

Also I will make one thing clear: keeping the needed work for this project for both, Maintaining, and also Contributing as low as possible has the absolute highest priority. Because: Money My current direct income from all my open source work is 1$/month https://www.patreon.com/Flyingmana Sure, the reputation is useful, but not what brings one trough a technical interview.

If someone really wants to set the requirements for this to a higher level,(because this is a requirement for their company?) then there are three options.

If this can not be fullfilled, then I suggest to put the changelog onto the github pages just beeing autogenerated out of the git log and later additional secondary sources to complement insufficient data from the git log. Key point of this approach is: keep this out of the contribution process, as it will block possible contributions.

edannenberg commented 6 years ago

Expecting squashing in general puts the contributor into the place to defend, why it may be usefull to not squash it in this case. For example because the commits are logically grouped in sub changes.

There are always exceptions to a rule, if it makes sense not to squash then don't. So to be more precise, PRs should not always be squashed, but useless "fixed typo", etc commits should be squashed and whatever commits get merged should be atomic in scope. That burden is on the maintainer, a good habit for any maintainer to learn though.

There is also no extra need for the usecase of @edannenberg as github provides all pullrequests also as single patch files like https://patch-diff.githubusercontent.com/raw/OpenMage/magento-lts/pull/403.patch

The point of atomic commits is to give users of this repo a decent base to work with. Imagine you just stumbled about this repo, it sounds like a really good idea. But what exactly are you buying into here? This is currently very hard to gauge, in large part due to the git history being a pretty big mess. This project is not a random Magento extension you can review quickly and move on. The large upstream merges in the past make the review process pretty much insane. So what are your options? You either say nope and continue to use vanilla Magento with your own patch set or painfully unravel the whole mess to gain the needed confidence using magento-lts on your production servers.

Also I will make one thing clear: keeping the needed work for this project for both, Maintaining, and also Contributing as low as possible has the absolute highest priority. Because: Money

I'd argue that the proposed changes have minimal time impact for the maintainers, well maybe except for finding a good commit message. :) It does however require some discipline until it becomes second nature.

In my experience a clean atomic git history is an asset for any repo user, while a messy one makes life just harder for no reason other having saved the maintainer a minute here and there in the past.

If someone really wants to set the requirements for this to a higher level,(because this is a requirement for their company?) then there are three options...

I'm using the squash-into-atomic process for a while now for the open source projects I maintain, the time investment is minimal and more then justified for what you get in return: a repo that is a pleasure to work with. If you had to choose, what you would rather work with:

https://github.com/OpenMage/magento-lts/commits/1.9.3.x https://github.com/edannenberg/magento-lts/commits/1.9.3.7

It would be trivial to get a fully automated and accurate changelog from the clean history in my fork:

git log --pretty=format:'%<(20) %ar %H %s' | grep '\[bug-'
git log --pretty=format:'%<(20) %ar %H %s' | grep '\[impr-'

Anyways, just my 2c. I'm fine with cherry picking a few commits now and then into the cleaned up fork.

Flyingmana commented 6 years ago

Reviewing a commits history via github is not really the recommended way.

I prefer an optimized tooling for this which brings me in the first draft to this output

https://gist.github.com/Flyingmana/7074e8c3d89b8ccab1c7371ab4c0d9e9

I thought about throwing out the merges as you see, but they contain the PR number and are therefore usefull, so I added another version with them kept in. The important part is the --topo-order which solves the big problem you have with commits from different branches appearing intermixed.

Yes, yours looks better, but if I had the choice, I would take the one with the non-squashed commits. To often I had do debug into problems, which were already in the code for months, and the original author either not reachable, no time, or just did not know anymore what in detail happens there. I was in such situations always thankfully for each little crumb of information which helps me to understand the author at the time of writing. Be it a commented out line of code, or some smaller refactoring the person did in the end.

So if I exaggerate this, I could say squashing commits is actually harmful for the longterm maintenance of a project

tmotyl commented 6 years ago

Just few notes from my experience as a core team member of TYPO3 - a top CMS in DACH market, which has 150-200 commits per month and 30-50 contributors per month (many of who are first time contributors). We also maintain 3 versions(branches) in parallel. In my opinion it's hard to find a project with cleaner and helpful git history then TYPO3.

How TYPO3 does it

Take a look here: https://github.com/TYPO3/TYPO3.CMS/commits/master and open some change.

It's is super easy for everybody to understand what given commit is doing

We squash every branch so it finally lands as a single change in the product branch. We keep changes small, often it's easy to split changes which grew over time into few pieces from which every one makes sense alone.

If you want to see the evolution of the patch, you can always find it in the pull request history. But to be honest it's you need it very rarely.

There are also some other rules, like - every breaking change, deprecation or a new feature requires a simple documentation file like this https://github.com/TYPO3/TYPO3.CMS/commit/fe563ffb2206b8d50d8e208c48401a6fde44f148

A core developer who is merging the change is responsible of checking if the commit message is correct and describe the change in a correct way. It is neither hard nor time consuming.

Goals

The changelog/git history and the readme should:

Proposal for MageLTS

This project is of course smaller and has less contributors than TYPO3, but I think we can adapt some of the concept with big benefit for us.

0) keep changes small

1) squash commits when merging

2) Start using "tags" in the commit message title like

3) Add PR and issue number/link in the commit body, so it's easy to track the full story

4) Describe in the commit body what has changed where and why (I'm not talking about writing a book, but 2-3 sentences are enough)

5) when new Magento version comes up, apply all the fixes onto it again, as @edannenberg suggested, would be nice if we track somewhere if some fix was fixed upstream in vanilla Magento

6) Provide a changelog (draft is autogenerated from git, for older commits it would require additional human touch, but new commits should be fine).

colinmollenhour commented 6 years ago
  1. when new Magento version comes up, apply all the fixes onto it again, as @edannenberg suggested, would be nice if we track somewhere if some fix was fixed upstream in vanilla Magento

This is the only point I'm not sure if I agree on (applying all fixes over a new branch). Users should be able to maintain their own branches and pull in upstream changes easily and I'm not sure how this works with the suggestion made. Also, there will be conflicts with upstream and I think the task of resolving these conflicts each time a new upstream version is released will be a lot of work and prone to errors each time they are resolved. That is, once a conflict has been resolved I don't see why we'd want to have to resolve it ever again.

If there is ever another "major" upstream release (e.g. 1.10.0.0 - I don't expect there to be) then at this time we can decide if we should start a new 1.10.0.x branch with all patches rebased on it, but I think for now we should focus on maintaining the 1.9 branch in the simplest way possible.

@Flyingmana Since we are using Github the full history is available in the Pull Request and doesn't need to be in the main branch history. Case in point, this PR resulted in 2 lines changed and 7 commits and although I squashed it when I merged it, you can still go back to the PR that is referenced in the commit message and see the full discussion and 7 commit history in all it's sloppy glory: https://github.com/OpenMage/magento-lts/pull/388/commits Keeping full history has pros and cons, but the way I see it with Github + Squash and Merge you can get the best of both worlds.

edannenberg commented 6 years ago

This is the only point I'm not sure if I agree on (applying all fixes over a new branch). Users should be able to maintain their own branches and pull in upstream changes easily and I'm not sure how this works with the suggestion made.

While it would be the cleanest and safest way to handle the upstream process you certainly have a point. I think what @tmotyl meant was to use this as verification before a larger upstream commit is merged into 1.9.3.x. Especially if there were any conflicts.

Also, there will be conflicts with upstream and I think the task of resolving these conflicts each time a new upstream version is released will be a lot of work and prone to errors each time they are resolved. That is, once a conflict has been resolved I don't see why we'd want to have to resolve it ever again.

Yep, as I said previously, for this to be feasible a clean git history is pretty much required. The heavy lifting for this has been done in my fork and I will be able to maintain it as part my of work, so this could be used for such occasions. Maintaining the clean fork/branch could also be automated with webhooks and a bit of shell scripting if patches/upstream commits are tagged accordingly on 1.9.3.x, just cherry pick and push.

For a new Magento release the process boils then down to (using 1.9.3.7 as example):

git checkout --orphan 1.9.3.7
git rm -r --cache .
rm -rf * .htaccess* .travis.yml
tar xjvf ~/downloads/magento-1.9.3.7-2017-11-27-05-34-19.tar.bz2 --strip 1
git add * .htaccess*
git commit -m "Import upstream Magento 1.9.3.7"
# gobble up all the commits from prev. version starting from 2nd commit
git cherry-pick 8b0a7169ec23d1a7016d76d78b7fc41162abaa58^..1.9.3.6

Then just run meld, diff or whatever to see if 1.9.3.7 and 1.9.3.x are the same.The upshot with this approach is that you are only merging in one direction, a set of patches on top of a new Magento version. If a patch fails you know exactly which one and can resolve it on the spot, keeping the commit atomic.

Doing this process before applying the upstream changes to the 1.9.3.x branch should also give a heads up if extra care is needed not to break any patches while doing the merge. If the patch set can be applied without conflicts the merge on 1.9.3.x can be treated as one way:

colinmollenhour commented 6 years ago

If the patch set can be applied without conflicts the merge on 1.9.3.x can be treated as one way:

Are you saying that at the end of creating the new 1.9.3.7 branch you would merge this new branch into 1.9.3.x? There are many ways to do this so please give a specific example of what you recommend. E.g. is it something like:

git merge 1.9.3.7 --no-commit --no-ff -X theirs
git reset 1.9.3.x
git add -A .
git commit -m "Apply diff from 1.9.3.7 with patches."

I've not tried the above but I think it would take the result from your ground-up method and apply it as a new commit to the existing branch.

tmotyl commented 6 years ago

I'm not dogmatic on the point 5), probably we need to battle test it to see whether our assumptions are correct. I expect that there will be no conflicts when cherry-pickking changes from one branch to another (if only we maintain the same order and Vanilla magento has not changed the same area of the same file. In that case (when Magento changed the same area, the conflict is an obvious thing and we need to resolve it (sometimes it means Magento fixed the same bug we did).

Having a clean git history also allows us to spot bugfixes which were also fixed in Vanilla Magento but in different way (differnt place) - in that case we would probably need to remove (skip) our patch.

Flyingmana commented 6 years ago

After taking some more time to think about it, and also realizing this Topic is now already going on for a year, I suggest we should find a way to start having a changelog and improve it incrementally then.

Also I will change my position on the Topic of stashing. If we notice in another year, that it is not working out we have at least proper reasons to refer to.

And overall it seems here are enough people with more experience then I have on this topic, therefore I should step back a bit and not hinder you. But maybe it would be good to identify, if we have only one Issue here, or if this are multiple ones we could solve/discuss separately.

Also I have to apologize, my last comments were to harsh and could be seen as harmful, Iam sorry for this.

sreichel commented 6 years ago

this Topic is now already going on for a year, I suggest we should find a way to start having a changelog and improve it incrementally then.

Yep. Anything i till better the nothing :P

edannenberg commented 6 years ago

@colinmollenhour said: Are you saying that at the end of creating the new 1.9.3.7 branch you would merge this new branch into 1.9.3.x?

Sorry for being unclear, no I wouldn't merge the diff directly into 1.9.3.x, unless there were no conflicts, as there is still a possibility for human fail. In case of conflicts my recommendation would be to do the upstream merge twice independently, ideally by two different persons:

As of 1.9.3.7 Magento offers diffs for the previous version on their download page, so the process for a new Magento release should look something like:

@tmotyl said: I expect that there will be no conflicts when cherry-pickking changes from one branch to another (if only we maintain the same order and Vanilla magento has not changed the same area of the same file.)

In terms of automating the "patch set" branch this would be guaranteed to work as both branches should be at all times identical. The webhook just cherry-picks everything but upstream merge commits from 1.9.3.x.

If a new Magento version is released a new branch is started for the patch set. I'm not sure this part can be fully automated, but worst case shouldn't be more than executing a shell script ./foo.sh magento-1.9.3.8.tar.bz2 plus any potential conflict resolving.

Regarding the change log, if a clean branch is maintained it could be used to generate the change log from scratch, the webhook maintaining the branch could also take care of pushing the updated change log to 1.9.3.x.

@Flyingmana said: I suggest we should find a way to start having a changelog and improve it incrementally then

So let's start with defining how commit message formats should look like and get this thing rolling, proposal:

[${commit_type}-#${pr_id}] ${commit_intent_aka_msg}

${commit_details_or_lets_get_technical}

commit_types, something like @tmotyl suggested, but as short as possible to leave room for the actual commit message, some suggestions just to get the ball rolling:

Project related commits like updating the README.md or taming Travis should have no commit_type.

commit_intent should be written for use in the actual change log, so it's better to describe the error/intention then the technical detail IMO.

sreichel commented 6 years ago

Just a note to commit message format ...

[${commit_type}-#${pr_id}] makes changelog definitly more readable, but i think it breaks Githubs feature to close issues with commit mesages. _(shoulndt it be issue_id instead of pr_id - if there is one?)_

see: https://help.github.com/articles/closing-issues-using-keywords/

How about ...

[${commit_type}] ${commit_intent_aka_msg}, fixes|closes|ref|see|... #${issue_id}
edannenberg commented 6 years ago

but i think it breaks Githubs feature to close issues with commit mesages

Ah, good point, didn't think of that. It's not really relevant for the change log though and more importantly steals precious space from the commit message. How about adding that kind of stuff below the commit details instead?

shoulndt it be issue_id instead of pr_id - if there is one?

My reasoning here was that there is most likely always a PR, but not always an issue for each PR. It may also happen that a PR has no reference to it's issue. Fine with either though.

For bugs it would be nice to have the first Magento version that introduced it in the message header, should be optional though:

[${commit_type}-#${pr_id}][${since_version}] ${commit_intent_aka_msg}

${commit_details_or_lets_get_technical}
---
closes #${pr_id} #${issue_id}, refs #${some_other_issue_id}, etc...
fballiano commented 4 years ago

Changelog was already present in the https://github.com/OpenMage/magento-lts/blob/1.9.4.x/RELEASE_NOTES.txt file, Magento later replaced the way the changelog was shows (just link an online version of the release notes) but I guest that could be a nice place to store it.

fballiano commented 3 years ago

This issue is 4.5 years old... it's time to decide something or close it

Flyingmana commented 3 years ago

it kind of is already decided to have a changelog. We do have a changelog, at least for new releases here: https://github.com/OpenMage/magento-lts/tree/20.0/.github/changelog I have some tooling which is mostly automating the changelog creation for new releases here: https://github.com/Cotya/maintainer-tooling

But we still need something to provide a changelog for the what happened before we started writing one for new releases

nimasan commented 2 years ago

Hi everyone!

In my company, I asked to use some conventions like

Moreover we use the Python CLI named Changelogdir to auto-generate our changelog by CI. This is kind of easy, contributors will just have to add a file describing what they've done.

colinmollenhour commented 2 years ago

I like the Conventional Commits proposal, very easy to adopt, although not as easy to enforce.

The changelog options both have downsides.. single file has conflicts and multi-file means moving files around to the right version (e.g. backporting is more work). Just doesn't seem like a great solution for a Github-based project.

However, by adopting Conventional Commits it would be easier to script a changelog generator (surely one already exists)