[PATCH] Remove the line length limit for graft files

[Johannes Schindelin]

Support for grafts predates Git's strbuf, and hence it is understandable
that there was a hard-coded line length limit of 1023 characters (which
was chosen a bit awkwardly, given that it is *exactly* one byte short of
aligning with the 41 bytes occupied by a commit name and the following
space or new-line character).

While regular commit histories hardly win comprehensibility in general
if they merge more than twenty-two branches in one go, it is not Git's
business to limit grafts in such a way.

In this particular developer's case, the use case that requires
substantially longer graft lines to be supported is the visualization of
the commits' order implied by their changes: commits are considered to
have an implicit relationship iff exchanging them in an interactive
rebase would result in merge conflicts.

Thusly implied branches tend to be very shallow in general, and the
resulting thicket of implied branches is usually very wide; It is
actually quite common that *most* of the commits in a topic branch have
not even one implied parent, so that a final merge commit has about as
many implied parents as there are commits in said branch.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 builtin/blame.c |  8 
 commit.c| 10 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 1407ae7..9047b6e 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1804,17 +1804,17 @@ static int prepare_lines(struct scoreboard *sb)
 static int read_ancestry(const char *graft_file)
 {
FILE *fp = fopen(graft_file, r);
-   char buf[1024];
+   struct strbuf buf = STRBUF_INIT;
if (!fp)
return -1;
-   while (fgets(buf, sizeof(buf), fp)) {
+   while (!strbuf_getwholeline(buf, fp, '\n')) {
/* The format is just Commit Parent1 Parent2 ...\n */
-   int len = strlen(buf);
-   struct commit_graft *graft = read_graft_line(buf, len);
+   struct commit_graft *graft = read_graft_line(buf.buf, buf.len);
if (graft)
register_commit_graft(graft, 0);
}
fclose(fp);
+   strbuf_release(buf);
return 0;
 }
 
diff --git a/commit.c b/commit.c
index de16a3c..57ebea2 100644
--- a/commit.c
+++ b/commit.c
@@ -196,19 +196,19 @@ bad_graft_data:
 static int read_graft_file(const char *graft_file)
 {
FILE *fp = fopen(graft_file, r);
-   char buf[1024];
+   struct strbuf buf = STRBUF_INIT;
if (!fp)
return -1;
-   while (fgets(buf, sizeof(buf), fp)) {
+   while (!strbuf_getwholeline(buf, fp, '\n')) {
/* The format is just Commit Parent1 Parent2 ...\n */
-   int len = strlen(buf);
-   struct commit_graft *graft = read_graft_line(buf, len);
+   struct commit_graft *graft = read_graft_line(buf.buf, buf.len);
if (!graft)
continue;
if (register_commit_graft(graft, 1))
-   error(duplicate graft data: %s, buf);
+   error(duplicate graft data: %s, buf.buf);
}
fclose(fp);
+   strbuf_release(buf);
return 0;
 }
 
-- 
1.8.4.msysgit.0.1109.g3c58b16

--
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] Remove the line length limit for graft files

[Johannes Schindelin]

Hi Jonathan,

On Fri, 27 Dec 2013, Jonathan Nieder wrote:

 Johannes Schindelin wrote:
 
  it returns EOF only if ch == EOF *and* sb-len == 0, i.e. if no characters
  have been read before hitting EOF.
 
 Yep.  api-strbuf.txt even says so.

I never bothered to look ;-)

 Sorry for the nonsense.

Nope, no nonsense at all. I actually had a look only after your review,
and it definitely makes sense to double-check.

 For what it's worth,
 Reviewed-by: Jonathan Nieder jrnie...@gmail.com

It is worth a lot. Believe me, I know just how thankless a task reviewing
is, and instead of getting praise for it, you even get abused by
contributors who would rather self-review their code for fear of having a
twist in their knockers pointed out publicly.

Your review makes the code better, even if it does not result in code
changes all the time: it increases the confidence that the code is good.

Thank you, Jonathan!
Dscho
--
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] Remove the line length limit for graft files

[Johannes Schindelin]

Hi Jonathan,

On Fri, 27 Dec 2013, Jonathan Nieder wrote:

 Johannes Schindelin wrote:
  On Fri, 27 Dec 2013, Jonathan Nieder wrote:
 
  Is this easy to reproduce so some interested but lazy person could
  write a test?
 
  Yep. Make 25 orphan commits, add a graft line to make the first a merge of
  the rest.
 
 Thanks.  Here's a pair of tests doing that.

Thank you very much!

 + for i in 0 1 2
 + do
 + for j in 0 1 2 3 4 5 6 7 8 9
 + do

for the record, I usually prefer

i=0
while test $i -t 30
do
...
i=$(($i+1))
done

but your code is just as good!

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

Git for Windows 1.8.5.2 (preview)

[Johannes Schindelin]

Dear fans of Git,

this mail brings to you the good news that Git for Windows is available in
a new version: 1.8.5.2-preview20131230

Many, many thanks go to the tireless developers working on this
particularly hard port of Git.

Changes since Git-1.8.4-preview20130916

New Features

- Comes with Git 1.8.5.2 plus Windows-specific patches.

- Windows-specific patches are now grouped into pseudo-branches which
  should make future development more robust despite the fact that we have
  had less than stellar success getting the Windows-specific patches accepted
  by upstream git.git.

- Works around more path length limitations (pull request #86)

- Has an optional stat() cache toggled via core.fscache (pull request #107)

Bugfixes

- Lots of installer fixes

- git-cmd: Handle home directory on a different drive correctly (pull
  request #146)

- git-cmd: add a helper to work with the ssh agent (pull request #135)

- Git-Cheetah: prevent duplicate menu entries (pull request #7)

- No longer replaces dos2unix with hd2u (a more powerful, but slightly
  incompatible version of dos2unix)

In keeping with the fine tradition of making a release on the eve of a
holiday, and immediately going on vacation, it is my pleasure to wish you
all a happy new year,
Johannes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Johannes Schindelin]

Hi Junio,

On Thu, 2 Jan 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  On Thu, 2 Jan 2014, Sebastian Schuberth wrote:
 
  See https://github.com/msysgit/git/pull/80.
 
  Thanks Sebastian!
 
  However, since the git.git project is not comfortable with the concept
  of pull requests (which is why you submitted this patch via mail), I
  believe that we have to explain the rationale in the commit message.
  So here goes my attempt:
 
 Thanks; the conclusion is correct --- you need a good commit message in
 the recorded history.  That does not have anything to do with
 integrating with pulling from subsystem maintainers, which we regularly
 do.

I should have pointed out that I was talking about the concept of pull
requests, not the concept of accepting pull requests from dedicated
subsystem maintainers.

  On Linux, we can get away with assuming that the directory separator
  is a forward slash, but that is wrong in general. For that purpose,
  the is_dir_sep() function was introduced a long time ago. By using it
  in safe_create_leading_directories(), we proof said function for use
  on platforms where the directory separator is different from Linux'.
 
 Perhaps with s|Linux|POSIX|,

No, for two reasons:

* OpenVMS supports POSIX, yes? And OpenVMS does not have the forward slash
  as directory separator but instead the dot, yes?

* it would be dishonest. The reason is not that we looked at the POSIX
  standard when we determined how to implement
  safe_create_leading_directories(), but at Linux.

 but more importantly, was there a specific error scenario that triggered
 this change?

Yes, the concrete use case that triggered the pull request whose change we
did not accept but instead would prefer Sebastian's patch is where a user
called

git clone URL C:\directory

in cmd.exe.

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

Re: [PATCH v2] Fix safe_create_leading_directories() for Windows

[Johannes Schindelin]

Hi,

On Thu, 2 Jan 2014, Sebastian Schuberth wrote:

 When cloning to a directory C:\foo\bar from Windows' cmd.exe where foo
 does not exist yet, Git would throw an error like
 
 fatal: could not create work tree dir 'c:\foo\bar'.: No such file or directory
 
 Fix this by not hard-coding a platform specific directory separator into
 safe_create_leading_directories().
 
 Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
 Signed-off-by: Sebastian Schuberth sschube...@gmail.com

It would be nice to have links to the original discussion, but I guess
that that would not be accepted.

Ciao,
Dscho
--
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] Fix safe_create_leading_directories() for Windows

[Johannes Schindelin]

Hi Junio,

On Thu, 2 Jan 2014, Junio C Hamano wrote:

 If we are going to change the meaning of the function so that it can
 now take any random path in platform-specific convention

Note that nothing in the function name or documentation suggests
otherwise.

 that may be incompatible with the internal notion of paths Git has (i.e.
 what is passed to safe_create_leading_directories() may have to be
 massaged into a slash-separated form before it can be used in the index

The safe_create_leading_directories() function never interacts with the
index, so you find me quite puzzled as to your objection.

 and parsed to be stuffed into trees), it is fine to do so as long as all
 the codepaths understands the new world order, but my earlier git grep
 hits did not tell me that such a change is warranted.

You call safe_create_leading_directories() with an argument that is
supposed to be the final path, correct? So what exactly is wrong with
safe_create_leading_directories() creating all the directories necessary
so that we can write to the path afterwards, *even* if that path is
interpreted in a platform-dependent manner (as one would actually expect
it to)?

Last time I checked we did not make a fuss about
safe_create_leading_directories() interpreting the argument in a
case-insensitive fashion on certain setups, either. So it is not exactly a
new thing that the paths are interpreted in a platform-dependent manner.

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

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Johannes Schindelin]

Hi,

On Tue, 7 Jan 2014, Erik Faye-Lund wrote:

 On Thu, Jan 2, 2014 at 9:46 PM, Johannes Schindelin
 johannes.schinde...@gmx.de wrote:

  Well, you and I both know how easy GitHub's pull request made things
  for us as well as for contributors. I really cannot thank Erik enough
  for bullying me into using and accepting them.
 
 Huh? I don't think you refer to me, because I really dislike them (and I
 always have IIRC).

Ah yes, I misremembered. You were actually opposed to using them and I
thought we should be pragmatic to encourage contributions.

In any case, I do think that the contributions we got via pull requests
were in general contributions we would not otherwise have gotten.

Sorry for my mistake!
Dscho
--
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: [msysGit] Consecutive git gc fails on Windows network share

[Johannes Schindelin]

Hi Torsten,

On Mon, 20 Jan 2014, Torsten Bögershausen wrote:

 (No top-posting, please see my comments below)

already very good! For extra brownie points, just edit the quoted part to
reflect the abridged version needed to understand the context.

 On 2014-01-20 15.02, Jochen wrote:
 On 01/16/2014 10:55 AM, Jochen Haag wrote:
  The rename command replaces a mv -f command of the original shell script. 
  Obviously the -f option can also override a read-only file but rename can 
  not on a network share.
 
 I allowed me to 
 a) reconstruct the original mail,

Please note that together with an exceedingly flakey internet connection,
not only having to scroll through the mail (most of which was actually not
relevant to your reply) and having to delete most parts again ate up my
complete Git time budget for today. Just something you might want to keep
in mind.

 b) add +++ at the places where you added the stat() and chmod(),
 c) and to send the question is this a good implementation ? to upstream git.

 I think your implementation makes sense.

As I said in my other reply, I think that the problem would be addressed
more generally in compat/mingw.c. It is to be doubted highly that upstream
wants to handle cases such as rename() cannot overwrite read-only files
on Windows everywhere they call rename() because the platforms upstream
cares about do not have that problem.

So we probably need just the same _wchmod we have in mingw_unlink also in
mingw_rename.

Ciao,
Johannes

Re: [msysGit] Consecutive git gc fails on Windows network share

[Johannes Schindelin]

Hi kusma,

On Tue, 21 Jan 2014, Erik Faye-Lund wrote:

 On Tue, Jan 21, 2014 at 12:25 AM, Johannes Schindelin
 johannes.schinde...@gmx.de wrote:
 
  On Mon, 20 Jan 2014, Torsten Bögershausen wrote:
 
  b) add +++ at the places where you added the stat() and chmod(),
  c) and to send the question is this a good implementation ? to upstream 
  git.
 
  I think your implementation makes sense.
 
  As I said in my other reply, I think that the problem would be
  addressed more generally in compat/mingw.c. It is to be doubted highly
  that upstream wants to handle cases such as rename() cannot overwrite
  read-only files on Windows everywhere they call rename() because the
  platforms upstream cares about do not have that problem.
 
 I'm not so sure. A quick test shows me that this is not the case for
 NTFS. Since this is over a network-share, the problem is probably
 server-side, and can affect other systems as well.
 
 So a work-around might be appropriate for all systems, no?

I do not think that the problem occurs if you run the same commands on
Linux, on a mounted Samba share. So I guess that upstream Git can enjoy
their luxury of not having to care.

In any case, if we would need this also for Linux, doing it for only one
user of rename() would probably not be good enough, either... so something
similar to mingw_rename() would be needed (interfering with mingw_rename
itself, of course).

Ciao,
Dscho

Re: Anomalous AUTHOR and DOCUMENTATION sections in manpages

[Johannes Schindelin]

Hi Michael,

On Wed, 22 Jan 2014, Michael Haggerty wrote:

 Would the mentioned authors (CCed) consent to the removal of these
 sections?

Fine by me!
Dscho

P.S.: Congrats on your new job!
--
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] repack.c: chmod +w before rename()

[Johannes Schindelin]

Hi Brian,

On Fri, 24 Jan 2014, brian m. carlson wrote:

 On Fri, Jan 24, 2014 at 10:05:14PM +0100, Torsten Bögershausen wrote:
  diff --git a/builtin/repack.c b/builtin/repack.c
  index ba66c6e..033b4c2 100644
  --- a/builtin/repack.c
  +++ b/builtin/repack.c
  @@ -324,6 +324,10 @@ int cmd_repack(int argc, const char **argv, const char 
  *prefix)
  statbuffer.st_mode = ~(S_IWUSR | S_IWGRP | 
  S_IWOTH);
  chmod(fname_old, statbuffer.st_mode);
  }
  +   if (!stat(fname, statbuffer)) {
  +   statbuffer.st_mode |= (S_IWUSR | S_IWGRP | 
  S_IWOTH);
  +   chmod(fname, statbuffer.st_mode);
  +   }
 
 Are we sure that the file in question can never be a symlink?

On Windows: yes. Otherwise, no.

Having said that, the files in question are files generated by Git, so you
really would have to tamper with things in order to get into trouble.

 Because if it is, we'll end up changing the permissions on the wrong
 file.

In any case, I'd rather change the permissions only when the rename
failed. *And* I feel uncomfortable ignoring the return value...

 In general, I'm wary of changing permissions on a file to suit Windows's
 rename because of the symlink issue and the security issues that can
 result.

I agree on the Windows issue.

 Hard links probably have the same issue.

No, hard links have their own permissions. If you change the permissions
on a hardlink, any other hardlinks to the same content are unaffected.

Ciao,
Johannes

Re: [msysGit] Re: git svn clone not work. It's stop with no error message.

[Johannes Schindelin]

Hi Tay,

On Mon, 17 Feb 2014, Tay Ray Chuan wrote:

 Posting to msysgit since this was on Windows.

Thanks.

 On Mon, Feb 17, 2014 at 3:45 PM, youngseonkim 1.youngsun@gmail.com 
 wrote:
  git svn clone https://my.svn.repo/url --stdlayout
 
  When I test a small svn repository and 'real working repository 1' with same
  this command, it's complete successfully.
  But it's not work in a 'real working repository 2', it just stop suddenly.

