Draft of Git Rev News edition 12

[Christian Couder]

Hi,

A draft of Git Rev News edition 12 is available here:

  https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-12.md

Everyone is welcome to contribute in any section either by editing the
above page on GitHub and sending a pull request, or by commenting on
this GitHub issue:

  https://github.com/git/git.github.io/issues/118

You can also reply to this email.

I tried to cc everyone who appears in this edition but maybe I missed
some people, sorry about that.

Thomas, Nicola and myself plan to publish this edition on Wednesday
the 10th of February.

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

Re: git show doesn't work on file names with square brackets

[Duy Nguyen]

On Sat, Feb 6, 2016 at 11:10 PM, Johannes Schindelin
 wrote:
> Hi Kirill,
>
> On Sat, 6 Feb 2016, Kirill Likhodedov wrote:
>
>> > On 06 Feb 2016, at 17:21 , Johannes Schindelin 
>> >  wrote:
>> >
>> > This is expected behavior of the Bash you are using. The commands that I
>> > think would reflect your intentions would be:
>> >
>> > git init brackets
>> > cd brackets
>> > echo 'asd' > 'bra[ckets].txt'
>> > git add 'bra[ckets].txt'
>> > git commit -m initial
>> > git show 'HEAD:bra[ckets].txt’
>>
>>
>> Nope. This command sequence doesn’t work for me: the same error is returned:
>>
>> # git show 'HEAD:bra[ckets].txt'
>> fatal: ambiguous argument 'HEAD:bra[ckets].txt': both revision and 
>> filename
>
> Whoops. Sorry. I actually ran those commands now and it is true that it
> still does not work, which is funny. Especially since
>
> git show 'HEAD:bra[ckets].txt' --
>
> actually *does* work.

It's from 28fcc0b (pathspec: avoid the need of "--" when wildcard is
used - 2015-05-02)
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ref-filter.c: don't stomp on memory

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Karthik,

If you need to re-roll your 'kn/ref-filter-atom-parsing' branch, could
you please squash this (or something like it) into the relevant patch
(commit 6613d5f1, "ref-filter: introduce parsing functions for each valid
atom", 31-01-2016).

This evening, (by mistake!) I built the pu branch with -fsanitize=address
in my CFLAGS. This resulted in many test failures, which were all caused
by the memcmp() call below stomping all over memory.

Hmm, as I was writing this email, I had a vague recollection of another
email on the list recently mentioning this code. So, if this has already
been reported, sorry for the noise!

ATB,
Ramsay Jones

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

diff --git a/ref-filter.c b/ref-filter.c
index d48e2a3..c98065e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -260,7 +260,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 * table.
 */
arg = memchr(sp, ':', ep - sp);
-   if ((!arg || len == arg - sp) &&
+   if ((( arg && len == arg - sp)  ||
+(!arg && len == ep - sp )) &&
!memcmp(valid_atom[i].name, sp, len))
break;
}
-- 
2.7.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Documentation/git-clean.txt: don't mention deletion of .git/modules/*

[Matt McCutchen]

I found no evidence of such behavior in the source code.

Signed-off-by: Matt McCutchen 
---

This is based on the maint branch, a08595f.

 Documentation/git-clean.txt | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 641681f..51a7e26 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -37,9 +37,7 @@ OPTIONS
    to false, 'git clean' will refuse to delete files or directories
    unless given -f, -n or -i. Git will refuse to delete directories
    with .git sub directory or file unless a second -f
-   is given. This affects also git submodules where the storage area
-   of the removed submodule under .git/modules/ is not removed until
-   -f is given twice.
+   is given.
 
 -i::
 --interactive::
-- 
2.5.0

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

[PATCH] setup.c: make check_filename() return 0 on ENAMETOOLONG

[Nguyễn Thái Ngọc Duy]

Noticed-by: Ole Tange 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 On Sun, Feb 7, 2016 at 4:56 AM, Ole Tange  wrote:
 > If file name too long it should just try to see if it is a reference
 > to a revision.

 Looks easy enough to fix.

 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 2c4b22c..ab8f85d 100644
--- a/setup.c
+++ b/setup.c
@@ -147,7 +147,7 @@ int check_filename(const char *prefix, const char *arg)
name = arg;
if (!lstat(name, ))
return 1; /* file exists */
-   if (errno == ENOENT || errno == ENOTDIR)
+   if (errno == ENOENT || errno == ENOTDIR || errno == ENAMETOOLONG)
return 0; /* file does not exist */
die_errno("failed to stat '%s'", arg);
 }
-- 
2.7.0.377.g4cd97dd

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

git diff HEAD^(255) fails

[Ole Tange]

git diff first looks for a file, then looks if it is a reference to a
revision. If the file fails due to being too long, the diff fails:

$ git init
$ git diff 
'HEAD^^^'
HEAD
fatal: failed to stat
'HEAD^^^':
File name too long

If file name too long it should just try to see if it is a reference
to a revision.


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

Re: changing colors in the tree view in gitk

[Johannes Schindelin]

Hi Philip,

On Fri, 5 Feb 2016, Philip Oakley wrote:

> From: "Britton Kerin" 
> >I upgraded from 2.5 to 2.7 and the branch names went from a light
> > green to dark green, the names of the tags are hard to read now.
> >
> > Is it possible to configure the branch name color in the tree view?
> > --
> Which Operating System is this on? and which Git version.?
> 
> 
> For the Git for Windows, the Mintty window colours can be adjusted. e.g.
> https://code.google.com/archive/p/mintty/wikis/Tips.wiki
> 
> Though I just changed my config setting for the awkward to read items (for me
> it was color.branch.upstream=green !)

I think Britton was talking about gitk, not about the console.

As it happens, this issue was already reported to the Git for Windows bug
tracker:

https://github.com/git-for-windows/git/issues/300

It would appear that the culprit is a change in Tcl/Tk, not in gitk. I
outlined a possible solution in that ticket but have no plans to implement
that solution myself.

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

Re: git show doesn't work on file names with square brackets

[Johannes Schindelin]

Hi Kirill,

On Sat, 6 Feb 2016, Kirill Likhodedov wrote:

> > On 06 Feb 2016, at 17:21 , Johannes Schindelin  
> > wrote:
> > 
> > This is expected behavior of the Bash you are using. The commands that I
> > think would reflect your intentions would be:
> > 
> > git init brackets
> > cd brackets
> > echo 'asd' > 'bra[ckets].txt'
> > git add 'bra[ckets].txt'
> > git commit -m initial
> > git show 'HEAD:bra[ckets].txt’
> 
> 
> Nope. This command sequence doesn’t work for me: the same error is returned:
> 
> # git show 'HEAD:bra[ckets].txt'
> fatal: ambiguous argument 'HEAD:bra[ckets].txt': both revision and 
> filename

