Automatic message when a commit is pushed on a branch : why see the dependencies ?

[Nathann Cohen]

Hellooooooo everybody !!!

I coded a bit too much in the last days, and created several tickets
depending on each other. The last of them (#15310) only contains three
commits (those who message begins with "Wilson's construction"), but
many more appear on the page. Wouldn't it be better if we only saw on
that page the list of patches that are related to THIS ticket ? All
the others will have been reviewed in the dependencies, and it's hard
to tell if a ticket contains a lot of code or not when you see the
ticket's code mixed with the code from its dependencies.

I just looked at the code of trac-githooks, and it looks like there is
not much to change, even though it's hard for me to .... actually try
anything :-P

The "log_table" function defined in
trac-githooks/post-receive.d/01-trac_branch features a "ignore"
argument (which contains master by default), and all that would be
needed would be to add there the reference toward the content of the
"dependencies" field of the trac ticket. What do you think ? And is
there any way for me to do the job rather than posting here ?

Have fuuuuuuuuuuuuun ! And thanks ;-)

Nathann

[Volker Braun]

Each branch contains all commits from all dependencies, i.e. his entire past in the DAG. All of this is related to THIS ticket since there is no guarantee that dependency tickets will be reviewed at all. If you want to disentangle the dependencies, have them reviewed first.

[Jean-Pierre Flori]

On Tuesday, October 22, 2013 7:11:57 PM UTC+2, Volker Braun wrote:

Each branch contains all commits from all dependencies, i.e. his entire past in the DAG. All of this is related to THIS ticket since there is no guarantee that dependency tickets will be reviewed at all. If you want to disentangle the dependencies, have them reviewed first.

Maybe we could have both views? :)

[Nathann Cohen]

> If you want to disentangle the dependencies, have them reviewed first.

Hey, man. We're a collaborative project, remember ? That means we
discuss things.

More to the point : that's what we already do with HG, i.e. naming the
dependencies explicitly, and we don't set a ticket to positive_review
before the dependencies have been reviewed. It worked for years.

I split a large ticket into smaller ones because the changes make more
sense this way, and are easier to review. I can't ask one person to
spend 6 months reviewing a big ticket, but I can expect to have
somebody review some changes from time to time. That makes sense.

And it would be nice if git does not force us to review 200k of commits at once.

Nathann

[Volker Braun]

On Tuesday, October 22, 2013 6:18:56 PM UTC+1, Nathann Cohen wrote:

More to the point : that's what we already do with HG, i.e. naming the
dependencies explicitly, and we don't set a ticket to positive_review
before the dependencies have been reviewed.

And nothing changes. Just point the reviewer at the base ticket (which doesn't depend on others) and have him review that first. Which is exactly what the workflow with HG would have been as well. Except that you were able to cheat with patches and pretend that you can review something that you can't without considering the dependencies.

[Nathann Cohen]

> And nothing changes.

Nono I swear, something changes ! Formerly I could have a ticket with
a 2-lines patch even if this thing depends on a 100k lines patch. I
would like to do the same here.

> Just point the reviewer at the base ticket (which
> doesn't depend on others) and have him review that first.

It would be nice if I had a reviewer right now. I don't. I would like
to be able to write several patches depending on each other, and I
don't want the last of them to be stuffed with totally unrelated
commits, i.e. commits that are dependencies, and not implemented by
this ticket.

> Except that you were able
> to cheat with patches and pretend that you can review something that you
> can't without considering the dependencies.

Cheating ? Come on Volker, the "dependencies" field is pretty clear,
everybody knows it and has been working this way for years.

What's the problem with not printing the commits from the dependencies ?

Nathann

[Jason Grout]

On 10/22/13 12:15 PM, Jean-Pierre Flori wrote:
> Maybe we could have both views? :)

+1.

Jason

[Volker Braun]

On Tuesday, October 22, 2013 6:25:58 PM UTC+1, Nathann Cohen wrote:

It would be nice if I had a reviewer right now. I don't. I would like
to be able to write several patches depending on each other, and I
don't want the last of them to be stuffed with totally unrelated
commits, i.e. commits that are dependencies, and not implemented by
this ticket.

Once the dependency is merged, the commits from the dependency will no longer be shown.

What's the problem with not printing the commits from the dependencies ?

The problem with sending patches around is that you'll mess up regularly and nobody notices. Its super easy to get an old version and nobody ever finds out. The whole point of git is that it performs the source management for you. So you get 100% reliability. But it also means that you can't be imprecise about what code is part of the ticket and what isn't.

Now, the problem with not showing the dependencies is that they are still dependencies. If you branch off that ticket, those dependencies are your dependencies. Git doesn't let you get away with any imprecision, and that is precisely the feature that we need to reliably develop software. The branch on the ticket is all of the code needed to merge that ticket. Trying to hide the fact that the ticket contains all of that code will just trick people into reviewing things in the wrong order, and burden the release manager with figuring out a parallel dependency graph to the git dependency graph to make sure that nobody messed up.

[Jason Grout]

On 10/22/13 12:40 PM, Volker Braun wrote:
> Trying to hide the fact that the ticket contains all of that code will
> just trick people into reviewing things in the wrong order, and burden
> the release manager with figuring out a parallel dependency graph to the
> git dependency graph to make sure that nobody messed up.

In some cases, for example, fixing a typo in another unreviewed commit,
there shouldn't be a problem with reviewing it before the dependency is
reviewed.

As for burden, surely we can automate the tools enough so that the
release manager only considers positively-reviewed tickets that connect
back to the main branch (through a chain of positively-reviewed commits).

Thanks,

Jason

[Volker Braun]

On Tuesday, October 22, 2013 6:45:45 PM UTC+1, Jason Grout wrote:

In some cases, for example, fixing a typo in another unreviewed commit,
there shouldn't be a problem with reviewing it before the dependency is
reviewed.

No, huge problems. For example, you can (accidentally or maliciously) remove the dependency after the typo ticket is reviewed. Then the wholew code will be pulled in by the typo ticket. And good luck writing a script that catches that, it depends on the entire trac history.

[Jason Grout]

On 10/22/13 12:55 PM, Volker Braun wrote:
> No, huge problems. For example, you can (accidentally or maliciously)
> remove the dependency after the typo ticket is reviewed. Then the wholew
> code will be pulled in by the typo ticket. And good luck writing a
> script that catches that, it depends on the entire trac history.

Changing something like that on a ticket would usually trigger a
re-review, right?

Does the current graph show back to the master branch, or back to the
most recently positively-reviewed ticket?

Thanks,

Jason

[Volker Braun]

On Tuesday, October 22, 2013 7:07:21 PM UTC+1, Jason Grout wrote:

Changing something like that on a ticket would usually trigger a
re-review, right? 

Theoretically yes, in practice I think a lot of people will not notice what is happening. And that is just the tip of the ice berg. You could abandon the dependency ticket and have it closed as invalid. Then the positively reviewed depedendent ticket is merged without ever having something changed. Or you can abandon just some commits on the dependency ticket, which now silently become part of the dependent ticket. And all that complication with only two tickets, if you have multiple tickets with circular trac dependencies then it gets even more complicated.

Does the current graph show back to the master branch, or back to the
most recently positively-reviewed ticket?

The master branch, since that is what is definitely merged.

[Nathann Cohen]

> Once the dependency is merged, the commits from the dependency will no
> longer be shown.

My post was about the messages that are added to the ticket when a
branch is updated. Surely this will not be edited backward when the
dependencies are reviewed ?

Second question : has the dependencies field become useless, then ?
And if not, what does it mean ?

Nathann

[Volker Braun]

On Tuesday, October 22, 2013 8:24:18 PM UTC+1, Nathann Cohen wrote:

My post was about the messages that are added to the ticket when a
branch is updated. Surely this will not be edited backward when the
dependencies are reviewed ?

Correct. But if you click on the branch or use "sage -dev diff" then you won't see the already-merged dependencies.

 

Second question : has the dependencies field become useless, then ?
And if not, what does it mean ? 

It means: Please review the dependency ticket first. Because if you start reviewing this ticket first then you'll have to wade through code that is meant to be reviewed elsewhere. 

[Jason Grout]

On 10/22/13 2:29 PM, Volker Braun wrote:
> It means: Please review the dependency ticket first. Because if you
> start reviewing this ticket first then you'll have to wade through code
> that is meant to be reviewed elsewhere.

Which is exactly why it makes sense to me to have a separate, additional
view of exactly what is on this ticket.

Suppose there is ticket A that is a dependency for B, which in turn is a
dependency for C.

I review A. When I go to B, I try to see the commits just on B to make
sure I know exactly what I've done and what I'm doing now. However,
since the list of commits shows everything since master (and A hasn't
been merged), I have to manually match things up from A. The problem
gets worse when I go to C. Talk about having to manually match up the
dependency graphs :).

