You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2007/11/28 19:44:36 UTC
[PATCH] please review: kicking off the tree-conflicts branch
Hi folks,
Steve and I have spent some time trying to get a patch done
that is suitable to kick off the tree conflicts branch.
The patch is supposed to show the ideas behind what we are going
to do on the branch. We don't want to start the branch off by
committing a diff that is so large that no one will be bothered
to review it, or will have a hard time doing so.
So we tried to keep implementation details out of it for now (mostly),
and left a few other changes we've already made here and there for later.
The comments in the treeconflicts.h header give an overview of the
current design ideas. That file is right at the top of the patch,
so if you don't want to review the whole thing you can still comfortably
comment on the design notes if you want to.
Most functions that we are currently planning to add on the branch
are declared in this patch, and their bodies are either empty
(if they are obvious) or contain some comments that we plan to change
into code over time as we continue working on the branch.
It would be great if especially people with skills in either
or both the working copy and conflict areas could take a look
and let us know if we are on the right track.
Patch attached.
Thanks,
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: [PATCH] please review: kicking off the tree-conflicts branch
Posted by Erik Huelsmann <eh...@gmail.com>.
On Dec 3, 2007 10:54 PM, Karl Fogel <kf...@red-bean.com> wrote:
> "Erik Huelsmann" <eh...@gmail.com> writes:
> > more feedback, but I had kind of an emergency this weekend (spending
> > most of it on my family).
>
> Urp? Everything okay...?
>
It is now, but for the third saturday in a row Eline was quite ill,
having both diarhea and throwing up. This is particularly a problem
because children her age are still too small to allow for longer
periods of fluid-loss.
So, we spent some time going to the docter's on saturday and
monitoring her on sunday, but, after another visit to the docter's
today, she's getting better again. We'll be re-introducing her slowly
to different kinds of food in the coming days. In school there are
many cases of the flu, so for now, she'll stay at home for at least
two weeks.
The problem (and the panic) comes from the fact that we don't really
know what caused the problems. We hope it's just accidental that she
caught different diseases from different places. In that case, this
was 'just' a one-off scare.
Thanks for inquiring!
Is all well on your side of the ocean?
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] please review: kicking off the tree-conflicts branch
Posted by Erik Huelsmann <eh...@gmail.com>.
On Dec 3, 2007 8:50 PM, Stefan Sperling <st...@elego.de> wrote:
> On Mon, Dec 03, 2007 at 11:32:30AM -0800, David Glasser wrote:
> > The reason it works now is that the only place to change a depth after
> > initialization is in libsvn_wc/update_editor.c(complete_directory),
> > which doesn't use svn_wc__entry_modify (which is the caller of
> > fold_entry). This is more of an explanation of why it works than why
> > it is this way, though.
>
> I'll guess I'll just keep the defines and tree-conflict related
> entry modification in svn_wc__modify_entry() for now.
> We can remove it later when it turns out we don't need it.
>
> I'm not entirely sure where exactly we'll end up updating
> the tree-conflict-data entry yet anyway.
That seems the right way to do it, especially after David explained
why it works for depth as it does. I'm sorry I wasn't able to give
more feedback, but I had kind of an emergency this weekend (spending
most of it on my family).
HTH,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] please review: kicking off the tree-conflicts branch
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Dec 03, 2007 at 11:32:30AM -0800, David Glasser wrote:
> The reason it works now is that the only place to change a depth after
> initialization is in libsvn_wc/update_editor.c(complete_directory),
> which doesn't use svn_wc__entry_modify (which is the caller of
> fold_entry). This is more of an explanation of why it works than why
> it is this way, though.
I'll guess I'll just keep the defines and tree-conflict related
entry modification in svn_wc__modify_entry() for now.
We can remove it later when it turns out we don't need it.
I'm not entirely sure where exactly we'll end up updating
the tree-conflict-data entry yet anyway.
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: [PATCH] please review: kicking off the tree-conflicts branch
Posted by David Glasser <gl...@davidglasser.net>.
On Dec 3, 2007 11:16 AM, Stefan Sperling <st...@elego.de> wrote:
> Hey Erik,
>
> On Wed, Nov 28, 2007 at 09:30:41PM +0100, Erik Huelsmann wrote:
> > - The patch picks up a comment line in libsvn_wc/wc.h about there
> > being no #define for DEPTH because it's only relevant to the THIS_DIR
> > entry. From your description, I get the feeling the same applies to
> > the TREE_CONFLICT_DATA and TREE_CONFLICT_DESCRIPTION items. Is that
> > correct? And do you think it's like DEPTH enough to do without the
> > defines?
>
> Hmmm...
>
> We honestly don't know.
>
> Can someone explain why fold_entry in libsvn_wc/entries.c does not require a
> modify flag for depth? A comment says depth was "only meaningful on the
> this-dir entry anyway". Why does not needing a modify flag follow from this?
> Is it because for files, the depth will be inherited from the parent directory?
Well, files don't have a depth, and almost all data about directories
should be drawn from their '.' entry in their own directory rather
than the one in their parent.
I don't really follow that comment, though.
The reason it works now is that the only place to change a depth after
initialization is in libsvn_wc/update_editor.c(complete_directory),
which doesn't use svn_wc__entry_modify (which is the caller of
fold_entry). This is more of an explanation of why it works than why
it is this way, though.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] please review: kicking off the tree-conflicts branch
Posted by Stefan Sperling <st...@elego.de>.
Hey Erik,
On Wed, Nov 28, 2007 at 09:30:41PM +0100, Erik Huelsmann wrote:
> - The patch picks up a comment line in libsvn_wc/wc.h about there
> being no #define for DEPTH because it's only relevant to the THIS_DIR
> entry. From your description, I get the feeling the same applies to
> the TREE_CONFLICT_DATA and TREE_CONFLICT_DESCRIPTION items. Is that
> correct? And do you think it's like DEPTH enough to do without the
> defines?
Hmmm...
We honestly don't know.
Can someone explain why fold_entry in libsvn_wc/entries.c does not require a
modify flag for depth? A comment says depth was "only meaningful on the
this-dir entry anyway". Why does not needing a modify flag follow from this?
Is it because for files, the depth will be inherited from the parent directory?
Our tree conflict info will also only be stored in the this-dir entry. But we
still need to modify it when conflicts are added/removed (resolved), hence we
need to modify the entry, hence we need a flag... or don't we?
Thanks,
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: [PATCH] please review: kicking off the tree-conflicts branch
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Nov 28, 2007 at 09:30:41PM +0100, Erik Huelsmann wrote:
> > It would be great if especially people with skills in either
> > or both the working copy and conflict areas could take a look
> > and let us know if we are on the right track.
>
> Hi Stefan,
>
> I'm reading the patch and typing comments here. I think it's great
> someone finally picks up this issue.
Hey Erik,
wow, thanks for your quick reply. I read it yesterday (it came within
minutes), but I was done with work for the day and did not feel
like replying immediately.
> I do have some comments and will just put them here in the order I
> encounter them in the patch:
We will address those, thanks!
> Well, that's what I got from cursory review. I'll keep an eye on the
> branch too, but if necessary, I'll send more comments on this patch
> later.
You can take a bit of time to review this patch actually.
Steve and I will be at OpenCon over the weekend (http://opencon.org),
and we'll catch a train to Venice later today.
So I don't think either of us will get much work done until
Monday ;)
And after OpenCon, I'll have to undergo medical treatment
(my last remaining wisdom tooth will be pulled out),
and I probably won't be able to create the tree conflicts
branch until I've recovered from that (some time later next
week, or even the week after, we'll see).
Steve will be able to improve things during the next week
though, and maybe post an updated patch at some point.
Thanks,
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: [PATCH] please review: kicking off the tree-conflicts branch
Posted by Stephen Butler <sb...@elego.de>.
Hi Erik,
Thanks for the quick feedback.
Quoting Erik Huelsmann <eh...@gmail.com>:
> - The description of svn_wc__write_tree_conflict_descs() looks
> incomplete (see the line before the last)
Will fix the typo (an excess word) in the comment.
> - svn_wc__is_tree_conflict_victim() has an output argument
> (*tree_conflict_victim), but it's placed in the 'wrong' place: the
> convention in our code is to put pools last and output arguments first
> in the argument list.
Will fix the argument order.
> - Since svn_wc__write_tree_conflict_descs() adds information to the
> directory entry, how do you envision this to be idempotent? The
> comment seems to suggest the loggy system has to do with that, but the
> loggy system is there for atomicity (not idempotenticity).
In svn_wc__add_tree_conflict_data() we add info to an entry field by
reading the field, then rewriting the entire field in one loggy step.
In svn_wc__tree_conflict_resolved() we remove info from the entry field
in the same way.
svn_wc__write_tree_conflict_descs() doesn't alter the entry. It
converts the info into readable, localized sentences and writes them
to a temp file. Then svn_wc__loggy_move() replaces the user-visible
file with the temp file. FWIW, we don't use the deprecated function
svn_wc__loggy_append(), because it's not idempotent.
We think this design is idempotent (assuming that
> - The patch picks up a comment line in libsvn_wc/wc.h about there
> being no #define for DEPTH because it's only relevant to the THIS_DIR
> entry. From your description, I get the feeling the same applies to
> the TREE_CONFLICT_DATA and TREE_CONFLICT_DESCRIPTION items. Is that
> correct? And do you think it's like DEPTH enough to do without the
> defines?
I'm afraid I don't know enough about DEPTH. I simply took CONFLICT_WRK
and friends as my model in setting up the new entry fields. They are
written in the working copy but not transmitted to the repository, and
the tree conflict fields should have the same behavior. The #defines
for the new fields are needed in entries.c and log.c.
Thanks,
Steve
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] please review: kicking off the tree-conflicts branch
Posted by Erik Huelsmann <eh...@gmail.com>.
On Nov 28, 2007 8:44 PM, Stefan Sperling <st...@elego.de> wrote:
> Hi folks,
>
> Steve and I have spent some time trying to get a patch done
> that is suitable to kick off the tree conflicts branch.
>
> The patch is supposed to show the ideas behind what we are going
> to do on the branch. We don't want to start the branch off by
> committing a diff that is so large that no one will be bothered
> to review it, or will have a hard time doing so.
>
> So we tried to keep implementation details out of it for now (mostly),
> and left a few other changes we've already made here and there for later.
>
> The comments in the treeconflicts.h header give an overview of the
> current design ideas. That file is right at the top of the patch,
> so if you don't want to review the whole thing you can still comfortably
> comment on the design notes if you want to.
>
> Most functions that we are currently planning to add on the branch
> are declared in this patch, and their bodies are either empty
> (if they are obvious) or contain some comments that we plan to change
> into code over time as we continue working on the branch.
>
> It would be great if especially people with skills in either
> or both the working copy and conflict areas could take a look
> and let us know if we are on the right track.
Hi Stefan,
I'm reading the patch and typing comments here. I think it's great
someone finally picks up this issue.
I do have some comments and will just put them here in the order I
encounter them in the patch:
- The description of svn_wc__write_tree_conflict_descs() looks
incomplete (see the line before the last)
- svn_wc__is_tree_conflict_victim() has an output argument
(*tree_conflict_victim), but it's placed in the 'wrong' place: the
convention in our code is to put pools last and output arguments first
in the argument list.
- Since svn_wc__write_tree_conflict_descs() adds information to the
directory entry, how do you envision this to be idempotent? The
comment seems to suggest the loggy system has to do with that, but the
loggy system is there for atomicity (not idempotenticity).
- The patch picks up a comment line in libsvn_wc/wc.h about there
being no #define for DEPTH because it's only relevant to the THIS_DIR
entry. From your description, I get the feeling the same applies to
the TREE_CONFLICT_DATA and TREE_CONFLICT_DESCRIPTION items. Is that
correct? And do you think it's like DEPTH enough to do without the
defines?
Well, that's what I got from cursory review. I'll keep an eye on the
branch too, but if necessary, I'll send more comments on this patch
later.
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org