Whoops. Sorry. I actually ran those commands now and it is true that it
still does not work, which is funny. Especially since

git show 'HEAD:bra[ckets].txt' --

actually *does* work.

Ciao,
Johannes

[ANNOUNCE] Git for Windows 2.7.1

[Johannes Schindelin]

Dear Git users,

It is my pleasure to announce that Git for Windows 2.7.1 is available from:

https://git-for-windows.github.io/

Changes since Git for Windows v2.7.0(2) (February 2nd 2016)

New Features

  ??? Comes with Git 2.7.1.

Bug Fixes

  ??? Git GUI now starts properly even when the working directory
contains non-ASCII characters.
  ??? We forgot to enable Address Space Layout Randomization and Data
Execution Prevention on our Git wrapper, and this is now fixed.
  ??? A bug in one of the DLLs used by Git for Windows was fixed that
prevented Git from working properly in 64-bit setups where the
FLG_LDR_TOP_DOWN global flag is set.

Filename | SHA-256
 | ---
Git-2.7.1-64-bit.exe | 
ab3eee9558f5bedffbe5518edcd84dbade813a013470d7640285a9c9c263be5a
Git-2.7.1-32-bit.exe | 
687e58df471bf88996a3ba619d25ccaaecd7243cbdb291f028abce68e8620569
PortableGit-2.7.1-64-bit.7z.exe | 
93b56b61973dce5b56127796b714cd29bd4777cce54e09e497dc1d0b2bb6057e
PortableGit-2.7.1-32-bit.7z.exe | 
f65ac7104a5b5c9d9bfa5b86df90acfe140ef5415ee8126daab050a157264cc7
Git-2.7.1-64-bit.tar.bz2 | 
2ab050864eaf60b158868a5a96ec5a2f2072a446a04dae9d290c4377871bb75f
Git-2.7.1-32-bit.tar.bz2 | 
a7b5d4d94b89e5eac5603c45ebbc28cb377f5835f6b3416f255972c77bd5226b

Ciao,
Johannes

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Daniel Stenberg]

On Fri, 5 Feb 2016, Junio C Hamano wrote:

OK, as Brian said, that use case would need to be in the log message, at 
least.  I am curious, though, if you can give just a random string to 
username, or at least that must match what the underlying authentication 
mechanism uses.


Brian, I can see how this would work in that use case, but I haven't 
convinced myself that the change would not affect other existing use cases 
that are supported--do you think of any that would negatively affect the 
user expeerience?


Even more, there is no other way to let libcurl to use GSS-Negotiate 
without username in URL.


Asking lubcurl expert about that might not be a bad idea; Cc'ed Daniel 
Stenberg.


It is correct that libcurl needs a username to trigger the use of HTTP 
authentication - any HTTP authentication - due to how we once designed the 
internals for this - but when using GSS-Negotiate the actually provided user 
name isn't used by libcurl for anything so it could be a fixed string or 
random junk, it doesn't matter as long as a name is provided.


--

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

Re: [PATCH v4 10/12] ref-filter: introduce remote_ref_atom_parser()

[Karthik Nayak]

On Fri, Feb 5, 2016 at 5:35 AM, Eric Sunshine  wrote:
> On Sun, Jan 31, 2016 at 11:12:54PM +0530, Karthik Nayak wrote:
>> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
>> and '%(push)' atoms and store information into the 'used_atom'
>> structure based on the modifiers used along with the corresponding
>> atom.
>>
>> Signed-off-by: Karthik Nayak 
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -50,6 +52,20 @@ static void color_atom_parser(struct used_atom *atom, 
>> const char *color_value)
>> +static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> + if (!arg) {
>> + atom->u.remote_ref = RR_NORMAL;
>> + } else if (!strcmp(arg, "short"))
>
> Style: drop unnecessary braces
>

Will do.

>> + atom->u.remote_ref = RR_SHORTEN;
>> + else if (!strcmp(arg, "track"))
>> + atom->u.remote_ref = RR_TRACK;
>> + else if (!strcmp(arg, "trackshort"))
>> + atom->u.remote_ref = RR_TRACKSHORT;
>> + else
>> + die(_("unrecognized format: %%(%s)"), atom->name);
>> +}



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

