[PATCH] Documentation: build technical/multi-pack-index

[Todd Zullinger]

The git-multi-pack-index doc links to technical/multi-pack-index.html.
Ensure it is built to prevent a broken link.

Signed-off-by: Todd Zullinger 
---
While building 2.20.0-rc0 I noticed the broken link from
git-multi-pack-index to technical/multi-pack-index.html.

 Documentation/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 48d261dc2c..d5d936e6a7 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -73,6 +73,7 @@ TECH_DOCS += technical/hash-function-transition
 TECH_DOCS += technical/http-protocol
 TECH_DOCS += technical/index-format
 TECH_DOCS += technical/long-running-process-protocol
+TECH_DOCS += technical/multi-pack-index
 TECH_DOCS += technical/pack-format
 TECH_DOCS += technical/pack-heuristics
 TECH_DOCS += technical/pack-protocol
-- 
2.19.1

Re: [PATCH] t5551-http-fetch-smart.sh: sort cookies before comparing

[Todd Zullinger]

Jeff King wrote:
> On Fri, Sep 07, 2018 at 07:22:05PM -0400, Todd Zullinger wrote:
> 
>> With curl-7.61.1 cookies are sorted by creation-time¹.  Sort the output
>> used in the 'cookies stored in http.cookiefile when http.savecookies
>> set' test before comparing it to the expected cookies.
>> 
>> ¹ https://github.com/curl/curl/commit/e2ef8d6fa ("cookies: support
>>   creation-time attribute for cookies", 2018-08-28)
> 
> According to that commit message, the creation-time sort is only for
> cookies of the same length. But it's not clear to me if that just means
> on-the-wire, and curl always stores by creation-time in the cookie file.

Yeah, I didn't dig into the curl code deeply to try and
understand it.  I did test with the only the curl package
downgraded to 7.61.0 to confirm the test worked without
sorting.  And I saw that the curl commit updated existing
tests to sort the test data.

> Either way, though, I guess it wouldn't matter for us as long as we
> choose some arbitrary re-ordering for what curl produces (i.e., the
> output of `sort`) and then make sure our "expect" output is in the same
> order. Which is basically what your patch does. One question, though:
> 
>> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
>> index 771f36f9ff..538656bfef 100755
>> --- a/t/t5551-http-fetch-smart.sh
>> +++ b/t/t5551-http-fetch-smart.sh
>> @@ -215,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile 
>> when http.savecookies set
>>  git config http.cookiefile cookies.txt &&
>>  git config http.savecookies true &&
>>  git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
>> -tail -3 cookies.txt >cookies_tail.txt &&
>> +tail -3 cookies.txt | sort >cookies_tail.txt &&
>>  test_cmp expect_cookies.txt cookies_tail.txt
>>  '
> 
> We pick the bottom 3 before sorting. How do we know those are the three
> we want to see?
> 
> ...Ah, OK. The lines we are skipping are not actually cookies at all,
> but just header cruft. I wonder if:
> 
>   grep "^[^#]" cookies.txt
> 
> would be a better way of doing that, but that is certainly not something
> new.
> 
> So this fix looks fine. It might be worth a comment above the creation
> of expect_cookies.txt to mention it must be in sorted order (of course
> anybody modifying it would see a test failure).

I thought about running the expect_cookies.txt file through
sort as well.  That would ensure that both files were using
the same sorting.  Whether that's needed on any platform
now, I don't know.  Maybe that would be a useful way to
protect against future edits to the expect_cookies.txt file
catching the editor?

I thought there might be a test function to sort the output,
but I was (incorrectly) thinking of check_access_log() which
Gábor added in e8b3b2e275 ("t/lib-httpd: avoid occasional
failures when checking access.log", 2018-07-12).

Perhaps it would be useful to have a test_cmp_sorted() to do
the simple dance of sorting the actual & expected.  I
haven't looked through the tests to see how often such a
function might be useful.

>> The in-development version of Fedora updated to the recently
>> released curl-7.61.1 in the past few days.  This isn't
>> breakage from the 2.19.0 cycle, but if the fix looks good to
>> everyone it would be nice to include it.  That way other
>> distributions and users who update git and curl to the most
>> recent releases won't run into this test failure.
>> 
>> I tested this against Fedora 30 (curl-7.61.1) as well as
>> previous releases from RHEL/CentOS 6/7 (7.19.7/7.29.0) and
>> Fedora 27/28/29 (7.55.1/7.59.0/7.61.0).
> 
> You're pretty late in the 2.19 cycle, since the release is tentatively
> scheduled for Sunday. Though since this is just touching the test
> script, and since it looks Obviously Correct, I'm not opposed.

Yep, I knew the final was coming very soon.  I would not
have been surprised to see it land tonight while I was
finishing my testing of this patch. :)

I'm certainly covered for the Fedora packages.  It's hard to
say whether there are many other users/packagers who might
upgrade both git and curl and run into this.  So it may not
be worth even a small risk of making the change at this
point.

On the other hand, the change only affects one test and may
be safe enough to apply.  I'll leave that choice in the
capable hands of our maintainer and the good folks here.

Thanks for a thoughtful review, as always.

-- 
Todd
~~
That which seems the height of absurdity in one generation often
becomes the height of wisdom in the next.
-- John Stuart Mill (1806-1873)

[PATCH] t5551-http-fetch-smart.sh: sort cookies before comparing

[Todd Zullinger]

With curl-7.61.1 cookies are sorted by creation-time¹.  Sort the output
used in the 'cookies stored in http.cookiefile when http.savecookies
set' test before comparing it to the expected cookies.

¹ https://github.com/curl/curl/commit/e2ef8d6fa ("cookies: support
  creation-time attribute for cookies", 2018-08-28)

Signed-off-by: Todd Zullinger 
---
[Resending with the list in Cc; sorry for spamming you,
Junio, Jeff, and Gábor.]

The in-development version of Fedora updated to the recently
released curl-7.61.1 in the past few days.  This isn't
breakage from the 2.19.0 cycle, but if the fix looks good to
everyone it would be nice to include it.  That way other
distributions and users who update git and curl to the most
recent releases won't run into this test failure.

I tested this against Fedora 30 (curl-7.61.1) as well as
previous releases from RHEL/CentOS 6/7 (7.19.7/7.29.0) and
Fedora 27/28/29 (7.55.1/7.59.0/7.61.0).

The verbose output is:

expecting success:
git config http.cookiefile cookies.txt &&
git config http.savecookies true &&
git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
tail -3 cookies.txt >cookies_tail.txt &&
test_cmp expect_cookies.txt cookies_tail.txt
++ git config http.cookiefile cookies.txt
++ git config http.savecookies true
++ git ls-remote http://127.0.0.1:5551/smart_cookies/repo.git master
7ae89caac6c721f16555e981eaeed64abc165c5drefs/heads/master
263207bb5fbfbefbdf1c9c3fa4ae5d9663323217
refs/namespaces/ns/refs/heads/master
++ tail -3 cookies.txt
++ test_cmp expect_cookies.txt cookies_tail.txt
++ diff -u expect_cookies.txt cookies_tail.txt
--- expect_cookies.txt  2018-09-07 07:29:05.231532462 +
+++ cookies_tail.txt2018-09-07 07:29:05.306532366 +
@@ -1,3 +1,3 @@

-127.0.0.1  FALSE   /smart_cookies/ FALSE   0   othername   
othervalue
 127.0.0.1  FALSE   /smart_cookies/repo.git/info/   FALSE   0   name
value
+127.0.0.1  FALSE   /smart_cookies/ FALSE   0   othername   
othervalue
error: last command exited with $?=1
not ok 22 - cookies stored in http.cookiefile when http.savecookies set

 t/t5551-http-fetch-smart.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 771f36f9ff..538656bfef 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -215,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile when 
http.savecookies set
git config http.cookiefile cookies.txt &&
git config http.savecookies true &&
git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
-   tail -3 cookies.txt >cookies_tail.txt &&
+   tail -3 cookies.txt | sort >cookies_tail.txt &&
test_cmp expect_cookies.txt cookies_tail.txt
 '
 
-- 
2.19.0.rc2

Re: [RFC/PATCH] drop vcs-svn experiment

[Todd Zullinger]

Hi Jeff,

Jeff King wrote:
>  .gitignore |   1 -
>  Makefile   |  22 --
>  contrib/svn-fe/.gitignore  |   4 -
>  contrib/svn-fe/Makefile| 105 ---
>  contrib/svn-fe/svn-fe.c|  18 --
>  contrib/svn-fe/svn-fe.txt  |  71 -
>  contrib/svn-fe/svnrdump_sim.py |  68 -
>  remote-testsvn.c   | 337 
>  t/helper/test-line-buffer.c|  81 -
>  t/helper/test-svn-fe.c |  52 
>  t/t9020-remote-svn.sh  |  89 --

Doesn't t/t9010-svn-fe.sh also need to be removed?  It uses
the test-svn-fe helper which is removed.

The Fedora git-svn package has included git-remote-testsvn
for years now but no one has ever filed any bug reports
about it.  I looked at whether it should be packaged last
year.  I came to the conclusion that while it could be used
outside of the test suite it was doubtful it actually was.

-- 
Todd
~~
No one ever went broke underestimating the taste of the American
public.
-- H. L. Mencken

Re: [RFC PATCH 1/1] recover: restoration of deleted worktree files

[Todd Zullinger]

Hi,

Robert P. J. Day wrote:
> On Sat, 4 Aug 2018, Junio C Hamano wrote:
>> In other words, I think this patch can be a fine addition to
>> somebody else's project (i.e. random collection of scripts that may
>> help Git users), so let's see how I can offer comments/inputs to
>> help you improve it.  So I won't comment on lang, log message, or
>> shell scripting style---these are project convention and the
>> git-core convention won't be relevant to this patch.
> 
>   not sure how relevant this is, but fedora bundles a bunch of neat
> utilities into two packages: git-tools and git-extras. i have no idea
> what relationship those packages have to official git, or who decides
> what goes into them.

For anyone curious, those packages (git-extras and
git-tools) are both entirely separate projects upstream and
in the fedora packaging.  A git-recover script may well be a
good fit in one of those upstream projects.

The git-(extras|tools) package names are a bit confusing
IMO.  But it's probably more confusing that they each add a
number of git-* commands in the default PATH the way they're
packaged.

We do package some bits from contrib/ (e.g. completion,
subtree, etc.) in the fedora git packages.  We don't add
scripts and commands from outside of the git tarballs as
part of the fedora git package, though.

So far, I don't recall anyone filing a bug report about
commands from git-extras or git-tools against git.  So it
seems that users of those additional packages aren't being
confused, thankfully.

-- 
Todd
~~
Between two evils, I always pick the one I never tried before.
-- Mae West

Re: [PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs

[Todd Zullinger]

Jeff King wrote:
> We could get rid around that by using $(DOC_ETC_GITCONFIG) or something,
> with:
> 
>   DOC_ETC_GITCONFIG ?= $(ETC_GITCONFIG)
> 
> in the Makefile. But that still leaves the choice of which generic text
> to use up to the caller. Maybe we should provide more guidance.
> 
> I.e., provide a knob like DOC_GENERIC that fills in generic values for
> these values (and maybe some others; it sounds like we have some
> existing problem cases).

That sounds pretty reasonable.  I have something
implementing this below.

>> It might be enough if the default values are relatively sane
>> and consistent.  That would be a slight improvement over the
>> current situation still.
> 
> Yeah. Taking a step back from the implementation questions, I think
> "$(prefix)/etc/gitconfig" is not very helpful text. I'd be happy to see
> us come up with a generic way of saying that which is more
> comprehensible to end-users. Your patches side-step that by filling in
> the real value, but unfortunately we can't do that everywhere. :)
> 
> There may not be a good "token" value, though. I.e., we may need to have
> two sets of verbiage: the specific one, and the generic one.

Yeah.  About the best generic term I can come up with is
using '$sysconfdir'.  But I have no idea if that's something
most readers will recognize as a placeholder for something
like /etc.

A number of the path references are in the FILES sections of
the man pages.  It might not be much of an improvement if we
try to stuff too much text in those references.  Perhaps if
those used '$sysconfdir/gitconfig' a subsequent note could
expand on that?  It could even be wrapped in an ifdef
similar to that used for the default editor/pager.

I don't want to make the plain .txt files significantly less
readable in the process, of course.

>>> It's a shame we have to repeat this logic from the Makefile, though I
>>> guess we already do so for prefix, bindir, etc, so far.
>>> 
>>> Should we factor the path logic from the top-level Makefile into an
>>> include that can be used from either?
>> 
>> Yeah, maybe.  I didn't like the duplication either, but as
>> you noticed, we do it already for many of the other
>> variables.  I suspect we could put these defaults into
>> config.mak.uname which Documentation/Makefile includes and
>> then you could run 'make -C Documentation' in a fresh clone
>> or tarball and get docs built with the defaults set for each
>> platform.
> 
> I think it shouldn't go into config.mak.uname, since the idea there was
> to keep the long list of platform defaults separate from other logic
> (the Makefile is already long enough!). So I'm basically proposing the
> same thing but in its own file. :)

Ahh, something that the top-level Makefile would create and
then subdir Makefiles would include, perhaps similar to the
way GIT-VERSION-FILE is updated and included?  That could
prove useful to some of the tools in contrib which duplicate
logic.

Skipping that larger de-duplication cleanup, here's a stab
at implementing the DOC_GENERIC knob (and the DOC_ overrides
for ETC_GIT(CONFIG|ATTRIBUTES).  The defaults with
'/GENERIC-SYSCONFDIR' in them are just placeholders waiting
for a better name. :)

Similarly, the use of {etc-git(config|attributes)} as the
attribute for the replacements could likely be improved for
readers of the .txt files.  {system-wide-gitconfig} is
likely better.  Maybe the default for the generic paths
could be /system-wide/git(config|attributes) too (or in
CAPS to make it more obviously a placeholder)?

Thanks for thinking this through and providing some good
directions to work on!

