Re: [PATCH 5/8] pathspec: convert strip_submodule_slash to take an index

[Junio C Hamano]

Brandon Williams  writes:

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

This conversion is not wrong per-se, but I wonder if there is a
practical situation where we want to validate a pathspec element
against an in-core index that does not represent the index currently
in effect (aka "the_index").  I do not think of any offhand myself.

Re: [PATCH 4/8] pathspec: rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP

[Junio C Hamano]

Brandon Williams  writes:

> Now that there is only a single flag which strips a submodule slash,
> rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to
> PATHSPEC_STRIP_SUBMODULE_SLASH.

This is a logical follow-up to the previous step.

Re: [PATCH 3/8] pathspec: change PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag

[Junio C Hamano]

Brandon Williams  writes:

> It's confusing to have two different 'strip submodule slash' flags which
> do subtly different things.  Both
> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and
> PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP will accomplish the same task of
> striping a slash from a pathspec which matches a submodule entry in the
> index.  The only difference is that
> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE will perform additional checks
> and die if a pathspec has a leading path component which corresponds to
> a submodule.  This additional functionality should be split out into its
> own flag.
>
> To this end, rename the PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag to
> PATHSPEC_SUBMODULE_LEADING_PATH and change its behavior to only die if a
> path descends into a submodule.  In addition add the
> PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to callers which relied on the
> old slash stripping functionality.

"PATHSPEC_SUBMODULE_LEADING_PATH" feels like an unfinished sentence
to me.  Do I understand your description correctly if I say it is
about "checking" the leading path to see if it overlaps with a
submodule path?  IOW, I am wondering if the name should have the
word CHECK somewhere in it.

Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s

[Mike Hommey]

On Wed, May 10, 2017 at 12:33:44AM -0400, Jeff King wrote:
> On Tue, May 09, 2017 at 09:22:11PM -0700, Shawn Pearce wrote:
> 
> > > Hmm. That makes sense generally, as the request should succeed. But it
> > > seems like we're creating a client that will sometimes succeed and
> > > sometimes fail, and the reasoning will be somewhat opaque to the user.
> > > I have a feeling I'm missing some context on when you'd expect this to
> > > kick in.
> > 
> > Specifically, someone I know was looking at building an application
> > that is passed only a SHA-1 for the tip of a ref on a popular hosting
> > site[1]. They wanted to run `git fetch URL SHA1`, but this failed
> > because the site doesn't have upload.allowtipsha1inwant enabled.
> > However the SHA1 was clearly in the output of git ls-remote.
> 
> OK. So this is basically a case where we expect that the user knows what
> they're doing.
> 
> > For various reasons they expected this to work, because it works
> > against other sites that do have upload.allowtipsha1inwant enabled.
> > And I personally just expected it to work because the fetch client
> > accepts SHA-1s, and the wire protocol uses "want SHA1" not "want ref",
> > and the SHA-1 passed on the command line was currently in the
> > advertisement when the connection opened, so its certainly something
> > the server is currently willing to serve.
> 
> Right, makes sense.  I wondered if GitHub should be turning on
> allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide
> some internal refs[1], and they're things that users wouldn't want to
> fetch. The problem for your case really is just on the client side, and
> this patch fixes it.

More broadly, I think it is desirable that any commit that can be
reached from public refs can be fetched by an explicit sha1 without
allowTipSHA1InWant.

Mike

What's cooking in git.git (May 2017, #03; Wed, 10)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

Git 2.13, together with maintenance releases for 9 maintenance
tracks, has been tagged.  Let's wait for a few days to see if users
find embarrassing regressions and then start the next cycle.  In the
meantime, please pay closer attention to the topics that have been
stalled or waiting to be reviewed for too long and think of ways to
help moving them forward, before you start sending your new shiny
toys to the list.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* jh/verify-index-checksum-only-in-fsck (2017-04-27) 1 commit
  (merged to 'next' on 2017-04-28 at afb4c70061)
 + t1450: avoid use of "sed" on the index, which is a binary file

 Update an unportable constructin a new test.

--
[New Topics]

* ab/compat-regex-update (2017-05-09) 2 commits
 - compat/regex: update the gawk regex engine from upstream
 - compat/regex: add a README with a maintenance guide

 Will merge to 'next'.


* jc/apply-fix-mismerge (2017-05-08) 1 commit
  (merged to 'next' on 2017-05-09 at e0b89532d0)
 + apply.c: fix whitespace-only mismerge

 Will merge to 'master'.


* jt/push-options-doc (2017-05-10) 2 commits
 - receive-pack: verify push options in cert
 - docs: correct receive.advertisePushOptions default

 The receive-pack program now makes sure that the push certificate
 records the same set of push options used for pushing.

 Will merge to 'next'.


* dt/unpack-save-untracked-cache-extension (2017-05-09) 1 commit
 - DONTMERGE: unpack-trees: preserve index extensions

--
[Stalled]

* mg/status-in-progress-info (2017-04-14) 1 commit
 - status: show in-progress info for short status

 "git status" learns an option to report various operations
 (e.g. "merging") that the user is in the middle of.

 It is still unclear if the participants decided that it is OK to
 spell "--inprogress" as a single word.


* mg/name-rev-debug (2017-03-31) 4 commits
 - describe: pass --debug down to name-rev
 - name-rev: provide debug output
 - name-rev: favor describing with tags and use committer date to tiebreak
 - name-rev: refactor logic to see if a new candidate is a better name

 "git describe --debug --contains" did not add any meaningful
 information, even though without "--contains" it did.

 Have been expecting a reroll of the tip two, but it has not seen
 any activity for too long.
 cf. 


* nd/worktree-move (2017-04-20) 6 commits
 - worktree remove: new command
 - worktree move: refuse to move worktrees with submodules
 - worktree move: accept destination as directory
 - worktree move: new command
 - worktree.c: add update_worktree_location()
 - worktree.c: add validate_worktree()

 "git worktree" learned move and remove subcommands.

 Expecting a reroll.
 cf. <20170420101024.7593-1-pclo...@gmail.com>
 cf. <20170421145916.mknekgqzhxffu...@sigill.intra.peff.net>
 cf. 


* df/dir-iter-remove-subtree (2017-04-19) 5 commits
 - remove_subtree(): reimplement using iterators
 - dir_iterator: rewrite state machine model
 - dir_iterator: refactor dir_iterator_advance
 - remove_subtree(): test removing nested directories
 - dir_iterator: add tests for dir_iterator API

 Update the dir-iterator API and use it to reimplement
 remove_subtree().

 A reroll exists that is based on the updated 'master', but I ran
 out of time trying to get it to work with other topics in flight
 in 'pu'.
 GSoC microproject.


* jc/bundle (2016-03-03) 6 commits
 - index-pack: --clone-bundle option
 - Merge branch 'jc/index-pack' into jc/bundle
 - bundle v3: the beginning
 - bundle: keep a copy of bundle file name in the in-core bundle header
 - bundle: plug resource leak
 - bundle doc: 'verify' is not about verifying the bundle

 The beginning of "split bundle", which could be one of the
 ingredients to allow "git clone" traffic off of the core server
 network to CDN.

 This was surrected from a "to be discarded" pile, as from time to
 time people wonder about resumable clone that can be primed without
 bothering Git servers with dynamic packfile creation, and some
 people seem to think that the topic could serve as a useful
 building block for that goal.  But nothing seem to have happend.
 Unless people really want it, I am inclined to discard this topic.
 Opinions?


* ja/doc-l10n (2017-03-20) 3 commits
 . SQUASH???
 . l10n: add git-add.txt to localized man pages
 . l10n: introduce framework for localizing man pages

 

[PATCH] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too

[Junio C Hamano]

Let's do the same for Macs, as it is BSD variant after all.

Signed-off-by: Junio C Hamano 
---
 config.mak.uname | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.uname b/config.mak.uname
index a25ffddb3e..1743890164 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -110,6 +110,7 @@ ifeq ($(uname_S),Darwin)
BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1
HAVE_BSD_SYSCTL = YesPlease
+   FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),SunOS)
NEEDS_SOCKET = YesPlease
-- 
2.13.0-336-gf73534b083

Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings

[Jeff King]

On Tue, May 02, 2017 at 03:22:23PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Is there any way with this to both supply CFLAGS & DEVELOPER=1 on the
> command-line, to get my custom -O & these -W flags? I.e.:
> 
> $ make DEVELOPER=1 V=1
> [...] -g -O2 -Wall -Werror -Wdeclaration-after-statement
> -Wno-format-zero-length -Wold-style-definition -Woverflow
> -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -I. [...]
> $ make DEVELOPER=1 CFLAGS="-g -O0 -Wall" V=1
> [...] -g -O0 -Wall -I. [...]
> 
> I thought the second case would prepend my "-g -O0 -Wall" but then be
> followed by the various -W developer flags, but it isn't, am I just
> doing something stupid, or is there no way to combine these two?

The problem is that when you give "make" a variable on the command line,
it overrides all of the modifications. So if you were to set that CFLAGS
in your config.mak, I think everything would work as you expect.

I actually do this in my config.mak:

  O = 0
  CFLAGS += -g -O$(O)

which lets me override the optimization level as a one-off on the
command-line:

  make O=2

without disturbing the rest of the CFLAGS. I also do this:

  CFLAGS += $(EXTRA_CFLAGS)

as a catch-all, so that I can do:

  make EXTRA_CFLAGS=-Wone-off-warning-that-I-am-testing

Perhaps those are things the main Makefile should support. I dunno. You
could see the full depths of my depravity at:

  https://github.com/peff/git/blame/meta/config/config.mak

(I linked to the blame view because there are a lot of WTF bits in there
that are explained by the commit messages).

-Peff

Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s

[Shawn Pearce]

On Tue, May 9, 2017 at 9:33 PM, Jeff King  wrote:
> On Tue, May 09, 2017 at 09:22:11PM -0700, Shawn Pearce wrote:
>
>> > Hmm. That makes sense generally, as the request should succeed. But it
>> > seems like we're creating a client that will sometimes succeed and
>> > sometimes fail, and the reasoning will be somewhat opaque to the user.
>> > I have a feeling I'm missing some context on when you'd expect this to
>> > kick in.
>>
>> Specifically, someone I know was looking at building an application
>> that is passed only a SHA-1 for the tip of a ref on a popular hosting
>> site[1]. They wanted to run `git fetch URL SHA1`, but this failed
>> because the site doesn't have upload.allowtipsha1inwant enabled.
>> However the SHA1 was clearly in the output of git ls-remote.
>
> OK. So this is basically a case where we expect that the user knows what
> they're doing.
>
>> For various reasons they expected this to work, because it works
>> against other sites that do have upload.allowtipsha1inwant enabled.
>> And I personally just expected it to work because the fetch client
>> accepts SHA-1s, and the wire protocol uses "want SHA1" not "want ref",
>> and the SHA-1 passed on the command line was currently in the
>> advertisement when the connection opened, so its certainly something
>> the server is currently willing to serve.
>
> Right, makes sense.  I wondered if GitHub should be turning on
> allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide
> some internal refs[1], and they're things that users wouldn't want to
> fetch. The problem for your case really is just on the client side, and
> this patch fixes it.
>
> Some of this context might be useful in the commit message.
>
> -Peff
>
> [1] The reachability checks from upload-pack don't actually do much on
> GitHub, because you can generally access the objects via the API or
> the web site anyway. So I'm not really opposed to turning on
> allowTipSHA1InWant if it would be useful for users, but after
> Jonathan's patch I don't see how it would be.

Right, I agree. That is why we proposed this patch to the client,
rather than a support request with GitHub to change server
configuration. :)

Re: Script to rebase branches

[Jeff King]

On Tue, May 09, 2017 at 02:32:37PM +0200, Johannes Schindelin wrote:

> > I didn't really expect anybody to use it verbatim, though. I was
> > providing it more for inspiration.
> 
> I deem it part of Git's mission is to avoid forcing everybody to write
> scripts so specific to their own needs that they cannot be shared easily.

Sure. I'd be happy if somebody used it as inspiration to make a tool for
everybody, too.

The two main reasons I don't polish and try to share the bits in my
Meta/ more widely are:

  1. Most of them are as much policy as they are implementation logic.
 So either you buy in completely to the worldview that I've assumed
 in my tools, or you end up fighting the tool (and by the time you
 make the tool configurable enough to handle all world-views, you
 haven't really helped anybody).

 I think the best thing to do with those logic bits is to add them
 as much as possible to Git itself (e.g., as command-line options).
 That keeps any personal scripts as thin wrappers that specify the
 policy.

  2. Some of the features are really powerful but also really dangerous.
 For example, my "rebase" script (which rebases all my topics) and
 my "private" script (which builds my daily "private" version of Git
 to run) both write a shell snippet into $GIT_DIR/continue.

 And then I have a git-continue alias that looks like this:

#!/bin/sh
SUBDIRECTORY_OK=Yes
. git-sh-setup
cd_to_toplevel

if test -f "$GIT_DIR/continue"; then
  eval "$(cat "$GIT_DIR/continue")"
elif test -d "$GIT_DIR/rebase-merge"; then
  git rebase --continue
elif test -d "$GIT_DIR/rebase-apply"; then
  if test -f "$GIT_DIR/rebase-apply/applying"; then
git am --continue
  else
git rebase --continue
  fi
elif test -f "$GIT_DIR/CHERRY_PICK_HEAD"; then
  git cherry-pick --continue
else
  echo >&2 "nothing to continue"
  exit 1
fi

 So when I run "git continue" it magically tries to pick up the
 operation in progress keep going. When it works, it works
 beautifully. But when it doesn't...well, you can dig yourself into
 a pretty confusing situation. It's worth it for me, because I can
 dig myself out. But I'm not sure it's something I'd encourage other
 people to use.

-Peff

Re: Script to rebase branches

[Jeff King]

On Wed, May 10, 2017 at 07:47:26AM +0900, Junio C Hamano wrote:

> > Yes, the script predates the invention of worktrees by several years. I
> > have occasionally played with worktrees, but don't use them extensively
> > (I'd usually use them for a one-off change, and then remove the
> > worktree).
> 
> I check out a different Meta/ at the top-level of my working tree
> when working on Git, but I do use an equivalent of "worktree" to
> have separate build areas for four integration branches.  It is
> trivial to check out Meta/ just once to the primary working tree and
> symlink it to others ;-)

Yeah, I guess I'd need to do that, too, if I used worktrees extensively.
I think the specific problem with the rebase script is just that it
expects to be able to checkout all the branches.

> One thing that struck me odd about your "rebase" script was that it
> didn't seem to have a special provision to handle a topic that
> builds on another topic. I saw toposort, but is that sufficient?

It topo-sorts so that a single run rebases everything (otherwise you may
need to run N times, where N is the deepest dependency chain). But it
also uses reflogs to try to find the fork point when the upstream topic
has been rebased.

The logic is in find_base(). Once upon a time it used "git pull
--rebase", but there were some complications. These days I think it
could probably use "rebase --fork-point", but I just never got around to
testing it.

-Peff

Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s

[Jeff King]

On Tue, May 09, 2017 at 09:22:11PM -0700, Shawn Pearce wrote:

> > Hmm. That makes sense generally, as the request should succeed. But it
> > seems like we're creating a client that will sometimes succeed and
> > sometimes fail, and the reasoning will be somewhat opaque to the user.
> > I have a feeling I'm missing some context on when you'd expect this to
> > kick in.
> 
> Specifically, someone I know was looking at building an application
> that is passed only a SHA-1 for the tip of a ref on a popular hosting
> site[1]. They wanted to run `git fetch URL SHA1`, but this failed
> because the site doesn't have upload.allowtipsha1inwant enabled.
> However the SHA1 was clearly in the output of git ls-remote.

OK. So this is basically a case where we expect that the user knows what
they're doing.

> For various reasons they expected this to work, because it works
> against other sites that do have upload.allowtipsha1inwant enabled.
> And I personally just expected it to work because the fetch client
> accepts SHA-1s, and the wire protocol uses "want SHA1" not "want ref",
> and the SHA-1 passed on the command line was currently in the
> advertisement when the connection opened, so its certainly something
> the server is currently willing to serve.

Right, makes sense.  I wondered if GitHub should be turning on
allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide
some internal refs[1], and they're things that users wouldn't want to
fetch. The problem for your case really is just on the client side, and
this patch fixes it.

Some of this context might be useful in the commit message.

-Peff

[1] The reachability checks from upload-pack don't actually do much on
GitHub, because you can generally access the objects via the API or
the web site anyway. So I'm not really opposed to turning on
allowTipSHA1InWant if it would be useful for users, but after
Jonathan's patch I don't see how it would be.

Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s

[Shawn Pearce]

On Tue, May 9, 2017 at 3:16 PM, Jeff King  wrote:
> On Tue, May 09, 2017 at 11:20:42AM -0700, Jonathan Tan wrote:
>
>> fetch-pack, when fetching a literal SHA-1 from a server that is not
>> configured with uploadpack.allowtipsha1inwant (or similar), always
>> returns an error message of the form "Server does not allow request for
>> unadvertised object %s". However, it is sometimes the case that such
>> object is advertised.
>>
>> Teach fetch-pack to also check the SHA-1s of the refs in the received
>> ref advertisement if a literal SHA-1 was given by the user.
>
> Hmm. That makes sense generally, as the request should succeed. But it
> seems like we're creating a client that will sometimes succeed and
> sometimes fail, and the reasoning will be somewhat opaque to the user.
> I have a feeling I'm missing some context on when you'd expect this to
> kick in.

Specifically, someone I know was looking at building an application
that is passed only a SHA-1 for the tip of a ref on a popular hosting
site[1]. They wanted to run `git fetch URL SHA1`, but this failed
because the site doesn't have upload.allowtipsha1inwant enabled.
However the SHA1 was clearly in the output of git ls-remote.

So a workaround is to do this in shell, which is just weird:

  r=$(git ls-remote $url | grep ^$sha1);
  if [ -n "$r" ]; then
exec git fetch $url $r:refs/tmp/foo
  else
echo >&2 "cannot find $sha1"
exit 1
  fi

For various reasons they expected this to work, because it works
against other sites that do have upload.allowtipsha1inwant enabled.
And I personally just expected it to work because the fetch client
accepts SHA-1s, and the wire protocol uses "want SHA1" not "want ref",
and the SHA-1 passed on the command line was currently in the
advertisement when the connection opened, so its certainly something
the server is currently willing to serve.

The application in question is using a SHA-1 rather than a ref name,
because thats what it was given by something else. And the other thing
basically wants this to fail-fast if it can't get that exact SHA-1. So
to pass a ref name to git fetch the supplier would have to actually
pass a tuple of ref+sha1, and then the fetcher has to check that the
ref it just obtained provides the sha1 it expected.

So this all turned into a bug report by me to Jonathan Tan, because
git fetch violated my assumption of what it would do if I handed it a
SHA-1 and the SHA-1 was currently the tip of a remote ref.


[1] github.com

Schönen Tag

[UniCredit ®]

Hallo
  Ich bin Frau Victoria. G.Harry, ein Finanz-Manager für eine private 
Kreditvergabe Agentur, Uni Kredit Darlehen UK. Ich bin hier, um ein Darlehen 
Programm, das Ihnen helfen, Ihre finanzielle und wirtschaftliche Situation zu 
verbessern und entlasten Sie alle finanziellen Krise / Problem.
Wir bieten kurz- und langfristige Darlehen an Kunden mit einer Rate von 3%, 
z.B. Auto Darlehen, persönliche Darlehen, Business-Darlehen (klein oder groß), 
etc.

Wenn Sie daran interessiert sind, ein Darlehen von uns zu erhalten, können 
Sie uns über diese E-Mail kontaktieren: uni_cred...@outlook.com
 
Auch Beratung zum Ausfüllen des Antragsformulars unten:

Name:
Sex:
Land / Adresse
Benötigte Menge:
Dauer des Darlehens:
Zweck:
Telefon:

Vielen Dank für Ihre Schirmherrschaft,

Frau Victoria .G. Harry

Re: [BUG] :(attr) pathspecs can die("BUG") in the tree-walker

[Jeff King]

On Tue, May 09, 2017 at 03:52:19PM -0700, Brandon Williams wrote:

> >   $ git log HEAD -- ':(attr:-diff)'
> >   fatal: BUG:tree-walk.c:947: unsupported magic 40
> > 
> > Whoops. This is presumably ls-tree is protected, but I think we are
> > missing a GUARD_PATHSPEC call somewhere.
> > 
> > This isn't a huge deal, as the correct behavior is probably to die like
> > ls-tree does, but we probably shouldn't be hitting BUG assertions as a
> > general rule.
> 
> The die("BUG: ..."); is from a GAURD_PATHSPEC call.  What really needs
> to happen is to update the magic_mask passed into the parse_pathspec
> call which initializes the pathspec object.  Its this magic_mask which
> catches unsupported magic and prints a better message.

Ah, right, I always get the pathspec safety bits mixed up. I think the
parse_pathspec() call in setup_revisions is a bit permissive:

  parse_pathspec(>prune_data, 0, 0,
 revs->prefix, prune_data.path);

You _shouldn't_ need to audit all the callers when you add new pathspec
magic. The callers should be masking out the bits that they know they
support, but this one isn't.  It looks like there are a number of such
permissive calls, though. I guess it was an attempt to not have to list
them manually at each call site (but then we suffer from the exact
problem I ran into).

> I guess this means I (or someone else :D) should audit all the
> parse_pathspec calls and ensure that 'attr' magic is turned off until we
> won't run into these GAURD_PATHSPEC die's.

Of course it would be nicer still if the 'attr' magic actually worked
via the tree-walking code. :)

-Peff

Re: [ANNOUNCE] Git v2.12.3 and others

[Jonathan Nieder]

Junio C Hamano wrote:

> Maintenance releases Git v2.4.12, v2.5.6, v2.6.7, v2.7.5, v2.8.5,
> v2.9.4, v2.10.3, v2.11.2, and v2.12.3 have been tagged and are now
> available at the usual places.
>
> These are primarily to fix a recently disclosed problem with "git
> shell", which may allow a user who comes over SSH to run an
> interactive pager by causing it to spawn "git upload-pack --help"
> (CVE-2017-8386).  Some (like v2.12.3) have other fixes that have
> been accumulating included as well.
>
> "git-shell" is a restricted login shell that can be used on a server
> to prevent SSH clients from running any programs except those needed
> for git fetches and pushes. If you are not running a server, or if
> your server has not been explicitly configured to use git-shell as a
> login shell, you are not affected.  Also note that sites running "git
> shell" behind gitolite are NOT vulnerable.

Thanks.  Credit for discovering this bug goes to Timo Schmid, ERNW GmbH.
They will probably have a blog post soon with more details.

1.6.1 is the earliest git version affected (so this goes back pretty
far).

> The tarballs are found at:
>
> https://www.kernel.org/pub/software/scm/git/
>
> The following public repositories all have a copy of these tags:
>
>   url = https://kernel.googlesource.com/pub/scm/git/git
>   url = git://repo.or.cz/alt-git.git
>   url = git://git.sourceforge.jp/gitroot/git-core/git.git
>   url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
>   url = https://github.com/gitster/git

Sincerely,
Jonathan

[ANNOUNCE] Git v2.12.3 and others

[Junio C Hamano]

Maintenance releases Git v2.4.12, v2.5.6, v2.6.7, v2.7.5, v2.8.5,
v2.9.4, v2.10.3, v2.11.2, and v2.12.3 have been tagged and are now
available at the usual places.

These are primarily to fix a recently disclosed problem with "git
shell", which may allow a user who comes over SSH to run an
interactive pager by causing it to spawn "git upload-pack --help"
(CVE-2017-8386).  Some (like v2.12.3) have other fixes that have
been accumulating included as well.

"git-shell" is a restricted login shell that can be used on a server
to prevent SSH clients from running any programs except those needed
for git fetches and pushes. If you are not running a server, or if
your server has not been explicitly configured to use git-shell as a
login shell, you are not affected.  Also note that sites running "git
shell" behind gitolite are NOT vulnerable.

The tarballs are found at:

https://www.kernel.org/pub/software/scm/git/

The following public repositories all have a copy of these tags:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

[ANNOUNCE] Git v2.13.0

[Junio C Hamano]

The latest feature release Git v2.13.0 is now available at the
usual places.  It is comprised of 729 non-merge commits since
v2.12.0, contributed by 65 people, 15 of which are new faces.

This release also contains the security patch in v2.12.3 and
others to fix CVE-2017-8386.

The tarballs are found at:

https://www.kernel.org/pub/software/scm/git/

The following public repositories all have a copy of the 'v2.13.0'
tag and the 'master' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

New contributors whose contributions weren't in v2.12.0 are as follows.
Welcome to the Git development community!

  Allan Xavier, Andreas Heiduk, Devin J. Pohly, Devin Lehmacher,
  Hiroshi Shirosaki, Johan Hovold, Maxim Moseychuk, Mostyn
  Bramley-Moore, Prathamesh Chavan, Quentin Pradet, René Genz,
  Segev Finer, Sergey Ryazanov, Stephen Hicks, and Valery Tolstov.

Returning contributors who helped this release are as follows.
Thanks for your continued support.

  Ævar Arnfjörð Bjarmason, Alexander Shopov, Alex Henrie,
  Brandon Williams, brian m. carlson, Christian Couder, Cornelius
  Weig, David Aguilar, David Turner, Eric Wong, Giuseppe Bilotta,
  Jacob Keller, Jean-Noel Avila, Jeff Hostetler, Jeff King,
  Jiang Xin, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
  Jordi Mas, Junio C Hamano, Karthik Nayak, Kevin Willford,
  Kyle Meyer, Lars Schneider, Linus Torvalds, Luke Diamand, Matt
  McCutchen, Michael Haggerty, Michael J Gruber, Michael Rappazzo,
  Mike Hommey, Nguyễn Thái Ngọc Duy, Patrick Steinhardt,
  Peter Krefting, Ralf Thielow, Ramsay Jones, Ray Chen, René
  Scharfe, Ross Lagerwall, Santiago Torres, Sebastian Schuberth,
  Simon Ruderich, Stefan Beller, SZEDER Gábor, Thomas Gummerer,
  Torsten Bögershausen, Trần Ngọc Quân, Vasco Almeida,
  and Vegard Nossum.



Git 2.13 Release Notes
==

Backward compatibility notes.

 * Use of an empty string as a pathspec element that is used for
   'everything matches' is still warned and Git asks users to use a
   more explicit '.' for that instead.  The hope is that existing
   users will not mind this change, and eventually the warning can be
   turned into a hard error, upgrading the deprecation into removal of
   this (mis)feature.  That is not scheduled to happen in the upcoming
   release (yet).

 * The historical argument order "git merge  HEAD ..."
   has been deprecated for quite some time, and is now removed.

 * The default location "~/.git-credential-cache/socket" for the
   socket used to communicate with the credential-cache daemon has
   been moved to "~/.cache/git/credential/socket".

 * Git now avoids blindly falling back to ".git" when the setup
   sequence said we are _not_ in Git repository.  A corner case that
   happens to work right now may be broken by a call to die("BUG").
   We've tried hard to locate such cases and fixed them, but there
   might still be cases that need to be addressed--bug reports are
   greatly appreciated.


Updates since v2.12
---

UI, Workflows & Features

 * "git describe" and "git name-rev" have been taught to take more
   than one refname patterns to restrict the set of refs to base their
   naming output on, and also learned to take negative patterns to
   name refs not to be used for naming via their "--exclude" option.

 * Deletion of a branch "foo/bar" could remove .git/refs/heads/foo
   once there no longer is any other branch whose name begins with
   "foo/", but we didn't do so so far.  Now we do.

 * When "git merge" detects a path that is renamed in one history
   while the other history deleted (or modified) it, it now reports
   both paths to help the user understand what is going on in the two
   histories being merged.

 * The  part in "http.." configuration variable
   can now be spelled with '*' that serves as wildcard.
   E.g. "http.https://*.example.com.proxy; can be used to specify the
   proxy used for https://a.example.com, https://b.example.com, etc.,
   i.e. any host in the example.com domain.

 * "git tag" did not leave useful message when adding a new entry to
   reflog; this was left unnoticed for a long time because refs/tags/*
   doesn't keep reflog by default.

 * The "negative" pathspec feature was somewhat more cumbersome to use
   than necessary in that its short-hand used "!" which needed to be
   escaped from shells, and it required "exclude from what?" specified.

 * The command line options for ssh invocation needs to be tweaked for
   some implementations of SSH (e.g. PuTTY plink wants "-P "
   while OpenSSH wants "-p " to specify port to connect to), and
   the variant was guessed when GIT_SSH environment variable is used
   to specify it.  

Re: [PATCH v2] travis-ci: retry if Git for Windows CI returns HTTP error 502 or 503

[Junio C Hamano]

Lars Schneider  writes:

>>> It would be great if we could test this is a little bit in pu.
>> 
>> This has been in 'pu' for a while.  
>> 
>> As the patch simply discards 502 (and others), it is unclear if the
>> failing test on 'next' is now gone, or the attempt to run 'pu'
>> happened to be lucky not to get one, from the output we can see in
>> https://travis-ci.org/git/git/jobs/229867212
>> 
>> Are you comfortable enough to move this forward?
>
> Yes, please move it forward. I haven't seen a "502 - Web server 
> received an invalid response" on pu for a while. That means the
> patch should work as expected.

Will do, thanks.

> Unrelated to this patch I have, however, seen two kinds of timeouts:
>
> (1) Timeout in the "notStarted" state. This job eventually finished
> with a failure but it did start only *after* 3h:
> https://travis-ci.org/git/git/jobs/230225611
>
> (2) Timeout in the "in progress" state. This job eventually finished
> successfully but it took longer than 3h:
> https://travis-ci.org/git/git/jobs/229867248
>
> Right now the timeout generates potential false negative results. 
> I would like to change that and respond with a successful build 
> *before* we approach the 3h timeout. This means we could generate
> false positives. Although this is not ideal, I think that is the better 
> compromise as a failing Windows build would usually fail quickly 
> (e.g. in the compile step).
>
> What do you guys think? Would you be OK with that reasoning?
> If the Git for Windows builds get more stable over time then
> we could reevaluate this compromise.

I'd rather see a false breakage on Windows build (i.e. "this might
have succeeded given enough time, but it didn't finish within the
alloted time") than a false sucess (i.e. "we successfully launched
and the build is still running, so let's assume the test succeeds").
Because I do not pay attention to what the overall build page [*1*]
says about a particular branch tip, and I instead look at the
summary list of the indiviaul "Build Jobs", e.g. [*2*]), seeing
errored/failed on [*1*] does not bother me personally, if that is
what you are getting at.


[References]

*1* https://travis-ci.org/git/git/builds/
*2* https://travis-ci.org/git/git/builds/230235081

Re: [PATCH] fixup! use perl instead of sed

[Ævar Arnfjörð Bjarmason]

On Wed, May 10, 2017 at 12:38 AM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason  writes:
>
>> On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan  
>> wrote:
>> ...
>>> # Tweak the push output to make the push option outside the cert
>>> # different, then replay it on a fresh dst, checking that ff is not
>>> # deleted.
>>> -   sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&
>>> +   perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak &&
>>> prepare_dst &&
>>> git -C dst config receive.certnonceseed sekrit &&
>>> git -C dst config receive.advertisepushoptions 1 &&
>>> -   git receive-pack dst out &&
>>> +   git receive-pack dst out &&
>>
>> The test should have a PERL prerequisite now, that's missing.
>
> For a single-liner like this, our stance has always been that t/
> scripts can assume _some_ version of Perl interpreter available for
> use, cf. t/README where it lists prerequisites and explains them:
>
>  - PERL
>
>Git wasn't compiled with NO_PERL=YesPlease.
>
>Even without the PERL prerequisite, tests can assume there is a
>usable perl interpreter at $PERL_PATH, though it need not be
>particularly modern.
>
> So unless "receive-pack" that is being tested here requires Perl at
> runtime, we do not want PERL prerequisite for this test.

Oops, sorry about that.

>> Also using \1 will likely be deprecated in future versions of perl at
>> some point:
>>
>> $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/"
>> \1 better written as $1 at -e line 1.
>> hifoo
>
> Very good advice from a Perl expert; thanks.

No problem. Hopefully my noisy advice will at least lead to fixing a
bug in perl if there is one :)

Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning

[Junio C Hamano]

Ramsay Jones  writes:

> Yeah, I had a similar comment in the commit message (but much more
> verbose than your concise addition above), but I edited it several
> times, without finding a wording that I liked. I eventually removed
> it, because it didn't really add any value. :(

I tend to agree that the proposed additional comment does not add
much value.  It assures the readers that we (at the time of applying
this patch) know that the earlier use of ULL was not done with a
good reason but was merely an accident, and strengthens the claim
that this is a good change, but the correctness of the change is
already obvious, and the readers would understand without being
explained where the incorrectness we have to fix with this patch
came from, I would think.
.

Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning

[Junio C Hamano]

Ramsay Jones  writes:

> In a similar vein, on systems which use a 64-bit representation of the
> 'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with
> the value 0777ULL. Although this does not cause any warning
> messages to be issued, it would be more appropriate for this constant
> to use an 'UL' type suffix rather than 'ULL'.

... it is more appropriate because we know the recipient is
"unsigned long", not "unsigned long long", in this case?  As opposed
to the case of timestamp_t, which is opaque and could be "unsigned
long long"?

That makes sense to me, even though it took a bit of thinking aloud
to understand.

Looks good; thanks.

Re: [PATCH v2 00/21]

[Junio C Hamano]

Duy Nguyen  writes:

> On Sun, May 7, 2017 at 11:20 AM, Junio C Hamano  wrote:
>> On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano  wrote:
>>>
>>> Nguyễn Thái Ngọc Duy   writes:
>>>
>>> > Changes since v1:
>>> >
>>> >  - fopen_or_warn() and warn_on_fopen_errors() are introduced. The
>>> >latter's name is probably not great...
>>> >  - A new patch (first one) to convert a bunch to using xfopen()
>>> >  - Fix test failure on Windows, found by Johannes Sixt
>>> >  - Fix the memory leak in log.c, found by Jeff
>>> >
>>> > There are still a couple of fopen() remained, but I'm getting slow
>>> > again so let's get these in first and worry about the rest when
>>> > somebody has time.
>>
>> Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?
>>
>> https://travis-ci.org/git/git/jobs/229585098#L3091
>>
>> seems to expect an error when test-config is fed a-directory but we are
>> not getting the expected warning and error?
>
> Sounds about right. Let me see if defining FREAD_READS_DIRECTORIES on
> MacOS X makes travis happy.

Thanks.  I should have suspected that myself to save a round-trip.

Re: [BUG] :(attr) pathspecs can die("BUG") in the tree-walker

[Brandon Williams]

On 05/09, Jeff King wrote:
> I was playing with the new :(attr) pathspecs in the upcoming v2.13
> today, and noticed:
> 
>   $ git ls-files -- ':(attr:-diff)'
>   t/t0110/url-1
>   t/t0110/url-10
>   [etc]
> 
> So far so good.
> 
>   $ git ls-tree HEAD -- ':(attr:-diff)'
>   fatal: :(attr:-diff): pathspec magic not supported by this command: 'attr'
> 
> Bummer, but I understand that sometimes the options need to be plumbed
> through to work everywhere.
> 
>   $ git log HEAD -- ':(attr:-diff)'
>   fatal: BUG:tree-walk.c:947: unsupported magic 40
> 
> Whoops. This is presumably ls-tree is protected, but I think we are
> missing a GUARD_PATHSPEC call somewhere.
> 
> This isn't a huge deal, as the correct behavior is probably to die like
> ls-tree does, but we probably shouldn't be hitting BUG assertions as a
> general rule.
> 
> -Peff

The die("BUG: ..."); is from a GAURD_PATHSPEC call.  What really needs
to happen is to update the magic_mask passed into the parse_pathspec
call which initializes the pathspec object.  Its this magic_mask which
catches unsupported magic and prints a better message.

I guess this means I (or someone else :D) should audit all the
parse_pathspec calls and ensure that 'attr' magic is turned off until we
won't run into these GAURD_PATHSPEC die's.

-- 
Brandon Williams

Re: [PATCH v4 11/25] checkout: fix memory leak

[Junio C Hamano]

Johannes Schindelin  writes:

>> > A leak is better than a use after free, so
>> > let's be extra careful here.  Would it leave the index inconsistent?  Or
>> > perhaps freeing it has become safe in the meantime?
>> >
>> > @Junio: Do you remember the reason for the leaks in 0cf8581e330
>> > (checkout -m: recreate merge when checking out of unmerged index).
>> 
>> Yes.
>> 
>> In the very old days it was not allowed to free(3) contents of
>> active_cache[] and this was an old brain fart that came out of
>> inertia.  We are manufacturing a brand new ce, only to feed it to
>> checkout_entry() without even registering it to the active_cache[]
>> array, and the ancient rule doesn't even apply to such a case.
>> 
>> So I think it was safe to free(3) even back then.
>
> So this patch is good, then?

Unless I from this year is failing to spot a breakage that will be
caused in the codepath that I from year 2008 and René spotted, I
think freeing ce after we are done updating the working tree file
with it is safe.  I'd need to find time to make sure, though.

>> > And result_buf is still leaked here, right?
>> 
>> Good spotting.  It typically would make a larger leak than a single
>> ce, I would suppose ;-)
>
> I saw you added this as a fixup! commit. If you don't mind, and if no
> other changes are requested, would you mind rebase'ing this yourself?

I think it would be better to leave the fix as a separate patch.  It
wasn't spotted by Coverity in the first place ;-)

Re: Script to rebase branches

[Junio C Hamano]

Jeff King  writes:

>> That requires Meta/ to be checked out and up-to-date. I'd bet there are
>> exactly two people who fall into that category.
>
> Actually, it is not Junio's Meta that needs checked out, but rather the
> "meta" branch where you will find that "rebase" script. If other people
> find them useful, the set of scripts could perhaps be transitioned to a
> namespace that is appropriate to go into people's $PATH.
>
> I didn't really expect anybody to use it verbatim, though. I was
> providing it more for inspiration.
>
>> Also, I see that you do not use worktrees. Otherwise your script would
>> fail.
>
> Yes, the script predates the invention of worktrees by several years. I
> have occasionally played with worktrees, but don't use them extensively
> (I'd usually use them for a one-off change, and then remove the
> worktree).

I check out a different Meta/ at the top-level of my working tree
when working on Git, but I do use an equivalent of "worktree" to
have separate build areas for four integration branches.  It is
trivial to check out Meta/ just once to the primary working tree and
symlink it to others ;-)

One thing that struck me odd about your "rebase" script was that it
didn't seem to have a special provision to handle a topic that
builds on another topic. I saw toposort, but is that sufficient?

Re: [PATCH] fixup! use perl instead of sed

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan  wrote:
> ...
>> # Tweak the push output to make the push option outside the cert
>> # different, then replay it on a fresh dst, checking that ff is not
>> # deleted.
>> -   sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&
>> +   perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak &&
>> prepare_dst &&
>> git -C dst config receive.certnonceseed sekrit &&
>> git -C dst config receive.advertisepushoptions 1 &&
>> -   git receive-pack dst out &&
>> +   git receive-pack dst out &&
>
> The test should have a PERL prerequisite now, that's missing.

For a single-liner like this, our stance has always been that t/
scripts can assume _some_ version of Perl interpreter available for
use, cf. t/README where it lists prerequisites and explains them:

 - PERL

   Git wasn't compiled with NO_PERL=YesPlease.

   Even without the PERL prerequisite, tests can assume there is a
   usable perl interpreter at $PERL_PATH, though it need not be
   particularly modern.

So unless "receive-pack" that is being tested here requires Perl at
runtime, we do not want PERL prerequisite for this test.

> Also using \1 will likely be deprecated in future versions of perl at
> some point:
>
> $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/"
> \1 better written as $1 at -e line 1.
> hifoo

Very good advice from a Perl expert; thanks.

[BUG] :(attr) pathspecs can die("BUG") in the tree-walker

[Jeff King]

I was playing with the new :(attr) pathspecs in the upcoming v2.13
today, and noticed:

  $ git ls-files -- ':(attr:-diff)'
  t/t0110/url-1
  t/t0110/url-10
  [etc]

So far so good.

  $ git ls-tree HEAD -- ':(attr:-diff)'
  fatal: :(attr:-diff): pathspec magic not supported by this command: 'attr'

Bummer, but I understand that sometimes the options need to be plumbed
through to work everywhere.

  $ git log HEAD -- ':(attr:-diff)'
  fatal: BUG:tree-walk.c:947: unsupported magic 40

Whoops. This is presumably ls-tree is protected, but I think we are
missing a GUARD_PATHSPEC call somewhere.

This isn't a huge deal, as the correct behavior is probably to die like
ls-tree does, but we probably shouldn't be hitting BUG assertions as a
general rule.

-Peff

Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s

[Jeff King]

On Tue, May 09, 2017 at 11:20:42AM -0700, Jonathan Tan wrote:

> fetch-pack, when fetching a literal SHA-1 from a server that is not
> configured with uploadpack.allowtipsha1inwant (or similar), always
> returns an error message of the form "Server does not allow request for
> unadvertised object %s". However, it is sometimes the case that such
> object is advertised.
> 
> Teach fetch-pack to also check the SHA-1s of the refs in the received
> ref advertisement if a literal SHA-1 was given by the user.

Hmm. That makes sense generally, as the request should succeed. But it
seems like we're creating a client that will sometimes succeed and
sometimes fail, and the reasoning will be somewhat opaque to the user.
I have a feeling I'm missing some context on when you'd expect this to
kick in.

> +static int is_literal_sha1(const struct ref *ref)
> +{
> + struct object_id oid;
> + return !get_oid_hex(ref->name, ) &&
> +!ref->name[40] &&
> +!oidcmp(, >old_oid);
> +}

I think the preferred method these days is to avoid the bare "40":

  struct object_oid oid;
  const char *end;
  return !parse_oid_hex(ref->name, , ) &&
 !*end &&
 !oidcmp(, >old_oid);

I was confused at first why we need this oidcmp() and the one below. But
this one is checking "does the name parse to itself", and the other is
checking "does this parse to our sought ref?". So both checks are
needed.

> + for (i = 0; i < nr_sought; i++) {
> + struct ref *s = sought[i];
> + if (!strcmp(ref->name, s->name) ||
> + (is_literal_sha1(s) &&
> +  !oidcmp(>old_oid, >old_oid))) {
> + keep = 1;
> + s->match_status = REF_MATCHED;
>   }
> - i++;
>   }

This will reparse ref->name as an oid via is_literal_sha1() for each
pass through the loop. Should it be hoisted out? Maybe that is just
premature optimization, though.

Other than those minor nits, the code itself looks fine to me.

-Peff

Re: [PATCH v3 00/53] object_id part 8

[Brandon Williams]

On 05/06, brian m. carlson wrote:
> This is the eighth series of patches to convert unsigned char [20] to
> struct object_id.  This series converts lookup_commit, lookup_blob,
> lookup_tree, lookup_tag, and finally parse_object to struct object_id.
> 
> A small number of functions have temporaries inserted during the
> conversion in order to allow conversion of functions that still need to
> take unsigned char *; they are removed either later in the series or
> will be in a future series.
> 
> This series can be fetched from the object-id-part8 branch from either
> of the follwing:
> 
> https://github.com/bk2204/git
> https://git.crustytoothpaste.net/git/bmc/git.git
> 
> Changes from v2:
> * Remove spurious space after ampersand.
> * Undo more needless line rewrapping.
> * Expand computation for notes path.
> * Remove check for line->len with parse_oid_hex.
> 
> Changes from v1:
> * Rebase on master.  This led to a conflict with the ref-cache changes in 
> patch
>   39.  Extra-careful review here would be welcome.
> * Undo the needless line rewrapping.
> * Fix the commit message typo.
> * Use GIT_MAX_RAWSZ instead of struct object_id for the pack checksum.

I wasn't able to find any issues with v3.  Looks good.

-- 
Brandon Williams

RE: git client debug with customer ssh client

[Randall S. Becker]

On May 5, 2017 7:50 AM  Pierre J. Ludwick wrote:

> How can we get more info from git client? Any helps suggestions welcomed?

It might be helpful to put a full trace in OpenSSH. Running ssh with -vvv 
should give you a lot of noise. I have used 
https://en.wikibooks.org/wiki/OpenSSH/Logging_and_Troubleshooting
to pull information when the platform's OpenSSH port was done. If you need 
access to the ported code, the sources are available in the usual spot at 
ITUGLIB.

Cheers,
Randall

Re: [PATCH] fixup! use perl instead of sed

[Ævar Arnfjörð Bjarmason]

On Tue, May 9, 2017 at 10:43 PM, Johannes Sixt  wrote:
> Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason:
>>
>> Finally, you can just use -i like you did with sed, no need for the
>> tempfile:
>
>
> Nope. Some implementations of perl attempt to remove the file that it has
> just opened. That doesn't work on Windows. You have to supply a backup file
> name as in `perl -i.bak ...` :-(

This should have been fixed in 2002, and is in 5.8, the oldest perl
release we support: https://github.com/Perl/perl5/commit/c030f24b81 &
http://www.nntp.perl.org/group/perl.perl5.porters/2002/05/msg60275.html

But maybe __CYGWIN__ isn't always defined on Windows, maybe this was a
mingw build or something and perl was missing a test for this when
support for that was added?

This is obviously a trivial issue for git, but if it's a bug in perl
I'd like to fix that.

>>
>>  $ echo hibar >push
>>  $ perl -pi -e 's/([^ ])bar/$1baz/' push
>>  $ cat push
>>  hibaz
>
>
> -- Hannes

Re: [PATCH] fixup! use perl instead of sed

[Jonathan Tan]

On 05/09/2017 01:43 PM, Johannes Sixt wrote:

Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason:

Finally, you can just use -i like you did with sed, no need for the
tempfile:


Nope. Some implementations of perl attempt to remove the file that it
has just opened. That doesn't work on Windows. You have to supply a
backup file name as in `perl -i.bak ...` :-(



 $ echo hibar >push
 $ perl -pi -e 's/([^ ])bar/$1baz/' push
 $ cat push
 hibaz


-- Hannes


Thanks - sent a fixup [1] in reply to my PATCH v3 cover letter (but 
forgot to CC you).


[1] <20170509210158.17898-1-jonathanta...@google.com>

[PATCH v3] fixup! don't use perl -i because it's not portable

[Jonathan Tan]

Signed-off-by: Jonathan Tan 
---

Ah...thanks, Johannes, for spotting this. Here's a fixup.

 t/t5534-push-signed.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 177b933a7..807267b7f 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -152,11 +152,11 @@ test_expect_success GPG,PERL 'inconsistent push options 
in signed push not allow
# Tweak the push output to make the push option outside the cert
# different, then replay it on a fresh dst, checking that ff is not
# deleted.
-   perl -pi -e "s/([^ ])bar/\$1baz/" push &&
+   perl -pe "s/([^ ])bar/\$1baz/" push >push.tweak &&
prepare_dst &&
git -C dst config receive.certnonceseed sekrit &&
git -C dst config receive.advertisepushoptions 1 &&
-   git receive-pack dst out &&
+   git receive-pack dst out &&
git -C dst rev-parse ff &&
grep "inconsistent push options" out
 '
-- 
2.13.0.rc2.291.g57267f2277-goog

Re: [PATCH] fixup! use perl instead of sed

[Johannes Sixt]

Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason:

Finally, you can just use -i like you did with sed, no need for the tempfile:


Nope. Some implementations of perl attempt to remove the file that it 
has just opened. That doesn't work on Windows. You have to supply a 
backup file name as in `perl -i.bak ...` :-(




 $ echo hibar >push
 $ perl -pi -e 's/([^ ])bar/$1baz/' push
 $ cat push
 hibaz


-- Hannes

Re: [noob] is this normal behavior

[Igor Djordjevic]

Hi Harry,

Both behaviours you report are normal, specifically:

On 09/05/2017 15:02, Harry Putnam wrote:
> Shouldn't files that changed but are already being tracked just show
> up as modified and not need adding?
> ...
> Since that file is already being tracked; shouldn't `git status' show
> that file as modified but ready to be committed instead of a file that
> is modified but needs to be added before a commit can happen?

No, it shouldn`t - even though the file you`ve modified is already 
being tracked, it doesn`t have to mean you want the modification 
inside your very next commit, and Git doesn`t force that upon you.

That is where index (or "staging area") comes in handy, allowing you 
to `git add` only the changes you actually want to commit now, 
leaving the others for later decision.

You don`t even have to add all the modifications inside the single 
file at once, for example by using `git add --patch`[1] you can 
select just some of the them, fine tuning what gets committed and when.

With some precaution/steps needed not to commit broken project states 
by accident, and some discipline not to overdo it, this allows you to 
fully focus on the actual work you do, making logically unrelated 
changes in place as you see fit, on the go, and only later organizing 
them into logically grouped commits, through diligent use of `git add`.

> Another side of this is that a `git diff FILE' only works before an
> `git add .' operation is done.
> 
> That is, if I run `git diff FILE' AFTER `git add' .. no diff is
> reported, even though it is not committed yet.
> 
> So, for example: if I'm committing and in the vi buffer of the commit
> and want to see a diff of FILE to aid my log notes.
> 
>  git diff FILE will report nothing whatever.
> 
> Is that expected behavior?

Yes, that is as expected - in the form you`ve given, `git diff` shows 
the differences between your working tree and index (staging area), 
so only changes you haven`t added yet. Once you `git add` the changes 
from the working to the index, there are no more differences to show, 
so no diff is produced.

If you want to see the added changes, what will be included in the 
commit if you make it, you can use `git diff --cached`, as per 
git-diff[2] documentation (--staged can also be used instead, being a synonym 
for --cached, but maybe easier to remember, relating it to "staging 
area").

That option shows differences between the staging area (index) and 
the specific commit - with none provided, it implies your currently 
checked-out position (referred to as HEAD), usually being your 
latest/previous commit, which is exactly what you`re interested in.


As a side note, if you think you don`t need it, you can skip staging 
area and commit all the modifications/deletions without a need of 
adding them first by using `git commit --all`, as per git-commit[3] 
documentation.

Just pay attention that untracked files are not affected, you still 
need to add them first to tell Git to start tracking them, including 
them in the next commit, but that seems to align nicely with your 
expectations already.

I personally find the staging area to be one of the greatest Git 
possibilities, but I do understand beginners getting confused by it, 
as admittedly I once was myself.


In the end, you may want to ask questions like this on Git users 
mailing list[4] on Google Groups, being a a nice place for beginners 
to get answers to their concerns.

[1] https://git-scm.com/docs/git-add
[2] https://git-scm.com/docs/git-diff
[3] https://git-scm.com/docs/git-commit
[4] https://groups.google.com/forum/?fromgroups#!forum/git-users

Regards,
Buga

Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning

[Ramsay Jones]

On 09/05/17 11:24, Johannes Schindelin wrote:
> Hi Ramsay,
> 
> On Mon, 8 May 2017, Ramsay Jones wrote:
> 
>> Commit dddbad728c ("timestamp_t: a new data type for timestamps",
>> 26-04-2017) introduced a new typedef 'timestamp_t', as a synonym for an
>> unsigned long, which was used at the time to represent timestamps in
>> git. A later commit 28f4aee3fb ("use uintmax_t for timestamps",
>> 26-04-2017) changed the typedef to use an 'uintmax_t' for the timestamp
>> representation type.
>>
>> When building on a 32-bit Linux system, sparse complains that a constant
>> (USTAR_MAX_MTIME) used to detect a 'far-future mtime' timestamp, is too
>> large; 'warning: constant 0777UL is so big it is unsigned long
>> long' on lines 335 and 338 of archive-tar.c. Note that both gcc and
>> clang only issue a warning if this constant is used in a context that
>> requires an 'unsigned long' (rather than an uintmax_t). (Since TIME_MAX
>> is no longer equal to 0x, even on a 32-bit system, the macro
>> USTAR_MAX_MTIME is set to 0777UL, which cannot be represented as
>> an 'unsigned long' constant).
>>
>> In order to suppress the warning, change the definition of the macro
>> constant USTAR_MAX_MTIME to use an 'ULL' type suffix.
>>
>> In a similar vein, on systems which use a 64-bit representation of the
>> 'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with
>> the value 0777ULL. Although this does not cause any warning
>> messages to be issued, it would be more appropriate for this constant
>> to use an 'UL' type suffix rather than 'ULL'.
> 
>   The reason for the current situation is that an earlier fix for
>   the USTAR_MAX_MTIME constant was applied to the USTAR_MAX_SIZE
>   constant by mistake.

Yeah, I had a similar comment in the commit message (but much more
verbose than your concise addition above), but I edited it several
times, without finding a wording that I liked. I eventually removed
it, because it didn't really add any value. :(

>> Signed-off-by: Ramsay Jones 
> 
> With that addition to the commit message: ACK

This patch is now in the 'next' branch, so I guess it's too late
to add this to the commit message (which I would be quite happy to do).

Well, at the beginning of the next cycle, 'next' will be rebuilt, so
I guess (if we remember!) this patch could be updated then.

ATB,
Ramsay Jones

Re: How to `git status' without scrambling modified with new, etc

[Harry Putnam]

Samuel Lijin  writes:

> You can use `git status -s` and match on the modification type (M
> corresponds to modified, A to new files). See the man page for more
> details on the interface.

ahh yes.  Just the ticket thanks

[PATCH v3 2/2] receive-pack: verify push options in cert

[Jonathan Tan]

In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack
was taught to include push options both within the signed cert (if the
push is a signed push) and outside the signed cert; however,
receive-pack ignores push options within the cert, only handling push
options outside the cert.

Teach receive-pack, in the case that push options are provided for a
signed push, to verify that the push options both within the cert and
outside the cert are consistent.

This sets in stone the requirement that send-pack redundantly send its
push options in 2 places, but I think that this is better than the
alternatives. Sending push options only within the cert is
backwards-incompatible with existing Git servers (which read push
options only from outside the cert), and sending push options only
outside the cert means that the push options are not signed for.

Signed-off-by: Jonathan Tan 
---
 Documentation/technical/pack-protocol.txt | 32 +++
 builtin/receive-pack.c| 51 +--
 t/t5534-push-signed.sh| 37 ++
 3 files changed, 112 insertions(+), 8 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt 
b/Documentation/technical/pack-protocol.txt
index 5b0ba3ef2..a34917153 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the 
obj-id currently on
 the server, the obj-id the client would like to update it to and the name
 of the reference.
 
-This list is followed by a flush-pkt. Then the push options are transmitted
-one per packet followed by another flush-pkt. After that the packfile that
-should contain all the objects that the server will need to complete the new
-references will be sent.
+This list is followed by a flush-pkt.
 
 
-  update-request=  *shallow ( command-list | push-cert ) [packfile]
+  update-requests   =  *shallow ( command-list | push-cert )
 
   shallow   =  PKT-LINE("shallow" SP obj-id)
 
@@ -500,12 +497,35 @@ references will be sent.
  PKT-LINE("pusher" SP ident LF)
  PKT-LINE("pushee" SP url LF)
  PKT-LINE("nonce" SP nonce LF)
+ *PKT-LINE("push-option" SP push-option LF)
  PKT-LINE(LF)
  *PKT-LINE(command LF)
  *PKT-LINE(gpg-signature-lines LF)
  PKT-LINE("push-cert-end" LF)
 
-  packfile  = "PACK" 28*(OCTET)
+  push-option   =  1*( VCHAR | SP )
+
+
+If the server has advertised the 'push-options' capability and the client has
+specified 'push-options' as part of the capability list above, the client then
+sends its push options followed by a flush-pkt.
+
+
+  push-options  =  *PKT-LINE(push-option) flush-pkt
+
+
+For backwards compatibility with older Git servers, if the client sends a push
+cert and push options, it MUST send its push options both embedded within the
+push cert and after the push cert. (Note that the push options within the cert
+are prefixed, but the push options after the cert are not.) Both these lists
+MUST be the same, modulo the prefix.
+
+After that the packfile that
+should contain all the objects that the server will need to complete the new
+references will be sent.
+
+
+  packfile  =  "PACK" 28*(OCTET)
 
 
 If the receiving end does not support delete-refs, the sending end MUST
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f96834f42..fff5c7a54 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -473,7 +473,8 @@ static char *prepare_push_cert_nonce(const char *path, 
unsigned long stamp)
  * after dropping "_commit" from its name and possibly moving it out
  * of commit.c
  */
-static char *find_header(const char *msg, size_t len, const char *key)
+static char *find_header(const char *msg, size_t len, const char *key,
+const char **next_line)
 {
int key_len = strlen(key);
const char *line = msg;
@@ -486,6 +487,8 @@ static char *find_header(const char *msg, size_t len, const 
char *key)
if (line + key_len < eol &&
!memcmp(line, key, key_len) && line[key_len] == ' ') {
int offset = key_len + 1;
+   if (next_line)
+   *next_line = *eol ? eol + 1 : eol;
return xmemdupz(line + offset, (eol - line) - offset);
}
line = *eol ? eol + 1 : NULL;
@@ -495,7 +498,7 @@ static char *find_header(const char *msg, size_t len, const 
char *key)
 
 static const char *check_nonce(const char *buf, size_t len)
 {
-   char *nonce = find_header(buf, len, "nonce");
+   char *nonce = find_header(buf, len, "nonce", NULL);
unsigned long stamp, ostamp;

[PATCH v3 0/2] Clarify interaction between signed pushes and push options

[Jonathan Tan]

Changes from v2:
 - incorporated Junio's suggestions
 - incorporated Ævar's suggestions

Jonathan Tan (2):
  docs: correct receive.advertisePushOptions default
  receive-pack: verify push options in cert

 Documentation/config.txt  |  5 ++-
 Documentation/technical/pack-protocol.txt | 32 +++
 builtin/receive-pack.c| 51 +--
 t/t5534-push-signed.sh| 37 ++
 4 files changed, 114 insertions(+), 11 deletions(-)

-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH v3 1/2] docs: correct receive.advertisePushOptions default

[Jonathan Tan]

In commit c714e45 ("receive-pack: implement advertising and receiving
push options", 2016-07-14), receive-pack was taught to (among other
things) advertise that it understood push options, depending on
configuration. It was documented that it advertised such ability by
default; however, it actually does not. (In that commit, notice that
advertise_push_options defaults to 0, unlike advertise_atomic_push which
defaults to 1.)

Update the documentation to state that it does not advertise the ability
by default.

Signed-off-by: Jonathan Tan 
---
 Documentation/config.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5..3a847a62e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2620,9 +2620,8 @@ receive.advertiseAtomic::
capability, set this variable to false.
 
 receive.advertisePushOptions::
-   By default, git-receive-pack will advertise the push options
-   capability to its clients. If you don't want to advertise this
-   capability, set this variable to false.
+   When set to true, git-receive-pack will advertise the push options
+   capability to its clients. False by default.
 
 receive.autogc::
By default, git-receive-pack will run "git-gc --auto" after
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH 3/8] pathspec: change PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag

[Brandon Williams]

It's confusing to have two different 'strip submodule slash' flags which
do subtly different things.  Both
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP will accomplish the same task of
striping a slash from a pathspec which matches a submodule entry in the
index.  The only difference is that
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE will perform additional checks
and die if a pathspec has a leading path component which corresponds to
a submodule.  This additional functionality should be split out into its
own flag.

To this end, rename the PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag to
PATHSPEC_SUBMODULE_LEADING_PATH and change its behavior to only die if a
path descends into a submodule.  In addition add the
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to callers which relied on the
old slash stripping functionality.

Signed-off-by: Brandon Williams 
---
 builtin/add.c  |  3 ++-
 builtin/check-ignore.c |  3 ++-
 pathspec.c | 32 
 pathspec.h |  9 +++--
 4 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index ec58e3679..2aa9aeab9 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -389,7 +389,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
parse_pathspec(, 0,
   PATHSPEC_PREFER_FULL |
   PATHSPEC_SYMLINK_LEADING_PATH |
-  PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE,
+  PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
+  PATHSPEC_SUBMODULE_LEADING_PATH,
   prefix, argv);
 
if (add_new_files) {
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 1d73d3ca3..73237b2b1 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -87,7 +87,8 @@ static int check_ignore(struct dir_struct *dir,
parse_pathspec(,
   PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,
   PATHSPEC_SYMLINK_LEADING_PATH |
-  PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE |
+  PATHSPEC_SUBMODULE_LEADING_PATH |
+  PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
   PATHSPEC_KEEP_ORDER,
   prefix, argv);
 
diff --git a/pathspec.c b/pathspec.c
index e37256c3b..f37d5769d 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -398,29 +398,29 @@ static void strip_submodule_slash_cheap(struct 
pathspec_item *item)
}
 }
 