Re: [PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

On Fri, Feb 5, 2016 at 3:55 AM, Eric Sunshine  wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  wrote:
>> Introduce color_atom_parser() which will parse a "color" atom and
>> store its color in the "used_atom" structure for further usage in
>> populate_value().
>>
>> Signed-off-by: Karthik Nayak 
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } 
>> cmp_type;
>>  static struct used_atom {
>> const char *name;
>> cmp_type type;
>> +   union {
>> +   char color[COLOR_MAXLEN];
>> +   } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>>  static int need_color_reset_at_eol;
>>
>> +static void color_atom_parser(struct used_atom *atom, const char 
>> *color_value)
>> +{
>> +   if (!color_value)
>> +   die(_("expected format: %%(color:)"));
>> +   if (color_parse(color_value, atom->u.color) < 0)
>> +   die(_("invalid color value: %s"), atom->u.color);
>
> Shouldn't this be:
>
> die(_("invalid color value: %s"), color_value);
>
> ?

Oops. You're right, it should.
Also the error is reported already in color_parse(...), so seems duplicated.

e.g.

git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
error: invalid color value: sfadf
fatal: invalid color value: sfadf

What would be an ideal way around this?

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

Re: [PATCH v4 08/12] ref-filter: introduce align_atom_parser()

[Karthik Nayak]

On Fri, Feb 5, 2016 at 5:18 AM, Eric Sunshine  wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  wrote:
>> Introduce align_atom_parser() which will parse an 'align' atom and
>> store the required alignment position and width in the 'used_atom'
>> structure for further usage in populate_value().
>>
>> Since this patch removes the last usage of match_atom_name(), remove
>> the function from ref-filter.c.
>>
>> Signed-off-by: Karthik Nayak 
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -55,6 +61,37 @@ static align_type parse_align_position(const char *s)
>> +static void align_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> +   struct align *align = >u.align;
>> +   struct strbuf **s, **to_free;
>> +   unsigned int width = ~0U;
>> +
>> +   if (!arg)
>> +   die(_("expected format: %%(align:,)"));
>> +   s = to_free = strbuf_split_str_omit_term(arg, ',', 0);
>> +
>> +   align->position = ALIGN_LEFT;
>> +
>> +   while (*s) {
>> +   int position;
>> +   arg = s[0]->buf;
>
> It's confusing to see 'arg' being re-used here for a different
> purpose, and leads the reader to wonder if this is done because the
> s[0]->buf might be needed outside the loop (when, in fact, it isn't).
> It would be better to declare a new variable here in the scope of the
> 'while' loop to hold this value.
>
> (I might have named the result of the strbuf split 'tokens' or even
> short-and-sweet 'v' -- for vector -- and then used 's' for the name of
> the new variable here in the 'while' loop, but these name suggestions
> aren't particularly important; it is important to declare a new
> variable here -- whatever you name it -- rather than re-using 'arg'.)
>

You're right, that is indeed confusing, I should stop reusing variables
and trying to micromanage.

I also like the naming scheme you suggested, so will stick to that.
Thanks.

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

Re: [PATCH v4 09/12] ref-filter: align: introduce long-form syntax

[Karthik Nayak]

On Fri, Feb 5, 2016 at 5:30 AM, Eric Sunshine  wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  wrote:
>> Introduce optional prefixes "width=" and "position=" for the align atom
>> so that the atom can be used as "%(align:width=,position=)".
>>
>> Add Documentation and tests for the same.
>>
>> Signed-off-by: Karthik Nayak 
>> ---
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> @@ -133,6 +133,48 @@ test_expect_success 'right alignment' '
>> +cat >expect <<-\EOF
>> +|   refname is refs/heads/master   |refs/heads/master
>> +|refname is refs/heads/side|refs/heads/side
>> +| refname is refs/odd/spot |refs/odd/spot
>> +| refname is refs/tags/double-tag  |refs/tags/double-tag
>> +|refname is refs/tags/four |refs/tags/four
>> +| refname is refs/tags/one |refs/tags/one
>> +| refname is refs/tags/signed-tag  |refs/tags/signed-tag
>> +|refname is refs/tags/three|refs/tags/three
>> +| refname is refs/tags/two |refs/tags/two
>> +EOF
>> +
>> +test_align_permutations() {
>> +   while read -r option
>> +   do
>> +   test_expect_success "align:$option" '
>> +   git for-each-ref --format="|%(align:$option)refname is 
>> %(refname)%(end)|%(refname)" >actual &&
>> +   test_cmp expect actual
>> +   '
>
> I think I mentioned this in the last round: The two lines following
> test_expect_success() are actually the test body, thus should be
> indented one more level. (Not necessarily worth a re-roll, though...)
>

I must have missed it, will change it.

>> +   done
>> +}
>> +
>> +test_align_permutations <<-\EOF
>> +   middle,42
>> +   42,middle
>> +   position=middle,42
>> +   42,position=middle
>> +   middle,width=42
>> +   width=42,middle
>> +   position=middle,width=42
>> +   width=42,position=middle
>> +EOF
>> +
>> +# Last one wins (silently) when multiple arguments of the same type are 
>> given
>> +
>> +test_align_permutations <<-\EOF
>> +   32,width=42,middle
>> +   width=30,42,middle
>> +   width=42,position=right,middle
>> +   42,right,position=middle
>> +EOF
>> +
>>  # Individual atoms inside %(align:...) and %(end) must not be quoted.
>>
>>  test_expect_success 'alignment with format quote' "
>> --
>> 2.7.0



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

Re: [PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Christian Couder]

On Sat, Feb 6, 2016 at 4:20 PM, Karthik Nayak  wrote:
> On Fri, Feb 5, 2016 at 3:55 AM, Eric Sunshine  wrote:
>> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  
>> wrote:
>>> Introduce color_atom_parser() which will parse a "color" atom and
>>> store its color in the "used_atom" structure for further usage in
>>> populate_value().
>>>
>>> Signed-off-by: Karthik Nayak 
>>> ---
>>> diff --git a/ref-filter.c b/ref-filter.c
>>> @@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } 
>>> cmp_type;
>>>  static struct used_atom {
>>> const char *name;
>>> cmp_type type;
>>> +   union {
>>> +   char color[COLOR_MAXLEN];
>>> +   } u;
>>>  } *used_atom;
>>>  static int used_atom_cnt, need_tagged, need_symref;
>>>  static int need_color_reset_at_eol;
>>>
>>> +static void color_atom_parser(struct used_atom *atom, const char 
>>> *color_value)
>>> +{
>>> +   if (!color_value)
>>> +   die(_("expected format: %%(color:)"));
>>> +   if (color_parse(color_value, atom->u.color) < 0)
>>> +   die(_("invalid color value: %s"), atom->u.color);
>>
>> Shouldn't this be:
>>
>> die(_("invalid color value: %s"), color_value);
>>
>> ?
>
> Oops. You're right, it should.
> Also the error is reported already in color_parse(...), so seems duplicated.
>
> e.g.
>
> git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
> error: invalid color value: sfadf
> fatal: invalid color value: sfadf
>
> What would be an ideal way around this?

Maybe it has already been discussed a lot and I missed the discussion,
but if possible the argument, the parameter or the atom itself might
just be ignored with a warning instead of dying when an atom argument,
format or parameter is not recognized, because in the next Git
versions we might want to add new arguments, formats and parameter and
it would be sad if old versions of Git die when those new things are
passed to them.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ref-filter.c: don't stomp on memory

[Eric Sunshine]

On Sat, Feb 6, 2016 at 7:23 PM, Ramsay Jones
 wrote:
> If you need to re-roll your 'kn/ref-filter-atom-parsing' branch, could
> you please squash this (or something like it) into the relevant patch
> (commit 6613d5f1, "ref-filter: introduce parsing functions for each valid
> atom", 31-01-2016).
>
> This evening, (by mistake!) I built the pu branch with -fsanitize=address
> in my CFLAGS. This resulted in many test failures, which were all caused
> by the memcmp() call below stomping all over memory.
>
> Hmm, as I was writing this email, I had a vague recollection of another
> email on the list recently mentioning this code. So, if this has already
> been reported, sorry for the noise!

You're probably thinking of [1]. Interestingly, the two proposed fixes
differ... (more below)

[1]: 
http://git.661346.n2.nabble.com/PATCH-v4-00-12-ref-filter-use-parsing-functions-td7646877i20.html#a7647418

> diff --git a/ref-filter.c b/ref-filter.c
> @@ -260,7 +260,8 @@ int parse_ref_filter_atom(const char *atom, const char 
> *ep)
>  * table.
>  */
> arg = memchr(sp, ':', ep - sp);
> -   if ((!arg || len == arg - sp) &&
> +   if ((( arg && len == arg - sp)  ||
> +(!arg && len == ep - sp )) &&
> !memcmp(valid_atom[i].name, sp, len))
> break;
> }

Your fix is pretty easy to to read and seems correct. Karthik's fix
required several re-reads, and I *think* it may be correct, however,
it's difficult to grok due to its logic inversion.

Aside from the two proposed fixes, a fix patterned after the original
code which patch 5/12 replaced would be even easier to understand.
That is, something like this:

arg = memchr(...);
if (!arg)
arg = ep;
if (len == arg - sp && !memcmp(...))
...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git submodule should honor "-c credential.helper" command line argument

[Jacob Keller]

On Wed, Feb 3, 2016 at 3:44 PM, Jacob Keller  wrote:
> Ok so I am not sure we even really need to use "-c" option in
> git-clone considering that we can just use the same flow we do for
> setting core.worktree values. I'll propose a patch with you two Cc'ed,
> which I think fixes the issue. There may actually be a set of
> configuration we want to include though, and the main issue I see is
> that it won't get updated correctly whenever the parent configuration
> changes.
>
> Thanks,
> Jake

I tried adding the config as part of module_clone in
submodule--helper.c but it didn't pass the test I added. I haven't had
time to look at this in the last few days, but I am stuck as to why
submodule--helper.c appeared to not use module_clone as I thought.

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

Re: [PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Eric Sunshine]

On Sat, Feb 6, 2016 at 10:51 AM, Christian Couder
 wrote:
> On Sat, Feb 6, 2016 at 4:20 PM, Karthik Nayak  wrote:
>> Also the error is reported already in color_parse(...), so seems duplicated.
>>
>> git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
>> error: invalid color value: sfadf
>> fatal: invalid color value: sfadf
>>
>> What would be an ideal way around this?
>
> Maybe it has already been discussed a lot and I missed the discussion,
> but if possible the argument, the parameter or the atom itself might
> just be ignored with a warning instead of dying when an atom argument,
> format or parameter is not recognized, because in the next Git
> versions we might want to add new arguments, formats and parameter and
> it would be sad if old versions of Git die when those new things are
> passed to them.

The current behavior of die()ing is inherited from existing code which
Karthik refactored to create ref-filter.c, so it is not a new issue,
and old versions of Git are already afflicted. Whether die()ing is
desirable or not is unrelated to the current series which is primarily
aimed at optimizing and slightly generalizing ref-filter.c.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ref-filter.c: don't stomp on memory

[Karthik Nayak]

On Sun, Feb 7, 2016 at 8:46 AM, Eric Sunshine  wrote:
> On Sat, Feb 6, 2016 at 7:23 PM, Ramsay Jones
>  wrote:
>> If you need to re-roll your 'kn/ref-filter-atom-parsing' branch, could
>> you please squash this (or something like it) into the relevant patch
>> (commit 6613d5f1, "ref-filter: introduce parsing functions for each valid
>> atom", 31-01-2016).
>>
>> This evening, (by mistake!) I built the pu branch with -fsanitize=address
>> in my CFLAGS. This resulted in many test failures, which were all caused
>> by the memcmp() call below stomping all over memory.
>>
>> Hmm, as I was writing this email, I had a vague recollection of another
>> email on the list recently mentioning this code. So, if this has already
>> been reported, sorry for the noise!
>

Thanks for reporting, I stumbled upon the same problem myself.

> You're probably thinking of [1]. Interestingly, the two proposed fixes
> differ... (more below)
>
> [1]: 
> http://git.661346.n2.nabble.com/PATCH-v4-00-12-ref-filter-use-parsing-functions-td7646877i20.html#a7647418
>
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -260,7 +260,8 @@ int parse_ref_filter_atom(const char *atom, const char 
>> *ep)
>>  * table.
>>  */
>> arg = memchr(sp, ':', ep - sp);
>> -   if ((!arg || len == arg - sp) &&
>> +   if ((( arg && len == arg - sp)  ||
>> +(!arg && len == ep - sp )) &&
>> !memcmp(valid_atom[i].name, sp, len))
>> break;
>> }
>
> Your fix is pretty easy to to read and seems correct. Karthik's fix
> required several re-reads, and I *think* it may be correct, however,
> it's difficult to grok due to its logic inversion.
>

True, its not so easy to comprehend at first.

> Aside from the two proposed fixes, a fix patterned after the original
> code which patch 5/12 replaced would be even easier to understand.
> That is, something like this:
>
> arg = memchr(...);
> if (!arg)
> arg = ep;
> if (len == arg - sp && !memcmp(...))
> ...

This seems good, will change, Thanks to both of you

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

Re: [PATCH v4 11/12] ref-filter: introduce contents_atom_parser()

[Karthik Nayak]

On Fri, Feb 5, 2016 at 5:52 AM, Eric Sunshine  wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  wrote:
>> Introduce contents_atom_parser() which will parse the '%(contents)'
>> atom and store information into the 'used_atom' structure based on the
>> modifiers used along with the atom. Also introduce body_atom_parser()
>> and subject_atom_parser() for parsing atoms '%(body)' and '%(subject)'
>> respectively.
>
> These latter two parsers are conceptually distinct from introduction
> of the %(contents) parser, thus could be done in a follow-on patch or
> two (though I don't care strongly enough to insist upon it).
>

I think they go together, considering they use the same contents structure in
used_atom, Introducing separate patches would leave us in a grey area where
%(contents:...) uses used_atom->contents whereas body and subject don't.
So I'd prefer them being together.

>> Signed-off-by: Karthik Nayak 
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -66,6 +70,38 @@ static void remote_ref_atom_parser(struct used_atom 
>> *atom, const char *arg)
>> +static void body_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> +   if (arg)
>> +   die("%%(body) atom does not take arguments");
>
> None of the other error messages bothers saying "atom" literally
> following a %(foo). For consistency, this likely should say merely:
>
> %(body) does not take arguments
>

Will change.

>> +   atom->u.contents.option = C_BODY_DEP;
>> +}
>> +
>> +static void subject_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> +   if (arg)
>> +   die("%%(subject) atom does not take arguments");
>
> Ditto.
>

Will change.

>> +   atom->u.contents.option = C_SUB;
>> +}
>> @@ -733,19 +763,15 @@ static void grab_sub_body_contents(struct atom_value 
>> *val, int deref, struct obj
>>
>> for (i = 0; i < used_atom_cnt; i++) {
>> const char *name = used_atom[i].name;
>> +   struct used_atom *atom = _atom[i];
>
> Not a big deal, but if you re-order these two lines, then the second,
> which extracts name, could do so from the variable declared by the
> first:
>
> struct used_atom *atom = _atom[i];
> const char *name = atom->name;
>

Seems good, will incorporate.

>> struct atom_value *v = [i];
>> -   const char *valp = NULL;
>> if (!!deref != (*name == '*'))
>> continue;
>> if (deref)
>> name++;
>> if (strcmp(name, "subject") &&
>> strcmp(name, "body") &&
>> -   strcmp(name, "contents") &&
>> -   strcmp(name, "contents:subject") &&
>> -   strcmp(name, "contents:body") &&
>> -   strcmp(name, "contents:signature") &&
>> -   !starts_with(name, "contents:lines="))
>> +   !starts_with(name, "contents"))
>> continue;
>
> This changes behavior in that it will also now match
> "contentsanything", whereas the original was much more strict. Is that
> desirable? (Genuine question.)
>

Well, we wont have something like that come through because
parse_ref_filter_atom()
would throw an error for %(contentsanything), but if in the future
sometime if we
introduce an atom %(contentsfoo) this might end up being a problem.

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

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Eric Sunshine]

On Sat, Feb 6, 2016 at 9:36 AM, Karthik Nayak  wrote:
> On Thu, Feb 4, 2016 at 3:49 AM, Eric Sunshine  wrote:
>> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  
>> wrote:
>>> -   const char *formatp = strchr(sp, ':');
>>> -   if (!formatp || ep < formatp)
>>> -   formatp = ep;
>>> -   if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, 
>>> len))
>>> +   arg = memchr(sp, ':', ep - sp);
>>
>> Why this change from strchr() to memchr()? I understand that you're
>> taking advantage of the fact that you know the extent of the string
>> via 'sp' and 'ep', however, was the original strchr() doing extra
>> work? Even if this change is desirable, it seems somewhat unrelated to
>> the overall purpose of this patch, thus might deserves its own.
>>
>> Aside from that, although the "expensive" strchr() / memchr() resides
>> within the loop, it will always return the same value since it doesn't
>> depend upon any condition local to the loop. This implies that it
>> ought to be hoisted out of the loop. (This problem is not new to this
>> patch; it's already present in the existing code.)
>
> I'm thinking I'll make a patch for that separately. i.e remove strchr()
> and introduce memchr() outside the loop.

I'd almost suggest making it two patches: (1) change strchr() to
memchr(), and (2) hoist it outside the loop. However, it would be nice
to see this series land with v5, and adding more refactoring patches
could delay its landing if problems are found with those new patches.
Consequently, it might make sense to forego any additional refactoring
for now and just keep the patch as is, except for fixing the
relatively minor issues (and bug or two) raised in the v4 review. If
you take that approach, then hoisting memchr() out of the loop can be
done as a follow-up patch after this series lands.

>>> +   if ((!arg || len == arg - sp) &&
>>> +   !memcmp(valid_atom[i].name, sp, len))
>>> break;
>>> }
>>>
>>> @@ -154,6 +155,10 @@ int parse_ref_filter_atom(const char *atom, const char 
>>> *ep)
>>> REALLOC_ARRAY(used_atom, used_atom_cnt);
>>> used_atom[at].name = xmemdupz(atom, ep - atom);
>>> used_atom[at].type = valid_atom[i].cmp_type;
>>> +   if (arg)
>>> +   arg = used_atom[at].name + (arg - atom) + 1;
>>
>> This is a harder to understand than it ought to be because it's
>> difficult to tell at first glance that you don't actually care about
>> 'arg' in relation to the original incoming string, but instead use it
>> only to compute an offset into the string which is ultimately stored
>> in the newly allocated used_atom[]. Re-using 'arg' for a different
>> purpose (in a manner of speaking) confuses the issue further.
>>
>> The intention might be easier to follow if you made it clear that you
>> were interested in the *offset* of the argument in the string, rather
>> than a pointer into the incoming string which you ultimately don't
>> use. A variable named 'arg_offset' might go a long way toward
>> clarifying this intention.
>
> I hope you mean something like this.
>
> if (arg) {
> int arg_offset;
>
> arg_offset = (arg - atom) + 1;
> arg = used_atom[at].name + arg_offset;
> }

That's one way, but I was actually thinking about computing arg_offset
earlier in the "is it a valid atom?" loop, which would make it clear
that you care only about the offset at that point, rather than the
pointer to the ':' in the original string (since that pointer is never
itself used other than to compute the offset). However, having tried
it myself, the code ends up being nosier, thus not necessarily a win,
so maybe just leave this as is for now.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Eric Sunshine]

On Sat, Feb 6, 2016 at 10:20 AM, Karthik Nayak  wrote:
> On Fri, Feb 5, 2016 at 3:55 AM, Eric Sunshine  wrote:
>> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  
>> wrote:
>>> +static void color_atom_parser(struct used_atom *atom, const char 
>>> *color_value)
>>> +{
>>> +   if (!color_value)
>>> +   die(_("expected format: %%(color:)"));
>>> +   if (color_parse(color_value, atom->u.color) < 0)
>>> +   die(_("invalid color value: %s"), atom->u.color);
>>
>> Shouldn't this be:
>>
>> die(_("invalid color value: %s"), color_value);
>>
>> ?
>
> Oops. You're right, it should.
> Also the error is reported already in color_parse(...), so seems duplicated.
>
> git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
> error: invalid color value: sfadf
> fatal: invalid color value: sfadf
>
> What would be an ideal way around this?

According to f6c5a29 (color_parse: do not mention variable name in
error message, 2014-10-07), the doubled diagnostic messages are
intentional, so I don't think you need to do anything about it in this
series. If you want to make the "fatal" message a bit more helpful,
you could add a %(color:) annotation, like this:

die(_("unrecognized color: %%(color:%s)"), color_value);

which would give you the slightly more helpful:

error: invalid color value: sfadf
fatal: unrecognized color: %(color:sfadf)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Eric Sunshine]

On Sat, Feb 6, 2016 at 10:15 AM, Karthik Nayak  wrote:
> On Sun, Jan 31, 2016 at 11:12 PM, Karthik Nayak  wrote:
>> @@ -138,10 +140,9 @@ int parse_ref_filter_atom(const char *atom, const char 
>> *ep)
>>  * shouldn't be used for checking against the valid_atom
>>  * table.
>>  */
>> -   const char *formatp = strchr(sp, ':');
>> -   if (!formatp || ep < formatp)
>> -   formatp = ep;
>> -   if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, 
>> len))
>> +   arg = memchr(sp, ':', ep - sp);
>> +   if ((!arg || len == arg - sp) &&
>> +   !memcmp(valid_atom[i].name, sp, len))
>> break;
>> }
>
> Also having a look at this, this breaks the previous error checking we
> had at parse_ref_filter_atom().
> e.g: git for-each-ref --format="%(refnameboo)" would not throw an error.
>
> I think the code needs to be changed to:
>
> -   if ((!arg || len == arg - sp) &&
> +   if ((arg || len == ep - sp) &&
> +   (!arg || len == arg - sp) &&

For completeness, for people reading the mailing list archive, a
couple alternate fixes were presented elsewhere[1], with a personal
bias toward:

arg = memchr(...);
if (!arg)
arg = ep;
if (len == arg - sp && !memcmp(...))
...

[1]: 
http://git.661346.n2.nabble.com/PATCH-ref-filter-c-don-t-stomp-on-memory-tp7647432p7647433.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Karthik Nayak]

On Sun, Jan 31, 2016 at 11:12 PM, Karthik Nayak  wrote:
> @@ -138,10 +140,9 @@ int parse_ref_filter_atom(const char *atom, const char 
> *ep)
>  * shouldn't be used for checking against the valid_atom
>  * table.
>  */
> -   const char *formatp = strchr(sp, ':');
> -   if (!formatp || ep < formatp)
> -   formatp = ep;
> -   if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, 
> len))
> +   arg = memchr(sp, ':', ep - sp);
> +   if ((!arg || len == arg - sp) &&
> +   !memcmp(valid_atom[i].name, sp, len))
> break;
> }
>

Also having a look at this, this breaks the previous error checking we
had at parse_ref_filter_atom().
e.g: git for-each-ref --format="%(refnameboo)" would not throw an error.

I think the code needs to be changed to:

-   if ((!arg || len == arg - sp) &&
+   if ((arg || len == ep - sp) &&
+   (!arg || len == arg - sp) &&

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

git show doesn't work on file names with square brackets

[Kirill Likhodedov]

I’ve faced a problem that `git show :` returns an error when 
 contains square brackets.

Interestingly, the problem is reproducible on "GNU bash, version 
3.2.57(1)-release (x86_64-apple-darwin15)", but not on "zsh 5.0.7 
(x86_64-pc-linux-gnu)”. The problem is also reproducible when called from a 
Java program by forking a process with given parameters.

Is it a bug or I just didn’t find the proper way to escape the brackets? 

Steps to reproduce:

git init brackets
cd brackets/
echo ‘asd’ > bra[ckets].txt
git add bra\[ckets\].txt
git commit -m initial
git show HEAD:bra[ckets].txt

Error:
fatal: ambiguous argument 'HEAD:bra[ckets].txt': both revision and filename
Use '--' to separate paths from revisions, like this:
'git  [...] -- [...]’

Neither escaping, not quoting doesn’t help:
git show HEAD:bra\[ckets\].txt
returns the same error

git show "HEAD:bra\[ckets\].txt”
returns empty output

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

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Karthik Nayak]

On Thu, Feb 4, 2016 at 3:49 AM, Eric Sunshine  wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  wrote:
>> Parsing atoms is done in populate_value(), this is repetitive and
>> hence expensive. Introduce a parsing function which would let us parse
>> atoms beforehand and store the required details into the 'used_atom'
>> structure for further usage.
>>
>> Helped-by: Eric Sunshine 
>> Signed-off-by: Karthik Nayak 
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -36,6 +36,7 @@ static int need_color_reset_at_eol;
>>  static struct {
>> const char *name;
>> cmp_type cmp_type;
>> +   void (*parser)(struct used_atom *atom, const char *arg);
>
> It's a little bit weird to pass in 'arg' as an additional argument
> considering that the parser already has access to the same information
> via the atom's 'name' field. I guess you're doing it as a convenience
> so that parsers don't have to do the strchr(':') or memchr(':')
> themselves (and because parse_ref_filter_atom() already has the
> information at hand), right? A typical parser interested in a
> (possibly optional) argument currently looks like this:
>
> void parse_foo(struct used_atom *a, const char *arg) {
> if (!arg)
> /* default behavior: arg absent */
> else
> /* process arg */
> }
>
> That doesn't change much if you drop the 'arg' argument:
>
> void parse_foo(struct used_atom *a) {
> const char *arg = strchr(a->name, ':')
> if (!arg)
> /* default behavior: arg absent */
> else
> /* process arg */
> }
>
> It does mean a very minimal amount of duplicated code (the single
> strchr() line per parser), but it also would remove a bit of the
> complexity which this patch adds to parse_ref_filter_atom(). So, I
> dunno...
>

This change is introduced as a result of he suggestion given from Junio[1].
Although we're adding a little complexity to parse_ref_filter_atom() I feel its
justified by the interface provided as you mentioned. This ensures that parser
functions don't need to implement their own methods for getting the arguments
and can rely on being provided the arguments to them.

1: http://article.gmane.org/gmane.comp.version-control.git/283499

>>  } valid_atom[] = {
>> { "refname" },
>> { "objecttype" },
>> @@ -138,10 +140,9 @@ int parse_ref_filter_atom(const char *atom, const char 
>> *ep)
>>  * shouldn't be used for checking against the valid_atom
>>  * table.
>>  */
>> -   const char *formatp = strchr(sp, ':');
>> -   if (!formatp || ep < formatp)
>> -   formatp = ep;
>> -   if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, 
>> len))
>> +   arg = memchr(sp, ':', ep - sp);
>
> Why this change from strchr() to memchr()? I understand that you're
> taking advantage of the fact that you know the extent of the string
> via 'sp' and 'ep', however, was the original strchr() doing extra
> work? Even if this change is desirable, it seems somewhat unrelated to
> the overall purpose of this patch, thus might deserves its own.
>