Also, setting a ticket to positive review does not mean it will get
merged as-is. There are lots of times that the release manager kicks
back a ticket because it conflicts with something else, or it fails a
doctest on Solaris, or some other reason. So even if I positively
review a chain of commits, the ticket that ties the branch to master can
be marked as not reviewed anymore---that happens all the time. Then
we're in the exact same situation you've described in the other
subthread, where a ticket is positively reviewed, but a dependency
isn't. Withholding information from the user won't prevent this problem.

I think it makes a lot of sense to have both views: commits from master,
and commits from the dependencies. You're just providing more valuable
information about exactly what is going on. This extra information
isn't enforcing one development style over another. I think it's
something of a straw man that we've gone way off track talking about
development practices.

Anyways, I think I've used up my share of bandwidth talking about this
issue.

Jason

[Nicolas M. Thiery]

Dear Volker,

On Tue, Oct 22, 2013 at 12:29:15PM -0700, Volker Braun wrote:
> It means: Please review the dependency ticket first. Because if you start
> reviewing this ticket first then you'll have to wade through code that is
> meant to be reviewed elsewhere.

Code base dependability and security are certainly worthwhile design
goals to pursue, and I really appreciate your pushing toward using the
best of git for a safer development workflow. Of course, there are
other design goals including productivity (i.e. getting high quality
code and high quality reviews done in a short amount of time), and in
case of conflict between two design goals we need to weight one
against the other.

I have seen many occasions during the Sage development where our
current workflow was a productivity sink. I ran into code
dependability issues a couple times (e.g. unreviewed code getting
inadvertently into Sage). And so far I personally have never witnessed
an attempt to introduce malicious code. Therefore my opinion is that,
for our project and at this point in time, productivity should have a
high weight.

Conditional reviews (reviewing a ticket depending on not-yet-reviewed
ticket) is a feature that many people, including myself, requested. It
does increase productivity, especially in the typical situation where
different sets of skills are needed for the different tickets to be
reviewed. We all have other tasks to do and stringent time frames of
availability. The workflow should adapt to our constraints, not the
converse.

Thanks again for all your work on sage-git; I am really looking
forward to the much increased productivity!

Cheers,
Nicolas
--
Nicolas M. Thi�ry "Isil" <nth...@users.sf.net>
http://Nicolas.Thiery.name/

[Volker Braun]

On Tuesday, October 22, 2013 8:50:15 PM UTC+1, Jason Grout wrote:

Suppose there is ticket A that is a dependency for B, which in turn is a
dependency for C.

At what time was it a trac dependency? Trac depedencies can change, this is precisely the issue here. There is just no way to reliably decide whether the assumptions that the reviewers had about the trac dependencies at the time of review were sufficient to correctly review the tickets. You can only be sure if your dependencies are immutable, which is why git is the way it is.

I review A.  When I go to B, I try to see the commits just on B to make
sure I know exactly what I've done and what I'm doing now. However,
since the list of commits shows everything since master (and A hasn't
been merged), I have to manually match things up from A.

If you have reviewed A then you won't have a problem looking at A+B. You can even view the B-A commits if you want using "git log A ^B" or "sage -dev diff --base B". All I'm saying is: When you positively review B, then you implicitly give positive review to A.

