Re: [PATCH] difftool: handle changing symlinks in dir-diff mode

[Junio C Hamano]

David Aguilar  writes:

> Reading the code again, the point of add_left_or_right()
> is to populate the worktree (done later in the loop) with
> the stuff we read from Git.  Thus, if we changed just this
> section to call get_symlink() then we should not even try
> to checkout any symlink entries in !use_wt_file(...)
> block where checkout_entry() / create_symlink_file() are called.
> 
> Since the symlinks2 hashmap already populates the worktree
> then that code should instead simply skip symlinks.

OK, that would simplify things, certainly.

> I'll take a stab at adding a get_symlink() helper, adjust
> the code so that add_left_or_right() is populated, and
> special-case the checkout_entry() code path to simply skip
> over null SHA1s.

Did you mean s/null SHA1s/S_ISLNK()s/?

> One minor thing I noticed is that I had to use "echo -n"
> for the stuff coming out of strbuf_readlink(), and
> plain "echo" for entries that come in via read_sha1_file()
> content passed to add_left_or_right().
>
> That suggests that maybe I should append a newline to the
> output from strbuf_readlink() so that it matches Git.
> Does that sound right?  Does Git store symlink entries
> with a terminating newline?

Do not append a newline.  Unless the pathname of the target you are
symlinking to ends with LF, readlink() won't end with LF, and the
stand-in file shouldn't, either.

By the way, avoid "echo -n"; use "printf '%s'" instead.

Thanks.

Re: [PATCH] difftool: handle changing symlinks in dir-diff mode

[Junio C Hamano]

Junio C Hamano  writes:

>> +struct strbuf path = STRBUF_INIT;
>> +struct strbuf link = STRBUF_INIT;
>> +
>> +int ok = 0;
>> +
>> +if (strbuf_readlink(, ce->name, ce_namelen(ce)) == 0) {
>> +strbuf_add(, state->base_dir, state->base_dir_len);
>> +strbuf_add(, ce->name, ce_namelen(ce));
>> +
>> +write_file_buf(path.buf, link.buf, link.len);
>
> This does "write content into symlink stand-in file", but why?  A
> symbolic link that is not changed on the right-hand side of the
> comparison (i.e. S_ISLNK(rmode) && !is_null_oid()) would go to
> checkout_entry() which
>
>  - creates a symbolic link, on a filesystem that supports symlink; or
>  - writes the stand-in file on a filesystem that does not.
>
> which is the right thing.  It seems that the other side of "if 
> (!use_wt_file())"
> if/elseif/... cascade also does the right thing manually.
>
> Shouldn't this helper also do the same (i.e. create symlink and fall
> back to stand-in)?
>
> Also, I am not sure if strbuf_readlink() can unconditionally used
> here.  On a filesystem without symbolic link, the working tree
> entity that corresponds to the ce that represents a symlink is a
> stand-in regular file, so shouldn't we be opening it as a regular
> file and reading its contents in that case?

I _think_ I was mistaken (please correct me again if my reasoning
below is wrong) and unconditional writing of a plain regular file is
what this codepath wants to do, because we are preparing two temporary
directories, to be fed to a Git-unaware tool that knows how to show
a diff of two directories (e.g. "diff -r A B").  Because the tool is
Git-unaware, if a symbolic link appears in either of these temporary
directories, it will try to dereference and show the difference of
the target of the symbolic link, which is not what we want, as the
goal of the dir-diff mode is to produce an output that is logically
equivalent to what "git diff" produces---most importantly, we want
to get textual comparison of the result of the readlink(2).  And
write_file_buf() used here is exactly that---we write the contents
of symlink to a regular file to force the external tool to compare
the readlink(2) result as if it is a text.  Even on a filesystem
that is capable of doing a symbolic link.

The strbuf_readlink() that read the contents of symlink, however,
is wrong on a filesystem that is not capable of a symbolic link; if
core.symlinks is false, we do need to read them as a regular file.

Re: [PATCH] difftool: handle changing symlinks in dir-diff mode

[David Aguilar]

