Re: Triggers status?
Ian,
I really don't understand why you
On Thu, 25 Oct 2007, Ian Jackson wrote:
> I made a deliberate choice to publish my triggers work as a branch
> that could be merged in both directions.
1/ Only the official branch might be merged repeatedly in your branch.
2/ Your branch will be merged only once on the official branch (further
bugfixes after the merge will be directly done on the officiel branch
since you have commit rights there).
> That was necessary to facilitate merging changes from Debian into the
Stop contesting everything please. It was not "necessary". Please
understand that rebasing one branch over the other also produces
a result where the changes are merged (but the history is presented in a
different way).
> revised. It meant I could merge fixes from the trunk into the
> triggers branch, and then into the flex branch, or just into the flex
And you could just as well rebase your flex branch on top of the triggers
branch.
> undue difficulty. (By undue difficulty I mean without imposing undue
> work on a human, for example by generating spurious conflicts on
> merging or requiring a particular conflict to be resolved twice.) It
If you had followed my advice from the beginning, you'd not have had to
resolve the conflict twice.
> meant that I could publish triggers, and my flex experiment, in a way
> that means that other people can edit their own versions and
> contribute aappropriate.
We already had this discussion on IRC, it's very annoying to have it
again.
1/ You're most likely the only person working on top of those branches (the
copies in the nature are just read-only).
2/ You can leave the published repository as is and create another
repository containing a rebased branch specifically for "upstream
submission". That was the suggestion made by Colin and which looked
sensible.
> I was able to do all of these things straightforwardly and with a
> minimum of fuss, once I'd got used to git's slightly odd terminology
> and the usual initial kind of minor stumbling block with a new tool.
>
> The only problem seems to me that you are objecting to this mode of
> interaction. Can you please explain what is lost by my approach ?
I'm not objecting to it in general. It's a very valid approach but since
you want the dpkg team to merge that branch in the main branch, I expect
that branch to be following the guidelines for the commit logs and to have
valid emails.
I suggested you to have those fixed. But this is not doable without
rewriting the history (using git-rebase and git-filter-branch).
Next time when you have everything straight from the beginning, it will be
ok to work this way.
On the other hand, I really have the impression that you have an
unexplained fear of git-rebase and that you refuse it without even trying
to understand what it does.
> > 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...
>
> No, it's due to a bug in the Debian git package, which should have
> acquired my email address in the standard way. In that case it would
> have used a correct address.
Honestly I don't care whose fault it is. Feel free to submit a wishlist
bug against git. Most git tutorials start by explaining that you need to
configure this.
> > 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.
>
> AIUI git rebase also prevents repeated merging backwards and forwards
> between different branches.
Yes. But you can just as well use rebase between those branches!
> > 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"
>
> I agree that this is unhelpful. I can't remember why I didn't use
> debcommit at that point but I'm using it nowadays and I'm happy to
> continue doing so.
>
> Is there really merit in going back and fixing old commit log entries ?
I have no ultimate answer. What I know is that if I were you, I'd do it.
> > 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
>
> The actual triggers functionality isn't sensibly divisible in this way
> and the individual bugfixes which I've made at the same time are too
> trivial to care about in this way.
Well, as someone who is not familiar with dpkg's C codebase, I really
disagree. Small commits are easier to understand because you see all the
related changes.
Here are some examples of changes that I would have liked to see in
seperate commits:
- Move some functions into libdpkg
- Created locksomething and modified lockdatabase to use that generic lock function
(needed to be able to lock the trigger directory separately)
- Factorize some duplicate code into fgets_checked
For those I noticed yesterday during the review. I'm sure there are other
similar logical changes.
> > Sure, patches are perfectly fine.
>
> If I were to supply patches and you were to apply them, my flex branch
> would be completely messed up, wouldn't it ?
Messed up is a bit strong.
> Next time I wanted to do a merge it would try to apply another copy of the
> triggers changes.
Yes but it would detects parts that are already merged provided that they don't
touch the same code.
> The whole point of using a drcs is to avoid that kind of thing.
Sure, but as I told you, git cares about the content and not that much
about the way we got to that particular content. I asked you last night to
try the experiment... since you didn't try, I'm going to do it.
$ git branch -a
* master <- your dpkg.triggers branch
test
test2
main/dpkg-shlibdeps-buxy
main/etch
main/master <- your dpkg.debian branch
origin/HEAD
origin/master
speedup/master <- your dpkg.speedup branch
$ git diff main/master >patch
$ git checkout -b debian-patch main/master
Branch debian-patch set up to track remote branch refs/remotes/main/master.
Switched to a new branch "debian-patch"
$ git-apply patch
Adds trailing whitespace.
patch:174:
Adds trailing whitespace.
patch:651:
Adds trailing whitespace.
patch:1422: *
Adds trailing whitespace.
patch:1427: *
Adds trailing whitespace.
patch:1432: *
warning: squelched 47 whitespace errors
warning: 52 lines add whitespace errors.
$ git status
[ lots of modified files ]
# Untracked files:
# (use "git add <file>..." to include in what will be committed)
#
# doc/
# lib/dlist.h
# lib/trigdeferred.l
# lib/triglib.c
# patch
# src/trigcmd.c
# src/trigproc.c
no changes added to commit (use "git add" and/or "git commit -a")
$ git add doc lib/dlist.h lib/trig* src/trig*
$ git commit -a -m "Apply triggers patch"
Created commit 695f40d: Apply triggers patch
54 files changed, 4300 insertions(+), 1368 deletions(-)
create mode 100644 doc/triggers.text
create mode 100644 lib/dlist.h
create mode 100644 lib/trigdeferred.l
create mode 100644 lib/triglib.c
create mode 100644 src/trigcmd.c
create mode 100644 src/trigproc.c
Ok, I have applied the patch on top of dpkg.debian. Let's see if I can update
your dpkg.speedup branch ...
$ git checkout -b speedup-update speedup/master
Branch speedup-update set up to track remote branch refs/remotes/speedup/master.
Switched to a new branch "speedup-update"
I try the merge directly: I get conflicts but not all on all files as you
expected.
$ git merge debian-patch
Auto-merged configure.ac
Auto-merged debian/control
Auto-merged dselect/pkglist.cc
Auto-merged lib/Makefile.am
CONFLICT (content): Merge conflict in lib/Makefile.am
Auto-merged lib/database.c
Auto-merged lib/dbmodify.c
CONFLICT (content): Merge conflict in lib/dbmodify.c
Auto-merged lib/dpkg-db.h
Auto-merged lib/dpkg.h
CONFLICT (content): Merge conflict in lib/dpkg.h
Auto-merged lib/dump.c
Auto-merged lib/ehandle.c
CONFLICT (content): Merge conflict in lib/ehandle.c
CONFLICT (delete/modify): lib/fields.c deleted in HEAD and modified in debian-patch. Version debian-patch of lib/fields.c left in tree.
Auto-merged lib/parse.c
CONFLICT (content): Merge conflict in lib/parse.c
Auto-merged lib/parsedump.h
Auto-merged lib/parsehelp.c
Auto-merged lib/tarfn.c
CONFLICT (content): Merge conflict in lib/tarfn.c
Auto-merged lib/trigdeferred.l
CONFLICT (add/add): Merge conflict in lib/trigdeferred.l
Auto-merged lib/triglib.c
CONFLICT (add/add): Merge conflict in lib/triglib.c
Removed scripts/dpkg-buildpackage.sh
Auto-merged src/Makefile.am
CONFLICT (content): Merge conflict in src/Makefile.am
Auto-merged src/archives.c
Auto-merged src/configure.c
CONFLICT (content): Merge conflict in src/configure.c
Auto-merged src/enquiry.c
Auto-merged src/errors.c
Auto-merged src/filesdb.c
Auto-merged src/help.c
Auto-merged src/packages.c
Auto-merged src/processarc.c
Auto-merged src/query.c
Auto-merged src/remove.c
Auto-merged src/trigproc.c
CONFLICT (add/add): Merge conflict in src/trigproc.c
Automatic merge failed; fix conflicts and then commit the result.
If we follow your theory, we should have conflicts everywhere
because all the changes done in the triggers branch
are "reapplied" via the "debian-update" branch. But it's not the case, git
detects that some changes are already applied and merges them smartly.
The conflicts are effectively parts of the code that got modified
in both branches. Git doesn't want to decide who is right and who is wrong.
See http://marc.info/?l=git&m=119198488411957&w=2 for some related discussion
where Linus explains how git handles such a situation.
So, yes, merging a patch is a bit less convenient for you, but it's not the end
of the world either. Merging your branches is the right thing to do for your
future contributions, but IMO this will happen only if the branches are "clean".
Thus my suggestion to fix/rebase both your branches and continue directly with
clean branches.
End of the story for me. I explained you git as well as I could, it's up to you
to see if you want to make that little effort or not. And it's up to
Guillem/Frank to decide what they're ready to apply.
> > 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.
>
> I don't understand how presenting it as a git branch can be inferior
> to presenting it as a patch.
It's not inferior, we can always generate a patch with the branch. But
generating that patch is not a one-step process if the branch is not
rebased/merged with the last version.
And when someone submits a branch instead of a patch, I somewhat
expect the branch to be of a quality suited to be merged.
Cheers,
--
Raphaël Hertzog
Premier livre français sur Debian GNU/Linux :
http://www.ouaza.com/livre/admin-debian/
Reply to: