Re: [RFC PATCH 0/5] Localise error headers

[Jeff King]

On Mon, Jan 02, 2017 at 12:14:49PM +0100, Michael J Gruber wrote:

> Currently, the headers "error: ", "warning: " etc. - generated by die(),
> warning() etc. - are not localized, but we feed many localized error messages
> into these functions so that we produce error messages with mixed 
> localisation.
> 
> This series introduces variants of die() etc. that use localised variants of
> the headers, i.e. _("error: ") etc., and are to be fed localized messages. So,
> instead of die(_("not workee")), which would produce a mixed localisation 
> (such
> as "error: geht ned"), one should use die_(_("not workee")) (resulting in
> "Fehler: geht ned").

I can't say I'm excited about having matching "_" variants for each
function. Are we sure that they are necessary? I.e., would it be
acceptable to just translate them always?

> 1/5 prepares the error machinery
> 2/5 provides new variants error_() etc.
> 3/5 has coccinelli rules error(_(E)) -> error_(_(E)) etc.
> 4/5 applies the coccinelli patches
> 
> 5/5 is not to be applied to the main tree, but helps you try out the feature:
> it has changes to de.po and git.pot so that e.g. "git branch" has fully 
> localised
> error messages (see the recipe in the commit message).

Your patches 4 and 5 don't seem to have made it to the list. Judging
from the diffstat, I'd guess they broke the 100K limit.

-Peff

Re: [PATCH v4 0/5] road to reentrant real_path

[Torsten Bögershausen]

On 04.01.17 01:48, Jeff King wrote:
> On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:
> 
>> Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
>> MAXDEPTH due to a naming conflict brought up by Lars Schneider.
> 
> Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's
> what all other similar functions will be using.
> 
> The only problem was that we were redefining the macro. So maybe:
> 
>   #ifndef MAXSYMLINKS
>   #define MAXSYMLINKS 5
>   #endif
> 
> would be a good solution?
Why 5  ? (looking at the  20..30 below)
And why 5 on one system and e.g. on my Mac OS
#define MAXSYMLINKS 32  

Would the same value value for all Git installations on all platforms make 
sense?
#define GITMAXSYMLINKS 20

> 
> It looks like the "usual" value is more like 20 or 30 on most systems,
> though.  We should probably also set errno to ELOOP when we hit the
> limit, which is what other symlink-resolving functions would do.
> 
> I'm surprised we didn't hit this on Linux, which has MAXSYMLINKS, too.
> We should be picking it up from .
> 
> -Peff
> 

Re: [PATCH v3 06/16] pathspec: copy and free owned memory

[Duy Nguyen]

On Wed, Dec 14, 2016 at 6:14 AM, Brandon Williams  wrote:
>  void clear_pathspec(struct pathspec *pathspec)
>  {
> +   int i;
> +
> +   for (i = 0; i < pathspec->nr; i++) {
> +   free(pathspec->items[i].match);
> +   free(pathspec->items[i].original);
> +   }
> free(pathspec->items);
> pathspec->items = NULL;

We should set pathspec->nr to zero so that calling this function again
won't cause any harm.

>  }
-- 
Duy

Re: [PATCH v3 08/16] pathspec: always show mnemonic and name in unsupported_magic

[Duy Nguyen]

On Wed, Dec 14, 2016 at 6:14 AM, Brandon Williams  wrote:
> @@ -340,8 +336,9 @@ static void NORETURN unsupported_magic(const char 
> *pattern,
> continue;
> if (sb.len)
> strbuf_addch(, ' ');
> -   if (short_magic & m->bit)
> -   strbuf_addf(, "'%c'", m->mnemonic);
> +
> +   if (m->mnemonic)
> +   strbuf_addf(, "'(%c)%s'", m->mnemonic, m->name);
> else
> strbuf_addf(, "'%s'", m->name);
> }

The die() call is out of diff context, but it'll print

pathspec magic not supported by this command: (!)top

which looks too much like :() pathspec syntax too me
and threw me off a bit. And it's a bit cryptic, isn't it? Since this
is meant for human, maybe we can just write

pathspec magic not supported by this command: top (mnemonic: '!')
-- 
Duy

Re: [PATCH v3 02/16] dir: remove struct path_simplify

[Duy Nguyen]

On Wed, Dec 14, 2016 at 6:14 AM, Brandon Williams  wrote:
> @@ -2010,14 +1987,11 @@ static struct untracked_cache_dir 
> *validate_untracked_cache(struct dir_struct *d
> return root;
>  }
>
> -int read_directory(struct dir_struct *dir, const char *path, int len, const 
> struct pathspec *pathspec)
> +int read_directory(struct dir_struct *dir, const char *path,
> +  int len, const struct pathspec *pathspec)
>  {
> -   struct path_simplify *simplify;
> struct untracked_cache_dir *untracked;
>
> -   /*
> -* Check out create_simplify()
> -*/
> if (pathspec)
> GUARD_PATHSPEC(pathspec,
>PATHSPEC_FROMTOP |


This GUARD_PATHSPEC macro should be moved into simplify_away() and
exclude_pathspec_matches(), so that next time somebody adds a new
pathspec magic, they can basically grep GUARD_PATHSPEC and determine
if these code can support their favorite magic or not.
-- 
Duy

Re: What's cooking in git.git (Dec 2016, #08; Tue, 27)

[Duy Nguyen]

On Thu, Dec 29, 2016 at 5:06 PM, Duy Nguyen  wrote:
> On Thu, Dec 29, 2016 at 1:18 AM, Brandon Williams  wrote:
>> On 12/27, Junio C Hamano wrote:
>>> * bw/pathspec-cleanup (2016-12-14) 16 commits
>>>  - pathspec: rename prefix_pathspec to init_pathspec_item
>>>  - pathspec: small readability changes
>>>  - pathspec: create strip submodule slash helpers
>>>  - pathspec: create parse_element_magic helper
>>>  - pathspec: create parse_long_magic function
>>>  - pathspec: create parse_short_magic function
>>>  - pathspec: factor global magic into its own function
>>>  - pathspec: simpler logic to prefix original pathspec elements
>>>  - pathspec: always show mnemonic and name in unsupported_magic
>>>  - pathspec: remove unused variable from unsupported_magic
>>>  - pathspec: copy and free owned memory
>>>  - pathspec: remove the deprecated get_pathspec function
>>>  - ls-tree: convert show_recursive to use the pathspec struct interface
>>>  - dir: convert fill_directory to use the pathspec struct interface
>>>  - dir: remove struct path_simplify
>>>  - mv: remove use of deprecated 'get_pathspec()'
>>>
>>>  Code clean-up in the pathspec API.
>>>
>>>  Waiting for the (hopefully) final round of review before 'next'.
>>
>> What more needs to be reviewed for this series?
>
> I wanted to have a look at it but I successfully managed to put if off
> so far. Will do soonish.

I have just sent a couple of minor comments. The rest looks good!
-- 
Duy

[PATCH 2/2] stash: --[no-]include-untracked option for create

[Marc Strapetz]

Expose internal option to include untracked files
for the stash 'create' subcommand.

Signed-off-by: Marc Strapetz 
---
 Documentation/git-stash.txt |  2 +-
 git-stash.sh| 14 --
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9cef06e..cc7944e59 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 [-u|--include-untracked] [-a|--all] []]
 'git stash' clear
-'git stash' create []
+'git stash' create [-u|--[no-]include-untracked] []
 'git stash' store [-m|--message ] [-q|--quiet] 
 
 DESCRIPTION
diff --git a/git-stash.sh b/git-stash.sh
index c6b9db694..16f5fe93e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -10,7 +10,7 @@ USAGE="list []
or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
   [-u|--include-untracked] [-a|--all] []]
or: $dashless clear
-   or: $dashless create []
+   or: $dashless create [-u|--[no-]include-untracked] []
or: $dashless store [-m|--message ] [-q|--quiet] "
 
 SUBDIRECTORY_OK=Yes
@@ -629,7 +629,17 @@ clear)
;;
 create)
shift
-   create_stash "$*" && echo "$w_commit"
+   case "$1" in
+   -u|--include-untracked)
+   untracked=untracked
+   shift
+   ;;
+   --no-include-untracked)
+   untracked=
+   shift
+   ;;
+   esac
+   create_stash "$*" "$untracked" && echo "$w_commit"
;;
 store)
shift
-- 
2.11.0

[PATCH 1/2] stash: fix USAGE text

[Marc Strapetz]

Add missing usage description for stash subcommands
'create' and 'store'.

Signed-off-by: Marc Strapetz 
---
 git-stash.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1a..c6b9db694 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -9,7 +9,9 @@ USAGE="list []
or: $dashless branch  []
or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
   [-u|--include-untracked] [-a|--all] []]
-   or: $dashless clear"
+   or: $dashless clear
+   or: $dashless create []
+   or: $dashless store [-m|--message ] [-q|--quiet] "
 
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
-- 
2.11.0

Re: [PATCH v3 06/21] t1700: add tests for core.splitIndex

[Junio C Hamano]

Christian Couder  writes:

> Ok, I will add a patch to update the style of the existing tests at
> the beginning of the series and then use the same new style in the
> tests I add in later patches.

That's not what I meant---I was expecting and was willing to accept
a corrected patch that leaves existing old-fashioned ones as they
are while making sure that added ones are modern, to reduce the cost
of finishing this series, leaving the style fixes of existing ones
for future clean-up task that can be done by anybody after the dust
from this series settles.

A preparatory clean-up patch before the series like you plan is fine
(eh, rather, "extra nice"), so... thanks.

Re: [RFC PATCH 0/5] Localise error headers

[Duy Nguyen]

On Mon, Jan 2, 2017 at 6:14 PM, Michael J Gruber
 wrote:
> Currently, the headers "error: ", "warning: " etc. - generated by die(),
> warning() etc. - are not localized, but we feed many localized error messages
> into these functions so that we produce error messages with mixed 
> localisation.
>
> This series introduces variants of die() etc. that use localised variants of
> the headers, i.e. _("error: ") etc., and are to be fed localized messages. So,
> instead of die(_("not workee")), which would produce a mixed localisation 
> (such
> as "error: geht ned"), one should use die_(_("not workee")) (resulting in
> "Fehler: geht ned").

Another option, not as thorough, but less effort, is changing
die/err/warn default routines to the "porcelain" versions where we do
_("fatal:") internally _with_ die(), not die_(). We can set this for
porcelain commands that we know can be fully i18n-ized. Then maybe
die_() will fill in the gap if there's still need for it.
-- 
Duy

Re: [PATCH v3 08/16] pathspec: always show mnemonic and name in unsupported_magic

[Brandon Williams]

On 01/03, Duy Nguyen wrote:
> On Wed, Dec 14, 2016 at 6:14 AM, Brandon Williams  wrote:
> > @@ -340,8 +336,9 @@ static void NORETURN unsupported_magic(const char 
> > *pattern,
> > continue;
> > if (sb.len)
> > strbuf_addch(, ' ');
> > -   if (short_magic & m->bit)
> > -   strbuf_addf(, "'%c'", m->mnemonic);
> > +
> > +   if (m->mnemonic)
> > +   strbuf_addf(, "'(%c)%s'", m->mnemonic, m->name);
> > else
> > strbuf_addf(, "'%s'", m->name);
> > }
> 
> The die() call is out of diff context, but it'll print
> 
> pathspec magic not supported by this command: (!)top
> 
> which looks too much like :() pathspec syntax too me
> and threw me off a bit. And it's a bit cryptic, isn't it? Since this
> is meant for human, maybe we can just write
> 
> pathspec magic not supported by this command: top (mnemonic: '!')

I was trying to keep it short and sweet, turns out that ends up being
more difficult to understand.  I like your suggestion, it definitely
makes things much clearer.

-- 
Brandon Williams

Re: [PATCH] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

On Sat, Dec 31, 2016 at 5:11 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Every once in a while someone complains to the mailing list to have
>> run into this weird assertion[1].
>>
>> The usual response from the mailing list is link to old discussions[2],
>> and acknowledging the problem stating it is known.
>>
>> For now just improve the user visible error message.
>
> Thans. judging from the date: header I take this is meant as v3 that
> supersedes v2 done on Wednesday.

Yes, that is correct. Sorry for being sloppy not numbering the
patches correctly.

>
> It is not clear in the above that what this thing is.  Given that we
> are de-asserting it, is the early part of the new code diagnosing an
> end-user error (i.e. you gave me a pathspec but that extends into a
> submodule which is a no-no)?  The "was a submodule issue" comment
> added is "this is an end-user mistake, there is nothing to fix in
> the code"?

This is not a fix in the code, but purely improving an error message.
So far anytime someone run into this assert, it was related to submodules.
I do not know the pathspec code well enough to claim this condition
can be produced via submodules *only*, though.

So I proposed a more defensive patch, which diagnoses if it is the
"no-no, pathspec extends into a submodule" first and then throws
a generic error afterwards in case it is not the submodule issue.

> I take that the new "BUG" thing tells the Git developers that no
> sane codepath should throw an pathspec_item that satisfies the
> condition of the if() statement for non-submodules?

If we want to keep the semantics of the assert around, then we
have to have a blank statement if the submodule error message
is not triggered.

I assume if we print this BUG, then there is an actual bug.

>
>> diff --git a/pathspec.c b/pathspec.c
>> index 22ca74a126..b446d79615 100644
>> --- a/pathspec.c
>> +++ b/pathspec.c
>> @@ -313,8 +313,23 @@ static unsigned prefix_pathspec(struct pathspec_item 
>> *item,
>>   }
>>
>>   /* sanity checks, pathspec matchers assume these are sane */
>> - assert(item->nowildcard_len <= item->len &&
>> -item->prefix <= item->len);
>> + if (item->nowildcard_len > item->len ||
>> + item->prefix > item->len) {
>> + /* Historically this always was a submodule issue */
>> + for (i = 0; i < active_nr; i++) {
>> + struct cache_entry *ce = active_cache[i];
>> + int ce_len = ce_namelen(ce);
>> + int len = ce_len < item->len ? ce_len : item->len;
>> + if (!S_ISGITLINK(ce->ce_mode))
>> + continue;
>
> Computation of ce_len and len are better done after this check, no?

Yes, though I trusted the modern-day-compilers to get it right. Will
fix in a reroll.

>> +test_expect_success 'setup a submodule' '
>> + test_commit 1 &&
>> + git submodule add ./ sub &&
>
> Is this adding our own project as its submodule?

Yes it is.

>
> It MIGHT be a handy hack when writing a test, but let's stop doing
> that insanity.

I agree that this is not a good idea.

>  No sane project does that in real life, doesn't it?

If such a project was cloned with submodules, it would recurse endlessly. :)

> Create a subdirectory, make it a repository, have a commit there and
> bind that as our own submodule.  That would be a more normal way to
> start your own superproject and its submodule pair if they originate
> together at the same place.

I wonder if we want to have a helper function in test-lib.sh to be used
for that. This use case (have a repository and a submodule) happens in
a lot of tests, so we could make life easier by providing a function
in the library so it is even easier than this HACK.

> Better yet create a separate repository, have a commit there, and
> then pull it in with "git submodule add && git submodule init" into
> our repository.  That would be the normal way to borrow somebody
> else's project as a part of your own superproject.

The library function could do that, yes.

Thanks,
Stefan

Re: [PATCH] archive-zip: load userdiff config

[René Scharfe]

Am 02.01.2017 um 23:25 schrieb Jeff King:

Since 4aff646d17 (archive-zip: mark text files in archives,
2015-03-05), the zip archiver will look at the userdiff
driver to decide whether a file is text or binary. This
usually doesn't need to look any further than the attributes
themselves (e.g., "-diff", etc). But if the user defines a
custom driver like "diff=foo", we need to look at
"diff.foo.binary" in the config. Prior to this patch, we
didn't actually load it.


Ah, didn't think of that, obviously.

Would it make sense for userdiff_find_by_path() to die if 
userdiff_config() hasn't been called, yet?



I also happened to notice that zipfiles are created using the local
timezone (because they have no notion of the timezone, so we have to
pick _something_). That's probably the least-terrible option, but it was
certainly surprising to me when I tried to bit-for-bit reproduce a
zipfile from GitHub on my local machine.


That reminds me of an old request to allow users better control over the 
meta-data written into archives.  Being able to specify a time zone 
offset could be a start.



 archive-zip.c  |  7 +++
 t/t5003-archive-zip.sh | 22 ++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/archive-zip.c b/archive-zip.c
index 9db47357b0..b429a8d974 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -554,11 +554,18 @@ static void dos_time(time_t *time, int *dos_date, int 
*dos_time)
*dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;
 }

+static int archive_zip_config(const char *var, const char *value, void *data)
+{
+   return userdiff_config(var, value);
+}
+
 static int write_zip_archive(const struct archiver *ar,
 struct archiver_args *args)
 {
int err;

+   git_config(archive_zip_config, NULL);
+


I briefly thought about moving this call to archive.c with the rest of 
the config-related stuff, but I agree it's better kept here.



dos_time(>time, _date, _time);

zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 14744b2a4b..55c7870997 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -64,6 +64,12 @@ check_zip() {
test_cmp_bin $original/nodiff.crlf $extracted/nodiff.crlf &&
test_cmp_bin $original/nodiff.lf   $extracted/nodiff.lf
"
+
+   test_expect_success UNZIP " validate that custom diff is unchanged " "
+   test_cmp_bin $original/custom.cr   $extracted/custom.cr &&
+   test_cmp_bin $original/custom.crlf $extracted/custom.crlf &&
+   test_cmp_bin $original/custom.lf   $extracted/custom.lf
+   "
 }

 test_expect_success \
@@ -78,6 +84,9 @@ test_expect_success \
  printf "text\r" >a/nodiff.cr &&
  printf "text\r\n"   >a/nodiff.crlf &&
  printf "text\n" >a/nodiff.lf &&
+ printf "text\r" >a/custom.cr &&
+ printf "text\r\n"   >a/custom.crlf &&
+ printf "text\n" >a/custom.lf &&
  printf "\0\r"   >a/binary.cr &&
  printf "\0\r\n" >a/binary.crlf &&
  printf "\0\n"   >a/binary.lf &&
@@ -112,15 +121,20 @@ test_expect_success 'add files to repository' '
 test_expect_success 'setup export-subst and diff attributes' '
echo "a/nodiff.* -diff" >>.git/info/attributes &&
echo "a/diff.* diff" >>.git/info/attributes &&
+   echo "a/custom.* diff=custom" >>.git/info/attributes &&
+   git config diff.custom.binary true &&
echo "substfile?" export-subst >>.git/info/attributes &&
git log --max-count=1 "--pretty=format:A${SUBSTFORMAT}O" HEAD \
>a/substfile1
 '

-test_expect_success \
-'create bare clone' \
-'git clone --bare . bare.git &&
- cp .git/info/attributes bare.git/info/attributes'
+test_expect_success 'create bare clone' '
+   git clone --bare . bare.git &&
+   cp .git/info/attributes bare.git/info/attributes &&
+   # Recreate our changes to .git/config rather than just copying it, as
+   # we do not want to clobber core.bare or other settings.
+   git -C bare.git config diff.custom.binary true
+'

 test_expect_success \
 'remove ignored file' \



Looks good, thanks!

René

Re: [PATCH v2 2/2] t9813: avoid using pipes

[Stefan Beller]

On Mon, Jan 2, 2017 at 10:45 AM, Pranit Bauva  wrote:
> The exit code of the upstream in a pipe is ignored thus we should avoid
> using it.

for commands under test, i.e. git things. Other parts can be piped if that makes
the test easier. Though I guess that can be guessed by the reader as well,
as you only convert git commands on upstream pipes.

> By writing out the output of the git command to a file, we can
> test the exit codes of both the commands.
>
> Signed-off-by: Pranit Bauva 

Thanks for taking ownership of this issue as well. :)

> ---
>  t/t9813-git-p4-preserve-users.sh | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t9813-git-p4-preserve-users.sh 
> b/t/t9813-git-p4-preserve-users.sh
> index 798bf2b67..9d7550ff3 100755
> --- a/t/t9813-git-p4-preserve-users.sh
> +++ b/t/t9813-git-p4-preserve-users.sh
> @@ -118,12 +118,12 @@ test_expect_success 'not preserving user with mixed 
> authorship' '
> make_change_by_user usernamefile3 Derek de...@example.com &&
> P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
> export P4EDITOR P4USER P4PASSWD &&
> -   git p4 commit |\
> -   grep "git author de...@example.com does not match" &&
> +   git p4 commit >actual 2>&1 &&

Why do we need to pipe 2>&1 here?
Originally the piping only fed the stdout to grep, so this patch changes the
test? Maybe

2>actual.err &&
test_must_be_empty actual.err

instead?

Thanks,
Stefan

Re: What's cooking in git.git (Dec 2016, #08; Tue, 27)

[Brandon Williams]

On 01/03, Duy Nguyen wrote:
> On Thu, Dec 29, 2016 at 5:06 PM, Duy Nguyen  wrote:
> > On Thu, Dec 29, 2016 at 1:18 AM, Brandon Williams  wrote:
> >> On 12/27, Junio C Hamano wrote:
> >>> * bw/pathspec-cleanup (2016-12-14) 16 commits
> >>>  - pathspec: rename prefix_pathspec to init_pathspec_item
> >>>  - pathspec: small readability changes
> >>>  - pathspec: create strip submodule slash helpers
> >>>  - pathspec: create parse_element_magic helper
> >>>  - pathspec: create parse_long_magic function
> >>>  - pathspec: create parse_short_magic function
> >>>  - pathspec: factor global magic into its own function
> >>>  - pathspec: simpler logic to prefix original pathspec elements
> >>>  - pathspec: always show mnemonic and name in unsupported_magic
> >>>  - pathspec: remove unused variable from unsupported_magic
> >>>  - pathspec: copy and free owned memory
> >>>  - pathspec: remove the deprecated get_pathspec function
> >>>  - ls-tree: convert show_recursive to use the pathspec struct interface
> >>>  - dir: convert fill_directory to use the pathspec struct interface
> >>>  - dir: remove struct path_simplify
> >>>  - mv: remove use of deprecated 'get_pathspec()'
> >>>
> >>>  Code clean-up in the pathspec API.
> >>>
> >>>  Waiting for the (hopefully) final round of review before 'next'.
> >>
> >> What more needs to be reviewed for this series?
> >
> > I wanted to have a look at it but I successfully managed to put if off
> > so far. Will do soonish.
> 
> I have just sent a couple of minor comments. The rest looks good!

Thanks!  I'll go take a look.

-- 
Brandon Williams

Re: [PATCH] don't use test_must_fail with grep

[Stefan Beller]

On Sat, Dec 31, 2016 at 3:44 AM, Pranit Bauva  wrote:
> test_must_fail should only be used for testing git commands. To test the
> failure of other commands use `!`.
>
> Reported-by: Stefan Beller 
> Signed-off-by: Pranit Bauva 

Thanks for writing up such a patch!
I had put it on my todo list, but you
were faster on actually going through.

Thanks,
Stefan

Re: builtin difftool parsing issue

[Stefan Beller]

On Mon, Jan 2, 2017 at 11:05 AM, Paul Sbarra  wrote:
>> I would have expected `git difftool --submodule=diff ...` to work... What
>> are the problems?
>
> The docs for difftool state...
> "git difftool is a frontend to git diff and accepts the same options
> and arguments."

I think such a sentence in the man page is dangerous, as nobody
was caught this issue until now. There have been numerous authors
and reviewers that touched "diff --submodule=, but as there
is no back-reference, hinting that the patch is only half done, and the
difftool also needs to implement such an option.

We should reword the man page either as

  "git difftool is a frontend to git diff and accepts the most(?) options
  and arguments."

or even be explicit and list the arguments instead. There we could also
describe differences if any (e.g. the formats available might be different
for --submodule=)

[PATCH v4 10/16] pathspec: factor global magic into its own function

[Brandon Williams]

Create helper functions to read the global magic environment variables
in additon to factoring out the global magic gathering logic into its
own function.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 127 +
 1 file changed, 78 insertions(+), 49 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 032436bc1..f760f44f9 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -87,6 +87,75 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, 
unsigned magic)
strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
+static inline int get_literal_global(void)
+{
+   static int literal = -1;
+
+   if (literal < 0)
+   literal = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT, 0);
+
+   return literal;
+}
+
+static inline int get_glob_global(void)
+{
+   static int glob = -1;
+
+   if (glob < 0)
+   glob = git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT, 0);
+
+   return glob;
+}
+
+static inline int get_noglob_global(void)
+{
+   static int noglob = -1;
+
+   if (noglob < 0)
+   noglob = git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT, 0);
+
+   return noglob;
+}
+
+static inline int get_icase_global(void)
+{
+   static int icase = -1;
+
+   if (icase < 0)
+   icase = git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT, 0);
+
+   return icase;
+}
+
+static int get_global_magic(int element_magic)
+{
+   int global_magic = 0;
+
+   if (get_literal_global())
+   global_magic |= PATHSPEC_LITERAL;
+
+   /* --glob-pathspec is overridden by :(literal) */
+   if (get_glob_global() && !(element_magic & PATHSPEC_LITERAL))
+   global_magic |= PATHSPEC_GLOB;
+
+   if (get_glob_global() && get_noglob_global())
+   die(_("global 'glob' and 'noglob' pathspec settings are 
incompatible"));
+
+   if (get_icase_global())
+   global_magic |= PATHSPEC_ICASE;
+
+   if ((global_magic & PATHSPEC_LITERAL) &&
+   (global_magic & ~PATHSPEC_LITERAL))
+   die(_("global 'literal' pathspec setting is incompatible "
+ "with all other global pathspec settings"));
+
+   /* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
+   if (get_noglob_global() && !(element_magic & PATHSPEC_GLOB))
+   global_magic |= PATHSPEC_LITERAL;
+
+   return global_magic;
+}
+
 /*
  * Take an element of a pathspec and check for magic signatures.
  * Append the result to the prefix. Return the magic bitmap.
@@ -104,46 +173,12 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item, unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
 {
-   static int literal_global = -1;
-   static int glob_global = -1;
-   static int noglob_global = -1;
-   static int icase_global = -1;
-   unsigned magic = 0, element_magic = 0, global_magic = 0;
+   unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
char *match;
int i, pathspec_prefix = -1;
 
-   if (literal_global < 0)
-   literal_global = 
git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT, 0);
-   if (literal_global)
-   global_magic |= PATHSPEC_LITERAL;
-
-   if (glob_global < 0)
-   glob_global = git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT, 0);
-   if (glob_global)
-   global_magic |= PATHSPEC_GLOB;
-
-   if (noglob_global < 0)
-   noglob_global = git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT, 
0);
-
-   if (glob_global && noglob_global)
-   die(_("global 'glob' and 'noglob' pathspec settings are 
incompatible"));
-
-
-   if (icase_global < 0)
-   icase_global = git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT, 0);
-   if (icase_global)
-   global_magic |= PATHSPEC_ICASE;
-
-   if ((global_magic & PATHSPEC_LITERAL) &&
-   (global_magic & ~PATHSPEC_LITERAL))
-   die(_("global 'literal' pathspec setting is incompatible "
- "with all other global pathspec settings"));
-
-   if (flags & PATHSPEC_LITERAL_PATH)
-   global_magic = 0;
-
-   if (elt[0] != ':' || literal_global ||
+   if (elt[0] != ':' || get_literal_global() ||
(flags & PATHSPEC_LITERAL_PATH)) {
; /* nothing to do */
} else if (elt[1] == '(') {
@@ -207,15 +242,11 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item, unsigned flags,
 
magic |= element_magic;
 
-   /* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
-   if (noglob_global && !(magic & PATHSPEC_GLOB))
-   global_magic |= PATHSPEC_LITERAL;
-
-   /* --glob-pathspec is overridden by :(literal) */
-   if ((global_magic & PATHSPEC_GLOB) && (magic & 

[PATCH v4 03/16] dir: convert fill_directory to use the pathspec struct interface

[Brandon Williams]

Convert 'fill_directory()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.

Signed-off-by: Brandon Williams 
---
 dir.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/dir.c b/dir.c
index 1df61f10f..e8ddd7f8a 100644
--- a/dir.c
+++ b/dir.c
@@ -174,17 +174,21 @@ char *common_prefix(const struct pathspec *pathspec)
 
 int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)
 {
-   size_t len;
+   char *prefix;
+   size_t prefix_len;
 
/*
 * Calculate common prefix for the pathspec, and
 * use that to optimize the directory walk
 */
-   len = common_prefix_len(pathspec);
+   prefix = common_prefix(pathspec);
+   prefix_len = prefix ? strlen(prefix) : 0;
 
/* Read the directory and prune it */
-   read_directory(dir, pathspec->nr ? pathspec->_raw[0] : "", len, 
pathspec);
-   return len;
+   read_directory(dir, prefix, prefix_len, pathspec);
+
+   free(prefix);
+   return prefix_len;
 }
 
 int within_depth(const char *name, int namelen,
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v4 06/16] pathspec: copy and free owned memory

[Brandon Williams]

The 'original' string entry in a pathspec_item is only duplicated some
of the time, instead always make a copy of the original and take
ownership of the memory.

Since both 'match' and 'original' string entries in a pathspec_item are
owned by the pathspec struct, they need to be freed when clearing the
pathspec struct (in 'clear_pathspec()') and duplicated when copying the
pathspec struct (in 'copy_pathspec()').

Also change the type of 'match' and 'original' to 'char *' in order to
more explicitly show the ownership of the memory.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 23 +++
 pathspec.h |  4 ++--
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 1f918cbae..b8faa8f46 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -259,8 +259,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
strbuf_addstr(, match);
item->original = strbuf_detach(, NULL);
-   } else
-   item->original = elt;
+   } else {
+   item->original = xstrdup(elt);
+   }
item->len = strlen(item->match);
item->prefix = prefixlen;
 
@@ -388,8 +389,8 @@ void parse_pathspec(struct pathspec *pathspec,
die("BUG: PATHSPEC_PREFER_CWD requires arguments");
 
pathspec->items = item = xcalloc(1, sizeof(*item));
-   item->match = prefix;
-   item->original = prefix;
+   item->match = xstrdup(prefix);
+   item->original = xstrdup(prefix);
item->nowildcard_len = item->len = strlen(prefix);
item->prefix = item->len;
pathspec->nr = 1;
@@ -453,13 +454,27 @@ void parse_pathspec(struct pathspec *pathspec,
 
 void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
 {
+   int i;
+
*dst = *src;
ALLOC_ARRAY(dst->items, dst->nr);
COPY_ARRAY(dst->items, src->items, dst->nr);
+
+   for (i = 0; i < dst->nr; i++) {
+   dst->items[i].match = xstrdup(src->items[i].match);
+   dst->items[i].original = xstrdup(src->items[i].original);
+   }
 }
 
 void clear_pathspec(struct pathspec *pathspec)
 {
+   int i;
+
+   for (i = 0; i < pathspec->nr; i++) {
+   free(pathspec->items[i].match);
+   free(pathspec->items[i].original);
+   }
free(pathspec->items);
pathspec->items = NULL;
+   pathspec->nr = 0;
 }
diff --git a/pathspec.h b/pathspec.h
index 70a592e91..49fd823dd 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -25,8 +25,8 @@ struct pathspec {
unsigned magic;
int max_depth;
struct pathspec_item {
-   const char *match;
-   const char *original;
+   char *match;
+   char *original;
unsigned magic;
int len, prefix;
int nowildcard_len;
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v4 3/5] real_path: create real_pathdup

[Brandon Williams]

Create real_pathdup which returns a caller owned string of the resolved
realpath based on the provide path.

Signed-off-by: Brandon Williams 
---
 abspath.c | 13 +
 cache.h   |  1 +
 2 files changed, 14 insertions(+)

diff --git a/abspath.c b/abspath.c
index c3a6acd4d..f4283f465 100644
--- a/abspath.c
+++ b/abspath.c
@@ -205,6 +205,19 @@ const char *real_path_if_valid(const char *path)
return strbuf_realpath(, path, 0);
 }
 
+char *real_pathdup(const char *path)
+{
+   struct strbuf realpath = STRBUF_INIT;
+   char *retval = NULL;
+
+   if (strbuf_realpath(, path, 0))
+   retval = strbuf_detach(, NULL);
+
+   strbuf_release();
+
+   return retval;
+}
+
 /*
  * Use this to get an absolute path from a relative one. If you want
  * to resolve links, you should use real_path.
diff --git a/cache.h b/cache.h
index 7a8129403..e12a5d912 100644
--- a/cache.h
+++ b/cache.h
@@ -1068,6 +1068,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char 
*path,
  int die_on_error);
 const char *real_path(const char *path);
 const char *real_path_if_valid(const char *path);
+char *real_pathdup(const char *path);
 const char *absolute_path(const char *path);
 const char *remove_leading_path(const char *in, const char *prefix);
 const char *relative_path(const char *in, const char *prefix, struct strbuf 
*sb);
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v4 4/5] real_path: have callers use real_pathdup and strbuf_realpath

[Brandon Williams]

Migrate callers of real_path() who duplicate the retern value to use
real_pathdup or strbuf_realpath.

Signed-off-by: Brandon Williams 
---
 builtin/init-db.c |  6 +++---
 environment.c |  2 +-
 setup.c   | 13 -
 sha1_file.c   |  2 +-
 submodule.c   |  2 +-
 transport.c   |  2 +-
 worktree.c|  2 +-
 7 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 2399b97d9..76d68fad0 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
 {
int reinit;
int exist_ok = flags & INIT_DB_EXIST_OK;
-   char *original_git_dir = xstrdup(real_path(git_dir));
+   char *original_git_dir = real_pathdup(git_dir);
 
if (real_git_dir) {
struct stat st;
@@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, init_db_options, 
init_db_usage, 0);
 
if (real_git_dir && !is_absolute_path(real_git_dir))
-   real_git_dir = xstrdup(real_path(real_git_dir));
+   real_git_dir = real_pathdup(real_git_dir);
 
if (argc == 1) {
int mkdir_tried = 0;
@@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
const char *git_dir_parent = strrchr(git_dir, '/');
if (git_dir_parent) {
char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
-   git_work_tree_cfg = xstrdup(real_path(rel));
+   git_work_tree_cfg = real_pathdup(rel);
free(rel);
}
if (!git_work_tree_cfg)
diff --git a/environment.c b/environment.c
index 0935ec696..9b943d2d5 100644
--- a/environment.c
+++ b/environment.c
@@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)
return;
}
git_work_tree_initialized = 1;
-   work_tree = xstrdup(real_path(new_work_tree));
+   work_tree = real_pathdup(new_work_tree);
 }
 
 const char *get_git_work_tree(void)
diff --git a/setup.c b/setup.c
index fe572b82c..1b534a750 100644
--- a/setup.c
+++ b/setup.c
@@ -256,8 +256,10 @@ int get_common_dir_noenv(struct strbuf *sb, const char 
*gitdir)
strbuf_addbuf(, );
strbuf_addstr(sb, real_path(path.buf));
ret = 1;
-   } else
+   } else {
strbuf_addstr(sb, gitdir);
+   }
+
strbuf_release();
strbuf_release();
return ret;
@@ -692,7 +694,7 @@ static const char *setup_discovered_git_dir(const char 
*gitdir,
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
if (offset != cwd->len && !is_absolute_path(gitdir))
-   gitdir = xstrdup(real_path(gitdir));
+   gitdir = real_pathdup(gitdir);
if (chdir(cwd->buf))
die_errno("Could not come back to cwd");
return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
@@ -800,11 +802,12 @@ static int canonicalize_ceiling_entry(struct 
string_list_item *item,
/* Keep entry but do not canonicalize it */
return 1;
} else {
-   const char *real_path = real_path_if_valid(ceil);
-   if (!real_path)
+   char *real_path = real_pathdup(ceil);
+   if (!real_path) {
return 0;
+   }
free(item->string);
-   item->string = xstrdup(real_path);
+   item->string = real_path;
return 1;
}
 }
diff --git a/sha1_file.c b/sha1_file.c
index 9c86d1924..6a03cc393 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -291,7 +291,7 @@ static int link_alt_odb_entry(const char *entry, const char 
*relative_base,
struct strbuf pathbuf = STRBUF_INIT;
 
if (!is_absolute_path(entry) && relative_base) {
-   strbuf_addstr(, real_path(relative_base));
+   strbuf_realpath(, relative_base, 1);
strbuf_addch(, '/');
}
strbuf_addstr(, entry);
diff --git a/submodule.c b/submodule.c
index ece17315d..55819ba9c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1301,7 +1301,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, 
const char *git_dir)
 {
struct strbuf file_name = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
-   const char *real_work_tree = xstrdup(real_path(work_tree));
+   const char *real_work_tree = real_pathdup(work_tree);
 
/* Update gitfile */
strbuf_addf(_name, "%s/.git", work_tree);
diff --git a/transport.c b/transport.c
index 04e5d6623..a3b98f198 100644
--- a/transport.c
+++ b/transport.c
@@ -1146,7 +1146,7 @@ 

[PATCH v4 2/5] real_path: convert real_path_internal to strbuf_realpath

[Brandon Williams]

Change the name of real_path_internal to strbuf_realpath.  In addition
push the static strbuf up to its callers and instead take as a
parameter a pointer to a strbuf to use for the final result.

This change makes strbuf_realpath reentrant.

Signed-off-by: Brandon Williams 
---
 abspath.c | 53 +
 cache.h   |  2 ++
 2 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/abspath.c b/abspath.c
index 0f34636a8..c3a6acd4d 100644
--- a/abspath.c
+++ b/abspath.c
@@ -55,21 +55,17 @@ static void get_next_component(struct strbuf *next, struct 
strbuf *remaining)
  * Return the real path (i.e., absolute path, with symlinks resolved
  * and extra slashes removed) equivalent to the specified path.  (If
  * you want an absolute path but don't mind links, use
- * absolute_path().)  The return value is a pointer to a static
- * buffer.
+ * absolute_path().)  Places the resolved realpath in the provided strbuf.
  *
  * The directory part of path (i.e., everything up to the last
  * dir_sep) must denote a valid, existing directory, but the last
  * component need not exist.  If die_on_error is set, then die with an
  * informative error message if there is a problem.  Otherwise, return
  * NULL on errors (without generating any output).
- *
- * If path is our buffer, then return path, as it's already what the
- * user wants.
  */
-static const char *real_path_internal(const char *path, int die_on_error)
+char *strbuf_realpath(struct strbuf *resolved, const char *path,
+ int die_on_error)
 {
-   static struct strbuf resolved = STRBUF_INIT;
struct strbuf remaining = STRBUF_INIT;
struct strbuf next = STRBUF_INIT;
struct strbuf symlink = STRBUF_INIT;
@@ -77,10 +73,6 @@ static const char *real_path_internal(const char *path, int 
die_on_error)
int num_symlinks = 0;
struct stat st;
 
-   /* We've already done it */
-   if (path == resolved.buf)
-   return path;
-
if (!*path) {
if (die_on_error)
die("The empty string is not a valid path");
@@ -88,16 +80,16 @@ static const char *real_path_internal(const char *path, int 
die_on_error)
goto error_out;
}
 
-   strbuf_reset();
+   strbuf_reset(resolved);
 
if (is_absolute_path(path)) {
/* absolute path; start with only root as being resolved */
int offset = offset_1st_component(path);
-   strbuf_add(, path, offset);
+   strbuf_add(resolved, path, offset);
strbuf_addstr(, path + offset);
} else {
/* relative path; can use CWD as the initial resolved path */
-   if (strbuf_getcwd()) {
+   if (strbuf_getcwd(resolved)) {
if (die_on_error)
die_errno("unable to get current working 
directory");
else
@@ -116,21 +108,21 @@ static const char *real_path_internal(const char *path, 
int die_on_error)
continue; /* '.' component */
} else if (next.len == 2 && !strcmp(next.buf, "..")) {
/* '..' component; strip the last path component */
-   strip_last_component();
+   strip_last_component(resolved);
continue;
}
 
/* append the next component and resolve resultant path */
-   if (!is_dir_sep(resolved.buf[resolved.len - 1]))
-   strbuf_addch(, '/');
-   strbuf_addbuf(, );
+   if (!is_dir_sep(resolved->buf[resolved->len - 1]))
+   strbuf_addch(resolved, '/');
+   strbuf_addbuf(resolved, );
 
-   if (lstat(resolved.buf, )) {
+   if (lstat(resolved->buf, )) {
/* error out unless this was the last component */
if (errno != ENOENT || remaining.len) {
if (die_on_error)
die_errno("Invalid path '%s'",
- resolved.buf);
+ resolved->buf);
else
goto error_out;
}
@@ -146,12 +138,12 @@ static const char *real_path_internal(const char *path, 
int die_on_error)
goto error_out;
}
 
-   len = strbuf_readlink(, resolved.buf,
+   len = strbuf_readlink(, resolved->buf,
  st.st_size);
if (len < 0) {
if (die_on_error)
die_errno("Invalid symlink '%s'",
-   

[PATCH v4 5/5] real_path: canonicalize directory separators in root parts

[Brandon Williams]

From: Johannes Sixt 

When an absolute path is resolved, resolution begins at the first path
component after the root part. The root part is just copied verbatim,
because it must not be inspected for symbolic links. For POSIX paths,
this is just the initial slash, but on Windows, the root part has the
forms c:\ or \\server\share. We do want to canonicalize the back-slashes
in the root part because these parts are compared to the result of
getcwd(), which does return a fully canonicalized path.

Factor out a helper that splits off the root part, and have it
canonicalize the copied part.

This change was prompted because t1504-ceiling-dirs.sh caught a breakage
in GIT_CEILING_DIRECTORIES handling on Windows.

Signed-off-by: Johannes Sixt 
---
 abspath.c | 29 +
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/abspath.c b/abspath.c
index f4283f465..3562d17bf 100644
--- a/abspath.c
+++ b/abspath.c
@@ -48,6 +48,19 @@ static void get_next_component(struct strbuf *next, struct 
strbuf *remaining)
strbuf_remove(remaining, 0, end - remaining->buf);
 }
 
+/* copies root part from remaining to resolved, canonicalizing it on the way */
+static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
+{
+   int offset = offset_1st_component(remaining->buf);
+
+   strbuf_reset(resolved);
+   strbuf_add(resolved, remaining->buf, offset);
+#ifdef GIT_WINDOWS_NATIVE
+   convert_slashes(resolved->buf);
+#endif
+   strbuf_remove(remaining, 0, offset);
+}
+
 /* We allow "recursive" symbolic links. Only within reason, though. */
 #define MAXDEPTH 5
 
@@ -80,14 +93,10 @@ char *strbuf_realpath(struct strbuf *resolved, const char 
*path,
goto error_out;
}
 
-   strbuf_reset(resolved);
+   strbuf_addstr(, path);
+   get_root_part(resolved, );
 
-   if (is_absolute_path(path)) {
-   /* absolute path; start with only root as being resolved */
-   int offset = offset_1st_component(path);
-   strbuf_add(resolved, path, offset);
-   strbuf_addstr(, path + offset);
-   } else {
+   if (!resolved->len) {
/* relative path; can use CWD as the initial resolved path */
if (strbuf_getcwd(resolved)) {
if (die_on_error)
@@ -95,7 +104,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char 
*path,
else
goto error_out;
}
-   strbuf_addstr(, path);
}
 
/* Iterate over the remaining path components */
@@ -150,10 +158,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char 
*path,
 
if (is_absolute_path(symlink.buf)) {
/* absolute symlink; set resolved to root */
-   int offset = offset_1st_component(symlink.buf);
-   strbuf_reset(resolved);
-   strbuf_add(resolved, symlink.buf, offset);
-   strbuf_remove(, 0, offset);
+   get_root_part(resolved, );
} else {
/*
 * relative symlink
-- 
2.11.0.390.gc69c2f50cf-goog

Re: [RFC PATCH 0/5] Localise error headers

[Stefan Beller]

On Mon, Jan 2, 2017 at 3:14 AM, Michael J Gruber
 wrote:
> Currently, the headers "error: ", "warning: " etc. - generated by die(),
> warning() etc. - are not localized, but we feed many localized error messages
> into these functions so that we produce error messages with mixed 
> localisation.
>
> This series introduces variants of die() etc. that use localised variants of
> the headers, i.e. _("error: ") etc., and are to be fed localized messages. So,
> instead of die(_("not workee")), which would produce a mixed localisation 
> (such
> as "error: geht ned"), one should use die_(_("not workee")) (resulting in
> "Fehler: geht ned").

Have you considered

die_("non localized error here")

to produce:

"Fehler: trotzdem uebersetzt hier"
("error: here it is translated")

>
> In this implementation, the gettext call for the header and the body are done
> in different places (error function vs. caller) but this call pattern seems to
> be the easiest variant for the caller, because the message body has to be 
> marked
> for localisation in any case, and N_() requires more letters than _(), an 
> extra
> argument to die() etc. even more than the extra "_" in the function name.

I see. We have to markup the strings to be translatable such that the .po files
are complete. It would be really handy if there was a way to say "anything that
is fed to this function (die_) needs to be marked for translation.

Looking through
https://www.gnu.org/software/gettext/manual/html_node/xgettext-Invocation.html
such a thing doesn't seem to exist.

So in that case die_(_(...)) seems to be the easiest way forward.

Thanks,
Stefan

[PATCH v4 07/16] pathspec: remove unused variable from unsupported_magic

[Brandon Williams]

Removed unused variable 'n' from the 'unsupported_magic()' function.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index b8faa8f46..b9a3819d6 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -333,8 +333,8 @@ static void NORETURN unsupported_magic(const char *pattern,
   unsigned short_magic)
 {
struct strbuf sb = STRBUF_INIT;
-   int i, n;
-   for (n = i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+   int i;
+   for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
const struct pathspec_magic *m = pathspec_magic + i;
if (!(magic & m->bit))
continue;
@@ -344,7 +344,6 @@ static void NORETURN unsupported_magic(const char *pattern,
strbuf_addf(, "'%c'", m->mnemonic);
else
strbuf_addf(, "'%s'", m->name);
-   n++;
}
/*
 * We may want to substitute "this command" with a command
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v4 14/16] pathspec: create strip submodule slash helpers

[Brandon Williams]

Factor out the logic responsible for stripping the trailing slash on
pathspecs referencing submodules into its own function.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 68 ++
 1 file changed, 42 insertions(+), 26 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index fe811a0a4..4a1f8ea44 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -258,6 +258,44 @@ static const char *parse_element_magic(unsigned *magic, 
int *prefix_len,
return parse_short_magic(magic, elem);
 }
 
+static void strip_submodule_slash_cheap(struct pathspec_item *item)
+{
+   if (item->len >= 1 && item->match[item->len - 1] == '/') {
+   int i = cache_name_pos(item->match, item->len - 1);
+
+   if (i >= 0 && S_ISGITLINK(active_cache[i]->ce_mode)) {
+   item->len--;
+   item->match[item->len] = '\0';
+   }
+   }
+}
+
+static void strip_submodule_slash_expensive(struct pathspec_item *item)
+{
+   int i;
+
+   for (i = 0; i < active_nr; i++) {
+   struct cache_entry *ce = active_cache[i];
+   int ce_len = ce_namelen(ce);
+
+   if (!S_ISGITLINK(ce->ce_mode))
+   continue;
+
+   if (item->len <= ce_len || item->match[ce_len] != '/' ||
+   memcmp(ce->name, item->match, ce_len))
+   continue;
+
+   if (item->len == ce_len + 1) {
+   /* strip trailing slash */
+   item->len--;
+   item->match[item->len] = '\0';
+   } else {
+   die(_("Pathspec '%s' is in submodule '%.*s'"),
+   item->original, ce_len, ce->name);
+   }
+   }
+}
+
 /*
  * Take an element of a pathspec and check for magic signatures.
  * Append the result to the prefix. Return the magic bitmap.
@@ -278,7 +316,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, 
unsigned flags,
unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
char *match;
-   int i, pathspec_prefix = -1;
+   int pathspec_prefix = -1;
 
/* PATHSPEC_LITERAL_PATH ignores magic */
if (flags & PATHSPEC_LITERAL_PATH) {
@@ -329,33 +367,11 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item, unsigned flags,
item->len = strlen(item->match);
item->prefix = prefixlen;
 
-   if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
-   (item->len >= 1 && item->match[item->len - 1] == '/') &&
-   (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
-   S_ISGITLINK(active_cache[i]->ce_mode)) {
-   item->len--;
-   match[item->len] = '\0';
-   }
+   if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
+   strip_submodule_slash_cheap(item);
 
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
-   for (i = 0; i < active_nr; i++) {
-   struct cache_entry *ce = active_cache[i];
-   int ce_len = ce_namelen(ce);
-
-   if (!S_ISGITLINK(ce->ce_mode))
-   continue;
-
-   if (item->len <= ce_len || match[ce_len] != '/' ||
-   memcmp(ce->name, match, ce_len))
-   continue;
-   if (item->len == ce_len + 1) {
-   /* strip trailing slash */
-   item->len--;
-   match[item->len] = '\0';
-   } else
-   die (_("Pathspec '%s' is in submodule '%.*s'"),
-elt, ce_len, ce->name);
-   }
+   strip_submodule_slash_expensive(item);
 
if (magic & PATHSPEC_LITERAL)
item->nowildcard_len = item->len;
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v4 08/16] pathspec: always show mnemonic and name in unsupported_magic

[Brandon Williams]

For better clarity, always show the mnemonic and name of the unsupported
magic being used.  This lets users have a more clear understanding of
what magic feature isn't supported.  And if they supplied a mnemonic,
the user will be told what its corresponding name is which will allow
them to more easily search the man pages for that magic type.

This also avoids passing an extra parameter around the pathspec
initialization code.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 24 +---
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index b9a3819d6..ee87494c7 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -101,9 +101,7 @@ static void prefix_short_magic(struct strbuf *sb, int 
prefixlen,
  * the prefix part must always match literally, and a single stupid
  * string cannot express such a case.
  */
-static unsigned prefix_pathspec(struct pathspec_item *item,
-   unsigned *p_short_magic,
-   unsigned flags,
+static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
 {
@@ -210,7 +208,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
 
magic |= short_magic;
-   *p_short_magic = short_magic;
 
/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
if (noglob_global && !(magic & PATHSPEC_GLOB))
@@ -329,8 +326,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
 }
 
 static void NORETURN unsupported_magic(const char *pattern,
-  unsigned magic,
-  unsigned short_magic)
+  unsigned magic)
 {
struct strbuf sb = STRBUF_INIT;
int i;
@@ -339,9 +335,11 @@ static void NORETURN unsupported_magic(const char *pattern,
if (!(magic & m->bit))
continue;
if (sb.len)
-   strbuf_addch(, ' ');
-   if (short_magic & m->bit)
-   strbuf_addf(, "'%c'", m->mnemonic);
+   strbuf_addstr(, ", ");
+
+   if (m->mnemonic)
+   strbuf_addf(, "'%s' (mnemonic: '%c')",
+   m->name, m->mnemonic);
else
strbuf_addf(, "'%s'", m->name);
}
@@ -413,11 +411,9 @@ void parse_pathspec(struct pathspec *pathspec,
prefixlen = prefix ? strlen(prefix) : 0;
 
for (i = 0; i < n; i++) {
-   unsigned short_magic;
entry = argv[i];
 
-   item[i].magic = prefix_pathspec(item + i, _magic,
-   flags,
+   item[i].magic = prefix_pathspec(item + i, flags,
prefix, prefixlen, entry);
if ((flags & PATHSPEC_LITERAL_PATH) &&
!(magic_mask & PATHSPEC_LITERAL))
@@ -425,9 +421,7 @@ void parse_pathspec(struct pathspec *pathspec,
if (item[i].magic & PATHSPEC_EXCLUDE)
nr_exclude++;
if (item[i].magic & magic_mask)
-   unsupported_magic(entry,
- item[i].magic & magic_mask,
- short_magic);
+   unsupported_magic(entry, item[i].magic & magic_mask);
 
if ((flags & PATHSPEC_SYMLINK_LEADING_PATH) &&
has_symlink_leading_path(item[i].match, item[i].len)) {
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v4 09/16] pathspec: simpler logic to prefix original pathspec elements

[Brandon Williams]

The logic used to prefix an original pathspec element with 'prefix'
magic is more general purpose and can be used for more than just short
magic.  Remove the extra code paths and rename 'prefix_short_magic' to
'prefix_magic' to better indicate that it can be used in more general
situations.

Also, slightly change the logic which decides when to prefix the
original element in order to prevent a pathspec of "." from getting
converted to "" (empty string).

Signed-off-by: Brandon Williams 
---
 pathspec.c | 33 +
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index ee87494c7..032436bc1 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -74,13 +74,12 @@ static struct pathspec_magic {
{ PATHSPEC_EXCLUDE, '!', "exclude" },
 };
 
-static void prefix_short_magic(struct strbuf *sb, int prefixlen,
-  unsigned short_magic)
+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
 {
int i;
strbuf_addstr(sb, ":(");
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
-   if (short_magic & pathspec_magic[i].bit) {
+   if (magic & pathspec_magic[i].bit) {
if (sb->buf[sb->len - 1] != '(')
strbuf_addch(sb, ',');
strbuf_addstr(sb, pathspec_magic[i].name);
@@ -109,8 +108,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item, 
unsigned flags,
static int glob_global = -1;
static int noglob_global = -1;
static int icase_global = -1;
-   unsigned magic = 0, short_magic = 0, global_magic = 0;
-   const char *copyfrom = elt, *long_magic_end = NULL;
+   unsigned magic = 0, element_magic = 0, global_magic = 0;
+   const char *copyfrom = elt;
char *match;
int i, pathspec_prefix = -1;
 
@@ -164,7 +163,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, 
unsigned flags,
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
if (strlen(pathspec_magic[i].name) == len &&
!strncmp(pathspec_magic[i].name, copyfrom, 
len)) {
-   magic |= pathspec_magic[i].bit;
+   element_magic |= pathspec_magic[i].bit;
break;
}
if (starts_with(copyfrom, "prefix:")) {
@@ -183,7 +182,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item, 
unsigned flags,
}
if (*copyfrom != ')')
die(_("Missing ')' at the end of pathspec magic in 
'%s'"), elt);
-   long_magic_end = copyfrom;
copyfrom++;
} else {
/* shorthand */
@@ -196,7 +194,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, 
unsigned flags,
break;
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
if (pathspec_magic[i].mnemonic == ch) {
-   short_magic |= pathspec_magic[i].bit;
+   element_magic |= pathspec_magic[i].bit;
break;
}
if (ARRAY_SIZE(pathspec_magic) <= i)
@@ -207,7 +205,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, 
unsigned flags,
copyfrom++;
}
 
-   magic |= short_magic;
+   magic |= element_magic;
 
/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
if (noglob_global && !(magic & PATHSPEC_GLOB))
@@ -242,18 +240,13 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item, unsigned flags,
 * Prefix the pathspec (keep all magic) and assign to
 * original. Useful for passing to another command.
 */
-   if (flags & PATHSPEC_PREFIX_ORIGIN) {
+   if ((flags & PATHSPEC_PREFIX_ORIGIN) &&
+   prefixlen && !literal_global) {
struct strbuf sb = STRBUF_INIT;
-   if (prefixlen && !literal_global) {
-   /* Preserve the actual prefix length of each pattern */
-   if (short_magic)
-   prefix_short_magic(, prefixlen, short_magic);
-   else if (long_magic_end) {
-   strbuf_add(, elt, long_magic_end - elt);
-   strbuf_addf(, ",prefix:%d)", prefixlen);
-   } else
-   strbuf_addf(, ":(prefix:%d)", prefixlen);
-   }
+
+   /* Preserve the actual prefix length of each pattern */
+   prefix_magic(, prefixlen, element_magic);
+
strbuf_addstr(, 

[PATCH v4 00/16] pathspec cleanup

[Brandon Williams]

v4 addresses a few comments from Duy.
* [2/16] push the guard pathspec macro into simplify_away() and
  exclude_matches_pathsepc().
* [6/16] when freeing a pathspec struct, set pathsepc->nr = 0.
* [8/16] tweak the die message when using unsupported magic to be more human
  readable.

Brandon Williams (16):
  mv: remove use of deprecated 'get_pathspec()'
  dir: remove struct path_simplify
  dir: convert fill_directory to use the pathspec struct interface
  ls-tree: convert show_recursive to use the pathspec struct interface
  pathspec: remove the deprecated get_pathspec function
  pathspec: copy and free owned memory
  pathspec: remove unused variable from unsupported_magic
  pathspec: always show mnemonic and name in unsupported_magic
  pathspec: simpler logic to prefix original pathspec elements
  pathspec: factor global magic into its own function
  pathspec: create parse_short_magic function
  pathspec: create parse_long_magic function
  pathspec: create parse_element_magic helper
  pathspec: create strip submodule slash helpers
  pathspec: small readability changes
  pathspec: rename prefix_pathspec to init_pathspec_item

 Documentation/technical/api-setup.txt |   2 -
 builtin/ls-tree.c |  16 +-
 builtin/mv.c  |  50 ++--
 cache.h   |   1 -
 dir.c | 193 ++
 pathspec.c| 480 +++---
 pathspec.h|   5 +-
 7 files changed, 390 insertions(+), 357 deletions(-)

--- interdiff between v3 and v4

diff --git a/dir.c b/dir.c
index 15f7c9993..e8ddd7f8a 100644
--- a/dir.c
+++ b/dir.c
@@ -1353,6 +1353,15 @@ static int simplify_away(const char *path, int pathlen,
 {
int i;
 
+   if (pathspec)
+   guard_pathspec(pathspec,
+  pathspec_fromtop |
+  pathspec_maxdepth |
+  pathspec_literal |
+  pathspec_glob |
+  pathspec_icase |
+  pathspec_exclude);
+
if (!pathspec || !pathspec->nr)
return 0;
 
@@ -1385,6 +1394,15 @@ static int exclude_matches_pathspec(const char *path, 
int pathlen,
 {
int i;
 
+   if (pathspec)
+   guard_pathspec(pathspec,
+  pathspec_fromtop |
+  pathspec_maxdepth |
+  pathspec_literal |
+  pathspec_glob |
+  pathspec_icase |
+  pathspec_exclude);
+
if (!pathspec || !pathspec->nr)
return 0;
 
@@ -1996,15 +2014,6 @@ int read_directory(struct dir_struct *dir, const char 
*path,
 {
struct untracked_cache_dir *untracked;
 
-   if (pathspec)
-   guard_pathspec(pathspec,
-  pathspec_fromtop |
-  pathspec_maxdepth |
-  pathspec_literal |
-  pathspec_glob |
-  pathspec_icase |
-  pathspec_exclude);
-
if (has_symlink_leading_path(path, len))
return dir->nr;

diff --git a/pathspec.c b/pathspec.c
index d4efcf666..bcf3ba039 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -414,10 +414,11 @@ static void NORETURN unsupported_magic(const char 
*pattern,
if (!(magic & m->bit))
continue;
if (sb.len)
-   strbuf_addch(, ' ');
+   strbuf_addstr(, ", ");
 
if (m->mnemonic)
-   strbuf_addf(, "'(%c)%s'", m->mnemonic, m->name);
+   strbuf_addf(, "'%s' (mnemonic: '%c')",
+   m->name, m->mnemonic);
else
strbuf_addf(, "'%s'", m->name);
}
@@ -544,4 +545,5 @@ void clear_pathspec(struct pathspec *pathspec)
}
free(pathspec->items);
pathspec->items = NULL;
+   pathspec->nr = 0;
 }

-- 
2.11.0.390.gc69c2f50cf-goog

Re: What's cooking in git.git (Dec 2016, #08; Tue, 27)

[Brandon Williams]

On 12/29, Lars Schneider wrote:
> 
> > On 28 Dec 2016, at 00:11, Junio C Hamano  wrote:
> > 
> > 
> > * bw/realpath-wo-chdir (2016-12-22) 5 commits
> >  (merged to 'next' on 2016-12-22 at fea8fa870f)
> > + real_path: canonicalize directory separators in root parts
> > + real_path: have callers use real_pathdup and strbuf_realpath
> > + real_path: create real_pathdup
> > + real_path: convert real_path_internal to strbuf_realpath
> > + real_path: resolve symlinks by hand
> > (this branch is used by bw/grep-recurse-submodules.)
> > 
> > The implementation of "real_path()" was to go there with chdir(2)
> > and call getcwd(3), but this obviously wouldn't be usable in a
> > threaded environment.  Rewrite it to manually resolve relative
> > paths including symbolic links in path components.
> 
> "real_path: resolve symlinks by hand" (05b458c) introduces
> "MAXSYMLINKS" which is already defined on macOS in
> 
> /usr/include/sys/param.h:197:9:
> 
>  * .., MAXSYMLINKS defines the
>  * maximum number of symbolic links that may be expanded in a path name.
>  * It should be set high enough to allow all legitimate uses, but halt
>  * infinite loops reasonably quickly.
>  */
> 
> 
> Log with JS: https://travis-ci.org/git/git/jobs/187092215
> Log without JS: 
> https://s3.amazonaws.com/archive.travis-ci.org/jobs/187092215/log.txt
> 
> - Lars
> 

Simple fix would be to just revert MAXSYMLINKS to be MAXDEPTH (which is
what the #define was before this series).  I'll resend the series with
that fix.

-- 
Brandon Williams

[PATCH v4 15/16] pathspec: small readability changes

[Brandon Williams]

A few small changes to improve readability.  This is done by grouping related
assignments, adding blank lines, ensuring lines are <80 characters, and
adding additional comments.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 25 +++--
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 4a1f8ea44..ae9e1401f 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -67,11 +67,11 @@ static struct pathspec_magic {
char mnemonic; /* this cannot be ':'! */
const char *name;
 } pathspec_magic[] = {
-   { PATHSPEC_FROMTOP, '/', "top" },
-   { PATHSPEC_LITERAL,   0, "literal" },
-   { PATHSPEC_GLOB,   '\0', "glob" },
-   { PATHSPEC_ICASE,  '\0', "icase" },
-   { PATHSPEC_EXCLUDE, '!', "exclude" },
+   { PATHSPEC_FROMTOP,  '/', "top" },
+   { PATHSPEC_LITERAL, '\0', "literal" },
+   { PATHSPEC_GLOB,'\0', "glob" },
+   { PATHSPEC_ICASE,   '\0', "icase" },
+   { PATHSPEC_EXCLUDE,  '!', "exclude" },
 };
 
 static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
@@ -336,6 +336,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, 
unsigned flags,
if ((magic & PATHSPEC_LITERAL) && (magic & PATHSPEC_GLOB))
die(_("%s: 'literal' and 'glob' are incompatible"), elt);
 
+   /* Create match string which will be used for pathspec matching */
if (pathspec_prefix >= 0) {
match = xstrdup(copyfrom);
prefixlen = pathspec_prefix;
@@ -343,11 +344,16 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item, unsigned flags,
match = xstrdup(copyfrom);
prefixlen = 0;
} else {
-   match = prefix_path_gently(prefix, prefixlen, , 
copyfrom);
+   match = prefix_path_gently(prefix, prefixlen,
+  , copyfrom);
if (!match)
die(_("%s: '%s' is outside repository"), elt, copyfrom);
}
+
item->match = match;
+   item->len = strlen(item->match);
+   item->prefix = prefixlen;
+
/*
 * Prefix the pathspec (keep all magic) and assign to
 * original. Useful for passing to another command.
@@ -364,8 +370,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item, 
unsigned flags,
} else {
item->original = xstrdup(elt);
}
-   item->len = strlen(item->match);
-   item->prefix = prefixlen;
 
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
strip_submodule_slash_cheap(item);
@@ -373,13 +377,14 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item, unsigned flags,
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
strip_submodule_slash_expensive(item);
 
-   if (magic & PATHSPEC_LITERAL)
+   if (magic & PATHSPEC_LITERAL) {
item->nowildcard_len = item->len;
-   else {
+   } else {
item->nowildcard_len = simple_length(item->match);
if (item->nowildcard_len < prefixlen)
item->nowildcard_len = prefixlen;
}
+
item->flags = 0;
if (magic & PATHSPEC_GLOB) {
/*
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v4 1/5] real_path: resolve symlinks by hand

[Brandon Williams]

The current implementation of real_path uses chdir() in order to resolve
symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
process as a whole and not just an individual thread.  Instead perform
the symlink resolution by hand so that the calls to chdir() can be
removed, making real_path one step closer to being reentrant.

Signed-off-by: Brandon Williams 
---
 abspath.c | 188 ++
 1 file changed, 128 insertions(+), 60 deletions(-)

diff --git a/abspath.c b/abspath.c
index 2825de859..0f34636a8 100644
--- a/abspath.c
+++ b/abspath.c
@@ -11,6 +11,43 @@ int is_directory(const char *path)
return (!stat(path, ) && S_ISDIR(st.st_mode));
 }
 
+/* removes the last path component from 'path' except if 'path' is root */
+static void strip_last_component(struct strbuf *path)
+{
+   size_t offset = offset_1st_component(path->buf);
+   size_t len = path->len;
+
+   /* Find start of the last component */
+   while (offset < len && !is_dir_sep(path->buf[len - 1]))
+   len--;
+   /* Skip sequences of multiple path-separators */
+   while (offset < len && is_dir_sep(path->buf[len - 1]))
+   len--;
+
+   strbuf_setlen(path, len);
+}
+
+/* get (and remove) the next component in 'remaining' and place it in 'next' */
+static void get_next_component(struct strbuf *next, struct strbuf *remaining)
+{
+   char *start = NULL;
+   char *end = NULL;
+
+   strbuf_reset(next);
+
+   /* look for the next component */
+   /* Skip sequences of multiple path-separators */
+   for (start = remaining->buf; is_dir_sep(*start); start++)
+   ; /* nothing */
+   /* Find end of the path component */
+   for (end = start; *end && !is_dir_sep(*end); end++)
+   ; /* nothing */
+
+   strbuf_add(next, start, end - start);
+   /* remove the component from 'remaining' */
+   strbuf_remove(remaining, 0, end - remaining->buf);
+}
+
 /* We allow "recursive" symbolic links. Only within reason, though. */
 #define MAXDEPTH 5
 
@@ -21,7 +58,6 @@ int is_directory(const char *path)
  * absolute_path().)  The return value is a pointer to a static
  * buffer.
  *
- * The input and all intermediate paths must be shorter than MAX_PATH.
  * The directory part of path (i.e., everything up to the last
  * dir_sep) must denote a valid, existing directory, but the last
  * component need not exist.  If die_on_error is set, then die with an
@@ -33,22 +69,16 @@ int is_directory(const char *path)
  */
 static const char *real_path_internal(const char *path, int die_on_error)
 {
-   static struct strbuf sb = STRBUF_INIT;
+   static struct strbuf resolved = STRBUF_INIT;
+   struct strbuf remaining = STRBUF_INIT;
+   struct strbuf next = STRBUF_INIT;
+   struct strbuf symlink = STRBUF_INIT;
char *retval = NULL;
-
-   /*
-* If we have to temporarily chdir(), store the original CWD
-* here so that we can chdir() back to it at the end of the
-* function:
-*/
-   struct strbuf cwd = STRBUF_INIT;
-
-   int depth = MAXDEPTH;
-   char *last_elem = NULL;
+   int num_symlinks = 0;
struct stat st;
 
/* We've already done it */
-   if (path == sb.buf)
+   if (path == resolved.buf)
return path;
 
if (!*path) {
@@ -58,74 +88,112 @@ static const char *real_path_internal(const char *path, 
int die_on_error)
goto error_out;
}
 
-   strbuf_reset();
-   strbuf_addstr(, path);
-
-   while (depth--) {
-   if (!is_directory(sb.buf)) {
-   char *last_slash = find_last_dir_sep(sb.buf);
-   if (last_slash) {
-   last_elem = xstrdup(last_slash + 1);
-   strbuf_setlen(, last_slash - sb.buf + 1);
-   } else {
-   last_elem = xmemdupz(sb.buf, sb.len);
-   strbuf_reset();
-   }
+   strbuf_reset();
+
+   if (is_absolute_path(path)) {
+   /* absolute path; start with only root as being resolved */
+   int offset = offset_1st_component(path);
+   strbuf_add(, path, offset);
+   strbuf_addstr(, path + offset);
+   } else {
+   /* relative path; can use CWD as the initial resolved path */
+   if (strbuf_getcwd()) {
+   if (die_on_error)
+   die_errno("unable to get current working 
directory");
+   else
+   goto error_out;
}
+   strbuf_addstr(, path);
+   }
 
-   if (sb.len) {
-   if (!cwd.len && strbuf_getcwd()) {
+   /* Iterate over the remaining path components */
+   

[PATCH v4 02/16] dir: remove struct path_simplify

[Brandon Williams]

Teach simplify_away() and exclude_matches_pathspec() to handle struct
pathspec directly, eliminating the need for the struct path_simplify.

Also renamed the len parameter to pathlen in exclude_matches_pathspec()
to match the parameter names used in simplify_away().

Signed-off-by: Brandon Williams 
---
 dir.c | 181 +-
 1 file changed, 78 insertions(+), 103 deletions(-)

diff --git a/dir.c b/dir.c
index bfa8c8a9a..1df61f10f 100644
--- a/dir.c
+++ b/dir.c
@@ -16,11 +16,6 @@
 #include "varint.h"
 #include "ewah/ewok.h"
 
-struct path_simplify {
-   int len;
-   const char *path;
-};
-
 /*
  * Tells read_directory_recursive how a file or directory should be treated.
  * Values are ordered by significance, e.g. if a directory contains both
@@ -50,7 +45,7 @@ struct cached_dir {
 
 static enum path_treatment read_directory_recursive(struct dir_struct *dir,
const char *path, int len, struct untracked_cache_dir *untracked,
-   int check_only, const struct path_simplify *simplify);
+   int check_only, const struct pathspec *pathspec);
 static int get_dtype(struct dirent *de, const char *path, int len);
 
 int fspathcmp(const char *a, const char *b)
@@ -1312,7 +1307,7 @@ static enum exist_status directory_exists_in_index(const 
char *dirname, int len)
 static enum path_treatment treat_directory(struct dir_struct *dir,
struct untracked_cache_dir *untracked,
const char *dirname, int len, int baselen, int exclude,
-   const struct path_simplify *simplify)
+   const struct pathspec *pathspec)
 {
/* The "len-1" is to strip the final '/' */
switch (directory_exists_in_index(dirname, len-1)) {
@@ -1341,7 +1336,7 @@ static enum path_treatment treat_directory(struct 
dir_struct *dir,
untracked = lookup_untracked(dir->untracked, untracked,
 dirname + baselen, len - baselen);
return read_directory_recursive(dir, dirname, len,
-   untracked, 1, simplify);
+   untracked, 1, pathspec);
 }
 
 /*
@@ -1349,24 +1344,34 @@ static enum path_treatment treat_directory(struct 
dir_struct *dir,
  * reading - if the path cannot possibly be in the pathspec,
  * return true, and we'll skip it early.
  */
-static int simplify_away(const char *path, int pathlen, const struct 
path_simplify *simplify)
+static int simplify_away(const char *path, int pathlen,
+const struct pathspec *pathspec)
 {
-   if (simplify) {
-   for (;;) {
-   const char *match = simplify->path;
-   int len = simplify->len;
+   int i;
 
-   if (!match)
-   break;
-   if (len > pathlen)
-   len = pathlen;
-   if (!memcmp(path, match, len))
-   return 0;
-   simplify++;
-   }
-   return 1;
+   if (pathspec)
+   GUARD_PATHSPEC(pathspec,
+  PATHSPEC_FROMTOP |
+  PATHSPEC_MAXDEPTH |
+  PATHSPEC_LITERAL |
+  PATHSPEC_GLOB |
+  PATHSPEC_ICASE |
+  PATHSPEC_EXCLUDE);
+
+   if (!pathspec || !pathspec->nr)
+   return 0;
+
+   for (i = 0; i < pathspec->nr; i++) {
+   const struct pathspec_item *item = >items[i];
+   int len = item->nowildcard_len;
+
+   if (len > pathlen)
+   len = pathlen;
+   if (!ps_strncmp(item, item->match, path, len))
+   return 0;
}
-   return 0;
+
+   return 1;
 }
 
 /*
@@ -1380,19 +1385,34 @@ static int simplify_away(const char *path, int pathlen, 
const struct path_simpli
  *   2. the path is a directory prefix of some element in the
  *  pathspec
  */
-static int exclude_matches_pathspec(const char *path, int len,
-   const struct path_simplify *simplify)
-{
-   if (simplify) {
-   for (; simplify->path; simplify++) {
-   if (len == simplify->len
-   && !memcmp(path, simplify->path, len))
-   return 1;
-   if (len < simplify->len
-   && simplify->path[len] == '/'
-   && !memcmp(path, simplify->path, len))
-   return 1;
-   }
+static int exclude_matches_pathspec(const char *path, int pathlen,
+   const struct pathspec *pathspec)
+{
+   int i;
+
+   if (pathspec)
+   GUARD_PATHSPEC(pathspec,
+  PATHSPEC_FROMTOP |
+

[PATCH v4 0/5] road to reentrant real_path

[Brandon Williams]

Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
MAXDEPTH due to a naming conflict brought up by Lars Schneider.

Brandon Williams (4):
  real_path: resolve symlinks by hand
  real_path: convert real_path_internal to strbuf_realpath
  real_path: create real_pathdup
  real_path: have callers use real_pathdup and strbuf_realpath

Johannes Sixt (1):
  real_path: canonicalize directory separators in root parts

 abspath.c | 225 +-
 builtin/init-db.c |   6 +-
 cache.h   |   3 +
 environment.c |   2 +-
 setup.c   |  13 ++--
 sha1_file.c   |   2 +-
 submodule.c   |   2 +-
 transport.c   |   2 +-
 worktree.c|   2 +-
 9 files changed, 173 insertions(+), 84 deletions(-)

--- interdiff with v3

diff --git a/abspath.c b/abspath.c
index 1d56f5ed9..3562d17bf 100644
--- a/abspath.c
+++ b/abspath.c
@@ -62,7 +62,7 @@ static void get_root_part(struct strbuf *resolved, struct 
strbuf *remaining)
 }
 
 /* We allow "recursive" symbolic links. Only within reason, though. */
-#define MAXSYMLINKS 5
+#define MAXDEPTH 5
 
 /*
  * Return the real path (i.e., absolute path, with symlinks resolved
@@ -138,10 +138,10 @@ char *strbuf_realpath(struct strbuf *resolved, const char 
*path,
ssize_t len;
strbuf_reset();
 
-   if (num_symlinks++ > MAXSYMLINKS) {
+   if (num_symlinks++ > MAXDEPTH) {
if (die_on_error)
die("More than %d nested symlinks "
-   "on path '%s'", MAXSYMLINKS, path);
+   "on path '%s'", MAXDEPTH, path);
else
goto error_out;
}

-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v4 12/16] pathspec: create parse_long_magic function

[Brandon Williams]

Factor out the logic responsible for parsing long magic into its own
function.  As well as hoist the prefix check logic outside of the inner
loop as there isn't anything that needs to be done after matching
"prefix:".

Signed-off-by: Brandon Williams 
---
 pathspec.c | 92 ++
 1 file changed, 57 insertions(+), 35 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 8574010d5..f1439f6f9 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -157,6 +157,60 @@ static int get_global_magic(int element_magic)
 }
 
 /*
+ * Parse the pathspec element looking for long magic
+ *
+ * saves all magic in 'magic'
+ * if prefix magic is used, save the prefix length in 'prefix_len'
+ * returns the position in 'elem' after all magic has been parsed
+ */
+static const char *parse_long_magic(unsigned *magic, int *prefix_len,
+   const char *elem)
+{
+   const char *pos;
+   const char *nextat;
+
+   for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {
+   size_t len = strcspn(pos, ",)");
+   int i;
+
+   if (pos[len] == ',')
+   nextat = pos + len + 1; /* handle ',' */
+   else
+   nextat = pos + len; /* handle ')' and '\0' */
+
+   if (!len)
+   continue;
+
+   if (starts_with(pos, "prefix:")) {
+   char *endptr;
+   *prefix_len = strtol(pos + 7, , 10);
+   if (endptr - pos != len)
+   die(_("invalid parameter for pathspec magic 
'prefix'"));
+   continue;
+   }
+
+   for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+   if (strlen(pathspec_magic[i].name) == len &&
+   !strncmp(pathspec_magic[i].name, pos, len)) {
+   *magic |= pathspec_magic[i].bit;
+   break;
+   }
+   }
+
+   if (ARRAY_SIZE(pathspec_magic) <= i)
+   die(_("Invalid pathspec magic '%.*s' in '%s'"),
+   (int) len, pos, elem);
+   }
+
+   if (*pos != ')')
+   die(_("Missing ')' at the end of pathspec magic in '%s'"),
+   elem);
+   pos++;
+
+   return pos;
+}
+
+/*
  * Parse the pathspec element looking for short magic
  *
  * saves all magic in 'magic'
@@ -218,41 +272,9 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item, unsigned flags,
; /* nothing to do */
} else if (elt[1] == '(') {
/* longhand */
-   const char *nextat;
-   for (copyfrom = elt + 2;
-*copyfrom && *copyfrom != ')';
-copyfrom = nextat) {
-   size_t len = strcspn(copyfrom, ",)");
-   if (copyfrom[len] == ',')
-   nextat = copyfrom + len + 1;
-   else
-   /* handle ')' and '\0' */
-   nextat = copyfrom + len;
-   if (!len)
-   continue;
-   for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
-   if (strlen(pathspec_magic[i].name) == len &&
-   !strncmp(pathspec_magic[i].name, copyfrom, 
len)) {
-   element_magic |= pathspec_magic[i].bit;
-   break;
-   }
-   if (starts_with(copyfrom, "prefix:")) {
-   char *endptr;
-   pathspec_prefix = strtol(copyfrom + 7,
-, 10);
-   if (endptr - copyfrom != len)
-   die(_("invalid parameter for 
pathspec magic 'prefix'"));
-   /* "i" would be wrong, but it does not 
matter */
-   break;
-   }
-   }
-   if (ARRAY_SIZE(pathspec_magic) <= i)
-   die(_("Invalid pathspec magic '%.*s' in '%s'"),
-   (int) len, copyfrom, elt);
-   }
-   if (*copyfrom != ')')
-   die(_("Missing ')' at the end of pathspec magic in 
'%s'"), elt);
-   copyfrom++;
+   copyfrom = parse_long_magic(_magic,
+   _prefix,
+   elt);
} else {
/* shorthand */
copyfrom = 

[PATCHv2] submodule.c: use GIT_DIR_ENVIRONMENT consistently

[Stefan Beller]

In C code we have the luxury of having constants for all the important
things that are hard coded. This is the only place in C, that hard codes
the git directory environment variable, so fix it.

Signed-off-by: Stefan Beller 
---

v2:
  argv_array_pushf and realigned.

v1:
  Signed-off-by-the-format-patch-config ;)
  
  This is the only occurrence for "GIT_DIR=" in C, but what about ".git"
  git grep "\.git\"" *.c finds some places, which we may want to convert
  to DEFAULT_GIT_DIR_ENVIRONMENT?
  (mainly things that are newer if I can judge the places correctly
  lots of submodules, worktrees and the no data in ".git" bug AFAICT)
  
  Thanks,
  Stefan

 submodule.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index ece17315d6..973b9f3f96 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1333,5 +1333,6 @@ void prepare_submodule_repo_env(struct argv_array *out)
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
}
-   argv_array_push(out, "GIT_DIR=.git");
+   argv_array_pushf(out, "%s=%s", GIT_DIR_ENVIRONMENT,
+DEFAULT_GIT_DIR_ENVIRONMENT);
 }
-- 
2.11.0.259.ga95e92af08.dirty

[PATCH v4 04/16] ls-tree: convert show_recursive to use the pathspec struct interface

[Brandon Williams]

Convert 'show_recursive()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.

Signed-off-by: Brandon Williams 
---
 builtin/ls-tree.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 0e30d8623..d7ebeb4ce 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -31,21 +31,18 @@ static const  char * const ls_tree_usage[] = {
 
 static int show_recursive(const char *base, int baselen, const char *pathname)
 {
-   const char **s;
+   int i;
 
if (ls_options & LS_RECURSIVE)
return 1;
 
-   s = pathspec._raw;
-   if (!s)
+   if (!pathspec.nr)
return 0;
 
-   for (;;) {
-   const char *spec = *s++;
+   for (i = 0; i < pathspec.nr; i++) {
+   const char *spec = pathspec.items[i].match;
int len, speclen;
 
-   if (!spec)
-   return 0;
if (strncmp(base, spec, baselen))
continue;
len = strlen(pathname);
@@ -59,6 +56,7 @@ static int show_recursive(const char *base, int baselen, 
const char *pathname)
continue;
return 1;
}
+   return 0;
 }
 
 static int show_tree(const unsigned char *sha1, struct strbuf *base,
@@ -175,8 +173,8 @@ int cmd_ls_tree(int argc, const char **argv, const char 
*prefix)
 * cannot be lifted until it is converted to use
 * match_pathspec() or tree_entry_interesting()
 */
-   parse_pathspec(, PATHSPEC_GLOB | PATHSPEC_ICASE |
- PATHSPEC_EXCLUDE,
+   parse_pathspec(, PATHSPEC_ALL_MAGIC &
+ ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
   PATHSPEC_PREFER_CWD,
   prefix, argv + 1);
for (i = 0; i < pathspec.nr; i++)
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v4 01/16] mv: remove use of deprecated 'get_pathspec()'

[Brandon Williams]

Convert the 'internal_copy_pathspec()' function to 'prefix_path()'
instead of using the deprecated 'get_pathspec()' interface.  Also,
rename 'internal_copy_pathspec()' to 'internal_prefix_pathspec()' to be
more descriptive of what the funciton is actually doing.

In addition to this, fix a memory leak caused by only duplicating some
of the pathspec elements.  Instead always duplicate all of the the
pathspec elements as an intermediate step (with modificationed based on
the passed in flags).  This way the intermediate strings can then be
freed after getting the result from 'prefix_path()'.

Signed-off-by: Brandon Williams 
---
 builtin/mv.c | 50 +++---
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 2f43877bc..4e86dc523 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2006 Johannes Schindelin
  */
 #include "builtin.h"
+#include "pathspec.h"
 #include "lockfile.h"
 #include "dir.h"
 #include "cache-tree.h"
@@ -19,31 +20,42 @@ static const char * const builtin_mv_usage[] = {
 #define DUP_BASENAME 1
 #define KEEP_TRAILING_SLASH 2
 
-static const char **internal_copy_pathspec(const char *prefix,
-  const char **pathspec,
-  int count, unsigned flags)
+static const char **internal_prefix_pathspec(const char *prefix,
+const char **pathspec,
+int count, unsigned flags)
 {
int i;
const char **result;
+   int prefixlen = prefix ? strlen(prefix) : 0;
ALLOC_ARRAY(result, count + 1);
-   COPY_ARRAY(result, pathspec, count);
-   result[count] = NULL;
+
+   /* Create an intermediate copy of the pathspec based on the flags */
for (i = 0; i < count; i++) {
-   int length = strlen(result[i]);
+   int length = strlen(pathspec[i]);
int to_copy = length;
+   char *it;
while (!(flags & KEEP_TRAILING_SLASH) &&
-  to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
+  to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
to_copy--;
-   if (to_copy != length || flags & DUP_BASENAME) {
-   char *it = xmemdupz(result[i], to_copy);
-   if (flags & DUP_BASENAME) {
-   result[i] = xstrdup(basename(it));
-   free(it);
-   } else
-   result[i] = it;
+
+   it = xmemdupz(pathspec[i], to_copy);
+   if (flags & DUP_BASENAME) {
+   result[i] = xstrdup(basename(it));
+   free(it);
+   } else {
+   result[i] = it;
}
}
-   return get_pathspec(prefix, result);
+   result[count] = NULL;
+
+   /* Prefix the pathspec and free the old intermediate strings */
+   for (i = 0; i < count; i++) {
+   const char *match = prefix_path(prefix, prefixlen, result[i]);
+   free((char *) result[i]);
+   result[i] = match;
+   }
+
+   return result;
 }
 
 static const char *add_slash(const char *path)
@@ -130,7 +142,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
if (read_cache() < 0)
die(_("index file corrupt"));
 
-   source = internal_copy_pathspec(prefix, argv, argc, 0);
+   source = internal_prefix_pathspec(prefix, argv, argc, 0);
modes = xcalloc(argc, sizeof(enum update_mode));
/*
 * Keep trailing slash, needed to let
@@ -140,16 +152,16 @@ int cmd_mv(int argc, const char **argv, const char 
*prefix)
flags = KEEP_TRAILING_SLASH;
if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
flags = 0;
-   dest_path = internal_copy_pathspec(prefix, argv + argc, 1, flags);
+   dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
submodule_gitfile = xcalloc(argc, sizeof(char *));
 
if (dest_path[0][0] == '\0')
/* special case: "." was normalized to "" */
-   destination = internal_copy_pathspec(dest_path[0], argv, argc, 
DUP_BASENAME);
+   destination = internal_prefix_pathspec(dest_path[0], argv, 
argc, DUP_BASENAME);
else if (!lstat(dest_path[0], ) &&
S_ISDIR(st.st_mode)) {
dest_path[0] = add_slash(dest_path[0]);
-   destination = internal_copy_pathspec(dest_path[0], argv, argc, 
DUP_BASENAME);
+   destination = internal_prefix_pathspec(dest_path[0], argv, 
argc, DUP_BASENAME);
} else {
if (argc != 1)
die(_("destination '%s' is not a directory"), 

[PATCH v4 13/16] pathspec: create parse_element_magic helper

[Brandon Williams]

Factor out the logic responsible for the magic in a pathspec element
into its own function.

Also avoid calling into the parsing functions when
`PATHSPEC_LITERAL_PATH` is specified since it causes magic to be
ignored and all paths to be treated as literals.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 37 -
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index f1439f6f9..fe811a0a4 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -245,6 +245,19 @@ static const char *parse_short_magic(unsigned *magic, 
const char *elem)
return pos;
 }
 
+static const char *parse_element_magic(unsigned *magic, int *prefix_len,
+  const char *elem)
+{
+   if (elem[0] != ':' || get_literal_global())
+   return elem; /* nothing to do */
+   else if (elem[1] == '(')
+   /* longhand */
+   return parse_long_magic(magic, prefix_len, elem);
+   else
+   /* shorthand */
+   return parse_short_magic(magic, elem);
+}
+
 /*
  * Take an element of a pathspec and check for magic signatures.
  * Append the result to the prefix. Return the magic bitmap.
@@ -267,26 +280,16 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item, unsigned flags,
char *match;
int i, pathspec_prefix = -1;
 
-   if (elt[0] != ':' || get_literal_global() ||
-   (flags & PATHSPEC_LITERAL_PATH)) {
-   ; /* nothing to do */
-   } else if (elt[1] == '(') {
-   /* longhand */
-   copyfrom = parse_long_magic(_magic,
-   _prefix,
-   elt);
-   } else {
-   /* shorthand */
-   copyfrom = parse_short_magic(_magic, elt);
-   }
-
-   magic |= element_magic;
-
/* PATHSPEC_LITERAL_PATH ignores magic */
-   if (flags & PATHSPEC_LITERAL_PATH)
+   if (flags & PATHSPEC_LITERAL_PATH) {
magic = PATHSPEC_LITERAL;
-   else
+   } else {
+   copyfrom = parse_element_magic(_magic,
+  _prefix,
+  elt);
+   magic |= element_magic;
magic |= get_global_magic(element_magic);
+   }
 
if (pathspec_prefix >= 0 &&
(prefixlen || (prefix && *prefix)))
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v4 11/16] pathspec: create parse_short_magic function

[Brandon Williams]

Factor out the logic responsible for parsing short magic into its own
function.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 54 --
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index f760f44f9..8574010d5 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -157,6 +157,41 @@ static int get_global_magic(int element_magic)
 }
 
 /*
+ * Parse the pathspec element looking for short magic
+ *
+ * saves all magic in 'magic'
+ * returns the position in 'elem' after all magic has been parsed
+ */
+static const char *parse_short_magic(unsigned *magic, const char *elem)
+{
+   const char *pos;
+
+   for (pos = elem + 1; *pos && *pos != ':'; pos++) {
+   char ch = *pos;
+   int i;
+
+   if (!is_pathspec_magic(ch))
+   break;
+
+   for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+   if (pathspec_magic[i].mnemonic == ch) {
+   *magic |= pathspec_magic[i].bit;
+   break;
+   }
+   }
+
+   if (ARRAY_SIZE(pathspec_magic) <= i)
+   die(_("Unimplemented pathspec magic '%c' in '%s'"),
+   ch, elem);
+   }
+
+   if (*pos == ':')
+   pos++;
+
+   return pos;
+}
+
+/*
  * Take an element of a pathspec and check for magic signatures.
  * Append the result to the prefix. Return the magic bitmap.
  *
@@ -220,24 +255,7 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item, unsigned flags,
copyfrom++;
} else {
/* shorthand */
-   for (copyfrom = elt + 1;
-*copyfrom && *copyfrom != ':';
-copyfrom++) {
-   char ch = *copyfrom;
-
-   if (!is_pathspec_magic(ch))
-   break;
-   for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
-   if (pathspec_magic[i].mnemonic == ch) {
-   element_magic |= pathspec_magic[i].bit;
-   break;
-   }
-   if (ARRAY_SIZE(pathspec_magic) <= i)
-   die(_("Unimplemented pathspec magic '%c' in 
'%s'"),
-   ch, elt);
-   }
-   if (*copyfrom == ':')
-   copyfrom++;
+   copyfrom = parse_short_magic(_magic, elt);
}
 
magic |= element_magic;
-- 
2.11.0.390.gc69c2f50cf-goog

Re: [PATCH v2 2/2] t9813: avoid using pipes

[Pranit Bauva]

Hey Stefan,

On Tue, Jan 3, 2017 at 11:28 PM, Stefan Beller  wrote:
> On Mon, Jan 2, 2017 at 10:45 AM, Pranit Bauva  wrote:
>> The exit code of the upstream in a pipe is ignored thus we should avoid
>> using it.
>
> for commands under test, i.e. git things. Other parts can be piped if that 
> makes
> the test easier. Though I guess that can be guessed by the reader as well,
> as you only convert git commands on upstream pipes.
>
>> By writing out the output of the git command to a file, we can
>> test the exit codes of both the commands.
>>
>> Signed-off-by: Pranit Bauva 
>
> Thanks for taking ownership of this issue as well. :)

Welcome! ;)

>> ---
>>  t/t9813-git-p4-preserve-users.sh | 8 
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/t/t9813-git-p4-preserve-users.sh 
>> b/t/t9813-git-p4-preserve-users.sh
>> index 798bf2b67..9d7550ff3 100755
>> --- a/t/t9813-git-p4-preserve-users.sh
>> +++ b/t/t9813-git-p4-preserve-users.sh
>> @@ -118,12 +118,12 @@ test_expect_success 'not preserving user with mixed 
>> authorship' '
>> make_change_by_user usernamefile3 Derek de...@example.com &&
>> P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
>> export P4EDITOR P4USER P4PASSWD &&
>> -   git p4 commit |\
>> -   grep "git author de...@example.com does not match" &&
>> +   git p4 commit >actual 2>&1 &&
>
> Why do we need to pipe 2>&1 here?
> Originally the piping only fed the stdout to grep, so this patch changes the
> test? Maybe
>
> 2>actual.err &&
> test_must_be_empty actual.err
>
> instead?

I tried this out but it seems that travis-ci build fails[1]. And I
don't have p4 on my machine to test what's happening actually. But I
just pushed out a few thing modifications to travis and it seems that
actual.err isn't really empty for some reason. So I think, I just
leave it as,

git p4 commit >actual &&
grep "git author de...@example.com does not match" actual &&

What do you think?

[1]: https://travis-ci.org/pranitbauva1997/git/jobs/188633734

Regards,
Pranit Bauva

Re: [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()

[Jeff King]

On Tue, Jan 03, 2017 at 12:11:25PM -0800, Stefan Beller wrote:

> On Mon, Jan 2, 2017 at 8:22 AM, Johannes Schindelin
>  wrote:
> > Technically, it is correct that git_exec_path() returns a possibly
> > malloc()ed string. Practically, it is *sometimes* not malloc()ed. So
> > let's just use a static variable to make it a singleton. That'll shut
> > Coverity up, hopefully.
> 
> I picked up this patch and applied it to the coverity branch
> that I maintain at github/stefanbeller/git.
> 
> I'd love to see this patch upstream as it reduces my maintenance
> burden of the coverity branch by a patch.

There is another lurking issue in that function, which is that the
return value of getenv() is not guaranteed to last beyond more calls to
getenv() or setenv(). It should probably xstrdup() that result, too.

At that point 2 out of 3 of the return cases would be malloc'd strings,
so we _could_ switch the third and say "caller must free the result".
But I think I prefer something like Dscho's solution (more on that
below).

> Early on when Git was new to coverity, some arguments were made
> that patches like these only clutter the main code base which is read
> by a lot of people, hence we want these quirks for coverity not upstream.
> And I think that still holds.
> 
> If this patch is only to appease coverity (as the commit message eludes
> to) I think this may be a bad idea for upstream.  If this patch fixes an
> actual problem, then the commit message needs to spell that out.

This is a real leak, though in all cases the program typically exits
soon afterwards. But we leak from list_commands(), for example, and it
is not immediately obvious that this is only called right before
exiting.

But I think more important is that caching the result in a static
variable communicates something (both to Coverity and to a reader of the
code). This is a value we expect to live through the life of the
program, and it is OK for it to "leak" when it goes out of scope by the
program exiting.

So even though the behavior does not really change, the annotation has
value.

-Peff

Re: [PATCH] pathspec: give better message for submodule related pathspec error

[Jeff King]

On Tue, Jan 03, 2017 at 10:15:38AM -0800, Stefan Beller wrote:

> > I take that the new "BUG" thing tells the Git developers that no
> > sane codepath should throw an pathspec_item that satisfies the
> > condition of the if() statement for non-submodules?
> 
> If we want to keep the semantics of the assert around, then we
> have to have a blank statement if the submodule error message
> is not triggered.
> 
> I assume if we print this BUG, then there is an actual bug.

Right. I think this isn't a new "BUG", but rather a loosening of an
existing one. IOW, two things are going on here:

  1. Switch assert() to die("BUG") to give a more readable message.

  2. Take one of the cases where we hit a BUG and turn it into a normal
 "there was something wrong with the input" message.

If I understand the submodule case correctly, then (2) is reasonable.
The user gave a bogus pathspec that crosses the submodule boundary.

I've no idea if there are other cases that could ever hit the remaining
BUG, but it seems like a good defensive measure to leave it in. If
somebody hits it, then we can investigate their case.

-Peff

Re: [PATCH] archive-zip: load userdiff config

[Jeff King]

On Tue, Jan 03, 2017 at 06:24:39PM +0100, René Scharfe wrote:

> Am 02.01.2017 um 23:25 schrieb Jeff King:
> > Since 4aff646d17 (archive-zip: mark text files in archives,
> > 2015-03-05), the zip archiver will look at the userdiff
> > driver to decide whether a file is text or binary. This
> > usually doesn't need to look any further than the attributes
> > themselves (e.g., "-diff", etc). But if the user defines a
> > custom driver like "diff=foo", we need to look at
> > "diff.foo.binary" in the config. Prior to this patch, we
> > didn't actually load it.
> 
> Ah, didn't think of that, obviously.
> 
> Would it make sense for userdiff_find_by_path() to die if userdiff_config()
> hasn't been called, yet?

Yeah, perhaps. That makes it impossible for a program to intentionally
ignore the config. But it looks like even plumbing diff commands load
userdiff (which makes sense; they control its behavior through things
like ALLOW_TEXTCONV). So it's probably fine to have it everywhere.

Other options include:

  1. Just loading it always as part of git_default_config.

  2. Lazy-loading it on the first call. This seems elegant, though it
 does open up hidden cache-invalidation issues. E.g., somebody asks
 for userdiff_find_by_path(), we load the values, then they
 setup_git_repository(), and we would need to reload. That's
 far-fetched for userdiff, but it makes lazy-loading as a general
 pattern a bit of a potential maintenance trap.

 We could also introduce some infrastructure to deal with that
 (e.g., if callers could ask the config machinery "have you been
 invalidated"). That would help here and in other places (e.g., I
 considered this when dealing with get_shared_repository()).

> > I also happened to notice that zipfiles are created using the local
> > timezone (because they have no notion of the timezone, so we have to
> > pick _something_). That's probably the least-terrible option, but it was
> > certainly surprising to me when I tried to bit-for-bit reproduce a
> > zipfile from GitHub on my local machine.
> 
> That reminds me of an old request to allow users better control over the
> meta-data written into archives.  Being able to specify a time zone offset
> could be a start.

I did it with:

  TZ=PST8PDT git archive ...

which let me get a bit-for-bit match with what GitHub generates. The
real problem was just knowing that I needed to do that. OTOH, we're
considering having GitHub generate all archives in UTC for sanity's
sake, and it would be nice to do that by setting zip.timezone instead of
hacking $TZ for each invocation.

> > +static int archive_zip_config(const char *var, const char *value, void 
> > *data)
> > +{
> > +   return userdiff_config(var, value);
> > +}
> > +
> >  static int write_zip_archive(const struct archiver *ar,
> >  struct archiver_args *args)
> >  {
> > int err;
> > 
> > +   git_config(archive_zip_config, NULL);
> > +
> 
> I briefly thought about moving this call to archive.c with the rest of the
> config-related stuff, but I agree it's better kept here.

That was my first thought, but there are already two config calls:
write_archive() loads default config, but then archive-tar loads
tar-specific config. Since only zip cares about userdiff, I patterned it
after the latter. But arguably everybody _could_ end up calling into
userdiff. If we take that philosophy, though, I'd be more inclined to
push it into git_default_config(). That covers archive writers _and_ any
other programs which might happen to call into the diff code.

> Looks good, thanks!

Thanks for reviewing.

-Peff

Re: [PATCH v4 4/5] real_path: have callers use real_pathdup and strbuf_realpath

[Jacob Keller]

On Tue, Jan 3, 2017 at 11:09 AM, Brandon Williams  wrote:
> Migrate callers of real_path() who duplicate the retern value to use
> real_pathdup or strbuf_realpath.

Nit: s/retern/return

Re: builtin difftool parsing issue

[Jacob Keller]

On Tue, Jan 3, 2017 at 10:47 AM, Stefan Beller  wrote:
> On Mon, Jan 2, 2017 at 11:05 AM, Paul Sbarra  wrote:
>>> I would have expected `git difftool --submodule=diff ...` to work... What
>>> are the problems?
>>
>> The docs for difftool state...
>> "git difftool is a frontend to git diff and accepts the same options
>> and arguments."
>
> I think such a sentence in the man page is dangerous, as nobody
> was caught this issue until now. There have been numerous authors
> and reviewers that touched "diff --submodule=, but as there
> is no back-reference, hinting that the patch is only half done, and the
> difftool also needs to implement such an option.
>
> We should reword the man page either as
>
>   "git difftool is a frontend to git diff and accepts the most(?) options
>   and arguments."
>
> or even be explicit and list the arguments instead. There we could also
> describe differences if any (e.g. the formats available might be different
> for --submodule=)

I agree with updating the documentation. Difftool would require
extensive work to support the --submodule format, and I'm not sure
it's worth it.

Thanks,
Jake

Re: [PATCH v4 0/5] road to reentrant real_path

[Jeff King]

On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:

> Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
> MAXDEPTH due to a naming conflict brought up by Lars Schneider.

Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's
what all other similar functions will be using.

The only problem was that we were redefining the macro. So maybe:

  #ifndef MAXSYMLINKS
  #define MAXSYMLINKS 5
  #endif

would be a good solution?

It looks like the "usual" value is more like 20 or 30 on most systems,
though.  We should probably also set errno to ELOOP when we hit the
limit, which is what other symlink-resolving functions would do.

I'm surprised we didn't hit this on Linux, which has MAXSYMLINKS, too.
We should be picking it up from .

-Peff

[PATCH 1/4] t7610: update branch names to match test number

[Richard Hansen]

Rename the testNN branches so that NN matches the test number.  This
should make it easier to troubleshoot test issues.  Use $test_count to
keep this future-proof.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 82 ++--
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 6d9f21511..14090739f 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -94,7 +94,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'custom mergetool' '
-   git checkout -b test1 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -113,7 +113,7 @@ test_expect_success 'custom mergetool' '
 
 test_expect_success 'mergetool crlf' '
test_config core.autocrlf true &&
-   git checkout -b test2 branch1 &&
+   git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
@@ -134,7 +134,7 @@ test_expect_success 'mergetool crlf' '
 '
 
 test_expect_success 'mergetool in subdir' '
-   git checkout -b test3 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
cd subdir &&
@@ -161,7 +161,7 @@ test_expect_success 'mergetool on file in parent dir' '
 '
 
 test_expect_success 'mergetool skips autoresolved' '
-   git checkout -b test4 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
@@ -192,7 +192,7 @@ test_expect_success 'mergetool merges all from subdir' '
 test_expect_success 'mergetool skips resolved paths when rerere is active' '
test_config rerere.enabled true &&
rm -rf .git/rr-cache &&
-   git checkout -b test5 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "l" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&
@@ -233,7 +233,7 @@ test_expect_success 'conflicted stash sets up rerere'  '
 test_expect_success 'mergetool takes partial path' '
git reset --hard &&
test_config rerere.enabled false &&
-   git checkout -b test12 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
 
@@ -308,12 +308,12 @@ test_expect_success 'mergetool keeps tempfiles when 
aborting delete/delete' '
 '
 
 test_expect_success 'deleted vs modified submodule' '
-   git checkout -b test6 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
git rm --cached submod &&
git commit -m "Submodule deleted from branch" &&
-   git checkout -b test6.a test6 &&
+   git checkout -b test$test_count.a test$test_count &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 
>/dev/null 2>&1 ) &&
@@ -329,7 +329,7 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by keeping module" &&
 
mv submod submod-movedaside &&
-   git checkout -b test6.b test6 &&
+   git checkout -b test$test_count.b test$test_count &&
git submodule update -N &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
@@ -343,9 +343,9 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by deleting module" &&
 
mv submod-movedaside submod &&
-   git checkout -b test6.c master &&
+   git checkout -b test$test_count.c master &&
git submodule update -N &&
-   test_must_fail git merge test6 &&
+   test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 
>/dev/null 2>&1 ) &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -359,9 +359,9 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by deleting module" &&
mv submod.orig submod &&
 
-   git checkout -b test6.d master &&
+   git checkout -b test$test_count.d master &&
git submodule update -N &&
-   test_must_fail git merge test6 &&
+   test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 
>/dev/null 2>&1 ) &&
( yes "" | git mergetool both >/dev/null 

[PATCH 4/4] mergetool: fix running in subdir when rerere enabled

[Richard Hansen]

If rerere is enabled and no pathnames are given, run cd_to_toplevel
before running 'git diff --name-only' so that 'git diff --name-only'
sees the pathnames emitted by 'git rerere remaining'.

Also run cd_to_toplevel before running 'git rerere remaining' in case
'git rerere remaining' is ever changed to print pathnames relative to
the current directory rather than to $GIT_WORK_TREE.

This fixes a regression introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).

Signed-off-by: Richard Hansen 
---
 git-mergetool.sh | 1 +
 t/t7610-mergetool.sh | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index e52b4e4f2..67ea0d6db 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -456,6 +456,7 @@ main () {
 
if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
then
+   cd_to_toplevel
set -- $(git rerere remaining)
if test $# -eq 0
then
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index e7b3e1866..1f7d63acb 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -200,7 +200,7 @@ test_expect_success 'mergetool merges all from subdir 
(rerere disabled)' '
)
 '
 
-test_expect_failure 'mergetool merges all from subdir (rerere enabled)' '
+test_expect_success 'mergetool merges all from subdir (rerere enabled)' '
git reset --hard &&
git checkout -b test$test_count branch1 &&
test_config rerere.enabled true &&
-- 
2.11.0.390.gc69c2f50cf-goog



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH 3/4] t7610: add test case for rerere+mergetool+subdir bug

[Richard Hansen]

If rerere is enabled and mergetool is run from a subdirectory,
mergetool always prints "No files need merging".  Add an expected
failure test case for this situation.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 8e2b4e147..e7b3e1866 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -182,7 +182,7 @@ test_expect_success 'mergetool skips autoresolved' '
git reset --hard
 '
 
-test_expect_success 'mergetool merges all from subdir' '
+test_expect_success 'mergetool merges all from subdir (rerere disabled)' '
git reset --hard &&
git checkout -b test$test_count branch1 &&
test_config rerere.enabled false &&
@@ -200,6 +200,25 @@ test_expect_success 'mergetool merges all from subdir' '
)
 '
 
+test_expect_failure 'mergetool merges all from subdir (rerere enabled)' '
+   git reset --hard &&
+   git checkout -b test$test_count branch1 &&
+   test_config rerere.enabled true &&
+   rm -rf .git/rr-cache &&
+   (
+   cd subdir &&
+   test_must_fail git merge master &&
+   ( yes "r" | git mergetool ../submod ) &&
+   ( yes "d" "d" | git mergetool --no-prompt ) &&
+   test "$(cat ../file1)" = "master updated" &&
+   test "$(cat ../file2)" = "master new" &&
+   test "$(cat file3)" = "master new sub" &&
+   ( cd .. && git submodule update -N ) &&
+   test "$(cat ../submod/bar)" = "master submodule" &&
+   git commit -m "branch2 resolved by mergetool from subdir"
+   )
+'
+
 test_expect_success 'mergetool skips resolved paths when rerere is active' '
git reset --hard &&
test_config rerere.enabled true &&
-- 
2.11.0.390.gc69c2f50cf-goog



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH 2/4] t7610: make tests more independent and debuggable

[Richard Hansen]

If a test fails it might leave the repository in a strange state.  Add
'git reset --hard' at the beginning of each test to increase the odds
of passing when an earlier test fails.

Also use test-specific branches to avoid interfering with later tests
and to make the tests easier to debug.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 14090739f..8e2b4e147 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -94,6 +94,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'custom mergetool' '
+   git reset --hard &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -112,6 +113,7 @@ test_expect_success 'custom mergetool' '
 '
 
 test_expect_success 'mergetool crlf' '
+   git reset --hard &&
test_config core.autocrlf true &&
git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -134,6 +136,7 @@ test_expect_success 'mergetool crlf' '
 '
 
 test_expect_success 'mergetool in subdir' '
+   git reset --hard &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
@@ -145,8 +148,13 @@ test_expect_success 'mergetool in subdir' '
 '
 
 test_expect_success 'mergetool on file in parent dir' '
+   git reset --hard &&
+   git checkout -b test$test_count branch1 &&
+   git submodule update -N &&
(
cd subdir &&
+   test_must_fail git merge master >/dev/null 2>&1 &&
+   ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 
2>&1 ) &&
( yes "" | git mergetool ../both >/dev/null 2>&1 ) &&
@@ -161,6 +169,7 @@ test_expect_success 'mergetool on file in parent dir' '
 '
 
 test_expect_success 'mergetool skips autoresolved' '
+   git reset --hard &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
@@ -174,6 +183,8 @@ test_expect_success 'mergetool skips autoresolved' '
 '
 
 test_expect_success 'mergetool merges all from subdir' '
+   git reset --hard &&
+   git checkout -b test$test_count branch1 &&
test_config rerere.enabled false &&
(
cd subdir &&
@@ -190,6 +201,7 @@ test_expect_success 'mergetool merges all from subdir' '
 '
 
 test_expect_success 'mergetool skips resolved paths when rerere is active' '
+   git reset --hard &&
test_config rerere.enabled true &&
rm -rf .git/rr-cache &&
git checkout -b test$test_count branch1 &&
@@ -204,6 +216,7 @@ test_expect_success 'mergetool skips resolved paths when 
rerere is active' '
 '
 
 test_expect_success 'conflicted stash sets up rerere'  '
+   git reset --hard &&
test_config rerere.enabled true &&
git checkout stash1 &&
echo "Conflicting stash content" >file11 &&
@@ -244,6 +257,7 @@ test_expect_success 'mergetool takes partial path' '
 '
 
 test_expect_success 'mergetool delete/delete conflict' '
+   git reset --hard &&
git checkout -b delete-base branch1 &&
mkdir -p a/a &&
(echo one; echo two; echo 3; echo 4) >a/a/file.txt &&
@@ -274,6 +288,7 @@ test_expect_success 'mergetool delete/delete conflict' '
 '
 
 test_expect_success 'mergetool produces no errors when keepBackup is used' '
+   git reset --hard &&
test_config mergetool.keepBackup true &&
test_must_fail git merge move-to-b &&
: >expect &&
@@ -284,6 +299,7 @@ test_expect_success 'mergetool produces no errors when 
keepBackup is used' '
 '
 
 test_expect_success 'mergetool honors tempfile config for deleted files' '
+   git reset --hard &&
test_config mergetool.keepTemporaries false &&
test_must_fail git merge move-to-b &&
echo d | git mergetool a/a/file.txt &&
@@ -292,6 +308,7 @@ test_expect_success 'mergetool honors tempfile config for 
deleted files' '
 '
 
 test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
+   git reset --hard &&
test_config mergetool.keepTemporaries true &&
test_must_fail git merge move-to-b &&
! (echo a; echo n) | git mergetool a/a/file.txt &&
@@ -308,6 +325,7 @@ test_expect_success 'mergetool keeps tempfiles when 
aborting delete/delete' '
 '
 
 test_expect_success 'deleted vs modified submodule' '
+   git reset --hard &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
@@ -377,6 +395,7 @@ test_expect_success 'deleted vs modified submodule' '
 '
 
 test_expect_success 'file vs modified submodule' '
+ 

[PATCH 0/4] fix mergetool+rerere+subdir regression

[Richard Hansen]

If rerere is enabled, no pathnames are given, and mergetool is run
from a subdirectory, mergetool always prints "No files need merging".
Fix the bug.

This regression was introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).

Richard Hansen (4):
  t7610: update branch names to match test number
  t7610: make tests more independent and debuggable
  t7610: add test case for rerere+mergetool+subdir bug
  mergetool: fix running in subdir when rerere enabled

 git-mergetool.sh |   1 +
 t/t7610-mergetool.sh | 132 ++-
 2 files changed, 90 insertions(+), 43 deletions(-)

-- 
2.11.0.390.gc69c2f50cf-goog



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCHv2] submodule.c: use GIT_DIR_ENVIRONMENT consistently

[Jeff King]

On Tue, Jan 03, 2017 at 10:30:47AM -0800, Stefan Beller wrote:

> In C code we have the luxury of having constants for all the important
> things that are hard coded. This is the only place in C, that hard codes
> the git directory environment variable, so fix it.
> 
> Signed-off-by: Stefan Beller 

This looks like a good change to me.

Minor nit: the comma after "C" in your commit message is extraneous. ;)

-Peff

[PATCHv4 0/2] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

> It MIGHT be a handy hack when writing a test, but let's stop doing
> that insanity.  No sane project does that in real life, doesn't it?

> Create a subdirectory, make it a repository, have a commit there and
> bind that as our own submodule.  That would be a more normal way to
> start your own superproject and its submodule pair if they originate
> together at the same place.

This comes as an extra patch before the actual fix.

The actual fixing patch was reworded borrowing some words from Jeff.

As this makes use of "test_commit -C", it goes on top of 
sb/submodule-embed-gitdir

Thanks,
Stefan


Stefan Beller (2):
  submodule tests: don't use itself as a submodule
  pathspec: give better message for submodule related pathspec error

 pathspec.c   | 24 ++--
 t/lib-submodule-update.sh|  2 ++
 t/t6134-pathspec-in-submodule.sh | 33 +
 t/t7001-mv.sh|  5 +++--
 t/t7507-commit-verbose.sh|  4 +++-
 t/t7800-difftool.sh  |  4 +++-
 t/test-lib-functions.sh  | 16 
 7 files changed, 82 insertions(+), 6 deletions(-)
 create mode 100755 t/t6134-pathspec-in-submodule.sh

-- 
2.11.0.rc2.31.g2cc886f

[PATCH 1/2] submodule tests: don't use itself as a submodule

[Stefan Beller]

In reality nobody would run "git submodule add ./. "
to add the repository to itself as a submodule as this comes with some
nasty surprises, such as infinite recursion when cloning that repository.
However we do this all the time in the test suite, because most of the
time this was the most convenient way to test a very specific thing
for submodule behavior.

This provides an easier way to have submodules in tests, by just setting
TEST_CREATE_SUBMODULE to a non empty string, similar to
TEST_NO_CREATE_REPO.

Make use of it in those tests that add a submodule from ./. except for
the occurrence in create_lib_submodule_repo as there it seems we craft
a repository deliberately for both inside as well as outside use.

The name "pretzel.[non]bare" was chosen deliberate to not introduce
more strings to the test suite containing "sub[module]" as searching for
"sub" already yields a lot of hits from different contexts. "pretzel"
doesn't occur in the test suite yet, so it is a good candidate for
a potential remote for a submodule.

Signed-off-by: Stefan Beller 
---
 t/lib-submodule-update.sh |  2 ++
 t/t7001-mv.sh |  5 +++--
 t/t7507-commit-verbose.sh |  4 +++-
 t/t7800-difftool.sh   |  4 +++-
 t/test-lib-functions.sh   | 16 
 5 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 79cdd34a54..58d76d9df8 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -44,6 +44,8 @@ create_lib_submodule_repo () {
git branch "no_submodule" &&
 
git checkout -b "add_sub1" &&
+   # Adding the repo itself as a submodule is a hack.
+   # Do not imitate this.
git submodule add ./. sub1 &&
git config -f .gitmodules submodule.sub1.ignore all &&
git config submodule.sub1.ignore all &&
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index e365d1ff77..6cb32f3a3a 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 
 test_description='git mv in subdirs'
+TEST_CREATE_SUBMODULE=yes
 . ./test-lib.sh
 
 test_expect_success \
@@ -288,12 +289,12 @@ rm -f moved symlink
 test_expect_success 'setup submodule' '
git commit -m initial &&
git reset --hard &&
-   git submodule add ./. sub &&
+   git submodule add ./pretzel.bare sub &&
echo content >file &&
git add file &&
git commit -m "added sub and file" &&
mkdir -p deep/directory/hierarchy &&
-   git submodule add ./. deep/directory/hierarchy/sub &&
+   git submodule add ./pretzel.bare deep/directory/hierarchy/sub &&
git commit -m "added another submodule" &&
git branch submodule
 '
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index ed2653d46f..d269900afa 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 
 test_description='verbose commit template'
+TEST_CREATE_SUBMODULE=yes
 . ./test-lib.sh
 
 write_script "check-for-diff" <<\EOF &&
@@ -74,11 +75,12 @@ test_expect_success 'diff in message is retained with -v' '
 
 test_expect_success 'submodule log is stripped out too with -v' '
git config diff.submodule log &&
-   git submodule add ./. sub &&
+   git submodule add ./pretzel.bare sub &&
git commit -m "sub added" &&
(
cd sub &&
echo "more" >>file &&
+   git add file &&
git commit -a -m "submodule commit"
) &&
(
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 70a2de461a..d13a5d0453 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -7,6 +7,7 @@ test_description='git-difftool
 
 Testing basic diff tool invocation
 '
+TEST_CREATE_SUBMODULE=Yes
 
 . ./test-lib.sh
 
@@ -534,7 +535,8 @@ test_expect_success PERL 'difftool --no-symlinks detects 
conflict ' '
 '
 
 test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
-   git submodule add ./. submod/ule &&
+   git submodule add ./pretzel.bare submod/ule &&
+   test_commit -C submod/ule second_commit &&
test_config -C submod/ule diff.tool checktrees &&
test_config -C submod/ule difftool.checktrees.cmd '\''
test -d "$LOCAL" && test -d "$REMOTE" && echo good
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 579e812506..aa327a7dff 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -800,6 +800,22 @@ test_create_repo () {
error "cannot run git init -- have you built things yet?"
mv .git/hooks .git/hooks-disabled
) || exit
+   if test -n "$TEST_CREATE_SUBMODULE"
+   then
+   (
+   cd "$repo"
+   TEST_CREATE_SUBMODULE=
+   export TEST_CREATE_SUBMODULE
+   test_create_repo 

[PATCH 2/2] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

Every once in a while someone complains to the mailing list to have
run into this weird assertion[1]. The usual response from the mailing
list is link to old discussions[2], and acknowledging the problem
stating it is known.

This patch accomplishes two things:

  1. Switch assert() to die("BUG") to give a more readable message.

  2. Take one of the cases where we hit a BUG and turn it into a normal
 "there was something wrong with the input" message.


[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] 
http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
https://www.spinics.net/lists/git/msg249473.html

Helped-by: Jeff King 
Helped-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
---
 pathspec.c   | 24 ++--
 t/t6134-pathspec-in-submodule.sh | 33 +
 2 files changed, 55 insertions(+), 2 deletions(-)
 create mode 100755 t/t6134-pathspec-in-submodule.sh

diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..574a0bb158 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -313,8 +313,28 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
 
/* sanity checks, pathspec matchers assume these are sane */
-   assert(item->nowildcard_len <= item->len &&
-  item->prefix <= item->len);
+   if (item->nowildcard_len > item->len ||
+   item->prefix > item->len) {
+   /* Historically this always was a submodule issue */
+   for (i = 0; i < active_nr; i++) {
+   struct cache_entry *ce = active_cache[i];
+   int len;
+
+   if (!S_ISGITLINK(ce->ce_mode))
+   continue;
+
+   len = ce_namelen(ce);
+   if (item->len < len)
+   len = item->len;
+
+   if (!memcmp(ce->name, item->match, len))
+   die (_("Pathspec '%s' is in submodule '%.*s'"),
+   item->original, ce_namelen(ce), 
ce->name);
+   }
+   /* The error is a new unknown bug */
+   die ("BUG: item->nowildcard_len > item->len || item->prefix > 
item->len)");
+   }
+
return magic;
 }
 
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
new file mode 100755
index 00..2900d8d06e
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+TEST_CREATE_SUBMODULE=yes
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+   git submodule add ./pretzel.bare sub &&
+   git commit -a -m "add submodule" &&
+   git submodule deinit --all
+'
+
+cat sub/a &&
+   test_must_fail git add sub/a 2>actual &&
+   test_cmp expect actual
+'
+
+cat actual &&
+   test_cmp expect actual
+'
+
+test_done
-- 
2.11.0.rc2.31.g2cc886f

Re: [PATCH v4 0/5] road to reentrant real_path

[Jeff King]

On Wed, Jan 04, 2017 at 07:56:02AM +0100, Torsten Bögershausen wrote:

> On 04.01.17 01:48, Jeff King wrote:
> > On Tue, Jan 03, 2017 at 11:09:18AM -0800, Brandon Williams wrote:
> > 
> >> Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to
> >> MAXDEPTH due to a naming conflict brought up by Lars Schneider.
> > 
> > Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's
> > what all other similar functions will be using.
> > 
> > The only problem was that we were redefining the macro. So maybe:
> > 
> >   #ifndef MAXSYMLINKS
> >   #define MAXSYMLINKS 5
> >   #endif
> > 
> > would be a good solution?
> Why 5  ? (looking at the  20..30 below)
> And why 5 on one system and e.g. on my Mac OS
> #define MAXSYMLINKS 32  

I mentioned "5" because that is the current value of MAXDEPTH. I do
think it would be reasonable to bump it to something higher.

> Would the same value value for all Git installations on all platforms make 
> sense?
> #define GITMAXSYMLINKS 20

I think it's probably more important to match the rest of the OS, so
that open("foo") and realpath("foo") behave similarly on the same
system. Though I think even that isn't always possible, as the limit is
dynamic on some systems.

I think the idea of the 20-30 range is that it's small enough to catch
an infinite loop quickly, and large enough that nobody will ever hit it
in practice. :)

-Peff

Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error

[Jeff King]

On Tue, Jan 03, 2017 at 05:48:35PM -0800, Stefan Beller wrote:

> Every once in a while someone complains to the mailing list to have
> run into this weird assertion[1]. The usual response from the mailing
> list is link to old discussions[2], and acknowledging the problem
> stating it is known.
> 
> This patch accomplishes two things:
> 
>   1. Switch assert() to die("BUG") to give a more readable message.
> 
>   2. Take one of the cases where we hit a BUG and turn it into a normal
>  "there was something wrong with the input" message.

As this last bit is quoted from me, I won't deny that it's brilliant as
usual.

But as this commit message needs to stand on its own, rather than as part of a
larger discussion thread, it might be worth expanding "one of the cases"
here. And talking about what's happening to the other cases.

Like:

  This assertion triggered for cases where there wasn't a programming
  bug, but just bogus input. In particular, if the user asks for a
  pathspec that is inside a submodule, we shouldn't assert() or
  die("BUG"); we should tell the user their request is bogus.

  We'll retain the assertion for non-submodule cases, though. We don't
  know of any cases that would trigger this, but it _would_ be
  indicative of a programming error, and we should catch it here.

or something. Writing the first paragraph made me wonder if a better
solution, though, would be to catch and complain about this case
earlier. IOW, this _is_ a programming bug, because we're violating some
assumption of the pathspec code. And whatever is putting that item into
the pathspec list is what should be fixed.

I haven't looked closely enough to have a real opinion on that, though.

> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..574a0bb158 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,28 @@ static unsigned prefix_pathspec(struct pathspec_item 
> *item,
>   }
>  
>   /* sanity checks, pathspec matchers assume these are sane */
> - assert(item->nowildcard_len <= item->len &&
> -item->prefix <= item->len);
> + if (item->nowildcard_len > item->len ||
> + item->prefix > item->len) {
> + /* Historically this always was a submodule issue */
> + for (i = 0; i < active_nr; i++) {
> + struct cache_entry *ce = active_cache[i];
> + int len;

Given the discussion, this comment seems funny now. Who cares about
"historically"? It should probably be something like:

  /*
   * This case can be triggered by the user pointing us to a pathspec
   * inside a submodule, which is an input error. Detect that here
   * and complain, but fallback in the non-submodule case to a BUG,
   * as we have no idea what would trigger that.
   */

-Peff

[PATCH v4 05/16] pathspec: remove the deprecated get_pathspec function

[Brandon Williams]

Now that all callers of the old 'get_pathspec' interface have been
migrated to use the new pathspec struct interface it can be removed
from the codebase.

Since there are no more users of the '_raw' field in the pathspec struct
it can also be removed.  This patch also removes the old functionality
of modifying the const char **argv array that was passed into
parse_pathspec.  Instead the constructed 'match' string (which is a
pathspec element with the prefix prepended) is only stored in its
corresponding pathspec_item entry.

Signed-off-by: Brandon Williams 
---
 Documentation/technical/api-setup.txt |  2 --
 cache.h   |  1 -
 pathspec.c| 42 +++
 pathspec.h|  1 -
 4 files changed, 3 insertions(+), 43 deletions(-)

diff --git a/Documentation/technical/api-setup.txt 
b/Documentation/technical/api-setup.txt
index 540e45568..eb1fa9853 100644
--- a/Documentation/technical/api-setup.txt
+++ b/Documentation/technical/api-setup.txt
@@ -27,8 +27,6 @@ parse_pathspec(). This function takes several arguments:
 
 - prefix and args come from cmd_* functions
 
-get_pathspec() is obsolete and should never be used in new code.
-
 parse_pathspec() helps catch unsupported features and reject them
 politely. At a lower level, different pathspec-related functions may
 not support the same set of features. Such pathspec-sensitive
diff --git a/cache.h b/cache.h
index a50a61a19..0f80e01bd 100644
--- a/cache.h
+++ b/cache.h
@@ -514,7 +514,6 @@ extern void set_git_work_tree(const char *tree);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
-extern const char **get_pathspec(const char *prefix, const char **pathspec);
 extern void setup_work_tree(void);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
diff --git a/pathspec.c b/pathspec.c
index 22ca74a12..1f918cbae 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -103,7 +103,7 @@ static void prefix_short_magic(struct strbuf *sb, int 
prefixlen,
  */
 static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned *p_short_magic,
-   const char **raw, unsigned flags,
+   unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
 {
@@ -240,7 +240,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
if (!match)
die(_("%s: '%s' is outside repository"), elt, copyfrom);
}
-   *raw = item->match = match;
+   item->match = match;
/*
 * Prefix the pathspec (keep all magic) and assign to
 * original. Useful for passing to another command.
@@ -381,8 +381,6 @@ void parse_pathspec(struct pathspec *pathspec,
 
/* No arguments with prefix -> prefix pathspec */
if (!entry) {
-   static const char *raw[2];
-
if (flags & PATHSPEC_PREFER_FULL)
return;
 
@@ -394,10 +392,7 @@ void parse_pathspec(struct pathspec *pathspec,
item->original = prefix;
item->nowildcard_len = item->len = strlen(prefix);
item->prefix = item->len;
-   raw[0] = prefix;
-   raw[1] = NULL;
pathspec->nr = 1;
-   pathspec->_raw = raw;
return;
}
 
@@ -415,7 +410,6 @@ void parse_pathspec(struct pathspec *pathspec,
pathspec->nr = n;
ALLOC_ARRAY(pathspec->items, n);
item = pathspec->items;
-   pathspec->_raw = argv;
prefixlen = prefix ? strlen(prefix) : 0;
 
for (i = 0; i < n; i++) {
@@ -423,7 +417,7 @@ void parse_pathspec(struct pathspec *pathspec,
entry = argv[i];
 
item[i].magic = prefix_pathspec(item + i, _magic,
-   argv + i, flags,
+   flags,
prefix, prefixlen, entry);
if ((flags & PATHSPEC_LITERAL_PATH) &&
!(magic_mask & PATHSPEC_LITERAL))
@@ -457,36 +451,6 @@ void parse_pathspec(struct pathspec *pathspec,
}
 }
 
-/*
- * N.B. get_pathspec() is deprecated in favor of the "struct pathspec"
- * based interface - see pathspec.c:parse_pathspec().
- *
- * Arguments:
- *  - prefix - a path relative to the root of the working tree
- *  - pathspec - a list of paths underneath the prefix path
- *
- * Iterates over pathspec, prepending each path with prefix,
- * and return the resulting list.
- *
- * If pathspec is empty, return a singleton list containing prefix.
- *
- * If pathspec and prefix are both empty, return an empty list.
- *
- * This is typically used by built-in commands such as add.c, in order
- * to normalize argv arguments provided 

[PATCH v4 16/16] pathspec: rename prefix_pathspec to init_pathspec_item

[Brandon Williams]

Give a more relevant name to the prefix_pathspec function as it does
more than just prefix a pathspec element.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 24 +++-
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index ae9e1401f..bcf3ba039 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -297,21 +297,11 @@ static void strip_submodule_slash_expensive(struct 
pathspec_item *item)
 }
 
 /*
- * Take an element of a pathspec and check for magic signatures.
- * Append the result to the prefix. Return the magic bitmap.
- *
- * For now, we only parse the syntax and throw out anything other than
- * "top" magic.
- *
- * NEEDSWORK: This needs to be rewritten when we start migrating
- * get_pathspec() users to use the "struct pathspec" interface.  For
- * example, a pathspec element may be marked as case-insensitive, but
- * the prefix part must always match literally, and a single stupid
- * string cannot express such a case.
+ * Perform the initialization of a pathspec_item based on a pathspec element.
  */
-static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
-   const char *prefix, int prefixlen,
-   const char *elt)
+static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
+  const char *prefix, int prefixlen,
+  const char *elt)
 {
unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
@@ -329,6 +319,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item, 
unsigned flags,
magic |= get_global_magic(element_magic);
}
 
+   item->magic = magic;
+
if (pathspec_prefix >= 0 &&
(prefixlen || (prefix && *prefix)))
die("BUG: 'prefix' magic is supposed to be used at worktree's 
root");
@@ -401,7 +393,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item, 
unsigned flags,
/* sanity checks, pathspec matchers assume these are sane */
assert(item->nowildcard_len <= item->len &&
   item->prefix <= item->len);
-   return magic;
 }
 
 static int pathspec_item_cmp(const void *a_, const void *b_)
@@ -501,8 +492,7 @@ void parse_pathspec(struct pathspec *pathspec,
for (i = 0; i < n; i++) {
entry = argv[i];
 
-   item[i].magic = prefix_pathspec(item + i, flags,
-   prefix, prefixlen, entry);
+   init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
 
if (item[i].magic & PATHSPEC_EXCLUDE)
nr_exclude++;
-- 
2.11.0.390.gc69c2f50cf-goog

Re: Wanted: shallow submodule clones with --no-single-branch.

[Stefan Beller]

On Fri, Dec 30, 2016 at 2:50 AM, Tor Andersson  wrote:
> Hi,
>
> When adding submodules with --depth=1 only the master branch is
> cloned. This often leaves the submodule pointing to a non-existing
> commit.

You can also use the "--branch=not_master" flag to track another branch.
This however doesn't clone the correct branch. I would have expected that
it cloned the correct branch instead.

>
> It would be useful if I could pass the --no-single-branch argument to
> the submodule clone process, since then a submodule can point to any
> tag or branch without ending up in this situation.


Adding --no-single-branch sounds like a good idea for general use,
but it seems like a clunky workaround when looking for a specific branch,
i.e. fixing the --branch flag sounds like the right approach for this
issue here?

> I've got a local
> patch to hardwire the --no-single-branch argument in the
> builtin/submodule--helper.c clone_submodule function, but I'm not sure
> if this will have any other adverse effects?

Well, when asking for depth=1, the user usually actually wants to have the
least amount of data, which is why --depth implies --single-branch
in clone.

So adding it unconditionally is a bad idea IMHO, we'd need to have a flag
for that propagated from git-submodule.sh (function cmd_add) to the
submodule--helpers module_clone.

>
> Better yet would be for the shallow submodule clone to automatically
> retrieve and graft the actual commit the submodule points to, but
> that's probably wishing for too much.

I think fixing the branch option comes a bit closer, but still doesn't
fix this root problem. "submodule update" tries to fetch by sha1
directly in the hope that the server has uploadpack.\
allowReachableSHA1InWant configured.

In "submodule add" I think it is sufficient to take the current remotes
$branch and record that, so I do not see the need in the add code
to support direct sha1s unlike the update command?

Thanks,
Stefan

Re: [PATCH v2] git-p4: do not pass '-r 0' to p4 commands

[Ori Rawlings]

Looks good to me.


Ori Rawlings

Re: [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()

[Stefan Beller]

On Mon, Jan 2, 2017 at 8:22 AM, Johannes Schindelin
 wrote:
> Technically, it is correct that git_exec_path() returns a possibly
> malloc()ed string. Practically, it is *sometimes* not malloc()ed. So
> let's just use a static variable to make it a singleton. That'll shut
> Coverity up, hopefully.

I picked up this patch and applied it to the coverity branch
that I maintain at github/stefanbeller/git.

I'd love to see this patch upstream as it reduces my maintenance
burden of the coverity branch by a patch.

Early on when Git was new to coverity, some arguments were made
that patches like these only clutter the main code base which is read
by a lot of people, hence we want these quirks for coverity not upstream.
And I think that still holds.

If this patch is only to appease coverity (as the commit message eludes
to) I think this may be a bad idea for upstream.  If this patch fixes an
actual problem, then the commit message needs to spell that out.

Thanks,
Stefan

Re: [PATCH v2 2/2] t9813: avoid using pipes

[Stefan Beller]

>
> git p4 commit >actual &&
> grep "git author de...@example.com does not match" actual &&
>
> What do you think?

>From the travis logs:

'actual.err' is not empty, it contains:
... - file(s) up-to-date.

I think(/hope) such a progress is tested for at another test,
and not relevant here so I'd think the proposed

git p4 commit >actual &&
grep "git author de...@example.com does not match" actual &&

is fine here.

Thanks,
Stefan

[PATCH v3 1/2] don't use test_must_fail with grep

[Pranit Bauva]

test_must_fail should only be used for testing git commands. To test the
failure of other commands use `!`.

Reported-by: Stefan Beller 
Signed-off-by: Pranit Bauva 
---
 t/t3510-cherry-pick-sequence.sh  |  6 +++---
 t/t5504-fetch-receive-strict.sh  |  2 +-
 t/t5516-fetch-push.sh|  2 +-
 t/t5601-clone.sh |  2 +-
 t/t6030-bisect-porcelain.sh  |  2 +-
 t/t7610-mergetool.sh |  2 +-
 t/t9001-send-email.sh|  2 +-
 t/t9117-git-svn-init-clone.sh| 12 ++--
 t/t9813-git-p4-preserve-users.sh |  8 
 t/t9814-git-p4-rename.sh |  6 +++---
 10 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 372307c21..0acf4b146 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -385,7 +385,7 @@ test_expect_success '--continue respects opts' '
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
-   test_must_fail grep "cherry picked from" initial_msg &&
+   ! grep "cherry picked from" initial_msg &&
grep "cherry picked from" unrelatedpick_msg &&
grep "cherry picked from" picked_msg &&
grep "cherry picked from" anotherpick_msg
@@ -426,9 +426,9 @@ test_expect_failure '--signoff is automatically propagated 
to resolved conflict'
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
-   test_must_fail grep "Signed-off-by:" initial_msg &&
+   ! grep "Signed-off-by:" initial_msg &&
grep "Signed-off-by:" unrelatedpick_msg &&
-   test_must_fail grep "Signed-off-by:" picked_msg &&
+   ! grep "Signed-off-by:" picked_msg &&
grep "Signed-off-by:" anotherpick_msg
 '
 
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 9b19cff72..49d3621a9 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -152,7 +152,7 @@ test_expect_success 'push with 
receive.fsck.missingEmail=warn' '
git --git-dir=dst/.git config --add \
receive.fsck.badDate warn &&
git push --porcelain dst bogus >act 2>&1 &&
-   test_must_fail grep "missingEmail" act
+   ! grep "missingEmail" act
 '
 
 test_expect_success \
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 26b2cafc4..0fc5a7c59 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1004,7 +1004,7 @@ test_expect_success 'push --porcelain' '
 test_expect_success 'push --porcelain bad url' '
mk_empty testrepo &&
test_must_fail git push >.git/bar --porcelain asdfasdfasd 
refs/heads/master:refs/remotes/origin/master &&
-   test_must_fail grep -q Done .git/bar
+   ! grep -q Done .git/bar
 '
 
 test_expect_success 'push --porcelain rejected' '
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index a43339420..4241ea5b3 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -151,7 +151,7 @@ test_expect_success 'clone --mirror does not repeat tags' '
git clone --mirror src mirror2 &&
(cd mirror2 &&
 git show-ref 2> clone.err > clone.out) &&
-   test_must_fail grep Duplicate mirror2/clone.err &&
+   ! grep Duplicate mirror2/clone.err &&
grep some-tag mirror2/clone.out
 
 '
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 5e5370feb..8c2c6eaef 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -407,7 +407,7 @@ test_expect_success 'good merge base when good and bad are 
siblings' '
test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
grep $HASH4 my_bisect_log.txt &&
git bisect good > my_bisect_log.txt &&
-   test_must_fail grep "merge base must be tested" my_bisect_log.txt &&
+   ! grep "merge base must be tested" my_bisect_log.txt &&
grep $HASH6 my_bisect_log.txt &&
git bisect reset
 '
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 63d36fb28..0fe7e58cf 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -602,7 +602,7 @@ test_expect_success MKTEMP 'temporary filenames are used 
with mergetool.writeToT
test_config mergetool.myecho.trustExitCode true &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool myecho -- both >actual &&
-   test_must_fail grep ^\./both_LOCAL_ actual >/dev/null &&
+   ! grep ^\./both_LOCAL_ actual >/dev/null &&
grep /both_LOCAL_ actual >/dev/null &&
git reset --hard master >/dev/null 2>&1
 '
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 3dc4a3454..0f398dd16 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -50,7 +50,7 @@ test_no_confirm () {

[PATCH v3 2/2] t9813: avoid using pipes

[Pranit Bauva]

The exit code of the upstream in a pipe is ignored thus we should avoid
using it. By writing out the output of the git command to a file, we can
test the exit codes of both the commands.

Signed-off-by: Pranit Bauva 
---
 t/t9813-git-p4-preserve-users.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 798bf2b67..2133b21ae 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -118,12 +118,12 @@ test_expect_success 'not preserving user with mixed 
authorship' '
make_change_by_user usernamefile3 Derek de...@example.com &&
P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
export P4EDITOR P4USER P4PASSWD &&
-   git p4 commit |\
-   grep "git author de...@example.com does not match" &&
+   git p4 commit >actual &&
+   grep "git author de...@example.com does not match" actual &&
 
make_change_by_user usernamefile3 Charlie char...@example.com &&
-   git p4 commit |\
-   grep "git author char...@example.com does not match" &&
+   git p4 commit >actual &&
+   grep "git author char...@example.com does not match" actual &&
 
make_change_by_user usernamefile3 alice al...@example.com &&
git p4 commit >actual 2>&1 &&
-- 
2.11.0