I'm thinking I'll make a patch for that separately. i.e remove strchr()
and introduce memchr() outside the loop.

> Aside from that, although the "expensive" strchr() / memchr() resides
> within the loop, it will always return the same value since it doesn't
> depend upon any condition local to the loop. This implies that it
> ought to be hoisted out of the loop. (This problem is not new to this
> patch; it's already present in the existing code.)
>

Yes, makes sense.

>> +   if ((!arg || len == arg - sp) &&
>> +   !memcmp(valid_atom[i].name, sp, len))
>> break;
>> }
>>
>> @@ -154,6 +155,10 @@ int parse_ref_filter_atom(const char *atom, const char 
>> *ep)
>> REALLOC_ARRAY(used_atom, used_atom_cnt);
>> used_atom[at].name = xmemdupz(atom, ep - atom);
>> used_atom[at].type = valid_atom[i].cmp_type;
>> +   if (arg)
>> +   arg = used_atom[at].name + (arg - atom) + 1;
>
> This is a harder to understand than it ought to be because it's
> difficult to tell at first glance that you don't actually care about
> 'arg' in relation to the original incoming string, but instead use it
> only to compute an offset into the string which is ultimately stored
> in the newly allocated used_atom[]. Re-using 'arg' for a different
> purpose (in a manner of speaking) confuses the issue further.
>
> The intention might be easier to follow if you made it clear that you
> were interested in the *offset* of the argument in the string, rather
> than a pointer into the incoming string which you ultimately don't
> use. A variable named 'arg_offset' might go a long way toward
> clarifying this intention.

