Re: Git Slowness on Windows w/o Internet

[Peter Kostyukov]

Good point. I'll check it out. Thanks for the tip.

Thanks,
Peter
Thanks,

Peter Kostyukov
Senior Systems Engineer
Kohl's Department Stores - KIC
Office: 262-703-6533


On Sat, Nov 3, 2018 at 6:48 PM Philip Oakley  wrote:
>
>
> On 03/11/2018 16:44, brian m. carlson wrote:
> > On Fri, Nov 02, 2018 at 11:10:51AM -0500, Peter Kostyukov wrote:
> >> Wanted to bring to your attention an issue that we discovered on our
> >> Windows Jenkins nodes with git scm installed (git.exe). Our Jenkins
> >> servers don't have Internet access. It appears that git.exe is trying
> >> to connect to various Cloudflare and Akamai CDN instances over the
> >> Internet when it first runs and it keeps trying to connect to these
> >> CDNs every git.exe execution until it makes a successful attempt. See
> >> the screenshot attached with the details.
> >>
> >> Enabling Internet access via proxy fixes the issue and git.exe
> >> continues to work fast on the next attempts to run git.exe
> >>
> >> Is there any configuration setting that can disable this git's
> >> behavior or is there any other workaround without allowing Internet
> >> access? Otherwise, every git command run on a server without the
> >> Internet takes about 30 seconds to complete.
> >
> > Git itself doesn't make any attempt to access those systems unless it's
> > configured to do so (e.g. a remote is set up to talk to those systems
> > and fetch or pull is used).
> >
> > It's possible that you're using a distribution package that performs
> > this behavior, say, to check for updates.  I'd recommend that you
> > contact the distributor, which in this case might be Git for Windows,
> > and see if they can tell you more about what's going on.  The URL for
> > that project is at https://github.com/git-for-windows/git.
> >
>
> The normal Git for Windows install includes an option to check for
> updates at a suitable rate. Maybe you are hitting that. It can be
> switched off.
>
> --
> Philip
CONFIDENTIALITY NOTICE:
This is a transmission from Kohl's Department Stores, Inc.
and may contain information which is confidential and proprietary.
If you are not the addressee, any disclosure, copying or distribution or use of 
the contents of this message is expressly prohibited.
If you have received this transmission in error, please destroy it and notify 
us immediately at 262-703-7000.

CAUTION:
Internet and e-mail communications are Kohl's property and Kohl's reserves the 
right to retrieve and read any message created, sent and received. Kohl's 
reserves the right to monitor messages by authorized Kohl's Associates at any 
time
without any further consent.

Re: Git Slowness on Windows w/o Internet

[Philip Oakley]

On 03/11/2018 16:44, brian m. carlson wrote:

On Fri, Nov 02, 2018 at 11:10:51AM -0500, Peter Kostyukov wrote:

Wanted to bring to your attention an issue that we discovered on our
Windows Jenkins nodes with git scm installed (git.exe). Our Jenkins
servers don't have Internet access. It appears that git.exe is trying
to connect to various Cloudflare and Akamai CDN instances over the
Internet when it first runs and it keeps trying to connect to these
CDNs every git.exe execution until it makes a successful attempt. See
the screenshot attached with the details.

Enabling Internet access via proxy fixes the issue and git.exe
continues to work fast on the next attempts to run git.exe

Is there any configuration setting that can disable this git's
behavior or is there any other workaround without allowing Internet
access? Otherwise, every git command run on a server without the
Internet takes about 30 seconds to complete.


Git itself doesn't make any attempt to access those systems unless it's
configured to do so (e.g. a remote is set up to talk to those systems
and fetch or pull is used).

It's possible that you're using a distribution package that performs
this behavior, say, to check for updates.  I'd recommend that you
contact the distributor, which in this case might be Git for Windows,
and see if they can tell you more about what's going on.  The URL for
that project is at https://github.com/git-for-windows/git.



The normal Git for Windows install includes an option to check for 
updates at a suitable rate. Maybe you are hitting that. It can be 
switched off.


--
Philip

Re: Git Slowness on Windows w/o Internet

[brian m. carlson]

On Fri, Nov 02, 2018 at 11:10:51AM -0500, Peter Kostyukov wrote:
> Wanted to bring to your attention an issue that we discovered on our
> Windows Jenkins nodes with git scm installed (git.exe). Our Jenkins
> servers don't have Internet access. It appears that git.exe is trying
> to connect to various Cloudflare and Akamai CDN instances over the
> Internet when it first runs and it keeps trying to connect to these
> CDNs every git.exe execution until it makes a successful attempt. See
> the screenshot attached with the details.
> 
> Enabling Internet access via proxy fixes the issue and git.exe
> continues to work fast on the next attempts to run git.exe
> 
> Is there any configuration setting that can disable this git's
> behavior or is there any other workaround without allowing Internet
> access? Otherwise, every git command run on a server without the
> Internet takes about 30 seconds to complete.

Git itself doesn't make any attempt to access those systems unless it's
configured to do so (e.g. a remote is set up to talk to those systems
and fetch or pull is used).

It's possible that you're using a distribution package that performs
this behavior, say, to check for updates.  I'd recommend that you
contact the distributor, which in this case might be Git for Windows,
and see if they can tell you more about what's going on.  The URL for
that project is at https://github.com/git-for-windows/git.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Git Slowness on Windows w/o Internet

[Peter Kostyukov]

Hello,

Wanted to bring to your attention an issue that we discovered on our
Windows Jenkins nodes with git scm installed (git.exe). Our Jenkins
servers don't have Internet access. It appears that git.exe is trying
to connect to various Cloudflare and Akamai CDN instances over the
Internet when it first runs and it keeps trying to connect to these
CDNs every git.exe execution until it makes a successful attempt. See
the screenshot attached with the details.

Enabling Internet access via proxy fixes the issue and git.exe
continues to work fast on the next attempts to run git.exe

Is there any configuration setting that can disable this git's
behavior or is there any other workaround without allowing Internet
access? Otherwise, every git command run on a server without the
Internet takes about 30 seconds to complete.

Please advise.

Thanks,
Peter
CONFIDENTIALITY NOTICE:
This is a transmission from Kohl's Department Stores, Inc.
and may contain information which is confidential and proprietary.
If you are not the addressee, any disclosure, copying or distribution or use of 
the contents of this message is expressly prohibited.
If you have received this transmission in error, please destroy it and notify 
us immediately at 262-703-7000.

CAUTION:
Internet and e-mail communications are Kohl's property and Kohl's reserves the 
right to retrieve and read any message created, sent and received. Kohl's 
reserves the right to monitor messages by authorized Kohl's Associates at any 
time
without any further consent.

[PATCH v3 0/2] Fixup for js/mingw-o-append

[Jeff Hostetler via GitGitGadget]

The recent change mingw O_APPEND change breaks writing to named pipes on
Windows. The first commit adds a new test to confirm the breakage and the
second commit fixes the problem. These could be squashed together or we can
just keep the fix and omit the test if that would be better.

d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND

The new mingw_open_append() routine successfully opens the client side of
the named pipe, but the first write() to it fails with EBADF. Adding the
FILE_WRITE_DATA corrects the problem.

 Signed-off-by: Jeff Hostetler jeffh...@microsoft.com
[jeffh...@microsoft.com]

Cc: j6t@kdbg.orgCc: johannes.schinde...@gmx.decc: gitster@pobox.comCc: 
p...@peff.net

Jeff Hostetler (2):
  t0051: test GIT_TRACE to a windows named pipe
  mingw: fix mingw_open_append to work with named pipes

 Makefile   |  1 +
 compat/mingw.c | 36 +--
 t/helper/test-tool.c   |  3 ++
 t/helper/test-tool.h   |  3 ++
 t/helper/test-windows-named-pipe.c | 72 ++
 t/t0051-windows-named-pipe.sh  | 17 +++
 6 files changed, 129 insertions(+), 3 deletions(-)
 create mode 100644 t/helper/test-windows-named-pipe.c
 create mode 100755 t/t0051-windows-named-pipe.sh


base-commit: d641097589160eb795127d8dbcb14c877c217b60
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-35/jeffhostetler/fixup-mingw-o-append-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/35

Range-diff vs v2:

 1:  ecb30eb47c = 1:  ecb30eb47c t0051: test GIT_TRACE to a windows named pipe
 2:  f0361dd306 ! 2:  5592300ca5 mingw: fix mingw_open_append to work with 
named pipes
 @@ -46,22 +46,20 @@
return fd;
   }
   
 -+#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))
  +/*
  + * Does the pathname map to the local named pipe filesystem?
  + * That is, does it have a "//./pipe/" prefix?
  + */
 -+static int mingw_is_local_named_pipe_path(const char *filename)
 ++static int is_local_named_pipe_path(const char *filename)
  +{
 -+ return (IS_SBS(filename[0]) &&
 -+ IS_SBS(filename[1]) &&
 ++ return (is_dir_sep(filename[0]) &&
 ++ is_dir_sep(filename[1]) &&
  + filename[2] == '.'  &&
 -+ IS_SBS(filename[3]) &&
 ++ is_dir_sep(filename[3]) &&
  + !strncasecmp(filename+4, "pipe", 4) &&
 -+ IS_SBS(filename[8]) &&
 ++ is_dir_sep(filename[8]) &&
  + filename[9]);
  +}
 -+#undef IS_SBS
  +
   int mingw_open (const char *filename, int oflags, ...)
   {
 @@ -71,7 +69,7 @@
filename = "nul";
   
  - if (oflags & O_APPEND)
 -+ if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename))
 ++ if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))
open_fn = mingw_open_append;
else
open_fn = _wopen;

-- 
gitgitgadget

[PATCH v2 0/2] Fixup for js/mingw-o-append

[Jeff Hostetler via GitGitGadget]

The recent change mingw O_APPEND change breaks writing to named pipes on
Windows. The first commit adds a new test to confirm the breakage and the
second commit fixes the problem. These could be squashed together or we can
just keep the fix and omit the test if that would be better.

d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND

The new mingw_open_append() routine successfully opens the client side of
the named pipe, but the first write() to it fails with EBADF. Adding the
FILE_WRITE_DATA corrects the problem.

 Signed-off-by: Jeff Hostetler jeffh...@microsoft.com
[jeffh...@microsoft.com]

Cc: j6t@kdbg.orgCc: johannes.schinde...@gmx.decc: gitster@pobox.comCc: 
p...@peff.net

Jeff Hostetler (2):
  t0051: test GIT_TRACE to a windows named pipe
  mingw: fix mingw_open_append to work with named pipes

 Makefile   |  1 +
 compat/mingw.c | 38 ++--
 t/helper/test-tool.c   |  3 ++
 t/helper/test-tool.h   |  3 ++
 t/helper/test-windows-named-pipe.c | 72 ++
 t/t0051-windows-named-pipe.sh  | 17 +++
 6 files changed, 131 insertions(+), 3 deletions(-)
 create mode 100644 t/helper/test-windows-named-pipe.c
 create mode 100755 t/t0051-windows-named-pipe.sh


base-commit: d641097589160eb795127d8dbcb14c877c217b60
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-35/jeffhostetler/fixup-mingw-o-append-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/35

Range-diff vs v1:

 1:  03453cb521 ! 1:  ecb30eb47c t0051: test GIT_TRACE to a windows named pipe
 @@ -140,7 +140,7 @@
  +
  +. ./test-lib.sh
  +
 -+test_expect_success MINGW 'o_append write to named pipe' '
 ++test_expect_failure MINGW 'o_append write to named pipe' '
  + GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 &&
  + { test-tool windows-named-pipe t0051 >actual 2>&1 & } &&
  + pid=$! &&
 2:  f433937d55 < -:  -- mingw: fix mingw_open_append to work with 
named pipes
 -:  -- > 2:  f0361dd306 mingw: fix mingw_open_append to work with 
named pipes

-- 
gitgitgadget

Re: [PATCH 0/2] Fixup for js/mingw-o-append

[Jeff Hostetler]

GitGitGadget botched the CCs when I submitted this.
Replying here to add them.

Sorry,
Jeff

https://github.com/gitgitgadget/gitgitgadget/issues/35


On 9/7/2018 2:19 PM, Jeff Hostetler via GitGitGadget wrote:

The recent change mingw O_APPEND change breaks writing to named pipes on
Windows. The first commit adds a new test to confirm the breakage and the
second commit fixes the problem. These could be squashed together or we can
just keep the fix and omit the test if that would be better.

d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND

The new mingw_open_append() routine successfully opens the client side of
the named pipe, but the first write() to it fails with EBADF. Adding the
FILE_WRITE_DATA corrects the problem.

  Signed-off-by: Jeff Hostetler jeffh...@microsoft.com
[jeffh...@microsoft.com]

Cc: j6t@kdbg.orgCc: johannes.schinde...@gmx.decc: gitster@pobox.comCc:
p...@peff.net

Jeff Hostetler (2):
   t0051: test GIT_TRACE to a windows named pipe
   mingw: fix mingw_open_append to work with named pipes

  Makefile   |  1 +
  compat/mingw.c |  2 +-
  t/helper/test-tool.c   |  3 ++
  t/helper/test-tool.h   |  3 ++
  t/helper/test-windows-named-pipe.c | 72 ++
  t/t0051-windows-named-pipe.sh  | 17 +++
  6 files changed, 97 insertions(+), 1 deletion(-)
  create mode 100644 t/helper/test-windows-named-pipe.c
  create mode 100755 t/t0051-windows-named-pipe.sh


base-commit: d641097589160eb795127d8dbcb14c877c217b60
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-35/jeffhostetler/fixup-mingw-o-append-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/35

[PATCH 0/2] Fixup for js/mingw-o-append

[Jeff Hostetler via GitGitGadget]

The recent change mingw O_APPEND change breaks writing to named pipes on
Windows. The first commit adds a new test to confirm the breakage and the
second commit fixes the problem. These could be squashed together or we can
just keep the fix and omit the test if that would be better.

d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND

The new mingw_open_append() routine successfully opens the client side of
the named pipe, but the first write() to it fails with EBADF. Adding the
FILE_WRITE_DATA corrects the problem.

 Signed-off-by: Jeff Hostetler jeffh...@microsoft.com
[jeffh...@microsoft.com]

Cc: j6t@kdbg.orgCc: johannes.schinde...@gmx.decc: gitster@pobox.comCc: 
p...@peff.net

Jeff Hostetler (2):
  t0051: test GIT_TRACE to a windows named pipe
  mingw: fix mingw_open_append to work with named pipes

 Makefile   |  1 +
 compat/mingw.c |  2 +-
 t/helper/test-tool.c   |  3 ++
 t/helper/test-tool.h   |  3 ++
 t/helper/test-windows-named-pipe.c | 72 ++
 t/t0051-windows-named-pipe.sh  | 17 +++
 6 files changed, 97 insertions(+), 1 deletion(-)
 create mode 100644 t/helper/test-windows-named-pipe.c
 create mode 100755 t/t0051-windows-named-pipe.sh


base-commit: d641097589160eb795127d8dbcb14c877c217b60
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-35/jeffhostetler/fixup-mingw-o-append-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/35
-- 
gitgitgadget

[PATCH 00/20] unconditional O(1) SHA-1 abbreviation

[Ævar Arnfjörð Bjarmason]

This patch series implements an entirely alternate method of achieving
some of the same ends as the MIDX, using the approach suggested by
Jeff King from the RFC thread back in January[1]. You can now do:

core.abbrev = 20
core.validateAbbrev = false

Or:

core.abbrev = +2
core.validateAbbrev = false

On linux.git `git log --oneline --raw --parents` with 64MB packs gives
this improvement with core.abbrev=15 & core.validateAbbrev=false
(v.s. true):

TestHEAD~   HEAD


0014.1: git log --oneline --raw --parents   95.68(95.07+0.53)   
42.74(42.33+0.39) -55.3%

While cleaning up the RFC version of this which I sent in [2] I
discovered that almost none of the existing functionality was tested
for, and was very inconsistent since we have 4 different places where
the abbrev config is parsed.

See 19/20 and 20/20 for what this whole thing is building towards, the
rest is all tests, cleanup, and preparatory work.

(There's still other long-standing issues with the existing behavior
which this doesn't change, but I had to stop somewhere to make this
digestible).

1. https://public-inbox.org/git/20180108102029.ga21...@sigill.intra.peff.net/
2. https://public-inbox.org/git/20180606102719.27145-1-ava...@gmail.com/

Ævar Arnfjörð Bjarmason (20):
  t/README: clarify the description of test_line_count
  test library: add a test_byte_count
  blame doc: explicitly note how --abbrev=40 gives 39 chars
  abbrev tests: add tests for core.abbrev and --abbrev
  abbrev tests: test "git-blame" behavior
  blame: fix a bug, core.abbrev should work like --abbrev
  abbrev tests: test "git branch" behavior
  abbrev tests: test for "git-describe" behavior
  abbrev tests: test for "git-log" behavior
  abbrev tests: test for "git-diff" behavior
  abbrev tests: test for plumbing behavior
  abbrev tests: test for --abbrev and core.abbrev=[+-]N
  parse-options-cb.c: convert uses of 40 to GIT_SHA1_HEXSZ
  config.c: use braces on multiple conditional arms
  parse-options-cb.c: use braces on multiple conditional arms
  abbrev: unify the handling of non-numeric values
  abbrev: unify the handling of empty values
  abbrev parsing: use braces on multiple conditional arms
  abbrev: support relative abbrev values
  abbrev: add a core.validateAbbrev setting

 Documentation/config.txt|  49 +++
 Documentation/diff-options.txt  |   3 +
 Documentation/git-blame.txt |  14 +
 Documentation/git-branch.txt|   3 +
 Documentation/git-describe.txt  |   3 +
 Documentation/git-ls-files.txt  |   3 +
 Documentation/git-ls-tree.txt   |   3 +
 Documentation/git-show-ref.txt  |   3 +
 builtin/blame.c |   2 +
 cache.h |   2 +
 config.c|  22 +-
 diff.c  |  24 +-
 environment.c   |   2 +
 parse-options-cb.c  |  19 +-
 revision.c  |  24 +-
 sha1-name.c |  15 +
 t/README|   6 +-
 t/perf/p0014-abbrev.sh  |  13 +
 t/t0014-abbrev.sh   | 452 
 t/t1512-rev-parse-disambiguation.sh |  47 +++
 t/t6006-rev-list-format.sh  |   6 +-
 t/test-lib-functions.sh |  23 ++
 22 files changed, 722 insertions(+), 16 deletions(-)
 create mode 100755 t/perf/p0014-abbrev.sh
 create mode 100755 t/t0014-abbrev.sh

-- 
2.17.0.290.gded63e768a

[RFC PATCH 0/2] unconditional O(1) SHA-1 abbreviation

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 06 2018, Ævar Arnfjörð Bjarmason wrote:

> On Mon, Jan 08 2018, Derrick Stolee wrote:
>
>> On 1/7/2018 5:42 PM, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> On Sun, Jan 07 2018, Derrick Stolee jotted:
>>>
  git log --oneline --raw --parents

 Num Packs | Before MIDX | After MIDX |  Rel % | 1 pack %
 --+-+++--
  1 | 35.64 s |35.28 s |  -1.0% |   -1.0%
 24 | 90.81 s |40.06 s | -55.9% |  +12.4%
127 |257.97 s |42.25 s | -83.6% |  +18.6%

 The last column is the relative difference between the MIDX-enabled repo
 and the single-pack repo. The goal of the MIDX feature is to present the
 ODB as if it was fully repacked, so there is still room for improvement.

 Changing the command to

  git log --oneline --raw --parents --abbrev=40

 has no observable difference (sub 1% change in all cases). This is likely
 due to the repack I used putting commits and trees in a small number of
 packfiles so the MRU cache workes very well. On more naturally-created
 lists of packfiles, there can be up to 20% improvement on this command.

 We are using a version of this patch with an upcoming release of GVFS.
 This feature is particularly important in that space since GVFS performs
 a "prefetch" step that downloads a pack of commits and trees on a daily
 basis. These packfiles are placed in an alternate that is shared by all
 enlistments. Some users have 150+ packfiles and the MRU misses and
 abbreviation computations are significant. Now, GVFS manages the MIDX file
 after adding new prefetch packfiles using the following command:

  git midx --write --update-head --delete-expired --pack-dir=
>>>
>>> (Not a critique of this, just a (stupid) question)
>>>
>>> What's the practical use-case for this feature? Since it doesn't help
>>> with --abbrev=40 the speedup is all in the part that ensures we don't
>>> show an ambiguous SHA-1.
>>
>> The point of including the --abbrev=40 is to point out that object
>> lookups do not get slower with the MIDX feature. Using these "git log"
>> options is a good way to balance object lookups and abbreviations with
>> object parsing and diff machinery.[...]
>
> [snip]
>
>> [...]And while the public data shape I shared did not show a
>> difference, our private testing of the Windows repository did show a
>> valuable improvement when isolating to object lookups and ignoring
>> abbreviation calculations.
>
> Replying to this old thread since I see you're prepearing the MIDX for
> submission again and this seemed like the best venue.
>
> Your WIP branch (github.com/git/derrickstolee/midx/upstream) still only
> references the speedups in abbreviation calculations, but here you
> allude to other improvements. It would be very nice to have some summary
> of that in docs / commit messages when you submit this.
>
> I've been meaning to get around to submitting something like I mentioned
> in https://public-inbox.org/git/87efn0bkls@evledraar.gmail.com/
> i.e. a way to expand the abbrev mode to not check disambiguations, which
> would look something like:
>
> core.abbrev = 20
> core.validateAbbrev = false
>
> Or:
>
> core.abbrev = +2
> core.validateAbbrev = false
>
> So, using the example from the above referenced E-Mail +2 would make
> linux.git emit hashes of 14 characters, without any abbreviation
> checking (just trusting in statistics to work in your favor).
>
> As noted by JS in this thread that wouldn't be acceptable for your
> use-case, but there's plenty of people (including me) who'd appreciate
> the speedup without being a 100% sure we're emitting unambiguous hashes,
> since that trade-off is better than time spent generating another index
> on-disk. So I see it as a complimentary & orthogonal feature.
>
> But with that implemented I wouldn't get any benefit from things that
> use the MIDX that aren't abbreviations, so what are those?

I won't have time to finish this today, but it's already in a shape
that I think is useful for discussion to see what others think.

