Skip to content
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.

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

Update branch name for harbour-storeman. JB#60207 #18

Merged
merged 1 commit into from Feb 17, 2023
Merged

Conversation

martyone
Copy link
Member

No description provided.

Copy link
Member

@vigejolla vigejolla left a comment

Choose a reason for hiding this comment

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

LGTM

@martyone martyone merged commit bb2643b into master Feb 17, 2023
@martyone martyone deleted the jb60207 branch February 17, 2023 12:45
@Olf0
Copy link

Olf0 commented Feb 23, 2023

I just read about this change at Sailfish Community News, 23rd February, CLAT#SDK and developer tools:

  1. It would have been kind to be addressed here to discuss if this makes sense.
  2. As neither the commit message, the PR title, the PR description, the approval note or the merge commit provides any reasoning: What sense is this supposed to make?

@vigejolla
Copy link
Member

What sense is this supposed to make?

I finally had time to investigate this a bit further. The original test case got broken when the default branch of the harbour-storeman repository was changed from master to devel. As only the default branch is cloned in the submodule, the git clone --branch ... command failed as the requested master branch was nowhere to be found. This PR fixed that issue, but a more long term solution can be found in the follow-up PR, which removes the branch parameter and uses a specific commit instead.

@Olf0
Copy link

Olf0 commented Mar 7, 2023

I finally had time to investigate this a bit further.

Thank you very much for investigating the motivation for this change.

The original test case got broken when the default branch of the harbour-storeman repository was changed from master to devel.

Now I see, what the original issue was, and that I caused it unknowingly. Sorry about that, even though IMO this should not have broken a properly defined workflow.

Background: Petr T. ("osetr" / "mentaljam") always used the workflow across multiple branches, which I documented (I solely renamed the branch dev to devel), but left the default branch at master (I assume he simply did not care), despite all commits and PRs had and have to go into the dev[el] branch, first. This was wrong and annoyed me quickly, because I accidentally committed to the default branch (master at that time) and others posed their PRs against it, breaking the specified commit workflow. The only right thing to do was to set the default branch to devel, because that is where all changes shall land first.

As only the default branch is cloned in the submodule, the git clone --branch ... command failed as the requested master branch was nowhere to be found.

Well, in my current understanding the real culprit is the "As only the default branch is cloned in the submodule" part. Because this made it appear as if …

This PR fixed that issue

…, which only made the checkout work again, but now with the devel branch, which is wrong under behavioural aspects.

but a more long term solution can be found in the follow-up PR, which removes the branch parameter and uses a specific commit instead.

As indicated before, I am convinced that this is worse than the original state (checking out the master branch), which was working fine for years. Now that I understand what happened technically and what your requirements are, I can even propose an improvement over the original, well working state. Will describe and provide reasons at PR #19.

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

Successfully merging this pull request may close these issues.

None yet

3 participants