a few more trivial patches

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

a few more trivial patches

Anthony Foiani
Greetings --

I have two small patches which you might want to consider integrating.

(And given that I can't get git to do what I want, you probably want
to just cherry-pick these, as I suspect I've completely destroyed my
repo history...)

https://github.com/tkil/OpenSC/commit/0c4a2e0c4063f31bc41c34e45869b9a9e7ca41d7
This uses "dir local" settings to configure Emacs indentation correctly.

https://github.com/tkil/OpenSC/commit/599bd1e6c906af63eb379c866076f98a91654cb2
I spotted an inconsistency in how the option argument pointers were
initialized; this fixes it (to make it more consistent).

Best Regards,
Anthony Foiani
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: a few more trivial patches

Ludovic Rousseau
2012/12/8 Anthony Foiani <[hidden email]>:
> Greetings --
>
> I have two small patches which you might want to consider integrating.
>
> (And given that I can't get git to do what I want, you probably want
> to just cherry-pick these, as I suspect I've completely destroyed my
> repo history...)

You should rebase your patches above OpenSC/OpenSC master.

> https://github.com/tkil/OpenSC/commit/0c4a2e0c4063f31bc41c34e45869b9a9e7ca41d7
> This uses "dir local" settings to configure Emacs indentation correctly.

I don't think that an Emacs configuration file should be added to the
OpenSC source code.
You should keep this change in your own branch.

> https://github.com/tkil/OpenSC/commit/599bd1e6c906af63eb379c866076f98a91654cb2
> I spotted an inconsistency in how the option argument pointers were
> initialized; this fixes it (to make it more consistent).

Not a bug but the code would be nicer.
Can you create a branch from OpenSC/OpenSC master with only this patch
and ask for a Pull Request?

Bye

--
 Dr. Ludovic Rousseau
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: a few more trivial patches

Anthony Foiani
Ludovic, greetings --

On Sun, Dec 9, 2012 at 7:19 AM, Ludovic Rousseau
<[hidden email]> wrote:

>
> 2012/12/8 Anthony Foiani <[hidden email]>:
> > Greetings --
> >
> > I have two small patches which you might want to consider integrating.
> >
> > (And given that I can't get git to do what I want, you probably want
> > to just cherry-pick these, as I suspect I've completely destroyed my
> > repo history...)
>
> You should rebase your patches above OpenSC/OpenSC master.

Ok, but pardon my git ignorance: I thought that one should never
rebase a tree that will be published and pulled from?  Or only if it's
published and someone tries to *base a new tree* off of it?

>
> > https://github.com/tkil/OpenSC/commit/0c4a2e0c4063f31bc41c34e45869b9a9e7ca41d7
> > This uses "dir local" settings to configure Emacs indentation correctly.
>
> I don't think that an Emacs configuration file should be added to the
> OpenSC source code.

Hm. Why not?  It would ensure that emacs users have their style set
appropriately for this project, and shouldn't affect anyone else in
any way.

In my own use case, I work on 3-4 projects in the same emacs session,
and each one has different indentation settings.  dir-local settings
seem the easiest way to assign a style per directory (tree).

> You should keep this change in your own branch.

And for my second question of git ignorance: how can I maintain "my
own branch", when merging upstream into a branch is discouraged?  Or
do I misunderstand the tone of the log comments when trying to check
in such a merge?

>
> > https://github.com/tkil/OpenSC/commit/599bd1e6c906af63eb379c866076f98a91654cb2
> > I spotted an inconsistency in how the option argument pointers were
> > initialized; this fixes it (to make it more consistent).
>
> Not a bug but the code would be nicer.

For whatever it's worth, my understanding is that uninitialized global
variables are actually allocated as a part of program runtime, and are
initialized to zero at that point.  *Initialized* global variables,
however, are stored in the binary itself, even if the initializer is
zero.

So as a matter of style, it might be better to leave all those
pointers uninitialized.  (This was a big stink on the linux-kernel
mailing list a few years back.)