Re: [PATCH v2 03/25] transport-helper.c: do not send null option to remote helper

[Duy Nguyen]

On Fri, Feb 5, 2016 at 6:22 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>
> This is even more strange.  Are the current callers broken and some
> sends value==NULL for an option that is not is_bool, resulting in
> a call to quote_c_style() with NULL?  I somehow find it hard to
> believe as that would lead to an immediate segfault.
>
> Assuming that no current caller passes NULL to value when is_bool is
> not in effect, there needs an explanation why future new callers may
> need to do so.  An alternative for a valueless option could be to
> send "option name\n" instead of the usual "option name value\n", but
> without such an explanation, readers cannot tell why not sending
> anything about "name", which is what this patch chooses to implement,
> is a better idea.

The source is backfill_tags() which, in future, resets some transport
options back to defaults. The current set_option() in there only deals
with booleans or number (depth). But in future it resets deepen-since,
which is a string.

I think the main reason is, we do not have a way to reset (or unset) a
transport option. Should I keep this commit and explain about this, or
have a new transport API to reset option?
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git archive should use vendor extension in pax header

[fuz]

On Sat, Feb 06, 2016 at 02:23:11PM +0100, René Scharfe wrote:
> Am 28.01.2016 um 00:45 schrieb f...@fuz.su:
> >>There is git get-tar-commit-id, which prints the commit ID if it
> >>finds a comment entry which looks like a hexadecimal SHA-1 hash.
> >>It's better than a hex editor at least. :)
> >
> >This is incredibly fuzzy and can get wrong for a pleothora of reasons.
> >I hope you agree though that the situation is suboptimal, git is doing
> >the equivalent of using a custom file format without an easily
> >recognizable magic number.
> 
> It is fuzzy in theory. But which other programs allow writing a
> comment header?  I'm not aware of any, but I have to admit that I
> didn't look too hard.