-- >8 --
Subject: [PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs

Replace `$(prefix)/etc/gitconfig` and `$(prefix)/etc/gitattributes` in
generated documentation with the paths chosen when building.  Readers of
the documentation should not need to know how `$(prefix)` was defined.

It's also more consistent than sometimes using `$(prefix)/etc/gitconfig`
and other times using `/etc/gitconfig` to refer to the system-wide
config file.

Update the SYNOPSIS of gitattributes(5) to include the system-wide
config file as well.

Support building generic documentation with a DOC_GENERIC Makefile knob.
The default generic paths may be further customized via the
DOC_ETC_GITCONFIG and DOC_ETC_GITATTRIBUTES variables.

Define DOC_GENERIC in dist-doc make target to ensure generic paths are
used in the generated html and manpage tarballs.

Helped-by: Jeff King 
Signed-off-by: Todd Zullinger 
---
 Documentation/Makefile  | 22 ++
 Documentation/config.txt|  4 ++--
 Documentation/git-config.txt| 10 +-
 Documentation/git.txt   |  4 ++--
 Documentation/gitattributes.txt |  4 ++--
 Makefi

Re: [PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs

[Todd Zullinger]

Junio C Hamano wrote:
> Jeff King  writes:
> 
>> Specifically, I'm thinking of:
>>
>>   1. The pre-built documentation that Junio builds for
>>  quick-install-doc. This _could_ be customized during the "quick"
>>  step, but it's probably not worth the effort. However, we'd want
>>  some kind of generic fill-in then, and hopefully not
>>  "/home/jch/etc" or something.
> 
> That is very likely to happen, actually X-<.

Obviously, we don't want the end result to cause regressions
in the common case or any burden on you.  Would setting the
ETC_GIT(CONFIG|ATTRIBUTES) variables in the dist-doc target
alleviate that concern?

Alternately, we can make the default use generic paths and
require some other knob to enable substituting the actual
paths when building documentation.

I tend to think that the default should be to build
documentation that is accurate for that build, but since
it's something I'll set once for my package builds it's not
a big deal either way to me.

-- 
Todd
~~
Einstein argued that there must be simplified explanations of nature,
because God is not capricious or arbitrary. No such faith comforts the
software engineer.
-- Fred Brooks

Re: [PATCH 1/2] gitignore.txt: clarify default core.excludesfile path

[Todd Zullinger]

Junio C Hamano wrote:
> Todd Zullinger  writes:
> 
>> The default core.excludesfile path is $XDG_CONFIG_HOME/git/ignore.
>> $HOME/.config/git/ignore is used if XDG_CONFIG_HOME is empty or unset,
> 
> ... because $HOME/.config is the default value for XDG_CONFIG_HOME
> when it is unset, that is?  If so, the change makes sense.

Indeed, that's the fallback path.

Thanks,

-- 
Todd

Re: [PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs

[Todd Zullinger]

I wrote:
> Jeff King wrote:
>>  (Related, there's a build target in the local Makefile for using
>>  asciidoctor; does it need updated, too?)
> 
> I didn't test asciidoctor specficially, but it also respects
> the ASCIIDOC_EXTRA parameters, so I think it will work just
> as well.  I'll try to confirm that later today.

Testing confirmed that asciidoctor works fine with this as
well.

Somewhat tangentially, I looked at using asciidoctor for the
Fedora packages last year and one issue that kept me from
using it then was the '[FIXME: source]' it includes in the
footer of the manpage.  When I dug into it at the time, it
appeared this was due to no  declaration
(similarly missing for manual, and version).  It wasn't
clear whether it was possible to include a custom header
template in plain asciidoctor.  I got the impression that it
would require using a custom backend, which in turn required
the rubygem 'tilt' for processing.

I spent about an hour poking around with it and decided that
I'd put off building with asciidoctor until that was fixed.
I felt that displaying '[FIXME: source]' wass worse than
simply not including the version.

It's always possible that I was doing something wrong in my
use of asciidoctor (I just set USE_ASCIIDOCTOR).  Or maybe
the Fedora packages are missing some dependency which I
missed.

It might also be that we need some adjustments similar to
https://patchwork.kernel.org/patch/10360207/ to get the
mansource attribute passed on to asciidoctor.  I only just
ran across that patch and haven't had a chance to test
sometime similar in the git manpage build.  That looks
promising though.

-- 
Todd
~~
Why is it that we rejoice at a birth and grieve at a funeral?  It is
because we are not the person involved.
-- Mark Twain

Re: [PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs

[Todd Zullinger]

Jeff King wrote:
> On Wed, Jun 27, 2018 at 12:56:37AM -0400, Todd Zullinger wrote:
> 
>> Replace `$(prefix)/etc/gitconfig` and `$(prefix)/etc/gitattributes` in
>> generated documentation with the paths chosen when building.  Readers of
>> the documentation should not need to know how `$(prefix)` was defined.
> 
> Yes, I was just complaining about this yesterday.

So what you're saying is that if I had procrastinated a
little, you may have written such a patch for me? :)

>   Besides readers not
> having any clue what $(prefix) means here, $(prefix)/etc is not even
> correct for builds with prefix=/usr.
>
> So I like the overall direction here, but it leaves me with one
> question: what happens for documentation outside of customized builds?
> 
> Specifically, I'm thinking of:
> 
>   1. The pre-built documentation that Junio builds for
>  quick-install-doc. This _could_ be customized during the "quick"
>  step, but it's probably not worth the effort. However, we'd want
>  some kind of generic fill-in then, and hopefully not
>  "/home/jch/etc" or something.

If building docs separately for such a use, the values can
be overridden by setting ETC_GITCONFIG and ETC_GITATTRIBUTES
(or prefix or sysconfig).  To keep the same values as we
currently use, for example:

make -C Documentation V=1 \
ETC_GITCONFIG='$$(prefix)/etc/gitconfig' \
ETC_GITATTRIBUTES='$$(prefix)/etc/gitattributes'

I don't know if that's sufficient for folks who build
documentation to share with a general audience or not.

It might be enough if the default values are relatively sane
and consistent.  That would be a slight improvement over the
current situation still.

>   2. The manpages on git-scm.com, which are built with asciidoctor. I
>  think we'd want the same generic value there. Ideally it would be
>  embedded in the asciidoc source as "if this attribute isn't
>  defined, then use this", but it's not the end of the world to
>  require a patch to the site to handle this.

We have that for the DEFAULT_(EDITOR|PAGER).  I didn't know
if we'd want that here, but maybe it's worth the effort if
it helps when building docs destined for the website and
such.  It might make the plain text files a bit less
readable though.

The EDITOR/PAGER bits are in git-var.txt, like this:

GIT_EDITOR::
Text editor for use by Git commands.  The value is meant to be
interpreted by the shell when it is used.  Examples: `~/bin/vi`,
`$SOME_ENVIRONMENT_VARIABLE`, `"C:\Program Files\Vim\gvim.exe"
--nofork`.  The order of preference is the `$GIT_EDITOR`
environment variable, then `core.editor` configuration, then
`$VISUAL`, then `$EDITOR`, and then the default chosen at compile
time, which is usually 'vi'.
ifdef::git-default-editor[]
The build you are using chose '{git-default-editor}' as the default.
endif::git-default-editor[]

The ifdef would be a little different to set the var if it
was not set, of course.

I think if we ensure that ETC_GITCONFIG / ETC_GITATTRIBUTES
are set sanely by default (and easily overridden) then we
can probably avoid the need to handle it within the asciidoc
file though.  (There's more on that though below.)

>  (Related, there's a build target in the local Makefile for using
>  asciidoctor; does it need updated, too?)

I didn't test asciidoctor specficially, but it also respects
the ASCIIDOC_EXTRA parameters, so I think it will work just
as well.  I'll try to confirm that later today.

>> diff --git a/Documentation/Makefile b/Documentation/Makefile
>> index d079d7c73a..75af671798 100644
>> --- a/Documentation/Makefile
>> +++ b/Documentation/Makefile
>> @@ -95,6 +95,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
>>  
>>  prefix ?= $(HOME)
>>  bindir ?= $(prefix)/bin
>> +sysconfdir ?= $(prefix)/etc
>>  htmldir ?= $(prefix)/share/doc/git-doc
>>  infodir ?= $(prefix)/share/info
>>  pdfdir ?= $(prefix)/share/doc/git-doc
>> @@ -205,6 +206,18 @@ DEFAULT_EDITOR_SQ = $(subst ','\'',$(DEFAULT_EDITOR))
>>  ASCIIDOC_EXTRA += -a 'git-default-editor=$(DEFAULT_EDITOR_SQ)'
>>  endif
>>  
>> +ifndef ETC_GITCONFIG
>> +ETC_GITCONFIG = $(sysconfdir)/gitconfig
>> +endif
>> +ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
>> +ASCIIDOC_EXTRA += -a 'etc-gitconfig=$(ETC_GITCONFIG_SQ)'
>> +
>> +ifndef ETC_GITATTRIBUTES
>> +ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
>> +endif
>> +ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
>> +ASCIIDOC_EXTRA += -a 'etc-gitattributes=$(ETC_GITATTRIBUTES_SQ)'
>> +
> 
> It's a shame we have to repeat this logic fro

[PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs

[Todd Zullinger]

Replace `$(prefix)/etc/gitconfig` and `$(prefix)/etc/gitattributes` in
generated documentation with the paths chosen when building.  Readers of
the documentation should not need to know how `$(prefix)` was defined.

It's also more consistent than sometimes using `$(prefix)/etc/gitconfig`
and other times using `/etc/gitconfig` to refer to the system-wide
config file.

Update the SYNOPSIS of gitattributes(5) to include the system-wide
config file as well.

Signed-off-by: Todd Zullinger 
---
I noticed this while looking to update gitattributes.txt to
note the system-wide config file.  I tested with and without
RUNTIME_PREFIX as well as make gitattributes.5 from within
Documentation.  I believe all methods do the right thing.

I couldn't figure out a good way to have asciidoc expand the
attributes inside of a "`" literal, so I changed to the "+"
form.  There might be a better way to do this, passing subs=
in asciidoc.conf, but I wasn't clear on where that would
fit.  I tested with asciidoc and not asciidoctor.

 Documentation/Makefile  | 13 +
 Documentation/config.txt|  4 ++--
 Documentation/git-config.txt| 10 +-
 Documentation/git.txt   |  4 ++--
 Documentation/gitattributes.txt |  4 ++--
 5 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index d079d7c73a..75af671798 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -95,6 +95,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
 
 prefix ?= $(HOME)
 bindir ?= $(prefix)/bin
+sysconfdir ?= $(prefix)/etc
 htmldir ?= $(prefix)/share/doc/git-doc
 infodir ?= $(prefix)/share/info
 pdfdir ?= $(prefix)/share/doc/git-doc
@@ -205,6 +206,18 @@ DEFAULT_EDITOR_SQ = $(subst ','\'',$(DEFAULT_EDITOR))
 ASCIIDOC_EXTRA += -a 'git-default-editor=$(DEFAULT_EDITOR_SQ)'
 endif
 
+ifndef ETC_GITCONFIG
+ETC_GITCONFIG = $(sysconfdir)/gitconfig
+endif
+ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
+ASCIIDOC_EXTRA += -a 'etc-gitconfig=$(ETC_GITCONFIG_SQ)'
+
+ifndef ETC_GITATTRIBUTES
+ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
+endif
+ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
+ASCIIDOC_EXTRA += -a 'etc-gitattributes=$(ETC_GITATTRIBUTES_SQ)'
+
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =
 
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1cc18a828c..ed903b60bd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -5,7 +5,7 @@ The Git configuration file contains a number of variables that 
affect
 the Git commands' behavior. The `.git/config` file in each repository
 is used to store the configuration for that repository, and
 `$HOME/.gitconfig` is used to store a per-user configuration as
-fallback values for the `.git/config` file. The file `/etc/gitconfig`
+fallback values for the `.git/config` file. The file +{etc-gitconfig}+
 can be used to store a system-wide default configuration.
 
 The configuration variables are used by both the Git plumbing
@@ -2815,7 +2815,7 @@ configuration files (e.g. `$HOME/.gitconfig`).
 
 Example:
 
-/etc/gitconfig
+{etc-gitconfig}
   push.pushoption = a
   push.pushoption = b
 
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 18ddc78f42..0d5ea5b58e 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -114,10 +114,10 @@ See also <>.
 
 --system::
For writing options: write to system-wide
-   `$(prefix)/etc/gitconfig` rather than the repository
+   +{etc-gitconfig}+ rather than the repository
`.git/config`.
 +
-For reading options: read only from system-wide `$(prefix)/etc/gitconfig`
+For reading options: read only from system-wide +{etc-gitconfig}+
 rather than from all available files.
 +
 See also <>.
@@ -263,7 +263,7 @@ FILES
 If not set explicitly with `--file`, there are four files where
 'git config' will search for configuration options:
 
-$(prefix)/etc/gitconfig::
+{etc-gitconfig}::
System-wide configuration file.
 
 $XDG_CONFIG_HOME/git/config::
@@ -310,11 +310,11 @@ ENVIRONMENT
 GIT_CONFIG::
Take the configuration from the given file instead of .git/config.
Using the "--global" option forces this to ~/.gitconfig. Using the
-   "--system" option forces this to $(prefix)/etc/gitconfig.
+   "--system" option forces this to {etc-gitconfig}.
 
 GIT_CONFIG_NOSYSTEM::
Whether to skip reading settings from the system-wide
-   $(prefix)/etc/gitconfig file. See linkgit:git[1] for details.
+   {etc-gitconfig} file. See linkgit:git[1] for details.
 
 See also <>.
 
diff --git a/Documentation/git.txt b/Documentation/git.txt
index dba7f0c18e..6a4646f991 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -567,10 +567,10 @@ for further details.
 
 `GIT_CONFIG_NOSYSTEM`::
Whether to skip reading settings from the s

[PATCH 1/2] gitignore.txt: clarify default core.excludesfile path

[Todd Zullinger]

The default core.excludesfile path is $XDG_CONFIG_HOME/git/ignore.
$HOME/.config/git/ignore is used if XDG_CONFIG_HOME is empty or unset,
as described later in the document.

Signed-off-by: Todd Zullinger 
---
 Documentation/gitignore.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index ff5d7f9ed6..d107daaffd 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -7,7 +7,7 @@ gitignore - Specifies intentionally untracked files to ignore
 
 SYNOPSIS
 
-$HOME/.config/git/ignore, $GIT_DIR/info/exclude, .gitignore
+$XDG_CONFIG_HOME/git/ignore, $GIT_DIR/info/exclude, .gitignore
 
 DESCRIPTION
 ---
-- 
2.18.0

[PATCH 2/2] dir.c: fix typos in core.excludesfile comment

[Todd Zullinger]

Make it easier to find references to core.excludesfile and the default
$XDG_CONFIG_HOME/git/ignore path.

Signed-off-by: Todd Zullinger 
---
I noticed the typo in core.excludesfile and $XDG_CONFIG_HOME while I was
verifing the previous change to clarify the documentation matched the code.
Fixing these minor issues in the comments will hopefully make it easier for
others to find the right places in the code in the future.

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

diff --git a/dir.c b/dir.c
index fe9bf58e4c..363a4837ae 100644
--- a/dir.c
+++ b/dir.c
@@ -2497,7 +2497,7 @@ void setup_standard_excludes(struct dir_struct *dir)
 {
dir->exclude_per_dir = ".gitignore";
 
-   /* core.excludefile defaulting to $XDG_HOME/git/ignore */
+   /* core.excludesfile defaulting to $XDG_CONFIG_HOME/git/ignore */
if (!excludes_file)
excludes_file = xdg_config_home("ignore");
if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
-- 
2.18.0

Re: [PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Todd Zullinger]

Junio C Hamano wrote:
> Todd Zullinger  writes:
>> Or Junio may just squash this onto js/rebase-i-root-fix.
> 
> Nah, not for a hotfix on the last couple of days before the final.
> We'd need to build on top, not "squash".

Indeed.  I somehow missed that you'd merged and pushed the
changes to master and next when I set this.  I was
mistakenly thinking it was only on the js/rebase-i-root-fix
integration branch.

Thanks,

-- 
Todd

[PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Todd Zullinger]

When testing a reworded root commit, ensure that the squash-onto commit
which is created and amended is still the root commit.

Suggested-by: Phillip Wood 
Helped-by: Johannes Schindelin 
Signed-off-by: Todd Zullinger 
---
Hi Johannes,

Johannes Schindelin wrote:
>On Mon, 18 Jun 2018, Todd Zullinger wrote:
>> Phillip Wood wrote:
>>> On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote:
>>>>
>>>> From: Todd Zullinger 
>>>>
>>>> +test_expect_failure 'rebase -i --root reword root commit' '
>>>> +  test_when_finished "test_might_fail git rebase --abort" &&
>>>> +  git checkout -b reword-root-branch master &&
>>>> +  set_fake_editor &&
>>>> +  FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
>>>> +  git rebase -i --root &&
>>>> +  git show HEAD^ | grep "A changed"
>>>
>>> I wonder if it should also check that HEAD^ is the root commit, to make
>>> sure that the squash-onto commit that's created and then amended has
>>> been squashed onto.
>>
>> Hmm, is that something which other tests don't cover or an
>> issue that could affect 'rebase -i --root' with reword
>> differently than other 'rebase -i' commands?
>>
>> I admit I'm not well-versed in the rebase -i tests and I
>> focused only on creating a test which demonstrated the bug I
>> noticed.
>
> I think we should test this here, to make sure it is tested, and it should
> be as easy as:
>
>test -z "$(git show -s --format=%p HEAD^)"
>
> Hopefully you beat me to it, otherwise I will try to take care of this
> tomorrow.

With luck, this will save you a few minutes, assuming the
commit message is reasonable (or can be improved with help
from Phillip and others). :)

Or Junio may just squash this onto js/rebase-i-root-fix.

Thanks.

 t/t3404-rebase-interactive.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e500d7c320..352a52e59d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -977,7 +977,8 @@ test_expect_success 'rebase -i --root reword root commit' '
set_fake_editor &&
FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
git rebase -i --root &&
-   git show HEAD^ | grep "A changed"
+   git show HEAD^ | grep "A changed" &&
+   test -z "$(git show -s --format=%p HEAD^)"
 '
 
 test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on 
non-interactive rebase' '
-- 
Todd
~~
Anyone who is capable of getting themselves made President should on
no account be allowed to do the job.
-- Douglas Adams, "The Hitchhiker's Guide to the Galaxy"

Re: What's cooking in git.git (Jun 2018, #05; Mon, 18)

[Todd Zullinger]

Hi Junio,

Junio C Hamano wrote:
> * tz/cred-netrc-cleanup (2018-06-18) 3 commits
>  - git-credential-netrc: fix exit status when tests fail
>  - git-credential-netrc: use in-tree Git.pm for tests
>  - git-credential-netrc: minor whitespace cleanup in test script
> 
>  Build and test procedure for netrc credential helper (in contrib/)
>  has been updated.

It looks like 1/4 from that series didn't make it into the
tz/cred-netrc-cleanup branch: git-credential-netrc: make
"all" default target of Makefile, which is in
<20180613031040.3109-2-...@pobox.com>.

Thanks,

-- 
Todd
~~
I used to think the brain was the most advanced part of the body.
Then I realized, look what's telling me that.
-- Emo Phillips

Re: [PATCH 1/2] rebase --root: demonstrate a bug while amending rootcommit messages

[Todd Zullinger]

Hi Phillip,

Phillip Wood wrote:
> On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote:
>> 
>> From: Todd Zullinger 
>>  
>> +test_expect_failure 'rebase -i --root reword root commit' '
>> +test_when_finished "test_might_fail git rebase --abort" &&
>> +git checkout -b reword-root-branch master &&
>> +set_fake_editor &&
>> +FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
>> +git rebase -i --root &&
>> +git show HEAD^ | grep "A changed"
> 
> I wonder if it should also check that HEAD^ is the root commit, to make
> sure that the squash-onto commit that's created and then amended has
> been squashed onto.

Hmm, is that something which other tests don't cover or an
issue that could affect 'rebase -i --root' with reword
differently than other 'rebase -i' commands?

I admit I'm not well-versed in the rebase -i tests and I
focused only on creating a test which demonstrated the bug I
noticed.

-- 
Todd
~~
The average woman would rather be beautiful than smart because the
average man can see better than he can think.

Re: [PATCH 0/2] rebase --root: fix `reword` on a root commit

[Todd Zullinger]

Hi,

Junio C Hamano wrote:
> Todd Zullinger  writes:
> 
>> Hi Johannes,
>>
>> Johannes Schindelin via GitGitGadget wrote:
>>> From: GitGitGadget 
>>> 
>>> Todd Zullinger reported this bug in
>>> https://public-inbox.org/git/20180615043111.gs3...@zaya.teonanacatl.net/:
>>> when calling git rebase --root and trying to reword the
>>> root commit's message, a BUG is reported.
>>>
>>> This fixes that.
>>> 
>>> IMO the bug fix is trivial enough to qualify for inclusion into v2.18.0, 
>>> still.
>>
>> It does indeed fix the issue.  I agree it would be nice to
>> see it in 2.18.0.  As a fix for a minor regression
>> introduced in this cycle, that seems reasonable.
> 
> Offhand it is not clear from the proposed log message where the
> original breakage happened, but if this is to fix a regression
> between v2.17.0 and v2.18.0, then let's have it.  As -rc2 slipped
> for a few days, it is reasonable to delay the final by a couple of
> days as well, if only to give the last minute fixes and translators
> reasonable time to breathe.

Perhaps replacing the first paragraph with this would make
it clearer?

Since 21d0764c82 ("rebase -i --root: let the sequencer handle even the
initial part", 2018-05-04), when splitting a repository, running `git
rebase -i --root` to reword the initial commit, Git dies with

Alternately, a similar note could be added at the end.

This regression was recently introduced in 21d0764c82 ("rebase -i 
--root: let the sequencer handle even the initial part", 2018-05-04).

-- 
Todd

Re: [PATCH 0/2] rebase --root: fix `reword` on a root commit

[Todd Zullinger]

Hi Johannes,

Johannes Schindelin via GitGitGadget wrote:
> From: GitGitGadget 
> 
> Todd Zullinger reported this bug in
> https://public-inbox.org/git/20180615043111.gs3...@zaya.teonanacatl.net/:
> when calling git rebase --root and trying to reword the
> root commit's message, a BUG is reported.
>
> This fixes that.
> 
> IMO the bug fix is trivial enough to qualify for inclusion into v2.18.0, 
> still.

It does indeed fix the issue.  I agree it would be nice to
see it in 2.18.0.  As a fix for a minor regression
introduced in this cycle, that seems reasonable.

> Johannes Schindelin (1):
>   rebase --root: fix amending root commit messages
> 
> Todd Zullinger (1):
>   rebase --root: demonstrate a bug while amending root commit messages
> 
>  sequencer.c   | 2 +-
>  t/t3404-rebase-interactive.sh | 9 +
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> 
> base-commit: 68372c88794aba15f853542008cda39def768372

-- 
Todd
~~
I don't mean to sound cold, or cruel, or vicious,
but I am, so that's the way it comes out.
-- Bill Hicks

BUG: sequencer.c:795: root commit without message -- when rewording root commit

[Todd Zullinger]

Hi Johannes,

I was splitting a repository tonight and ran 'rebase -i
--root' to reword the initial commit. Then git died with
'BUG: sequencer.c:795: root commit without message.'

A simple test case to show the failure:

-- >8 --
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 59c766540..bc5e228b8 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -971,6 +971,14 @@ test_expect_success 'rebase -i --root fixup root commit' '
test 0 = $(git cat-file commit HEAD | grep -c ^parent\ )
 '
 
+test_expect_success 'rebase -i --root reword root commit' '
+   test_when_finished "test_might_fail git rebase --abort" &&
+   git checkout -b reword-root-branch master &&
+   set_fake_editor &&
+   FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" git rebase -i 
--root &&
+   git show HEAD^ | grep "A changed"
+'
+
 test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on 
non-interactive rebase' '
git reset --hard &&
git checkout conflict-branch &&
-- >8 --

Not surprisingly (among the commits which changed between
2.17.1 and 2.18.0-rc2, at least), git bisect points to
21d0764c82 ("rebase -i --root: let the sequencer handle even
the initial part", 2018-05-04).  With luck, the fix will be
obvious to trained eyes and can be added before 2.18.0. :)

Thanks,

-- 
Todd
~~
All decent people live beyond their incomes nowadays, and those who
aren't respectable live beyond other peoples'.
-- Saki

Re: [PATCH] git-credential-netrc: remove use of "autodie"

[Todd Zullinger]

Hi,

Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason   writes:
> 
>> Per my reading of the file this was the only thing autodie was doing
>> in this file (there was no other code it altered). So let's remove it,
>> both to fix the logic error and to get rid of the dependency.
>>
>> 1. <87efhfvxzu@evledraar.gmail.com>
>>(https://public-inbox.org/git/87efhfvxzu@evledraar.gmail.com/)
>> 2. 
>>
>> (https://public-inbox.org/git/cahqjxre8okskcck1aphahcclzhox+tzi8nnu2ra74rerx8s...@mail.gmail.com/)
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason 
>> ---
>>  contrib/credential/netrc/git-credential-netrc | 1 -
>>  1 file changed, 1 deletion(-)
> 
> Even though this may not be all that release-critical, let's queue
> it so that we do not have to remember to dig it up later ;-)
> 
> Thank you very much to all of you involved in the thread.

Should I resend the RFC v2 series as well?  The only change
I'd make would be to add my signoff on the patches which are
'From: Luis' per suggestion.

If you think that's worth adding, I can resend or you may
add it while queueing.  Whichever is easier for you works
for me.

Thanks,

-- 
Todd
~~
Age doesn't always bring wisdom.  Sometimes it comes alone.

Re: [RFC PATCH v2 2/4] git-credential-netrc: minor whitespace cleanup in test script

[Todd Zullinger]

Eric Sunshine wrote:
> On Tue, Jun 12, 2018 at 11:10 PM, Todd Zullinger  wrote:
>> Signed-off-by: Todd Zullinger 
>> ---
>> diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh 
>> b/contrib/credential/netrc/t-git-credential-netrc.sh
>> index 58191a62f8..c5661087fe 100755
>> --- a/contrib/credential/netrc/t-git-credential-netrc.sh
>> +++ b/contrib/credential/netrc/t-git-credential-netrc.sh
>> @@ -17,15 +17,15 @@
>> # set up test repository
>>
>> test_expect_success \
>> -'set up test repository' \
>> -'git config --add gpg.program test.git-config-gpg'
>> +   'set up test repository' \
>> +   'git config --add gpg.program test.git-config-gpg'
> 
> Since you're touching all the tests in this script anyhow, perhaps
> modernize them so the title and opening quote of the test body are on
> the same line as test_expect_success, and the closing body quote is on
> a line of its own?
> 
> test_expect_sucess 'setup test repository' '
> ...test body...
> '
> 
> I also changed "set up" to "setup" to follow existing practice.
> 
> (Not necessarily worth a re-roll.)

These tests were based on similar test_external tests which
use perl. like t0202 & t9700.  Both examples use the same
formatting (and use of 'set up').  Perhaps a later clean up
can adjust all three tests?

-- 
Todd
~~
How can I tell that the past isn't a fiction designed to account for
the discrepancy between my immediate physical sensation and my state
of mind?
-- Douglas Adams

[RFC PATCH v2 3/4] git-credential-netrc: use in-tree Git.pm for tests

[Todd Zullinger]

From: Luis Marsano 

The netrc test.pl script calls git-credential-netrc which imports the
Git module.  Pass GITPERLLIB to git-credential-netrc via PERL5LIB to
ensure the in-tree Git module is used for testing.

Signed-off-by: Luis Marsano 
---
 contrib/credential/netrc/t-git-credential-netrc.sh | 3 ++-
 contrib/credential/netrc/test.pl   | 1 -
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh 
b/contrib/credential/netrc/t-git-credential-netrc.sh
index c5661087fe..07227d0228 100755
--- a/contrib/credential/netrc/t-git-credential-netrc.sh
+++ b/contrib/credential/netrc/t-git-credential-netrc.sh
@@ -23,9 +23,10 @@
# The external test will outputs its own plan
test_external_has_tap=1
 
+   export PERL5LIB="$GITPERLLIB"
test_external \
'git-credential-netrc' \
-   perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl
+   perl "$GIT_BUILD_DIR"/contrib/credential/netrc/test.pl
 
test_done
 )
diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
index 1e1001030e..2b5280ad6a 100755
--- a/contrib/credential/netrc/test.pl
+++ b/contrib/credential/netrc/test.pl
@@ -1,5 +1,4 @@
 #!/usr/bin/perl
-use lib (split(/:/, $ENV{GITPERLLIB}));
 
 use warnings;
 use strict;

[RFC PATCH v2 4/4] git-credential-netrc: fix exit status when tests fail

[Todd Zullinger]

From: Luis Marsano 

Signed-off-by: Luis Marsano 
---
 contrib/credential/netrc/test.pl | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
index 2b5280ad6a..c0fb3718b2 100755
--- a/contrib/credential/netrc/test.pl
+++ b/contrib/credential/netrc/test.pl
@@ -11,7 +11,6 @@ BEGIN
# t-git-credential-netrc.sh kicks off our testing, so we have to go
# from there.
Test::More->builder->current_test(1);
-   Test::More->builder->no_ending(1);
 }
 
 my @global_credential_args = @ARGV;
@@ -103,6 +102,9 @@ BEGIN
 
 ok(scalar keys %$cred == 2, 'Got keys decrypted by command option');
 
+my $is_passing = eval { Test::More->is_passing };
+exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;
+
 sub run_credential
 {
my $args = shift @_;

[RFC PATCH v2 0/4] contrib/credential/netrc Makefile & test improvements

[Todd Zullinger]

This replaces my 2/2 "use in-tree Git.pm for tests" with
Luis's improved version.  It also adds Luis's fix to ensure
the proper exit status on test failures and a minor
whitespace cleanup.

Is it alright to forge your signoff Luis?

Luis Marsano (2):
  git-credential-netrc: use in-tree Git.pm for tests
  git-credential-netrc: fix exit status when tests fail

Todd Zullinger (2):
  git-credential-netrc: make "all" default target of Makefile
  git-credential-netrc: minor whitespace cleanup in test script

 contrib/credential/netrc/Makefile  | 3 +++
 contrib/credential/netrc/t-git-credential-netrc.sh | 9 +
 contrib/credential/netrc/test.pl   | 5 +++--
 3 files changed, 11 insertions(+), 6 deletions(-)

[RFC PATCH v2 1/4] git-credential-netrc: make "all" default target of Makefile

[Todd Zullinger]

Running "make" in contrib/credential/netrc should run the "all" target
rather than the "test" target.  Add an empty "all::" target like most of
our other Makefiles.

Signed-off-by: Todd Zullinger 
---
 contrib/credential/netrc/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/contrib/credential/netrc/Makefile 
b/contrib/credential/netrc/Makefile
index 0ffa407191..6174e3bb83 100644
--- a/contrib/credential/netrc/Makefile
+++ b/contrib/credential/netrc/Makefile
@@ -1,3 +1,6 @@
+# The default target of this Makefile is...
+all::
+
 test:
./t-git-credential-netrc.sh
 

[RFC PATCH v2 2/4] git-credential-netrc: minor whitespace cleanup in test script

[Todd Zullinger]

Signed-off-by: Todd Zullinger 
---
 contrib/credential/netrc/t-git-credential-netrc.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh 
b/contrib/credential/netrc/t-git-credential-netrc.sh
index 58191a62f8..c5661087fe 100755
--- a/contrib/credential/netrc/t-git-credential-netrc.sh
+++ b/contrib/credential/netrc/t-git-credential-netrc.sh
@@ -17,15 +17,15 @@
# set up test repository
 
test_expect_success \
-'set up test repository' \
-'git config --add gpg.program test.git-config-gpg'
+   'set up test repository' \
+   'git config --add gpg.program test.git-config-gpg'
 
# The external test will outputs its own plan
test_external_has_tap=1
 
test_external \
-'git-credential-netrc' \
-perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl
+   'git-credential-netrc' \
+   perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl
 
test_done
 )

Re: [RFC PATCH 0/2] contrib/credential/netrc Makefile & test improvements

[Todd Zullinger]

Hi,

Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Jun 10 2018, Todd Zullinger wrote:
> 
>>> I added 'use autodie;' without realizing it had external dependencies.
>>> According to the documentation
>>> http://perldoc.perl.org/autodie.html
>>> it's a pragma since perl 5.10.1
>>> Removing 'use autodie;' should be fine: it's not critical.
>>
>> I should clarify that part of why autodie isn't in my build
>> environment is that the Fedora and RHEL7+ perl packages
>> split out many modules which are shipped as part of the core
>> perl tarball.  So while all the platforms I care about have
>> perl >= 5.10.1, the Fedora and newer RHEL systems have the
>> autodie module in a separate package.
>>
>> That said, the INSTALL docs still suggest that we only
>> require perl >= 5.8, so if autodie is easily removed, that
>> would probably be a good plan.
> 
> The intent of those docs was and still is to say "5.8 and the modules it
> ships with".
> 
> This was discussed when 2.17.0 was released with my changes to make us
> unconditionally use Digest::MD5:
> https://public-inbox.org/git/87fu50e0ht@evledraar.gmail.com/
> 
> As noted there that's not some dogmatic "RedHat altered perl so we don't
> care about them", but rather that in practice this doesn't impact users
> negatively, so I think it's fine.

Yeah, that was my understanding.  I only included the
information on why it was missing from my build environment
despite having perl-5.10.1 was due to the Fedora/Red Hat
packaging.  I agree that any issues which fall out of those
packaging differences are on Fedora/Red Hat packagers to
fix.

(Which should all be relatively trivial, as the perl modules
contain a 'Provides: perl($module)'.  That allows dnf|yum
install 'perl(autodie)' to easily pull in the right package.
And the rpm perl dep generator creates a 'Requires:
perl(autodie)' when is sees 'use autodie'.)

Sorry if I muddied the conversation with that tangential
info. :)

>> Ævar brought up bumping the minimum supported perl to 5.10.0
>> last December, in <20171223174400.26668-1-ava...@gmail.com>
>> (https://public-inbox.org/git/20171223174400.26668-1-ava...@gmail.com/).
>> The general consensus seemed positive, but I don't think
>> it's happened.  Even so, that was 5.10.0, not the 5.10.1
>> which added autodie.
> 
> Right, this doesn't apply to autodie. Looking at
> https://www.cpan.org/ports/binaries.html there were a lot of releases
> that had 5.10.0, not *.1.
> 
> Also git-credential-netrc is in contrib, I don't know if that warrants
> treating it differently, I don't use it myself, and don't know how much
> it's "not really contrib" in practice (e.g. like the bash completion
> which is installed everywhere...)>

Indeed, that's a fine question.  All the platforms I care
about have 5.10.1 or newer, so either way works for me.

> But yeah, skimming the code it would be easy to remove the dependency.

I wrapped up the changes from Luis which replace my 2/2 "use
in-tree Git.pm for tests" and to ensure the tests exit
properly on failures.  I'll send those out now in the hope
that it saves a little effort in moving these minor fixes
forward.

As far as removing the autodie dep, is there anything more
to that than dropping the 'use autodie' line?  It looks like
doing so leaves us no worse than we were before, but I
haven't written any perl in a long time.

Thanks,

-- 
Todd
~~
Ocean, n. A body of water occupying about two-thirds of a world made
for man -- who has no gills.
-- Ambrose Bierce, "The Devil's Dictionary"

Re: [RFC PATCH 0/2] contrib/credential/netrc Makefile & test improvements

[Todd Zullinger]

Hi Luis,

Luis Marsano wrote:
> Thanks for looking into this and addressing these issues.

And thank you for digging further. :)

> On Thu, Jun 7, 2018 at 1:20 AM Todd Zullinger  wrote:
>> I noticed failures from the contrib/credential/netrc tests
>> while building 2.18.0 release candidates.  I was surprised
>> to see the tests being run when called with a simple 'make'
>> command.
>>
>> The first patch in the series adds an empty 'all::' make
>> target to match most of our other Makefiles and avoid the
>> surprise of running tests by default.  (When the netrc
>> helper was added to the fedora builds, it copied the same
>> 'make -C contrib/credential/...' pattern from other
>> credential helpers -- despite the lack of anything to
>> build.)
> 
> I think this is a good idea.
> 
>> The actual test failures were initially due to my build
>> environment lacking the perl autodie module, which was added
>> in 786ef50a23 ("git-credential-netrc: accept gpg option",
>> 2018-05-12).
> 
> I added 'use autodie;' without realizing it had external dependencies.
> According to the documentation
> http://perldoc.perl.org/autodie.html
> it's a pragma since perl 5.10.1
> Removing 'use autodie;' should be fine: it's not critical.

I should clarify that part of why autodie isn't in my build
environment is that the Fedora and RHEL7+ perl packages
split out many modules which are shipped as part of the core
perl tarball.  So while all the platforms I care about have
perl >= 5.10.1, the Fedora and newer RHEL systems have the
autodie module in a separate package.

That said, the INSTALL docs still suggest that we only
require perl >= 5.8, so if autodie is easily removed, that
would probably be a good plan.

Ævar brought up bumping the minimum supported perl to 5.10.0
last December, in <20171223174400.26668-1-ava...@gmail.com>
(https://public-inbox.org/git/20171223174400.26668-1-ava...@gmail.com/).
The general consensus seemed positive, but I don't think
it's happened.  Even so, that was 5.10.0, not the 5.10.1
which added autodie.

>> After installing the autodie module, the failures were due
>> to the build environment lacking a git install (specifically
>> the perl Git module).  The tests needing a pre-installed
>> perl Git seemed odd and worth fixing.
> 
> I mistakenly thought 'use lib (split(/:/, $ENV{GITPERLLIB}));' in
> test.pl handled this.
> Since it doesn't, and I was only following an example from
> t/t9700/test.pl that doesn't fit, this line should be removed and it
> might make more sense to set the environment from
> t-git-credential-netrc.sh near '. ./test-lib.sh', which also sets the
> environment.
> Something like
> 
> diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh
> b/contrib/credential/netrc/t-git-credential-netrc.sh
> index 58191a6..9e18611 100755
> --- a/contrib/credential/netrc/t-git-credential-netrc.sh
> +++ b/contrib/credential/netrc/t-git-credential-netrc.sh
> @@ -23,5 +23,6 @@
> # The external test will outputs its own plan
> test_external_has_tap=1
> 
> +   export PERL5LIB="$GITPERLLIB"
> test_external \
>  'git-credential-netrc' \
> 
> Your solution, however, is reasonable, and I don't know which is preferred.

I think your placement is better.  As you say, it could also
be placed closer to '. ./test-lib.sh'.

It doesn't come up very often, but I wonder if there's any
downside to having test-lib.sh export PERL5LIB?

> It looks like you found an issue with t/t9700/test.pl, too.
> When altered to fail, it first reports ok (then reports failed and
> returns non-0).
> 
> not ok 46 - unquote simple quoted path
> not ok 47 - unquote escape sequences
> 1..47
> # test_external test Perl API was ok
> # test_external_without_stderr test no stderr: Perl API failed: perl
> /home/luism/project/git/t/t9700/test.pl:
> $ echo $?
> 1

Oops.  Nice catch.  At least that does exit non-zero I
guess.

> To make git-credential-netrc tests behave correctly, I ended up making
> the changes below.
> They might be okay, unless someone knows better.
> 
> diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh
> b/contrib/credential/netrc/t-git-credential-netrc.sh
> index 58191a6..9e18611 100755
> --- a/contrib/credential/netrc/t-git-credential-netrc.sh
> +++ b/contrib/credential/netrc/t-git-credential-netrc.sh
> @@ -23,9 +23,10 @@
> # The external test will outputs its own plan
> test_external_has_tap=1
> 
> +   export PERL5LIB="$GITPERLLIB"
> test_external \
>  'git-credential-netrc' \
> -perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl
> +perl "$GIT_BUI

[RFC PATCH 0/2] contrib/credential/netrc Makefile & test improvements

[Todd Zullinger]

Hi,

I noticed failures from the contrib/credential/netrc tests
while building 2.18.0 release candidates.  I was surprised
to see the tests being run when called with a simple 'make'
command.

The first patch in the series adds an empty 'all::' make
target to match most of our other Makefiles and avoid the
surprise of running tests by default.  (When the netrc
helper was added to the fedora builds, it copied the same
'make -C contrib/credential/...' pattern from other
credential helpers -- despite the lack of anything to
build.)

The actual test failures were initially due to my build
environment lacking the perl autodie module, which was added
in 786ef50a23 ("git-credential-netrc: accept gpg option",
2018-05-12).

After installing the autodie module, the failures were due
to the build environment lacking a git install (specifically
the perl Git module).  The tests needing a pre-installed
perl Git seemed odd and worth fixing.

The second patch in the series aims to fix this.  I'm not
sure if there's a better or more preferable way to fix this,
which is one of the reasons for the RFC tag. (It's also why
I added you to the Cc Ævar, as you're one of the
knowledgeable perl folks here.)

The other reason for the RFC tag is that I'm unsure of how
to fix the last issue I found.  The tests exit cleanly even
when there are failures, which seems undesirable.  I'm not
familiar with the perl test_external framework to suggest a
fix in patch form.  It might be a matter of adding something
like this, from t/t9700/test.pl:

my $is_passing = eval { Test::More->is_passing };
exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;

?  But that's a wild guess which I haven't tested.

Here's the output from 'make test' showing that most tests
fail and we still get a clean exit status:

$ make -C contrib/credential/netrc test ; echo "netrc test exit status: $?"
make: Entering directory 
'/builddir/build/BUILD/git-2.18.0.rc1/contrib/credential/netrc'
./t-git-credential-netrc.sh
ok 1 - set up test repository
# run 1: git-credential-netrc (perl 
/builddir/build/BUILD/git-2.18.0.rc1/t/../contrib/credential/netrc/test.pl)
ok 2 - Got 0 keys from insecure file
ok 3 - Got 0 keys from missing file
not ok 4 - Got first found keys with bad data
ok 5 - Got no corovamilkbar keys
not ok 6 - Got 2 Github keys
not ok 7 - Got correct Github password
not ok 8 - Got correct Github username
not ok 9 - Got 2 username-specific keys
not ok 10 - Got correct user-specific password
not ok 11 - Got correct user-specific protocol
not ok 12 - Got 2 host:port-specific keys
not ok 13 - Got correct host:port-specific password
not ok 14 - Got correct host:port-specific username
not ok 15 - Got 2 'host:port kills host' keys
not ok 16 - Got correct 'host:port kills host' password
not ok 17 - Got correct 'host:port kills host' username
not ok 18 - Got keys decrypted by git config option
not ok 19 - Got keys decrypted by command option
# test_external test git-credential-netrc was ok
make: Leaving directory 
'/builddir/build/BUILD/git-2.18.0.rc1/contrib/credential/netrc'
netrc test exit status: 0

Todd Zullinger (2):
  git-credential-netrc: make "all" default target of Makefile
  git-credential-netrc: use in-tree Git.pm for tests

 contrib/credential/netrc/Makefile | 3 +++
 contrib/credential/netrc/test.pl  | 3 +++
 2 files changed, 6 insertions(+)

Thanks all.

-- 
2.18.0.rc1

[RFC PATCH 1/2] git-credential-netrc: make "all" default target of Makefile

[Todd Zullinger]

Running "make" in contrib/credential/netrc should run the "all" target
rather than the "test" target.  Add an empty "all::" target like most of
our other Makefiles.

Signed-off-by: Todd Zullinger 
---
 contrib/credential/netrc/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/contrib/credential/netrc/Makefile 
b/contrib/credential/netrc/Makefile
index 0ffa407191..6174e3bb83 100644
--- a/contrib/credential/netrc/Makefile
+++ b/contrib/credential/netrc/Makefile
@@ -1,3 +1,6 @@
+# The default target of this Makefile is...
+all::
+
 test:
./t-git-credential-netrc.sh
 
-- 
2.18.0.rc1

[RFC PATCH 2/2] git-credential-netrc: use in-tree Git.pm for tests

[Todd Zullinger]

The netrc test.pl script calls git-credential-netrc which imports the
Git module.  Pass GITPERLLIB to git-credential-netrc via PERL5LIB to
ensure the in-tree Git module is used for testing.

Signed-off-by: Todd Zullinger 
---
 contrib/credential/netrc/test.pl | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
index 1e1001030e..5e26b4d190 100755
--- a/contrib/credential/netrc/test.pl
+++ b/contrib/credential/netrc/test.pl
@@ -27,6 +27,9 @@ BEGIN
   ? $ENV{PATH}
   : ();
 
+# use in-tree Git.pm
+local $ENV{PERL5LIB} = $ENV{GITPERLLIB};
+
 diag "Testing insecure file, nothing should be found\n";
 chmod 0644, $netrc;
 my $cred = run_credential(['-f', $netrc, 'get'],
-- 
2.18.0.rc1

Re: [PATCH v7 2/2] json-writer: t0019: add Python unit test

[Todd Zullinger]

Ramsay Jones wrote:
[...]
> I don't run the p4 or svn tests, so ... :-D

Heh, lucky you. :)

I try to run them all as part of the fedora builds since
they cover much more than I'd ever use.  That's the main
reason I noticed the bare python.  That would trip me up
when it came time to build on a near-future fedora where
python isn't installed by default and I only wanted to
require python3 for the build/runtime scripts.

> On 06/06/18 22:03, Jeff King wrote:
>> really the only user in the whole code base outside of a few fringe
>> commands). Leaving aside any perl vs python flame-war, I think there's
>> value in keeping the number of languages limited when there's not a
>> compelling reason to do otherwise.
> 
> I agree that fewer languages is (generally) a good idea.

Yep, that's certainly even better and if Jeff H. can use
perl relatively easily (or one of the many perl folks here
can help with that part of the test), that's great.  The
best way to solve many problems is avoid having them. :)

Thanks,

-- 
Todd
~~
Chaos, panic, and disorder - my job is done here.

Re: git rm bug

[Todd Zullinger]

Thomas Fischer wrote:
> I agree that the entire chain of empty directories should not be tracked, as 
> git tracks content, not files.
> 
> However, when I run 'rm path/to/some/file', I expect path/to/some/ to still 
> exist.
> 
> Similarly, when I run 'git rm path/to/some/file', I expect path/to/some/ to 
> exist, *albeit untracked*.
> 
> I do NOT expect git to *track* empty directories. But I also do NOT expect it 
> to remove untracked directories.

It looks like this behavior has been in place for many
years, since d9b814cc97 ("Add builtin "git rm" command",
2006-05-19).  Interestingly, Linus noted in the commit
message that the removal of leading directories was
different than when git-rm was a shell script.  And he
wondered if it might be worth having an option to control
that behavior.

I imagine that most users either want the current behavior
or they rarely run across this and are surprised, given how
long git rm has worked this way.

It does seem like something which could be noted in the git
rm docs.  Perhaps you'd care to take a stab at a patch to
add a note to Documentation/git-rm.txt Thomas?  Maybe a note
at the end of the DISCUSSION section?

-- 
Todd
~~
If everything seems to be going well, you have obviously overlooked
something.

Re: [PATCH v7 2/2] json-writer: t0019: add Python unit test

[Todd Zullinger]

g...@jeffhostetler.com wrote:
> +# As a sanity check, ask Python to parse our generated JSON.  Let Python
> +# recursively dump the resulting dictionary in sorted order.  Confirm that
> +# that matches our expectations.
> +test_expect_success PYTHON 'parse JSON using Python' '
[...]
> + python "$TEST_DIRECTORY"/t0019/parse_json_1.py actual &&

Would this be better using $PYTHON_PATH rather than
hard-coding python as the command?

-- 
Todd
~~
Sometimes I think I understand everything, then I regain
consciousness.

Re: 2.17.0 Regression When Adding Patches Without Whitespace In Initial Column

[Todd Zullinger]

Hi Jeff,

Jeff Felchner wrote:
> Ever since 2.17.0, when saving a patch (using add --patch
> but probably other ways as well), if the whitespace is
> removed from the initial column, the patch doesn't apply.

This sounds a bit like the issue discussed in this thread a
few weeks ago:

https://public-inbox.org/git/e8aedc6b-5b3e-cfb2-be9d-971bfd9ad...@talktalk.net/

But I didn't download or watch the video, so I'm not positive.

> Full walkthrough (including comparison with 2.16.3) in the
> video attached to this link:
> 
> https://www.dropbox.com/s/s1ophi4mwmf9ogv/git-add-patch-whitespace-bug.mp4?dl=1

-- 
Todd
~~
Everybody knows how to raise children, except the people who have
them.
-- P.J. O'Rourke

Re: [PATCH] packfile: Correct zlib buffer handling

[Todd Zullinger]

Jeremy Linton wrote:
> The buffer being passed to zlib includes a null terminator that
> git needs to keep in place. unpack_compressed_entry() attempts to
> detect the case that the source buffer hasn't been fully consumed
> by checking to see if the destination buffer has been over consumed.
> 
> This yields two problems, first a single byte overrun won't be detected
> properly because the Z_STREAM_END will then be set, but the null
> terminator will have been overwritten. The other problem is that
> more recent zlib patches have been poisoning the unconsumed portions
> of the buffers which also overwrites the null, while correctly
> returning length and status.
> 
> Lets rely on the fact that the source buffer will only be fully
> consumed when the when the destination buffer is inflated to the
> correct size. We can do this by passing zlib the correct buffer size
> and properly checking the return status. The latter check actually
> already exists if the buffer size is correct.
> 
> Signed-off-by: Jeremy Linton 
> ---

As a little background, earlier today Pavel Cahyna filed a
ticket about a regression in a recent zlib update on aarch64
in Fedora[1].

While he was doing that I was just beginning to look at why
the git test suite failed fairly badly a build last
night[2].  The aarch64 build on Fedora 28 failed, while it
succeeded on all other architectures (armv7hl, i686, ppc64,
ppc64le, s390x, x86_64).  It also passed on newer and older
Fedora releases.

The difference was that the Fedora 28 zlib build has some
aarch64 optimization patches applied[3].  (Those patches are
in rawhide/f29 as well, but due to an unrelated issue have
not yet made it into the buildroot used for the git build.)

I'm woefully unqualified to comment on the patch, but if
there are any questions about how this was found, I hope the
above background will be helpful.

A big thanks to Jeremy for dropping whatever he had planned
to do today and dig into this issue.  I can only hope it was
either more fun or less work than what he hoped to do with
his Friday. :)

Thanks also to Pavel for finding the source of the failures
and filing a detailed bug report to get things moving.

[1] https://bugzilla.redhat.com/1582555
[2] https://fedorapeople.org/~tmz/git-aarch64-make-test
[3] https://src.fedoraproject.org/rpms/zlib/c/25e9802

>  packfile.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/packfile.c b/packfile.c
> index 7c1a2519f..245eb3204 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct packed_git 
> *p,
>   return NULL;
>   memset(, 0, sizeof(stream));
>   stream.next_out = buffer;
> - stream.avail_out = size + 1;
> + stream.avail_out = size;
>  
>   git_inflate_init();
>   do {
> @@ -1424,7 +1424,7 @@ static void *unpack_compressed_entry(struct packed_git 
> *p,
>   stream.next_in = in;
>   st = git_inflate(, Z_FINISH);
>   if (!stream.avail_out)
> - break; /* the payload is larger than it should be */
> + break; /* done, st indicates if source fully consumed */
>   curpos += stream.next_in - in;
>   } while (st == Z_OK || st == Z_BUF_ERROR);
>   git_inflate_end();

-- 
Todd
~~
An election is coming. Universal peace is declared and the foxes have
a sincere interest in prolonging the lives of the poultry.
-- T.S. Eliot

Re: [PATCH v2] rev-parse: check lookup'ed commit references for NULL

[Todd Zullinger]

[Added Florian to Cc]

Elijah Newren wrote:
> Commits 2122f8b963d4 ("rev-parse: Add support for the ^! and ^@ syntax",
> 2008-07-26) and 3dd4e7320d ("Teach rev-parse the ... syntax.", 2006-07-04)
> taught rev-parse new syntax, and used lookup_commit_reference() as part of
> their logic.  Neither usage checked the returned commit to see if it was
> non-NULL before using it.  Check for NULL and ensure an appropriate error
> is reported to the user.
> 
> Reported by Florian Weimer and Todd Zullinger.
> 
> Helped-by: Jeff King <p...@peff.net>
> Signed-off-by: Elijah Newren <new...@gmail.com>
> ---

The output is now much more consistent with other invalid
input.  The only (minor) difference I noticed was when using
the fff...fff form.  With exactly 40 chars, rev-parse prints
both refs separately and then the full input string before
the "fatal:" error.  I doubt it's terribly important.

# exactly 40 chars
$ ./git-rev-parse 
...


...
fatal: ambiguous argument 
'...':
 unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git  [...] -- [...]'

# not 40 chars
$ ./git-rev-parse 
fff...fff
fff...fff
fatal: ambiguous argument 
'fff...fff':
 unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git  [...] -- [...]'

> I would have used a Reported-by tag for Florian and Todd, but looking at
> the bugzilla.redhat.com bug report doesn't show me Florian's email
> address.  I grepped through git logs and found two associated with that
> name, but didn't know if they were still accurate, or were a different
> Florian.  So I just went with the sentence instead.

I added Florian to Cc, in case he wants to provide a
preferred address.  (The Red Hat Bugzilla only shows
email addresses if you're logged in.)

Thanks Elijah and Peff.

>  builtin/rev-parse.c  | 8 ++--
>  t/t6101-rev-parse-parents.sh | 8 
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index a1e680b5e9..a0a0ace38d 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -282,6 +282,10 @@ static int try_difference(const char *arg)
>   struct commit *a, *b;
>   a = lookup_commit_reference(_oid);
>   b = lookup_commit_reference(_oid);
> + if (!a || !b) {
> + *dotdot = '.';
> + return 0;
> + }
>   exclude = get_merge_bases(a, b);
>   while (exclude) {
>   struct commit *commit = pop_commit();
> @@ -328,12 +332,12 @@ static int try_parent_shorthands(const char *arg)
>   return 0;
>  
>   *dotdot = 0;
> - if (get_oid_committish(arg, )) {
> + if (get_oid_committish(arg, ) ||
> + !(commit = lookup_commit_reference())) {
>   *dotdot = '^';
>   return 0;
>   }
>  
> - commit = lookup_commit_reference();
>   if (exclude_parent &&
>   exclude_parent > commit_list_count(commit->parents)) {
>   *dotdot = '^';
> diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
> index 8c617981a3..7683e4a114 100755
> --- a/t/t6101-rev-parse-parents.sh
> +++ b/t/t6101-rev-parse-parents.sh
> @@ -214,4 +214,12 @@ test_expect_success 'rev-list merge^-1x (garbage after 
> ^-1)' '
>   test_must_fail git rev-list merge^-1x
>  '
>  
> +test_expect_success 'rev-parse $garbage^@ does not segfault' '
> + test_must_fail git rev-parse $EMPTY_TREE^@
> +'
> +
> +test_expect_success 'rev-parse $garbage...$garbage does not segfault' '
> + test_must_fail git rev-parse $EMPTY_TREE...$EMPTY_BLOB
> +'
> +
>  test_done

-- 
Todd
~~
If the triangles were to make a God they would give him three sides.
-- Montesquieu

Re: [PATCH 2/2] rev-parse: verify that commit looked up is not NULL

[Todd Zullinger]

I wrote:
> Thanks.  This fixes the segfault.  While I was testing this,
> I wondered if the following cases should differ:

Nevermind me.  Jeff beat me to a reply and included much
more useful details about why this occurs and suggestions
for fixing it. :)

> #  f*40
> $ ./git-rev-parse ^@ ; echo $?
> 0
> 
> #  f*39
> $ ./git-rev-parse fff^@ ; echo $?
> fff^@
> fatal: ambiguous argument 'fff^@': 
> unknown revision or path not in the working tree.
> Use '--' to separate paths from revisions, like this:
> 'git  [...] -- [...]'
> 128
> 
> Looking a little further, this is deeper than the rev-parse
> handling.  The difference in how these invalid refs are
> handled appears in 'git show' as well.  With 'git show' a
> (different) fatal error is returned in both cases.
> 
> #  f*40
> $ git show 
> fatal: bad object 
> 
> #  39*f
> $ git show fff
> fatal: ambiguous argument 'fff': unknown 
> revision or path not in the working tree.
> Use '--' to separate paths from revisions, like this:
> 'git  [...] -- [...]'
> 
> Should rev-parse return an error as well, rather than
> silenty succeeding?

-- 
Todd
~~
How can I tell that the past isn't a fiction designed to account for
the discrepancy between my immediate physical sensation and my state
of mind?
-- Douglas Adams

Re: [PATCH 2/2] rev-parse: verify that commit looked up is not NULL

[Todd Zullinger]

Elijah Newren wrote:
> In commit 2122f8b963d4 ("rev-parse: Add support for the ^! and ^@ syntax",
> 2008-07-26), try_parent_shorthands() was introduced to parse the special
> ^! and ^@ syntax.  However, it did not check the commit returned from
> lookup_commit_reference() before proceeding to use it.  If it is NULL,
> bail early and notify the caller that this cannot be a valid revision
> range.

Thanks.  This fixes the segfault.  While I was testing this,
I wondered if the following cases should differ:

#  f*40
$ ./git-rev-parse ^@ ; echo $?
0

#  f*39
$ ./git-rev-parse fff^@ ; echo $?
fff^@
fatal: ambiguous argument 'fff^@': unknown 
revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git  [...] -- [...]'
128

Looking a little further, this is deeper than the rev-parse
handling.  The difference in how these invalid refs are
handled appears in 'git show' as well.  With 'git show' a
(different) fatal error is returned in both cases.

#  f*40
$ git show 
fatal: bad object 

#  39*f
$ git show fff
fatal: ambiguous argument 'fff': unknown 
revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git  [...] -- [...]'

Should rev-parse return an error as well, rather than
silenty succeeding?

-- 
Todd
~~
I refuse to spend my life worrying about what I eat. There is no
pleasure worth foregoing just for an extra three years in the
geriatric ward.
-- John Mortimer

Re: BUG: rev-parse segfault with invalid input

[Todd Zullinger]

Hi,

Elijah Newren wrote:
> Thanks for the detailed report.  This apparently goes back to
> git-1.6.0 with commit 2122f8b963d4 ("rev-parse: Add support for the ^!
> and ^@ syntax", 2008-07-26).  We aren't checking that the commit from
> lookup_commit_reference() is non-NULL before proceeding.  Looks like
> it's simple to fix.  I'll send a patch shortly...

Thanks Elijah!  I thought it was likely to be a simple fix.
But I also don't know the area well and that kept me from
being too ambitious about suggesting a fix or the difficulty
of one. :)

-- 
Todd
~~
I believe in the noble, aristocratic art of doing absolutely nothing.
And someday, I hope to be in a position where I can do even less.

BUG: rev-parse segfault with invalid input

[Todd Zullinger]

Hi,

Certain invalid input causes git rev-parse to crash rather
than return a 'fatal: ambiguous argument ...' error.

This was reported against the Fedora git package:

https://bugzilla.redhat.com/1581678

Simple reproduction recipe and analysis, from the bug:

$ git init
Initialized empty Git repository in /tmp/t/.git/
$ git rev-parse ^@
Segmentation fault (core dumped)

gdb) break lookup_commit_reference
Breakpoint 1 at 0x55609f00: lookup_commit_reference. (3 locations)
(gdb) r
Starting program: /usr/bin/git rev-parse 
\^@
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Breakpoint 1, lookup_commit_reference (oid=oid@entry=0x7fffd550) at 
commit.c:34
34  return lookup_commit_reference_gently(oid, 0);
(gdb) finish
Run till exit from #0  lookup_commit_reference 
(oid=oid@entry=0x7fffd550) at commit.c:34
try_parent_shorthands (arg=0x7fffdd44 'f' ) at 
builtin/rev-parse.c:314
314 include_parents = 1;
Value returned is $1 = (struct commit *) 0x0
(gdb) c

(gdb) c
Continuing.

Program received signal SIGSEGV, Segmentation fault.
try_parent_shorthands (arg=0x7fffdd44 'f' ) at 
builtin/rev-parse.c:345
345 for (parents = commit->parents, parent_number = 1;
(gdb) l 336,+15
336 commit = lookup_commit_reference();
337 if (exclude_parent &&
338 exclude_parent > commit_list_count(commit->parents)) {
339 *dotdot = '^';
340 return 0;
341 }
342 
343 if (include_rev)
344 show_rev(NORMAL, , arg);
345 for (parents = commit->parents, parent_number = 1;
346  parents;
347  parents = parents->next, parent_number++) {
348 char *name = NULL;
349 
350 if (exclude_parent && parent_number != 
exclude_parent)
351 continue;

Looks like a null pointer check is missing.

This occurs on master and as far back as 1.8.3.1 (what's in
RHEL-6, I didn't try to test anything older).  Only a string
with 40 valid hex characters and ^@, @-, of ^!  seems to
trigger it.

-- 
Todd
~~
I don't mind arguing with myself. It's when I lose that it bothers me.
-- Richard Powers

Re: [PATCH v2] completion: reduce overhead of clearing cached --options

[Todd Zullinger]

Hi Matthew,

Matthew Coleman wrote:
> I haven't seen any discussion about this recently. What
> are the chances of getting it merged? I'd like to see this
> included in 2.18.

Junio's last few "What's cooking" updates have mentioned it:

> * sg/completion-clear-cached (2018-04-18) 1 commit
>   (merged to 'next' on 2018-04-25 at 9178da6c3d)
>  + completion: reduce overhead of clearing cached --options
> 
>  The completion script (in contrib/) learned to clear cached list of
>  command line options upon dot-sourcing it again in a more efficient
>  way.
> 
>  Will merge to 'master'.

I imagine it will show up on master sometime soon, in time
for 2.18.0.

-- 
Todd
~~
I expected times like this -- but never thought they'd be so bad, so
long, and so frequent.
-- Demotivators (www.despair.com)

Re: [PATCH v2 12/18] branch-diff: use color for the commit pairs

[Todd Zullinger]

Hi Johannes,

Johannes Schindelin wrote:
> Hi Todd,
> 
> On Sat, 5 May 2018, Todd Zullinger wrote:
> 
>>> @@ -430,6 +451,8 @@ int cmd_branch_diff(int argc, const char **argv, const 
>>> char *prefix)
>>> struct string_list branch1 = STRING_LIST_INIT_DUP;
>>> struct string_list branch2 = STRING_LIST_INIT_DUP;
>>>  
>>> +   git_diff_basic_config("diff.color.frag", "magenta", NULL);
>>> +
>>> diff_setup();
>>> diffopt.output_format = DIFF_FORMAT_PATCH;
>>> diffopt.flags.suppress_diff_headers = 1;
>> 
>> Should this also (or only) check color.diff.frag?
> 
> This code is not querying diff.color.frag, it is setting it. Without
> any way to override it.
> 
> Having thought about it longer, and triggered by Peff's suggestion to
> decouple the "reverse" part from the actual color, I fixed this by
> 
> - *not* setting .frag to magenta,
> 
> - using the reverse method also to mark outer *hunk headers* (not only the
>   outer -/+ markers).
> 
> - actually calling git_diff_ui_config()...

Excellent.  That seems to work nicely now, respecting the
color.diff. config.

> The current work in progress can be pulled as `branch-diff` from
> https://github.com/dscho/git, if I could ask you to test?

While the colors and 'branch --diff' usage seem to work
nicely, I found that with 4ac3413cc8 ("branch-diff: left-pad
patch numbers", 2018-05-05), 'git branch' itself is broken.

Running 'git branch' creates a branch named 'branch'.
Calling 'git branch --list' shows only 'branch' as the only
branch.

I didn't look too closely, but I'm guessing that the argv
handling is leaving the 'branch' argument in place where it
should be stripped?

This unsurprisingly breaks a large number of tests. :)

Thanks,

-- 
Todd
~~
A common mistake people make when trying to design something
completely foolproof is to underestimate the ingenuity of complete
fools.
-- Douglas Adams

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Todd Zullinger]

I wrote:
> Would it be possible and reasonable to teach 'git branch' to
> call this as a subcommand, i.e. as 'git branch diff'?  Then
> the completion wouldn't offer git branch-diff.

Of course right after I sent this, it occurred to me that
'git branch diff' would make mask the ability to create a
branch named diff.  Using 'git branch --diff ...' wouldn't
suffer that problem.

It does add a bit more overhead to the 'git branch' command,
in terms of documentation and usage.  I'm not sure it's too
much though.  The git-branch summary wouldn't change much:

-git-branch - List, create, or delete branches
+git-branch - List, create, delete, or diff branches

I hesitate to hit send again, in case I'm once again
overlooking a glaringly obvious problem with this idea. ;)

-- 
Todd
~~
Quick to judge, quick to anger, slow to understand.
Ignorance and prejudice and fear walk hand in hand.

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Todd Zullinger]

Hi Johannes,

Johannes Schindelin wrote:
> On Sat, 5 May 2018, Jeff King wrote:
>> One minor point about the name: will it become annoying as a tab
>> completion conflict with git-branch?
> 
> I did mention this in the commit message of 18/18:
> 
> Without this patch, we would only complete the `branch-diff` part but
> not the options and other arguments.
> 
> This of itself may already be slightly disruptive for well-trained
> fingers that assume that `git braorimas` would expand to
> `git branch origin/master`, as we now no longer automatically append a
> space after completing `git branch`: this is now ambiguous.
> 
>> It feels really petty complaining about the name, but I just want to
>> raise the point, since it will never be easier to change than right now.
> 
> I do hear you. Especially since I hate `git cherry` every single time that
> I try to tab-complete `git cherry-pick`.
> 
>> (And no, I don't really have another name in mind; I'm just wondering if
>> "subset" names like this might be a mild annoyance in the long run).
> 
> They totally are, and if you can come up with a better name, I am really
> interested in changing it before this hits `next`, even.

Would it be possible and reasonable to teach 'git branch' to
call this as a subcommand, i.e. as 'git branch diff'?  Then
the completion wouldn't offer git branch-diff.

Users could still call it directly if they wanted, though
I'd tend to think that should be discouraged and have it
treated as an implementation detail that it's a separate
binary.

We have a number of commands which take subcommands this way
(bundle, bisect, notes, submodule, and stash come to mind).
I don't know if any are used with and without a subcommand,
but it doesn't seem too strange from a UI point of view, to
me.

(I don't know if it's coincidental that of the existing
commands I noted above, 3 of the 5 are currently implemented
as shell scripts.  But they've all seen at least some work
toward converting them to C, I believe).

The idea might be gross and/or unreasonable from an
implementation or UI view.  I'm not sure, but I thought I
would toss the idea out.

This wouldn't work for git cherry{,-pick} where you wouldn't
consider 'git cherry pick' as related to 'git cherry'
though.

We also have this with git show{,-branch} and some others.
It's a mild annoyance, but muscle memory adapts eventually.

-- 
Todd
~~
A budget is just a method of worrying before you spend money, as well
as afterward.

Re: [PATCH v2 12/18] branch-diff: use color for the commit pairs

[Todd Zullinger]

Hi Johannes,

As many others have already said, thanks for this series!
I've used tbdiff a bit over the years, but having a builtin
will make it much more convenient (and the speed boost from
a C implementation will be a very nice bonus).

Johannes Schindelin wrote:
> @@ -430,6 +451,8 @@ int cmd_branch_diff(int argc, const char **argv, const 
> char *prefix)
>   struct string_list branch1 = STRING_LIST_INIT_DUP;
>   struct string_list branch2 = STRING_LIST_INIT_DUP;
>  
> + git_diff_basic_config("diff.color.frag", "magenta", NULL);
> +
>   diff_setup();
>   diffopt.output_format = DIFF_FORMAT_PATCH;
>   diffopt.flags.suppress_diff_headers = 1;

Should this also (or only) check color.diff.frag?  I thought
that color.diff.* was preferred over diff.color.*, though
that doesn't seem to be entirely true in all parts of the
current codebase.

In testing this series it seems that setting color.diff
options to change the various colors read earlier in this
patch via diff_get_color_opt, as well as the 'frag' slot,
are ignored.  Setting them via diff.color. does work.

The later patch adding a man page documents branch-diff as
using `diff.color.*` and points to git-config(1), but the
config docs only list color.diff.

Is this a bug in the diff_get_color{,_opt}() tooling?
It's certainly not anything you've introduced here, of
course.  I just noticed that some custom color.diff settings
I've used weren't picked up by branch-diff, despite your
clear intention to respect colors from the config.

-- 
Todd
~~
Abandon the search for Truth; settle for a good fantasy.

[PATCH] doc/clone: update caption for GIT URLS cross-reference

[Todd Zullinger]

The description of the  argument directs readers to "See the
URLS section below".  When generating HTML this becomes a link to the
"GIT URLS" section.  When reading the man page in a terminal, the
caption is slightly misleading.  Use "GIT URLS" as the caption to avoid
any confusion.

Signed-off-by: Todd Zullinger <t...@pobox.com>
---
Martin Ågren wrote:
> On 18 April 2018 at 22:56, Todd Zullinger <t...@pobox.com> wrote:
>> Subject: [PATCH] doc/clone: update caption for GIT URLS cross-reference
>>
>> The description of the  argument directs readers to "See the
>> URLS section below".  When generating HTML this becomes a link to the
>> "GIT URLS" section.  When reading the man page in a terminal, the
>> caption is slightly misleading.  Use "GIT URLS" as the caption to avoid
>> an confusion.
> 
> s/an/any/?

Indeed, thanks.

>> The man page produced by asciidoc doesn't include hyperlinks.  The
>> description of the  argument simply
> 
> Abandoned first attempt at log message? ;-)

That or it was when a squirrel ran by my window. ;)

Thanks for catching both of these mistakes.

 Documentation/git-clone.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 42ca7b5095..b844b9957c 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -260,7 +260,7 @@ or `--mirror` is given)
 
 ::
The (possibly remote) repository to clone from.  See the
-   <<URLS,URLS>> section below for more information on specifying
+   <<URLS,GIT URLS>> section below for more information on specifying
repositories.
 
 ::
-- 
2.17.0

-- 
Todd
~~
Whenever you find yourself on the side of the majority, it is time to
pause and reflect.
-- Mark Twain

Re: man page for "git remote set-url" seems confusing/contradictory

[Todd Zullinger]

Junio C Hamano wrote:
> Jacob Keller <jacob.kel...@gmail.com> writes:
> 
>> Maybe something like:
>>
>> "Note that the push URL and the fetch URL, even though they can be set
>> differently, are expected to refer to the same repository. For
>> example, the fetch URL might use an unauthenticated transport, while
>> the push URL generally requires authentication" Then follow this bit
>> with the mention of multiple remotes.
>>
>> (I think "repository" conveys the meaning better than "place".
>> Technically, the URLs can be completely different as long as they end
>> up contacting the same real server repository.)
> 
> Sounds sensible.  Let's see if there is any further input and then
> somebody pleaes wrap this up in a final patch ;-)

A pointer to the "GIT URLS" section in git-fetch(1) might
also be useful?  That section is provided via urls.txt and
urls-remotes.txt and is included the git-clone, git-fetch,
git-pull, and git-push man pages.

We could also include urls-remotes.txt in git-remote, though
that seems like a lot of text to add to yet another man
page.  Maybe a giturls.txt could be created and referenced
(as a further enhancement if someone is inclined).

Tangentially (and I don't know if it's worth fixing), while
poking around the documentation which includes urls.txt I
noticed that git-clone.txt refers readers to the "URLS
section below" when the name of the section is "GIT URLS".

I doubt any readers would be confused, but it would be
consistent with the other files which include urls.txt to
use "GIT URLS" as the referenced section name.

-- >& --
Subject: [PATCH] doc/clone: update caption for GIT URLS cross-reference

The description of the  argument directs readers to "See the
URLS section below".  When generating HTML this becomes a link to the
"GIT URLS" section.  When reading the man page in a terminal, the
caption is slightly misleading.  Use "GIT URLS" as the caption to avoid
an confusion.

The man page produced by asciidoc doesn't include hyperlinks.  The
description of the  argument simply

Signed-off-by: Todd Zullinger <t...@pobox.com>
---
 Documentation/git-clone.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 42ca7b5095..b844b9957c 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -260,7 +260,7 @@ or `--mirror` is given)
 
 ::
The (possibly remote) repository to clone from.  See the
-   <<URLS,URLS>> section below for more information on specifying
+   <<URLS,GIT URLS>> section below for more information on specifying
repositories.
 
 ::
-- 
2.17.0

-- 
Todd
~~
The ultimate result of shielding men from the effects of folly is to
fill the world with fools.
-- Herbert Spencer

Re: [PATCH v4] git{,-blame}.el: remove old bitrotting Emacs code

[Todd Zullinger]

Thanks for working on this cruft cleanup Ævar and to
Jonathan & Junio for asking questions about how to improve
this transition for packagers & users.

Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  writes:
> 
>>> On the other hand, the 6-lines of e-lisp you wrote for git.el
>>> replacement is something the packagers could have written for their
>>> users, so (1) if we really want to go extra mile without trusting
>>> that distro packagers are less competent than us in helping their
>>> users, we'd be better off to leave Makefile in, or (2) if we trust
>>> packagers and leave possible end-user confusion as their problem
>>> (not ours), then we can just remove as your previous round did.
>>>
>>> And from that point of view, I find this round slightly odd.
>>
>> I think the way it is makes sense. In Debian debian/git-el.install just
>> does:
>> ...
>> RedHat does use contrib/emacs/Makefile:
>> ...
>> But they can either just do their own byte compilation as they surely do
>> for other elisp packages,...
>
> In short, Debian happens to be OK, but RedHat folks need to do work
> and cannot use what we ship out of the box, *IF* they care about end
> user experience.

I don't think it's a big deal for the Fedora/Red Hat
packages to adjust and manually install the stub git-el.

Anyone doing automated rebuilds from the current Fedora
git.spec will notice the make failure and can then check the
relese notes to find out about the change, I imagine.

> That was exactly why I felt it was "odd" (iow, "uneven").  We bother
> to give a stub git.el; we do not bother to make sure it would keep
> being installed if the packagers do not bother to update their
> procedure.

I wonder if leaving the Makefile in place would prevent
packages from even noticing the change?  It might still be a
good plan to help ease the transition.  I don't know if
that's as important for something in contrib/ or not.

> If we do not change anything other than making *.el into stubs, then
> it is a lot more likely that end user experience on *any* distro
> that have been shipping contrib/emacs/ stuff will by default
> (i.e. if the packagers do not do anything to adjust) be what we
> design here---upon loading they'd see (error) triggering that nudge
> them towards modern and maintained alternatives.  If we do more than
> that, e.g. remove Makefile, then some distros need to adjust, or
> their build would be broken.
> 
> I suspect that the set of people Cc'ed on the thread are a lot more
> familiar than I am with how distro packagers prefer us to deliber,
> so I'll stop speculating at this point.

I should note that I'm not an emacs user, so I likely lack a
good understanding of how people use the current
git{,-blame}.el files.  I could simply be doing it wrong in
the steps I took to test this.

With the fedora packaging, we've shipped a git-init.el that
adds autoload entries for git-status and git-blame-mode
(since adding the emacs files in 2007).  That allows users
to make use of those features without adding anything to
their local emacs configuration.

If I open emacs with a current fedora packaging, I can issue
M-x git-status or M-x git-blame-mode.  After applying this
patch and removing the git-init.el, that no longer works
but rather than the detailed warning message, I just get a
transient "no matches" in the emacs status line.

However, if I add "(require 'git)" to ~/.emacs, then I get
the "error: git.el no longer ships with git" message in the
warnings buffer.

Does this mean that only users who have manually loaded
git.el will see the warning?  If so, is there a preferred
method to have the warning appear when calling the commands
previously provided, to give a better user experience?

-- 
Todd
~~
Faith, n. Belief without evidence in what is told by one who speaks
without knowledge, of things without parallel.
-- Ambrose Bierce, "The Devil's Dictionary"

Re: [PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

[Todd Zullinger]

Hi Stefan,

Stefan Beller wrote:
> Please see the "What's cooking?" email on the mailing list that is
> sent out periodically by Junio.
> the last one is
> https://public-inbox.org/git/xmqqd0z865pk@gitster-ct.c.googlers.com/
> which says:
> 
>> * jk/ref-array-push (2018-04-09) 3 commits
>> - ref-filter: factor ref_array pushing into its own function
>> - ref-filter: make ref_array_item allocation more consistent
>> - ref-filter: use "struct object_id" consistently
>> (this branch is used by hn/sort-ls-remote.)
>>
>> API clean-up aournd ref-filter code.
>>
>> Will merge to 'next'.
> 
> It will be merged to next and if no people speak up (due to bugs
> observed or such)
> then it will be merged to master eventually, later.
> 
> I am not able to find the documentation for the workflow right now,
> though it is partially covered in Documentation/SubmittingPatches.

Perhaps Documentation/howto/maintain-git.txt is the
documentation you're thinking of?

https://kernel.org/pub/software/scm/git/docs/howto/maintain-git.html

There's also MaintNotes in the todo branch:

https://raw.githubusercontent.com/git/git/todo/MaintNotes

-- 
Todd
~~
Be who you are and say what you feel because those who mind don't
matter and those who matter don't mind.
-- Dr Seuss, "Oh the Places You'll Go"

Re: [PATCH] Fix condition for redirecting stderr

[Todd Zullinger]

Lucas Werkmeister wrote:
> Since the --log-destination option was added in 0c591cacb ("daemon: add
> --log-destination=(stderr|syslog|none)", 2018-02-04) with the explicit
> goal of allowing logging to stderr when running in inetd mode, we should
> not always redirect stderr to /dev/null in inetd mode, but rather only
> when stderr is not being used for logging.

Perhaps 's/^F/daemon: f/' on the subject?  (Junio may well
already have done so while queueing locally.)

The patch itself looks reasonable (to my relatively untrained eyes).

-- 
Todd
~~
Hardware:  the parts of a computer that can be kicked.
-- Jeff Pesis

[PATCH] completion: complete tags with git tag --delete/--verify

[Todd Zullinger]

Completion of tag names has worked for the short -d/-v options since
88e21dc746 ("Teach bash about completing arguments for git-tag",
2007-08-31).  The long options were not added to "git tag" until many
years later, in c97eff5a95 ("git-tag: introduce long forms for the
options", 2011-08-28).

Extend tag name completion to --delete/--verify.

Signed-off-by: Todd Zullinger <t...@pobox.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 6da95b8095..c7957f0a90 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2967,7 +2967,7 @@ _git_tag ()
while [ $c -lt $cword ]; do
i="${words[c]}"
case "$i" in
-   -d|-v)
+   -d|--delete|-v|--verify)
__gitcomp_direct "$(__git_tags "" "$cur" " ")"
return
;;
-- 
2.17.0.rc0

Re: [PATCH] RelNotes: add details on Perl module changes

[Todd Zullinger]

I wrote:
> Document changes to core and non-core Perl module handling in 2.17.

This should have:

Signed-off-by: Todd Zullinger <t...@pobox.com>

And perhaps also:

Helped-by: Junio C Hamano <gits...@pobox.com>

since I borrowed liberally from your initial text. :)

> ---
> Junio C Hamano <gits...@pobox.com> writes:
> 
>>> I haven't wordsmithed it fully, but it should say something along
>>> the lines of ...
>>>
>>>  Documentation/RelNotes/2.16.0.txt | 10 ++
>>>  1 file changed, 10 insertions(+)
>>
>> Eh, of course the addition should go to 2.17 release notes ;-)  I
>> just happened to be reviewing a topic forked earlier.
> 
> Maybe something like this?  I had intended to suggest a note about
> NO_PERL_CPAN_FALLBACKS as well, so that's included too.  I don't know if that
> should be expanded to provide more of a hint to users/packagers on platforms
> where these modules are harder to install, letting them know that we now have
> fallbacks to Error and Mail::Address.  That might allow scripts which were
> previously excluded to be included on their platforms.
> 
>  Documentation/RelNotes/2.17.0.txt | 14 ++
>  INSTALL   |  3 ++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/RelNotes/2.17.0.txt 
> b/Documentation/RelNotes/2.17.0.txt
> index c828d37345..085bf1dba1 100644
> --- a/Documentation/RelNotes/2.17.0.txt
> +++ b/Documentation/RelNotes/2.17.0.txt
> @@ -75,6 +75,20 @@ Performance, Internal Implementation, Development Support 
> etc.
>   * The build procedure for perl/ part has been greatly simplified by
> weaning ourselves off of MakeMaker.
>  
> + * Perl 5.8 or greater has been required since Git 1.7.4 released in
> +   2010, but we continued to assume some core modules may not exist and
> +   used a conditional "eval { require <> }"; we no longer do
> +   this.  Some platforms (Fedora/RedHat/CentOS, for example) ship Perl
> +   without all core modules by default (e.g. Digest::MD5, File::Temp,
> +   File::Spec, Net::Domain, Net::SMTP).  Users on such platforms may
> +   need to install these additional modules.
> +
> + * As a convenience, we install copies of Perl modules we require which
> +   are not part of the core Perl distribution (e.g. Error and
> +   Mail::Address).  Users and packagers whose operating system provides
> +   these modules can set NO_PERL_CPAN_FALLBACKS to avoid installing the
> +   bundled modules.
> +
>   * In preparation for implementing narrow/partial clone, the machinery
> for checking object connectivity used by gc and fsck has been
> taught that a missing object is OK when it is referenced by a
> diff --git a/INSTALL b/INSTALL
> index 60e515eaf7..c39006e8e7 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -126,7 +126,8 @@ Issues of note:
> Redhat/Fedora are reported to ship Perl binary package with some
> core modules stripped away (see http://lwn.net/Articles/477234/),
> so you might need to install additional packages other than Perl
> -   itself, e.g. Time::HiRes.
> +   itself, e.g. Digest::MD5, File::Spec, File::Temp, Net::Domain,
> +   Net::SMTP, and Time::HiRes.
>  
>   - git-imap-send needs the OpenSSL library to talk IMAP over SSL if
> you are using libcurl older than 7.34.0.  Otherwise you can use

-- 
Todd
~~
The secret of life is honesty and fair dealing. If you can fake that,
you've got it made.
-- Groucho Marx

[PATCH] RelNotes: add details on Perl module changes

[Todd Zullinger]

Document changes to core and non-core Perl module handling in 2.17.
---
Junio C Hamano  writes:

>> I haven't wordsmithed it fully, but it should say something along
>> the lines of ...
>>
>>  Documentation/RelNotes/2.16.0.txt | 10 ++
>>  1 file changed, 10 insertions(+)
>
> Eh, of course the addition should go to 2.17 release notes ;-)  I
> just happened to be reviewing a topic forked earlier.

Maybe something like this?  I had intended to suggest a note about
NO_PERL_CPAN_FALLBACKS as well, so that's included too.  I don't know if that
should be expanded to provide more of a hint to users/packagers on platforms
where these modules are harder to install, letting them know that we now have
fallbacks to Error and Mail::Address.  That might allow scripts which were
previously excluded to be included on their platforms.

 Documentation/RelNotes/2.17.0.txt | 14 ++
 INSTALL   |  3 ++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Documentation/RelNotes/2.17.0.txt 
b/Documentation/RelNotes/2.17.0.txt
index c828d37345..085bf1dba1 100644
--- a/Documentation/RelNotes/2.17.0.txt
+++ b/Documentation/RelNotes/2.17.0.txt
@@ -75,6 +75,20 @@ Performance, Internal Implementation, Development Support 
etc.
  * The build procedure for perl/ part has been greatly simplified by
weaning ourselves off of MakeMaker.
 
+ * Perl 5.8 or greater has been required since Git 1.7.4 released in
+   2010, but we continued to assume some core modules may not exist and
+   used a conditional "eval { require <> }"; we no longer do
+   this.  Some platforms (Fedora/RedHat/CentOS, for example) ship Perl
+   without all core modules by default (e.g. Digest::MD5, File::Temp,
+   File::Spec, Net::Domain, Net::SMTP).  Users on such platforms may
+   need to install these additional modules.
+
+ * As a convenience, we install copies of Perl modules we require which
+   are not part of the core Perl distribution (e.g. Error and
+   Mail::Address).  Users and packagers whose operating system provides
+   these modules can set NO_PERL_CPAN_FALLBACKS to avoid installing the
+   bundled modules.
+
  * In preparation for implementing narrow/partial clone, the machinery
for checking object connectivity used by gc and fsck has been
taught that a missing object is OK when it is referenced by a
diff --git a/INSTALL b/INSTALL
index 60e515eaf7..c39006e8e7 100644
--- a/INSTALL
+++ b/INSTALL
@@ -126,7 +126,8 @@ Issues of note:
  Redhat/Fedora are reported to ship Perl binary package with some
  core modules stripped away (see http://lwn.net/Articles/477234/),
  so you might need to install additional packages other than Perl
- itself, e.g. Time::HiRes.
+ itself, e.g. Digest::MD5, File::Spec, File::Temp, Net::Domain,
+ Net::SMTP, and Time::HiRes.
 
- git-imap-send needs the OpenSSL library to talk IMAP over SSL if
  you are using libcurl older than 7.34.0.  Otherwise you can use
-- 
2.17.0.rc0

Re: [PATCH v2] Fix misconversion of gitsubmodule.txt

[Todd Zullinger]

Hi,

marmot1123 wrote:
> In the 2nd and 4th paragraph of DESCRIPTION, there ware misconversions 
> `submodule’s`.
> It seems non-ASCII apostrophes, so I rewrite ASCII apostrophes.

If replacing the non-ASCI apostrophes is the goal, aren't
there a number of others in the same file worth cleaning up
at the same time?

$ grep '’' Documentation/gitsubmodules.txt
the submodule’s working directory pointing to (i).
superproject expects the submodule’s working directory to be at.
When deinitialized or deleted (see below), the submodule’s Git
but no submodule working directory. The submodule’s Git directory
the superproject’s `$GIT_DIR/config` file, so the superproject’s history
The deletion removes the superproject’s tracking data, which are
The submodule’s working directory is removed from the file

This does seem to be the only file which includes the
non-ASCII apostrophe under Documentation.

Some tests include it (intentionally) as does
contrib/credential/netrc/git-credential-netrc.

-- 
Todd
~~
The direct use of physical force is so poor a solution to the problem
of limited resources that it is commonly employed only by small
children and great nations.
-- David Friedman

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Todd Zullinger]

I wrote:
> Hi Jonathan,
> 
> Jonathan Nieder wrote:
>> Todd Zullinger wrote:
> [...]
>>> +++ b/Makefile
>>> @@ -296,6 +296,9 @@ all::
>>>  #
>>>  # Define NO_PERL if you do not want Perl scripts or libraries at all.
>>>  #
>>> +# Define NO_PERL_CPAN_FALLBACKS if you do not want to install fallbacks for
>>> +# perl CPAN modules.
>> 
>> nit: Looking at this as a naive user, it's not obvious what kind of
>> fallbacks are meant. How about:
>> 
>>  Define NO_PERL_CPAN_FALLBACKS if you do not want to install bundled
>>  copies of CPAN modules that serve as a fallback in case the modules are
>>  not available on the system.
>> 
>> Or perhaps:
>> 
>>  Define HAVE_CPAN_MODULES if you have Error.pm and Mail::Address 
>> installed
>>  and do not want to install the fallback copies from perl/FromCPAN.
> 
> Hmm, a positive variable like HAVE_CPAN_MODULES is
> appealing.
> 
> I don't know about listing the modules, as those seem likely
> to change and then the comment becomes stale.  It's nice to
> have a shorter name.  I could easily go back and forth.
> Hopefully some other folks will chime in with preferences.
> 
>> Would this patch need to update the loader to expect the modules in
>> the new location?
> 
> That's a good catch.  In checking how this ends up when not
> setting NO_PERL_CPAN_FALLBACKS, we end up installing
> FromCPAN at the root of $perllibdir rather than under the
> Git dir.
> 
> While we could probably fix Git::LoadCPAN, I doubt we want
> to pollute the namespace. ;) So we'll want to ensure the
> files get put in the right place from the start.  I'll try
> to fix that up.

I ended up adding a second variable to hold the FromCPAN
wildcard match, as I couldn't come up with any clean way to
do it in the same LIB_PERL{,_GEN} vars.

I tested it with and without NO_PERL_CPAN_FALLBACKS set and
with and without the system Error and Mail::Address modules
installed.  There's nothing which automatically removes the
perl/build/lib/Git/FromCPAN tree if make was run without
NO_PERL_CPAN_FALLBACKS set and then re-run with it.  I don't
know if that's something we'd care to support or not.  I
suspect it's simpler to require 'make clean' between such
runs.

(I had to restore the FromCPAN copy of Address.pm, as noted
in <87tvuif10e@evledraar.gmail.com>, of course.)

Here's what it looks like now (still subject to changing the
NO_PERL_CPAN_FALLBACKS variable).

 8> 
From: Todd Zullinger <t...@pobox.com>
Subject: [PATCH] Makefile: add NO_PERL_CPAN_FALLBACKS knob

We include some perl modules which are not part of the core perl
install, as a convenience.  This allows us to rely on those modules in
our perl-based tools and scripts without requiring users to install the
modules from CPAN or their operating system packages.

Users whose operating system provides these modules and packagers of Git
often don't want to ship or use these bundled modules.  Allow these
users to set NO_PERL_CPAN_FALLBACKS to avoid installing the bundled
modules.

Signed-off-by: Todd Zullinger <t...@pobox.com>
---
 Makefile| 14 ++
 perl/{Git => }/FromCPAN/.gitattributes  |  0
 perl/{Git => }/FromCPAN/Error.pm|  0
 perl/{Git => }/FromCPAN/Mail/.gitattributes |  0
 perl/{Git => }/FromCPAN/Mail/Address.pm |  0
 5 files changed, 14 insertions(+)
 rename perl/{Git => }/FromCPAN/.gitattributes (100%)
 rename perl/{Git => }/FromCPAN/Error.pm (100%)
 rename perl/{Git => }/FromCPAN/Mail/.gitattributes (100%)
 rename perl/{Git => }/FromCPAN/Mail/Address.pm (100%)

diff --git a/Makefile b/Makefile
index bdd50069af..49244fb116 100644
--- a/Makefile
+++ b/Makefile
@@ -296,6 +296,10 @@ all::
 #
 # Define NO_PERL if you do not want Perl scripts or libraries at all.
 #
+# Define NO_PERL_CPAN_FALLBACKS if you do not want to install bundled
+# copies of CPAN modules that serve as a fallback in case the modules are
+# not available on the system.
+#
 # Define PYTHON_PATH to the path of your Python binary (often /usr/bin/python
 # but /usr/bin/python2.7 on some platforms).
 #
@@ -2300,15 +2304,25 @@ po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
 
 LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm 
perl/Git/*/*/*.pm)
 LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
+ifndef NO_PERL_CPAN_FALLBACKS
+LIB_CPAN := $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
+LIB_CPAN_GEN := $(patsubst perl/%.pm,perl/build/lib/Git/%.pm,$(LIB_CPAN))
+endif
 
 ifndef NO_PERL
 all:: $(LIB_PERL_GEN)
+ifndef NO_PERL_CPAN_FALLBACKS
+all:: $(LIB_CPAN_GEN)
+endif
 endif
 
 perl/build/lib/%.pm: perl/%.pm
$(QUIET_GEN)mkdir -p $(dir $@) && \
sed -e 's|@@LOCALED

[PATCH] Makefile: remove *.spec from clean target

[Todd Zullinger]

Support for generating an rpm was dropped in ab214331cf ("Makefile: stop
pretending to support rpmbuild", 2016-04-04).  We don't generate any
*.spec files so there is no need to clean them up.

Signed-off-by: Todd Zullinger <t...@pobox.com>
---

I noticed this minor cruft today.  Since we don't generate a
spec file, this is at best unneeded.  At worst we could
wrongly delete a users spec file if they happened to be
working on it in their git clone and called make clean.

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index c56fdc14ca..d135f8baa1 100644
--- a/Makefile
+++ b/Makefile
@@ -2734,7 +2734,7 @@ clean: profile-clean coverage-clean
$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
$(RM) -r bin-wrappers $(dep_dirs)
$(RM) -r po/build/
-   $(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) 
tags cscope*
+   $(RM) *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags 
cscope*
$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz
$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
-- 
2.16.2

-- 
Todd
~~
A thing worth having is a thing worth cheating for.
-- W. C. Fields

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Todd Zullinger]

Hi Jonathan,

Jonathan Nieder wrote:
> Todd Zullinger wrote:
[...]
>> +++ b/Makefile
>> @@ -296,6 +296,9 @@ all::
>>  #
>>  # Define NO_PERL if you do not want Perl scripts or libraries at all.
>>  #
>> +# Define NO_PERL_CPAN_FALLBACKS if you do not want to install fallbacks for
>> +# perl CPAN modules.
> 
> nit: Looking at this as a naive user, it's not obvious what kind of
> fallbacks are meant. How about:
> 
>   Define NO_PERL_CPAN_FALLBACKS if you do not want to install bundled
>   copies of CPAN modules that serve as a fallback in case the modules are
>   not available on the system.
> 
> Or perhaps:
> 
>   Define HAVE_CPAN_MODULES if you have Error.pm and Mail::Address 
> installed
>   and do not want to install the fallback copies from perl/FromCPAN.

Hmm, a positive variable like HAVE_CPAN_MODULES is
appealing.

I don't know about listing the modules, as those seem likely
to change and then the comment becomes stale.  It's nice to
have a shorter name.  I could easily go back and forth.
Hopefully some other folks will chime in with preferences.

> Would this patch need to update the loader to expect the modules in
> the new location?

That's a good catch.  In checking how this ends up when not
setting NO_PERL_CPAN_FALLBACKS, we end up installing
FromCPAN at the root of $perllibdir rather than under the
Git dir.

While we could probably fix Git::LoadCPAN, I doubt we want
to pollute the namespace. ;) So we'll want to ensure the
files get put in the right place from the start.  I'll try
to fix that up.

Thanks for the careful eyes, as usual.

-- 
Todd
~~
Happiness is like peeing on yourself. Everyone can see it, but only
you can feel its warmth

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Todd Zullinger]

Ævar Arnfjörð Bjarmason wrote:
> On Thu, Feb 15 2018, Todd Zullinger jotted:
>> What about moving perl/Git/FromCPAN to perl/FromCPAN and
>> then including perl/FromCPAN in LIB_PERL{,_GEN} only if
>> NO_PERL_CPAN_FALLBACKS is unset?
>>
>>  LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm 
>> perl/Git/*/*/*.pm)
>> +ifndef NO_PERL_CPAN_FALLBACKS
>> +LIB_PERL += $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
>> +endif
>>  LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
>>
>> I haven't tested that at all, so it could be broken in many
>> ways.
> 
> Yes that's a much better idea, it evades the whole problem of conflating
> the perl/Git* glob.

I did test this yesterday and it seems to work well.  Here
it is in patch form, in case it's helpful to you for the
next version.  It might well be better as part of a commit
teaching Git::LoadCPAN to respect NO_PERL_CPAN_FALLBACKS.

 8< 
From: Todd Zullinger <t...@pobox.com>
Subject: [PATCH] Makefile: add NO_PERL_CPAN_FALLBACKS to disable fallback
 module install

As noted in perl/Git/LoadCPAN.pm, operating system packages often don't
want to ship Git::FromCPAN tree at all, preferring to use their own
packaging of CPAN modules instead.  Allow such packagers to set
NO_PERL_CPAN_FALLBACKS to easily avoid installing these fallback perl
CPAN modules.

Signed-off-by: Todd Zullinger <t...@pobox.com>
---
 Makefile| 6 ++
 perl/{Git => }/FromCPAN/.gitattributes  | 0
 perl/{Git => }/FromCPAN/Error.pm| 0
 perl/{Git => }/FromCPAN/Mail/.gitattributes | 0
 perl/{Git => }/FromCPAN/Mail/Address.pm | 0
 5 files changed, 6 insertions(+)
 rename perl/{Git => }/FromCPAN/.gitattributes (100%)
 rename perl/{Git => }/FromCPAN/Error.pm (100%)
 rename perl/{Git => }/FromCPAN/Mail/.gitattributes (100%)
 rename perl/{Git => }/FromCPAN/Mail/Address.pm (100%)

diff --git a/Makefile b/Makefile
index 5bcd83ddf3..838b3c6393 100644
--- a/Makefile
+++ b/Makefile
@@ -296,6 +296,9 @@ all::
 #
 # Define NO_PERL if you do not want Perl scripts or libraries at all.
 #
+# Define NO_PERL_CPAN_FALLBACKS if you do not want to install fallbacks for
+# perl CPAN modules.
+#
 # Define PYTHON_PATH to the path of your Python binary (often /usr/bin/python
 # but /usr/bin/python2.7 on some platforms).
 #
@@ -2297,6 +2300,9 @@ po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
$(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<
 
 LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm 
perl/Git/*/*/*.pm)
+ifndef NO_PERL_CPAN_FALLBACKS
+LIB_PERL += $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
+endif
 LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
 
 ifndef NO_PERL
diff --git a/perl/Git/FromCPAN/.gitattributes b/perl/FromCPAN/.gitattributes
similarity index 100%
rename from perl/Git/FromCPAN/.gitattributes
rename to perl/FromCPAN/.gitattributes
diff --git a/perl/Git/FromCPAN/Error.pm b/perl/FromCPAN/Error.pm
similarity index 100%
rename from perl/Git/FromCPAN/Error.pm
rename to perl/FromCPAN/Error.pm
diff --git a/perl/Git/FromCPAN/Mail/.gitattributes 
b/perl/FromCPAN/Mail/.gitattributes
similarity index 100%
rename from perl/Git/FromCPAN/Mail/.gitattributes
rename to perl/FromCPAN/Mail/.gitattributes
diff --git a/perl/Git/FromCPAN/Mail/Address.pm b/perl/FromCPAN/Mail/Address.pm
similarity index 100%
rename from perl/Git/FromCPAN/Mail/Address.pm
rename to perl/FromCPAN/Mail/Address.pm
-- 
2.16.1

-- 
Todd
~~
Damn you and your estrogenical treachery!
-- Stewie Griffin

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Todd Zullinger]

[I dropped bbour...@slb.com from the Cc: list, as it bounced
on my previous reply.]

Ævar Arnfjörð Bjarmason wrote:
> That makes sense, I'll incorporate that in a re-roll. I like
> NO_PERL_CPAN_FALLBACKS or just NO_CPAN_FALLBACKS better.

Either is an improvement.  Starting with NO_PERL_ seems
like a slightly better bikeshed color. :)

> I'd really like to find some solution that works differently though,
> because with this approach we'll run the full test suite against a
> system where our fallbacks will be in place (although if the OS
> distributor has done as promised we won't use them), and then just
> remove this at 'make install' time, also meaning we'll re-gen it before
> running 'make install' again, only to rm it again.
> 
> The former issue we could deal with by munging the Git::LoadCPAN file so
> it knows about NO_PERL_CPAN_FALLBACKS, and will always refuse to use the
> fallbacks if that's set. That's a good idea anyway, because right now if
> you e.g. uninstall Error.pm on Debian (which strips the CPAN fallbacks)
> you get a cryptic "BUG: ..." message, it should instead say "we couldn't
> get this module the OS promised we'd have" or something to that effect.

Teaching Git::LoadCPAN to never fallback sounds like a good
idea.  At least then if the packager intended to avoid the
fallbacks and didn't get it right the error message could be
more useful.

Hopefully that's not a common problem for packagers though.
(And adding the Makefile knob was intended to help make it
easier for packagers to achieve this common goal.)

> The latter is trickier, I don't see an easy way to coerce the Makefile
> into not copying the FromCPAN directory without going back to a
> hardcoded list again, the easiest thing is probably to turn that:
> 
> $(TAR) cf - .)
> 
> Into:
> 
> $(TAR) cf - $(find ... -not )
> 
> Or something like that to get all the stuff that isn't the Git/FromCPAN
> directory.
> 
> Other suggestions most welcome.

What about moving perl/Git/FromCPAN to perl/FromCPAN and
then including perl/FromCPAN in LIB_PERL{,_GEN} only if
NO_PERL_CPAN_FALLBACKS is unset?

 LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm 
perl/Git/*/*/*.pm)
+ifndef NO_PERL_CPAN_FALLBACKS
+LIB_PERL += $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
+endif
 LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))

I haven't tested that at all, so it could be broken in many
ways.

Thanks,

-- 
Todd
~~
The nice thing about egotists is that they don't talk about other
people.
-- Lucille S. Harper

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Todd Zullinger]

Hi Jonathan,

Jonathan Nieder wrote:
> Ævar Arnfjörð Bjarmason wrote:
> [...]
>> +++ b/perl/Git/LoadCPAN.pm
>> @@ -0,0 +1,74 @@
> [...]
>> +The Perl code in Git depends on some modules from the CPAN, but we
>> +don't want to make those a hard requirement for anyone building from
>> +source.
> 
> not about this patch: have we considered making it a hard requirement?
> Both Mail::Address and Error.pm are pretty widely available, and I
> wonder if we could make the instructions in the INSTALL file say that
> they are required dependencies to simplify things.
> 
> I admit part of my bias here is coming from the distro world, where we
> have to do extra work to get rid of the FromCPAN embedded copies and
> would be happier not to have to.

Heh, a similar bias is what led me to suggest a Makefile
knob to prevent installing the fallbacks.  I neglected to
add you to the Cc list on that reply.  But I was thinking of
Debian and other distributions whom I know would similarly
want to exclude Git/FromCPAN from their git packages.  I
know it's a simple rm, but it might as well be a simple rm
in one place than spread across each package.  :)

It may still be worth considering whether it's reasonable to
make Mail::Address and Error.pm hard requirements, but I'm
not sure if there are any platforms where such a requirement
would be a pain.  If there are folks here who are happy to
maintain this fallback method, then a simple Makefile knob
gives us the best of both worlds.

-- 
Todd
~~
Historian, n. A broad-gauge gossip.
-- Ambrose Bierce, "The Devil's Dictionary"

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Todd Zullinger]

Hi Ævar,

Ævar Arnfjörð Bjarmason wrote:
> +Git::LoadCPAN - Wrapper for loading modules from the CPAN (OS) or Git's own 
> copy
> +
> +=head1 DESCRIPTION
> +
> +The Perl code in Git depends on some modules from the CPAN, but we
> +don't want to make those a hard requirement for anyone building from
> +source.
> +
> +Therefore the L namespace shipped with Git contains
> +wrapper modules like C that will first
> +attempt to load C from the OS, and if that doesn't work
> +will fall back on C shipped with Git
> +itself.
> +
> +Usually OS's will not ship with Git's Git::FromCPAN tree at all,
> +preferring to use their own packaging of CPAN modules instead.

This is something I wondered about.  What's the recommended
method to ensure git packaged for an OS/distribution doesn't
ever use the fallbacks?  Remove $perllibdir/Git/FromCPAN
after make install?

If so, would it be useful to add a Makefile knob to not
install the FromCPAN bits, which may be generally useful to
packagers?

Something like the following, perhaps?

(I'd feel bad suggesting this without a patch, after all the
work you've already done to simplify and improve the perl
bits.)

 8< 
From: Todd Zullinger <t...@pobox.com>
Date: Wed, 14 Feb 2018 23:00:30 -0500
Subject: [PATCH] Makefile: add NO_PERL_CPAN to disable fallback module install

As noted in perl/Git/LoadCPAN.pm, operating system packages often don't
want to ship Git::FromCPAN tree at all, preferring to use their own
packaging of CPAN modules instead.  Allow such packagers to set
NO_PERL_CPAN to easily avoid installing these fallback perl CPAN
modules.

Signed-off-by: Todd Zullinger <t...@pobox.com>
---
 Makefile | 4 
 1 file changed, 4 insertions(+)

diff --git a/Makefile b/Makefile
index 5bcd83ddf3..c4e035e5bf 100644
--- a/Makefile
+++ b/Makefile
@@ -296,6 +296,9 @@ all::
 #
 # Define NO_PERL if you do not want Perl scripts or libraries at all.
 #
+# Define NO_PERL_CPAN if you do not want to install fallbacks for perl CPAN
+# modules.
+#
 # Define PYTHON_PATH to the path of your Python binary (often /usr/bin/python
 # but /usr/bin/python2.7 on some platforms).
 #
@@ -2572,6 +2575,7 @@ ifndef NO_GETTEXT
 endif
 ifndef NO_PERL
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perllibdir_SQ)'
+   test -z "$(NO_PERL_CPAN)" || rm -rf perl/build/lib/Git/FromCPAN
(cd perl/build/lib && $(TAR) cf - .) | \
(cd '$(DESTDIR_SQ)$(perllibdir_SQ)' && umask 022 && $(TAR) xof -)
$(MAKE) -C gitweb install
-- 
2.16.1

I don't particularly like NO_PERL_CPAN, but I'm confident
someone else will suggest an obviously better name.

I thought about moving the 'rm -rf Git/FromCPAN' after the
tar/untar, to keep the files in place for the tests.  No
tests seem to rely on those local files, so I stuck with
removing them before.  That diff was:

--- a/Makefile
+++ b/Makefile
@@ -2574,6 +2574,7 @@
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perllibdir_SQ)'
(cd perl/build/lib && $(TAR) cf - .) | \
(cd '$(DESTDIR_SQ)$(perllibdir_SQ)' && umask 022 && $(TAR) xof -)
+   test -n "$(NO_PERL_CPAN)" && rm -rf 
'$(DESTDIR_SQ)$(perllibdir_SQ)'/Git/FromCPAN
$(MAKE) -C gitweb install
 endif
 ifndef NO_TCLTK

-- 
Todd
~~
Man has made use of his intelligence, he invented stupidity.
-- Remy De Gourmant

Re: categorization, documentation and packaging of "git core" commands

[Todd Zullinger]

Robert P. J. Day wrote:
> not to belabour this (and i'm sure it's *way* too late for that),
> but fedora has the following packaging scheme.  first, there's a bunch
> of stuff in "git-core", which has no dependencies on any other
> git-related packages.

The split in Fedora between git and git-core is done to
minimize the dependencies required for a minimal git
install.  The initial reason was to to allow installing the
git-core package on systems, in containers, etc. without
requiring perl and its various dependencies to be installed.

The name git-core was not chosen to imply any official
status as core versus contrib from upstream.

(Farther back in the past, the main git package (and the
upstream tarball, IIRC) was named git-core due to conflicts
with another tool named git (GNU interactive tools).)

> so with fedora, "git" drags in "git-core" and a small number of
> additional git utilities. all of this leads one to wonder -- is there
> any comprehensible relationship between:
> 
>   1) commands that claim to be in the "git suite"
>   2) commands that come from contrib/
>   3) commands listed at
>  https://www.kernel.org/pub/software/scm/git/docs/
>   4) how different distros package all of the above
> 
> as i think we've noticed, it's not at all clear how git decides what
> is and isn't part of the "official" git suite.

I don't think there's any good reason to use the packaging
of any distribution as the source for what the git project
considers officially part of the suite.  For that, you
should look at the git source and particularly
contrib/README.  The first paragraph says:

Although these pieces are available as part of the official git
source tree, they are in somewhat different status.  The
intention is to keep interesting tools around git here, maybe
even experimental ones, to give users an easier access to them,
and to give tools wider exposure, so that they can be improved
faster.

If anything the Fedora packging does in the splitting or
naming of the git packages is something the git project
feels is incorrect or needlessly confusing, I (with my
Fedora maintainer hat on) would be happy to make any changes
I can to improve things.

-- 
Todd
~~
Some god must protect drunkards and fools, there are so many of them
still around.

Re: categorization, documentation and packaging of "git core" commands

[Todd Zullinger]

Robert P. J. Day wrote:
> On Wed, 7 Feb 2018, Todd Zullinger wrote:
> 
>> Robert P. J. Day wrote:
>>> first, here are the executables under /usr/libexec/git-core/ that
>>> are unreferenced by that web page, but that should be fine as
>>> almost all of them would be considered underlying helpers or
>>> utilities (except for things like git-subtree, but we're still
>>> unclear on its status, right?):
>>
>> I don't think there's anything unclear about git subtree's status.
>> It's in contrib/ within the source, so it's not part of the core git
>> suite.  Some distributions (Fedora being one of them) ship a
>> git-subtree package to provide it for users who want it.
> 
>   not true, "git-subtree" is part of fedora's lowest-level
> "git-core" package.

Eek, my apologies for providing bad information.  I really
should know the Fedora git packaging better than that. :/

Amusingly, I did suggest packaging it as a subpackage
specifically to avoid any confusion that it was a core
command in the Fedora bug which requested it be included in
the git packaging:

https://bugzilla.redhat.com/show_bug.cgi?id=864651

I'll see about changing that going forward in the Fedora
packaging.  I think it deserves to be in a subpackage.

-- 
Todd
~~
Doing a job RIGHT the first time gets the job done.
Doing the job WRONG fourteen times gives you job security.

Re: categorization, documentation and packaging of "git core" commands

[Todd Zullinger]

Robert P. J. Day wrote:
> first, here are the executables under /usr/libexec/git-core/ that
> are unreferenced by that web page, but that should be fine as almost
> all of them would be considered underlying helpers or utilities
> (except for things like git-subtree, but we're still unclear on its
> status, right?):

I don't think there's anything unclear about git subtree's
status.  It's in contrib/ within the source, so it's not
part of the core git suite.  Some distributions (Fedora
being one of them) ship a git-subtree package to provide it
for users who want it.

> on the other hand (and this is not so much a git issue as a fedora
> packaging issue), there are a number of command links at that web page
> that are supplied by distinct RPM packages rather than by the basic
> fedora git package, so one would need to install the following
> packages to get some of those commands on fedora:
> 
>   * gitk
>   * git-cvs
>   * git-svn
>   * git-p4
>   * git-email (provides git-send-email)

These packages are in separate sub-packages in Fedora (and
some other distributions) because they are no required by
all users and they pull in dependencies which are not wanted
on minimal installs.  In Fedora, you can install git-all to
get all the available git sub-packages.

> finally, from fedora, i am utterly unable to find a package that
> provides git-archimport. pretty sure fedora used to have a "git-arch"
> package but it's not there now.

It hasn't been in Fedora since 2011.  The tla command which
is required for git-archimport  was retired, thus we removed
the git-arch package.  The rpm changelog shows this:

* Tue Jul 26 2011 Todd Zullinger <t...@pobox.com> - 1.7.6-4
- Drop git-arch on fedora >= 16, the tla package has been retired

As does the git history for the package:

https://src.fedoraproject.org/rpms/git/c/3f0dc974fa

The tla package was retired because it failed to build for
several releases:

https://src.fedoraproject.org/rpms/tla/blob/master/f/dead.package

-- 
Todd
~~
Going to trial with a lawyer who considers your whole life-style a
Crime in Progress is not a happy prospect.
-- Hunter S. Thompson

Re: "git branch" issue in 2.16.1

[Todd Zullinger]

Hi Jason,

Jason Racey wrote:
> After upgrading git from 2.16.0 to 2.16.1 (via Homebrew -
> I’m on macOS) I noticed that the “git branch” command
> appears to display the branch listing in something similar
> to a vi editor - though not quite the same. I don’t know
> the technical term for this state. You can’t actually edit
> the output of the command, but you’re in a state where you
> have to type “q” to exit and then the list disappears.
> It’s very inconvenient and it doesn’t seem like it was by
> design. I’m using zsh in iTerm2 if that helps. Thanks.

In 2.16.0 `git branch --list` is sent to a pager by default.
(Without arguments, --list is the default, so this applies
to `git branch`).

You can set pager.branch to false to disable this in the
config, or use git --no-pager branch to do so for a single
invocation.

I can't say why you're seeing this with 2.16.1 and not
2.16.0, but I'm not familiar with homebrew, so perhaps
something didn't work as intended in 2.16.0.

-- 
Todd
~~
Historian, n. A broad-gauge gossip.
-- Ambrose Bierce, "The Devil's Dictionary"

Re: t9128 failing randomly with svn 1.9?

[Todd Zullinger]

I wrote:
> The 'git svn' tests are not run in Travis because the perl
> subversion bindings are not installed.  I haven't made time
> to try installing them and running the tests in Travis to
> see if the failures occur there, but I suspect they would.

Before anyone spends time wondering about this, I was
apparently looking at the MacOS build logs rather than the
Linux logs.  The git svn tests are indeed run in Travis, as
long as you look at the right build logs. :/

Sadly (or amusingly), I think I looked at this before and
made the same mistake, realized it, and then did it again
today.  Hopefully if I make the same mistake again I'll
realize it before I post it to the list. ;)

Anyway, the Travis Linux container uses Ubuntu Trusty, which
has subversion 1.8.8.  I suppose that's why these random
failures haven't turned up there.

-- 
Todd
~~
I got stopped by a cop the other day.  He said, "Why'd you run that
stop sign?"  I said, "Because I don't believe everything I read."
-- Stephen Wright



signature.asc
Description: PGP signature

Re: t9128 failing randomly with svn 1.9?

[Todd Zullinger]

Eric Wong wrote:
> Todd Zullinger <t...@pobox.com> wrote:
> Just a guess, but it might be related to destruction order.
> Running t9128 on a 32-bit Pentium-M, it took me 39 tries to
> fail.
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index 76a75d0b3d..2ba14269bb 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1200,6 +1200,11 @@ sub cmd_branch {
>   $ctx->copy($src, $rev, $dst)
>   unless $_dry_run;
>  
> + # Release resources held by ctx before creating another SVN::Ra
> + # so destruction is orderly.  This seems necessary Subversion 1.9.5
> + # to avoid segfaults.
> + $ctx = undef;
> +
>   $gs->fetch_all;
>  }
>  
> I'll be looping t9128, t9141 and t9167 with that for a few
> hours or day.  Will report back sooner if it fails.
> I'm on an ancient 32-bit system, I guess you guys encountered
> it on 64-bit machines?

Yeah.  I saw it on numerous architectures, x86 and x86_64.
I believe I saw it on aarch64 and ppc as well, but I don't
have build logs at hand to confirm.

I'm running the tests with and without your patch as well.
So far I've run t9128 300 times with the patch and no
failures.  Without it, it's failed 3 times in only a few
dozen runs.  That's promising.

Thanks for poking this Eric, and Brian for bringing it up.
I had intended to look at it again and mention it,
eventually. :)

-- 
Todd
~~
I figure that if God actually does exist, He's big enough to
understand an honest difference of opinion.
-- Isaac Asimov

Re: t9128 failing randomly with svn 1.9?

[Todd Zullinger]

brian m. carlson wrote:
> While running tests for my object_id part 11 series, I noticed a random
> segfault in git svn while running t9128.  I've reproduced this on a
> different machine as well, using both Subversion 1.9.5 and 1.9.7 (Debian
> stable and unstable).  It is reproducible on master.
> 
> When the test fails, it fails on the "git svn tag tag3" step, and I get
> the following:
> 
> Copying 
> file:///home/bmc/checkouts/git/t/trash%20directory.t9128-git-svn-cmd-branch/svnrepo/trunk
>  at r2 to 
> file:///home/bmc/checkouts/git/t/trash%20directory.t9128-git-svn-cmd-branch/svnrepo/tags/tag3...
> Found possible branch point: 
> file:///home/bmc/checkouts/git/t/trash%20directory.t9128-git-svn-cmd-branch/svnrepo/trunk
>  => 
> file:///home/bmc/checkouts/git/t/trash%20directory.t9128-git-svn-cmd-branch/svnrepo/tags/tag3,
>  2
> Found branch parent: (refs/remotes/origin/tags/tag3) 
> 0604824a81a121ad05aaf8caea65d8ca8f86c018
> Following parent with do_switch
> Successfully followed parent
> r7 = f8467f2cee3bcead03e84cb51cf44f467a87457d (refs/remotes/origin/tags/tag3)
> error: git-svn died of signal 11
> 
> Doing the following three times, I had two crashes.
> 
> (set -e; for i in $(seq 1 20); do (cd t && ./t9128-git-svn-cmd-branch.sh 
> --verbose); done)
> 
> I'm not really familiar with git svn or its internals, and I didn't see
> anything recently on the list about this.  Is this issue known?

I see the same test fail randomly on Fedora (and I think on
CentOS/RHEL, but I run the full tests more often on Fedora).

For me, it's tests 3 and 4 which fail with the same error.
I thought it was a failure in subversion or the perl
bindings rather than git, so I simply disabled them in the
Fedora builds.  The Debian packages skip 9128 as well (and
9167, which fails similarly).

I've seen the failures in t9141 from 'git svn branch' as
well.  I made a note to re-enable those tests after Jeff's
work to make it easier to run with shell tracing enabled by
default, but have not done so yet.

The 'git svn' tests are not run in Travis because the perl
subversion bindings are not installed.  I haven't made time
to try installing them and running the tests in Travis to
see if the failures occur there, but I suspect they would.

-- 
Todd
~~
Going to trial with a lawyer who considers your whole life-style a
Crime in Progress is not a happy prospect.
-- Hunter S. Thompson



signature.asc
Description: PGP signature

[PATCH] doc: mention 'git show' defaults to HEAD

[Todd Zullinger]

When 'git show' is called without any object it defaults to HEAD.  This
has been true since d4ed9793fd ("Simplify common default options setup
for built-in log family.", 2006-04-16).

The SYNOPSIS suggests that the object argument is required.  Clarify
that it is not required and note the default.

Signed-off-by: Todd Zullinger <t...@pobox.com>
---
This was mentioned in #git today by qaz.  It seems reasonable to document the
default.

 Documentation/git-show.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-show.txt b/Documentation/git-show.txt
index 82a4125a2d..e73ef54017 100644
--- a/Documentation/git-show.txt
+++ b/Documentation/git-show.txt
@@ -9,7 +9,7 @@ git-show - Show various types of objects
 SYNOPSIS
 
 [verse]
-'git show' [options] ...
+'git show' [options] [...]
 
 DESCRIPTION
 ---
@@ -35,7 +35,7 @@ This manual page describes only the most frequently used 
options.
 OPTIONS
 ---
 ...::
-   The names of objects to show.
+   The names of objects to show (defaults to 'HEAD').
For a more complete list of ways to spell object names, see
"SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
 
-- 
2.16.1

Re: [PATCH v4 1/4] Add tar extract install options override in installation processing.

[Todd Zullinger]

Junio C Hamano wrote:
> randall.s.bec...@rogers.com writes:
>> +# Define TAR_EXTRACT_OPTIONS if you want to change the default behaviour
>> +# from xvf to something else during installation. The option only includes
   ^^^
Shouldn't this be xof?

>> +# "o" as xf are required.

-- 
Todd
~~
Wisdom has two parts: (1) having a lot to say and (2) not saying it.

Re: Segmentation fault on clone

[Todd Zullinger]

Hi Stephen,

Stephen M. McQuay wrote:
> I submitted a bug against the brew project when git
> version 2.16.0 started segfaulting:
> 
> https://github.com/Homebrew/homebrew-core/issues/23045#issuecomment-358891009

This seems likely to be the same segfault as:

https://public-inbox.org/git/calwadsgfb10f5+nofn-phct4z1skwmcvshn8kokcycm0v6k...@mail.gmail.com/

There's a patch in that thread.

-- 
Todd
~~
The man who is a pessimist before forty-eight knows too much; the man
who is an optimist after forty-eight knows too little.
-- Mark Twain

Re: git 2.16.0 segfaults on clone of specific repo

[Todd Zullinger]

Eric Sunshine wrote:
> Nice detective work. This particular manifestation is caught by the
> following test which fails without brian's patch on MacOS (and
> presumably Windows) and succeeds with it. On Linux and BSD, it will of
> course succeed always, so I'm not sure how much practical value it
> has.

The CASE_INSENSITIVE_FS prereq could be used to avoid
running the test on systems where it won't provide much
value, couldn't it?

> --- >8 ---
> hex2oct() {
>   perl -ne 'printf "\\%03o", hex for /../g'
> }
> 
> test_expect_success 'clone on case-insensitive fs' '
>   o=$(git hash-object -w --stdint=$(printf "100644 X\0${o}100644 x\0${o}" |
>  git hash-object -w -t tree --stdin) &&
>   c=$(git commit-tree -m bogus $t) &&
>   git update-ref refs/heads/bogus $c &&
>   git clone -b bogus . bogus
> '
> --- >8 ---
> 
> (hex2oct lifted from t1007/t1450)

-- 
Todd
~~
Money frees you from doing things you dislike. Since I dislike doing
nearly everything, money is handy.
-- Groucho Marx

Re: Git uses wrong subkey for signing commits with GPG key

[Todd Zullinger]

Andrzej Ośmiałowski wrote:
> On Sat, Jan 13, 2018 at 1:22 AM, Todd Zullinger <t...@pobox.com> wrote:
>> I could be wrong, but I think you need to append '!' to
>> KEYID to force gpg to use that specific signing subkey.
[...]
> thanks for reply. You just solved my issue. I will prepare a PR to the
> docs to add relevant information.

Glad it helped.  The git-tag documentation points to
git-config and the user.signingKey variable in the
CONFIGURATION section.  The git-config documentation for
that variable currently says:

If linkgit:git-tag[1] or linkgit:git-commit[1] is not selecting the
key you want it to automatically when creating a signed tag or
commit, you can override the default selection with this variable.
This option is passed unchanged to gpg's --local-user parameter,
so you may specify a key using any method that gpg supports.

Whether that can be improved without being too verbose (or
duplicating too much of the gpg documentation), I don't
know.

Maybe it could point to the gpg documentation, though that
can be in gpg(1), gpg1(1), or gpg2(1), depending on how
their system installs gpg.

The online link covering the many formats that gpg accepts
for the --local-user (-u) option is:

https://www.gnupg.org/documentation/manuals/gnupg/Specify-a-User-ID.html

-- 
Todd
~~
It is impossible to enjoy idling thoroughly unless one has plenty of
work to do.
-- Jerome K. Jerome

Re: Git uses wrong subkey for signing commits with GPG key

[Todd Zullinger]

Hi Andrzej,

Andrzej Ośmiałowski wrote:
> I have an issue with git and signing commits with GPG subkey.
> 
> My setup:
> - master key used for certification only
> - subkey for my main workstation
> - subkey for my mobile workstation (a notebook).
> 
> Both subkeys are used for signing only.
> 
> I've configured git to use my specific subkey however it does not
> work: git config --global user.signingkey = KEYID. Every commit is
> being signed using the newest subkey. I've verified the same behavior
> on three systems (although with the same setup). I've tried to use
> --gpg-sign=KEYID flag, but it does not work either.

I could be wrong, but I think you need to append '!' to
KEYID to force gpg to use that specific signing subkey.

-- 
Todd
~~
A vacuum is a hell of a lot better than some of the stuff that nature
replaces it with.
-- Tennessee Williams

Re: [PATCH] http: fix v1 protocol tests with apache httpd < 2.4

[Todd Zullinger]

Jeff King wrote:
> On Tue, Jan 02, 2018 at 07:39:46PM -0500, Todd Zullinger wrote:
>> I don't know if there's a clean way to do that
>> automatically, short of parsing the output of 'httpd -v'
>> should we ever need to add such a prereq.
> 
> In the general case, we could probably define an endpoint within an 
> block, and then try to access the endpoint from the test script.
> 
> E.g., something like:
> 
> = 2.4>
> Alias /have-2.4.txt www/yes.txt
> 
> 
> in the apache config, and then:
> 
>   test_lazy_prereq APACHE24 '
> echo yes >"$HTTPD_DOCUMENT_ROOT_PATH/yes.txt" &&
> curl -f "$HTTPD_URL/have-2.4.txt"
>   '
> 
> in the test script (of course we may not want to depend on having
> command-line curl, but we could replace that with "git ls-remote" or
> similar).
> 
> One nice thing about that approach is that it can be extended to other
> "If" blocks, like if we have a particular module available, or if ssl is
> configured.

That's quite elegant.  I even modified an IfVersion block
and didn't think about using it that way to create a prereq.
Neat!

-- 
Todd
~~
You're not drunk if you can lie on the floor without holding on.
-- Dean Martin

Re: [PATCH] http: fix v1 protocol tests with apache httpd < 2.4

[Todd Zullinger]

Brandon Williams wrote:
> Seems good to me.  I think I was just being overly cautious when i was
> implementing those patches.  Thanks!

Cool, thanks for taking a look.

In this case, the test isn't dependent on apache > 2.4, but
before I checked that I wondered if we had any way to run a
test only if the apache version was greater or lesser than
some release.  Luckily, I didn't have to work out such a
method. :)

I don't know if there's a clean way to do that
automatically, short of parsing the output of 'httpd -v'
should we ever need to add such a prereq.

-- 
Todd
~~
Sometimes you get the blues because your baby leaves you. Sometimes
you get'em 'cause she comes back.
-- B.B. King

[PATCH] doc/SubmittingPatches: improve text formatting

[Todd Zullinger]

049e64aa50 ("Documentation: convert SubmittingPatches to AsciiDoc",
2017-11-12) changed the `git blame` and `git shortlog` examples given in
the section on sending your patches.

In order to italicize the `$path` argument the commands are enclosed in
plus characters as opposed to backticks.  The difference between the
quoting methods is that backtick enclosed text is not subject to further
expansion.  This formatting makes reading SubmittingPatches in a git
clone a little more difficult.  In addition to the underscores around
`$path` the `--` chars in `git shortlog --no-merges` must be replaced
with `{litdd}`.

Use backticks to quote these commands.  The italicized `$path` is lost
from the html version but the commands can be read (and copied) more
easily by users reading the text version.  These readers are more likely
to use the commands while submitting patches.  Make it easier for them.

Signed-off-by: Todd Zullinger <t...@pobox.com>
---

I missed this in the initial discussion.  It was mentioned by
Junio and brian explained the reasoning for using it in
<20171031012710.jfemqb6ybiog4...@genre.crustytoothpaste.net>:

> > The +fixed width with _italics_ mixed in+ mark-up is something not
> > exactly new, but it is rarely used in our documentation set, so I
> > had to double check by actually seeing how it got rendered, and it
> > looked alright.
>
> I thought it provided some hint to the reader that this wasn't meant to
> be typed literally.  It's a preference of mine and I think it aids in
> readability, but it can be changed if we want.

That's what I had guessed before I looked back at the list
archives.  I understand the desire to make it clear that $path
isn't a literal value.  I think that it's worth losing that subtle
hint in order to make the plain text SubmittingPatches file a
little easier to read.

Whether the people most likely to be considering sending a patch
for git.git are will read the document from a git clone or the
rendered html output is the main question.  Though even readers of
the installed text file in their packaged git will suffer a bit.

It's great having the document in HTML to aid in sharing it's
guidance with others, no doubt.  I've wanted to symlink to
portions of the document numerous times.  I hope a small change to
make it more legible to those who also like plain text will be
welcome.

I considered using backticks for the commands and then +_$path_+
for the argument.  Maybe that's a reasonable compromise?  I.e.:

-+git blame _$path_+ and +git shortlog {litdd}no-merges _$path_+ would help to
+`git blame` +_$path_+ and `git shortlog --no-merges` +_$path_+ would help to

The text version is still a bit strange with that method, but it
certainly separates "$path" from the rest of the command in both
the text and html output. :)

 Documentation/SubmittingPatches | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 3ef30922ec..a1d0feca36 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -261,7 +261,7 @@ not a text/plain, it's something else.
 
 Send your patch with "To:" set to the mailing list, with "cc:" listing
 people who are involved in the area you are touching (the output from
-+git blame _$path_+ and +git shortlog {litdd}no-merges _$path_+ would help to
+`git blame $path` and `git shortlog --no-merges $path` would help to
 identify them), to solicit comments and reviews.
 
 :1: footnote:[The current maintainer: gits...@pobox.com]
-- 
2.16.0.rc0

[PATCH] http: fix v1 protocol tests with apache httpd < 2.4

[Todd Zullinger]

The apache config used by tests was updated to use the SetEnvIf
directive to set the Git-Protocol header in 19113a26b6 ("http: tell
server that the client understands v1", 2017-10-16).

Setting the Git-Protocol header is restricted to httpd >= 2.4, but
mod_setenvif and the SetEnvIf directive work with lower versions, at
least as far back as 2.0, according to the httpd documentation:

https://httpd.apache.org/docs/2.0/mod/mod_setenvif.html

Drop the restriction.  Tested with httpd 2.2 and 2.4.

Signed-off-by: Todd Zullinger <t...@pobox.com>
---

These tests fail with 2.16.0-rc0 on CentOS-6, which uses
httpd-2.2.

I removed the version restriction entirely rather than adjust
the version.  I believe SetEnvIf works on httpd >= 2.0.  I'm
not sure if we aim to support anything less than httpd 2.0,
but I'm betting not.  If that's incorrect, I can add some
IfVersion conditions.

As noted in the commit message, I only tested with httpd 2.2
and 2.4 (on CentOS 6/7 and Fedora 26-28).  If anyone has older
httpd systems which need support, it would be great if they
could test this before 2.16.0 is finalized, so we can avoid
shipping with any failing tests.

 t/lib-httpd/apache.conf | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index df19436314..724d9ae462 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -25,6 +25,9 @@ ErrorLog error.log
 
LoadModule headers_module modules/mod_headers.so
 
+
+   LoadModule setenvif_module modules/mod_setenvif.so
+
 
 
 LockFile accept.lock
@@ -67,9 +70,6 @@ LockFile accept.lock
 
LoadModule unixd_module modules/mod_unixd.so
 
-
-   LoadModule setenvif_module modules/mod_setenvif.so
-
 
 
 PassEnv GIT_VALGRIND
@@ -79,9 +79,7 @@ PassEnv ASAN_OPTIONS
 PassEnv GIT_TRACE
 PassEnv GIT_CONFIG_NOSYSTEM
 
-= 2.4>
-   SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
-
+SetEnvIf Git-Protocol ".*" GIT_PROTOCOL=$0
 
 Alias /dumb/ www/
 Alias /auth/dumb/ www/auth/dumb/
-- 
2.16.0.rc0

Re: [PATCH] Add shell completion for git remote rm

[Todd Zullinger]

Ævar Arnfjörð Bjarmason wrote:
>> I think adding 'rm' to completion definitely counts as advertisement.
>> It doesn't have much practical use, after all: typing 'rm' with
>> completion is actually one more keystroke than without (rm vs. rm).
> 
> This is only one use of the completion interface, maybe you only use it
> like that, but not everyone does.
> 
> The completion interface has two uses, one is to actually save you
> typing, the other is subcommand discovery, and maybe someone who has a
> use neither you or I have thought of is about to point out a third.
> 
> I'll type "git $whatever $subcommand" as *validation* that I've
> found the right command, not to complete it for me. This is a thing
> that's in my muscle memory for everything.

Is that meant to be in favor of including rm in the
completion results or against? :)

> Since I've been typing "git remote rm" for a while (started before
> this deprecation happened) I've actually been meaning to submit
> completion for "rm" since it works, not knowing about Duy's patch until
> now.
> 
> Now, even if someone disagrees that we should have "rm" at all I think
> that in general we should not conflate two different things, one is
> whether:
> 
> git remote 
> 
> shows both "rm" and "remove" in the list, and the other is whether:
> 
> git remote rm
> 
> Should yield:
> 
> git remote rm
> 
> Or, as now happens:
> 
> git remote rm
> 
> I can see why we'd, in general, we'd like to not advertise certain
> options for completion (due to deprecation, or just to avoid verbosity),
> but complete them once they're unambiguously typed out.
> 
> I don't know whether the bash completion interface supports making that
> distinction, but it would be useful.

It can be done, though I think that it's probably better to
subtly lead people to use 'git remote remove' going forward,
to keep things consistent.  I don't have a strong preference
for or against the rm -> remove change, but since that's
been done I think there's a benefit to keeping things
consistent in the UI.  And I think that should also apply to
not offering completion for commands/subcommands/options
which are only kept for backward compatibility.

Here's one way to make 'git remote rm ' work without
including it in the output of 'git remote ':

diff --git i/contrib/completion/git-completion.bash 
w/contrib/completion/git-completion.bash
index 3683c772c5..aa63f028ab 100644
--- i/contrib/completion/git-completion.bash
+++ w/contrib/completion/git-completion.bash
@@ -2668,7 +2668,9 @@ _git_remote ()
add rename remove set-head set-branches
get-url set-url show prune update
"
-   local subcommand="$(__git_find_on_cmdline "$subcommands")"
+   # Don't advertise rm by including it in subcommands, but complete
+   # remotes if it is used.
+   local subcommand="$(__git_find_on_cmdline "$subcommands rm")"
if [ -z "$subcommand" ]; then
case "$cur" in
--*)

Keeping 'git remote rm' working to avoid breaking scripts is
one thing, but having it in the completion code makes it
more likely that it will continue to be seen as a
recommended subcommand.

This leads to patches like this one, where it's presumed
that the lack of completion is simply an oversight or a bug.
Of course, the lack of completion hasn't caused everyone to
forget that 'remote rm' was changed to 'remote remove', so
that reasoning may be full of hot air (or worse). ;)

The current result of 'git remote rm ' isn't so great.
It's arguably worse to have it pretend that no subcommand
was given than to list the remotes.

$ git remote rm 
addremove set-head   update
get-urlrename set-url
prune  set-branches   show

I think completing nothing or completing the remotes
(without offering rm in the subcommand list) would be
better, after looking at it a bit.

I don't know how to disable file completion, but I'm not
intimately familiar with the git completion script (thanks
to it working so damn well).  I'm guessing there's a way to
do that, if there's a strong desire to not complete the
remotes at all.

I don't think we should include rm in 'git remote '
completion, but I don't care much either way what 'git
remote rm ' includes.  But it should be better than
including the other subcommands.

-- 
Todd
~~
There, I've gone and soiled myself, are you happy now?!
-- Stewie Griffin

Re: [PATCH] diff-tree: obey the color.ui configuration

[Todd Zullinger]

Ævar Arnfjörð Bjarmason wrote:
> No idea how to test this, in particular trying to pipe the output of
> color.ui=never v.s. color.ui=auto to a file as "auto" will disable
> coloring when it detects a pipe, but this fixes the issue.

You might be able to use similar methods as those Jeff used
in the series merged from jk/ui-color-always-to-auto:

https://github.com/gitster/git/tree/jk/ui-color-always-to-auto

He may also have some ideas about this issue in general.
(Or they could be tramatic memories, depending on how
painful it was to dig into the color code.)

-- 
Todd
~~
Subtlety is the art of saying what you think and getting out of the
way before it is understood.
-- Anonymous

Re: [PATCH] Add shell completion for git remote rm

[Todd Zullinger]

Keith Smiley wrote:
> It looks like that was just about preferring remove in documentation
> and the like, I think it would still make sense to have both for
> completion since rm is still supported.

I read it as a first step in a long process to eventually
remove 'remote rm', but if that's never intended, then sure,
restoring completion for it seems reasonable.

It would be good to hear from those who know or recall the
intention.

I think we should only complete the preferred subcommand.
That encourages use of 'remote remove' even if 'remote rm'
will stay forever to avoid breaking existing scripts.

If it does make sense to restore completion, adding a link
back to e17dba8fe1 and explaining why the completion is
being restored would be good.  Reading the commit message
now makes it sound like 'remote rm' was never present and is
being added to correct an oversight.

Maybe something like:

completion: restore 'remote rm'

e17dba8fe1 ("remote: prefer subcommand name 'remove' to
'rm'", 2012-09-06) removed the 'rm' subcommand from
completion.  The 'remote rm' subcommand is still supported
and not planned to be removed.  Offer completions for it.

I also noticed that in your original commit that you say
"list of removes as remove does." That should be "remotes"
instead of "removes" there. -- I've made that typo myself
quite often. :)

-- 
Todd
~~
There are no stupid questions, but there are a LOT of inquisitive
idiots.
-- Demotivators (www.despair.com)

Re: [PATCH] Add shell completion for git remote rm

[Todd Zullinger]

Hi Keith,

Keith Smiley wrote:
> Previously git remote rm did not complete your list of removes as remove
> does.

Looking through the history, the rm subcomand completion was
explicitly removed in e17dba8fe1 ("remote: prefer subcommand
name 'remove' to 'rm'", 2012-09-06).

-- 
Todd
~~
Genius is 1% inspiration and 99% perspiration, which is why engineers
sometimes smell really bad.
-- Demotivators (www.despair.com)

Re: [PATCH v6] Makefile: replace perl/Makefile.PL with simple make rules

[Todd Zullinger]

Ævar Arnfjörð Bjarmason wrote:
> On Wed, Dec 20, 2017 at 6:41 PM, Todd Zullinger <t...@pobox.com> wrote:
>> /usr/share/perl5/vendor_perl/Git
>> /usr/share/perl5/vendor_perl/Git.pm
>> /usr/share/perl5/vendor_perl/Git/Error.pm
>> [...]
>> /usr/share/perl5/vendor_perl/build
>> /usr/share/perl5/vendor_perl/build/lib
>> /usr/share/perl5/vendor_perl/build/lib/Git
>> /usr/share/perl5/vendor_perl/build/lib/Git.pm
>> /usr/share/perl5/vendor_perl/build/lib/Git/Error.pm
>> [...]
>> Note that not all of the .pm files are matched, which I
>> believe is due to the glob matches only going 4 levels deep
>> under the perl dir.
> 
> Ouch, that's a stupid mistake of mine. Didn't consider that changing
> it from *.pm to *.pmc would of course impact that glob match.
> 
> This fixes it, changes against v5:
> 
> @@ -224,7 +224,7 @@
>   po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
> $(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<
>   
> -+LIB_PERL := $(wildcard perl/*.pm perl/*/*.pm perl/*/*/*.pm 
> perl/*/*/*/*.pm)
> ++LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm 
> perl/Git/*/*/*.pm)
>  +LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
>  +
>  +ifndef NO_PERL
> 
> I.e. let's keep calling it "build" for consistency with other stuff
> and so "ls" will show it, but just alter the glob so we'll only match
> modules like Git{,::*}. I don't think we'll ever add anything outside
> that namespace, so this seems like the best solution.

Sounds good.  While it might not have been too bad to have a
hidden dir for build artifacts, using the more explicit glob
pattern is much nicer.

I'll use this locally and let you know if I notice any
issues.  Thanks for working on this.

-- 
Todd
~~
Some people never go crazy. What truly horrible lives they must
live.
-- Charles Bukowski

Re: [PATCH v5] Makefile: replace perl/Makefile.PL with simple make rules

[Todd Zullinger]

Ævar Arnfjörð Bjarmason wrote:
> [Sorry for not CC-ing you on the last version. Forgot to update the
> giant send-email line I'm juggling for this series].

No worries.  It is a rather large CC list at this point. :)

> This *.pmc thing is just me being overly clever. Here's a v5 that gets
> rid of it. Diff with v4:

Ahh, thanks for the additional details on this.

> -+PMFILES := $(wildcard perl/*.pm perl/*/*.pm perl/*/*/*.pm 
> perl/*/*/*/*.pm)
> -+PMCFILES := $(patsubst perl/%.pm,perl/build/lib/%.pmc,$(PMFILES))
> ++LIB_PERL := $(wildcard perl/*.pm perl/*/*.pm perl/*/*/*.pm 
> perl/*/*/*/*.pm)
> ++LIB_PERL_GEN := $(patsubst 
> perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
>  +
>  +ifndef NO_PERL
> -+all:: $(PMCFILES)
> ++all:: $(LIB_PERL_GEN)
>  +endif
>  +
> -+perl/build/lib/%.pmc: perl/%.pm
> ++perl/build/lib/%.pm: perl/%.pm
>  +  $(QUIET_GEN)mkdir -p $(dir $@) && \
>  +  sed -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' < $< > $@
>  +

Without the .pmc extensions, rpm picks up the perl
dependencies, which is good.  But an additional build/lib
dir is created, which ends up in $perllibdir after install.

Here's an extract from a local build:

mkdir -p perl/build/lib/build/lib/ && \
sed -e 's|@@LOCALEDIR@@|/usr/share/locale|g' < perl/build/lib/Git.pm > 
perl/build/lib/build/lib/Git.pm
mkdir -p perl/build/lib/build/lib/Git/ && \
sed -e 's|@@LOCALEDIR@@|/usr/share/locale|g' < perl/build/lib/Git/IndexInfo.pm 
> perl/build/lib/build/lib/Git/IndexInfo.pm
mkdir -p perl/build/lib/build/lib/Git/ && \
sed -e 's|@@LOCALEDIR@@|/usr/share/locale|g' < perl/build/lib/Git/SVN.pm > 
perl/build/lib/build/lib/Git/SVN.pm
mkdir -p perl/build/lib/build/lib/Git/ && \
sed -e 's|@@LOCALEDIR@@|/usr/share/locale|g' < perl/build/lib/Git/Error.pm > 
perl/build/lib/build/lib/Git/Error.pm
mkdir -p perl/build/lib/build/lib/Git/ && \
sed -e 's|@@LOCALEDIR@@|/usr/share/locale|g' < perl/build/lib/Git/I18N.pm > 
perl/build/lib/build/lib/Git/I18N.pm

When PMFILES and PMCFILES matched .pm and .pmc respectively,
the glob didn't match duplicated files under build/lib, so
this wasn't an issue.  I'm not sure where this is best
fixed.  The build/lib dir could be moved outside of perl or
it could be made .build/lib to avoid the wildcard match,
perhaps.

Building with perllibdir=/usr/share/perl5/vendor_perl
results in:

/usr/share/perl5/vendor_perl/Git
/usr/share/perl5/vendor_perl/Git.pm
/usr/share/perl5/vendor_perl/Git/Error.pm
/usr/share/perl5/vendor_perl/Git/FromCPAN
/usr/share/perl5/vendor_perl/Git/FromCPAN/Error.pm
/usr/share/perl5/vendor_perl/Git/I18N.pm
/usr/share/perl5/vendor_perl/Git/IndexInfo.pm
/usr/share/perl5/vendor_perl/Git/SVN
/usr/share/perl5/vendor_perl/Git/SVN.pm
/usr/share/perl5/vendor_perl/Git/SVN/Editor.pm
/usr/share/perl5/vendor_perl/Git/SVN/Fetcher.pm
/usr/share/perl5/vendor_perl/Git/SVN/GlobSpec.pm
/usr/share/perl5/vendor_perl/Git/SVN/Log.pm
/usr/share/perl5/vendor_perl/Git/SVN/Memoize
/usr/share/perl5/vendor_perl/Git/SVN/Memoize/YAML.pm
/usr/share/perl5/vendor_perl/Git/SVN/Migration.pm
/usr/share/perl5/vendor_perl/Git/SVN/Prompt.pm
/usr/share/perl5/vendor_perl/Git/SVN/Ra.pm
/usr/share/perl5/vendor_perl/Git/SVN/Utils.pm
/usr/share/perl5/vendor_perl/build
/usr/share/perl5/vendor_perl/build/lib
/usr/share/perl5/vendor_perl/build/lib/Git
/usr/share/perl5/vendor_perl/build/lib/Git.pm
/usr/share/perl5/vendor_perl/build/lib/Git/Error.pm
/usr/share/perl5/vendor_perl/build/lib/Git/I18N.pm
/usr/share/perl5/vendor_perl/build/lib/Git/IndexInfo.pm
/usr/share/perl5/vendor_perl/build/lib/Git/SVN.pm

Note that not all of the .pm files are matched, which I
believe is due to the glob matches only going 4 levels deep
under the perl dir.

Thanks,

-- 
Todd
~~
Never do today that which will become someone else's responsibility
tomorrow.

Re: [PATCH v4] Makefile: replace perl/Makefile.PL with simple make rules

[Todd Zullinger]

Hi Ævar,

Ævar Arnfjörð Bjarmason wrote:
> Here's a hopefully final version. The only difference with v3 is:
> 
> -local @_ = ($caller, @_);
> +unshift @_, $caller;
> 
> As it turns out localizing @_ isn't something that worked properly
> until
> https://github.com/Perl/perl5/commit/049bd5ffd62b73325d4b2e75e59ba04b3569137d
> 
> That commit isn't part of the 5.16.3 version that ships with CentOS 7,
> which explains why Michael J Gruber had issues with it. I've tested
> this on CentOS 7 myself, it passes all tests now.

Thanks for tracking this down!

FWIW, I applied this version to next and tested it with
CentOS 6 and 7.  The tests pass on both (though there are
some unrelated failures on CentOS 6 in t5700-protocol-v1,
which I haven't looked into further yet).

I also applied this patch to 2.15.1 and ran the tests in the
Fedora build system for all fedora and epel releases, which
also passed (though with some spurious git-svn failures on
x86_64 in fedora 28, AKA rawhide).

The .pmc extensions seem to cause rpm to fail to parse the
files for rpm 'provides' as it normally would.  This causes
scripts like git-send-email which generates a 'requires' on
Git::Error to fail to find anything which provides it.

I'm not familiar with the .pmc extenstion.  Searching the
fedora repositories, there is only one other package - and
one file within it - which has a .pmc extension.

(The package is perl-test, the file is
/usr/libexec/perl5-tests/perl-tests/t/run/flib/t2.pmc.)

Perhaps it's a bug in rpm's perl dependency generator, but
I'd like to think that git wouldn't be the first package to
find it.

Is the .pmc extension important to ensure these files are
loaded in the right order?  Since they're all in the Git
namespace, I don't imagine there should be anything else in
@INC which would be provided by the system or another
package.

Pardon my ignorance if I've missed the obvious (I haven't
fully read "perldoc -f require" which you referenced in the
commit message).

-- 
Todd
~~
Suppose I were a member of Congress, and suppose I were an idiot. But,
I repeat myself.
-- Mark Twain

Re: [PATCH v3] Makefile: replace perl/Makefile.PL with simple make rules

[Todd Zullinger]

Hi Michael,

Michael J Gruber wrote:
> This patch (currently in origin/next) makes a ton of tests from our test
> suite fail for me on pretty standard systems (Fedora 27, CentOS 7.4.1708).
> 
> Is there anything I'm supposed to do differently now to make our test
> suite run? If yes then a clear and short hint in the patch description
> would me more than approriate.

Interesting.  I'll have to try building next.  I was going
to wait until the first 2.16.0 rc for a full test.

I did apply this patch on top of 2.15.1 on 12/10 and built
an rpm locally for fedora (only f26 though).  I didn't see
any test failures, which is why I thought I'd wait for
2.16.0-rc0 to test further.

FWIW, the minor spec file changes I made (against fedora's
git package master branch) are here:

https://src.fedoraproject.org/fork/tmz/rpms/git/branch/perl-makefile

The only change in the patch since I tested it is: 

+-modules += Git/Packet

in perl/Makefile.

I don't think any of these changes to the rpm spec file or
the Git/Packet addition in modules look like they'd affect
the test suite, but it's early here and I could be wrong.

I'll try to test a build of next soon to see if I get
similar failures on Fedora/CentOS.

-- 
Todd
~~
After one look at this planet any visitor from outer space would say
"I want to see the manager."
-- William S. Burroughs

Re: [PATCH] git-svn: convert CRLF to LF in commit message to SVN

[Todd Zullinger]

Hi Brian,

Bennett, Brian wrote:
> Thank you for your fast response,
> 
> I haven't done a build of this type before (so I could
> test the patch first) so I'm trying to do that and get
> this far:
...
> I don't want to drag out testing the patch, so if either
> of you are able to quickly guide me on what I am doing
> incorrectly I am willing to get the build done so I can
> test it. If not, could one of you build with the patch and
> somehow get that to me so I could test?

I don't know about building git for windows, but since the
git-svn command is a perl script, it might be easier to just
patch that file.  I think you can find the path where
git-svn is installed using: git --exec-path

For this one-liner, I'd just manually apply it.

(If you want to use 'git apply' or the patch command, you'll
have to edit the patch to adjust the name of the file, as
it's git-svn.perl in the git tree.  The .perl suffix is
dropped in the installed version.)

Hopefully that makes it easier for you to test Eric's patch.

-- 
Todd
~~
I am in shape.  Round is a shape.

Re: [PATCH 0/2] t/lib-git-svn.sh: improve svnserve tests with parallel make test

[Todd Zullinger]

Junio C Hamano wrote:
> Todd Zullinger <t...@pobox.com> writes:
>> I wanted to check if this minor patch series had slipped
>> under your radar.
> 
> Totally.  Queued.
> 
> As they come with Ack by the area maintainer, I'll fast-track them
> down to 'master' (other topics typically cook at least for a week in
> 'next').
> 
> Thanks for pinging.

Thank you, as always.

-- 
Todd
~~
There is considerable overlap between the intelligence of the smartest
bears and the dumbest tourists.
  -- Park ranger yro.slashdot.org/comments.pl?sid=191810=15757347

[PATCH 0/2] t/lib-git-svn.sh: improve svnserve tests with parallel make test

[Todd Zullinger]

Hi Junio,

I wanted to check if this minor patch series had slipped
under your radar.  If it's waiting patiently in your queue
for other topics to advance, that's fine, of course. :)

Finished patches: <20171201155653.29553-1-...@pobox.com> and
<20171201155653.29553-2-...@pobox.com>.

Thanks,

-- 
Todd
~~
Anyone who is capable of getting themselves made President should on
no account be allowed to do the job.
-- Douglas Adams, "The Hitchhiker's Guide to the Galaxy"

[PATCH] RelNotes: minor typo fixes in 2.16.0 draft

[Todd Zullinger]

Signed-off-by: Todd Zullinger <t...@pobox.com>
---
 Documentation/RelNotes/2.16.0.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/RelNotes/2.16.0.txt 
b/Documentation/RelNotes/2.16.0.txt
index 3eeeb83674..f7fca7123f 100644
--- a/Documentation/RelNotes/2.16.0.txt
+++ b/Documentation/RelNotes/2.16.0.txt
@@ -304,10 +304,10 @@ Fixes since v2.15
  * "git branch --set-upstream" has been deprecated and (sort of)
removed, as "--set-upstream-to" is the preferred one these days.
The documentation still had "--set-upstream" listed on its
-   synopsys section, which has been corrected.
+   synopsis section, which has been corrected.
(merge a060f3d3d8 tz/branch-doc-remove-set-upstream later to maint).
 
- * Internaly we use 0{40} as a placeholder object name to signal the
+ * Internally we use 0{40} as a placeholder object name to signal the
codepath that there is no such object (e.g. the fast-forward check
while "git fetch" stores a new remote-tracking ref says "we know
there is no 'old' thing pointed at by the ref, as we are creating
-- 
2.15.1

Re: [PATCH] t/helper: ignore everything but sources

[Todd Zullinger]

Hi Stefan,

Stefan Beller wrote:
>> If we ignore everything but resurrect *.[ch] with negative exclude
>> rules, can we do the same without moving things around?
> 
> Yes, there is also one lonely shell script in there, which also needs
> exclusion.

There aren't currently any .h files, but I suppose it doesn't hurt to
include that pattern to be safer for the future.

> +*
> +!.sh
> +!.[ch]

The ! patterns are missing a '*'.  I think it should be:

*
!*.[ch]
!*.sh

Does it make sense to also include !.gitignore as well?
It's already committed, so it's not ignored.  But perhaps
having it listed will save someone from getting their repo
into a state where local changes to .gitignore aren't picked
up (I know that's a bit of a stretch).

-- 
Todd
~~
How much does it cost to entice a dope-smoking UNIX system guru to
Dayton?
-- Brian Boyle, UNIX/WORLD's First Annual Salary Survey

Re: [PATCH] doc: clarify usage of XDG_CONFIG_HOME config file

[Todd Zullinger]

Hi Jacob,

Jacob Keller wrote:
> The documentation for git config and how it reads the user specific
> configuration file is misleading. In some places it implies that
> $XDG_CONFIG_HOME/git/config will always be read. In others, it implies
> that only one of ~/.gitconfig and $XDG_CONFIG_HOME/git/config will be
> read.
> 
> Improve the documentation explaining how the various configuration files
> are read, and combined.
> 
> Instead of referencing each file individually, reference each type of
> location git will check. When discussing the user configuration, explain
> how we switch between one of three choices. Ensure to note that only one
> of the three choices is used.

Perhaps it would read a little easier as "Make it clear ..."
rather than "Ensure to note that ..." ?

> +Note that git will only ever use one of these files as the global user
> +configuration file at once. Additionally if you sometimes use an older 
> version
> +of git, it is best to only rely on `~/.gitconfig` as support for the others 
> was
> +added fairly recently.

Is it really accurate to say these were added fairly
recently?  It looks like XDG_CONFIG_HOME was added in
21cf322791 ("config: read (but not write) from
$XDG_CONFIG_HOME/git/config file", 2012-06-22) and
0e8593dc5b ("config: write to $XDG_CONFIG_HOME/git/config
file when appropriate", 2012-06-22) which are in 1.7.12.

Would it be better to say something like "if you sometimes
use a version of git prior to 1.7.12" here?

Or maybe we can drop "Additionally ..." altogether now?
Someone using a 5 year old git version sometimes will
hopefully know to check the documentation for that older
version.

-- 
Todd
~~
Now don't say you can't swear off drinking; it's easy. I've done it a
thousand times.
-- W.C. Fields

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Todd Zullinger]

Jeff Hostetler wrote:

I'm looking at t5616 now on my mac.
Looks like the MAC doesn't like my line counting in the tests.
I'll fix in my next version.


Perhaps that's as simple as using the test_line_counts helper?

diff --git i/t/t5616-partial-clone.sh w/t/t5616-partial-clone.sh
index fa573f8cdb..6d673de90c 100755
--- i/t/t5616-partial-clone.sh
+++ w/t/t5616-partial-clone.sh
@@ -19,7 +19,7 @@ test_expect_success 'setup normal src repo' '
git -C src ls-files -s file.$n.txt >>temp
done &&
awk -f print_2.awk expect_1.oids &&
-   test "$(wc -l expect.blame
'

@@ -100,7 +100,7 @@ test_expect_success 'partial fetch inherits filter 
settings' '
# it should be in a new packfile (since the promisor boundary is
# currently a packfile, it should not get unpacked upon receipt.)
test_expect_success 'verify diff causes dynamic object fetch' '
-   test "$(wc -l observed.oids &&
cat observed.oids &&
-   test "$(wc -l 

Re: What's cooking in git.git (Dec 2017, #01; Mon, 4)

[Todd Zullinger]

Johannes Schindelin wrote:

That is not the only thing going wrong:

https://travis-ci.org/git/git/builds/312551566

It would seem that t9001 is broken on Linux32, t5616 is broken on macOS,
and something really kinky is going on with the GETTEXT_POISON text, as it
seems to just abort while trying to run t6120.


I thought the verbose logs from the test might be useful, but looking
at the travis output for that job[1], there's an unrelated problem
preventing the ci/print-test-failures.sh script from running properly:

   $ ci/print-test-failures.sh
   cat: t/test-results/t1304-default-acl.exit: Permission denied
   
   t/test-results/t1304-default-acl.out...
   
   cat: t/test-results/t1304-default-acl.out: Permission denied

[1] https://travis-ci.org/git/git/jobs/312551595#L2185

I didn't see the same failure for other build targets at a glance, so
the permission issue might only be a problem for the linux32 builds.

--
Todd
~~
A diplomat is a person who can tell you to go to Hell in such a way
that you actually look forward to the trip.
   -- Anonymous

Re: [PATCH] Add a sample hook which saves push certs as notes

[Todd Zullinger]

Hi Shikher,

I'm not familiar with push certs, but I did notice some general issues
in the sample hook.  I hope they're helpful.

Shikher Verma wrote:

index 0..b4366e43f
--- /dev/null
+++ b/templates/hooks--post-receive.sample
+#!/bin/sh

...

+if test -z GIT_PUSH_CERT ; then
+exit 0
+fi


The $ is missing from GIT_PUSH_CERT.  test -z GIT_PUSH_CERT will
always be false. :)

The variable should also be quoted.  Not all sh implementations accept
a missing argument to test -z, as bash does.

More minor, Documentation/CodingGuidelines suggests placing 'then' on
a new line:

   if test -z "$GIT_PUSH_CERT"
   then
   exit 0
   fi

(There is plenty of code that doesn't follow that, so I don't know how
strong that preference is.)

This could also be written as:

   test -z "$GIT_PUSH_CERT" && exit 0

I don't know if there's any general preference to shorten it in git's
code or not.


+push_cert=$(git cat-file -p  $GIT_PUSH_CERT)


Very minor: there's an extra space before the variable here.

(I also noticed the tests which use $GIT_PUSH_CERT, like t5534, use
'cat-file blob ...' rather than 'cat-file -p ...'.  I don't know if
that's much safer/better than letting cat-file guess the object type
in the hook.  I have no idea if there's a chance that "$GIT_PUSH_CERT"
has some unexpected, non-blob object type.)


+while read oval nval ref
+do
+   # Verify that the ref update matches that in push certificate.
+   if [[ $push_cert == *$oval" "$nval" "$ref* ]]; then


[[ isn't portable across all the sh implementations git strives to
support, as far as I know.

The minor point about 'then' on new line is applicable here too.  It
would also better match the outer 'while' loop.


+   # add the push cert as note (namespaced pushcerts) to nval.
+   git notes --ref=pushcerts add -m "$push_cert" $nval -f
+   fi
+done


--
Todd
~~
Learn from the mistakes of others--you can never live long enough to
make them all yourself.
   -- John Luther

[PATCH v2 2/2] t/lib-git-svn.sh: improve svnserve tests with parallel make test

[Todd Zullinger]

Setting SVNSERVE_PORT enables several tests which require a local
svnserve daemon to be run (in t9113 & t9126).  The tests share setup of
the local svnserve via `start_svnserve()`.  The function uses svnserve's
`--listen-once` option, which causes svnserve to accept one connection
on the port, serve it, and exit.  When running the tests in parallel
this fails if one test tries to start svnserve while the other is still
running.

Use the test number as the svnserve port (similar to httpd tests) to
avoid port conflicts.  Developers can set GIT_TEST_SVNSERVE to any value
other than 'false' or 'auto' to enable these tests.

Acked-by: Eric Wong <e...@80x24.org>
Reviewed-by: Jonathan Nieder <jrnie...@gmail.com>
Signed-off-by: Todd Zullinger <t...@pobox.com>
---
 t/lib-git-svn.sh | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 84366b2624..4c1f81f167 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -110,14 +110,16 @@ EOF
 }
 
 require_svnserve () {
-   if test -z "$SVNSERVE_PORT"
+   test_tristate GIT_TEST_SVNSERVE
+   if ! test "$GIT_TEST_SVNSERVE" = true
then
-   skip_all='skipping svnserve test. (set $SVNSERVE_PORT to 
enable)'
+   skip_all='skipping svnserve test. (set $GIT_TEST_SVNSERVE to 
enable)'
test_done
fi
 }
 
 start_svnserve () {
+   SVNSERVE_PORT=${SVNSERVE_PORT-${this_test#t}}
svnserve --listen-port $SVNSERVE_PORT \
 --root "$rawsvnrepo" \
 --listen-once \
-- 
2.15.1

[PATCH v2 1/2] t/lib-git-svn: cleanup inconsistent tab/space usage

[Todd Zullinger]

Acked-by: Eric Wong <e...@80x24.org>
Reviewed-by: Jonathan Nieder <jrnie...@gmail.com>
Signed-off-by: Todd Zullinger <t...@pobox.com>
---
 t/lib-git-svn.sh | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 688313ed5c..84366b2624 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -17,8 +17,8 @@ SVN_TREE=$GIT_SVN_DIR/svn-tree
 svn >/dev/null 2>&1
 if test $? -ne 1
 then
-skip_all='skipping git svn tests, svn not found'
-test_done
+   skip_all='skipping git svn tests, svn not found'
+   test_done
 fi
 
 svnrepo=$PWD/svnrepo
@@ -110,18 +110,18 @@ EOF
 }
 
 require_svnserve () {
-if test -z "$SVNSERVE_PORT"
-then
-   skip_all='skipping svnserve test. (set $SVNSERVE_PORT to enable)'
-test_done
-fi
+   if test -z "$SVNSERVE_PORT"
+   then
+   skip_all='skipping svnserve test. (set $SVNSERVE_PORT to 
enable)'
+   test_done
+   fi
 }
 
 start_svnserve () {
-svnserve --listen-port $SVNSERVE_PORT \
- --root "$rawsvnrepo" \
- --listen-once \
- --listen-host 127.0.0.1 &
+   svnserve --listen-port $SVNSERVE_PORT \
+--root "$rawsvnrepo" \
+--listen-once \
+--listen-host 127.0.0.1 &
 }
 
 prepare_a_utf8_locale () {
-- 
2.15.1

Re: [PATCH 2/2] t/lib-git-svn.sh: improve svnserve tests with parallel make test

[Todd Zullinger]

Jonathan Nieder wrote:
Yep, with this description it is 
Reviewed-by: Jonathan Nieder 


Thanks for putting up with my nits. :)


Thank you for taking the time and looking at the details. :)

I'll send a v2 with the changes in the morning, in case there are any 
other comments (but mostly because it's late and time for a swim).


--
Todd
~~
It is impossible to enjoy idling thoroughly unless one has plenty of
work to do.
   -- Jerome K. Jerome