search

BlueSCSI / BlueSCSI-v2

Open source, open hardware, SCSI emulator using the Pi Pico PR2040
https://bluescsi.com
GNU General Public License v3.0
227 stars 23 forks source link

Attribution in merge commits #9

Closed PetteriAimonen closed 1 year ago

PetteriAimonen commented 1 year ago

Congrats on your release - I think it is good to have a hobbyist-buildable design available. The support for Pico-W also opens up interesting possibilities.

You mention "Keeping the lineage of a project is important - who wrote what code, what changed when and where. We've restored the missing attribution and history for SCSI2SD, as well as kept the entire history of ArdSCSIno and BlueSCSI all in one repository."

I agree that I should probably have added SCSI2SD with full version history in the first place.

However, I think there may have been some mistake with this merge commit: https://github.com/BlueSCSI/BlueSCSI-v2/commit/d789904cd646029639c33cb700778454b19b653a

It seems to mix together a lot of unrelated commits:

It is probably difficult to directly merge changes between the projects due to different file paths. Maybe it would be possible to develop a script using git-filter-branch for the purpose?

At the very least, it would be helpful if the merge commit message could include attribution of where it is merging from and what commits are included.

erichelgeson commented 1 year ago

This indeed was a bad merge which lost history and I didn't notice. I have a flow on my local machine to do this to preserve.

It is probably difficult to directly merge changes between the projects due to different file paths.

Git knows when files move - new files need to be moved and the history is kept.

At the very least, it would be helpful if the merge commit message could include attribution of where it is merging from and what commits are included.

Agree I will amend the commit to include the changes in the description. Thanks for bringing this up. Will close when that is done.

PetteriAimonen commented 1 year ago

Thanks!

I think I'll separate the SdFat_NoArduino as #ifdef's in the https://github.com/rabbitholecomputing/SdFat repo at some point in future. It's only used on GD32 anyway. I haven't been able to get the changes we need through to upstream SdFat (greiman/SdFat#354, greiman/SdFat#406), but it will be easier to maintain these changes in a separate repo instead of a subfolder.

I wonder if it might make sense to have a separate repository for SCSI2SD-as-a-library also? While SCSI2SD is originally a stand-alone firmware, the good separation in its code made it quite easy to use as a library. So far the differences to upstream SCSI2SD-V6 are quite small, but once availability problems on SCSI2SD hardware get resolved, there might be more development that would benefit all three projects.

erichelgeson commented 1 year ago

I haven't been able to get the changes we need through to upstream SdFat ... but it will be easier to maintain these changes in a separate repo instead of a subfolder.

Yes I've been watching those, thank you for pushing those issues forward even though they haven't got into upstream.

I wonder if it might make sense to have a separate repository for SCSI2SD-as-a-library also?

That would be appreciated and likely work better. Though with the _disk.cpp and (possibly) other files that are big rewrites it may be make it more difficult. Also things like hooking in command handling for vendor command sets would be needed (or else applied to a vendor fork)

PetteriAimonen commented 1 year ago

Yeah, the _disk.cpp is a bit problematic currently, especially how it mixes handling of image files and handling of SCSI commands. I haven't figured out anything better though, so that would stay local to each project.

So the SCSI2SD-library repo would be what is currently lib/SCSI2SD folder. Further improvements for library use could then be included, such as adding hooks for customizing the behavior with ifdefs or callback functions.

Main problem with just doing that is how scsiPhy.h and scsi2sd_time.h get included from the platform folder. This may need a few tweaks to create a platform-neutral interface without compromising performance.

I probably won't have time to work on the separation in any near future, as it is not important feature-wise. But it could be an useful path to simplify co-operation in the future.

erichelgeson commented 1 year ago

Dug in a bit and found out why this happened. First we did a git merge z/main, conflicts as expected so resolved them. Then did git commit -m ' Merge z/main' - but, oddly, specifying the commit message actually doesn't resolve the commit - it just commits the code as your commit message, leaving the repo still in MERGING status. I've been using git since 2008/9 and never seen this behavior (though my day to day workflow is rebase)

Regardless apologies again as it was unintended. Upstream merges going forward will be in PR's and we'll ensure all commits show up correctly. I've dumped the missing commit messages and details via git log ... --stat and will amend that "merge" commit.

Thanks for bringing it to our attention.

PetteriAimonen commented 1 year ago

No problem! I think I have done the same in the past, noticing much later that git status reports that it thinks that I'm still in middle of a merge.

erichelgeson commented 1 year ago

Resolved with https://github.com/BlueSCSI/BlueSCSI-v2/commit/45e7f55592db1910045a747b3c53524bf6dc3cb5 - thanks.

erichelgeson commented 1 year ago

@PetteriAimonen - was this resolved to your satisfaction? I can of course go back and cherry pick each commit, it's really not a problem. From your last message I thought we were ok with the course of action.

PetteriAimonen commented 1 year ago

Yes, personally I am satisfied with the solution. Legally this is work-for-hire, so Rabbit Hole Computing owns the copyright to my code. They have some further concerns, but I try not to be directly involved in that.