Well, let's say what happens if the Mercurial folks were to implement
the same thing? Suddenly there is a conflict. Yes, of course, right now
there might be no program that uses the comment field for its own
purpose but such design decisions tend to be not future proof. There is
a very good reason why file formats typically have magic numbers and
don't just rely on people knowing that the file has a certain type and
that is the same reason why git should mark its meta data in a unique
fashion.

> >>But I'm still interested how you got a collection of tar files with
> >>unknown origin.  Just curious.
> >
> >Easy: Just download the (source) distribution archives of a distribution
> >of choice and try to verify that the tarballs they use to compile their
> >packages actually come from the project's public git repositories.
> 
> OK, that's easier than calculating checksums and comparing them with
> those published by the respective projects, but also less
> trustworthy.

If you have a known trusted archive, you could use it directly, no need
for cross-verification. The intent is to be able to check if archives
generated by someone from some sources could have plausibly been
generated from these sources.

> >There are other reasons why an easily detectable git hash might be
> >useful.  For example, file(1) could show that the archive comes from
> >git.  Other utilities could use this to work around git-specific bugs.
> >An unpacker could add corresponding meta-data when unpacking the file.
> 
> file(1) could use the same heuristic as git get-tar-commit-id.
> Something like this would work (the first line is already shipped
> with file):
> 
>   257 string  ustar\0 POSIX tar archive
>   >156string  g
>   >>512   string  52\ comment=
>   >>>523  regex   [0-9a-f]{40}\b, git commit %s
> 
> NB: With Ian Darwin's file you need to use -e tar in order to turn
> off its internal tar test.

Same issues as above.

> I'm very interested in hearing about any git specific bugs.

I don't know any. Bugs tens to be known only after 1000s of buggy
archives have been published (just as with some GNU tar bugs). It's
great to have a way to detect that the archive might be affected by
a bug so you know that you need to work around it.

> >It would be much more useful if git created a
> >custom key. As per POSIX suggestions, something like this would be
> >appropriate:
> >
> > GIT.commit=57ca140635bf157354124e4e4b3c8e1bde2832f1
> 
> This would be included in addition to the comment in order to avoid
> breaking existing users, I guess.
> >>>
> >>>Good point.  I'm not sure how many user use the comment header at all.
> >>
> >>Apart from git get-tar-commit-id I don't know any program for
> >>extracting pax comments.  And I don't know how widely used that is,
> >>but I assume there is *someone* out there, extracting commit IDs
> >>with it.
> >
> >Neither do I.  But remember, POSIX explicitly specifies that programs
> >that parse pax file must ignore pax comments so an unpacker that
> >interpretes the content of such a comment in any way is in violation of
> >the pax specification.
> 
> Almost right: The spec says that *pax* shall ignore comments.  Which
> is good -- we can use this field to transport anything without pax
> complaining.

The intent of the committee is that comments shall be ignored by all
software that processes tar files. Of course you can put metadata into
the comments, but that is just a perversion of the file format. POSIX
explicitly states that unknown keys in extended headers are to be
ignored by extractors, so using these should be fine, too. But you are
right, some research needs to be done as to how different archivers deal
with unexpected keys in pax headers.

> >>>Maybe there should also be an
> >>>GIT.original-name key.
> >>
> >>What would it be used for?
> >
> >In case an export substition changes the file name so the implementation
> >can verify that the original file could plausibly have been substituted
> >into the current name.  Also for the case where multiple files
> >substitute into the same name to tell which file git should check
> >equivalency with.
> 
> Stupid question: Could you please provide an example?  The only
> possibility for name changes 

Re: git show doesn't work on file names with square brackets

[Kirill Likhodedov]

Hi Johannes,

thanks for your answer, but unfortunately it doesn’t help.

> On 06 Feb 2016, at 17:21 , Johannes Schindelin  
> wrote:
> 
> This is expected behavior of the Bash you are using. The commands that I
> think would reflect your intentions would be:
> 
>   git init brackets
>   cd brackets
>   echo 'asd' > 'bra[ckets].txt'
>   git add 'bra[ckets].txt'
>   git commit -m initial
>   git show 'HEAD:bra[ckets].txt’