The git-svn we have on Windows is really old. As an easy way out, you can
use Vagrant (https://github.com/msysgit/msysgit/wiki/Vagrant) to run a
Linux Git on your machine. That should fix your issue.

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

[msysGit] Git for Windows 1.9.0 (fwd)

[Johannes Schindelin]

Hopefully the Postfix Greylisting Policy Server will not try again to
greylist me, as it might however do without violating the RFC.

-- Forwarded message --
Date: Tue, 18 Feb 2014 00:38:54 +0100 (CET)
From: Johannes Schindelin johannes.schinde...@gmx.de
To: msys...@googlegroups.com
Cc: git@vger.kernel.org
Subject: [msysGit] Git for Windows 1.9.0

Dear Git fanbois,

this announcement informs you that the small team of volunteers who keep
the Git ship afloat for the most prevalent desktop operating system
managed to release yet another version of Git for Windows:

Git Release Notes (Git-1.9.0-preview20140217)
Last update: 17 February 2013

Changes since Git-1.8.5.2-preview20131230

New Features
- Comes with Git 1.9.0 plus Windows-specific patches.
- Better work-arounds for Windows-specific path length limitations (pull
  request #122)
- Uses optimized TortoiseGitPLink when detected (msysGit pull request
  #154)
- Allow Windows users to use Linux Git on their files, using Vagrant
  http://www.vagrantup.com/ (msysGit pull request #159)
- InnoSetup 5.5.4 is now used to generate the installer (msysGit pull
  request #167)

Bugfixes
- Fixed regression with interactive password prompt for remotes using the
  HTTPS protocol (issue #111)
- We now work around Subversion servers printing non-ISO-8601-compliant
  time stamps (pull request #126)
- The installer no longer sets the HOME environment variable (msysGit pull
  request #166)
- Perl no longer creates empty sys$command files when no stdin is
  connected (msysGit pull request #152)

Ciao,
Johannes

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github 
accounts are free.

You received this message because you are subscribed to the Google
Groups msysGit group.
To post to this group, send email to msys...@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscr...@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups 
msysGit group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to msysgit+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
--
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] t5510: Do not use $(pwd) when fetching / pushing / pulling via rsync

[Johannes Schindelin]

Hi Sebastian,

On Wed, 19 Mar 2014, Sebastian Schuberth wrote:

 On MINGW, pwd is defined as pwd -W in test-lib.sh. This usually is the
 right thing, but the absolute Windows path with a colon confuses rsync. We
 could use $PWD in this case to work around the issue, but in fact there is
 no need to use an absolute path in the first place, so get rid of it.
 
 This was discovered in the context of the mingwGitDevEnv project and only
 did not surface before with msysgit because the latter does not ship
 rsync.

ACK

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

Git for Windows 1.9.2

[Johannes Schindelin]

Hi,

the Git for Windows team just released version 1.9.2 of the
Windows-specific installers.

New Features
* Comes with Git 1.9.2 plus Windows-specific patches.
* Custom installer settings can be saved and loaded, for unsupervised
  installation on batches of machines (msysGit PR #168).
* Comes with VIM 7.4 (msysGit PR #170).
* Comes with ZLib 1.2.8.
* Comes with xargs 4.4.2.

Bugfixes
* Work around stack limitations when listing an insane number of tags (PR
  #154).
* Assorted test fixes (PRs #156, #158).
* Compile warning fix in config.c (PR #159).
* Ships with actual dos2unix and unix2dos.
* The installer no longer recommends mixing with Cygwin.
* Fixes a regression in Git-Cheetah which froze the Explorer upon calling
  Git Bash from the context menu (Git-Cheetah PRs #14 and #15).

It can be downloaded here:


https://github.com/msysgit/msysgit/releases/download/Git-1.9.2-preview20140411/Git-1.9.2-preview20140411.exe

This release also marks a change relevant only for developers wanting to
help with the development of Git for Windows: only the net installer (i.e.
a rudimentary Git environment that simply clones everything necessary to
build Git for Windows) is available for download; the full installers
were not useful for Git for Windows contributors.

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

GitMinutes about Git for Windows

[Johannes Schindelin]

Dear friends of Git for Windows,

it was very delightful to be on the show, hosted by Thomas Ferris
Nicolaisen:

http://episodes.gitminutes.com/2014/04/gitminutes-28-johannes-schindelin-on.html

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

Re: [PATCH] git tag --contains : avoid stack overflow

[Johannes Schindelin]

Hi Peff,

On Wed, 16 Apr 2014, Jeff King wrote:

 On Wed, Apr 16, 2014 at 04:15:19PM +0200, Stepan Kasal wrote:
 
  From: Jean-Jacques Lafay at Sat, 10 Nov 2012 18:36:10 +0100
  
  In large repos, the recursion implementation of contains(commit,
  commit_list) may result in a stack overflow. Replace the recursion
  with a loop to fix it.
  
  This problem is more apparent on Windows than on Linux, where the
  stack is more limited by default.
 
 I think this is a good thing to be doing, and it looks mostly good to
 me. A few comments:
 
  -static int contains_recurse(struct commit *candidate,
  +/*
  + * Test whether the candidate or one of its parents is contained in the 
  list.
  + * Do not recurse to find out, though, but return -1 if inconclusive.
  + */
  +static int contains_test(struct commit *candidate,
  const struct commit_list *want)
 
 Can we turn this return value into
 
   enum {
   CONTAINS_UNKNOWN = -1,
   CONTAINS_NO = 0,
   CONTAINS_YES = 1,
   } contains_result;
 
 to make the code a little more self-documenting?

Good idea!

 [... detailed instructions what changes are implied by the enum ...]
 
  +expect
  +# ulimit is a bash builtin; we can rely on that in MinGW, but nowhere else
  +test_expect_success MINGW '--contains works in a deep repo' '
  +   ulimit -s 64
 
 It would be nice to test this on Linux.
 
 Can we do something like:
 
   test_lazy_prereq BASH 'bash --version'
 
   test_expect_success BASH '--contains works in a deep repo' '
   ... setup repo ...
   bash -c ulimit -s 64  git tag --contains HEAD actual 
   test_cmp expect actual
   '
 
 As a bonus, then our ulimit call does not pollute the environment of
 subsequent tests.

That's a very good idea! We mulled it over a bit and did not come up with
this excellent solution.

Please see https://github.com/msysgit/git/c63d196 for the fixup, and
https://github.com/msysgit/git/compare/tag-contains%5E...tag-contains for
the updated patch.

Thanks,
Dscho
--
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] git tag --contains : avoid stack overflow

[Johannes Schindelin]

Hi Peff,

On Thu, 17 Apr 2014, Jeff King wrote:

 On Thu, Apr 17, 2014 at 07:31:54PM +0200, Johannes Schindelin wrote:
 
 bash -c ulimit -s 64  git tag --contains HEAD actual 
  [...]
  Please see https://github.com/msysgit/git/c63d196 for the fixup, and
  https://github.com/msysgit/git/compare/tag-contains%5E...tag-contains for
  the updated patch.
 
 I tried running the test on my Linux box, but it doesn't fail with the
 existing recursive code.

I cannot recall how I came to choose 64, but I *think* I only tested on
Windows, and I *think* I reduced the number of tags in order to make
things faster (Windows is *unbearably* slow with spawn-happy programs such
as Git's tests -- literally every single line in a shell script tests the
patience of this developer, running the complete test suite with 15
parallel threads takes several hours, no kidding).

 The results are strangely non-deterministic, but with -O0, we generally
 die reliably below about 60. With -O2, though, it's more like 43. We
 can't go _too_ low here, though, as lots of things start breaking around
 32.

How about using 40, then? I am more interested in reducing the runtime
than reducing the number of false negatives. The problem will be exercised
enough on Windows, but not if the test suite becomes even slower than it
already is.

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

Re: [msysGit] Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows

[Johannes Schindelin]

Hi,

On Sat, 19 Apr 2014, Heiko Voigt wrote:

 On Thu, Apr 03, 2014 at 05:18:50PM +0400, ma...@slonopotamus.org wrote:
  I'm proud to announce WinGit:
  an attempt to bring Git powers to 64-bit Windows.
 
 So the reason for this new package is that you need 64bit binaries?
 
  Relationship with msysgit
  =
  
  Unlike msysgit, WinGit is a pure-Windows binary build with MSVC.

Marat, please do not add to the confusion. msysGit is the name of the
*development environment* for developing Git for Windows. It also brings
facilities to use MSVC instead of GCC.

So do not compare WinGit to msysgit (that would be like comparing Git to
GCC). Compare WinGit to Git for Windows (and clarify that you mean a
different WinGit than the old name of Git for Windows).

  Like msysgit, WinGit also uses msys environment (sh/perl/etc) both
  during build-time and runtime.

So it is not purely 64-bit, because MSys is not.

 I can see the need for a pure Windows solution (no msys tools at least for
 runtime). But this sounds to me that the only thing you changed is the
 compiler and 64bit? The git binaries in msysgit are already pure Windows
 binaries with no need of msys.dll. The only reason why so many other
 tools are shipped with msysgit is to run scripted commands (e.g. like
 gitk or rebase).
 
 What is the reason of using a closed source compiler? Why not use the
 64bit mingw that is already used to build the 64bit explorer extension
 to package 64bit binaries along with the 32bit ones in the installer?
 
 Sorry if I am a little bit skeptic, but I am wondering whether it does
 make sense for you to join forces with msysgit instead of creating a
 fork? I think the main reason why there are no 64 bit binaries shipped
 with msysgit is that nobody needed them and the need to ship both (at
 least for some time).

We do have a facility to build 64-bit binaries with msysGit. It is even
dirt-easy: just run the two release scripts in /src/mingw-w64/, and then
build Git with make W64=1.

The real reason why Git for Windows does not ship 64-bit binaries is that
they did not pass the test suite last time I tried.

And for the record: I would have welcome contributions to the Git for
Windows project. I still will. After all, there is no reason for yet
another fork.

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

Re: [msysGit] Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows

[Johannes Schindelin]

Hi Marat,

On Sat, 19 Apr 2014, Marat Radchenko wrote:

 But in practice, msysgit is:
  1) outdated msys that was patched in multiple ways without
   sending patches upstream
  2) heavily patched git, again not upstreamed

Again, this time explicitly: I wish you had done a little more research on
the matter, and I wish you had participated in the msysGit community
before going on to reinvent the wheel.

I have nothing per se against your effort, of course, you can spend your
time as you want, but please refrain from claiming things that are
provably incorrect.

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

Re: [msysGit] Re: Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows

[Johannes Schindelin]

Hi Heiko,

On Sat, 19 Apr 2014, Heiko Voigt wrote:

 Regarding mingwGitDevEnv[2]: That is a project started by Sebastian who
 also contributes to msysgit (and Git for Windows).

In fact, Sebastian is not only a contributor. He is co-maintainer of Git
for Windows.

 It eventually can (and probably should) be a successor of the current
 situation [...]

Sebastian hinted at it in many a discussion on the msysgit mailing list
(where those people who are serious about Git for Windows development hang
out, hint, hint, hint) that mingwGitDevEnv was born out of the needs
identified while maintaining Git for Windows.

Our tentative plan is to switch with Git 2.0 (unless the timing turns out
to be unfortunate). We have to put some more effort into mingwGitDevEnv
first, though: due to the differences between the old MSys committed into
msysgit.git repository on the one hand, and the MSys environment
initialized and updated by mingwGitDevEnv on the other hand, some of the
tests do not pass yet. (I also would like to look into getting the
performance improvement Hannes Sixt achieved by his patch [*1*] into
mingwGitDevEnv's Git installation, too.)

Despite the common lack of time and of developers willing to spend time to
contribute to the Git for Windows effort, I think we are well on track,
and it will be pretty exciting when we switch to mingwGitDevEnv-based
development of Git for Windows!

Ciao,
Dscho
--
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: [msysGit] Re: Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows

[Johannes Schindelin]

On Mon, 21 Apr 2014, Johannes Schindelin wrote:

 (I also would like to look into getting the performance improvement
 Hannes Sixt achieved by his patch [*1*] into mingwGitDevEnv's Git
 installation, too.)

Whoops. Footnote *1*: https://github.com/msysgit/msysgit/commit/a0f5d4f
--
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: [msysGit] Re: Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows

[Johannes Schindelin]

Hi Sebastian,

On Mon, 21 Apr 2014, Sebastian Schuberth wrote:

 On 21.04.2014 00:10, Johannes Schindelin wrote:
 
  tests do not pass yet. (I also would like to look into getting the
  performance improvement Hannes Sixt achieved by his patch [*1*] into
  mingwGitDevEnv's Git installation, too.)
 
  Whoops. Footnote *1*: https://github.com/msysgit/msysgit/commit/a0f5d4f
 
 Dscho, this patch of Hannes is already in, see [1].

Ah, I missed that. That's very good news!

Marat, you see that the patches *are* sent upstream.

Now, clearly you have all the motivation that is needed to get 64-bit
builds of Git for Windows going, and all the motivation required to make
sure that the MSVC support of the msysGit project works.

How about joining forces?

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

Re: [msysGit] Re: Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows

[Johannes Schindelin]

Hi,

On Mon, 21 Apr 2014, Felipe Contreras wrote:

 Johannes Schindelin wrote:
  Now, clearly you have all the motivation that is needed to get 64-bit
  builds of Git for Windows going, and all the motivation required to make
  sure that the MSVC support of the msysGit project works.
 
 s/msysGit/Git/

No. I meant the msysGit project; the project that maintains the current
development environment for Git for Windows. Please do not try to
reinterpret what I am saying.

 Personally I don't see why ideally I shouldn't be able to build upstream
 Git for Windows with mingw without leaving my Linux system.

Maybe because you could not even test it properly, let alone run the test
suite? And maybe because according to the famous scratch your own itch
credo, it is actually the duty of Windows users -- i.e. users who do not
even have your Linux system -- to fix the bugs that would never be
encountered anywhere but Windows?

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

Re: [NOT_A_PATCH] A naive attempt to cross-build Linux-mingw64 Git

[Johannes Schindelin]

Hi Marat,

On Tue, 22 Apr 2014, Marat Radchenko wrote:

 Johannes says building mingw64 Git is dirt-easy.

Marat, please let's stop misquoting me, okay?

What I said was more along the lines that there had been some start of a
work on getting things to compile for 64-bit Windows, but that the test
suite did not pass.

Even cutting out the part about the test suite from quoting me leaves out
the main point of what I said.

And for the record: I just had a look; the beginnings of W64 support are
in https://github.com/msysgit/git/compare/7f37564...work/w64.

And again for the record: at least from my side, there is more than just
willingness to cooperate. We'd just need to start a conversation in the
second person (as opposed to the third person).

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

Re: [msysGit] Re: Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows

[Johannes Schindelin]

Hi Felipe,

On Tue, 22 Apr 2014, Felipe Contreras wrote:

 Johannes Schindelin wrote:
  On Mon, 21 Apr 2014, Felipe Contreras wrote:
   Johannes Schindelin wrote:
Now, clearly you have all the motivation that is needed to get 64-bit
builds of Git for Windows going, and all the motivation required to make
sure that the MSVC support of the msysGit project works.
   
   s/msysGit/Git/
  
  No. I meant the msysGit project; the project that maintains the current
  development environment for Git for Windows. Please do not try to
  reinterpret what I am saying.
 
 I don't care what you are saying,

That is quite obvious and did not need clarifying.

Nevertheless, by stating that substitute command, you corrected me.
Except that you did not, I intended to say something different, and your
correction was quite misplaced.

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

Re: [PATCH v2] git tag --contains : avoid stack overflow

[Johannes Schindelin]

Hi,

On Wed, 23 Apr 2014, Stepan Kasal wrote:

 I have found out that ulimit -s does not work on Windows.
 Adding this as a prerequisite, we will skip the test there.

The interdiff can be seen here:

https://github.com/msysgit/git/commit/c68e27d5

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

Re: [PATCH v2] git tag --contains : avoid stack overflow

[Johannes Schindelin]

Hi Peff,

On Wed, 23 Apr 2014, Jeff King wrote:

 On Wed, Apr 23, 2014 at 09:53:25AM +0200, Stepan Kasal wrote:
 
  I have found out that ulimit -s does not work on Windows.  Adding
  this as a prerequisite, we will skip the test there.
 
 I found this bit weird, as the test originated on Windows. Did it never
 actually cause a failure there (i.e., the ulimit -s doesn't do
 anything)? Or does ulimit fail?

I must have forgotten to test on Windows. For performance reasons (you
know that I only have a Git time budget of about 15min/day), I developed
the test and the patch on Linux.

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

Re: [msysGit] Re: git version 1.9.0 missing git-http-push?

[Johannes Schindelin]

Hi kusma,

On Mon, 28 Apr 2014, Erik Faye-Lund wrote:

 On Mon, Apr 28, 2014 at 10:48 AM, Erik Faye-Lund kusmab...@gmail.com wrote:
  So it seems that 08900987 (Decide whether to build http-push in the
  Makefile) makes a bad assumption about the availability of
  curl-config on new libcurl installations; it's not present on stock
  Windows builds.
 
 I wonder, though. That check is over 8 years old. Are that old systems
 (that haven't been upgraded) still able to build Git? Even my old
 RedHat 5 setup has curl 7.15.5...

The easiest way in my humble opinion would be to install a script like
this into /mingw/bin/:

-- snip --
#!/bin/sh

case $1 in
--vernum)
version=$(curl -V) || exit
version=$(echo ${version%% (*)*} | tr . \ )
eval printf %02d%02d%02d ${version#curl }
;;
--cflags)
;;
--libs)
echo -lcurl
;;
esac
-- snap --

That way, upstream Git does not have anything to change (and we avoid
discussing five versions of essentially the same patch :-P).

Hmm?

Ciao,
Dscho
--
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 08/12] MINGW: fix main() signature in http-fetch.c and remote-curl.c

[Johannes Schindelin]

Hi kusma,

On Wed, 30 Apr 2014, Erik Faye-Lund wrote:

 While it's certainly a good point, I think this is *our* fault for not
 upstreaming our changes, and the responsibility of cleaning up merges
 should lie on our shoulders. We've diverged a lot. Sure, Dscho does a
 great job making the divergence less painful, but IMO we should try to
 reduce the delta as well.

Just for historical context: we *did* try to get our changes upstream. In
fact, that was in large part everything Steffen Prohaska did while he was
maintaining Git for Windows. The going has been tough, though, to the
point that we were losing contributors who were not *quite* willing to put
up with such a detailed vetting process as the Git mailing list requires.

I have to admit that it is really, really hard even for someone like me,
who is used to the ways of the Git mailing list, because just a simple
thing like this curl-config issue already eats up several days of my Git
time budget.

So while I am sympathetic to the point of view that the Git for Windows
project failed to upstream its changes, I am *equally* sympathetic to the
point of view that it is an undue burden to have to go through the process
of getting patches included by upstream Git. I, for one, simply ain't got
the time, man.

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

Re: [PATCH/RFC] Makefile: do not depend on curl-config

[Johannes Schindelin]

Hi kusma,

On Wed, 30 Apr 2014, Erik Faye-Lund wrote:

 We can keep this patch in the msysGit repo for the 2.0 release.

FWIW the plan is to switch to mingwGitDevEnv for the 2.0 release. It is
not quite clear as of yet how patches will be managed with said
environment.

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

Re: [PATCH] grep -I: do not bother to read known-binary files

[Johannes Schindelin]

Hi Peff,

On Wed, 14 May 2014, Jeff King wrote:

 On Wed, May 14, 2014 at 05:44:19PM +0200, Stepan Kasal wrote:
 
  From: Johannes Schindelin johannes.schinde...@gmx.de
  Date: Mon, 8 Nov 2010 16:10:43 +0100
  
  Incidentally, this makes grep -I respect the binary attribute (actually,
  the -text attribute, but binary implies that).
 
 Hrm. Is this patch still necessary? In the time since this patch was
 written, we did 0826579 (grep: load file data after checking
 binary-ness, 2012-02-02)

I have no time to test this but I trust that you made sure that it works
as advertised. In my case, there were about 500 gigabytes of image data
intermixed with code, and waiting for 'git grep' was not funny at all (and
I did not have time back then to go through a full code submission cycle
on the Git mailing list, either).

So I guess we can drop my patch.

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

Re: [PATCH] git grep -O -i: if the pager is 'less', pass the '-i' option

[Johannes Schindelin]

Hi Junio,

On Wed, 14 May 2014, Junio C Hamano wrote:

 Jonathan Nieder jrnie...@gmail.com writes:
 
  + if (opt.ignore_case  !strcmp(less, pager))
  + string_list_append(path_list, -i);
 
  I have a feeling that this goes against the recent trend of not
  mucking with the expectation of the users on their pagers, if I
  recall correctly the arguments for dropping S from the default given
  to an unconfigured LESS environment variable.
 
  It's just missing an explanation.
  ...
  (That's -I, not -i, because it ought to work even when the pattern
  contains capital letters.)
 
 Spot on.  The change, especially with -I, makes sense.

Except that it was not tested with -I. If you change it that way and it
stops working on Windows, it's useless to me.

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

Re: [PATCH] git grep -O -i: if the pager is 'less', pass the '-i' option

[Johannes Schindelin]

Hi Peff,

On Wed, 14 May 2014, Jeff King wrote:

 On Wed, May 14, 2014 at 11:26:54AM -0700, Jonathan Nieder wrote:
 
  git grep has other options that affect interpretation of the pattern
  which this patch does not help with:
  
   * -v / --ignore-match: probably should disable this feature of -O.
   * -E / --extended-regexp
   * -P / --perl-regexp
   * -F / --fixed-strings: ideally would auto-escape regex specials.
   * -epattern1 --or -epattern2
  
  And git grep -Ovi has a similar bug, for which the fix is to add
  \c to the pattern instead of passing an -I option.
 
 We've already found the lines of interest to the user. It would be nice
 if we could somehow point the pager at them by number, rather than
 repeating the (slightly incompatible) search.

FWIW it is exactly that type of I want your patch to do more than you do
type of comments that makes it impossible for myself to contribute patches
anymore. It just does not fit inside my Git time budget anymore.

Besides, it breaks exactly the intended usage. My intent is not just to
see the matching lines in the pager, but to be able to adjust the search
pattern further if needed. Your suggestion completely breaks that usage.

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

Re: [msysGit] [PATCH] transport-helper: add trailing --

[Johannes Schindelin]

Hi,

On Thu, 15 May 2014, Stepan Kasal wrote:

 From: Sverre Rabbelier srabbel...@gmail.com
 Date: Sat, 28 Aug 2010 20:49:01 -0500
 
 [PT: ensure we add an additional element to the argv array]

IIRC we had problems with refs vs file names.

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

Re: [PATCH] git grep -O -i: if the pager is 'less', pass the '-i' option

[Johannes Schindelin]

Hi,

On Thu, 15 May 2014, Jonathan Nieder wrote:

 Johannes Schindelin wrote:
  On Wed, 14 May 2014, Junio C Hamano wrote:
 
  Spot on.  The change, especially with -I, makes sense.
 
  Except that it was not tested with -I. If you change it that way and it
  stops working on Windows, it's useless to me.
 
 Are you saying that less on Windows doesn't have an -I option?

I did not say that.

 version.c tells me it was added in v266 (1994-12-26).

Thanks. That was the thing I was really looking for: an indication that -I
is not a new-fangled thing but a well-tested option that even old greps
have.

Ciao,
Dscho
--
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 6/6] Make sure that index-pack --strict fails upon invalid tag objects

[Johannes Schindelin]

One of the most important use cases for the strict tag object checking
is when transfer.fsckobjects is set to true to catch invalid objects
early on. This new regression test essentially tests the same code path
by directly calling 'index-pack --strict' on a pack containing an
invalid tag object.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 t/t5302-pack-index.sh | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index 4bbb718..80bd3ee 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -243,4 +243,23 @@ test_expect_success 'running index-pack in the object 
store' '
 test -f .git/objects/pack/pack-${pack1}.idx
 '
 
+test_expect_success 'index-pack --strict fails upon invalid tag' '
+sha=$(git rev-parse HEAD) 
+cat wrong-tag EOF 
+object $sha
+type commit
+tag guten tag
+
+This is an invalid tag.
+EOF
+
+tag=$(git hash-object -t tag -w --stdin wrong-tag) 
+pack1=$(echo $tag | git pack-objects tag-test) 
+echo remove tag object 
+thirtyeight=${tag#??} 
+rm -f .git/objects/${tag%$thirtyeight}/$thirtyeight 
+test_must_fail git index-pack --strict tag-test-${pack1}.pack 2 err 
+grep invalid .tag. name err
+'
+
 test_done
-- 
2.0.0.rc3.9669.g840d1f9
--
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 5/6] Add regression tests for stricter tag fsck'ing

[Johannes Schindelin]

The intent of the two new test cases is to catch general breakages in
the fsck_tag() function, not so much to test it extensively, trying to
strike the proper balance between thoroughness and speed.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 t/t1450-fsck.sh | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 8c739c9..16b3e4a 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -194,6 +194,45 @@ test_expect_success 'tag pointing to something else than 
its type' '
test_must_fail git fsck --tags
 '
 
+test_expect_success 'tag with incorrect tag name' '
+   sha=$(git rev-parse HEAD) 
+   cat wrong-tag -EOF 
+   object $sha
+   type commit
+   tag wrong name format
+   tagger T A Gger tag...@example.com 1234567890 -
+
+   This is an invalid tag.
+   EOF
+
+   tag=$(git hash-object -t tag -w --stdin wrong-tag) 
+   test_when_finished remove_object $tag 
+   echo $tag .git/refs/tags/wrong 
+   test_when_finished git update-ref -d refs/tags/wrong 
+   git fsck --tags 2out 
+   cat out 
+   grep invalid .tag. name out
+'
+
+test_expect_success 'tag with missing tagger' '
+   sha=$(git rev-parse HEAD) 
+   cat wrong-tag -EOF 
+   object $sha
+   type commit
+   tag gutentag
+
+   This is an invalid tag.
+   EOF
+
+   tag=$(git hash-object -t tag -w --stdin wrong-tag) 
+   test_when_finished remove_object $tag 
+   echo $tag .git/refs/tags/wrong 
+   test_when_finished git update-ref -d refs/tags/wrong 
+   git fsck --tags 2out 
+   cat out 
+   grep expected .tagger. line out
+'
+
 test_expect_success 'cleaned up' '
git fsck actual 21 
test_cmp empty actual
-- 
2.0.0.rc3.9669.g840d1f9

--
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 1/6] Refactor type_from_string() to avoid die()ing in case of errors

[Johannes Schindelin]

In the next commits, we will enhance the fsck_tag() function to check
tag objects more thoroughly. To this end, we need a function to verify
that a given string is a valid object type, but that does not die() in
the negative case.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 object.c | 13 -
 object.h |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/object.c b/object.c
index a16b9f9..5eee592 100644
--- a/object.c
+++ b/object.c
@@ -33,13 +33,24 @@ const char *typename(unsigned int type)
return object_type_strings[type];
 }
 
-int type_from_string(const char *str)
+int type_from_string_gently(const char *str)
 {
int i;
 
for (i = 1; i  ARRAY_SIZE(object_type_strings); i++)
if (!strcmp(str, object_type_strings[i]))
return i;
+
+   return -1;
+}
+
+int type_from_string(const char *str)
+{
+   int i = type_from_string_gently(str);
+
+   if (i = 0)
+   return i;
+
die(invalid object type \%s\, str);
 }
 
diff --git a/object.h b/object.h
index 5e8d8ee..5c5d22f 100644
--- a/object.h
+++ b/object.h
@@ -54,6 +54,7 @@ struct object {
 
 extern const char *typename(unsigned int type);
 extern int type_from_string(const char *str);
+extern int type_from_string_gently(const char *str);
 
 /*
  * Return the current number of buckets in the object hashmap.
-- 
2.0.0.rc3.9669.g840d1f9

--
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/6] Accept object data in the fsck_object() function

[Johannes Schindelin]

When fsck'ing an incoming pack, we need to fsck objects that cannot be
read via read_sha1_file() because they are not local yet (and might even
be rejected if transfer.fsckobjects is set to 'true').

For commits, there is a hack in place: we basically cache commit
objects' buffers anyway, but the same is not true, say, for tag objects.

By refactoring fsck_object() to take the object buffer and size as
optional arguments -- optional, because we still fall back to the
previous method to look at the cached commit objects if the caller
passes NULL -- we prepare the machinery for the upcoming handling of tag
objects.

The assumption that such buffers are inherently NUL terminated is now
wrong, of course, hence we pass the size of the buffer so that we can
add a sanity check later, to prevent running past the end of the buffer.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 builtin/fsck.c   |  2 +-
 builtin/index-pack.c |  3 ++-
 builtin/unpack-objects.c | 14 ++
 fsck.c   | 24 +++-
 fsck.h   |  4 +++-
 5 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index d42a27d..d9f4e6e 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -298,7 +298,7 @@ static int fsck_obj(struct object *obj)
 
if (fsck_walk(obj, mark_used, NULL))
objerror(obj, broken links);
-   if (fsck_object(obj, check_strict, fsck_error_func))
+   if (fsck_object(obj, NULL, 0, check_strict, fsck_error_func))
return -1;
 
if (obj-type == OBJ_TREE) {
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 5568a5b..f2465ff 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -773,7 +773,8 @@ static void sha1_object(const void *data, struct 
object_entry *obj_entry,
if (!obj)
die(_(invalid %s), typename(type));
if (do_fsck_object 
-   fsck_object(obj, 1, fsck_error_function))
+   fsck_object(obj, buf, size, 1,
+   fsck_error_function))
die(_(Error in object));
if (fsck_walk(obj, mark_link, NULL))
die(_(Not all child objects of %s are 
reachable), sha1_to_hex(obj-sha1));
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 99cde45..855d94b 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -164,10 +164,10 @@ static unsigned nr_objects;
  * Called only from check_object() after it verified this object
  * is Ok.
  */
-static void write_cached_object(struct object *obj)
+static void write_cached_object(struct object *obj, struct obj_buffer *obj_buf)
 {
unsigned char sha1[20];
-   struct obj_buffer *obj_buf = lookup_object_buffer(obj);
+
if (write_sha1_file(obj_buf-buffer, obj_buf-size, 
typename(obj-type), sha1)  0)
die(failed to write object %s, sha1_to_hex(obj-sha1));
obj-flags |= FLAG_WRITTEN;
@@ -180,6 +180,8 @@ static void write_cached_object(struct object *obj)
  */
 static int check_object(struct object *obj, int type, void *data)
 {
+   struct obj_buffer *obj_buf;
+
if (!obj)
return 1;
 
@@ -198,11 +200,15 @@ static int check_object(struct object *obj, int type, 
void *data)
return 0;
}
 
-   if (fsck_object(obj, 1, fsck_error_function))
+   obj_buf = lookup_object_buffer(obj);
+   if (!obj_buf)
+   die(Whoops! Cannot find object '%s', sha1_to_hex(obj-sha1));
+   if (fsck_object(obj, obj_buf-buffer, obj_buf-size, 1,
+   fsck_error_function))
die(Error in object);
if (fsck_walk(obj, check_object, NULL))
die(Error on reachable objects of %s, sha1_to_hex(obj-sha1));
-   write_cached_object(obj);
+   write_cached_object(obj, obj_buf);
return 0;
 }
 
diff --git a/fsck.c b/fsck.c
index 56156ff..dd77628 100644
--- a/fsck.c
+++ b/fsck.c
@@ -277,7 +277,7 @@ static int fsck_ident(const char **ident, struct object 
*obj, fsck_error error_f
 }
 
 static int fsck_commit_buffer(struct commit *commit, const char *buffer,
- fsck_error error_func)
+   unsigned long size, fsck_error error_func)
 {
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
@@ -322,15 +322,18 @@ static int fsck_commit_buffer(struct commit *commit, 
const char *buffer,
return 0;
 }
 
-static int fsck_commit(struct commit *commit, fsck_error error_func)
+static int fsck_commit(struct commit *commit, const char *data,
+   unsigned long size, fsck_error error_func)
 {
-   const char *buffer = get_commit_buffer(commit, NULL);
-   int ret = fsck_commit_buffer(commit, buffer, error_func);
-   unuse_commit_buffer(commit

[PATCH 3/6] Make sure fsck_commit_buffer() does not run out of the buffer

[Johannes Schindelin]

So far, we assumed that the buffer is NUL terminated, but this is not
a safe assumption, now that we opened the fsck_object() API to pass a
buffer directly.

So let's make sure that there is at least an empty line in the buffer.
That way, our checks would fail if the empty line was encountered
prematurely, and consequently we can get away with the current string
comparisons even with non-NUL-terminated buffers are passed to
fsck_object().

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 fsck.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/fsck.c b/fsck.c
index dd77628..db6aaa4 100644
--- a/fsck.c
+++ b/fsck.c
@@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, 
fsck_error error_func)
return retval;
 }
 
+static int must_have_empty_line(const void *data, unsigned long size,
+   struct object *obj, fsck_error error_func)
+{
+   const char *buffer = (const char *)data;
+   int i;
+
+   for (i = 0; i  size; i++) {
+   switch (buffer[i]) {
+   case '\0':
+   return error_func(obj, FSCK_ERROR,
+   invalid message: NUL at offset %d, i);
+   case '\n':
+   if (i + 1  size  buffer[i + 1] == '\n')
+   return 0;
+   }
+   }
+
+   return error_func(obj, FSCK_ERROR, invalid buffer: missing empty 
line);
+}
+
 static int fsck_ident(const char **ident, struct object *obj, fsck_error 
error_func)
 {
char *end;
@@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit, const 
char *buffer,
unsigned parent_count, parent_line_count = 0;
int err;
 
+   if (must_have_empty_line(buffer, size, commit-object, error_func))
+   return -1;
+
if (!skip_prefix(buffer, tree , buffer))
return error_func(commit-object, FSCK_ERROR, invalid format 
- expected 'tree' line);
if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
-- 
2.0.0.rc3.9669.g840d1f9

--
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 4/6] fsck: check tag objects' headers

[Johannes Schindelin]

We inspect commit objects pretty much in detail in git-fsck, but we just
glanced over the tag objects. Let's be stricter.

This work was sponsored by GitHub Inc.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 fsck.c | 88 +-
 1 file changed, 87 insertions(+), 1 deletion(-)

diff --git a/fsck.c b/fsck.c
index db6aaa4..d30946b 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
 #include commit.h
 #include tag.h
 #include fsck.h
+#include refs.h
 
 static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
 {
@@ -355,6 +356,90 @@ static int fsck_commit(struct commit *commit, const char 
*data,
return ret;
 }
 
+static int fsck_tag_buffer(struct tag *tag, const char *data,
+   unsigned long size, fsck_error error_func)
+{
+   unsigned char commit_sha1[20];
+   int ret = 0;
+   const char *buffer;
+   char *tmp = NULL, *eol;
+
+   if (data)
+   buffer = data;
+   else {
+   enum object_type type;
+
+   buffer = tmp = read_sha1_file(tag-object.sha1, type, size);
+   if (!buffer)
+   return error_func(tag-object, FSCK_ERROR,
+   cannot read tag object);
+
+   if (type != OBJ_TAG) {
+   ret = error_func(tag-object, FSCK_ERROR,
+   expected tag got %s,
+   typename(type));
+   goto done;
+   }
+   }
+
+   if (must_have_empty_line(buffer, size, tag-object, error_func))
+   goto done;
+
+   if (!skip_prefix(buffer, object , buffer)) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
expected 'object' line);
+   goto done;
+   }
+   if (get_sha1_hex(buffer, commit_sha1) || buffer[40] != '\n') {
+   ret = error_func(tag-object, FSCK_ERROR, invalid 'object' 
line format - bad sha1);
+   goto done;
+   }
+   buffer += 41;
+
+   if (!skip_prefix(buffer, type , buffer)) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
expected 'type' line);
+   goto done;
+   }
+   eol = strchr(buffer, '\n');
+   if (!eol) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
unexpected end after 'type' line);
+   goto done;
+   }
+   *eol = '\0';
+   if (type_from_string_gently(buffer)  0)
+   ret = error_func(tag-object, FSCK_ERROR, invalid 'type' 
value);
+   *eol = '\n';
+   if (ret)
+   goto done;
+   buffer = eol + 1;
+
+   if (!skip_prefix(buffer, tag , buffer)) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
expected 'tag' line);
+   goto done;
+   }
+   eol = strchr(buffer, '\n');
+   if (!eol) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
unexpected end after 'type' line);
+   goto done;
+   }
+   *eol = '\0';
+   if (check_refname_format(buffer, REFNAME_ALLOW_ONELEVEL))
+   ret = error_func(tag-object, FSCK_ERROR, invalid 'tag' name: 
%s, buffer);
+   *eol = '\n';
+   if (ret)
+   goto done;
+   buffer = eol + 1;
+
+   if (!skip_prefix(buffer, tagger , buffer)) {
+   /* early tags do not contain 'tagger' lines; warn only */
+   error_func(tag-object, FSCK_WARN, invalid format - expected 
'tagger' line);
+   }
+   ret = fsck_ident(buffer, tag-object, error_func);
+
+done:
+   free(tmp);
+   return ret;
+}
+
 static int fsck_tag(struct tag *tag, const char *data,
unsigned long size, fsck_error error_func)
 {
@@ -362,7 +447,8 @@ static int fsck_tag(struct tag *tag, const char *data,
 
if (!tagged)
return error_func(tag-object, FSCK_ERROR, could not load 
tagged object);
-   return 0;
+
+   return fsck_tag_buffer(tag, data, size, error_func);
 }
 
 int fsck_object(struct object *obj, void *data, unsigned long size,
-- 
2.0.0.rc3.9669.g840d1f9

--
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 0/6] Improve tag checking in fsck and with transfer.fsckobjects

[Johannes Schindelin]

This patch series introduces detailed checking of tag objects when calling
git fsck, and also when transfer.fsckobjects is set to true.

To this end, the fsck machinery is reworked to accept the buffer and size
of the object to check, and for commit and tag objects, we verify that the
buffers end in a NUL.

This work was sponsored by GitHub.

Johannes Schindelin (5):
  Refactor type_from_string() to avoid die()ing in case of errors
  fsck: inspect tag objects more closely
  Add regression tests for stricter tag fsck'ing
  Refactor out fsck_object_buffer() accepting the object data
  Make sure that index-pack --strict fails upon invalid tag objects

 builtin/index-pack.c |   3 +-
 builtin/unpack-objects.c |  14 --
 fsck.c   | 109 ---
 fsck.h   |   3 ++
 object.c |  13 +-
 object.h |   1 +
 t/t1450-fsck.sh  |  39 +
 t/t5302-pack-index.sh|  19 +
 8 files changed, 188 insertions(+), 13 deletions(-)

-- 
2.0.0.rc3.9669.g840d1f9
--
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/6] Accept object data in the fsck_object() function

[Johannes Schindelin]

When fsck'ing an incoming pack, we need to fsck objects that cannot be
read via read_sha1_file() because they are not local yet (and might even
be rejected if transfer.fsckobjects is set to 'true').

For commits, there is a hack in place: we basically cache commit
objects' buffers anyway, but the same is not true, say, for tag objects.

By refactoring fsck_object() to take the object buffer and size as
optional arguments -- optional, because we still fall back to the
previous method to look at the cached commit objects if the caller
passes NULL -- we prepare the machinery for the upcoming handling of tag
objects.

The assumption that such buffers are inherently NUL terminated is now
wrong, of course, hence we pass the size of the buffer so that we can
add a sanity check later, to prevent running past the end of the buffer.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 builtin/fsck.c   |  2 +-
 builtin/index-pack.c |  3 ++-
 builtin/unpack-objects.c | 14 ++
 fsck.c   | 24 +++-
 fsck.h   |  4 +++-
 5 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index d42a27d..d9f4e6e 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -298,7 +298,7 @@ static int fsck_obj(struct object *obj)
 
if (fsck_walk(obj, mark_used, NULL))
objerror(obj, broken links);
-   if (fsck_object(obj, check_strict, fsck_error_func))
+   if (fsck_object(obj, NULL, 0, check_strict, fsck_error_func))
return -1;
 