I still need to make this be supported by --abbrev=* and have
e.g. --abbrev=+2 work. I got as far as this with that:

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 0f9f311a7a..7cc9d3dfe6 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -16,13 +16,23 @@ int parse_opt_abbrev_cb(const struct option *opt, const 
char *arg, int unset)
if (!arg) {
v = unset ? 0 : DEFAULT_ABBREV;
} else {
+   const char *origarg = arg;
v = strtol(arg, (char **), 10);
if (*arg)
return opterror(opt, "expects a numerical value", 0);
-   if (v && v < MINIMUM_ABBREV)
+   if (*origarg == '+' || *origarg == '-') {
+   if (v == 0) {

[PATCH v2 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Taylor Blau]

Teach GNU grep(1)'s '-o' ('--only-matching') to 'git-grep'. This option
prints only the matching components of each line. It writes multiple
lines if more than one match exists on a given line.

For example:

  $ /git grep -on --column git -- README.md | head -3
  README.md:15:56:git
  README.md:18:20:git
  README.md:19:16:git

By using show_line_header(), 'git grep --only-matching' correctly
respects the '--heading' option:

  $ git grep -on --column --heading git -- README.md | head -4
  README.md
  15:56:git
  18:20:git
  19:16:git

We mirror GNU grep's behavior when given -A, -B, or -C with
--only-matching, by displaying only the matching portion(s) of a line,
ignoring contextual line(s), but displaying '--' (context separator)
line(s).

Notably: when show_line() is called on a line that contains _multiple_
matches, we keep track of a relative offset from the beginning of the
line and increment 'cno' in subsequent calls to show_line_header() when
the expression is not extended. In the extended case, we do not do this,
because the column of the first match is undefined, thus relative
offsets are meaningless.

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-grep.txt |  6 +++-
 builtin/grep.c |  1 +
 grep.c | 34 +--
 grep.h |  1 +
 t/t7810-grep.sh| 69 ++
 5 files changed, 107 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index c48a578cb1..5c09abec4a 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -20,7 +20,7 @@ SYNOPSIS
   [-c | --count] [--all-match] [-q | --quiet]
   [--max-depth ]
   [--color[=] | --no-color]
-  [--break] [--heading] [-p | --show-function]
+  [--break] [--heading] [-o | --only-matching] [-p | --show-function]
   [-A ] [-B ] [-C ]
   [-W | --function-context]
   [--threads ]
@@ -223,6 +223,10 @@ providing this option will cause it to die.
Show the filename above the matches in that file instead of
at the start of each shown line.
 
+--o::
+--only-matching::
+   Prints only the matching part of the lines.
+
 -p::
 --show-function::
Show the preceding line that contains the function name of
diff --git a/builtin/grep.c b/builtin/grep.c
index f9f516dfc4..0507ac335a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -851,6 +851,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
N_("print empty line between matches from different 
files")),
OPT_BOOL(0, "heading", ,
N_("show filename only once above matches from same 
file")),
+   OPT_BOOL('o', "only-matching", _matching, N_("show 
only matches")),
OPT_GROUP(""),
OPT_CALLBACK('C', "context", , N_("n"),
N_("show  context lines before and after matches"),
diff --git a/grep.c b/grep.c
index 36bf7cf08d..9297fde643 100644
--- a/grep.c
+++ b/grep.c
@@ -1422,12 +1422,24 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
opt->output(opt, "\n", 1);
}
}
+   if (opt->only_matching && sign != ':') {
+   /*
+* If we're given '--only-matching' and the line is a contextual
+* one (i.e., we're given '-A', '-B', or '-C'), mark the line as
+* shown (to advance opt->last_shown), but do not show it (since
+* we are given '--only-matching').
+*/
+   opt->last_shown = lno;
+   return;
+   }
show_line_header(opt, name, lno, cno, sign);
-   if (opt->color) {
+   if (opt->color || opt->only_matching) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
int ch = *eol;
int eflags = 0;
+   int first = 1;
+   int offset = 1;
 
if (sign == ':')
match_color = opt->color_match_selected;
@@ -1444,16 +1456,32 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
if (match.rm_so == match.rm_eo)
break;
 
-   output_color(opt, bol, match.rm_so, line_color);
+   if (!opt->only_matching) {
+   output_color(opt, bol, match.rm_so, line_color);
+   } else if (!first) {
+   /*
+* We are given --only-matching, and this is not
+* the first match

[PATCH v2 0/2] builtin/grep.c: teach '-o', '--only-matching'

[Taylor Blau]

Hi,

Attached is the second re-roll of my series to add GNU grep's
'--only-matching' to git-grep.

The main thing that has changed since last time is our handling of
-{A,B,C}. Previously, as Peff points out in [1], we handle this in a
buggy way different than GNU.

I agree that although 'git grep -C -o ...' is an unusual invocation,
it is useful to (1) maintain as much consistency as reasonably makes
sense, and (2) to at least not be buggy.

I have also responded to Eric's suggestions in [2], and [3].

Thanks as always for your kind review :-).


Thanks,
Taylor

[1]: https://public-inbox.org/git/20180510064014.ga31...@sigill.intra.peff.net
[2]: 
https://public-inbox.org/git/capig+csrjww4-7vj6wk8aofnb20bqucsooysjdpci1r5vb8...@mail.gmail.com
[3]: 
https://public-inbox.org/git/capig+crbbz+qtqgiw_wq9e-groa-wtevp1vcrqmj5yqj8ty...@mail.gmail.com

Taylor Blau (2):
  grep.c: extract show_line_header()
  builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

 Documentation/git-grep.txt |  6 ++-
 builtin/grep.c |  1 +
 grep.c | 78 +++---
 grep.h |  1 +
 t/t7810-grep.sh| 69 +
 5 files changed, 132 insertions(+), 23 deletions(-)

--
2.17.0

Re: [PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Jeff King]

On Wed, May 09, 2018 at 07:00:10PM -0700, Taylor Blau wrote:

> >  - they test with context (-C3) for us. It looks like GNU grep omits
> >context lines with "-o", but we show a bunch of blank lines. This is
> >I guess a bug (though it seems kind of an odd combination to specify
> >in the first place)
> 
> I'm torn on what to do here. Currently, with -C3, I get a bunch of lines
> like:
> 
>   -
> 
> Which I think is technically _right_, but I agree that it is certainly
> an odd combination of things to give to 'git grep'. I think that we
> could either:
> 
>   1. Continue outputting blank lines,
>   2. Ignore '-C' with '-o', or
>   3. die() when given this combination.
> 
> I think that (1) is the most "correct" (for some definition), so I'm
> inclined to adopt that approach. I suppose that (2) is closer to what
> GNU grep offers, and that is the point of this series, so perhaps it
> makes sense to pick that instead.
> 
> I'll go with (2) for now, but I would appreciate others' thoughts before
> sending a subsequent re-roll of this series.

We talked about this off-list, so I want to summarize here for the
archive.

It turns out that the GNU grep behavior isn't universal.  Here's what I
get:

  $ grep -o -C3 the README.md
  the
  the
  the
  the
  the
  the
  the
  the
  --
  the
  the
  the
  the
  the
  the
  the
  the
  the
  the
  the
  the
  the
  the

So that's not _quite_ ignoring -C. It's still showing the "--", which
indicates that the first chunk are all matches within 6 lines of each
other (so their context is melded into a single hunk), but it omits the
lines entirely. Which at least seems like it could be _plausibly_
useful.

BSD grep seems to actually show the context lines. Which is more
information, but if you want to actually see the context, why are you
using "-o" in the first place?

So my general feeling is that disallowing the combination is probably
fine, because it's a vaguely crazy thing to ask for. But that GNU grep's
behavior is the most likely to actually be useful. The BSD behavior
seems more like "this is what we happen to produce" to me.

And it should be pretty easy to reproduce the GNU grep behavior by just
not outputting anything in show_line().

-Peff

Re: [PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Taylor Blau]

On Tue, May 08, 2018 at 01:25:17PM -0400, Jeff King wrote:
> On Sat, May 05, 2018 at 08:49:43AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> > > +test_expect_success 'grep --only-matching --heading' '
> > > + git grep --only-matching --heading --line-number --column mmap file 
> > > >actual &&
> > > + test_cmp expected actual
> > > +'
> > > +
> > >  cat >expected < > >  <BOLD;GREEN>hello.c
> > >  4:int main(int argc, const <BLACK;BYELLOW>char **argv)
> >
> > We should test this a lot more, I think a good way to do that would be
> > to extend this series by first importing GNU grep's -o tests, see
> > http://git.savannah.gnu.org/cgit/grep.git/tree/tests/foad1 they are
> > license-compatible. Then change the grep_test() function to call git
> > grep instead.
>
> I'm trying to figure out what GNU grep's tests are actually checking
> that we don't have. I see:
>
>  - they check that "-i" returns the actual found string in its original
>case. This seems like a subset of finding a non-trivial regex. I.e.,
>"foo.*" should find "foobar". We probably should have a test like
>that.
>
>  - they test multiple hits on the same line, which seems like an
>important and easy-to-screw-up case; we do that, too.

Agree.

>  - they test everything other thing with and without "-i" because those
>are apparently separate code paths in GNU grep, but I don't think
>that applies to us.
>
>  - they test each case with "-b", but we don't have that (we do test
>with "--column", which is good)

Right, I think that we can ignore these groups.

>  - they test with "-n", which we do here (we don't test without, but
>that seems like an unlikely bug, knowing how it is implemented)

Fair, let's leave that as is.

>  - they test with -H, but that is already the default for git-grep

Good, we can ignore this one.

>  - they test with context (-C3) for us. It looks like GNU grep omits
>context lines with "-o", but we show a bunch of blank lines. This is
>I guess a bug (though it seems kind of an odd combination to specify
>in the first place)

I'm torn on what to do here. Currently, with -C3, I get a bunch of lines
like:

  -

Which I think is technically _right_, but I agree that it is certainly
an odd combination of things to give to 'git grep'. I think that we
could either:

  1. Continue outputting blank lines,
  2. Ignore '-C' with '-o', or
  3. die() when given this combination.

I think that (1) is the most "correct" (for some definition), so I'm
inclined to adopt that approach. I suppose that (2) is closer to what
GNU grep offers, and that is the point of this series, so perhaps it
makes sense to pick that instead.

I'll go with (2) for now, but I would appreciate others' thoughts before
sending a subsequent re-roll of this series.

> So I think it probably makes sense to just pick through the list I just
> wrote and write our own tests than to try to import GNU grep's specific
> tests (and there's a ton of other unrelated tests in that file that may
> or may not even run with git-grep).

I agree. Since some of these cases are already covered, and some don't
have analogues, I think that it is most sensible to go through the above
and add _those_, instead of porting the whole test suite over from GNU.

> > It should also be tested with the various grep.patternType options to
> > make sure it works with basic, extended, perl, fixed etc.
>
> Hmm, this code is all external to the actual matching. So unless one of
> those is totally screwing up the regmatch_t return, I'm not sure that's
> accomplishing much (and if one of them is, it's totally broken for
> coloring, "-c", and maybe other features).
>
> We've usually taken a pretty white-box approach to our testing, covering
> things that seem likely to go wrong given the general problem space and
> our implementation. But maybe I'm missing something in particular that
> you think might be tricky.
>
> -Peff

Thanks,
Taylor

Re: [PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Jeff King]

On Sat, May 05, 2018 at 08:49:43AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > +test_expect_success 'grep --only-matching --heading' '
> > +   git grep --only-matching --heading --line-number --column mmap file 
> > >actual &&
> > +   test_cmp expected actual
> > +'
> > +
> >  cat >expected < >  <BOLD;GREEN>hello.c
> >  4:int main(int argc, const <BLACK;BYELLOW>char **argv)
> 
> We should test this a lot more, I think a good way to do that would be
> to extend this series by first importing GNU grep's -o tests, see
> http://git.savannah.gnu.org/cgit/grep.git/tree/tests/foad1 they are
> license-compatible. Then change the grep_test() function to call git
> grep instead.

I'm trying to figure out what GNU grep's tests are actually checking
that we don't have. I see:

 - they check that "-i" returns the actual found string in its original
   case. This seems like a subset of finding a non-trivial regex. I.e.,
   "foo.*" should find "foobar". We probably should have a test like
   that.

 - they test multiple hits on the same line, which seems like an
   important and easy-to-screw-up case; we do that, too.

 - they test everything other thing with and without "-i" because those
   are apparently separate code paths in GNU grep, but I don't think
   that applies to us.

 - they test each case with "-b", but we don't have that (we do test
   with "--column", which is good)

 - they test with "-n", which we do here (we don't test without, but
   that seems like an unlikely bug, knowing how it is implemented)

 - they test with -H, but that is already the default for git-grep

 - they test with context (-C3) for us. It looks like GNU grep omits
   context lines with "-o", but we show a bunch of blank lines. This is
   I guess a bug (though it seems kind of an odd combination to specify
   in the first place)

So I think it probably makes sense to just pick through the list I just
wrote and write our own tests than to try to import GNU grep's specific
tests (and there's a ton of other unrelated tests in that file that may
or may not even run with git-grep).

> It should also be tested with the various grep.patternType options to
> make sure it works with basic, extended, perl, fixed etc.

Hmm, this code is all external to the actual matching. So unless one of
those is totally screwing up the regmatch_t return, I'm not sure that's
accomplishing much (and if one of them is, it's totally broken for
coloring, "-c", and maybe other features).

We've usually taken a pretty white-box approach to our testing, covering
things that seem likely to go wrong given the general problem space and
our implementation. But maybe I'm missing something in particular that
you think might be tricky.

-Peff

Re: [PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Taylor Blau]

On Sat, May 05, 2018 at 03:36:12AM -0400, Eric Sunshine wrote:
> On Sat, May 5, 2018 at 12:03 AM, Taylor Blau <m...@ttaylorr.com> wrote:
> > Teach GNU grep(1)'s '-o' ('--only-matching') to 'git-grep'. This option
> > prints only the matching components of each line. It writes multiple
> > lines if more than one match exists on a given line.
> >
> > For example:
> >
> >   $ git grep -on --column --heading git -- README.md | head -3
> >   README.md
> >   15:56:git
> >   18:20:git
> >
> > By using show_line_header(), 'git grep --only-matching' correctly
> > respects the '--header' option:
>
> What is the '--header' option? I don't see it used in any example.

I think '--header' is a typo for '--heading', which is used in the
following example.

> >   $ git grep -on --column --heading git -- README.md | head -4
> >   README.md
> >   15:56:git
> >   18:20:git
> >   19:16:git
>
> How does this example differ from the earlier example (other than
> showing 4 lines of output rather than 3)?

Ack. I clipped from my terminal what I meant to be the seocnd
example, and pasted it in for both examples. They are meant to be as
follows:

  1. 'git grep' without heading, showing the full line prefix, and
  2. 'git grep' with heading, showing the file heading with '--heading'.

The later has '| head -n4' on the end to include 3+1 lines (3 matches, 1
heading) whereas the former has '| head -n3' to include 3 lines (3
matches, no heading).

I have updated my patch locally to reflect this.


Thanks,
Taylor

Re: [PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Eric Sunshine]

On Sat, May 5, 2018 at 12:03 AM, Taylor Blau <m...@ttaylorr.com> wrote:
> Teach GNU grep(1)'s '-o' ('--only-matching') to 'git-grep'. This option
> prints only the matching components of each line. It writes multiple
> lines if more than one match exists on a given line.
>
> For example:
>
>   $ git grep -on --column --heading git -- README.md | head -3
>   README.md
>   15:56:git
>   18:20:git
>
> By using show_line_header(), 'git grep --only-matching' correctly
> respects the '--header' option:

What is the '--header' option? I don't see it used in any example.

>   $ git grep -on --column --heading git -- README.md | head -4
>   README.md
>   15:56:git
>   18:20:git
>   19:16:git

How does this example differ from the earlier example (other than
showing 4 lines of output rather than 3)?

> Signed-off-by: Taylor Blau <m...@ttaylorr.com>

Re: [PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Ævar Arnfjörð Bjarmason]

On Sat, May 05 2018, Taylor Blau wrote:

> +--o::
> +--only-matching::
> + Show only the matching part of the lines.
> +

Makes sense to steal GNU grep's description here:

Print only the matched (non-empty) parts of a matching line, with
each such part on a separate output line.

> + if (!opt->only_matching)
> + output_color(opt, bol, match.rm_so, line_color);

This should also have braces, see "When there are multiple arms to a
conditional" in Documentation/CodingGuidelines.


>  '
>
> +cat >expected < +file:1:5:mmap
> +file:2:5:mmap
> +file:3:5:mmap
> +file:3:14:mmap
> +file:4:5:mmap
> +file:4:14:mmap
> +file:5:5:mmap
> +file:5:14:mmap
> +EOF

This should be set up as part of the test itself, see e.g. my c8b2cec09e
("branch: add test for -m renaming multiple config sections",
2017-06-18) for how to do that.

> +test_expect_success 'grep --only-matching' '
> + git grep --only-matching --line-number --column mmap file >actual &&
> + test_cmp expected actual
> +'
> +
> +cat >expected < +file
> +1:5:mmap
> +2:5:mmap
> +3:5:mmap
> +3:14:mmap
> +4:5:mmap
> +4:14:mmap
> +5:5:mmap
> +5:14:mmap
> +EOF
> +
> +test_expect_success 'grep --only-matching --heading' '
> + git grep --only-matching --heading --line-number --column mmap file 
> >actual &&
> + test_cmp expected actual
> +'
> +
>  cat >expected <  <BOLD;GREEN>hello.c
>  4:int main(int argc, const <BLACK;BYELLOW>char **argv)

We should test this a lot more, I think a good way to do that would be
to extend this series by first importing GNU grep's -o tests, see
http://git.savannah.gnu.org/cgit/grep.git/tree/tests/foad1 they are
license-compatible. Then change the grep_test() function to call git
grep instead.

It should also be tested with the various grep.patternType options to
make sure it works with basic, extended, perl, fixed etc.

[PATCH 0/2] builtin/grep.c: teach '-o', '--only-matching'

[Taylor Blau]

Hi,

Attached is a series to teach 'git-grep(1)' how to respond to
'--only-matching' (a-la GNU grep(1)'s --only-matching, including an '-o'
synonym) to only print the matching component(s) of a line. It is based
on v4 of tb/grep-colno, which was sent in [1].

This was suggested to me by Ævar in [2].

This change was fairly straightforward, as Ævar suggests in [3], with
the only complication being that we must print a line header multiple
times when there are >1 matches per-line. This requirement pushes the
implementation towards the extraction of a show_line_header() function,
and some minor changes in show_line() to reflect.

Thank you in advance for your review.


Thanks,
Taylor

[1]: https://public-inbox.org/git/cover.1525488108.git...@ttaylorr.com
[2]: https://public-inbox.org/git/874lk2e4he@evledraar.gmail.com
[3]: https://public-inbox.org/git/87in9ucsbb@evledraar.gmail.com

Taylor Blau (2):
  grep.c: extract show_line_header()
  builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

 Documentation/git-grep.txt |  6 +++-
 builtin/grep.c |  1 +
 grep.c | 67 +-
 grep.h |  1 +
 t/t7810-grep.sh| 33 +++
 5 files changed, 85 insertions(+), 23 deletions(-)

--
2.17.0

[PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Taylor Blau]

Teach GNU grep(1)'s '-o' ('--only-matching') to 'git-grep'. This option
prints only the matching components of each line. It writes multiple
lines if more than one match exists on a given line.

For example:

  $ git grep -on --column --heading git -- README.md | head -3
  README.md
  15:56:git
  18:20:git

By using show_line_header(), 'git grep --only-matching' correctly
respects the '--header' option:

  $ git grep -on --column --heading git -- README.md | head -4
  README.md
  15:56:git
  18:20:git
  19:16:git

Signed-off-by: Taylor Blau <m...@ttaylorr.com>
---
 Documentation/git-grep.txt |  6 +-
 builtin/grep.c |  1 +
 grep.c | 23 ---
 grep.h |  1 +
 t/t7810-grep.sh| 33 +
 5 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index d451cd8883..9754923041 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -20,7 +20,7 @@ SYNOPSIS
   [-c | --count] [--all-match] [-q | --quiet]
   [--max-depth ]
   [--color[=] | --no-color]
-  [--break] [--heading] [-p | --show-function]
+  [--break] [--heading] [-o | --only-matching] [-p | --show-function]
   [-A ] [-B ] [-C ]
   [-W | --function-context]
   [--threads ]
@@ -221,6 +221,10 @@ providing this option will cause it to die.
Show the filename above the matches in that file instead of
at the start of each shown line.
 
+--o::
+--only-matching::
+   Show only the matching part of the lines.
+
 -p::
 --show-function::
Show the preceding line that contains the function name of
diff --git a/builtin/grep.c b/builtin/grep.c
index 5c83f17759..5028bf96cf 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -851,6 +851,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
N_("print empty line between matches from different 
files")),
OPT_BOOL(0, "heading", ,
N_("show filename only once above matches from same 
file")),
+   OPT_BOOL('o', "only-matching", _matching, N_("show 
only matches")),
OPT_GROUP(""),
OPT_CALLBACK('C', "context", , N_("n"),
N_("show  context lines before and after matches"),
diff --git a/grep.c b/grep.c
index 89dd719e4d..da3f8e6266 100644
--- a/grep.c
+++ b/grep.c
@@ -1422,11 +1422,13 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
}
}
show_line_header(opt, name, lno, cno, sign);
-   if (opt->color) {
+   if (opt->color || opt->only_matching) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
int ch = *eol;
int eflags = 0;
+   int first = 1;
+   int offset = 1;
 
if (sign == ':')
match_color = opt->color_match_selected;
@@ -1443,16 +1445,31 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
if (match.rm_so == match.rm_eo)
break;
 
-   output_color(opt, bol, match.rm_so, line_color);
+   if (!opt->only_matching)
+   output_color(opt, bol, match.rm_so, line_color);
+   else if (!first) {
+   /*
+* We are given --only-matching, and this is not
+* the first match on a line. Reprint the
+* newline and header before showing another
+* match.
+*/
+   opt->output(opt, "\n", 1);
+   show_line_header(opt, name, lno,
+   offset+match.rm_so, sign);
+   }
output_color(opt, bol + match.rm_so,
 match.rm_eo - match.rm_so, match_color);
+   offset += match.rm_eo;
bol += match.rm_eo;
rest -= match.rm_eo;
eflags = REG_NOTBOL;
+   first = 0;
}
*eol = ch;
}
-   output_color(opt, bol, rest, line_color);
+   if (!opt->only_matching)
+   output_color(opt, bol, rest, line_color);
opt->output(opt, "\n", 1);
 }
 
diff --git a/grep.h b/grep.h
index 08a0b391c5..24c1460100 100644
--- a/grep.h
+++ b/grep.h
@@ -126,6 +126,7 @@ struct grep_opt {
const char *prefix;
int prefix_length;
regex

[PATCH v3 4/7] doc: add '-d' and '-o' for 'git push'

[Andreas Heiduk]

Add the missing `-o` shortcut for `--push-option` to the synopsis.
Add the missing `-d` shortcut for `--delete` in the main section.

Signed-off-by: Andreas Heiduk <ashei...@gmail.com>
Reviewed-by: Martin Ågren <martin.ag...@gmail.com>
---
 Documentation/git-push.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 5b08302fc2..f2bbda6e32 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | 
--dry-run] [--receive-pack=]
   [--repo=] [-f | --force] [-d | --delete] [--prune] [-v | 
--verbose]
-  [-u | --set-upstream] [--push-option=]
+  [-u | --set-upstream] [-o  | --push-option=]
   [--[no-]signed|--signed=(true|false|if-asked)]
   [--force-with-lease[=[:]]]
   [--no-verify] [ [...]]
@@ -123,6 +123,7 @@ already exists on the remote side.
will be tab-separated and sent to stdout instead of stderr.  The full
symbolic names of the refs will be given.
 
+-d::
 --delete::
All listed refs are deleted from the remote repository. This is
the same as prefixing all refs with a colon.
-- 
2.16.2

Re: [PATCH v3] unpack_trees: fix breakage when o->src_index != o->dst_index

[Duy Nguyen]

On Mon, Apr 30, 2018 at 6:19 PM, Elijah Newren <new...@gmail.com> wrote:
> Hi Duy,
>
> On Mon, Apr 30, 2018 at 7:45 AM, Duy Nguyen <pclo...@gmail.com> wrote:
>> On Mon, Apr 30, 2018 at 4:42 PM, Duy Nguyen <pclo...@gmail.com> wrote:
>>> On Sun, Apr 29, 2018 at 10:53 PM, Johannes Schindelin
>>> <johannes.schinde...@gmx.de> wrote:
>>>>> > @@ -1412,12 +1422,13 @@ int unpack_trees(unsigned len, struct tree_desc 
>>>>> > *t, struct unpack_trees_options
>>>>> >   WRITE_TREE_SILENT |
>>>>> >       WRITE_TREE_REPAIR);
>>>>> > }
>>>>> > -   move_index_extensions(>result, o->dst_index);
>>>>> > +   move_index_extensions(>result, o->src_index);
>>>>>
>>>>> While this looks like the right thing to do on paper, I believe it's
>>>>> actually broken for a specific case of untracked cache. In short,
>>>>> please do not touch this line. I will send a patch to revert
>>>>> edf3b90553 (unpack-trees: preserve index extensions - 2017-05-08),
>>>>> which essentially deletes this line, with proper explanation and
>>>>> perhaps a test if I could come up with one.
>>>>>
>>>>> When we update the index, we depend on the fact that all updates must
>>>>> invalidate the right untracked cache correctly. In this unpack
>>>>> operations, we start copying entries over from src to result. Since
>>>>> 'result' (at least from the beginning) does not have an untracked
>>>>> cache, it has nothing to invalidate when we copy entries over. By the
>>>>> time we have done preparing 'result', what's recorded in src's (or
>>>>> dst's for that matter) untracked cache may or may not apply to
>>>>> 'result'  index anymore. This copying only leads to more problems when
>>>>> untracked cache is used.
>>>>
>>>> Is there really no way to invalidate just individual entries?
>>>
>>> Grr the short answer is the current code (i.e. without Elijah's
>>> changes) works but in a twisted way. So you get to keep untracked
>>> cache in the end.
>>
>> GAAAHH.. it works _with_ Elijah's changes (since he made the change
>> from dst to src) not without (and no performance regression).
>
> So...is that an Acked-by for the patch

Yes, Acked-by: me.

> or does the "two wrong make a
> right, I guess" comment suggest that we should still drop the
> move_index_extensions change (essentially reverting to v1 of the PATCH
> as found at 20180421193736.12722-1-new...@gmail.com), and you'll fix
> things up further in a separate series?

I think I'll stay away from this file for a while. When I gather
enough courage, I'll need to read it through since it sounds like a
mine field.
-- 
Duy

Re: [PATCH v3] unpack_trees: fix breakage when o->src_index != o->dst_index

[Elijah Newren]

Hi Duy,

On Mon, Apr 30, 2018 at 7:45 AM, Duy Nguyen <pclo...@gmail.com> wrote:
> On Mon, Apr 30, 2018 at 4:42 PM, Duy Nguyen <pclo...@gmail.com> wrote:
>> On Sun, Apr 29, 2018 at 10:53 PM, Johannes Schindelin
>> <johannes.schinde...@gmx.de> wrote:
>>>> > @@ -1412,12 +1422,13 @@ int unpack_trees(unsigned len, struct tree_desc 
>>>> > *t, struct unpack_trees_options
>>>> >   WRITE_TREE_SILENT |
>>>> >   WRITE_TREE_REPAIR);
>>>> > }
>>>> > -   move_index_extensions(>result, o->dst_index);
>>>> > +   move_index_extensions(>result, o->src_index);
>>>>
>>>> While this looks like the right thing to do on paper, I believe it's
>>>> actually broken for a specific case of untracked cache. In short,
>>>> please do not touch this line. I will send a patch to revert
>>>> edf3b90553 (unpack-trees: preserve index extensions - 2017-05-08),
>>>> which essentially deletes this line, with proper explanation and
>>>> perhaps a test if I could come up with one.
>>>>
>>>> When we update the index, we depend on the fact that all updates must
>>>> invalidate the right untracked cache correctly. In this unpack
>>>> operations, we start copying entries over from src to result. Since
>>>> 'result' (at least from the beginning) does not have an untracked
>>>> cache, it has nothing to invalidate when we copy entries over. By the
>>>> time we have done preparing 'result', what's recorded in src's (or
>>>> dst's for that matter) untracked cache may or may not apply to
>>>> 'result'  index anymore. This copying only leads to more problems when
>>>> untracked cache is used.
>>>
>>> Is there really no way to invalidate just individual entries?
>>
>> Grr the short answer is the current code (i.e. without Elijah's
>> changes) works but in a twisted way. So you get to keep untracked
>> cache in the end.
>
> GAAAHH.. it works _with_ Elijah's changes (since he made the change
> from dst to src) not without (and no performance regression).

So...is that an Acked-by for the patch, or does the "two wrong make a
right, I guess" comment suggest that we should still drop the
move_index_extensions change (essentially reverting to v1 of the PATCH
as found at 20180421193736.12722-1-new...@gmail.com), and you'll fix
things up further in a separate series?

> This file really messes my brain up.

I'm glad I'm not the only one.  :-)


Elijah

Re: [PATCH v3] unpack_trees: fix breakage when o->src_index != o->dst_index

[Duy Nguyen]

On Mon, Apr 30, 2018 at 4:42 PM, Duy Nguyen <pclo...@gmail.com> wrote:
> On Sun, Apr 29, 2018 at 10:53 PM, Johannes Schindelin
> <johannes.schinde...@gmx.de> wrote:
>>> > @@ -1412,12 +1422,13 @@ int unpack_trees(unsigned len, struct tree_desc 
>>> > *t, struct unpack_trees_options
>>> >   WRITE_TREE_SILENT |
>>> >   WRITE_TREE_REPAIR);
>>> > }
>>> > -   move_index_extensions(>result, o->dst_index);
>>> > +   move_index_extensions(>result, o->src_index);
>>>
>>> While this looks like the right thing to do on paper, I believe it's
>>> actually broken for a specific case of untracked cache. In short,
>>> please do not touch this line. I will send a patch to revert
>>> edf3b90553 (unpack-trees: preserve index extensions - 2017-05-08),
>>> which essentially deletes this line, with proper explanation and
>>> perhaps a test if I could come up with one.
>>>
>>> When we update the index, we depend on the fact that all updates must
>>> invalidate the right untracked cache correctly. In this unpack
>>> operations, we start copying entries over from src to result. Since
>>> 'result' (at least from the beginning) does not have an untracked
>>> cache, it has nothing to invalidate when we copy entries over. By the
>>> time we have done preparing 'result', what's recorded in src's (or
>>> dst's for that matter) untracked cache may or may not apply to
>>> 'result'  index anymore. This copying only leads to more problems when
>>> untracked cache is used.
>>
>> Is there really no way to invalidate just individual entries?
>
> Grr the short answer is the current code (i.e. without Elijah's
> changes) works but in a twisted way. So you get to keep untracked
> cache in the end.

GAAAHH.. it works _with_ Elijah's changes (since he made the change
from dst to src) not without (and no performance regression). This
file really messes my brain up.
-- 
Duy

Re: [PATCH v3] unpack_trees: fix breakage when o->src_index != o->dst_index

[Duy Nguyen]

On Sun, Apr 29, 2018 at 10:53 PM, Johannes Schindelin
<johannes.schinde...@gmx.de> wrote:
>> > @@ -1412,12 +1422,13 @@ int unpack_trees(unsigned len, struct tree_desc 
>> > *t, struct unpack_trees_options
>> >   WRITE_TREE_SILENT |
>> >   WRITE_TREE_REPAIR);
>> > }
>> > -   move_index_extensions(>result, o->dst_index);
>> > +   move_index_extensions(>result, o->src_index);
>>
>> While this looks like the right thing to do on paper, I believe it's
>> actually broken for a specific case of untracked cache. In short,
>> please do not touch this line. I will send a patch to revert
>> edf3b90553 (unpack-trees: preserve index extensions - 2017-05-08),
>> which essentially deletes this line, with proper explanation and
>> perhaps a test if I could come up with one.
>>
>> When we update the index, we depend on the fact that all updates must
>> invalidate the right untracked cache correctly. In this unpack
>> operations, we start copying entries over from src to result. Since
>> 'result' (at least from the beginning) does not have an untracked
>> cache, it has nothing to invalidate when we copy entries over. By the
>> time we have done preparing 'result', what's recorded in src's (or
>> dst's for that matter) untracked cache may or may not apply to
>> 'result'  index anymore. This copying only leads to more problems when
>> untracked cache is used.
>
> Is there really no way to invalidate just individual entries?

Grr the short answer is the current code (i.e. without Elijah's
changes) works but in a twisted way. So you get to keep untracked
cache in the end.

I was right about the invalidation stuff. I knew about
invalidate_ce_path() in this file. What I didn't remember was this
function actually invalidates entries from the _source_ index, not the
result one. What kind of logic is that? You copy/move entries from
source to result than you go invalidate the source. Since the original
move_index_extensions() call moves extensions from the source, these
are already properly invalidated (both untracked cache and cache
tree), it it looks like it does the right thing. Two wrongs make a
right, I guess.

Sorry for venting. I was not happy with what I found. And sorry for
wasting your time making this move_index.. change then remove it.

> I have a couple of worktrees which are *huge*. And edf3b90553 really
> helped relieve the pain a bit when running `git status`. Now you say that
> even a `git checkout -b new-branch` would blow the untracked cache away
> again?
>
> It would be *really* nice if we could prevent that performance regression
> somehow.
>
> Ciao,
> Dscho



-- 
Duy

Re: [PATCH v3] unpack_trees: fix breakage when o->src_index != o->dst_index

[Johannes Schindelin]

Hi Duy,

On Sun, 29 Apr 2018, Duy Nguyen wrote:

> On Tue, Apr 24, 2018 at 8:50 AM, Elijah Newren <new...@gmail.com> wrote:
> > Currently, all callers of unpack_trees() set o->src_index == o->dst_index.
> > The code in unpack_trees() does not correctly handle them being different.
> > There are two separate issues:
> >
> > First, there is the possibility of memory corruption.  Since
> > unpack_trees() creates a temporary index in o->result and then discards
> > o->dst_index and overwrites it with o->result, in the special case that
> > o->src_index == o->dst_index, it is safe to just reuse o->src_index's
> > split_index for o->result.  However, when src and dst are different,
> > reusing o->src_index's split_index for o->result will cause the
> > split_index to be shared.  If either index then has entries replaced or
> > removed, it will result in the other index referring to free()'d memory.
> >
> > Second, we can drop the index extensions.  Previously, we were moving
> > index extensions from o->dst_index to o->result.  Since o->src_index is
> > the one that will have the necessary extensions (o->dst_index is likely to
> > be a new index temporary index created to store the results), we should be
> > moving the index extensions from there.
> >
> > Signed-off-by: Elijah Newren <new...@gmail.com>
> > ---
> >
> > Differences from v2:
> >   - Don't NULLify src_index until we're done using it
> >   - Actually built and tested[1]
> >
> > But it now passes the testsuite on both linux and mac[2], and I even 
> > re-merged
> > all 53288 merge commits in linux.git (with a merge of this patch together 
> > with
> > the directory rename detection series) for good measure.  [Only 7 commits
> > showed a difference, all due to directory rename detection kicking in.]
> >
> > [1] Turns out that getting all fancy with an m4.10xlarge and nice levels of
> > parallelization are great until you realize that your new setup omitted a
> > critical step, leaving you running a slightly stale version of git 
> > instead...
> > :-(
> >
> > [2] Actually, I get two test failures on mac from t0050-filesystem.sh, both
> > with unicode normalization tests, but those two tests fail before my changes
> > too.  All the other tests pass.
> >
> >  unpack-trees.c | 19 +++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/unpack-trees.c b/unpack-trees.c
> > index e73745051e..49526d70aa 100644
> > --- a/unpack-trees.c
> > +++ b/unpack-trees.c
> > @@ -1284,9 +1284,20 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> > struct unpack_trees_options
> > o->result.timestamp.sec = o->src_index->timestamp.sec;
> > o->result.timestamp.nsec = o->src_index->timestamp.nsec;
> > o->result.version = o->src_index->version;
> > -   o->result.split_index = o->src_index->split_index;
> > -   if (o->result.split_index)
> > +   if (!o->src_index->split_index) {
> > +   o->result.split_index = NULL;
> > +   } else if (o->src_index == o->dst_index) {
> > +   /*
> > +* o->dst_index (and thus o->src_index) will be discarded
> > +* and overwritten with o->result at the end of this 
> > function,
> > +* so just use src_index's split_index to avoid having to
> > +* create a new one.
> > +*/
> > +   o->result.split_index = o->src_index->split_index;
> > o->result.split_index->refcount++;
> > +   } else {
> > +   o->result.split_index = init_split_index(>result);
> > +   }
> > hashcpy(o->result.sha1, o->src_index->sha1);
> > o->merge_size = len;
> > mark_all_ce_unused(o->src_index);
> > @@ -1401,7 +1412,6 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> > struct unpack_trees_options
> > }
> > }
> >
> > -   o->src_index = NULL;
> > ret = check_updates(o) ? (-2) : 0;
> > if (o->dst_index) {
> > if (!ret) {
> > @@ -1412,12 +1422,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> > struct unpack_trees_options
> >   WRITE_TREE_SILENT |
> >   WRITE_TREE_REPAIR);
>

Re: [PATCH v3] unpack_trees: fix breakage when o->src_index != o->dst_index

[Duy Nguyen]

On Tue, Apr 24, 2018 at 8:50 AM, Elijah Newren <new...@gmail.com> wrote:
> Currently, all callers of unpack_trees() set o->src_index == o->dst_index.
> The code in unpack_trees() does not correctly handle them being different.
> There are two separate issues:
>
> First, there is the possibility of memory corruption.  Since
> unpack_trees() creates a temporary index in o->result and then discards
> o->dst_index and overwrites it with o->result, in the special case that
> o->src_index == o->dst_index, it is safe to just reuse o->src_index's
> split_index for o->result.  However, when src and dst are different,
> reusing o->src_index's split_index for o->result will cause the
> split_index to be shared.  If either index then has entries replaced or
> removed, it will result in the other index referring to free()'d memory.
>
> Second, we can drop the index extensions.  Previously, we were moving
> index extensions from o->dst_index to o->result.  Since o->src_index is
> the one that will have the necessary extensions (o->dst_index is likely to
> be a new index temporary index created to store the results), we should be
> moving the index extensions from there.
>
> Signed-off-by: Elijah Newren <new...@gmail.com>
> ---
>
> Differences from v2:
>   - Don't NULLify src_index until we're done using it
>   - Actually built and tested[1]
>
> But it now passes the testsuite on both linux and mac[2], and I even re-merged
> all 53288 merge commits in linux.git (with a merge of this patch together with
> the directory rename detection series) for good measure.  [Only 7 commits
> showed a difference, all due to directory rename detection kicking in.]
>
> [1] Turns out that getting all fancy with an m4.10xlarge and nice levels of
> parallelization are great until you realize that your new setup omitted a
> critical step, leaving you running a slightly stale version of git instead...
> :-(
>
> [2] Actually, I get two test failures on mac from t0050-filesystem.sh, both
> with unicode normalization tests, but those two tests fail before my changes
> too.  All the other tests pass.
>
>  unpack-trees.c | 19 +++
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index e73745051e..49526d70aa 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1284,9 +1284,20 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> struct unpack_trees_options
> o->result.timestamp.sec = o->src_index->timestamp.sec;
> o->result.timestamp.nsec = o->src_index->timestamp.nsec;
> o->result.version = o->src_index->version;
> -   o->result.split_index = o->src_index->split_index;
> -   if (o->result.split_index)
> +   if (!o->src_index->split_index) {
> +   o->result.split_index = NULL;
> +   } else if (o->src_index == o->dst_index) {
> +   /*
> +* o->dst_index (and thus o->src_index) will be discarded
> +    * and overwritten with o->result at the end of this function,
> +* so just use src_index's split_index to avoid having to
> +    * create a new one.
> +*/
> +   o->result.split_index = o->src_index->split_index;
> o->result.split_index->refcount++;
> +   } else {
> +   o->result.split_index = init_split_index(>result);
> +   }
> hashcpy(o->result.sha1, o->src_index->sha1);
> o->merge_size = len;
> mark_all_ce_unused(o->src_index);
> @@ -1401,7 +1412,6 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> struct unpack_trees_options
> }
> }
>
> -   o->src_index = NULL;
> ret = check_updates(o) ? (-2) : 0;
> if (o->dst_index) {
>     if (!ret) {
> @@ -1412,12 +1422,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> struct unpack_trees_options
>   WRITE_TREE_SILENT |
>   WRITE_TREE_REPAIR);
> }
> -   move_index_extensions(>result, o->dst_index);
> +   move_index_extensions(>result, o->src_index);

