Re: [PATCH v4 2/5] unpack-trees: add performance tracing

2018-08-12 Thread Thomas Adam
On Sun, 12 Aug 2018 at 09:19, Nguyễn Thái Ngọc Duy  wrote:

Hi,

> +   trace_performance_leave("cache_tree_update");

I would suggest trace_performance_leave() calls use __func__ instead.
That way, there's no ambiguity if the function name ever changes.

Kindly,
Thomas


Re: [PATCH] git-send-email: fix get_maintainer.pl regression

2017-12-12 Thread Thomas Adam
Hi,

On Mon, Dec 11, 2017 at 08:46:46PM +0100, Ævar Arnfjörð Bjarmason wrote:
> I.e. we'd just ship a copy of Email::Valid and Mail::Address in
> perl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't
> need to if/else this at the code level, just always use the module,
> and it would work even on core perl.

I disagree with the premise of this, Ævar.  As soon as you go down this route,
it increases maintenance to ensure we keep up to date with what's on CPAN for
a tiny edge-case which I don't believe exists.

You may as well just use App::FatPacker.

We're talking about package maintenance here -- and as I said before, there's
plenty of it around.  For those distributions which ship Git (and hence also
package git-send-email), the dependencies are already there, too.  I just
cannot see this being a problem in relying on non-core perl modules.  Every
perl program does this, and they don't go down this route of having copies of
various CPAN modules just in case.  So why should we?  We're not a special
snowflake.

-- Thomas Adam


Re: [PATCH] git-send-email: fix get_maintainer.pl regression

2017-12-11 Thread Thomas Adam
On Mon, Dec 11, 2017 at 05:13:53PM +, Alex Bennée wrote:
> So have we come to a consensus about the best solution here?
> 
> I'm perfectly happy to send a reversion patch because to be honest
> hacking on a bunch of perl to handle special mail cases is not my idea
> of fun spare time hacking ;-)
> 
> I guess the full solution is to make Mail::Address a hard dependency?

This is what I was suggesting, and then as a follow-up, addressing the point
that there's a bunch of require() hacks to also get around needing
hard-dependencies.

-- Thomas Adam


Re: [PATCH v4 0/2] launch_editor(): indicate that Git waits for user input

2017-11-30 Thread Thomas Adam
On Thu, Nov 30, 2017 at 03:12:17PM -0500, Jeff King wrote:
> On Wed, Nov 29, 2017 at 06:35:16PM +0000, Thomas Adam wrote:
> 
> > On Wed, Nov 29, 2017 at 03:37:50PM +0100, lars.schnei...@autodesk.com wrote:
> > > + if (print_waiting_for_editor) {
> > > + fprintf(stderr, _("hint: Waiting for your editor 
> > > input..."));
> > >   fflush(stderr);
> > 
> > Just FYI, stderr is typically unbuffered on most systems I've used, and
> > although the call to fflush() is harmless, I suspect it's not having any
> > effect.  That said, there's plenty of other places in Git which seems to 
> > think
> > fflush()ing stderr actually does something.
> 
> I'd prefer to keep them (including this one), even if they are noops on
> most platforms, because:
> 
>   1. They serve as a note for readers of the code that it's important
>  for the output to have been printed immediately.
> 
>   2. We build on some funny and antique platforms. I wouldn't be
>  surprised if there's one that line buffers by default. Or even a
>  modern system with funny settings (e.g., using the GNU stdbuf
>  tool).
> 
> (I know you said later you don't think this case needs to be removed,
> but I want to make it clear I think it's a reasonable project-wide
> policy to not assume we we know how stderr is buffered).

We're talking past each other, Peff.  I'm agreeing with you.  I was surprised
to see the introduction of fflush(stderr) in the interdiff, when it wasn't
present before, was curious to understand why.  I've done that, and since
stated it's fine to leave it as-is.

-- Thomas Adam


Re: [PATCH v4 0/2] launch_editor(): indicate that Git waits for user input

2017-11-30 Thread Thomas Adam
On Thu, Nov 30, 2017 at 02:55:35PM +0100, Lars Schneider wrote:
> 
> > On 29 Nov 2017, at 19:35, Thomas Adam <tho...@xteddy.org> wrote:
> > 
> > On Wed, Nov 29, 2017 at 03:37:50PM +0100, lars.schnei...@autodesk.com wrote:
> >> +  if (print_waiting_for_editor) {
> >> +  fprintf(stderr, _("hint: Waiting for your editor 
> >> input..."));
> >>fflush(stderr);
> > 
> > Just FYI, stderr is typically unbuffered on most systems I've used, and
> > although the call to fflush() is harmless, I suspect it's not having any
> > effect.  That said, there's plenty of other places in Git which seems to 
> > think
> > fflush()ing stderr actually does something.
> 
> I agree with the "unbuffered" statement. I am surprised that you expect 
> fflush()
> to do nothing in that situation... but I am no expert in that area. Can you
> point me to some documentation?

Because stderr is unbuffered, it will get printed immediately.

> In any way, would all this be a problem here? The worst that could happen 
> would
> be that the user would not see the message, right?

Correct -- I only bring this up because your interdiff showed you added the
fflush() call and I was merely pointing that out.  I don't expect you to
change it.

> Are you aware of stderr usage in Git that could cause more trouble?

No.  It'll all be fine.

-- Thomas Adam


Re: [PATCH v4 0/2] launch_editor(): indicate that Git waits for user input

2017-11-29 Thread Thomas Adam
On Wed, Nov 29, 2017 at 03:37:50PM +0100, lars.schnei...@autodesk.com wrote:
> + if (print_waiting_for_editor) {
> + fprintf(stderr, _("hint: Waiting for your editor 
> input..."));
>   fflush(stderr);

Just FYI, stderr is typically unbuffered on most systems I've used, and
although the call to fflush() is harmless, I suspect it's not having any
effect.  That said, there's plenty of other places in Git which seems to think
fflush()ing stderr actually does something.

-- Thomas Adam


Re: [PATCH] git-send-email: fix get_maintainer.pl regression

2017-11-22 Thread Thomas Adam
On Wed, Nov 22, 2017 at 09:05:41AM +, Alex Bennée wrote:
> My hacky guess about GIT's perl use calls is:
> 
>find . -iname "*.perl" -or -iname "*.pm" -or -iname "*.pl" | xargs grep -h 
>  "use .*::" | sort | uniq | wc -l
>88

So let us concentrate just on git-send-email.perl for now.  In the
Module::Extract::Use module (which happens to be what corelist uses), there's
an example script called 'extract_modules' which will statically analyse a
perl file and tell you the following information:

% perl extract_modules ./git-send-email.perl
Modules required by ./git-send-email.perl:
- Authen::SASL
- Cwd (first released with Perl 5)
- Email::Valid
- Error
- File::Spec::Functions (first released with Perl 5.00504)
- File::Temp (first released with Perl 5.006001)
- Getopt::Long (first released with Perl 5)
- Git
- Git::I18N
- IO::Socket::SSL
- MIME::Base64 (first released with Perl 5.007003)
- MIME::QuotedPrint (first released with Perl 5.007003)
- Net::Domain (first released with Perl 5.007003)
- Net::SMTP (first released with Perl 5.007003)
- Net::SMTP::SSL
- POSIX (first released with Perl 5)
- Sys::Hostname (first released with Perl 5)
- Term::ANSIColor (first released with Perl 5.006)
- Term::ReadLine (first released with Perl 5.002)
- Text::ParseWords (first released with Perl 5)
- strict (first released with Perl 5)
- warnings (first released with Perl 5.006)

Therefore, we have the following modules which are not standard:

- Email::Valid
- Error
- Git
- Git::I18N
- IO::Socket::SSL
- NET::SMTP::SSL

Looking at the code for git-send-email.perl, it seems most of those are
eval()d at the point they're needed, which seems in many cases to be fallback
responses to something we've written, or a means of ensuring we don't need to
explicitly handle the case of it not being present at run-time.

> Should the solution be to just make Mail::Address a hard dependency and
> not have the fallback?

This seems like a slight on ensuring a running script which may or may not
have additional functionality depending on which modules are installed.  Given
the pretty good state of packaging across those platforms which Git runs on, I
would argue we're now in a much better position to explicitly check for
non-core modules at BEGIN{} time, and moan loudly if they're not installed.

-- Thomas Adam


Re: [PATCH] git-send-email: fix get_maintainer.pl regression

2017-11-21 Thread Thomas Adam
On Tue, Nov 21, 2017 at 08:46:59PM +, Alex Bennée wrote:
> 
> Eric Sunshine <sunsh...@sunshineco.com> writes:
> > Aside from those observations, it looks like the tokenizer in this
> > function is broken. For any input with the address enclosed in "<" and
> > ">", the comment is lost entirely; it doesn't even end up in the
> > @tokens array. Since you're already fixing bugs/regressions in this
> > code, perhaps that's something you'd like to tackle as well in a
> > separate patch? ("No" is an acceptable answer, of course.)
> >
> >> } elsif ($token eq "<") {
> >> push @phrase, (splice @address), (splice @buffer);
> >> } elsif ($token eq ">") {
> 
> I can have a go but my perl-fu has weakened somewhat since I stopped
> having to maintain perl code for a living. It's almost as though my
> brain was glad to dump the knowledge ;-)
> 
> I guess we could maintain a nesting count and a current token type and
> use that to more intelligently direct the nested portions to the
> appropriate bits. Maybe Matthieu or Remi (CC'ed) might want to chime in
> on other options?

Trying to come up with a reinvention of regexps for email addresses is asking
for trouble, not to mention a crappy rod for your own back.  Don't do that.
This is why people use Mail::Address.

https://metacpan.org/pod/distribution/MailTools/lib/Mail/Address.pod

-- Thomas Adam


Re: [PATCH v4 3/7] remote-mediawiki: show known namespace choices on failure

2017-11-07 Thread Thomas Adam
On Mon, Nov 06, 2017 at 04:19:49PM -0500, Antoine Beaupré wrote:
> If we fail to find a requested namespace, we should tell the user
> which ones we know about, since those were already fetched. This
> allows users to fetch all namespaces by specifying a dummy namespace,
> failing, then copying the list of namespaces in the config.
> 
> Eventually, we should have a flag that allows fetching all namespaces
> automatically.
> 
> Reviewed-by: Antoine Beaupré <anar...@debian.org>
> Signed-off-by: Antoine Beaupré <anar...@debian.org>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl 
> b/contrib/mw-to-git/git-remote-mediawiki.perl
> index a1d783789..6364d4e91 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -1334,7 +1334,8 @@ sub get_mw_namespace_id {
>   my $id;
>  
>   if (!defined $ns) {
> - print {*STDERR} "No such namespace ${name} on MediaWiki.\n";
> + my @namespaces = map { s/ /_/g; $_; } sort keys %namespaces_id;

Oops.  This was my typo from my original suggestion.  The hash is
'%namespace_id', not '%namespaces_id'.  However, how did this slip through
testing?  I'm assuming you blindly copied this from my example, which although
quick to do, is only being caught because of my sharp eyes...

-- Thomas Adam


Re: [PATCH v4 2/7] remote-mediawiki: allow fetching namespaces with spaces

2017-11-06 Thread Thomas Adam
On Mon, Nov 06, 2017 at 04:19:48PM -0500, Antoine Beaupré wrote:
> From: Ingo Ruhnke <grum...@gmail.com>
> 
> we still want to use spaces as separators in the config, but we should
> allow the user to specify namespaces with spaces, so we use underscore
> for this.
> 
> Reviewed-by: Antoine Beaupré <anar...@debian.org>
> Signed-off-by: Antoine Beaupré <anar...@debian.org>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl 
> b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 5ffb57595..a1d783789 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -65,6 +65,7 @@ chomp(@tracked_categories);
>  
>  # Just like @tracked_categories, but for MediaWiki namespaces.
>  my @tracked_namespaces = split(/[ \n]/, run_git("config --get-all 
> remote.${remotename}.namespaces"));
> +for (@tracked_namespaces) { s/_/ /g; }
>  chomp(@tracked_namespaces);

Depending on the number if namespaces returned, it might be easier to convert
this to the following:

my @tracked_namespaces = map {
chomp; s/_/ /g; $_;
} split(/[ \n]/, run_git("config --get-all 
remote.${remotename}.namespaces"));

This would, once again, avoid creating @tracked_namespaces, and iterating over
it.

Note that this isn't about trying to 'golf' this; it's a performance
consideration.

Kindly,
Thomas Adam


Re: [PATCH 3/4] remote-mediawiki: show known namespace choices on failure

2017-11-04 Thread Thomas Adam
On Sun, Oct 29, 2017 at 12:08:56PM -0400, Antoine Beaupré wrote:
> if we fail to find a requested namespace, we should tell the user
> which ones we know about, since we already do. this allows users to
> feetch all namespaces by specifying a dummy namespace, failing, then
> copying the list of namespaces in the config.
> 
> eventually, we should have a flag that allows fetching all namespaces
> automatically.
> 
> Reviewed-by: Antoine Beaupré <anar...@debian.org>
> Signed-off-by: Antoine Beaupré <anar...@debian.org>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl 
> b/contrib/mw-to-git/git-remote-mediawiki.perl
> index fc48846a1..07cc74bac 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -1334,7 +1334,9 @@ sub get_mw_namespace_id {
>   my $id;
>  
>   if (!defined $ns) {
> - print {*STDERR} "No such namespace ${name} on MediaWiki.\n";
> + my @namespaces = sort keys %namespace_id;
> + for (@namespaces) { s/ /_/g; }

I am sure we can improve upon the need to process @namespaces twice:

my @namespaces = map { s/ /_/g; $_; } sort keys %namespaces_id;

-- Thomas Adam


Re: [PATCH v3 7/7] remote-mediawiki: show progress while fetching namespaces

2017-11-04 Thread Thomas Adam
On Thu, Nov 02, 2017 at 07:10:17PM -0400, Antoine Beaupré wrote:
> Actually, is there a standard way to do this in git with Perl
> extensions? I know about "option verbosity N" but how should I translate
> this into Perl? Carp? Warn? Log::Any? Log4perl?

No, not really.  From a quick glance at some of the existing perl code in git,
a lot of it continues to use "print STDERR" -- but then to be fair, a lot of
the perl code also reads like it has been written by C programmers...

While there's nothing wrong with using "print STDERR", it's probably wiser to
transition this to using Carp in the long run -- it would decrease the
round-trip time to debugging should there be a situation where that was
needed, and hence I would recommend using "warn" for less-severe
errors/debugging.

-- Thomas Adam


Re: [PATCH v3 7/7] remote-mediawiki: show progress while fetching namespaces

2017-11-02 Thread Thomas Adam
On Thu, Nov 02, 2017 at 06:26:43PM -0400, Antoine Beaupré wrote:
> On 2017-11-02 22:18:07, Thomas Adam wrote:
> > Hi,
> >
> > On Thu, Nov 02, 2017 at 05:25:18PM -0400, Antoine Beaupré wrote:
> >> +print {*STDERR} "$#{$mw_pages} found in namespace 
> >> $local_namespace ($namespace_id)\n";
> >
> > How is this any different to using warn()?  I appreciate you're using a
> > globbed filehandle, but it seems superfluous to me.
> 
> It's what is used everywhere in the module, I'm just tagging along.
> 
> This was discussed before: there's an issue about cleaning up the
> messaging in that module, that can be fixed separately.

Understood.  That should happen sooner rather than later.

-- Thomas Adam


Re: [PATCH v3 7/7] remote-mediawiki: show progress while fetching namespaces

2017-11-02 Thread Thomas Adam
Hi,

On Thu, Nov 02, 2017 at 05:25:18PM -0400, Antoine Beaupré wrote:
> +print {*STDERR} "$#{$mw_pages} found in namespace $local_namespace 
> ($namespace_id)\n";

How is this any different to using warn()?  I appreciate you're using a
globbed filehandle, but it seems superfluous to me.

Kindly,
Thomas


Ascertaining amount of "original" code across files/repo

2017-10-22 Thread Thomas Adam
Hi all,

I was recently left with an interesting problem of looking at a heuristic to
determine how much original code was left in a repository.  Or to put another
way, how much the code had changed since.  In my case "original code" means
"since the initial commit", as this code base had been imported from CVS long
ago; and that was the correct starting point.

What I came up with was the following heuristic.  What I'm curious to know is
whether there's an alternative way to look at this and/or if such tooling
already exists.

What I did was first of all ascertain the number of original lines in each of
the files I was interested in:

for i in *.[ch]
do
c="$(git --no-pager blame "$i" | grep -c '^\^')"
[ $c -gt 0 ] && echo "$i:$c"
done | sort -t':' -k2 -nr

Given this, I then did some maths on the total lines from each of those files
and to work out a percentage by file, and over all.

What I'm curious to know is whether this approach of using "git blame" is a
good approach or not.

Thanks for your time.

-- Thomas Adam


Re: When does git check for branch-X being uptodate with origin/branch-X?

2016-03-21 Thread Thomas Adam
On 21 March 2016 at 21:18, Jeff King <p...@peff.net> wrote:
> Again, just my opinion, but that looks awfully clunky. And it doesn't
> address the other messages (you might be behind origin/branch-X by N
> commits, or ahead by N, but only as of that particular date). Do we want
> to annotate every message whose value was computed based on a tracking
> branch?

Hmm.  I would hope not.  I'll wait for others to make a call on this,
but it's about all I can suggest without significantly bloating the
message which isn't desirable either.

>> No one's suggesting that this message is removed, I'm not sure where
>> you got that from?
>
> You said earlier:
>
>> [...]it's more a case of whether even printing that message is useful?
>
> I didn't know quite what you had in mind, which is why I asked. If we
> all agree that removing it is a bad idea, then good, we don't have to
> bother discussing that option. :)

Ah, oops!  I was meaning more, whether to print the message in the
case where the branch was uptodate, but now I appreciate it's cached.
Apologies for the confusion.

-- Thomas Adam
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: When does git check for branch-X being uptodate with origin/branch-X?

2016-03-21 Thread Thomas Adam
On 21 March 2016 at 20:50, Jeff King <p...@peff.net> wrote:
> But that's just my opinion. Did you have some specific change you're
> interested in? I don't think removing that message is productive; it
> _is_ useful information. Perhaps it could be more clear that we are
> talking about the tracking branch?

I don't have a specific change in mind per-se, rather than to discuss
how we might be able to improve the error message, or document
somewhere that it's referring to the tracking branch.  Maybe that's
the point--is it worth mentioning the time/date of when the cache was
last updated?  That is:

"branch-X is uptodate with origin/branch-X (as of DD-MM-YY HH:MM:SS)"

No one's suggesting that this message is removed, I'm not sure where
you got that from?

-- Thomas Adam
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: When does git check for branch-X being uptodate with origin/branch-X?

2016-03-21 Thread Thomas Adam
On 21 March 2016 at 20:28, Jeff King <p...@peff.net> wrote:
> We never contact other repositories unless explicitly asked to by
> fetch, pull, push, etc. If you want to have the most up-to-date value
> without merging, you can just "git fetch" to update the tracking
> branches.

Thanks.  I understand how to use git-fetch, it's more a case of
whether even printing that message is useful?  I appreciate it can
only go on the cached value, but it is still misleading to print that
in the case where the cache might not be up to date.  Of course,
determining that is a different problem.

-- Thomas Adam
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


When does git check for branch-X being uptodate with origin/branch-X?

2016-03-21 Thread Thomas Adam
Hi all,

Something I've seen a few times of late (although I doubt that's any
indication that the code has changed in Git) is the reporting of
branch-X being uptodate with origin/branch-X when it isn't.

When does git check to see if branch-X has a remote tracking branch
and that it has changes on it?  Only, the output below is misleading:

% git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'fvwmorg/master'.

[fvwm-cvs-to-git/docs]{10345}[0][master] % git pull
remote: Counting objects: 26, done.
remote: Compressing objects: 100% (12/12), done.
remote: Total 26 (delta 18), reused 22 (delta 14), pack-reused 0
Unpacking objects: 100% (26/26), done.
>From github.com:fvwmorg/fvwm
   c029868..36cc898  master -> fvwmorg/master
   4f0c7ec..36cc898  ta/git-docs -> fvwmorg/ta/git-docs
Updating c029868..36cc898
Fast-forward
[...]

Clearly, it's obvious that "Your branch is up-to-date with
'fvwmorg/master'." is misleading.  Note that in this case, there's no
passwords or other hindrances to Git being able to work out that a
branch is behind another.

Any light that can be shed on this, is much appreciated!

Kindly,
Thomas Adam
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC] Code reorgnization

2016-03-19 Thread Thomas Adam
On 17 March 2016 at 11:11, Duy Nguyen <pclo...@gmail.com> wrote:
> Git's top directory is crowded and I think it's agreed that moving
> test-* to t/helper is a good move. I just wanted to check if we could
> take this opportunity (after v2.8.0) to move some other files too. I
> propose the following new subdirs

I wonder whether previous discussions on this still count?  See:

http://marc.info/?l=git=129650572621523=1

-- Thomas Adam
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Question about rerere

2014-11-06 Thread Thomas Adam
On 6 November 2014 11:30, Tristan Roussel trous...@phare.normalesup.org wrote:
 Hello,

 I’ve just learnt about rerere and it’s going to make my life so much easier, 
 I have a question though.

 I enabled rerere very lately and I wanted to know if there was a way to feed 
 rerere with old merge conflict resolutions (and if not, would it be 
 considered a good feature request)? I’d like to do a rebase of my work 
 because I unintentionally merged a branch I didn’t want and the merge is a 
 bit far in the git history and I don’t want to re-resolve merge conflicts I 
 had after that.

Have a look at contrib/rerere-train.sh

-- Thomas Adam
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v1 1/2] Remove 'git archimport'

2014-05-09 Thread Thomas Adam
On 9 May 2014 18:52, Felipe Contreras felipe.contre...@gmail.com wrote:

 Such incredible double standards.

I think I speak for everyone when I say: fuck off.

I'm sick and tired of seeing you pop up here with polemic and
rhetoric, publicly outing those who are making Git great, just because
you're not getting your own way.

You have a fork of git which you created, go away and use it.

-- Thomas Adam
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] contrib/diff-highlight: multibyte characters diff

2014-02-12 Thread Thomas Adam
On 12 February 2014 20:59, Jeff King p...@peff.net wrote:
 +sub decode {
 +   my $orig = shift;
 +   my $decoded = eval { decode_utf8($orig, Encode::FB_CROAK) };
 +   return defined $decoded ?

I'd still advocate checking $@ here, rather than the defined $decoded check.

 +  ($decoded, sub { encode_utf8(shift) }) :
 +  ($orig, sub { shift });
 +}
 +

-- Thomas Adam
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] status: display the SHA1 of the commit being currently processed

2013-06-17 Thread Thomas Adam
Hi,

[I rarely do reviews on this list, so feel free to ignore this.]

On 17 June 2013 13:10, Mathieu Lienard--Mayor
mathieu.lienard--ma...@ensimag.imag.fr wrote:
 diff --git a/wt-status.c b/wt-status.c
 index bf84a86..5f5cddf 100644
 --- a/wt-status.c
 +++ b/wt-status.c
 @@ -885,8 +885,19 @@ static void show_rebase_in_progress(struct wt_status *s,
 struct wt_status_state *state,
 const char *color)
  {
 +   char *stopped_sha = 
 read_line_from_git_path(rebase-merge/stopped-sha);
 +   int must_free_stopped_sha = 1;
 struct stat st;

 +   /*
 +* If the file stopped-sha does not exist
 +* we go back to the old output saying a commit
 +* instead of providing the commit's SHA1.
 +*/
 +   if (!stopped_sha) {
 +   stopped_sha = a commit;
 +   must_free_stopped_sha = 0;
 +   }

Rather than having to assign a toggle of deciding when to free
stopped_sha, how much overhead would be introduced by just doing:

if (!stopped_sha)
stopped_sha = strdup(a commit);

And then at the end just do:

free (stopped_sha);

Just a thought.

-- Thomas Adam
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Documentation/CommunityGuidelines

2013-06-13 Thread Thomas Adam
Hello,

On Mon, Jun 10, 2013 at 06:58:47PM +0530, Ramkumar Ramachandra wrote:
 I've tried to write down a bare minimum, without restating the obvious.

[...]

I often come across so-called community guidelines in other
projects---some of which adhere to them quite strictly, and others simply
document something for the curious.  But usually the reason for their
existence in the first place are tell-tale signs of trying to fix a problem
at the wrong end, and I believe this is what is about to happen if a
document such as this ever becomes official.

There's no disputing the fact that over the last few weeks, FC's behaviour
has been called in to question.  He's managed to rub a lot of core people up
the wrong way, and in doing so has deliberately side-stepped that problem by
doing the one thing which puts anyone trying to raise that point muted; by
assuming that he's right.

It's a point on which one is never going to win, because no matter what one
says, it'll just get twisted round in such a way that one then ends up
questioning their own words, and their own conduct, and that's bad, because
there never was anything wrong with them to begin with.

So when you realise this point, it becomes almost impossible to proceed
further with any kind of discussion, because even the technical points of
discussion then end up being lost in a tirade of needless side-stepping
discussion.  It's a bullying tactic on the part of FC which means he'll do
any, and everything, to get his own way.

So I say to all those seasoned reviewers out there on this list not to put
up with it.

There comes a point, regardless of how useful someone's contribution may be,
that if the barrier to entry is so high that any kind of criticism or
comment made against code comes with a massive chance of having to defend
yourself against innocence on the part of the reviewer, that those
contributions should be shelved.  I've seen also another yardstick used to
defend FC's behaviour, and that is one of commits within the last three
months.  That count is completely meaningless, since the review process is
always going to be the same.

So these guidelines gain the community nothing, and only serve to punish
those who are already following them, without them being written down,
because the root-cause of the problem is still here, and isn't going to go
away, no matter how much referring to these guidelines might help.

That is why I think this is the wrong thing to do.

-- Thomas Adam
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3] Add new @ shortcut for HEAD

2013-05-01 Thread Thomas Adam
On 1 May 2013 11:12, Eric Sunshine sunsh...@sunshineco.com wrote:
 On Wed, May 1, 2013 at 5:51 AM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 So HEAD@{0}~0^0 is too much to type, but we can remove '^0', and we can
 remove '~0', and we can remove 'HEAD', which leaves us with @{0}, but we
 can't remove '{0}'?

 This patch allows '@' to be the same as 'HEAD'.

 So now we can use 'git show @~1', and all that goody goodness.

 Until now '@' was a valid name, but it conflicts with this idea, so lets

 s/lets/let's/  (contraction of let us)

Ah, the contraction versus the first person singular.  In this case
where the context is concluding in decision, rather than making a
statement (Let's go to the shops, for example) then lets is the
correct word to use here.

-- Thomas Adam
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC PATCH 0/1] status: Allow for short-form output by default

2012-11-11 Thread Thomas Adam
Hi,

It was asked recently whether git status could output the short-form instead
of the long output (via its -sb options).  To that end, I've created a
rough POC on how this might look.  It's deliberately lacking documentation;
I was curious to know whether:

status.shortwithbranch = true

Was going to cut it.  Likewise, I was unsure if instead there should  be two
separate options, one for just short (i.e. '-s') also?

What do others think?

ISTR reading on this list recently the addition of a --long option to git
status?  I'm wondering how this would relate to that, if at all?

Kindly,

Thomas Adam (1):
  status: Allow for short-form via config option

 builtin/commit.c | 12 
 1 file changed, 12 insertions(+)

-- 
1.7.11.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC PATCH 1/1] status: Allow for short-form via config option

2012-11-11 Thread Thomas Adam
It is currently not possible to use the short-form output of git status
without declaring an alias to do so.

This isn't always desirable therfore, define a git config option which can
be set to display the short-form:  status.shortwithbranch

Signed-off-by: Thomas Adam tho...@xteddy.org
---
 builtin/commit.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index a17a5df..552a9f1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1142,6 +1142,18 @@ static int git_status_config(const char *k, const char 
*v, void *cb)
return error(_(Invalid untracked files mode '%s'), v);
return 0;
}
+
+   if (!strcmp(k, status.shortwithbranch)) {
+   if (git_config_bool(k, v)) {
+   if (!v)
+   return config_error_nonbool(k);
+   else if(!strcmp(v, true)) {
+   status_format = STATUS_FORMAT_SHORT;
+   s-show_branch = 1;
+   }
+   return 0;
+   }
+   }
return git_diff_ui_config(k, v, NULL);
 }
 
-- 
1.7.11.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 00/13] New remote-hg helper

2012-11-02 Thread Thomas Adam
On 2 November 2012 18:39, Felipe Contreras felipe.contre...@gmail.com wrote:
 I disagree. The open source process works not by making favors to each
 other, but by everyone sharing and improving the code, by
 *collaborating*. I review your code if you review mine, or if you
 by me a bear in the next conference is not the spirit of open source,
 although it might happen often.

So shunning any attempt at explanation, and peddling your own thoughts
over and over again, irrespective of whether you contribute code or
not -- doesn't mean to say you're right, Felipe.  And that's the
fundamental issue here -- your code speaks for itself, sure, no one
denies that, but the code is not even *half* of what makes up the
discussion.  And so far, the surrounding context and attitude from you
doesn't help or enhance the process under which your code is reviewed.
 And no, you cannot philosophise this, or wriggle out of it through
idealism or some other charter or code of conduct -- as reviewers
of your code, we have to interact with you to be able to better it.
But you seem very reluctant to do that.

The fact that we're even having the conversation is evident of that.

-- Thomas Adam
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Wrap commit messages on `git commit -m`

2012-11-01 Thread Thomas Adam
Hi,

On 1 November 2012 16:07, Ramkumar Ramachandra artag...@gmail.com wrote:

 Hi,

 Some of my colleagues are lazy to fire up an editor and write proper
 commit messages- they often write one-liners using `git commit -m`.
 However, that line turns out to be longer than 72 characters, and the
 resulting `git log` output is ugly.  So, I was wondering if it would
 be a good idea to wrap these one-liners to 72 characters
 automatically.

Can't you do this already?  From git-log(1):

 %w([w[,i1[,i2]]]): switch line wrapping, like the -w
option of git-shortlog(1).

-- Thomas Adam
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html