if (obj-type == OBJ_TREE) {
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 5568a5b..f2465ff 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -773,7 +773,8 @@ static void sha1_object(const void *data, struct 
object_entry *obj_entry,
if (!obj)
die(_(invalid %s), typename(type));
if (do_fsck_object 
-   fsck_object(obj, 1, fsck_error_function))
+   fsck_object(obj, buf, size, 1,
+   fsck_error_function))
die(_(Error in object));
if (fsck_walk(obj, mark_link, NULL))
die(_(Not all child objects of %s are 
reachable), sha1_to_hex(obj-sha1));
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 99cde45..855d94b 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -164,10 +164,10 @@ static unsigned nr_objects;
  * Called only from check_object() after it verified this object
  * is Ok.
  */
-static void write_cached_object(struct object *obj)
+static void write_cached_object(struct object *obj, struct obj_buffer *obj_buf)
 {
unsigned char sha1[20];
-   struct obj_buffer *obj_buf = lookup_object_buffer(obj);
+
if (write_sha1_file(obj_buf-buffer, obj_buf-size, 
typename(obj-type), sha1)  0)
die(failed to write object %s, sha1_to_hex(obj-sha1));
obj-flags |= FLAG_WRITTEN;
@@ -180,6 +180,8 @@ static void write_cached_object(struct object *obj)
  */
 static int check_object(struct object *obj, int type, void *data)
 {
+   struct obj_buffer *obj_buf;
+
if (!obj)
return 1;
 
@@ -198,11 +200,15 @@ static int check_object(struct object *obj, int type, 
void *data)
return 0;
}
 
-   if (fsck_object(obj, 1, fsck_error_function))
+   obj_buf = lookup_object_buffer(obj);
+   if (!obj_buf)
+   die(Whoops! Cannot find object '%s', sha1_to_hex(obj-sha1));
+   if (fsck_object(obj, obj_buf-buffer, obj_buf-size, 1,
+   fsck_error_function))
die(Error in object);
if (fsck_walk(obj, check_object, NULL))
die(Error on reachable objects of %s, sha1_to_hex(obj-sha1));
-   write_cached_object(obj);
+   write_cached_object(obj, obj_buf);
return 0;
 }
 
diff --git a/fsck.c b/fsck.c
index 56156ff..dd77628 100644
--- a/fsck.c
+++ b/fsck.c
@@ -277,7 +277,7 @@ static int fsck_ident(const char **ident, struct object 
*obj, fsck_error error_f
 }
 
 static int fsck_commit_buffer(struct commit *commit, const char *buffer,
- fsck_error error_func)
+   unsigned long size, fsck_error error_func)
 {
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
@@ -322,15 +322,18 @@ static int fsck_commit_buffer(struct commit *commit, 
const char *buffer,
return 0;
 }
 
-static int fsck_commit(struct commit *commit, fsck_error error_func)
+static int fsck_commit(struct commit *commit, const char *data,
+   unsigned long size, fsck_error error_func)
 {
-   const char *buffer = get_commit_buffer(commit, NULL);
-   int ret = fsck_commit_buffer(commit, buffer, error_func);
-   unuse_commit_buffer(commit

[PATCH v2 1/6] Refactor type_from_string() to avoid die()ing in case of errors

[Johannes Schindelin]

In the next commits, we will enhance the fsck_tag() function to check
tag objects more thoroughly. To this end, we need a function to verify
that a given string is a valid object type, but that does not die() in
the negative case.

While at it, prepare type_from_string() for counted strings, i.e. strings
with an explicitly specified length rather than a NUL termination.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 object.c | 11 +--
 object.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/object.c b/object.c
index a16b9f9..aedac24 100644
--- a/object.c
+++ b/object.c
@@ -33,13 +33,20 @@ const char *typename(unsigned int type)
return object_type_strings[type];
 }
 
-int type_from_string(const char *str)
+int type_from_string_gently(const char *str, ssize_t len, int gentle)
 {
int i;
 
+   if (len  0)
+   len = strlen(str);
+
for (i = 1; i  ARRAY_SIZE(object_type_strings); i++)
-   if (!strcmp(str, object_type_strings[i]))
+   if (!strncmp(str, object_type_strings[i], len))
return i;
+
+   if (gentle)
+   return -1;
+
die(invalid object type \%s\, str);
 }
 
diff --git a/object.h b/object.h
index 5e8d8ee..e028ced 100644
--- a/object.h
+++ b/object.h
@@ -53,7 +53,8 @@ struct object {
 };
 
 extern const char *typename(unsigned int type);
-extern int type_from_string(const char *str);
+extern int type_from_string_gently(const char *str, ssize_t, int gentle);
+#define type_from_string(str) type_from_string_gently(str, -1, 0)
 
 /*
  * Return the current number of buckets in the object hashmap.
-- 
2.0.0.rc3.9669.g840d1f9

--
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 0/6] Improve tag checking in fsck and with transfer.fsckobjects

[Johannes Schindelin]

This patch series introduces detailed checking of tag objects when calling
git fsck, and also when transfer.fsckobjects is set to true.

To this end, the fsck machinery is reworked to accept the buffer and size
of the object to check, and for commit and tag objects, we verify that the
buffers contain an end of header (i.e. an empty line) to guarantee that our
checks do not run beyond the buffer.

This work was sponsored by GitHub.

Changes since v1:

- Let type_from_string_gently() accept an optional string length

- Renamed 'tmp' to 'to_free'

- Renamed must_have_empty_line() to require_end_of_header()

- Renamed 'commit_sha1' in fsck_tag_buffer to 'sha1'

- Avoided temporarily inserting NUL characters into the buffer

- Demoted problematic 'tag' value to a mere warning

- Mentioned GitHub only in the cover letter, i.e. keeping it out of the git log

Still unaddressed:

- getting rid of struct object altogether in fsck (I felt this was quite a big
  task, getting much more familiar with the non-tag code paths, and I did not
  want to delay this patch series up any further)

- ensuring that index-pack passes only NUL-terminated buffers to fsck (again,
  I am not familiar enough with the code, and IIRC the problematic unit test
  that revealed that these buffers are not always NUL-terminated exercised the
  unpack-objects code path, not index-pack, again nothing I wanted to let
  delay this patch series any further).

Johannes Schindelin (6):
  Refactor type_from_string() to avoid die()ing in case of errors
  Accept object data in the fsck_object() function
  Make sure fsck_commit_buffer() does not run out of the buffer
  fsck: check tag objects' headers
  Add regression tests for stricter tag fsck'ing
  Make sure that index-pack --strict fails upon invalid tag objects

 builtin/fsck.c   |   2 +-
 builtin/index-pack.c |   3 +-
 builtin/unpack-objects.c |  14 +++--
 fsck.c   | 132 +++
 fsck.h   |   4 +-
 object.c |  11 +++-
 object.h |   3 +-
 t/t1450-fsck.sh  |  39 ++
 t/t5302-pack-index.sh|  19 +++
 9 files changed, 207 insertions(+), 20 deletions(-)

-- 
2.0.0.rc3.9669.g840d1f9
--
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 4/6] fsck: check tag objects' headers

[Johannes Schindelin]

We inspect commit objects pretty much in detail in git-fsck, but we just
glanced over the tag objects. Let's be stricter.

Since we do not want to limit 'tag' lines unduly, values that would fail
the refname check only result in warnings, not errors.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 fsck.c | 85 +-
 1 file changed, 84 insertions(+), 1 deletion(-)

diff --git a/fsck.c b/fsck.c
index 9dd7d12..8341a30 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
 #include commit.h
 #include tag.h
 #include fsck.h
+#include refs.h
 
 static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
 {
@@ -355,6 +356,87 @@ static int fsck_commit(struct commit *commit, const char 
*data,
return ret;
 }
 
+static int fsck_tag_buffer(struct tag *tag, const char *data,
+   unsigned long size, fsck_error error_func)
+{
+   unsigned char sha1[20];
+   int ret = 0;
+   const char *buffer;
+   char *to_free = NULL, *eol;
+   struct strbuf sb = STRBUF_INIT;
+
+   if (data)
+   buffer = data;
+   else {
+   enum object_type type;
+
+   buffer = to_free =
+   read_sha1_file(tag-object.sha1, type, size);
+   if (!buffer)
+   return error_func(tag-object, FSCK_ERROR,
+   cannot read tag object);
+
+   if (type != OBJ_TAG) {
+   ret = error_func(tag-object, FSCK_ERROR,
+   expected tag got %s,
+   typename(type));
+   goto done;
+   }
+   }
+
+   if (require_end_of_header(buffer, size, tag-object, error_func))
+   goto done;
+
+   if (!skip_prefix(buffer, object , buffer)) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
expected 'object' line);
+   goto done;
+   }
+   if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n') {
+   ret = error_func(tag-object, FSCK_ERROR, invalid 'object' 
line format - bad sha1);
+   goto done;
+   }
+   buffer += 41;
+
+   if (!skip_prefix(buffer, type , buffer)) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
expected 'type' line);
+   goto done;
+   }
+   eol = strchr(buffer, '\n');
+   if (!eol) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
unexpected end after 'type' line);
+   goto done;
+   }
+   if (type_from_string_gently(buffer, eol - buffer, 1)  0)
+   ret = error_func(tag-object, FSCK_ERROR, invalid 'type' 
value);
+   if (ret)
+   goto done;
+   buffer = eol + 1;
+
+   if (!skip_prefix(buffer, tag , buffer)) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
expected 'tag' line);
+   goto done;
+   }
+   eol = strchr(buffer, '\n');
+   if (!eol) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
unexpected end after 'type' line);
+   goto done;
+   }
+   strbuf_addf(sb, refs/tags/%.*s, (int)(eol - buffer), buffer);
+   if (check_refname_format(sb.buf, 0))
+   error_func(tag-object, FSCK_WARN, invalid 'tag' name: %s, 
buffer);
+   buffer = eol + 1;
+
+   if (!skip_prefix(buffer, tagger , buffer)) {
+   /* early tags do not contain 'tagger' lines; warn only */
+   error_func(tag-object, FSCK_WARN, invalid format - expected 
'tagger' line);
+   }
+   ret = fsck_ident(buffer, tag-object, error_func);
+
+done:
+   free(to_free);
+   return ret;
+}
+
 static int fsck_tag(struct tag *tag, const char *data,
unsigned long size, fsck_error error_func)
 {
@@ -362,7 +444,8 @@ static int fsck_tag(struct tag *tag, const char *data,
 
if (!tagged)
return error_func(tag-object, FSCK_ERROR, could not load 
tagged object);
-   return 0;
+
+   return fsck_tag_buffer(tag, data, size, error_func);
 }
 
 int fsck_object(struct object *obj, void *data, unsigned long size,
-- 
2.0.0.rc3.9669.g840d1f9

--
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 3/6] Make sure fsck_commit_buffer() does not run out of the buffer

[Johannes Schindelin]

So far, we assumed that the buffer is NUL terminated, but this is not
a safe assumption, now that we opened the fsck_object() API to pass a
buffer directly.

So let's make sure that there is at least an empty line in the buffer.
That way, our checks would fail if the empty line was encountered
prematurely, and consequently we can get away with the current string
comparisons even with non-NUL-terminated buffers are passed to
fsck_object().

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 fsck.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/fsck.c b/fsck.c
index dd77628..9dd7d12 100644
--- a/fsck.c
+++ b/fsck.c
@@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, 
fsck_error error_func)
return retval;
 }
 
+static int require_end_of_header(const void *data, unsigned long size,
+   struct object *obj, fsck_error error_func)
+{
+   const char *buffer = (const char *)data;
+   int i;
+
+   for (i = 0; i  size; i++) {
+   switch (buffer[i]) {
+   case '\0':
+   return error_func(obj, FSCK_ERROR,
+   invalid message: NUL at offset %d, i);
+   case '\n':
+   if (i + 1  size  buffer[i + 1] == '\n')
+   return 0;
+   }
+   }
+
+   return error_func(obj, FSCK_ERROR, invalid buffer: missing empty 
line);
+}
+
 static int fsck_ident(const char **ident, struct object *obj, fsck_error 
error_func)
 {
char *end;
@@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit, const 
char *buffer,
unsigned parent_count, parent_line_count = 0;
int err;
 
+   if (require_end_of_header(buffer, size, commit-object, error_func))
+   return -1;
+
if (!skip_prefix(buffer, tree , buffer))
return error_func(commit-object, FSCK_ERROR, invalid format 
- expected 'tree' line);
if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
-- 
2.0.0.rc3.9669.g840d1f9

--
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 6/6] Make sure that index-pack --strict fails upon invalid tag objects

[Johannes Schindelin]

One of the most important use cases for the strict tag object checking
is when transfer.fsckobjects is set to true to catch invalid objects
early on. This new regression test essentially tests the same code path
by directly calling 'index-pack --strict' on a pack containing an
invalid tag object.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 t/t5302-pack-index.sh | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index 4bbb718..80bd3ee 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -243,4 +243,23 @@ test_expect_success 'running index-pack in the object 
store' '
 test -f .git/objects/pack/pack-${pack1}.idx
 '
 
+test_expect_success 'index-pack --strict fails upon invalid tag' '
+sha=$(git rev-parse HEAD) 
+cat wrong-tag EOF 
+object $sha
+type commit
+tag guten tag
+
+This is an invalid tag.
+EOF
+
+tag=$(git hash-object -t tag -w --stdin wrong-tag) 
+pack1=$(echo $tag | git pack-objects tag-test) 
+echo remove tag object 
+thirtyeight=${tag#??} 
+rm -f .git/objects/${tag%$thirtyeight}/$thirtyeight 
+test_must_fail git index-pack --strict tag-test-${pack1}.pack 2 err 
+grep invalid .tag. name err
+'
+
 test_done
-- 
2.0.0.rc3.9669.g840d1f9
--
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 5/6] Add regression tests for stricter tag fsck'ing

[Johannes Schindelin]

The intent of the two new test cases is to catch general breakages in
the fsck_tag() function, not so much to test it extensively, trying to
strike the proper balance between thoroughness and speed.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 t/t1450-fsck.sh | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 8c739c9..16b3e4a 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -194,6 +194,45 @@ test_expect_success 'tag pointing to something else than 
its type' '
test_must_fail git fsck --tags
 '
 
+test_expect_success 'tag with incorrect tag name' '
+   sha=$(git rev-parse HEAD) 
+   cat wrong-tag -EOF 
+   object $sha
+   type commit
+   tag wrong name format
+   tagger T A Gger tag...@example.com 1234567890 -
+
+   This is an invalid tag.
+   EOF
+
+   tag=$(git hash-object -t tag -w --stdin wrong-tag) 
+   test_when_finished remove_object $tag 
+   echo $tag .git/refs/tags/wrong 
+   test_when_finished git update-ref -d refs/tags/wrong 
+   git fsck --tags 2out 
+   cat out 
+   grep invalid .tag. name out
+'
+
+test_expect_success 'tag with missing tagger' '
+   sha=$(git rev-parse HEAD) 
+   cat wrong-tag -EOF 
+   object $sha
+   type commit
+   tag gutentag
+
+   This is an invalid tag.
+   EOF
+
+   tag=$(git hash-object -t tag -w --stdin wrong-tag) 
+   test_when_finished remove_object $tag 
+   echo $tag .git/refs/tags/wrong 
+   test_when_finished git update-ref -d refs/tags/wrong 
+   git fsck --tags 2out 
+   cat out 
+   grep expected .tagger. line out
+'
+
 test_expect_success 'cleaned up' '
git fsck actual 21 
test_cmp empty actual
-- 
2.0.0.rc3.9669.g840d1f9

--
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 v2 0/6] Improve tag checking in fsck and with transfer.fsckobjects

[Johannes Schindelin]

Hi Junio,

On Wed, 10 Sep 2014, Johannes Schindelin wrote:

 Still unaddressed:
 
 - getting rid of struct object altogether in fsck (I felt this was quite a big
   task, getting much more familiar with the non-tag code paths, and I did not
   want to delay this patch series up any further)
 
 - ensuring that index-pack passes only NUL-terminated buffers to fsck (again,
   I am not familiar enough with the code, and IIRC the problematic unit test
   that revealed that these buffers are not always NUL-terminated exercised the
   unpack-objects code path, not index-pack, again nothing I wanted to let
   delay this patch series any further).

To clarify: I am planning to address these issues later this year, but
consider them not critical enough to finish the fsck-tag patch series.

Please let me know if you disagree.

Ciao,
Dscho
--
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 v2 3/6] Make sure fsck_commit_buffer() does not run out of the buffer

[Johannes Schindelin]

Hi Junio,

On Wed, 10 Sep 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  diff --git a/fsck.c b/fsck.c
  index dd77628..9dd7d12 100644
  --- a/fsck.c
  +++ b/fsck.c
  @@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, 
  fsck_error error_func)
  return retval;
   }
   
  +static int require_end_of_header(const void *data, unsigned long size,
  +   struct object *obj, fsck_error error_func)
  +{
  +   const char *buffer = (const char *)data;
  +   int i;
  +
  +   for (i = 0; i  size; i++) {
  +   switch (buffer[i]) {
  +   case '\0':
  +   return error_func(obj, FSCK_ERROR,
  +   invalid message: NUL at offset %d, i);
 
 Isn't this invalid header?  After all we haven't escaped this loop
 and haven't seen the message part of the commit object (and it is
 the same if you are going to later reuse this for tag objects).

My reasoning for keeping it saying message was that a message consists
of a header and a body. I will change it to unterminated header instead,
also in the error message when no NUL was found.

Ciao,
Dscho
--
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 v2 5/6] Add regression tests for stricter tag fsck'ing

[Johannes Schindelin]

Hi Junio,

On Wed, 10 Sep 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  +   test_when_finished git update-ref -d refs/tags/wrong 
  +   git fsck --tags 2out 
 
 I wonder what the command does with or without --tags option
 (applies to both tests added by this patch)?
 
 Does running fsck without the option not to report broken tags
 detected by the new checks added in this series?  Should it?

fsck fails perfectly fine without the --tags option; this option just
triggers that fsck will show also the objects referenced by the tag
objects (which is nice for debugging).

Ciao,
Dscho
--
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 v2 6/6] Make sure that index-pack --strict fails upon invalid tag objects

[Johannes Schindelin]

Hi,

On Wed, 10 Sep 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  +tag=$(git hash-object -t tag -w --stdin wrong-tag) 
  +pack1=$(echo $tag | git pack-objects tag-test) 
  +echo remove tag object 
  +thirtyeight=${tag#??} 
  +rm -f .git/objects/${tag%$thirtyeight}/$thirtyeight 
  +test_must_fail git index-pack --strict tag-test-${pack1}.pack 2 err 
 
 I had to drop must fail from this one (will amend the SQUASH???
 again).

Funny. It failed here, but for the wrong reason: index-pack --strict
failed here because the object referenced by the tag object was not in the
pack.

Ciao,
Dscho
--
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 2/6] Accept object data in the fsck_object() function

[Johannes Schindelin]

When fsck'ing an incoming pack, we need to fsck objects that cannot be
read via read_sha1_file() because they are not local yet (and might even
be rejected if transfer.fsckobjects is set to 'true').

For commits, there is a hack in place: we basically cache commit
objects' buffers anyway, but the same is not true, say, for tag objects.

By refactoring fsck_object() to take the object buffer and size as
optional arguments -- optional, because we still fall back to the
previous method to look at the cached commit objects if the caller
passes NULL -- we prepare the machinery for the upcoming handling of tag
objects.

The assumption that such buffers are inherently NUL terminated is now
wrong, of course, hence we pass the size of the buffer so that we can
add a sanity check later, to prevent running past the end of the buffer.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 builtin/fsck.c   |  2 +-
 builtin/index-pack.c |  3 ++-
 builtin/unpack-objects.c | 14 ++
 fsck.c   | 24 +++-
 fsck.h   |  4 +++-
 5 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index d42a27d..d9f4e6e 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -298,7 +298,7 @@ static int fsck_obj(struct object *obj)
 
if (fsck_walk(obj, mark_used, NULL))
objerror(obj, broken links);
-   if (fsck_object(obj, check_strict, fsck_error_func))
+   if (fsck_object(obj, NULL, 0, check_strict, fsck_error_func))
return -1;
 
if (obj-type == OBJ_TREE) {
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 5568a5b..f2465ff 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -773,7 +773,8 @@ static void sha1_object(const void *data, struct 
object_entry *obj_entry,
if (!obj)
die(_(invalid %s), typename(type));
if (do_fsck_object 
-   fsck_object(obj, 1, fsck_error_function))
+   fsck_object(obj, buf, size, 1,
+   fsck_error_function))
die(_(Error in object));
if (fsck_walk(obj, mark_link, NULL))
die(_(Not all child objects of %s are 
reachable), sha1_to_hex(obj-sha1));
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 99cde45..855d94b 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -164,10 +164,10 @@ static unsigned nr_objects;
  * Called only from check_object() after it verified this object
  * is Ok.
  */
-static void write_cached_object(struct object *obj)
+static void write_cached_object(struct object *obj, struct obj_buffer *obj_buf)
 {
unsigned char sha1[20];
-   struct obj_buffer *obj_buf = lookup_object_buffer(obj);
+
if (write_sha1_file(obj_buf-buffer, obj_buf-size, 
typename(obj-type), sha1)  0)
die(failed to write object %s, sha1_to_hex(obj-sha1));
obj-flags |= FLAG_WRITTEN;
@@ -180,6 +180,8 @@ static void write_cached_object(struct object *obj)
  */
 static int check_object(struct object *obj, int type, void *data)
 {
+   struct obj_buffer *obj_buf;
+
if (!obj)
return 1;
 
@@ -198,11 +200,15 @@ static int check_object(struct object *obj, int type, 
void *data)
return 0;
}
 
-   if (fsck_object(obj, 1, fsck_error_function))
+   obj_buf = lookup_object_buffer(obj);
+   if (!obj_buf)
+   die(Whoops! Cannot find object '%s', sha1_to_hex(obj-sha1));
+   if (fsck_object(obj, obj_buf-buffer, obj_buf-size, 1,
+   fsck_error_function))
die(Error in object);
if (fsck_walk(obj, check_object, NULL))
die(Error on reachable objects of %s, sha1_to_hex(obj-sha1));
-   write_cached_object(obj);
+   write_cached_object(obj, obj_buf);
return 0;
 }
 
diff --git a/fsck.c b/fsck.c
index 56156ff..dd77628 100644
--- a/fsck.c
+++ b/fsck.c
@@ -277,7 +277,7 @@ static int fsck_ident(const char **ident, struct object 
*obj, fsck_error error_f
 }
 
 static int fsck_commit_buffer(struct commit *commit, const char *buffer,
- fsck_error error_func)
+   unsigned long size, fsck_error error_func)
 {
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
@@ -322,15 +322,18 @@ static int fsck_commit_buffer(struct commit *commit, 
const char *buffer,
return 0;
 }
 
-static int fsck_commit(struct commit *commit, fsck_error error_func)
+static int fsck_commit(struct commit *commit, const char *data,
+   unsigned long size, fsck_error error_func)
 {
-   const char *buffer = get_commit_buffer(commit, NULL);
-   int ret = fsck_commit_buffer(commit, buffer, error_func);
-   unuse_commit_buffer(commit

[PATCH v3 1/6] Refactor type_from_string() to avoid die()ing in case of errors

[Johannes Schindelin]

In the next commits, we will enhance the fsck_tag() function to check
tag objects more thoroughly. To this end, we need a function to verify
that a given string is a valid object type, but that does not die() in
the negative case.

While at it, prepare type_from_string() for counted strings, i.e. strings
with an explicitly specified length rather than a NUL termination.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 object.c | 11 +--
 object.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/object.c b/object.c
index a16b9f9..aedac24 100644
--- a/object.c
+++ b/object.c
@@ -33,13 +33,20 @@ const char *typename(unsigned int type)
return object_type_strings[type];
 }
 
-int type_from_string(const char *str)
+int type_from_string_gently(const char *str, ssize_t len, int gentle)
 {
int i;
 
+   if (len  0)
+   len = strlen(str);
+
for (i = 1; i  ARRAY_SIZE(object_type_strings); i++)
-   if (!strcmp(str, object_type_strings[i]))
+   if (!strncmp(str, object_type_strings[i], len))
return i;
+
+   if (gentle)
+   return -1;
+
die(invalid object type \%s\, str);
 }
 
diff --git a/object.h b/object.h
index 5e8d8ee..e028ced 100644
--- a/object.h
+++ b/object.h
@@ -53,7 +53,8 @@ struct object {
 };
 
 extern const char *typename(unsigned int type);
-extern int type_from_string(const char *str);
+extern int type_from_string_gently(const char *str, ssize_t, int gentle);
+#define type_from_string(str) type_from_string_gently(str, -1, 0)
 
 /*
  * Return the current number of buckets in the object hashmap.
-- 
2.0.0.rc3.9669.g840d1f9

--
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 0/6] Improve tag checking in fsck and with transfer.fsckobjects

[Johannes Schindelin]

This patch series introduces detailed checking of tag objects when calling
git fsck, and also when transfer.fsckobjects is set to true.

To this end, the fsck machinery is reworked to accept the buffer and size
of the object to check, and for commit and tag objects, we verify that the
buffers contain an end of header (i.e. an empty line) to guarantee that our
checks do not run beyond the buffer.

This work was sponsored by GitHub.

Changes since v2:

- replaced 'invalid message' with 'unterminated header'

- avoided comparison between int and unsigned long (thanks, Eric Sunshine)

- made ident parsing conditional on finding the optional 'tagger' line

- added forgotten strbuf_release()

Still unaddressed:

- getting rid of struct object altogether in fsck (I felt this was quite a big
  task, getting much more familiar with the non-tag code paths, and I did not
  want to delay this patch series up any further)

- ensuring that index-pack passes only NUL-terminated buffers to fsck (again,
  I am not familiar enough with the code, and IIRC the problematic unit test
  that revealed that these buffers are not always NUL-terminated exercised the
  unpack-objects code path, not index-pack, again nothing I wanted to let
  delay this patch series any further).

Johannes Schindelin (6):
  Refactor type_from_string() to avoid die()ing in case of errors
  Accept object data in the fsck_object() function
  Make sure fsck_commit_buffer() does not run out of the buffer
  fsck: check tag objects' headers
  Add regression tests for stricter tag fsck'ing
  Make sure that index-pack --strict checks tag objects

 builtin/fsck.c   |   2 +-
 builtin/index-pack.c |   3 +-
 builtin/unpack-objects.c |  14 +++--
 fsck.c   | 133 +++
 fsck.h   |   4 +-
 object.c |  11 +++-
 object.h |   3 +-
 t/t1450-fsck.sh  |  20 +++
 t/t5302-pack-index.sh|  19 +++
 9 files changed, 189 insertions(+), 20 deletions(-)

-- 
2.0.0.rc3.9669.g840d1f9
--
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 3/6] Make sure fsck_commit_buffer() does not run out of the buffer

[Johannes Schindelin]

So far, we assumed that the buffer is NUL terminated, but this is not
a safe assumption, now that we opened the fsck_object() API to pass a
buffer directly.

So let's make sure that there is at least an empty line in the buffer.
That way, our checks would fail if the empty line was encountered
prematurely, and consequently we can get away with the current string
comparisons even with non-NUL-terminated buffers are passed to
fsck_object().

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 fsck.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/fsck.c b/fsck.c
index dd77628..73da6f8 100644
--- a/fsck.c
+++ b/fsck.c
@@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, 
fsck_error error_func)
return retval;
 }
 
+static int require_end_of_header(const void *data, unsigned long size,
+   struct object *obj, fsck_error error_func)
+{
+   const char *buffer = (const char *)data;
+   unsigned long i;
+
+   for (i = 0; i  size; i++) {
+   switch (buffer[i]) {
+   case '\0':
+   return error_func(obj, FSCK_ERROR,
+   unterminated header: NUL at offset %d, i);
+   case '\n':
+   if (i + 1  size  buffer[i + 1] == '\n')
+   return 0;
+   }
+   }
+
+   return error_func(obj, FSCK_ERROR, unterminated header);
+}
+
 static int fsck_ident(const char **ident, struct object *obj, fsck_error 
error_func)
 {
char *end;
@@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit, const 
char *buffer,
unsigned parent_count, parent_line_count = 0;
int err;
 
+   if (require_end_of_header(buffer, size, commit-object, error_func))
+   return -1;
+
if (!skip_prefix(buffer, tree , buffer))
return error_func(commit-object, FSCK_ERROR, invalid format 
- expected 'tree' line);
if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
-- 
2.0.0.rc3.9669.g840d1f9

--
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 6/6] Make sure that index-pack --strict checks tag objects

[Johannes Schindelin]

One of the most important use cases for the strict tag object checking
is when transfer.fsckobjects is set to true to catch invalid objects
early on. This new regression test essentially tests the same code path
by directly calling 'index-pack --strict' on a pack containing an
tag object without a 'tagger' line.

Technically, this test is not enough: it only exercises a code path that
*warns*, not one that *fails*. The reason is that it would be exquisitely
convoluted to test that: not only hash-object, but also pack-index
actually *parse* tag objects when encountering them. Therefore we would
have to actively *break* pack-index in order to test this. Or rewrite
both hash-object and pack-index in shell script. Ain't gonna happen.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 t/t5302-pack-index.sh | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index 4bbb718..4d033df 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -243,4 +243,23 @@ test_expect_success 'running index-pack in the object 
store' '
 test -f .git/objects/pack/pack-${pack1}.idx
 '
 
+test_expect_success 'index-pack --strict warns upon missing tagger in tag' '
+sha=$(git rev-parse HEAD) 
+cat wrong-tag EOF 
+object $sha
+type commit
+tag guten tag
+
+This is an invalid tag.
+EOF
+
+tag=$(git hash-object -t tag -w --stdin wrong-tag) 
+pack1=$(echo $tag $sha | git pack-objects tag-test) 
+echo remove tag object 
+thirtyeight=${tag#??} 
+rm -f .git/objects/${tag%$thirtyeight}/$thirtyeight 
+git index-pack --strict tag-test-${pack1}.pack 2 err 
+grep ^error:.* expected .tagger. line err
+'
+
 test_done
-- 
2.0.0.rc3.9669.g840d1f9
--
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 v2 0/6] Improve tag checking in fsck and with transfer.fsckobjects

[Johannes Schindelin]

Hi Junio,

On Wed, 10 Sep 2014, Junio C Hamano wrote:

 Junio C Hamano gits...@pobox.com writes:
 
  Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  This patch series introduces detailed checking of tag objects when calling
  git fsck, and also when transfer.fsckobjects is set to true.
 
  To this end, the fsck machinery is reworked to accept the buffer and size
  of the object to check, and for commit and tag objects, we verify that the
  buffers contain an end of header (i.e. an empty line) to guarantee that our
  checks do not run beyond the buffer.
 
  Overall they looked sensible and I am trying to queue them on 'pu'
  for today's pushout.
 
  Thanks.
 
 I was expecting to see interesting interactions with the oops, fsck
 was not exiting with non-zero status in some cases fix by Peff with
 the patch 5/6 of this series that adds two new tests to t1450, but
 because the breakage in these two cases are both only warning-events
 and not error-events, I didn't prefix git fsck with test_must_fail
 after all.

There were a couple of issues, thanks for pointing out that I missed
something. Short story: hash-object, fsck *and* pack-objects parse tag
objects as they are encountered. Therefore, it would be a bit hard to test
because we would have to emulate broken hash-object and pack-objects to
generate a pack containing an invalid tag object.

As a compromise, I now test for the warnings to make sure that the code
path is triggered, but do not explicitly test with a pack that would make
index-pack --strict *fail*.

Okay?

Ciao,
Dscho

P.S.: I squashed your changes in slightly modified form.
--
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 4/6] fsck: check tag objects' headers

[Johannes Schindelin]

We inspect commit objects pretty much in detail in git-fsck, but we just
glanced over the tag objects. Let's be stricter.

Since we do not want to limit 'tag' lines unduly, values that would fail
the refname check only result in warnings, not errors.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 fsck.c | 86 +-
 1 file changed, 85 insertions(+), 1 deletion(-)

diff --git a/fsck.c b/fsck.c
index 73da6f8..2fffa43 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
 #include commit.h
 #include tag.h
 #include fsck.h
+#include refs.h
 
 static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
 {
@@ -355,6 +356,88 @@ static int fsck_commit(struct commit *commit, const char 
*data,
return ret;
 }
 
+static int fsck_tag_buffer(struct tag *tag, const char *data,
+   unsigned long size, fsck_error error_func)
+{
+   unsigned char sha1[20];
+   int ret = 0;
+   const char *buffer;
+   char *to_free = NULL, *eol;
+   struct strbuf sb = STRBUF_INIT;
+
+   if (data)
+   buffer = data;
+   else {
+   enum object_type type;
+
+   buffer = to_free =
+   read_sha1_file(tag-object.sha1, type, size);
+   if (!buffer)
+   return error_func(tag-object, FSCK_ERROR,
+   cannot read tag object);
+
+   if (type != OBJ_TAG) {
+   ret = error_func(tag-object, FSCK_ERROR,
+   expected tag got %s,
+   typename(type));
+   goto done;
+   }
+   }
+
+   if (require_end_of_header(buffer, size, tag-object, error_func))
+   goto done;
+
+   if (!skip_prefix(buffer, object , buffer)) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
expected 'object' line);
+   goto done;
+   }
+   if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n') {
+   ret = error_func(tag-object, FSCK_ERROR, invalid 'object' 
line format - bad sha1);
+   goto done;
+   }
+   buffer += 41;
+
+   if (!skip_prefix(buffer, type , buffer)) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
expected 'type' line);
+   goto done;
+   }
+   eol = strchr(buffer, '\n');
+   if (!eol) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
unexpected end after 'type' line);
+   goto done;
+   }
+   if (type_from_string_gently(buffer, eol - buffer, 1)  0)
+   ret = error_func(tag-object, FSCK_ERROR, invalid 'type' 
value);
+   if (ret)
+   goto done;
+   buffer = eol + 1;
+
+   if (!skip_prefix(buffer, tag , buffer)) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
expected 'tag' line);
+   goto done;
+   }
+   eol = strchr(buffer, '\n');
+   if (!eol) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
unexpected end after 'type' line);
+   goto done;
+   }
+   strbuf_addf(sb, refs/tags/%.*s, (int)(eol - buffer), buffer);
+   if (check_refname_format(sb.buf, 0))
+   error_func(tag-object, FSCK_WARN, invalid 'tag' name: %s, 
buffer);
+   buffer = eol + 1;
+
+   if (!skip_prefix(buffer, tagger , buffer))
+   /* early tags do not contain 'tagger' lines; warn only */
+   error_func(tag-object, FSCK_WARN, invalid format - expected 
'tagger' line);
+   else
+   ret = fsck_ident(buffer, tag-object, error_func);
+
+done:
+   strbuf_release(sb);
+   free(to_free);
+   return ret;
+}
+
 static int fsck_tag(struct tag *tag, const char *data,
unsigned long size, fsck_error error_func)
 {
@@ -362,7 +445,8 @@ static int fsck_tag(struct tag *tag, const char *data,
 
if (!tagged)
return error_func(tag-object, FSCK_ERROR, could not load 
tagged object);
-   return 0;
+
+   return fsck_tag_buffer(tag, data, size, error_func);
 }
 
 int fsck_object(struct object *obj, void *data, unsigned long size,
-- 
2.0.0.rc3.9669.g840d1f9

--
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 5/6] Add regression tests for stricter tag fsck'ing

[Johannes Schindelin]

The intent of the new test case is to catch general breakages in
the fsck_tag() function, not so much to test it extensively, trying to
strike the proper balance between thoroughness and speed.

While it *would* have been nice to test the code path where fsck_object()
encounters an invalid tag object, this is not possible using git fsck: tag
objects are parsed already before fsck'ing (and the parser already fails
upon such objects).

Even worse: we would not even be able write out invalid tag objects
because git hash-object parses those objects, too, unless we resorted to
really ugly hacks such as using something like this in the unit tests
(essentially depending on Perl *and* Compress::Zlib):

hash_invalid_object () {
contents=$(printf '%s %d\0%s' $1 ${#2} $2) 
sha1=$(echo $contents | test-sha1) 
suffix=${sha1#??} 
mkdir -p .git/objects/${sha1%$suffix} 
echo $contents |
perl -MCompress::Zlib -e 'undef $/; print compress()' \
 .git/objects/${sha1%$suffix}/$suffix 
echo $sha1
}

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 t/t1450-fsck.sh | 20 
 1 file changed, 20 insertions(+)

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 8c739c9..2b6a6f2 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -194,6 +194,26 @@ test_expect_success 'tag pointing to something else than 
its type' '
test_must_fail git fsck --tags
 '
 
+test_expect_success 'tag with incorrect tag name  missing tagger' '
+   sha=$(git rev-parse HEAD) 
+   cat wrong-tag -EOF 
+   object $sha
+   type commit
+   tag wrong name format
+
+   This is an invalid tag.
+   EOF
+
+   tag=$(git hash-object -t tag -w --stdin wrong-tag) 
+   test_when_finished remove_object $tag 
+   echo $tag .git/refs/tags/wrong 
+   test_when_finished git update-ref -d refs/tags/wrong 
+   git fsck --tags 2out 
+   cat out 
+   grep invalid .tag. name out 
+   grep expected .tagger. line out
+'
+
 test_expect_success 'cleaned up' '
git fsck actual 21 
test_cmp empty actual
-- 
2.0.0.rc3.9669.g840d1f9

--
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 v2 6/6] Make sure that index-pack --strict fails upon invalid tag objects

[Johannes Schindelin]

Hi Junio,

On Thu, 11 Sep 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  On Wed, 10 Sep 2014, Junio C Hamano wrote:
 
  Johannes Schindelin johannes.schinde...@gmx.de writes:
  
   +tag=$(git hash-object -t tag -w --stdin wrong-tag) 
   +pack1=$(echo $tag | git pack-objects tag-test) 
   +echo remove tag object 
   +thirtyeight=${tag#??} 
   +rm -f .git/objects/${tag%$thirtyeight}/$thirtyeight 
   +test_must_fail git index-pack --strict tag-test-${pack1}.pack 2 
   err 
  
  I had to drop must fail from this one (will amend the SQUASH???
  again).
 
  Funny. It failed here, but for the wrong reason: index-pack --strict
  failed here because the object referenced by the tag object was not in the
  pack.
 
 That is strange.  It failed with the version you sent to the list
 for me for a different reason---it tried to verify the ident that
 did not exist in the tested object (which we fixed in the squash).

Hmm. It is very well possible that I ran the tests in the middle of a
rebase, i.e. without my changes to t5302. Will pay more attention next
time, sorry!

Ciao,
Dscho
--
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 6/6] Make sure that index-pack --strict checks tag objects

[Johannes Schindelin]

Hi Junio,

On Thu, 11 Sep 2014, Junio C Hamano wrote:

 Junio C Hamano gits...@pobox.com writes:
 
  When our toolset has become too tight without leaving enough escape
  hatch to hinder further development, it is very sensible to at least
  think about adding a new --for-debug option to hash-object and
  pack-objects that allows us to deliberately create broken
  datastreams to test against.
 
  [...]
 
 It wasn't too painful to do one of them, and the result looks rather
 nice.

I was loathe to make it easier for interested parties to create invalid
Git objects and to push them onto servers that cannot yet benefit from my
patch series. At the very least, I would have preferred to put such
functionality into test-* executables (where I searched for that
functionality in the first place), i.e. outside the distributed binaries.

But since you already did the work and it does the job, I won't worry
about it.

A bigger worry is that the additional test verifies that fsck catches the
invalid tag object and exits, when we really want to be certain that git
fetch --strict will abort on such an object. So the test is still
indirect, although I admit that it is closer now to what we want.

Version 4 of the patch series (without your hash-object --literally patch
because mailed patch series cannot declare on what branches from 'pu' they
rely, I always base my patch series on 'next' for that reason [*1*])
coming up.

Ciao,
Dscho

Footnote *1*: As always, I push my patch series to a topic branch on
GitHub. The one corresponding to the upcoming patch series is in
https://github.com/dscho/git/compare/next...fsck-tag, the one with your
additional test in
https://github.com/dscho/git/compare/next...fsck-tag-plus (the latter
being a thicket rather than a linear topic branch).
--
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 v4 2/6] Accept object data in the fsck_object() function

[Johannes Schindelin]

When fsck'ing an incoming pack, we need to fsck objects that cannot be
read via read_sha1_file() because they are not local yet (and might even
be rejected if transfer.fsckobjects is set to 'true').

For commits, there is a hack in place: we basically cache commit
objects' buffers anyway, but the same is not true, say, for tag objects.

By refactoring fsck_object() to take the object buffer and size as
optional arguments -- optional, because we still fall back to the
previous method to look at the cached commit objects if the caller
passes NULL -- we prepare the machinery for the upcoming handling of tag
objects.

The assumption that such buffers are inherently NUL terminated is now
wrong, of course, hence we pass the size of the buffer so that we can
add a sanity check later, to prevent running past the end of the buffer.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 builtin/fsck.c   |  2 +-
 builtin/index-pack.c |  3 ++-
 builtin/unpack-objects.c | 14 ++
 fsck.c   | 24 +++-
 fsck.h   |  4 +++-
 5 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index d42a27d..d9f4e6e 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -298,7 +298,7 @@ static int fsck_obj(struct object *obj)
 
if (fsck_walk(obj, mark_used, NULL))
objerror(obj, broken links);
-   if (fsck_object(obj, check_strict, fsck_error_func))
+   if (fsck_object(obj, NULL, 0, check_strict, fsck_error_func))
return -1;
 
if (obj-type == OBJ_TREE) {
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 5568a5b..f2465ff 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -773,7 +773,8 @@ static void sha1_object(const void *data, struct 
object_entry *obj_entry,
if (!obj)
die(_(invalid %s), typename(type));
if (do_fsck_object 
-   fsck_object(obj, 1, fsck_error_function))
+   fsck_object(obj, buf, size, 1,
+   fsck_error_function))
die(_(Error in object));
if (fsck_walk(obj, mark_link, NULL))
die(_(Not all child objects of %s are 
reachable), sha1_to_hex(obj-sha1));
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 99cde45..855d94b 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -164,10 +164,10 @@ static unsigned nr_objects;
  * Called only from check_object() after it verified this object
  * is Ok.
  */
-static void write_cached_object(struct object *obj)
+static void write_cached_object(struct object *obj, struct obj_buffer *obj_buf)
 {
unsigned char sha1[20];
-   struct obj_buffer *obj_buf = lookup_object_buffer(obj);
+
if (write_sha1_file(obj_buf-buffer, obj_buf-size, 
typename(obj-type), sha1)  0)
die(failed to write object %s, sha1_to_hex(obj-sha1));
obj-flags |= FLAG_WRITTEN;
@@ -180,6 +180,8 @@ static void write_cached_object(struct object *obj)
  */
 static int check_object(struct object *obj, int type, void *data)
 {
+   struct obj_buffer *obj_buf;
+
if (!obj)
return 1;
 
@@ -198,11 +200,15 @@ static int check_object(struct object *obj, int type, 
void *data)
return 0;
}
 
-   if (fsck_object(obj, 1, fsck_error_function))
+   obj_buf = lookup_object_buffer(obj);
+   if (!obj_buf)
+   die(Whoops! Cannot find object '%s', sha1_to_hex(obj-sha1));
+   if (fsck_object(obj, obj_buf-buffer, obj_buf-size, 1,
+   fsck_error_function))
die(Error in object);
if (fsck_walk(obj, check_object, NULL))
die(Error on reachable objects of %s, sha1_to_hex(obj-sha1));
-   write_cached_object(obj);
+   write_cached_object(obj, obj_buf);
return 0;
 }
 
diff --git a/fsck.c b/fsck.c
index 56156ff..dd77628 100644
--- a/fsck.c
+++ b/fsck.c
@@ -277,7 +277,7 @@ static int fsck_ident(const char **ident, struct object 
*obj, fsck_error error_f
 }
 
 static int fsck_commit_buffer(struct commit *commit, const char *buffer,
- fsck_error error_func)
+   unsigned long size, fsck_error error_func)
 {
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
@@ -322,15 +322,18 @@ static int fsck_commit_buffer(struct commit *commit, 
const char *buffer,
return 0;
 }
 
-static int fsck_commit(struct commit *commit, fsck_error error_func)
+static int fsck_commit(struct commit *commit, const char *data,
+   unsigned long size, fsck_error error_func)
 {
-   const char *buffer = get_commit_buffer(commit, NULL);
-   int ret = fsck_commit_buffer(commit, buffer, error_func);
-   unuse_commit_buffer(commit

[PATCH v4 3/6] Make sure fsck_commit_buffer() does not run out of the buffer

[Johannes Schindelin]

So far, we assumed that the buffer is NUL terminated, but this is not
a safe assumption, now that we opened the fsck_object() API to pass a
buffer directly.

So let's make sure that there is at least an empty line in the buffer.
That way, our checks would fail if the empty line was encountered
prematurely, and consequently we can get away with the current string
comparisons even with non-NUL-terminated buffers are passed to
fsck_object().

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 fsck.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/fsck.c b/fsck.c
index dd77628..73da6f8 100644
--- a/fsck.c
+++ b/fsck.c
@@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, 
fsck_error error_func)
return retval;
 }
 
+static int require_end_of_header(const void *data, unsigned long size,
+   struct object *obj, fsck_error error_func)
+{
+   const char *buffer = (const char *)data;
+   unsigned long i;
+
+   for (i = 0; i  size; i++) {
+   switch (buffer[i]) {
+   case '\0':
+   return error_func(obj, FSCK_ERROR,
+   unterminated header: NUL at offset %d, i);
+   case '\n':
+   if (i + 1  size  buffer[i + 1] == '\n')
+   return 0;
+   }
+   }
+
+   return error_func(obj, FSCK_ERROR, unterminated header);
+}
+
 static int fsck_ident(const char **ident, struct object *obj, fsck_error 
error_func)
 {
char *end;
@@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit, const 
char *buffer,
unsigned parent_count, parent_line_count = 0;
int err;
 
+   if (require_end_of_header(buffer, size, commit-object, error_func))
+   return -1;
+
if (!skip_prefix(buffer, tree , buffer))
return error_func(commit-object, FSCK_ERROR, invalid format 
- expected 'tree' line);
if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
-- 
2.0.0.rc3.9669.g840d1f9

--
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 v4 0/6] Improve tag checking in fsck and with transfer.fsckobjects

[Johannes Schindelin]

This patch series introduces detailed checking of tag objects when
calling git fsck, and also when transfer.fsckobjects is set to true.

To this end, the fsck machinery is reworked to accept the buffer and
size of the object to check, and for commit and tag objects, we verify
that the buffers contain an end of header (i.e. an empty line) to
guarantee that our checks do not run beyond the buffer.

This work was sponsored by GitHub.

Changes since v3:

- removed undesired negativity from commit message

- removed space in '2 err'

Johannes Schindelin (6):
  Refactor type_from_string() to avoid die()ing in case of errors
  Accept object data in the fsck_object() function
  Make sure fsck_commit_buffer() does not run out of the buffer
  fsck: check tag objects' headers
  Add regression tests for stricter tag fsck'ing
  Make sure that index-pack --strict checks tag objects

 builtin/fsck.c   |   2 +-
 builtin/index-pack.c |   3 +-
 builtin/unpack-objects.c |  14 +++--
 fsck.c   | 133 +++
 fsck.h   |   4 +-
 object.c |  11 +++-
 object.h |   3 +-
 t/t1450-fsck.sh  |  20 +++
 t/t5302-pack-index.sh|  19 +++
 9 files changed, 189 insertions(+), 20 deletions(-)

-- 
2.0.0.rc3.9669.g840d1f9
--
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 v4 1/6] Refactor type_from_string() to avoid die()ing in case of errors

[Johannes Schindelin]

In the next commits, we will enhance the fsck_tag() function to check
tag objects more thoroughly. To this end, we need a function to verify
that a given string is a valid object type, but that does not die() in
the negative case.

While at it, prepare type_from_string() for counted strings, i.e. strings
with an explicitly specified length rather than a NUL termination.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 object.c | 11 +--
 object.h |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/object.c b/object.c
index a16b9f9..aedac24 100644
--- a/object.c
+++ b/object.c
@@ -33,13 +33,20 @@ const char *typename(unsigned int type)
return object_type_strings[type];
 }
 
-int type_from_string(const char *str)
+int type_from_string_gently(const char *str, ssize_t len, int gentle)
 {
int i;
 
+   if (len  0)
+   len = strlen(str);
+
for (i = 1; i  ARRAY_SIZE(object_type_strings); i++)
-   if (!strcmp(str, object_type_strings[i]))
+   if (!strncmp(str, object_type_strings[i], len))
return i;
+
+   if (gentle)
+   return -1;
+
die(invalid object type \%s\, str);
 }
 
diff --git a/object.h b/object.h
index 5e8d8ee..e028ced 100644
--- a/object.h
+++ b/object.h
@@ -53,7 +53,8 @@ struct object {
 };
 
 extern const char *typename(unsigned int type);
-extern int type_from_string(const char *str);
+extern int type_from_string_gently(const char *str, ssize_t, int gentle);
+#define type_from_string(str) type_from_string_gently(str, -1, 0)
 
 /*
  * Return the current number of buckets in the object hashmap.
-- 
2.0.0.rc3.9669.g840d1f9

--
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 v4 4/6] fsck: check tag objects' headers

[Johannes Schindelin]

We inspect commit objects pretty much in detail in git-fsck, but we just
glanced over the tag objects. Let's be stricter.

Since we do not want to limit 'tag' lines unduly, values that would fail
the refname check only result in warnings, not errors.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 fsck.c | 86 +-
 1 file changed, 85 insertions(+), 1 deletion(-)

diff --git a/fsck.c b/fsck.c
index 73da6f8..2fffa43 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
 #include commit.h
 #include tag.h
 #include fsck.h
+#include refs.h
 
 static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
 {
@@ -355,6 +356,88 @@ static int fsck_commit(struct commit *commit, const char 
*data,
return ret;
 }
 
+static int fsck_tag_buffer(struct tag *tag, const char *data,
+   unsigned long size, fsck_error error_func)
+{
+   unsigned char sha1[20];
+   int ret = 0;
+   const char *buffer;
+   char *to_free = NULL, *eol;
+   struct strbuf sb = STRBUF_INIT;
+
+   if (data)
+   buffer = data;
+   else {
+   enum object_type type;
+
+   buffer = to_free =
+   read_sha1_file(tag-object.sha1, type, size);
+   if (!buffer)
+   return error_func(tag-object, FSCK_ERROR,
+   cannot read tag object);
+
+   if (type != OBJ_TAG) {
+   ret = error_func(tag-object, FSCK_ERROR,
+   expected tag got %s,
+   typename(type));
+   goto done;
+   }
+   }
+
+   if (require_end_of_header(buffer, size, tag-object, error_func))
+   goto done;
+
+   if (!skip_prefix(buffer, object , buffer)) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
expected 'object' line);
+   goto done;
+   }
+   if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n') {
+   ret = error_func(tag-object, FSCK_ERROR, invalid 'object' 
line format - bad sha1);
+   goto done;
+   }
+   buffer += 41;
+
+   if (!skip_prefix(buffer, type , buffer)) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
expected 'type' line);
+   goto done;
+   }
+   eol = strchr(buffer, '\n');
+   if (!eol) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
unexpected end after 'type' line);
+   goto done;
+   }
+   if (type_from_string_gently(buffer, eol - buffer, 1)  0)
+   ret = error_func(tag-object, FSCK_ERROR, invalid 'type' 
value);
+   if (ret)
+   goto done;
+   buffer = eol + 1;
+
+   if (!skip_prefix(buffer, tag , buffer)) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
expected 'tag' line);
+   goto done;
+   }
+   eol = strchr(buffer, '\n');
+   if (!eol) {
+   ret = error_func(tag-object, FSCK_ERROR, invalid format - 
unexpected end after 'type' line);
+   goto done;
+   }
+   strbuf_addf(sb, refs/tags/%.*s, (int)(eol - buffer), buffer);
+   if (check_refname_format(sb.buf, 0))
+   error_func(tag-object, FSCK_WARN, invalid 'tag' name: %s, 
buffer);
+   buffer = eol + 1;
+
+   if (!skip_prefix(buffer, tagger , buffer))
+   /* early tags do not contain 'tagger' lines; warn only */
+   error_func(tag-object, FSCK_WARN, invalid format - expected 
'tagger' line);
+   else
+   ret = fsck_ident(buffer, tag-object, error_func);
+
+done:
+   strbuf_release(sb);
+   free(to_free);
+   return ret;
+}
+
 static int fsck_tag(struct tag *tag, const char *data,
unsigned long size, fsck_error error_func)
 {
@@ -362,7 +445,8 @@ static int fsck_tag(struct tag *tag, const char *data,
 
if (!tagged)
return error_func(tag-object, FSCK_ERROR, could not load 
tagged object);
-   return 0;
+
+   return fsck_tag_buffer(tag, data, size, error_func);
 }
 
 int fsck_object(struct object *obj, void *data, unsigned long size,
-- 
2.0.0.rc3.9669.g840d1f9

--
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 v4 5/6] Add regression tests for stricter tag fsck'ing

[Johannes Schindelin]

The intent of the new test case is to catch general breakages in
the fsck_tag() function, not so much to test it extensively, trying to
strike the proper balance between thoroughness and speed.

While it *would* have been nice to test the code path where fsck_object()
encounters an invalid tag object, this is not possible using git fsck: tag
objects are parsed already before fsck'ing (and the parser already fails
upon such objects).

Even worse: we would not even be able write out invalid tag objects
because git hash-object parses those objects, too, unless we resorted to
really ugly hacks such as using something like this in the unit tests
(essentially depending on Perl *and* Compress::Zlib):

hash_invalid_object () {
contents=$(printf '%s %d\0%s' $1 ${#2} $2) 
sha1=$(echo $contents | test-sha1) 
suffix=${sha1#??} 
mkdir -p .git/objects/${sha1%$suffix} 
echo $contents |
perl -MCompress::Zlib -e 'undef $/; print compress()' \
 .git/objects/${sha1%$suffix}/$suffix 
echo $sha1
}

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 t/t1450-fsck.sh | 20 
 1 file changed, 20 insertions(+)

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 8c739c9..2b6a6f2 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -194,6 +194,26 @@ test_expect_success 'tag pointing to something else than 
its type' '
test_must_fail git fsck --tags
 '
 
+test_expect_success 'tag with incorrect tag name  missing tagger' '
+   sha=$(git rev-parse HEAD) 
+   cat wrong-tag -EOF 
+   object $sha
+   type commit
+   tag wrong name format
+
+   This is an invalid tag.
+   EOF
+
+   tag=$(git hash-object -t tag -w --stdin wrong-tag) 
+   test_when_finished remove_object $tag 
+   echo $tag .git/refs/tags/wrong 
+   test_when_finished git update-ref -d refs/tags/wrong 
+   git fsck --tags 2out 
+   cat out 
+   grep invalid .tag. name out 
+   grep expected .tagger. line out
+'
+
 test_expect_success 'cleaned up' '
git fsck actual 21 
test_cmp empty actual
-- 
2.0.0.rc3.9669.g840d1f9

--
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 v4 6/6] Make sure that index-pack --strict checks tag objects

[Johannes Schindelin]

One of the most important use cases for the strict tag object checking
is when transfer.fsckobjects is set to true to catch invalid objects
early on. This new regression test essentially tests the same code path
by directly calling 'index-pack --strict' on a pack containing an
tag object without a 'tagger' line.

Technically, this test is not enough: it only exercises a code path that
*warns*, not one that *fails*. The reason is that hash-object and
pack-objects both insist on parsing the tag objects and would fail on
invalid tag objects at this time.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 t/t5302-pack-index.sh | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index 4bbb718..61bc8da 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -243,4 +243,23 @@ test_expect_success 'running index-pack in the object 
store' '
 test -f .git/objects/pack/pack-${pack1}.idx
 '
 
+test_expect_success 'index-pack --strict warns upon missing tagger in tag' '
+sha=$(git rev-parse HEAD) 
+cat wrong-tag EOF 
+object $sha
+type commit
+tag guten tag
+
+This is an invalid tag.
+EOF
+
+tag=$(git hash-object -t tag -w --stdin wrong-tag) 
+pack1=$(echo $tag $sha | git pack-objects tag-test) 
+echo remove tag object 
+thirtyeight=${tag#??} 
+rm -f .git/objects/${tag%$thirtyeight}/$thirtyeight 
+git index-pack --strict tag-test-${pack1}.pack 2err 
+grep ^error:.* expected .tagger. line err
+'
+
 test_done
-- 
2.0.0.rc3.9669.g840d1f9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/6] Improve tag checking in fsck and with transfer.fsckobjects

[Johannes Schindelin]

Hi Junio,

On Fri, 12 Sep 2014, Junio C Hamano wrote:

 Thanks. I think this is ready for 'next' and then down to 'master'
 for the next release.

Thank you!
Dscho
--
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: [msysGit] MinGW(-W64) compilation

[Johannes Schindelin]

Hi Marat,

On Sun, 28 Sep 2014, Marat Radchenko wrote:

 This patch series fixes building on modern MinGW and MinGW-W64
 (including x86_64!).

Awesome work! I'll have a look at it as soon as I can!

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

Re: [msysGit] Re: [PATCH v4] MinGW(-W64) compilation

[Johannes Schindelin]

Hi Marat,

On Wed, 8 Oct 2014, Marat Radchenko wrote:

 On Wed, Oct 08, 2014 at 01:09:20AM +0200, Thomas Braun wrote:

  I wanted to verify that on msysgit but some patches fail to apply
  cleanly. Did you also had to tweak the patches?
  If yes, are these tweaked patches still available somewhere?
 
 msysgit != git-for-windows, as msysgit folks say.

That's not what msysgit folks say (they say that msysgit is the
development environment to build Git for Windows [*1*]), and Thomas is
well aware of the situation because he is a busy contributor on the
Windows side.

 I tested my patches by applying them to git.git/master and building
 inside msysgit.

So the idea would be to rebase from git/git/master onto
msysgit/git/master. Did you do that yet?

Ciao,
Johannes

Footnote *1*: msysgit is about to be phased out. As soon as
https://github.com/git-for-windows/sdk is able to produce a Git for
Windows installer (it already able to build Git and pass the test suite),
we will switch to the new development environment and mark msysgit as
obsolete, keeping it around only for reference.
--
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: [msysGit] Re: [PATCH v4] MinGW(-W64) compilation

[Johannes Schindelin]

Hi Thomas,

On Wed, 8 Oct 2014, Thomas Braun wrote:

 I wanted to verify that on msysgit but some patches fail to apply
 cleanly. Did you also had to tweak the patches?

I applied the patches to git-for-windows/git's master, manually fixing
three of them, and pushed the result to the 'w64' branch in my fork.
Please find them here:

https://github.com/dscho/git/compare/git-for-windows:master...w64

and rebased onto msysgit's master:

https://github.com/dscho/git/compare/msysgit:master...w64-msysgit

Ciao,
Dscho
--
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: [msysGit] [PATCH v4] MinGW(-W64) compilation

[Johannes Schindelin]

Hi all,

On Tue, 30 Sep 2014, Marat Radchenko wrote:

 This patch series fixes building on modern MinGW and MinGW-W64
 (including x86_64!).

To make it easier to review and substantially easier to work on this patch
series with Git, I opened a pull request on GitHub:

https://github.com/msysgit/git/pull/264

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

Re: [msysGit] Re: [PATCH v4] MinGW(-W64) compilation

[Johannes Schindelin]

Hi Marat,

On Wed, 8 Oct 2014, Marat Radchenko wrote:

 On Wed, Oct 08, 2014 at 10:59:57AM +0200, Johannes Schindelin wrote:

  So the idea would be to rebase from git/git/master onto
  msysgit/git/master. Did you do that yet?
 
 No, what for?

To work together?

If you are not interested in working together to make Git on 64-bit
Windows kick-ass, just say so.

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

Re: [msysGit] [PATCH v4] MinGW(-W64) compilation

[Johannes Schindelin]

Hi Marat,

On Wed, 8 Oct 2014, Marat Radchenko wrote:

 On Wed, Oct 08, 2014 at 11:40:17AM +0200, Johannes Schindelin wrote:
  To make it easier to review and substantially easier to work on this patch
  series with Git, I opened a pull request on GitHub:
 
https://github.com/msysgit/git/pull/264
 
 
 1. I fail to see how using a tool that doesn't send emails about review
comments is *easier* than just sending emails.

You probably missed how I commented on exact lines without you having to
guess from the quoted context what part of your patches I am talking
about.

You probably also missed the fact that comments on rewritten commits
automatically drop out of sight, decluttering the set of comments and
making it obvious which comments have not been addressed yet.

And finally, you probably also missed the fact that the official Git fork
for Windows was asked to review your patches because Junio defers
Windows-specific stuff to us. And as you refused to work against our
integration branches (yes, we have two, because we are working towards
switching to a more sustainable development environment, something you
already mocked successfully), we had to rebase your work onto two
branches, which is also substantially easier to do using GitHub rather
than via mails.

But I get it: you want to roll your own thing and not help us review it
let alone make use of it. That's fine, we'll manage.

 2. Please, do not hijack patchset discussion by moving it from git@ ML to 
GitHub comments.

I mistook your work on Git and the fact that you have an account on GitHub
for your willingness to collaborate on this effectively. My mistake, I
apologize!

 3. And I repeat, my goal is to push this stuff in git.git,
not in msysgit.git, not in git-for-windows.git, not in msys2.git, not in 
 other
4k+ forks on GitHub.

Yes, your objection is noted.

Junio, we'll take it from here, don't worry.

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

Re: [msysGit] Re: [PATCH 13/14] git-compat-util.h: fix integer overflow on IL32P64 systems

[Johannes Schindelin]

Hi Junio,

On Wed, 8 Oct 2014, Junio C Hamano wrote:

 Marat Radchenko ma...@slonopotamus.org writes:
 
   #define DEFAULT_PACKED_GIT_LIMIT \
  -   ((1024L * 1024L) * (sizeof(void*) = 8 ? 8192 : 256))
  +   ((size_t)(1024L * 1024L) * (sizeof(void*) = 8 ? 8192 : 256))
 
 1024 * 1024 * 8192 overflows 32-bit unsigned, but is size_t always
 large enough?  Just checking.

The diff is a bit misleading as to what it *actually* changes. It *just*
casts the result to size_t. The arithmetic is performed with longs (thanks
to the l in 1024l) and it only overflows 32 bit iff the sizeof() test
verifies that we're at least on 64 bit -- this arithmetic operation is the
same as before the patch. I was fooled by the diff myself (adding another
parenthesis just to add the cast would probably have helped, though).

IMHO this is a good demonstration how a commit message that goes slightly
beyond the necessary can help tons of time by avoiding to let every
reviewer/reader go through the exact same steps of puzzlement.

Ciao,
Dscho
--
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: [msysGit] Re: [PATCH 14/14] MINGW: config.mak.uname: auto-detect MinGW build from compiler

[Johannes Schindelin]

Hi Junio,

On Thu, 9 Oct 2014, Junio C Hamano wrote:

 Marat Radchenko ma...@slonopotamus.org writes:
 
  On Wed, Oct 08, 2014 at 12:26:52PM -0700, Junio C Hamano wrote:
  ...
  What I am wondering is if it is a better solution to make it easier
  to allow somebody who is cross compiling to express Mr.  Makefile,
  we know better than you and want you to do a MINGW build for us
  without checking with `uname -?` yourself, i.e.
  
 $ make uname_O=MINGW uname_S=MINGW
  
  which would hopefully allow cross-compilation into other
  environments, not just MINGW.
 
  So, do you really want this patch to be changed from 5-liner into
  a full-blow system detection rewrite based on `cc -dumpmachine`
  instead of `uname`?
 
 No, and I do not quite see why you even need to look at -dumbmachine

Nice Freudian ;-)

 output when your goal is to make this command line
 
 $ make uname_O=MINGW uname_S=MINGW
 
 work sensibly.  Wouldn't it be more like a series of
 
   ifndef uname_O
 uname_O := $(shell uname -o)
   endif
 
 or something like that?