Also, setting a ticket to positive review does not mean it will get
merged as-is.  

True, but to solve any problems found you can only add further commits, not go back in history. Any dependent ticket will either 

* have the same problem, so it'll work once it merges the bugfix from the base.

* or not, in which case it is fine to merge the dependency on its own. 

 

[Volker Braun]

On Tuesday, October 22, 2013 8:59:10 PM UTC+1, Nicolas M. Thiéry wrote:

Conditional reviews (reviewing a ticket depending on not-yet-reviewed
ticket) is a feature that many people, including myself, requested.

And nothing is stopping you. I'm just against pretending that doing the conditional review is safe. The default must always be the safe thing to do, not take your leg off if you don't know about the hidden assumptions.

Using git or the dev tools you can look at the changes with the dependencies removed. If you are aware of the risk, you can make conditional reviews. Git doesn't know or care about what you review, it just makes sure that you can't loose any commits. 

[Stefan van Zwam]

Does the current graph show back to the master branch, or back to the
most recently positively-reviewed ticket?

The master branch, since that is what is definitely merged.

That sounds counterproductive. Suppose I'm on a reviewing spree, and I want to review A -> B -> C. Once I've given A a positive review, I don't want to wait until it's merged before giving B a positive review! So I want to see what B adds relative to A.

So +1 to (also) having a view that only shows the changes C makes on top of A and B. 

[Dima Pasechnik]

Yet it would be very useful to hide unreviewed commits (or should I say patches?)
that commute with commits consituting the ticket. Surely they may
actually inerfere, but more often they won't.

Dima

>

[Volker Braun]

And, just to repeat myself: If you just reviewed A then you can't possibly be confused by seeing A+B as the default diff. You can use git or the dev scripts to see the A-B diff if that is what you want. 

But if you had _not_ just reviewed A then it would absolutely terrible to just show you A-B as default. You would be mislead to assume that this is all there is, and to catch your mistake would potentially require reconstructing the trac history (i.e. is impossible).

[Volker Braun]

On Wednesday, October 23, 2013 5:48:50 PM UTC+1, Dima Pasechnik wrote:

Yet it would be very useful to hide unreviewed commits (or should I say patches?)
that commute with commits consituting the ticket.

I'm going to copy and paste the same reply to this question from now on ;-)

You can exclude any commit or set of commits from the diff as you want with git or with the dev scripts. That is what git is made for, to manage changes and track differences between any two states of the source tree. But the only sane default(!) is to show the entire code on the ticket. Anything else would be terribly confusing to people not familiar with what is going on behind the scenes.

[Nicolas M. Thiery]

On Wed, Oct 23, 2013 at 12:54:49PM -0700, Volker Braun wrote:
> I'm going to copy and paste the same reply to this question from now on
> ;-)
> You can exclude any commit or set of commits from the diff as you want
> with git or with the dev scripts. That is what git is made for, to manage
> changes and track differences between any two states of the source tree.
> But the only sane default(!) is to show the entire code on the ticket.
> Anything else would be terribly confusing to people not familiar with what
> is going on behind the scenes.

I can be convinced that this is a sane default indeed. And yes you can
get the diff-wrt-dependencies with git. But this is an important
enough operation that it should not only be possible, but trivial to
get it, in particular from trac (I am happy with whatever warning you
will see fit when you open that view).

[Andrew Mathas]

On Wednesday, 23 October 2013 21:47:09 UTC+2, Volker Braun wrote:

And, just to repeat myself: If you just reviewed A then you can't possibly be confused by seeing A+B as the default diff. You can use git or the dev scripts to see the A-B diff if that is what you want. 

But if you had _not_ just reviewed A then it would absolutely terrible to just show you A-B as default. You would be mislead to assume that this is all there is, and to catch your mistake would potentially require reconstructing the trac history (i.e. is impossible).

 

Please correct me if I am wrong, but rereading this entire thread it seems that everyone -- Nathann, Stefan, Dima, Nicolas and Jason -- is arguing for just displaying the diffs for just the current patch with the exception of Volker. I also vote for this.

If I am reviewing ticket B which depends on A then I ONLY want to see the changes that are made by B. After all, this what I am supposed to be reviewing (I'm definitely NOT reviewing A). We have to assume that the reviewing process is robust enough that any issues with A will be detected when it is reviewed. If A changes and causes a rebase of B then this should automatically change any "positive review" of B to "needs review". I don't see any problems with this workflow. On the other hand, if by default I only see the changes made by both patches then, at least for large patches, it is more difficult to see what B is doing. This means that there is NO advantage  in breaking large patches into smaller ones.

The fact that it is possible to see the full changes A\pm B by playing tricks with git is neither here nor there: as with (almost) everyone else, I think that the default display should be the changes caused by the ticket - as we had in the halcyon pre-git days.

Andrew

[Keshav Kini]

Andrew Mathas <andrew...@gmail.com> writes:
> On Wednesday, 23 October 2013 21:47:09 UTC+2, Volker Braun wrote:
>
> And, just to repeat myself: If you just reviewed A then you can't
> possibly be confused by seeing A+B as the default diff. You can
> use git or the dev scripts to see the A-B diff if that is what
> you want.
>
> But if you had _not_ just reviewed A then it would absolutely
> terrible to just show you A-B as default. You would be mislead to
> assume that this is all there is, and to catch your mistake would
> potentially require reconstructing the trac history (i.e. is
> impossible).
>
>
>
> Please correct me if I am wrong, but rereading this entire thread it
> seems that everyone -- Nathann, Stefan, Dima, Nicolas and Jason -- is
> arguing for just displaying the diffs for just the current patch with
> the exception of Volker. I also vote for this.

Hi. I haven't been paying much attention to Sage for the last several
months but has it been worked out what exactly this would even mean?
One advantage of Volker's idea (which is the default behavior in git
world) is that it is obviously fully well-defined. As I recall, at the
Sage Git Days in Portland in March, the concept of "just displaying the
diffs for just the current patch" was somewhat of a stumbling block.