On Mon, Mar 13, 2017 at 02:33:09PM -0700, Junio C Hamano wrote:
> Johannes Schindelin  writes:
> 
> >> > +if (strbuf_readlink(, ce->name, ce_namelen(ce)) == 0) {
> >> > +strbuf_add(, state->base_dir, state->base_dir_len);
> >> > +strbuf_add(, ce->name, ce_namelen(ce));
> >> > +
> >> > +write_file_buf(path.buf, link.buf, link.len);
> >> 
> >> This does "write content into symlink stand-in file", but why?
> >
> > From the commit message:
> >
> > > Detect the null object ID for symlinks in dir-diff so that
> > > difftool can prepare temporary files that matches how git
> > > handles symlinks.
> 
> Yes I read what the proposed log message said, and noticed that
> symbolic link is _always_ written as a regular file.
> 
> I was questioning why.  I know Git falls back to such behaviour when
> the filesystem does not support symbolic links.  "Why do so
> unconditionally, even when the filesystem does?" was the question.
> 
> > The obvious connection: when core.symlinks = false, Git already falls back
> > to writing plain files with the link target as contents. This function
> > does the same, for the same motivation: it is the best we can do in this
> > case.
> 
> And that "obvious connection" does not answer the question.

Thanks for the thorough review.

I'll try to answer questions from the various emails in just this
one spot in case it helps.

Dscho wrote:
> Given that we explicitly ask use_wt_file() whether we can use the
> worktree's file, and we get the answer "no" before we enter the modified
> code block, I would really expect us *not* to want to read the link from
> disk at all.

That probably deserves a comment on its own.

The use_wt_file() function really answers the question --
"does the worktree contain content that does is unknown to
 Git, and thus we should symlink to the worktree?"

Since these are symlinks, and symlinks are already used to
point back to the worktree (so that difftools will write
back to the worktree in case the user edited in-tool)
then the meaning of use_wt_file() in this context
can be misleading.

What we're trying to do is handle the case where Git knows it's dealing
with an entry that it wants to checkout into a temporary area
but it has no way to do so.  These entries always have the
0{40} null SHA1 because that is what git diff-files reports
for content that exists in the worktree only.

Dscho wrote:
> I think you are right, we cannot simply call strbuf_readlink(), we would
> have to check the core_symlinks variable to maybe read the file contents
> instead.
>
> But then, it may not be appropriate to read the worktree to begin
> with...
> see my reply to the patch that I will send out in a couple of minutes.

In this case, where the null SHA1 indicates that it is unknown content,
then I believe we must read from the worktree to simulate what Git
would have checked out.  Checking for core.symlinks should probably be
done before calling strbuf_readlink, though.

Junio wrote:
> > > Detect the null object ID for symlinks in dir-diff so that
> > > difftool can prepare temporary files that matches how git
> > > handles symlinks.
>
> Yes I read what the proposed log message said, and noticed that
> symbolic link is _always_ written as a regular file.
>
> I was questioning why.  I know Git falls back to such behaviour when
> the filesystem does not support symbolic links.  "Why do so
> unconditionally, even when the filesystem does?" was the question.

I have to re-read the code to see where this is special-cased, but
it seems that symlinks are always written as raw files by the
dir-diff code for the purposes of full-tree diffing.

I think the "why" is tied up in the implementation details of
the symlink-back-to-the-worktree-to-allow-editing feature.
By special-casing in-tree symlinks and writing them as raw
files we can hijack on-disk symlinks.  We use on-disk symlinks to
link back to the worktree so that external diff tools can write
to the worktree through the symlink.

Junio wrote:
> On this part I didn't comment in my previous message, but what is
> the implication of omitting add-left-or-right and not registering
> this symbolic link modified in the working tree to the symlinks2
> table?
>
> I am wondering if these should be more like
>
> if (S_ISLNK(lmode) {
> char *content = get_symlink(src_path, );
> add_left_or_right(, src_path, content, 0);
> free(content);
> }
>
> with get_symlink() helper that does
>
>  - if the object name is not 0{40}, read from the object store
>
>  - if the object name is 0{40}, that means we need to read the real
>contents from the working tree file, so do the "readlink(2) if
>symbolic link is supported, otherwise open/read/close the stub
>file sitting there" thing.
>
> Similary to the right hand side tree.

I'll take a look at trying this out.

Reading the code again, the 

[PATCH 6/6 v5] sha1_name.c: avoid parsing @{-1} unnecessarily

[mash]

Move dash is previous branch check to get_sha1_basic.
Introduce helper function that gets nth prior branch switch from reflog.

Signed-off-by: mash 
---
RE: [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
> + if (*name == '-' && len == 1) {
> + name = "@{-1}";
> + len = 5;
> + }

We could avoid parsing @{-1} unnecessarily with something like this patch.

Forgive me I don't understand how the patch numbering works just yet. This is
6/6 because format-patch made it 6/6 with however I got the patches applied on
my end. This should apply cleanly on pu anyways.

Thanks to Stefan since he suggested that I might want to review this.

mash

 sha1_name.c | 39 ---
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 2f86bc9..363bbe7 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -568,6 +568,7 @@ static inline int push_mark(const char *string, int len)
 }
 
 static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned 
lookup_flags);
+static int get_branch_switch(int nth, struct strbuf *buf);
 static int interpret_nth_prior_checkout(const char *name, int namelen, struct 
strbuf *buf);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
@@ -628,11 +629,12 @@ static int get_sha1_basic(const char *str, int len, 
unsigned char *sha1,
if (len && ambiguous_path(str, len))
return -1;
 
-   if (nth_prior) {
+   if (nth_prior || !strcmp(str, "-")) {
struct strbuf buf = STRBUF_INIT;
int detached;
 
-   if (interpret_nth_prior_checkout(str, len, ) > 0) {
+   if (nth_prior ? interpret_nth_prior_checkout(str, len, ) > 0
+ : get_branch_switch(1, ) > 0) {
detached = (buf.len == 40 && !get_sha1_hex(buf.buf, 
sha1));
strbuf_release();
if (detached)
@@ -1078,6 +1080,25 @@ static int grab_nth_branch_switch(unsigned char *osha1, 
unsigned char *nsha1,
return 0;
 }
 
+static int get_branch_switch(int nth, struct strbuf *buf)
+{
+   int retval;
+   struct grab_nth_branch_switch_cbdata cb;
+
+   cb.remaining = nth;
+   strbuf_init(, 20);
+
+   retval = for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch,
+);
+   if (0 < retval) {
+   strbuf_reset(buf);
+   strbuf_addbuf(buf, );
+   }
+
+   strbuf_release();
+   return retval;
+}
+
 /*
  * Parse @{-N} syntax, return the number of characters parsed
  * if successful; otherwise signal an error with negative value.
@@ -1086,8 +1107,6 @@ static int interpret_nth_prior_checkout(const char *name, 
int namelen,
struct strbuf *buf)
 {
long nth;
-   int retval;
-   struct grab_nth_branch_switch_cbdata cb;
const char *brace;
char *num_end;
 
@@ -1103,18 +1122,8 @@ static int interpret_nth_prior_checkout(const char 
*name, int namelen,
return -1;
if (nth <= 0)
return -1;
-   cb.remaining = nth;
-   strbuf_init(, 20);
 
-   retval = 0;
-   if (0 < for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch, 
)) {
-   strbuf_reset(buf);
-   strbuf_addbuf(buf, );
-   retval = brace - name + 1;
-   }
-
-   strbuf_release();
-   return retval;
+   return 0 < get_branch_switch(nth, buf) ? brace - name + 1 : 0;
 }
 
 int get_oid_mb(const char *name, struct object_id *oid)
-- 
2.9.3

git checkout exit value and post-commit hooks

[Andreas Politz]

Hi,

the exit value of a `git checkout' seems to depend on the exit values of
the hooks it runs. This breaks for example `git bisect', as seen in the
following example.

$ mkdir gitbug
$ cd gitbug
$ git init
$ ln -s /bin/false .git/hooks/post-commit
$ git bisect start
$ git bisect reset
fatal: invalid reference: master
Could not check out original HEAD 'master'.
Try 'git bisect reset '.

-ap

Re: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Junio C Hamano]

Devin Lehmacher  writes:

> diff --git a/credential-cache.c b/credential-cache.c
> index db1343b46..63236adc2 100644
> --- a/credential-cache.c
> +++ b/credential-cache.c
> @@ -83,12 +83,18 @@ static void do_cache(const char *socket, const char 
> *action, int timeout,
>   strbuf_release();
>  }
>  
> +static int is_socket(char *path) {
> + struct stat sb;
> + int ret = lstat(path, );
> + return ret && S_IFSOCK(sb.st_mode);
> +}
> +
>  static char *get_socket_path(void) {
>   char *home_socket;
>  
>   home_socket = expand_user_path("~/.git-credential-cache/socket");
>   if (home_socket) {
> - if (file_exists(home_socket))
> + if (is_socket(home_socket))

This should be done as part of 2/3, no?  It does not make sense to
add 2/3 and then immediately say "oops, the check in 2/3 is wrong,
and let's update it like so".

Re: [GSoC][PATCH/RFC v3 2/3] credential-cache: use XDG_CACHE_HOME for socket

[Junio C Hamano]

Devin Lehmacher  writes:

> Signed-off-by: Devin Lehmacher 
> Reviewed-by: Junio C Hamano, Jeff King

The last line is premature; neither of us reviewed this exact
version and haven't checked if all the issues we mentioned in the
previous review have been addressed.


> +static char *get_socket_path(void) {

Compare ths line with how cmd_main() below is declared.  Notice
any difference?  We begin our functions like this:

static char *get_socket_path(void)
{

Re: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Devin Lehmacher]

> Best practice for submitting patches would be to ensure that each patch
> compiles without errors (with the DEVELOPER=1 flag set) and that the
> entire test suite passes with no errors; this is to maintain
> bisect-ability.  Only after you've done this should you send your
> patches to the mailing list.

Thanks for the advice. I will be more careful in the future.

-Devin

Re: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Brandon Williams]

On 03/13, Devin Lehmacher wrote:
> > +static int is_socket(char *path) {
> > +   struct stat sb;
> > +   int ret = lstat(path, );
> > +   return ret && S_IFSOCK(sb.st_mode);
> > +}
> 
> This patch won’t even compile. S_IFSOCK(sb.st_mode) should have been S_IFSOCK 
> & sb.st_mode.
> 
> (I guess I should have compiled first)
> 
> After making that change this patch compiles (currently running tests).

Best practice for submitting patches would be to ensure that each patch
compiles without errors (with the DEVELOPER=1 flag set) and that the
entire test suite passes with no errors; this is to maintain
bisect-ability.  Only after you've done this should you send your
patches to the mailing list.

-- 
Brandon Williams

Re: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Devin Lehmacher]

> +static int is_socket(char *path) {
> + struct stat sb;
> + int ret = lstat(path, );
> + return ret && S_IFSOCK(sb.st_mode);
> +}

This patch won’t even compile. S_IFSOCK(sb.st_mode) should have been S_IFSOCK & 
sb.st_mode.

(I guess I should have compiled first)

After making that change this patch compiles (currently running tests).

-Devin

[GSoC][PATCH/RFC v3 1/3] path.c: add xdg_cache_home

[Devin Lehmacher]

We already have xdg_config_home to format paths relative to
XDG_CONFIG_HOME. Let's provide a similar function xdg_cache_home to do
the same for paths relative to XDG_CACHE_HOME.

Signed-off-by: Devin Lehmacher 
---
 cache.h |  7 +++
 path.c  | 15 +++
 2 files changed, 22 insertions(+)

diff --git a/cache.h b/cache.h
index 8c0e64420..5db29a945 100644
--- a/cache.h
+++ b/cache.h
@@ -1169,6 +1169,13 @@ extern int is_ntfs_dotgit(const char *name);
  */
 extern char *xdg_config_home(const char *filename);
 
+/**
+ * Return a newly allocated string with the evaluation of
+ * "$XDG_CACHE_HOME/git/$filename" if $XDG_CACHE_HOME is non-empty, otherwise
+ * "$HOME/.cache/git/$filename". Return NULL upon error.
+ */
+extern char *xdg_cache_home(const char *filename);
+
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
 #define LOOKUP_UNKNOWN_OBJECT 2
diff --git a/path.c b/path.c
index efcedafba..22248436b 100644
--- a/path.c
+++ b/path.c
@@ -1272,6 +1272,21 @@ char *xdg_config_home(const char *filename)
return NULL;
 }
 
+char *xdg_cache_home(const char *filename)
+{
+   const char *home, *cache_home;
+
+   assert(filename);
+   cache_home = getenv("XDG_CACHE_HOME");
+   if (cache_home && *cache_home)
+   return mkpathdup("%s/git/%s", cache_home, filename);
+
+   home = getenv("HOME");
+   if (home)
+   return mkpathdup("%s/.cache/git/%s", home, filename);
+   return NULL;
+}
+
 GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD")
 GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD")
 GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG")
-- 
2.11.0

[GSoC][PATCH/RFC v3 2/3] credential-cache: use XDG_CACHE_HOME for socket

[Devin Lehmacher]

Make git-credential-cache follow the XDG base path specification by
default. This increases consistency with other applications and helps
keep clutter out of users' home directories.

Check the old socket location, ~/.git-credential-cache/socket and use it
instead if there is already a socket at that location rather than
forcibly creating a new socket at the new location.
If there is not a socket at that location create a new one at
$XDG_CACHE_HOME/git/credential/socket following XDG base path
specification. Use the subdirectory credential/ in case other files are
stored under $XDG_CACHE_HOME/git/ in the future and to make the socket's
purpose clear.

Signed-off-by: Devin Lehmacher 
Reviewed-by: Junio C Hamano, Jeff King
---
 Documentation/git-credential-cache.txt | 10 ++
 credential-cache.c | 16 +++-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-credential-cache.txt 
b/Documentation/git-credential-cache.txt
index 96208f822..fce6319e8 100644
--- a/Documentation/git-credential-cache.txt
+++ b/Documentation/git-credential-cache.txt
@@ -33,10 +33,12 @@ OPTIONS
 --socket ::
 
Use `` to contact a running cache daemon (or start a new
-   cache daemon if one is not started). Defaults to
-   `~/.git-credential-cache/socket`. If your home directory is on a
-   network-mounted filesystem, you may need to change this to a
-   local filesystem. You must specify an absolute path.
+   cache daemon if one is not started).
+   Defaults to `~/.git-credential-cache/socket` if it exists and
+   `$XDG_CACHE_HOME/git/credential/socket` otherwise.
+   If your home directory is on a network-mounted filesystem, you
+   may need to change this to a local filesystem. You must specify
+   an absolute path.
 
 CONTROLLING THE DAEMON
 --
diff --git a/credential-cache.c b/credential-cache.c
index cc8a6ee19..db1343b46 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -83,6 +83,20 @@ static void do_cache(const char *socket, const char *action, 
int timeout,
strbuf_release();
 }
 
+static char *get_socket_path(void) {
+   char *home_socket;
+
+   home_socket = expand_user_path("~/.git-credential-cache/socket");
+   if (home_socket) {
+   if (file_exists(home_socket))
+   return home_socket;
+   else
+   free(home_socket);
+   }
+
+   return xdg_cache_home("credential/socket");
+}
+
 int cmd_main(int argc, const char **argv)
 {
char *socket_path = NULL;
@@ -106,7 +120,7 @@ int cmd_main(int argc, const char **argv)
op = argv[0];
 
if (!socket_path)
-   socket_path = 
expand_user_path("~/.git-credential-cache/socket");
+   socket_path = get_socket_path();
if (!socket_path)
die("unable to find a suitable socket path; use --socket");
 
-- 
2.11.0

[GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Devin Lehmacher]

Create function is_socket.
Make get_socket_path return check if ~/.git-credential-cache/socket is a
socket and not just a file. If file_exists behavior could change in an
unexpected way. Additionally a file at ~/.git-credential-cache/socket
could cause false positives which would otherwise lead to crashes.

Signed-off-by: Devin Lehmacher 
---
 credential-cache.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/credential-cache.c b/credential-cache.c
index db1343b46..63236adc2 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -83,12 +83,18 @@ static void do_cache(const char *socket, const char 
*action, int timeout,
strbuf_release();
 }
 
+static int is_socket(char *path) {
+   struct stat sb;
+   int ret = lstat(path, );
+   return ret && S_IFSOCK(sb.st_mode);
+}
+
 static char *get_socket_path(void) {
char *home_socket;
 
home_socket = expand_user_path("~/.git-credential-cache/socket");
if (home_socket) {
-   if (file_exists(home_socket))
+   if (is_socket(home_socket))
return home_socket;
else
free(home_socket);
-- 
2.11.0

[GSoC][PATCH/RFC v3 0/3] Fix commit messages, check if socket is socket

[Devin Lehmacher]

I fixed all of the commit messages and the weird indentation.
I also now check that the socket is actually a socket.

What do you think of the function is_socket? Is it general enough to be put
in dir.h or unix_socket.h for use in other files? Or should it be left as is?

- Devin

Re: What's cooking in git.git (Mar 2017, #05; Mon, 13)

[Junio C Hamano]

Stefan Beller  writes:

>>
>> * sb/rev-parse-show-superproject-root (2017-03-08) 1 commit
>>  - rev-parse: add --show-superproject-working-tree
>>
>>  From a working tree of a repository, a new option of "rev-parse"
>>  lets you ask if the repository is used as a submodule of another
>>  project, and where the root level of the working tree of that
>>  project (i.e. your superproject) is.
>>
>>  Almost there, but documentation needs a bit more work.
>
> Care to clarify what parts of docs you mean?
> https://public-inbox.org/git/xmqqr327z5rn@gitster.mtv.corp.google.com/
> sounded like the topic is done.

Thanks for spotting a leftover comment I wrote about v3; what is
queued is v4 that addresses the issues I found in that version.

Will update its status to "Will merge to 'next'", which means
reviewers (including me) are encouraged to take another look for the
last time with fresh eyes.

Thanks.

Re: [PATCH v5 10/11] setup_git_directory_gently_1(): avoid die()ing

[Junio C Hamano]

Johannes Schindelin  writes:

> On Mon, 13 Mar 2017, Junio C Hamano wrote:
>
>> When a patch series is refactoring an existing function to be used
>> in different codepaths, an existing issue inherited from the
>> original code (e.g. perhaps an existing error checking that is
>> looser than ideal) may have been OK in the original context (e.g.
>> perhaps it died and refused to run until the user corrected the
>> repository), and it still is OK in the codepath that uses the
>> refactored building blocks to replace the original code, but it may
>> not be acceptable to leave the issue in the original code when a new
>> caller calls the refactored building block (e.g. perhaps the
>> refactoring made it possible for a caller to ask the function not to
>> die and instead act on different kinds of errors in different ways).
>
> In this case, ...

It becomes somewhat irritating when you get combative and defensive
unnecessarily.  We already established this case is OK two messages
ago, I think.

The above is only to make sure that people cannot take the "issues
in the original is OK to leave outside the scope of a new series" in
my message out of context and treat it as a general rule to justify
their sloppy patches in the future.  We need to see if issues
inherited from the original is necessary to fix before refactoring
even begins on case-by-case basis, seeing what the requirement of
the new code that uses the refactored code.  And the case-by-case
thing we already did for _this_ case.

Re: Possible git blame bug?

[Domagoj Stolfa]

Hello,

> > Thanks for clearing this up. Is this documented somewhere, so that if it 
> > happens
> > again I can point people to the docs that explain this behaviour?
> 
> Is this from "git blame --help" sufficient?
> 
> When you are not interested in changes older than version
> v2.6.18, or changes older than 3 weeks, you can use revision
> range specifiers  similar to 'git rev-list':
> 
> git blame v2.6.18.. -- foo
> git blame --since=3.weeks -- foo
> 
> When revision range specifiers are used to limit the annotation,
> lines that have not changed since the range boundary (either the
> commit v2.6.18 or the most recent commit that is more than 3
> weeks old in the above example) are blamed for that range
> boundary commit.

re-reading it post your explanation, it seems to make perfect sense. Thanks
again for the detailed explanation of the behaviour!

-- 
Best regards,
Domagoj Stolfa


signature.asc
Description: PGP signature

Re: [PATCH 1/1] archive: learn to include submodules in output archive

[Junio C Hamano]

Stefan Beller  writes:

> On Mon, Mar 13, 2017 at 3:12 PM, Stefan Beller  wrote:
>
>>> will recursively traverse submodules in the repository and
>>> consider their contents for inclusion in the output archive, subject to
>>> any pathspec filters.
>
> git-archive pays attention to export-ignore and export-subst attribute
> as read from .gitattributes or $GIT_DIR/info/attributes
>
> When recursing into submodules, we'd need to clarify if the attributes
> from the superproject or from the submodules are applied; or both.

I think the most natural expectation is for the output from the
"archive --recurse-submodules" command is to be logical
concatenation of "archive" run at the top-level and submodules,
adjusting the output from the latter with leading paths to the
submodules.  For that to happen, the attributes that apply to paths
inside a submodule must come from that submodule's setting.

Re: Possible git blame bug?

[Junio C Hamano]

Domagoj Stolfa  writes:

> Thanks for clearing this up. Is this documented somewhere, so that if it 
> happens
> again I can point people to the docs that explain this behaviour?

Is this from "git blame --help" sufficient?

When you are not interested in changes older than version
v2.6.18, or changes older than 3 weeks, you can use revision
range specifiers  similar to 'git rev-list':

git blame v2.6.18.. -- foo
git blame --since=3.weeks -- foo

When revision range specifiers are used to limit the annotation,
lines that have not changed since the range boundary (either the
commit v2.6.18 or the most recent commit that is more than 3
weeks old in the above example) are blamed for that range
boundary commit.

Re: Possible git blame bug?

[Domagoj Stolfa]

Hello,

> > For example, saying:
> >
> > $ git blame time.h --since=2017
> > ^e19f2a27ed8 (Domagoj Stolfa 2017-03-12 20:43:01 +0100  33) #ifndef 
> > _SYS_TIME_H_
> >
> > $ git blame time.h --since=2016
> > ^21613a57af9 (bz  2016-03-13 21:26:18 +  33) #ifndef _SYS_TIME_H_
> >
> > $ git blame time.h --since=2015
> > ^48507f436f0 (mav 2015-03-13 21:01:25 +  33) #ifndef _SYS_TIME_H_
> >
> > and so on, with different hashes.
> 
> The output lines "^deadbeef" does *NOT* mean that commit deadbeef
> changed the revision.  It just is telling you that the hisory was
> dug down to that revision and it was found that since that revision
> there is no change (and you told the command not to bother looking
> beyond that time range, so we do not know what happened before that
> time).
> 
> It is understandable, when your history has a lot of merges, the
> history traversal may stop at commits on different branches.
> 
> Imagine a case where the line in question never changed throughout
> the history:
> 
>   o---o---B
>  / \
> O---o---o---A---C---o---o
> 
> Imagine A is from 2015, B is from 2016 and C is from 2017.  C's
> first parent, i.e. C^1, is A and C^2 is B.
> 
> If you ask the command to stop digging when you hit a commit on or
> before 2017-03-13 (03-13 is because today's date is appended to your
> 2017), your traversal will stop at C and you get a line that begins
> with ^C.
> 
> If you ask it to stop at 2016, A won't be even looked at because it
> is older.  The command will keep digging from C to find B.  If B's
> parent is also newer than the cutoff, but its parent is older, then
> the line will be shown with ^ and commit object name of B's parent.
> 
> If you ask it to stop at 2015, the command will first consider A
> (C's earlier parent) and pass blame to the lines common between
> these two commits.  In this illustration, we are pretending that the
> file did not change throughout the hsitory, so blame for all lines
> are passed to A and we don't even look at B.  Then we keep digging
> through A to find the culprit, or hit a commit older than the
> specified cut-off time.  The line will be shown with ^A or perhaps
> its ancestor.
> 
> So it is entirely sane if you saw three boundary commits named with
> three different time ranges.

Thanks for clearing this up. Is this documented somewhere, so that if it happens
again I can point people to the docs that explain this behaviour?

-- 
Best regards,
Domagoj Stolfa


signature.asc
Description: PGP signature

Re: What's cooking in git.git (Mar 2017, #05; Mon, 13)

[Stefan Beller]

>
> * sb/rev-parse-show-superproject-root (2017-03-08) 1 commit
>  - rev-parse: add --show-superproject-working-tree
>
>  From a working tree of a repository, a new option of "rev-parse"
>  lets you ask if the repository is used as a submodule of another
>  project, and where the root level of the working tree of that
>  project (i.e. your superproject) is.
>
>  Almost there, but documentation needs a bit more work.

Care to clarify what parts of docs you mean?
https://public-inbox.org/git/xmqqr327z5rn@gitster.mtv.corp.google.com/
sounded like the topic is done.

Re: [PATCH 1/1] archive: learn to include submodules in output archive

[Stefan Beller]

On Mon, Mar 13, 2017 at 3:12 PM, Stefan Beller  wrote:

>> will recursively traverse submodules in the repository and
>> consider their contents for inclusion in the output archive, subject to
>> any pathspec filters.

git-archive pays attention to export-ignore and export-subst attribute
as read from .gitattributes or $GIT_DIR/info/attributes

When recursing into submodules, we'd need to clarify if the attributes
from the superproject or from the submodules are applied; or both.

Thanks,
Stefan

Re: [PATCH v3 00/10] decoupling a submodule's existence and its url

[Stefan Beller]

On Mon, Mar 13, 2017 at 2:43 PM, Brandon Williams  wrote:
> changes in v3:
>
> * Droped a patch which tried to use a more accurate URL for deinit.  It didn't
>   really fit inside the scope of this series.  It may be something we want to
>   revisit later though.
>
> * The --init-active flag now ensure that all submodules which are configured 
> to
>   be 'active' (either via 'submodule.active' or 'submodule..active') go
>   through the initialization phase and have their relevent info copied over to
>   the config.
>

This round looks sensible to me.

Thanks,
Stefan

Re: [PATCH v3 05/10] submodule: decouple url and submodule existence

[Stefan Beller]

+ cc Jens, FYI.

Once upon a time I brought up different addressing/activating mechanism for
submodules and I remember Jens having some uneasy thoughts about
that back in the day. This series addresses the user confusion and documentation
better than what I had back then.

On Mon, Mar 13, 2017 at 2:43 PM, Brandon Williams  wrote:
> Currently the submodule..url config option is used to determine
> if a given submodule exists and is interesting to the user.  This
> however doesn't work very well because the URL is a config option for
> the scope of a repository, whereas the existence of a submodule is an
> option scoped to the working tree.
>
> In a future with worktree support for submodules, there will be multiple
> working trees, each of which may only need a subset of the submodules
> checked out.  The URL (which is where the submodule repository can be
> obtained) should not differ between different working trees.
>
> It may also be convenient for users to more easily specify groups of
> submodules they are interested in as apposed to running "git submodule
> init " on each submodule they want checked out in their working
> tree.
>
> To this end two config options are introduced, submodule.active and
> submodule..active.  The submodule.active config holds a pathspec
> that specifies which submodules should exist in the working tree.  The
> submodule..active config is a boolean flag used to indicate if
> that particular submodule should exist in the working tree.
>
> Given these multiple ways to check for a submodule's existence the more
> fine-grained submodule..active option has the highest order of
> precedence followed by the pathspec check against submodule.active. To
> ensure backwards compatibility, if neither of these options are set git
> falls back to checking the submodule..url option to determine a
> submodule's existence.
>




>
> +submodule..active::
> +   Boolean value indicating if the submodule is of interest to git
> +   commands.  This config option takes precedence over the
> +   submodule.active config option.

... which itself takes precedence over the (deprecated) .URL
We conveniently do not talk about the URL here anymore.
But! We need to change submodule..URL now?

Re: Possible git blame bug?

[Junio C Hamano]

Junio C Hamano  writes:

> Domagoj Stolfa  writes:
>
>> For example, saying:
>>
>> $ git blame time.h --since=2017
>> ^e19f2a27ed8 (Domagoj Stolfa 2017-03-12 20:43:01 +0100  33) #ifndef 
>> _SYS_TIME_H_
>>
>> $ git blame time.h --since=2016
>> ^21613a57af9 (bz  2016-03-13 21:26:18 +  33) #ifndef _SYS_TIME_H_
>>
>> $ git blame time.h --since=2015
>> ^48507f436f0 (mav 2015-03-13 21:01:25 +  33) #ifndef _SYS_TIME_H_
>>
>> and so on, with different hashes.
>
> The output lines "^deadbeef" does *NOT* mean that commit deadbeef
> changed the revision.  It just is telling you that the hisory was
> dug down to that revision and it was found that since that revision
> there is no change (and you told the command not to bother looking
> beyond that time range, so we do not know what happened before that
> time).
> ...
> So it is entirely sane if you saw three boundary commits named with
> three different time ranges.

Side note.  If the displaying of the boundary commit object names
are distracting, the user can always use the -b option to blank them
out.

What's cooking in git.git (Mar 2017, #05; Mon, 13)

[Junio C Hamano]

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

A handful of topics are ready to be merged to 'next'.  Giving them a
final round of reviewing and testing is greatly appreciated:

- bc/object-id
- bw/attr-pathspec
- js/early-config

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

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

--
[Graduated to "master"]

* ah/doc-ls-files-quotepath (2017-03-02) 1 commit
  (merged to 'next' on 2017-03-10 at 5dfa78423a)
 + Documentation: improve description for core.quotePath

 Documentation for "git ls-files" did not refer to core.quotePath


* ax/line-log-range-merge-fix (2017-03-03) 1 commit
  (merged to 'next' on 2017-03-10 at 201073f113)
 + line-log.c: prevent crash during union of too many ranges

 The code to parse "git log -L..." command line was buggy when there
 are many ranges specified with -L; overrun of the allocated buffer
 has been fixed.


* jc/diff-populate-filespec-size-only-fix (2017-03-02) 1 commit
  (merged to 'next' on 2017-03-10 at 9b2d1ca50f)
 + diff: do not short-cut CHECK_SIZE_ONLY check in diff_populate_filespec()

 "git diff --quiet" relies on the size field in diff_filespec to be
 correctly populated, but diff_populate_filespec() helper function
 made an incorrect short-cut when asked only to populate the size
 field for paths that need to go through convert_to_git() (e.g. CRLF
 conversion).


* jh/mingw-openssl-sha1 (2017-02-09) 1 commit
  (merged to 'next' on 2017-03-10 at 8a1aa07def)
 + mingw: use OpenSSL's SHA-1 routines

 Windows port wants to use OpenSSL's implementation of SHA-1
 routines, so let them.


* jk/add-i-patch-do-prompt (2017-03-02) 1 commit
  (merged to 'next' on 2017-03-10 at 891ec6f5ba)
 + add--interactive: fix missing file prompt for patch mode with "-i"

 The patch subcommand of "git add -i" was meant to have paths
 selection prompt just like other subcommand, unlike "git add -p"
 directly jumps to hunk selection.  Recently, this was broken and
 "add -i" lost the paths selection dialog, but it now has been
 fixed.


* jk/ewah-use-right-type-in-sizeof (2017-03-06) 1 commit
  (merged to 'next' on 2017-03-10 at ad66adacda)
 + ewah: fix eword_t/uint64_t confusion

 Code clean-up.


* js/realpath-pathdup-fix (2017-03-08) 2 commits
  (merged to 'next' on 2017-03-10 at 5a84dbbd1d)
 + real_pathdup(): fix callsites that wanted it to die on error
 + t1501: demonstrate NULL pointer access with invalid GIT_WORK_TREE

 Git v2.12 was shipped with an embarrassing breakage where various
 operations that verify paths given from the user stopped dying when
 seeing an issue, and instead later triggering segfault.
 ... and then to down to 'maint'.


* ss/remote-bzr-hg-placeholder-wo-python (2017-03-03) 1 commit
  (merged to 'next' on 2017-03-10 at c8c4bb78a2)
 + contrib: git-remote-{bzr,hg} placeholders don't need Python

 There is no need for Python only to give a few messages to the
 standard error stream, but we somehow did.


* vn/line-log-memcpy-size-fix (2017-03-06) 1 commit
  (merged to 'next' on 2017-03-10 at 2e65ff89b7)
 + line-log: use COPY_ARRAY to fix mis-sized memcpy

 The command-line parsing of "git log -L" copied internal data
 structures using incorrect size on ILP32 systems.

--
[New Topics]

* ab/ref-filter-no-contains (2017-03-11) 1 commit
 - ref-filter: add --no-contains option to tag/branch/for-each-ref
 (this branch uses jk/ref-filter-flags-cleanup.)

 "git tag/branch/for-each-ref" family of commands long allowed to
 filter the refs by "--contains X" (show only the refs that are
 descendants of X), "--merged X" (show only the refs that are
 ancestors of X), "--no-merged X" (show only the refs that are not
 ancestors of X).  One curious omission, "--no-contains X" (show
 only the refs that are not descendants of X) has been added to
 them.

 Expecting a reroll.
 cf. 
 The topic is almost there.


* bc/sha1-header-selection-with-cpp-macros (2017-03-11) 1 commit
 - Move SHA-1 implementation selection into a header file

 Our source code has used the SHA1_HEADER cpp macro after "#include"
 in the C code to switch among the SHA-1 implementations. Instead,
 list the exact header file names and switch among implementations
 using "#ifdef BLK_SHA1/#include "block-sha1/sha1.h"/.../#endif";
 this helps some IDE tools.

 Expecting a reroll.


* bw/attr-pathspec (2017-03-13) 2 commits
 - pathspec: allow escaped query values
 - pathspec: allow querying for attributes

 The pathspec mechanism learned to further limit the paths that
 match the pattern to those that have specified 

Re: [PATCH v3 0/2] bringing attributes to pathspecs

[Brandon Williams]

On 03/13, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > v3 fixes some nits in style in the test script (using <<-\EOF instead of 
> > <<-EOF)
> > as well as fixing a few other minor things reported by Junio and Jonathan.
> 
> Thanks.  Will replace.
> 
> I think this is ready for 'next', so let's ask reviewers to really
> pay attention to this round, wait for a few days and merge it by the
> end of the week at the latest.

Sounds good to me, Thanks!

-- 
Brandon Williams

Re: [PATCH v3 01/10] submodule--helper: add is_active command

[Stefan Beller]

On Mon, Mar 13, 2017 at 2:43 PM, Brandon Williams  wrote:
> There are a lot of places where an explicit check for
> submodule."".url is done to see if a submodule exists.  In order
> to centralize this check introduce a helper which can be used to query
> if a submodule is active or not.

With this patch in mind, I need to take notes for rerolling
http://public-inbox.org/git/20170209020855.23486-1-sbel...@google.com/

>  #define SUPPORT_SUPER_PREFIX (1<<0)
...
> +   {"is-active", is_active, 0},

I think you can even mark it as SUPPORT_SUPER_PREFIX.
The only messages produced are from die()ing in git_config_get_string
in is_submodule_initialized.

This alone doesn't warrant a reroll though; just in case you do reroll, this
may be worth checking.

Re: [PATCH v6 00/12] Fix the early config

[Junio C Hamano]

Johannes Schindelin  writes:

> These patches are an attempt to make Git's startup sequence a bit less
> surprising.

I think this is ready for 'next', so let's ask reviewers to really
pay attention to this round, wait for a few days and merge it by the
end of the week at the latest.

Thanks.

Re: [PATCH v3 0/2] bringing attributes to pathspecs

[Junio C Hamano]

Brandon Williams  writes:

> v3 fixes some nits in style in the test script (using <<-\EOF instead of 
> <<-EOF)
> as well as fixing a few other minor things reported by Junio and Jonathan.

Thanks.  Will replace.

I think this is ready for 'next', so let's ask reviewers to really
pay attention to this round, wait for a few days and merge it by the
end of the week at the latest.

Thanks.

[PATCH/RFC V2] stash: implement builtin stash

[Joel Teichroeb]

Implement all git stash functionality as a builtin command

Signed-off-by: Joel Teichroeb 
---
I've been working on rewriting git stash as a c builtin and I have all
but three tests passing. I'm having a bit of trouble fixing them, as
well as a few other issues, so I'd really appreciate some help. Don't
bother commenting on the small details yet as I still need to go
though the code to make sure it matches the code style guidelines.

Test Summary Report
---
t7601-merge-pull-config.sh   (Wstat: 256 Tests: 14
Failed: 2)
  Failed tests:  11-12
  Non-zero exit status: 1
t3903-stash.sh   (Wstat: 256 Tests: 74
Failed: 1)
  Failed test:  69
  Non-zero exit status: 1

It looks to be the same issue for both of these cases where
merge-recursive reports:
error: Your local changes to the following files would be overwritten by merge:
file
other-file

which doesn't make sense as those files didn't exist before the merge.
Furthermore if I take the existing git stash implementation and have
it stop before running the merge-recursive command and then run it on
the commandline manually, I get the same issue. I've tried setting all
the same environment variables that the existing git stash
implementation does, but it doesn't help. It seems like there could be
a bug in merge-recursive, but I'm not sure how to track it down.

git stash uses the GIT_INDEX_FILE environment variable in order to not
trash the main index. I ended up doing things like this:

discard_cache();
ret = read_cache_from(index_path);
write_index_as_tree(orig_tree.hash, _index, index_path, 0, NULL);
discard_cache();
ret = read_cache_from(index_path);

in order to have an empty cache. Could someone take a look at my uses
of the index and point out better ways to do it?

My main goal right now is replace as many of the cmd_* calls as
possible.

changes in v2:
* Better follow coding guidelines
* Improve error handling

 Makefile  |2 +-
 builtin.h |1 +
 builtin/stash.c   | 1266 +
 git-stash.sh  |  731 ---
 git.c |1 +
 merge-recursive.c |5 +-
 6 files changed, 1271 insertions(+), 735 deletions(-)
 create mode 100644 builtin/stash.c
 delete mode 100755 git-stash.sh

diff --git a/Makefile b/Makefile
index ba524f3a7..73915a2e0 100644
--- a/Makefile
+++ b/Makefile
@@ -516,7 +516,6 @@ SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
-SCRIPT_SH += git-stash.sh
 SCRIPT_SH += git-submodule.sh
 SCRIPT_SH += git-web--browse.sh
 
@@ -949,6 +948,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 9e4a89816..16e8a437d 100644
--- a/builtin.h
+++ b/builtin.h
@@ -121,6 +121,7 @@ extern int cmd_shortlog(int argc, const char **argv, const 
char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash.c b/builtin/stash.c
new file mode 100644
index 0..785fc18d5
--- /dev/null
+++ b/builtin/stash.c
@@ -0,0 +1,1266 @@
+#include "builtin.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "tree.h"
+#include "lockfile.h"
+#include "object.h"
+#include "tree-walk.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "diff.h"
+#include "revision.h"
+#include "commit.h"
+#include "diffcore.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+
+static const char * const git_stash_usage[] = {
+   N_("git stash list []"),
+   N_("git stash show []"),
+   N_("git stash drop [-q|--quiet] []"),
+   N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"),
+   N_("git stash branch  []"),
+   N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"),
+   N_("[-u|--include-untracked] [-a|--all] []]"),
+   N_("git stash clear"),
+   N_("git stash create []"),
+   N_("git stash store [-m|--message ] [-q|--quiet] "),
+   NULL
+};
+
+static const char * const git_stash_list_usage[] = {
+   N_("git stash list []"),
+   NULL
+};
+
+static const 

Re: Possible git blame bug?

[Junio C Hamano]

Domagoj Stolfa  writes:

> For example, saying:
>
> $ git blame time.h --since=2017
> ^e19f2a27ed8 (Domagoj Stolfa 2017-03-12 20:43:01 +0100  33) #ifndef 
> _SYS_TIME_H_
>
> $ git blame time.h --since=2016
> ^21613a57af9 (bz  2016-03-13 21:26:18 +  33) #ifndef _SYS_TIME_H_
>
> $ git blame time.h --since=2015
> ^48507f436f0 (mav 2015-03-13 21:01:25 +  33) #ifndef _SYS_TIME_H_
>
> and so on, with different hashes.

The output lines "^deadbeef" does *NOT* mean that commit deadbeef
changed the revision.  It just is telling you that the hisory was
dug down to that revision and it was found that since that revision
there is no change (and you told the command not to bother looking
beyond that time range, so we do not know what happened before that
time).

It is understandable, when your history has a lot of merges, the
history traversal may stop at commits on different branches.

Imagine a case where the line in question never changed throughout
the history:

  o---o---B
 / \
O---o---o---A---C---o---o

Imagine A is from 2015, B is from 2016 and C is from 2017.  C's
first parent, i.e. C^1, is A and C^2 is B.

If you ask the command to stop digging when you hit a commit on or
before 2017-03-13 (03-13 is because today's date is appended to your
2017), your traversal will stop at C and you get a line that begins
with ^C.

If you ask it to stop at 2016, A won't be even looked at because it
is older.  The command will keep digging from C to find B.  If B's
parent is also newer than the cutoff, but its parent is older, then
the line will be shown with ^ and commit object name of B's parent.

If you ask it to stop at 2015, the command will first consider A
(C's earlier parent) and pass blame to the lines common between
these two commits.  In this illustration, we are pretending that the
file did not change throughout the hsitory, so blame for all lines
are passed to A and we don't even look at B.  Then we keep digging
through A to find the culprit, or hit a commit older than the
specified cut-off time.  The line will be shown with ^A or perhaps
its ancestor.

So it is entirely sane if you saw three boundary commits named with
three different time ranges.

Re: [PATCH 1/1] archive: learn to include submodules in output archive

[Stefan Beller]

Welcome to the Git community!

On Sat, Mar 11, 2017 at 11:54 PM, Nikhil Benesch
 wrote:
> This commit is a revival of Lars Hjemli's 2009 patch to provide an
> option to include submodules in the output of `git archive`.

I am unaware of said patch, could you link to it, e.g.
on https://public-inbox.org/git/ ?

>
> The `--recurse-submodules` option (named consistently with fetch, clone,
> and ls-files)

... and unlike fetch and push we do not need to introduce extra options?
(or they can be added later when the need arises)

> will recursively traverse submodules in the repository and
> consider their contents for inclusion in the output archive, subject to
> any pathspec filters.

ok.

> Like other commands that have learned
> `--recurse-submodules`, submodules that have not been checked out will
> not be traversed.

Junio writes:
> A question to the submodule folks: Is "checked-out" the right terminology
> in this context?  I am wondering if we have reached more official set
> of words to express submodule states discussed in [*1*].

http://public-inbox.org/git/20170209020855.23486-1-sbel...@google.com/

The "checked-out" here would translate to "populated" in said proposal.

I guess that is sufficient for the first implementation, but eventually we
might be interested in "recurse-submodules=active/init" as well.
Consider the case, where you delete a submodule and commit it.
Then you still want to be able to create an archive for HEAD^ (with
submodules). So in that case we'd want to recurse into any submodule
that has a git directory with the commit as recorded by the superproject,
i.e. the example given would refer to "depopulated" in the referenced
proposal.

When the initialization is missing, we may not be interested in that
submodule any more, but we don't know for the user, so a user may
want to ask for "archive --recurse-submodules={all / initialized-only /
all-submoduless-with-git-dir, none, working-tree}". No need to add all
these options in the first patch, but keep that in mind for future
extensions.

> @@ -59,6 +59,10 @@ OPTIONS
> Look for attributes in .gitattributes files in the working tree
> as well (see <>).
>
> +--recurse-submodules::
> +   Recursively include the contents of any checked-out submodules in
> +   the archive.
> +

Here is "any", in the commit message we had "subject to any pathspec filters."

> @@ -132,18 +133,15 @@ static int write_archive_entry(const unsigned char 
> *sha1, const char *base,
> args->convert = ATTR_TRUE(check->items[1].value);
> }
>
> -   if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
> -   if (args->verbose)
> -   fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
> -   err = write_entry(args, sha1, path.buf, path.len, mode);
> -   if (err)
> -   return err;
> -   return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
> -   }
> -
> if (args->verbose)
> fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
> -   return write_entry(args, sha1, path.buf, path.len, mode);
> +   err = write_entry(args, sha1, path.buf, path.len, mode);
> +   if (err)
> +   return err;
> +   if (S_ISDIR(mode) || (S_ISGITLINK(mode) && args->recurse_submodules &&
> + !add_submodule_odb(path_without_prefix)))

This is a bit hard to read. Maybe reformatting can help

 if (S_ISDIR(mode) ||
(S_ISGITLINK(mode) &&
 args->recurse_submodules &&
 !add_submodule_odb(path_without_prefix)))
return ...

Though I wonder if we need to special case the
return value of add_submodule_odb as that could
be an error instead of fall through "return 0;" ?

> @@ -419,6 +418,8 @@ static int parse_archive_args(int argc, const char **argv,
> N_("prepend prefix to each pathname in the archive")),
> OPT_STRING('o', "output", , N_("file"),
> N_("write the archive to this file")),
> +   OPT_BOOL(0, "recurse-submodules", _submodules,
> +   N_("recurse through submodules")),

This is the first time hearing "through" used with submodules.
I guess it makes sense when looking at the contents on disk as
a tree data structure.

push:N_("control recursive pushing of submodules"),
fetch:N_("control recursive fetching of submodules"),
grep:N_("recursivley search in each submodule")),
ls-files:N_("recurse through submodules")),

Ok, we introduced recursing "through" submodules with ls-files.

...
> +
> +add_submodule()
> +{
> +   mkdir $1 && (
> +   cd $1 &&
> +   git init &&
> +   echo "File $2" >$2 &&
> +   add_file $2

test_create_repo / test_commit
might come in handy here as well.

> +   ) &&
> +   add_file $1
> +}
> +
> +test_expect_success 'by default, submodules are not 

Re: [GSoC][PATCH v2 2/2] credential-cache: use XDG_CACHE_HOME for socket

[Junio C Hamano]

Devin Lehmacher  writes:

> git-credential-cache will now use a socket following the XDG base path
> specification by default. This increases consistency with other
> applications and helps keep clutter out of users' home directories.

We tend to write our log messages in imperative mood, as if you are
giving an order to the codebase to "be like so" (alternatively, you
can read them as if you are giving an order to the maintainer of the
code to "make these changes").

We have been using ~/.git-credential-cache/socket as the
location to store the UNIX socket to communicate with the
credential daemon process.  In order to make it more
consistent with other applications and reduce clutter in the
home directory, move it to $XDG_CACHE_HOME/git/credential/socket,
which matches what XDG base path specification suggests.

Similarly for the other two paragraphs.  Instead of "We still
check the old location ...", just "Check the old location ...", etc.

> If there is not a socket at that location we create a new one at
> $XDG_CACHE_HOME/git/credential/socket. This complies with the XDG
> standard and good for the reasons previously mentioned. 

And the second sentence can go; you already said why you think
XDG_CACHE_HOME is a good idea.

> We use the
> subdirectory credential/ in case we later want to store other files
> under $XDG_CACHE_HOME/git/ and to make the purpose of the socket clear.

And this probably can disappear (or rolled into the first paragraph,
if we really want; personally I think it is obvious why we want the
extra "credential" directory under "git" there).

> I also change to documentation to reflect the new default socket
> location.

This probably does not have to be said, as it is obvious from the
diffstat.

Missing sign-off.

> ---
>  Documentation/git-credential-cache.txt |  3 ++-
>  credential-cache.c | 16 +++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-credential-cache.txt 
> b/Documentation/git-credential-cache.txt
> index 96208f822..4b9db3856 100644
> --- a/Documentation/git-credential-cache.txt
> +++ b/Documentation/git-credential-cache.txt
> @@ -34,7 +34,8 @@ OPTIONS
>  
>   Use `` to contact a running cache daemon (or start a new
>   cache daemon if one is not started). Defaults to
> - `~/.git-credential-cache/socket`. If your home directory is on a
> + `~/.git-credential-cache/socket` if it exists and otherwise
> +`$XDG_CACHE_HOME/git/credential/socket`. If your home directory is on a

There is a funny indentation here.

>   network-mounted filesystem, you may need to change this to a
>   local filesystem. You must specify an absolute path.
>  
> diff --git a/credential-cache.c b/credential-cache.c
> index cc8a6ee19..db1343b46 100644
> --- a/credential-cache.c
> +++ b/credential-cache.c
> @@ -83,6 +83,20 @@ static void do_cache(const char *socket, const char 
> *action, int timeout,
>   strbuf_release();
>  }
>  
> +static char *get_socket_path(void) {
> + char *home_socket;
> +
> + home_socket = expand_user_path("~/.git-credential-cache/socket");
> + if (home_socket) {
> + if (file_exists(home_socket))

Don't we want to make sure that this path _is_ a socket?  In general
I think file_exists() is a poor choice to use here (the existing use
are all about having a regular file there, and its definition may be
later tightened from "does lstat() succeed?" to something a bit more
sensible, and FIFO may start failing the updated test.

Thanks.

Re: [PATCH v5 10/11] setup_git_directory_gently_1(): avoid die()ing

[Johannes Schindelin]

Hi Junio,

On Mon, 13 Mar 2017, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > The former case may split into two.  "Yes I agree that is bad and it
> > is the same badness as the version without this change", in which case
> > we may want to leave a "NEEDSWORK" comment in-code so that people can
> > notice when browsing the code (but fixing the badness would be outside
> > the scope of the series), and "yes that is bad and we shouldn't
> > introduce that badness", in which case we do want to fix it in the
> > patch.
> 
> We however need to be a bit careful here, though.
> 
> When a patch series is refactoring an existing function to be used
> in different codepaths, an existing issue inherited from the
> original code (e.g. perhaps an existing error checking that is
> looser than ideal) may have been OK in the original context (e.g.
> perhaps it died and refused to run until the user corrected the
> repository), and it still is OK in the codepath that uses the
> refactored building blocks to replace the original code, but it may
> not be acceptable to leave the issue in the original code when a new
> caller calls the refactored building block (e.g. perhaps the
> refactoring made it possible for a caller to ask the function not to
> die and instead act on different kinds of errors in different ways).

In this case, discover_git_directory() is intended to use the exact same
logic as setup_git_directory() (including bugs and all) so that they do
discover the same directory.

It would not do for discover_git_directory() to detect a *different*
directory, no matter how much "more correct" one would deem it.

Ciao,
Johannes

Re: [PATCH v6 03/12] Prepare setup_discovered_git_directory() the root directory

[Johannes Schindelin]

Hi Junio,

On Mon, 13 Mar 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Subject: Re: [PATCH v6 03/12] Prepare setup_discovered_git_directory() the 
> > root directory
> 
> I do not think you've changed this title throughout the rerolls, but
> I cannot quite parse it.

That is because a "for" is missing between "the" and "root".

Ciao,
Dscho

[PATCH v3 06/10] submodule update: add `--init-active` switch

[Brandon Williams]

The new switch `--init-active` initializes the submodules which are
configured in `submodule.active` and `submodule..active` instead
of those given as command line arguments before updating. In the first
implementation this is made incompatible with further command line
arguments as it is unclear what the user means by

git submodule update --init --init-active 

This new switch allows users to record more complex patterns as it saves
retyping them whenever you invoke update.

Based on a patch by Stefan Beller 
Signed-off-by: Brandon Williams 
---
 Documentation/git-submodule.txt |  9 +++
 builtin/submodule--helper.c | 35 -
 git-submodule.sh| 26 +++---
 t/t7400-submodule-basic.sh  | 58 +
 4 files changed, 123 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index e05d0cdde..6b17cd707 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -198,6 +198,10 @@ If the submodule is not yet initialized, and you just want 
to use the
 setting as stored in .gitmodules, you can automatically initialize the
 submodule with the `--init` option.
 
+You can configure a set of submodules using pathspec syntax in
+submodule.active you can use `--init-active` to initialize
+those before updating.
+
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and update any nested submodules within.
 --
@@ -384,6 +388,11 @@ the submodule itself.
Initialize all submodules for which "git submodule init" has not been
called so far before updating.
 
+--init-active::
+   This option is only valid for the update command.
+   Initialize all submodules configured in "`submodule.active`"
+   that have not been updated before.
+
 --name::
This option is only valid for the add command. It sets the submodule's
name to the given string instead of defaulting to its path. The name
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f38e332c5..a3acc9e4c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -270,6 +270,34 @@ static int module_list_compute(int argc, const char **argv,
return result;
 }
 
+static void module_list_active(struct module_list *list)
+{
+   int i;
+
+   if (read_cache() < 0)
+   die(_("index file corrupt"));
+
+   gitmodules_config();
+
+   for (i = 0; i < active_nr; i++) {
+   const struct cache_entry *ce = active_cache[i];
+
+   if (!S_ISGITLINK(ce->ce_mode) ||
+   !is_submodule_initialized(ce->name))
+   continue;
+
+   ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
+   list->entries[list->nr++] = ce;
+   while (i + 1 < active_nr &&
+  !strcmp(ce->name, active_cache[i + 1]->name))
+   /*
+* Skip entries with the same name in different stages
+* to make sure an entry is returned only once.
+*/
+   i++;
+   }
+}
+
 static int module_list(int argc, const char **argv, const char *prefix)
 {
int i;
@@ -402,9 +430,12 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
struct pathspec pathspec;
struct module_list list = MODULE_LIST_INIT;
int quiet = 0;
+   int active = 0;
int i;
 
struct option module_init_options[] = {
+   OPT_BOOL(0, "active", ,
+N_("ensure all active submodules are properly 
initialized")),
OPT__QUIET(, N_("Suppress output for initializing a 
submodule")),
OPT_END()
};
@@ -417,7 +448,9 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
argc = parse_options(argc, argv, prefix, module_init_options,
 git_submodule_helper_usage, 0);
 
-   if (module_list_compute(argc, argv, prefix, , ) < 0)
+   if (active)
+   module_list_active();
+   else if (module_list_compute(argc, argv, prefix, , ) < 0)
return 1;
 
for (i = 0; i < list.nr; i++)
diff --git a/git-submodule.sh b/git-submodule.sh
index e2d08595f..3c7da08aa 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b ] [-f|--force] [--name ] 
[--reference ...]
or: $dashless [--quiet] init [--] [...]
or: $dashless [--quiet] deinit [-f|--force] (--all| [--] ...)
-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] 
[--reference ] [--recursive] [--] [...]
+   or: $dashless [--quiet] update [--init[-active]] [--remote] [-N|--no-fetch] 

[PATCH v3 10/10] submodule add: respect submodule.active and submodule..active

[Brandon Williams]

In addition to adding submodule..url to the config, set
submodule..active to true unless submodule.active is configured
and the submodule's path matches the configured pathspec.

Signed-off-by: Brandon Williams 
---
 git-submodule.sh   | 12 
 t/t7413-submodule-is-active.sh | 21 +
 2 files changed, 33 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index 3c7da08aa..2c510038d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -278,6 +278,18 @@ or you are unsure what this means choose another name with 
the '--name' option."
fi &&
git add --force .gitmodules ||
die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
+
+   if git config --get submodule.active >/dev/null
+   then
+   # If the submodule being adding isn't already covered by the
+   # current configured pathspec, set the submodule's active flag
+   if ! git submodule--helper is-active "$sm_path"
+   then
+   git config --add submodule."$sm_name".active "true"
+   fi
+   else
+   git config --add submodule."$sm_name".active "true"
+   fi
 }
 
 #
diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
index c41b899ab..865931978 100755
--- a/t/t7413-submodule-is-active.sh
+++ b/t/t7413-submodule-is-active.sh
@@ -15,6 +15,12 @@ test_expect_success 'setup' '
test_commit -C super initial &&
git -C super submodule add ../sub sub1 &&
git -C super submodule add ../sub sub2 &&
+
+   # Remove submodule..active entries in order to test in an
+   # environment where only URLs are present in the conifg
+   git -C super config --unset submodule.sub1.active &&
+   git -C super config --unset submodule.sub2.active &&
+
git -C super commit -a -m "add 2 submodules at sub{1,2}"
 '
 
@@ -83,4 +89,19 @@ test_expect_success 'is-active with submodule.active and 
submodule..active
git -C super config --unset submodule.sub2.active
 '
 
+test_expect_success 'is-active, submodule.active and submodule add' '
+   test_when_finished "rm -rf super2" &&
+   git init super2 &&
+   test_commit -C super2 initial &&
+   git -C super2 config --add submodule.active "sub*" &&
+
+   # submodule add should only add submodule..active
+   # to the config if not matched by the pathspec
+   git -C super2 submodule add ../sub sub1 &&
+   test_must_fail git -C super2 config --get submodule.sub1.active &&
+
+   git -C super2 submodule add ../sub mod &&
+   git -C super2 config --get submodule.mod.active
+'
+
 test_done
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH v3 09/10] submodule--helper init: set submodule..active

[Brandon Williams]

When initializing a submodule set the submodule..active config to
true to indicate that the submodule is active.

Signed-off-by: Brandon Williams 
---
 builtin/submodule--helper.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a3acc9e4c..b669ed031 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -361,6 +361,13 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
die(_("No url found for submodule path '%s' in .gitmodules"),
displaypath);
 
+   /* Set active flag for the submodule being initialized */
+   if (!is_submodule_initialized(path)) {
+   strbuf_reset();
+   strbuf_addf(, "submodule.%s.active", sub->name);
+   git_config_set_gently(sb.buf, "true");
+   }
+
/*
 * Copy url setting when it is not set yet.
 * To look up the url in .git/config, we must not fall back to
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH v3 04/10] submodule--helper clone: check for configured submodules using helper

[Brandon Williams]

Use the 'is_submodule_initialized()' helper to check for configured
submodules instead of manually checking for the submodule's URL in the
config.

Signed-off-by: Brandon Williams 
---
 builtin/submodule--helper.c | 15 +++
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5fe7e23b1..f38e332c5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -759,7 +759,6 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
struct strbuf displaypath_sb = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
const char *displaypath = NULL;
-   char *url = NULL;
int needs_cloning = 0;
 
if (ce_stage(ce)) {
@@ -793,15 +792,8 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
goto cleanup;
}
 
-   /*
-* Looking up the url in .git/config.
-* We must not fall back to .gitmodules as we only want
-* to process configured submodules.
-*/
-   strbuf_reset();
-   strbuf_addf(, "submodule.%s.url", sub->name);
-   git_config_get_string(sb.buf, );
-   if (!url) {
+   /* Check if the submodule has been initialized. */
+   if (!is_submodule_initialized(ce->name)) {
next_submodule_warn_missing(suc, out, displaypath);
goto cleanup;
}
@@ -835,7 +827,7 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
argv_array_push(>args, "--depth=1");
argv_array_pushl(>args, "--path", sub->path, NULL);
argv_array_pushl(>args, "--name", sub->name, NULL);
-   argv_array_pushl(>args, "--url", url, NULL);
+   argv_array_pushl(>args, "--url", sub->url, NULL);
if (suc->references.nr) {
struct string_list_item *item;
for_each_string_list_item(item, >references)
@@ -845,7 +837,6 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
argv_array_push(>args, suc->depth);
 
 cleanup:
-   free(url);
strbuf_reset(_sb);
strbuf_reset();
 
-- 
2.12.0.246.ga2ecc84866-goog

Re: Possible git blame bug?

[Domagoj Stolfa]

Hello,

> >> The question is whether this is a bug or not, as --since= might not 
> >> be a
> >> valid filter.
> >
> > I do not think blame ever was designed to work with --since, so that
> > is indeed the case.
> 
> Actually, I do see that we had a cut-off based on rev->max_age since we
> introduced it at cee7f245 ("git-pickaxe: blame rewritten.", 2006-10-19).
> 
> I do not know offhand if --since=2000 _means_ --since=2000-01-01 or something
> completely different, though.

It seems to indicate something entirely different. The problem with it is that
it mentions commits that haven't even touched the file though. Output with
commit hashes that have touched that file would be sensible, albeit wrong in the
sense that the user did not want to see that behaviour.

For example, saying:

$ git blame time.h --since=2017
^e19f2a27ed8 (Domagoj Stolfa 2017-03-12 20:43:01 +0100  33) #ifndef _SYS_TIME_H_

$ git blame time.h --since=2016
^21613a57af9 (bz  2016-03-13 21:26:18 +  33) #ifndef _SYS_TIME_H_

$ git blame time.h --since=2015
^48507f436f0 (mav 2015-03-13 21:01:25 +  33) #ifndef _SYS_TIME_H_

and so on, with different hashes.

Looking at these commits:

(1) 
https://github.com/dstolfa/freebsd/commit/e19f2a27ed82f616d47dd4e0dc75722139e72957
(2) 
https://github.com/dstolfa/freebsd/commit/21613a57af9500acca9b3528958312dd3ae12bb4
(3) 
https://github.com/dstolfa/freebsd/commit/48507f436f07a9515c6d7108509a50dd4fd403b2

neither of them seems to touch time.h, yet git blame reports them to do so.
Interestingly enough, it seems to be taking the commit that is the closest to
the current date in that year, and blaming it on that one, regardless of what it
actually did and the file specified.

-- 
Best regards,
Domagoj Stolfa


signature.asc
Description: PGP signature

[PATCH v3 08/10] completion: clone can initialize specific submodules

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 contrib/completion/git-completion.bash | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index fc32286a4..eb13433d5 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1212,6 +1212,7 @@ _git_clone ()
--recurse-submodules
--no-single-branch
--shallow-submodules
+   --submodule-spec
"
return
;;
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH v3 07/10] clone: add --submodule-spec= switch

[Brandon Williams]

The new switch passes the pathspec to `git submodule update
--init-active` which is called after the actual clone is done.

Additionally this configures the submodule.active option to
be the given pathspec, such that any future invocation of
`git submodule update --init-active` will keep up
with the pathspec.

Based on a patch by Stefan Beller 

Signed-off-by: Brandon Williams 
---
 Documentation/git-clone.txt | 23 ++-
 builtin/clone.c | 36 +--
 t/t7400-submodule-basic.sh  | 70 +
 3 files changed, 120 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 35cc34b2f..9692eab30 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,8 @@ SYNOPSIS
  [--dissociate] [--separate-git-dir ]
  [--depth ] [--[no-]single-branch]
  [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
- [--jobs ] [--]  []
+ [--submodule-spec ] [--jobs ] [--]
+  []
 
 DESCRIPTION
 ---
@@ -217,12 +218,20 @@ objects from the source repository into a pack in the 
cloned repository.
 
 --recursive::
 --recurse-submodules::
-   After the clone is created, initialize all submodules within,
-   using their default settings. This is equivalent to running
-   `git submodule update --init --recursive` immediately after
-   the clone is finished. This option is ignored if the cloned
-   repository does not have a worktree/checkout (i.e. if any of
-   `--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
+   After the clone is created, initialize and clone all submodules
+   within, using their default settings. This is equivalent to
+   running `git submodule update --recursive --init` immediately
+   after the clone is finished. This option is ignored if the
+   cloned repository does not have a worktree/checkout (i.e.  if
+   any of `--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
+
+--submodule-spec::
+   After the clone is created, initialize and clone specified
+   submodules within, using their default settings. It is possible
+   to give multiple specifications by giving this argument multiple
+   times. This is equivalent to configuring `submodule.active`
+   and then running `git submodule update --init-active`
+   immediately after the clone is finished.
 
 --[no-]shallow-submodules::
All submodules which are cloned will be shallow with a depth of 1.
diff --git a/builtin/clone.c b/builtin/clone.c
index 3f63edbbf..c6731379b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -56,6 +56,16 @@ static struct string_list option_required_reference = 
STRING_LIST_INIT_NODUP;
 static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
+static struct string_list submodule_spec;
+
+static int submodule_spec_cb(const struct option *opt, const char *arg, int 
unset)
+{
+   if (unset)
+   return -1;
+
+   string_list_append((struct string_list *)opt->value, arg);
+   return 0;
+}
 
 static struct option builtin_clone_options[] = {
OPT__VERBOSITY(_verbosity),
@@ -112,6 +122,9 @@ static struct option builtin_clone_options[] = {
TRANSPORT_FAMILY_IPV4),
OPT_SET_INT('6', "ipv6", , N_("use IPv6 addresses only"),
TRANSPORT_FAMILY_IPV6),
+   OPT_CALLBACK(0, "submodule-spec", _spec, N_(""),
+   N_("clone specific submodules. Pass multiple times for 
complex pathspecs"),
+   submodule_spec_cb),
OPT_END()
 };
 
@@ -733,13 +746,21 @@ static int checkout(int submodule_progress)
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
   sha1_to_hex(sha1), "1", NULL);
 
-   if (!err && option_recursive) {
+   if (!err && (option_recursive || submodule_spec.nr > 0)) {
struct argv_array args = ARGV_ARRAY_INIT;
-   argv_array_pushl(, "submodule", "update", "--init", 
"--recursive", NULL);
+   argv_array_pushl(, "submodule", "update", NULL);
+
+   if (submodule_spec.nr > 0)
+   argv_array_pushf(, "--init-active");
+   else
+   argv_array_pushf(, "--init");
 
if (option_shallow_submodules == 1)
argv_array_push(, "--depth=1");
 
+   if (option_recursive)
+   argv_array_pushf(, "--recursive");
+
if (max_jobs != -1)
argv_array_pushf(, "--jobs=%d", max_jobs);
 
@@ -887,6 +908,17 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
option_no_checkout = 1;
}
 
+   if (submodule_spec.nr > 0) {
+

[PATCH v3 03/10] submodule sync: use submodule--helper is-active

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ab233712d..e2d08595f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -,7 +,7 @@ cmd_sync()
;;
esac
 
-   if git config "submodule.$name.url" >/dev/null 2>/dev/null
+   if git submodule--helper is-active "$sm_path"
then
displaypath=$(git submodule--helper relative-path 
"$prefix$sm_path" "$wt_prefix")
say "$(eval_gettext "Synchronizing submodule url for 
'\$displaypath'")"
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH v3 05/10] submodule: decouple url and submodule existence

[Brandon Williams]

Currently the submodule..url config option is used to determine
if a given submodule exists and is interesting to the user.  This
however doesn't work very well because the URL is a config option for
the scope of a repository, whereas the existence of a submodule is an
option scoped to the working tree.

In a future with worktree support for submodules, there will be multiple
working trees, each of which may only need a subset of the submodules
checked out.  The URL (which is where the submodule repository can be
obtained) should not differ between different working trees.

It may also be convenient for users to more easily specify groups of
submodules they are interested in as apposed to running "git submodule
init " on each submodule they want checked out in their working
tree.

To this end two config options are introduced, submodule.active and
submodule..active.  The submodule.active config holds a pathspec
that specifies which submodules should exist in the working tree.  The
submodule..active config is a boolean flag used to indicate if
that particular submodule should exist in the working tree.

Given these multiple ways to check for a submodule's existence the more
fine-grained submodule..active option has the highest order of
precedence followed by the pathspec check against submodule.active. To
ensure backwards compatibility, if neither of these options are set git
falls back to checking the submodule..url option to determine a
submodule's existence.

Signed-off-by: Brandon Williams 
---
 Documentation/config.txt   | 15 ++--
 submodule.c| 36 ---
 t/t7413-submodule-is-active.sh | 55 ++
 3 files changed, 100 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5e5c2ae5f..d2d79b9d4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2920,8 +2920,9 @@ submodule..url::
The URL for a submodule. This variable is copied from the .gitmodules
file to the git config via 'git submodule init'. The user can change
the configured URL before obtaining the submodule via 'git submodule
-   update'. After obtaining the submodule, the presence of this variable
-   is used as a sign whether the submodule is of interest to git commands.
+   update'. If neither submodule..active or submodule.active are
+   set, the presence of this variable is used as a fallback to indicate
+   whether the submodule is of interest to git commands.
See linkgit:git-submodule[1] and linkgit:gitmodules[5] for details.
 
 submodule..update::
@@ -2959,6 +2960,16 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
 
+submodule..active::
+   Boolean value indicating if the submodule is of interest to git
+   commands.  This config option takes precedence over the
+   submodule.active config option.
+
+submodule.active::
+   A repeated field which contains a pathspec used to match against a
+   submodule's path to determine if the submodule is of interest to git
+   commands.
+
 submodule.fetchJobs::
Specifies how many submodules are fetched/cloned at the same time.
A positive integer allows up to that number of submodules fetched
diff --git a/submodule.c b/submodule.c
index 0a2831d84..2b33bd70f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -217,13 +217,41 @@ void gitmodules_config_sha1(const unsigned char 
*commit_sha1)
 int is_submodule_initialized(const char *path)
 {
int ret = 0;
-   const struct submodule *module = NULL;
+   char *key;
+   const struct string_list *sl;
+   const struct submodule *module = submodule_from_path(null_sha1, path);
 
-   module = submodule_from_path(null_sha1, path);
+   /* early return if there isn't a path->module mapping */
+   if (!module)
+   return 0;
+
+   /* submodule..active is set */
+   key = xstrfmt("submodule.%s.active", module->name);
+   if (!git_config_get_bool(key, )) {
+   free(key);
+   return ret;
+   }
+   free(key);
+
+   sl = git_config_get_value_multi("submodule.active");
 
-   if (module) {
-   char *key = xstrfmt("submodule.%s.url", module->name);
+   if (sl) {
+   struct pathspec ps;
+   struct argv_array args = ARGV_ARRAY_INIT;
+   const struct string_list_item *item;
+
+   for_each_string_list_item(item, sl) {
+   argv_array_push(, item->string);
+   }
+
+   parse_pathspec(, 0, 0, 0, args.argv);
+   ret = match_pathspec(, path, strlen(path), 0, NULL, 1);
+
+   argv_array_clear();
+   clear_pathspec();
+   } else {
char *value = NULL;
+   key = 

[PATCH v3 02/10] submodule status: use submodule--helper is-active

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 git-submodule.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 136e26a2c..ab233712d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1010,14 +1010,13 @@ cmd_status()
do
die_if_unmatched "$mode" "$sha1"
name=$(git submodule--helper name "$sm_path") || exit
-   url=$(git config submodule."$name".url)
displaypath=$(git submodule--helper relative-path 
"$prefix$sm_path" "$wt_prefix")
if test "$stage" = U
then
say "U$sha1 $displaypath"
continue
fi
-   if test -z "$url" ||
+   if ! git submodule--helper is-active "$sm_path" ||
{
! test -d "$sm_path"/.git &&
! test -f "$sm_path"/.git
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH v3 01/10] submodule--helper: add is_active command

[Brandon Williams]

There are a lot of places where an explicit check for
submodule."".url is done to see if a submodule exists.  In order
to centralize this check introduce a helper which can be used to query
if a submodule is active or not.

Signed-off-by: Brandon Williams 
---
 builtin/submodule--helper.c| 11 +++
 t/t7413-submodule-is-active.sh | 31 +++
 2 files changed, 42 insertions(+)
 create mode 100755 t/t7413-submodule-is-active.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 15a5430c0..5fe7e23b1 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1127,6 +1127,16 @@ static int absorb_git_dirs(int argc, const char **argv, 
const char *prefix)
return 0;
 }
 
+static int is_active(int argc, const char **argv, const char *prefix)
+{
+   if (argc != 2)
+   die("submodule--helper is-active takes exactly 1 arguments");
+
+   gitmodules_config();
+
+   return !is_submodule_initialized(argv[1]);
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -1146,6 +1156,7 @@ static struct cmd_struct commands[] = {
{"init", module_init, SUPPORT_SUPER_PREFIX},
{"remote-branch", resolve_remote_submodule_branch, 0},
{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
+   {"is-active", is_active, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
new file mode 100755
index 0..f18e0c925
--- /dev/null
+++ b/t/t7413-submodule-is-active.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+
+test_description='Test submodule--helper is-active
+
+This test verifies that `git submodue--helper is-active` correclty identifies
+submodules which are "active" and interesting to the user.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+   git init sub &&
+   test_commit -C sub initial &&
+   git init super &&
+   test_commit -C super initial &&
+   git -C super submodule add ../sub sub1 &&
+   git -C super submodule add ../sub sub2 &&
+   git -C super commit -a -m "add 2 submodules at sub{1,2}"
+'
+
+test_expect_success 'is-active works with urls' '
+   git -C super submodule--helper is-active sub1 &&
+   git -C super submodule--helper is-active sub2 &&
+
+   git -C super config --unset submodule.sub1.URL &&
+   test_must_fail git -C super submodule--helper is-active sub1 &&
+   git -C super config submodule.sub1.URL ../sub &&
+   git -C super submodule--helper is-active sub1
+'
+
+test_done
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH v3 00/10] decoupling a submodule's existence and its url

[Brandon Williams]

changes in v3:

* Droped a patch which tried to use a more accurate URL for deinit.  It didn't
  really fit inside the scope of this series.  It may be something we want to
  revisit later though.

* The --init-active flag now ensure that all submodules which are configured to
  be 'active' (either via 'submodule.active' or 'submodule..active') go
  through the initialization phase and have their relevent info copied over to
  the config.


Brandon Williams (10):
  submodule--helper: add is_active command
  submodule status: use submodule--helper is-active
  submodule sync: use submodule--helper is-active
  submodule--helper clone: check for configured submodules using helper
  submodule: decouple url and submodule existence
  submodule update: add `--init-active` switch
  clone: add --submodule-spec= switch
  completion: clone can initialize specific submodules
  submodule--helper init: set submodule..active
  submodule add: respect submodule.active and submodule..active

 Documentation/config.txt   |  15 +++-
 Documentation/git-clone.txt|  23 --
 Documentation/git-submodule.txt|   9 +++
 builtin/clone.c|  36 +-
 builtin/submodule--helper.c|  68 ++
 contrib/completion/git-completion.bash |   1 +
 git-submodule.sh   |  43 +--
 submodule.c|  36 --
 t/t7400-submodule-basic.sh | 128 +
 t/t7413-submodule-is-active.sh | 107 +++
 10 files changed, 431 insertions(+), 35 deletions(-)
 create mode 100755 t/t7413-submodule-is-active.sh

-- 
2.12.0.246.ga2ecc84866-goog

Re: [GSoC][PATCH v2 1/2] path.c: add xdg_cache_home

[Junio C Hamano]

Devin Lehmacher  writes:

> We already have xdg_config_home to format paths relative to
> XDG_CONFIG_HOME. Let's provide a similar function xdg_cache_home to do
> the same for paths relative to XDG_CACHE_HOME.

Nicely explained.

> +/**
> + * Return a newly allocated string with the evaluation of
> + * "$XDG_CACHE_HOME/git/$filename" if $XDG_CONFIG_HOME is non-empty, 
> otherwise
> + * "$HOME/.config/git/$filename". Return NULL upon error.

s|CONFIG|CACHE| and s|.config|.cache| are needed, methinks.

Will fix while queuing, so no need to resend only to fix this typo.

Re: [PATCH] difftool: handle changing symlinks in dir-diff mode

[Johannes Schindelin]

Hi David,

Thank you very much for picking this up!

On Mon, 13 Mar 2017, David Aguilar wrote:

> Detect the null object ID for symlinks in dir-diff so that difftool can
> prepare temporary files that matches how git handles symlinks.

Maybe a description is needed how the OID can be null in that case. I have
to admit that I failed to wrap my head around this so far.

> Previously, a null object ID would crash difftool.  We now detect null
> object IDs and write the symlink's content into the temporary symlink
> stand-in file.
> 
> Original-patch-by: Johannes Schindelin 
> Signed-off-by: David Aguilar 
> ---
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index d13350ce83..6c20e20b45 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -254,6 +254,31 @@ static int ensure_leading_directories(char *path)
>   }
>  }
>  
> +static int create_symlink_file(struct cache_entry* ce, struct checkout* 
> state)
> +{
> + /*
> +  * Dereference a worktree symlink and writes its contents
> +  * into the checkout state's path.
> +  */
> + struct strbuf path = STRBUF_INIT;
> + struct strbuf link = STRBUF_INIT;
> +
> + int ok = 0;

It would appear that the convention in Git's C code is for functions to
return 0 upon success and -1 upon error, and to use `int ret` for that
purpose.

> + if (strbuf_readlink(, ce->name, ce_namelen(ce)) == 0) {

Looking at the calling site, I would have expected the code to read the
contents as per ce->hash... After all, we are calling this in the
!use_wt_file() case.

But that is exactly that null hash, isn't it?

> @@ -414,7 +439,12 @@ static int run_dir_diff(const char *extcmd, int 
> symlinks, const char *prefix,

The lines before this hunk read:

>   if (!use_wt_file(workdir, dst_path, )) {
>   ce->ce_mode = rmode;

... and then follow these lines:

>   oidcpy(>oid, );
>   strcpy(ce->name, dst_path);
>   ce->ce_namelen = dst_path_len;
> - if (checkout_entry(ce, , NULL))
> +
> + if (S_ISLNK(rmode) && is_null_oid()) {
> + if (!create_symlink_file(ce, ))
> + return error("unable to create 
> symlink file %s",
> +  dst_path);
> + } else if (checkout_entry(ce, , NULL))
>   return error("could not write '%s'",
>dst_path);
>   } else if (!is_null_oid()) {

Given that we explicitly ask use_wt_file() whether we can use the
worktree's file, and we get the answer "no" before we enter the modified
code block, I would really expect us *not* to want to read the link from
disk at all.

Further, reading the code of use_wt_file(), there seems to be another case
where roid is left alone: when the file could not be lstat()ed. So I
wonder whether the create_symlink_file() is the correct solution, or
whether we could somehow fill roid correctly instead, and keep the
checkout_entry() call?

Ciao,
Dscho

Re: bug?: git reset --mixed ignores deinitialized submodules

[David Turner]

On Mon, 2017-03-13 at 14:19 -0700, Stefan Beller wrote:
> > > The change is not really lost, as you can get it via
> > > 
> > > git checkout HEAD@{1}
> > > git submodule update --init
> > 
> > Sure, the commit isn't lost entirely.  But a mixed reset is often
> > used
> > to mean "go back to before I committed", and here, that's not
> > precisely
> > what's happening.
> 
> Well, you ran the deinit after the commit, so I don't think
> we expect to undo everything up to "just before the commit".

Sure, but other workdir changes after the commit would have been blown
away; so why not this one?

> > > * lack of --recurse-submodules in git-reset? (and that not
> > >   being default, see prior point)
> > 
> > Or possibly this.
> 
> Well even in this case a reset recursing into submodules doesn't
> change the (de-)init status of a submodule. All it would alter is the
> submodule HEAD pointing to, IMHO.

That's OK with me.  It's weird, but at least it's explicable. 

> > For me, the bug (if any) is the bad user experience of doing a
> > mixed
> > reset and expecting to be able to commit (possibly with some git-
> > add
> > operations) from there and get back something like the commit to
> > which
> > the user had git-reset.
> 
> Technically this is also doable,
> 
> git reset --mixed HEAD^ # as in the original email
> git update-index --add --cacheinfo \
> 16,$(git -C .git/modules/sub1 rev-parse HEAD),sub1
> git add another_file
> git commit -m "recreate the commit"

Yeah, unless the user had done various other operations that messed
with .git/modules/sub1 (e.g. if they had first checked out another
branch with --recurse-submodules).

Also, this updates the index, which a mixed reset isn't supposed to
touch. 

> > That's why I have the question mark there -- it's not clear that
> > this
> > is a reasonable expectation.
> 
> Fuzzing around with gitlinks that are non-populated submodules is
> a messy business.

Agreed.

> So another way would be to populate the submodule manually
> 
> GIT_DIR=.git/modules/sub1 \
> GIT_WORKTREE=sub1 \
> git checkout -f # implies last HEAD
> 
> and then continue in the superproject.

(see above for why this is not a general solution)

> I am making up excuses for poor UX here, though.
> I do agree that the expectations may vary wildly because of poor UX
> in submodules.

I agree that it's not quite clear what to expect.  I can just say that
I was quite surprised when my colleague demoed this one for me.

Re: [PATCH] difftool: handle changing symlinks in dir-diff mode

[Junio C Hamano]

Johannes Schindelin  writes:

>> > +  if (strbuf_readlink(, ce->name, ce_namelen(ce)) == 0) {
>> > +  strbuf_add(, state->base_dir, state->base_dir_len);
>> > +  strbuf_add(, ce->name, ce_namelen(ce));
>> > +
>> > +  write_file_buf(path.buf, link.buf, link.len);
>> 
>> This does "write content into symlink stand-in file", but why?
>
> From the commit message:
>
>   > Detect the null object ID for symlinks in dir-diff so that
>   > difftool can prepare temporary files that matches how git
>   > handles symlinks.

Yes I read what the proposed log message said, and noticed that
symbolic link is _always_ written as a regular file.

I was questioning why.  I know Git falls back to such behaviour when
the filesystem does not support symbolic links.  "Why do so
unconditionally, even when the filesystem does?" was the question.

> The obvious connection: when core.symlinks = false, Git already falls back
> to writing plain files with the link target as contents. This function
> does the same, for the same motivation: it is the best we can do in this
> case.

And that "obvious connection" does not answer the question.

Re: fatal: Could not get current working directory: Permission denied | affected 2.10,2.11,2.12, but not 1.9.5 |

[René Scharfe]

Am 13.03.2017 um 14:23 schrieb Zenobiusz Kunegunda:

Bisecting: 0 revisions left to test after this (roughly 0 steps)
[a26bc613a64ac2c7ee69a50675e61b004a26382d] pretty.c: make 
git_pretty_formats_config return -1 on git_config_string failure

This is what I found with git bisect


Strange, I don't think git_pretty_formats_config() is even called by git 
status.


René

Re: Possible git blame bug?

[Junio C Hamano]

On Mon, Mar 13, 2017 at 1:38 PM, Junio C Hamano  wrote:
> Domagoj Stolfa  writes:
>
>> The question is whether this is a bug or not, as --since= might not be 
>> a
>> valid filter.
>
> I do not think blame ever was designed to work with --since, so that
> is indeed the case.

Actually, I do see that we had a cut-off based on rev->max_age since we
introduced it at cee7f245 ("git-pickaxe: blame rewritten.", 2006-10-19).

I do not know offhand if --since=2000 _means_ --since=2000-01-01 or something
completely different, though.

Re: [PATCH] difftool: handle changing symlinks in dir-diff mode

[Johannes Schindelin]

Hi Junio,

On Mon, 13 Mar 2017, Junio C Hamano wrote:

> David Aguilar  writes:
> 
> > +static int create_symlink_file(struct cache_entry* ce, struct checkout* 
> > state)
> 
> Asterisk sticks to variable, not type.

If only we had tools to format the code so that authors as well as
reviewers could concentrate on essential parts of the patches :-)

> > +* into the checkout state's path.
> > +*/
> > +   struct strbuf path = STRBUF_INIT;
> > +   struct strbuf link = STRBUF_INIT;
> > +
> > +   int ok = 0;
> > +
> > +   if (strbuf_readlink(, ce->name, ce_namelen(ce)) == 0) {
> > +   strbuf_add(, state->base_dir, state->base_dir_len);
> > +   strbuf_add(, ce->name, ce_namelen(ce));
> > +
> > +   write_file_buf(path.buf, link.buf, link.len);
> 
> This does "write content into symlink stand-in file", but why?

>From the commit message:

> Detect the null object ID for symlinks in dir-diff so that
> difftool can prepare temporary files that matches how git
> handles symlinks.

The obvious connection: when core.symlinks = false, Git already falls back
to writing plain files with the link target as contents. This function
does the same, for the same motivation: it is the best we can do in this
case.

> Also, I am not sure if strbuf_readlink() can unconditionally used
> here.  On a filesystem without symbolic link, the working tree
> entity that corresponds to the ce that represents a symlink is a
> stand-in regular file, so shouldn't we be opening it as a regular
> file and reading its contents in that case?

I think you are right, we cannot simply call strbuf_readlink(), we would
have to check the core_symlinks variable to maybe read the file contents
instead.

But then, it may not be appropriate to read the worktree to begin with...
see my reply to the patch that I will send out in a couple of minutes.

Ciao,
Johannes

Re: bug?: git reset --mixed ignores deinitialized submodules

[Stefan Beller]

>> The change is not really lost, as you can get it via
>>
>> git checkout HEAD@{1}
>> git submodule update --init
>
> Sure, the commit isn't lost entirely.  But a mixed reset is often used
> to mean "go back to before I committed", and here, that's not precisely
> what's happening.

Well, you ran the deinit after the commit, so I don't think
we expect to undo everything up to "just before the commit".

> In other words, it's not confusing to the user.

ok, great :)

>
>> This works most of the time, but it is unreliable as the
>> submodule may have had some gc inbetween which
>> threw away important objects.
>
> Sure; that's a separate issue.
>
>> Steping back a bit, rereading the subject line,
>> what do you think is the bug here?
>>
>> * git-status not reporting about uninitialized submodules?
>
> Here, I think git-status is correctly reporting the state of the repo.
>
>> * git reset --mixed not touching the submodule worktree
>
> Yes, possibly.
>
>> * lack of --recurse-submodules in git-reset? (and that not
>>   being default, see prior point)
>
> Or possibly this.

Well even in this case a reset recursing into submodules doesn't change
the (de-)init status of a submodule. All it would alter is the
submodule HEAD pointing to, IMHO.

>
>> * submodules being in detached HEAD all the time?
>
> In this case, the submodule is not initialized, so it is not in any
> state at all.

Oh right.

>
>
> For me, the bug (if any) is the bad user experience of doing a mixed
> reset and expecting to be able to commit (possibly with some git-add
> operations) from there and get back something like the commit to which
> the user had git-reset.

Technically this is also doable,

git reset --mixed HEAD^ # as in the original email
git update-index --add --cacheinfo \
16,$(git -C .git/modules/sub1 rev-parse HEAD),sub1
git add another_file
git commit -m "recreate the commit"

>
> That's why I have the question mark there -- it's not clear that this
> is a reasonable expectation.

Fuzzing around with gitlinks that are non-populated submodules is
a messy business.

So another way would be to populate the submodule manually

GIT_DIR=.git/modules/sub1 \
GIT_WORKTREE=sub1 \
git checkout -f # implies last HEAD

and then continue in the superproject.

I am making up excuses for poor UX here, though.
I do agree that the expectations may vary wildly because of poor UX
in submodules.

Stefan

Re: [PATCH] Put sha1dc on a diet

[Marc Stevens]

I think I now understand.
The Makefile indeed seems to fail to correctly rebuild when a header has 
changed.

As the performance branch has removed the 'int bigendian' from SHA1_CTX in 
lib/sha1.h,
the perf-branch and master-branch are binary incompatible.
So the command-line utility does not get fully recompiled 
and instead of the value of found_collision will read a different value of 
SHA1_CTX.

So be careful to always do a 'make clean' for now.

-- Marc

- Original Message -
From: "Jeff King" 
To: "Marc Stevens" 
Cc: "Linus Torvalds" , "Dan Shumow" 
, "Junio C Hamano" , "Git Mailing 
List" 
Sent: Monday, March 13, 2017 10:00:23 PM
Subject: Re: [PATCH] Put sha1dc on a diet

On Mon, Mar 13, 2017 at 09:47:54PM +0100, Marc Stevens wrote:

> Linus:
> I would be surprised, the dependencies should be automatically determined.
> 
> BTW Did you make local changes to this perf branch?

I can reproduce it with:

  cd sha1collisiondetection
  git clean -dqfx ;# make sure we are starting from scratch

  git checkout 9c8e73cadb35776d3310e3f8ceda7183fa75a39f
  make
  bin/sha1dcsum $file

  git checkout 55d1db0980501e582f6cd103a04f493995b1df78
  make
  bin/sha1dcsum $file

The final call to sha1dcsum will report a collision, even though the
first one did not.

It also reproduces with the original snippet I posted. I didn't notice
because I was just collecting the timings then (and I originally noticed
the problem on the versions I had pulled into Git, where it works as
expected; but then I am just pulling in the two source files, without
all of the libtool magic).

-Peff

Re: [PATCH] difftool: handle changing symlinks in dir-diff mode

[Johannes Schindelin]

Hi Junio,

On Mon, 13 Mar 2017, Junio C Hamano wrote:

> David Aguilar  writes:
> 
> > Detect the null object ID for symlinks in dir-diff so that difftool
> > can prepare temporary files that matches how git handles symlinks.
> >
> > Previously, a null object ID would crash difftool.  We now detect null
> > object IDs and write the symlink's content into the temporary symlink
> > stand-in file.
> >
> > Original-patch-by: Johannes Schindelin 
> > Signed-off-by: David Aguilar 
> > ---
> 
> I would have appreciated (and I suspect other reviewers would, too)
> a bit of back-story wrt how "Original-patch-by" resulted in this
> patch after the three-dashes line.  It is perfectly fine if you two
> coordinated privately; I mostly wanted to hear something like "Dscho
> has been working on this and I asked him if it is OK to take over
> his WIP to produce a quick-fix we can ship on the maint branch, here
> is the result of that collaboration."  IOW, the person who is named
> as the original author is fine to be named like so (I care only
> because I do not think we saw the "original" here on the list).

The story is more like: Johannes started working on this but got pulled
away by other tasks and sent out a link to the initial patch, along with a
note that he hopes to be able to get back to working on that patch before
long (but of course he did not get the chance):

http://public-inbox.org/git/alpine.DEB.2.20.1703072332370.3767@virtualbox/

There was no private exchange. I am happy that David picked up the
project.

Ciao,
Johannes

Re: [PATCH] Put sha1dc on a diet

[Jeff King]

On Mon, Mar 13, 2017 at 09:47:54PM +0100, Marc Stevens wrote:

> Linus:
> I would be surprised, the dependencies should be automatically determined.
> 
> BTW Did you make local changes to this perf branch?

I can reproduce it with:

  cd sha1collisiondetection
  git clean -dqfx ;# make sure we are starting from scratch

  git checkout 9c8e73cadb35776d3310e3f8ceda7183fa75a39f
  make
  bin/sha1dcsum $file

  git checkout 55d1db0980501e582f6cd103a04f493995b1df78
  make
  bin/sha1dcsum $file

The final call to sha1dcsum will report a collision, even though the
first one did not.

It also reproduces with the original snippet I posted. I didn't notice
because I was just collecting the timings then (and I originally noticed
the problem on the versions I had pulled into Git, where it works as
expected; but then I am just pulling in the two source files, without
all of the libtool magic).

-Peff

Re: [PATCH 12/17] update submodules: add submodule_move_head

[Stefan Beller]

On Sat, Mar 11, 2017 at 11:09 PM, Junio C Hamano  wrote:
> Brandon Williams  writes:
>
>>> diff --git a/submodule.c b/submodule.c
>>> index 0b2596e88a..bc5fecf8c5 100644
>>> --- a/submodule.c
>>> +++ b/submodule.c
>>> @@ -1239,6 +1239,141 @@ int bad_to_remove_submodule(const char *path, 
>>> unsigned flags)
>>>  return ret;
>>>  }
>>>
>>> +static int submodule_has_dirty_index(const struct submodule *sub)
>>> +{
>>> +struct child_process cp = CHILD_PROCESS_INIT;
>>> +
>>> +prepare_submodule_repo_env_no_git_dir(_array);
>>> +
>>> +cp.git_cmd = 1;
>>> +argv_array_pushl(, "diff-index", "--quiet", \
>>> +"--cached", "HEAD", NULL);
>>
>> The formatting of this line is a little odd.  Also you can drop the
>> backslash.
>
> Heh.  I think I saw and pointed out the same during the review of
> the previous round X-<.  It is a bit disappointing.

Yes. I remember having a local mixup of branches, such that I
fear I may have missed other feedback of the previous round as
well then. I do remember removing a backslash, so maybe I
got confused with another backslash as well.

I'll reroll with this fixed.

>
>>> +static void submodule_reset_index(const char *path)
>>> +{
>>> +struct child_process cp = CHILD_PROCESS_INIT;
>>> +prepare_submodule_repo_env_no_git_dir(_array);
>>> +
>>> +cp.git_cmd = 1;
>>> +cp.no_stdin = 1;
>>> +cp.dir = path;
>>> +
>>> +argv_array_pushf(, "--super-prefix=%s/", path);
>>> +argv_array_pushl(, "read-tree", "-u", "--reset", NULL);
>>> +
>>> +argv_array_push(, EMPTY_TREE_SHA1_HEX);
>
> Somewhat related; will this use of --super-prefix be affected when
> we split it into two for "adjust pathspec" prefix and "adjust
> output" prefix?

Let's see how the superproject prefixing evolves and what Brandon
sends out. When the superprefix changes its meaning I'll adjust here.

Re: [PATCH] Put sha1dc on a diet

[Marc Stevens]

Linus:
I would be surprised, the dependencies should be automatically determined.

BTW Did you make local changes to this perf branch?
Specifically did you disable the safe hash mode that is on by default?
Because if you did not, it might also be something else as all three hashes 
below are the same.

-- Marc

- Original Message -
From: "Linus Torvalds" 
To: "Marc Stevens" 
Cc: "Jeff King" , "Dan Shumow" , "Junio C 
Hamano" , "Git Mailing List" 
Sent: Monday, March 13, 2017 9:20:02 PM
Subject: Re: [PATCH] Put sha1dc on a diet

On Mon, Mar 13, 2017 at 1:12 PM, Marc Stevens  wrote:
> Indeed, I've committed a fix, and a small bug fix for the new code just now.

Unrelated side note: there may be some missing dependencies in the
build infrastructure or something, because when I tried Jeff's script
that did that "test the merge and the two parents", and used the
pack-file of the kernel for testing, I got:

  5611971c610143e6d38bbdca463f4c9f79a056a0
/home/torvalds/v2.6/linux/.git/objects/pack/pack-153bb8cd11846cf9a27ef7b1069aa9cb9f5b724f.pack

  real 0m2.432s
  user 0m2.348s
  sys 0m0.084s

  5611971c610143e6d38bbdca463f4c9f79a056a0
/home/torvalds/v2.6/linux/.git/objects/pack/pack-153bb8cd11846cf9a27ef7b1069aa9cb9f5b724f.pack

  real 0m3.747s
  user 0m3.672s
  sys 0m0.076s

  5611971c610143e6d38bbdca463f4c9f79a056a0 *coll*
/home/torvalds/v2.6/linux/.git/objects/pack/pack-153bb8cd11846cf9a27ef7b1069aa9cb9f5b724f.pack

  real 0m5.061s
  user 0m4.984s
  sys 0m0.077s

never mind the performace, notice the *coll* in that last case.

But doing a "git clean -dqfx; make -j8" and re-testing the same tree,
the issue is gone.

I suspect some dependency on a header file is broken, causing some
object file to not be properly re-built, which in turn then
incorrectly causes the 'ctx2.found_collision' test to test the wrong
bit or something.

 Linus

Re: Possible git blame bug?

[Domagoj Stolfa]

Hello,
 
> > The question is whether this is a bug or not, as --since= might not 
> > be a
> > valid filter.
> 
> I do not think blame ever was designed to work with --since, so that
> is indeed the case.  
> 
> Making it work with --since= might be a worthy addition.
> Patches welcome.
 
Thanks for the information. I wasn't aware that it wasn't designed with that in
mind, though it would indicate to me that --since seems to work with git-blame
according to [1], but expects a date, as I have usually been using it. It also
seems to produce correct results on FreeBSD 12.0-CURRENT with git 2.11.1, except
when given a filter such as --since=, in which case perhaps nothing should
be displayed?
 
Could you please clarify which bits wouldn't work with --since in git-blame?
 
[1] https://www.git-scm.com/docs/git-blame
 
-- 
Best regards,
Domagoj Stolfa


signature.asc
Description: PGP signature

[GSoC][PATCH v2 1/2] path.c: add xdg_cache_home

[Devin Lehmacher]

We already have xdg_config_home to format paths relative to
XDG_CONFIG_HOME. Let's provide a similar function xdg_cache_home to do
the same for paths relative to XDG_CACHE_HOME.

Signed-off-by: Devin Lehmacher 
---
 cache.h |  7 +++
 path.c  | 15 +++
 2 files changed, 22 insertions(+)

diff --git a/cache.h b/cache.h
index 8c0e64420..378a636e1 100644
--- a/cache.h
+++ b/cache.h
@@ -1169,6 +1169,13 @@ extern int is_ntfs_dotgit(const char *name);
  */
 extern char *xdg_config_home(const char *filename);
 
+/**
+ * Return a newly allocated string with the evaluation of
+ * "$XDG_CACHE_HOME/git/$filename" if $XDG_CONFIG_HOME is non-empty, otherwise
+ * "$HOME/.config/git/$filename". Return NULL upon error.
+ */
+extern char *xdg_cache_home(const char *filename);
+
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
 #define LOOKUP_UNKNOWN_OBJECT 2
diff --git a/path.c b/path.c
index efcedafba..22248436b 100644
--- a/path.c
+++ b/path.c
@@ -1272,6 +1272,21 @@ char *xdg_config_home(const char *filename)
return NULL;
 }
 
+char *xdg_cache_home(const char *filename)
+{
+   const char *home, *cache_home;
+
+   assert(filename);
+   cache_home = getenv("XDG_CACHE_HOME");
+   if (cache_home && *cache_home)
+   return mkpathdup("%s/git/%s", cache_home, filename);
+
+   home = getenv("HOME");
+   if (home)
+   return mkpathdup("%s/.cache/git/%s", home, filename);
+   return NULL;
+}
+
 GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD")
 GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD")
 GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG")
-- 
2.11.0

[GSoC][PATCH v2 2/2] credential-cache: use XDG_CACHE_HOME for socket

[Devin Lehmacher]

git-credential-cache will now use a socket following the XDG base path
specification by default. This increases consistency with other
applications and helps keep clutter out of users' home directories.

We still check the old socket location, ~/.git-credential-cache/socket
first in case the user already has a socket at that location. This
ensures that a socket previously created will be used over forcibly
switching to the new socket location.

If there is not a socket at that location we create a new one at
$XDG_CACHE_HOME/git/credential/socket. This complies with the XDG
standard and good for the reasons previously mentioned. We use the
subdirectory credential/ in case we later want to store other files
under $XDG_CACHE_HOME/git/ and to make the purpose of the socket clear.

I also change to documentation to reflect the new default socket
location.
---
 Documentation/git-credential-cache.txt |  3 ++-
 credential-cache.c | 16 +++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-credential-cache.txt 
b/Documentation/git-credential-cache.txt
index 96208f822..4b9db3856 100644
--- a/Documentation/git-credential-cache.txt
+++ b/Documentation/git-credential-cache.txt
@@ -34,7 +34,8 @@ OPTIONS
 
Use `` to contact a running cache daemon (or start a new
cache daemon if one is not started). Defaults to
-   `~/.git-credential-cache/socket`. If your home directory is on a
+   `~/.git-credential-cache/socket` if it exists and otherwise
+`$XDG_CACHE_HOME/git/credential/socket`. If your home directory is on a
network-mounted filesystem, you may need to change this to a
local filesystem. You must specify an absolute path.
 
diff --git a/credential-cache.c b/credential-cache.c
index cc8a6ee19..db1343b46 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -83,6 +83,20 @@ static void do_cache(const char *socket, const char *action, 
int timeout,
strbuf_release();
 }
 
+static char *get_socket_path(void) {
+   char *home_socket;
+
+   home_socket = expand_user_path("~/.git-credential-cache/socket");
+   if (home_socket) {
+   if (file_exists(home_socket))
+   return home_socket;
+   else
+   free(home_socket);
+   }
+
+   return xdg_cache_home("credential/socket");
+}
+
 int cmd_main(int argc, const char **argv)
 {
char *socket_path = NULL;
@@ -106,7 +120,7 @@ int cmd_main(int argc, const char **argv)
op = argv[0];
 
if (!socket_path)
-   socket_path = 
expand_user_path("~/.git-credential-cache/socket");
+   socket_path = get_socket_path();
if (!socket_path)
die("unable to find a suitable socket path; use --socket");
 
-- 
2.11.0

[GSoC][PATCH v2 2/2] credential-cache: use XDG_CACHE_HOME for socket

[Devin Lehmacher]

git-credential-cache will now use a socket following the XDG base path
specification by default. This increases consistency with other
applications and helps keep clutter out of users' home directories.

We still check the old socket location, ~/.git-credential-cache/socket
first in case the user already has a socket at that location. This
ensures that a socket previously created will be used over forcibly
switching to the new socket location.

If there is not a socket at that location we create a new one at
$XDG_CACHE_HOME/git/credential/socket. This complies with the XDG
standard and good for the reasons previously mentioned. We use the
subdirectory credential/ in case we later want to store other files
under $XDG_CACHE_HOME/git/ and to make the purpose of the socket clear.

I also change to documentation to reflect the new default socket
location.
---
 Documentation/git-credential-cache.txt |  3 ++-
 credential-cache.c | 16 +++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-credential-cache.txt 
b/Documentation/git-credential-cache.txt
index 96208f822..4b9db3856 100644
--- a/Documentation/git-credential-cache.txt
+++ b/Documentation/git-credential-cache.txt
@@ -34,7 +34,8 @@ OPTIONS
 
Use `` to contact a running cache daemon (or start a new
cache daemon if one is not started). Defaults to
-   `~/.git-credential-cache/socket`. If your home directory is on a
+   `~/.git-credential-cache/socket` if it exists and otherwise
+`$XDG_CACHE_HOME/git/credential/socket`. If your home directory is on a
network-mounted filesystem, you may need to change this to a
local filesystem. You must specify an absolute path.
 
diff --git a/credential-cache.c b/credential-cache.c
index cc8a6ee19..db1343b46 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -83,6 +83,20 @@ static void do_cache(const char *socket, const char *action, 
int timeout,
strbuf_release();
 }
 
+static char *get_socket_path(void) {
+   char *home_socket;
+
+   home_socket = expand_user_path("~/.git-credential-cache/socket");
+   if (home_socket) {
+   if (file_exists(home_socket))
+   return home_socket;
+   else
+   free(home_socket);
+   }
+
+   return xdg_cache_home("credential/socket");
+}
+
 int cmd_main(int argc, const char **argv)
 {
char *socket_path = NULL;
@@ -106,7 +120,7 @@ int cmd_main(int argc, const char **argv)
op = argv[0];
 
if (!socket_path)
-   socket_path = 
expand_user_path("~/.git-credential-cache/socket");
+   socket_path = get_socket_path();
if (!socket_path)
die("unable to find a suitable socket path; use --socket");
 
-- 
2.11.0

Re: Possible git blame bug?

[Junio C Hamano]

Domagoj Stolfa  writes:

> The question is whether this is a bug or not, as --since= might not be a
> valid filter.

I do not think blame ever was designed to work with --since, so that
is indeed the case.  

Making it work with --since= might be a worthy addition.
Patches welcome.

Re: [PATCH v6 03/12] Prepare setup_discovered_git_directory() the root directory

[Junio C Hamano]

Johannes Schindelin  writes:

> Subject: Re: [PATCH v6 03/12] Prepare setup_discovered_git_directory() the 
> root directory

I do not think you've changed this title throughout the rerolls, but
I cannot quite parse it.  Would something like this

setup.c: only append '/' when not at root in setup_discovered_git_dir()

give enough information to the readers while making it readable?

> Currently, the offset parameter (indicating what part of the cwd
> parameter corresponds to the current directory after discovering the
> .git/ directory) is set to 0 when we are running in the root directory.
>
> However, in the next patches we will avoid changing the current working
> directory while searching for the .git/ directory, meaning that the
> offset corresponding to the root directory will have to be 1 to reflect
> that this directory is characterized by the path "/" (and not "").

OK.  C:\ would be 3 not 1 but the same logic to compare with
offset_1st_component() covers the case, which is good.

> So let's make sure that setup_discovered_git_directory() only tries to
> append the trailing slash to non-root directories.
>
> Note: the setup_bare_git_directory() does not need a corresponding
> change, as it does not want to return a prefix.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  setup.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 2ac891d4b9a..23114cb7aa3 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -721,8 +721,10 @@ static const char *setup_discovered_git_dir(const char 
> *gitdir,
>   if (offset == cwd->len)
>   return NULL;
>  
> - /* Make "offset" point to past the '/', and add a '/' at the end */
> - offset++;
> + /* Make "offset" point past the '/' (already the case for root dirs) */
> + if (offset != offset_1st_component(cwd->buf))
> + offset++;
> + /* Add a '/' at the end */
>   strbuf_addch(cwd, '/');
>   return cwd->buf + offset;
>  }

Re: [PATCH v5 10/11] setup_git_directory_gently_1(): avoid die()ing

[Junio C Hamano]

Junio C Hamano  writes:

> The former case may split into two.  "Yes I agree that is bad and it
> is the same badness as the version without this change", in which
> case we may want to leave a "NEEDSWORK" comment in-code so that
> people can notice when browsing the code (but fixing the badness
> would be outside the scope of the series), and "yes that is bad and
> we shouldn't introduce that badness", in which case we do want to
> fix it in the patch.

We however need to be a bit careful here, though.

When a patch series is refactoring an existing function to be used
in different codepaths, an existing issue inherited from the
original code (e.g. perhaps an existing error checking that is
looser than ideal) may have been OK in the original context (e.g.
perhaps it died and refused to run until the user corrected the
repository), and it still is OK in the codepath that uses the
refactored building blocks to replace the original code, but it may
not be acceptable to leave the issue in the original code when a new
caller calls the refactored building block (e.g. perhaps the
refactoring made it possible for a caller to ask the function not to
die and instead act on different kinds of errors in different ways).

So "being bug-to-bug compatible with existing code" is not always a
good thing---we need to see case by case if a problem identified in
the review as inherited from the original needs to be addressed in
the series.

It would be better to address issues we identify even if it is an
old one.  After all, any change has potential to introduce new bugs,
and striving to leave known bug inherited from the old code around
would guarantee that the resulting code will be buggier than the
original ;-) 

But in order to keep bisectability, such "further fixups" should be
done as a follow-up to the series, not as part of it.

Re: [PATCH] Put sha1dc on a diet

[Linus Torvalds]

On Mon, Mar 13, 2017 at 1:12 PM, Marc Stevens  wrote:
> Indeed, I've committed a fix, and a small bug fix for the new code just now.

Unrelated side note: there may be some missing dependencies in the
build infrastructure or something, because when I tried Jeff's script
that did that "test the merge and the two parents", and used the
pack-file of the kernel for testing, I got:

  5611971c610143e6d38bbdca463f4c9f79a056a0
/home/torvalds/v2.6/linux/.git/objects/pack/pack-153bb8cd11846cf9a27ef7b1069aa9cb9f5b724f.pack

  real 0m2.432s
  user 0m2.348s
  sys 0m0.084s

  5611971c610143e6d38bbdca463f4c9f79a056a0
/home/torvalds/v2.6/linux/.git/objects/pack/pack-153bb8cd11846cf9a27ef7b1069aa9cb9f5b724f.pack

  real 0m3.747s
  user 0m3.672s
  sys 0m0.076s

  5611971c610143e6d38bbdca463f4c9f79a056a0 *coll*
/home/torvalds/v2.6/linux/.git/objects/pack/pack-153bb8cd11846cf9a27ef7b1069aa9cb9f5b724f.pack

  real 0m5.061s
  user 0m4.984s
  sys 0m0.077s

never mind the performace, notice the *coll* in that last case.

But doing a "git clean -dqfx; make -j8" and re-testing the same tree,
the issue is gone.

I suspect some dependency on a header file is broken, causing some
object file to not be properly re-built, which in turn then
incorrectly causes the 'ctx2.found_collision' test to test the wrong
bit or something.

 Linus

Re: [PATCH] Put sha1dc on a diet

[Marc Stevens]

Indeed, I've committed a fix, and a small bug fix for the new code just now.

The merge incorrectly removed some control logic,
which caused more unnecessary checks to happen.
I already marked this in the PR, but committed a fix only today.

BTW as noted in the Readme, the theoretic false positive probability is
<<2^-90, almost non-existent.

Best regards,
Marc Stevens


On 3/13/2017 8:48 PM, Jeff King wrote:
> On Mon, Mar 13, 2017 at 07:42:17PM +, Dan Shumow wrote:
>
>> Marc just made a commit this morning fixing problems with the merge.
>> Please give the latest in feature/performance a try, as that seems to
>> eliminate the problem.
> Yeah, b17728507 makes the problem go away for me. Thanks.
>
> FWIW, I have all sha1s on github.com running through this right now
> (actually, the ad744c8b7 version), and logging any false-positives on
> the collision detection. Nothing so far, after a few hours.
>
> -Peff

[PATCH v6 12/12] setup.c: mention unresolved problems

[Johannes Schindelin]

During the review of the `early-config` patch series, two issues have
been identified that have been with us forever.

The idea of that patch series was to fix the hard-coded (and sometimes
wrong) .git/config path when looking for the pager configurations. To
that end, the patches refactor the helper functions behind the
functionality of setup_git_directory(), to make it reusable without
changing any global state. Not to change said functionality.

So let's just mark the identified problems for later so that we do not
forget them.

Signed-off-by: Johannes Schindelin 
---
 setup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/setup.c b/setup.c
index f31abf8a990..64f922a9378 100644
--- a/setup.c
+++ b/setup.c
@@ -531,6 +531,7 @@ const char *read_gitfile_gently(const char *path, int 
*return_error_code)
ssize_t len;
 
if (stat(path, )) {
+   /* NEEDSWORK: discern between ENOENT vs other errors */
error_code = READ_GITFILE_ERR_STAT_FAILED;
goto cleanup_return;
}
@@ -902,6 +903,7 @@ static enum discovery_result 
setup_git_directory_gently_1(struct strbuf *dir,
if (!gitdirenv) {
if (die_on_error ||
error_code == READ_GITFILE_ERR_NOT_A_FILE) {
+   /* NEEDSWORK: fail if .git is not file nor dir 
*/
if (is_git_directory(dir->buf))
gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v6 10/12] setup_git_directory_gently_1(): avoid die()ing

[Johannes Schindelin]

This function now has a new caller in addition to setup_git_directory():
the newly introduced discover_git_directory(). That function wants to
discover the current .git/ directory, and in case of a corrupted one
simply pretend that there is none to be found.

Example: if a stale .git file exists in the parent directory, and the
user calls `git -p init`, we want Git to simply *not* read any
repository config for the pager (instead of aborting with a message that
the .git file is corrupt).

Let's actually pretend that there was no GIT_DIR to be found in that case
when being called from discover_git_directory(), but keep the previous
behavior (i.e. to die()) for the setup_git_directory() case.

Signed-off-by: Johannes Schindelin 
---
 setup.c | 25 +
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/setup.c b/setup.c
index 411e8342972..f31abf8a990 100644
--- a/setup.c
+++ b/setup.c
@@ -825,7 +825,8 @@ enum discovery_result {
GIT_DIR_BARE,
/* these are errors */
GIT_DIR_HIT_CEILING = -1,
-   GIT_DIR_HIT_MOUNT_POINT = -2
+   GIT_DIR_HIT_MOUNT_POINT = -2,
+   GIT_DIR_INVALID_GITFILE = -3
 };
 
 /*
@@ -842,7 +843,8 @@ enum discovery_result {
  * is relative to `dir` (i.e. *not* necessarily the cwd).
  */
 static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
- struct strbuf *gitdir)
+ struct strbuf *gitdir,
+ int die_on_error)
 {
const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
@@ -890,14 +892,21 @@ static enum discovery_result 
setup_git_directory_gently_1(struct strbuf *dir,
if (one_filesystem)
current_device = get_device_or_die(dir->buf, NULL, 0);
for (;;) {
-   int offset = dir->len;
+   int offset = dir->len, error_code = 0;
 
if (offset > min_offset)
strbuf_addch(dir, '/');
strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
-   gitdirenv = read_gitfile(dir->buf);
-   if (!gitdirenv && is_git_directory(dir->buf))
-   gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
+   gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
+   NULL : _code);
+   if (!gitdirenv) {
+   if (die_on_error ||
+   error_code == READ_GITFILE_ERR_NOT_A_FILE) {
+   if (is_git_directory(dir->buf))
+   gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
+   } else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
+   return GIT_DIR_INVALID_GITFILE;
+   }
strbuf_setlen(dir, offset);
if (gitdirenv) {
strbuf_addstr(gitdir, gitdirenv);
@@ -934,7 +943,7 @@ const char *discover_git_directory(struct strbuf *gitdir)
return NULL;
 
cwd_len = dir.len;
-   if (setup_git_directory_gently_1(, gitdir) <= 0) {
+   if (setup_git_directory_gently_1(, gitdir, 0) <= 0) {
strbuf_release();
return NULL;
}
@@ -994,7 +1003,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
die_errno(_("Unable to read current working directory"));
strbuf_addbuf(, );
 
-   switch (setup_git_directory_gently_1(, )) {
+   switch (setup_git_directory_gently_1(, , 1)) {
case GIT_DIR_NONE:
prefix = NULL;
break;
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v6 07/12] read_early_config(): avoid .git/config hack when unneeded

[Johannes Schindelin]

So far, we only look whether the startup_info claims to have seen a
git_dir.

However, do_git_config_sequence() (and consequently the
git_config_with_options() call used by read_early_config() asks the
have_git_dir() function whether we have a .git/ directory, which in turn
also looks at git_dir and at the environment variable GIT_DIR. And when
this is the case, the repository config is handled already, so we do not
have to do that again explicitly.

Let's just use the same function, have_git_dir(), to determine whether we
have to handle .git/config explicitly.

Signed-off-by: Johannes Schindelin 
---
 config.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 8808b132761..fc4625a71dd 100644
--- a/config.c
+++ b/config.c
@@ -1518,14 +1518,15 @@ void read_early_config(config_fn_t cb, void *data)
 * core.repositoryformatversion), we'll read whatever is in .git/config
 * blindly. Similarly, if we _are_ in a repository, but not at the
 * root, we'll fail to find .git/config (because it's really
-* ../.git/config, etc). See t7006 for a complete set of failures.
+* ../.git/config, etc), unless setup_git_directory() was already 
called.
+* See t7006 for a complete set of failures.
 *
 * However, we have historically provided this hack because it does
 * work some of the time (namely when you are at the top-level of a
 * valid repository), and would rarely make things worse (i.e., you do
 * not generally have a .git/config file sitting around).
 */
-   if (!startup_info->have_repository) {
+   if (!have_git_dir()) {
struct git_config_source repo_config;
 
memset(_config, 0, sizeof(repo_config));
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v6 09/12] Add t1309 to test read_early_config()

[Johannes Schindelin]

So far, we had no explicit tests of that function.

Signed-off-by: Johannes Schindelin 
---
 t/helper/test-config.c  | 15 +++
 t/t1309-early-config.sh | 50 +
 2 files changed, 65 insertions(+)
 create mode 100755 t/t1309-early-config.sh

diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 83a4f2ab869..8e3ed6a76cb 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -66,6 +66,16 @@ static int iterate_cb(const char *var, const char *value, 
void *data)
return 0;
 }
 
+static int early_config_cb(const char *var, const char *value, void *vdata)
+{
+   const char *key = vdata;
+
+   if (!strcmp(key, var))
+   printf("%s\n", value);
+
+   return 0;
+}
+
 int cmd_main(int argc, const char **argv)
 {
int i, val;
@@ -73,6 +83,11 @@ int cmd_main(int argc, const char **argv)
const struct string_list *strptr;
struct config_set cs;
 
+   if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
+   read_early_config(early_config_cb, (void *)argv[2]);
+   return 0;
+   }
+
setup_git_directory();
 
git_configset_init();
diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
new file mode 100755
index 000..0c55dee514c
--- /dev/null
+++ b/t/t1309-early-config.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='Test read_early_config()'
+
+. ./test-lib.sh
+
+test_expect_success 'read early config' '
+   test_config early.config correct &&
+   test-config read_early_config early.config >output &&
+   test correct = "$(cat output)"
+'
+
+test_expect_success 'in a sub-directory' '
+   test_config early.config sub &&
+   mkdir -p sub &&
+   (
+   cd sub &&
+   test-config read_early_config early.config
+   ) >output &&
+   test sub = "$(cat output)"
+'
+
+test_expect_success 'ceiling' '
+   test_config early.config ceiling &&
+   mkdir -p sub &&
+   (
+   GIT_CEILING_DIRECTORIES="$PWD" &&
+   export GIT_CEILING_DIRECTORIES &&
+   cd sub &&
+   test-config read_early_config early.config
+   ) >output &&
+   test -z "$(cat output)"
+'
+
+test_expect_success 'ceiling #2' '
+   mkdir -p xdg/git &&
+   git config -f xdg/git/config early.config xdg &&
+   test_config early.config ceiling &&
+   mkdir -p sub &&
+   (
+   XDG_CONFIG_HOME="$PWD"/xdg &&
+   GIT_CEILING_DIRECTORIES="$PWD" &&
+   export GIT_CEILING_DIRECTORIES XDG_CONFIG_HOME &&
+   cd sub &&
+   test-config read_early_config early.config
+   ) >output &&
+   test xdg = "$(cat output)"
+'
+
+test_done
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v6 11/12] t1309: document cases where we would want early config not to die()

[Johannes Schindelin]

Jeff King came up with a couple examples that demonstrate how the new
read_early_config() that looks harder for the current .git/ directory
could die() in an undesirable way.

Let's add those cases to the test script, to document what we would like
to happen when early config encounters problems.

Signed-off-by: Johannes Schindelin 
---
 t/t1309-early-config.sh | 25 +
 1 file changed, 25 insertions(+)

diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
index 0c55dee514c..027eca63a3c 100755
--- a/t/t1309-early-config.sh
+++ b/t/t1309-early-config.sh
@@ -47,4 +47,29 @@ test_expect_success 'ceiling #2' '
test xdg = "$(cat output)"
 '
 
+test_with_config ()
+{
+   rm -rf throwaway &&
+   git init throwaway &&
+   (
+   cd throwaway &&
+   echo "$*" >.git/config &&
+   test-config read_early_config early.config
+   )
+}
+
+test_expect_success 'ignore .git/ with incompatible repository version' '
+   test_with_config "[core]repositoryformatversion = 99" 2>err &&
+   grep "warning:.* Expected git repo version <= [1-9]" err
+'
+
+test_expect_failure 'ignore .git/ with invalid repository version' '
+   test_with_config "[core]repositoryformatversion = invalid"
+'
+
+
+test_expect_failure 'ignore .git/ with invalid config' '
+   test_with_config "["
+'
+
 test_done
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v6 06/12] Make read_early_config() reusable

[Johannes Schindelin]

The pager configuration needs to be read early, possibly before
discovering any .git/ directory.

Let's not hide this function in pager.c, but make it available to other
callers.

Signed-off-by: Johannes Schindelin 
---
 cache.h  |  1 +
 config.c | 31 +++
 pager.c  | 31 ---
 3 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/cache.h b/cache.h
index 086bd9fa433..973cc23d6d4 100644
--- a/cache.h
+++ b/cache.h
@@ -1801,6 +1801,7 @@ extern int git_config_from_blob_sha1(config_fn_t fn, 
const char *name,
 const unsigned char *sha1, void *data);
 extern void git_config_push_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
+extern void read_early_config(config_fn_t cb, void *data);
 extern void git_config(config_fn_t fn, void *);
 extern int git_config_with_options(config_fn_t fn, void *,
   struct git_config_source *config_source,
diff --git a/config.c b/config.c
index 0e9e1ebefc0..8808b132761 100644
--- a/config.c
+++ b/config.c
@@ -1503,6 +1503,37 @@ static void configset_iter(struct config_set *cs, 
config_fn_t fn, void *data)
}
 }
 
+void read_early_config(config_fn_t cb, void *data)
+{
+   git_config_with_options(cb, data, NULL, 1);
+
+   /*
+* Note that this is a really dirty hack that does the wrong thing in
+* many cases. The crux of the problem is that we cannot run
+* setup_git_directory() early on in git's setup, so we have no idea if
+* we are in a repository or not, and therefore are not sure whether
+* and how to read repository-local config.
+*
+* So if we _aren't_ in a repository (or we are but we would reject its
+* core.repositoryformatversion), we'll read whatever is in .git/config
+* blindly. Similarly, if we _are_ in a repository, but not at the
+* root, we'll fail to find .git/config (because it's really
+* ../.git/config, etc). See t7006 for a complete set of failures.
+*
+* However, we have historically provided this hack because it does
+* work some of the time (namely when you are at the top-level of a
+* valid repository), and would rarely make things worse (i.e., you do
+* not generally have a .git/config file sitting around).
+*/
+   if (!startup_info->have_repository) {
+   struct git_config_source repo_config;
+
+   memset(_config, 0, sizeof(repo_config));
+   repo_config.file = ".git/config";
+   git_config_with_options(cb, data, _config, 1);
+   }
+}
+
 static void git_config_check_init(void);
 
 void git_config(config_fn_t fn, void *data)
diff --git a/pager.c b/pager.c
index ae796433630..73ca8bc3b17 100644
--- a/pager.c
+++ b/pager.c
@@ -43,37 +43,6 @@ static int core_pager_config(const char *var, const char 
*value, void *data)
return 0;
 }
 
-static void read_early_config(config_fn_t cb, void *data)
-{
-   git_config_with_options(cb, data, NULL, 1);
-
-   /*
-* Note that this is a really dirty hack that does the wrong thing in
-* many cases. The crux of the problem is that we cannot run
-* setup_git_directory() early on in git's setup, so we have no idea if
-* we are in a repository or not, and therefore are not sure whether
-* and how to read repository-local config.
-*
-* So if we _aren't_ in a repository (or we are but we would reject its
-* core.repositoryformatversion), we'll read whatever is in .git/config
-* blindly. Similarly, if we _are_ in a repository, but not at the
-* root, we'll fail to find .git/config (because it's really
-* ../.git/config, etc). See t7006 for a complete set of failures.
-*
-* However, we have historically provided this hack because it does
-* work some of the time (namely when you are at the top-level of a
-* valid repository), and would rarely make things worse (i.e., you do
-* not generally have a .git/config file sitting around).
-*/
-   if (!startup_info->have_repository) {
-   struct git_config_source repo_config;
-
-   memset(_config, 0, sizeof(repo_config));
-   repo_config.file = ".git/config";
-   git_config_with_options(cb, data, _config, 1);
-   }
-}
-
 const char *git_pager(int stdout_is_tty)
 {
const char *pager;
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v6 08/12] read_early_config(): really discover .git/

[Johannes Schindelin]

Earlier, we punted and simply assumed that we are in the top-level
directory of the project, and that there is no .git file but a .git/
directory so that we can read directly from .git/config.

However, that is not necessarily true. We may be in a subdirectory. Or
.git may be a gitfile. Or the environment variable GIT_DIR may be set.

To remedy this situation, we just refactored the way
setup_git_directory() discovers the .git/ directory, to make it
reusable, and more importantly, to leave all global variables and the
current working directory alone.

Let's discover the .git/ directory correctly in read_early_config() by
using that new function.

This fixes 4 known breakages in t7006.

Signed-off-by: Johannes Schindelin 
---
 config.c | 31 ---
 t/t7006-pager.sh |  8 
 2 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/config.c b/config.c
index fc4625a71dd..030acc50aa2 100644
--- a/config.c
+++ b/config.c
@@ -1505,34 +1505,27 @@ static void configset_iter(struct config_set *cs, 
config_fn_t fn, void *data)
 
 void read_early_config(config_fn_t cb, void *data)
 {
+   struct strbuf buf = STRBUF_INIT;
+
git_config_with_options(cb, data, NULL, 1);
 
/*
-* Note that this is a really dirty hack that does the wrong thing in
-* many cases. The crux of the problem is that we cannot run
-* setup_git_directory() early on in git's setup, so we have no idea if
-* we are in a repository or not, and therefore are not sure whether
-* and how to read repository-local config.
-*
-* So if we _aren't_ in a repository (or we are but we would reject its
-* core.repositoryformatversion), we'll read whatever is in .git/config
-* blindly. Similarly, if we _are_ in a repository, but not at the
-* root, we'll fail to find .git/config (because it's really
-* ../.git/config, etc), unless setup_git_directory() was already 
called.
-* See t7006 for a complete set of failures.
-*
-* However, we have historically provided this hack because it does
-* work some of the time (namely when you are at the top-level of a
-* valid repository), and would rarely make things worse (i.e., you do
-* not generally have a .git/config file sitting around).
+* When setup_git_directory() was not yet asked to discover the
+* GIT_DIR, we ask discover_git_directory() to figure out whether there
+* is any repository config we should use (but unlike
+* setup_git_directory_gently(), no global state is changed, most
+* notably, the current working directory is still the same after the
+* call).
 */
-   if (!have_git_dir()) {
+   if (!have_git_dir() && discover_git_directory()) {
struct git_config_source repo_config;
 
memset(_config, 0, sizeof(repo_config));
-   repo_config.file = ".git/config";
+   strbuf_addstr(, "/config");
+   repo_config.file = buf.buf;
git_config_with_options(cb, data, _config, 1);
}
+   strbuf_release();
 }
 
 static void git_config_check_init(void);
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 304ae06c600..4f3794d415e 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -360,19 +360,19 @@ test_pager_choices   'git aliasedlog'
 test_default_pagerexpect_success 'git -p aliasedlog'
 test_PAGER_overrides  expect_success 'git -p aliasedlog'
 test_core_pager_overrides expect_success 'git -p aliasedlog'
-test_core_pager_subdirexpect_failure 'git -p aliasedlog'
+test_core_pager_subdirexpect_success 'git -p aliasedlog'
 test_GIT_PAGER_overrides  expect_success 'git -p aliasedlog'
 
 test_default_pagerexpect_success 'git -p true'
 test_PAGER_overrides  expect_success 'git -p true'
 test_core_pager_overrides expect_success 'git -p true'
-test_core_pager_subdirexpect_failure 'git -p true'
+test_core_pager_subdirexpect_success 'git -p true'
 test_GIT_PAGER_overrides  expect_success 'git -p true'
 
 test_default_pagerexpect_success test_must_fail 'git -p request-pull'
 test_PAGER_overrides  expect_success test_must_fail 'git -p request-pull'
 test_core_pager_overrides expect_success test_must_fail 'git -p request-pull'
-test_core_pager_subdirexpect_failure test_must_fail 'git -p request-pull'
+test_core_pager_subdirexpect_success test_must_fail 'git -p request-pull'
 test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p request-pull'
 
 test_default_pagerexpect_success test_must_fail 'git -p'
@@ -380,7 +380,7 @@ test_PAGER_overrides  expect_success test_must_fail 
'git -p'
 test_local_config_ignored expect_failure test_must_fail 'git -p'
 test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p'
 
-test_expect_failure TTY 'core.pager in 

[PATCH v6 05/12] Introduce the discover_git_directory() function

[Johannes Schindelin]

We modified the setup_git_directory_gently_1() function earlier to make
it possible to discover the GIT_DIR without changing global state.

However, it is still a bit cumbersome to use if you only need to figure
out the (possibly absolute) path of the .git/ directory. Let's just
provide a convenient wrapper function with an easier signature that
*just* discovers the .git/ directory.

We will use it in a subsequent patch to fix the early config.

Signed-off-by: Johannes Schindelin 
---
 cache.h |  7 +++
 setup.c | 43 +++
 2 files changed, 50 insertions(+)

diff --git a/cache.h b/cache.h
index 8c0e6442076..086bd9fa433 100644
--- a/cache.h
+++ b/cache.h
@@ -518,6 +518,13 @@ extern void set_git_work_tree(const char *tree);
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
 extern void setup_work_tree(void);
+/*
+ * Find GIT_DIR of the repository that contains the current working directory,
+ * without changing the working directory or other global state. The result is
+ * appended to gitdir. The return value is either NULL if no repository was
+ * found, or pointing to the path inside gitdir's buffer.
+ */
+extern const char *discover_git_directory(struct strbuf *gitdir);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
 extern char *prefix_path(const char *prefix, int len, const char *path);
diff --git a/setup.c b/setup.c
index 99a722ed5f9..411e8342972 100644
--- a/setup.c
+++ b/setup.c
@@ -924,6 +924,49 @@ static enum discovery_result 
setup_git_directory_gently_1(struct strbuf *dir,
}
 }
 
+const char *discover_git_directory(struct strbuf *gitdir)
+{
+   struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
+   size_t gitdir_offset = gitdir->len, cwd_len;
+   struct repository_format candidate;
+
+   if (strbuf_getcwd())
+   return NULL;
+
+   cwd_len = dir.len;
+   if (setup_git_directory_gently_1(, gitdir) <= 0) {
+   strbuf_release();
+   return NULL;
+   }
+
+   /*
+* The returned gitdir is relative to dir, and if dir does not reflect
+* the current working directory, we simply make the gitdir absolute.
+*/
+   if (dir.len < cwd_len && !is_absolute_path(gitdir->buf + 
gitdir_offset)) {
+   /* Avoid a trailing "/." */
+   if (!strcmp(".", gitdir->buf + gitdir_offset))
+   strbuf_setlen(gitdir, gitdir_offset);
+   else
+   strbuf_addch(, '/');
+   strbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);
+   }
+
+   strbuf_reset();
+   strbuf_addf(, "%s/config", gitdir->buf + gitdir_offset);
+   read_repository_format(, dir.buf);
+   strbuf_release();
+
+   if (verify_repository_format(, ) < 0) {
+   warning("ignoring git dir '%s': %s",
+   gitdir->buf + gitdir_offset, err.buf);
+   strbuf_release();
+   return NULL;
+   }
+
+   return gitdir->buf + gitdir_offset;
+}
+
 const char *setup_git_directory_gently(int *nongit_ok)
 {
static struct strbuf cwd = STRBUF_INIT;
-- 
2.12.0.windows.1.7.g94dafc3b124

Possible git blame bug?

[Domagoj Stolfa]

Hello,

yesterday I came across sort of a weird behaviour with git-blame. It would
appear when one queries the git blame on a specific file, such as:

$ git blame  --since=

it will blame the entire file on some commit of that year, regardless of the
fact whether the commit has actually touched that file or not.

This seems consistent across all the tested systems: FreeBSD, macOS, Ubuntu and
Fedora.

An example of the blame can be seen:

$ git blame tcp_output.c
cd0bc69e2fdd (wollman  1995-09-22 20:05:58 +   29)  *   
@(#)tcp_output.c8.4 (Berkeley) 5/24/95

compared to:

$ git blame tcp_output.c --since=2017
^e19f2a27ed8 (Domagoj Stolfa 2017-03-12 20:43:01 +0100   29)  * 
@(#)tcp_output.c8.4 (Berkeley) 5/24/95

$ git blame tcp_output.c --since=2016
^e4bdb83e76c (jceel2016-03-13 19:50:17 +   29)  *   
@(#)tcp_output.c8.4 (Berkeley) 5/24/95

$ git blame tcp_output.c --since=2015
^d749a6e6c70 (pfg  2015-03-13 18:42:43 +   29)  *   
@(#)tcp_output.c8.4 (Berkeley) 5/24/95

Of course, specifiying

$ git blame --since=1.1.2017

gives correct results, as expected.

The question is whether this is a bug or not, as --since= might not be a
valid filter. However, this might surprise a lot of users and mislead
development. I would personally like to see this behaviour changed to either a
blank report, a report of that year(making it a valid filter), but certainly not
blaming it on commits that have never touched that file.

-- 
Best regards,
Domagoj Stolfa


signature.asc
Description: PGP signature

[PATCH v6 04/12] setup_git_directory_1(): avoid changing global state

[Johannes Schindelin]

For historical reasons, Git searches for the .git/ directory (or the
.git file) by changing the working directory successively to the parent
directory of the current directory, until either anything was found or
until a ceiling or a mount point is hit.

Further global state may be changed in case a .git/ directory was found.

We do have a use case, though, where we would like to find the .git/
directory without having any global state touched, though: when we read
the early config e.g. for the pager or for alias expansion.

Let's just move all of code that changes any global state out of the
function `setup_git_directory_gently_1()` into
`setup_git_directory_gently()`.

In subsequent patches, we will use the _1() function in a new
`discover_git_directory()` function that we will then use for the early
config code.

Note: the new loop is a *little* tricky, as we have to handle the root
directory specially: we cannot simply strip away the last component
including the slash, as the root directory only has that slash. To remedy
that, we introduce the `min_offset` variable that holds the minimal length
of an absolute path, and using that to special-case the root directory,
including an early exit before trying to find the parent of the root
directory.

Signed-off-by: Johannes Schindelin 
---
 setup.c | 193 +++-
 1 file changed, 118 insertions(+), 75 deletions(-)

diff --git a/setup.c b/setup.c
index 23114cb7aa3..99a722ed5f9 100644
--- a/setup.c
+++ b/setup.c
@@ -818,50 +818,49 @@ static int canonicalize_ceiling_entry(struct 
string_list_item *item,
}
 }
 
+enum discovery_result {
+   GIT_DIR_NONE = 0,
+   GIT_DIR_EXPLICIT,
+   GIT_DIR_DISCOVERED,
+   GIT_DIR_BARE,
+   /* these are errors */
+   GIT_DIR_HIT_CEILING = -1,
+   GIT_DIR_HIT_MOUNT_POINT = -2
+};
+
 /*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
+ *
+ * Also, we avoid changing any global state (such as the current working
+ * directory) to allow early callers.
+ *
+ * The directory where the search should start needs to be passed in via the
+ * `dir` parameter; upon return, the `dir` buffer will contain the path of
+ * the directory where the search ended, and `gitdir` will contain the path of
+ * the discovered .git/ directory, if any. If `gitdir` is not absolute, it
+ * is relative to `dir` (i.e. *not* necessarily the cwd).
  */
-static const char *setup_git_directory_gently_1(int *nongit_ok)
+static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
+ struct strbuf *gitdir)
 {
const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
-   static struct strbuf cwd = STRBUF_INIT;
-   const char *gitdirenv, *ret;
-   char *gitfile;
-   int offset, offset_parent, ceil_offset = -1;
+   const char *gitdirenv;
+   int ceil_offset = -1, min_offset = has_dos_drive_prefix(dir->buf) ? 3 : 
1;
dev_t current_device = 0;
int one_filesystem = 1;
 
/*
-* We may have read an incomplete configuration before
-* setting-up the git directory. If so, clear the cache so
-* that the next queries to the configuration reload complete
-* configuration (including the per-repo config file that we
-* ignored previously).
-*/
-   git_config_clear();
-
-   /*
-* Let's assume that we are in a git repository.
-* If it turns out later that we are somewhere else, the value will be
-* updated accordingly.
-*/
-   if (nongit_ok)
-   *nongit_ok = 0;
-
-   if (strbuf_getcwd())
-   die_errno(_("Unable to read current working directory"));
-   offset = cwd.len;
-
-   /*
 * If GIT_DIR is set explicitly, we're not going
 * to do any discovery, but we still do repository
 * validation.
 */
gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
-   if (gitdirenv)
-   return setup_explicit_git_dir(gitdirenv, , nongit_ok);
+   if (gitdirenv) {
+   strbuf_addstr(gitdir, gitdirenv);
+   return GIT_DIR_EXPLICIT;
+   }
 
if (env_ceiling_dirs) {
int empty_entry_found = 0;
@@ -869,15 +868,15 @@ static const char *setup_git_directory_gently_1(int 
*nongit_ok)
string_list_split(_dirs, env_ceiling_dirs, PATH_SEP, 
-1);
filter_string_list(_dirs, 0,
   canonicalize_ceiling_entry, 
_entry_found);
-   ceil_offset = longest_ancestor_length(cwd.buf, _dirs);
+   ceil_offset = longest_ancestor_length(dir->buf, _dirs);
string_list_clear(_dirs, 0);
}
 

[PATCH v6 03/12] Prepare setup_discovered_git_directory() the root directory

[Johannes Schindelin]

Currently, the offset parameter (indicating what part of the cwd
parameter corresponds to the current directory after discovering the
.git/ directory) is set to 0 when we are running in the root directory.

However, in the next patches we will avoid changing the current working
directory while searching for the .git/ directory, meaning that the
offset corresponding to the root directory will have to be 1 to reflect
that this directory is characterized by the path "/" (and not "").

So let's make sure that setup_discovered_git_directory() only tries to
append the trailing slash to non-root directories.

Note: the setup_bare_git_directory() does not need a corresponding
change, as it does not want to return a prefix.

Signed-off-by: Johannes Schindelin 
---
 setup.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index 2ac891d4b9a..23114cb7aa3 100644
--- a/setup.c
+++ b/setup.c
@@ -721,8 +721,10 @@ static const char *setup_discovered_git_dir(const char 
*gitdir,
if (offset == cwd->len)
return NULL;
 
-   /* Make "offset" point to past the '/', and add a '/' at the end */
-   offset++;
+   /* Make "offset" point past the '/' (already the case for root dirs) */
+   if (offset != offset_1st_component(cwd->buf))
+   offset++;
+   /* Add a '/' at the end */
strbuf_addch(cwd, '/');
return cwd->buf + offset;
 }
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v6 00/12] Fix the early config

[Johannes Schindelin]

These patches are an attempt to make Git's startup sequence a bit less
surprising.

The idea here is to refactor setup_git_directory()'s code so that the
underlying .git/ directory discovery can be run without changing any
global state nor the current working directory, and then to use that
functionality also in read_early_config().

My own use case: in the GVFS Git fork, we need to execute pre-command
and post-command hooks before and after *every* Git command. A previous
version of the pre-command/post-command hook support was broken, as it
used run_hook() which implicitly called setup_git_directory() too early.
The discover_git_directory() function (and due to core.hooksPath also
the read_early_config() function) helped me fix this.

Notable notes:

- Even if it can cause surprising problems, `init` and `clone` are not
  special-cased. Rationale: it would introduce technical debt and
  violate the Principle Of Least Astonishment.

- The read_early_config() function does not cache Git directory
  discovery nor read values. This is left for another patch series, if
  it ever becomes necessary.

- The alias handling in git.c could possibly benefit from this work, but
  again, this is a separate topic from the current patch series.

Changes since v5:

- Reworded comment about `gitdir` being relative to `dir` so that it
  does not confuse Junio anymore

- Removed a superfluous `error_code &&` in an if() condition

- Reworded the oneline of 9/11

- Added a new patch at the end that marks the two NEEDSWORK issues that
  Junio identified (but that should not be addressed in this patch
  series, of course, because the entire idea of the early-config work is
  to *preserve* the current behavior of setup_git_directory() while making
  it possible for read_early_config() to reuse the same code paths)


Johannes Schindelin (12):
  t7006: replace dubious test
  setup_git_directory(): use is_dir_sep() helper
  Prepare setup_discovered_git_directory() the root directory
  setup_git_directory_1(): avoid changing global state
  Introduce the discover_git_directory() function
  Make read_early_config() reusable
  read_early_config(): avoid .git/config hack when unneeded
  read_early_config(): really discover .git/
  Add t1309 to test read_early_config()
  setup_git_directory_gently_1(): avoid die()ing
  t1309: document cases where we would want early config not to die()
  setup.c: mention unresolved problems

 cache.h |   8 ++
 config.c|  25 +
 pager.c |  31 --
 setup.c | 253 +---
 t/helper/test-config.c  |  15 +++
 t/t1309-early-config.sh |  75 ++
 t/t7006-pager.sh|  18 +++-
 7 files changed, 314 insertions(+), 111 deletions(-)
 create mode 100755 t/t1309-early-config.sh


base-commit: d6db3f216544d05e09159756812ccbcb16861d71
Published-As: https://github.com/dscho/git/releases/tag/early-config-v6
Fetch-It-Via: git fetch https://github.com/dscho/git early-config-v6

Interdiff vs v5:

 diff --git a/setup.c b/setup.c
 index 6733ba5fe82..64f922a9378 100644
 --- a/setup.c
 +++ b/setup.c
 @@ -531,6 +531,7 @@ const char *read_gitfile_gently(const char *path, int 
*return_error_code)
ssize_t len;
  
if (stat(path, )) {
 +  /* NEEDSWORK: discern between ENOENT vs other errors */
error_code = READ_GITFILE_ERR_STAT_FAILED;
goto cleanup_return;
}
 @@ -839,8 +840,8 @@ enum discovery_result {
   * The directory where the search should start needs to be passed in via the
   * `dir` parameter; upon return, the `dir` buffer will contain the path of
   * the directory where the search ended, and `gitdir` will contain the path of
 - * the discovered .git/ directory, if any. This path may be relative against
 - * `dir` (i.e. *not* necessarily the cwd).
 + * the discovered .git/ directory, if any. If `gitdir` is not absolute, it
 + * is relative to `dir` (i.e. *not* necessarily the cwd).
   */
  static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
  struct strbuf *gitdir,
 @@ -902,10 +903,10 @@ static enum discovery_result 
setup_git_directory_gently_1(struct strbuf *dir,
if (!gitdirenv) {
if (die_on_error ||
error_code == READ_GITFILE_ERR_NOT_A_FILE) {
 +  /* NEEDSWORK: fail if .git is not file nor dir 
*/
if (is_git_directory(dir->buf))
gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
 -  } else if (error_code &&
 - error_code != READ_GITFILE_ERR_STAT_FAILED)
 +  } else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
return GIT_DIR_INVALID_GITFILE;
}
strbuf_setlen(dir, 

[PATCH v6 01/12] t7006: replace dubious test

[Johannes Schindelin]

The idea of the test case "git -p - core.pager is not used from
subdirectory" was to verify that the setup_git_directory() function had
not been called just to obtain the core.pager setting.

However, we are about to fix the early config machinery so that it
*does* work, without messing up the global state.

Once that is done, the core.pager setting *will* be used, even when
running from a subdirectory, and that is a Good Thing.

The intention of that test case, however, was to verify that the
setup_git_directory() function has not run, because it changes global
state such as the current working directory.

To keep that spirit, but fix the incorrect assumption, this patch
replaces that test case by a new one that verifies that the pager is
run in the subdirectory, i.e. that the current working directory has
not been changed at the time the pager is configured and launched, even
if the `rev-parse` command requires a .git/ directory and *will* change
the working directory.

Signed-off-by: Johannes Schindelin 
---
 t/t7006-pager.sh | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index c8dc665f2fd..304ae06c600 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -378,9 +378,19 @@ test_GIT_PAGER_overrides  expect_success test_must_fail 
'git -p request-pull'
 test_default_pagerexpect_success test_must_fail 'git -p'
 test_PAGER_overrides  expect_success test_must_fail 'git -p'
 test_local_config_ignored expect_failure test_must_fail 'git -p'
-test_no_local_config_subdir expect_success test_must_fail 'git -p'
 test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p'
 
+test_expect_failure TTY 'core.pager in repo config works and retains cwd' '
+   sane_unset GIT_PAGER &&
+   test_config core.pager "cat >cwd-retained" &&
+   (
+   cd sub &&
+   rm -f cwd-retained &&
+   test_terminal git -p rev-parse HEAD &&
+   test_path_is_file cwd-retained
+   )
+'
+
 test_doesnt_paginate  expect_failure test_must_fail 'git -p nonsense'
 
 test_pager_choices   'git shortlog'
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH v6 02/12] setup_git_directory(): use is_dir_sep() helper

[Johannes Schindelin]

It is okay in practice to test for forward slashes in the output of
getcwd(), because we go out of our way to convert backslashes to forward
slashes in getcwd()'s output on Windows.

Still, the correct way to test for a dir separator is by using the
helper function we introduced for that very purpose. It also serves as a
good documentation what the code tries to do (not "how").

Signed-off-by: Johannes Schindelin 
---
 setup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 8f64fbdfb28..2ac891d4b9a 100644
--- a/setup.c
+++ b/setup.c
@@ -910,7 +910,9 @@ static const char *setup_git_directory_gently_1(int 
*nongit_ok)
return setup_bare_git_dir(, offset, nongit_ok);
 
offset_parent = offset;
-   while (--offset_parent > ceil_offset && cwd.buf[offset_parent] 
!= '/');
+   while (--offset_parent > ceil_offset &&
+  !is_dir_sep(cwd.buf[offset_parent]))
+   ; /* continue */
if (offset_parent <= ceil_offset)
return setup_nongit(cwd.buf, nongit_ok);
if (one_filesystem) {
-- 
2.12.0.windows.1.7.g94dafc3b124

[PATCH]v2 adding built-in driver for javascript

[sourav mondal]

javascript is one of the famous langugae,it's needs a built-in driver. As it 
was not present in the userdiff & this leads to the patch.
first line consists of some of the well used javascript 
keywords.statements in js use one or many keywords like variable declaration, 
function definition, logical opreation etc.The following line is for statements 
of type object.method() & it is expected to end any statement using ";". The 
word_regex in js is usual alpha-numeric.last two line shows all the different 
types of opreators in js and different types of number system used in js are 
also defined. 

Signed-off-by: sourav mondal 
---

I'm working on "Add more built-in driver for userdiff" as my microproject for 
Gsoc17. This patch is for javascript which is one of the popular language at 
this time. I'm willing to add more driver for other laguage that isn't present 
in userdiff.c and again I'm willing to participate in Gsoc17 with git. I'm 
eager to know about this patch.

thanks & regards
sourav
   
 userdiff.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/userdiff.c b/userdiff.c
index 8b732e4..2f8e078 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -160,6 +160,16 @@ IPATTERN("css",
 "-?[_a-zA-Z][-_a-zA-Z0-9]*" /* identifiers */
 "|-?[0-9]+|\\#[0-9a-fA-F]+" /* numbers */
 ),
+PATTERNS("javascript",
+/* keywords/patterns*/
+"^[ 
\t]*(var|if|else|for|do|while|switch|case|function|break|continue|new|return|class|super|instanceof)"
+"^[ \t]*(([a-zA-Z_][a-zA-Z0-9])*[ \t]*\\.?[a-zA-Z_]*\\(\\)[ \t]*);$",
+/* word_regex */
+"[a-zA-Z_][a-zA-Z0-9]*"
+"|[-+0-9.eE]+|0[bB]?|[xX]?|o?[0-9a-fA-F]+"
+"|[==-+*/%<>&|!**=^]="
+"|--|\\+\\+|<<=?|>>>?=?|&&|\|\|"
+),
 { "default", NULL, -1, { NULL, 0 } },
 };
 #undef PATTERNS
-- 
2.9.3

Re: git-push branch confusion caused by user mistake

[Phil Hord]

On Mon, Mar 13, 2017 at 1:55 AM Jacob Keller  wrote:
> On Fri, Mar 10, 2017 at 2:13 PM, Junio C Hamano  wrote:
> > Phil Hord  writes:
> >> I think git should be smarter about deducing the dest ref from the
> >> source ref if the source ref is in refs/remotes, but I'm not sure how
> >> far to take it.
> >
> > My knee-jerk reaction is "Don't take it anywhere".
> >
> > Giving a refspec from the command line is an established way to
> > defeat the default behaviour when you do not give any and only the
> > remote, and making it do things behind user's back, you would be
> > robbing the escape hatch from people.
>
> It might be worth having some warning or something happen here? I've
> had several  co-workers at $DAYJOB get confused by this sort of thing.

On one very active project at $work, we have 380,000 commits, 4600
branches in refs/heads and 96 branches in refs/remotes.  About half of
the refs/remotes (43) are obviously user errors.  The other half it's
not possible for me to know.

I suggested to our admins to block attempts to push to
'refs/remotes/*' so in the future users don't lose track of commits
they think they pushed.  But I don't know if that will really happen.

Thanks for the counterexample feedback.

Re: Stable GnuPG interface, git should use GPGME

[Christian Neukirchen]

"brian m. carlson"  writes:

> Because the amount of the gpg API we actually use is very small, a user
> who wants to use a custom signature program (say, OpenBSD's signify),
> can actually write a simple wrapper that mimics it and use that instead.

FWIW, I did this, and it was quite some effort to figure out the
actual API that is needed:

http://chneukirchen.org/dotfiles/bin/git-signify

-- 
Christian Neukirchen    http://chneukirchen.org

Re: [PATCH] Put sha1dc on a diet

[Jeff King]

On Mon, Mar 13, 2017 at 07:42:17PM +, Dan Shumow wrote:

> Marc just made a commit this morning fixing problems with the merge.
> Please give the latest in feature/performance a try, as that seems to
> eliminate the problem.

Yeah, b17728507 makes the problem go away for me. Thanks.

FWIW, I have all sha1s on github.com running through this right now
(actually, the ad744c8b7 version), and logging any false-positives on
the collision detection. Nothing so far, after a few hours.

-Peff

Re: [PATCH v5 10/11] setup_git_directory_gently_1(): avoid die()ing

[Junio C Hamano]

Johannes Schindelin  writes:

> PLEASE NOTE: the purpose of this patch series is to allow the same
> function (setup_git_directory_gently_1()) to be the work horse for
> setup_git_directory() as before, but also for the new
> discover_git_directory() function that is introduced to fix
> read_early_config() to avoid hardconfig .git/config.
>
> The purpose is *not* to change any current behavior. Please do not ask me
> to do that in this patch series.

Please note that pointing out potential problems is only asking for
confirmation ("yes that is bad") or for enlightenment ("no, you are
reading the code wrong and that bad thing does not happen
because...").  Even in the former case, unless the badness is
introduced by the change, pointing out potential problems is *NOT*
asking to change the patch to fix existing issues.

The former case may split into two.  "Yes I agree that is bad and it
is the same badness as the version without this change", in which
case we may want to leave a "NEEDSWORK" comment in-code so that
people can notice when browsing the code (but fixing the badness
would be outside the scope of the series), and "yes that is bad and
we shouldn't introduce that badness", in which case we do want to
fix it in the patch.

So do not read my (or anybody else's) reviews as _asking_ changes
and making further fixes, and you'll do better.

Re: [GSoC][PATCH 1/3] path.c: Add xdg_cache_home to get paths under XDG_CACHE_HOME

[Devin Lehmacher]

> And one final note. I notice that we return NULL if the user has no
> HOME. But I'm not sure most callers are prepared to handle this. E.g.,
> if you have no ident set and no HOME, then we will pass NULL to lstat().
> On Linux at least that just gets you EFAULT, but I wouldn't be surprised
> if it's a segfault on other systems (probably at least Windows, where we
> have an lstat wrapper that calls strlen on the filename).

Right now we check the return value from these two functions and if it
is NULL we instead use some other configuration location.

That said I agree that this could lead to unexpected behavior in some
scenarios.

Devin

Re: [PATCH v5 10/11] setup_git_directory_gently_1(): avoid die()ing

[Johannes Schindelin]

Hi Junio,

PLEASE NOTE: the purpose of this patch series is to allow the same
function (setup_git_directory_gently_1()) to be the work horse for
setup_git_directory() as before, but also for the new
discover_git_directory() function that is introduced to fix
read_early_config() to avoid hardconfig .git/config.

The purpose is *not* to change any current behavior. Please do not ask me
to do that in this patch series.

Now to your comments:

On Fri, 10 Mar 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > @@ -890,14 +892,22 @@ static enum discovery_result 
> > setup_git_directory_gently_1(struct strbuf *dir,
> > if (one_filesystem)
> > current_device = get_device_or_die(dir->buf, NULL, 0);
> > for (;;) {
> > -   int offset = dir->len;
> > +   int offset = dir->len, error_code = 0;
> >  
> > if (offset > min_offset)
> > strbuf_addch(dir, '/');
> > strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
> > -   gitdirenv = read_gitfile(dir->buf);
> > -   if (!gitdirenv && is_git_directory(dir->buf))
> > -   gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
> > +   gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
> > +   NULL : _code);
> > +   if (!gitdirenv) {
> 
> We tried to read ".git" as a regular file, but couldn't.  There are
> some cases but I am having trouble to convince myself all cases are
> covered correctly here, so let me follow the code aloud.

It is subtle and finicky, I agree.

> > +   if (die_on_error ||
> > +   error_code == READ_GITFILE_ERR_NOT_A_FILE) {
> > +   if (is_git_directory(dir->buf))
> > +   gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
> 
> If we are told to die on error, but if it is a Git directory, then
> we do not have to (and want to) die and instead report that
> directory as discovered.
> 
> If we are to report the failure status instead of dying, we also do
> want to recover when the read_gitfile_gentrly() failed only because
> it was a real Git directory.  READ_GITFILE_ERR_NOT_A_FILE could be a
> signal for that, and we recover after making sure it is a Git
> directory.
> 
> Among the READ_GITFILE_ERR_* codes, ERR_NOT_A_FILE is the only one
> we attempt this recovery.  All others just take the "else if" thing
> below.
> 
> What happens when is_git_directory() test here does not pass,
> though?  Let's say stat() in read_gitfile_gently() found a FIFO
> there, it gives us ERR_NOT_A_FILE, is_git_directory() would say
> "Nah", and then ...?  Don't we want the other side for this if()
> statement that returns failure?

The current code as per `master` detects the NOT_A_FILE condition, and as
the parameter return_error_code was passed as NULL (because read_gitfile is
actually #define'd in cache.h to call read_gitfile_gently with NULL as
second parameter), it calls read_gitfile_error_die(). That function
*ignores* the NOT_A_FILE error condition, and as a consequence returns to
read_gitfile_gently() which then returns NULL because there *was* an
error_code.

That means that setup_git_directory_gently_1() will retrieve a NULL from
the read_gitfile() call, which means it then goes on to test whether it
is looking at a .git/ directory via is_git_directory() and if that returns
false, the loop will continue to look at the *parent* directory.

That, in turn, means that what you are asking for is a change in behavior,
and as I am unwilling to introduce a change in behavior with this patch
series, my patch does the exact right thing.

> > +   } else if (error_code &&
> > +  error_code != READ_GITFILE_ERR_STAT_FAILED)
> > +   return GIT_DIR_INVALID_GITFILE;
> > +   }
> 
> On the other hand, if read_gitfile_gently() didn't say "we found
> something that is not a regular file" we come here.  If the error
> came because there wasn't ".git" in the directory we are looking at,
> i.e. stat(2) in read_gitfile_gently() gave ENOENT, it is perfectly
> normal and we just want to go one level up.
> 
> But shouldn't read_gitfile_gently() be inspecting errno when it sees
> a stat() failure?  If there _is_ ".git" but we failed to stat it for
> whatever reason (EACCES, ELOOP,...), we do not want to go up but
> give the INVALID_GITFILE from here, just like other cases, no?
> For that I imagine that ERR_STAT_FAILED needs to be split into two,
> one for true ERR_STAT_FAILED (from which we cannot recover) and the
> other for ERR_ENOENT to signal us that there is nothing there (which
> allows us to go up).

True. But again, you are asking for a *change in behavior*, which is not
the purpose of this patch series.

> By the way, is the "error_code &&" needed?

It is not! I had the order of the if/else blocks completely differently
originally [*1*] 

Re: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials

[Devin Lehmacher]

> They wouldn't.  It was my oblique way to say "it is unclear from the
> patch description and the code why this is a good idea---it needs to
> be explained better" ;-).

Ok, I will submit a new patch with a better explanation.

Devin

Re: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials

[Devin Lehmacher]

>  I somehow feel that the order of precedence should be the other way
>  around, though.
>
>  If somebody really wants to use the xdg location and has a socket
>  already there, using that existing socket would be the right thing
>  to do.  However, when neither ~/.git-credential-cache/socket nor
>  ~/.cache/git/socket exists, why should we prefer the latter over the
>  former?

At least to me the latter feels superior. It keeps clutter out of the
home directory putting it in a directory with all other cache files from
other programs. If we continue to put the socket at
~/.git-credential-cache/socket by default then checking
~/.cache/git/credential/socket seems useless since the daemon will check
socket locations provided with —-socket anyways and there is no other
way to create the socket at the new location.

To me it would make sense to check the old location and if it is not 
there then use the XDG location to follow that standard. 

I guess that this is not the same behavior that git config —-global 
(it will only put its configuration file there if .gitconfig is unreadable)
has right now though so we should stay consistent and reverse the precedence
here.

Thanks for the feedback,
Devin

Re: 'git add --patch' no longer allows you to select files before selecting patches

[Junio C Hamano]

Jeff King  writes:

> My, this seems to be a popular bug. This is the third report already. :)
>
> The answer to your final question is that it's a bug. The fix is in
> c852bd54bd87fdcdc825f5d45c26aa745be13ba6, which is currently in the
> "master" branch of git.git.

If only more people learn to run 'next', we would get less reports on
already fixed bug and instead get more reports on unexpected regressions
caused by these fixes, both of which would be a very good thing ;-)

> Junio, I notice this isn't in "maint", and probably should be. It's a
> regression in v2.12.0, so hopefully we'd fix it in v2.12.1.

Yes, thanks for an extra reminder.  The reason why I pushed it out
on 'master' during weekend is so that we can have it at least for a
few days there before merging it down to 'maint'.

Re: 'git add --patch' no longer allows you to select files before selecting patches

[Junio C Hamano]

Anthony Scian  writes:

> Similarly the patch sub-command in ‘git add —interactive’ goes
> immediately to selecting patches starting with the first file.  Is
> there a git configuration that would being back the old behaviour?
> Why was this changed?

Because people are careless ;-) 

The fix is already proposed and in the 'master' branch.  Please keep
using that version to make sure there is no unexpected regression
caused by the fix.

Thanks.

Re: [RFC][PATCH] index-pack: add testcases found using AFL

[Vegard Nossum]

On 13/03/2017 18:11, Junio C Hamano wrote:

Vegard Nossum  writes:


However, I think it's more useful to think of these testcases not as
"binary test that nobody knows what they are doing", but as "(sometimes
invalid) packfiles which tickle interesting code paths in the packfile
parser".

With this perspective it becomes clearer that while they were generated
from the code, they also in a sense describe the packfile format itself.


I do agree with these two paragraphs (that is why I said that
continuously running fuzzer tests on the codebase would have value),
and I really appreciate the effort.


I did a few experiments in changing the code of the packfile reader in
various small ways (e.g. deleting a check, reordering some code) to see
the effects of the testcases found by fuzzing, and I have to admit it
was fairly disappointing. The testcases I added did not catch a single
buggy change, whereas the other testcases did catch many of them.


In short, the summary of the above three paragraphs is that we still
do believe the general approach of using fuzzer has value, but your
experiment indicates that data presented in the patch in this thread
weren't particularly good examples to demonstrate the merit?


Correct.

I thought a priori that the testcases found by AFL would work well as a
regression suite in the face of buggy code changes, but this turned out
not to be the case in practice when I tried introducing bugs on purpose.

The testcases would still have value for the following purposes:

- as a seed for continued fuzzing (as the fuzzing effort would not have
to start over from scratch)

- as a way to quickly find an input that reaches a specific line of code
without having to manually poke at a packfile

- as a basis for writing new testcases with specific expected results


Vegard

Re: 'git add --patch' no longer allows you to select files before selecting patches

[Jeff King]

On Mon, Mar 13, 2017 at 02:51:52PM -0400, Anthony Scian wrote:

> Similarly the patch sub-command in ‘git add —interactive’ goes
> immediately to selecting patches starting with the first file.
> Is there a git configuration that would being back the old behaviour?
> Why was this changed?

My, this seems to be a popular bug. This is the third report already. :)

The answer to your final question is that it's a bug. The fix is in
c852bd54bd87fdcdc825f5d45c26aa745be13ba6, which is currently in the
"master" branch of git.git.

Junio, I notice this isn't in "maint", and probably should be. It's a
regression in v2.12.0, so hopefully we'd fix it in v2.12.1.

-Peff

Re: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials

[Junio C Hamano]

Jeff King  writes:

> On Mon, Mar 13, 2017 at 11:09:11AM -0700, Junio C Hamano wrote:
>
>> > +  if (home_socket)
>> > +  if (file_exists(home_socket))
>> > +  return home_socket;
>> > +  else
>> > +  free(home_socket);
>> > +
>> > +  return xdg_cache_home("credential/socket");
>> > +}
>> 
>> I somehow feel that the order of precedence should be the other way
>> around, though.  
>> 
>> If somebody really wants to use the xdg location and has a socket
>> already there, using that existing socket would be the right thing
>> to do.  However, when neither ~/.git-credential-cache/socket nor
>> ~/.cache/git/socket exists, why should we prefer the latter over the
>> former?
>
> How would they get a socket in the xdg location if we never create one?

They wouldn't.  It was my oblique way to say "it is unclear from the
patch description and the code why this is a good idea---it needs to
be explained better" ;-).

Re: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials

[Jeff King]

On Mon, Mar 13, 2017 at 11:09:11AM -0700, Junio C Hamano wrote:

> > +   if (home_socket)
> > +   if (file_exists(home_socket))
> > +   return home_socket;
> > +   else
> > +   free(home_socket);
> > +
> > +   return xdg_cache_home("credential/socket");
> > +}
> 
> I somehow feel that the order of precedence should be the other way
> around, though.  
> 
> If somebody really wants to use the xdg location and has a socket
> already there, using that existing socket would be the right thing
> to do.  However, when neither ~/.git-credential-cache/socket nor
> ~/.cache/git/socket exists, why should we prefer the latter over the
> former?

How would they get a socket in the xdg location if we never create one?

I think the point of this commit is that we should generally prefer the
xdg locations to ones that were simply made up by Git.

As an aside, I also wondered if stale socket files might cause us to
keep picking the "old" location forever. But it looks like
credential-cache--daemon does use a tempfile handler to make sure it
cleans up the socket afterwards (and we are checking for the actual
socket, not just the presence of the outer directory, which is good).

-Peff

Re: [PATCH] difftool: handle changing symlinks in dir-diff mode

[Junio C Hamano]

David Aguilar  writes:

> - if (S_ISLNK(lmode)) {
> + if (S_ISLNK(lmode) && !is_null_oid()) {
>   char *content = read_sha1_file(loid.hash, , );
>   add_left_or_right(, src_path, content, 0);
>   free(content);
>   }
>  
> - if (S_ISLNK(rmode)) {
> + if (S_ISLNK(rmode) && !is_null_oid()) {
>   char *content = read_sha1_file(roid.hash, , );
>   add_left_or_right(, dst_path, content, 1);
>   free(content);

On this part I didn't comment in my previous message, but what is
the implication of omitting add-left-or-right and not registering
this symbolic link modified in the working tree to the symlinks2
table?

I am wondering if these should be more like

if (S_ISLNK(lmode) {
char *content = get_symlink(src_path, );
add_left_or_right(, src_path, content, 0);
free(content);
}

with get_symlink() helper that does

 - if the object name is not 0{40}, read from the object store

 - if the object name is 0{40}, that means we need to read the real
   contents from the working tree file, so do the "readlink(2) if
   symbolic link is supported, otherwise open/read/close the stub
   file sitting there" thing.

Similary to the right hand side tree.

Discarding "content" after reading feels wasteful, as that is the
information we would be using when populating the rstate and lstaten
working trees later in the loop, but that would probably need a
larger surgery to the code to optimize, I would imagine.