On 04/08/14 19:18, Yuriy Taraday wrote:
Hello, git-review users!

I'd like to gather feedback on a feature I want to implement that might
turn out useful for you.

I like using Git for development. It allows me to keep track of current
development process, it remembers everything I ever did with the code
(and more).

_CVS_ allowed you to remember everything you ever did; Git is _much_ more useful.

I also really like using Gerrit for code review. It provides clean
interfaces, forces clean histories (who needs to know that I changed one
line of code in 3am on Monday?) and allows productive collaboration.


What I really hate is having to throw away my (local, precious for me)
history for all change requests because I need to upload a change to Gerrit.

IMO Ben is 100% correct and, to be blunt, the problem here is your workflow.

Don't get me wrong, I sympathise - really, I do. Nobody likes to change their workflow. I *hate* it when I have to change mine. However what you're proposing is to modify the tools to make it easy for other people - perhaps new developers - to use a bad workflow instead of to learn a good one from the beginning, and that would be a colossal mistake. All of the things you want to be made easy are currently hard because doing them makes the world a worse place.

The big advantage of Git is that you no longer have to choose between having no version control while you work or having a history full of broken commits, like you did back in the bad old days of CVS. The fact that local history is editable is incredibly powerful, and you should start making use of it.

A history of small, incremental, *working* changes is the artifact we want to produce. For me, trying to reconstruct that from a set of broken changes, or changes that only worked with now-obsolete versions of master, is highly counterproductive. I work with patches in the form that I intend to submit them (which changes as I work) because that means I don't have to maintain that form in my head, instead Git can do it for me. Of course while I'm working I might need to retain a small amount of history - the most basic level of that just the working copy, but it often extends to some temporary patches that will later be squashed with others or dropped altogether. Don't forget about "git add -p" either - that makes it easy to split changes in a single file into separate commits, which is *much* more effective than using a purely time-based history.

When master changes I rebase my work, because I need to test all of the patches that I will propose in a series against the current master into which they will be submitted. Retaining a change that did once work in a world that no longer exists is rarely interesting to me, but on those occasions where it is I would just create a local branch to hold on to it.

You are struggling because you think of "history" as a linear set of temporal changes. If you accept that history can instead contain an arbitrarily-ordered set of logical changes then your life will be much easier, since that corresponds exactly to what you need to deliver for review. As soon as you stop fighting against Gerrit and learn how to work with it, all of your problems evaporate. Tools that make it easier to fight it will ultimately only add complexity without solving the underlying problems.

(BTW a tool I use to help with this is Stacked Git. It makes things like editing an early patch in a series much easier than rebase -i... I just do `stg refresh -p <patch_name>` and the right patch gets the changes. For dead-end ideas I just do `stg pop` and the patch stays around for reference but isn't part of the history. I usually don't recommend this tool to the community, however, because StGit doesn't run the commit hook that is needed to insert the ChangeId for Gerrit. I'd be happy to send you my patches that fix it if you want to try it out though.)

That's why I want to propose making git-review to support the workflow
that will make me happy. Imagine you could do smth like this:

0. create new local branch;

master: M--....
feature:  *

1. start hacking, doing small local meaningful (to you) commits;

master: M--....
feature:  A-B-...-C

2. since hacking takes tremendous amount of time (you're doing a Cool
Feature (tm), nothing less) you need to update some code from master, so
you're just merging master in to your branch (i.e. using Git as you'd
use it normally);

This is not how I'd use Git normally.

master: M--....-N-O-...
          \    \    \
feature:  A-B-...-C-D-...

3. and now you get the first version that deserves to be seen by
community, so you run 'git review', it asks you for desired commit
message, and <poof, magic-magic> all changes from your branch is
uploaded to Gerrit as _one_ change request;

You just said that this was a "Cool Feature (tm)" taking a "tremendous amount of time". Yet your solution is to squash everything into a single patch.

The cases where feature development is arduous enough to supposedly require this workflow, yet the feature not so big that it needs multiple patches must be exceedingly rare.

Meanwhile, under your proposal the entire subset of developers who have enabled this option will always have all of their commits squashed into one. I don't see how you can credibly claim to be in favour of this and also of not "squashing together commits that should belong to separate change requests", because that is an inevitable result.

master: M--....-N-O-...
          \    \    \----E* <= uploaded
feature:  A-B-...-C-D-...-E

4. you repeat steps 1 and 2 as much as you like;
5. and all consecutive calls to 'git review' will show you last commit
message you used for upload and use it to upload new state of your local
branch to Gerrit, as one change request.

Note that during this process git-review will never run rebase or merge
operations. All such operations are done by user in local branch instead.

Now, to the dirty implementations details.

I'm certain that nothing I have said above will have convinced you, but luckily there is nothing stopping you from just implementing a tiny script that does this for you. So just write it and use it locally instead of git review and you can keep your workflow. But please don't encourage others to adopt it.

- Since suggested feature changes default behavior of git-review, it'll
have to be explicitly turned on in config (review.shadow_branches?
review.local_branches?). It should also be implicitly disabled on master
branch (or whatever is in .gitreview config).
- Last uploaded commit for branch <branch-name> will be kept in
- For every call of 'git review' it will find latest commit in
gerrit/master (or remote and branch from .gitreview), create a new one
that will have that commit as its parent and a tree of current commit
from local branch as its tree.
- While creating new commit, it'll open an editor to fix commit message
for that new commit taking it's initial contents from
refs/review-branches/<branch-name> if it exists.
- Creating this new commit might involve generating a temporary bare
repo (maybe even with shared objects dir) to prevent changes to current
index and HEAD while using bare 'git commit' to do most of the work
instead of loads of plumbing commands.

When you say "prevent changes to current index and HEAD", I hear "I want to automatically submit for review a patch that is different to what I have tested locally".

Note that such approach won't work for uploading multiple change request
without some complex tweaks, but I imagine later we can improve it and
support uploading several interdependent change requests from several
local branches. We can resolve dependencies between them by tracking
latest merges (if branch myfeature-a has been merged to myfeature-b then
change request from myfeature-b will depend on change request from

master:    M--....-N-O-...
             \    \    \---------E*
myfeature-a: A-B-...-C-D-...-E   \
                       \       \   J*<= uploaded
myfeature-b:           F-...-G-I-J

So now every patch in a series would be a separate branch???

This improvement would be implemented later if needed.

I hope such feature seams to be useful not just for me and I'm looking
forward to some comments on it.

-1, sorry.


OpenStack-dev mailing list

Reply via email to