Nope. This command sequence doesn’t work for me: the same error is returned:

# git show 'HEAD:bra[ckets].txt'
fatal: ambiguous argument 'HEAD:bra[ckets].txt': both revision and filename


> You could also escape the brackets with a backslash, as you did, but you
> would have to do it *every* time you write the path, not just in the `git
> add` incantation.


As I mentioned at the end of my original message, escaping doesn't help either. 
`git add` works fine both with and without escape. It was auto-completed by 
bash completion, and I just forgot to remove the backslashes before pasting the 
code here. At any case, escaping doesn’t work with `git show`.--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git archive should use vendor extension in pax header

[René Scharfe]

Am 28.01.2016 um 00:45 schrieb f...@fuz.su:

There is git get-tar-commit-id, which prints the commit ID if it
finds a comment entry which looks like a hexadecimal SHA-1 hash.
It's better than a hex editor at least. :)


This is incredibly fuzzy and can get wrong for a pleothora of reasons.
I hope you agree though that the situation is suboptimal, git is doing
the equivalent of using a custom file format without an easily
recognizable magic number.


It is fuzzy in theory. But which other programs allow writing a comment 
header?  I'm not aware of any, but I have to admit that I didn't look 
too hard.



But I'm still interested how you got a collection of tar files with
unknown origin.  Just curious.


Easy: Just download the (source) distribution archives of a distribution
of choice and try to verify that the tarballs they use to compile their
packages actually come from the project's public git repositories.


OK, that's easier than calculating checksums and comparing them with 
those published by the respective projects, but also less trustworthy.



There are other reasons why an easily detectable git hash might be
useful.  For example, file(1) could show that the archive comes from
git.  Other utilities could use this to work around git-specific bugs.
An unpacker could add corresponding meta-data when unpacking the file.


file(1) could use the same heuristic as git get-tar-commit-id. 
Something like this would work (the first line is already shipped with 
file):


257 string  ustar\0 POSIX tar archive
>156 string  g
>>512 string  52\ comment=
>>>523 regex   [0-9a-f]{40}\b, git commit %s

NB: With Ian Darwin's file you need to use -e tar in order to turn off 
its internal tar test.


I'm very interested in hearing about any git specific bugs.


It would be much more useful if git created a
custom key. As per POSIX suggestions, something like this would be
appropriate:

 GIT.commit=57ca140635bf157354124e4e4b3c8e1bde2832f1


This would be included in addition to the comment in order to avoid
breaking existing users, I guess.


Good point.  I'm not sure how many user use the comment header at all.


Apart from git get-tar-commit-id I don't know any program for
extracting pax comments.  And I don't know how widely used that is,
but I assume there is *someone* out there, extracting commit IDs
with it.


Neither do I.  But remember, POSIX explicitly specifies that programs
that parse pax file must ignore pax comments so an unpacker that
interpretes the content of such a comment in any way is in violation of
the pax specification.


Almost right: The spec says that *pax* shall ignore comments.  Which is 
good -- we can use this field to transport anything without pax complaining.



If you have a random archive and want to know if it was generated by
git then your next question might be which options and substitutions
were used.  That reminds me of this thread regarding verifiable
archives:

 http://article.gmane.org/gmane.comp.version-control.git/240244


Good point.  Something like this should be enough to be enough to have
reproducable archives if archives with a tree ID were to have a time
stamp of 0 (1970-01-01) instead of the current date:

 comment=...(for compatibility)
 GIT.commit=... (like comment)
 GIT.umask=...  (tar.umask)
 GIT.prefix=... (--prefix=)
 GIT.path=...   (see below
 GIT.export-subst=1 (in extended header instead of global header)

A different key such as GIT.treeish might be appropriate.  The
GIT.export-subst key should be set only for those files where a
substitution has taken place.


What would GIT.export-subst contain? There can be multiple
replacements in a file.


GIT.export-subst would only contain a 1 if substitution is turned on.
The goal is to have reproduceable archives, not the ability to turn an
archive back into a git repository.


OK.


Maybe there should also be an
GIT.original-name key.


What would it be used for?


In case an export substition changes the file name so the implementation
can verify that the original file could plausibly have been substituted
into the current name.  Also for the case where multiple files
substitute into the same name to tell which file git should check
equivalency with.


Stupid question: Could you please provide an example?  The only 
possibility for name changes that I'm aware of is using --prefix.



An option GIT.export-ignore is not required.  Instead it would be more
useful to have a special file type G (for git) with the convention that
the file name .gitattributes means “attributes that apply to this git
archive.”


That would be a non-standard extension.  Archivers would extract
these as regular files.  Storing a list of excluded paths (in
GIT.exclude or so) might be a better idea.


No, that's not a good idea as pax headers are interpreted as “attributes
pertaining to a file.”  A file doesn't have the attribute that other
files have been 

Re: git show doesn't work on file names with square brackets

[Johannes Schindelin]

Hi Kirill,

On Sat, 6 Feb 2016, Kirill Likhodedov wrote:

> Is it a bug or I just didn’t find the proper way to escape the brackets? 
> 
> Steps to reproduce:
> 
> git init brackets
> cd brackets/
> echo ‘asd’ > bra[ckets].txt
> git add bra\[ckets\].txt
> git commit -m initial
> git show HEAD:bra[ckets].txt

This is expected behavior of the Bash you are using. The commands that I
think would reflect your intentions would be:

git init brackets
cd brackets
echo 'asd' > 'bra[ckets].txt'
git add 'bra[ckets].txt'
git commit -m initial
git show 'HEAD:bra[ckets].txt'

You could also escape the brackets with a backslash, as you did, but you
would have to do it *every* time you write the path, not just in the `git
add` incantation.

Ciao,
Johannes

[PATCH] Ignore generated test executable

[Johannes Schindelin]

In "mingw: fix t5601-clone.sh", this developer introduced a new test
executable, test-fake-ssh. Naturally, he forgot to update the .gitignore
file accordingly. This patch fixes that.

Signed-off-by: Johannes Schindelin 
---

This is on top of 'next', of course. My apologies that I did not
catch this earlier.

 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 1c2f832..5087ce1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -187,6 +187,7 @@
 /test-dump-cache-tree
 /test-dump-split-index
 /test-dump-untracked-cache
+/test-fake-ssh
 /test-scrap-cache-tree
 /test-genrandom
 /test-hashmap
-- 
2.7.0.windows.1.7.g55a05c8
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html