-static void strip_submodule_slash_expensive(struct pathspec_item *item)
+static void die_path_inside_submodule(const struct pathspec_item *item,
+ const struct index_state *istate)
 {
int i;
 
-   for (i = 0; i < active_nr; i++) {
-   struct cache_entry *ce = active_cache[i];
+   for (i = 0; i < istate->cache_nr; i++) {
+   struct cache_entry *ce = istate->cache[i];
int ce_len = ce_namelen(ce);
 
if (!S_ISGITLINK(ce->ce_mode))
continue;
 
-   if (item->len <= ce_len || item->match[ce_len] != '/' ||
-   memcmp(ce->name, item->match, ce_len))
+   if (item->len <= ce_len)
+   continue;
+   if (item->match[ce_len] != '/')
+   continue;
+   if (strncmp(ce->name, item->match, ce_len))
+   continue;
+   if (item->len == ce_len + 1)
continue;
 
-   if (item->len == ce_len + 1) {
-   /* strip trailing slash */
-   item->len--;
-   item->match[item->len] = '\0';
-   } else {
-   die(_("Pathspec '%s' is in submodule '%.*s'"),
-   item->original, ce_len, ce->name);
-   }
+   die(_("Pathspec '%s' is in submodule '%.*s'"),
+   item->original, ce_len, ce->name);
}
 }
 
@@ -499,9 +499,6 @@ static void init_pathspec_item(struct pathspec_item *item, 
unsigned flags,
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
strip_submodule_slash_cheap(item);
 
-   if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
-   strip_submodule_slash_expensive(item);
-
if (magic & PATHSPEC_LITERAL) {
item->nowildcard_len = item->len;
} else {
@@ -639,6 +636,9 @@ void parse_pathspec(struct pathspec *pathspec,
die(_("pathspec '%s' is beyond a symbolic link"), 
entry);
}
 
+   if (flags & PATHSPEC_SUBMODULE_LEADING_PATH)
+   die_path_inside_submodule(item + i, _index);
+
if (item[i].nowildcard_len < item[i].len)
pathspec->has_wildcard = 1;
pathspec->magic |= item[i].magic;

[PATCH 5/8] pathspec: convert strip_submodule_slash to take an index

[Brandon Williams]

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

diff --git a/pathspec.c b/pathspec.c
index a1297ca02..cff069536 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -386,12 +386,13 @@ static const char *parse_element_magic(unsigned *magic, 
int *prefix_len,
return parse_short_magic(magic, elem);
 }
 
-static void strip_submodule_slash(struct pathspec_item *item)
+static void strip_submodule_slash(struct pathspec_item *item,
+ const struct index_state *istate)
 {
if (item->len >= 1 && item->match[item->len - 1] == '/') {
-   int i = cache_name_pos(item->match, item->len - 1);
+   int i = index_name_pos(istate, item->match, item->len - 1);
 
-   if (i >= 0 && S_ISGITLINK(active_cache[i]->ce_mode)) {
+   if (i >= 0 && S_ISGITLINK(istate->cache[i]->ce_mode)) {
item->len--;
item->match[item->len] = '\0';
}
@@ -497,7 +498,7 @@ static void init_pathspec_item(struct pathspec_item *item, 
unsigned flags,
}
 
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH)
-   strip_submodule_slash(item);
+   strip_submodule_slash(item, _index);
 
if (magic & PATHSPEC_LITERAL) {
item->nowildcard_len = item->len;
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH 6/8] pathspec: convert find_pathspecs_matching_against_index to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 builtin/add.c  |  4 ++--
 builtin/check-ignore.c |  2 +-
 pathspec.c | 10 ++
 pathspec.h |  7 +--
 4 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 51d7a5506..4e3bf20c2 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -136,7 +136,7 @@ static char *prune_directory(struct dir_struct *dir, struct 
pathspec *pathspec,
*dst++ = entry;
}
dir->nr = dst - dir->entries;
-   add_pathspec_matches_against_index(pathspec, seen);
+   add_pathspec_matches_against_index(pathspec, _index, seen);
return seen;
 }
 
@@ -418,7 +418,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int i;
 
if (!seen)
-   seen = find_pathspecs_matching_against_index();
+   seen = find_pathspecs_matching_against_index(, 
_index);
 
/*
 * file_exists() assumes exact match
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 694e4c61b..446b76dcf 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -97,7 +97,7 @@ static int check_ignore(struct dir_struct *dir,
 * should not be ignored, in order to be consistent with
 * 'git status', 'git add' etc.
 */
-   seen = find_pathspecs_matching_against_index();
+   seen = find_pathspecs_matching_against_index(, _index);
for (i = 0; i < pathspec.nr; i++) {
full_path = pathspec.items[i].match;
exclude = NULL;
diff --git a/pathspec.c b/pathspec.c
index cff069536..bbd71b48b 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -17,6 +17,7 @@
  * to use find_pathspecs_matching_against_index() instead.
  */
 void add_pathspec_matches_against_index(const struct pathspec *pathspec,
+   const struct index_state *istate,
char *seen)
 {
int num_unmatched = 0, i;
@@ -32,8 +33,8 @@ void add_pathspec_matches_against_index(const struct pathspec 
*pathspec,
num_unmatched++;
if (!num_unmatched)
return;
-   for (i = 0; i < active_nr; i++) {
-   const struct cache_entry *ce = active_cache[i];
+   for (i = 0; i < istate->cache_nr; i++) {
+   const struct cache_entry *ce = istate->cache[i];
ce_path_match(ce, pathspec, seen);
}
 }
@@ -46,10 +47,11 @@ void add_pathspec_matches_against_index(const struct 
pathspec *pathspec,
  * nature of the "closest" (i.e. most specific) matches which each of the
  * given pathspecs achieves against all items in the index.
  */
-char *find_pathspecs_matching_against_index(const struct pathspec *pathspec)
+char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
+   const struct index_state *istate)
 {
char *seen = xcalloc(pathspec->nr, 1);
-   add_pathspec_matches_against_index(pathspec, seen);
+   add_pathspec_matches_against_index(pathspec, istate, seen);
return seen;
 }
 
diff --git a/pathspec.h b/pathspec.h
index dd6456d5d..dfb22440a 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -103,7 +103,10 @@ static inline int ps_strcmp(const struct pathspec_item 
*item,
return strcmp(s1, s2);
 }
 
-extern char *find_pathspecs_matching_against_index(const struct pathspec 
*pathspec);
-extern void add_pathspec_matches_against_index(const struct pathspec 
*pathspec, char *seen);
+extern void add_pathspec_matches_against_index(const struct pathspec *pathspec,
+  const struct index_state *istate,
+  char *seen);
+extern char *find_pathspecs_matching_against_index(const struct pathspec 
*pathspec,
+  const struct index_state 
*istate);
 
 #endif /* PATHSPEC_H */
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH 7/8] pathspec: convert init_pathspec_item to take an index

[Brandon Williams]

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

diff --git a/pathspec.c b/pathspec.c
index bbd71b48b..c7ed8b3fb 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -430,7 +430,9 @@ static void die_path_inside_submodule(const struct 
pathspec_item *item,
 /*
  * Perform the initialization of a pathspec_item based on a pathspec element.
  */
-static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
+static void init_pathspec_item(struct pathspec_item *item,
+  const struct index_state *istate,
+  unsigned flags,
   const char *prefix, int prefixlen,
   const char *elt)
 {
@@ -500,7 +502,7 @@ static void init_pathspec_item(struct pathspec_item *item, 
unsigned flags,
}
 
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH)
-   strip_submodule_slash(item, _index);
+   strip_submodule_slash(item, istate);
 
if (magic & PATHSPEC_LITERAL) {
item->nowildcard_len = item->len;
@@ -627,7 +629,8 @@ void parse_pathspec(struct pathspec *pathspec,
for (i = 0; i < n; i++) {
entry = argv[i];
 
-   init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
+   init_pathspec_item(item + i, _index, flags,
+  prefix, prefixlen, entry);
 
if (item[i].magic & PATHSPEC_EXCLUDE)
nr_exclude++;
@@ -653,7 +656,7 @@ void parse_pathspec(struct pathspec *pathspec,
 */
if (nr_exclude == n) {
int plen = (!(flags & PATHSPEC_PREFER_CWD)) ? 0 : prefixlen;
-   init_pathspec_item(item + n, 0, prefix, plen, "");
+   init_pathspec_item(item + n, _index, 0, prefix, plen, "");
pathspec->nr++;
}
 
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH 2/8] submodule: add die_in_unpopulated_submodule function

[Brandon Williams]

Currently 'git add' is the only command which dies when launched from an
unpopulated submodule (the place-holder directory for a submodule which
hasn't been checked out).  This is triggered implicitly by passing the
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag to 'parse_pathspec()'.

Instead make this desire more explicit by creating a function
'die_in_unpopulated_submodule()' which dies if the provided 'prefix' has
a leading path component which matches a submodule in the the index.

Signed-off-by: Brandon Williams 
---
 builtin/add.c|  3 +++
 pathspec.c   | 29 -
 submodule.c  | 30 ++
 submodule.h  |  2 ++
 t/t6134-pathspec-in-submodule.sh |  6 +-
 5 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 9f53f020d..ec58e3679 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -17,6 +17,7 @@
 #include "revision.h"
 #include "bulk-checkin.h"
 #include "argv-array.h"
+#include "submodule.h"
 
 static const char * const builtin_add_usage[] = {
N_("git add [] [--] ..."),
@@ -379,6 +380,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (read_cache() < 0)
die(_("index file corrupt"));
 
+   die_in_unpopulated_submodule(_index, prefix);
+
/*
 * Check the "pathspec '%s' did not match any files" block
 * below before enabling new magic.
diff --git a/pathspec.c b/pathspec.c
index 61b5b1273..e37256c3b 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -424,27 +424,6 @@ static void strip_submodule_slash_expensive(struct 
pathspec_item *item)
}
 }
 
-static void die_inside_submodule_path(struct pathspec_item *item)
-{
-   int i;
-
-   for (i = 0; i < active_nr; i++) {
-   struct cache_entry *ce = active_cache[i];
-   int ce_len = ce_namelen(ce);
-
-   if (!S_ISGITLINK(ce->ce_mode))
-   continue;
-
-   if (item->len < ce_len ||
-   !(item->match[ce_len] == '/' || item->match[ce_len] == 
'\0') ||
-   memcmp(ce->name, item->match, ce_len))
-   continue;
-
-   die(_("Pathspec '%s' is in submodule '%.*s'"),
-   item->original, ce_len, ce->name);
-   }
-}
-
 /*
  * Perform the initialization of a pathspec_item based on a pathspec element.
  */
@@ -547,14 +526,6 @@ static void init_pathspec_item(struct pathspec_item *item, 
unsigned flags,
/* sanity checks, pathspec matchers assume these are sane */
if (item->nowildcard_len > item->len ||
item->prefix > item->len) {
-   /*
-* This case can be triggered by the user pointing us to a
-* pathspec inside a submodule, which is an input error.
-* Detect that here and complain, but fallback in the
-* non-submodule case to a BUG, as we have no idea what
-* would trigger that.
-*/
-   die_inside_submodule_path(item);
die ("BUG: error initializing pathspec_item");
}
 }
diff --git a/submodule.c b/submodule.c
index 7c3c4b17f..9e0502f25 100644
--- a/submodule.c
+++ b/submodule.c
@@ -281,6 +281,36 @@ int is_submodule_populated_gently(const char *path, int 
*return_error_code)
return ret;
 }
 
+/*
+ * Dies if the provided 'prefix' corresponds to an unpopulated submodule
+ */
+void die_in_unpopulated_submodule(const struct index_state *istate,
+ const char *prefix)
+{
+   int i, prefixlen;
+
+   if (!prefix)
+   return;
+
+   prefixlen = strlen(prefix);
+
+   for (i = 0; i < istate->cache_nr; i++) {
+   struct cache_entry *ce = istate->cache[i];
+   int ce_len = ce_namelen(ce);
+
+   if (!S_ISGITLINK(ce->ce_mode))
+   continue;
+   if (prefixlen <= ce_len)
+   continue;
+   if (strncmp(ce->name, prefix, ce_len))
+   continue;
+   if (prefix[ce_len] != '/')
+   continue;
+
+   die(_("in unpopulated submodule '%s'"), ce->name);
+   }
+}
+
 int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst)
 {
diff --git a/submodule.h b/submodule.h
index 8a8bc49dc..f4fe6374d 100644
--- a/submodule.h
+++ b/submodule.h
@@ -48,6 +48,8 @@ extern int is_submodule_initialized(const char *path);
  * Otherwise the return error code is the same as of resolve_gitdir_gently.
  */
 extern int is_submodule_populated_gently(const char *path, int 
*return_error_code);
+extern void die_in_unpopulated_submodule(const struct index_state *istate,
+const char *prefix);
 extern int 

[PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Brandon Williams]

Convert 'parse_pathspec()' to take an index parameter.

Since the index is only needed when the PATHSPEC_SUBMODULE_LEADING_PATH
and PATHSPEC_STRIP_SUBMODULE_SLASH flags are given, add a check
requiring a non-NULL index when either of these flags are given.
Convert callers which use these two flags to pass in an index while
having other callers pass in NULL.

Now that pathspec.c does not use any cache macros and has no references
to 'the_index', mark it by defining NO_THE_INDEX_COMPATIBILITY_MACROS.

Signed-off-by: Brandon Williams 
---
 archive.c   |  4 ++--
 builtin/add.c   |  4 ++--
 builtin/blame.c |  2 +-
 builtin/check-ignore.c  |  2 +-
 builtin/checkout.c  |  2 +-
 builtin/clean.c |  2 +-
 builtin/commit.c|  4 ++--
 builtin/grep.c  |  2 +-
 builtin/ls-files.c  |  4 ++--
 builtin/ls-tree.c   |  5 +++--
 builtin/rerere.c|  2 +-
 builtin/reset.c |  2 +-
 builtin/rm.c|  2 +-
 builtin/submodule--helper.c |  2 +-
 builtin/update-index.c  |  2 +-
 line-log.c  |  2 +-
 pathspec.c  | 13 ++---
 pathspec.h  |  1 +
 revision.c  |  5 +++--
 submodule.c |  2 +-
 tree-diff.c |  2 +-
 21 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/archive.c b/archive.c
index 60b889198..ce9b30f2e 100644
--- a/archive.c
+++ b/archive.c
@@ -306,7 +306,7 @@ static int path_exists(struct tree *tree, const char *path)
struct pathspec pathspec;
int ret;
 
-   parse_pathspec(, 0, 0, "", paths);
+   parse_pathspec(, NULL, 0, 0, "", paths);
pathspec.recursive = 1;
ret = read_tree_recursive(tree, "", 0, 0, ,
  reject_entry, );
@@ -322,7 +322,7 @@ static void parse_pathspec_arg(const char **pathspec,
 * Also if pathspec patterns are dependent, we're in big
 * trouble as we test each one separately
 */
-   parse_pathspec(_args->pathspec, 0,
+   parse_pathspec(_args->pathspec, NULL, 0,
   PATHSPEC_PREFER_FULL,
   "", pathspec);
ar_args->pathspec.recursive = 1;
diff --git a/builtin/add.c b/builtin/add.c
index 4e3bf20c2..23606db39 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -181,7 +181,7 @@ int interactive_add(int argc, const char **argv, const char 
*prefix, int patch)
 {
struct pathspec pathspec;
 
-   parse_pathspec(, 0,
+   parse_pathspec(, NULL, 0,
   PATHSPEC_PREFER_FULL |
   PATHSPEC_SYMLINK_LEADING_PATH |
   PATHSPEC_PREFIX_ORIGIN,
@@ -386,7 +386,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 * Check the "pathspec '%s' did not match any files" block
 * below before enabling new magic.
 */
-   parse_pathspec(, 0,
+   parse_pathspec(, _index, 0,
   PATHSPEC_PREFER_FULL |
   PATHSPEC_SYMLINK_LEADING_PATH |
   PATHSPEC_STRIP_SUBMODULE_SLASH |
diff --git a/builtin/blame.c b/builtin/blame.c
index f7aa95f4b..e37837c17 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -557,7 +557,7 @@ static struct origin *find_origin(struct scoreboard *sb,
paths[0] = origin->path;
paths[1] = NULL;
 
-   parse_pathspec(_opts.pathspec,
+   parse_pathspec(_opts.pathspec, NULL,
   PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
   PATHSPEC_LITERAL_PATH, "", paths);
diff_setup_done(_opts);
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 446b76dcf..90169e79a 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -84,7 +84,7 @@ static int check_ignore(struct dir_struct *dir,
 * check-ignore just needs paths. Magic beyond :/ is really
 * irrelevant.
 */
-   parse_pathspec(,
+   parse_pathspec(, _index,
   PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,
   PATHSPEC_SYMLINK_LEADING_PATH |
   PATHSPEC_SUBMODULE_LEADING_PATH |
diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfa5419f3..cb8ed09f6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1274,7 +1274,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
}
 
if (argc) {
-   parse_pathspec(, 0,
+   parse_pathspec(, NULL, 0,
   opts.patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0,
   prefix, argv);
 
diff --git a/builtin/clean.c b/builtin/clean.c
index d861f836a..ebc980b4f 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -926,7 +926,7 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
for (i = 0; i < exclude_list.nr; i++)

[PATCH 1/8] pathspec: provide a more descriptive die message

[Brandon Williams]

The current message displayed upon an internal error in
'init_pathspec_item()' isn't very descriptive and doesn't provide much
context to where the error occurred.  Update the error message to
provide more context to where the error occured.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pathspec.c b/pathspec.c
index 303efda83..61b5b1273 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -555,7 +555,7 @@ static void init_pathspec_item(struct pathspec_item *item, 
unsigned flags,
 * would trigger that.
 */
die_inside_submodule_path(item);
-   die ("BUG: item->nowildcard_len > item->len || item->prefix > 
item->len)");
+   die ("BUG: error initializing pathspec_item");
}
 }
 
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH 0/8] convert pathspec.c to take an index parameter

[Brandon Williams]

This is another conversion series to convert the pathspec library code to take
in an index parameter instead of relying on cache macros or on the global
variable 'the_index'.

While I was working in the pathspec code I thought it would be good to do a
little more cleanup and make the API cleaner.  More specifically consolidating
the 'strip submodule slash' flags into a single flag while splitting out the
'submodule leading path' check performed in the expensive case into its own
flag.

Brandon Williams (8):
  pathspec: provide a more descriptive die message
  submodule: add die_in_unpopulated_submodule function
  pathspec: change PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag
  pathspec: rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP
  pathspec: convert strip_submodule_slash to take an index
  pathspec: convert find_pathspecs_matching_against_index to take an
index
  pathspec: convert init_pathspec_item to take an index
  pathspec: convert parse_pathspec to take an index

 archive.c|  4 +-
 builtin/add.c| 14 +++---
 builtin/blame.c  |  2 +-
 builtin/check-ignore.c   |  7 +--
 builtin/checkout.c   |  2 +-
 builtin/clean.c  |  2 +-
 builtin/commit.c |  4 +-
 builtin/grep.c   |  2 +-
 builtin/ls-files.c   |  6 +--
 builtin/ls-tree.c|  5 ++-
 builtin/rerere.c |  2 +-
 builtin/reset.c  |  4 +-
 builtin/rm.c |  4 +-
 builtin/submodule--helper.c  |  4 +-
 builtin/update-index.c   |  2 +-
 line-log.c   |  2 +-
 pathspec.c   | 92 +---
 pathspec.h   | 19 +
 revision.c   |  5 ++-
 submodule.c  | 32 +-
 submodule.h  |  2 +
 t/t6134-pathspec-in-submodule.sh |  6 +--
 tree-diff.c  |  2 +-
 23 files changed, 122 insertions(+), 102 deletions(-)

-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH 4/8] pathspec: rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP

[Brandon Williams]

Now that there is only a single flag which strips a submodule slash,
rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to
PATHSPEC_STRIP_SUBMODULE_SLASH.

Signed-off-by: Brandon Williams 
---
 builtin/add.c   | 2 +-
 builtin/check-ignore.c  | 2 +-
 builtin/ls-files.c  | 2 +-
 builtin/reset.c | 2 +-
 builtin/rm.c| 2 +-
 builtin/submodule--helper.c | 2 +-
 pathspec.c  | 6 +++---
 pathspec.h  | 2 +-
 8 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 2aa9aeab9..51d7a5506 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -389,7 +389,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
parse_pathspec(, 0,
   PATHSPEC_PREFER_FULL |
   PATHSPEC_SYMLINK_LEADING_PATH |
-  PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
+  PATHSPEC_STRIP_SUBMODULE_SLASH |
   PATHSPEC_SUBMODULE_LEADING_PATH,
   prefix, argv);
 
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 73237b2b1..694e4c61b 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -88,7 +88,7 @@ static int check_ignore(struct dir_struct *dir,
   PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,
   PATHSPEC_SYMLINK_LEADING_PATH |
   PATHSPEC_SUBMODULE_LEADING_PATH |
-  PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
+  PATHSPEC_STRIP_SUBMODULE_SLASH |
   PATHSPEC_KEEP_ORDER,
   prefix, argv);
 
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index d449e46db..e9dee2e41 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -618,7 +618,7 @@ int cmd_ls_files(int argc, const char **argv, const char 
*cmd_prefix)
 
parse_pathspec(, 0,
   PATHSPEC_PREFER_CWD |
-  PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+  PATHSPEC_STRIP_SUBMODULE_SLASH,
   prefix, argv);
 
/*
diff --git a/builtin/reset.c b/builtin/reset.c
index fc3b906c4..529f2f9be 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -236,7 +236,7 @@ static void parse_args(struct pathspec *pathspec,
 
parse_pathspec(pathspec, 0,
   PATHSPEC_PREFER_FULL |
-  PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
+  PATHSPEC_STRIP_SUBMODULE_SLASH |
   (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),
   prefix, argv);
 }
diff --git a/builtin/rm.c b/builtin/rm.c
index fb79dcab1..8fe12d0a7 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -272,7 +272,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 
parse_pathspec(, 0,
   PATHSPEC_PREFER_CWD |
-  PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+  PATHSPEC_STRIP_SUBMODULE_SLASH,
   prefix, argv);
refresh_index(_index, REFRESH_QUIET, , NULL, NULL);
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 85aafe46a..69149b557 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -234,7 +234,7 @@ static int module_list_compute(int argc, const char **argv,
char *ps_matched = NULL;
parse_pathspec(pathspec, 0,
   PATHSPEC_PREFER_FULL |
-  PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+  PATHSPEC_STRIP_SUBMODULE_SLASH,
   prefix, argv);
 
if (pathspec->nr)
diff --git a/pathspec.c b/pathspec.c
index f37d5769d..a1297ca02 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -386,7 +386,7 @@ static const char *parse_element_magic(unsigned *magic, int 
*prefix_len,
return parse_short_magic(magic, elem);
 }
 
-static void strip_submodule_slash_cheap(struct pathspec_item *item)
+static void strip_submodule_slash(struct pathspec_item *item)
 {
if (item->len >= 1 && item->match[item->len - 1] == '/') {
int i = cache_name_pos(item->match, item->len - 1);
@@ -496,8 +496,8 @@ static void init_pathspec_item(struct pathspec_item *item, 
unsigned flags,
item->original = xstrdup(elt);
}
 
-   if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
-   strip_submodule_slash_cheap(item);
+   if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH)
+   strip_submodule_slash(item);
 
if (magic & PATHSPEC_LITERAL) {
item->nowildcard_len = item->len;
diff --git a/pathspec.h b/pathspec.h
index 93a819cbf..dd6456d5d 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -59,7 +59,7 @@ struct pathspec {
 #define PATHSPEC_PREFER_FULL (1<<1) /* No args means match everything */
 #define PATHSPEC_MAXDEPTH_VALID (1<<2) /* max_depth field is valid */
 /* strip the trailing slash if the 

[PATCH] fetch-pack: always allow fetching of literal SHA1s

[Jonathan Tan]

fetch-pack, when fetching a literal SHA-1 from a server that is not
configured with uploadpack.allowtipsha1inwant (or similar), always
returns an error message of the form "Server does not allow request for
unadvertised object %s". However, it is sometimes the case that such
object is advertised.

Teach fetch-pack to also check the SHA-1s of the refs in the received
ref advertisement if a literal SHA-1 was given by the user.

Signed-off-by: Jonathan Tan 
---
 fetch-pack.c  | 30 --
 t/t5500-fetch-pack.sh |  6 ++
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index afb8b0502..180405dff 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -592,6 +592,14 @@ static void mark_recent_complete_commits(struct 
fetch_pack_args *args,
}
 }
 
+static int is_literal_sha1(const struct ref *ref)
+{
+   struct object_id oid;
+   return !get_oid_hex(ref->name, ) &&
+  !ref->name[40] &&
+  !oidcmp(, >old_oid);
+}
+
 static void filter_refs(struct fetch_pack_args *args,
struct ref **refs,
struct ref **sought, int nr_sought)
@@ -601,7 +609,6 @@ static void filter_refs(struct fetch_pack_args *args,
struct ref *ref, *next;
int i;
 
-   i = 0;
for (ref = *refs; ref; ref = next) {
int keep = 0;
next = ref->next;
@@ -610,15 +617,14 @@ static void filter_refs(struct fetch_pack_args *args,
check_refname_format(ref->name, 0))
; /* trash */
else {
-   while (i < nr_sought) {
-   int cmp = strcmp(ref->name, sought[i]->name);
-   if (cmp < 0)
-   break; /* definitely do not have it */
-   else if (cmp == 0) {
-   keep = 1; /* definitely have it */
-   sought[i]->match_status = REF_MATCHED;
+   for (i = 0; i < nr_sought; i++) {
+   struct ref *s = sought[i];
+   if (!strcmp(ref->name, s->name) ||
+   (is_literal_sha1(s) &&
+!oidcmp(>old_oid, >old_oid))) {
+   keep = 1;
+   s->match_status = REF_MATCHED;
}
-   i++;
}
}
 
@@ -637,14 +643,10 @@ static void filter_refs(struct fetch_pack_args *args,
 
/* Append unmatched requests to the list */
for (i = 0; i < nr_sought; i++) {
-   unsigned char sha1[20];
-
ref = sought[i];
if (ref->match_status != REF_NOT_MATCHED)
continue;
-   if (get_sha1_hex(ref->name, sha1) ||
-   ref->name[40] != '\0' ||
-   hashcmp(sha1, ref->old_oid.hash))
+   if (!is_literal_sha1(ref))
continue;
 
if ((allow_unadvertised_object_request &
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index b5865b385..d87983bc2 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -547,6 +547,12 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '
git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)
 '
 
+test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a 
ref' '
+   git init server &&
+   test_commit -C server 4 &&
+   git fetch-pack server $(git -C server rev-parse refs/heads/master)
+'
+
 check_prot_path () {
cat >expected <<-EOF &&
Diag: url=$1
-- 
2.13.0.rc2.291.g57267f2277-goog

Re: [PATCH v2] travis-ci: retry if Git for Windows CI returns HTTP error 502 or 503

[Lars Schneider]

> On 09 May 2017, at 07:31, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>> The Git for Windows CI web app sometimes returns HTTP errors of
>> "502 bad gateway" or "503 service unavailable" [1]. We also need to
>> check the HTTP content because the GfW web app seems to pass through
>> (error) results from other Azure calls with HTTP code 200.
>> Wait a little and retry the request if this happens.
>> 
>> [1] 
>> https://docs.microsoft.com/en-in/azure/app-service-web/app-service-web-troubleshoot-http-502-http-503
>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> 
>> Hi Junio,
>> 
>> I can't really test this as my TravisCI account does not have the
>> extended timeout and I am unable to reproduce the error.
>> 
>> It would be great if we could test this is a little bit in pu.
> 
> This has been in 'pu' for a while.  
> 
> As the patch simply discards 502 (and others), it is unclear if the
> failing test on 'next' is now gone, or the attempt to run 'pu'
> happened to be lucky not to get one, from the output we can see in
> https://travis-ci.org/git/git/jobs/229867212
> 
> Are you comfortable enough to move this forward?

Yes, please move it forward. I haven't seen a "502 - Web server 
received an invalid response" on pu for a while. That means the
patch should work as expected.


Unrelated to this patch I have, however, seen two kinds of timeouts:

(1) Timeout in the "notStarted" state. This job eventually finished
with a failure but it did start only *after* 3h:
https://travis-ci.org/git/git/jobs/230225611

(2) Timeout in the "in progress" state. This job eventually finished
successfully but it took longer than 3h:
https://travis-ci.org/git/git/jobs/229867248

Right now the timeout generates potential false negative results. 
I would like to change that and respond with a successful build 
*before* we approach the 3h timeout. This means we could generate
false positives. Although this is not ideal, I think that is the better 
compromise as a failing Windows build would usually fail quickly 
(e.g. in the compile step).

What do you guys think? Would you be OK with that reasoning?
If the Git for Windows builds get more stable over time then
we could reevaluate this compromise.


- Lars

Re: [PATCH] fixup! use perl instead of sed

[Ævar Arnfjörð Bjarmason]

On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan  wrote:
> Signed-off-by: Jonathan Tan 
> ---
>
> Thanks - I didn't realize the existence of the test lint. Here's a
> fixup. Let me know if you prefer a full reroll.
>
> I had to use perl because "push" is a binary file (the first line
> contains a NUL).
>
>
>  t/t5534-push-signed.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
> index 0ef6f66b5..3ee58dafb 100755
> --- a/t/t5534-push-signed.sh
> +++ b/t/t5534-push-signed.sh
> @@ -152,11 +152,11 @@ test_expect_success GPG 'inconsistent push options in 
> signed push not allowed' '
> # Tweak the push output to make the push option outside the cert
> # different, then replay it on a fresh dst, checking that ff is not
> # deleted.
> -   sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&
> +   perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak &&
> prepare_dst &&
> git -C dst config receive.certnonceseed sekrit &&
> git -C dst config receive.advertisepushoptions 1 &&
> -   git receive-pack dst out &&
> +   git receive-pack dst out &&

The test should have a PERL prerequisite now, that's missing.

Also using \1 will likely be deprecated in future versions of perl at
some point:

$ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/"
\1 better written as $1 at -e line 1.
hifoo

Finally, you can just use -i like you did with sed, no need for the tempfile:

$ echo hibar >push
$ perl -pi -e 's/([^ ])bar/$1baz/' push
$ cat push
hibaz

> git -C dst rev-parse ff &&
> grep "inconsistent push options" out
>  '
> --
> 2.13.0.rc2.291.g57267f2277-goog
>

[PATCH] fixup! use perl instead of sed

[Jonathan Tan]

Signed-off-by: Jonathan Tan 
---

Thanks - I didn't realize the existence of the test lint. Here's a
fixup. Let me know if you prefer a full reroll.

I had to use perl because "push" is a binary file (the first line
contains a NUL).


 t/t5534-push-signed.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 0ef6f66b5..3ee58dafb 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -152,11 +152,11 @@ test_expect_success GPG 'inconsistent push options in 
signed push not allowed' '
# Tweak the push output to make the push option outside the cert
# different, then replay it on a fresh dst, checking that ff is not
# deleted.
-   sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&
+   perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak &&
prepare_dst &&
git -C dst config receive.certnonceseed sekrit &&
git -C dst config receive.advertisepushoptions 1 &&
-   git receive-pack dst out &&
+   git receive-pack dst out &&
git -C dst rev-parse ff &&
grep "inconsistent push options" out
 '
-- 
2.13.0.rc2.291.g57267f2277-goog

Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)

[Ævar Arnfjörð Bjarmason]

On Tue, May 9, 2017 at 4:22 PM, demerphq  wrote:
> On 9 May 2017 at 13:12, Ævar Arnfjörð Bjarmason  wrote:
>> On Tue, May 9, 2017 at 2:37 AM, brian m. carlson
>>  wrote:
>>> On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> * gitweb is vulnerable to CPU DoS now in its default configuration.
>> It's easy to provide an ERE that ends up slurping up 100% CPU for
>> several seconds on any non-trivial sized repo, do that in parallel &
>> you have a DoS vector.
>
> Does one need an ERE? Can't one do that now to many parts of git just
> with a glob?

in practice I don't think so because:

1) I'm now aware of any place where we expose globbing over the wire.

2) AFAICT for the issue detailed in [1] to trigger you also need a
pathological filename in the repo, e.g. I can't get git-ls-files to go
quadratic on either git.git or linux.git, whereas it's pretty easy to
come up with a really expensive regex since there's more content to
choose from when matching file content than filenames.

1. https://public-inbox.org/git/20170424211249.28553-1-ava...@gmail.com/

Re: [GIT PULL] l10n updates for 2.13.0 round 2.1

[Junio C Hamano]

Jiang Xin  writes:

> Merged another l10n contribution, please pull the new tag
> l10n-2.13.0-rnd2.1 (old tag is deleted):

Yeah, I see our mails crossed.  Will pull.

Thanks!

Re: [GIT PULL] l10n updates for 2.13.0 round 2

[Junio C Hamano]

Jiang Xin  writes:

> Hi Junio,
>
> I can not send email outside at work, but now I am back home. Here is
> the pull request:
>
> The following changes since commit 4fa66c85f11bc5a541462ca5ae3246aa0ce02e74:
>
>   Git 2.13-rc2 (2017-05-04 16:27:19 +0900)
>
> are available in the git repository at:
>
>   git://github.com/git-l10n/git-po tags/l10n-2.13.0-rnd2
>
> for you to fetch changes up to 60638e9816d0aae40d4234d1a068f94fabc2fd4d:
>
>   l10n: zh_CN: for git v2.13.0 l10n round 2 (2017-05-09 21:55:38 +0800)

Thanks.  

I see another merge for sv and the tag is now l10n-2.13.0-rnd2.1
(i.e. I couldn't find -rnd2 tag), which I assume is what you meant
me to fetch?

Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)

[demerphq]

On 9 May 2017 at 13:12, Ævar Arnfjörð Bjarmason  wrote:
> On Tue, May 9, 2017 at 2:37 AM, brian m. carlson
>  wrote:
>> On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
> * gitweb is vulnerable to CPU DoS now in its default configuration.
> It's easy to provide an ERE that ends up slurping up 100% CPU for
> several seconds on any non-trivial sized repo, do that in parallel &
> you have a DoS vector.

Does one need an ERE? Can't one do that now to many parts of git just
with a glob?

Yves

Re: [GIT PULL] l10n updates for 2.13.0 round 2.1

[Jiang Xin]

Hi Junio,

Merged another l10n contribution, please pull the new tag
l10n-2.13.0-rnd2.1 (old tag is deleted):

The following changes since commit 4fa66c85f11bc5a541462ca5ae3246aa0ce02e74:

  Git 2.13-rc2 (2017-05-04 16:27:19 +0900)

are available in the git repository at:

  git://github.com/git-l10n/git-po tags/l10n-2.13.0-rnd2.1

for you to fetch changes up to 961f9c8b1b0f6bff09cb876f95e9ca33702763ac:

  Merge branch 'master' of git://github.com/nafmo/git-l10n-sv
(2017-05-09 22:12:34 +0800)


l10n for Git 2.13.0 round 2.1


Alexander Shopov (2):
  l10n: bg.po: Updated Bulgarian translation (3201t)
  l10n: bg.po: Updated Bulgarian translation (3195t)

Jean-Noel Avila (2):
  l10n: fr.po v2.13 round 1
  l10n: fr.po v2.13 rnd 2

Jiang Xin (10):
  l10n: git.pot: v2.13.0 round 1 (96 new, 37 removed)
  Merge branch 'master' of https://github.com/vnwildman/git
  Merge branch 'fr_l10n_v2.13_rnd1' of git://github.com/jnavila/git
  l10n: zh_CN: for git v2.13.0 l10n round 1
  Merge branch 'master' of git://github.com/git-l10n/git-po
  l10n: git.pot: v2.13.0 round 2 (4 new, 7 removed)
  Merge branch 'fr_l10n_v2.13_rnd2' of git://github.com/jnavila/git
  Merge branch 'master' of https://github.com/vnwildman/git
  l10n: zh_CN: for git v2.13.0 l10n round 2
  Merge branch 'master' of git://github.com/nafmo/git-l10n-sv

Jordi Mas (1):
  l10n: Update Catalan translation

Michael J Gruber (1):
  l10n: de.po: lower case after semi-colon

Peter Krefting (2):
  l10n: sv.po: Update Swedish translation (3199t0f0u)
  l10n: sv.po: Update Swedish translation (3195t0f0u)

Ralf Thielow (2):
  l10n: de.po: update German translation
  l10n: de.po: translate 4 new messages

Ray Chen (1):
  l10n: zh_CN: review for git v2.13.0 l10n round 1

Trần Ngọc Quân (2):
  l10n: vi.po(3198t): Updated Vietnamese translation for v2.13.0-rc0
  l10n: vi.po(3195t): Update translation for v2.13.0 round 2

Vasco Almeida (1):
  l10n: pt_PT: update Portuguese translation

 po/bg.po| 6511 +--
 po/ca.po| 4617 +++---
 po/de.po| 4691 ++
 po/fr.po| 4548 ++---
 po/git.pot  | 4387 +---
 po/pt_PT.po | 4528 ++---
 po/sv.po| 4533 ++---
 po/vi.po| 4548 ++---
 po/zh_CN.po | 4569 ++---
 9 files changed, 23745 insertions(+), 19187 deletions(-)

2017-05-09 22:08 GMT+08:00 Jiang Xin :
> Hi Junio,
>
> I can not send email outside at work, but now I am back home. Here is
> the pull request:
>
> The following changes since commit 4fa66c85f11bc5a541462ca5ae3246aa0ce02e74:
>
>   Git 2.13-rc2 (2017-05-04 16:27:19 +0900)
>
> are available in the git repository at:
>
>   git://github.com/git-l10n/git-po tags/l10n-2.13.0-rnd2
>
> for you to fetch changes up to 60638e9816d0aae40d4234d1a068f94fabc2fd4d:
>
>   l10n: zh_CN: for git v2.13.0 l10n round 2 (2017-05-09 21:55:38 +0800)
>
> 
> l10n for Git 2.13.0 round 2
>
> 
> Alexander Shopov (2):
>   l10n: bg.po: Updated Bulgarian translation (3201t)
>   l10n: bg.po: Updated Bulgarian translation (3195t)
>
> Jean-Noel Avila (2):
>   l10n: fr.po v2.13 round 1
>   l10n: fr.po v2.13 rnd 2
>
> Jiang Xin (9):
>   l10n: git.pot: v2.13.0 round 1 (96 new, 37 removed)
>   Merge branch 'master' of https://github.com/vnwildman/git
>   Merge branch 'fr_l10n_v2.13_rnd1' of git://github.com/jnavila/git
>   l10n: zh_CN: for git v2.13.0 l10n round 1
>   Merge branch 'master' of git://github.com/git-l10n/git-po
>   l10n: git.pot: v2.13.0 round 2 (4 new, 7 removed)
>   Merge branch 'fr_l10n_v2.13_rnd2' of git://github.com/jnavila/git
>   Merge branch 'master' of https://github.com/vnwildman/git
>   l10n: zh_CN: for git v2.13.0 l10n round 2
>
> Jordi Mas (1):
>   l10n: Update Catalan translation
>
> Michael J Gruber (1):
>   l10n: de.po: lower case after semi-colon
>
> Peter Krefting (1):
>   l10n: sv.po: Update Swedish translation (3199t0f0u)
>
> Ralf Thielow (2):
>   l10n: de.po: update German translation
>   l10n: de.po: translate 4 new messages
>
> Ray Chen (1):
>   l10n: zh_CN: review for git v2.13.0 l10n round 1
>
> Trần Ngọc Quân (2):
>   l10n: vi.po(3198t): Updated Vietnamese translation for v2.13.0-rc0
>   l10n: vi.po(3195t): Update translation for v2.13.0 round 2
>
> Vasco Almeida (1):
>   l10n: pt_PT: update Portuguese 

[GIT PULL] l10n updates for 2.13.0 round 2

[Jiang Xin]

Hi Junio,

I can not send email outside at work, but now I am back home. Here is
the pull request:

The following changes since commit 4fa66c85f11bc5a541462ca5ae3246aa0ce02e74:

  Git 2.13-rc2 (2017-05-04 16:27:19 +0900)

are available in the git repository at:

  git://github.com/git-l10n/git-po tags/l10n-2.13.0-rnd2

for you to fetch changes up to 60638e9816d0aae40d4234d1a068f94fabc2fd4d:

  l10n: zh_CN: for git v2.13.0 l10n round 2 (2017-05-09 21:55:38 +0800)


l10n for Git 2.13.0 round 2


Alexander Shopov (2):
  l10n: bg.po: Updated Bulgarian translation (3201t)
  l10n: bg.po: Updated Bulgarian translation (3195t)

Jean-Noel Avila (2):
  l10n: fr.po v2.13 round 1
  l10n: fr.po v2.13 rnd 2

Jiang Xin (9):
  l10n: git.pot: v2.13.0 round 1 (96 new, 37 removed)
  Merge branch 'master' of https://github.com/vnwildman/git
  Merge branch 'fr_l10n_v2.13_rnd1' of git://github.com/jnavila/git
  l10n: zh_CN: for git v2.13.0 l10n round 1
  Merge branch 'master' of git://github.com/git-l10n/git-po
  l10n: git.pot: v2.13.0 round 2 (4 new, 7 removed)
  Merge branch 'fr_l10n_v2.13_rnd2' of git://github.com/jnavila/git
  Merge branch 'master' of https://github.com/vnwildman/git
  l10n: zh_CN: for git v2.13.0 l10n round 2

Jordi Mas (1):
  l10n: Update Catalan translation

Michael J Gruber (1):
  l10n: de.po: lower case after semi-colon

Peter Krefting (1):
  l10n: sv.po: Update Swedish translation (3199t0f0u)

Ralf Thielow (2):
  l10n: de.po: update German translation
  l10n: de.po: translate 4 new messages

Ray Chen (1):
  l10n: zh_CN: review for git v2.13.0 l10n round 1

Trần Ngọc Quân (2):
  l10n: vi.po(3198t): Updated Vietnamese translation for v2.13.0-rc0
  l10n: vi.po(3195t): Update translation for v2.13.0 round 2

Vasco Almeida (1):
  l10n: pt_PT: update Portuguese translation

 po/bg.po| 6511 +--
 po/ca.po| 4617 +++---
 po/de.po| 4691 ++
 po/fr.po| 4548 ++---
 po/git.pot  | 4387 +---
 po/pt_PT.po | 4528 ++---
 po/sv.po| 4232 --
 po/vi.po| 4548 ++---
 po/zh_CN.po | 4569 ++---
 9 files changed, 23592 insertions(+), 19039 deletions(-)


2017-05-09 12:57 GMT+08:00 Junio C Hamano :
> Jiang Xin  writes:
>
>> Git v2.13.0-rc2 introduced 4 new messages, let's start round 2 for Git
>> 2.13.0 l10n.
>>
>> You can get it from the usual place:
>>
>> https://github.com/git-l10n/git-po/
>>
>> As how to update your XX.po and help to translate Git, please see
>> "Updating a XX.po file" and other sections in “po/README" file.
>
> What's the doneness of l10n for the upcoming release?  It would be
> preferrable if I can get a "done -- pull from me now" within 16
> hours or so, as I'd like to do the release at around May 10th 00:00
> UTC.
>
> Thanks everybody, as always, for your translations and general
> support of the project.

Re: [PATCH v4 11/25] checkout: fix memory leak

[Johannes Schindelin]

Hi Junio & René,

On Mon, 8 May 2017, Junio C Hamano wrote:

> René Scharfe  writes:
> 
> >>/*
> >> * NEEDSWORK:
> >> * There is absolutely no reason to write this as a blob object
> >> -   * and create a phony cache entry just to leak.  This hack is
> >> -   * primarily to get to the write_entry() machinery that massages
> >> -   * the contents to work-tree format and writes out which only
> >> -   * allows it for a cache entry.  The code in write_entry() needs
> >> -   * to be refactored to allow us to feed a 
> >> -   * instead of a cache entry.  Such a refactoring would help
> >> -   * merge_recursive as well (it also writes the merge result to the
> >> -   * object database even when it may contain conflicts).
> >> +   * and create a phony cache entry.  This hack is primarily to get
> >> +   * to the write_entry() machinery that massages the contents to
> >> +   * work-tree format and writes out which only allows it for a
> >> +   * cache entry.  The code in write_entry() needs to be refactored
> >> +   * to allow us to feed a  instead of a cache
> >> +   * entry.  Such a refactoring would help merge_recursive as well
> >> +   * (it also writes the merge result to the object database even
> >> +   * when it may contain conflicts).
> >> */
> >>if (write_sha1_file(result_buf.ptr, result_buf.size,
> >>blob_type, oid.hash))
> >
> > Random observation: Using pretend_sha1_file here would at least avoid
> > writing the blob.
> 
> Yup, you should have told that to me back in Aug 2008 ;-) when I did
> 0cf8581e ("checkout -m: recreate merge when checking out of unmerged
> index", 2008-08-30); pretend_sha1_file() was available since early
> 2007, and there is no excuse that this codepath did not use it.

I hope y'all agree that this is outside the scope of my patch series...

> >> @@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct 
> >> checkout *state)
> >>if (!ce)
> >>die(_("make_cache_entry failed for path '%s'"), path);
> >>status = checkout_entry(ce, state, NULL);
> >> +  free(ce);
> >>return status;
> >>   }
> >
> > I wonder if that's safe.  Why document a leak when it could have been
> > plugged this easily instead?
> >
> > A leak is better than a use after free, so
> > let's be extra careful here.  Would it leave the index inconsistent?  Or
> > perhaps freeing it has become safe in the meantime?
> >
> > @Junio: Do you remember the reason for the leaks in 0cf8581e330
> > (checkout -m: recreate merge when checking out of unmerged index).
> 
> Yes.
> 
> In the very old days it was not allowed to free(3) contents of
> active_cache[] and this was an old brain fart that came out of
> inertia.  We are manufacturing a brand new ce, only to feed it to
> checkout_entry() without even registering it to the active_cache[]
> array, and the ancient rule doesn't even apply to such a case.
> 
> So I think it was safe to free(3) even back then.

So this patch is good, then?

> > And result_buf is still leaked here, right?
> 
> Good spotting.  It typically would make a larger leak than a single
> ce, I would suppose ;-)

I saw you added this as a fixup! commit. If you don't mind, and if no
other changes are requested, would you mind rebase'ing this yourself?

Thanks,
Dscho

Re: Would this tool be useful - encoding repository data into single URL

[Ævar Arnfjörð Bjarmason]

On Tue, May 9, 2017 at 2:48 PM, Sebastian Gniazdowski
 wrote:
> Hello
> I wonder about usability of following tool. Quick-start:
>
> giturl https://github.com/zdharma/giturl -r devel -p 
> lib/coding_functions.cpp
>
> Protocol: https
> Site: github.com
> Repo: zdharma/giturl
> Revision: devel
> File: lib/coding_functions.cpp
>
> gitu://ŬṽǚǫoŒẗ6ẏȅcЭÑẩőn4ầŘїệαЃȣϟṈӛŀї
>
> It does Huffman encoding and base-1024 encoding to pack given data into 
> single URL. The Unicode characters selected for base-1024 encoding are 
> letters, not symbols, so double-clicking in e.g. web browser selects the 
> whole code, making it easy to grab a repository data.
>
> Decoding:
>
> giturl -qd ŬṽǚǫoŒẗ6ẏȅcЭÑẩőn4ầŘїệαЃȣϟṈӛŀї
> https://github.com/zdharma/giturl / rev:devel / 
> file:lib/coding_functions.cpp
>
> I can also encode commits relative to given revision, e.g. bits 10011 are 
> commits 1, 4, 5. Easy to add to the g-code. Selecting 10th commit is only 1 
> character in base-1024.
>
> However I wonder if this has any uses. Could be patches sent this way? Having 
> refs/patches/, encoding  in URL, sending it instead of 
> inlining/attaching a diff, selecting e.g. 3 commits via the bit-mask 
> mentioned. That said, it's more about easy-grab of repository data and 
> storage in well-defined, consistent format, not in language "the branch is 
> ..., commit a7a35cb". Does this make sense?
>
> There are 2 implementations, in Zsh (uses Zshell like e.g. Ruby, not 
> interactively) and C++11 (mostly because of std::regex):
>
> https://github.com/zdharma/giturl
> https://github.com/zdharma/cgiturl

This looks really cool. Yeah patches can be sent this way, see
Documentation/SubmittingPatches in the git.git repository, online
version: https://github.com/git/git/blob/master/Documentation/SubmittingPatches

Re: [PATCH v3 12/25] split_commit_in_progress(): fix memory leak

[Johannes Schindelin]

Hi René,

On Sat, 6 May 2017, René Scharfe wrote:

> Am 04.05.2017 um 12:59 schrieb Johannes Schindelin:
>
> > diff --git a/wt-status.c b/wt-status.c
> > index 1f3f6bcb980..117ac8cfb01 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -1082,34 +1082,29 @@ static char *read_line_from_git_path(const char
> > *filename)
> >   static int split_commit_in_progress(struct wt_status *s)
> >   {
> > int split_in_progress = 0;
> > -   char *head = read_line_from_git_path("HEAD");
> > -   char *orig_head = read_line_from_git_path("ORIG_HEAD");
> > -   char *rebase_amend = read_line_from_git_path("rebase-merge/amend");
> > -   char *rebase_orig_head = 
> > read_line_from_git_path("rebase-merge/orig-head");
> > -
> > -   if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
> > -   !s->branch || strcmp(s->branch, "HEAD")) {
> > -   free(head);
> > -   free(orig_head);
> > -   free(rebase_amend);
> > -   free(rebase_orig_head);
> > -   return split_in_progress;
> > -   }
> > -
> > -   if (!strcmp(rebase_amend, rebase_orig_head)) {
> > -   if (strcmp(head, rebase_amend))
> > -   split_in_progress = 1;
> > -   } else if (strcmp(orig_head, rebase_orig_head)) {
> > -   split_in_progress = 1;
> > -   }
> > +   char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> > +
> > +   if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
> > +   !s->branch || strcmp(s->branch, "HEAD"))
> > +   return 0;
> >   - if (!s->amend && !s->nowarn && !s->workdir_dirty)
> > -   split_in_progress = 0;
> > +   head = read_line_from_git_path("HEAD");
> > +   orig_head = read_line_from_git_path("ORIG_HEAD");
> > +   rebase_amend = read_line_from_git_path("rebase-merge/amend");
> > +   rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
> 
> Further improvement idea (for a later series): Use rebase_path_amend()
> and rebase_path_orig_head() somehow, to build each path only once.
> 
> Accessing the files HEAD and ORIG_HEAD directly seems odd.
> 
> The second part of the function should probably be moved to sequencer.c.

Sure. On all four accounts.

> > +
> > +   if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
> > +   ; /* fall through, no split in progress */
> 
> You could set split_in_progress to zero here.  Would save a comment
> without losing readability.

But that would confuse e.g. myself, 6 months down the road: why assign
that variable when it already has been assigned? (And we have to assign it
because there is still a code path entering none of these if/else if's
arms).

> > +   else if (!strcmp(rebase_amend, rebase_orig_head))
> > +   split_in_progress = !!strcmp(head, rebase_amend);
> > +   else if (strcmp(orig_head, rebase_orig_head))
> > +   split_in_progress = 1;
> >   
> >free(head);
> >free(orig_head);
> >free(rebase_amend);
> >free(rebase_orig_head);
> > +
> 
> Isn't the patch big enough already without rearranging the else blocks
> and adding that blank line? :)

The else blocks are not really rearranged; apart from the early return,
the rest of the logic has been painstakingly kept in the same order.

Ciao,
Dscho

[noob] is this normal behavior

[Harry Putnam]

Setup: Running openindiana (hipster) a solaris 11 open source branch
   git 2.12.2
   
I've recently moved from mercurial to git mainly because git appears
to receive the most development and the heaviest use.

I'm using git with hold over behavior developed from my time using cvs
long ago.

I keep a hierarchy of directories and files that mirrors the structure
on the OS where git is installed [just where ever tracked files are
located on OS] not the entire structure.

I keep a git repo on the same OS that is created by rsyncing  the
BUFFERED hierarchy into a git repo

Many of the BUFFERED files are symlinked to there places in the OS

Some are not because certain files being symlinked seemed to cause
permissions type problems on the OS.  In those cases they are
periodically copied to the BUFFER hierarchy 

So, not to get too tangled up in explaining my setup... tracked files
change in the buffer area or new files are created there.

Periodically I rsync the files in BUFFER over the files in git repo

And record the changes thru normal git operations

git status
git add (when needed)
git commit

,
| Aside:
| While I would welcome comments or suggestions about using a setup like
| this, that is not the subject of this post.
`

This is my most recent attempt at learning and using git but I have
done a few tries before.

I'm noticing behavior I do not recall seeing in earlier attempts.

rsyncing the buffer over the git repo is now producing a result where
`git status' shows any changed or added files as modified or new but
all need to be added before a commit can take place, not just the new
ones.

Shouldn't files that changed but are already being tracked just show
up as modified and not need adding?

That is, if I have the file ~/.bashrc, which is really a symlink to
BUFFER/OS/export/home/reader/.bashrc

  And is tracked in git repo as REPO/OS/export/home/reader/.bashrc

When;
  BUFFER/OS/export/home/reader/.bashrc

is edited.

Then  rsynced like below:
  BUFFER  /OS/export/home/reader/.bashrc
over
  GITREPO  /OS/export/home/reader.bashrc

Thereby changing the repo file in place

Since that file is already being tracked; shouldn't `git status' show
that file as modified but ready to be committed instead of a file that
is modified but needs to be added before a commit can happen?

Another side of this is that a `git diff FILE' only works before an
`git add .' operation is done.

That is, if I run `git diff FILE' AFTER `git add' .. no diff is
reported, even though it is not committed yet.

So, for example: if I'm committing and in the vi buffer of the commit
and want to see a diff of FILE to aid my log notes.

 git diff FILE will report nothing whatever.

Is that expected behavior?

RE: Add an option to automatically submodule update on checkout

[Randall S. Becker]

On May 8, 2017 10:58 PM, Junio C Hamano wrote:
>"Randall S. Becker"  writes:
>> I have to admit that I just assumed it would have to work that way 
>> this would not be particularly useful. However, in thinking about it, 
>> we might want to limit the depth of how far -b  takes effect. If 
>> the super module brings in submodules entirely within control of the 
>> development group, having -b  apply down to leaf submodules 
>> makes sense (in some policies). However, if some submodules span out 
>> to, say, gnulib, that might not make particular sense.

>I do not see a strong reason to avoid your own branches in "other people's
project" like this.
>The submodule's upstream may be a project you have no control over, but the
repository you have locally is under your total control and you can use >any
branch names to suit the need of your project as the whole (i.e. the
superproject and submodules bound to it).

>The fact that local branch names are under your control and for your own
use is true even when you are not using submodules, by the way.

I agree with the technical aspects of this, but doing a checkout -b into
something like gnulib will pin the code you are using in that submodule to
whatever commit was referenced when you did the checkout. Example: In a
situation like that, I would want gnulib to stay on 'master'. It is my
opinion, FWIW, that this is a matter of policy or standards within the
organization using git that we should not be imposing one way or another. In
the current state of affairs (2.12.x), when I checkout, I make sure that
people are aware of which branch each submodule is on because git won't go
into the submodules - I'm fine with imposing that as a policy at present
because it takes positive action by the developers and I keep the master
branch on my own repositories locked down and it's obvious when they are
accidentally on it. But we're talking changing this so that checkout
branches can apply recursively. This changes the policy requirements so that
people have to further act to undo what git will do by default on recursion.
The policy will be at a high level the same (i.e., always make sure you know
what branch you are on in submodules), but the implementation of it will
need to be different (i.e., after you checkout recursive, go into each
submodule and undo what git just did by checking out the default branch on
some submodules ___ ___ ___, which depends on which super repository they
are using, is onerous for me to manage, and for my developers to remember to
do).

With Respect,
Randall

Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()

[Johannes Schindelin]

Hi Duy,

On Tue, 9 May 2017, Duy Nguyen wrote:

> Sorry for super late reply. I'm slowly catching up.
> 
> On Wed, May 3, 2017 at 10:22 PM, Johannes Schindelin
>  wrote:
> > Hi Duy,
> >
> >
> > On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
> >
> >> There's plenty of error() in this code to safely assume --quiet is not a
> >> concern.
> >>
> >> t5512 needs update because if we check the path 'refs*master' (i.e. the
> >> asterisk is part of the path) then we'll get an EINVAL error.
> >
> > So the first change in this patch unmasks a bug that is fixed by the
> > second patch?
> 
> The change in read_branches_file() in this patch causes the failure.
> See the original report [1],
> 
> [1] http://public-inbox.org/git/3a2686c2-6766-1235-001f-1b5283b5f...@kdbg.org/

I disagree. I do not think that the first part of the patch causes the
failure. I think the failure was always there, we just did not bother to
report (and catch) it properly.

Ciao,
Dscho

[PATCH v3 4/6] t3901: move supporting files into t/t3901/

[Johannes Schindelin]

The current convention is to either generate files on the fly in tests,
or to use supporting files taken from a t/t/ directory (where 
matches the test's number, or the number of the test from which we
borrow supporting files).

The test t3901-i18n-patch.sh was obviously introduced before that
convention was in full swing, hence its supporting files still lived in
t/t3901-8859-1.txt and t/t3901-utf8.txt, respectively.

Let's adjust to the current convention.

Signed-off-by: Johannes Schindelin 
---
 t/t0203-gettext-setlocale-sanity.sh  |  4 ++--
 t/t3901-i18n-patch.sh| 38 
 t/{t3901-8859-1.txt => t3901/8859-1.txt} |  0
 t/{t3901-utf8.txt => t3901/utf8.txt} |  0
 t/t9350-fast-export.sh   |  2 +-
 t/t9500-gitweb-standalone-no-errors.sh   |  4 ++--
 6 files changed, 24 insertions(+), 24 deletions(-)
 rename t/{t3901-8859-1.txt => t3901/8859-1.txt} (100%)
 rename t/{t3901-utf8.txt => t3901/utf8.txt} (100%)

diff --git a/t/t0203-gettext-setlocale-sanity.sh 
b/t/t0203-gettext-setlocale-sanity.sh
index a2124600811..71b0d74b4dd 100755
--- a/t/t0203-gettext-setlocale-sanity.sh
+++ b/t/t0203-gettext-setlocale-sanity.sh
@@ -8,7 +8,7 @@ test_description="The Git C functions aren't broken by 
setlocale(3)"
 . ./lib-gettext.sh
 
 test_expect_success 'git show a ISO-8859-1 commit under C locale' '
-   . "$TEST_DIRECTORY"/t3901-8859-1.txt &&
+   . "$TEST_DIRECTORY"/t3901/8859-1.txt &&
test_commit "iso-c-commit" iso-under-c &&
git show >out 2>err &&
! test -s err &&
@@ -16,7 +16,7 @@ test_expect_success 'git show a ISO-8859-1 commit under C 
locale' '
 '
 
 test_expect_success GETTEXT_LOCALE 'git show a ISO-8859-1 commit under a UTF-8 
locale' '
-   . "$TEST_DIRECTORY"/t3901-8859-1.txt &&
+   . "$TEST_DIRECTORY"/t3901/8859-1.txt &&
test_commit "iso-utf8-commit" iso-under-utf8 &&
LANGUAGE=is LC_ALL="$is_IS_locale" git show >out 2>err &&
! test -s err &&
diff --git a/t/t3901-i18n-patch.sh b/t/t3901-i18n-patch.sh
index f663d567c8a..923eb01f0ea 100755
--- a/t/t3901-i18n-patch.sh
+++ b/t/t3901-i18n-patch.sh
@@ -31,7 +31,7 @@ test_expect_success setup '
 
# use UTF-8 in author and committer name to match the
# i18n.commitencoding settings
-   . "$TEST_DIRECTORY"/t3901-utf8.txt &&
+   . "$TEST_DIRECTORY"/t3901/utf8.txt &&
 
test_tick &&
echo "$GIT_AUTHOR_NAME" >mine &&
@@ -55,7 +55,7 @@ test_expect_success setup '
# the second one on the side branch is ISO-8859-1
git config i18n.commitencoding ISO8859-1 &&
# use author and committer name in ISO-8859-1 to match it.
-   . "$TEST_DIRECTORY"/t3901-8859-1.txt
+   . "$TEST_DIRECTORY"/t3901/8859-1.txt
fi &&
test_tick &&
echo Yet another >theirs &&
@@ -100,7 +100,7 @@ test_expect_success 'rebase (U/U)' '
 
# The result will be committed by GIT_COMMITTER_NAME --
# we want UTF-8 encoded name.
-   . "$TEST_DIRECTORY"/t3901-utf8.txt &&
+   . "$TEST_DIRECTORY"/t3901/utf8.txt &&
git checkout -b test &&
git rebase master &&
 
@@ -110,7 +110,7 @@ test_expect_success 'rebase (U/U)' '
 test_expect_success 'rebase (U/L)' '
git config i18n.commitencoding UTF-8 &&
git config i18n.logoutputencoding ISO8859-1 &&
-   . "$TEST_DIRECTORY"/t3901-utf8.txt &&
+   . "$TEST_DIRECTORY"/t3901/utf8.txt &&
 
git reset --hard side &&
git rebase master &&
@@ -122,7 +122,7 @@ test_expect_success !MINGW 'rebase (L/L)' '
# In this test we want ISO-8859-1 encoded commits as the result
git config i18n.commitencoding ISO8859-1 &&
git config i18n.logoutputencoding ISO8859-1 &&
-   . "$TEST_DIRECTORY"/t3901-8859-1.txt &&
+   . "$TEST_DIRECTORY"/t3901/8859-1.txt &&
 
git reset --hard side &&
git rebase master &&
@@ -135,7 +135,7 @@ test_expect_success !MINGW 'rebase (L/U)' '
# to get ISO-8859-1 results.
git config i18n.commitencoding ISO8859-1 &&
git config i18n.logoutputencoding UTF-8 &&
-   . "$TEST_DIRECTORY"/t3901-8859-1.txt &&
+   . "$TEST_DIRECTORY"/t3901/8859-1.txt &&
 
git reset --hard side &&
git rebase master &&
@@ -148,7 +148,7 @@ test_expect_success 'cherry-pick(U/U)' '
 
git config i18n.commitencoding UTF-8 &&
git config i18n.logoutputencoding UTF-8 &&
-   . "$TEST_DIRECTORY"/t3901-utf8.txt &&
+   . "$TEST_DIRECTORY"/t3901/utf8.txt &&
 
git reset --hard master &&
git cherry-pick side^ &&
@@ -163,7 +163,7 @@ test_expect_success !MINGW 'cherry-pick(L/L)' '
 
git config i18n.commitencoding ISO8859-1 &&
git config i18n.logoutputencoding ISO8859-1 &&
-   . "$TEST_DIRECTORY"/t3901-8859-1.txt &&
+   . "$TEST_DIRECTORY"/t3901/8859-1.txt &&
 
git reset --hard master &&
  

[PATCH v3 3/6] completion: mark bash script as LF-only

[Johannes Schindelin]

Without this change, the completion script does not work, as Bash expects
its scripts to have line feeds as end-of-line markers (this is
particularly prominent in quoted multi-line strings, where carriage
returns would slip into the strings as verbatim characters otherwise).

This change is required to let t9902-completion pass when Git's source
code is checked out with `core.autocrlf = true`.

Signed-off-by: Johannes Schindelin 
Reviewed-by: Jonathan Nieder 
---
 contrib/completion/.gitattributes | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 contrib/completion/.gitattributes

diff --git a/contrib/completion/.gitattributes 
b/contrib/completion/.gitattributes
new file mode 100644
index 000..19116944c15
--- /dev/null
+++ b/contrib/completion/.gitattributes
@@ -0,0 +1 @@
+*.bash eol=lf
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v3 5/6] Fix the remaining tests that failed with core.autocrlf=true

[Johannes Schindelin]

The test suite is mainly developed on Linux and MacOSX, which is the
reason that nobody thought to mark files as LF-only as needed.

The symptom is a test suite that fails left and right when being checked
out using Git for Windows (which defaults to core.autocrlf=true).

Mostly, the problems stem from Git's (LF-only) output being compared to
hard-coded files that are checked out with line endings according to
core.autocrlf (which is of course incorrect). This includes the two test
files in t/diff-lib/, README and COPYING.

This patch can be validated even on Linux by using this cadence:

git config core.autocrlf true
rm .git/index && git stash
make -j15 DEVELOPER=1 test

Signed-off-by: Johannes Schindelin 
Reviewed-by: Jonathan Nieder 
---
 t/.gitattributes | 21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/t/.gitattributes b/t/.gitattributes
index 2d44088f56e..11e5fe37281 100644
--- a/t/.gitattributes
+++ b/t/.gitattributes
@@ -1,2 +1,21 @@
 t[0-9][0-9][0-9][0-9]/* -whitespace
-t0110/url-* binary
+/diff-lib/* eol=lf
+/t0110/url-* binary
+/t3900/*.txt eol=lf
+/t3901/*.txt eol=lf
+/t4034/*/* eol=lf
+/t4013/* eol=lf
+/t4018/* eol=lf
+/t4100/* eol=lf
+/t4101/* eol=lf
+/t4109/* eol=lf
+/t4110/* eol=lf
+/t4135/* eol=lf
+/t4211/* eol=lf
+/t4252/* eol=lf
+/t5100/* eol=lf
+/t5515/* eol=lf
+/t556x_common eol=lf
+/t7500/* eol=lf
+/t8005/*.txt eol=lf
+/t9*/*.dump eol=lf
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v3 6/6] t4051: mark supporting files as requiring LF-only line endings

[Johannes Schindelin]

The test t4051-diff-function-context.sh passes on Linux when
core.autocrlf=true even without marking its support files as LF-only,
but they fail when core.autocrlf=true in Git for Windows' SDK.

The reason is that `grep ... >file.c.new` will keep CR/LF line endings
on Linux (obviously treating CRs as if they were regular characters),
but will be converted to LF-only line endings with MSYS2's grep that is
used in Git for Windows.

As we do not want to validate the way the available `grep` works, let's
just mark the input as LF-only and move on.

Signed-off-by: Johannes Schindelin 
Reviewed-by: Jonathan Nieder 
---
 t/.gitattributes | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/.gitattributes b/t/.gitattributes
index 11e5fe37281..3bd959ae523 100644
--- a/t/.gitattributes
+++ b/t/.gitattributes
@@ -6,6 +6,7 @@ t[0-9][0-9][0-9][0-9]/* -whitespace
 /t4034/*/* eol=lf
 /t4013/* eol=lf
 /t4018/* eol=lf
+/t4051/* eol=lf
 /t4100/* eol=lf
 /t4101/* eol=lf
 /t4109/* eol=lf
-- 
2.12.2.windows.2.800.gede8f145e06

Would this tool be useful - encoding repository data into single URL

[Sebastian Gniazdowski]

Hello
I wonder about usability of following tool. Quick-start:

    giturl https://github.com/zdharma/giturl -r devel -p 
lib/coding_functions.cpp

    Protocol: https
    Site:     github.com
    Repo:     zdharma/giturl
    Revision: devel
    File:     lib/coding_functions.cpp

    gitu://ŬṽǚǫoŒẗ6ẏȅcЭÑẩőn4ầŘїệαЃȣϟṈӛŀї

It does Huffman encoding and base-1024 encoding to pack given data into single 
URL. The Unicode characters selected for base-1024 encoding are letters, not 
symbols, so double-clicking in e.g. web browser selects the whole code, making 
it easy to grab a repository data.

Decoding:

    giturl -qd ŬṽǚǫoŒẗ6ẏȅcЭÑẩőn4ầŘїệαЃȣϟṈӛŀї
    https://github.com/zdharma/giturl / rev:devel / 
file:lib/coding_functions.cpp

I can also encode commits relative to given revision, e.g. bits 10011 are 
commits 1, 4, 5. Easy to add to the g-code. Selecting 10th commit is only 1 
character in base-1024.

However I wonder if this has any uses. Could be patches sent this way? Having 
refs/patches/, encoding  in URL, sending it instead of 
inlining/attaching a diff, selecting e.g. 3 commits via the bit-mask mentioned. 
That said, it's more about easy-grab of repository data and storage in 
well-defined, consistent format, not in language "the branch is ..., commit 
a7a35cb". Does this make sense?

There are 2 implementations, in Zsh (uses Zshell like e.g. Ruby, not 
interactively) and C++11 (mostly because of std::regex):

https://github.com/zdharma/giturl
https://github.com/zdharma/cgiturl

--
Sebastian Gniazdowski
psprint /at/ zdharma.org

[PATCH v3 1/6] Fix build with core.autocrlf=true

[Johannes Schindelin]

On Windows, the default line endings are denoted by a Carriage Return
byte followed by a Line Feed byte, while Linux and MacOSX use a single
Line Feed byte to denote a line ending.

To help with this situation, Git introduced several mechanisms over the
last decade, most prominently the `core.autocrlf` setting.

Sometimes, however, a single setting is incorrect, e.g. when certain
files in the source code are to be consumed by software that can handle
only LF line endings, while other files can use whatever is appropriate
for the current platform.

To allow for that, Git added the `eol` option to its .gitattributes
handling, expecting every user of Git to mark their source code
appropriately.

Bash assumes that line-endings of scripts are denoted by a single Line
Feed byte. Therefore, shell scripts in Git's source code are one example
where that `eol=lf` option is *required*.

When generating common-cmds.h, the Unix tools we use generally operate on
the assumption that input and output deliminate their lines using LF-only
line endings. Consequently, they would happily copy the CR byte verbatim
into the strings in common-cmds.h, which in turn makes the C preprocessor
barf (that interprets them as MacOS-style line endings). Therefore, we
have to mark the input files as LF-only: command-list.txt and
Documentation/git-*.txt.

Quite a bit belatedly, this patch brings Git's own source code in line
with those expectations by setting those attributes to allow for a
correct build even when core.autocrlf=true.

This patch can be validated even on Linux, by using this cadence:

git config core.autocrlf true
rm .git/index && git stash
make -j15 DEVELOPER=1

Signed-off-by: Johannes Schindelin 
Reviewed-by: Jonathan Nieder 
---
 .gitattributes | 8 +++-
 git-gui/.gitattributes | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/.gitattributes b/.gitattributes
index 320e33c327c..8ce9c6b 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -1,3 +1,9 @@
 * whitespace=!indent,trail,space
 *.[ch] whitespace=indent,trail,space diff=cpp
-*.sh whitespace=indent,trail,space
+*.sh whitespace=indent,trail,space eol=lf
+*.perl eol=lf
+*.pm eol=lf
+/Documentation/git-*.txt eol=lf
+/command-list.txt eol=lf
+/GIT-VERSION-GEN eol=lf
+/mergetools/* eol=lf
diff --git a/git-gui/.gitattributes b/git-gui/.gitattributes
index 33d07c06bd9..59cd41dbff7 100644
--- a/git-gui/.gitattributes
+++ b/git-gui/.gitattributes
@@ -2,3 +2,4 @@
 *   encoding=US-ASCII
 git-gui.sh  encoding=UTF-8
 /po/*.poencoding=UTF-8
+/GIT-VERSION-GEN eol=lf
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v3 2/6] git-new-workdir: mark script as LF-only

[Johannes Schindelin]

Bash does not handle scripts with CR/LF line endings correctly, therefore
they *have* to be forced to LF-only line endings.

Funnily enough, this fixes t3000-ls-files-others and
t1021-rerere-in-workdir when git.git was checked out with
core.autocrlf=true, as these test still use git-new-workdir (once `git
worktree` is no longer marked as experimental, both scripts probably
want to be ported to using that command instead).

Signed-off-by: Johannes Schindelin 
Reviewed-by: Jonathan Nieder 
---
 contrib/workdir/.gitattributes | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 contrib/workdir/.gitattributes

diff --git a/contrib/workdir/.gitattributes b/contrib/workdir/.gitattributes
new file mode 100644
index 000..1f78c5d1bd3
--- /dev/null
+++ b/contrib/workdir/.gitattributes
@@ -0,0 +1 @@
+/git-new-workdir eol=lf
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v3 0/6] Abide by our own rules regarding line endings

[Johannes Schindelin]

Over the past decade, there have been a couple of attempts to remedy the
situation regarding line endings (Windows/DOS line endings are
traditionally CR/LF even if many current applications can handle LF,
too, Linux/MacOSX/*BSD/Unix uses LF-only line handlings, and old MacOS
software used to use CR-only line endings).

The current idea seems to be that the default line endings differ
depending on the platform, and for files that should be exempt from that
default, we strongly recommend using .gitattributes to define what the
source code requires.

It is time to heed our own recommendation and mark the files that
require LF-only line endings in our very own .gitattributes.

For starters, those files include shell scripts: the most prevalent
shell interpreter in use (and certainly used in Git for Windows) is
Bash, and Bash does not handle CR/LF line endings gracefully.

There are even two shell scripts that are used in the test suite even if
they are not technically supposed to be part of core Git, as indicated
by their habitat inside contrib/: git-new-workdir and
git-completion.bash.

Related to shell scripts: when generating common-cmds.h, we use tools
that generally operate on the assumption that input and output
deliminate their lines using LF-only line endings. Consequently, they
would happily copy the CR byte verbatim into the strings in
common-cmds.h, which in turn makes the C preprocessor barf (that
interprets them as MacOS-style line endings).

Further, the most obvious required fixes concern tests' support files
committed verbatim, to be compared to Git's output, which is always
LF-only. This includes the two files in t/diff-lib/ (which does not, in
fact contain library functions as suggested by its name, but a copy of
the README and the COPYING files, specifically for use in the tests).

There are a few SVN dump files, too, supporting the Subversion-related
tests, requiring LF-only line endings.

Without these fixes, Git will fail to build and pass the test suite, as
can be verified even on Linux using this cadence:

git config core.autocrlf true
rm .git/index && git stash
make DEVELOPER=1 -j15 test

Note: I separated out the change marking t/t4051/* as LF-only into an
individual commit for one reason: it would appear that some grep builds
on Windows automagically convert CR/LF input into LF-only output. I went
the easy route to work around this issue, as I do not want to bother
changing MSYS2 grep's behavior.

Changes since v2:

- marked t/diff-lib/* as LF-only, dropping the ugly `tr -d "\015"` patch.


Johannes Schindelin (6):
  Fix build with core.autocrlf=true
  git-new-workdir: mark script as LF-only
  completion: mark bash script as LF-only
  t3901: move supporting files into t/t3901/
  Fix the remaining tests that failed with core.autocrlf=true
  t4051: mark supporting files as requiring LF-only line endings

 .gitattributes   |  8 ++-
 contrib/completion/.gitattributes|  1 +
 contrib/workdir/.gitattributes   |  1 +
 git-gui/.gitattributes   |  1 +
 t/.gitattributes | 22 +-
 t/t0203-gettext-setlocale-sanity.sh  |  4 ++--
 t/t3901-i18n-patch.sh| 38 
 t/{t3901-8859-1.txt => t3901/8859-1.txt} |  0
 t/{t3901-utf8.txt => t3901/utf8.txt} |  0
 t/t9350-fast-export.sh   |  2 +-
 t/t9500-gitweb-standalone-no-errors.sh   |  4 ++--
 11 files changed, 55 insertions(+), 26 deletions(-)
 create mode 100644 contrib/completion/.gitattributes
 create mode 100644 contrib/workdir/.gitattributes
 rename t/{t3901-8859-1.txt => t3901/8859-1.txt} (100%)
 rename t/{t3901-utf8.txt => t3901/utf8.txt} (100%)


base-commit: 9b669787fc6ebc527df9ad058c4bcaf46bacc267
Published-As: https://github.com/dscho/git/releases/tag/lf-attrs-v3
Fetch-It-Via: git fetch https://github.com/dscho/git lf-attrs-v3

Interdiff vs v2:

 diff --git a/t/.gitattributes b/t/.gitattributes
 index bdd82cf31f7..3bd959ae523 100644
 --- a/t/.gitattributes
 +++ b/t/.gitattributes
 @@ -1,4 +1,5 @@
  t[0-9][0-9][0-9][0-9]/* -whitespace
 +/diff-lib/* eol=lf
  /t0110/url-* binary
  /t3900/*.txt eol=lf
  /t3901/*.txt eol=lf
 diff --git a/t/t4003-diff-rename-1.sh b/t/t4003-diff-rename-1.sh
 index c3e0a3c3fc9..df2accb6555 100755
 --- a/t/t4003-diff-rename-1.sh
 +++ b/t/t4003-diff-rename-1.sh
 @@ -11,7 +11,7 @@ test_description='More rename detection
  
  test_expect_success \
  'prepare reference tree' \
 -'tr -d "\015" <"$TEST_DIRECTORY"/diff-lib/COPYING >COPYING &&
 +'cat "$TEST_DIRECTORY"/diff-lib/COPYING >COPYING &&
   echo frotz >rezrov &&
  git update-index --add COPYING rezrov &&
  tree=$(git write-tree) &&
 @@ -99,7 +99,7 @@ test_expect_success \
  
  test_expect_success \
  'prepare work tree once again' \
 -'tr -d "\015" <"$TEST_DIRECTORY"/diff-lib/COPYING >COPYING &&
 +'cat "$TEST_DIRECTORY"/diff-lib/COPYING 

Re: [PATCH 0/1] Preserve the untracked cache across checkout, reset --hard, etc

[Ben Peart]

On 5/9/2017 1:02 AM, Junio C Hamano wrote:

David Turner  writes:


Can you actually keep the email address as my Twopensource one?  I want to make 
sure that Twitter, my employer at the time, gets credit for this work (just as 
I want to make sure that my current employer, Two Sigma, gets credit for my 
current work).

Please feel free to add Signed-off-by: David Turner  in 
case that makes tracking easier.

Thanks.

WRT the actual patch, I want to note that past me did not do a
great job here.  The tests do not correctly check that the
post-checkout untracked cache is still valid after a checkout.
For example, let's say that previously, the directory foo was
entirely untracked (but it contained a file bar), but after the
checkout, there is a file foo/baz.  Does the untracked cache need
to get updated?

Unfortunately, the untracked cache is very unlikely to make it to
the top of my priority list any time soon, so I won't be able to
correct this test (and, if necessary, correct the code).  But I
would strongly suggest that the test be improved before this code
is merged.

Thanks for CCing me.

I will try to find time to tweak what was sent to the list here to
reflect your affiliations better, but marked with DONTMERGE waiting
for the necessary updates you mentioned above, so that this change
is not forgotten.  It may turn out to be that copying from src to
dst like the patch does is all that is needed, or the cache may need
further invalidation when the copying happens, and I haven't got a
good feeling that anybody who are familiar with the codepath vetted
the correctness from seeing the discussion from sidelines (yet).

Thanks.


I've been looking into similar issues with the cache associated with 
using a file system monitor (aka Watchman) 
(https://github.com/git-for-windows/git/compare/master...benpeart:fsmonitor) 
to speed updating the index to reflect changes in the working directory.


I can take a look and see if this patch results in valid results and 
reply to the thread or resubmit as needed.


Ben

Re: Script to rebase branches

[Johannes Schindelin]

Hi Peff,

On Tue, 9 May 2017, Jeff King wrote:

> On Tue, May 09, 2017 at 12:50:22PM +0200, Johannes Schindelin wrote:
> 
> > > This is what I use:
> > > 
> > >   https://github.com/peff/git/blob/meta/rebase
> > > 
> > > There's no documentation in the script, but the commit message in its
> > > history should give a good sense of what each part does.
> > 
> > That requires Meta/ to be checked out and up-to-date. I'd bet there are
> > exactly two people who fall into that category.
> 
> Actually, it is not Junio's Meta that needs checked out, but rather the
> "meta" branch where you will find that "rebase" script.

Oh, okay, I misunderstood that part, indeed.

> If other people find them useful, the set of scripts could perhaps be
> transitioned to a namespace that is appropriate to go into people's
> $PATH.
> 
> I didn't really expect anybody to use it verbatim, though. I was
> providing it more for inspiration.

I deem it part of Git's mission is to avoid forcing everybody to write
scripts so specific to their own needs that they cannot be shared easily.

There are lots of parts in the interactive rebase, for example, that I do
not want to use myself. But enough users do to make it worthwhile to
support in Git proper.

Likewise, I assume that most developers work on one topic branch at a
time, until it is submitted as a Pull Request, and then move to the next
topic branch after the previous has been merged. But there are enough
users, I'd wager, including both you and me, who need to work on multiple
topics in parallel (for a plethora of reasons).

Certainly there will be more developers with the need to rebase multiple
branches at once than there are developers needing to send patches via
mail (and we support the latter operation with several top-level Git
commands).

And that makes me believe that there is enough need for multi-branch
operations that we should consider supporting them out of the box.

> > Also, I see that you do not use worktrees. Otherwise your script would
> > fail.
> 
> Yes, the script predates the invention of worktrees by several years. I
> have occasionally played with worktrees, but don't use them extensively
> (I'd usually use them for a one-off change, and then remove the
> worktree).

I assumed as much, and I did not mean to criticise you for it. I was just
pointing out, implicitly, that the script has room for improvement to make
it applicable to a broader audience.

Personally, I am a heavy user of worktrees, and once the vexing bug that
corrupts them by gc'ing in-use objects away.

I kind of use them not only for developing the topic branches (as you
know, my patch series frequently see many iterations and many weeks going
by before they hit home), but also as kind of a Pensive, where I keep
things I want to work on but do not have time right now. Some of my
worktrees are both: they are topic branches in flight, and also remind me
that a certain topic was not yet accepted.

> > When I still hoped to be able to get the rebase--helper related topic
> > branches in by August last year, I had grandiose plans to teach the
> > sequencer not only to perform the Git garden shears' trick (i.e.
> > recreate merges), but also to optionally update local branches
> > corresponding to the merge commits, including updates to the worktrees
> > that checked them out (if any).
> 
> I don't think I need anything that fancy. But simply checking "is this
> checked out in a worktree" for each branch and then doing "cd
> /path/to/worktree && git rebase" instead of just "git rebase $branch"
> would be enough, I think.
> 
> (I'm assuming the problem you see is just that the directory running
> Meta/rebase cannot check out a branch that is checked out in another
> worktree).

I understand that you do not need anything that fancy. And you probably
understand that I was not talking about your needs only, nor mine. I was
talking about a broader audience of power users with the need to work on
multiple branches in parallel, to automate things in order to make work
more fun.

Kinda like Git, you know?

Ciao,
Dscho

Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)

[Ævar Arnfjörð Bjarmason]

On Tue, May 9, 2017 at 12:40 PM, Johannes Schindelin
 wrote:
> Hi,
>
> On Tue, 9 May 2017, brian m. carlson wrote:
>
>> On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> > On Tue, May 9, 2017 at 1:32 AM, brian m. carlson
>> >  wrote:
>> > > PCRE and PCRE2 also tend to have a lot of security updates, so I
>> > > would prefer if we didn't import them into the tree.  It is far
>> > > better for users to use their distro's packages for PCRE, as it
>> > > means they get automatic security updates even if they're using an
>> > > old Git.
>> > >
>> > > We shouldn't consider shipping anything with a remotely frequent
>> > > history of security updates in our tree, since people very
>> > > frequently run old or ancient versions of Git.
>> >
>> > I'm aware of its security record[1], but I wonder what threat model
>> > you have in mind here. I'm not aware of any parts of git (except maybe
>> > gitweb?) where we take regexes from untrusted sources.
>> >
>> > I.e. yes there have been DoS's & even some overflow bugs leading code
>> > execution in PCRE, but in the context of powering git-grep & git-log
>> > with PCRE this falls into the "stop hitting yourself" category.
>>
>> Just because you don't drive Git with untrusted regexes doesn't mean
>> other people don't.
>
> Or other applications.
>
>> It's not a good idea to require a stronger security model than we
>> absolutely have to, since people can and will violate it.  Think how
>> devastating Shellshock was even though technically nobody should provide
>> insecure environment variables to the shell.
>>
>> And, yes, gitweb does in fact call git grep. That means that git grep
>> must in fact be secure against untrusted regexes, or you have a remote
>> code execution vulnerability.
>
> And not only grep is affected. Think HEAD^{/}. There are plenty of
> sites where you are allowed to specify revs in a freer form than SHA-1s.

That will still use reg(comp|exec) for the foreseeable future. We have
plenty of manual use of that all over the place:

$ git grep 'reg(comp|exec)\(' *.[ch] builtin/*.[ch]

And the ^{/rx} feature is powered by the one in sha1_name.c

> Having said that, I do like the prospect of a faster git grep.
>
> Hopefully there will be a way to make use of PCRE that can be switched
> off? Like, a compile-time replacement of the regex API backed by PCRE v2
> *iff* PCRE v2 is used for building?

Yup, see my just-sent

Re: [PATCH v2 7/7] t4051: mark supporting files as requiring LF-only line endings

[Johannes Schindelin]

Hi Junio,

On Mon, 8 May 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > The test t4051-diff-function-context.sh passes on Linux when
> > core.autocrlf=true even without marking its support files as LF-only,
> > but they fail when core.autocrlf=true in Git for Windows' SDK.
> >
> > The reason is that `grep ... >file.c.new` will keep CR/LF line endings
> > on Linux (obviously treating CRs as if they were regular characters),
> > but will be converted to LF-only line endings with MSYS2's grep that
> > is used in Git for Windows.
> 
> Ahem.  
> 
> I thought that according to your claim a UNIX tool like "grep" would
> alway use LF endings?  ;-)

The maintainers of the MSYS2 grep package apparently disagree with me on
that point. You should be familiar with that reaction.

> > As we do not want to validate the way the available `grep` works,
> > let's just mark the input as LF-only and move on.
> 
> I agree with this conclusion; just like we do not want to worry about
> how `grep` works when given CRLF files in this patch, we do not want to
> worry about how other commands like `sh` works when given CRLF files.
> And that is consistent with the overall theme of this series that marked
> *.sh, *.perl and other files with eol=lf attribute.

Good. That agreement is something on which you and I can build.

> The only question I still have with this series is about the
> README/COPYING thing.  I _think_ it was my ancient mistake to use the
> toplevel README and COPYING as test files, and that mistake was
> corrected by somebody else earlier by having a frozen copy in
> t/diff-lib/ and making tests use these files from that directory.  If we
> broke our tests to again use these files from outside t/diff-lib/, then
> we would need to fix the tests not to do so.  And if we are only looking
> at t/diff-lib/ copy, then I think it is more consistent with the rest of
> this series to mark them with eol=lf rather than passing them through
> "tr -d '\015'".

Thank you for pointing out that the README and COPYING files were
reproduced in t/diff-lib/ specifically to serve as files for use in the
tests. It had not really occurred to me, as I mistook this to be an extra
anal licensing clarification for the diff-lib ;-)

I will "re-roll" the patch series, dropping the ugly tr calls and marking
t/diff-lib/* as LF-only, as you suggested.

Ciao,
Dscho

Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)

[Ævar Arnfjörð Bjarmason]

On Tue, May 9, 2017 at 2:37 AM, brian m. carlson
 wrote:
> On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> On Tue, May 9, 2017 at 1:32 AM, brian m. carlson
>>  wrote:
>> > PCRE and PCRE2 also tend to have a lot of security updates, so I would
>> > prefer if we didn't import them into the tree.  It is far better for
>> > users to use their distro's packages for PCRE, as it means they get
>> > automatic security updates even if they're using an old Git.
>> >
>> > We shouldn't consider shipping anything with a remotely frequent history
>> > of security updates in our tree, since people very frequently run old or
>> > ancient versions of Git.
>>
>> I'm aware of its security record[1], but I wonder what threat model
>> you have in mind here. I'm not aware of any parts of git (except maybe
>> gitweb?) where we take regexes from untrusted sources.
>>
>> I.e. yes there have been DoS's & even some overflow bugs leading code
>> execution in PCRE, but in the context of powering git-grep & git-log
>> with PCRE this falls into the "stop hitting yourself" category.
>
> Just because you don't drive Git with untrusted regexes doesn't mean
> other people don't.  It's not a good idea to require a stronger security
> model than we absolutely have to, since people can and will violate it.
> Think how devastating Shellshock was even though technically nobody
> should provide insecure environment variables to the shell.

Yes this is definitely something we should keep in mind. I see my
comment could be read as dismissing your concerns, I didn't mean that.
This is definitely something to be concerned about.

I was trying to solicit feedback on whether there were any other parts
of stock git that could take external input as regexes than gitweb,
I'm not aware of any.

I thought that gitweb wouldn't take regexes by default, but I see now
that the 'search' feature is on by default, and that allows you to
grep / pickaxe with regexes. Either -E or -F for grep, or
--pickaxe-regex (which implies -E) for log.

> And, yes, gitweb does in fact call git grep.  That means that git grep
> must in fact be secure against untrusted regexes, or you have a remote
> code execution vulnerability.

Indeed, but it's not as clear-cut as turning on on PCRE making you vulnerable.

* gitweb is vulnerable to CPU DoS now in its default configuration.
It's easy to provide an ERE that ends up slurping up 100% CPU for
several seconds on any non-trivial sized repo, do that in parallel &
you have a DoS vector.

* Obviously something that's 2x as fast or more (which my WIP code is)
makes this better.

* PCRE tends to be worse at pathological patterns, but this is because
it has really large limits by default and will try rather insanely
hard to match your pattern.

-DHEAP_LIMIT=2000 \
-DMATCH_LIMIT=1000 \
-DMATCH_LIMIT_DEPTH=1000 \
-DMAX_NAME_COUNT=1 \
-DMAX_NAME_SIZE=32 \
-DPARENS_NEST_LIMIT=250 \

This can be reduced dynamically via the API (and the patterns can't
change it, except to reduce it). For example on my system 2.11.0
(before any of my changes) on linux.git:

$ time git grep -E -- '-?-?-?-?-?-?-?-?-?-?-?---$' |wc -l
12434
real0m0.444s
$ time git grep -P -- '-?-?-?-?-?-?-?-?-?-?-?---$' | wc -l
12434
real1m20.849s

With my JIT changes:

$ time ~/g/git/git --exec-path=/home/avar/g/git grep -P --
'-?-?-?-?-?-?-?-?-?-?-?---$' | wc -l
12434
real0m5.334s

However for user-supplied patterns we can just turn on really
conservative settings:

$ time ~/g/git/git --exec-path=/home/avar/g/git grep -P --
'(*LIMIT_RECURSION=1000)(*LIMIT_MATCH=1000)-?-?-?-?-?-?-?-?-?-?-?---$'
| wc -l
fatal: pcre2_jit_match failed with error code -47: match limit exceeded
0
real0m0.021s

When I locally compile git with something like this:

-   -DMATCH_LIMIT=1000 \
-   -DMATCH_LIMIT_DEPTH=1000 \
-   -DMATCH_LIMIT_RECURSION=1000 \
-   -DMAX_NAME_COUNT=1 \
+   -DMATCH_LIMIT=1000 \
+   -DMATCH_LIMIT_DEPTH=1000 \
+   -DMATCH_LIMIT_RECURSION=1000 \
+   -DMAX_NAME_COUNT=100 \
-DMAX_NAME_SIZE=32 \
-   -DPARENS_NEST_LIMIT=250 \
+   -DPARENS_NEST_LIMIT=10 \

All tests still pass, and from my own testing I can't find any
non-pathological patterns this would break. So we might actually
consider turning this down for git by default.

* Much of the rest of the patterns PCRE has CVEs for can similarly be
mitigated by simply turning various features off.

> Furthermore, at work we distribute Git with all releases of our product.
> We normally only do non-security updates to the last couple of releases,
> but we provide security updates to all supported 

Re: Script to rebase branches

[Jeff King]

On Tue, May 09, 2017 at 12:50:22PM +0200, Johannes Schindelin wrote:

> > This is what I use:
> > 
> >   https://github.com/peff/git/blob/meta/rebase
> > 
> > There's no documentation in the script, but the commit message in its
> > history should give a good sense of what each part does.
> 
> That requires Meta/ to be checked out and up-to-date. I'd bet there are
> exactly two people who fall into that category.

Actually, it is not Junio's Meta that needs checked out, but rather the
"meta" branch where you will find that "rebase" script. If other people
find them useful, the set of scripts could perhaps be transitioned to a
namespace that is appropriate to go into people's $PATH.

I didn't really expect anybody to use it verbatim, though. I was
providing it more for inspiration.

> Also, I see that you do not use worktrees. Otherwise your script would
> fail.

Yes, the script predates the invention of worktrees by several years. I
have occasionally played with worktrees, but don't use them extensively
(I'd usually use them for a one-off change, and then remove the
worktree).

> When I still hoped to be able to get the rebase--helper related topic
> branches in by August last year, I had grandiose plans to teach the
> sequencer not only to perform the Git garden shears' trick (i.e. recreate
> merges), but also to optionally update local branches corresponding to the
> merge commits, including updates to the worktrees that checked them out
> (if any).

I don't think I need anything that fancy. But simply checking "is this
checked out in a worktree" for each branch and then doing "cd
/path/to/worktree && git rebase" instead of just "git rebase $branch"
would be enough, I think.

(I'm assuming the problem you see is just that the directory running
Meta/rebase cannot check out a branch that is checked out in another
worktree).

-Peff

Re: Script to rebase branches

[Johannes Schindelin]

Hi Peff,

On Tue, 9 May 2017, Jeff King wrote:

> On Sat, May 06, 2017 at 12:23:32PM +0200, Lars Schneider wrote:
> 
> > I am about to write a bash/sh script that helps me to rebase a bunch of 
> > branches (e.g. select branches based on prefix, conflict resolution/
> > rerere support, ...).
> > 
> > I wonder if anyone has such a script already and is willing to share it.
> 
> This is what I use:
> 
>   https://github.com/peff/git/blob/meta/rebase
> 
> There's no documentation in the script, but the commit message in its
> history should give a good sense of what each part does.

That requires Meta/ to be checked out and up-to-date. I'd bet there are
exactly two people who fall into that category.

Also, I see that you do not use worktrees. Otherwise your script would
fail.

When I still hoped to be able to get the rebase--helper related topic
branches in by August last year, I had grandiose plans to teach the
sequencer not only to perform the Git garden shears' trick (i.e. recreate
merges), but also to optionally update local branches corresponding to the
merge commits, including updates to the worktrees that checked them out
(if any).

Maybe I'll still get around to do this, some time this year. And it'll
probably hit git.git by mid next year ;-)

Ciao,
Dscho

Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)

[Johannes Schindelin]

Hi,

On Tue, 9 May 2017, brian m. carlson wrote:

> On Tue, May 09, 2017 at 02:00:18AM +0200, Ævar Arnfjörð Bjarmason wrote:
> > On Tue, May 9, 2017 at 1:32 AM, brian m. carlson
> >  wrote:
> > > PCRE and PCRE2 also tend to have a lot of security updates, so I
> > > would prefer if we didn't import them into the tree.  It is far
> > > better for users to use their distro's packages for PCRE, as it
> > > means they get automatic security updates even if they're using an
> > > old Git.
> > >
> > > We shouldn't consider shipping anything with a remotely frequent
> > > history of security updates in our tree, since people very
> > > frequently run old or ancient versions of Git.
> > 
> > I'm aware of its security record[1], but I wonder what threat model
> > you have in mind here. I'm not aware of any parts of git (except maybe
> > gitweb?) where we take regexes from untrusted sources.
> > 
> > I.e. yes there have been DoS's & even some overflow bugs leading code
> > execution in PCRE, but in the context of powering git-grep & git-log
> > with PCRE this falls into the "stop hitting yourself" category.
> 
> Just because you don't drive Git with untrusted regexes doesn't mean
> other people don't.

Or other applications.

> It's not a good idea to require a stronger security model than we
> absolutely have to, since people can and will violate it.  Think how
> devastating Shellshock was even though technically nobody should provide
> insecure environment variables to the shell.
> 
> And, yes, gitweb does in fact call git grep. That means that git grep
> must in fact be secure against untrusted regexes, or you have a remote
> code execution vulnerability.

And not only grep is affected. Think HEAD^{/}. There are plenty of
sites where you are allowed to specify revs in a freer form than SHA-1s.

Having said that, I do like the prospect of a faster git grep.

Hopefully there will be a way to make use of PCRE that can be switched
off? Like, a compile-time replacement of the regex API backed by PCRE v2
*iff* PCRE v2 is used for building?

Ciao,
Dscho

Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning

[Johannes Schindelin]

Hi Ramsay,

On Mon, 8 May 2017, Ramsay Jones wrote:

> Commit dddbad728c ("timestamp_t: a new data type for timestamps",
> 26-04-2017) introduced a new typedef 'timestamp_t', as a synonym for an
> unsigned long, which was used at the time to represent timestamps in
> git. A later commit 28f4aee3fb ("use uintmax_t for timestamps",
> 26-04-2017) changed the typedef to use an 'uintmax_t' for the timestamp
> representation type.
> 
> When building on a 32-bit Linux system, sparse complains that a constant
> (USTAR_MAX_MTIME) used to detect a 'far-future mtime' timestamp, is too
> large; 'warning: constant 0777UL is so big it is unsigned long
> long' on lines 335 and 338 of archive-tar.c. Note that both gcc and
> clang only issue a warning if this constant is used in a context that
> requires an 'unsigned long' (rather than an uintmax_t). (Since TIME_MAX
> is no longer equal to 0x, even on a 32-bit system, the macro
> USTAR_MAX_MTIME is set to 0777UL, which cannot be represented as
> an 'unsigned long' constant).
> 
> In order to suppress the warning, change the definition of the macro
> constant USTAR_MAX_MTIME to use an 'ULL' type suffix.
> 
> In a similar vein, on systems which use a 64-bit representation of the
> 'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with
> the value 0777ULL. Although this does not cause any warning
> messages to be issued, it would be more appropriate for this constant
> to use an 'UL' type suffix rather than 'ULL'.

The reason for the current situation is that an earlier fix for
the USTAR_MAX_MTIME constant was applied to the USTAR_MAX_SIZE
constant by mistake.

> Signed-off-by: Ramsay Jones 

With that addition to the commit message: ACK

Ciao,
Dscho

Re: [PATCH v2 00/21]

[Duy Nguyen]

On Sun, May 7, 2017 at 11:20 AM, Junio C Hamano  wrote:
> On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano  wrote:
>>
>> Nguyễn Thái Ngọc Duy   writes:
>>
>> > Changes since v1:
>> >
>> >  - fopen_or_warn() and warn_on_fopen_errors() are introduced. The
>> >latter's name is probably not great...
>> >  - A new patch (first one) to convert a bunch to using xfopen()
>> >  - Fix test failure on Windows, found by Johannes Sixt
>> >  - Fix the memory leak in log.c, found by Jeff
>> >
>> > There are still a couple of fopen() remained, but I'm getting slow
>> > again so let's get these in first and worry about the rest when
>> > somebody has time.
>
> Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?
>
> https://travis-ci.org/git/git/jobs/229585098#L3091
>
> seems to expect an error when test-config is fed a-directory but we are
> not getting the expected warning and error?

Sounds about right. Let me see if defining FREAD_READS_DIRECTORIES on
MacOS X makes travis happy.
-- 
Duy

Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()

[Duy Nguyen]

Sorry for super late reply. I'm slowly catching up.

On Wed, May 3, 2017 at 10:22 PM, Johannes Schindelin
 wrote:
> Hi Duy,
>
>
> On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
>
>> There's plenty of error() in this code to safely assume --quiet is not a
>> concern.
>>
>> t5512 needs update because if we check the path 'refs*master' (i.e. the
>> asterisk is part of the path) then we'll get an EINVAL error.
>
> So the first change in this patch unmasks a bug that is fixed by the
> second patch?

The change in read_branches_file() in this patch causes the failure.
See the original report [1],

[1] http://public-inbox.org/git/3a2686c2-6766-1235-001f-1b5283b5f...@kdbg.org/
-- 
Duy

Re: vger not relaying some of Junio's messages today?

[Johannes Schindelin]

Hi Brandon,

On Mon, 8 May 2017, Brandon Williams wrote:

> On 05/08, Johannes Schindelin wrote:
> 
> > On Sat, 6 May 2017, Ævar Arnfjörð Bjarmason wrote:
> > 
> > > I have one [script] to git am a patch from a msgid, thought I should
> > > write something to handle a series in some DWIM fashion (e.g. apply
> > > the latest continuous sequence of patches matching --author) but
> > > figured that someone's probably wrote this already & I don't need to
> > > hack it up myself...
> > 
> > You probably missed my previous mails mentioning
> > 
> > https://github.com/git-for-windows/build-extra/blob/master/apply-from-public-inbox.sh
> > 
> > You can use this script to apply single patches (identified by their
> > Message-ID), and patch series (identified by their cover letter's
> > Message-ID).
> > 
> > As I mentioned at the Contributors' Summit at GitMerge 2017: I would
> > *love* to collaborate on tools that make any part of the
> > contribution/review process less cumbersome than it is right now.
> 
> Yeah its not the most streamlined process.  I'm sure everyone writes
> their own scripts (like I did) tailored to their workflow.

I am sure you are right. What a waste of time, for everybody to come up
with essentially the same sort of scripts, just to be able to participate.

> For example I just tag a bunch of mails in mutt and then have a scripts
> which 'git am's them on a branch/base of my choosing.  But its specific
> to my workflow so idk how useful it would be to others :(

Hmm. So it looks more and more as if you *have* to use mutt in order to
be rewarded with the option for an efficient workflow.

I'm just so used to my good ole' Alpine. And others may be so used to
their Thunderbird, Outlook, GMail, whatevs.

But hey, maybe the vger woes will eventually become so bad that even mutt
and NNTP users will be affected negatively. At that point, we may look
into alternatives.

Ciao,
Dscho

Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required

[Ævar Arnfjörð Bjarmason]

On Tue, May 9, 2017 at 10:18 AM, Jean-Noël AVILA  wrote:
> Le jeudi 4 mai 2017, 10:14:58 CEST Kerry, Richard a écrit :
>>
>> My point was to ensure that where English is used on-screen it should make 
>> sense, which in this particular case it didn't (a French idiom which, on 
>> using an automatic translator, didn't make sense in English).  The same of 
>> course applies to other languages used on-screen.
>>
>> I agree about ensuring that the application doesn't elicit a response that 
>> it won't, or can't, actually handle.  A rhetorical question is fine, so long 
>> as it is clear that the program won't accept any further input.
>>
>> Though I don't agree about the issue of the length of words, as presented to 
>> a non-native speaker.  Sometimes a longer word can be very specific in its 
>> meaning, and can be looked up in a dictionary if the reader is not familiar 
>> with it.  Sometimes using shorter words can result in a less clear meaning, 
>> or perhaps be an idiomatic usage, which might be missed by a non-native 
>> speaker.
>>
>
> Thanks. So what's the status of this patch series? I don't buy the idea of 
> rhetorical HMI. That's a sure way to confuse non-native speakers. Please note 
> that I kept the questions when there is a following text. Only questions 
> addressing the user at the end of output have been rephrased.
>
> For the "do you mean" questions, the proposition would then simply be: "the 
> most similar command is:" or "the most similar commands are:".
>
> and then  what about the other patches?

When you submit patches you can monitor the next "What's cooking" mail
for the status. See "ja/do-not..." here:
https://public-inbox.org/git/xmqqlgq77pse@gitster.mtv.corp.google.com/

It got picked up for the "pu" branch. You can fetch git.git and see it there.

My feedback on the 3:

* 1/3: Mostly covered above. I did notice after my last comment that
every time gcc wants to suggest you should do something different
(e.g. misspelled variable or macro) it'll say "did you mean?" similar
to what git does now.

While I think this is a rather tragic story of *nix usability ("user
gets asked a question, types yes, gets a few GB/s of y as output") the
main UX problem is surely that the user in question didn't understand
from the terminal output when the program had exited & wasn't
interactive anymore.

But overall this seems like optimizing for a really obscure edge case
at the expense of making the wording more clever. I don't think "did
you mean?" will confuse non-native speakers, as the bug report shows
the user in question has a reasonable command of English, they're
fundimentally confused about how the shell interface works.

* 2/3: Looks great, surprised it took so long for someone to remove
that cutsey but bad message.

* 3/3: I think this partly makes things slightly worse. I.e. now you
get a specific error message about refs being missing, after it shows
you the entire usage info, so you don't know if you e.g. misspelled a
command-line flag or what. I couldn't find any pattern in the existing
shell scripts for "print usage with custom message" thoug.

>> Regards,
>> Richard.
>>
>>
>>
>>
>> Richard Kerry
>> BNCS Engineer, SI SOL Telco & Media Vertical Practice
>> T: +44 (0)20 3618 2669
>> M: +44 (0)7812 325518
>> 4 Triton Square, Regent’s Place, London NW1 3HG
>> richard.ke...@atos.net
>>
>>
>> This e-mail and the documents attached are confidential and intended solely 
>> for the addressee; it may also be privileged. If you receive this e-mail in 
>> error, please notify the sender immediately and destroy it. As its integrity 
>> cannot be secured on the Internet, the Atos group liability cannot be 
>> triggered for the message content. Although the sender endeavours to 
>> maintain a computer virus-free network, the sender does not warrant that 
>> this transmission is virus-free and will not be liable for any damages 
>> resulting from any virus transmitted.
>>
>> 
>> From: Ævar Arnfjörð Bjarmason [ava...@gmail.com]
>> Sent: 04 May 2017 10:09
>> To: Kerry, Richard
>> Cc: git@vger.kernel.org
>> Subject: Re: [PATCH v2 1/3] usability: don't ask questions if no reply is 
>> required
>>
>> On Thu, May 4, 2017 at 10:52 AM, Kerry, Richard  
>> wrote:
>> >
>> > May I suggest that " The most approaching commands" doesn't make much 
>> > sense as English (I don't think a command can "approach").
>> > Perhaps it should be " The most appropriate commands".
>>
>> I had the same concern, saying "appropriate" is IMO also confusing.
>> The point of this UI is not to point out what you should be running,
>> which "appropriate" implies, but just "we couldn't find what you
>> meant, did you mean one of these?".
>>
>> I think nothing needs to change here. The whole premise here is that a
>> program should never ask a question when you can't give an answer, I
>> think that's nonsense. There's such a thing as a rhetorical 

Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required

[Jean-Noël AVILA]

Le jeudi 4 mai 2017, 10:14:58 CEST Kerry, Richard a écrit :
> 
> My point was to ensure that where English is used on-screen it should make 
> sense, which in this particular case it didn't (a French idiom which, on 
> using an automatic translator, didn't make sense in English).  The same of 
> course applies to other languages used on-screen.
> 
> I agree about ensuring that the application doesn't elicit a response that it 
> won't, or can't, actually handle.  A rhetorical question is fine, so long as 
> it is clear that the program won't accept any further input.
> 
> Though I don't agree about the issue of the length of words, as presented to 
> a non-native speaker.  Sometimes a longer word can be very specific in its 
> meaning, and can be looked up in a dictionary if the reader is not familiar 
> with it.  Sometimes using shorter words can result in a less clear meaning, 
> or perhaps be an idiomatic usage, which might be missed by a non-native 
> speaker.
> 

Thanks. So what's the status of this patch series? I don't buy the idea of 
rhetorical HMI. That's a sure way to confuse non-native speakers. Please note 
that I kept the questions when there is a following text. Only questions 
addressing the user at the end of output have been rephrased.

For the "do you mean" questions, the proposition would then simply be: "the 
most similar command is:" or "the most similar commands are:".

and then  what about the other patches?

Thanks


> Regards,
> Richard.
> 
> 
> 
> 
> Richard Kerry
> BNCS Engineer, SI SOL Telco & Media Vertical Practice
> T: +44 (0)20 3618 2669
> M: +44 (0)7812 325518
> 4 Triton Square, Regent’s Place, London NW1 3HG
> richard.ke...@atos.net
> 
> 
> This e-mail and the documents attached are confidential and intended solely 
> for the addressee; it may also be privileged. If you receive this e-mail in 
> error, please notify the sender immediately and destroy it. As its integrity 
> cannot be secured on the Internet, the Atos group liability cannot be 
> triggered for the message content. Although the sender endeavours to maintain 
> a computer virus-free network, the sender does not warrant that this 
> transmission is virus-free and will not be liable for any damages resulting 
> from any virus transmitted.
> 
> 
> From: Ævar Arnfjörð Bjarmason [ava...@gmail.com]
> Sent: 04 May 2017 10:09
> To: Kerry, Richard
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH v2 1/3] usability: don't ask questions if no reply is 
> required
> 
> On Thu, May 4, 2017 at 10:52 AM, Kerry, Richard  
> wrote:
> >
> > May I suggest that " The most approaching commands" doesn't make much sense 
> > as English (I don't think a command can "approach").
> > Perhaps it should be " The most appropriate commands".
> 
> I had the same concern, saying "appropriate" is IMO also confusing.
> The point of this UI is not to point out what you should be running,
> which "appropriate" implies, but just "we couldn't find what you
> meant, did you mean one of these?".
> 
> I think nothing needs to change here. The whole premise here is that a
> program should never ask a question when you can't give an answer, I
> think that's nonsense. There's such a thing as a rhetorical question,
> and sometimes using that form is the most obvious & succinct way to
> put things.
> 
> Which is not to say that phrasing these things as a non-question can't
> be better, but the suggestions so far just seem more complex.
> 
> Also keep in mind that a huge part of the user base for git using the
> English UI consists of non-native speakers, and when in doubt we
> should definitely be picking simpler English like "did you mean?" v.s.
> alternatives with >10 character more obscure words.
> 
> > Richard Kerry
> > BNCS Engineer, SI SOL Telco & Media Vertical Practice
> >
> > T: +44 (0)20 3618 2669
> > M: +44 (0)7812 325518
> > Lync: +44 (0) 20 3618 0778
> > Room G300, Stadium House, Wood Lane, London, W12 7TA
> > richard.ke...@atos.net
> >
> >
> >
> >
> > -Original Message-
> > From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On 
> > Behalf Of Jean-Noel Avila
> > Sent: Wednesday, May 03, 2017 10:07 PM
> > To: git@vger.kernel.org
> > Cc: rashmipa...@gmail.com; Jean-Noel Avila 
> > Subject: [PATCH v2 1/3] usability: don't ask questions if no reply is 
> > required
> >
> > There has been a bug report by a corporate user that stated that "spelling 
> > mistake of stash followed by a yes prints character 'y'
> > infinite times."
> >
> > This analysis was false. When the spelling of a command contains errors, 
> > the git program tries to help the user by providing candidates which are 
> > close to the unexisting command. E.g Git prints the
> > following:
> >
> > git: 'stahs' is not a git command. See 'git --help'.
> > Did you mean this?
> >
> > stash
> >
> > and then exits.
> >
> > The problem with this hint is that it 

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Jeff King]

On Tue, May 09, 2017 at 09:58:28AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Out of curiosity, how do you generate the patch-ids? Is it with
> > something like diff-tree piped to patch-id?
> 
> This:
> 
> my $cmd = qq[git --git-dir="$repository_path" log --since="$since"
> --until="$until" --all --pretty=format:%H --binary | git patch-id];
> open my $patch_id_fh, " $cmd |";

Ah, OK. I was specifically curious whether the decision to respect the
config switch in plumbing would have any impact for your script. But it
wouldn't, as it was already using log (though I suspect the real
protection for your script is that it is used from a vanilla
environment, not by random users).

-Peff

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Ævar Arnfjörð Bjarmason]

On Tue, May 9, 2017 at 5:16 AM, Jeff King  wrote:
> On Mon, May 01, 2017 at 12:34:38PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > I don't know if we would want to be extra paranoid about patch-ids.
>> > There is no helping:
>> >
>> >   git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable
>> >
>> > because diff-tree doesn't know that it's trying for "--stable" output.
>> > But the diffs we compute internally for patch-id could disable the
>> > heuristics. I'm not sure if those matter, though. AFAIK those are used
>> > only for internal comparisons within a single program. I.e., we never
>> > compare them against input from the user, nor do we output them to the
>> > user. So they'll change, but I don't think anybody would care.
>>
>> I have a few-million row table with commit_id as one column & patch_id
>> as another. I.e. a commit -> patch_id mapping.
>
> Thanks for this data point. It's always interesting to hear about
> unforeseen uses of the tools.
>
> Out of curiosity, how do you generate the patch-ids? Is it with
> something like diff-tree piped to patch-id?

This:

my $cmd = qq[git --git-dir="$repository_path" log --since="$since"
--until="$until" --all --pretty=format:%H --binary | git patch-id];
open my $patch_id_fh, " $cmd |";

Which is part of a loop that generates since/until for continuous
pull/insertion. Also, a few lines later there's a workaround for the
git.git bug of patch-id being ^0+$ (fixed in 2485eab55c
("git-patch-id: do not trip over "no newline" markers", 2011-02-17)),
which gives you a sense of how long it's been since anyone's touched
this.

> I do feel a bit sad about breaking this case (or at the very least
> forcing you to set an option to retain cross-version compatibility). But
> my gut says that we don't want to lock ourselves into never changing the
> diff algorithm (and I'm sure we've done it inadvertently a few times
> over the years; even the recent switch to turning on renames would have
> had that impact).

As noted I think it's completely fine to change the patch-ids by
changing the diff algorithm.

I'm about to give some more detail on this in the other thread, but I
find that on our repos the indent heuristic changes the patch-id for
around 2% of patches, which seems fairly typical for non-changelog-y
code. You *then* need to be using topic branches you didn't delete as
well as having authored such a patch for this change to kick in, so
the impact is really minimal.

Even if it somehow changed 100% of the ids that would be fine too. It
would auto-heal as the same git version started reading & inserting
the ids, which are only relevant in a moving window.

Re: [PATCH v2] travis-ci: retry if Git for Windows CI returns HTTP error 502 or 503

[Junio C Hamano]

Lars Schneider  writes:

> The Git for Windows CI web app sometimes returns HTTP errors of
> "502 bad gateway" or "503 service unavailable" [1]. We also need to
> check the HTTP content because the GfW web app seems to pass through
> (error) results from other Azure calls with HTTP code 200.
> Wait a little and retry the request if this happens.
>
> [1] 
> https://docs.microsoft.com/en-in/azure/app-service-web/app-service-web-troubleshoot-http-502-http-503
>
> Signed-off-by: Lars Schneider 
> ---
>
> Hi Junio,
>
> I can't really test this as my TravisCI account does not have the
> extended timeout and I am unable to reproduce the error.
>
> It would be great if we could test this is a little bit in pu.

This has been in 'pu' for a while.  

As the patch simply discards 502 (and others), it is unclear if the
failing test on 'next' is now gone, or the attempt to run 'pu'
happened to be lucky not to get one, from the output we can see in
https://travis-ci.org/git/git/jobs/229867212

Are you comfortable enough to move this forward?  It's not like a
possible breakage in this patch will harm anything (the relaying to
the Windows CI is flaky if the build server cannot deal with the
load anyway), so I would rather have this early in 'next', while we
deal with a few other topics that Windows build is not happy with
that are on 'pu'.