Or uname_O ?= $(shell uname -o)

To clarify: it would be enough to look at CROSS_COMPILE to determine
whether we're cross-compiling for MinGW.

The output of -dumpmachine is still needed for the correct CFLAGS/LDFLAGS.

Ciao,
Dscho
--
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: [msysGit] [PATCH v5] MinGW(-W64) compilation

[Johannes Schindelin]

Hi all,

On Wed, 8 Oct 2014, Marat Radchenko wrote:

 This patch series fixes building on modern MinGW and MinGW-W64
 (including x86_64).

To make it more convenient to work on this patch series using Git, I
pushed this branch to

https://github.com/dscho/git/compare/git:master...w64-slonopotamus

I also added one patch I find highly convenient:

https://github.com/dscho/git/commit/29749c7d7b4638c63369d6cf067f5d524d0092f9

-- snipsnap --
Subject: [PATCH] MinGW-w64: Work around bug in MinGW-w64's winuser.h

This allows contributors to compile 64-bit Windows builds of Git without
forcing them to use an unstable version of MinGW-w64.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 compat/poll/poll.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/compat/poll/poll.c b/compat/poll/poll.c
index 8941249..dcbcbaf 100644
--- a/compat/poll/poll.c
+++ b/compat/poll/poll.c
@@ -76,6 +76,18 @@
 
 #ifdef WIN32_NATIVE
 
+/*
+ * Work around https://sourceforge.net/p/mingw-w64/bugs/397. In short, some
+ * definitions were put into the GUI-only section of winuser.h by mistake
+ * in MinGW-w64 versions up to 3.1.0.
+ */
+#ifndef QS_ALLINPUT
+#define QS_ALLINPUT 0xff
+extern WINUSERAPI DWORD WINAPI MsgWaitForMultipleObjects(DWORD nCount,
+   CONST HANDLE *pHandles, WINBOOL fWaitAll, DWORD dwMilliseconds,
+   DWORD dwWakeMask);
+#endif
+
 #define IsConsoleHandle(h) (((intptr_t) (h)  3) == 3)
 
 static BOOL
-- 
2.0.0.rc3.9669.g840d1f9


--
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: [msysGit] Re: [PATCH 14/14] MINGW: config.mak.uname: auto-detect MinGW build from compiler

[Johannes Schindelin]

Hi Junio,

On Thu, 9 Oct 2014, Junio C Hamano wrote:

 Isn't the primary reason we use colon-assign to avoid running the same
 $(shell) over and over again every time $(uname_?) gets referenced? How
 would it work with ?= ???

I was under the impression that ?= would only define the variable once,
the next time ?= would be encountered, no assignment (and actually, no
evaluation) would take place because the variable already has a value?

According to

https://www.gnu.org/software/make/manual/make.html#index-_003f_003d

FOO ?= bar

is exactly equivalent to this (see The origin Function):

ifeq ($(origin FOO), undefined)
  FOO = bar
endif

 Pardon misspellings and grammos, typed on a tablet.

No worries. But you really should use your tablet for reading books after
midnight instead of doing work... ;-)

Ciao,
Dscho
--
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: [msysGit] Re: [PATCH 14/14] MINGW: config.mak.uname: auto-detect MinGW build from compiler

[Johannes Schindelin]

Hi Junio,

On Thu, 9 Oct 2014, Junio C Hamano wrote:

 I didn't mean multiple uses of ?= for the same variable. I meant
 multiple uses of (references to) the variable. I.e. wouldn't FOO and
 BAR behave differently below?
 
 FOO := $(shell random)
 BAR = $(shell random)
 all::
echo $(FOO) and $(BAR)
echo twice $(FOO) and $(BAR)

You're correct, of course, my mistake. I just tested with this:

R ?= $(shell echo $$RANDOM)

all:
echo The values of $(R), $(R) and $(R)

and of course a make yields three different numbers. Sorry for missing
that.

So what we should do is something like

ifeq ($(uname_S),)
uname_S := $(shell uname -s)
endif

even if repeating that pattern is kind of ugly...

Thanks for correcting my mistake,
Dscho
--
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: [msysGit] [PATCH 07/14] Fix BASIC_LDFLAGS and COMPAT_CFLAGS for 64bit MinGW-w64

[Johannes Schindelin]

Hi,

On Wed, 8 Oct 2014, Marat Radchenko wrote:

 +CC_MACH := $(shell sh -c '$(CC) -dumpmachine 2/dev/null || echo not')

There is a rather huge problem with that. The latest mingw-w64 release,
4.9.1, does not do what you expect here: while '.../mingw32/bin/gcc -m32
-o 32.exe test.c' and '.../mingw32/bin/gcc -m64 -o 64.exe test.c' work
fine, producing i686 and x86_64 executables respectively,
'.../mingw32/bin/gcc -dumpmachine' prints i686-w64-mingw32 *always*, even
when specifying the -m64 option.

So unfortunately, the test introduced by this patch (intended to figure
out whether the build targets i686, and skip a compiler and a linker
option otherwise) is incorrect.

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

Re: [msysGit] [PATCH 07/14] Fix BASIC_LDFLAGS and COMPAT_CFLAGS for 64bit MinGW-w64

[Johannes Schindelin]

Hi Ray,

On Thu, 9 Oct 2014, Ray Donnelly wrote:

 On Thu, Oct 9, 2014 at 8:22 PM, Johannes Schindelin
 johannes.schinde...@gmx.de wrote:
 
  On Wed, 8 Oct 2014, Marat Radchenko wrote:
 
  +CC_MACH := $(shell sh -c '$(CC) -dumpmachine 2/dev/null || echo not')
 
  There is a rather huge problem with that. The latest mingw-w64
  release, 4.9.1, does not do what you expect here: while
  '.../mingw32/bin/gcc -m32 -o 32.exe test.c' and '.../mingw32/bin/gcc
  -m64 -o 64.exe test.c' work fine, producing i686 and x86_64
  executables respectively, '.../mingw32/bin/gcc -dumpmachine' prints
  i686-w64-mingw32 *always*, even when specifying the -m64 option.
 
  So unfortunately, the test introduced by this patch (intended to
  figure out whether the build targets i686, and skip a compiler and a
  linker option otherwise) is incorrect.
 
 Which release are you talking about? Can you point me to the tarball
 please?

Certainly:

http://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win32/Personal%20Builds/mingw-builds/4.9.1/threads-win32/sjlj/

(rev1, not rev0)

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

Re: [msysGit] [PATCH 07/14] Fix BASIC_LDFLAGS and COMPAT_CFLAGS for 64bit MinGW-w64

[Johannes Schindelin]

Hi Ray,

On Fri, 10 Oct 2014, Ray Donnelly wrote:

 On Thu, Oct 9, 2014 at 8:47 PM, Johannes Schindelin 
 johannes.schinde...@gmx.de wrote:
 
  On Thu, 9 Oct 2014, Ray Donnelly wrote:
 
  On Thu, Oct 9, 2014 at 8:22 PM, Johannes Schindelin
  johannes.schinde...@gmx.de wrote:
  
   On Wed, 8 Oct 2014, Marat Radchenko wrote:
  
   +CC_MACH := $(shell sh -c '$(CC) -dumpmachine 2/dev/null || echo
 not')
  
   There is a rather huge problem with that. The latest mingw-w64
   release, 4.9.1, does not do what you expect here: while
   '.../mingw32/bin/gcc -m32 -o 32.exe test.c' and
   '.../mingw32/bin/gcc -m64 -o 64.exe test.c' work fine, producing
   i686 and x86_64 executables respectively, '.../mingw32/bin/gcc
   -dumpmachine' prints i686-w64-mingw32 *always*, even when
   specifying the -m64 option.
  
   So unfortunately, the test introduced by this patch (intended to
   figure out whether the build targets i686, and skip a compiler and
   a linker option otherwise) is incorrect.
 
  Which release are you talking about? Can you point me to the tarball
  please?
 
 http://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win32/Personal%20Builds/mingw-builds/4.9.1/threads-win32/sjlj/
 
  (rev1, not rev0)
 
 I guess I can add passing in CFLAGS also to try to catch that case.

Well, my tests say that the CFLAGS do *not* change the behavior of
-dumpmachine. IOW `i686-w64-mingw32-gcc -m64 -dumpmachine` *still* spits
out i686-w64-mingw32. Even if the -m64 flag would cause the compiler to
generate 64-bit binaries.

 I've added support to build using your branch to MSYS2's MINGW-packages
 git-git package in case anyone wants to help out:
 
 https://github.com/Alexpux/MINGW-packages/tree/master/mingw-w64-git-git

Interesting.

With Git for Windows, we aim to become way more standards-compliant by
providing Git as a regular mingw-get'able package. To this end, we use
mgwport recipes to build the required packages.

It looks as if the PKGBUILD system is similar, but *just* incompatible
enough with mgwport to prevent code sharing. Is this fixable?

 Change _based_on_dscho_w64_msysgit=no to =yes. Note also that some more
 patches are needed before we can build, and I think more are needed.
 Using plain msysGit (I.e. =no) and 15 patches we are able to build a
 somewhat functional git.

So here is my plan, please let me know whether you think we can compromise
on a strategy that benefits both of us:

Since I want mingw-get'able packages – also for 64-bit – I would like to
keep the CPU architecture dependent parts as contained as possible and use
only one package system for both. Likewise, I would really prefer to have
a single development environment for both architectures, and the Git for
Windows SDK is really coming along nicely, thanks to the tremendous
efforts put in by Thomas Braun and Sebastian Schuberth.

I am planning, therefore, to provide the MinGW-w64 compiler as an add-on
package that needs to be installed in order to build 64-bit stuff.

At this stage, it is actually *more* than a plan: I already have a package
to install 7za – required to unpack MinGW-w64 pre-built packages – and the
script to package mingw-w64 is in the process of being fleshed out.

With this compiler, and the 'w64' branch from https://github.com/dscho/git
– intended to be merged into https://github.com/git-for-windows/git – the
following command-line produces 64-bit Git:

PATH=/path/to/unpacked/mingw-w64/mingw64/bin/:$PATH \
make \
CROSS_COMPILE=x86_64-w64-mingw32- CC='$(CROSS_COMPILE)gcc' \
AR=ar RC=windres \
NO_ICONV=1 NO_OPENSSL=1 NO_CURL=1 NEEDS_LIBICONV= USE_LIBPCRE=

The test suite passes so far (still running, at the time of writing it is
going through t3404).

Ciao,
Johannes

Re: [msysGit] [PATCH 07/14] Fix BASIC_LDFLAGS and COMPAT_CFLAGS for 64bit MinGW-w64

[Johannes Schindelin]

Hi,

On Fri, 10 Oct 2014, Johannes Schindelin wrote:

 With this [mingw-w64] compiler, and the 'w64' branch from
 https://github.com/dscho/git – intended to be merged into
 https://github.com/git-for-windows/git – the following command-line
 produces 64-bit Git:
 
   PATH=/path/to/unpacked/mingw-w64/mingw64/bin/:$PATH \
   make \
   CROSS_COMPILE=x86_64-w64-mingw32- CC='$(CROSS_COMPILE)gcc' \
   AR=ar RC=windres \
   NO_ICONV=1 NO_OPENSSL=1 NO_CURL=1 NEEDS_LIBICONV= USE_LIBPCRE=
 
 The test suite passes so far (still running, at the time of writing it is
 going through t3404).

And it stopped at t3900-i18n-commit.txt:

not ok 15 - ISO8859-1 should be shown in UTF-8 now
not ok 16 - eucJP should be shown in UTF-8 now
not ok 17 - ISO-2022-JP should be shown in UTF-8 now
not ok 23 - ISO8859-1 should be shown in UTF-8 now
not ok 24 - eucJP should be shown in UTF-8 now
not ok 25 - ISO-2022-JP should be shown in UTF-8 now
not ok 27 - ISO-2022-JP should be shown in eucJP now
not ok 28 - eucJP should be shown in ISO-2022-JP now

Inspecting the test case 15 above, it appears as if ISO-8859-1 was still
shown as ISO-8859-1 instead of UTF-8:

$ hexdump.exe /git/t/t3900/1-UTF-8.txt trash
directory.t3900-i18n-commit/current
 /git/t/t3900/1-UTF-8.txt 
  c3 84 c3 8b c3 91 c3 8f  c3 96 0a 0a c3 81 62 c3
0010  a7 64 c3 a8 66 67 0a
 trash directory.t3900-i18n-commit/current 
  c4 cb d1 cf d6 0a 0a c1  62 e7 64 e8 66 67 0a

So I fear we have still a ways to go before Git works as a 64-bit Windows
binary...

Ciao,
Johannes

Re: [msysGit] [PATCH 07/14] Fix BASIC_LDFLAGS and COMPAT_CFLAGS for 64bit MinGW-w64

[Johannes Schindelin]

Hi Ray,

On Fri, 10 Oct 2014, Ray Donnelly wrote:

 what's the difference between https://github.com/msysgit/git and
 https://github.com/git-for-windows/git ? I noticed that your fork is
 forked from msysgit, not git-for-windows?

I am glad you asked!

Git for Windows was developed using the development environment called
msysGit ever since 2007. Unfortunately the name caused a *real* lot of
confusion, not only because some people wondered what the heck MSys is,
but those who did not wonder mistook it for a *different* version of Git
for Windows.

Apart from the name, msysGit also has the shortcoming of abusing Git to
deploy binaries. In other words, msysGit itself is a Git-managed project
that delivers the complete development environment. Upgrading individual
components is unnecessarily hard, but msysGit's way was necessary because
there was no nice package manager for MinGW/MSys yet.

Things have changed in the meantime, and pushed forward by Thomas Braun
and Sebastian Schuberth, we now have a light-weight Git for Windows SDK –
which is essentially a standard MinGW/MSys system managed through the
package manager mingw-get. The only two add-ons we have is a nicer
installer (that Sebastian offered to MinGW but they declined) and the
addition of our own mingw-get'able packages (such as openssl, libpcre, and
git itself).

Needless to say, I am a big fan of that new strategy. This is why we
decided to just phase out the name msysGit (as well as the GitHub org of
the same name) and work on Git for Windows (with the corresponding GitHub
org, and using the name Git for Windows for the installer aimed at
end-users and Git for Windows SDK for the development environment
targeting Git for Windows developers).

I also added this writeup to the FAQ on the msysGit wiki:
https://github.com/msysgit/msysgit/wiki/Relationship-to-Git-for-Windows