master
| .---------------B
| / `-------------.
O< .---`--C
| \ /
| `---------------A

Say C depends on A and B. What diff do we display here? By Volker's
suggestion, as I understand it, we just show the diff between the files
as they are at O and the files as they are at C, which is pretty simple.
What's the diff you suggest showing?

We did discuss this in March and came up with some kind of merge
caching, but it was rather unsatisfactory as I recall. Have people
decided on how this works now?

Sorry if I'm missing something that has been decided such that the above
problem has an obvious easy solution or is nonsensical in the first
place. I mostly just like drawing ASCII diagrams :)

-Keshav

[Andrew Mathas]

On Thursday, 24 October 2013 06:17:06 UTC+2, Keshav Kini wrote:

Sorry if I'm missing something that has been decided such that the above
problem has an obvious easy solution or is nonsensical in the first
place.  I mostly just like drawing ASCII diagrams :)

I'm not sure of what was discussed in March and I also apologise if I am missing something obvious:), but I would have thought you would apply all dependencies, in this case A and B to O, then give the diff when you apply C. As far as I can see, this is both easy to implement and well-defined.

When there are only a few dependencies, as in the mock examples above, I don't think that there is much of an issue. However, there are quite a few patches on trac which have 5+ dependencies. If the diff is displaying all of these dependencies, in addition to the patch that you are reviewing, then I don't think it the the diff is of much help when reviewing the patch.

Andrew

[Keshav Kini]

Andrew Mathas <andrew...@gmail.com> writes:

> On Thursday, 24 October 2013 06:17:06 UTC+2, Keshav Kini wrote:
>
>
> Sorry if I'm missing something that has been decided such that
> the above
> problem has an obvious easy solution or is nonsensical in the
> first
> place. I mostly just like drawing ASCII diagrams :)
>
>
>
> I'm not sure of what was discussed in March and I also apologise if I
> am missing something obvious:), but I would have thought you would
> apply all dependencies, in this case A and B to O, then give the diff
> when you apply C. As far as I can see, this is both easy to implement
> and well-defined.

This was one of the first suggestions back in March and we discarded it.

First of all, your suggestion is not easy to implement. Merging A or B
into O individually is trivial (in fact, they are already merged into O
-- think about why this is true). Merging both A and B into O means
merging A and B together. This may be nontrivial as conflicts may
arise. As A and B might not depend on each other, nobody is under any
obligation to have resolved these conflicts, and so such a merge will
not be possible to do automatically in general.

Furthermore, I would claim that this is not even the diff you want to
review. In the diff you suggest, the extra commits in A that are not in
C will be shown being undone in the diff. By positively reviewing such
a diff, you are certifying that you think that A should be rewound to
the point where C diverged from it. But if A and B are merged into
master, and then C is merged into master, this will not match with the
final state of the codebase.

-Keshav

[Keshav Kini]

Sorry, s/A/B/ in this paragraph. B is the one with extra commits that
aren't in C.

-Keshav

[Nathann Cohen]

Hmmmmmmm....

While talking with Nicolas and Florent, I wondered if there should not be some additional checking of compatibility using the TRAC information :

A ticket contains a "dependencies"  field, and this field contains an information that GIT does not have : a "branch" depends on others "branches", while GIT only understands that a branch may depend on a certain commit. Branches change, commits don't.

But while a branch B can use some commit from branch A in its history, perhaps the later additional commit added to branch A would make it incompatible with B (=can't be merged trivially)
--> In which case, the author of B would probably want to learn it, for his patch needs to be rebased on the latest commit of A !

Same way, as Keshav suggest, if his branch O is based on two branches A and B that are incompatible with each other, even though nobody may have noticed (or cared) at the moment.

We could probably have the patchbot tell us whenever "This ticket cannot be merged on top of all its dependencies in a trivial way".
Possibly by taking any order on the dependencies (or the order given by the "dependencies" field). Anyway some order will have to be picked eventually when these branches will be merged into Sage.

Annnnnnd I may have missed your explanatin Keshav, but I still don't get why "git log the_branch ^master ^dependency1 ^dependency2 ^dependency3" wouldn't make it. Looks good to me O_o

Nathann

[Keshav Kini]

Nathann Cohen <nathan...@gmail.com> writes:
> Hmmmmmmm....
>
> While talking with Nicolas and Florent, I wondered if there should
> not be some additional checking of compatibility using the TRAC
> information :
>
> A ticket contains a "dependencies"  field, and this field contains an
> information that GIT does not have : a "branch" depends on others
> "branches", while GIT only understands that a branch may depend on a
> certain commit. Branches change, commits don't.
>
> But while a branch B can use some commit from branch A in its
> history, perhaps the later additional commit added to branch A would
> make it incompatible with B (=can't be merged trivially)
> --> In which case, the author of B would probably want to learn it,
> for his patch needs to be rebased on the latest commit of A !

Under Volker's scheme (and the way git is usually used in other
projects), simply reviewing the diff is not enough for positive review.
You must also make sure that the branch can be successfully merged into
master without conflicts. One way to do so is to manually merge master
into your branch.

Also, as soon as you start rebasing things, you lose the history of
unification of branches. If you want to maintain that (for example
because you want some system that produces specialized diffs to be aware
of dependencies and their historical relationships), then you must
synchronously rebase all involved branches in the repo. This is
unworkable, so I don't recommend rebasing branches, especially if you
care about dependency tracking on the history graph level.

> Same way, as Keshav suggest, if his branch O is based on two branches
> A and B that are incompatible with each other, even though nobody may
> have noticed (or cared) at the moment.

Obviously they were still compatible at the point where the branch
(let's go back to calling it C since in my diagram O is something
different) split off from A and B.

It seems to me that since C is only using the commits it merged into
itself, C shouldn't be forced to care about whatever other stuff happens
to A and B afterwards. If there are merge conflicts that will make the
branch unmergeable to master once A and B get into master. If there are
more subtle conflicts (i.e. code behavior incompatibilities), presumably
the patchbot will notice this. (We discussed in March the necessity for
the patchbot to test not the branch's commit itself but the result of
merging it into master.)

> We could probably have the patchbot tell us whenever "This ticket
> cannot be merged on top of all its dependencies in a trivial way".
> Possibly by taking any order on the dependencies (or the order given
> by the "dependencies" field). Anyway some order will have to be
> picked eventually when these branches will be merged into Sage.

This seems like overcomplication to me.

> Annnnnnd I may have missed your explanatin Keshav, but I still don't
> get why "git log the_branch ^master ^dependency1 ^dependency2 ^
> dependency3" wouldn't make it. Looks good to me O_o

As a *log*, it is fine. What's wrong with it is that it doesn't have a
single base commit against which to *diff* the branch head to produce a
diff, which is what my question to the list was about. To repeat the
diagram I drew earlier:

master
| .---------------B
| / `-------------.
O< .---`--C
| \ /
| `---------------A

