[ANNOUNCE] tig-2.2.1

[Jonas Fonseca]

Hello,

A new minor version of tig is available which adds support for
diff-highlight (see instructions below) and navigation between merge
commits as well as several keybinding tweaks.

Tarballs should now be downloaded from GitHub. Either go to
https://github.com/jonas/tig/releases or use the following pattern:


https://github.com/jonas/tig/releases/download/tig-VERSION/tig-VERSION.tar.gz

MD5 checksums can be found at:


https://github.com/jonas/tig/releases/download/tig-VERSION/tig-VERSION.tar.gz.md5

Similarly, the home page is now also on GitHub at
https://jonas.github.io/tig/. A big thanks to Simon L. B. Nielsen for
generously hosting Tig on nitro.dk!

Release notes
-
Improvements:

 - Support Git's 'diff-highlight' program when `diff-highlight` is set
   to either true or the path of the script to use for post-processing.
 - Add navigation between merge commits. (GH #525)
 - Add 'A' as a binding to apply a stash without dropping it.
 - Bind 'Ctrl-D' and 'Ctrl-U' to half-page movements by default.
 - manual: Mention how to change default Up/Down behavior in diff view.

Bug fixes

 - Reorganize checking of libraries for termcap functions.
 - Fix `:goto ` error message.

Change summary
--
The short diffstat and log summary for changes made in this release.

 118 files changed, 3765 insertions(+), 3284 deletions(-)

22  Jonas Fonseca
 1  Frank Fesevur
 1  Jelte Fennema
 1  Jeremy Lin
 1  Parker Coates
 1  Philipp Gesang
 1  Ramsay Jones
 1  David Lin
 1  lightside

-- 
Jonas Fonseca

Re: [PATCH v15 07/27] bisect--helper: `bisect_reset` shell function in C

[Pranit Bauva]

Hey Stephan,

On Thu, Nov 17, 2016 at 4:53 AM, Stephan Beyer  wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 4254d61..d84ba86 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -84,12 +89,47 @@ static int write_terms(const char *bad, const char *good)
>>   return (res < 0) ? -1 : 0;
>>  }
>>
>> +static int bisect_reset(const char *commit)
>> +{
>> + struct strbuf branch = STRBUF_INIT;
>> +
>> + if (!commit) {
>> + if (strbuf_read_file(, git_path_bisect_start(), 0) < 1) 
>> {
>> + printf("We are not bisecting.\n");
>
> I think this string should be marked for translation.

True. Thanks!

>> + return 0;
>> + }
>> + strbuf_rtrim();
> [...]
>> @@ -121,6 +163,10 @@ int cmd_bisect__helper(int argc, const char **argv, 
>> const char *prefix)
>>   if (argc != 0)
>>   die(_("--bisect-clean-state requires no arguments"));
>>   return bisect_clean_state();
>> + case BISECT_RESET:
>> + if (argc > 1)
>> + die(_("--bisect-reset requires either zero or one 
>> arguments"));
>
> This error message is imho a little unspecific (but this might not be an
> issue because bisect--helper commands are not really exposed to the
> user). Maybe "--bisect-reset requires either no argument or a commit."?

That sounds good.

>> + return bisect_reset(argc ? argv[0] : NULL);
>>   default:
>>   die("BUG: unknown subcommand '%d'", cmdmode);
>>   }

Regards,
Pranit Bauva

Re: Rebasing cascading topic trees

[Norbert Kiesel]

More things I learned!

So there are (at least) 2 possible approaches: using history, or using
local tracking branches.  The latter looks actually nicer to me, with
the exception that if asks for a `git pull`.  Using `git pull
--rebase` actually also ends up with the same tree, but I like the
rebase better.

The following 2 scripts show the 2 approaches.  Only difference is in
the creation of topic/b and the last rebase command.

# History
mkdir topic; cd topic
git init
date > a; git add a; git commit -m a
date > b; git add b; git commit -m b
git branch -b topic/a
git checkout -b topic1
date > c; git add c; git commit -m c
date > d; git add d; git commit -m d
git checkout -b topic2
date > e; git add e; git commit -m e
date > f; git add f; git commit -m f
git checkout master
date > g; git add g; git commit -m g
echo "before rebase"; git log --oneline --graph --all
git rebase master topic1
echo "after rebase of topic1"; git log --oneline --graph --all
git rebase --onto=topic1 topic1@{1} topic2
echo "after rebase of topic2"; git log --oneline --graph --all

# Tracking
mkdir topic; cd topic
git init
date > a; git add a; git commit -m a
date > b; git add b; git commit -m b
git branch -b topic/a
git checkout -b topic1
date > c; git add c; git commit -m c
date > d; git add d; git commit -m d
git checkout --track topic1 -b topic2
date > e; git add e; git commit -m e
date > f; git add f; git commit -m f
git checkout master
date > g; git add g; git commit -m g
echo "before rebase"; git log --oneline --graph --all
git rebase master topic1
echo "after rebase of topic1"; git log --oneline --graph --all
git rebase --onto=topic1 master topic2
echo "after rebase of topic2"; git log --oneline --graph --all

On Wed, Nov 16, 2016 at 5:45 PM, Jeff King  wrote:
> On Wed, Nov 16, 2016 at 04:12:20PM -0800, Norbert Kiesel wrote:
>
>> Yes, `git rebase --onto topic1 topic1@{1} topic2` is the answer!
>
> See also the `--fork-point` option, which (I think) should do this for
> you (and is the default if "topic1" is the configured upstream for
> topic2 and you just run "git rebase").
>
> -Peff

Re: Rebasing cascading topic trees

[Jeff King]

On Wed, Nov 16, 2016 at 04:12:20PM -0800, Norbert Kiesel wrote:

> Yes, `git rebase --onto topic1 topic1@{1} topic2` is the answer!

See also the `--fork-point` option, which (I think) should do this for
you (and is the default if "topic1" is the configured upstream for
topic2 and you just run "git rebase").

-Peff

Re: Re* Protecting old temporary objects being reused from concurrent "git gc"?

[Jeff King]

On Wed, Nov 16, 2016 at 05:35:47PM -0800, Junio C Hamano wrote:

> OK, here is what I have queued.
> 
> -- >8 --
> Subject: cache-tree: make sure to "touch" tree objects the cache-tree records
> 
> The cache_tree_fully_valid() function is called by callers that want
> to know if they need to call cache_tree_update(), i.e. as an attempt
> to optimize. They all want to have a fully valid cache-tree in the
> end so that they can write a tree object out.

That makes sense. I was focusing on cache_tree_update() call, but we do
not even get there in the fully-valid case.

So I think this approach is nice as long as there is not a caller who
asks "are we fully valid? I do not need to write, but was just
wondering". That should be a read-only operation, but the freshen calls
may fail with EPERM, for example.

I do not see any such callers, nor do I really expect any. Just trying
to think through the possible consequences.

> Strictly speaking, freshing these tree objects at each and every
> level is probably unnecessary, given that anything reachable from a
> young object inherits the youth from the referring object to be
> protected from pruning.  It should be sufficient to freshen only the
> very top-level tree instead.  Benchmarking and optimization is left
> as an exercise for later days.

Good observation, and nicely explained all around.

-Peff

Re* Protecting old temporary objects being reused from concurrent "git gc"?

[Junio C Hamano]

Jeff King  writes:

> I think the case that is helped here is somebody who runs "git
> write-tree" and expects that the timestamp on those trees is fresh. So
> even more a briefly used index, like:
>
>   export GIT_INDEX_FILE=/tmp/foo
>   git read-tree ...
>   git write-tree
>   rm -f $GIT_INDEX_FILE
>
> we'd expect that a "git gc" which runs immediately after would see those
> trees as recent and avoid pruning them (and transitively, any blobs that
> are reachable from the trees). But I don't think that write-tree
> actually freshens them (it sees "oh, we already have these; there is
> nothing to write").

OK, here is what I have queued.

-- >8 --
Subject: cache-tree: make sure to "touch" tree objects the cache-tree records

The cache_tree_fully_valid() function is called by callers that want
to know if they need to call cache_tree_update(), i.e. as an attempt
to optimize. They all want to have a fully valid cache-tree in the
end so that they can write a tree object out.

We used to check if the cached tree object is up-to-date (i.e. the
index entires covered by the cache-tree entry hasn't been changed
since the roll-up hash was computed for the cache-tree entry) and
made sure the tree object is already in the object store.  Since the
top-level tree we would soon be asked to write out (and would find
in the object store) may not be anchored to any refs or commits
immediately, freshen the tree object when it happens.

Similarly, when we actually compute the cache-tree entries in
cache_tree_update(), we refrained from writing a tree object out
when it already exists in the object store.  We would want to
freshen the tree object in that case to protect it from premature
pruning.

Strictly speaking, freshing these tree objects at each and every
level is probably unnecessary, given that anything reachable from a
young object inherits the youth from the referring object to be
protected from pruning.  It should be sufficient to freshen only the
very top-level tree instead.  Benchmarking and optimization is left
as an exercise for later days.

Signed-off-by: Junio C Hamano 
---
 cache-tree.c | 6 +++---
 cache.h  | 1 +
 sha1_file.c  | 9 +++--
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 3ebf9c3aa4..c8c74a1e07 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -225,7 +225,7 @@ int cache_tree_fully_valid(struct cache_tree *it)
int i;
if (!it)
return 0;
-   if (it->entry_count < 0 || !has_sha1_file(it->sha1))
+   if (it->entry_count < 0 || !freshen_object(it->sha1))
return 0;
for (i = 0; i < it->subtree_nr; i++) {
if (!cache_tree_fully_valid(it->down[i]->cache_tree))
@@ -253,7 +253,7 @@ static int update_one(struct cache_tree *it,
 
*skip_count = 0;
 
-   if (0 <= it->entry_count && has_sha1_file(it->sha1))
+   if (0 <= it->entry_count && freshen_object(it->sha1))
return it->entry_count;
 
/*
@@ -393,7 +393,7 @@ static int update_one(struct cache_tree *it,
if (repair) {
unsigned char sha1[20];
hash_sha1_file(buffer.buf, buffer.len, tree_type, sha1);
-   if (has_sha1_file(sha1))
+   if (freshen_object(sha1))
hashcpy(it->sha1, sha1);
else
to_invalidate = 1;
diff --git a/cache.h b/cache.h
index 4ff196c259..72c0b321ac 100644
--- a/cache.h
+++ b/cache.h
@@ -1077,6 +1077,7 @@ extern int sha1_object_info(const unsigned char *, 
unsigned long *);
 extern int hash_sha1_file(const void *buf, unsigned long len, const char 
*type, unsigned char *sha1);
 extern int write_sha1_file(const void *buf, unsigned long len, const char 
*type, unsigned char *return_sha1);
 extern int hash_sha1_file_literally(const void *buf, unsigned long len, const 
char *type, unsigned char *sha1, unsigned flags);
+extern int freshen_object(const unsigned char *);
 extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned 
char *);
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);
 extern int git_open_noatime(const char *name);
diff --git a/sha1_file.c b/sha1_file.c
index d0f2aa029b..9acce3d3b8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3151,6 +3151,11 @@ static int freshen_packed_object(const unsigned char 
*sha1)
return 1;
 }
 
+int freshen_object(const unsigned char *sha1)
+{
+   return freshen_packed_object(sha1) || freshen_loose_object(sha1);
+}
+
 int write_sha1_file(const void *buf, unsigned long len, const char *type, 
unsigned char *sha1)
 {
char hdr[32];
@@ -3160,7 +3165,7 @@ int write_sha1_file(const void *buf, unsigned long len, 
const char *type, unsign
 * it out into .git/objects/??/?{38} file.
 */
write_sha1_file_prepare(buf, len, type, sha1, hdr, );
-   if (freshen_packed_object(sha1) || 

Re: Protecting old temporary objects being reused from concurrent "git gc"?

[Jeff King]

On Wed, Nov 16, 2016 at 10:58:30AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I suspect the issue is that read-tree populates the cache-tree index
> > extension, and then write-tree omits the object write before it even
> > gets to write_sha1_file().
> 
> Wait a minute.  The entries in the index and trees in the cache-tree
> are root of "still in use" traversal for the purpose of pruning,
> which makes the "something like this" patch unnecessary for the real
> index file.
> 
> And for temporary index files that is kept for 6 months, touching
> tree objects that cache-tree references is irrelevant---the blobs
> recorded in the "list of objects" part of the index will go stale,
> which is a lot more problematic.

I think the case that is helped here is somebody who runs "git
write-tree" and expects that the timestamp on those trees is fresh. So
even more a briefly used index, like:

  export GIT_INDEX_FILE=/tmp/foo
  git read-tree ...
  git write-tree
  rm -f $GIT_INDEX_FILE

we'd expect that a "git gc" which runs immediately after would see those
trees as recent and avoid pruning them (and transitively, any blobs that
are reachable from the trees). But I don't think that write-tree
actually freshens them (it sees "oh, we already have these; there is
nothing to write").

I could actually see an argument that the read-tree operation should
freshen the blobs themselves (because we know those blobs are now in
active use, and probably shouldn't be pruned), but I am not sure I agree
there. If only because it is weird that an operation which is otherwise
read-only with respect to the repository would modify the object
database.

-Peff

Re: Rebasing cascading topic trees

[Norbert Kiesel]

Yes, `git rebase --onto topic1 topic1@{1} topic2` is the answer!

Thanks so much, learned something new today.

On Wed, Nov 16, 2016 at 3:44 PM, Junio C Hamano  wrote:
> Norbert Kiesel  writes:
>
>> I currently have a situation with cascading topic branches that I need to 
>> rebase
>> regularly.  In the picture below, I want to rebase the tree starting with 
>> `E` to
>> be rebased onto master (my actually cascade is 4 branches deep).
>>
>> A--B--C--D (master)
>>\
>> E--F (topic1)
>>\
>> G--H (topic2)
>>
>> After running `git rebase --onto master master topic1`, I end up with
>>
>> A--B--C--D (master)
>>| \
>>\  E'--F' (topic1)
>> E--F
>>\
>> G--H (topic2)
>>
>> I then need to also run `git rebase --onto topic1 F topic2` to arrive at the
>> desired
>>
>> A--B--C--D (master)
>>| \
>>\  E'--F' (topic1)
>> E--F  \
>>|   G'--H' (topic2)
>>\
>> G--H
>>
>> Problem here is that I don't have a nice symbolic name for `F` anymore after 
>> the
>> first rebase. Rebasing `topic2` first is not really possible, because I do 
>> not
>> have a new graft-point yet.  I currently write down `F` ahead of time (or use
>> `reflog` if I forgot) `F`, but I wonder if there is a better solution.
>
> Doesn't topic1@{1} point at "F" after the rebase of the topic1
> finishes?
>

Re: [PATCH v15 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

[Stephan Beyer]

Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index d84ba86..c542e8b 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -123,13 +123,40 @@ static int bisect_reset(const char *commit)
>   return bisect_clean_state();
>  }
>  
> +static int is_expected_rev(const char *expected_hex)
> +{
> + struct strbuf actual_hex = STRBUF_INIT;
> + int res = 0;
> + if (strbuf_read_file(_hex, git_path_bisect_expected_rev(), 0) >= 
> 40) {
> + strbuf_trim(_hex);
> + res = !strcmp(actual_hex.buf, expected_hex);
> + }
> + strbuf_release(_hex);
> + return res;
> +}

I am not sure it does what it should.

I would expect the following behavior from this function:
 - file does not exist (or is "broken") => return 0
 - actual_hex != expected_hex => return 0
 - otherwise return 1

If I am not wrong, the code does the following instead:
 - file does not exist (or is "broken") => return 0
 - actual_hex != expected_hex => return 1
 - otherwise => return 0

> +static int check_expected_revs(const char **revs, int rev_nr)
> +{
> + int i;
> +
> + for (i = 0; i < rev_nr; i++) {
> + if (!is_expected_rev(revs[i])) {
> + unlink_or_warn(git_path_bisect_ancestors_ok());
> + unlink_or_warn(git_path_bisect_expected_rev());
> + return 0;
> + }
> + }
> + return 0;
> +}

Here I am not sure what the function *should* do. However, I see that it
basically mimics the behavior of the shell function (assuming
is_expected_rev() is implemented correctly).

I don't understand why the return value is int and not void. To avoid a
"return 0;" line when calling this function?

> @@ -167,6 +196,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
> char *prefix)
>   if (argc > 1)
>   die(_("--bisect-reset requires either zero or one 
> arguments"));
>   return bisect_reset(argc ? argv[0] : NULL);
> + case CHECK_EXPECTED_REVS:
> + return check_expected_revs(argv, argc);

I note that you check the correct number of arguments for some
subcommands and you do not check it for some other subcommands like this
one. (I don't care, I just want to mention it.)

>   default:
>   die("BUG: unknown subcommand '%d'", cmdmode);
>   }

~Stephan

Re: RFC: Enable delayed responses to Git clean/smudge filter requests

[Junio C Hamano]

Jakub Narębski  writes:

>> I intend to implement this feature only for the new long running filter
>> process protocol. OK with you?
>
> If I remember and understand it correctly, current version of long
> running process protocol processes files sequentially, one by one:
> git sends file to filter wholly, and receives response wholly.
>
> In the single-file filter case, git calls filter process as async
> task, in a separate thread, so that one thread feeds the filter,
> and main thread (I think?) reads from it, to avoid deadlocks.
>
> Couldn't something like this be done for long running filter process,
> via protocol extension?

My reading of the message you are responding to is that Lars means
doing so by "implement this feature".  Instead of returning the
filtered bytes, a new protocol lets his filter to say "No result yet
for you to process, ask me later".

Re: Rebasing cascading topic trees

[Junio C Hamano]

Norbert Kiesel  writes:

> I currently have a situation with cascading topic branches that I need to 
> rebase
> regularly.  In the picture below, I want to rebase the tree starting with `E` 
> to
> be rebased onto master (my actually cascade is 4 branches deep).
>
> A--B--C--D (master)
>\
> E--F (topic1)
>\
> G--H (topic2)
>
> After running `git rebase --onto master master topic1`, I end up with
>
> A--B--C--D (master)
>| \
>\  E'--F' (topic1)
> E--F
>\
> G--H (topic2)
>
> I then need to also run `git rebase --onto topic1 F topic2` to arrive at the
> desired
>
> A--B--C--D (master)
>| \
>\  E'--F' (topic1)
> E--F  \
>|   G'--H' (topic2)
>\
> G--H
>
> Problem here is that I don't have a nice symbolic name for `F` anymore after 
> the
> first rebase. Rebasing `topic2` first is not really possible, because I do not
> have a new graft-point yet.  I currently write down `F` ahead of time (or use
> `reflog` if I forgot) `F`, but I wonder if there is a better solution.

Doesn't topic1@{1} point at "F" after the rebase of the topic1
finishes?

Rebasing cascading topic trees

[Norbert Kiesel]

I currently have a situation with cascading topic branches that I need to rebase
regularly.  In the picture below, I want to rebase the tree starting with `E` to
be rebased onto master (my actually cascade is 4 branches deep).

A--B--C--D (master)
   \
E--F (topic1)
   \
G--H (topic2)

After running `git rebase --onto master master topic1`, I end up with

A--B--C--D (master)
   | \
   \  E'--F' (topic1)
E--F
   \
G--H (topic2)

I then need to also run `git rebase --onto topic1 F topic2` to arrive at the
desired

A--B--C--D (master)
   | \
   \  E'--F' (topic1)
E--F  \
   |   G'--H' (topic2)
   \
G--H

Problem here is that I don't have a nice symbolic name for `F` anymore after the
first rebase. Rebasing `topic2` first is not really possible, because I do not
have a new graft-point yet.  I currently write down `F` ahead of time (or use
`reflog` if I forgot) `F`, but I wonder if there is a better solution.

[Bug?] git notes are not copied during rebase

[Norbert Kiesel]

Hi,

I am currently a heavy user of rebasing and noticed that my notes
don't get correctly applied, even if notes.rewrite.rebase is set
explicitly to true (though manual says that is the default).

Below is a use case that shows that a commit on a branch got rebased,
but the note was not copied to the new commit.

% mkdir notes
% cd notes
% git init
Initialized empty Git repository in /tmp/notes/.git/
% date
% git add a
% git commit -m c1
[master (root-commit) 2e24a91] c1
 1 file changed, 1 insertion(+)
 create mode 100644 a
% git checkout -b mybranch
Switched to a new branch 'mybranch'
% date
% git add b
% git commit -m c2
[mybranch 5ef9954] c2
 1 file changed, 1 insertion(+)
 create mode 100644 b
% git notes add -m note1
% git log
commit 5ef9954 (HEAD -> mybranch)
Author: Norbert Kiesel 
Date:   Mon Nov 14 15:48:00 2016 -0800

c2

Notes:
note1

commit 2e24a91 (master)
Author: Norbert Kiesel 
Date:   Mon Nov 14 15:48:00 2016 -0800

c1
% git notes
c39895a0948c17df2028f07c3ec0993a532edabf
5ef9954dbadddfccefe95277be5e7a995335124b
% git checkout master
Switched to branch 'master'
% date
% git commit -a -m c3
[master 1368832] c3
 1 file changed, 1 insertion(+)
% git rebase master mybranch
First, rewinding head to replay your work on top of it...
Applying: c2
% git log
commit 8921cb7 (HEAD -> mybranch)
Author: Norbert Kiesel 
Date:   Mon Nov 14 15:48:00 2016 -0800

c2

commit 1368832 (master)
Author: Norbert Kiesel 
Date:   Mon Nov 14 15:48:00 2016 -0800

c3

commit 2e24a91
Author: Norbert Kiesel 
Date:   Mon Nov 14 15:48:00 2016 -0800

c1
% git notes
c39895a0948c17df2028f07c3ec0993a532edabf
5ef9954dbadddfccefe95277be5e7a995335124b

Re: [PATCH v15 07/27] bisect--helper: `bisect_reset` shell function in C

[Stephan Beyer]

Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 4254d61..d84ba86 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -84,12 +89,47 @@ static int write_terms(const char *bad, const char *good)
>   return (res < 0) ? -1 : 0;
>  }
>  
> +static int bisect_reset(const char *commit)
> +{
> + struct strbuf branch = STRBUF_INIT;
> +
> + if (!commit) {
> + if (strbuf_read_file(, git_path_bisect_start(), 0) < 1) {
> + printf("We are not bisecting.\n");

I think this string should be marked for translation.

> + return 0;
> + }
> + strbuf_rtrim();
[...]
> @@ -121,6 +163,10 @@ int cmd_bisect__helper(int argc, const char **argv, 
> const char *prefix)
>   if (argc != 0)
>   die(_("--bisect-clean-state requires no arguments"));
>   return bisect_clean_state();
> + case BISECT_RESET:
> + if (argc > 1)
> + die(_("--bisect-reset requires either zero or one 
> arguments"));

This error message is imho a little unspecific (but this might not be an
issue because bisect--helper commands are not really exposed to the
user). Maybe "--bisect-reset requires either no argument or a commit."?

> + return bisect_reset(argc ? argv[0] : NULL);
>   default:
>   die("BUG: unknown subcommand '%d'", cmdmode);
>   }

~Stephan

Re: RFC: Enable delayed responses to Git clean/smudge filter requests

[Jakub Narębski]

W dniu 16.11.2016 o 10:53, Lars Schneider pisze:
> On 15 Nov 2016, at 19:03, Junio C Hamano  wrote:
>> Lars Schneider  writes:
>>
 The filter itself would need to be aware of parallelism
 if it lives for multiple objects, right?
>>>
>>> Correct. This way Git doesn't need to deal with threading...
[...]

>> * You'd need to rein in the maximum parallelism somehow, as you do
>>   not want to see hundreds of competing filter processes starting
>>   only to tell the main loop over an index with hundreds of entries
>>   that they are delayed checkouts.
> 
> I intend to implement this feature only for the new long running filter
> process protocol. OK with you?

If I remember and understand it correctly, current version of long
running process protocol processes files sequentially, one by one:
git sends file to filter wholly, and receives response wholly.

In the single-file filter case, git calls filter process as async
task, in a separate thread, so that one thread feeds the filter,
and main thread (I think?) reads from it, to avoid deadlocks.

Couldn't something like this be done for long running filter process,
via protocol extension?  Namely, Git would send in an async task
content to be filtered, and filter process would stream the response
back, in any order.  If it would be not enough, we could borrow
idea of channels, and be sending few files back concurrently in
parallel, as separate channels... though that would probably
require quite a bit of change in caller.

Best,
-- 
Jakub Narębski

What's cooking in git.git (Nov 2016, #03; Wed, 16)

[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.

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

--
[New Topics]

* dt/empty-submodule-in-merge (2016-11-12) 1 commit
 - submodules: allow empty working-tree dirs in merge/cherry-pick

 An empty directory in a working tree that can simply be nuked used
 to interfere while merging or cherry-picking a change to create a
 submodule directory there, which has been fixed..


* bw/grep-recurse-submodules (2016-11-14) 6 commits
 - grep: search history of moved submodules
 - grep: enable recurse-submodules to work on  objects
 - grep: optionally recurse into submodules
 - grep: add submodules as a grep source type
 - submodules: load gitmodules file from commit sha1
 - submodules: add helper functions to determine presence of submodules

 "git grep" learns to optionally recurse into submodules


* dt/smart-http-detect-server-going-away (2016-11-14) 2 commits
 - upload-pack: optionally allow fetching any sha1
 - remote-curl: don't hang when a server dies before any output

 When the http server gives an incomplete response to a smart-http
 rpc call, it could lead to client waiting for a full response that
 will never come.  Teach the client side to notice this condition
 and abort the transfer.

 An improvement counterproposal exists.
 cf. <20161114194049.mktpsvgdhex2f...@sigill.intra.peff.net>


* mm/push-social-engineering-attack-doc (2016-11-14) 1 commit
  (merged to 'next' on 2016-11-16 at b7c1b27563)
 + doc: mention transfer data leaks in more places

 Doc update on fetching and pushing.

 Will cook in 'next'.


* nd/worktree-lock (2016-11-13) 1 commit
  (merged to 'next' on 2016-11-16 at 67b731de07)
 + git-worktree.txt: fix typo "to"/"two", and add comma

 Typofix.

 Will merge to 'master'.


* nd/worktree-move (2016-11-12) 11 commits
 . worktree remove: new command
 . worktree move: refuse to move worktrees with submodules
 . worktree move: accept destination as directory
 . worktree move: new command
 . worktree.c: add update_worktree_location()
 . worktree.c: add validate_worktree()
 . copy.c: convert copy_file() to copy_dir_recursively()
 . copy.c: style fix
 . copy.c: convert bb_(p)error_msg to error(_errno)
 . copy.c: delete unused code in copy_file()
 . copy.c: import copy_file() from busybox

 "git worktree" learned move and remove subcommands.

 Seems to break a test or two.


* tk/diffcore-delta-remove-unused (2016-11-14) 1 commit
  (merged to 'next' on 2016-11-16 at 51e66c2fa7)
 + diffcore-delta: remove unused parameter to diffcore_count_changes()

 Code cleanup.

 Will merge to 'master'.


* jc/compression-config (2016-11-15) 1 commit
 - compression: unify pack.compression configuration parsing

 Compression setting for producing packfiles were spread across
 three codepaths, one of which did not honor any configuration.
 Unify these so that all of them honor core.compression and
 pack.compression variables the same way.

 Needs tests for pack-objects and fast-import.


* mm/gc-safety-doc (2016-11-16) 1 commit
 - git-gc.txt: expand discussion of races with other processes

 Doc update.

 Will merge to 'next'.

--
[Stalled]

* sb/push-make-submodule-check-the-default (2016-10-10) 2 commits
 - push: change submodule default to check when submodules exist
 - submodule add: extend force flag to add existing repos

 Turn the default of "push.recurseSubmodules" to "check" when
 submodules seem to be in use.

 Will hold to wait for hv/submodule-not-yet-pushed-fix


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

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

 While I think it would make it easier for people to experiment and
 build on if the topic is merged to 'next', I am at the same time a
 bit reluctant to merge an unproven new topic that introduces a new
 file format, which we may end up having to support til the end of
 time.  It is likely that to support a "prime clone from CDN", it
 would need a lot more than just "these are the heads and the pack
 data is over there", so this may not be sufficient.

 Will discard.


* mh/connect (2016-06-06) 10 commits
 - connect: [host:port] is legacy for ssh
 - connect: move ssh command line preparation to a separate function
 - connect: 

[PATCH] git-gui: Sort entries in optimized tclIndex

[Anders Kaseorg]

auto_mkindex expands wildcards in directory order, which depends on
the underlying filesystem.  To improve build reproducibility, sort the
list of *.tcl files in the Makefile.

The unoptimized loading case (7 lines below) was previously fixed in
v2.11.0-rc0~31^2^2~14 “git-gui: sort entries in tclIndex”.

Signed-off-by: Anders Kaseorg 
---
 git-gui/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-gui/Makefile b/git-gui/Makefile
index fe30be38d..f94b3e13d 100644
--- a/git-gui/Makefile
+++ b/git-gui/Makefile
@@ -252,7 +252,7 @@ $(ALL_MSGFILES): %.msg : %.po
 lib/tclIndex: $(ALL_LIBFILES) GIT-GUI-VARS
$(QUIET_INDEX)if echo \
  $(foreach p,$(PRELOAD_FILES),source $p\;) \
- auto_mkindex lib '*.tcl' \
+ auto_mkindex lib $(patsubst lib/%,%,$(sort $(ALL_LIBFILES))) \
| $(TCL_PATH) $(QUIET_2DEVNULL); then : ok; \
else \
 echo >&2 "* $(TCL_PATH) failed; using unoptimized loading"; \
-- 
2.11.0.rc0

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Junio C Hamano]

Johannes Schindelin  writes:

> This is the offending part from last night's build:
>
> -- snipsnap --
> 2016-11-16T00:31:57.5321220Z copy.c: In function 'copy_dir_1':
> 2016-11-16T00:31:57.5321220Z copy.c:369:8: error: implicit declaration of 
> function 'lchown' [-Werror=implicit-function-declaration]
> 2016-11-16T00:31:57.5321220Z if (lchown(dest, source_stat.st_uid, 
> source_stat.st_gid) < 0)
> 2016-11-16T00:31:57.5321220Z ^~
> 2016-11-16T00:31:57.5321220Z copy.c:391:7: error: implicit declaration of 
> function 'mknod' [-Werror=implicit-function-declaration]
> 2016-11-16T00:31:57.5321220Zif (mknod(dest, source_stat.st_mode, 
> source_stat.st_rdev) < 0)
> 2016-11-16T00:31:57.5321220Z^
> 2016-11-16T00:31:57.5321220Z copy.c:405:7: error: implicit declaration of 
> function 'utimes' [-Werror=implicit-function-declaration]
> 2016-11-16T00:31:57.5321220Zif (utimes(dest, times) < 0)
> 2016-11-16T00:31:57.5321220Z^~
> 2016-11-16T00:31:57.5321220Z copy.c:407:7: error: implicit declaration of 
> function 'chown' [-Werror=implicit-function-declaration]
> 2016-11-16T00:31:57.5321220Zif (chown(dest, source_stat.st_uid, 
> source_stat.st_gid) < 0) {
> 2016-11-16T00:31:57.5321220Z^
> 2016-11-16T00:31:57.7982432Z CC ctype.o
> 2016-11-16T00:31:58.1418929Z cc1.exe: all warnings being treated as errors
> 2016-11-16T00:31:58.6368128Z make: *** [Makefile:1988: copy.o] Error 1

That looks like a part of the new 'instead of run_command("cp -R"),
let's borrow code from somewhere and do that ourselves' in the
nd/worktree-move topic.

The offending part is this:

+   if (S_ISBLK(source_stat.st_mode) ||
+   S_ISCHR(source_stat.st_mode) ||
+   S_ISSOCK(source_stat.st_mode) ||
+   S_ISFIFO(source_stat.st_mode)) {
+   if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0)
+   return error_errno(_("can't create '%s'"), dest);
+   } else
+   return error(_("unrecognized file '%s' with mode %x"),
+source, source_stat.st_mode);

I think all of this is meant to be used to copy what is in the
working tree, and what is in the working tree is meant to be tracked
by Git, none of the four types that triggers mknod() would be
relevant for our purpose.  The simplest and cleanest would be to
make the above to return error("unsupported filetype").

I do not mind run_command("cp -R") and get rid of this code
altogether; that might be a more portable and sensible approach.

Re: [PATCH v4 4/4] submodule_needs_pushing() NEEDSWORK when we can not answer this question

[Heiko Voigt]

On Wed, Nov 16, 2016 at 11:18:07AM -0800, Junio C Hamano wrote:
> Heiko Voigt  writes:
> 
> > Signed-off-by: Heiko Voigt 
> > ---
> 
> Needs retitle ;-)  Here is what I tentatively queued.

Thanks ;-) Missed that one.

> submodule_needs_pushing(): explain the behaviour when we cannot answer
> 
> When we do not have commits that are involved in the update of the
> superproject in our copy of submodule, we cannot tell if the remote
> end needs to acquire these commits to be able to check out the
> superproject tree.  Explain why we answer "no there is no need/point
> in pushing from our submodule repository" in this case.
> 
> Signed-off-by: Heiko Voigt 
> Signed-off-by: Junio C Hamano 

Sound fine to me.

Cheers Heiko

git stash can recursively delete a directory with no warning

[Russell Yanofsky]

Using git 2.10.1, I recently lost the contents of an entire directory
by running a "git stash" command. I don't know if this known behavior,
but it seems pretty dangerous. To trigger the bug, all you have to do
is check out a repository containing a symlink, delete the symlink,
and then create a directory with files at the path where the deleted
symlink was. After this, running "git stash" will recursively delete
the directory, leaving no way to recover the data.

Here are minimal steps to reproduce:

mkdir test-repo
cd test-repo
git init
ln -s location symlink
git add symlink
git commit -m'add symlink'
rm symlink
mkdir symlink
echo important-data > symlink/important-data
git stash # recursively deletes entire contents of "symlink" directory

Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS

[Torsten Bögershausen]

On 16.11.16 15:39, Heiko Voigt wrote:
> On Tue, Nov 15, 2016 at 10:31:59AM -0500, Jeff King wrote:
>> On Tue, Nov 15, 2016 at 01:07:18PM +0100, Heiko Voigt wrote:
>>
>>> On Fri, Nov 11, 2016 at 09:22:51AM +0100, Lars Schneider wrote:
 To all macOS users on the list:
 Does anyone execute the tests with GIT_TEST_HTTPD enabled successfully?
>>>
>>> Nope. The following tests fail for me on master: 5539, 5540, 5541, 5542,
>>> 5550, 5551, 5561, 5812.
>>
>> Failing how? Does apache fail to start up? Do tests fails? What does
>> "-v" say? Is there anything interesting in httpd/error.log in the trash
>> directory?
> 
> This is what I see for 5539:
> 
> $ GIT_TEST_HTTPD=1 ./t5539-fetch-http-shallow.sh -v
> Initialized empty Git repository in /Users/hvoigt/Repository/git4/t/trash 
> directory.t5539-fetch-http-shallow/.git/
> checking prerequisite: NOT_ROOT
> 
> mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" &&
> (
>   cd "$TRASH_DIRECTORY/prereq-test-dir" &&
>   uid=$(id -u) &&
>   test "$uid" != 0
> 
> )
> prerequisite NOT_ROOT ok
> httpd: Syntax error on line 65 of 
> /Users/hvoigt/Repository/git4/t/lib-httpd/apache.conf: Cannot load 
> modules/mod_mpm_prefork.so into server: 
> dlopen(/Users/hvoigt/Repository/git4/t/trash 
> directory.t5539-fetch-http-shallow/httpd/modules/mod_mpm_prefork.so, 10): 
> image not found
> error: web server setup failed
> 
Yes, same here.

If we take that out:

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index c3e6313..1925fdb 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -61,9 +61,6 @@ LockFile accept.lock
 
LoadModule access_compat_module modules/mod_access_compat.so
 
-
-   LoadModule mpm_prefork_module modules/mod_mpm_prefork.so
-
 
LoadModule unixd_module modules/mod_unixd.so
 

I run into other issues:

 [core:emerg] [pid 2502] (2)No such file or directory: AH00023: Couldn't create 
the rewrite-map mutex (file /private/var/run/rewrite-map.2502)
AH00016: Configuration Failed

(apache2 comes via MacPorts)

Can I squash the merge created by "git subtree add"?

[Kannan Goundan]

When I do a "git subtree add", I get two commits.

df7e8f5 Merge commit '6de34775ea846c90e3f28e9e7fdfe690385c068b' as
'go/src/gopkg.in/ns1/ns1-go.v1'
6de3477 Squashed 'go/src/gopkg.in/ns1/ns1-go.v1/' content from
commit 1d343da

Unfortunately, in the environment I'm currently working in, merge
commits aren't allowed.

Is it safe to squash these two commits into a single commit?  Will
future "subtree" commands still work correctly?

Re: RFC: Enable delayed responses to Git clean/smudge filter requests

[Junio C Hamano]

Lars Schneider  writes:

>> On 16 Nov 2016, at 19:15, Junio C Hamano  wrote:
>> 
>> Lars Schneider  writes:
>> 
 * You'd need to rein in the maximum parallelism somehow, as you do
  not want to see hundreds of competing filter processes starting
  only to tell the main loop over an index with hundreds of entries
  that they are delayed checkouts.
>>> 
>>> I intend to implement this feature only for the new long running filter
>>> process protocol. OK with you?
>> 
>> Do you mean that a long-running filter process interacting with
>> convert_to_worktree() called from checkout_entry() will be the only
>> codepath that will spawn multiple processes or threads?  
>> 
>> That is fine, but it does not change the fact that you still need to
>> limit the maximum parallelism there.
>
> Filters using the long running protocol are spawned only once by Git. 
> The filter process would get all the smudge requests via the pipe
> protocol and is supposed to manage the parallelism on its own.

Yes, I think we are on the same page.  You need to be careful not to
let the filter process go berserk spawning too many threads or
processes.

Re: [PATCH v4 4/4] submodule_needs_pushing() NEEDSWORK when we can not answer this question

[Junio C Hamano]

Heiko Voigt  writes:

> Signed-off-by: Heiko Voigt 
> ---

Needs retitle ;-)  Here is what I tentatively queued.

submodule_needs_pushing(): explain the behaviour when we cannot answer

When we do not have commits that are involved in the update of the
superproject in our copy of submodule, we cannot tell if the remote
end needs to acquire these commits to be able to check out the
superproject tree.  Explain why we answer "no there is no need/point
in pushing from our submodule repository" in this case.

Signed-off-by: Heiko Voigt 
Signed-off-by: Junio C Hamano 

>  submodule.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index 11391fa..00dd655 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -531,6 +531,17 @@ static int submodule_has_commits(const char *path, 
> struct sha1_array *commits)
>  static int submodule_needs_pushing(const char *path, struct sha1_array 
> *commits)
>  {
>   if (!submodule_has_commits(path, commits))
> + /*
> +  * NOTE: We do consider it safe to return "no" here. The
> +  * correct answer would be "We do not know" instead of
> +  * "No push needed", but it is quite hard to change
> +  * the submodule pointer without having the submodule
> +  * around. If a user did however change the submodules
> +  * without having the submodule around, this indicates
> +  * an expert who knows what they are doing or a
> +  * maintainer integrating work from other people. In
> +  * both cases it should be safe to skip this check.
> +  */
>   return 0;
>  
>   if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {

Re: [PATCH 01/16] submodule.h: add extern keyword to functions, break line before 80

[Junio C Hamano]

Stefan Beller  writes:

> submodule.h: add extern keyword to functions, break line before 80

The former is probably a good change for consistency.  As the latter
change breaks a workflow around quickly checking the output from
"git grep funcname \*.h", I am not sure if it is a good idea.

Especially things like this look like a usability regression:

-void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
+extern void handle_ignore_submodules_arg(struct diff_options *diffopt,
+const char *);

Perhaps the name "diffopt" can be dropped from there if we want it
to be shorter.  Names in prototypes are valuable for parameters of
more generic types like "char *", especially when there are more
than one parameters of the same type, but in this case the typename
is specific enough for readers to tell what it is.

Re: [PATCH 02/16] submodule: modernize ok_to_remove_submodule to use argv_array

[Junio C Hamano]

David Turner  writes:

>> -"-u",
> ...
>> +argv_array_pushl(, "status", "--porcelain", "-uall",
>
> This also changes -u to -uall, which is not mentioned in the
> commit message.  That should probably be called out.

Or not making that change at all.  Isn't "-u" the same as "-uall"?

Re: Protecting old temporary objects being reused from concurrent "git gc"?

[Junio C Hamano]

Jeff King  writes:

> I suspect the issue is that read-tree populates the cache-tree index
> extension, and then write-tree omits the object write before it even
> gets to write_sha1_file().

Wait a minute.  The entries in the index and trees in the cache-tree
are root of "still in use" traversal for the purpose of pruning,
which makes the "something like this" patch unnecessary for the real
index file.

And for temporary index files that is kept for 6 months, touching
tree objects that cache-tree references is irrelevant---the blobs
recorded in the "list of objects" part of the index will go stale,
which is a lot more problematic.

Re: RFC: Enable delayed responses to Git clean/smudge filter requests

[Lars Schneider]

> On 16 Nov 2016, at 19:15, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>>> * You'd need to rein in the maximum parallelism somehow, as you do
>>>  not want to see hundreds of competing filter processes starting
>>>  only to tell the main loop over an index with hundreds of entries
>>>  that they are delayed checkouts.
>> 
>> I intend to implement this feature only for the new long running filter
>> process protocol. OK with you?
> 
> Do you mean that a long-running filter process interacting with
> convert_to_worktree() called from checkout_entry() will be the only
> codepath that will spawn multiple processes or threads?  
> 
> That is fine, but it does not change the fact that you still need to
> limit the maximum parallelism there.

Filters using the long running protocol are spawned only once by Git. 
The filter process would get all the smudge requests via the pipe
protocol and is supposed to manage the parallelism on its own.

- Lars

Re: [PATCH 00/11] git worktree (re)move

[Junio C Hamano]

Junio C Hamano  writes:

> Duy Nguyen  writes:
>
>> The following patch should fix it if that's the same thing you saw. I
>> could pile it on worktree-move series, or you can make it a separate
>> one-patch series. What's your preference?
>
> Giving a stable output to the users is probably a good preparatory
> fix to what is already in the released versions, so it would make
> the most sense to make it a separate patch to be applied to maint
> then build the remainder on top.
>
> I do not think "always show the primary first" is necessarily a good
> idea (I would have expected an output more like "git branch --list").

Yikes, "worktree list" is documented like so:

List details of each worktree. The main worktree is listed
first, followed by each of the linked worktrees.

If the primary workree is somehow missing, it still should be listed
first as missing---otherwise the readers of --porcelain readers will
have hard time telling what is going on.

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Junio C Hamano]

Junio C Hamano  writes:

> Johannes Schindelin  writes:
>
>> On Mon, 14 Nov 2016, Junio C Hamano wrote:
>>
>>> I _think_ the no-index mode was primarily for those who want to use
>>> our diff as a replacement for GNU and other diffs, and from that
>>> point of view, I'd favour not doing the "comparing symbolic link?
>>> We'll show the difference between the link contents, not target"
>>> under no-index mode myself.
>>
>> If I read this correctly,...
>
> Now I re-read it and I can see it can be read either way.
>
> By "link contents" in "comparing symbolic link? We'll show the
> difference between the link contents, not target", I meant the
> result you get from readlink(2), which will result in
>
> diff --git a/RelNotes b/RelNotes
> index c02235fe8c..b54330f7cd 12
> --- a/RelNotes
> +++ b/RelNotes
> @@ -1 +1 @@
> -Documentation/RelNotes/2.10.2.txt
> \ No newline at end of file
> +Documentation/RelNotes/2.11.0.txt
> \ No newline at end of file
>
> not the comparison between the files that are link targets,
> i.e. hypothetical
>
> diff --git a/RelNotes b/RelNotes
> index c4d4397023..7a1fce7720 100644
> --- a/Documentation/RelNotes/2.10.2.txt
> +++ b/Documentation/RelNotes/2.11.0.txt
> @@ -1,41 +1,402 @@
> -Git v2.10.2 Release Notes
> -=
> +Git 2.11 Release Notes
> ...
>
> And I'd favour *NOT* doing that if we are using our diff as a

Again, this can be read both ways.  By "that" on the above line I
meant "the former".

> replacement for GNU and other diffs in "no-index" mode.  Which leads
> to ...
>
>>> That is a lot closer to the diff other people implemented, not ours.
>>> Hence the knee-jerk reaction I gave in
>>> 
>>> http://public-inbox.org/git/xmqqinrt1zcx@gitster.mtv.corp.google.com
>
> ... this conclusion, which is consistent with ...
>
>>
>> Let me quote the knee-jerk reaction:
>>
>>> My knee-jerk reaction is:
>>>
>>>  * The --no-index mode should default to your --follow-symlinks
>>>behaviour, without any option to turn it on or off.
>
> ... this one.
>
> But notice "I _think_" in the first sentence you quoted.  That is a
> basic assumption that leads to the conclusion, and that assumption
> is not a fact.  Maybe users do *not* want the "no-index" mode as a
> replacement for GNU and other diffs, in which case comparing the
> result of readlink(2) even in no-index mode might have merit.  I
> just didn't think it was the case.

And "I just didn't think it was the case", when fully spelt out, is
"I just didn't think that the assumption was incorrect."

[ANNOUNCE] Git Rev News edition 20

[Christian Couder]

Hi everyone,

I'm happy announce that the 21th edition of Git Rev News is now published:

https://git.github.io/rev_news/2016/11/16/edition-21/

Thanks a lot to all the contributors and helpers, especially Jacob,
Dscho, Markus, Gábor and Peff!

Enjoy,
Christian, Thomas and Jakub.

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Junio C Hamano]

Johannes Schindelin  writes:

> On Mon, 14 Nov 2016, Junio C Hamano wrote:
>
>> I _think_ the no-index mode was primarily for those who want to use
>> our diff as a replacement for GNU and other diffs, and from that
>> point of view, I'd favour not doing the "comparing symbolic link?
>> We'll show the difference between the link contents, not target"
>> under no-index mode myself.
>
> If I read this correctly,...

Now I re-read it and I can see it can be read either way.

By "link contents" in "comparing symbolic link? We'll show the
difference between the link contents, not target", I meant the
result you get from readlink(2), which will result in

diff --git a/RelNotes b/RelNotes
index c02235fe8c..b54330f7cd 12
--- a/RelNotes
+++ b/RelNotes
@@ -1 +1 @@
-Documentation/RelNotes/2.10.2.txt
\ No newline at end of file
+Documentation/RelNotes/2.11.0.txt
\ No newline at end of file

not the comparison between the files that are link targets,
i.e. hypothetical

diff --git a/RelNotes b/RelNotes
index c4d4397023..7a1fce7720 100644
--- a/Documentation/RelNotes/2.10.2.txt
+++ b/Documentation/RelNotes/2.11.0.txt
@@ -1,41 +1,402 @@
-Git v2.10.2 Release Notes
-=
+Git 2.11 Release Notes
...

And I'd favour *NOT* doing that if we are using our diff as a
replacement for GNU and other diffs in "no-index" mode.  Which leads
to ...

>> That is a lot closer to the diff other people implemented, not ours.
>> Hence the knee-jerk reaction I gave in
>> 
>> http://public-inbox.org/git/xmqqinrt1zcx@gitster.mtv.corp.google.com

... this conclusion, which is consistent with ...

>
> Let me quote the knee-jerk reaction:
>
>> My knee-jerk reaction is:
>>
>>  * The --no-index mode should default to your --follow-symlinks
>>behaviour, without any option to turn it on or off.

... this one.

But notice "I _think_" in the first sentence you quoted.  That is a
basic assumption that leads to the conclusion, and that assumption
is not a fact.  Maybe users do *not* want the "no-index" mode as a
replacement for GNU and other diffs, in which case comparing the
result of readlink(2) even in no-index mode might have merit.  I
just didn't think it was the case.

Re: Protecting old temporary objects being reused from concurrent "git gc"?

[Junio C Hamano]

Jeff King  writes:

> ... I notice there is
> a return very early on in update_one() when has_sha1_file() matches, and
> it seems like that would trigger in some interesting cases, too.

Yeah, I missed that.  It says "we were asked to update one
cache_tree that corresponds to this subdirectory, found that
hashes everything below has been rolled up and still valid, and we
already have the right tree object in the object store".

It can simply become freshen(), which is "do we have it in the
object store?" with a side effect of touching iff the answer is
"yes".

Re: RFC: Enable delayed responses to Git clean/smudge filter requests

[Junio C Hamano]

Lars Schneider  writes:

>> * You'd need to rein in the maximum parallelism somehow, as you do
>>   not want to see hundreds of competing filter processes starting
>>   only to tell the main loop over an index with hundreds of entries
>>   that they are delayed checkouts.
>
> I intend to implement this feature only for the new long running filter
> process protocol. OK with you?

Do you mean that a long-running filter process interacting with
convert_to_worktree() called from checkout_entry() will be the only
codepath that will spawn multiple processes or threads?  

That is fine, but it does not change the fact that you still need to
limit the maximum parallelism there.

Re: [PATCH 00/11] git worktree (re)move

[Junio C Hamano]

Duy Nguyen  writes:

> The following patch should fix it if that's the same thing you saw. I
> could pile it on worktree-move series, or you can make it a separate
> one-patch series. What's your preference?

Giving a stable output to the users is probably a good preparatory
fix to what is already in the released versions, so it would make
the most sense to make it a separate patch to be applied to maint
then build the remainder on top.

I do not think "always show the primary first" is necessarily a good
idea (I would have expected an output more like "git branch --list").

[PATCH] rev-parse: fix parent shorthands with --symbolic

[Jeff King]

The try_parent_shorthands() function shows each parent via
show_rev(). We pass the correct parent sha1, but our "name"
parameter still points at the original refname. So asking
for a regular rev-parse works fine (it prints the sha1s),
but asking for the symbolic name gives nonsense like:

$ git rev-parse --symbolic HEAD^-1
HEAD
^HEAD

which is always an empty set of commits. Asking for "^!" is
likewise broken, with the added bonus that its prints ^HEAD
for _each_ parent. And "^@" just prints HEAD repeatedly.

Arguably it would be correct to just pass NULL as the name
here, and always get the parent expressed as a sha1. The
"--symbolic" documentaton claims only "as close to the
original input as possible", and we certainly fallback to
sha1s where necessary. But it's pretty easy to generate a
symbolic name on the fly from the original.

Signed-off-by: Jeff King 
---

I noticed this because tig uses "--symbolic", and "tig 1234abcd^-1" does
not work without this patch.

I thought at first it might be a regression in the upcoming v2.11 from
8779351dd (revision: new rev^-n shorthand for rev^n..rev, 2016-09-27).
But nope, "^!" has been broken at least as far back as v1.6.6.3 (I
didn't check further).

So assuming it's too late in the -rc cycle for a non-regression fix,
this is probably post-v2.11 maint material.

 builtin/rev-parse.c  |  7 ++-
 t/t6101-rev-parse-parents.sh | 18 ++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index cfb0f1510..ff13e59e1 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -342,11 +342,16 @@ static int try_parent_shorthands(const char *arg)
for (parents = commit->parents, parent_number = 1;
 parents;
 parents = parents->next, parent_number++) {
+   char *name = NULL;
+
if (exclude_parent && parent_number != exclude_parent)
continue;
 
+   if (symbolic)
+   name = xstrfmt("%s^%d", arg, parent_number);
show_rev(include_parents ? NORMAL : REVERSED,
-parents->item->object.oid.hash, arg);
+parents->item->object.oid.hash, name);
+   free(name);
}
 
*dotdot = '^';
diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
index 64a9850e3..8c617981a 100755
--- a/t/t6101-rev-parse-parents.sh
+++ b/t/t6101-rev-parse-parents.sh
@@ -83,12 +83,24 @@ test_expect_success 'final^1^@ = final^1^1 final^1^2' '
test_cmp expect actual
 '
 
+test_expect_success 'symbolic final^1^@ = final^1^1 final^1^2' '
+   git rev-parse --symbolic final^1^1 final^1^2 >expect &&
+   git rev-parse --symbolic final^1^@ >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'final^1^! = final^1 ^final^1^1 ^final^1^2' '
git rev-parse final^1 ^final^1^1 ^final^1^2 >expect &&
git rev-parse final^1^! >actual &&
test_cmp expect actual
 '
 
+test_expect_success 'symbolic final^1^! = final^1 ^final^1^1 ^final^1^2' '
+   git rev-parse --symbolic final^1 ^final^1^1 ^final^1^2 >expect &&
+   git rev-parse --symbolic final^1^! >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'large graft octopus' '
test_cmp_rev_output b31 "git rev-parse --verify b1^30"
 '
@@ -143,6 +155,12 @@ test_expect_success 'rev-parse merge^-2 = merge^2..merge' '
test_cmp expect actual
 '
 
+test_expect_success 'symbolic merge^-1 = merge^1..merge' '
+   git rev-parse --symbolic merge^1..merge >expect &&
+   git rev-parse --symbolic merge^-1 >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'rev-parse merge^-0 (invalid parent)' '
test_must_fail git rev-parse merge^-0
 '
-- 
2.11.0.rc1.280.gf9bb6f9

Re: Protecting old temporary objects being reused from concurrent "git gc"?

[Jeff King]

On Tue, Nov 15, 2016 at 12:01:35PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I suspect the issue is that read-tree populates the cache-tree index
> > extension, and then write-tree omits the object write before it even
> > gets to write_sha1_file(). The solution is that it should probably be
> > calling one of the freshen() functions (possibly just replacing
> > has_sha1_file() with check_and_freshen(), but I haven't looked).
> 
> I think the final writing always happens via write_sha1_file(), but
> an earlier cache-tree update that says "if we have a tree object
> already, then use it, otherwise even though we know the object name
> for this subtree, do not record it in the cache-tree" codepath may
> decide to record the subtree's sha1 without refreshing the referent.
> 
> A fix may look like this.

Yeah, that's along the lines I was expecting, though I'm not familiar
enough with cache-tree to say whether it's sufficient. I notice there is
a return very early on in update_one() when has_sha1_file() matches, and
it seems like that would trigger in some interesting cases, too.

-Peff

Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C

[Pranit Bauva]

Hey Stephan,

On Wed, Nov 16, 2016 at 4:49 AM, Stephan Beyer  wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 6a5878c..1d3e17f 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -24,6 +27,8 @@ static const char * const git_bisect_helper_usage[] = {
>>   N_("git bisect--helper --bisect-check-and-set-terms  
>>  "),
>>   N_("git bisect--helper --bisect-next-check []  
>> >   N_("git bisect--helper --bisect-terms [--term-good | --term-old | 
>> --term-bad | --term-new]"),
>> + N_("git bisect--helper --bisect start [--term-{old,good}= 
>> --term-{new,bad}=]"
>> +   "[--no-checkout] [ 
>> [...]] [--] [...]"),
>
> Typo: "--bisect start" with space instead of "-"
>
>> @@ -403,6 +408,205 @@ static int bisect_terms(struct bisect_terms *terms, 
>> const char **argv, int argc)
>>   return 0;
>>  }
>>
>> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
>> + const char **argv, int argc)
>> +{
>> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
>> + int flags, pathspec_pos, retval = 0;
>> + struct string_list revs = STRING_LIST_INIT_DUP;
>> + struct string_list states = STRING_LIST_INIT_DUP;
>> + struct strbuf start_head = STRBUF_INIT;
>> + struct strbuf bisect_names = STRBUF_INIT;
>> + struct strbuf orig_args = STRBUF_INIT;
>> + const char *head;
>> + unsigned char sha1[20];
>> + FILE *fp = NULL;
>> + struct object_id oid;
>> +
>> + if (is_bare_repository())
>> + no_checkout = 1;
>> +
>> + for (i = 0; i < argc; i++) {
>> + if (!strcmp(argv[i], "--")) {
>> + has_double_dash = 1;
>> + break;
>> + }
>> + }
>> +
>> + for (i = 0; i < argc; i++) {
>> + const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
>> + const char *arg = argv[i];
>> + if (!strcmp(argv[i], "--")) {
>> + has_double_dash = 1;
>
> This is without effect since has_double_dash is already set to 1 by the
> loop above. I think you can remove this line.

True. I will remove this line.

>> + break;
>> + } else if (!strcmp(arg, "--no-checkout")) {
>> + no_checkout = 1;
>> + } else if (!strcmp(arg, "--term-good") ||
>> +  !strcmp(arg, "--term-old")) {
>> + must_write_terms = 1;
>> + terms->term_good = xstrdup(argv[++i]);
>> + } else if (skip_prefix(arg, "--term-good=", )) {
>> + must_write_terms = 1;
>> + terms->term_good = xstrdup(arg);
>> + } else if (skip_prefix(arg, "--term-old=", )) {
>> + must_write_terms = 1;
>> + terms->term_good = xstrdup(arg);
>
> I think you can join the last two branches:
>
> +   } else if (skip_prefix(arg, "--term-good=", ) ||
> +  skip_prefix(arg, "--term-old=", )) {
> +   must_write_terms = 1;
> +   terms->term_good = xstrdup(arg);
>
>> + } else if (!strcmp(arg, "--term-bad") ||
>> +  !strcmp(arg, "--term-new")) {
>> + must_write_terms = 1;
>> + terms->term_bad = xstrdup(argv[++i]);
>> + } else if (skip_prefix(arg, "--term-bad=", )) {
>> + must_write_terms = 1;
>> + terms->term_bad = xstrdup(arg);
>> + } else if (skip_prefix(arg, "--term-new=", )) {
>> + must_write_terms = 1;
>> + terms->term_good = xstrdup(arg);
>
> This has to be terms->term_bad = ...

My bad.

> Also, you can join the last two branches, again, ie,

Sure!

> +   } else if (skip_prefix(arg, "--term-bad=", ) ||
> +  skip_prefix(arg, "--term-new=", )) {
> +   must_write_terms = 1;
> +   terms->term_bad = xstrdup(arg);
>
>> + } else if (starts_with(arg, "--") &&
>> +  !one_of(arg, "--term-good", "--term-bad", NULL)) {
>> + die(_("unrecognised option: '%s'"), arg);
> [...]
>> + /*
>> +  * Verify HEAD
>> +  */
>> + head = resolve_ref_unsafe("HEAD", 0, sha1, );
>> + if (!head)
>> + if (get_sha1("HEAD", sha1))
>> + die(_("Bad HEAD - I need a HEAD"));
>> +
>> + if (!is_empty_or_missing_file(git_path_bisect_start())) {
>
> You were so eager to re-use the comments from the shell script, but you
> forgot the "Check if we are bisecting." comment above this line ;-)

I will add it back again.

>> + /* Reset to the rev from where we started */
>> +

RE: [PATCH 14/16] checkout: recurse into submodules if asked to

[David Turner]

Sorry, my previous message accidentally sent before I was done.  One more 
comment:

> -Original Message-
> From: Stefan Beller [mailto:sbel...@google.com]

> +test_expect_failure '"checkout --recurse-submodules" needs -f to update
> modifed submodule content' '
> + echo modified >submodule/second.t &&
> + test_must_fail git checkout --recurse-submodules HEAD^ &&
> + test_must_fail git diff-files --quiet submodule &&
> + git diff-files --quiet file &&
> + git checkout --recurse-submodules -f HEAD^ &&
> + git diff-files --quiet &&
> + git diff-index --quiet --cached HEAD &&
> + git checkout --recurse-submodules -f master &&
> + git diff-files --quiet &&
> + git diff-index --quiet --cached HEAD
> +'

It might be worth adding some comments explaining why you expect these to fail. 
 

RE: [PATCH 14/16] checkout: recurse into submodules if asked to

[David Turner]

> -Original Message-
> From: Stefan Beller [mailto:sbel...@google.com]
> 
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index
> 79cdd34..e0773c6 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -634,7 +634,13 @@ test_submodule_forced_switch () {
> 
>   ## Modified submodule
> #
>   # Updating a submodule sha1 doesn't update the submodule's work tree
> - test_expect_success "$command: modified submodule does not update
> submodule work tree" '
> + if test
> "$KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST" =
> 1
> + then
> + RESULT="failure"
> + else
> + RESULT="success"
> + fi
> + test_expect_$RESULT "$command: modified submodule does not update
> submodule work tree" '

Why does this break?  I thought it was only if checkout is run with 
--recurse-submodules that anything should change?

> +test_expect_success 'dirty file file is not deleted' '

Duplicate 'file' in this test name.
> +# This is ok in theory, we just need to make sure # the garbage
> +collection doesn't eat the commit.
> +test_expect_success 'different commit prevents from deleting' '

This isn't a different commit -- it's a dirty index, right? 

> +test_expect_failure '"checkout --recurse-submodules" does not care about
> untracked submodule content' '
> + echo untracked >submodule/untracked &&
> + git checkout --recurse-submodules master &&
> + git diff-files --quiet --ignore-submodules=untracked &&
> + git diff-index --quiet --cached HEAD &&
> + rm submodule/untracked
> +'

Use test_when_finished for cleanup.

> +test_expect_failure '"checkout --recurse-submodules" needs -f when
> submodule commit is not present (but does fail anyway)' '
> + git checkout --recurse-submodules -b bogus_commit master &&
> + git update-index --cacheinfo 16
> 0123456789012345678901234567890123456789 submodule &&
> + BOGUS_TREE=$(git write-tree) &&
> + BOGUS_COMMIT=$(echo "bogus submodule commit" | git commit-tree
> $BOGUS_TREE) &&
> + git commit -m "bogus submodule commit" &&
> + git checkout --recurse-submodules -f master &&
> + test_must_fail git checkout --recurse-submodules bogus_commit &&
> + git diff-files --quiet &&
> + test_must_fail git checkout --recurse-submodules -f bogus_commit &&
> + test_must_fail git diff-files --quiet submodule &&
> + git diff-files --quiet file &&
> + git diff-index --quiet --cached HEAD &&
> + git checkout --recurse-submodules -f master '
> +KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST=1
>  test_submodule_switch "git checkout"
> 
> +KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST=
>  test_submodule_forced_switch "git checkout -f"
> 
>  test_done
> --
> 2.10.1.469.g00a8914

Re: [PATCH v15 04/27] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

Hey Junio,

On Wed, Nov 16, 2016 at 3:10 AM, Junio C Hamano  wrote:
>
> Stephan Beyer  writes:
>
> >> +int bisect_clean_state(void)
> >> +{
> >> +int result = 0;
> >> +
> >> +/* There may be some refs packed during bisection */
> >> +struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
> >> +for_each_ref_in("refs/bisect", mark_for_removal, (void *) 
> >> _for_removal);
> >> +string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
> >> +result = delete_refs(_for_removal, REF_NODEREF);
> >> +refs_for_removal.strdup_strings = 1;
> >> +string_list_clear(_for_removal, 0);
> >
> > Does it have advantages to populate a list (with duplicated strings),
> > hand it to delete_refs(), and clear the list (and strings), instead of
> > just doing a single delete_ref() (or whatever name the singular function
> > has) in the callback?
>
> Depending on ref backends, removing multiple refs may be a lot more
> efficient than calling a single ref removal for the same set of
> refs, and the comment upfront I think hints that the code was
> written in the way exactly with that in mind.  Removing N refs from
> a packed refs file will involve a loop that runs N times, each
> iteration loading the file, locating an entry among possibly 100s of
> refs to remove, and then rewriting the file.
>
> Besides, it is bad taste to delete each individual item being
> iterated over in an interator in general, isn't it?
>

Not just that, deleting a ref inside for_each*() is illegal because it
builds some kind of index and that is spoiled if anything is deleted
in between. Thus it gives a seg fault. See this[1]. I did the same
mistake when making this patch and I was confused about that was
happening but then Michael Haggerty pointed this out[2].

[1]: https://github.com/git/git/blob/v2.11.0-rc1/refs.h#L183-L191
[2]: http://public-inbox.org/git/574d122f.7080...@alum.mit.edu/

Regards,
Pranit Bauva

Re: merge --no-ff is NOT mentioned in help

[Mike Rappazzo]

(Please reply inline)

On Wed, Nov 16, 2016 at 10:48 AM, Vanderhoof, Tzadik
 wrote:
> I am running:git version 2.10.1.windows.1
>
> I typed: git merge -h
>
> and got:
>
> usage: git merge [] [...]
>or: git merge []  HEAD 
>or: git merge --abort
>
> -ndo not show a diffstat at the end of the merge
> --statshow a diffstat at the end of the merge
> --summary (synonym to --stat)
> --log[=]   add (at most ) entries from shortlog to merge 
> commit message
> --squash  create a single commit instead of doing a merge
> --commit  perform a commit if the merge succeeds (default)
> -e, --editedit message before committing
> --ff  allow fast-forward (default)
> --ff-only abort if fast-forward is not possible
> --rerere-autoupdate   update the index with reused conflict resolution if 
> possible
> --verify-signatures   verify that the named commit has a valid GPG 
> signature
> -s, --strategy 
>   merge strategy to use
> -X, --strategy-option 

RE: merge --no-ff is NOT mentioned in help

[Vanderhoof, Tzadik]

I am running:git version 2.10.1.windows.1

I typed: git merge -h

and got:

usage: git merge [] [...]
   or: git merge []  HEAD 
   or: git merge --abort

-ndo not show a diffstat at the end of the merge
--statshow a diffstat at the end of the merge
--summary (synonym to --stat)
--log[=]   add (at most ) entries from shortlog to merge 
commit message
--squash  create a single commit instead of doing a merge
--commit  perform a commit if the merge succeeds (default)
-e, --editedit message before committing
--ff  allow fast-forward (default)
--ff-only abort if fast-forward is not possible
--rerere-autoupdate   update the index with reused conflict resolution if 
possible
--verify-signatures   verify that the named commit has a valid GPG signature
-s, --strategy 
  merge strategy to use
-X, --strategy-option 

Re: merge --no-ff is NOT mentioned in help

[Mike Rappazzo]

On Wed, Nov 16, 2016 at 10:16 AM, Vanderhoof, Tzadik
 wrote:
> When I do: "git merge -h"  to get help, the option "--no-ff" is left out of 
> the list of options.

I am running git version 2.10.0, and running git merge --help contains
these lines:

   --ff
   When the merge resolves as a fast-forward, only update the
branch pointer, without creating a merge commit. This is the default
behavior.

   --no-ff
   Create a merge commit even when the merge resolves as a
fast-forward. This is the default behaviour when merging an annotated
(and possibly signed) tag.

   --ff-only
   Refuse to merge and exit with a non-zero status unless the
current HEAD is already up-to-date or the merge can be resolved as a
fast-forward.

>
> This e-mail, including attachments, may include confidential and/or
> proprietary information, and may be used only by the person or entity
> to which it is addressed. If the reader of this e-mail is not the intended
> recipient or his or her authorized agent, the reader is hereby notified
> that any dissemination, distribution or copying of this e-mail is
> prohibited. If you have received this e-mail in error, please notify the
> sender by replying to this message and delete this e-mail immediately.
>

Re: [PATCH v7 00/17] port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Wed, Nov 16, 2016 at 2:13 AM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> This is part of unification of the commands 'git tag -l, git branch -l
>> and git for-each-ref'. This ports over branch.c to use ref-filter's
>> printing options.
>>
>> Karthik Nayak (17):
>>   ref-filter: implement %(if), %(then), and %(else) atoms
>>   ref-filter: include reference to 'used_atom' within 'atom_value'
>>   ref-filter: implement %(if:equals=) and
>> %(if:notequals=)
>>   ref-filter: modify "%(objectname:short)" to take length
>>   ref-filter: move get_head_description() from branch.c
>>   ref-filter: introduce format_ref_array_item()
>>   ref-filter: make %(upstream:track) prints "[gone]" for invalid
>> upstreams
>>   ref-filter: add support for %(upstream:track,nobracket)
>>   ref-filter: make "%(symref)" atom work with the ':short' modifier
>>   ref-filter: introduce refname_atom_parser_internal()
>>   ref-filter: introduce symref_atom_parser() and refname_atom_parser()
>>   ref-filter: make remote_ref_atom_parser() use
>> refname_atom_parser_internal()
>>   ref-filter: add `:dir` and `:base` options for ref printing atoms
>>   ref-filter: allow porcelain to translate messages in the output
>>   branch, tag: use porcelain output
>>   branch: use ref-filter printing APIs
>>   branch: implement '--format' option
>
> This is not a new issue, but --format='%(HEAD)' you stole from
> for-each-ref is broken when you are on an unborn branch, and the
> second patch from the tip makes "git branch" (no other args) on
> an unborn branch to segfault, when there are real branches that
> have commits.
>
> Something like this needs to go before that step.
>
>  ref-filter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 944671af5a..c71d7360d2 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1318,7 +1318,7 @@ static void populate_value(struct ref_array_item *ref)
>
> head = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
>   sha1, NULL);
> -   if (!strcmp(ref->refname, head))
> +   if (head && !strcmp(ref->refname, head))
> v->s = "*";
> else
> v->s = " ";
>
>

Thanks, will add it in.

-- 
Regards,
Karthik Nayak

merge --no-ff is NOT mentioned in help

[Vanderhoof, Tzadik]

When I do: "git merge -h"  to get help, the option "--no-ff" is left out of the 
list of options.

This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or his or her authorized agent, the reader is hereby notified
that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.

[PATCH v4 3/4] batch check whether submodule needs pushing into one call

[Heiko Voigt]

We run a command for each sha1 change in a submodule. This is
unnecessary since we can simply batch all sha1's we want to check into
one command. Lets do it so we can speedup the check when many submodule
changes are in need of checking.

Signed-off-by: Heiko Voigt 
---
 submodule.c | 62 -
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/submodule.c b/submodule.c
index 12ac1ea..11391fa 100644
--- a/submodule.c
+++ b/submodule.c
@@ -507,27 +507,49 @@ static int append_sha1_to_argv(const unsigned char 
sha1[20], void *data)
return 0;
 }
 
-static int submodule_needs_pushing(const char *path, const unsigned char 
sha1[20])
+static int check_has_commit(const unsigned char sha1[20], void *data)
 {
-   if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
+   int *has_commit = data;
+
+   if (!lookup_commit_reference(sha1))
+   *has_commit = 0;
+
+   return 0;
+}
+
+static int submodule_has_commits(const char *path, struct sha1_array *commits)
+{
+   int has_commit = 1;
+
+   if (add_submodule_odb(path))
+   return 0;
+
+   sha1_array_for_each_unique(commits, check_has_commit, _commit);
+   return has_commit;
+}
+
+static int submodule_needs_pushing(const char *path, struct sha1_array 
*commits)
+{
+   if (!submodule_has_commits(path, commits))
return 0;
 
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
struct child_process cp = CHILD_PROCESS_INIT;
-   const char *argv[] = {"rev-list", NULL, "--not", "--remotes", 
"-n", "1" , NULL};
struct strbuf buf = STRBUF_INIT;
int needs_pushing = 0;
 
-   argv[1] = sha1_to_hex(sha1);
-   cp.argv = argv;
+   argv_array_push(, "rev-list");
+   sha1_array_for_each_unique(commits, append_sha1_to_argv, 
);
+   argv_array_pushl(, "--not", "--remotes", "-n", "1" , 
NULL);
+
prepare_submodule_repo_env(_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
cp.dir = path;
if (start_command())
-   die("Could not run 'git rev-list %s --not --remotes -n 
1' command in submodule %s",
-   sha1_to_hex(sha1), path);
+   die("Could not run 'git rev-list  --not 
--remotes -n 1' command in submodule %s",
+   path);
if (strbuf_read(, cp.out, 41))
needs_pushing = 1;
finish_command();
@@ -582,22 +604,6 @@ static void find_unpushed_submodule_commits(struct commit 
*commit,
diff_tree_combined_merge(commit, 1, );
 }
 
-struct collect_submodule_from_sha1s_data {
-   char *submodule_path;
-   struct string_list *needs_pushing;
-};
-
-static int collect_submodules_from_sha1s(const unsigned char sha1[20],
-   void *data)
-{
-   struct collect_submodule_from_sha1s_data *me = data;
-
-   if (submodule_needs_pushing(me->submodule_path, sha1))
-   string_list_insert(me->needs_pushing, me->submodule_path);
-
-   return 0;
-}
-
 static void free_submodules_sha1s(struct string_list *submodules)
 {
struct string_list_item *item;
@@ -634,12 +640,10 @@ int find_unpushed_submodules(struct sha1_array *commits,
argv_array_clear();
 
for_each_string_list_item(submodule, ) {
-   struct collect_submodule_from_sha1s_data data;
-   data.submodule_path = submodule->string;
-   data.needs_pushing = needs_pushing;
-   sha1_array_for_each_unique((struct sha1_array *) 
submodule->util,
-   collect_submodules_from_sha1s,
-   );
+   struct sha1_array *commits = (struct sha1_array *) 
submodule->util;
+
+   if (submodule_needs_pushing(submodule->string, commits))
+   string_list_insert(needs_pushing, submodule->string);
}
free_submodules_sha1s();
 
-- 
2.10.1.386.gc503e45

[PATCH v4 2/4] serialize collection of refs that contain submodule changes

[Heiko Voigt]

We are iterating over each pushed ref and want to check whether it
contains changes to submodules. Instead of immediately checking each ref
lets first collect them and then do the check for all of them in one
revision walk.

Signed-off-by: Heiko Voigt 
---
 submodule.c | 35 ---
 submodule.h |  5 +++--
 transport.c | 29 +
 3 files changed, 44 insertions(+), 25 deletions(-)

diff --git a/submodule.c b/submodule.c
index b2908fe..12ac1ea 100644
--- a/submodule.c
+++ b/submodule.c
@@ -500,6 +500,13 @@ static int has_remote(const char *refname, const struct 
object_id *oid,
return 1;
 }
 
+static int append_sha1_to_argv(const unsigned char sha1[20], void *data)
+{
+   struct argv_array *argv = data;
+   argv_array_push(argv, sha1_to_hex(sha1));
+   return 0;
+}
+
 static int submodule_needs_pushing(const char *path, const unsigned char 
sha1[20])
 {
if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
@@ -599,25 +606,24 @@ static void free_submodules_sha1s(struct string_list 
*submodules)
string_list_clear(submodules, 1);
 }
 
-int find_unpushed_submodules(unsigned char new_sha1[20],
+int find_unpushed_submodules(struct sha1_array *commits,
const char *remotes_name, struct string_list *needs_pushing)
 {
struct rev_info rev;
struct commit *commit;
-   const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
-   int argc = ARRAY_SIZE(argv) - 1;
-   char *sha1_copy;
struct string_list submodules = STRING_LIST_INIT_DUP;
struct string_list_item *submodule;
+   struct argv_array argv = ARGV_ARRAY_INIT;
 
-   struct strbuf remotes_arg = STRBUF_INIT;
-
-   strbuf_addf(_arg, "--remotes=%s", remotes_name);
init_revisions(, NULL);
-   sha1_copy = xstrdup(sha1_to_hex(new_sha1));
-   argv[1] = sha1_copy;
-   argv[3] = remotes_arg.buf;
-   setup_revisions(argc, argv, , NULL);
+
+   /* argv.argv[0] will be ignored by setup_revisions */
+   argv_array_push(, "find_unpushed_submodules");
+   sha1_array_for_each_unique(commits, append_sha1_to_argv, );
+   argv_array_push(, "--not");
+   argv_array_pushf(, "--remotes=%s", remotes_name);
+
+   setup_revisions(argv.argc, argv.argv, , NULL);
if (prepare_revision_walk())
die("revision walk setup failed");
 
@@ -625,8 +631,7 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
find_unpushed_submodule_commits(commit, );
 
reset_revision_walk();
-   free(sha1_copy);
-   strbuf_release(_arg);
+   argv_array_clear();
 
for_each_string_list_item(submodule, ) {
struct collect_submodule_from_sha1s_data data;
@@ -663,12 +668,12 @@ static int push_submodule(const char *path)
return 1;
 }
 
-int push_unpushed_submodules(unsigned char new_sha1[20], const char 
*remotes_name)
+int push_unpushed_submodules(struct sha1_array *commits, const char 
*remotes_name)
 {
int i, ret = 1;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
 
-   if (!find_unpushed_submodules(new_sha1, remotes_name, _pushing))
+   if (!find_unpushed_submodules(commits, remotes_name, _pushing))
return 1;
 
for (i = 0; i < needs_pushing.nr; i++) {
diff --git a/submodule.h b/submodule.h
index d9e197a..9454806 100644
--- a/submodule.h
+++ b/submodule.h
@@ -3,6 +3,7 @@
 
 struct diff_options;
 struct argv_array;
+struct sha1_array;
 
 enum {
RECURSE_SUBMODULES_CHECK = -4,
@@ -62,9 +63,9 @@ int submodule_uses_gitfile(const char *path);
 int ok_to_remove_submodule(const char *path);
 int merge_submodule(unsigned char result[20], const char *path, const unsigned 
char base[20],
const unsigned char a[20], const unsigned char b[20], int 
search);
-int find_unpushed_submodules(unsigned char new_sha1[20], const char 
*remotes_name,
+int find_unpushed_submodules(struct sha1_array *commits, const char 
*remotes_name,
struct string_list *needs_pushing);
-int push_unpushed_submodules(unsigned char new_sha1[20], const char 
*remotes_name);
+int push_unpushed_submodules(struct sha1_array *commits, const char 
*remotes_name);
 void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
 int parallel_submodules(void);
 
diff --git a/transport.c b/transport.c
index d57e8de..f482869 100644
--- a/transport.c
+++ b/transport.c
@@ -949,23 +949,36 @@ int transport_push(struct transport *transport,
 
if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && 
!is_bare_repository()) {
struct ref *ref = remote_refs;
+   struct sha1_array commits = SHA1_ARRAY_INIT;
+
for (; ref; ref = ref->next)
-   if (!is_null_oid(>new_oid) &&
-   

[PATCH v4 4/4] submodule_needs_pushing() NEEDSWORK when we can not answer this question

[Heiko Voigt]

Signed-off-by: Heiko Voigt 
---
 submodule.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/submodule.c b/submodule.c
index 11391fa..00dd655 100644
--- a/submodule.c
+++ b/submodule.c
@@ -531,6 +531,17 @@ static int submodule_has_commits(const char *path, struct 
sha1_array *commits)
 static int submodule_needs_pushing(const char *path, struct sha1_array 
*commits)
 {
if (!submodule_has_commits(path, commits))
+   /*
+* NOTE: We do consider it safe to return "no" here. The
+* correct answer would be "We do not know" instead of
+* "No push needed", but it is quite hard to change
+* the submodule pointer without having the submodule
+* around. If a user did however change the submodules
+* without having the submodule around, this indicates
+* an expert who knows what they are doing or a
+* maintainer integrating work from other people. In
+* both cases it should be safe to skip this check.
+*/
return 0;
 
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
-- 
2.10.1.386.gc503e45

[PATCH v4 1/4] serialize collection of changed submodules

[Heiko Voigt]

To check whether a submodule needs to be pushed we need to collect all
changed submodules. Lets collect them first and then execute the
possibly expensive test whether certain revisions are already pushed
only once per submodule.

There is further potential for optimization since we can assemble one
command and only issued that instead of one call for each remote ref in
the submodule.

Signed-off-by: Heiko Voigt 
---
 submodule.c | 59 +++
 1 file changed, 55 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index 6f7d883..b2908fe 100644
--- a/submodule.c
+++ b/submodule.c
@@ -532,19 +532,34 @@ static int submodule_needs_pushing(const char *path, 
const unsigned char sha1[20
return 0;
 }
 
+static struct sha1_array *submodule_commits(struct string_list *submodules,
+   const char *path)
+{
+   struct string_list_item *item;
+
+   item = string_list_insert(submodules, path);
+   if (item->util)
+   return (struct sha1_array *) item->util;
+
+   /* NEEDSWORK: should we have sha1_array_init()? */
+   item->util = xcalloc(1, sizeof(struct sha1_array));
+   return (struct sha1_array *) item->util;
+}
+
 static void collect_submodules_from_diff(struct diff_queue_struct *q,
 struct diff_options *options,
 void *data)
 {
int i;
-   struct string_list *needs_pushing = data;
+   struct string_list *submodules = data;
 
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
+   struct sha1_array *commits;
if (!S_ISGITLINK(p->two->mode))
continue;
-   if (submodule_needs_pushing(p->two->path, p->two->oid.hash))
-   string_list_insert(needs_pushing, p->two->path);
+   commits = submodule_commits(submodules, p->two->path);
+   sha1_array_append(commits, p->two->oid.hash);
}
 }
 
@@ -560,6 +575,30 @@ static void find_unpushed_submodule_commits(struct commit 
*commit,
diff_tree_combined_merge(commit, 1, );
 }
 
+struct collect_submodule_from_sha1s_data {
+   char *submodule_path;
+   struct string_list *needs_pushing;
+};
+
+static int collect_submodules_from_sha1s(const unsigned char sha1[20],
+   void *data)
+{
+   struct collect_submodule_from_sha1s_data *me = data;
+
+   if (submodule_needs_pushing(me->submodule_path, sha1))
+   string_list_insert(me->needs_pushing, me->submodule_path);
+
+   return 0;
+}
+
+static void free_submodules_sha1s(struct string_list *submodules)
+{
+   struct string_list_item *item;
+   for_each_string_list_item(item, submodules)
+   sha1_array_clear((struct sha1_array *) item->util);
+   string_list_clear(submodules, 1);
+}
+
 int find_unpushed_submodules(unsigned char new_sha1[20],
const char *remotes_name, struct string_list *needs_pushing)
 {
@@ -568,6 +607,8 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
int argc = ARRAY_SIZE(argv) - 1;
char *sha1_copy;
+   struct string_list submodules = STRING_LIST_INIT_DUP;
+   struct string_list_item *submodule;
 
struct strbuf remotes_arg = STRBUF_INIT;
 
@@ -581,12 +622,22 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
die("revision walk setup failed");
 
while ((commit = get_revision()) != NULL)
-   find_unpushed_submodule_commits(commit, needs_pushing);
+   find_unpushed_submodule_commits(commit, );
 
reset_revision_walk();
free(sha1_copy);
strbuf_release(_arg);
 
+   for_each_string_list_item(submodule, ) {
+   struct collect_submodule_from_sha1s_data data;
+   data.submodule_path = submodule->string;
+   data.needs_pushing = needs_pushing;
+   sha1_array_for_each_unique((struct sha1_array *) 
submodule->util,
+   collect_submodules_from_sha1s,
+   );
+   }
+   free_submodules_sha1s();
+
return needs_pushing->nr;
 }
 
-- 
2.10.1.386.gc503e45

[PATCH v4 0/4] Speedup finding of unpushed submodules

[Heiko Voigt]

You can find the third iteration of this series here:

http://public-inbox.org/git/cover.1479221071.git.hvo...@hvoigt.net/

All comments from the last iteration should be addressed.

Cheers Heiko

Heiko Voigt (4):
  serialize collection of changed submodules
  serialize collection of refs that contain submodule changes
  batch check whether submodule needs pushing into one call
  submodule_needs_pushing() NEEDSWORK when we can not answer this
question

 submodule.c | 123 +++-
 submodule.h |   5 ++-
 transport.c |  29 ++
 3 files changed, 121 insertions(+), 36 deletions(-)

-- 
2.10.1.386.gc503e45

Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS

[Heiko Voigt]

On Tue, Nov 15, 2016 at 10:31:59AM -0500, Jeff King wrote:
> On Tue, Nov 15, 2016 at 01:07:18PM +0100, Heiko Voigt wrote:
> 
> > On Fri, Nov 11, 2016 at 09:22:51AM +0100, Lars Schneider wrote:
> > > To all macOS users on the list:
> > > Does anyone execute the tests with GIT_TEST_HTTPD enabled successfully?
> > 
> > Nope. The following tests fail for me on master: 5539, 5540, 5541, 5542,
> > 5550, 5551, 5561, 5812.
> 
> Failing how? Does apache fail to start up? Do tests fails? What does
> "-v" say? Is there anything interesting in httpd/error.log in the trash
> directory?

This is what I see for 5539:

$ GIT_TEST_HTTPD=1 ./t5539-fetch-http-shallow.sh -v
Initialized empty Git repository in /Users/hvoigt/Repository/git4/t/trash 
directory.t5539-fetch-http-shallow/.git/
checking prerequisite: NOT_ROOT

mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" &&
(
cd "$TRASH_DIRECTORY/prereq-test-dir" &&
uid=$(id -u) &&
test "$uid" != 0

)
prerequisite NOT_ROOT ok
httpd: Syntax error on line 65 of 
/Users/hvoigt/Repository/git4/t/lib-httpd/apache.conf: Cannot load 
modules/mod_mpm_prefork.so into server: 
dlopen(/Users/hvoigt/Repository/git4/t/trash 
directory.t5539-fetch-http-shallow/httpd/modules/mod_mpm_prefork.so, 10): image 
not found
error: web server setup failed


It seems the other failures have the same cause.

Cheers Heiko

Re: [PATCH v3 3/4] batch check whether submodule needs pushing into one call

[Heiko Voigt]

On Tue, Nov 15, 2016 at 02:28:31PM -0800, Stefan Beller wrote:
> On Tue, Nov 15, 2016 at 6:56 AM, Heiko Voigt  wrote:
> 
> > -static int submodule_needs_pushing(const char *path, const unsigned char 
> > sha1[20])
> > +static int check_has_commit(const unsigned char sha1[20], void *data)
> >  {
> > -   if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
> > +   int *has_commit = (int *) data;
> 
> nit: just as prior patches ;) void* can be cast implicitly.

Even though its just a nit: Will remove all the void casts. :)

Cheers Heiko

Re: [PATCH v3 4/4] submodule_needs_pushing() NEEDSWORK when we can not answer this question

[Heiko Voigt]

On Tue, Nov 15, 2016 at 04:13:51PM -0800, Junio C Hamano wrote:
> Stefan Beller  writes:
> 
> >> "We do not know" ...
> >
> > ... because there is no way to check for us as we don't have the
> > submodule commits.
> >
> > " We do consider it safe as no one in their sane mind would
> > have changed the submodule pointers without having the
> > submodule around. If a user did however change the submodules
> > without having the submodule commits around, this indicates an
> > expert who knows what they were doing."
> 
> I didn't think it through myself to arrive at such a conclusion, but
> to me the above sounds like a sensible reasoning [*1*].

I think you have a point here. If I rephrase it like this: "We do
consider it safe as no one in their sane mind *could* have changed the
submodule pointers without having the submodule around..."

Since its actually hard to create such a situation without the submodule
commit around I agree here.

> *1* My version was more like "we do not know if they would get into
> a situation where they do not have enough submodule commits if
> we pushed our superproject, but more importantly, we DO KNOW
> that it would not help an iota if we pushed our submodule to
> them, so there is no point stopping the push of superproject
> saying 'no, no, no, you must push the submodule first'".

Yes saying that would be wrong. I was rather suggesting that we tell the
user that we could not find the submodule commits to and that if he
wants to proceed he should either pass --recurse-submodules=no or
initialize the submodule.

But I think the above reasoning obsoletes my suggestion. I would adjust
the comment accordingly but still keep the patch so we have
documentation that this behavior is on purpose.

Cheers Heiko

Re: [RFH] limiting ref advertisements

[Duy Nguyen]

On Tue, Nov 15, 2016 at 4:21 AM, Jeff King  wrote:
> Thanks for responding to this.

Glad to help (or more precisely annoy you somewhat :D)

> I've been meaning to get back to it with
> some code experiments, but they keep getting bumped down in priority. So
> let me at least outline some of my thoughts, without code. :)
>
> I was hoping to avoid right-anchoring because it's expensive to find all
> of the right-anchored cases (assuming that ref storage is generally
> hierarchical, which it is now and probably will be for future backends).

Urgh.. I completely forgot about future refs backends. Yeah this would
kick both wildmatch and right-anchoring out of the window.

For the record I almost suggested BPF as well (to keep the server side
simple, but at the super high cost of client side). That would also go
out of the window the same way wildmatch and right-anchoring does.

>  But remember that these are "early capabilities", before the server
>  has spoken at all. So the client doesn't know if we can handle v2.
>  So we have to send _both_ (and v2-aware servers can ignore the v1).
>
>advertise-lookup-v1=master
>advertise-lookup-v2=master
>
>  But that's not quite enough. A v1 server won't look in refs/notes
>  at all. So we have to say that, too:
>
>advertise-lookup-v1=refs/notes/master
>
>  And of course the v1 server has no idea that this isn't necessary
>  if we already found refs/heads/master.

We discussed a bit about upgrading upload-pack version 1 to 2 in more
than one session: the first fetch just does v1 as normal, the server
returns v1 response to but also advertises that v2 is supported. The
client keeps this info and skips v1 and tries v2 right away in the
following fetches, falling back to v1 (new fetch session) if v2 is
unsupported. Can it work the same way here too?

I'm in favor of this option 2 (without trying to be absolute backward
compatible with older lookup versions) since it allows us to optimize
for common case and experiment a bit. Once we know better we can make
the next version that hopefully suits everybody.

>  So I think you really do need the client to be able to say "also
>  look at this pattern".

What about the order of patterns? Does it matter "this pattern" is in
the middle or the end of the pattern list? I suppose not, just
checking...

But does this call for the ability to remove a pattern from the
pattern list as well, as a way to narrow down the search scope and
avoid sending unwanted refs?

> Of course we do still want left-anchoring, too. Wildcards like
> "refs/heads/*" are always left-anchored. So I think we'd have two types,
> and a full request for
>
>   git fetch origin +refs/heads/*:refs/remotes/origin/* master:foo
>
> would look like:
>
>   (1) advertise-pattern-v1
>   (2) advertise-pattern=refs/notes/%s
>   (3) advertise-prefix=refs/heads
>   (4) advertise-lookup=master
>
> where the lines mean:
>
>   1. Use the standard v1 patterns (we could spell them out, but this
>  just saves bandwidth. In fact, it could just be implicit that v1
>  patterns are included, and we could skip this line).
>
>   2. This is for our fictional future version where the client knows
>  added refs/notes/* to its DWIM but the server hasn't yet.
>
>   3. Give me all of refs/heads/*
>
>   4. Look up "master" using the advertise patterns and give me the first
>  one you find.

Well.. it sounds good to me. But I would not trust myself on refs matters :D

> So given that we can omit (1), and that (2) is just an example for the
> future, it could look like:
>
>   advertise-prefix=refs/heads
>   advertise-lookup=master
>
> which is pretty reasonable. It's not _completely_ bulletproof in terms
> of backwards compatibility. The "v1" thing means the client can't insert
> a new pattern in the middle (remember they're ordered by priority).

OK so pattern order probably matters...

> So
> maybe it is better to spell them all out (one thing that makes me
> hesitate is that these will probably end up as URL parameters for the
> HTTP version, which means our URL can start to get a little long).
>
> Anyway. That's the direction I'm thinking. I haven't written the code
> yet. The trickiest thing will probably be that the server would want to
> avoid advertising the same ref twice via two mechanisms (or perhaps the
> client just be tolerant of duplicates; that relieves the server of any
> duplicate-storage requirements).

Thanks for sharing.
-- 
Duy

Re: [PATCH] worktree: fix a sparse 'Using plain integer as NULL pointer' warning

[Duy Nguyen]

On Wed, Nov 16, 2016 at 3:28 AM, Ramsay Jones
 wrote:
>
> Signed-off-by: Ramsay Jones 
> ---
>
> Hi Duy,
>
> If you need to re-roll your 'nd/worktree-move' branch, could you
> please squash this into the relevant patch [commit c49e92f5c
> ("worktree move: refuse to move worktrees with submodules", 12-11-2016)].

Will do, thanks (and it's also "thanks" for your other similar emails,
I just don't want to send a mail with just 'thanks' that adds nothing
else).

> Also, one of the new tests introduced by commit 31a8f3066 ("worktree move:
> new command", 12-11-2016), fails for me, thus:
>
>   $ ./t2028-worktree-move.sh -i -v
>   ...
>   --- expected  2016-11-15 20:22:50.647241458 +
>   +++ actual2016-11-15 20:22:50.647241458 +
>   @@ -1,3 +1,3 @@
>worktree /home/ramsay/git/t/trash directory.t2028-worktree-move
>   -worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/destination
>worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/elsewhere
>   +worktree /home/ramsay/git/t/trash directory.t2028-worktree-move/destination
>   not ok 12 - move worktree
>   #
>   # git worktree move source destination &&
>   # test_path_is_missing source &&
>   # git worktree list --porcelain | grep "^worktree" >actual &&
>   # cat <<-EOF >expected &&
>   # worktree $TRASH_DIRECTORY
>   # worktree $TRASH_DIRECTORY/destination
>   # worktree $TRASH_DIRECTORY/elsewhere
>   # EOF
>   # test_cmp expected actual &&
>   # git -C destination log --format=%s >actual2 &&
>   # echo init >expected2 &&
>   # test_cmp expected2 actual2
>   #
>   $
>
> Is there an expectation that the submodules will be listed in
> any particular order by 'git worktree list --porcelain' ?

I just sent a patch [1] to fix this before reading this mail. The
order so far has been determined by readdir() which is not great.

[1] 
https://public-inbox.org/git/CACsJy8DOT_4N_48UaoYK61G_8JUaXbEs7N=n24CH2q1GN=++5...@mail.gmail.com/T/#mfcf797219a1a143ed2ac45198015f19e82c70db2
-- 
Duy

Re: Git status takes too long- How to improve the performance of git

[Fredrik Gustafsson]

On Wed, Nov 16, 2016 at 05:13:57PM +0530, Renuka Pampana wrote:
> > On Tue, Nov 15, 2016 at 02:33:12AM -0700, ravalika wrote:
> > > It is an centralized server and git status takes too long
> >
> > A centralized server? How? git is designed to be runned locally. If
> > you're running git on a network file system, the performance will
> > suffer. Could you elaborate on how your environment is setup?
> >
> >
> We have setup main git repository in remote location on Linux server
> And created a git repository in local Linux server, as a reference for the
> remote git repository,
> And update the local git repository for every 15 min in local server
> 
> Users will be able to access the  local git repository through NFS

And each user will have their own copy of the repository locally on
their machine? That is having done a git clone?
> 
> All users will clone the git repository from remote project url  by using
> local git repo as reference
> 
>  For example : git clone --reference ~/gitcaches/reference user@drupal
> :/home/project/drupal.git
> 
> All the users have ssh credentials for the remote server

Why are you using --reference for a 8.9MB big clone?
> 
> 
> What is the best way to implement remote git repo and able to access the
> git repo from other location, without any performance glitches?
> Users should be able to access git repo from different servers and from
> different locations.

The best way is to have it locally cloned. Yes the initial clone will be
expensive but operations after that will be fairly smooth. You do not(!)
want to execute git on one machine and having the repository beeing on
an other machine (for example via a network file system, except git
clone, git fetch, git push, etc.).

> >
> > > How to improve the performance of git status
> > >
> > > Git repo details:
> > >
> > > Size of the .git folder is 8.9MB
> > > Number of commits approx 53838  (git rev-list HEAD --count)
> > > Number of branches -  330
> > > Number of files - 63883
> > > Working tree clone size is 4.3GB
> >
> > .git folder of 8.9 MEGABYTE and working tree of 4.3 GIGABYTE? Is this a
> > typo?
> >
> > All git related information is stored in .git directory of the working
> directory
>   It is 8.9M
> And size of the local workspace is 4.3G

Can you please elaborate on this? How can you store 8.9 MB of data that
will result in a 4.3 G workspace?

-- 
Fredrik Gustafsson

phone: +46 733-608274
e-mail: iv...@iveqy.com
website: http://www.iveqy.com

Re: [PATCH 00/11] git worktree (re)move

[Duy Nguyen]

On Wed, Nov 16, 2016 at 8:05 PM, Duy Nguyen  wrote:
> diff --git a/worktree.c b/worktree.c
> index f7869f8..fe92d6f 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -173,6 +173,13 @@ static void mark_current_worktree(struct worktree 
> **worktrees)
> free(git_dir);
>  }
>
> +static int compare_worktree(const void *a_, const void *b_)
> +{
> +   const struct worktree *const *a = a_;
> +   const struct worktree *const *b = b_;
> +   return fspathcmp((*a)->path, (*b)->path);
> +}
> +
>  struct worktree **get_worktrees(void)
>  {
> struct worktree **list = NULL;
> @@ -205,6 +212,11 @@ struct worktree **get_worktrees(void)
> ALLOC_GROW(list, counter + 1, alloc);
> list[counter] = NULL;
>
> +   /*
> +* don't sort the first item (main worktree), which will
> +* always be the first
> +*/

Urgh.. I should review my patches more carefully before sending out :(
The main worktree could be missing (failing to parse HEAD) so I need a
better trick than simply assuming the first item is the main worktree
here. Tests did not catch this, naturally..

> +   qsort(list + 1, counter - 1, sizeof(*list), compare_worktree);
> mark_current_worktree(list);
> return list;
>  }
-- 
Duy

Re: [PATCH 00/11] git worktree (re)move

[Duy Nguyen]

On Sat, Nov 12, 2016 at 06:53:44PM -0800, Junio C Hamano wrote:
> not ok 12 - move worktree
> # 
> # git worktree move source destination &&
> # test_path_is_missing source &&
> # git worktree list --porcelain | grep "^worktree" >actual &&
> # cat <<-EOF >expected &&
> # worktree $TRASH_DIRECTORY
> # worktree $TRASH_DIRECTORY/destination
> # worktree $TRASH_DIRECTORY/elsewhere
> # EOF
> # test_cmp expected actual &&
> # git -C destination log --format=%s >actual2 &&
> # echo init >expected2 &&
> # test_cmp expected2 actual2

I think I've seen this (i.e. 'expected' and 'actual' differ only in
the order of items) once after a rebase and ignored it, assuming
something was changed during the rebase that caused this.

The following patch should fix it if that's the same thing you saw. I
could pile it on worktree-move series, or you can make it a separate
one-patch series. What's your preference?

-- 8< --
Subject: [PATCH] worktree list: keep the list sorted

It makes it easier to write tests for. But it should also be good for
the user since locating a worktree by eye would be easier once they
notice this.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 worktree.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/worktree.c b/worktree.c
index f7869f8..fe92d6f 100644
--- a/worktree.c
+++ b/worktree.c
@@ -173,6 +173,13 @@ static void mark_current_worktree(struct worktree 
**worktrees)
free(git_dir);
 }
 
+static int compare_worktree(const void *a_, const void *b_)
+{
+   const struct worktree *const *a = a_;
+   const struct worktree *const *b = b_;
+   return fspathcmp((*a)->path, (*b)->path);
+}
+
 struct worktree **get_worktrees(void)
 {
struct worktree **list = NULL;
@@ -205,6 +212,11 @@ struct worktree **get_worktrees(void)
ALLOC_GROW(list, counter + 1, alloc);
list[counter] = NULL;
 
+   /*
+* don't sort the first item (main worktree), which will
+* always be the first
+*/
+   qsort(list + 1, counter - 1, sizeof(*list), compare_worktree);
mark_current_worktree(list);
return list;
 }
-- 
2.8.2.524.g6ff3d78

-- 8< --

Revert on this

["Akram F Ahmed"]

I have a lucrative crude oil proposal and will need your partnership. If you 
are willing to get involved let me know so I can give you further details on 
the project.

Thanks,
Akram F Ahmed

Re: RFC: Enable delayed responses to Git clean/smudge filter requests

[Lars Schneider]

On 15 Nov 2016, at 19:03, Junio C Hamano  wrote:

> Lars Schneider  writes:
> 
>>> The filter itself would need to be aware of parallelism
>>> if it lives for multiple objects, right?
>> 
>> Correct. This way Git doesn't need to deal with threading...
> 
> I think you need to be careful about three things (at least; there
> may be more):
> 
> * Codepaths that check out multiple cache entries do rely on the
>   order of checkout.  We checkout removals first to make room so
>   that creation of a path X can succeed if an existing path X/Y
>   that used to want to see X as a directory can succeed (see the
>   use of checkout_entry() by "git checkout", which does have two
>   separate loops to explicitly guarantee this), for example.  I
>   think "remove all and then create" you do not specifically have
>   to worry about with the proposed change, but you may need to
>   inspect and verify there aren't other kind of order dependency.

OK


> * Done naively, it will lead to unmaintainable code, like this:
> 
>   + struct list_of_cache_entries *list = ...;
> for (i = 0; i < active_nr; i++)
>   -checkout_entry(active_cache[i], state, NULL);
>   +if (checkout_entry(active_cache[i], state, NULL) == DELAYED)
>   +   add_cache_to_queue(, active_cache[i]);
>   + while (list) {
>   +wait_for_checkout_to_finish(*list);
>   +list = list->next;
>   + }
> 
>   I do not think we want to see such a rewrite all over the
>   codepaths.  It might be OK to add such a "these entries are known
>   to be delayed" list in struct checkout so that the above becomes
>   more like this:
> 
> for (i = 0; i < active_nr; i++)
>checkout_entry(active_cache[i], state, NULL);
>   + checkout_entry_finish(state);
> 
>   That is, addition of a single "some of the checkout_entry() calls
>   done so far might have been lazy, and I'll give them a chance to
>   clean up" might be palatable.  Anything more than that on the
>   caller side is not.

I haven't thought hard about the implementation, yet, but I'll try 
to stick to your suggestion and change as less code as possible on 
the caller sides.


> * You'd need to rein in the maximum parallelism somehow, as you do
>   not want to see hundreds of competing filter processes starting
>   only to tell the main loop over an index with hundreds of entries
>   that they are delayed checkouts.

I intend to implement this feature only for the new long running filter
process protocol. OK with you?


Thanks,
Lars

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Johannes Schindelin]

Hi Junio,

On Mon, 14 Nov 2016, Junio C Hamano wrote:

> I _think_ the no-index mode was primarily for those who want to use
> our diff as a replacement for GNU and other diffs, and from that
> point of view, I'd favour not doing the "comparing symbolic link?
> We'll show the difference between the link contents, not target"
> under no-index mode myself.

If I read this correctly, then we are in agreement that the default for
--no-index should be as it is right now, i.e. comparing symlink targets as
opposed to --follow-links.

> That is a lot closer to the diff other people implemented, not ours.
> Hence the knee-jerk reaction I gave in
> 
> http://public-inbox.org/git/xmqqinrt1zcx@gitster.mtv.corp.google.com

Let me quote the knee-jerk reaction:

> My knee-jerk reaction is:
>
>  * The --no-index mode should default to your --follow-symlinks
>behaviour, without any option to turn it on or off.

But this is the exact opposite of what I find reasonable.

Ciao,
Johannes

Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables

[Johannes Schindelin]

Hi,

On Tue, 15 Nov 2016, Johannes Schindelin wrote:

> On Mon, 14 Nov 2016, Junio C Hamano wrote:
> 
> > Dscho's mention of 'still times out' may be an indiciation that
> > something unspecified on 'pu' is not ready to be merged to 'next',
> > but blocking all of 'pu' with a blanket statement is not useful,
> > and that was where my response comes from.
> 
> Until the time when the test suite takes less than the insane three hours
> to run, I am afraid that a blanket statement on `pu` is the best I can do.

Well, I should add that the test suite does not take 3 hours to run for
`pu` these days.

It used to time out after four hours until two days ago (I think; I was a
bit too busy with other CI work to pay close attention to the constantly
failing `pu` job, with quite a few failing `next`s and even a couple of
failing `master`s thrown in).

As of two days ago, the test suite takes no time at all. The build already
fails (which makes me wonder why a couple of patch series I contributed
had such a hard time getting into `pu` when they compiled and tested
just fine, whereas some obviously non-building stuff gets into `pu`
already, makes no sense to me).

This is the offending part from last night's build:

-- snipsnap --
2016-11-16T00:31:57.5321220Z copy.c: In function 'copy_dir_1':
2016-11-16T00:31:57.5321220Z copy.c:369:8: error: implicit declaration of 
function 'lchown' [-Werror=implicit-function-declaration]
2016-11-16T00:31:57.5321220Z if (lchown(dest, source_stat.st_uid, 
source_stat.st_gid) < 0)
2016-11-16T00:31:57.5321220Z ^~
2016-11-16T00:31:57.5321220Z copy.c:391:7: error: implicit declaration of 
function 'mknod' [-Werror=implicit-function-declaration]
2016-11-16T00:31:57.5321220Zif (mknod(dest, source_stat.st_mode, 
source_stat.st_rdev) < 0)
2016-11-16T00:31:57.5321220Z^
2016-11-16T00:31:57.5321220Z copy.c:405:7: error: implicit declaration of 
function 'utimes' [-Werror=implicit-function-declaration]
2016-11-16T00:31:57.5321220Zif (utimes(dest, times) < 0)
2016-11-16T00:31:57.5321220Z^~
2016-11-16T00:31:57.5321220Z copy.c:407:7: error: implicit declaration of 
function 'chown' [-Werror=implicit-function-declaration]
2016-11-16T00:31:57.5321220Zif (chown(dest, source_stat.st_uid, 
source_stat.st_gid) < 0) {
2016-11-16T00:31:57.5321220Z^
2016-11-16T00:31:57.7982432Z CC ctype.o
2016-11-16T00:31:58.1418929Z cc1.exe: all warnings being treated as errors
2016-11-16T00:31:58.6368128Z make: *** [Makefile:1988: copy.o] Error 1

Re: gitweb html validation

[Raphaël Gertz]

Le 16.11.2016 01:05, Junio C Hamano a écrit :

Ralf Thielow  writes:


Only block level elements are
allowed to be inside form tags, according to
https://www.w3.org/2010/04/xhtml10-strict.html#elem_form
...
I think it's better to just move the -Tag outside of the
surrounding div?
Something like this perhaps, I didn't test it myself yet.


That sounds like a sensible update to me (no, I do not run gitweb
myself).  Is this the only  we have in the UI, or is it the
only one that is problematic?


There is an other form in the cgi line 4110 :
print $cgi->start_form(-method => "get", -action => $action) .
  "\n" .

But this one has a  inside.

The problem with projsearch I want to change is that the div is around 
the form without a container inside.


I agree with moving the  inside the form if it's 
a better option.


Best regards