Ciao,
Johannes

Re: [msysGit] [PATCH 07/14] Fix BASIC_LDFLAGS and COMPAT_CFLAGS for 64bit MinGW-w64

[Johannes Schindelin]

Hi,

On Fri, 10 Oct 2014, Johannes Schindelin wrote:

 On Fri, 10 Oct 2014, Johannes Schindelin wrote:
 
  With this [mingw-w64] compiler, and the 'w64' branch from
  https://github.com/dscho/git – intended to be merged into
  https://github.com/git-for-windows/git – the following command-line
  produces 64-bit Git:
  
  PATH=/path/to/unpacked/mingw-w64/mingw64/bin/:$PATH \
  make \
  CROSS_COMPILE=x86_64-w64-mingw32- CC='$(CROSS_COMPILE)gcc' \
  AR=ar RC=windres \
  NO_ICONV=1 NO_OPENSSL=1 NO_CURL=1 NEEDS_LIBICONV= USE_LIBPCRE=
  
  The test suite passes so far (still running, at the time of writing it is
  going through t3404).
 [...]
 
 So I fear we have still a ways to go before Git works as a 64-bit Windows
 binary...

It seems to be not *all* that bad: only t3900, t3901, t4041, t4205, t4210,
t5100, t6006 and t7102 display test failures.

Ciao,
Dscho

Re: [msysGit] Re: msysgit works on wine

[Johannes Schindelin]

Hi Michael,

On Mon, 13 Oct 2014, Michael Stefaniuc wrote:

 On 10/10/2014 02:04 PM, Duy Nguyen wrote:
  On Fri, Oct 10, 2014 at 7:02 PM, Thomas Braun
 
  Are you compiling git.git or msysgit.git?
  
  git.git
  
  And how about the test suite?
  
  running right now, fingers crossed.. kinda slow, not sure if it's wine
  or it's the msys thing.

 We (Wine) are interested in bug reports for git tests failing on Wine
 that succeed on Windows/Linux. Performance issues compared to Windows
 are highly desired too.

Awesome, thank you for the offer!

I haven't tried this in a long time, mainly because at some stage Wine
required a separate console from my terminal to run command-line programs.
And it seemed to me as if in particular process-spawning and heavy-duty
file system operations were the bottleneck.

Hopefully I will find some time soon to perform those tests again.

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

Re: [PATCH] core.filemode may need manual action

[Johannes Schindelin]

Hi,

On Thu, 16 Oct 2014, Thomas Braun wrote:

 Am 16.10.2014 um 21:29 schrieb Torsten Bögershausen:
  core.filemode is set automatically when a repo is created.
  But when a repo is exported via CIFS or cygwin is mixed with Git for Windows
  core.filemode may better be set manually to false.
  Update and improve the documentation.
  
  Helped-by: Junio C Hamano gits...@pobox.com
  Signed-off-by: Torsten Bögershausen tbo...@web.de
  ---
  
  Does this reflect the discussion via email ?
  Or is more tweaking needed ?
  
  
   Documentation/config.txt | 19 +--
   1 file changed, 17 insertions(+), 2 deletions(-)
  
  diff --git a/Documentation/config.txt b/Documentation/config.txt
  index 4333636..b4fea43 100644
  --- a/Documentation/config.txt
  +++ b/Documentation/config.txt
  @@ -204,8 +204,23 @@ advice.*::
   --
   
   core.fileMode::
  -   If false, the executable bit differences between the index and
  -   the working tree are ignored; useful on broken filesystems like FAT.
  +   Tells Git if the executable bit of files in the working tree
  +   is to be honored.
  +
  +   Some filesystems lose the executable bit when a file that is
  +   marked as executable is checked out, or checks out an
  +   non-executable file with executable bit on.  git init and
  +   git clone probe the filesystem to see if it records
  +   executable bit correctly when they create a new repository
  +   and this variable is automatically set as necessary.
  +
  +   A repository, however, may be on a filesystem that records
  +   the filemode correctly, and this variable is set to 'true'
  +   when created, but later may be made accessible from another
  +   environment that loses the filemode (e.g. exporting ext4 via
  +   CIFS mount, visiting a Cygwin managed repository with
  +   MsysGit).  In such a case, it may be necessary to set this
  +   variable to 'false'.
  See linkgit:git-update-index[1].
   +
   The default is true, except linkgit:git-clone[1] or linkgit:git-init[1]
  
 
 [CC'ing msysgit aka git-for-windows/sdk for input]
 
 I'm not really happy with the term MsysGit here.
 Would it be too bold to say
 [... ] visiting a Cygwin managed repository with Git for Windows.
 ?

I agree that msysGit is the wrong term. Not only is it about to be
replaced by the Git for Windows SDK, it is *actively* wrong because
msysGit is just the *development environment* to build Git for Windows.
Most users do *not* need msysGit at all.

Ciao,
Dscho

[PATCH 0/2] Support updating working trees when pushing into non-bare repos

[Johannes Schindelin]

A few years ago, this developer was convinced that it was a bad idea to
auto-update working directories when pushing into the current branch, and
that an excellent way to prove this was to implement that feature. To his
surprise, it turned out to be the one thing he misses most in upstream Git.

So here goes: this patch series adds support for two new
receive.denyCurrentBranch settings: one to update the working directory
(which must be clean, i.e. there must not be any uncommitted changes) when
pushing into the current branch, the other setting detaches the HEAD
instead.

The scenario in which in particular the 'updateInstead' setting became a
boon in this developer's daily work is a multi-laptop one, where working
directories need to be updated between computers without a hassle.

Johannes Schindelin (2):
  Add a few more values for receive.denyCurrentBranch
  Let deny.currentBranch=updateInstead ignore submodules

 Documentation/config.txt |  5 +
 builtin/receive-pack.c   | 58 ++--
 t/t5516-fetch-push.sh| 36 ++
 3 files changed, 97 insertions(+), 2 deletions(-)

-- 
2.0.0.rc3.9669.g840d1f9
--
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 1/2] Add a few more values for receive.denyCurrentBranch

[Johannes Schindelin]

Under certain circumstances, it makes a *lot* of sense to allow pushing
into the current branch. For example, when two machines with different
Operating Systems are required for testing, it makes much more sense to
synchronize between working directories than having to go via a third
server.

Under different circumstances, the working directory needs to be left
untouched, for example when a bunch of VMs need to be shut down to save
RAM and one needs to push everything out into the host's non-bare
repositories quickly.

This change supports both workflows by offering two new values for the
denyCurrentBranch setting:

'updateInstead':
Update the working tree accordingly, but refuse to do so if there
are any uncommitted changes.

'detachInstead':
Detach the HEAD, thereby keeping currently checked-out revision,
index and working directory unchanged.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 Documentation/config.txt |  5 +
 builtin/receive-pack.c   | 58 ++--
 t/t5516-fetch-push.sh| 36 ++
 3 files changed, 97 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e8dd76d..6fc0d6d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2129,6 +2129,11 @@ receive.denyCurrentBranch::
print a warning of such a push to stderr, but allow the push to
proceed. If set to false or ignore, allow such pushes with no
message. Defaults to refuse.
++
+There are two more options: updateInstead which will update the working
+directory (must be clean) if pushing into the current branch, and
+detachInstead which will leave the working directory untouched, detaching
+the HEAD so it does not need to change.
 
 receive.denyNonFastForwards::
If set to true, git-receive-pack will deny a ref update which is
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 32fc540..be4172f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -26,7 +26,9 @@ enum deny_action {
DENY_UNCONFIGURED,
DENY_IGNORE,
DENY_WARN,
-   DENY_REFUSE
+   DENY_REFUSE,
+   DENY_UPDATE_INSTEAD,
+   DENY_DETACH_INSTEAD,
 };
 
 static int deny_deletes;
@@ -120,7 +122,12 @@ static int receive_pack_config(const char *var, const char 
*value, void *cb)
}
 
if (!strcmp(var, receive.denycurrentbranch)) {
-   deny_current_branch = parse_deny_action(var, value);
+   if (value  !strcasecmp(value, updateinstead))
+   deny_current_branch = DENY_UPDATE_INSTEAD;
+   else if (value  !strcasecmp(value, detachinstead))
+   deny_current_branch = DENY_DETACH_INSTEAD;
+   else
+   deny_current_branch = parse_deny_action(var, value);
return 0;
}
 
@@ -730,6 +737,44 @@ static int update_shallow_ref(struct command *cmd, struct 
shallow_info *si)
return 0;
 }
 
+static void merge_worktree(unsigned char *sha1)
+{
+   const char *update_refresh[] = {
+   update-index, --refresh, NULL
+   };
+   const char *read_tree[] = {
+   read-tree, -u, -m, sha1_to_hex(sha1), NULL
+   };
+   struct child_process child;
+   struct strbuf git_env = STRBUF_INIT;
+   const char *env[2];
+
+   if (is_bare_repository())
+   die (denyCurrentBranch = updateInstead needs a worktree);
+
+   strbuf_addf(git_env, GIT_DIR=%s, absolute_path(get_git_dir()));
+   env[0] = git_env.buf;
+   env[1] = NULL;
+
+   memset(child, 0, sizeof(child));
+   child.argv = update_refresh;
+   child.env = env;
+   child.dir = git_work_tree_cfg ? git_work_tree_cfg : ..;
+   child.stdout_to_stderr = 1;
+   child.git_cmd = 1;
+   if (run_command(child))
+   die (Could not refresh the index);
+
+   child.argv = read_tree;
+   child.no_stdin = 1;
+   child.no_stdout = 1;
+   child.stdout_to_stderr = 0;
+   if (run_command(child))
+   die (Could not merge working tree with new HEAD.  Good luck.);
+
+   strbuf_release(git_env);
+}
+
 static const char *update(struct command *cmd, struct shallow_info *si)
 {
const char *name = cmd-ref_name;
@@ -760,6 +805,13 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
if (deny_current_branch == DENY_UNCONFIGURED)
refuse_unconfigured_deny();
return branch is currently checked out;
+   case DENY_UPDATE_INSTEAD:
+   merge_worktree(new_sha1);
+   break;
+   case DENY_DETACH_INSTEAD:
+   update_ref(push into current branch (detach), HEAD,
+   old_sha1, NULL, REF_NODEREF

[PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Johannes Schindelin]

They are not affected by the update anyway.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 builtin/receive-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index be4172f..4ba51df 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, struct 
shallow_info *si)
 static void merge_worktree(unsigned char *sha1)
 {
const char *update_refresh[] = {
-   update-index, --refresh, NULL
+   update-index, --ignore-submodules, --refresh, NULL
};
const char *read_tree[] = {
read-tree, -u, -m, sha1_to_hex(sha1), NULL
-- 
2.0.0.rc3.9669.g840d1f9
--
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 1/2] Add a few more values for receive.denyCurrentBranch

[Johannes Schindelin]

Hi Junio,

On Fri, 7 Nov 2014, Junio C Hamano wrote:

 [...]

I will address your concerns after the weekend.

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

Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch

[Johannes Schindelin]

Hi Junio,

On Fri, 7 Nov 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  Under certain circumstances, it makes a *lot* of sense to allow pushing
  into the current branch. For example, when two machines with different
  Operating Systems are required for testing, it makes much more sense to
  synchronize between working directories than having to go via a third
  server.
 
 Even if you do not have a third server, each time you decide to do
 such a push, you have a single source of truth, i.e. the repository
 you are pushing from, so instead of doing that push, all the others
 could instead pull from that single source of truth.  In that sense,
 even though I wouldn't say it is wrong to use push in the opposite
 direction for this synchronization, I have to say that the above is
 not a very strong argument.  It certainly does not deserve *lot*
 in bold font ;-)

I am sorry, I should have been more explicit about the fact that other
Operating System includes also Windows, where it is a major hassle to
set up an ssh daemon, hence the asymmetry of ease to connect from one to
another machine. I really thought that was obvious, my apologies. My next
patch iteration will have the Windows scenario as motivation.

Note that I did not even dive into the problem of many loaner laptops
where you are not allowed to set up an ssh daemon, or even know the
user's password. I did not mention those problems because I again assumed
(again, apologies) that this was obvious because it occurred to me
automatically when thinking about the multi-laptop problem.

And I must apologize yet again, for I also failed to specify explicitly
another reason why pushing makes a *lot* more sense than pulling: at least
for me, personally, having to switch keyboards just to synchronize Git
checkouts from one to another computer *is* a hassle.

  Under different circumstances, the working directory needs to be left
  untouched, for example when a bunch of VMs need to be shut down to save
  RAM and one needs to push everything out into the host's non-bare
  repositories quickly.
 
 For this use case, if you assume that the tips of branches in the
 repositories on a bunch of VMs could be pointing at different
 commits (i.e. each of them has acquired more work without
 coordination), you are risking lossage by pushing into refs/heads/,
 not refs/remotes/vm$n/, aren't you?  When you want to save things
 away quickly, you do not want to be worried about a push from VM1
 stomping on what was stored from VM0, and using separate remotes,
 i.e. refs/remotes/vm$n/, has been the recommended way to do so.  So
 this is not a very strong argument, either.

I thank you for your assessment of my personal working style.

:-)

And yet again I have to apologize because I find that relying on Git's
fast-forward default (you need to force non-fast-forward ref updates,
which I don't, at least not by default) saves me from that lossage, and
will therefore not change my personal working style that served me so well
for years.

 I do not think of a good justification of detachInstead offhand, but
 you must have thought things through a lot more than I did, so you
 can come up with a work flow description that is more usable by mere
 mortals to justify that mode.

The main justification is that it is safer than updateInstead because it
is guaranteed not to modify the working directory. I intended it for use
by developers who are uncomfortable with updating the working directory
through a push, and as uncomfortable with forgetting about temporary
branches pushed, say, via git push other-computer HEAD:refs/heads/tmp.

However, I have to admit that I never used this myself after the first few
weeks of playing with this patch series.

 Without such a description to justify why detachInstead makes sense, for
 example, I cannot even answer this simple question:
 
 Would it make sense to have a third mode, check out if the
 working tree is clean, detach otherwise?

I imagine that some developer might find that useful. If you insist, I
will implement it, even if I personally deem this mode way too complicated
a concept for myself to be used safely.

  [... snip a lot of text that made me almost miss the comments below ...]
 
  diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
  index 32fc540..be4172f 100644
  --- a/builtin/receive-pack.c
  +++ b/builtin/receive-pack.c
  @@ -26,7 +26,9 @@ enum deny_action {
  DENY_UNCONFIGURED,
  DENY_IGNORE,
  DENY_WARN,
  -   DENY_REFUSE
  +   DENY_REFUSE,
  +   DENY_UPDATE_INSTEAD,
  +   DENY_DETACH_INSTEAD,
 
 Don't add trailing comma after the last element of enum.

Will fix in the next iteration.

  @@ -730,6 +737,44 @@ static int update_shallow_ref(struct command *cmd, 
  struct shallow_info *si)
  return 0;
   }
   
  +static void merge_worktree(unsigned char *sha1)
  +{
  +   const char *update_refresh[] = {
  +   update-index, --refresh, NULL

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Johannes Schindelin]

Hi Junio,

On Fri, 7 Nov 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
  ---
   builtin/receive-pack.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
 
  diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
  index be4172f..4ba51df 100644
  --- a/builtin/receive-pack.c
  +++ b/builtin/receive-pack.c
  @@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, 
  struct shallow_info *si)
   static void merge_worktree(unsigned char *sha1)
   {
  const char *update_refresh[] = {
  -   update-index, --refresh, NULL
  +   update-index, --ignore-submodules, --refresh, NULL
  };
  const char *read_tree[] = {
  read-tree, -u, -m, sha1_to_hex(sha1), NULL
 
 I suspect that you did not squash this into 1/2 on purpose, and I am
 guessing the reason is because you were unsure what should happen
 when there were differences in submodules' working trees (otherwise,
 you would have simply squashed without oops it was a thinko to
 forget passing this option as a separate patch).  I am not sure
 either.

This change is squashed into the first patch in the next iteration.

 By the way, if the expected use case of updateInstead is what I
 outlined in the previous message, would it make more sense not to
 fail with update-index --refresh failure (i.e. the working tree
 files have no changes since the index)?
 
 Thinking about it a bit more, checking with update-index --refresh
 feels doubly wrong.  You not just want the working tree files to be
 pristine with respect to the index, but also you do not want to see
 any change between the index and the original HEAD, i.e.
 
   $ git reset --hard  echo Makefile ; git add Makefile
 $ git update-index --refresh ; echo $?
 0
 
 this is not a good state from which you would want to update the
 working tree.
 
 Wouldn't the two-tree form read-tree -u -m that is the equivalent
 to branch switching do a sufficient check?

That is indeed what the patched code calls.

 Also, regarding the new calls to die() in the main patch, shouldn't
 they just be returning the error reason in string form, just like
 DENY_REFUSE returns branch is currently checked out to signal a
 push failure to the caller?

Changed in the next iteration.

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

Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch

[Johannes Schindelin]

Hi Peff,

On Sat, 8 Nov 2014, Jeff King wrote:

 On Fri, Nov 07, 2014 at 02:58:17PM +0100, Johannes Schindelin wrote:
 
  Under certain circumstances, it makes a *lot* of sense to allow pushing
  into the current branch. For example, when two machines with different
  Operating Systems are required for testing, it makes much more sense to
  synchronize between working directories than having to go via a third
  server.
 
 FWIW, I do this without a third server (and without resorting to pull),
 with:
 
   host1$ git push host2 master:refs/remotes/host1/master
   host2$ git merge host1/master
 
 You can even set up a push refspec to make git push host2 do the right
 thing.
 
 That being said, I do like the premise of your patch, as it eliminates
 the extra step on the remote side (which is not that big a deal in
 itself, but when you realize that host2 _did_ have some changes on it,
 then you end up doing the merge there, when in general I'd prefer to do
 all the work on host1 via git pull).

Plus: you have the luxury of working on an OS that makes ssh'ing from
another machine relatively easy. At least if you have the root password on
your machine. Which, I hate to point it out, is not too common a
commodity.

Ciao,
Dscho
--
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 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Johannes Schindelin]

Hi Jens,

On Sun, 9 Nov 2014, Jens Lehmann wrote:

 Am 07.11.2014 um 20:20 schrieb Junio C Hamano:
  Johannes Schindelin johannes.schinde...@gmx.de writes:
 
   Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
   ---
 builtin/receive-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  
   diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
   index be4172f..4ba51df 100644
   --- a/builtin/receive-pack.c
   +++ b/builtin/receive-pack.c
   @@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd,
   struct shallow_info *si)
 static void merge_worktree(unsigned char *sha1)
 {
 const char *update_refresh[] = {
   - update-index, --refresh, NULL
   + update-index, --ignore-submodules, --refresh, NULL
  };
  const char *read_tree[] = {
   read-tree, -u, -m, sha1_to_hex(sha1), NULL
 
  I suspect that you did not squash this into 1/2 on purpose, and I am
  guessing the reason is because you were unsure what should happen
  when there were differences in submodules' working trees (otherwise,
  you would have simply squashed without oops it was a thinko to
  forget passing this option as a separate patch).  I am not sure
  either.
 
 I think --ignore-submodules is currently the right thing to do here
 and would rather squash this into the first commit.

Done.

Thanks,
Dscho
--
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