Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH] pending-author.t fallback to the last commit #14906

Closed
p5pRT opened this issue Sep 14, 2015 · 13 comments
Closed

[PATCH] pending-author.t fallback to the last commit #14906

p5pRT opened this issue Sep 14, 2015 · 13 comments

Comments

@p5pRT
Copy link

p5pRT commented Sep 14, 2015

Migrated from rt.perl.org#126057 (status was 'rejected')

Searchable as RT126057$

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2015

From @atoomic

Created by @atoomic

pending-author.t tries to detect if the current
author is unknown but when your branch is clean
and you've commited your work, no check is performed.

The documentation suggests to generate your patches
after running 'make test'. (and patches are ignored
by git).

The earlier we detect an unknown author the better it's
and this should happen on the submitter side if possible.

Perl Info

Flags:
    category=core
    severity=low
    Type=Patch
    PatchStatus=HasPatch

Site configuration information for perl 5.23.3:

Configured by nicolas at Fri Sep 11 07:53:54 CDT 2015.

Summary of my perl5 (revision 5 version 23 subversion 3) configuration:
  Commit id: 2efb8b4b644d5f3c28974a8f577081b4142decd2
  Platform:
    osname=darwin, osvers=14.5.0, archname=darwin-2level
    uname='darwin nicolass-macbook.local 14.5.0 darwin kernel version
14.5.0: wed jul 29 02:26:53 pdt 2015; root:xnu-2782.40.9~1release_x86_64
x86_64 '
    config_args='-des -Dusedevel'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-fno-common -DPERL_DARWIN -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include',
    optimize='-O3',
    cppflags='-fno-common -DPERL_DARWIN -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include'
    ccversion='', gccversion='4.2.1 Compatible Apple LLVM 6.1.0
(clang-602.0.53)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678,
doublekind=3
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16,
longdblkind=3
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='env MACOSX_DEPLOYMENT_TARGET=10.3 cc', ldflags ='
-fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/local/lib
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/6.1.0/lib
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib
/usr/lib
    libs=-lpthread -ldbm -ldl -lm -lutil -lc
    perllibs=-lpthread -ldl -lm -lutil -lc
    libc=, so=dylib, useshrplib=false, libperl=libperl.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=bundle, d_dlsymun=undef, ccdlflags=' '
    cccdlflags=' ', lddlflags=' -bundle -undefined dynamic_lookup
-L/usr/local/lib -fstack-protector-strong'



@INC for perl 5.23.3:
    lib
    /Users/nicolas/.dotfiles/perl-must-have/lib
    /Users/nicolas/perl5/lib/perl5/
    /usr/local/lib/perl5/site_perl/5.23.3/darwin-2level
    /usr/local/lib/perl5/site_perl/5.23.3
    /usr/local/lib/perl5/5.23.3/darwin-2level
    /usr/local/lib/perl5/5.23.3
    .


Environment for perl 5.23.3:
    DYLD_LIBRARY_PATH (unset)
    HOME=/Users/nicolas
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=...
    PERL5DB=use Devel::NYTProf

PERL5LIB=/Users/nicolas/.dotfiles/perl-must-have/lib:/Users/nicolas/perl5/lib/perl5/
    PERLBREW_BASHRC_VERSION=0.73
    PERLBREW_HOME=/Users/nicolas/.perlbrew
    PERLBREW_MANPATH=/Users/nicolas/perl5/perlbrew/perls/perl-5.14.4/man

PERLBREW_PATH=/Users/nicolas/perl5/perlbrew/bin:/Users/nicolas/perl5/perlbrew/perls/perl-5.14.4/bin
    PERLBREW_PERL=perl-5.14.4
    PERLBREW_ROOT=/Users/nicolas/perl5/perlbrew
    PERLBREW_VERSION=0.73
    PERL_BADLANG (unset)
    PERL_CPANM_OPT=--quiet
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2015

From @atoomic

0001-pending-author.t-fallback-to-the-last-commit.patch
From b3c3762ae339c62de2ac1a5b599aed134e5c3d72 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Fri, 11 Sep 2015 09:33:08 -0500
Subject: [PATCH] pending-author.t fallback to the last commit

pending-author.t tries to detect if the current
author is unknown but when your branch is clean
and you've commited your work, no check is performed.

We are now falling back to the last commit when
no changes are pending, to detect a missing author
entry earlier.

The documentation suggests to generate your patches
after running 'make test'.
---
 AUTHORS                    |  1 +
 t/porting/pending-author.t | 23 ++++++++++++++++-------
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/AUTHORS b/AUTHORS
index b5ef912..b6188e2 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -891,6 +891,7 @@ Nick Ing-Simmons
 Nick Johnston			<nickjohnstonsky@gmail.com>
 Nick Williams			<Nick.Williams@morganstanley.com>
 Nicolas Kaiser			<nikai@nikai.net>
+Nicolas R.  			<atoomic@cpan.org>
 Niels Thykier			<niels@thykier.net>
 Nigel Sandever			<njsandever@hotmail.com>
 Niko Tyni			<ntyni@debian.org>
diff --git a/t/porting/pending-author.t b/t/porting/pending-author.t
index 4dceaf6..1156e22 100644
--- a/t/porting/pending-author.t
+++ b/t/porting/pending-author.t
@@ -31,15 +31,25 @@ require 't/test.pl';
 find_git_or_skip('all');
 
 my $devnull = File::Spec->devnull;
+my $log;
 my $changes;
-foreach (`git status --porcelain 2>$devnull`) {
+foreach (qx{git status --porcelain 2>$devnull}) {
     next if /^\?\?/;
     ++$changes;
     last;
 }
 
-skip_all("No pending changes (or git status --porcelain doesn't work here)")
-    unless $changes;
+if ( $changes ) {
+    my $email = get('email') or die "No email set in git.config";
+    my $name = get('name') or die "No name set in git.config";
+
+    $log = "Author: $name <$email>\n";
+}
+else {
+    diag("No pending changes (or git status --porcelain doesn't work here), checking last commit");
+    $log = qx{git log -n1 2>$devnull};
+    die "git log failed" if $? != 0 || !$log;
+}
 
 sub get {
     my $key = shift;
@@ -51,10 +61,9 @@ sub get {
     return $value;
 }
 
-my $email = get('email');
-my $name = get('name');
-
 open my $fh, '|-', "$^X Porting/checkAUTHORS.pl --tap -"
     or die $!;
-print $fh "Author: $name <$email>\n";
+print {$fh} $log;
 close $fh or die $!;
+
+1;
-- 
2.3.2 (Apple Git-55)

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2015

From @jkeenan

On Mon Sep 14 09​:30​:31 2015, atoomic@​cpan.org wrote​:

This is a bug report for perl from atoomic@​cpan.org,
generated with the help of perlbug 1.40 running under perl 5.23.3.

-----------------------------------------------------------------
[Please describe your issue here]

pending-author.t tries to detect if the current
author is unknown but when your branch is clean
and you've commited your work, no check is performed.

The documentation suggests to generate your patches
after running 'make test'. (and patches are ignored
by git).

The earlier we detect an unknown author the better it's
and this should happen on the submitter side if possible.

Speaking as someone who applies a lot of patches from previously unknown authors, I have mixed feelings about this. On the one hand, it's nice when the (presumably first-time) submitter has added name and email address, correctly formatted, to AUTHORS.

On the other hand, that's one more thing that the first-time submitter has to get right -- and I'm reluctant to impose that burden on first-timers.

Input from other p5pers who often commit first-timer's patches is requested.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2015

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2015

From @atoomic

We could fail with a better message like, please add $name $email to the AUTHOR file
I'm pretty sure that most first submitter would be happy to include their name in the AUTHOR file.

On Mon Sep 14 17​:00​:03 2015, jkeenan wrote​:

On Mon Sep 14 09​:30​:31 2015, atoomic@​cpan.org wrote​:

This is a bug report for perl from atoomic@​cpan.org,
generated with the help of perlbug 1.40 running under perl 5.23.3.

-----------------------------------------------------------------
[Please describe your issue here]

pending-author.t tries to detect if the current
author is unknown but when your branch is clean
and you've commited your work, no check is performed.

The documentation suggests to generate your patches
after running 'make test'. (and patches are ignored
by git).

The earlier we detect an unknown author the better it's
and this should happen on the submitter side if possible.

Speaking as someone who applies a lot of patches from previously
unknown authors, I have mixed feelings about this. On the one hand,
it's nice when the (presumably first-time) submitter has added name
and email address, correctly formatted, to AUTHORS.

On the other hand, that's one more thing that the first-time submitter
has to get right -- and I'm reluctant to impose that burden on first-
timers.

Input from other p5pers who often commit first-timer's patches is
requested.

Thank you very much.

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2015

From @khwilliamson

On 09/14/2015 06​:00 PM, James E Keenan via RT wrote​:

On Mon Sep 14 09​:30​:31 2015, atoomic@​cpan.org wrote​:

This is a bug report for perl from atoomic@​cpan.org,
generated with the help of perlbug 1.40 running under perl 5.23.3.

-----------------------------------------------------------------
[Please describe your issue here]

pending-author.t tries to detect if the current
author is unknown but when your branch is clean
and you've commited your work, no check is performed.

The documentation suggests to generate your patches
after running 'make test'. (and patches are ignored
by git).

The earlier we detect an unknown author the better it's
and this should happen on the submitter side if possible.

Speaking as someone who applies a lot of patches from previously unknown authors, I have mixed feelings about this. On the one hand, it's nice when the (presumably first-time) submitter has added name and email address, correctly formatted, to AUTHORS.

On the other hand, that's one more thing that the first-time submitter has to get right -- and I'm reluctant to impose that burden on first-timers.

Input from other p5pers who often commit first-timer's patches is requested.

I have mixed feelings about this. I agree it is an internal bookkeeping
detail that a first time submitter should not have to think about on the
other hand it gets them thinking about how they wish to be publicly
identified.

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2015

From @tonycoz

On Mon Sep 14 09​:30​:31 2015, atoomic@​cpan.org wrote​:

pending-author.t tries to detect if the current
author is unknown but when your branch is clean
and you've commited your work, no check is performed.

The documentation suggests to generate your patches
after running 'make test'. (and patches are ignored
by git).

The earlier we detect an unknown author the better it's
and this should happen on the submitter side if possible.

Isn't the check against the head commit (and every other commit) already done by t/porting/authors.t ?

Tony

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2015

From @iabyn

On Mon, Sep 14, 2015 at 09​:30​:31AM -0700, Nicolas R. wrote​:

pending-author.t tries to detect if the current
author is unknown but when your branch is clean
and you've commited your work, no check is performed.

The documentation suggests to generate your patches
after running 'make test'. (and patches are ignored
by git).

The earlier we detect an unknown author the better it's
and this should happen on the submitter side if possible.

More generally, I wonder whether we should still maintain an AUTHORS file?
The information within it is now (mostly?) available from the public git
repository, and AUTHORS could just be a short text file explaining how to
generate this info using git.

In terms of Kudos, each release announcement contains a list of the
names of the people who have contributed to that release; perhaps this is
sufficient?

It would remove a whole load of extra makework.

--
I thought I was wrong once, but I was mistaken.

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2015

From @atoomic

Indeed did not realized that t/porting/authors.t was already covering this check
patch is then useless

also agreed about the burden to maintain the AUTHOR file, as git is going to give that information easily
at any time for any release

On Mon Sep 14 21​:56​:42 2015, tonyc wrote​:

On Mon Sep 14 09​:30​:31 2015, atoomic@​cpan.org wrote​:

pending-author.t tries to detect if the current
author is unknown but when your branch is clean
and you've commited your work, no check is performed.

The documentation suggests to generate your patches
after running 'make test'. (and patches are ignored
by git).

The earlier we detect an unknown author the better it's
and this should happen on the submitter side if possible.

Isn't the check against the head commit (and every other commit)
already done by t/porting/authors.t ?

Tony

@p5pRT
Copy link
Author

p5pRT commented Sep 16, 2015

From @tonycoz

On Tue Sep 15 05​:25​:27 2015, davem wrote​:

More generally, I wonder whether we should still maintain an AUTHORS file?
The information within it is now (mostly?) available from the public git
repository, and AUTHORS could just be a short text file explaining how to
generate this info using git.

In terms of Kudos, each release announcement contains a list of the
names of the people who have contributed to that release; perhaps this is
sufficient?

It would remove a whole load of extra makework.

The main issue would be authors from pre-git - not everyone in AUTHORS is visible in git log.

Tony

@p5pRT
Copy link
Author

p5pRT commented Sep 19, 2015

From @iabyn

On Tue, Sep 15, 2015 at 08​:39​:52PM -0700, Tony Cook via RT wrote​:

On Tue Sep 15 05​:25​:27 2015, davem wrote​:

More generally, I wonder whether we should still maintain an AUTHORS file?
The information within it is now (mostly?) available from the public git
repository, and AUTHORS could just be a short text file explaining how to
generate this info using git.

In terms of Kudos, each release announcement contains a list of the
names of the people who have contributed to that release; perhaps this is
sufficient?

It would remove a whole load of extra makework.

The main issue would be authors from pre-git - not everyone in AUTHORS
is visible in git log.

In which case AUTHORS could be re-purposed as a static list of older
contributors who don't show in git? With a comment at the explaining this.

--
"I do not resent criticism, even when, for the sake of emphasis,
it parts for the time with reality".
  -- Winston Churchill, House of Commons, 22nd Jan 1941.

@p5pRT
Copy link
Author

p5pRT commented Sep 19, 2015

From @jkeenan

On Tue Sep 15 08​:23​:42 2015, atoomic wrote​:

Indeed did not realized that t/porting/authors.t was already covering
this check
patch is then useless

Since the original poster has indicated that the patch is not needed, and since no one else has spoken up for applying it, I'm closing this ticket.

I would like to recommend that further discussion about the existence/maintenance of the AUTHORS file take place on the p5p list, as there are likely to be a wide range of opinions.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 19, 2015

@jkeenan - Status changed from 'open' to 'rejected'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant