Thread: Avisynth+
View Single Post
Old 19th September 2013, 12:45   #34  |  Link
qyot27
...?
 
qyot27's Avatar
 
Join Date: Nov 2005
Location: Florida
Posts: 1,419
Some basic things related to the git history that I find I do want to comment on now as actual feedback, related to the file structure and general practices with git (and this also applies to generating out patches with 'git format-patch' for submission upstream).

I do feel the new structure is an improvement, but it's the presentation that I think needs to be done a bit better so it can be discussed and a more solid case can be made for why the individual choices made were made.

The renames, moves, adds, and deletes should be split into many smaller and separate commits, and the commit messages should fully (or as fully as possible) explain what is happening in each one, such as where files are getting moved/renamed to, or why binary files are deleted, etc.

Things that should have their own dedicated commits, based on what I've observed about the new structure:
  • Expanding the Examples out of their .zip archive
  • Deleting binary files from the git repository/history
  • The basic rename of the 'src' directory to 'avs_core' (and nothing else); this is probably 90% of the difference in the first place
  • Adding the docs to the history and noting where they are stored
  • Adding the CMake build system (if the CMake files for the newly-split plugins is purely for making these into separate plugins, leave those CMake files out for now; if they actually do have relevance for basic compilation as traditionally done, leave them in)
  • Removing the old build system's files, and explaining why they are being removed
  • Minor file renames - and make sure the renames are listed in the commit message itself.
  • Reorganizing of files to more appropriate locations (such as putting all source files related to converting things in the 'convert' directory); this should be separated into as many distinct commits as there are distinct subtopics for reorganizing (in other words, in the convert example I just gave, that is one commit; if there's another one related to some other destination directory, do it in a second commit, and so on)
  • Making Shibatch/ImageSeq/SoundTouch their own plugins (and each one should have its own commit) - IMO it's okay if any renaming and moving is consolidated into here in one patch, just so long as the moving/renaming is immediately pertinent to the goal of making the plugins distinct, and the rationale is - once again - explained in the commit message
Having these split into distinct commits makes it easier to take it piece by piece, explain the rationale for each change, and get approval or critiques from the upstream devs on what should be done. Explaining things in the commit messages and keeping them small in subject matter also doesn't give off the impression of a dramatic change and it's easier to see the immediate connection between where a file was and where it was moved to.

Another point more immediately related to git itself, is that the formatting of the commit messages should have a single concise summary as its headline, and then a longer explanation further down after a double-space. The lines in the commit message should be wrapped at 80 characters (the headline should be <= 80 characters in length). This is to make it easier for users pulling up the history with git log to view it without it overflowing their Terminal's width buffer (since most default to 80 characters), or for any need to use git rebase -i - if the lines exceed the width buffer, then it has the potential to interfere with git rebase -i in a really bad way.

Also, somewhat of a minor point, but let's say you want to focus on the development of a single issue, like multithreading. The work on this should be in its own branch, separate from master. This keeps the master branch clean so that integration of newer changes from upstream and rebasing the branches against master is easier and keeps the general noise down (IMO, development branches should always be considered volatile and subject to rebasing, even if the git repository is public; but never, ever, ever rebase master).

So another list of general git management points I think would make this easier:
  • Checkout a new branch for the changes you've made. All 54 of them that are currently on the github repo should be in a separate branch, and then reset --hard master's HEAD back to where upstream leaves off
  • Use git rebase -i on the new branch to edit the commit messages to comply with the 80 char limit and separate headline/explanation format, without changing the contents of any of the actual commits.
  • Use git rebase -i again, but this time to split the file structure changes into many smaller, more topical commits.



Also, I think there should be a new thread to discuss the changes ultim is proposing, so that the alpha4 thread doesn't get dragged any more off-topic (especially now that alpha5's been released).
qyot27 is online now