Skip to contentSkip to conversation

Development strategy

DISCUSSION Lea Wiemann 6 participants 10 messages Jun 15, 2016

lore
Share & tools
lore
#0 lore raw

Hi everyone,

[For those uninitiated, this is about my GSoC project about adding caching to Gitweb, on which I am currently working full-time; my branch is here: http://repo.or.cz/w/git/gitweb-caching.git]

I'm seeing a process-wise problem arising with my current work on Git.pm and Gitweb, in that I'm producing patches at a higher frequency than the developer community can handle. Right now, I have a stack of 7 patches that haven't been "approved" in any way (i.e. either they are under discussion or nobody has reviewed them) and that my future work will depend on. Chronologically,

! 2f15087248 perl/Git.pm: add get_hash method
abd799435c gitweb: use Git.pm, and use its get_hash method
! 5e53e2e67e t/test-lib.sh: add test_external
! c5c97f896c Git.pm: fix documentation of hash_object
! 8db2d73809 test suite for Git.pm
261a54ff5b gitweb: removed unused parse_ref method [not posted yet] e9166526a3 Git.pm: add get_type method [not posted yet]

The patches marked "!" I cannot change without breaking at least one subsequent patch, so every time I want to make a change to one of those, I also need to change at least one subsequent commit, and worse, resubmit it to the mailing list. (E.g. the the recent rename from parse_rev to get_hash necessitated two extra patches to accommodate the change.)

If I keep on committing revisions like I'm doing right now, I'll end up with a long queue of interdependent patches of which only the newest few are easily changeable. Here are some ideas to prevent this:

1) My current patch frequency is probably too high, in particular if minor changes (like method renames or documentation changes) cause re-posts of patches. Hence, I suggest that I stay on my branch for now and only post patches on which I actually want feedback, without reposting corrected versions after getting feedback. That is, no patches just for "please someone approve this".

2) I should probably also try to squash patches into larger ones. This will make it easier to make changes in older commits, since I don't have to edit several commits, and it reduces the probability of merge conflicts. I'm not sure how much squashing is appropriate though: For example, would it be okay to squash a fix like "Git.pm: fix documentation of hash_object" into a larger "Git.pm: add and fix several methods" commit, where I only mention the documentation-fix in the log message? It would certainly be helpful in that it reduces the number of conflicts, but it will make the logs coarser.

-- Lea

#1 lore raw
Quoted 8 lines · Lea Wiemann

! 2f15087248 perl/Git.pm: add get_hash method
abd799435c gitweb: use Git.pm, and use its get_hash method
! 5e53e2e67e t/test-lib.sh: add test_external
! c5c97f896c Git.pm: fix documentation of hash_object
! 8db2d73809 test suite for Git.pm
261a54ff5b gitweb: removed unused parse_ref method [not posted yet] e9166526a3 Git.pm: add get_type method [not posted yet]

Quoted 6 lines

The patches marked "!" I cannot change without breaking at least one subsequent patch, so every time I want to make a change to one of those, I also need to change at least one subsequent commit, and worse, resubmit it to the mailing list. (E.g. the the recent rename from parse_rev to get_hash necessitated two extra patches to accommodate the change.)

If the need to cascade changes is about something minor like naming, do not be too picky while you are presenting the progress. I personally think the method for getting the output from rev-parse should be named rev_parse and not get_hash, if only not to confuse plumbing users, but do not scramble and rebase -i many patches only because I said so today.

That will be a waste of your time. Something small like that, I do not think anybody would mind if at the very end of the series there is a patch that modifies all users of get_hash to use rev_parse and rename the method. I certainly wouldn't.

If on the other hand the issue is about a design mistake you had in earlier patches in the series, well, you may have to re-roll the later ones to adjust to the interface change _if_ you want to eradicate the design mistake from the history. But this too depends on the nature of the mistake.

For example, you may introduce a new sub "foo" in commit #1 to support a small feature or two you add in commit #2 and #3. Later, you would want add yet another new thing that can use something very close to what "foo" already does, but realize that you need to call "foo" slightly differently by giving an additional parameter.

You might wish that you had a perfect foresight to have that extra parameter from the beginning. But _nobody has perfect foresight_. I would not want you to start amending from commit #1 in such a case. Just make commit #4 to be "Enhance 'foo' to deal with this new situation", and that commit will change the implementation of "foo" and add the necessary extra parameter to the existing calling site you introduced in commit #2 and #3. Then commit #5 will use that new interface to introduce the new feature you needed an enhanced "foo" and you go on. Such a case of "the helper introduced in earlier round was too limited and it is later found that it needs enhancement" is not even a mistake you would want to remove from the history. It is just a normal course of code evolution.

Keep your commits small, logically independent steps.

Do not _artificially_ squash the commits --- it is _much worse_ than following up with enhancement patches. I think your current patch granularity is quite fine (in good/bad sense not in big/small sense) and makes pleasant read.

#2 lore raw
In reply to Lea Wiemann
Quoted 8 lines · Lea Wiemann

2) I should probably also try to squash patches into larger ones. This will make it easier to make changes in older commits, since I don't have to edit several commits, and it reduces the probability of merge conflicts. I'm not sure how much squashing is appropriate though: For example, would it be okay to squash a fix like "Git.pm: fix documentation of hash_object" into a larger "Git.pm: add and fix several methods" commit, where I only mention the documentation-fix in the log message? It would certainly be helpful in that it reduces the number of conflicts, but it will make the logs coarser.

Why not go for the win and do both? Keep your commit strategy as is, and create a branch whenever you want review, say review-02-06 or such and in that branch you squash the commits as appropriate (perhaps StGit can help here). These patches are mostly not intended for inclusion just yet (but for review only), right? Instead of sending all the small commits you send a bigger one, perhaps with a link to where the branch can be found for easy checkout. You can keep the branch for the a record or delete it after the review and go on with the next branch.

Cheers,

Sverre, who thinks a few RFC patches are in place soon for git-stats too and is glad you are doing the fine-tuning wrt patch submission.

#3 lore raw
Quoted 3 lines · Sverre Rabbelier

Keep your commit strategy as is [with small commits], and create a branch whenever you want review and in that branch you squash the commits as appropriate

Are you suggesting that the squashed patches get merged, or that the squashed patches get reviewed but the finer-granulated patches get merged? In the former case, I'd probably prefer to work with larger patches in the first place (and not just squash them on the review branch), since they are easier to handle -- e.g. I sometimes need to go back and change things in earlier commits, and in those cases larger commits are easier.

-- Lea

#4 lore raw
Quoted 2 lines · Lea Wiemann

Are you suggesting that the squashed patches get merged, or that the squashed patches get reviewed but the finer-granulated patches get merged?

The latter is what I suggested :). I reckon you'll want ot keep the finger-granulated history for future reference / bisecting, but when reviewing that seems overkill.

Quoted 4 lines

In the former case, I'd probably prefer to work with larger patches in the
first place (and not just squash them on the review branch), since they are easier to handle -- e.g. I sometimes need to go back and change things in earlier commits, and in those cases larger commits are easier.

If you don't think you're omitting information you will want later on (meaning the more specific commit history), by all means, go with the bigger patches. I find it nicer to create small commits, with the plus that it cuts down the size of the commit msg too ;).

Signature · 3 lines
Cheers,

Sverre Rabbelier
#5 lore raw
In reply to Sverre Rabbelier
Quoted 4 lines · Sverre Rabbelier

Cheers,

Sverre, who thinks a few RFC patches are in place soon for git-stats too and is glad you are doing the fine-tuning wrt patch submission.

By the way, do you have public git repository set up somewhere (for example at repo.or.cz)?

Shawn, what is the status of other git GSoC 2008 projects?

Signature · 3 lines
Jakub Narebski
Poland
ShadeHawk on #git
#6 lore raw
Quoted 1 line

Shawn, what is the status of other git GSoC 2008 projects?

I don't think he is informed about every project. ;-)

Well, I -- the one who does git-sequencer -- am currently on making a first prototype (according to a first spec) work/clean ;)

I have a repo at repo.or.cz, but have not yet pushed to it because my compromised Debian SSH key has still access to repo.or.cz and my new one hasn't.

Regards,
Stephan

Signature · 1 line
Stephan Beyer <[email protected]>, PGP 0x6EDDD207FCC5040F
#7 lore raw

Hi,

Quoted 3 lines · Stephan Beyer

I have a repo at repo.or.cz, but have not yet pushed to it because my compromised Debian SSH key has still access to repo.or.cz and my new one hasn't.

Why don't you just remove your old account from that repository, add a new account with the new keys, and use that, until Pasky finally integrated the changes to repo.git to change your keys?

Ciao,
Dscho

#8 lore raw
In reply to Jakub Narebski

Hi,

Quoted 9 lines · Jakub Narebski
Quoted 2 lines · Sverre Rabbelier

Sverre, who thinks a few RFC patches are in place soon for git-stats too and is glad you are doing the fine-tuning wrt patch submission.

By the way, do you have public git repository set up somewhere (for example at repo.or.cz)?

Shawn, what is the status of other git GSoC 2008 projects?

Let's not ask Shawn who is overloaded with work already, but the mentors/students.

As for my student, Miklos, who is pretty close to submitting builtin-merge rc1 (to be held off until after 1.5.6), he has

	http://repo.or.cz/w/git/vmiklos.git

Ciao,
Dscho