Your log would show these commits:

`-------------.
.---`--C
/

So where is the diff going to come from now?

The best solution we came up with in March (as far as I remember) was
that we take the dangling ends there, try to merge them, and then
provide a diff between that merge and C, assuming the merge succeeds. I
find that rather unsatisfactory because we're showing a diff against a
commit that was made automatically and no human being ever intentionally
committed into the repository. So the < side of the diff will be
showing a set of files that potentially nobody had ever checked out on
their machine, ever.

Furthermore, walking up the history, the developer of the branch has
already manually merged together those dangling ends by the time you
reach the branch head at C. If your automatic merge of the ends matches
in its mechanical details what the developer did manually (as may happen
thanks to git rerere cache or whatever), then your diff purposely
excludes some part of the changes that the developer made on the ticket,
namely some merge conflict resolution. This is not what you want to do
when the purpose is to present for review an artifact which represents
the work done on and changes introduced by the ticket.


Note how the commits selected simply by `C ^master` (or more simply
`master..C`) don't have this problem:

.------
/ `-------------.
< .---`--C
\ /
`---------------A

-Keshav

[Nathann Cohen]

Yoooooooooo !!

> Under Volker's scheme (and the way git is usually used in other
> projects), simply reviewing the diff is not enough for positive review.

I absolutely *never* mentionned "reviewing the diff". I just wanted to see the list of commits that belong to the ticket and to none of its dependencies. If this collection of commits is not linear I do not care much. I just want to see which volume of code I have to review when I read a ticket.

> You must also make sure that the branch can be successfully merged into
> master without conflicts.  One way to do so is to manually merge master
> into your branch.

Yepyepyep.

> Also, as soon as you start rebasing things, you lose the history of
> unification of branches.

Yeah, sorry. I meant "to merge". I'm not used to the GIT terminology yet, and "merging" in GIT is how we used to "rebase" HG patches.

> Obviously they were still compatible at the point where the branch
> (let's go back to calling it C since in my diagram O is something
> different) split off from A and B.

Definitely :-P

> It seems to me that since C is only using the commits it merged into
> itself, C shouldn't be forced to care about whatever other stuff happens
> to A and B afterwards.

I totally disagree.
First, because some things which have been done in A or B and that are needed by C may have been undone since in later patches, like a function renamed during the review. C wants to know that.
Secondly, because *tickets* (=branches) will be merged into Sage, and that A will be merged before C if C depends on A.
So if C is not compatible with all commits of its dependencies (understood as branches) we have a problem.

> If there are merge conflicts that will make the
> branch unmergeable to master once A and B get into master.

And C certainly wants to know that, in order to avoid adding code to a branch that is already incompatible with one of its dependencies, because new commits are added to the dependency.
Sage code is not reviewed instantly, and waiting for the dependencies to be merged takes time.

> If there are
> more subtle conflicts (i.e. code behavior incompatibilities), presumably
> the patchbot will notice this.  (We discussed in March the necessity for
> the patchbot to test not the branch's commit itself but the result of
> merging it into master.)

+1 to that. The problem with what you "discussed in march" is that is very quickly becomes "what we decided for everybody without asking them", unless you have the conversation on sage-devel.

> > We could probably have the patchbot tell us whenever "This ticket
> > cannot be merged on top of all its dependencies in a trivial way".
> > Possibly by taking any order on the dependencies (or the order given
> > by the "dependencies" field). Anyway some order will have to be
> > picked eventually when these branches will be merged into Sage.
>
> This seems like overcomplication to me.

Even with my earlier remark about renamed functions ?

> > Annnnnnd I may have missed your explanatin Keshav, but I still don't
> > get why "git log the_branch ^master ^dependency1 ^dependency2 ^
> > dependency3" wouldn't make it. Looks good to me O_o
>
> As a *log*, it is fine. What's wrong with it is that it doesn't have a
> single base commit against which to *diff* the branch head to produce a
> diff, which is what my question to the list was about.

I personally do not care about the diff, or what you call "reviewing the diff'. I just want to see what amount of code I have to review, get a quick idea of what it does, and not see 9 commits instead of 2 like on #15288.

So unless somebody here really wants to "review the diff" I don't know what the problem is.

Still, and insising on the fact that I am not interested on reviewing the diff, I don't see the problem with this either.

(In any case, please don't only answer to what follows but also to the log display, please ...)

The tree that you showed us can happen in Git, that is true, it is technically possible. I just don't see how it could happen, for when I work on a git branch my commits are linear except when I merge another branch into it. Still, the list of commits which are included in my branch and in none of its dependencies *is a path*. So no problem with that, even though some nodes of this path may be also linked to something outside of it *that belongs to another branch*.

Hence, I don't think that this could happen :

    A2 --- ..... (branch A)

   /

A1

   \

     C1 --- C2 

                   \

                     C3 --- C4 --- (branch C)

                   /

     C'1 --- C'2

    /

B1

    \

     B2 --- ... (branch B)

It is technically possible, but I don't see when that would make sense *for one branch (=ticket)*. I don't see when "the list of commit belonging to branch C and to none of its dependencies" would not be linear.

Which is what we would need for a diff.

Even though I am only interested in getting a correct log list.

Nathann

[Stefan van Zwam]

"I just wanted to see the list of commits that belong to the ticket and to none of its dependencies."

This.

[Andrew Mathas]

On Thursday, 24 October 2013 07:39:53 UTC+2, Keshav Kini wrote:

First of all, your suggestion is not easy to implement.  Merging A or B
into O individually is trivial (in fact, they are already merged into O
-- think about why this is true).  Merging both A and B into O means
merging A and B together.  This may be nontrivial as conflicts may
arise.  As A and B might not depend on each other, nobody is under any
obligation to have resolved these conflicts, and so such a merge will
not be possible to do automatically in general.

Thanks for the explanation, but I am afraid that I am lost. I think that I am just confused about the terminology of the new  git work-flow. I have been putting off learning about this until it stabilises. My confusion may also explain why Volker is so adamant that his way is that right way even though there seems to be a lot of opposition to his proposal.

I am thinking of A, B and C as just being patches which are applied to the master. As we are talking about git work flow, I accept that I need to change my perspective and see it your way. Please forgive me for going through this slowly but I want to understand.

Is this the following correct? You thinking of A,B and C  as git branches. To my naive view of the world, these are the same as a (usually linear) sequence of commits and hence ultimately to a final patch file. I guess that you also have some history built into your branches. So in your picture, and in the new work-flow, C is a branch which depends on the A and B branches (sorry, your fancy ascii art is just a messy disconnected graph for me:).  That is, C branched off A and B at some point in their history.

So problem that you are highlighting is that the branch C depends upon the A and B branches, but these branches may have evolved since C branched off them? As a consequence it is non-trivial to automatically merge the three branches and ceate the diff that I suggested.

If this is what you are saying then I do not really see a problem here: if it is not possible to automatically apply A and B in some order and then C then C should be automatically marked as needing work: if the patch cannot be merged then I think that C is not ready for review. To put it another way, if C depends on A and B then it does not make sense to review C unless you can already merge these patches/branches.

I think that if C is marked as being ready for review then having a functional version of this code in branch C is not enough: you also need to be able to merge the branches A, B and C.

Think about it: the code in branch C works fine, based on code that lives in the history of branches A and B. I look at this code and because it works I play with it, debug it, try to break it, and end up writing a review patch, with a few more changes, and giving it a positive review. Some time later, A and B are finalised and merged into the master at which point it is found that the code in branch C has been incompatible with A and/or B from almost the time that it branched. As a result, the whole patch/branch for C has to be rewritten and my time reviewing C was totally wasted. In the hg-world this scenario would never have happened because the three patches (honest patches now, not branches) would not have applied. It would be nice if in the git-world it cold at least be flagged as a problem it the branches are not compatible

Having said this, I am sure that the git developers have thought longer and harder about this than I have, so I am sure what Volker is proposing has merits. With my old-world view, however,  I am struggling to appreciate this.

Andrew

[Volker Braun]

On Thursday, October 24, 2013 4:55:41 AM UTC+1, Andrew Mathas wrote:

Please correct me if I am wrong, but rereading this entire thread it seems that everyone -- Nathann, Stefan, Dima, Nicolas and Jason -- is arguing for just displaying the diffs for just the current patch with the exception of Volker. I also vote for this.

Unfortunately, the truth can't be chosen by democratic vote. 

If you have never contributed a commit to a large git-using distributed project like the kernel or cyanogenmod then you you should have the humility to listen.

The "changes caused by the ticket" are the list of all commits leading to the branch head. This is what I'm trying to tell you all the time. These may or may not be part of any single ticket. If you were reasonably disciplined in how you merge branches into each other and review branches in order, then you can assign to each commit a ticket. But in general a commit does not belong to any single ticket, for example after merging branches both ways.

[Keshav Kini]

Nathann Cohen <nathan...@gmail.com> writes:

> Yoooooooooo !!
>
>> Under Volker's scheme (and the way git is usually used in other
>> projects), simply reviewing the diff is not enough for positive
> review.
>
> I absolutely *never* mentionned "reviewing the diff". I just wanted
> to see the list of commits that belong to the ticket and to none of
> its dependencies. If this collection of commits is not linear I do
> not care much. I just want to see which volume of code I have to
> review when I read a ticket.

I was responding to Andrew, not you, and he was talking about diffs.

>> It seems to me that since C is only using the commits it merged
> into
>> itself, C shouldn't be forced to care about whatever other stuff
> happens
>> to A and B afterwards.
>
> I totally disagree.
> First, because some things which have been done in A or B and that
> are needed by C may have been undone since in later patches, like a
> function renamed during the review. C wants to know that.
> Secondly, because *tickets* (=branches) will be merged into Sage, and
> that A will be merged before C if C depends on A.
> So if C is not compatible with all commits of its dependencies
> (understood as branches) we have a problem.

The developer of C should certainly pay attention to what A and B are
doing, but that's different from trying to mess around with the history
graph. The fundamental issue here is that git allows non-linear
history, and non-linear history is more complicated. Less
synchronization is forced upon you than when using patch series, but
that also means you need to pay more attention to what your dependencies
are doing.

>> If there are
>> more subtle conflicts (i.e. code behavior incompatibilities),
> presumably
>> the patchbot will notice this.  (We discussed in March the
> necessity for
>> the patchbot to test not the branch's commit itself but the result
> of
>> merging it into master.)
>
> +1 to that. The problem with what you "discussed in march" is that is
> very quickly becomes "what we decided for everybody without asking
> them", unless you have the conversation on sage-devel.

I mention the discussions in March not because I think whatever was
discussed and agreed upon by people at the Sage Git Days in March is
uncontrovertible law but because it was an occasion upon which a group
of people containing both Sage experts and Git experts sat down together
for a full week and thought pretty hard about a lot of the potential
issues that might arise during the git transition, and I think ideas
that came out of that discussion are particularly worth listening to,
not necessarily compared to what other people say, but compared to what
I myself say without mentioning discussions in March.

>> > We could probably have the patchbot tell us whenever "This ticket
>> > cannot be merged on top of all its dependencies in a trivial
> way".
>> > Possibly by taking any order on the dependencies (or the order
> given
>> > by the "dependencies" field). Anyway some order will have to be
>> > picked eventually when these branches will be merged into Sage.
>>
>> This seems like overcomplication to me.
>
> Even with my earlier remark about renamed functions ?

OK, sure, why not. You can have the patchbot do whatever you want, if
you can define it properly and implement it. It can't hurt and can only
help.

>> > Annnnnnd I may have missed your explanatin Keshav, but I still
> don't
>> > get why "git log the_branch ^master ^dependency1 ^dependency2 ^
>> > dependency3" wouldn't make it. Looks good to me O_o
>>
>> As a *log*, it is fine. What's wrong with it is that it doesn't
> have a
>> single base commit against which to *diff* the branch head to
> produce a
>> diff, which is what my question to the list was about.
>
> I personally do not care about the diff, or what you call "reviewing
> the diff'. I just want to see what amount of code I have to review,
> get a quick idea of what it does, and not see 9 commits instead of 2
> like on #15288.
>
> So unless somebody here really wants to "review the diff" I don't
> know what the problem is.

OK, fine, but again, I was talking to Andrew. Maybe he was not talking
about the diff either, and I misunderstood the conversation, in which
case I apologize.

You have apparently not looked at my diagram, as it clearly shows a list
of commits belonging to a branch C and to none of its dependencies which
is not linear, and yet which can arise very naturally. It is also very
different from the diagram you drew above. Perhaps you are viewing my
mails in the variable-width UI provided by Google Groups rather than a
traditional fixed-width mail client, and thus cannot see my diagram
correctly. In that case, try this link instead:

http://thread.gmane.org/gmane.comp.mathematics.sage.gitworkflow/868/focus=897

In any case, your diagram (while certainly more far-fetched than mine)
is still perfectly possible. C1, C2, C'1, and C'2 do not need to be made
by you. They could be parts of other branches that have since been
discarded by their authors, or worse, rebased so that they now
correspond to the branches marked A and B. Remember, authors of other
tickets are under no obligation to keep their branch strictly ff-only.

As an aside, I suspect a lot of the problems many of you are envisioning
will be minimized by the existence of a master branch into which
positively reviewed code is constantly being merged.

-Keshav

[Stefan van Zwam]

Ok, I'll stop theorizing, and expect that things will work out just fine once we actually start using this stuff.

Let me look at Nathann's example string of tickets:

http://trac.sagemath.org/ticket/15107

http://trac.sagemath.org/ticket/15285

http://trac.sagemath.org/ticket/15286

http://trac.sagemath.org/ticket/15287

http://trac.sagemath.org/ticket/15310

Let's start with 15107. That's easy enough. I click u/ncohen/15107, I get the differences compared to the master branch, and can fire off a review.

Now, let's assume that 15107 has positive review, but is not yet merged. My next task would be 15285. If I do the same as before, I see all changes from 15107 PLUS the new ones. But there's a way out: I can click on (Commits), and from the graph it's easy to see which diff I should investigate.

This goes well for a while. Now assume all of 15107 .. 15287 have positive review, but are not yet merged. I'm looking at 15310 now, and I'm lost. Neither option from before seems to give me a clear view of what this ticket changed on top of the old ones... What do I do?

(By the way, what this shows is that I'm doing my reviewing from the Trac website.)

[Stefan van Zwam]

As an aside, I suspect a lot of the problems many of you are envisioning
will be minimized by the existence of a master branch into which
positively reviewed code is constantly being merged.

Yes. That would certainly deal with Nathan's example. But how realistic is this? It requires a release manager who is VERY much on top of things. And wouldn't it still make it difficult to go on a reviewing spree that dealt with Nathann's tickets all in one afternoon? 

[Keshav Kini]

OK, I see. I don't want to get too engaged in this conversation so
forgive me if I don't take the time to explain git concepts to you.

But in short: a branch is not a linear sequence of commits. A branch is
a pointer to a commit in the history graph. A commit is not a patch. A
commit is a snapshot of the state of the codebase. A patch is a diff.
A diff is a difference between two commits.

I suggest you read this article:

http://gitolite.com/gcs/

It explains the low-level mechanics of git, which I think should clarify
the high-level picture as well.

As for my ASCII art, perhaps you can see it better on Gmane:

http://thread.gmane.org/gmane.comp.mathematics.sage.gitworkflow/868/focus=892

> So problem that you are highlighting is that the branch C depends
> upon the A and B branches, but these branches may have evolved since
> C branched off them? As a consequence it is non-trivial to
> automatically merge the three branches and ceate the diff that I
> suggested.

No, the main problem is that C is depending upon two non-linearized
branches A and B. The fact that B has evolved beyond the point where C
merged B into itself is simply a further complication.

> If this is what you are saying then I do not really see a problem
> here: if it is not possible to automatically apply A and B in some
> order and then C then C should be automatically marked as needing
> work: if the patch cannot be merged then I think that C is not ready
> for review. To put it another way, if C depends on A and B then it
> does not make sense to review C unless you can already merge these
> patches/branches.

So what you're saying is that when a ticket is not ready for review the
trac ticket shouldn't display a diff? Or perhaps that it should display
different kinds of diffs depending on how reviewable some system decides
the ticket is?

Perhaps we are thinking of different things. Personally I would always
want to see a diff on the ticket, and one that has a simple to
understand meaning behind it, regardless of whether it was ready for
review.

> I think that if C is marked as being ready for review then having a
> functional version of this code in branch C is not enough: you also
> need to be able to merge the branches A, B and C.
>
> Think about it: the code in branch C works fine, based on code that
> lives in the history of branches A and B. I look at this code and
> because it works I play with it, debug it, try to break it, and end
> up writing a review patch, with a few more changes, and giving it a
> positive review. Some time later, A and B are finalised and merged
> into the master at which point it is found that the code in branch C
> has been incompatible with A and/or B from almost the time that it
> branched. As a result, the whole patch/branch for C has to be
> rewritten and my time reviewing C was totally wasted. In the hg-world
> this scenario would never have happened because the three patches
> (honest patches now, not branches) would not have applied. It would
> be nice if in the git-world it cold at least be flagged as a problem
> it the branches are not compatible

I agree with most of this but I don't see the relevance to diff views.

-Keshav

[Keshav Kini]

Perhaps, I don't know. Maybe it's conceivable that we might have
multiple people with the ability to merge things into master in the
future. One main reason to have a single-point-of-failure release
manager in the old workflow is that merging tickets involves munging
patch files in a directory on boxen to create "development releases",
rather than merging branches into a branch on a public repository in
real time, and the former is much harder to share between multiple
people than the latter. Of course, there are other reasons to have a
single-point-of-failure release manager as well, I'm sure.

-Keshav

[Volker Braun]

On Wednesday, October 23, 2013 11:26:52 PM UTC+1, Nicolas M. Thiéry wrote:

But this is an important
enough operation that it should not only be possible, but trivial to
get it, in particular from trac (I am happy with whatever warning you
will see fit when you open that view).

I tend to disagree, you want to do something that is inherently against the grain of good development practices. It should be difficult enough so that only people with a solid grasp of what it entails can do it. We definitely shouldn't script bad practices into a trac plugin to teach more people how to follow bad practices.

What you really are asking for is a (post-commit) review style where you don't review branches but individual commits. A branch is just a set of commits, but a particular set: all parents of the branch head. Reviewing commits individually gives you more freedom to break up reviews, at the expense of having more individual commits to click through and more bookkeeping. I'm not opposed to that, but then we should do it right. Hacking a trac plugin to review not-so-clearly defined sets of commits and without proper bookkeeping of commits would just suck. There are various dedicated tools to do that, for example barkeep (https://github.com/ooyala/barkeep) which also has a handy comparison chart to other such systems (https://github.com/ooyala/barkeep/wiki/Comparing-Barkeep-to-other-code-review-tools). But as far as I know, trac doesn't have anything to offer in that department.

But before going that route, I would rather get started with sage-on-git and try out the (perhaps deceptively) simpler workflow where you just review branches. Maybe it turns out to that it is too slow. Maybe it is too overwhelming to be shown diffs that include other tickets. We can always switch our review style, we can even just switch the review style just for sage-combinat or any other subproject. We are hardly the first ones to review changes from git repositories, and there is lots to be learned from other project's workflows. 

[Nathann Cohen]

I am answering this thread again, because I would like to review #15278 right now, and I have NO F*** CLUE of what is implemented in the patch and what is implemented elsewhere.

And I really have no idea how I am supposed to review that. Thus, I repeat that it would be PRETTY COOL to be able to see what is implemented in a ticket and that has not already been reviewed elsewhere. And it seemed that everybody (but Volker, but that's still a majority) agreed.

Nathann

On Tuesday, October 22, 2013 6:56:39 PM UTC+2, Nathann Cohen wrote:

Hellooooooo everybody !!!

I coded a bit too much in the last days, and created several tickets
depending on each other. The last of them (#15310) only contains three
commits (those who message begins with "Wilson's construction"), but
many more appear on the page. Wouldn't it be better if we only saw on
that page the list of patches that are related to THIS ticket ? All
the others will have been reviewed in the dependencies, and it's hard
to tell if a ticket contains a lot of code or not when you see the
ticket's code mixed with the code from its dependencies.

I just looked at the code of trac-githooks, and it looks like there is
not much to change, even though it's hard for me to .... actually try
anything :-P

The "log_table" function defined in
trac-githooks/post-receive.d/01-trac_branch features a "ignore"
argument (which contains master by default), and all that would be
needed would be to add there the reference toward the content of the
"dependencies" field of the trac ticket. What do you think ? And is
there any way for me to do the job rather than posting here ?

Have fuuuuuuuuuuuuun ! And thanks ;-)

Nathann

[Volker Braun]

The comits that are currently associated to on #15278 are:

git diff 2fc8a772ee12fce7ac6abc4ecf9916f4746f5ee2 ^6cf1fad162f761dd9a0191a7f8d862cb6300583b ^020cc82f8f9ba8bc8295ac1e79c396a12a4a5fd8

[Nathann Cohen]

> The comits that are currently associated to on #15278 are:
>
> git diff 2fc8a772ee12fce7ac6abc4ecf9916f4746f5ee2 ^6cf1fad162f761dd9a0191a7f8d862cb6300583b ^020cc82f8f9ba8bc8295ac1e79c396a12a4a5fd8

Okay, so we will post a message here whenever we wonder ?

Nathann

[Volker Braun]

On Monday, December 9, 2013 11:59:02 AM UTC, Nathann Cohen wrote:

Okay, so we will post a message here whenever we wonder ?

Viable alternatives are 1) learn how to exclude stuff from git log / git diff and 2) finish the git transitions asap so positively reviewed tickets can be merged.

 

[Nathann Cohen]

> Viable alternatives are 1) learn how to exclude stuff from git log / git
> diff and 2) finish the git transitions asap so positively reviewed tickets
> can be merged.

And we will also make it easier to see this on the trac server,
because right now reviewing stuff is complicated. And because it looks
like we all agree on this.

Nathann

[Volker Braun]

On Monday, December 9, 2013 12:07:04 PM UTC, Nathann Cohen wrote:

And we will also make it easier to see this on the trac server,
because right now reviewing stuff is complicated.

Once the dependency is merged its not shown in the commit list on the trac server, so this is solved as a by-product of 2)

 

[Nathann Cohen]

> Once the dependency is merged its not shown in the commit list on the trac
> server, so this is solved as a by-product of 2)

Yep. So we have to solve the problem, if only for the time before it is merged.

Nathann

[Andrew]

Correct me if I am wrong (I'm sure some one will:), but I think that this is already possible from trac: click on the "commits" link in the branch field to see the complete list of commit for the ticket. Now select the two commit that you want to compare and then click "view changes" to see the diffs between them.

Andrew

Andrew

[Volker Braun]

That lets Nathann exclude one dependency, but he wanted to exclude two of them.