On the other hand, I don't know if this behavior is true across all
platforms, and the space/time cost in this case is trivial.

> Can you create a branch from OpenSC/OpenSC master with only this patch
> and ask for a Pull Request?

I'll try.  :)  Every time I try to use git for anything fancier than
an svn-replacement, I seem to get burned...

In this case, it looks like I'll have to fork the OpenSC version
(instead of the CardContact version), then branch in my new fork,
commit this change, and then request a pull of my new branch on the
new fork?  (Not complaining about amount of work, just trying to make
sure I have the flow correct.)

Many thanks,
Anthony Foiani
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: a few more trivial patches

Frank Morgner
Hi!

> > > https://github.com/tkil/OpenSC/commit/0c4a2e0c4063f31bc41c34e45869b9a9e7ca41d7
> > > This uses "dir local" settings to configure Emacs indentation correctly.
> >
> > I don't think that an Emacs configuration file should be added to the
> > OpenSC source code.
>
> Hm. Why not?  It would ensure that emacs users have their style set
> appropriately for this project, and shouldn't affect anyone else in
> any way.
>
> In my own use case, I work on 3-4 projects in the same emacs session,
> and each one has different indentation settings.  dir-local settings
> seem the easiest way to assign a style per directory (tree).
Your editor should be intelligent enough to guess the right settings.
For vim I recently found this plugin
http://www.vim.org/scripts/script.php?script_id=4251

--
Frank Morgner

Virtual Smart Card Architecture http://vsmartcard.sourceforge.net
OpenPACE                        http://openpace.sourceforge.net
IFD Handler for libnfc Devices  http://sourceforge.net/projects/ifdnfc

_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel

attachment0 (501 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: a few more trivial patches

Anthony Foiani
Frank, greetings --

On Mon, Dec 10, 2012 at 12:23 AM, Frank Morgner
<[hidden email]> wrote:
> Your editor should be intelligent enough to guess the right settings.

Why have the editor guess, when it could know the right answer?

> For vim I recently found this plugin
> http://www.vim.org/scripts/script.php?script_id=4251

There is a similar feature for emacs (c-guess and friends).

I am impressed by their cleverness, but it seems better to specify
than to guess (with the chance of guessing wrongly).

And it's not just tabs-vs-spaces, but also a human judgement as to
whether those tabs are intended to represent 4 spaces or 8.

No good answers.  I just offered the directory-wide settings for emacs
as a way to reduce my pain, and possibly to avoid patch submissions
with incorrect indentation.

Anyway.  If Ludovic doesn't want it around, then that's fine by me;
I'll find another way to make sure that I use the correct style.  :)

Best regards,
Anthony Foiani
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: a few more trivial patches

Ludovic Rousseau
In reply to this post by Anthony Foiani
2012/12/10 Anthony Foiani <[hidden email]>:

> Ludovic, greetings --
>
> On Sun, Dec 9, 2012 at 7:19 AM, Ludovic Rousseau
> <[hidden email]> wrote:
>>
>> 2012/12/8 Anthony Foiani <[hidden email]>:
>> > Greetings --
>> >
>> > I have two small patches which you might want to consider integrating.
>> >
>> > (And given that I can't get git to do what I want, you probably want
>> > to just cherry-pick these, as I suspect I've completely destroyed my
>> > repo history...)
>>
>> You should rebase your patches above OpenSC/OpenSC master.
>
> Ok, but pardon my git ignorance: I thought that one should never
> rebase a tree that will be published and pulled from?  Or only if it's
> published and someone tries to *base a new tree* off of it?

That is what I thought also.
But it is far easier to review a patch when the history is clean.

>> > https://github.com/tkil/OpenSC/commit/0c4a2e0c4063f31bc41c34e45869b9a9e7ca41d7
>> > This uses "dir local" settings to configure Emacs indentation correctly.
>>
>> I don't think that an Emacs configuration file should be added to the
>> OpenSC source code.
>
> Hm. Why not?  It would ensure that emacs users have their style set
> appropriately for this project, and shouldn't affect anyone else in
> any way.
>
> In my own use case, I work on 3-4 projects in the same emacs session,
> and each one has different indentation settings.  dir-local settings
> seem the easiest way to assign a style per directory (tree).
>
>> You should keep this change in your own branch.
>
> And for my second question of git ignorance: how can I maintain "my
> own branch", when merging upstream into a branch is discouraged?  Or
> do I misunderstand the tone of the log comments when trying to check
> in such a merge?

Or just keep the file.dir-locals.el out of git.

I have no objection to add this file. I do not use Emacs myself.

I see it can help code quality so unless someone objects I will merge
it upstream.
Please submit a pull request.

>> > https://github.com/tkil/OpenSC/commit/599bd1e6c906af63eb379c866076f98a91654cb2
>> > I spotted an inconsistency in how the option argument pointers were
>> > initialized; this fixes it (to make it more consistent).
>>
>> Not a bug but the code would be nicer.
>
> For whatever it's worth, my understanding is that uninitialized global
> variables are actually allocated as a part of program runtime, and are
> initialized to zero at that point.  *Initialized* global variables,
> however, are stored in the binary itself, even if the initializer is
> zero.
>
> So as a matter of style, it might be better to leave all those
> pointers uninitialized.  (This was a big stink on the linux-kernel
> mailing list a few years back.)
>
> On the other hand, I don't know if this behavior is true across all
> platforms, and the space/time cost in this case is trivial.
>
>> Can you create a branch from OpenSC/OpenSC master with only this patch
>> and ask for a Pull Request?
>
> I'll try.  :)  Every time I try to use git for anything fancier than
> an svn-replacement, I seem to get burned...
>
> In this case, it looks like I'll have to fork the OpenSC version
> (instead of the CardContact version), then branch in my new fork,
> commit this change, and then request a pull of my new branch on the
> new fork?  (Not complaining about amount of work, just trying to make
> sure I have the flow correct.)

Now merged upstream.

Merging a pull request from github adds a "merge pull request" commit.
The history is then not very nice (linear) but I don't know a better
way using the github web interface.

Thanks

--
 Dr. Ludovic Rousseau
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: a few more trivial patches

Greg Troxel
In reply to this post by Anthony Foiani

Anthony Foiani <[hidden email]> writes:

> Ludovic, greetings --
>
> On Sun, Dec 9, 2012 at 7:19 AM, Ludovic Rousseau
> <[hidden email]> wrote:
>>
>> 2012/12/8 Anthony Foiani <[hidden email]>:
>> > Greetings --
>> >
>> > I have two small patches which you might want to consider integrating.
>> >
>> > (And given that I can't get git to do what I want, you probably want
>> > to just cherry-pick these, as I suspect I've completely destroyed my
>> > repo history...)
>>
>> You should rebase your patches above OpenSC/OpenSC master.
>
> Ok, but pardon my git ignorance: I thought that one should never
> rebase a tree that will be published and pulled from?  Or only if it's
> published and someone tries to *base a new tree* off of it?
This is somewhat controversial, but from my experience in both open
source and large private projects, the key issue is not to rebase
branches that other people have made commits on top of, or merged into
other branches.  I find that it's helpful to rebase branches proposed
for merging.  The point for me is not so much to have them based off
recent master to avoid a merge commit, but to produce the clean series
of commits that would have appeared had there been no mistakes.

Achieving the goal of not rebasing branches others have derived commits
From can be accomplished by

  not rebasing published branches

or

  having an understanding within the project that branches should not be
  cross-merged, so that rebasing them is still safe.  Even if multitple
  people commit to a branch, with a little coordination this is not a
  big deal.


_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel

attachment0 (200 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: a few more trivial patches

Viktor Tarasov-3
Le 10/12/2012 17:10, Greg Troxel a écrit :

> Anthony Foiani <[hidden email]> writes:
>
>> Ludovic, greetings --
>>
>> On Sun, Dec 9, 2012 at 7:19 AM, Ludovic Rousseau
>> <[hidden email]> wrote:
>>> 2012/12/8 Anthony Foiani <[hidden email]>:
>>>> Greetings --
>>>>
>>>> I have two small patches which you might want to consider integrating.
>>>>
>>>> (And given that I can't get git to do what I want, you probably want
>>>> to just cherry-pick these, as I suspect I've completely destroyed my
>>>> repo history...)
>>> You should rebase your patches above OpenSC/OpenSC master.
>> Ok, but pardon my git ignorance: I thought that one should never
>> rebase a tree that will be published and pulled from?  Or only if it's
>> published and someone tries to *base a new tree* off of it?
> This is somewhat controversial, but from my experience in both open
> source and large private projects, the key issue is not to rebase
> branches that other people have made commits on top of, or merged into
> other branches.  I find that it's helpful to rebase branches proposed
> for merging.  The point for me is not so much to have them based off
> recent master to avoid a merge commit, but to produce the clean series
> of commits that would have appeared had there been no mistakes.
>
> Achieving the goal of not rebasing branches others have derived commits
> From can be accomplished by
>
>   not rebasing published branches
>
> or
>
>   having an understanding within the project that branches should not be
>   cross-merged, so that rebasing them is still safe.  Even if multitple
>   people commit to a branch, with a little coordination this is not a
>   big deal.


I also vote for rebase of the feature branch before merging it to the master branch.
If this procedure seems annoying, then use cherry-pick, especially when it's going about the minor changes.

If no objections, I will rebase two last commits of the current 'master'.


Kind regards,
Viktor.

_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: a few more trivial patches

Peter Stuge-4
In reply to this post by Ludovic Rousseau
Ludovic Rousseau wrote:
> Merging a pull request from github adds a "merge pull request" commit.
> The history is then not very nice (linear) but I don't know a better
> way using the github web interface.

It isn't neccessary to use the github web interface just because
github is used to host the repository.

It works just as well to pull from any remotes and create
fast-forward merges.

But of course that will then work around the pull request workflow
that github enforces.


Greg Troxel wrote:

> >> You should rebase your patches above OpenSC/OpenSC master.
> >
> > Ok, but pardon my git ignorance: I thought that one should never
> > rebase a tree that will be published and pulled from?  Or only if it's
> > published and someone tries to *base a new tree* off of it?
>
> This is somewhat controversial, but from my experience in both open
> source and large private projects, the key issue is not to rebase
> branches that other people have made commits on top of, or merged into
> other branches.

I disagree that rebasing is controversial. This is just a tool.

Rewriting history is never a problem, as long as everyone who is
working together wants to understand how repositories work and how
easy it is to resync with rewritten branches.

Given a local branch foobar that tracks contributor/master here's
what to do when the contributor has rewritten her master:

git checkout -b orig_contrib_master contributor/master && \
  git fetch contributor && \
  git rebase --onto contributor/master orig_contrib_master foobar

Clean up after the rebase is done: git branch -D orig_contrib_master

Since rebase of the local branch is required, of course it is
important that the mechanics of rebase are well understood.


> I find that it's helpful to rebase branches proposed for merging.

There are arguments both ways. I like the look of linear history. On
the other hand, depending on the development model, merges may be a
more accurate representation of the code history.


Viktor Tarasov wrote:
> I also vote for rebase of the feature branch before merging it to
> the master branch.

Note that any testing and review of the branch should happen *after*
that rebase, to avoid a lot of wasted effort.


> If this procedure seems annoying, then use cherry-pick, especially
> when it's going about the minor changes.

cherry-pick is almost the only thing that rebase does. It's often
easier to just use interactive rebase to do the picking.


//Peter
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel