Git development
 help / color / mirror / Atom feed
* Re: [RFC PATCH 3/2] vreportf: add prefix to each line
From: Jeff King @ 2017-01-12  6:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqpojtmeld.fsf@gitster.mtv.corp.google.com>

On Wed, Jan 11, 2017 at 02:11:42PM -0800, Junio C Hamano wrote:

> > It's actually kind of ugly.  For instance, a failing test in
> > t3600 now produces:
> >
> >    error: the following files have staged content different from both the
> >    error: file and the HEAD:
> >    error:     bar.txt
> >    error:     foo.txt
> >    error: (use -f to force removal)
> >
> > which seems a bit aggressive.  
> 
> I agree that it is ugly, but one reason I was hoping to do this
> myself (or have somebody else do it by procrastinating) was that I
> thought it may help i18n.  That is, for an original
> 
> 	error(_("we found an error"))
> 
> a .po file may translate the string to a multi-line string that has
> LFs in it and the output would look correct.  The translator already
> can do so by indenting the second and subsequent lines by the same
> column-width as "error: " (or its translation in their language, if
> we are going to i18n these headers), but that (1) is extra work for
> them, and (2) makes it impossible to have the same message appear in
> different contexts (i.e. "error:" vs "warning:" that have different
> column-widths).

Yes, I agree that would be a functional benefit. I'm just hoping we can
do it in a way that is visually pleasing.

> > It also screws up messages which indent with tabs (the prefix eats
> > up some of the tabstop, making the indentation smaller).
> 
> This is unavoidable and at the same time is a non-issue, isn't it?
> Messages that indent the second and subsequent lines with tabs are
> compensating the lack of the multi-line support of vreportf(), which
> this RFC patch fixes.  They may need to be adjusted to the new world
> order, but that is a good thing.  New multi-line messages no longer
> have to worry about the prefix that is added only to the first line
> when continuing the message to multiple lines.

I'm not so sure it is just about compensating. Look at the message
quoted above. The original looks like:

  error: the following files have staged content different from both the
  file and the HEAD:
      bar.txt
      foo.txt
  (use -f to force removal)

The leading whitespace is visually separating the list of files, not
just from the line with "error:", but from the other lines.

Though I think if we replaced tabs with spaces in this instance, then
they would still be bumped relative to the rest of the text.

> > It may be possible to address some of that by using some
> > other kind of continuation marker (instead of just repeating
> > the prefix), and expanding initial tabs.
> 
> Yes indeed.  The "some other kind of continuation marker" could just
> be a run of spaces that fill the same column as the "error: " or
> other prefix given to the first line.

I tried that, along with several other variants, but it gets
confusing/ugly when mixed with indentation that is significant to the
line. For example, the t3600 message becomes:

  error: the following files have staged content different from both the
         file and the HEAD:
             bar.txt
             foo.txt
         (use -f to force removal)

Which is arguably better than what we have now, but still looks pretty
bad to me.

I wonder if it would help for the marker to end in a non-whitespace
character. Like:

  error: the following files have staged content different from both the
       :  file and the HEAD:
       :     bar.txt
       :     foo.txt
       : (use -f to force removal)

or something. The ":" is a bit sparse looking. Maybe there are better
options. That does ruin line-by-line selection for cut-and-paste,
though.

So I dunno. I am open to ideas, but I didn't find one that I really
liked (this patch is actually a leftover from before Christmas, so I
don't even remember all the things I tried, just that I didn't like any
of them ;) ).

-Peff

^ permalink raw reply

* Re: [PATCH 2/2] Use 'env' to find perl instead of fixed path
From: Johannes Sixt @ 2017-01-12  6:27 UTC (permalink / raw)
  To: Pat Pannuto; +Cc: gitster, git
In-Reply-To: <20170112055140.29877-3-pat.pannuto@gmail.com>

Am 12.01.2017 um 06:51 schrieb Pat Pannuto:
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index cf6fc926a..6d7b6c35d 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1,4 +1,4 @@
> -#!/usr/bin/perl
> +#!/usr/bin/env perl

This change, and all others that affect installed external git programs, 
is a no-go. On Windows, our execve emulation is not complete. It would 
invoke only `env` (looked up in PATH), but not pass 'perl' as argument.

Sorry for the bad news.

I would have suggested to set PERL_PATH in your config.mak, but that 
does not change the generated perl scripts, I think. Perhaps you should 
implement that?

I'm not thrilled about your changes to the test scripts, but I do not 
expect that they break on Windows.

-- Hannes


^ permalink raw reply

* Re: [PATCH 0/2] Use env for all perl invocations
From: Junio C Hamano @ 2017-01-12  6:21 UTC (permalink / raw)
  To: Pat Pannuto; +Cc: git
In-Reply-To: <20170112055140.29877-1-pat.pannuto@gmail.com>

Pat Pannuto <pat.pannuto@gmail.com> writes:

> I spent a little while debugging why git-format-patch refused to believe
> that SSL support was installed (Can't locate Net/SMTP/SSL.pm in @INC...)
> Turns out that it was installed for my system's preferred /usr/local/bin/perl,
> but not for git-format-patch's hard-coded /usr/bin/perl; changing the shebang
> allowed git format-patch to work as expected.

Isn't that an indication that you are not building correctly?
Perhaps

    $ git grep 'Define PERL_' Makefile
    $ make PERL_PATH=/usr/local/bin/perl

would help?

^ permalink raw reply

* [PATCH 2/2] Use 'env' to find perl instead of fixed path
From: Pat Pannuto @ 2017-01-12  5:51 UTC (permalink / raw)
  To: gitster, git; +Cc: Pat Pannuto
In-Reply-To: <20170112055140.29877-1-pat.pannuto@gmail.com>

Depending on system configuration, /usr/bin/perl may not be the
preferred perl interpreter, use /usr/bin/env perl to invoke perl
instead to repsect user preferences.

In most cases this will not matter as any perl install will work,
but for applications such as git-format-patch, which may require
plugins such as Net/SMTP/SSL.pm to be installed, it is very confusing
when git fails to work after installation because it chose the
wrong perl installation.

This patch converts all shebangs to use env for consistency across
the project.

Signed-off-by: Pat Pannuto <pat.pannuto@gmail.com>
---
 Documentation/build-docdep.perl               | 2 +-
 Documentation/cat-texi.perl                   | 2 +-
 Documentation/cmd-list.perl                   | 2 +-
 Documentation/fix-texi.perl                   | 2 +-
 Documentation/lint-gitlink.perl               | 2 +-
 compat/vcbuild/scripts/clink.pl               | 2 +-
 compat/vcbuild/scripts/lib.pl                 | 2 +-
 contrib/buildsystems/engine.pl                | 2 +-
 contrib/buildsystems/generate                 | 2 +-
 contrib/buildsystems/parse.pl                 | 2 +-
 contrib/contacts/git-contacts                 | 2 +-
 contrib/credential/netrc/git-credential-netrc | 2 +-
 contrib/credential/netrc/test.pl              | 2 +-
 contrib/diff-highlight/diff-highlight         | 2 +-
 contrib/examples/git-remote.perl              | 2 +-
 contrib/examples/git-rerere.perl              | 2 +-
 contrib/examples/git-svnimport.perl           | 2 +-
 contrib/fast-import/git-import.perl           | 2 +-
 contrib/fast-import/import-directories.perl   | 2 +-
 contrib/fast-import/import-tars.perl          | 2 +-
 contrib/hooks/setgitperms.perl                | 2 +-
 contrib/hooks/update-paranoid                 | 2 +-
 contrib/long-running-filter/example.pl        | 2 +-
 contrib/mw-to-git/git-mw.perl                 | 2 +-
 contrib/mw-to-git/git-remote-mediawiki.perl   | 2 +-
 contrib/mw-to-git/t/test-gitmw.pl             | 2 +-
 contrib/stats/mailmap.pl                      | 2 +-
 contrib/stats/packinfo.pl                     | 2 +-
 git-add--interactive.perl                     | 2 +-
 git-archimport.perl                           | 2 +-
 git-cvsexportcommit.perl                      | 2 +-
 git-cvsimport.perl                            | 2 +-
 git-cvsserver.perl                            | 2 +-
 git-difftool.perl                             | 2 +-
 git-relink.perl                               | 2 +-
 git-send-email.perl                           | 2 +-
 git-svn.perl                                  | 2 +-
 gitweb/gitweb.perl                            | 2 +-
 t/Git-SVN/Utils/can_compress.t                | 2 +-
 t/Git-SVN/Utils/fatal.t                       | 2 +-
 t/check-non-portable-shell.pl                 | 2 +-
 t/gitweb-lib.sh                               | 2 +-
 t/perf/aggregate.perl                         | 2 +-
 t/perf/min_time.perl                          | 2 +-
 t/t0202/test.pl                               | 2 +-
 t/t4034/perl/post                             | 2 +-
 t/t4034/perl/pre                              | 2 +-
 t/t9000/test.pl                               | 2 +-
 t/t9500-gitweb-standalone-no-errors.sh        | 2 +-
 t/t9700/test.pl                               | 2 +-
 t/test-terminal.perl                          | 2 +-
 51 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/Documentation/build-docdep.perl b/Documentation/build-docdep.perl
index ba4205e03..dc50f21f3 100755
--- a/Documentation/build-docdep.perl
+++ b/Documentation/build-docdep.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 my %include = ();
 my %included = ();
diff --git a/Documentation/cat-texi.perl b/Documentation/cat-texi.perl
index 1cd28b1b5..98dcc2c42 100755
--- a/Documentation/cat-texi.perl
+++ b/Documentation/cat-texi.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use warnings;
 
diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
index ba640a441..f440eebe3 100755
--- a/Documentation/cmd-list.perl
+++ b/Documentation/cmd-list.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use warnings;
 
diff --git a/Documentation/fix-texi.perl b/Documentation/fix-texi.perl
index c247aece7..61287a0a9 100755
--- a/Documentation/fix-texi.perl
+++ b/Documentation/fix-texi.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use warnings;
 
diff --git a/Documentation/lint-gitlink.perl b/Documentation/lint-gitlink.perl
index 476cc30b8..3e4b00eda 100755
--- a/Documentation/lint-gitlink.perl
+++ b/Documentation/lint-gitlink.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use File::Find;
 use Getopt::Long;
diff --git a/compat/vcbuild/scripts/clink.pl b/compat/vcbuild/scripts/clink.pl
index 46eb61c5c..857847703 100755
--- a/compat/vcbuild/scripts/clink.pl
+++ b/compat/vcbuild/scripts/clink.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 ######################################################################
 # Compiles or links files
 #
diff --git a/compat/vcbuild/scripts/lib.pl b/compat/vcbuild/scripts/lib.pl
index e571b8470..9b43f1270 100755
--- a/compat/vcbuild/scripts/lib.pl
+++ b/compat/vcbuild/scripts/lib.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 ######################################################################
 # Libifies files on Windows
 #
diff --git a/contrib/buildsystems/engine.pl b/contrib/buildsystems/engine.pl
index a173669ce..ace1fd4bf 100755
--- a/contrib/buildsystems/engine.pl
+++ b/contrib/buildsystems/engine.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 ######################################################################
 # Do not call this script directly!
 #
diff --git a/contrib/buildsystems/generate b/contrib/buildsystems/generate
index 9af89454a..d6ce919cb 100755
--- a/contrib/buildsystems/generate
+++ b/contrib/buildsystems/generate
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 ######################################################################
 # Generate buildsystem files
 #
diff --git a/contrib/buildsystems/parse.pl b/contrib/buildsystems/parse.pl
index 33ca89eb0..492acf447 100755
--- a/contrib/buildsystems/parse.pl
+++ b/contrib/buildsystems/parse.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 ######################################################################
 # Do not call this script directly!
 #
diff --git a/contrib/contacts/git-contacts b/contrib/contacts/git-contacts
index dbe2abf27..70ec2039a 100755
--- a/contrib/contacts/git-contacts
+++ b/contrib/contacts/git-contacts
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 # List people who might be interested in a patch.  Useful as the argument to
 # git-send-email --cc-cmd option, and in other situations.
diff --git a/contrib/credential/netrc/git-credential-netrc b/contrib/credential/netrc/git-credential-netrc
index 1571a7b26..ec8178f26 100755
--- a/contrib/credential/netrc/git-credential-netrc
+++ b/contrib/credential/netrc/git-credential-netrc
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 use warnings;
diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
index 169b6463c..6b44ba54d 100755
--- a/contrib/credential/netrc/test.pl
+++ b/contrib/credential/netrc/test.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use warnings;
 use strict;
diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index 81bd8040e..ecf419542 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use 5.008;
 use warnings FATAL => 'all';
diff --git a/contrib/examples/git-remote.perl b/contrib/examples/git-remote.perl
index 5bf3ffd4c..2c8f18a13 100755
--- a/contrib/examples/git-remote.perl
+++ b/contrib/examples/git-remote.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 use warnings;
diff --git a/contrib/examples/git-rerere.perl b/contrib/examples/git-rerere.perl
index 4f692091e..110c27f4f 100755
--- a/contrib/examples/git-rerere.perl
+++ b/contrib/examples/git-rerere.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 #
 # REuse REcorded REsolve.  This tool records a conflicted automerge
 # result and its hand resolution, and helps to resolve future
diff --git a/contrib/examples/git-svnimport.perl b/contrib/examples/git-svnimport.perl
index c414f0d9c..d183a8d66 100755
--- a/contrib/examples/git-svnimport.perl
+++ b/contrib/examples/git-svnimport.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 # This tool is copyright (c) 2005, Matthias Urlichs.
 # It is released under the Gnu Public License, version 2.
diff --git a/contrib/fast-import/git-import.perl b/contrib/fast-import/git-import.perl
index 0891b9e36..440790523 100755
--- a/contrib/fast-import/git-import.perl
+++ b/contrib/fast-import/git-import.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 #
 # Performs an initial import of a directory. This is the equivalent
 # of doing 'git init; git add .; git commit'. It's a little slower,
diff --git a/contrib/fast-import/import-directories.perl b/contrib/fast-import/import-directories.perl
index 4dec1f18e..197307570 100755
--- a/contrib/fast-import/import-directories.perl
+++ b/contrib/fast-import/import-directories.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 #
 # Copyright 2008-2009 Peter Krefting <peter@softwolves.pp.se>
 #
diff --git a/contrib/fast-import/import-tars.perl b/contrib/fast-import/import-tars.perl
index d60b4315e..30f3ff384 100755
--- a/contrib/fast-import/import-tars.perl
+++ b/contrib/fast-import/import-tars.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 ## tar archive frontend for git-fast-import
 ##
diff --git a/contrib/hooks/setgitperms.perl b/contrib/hooks/setgitperms.perl
index 2770a1b1d..18444e015 100755
--- a/contrib/hooks/setgitperms.perl
+++ b/contrib/hooks/setgitperms.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 #
 # Copyright (c) 2006 Josh England
 #
diff --git a/contrib/hooks/update-paranoid b/contrib/hooks/update-paranoid
index d18b317b2..95595acf5 100755
--- a/contrib/hooks/update-paranoid
+++ b/contrib/hooks/update-paranoid
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 use File::Spec;
diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
index a677569dd..0ca49e370 100755
--- a/contrib/long-running-filter/example.pl
+++ b/contrib/long-running-filter/example.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 #
 # Example implementation for the Git filter protocol version 2
 # See Documentation/gitattributes.txt, section "Filter Protocol"
diff --git a/contrib/mw-to-git/git-mw.perl b/contrib/mw-to-git/git-mw.perl
index 28df3ee32..50c0549b5 100755
--- a/contrib/mw-to-git/git-mw.perl
+++ b/contrib/mw-to-git/git-mw.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 # Copyright (C) 2013
 #     Benoit Person <benoit.person@ensimag.imag.fr>
diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 41e74fba1..f8ce245cf 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -1,4 +1,4 @@
-#! /usr/bin/perl
+#!/usr/bin/env perl
 
 # Copyright (C) 2011
 #     Jérémie Nikaes <jeremie.nikaes@ensimag.imag.fr>
diff --git a/contrib/mw-to-git/t/test-gitmw.pl b/contrib/mw-to-git/t/test-gitmw.pl
index 8d0e7c078..a8272d942 100755
--- a/contrib/mw-to-git/t/test-gitmw.pl
+++ b/contrib/mw-to-git/t/test-gitmw.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 # Copyright (C) 2012
 #     Charles Roussel <charles.roussel@ensimag.imag.fr>
 #     Simon Cathebras <simon.cathebras@ensimag.imag.fr>
diff --git a/contrib/stats/mailmap.pl b/contrib/stats/mailmap.pl
index 9513f5e35..469af8240 100755
--- a/contrib/stats/mailmap.pl
+++ b/contrib/stats/mailmap.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use warnings 'all';
 use strict;
diff --git a/contrib/stats/packinfo.pl b/contrib/stats/packinfo.pl
index be188c0f1..51823ac94 100755
--- a/contrib/stats/packinfo.pl
+++ b/contrib/stats/packinfo.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 #
 # This tool will print vaguely pretty information about a pack.  It
 # expects the output of "git verify-pack -v" as input on stdin.
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index cf6fc926a..6d7b6c35d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use 5.008;
 use strict;
diff --git a/git-archimport.perl b/git-archimport.perl
index 9cb123a07..bb423e781 100755
--- a/git-archimport.perl
+++ b/git-archimport.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 #
 # This tool is copyright (c) 2005, Martin Langhoff.
 # It is released under the Gnu Public License, version 2.
diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index d13f02da9..c2ebfb59f 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use 5.008;
 use strict;
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 1e4e65a45..5dcdd8106 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 # This tool is copyright (c) 2005, Matthias Urlichs.
 # It is released under the Gnu Public License, version 2.
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index d50c85ed7..3ae5e748b 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 ####
 #### This application is a CVS emulation layer for git.
diff --git a/git-difftool.perl b/git-difftool.perl
index df59bdfe9..a34d0f7fd 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 # Copyright (c) 2009, 2010 David Aguilar
 # Copyright (c) 2012 Tim Henigan
 #
diff --git a/git-relink.perl b/git-relink.perl
index 236a3521a..b51348fa6 100755
--- a/git-relink.perl
+++ b/git-relink.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 # Copyright 2005, Ryan Anderson <ryan@michonline.com>
 # Distribution permitted under the GPL v2, as distributed
 # by the Free Software Foundation.
diff --git a/git-send-email.perl b/git-send-email.perl
index 068d60b3e..678d823af 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 #
 # Copyright 2002,2005 Greg Kroah-Hartman <greg@kroah.com>
 # Copyright 2005 Ryan Anderson <ryan@michonline.com>
diff --git a/git-svn.perl b/git-svn.perl
index fa4236478..ae3991524 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 # Copyright (C) 2006, Eric Wong <normalperson@yhbt.net>
 # License: GPL v2 or later
 use 5.008;
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7cf68f07b..d084360e6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 # gitweb - simple web interface to track changes in git repositories
 #
diff --git a/t/Git-SVN/Utils/can_compress.t b/t/Git-SVN/Utils/can_compress.t
index d7b49b8d5..0cdedb25b 100755
--- a/t/Git-SVN/Utils/can_compress.t
+++ b/t/Git-SVN/Utils/can_compress.t
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 use warnings;
diff --git a/t/Git-SVN/Utils/fatal.t b/t/Git-SVN/Utils/fatal.t
index 49e143829..75aaf1633 100755
--- a/t/Git-SVN/Utils/fatal.t
+++ b/t/Git-SVN/Utils/fatal.t
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 use warnings;
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b170cbc04..3c3e2e7d4 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 # Test t0000..t9999.sh for non portable shell scripts
 # This script can be called with one or more filenames as parameters
diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh
index d5dab5a94..b23843ff4 100644
--- a/t/gitweb-lib.sh
+++ b/t/gitweb-lib.sh
@@ -7,7 +7,7 @@
 gitweb_init () {
 	safe_pwd="$(perl -MPOSIX=getcwd -e 'print quotemeta(getcwd)')"
 	cat >gitweb_config.perl <<EOF
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 # gitweb configuration for tests
 
diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index 924b19dab..2c6278ca6 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use lib '../../perl/blib/lib';
 use strict;
diff --git a/t/perf/min_time.perl b/t/perf/min_time.perl
index c1a2717e0..2595eae61 100755
--- a/t/perf/min_time.perl
+++ b/t/perf/min_time.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 my $minrt = 1e100;
 my $min;
diff --git a/t/t0202/test.pl b/t/t0202/test.pl
index 2cbf7b959..d8e967bbc 100755
--- a/t/t0202/test.pl
+++ b/t/t0202/test.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 use 5.008;
 use lib (split(/:/, $ENV{GITPERLLIB}));
 use strict;
diff --git a/t/t4034/perl/post b/t/t4034/perl/post
index e8b72ef5d..87500971d 100644
--- a/t/t4034/perl/post
+++ b/t/t4034/perl/post
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 
diff --git a/t/t4034/perl/pre b/t/t4034/perl/pre
index f6610d37b..5ab5aa42f 100644
--- a/t/t4034/perl/pre
+++ b/t/t4034/perl/pre
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 
diff --git a/t/t9000/test.pl b/t/t9000/test.pl
index dfeaa9c65..4e47f3887 100755
--- a/t/t9000/test.pl
+++ b/t/t9000/test.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 use lib (split(/:/, $ENV{GITPERLLIB}));
 
 use 5.008;
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index 6d06ed96c..d8b5622b5 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -712,7 +712,7 @@ test_expect_success HIGHLIGHT \
 test_expect_success HIGHLIGHT \
 	'syntax highlighting (highlighter language autodetection)' \
 	'git config gitweb.highlight yes &&
-	 echo "#!/usr/bin/perl" > test &&
+	 echo "#!/usr/bin/env perl" > test &&
 	 git add test &&
 	 git commit -m "Add test" &&
 	 gitweb_run "p=.git;a=blob;f=test"'
diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 1b75c9196..c0be7e895 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 use lib (split(/:/, $ENV{GITPERLLIB}));
 
 use 5.008;
diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 96b6a03e1..d1d40c0db 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 use 5.008;
 use strict;
 use warnings;
-- 
2.11.0


^ permalink raw reply related

* [PATCH 0/2] Use env for all perl invocations
From: Pat Pannuto @ 2017-01-12  5:51 UTC (permalink / raw)
  To: gitster, git; +Cc: Pat Pannuto

I spent a little while debugging why git-format-patch refused to believe
that SSL support was installed (Can't locate Net/SMTP/SSL.pm in @INC...)
Turns out that it was installed for my system's preferred /usr/local/bin/perl,
but not for git-format-patch's hard-coded /usr/bin/perl; changing the shebang
allowed git format-patch to work as expected.

This patch set converts all perl invocations in git to use env so that the
user-preferred perl interpreter is always used.

Pat Pannuto (2):
  Convert all 'perl -w' to 'perl' + 'use warnings;'
  Use 'env' to find perl instead of fixed path

 Documentation/build-docdep.perl               | 2 +-
 Documentation/cat-texi.perl                   | 4 +++-
 Documentation/cmd-list.perl                   | 4 +++-
 Documentation/fix-texi.perl                   | 4 +++-
 Documentation/lint-gitlink.perl               | 2 +-
 compat/vcbuild/scripts/clink.pl               | 3 ++-
 compat/vcbuild/scripts/lib.pl                 | 3 ++-
 contrib/buildsystems/engine.pl                | 3 ++-
 contrib/buildsystems/generate                 | 3 ++-
 contrib/buildsystems/parse.pl                 | 3 ++-
 contrib/contacts/git-contacts                 | 2 +-
 contrib/credential/netrc/git-credential-netrc | 2 +-
 contrib/credential/netrc/test.pl              | 2 +-
 contrib/diff-highlight/diff-highlight         | 2 +-
 contrib/examples/git-remote.perl              | 3 ++-
 contrib/examples/git-rerere.perl              | 2 +-
 contrib/examples/git-svnimport.perl           | 2 +-
 contrib/fast-import/git-import.perl           | 2 +-
 contrib/fast-import/import-directories.perl   | 2 +-
 contrib/fast-import/import-tars.perl          | 2 +-
 contrib/hooks/setgitperms.perl                | 2 +-
 contrib/hooks/update-paranoid                 | 2 +-
 contrib/long-running-filter/example.pl        | 2 +-
 contrib/mw-to-git/git-mw.perl                 | 2 +-
 contrib/mw-to-git/git-remote-mediawiki.perl   | 2 +-
 contrib/mw-to-git/t/test-gitmw.pl             | 5 ++++-
 contrib/stats/mailmap.pl                      | 2 +-
 contrib/stats/packinfo.pl                     | 2 +-
 git-add--interactive.perl                     | 2 +-
 git-archimport.perl                           | 2 +-
 git-cvsexportcommit.perl                      | 2 +-
 git-cvsimport.perl                            | 2 +-
 git-cvsserver.perl                            | 2 +-
 git-difftool.perl                             | 2 +-
 git-relink.perl                               | 2 +-
 git-send-email.perl                           | 2 +-
 git-svn.perl                                  | 2 +-
 gitweb/gitweb.perl                            | 2 +-
 t/Git-SVN/Utils/can_compress.t                | 2 +-
 t/Git-SVN/Utils/fatal.t                       | 2 +-
 t/check-non-portable-shell.pl                 | 2 +-
 t/gitweb-lib.sh                               | 2 +-
 t/perf/aggregate.perl                         | 2 +-
 t/perf/min_time.perl                          | 2 +-
 t/t0202/test.pl                               | 2 +-
 t/t4034/perl/post                             | 2 +-
 t/t4034/perl/pre                              | 2 +-
 t/t9000/test.pl                               | 2 +-
 t/t9500-gitweb-standalone-no-errors.sh        | 2 +-
 t/t9700/test.pl                               | 2 +-
 t/test-terminal.perl                          | 2 +-
 51 files changed, 66 insertions(+), 51 deletions(-)

-- 
2.11.0


^ permalink raw reply

* [PATCH 1/2] Convert all 'perl -w' to 'perl' + 'use warnings;'
From: Pat Pannuto @ 2017-01-12  5:51 UTC (permalink / raw)
  To: gitster, git; +Cc: Pat Pannuto
In-Reply-To: <20170112055140.29877-1-pat.pannuto@gmail.com>

This commit is in preparation for converting all shebangs to use 'env'
instead of a fixed perl path, which will not allow for arguments to 'perl'.

Signed-off-by: Pat Pannuto <pat.pannuto@gmail.com>
---
 Documentation/cat-texi.perl       | 4 +++-
 Documentation/cmd-list.perl       | 4 +++-
 Documentation/fix-texi.perl       | 4 +++-
 compat/vcbuild/scripts/clink.pl   | 3 ++-
 compat/vcbuild/scripts/lib.pl     | 3 ++-
 contrib/buildsystems/engine.pl    | 3 ++-
 contrib/buildsystems/generate     | 3 ++-
 contrib/buildsystems/parse.pl     | 3 ++-
 contrib/examples/git-remote.perl  | 3 ++-
 contrib/mw-to-git/t/test-gitmw.pl | 5 ++++-
 10 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/Documentation/cat-texi.perl b/Documentation/cat-texi.perl
index 87437f8a9..1cd28b1b5 100755
--- a/Documentation/cat-texi.perl
+++ b/Documentation/cat-texi.perl
@@ -1,4 +1,6 @@
-#!/usr/bin/perl -w
+#!/usr/bin/perl
+
+use warnings;
 
 my @menu = ();
 my $output = $ARGV[0];
diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
index 5aa73cfe4..ba640a441 100755
--- a/Documentation/cmd-list.perl
+++ b/Documentation/cmd-list.perl
@@ -1,4 +1,6 @@
-#!/usr/bin/perl -w
+#!/usr/bin/perl
+
+use warnings;
 
 use File::Compare qw(compare);
 
diff --git a/Documentation/fix-texi.perl b/Documentation/fix-texi.perl
index ff7d78f62..c247aece7 100755
--- a/Documentation/fix-texi.perl
+++ b/Documentation/fix-texi.perl
@@ -1,4 +1,6 @@
-#!/usr/bin/perl -w
+#!/usr/bin/perl
+
+use warnings;
 
 while (<>) {
 	if (/^\@setfilename/) {
diff --git a/compat/vcbuild/scripts/clink.pl b/compat/vcbuild/scripts/clink.pl
index a87d0da51..46eb61c5c 100755
--- a/compat/vcbuild/scripts/clink.pl
+++ b/compat/vcbuild/scripts/clink.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl -w
+#!/usr/bin/perl
 ######################################################################
 # Compiles or links files
 #
@@ -10,6 +10,7 @@
 # Copyright (C) 2009 Marius Storm-Olsen <mstormo@gmail.com>
 ######################################################################
 use strict;
+use warnings;
 my @args = ();
 my @cflags = ();
 my $is_linking = 0;
diff --git a/compat/vcbuild/scripts/lib.pl b/compat/vcbuild/scripts/lib.pl
index d8054e469..e571b8470 100755
--- a/compat/vcbuild/scripts/lib.pl
+++ b/compat/vcbuild/scripts/lib.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl -w
+#!/usr/bin/perl
 ######################################################################
 # Libifies files on Windows
 #
@@ -10,6 +10,7 @@
 # Copyright (C) 2009 Marius Storm-Olsen <mstormo@gmail.com>
 ######################################################################
 use strict;
+use warnings;
 my @args = ();
 while (@ARGV) {
 	my $arg = shift @ARGV;
diff --git a/contrib/buildsystems/engine.pl b/contrib/buildsystems/engine.pl
index 23da787dc..a173669ce 100755
--- a/contrib/buildsystems/engine.pl
+++ b/contrib/buildsystems/engine.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl -w
+#!/usr/bin/perl
 ######################################################################
 # Do not call this script directly!
 #
@@ -8,6 +8,7 @@
 # Copyright (C) 2009 Marius Storm-Olsen <mstormo@gmail.com>
 ######################################################################
 use strict;
+use warnings;
 use File::Basename;
 use File::Spec;
 use Cwd;
diff --git a/contrib/buildsystems/generate b/contrib/buildsystems/generate
index bc10f25ff..9af89454a 100755
--- a/contrib/buildsystems/generate
+++ b/contrib/buildsystems/generate
@@ -1,4 +1,4 @@
-#!/usr/bin/perl -w
+#!/usr/bin/perl
 ######################################################################
 # Generate buildsystem files
 #
@@ -19,6 +19,7 @@
 # Copyright (C) 2009 Marius Storm-Olsen <mstormo@gmail.com>
 ######################################################################
 use strict;
+use warnings;
 use File::Basename;
 use Cwd;
 
diff --git a/contrib/buildsystems/parse.pl b/contrib/buildsystems/parse.pl
index c9656ece9..33ca89eb0 100755
--- a/contrib/buildsystems/parse.pl
+++ b/contrib/buildsystems/parse.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl -w
+#!/usr/bin/perl
 ######################################################################
 # Do not call this script directly!
 #
@@ -8,6 +8,7 @@
 # Copyright (C) 2009 Marius Storm-Olsen <mstormo@gmail.com>
 ######################################################################
 use strict;
+use warnings;
 use File::Basename;
 use Cwd;
 
diff --git a/contrib/examples/git-remote.perl b/contrib/examples/git-remote.perl
index d42df7b41..5bf3ffd4c 100755
--- a/contrib/examples/git-remote.perl
+++ b/contrib/examples/git-remote.perl
@@ -1,6 +1,7 @@
-#!/usr/bin/perl -w
+#!/usr/bin/perl
 
 use strict;
+use warnings;
 use Git;
 my $git = Git->repository();
 
diff --git a/contrib/mw-to-git/t/test-gitmw.pl b/contrib/mw-to-git/t/test-gitmw.pl
index 0ff76259f..8d0e7c078 100755
--- a/contrib/mw-to-git/t/test-gitmw.pl
+++ b/contrib/mw-to-git/t/test-gitmw.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl -w -s
+#!/usr/bin/perl
 # Copyright (C) 2012
 #     Charles Roussel <charles.roussel@ensimag.imag.fr>
 #     Simon Cathebras <simon.cathebras@ensimag.imag.fr>
@@ -22,6 +22,9 @@
 #     "edit_page"
 #     "getallpagename"
 
+use strict;
+use warnings;
+
 use MediaWiki::API;
 use Getopt::Long;
 use encoding 'utf8';
-- 
2.11.0


^ permalink raw reply related

* Re: [ANNOUNCE] git-test: run automated tests against a range of Git commits
From: Michael Haggerty @ 2017-01-12  5:02 UTC (permalink / raw)
  To: Jeff King; +Cc: git discussion list
In-Reply-To: <20170110093502.wbfgof55gdo6mtov@sigill.intra.peff.net>

On 01/10/2017 10:35 AM, Jeff King wrote:
> On Sun, Jan 08, 2017 at 10:52:25AM +0100, Michael Haggerty wrote:
>> [...]

Since my last email, I have implemented a bunch of what we discussed
[1]. Because of the new semantics, I also *renamed the main command* from

    git test range [...]

to

    git test run [...]

>> I'm thinking of maybe
>>
>> * If an argument matches `*..*`, pass it to `rev-list` (like now).

This was already implemented.

>> * Otherwise, treat each argument as a single commit/tree (i.e., pass it
>> to `rev-parse`).

This is now implemented, too. Plus, it is now allowed to specify
multiple commits and/or ranges in a single invocation.

>> * If no argument is specified, test `@{u}..` (assuming that an
>>   upstream is configured). Though actually, this won't be as
>>   convenient as it sounds, because (a) `git test` is often run
>>   in a separate worktree, and (2) on errors, it currently leaves the
>>   repository with a detached `HEAD`.

I decided that if no argument is specified, it makes more sense to test
HEAD if the working tree is clean, and the contents of the working tree
otherwise. In the latter case no results are stored to notes, but it is
still useful to be able to run a preconfigured test quickly while
working. These are both implemented.

>> * Support a `--stdin` option, to read a list of commits/trees to test
>>   from standard input. By this mechanism, users could use arbitrary
>>   `rev-list` commands to choose what to test.

This is now implemented, too.

> [...]
>> I think ideally `git notes add` would look for pre-existing notes, and:
>>
>> * If none are found, create an empty notes reference.
>>
>> * If pre-existing notes are found and there was no existing test with
>>   that name, probably just leave the old notes in place.
>>
>> * If pre-existing notes are found and there was already a test with
>>   that name but a different command, perhaps insist that the user
>>   decide explicitly whether to forget the old results or continue using
>>   them. This might help users avoid the mistake of re-using old results
>>   even if they change the manner of testing.
> 
> I'm not quite sure what you mean here. By "test" and "command", do you
> mean the test name that is used in the notes ref, and the command that
> it is defined as?

By "test", I mean a test as configured in `git-test` and referred to by
its name (e.g., in `--test=name`). Currently the only configuration for
a test is `test.<name>.command`, but I structured the namespace that way
to leave room to add more configuration in the future.

> In the notes-cache.c subsystem, the commit message stores a validity
> token which must match in order to use the cache. You could do something
> similar here (store the executed command in the commit message, and
> invalidate the cache the user has changed the command). The notes-cache
> stuff isn't available outside of the C code, though. You could either
> expose it, or just do something similar longhand.
> 
> Thinking about it, though, I did notice that the tree sha1 is not the
> only input to the cache. Things like config.mak (not to mention the
> system itself) contribute to the test results. So no system will ever be
> perfect, and it seems like just making an easy way to force a retest (or
> just invalidate the whole cache) would be sufficient.

It's true that the tree and the test command are not the only inputs to
the testing machinery. I wasn't hoping to build a watertight system to
prevent accidental use of old results when `config.mak` or something
else in the environment has changed. I was only trying to give the user
a reminder that if they change the command, it's a good time to consider
whether the old results have to be invalidated.

I suppose if we wanted to make this system more watertight, we could let
the test configuration specify the names of other files on which its
results depend; for example,

    test.default.command = make -j16 test
    test.default.auxiliaryInput = config.mak

and include some kind of hash of the auxiliary inputs in a validity
token. But that feels like overkill.

> [...]
>> Yeah, this is awkward, not only because many people don't know what to
>> make of detached HEAD, but also because it makes it awkward in general
>> to use `git test` in your main working directory. I didn't model this
>> behavior on `git rebase --interactive`'s `edit` command, because I
>> rarely use that. But I can see how they would fit together pretty well
>> for people who like that workflow.
> 
> Yeah, after sleeping on it, I think it's best if "git test" remains
> separate from that. It's primary function is to run the test, possibly
> serving up a cached answer. So it would be perfectly reasonable to do:
> 
>   git rebase -x 'git test range HEAD'
> 
> to accomplish the interactive testing (though perhaps just "git test"
> would be a nice synonym for that).

That invocation can now be written `git test run`, which is a bit
shorter. There could be a special case that `git test` is shorthand for
`git test run`, but it quickly gets ambiguous if the user wants to start
adding any `run` arguments.

> And then "jump to a thing that I know is broken" becomes a separate
> action, whether you are using "git test" or not. I wonder if we could
> have:
> 
>   git rebase -e HEAD~2
> 
> to do an interactive rebase, with "edit" for HEAD~2. I feel like
> somebody even proposed that at one point, but I don't think it got
> merged.

That sounds handy. (The GIT_EDITOR thing is pretty hacky.)

> And then "git test fix" basically becomes:
> 
>   git rebase -e "$(git test first-broken)"
> 
> though I think you'd still want a shorthand for that.

There could be a `git test fix --rebase <range>` that does this. Plus my
preference, `git test fix --branch=fix <range>`, which creates a branch
named `fix` at the first broken commit and checks it out. (If the fix is
nontrivial, my next step is usually to `git imerge rebase` the original
branch onto the `fix` branch.)

>>> I think it should be possible to script the next steps, though.
>>> Something like like "git test fix foo", which would: [...]
>>
>> I think you would usually only want to mark only the *first* broken
>> commit as "edit", because often errors cascade to descendant commits.
> 
> Yeah, I had a similar thought. OTOH, if you didn't say "--keep-going",
> you'd only have one breakage either way. [...]

True enough.

Thanks for all your feedback!

Michael

[1] https://github.com/mhagger/git-test


^ permalink raw reply

* [PATCH 1/5] doc: add documentation for OPT_STRING_LIST
From: Jacob Keller @ 2017-01-12  0:17 UTC (permalink / raw)
  To: git; +Cc: Jacob Keller
In-Reply-To: <20170112001721.2534-1-jacob.e.keller@intel.com>

From: Jacob Keller <jacob.keller@gmail.com>

Commit c8ba16391655 ("parse-options: add OPT_STRING_LIST helper",
2011-06-09) added the OPT_STRING_LIST as a way to accumulate a repeated
list of strings. However, this was not documented in the
api-parse-options documentation. Add documentation now so that future
developers may learn of its existence.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/technical/api-parse-options.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 27bd701c0d68..15e876e4c804 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -168,6 +168,11 @@ There are some macros to easily define options:
 	Introduce an option with string argument.
 	The string argument is put into `str_var`.
 
+`OPT_STRING_LIST(short, long, &list, arg_str, description)`::
+	Introduce an option with a string argument. Repeated invocations
+	accumulate into a list of strings. Reset and clear the list with
+	`--no-option`.
+
 `OPT_INTEGER(short, long, &int_var, description)`::
 	Introduce an option with integer argument.
 	The integer is put into `int_var`.
-- 
2.11.0.403.g196674b8396b


^ permalink raw reply related

* [PATCH 3/5] name-rev: add support to discard refs by pattern match
From: Jacob Keller @ 2017-01-12  0:17 UTC (permalink / raw)
  To: git; +Cc: Jacob Keller
In-Reply-To: <20170112001721.2534-1-jacob.e.keller@intel.com>

From: Jacob Keller <jacob.keller@gmail.com>

Extend name-rev further to support matching refs by adding `--discard`
patterns. These patterns will limit the scope of refs by discarding any
ref that matches at least one discard pattern. Checking the discard refs
shall happen first, before checking the include --refs patterns. This
will allow more flexibility to matching certain kinds of references.

Add tests and update Documentation for this change.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/git-name-rev.txt       |  7 +++++++
 builtin/name-rev.c                   | 14 +++++++++++++-
 t/t6007-rev-list-cherry-pick-file.sh |  7 +++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index 7433627db12d..9b46e5ea9aae 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -30,6 +30,13 @@ OPTIONS
 	given multiple times, use refs whose names match any of the given shell
 	patterns. Use `--no-refs` to clear any previous ref patterns given.
 
+--discard=<pattern>::
+	Do not use any ref whose name matches a given shell pattern. The
+	pattern can be one of branch name, tag name or fully qualified ref
+	name. If given multiple times, discard refs that match any of the given
+	shell patterns. Use `--no-discards` to clear the list of discard
+	patterns.
+
 --all::
 	List all commits reachable from all refs
 
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 000a2a700ed3..86479c17a7c9 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -109,6 +109,7 @@ struct name_ref_data {
 	int tags_only;
 	int name_only;
 	struct string_list ref_filters;
+	struct string_list discard_filters;
 };
 
 static struct tip_table {
@@ -150,6 +151,15 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
 	if (data->tags_only && !starts_with(path, "refs/tags/"))
 		return 0;
 
+	if (data->discard_filters.nr) {
+		struct string_list_item *item;
+
+		for_each_string_list_item(item, &data->discard_filters) {
+			if (subpath_matches(path, item->string) >= 0)
+				return 0;
+		}
+	}
+
 	if (data->ref_filters.nr) {
 		struct string_list_item *item;
 		int matched = 0;
@@ -323,12 +333,14 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 {
 	struct object_array revs = OBJECT_ARRAY_INIT;
 	int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
-	struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP };
+	struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP };
 	struct option opts[] = {
 		OPT_BOOL(0, "name-only", &data.name_only, N_("print only names (no SHA-1)")),
 		OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name the commits")),
 		OPT_STRING_LIST(0, "refs", &data.ref_filters, N_("pattern"),
 				   N_("only use refs matching <pattern>")),
+		OPT_STRING_LIST(0, "discard", &data.discard_filters, N_("pattern"),
+				   N_("ignore refs matching <pattern>")),
 		OPT_GROUP(""),
 		OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
 		OPT_BOOL(0, "stdin", &transform_stdin, N_("read from stdin")),
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index d072ec43b016..8a4c35f6ffee 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -118,6 +118,13 @@ test_expect_success 'name-rev --refs excludes non-matched patterns' '
 	test_cmp actual.named expect
 '
 
+test_expect_success 'name-rev --discard excludes matched patterns' '
+	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" --discard="*E" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
 cat >expect <<EOF
 $(git rev-list --left-right --cherry-pick F...E -- bar)
 EOF
-- 
2.11.0.403.g196674b8396b


^ permalink raw reply related

* [PATCH 2/5] name-rev: extend --refs to accept multiple patterns
From: Jacob Keller @ 2017-01-12  0:17 UTC (permalink / raw)
  To: git; +Cc: Jacob Keller
In-Reply-To: <20170112001721.2534-1-jacob.e.keller@intel.com>

From: Jacob Keller <jacob.keller@gmail.com>

Teach git name-rev to take a string list of patterns from --refs instead
of only a single pattern. The list of patterns will be matched
inclusively, such that a ref only needs to match one pattern to be
included. If a ref will only be excluded if it does not match any of the
patterns.

Add tests and documentation for this change.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/git-name-rev.txt       |  4 +++-
 builtin/name-rev.c                   | 41 +++++++++++++++++++++++++-----------
 t/t6007-rev-list-cherry-pick-file.sh | 30 ++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index ca28fb8e2a07..7433627db12d 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -26,7 +26,9 @@ OPTIONS
 
 --refs=<pattern>::
 	Only use refs whose names match a given shell pattern.  The pattern
-	can be one of branch name, tag name or fully qualified ref name.
+	can be one of branch name, tag name or fully qualified ref name. If
+	given multiple times, use refs whose names match any of the given shell
+	patterns. Use `--no-refs` to clear any previous ref patterns given.
 
 --all::
 	List all commits reachable from all refs
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48b65e8..000a2a700ed3 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -108,7 +108,7 @@ static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
 struct name_ref_data {
 	int tags_only;
 	int name_only;
-	const char *ref_filter;
+	struct string_list ref_filters;
 };
 
 static struct tip_table {
@@ -150,16 +150,33 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
 	if (data->tags_only && !starts_with(path, "refs/tags/"))
 		return 0;
 
-	if (data->ref_filter) {
-		switch (subpath_matches(path, data->ref_filter)) {
-		case -1: /* did not match */
-			return 0;
-		case 0:  /* matched fully */
-			break;
-		default: /* matched subpath */
-			can_abbreviate_output = 1;
-			break;
+	if (data->ref_filters.nr) {
+		struct string_list_item *item;
+		int matched = 0;
+
+		/* See if any of the patterns match. */
+		for_each_string_list_item(item, &data->ref_filters) {
+			/*
+			 * We want to check every pattern even if we already
+			 * found a match, just in case one of the later
+			 * patterns could abbreviate the output.
+			 */
+			switch (subpath_matches(path, item->string)) {
+			case -1: /* did not match */
+				break;
+			case 0: /* matched fully */
+				matched = 1;
+				break;
+			default: /* matched subpath */
+				matched = 1;
+				can_abbreviate_output = 1;
+				break;
+			}
 		}
+
+		/* If none of the patterns matched, stop now */
+		if (!matched)
+			return 0;
 	}
 
 	add_to_tip_table(oid->hash, path, can_abbreviate_output);
@@ -306,11 +323,11 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 {
 	struct object_array revs = OBJECT_ARRAY_INIT;
 	int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
-	struct name_ref_data data = { 0, 0, NULL };
+	struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP };
 	struct option opts[] = {
 		OPT_BOOL(0, "name-only", &data.name_only, N_("print only names (no SHA-1)")),
 		OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name the commits")),
-		OPT_STRING(0, "refs", &data.ref_filter, N_("pattern"),
+		OPT_STRING_LIST(0, "refs", &data.ref_filters, N_("pattern"),
 				   N_("only use refs matching <pattern>")),
 		OPT_GROUP(""),
 		OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 1408b608eb03..d072ec43b016 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -99,6 +99,36 @@ test_expect_success '--cherry-pick bar does not come up empty (II)' '
 	test_cmp actual.named expect
 '
 
+test_expect_success 'name-rev multiple --refs combine inclusive' '
+	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+<tags/F
+$(git rev-list --left-right --right-only --cherry-pick F...E -- bar)
+EOF
+
+test_expect_success 'name-rev --refs excludes non-matched patterns' '
+	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/F" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+$(git rev-list --left-right --cherry-pick F...E -- bar)
+EOF
+
+test_expect_success 'name-rev --no-refs clears the refs list' '
+	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
 cat >expect <<EOF
 +tags/F
 =tags/D
-- 
2.11.0.403.g196674b8396b


^ permalink raw reply related

* [PATCH 4/5] describe: teach --match to accept multiple patterns
From: Jacob Keller @ 2017-01-12  0:17 UTC (permalink / raw)
  To: git; +Cc: Jacob Keller
In-Reply-To: <20170112001721.2534-1-jacob.e.keller@intel.com>

From: Jacob Keller <jacob.keller@gmail.com>

Teach `--match` to be accepted multiple times, accumulating a list of
patterns to match into a string list. Each pattern is inclusive, such
that a tag need only match one of the provided patterns to be
considered for matching.

This extension is useful as it enables more flexibility in what tags
match, and may avoid the need to run the describe command multiple
times to get the same result.

Add tests and update the documentation for this change.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/git-describe.txt |  5 ++++-
 builtin/describe.c             | 30 +++++++++++++++++++++++-------
 t/t6120-describe.sh            | 19 +++++++++++++++++++
 3 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index e4ac448ff565..7ad41e2f6ade 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -83,7 +83,10 @@ OPTIONS
 --match <pattern>::
 	Only consider tags matching the given `glob(7)` pattern,
 	excluding the "refs/tags/" prefix.  This can be used to avoid
-	leaking private tags from the repository.
+	leaking private tags from the repository. If given multiple times, a
+	list of patterns will be accumulated, and tags matching any of the
+	patterns will be considered. Use `--no-match` to clear and reset the
+	list of patterns.
 
 --always::
 	Show uniquely abbreviated commit object as fallback.
diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a157efc..5cc9e9abe798 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -28,7 +28,7 @@ static int abbrev = -1; /* unspecified */
 static int max_candidates = 10;
 static struct hashmap names;
 static int have_util;
-static const char *pattern;
+static struct string_list patterns = STRING_LIST_INIT_NODUP;
 static int always;
 static const char *dirty;
 
@@ -129,9 +129,24 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi
 	if (!all && !is_tag)
 		return 0;
 
-	/* Accept only tags that match the pattern, if given */
-	if (pattern && (!is_tag || wildmatch(pattern, path + 10, 0, NULL)))
-		return 0;
+	/*
+	 * If we're given patterns, accept only tags which match at least one
+	 * pattern.
+	 */
+	if (patterns.nr) {
+		struct string_list_item *item;
+
+		if (!is_tag)
+			return 0;
+
+		for_each_string_list_item(item, &patterns) {
+			if (!wildmatch(item->string, path + 10, 0, NULL))
+				break;
+
+			/* If we get here, no pattern matched. */
+			return 0;
+		}
+	}
 
 	/* Is it annotated? */
 	if (!peel_ref(path, peeled.hash)) {
@@ -404,7 +419,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			    N_("only output exact matches"), 0),
 		OPT_INTEGER(0, "candidates", &max_candidates,
 			    N_("consider <n> most recent tags (default: 10)")),
-		OPT_STRING(0, "match",       &pattern, N_("pattern"),
+		OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
 			   N_("only consider tags matching <pattern>")),
 		OPT_BOOL(0, "always",        &always,
 			N_("show abbreviated commit object as fallback")),
@@ -430,6 +445,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		die(_("--long is incompatible with --abbrev=0"));
 
 	if (contains) {
+		struct string_list_item *item;
 		struct argv_array args;
 
 		argv_array_init(&args);
@@ -440,8 +456,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			argv_array_push(&args, "--always");
 		if (!all) {
 			argv_array_push(&args, "--tags");
-			if (pattern)
-				argv_array_pushf(&args, "--refs=refs/tags/%s", pattern);
+			for_each_string_list_item(item, &patterns)
+				argv_array_pushf(&args, "--refs=refs/tags/%s", item->string);
 		}
 		if (argc)
 			argv_array_pushv(&args, argv);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 85f269411cb3..9e5db9b87a1f 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -182,6 +182,10 @@ check_describe "test2-lightweight-*" --tags --match="test2-*"
 
 check_describe "test2-lightweight-*" --long --tags --match="test2-*" HEAD^
 
+check_describe "test1-lightweight-*" --long --tags --match="test1-*" --match="test2-*" HEAD^
+
+check_describe "test2-lightweight-*" --long --tags --match="test1-*" --no-match --match="test2-*" HEAD^
+
 test_expect_success 'name-rev with exact tags' '
 	echo A >expect &&
 	tag_object=$(git rev-parse refs/tags/A) &&
@@ -206,4 +210,19 @@ test_expect_success 'describe --contains with the exact tags' '
 	test_cmp expect actual
 '
 
+test_expect_success 'describe --contains and --match' '
+	echo "A^0" >expect &&
+	tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+	test_must_fail git describe --contains --match="B" $tagged_commit &&
+	git describe --contains --match="B" --match="A" $tagged_commit >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'describe --contains and --no-match' '
+	echo "A^0" >expect &&
+	tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+	git describe --contains --match="B" --no-match $tagged_commit >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.403.g196674b8396b


^ permalink raw reply related

* [PATCH 5/5] describe: teach describe negative pattern matches
From: Jacob Keller @ 2017-01-12  0:17 UTC (permalink / raw)
  To: git; +Cc: Jacob Keller
In-Reply-To: <20170112001721.2534-1-jacob.e.keller@intel.com>

From: Jacob Keller <jacob.keller@gmail.com>

Teach git-describe the `--discard` option which will allow specifying
a glob pattern of tags to ignore. This can be combined with the
`--match` patterns to enable more flexibility in determining which tags
to consider.

For example, suppose you wish to find the first official release tag
that contains a certain commit. If we assume that official release tags
are of the form "v*" and pre-release candidates include "*rc*" in their
name, we can now find the first tag that introduces commit abcdef via:

  git describe --contains --match="v*" --discard="*rc*"

Add documentation and tests for this change.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/git-describe.txt |  8 ++++++++
 builtin/describe.c             | 21 +++++++++++++++++++++
 t/t6120-describe.sh            |  8 ++++++++
 3 files changed, 37 insertions(+)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index 7ad41e2f6ade..a89bbde207b2 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -88,6 +88,14 @@ OPTIONS
 	patterns will be considered. Use `--no-match` to clear and reset the
 	list of patterns.
 
+--discard <pattern>::
+	Do not consider tags matching the given `glob(7)` pattern, excluding
+	the "refs/tags/" prefix. This can be used to narrow the tag space and
+	find only tags matching some meaningful criteria. If given multiple
+	times, a list of patterns will be accumulated and tags matching any
+	of the patterns will be discarded. Use `--no-discard` to clear and
+	reset the list of patterns.
+
 --always::
 	Show uniquely abbreviated commit object as fallback.
 
diff --git a/builtin/describe.c b/builtin/describe.c
index 5cc9e9abe798..c09288ee6321 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -29,6 +29,7 @@ static int max_candidates = 10;
 static struct hashmap names;
 static int have_util;
 static struct string_list patterns = STRING_LIST_INIT_NODUP;
+static struct string_list discard_patterns = STRING_LIST_INIT_NODUP;
 static int always;
 static const char *dirty;
 
@@ -130,6 +131,22 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi
 		return 0;
 
 	/*
+	 * If we're given discard patterns, first discard any tag which match
+	 * any of the discard pattern.
+	 */
+	if (discard_patterns.nr) {
+		struct string_list_item *item;
+
+		if (!is_tag)
+			return 0;
+
+		for_each_string_list_item(item, &discard_patterns) {
+			if (!wildmatch(item->string, path + 10, 0, NULL))
+				return 0;
+		}
+	}
+
+	/*
 	 * If we're given patterns, accept only tags which match at least one
 	 * pattern.
 	 */
@@ -421,6 +438,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			    N_("consider <n> most recent tags (default: 10)")),
 		OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
 			   N_("only consider tags matching <pattern>")),
+		OPT_STRING_LIST(0, "discard", &discard_patterns, N_("pattern"),
+			   N_("do not consider tags matching <pattern>")),
 		OPT_BOOL(0, "always",        &always,
 			N_("show abbreviated commit object as fallback")),
 		{OPTION_STRING, 0, "dirty",  &dirty, N_("mark"),
@@ -458,6 +477,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			argv_array_push(&args, "--tags");
 			for_each_string_list_item(item, &patterns)
 				argv_array_pushf(&args, "--refs=refs/tags/%s", item->string);
+			for_each_string_list_item(item, &discard_patterns)
+				argv_array_pushf(&args, "--discard=refs/tags/%s", item->string);
 		}
 		if (argc)
 			argv_array_pushv(&args, argv);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 9e5db9b87a1f..4e4a9f2e5305 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -218,6 +218,14 @@ test_expect_success 'describe --contains and --match' '
 	test_cmp expect actual
 '
 
+test_expect_success 'describe --discard' '
+	echo "c~1" >expect &&
+	tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+	test_must_fail git describe --contains --match="B" $tagged_commit &&
+	git describe --contains --match="?" --discard="A" $tagged_commit >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'describe --contains and --no-match' '
 	echo "A^0" >expect &&
 	tagged_commit=$(git rev-parse "refs/tags/A^0") &&
-- 
2.11.0.403.g196674b8396b


^ permalink raw reply related

* [PATCH 0/5] extend git-describe pattern matching
From: Jacob Keller @ 2017-01-12  0:17 UTC (permalink / raw)
  To: git; +Cc: Jacob Keller

From: Jacob Keller <jacob.keller@gmail.com>

Teach git describe and git name-rev the ability to match multiple
patterns inclusively. Additionally, teach these commands to also accept
negative patterns to discard any refs which match.

The pattern lists for positive and negative patterns are inclusive. This
means that for the positive patterns, a reference will be considered as
long as it matches at least one of the match patterns. It need not match
all given patterns. Additionally for negative patterns, we will not
consider any ref which matches any negative pattern, even if it matches
one of the positive patterns.

Together this allows the ability to express far more sets of tags than a
single match pattern alone. It does not provide quite the same depth as
would teaching full regexp but it is simpler and easy enough to
understand.

This is a re-send of a series from a month or so ago, I've since
re-based this on next since it appears that it was not picked up before.

Jacob Keller (5):
  doc: add documentation for OPT_STRING_LIST
  name-rev: extend --refs to accept multiple patterns
  name-rev: add support to discard refs by pattern match
  describe: teach --match to accept multiple patterns
  describe: teach describe negative pattern matches

 Documentation/git-describe.txt                | 13 ++++++-
 Documentation/git-name-rev.txt                | 11 +++++-
 Documentation/technical/api-parse-options.txt |  5 +++
 builtin/describe.c                            | 51 ++++++++++++++++++++++----
 builtin/name-rev.c                            | 53 +++++++++++++++++++++------
 t/t6007-rev-list-cherry-pick-file.sh          | 37 +++++++++++++++++++
 t/t6120-describe.sh                           | 27 ++++++++++++++
 7 files changed, 176 insertions(+), 21 deletions(-)

-- 
2.11.0.403.g196674b8396b


^ permalink raw reply

* [PATCHv2 4/4] unpack-trees: support super-prefix option
From: Stefan Beller @ 2017-01-12  0:12 UTC (permalink / raw)
  To: gitster; +Cc: git, bmwill, novalis, Stefan Beller
In-Reply-To: <xmqq37gpnuyi.fsf@gitster.mtv.corp.google.com>

In the future we want to support working tree operations within submodules,
e.g. "git checkout --recurse-submodules", which will update the submodule
to the commit as recorded in its superproject. In the submodule the
unpack-tree operation is carried out as usual, but the reporting to the
user needs to prefix any path with the superproject. The mechanism for
this is the super-prefix. (see 74866d757, git: make super-prefix option)

Add support for the super-prefix option for commands that unpack trees
by wrapping any path output in unpacking trees in the newly introduced
super_prefixed function. This new function prefixes any path with the
super-prefix if there is one.  Assuming the submodule case doesn't happen
in the majority of the cases, we'd want to have a fast behavior for no
super prefix, i.e. no reallocation/copying, but just returning path.

Another aspect of introducing the `super_prefixed` function is to consider
who owns the memory and if this is the right place where the path gets
modified. As the super prefix ought to change the output behavior only and
not the actual unpack tree part, it is fine to be that late in the line.
As we get passed in 'const char *path', we cannot change the path itself,
which means in case of a super prefix we have to copy over the path.
We need two static buffers in that function as the error messages
contain at most two paths.

For testing purposes enable it in read-tree, which has no output
of paths other than an unpack-trees.c. These are all converted in
this patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

This is only patchv4 that is rerolled, patches 1-3 remain as is.

The test uses '\'' now, and the super_prefixed function is rewritten
using the flow Junio suggested.

The commit message got enhanced.

Thanks,
Stefan

 git.c                       |  2 +-
 t/t1001-read-tree-m-2way.sh |  8 ++++++++
 unpack-trees.c              | 38 +++++++++++++++++++++++++++++++++++---
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index dce529fcbf..acbabd1298 100644
--- a/git.c
+++ b/git.c
@@ -471,7 +471,7 @@ static struct cmd_struct commands[] = {
 	{ "prune-packed", cmd_prune_packed, RUN_SETUP },
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
-	{ "read-tree", cmd_read_tree, RUN_SETUP },
+	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
 	{ "remote", cmd_remote, RUN_SETUP },
diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 7b70089705..5ededd8e40 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -363,6 +363,14 @@ test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
 	test -f a/b
 '
 
+test_expect_success 'read-tree supports the super-prefix' '
+	cat <<-EOF >expect &&
+		error: Updating '\''fictional/a'\'' would lose untracked files in it
+	EOF
+	test_must_fail git --super-prefix fictional/ read-tree -u -m "$treeH" "$treeM" 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'a/b vs a, plus c/d case setup.' '
 	rm -f .git/index &&
 	rm -fr a &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 7a6df99d10..0a24472359 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -52,6 +52,36 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
 	  ? ((o)->msgs[(type)])      \
 	  : (unpack_plumbing_errors[(type)]) )
 
+static const char *super_prefixed(const char *path)
+{
+	static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
+	static int super_prefix_len = -1;
+	static unsigned idx = 0;
+
+	if (super_prefix_len < 0) {
+		if (!get_super_prefix())
+			super_prefix_len = 0;
+		else {
+			int i;
+
+			super_prefix_len = strlen(get_super_prefix());
+			for (i = 0; i < ARRAY_SIZE(buf); i++)
+				strbuf_addstr(&buf[i], get_super_prefix());
+		}
+	}
+
+	if (!super_prefix_len)
+		return path;
+
+	if (++idx >= ARRAY_SIZE(buf))
+		idx = 0;
+
+	strbuf_setlen(&buf[idx], super_prefix_len);
+	strbuf_addstr(&buf[idx], path);
+
+	return buf[idx].buf;
+}
+
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 				  const char *cmd)
 {
@@ -172,7 +202,7 @@ static int add_rejected_path(struct unpack_trees_options *o,
 			     const char *path)
 {
 	if (!o->show_all_errors)
-		return error(ERRORMSG(o, e), path);
+		return error(ERRORMSG(o, e), super_prefixed(path));
 
 	/*
 	 * Otherwise, insert in a list for future display by
@@ -196,7 +226,7 @@ static void display_error_msgs(struct unpack_trees_options *o)
 			something_displayed = 1;
 			for (i = 0; i < rejects->nr; i++)
 				strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
-			error(ERRORMSG(o, e), path.buf);
+			error(ERRORMSG(o, e), super_prefixed(path.buf));
 			strbuf_release(&path);
 		}
 		string_list_clear(rejects, 0);
@@ -1918,7 +1948,9 @@ int bind_merge(const struct cache_entry * const *src,
 			     o->merge_size);
 	if (a && old)
 		return o->gently ? -1 :
-			error(ERRORMSG(o, ERROR_BIND_OVERLAP), a->name, old->name);
+			error(ERRORMSG(o, ERROR_BIND_OVERLAP),
+			      super_prefixed(a->name),
+			      super_prefixed(old->name));
 	if (!a)
 		return keep_entry(old, o);
 	else
-- 
2.11.0.259.g7b30ecf4f0


^ permalink raw reply related

* Re: [PATCH 4/4] unpack-trees: support super-prefix option
From: Stefan Beller @ 2017-01-11 23:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Brandon Williams, David Turner, git@vger.kernel.org
In-Reply-To: <xmqqh955mb1p.fsf@gitster.mtv.corp.google.com>

On Wed, Jan 11, 2017 at 3:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> Preparing the expected output "expect" outside test_expect_success
>>> block is also old-school.  Move it inside the new test?
>>
>> I looked into that. What is our current stance on using single/double quotes?
>
> Using dq around executable part, i.e.
>
>         test_expect_success title "
>                 echo \"'foo'\"
>         "
>
> is a million-times more grave sin even if the test initially does
> not refer to any variable in its body.  Somebody will eventually
> need to add a line that refers to a variable and either forgets to
> quote \$ in front or properly quotes it.

agreed.

>  The former makes the
> variable interpolated while the arguments to the test_expect_success
> is prepared (which is a bug) and the latter makes the result quite
> ugly, like so:
>
>         test_expect_success title "
>                 sq=\' var=foo &&
>                 echo \"\${sq}\$value\${sq}\"
>         "
>
> Enclosing the body always in sq-pair does mean something ugly like
> this from the beginning once you need to use sq inside:
>
>         test_expect_success title '
>                 sq='\'' &&
>                 echo "${sq}foo${sq}"
>         '

This one fails
error: bug in the test script: broken &&-chain:
                sq=' &&
                echo "${sq}foo${sq}"
both other occurrences of using ${sq} are defining sq outside
(one as sq=\' and the other as sq="'")

So I think we either have to keep sq out of the test,
    sq="'"
    test_expect_success title '
         echo "${sq}foo${sq}"
    '

or do the echo/printf trick inside:

    test_expect_success title '
         sq=$(echo -e "\x27") &&
         echo "${sq}foo${sq}"
    '

Eh, I just realize the '\'' works inside here-doc, too.
Nevermind then, I'll resend it with just quoted sq in the here-doc then.

Thanks,
Stefan

^ permalink raw reply

* Re: [PATCH v10 19/20] branch: use ref-filter printing APIs
From: Jacob Keller @ 2017-01-11 23:47 UTC (permalink / raw)
  To: Karthik Nayak; +Cc: Git mailing list, Junio C Hamano
In-Reply-To: <20170110084953.15890-20-Karthik.188@gmail.com>

On Tue, Jan 10, 2017 at 12:49 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 34cd61cd9..f293ee5b0 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -37,11 +37,11 @@ static unsigned char head_sha1[20];
>  static int branch_use_color = -1;
>  static char branch_colors[][COLOR_MAXLEN] = {
>         GIT_COLOR_RESET,
> -       GIT_COLOR_NORMAL,       /* PLAIN */
> -       GIT_COLOR_RED,          /* REMOTE */
> -       GIT_COLOR_NORMAL,       /* LOCAL */
> -       GIT_COLOR_GREEN,        /* CURRENT */
> -       GIT_COLOR_BLUE,         /* UPSTREAM */
> +       GIT_COLOR_NORMAL,       /* PLAIN */
> +       GIT_COLOR_RED,          /* REMOTE */
> +       GIT_COLOR_NORMAL,       /* LOCAL */
> +       GIT_COLOR_GREEN,        /* CURRENT */
> +       GIT_COLOR_BLUE,         /* UPSTREAM */
>  };


What's... actually changing here? It looks like just white space? Is
there a strong reason for why this is changing?

Thanks,
Jake

^ permalink raw reply

* Re: [PATCH 4/4] unpack-trees: support super-prefix option
From: Junio C Hamano @ 2017-01-11 23:28 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Brandon Williams, David Turner, git@vger.kernel.org
In-Reply-To: <CAGZ79kaHDnVLQr8WLoaD5KKs7EqeW=KbkptF=iHpU5t054Xcdw@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

>> Preparing the expected output "expect" outside test_expect_success
>> block is also old-school.  Move it inside the new test?
>
> I looked into that. What is our current stance on using single/double quotes?

Using dq around executable part, i.e.

	test_expect_success title "
		echo \"'foo'\"
	"

is a million-times more grave sin even if the test initially does
not refer to any variable in its body.  Somebody will eventually
need to add a line that refers to a variable and either forgets to
quote \$ in front or properly quotes it.  The former makes the
variable interpolated while the arguments to the test_expect_success
is prepared (which is a bug) and the latter makes the result quite
ugly, like so:

	test_expect_success title "
		sq=\' var=foo &&
		echo \"\${sq}\$value\${sq}\"
	"

Enclosing the body always in sq-pair does mean something ugly like
this from the beginning once you need to use sq inside:

	test_expect_success title '
		sq='\'' &&
		echo "${sq}foo${sq}"
	'

or

	test_expect_success title '
		echo "'\''foo'\''"
	'

but the ugliness is localized to places where single quote is
absolutely needed, and '\'' is something eyes soon get used to as an
idiom [*1*].  It does not affect every reference of variables like
enclosing with dq does.


[Footnote]

*1* That "idiom"-ness is the reason why the last example above is
    not written like this:

	test_expect_success title '
		echo "'\'foo\''"
	'

    even though the sq pair surrounding "foo" is not technically
    necessary.  It lets us think of the string as almost always is
    inside sq context, with the single exception that we step
    outside sq context and append a sq by saying bs-sq (\').

^ permalink raw reply

* Re: [PATCH 4/4] unpack-trees: support super-prefix option
From: Stefan Beller @ 2017-01-11 22:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Brandon Williams, David Turner, git@vger.kernel.org
In-Reply-To: <xmqq37gpnuyi.fsf@gitster.mtv.corp.google.com>

On Wed, Jan 11, 2017 at 1:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Add support for the super-prefix option for commands that unpack trees.
>> For testing purposes enable it in read-tree, which has no other path
>> related output.
>
> "path related output"?  I am not sure I understand this.

Well, s/path related output/output of paths/.

> When read-tree reads N trees, or unpack_trees() is asked to "merge"
> N trees, how does --super-prefix supposed to interact with the paths
> in these trees?  Does the prefix added to paths in all trees?

Internally the super-prefix is ignored. Only the (input and) output
is using that super prefix for messages.

It was introduced for grepping recursively into submodules, i.e.

invoke as

    git grep --recurse-submodules \
      -e path/inside/submodule/and/further/down
    # internally it invokes:
    git -C .. --super-prefix .. grep ..
    # which operates "just normal" except for the
    # input parsing and output

The use case for this patch is working tree related things, i.e.
    git checkout --recurse-submodules
    # internally when recursing we call "git read-tree -u", but
    # reporting could be:
    Your local changes to the following files would be overwritten by checkout:
      path/inside/submodule/file.txt


>
> Did you mean that read-tree (and unpack_trees() in general) is a
> whole-tree operation and there is no path related input (i.e.
> pathspec limiting), but if another command that started in the
> superproject is running us in its submodule, it wants us to prefix
> the path to the submodule when we report errors?

Yes. I tried to explain it better above, but you got it here.

>
> If that is what is going on, I think it makes sense, but it may be
> asking too much from the readers to guess that from "Add support for
> the super-prefix option".

I need to enhance the commit message by a lot then.

>
>> --- a/t/t1001-read-tree-m-2way.sh
>> +++ b/t/t1001-read-tree-m-2way.sh
>> @@ -363,6 +363,15 @@ test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
>>       test -f a/b
>>  '
>>
>> +cat <<-EOF >expect &&
>> +     error: Updating 'fictional/a' would lose untracked files in it
>> +EOF
>> +
>> +test_expect_success 'read-tree supports the super-prefix' '
>> +     test_must_fail git --super-prefix fictional/ read-tree -u -m "$treeH" "$treeM" 2>actual &&
>> +     test_cmp expect actual
>> +'
>> +
>
> Preparing the expected output "expect" outside test_expect_success
> block is also old-school.  Move it inside the new test?

I looked into that. What is our current stance on using single/double quotes?
Most tests use single quotes for the test title as well as the test
instructions,
such that double quotes can be used naturally in there. But single quotes
cannot be used even escaped, such that we need to do a thing like

sq="'"

test_expect_success 'test title' '
    cat <<-EOF >expect &&
        error for ${sq}path${sq}
    EOF
    test instructions go here
'

though that is not used as often as I would think as
grep \${sq} yields t1507 and t3005.

>
> Hmph, as a reader of the code, I do not even want to wonder how
> expensive get_super_prefix() is.  If the executable part of the
> above were written like this, it would have been easier to
> understand:
>
>         if (super_prefix_len < 0) {
>                 if (!get_super_prefix())
>                         super_prefix_len = 0;
>                 else {
>                         int i;
>                         ... prepare buf[] and set super_prefix_len ...;
>                 }
>         }
>
>         if (!super_prefix_len)
>                 return path;
>
>         ... use buf[] to do the prefixing and return it ...
>

good point. I'll fix that.

^ permalink raw reply

* Re: [RFC PATCH 3/2] vreportf: add prefix to each line
From: Junio C Hamano @ 2017-01-11 22:11 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <20170111140758.yyfsc3r3spqpi6es@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Wed, Jan 11, 2017 at 09:01:38AM -0500, Jeff King wrote:
>
>>   [1/2]: Revert "vreportf: avoid intermediate buffer"
>>   [2/2]: vreport: sanitize ASCII control chars
>
> We've talked before about repeating the "error:" header for multi-line
> messages. The reversion in patch 1 makes this easy to play with, so I
> did. I kind of hate it. But here it is, for your viewing pleasure.

Thanks.

>
> -- >8 --
> Subject: vreportf: add prefix to each line
>
> Some error and warning messages are multi-line, but we put
> the prefix only on the first line. This means that either
> the subsequent lines don't have any indication that they're
> connected to the original error, or the caller has to make
> multiple calls to error() or warning(). And that's not even
> possible for die().
>
> Instead, let's put the prefix at the start of each line,
> just as advise() does.
>
> Note that this patch doesn't update the tests yet, so it
> causes tons of failures. This is just to see what it might
> look like.
>
> It's actually kind of ugly.  For instance, a failing test in
> t3600 now produces:
>
>    error: the following files have staged content different from both the
>    error: file and the HEAD:
>    error:     bar.txt
>    error:     foo.txt
>    error: (use -f to force removal)
>
> which seems a bit aggressive.  

I agree that it is ugly, but one reason I was hoping to do this
myself (or have somebody else do it by procrastinating) was that I
thought it may help i18n.  That is, for an original

	error(_("we found an error"))

a .po file may translate the string to a multi-line string that has
LFs in it and the output would look correct.  The translator already
can do so by indenting the second and subsequent lines by the same
column-width as "error: " (or its translation in their language, if
we are going to i18n these headers), but that (1) is extra work for
them, and (2) makes it impossible to have the same message appear in
different contexts (i.e. "error:" vs "warning:" that have different
column-widths).

> It also screws up messages which indent with tabs (the prefix eats
> up some of the tabstop, making the indentation smaller).

This is unavoidable and at the same time is a non-issue, isn't it?
Messages that indent the second and subsequent lines with tabs are
compensating the lack of the multi-line support of vreportf(), which
this RFC patch fixes.  They may need to be adjusted to the new world
order, but that is a good thing.  New multi-line messages no longer
have to worry about the prefix that is added only to the first line
when continuing the message to multiple lines.

> And the result is
> a little harder if you need to cut-and-paste the file lines
> (if your terminal lets you triple-click to select a whole
> line, now you have this "error:" cruft on the front).

This might be an issue, but I personally do not care (as I know how
to drive my "screen", namely, use 'c' while cutting) ;-).

> It may be possible to address some of that by using some
> other kind of continuation marker (instead of just repeating
> the prefix), and expanding initial tabs.

Yes indeed.  The "some other kind of continuation marker" could just
be a run of spaces that fill the same column as the "error: " or
other prefix given to the first line.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  usage.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/usage.c b/usage.c
> index ad6d2910f..8a1f6ff4e 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -8,18 +8,30 @@
>  
>  static FILE *error_handle;
>  
> +static void show_line(FILE *fh, const char *prefix, const char *line, int len)
> +{
> +	fprintf(fh, "%s%.*s\n", prefix, len, line);
> +}
> +
>  void vreportf(const char *prefix, const char *err, va_list params)
>  {
>  	char msg[4096];
>  	FILE *fh = error_handle ? error_handle : stderr;
> +	const char *base;
>  	char *p;
>  
>  	vsnprintf(msg, sizeof(msg), err, params);
> +
> +	base = msg;
>  	for (p = msg; *p; p++) {
> -		if (iscntrl(*p) && *p != '\t' && *p != '\n')
> +		if (*p == '\n') {
> +			show_line(fh, prefix, base, p - base);
> +			base = p + 1;
> +		} else if (iscntrl(*p) && *p != '\t')
>  			*p = '?';
>  	}
> -	fprintf(fh, "%s%s\n", prefix, msg);
> +
> +	show_line(fh, prefix, base, p - base);
>  }
>  
>  static NORETURN void usage_builtin(const char *err, va_list params)

^ permalink raw reply

* Re: [PATCHv2 1/2] builtin/commit.c: drop use snprintf via dynamic allocation
From: Junio C Hamano @ 2017-01-11 21:48 UTC (permalink / raw)
  To: Elia Pinto; +Cc: git
In-Reply-To: <20170111071032.27797-1-gitter.spiros@gmail.com>

Elia Pinto <gitter.spiros@gmail.com> writes:

> In general snprintf is bad because it may silently truncate results if we're
> wrong. In this patch where we use PATH_MAX, we'd want to handle larger
> paths anyway, so we switch to dynamic allocation.
>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> This is the second version of the patch.
>
> I have split the original commit in two, as discussed here
> http://public-inbox.org/git/20161213132717.42965-1-gitter.spiros@gmail.com/.

And both halves have the same title?

I think the the primary value of this one is that it removes the
PATH_MAX limitation from the environment setting that points to a
filename.  Reducing the number of snprintf() call is a side effect,
even though that may have been what motivated you originally.  The
original code used snprintf() correctly after all.

The primary value of 2/2 is reduction of the cognitive burden from
the programmers---switching to xstrfmt(), they no longer have to
count bytes needed for static allocation.  Reducing the number of
snprintf() call is a side effect, even though that may have been
what motivated you originally.  The original code used snprintf()
correctly after all.

The code looks correct in both patches (except that in 1/2 _clear()
immediately before exit() wouldn't have to be there, but it does not
hurt to have one either).  They need to be better explained.

Thanks.

>  builtin/commit.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0ed634b26..09bcc0f13 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -960,15 +960,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		return 0;
>  
>  	if (use_editor) {
> -		char index[PATH_MAX];
> -		const char *env[2] = { NULL };
> -		env[0] =  index;
> -		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> -		if (launch_editor(git_path_commit_editmsg(), NULL, env)) {
> +		struct argv_array env = ARGV_ARRAY_INIT;
> +
> +		argv_array_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
> +		if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
>  			fprintf(stderr,
>  			_("Please supply the message using either -m or -F option.\n"));
> +			argv_array_clear(&env);
>  			exit(1);
>  		}
> +		argv_array_clear(&env);
>  	}
>  
>  	if (!no_verify &&
> @@ -1557,23 +1558,22 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
>  
>  int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)
>  {
> -	const char *hook_env[3] =  { NULL };
> -	char index[PATH_MAX];
> +	struct argv_array hook_env = ARGV_ARRAY_INIT;
>  	va_list args;
>  	int ret;
>  
> -	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> -	hook_env[0] = index;
> +	argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
>  
>  	/*
>  	 * Let the hook know that no editor will be launched.
>  	 */
>  	if (!editor_is_used)
> -		hook_env[1] = "GIT_EDITOR=:";
> +		argv_array_push(&hook_env, "GIT_EDITOR=:");
>  
>  	va_start(args, name);
> -	ret = run_hook_ve(hook_env, name, args);
> +	ret = run_hook_ve(hook_env.argv,name, args);
>  	va_end(args);
> +	argv_array_clear(&hook_env);
>  
>  	return ret;
>  }

^ permalink raw reply

* Re: [PATCH] index: improve constness for reading blob data
From: Junio C Hamano @ 2017-01-11 21:36 UTC (permalink / raw)
  To: Brandon Williams; +Cc: git
In-Reply-To: <20170110200610.146596-1-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

> Improve constness of the index_state parameter to the
> 'read_blob_data_from_index' function.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---

Thanks.

^ permalink raw reply

* Re: [PATCH 4/4] unpack-trees: support super-prefix option
From: Junio C Hamano @ 2017-01-11 21:32 UTC (permalink / raw)
  To: Stefan Beller; +Cc: bmwill, novalis, git
In-Reply-To: <20170110014542.19352-5-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> Add support for the super-prefix option for commands that unpack trees.
> For testing purposes enable it in read-tree, which has no other path
> related output.

"path related output"?  I am not sure I understand this.

When read-tree reads N trees, or unpack_trees() is asked to "merge"
N trees, how does --super-prefix supposed to interact with the paths
in these trees?  Does the prefix added to paths in all trees?

Did you mean that read-tree (and unpack_trees() in general) is a
whole-tree operation and there is no path related input (i.e.
pathspec limiting), but if another command that started in the
superproject is running us in its submodule, it wants us to prefix
the path to the submodule when we report errors?

If that is what is going on, I think it makes sense, but it may be
asking too much from the readers to guess that from "Add support for
the super-prefix option".

> --- a/t/t1001-read-tree-m-2way.sh
> +++ b/t/t1001-read-tree-m-2way.sh
> @@ -363,6 +363,15 @@ test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
>  	test -f a/b
>  '
>  
> +cat <<-EOF >expect &&
> +	error: Updating 'fictional/a' would lose untracked files in it
> +EOF
> +
> +test_expect_success 'read-tree supports the super-prefix' '
> +	test_must_fail git --super-prefix fictional/ read-tree -u -m "$treeH" "$treeM" 2>actual &&
> +	test_cmp expect actual
> +'
> +

Preparing the expected output "expect" outside test_expect_success
block is also old-school.  Move it inside the new test?

> diff --git a/unpack-trees.c b/unpack-trees.c
> index 7a6df99d10..bc56195e27 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -52,6 +52,37 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
>  	  ? ((o)->msgs[(type)])      \
>  	  : (unpack_plumbing_errors[(type)]) )
>  
> +static const char *super_prefixed(const char *path)
> +{
> +	/*
> +	 * This is used for the error messages above.
> +	 * We need to have exactly two buffer spaces.
> +	 */
> +	static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
> +	static int super_prefix_len = -1;
> +	static unsigned idx = 0;
> +
> +	if (!get_super_prefix())
> +		return path;
> +
> +	if (super_prefix_len < 0) {
> +		int i;
> +
> +		for (i = 0; i < ARRAY_SIZE(buf); i++)
> +			strbuf_addstr(&buf[i], get_super_prefix());
> +
> +		super_prefix_len = strlen(get_super_prefix());
> +	}
> +
> +	if (++idx >= ARRAY_SIZE(buf))
> +		idx = 0;
> +
> +	strbuf_setlen(&buf[idx], super_prefix_len);
> +	strbuf_addstr(&buf[idx], path);
> +
> +	return buf[idx].buf;
> +}

Hmph, as a reader of the code, I do not even want to wonder how
expensive get_super_prefix() is.  If the executable part of the
above were written like this, it would have been easier to
understand:

	if (super_prefix_len < 0) {
		if (!get_super_prefix())
			super_prefix_len = 0;
		else {
			int i;
			... prepare buf[] and set super_prefix_len ...;
                }
	}

        if (!super_prefix_len)
		return path;

	... use buf[] to do the prefixing and return it ...


^ permalink raw reply

* Re: [PATCH 2/2] diff: document the pattern format for diff.orderFile
From: Junio C Hamano @ 2017-01-11 21:06 UTC (permalink / raw)
  To: Richard Hansen; +Cc: git
In-Reply-To: <2fb4296d-f084-4a76-44f3-7dc7d7cca7b1@google.com>

Richard Hansen <hansenr@google.com> writes:

>> Back then we didn't even have wildmatch(), and used fnmatch()
>> instead, so forcing FNM_PATHNAME would have meant that people
>> wouldn't be able to say "foo*bar" to match "foo/other/bar"; with
>> wildmatch, "foo**bar" lets you defeat WM_PATHNAME so having
>> WM_PATHNAME always in effect is less of an issue, but with
>> fnmatch(), having FNM_PATHNAME always in effect has a lot of
>> downside.
>
> Ah, that makes sense.
>>
>> I'd expect that orderfile people have today will be broken and
>> require tweaking if you switched WM_PATHNAME on.
>
> OK, so we don't want to turn on WM_PATHNAME unless we do it for a new
> major version.

I do agree with you that if we were starting Git from scratch, or at
least if we were adding diff.orderfile feature today, we would have
used wildmatch(WM_PATHNAME) for this matching.  We would also have
used the same parser as used to read the exclude files (and when we
see negative matching entries in the parsed result, either errored
out or ignored them with warning).  That kind of change unfortunately
would require a major version bump, I am afraid.

^ permalink raw reply

* [PATCH] submodule absorbgitdirs: mention in docstring help
From: Stefan Beller @ 2017-01-11 20:59 UTC (permalink / raw)
  To: gitster; +Cc: git, jrnieder, Stefan Beller

This part was missing in f6f85861 (submodule: add absorb-git-dir function,
2016-12-12).

Noticed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 554bd1c494..b43af1742c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -12,7 +12,8 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <re
    or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
    or: $dashless [--quiet] foreach [--recursive] <command>
-   or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
+   or: $dashless [--quiet] sync [--recursive] [--] [<path>...]
+   or: $dashless [--quiet] absorbgitdirs [--] [<path>...]"
 OPTIONS_SPEC=
 SUBDIRECTORY_OK=Yes
 . git-sh-setup
-- 
2.11.0.259.g7b30ecf4f0


^ permalink raw reply related

* Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile
From: Junio C Hamano @ 2017-01-11 20:53 UTC (permalink / raw)
  To: Jeff King; +Cc: Richard Hansen, git
In-Reply-To: <20170111144158.ef6kle3vw3ejgmut@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Tue, Jan 10, 2017 at 02:15:11PM -0800, Junio C Hamano wrote:
>
>> Richard Hansen <hansenr@google.com> writes:
>> ...
>> > I think so.  (For bare repositories anyway; non-bare should be
>> > relative to GIT_WORK_TREE.)  Perhaps git_config_pathname() itself
>> > should convert relative paths to absolute so that every pathname
>> > setting automatically works without changing any calling code.
>> 
>> Yes, that was what I was alluding to.  We might have to wait until
>> major version boundary to do so, but I think that it is the sensible
>> way forward in the longer term to convert relative to absolute in
>> git_config_pathname().
>
> Yeah, I'd agree.
>
> I'm undecided on whether it would need to happen at a major version
> bump. ...
>
> So I dunno. I do hate to break even corner cases, but I'm having trouble
> imagining the scenario where somebody is actually using the current
> behavior in a useful way.

Thanks for a detailed analysis (I probably should have spelt them
out when I said "we might have to" to save you the trouble).


^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox