Re: Triggers status?
On Wed, 24 Oct 2007, Joey Hess wrote:
> I can understand why all these might be best practices for submitting
> patches via git for a project like the linux kernel, but dpkg is not
> exactly drowning in perfectly-presented git patchsets, and IMHO there are
> good reasons to ignore every suggestion you made.
When there are good reasons, I'm happy to let anyone ignore my
suggestions. But in this case, given that Ian is just learning git
usage I believe that most of the things listed are consequences of
errors / lack of knowledge rather than deliberate choices.
> > - please rebase it on the current master branch (that way you're sure that
> > there are no conflicts)
>
> I have yet to see a use of git rebase that I am comfortable with.
> History is very important to me, even the ugly bits. Being able to see
> every misttep that was committed actually helps understanding the end
> result.
>
> Merging it with the current upstream master would have the same effect
> with a more "ugly" history, surely?
Well, plain "git rebase" compared to "git merge" only avoids (possibly
repeated) merge commits. It doesn't change contents of the patches and
doesn't changes author dates / emails.
I fail to see what (important) history is lost.
> > - please update your git identity, I see "Ian Jackson
> > <ian@anarres.relativity.greenend.org.uk>". You should standardize
> > your contributions to a single email. FYI, your previous patches have been
> > applied as "Ian Jackson <iwj@ubuntu.com>". You can use git-filter-branch
> > to rewrite that field automatically.
>
> Again, history is important to me, in this case I quite like that git
> preserves a history of where I did my work, so I have no interest in
> using a single email address.
>
> (AFAIK, this isn't even a general best practice. For example Linus
> commits from linus@woody.linux-foundation.org and signs off patches from
> linus@linux-foundation.org. Earlier in the kernel history, it's
> torvalds@ppc970.osdl.org.)
If this is a deliberate choice, then why not. But I believe this is just
the result of not having set up the user.name and user.email git
configuration items...
> > - please update the commit logs, a copy/paste of the ubuntu changelog
> > doesn't look good to me, follow the recommandation of using
> > a summary in the first line, then an empty line, then a long
> > description.
> > see http://wiki.debian.org/Teams/Dpkg/GitUsage
> > (this can be done during the interactive rebase process, see man
> > git-rebase and its -i option)
>
> Again, history is important to me, moreover, this is a lot of work for
> essentially zero gain. Yes, I'm lazy. If I have a perfectly ok changelog
> entry in debian/changelog, I will debcommit it, and not reiterate the
> same changes in a different style.
Many git tools rely on this convention to offer differing short/long view
of changesets. So I don't like when the first line of the commit log is:
"dpkg (1.14.5ubuntu16) gutsy; urgency=low"
At least debcommit provides real a changelog entry on the first line...
even if it's not a summary line, it's way better than this (in particular
when your respect the "a commit == a change" rule).
> > - you can also restructure the serie of changes with "git rebase -i origin/master"
> > (provided that origin/master is an up-to-date copy of the master branch
> > of the main dpkg repository)
> > (merge several commits in a single, in particular bug fixes with initial
> > implementation so that while reviewing diff you don't find bugs which
> > are in fact fixed by a subsequent commit)
>
> This loses history, is highly complex, requires a significant knowledge
> of git (more than I am comfortable with after using it for a month), and
> AFAICS is an anourmous time sink for no benefit. I'd rather be coding.
Honestly I wouldn't suggest it if it wasn't dead easy. I takes a few
seconds at most to squash a changeset in another one.
Then you're right, it looses history, so it's not required to proceed that
way. But in many cases, it can ease the review because you'll present your
change as a coherent serie of patches:
- logical change 1
- logical change 2
- logical change 3
- ...
Instead of:
- logical change 1
- logical change 2
- bug fix on logical change 1
- bug fix on ...
- ...
- logical change 3
- ...
> > I can't judge the content but following the "right form" is more likely to
> > lead to a quicker review.
>
> If that's actually true of dpkg, I'll be sure to limit the information I
> publish about dpkg patches to unified diffs and avoid all this nonsense.
Sure, patches are perfectly fine.
> As someone who needs this patch merged as soon as possible so we can
> start *using* it, your comments came off as somewhat tinged with
> obstructionism and git fanboyism. I'm sure you didn't intend that, but
> that was my impression. :-/
I certainly didn't intend that and if you have followed, I pushed Guillem
and Frank hard to get my own work merged. So I only share my experience.
I wish we could have a quicker review/integration cycle. But submitting a
git branch in the form that Ian used doesn't help much unless you follow
the conventions of the team.
Cheers,
--
Raphaël Hertzog
Premier livre français sur Debian GNU/Linux :
http://www.ouaza.com/livre/admin-debian/
Reply to: