How to review issues on GitHub?

[Andrey Novoseltsev]

Hello,

So - what do I need to do to review an issue on GitHub? Specifically
this one:
https://github.com/sagemath/sagenb/pull/97

I've followed Development workflow and got my own fork and local copy
of the main notebook, but I am not sure what to do to get John's
changes for testing.

I am also not sure how the comment system works on GitHub - I've asked
a question on the issue, but didn't get any notification of it as
usually happens with trac. Did John get a notification or nobody will
see my question unless they go to the issue themselves?

Thank you!
Andrey

[P Purkayastha]

You need to add jhpalmieri's branch too. Like so:

1. first add the remote sagenb
$ git remote add jhpalmieri git://github.com/jhpalmieri/sagenb.git

2. Next, you can either cherry-pick the changes you want, or change
temporarily to the mathmode branch.
$ git checkout jhpalmieri/mathmode

3. Now, create your own copy of this remote branch. Let's call your own
branch jh-mathmode.
$ git checkout -b jh-mathmode

4. Now you can test, do modifications, etc.

5. When you want to return back to the default branch, just do
$ git checkout master



As for notifications, you will get a notification at your email, and
also at github, if someone replies or comments. Whoever prior to you
participated in that pull request will have got a notification email.

[P Purkayastha]

On 10/21/2012 11:03 PM, P Purkayastha wrote:
> On 10/21/2012 10:45 PM, Andrey Novoseltsev wrote:
>> Hello,
>>
>> So - what do I need to do to review an issue on GitHub? Specifically
>> this one:
>> https://github.com/sagemath/sagenb/pull/97
>>
>> I've followed Development workflow and got my own fork and local copy
>> of the main notebook, but I am not sure what to do to get John's
>> changes for testing.
>>
>> I am also not sure how the comment system works on GitHub - I've asked
>> a question on the issue, but didn't get any notification of it as
>> usually happens with trac. Did John get a notification or nobody will
>> see my question unless they go to the issue themselves?
>>
>> Thank you!
>> Andrey
>>
>
> You need to add jhpalmieri's branch too. Like so:
>
> 1. first add the remote sagenb
> $ git remote add jhpalmieri git://github.com/jhpalmieri/sagenb.git

Sorry, I forgot a step in between:

$ git fetch jhpalmieri

[Andrey Novoseltsev]

Thank you, that worked!

Is this a usual workflow with git repositories? It seems that I had to
download 7M+ to test just a few lines, not like that was a big deal on
a 20Mbit connection, but if it was the same with 300M+ Sage, reviewing
patches would get annoying on any connection...

Andrey

[Andrey Novoseltsev]

OK, what do I do now that I like the change? I.e. "positive review"?

[P Purkayastha]

On 10/22/2012 04:48 AM, Andrey Novoseltsev wrote:
> OK, what do I do now that I like the change? I.e. "positive review"?
>

Yes. Just comment on the ticket :)

[P Purkayastha]

On 10/22/2012 12:41 AM, Andrey Novoseltsev wrote:
> Thank you, that worked!
>
> Is this a usual workflow with git repositories? It seems that I had to
> download 7M+ to test just a few lines, not like that was a big deal on
> a 20Mbit connection, but if it was the same with 300M+ Sage, reviewing
> patches would get annoying on any connection...
>
> Andrey

I think you downloaded 7M probably because you didn't have all the
files. AFAIK, this shouldn't happen every time. Maybe kini knows better.

The other option is to go directly to the code and comment there. You
can go here, (in this particular case)
https://github.com/sagemath/sagenb/pull/97/files

On the left of every line you will see the option to add a comment to
that particular line. This you can do if you have anything specific to
comment on.

[Keshav Kini]

P Purkayastha <ppu...@gmail.com> writes:
> On 10/22/2012 12:41 AM, Andrey Novoseltsev wrote:
>> Thank you, that worked!
>>
>> Is this a usual workflow with git repositories? It seems that I had to
>> download 7M+ to test just a few lines, not like that was a big deal on
>> a 20Mbit connection, but if it was the same with 300M+ Sage, reviewing
>> patches would get annoying on any connection...
>>
>> Andrey
>
> I think you downloaded 7M probably because you didn't have all the
> files. AFAIK, this shouldn't happen every time. Maybe kini knows
> better.

The 7M is because you did `git fetch jhpalmieri`, which not only fetches
the particular branch you wanted, but all other things that jhpalmieri
had on his branch which your local repository didn't already "know
about". Possibly there were some large things in that category. Note
that this doesn't have to be stuff that only jhpalmieri had - it could
also be new stuff in the main sagemath master branch which you hadn't
fetched but jhpalmieri had, and which jhpalmieri had based one or more
of his branches on.

If you want to avoid this and only get the commits from a particular
branch, you should do `git fetch jhpalmieri A:B` where A is the name of
jhpalmieri's branch on his repository, and B is the name of your branch
which you want to be updated to match jhpalmieri's branch (B doesn't
need to exist yet).

Personally I'd just do `git fetch jhpalmieri` and not worry too much
about network traffic - it should only be proportional to the amount of
development that jhpalmieri is doing, not to the size of Sage. Plus, it
gets you the handy jhpalmieri/* "remote tracking branches", which `git
fetch jhpalmieri A:B` doesn't, I think. And really, 7MB is normally
pretty huge IME. I guess either your master was way behind the upstream
master, or you missed some large commit like a javascript library update
or something...

-Keshav

----
Join us in #sagemath on irc.freenode.net !

[Keshav Kini]

P Purkayastha <ppu...@gmail.com> writes:
> On 10/22/2012 12:41 AM, Andrey Novoseltsev wrote:
>> Thank you, that worked!
>>
>> Is this a usual workflow with git repositories? It seems that I had to
>> download 7M+ to test just a few lines, not like that was a big deal on
>> a 20Mbit connection, but if it was the same with 300M+ Sage, reviewing
>> patches would get annoying on any connection...
>>
>> Andrey
>
> I think you downloaded 7M probably because you didn't have all the
> files. AFAIK, this shouldn't happen every time. Maybe kini knows
> better.

[Keshav Kini]

Whoops, sorry for the double post.

-Keshav