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.
+1
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
refs/review-branches/<branch-name>.
- 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
myfeature-a):
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.
cheers,
Zane.
_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