While this looks like the right thing to do on paper, I believe it's
actually broken for a specific case of untracked cache. In short,
please do not touch this line. I will send a patch to revert
edf3b90553 (unpack-trees: preserve index extensions - 2017-05-08),
which essentially deletes this line, with proper explanation and
perhaps a test if I

[PATCH v2 4/6] doc: add '-d' and '-o' for 'git push'

[Andreas Heiduk]

Add the missing `-o` shortcut for `--push-option` to the synopsis.
Add the missing `-d` shortcut for `--delete` in the main section.

Signed-off-by: Andreas Heiduk <ashei...@gmail.com>
Reviewed-by: Martin Ågren <martin.ag...@gmail.com>
---
 Documentation/git-push.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 5b08302fc2..f2bbda6e32 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | 
--dry-run] [--receive-pack=]
   [--repo=] [-f | --force] [-d | --delete] [--prune] [-v | 
--verbose]
-  [-u | --set-upstream] [--push-option=]
+  [-u | --set-upstream] [-o  | --push-option=]
   [--[no-]signed|--signed=(true|false|if-asked)]
   [--force-with-lease[=[:]]]
   [--no-verify] [ [...]]
@@ -123,6 +123,7 @@ already exists on the remote side.
will be tab-separated and sent to stdout instead of stderr.  The full
symbolic names of the refs will be given.
 
+-d::
 --delete::
All listed refs are deleted from the remote repository. This is
the same as prefixing all refs with a colon.
-- 
2.16.2

[PATCH v3] unpack_trees: fix breakage when o->src_index != o->dst_index

[Elijah Newren]

Currently, all callers of unpack_trees() set o->src_index == o->dst_index.
The code in unpack_trees() does not correctly handle them being different.
There are two separate issues:

First, there is the possibility of memory corruption.  Since
unpack_trees() creates a temporary index in o->result and then discards
o->dst_index and overwrites it with o->result, in the special case that
o->src_index == o->dst_index, it is safe to just reuse o->src_index's
split_index for o->result.  However, when src and dst are different,
reusing o->src_index's split_index for o->result will cause the
split_index to be shared.  If either index then has entries replaced or
removed, it will result in the other index referring to free()'d memory.

Second, we can drop the index extensions.  Previously, we were moving
index extensions from o->dst_index to o->result.  Since o->src_index is
the one that will have the necessary extensions (o->dst_index is likely to
be a new index temporary index created to store the results), we should be
moving the index extensions from there.

Signed-off-by: Elijah Newren <new...@gmail.com>
---

Differences from v2:
  - Don't NULLify src_index until we're done using it
  - Actually built and tested[1]

But it now passes the testsuite on both linux and mac[2], and I even re-merged
all 53288 merge commits in linux.git (with a merge of this patch together with
the directory rename detection series) for good measure.  [Only 7 commits
showed a difference, all due to directory rename detection kicking in.]

[1] Turns out that getting all fancy with an m4.10xlarge and nice levels of
parallelization are great until you realize that your new setup omitted a
critical step, leaving you running a slightly stale version of git instead...
:-(

[2] Actually, I get two test failures on mac from t0050-filesystem.sh, both
with unicode normalization tests, but those two tests fail before my changes
too.  All the other tests pass.

 unpack-trees.c | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index e73745051e..49526d70aa 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1284,9 +1284,20 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
o->result.timestamp.sec = o->src_index->timestamp.sec;
o->result.timestamp.nsec = o->src_index->timestamp.nsec;
o->result.version = o->src_index->version;
-   o->result.split_index = o->src_index->split_index;
-   if (o->result.split_index)
+   if (!o->src_index->split_index) {
+   o->result.split_index = NULL;
+   } else if (o->src_index == o->dst_index) {
+   /*
+    * o->dst_index (and thus o->src_index) will be discarded
+* and overwritten with o->result at the end of this function,
+* so just use src_index's split_index to avoid having to
+* create a new one.
+*/
+   o->result.split_index = o->src_index->split_index;
o->result.split_index->refcount++;
+   } else {
+   o->result.split_index = init_split_index(>result);
+   }
hashcpy(o->result.sha1, o->src_index->sha1);
o->merge_size = len;
mark_all_ce_unused(o->src_index);
@@ -1401,7 +1412,6 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
}
}
 
-   o->src_index = NULL;
ret = check_updates(o) ? (-2) : 0;
if (o->dst_index) {
if (!ret) {
@@ -1412,12 +1422,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
  WRITE_TREE_SILENT |
  WRITE_TREE_REPAIR);
}
-   move_index_extensions(>result, o->dst_index);
+   move_index_extensions(>result, o->src_index);
discard_index(o->dst_index);
*o->dst_index = o->result;
} else {
discard_index(>result);
}
+   o->src_index = NULL;
 
 done:
clear_exclude_list();
-- 
2.17.0.253.g32393f1d0a

Re: [PATCH v2] unpack_trees: fix breakage when o->src_index != o->dst_index

[Junio C Hamano]

Elijah Newren <new...@gmail.com> writes:

>  unpack-trees.c | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index e73745051e..08f6cab82e 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1284,9 +1284,20 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> struct unpack_trees_options
>   o->result.timestamp.sec = o->src_index->timestamp.sec;
>   o->result.timestamp.nsec = o->src_index->timestamp.nsec;
>   o->result.version = o->src_index->version;
> - o->result.split_index = o->src_index->split_index;
> - if (o->result.split_index)
> + if (!o->src_index->split_index) {
> + o->result.split_index = NULL;
> + } else if (o->src_index == o->dst_index) {
> + /*
> +  * o->dst_index (and thus o->src_index) will be discarded
> +  * and overwritten with o->result at the end of this function,
> +  * so just use src_index's split_index to avoid having to
> +      * create a new one.
> +  */
> +     o->result.split_index = o->src_index->split_index;
>       o->result.split_index->refcount++;
> + } else {
> + o->result.split_index = init_split_index(>result);
> + }
>   hashcpy(o->result.sha1, o->src_index->sha1);
>   o->merge_size = len;
>   mark_all_ce_unused(o->src_index);
> @@ -1412,7 +1423,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
> struct unpack_trees_options
> WRITE_TREE_SILENT |
> WRITE_TREE_REPAIR);
>   }
> - move_index_extensions(>result, o->dst_index);
> + move_index_extensions(>result, o->src_index);

Can src_index be NULL here?  I am getting segfaults everywhere,
starting from t-basic that populates the index by reading one
tree object via read-tree.

>   discard_index(o->dst_index);
>   *o->dst_index = o->result;
>   } else {

Re: [PATCH v2] unpack_trees: fix breakage when o->src_index != o->dst_index

[Junio C Hamano]

Elijah Newren  writes:

> Marked as PATCH v2, though I marked the previous one as "RFC PATCH v10
> 32.5/36" because I thought I was going to put it in my series.  But it is
> an independent fix that my series needs.

Thanks.  Let's take this before the remainder of the series ;-)

[PATCH v2] unpack_trees: fix breakage when o->src_index != o->dst_index

[Elijah Newren]

Currently, all callers of unpack_trees() set o->src_index == o->dst_index.
The code in unpack_trees() does not correctly handle them being different.
There are two separate issues:

First, there is the possibility of memory corruption.  Since
unpack_trees() creates a temporary index in o->result and then discards
o->dst_index and overwrites it with o->result, in the special case that
o->src_index == o->dst_index, it is safe to just reuse o->src_index's
split_index for o->result.  However, when src and dst are different,
reusing o->src_index's split_index for o->result will cause the
split_index to be shared.  If either index then has entries replaced or
removed, it will result in the other index referring to free()'d memory.

Second, we can drop the index extensions.  Previously, we were moving
index extensions from o->dst_index to o->result.  Since o->src_index is
the one that will have the necessary extensions (o->dst_index is likely to
be a new index temporary index created to store the results), we should be
moving the index extensions from there.  (Thanks to Duy Nguyen for
noticing and suggesting this fix.)

Helped by: Duy Nguyen <pclo...@gmail.com>
Signed-off-by: Elijah Newren <new...@gmail.com>
---

Marked as PATCH v2, though I marked the previous one as "RFC PATCH v10
32.5/36" because I thought I was going to put it in my series.  But it is
an independent fix that my series needs.

Also, I reran my merge-all-linux.git merges comparison[1]; as expected,
this updated patch didn't change the results.

[1] 
https://public-inbox.org/git/cabpp-bhmt1hjr8a_wkxvsexv9algg5032vv5uee2-htpyua...@mail.gmail.com/

 unpack-trees.c | 17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index e73745051e..08f6cab82e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1284,9 +1284,20 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
    o->result.timestamp.sec = o->src_index->timestamp.sec;
o->result.timestamp.nsec = o->src_index->timestamp.nsec;
o->result.version = o->src_index->version;
-   o->result.split_index = o->src_index->split_index;
-   if (o->result.split_index)
+   if (!o->src_index->split_index) {
+   o->result.split_index = NULL;
+   } else if (o->src_index == o->dst_index) {
+       /*
+* o->dst_index (and thus o->src_index) will be discarded
+* and overwritten with o->result at the end of this function,
+* so just use src_index's split_index to avoid having to
+    * create a new one.
+*/
+   o->result.split_index = o->src_index->split_index;
o->result.split_index->refcount++;
+   } else {
+   o->result.split_index = init_split_index(>result);
+   }
hashcpy(o->result.sha1, o->src_index->sha1);
o->merge_size = len;
mark_all_ce_unused(o->src_index);
@@ -1412,7 +1423,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
      WRITE_TREE_SILENT |
      WRITE_TREE_REPAIR);
    }
