Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

t7407: simplify funny CR workaround #158

Merged
merged 1 commit into from Apr 10, 2014
Merged

Conversation

kasal
Copy link
Member

@kasal kasal commented Apr 10, 2014

The dos2unix calls can be removed:

  • in two cases, current git-submodule.sh produces CR-free output
  • in one case, it is enough to use test_cmp_text instead of test_cmp
  • in one case, test_i18ncmp calls test_cmp_text for us

The dos2unix calls can be removed:
- in two cases, current git-submodule.sh produces CR-free output
- in one case, it is enough to use test_cmp_text instead of test_cmp
- in one case, test_i18ncmp calls test_cmp_text for us
@dscho
Copy link
Member

dscho commented Apr 10, 2014

Thanks! Could you split this commit into individual commits, committing with --fixup <commit> fixing up the respective commits in the win-tests topic branch?

@dscho
Copy link
Member

dscho commented Apr 10, 2014

Ah, never mind, I found it. Just need to remember to squash it next time round.

dscho added a commit that referenced this pull request Apr 10, 2014
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho merged commit c03375d into msysgit:master Apr 10, 2014
@dscho
Copy link
Member

dscho commented Apr 10, 2014

Thanks!

@kasal
Copy link
Member Author

kasal commented Apr 11, 2014

Actually, there is more things to be cleaned up around the "funny CR issue".
I would really like to do some rebasing of the win-tests topic branch, but where can I find it?

@dscho
Copy link
Member

dscho commented Apr 11, 2014

@kasal win-tests-fixes (I said win-tests before, by mistake) is not referenced as a stand-alone branch, it is rebased in the thicket of branches that we rebase on top of upstream. Just look for a commit saying "Start the merging-rebase to [...]". The latest one at time of writing is 14c3a3a. Then just have a look via gitk: gitk 14c3a3a... Alternatively, you can use git log --graph --oneline --boundary --left-right 14c3a3a.. if you prefer the console or like ASCII art.

You will find that 616c7ba merges that topic branch, and acf70ef is its first commit (branching directly off of the commit starting the merging-rebase).

I considered enhancing the shears.sh script to update refs corresponding to the rebased merge commits, but there is not really a good way to avoid overwriting refs that do not correspond to the rebased merge commits (even if we could do it locally, thanks to update-ref's optional <oldvalue> parameter, the same is not possible with push, so we would always run the danger of overwriting history unintendedly).

@kasal
Copy link
Member Author

kasal commented Apr 11, 2014

"@dscho Never mind, I have found that in the log."
I'm sorry that I haven't pushed the "Comment" button earlier, that would save you some time.

@kasal
Copy link
Member Author

kasal commented Apr 11, 2014

... but your comment shows how to get there with git log (gitk). I was using merge-rebase.sh --dry-run

Actually, the win-tests-fixes branch does not contain only test fixes, it also contains fixes to git.

My plan is like this:

  • take the code changes, together with an attached test, and place it to a new branch some-CR-fixes (it is all related to CRLF issue)
  • drop lot of other "funny CR" workarounds (explanation below [1])
  • take the remains of win-tests-fixes squashing in my PR Fix test suite issues with iso8859-1 parameters  #156

I could then send you PR's for branches some-CR-fixes and win-tests-fixes.

Would you be willing to redo the 1.9.2 rebase so that these changes could be included into msysgit 1.9.2?

If yes, by when do you need the PR's from me?

[1] Explanation:
Commit 4d715ac, accepted upstream, does actually resolve the things we played with recently. So all of the dos2unix calls and your good old 8d0429c are obsoled by this.

@kasal kasal deleted the test-simplify branch April 11, 2014 15:03
@dscho
Copy link
Member

dscho commented Apr 11, 2014

My preferred way would be one of two ways:

  1. make a new pull request, based on the current master, reverting the merge of win-tests-fixes as first commit, then re-add the commits in the order you want, together with other related commits you might want to have.
  2. Run /share/msysGit/shears.sh --merging 14c3a3a and reorder the commits as you want them to be reordered. The result would still fast-forward, but add another "Start the merging rebase to [...]" commit. Even better, a simple git diff 14c3a3a would show the effective code differences introduced (or reverted) by your rebase.

As to the time line: I planned on releasing today ("In keeping with the fine tradition of releasing on a Friday and immediately leaving for the weekend ...").

@kasal
Copy link
Member Author

kasal commented Apr 11, 2014

ok, so first branch is some-CR-fixes, contains some hacks so that git produces less CR and then some tests.

Second branch win-tests-rebase is meant to replace win-tests-fixes, with several commits dropped or modified, as they are (no longer) necessary. This branch also contains everything from PR #156 and PR #158 so please remove these two PR's from the rebased version of v1.9.2.

I have not tested the resulting version, but if I broke something, it should be obvious to get the fix back.... ;-)

@dscho
Copy link
Member

dscho commented Apr 11, 2014

See https://github.com/msysgit/git/compare/win-tests-rebase...

Now for a test run...

dscho added a commit that referenced this pull request Apr 11, 2014
This reverts commit 56e5a97, reversing
changes made to 5769bd5.

We will merge the `win-tests-rebase` branch in a moment which makes this
pull request obsolete.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@kasal
Copy link
Member Author

kasal commented Apr 12, 2014

@dscho Unfortunately, this is not right. You merged some-CR-fixes first, and then reverted it by 9d3f42a when you reverted the original win-tests-fixes. That is because some-CR-fixes is mostly a subset of win-tests-fixes.

It would work, if you did the reverting first, then merged both some-CR-fixes and win-tests-rebase. It could be a good idea to reset master to 56e5a97 and redo this again.

Or, if you are willing to do even more rewriting of published version, you could

  • reset to 648a344
  • cherry-pick 2d09d22
  • revert Merge of win-tests-fixes
  • then merge some-CR-fixes and win-tests-rebase

@kasal
Copy link
Member Author

kasal commented Apr 12, 2014

My original idea was to create a new starting point "Start the merging-rebase to v1.9.2", then redo all the merging rebase, omitting win-tests-fixes plus PR #156 and PR #158, and adding instead the two new branches. But I'm not able to do it, as I do not understand merging-rebase.sh.

But I can see now that the point of the "Start merging rebase" commit is that it takes all content from upstream, yet it is a successor of previous master, so there is no history rewrite.
This would mean that redoing the merging-rebase against v1.9.2 once more might actually be a good idea.

@dscho
Copy link
Member

dscho commented Apr 12, 2014

Well, at the moment, there are three fixup! commits for the branches that you did want to revert. A merging rebase would most likely help me remember to keep the some-CR-fixes and win-tests-rebase branches only.

Unless you insist to have it fixed right away (in which case you can easily perform what I would do later by calling /share/msysGit/shears.sh --merging 14c3a3a. IIRC you'll have to look for the fixup! commits and remove them as well as the respective merge lines.

It is not a big deal if you cannot find the time to do it; As for myself, I spent two weeks of my Git time budget on the release, so I will have to refrain from doing much on Git for Windows for one week.

@kasal
Copy link
Member Author

kasal commented Apr 13, 2014

Thank you very much for all the explanations.
I would like to try the shears script, but I'm afraid I'll get to this after a week as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants