vlsi commented on PR #124: URL: https://github.com/apache/xalan-java/pull/124#issuecomment-1815846729
> Moreover, it makes life harder for developers who manage multiple PRs depending on each other @kriegaex , There are two issues here: 1) Certain projects (Xalan included) lack review power, commit power, and code quality at the same time. It forces contributors to run into "multiple depending PRs" or "wait N+1 years in-between the PRs so they are merged". ^^ I am not sure it is something that can be fixed in Xalan. There are virtually no active committers, committers rarely review others code, and the code quality is wonderful. 2) Sure it was a bad idea to squash https://github.com/apache/xalan-java/pull/122 into a single commit. +100500 to what you say. > In any case: Normally I'd only squish upon merge into the main stream. Which should normally be after regression testing has been passed, unlike the individual commits @jkesselm , please re-read what @kriegaex says. There are several very valid reasons to keep commits like in https://github.com/apache/xalan-java/pull/122 separate, and I 100% agree with @kriegaex that structuring PR122 as five independent commits, and merging it as 5 independent commits was the best idea possible. Take this as an example >kriegaex: If I can use git bisect on many small commits, I can more easily spot the commit that broke something than by finding one big commit Imagine I upgrade Xalan in my enterprise app, and imagine it produces a weird NullPointerException after the upgrade. I might try isolating the problematic change in Xalan that introduced the failure, and I could use [git bisect](https://git-scm.com/docs/git-bisect) to find the first Xalan commit that causes the NPE. Since PR122 was merged in a single commit, the best I would get is "ok, commit 4af37e7a84a81f59af991a5a887e2e88e540d7b2 modified multiple unrelated files which introduced the NPE". It would be harder for me to understand the intention behind the commit, and it would be hard to understand the reasons for NPE. If the commits were individual, then `git bisect` would properly tell me which one is problematic. Of course, backing out a single commit does not guarantee the system would work, however, having five commits would simplify the analysis and review a lot. > I can understand the ask, though I think the need was overstated, especially as the arguably easier review enabled by that separation doesn't actually seem to have produced much additional review feedback. I think you refer to https://github.com/apache/xalan-java/pull/105#issuecomment-1767849230, however, it seems you thought the key reason was to simplify Maven PR review. Let me clarify: I never cared reviewing Maven scripts as those are a waste of time. Of course, having commits separate would help review for those who care to review, however, it was not the sole motivation. Later, in [my comment I highlighted **other** reasons](https://github.com/apache/xalan-java/pull/105#issuecomment-1771368046) to split "file move" into a separate commit: 1) Just like with "git bisect" sample above, separating "move files" to its commit would help analysis of Xalan misbehavior. Imagine I upgrade Xalan, I get ArrayIndexOutOfBoundsException, and then I open Git sources to see why the particular line was changed. It would be way easier to ignore "move files" commits when doing such an analysis rather than trying to decipher what was changed 2) Many tools perform badly when dealing with big diffs. If you commit "moving of 1000 files and making some changes" into a single commit, then it would be hard to view such diff in GitHub UI (e.g. when analyzing a bug later), in IDEs, and in other tools. It is just like "The formerly logical separation of concerns in small commits" @kriegaex mentions 3) Technical commits can be hidden by default in GitHub UI and IDEs with `git-blame-ignore-revs` feature, so it is yet another reason for splitting "move many files" from other changes. Let me remind you that the effort we were talking about was [15 minutes](https://github.com/apache/xalan-java/pull/105#issuecomment-1774104757), so "overstated" does not sound right, especially since the benefits of separating "file move" extend far beyond "merging the PR". The benefits are for everybody analyzing Xalan failures later in their systems, and for those who would maintain Xalan. Having "move files" as a separate commit easily saves 15+ minutes for everybody looking into that commit (e.g. reviewing the PR or analyzing the system failure when upgrading Xalan). > The more serious continuous integration system we had running for Websphere Liberty still worked in terms of PRs rather than commits, since that was the level at which automatically backing out interacting or otherwise suspicious changes was practical. Backing out individual commit points would have left the system between stable versions, since non-squashed commits are not promised to be complete enough or isolated enough to accept individually. That is not relevant. If you were to say "all logical contributions must be filed as separate PRs", then just say so. I can't understand how the mention of Websphere Liberty relates to PR122. >doesn't actually seem to have produced much additional review feedback. Well, fixing the build system for Xalan is a couple of days activity. I would be surprised if one could produce "much review feedback" out of that. There's just nothing to review. I would like to remind you that as you separated file movements, I noticed issues with `xalan2taglet.jar`, `package.html`, and `xalan2jdoc.jar`: https://github.com/apache/xalan-java/pull/105#issuecomment-1774580602 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@xalan.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@xalan.apache.org For additional commands, e-mail: dev-h...@xalan.apache.org