-   move_index_extensions(>result, o->dst_index);
+   move_index_extensions(>result, o->src_index);
discard_index(o->dst_index);
*o->dst_index = o->result;
} else {
-- 
2.17.0.296.gaac25b4b81

[PATCH v10 15/36] merge-recursive: make !o->detect_rename codepath more obvious

[Elijah Newren]

Previously, if !o->detect_rename then get_renames() would return an
empty string_list, and then process_renames() would have nothing to
iterate over.  It seems more straightforward to simply avoid calling
either function in that case.

Reviewed-by: Stefan Beller <sbel...@google.com>
Signed-off-by: Elijah Newren <new...@gmail.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 merge-recursive.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index fc96653f63..5da60b9516 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1339,8 +1339,6 @@ static struct string_list *get_renames(struct 
merge_options *o,
struct diff_options opts;
 
renames = xcalloc(1, sizeof(struct string_list));
-   if (!o->detect_rename)
-   return renames;
 
diff_setup();
opts.flags.recursive = 1;
@@ -1658,6 +1656,12 @@ static int handle_renames(struct merge_options *o,
  struct string_list *entries,
  struct rename_info *ri)
 {
+   ri->head_renames = NULL;
+   ri->merge_renames = NULL;
+
+   if (!o->detect_rename)
+   return 1;
+
    ri->head_renames  = get_renames(o, head, common, head, merge, entries);
    ri->merge_renames = get_renames(o, merge, common, head, merge, entries);
return process_renames(o, ri->head_renames, ri->merge_renames);
@@ -1668,6 +1672,9 @@ static void cleanup_rename(struct string_list *rename)
const struct rename *re;
int i;
 
+   if (rename == NULL)
+   return;
+
for (i = 0; i < rename->nr; i++) {
re = rename->items[i].util;
diff_free_filepair(re->pair);
-- 
2.17.0.290.ge988e9ce2a

Witam potrzebuję Pilnej odpowiedzi, abyśmy mogli porozmawiać o ważnej sprawie

[Sergeant Schieble Anderson]

[PATCH v8 15/29] merge-recursive: make !o->detect_rename codepath more obvious

[Elijah Newren]

Previously, if !o->detect_rename then get_renames() would return an
empty string_list, and then process_renames() would have nothing to
iterate over.  It seems more straightforward to simply avoid calling
either function in that case.

Reviewed-by: Stefan Beller <sbel...@google.com>
Signed-off-by: Elijah Newren <new...@gmail.com>
---
 merge-recursive.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1986af79a9..4e6d0c248e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1338,8 +1338,6 @@ static struct string_list *get_renames(struct 
merge_options *o,
struct diff_options opts;
 
renames = xcalloc(1, sizeof(struct string_list));
-   if (!o->detect_rename)
-   return renames;
 
diff_setup();
opts.flags.recursive = 1;
@@ -1657,6 +1655,12 @@ static int handle_renames(struct merge_options *o,
  struct string_list *entries,
  struct rename_info *ri)
 {
+   ri->head_renames = NULL;
+   ri->merge_renames = NULL;
+
+   if (!o->detect_rename)
+   return 1;
+
ri->head_renames  = get_renames(o, head, common, head, merge, entries);
ri->merge_renames = get_renames(o, merge, common, head, merge, entries);
return process_renames(o, ri->head_renames, ri->merge_renames);
@@ -1667,6 +1671,9 @@ static void cleanup_rename(struct string_list *rename)
const struct rename *re;
int i;
 
+   if (rename == NULL)
+   return;
+
for (i = 0; i < rename->nr; i++) {
re = rename->items[i].util;
diff_free_filepair(re->pair);
-- 
2.16.1.232.g28d5be9217

Re: [PATCH v7 15/31] merge-recursive: make !o->detect_rename codepath more obvious

[Stefan Beller]

On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren <new...@gmail.com> wrote:
> Previously, if !o->detect_rename then get_renames() would return an
> empty string_list, and then process_renames() would have nothing to
> iterate over.  It seems more straightforward to simply avoid calling
> either function in that case.
>
> Signed-off-by: Elijah Newren <new...@gmail.com>
> ---
>  merge-recursive.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 1986af79a9..4e6d0c248e 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1338,8 +1338,6 @@ static struct string_list *get_renames(struct 
> merge_options *o,
> struct diff_options opts;
>
>     renames = xcalloc(1, sizeof(struct string_list));
> -   if (!o->detect_rename)
> -   return renames;
>
> diff_setup();
> opts.flags.recursive = 1;
> @@ -1657,6 +1655,12 @@ static int handle_renames(struct merge_options *o,
>   struct string_list *entries,
>   struct rename_info *ri)
>  {
> +   ri->head_renames = NULL;
> +   ri->merge_renames = NULL;
> +
> +   if (!o->detect_rename)
> +   return 1;
> +
> ri->head_renames  = get_renames(o, head, common, head, merge, 
> entries);
> ri->merge_renames = get_renames(o, merge, common, head, merge, 
> entries);

So this pulls the condition out and we just do not call get_renames.
Makes sense as then we also do not allocate memory for the stringlists
in get_renames

Reviewed-by: Stefan Beller <sbel...@google.com>

[PATCH v7 15/31] merge-recursive: make !o->detect_rename codepath more obvious

[Elijah Newren]

Previously, if !o->detect_rename then get_renames() would return an
empty string_list, and then process_renames() would have nothing to
iterate over.  It seems more straightforward to simply avoid calling
either function in that case.

Signed-off-by: Elijah Newren <new...@gmail.com>
---
 merge-recursive.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1986af79a9..4e6d0c248e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1338,8 +1338,6 @@ static struct string_list *get_renames(struct 
merge_options *o,
struct diff_options opts;
 
renames = xcalloc(1, sizeof(struct string_list));
-       if (!o->detect_rename)
-   return renames;
 
diff_setup();
opts.flags.recursive = 1;
@@ -1657,6 +1655,12 @@ static int handle_renames(struct merge_options *o,
  struct string_list *entries,
  struct rename_info *ri)
 {
+   ri->head_renames = NULL;
+   ri->merge_renames = NULL;
+
+   if (!o->detect_rename)
+   return 1;
+
ri->head_renames  = get_renames(o, head, common, head, merge, entries);
ri->merge_renames = get_renames(o, merge, common, head, merge, entries);
    return process_renames(o, ri->head_renames, ri->merge_renames);
@@ -1667,6 +1671,9 @@ static void cleanup_rename(struct string_list *rename)
const struct rename *re;
int i;
 
+   if (rename == NULL)
+   return;
+
for (i = 0; i < rename->nr; i++) {
re = rename->items[i].util;
diff_free_filepair(re->pair);
-- 
2.16.1.106.gf69932adfe

[PATCHv6 15/31] merge-recursive: make !o->detect_rename codepath more obvious

[Elijah Newren]

Previously, if !o->detect_rename then get_renames() would return an
empty string_list, and then process_renames() would have nothing to
iterate over.  It seems more straightforward to simply avoid calling
either function in that case.

Signed-off-by: Elijah Newren <new...@gmail.com>
---
 merge-recursive.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index cdd0afa04..da7c67eb8 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1329,8 +1329,6 @@ static struct string_list *get_renames(struct 
merge_options *o,
struct diff_options opts;
 
renames = xcalloc(1, sizeof(struct string_list));
-       if (!o->detect_rename)
-   return renames;
 
diff_setup();
opts.flags.recursive = 1;
@@ -1648,6 +1646,12 @@ static int handle_renames(struct merge_options *o,
  struct string_list *entries,
  struct rename_info *ri)
 {
+   ri->head_renames = NULL;
+   ri->merge_renames = NULL;
+
+   if (!o->detect_rename)
+   return 1;
+
ri->head_renames  = get_renames(o, head, common, head, merge, entries);
ri->merge_renames = get_renames(o, merge, common, head, merge, entries);
    return process_renames(o, ri->head_renames, ri->merge_renames);
@@ -1658,6 +1662,9 @@ static void cleanup_rename(struct string_list *rename)
const struct rename *re;
int i;
 
+   if (rename == NULL)
+   return;
+
for (i = 0; i < rename->nr; i++) {
re = rename->items[i].util;
diff_free_filepair(re->pair);
-- 
2.14.2

[PATCH v5 18/34] merge-recursive: make !o->detect_rename codepath more obvious

[Elijah Newren]

Previously, if !o->detect_rename then get_renames() would return an
empty string_list, and then process_renames() would have nothing to
iterate over.  It seems more straightforward to simply avoid calling
either function in that case.

Signed-off-by: Elijah Newren <new...@gmail.com>
---
 merge-recursive.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index cdd0afa04..da7c67eb8 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1329,8 +1329,6 @@ static struct string_list *get_renames(struct 
merge_options *o,
struct diff_options opts;
 
renames = xcalloc(1, sizeof(struct string_list));
-       if (!o->detect_rename)
-   return renames;
 
diff_setup();
opts.flags.recursive = 1;
@@ -1648,6 +1646,12 @@ static int handle_renames(struct merge_options *o,
  struct string_list *entries,
  struct rename_info *ri)
 {
+   ri->head_renames = NULL;
+   ri->merge_renames = NULL;
+
+   if (!o->detect_rename)
+   return 1;
+
ri->head_renames  = get_renames(o, head, common, head, merge, entries);
ri->merge_renames = get_renames(o, merge, common, head, merge, entries);
    return process_renames(o, ri->head_renames, ri->merge_renames);
@@ -1658,6 +1662,9 @@ static void cleanup_rename(struct string_list *rename)
const struct rename *re;
int i;
 
+   if (rename == NULL)
+   return;
+
for (i = 0; i < rename->nr; i++) {
re = rename->items[i].util;
diff_free_filepair(re->pair);
-- 
2.15.0.408.g8e199d483

[PATCH v4 18/34] merge-recursive: make !o->detect_rename codepath more obvious

[Elijah Newren]

Previously, if !o->detect_rename then get_renames() would return an
empty string_list, and then process_renames() would have nothing to
iterate over.  It seems more straightforward to simply avoid calling
either function in that case.

Signed-off-by: Elijah Newren <new...@gmail.com>
---
 merge-recursive.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index cdd0afa047..da7c67eb82 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1329,8 +1329,6 @@ static struct string_list *get_renames(struct 
merge_options *o,
struct diff_options opts;
 
renames = xcalloc(1, sizeof(struct string_list));
-       if (!o->detect_rename)
-   return renames;
 
diff_setup();
opts.flags.recursive = 1;
@@ -1648,6 +1646,12 @@ static int handle_renames(struct merge_options *o,
  struct string_list *entries,
  struct rename_info *ri)
 {
+   ri->head_renames = NULL;
+   ri->merge_renames = NULL;
+
+   if (!o->detect_rename)
+   return 1;
+
ri->head_renames  = get_renames(o, head, common, head, merge, entries);
ri->merge_renames = get_renames(o, merge, common, head, merge, entries);
    return process_renames(o, ri->head_renames, ri->merge_renames);
@@ -1658,6 +1662,9 @@ static void cleanup_rename(struct string_list *rename)
const struct rename *re;
int i;
 
+   if (rename == NULL)
+   return;
+
for (i = 0; i < rename->nr; i++) {
re = rename->items[i].util;
diff_free_filepair(re->pair);
-- 
2.15.0.408.g850bc54b15

[PATCH v3 18/33] merge-recursive: make !o->detect_rename codepath more obvious

[Elijah Newren]

Previously, if !o->detect_rename then get_renames() would return an
empty string_list, and then process_renames() would have nothing to
iterate over.  It seems more straightforward to simply avoid calling
either function in that case.

Signed-off-by: Elijah Newren <new...@gmail.com>
---
 merge-recursive.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index db8590ab1a..7f975f3f52 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1329,8 +1329,6 @@ static struct string_list *get_renames(struct 
merge_options *o,
struct diff_options opts;
 
renames = xcalloc(1, sizeof(struct string_list));
-       if (!o->detect_rename)
-   return renames;
 
diff_setup();
opts.flags.recursive = 1;
@@ -1648,6 +1646,12 @@ static int handle_renames(struct merge_options *o,
  struct string_list *entries,
  struct rename_info *ri)
 {
+   ri->head_renames = NULL;
+   ri->merge_renames = NULL;
+
+   if (!o->detect_rename)
+   return 1;
+
ri->head_renames  = get_renames(o, head, common, head, merge, entries);
ri->merge_renames = get_renames(o, merge, common, head, merge, entries);
    return process_renames(o, ri->head_renames, ri->merge_renames);
@@ -1658,6 +1662,9 @@ static void cleanup_rename(struct string_list *rename)
const struct rename *re;
int i;
 
+   if (rename == NULL)
+   return;
+
for (i = 0; i < rename->nr; i++) {
re = rename->items[i].util;
diff_free_filepair(re->pair);
-- 
2.15.0.309.g62ce55426d

[PATCH v2 18/33] merge-recursive: make !o->detect_rename codepath more obvious

[Elijah Newren]

Previously, if !o->detect_rename then get_renames() would return an
empty string_list, and then process_renames() would have nothing to
iterate over.  It seems more straightforward to simply avoid calling
either function in that case.

Signed-off-by: Elijah Newren <new...@gmail.com>
---
 merge-recursive.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index db8590ab1a..be04e61c10 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1329,8 +1329,6 @@ static struct string_list *get_renames(struct 
merge_options *o,
struct diff_options opts;
 
renames = xcalloc(1, sizeof(struct string_list));
-       if (!o->detect_rename)
-   return renames;
 
diff_setup();
opts.flags.recursive = 1;
@@ -1648,6 +1646,12 @@ static int handle_renames(struct merge_options *o,
  struct string_list *entries,
  struct rename_info *ri)
 {
+   if (!o->detect_rename) {
+   ri->head_renames = NULL;
+   ri->merge_renames = NULL;
+   return 1;
+   }
+
ri->head_renames  = get_renames(o, head, common, head, merge, entries);
ri->merge_renames = get_renames(o, merge, common, head, merge, entries);
    return process_renames(o, ri->head_renames, ri->merge_renames);
@@ -1658,6 +1662,9 @@ static void cleanup_rename(struct string_list *rename)
const struct rename *re;
int i;
 
+   if (rename == NULL)
+   return;
+
for (i = 0; i < rename->nr; i++) {
re = rename->items[i].util;
diff_free_filepair(re->pair);
-- 
2.15.0.309.g00c152f825

git archive --remote should generate tar.gz format indicated by -o filename

[git-scm]

git archive -o name.tar.gz generates a gzipped file without needing an
explicit --format switch.

However, git archive -o name.tar.gz --remote [url] generates a tar
file, which is unexpected, bandwidth-heavier, and additionally in some
cases it's not immediately obvious that this has happened.

git archive -o name.tar.gz --remote [url] --format tar.gz generates a
gzipped file, so there's obviously no limitation with e.g. git-upload-
archive.

Given the above, either git archive or git-upload-archive should apply
the same tar.gz filename heuristic and generate the expected format.

Presumably e.g. tar.xz support when using --remote would be more
problematic since, in the local case, it involves specifying an
arbitrary command.

[PATCH 18/30] merge-recursive: Make !o->detect_rename codepath more obvious

[Elijah Newren]

Previously, if !o->detect_rename then get_renames() would return an
empty string_list, and then process_renames() would have nothing to
iterate over.  It seems more straightforward to simply avoid calling
either function in that case.

Signed-off-by: Elijah Newren <new...@gmail.com>
---
 merge-recursive.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 7a3402e50c..f40c70990c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1332,8 +1332,6 @@ static struct string_list *get_renames(struct 
merge_options *o,
struct diff_options opts;
 
renames = xcalloc(1, sizeof(struct string_list));
-       if (!o->detect_rename)
-   return renames;
 
diff_setup();
DIFF_OPT_SET(, RECURSIVE);
@@ -1652,6 +1650,10 @@ static struct rename_info *handle_renames(struct 
merge_options *o,
 {
struct rename_info *rei = xcalloc(1, sizeof(struct rename_info));
 
+   *clean = 1;
+   if (!o->detect_rename)
+   return NULL;
+
rei->head_renames  = get_renames(o, head, common, head, merge, entries);
rei->merge_renames = get_renames(o, merge, common, head, merge, 
entries);
*clean = process_renames(o, rei->head_renames, rei->merge_renames);
@@ -1664,6 +1666,9 @@ static void cleanup_renames(struct rename_info *re_info)
const struct rename *re;
int i;
 
+   if (!re_info)
+   return;
+
for (i = 0; i < re_info->head_renames->nr; i++) {
re = re_info->head_renames->items[i].util;
diff_free_filepair(re->pair);
-- 
2.15.0.5.g9567be9905

[RFC 4/7] t: fix ssh tests to cope with using '-o SendEnv=GIT_PROTOCOL'

[Brandon Williams]

Update some of our tests to cope with ssh being launched with the option
to send the protocol version.

Signed-off-by: Brandon Williams <bmw...@google.com>
---
 t/lib-proto-disable.sh   |  1 +
 t/t5601-clone.sh | 10 +-
 t/t5602-clone-remote-exec.sh |  4 ++--
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index 83babe57d..d19c88f96 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -194,6 +194,7 @@ setup_ssh_wrapper () {
test_expect_success 'setup ssh wrapper' '
write_script ssh-wrapper <<-\EOF &&
echo >&2 "ssh: $*"
+   shift; shift
host=$1; shift
cd "$TRASH_DIRECTORY/$host" &&
eval "$*"
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9c56f771b..7e65013c5 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -332,13 +332,13 @@ expect_ssh () {
1)
;;
2)
-   echo "ssh: $1 git-upload-pack '$2'"
+   echo "ssh: -o SendEnv=GIT_PROTOCOL $1 git-upload-pack 
'$2'"
;;
3)
-   echo "ssh: $1 $2 git-upload-pack '$3'"
+   echo "ssh: -o SendEnv=GIT_PROTOCOL $1 $2 
git-upload-pack '$3'"
;;
*)
-   echo "ssh: $1 $2 git-upload-pack '$3' $4"
+   echo "ssh: $1 -o SendEnv=GIT_PROTOCOL $2 $3 
git-upload-pack '$4'"
esac
} >"$TRASH_DIRECTORY/ssh-expect" &&
(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
@@ -390,7 +390,7 @@ test_expect_success 'double quoted plink.exe in 
GIT_SSH_COMMAND' '
copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
GIT_SSH_COMMAND="\"$TRASH_DIRECTORY/plink.exe\" -v" \
git clone "[myhost:123]:src" ssh-bracket-clone-plink-3 &&
-   expect_ssh "-v -P 123" myhost src
+   expect_ssh "-v" "-P 123" myhost src
 '
 
 SQ="'"
@@ -398,7 +398,7 @@ test_expect_success 'single quoted plink.exe in 
GIT_SSH_COMMAND' '
copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
GIT_SSH_COMMAND="$SQ$TRASH_DIRECTORY/plink.exe$SQ -v" \
git clone "[myhost:123]:src" ssh-bracket-clone-plink-4 &&
-   expect_ssh "-v -P 123" myhost src
+   expect_ssh "-v" "-P 123" myhost src
 '
 
 test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
diff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh
index cbcceab9d..b0d80cadd 100755
--- a/t/t5602-clone-remote-exec.sh
+++ b/t/t5602-clone-remote-exec.sh
@@ -13,14 +13,14 @@ test_expect_success setup '
 
 test_expect_success 'clone calls git upload-pack unqualified with no -u 
option' '
test_must_fail env GIT_SSH=./not_ssh git clone localhost:/path/to/repo 
junk &&
-   echo "localhost git-upload-pack '\''/path/to/repo'\''" >expected &&
+   echo "-o SendEnv=GIT_PROTOCOL localhost git-upload-pack 
'\''/path/to/repo'\''" >expected &&
test_cmp expected not_ssh_output
 '
 
 test_expect_success 'clone calls specified git upload-pack with -u option' '
test_must_fail env GIT_SSH=./not_ssh \
git clone -u ./something/bin/git-upload-pack 
localhost:/path/to/repo junk &&
-   echo "localhost ./something/bin/git-upload-pack '\''/path/to/repo'\''" 
>expected &&
+   echo "-o SendEnv=GIT_PROTOCOL localhost ./something/bin/git-upload-pack 
'\''/path/to/repo'\''" >expected &&
test_cmp expected not_ssh_output
 '
 
-- 
2.14.1.342.g6490525c54-goog

Re: git send-email (w/o Cc: stable)

[Borislav Petkov]

On Thu, Jun 29, 2017 at 03:11:37PM -0700, Luck, Tony wrote:
> So there is a "--cc-cmd" option that can do the same as those "-cc" arguments.
> Combine that with --suppress-cc=bodycc and things get a bit more automated.

Yeah, whatever works for you.

I did play with cc-cmd somewhat but can't be bothered to generate the CC
list per hand each time.

I'd prefer if that switch:

--suppress-cc=

had the obvious  of single email address too:

--suppress-cc=sta...@vger.kernel.org

so that we can send patches and unconditionally suppress only that
single recipient from the CC list.

And maybe there is a way...

Let me CC the git ML.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH] git-p4: changelist template with p4 -G change -o

[miguel torroja]

Hi Lars/Luke,

I tried a first test extending t9807-git-p4-submit.sh. I set this p4
trigger: 'p4test command pre-user-change "echo verbose trigger" '. I'm
able to reproduce the issue I wanted to fix. However I found yet
another issue, in this case when reading the result from
p4_read_pipe_lines (in function p4ChangesForPaths), the
pre-user-change is triggered with any "p4 change" and "p4 changes"
command (the p4 server we have in production, only shows "extra"
messages with p4 change).
   I'll collapse in one single commit the fix for p4 change/p4 changes
and the new test.

Thanks,

Miguel

On Sat, Jun 24, 2017 at 10:37 PM, miguel torroja
<miguel.torr...@gmail.com> wrote:
> Hi Lars,
>
> I think it's doable to set a custom p4 trigger, created by the test case,
> that outputs "extra info" when requesting a changelist description.
> I'll do a specific test and post it to this thread.
>
>
> Thanks,
>
>
> El 24 jun. 2017 7:36 p. m., "Lars Schneider" <larsxschnei...@gmail.com>
> escribió:
>
>
>> On 24 Jun 2017, at 13:49, Luke Diamand <l...@diamand.org> wrote:
>>
>> On 22 June 2017 at 18:32, Junio C Hamano <gits...@pobox.com> wrote:
>>> Miguel Torroja <miguel.torr...@gmail.com> writes:
>>>
>>>> The option -G of p4 (python marshal output) gives more context about the
>>>> data being output. That's useful when using the command "change -o" as
>>>> we can distinguish between warning/error line and real change
>>>> description.
>>>>
>>>> Some p4 plugin/hooks in the server side generates some warnings when
>>>> executed. Unfortunately those messages are mixed with the output of
>>>> "p4 change -o". Those extra warning lines are reported as
>>>> {'code':'info'}
>>>> in python marshal output (-G). The real change output is reported as
>>>> {'code':'stat'}
>>
>> I think this seems like a reasonable thing to do if "p4 change -o" is
>> jumbling up output.
>>
>> One thing I notice trying it out by hand is that we seem to have lost
>> the annotation of the Perforce per-file modification type (is there a
>> proper name for this?).
>>
>> For example, if I add a file called "baz", then the original version
>> creates a template which looks like this:
>>
>>   //depot/baz# add
>>
>> But the new one creates a template which looks like:
>>
>>   //depot/baz
>
> @Miguel: You wrote that p4 plugins/hooks generate these warnings.
> I wonder if you see a way to replicate that in a test case. Either
> in t9800 or a new t98XX test case file?
>
> - Lars
>
>

Re: [PATCH] git-p4: changelist template with p4 -G change -o

[Lars Schneider]

> On 24 Jun 2017, at 13:49, Luke Diamand <l...@diamand.org> wrote:
> 
> On 22 June 2017 at 18:32, Junio C Hamano <gits...@pobox.com> wrote:
>> Miguel Torroja <miguel.torr...@gmail.com> writes:
>> 
>>> The option -G of p4 (python marshal output) gives more context about the
>>> data being output. That's useful when using the command "change -o" as
>>> we can distinguish between warning/error line and real change description.
>>> 
>>> Some p4 plugin/hooks in the server side generates some warnings when
>>> executed. Unfortunately those messages are mixed with the output of
>>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>>> in python marshal output (-G). The real change output is reported as
>>> {'code':'stat'}
> 
> I think this seems like a reasonable thing to do if "p4 change -o" is
> jumbling up output.
> 
> One thing I notice trying it out by hand is that we seem to have lost
> the annotation of the Perforce per-file modification type (is there a
> proper name for this?).
> 
> For example, if I add a file called "baz", then the original version
> creates a template which looks like this:
> 
>   //depot/baz# add
> 
> But the new one creates a template which looks like:
> 
>   //depot/baz

@Miguel: You wrote that p4 plugins/hooks generate these warnings.
I wonder if you see a way to replicate that in a test case. Either
in t9800 or a new t98XX test case file?

- Lars

Re: [PATCH] git-p4: changelist template with p4 -G change -o

[miguel torroja]

You are right about the "# add"comment. I couldn't find any extra info
in the marshaled output that I can use to add the change action
comment after the path. That's one downside of that change.

On Sat, Jun 24, 2017 at 1:49 PM, Luke Diamand <l...@diamand.org> wrote:
> On 22 June 2017 at 18:32, Junio C Hamano <gits...@pobox.com> wrote:
>> Miguel Torroja <miguel.torr...@gmail.com> writes:
>>
>>> The option -G of p4 (python marshal output) gives more context about the
>>> data being output. That's useful when using the command "change -o" as
>>> we can distinguish between warning/error line and real change description.
>>>
>>> Some p4 plugin/hooks in the server side generates some warnings when
>>> executed. Unfortunately those messages are mixed with the output of
>>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>>> in python marshal output (-G). The real change output is reported as
>>> {'code':'stat'}
>
> I think this seems like a reasonable thing to do if "p4 change -o" is
> jumbling up output.
>
> One thing I notice trying it out by hand is that we seem to have lost
> the annotation of the Perforce per-file modification type (is there a
> proper name for this?).
>
> For example, if I add a file called "baz", then the original version
> creates a template which looks like this:
>
>//depot/baz# add
>
> But the new one creates a template which looks like:
>
>//depot/baz
>
> Luke

Re: [PATCH] git-p4: changelist template with p4 -G change -o

[Luke Diamand]

On 22 June 2017 at 18:32, Junio C Hamano <gits...@pobox.com> wrote:
> Miguel Torroja <miguel.torr...@gmail.com> writes:
>
>> The option -G of p4 (python marshal output) gives more context about the
>> data being output. That's useful when using the command "change -o" as
>> we can distinguish between warning/error line and real change description.
>>
>> Some p4 plugin/hooks in the server side generates some warnings when
>> executed. Unfortunately those messages are mixed with the output of
>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>> in python marshal output (-G). The real change output is reported as
>> {'code':'stat'}

I think this seems like a reasonable thing to do if "p4 change -o" is
jumbling up output.

One thing I notice trying it out by hand is that we seem to have lost
the annotation of the Perforce per-file modification type (is there a
proper name for this?).

For example, if I add a file called "baz", then the original version
creates a template which looks like this:

   //depot/baz# add

But the new one creates a template which looks like:

   //depot/baz

Luke

Re: [PATCH] git-p4: changelist template with p4 -G change -o

[Junio C Hamano]

Miguel Torroja <miguel.torr...@gmail.com> writes:

> The option -G of p4 (python marshal output) gives more context about the
> data being output. That's useful when using the command "change -o" as
> we can distinguish between warning/error line and real change description.
>
> Some p4 plugin/hooks in the server side generates some warnings when
> executed. Unfortunately those messages are mixed with the output of
> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
> in python marshal output (-G). The real change output is reported as
> {'code':'stat'}
>
> Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
> ---

Asking for help reviewing to those who have worked on git-p4 in the
past...

Thanks.

>  git-p4.py | 77 
> ++-
>  1 file changed, 51 insertions(+), 26 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 8d151da..a300474 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1526,37 +1526,62 @@ class P4Submit(Command, P4UserMap):
>  
>  [upstream, settings] = findUpstreamBranchPoint()
>  
> -template = ""
> +template = """\
> +# A Perforce Change Specification.
> +#
> +#  Change:  The change number. 'new' on a new changelist.
> +#  Date:The date this specification was last modified.
> +#  Client:  The client on which the changelist was created.  Read-only.
> +#  User:The user who created the changelist.
> +#  Status:  Either 'pending' or 'submitted'. Read-only.
> +#  Type:Either 'public' or 'restricted'. Default is 'public'.
> +#  Description: Comments about the changelist.  Required.
> +#  Jobs:What opened jobs are to be closed by this changelist.
> +#   You may delete jobs from this list.  (New changelists only.)
> +#  Files:   What opened files from the default changelist are to be added
> +#   to this changelist.  You may delete files from this list.
> +#   (New changelists only.)
> +"""
> +files_list = []
>  inFilesSection = False
> +change_entry = None
>  args = ['change', '-o']
>  if changelist:
>  args.append(str(changelist))
> -
> -for line in p4_read_pipe_lines(args):
> -if line.endswith("\r\n"):
> -line = line[:-2] + "\n"
> -if inFilesSection:
> -if line.startswith("\t"):
> -# path starts and ends with a tab
> -path = line[1:]
> -lastTab = path.rfind("\t")
> -if lastTab != -1:
> -path = path[:lastTab]
> -if settings.has_key('depot-paths'):
> -if not [p for p in settings['depot-paths']
> -if p4PathStartsWith(path, p)]:
> -continue
> -else:
> -if not p4PathStartsWith(path, self.depotPath):
> -continue
> +for entry in p4CmdList(args):
> +if not entry.has_key('code'):
> +continue
> +if entry['code'] == 'stat':
> +change_entry = entry
> +break
> +if not change_entry:
> +die('Failed to decode output of p4 change -o')
> +for key, value in change_entry.iteritems():
> +if key.startswith('File'):
> +if settings.has_key('depot-paths'):
> +if not [p for p in settings['depot-paths']
> +if p4PathStartsWith(value, p)]:
> +continue
>  else:
> -inFilesSection = False
> -else:
> -if line.startswith("Files:"):
> -inFilesSection = True
> -
> -template += line
> -
> +if not p4PathStartsWith(value, self.depotPath):
> +continue
> +files_list.append(value)
> +continue
> +# Output in the order expected by prepareLogMessage
> +for key in ['Change','Client','User','Status','Description','Jobs']:
> +if not change_entry.has_key(key):
> +continue
> +template += '\n'
> +template += key + ':'
> +if key == 'Description':
> +template += '\n'
> +for field_line in change_entry[key].splitlines():
> +template += '\t'+field_line+'\n'
> +if len(files_list) > 0:
> +template += '\n'
> +template += 'Files:\n'
> +for path in files_list:
> +template += '\t'+path+'\n'
>  return template
>  
>  def edit_template(self, template_file):

[PATCH] git-p4: changelist template with p4 -G change -o

[Miguel Torroja]

The option -G of p4 (python marshal output) gives more context about the
data being output. That's useful when using the command "change -o" as
we can distinguish between warning/error line and real change description.

Some p4 plugin/hooks in the server side generates some warnings when
executed. Unfortunately those messages are mixed with the output of
"p4 change -o". Those extra warning lines are reported as {'code':'info'}
in python marshal output (-G). The real change output is reported as
{'code':'stat'}

Signed-off-by: Miguel Torroja <miguel.torr...@gmail.com>
---
 git-p4.py | 77 ++-
 1 file changed, 51 insertions(+), 26 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 8d151da..a300474 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1526,37 +1526,62 @@ class P4Submit(Command, P4UserMap):
 
 [upstream, settings] = findUpstreamBranchPoint()
 
-template = ""
+template = """\
+# A Perforce Change Specification.
+#
+#  Change:  The change number. 'new' on a new changelist.
+#  Date:The date this specification was last modified.
+#  Client:  The client on which the changelist was created.  Read-only.
+#  User:The user who created the changelist.
+#  Status:  Either 'pending' or 'submitted'. Read-only.
+#  Type:Either 'public' or 'restricted'. Default is 'public'.
+#  Description: Comments about the changelist.  Required.
+#  Jobs:What opened jobs are to be closed by this changelist.
+#   You may delete jobs from this list.  (New changelists only.)
+#  Files:   What opened files from the default changelist are to be added
+#   to this changelist.  You may delete files from this list.
+#   (New changelists only.)
+"""
+files_list = []
 inFilesSection = False
+change_entry = None
 args = ['change', '-o']
 if changelist:
 args.append(str(changelist))
-
-for line in p4_read_pipe_lines(args):
-if line.endswith("\r\n"):
-line = line[:-2] + "\n"
-if inFilesSection:
-if line.startswith("\t"):
-# path starts and ends with a tab
-path = line[1:]
-lastTab = path.rfind("\t")
-if lastTab != -1:
-path = path[:lastTab]
-if settings.has_key('depot-paths'):
-if not [p for p in settings['depot-paths']
-if p4PathStartsWith(path, p)]:
-continue
-else:
-if not p4PathStartsWith(path, self.depotPath):
-continue
+for entry in p4CmdList(args):
+if not entry.has_key('code'):
+continue
+if entry['code'] == 'stat':
+change_entry = entry
+    break
+if not change_entry:
+die('Failed to decode output of p4 change -o')
+for key, value in change_entry.iteritems():
+if key.startswith('File'):
+if settings.has_key('depot-paths'):
+if not [p for p in settings['depot-paths']
+if p4PathStartsWith(value, p)]:
+continue
 else:
-inFilesSection = False
-else:
-if line.startswith("Files:"):
-inFilesSection = True
-
-template += line
-
+if not p4PathStartsWith(value, self.depotPath):
+continue
+files_list.append(value)
+continue
+# Output in the order expected by prepareLogMessage
+for key in ['Change','Client','User','Status','Description','Jobs']:
+if not change_entry.has_key(key):
+continue
+template += '\n'
+template += key + ':'
+if key == 'Description':
+template += '\n'
+for field_line in change_entry[key].splitlines():
+template += '\t'+field_line+'\n'
+if len(files_list) > 0:
+template += '\n'
+template += 'Files:\n'
+for path in files_list:
+template += '\t'+path+'\n'
 return template
 
 def edit_template(self, template_file):
-- 
2.1.4

Re: [PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

[René Scharfe]

Am 29.03.2017 um 06:54 schrieb Christian Couder:

On Tue, Mar 28, 2017 at 11:49 PM, Jeff King  wrote:

On Tue, Mar 28, 2017 at 11:15:12PM +0200, Christian Couder wrote:


On Sun, Mar 26, 2017 at 3:43 PM, René Scharfe  wrote:

FreeBSD implements getcwd(3) as a syscall, but falls back to a version
based on readdir(3) if it fails for some reason.  The latter requires
permissions to read and execute path components, while the former does
not.  That means that if our buffer is too small and we're missing
rights we could get EACCES, but we may succeed with a bigger buffer.

Keep retrying if getcwd(3) indicates lack of permissions until our
buffer can fit PATH_MAX bytes, as that's the maximum supported by the
syscall on FreeBSD anyway.  This way we do what we can to be able to
benefit from the syscall, but we also won't loop forever if there is a
real permission issue.


Sorry to be late and maybe I missed something obvious, but the above
and the patch seem complex to me compared with something like:

diff --git a/strbuf.c b/strbuf.c
index ace58e7367..25eadcbedc 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -441,7 +441,7 @@ int strbuf_readlink(struct strbuf *sb, const char
*path, size_t hint)
 int strbuf_getcwd(struct strbuf *sb)
 {
size_t oldalloc = sb->alloc;
-   size_t guessed_len = 128;
+   size_t guessed_len = PATH_MAX > 128 ? PATH_MAX : 128;

for (;; guessed_len *= 2) {
strbuf_grow(sb, guessed_len);


I think the main reason is just that we do not have to pay the price to
allocate PATH_MAX-sized buffers when they are rarely used.

I doubt it matters all that much in practice, though.


Yeah, I can understand that.


Right, using PATH_MAX as initial size (no ?: operator needed) is the 
easiest fix.  And yes, I wanted to avoid wasting memory just to fix an 
odd special case.


Given that the function isn't called very often the impact is probably 
not that high either way, but increasing memory usage by 3200% on Linux 
just didn't sit right with me -- even though 4KB isn't much in absolute 
terms, of course.



But I also wonder if René's patch relies on PATH_MAX being a multiple
of 128 on FreeBSD and what would happen for a path between 129 and
PATH_MAX if PATH_MAX was something like 254.


We can use any value bigger than zero to initialize guessed_len. 
getcwd() won't have any problems fitting e.g. a path with a length of 
130 bytes into a buffer of 256 bytes in the second round of the loop.


René

Re: [PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

[Christian Couder]

On Tue, Mar 28, 2017 at 11:49 PM, Jeff King  wrote:
> On Tue, Mar 28, 2017 at 11:15:12PM +0200, Christian Couder wrote:
>
>> On Sun, Mar 26, 2017 at 3:43 PM, René Scharfe  wrote:
>> > FreeBSD implements getcwd(3) as a syscall, but falls back to a version
>> > based on readdir(3) if it fails for some reason.  The latter requires
>> > permissions to read and execute path components, while the former does
>> > not.  That means that if our buffer is too small and we're missing
>> > rights we could get EACCES, but we may succeed with a bigger buffer.
>> >
>> > Keep retrying if getcwd(3) indicates lack of permissions until our
>> > buffer can fit PATH_MAX bytes, as that's the maximum supported by the
>> > syscall on FreeBSD anyway.  This way we do what we can to be able to
>> > benefit from the syscall, but we also won't loop forever if there is a
>> > real permission issue.
>>
>> Sorry to be late and maybe I missed something obvious, but the above
>> and the patch seem complex to me compared with something like:
>>
>> diff --git a/strbuf.c b/strbuf.c
>> index ace58e7367..25eadcbedc 100644
>> --- a/strbuf.c
>> +++ b/strbuf.c
>> @@ -441,7 +441,7 @@ int strbuf_readlink(struct strbuf *sb, const char
>> *path, size_t hint)
>>  int strbuf_getcwd(struct strbuf *sb)
>>  {
>> size_t oldalloc = sb->alloc;
>> -   size_t guessed_len = 128;
>> +   size_t guessed_len = PATH_MAX > 128 ? PATH_MAX : 128;
>>
>> for (;; guessed_len *= 2) {
>> strbuf_grow(sb, guessed_len);
>
> I think the main reason is just that we do not have to pay the price to
> allocate PATH_MAX-sized buffers when they are rarely used.
>
> I doubt it matters all that much in practice, though.

Yeah, I can understand that.

But I also wonder if René's patch relies on PATH_MAX being a multiple
of 128 on FreeBSD and what would happen for a path between 129 and
PATH_MAX if PATH_MAX was something like 254.

Re: [PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

[Jeff King]

On Tue, Mar 28, 2017 at 11:15:12PM +0200, Christian Couder wrote:

> On Sun, Mar 26, 2017 at 3:43 PM, René Scharfe  wrote:
> > FreeBSD implements getcwd(3) as a syscall, but falls back to a version
> > based on readdir(3) if it fails for some reason.  The latter requires
> > permissions to read and execute path components, while the former does
> > not.  That means that if our buffer is too small and we're missing
> > rights we could get EACCES, but we may succeed with a bigger buffer.
> >
> > Keep retrying if getcwd(3) indicates lack of permissions until our
> > buffer can fit PATH_MAX bytes, as that's the maximum supported by the
> > syscall on FreeBSD anyway.  This way we do what we can to be able to
> > benefit from the syscall, but we also won't loop forever if there is a
> > real permission issue.
> 
> Sorry to be late and maybe I missed something obvious, but the above
> and the patch seem complex to me compared with something like:
> 
> diff --git a/strbuf.c b/strbuf.c
> index ace58e7367..25eadcbedc 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -441,7 +441,7 @@ int strbuf_readlink(struct strbuf *sb, const char
> *path, size_t hint)
>  int strbuf_getcwd(struct strbuf *sb)
>  {
> size_t oldalloc = sb->alloc;
> -   size_t guessed_len = 128;
> +   size_t guessed_len = PATH_MAX > 128 ? PATH_MAX : 128;
> 
> for (;; guessed_len *= 2) {
> strbuf_grow(sb, guessed_len);

I think the main reason is just that we do not have to pay the price to
allocate PATH_MAX-sized buffers when they are rarely used.

I doubt it matters all that much in practice, though.

-Peff

Re: [PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

[Christian Couder]

On Tue, Mar 28, 2017 at 11:24 PM, Stefan Beller  wrote:
> On Tue, Mar 28, 2017 at 2:15 PM, Christian Couder
>  wrote:
>> On Sun, Mar 26, 2017 at 3:43 PM, René Scharfe  wrote:
>>> FreeBSD implements getcwd(3) as a syscall, but falls back to a version
>>> based on readdir(3) if it fails for some reason.  The latter requires
>>> permissions to read and execute path components, while the former does
>>> not.  That means that if our buffer is too small and we're missing
>>> rights we could get EACCES, but we may succeed with a bigger buffer.
>>>
>>> Keep retrying if getcwd(3) indicates lack of permissions until our
>>> buffer can fit PATH_MAX bytes, as that's the maximum supported by the
>>> syscall on FreeBSD anyway.  This way we do what we can to be able to
>>> benefit from the syscall, but we also won't loop forever if there is a
>>> real permission issue.
>>
>> Sorry to be late and maybe I missed something obvious, but the above
>> and the patch seem complex to me compared with something like:
>>
>> diff --git a/strbuf.c b/strbuf.c
>> index ace58e7367..25eadcbedc 100644
>> --- a/strbuf.c
>> +++ b/strbuf.c
>> @@ -441,7 +441,7 @@ int strbuf_readlink(struct strbuf *sb, const char
>> *path, size_t hint)
>>  int strbuf_getcwd(struct strbuf *sb)
>>  {
>> size_t oldalloc = sb->alloc;
>> -   size_t guessed_len = 128;
>> +   size_t guessed_len = PATH_MAX > 128 ? PATH_MAX : 128;
>>
>> for (;; guessed_len *= 2) {
>> strbuf_grow(sb, guessed_len);
>
> From f22a76e911 (strbuf: add strbuf_getcwd(), 2014-07-28):
> Because it doesn't use a fixed-size buffer it supports
> arbitrarily long paths, provided the platform's getcwd() does as well.
> At least on Linux and FreeBSD it handles paths longer than PATH_MAX
> just fine.

Well René's patch says:

>>> Keep retrying if getcwd(3) indicates lack of permissions until our
>>> buffer can fit PATH_MAX bytes, as that's the maximum supported by the
>>> syscall on FreeBSD anyway.

So it says that FreeBSD syscall doesn't handle paths longer than PATH_MAX.

> So with your patch, we'd still see the original issue for paths > PATH_MAX
> IIUC.

Also, René's patch just adds:

+   if (errno == EACCES && guessed_len < PATH_MAX)
+   continue;

so if the length of the path is > PATH_MAX, then guessed_len will have
to be > PATH_MAX and then René's patch will do nothing.

Re: [PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

[Stefan Beller]

On Tue, Mar 28, 2017 at 2:15 PM, Christian Couder
 wrote:
> On Sun, Mar 26, 2017 at 3:43 PM, René Scharfe  wrote:
>> FreeBSD implements getcwd(3) as a syscall, but falls back to a version
>> based on readdir(3) if it fails for some reason.  The latter requires
>> permissions to read and execute path components, while the former does
>> not.  That means that if our buffer is too small and we're missing
>> rights we could get EACCES, but we may succeed with a bigger buffer.
>>
>> Keep retrying if getcwd(3) indicates lack of permissions until our
>> buffer can fit PATH_MAX bytes, as that's the maximum supported by the
>> syscall on FreeBSD anyway.  This way we do what we can to be able to
>> benefit from the syscall, but we also won't loop forever if there is a
>> real permission issue.
>
> Sorry to be late and maybe I missed something obvious, but the above
> and the patch seem complex to me compared with something like:
>
> diff --git a/strbuf.c b/strbuf.c
> index ace58e7367..25eadcbedc 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -441,7 +441,7 @@ int strbuf_readlink(struct strbuf *sb, const char
> *path, size_t hint)
>  int strbuf_getcwd(struct strbuf *sb)
>  {
> size_t oldalloc = sb->alloc;
> -   size_t guessed_len = 128;
> +   size_t guessed_len = PATH_MAX > 128 ? PATH_MAX : 128;
>
> for (;; guessed_len *= 2) {
> strbuf_grow(sb, guessed_len);

>From f22a76e911 (strbuf: add strbuf_getcwd(), 2014-07-28):
Because it doesn't use a fixed-size buffer it supports
arbitrarily long paths, provided the platform's getcwd() does as well.
At least on Linux and FreeBSD it handles paths longer than PATH_MAX
just fine.

So with your patch, we'd still see the original issue for paths > PATH_MAX
IIUC.

Thanks,
Stefan

Re: [PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

[Christian Couder]

On Sun, Mar 26, 2017 at 3:43 PM, René Scharfe  wrote:
> FreeBSD implements getcwd(3) as a syscall, but falls back to a version
> based on readdir(3) if it fails for some reason.  The latter requires
> permissions to read and execute path components, while the former does
> not.  That means that if our buffer is too small and we're missing
> rights we could get EACCES, but we may succeed with a bigger buffer.
>
> Keep retrying if getcwd(3) indicates lack of permissions until our
> buffer can fit PATH_MAX bytes, as that's the maximum supported by the
> syscall on FreeBSD anyway.  This way we do what we can to be able to
> benefit from the syscall, but we also won't loop forever if there is a
> real permission issue.

Sorry to be late and maybe I missed something obvious, but the above
and the patch seem complex to me compared with something like:

diff --git a/strbuf.c b/strbuf.c
index ace58e7367..25eadcbedc 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -441,7 +441,7 @@ int strbuf_readlink(struct strbuf *sb, const char
*path, size_t hint)
 int strbuf_getcwd(struct strbuf *sb)
 {
size_t oldalloc = sb->alloc;
-   size_t guessed_len = 128;
+   size_t guessed_len = PATH_MAX > 128 ? PATH_MAX : 128;

for (;; guessed_len *= 2) {
strbuf_grow(sb, guessed_len);

Re: [PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

[Junio C Hamano]

Zenobiusz Kunegunda <zenobiusz.kunegu...@interia.pl> writes:

> Od: "Junio C Hamano" <gits...@pobox.com>
> Do: "René Scharfe" <l@web.de>;
> Wysłane: 2:40 Poniedziałek 2017-03-27
> Temat: Re: [PATCH] strbuf: support long paths w/o read rights in 
> strbuf_getcwd() on FreeBSD
>
>> 
>> Nicely analysed and fixed (or is the right word "worked around"?)
>> 
>> Thanks, will queue.
>
> Is this patch going to be included in next git version ( or sooner ) by any 
> chance?

Absolutely.  Thanks for your initial report and sticking with us
during the session to identify the root cause that led to this
solution.

Again, René, thanks for your superb analysis and solution.

Re: [PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

[Zenobiusz Kunegunda]

Od: "Junio C Hamano" <gits...@pobox.com>
Do: "René Scharfe" <l@web.de>;
Wysłane: 2:40 Poniedziałek 2017-03-27
Temat: Re: [PATCH] strbuf: support long paths w/o read rights in 
strbuf_getcwd() on FreeBSD

> 
> Nicely analysed and fixed (or is the right word "worked around"?)
> 
> Thanks, will queue.
> 


Is this patch going to be included in next git version ( or sooner ) by any 
chance?

Thank you, everyone,  for your attention to the problem.

Re: [PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

[Junio C Hamano]

René Scharfe  writes:

> FreeBSD implements getcwd(3) as a syscall, but falls back to a version
> based on readdir(3) if it fails for some reason.  The latter requires
> permissions to read and execute path components, while the former does
> not.  That means that if our buffer is too small and we're missing
> rights we could get EACCES, but we may succeed with a bigger buffer.

WOW.  Just WOW.  Looking at the debugging exchange from the
sideline, I didn't expect this end result.

> Keep retrying if getcwd(3) indicates lack of permissions until our
> buffer can fit PATH_MAX bytes, as that's the maximum supported by the
> syscall on FreeBSD anyway.  This way we do what we can to be able to
> benefit from the syscall, but we also won't loop forever if there is a
> real permission issue.
>
> This fixes a regression introduced with 7333ed17 (setup: convert
> setup_git_directory_gently_1 et al. to strbuf, 2014-07-28) for paths
> longer than 127 bytes with components that miss read or execute
> permissions (e.g. 0711 on /home for privacy reasons); we used a fixed
> PATH_MAX-sized buffer before.
>
> Reported-by: Zenobiusz Kunegunda 
> Signed-off-by: Rene Scharfe 
> ---


Nicely analysed and fixed (or is the right word "worked around"?)

Thanks, will queue.

[PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

[René Scharfe]

FreeBSD implements getcwd(3) as a syscall, but falls back to a version
based on readdir(3) if it fails for some reason.  The latter requires
permissions to read and execute path components, while the former does
not.  That means that if our buffer is too small and we're missing
rights we could get EACCES, but we may succeed with a bigger buffer.

Keep retrying if getcwd(3) indicates lack of permissions until our
buffer can fit PATH_MAX bytes, as that's the maximum supported by the
syscall on FreeBSD anyway.  This way we do what we can to be able to
benefit from the syscall, but we also won't loop forever if there is a
real permission issue.

This fixes a regression introduced with 7333ed17 (setup: convert
setup_git_directory_gently_1 et al. to strbuf, 2014-07-28) for paths
longer than 127 bytes with components that miss read or execute
permissions (e.g. 0711 on /home for privacy reasons); we used a fixed
PATH_MAX-sized buffer before.

Reported-by: Zenobiusz Kunegunda 
Signed-off-by: Rene Scharfe 
---
 strbuf.c| 11 +++
 t/t0001-init.sh | 14 ++
 2 files changed, 25 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index ace58e7367..00457940cf 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -449,6 +449,17 @@ int strbuf_getcwd(struct strbuf *sb)
strbuf_setlen(sb, strlen(sb->buf));
return 0;
}
+
+   /*
+* If getcwd(3) is implemented as a syscall that falls
+* back to a regular lookup using readdir(3) etc. then
+* we may be able to avoid EACCES by providing enough
+* space to the syscall as it's not necessarily bound
+* to the same restrictions as the fallback.
+*/
+   if (errno == EACCES && guessed_len < PATH_MAX)
+   continue;
+
if (errno != ERANGE)
break;
}
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index e424de5363..5f81fbe07c 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -315,6 +315,20 @@ test_expect_success 'init with separate gitdir' '
test_path_is_dir realgitdir/refs
 '
 
+test_expect_success 'init in long base path' '
+   # exceed initial buffer size of strbuf_getcwd()
+   component=123456789abcdef &&
+   test_when_finished "chmod 0700 $component; rm -rf $component" &&
+   p31=$component/$component &&
+   p127=$p31/$p31/$p31/$p31 &&
+   mkdir -p $p127 &&
+   chmod 0111 $component &&
+   (
+   cd $p127 &&
+   git init newdir
+   )
+'
+
 test_expect_success 're-init on .git file' '
( cd newdir && git init )
 '
-- 
2.12.2

[PATCH v3 2/2] diff: document the format of the -O (diff.orderFile) file

[Richard Hansen]

Signed-off-by: Richard Hansen <hans...@google.com>
---
 Documentation/diff-config.txt  |  5 ++---
 Documentation/diff-options.txt | 34 --
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 875212045..9e4111320 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -99,11 +99,10 @@ diff.noprefix::
If set, 'git diff' does not show any source or destination prefix.
 
 diff.orderFile::
-   File indicating how to order files within a diff, using
-   one shell glob pattern per line.
+   File indicating how to order files within a diff.
+   See the '-O' option to linkgit:git-diff[1] for details.
If `diff.orderFile` is a relative pathname, it is treated as
relative to the top of the work tree.
-   Can be overridden by the '-O' option to linkgit:git-diff[1].
 
 diff.renameLimit::
The number of files to consider when performing the copy/rename
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e6215c372..d4fb70704 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -466,11 +466,41 @@ information.
 endif::git-format-patch[]
 
 -O::
-   Output the patch in the order specified in the
-   , which has one shell glob pattern per line.
+   Control the order in which files appear in the output.
This overrides the `diff.orderFile` configuration variable
(see linkgit:git-config[1]).  To cancel `diff.orderFile`,
use `-O/dev/null`.
++
+The output order is determined by the order of glob patterns in
+.
+All files with pathnames that match the first pattern are output
+first, all files with pathnames that match the second pattern (but not
+the first) are output next, and so on.
+All files with pathnames that do not match any pattern are output
+last, as if there was an implicit match-all pattern at the end of the
+file.
+If multiple pathnames have the same rank (they match the same pattern
+but no earlier patterns), their output order relative to each other is
+the normal order.
++
+ is parsed as follows:
++
+--
+ - Blank lines are ignored, so they can be used as separators for
+   readability.
+
+ - Lines starting with a hash ("`#`") are ignored, so they can be used
+   for comments.  Add a backslash ("`\`") to the beginning of the
+   pattern if it starts with a hash.
+
+ - Each other line contains a single pattern.
+--
++
+Patterns have the same syntax and semantics as patterns used for
+fnmantch(3) without the FNM_PATHNAME flag, except a pathname also
+matches a pattern if removing any number of the final pathname
+components matches the pattern.  For example, the pattern "`foo*bar`"
+matches "`fooasdfbar`" and "`foo/bar/baz/asdf`" but not "`foobarx`".
 
 ifndef::git-format-patch[]
 -R::
-- 
2.11.0.483.g087da7b7c-goog

[PATCH v2 2/2] diff: document the format of the -O (diff.orderFile) file

[Richard Hansen]

Signed-off-by: Richard Hansen <hans...@google.com>
---
 Documentation/diff-config.txt  |  5 ++--
 Documentation/diff-options.txt | 54 --
 2 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 875212045..9e4111320 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -99,11 +99,10 @@ diff.noprefix::
If set, 'git diff' does not show any source or destination prefix.
 
 diff.orderFile::
-   File indicating how to order files within a diff, using
-   one shell glob pattern per line.
+   File indicating how to order files within a diff.
+   See the '-O' option to linkgit:git-diff[1] for details.
If `diff.orderFile` is a relative pathname, it is treated as
relative to the top of the work tree.
-   Can be overridden by the '-O' option to linkgit:git-diff[1].
 
 diff.renameLimit::
The number of files to consider when performing the copy/rename
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e6215c372..e57e9f810 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -466,11 +466,61 @@ information.
 endif::git-format-patch[]
 
 -O::
-   Output the patch in the order specified in the
-   , which has one shell glob pattern per line.
+   Control the order in which files appear in the output.
This overrides the `diff.orderFile` configuration variable
(see linkgit:git-config[1]).  To cancel `diff.orderFile`,
use `-O/dev/null`.
++
+The output order is determined by the order of glob patterns in
+.
+All files with pathnames that match the first pattern are output
+first, all files with pathnames that match the second pattern (but not
+the first) are output next, and so on.
+All files with pathnames that do not match any pattern are output
+last, as if there was an implicit match-all pattern at the end of the
+file.
+If multiple pathnames have the same rank, their output order relative
+to each other is the normal order.
++
+ is parsed as follows:
++
+--
+ - Blank lines are ignored, so they can be used as separators for
+   readability.
+
+ - Lines starting with a hash ("`#`") are ignored, so they can be used
+   for comments.  Add a backslash ("`\`") to the beginning of the
+   pattern if it starts with a hash.
+
+ - Each other line contains a single pattern.
+--
++
+Patterns have the same syntax and semantics as patterns used for
+fnmantch(3) with the FNM_PATHNAME flag, except multiple consecutive
+unescaped asterisks (e.g., "`**`") have a special meaning:
++
+--
+ - A pattern beginning with "`**/`" means match in all directories.
+   For example, "`**/foo`" matches filename "`foo`" anywhere, and
+   "`**/foo/bar`" matches filename "`bar`" anywhere that is directly
+   under directory "`foo`".
+
+ - A pattern ending with "`/**`" matches everything inside a
+   directory, with infinite depth.  For example, "`abc/**`" matches
+   "`abc/def/ghi`" but not "`foo/abc/def`".
+
+ - A slash followed by two consecutive asterisks then a slash
+   ("`/**/`") matches zero or more directory components.  For example,
+   "`a/**/b`" matches "`a/b`", "`a/x/b`", "`a/x/y/b`" and so on.
+
+ - A pattern with more than one consecutive unescaped asterisk is
+   invalid.
+--
++
+In addition, a pathname matches a pattern if the pathname with any
+number of its final pathname components removed matches the pattern.
+For example, the pattern "`foo/*bar`" matches "`foo/asdfbar`" and
+"`foo/bar/baz`" but not "`foo/barx`".
 
 ifndef::git-format-patch[]
 -R::
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v5 13/14] mergetool: take the "-O" out of $orderfile

[Richard Hansen]

This will make it easier for a future commit to convert a relative
orderfile pathname to either absolute or relative to the top-level
directory.  It also improves code readability.

Signed-off-by: Richard Hansen <hans...@google.com>
---
 git-mergetool.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index e52b4e4f2..b506896dc 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -421,7 +421,7 @@ main () {
prompt=true
;;
    -O*)
-   orderfile="$1"
+   orderfile="${1#-O}"
;;
--)
shift
@@ -465,7 +465,7 @@ main () {
 
files=$(git -c core.quotePath=false \
diff --name-only --diff-filter=U \
-   ${orderfile:+"$orderfile"} -- "$@")
+   ${orderfile:+"-O$orderfile"} -- "$@")
 
cd_to_toplevel
 
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v4 13/14] mergetool: take the "-O" out of $orderfile

[Richard Hansen]

This will make it easier for a future commit to convert a relative
orderfile pathname to either absolute or relative to the top-level
directory.  It also improves code readability.

Signed-off-by: Richard Hansen <hans...@google.com>
---
 git-mergetool.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index e52b4e4f2..b506896dc 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -421,7 +421,7 @@ main () {
prompt=true
;;
    -O*)
-   orderfile="$1"
+   orderfile="${1#-O}"
;;
--)
shift
@@ -465,7 +465,7 @@ main () {
 
files=$(git -c core.quotePath=false \
diff --name-only --diff-filter=U \
-   ${orderfile:+"$orderfile"} -- "$@")
+   ${orderfile:+"-O$orderfile"} -- "$@")
 
cd_to_toplevel
 
-- 
2.11.0.390.gc69c2f50cf-goog



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v3 12/13] mergetool: take the "-O" out of $orderfile

[Richard Hansen]

This will make it easier for a future commit to convert a relative
orderfile pathname to either absolute or relative to the top-level
directory.  It also improves code readability.

Signed-off-by: Richard Hansen <hans...@google.com>
---
 git-mergetool.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index e52b4e4f2..b506896dc 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -421,7 +421,7 @@ main () {
prompt=true
;;
    -O*)
-   orderfile="$1"
+   orderfile="${1#-O}"
;;
--)
shift
@@ -465,7 +465,7 @@ main () {
 
files=$(git -c core.quotePath=false \
diff --name-only --diff-filter=U \
-   ${orderfile:+"$orderfile"} -- "$@")
+   ${orderfile:+"-O$orderfile"} -- "$@")
 
cd_to_toplevel
 
-- 
2.11.0.390.gc69c2f50cf-goog



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v4 4/4] mergetool: honor -O

[Junio C Hamano]

David Aguilar  writes:

>> I only see 4/4 in v4; am I correct to assume that 1-3/4 of v4 are
>> the same as their counterparts in v3?
>
> Yes, 1-3 are unchanged since v3.
> Thanks for checking,

I'll queue these four with Reviewed-by's from j6t.

Thanks, both.

Re: [PATCH v4 4/4] mergetool: honor -O

[David Aguilar]

On Mon, Oct 10, 2016 at 11:28:35AM -0700, Junio C Hamano wrote:
> David Aguilar <dav...@gmail.com> writes:
> 
> > Teach mergetool to pass "-O" down to `git diff` when
> > specified on the command-line.
> >
> > Helped-by: Johannes Sixt <j...@kdbg.org>
> > Signed-off-by: David Aguilar <dav...@gmail.com>
> > ---
> > Since v3:
> >
> > I missed one last piped invocation of "git mergetool" in the tests,
> > which has been fixed.
> 
> I only see 4/4 in v4; am I correct to assume that 1-3/4 of v4 are
> the same as their counterparts in v3?

Yes, 1-3 are unchanged since v3.
Thanks for checking,
-- 
David

Re: [PATCH v4 4/4] mergetool: honor -O

[Junio C Hamano]

David Aguilar <dav...@gmail.com> writes:

> Teach mergetool to pass "-O" down to `git diff` when
> specified on the command-line.
>
> Helped-by: Johannes Sixt <j...@kdbg.org>
> Signed-off-by: David Aguilar <dav...@gmail.com>
> ---
> Since v3:
>
> I missed one last piped invocation of "git mergetool" in the tests,
> which has been fixed.

I only see 4/4 in v4; am I correct to assume that 1-3/4 of v4 are
the same as their counterparts in v3?

Re: [PATCH v4 4/4] mergetool: honor -O

[Johannes Sixt]

With this final fixup, the series looks good to me, and I have no 
further comments.


Reviewed-by: Johannes Sixt 

-- Hannes

[PATCH v4 4/4] mergetool: honor -O

[David Aguilar]

Teach mergetool to pass "-O" down to `git diff` when
specified on the command-line.

Helped-by: Johannes Sixt <j...@kdbg.org>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
Since v3:

I missed one last piped invocation of "git mergetool" in the tests,
which has been fixed.

 Documentation/git-mergetool.txt | 10 ++
 git-mergetool.sh|  9 +++--
 t/t7610-mergetool.sh| 27 +++
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index c4c1a9b..3622d66 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -79,10 +79,12 @@ success of the resolution after the custom tool has exited.
Prompt before each invocation of the merge resolution program
to give the user a chance to skip the path.
 
-DIFF ORDER FILES
-
-`git mergetool` honors the `diff.orderFile` configuration variable
-used by `git diff`.  See linkgit:git-config[1] for more details.
+-O::
+   Process files in the order specified in the
+   , which has one shell glob pattern per line.
+   This overrides the `diff.orderFile` configuration variable
+   (see linkgit:git-config[1]).  To cancel `diff.orderFile`,
+   use `-O/dev/null`.
 
 TEMPORARY FILES
 ---
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 65696d8..e52b4e4 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] 
...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O] 
[file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -390,6 +390,7 @@ print_noop_and_exit () {
 main () {
prompt=$(git config --bool mergetool.prompt)
guessed_merge_tool=false
+   orderfile=
 
while test $# != 0
do
@@ -419,6 +420,9 @@ main () {
--prompt)
prompt=true
    ;;
+   -O*)
+   orderfile="$1"
+   ;;
--)
shift
break
@@ -460,7 +464,8 @@ main () {
fi
 
files=$(git -c core.quotePath=false \
-   diff --name-only --diff-filter=U -- "$@")
+   diff --name-only --diff-filter=U \
+   ${orderfile:+"$orderfile"} -- "$@")
 
cd_to_toplevel
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 38c1e4d..6d9f215 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -638,5 +638,32 @@ test_expect_success 'diff.orderFile configuration is 
honored' '
test_cmp expect actual &&
git reset --hard >/dev/null
 '
+test_expect_success 'mergetool -Oorder-file is honored' '
+   test_config diff.orderFile order-file &&
+   test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+   test_config mergetool.myecho.trustExitCode true &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   a
+   b
+   EOF
+   git mergetool -O/dev/null --no-prompt --tool myecho >output &&
+   git grep --no-index -h -A2 Merging: output >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1 &&
+
+   git config --unset diff.orderFile &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   b
+   a
+   EOF
+   git mergetool -Oorder-file --no-prompt --tool myecho >output &&
+   git grep --no-index -h -A2 Merging: output >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1
+'
 
 test_done
-- 
2.10.1.386.g8ee99a0

[PATCH v3 4/4] mergetool: honor -O

[David Aguilar]

Teach mergetool to pass "-O" down to `git diff` when
specified on the command-line.

Helped-by: Johannes Sixt <j...@kdbg.org>
Signed-off-by: David Aguilar <dav...@gmail.com>
---
Changes since v2:

The tests no longer rely on "grep -A" and instead use "git grep"
for portability.  The mergetool output in the test is redirected
to a file so that we can catch its exit code.

The $orderfile variable is now passed using ${xxx:+"$xxx"}
to avoid conditional logic.

 Documentation/git-mergetool.txt | 10 ++
 git-mergetool.sh|  9 +++--
 t/t7610-mergetool.sh| 27 +++
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index c4c1a9b..3622d66 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -79,10 +79,12 @@ success of the resolution after the custom tool has exited.
Prompt before each invocation of the merge resolution program
to give the user a chance to skip the path.
 
-DIFF ORDER FILES
-
-`git mergetool` honors the `diff.orderFile` configuration variable
-used by `git diff`.  See linkgit:git-config[1] for more details.
+-O::
+   Process files in the order specified in the
+   , which has one shell glob pattern per line.
+   This overrides the `diff.orderFile` configuration variable
+   (see linkgit:git-config[1]).  To cancel `diff.orderFile`,
+   use `-O/dev/null`.
 
 TEMPORARY FILES
 ---
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 65696d8..e52b4e4 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] 
...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O] 
[file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -390,6 +390,7 @@ print_noop_and_exit () {
 main () {
prompt=$(git config --bool mergetool.prompt)
guessed_merge_tool=false
+   orderfile=
 
while test $# != 0
do
@@ -419,6 +420,9 @@ main () {
--prompt)
prompt=true
;;
+   -O*)
+   orderfile="$1"
+   ;;
--)
shift
break
@@ -460,7 +464,8 @@ main () {
fi
 
files=$(git -c core.quotePath=false \
-   diff --name-only --diff-filter=U -- "$@")
+   diff --name-only --diff-filter=U \
+   ${orderfile:+"$orderfile"} -- "$@")
 
cd_to_toplevel
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 38c1e4d..5cfad76 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -638,5 +638,32 @@ test_expect_success 'diff.orderFile configuration is 
honored' '
test_cmp expect actual &&
git reset --hard >/dev/null
 '
+test_expect_success 'mergetool -Oorder-file is honored' '
+   test_config diff.orderFile order-file &&
+   test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+   test_config mergetool.myecho.trustExitCode true &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   a
+   b
+   EOF
+   git mergetool -O/dev/null --no-prompt --tool myecho |
+   grep -A 2 Merging: >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1 &&
+
+   git config --unset diff.orderFile &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   b
+   a
+   EOF
+   git mergetool -Oorder-file --no-prompt --tool myecho >output &&
+   git grep --no-index -h -A2 Merging: output >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1
+'
 
 test_done
-- 
2.10.1.386.g8ee99a0

Re: [PATCH 4/4] mergetool: honor -O

[Johannes Sixt]

Am 07.10.2016 um 01:47 schrieb David Aguilar:

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 65696d8..9dca58b 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,7 @@
 # at the discretion of Junio C Hamano.
 #

-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] 
...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O] 
[file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -390,6 +390,7 @@ print_noop_and_exit () {
 main () {
prompt=$(git config --bool mergetool.prompt)
guessed_merge_tool=false
+   order_file=

while test $# != 0
do
@@ -419,6 +420,9 @@ main () {
--prompt)
prompt=true
;;
+   -O*)
+   order_file="$1"
+   ;;
--)
shift
break
@@ -459,8 +463,14 @@ main () {
fi
fi

+   if test -n "$order_file"
+   then
+   set -- "$order_file" -- "$@"
+   else
+   set -- -- "$@"
+   fi
files=$(git -c core.quotePath=false \
-   diff --name-only --diff-filter=U -- "$@")
+   diff --name-only --diff-filter=U "$@")


You can write this as

files=$(git -c core.quotePath=false \
diff --name-only --diff-filter=U \
${order_file:+"$order_file"} -- "$@")

without the case distinction earlier. This construct is usually used to 
attach the option (-O) in front of the argument, but it is already 
included in the value; so, we use the construct only to avoid an empty 
argument when the option is unset and to get a quoted string when it is set.




cd_to_toplevel

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index fb2aeef..0f9869a 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -637,6 +637,33 @@ test_expect_success 'diff.orderFile configuration is 
honored' '
test_cmp expect actual &&
git reset --hard >/dev/null
 '
+
+test_expect_success 'mergetool -Oorder-file is honored' '
+   test_config diff.orderFile order-file &&
+   test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+   test_config mergetool.myecho.trustExitCode true &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   a
+   b
+   EOF
+   git mergetool -O/dev/null --no-prompt --tool myecho |
+   grep -A 2 Merging: >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1 &&
+
+   git config --unset diff.orderFile &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   b
+   a
+   EOF
+   git mergetool -Oorder-file --no-prompt --tool myecho |
+   grep -A 2 Merging: >actual &&


grep -A again. Furthermore, you call git mergetool in the upstream of a 
pipe. This will ignore the exit status.



+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1
 '


Ah, the earlier lone quote should have been added in this patch.

-- Hannes

[PATCH v2 4/4] mergetool: honor -O

[David Aguilar]

Teach mergetool to pass "-O" down to `git diff` when
specified on the command-line.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
This is a replacement patch for 4/4 from the original series.

The changes are stylistic -- the "order_file" variable name and
"-O" in the usage were changed to "orderfile" and
"-O" for consistency with the documentation.

 Documentation/git-mergetool.txt | 10 ++
 git-mergetool.sh| 14 --
 t/t7610-mergetool.sh| 27 +++
 3 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index c4c1a9b..3622d66 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -79,10 +79,12 @@ success of the resolution after the custom tool has exited.
Prompt before each invocation of the merge resolution program
to give the user a chance to skip the path.
 
-DIFF ORDER FILES
-
-`git mergetool` honors the `diff.orderFile` configuration variable
-used by `git diff`.  See linkgit:git-config[1] for more details.
+-O::
+   Process files in the order specified in the
+   , which has one shell glob pattern per line.
+   This overrides the `diff.orderFile` configuration variable
+   (see linkgit:git-config[1]).  To cancel `diff.orderFile`,
+   use `-O/dev/null`.
 
 TEMPORARY FILES
 ---
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 65696d8..1a0fe7a 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] 
...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O] 
[file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -390,6 +390,7 @@ print_noop_and_exit () {
 main () {
prompt=$(git config --bool mergetool.prompt)
guessed_merge_tool=false
+   orderfile=
 
while test $# != 0
do
@@ -419,6 +420,9 @@ main () {
--prompt)
prompt=true
;;
+   -O*)
+   orderfile="$1"
+   ;;
--)
shift
break
@@ -459,8 +463,14 @@ main () {
fi
fi
 
+   if test -n "$orderfile"
+   then
+   set -- "$orderfile" -- "$@"
+   else
+   set -- -- "$@"
+   fi
files=$(git -c core.quotePath=false \
-   diff --name-only --diff-filter=U -- "$@")
+   diff --name-only --diff-filter=U "$@")
 
cd_to_toplevel
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index fb2aeef..0f9869a 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -637,6 +637,33 @@ test_expect_success 'diff.orderFile configuration is 
honored' '
test_cmp expect actual &&
git reset --hard >/dev/null
 '
+
+test_expect_success 'mergetool -Oorder-file is honored' '
+   test_config diff.orderFile order-file &&
+   test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+   test_config mergetool.myecho.trustExitCode true &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   a
+   b
+   EOF
+   git mergetool -O/dev/null --no-prompt --tool myecho |
+   grep -A 2 Merging: >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1 &&
+
+   git config --unset diff.orderFile &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   b
+   a
+   EOF
+   git mergetool -Oorder-file --no-prompt --tool myecho |
+   grep -A 2 Merging: >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1
 '
 
 test_done
-- 
2.10.1.386.g42aabb0

[PATCH 4/4] mergetool: honor -O

[David Aguilar]

Teach mergetool to pass "-O" down to `git diff` when
specified on the command-line.

Signed-off-by: David Aguilar <dav...@gmail.com>
---
 Documentation/git-mergetool.txt | 10 ++
 git-mergetool.sh| 14 --
 t/t7610-mergetool.sh| 27 +++
 3 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index c4c1a9b..3622d66 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -79,10 +79,12 @@ success of the resolution after the custom tool has exited.
Prompt before each invocation of the merge resolution program
to give the user a chance to skip the path.
 
-DIFF ORDER FILES
-
-`git mergetool` honors the `diff.orderFile` configuration variable
-used by `git diff`.  See linkgit:git-config[1] for more details.
+-O::
+   Process files in the order specified in the
+   , which has one shell glob pattern per line.
+   This overrides the `diff.orderFile` configuration variable
+   (see linkgit:git-config[1]).  To cancel `diff.orderFile`,
+   use `-O/dev/null`.
 
 TEMPORARY FILES
 ---
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 65696d8..9dca58b 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] 
...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O] 
[file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -390,6 +390,7 @@ print_noop_and_exit () {
 main () {
prompt=$(git config --bool mergetool.prompt)
guessed_merge_tool=false
+   order_file=
 
while test $# != 0
do
@@ -419,6 +420,9 @@ main () {
--prompt)
prompt=true
;;
+   -O*)
+   order_file="$1"
+   ;;
--)
shift
break
@@ -459,8 +463,14 @@ main () {
fi
fi
 
+   if test -n "$order_file"
+   then
+   set -- "$order_file" -- "$@"
+   else
+   set -- -- "$@"
+   fi
files=$(git -c core.quotePath=false \
-   diff --name-only --diff-filter=U -- "$@")
+   diff --name-only --diff-filter=U "$@")
 
cd_to_toplevel
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index fb2aeef..0f9869a 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -637,6 +637,33 @@ test_expect_success 'diff.orderFile configuration is 
honored' '
test_cmp expect actual &&
git reset --hard >/dev/null
 '
+
+test_expect_success 'mergetool -Oorder-file is honored' '
+   test_config diff.orderFile order-file &&
+   test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+   test_config mergetool.myecho.trustExitCode true &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   a
+   b
+   EOF
+   git mergetool -O/dev/null --no-prompt --tool myecho |
+   grep -A 2 Merging: >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1 &&
+
+   git config --unset diff.orderFile &&
+   test_must_fail git merge order-file-side1 &&
+   cat >expect <<-\EOF &&
+   Merging:
+   b
+   a
+   EOF
+   git mergetool -Oorder-file --no-prompt --tool myecho |
+   grep -A 2 Merging: >actual &&
+   test_cmp expect actual &&
+   git reset --hard >/dev/null 2>&1
 '
 
 test_done
-- 
2.10.1.355.g33afd83.dirty

[PATCH 3/3] http-walker: reduce O(n) ops with doubly-linked list

[Eric Wong]

Using the a Linux-kernel-derived doubly-linked list
implementation from the Userspace RCU library allows us to
enqueue and delete items from the object request queue in
constant time.

This change reduces enqueue times in the prefetch() function
where object request queue could grow to several thousand
objects.

I left out the list_for_each_entry* family macros from list.h
which relied on the __typeof__ operator as we support platforms
without it.  Thus, list_entry (aka "container_of") needs to be
called explicitly inside macro-wrapped for loops.

The downside is this costs us an additional pointer per object
request, but this is offset by reduced overhead on queue
operations leading to improved performance and shorter queue
depths.

Signed-off-by: Eric Wong 
---
 http-walker.c |  42 ++-
 list.h| 164 ++
 2 files changed, 179 insertions(+), 27 deletions(-)
 create mode 100644 list.h

diff --git a/http-walker.c b/http-walker.c
index 48f2df4..0b24255 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -2,6 +2,7 @@
 #include "commit.h"
 #include "walker.h"
 #include "http.h"
+#include "list.h"
 
 struct alt_base {
char *base;
@@ -23,7 +24,7 @@ struct object_request {
struct alt_base *repo;
enum object_request_state state;
struct http_object_request *req;
-   struct object_request *next;
+   struct list_head node;
 };
 
 struct alternates_request {
@@ -41,7 +42,7 @@ struct walker_data {
struct alt_base *alt;
 };
 
-static struct object_request *object_queue_head;
+static LIST_HEAD(object_queue_head);
 
 static void fetch_alternates(struct walker *walker, const char *base);
 
@@ -110,19 +111,10 @@ static void process_object_response(void *callback_data)
 
 static void release_object_request(struct object_request *obj_req)
 {
-   struct object_request *entry = object_queue_head;
-
if (obj_req->req !=NULL && obj_req->req->localfile != -1)
error("fd leakage in release: %d", obj_req->req->localfile);
-   if (obj_req == object_queue_head) {
-   object_queue_head = obj_req->next;
-   } else {
-   while (entry->next != NULL && entry->next != obj_req)
-   entry = entry->next;
-   if (entry->next == obj_req)
-   entry->next = entry->next->next;
-   }
 
+   list_del(_req->node);
free(obj_req);
 }
 
@@ -130,8 +122,10 @@ static void release_object_request(struct object_request 
*obj_req)
 static int fill_active_slot(struct walker *walker)
 {
struct object_request *obj_req;
+   struct list_head *pos, *tmp, *head = _queue_head;
 
-   for (obj_req = object_queue_head; obj_req; obj_req = obj_req->next) {
+   list_for_each_safe(pos, tmp, head) {
+   obj_req = list_entry(pos, struct object_request, node);
if (obj_req->state == WAITING) {
if (has_sha1_file(obj_req->sha1))
obj_req->state = COMPLETE;
@@ -148,7 +142,6 @@ static int fill_active_slot(struct walker *walker)
 static void prefetch(struct walker *walker, unsigned char *sha1)
 {
struct object_request *newreq;
-   struct object_request *tail;
struct walker_data *data = walker->data;
 
newreq = xmalloc(sizeof(*newreq));
@@ -157,18 +150,9 @@ static void prefetch(struct walker *walker, unsigned char 
*sha1)
newreq->repo = data->alt;
newreq->state = WAITING;
newreq->req = NULL;
-   newreq->next = NULL;
 
http_is_verbose = walker->get_verbosely;
-
-   if (object_queue_head == NULL) {
-   object_queue_head = newreq;
-   } else {
-   tail = object_queue_head;
-   while (tail->next != NULL)
-   tail = tail->next;
-   tail->next = newreq;
-   }
+   list_add_tail(>node, _queue_head);
 
 #ifdef USE_CURL_MULTI
fill_active_slots();
@@ -451,11 +435,15 @@ static int fetch_object(struct walker *walker, unsigned 
char *sha1)
 {
char *hex = sha1_to_hex(sha1);
int ret = 0;
-   struct object_request *obj_req = object_queue_head;
+   struct object_request *obj_req = NULL;
struct http_object_request *req;
+   struct list_head *pos, *head = _queue_head;
 
-   while (obj_req != NULL && hashcmp(obj_req->sha1, sha1))
-   obj_req = obj_req->next;
+   list_for_each(pos, head) {
+   obj_req = list_entry(pos, struct object_request, node);
+   if (!hashcmp(obj_req->sha1, sha1))
+   break;
+   }
if (obj_req == NULL)
return error("Couldn't find request for %s in the queue", hex);
 
diff --git a/list.h b/list.h
new file mode 100644
index 000..f65edce
--- /dev/null
+++ b/list.h
@@ -0,0 +1,164 @@
+/*
+ * Copyright (C) 2002 Free Software Foundation, Inc.
+ * 

Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects

[Ramsay Jones]

On 09/06/16 18:12, Jeff King wrote:
> On Thu, Jun 09, 2016 at 03:34:59PM +0100, Ramsay Jones wrote:
> 
>> Just FYI, this patch removes the last use of write_or_whine() - should it
>> be removed?
> 
> That sounds reasonable. Want to do a patch on top?

OK, will do.

ATB,
Ramsay Jones


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

Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects

[Matthieu Moy]

Junio C Hamano  writes:

> Matthieu Moy  writes:
>
>> Jeff King  writes:
>>
>>> --- a/send-pack.c
>>> +++ b/send-pack.c
>>> @@ -36,18 +36,15 @@ int option_parse_push_signed(const struct option *opt,
>>> die("bad %s argument: %s", opt->long_name, arg);
>>>  }
>>>  
>>> -static int feed_object(const unsigned char *sha1, int fd, int negative)
>>> +static void feed_object(const unsigned char *sha1, FILE *fh, int negative)
>>>  {
>>> -   char buf[42];
>>> -
>>> if (negative && !has_sha1_file(sha1))
>>> -   return 1;
>>> +   return;
>> [...]
>>> @@ -97,21 +95,22 @@ static int pack_objects(int fd, struct ref *refs, 
>>> struct sha1_array *extra, stru
>> [...]
>>> for (i = 0; i < extra->nr; i++)
>>> -   if (!feed_object(extra->sha1[i], po.in, 1))
>>> -   break;
>>> +   feed_object(extra->sha1[i], po_in, 1);
>>
>> I may have missed the obvious, but doesn't this change the behavior when
>> "negative && !has_sha1_file(sha1)" happens? I understand that you don't
>> need write_or_whine anymore, but don't understand how you get rid of the
>> "return 1" here.
>
> The original feed_object() has somewhat strange interface in that a
> non-zero return from it is "Everything went alright!", and zero
> means "Oops, something went wrong".

Indeed, this is the "obvious" I've missed. So, indeed, the new "return"
does the same thing as the old "return 1".

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects

[Jeff King]

On Thu, Jun 09, 2016 at 09:40:42AM -0700, Junio C Hamano wrote:

> >>for (i = 0; i < extra->nr; i++)
> >> -  if (!feed_object(extra->sha1[i], po.in, 1))
> >> -  break;
> >> +  feed_object(extra->sha1[i], po_in, 1);
> >
> > I may have missed the obvious, but doesn't this change the behavior when
> > "negative && !has_sha1_file(sha1)" happens? I understand that you don't
> > need write_or_whine anymore, but don't understand how you get rid of the
> > "return 1" here.
> [...]
> The original caller checks for errors to break out the feeding of
> the process early, with things like:
> 
>   if (!feed_object(...))
>   break;
> 
> IOW, the caller would have continued when hitting that "return 1"
> codepath.
> 
> And the code with the patch, the caller continues unconditionally,
> so there is no behaviour change, if I am reading the code correctly.

Right, that's my reading as well (and IMHO another good motivation for
the patch, if it makes this all less confusing).

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

Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects

[Jeff King]

On Thu, Jun 09, 2016 at 03:34:59PM +0100, Ramsay Jones wrote:

> Just FYI, this patch removes the last use of write_or_whine() - should it
> be removed?

That sounds reasonable. Want to do a patch on top?

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

Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects

[Junio C Hamano]

Matthieu Moy  writes:

> Jeff King  writes:
>
>> --- a/send-pack.c
>> +++ b/send-pack.c
>> @@ -36,18 +36,15 @@ int option_parse_push_signed(const struct option *opt,
>>  die("bad %s argument: %s", opt->long_name, arg);
>>  }
>>  
>> -static int feed_object(const unsigned char *sha1, int fd, int negative)
>> +static void feed_object(const unsigned char *sha1, FILE *fh, int negative)
>>  {
>> -char buf[42];
>> -
>>  if (negative && !has_sha1_file(sha1))
>> -return 1;
>> +return;
> [...]
>> @@ -97,21 +95,22 @@ static int pack_objects(int fd, struct ref *refs, struct 
>> sha1_array *extra, stru
> [...]
>>  for (i = 0; i < extra->nr; i++)
>> -if (!feed_object(extra->sha1[i], po.in, 1))
>> -break;
>> +feed_object(extra->sha1[i], po_in, 1);
>
> I may have missed the obvious, but doesn't this change the behavior when
> "negative && !has_sha1_file(sha1)" happens? I understand that you don't
> need write_or_whine anymore, but don't understand how you get rid of the
> "return 1" here.

The original feed_object() has somewhat strange interface in that a
non-zero return from it is "Everything went alright!", and zero
means "Oops, something went wrong".  When the function actually
writes things out, it calls write_or_whine(), whose return value
also uses that (unusual) convention, which is the reason why it
behaves that way.

The "return 1" you noticed happens when the function is told not to
send history behind one object, but that object does not exist in
our repository.  This is a perfectly normal condition and the
function just ignores it and returns without feeding it to
pack-objects.  It reports to the caller "Everything went alright!".

The original caller checks for errors to break out the feeding of
the process early, with things like:

if (!feed_object(...))
break;

IOW, the caller would have continued when hitting that "return 1"
codepath.

And the code with the patch, the caller continues unconditionally,
so there is no behaviour change, if I am reading the code correctly.

Thanks for carefully reading the change and pointing out hard-to-read
parts, as always.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects

[Ramsay Jones]

On 09/06/16 13:10, Matthieu Moy wrote:
> Jeff King  writes:
> 
>> --- a/send-pack.c
>> +++ b/send-pack.c
>> @@ -36,18 +36,15 @@ int option_parse_push_signed(const struct option *opt,
>>  die("bad %s argument: %s", opt->long_name, arg);
>>  }
>>  
>> -static int feed_object(const unsigned char *sha1, int fd, int negative)
>> +static void feed_object(const unsigned char *sha1, FILE *fh, int negative)
>>  {
>> -char buf[42];
>> -
>>  if (negative && !has_sha1_file(sha1))
>> -return 1;
>> +return;
> [...]
>> @@ -97,21 +95,22 @@ static int pack_objects(int fd, struct ref *refs, struct 
>> sha1_array *extra, stru
> [...]
>>  for (i = 0; i < extra->nr; i++)
>> -if (!feed_object(extra->sha1[i], po.in, 1))
>> -break;
>> +feed_object(extra->sha1[i], po_in, 1);
> 
> I may have missed the obvious, but doesn't this change the behavior when
> "negative && !has_sha1_file(sha1)" happens? I understand that you don't
> need write_or_whine anymore, but don't understand how you get rid of the
> "return 1" here.

Just FYI, this patch removes the last use of write_or_whine() - should it
be removed?

ATB,
Ramsay Jones


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

Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects

[Matthieu Moy]

Jeff King  writes:

> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -36,18 +36,15 @@ int option_parse_push_signed(const struct option *opt,
>   die("bad %s argument: %s", opt->long_name, arg);
>  }
>  
> -static int feed_object(const unsigned char *sha1, int fd, int negative)
> +static void feed_object(const unsigned char *sha1, FILE *fh, int negative)
>  {
> - char buf[42];
> -
>   if (negative && !has_sha1_file(sha1))
> - return 1;
> + return;
[...]
> @@ -97,21 +95,22 @@ static int pack_objects(int fd, struct ref *refs, struct 
> sha1_array *extra, stru
[...]
>   for (i = 0; i < extra->nr; i++)
> - if (!feed_object(extra->sha1[i], po.in, 1))
> - break;
> + feed_object(extra->sha1[i], po_in, 1);

I may have missed the obvious, but doesn't this change the behavior when
"negative && !has_sha1_file(sha1)" happens? I understand that you don't
need write_or_whine anymore, but don't understand how you get rid of the
"return 1" here.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] send-pack: use buffered I/O to talk to pack-objects

[Jeff King]

On Wed, Jun 08, 2016 at 03:19:18PM -0400, Jeff King wrote:

> That made me wonder if we could repeatedly reuse a buffer attached to
> the file descriptor. And indeed, isn't that what stdio is? The whole
> reason this buffer exists is because we are using a direct descriptor
> write. If we switched this function to use fprintf(), we'd avoid the
> whole buffer question, have a fixed cap on our memory use (since we just
> flush anytime the buffer is full) _and_ we'd reduce the number of
> write syscalls we're making by almost a factor of 100.

So all of this strbuf discussion aside, I think it is worth doing
something like this for this particular case.

-- >8 --
Subject: send-pack: use buffered I/O to talk to pack-objects

We start a pack-objects process and then write all of the
positive and negative sha1s to it over a pipe. We do so by
formatting each item into a fixed-size buffer and then
writing each individually. This has two drawbacks:

  1. There's some manual computation of the buffer size,
 which is not immediately obvious is correct (though it
 is).

  2. We write() once per sha1, which means a lot more system
 calls than are necessary.

We can solve both by wrapping the pipe descriptor in a stdio
handle; this is the same technique used by upload-pack when
serving fetches.

Note that we can also simplify and improve the error
handling here. The original detected a single write error
and broke out of the loop (presumably to avoid writing the
error message over and over), but never actually acted on
seeing an error; we just fed truncated input and took
whatever pack-objects returned.

In practice, this probably didn't matter, as the likely
errors would be caused by pack-objects dying (and we'd
probably just die with SIGPIPE anyway). But we can easily
make this simpler and more robust; the stdio handle keeps an
error flag, which we can check at the end.

Signed-off-by: Jeff King <p...@peff.net>
---
 send-pack.c | 33 -
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 37ee04e..299d303 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -36,18 +36,15 @@ int option_parse_push_signed(const struct option *opt,
die("bad %s argument: %s", opt->long_name, arg);
 }
 
-static int feed_object(const unsigned char *sha1, int fd, int negative)
+static void feed_object(const unsigned char *sha1, FILE *fh, int negative)
 {
-   char buf[42];
-
if (negative && !has_sha1_file(sha1))
-   return 1;
+   return;
 
-   memcpy(buf + negative, sha1_to_hex(sha1), 40);
if (negative)
-   buf[0] = '^';
-   buf[40 + negative] = '\n';
-   return write_or_whine(fd, buf, 41 + negative, "send-pack: send refs");
+   putc('^', fh);
+   fputs(sha1_to_hex(sha1), fh);
+   putc('\n', fh);
 }
 
 /*
@@ -73,6 +70,7 @@ static int pack_objects(int fd, struct ref *refs, struct 
sha1_array *extra, stru
NULL,
};
struct child_process po = CHILD_PROCESS_INIT;
+   FILE *po_in;
int i;
 
i = 4;
@@ -97,21 +95,22 @@ static int pack_objects(int fd, struct ref *refs, struct 
sha1_array *extra, stru
 * We feed the pack-objects we just spawned with revision
 * parameters by writing to the pipe.
 */
+   po_in = xfdopen(po.in, "w");
for (i = 0; i < extra->nr; i++)
-   if (!feed_object(extra->sha1[i], po.in, 1))
-   break;
+   feed_object(extra->sha1[i], po_in, 1);
 
while (refs) {
-   if (!is_null_oid(>old_oid) &&
-   !feed_object(refs->old_oid.hash, po.in, 1))
-   break;
-   if (!is_null_oid(>new_oid) &&
-   !feed_object(refs->new_oid.hash, po.in, 0))
-   break;
+   if (!is_null_oid(>old_oid))
+   feed_object(refs->old_oid.hash, po_in, 1);
+   if (!is_null_oid(>new_oid))
+   feed_object(refs->new_oid.hash, po_in, 0);
refs = refs->next;
}
 
-   close(po.in);
+   fflush(po_in);
+   if (ferror(po_in))
+   die_errno("error writing to pack-objects");
+   fclose(po_in);
 
if (args->stateless_rpc) {
char *buf = xmalloc(LARGE_PACKET_MAX);
-- 
2.9.0.rc2.138.geb72a36

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

Re: [PATCH] sequencer.c: fix detection of duplicate s-o-b

[Willy Tarreau]

Hi Christian,

On Thu, Apr 07, 2016 at 04:06:59PM -0400, Christian Couder wrote:
> On Wed, Apr 6, 2016 at 12:37 PM, Willy Tarreau <w...@1wt.eu> wrote:
> > On Wed, Apr 06, 2016 at 07:57:01AM -0700, Junio C Hamano wrote:
> >> This seems to have been lost, perhaps because the top part that was
> >> quite long didn't look like a patch submission message or something.
> >
> > Don't worry, we all know it's the submitter's responsibility to retransmit,
> > I apply the same principle :-)
> >
> >> Git 1.7.12 is a quite ancient release and I wouldn't be surprised if
> >> we made the behaviour change during the period leading to v2.6 on
> >> purpose, but nothing immediately comes to mind. Christian (as the
> >> advocate for the trailer machinery) and Brandon ("git shortlog
> >> sequencer.c" suggests you), can you take a look?
> 
> Ok, I will try to have a look at that next week.
> 
> > FWIW it wad changed in 1.8.3 by commit bab4d10 ("sequencer.c: teach
> > append_signoff how to detect duplicate s-o-b").
> 
> So the change is quite old and was made before I started working on
> the trailer machinery.
> 
> > The change made a lot of sense but it didn't assume that this practice
> > was common. And indeed I think this practice only happens in maintenance
> > branches where people have to make a lot of adaptations to existing
> > patches that they're cherry-picking. We do that a lot in stable kernels
> > to keep track of what we may need to revisit if we break something.
> 
> Yeah, we know for some time, but after the above patch breakage
> happened and after I worked on interpret-trailers, that some lines
> inside [] are added by kernel people in the trailer part and that the
> trailer machinery doesn't work properly with such lines.
> 
> Anyway if you want your patch to be applied, it will probably need tests.

Thanks. I've discussed with Junio yesterday who notified me that my patch
broke some existing tests that I hadn't updated (I didn't know they existed)
namely one supposed to make the distinction between a normal footer and a
special case where the s-o-b is in fact part of the last paragraph. So I
said I'll rework the patch to only consider the parts between brackets
above a footer. Thus no need to waste your time testing the current patch
next week.

Thanks,
Willy

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

Re: [PATCH] sequencer.c: fix detection of duplicate s-o-b

[Christian Couder]

On Wed, Apr 6, 2016 at 12:37 PM, Willy Tarreau <w...@1wt.eu> wrote:
> On Wed, Apr 06, 2016 at 07:57:01AM -0700, Junio C Hamano wrote:
>> This seems to have been lost, perhaps because the top part that was
>> quite long didn't look like a patch submission message or something.
>
> Don't worry, we all know it's the submitter's responsibility to retransmit,
> I apply the same principle :-)
>
>> Git 1.7.12 is a quite ancient release and I wouldn't be surprised if
>> we made the behaviour change during the period leading to v2.6 on
>> purpose, but nothing immediately comes to mind. Christian (as the
>> advocate for the trailer machinery) and Brandon ("git shortlog
>> sequencer.c" suggests you), can you take a look?

Ok, I will try to have a look at that next week.

> FWIW it wad changed in 1.8.3 by commit bab4d10 ("sequencer.c: teach
> append_signoff how to detect duplicate s-o-b").

So the change is quite old and was made before I started working on
the trailer machinery.

> The change made a lot of sense but it didn't assume that this practice
> was common. And indeed I think this practice only happens in maintenance
> branches where people have to make a lot of adaptations to existing
> patches that they're cherry-picking. We do that a lot in stable kernels
> to keep track of what we may need to revisit if we break something.

Yeah, we know for some time, but after the above patch breakage
happened and after I worked on interpret-trailers, that some lines
inside [] are added by kernel people in the trailer part and that the
trailer machinery doesn't work properly with such lines.

Anyway if you want your patch to be applied, it will probably need tests.

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

Re: [PATCH] sequencer.c: fix detection of duplicate s-o-b

[Willy Tarreau]

On Wed, Apr 06, 2016 at 07:57:01AM -0700, Junio C Hamano wrote:
> This seems to have been lost, perhaps because the top part that was
> quite long didn't look like a patch submission message or something.

Don't worry, we all know it's the submitter's responsibility to retransmit,
I apply the same principle :-)

> Git 1.7.12 is a quite ancient release and I wouldn't be surprised if
> we made the behaviour change during the period leading to v2.6 on
> purpose, but nothing immediately comes to mind. Christian (as the
> advocate for the trailer machinery) and Brandon ("git shortlog
> sequencer.c" suggests you), can you take a look?

FWIW it wad changed in 1.8.3 by commit bab4d10 ("sequencer.c: teach
append_signoff how to detect duplicate s-o-b").

The change made a lot of sense but it didn't assume that this practice
was common. And indeed I think this practice only happens in maintenance
branches where people have to make a lot of adaptations to existing
patches that they're cherry-picking. We do that a lot in stable kernels
to keep track of what we may need to revisit if we break something.

Thanks!
Willy

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

Re: [PATCH] sequencer.c: fix detection of duplicate s-o-b

[Junio C Hamano]

This seems to have been lost, perhaps because the top part that was
quite long didn't look like a patch submission message or something.

Git 1.7.12 is a quite ancient release and I wouldn't be surprised if
we made the behaviour change during the period leading to v2.6 on
purpose, but nothing immediately comes to mind.  Christian (as the
advocate for the trailer machinery) and Brandon ("git shortlog
sequencer.c" suggests you), can you take a look?

Willy Tarreau <w...@1wt.eu> writes:

> Hi,
>
> after I upgraded my machine, I switched from git 1.7.12.2 to 2.6.4
> and experienced an annoying regression when dealing with stable
> kernel backports.
>
> I'm using a "dorelease" script which relies on git-cherry-pick's
> ability to properly detect duplicate s-o-b to ensure that all merged
> commits are properly signed in a release. Today while preparing the
> last 2.6.32 release, I did a git log before pushing and found some
> commits having two s-o-b lines with myself. I found that these ones
> were always those containing some backporting notes between the s-o-b
> lines (which we all do in stable branches to indicate what was changed
> in the backport process).
>
> I didn't feel brave enough to individually deal with each offending
> patch by hand so instead I bisected the git changes and found that the
> behaviour changed with commit bab4d10 ("sequencer.c: teach append_signoff
> how to detect duplicate s-o-b").
>
> The reason is that function has_conforming_footer() immediately stops
> after the first non-conforming line without checking if there are
> conforming lines after. But if someone added signed-off-by anywhere
> after a non-conforming block, it should always be considered as part
> of the footer. Thus I adjusted the logic to check till the end of the
> footer and report the presence of valid rfc2822 or cherry-picked lines
> after the last non-conformant one and now it correctly handles all types
> of commits I had to deal with (ie: only adds s-o-b when it doesn't match
> the last one and doesn't add an empty line after a conformant one). For
> example, this footer :
>
> Signed-off-by: Mike Galbraith <umgwanakikb...@gmail.com>
> [bwh: Backported to 3.2:
>  - Adjust numbering in the comment
>  - Adjust filename]
> Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
> Cc: Byungchul Park <byungchul.p...@lge.com>
> Cc: Peter Zijlstra <pet...@infradead.org>
> Cc: Willy Tarreau <w...@1wt.eu>
>
> Used to be turned into this :
>
> Signed-off-by: Mike Galbraith <umgwanakikb...@gmail.com>
> [bwh: Backported to 3.2:
>  - Adjust numbering in the comment
>  - Adjust filename]
> Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
> Cc: Byungchul Park <byungchul.p...@lge.com>
> Cc: Peter Zijlstra <pet...@infradead.org>
> Cc: Willy Tarreau <w...@1wt.eu>
>
> Signed-off-by: Willy Tarreau <w...@1wt.eu>
>
> And is now properly converted to :
>
> Signed-off-by: Mike Galbraith <umgwanakikb...@gmail.com>
> [bwh: Backported to 3.2:
>  - Adjust numbering in the comment
>  - Adjust filename]
> Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
> Cc: Byungchul Park <byungchul.p...@lge.com>
> Cc: Peter Zijlstra <pet...@infradead.org>
> Cc: Willy Tarreau <w...@1wt.eu>
> Signed-off-by: Willy Tarreau <w...@1wt.eu>
>
> Also, cherry-picking the last commit above again would produce this
> before :
>
> Signed-off-by: Mike Galbraith <umgwanakikb...@gmail.com>
> [bwh: Backported to 3.2:
>  - Adjust numbering in the comment
>  - Adjust filename]
> Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
> Cc: Byungchul Park <byungchul.p...@lge.com>
> Cc: Peter Zijlstra <pet...@infradead.org>
> Cc: Willy Tarreau <w...@1wt.eu>
> Signed-off-by: Willy Tarreau <w...@1wt.eu>
>
> Signed-off-by: Willy Tarreau <w...@1wt.eu>
>
> And it now is properly left untouched since the last s-o-b line
> is properly matched.
>
> I'm appending the patch, please include it upstream.
>
> Thanks!
> Willy
>
>
> From be9624a0df4c649d452f898925953a81dc9163fc Mon Sep 17 00:00:00 2001
> From: Willy Tarreau <w...@1wt.eu>
> Date: Sat, 12 Mar 2016 13:35:35 +0100
> Subject: sequencer.c: fix detection of duplicate s-o-b
>
> Commit bab4d10 ("sequencer.c: teach append_signoff how to detect
> duplicate s-o-b") changed the method used to detect duplicate s-o-b,
> but it introduced a regression for a case where some non-compliant
> information are present in 

[PATCH] sequencer.c: fix detection of duplicate s-o-b

[Willy Tarreau]

Hi,

after I upgraded my machine, I switched from git 1.7.12.2 to 2.6.4
and experienced an annoying regression when dealing with stable
kernel backports.

I'm using a "dorelease" script which relies on git-cherry-pick's
ability to properly detect duplicate s-o-b to ensure that all merged
commits are properly signed in a release. Today while preparing the
last 2.6.32 release, I did a git log before pushing and found some
commits having two s-o-b lines with myself. I found that these ones
were always those containing some backporting notes between the s-o-b
lines (which we all do in stable branches to indicate what was changed
in the backport process).

I didn't feel brave enough to individually deal with each offending
patch by hand so instead I bisected the git changes and found that the
behaviour changed with commit bab4d10 ("sequencer.c: teach append_signoff
how to detect duplicate s-o-b").

The reason is that function has_conforming_footer() immediately stops
after the first non-conforming line without checking if there are
conforming lines after. But if someone added signed-off-by anywhere
after a non-conforming block, it should always be considered as part
of the footer. Thus I adjusted the logic to check till the end of the
footer and report the presence of valid rfc2822 or cherry-picked lines
after the last non-conformant one and now it correctly handles all types
of commits I had to deal with (ie: only adds s-o-b when it doesn't match
the last one and doesn't add an empty line after a conformant one). For
example, this footer :

Signed-off-by: Mike Galbraith <umgwanakikb...@gmail.com>
[bwh: Backported to 3.2:
 - Adjust numbering in the comment
 - Adjust filename]
Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
Cc: Byungchul Park <byungchul.p...@lge.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Willy Tarreau <w...@1wt.eu>

Used to be turned into this :

Signed-off-by: Mike Galbraith <umgwanakikb...@gmail.com>
[bwh: Backported to 3.2:
 - Adjust numbering in the comment
 - Adjust filename]
Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
Cc: Byungchul Park <byungchul.p...@lge.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Willy Tarreau <w...@1wt.eu>

Signed-off-by: Willy Tarreau <w...@1wt.eu>

And is now properly converted to :

Signed-off-by: Mike Galbraith <umgwanakikb...@gmail.com>
[bwh: Backported to 3.2:
 - Adjust numbering in the comment
 - Adjust filename]
Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
Cc: Byungchul Park <byungchul.p...@lge.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Willy Tarreau <w...@1wt.eu>
Signed-off-by: Willy Tarreau <w...@1wt.eu>

Also, cherry-picking the last commit above again would produce this
before :

Signed-off-by: Mike Galbraith <umgwanakikb...@gmail.com>
[bwh: Backported to 3.2:
 - Adjust numbering in the comment
 - Adjust filename]
Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
Cc: Byungchul Park <byungchul.p...@lge.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Willy Tarreau <w...@1wt.eu>
Signed-off-by: Willy Tarreau <w...@1wt.eu>

Signed-off-by: Willy Tarreau <w...@1wt.eu>

And it now is properly left untouched since the last s-o-b line
is properly matched.

I'm appending the patch, please include it upstream.

Thanks!
Willy


>From be9624a0df4c649d452f898925953a81dc9163fc Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w...@1wt.eu>
Date: Sat, 12 Mar 2016 13:35:35 +0100
Subject: sequencer.c: fix detection of duplicate s-o-b

Commit bab4d10 ("sequencer.c: teach append_signoff how to detect
duplicate s-o-b") changed the method used to detect duplicate s-o-b,
but it introduced a regression for a case where some non-compliant
information are present in the footer. In maintenance branches, it's
very common to add some elements after the signed-off and to add your
s-o-b after. This is used a lot in the stable kernel series, for
example this commit backported from 3.2 to 2.6.32 :

ALSA: usb-audio: avoid freeing umidi object twice

commit 07d86ca93db7e5cdf4743564d98292042ec21af7 upstream.

The 'umidi' object will be free'd on the error path by snd_usbmidi_free()
when tearing down the rawmidi interface. So we shouldn't try to free it
in snd_usbmidi_create() after having registered the rawmidi interface.

Found by KASAN.

Signed-off-by: Andrey Konovalov <andreyk...@gmail.com>
Acked-by: Clemens Ladisch <clem...@ladisch.de>
Signed-off-by: Takashi Iwai <ti...@suse.de>
Signed-off-by: Ben Hutchings <b...@decadent.org.uk>
[wt: file is sound/midi/usbmidi.c in 2.6.32]
Signed-off-by: Willy Tarreau <w...@1wt.eu>

Prior to the commit above, a

Re: Minor bug, git ls-files -o aborts because of broken submodules

[Andreas Krey]

On Fri, 22 Jan 2016 17:26:50 +, Jeff King wrote:
...
> Here it is. I think this is the right fix, based on the previous attempt
> by Andreas and my comments. Sorry for stealing your topic,

This seems to keep happening with things I try to patch. :-)

> but I hope
> the perf numbers in the second patch will brighten your day. :)

The patches are 'quadratically' improving my case as well,
many thanks for completing this. (I was just mustering
the steam for another round of work on this.)

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds 
Date: Fri, 22 Jan 2010 07:29:21 -0800
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Minor bug, git ls-files -o aborts because of broken submodules

[Jeff King]

On Fri, Jan 22, 2016 at 04:17:29PM +0700, Duy Nguyen wrote:

> $ git init abc
> $ cd abc
> $ mkdir def
> $ echo 'gitdir: blah blah' >def/.git
> $ git ls-files -o
> fatal: Not a git repository: def/blah blah
> 
> If some directory looks like a submodule but turns out not, that's not
> a fatal error. The stack trace is something like this. I suspect
> do_submodule_path should use the gently version..
> 
> #1  0x00588a78 in die
> #2  0x00558ded in read_gitfile_gently
> #3  0x0051e2f6 in do_submodule_path
> #4  0x0051e484 in git_pathdup_submodule
> #5  0x005340ac in resolve_gitlink_ref_recursive
> #6  0x005342cf in resolve_gitlink_ref
> #7  0x004dd20d in treat_directory
> #8  0x004dd760 in treat_one_path
> #9  0x004dd971 in treat_path
> #10 0x004de038 in read_directory_recursive

I think we'd have to change the semantics going up the stack, as
do_submodule_path has no way to report failure.

But I think this is another case of

  http://thread.gmane.org/gmane.comp.version-control.git/265560/focus=281253

There the question was about performance (lots of these clog up the
linear ref_cache list), but I think the root cause is the same: going
too far into ref resolution without realizing we don't have a real
submodule.

Andreas posted some patches, but they didn't make it to the list. Here
are my replies, which did:

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

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

  http://thread.gmane.org/gmane.comp.version-control.git/282030

However, from going over them, I think there was a problem in the series;
we really do need to know the sha1 in some of these cases. So I think
maybe the simplest thing would be to catch this case in
resolve_gitlink_ref(), before we go deeper. Let me see if I can come up
with something.

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

Minor bug, git ls-files -o aborts because of broken submodules

[Duy Nguyen]

$ git init abc
$ cd abc
$ mkdir def
$ echo 'gitdir: blah blah' >def/.git
$ git ls-files -o
fatal: Not a git repository: def/blah blah

If some directory looks like a submodule but turns out not, that's not
a fatal error. The stack trace is something like this. I suspect
do_submodule_path should use the gently version..

#1  0x00588a78 in die
#2  0x00558ded in read_gitfile_gently
#3  0x0051e2f6 in do_submodule_path
#4  0x0051e484 in git_pathdup_submodule
#5  0x005340ac in resolve_gitlink_ref_recursive
#6  0x005342cf in resolve_gitlink_ref
#7  0x004dd20d in treat_directory
#8  0x004dd760 in treat_one_path
#9  0x004dd971 in treat_path
#10 0x004de038 in read_directory_recursive
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Minor bug, git ls-files -o aborts because of broken submodules

[Jeff King]

On Fri, Jan 22, 2016 at 04:18:03PM -0500, Jeff King wrote:

> But I think this is another case of
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/265560/focus=281253
> 
> There the question was about performance (lots of these clog up the
> linear ref_cache list), but I think the root cause is the same: going
> too far into ref resolution without realizing we don't have a real
> submodule.
> 
> Andreas posted some patches, but they didn't make it to the list. Here
> are my replies, which did:
> 
>   http://article.gmane.org/gmane.comp.version-control.git/282028
> 
>   http://article.gmane.org/gmane.comp.version-control.git/282029
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/282030
> 
> However, from going over them, I think there was a problem in the series;
> we really do need to know the sha1 in some of these cases. So I think
> maybe the simplest thing would be to catch this case in
> resolve_gitlink_ref(), before we go deeper. Let me see if I can come up
> with something.

Here it is. I think this is the right fix, based on the previous attempt
by Andreas and my comments. Sorry for stealing your topic, but I hope
the perf numbers in the second patch will brighten your day. :)

  [1/2]: clean: make is_git_repository a public function
  [2/2]: resolve_gitlink_ref: ignore non-repository paths

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

[PATCH 2/2] run-parallel: Run sequential if nonblocking I/O is unavailable

[Stefan Beller]

Windows doesn't have O_NONBLOCK nor F_GETFL defined, so we need cannot run
in parallel there. Instead the children will output directly to our stderr
and we run one child at a time.

Bonus: We are setting process.err = -1; which we previously expected the
get_next_task callback to do. It is easy to forget that part in the callback
leading to hard to debug errors.

Signed-off-by: Stefan Beller 
---
 run-command.c | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 86fbe50..19de253 100644
--- a/run-command.c
+++ b/run-command.c
@@ -958,6 +958,10 @@ static struct parallel_processes *pp_init(int n,
if (n < 1)
n = online_cpus();
 
+#if !(defined (O_NONBLOCK) && defined (F_GETFL))
+   n = 1;
+#endif
+
pp->max_processes = n;
pp->data = data;
if (!get_next_task)
@@ -1006,6 +1010,7 @@ static void pp_cleanup(struct parallel_processes *pp)
sigchain_pop_common();
 }
 
+#if defined (O_NONBLOCK) && defined (F_GETFL)
 static void set_nonblocking_or_die(int fd)
 {
int flags = fcntl(fd, F_GETFL);
@@ -1014,6 +1019,7 @@ static void set_nonblocking_or_die(int fd)
else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
die("Could not set file status flags");
 }
+#endif
 
 /* returns
  *  0 if a new task was started.
@@ -1031,6 +1037,12 @@ static int pp_start_one(struct parallel_processes *pp)
if (i == pp->max_processes)
die("BUG: bookkeeping is hard");
 
+#if defined (O_NONBLOCK) && defined (F_GETFL)
+   pp->children[i].process.err = -1;
+#else
+   pp->children[i].process.err = 2;
+#endif
+
if (!pp->get_next_task(>children[i].data,
   >children[i].process,
   >children[i].err,
@@ -1049,8 +1061,9 @@ static int pp_start_one(struct parallel_processes *pp)
strbuf_reset(>children[i].err);
return code ? -1 : 1;
}
-
+#if defined (O_NONBLOCK) && defined (F_GETFL)
set_nonblocking_or_die(pp->children[i].process.err);
+#endif
 
pp->nr_processes++;
pp->children[i].in_use = 1;
-- 
2.6.1.247.ge8f2a41.dirty

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

[PATCH v2 2/3] Documentation/grep: fix documentation of -O

[Matthieu Moy]

Since the argument of -O, --open-file-in-pager is optional, it must be
stuck to the command. Reflect this in the documentation.

Signed-off-by: Matthieu Moy <matthieu@imag.fr>
---
 Documentation/git-grep.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 31811f1..1c07c7f 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -160,8 +160,8 @@ OPTIONS
For better compatibility with 'git diff', `--name-only` is a
synonym for `--files-with-matches`.
 
--O []::
---open-files-in-pager []::
+-O[]::
+--open-files-in-pager[=]::
Open the matching files in the pager (not the output of 'grep').
If the pager happens to be "less" or "vi", and the user
specified only one pattern, the first file is positioned at
-- 
2.5.0.402.g8854c44

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

[PATCH 2/3] Documentation/grep: fix documentation of -O

[Matthieu Moy]

Since the argument of -O, --open-file-in-pager is optional, it must be
stuck to the command. Reflect this in the documentation.

Signed-off-by: Matthieu Moy <matthieu@imag.fr>
---
 Documentation/git-grep.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 31811f1..1c07c7f 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -160,8 +160,8 @@ OPTIONS
For better compatibility with 'git diff', `--name-only` is a
synonym for `--files-with-matches`.
 
--O []::
---open-files-in-pager []::
+-O[]::
+--open-files-in-pager[=]::
Open the matching files in the pager (not the output of 'grep').
If the pager happens to be "less" or "vi", and the user
specified only one pattern, the first file is positioned at
-- 
2.5.0.402.g8854c44

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

Re: [PATCH v3 08/18] rerere: explain the rerere I/O abstraction

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 +/*
 + * Write a conflict marker to io-output (if defined).
 + */
  static void rerere_io_putconflict(int ch, int size, struct rerere_io *io)
  {
   char buf[64];

This is unrelated to the topic of this step, but this function seems
to be very poorly put together with duct tape.

 static void rerere_io_putconflict(int ch, int size, struct rerere_io *io)
 {
   char buf[64];
 
   while (size) {
   if (size  sizeof(buf) - 2) {
   memset(buf, ch, size);
   buf[size] = '\n';
   buf[size + 1] = '\0';
   size = 0;

The if() guarding this block has an off-by-one error in the benign
direction.  When size is 62, the marker with terminating LF and NUL
should still fit within the buf[], so it should use =, not ,
to compare.

   } else {
   int sz = sizeof(buf) - 1;
   if (size = sz)
   sz -= (sz - size) + 1;
   memset(buf, ch, sz);
   buf[sz] = '\0';
   size -= sz;

This is an even more awkward construct.  sz is what we have to work
with the substring that we cannot emit with a single call (because
buf[] is too small), so naturally I would expect it to be more like

int to_emit = size;
if (sz = size)
to_emit = sz;
memset(buf, ch, to_emit);
buf[to_emit] = '\0';
size -= to_emit;

which makes the if (size = sz) in the code look very suspicious.

But rewriting it to the more natural version would make it buggy.
At a right boundary condition, i.e. size == 63, we may find that the
conflict marker is too long with LF and NUL to fit in buf[] and come
here, and then fill the full conflict marker with NUL just fine in
buf[], decrementing size to 0, emit that 63-byte long marker.  The
next iteration will exit the loop without emitting the LF.

The unnatural is what is saving us from such a bug.

if (size = sz)
sz -= (sz - size) + 1;

It ensures that subtraction of sz (i.e. to_emit) from size before
the next iteration will never brings size down to zero by reducing
sz by one too much, forcing another iteration, which will then have
size smaller than sizeof(buf) - 2 and have us emit the LF.

Not buggy, but ugly.

   }
   rerere_io_putstr(buf, io);
   }
 }
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 07/16] engine.pl: Fix i18n -o option in msvc buildsystem generator

[Philip Oakley]

The i18n 5e9637c (i18n: add infrastructure for translating
Git with gettext, 2011-11-18) introduced an extra '-o' option
into the make file.

If the msvc buildsystem is run without NO_GETTEXT being set
then this broke the engine.pl code for extracting the git.sln
for msvc gui-IDE. The setting of NO_GETTEXT was not fixed until
later, relative to the Msysgit project where this issue was being
investigated.

The presence of these options in the Makefile output should not
compromise the derived build structure. They should be ignored.

Add tests to remove these non linker options, in same vein as
74cf9bd (engine.pl: Fix a recent breakage of the buildsystem
generator, 2010-01-22).

Signed-off-by: Philip Oakley philipoak...@iee.org
---
 contrib/buildsystems/engine.pl | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/contrib/buildsystems/engine.pl b/contrib/buildsystems/engine.pl
index 24b8992..60c7a7d 100755
--- a/contrib/buildsystems/engine.pl
+++ b/contrib/buildsystems/engine.pl
@@ -141,6 +141,12 @@ sub parseMakeOutput
 next;
 }
 
+if ($text =~ /^(mkdir|msgfmt) /) {
+# options to the Portable Object translations
+# the line mkdir ...  msgfmt ... contains no linker options
+next;
+}
+
 if($text =~ / -c /) {
 # compilation
 handleCompileLine($text, $line);
-- 
2.4.2.windows.1.5.gd32afb6

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

[PATCH v3 08/18] rerere: explain the rerere I/O abstraction

[Junio C Hamano]

Explain the internals of rerere as in-code comments.

This one covers our thin I/O abstraction to read from either
a file or a memory while optionally writing out to a file.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
 rerere.c | 38 +++---
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/rerere.c b/rerere.c
index 7b1419c..7ed20f1 100644
--- a/rerere.c
+++ b/rerere.c
@@ -83,6 +83,21 @@ static int write_rr(struct string_list *rr, int out_fd)
return 0;
 }
 
+/*
+ * rerere interacts with conflicted file contents using this I/O
+ * abstraction.  It reads a conflicted contents from one place via
+ * getline() method, and optionally can write it out after
+ * normalizing the conflicted hunks to the output.  Subclasses of
+ * rerere_io embed this structure at the beginning of their own
+ * rerere_io object.
+ */
+struct rerere_io {
+   int (*getline)(struct strbuf *, struct rerere_io *);
+   FILE *output;
+   int wrerror;
+   /* some more stuff */
+};
+
 static void ferr_write(const void *p, size_t count, FILE *fp, int *err)
 {
if (!count || *err)
@@ -96,19 +111,15 @@ static inline void ferr_puts(const char *s, FILE *fp, int 
*err)
ferr_write(s, strlen(s), fp, err);
 }
 
-struct rerere_io {
-   int (*getline)(struct strbuf *, struct rerere_io *);
-   FILE *output;
-   int wrerror;
-   /* some more stuff */
-};
-
 static void rerere_io_putstr(const char *str, struct rerere_io *io)
 {
if (io-output)
ferr_puts(str, io-output, io-wrerror);
 }
 
+/*
+ * Write a conflict marker to io-output (if defined).
+ */
 static void rerere_io_putconflict(int ch, int size, struct rerere_io *io)
 {
char buf[64];
@@ -137,11 +148,17 @@ static void rerere_io_putmem(const char *mem, size_t sz, 
struct rerere_io *io)
ferr_write(mem, sz, io-output, io-wrerror);
 }
 
+/*
+ * Subclass of rerere_io that reads from an on-disk file
+ */
 struct rerere_io_file {
struct rerere_io io;
FILE *input;
 };
 
+/*
+ * ... and its getline() method implementation
+ */
 static int rerere_file_getline(struct strbuf *sb, struct rerere_io *io_)
 {
struct rerere_io_file *io = (struct rerere_io_file *)io_;
@@ -286,11 +303,18 @@ static int handle_file(const char *path, unsigned char 
*sha1, const char *output
return hunk_no;
 }
 
+/*
+ * Subclass of rerere_io that reads from an in-core buffer that is a
+ * strbuf
+ */
 struct rerere_io_mem {
struct rerere_io io;
struct strbuf input;
 };
 
+/*
+ * ... and its getline() method implementation
+ */
 static int rerere_mem_getline(struct strbuf *sb, struct rerere_io *io_)
 {
struct rerere_io_mem *io = (struct rerere_io_mem *)io_;
-- 
2.5.0-rc2-340-g016

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