Re: [ANNOUNCE] Git v2.16.0-rc0

[Jeff King]

On Thu, Jun 14, 2018 at 09:20:33PM -0700, Jonathan Nieder wrote:

> What we've switched to is a versioned interface.  By setting
> GIT_SSH_VARIANT=simple, you are asking Git to promise to pass exactly
>  options.  If Git has a new option it wants to pass (like the "-o
> SendEnv" thing) but can live without it, then it can avoid breaking
> your wrapper and continue to follow this new promise.
> 
> The trouble is that GIT_SSH_VARIANT=simple is too... simple.  You
> would like a variant that passes in [-p port] [-4] [-6] as well.  We
> didn't implement that because we didn't have the attention of any
> wrapper writer who wanted it; in absence of a potential user, we
> decided to wait for a user to propose the interface they want.  Now we
> can celebrate, since that day has come.

I'm not sure I'm celebrating. It seems like work for not much benefit.
I'd just as soon use VARIANT=ssh and deal with any fallouts if my script
does not behave exactly like openssh in all regards.

> How would you like your ssh variant to work?  Some possibilities to
> get the thought process going:
> 
>  A. Would you want to set a variable 'GIT_SSH_SUPPORTS_OPTIONS=p46'
> to inform Git about what options you support?

Not really. That just creates more work when I have to later use "op46"
to support "-o", even though my script already handles it fine (or
worse, since "-o" is open-ended).

>  B. Alternatively, what about a 'GIT_SSH_VARIANT=capabilities' variant
> that calls "your-ssh-variant --capabailities" to get a
> machine-readable list of capabilities it supports?

Not really. Now I have to implement this --capabilities thing. This is
literally a 10 line script.

>  C. Alternatively, would you like all parameters to come in on stdin,
> credential helper style?

That's even worse. ;)

>  D. Other ideas?

I really am happy just saying "look, my script is basically openssh, so
just assume that". If it breaks, it breaks, and I'll fix it.

The one thing I was left puzzled by is why "-G" didn't work in the first
place, since the script really does just pass through its options.
Poking around, I think the problem actually _is_ the old ssh version
thing. I have a new one on my workstation, but our CI boxes are jessie,
and have openssh 1:6.7p1-5+deb8u4. And that's where I was digging into
the failure.

So sorry for misleading you earlier. The wrapper script looks like it's
a red herring to some degree (I guess its name not being "ssh"
contributed, but then the -G detection failed).

> If you were using an old version of OpenSSH, this would be a reason to
> revive the old patch, but I'm tempted to stall longer just to get more
> use cases like this to come out of the woodwork.

So apparently I am using an old version. Just switching to use "-V"
seems like it might be another solution, then.

-Peff

[PATCH] unpack-trees: do not fail reset because of unmerged skipped entry

[Max Kirillov]

After modify/delete merge conflict happens in a file skipped by sparse
checkout, "git reset --merge", which implements the "--abort" actions, and
"git reset --hard" fail with message "Entry * not uptodate. Cannot update
sparse checkout." The reason is that the entry is verified in
apply_sparse_checkout() for being up-to-date even when it has a conflict.
Checking conflicted entry for being up-to-date is not performed in other
cases. One obvious reason to not check it is that it is already modified
by inserting conflict marks.

Fix by not checking conflicted entries before performing reset.
Also, add test case which verifies the issue is fixed.

Signed-off-by: Max Kirillov 
---
I have tried to use sparse-checkout for merging and cherrypicking, to save on IO
and disk space. It works, mostly, but there are issues here and there.
This one was low hanging, and also pretty annoying.

 t/t3035-merge-sparse.sh | 46 +
 unpack-trees.c  |  2 +-
 2 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100755 t/t3035-merge-sparse.sh

diff --git a/t/t3035-merge-sparse.sh b/t/t3035-merge-sparse.sh
new file mode 100755
index 00..c6b2b0b82a
--- /dev/null
+++ b/t/t3035-merge-sparse.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='merge with sparse files'
+
+. ./test-lib.sh
+
+# test_file $filename $content
+test_file () {
+   echo "$2" > "$1" &&
+   git add "$1"
+}
+
+# test_commit_this $message_and_tag
+test_commit_this () {
+   git commit -m "$1" &&
+   git tag "$1"
+}
+
+test_expect_success 'setup' '
+   test_file checked-out init &&
+   test_file modify_delete modify_delete_init &&
+   test_commit_this init &&
+   test_file modify_delete modify_delete_theirs &&
+   test_commit_this theirs &&
+   git reset --hard init &&
+   git rm modify_delete &&
+   test_commit_this ours &&
+   git config core.sparseCheckout true &&
+   echo "/checked-out" >.git/info/sparse-checkout &&
+   git reset --hard &&
+   ! git merge theirs
+'
+
+test_expect_success 'reset --hard works after the conflict' '
+   git reset --hard
+'
+
+test_expect_success 'setup: conflict back' '
+   ! git merge theirs
+'
+
+test_expect_success 'Merge abort works after the conflict' '
+   git merge --abort
+'
+
+test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index e73745051e..65ae0721a6 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -468,7 +468,7 @@ static int apply_sparse_checkout(struct index_state *istate,
 * also stat info may have lost after merged_entry() so calling
 * verify_uptodate() again may fail
 */
-   if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, 
o))
+   if (!(ce->ce_flags & CE_UPDATE) && !(ce->ce_flags & 
CE_CONFLICTED) && verify_uptodate_sparse(ce, o))
return -1;
ce->ce_flags |= CE_WT_REMOVE;
ce->ce_flags &= ~CE_UPDATE;
-- 
2.17.0.1185.g782057d875

RESEARCHERS

[Sarah Paige]

Greetings,
 
We are contracted probate researchers. This is an investigation about a late 
client with whom you share the same surname; your assistance will be greatly 
appreciated. Are you aware of any investment made by such a person with us? 
Please clarify,

EmaiL Reply to : research...@mail2consultant.com

 Sarah Paige,
For Research Firm.

BUG: sequencer.c:795: root commit without message -- when rewording root commit

[Todd Zullinger]

Hi Johannes,

I was splitting a repository tonight and ran 'rebase -i
--root' to reword the initial commit. Then git died with
'BUG: sequencer.c:795: root commit without message.'

A simple test case to show the failure:

-- >8 --
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 59c766540..bc5e228b8 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -971,6 +971,14 @@ test_expect_success 'rebase -i --root fixup root commit' '
test 0 = $(git cat-file commit HEAD | grep -c ^parent\ )
 '
 
+test_expect_success 'rebase -i --root reword root commit' '
+   test_when_finished "test_might_fail git rebase --abort" &&
+   git checkout -b reword-root-branch master &&
+   set_fake_editor &&
+   FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" git rebase -i 
--root &&
+   git show HEAD^ | grep "A changed"
+'
+
 test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on 
non-interactive rebase' '
git reset --hard &&
git checkout conflict-branch &&
-- >8 --

Not surprisingly (among the commits which changed between
2.17.1 and 2.18.0-rc2, at least), git bisect points to
21d0764c82 ("rebase -i --root: let the sequencer handle even
the initial part", 2018-05-04).  With luck, the fix will be
obvious to trained eyes and can be added before 2.18.0. :)

Thanks,

-- 
Todd
~~
All decent people live beyond their incomes nowadays, and those who
aren't respectable live beyond other peoples'.
-- Saki

Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

[Jeff King]

On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote:

> The Makefile tweak NO_ICONV is meant to allow Git to be built without
> iconv in case iconv is not installed or is otherwise dysfunctional.
> However, NO_ICONV's disabling of iconv is incomplete and can incorrectly
> allow "-liconv" to slip into the linker flags when NEEDS_LIBICONV is
> defined, which breaks the build when iconv is not installed.
> 
> On some platforms, iconv lives directly in libc, whereas, on others it
> resides in libiconv. For the latter case, NEEDS_LIBICONV instructs the
> Makefile to add "-liconv" to the linker flags. config.mak.uname
> automatically defines NEEDS_LIBICONV for platforms which require it.
> The adding of "-liconv" is done unconditionally, despite NO_ICONV.
> 
> Work around this problem by making NO_ICONV take precedence over
> NEEDS_LIBICONV.

Nicely explained.

We have OLD_ICONV, too, which should probably do nothing if NO_ICONV is
set. I think that works OK. We end up setting -DOLD_ICONV on the command
line, but that's only consider inside "#ifndef NO_ICONV" within the
code.

> This patch is extra noisy due to the indentation change. Viewing it with
> "git diff -w" helps. An alternative to re-indenting would have been to
> "undefine NEEDS_LIBICONV", however, 'undefine' was added to GNU make in
> 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided.

Yeah, with "-w" it looks pretty obviously correct.

-Peff

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Thu, Jun 14, 2018 at 11:55:22AM -0700, Jonathan Nieder wrote:

>> Do you mean that it doesn't pass "-G" through, or that when using old
>> versions of openssh that doesn't support "-G" the probing fails?
>
> It just doesn't pass "-G" through.

Thanks.

>> If the former, then detecting the wrapper as something other than
>> "ssh" is intended behavior (though we might want to change what that
>> something is, as discussed in the previous thread).  If the latter,
>> then this is https://crbug.com/git/7 which I consider to be a bug.
>
> I certainly see the argument that "well, if it doesn't do '-G' then it's
> not _really_ openssh". My counter to that is that we don't actually
> _care_ about -G (and never did before recently). It's just a proxy for
> "do we understand -p", which my script does understand. My wrapper might
> eventually break if we depend on new options (like "-o SendEnv")

Exactly: what we want to detect is not whether the script happens to
support the OpenSSH options we use today, but whether it is a thin
wrapper around OpenSSH that supports *all* options.

This wrapper definitely does not qualify.  As we start to use more
OpenSSH options, we are likely to break it.  As you say,

> the worst case there is generally no different before or after your
> patch: the command barfs.

but as a caller, Git has no way to know that: it is easily possible
that the wrapper would ignore the new option we want to pass, for
example.

In other words, I think your response is based on the interface that
we previously, foolishly advertised: "We will pass exactly these
options to your wrapper".  Except those options changed over time,
multiple times.  It was a terrible interface.

What we've switched to is a versioned interface.  By setting
GIT_SSH_VARIANT=simple, you are asking Git to promise to pass exactly
 options.  If Git has a new option it wants to pass (like the "-o
SendEnv" thing) but can live without it, then it can avoid breaking
your wrapper and continue to follow this new promise.

The trouble is that GIT_SSH_VARIANT=simple is too... simple.  You
would like a variant that passes in [-p port] [-4] [-6] as well.  We
didn't implement that because we didn't have the attention of any
wrapper writer who wanted it; in absence of a potential user, we
decided to wait for a user to propose the interface they want.  Now we
can celebrate, since that day has come.

How would you like your ssh variant to work?  Some possibilities to
get the thought process going:

 A. Would you want to set a variable 'GIT_SSH_SUPPORTS_OPTIONS=p46'
to inform Git about what options you support?

 B. Alternatively, what about a 'GIT_SSH_VARIANT=capabilities' variant
that calls "your-ssh-variant --capabailities" to get a
machine-readable list of capabilities it supports?

 C. Alternatively, would you like all parameters to come in on stdin,
credential helper style?

 D. Other ideas?

[...]
> But again, I'm just describing what makes sense to me. If you feel
> strongly about requiring the variant to be explicitly specified, I can
> certainly live with that.

I think I do feel strongly.

If you were using an old version of OpenSSH, this would be a reason to
revive the old patch, but I'm tempted to stall longer just to get more
use cases like this to come out of the woodwork.

Of course that's a dangerous thought process.  Probably by tomorrow
I'll think better of it and revive the patch.  In the meantime I would
be happy to see your answers to the questions above.

Sincerely,
Jonathan

[PATCH 4/3] ewah: adjust callers of ewah_read_mmap()

[Jeff King]

On Thu, Jun 14, 2018 at 11:28:50PM -0400, Jeff King wrote:

> Yep. We also fail to check if we even have enough bytes to read the
> buffer_size in the first place.
> 
> Here are some patches. The first one fixes the problem you found. The
> second one drops some dead code that has a related problem. And the
> third just drops some dead code that I noticed in the same file. :)
> 
>   [1/3]: ewah_read_mmap: bounds-check mmap reads
>   [2/3]: ewah: drop ewah_deserialize function
>   [3/3]: ewah: drop ewah_serialize_native function

Actually, we'd want this one on top. Arguably it could be squashed into
patch 1.

-- >8 --
Subject: ewah: adjust callers of ewah_read_mmap()

The return value of ewah_read_mmap() is now an ssize_t,
since we could (in theory) process up to 32GB of data. This
would never happen in practice, but a corrupt or malicious
.bitmap or index file could convince us to do so.

Let's make sure that we don't stuff the value into an int,
which would cause us to incorrectly move our pointer
forward.  We'd always move too little, since negative values
are used for reporting errors. So the worst case is just
that we end up reporting a corrupt file, not an
out-of-bounds read.

Signed-off-by: Jeff King 
---
 dir.c | 3 ++-
 pack-bitmap.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 61b513a078..d5185660f1 100644
--- a/dir.c
+++ b/dir.c
@@ -2725,7 +2725,8 @@ struct untracked_cache *read_untracked_extension(const 
void *data, unsigned long
struct read_data rd;
const unsigned char *next = data, *end = (const unsigned char *)data + 
sz;
const char *ident;
-   int ident_len, len;
+   int ident_len;
+   ssize_t len;
const char *exclude_per_dir;
 
if (sz <= 1 || end[-1] != '\0')
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 369bf69d75..2f27b10e35 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -125,7 +125,7 @@ static struct ewah_bitmap *read_bitmap_1(struct 
bitmap_index *index)
 {
struct ewah_bitmap *b = ewah_pool_new();
 
-   int bitmap_size = ewah_read_mmap(b,
+   ssize_t bitmap_size = ewah_read_mmap(b,
index->map + index->map_pos,
index->map_size - index->map_pos);
 
-- 
2.18.0.rc2.534.g53d976aeb8

[PATCH 3/3] ewah: drop ewah_serialize_native function

[Jeff King]

We don't call this function, and never have. The on-disk
bitmap format uses network-byte-order integers, meaning that
we cannot use the native-byte-order format written here.

Let's drop it in the name of simplicity.

Signed-off-by: Jeff King 
---
 ewah/ewah_io.c | 26 --
 ewah/ewok.h|  1 -
 2 files changed, 27 deletions(-)

diff --git a/ewah/ewah_io.c b/ewah/ewah_io.c
index 97c74266da..586396122f 100644
--- a/ewah/ewah_io.c
+++ b/ewah/ewah_io.c
@@ -20,32 +20,6 @@
 #include "ewok.h"
 #include "strbuf.h"
 
-int ewah_serialize_native(struct ewah_bitmap *self, int fd)
-{
-   uint32_t write32;
-   size_t to_write = self->buffer_size * 8;
-
-   /* 32 bit -- bit size for the map */
-   write32 = (uint32_t)self->bit_size;
-   if (write(fd, , 4) != 4)
-   return -1;
-
-   /** 32 bit -- number of compressed 64-bit words */
-   write32 = (uint32_t)self->buffer_size;
-   if (write(fd, , 4) != 4)
-   return -1;
-
-   if (write(fd, self->buffer, to_write) != to_write)
-   return -1;
-
-   /** 32 bit -- position for the RLW */
-   write32 = self->rlw - self->buffer;
-   if (write(fd, , 4) != 4)
-   return -1;
-
-   return (3 * 4) + to_write;
-}
-
 int ewah_serialize_to(struct ewah_bitmap *self,
  int (*write_fun)(void *, const void *, size_t),
  void *data)
diff --git a/ewah/ewok.h b/ewah/ewok.h
index 7e25ca2e61..e6102e6c75 100644
--- a/ewah/ewok.h
+++ b/ewah/ewok.h
@@ -87,7 +87,6 @@ int ewah_serialize_to(struct ewah_bitmap *self,
  int (*write_fun)(void *out, const void *buf, size_t len),
  void *out);
 int ewah_serialize(struct ewah_bitmap *self, int fd);
-int ewah_serialize_native(struct ewah_bitmap *self, int fd);
 int ewah_serialize_strbuf(struct ewah_bitmap *self, struct strbuf *);
 
 ssize_t ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len);
-- 
2.18.0.rc2.534.g53d976aeb8

[PATCH 2/3] ewah: drop ewah_deserialize function

[Jeff King]

We don't call this function, and in fact never have since it
was added (at least not in iterations of the ewah patches
that got merged). Instead we use ewah_read_mmap().

Let's drop the unused code.

Note to anybody who later wants to resurrect this: it does
not check for integer overflow in the ewah data size,
meaning it may be possible to convince the code to allocate
a too-small buffer and read() into it.

Signed-off-by: Jeff King 
---
 ewah/ewah_io.c | 55 --
 ewah/ewok.h|  1 -
 2 files changed, 56 deletions(-)

diff --git a/ewah/ewah_io.c b/ewah/ewah_io.c
index 33c08c40f8..97c74266da 100644
--- a/ewah/ewah_io.c
+++ b/ewah/ewah_io.c
@@ -168,58 +168,3 @@ ssize_t ewah_read_mmap(struct ewah_bitmap *self, const 
void *map, size_t len)
 
return ptr - (const uint8_t *)map;
 }
-
-int ewah_deserialize(struct ewah_bitmap *self, int fd)
-{
-   size_t i;
-   eword_t dump[2048];
-   const size_t words_per_dump = sizeof(dump) / sizeof(eword_t);
-   uint32_t bitsize, word_count, rlw_pos;
-
-   eword_t *buffer = NULL;
-   size_t words_left;
-
-   ewah_clear(self);
-
-   /* 32 bit -- bit size for the map */
-   if (read(fd, , 4) != 4)
-   return -1;
-
-   self->bit_size = (size_t)ntohl(bitsize);
-
-   /** 32 bit -- number of compressed 64-bit words */
-   if (read(fd, _count, 4) != 4)
-   return -1;
-
-   self->buffer_size = self->alloc_size = (size_t)ntohl(word_count);
-   REALLOC_ARRAY(self->buffer, self->alloc_size);
-
-   /** 64 bit x N -- compressed words */
-   buffer = self->buffer;
-   words_left = self->buffer_size;
-
-   while (words_left >= words_per_dump) {
-   if (read(fd, dump, sizeof(dump)) != sizeof(dump))
-   return -1;
-
-   for (i = 0; i < words_per_dump; ++i, ++buffer)
-   *buffer = ntohll(dump[i]);
-
-   words_left -= words_per_dump;
-   }
-
-   if (words_left) {
-   if (read(fd, dump, words_left * 8) != words_left * 8)
-   return -1;
-
-   for (i = 0; i < words_left; ++i, ++buffer)
-   *buffer = ntohll(dump[i]);
-   }
-
-   /** 32 bit -- position for the RLW */
-   if (read(fd, _pos, 4) != 4)
-   return -1;
-
-   self->rlw = self->buffer + ntohl(rlw_pos);
-   return 0;
-}
diff --git a/ewah/ewok.h b/ewah/ewok.h
index 357fd93c84..7e25ca2e61 100644
--- a/ewah/ewok.h
+++ b/ewah/ewok.h
@@ -90,7 +90,6 @@ int ewah_serialize(struct ewah_bitmap *self, int fd);
 int ewah_serialize_native(struct ewah_bitmap *self, int fd);
 int ewah_serialize_strbuf(struct ewah_bitmap *self, struct strbuf *);
 
-int ewah_deserialize(struct ewah_bitmap *self, int fd);
 ssize_t ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len);
 
 uint32_t ewah_checksum(struct ewah_bitmap *self);
-- 
2.18.0.rc2.534.g53d976aeb8

[PATCH 1/3] ewah_read_mmap: bounds-check mmap reads

[Jeff King]

The on-disk ewah format tells us how big the ewah data is,
and we blindly read that much from the buffer without
considering whether the mmap'd data is long enough, which
can lead to out-of-bound reads.

Let's make sure we have data available before reading it,
both for the ewah header/footer as well as for the bit data
itself. In particular:

  - keep our ptr/len pair in sync as we move through the
buffer, and check it before each read

  - check the size for integer overflow (this should be
impossible on 64-bit, as the size is given as a 32-bit
count of 8-byte words, but is possible on a 32-bit
system)

  - return the number of bytes read as an ssize_t instead of
an int, again to prevent integer overflow

  - compute the return value using a pointer difference;
this should yield the same result as the existing code,
but makes it more obvious that we got our computations
right

The included test is far from comprehensive, as it just
picks a static point at which to truncate the generated
bitmap. But in practice this will hit in the middle of an
ewah and make sure we're at least exercising this code.

Reported-by: Luat Nguyen 
Signed-off-by: Jeff King 
---
 ewah/ewah_io.c  | 25 +
 ewah/ewok.h |  2 +-
 t/t5310-pack-bitmaps.sh | 13 +
 3 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/ewah/ewah_io.c b/ewah/ewah_io.c
index bed1994551..33c08c40f8 100644
--- a/ewah/ewah_io.c
+++ b/ewah/ewah_io.c
@@ -122,16 +122,23 @@ int ewah_serialize_strbuf(struct ewah_bitmap *self, 
struct strbuf *sb)
return ewah_serialize_to(self, write_strbuf, sb);
 }
 
-int ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len)
+ssize_t ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len)
 {
const uint8_t *ptr = map;
+   size_t data_len;
size_t i;
 
+   if (len < sizeof(uint32_t))
+   return error("corrupt ewah bitmap: eof before bit size");
self->bit_size = get_be32(ptr);
ptr += sizeof(uint32_t);
+   len -= sizeof(uint32_t);
 
+   if (len < sizeof(uint32_t))
+   return error("corrupt ewah bitmap: eof before length");
self->buffer_size = self->alloc_size = get_be32(ptr);
ptr += sizeof(uint32_t);
+   len -= sizeof(uint32_t);
 
REALLOC_ARRAY(self->buffer, self->alloc_size);
 
@@ -141,15 +148,25 @@ int ewah_read_mmap(struct ewah_bitmap *self, const void 
*map, size_t len)
 * the endianness conversion in a separate pass to ensure
 * we're loading 8-byte aligned words.
 */
-   memcpy(self->buffer, ptr, self->buffer_size * sizeof(eword_t));
-   ptr += self->buffer_size * sizeof(eword_t);
+   data_len = st_mult(self->buffer_size, sizeof(eword_t));
+   if (len < data_len)
+   return error("corrupt ewah bitmap: eof in data "
+"(%"PRIuMAX" bytes short)",
+(uintmax_t)(data_len - len));
+   memcpy(self->buffer, ptr, data_len);
+   ptr += data_len;
+   len -= data_len;
 
for (i = 0; i < self->buffer_size; ++i)
self->buffer[i] = ntohll(self->buffer[i]);
 
+   if (len < sizeof(uint32_t))
+   return error("corrupt ewah bitmap: eof before rlw");
self->rlw = self->buffer + get_be32(ptr);
+   ptr += sizeof(uint32_t);
+   len -= sizeof(uint32_t);
 
-   return (3 * 4) + (self->buffer_size * 8);
+   return ptr - (const uint8_t *)map;
 }
 
 int ewah_deserialize(struct ewah_bitmap *self, int fd)
diff --git a/ewah/ewok.h b/ewah/ewok.h
index dc43d05b64..357fd93c84 100644
--- a/ewah/ewok.h
+++ b/ewah/ewok.h
@@ -91,7 +91,7 @@ int ewah_serialize_native(struct ewah_bitmap *self, int fd);
 int ewah_serialize_strbuf(struct ewah_bitmap *self, struct strbuf *);
 
 int ewah_deserialize(struct ewah_bitmap *self, int fd);
-int ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len);
+ssize_t ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len);
 
 uint32_t ewah_checksum(struct ewah_bitmap *self);
 
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 423c0a475f..237ee6e5fc 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -331,4 +331,17 @@ test_expect_success 'pack reuse respects --incremental' '
git show-index actual &&
test_cmp expect actual
 '
+
+test_expect_success 'truncated bitmap fails gracefully' '
+   git repack -ad &&
+   git rev-list --use-bitmap-index --count --all >expect &&
+   bitmap=$(ls .git/objects/pack/*.bitmap) &&
+   test_when_finished "rm -f $bitmap" &&
+   head -c 512 <$bitmap >$bitmap.tmp &&
+   mv $bitmap.tmp $bitmap &&
+   git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
+   test_cmp expect actual &&
+   test_i18ngrep corrupt stderr
+'
+
 test_done
-- 
2.18.0.rc2.534.g53d976aeb8

Re: security: potential out-of-bound read at ewah_io.c |ewah_read_mmap|

[Jeff King]

On Fri, Jun 15, 2018 at 06:59:43AM +0800, Luat Nguyen wrote:

> Recently, I’ve found a security issue related to out-of-bound read at 
> function named `ewah_read_mmap`

Thanks, this is definitely a bug worth addressing. But note...

> Assume that, an attacker can put malicious `./git/index` into a repo by 
> somehow.

We generally don't consider .git/index (or pack .bitmap files, which
also use this implementation) to be a major part of Git's attack
surface, since they are generated locally. And if you can write to
somebody's .git directory, there are already much easier ways to execute
arbitrary code.

> Since there is lack of check whether the remaining size of `ptr`is
> equal to `buffer_size` or not.

Yep. We also fail to check if we even have enough bytes to read the
buffer_size in the first place.

Here are some patches. The first one fixes the problem you found. The
second one drops some dead code that has a related problem. And the
third just drops some dead code that I noticed in the same file. :)

  [1/3]: ewah_read_mmap: bounds-check mmap reads
  [2/3]: ewah: drop ewah_deserialize function
  [3/3]: ewah: drop ewah_serialize_native function

 ewah/ewah_io.c  | 106 
 ewah/ewok.h |   4 +-
 t/t5310-pack-bitmaps.sh |  13 +
 3 files changed, 35 insertions(+), 88 deletions(-)

-Peff

[PATCH] Makefile: make NO_ICONV really mean "no iconv"

[Eric Sunshine]

The Makefile tweak NO_ICONV is meant to allow Git to be built without
iconv in case iconv is not installed or is otherwise dysfunctional.
However, NO_ICONV's disabling of iconv is incomplete and can incorrectly
allow "-liconv" to slip into the linker flags when NEEDS_LIBICONV is
defined, which breaks the build when iconv is not installed.

On some platforms, iconv lives directly in libc, whereas, on others it
resides in libiconv. For the latter case, NEEDS_LIBICONV instructs the
Makefile to add "-liconv" to the linker flags. config.mak.uname
automatically defines NEEDS_LIBICONV for platforms which require it.
The adding of "-liconv" is done unconditionally, despite NO_ICONV.

Work around this problem by making NO_ICONV take precedence over
NEEDS_LIBICONV.

Reported by: Mahmoud Al-Qudsi 
Signed-off-by: Eric Sunshine 
---

This patch is extra noisy due to the indentation change. Viewing it with
"git diff -w" helps. An alternative to re-indenting would have been to
"undefine NEEDS_LIBICONV", however, 'undefine' was added to GNU make in
3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided.

Reported here: 
https://public-inbox.org/git/cacctrkepbgycbxnen5nz+cs-tidyye_dkdwttxpp0cueeic...@mail.gmail.com/T/#u

 Makefile | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/Makefile b/Makefile
index 1d27f36365..e4b503d259 100644
--- a/Makefile
+++ b/Makefile
@@ -1351,17 +1351,19 @@ ifdef APPLE_COMMON_CRYPTO
LIB_4_CRYPTO += -framework Security -framework CoreFoundation
 endif
 endif
-ifdef NEEDS_LIBICONV
-   ifdef ICONVDIR
-   BASIC_CFLAGS += -I$(ICONVDIR)/include
-   ICONV_LINK = -L$(ICONVDIR)/$(lib) 
$(CC_LD_DYNPATH)$(ICONVDIR)/$(lib)
-   else
-   ICONV_LINK =
-   endif
-   ifdef NEEDS_LIBINTL_BEFORE_LIBICONV
-   ICONV_LINK += -lintl
+ifndef NO_ICONV
+   ifdef NEEDS_LIBICONV
+   ifdef ICONVDIR
+   BASIC_CFLAGS += -I$(ICONVDIR)/include
+   ICONV_LINK = -L$(ICONVDIR)/$(lib) 
$(CC_LD_DYNPATH)$(ICONVDIR)/$(lib)
+   else
+   ICONV_LINK =
+   endif
+   ifdef NEEDS_LIBINTL_BEFORE_LIBICONV
+   ICONV_LINK += -lintl
+   endif
+   EXTLIBS += $(ICONV_LINK) -liconv
endif
-   EXTLIBS += $(ICONV_LINK) -liconv
 endif
 ifdef NEEDS_LIBGEN
EXTLIBS += -lgen
-- 
2.18.0.rc1.256.g331a1db143

security: potential out-of-bound read at ewah_io.c |ewah_read_mmap|

[Luat Nguyen]

Hi folks,

Recently, I’ve found a security issue related to out-of-bound read at function 
named `ewah_read_mmap`

Assume that, an attacker can put malicious `./git/index` into a repo by somehow.

Since there is lack of check whether the remaining size of `ptr`is equal to 
`buffer_size` or not.

So the code reads exceed the buffer of `ptr` and reach to higher page. In this 
case, it is `/lib/x86_64-linux-gnu/ld-2.23.so`.

Leads to infoleak. You can find more details and asan crash below.



# xxd .git/index
: 4449 5243  0002   4653 4d4e  DIRCFSMN
0010:  0024  0001 1538 2489 c8fc 3616  ...$.8$...6.
0020:  0014    2000 4141   .. .AA
^ evil size here = 0x2000


* SNIP CODE *

int ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len)
{
… 
self->buffer_size = self->alloc_size = get_be32(ptr);
ptr += sizeof(uint32_t);
… 
memcpy(self->buffer, ptr, self->buffer_size * sizeof(eword_t));


[memory map]

0x7f990eca3000 0x7f990eca4000 r--p 1000 0  
/media/sf_Fuzz/vuln_repo/.git/index <— where `ptr` is placed
0x7f990eca4000 0x7f990eca5000 r--p 1000 25000  
/lib/x86_64-linux-gnu/ld-2.23.so <— memcpy will reach here
0x7f990eca5000 0x7f990eca6000 rw-p 1000 26000  
/lib/x86_64-linux-gnu/ld-2.23.so <— and here 


[ ASAN log ]

root@guest:/media/sf_SHARE/vuln_repo# /media/sf_SHARE/git-master-asan/git status
=
==4324==ERROR: AddressSanitizer: unknown-crash on address 0x7f6f235b at pc 
0x004bba79 bp 0x7ffc75e68850 sp 0x7ffc75e68000
READ of size 65536 at 0x7f6f235b thread T0
#0 0x4bba78 in __asan_memcpy 
/tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3
#1 0x8c910e in ewah_read_mmap 
/media/sf_SHARE/git-master-asan/ewah/ewah_io.c:144:2
#2 0x8e2534 in read_fsmonitor_extension 
/media/sf_SHARE/git-master-asan/fsmonitor.c:46:8
#3 0xa05862 in read_index_extension 
/media/sf_SHARE/git-master-asan/read-cache.c:1615:3
#4 0xa046f3 in do_read_index 
/media/sf_SHARE/git-master-asan/read-cache.c:1872:7
#5 0xa03325 in read_index_from 
/media/sf_SHARE/git-master-asan/read-cache.c:1913:8
#6 0xa03231 in read_index 
/media/sf_SHARE/git-master-asan/read-cache.c:1634:9
#7 0x9de5e8 in read_index_preload 
/media/sf_SHARE/git-master-asan/preload-index.c:119:15
#8 0x566cc6 in cmd_status 
/media/sf_SHARE/git-master-asan/builtin/commit.c:1358:2
#9 0x4ede8c in run_builtin /media/sf_SHARE/git-master-asan/git.c:417:11
#10 0x4ea939 in handle_builtin /media/sf_SHARE/git-master-asan/git.c:632:8
#11 0x4ed655 in run_argv /media/sf_SHARE/git-master-asan/git.c:684:4
#12 0x4ea037 in cmd_main /media/sf_SHARE/git-master-asan/git.c:761:19
#13 0x759c8b in main /media/sf_SHARE/git-master-asan/common-main.c:45:9
#14 0x7f6f2243382f in __libc_start_main 
(/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
#15 0x41c268 in _start (/media/sf_SHARE/git-master-asan/git+0x41c268)

Address 0x7f6f235b is a wild pointer.
SUMMARY: AddressSanitizer: unknown-crash 
/tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3
 in __asan_memcpy
Shadow bytes around the buggy address:
  0x0fee646adfb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adfc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adfd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adfe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0fee646ae000:[fe]fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae010: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae020: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae030: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae040: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae050: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:   00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:   fa
  Freed heap region:   fd
  Stack left redzone:  f1
  Stack mid redzone:   f2
  Stack right redzone: f3
  Stack after return:  f5
  Stack use after scope:   f8
  Global redzone:  f9
  Global init order:   f6
  Poisoned by user:f7
  Container overflow:  fc
  Array cookie:ac
  Intra object redzone:bb
  ASan internal:   fe
  Left alloca redzone: ca
  Right alloca redzone:cb
==4324==ABORTING
root@guest:/media/sf_SHARE/vuln_repo#


Regards,
Luat Nguyen.

Re: [PATCH v2 7/8] fetch-pack: put shallow info in output parameter

[Jonathan Tan]

> @@ -1122,6 +1124,7 @@ static int do_fetch(struct transport *transport,
>   int autotags = (transport->remote->fetch_tags == 1);
>   int retcode = 0;
>   const struct ref *remote_refs;
> + struct ref *new_remote_refs = NULL;

Above, you use the name "updated_remote_refs" - it's probably better to
standardize on one. I think "updated" is better.

(The transport calling it "fetched_refs" is fine, because that's what
they are from the perspective of the transport. From the perspective of
fetch-pack, it is indeed a new or updated set of remote refs.)

> - if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) 
> {
> +
> + if (fetch_refs(transport, ref_map, _remote_refs)) {
> + free_refs(ref_map);
> + retcode = 1;
> + goto cleanup;
> + }
> + if (new_remote_refs) {
> + free_refs(ref_map);
> + ref_map = get_ref_map(transport->remote, new_remote_refs, rs,
> +   tags, );
> + free_refs(new_remote_refs);
> + }
> + if (consume_refs(transport, ref_map)) {
>   free_refs(ref_map);
>   retcode = 1;
>   goto cleanup;

Here, if we got updated remote refs, we need to regenerate ref_map,
since it is the source of truth.

Maybe add a comment in the "if (new_remote_refs)" block explaining this
- something like: Regenerate ref_map using the updated remote refs,
because the transport would place shallow (and other) information
there.

> - for (i = 0; i < nr_sought; i++)
> + for (r = refs; r; r = r->next, i++)
>   if (status[i])
> - sought[i]->status = REF_STATUS_REJECT_SHALLOW;
> + r->status = REF_STATUS_REJECT_SHALLOW;

You use i here without initializing it to 0. t5703 also fails with this
patch - probably related to this, but I didn't check.

If you initialize i here, I don't think you need to initialize it to 0
at the top of this function.

[PATCH v3 5/7] fetch-pack: make negotiation-related vars local

[Jonathan Tan]

Reduce the number of global variables by making the priority queue and
the count of non-common commits in it local, passing them as a struct to
various functions where necessary.

This also helps in the case that fetch_pack() is invoked twice in the
same process (when tag following is required when using a transport that
does not support tag following), in that different priority queues will
now be used in each invocation, instead of reusing the possibly
non-empty one.

Signed-off-by: Jonathan Tan 
---
 fetch-pack.c | 116 ++-
 1 file changed, 69 insertions(+), 47 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 806c40021..473e415c5 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -50,8 +50,12 @@ static int marked;
  */
 #define MAX_IN_VAIN 256
 
-static struct prio_queue rev_list = { compare_commits_by_commit_date };
-static int non_common_revs, multi_ack, use_sideband;
+struct negotiation_state {
+   struct prio_queue rev_list;
+   int non_common_revs;
+};
+
+static int multi_ack, use_sideband;
 /* Allow specifying sha1 if it is a ref tip. */
 #define ALLOW_TIP_SHA1 01
 /* Allow request of a sha1 if it is reachable from a ref (possibly hidden 
ref). */
@@ -93,7 +97,9 @@ static void cache_one_alternate(const char *refname,
cache->items[cache->nr++] = obj;
 }
 
-static void for_each_cached_alternate(void (*cb)(struct object *))
+static void for_each_cached_alternate(struct negotiation_state *ns,
+ void (*cb)(struct negotiation_state *,
+struct object *))
 {
static int initialized;
static struct alternate_object_cache cache;
@@ -105,10 +111,11 @@ static void for_each_cached_alternate(void (*cb)(struct 
object *))
}
 
for (i = 0; i < cache.nr; i++)
-   cb(cache.items[i]);
+   cb(ns, cache.items[i]);
 }
 
-static void rev_list_push(struct commit *commit, int mark)
+static void rev_list_push(struct negotiation_state *ns,
+ struct commit *commit, int mark)
 {
if (!(commit->object.flags & mark)) {
commit->object.flags |= mark;
@@ -116,19 +123,21 @@ static void rev_list_push(struct commit *commit, int mark)
if (parse_commit(commit))
return;
 
-   prio_queue_put(_list, commit);
+   prio_queue_put(>rev_list, commit);
 
if (!(commit->object.flags & COMMON))
-   non_common_revs++;
+   ns->non_common_revs++;
}
 }
 
-static int rev_list_insert_ref(const char *refname, const struct object_id 
*oid)
+static int rev_list_insert_ref(struct negotiation_state *ns,
+  const char *refname,
+  const struct object_id *oid)
 {
struct object *o = deref_tag(parse_object(oid), refname, 0);
 
if (o && o->type == OBJ_COMMIT)
-   rev_list_push((struct commit *)o, SEEN);
+   rev_list_push(ns, (struct commit *)o, SEEN);
 
return 0;
 }
@@ -136,7 +145,7 @@ static int rev_list_insert_ref(const char *refname, const 
struct object_id *oid)
 static int rev_list_insert_ref_oid(const char *refname, const struct object_id 
*oid,
   int flag, void *cb_data)
 {
-   return rev_list_insert_ref(refname, oid);
+   return rev_list_insert_ref(cb_data, refname, oid);
 }
 
 static int clear_marks(const char *refname, const struct object_id *oid,
@@ -156,7 +165,7 @@ static int clear_marks(const char *refname, const struct 
object_id *oid,
when only the server does not yet know that they are common).
 */
 
-static void mark_common(struct commit *commit,
+static void mark_common(struct negotiation_state *ns, struct commit *commit,
int ancestors_only, int dont_parse)
 {
if (commit != NULL && !(commit->object.flags & COMMON)) {
@@ -166,12 +175,12 @@ static void mark_common(struct commit *commit,
o->flags |= COMMON;
 
if (!(o->flags & SEEN))
-   rev_list_push(commit, SEEN);
+   rev_list_push(ns, commit, SEEN);
else {
struct commit_list *parents;
 
if (!ancestors_only && !(o->flags & POPPED))
-   non_common_revs--;
+   ns->non_common_revs--;
if (!o->parsed && !dont_parse)
if (parse_commit(commit))
return;
@@ -179,7 +188,8 @@ static void mark_common(struct commit *commit,
for (parents = commit->parents;
parents;
parents = parents->next)
-   mark_common(parents->item, 0, dont_parse);
+ 

[PATCH v3 2/7] fetch-pack: clear marks before re-marking

[Jonathan Tan]

If tag following is required when using a transport that does not
support tag following, fetch_pack() will be invoked twice in the same
process, necessitating a clearing of the object flags used by
fetch_pack() sometime during the second invocation. This is currently
done in find_common(), which means that the invocation of
mark_complete_and_common_ref() in do_fetch_pack() is useless.

(This cannot be reproduced with Git alone, because all transports that
come with Git support tag following.)

Therefore, move this clearing from find_common() to its
parent function do_fetch_pack(), right before it calls
mark_complete_and_common_ref().

This has been occurring since the commit that introduced the clearing of
marks, 420e9af498 ("Fix tag following", 2008-03-19).

The corresponding code for protocol v2 in do_fetch_pack_v2() does not
have this problem, as the clearing of flags is done before any marking
(whether by rev_list_insert_ref_oid() or
mark_complete_and_common_ref()).

Signed-off-by: Jonathan Tan 
---
 fetch-pack.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 5c87bb8bb..2812499a5 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -336,9 +336,6 @@ static int find_common(struct fetch_pack_args *args,
 
if (args->stateless_rpc && multi_ack == 1)
die(_("--stateless-rpc requires multi_ack_detailed"));
-   if (marked)
-   for_each_ref(clear_marks, NULL);
-   marked = 1;
 
for_each_ref(rev_list_insert_ref_oid, NULL);
for_each_cached_alternate(insert_one_alternate_object);
@@ -1070,6 +1067,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args 
*args,
if (!server_supports("deepen-relative") && args->deepen_relative)
die(_("Server does not support --deepen"));
 
+   if (marked)
+   for_each_ref(clear_marks, NULL);
+   marked = 1;
mark_complete_and_common_ref(args, );
filter_refs(args, , sought, nr_sought);
if (everything_local(args, )) {
-- 
2.17.0.582.gccdcbd54c4

[PATCH v3 3/7] fetch-pack: directly end negotiation if ACK ready

[Jonathan Tan]

When "ACK %s ready" is received, find_common() clears rev_list in an
attempt to stop further "have" lines from being sent [1]. It is much
more readable to explicitly break from the loop instead.

So explicitly break from the loop, and make the clearing of the rev_list
happen unconditionally.

[1] The rationale is further described in the originating commit
f2cba9299b ("fetch-pack: Finish negotation if remote replies "ACK %s
ready"", 2011-03-14).

Signed-off-by: Jonathan Tan 
---
 fetch-pack.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 2812499a5..60adfc073 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -517,10 +517,8 @@ static int find_common(struct fetch_pack_args *args,
mark_common(commit, 0, 1);
retval = 0;
got_continue = 1;
-   if (ack == ACK_ready) {
-   clear_prio_queue(_list);
+   if (ack == ACK_ready)
got_ready = 1;
-   }
break;
}
}
@@ -530,6 +528,8 @@ static int find_common(struct fetch_pack_args *args,
print_verbose(args, _("giving up"));
break; /* give up */
}
+   if (got_ready)
+   break;
}
}
 done:
@@ -1096,6 +1096,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args 
*args,
die(_("git fetch-pack: fetch failed."));
 
  all_done:
+   clear_prio_queue(_list);
return ref;
 }
 
@@ -1300,7 +1301,6 @@ static int process_acks(struct packet_reader *reader, 
struct oidset *common)
}
 
if (!strcmp(reader->line, "ready")) {
-   clear_prio_queue(_list);
received_ready = 1;
continue;
}
@@ -1441,6 +1441,7 @@ static struct ref *do_fetch_pack_v2(struct 
fetch_pack_args *args,
}
}
 
+   clear_prio_queue(_list);
oidset_clear();
return ref;
 }
-- 
2.17.0.582.gccdcbd54c4

[PATCH v3 7/7] fetch-pack: introduce negotiator API

[Jonathan Tan]

Introduce the new files fetch-negotiator.{h,c}, which contains an API
behind which the details of negotiation are abstracted. Currently, only
one algorithm is available: the existing one.

This patch is written to be easily reviewed: static functions are
moved verbatim from fetch-pack.c to negotiator/default.c, and it can be
seen that the lines replaced by negotiator->X() calls are present in the
X() functions respectively.

Signed-off-by: Jonathan Tan 
---
 Makefile |   2 +
 fetch-negotiator.c   |   8 ++
 fetch-negotiator.h   |  57 
 fetch-pack.c | 205 ---
 negotiator/default.c | 176 +
 negotiator/default.h |   8 ++
 object.h |   3 +-
 7 files changed, 292 insertions(+), 167 deletions(-)
 create mode 100644 fetch-negotiator.c
 create mode 100644 fetch-negotiator.h
 create mode 100644 negotiator/default.c
 create mode 100644 negotiator/default.h

diff --git a/Makefile b/Makefile
index 1d27f3636..96f84d1dc 100644
--- a/Makefile
+++ b/Makefile
@@ -859,6 +859,7 @@ LIB_OBJS += ewah/ewah_bitmap.o
 LIB_OBJS += ewah/ewah_io.o
 LIB_OBJS += ewah/ewah_rlw.o
 LIB_OBJS += exec-cmd.o
+LIB_OBJS += fetch-negotiator.o
 LIB_OBJS += fetch-object.o
 LIB_OBJS += fetch-pack.o
 LIB_OBJS += fsck.o
@@ -891,6 +892,7 @@ LIB_OBJS += merge-blobs.o
 LIB_OBJS += merge-recursive.o
 LIB_OBJS += mergesort.o
 LIB_OBJS += name-hash.o
+LIB_OBJS += negotiator/default.o
 LIB_OBJS += notes.o
 LIB_OBJS += notes-cache.o
 LIB_OBJS += notes-merge.o
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
new file mode 100644
index 0..2675d120f
--- /dev/null
+++ b/fetch-negotiator.c
@@ -0,0 +1,8 @@
+#include "git-compat-util.h"
+#include "fetch-negotiator.h"
+#include "negotiator/default.h"
+
+void fetch_negotiator_init(struct fetch_negotiator *negotiator)
+{
+   default_negotiator_init(negotiator);
+}
diff --git a/fetch-negotiator.h b/fetch-negotiator.h
new file mode 100644
index 0..b1290aa9c
--- /dev/null
+++ b/fetch-negotiator.h
@@ -0,0 +1,57 @@
+#ifndef FETCH_NEGOTIATOR
+#define FETCH_NEGOTIATOR
+
+struct commit;
+
+/*
+ * An object that supplies the information needed to negotiate the contents of
+ * the to-be-sent packfile during a fetch.
+ *
+ * To set up the negotiator, call fetch_negotiator_init(), then known_common()
+ * (0 or more times), then add_tip() (0 or more times).
+ *
+ * Then, when "have" lines are required, call next(). Call ack() to report what
+ * the server tells us.
+ *
+ * Once negotiation is done, call release(). The negotiator then cannot be used
+ * (unless reinitialized with fetch_negotiator_init()).
+ */
+struct fetch_negotiator {
+   /*
+* Before negotiation starts, indicate that the server is known to have
+* this commit.
+*/
+   void (*known_common)(struct fetch_negotiator *, struct commit *);
+
+   /*
+* Once this function is invoked, known_common() cannot be invoked any
+* more.
+*
+* Indicate that this commit and all its ancestors are to be checked
+* for commonality with the server.
+*/
+   void (*add_tip)(struct fetch_negotiator *, struct commit *);
+
+   /*
+* Once this function is invoked, known_common() and add_tip() cannot
+* be invoked any more.
+*
+* Return the next commit that the client should send as a "have" line.
+*/
+   const struct object_id *(*next)(struct fetch_negotiator *);
+
+   /*
+* Inform the negotiator that the server has the given commit. This
+* method must only be called on commits returned by next().
+*/
+   int (*ack)(struct fetch_negotiator *, struct commit *);
+
+   void (*release)(struct fetch_negotiator *);
+
+   /* internal use */
+   void *data;
+};
+
+void fetch_negotiator_init(struct fetch_negotiator *negotiator);
+
+#endif
diff --git a/fetch-pack.c b/fetch-pack.c
index fb76d4017..4b13d3389 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -15,10 +15,10 @@
 #include "connect.h"
 #include "transport.h"
 #include "version.h"
-#include "prio-queue.h"
 #include "sha1-array.h"
 #include "oidset.h"
 #include "packfile.h"
+#include "fetch-negotiator.h"
 
 static int transfer_unpack_limit = -1;
 static int fetch_unpack_limit = -1;
@@ -36,13 +36,7 @@ static const char *alternate_shallow_file;
 
 /* Remember to update object flag allocation in object.h */
 #define COMPLETE   (1U << 0)
-#define COMMON (1U << 1)
-#define COMMON_REF (1U << 2)
-#define SEEN   (1U << 3)
-#define POPPED (1U << 4)
-#define ALTERNATE  (1U << 5)
-
-static int marked;
+#define ALTERNATE  (1U << 1)
 
 /*
  * After sending this many "have"s if we do not get any new ACK , we
@@ -50,11 +44,6 @@ static int marked;
  */
 #define MAX_IN_VAIN 256
 
-struct negotiation_state {
-   struct prio_queue rev_list;
-   int non_common_revs;
-};
-
 static int multi_ack, 

[PATCH v3 4/7] fetch-pack: use ref adv. to prune "have" sent

[Jonathan Tan]

In negotiation using protocol v2, fetch-pack sometimes does not make
full use of the information obtained in the ref advertisement:
specifically, that if the server advertises a commit that the client
also has, the client never needs to inform the server that it has the
commit's parents, since it can just tell the server that it has the
advertised commit and it knows that the server can and will infer the
rest.

This is because, in do_fetch_pack_v2(), rev_list_insert_ref_oid() is
invoked before mark_complete_and_common_ref(). This means that if we
have a commit that is both our ref and their ref, it would be enqueued
by rev_list_insert_ref_oid() as SEEN, and since it is thus already SEEN,
mark_complete_and_common_ref() would not enqueue it.

If mark_complete_and_common_ref() were invoked first, as it is in
do_fetch_pack() for protocol v0, then mark_complete_and_common_ref()
would enqueue it with COMMON_REF | SEEN. The addition of COMMON_REF
ensures that its parents are not sent as "have" lines.

Change the order in do_fetch_pack_v2() to be consistent with
do_fetch_pack(), and to avoid sending unnecessary "have" lines.

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

diff --git a/fetch-pack.c b/fetch-pack.c
index 60adfc073..806c40021 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1392,9 +1392,6 @@ static struct ref *do_fetch_pack_v2(struct 
fetch_pack_args *args,
for_each_ref(clear_marks, NULL);
marked = 1;
 
-   for_each_ref(rev_list_insert_ref_oid, NULL);
-   for_each_cached_alternate(insert_one_alternate_object);
-
/* Filter 'ref' by 'sought' and those that aren't local 
*/
mark_complete_and_common_ref(args, );
filter_refs(args, , sought, nr_sought);
@@ -1402,6 +1399,9 @@ static struct ref *do_fetch_pack_v2(struct 
fetch_pack_args *args,
state = FETCH_DONE;
else
state = FETCH_SEND_REQUEST;
+
+   for_each_ref(rev_list_insert_ref_oid, NULL);
+   for_each_cached_alternate(insert_one_alternate_object);
break;
case FETCH_SEND_REQUEST:
if (send_fetch_request(fd[1], args, ref, ,
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index d4f435155..e0cdc295d 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -755,6 +755,39 @@ test_expect_success 'fetching deepen' '
)
 '
 
+test_expect_success 'use ref advertisement to prune "have" lines sent' '
+   rm -rf server client &&
+   git init server &&
+   test_commit -C server both_have_1 &&
+   git -C server tag -d both_have_1 &&
+   test_commit -C server both_have_2 &&
+
+   git clone server client &&
+   test_commit -C server server_has &&
+   test_commit -C client client_has &&
+
+   # In both protocol v0 and v2, ensure that the parent of both_have_2 is
+   # not sent as a "have" line. The client should know that the server has
+   # both_have_2, so it only needs to inform the server that it has
+   # both_have_2, and the server can infer the rest.
+
+   rm -f trace &&
+   cp -r client clientv0 &&
+   GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 \
+   fetch origin server_has both_have_2 &&
+   grep "have $(git -C client rev-parse client_has)" trace &&
+   grep "have $(git -C client rev-parse both_have_2)" trace &&
+   ! grep "have $(git -C client rev-parse both_have_2^)" trace &&
+
+   rm -f trace &&
+   cp -r client clientv2 &&
+   GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \
+   fetch origin server_has both_have_2 &&
+   grep "have $(git -C client rev-parse client_has)" trace &&
+   grep "have $(git -C client rev-parse both_have_2)" trace &&
+   ! grep "have $(git -C client rev-parse both_have_2^)" trace
+'
+
 test_expect_success 'filtering by size' '
rm -rf server client &&
test_create_repo server &&
-- 
2.17.0.582.gccdcbd54c4

[PATCH v3 0/7] Refactor fetch negotiation into its own API

[Jonathan Tan]

Thanks, Brandon and Junio, for your comments.

Updates since v2:
 - nothing new in patches 1 and 2
 - patch 3: now clear priority queue unconditionally once we are done
   with it (as requested by Brandon in a comment for a later patch)
 - patch 4: updated commit message to not mention everything_local() (as
   pointed out by Brandon), updated test to not rely on the fact that
   fetch-pack uses prefix matching (thanks, Junio, for the observation)
 - patch 5: used a more descriptive name ("struct negotiation_state")
   for the struct, instead of "struct data"
 - squashed patch 8 into patch 7; this means that the comments are not
   moved verbatim, but for the reviewer, verbatim-ness of comments is
   probably not that important anyway

Jonathan Tan (7):
  fetch-pack: split up everything_local()
  fetch-pack: clear marks before re-marking
  fetch-pack: directly end negotiation if ACK ready
  fetch-pack: use ref adv. to prune "have" sent
  fetch-pack: make negotiation-related vars local
  fetch-pack: move common check and marking together
  fetch-pack: introduce negotiator API

 Makefile  |   2 +
 fetch-negotiator.c|   8 ++
 fetch-negotiator.h|  57 ++
 fetch-pack.c  | 255 ++
 negotiator/default.c  | 176 +
 negotiator/default.h  |   8 ++
 object.h  |   3 +-
 t/t5500-fetch-pack.sh |  33 ++
 8 files changed, 373 insertions(+), 169 deletions(-)
 create mode 100644 fetch-negotiator.c
 create mode 100644 fetch-negotiator.h
 create mode 100644 negotiator/default.c
 create mode 100644 negotiator/default.h

-- 
2.17.0.582.gccdcbd54c4

[PATCH v3 1/7] fetch-pack: split up everything_local()

[Jonathan Tan]

The function everything_local(), despite its name, also (1) marks
commits as COMPLETE and COMMON_REF and (2) invokes filter_refs() as
important side effects. Extract (1) into its own function
(mark_complete_and_common_ref()) and remove
(2).

The restoring of save_commit_buffer, which was introduced in a1c6d7c1a7
("fetch-pack: restore save_commit_buffer after use", 2017-12-08), is a
concern of the parse_object() call in mark_complete_and_common_ref(), so
it has been moved from the end of everything_local() to the end of
mark_complete_and_common_ref().

Signed-off-by: Jonathan Tan 
---
 fetch-pack.c | 39 ++-
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index a320ce987..5c87bb8bb 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -734,12 +734,20 @@ static int add_loose_objects_to_set(const struct 
object_id *oid,
return 0;
 }
 
-static int everything_local(struct fetch_pack_args *args,
-   struct ref **refs,
-   struct ref **sought, int nr_sought)
+/*
+ * Mark recent commits available locally and reachable from a local ref as
+ * COMPLETE. If args->no_dependents is false, also mark COMPLETE remote refs as
+ * COMMON_REF (otherwise, we are not planning to participate in negotiation, 
and
+ * thus do not need COMMON_REF marks).
+ *
+ * The cutoff time for recency is determined by this heuristic: it is the
+ * earliest commit time of the objects in refs that are commits and that we 
know
+ * the commit time of.
+ */
+static void mark_complete_and_common_ref(struct fetch_pack_args *args,
+struct ref **refs)
 {
struct ref *ref;
-   int retval;
int old_save_commit_buffer = save_commit_buffer;
timestamp_t cutoff = 0;
struct oidset loose_oid_set = OIDSET_INIT;
@@ -812,7 +820,18 @@ static int everything_local(struct fetch_pack_args *args,
}
}
 
-   filter_refs(args, refs, sought, nr_sought);
+   save_commit_buffer = old_save_commit_buffer;
+}
+
+/*
+ * Returns 1 if every object pointed to by the given remote refs is available
+ * locally and reachable from a local ref, and 0 otherwise.
+ */
+static int everything_local(struct fetch_pack_args *args,
+   struct ref **refs)
+{
+   struct ref *ref;
+   int retval;
 
for (retval = 1, ref = *refs; ref ; ref = ref->next) {
const struct object_id *remote = >old_oid;
@@ -829,8 +848,6 @@ static int everything_local(struct fetch_pack_args *args,
  ref->name);
}
 
-   save_commit_buffer = old_save_commit_buffer;
-
return retval;
 }
 
@@ -1053,7 +1070,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args 
*args,
if (!server_supports("deepen-relative") && args->deepen_relative)
die(_("Server does not support --deepen"));
 
-   if (everything_local(args, , sought, nr_sought)) {
+   mark_complete_and_common_ref(args, );
+   filter_refs(args, , sought, nr_sought);
+   if (everything_local(args, )) {
packet_flush(fd[1]);
goto all_done;
}
@@ -1377,7 +1396,9 @@ static struct ref *do_fetch_pack_v2(struct 
fetch_pack_args *args,
for_each_cached_alternate(insert_one_alternate_object);
 
/* Filter 'ref' by 'sought' and those that aren't local 
*/
-   if (everything_local(args, , sought, nr_sought))
+   mark_complete_and_common_ref(args, );
+   filter_refs(args, , sought, nr_sought);
+   if (everything_local(args, ))
state = FETCH_DONE;
else
state = FETCH_SEND_REQUEST;
-- 
2.17.0.582.gccdcbd54c4

[PATCH v3 6/7] fetch-pack: move common check and marking together

[Jonathan Tan]

When receiving 'ACK  continue' for a common commit, check if
the commit was already known to be common and mark it as such if not up
front. This should make future refactoring of how the information about
common commits is stored more straightforward.

No visible change intended.

Signed-off-by: Jonathan Tan 
---
 fetch-pack.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 473e415c5..fb76d4017 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -505,11 +505,14 @@ static int find_common(struct negotiation_state *ns,
case ACK_continue: {
struct commit *commit =
lookup_commit(result_oid);
+   int was_common;
if (!commit)
die(_("invalid commit %s"), 
oid_to_hex(result_oid));
+   was_common = commit->object.flags & 
COMMON;
+   mark_common(ns, commit, 0, 1);
if (args->stateless_rpc
 && ack == ACK_common
-&& !(commit->object.flags & COMMON)) {
+&& !was_common) {
/* We need to replay the have 
for this object
 * on the next RPC request so 
the peer knows
 * it is in common with us.
@@ -526,7 +529,6 @@ static int find_common(struct negotiation_state *ns,
} else if (!args->stateless_rpc
   || ack != ACK_common)
in_vain = 0;
-   mark_common(ns, commit, 0, 1);
retval = 0;
got_continue = 1;
if (ack == ACK_ready)
-- 
2.17.0.582.gccdcbd54c4

Is NO_ICONV misnamed or is it broken?

[Mahmoud Al-Qudsi]

Hello list,

With regards to the Makefile define/variable `NO_ICONV` - the Makefile
comments imply that it should be used if "your libc doesn't properly support
iconv," which could mean anything from "a patch will be applied" to "iconv
won't be used."

Based off the name of the varibale, the assumption is that iconv is an
optional dependency that can be omitted if compiled with NO_ICONV. However, in
practice attempting to compile git with `make ... NO_ICONV=1` and libiconv not
installed results in linker errors as follows:

```
~> make clean
# omitted
~> make NO_ICONV=1
# ommitted
LINK git-credential-store
/usr/bin/ld: cannot find -liconv
cc: error: linker command failed with exit code 1 (use -v to see invocation)
gmake: *** [Makefile:2327: git-credential-store] Error 1
```

Am I misunderstanding the intended behavior when NO_ICONV is defined (i.e. it
does not remove the dependency on libiconv) or is this a bug and iconv should
not, in fact, be required?

Many thanks,

Mahmoud Al-Qudsi
NeoSmart Technologies

Re: OAuth2 support in git?

[brian m. carlson]

On Thu, Jun 14, 2018 at 11:15:07AM -0400, Jeff King wrote:
> On Thu, Jun 14, 2018 at 10:13:42AM +, brian m. carlson wrote:
> > There isn't any support for Bearer authentication in Git.  For HTTP, we
> > use libcurl, which doesn't provide this natively.  While it could in
> > theory be added, it would require some reworking of the auth code.
> > 
> > You are, of course, welcome to send a patch.
> 
> If it's just a custom Authorization header, we should be able to support
> it with existing curl versions without _too_ much effort.

It shouldn't be too difficult, but we have some fallback among various
authentication types that would need reworking.

> I think there are probably two possible directions:
> 
>  1. add a special "bearer" command line option, etc, as a string
> 
>  2. add a boolean option to send the existing "password" field as a
> "bearer" header
> 
> I suspect (2) would fit in with the existing code better, as the special
> case would mostly be limited to the manner in which we feed the
> credential to curl. And you could probably just set a config option for
> "this url's auth will be oauth2", and use the existing mechanisms for
> providing the password.

I agree option (2) would be better.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: (Bug report + fix) gitk "IgnCase" search doesn't ignore case

[Eric Sunshine]

On Thu, Jun 14, 2018 at 02:53:03PM +0200, Juan Navarro wrote:
> Gitk "find commit" search function doesn't follow the "IgnCase" option that
> is selectable with a combo selector on the right side of the window; it
> should be searching in a case-insensitive way, but it doesn't.
> 
> Steps to reproduce:
> [...]
> 3. In the "Find commit" bar, select "changing lines matching"
> 4. In the right side of the same bar, select "IgnCase"
> [...]
> 
> Proposed solution is almost trivial: check if the "IgnCase" option is
> enabled, and in that case add the flag "-i" to the git command. Now that we
> are at it, it's probably correct to add that option to all search modes.
> A diff is attached to this email, hoping that someone is able to apply it
> (sorry I'm not familiarized with contributing with patch files, but the diff
> is pretty small anyways).

Thanks for reporting this.

A different way to interpret the situation is that the user-interface
is being inconsistent. For instance, the "fields" pop-up next to the
"exact/ignore-case/regexp" pop-up does not seem to make sense for
search types other than "containing", so it probably ought to be
disabled for anything other than "containing". By extension, one could
argue that the "exact/ignore-case/regexp" pop-up also ought be
disabled for non-"containing" searches. The fact that they are not
disabled confuses people into thinking that they should be functional
for all searches, not just for "containing" searches, even though such
functionality was never implemented (and indeed, may be difficult to
implement fully).

Your proposed fix handles only the "ignore case" item; it does not
implement "regexp" functionality, so it could be considered
incomplete. A more complete fix would also disable the "regexp" item
to avoid misleading users, and to head off future bug reports similar
to this one saying that "regexp" doesn't work for non-"containing"
searches. (Bonus points for also disabling the "fields" pop-up for
non-"containing" searches when it's not applicable.)

Below is your fix wrapped up as a proper patch and sent in-line rather
than as an attachment. It's also slightly simplified by dropping the
unnecessary extra variable. You'll need to sign-off on the patch if it
is ever to be accepted. You can do so by adding it after my sign-off.
If you don't feel like re-sending the patch with your sign-off, you
can alternately reply to this email with a "Signed-off-by: Your Name
" line.

Note, however, that the gitk project is, at best, deeply slumbering,
so it's not clear when or even if patches will be incorporated.
(Indeed, other recent gitk-related patches sent to the mailing list
have not yet been picked up.)

--- >8 ---
From: Juan Navarro 
Subject: [PATCH] gitk: support "ignore case" for non-"containing" searches

The "Exact/Ignore Case/Regexp" pop-up control only affects "containing"
searches. Other types of searches ("touching paths", "adding/removing
strings", "changing lines matching") ignore this option. Improve the
user experience by also recognizing "ignore case" for non-"containing"
searches.

Note: This change only implements the "ignore case" option for these
other search types; it does not add support for the "regexp" option
(which still only affects "containing" searches). A more complete "fix"
would improve the user experience even more by making the UI more
consistent; namely, by disabling options which don't make sense or are
not easily implemented for non-"containing" searches. In particular, the
"regexp" pop-up item and the neighboring "fields" pop-up control ought
perhaps be disabled when a non-"containing" search is selected.

[es: wrote commit message; slightly simplified proposed "fix"]

Signed-off-by: Eric Sunshine 
---
diff --git a/gitk-git/gitk b/gitk-git/gitk
index a14d7a16b2..fbb75f7390 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -4806,6 +4806,9 @@ proc do_file_hl {serial} {
# must be "containing:", i.e. we're searching commit info
return
 }
+if {$findtype eq [mc "IgnCase"]} {
+   set gdtargs [linsert $gdtargs 0 "-i"]
+}
 set cmd [concat | git diff-tree -r -s --stdin $gdtargs]
 set filehighlight [open $cmd r+]
 fconfigure $filehighlight -blocking 0
-- 
2.18.0.rc1.256.g331a1db143

Re: [PATCH v2 05/31] tree: add repository argument to lookup_tree

[Stefan Beller]

On Thu, Jun 14, 2018 at 12:33 PM Derrick Stolee  wrote:
> > The 'tree' member of 'struct commit' was renamed to 'maybe_tree'.
>
> Resolving the merge was not very simple. I have a working merge
> available on GitHub [1] as commit 99a899f7c12ef73840dbe79c71acb11034d707dd.

Thanks for pointing this out and resolving the merge.
ds/generation-numbers seems to be cooking in next for longer already, so
I might just rebase this series on top of that as well.

Thanks for the heads up!
Stefan

Re: [PATCH 10/35] commit: add repository argument to lookup_commit

[Brandon Williams]

On 06/14, Stefan Beller wrote:
> On Thu, Jun 14, 2018 at 9:22 AM Duy Nguyen  wrote:
> >
> > On Wed, May 30, 2018 at 2:51 AM Stefan Beller  wrote:
> > > diff --git a/shallow.c b/shallow.c
> > > index 9bb07a56dca..60fe1fe1e58 100644
> > > --- a/shallow.c
> > > +++ b/shallow.c
> > > @@ -31,7 +31,7 @@ int register_shallow(struct repository *r, const struct 
> > > object_id *oid)
> > >  {
> > > struct commit_graft *graft =
> > > xmalloc(sizeof(struct commit_graft));
> > > -   struct commit *commit = lookup_commit(oid);
> > > +   struct commit *commit = lookup_commit(the_repository, oid);
> >
> > This looks wrong. register_shallow() has struct repository argument
> > 'r' and it should be used here instead.
> 
> Right.
> 
> > If this is a mechanical conversion, I will also be happy that the
> > switch from the_repo to r is done in a separate patch.
> 
> This part of the code is not touched later in this series,
> so I'll fix it if a reroll is needed.

Yeah maybe at some point when lookup_commit can understand arbitrary
repositories we can change this from the_repository to r.  This patch is
part of that mechanical change and has to be the_repository till
lookup_commit has been fully converted.

> 
> > FYI I noticed this because I'm in a quest to kill the_index by passing
> > 'struct index_state *' throughout library code, and sometimes I pass
> > 'struct repository *' instead when I see that code uses more things
> > that just the index.  And I have started to replace the_repository in
> > some places with a function argument.
> >
> > If some of my patches come first while you have not finished
> > repository conversion (very likely), you and I will have to pay
> > attention to this more often.

-- 
Brandon Williams

Re: [PATCH v2 8/8] fetch-pack: implement ref-in-want

[Brandon Williams]

On 06/14, Stefan Beller wrote:
> On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams  wrote:
> 
> > +static void receive_wanted_refs(struct packet_reader *reader, struct ref 
> > *refs)
> > +{
> ...
> > +
> > +   for (r = refs; r; r = r->next) {
> > +   if (!strcmp(end, r->name)) {
> > +   oidcpy(>old_oid, );
> > +   break;
> > +   }
> > +   }
> 
> The server is documented as MUST NOT send additional refs,
> which is fine here, as we'd have no way of storing them anyway.
> Do we want to issue a warning, though?
> 
> if (!r) /* never break'd */
> warning ("server send unexpected line '%s'", reader.line);

Depends, does this warning help out the end user or do you think it
would confuse users to see this and still have their fetch succeed?

> 
> 
> 
> > diff --git a/remote.c b/remote.c
> > index abe80c139..c9d452ac0 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
> > if (refspec->exact_sha1) {
> > ref_map = alloc_ref(name);
> > get_oid_hex(name, _map->old_oid);
> > +   ref_map->exact_sha1 = 1;
> > } else {
> > ref_map = get_remote_ref(remote_refs, name);
> > }
> > diff --git a/remote.h b/remote.h
> > index 45ecc6cef..e5338e368 100644
> > --- a/remote.h
> > +++ b/remote.h
> > @@ -73,6 +73,7 @@ struct ref {
> > force:1,
> > forced_update:1,
> > expect_old_sha1:1,
> > +   exact_sha1:1,
> 
> Can we rename that to exact_oid ?

I'll fix this.

-- 
Brandon Williams

Re: [PATCH 10/35] commit: add repository argument to lookup_commit

[Stefan Beller]

On Thu, Jun 14, 2018 at 9:22 AM Duy Nguyen  wrote:
>
> On Wed, May 30, 2018 at 2:51 AM Stefan Beller  wrote:
> > diff --git a/shallow.c b/shallow.c
> > index 9bb07a56dca..60fe1fe1e58 100644
> > --- a/shallow.c
> > +++ b/shallow.c
> > @@ -31,7 +31,7 @@ int register_shallow(struct repository *r, const struct 
> > object_id *oid)
> >  {
> > struct commit_graft *graft =
> > xmalloc(sizeof(struct commit_graft));
> > -   struct commit *commit = lookup_commit(oid);
> > +   struct commit *commit = lookup_commit(the_repository, oid);
>
> This looks wrong. register_shallow() has struct repository argument
> 'r' and it should be used here instead.

Right.

> If this is a mechanical conversion, I will also be happy that the
> switch from the_repo to r is done in a separate patch.

This part of the code is not touched later in this series,
so I'll fix it if a reroll is needed.

> FYI I noticed this because I'm in a quest to kill the_index by passing
> 'struct index_state *' throughout library code, and sometimes I pass
> 'struct repository *' instead when I see that code uses more things
> that just the index.  And I have started to replace the_repository in
> some places with a function argument.
>
> If some of my patches come first while you have not finished
> repository conversion (very likely), you and I will have to pay
> attention to this more often.

Re: OAuth2 support in git?

[Jeff King]

On Thu, Jun 14, 2018 at 04:46:10PM -0400, Randall S. Becker wrote:

> > I suspect (2) would fit in with the existing code better, as the special 
> > case
> > would mostly be limited to the manner in which we feed the credential to
> > curl. And you could probably just set a config option for "this url's auth 
> > will
> > be oauth2", and use the existing mechanisms for providing the password.
> > 
> > We'd maybe also want to allow credential helpers to say "by the way, this
> > password should be treated as a bearer token", for cases where you might
> > sometimes use oauth2 and sometimes a real password.
> 
> Be aware that there are 4 (ish) flavours of OAuth2 the last time I
> checked. It is important to know which one (or all) to implement. The
> embedded form is probably the easiest to comprehend - and the least
> implemented from my research. More common OAuth2 instances use a
> third-man website to hold session keys and authorization. That may be
> problematic for a whole bunch of us who do not play in that world.

I think Git's usage would be limited to "how do I present this token for
my requests". I don't think we'd ever recognize an oauth redirect and
try to fulfill it ourselves.  We'd rely on getting a 401 and punting all
those bits to a credential helper to do the heavy lifting.

I say that not knowing much about oauth2, of course, so maybe there
would be complications with that approach (I do know there are multiple
ways you can present a token, but we'd support whichever ones people are
interested in enough to show up and provide a patch for).

-Peff

Re: [PATCH v4 00/23] Fix incorrect use of the_index

[Elijah Newren]

On Thu, Jun 14, 2018 at 10:18 AM, Duy Nguyen  wrote:
> On Mon, Jun 11, 2018 at 06:49:00PM +0200, Duy Nguyen wrote:
>> On Mon, Jun 11, 2018 at 6:44 PM Elijah Newren  wrote:
>> > > What about merge-recursive.c? Given that this whole thing will take
>> > > many release cycles to finish, your work may get merged before mine
>> > > and I could do the conversion now (and resolve conflicts and resubmit
>> > > later). Of course if you like to keep merge-recursive.c the_index-free
>> > > now, I will not stop you ;-)
>> >
>> > I was just worried that since I was making changes in
>> > merge-recursive.c that it'd cause you conflict pain, so I offered to
>> > convert it.  If that pain doesn't bother you, feel free to do the
>> > conversion and we'll just work through the minor conflicts as we go.
>> > Besides, sounds like you've converted nearly all of git, so it may
>> > make sense to just keep it together in one big series.
>>
>> OK let's just "quickly" get your series in then. I still have a few
>> files to go and a couple more places to look back and ponder. I'm in
>> no hurry to convert merge-recursive now :D
>
> Actually could you quickly check if I pass the right index in the
> following patch? I realize you only have one extra index, but I don't
> know if it gets passed elsewhere and in some function I'm supposed to
> use another index than the default "o->repo->index". Sometimes
> o->repo->index is not the correct answer.
>
> It takes many patches to get to this stage, so if you diff context
> looks so strange to you, just ignore this for now. I'll eventually
> send this one out in a series.
>
> PS. I put a 'struct repository *' in 'struct merge_options' instead
> because it turns out we do need full repo struct in some function.

Yes, this looks right.

It'd be nice to be able to apply the patch locally so I could also
look at the --color-words version, but it's not a real big deal.

In general, this is how the index stuff works in merge-recursive:
  * In unpack_trees_start(), we call unpack_trees() using the_index as
src, and tmp_index as dest.
  * As soon as unpack_trees() finishes, we swap: (orig_index,
the_index) = (the_index, tmp_index).
  * We ONLY use orig_index (also called unpack_opts.src_index) in the
was_*() family of functions, which query about what the index was like
before the merge started.  (To name these functions explicitly:
was_tracked_and_matches(), was_tracked(), and was_dirty()).  ALL other
callsites within merge-recursive.c should be operating on the_index,
or, after your patch, o->repo->index.


(Also, if you want to avoid merge conflicts, you might want to avoid
unrelated changes like the whitespace and alignment fixes.  You'll be
happy to know, though, that I think I fixed them all up as part of the
en/merge-recursive-cleanup series in pu.)

Elijah

>
> -- 8< --
> diff --git a/builtin/am.c b/builtin/am.c
> index 3961878871..5d2ff9aa8c 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1592,7 +1592,7 @@ static int fall_back_threeway(const struct am_state 
> *state, const char *index_pa
>  * changes.
>  */
>
> -   init_merge_options();
> +   init_merge_options(, the_repository);
>
> o.branch1 = "HEAD";
> their_tree_name = xstrfmt("%.*s", linelen(state->msg), state->msg);
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 528c6de7e1..4fd53cec6e 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -570,7 +570,7 @@ static int merge_working_tree(const struct checkout_opts 
> *opts,
>  * a pain; plumb in an option to set
>  * o.renormalize?
>  */
> -   init_merge_options();
> +   init_merge_options(, the_repository);
> o.verbosity = 0;
> work = write_tree_from_memory();
>
> diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
> index 0dd9021958..c4a9e575d6 100644
> --- a/builtin/merge-recursive.c
> +++ b/builtin/merge-recursive.c
> @@ -28,7 +28,7 @@ int cmd_merge_recursive(int argc, const char **argv, const 
> char *prefix)
> struct merge_options o;
> struct commit *result;
>
> -   init_merge_options();
> +   init_merge_options(, the_repository);
> if (argv[0] && ends_with(argv[0], "-subtree"))
> o.subtree_shift = "";
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index db460a35cf..2ebd939b76 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -671,7 +671,7 @@ static int try_merge_strategy(const char *strategy, 
> struct commit_list *common,
> return 2;
> }
>
> -   init_merge_options();
> +   init_merge_options(, the_repository);
> if (!strcmp(strategy, "subtree"))
> o.subtree_shift = "";
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index a296ba3874..fd6556b6de 100644
> --- 

RE: OAuth2 support in git?

[Randall S. Becker]

On June 14, 2018 11:15 AM, Jeff King wrote:
> On Thu, Jun 14, 2018 at 10:13:42AM +, brian m. carlson wrote:
> 
> > > I know that other git server environments like github support that
> > > on client side by allowing tokens to be used as usernames in a BASIC
> > > authentication flow. We could do the same but I am asking whether
> > > there is also a way to transport tokens in a standard conform
> > > "Authorization: Bearer ..." Header field.
> >
> > There isn't any support for Bearer authentication in Git.  For HTTP,
> > we use libcurl, which doesn't provide this natively.  While it could
> > in theory be added, it would require some reworking of the auth code.
> >
> > You are, of course, welcome to send a patch.
> 
> If it's just a custom Authorization header, we should be able to support it
> with existing curl versions without _too_ much effort.
> 
> I think there are probably two possible directions:
> 
>  1. add a special "bearer" command line option, etc, as a string
> 
>  2. add a boolean option to send the existing "password" field as a
> "bearer" header
> 
> I suspect (2) would fit in with the existing code better, as the special case
> would mostly be limited to the manner in which we feed the credential to
> curl. And you could probably just set a config option for "this url's auth 
> will
> be oauth2", and use the existing mechanisms for providing the password.
> 
> We'd maybe also want to allow credential helpers to say "by the way, this
> password should be treated as a bearer token", for cases where you might
> sometimes use oauth2 and sometimes a real password.

Be aware that there are 4 (ish) flavours of OAuth2 the last time I checked. It 
is important to know which one (or all) to implement. The embedded form is 
probably the easiest to comprehend - and the least implemented from my 
research. More common OAuth2 instances use a third-man website to hold session 
keys and authorization. That may be problematic for a whole bunch of us who do 
not play in that world.

Cheers,
Randall

-- Brief whoami:
  NonStop developer since approximately NonStop(2112884442)
  UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [GSoC][PATCH v2 2/2] rebase--interactive: rewrite the edit-todo functionality in C

[Elijah Newren]

Hi Alban,

On Thu, Jun 14, 2018 at 1:24 PM, Alban Gruin  wrote:
> Le 14/06/2018 à 22:14, Junio C Hamano a écrit :
>> Alban Gruin  writes:
>>
>>> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
>>> index ded5e291d..d2990b210 100644
>>> --- a/builtin/rebase--helper.c
>>> +++ b/builtin/rebase--helper.c
>>> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] 
>>> = {
>>>  int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>>>  {
>>>  struct replay_opts opts = REPLAY_OPTS_INIT;
>>> -unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo 
>>> = 0;
>>> +unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
>>
>> Sorry, but where does "write_edit_todo = 0" in the preimage come from?
>>
>
> It comes from my conversion of append_todo_help()[0], on which this
> series is based.
>
> [0]
> https://public-inbox.org/git/20180607103012.22981-1-alban.gr...@gmail.com/

Given that both Junio and I had the same basic question, you'll
probably want to put dependency information into your cover letters.
If a series applies on master, then this isn't needed.  But if it
requires other changes that are in a topic in next or pu, you can name
the topic that it depends upon (e.g. ag/rebase-p, as from the "What's
cooking" emails or you can grab them as branchnames from
git://github.com/gitster/git).  If the dependency is something that
Junio hasn't picked up yet, provide a link to the other submission.

Others may have additional suggestions here...

Re: [PATCH] t5526: test recursive submodules when fetching moved submodules

[Heiko Voigt]

On Thu, Jun 14, 2018 at 10:37:30AM -0700, Stefan Beller wrote:
> The topic merged in 0c7ecb7c311 (Merge branch 'sb/submodule-move-nested',
> 2018-05-08) provided support for moving nested submodules.
> 
> Remove the NEEDSWORK comment and implement the nested submodules test as
> the comment hinted at.
> 
> Signed-off-by: Stefan Beller 
> ---

Looks good to me.

Cheers Heiko

Re: [PATCH] submodule: fix NULL correctness in renamed broken submodules

[Heiko Voigt]

Hi,

On Thu, Jun 14, 2018 at 10:31:07AM -0700, Stefan Beller wrote:
> When fetching with recursing into submodules, the fetch logic inspects
> the superproject which submodules actually need to be fetched. This is
> tricky for submodules that were renamed in the fetched range of commits.
> This was implemented in c68f8375760 (implement fetching of moved
> submodules, 2017-10-16), and this patch fixes a mistake in the logic
> there.
> 
> When the warning is printed, the `name` might be NULL as
> default_name_or_path can return NULL, so fix the warning to use the path
> as obtained from the diff machinery, as that is not NULL.
> 
> While at it, make sure we only attempt to load the submodule if a git
> directory of the submodule is found as default_name_or_path will return
> NULL in case the git directory cannot be found. Note that passing NULL
> to submodule_from_name is just a semantic error, as submodule_from_name
> accepts NULL as a value, but then the return value is not the submodule
> that was asked for, but some arbitrary other submodule. (Cf. 'config_from'
> in submodule-config.c: "If any parameter except the cache is a NULL
> pointer just return the first submodule. Can be used to check whether
> there are any submodules parsed.")
> 
> Reported-by: Duy Nguyen 
> Helped-by: Duy Nguyen 
> Helped-by: Heiko Voigt 
> Signed-off-by: Stefan Beller 
> ---

Looks good to me.

Cheers Heiko

Re: [GSoC][PATCH v2 2/2] rebase--interactive: rewrite the edit-todo functionality in C

[Alban Gruin]

Hi Junio,

Le 14/06/2018 à 22:14, Junio C Hamano a écrit :
> Alban Gruin  writes:
> 
>> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
>> index ded5e291d..d2990b210 100644
>> --- a/builtin/rebase--helper.c
>> +++ b/builtin/rebase--helper.c
>> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] 
>> = {
>>  int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>>  {
>>  struct replay_opts opts = REPLAY_OPTS_INIT;
>> -unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo 
>> = 0;
>> +unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
> 
> Sorry, but where does "write_edit_todo = 0" in the preimage come from?
> 

It comes from my conversion of append_todo_help()[0], on which this
series is based.

[0]
https://public-inbox.org/git/20180607103012.22981-1-alban.gr...@gmail.com/

Cheers,
Alban

Re: [GSoC][PATCH v2 2/2] rebase--interactive: rewrite the edit-todo functionality in C

[Junio C Hamano]

Alban Gruin  writes:

> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index ded5e291d..d2990b210 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = 
> {
>  int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  {
>   struct replay_opts opts = REPLAY_OPTS_INIT;
> - unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo 
> = 0;
> + unsigned flags = 0, keep_empty = 0, rebase_merges = 0;

Sorry, but where does "write_edit_todo = 0" in the preimage come from?

Re: [PATCH v2 8/8] fetch-pack: implement ref-in-want

[Stefan Beller]

On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams  wrote:

> +static void receive_wanted_refs(struct packet_reader *reader, struct ref 
> *refs)
> +{
...
> +
> +   for (r = refs; r; r = r->next) {
> +   if (!strcmp(end, r->name)) {
> +   oidcpy(>old_oid, );
> +   break;
> +   }
> +   }

The server is documented as MUST NOT send additional refs,
which is fine here, as we'd have no way of storing them anyway.
Do we want to issue a warning, though?

if (!r) /* never break'd */
warning ("server send unexpected line '%s'", reader.line);



> diff --git a/remote.c b/remote.c
> index abe80c139..c9d452ac0 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
> if (refspec->exact_sha1) {
> ref_map = alloc_ref(name);
> get_oid_hex(name, _map->old_oid);
> +   ref_map->exact_sha1 = 1;
> } else {
> ref_map = get_remote_ref(remote_refs, name);
> }
> diff --git a/remote.h b/remote.h
> index 45ecc6cef..e5338e368 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -73,6 +73,7 @@ struct ref {
> force:1,
> forced_update:1,
> expect_old_sha1:1,
> +   exact_sha1:1,

Can we rename that to exact_oid ?
(bonus points for also converting expect_old_sha1)

Thanks,
Stefan

Re: [PATCH v2 4/8] fetch-pack: use ref adv. to prune "have" sent

[Junio C Hamano]

Jonathan Tan  writes:

> +test_expect_success 'use ref advertisement to prune "have" lines sent' '
> + rm -rf server client &&
> + git init server &&
> + test_commit -C server both_have_1 &&
> + git -C server tag -d both_have_1 &&
> + test_commit -C server both_have_2 &&
> +
> + # In this test, the ref name that only the server has is a prefix of all
> + # other refs. This is because in protocol v2, the client sends
> + # "ref-prefix" to limit the ref advertisement. Naming the ref "bo" means
> + # that "ref-prefix refs/tags/bo*" is sent, resulting in the client also
> + # knowing about refs/tags/both_have_2, just as it would when it uses
> + # protocol v0.

I have a mixed feeling about this test.

The client wants to fetch "bo" and nothing else in this example.
And refs "both_have_*", which have *nothing* to do with the ref the
client actually wants, is advertised merely because "both_have_*"
begins with "bo" with v2.  But that is an implementation detail that
we do not necessarily want to cast in stone, isn't it?  After all,
in future iterations of the protocol, we may find it too excessive
that we have to advertise both_have_1..both_have_1 when the
client asks for bo and change either the pattern matching rule of
what "ref-prefix refs/tags/bo" matches, or the ref-prefix sent by
the client, and at that point, the expectation that both_have_2 is
sent as "have" but both_have_2^1 is not may have to change, no?

> + git clone server client &&
> + test_commit -C server bo &&
> + test_commit -C client client_has &&
> +
> + # In both protocol v0 and v2, ensure that the parent of both_have_2 is
> + # not sent as a "have" line. The client should know that the server has
> + # both_have_2, so it only needs to inform the server that it has
> + # both_have_2, and the server can infer the rest.
> +
> + rm -f trace &&
> + cp -r client clientv0 &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 \
> + fetch origin bo &&

> + grep "have $(git -C client rev-parse client_has)" trace &&
> + grep "have $(git -C client rev-parse both_have_2)" trace &&
> + ! grep "have $(git -C client rev-parse both_have_2^)" trace &&
> +
> + rm -f trace &&
> + cp -r client clientv2 &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \
> + fetch origin bo &&
> + grep "have $(git -C client rev-parse client_has)" trace &&
> + grep "have $(git -C client rev-parse both_have_2)" trace &&
> + ! grep "have $(git -C client rev-parse both_have_2^)" trace
> +'
> +
>  test_expect_success 'filtering by size' '
>   rm -rf server client &&
>   test_create_repo server &&

Re: [PATCH v2 7/8] fetch-pack: put shallow info in output parameter

[Stefan Beller]

> +   !oidcmp(>peer_ref->old_oid, >old_oid)) {
> +   /*
> +* These need to be reported as fetched, but we don 
> not

do not or don't; there is no middle way. ;)

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jeff King]

On Thu, Jun 14, 2018 at 11:55:22AM -0700, Jonathan Nieder wrote:

> > No, my wrapper _isn't_ simple. It passes most options to openssh, but
> > just doesn't understand the "-G" probing.  So if the default was
> > openssh-like instead of "simple", then that would work fine without me
> > setting anything, just like it did before.
> >
> > Which I thought was where the discussion ended up, but perhaps I'm
> > misunderstanding.
> 
> Do you mean that it doesn't pass "-G" through, or that when using old
> versions of openssh that doesn't support "-G" the probing fails?

It just doesn't pass "-G" through.

> If the former, then detecting the wrapper as something other than
> "ssh" is intended behavior (though we might want to change what that
> something is, as discussed in the previous thread).  If the latter,
> then this is https://crbug.com/git/7 which I consider to be a bug.

I certainly see the argument that "well, if it doesn't do '-G' then it's
not _really_ openssh". My counter to that is that we don't actually
_care_ about -G (and never did before recently). It's just a proxy for
"do we understand -p", which my script does understand. My wrapper might
eventually break if we depend on new options (like "-o SendEnv"), but
the worst case there is generally no different before or after your
patch: the command barfs.

I say "generally" because of course you can come up with an example
where my script quietly interprets "-o" as something else, but it seems
like most uses there would cause an error.  And anyway, by making me set
GIT_SSH_VARIANT all we've bought is plausible deniability that it's _my_
fault for doing so when my script doesn't handle the new option
gracefully. ;)

But again, I'm just describing what makes sense to me. If you feel
strongly about requiring the variant to be explicitly specified, I can
certainly live with that.

-Peff

Re: [PATCH v2 5/8] fetch-pack: make negotiation-related vars local

[Junio C Hamano]

Jonathan Tan  writes:

> -static struct prio_queue rev_list = { compare_commits_by_commit_date };
> -static int non_common_revs, multi_ack, use_sideband;
> +struct data {
> + struct prio_queue rev_list;
> + int non_common_revs;
> +};
> +
> +static int multi_ack, use_sideband;

Aside from Brandon's comments, I think passing these throughout the
callchain instead of having them as static-globals is a step in the
right direction.  Could you, however, give the structure a bit more
descriptive name, than simply "data"?  Anything the computer works
on is "data", and there must be something more specific than that
about this particular struct ;-)

Re: [PATCH v2 05/31] tree: add repository argument to lookup_tree

[Derrick Stolee]

On 6/14/2018 1:55 PM, Derrick Stolee wrote:

On 6/13/2018 7:04 PM, Stefan Beller wrote:

diff --git a/commit-graph.c b/commit-graph.c
index 71125d7cbb6..88a4b0d2a47 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -261,7 +261,7 @@ static int fill_commit_in_graph(struct commit 
*item, struct commit_graph *g, uin

  item->graph_pos = pos;
    hashcpy(oid.hash, commit_data);
-    item->tree = lookup_tree();
+    item->tree = lookup_tree(the_repository, );
    date_high = get_be32(commit_data + g->hash_len + 8) & 0x3;
  date_low = get_be32(commit_data + g->hash_len + 12);
diff --git a/commit.c b/commit.c
index d76d64d4dfc..755b8b9d94f 100644
--- a/commit.c
+++ b/commit.c
@@ -354,7 +354,7 @@ int parse_commit_buffer(struct commit *item, 
const void *buffer, unsigned long s

  if (get_sha1_hex(bufptr + 5, parent.hash) < 0)
  return error("bad tree pointer in commit %s",
   oid_to_hex(>object.oid));
-    item->tree = lookup_tree();
+    item->tree = lookup_tree(the_repository, );
  bufptr += tree_entry_len + 1; /* "tree " + "hex sha1" + "\n" */
  pptr = >parents;
  diff --git a/fsck.c b/fsck.c
index 2d372f2a3f3..b07abb9796c 100644


I'm a bit late here, but you don't have ds/lazy-load-trees merged in 
your history, so this will conflict with 'master'. I caught this as I 
was trying to merge ds/generation-numbers with your branch.


The 'tree' member of 'struct commit' was renamed to 'maybe_tree'.


Resolving the merge was not very simple. I have a working merge 
available on GitHub [1] as commit 99a899f7c12ef73840dbe79c71acb11034d707dd.


Thanks,
-Stolee

[1] 
https://github.com/derrickstolee/git/commits/generation-numbers-object-store

Re: [PATCH v2 6/8] fetch: refactor to make function args narrower

[Stefan Beller]

On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams  wrote:
>
> Refactor find_non_local_tags and get_ref_map to only take the
> information they need instead of the entire transport struct. Besides
> improving code clarity, this also improves their flexibility, allowing
> for a different set of refs to be used instead of relying on the ones
> stored in the transport struct.
>

This patch and the two prior refactoring patches are
Reviewed-by: Stefan Beller 

Re: [PATCH v2 3/8] upload-pack: test negotiation with changing repository

[Stefan Beller]

On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams  wrote:
>
> Add tests to check the behavior of fetching from a repository which
> changes between rounds of negotiation (for example, when different
> servers in a load-balancing agreement participate in the same stateless
> RPC negotiation). This forms a baseline of comparison to the ref-in-want
> functionality (which will be introduced to the client in subsequent
> commits), and ensures that subsequent commits do not change existing
> behavior.
>
> As part of this effort, a mechanism to substitute strings in a single
> HTTP response is added.

This patch looks good to me.
Thanks,
Stefan

Re: [PATCH v2 1/8] test-pkt-line: add unpack-sideband subcommand

[Brandon Williams]

On 06/14, Stefan Beller wrote:
> On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams  wrote:
> >
> > Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
> > enable unpacking packet line data sent multiplexed using a sideband.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  t/helper/test-pkt-line.c | 37 +
> >  1 file changed, 37 insertions(+)
> >
> > diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
> > index 0f19e53c7..2a551 100644
> > --- a/t/helper/test-pkt-line.c
> > +++ b/t/helper/test-pkt-line.c
> > @@ -1,3 +1,4 @@
> > +#include "cache.h"
> >  #include "pkt-line.h"
> >
> >  static void pack_line(const char *line)
> > @@ -48,6 +49,40 @@ static void unpack(void)
> > }
> >  }
> >
> > +static void unpack_sideband(void)
> > +{
> > +   struct packet_reader reader;
> > +   packet_reader_init(, 0, NULL, 0,
> > +  PACKET_READ_GENTLE_ON_EOF |
> > +  PACKET_READ_CHOMP_NEWLINE);
> > +
> > +   while (packet_reader_read() != PACKET_READ_EOF) {
> > +   int band;
> > +   int fd;
> > +
> > +   switch (reader.status) {
> > +   case PACKET_READ_EOF:
> > +   break;
> > +   case PACKET_READ_NORMAL:
> > +   band = reader.line[0] & 0xff;
> > +   if (band == 1)
> > +   fd = 1;
> > +   else
> > +   fd = 2;
> > +
> > +   write_or_die(fd, reader.line+1, reader.pktlen-1);
> 
> white space around + and - ?

Will fix.

> 
> > +
> > +   if (band == 3)
> > +   die("sind-band error");
> 
> s/sind/side/ ?

Thanks for catching this.

> 
> What values for band are possible?
> e.g. band==4 would also just write to fd=1;
> but I suspect we don't want that, yet.
> 
> So maybe
> 
> band = reader.line[0] & 0xff;
> if (band < 1 || band > 2)
> die("unexpected side band %d", band)
> fd = band;
> 
> instead?

Yeah that's must cleaner logic.

-- 
Brandon Williams

Re: [PATCH 19/20] abbrev: support relative abbrev values

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 14 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  writes:
>
>> On Wed, Jun 13 2018, Junio C Hamano wrote:
>>
>>> Ævar Arnfjörð Bjarmason  writes:
>>>
 E.g. here's a breakdown of my dotfiles repo:

 $ git -c core.abbrev=4 log  --pretty=format:%h|perl -nE 'chomp;say 
 length'|sort|uniq -c|sort -nr
 784 4
  59 5
   7 6

 I don't have a single commit that needs 7 characters, yet that's our
 default. This is a sane trade-off for the kernel, but for something
 that's just a toy or something you're playing around with something
 shorter can make sense.

 SHA-1s aren't just written down, but also e.g. remembered in wetware
 short-term memory.
>>>
>>> That's a fine example of what I called "supporting absurdly small
>>> absolute values like 4"; I still do not see why you want "negative
>>> relative values" from that example.
>>
>> Because hardcoding -2 is very different than setting it to 5, because
>> the -2 will scale to the size of the repository, but 5 is just 7-2 where
>> 7 is our default value.
>>
>> So, in general if you want less future proof hashes by some
>> probabilistic metric (whether you use core.validateAbbrev or not) you'd
>> use -2 or -3, just like you might use +2 or +3 if you'd like to have
>> more future-proof hashes (especially with core.validateAbbrev=true).
>
> That still does not make much sense to me at all.
>
> I do agree that something shorter than the default 7 may be more
> appropriate for our wetware short-term memory, and it would make
> sense to grow the "riskier to collide than the default heuristics
> but more memorable" variant as the project grows, _ONLY_ _IF_ our
> wetware short-term memory scales with the project we happen to be
> working on.  But our wetware does not scale with the project we work
> on; at least mine does not.

Yes, it's a trade-off, but just because something is a trade-off doesn't
make it useless.

Aside from the feature I'm proposing, the same thing applies to the
short SHA-1 currently. My ~1k commit dotfiles has 7 characters, but
linux.git has 12. Does that make printing the short SHA-1 at all useless
and we should just fall back to 40 characters if it's say >= 12? No.

12 is still better than 40, but if we could get away with it 10 would be
better than 12. Right now the "get away with it" calculation is a
hardcoded constant, this makes it configurable.

The reason to make it configurable is because you may want more future
proof *or* less future-proof SHA-1s depending on the use-case. Printing
a longer SHA-1 has a cost, including but not limited to:

 * Remembering it for a short time
 * Seeing it on one screen and typing it into another computer manually
 * `git log --oneline` output in a terminal where horizontal space is at
   a premium

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jonathan Nieder]

Hi,

Sorry for the slow replies.  I was out of office earlier and am back
now.

Jeff King wrote:
> On Tue, Jun 12, 2018 at 09:29:13AM -0700, Junio C Hamano wrote:
>> Jeff King  writes:

>>> To be honest, I could easily see an argument that I _should_ be setting
>>> GIT_SSH_VARIANT to explain what my wrapper is expecting, even though it
>>> happened to work before.

Yes, I encourage anyone setting GIT_SSH to also set GIT_SSH_VARIANT.
This way, there can be no confusion about what the setter intends.

The autodetection is in the category of "necessary evil" that wouldn't
exist if we were starting over.

[...]
> No, my wrapper _isn't_ simple. It passes most options to openssh, but
> just doesn't understand the "-G" probing.  So if the default was
> openssh-like instead of "simple", then that would work fine without me
> setting anything, just like it did before.
>
> Which I thought was where the discussion ended up, but perhaps I'm
> misunderstanding.

Do you mean that it doesn't pass "-G" through, or that when using old
versions of openssh that doesn't support "-G" the probing fails?

If the former, then detecting the wrapper as something other than
"ssh" is intended behavior (though we might want to change what that
something is, as discussed in the previous thread).  If the latter,
then this is https://crbug.com/git/7 which I consider to be a bug.

[...]
> So I'm OK if we just leave it as-is. It's mostly that I dug into the
> thread and was left with the impression that it was an unfinished
> leftover that we meant to do.
[...]
>> In any case, from where I sit, I am still waiting for this offer
>> by Jonathan
>>
>>> It's good you caught this flaw in the detection.  Would something like
>>> the following make sense?  If so, I can resend with a commit message
>>> and tests tomorrow or the day after.
>>
>> to be followed up ;-)
>
> Yes, that was the part that left the impression. :)

Thanks for the poke.

The patch [1] didn't leave me super happy, since it means there is yet
another ssh variant for people to understand, in order to accomodate
an old version of OpenSSH that is going to go away eventually.  In
Debian, modern versions of git declare an incompatibility with old
versions of OpenSSH in their package metadata to avoid this issue.

In my defense, I said "If so" and no one seemed too enthusiastic about
the patch making sense. ;-)

I'll take another look and probably resend.

Sincerely,
Jonathan

[1] 
https://public-inbox.org/git/20180103050730.ga87...@aiede.mtv.corp.google.com/

Re: [PATCH v2 2/8] upload-pack: implement ref-in-want

[Brandon Williams]

On 06/14, Stefan Beller wrote:
> Hi Brandon,
> On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams  wrote:
> > negotiation, which may happen if, for example, the desired repository is
> > provided by multiple Git servers in a load-balancing arrangement.
> 
> ... and the repository is not replicated evenly to all servers, yet.

I'll update the commit msg to also include this.

> 
> > In order to eliminate this vulnerability, implement the ref-in-want
> > feature for the 'fetch' command in protocol version 2.  This feature
> > enables the 'fetch' command to support requests in the form of ref names
> > through a new "want-ref " parameter.  At the conclusion of
> > negotiation, the server will send a list of all of the wanted references
> > (as provided by "want-ref" lines) in addition to the generated packfile.
> 
> This paragraph makes it sound as if it can be combined technically,
> i.e.
> 
> client:
> want 01234...
> want-ref master
> 
> .. usual back and forth + pack..
> 
> server:
> 
>   wanted-ref: master 2345..
> 
> What happens if the client "wants" a sha1 that is advertised,
> but happens to be the same as a wanted-ref?

This would be fine, same as sending a want line with the same sha1 lots
of times.  Though there would still be a wanted-ref section from the
server for the wanted-ref.

> 
> > Signed-off-by: Brandon Williams 
> > ---
> >  Documentation/config.txt|   7 ++
> >  Documentation/technical/protocol-v2.txt |  29 -
> >  t/t5703-upload-pack-ref-in-want.sh  | 153 
> >  upload-pack.c   |  64 ++
> >  4 files changed, 252 insertions(+), 1 deletion(-)
> >  create mode 100755 t/t5703-upload-pack-ref-in-want.sh
> >
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index ab641bf5a..fb1dd7428 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if 
> > it is seen in the
> >  repository-level config (this is a safety measure against fetching from
> >  untrusted repositories).
> >
> > +uploadpack.allowRefInWant::
> > +   If this option is set, `upload-pack` will support the `ref-in-want`
> > +   feature of the protocol version 2 `fetch` command.  This feature
> > +   is intended for the benefit of load-balanced servers which may
> > +   not have the same view of what OIDs their refs point to due to
> > +   replication delay.
> 
> Instead of saying who benefits, can we also say what the feature is about?
> Didn't someone mention on the first round of this series, that technically
> ref-in-want also provides smaller net work load as refs usually are shorter
> than oids (specifically as oids will grow in the hash transisition plan 
> later)?
> Is that worth mentioning?

Well I basically just took this from what a previous reviewer thought it
should say.  I think what you have listed here isn't really a big
benefit of using ref-in-want, its the issue with load-balanced servers
that this is trying to solve.

> 
> When using this feature is a ref advertisement still needed?

Maybe in the future no, but as of right now the code is structured to
still request a ref advertisement.

> 
> > +
> >  url..insteadOf::
> > Any URL that starts with this value will be rewritten to
> > start, instead, with . In cases where some site serves a
> > diff --git a/Documentation/technical/protocol-v2.txt 
> > b/Documentation/technical/protocol-v2.txt
> > index 49bda76d2..6020632b4 100644
> > --- a/Documentation/technical/protocol-v2.txt
> > +++ b/Documentation/technical/protocol-v2.txt
> > @@ -299,12 +299,22 @@ included in the client's request:
> > for use with partial clone and partial fetch operations. See
> > `rev-list` for possible "filter-spec" values.
> >
> > +If the 'ref-in-want' feature is advertised, the following argument can
> > +be included in the client's request as well as the potential addition of
> > +the 'wanted-refs' section in the server's response as explained below.
> > +
> > +want-ref 
> > +   Indicates to the server that the client wants to retrieve a
> > +   particular ref, where  is the full name of a ref on the
> > +   server.  A server should ignore any "want-ref " lines where
> > +doesn't exist on the server.
> 
> Are patterns allowed?, e.g. I might want refs/tags/* at all times.

Nope, "Where  is the full name of a ref".  We can maybe allow this
at a later point in time.

> 
> > @@ -319,6 +329,10 @@ header.
> >  shallow = "shallow" SP obj-id
> >  unshallow = "unshallow" SP obj-id
> >
> > +wanted-refs = PKT-LINE("wanted-refs" LF)
> > + *PKT-LINE(wanted-ref LF)
> > +wanted-ref = obj-id SP refname
> > +
> >  packfile = PKT-LINE("packfile" LF)
> >*PKT-LINE(%x01-03 *%x00-ff)
> >
> > @@ -379,6 +393,19 @@ header.
> > * This section is only included if a packfile 

Re: [PATCH v2 2/8] upload-pack: implement ref-in-want

[Stefan Beller]

Hi Brandon,
On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams  wrote:
>
> Currently, while performing packfile negotiation, clients are only
> allowed to specify their desired objects using object ids.  This causes
> a vulnerability to failure when an object turns non-existent during

I stopped reading when stumbling upon 'vulnerability to failure' a I
found it hard to read. A quick search turns out this is insider slang
of civil engineers. :)

> negotiation, which may happen if, for example, the desired repository is
> provided by multiple Git servers in a load-balancing arrangement.

... and the repository is not replicated evenly to all servers, yet.

> In order to eliminate this vulnerability, implement the ref-in-want
> feature for the 'fetch' command in protocol version 2.  This feature
> enables the 'fetch' command to support requests in the form of ref names
> through a new "want-ref " parameter.  At the conclusion of
> negotiation, the server will send a list of all of the wanted references
> (as provided by "want-ref" lines) in addition to the generated packfile.

This paragraph makes it sound as if it can be combined technically,
i.e.

client:
want 01234...
want-ref master

.. usual back and forth + pack..

server:

  wanted-ref: master 2345..

What happens if the client "wants" a sha1 that is advertised,
but happens to be the same as a wanted-ref?

> Signed-off-by: Brandon Williams 
> ---
>  Documentation/config.txt|   7 ++
>  Documentation/technical/protocol-v2.txt |  29 -
>  t/t5703-upload-pack-ref-in-want.sh  | 153 
>  upload-pack.c   |  64 ++
>  4 files changed, 252 insertions(+), 1 deletion(-)
>  create mode 100755 t/t5703-upload-pack-ref-in-want.sh
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ab641bf5a..fb1dd7428 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it 
> is seen in the
>  repository-level config (this is a safety measure against fetching from
>  untrusted repositories).
>
> +uploadpack.allowRefInWant::
> +   If this option is set, `upload-pack` will support the `ref-in-want`
> +   feature of the protocol version 2 `fetch` command.  This feature
> +   is intended for the benefit of load-balanced servers which may
> +   not have the same view of what OIDs their refs point to due to
> +   replication delay.

Instead of saying who benefits, can we also say what the feature is about?
Didn't someone mention on the first round of this series, that technically
ref-in-want also provides smaller net work load as refs usually are shorter
than oids (specifically as oids will grow in the hash transisition plan later)?
Is that worth mentioning?

When using this feature is a ref advertisement still needed?

> +
>  url..insteadOf::
> Any URL that starts with this value will be rewritten to
> start, instead, with . In cases where some site serves a
> diff --git a/Documentation/technical/protocol-v2.txt 
> b/Documentation/technical/protocol-v2.txt
> index 49bda76d2..6020632b4 100644
> --- a/Documentation/technical/protocol-v2.txt
> +++ b/Documentation/technical/protocol-v2.txt
> @@ -299,12 +299,22 @@ included in the client's request:
> for use with partial clone and partial fetch operations. See
> `rev-list` for possible "filter-spec" values.
>
> +If the 'ref-in-want' feature is advertised, the following argument can
> +be included in the client's request as well as the potential addition of
> +the 'wanted-refs' section in the server's response as explained below.
> +
> +want-ref 
> +   Indicates to the server that the client wants to retrieve a
> +   particular ref, where  is the full name of a ref on the
> +   server.  A server should ignore any "want-ref " lines where
> +doesn't exist on the server.

Are patterns allowed?, e.g. I might want refs/tags/* at all times.

> @@ -319,6 +329,10 @@ header.
>  shallow = "shallow" SP obj-id
>  unshallow = "unshallow" SP obj-id
>
> +wanted-refs = PKT-LINE("wanted-refs" LF)
> + *PKT-LINE(wanted-ref LF)
> +wanted-ref = obj-id SP refname
> +
>  packfile = PKT-LINE("packfile" LF)
>*PKT-LINE(%x01-03 *%x00-ff)
>
> @@ -379,6 +393,19 @@ header.
> * This section is only included if a packfile section is also
>   included in the response.
>
> +wanted-refs section
> +   * This section is only included if the client has requested a
> + ref using a 'want-ref' line and if a packfile section is also
> + included in the response.

Is it possible to fetch non-fast-forwarded refs this way? Or specifcially
refs that were reset to an older point in history such that no pack file
is needed to transfer; would we transfer an empty pack and then
the wanted-refs section for that use case?


> +

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jeff King]

On Tue, Jun 12, 2018 at 09:29:13AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > To be honest, I could easily see an argument that I _should_ be setting
> > GIT_SSH_VARIANT to explain what my wrapper is expecting, even though it
> > happened to work before.
> 
> The way I read that message is that the patch proposed in
> 
>   
> https://public-inbox.org/git/20180103050730.ga87...@aiede.mtv.corp.google.com
> 
> is "lesser of two evils" in that it is still evil because somebody
> still has to be asked to explicitly set "variant" anyway but the
> changing what 'simple' means in the middle would mean those who did
> not have to set it now have to.  So, you should be setting it, even
> if we adopt the patch, right? ;-)

No, my wrapper _isn't_ simple. It passes most options to openssh, but
just doesn't understand the "-G" probing.  So if the default was
openssh-like instead of "simple", then that would work fine without me
setting anything, just like it did before.

Which I thought was where the discussion ended up, but perhaps I'm
misunderstanding.

> My impression is that regression "fix" does not exist---rather, it
> was a proposal (and it may make a better tradeoff than the status
> quo) to help users of older OpenSSH by inconveniencing users of
> different implementations that do -4/6/p differently from OpenSSH.

Right. That fixes the regression for openssh users, at the cost of not
help people on other implementations automatically. But those people
have to set the flag anyway, since the current behavior is "whoops, we
don't know how to support you, set the flag".

To be clear, I've already fixed my setup, and it wasn't that big a deal.
And I doubt all that many people would be affected either way. My use
case here was literally instrumenting the ssh calls of the git client as
part of a regression test suite. How many git test suites could there
possibly be? :)

So I'm OK if we just leave it as-is. It's mostly that I dug into the
thread and was left with the impression that it was an unfinished
leftover that we meant to do.

> In any case, from where I sit, I am still waiting for this offer
> by Jonathan
> 
> > It's good you caught this flaw in the detection.  Would something like
> > the following make sense?  If so, I can resend with a commit message
> > and tests tomorrow or the day after.
> 
> to be followed up ;-)

Yes, that was the part that left the impression. :)

-Peff

Re: [PATCH v2 00/31] object-store: lookup_commit

[Brandon Williams]

On 06/13, Stefan Beller wrote:
> * removed mentions of cooci patches
> * added forward declaration of commit buffer slabs.
> * dropped 3 patches that add the repository to lookup_unkonwn_object,
>   parse_commit and parse_commit_gently, but were not converting those
>   functions. We'll convert these in the next series, as this series is
>   growing big already.
> * This series can be found as branch 'object-store-lookup-commit' on github,
>   it applies on top of nd/commit-util-to-slab merged with 
> sb/object-store-grafts
> 
> v1, https://public-inbox.org/git/20180530004810.30076-1-sbel...@google.com/
> 
> This applies on the merge of nd/commit-util-to-slab and 
> sb/object-store-grafts,
> and is available at http://github.com/stefanbeller/ as branch 
> object-store-lookup-commit
> as the merge has some merge conflicts as well as syntactical conflicts 
> (upload-pack.c
> and fetch-pack.c introduce new calls of functions that would want to take a 
> repository struct
> in the object-store-grafts series)
> 
> As layed out in 
> https://public-inbox.org/git/20180517225154.9200-1-sbel...@google.com/
> this is getting close to finishing the set of object store series though the 
> last
> unfinished part of this RFC hints at new work on the plate:
> * To give this series a nice polish, we'd want to convert parse_commit, too.
>   But that requires the conversion of the new commit graph. Maybe we need
>   to split this series into 2. 
> * Once this is in good shape we can talk about converting parts of the 
> revision
>   walking code,
> * which then can be used by the submodule code as the end goal for the
>   object store series.

I've taken a look at the series and it looks good.  I'm glad we're
getting closer to this set of series being completed.  Thanks for
pushing this through :)

-- 
Brandon Williams

Re: [RFC PATCH 4/4] t/lib-httpd: sort log based on timestamp to avoid occasional failure

[Junio C Hamano]

Jeff King  writes:

>> Another alternative is to simply accept that these tests are racy, and
>> that the resulting test failures are rare enough that it isn't worth
>> the complexity of the workaround, but adding a comment to the affected
>> tests warning about the raciness is sufficient.  (But I wrote this
>> when I first saw and tracked down this failure; since then I observed
>> it four more times... :)
>
> It's definitely bugged me. I'd be happy to see some solution. I've been
> close to suggesting that reading apache logs is simply not robust, and
> we should focus our tests on the git-visible state changes (e.g., seeing
> successful requests, updated refs, etc).

Hmph, that certainly is "checking only the things that matter",
which is desirable.

> A side effect of that is that it would become a lot easier to support
> other webservers in our test scripts (though that may still be a fool's
> errand just due to the amount of custom config we seem to carry).

RE: fatal: could not reset submodule index

[Antoine W. Campagna]

On Wed, Jun 13, 2018 at 18:19, Antoine W. Campagna wrote:
> Here is the full reproduction instructions:

Newlines got mangled, making my message hard to read. Sorry.

Here is the corrected reproduction instructions:

# Create a repository
mkdir main
cd main
git init
touch main.txt
git add main.txt
git commit -a -m "Initial commit"
cd ..

# Create a second repository
mkdir sub
cd sub
git init
touch sub.txt
git add sub.txt
git commit -a -m "Initial commit of repo sub"
cd ..

# Add the second repository as submodule, on a separate branch
cd main
git branch with-submodule
git checkout with-submodule
git submodule add ../sub sub
git commit -a -m "Add submodule"

# Set main repo back to master branch (without the submodule)
git checkout master
cd ..

# Make a clone and checkout the branch
git clone main clone1
cd clone1
git checkout with-submodule
# Submodule is not automatically updated (sub folder is empty)
git submodule update --init --recursive
# Now the submodule content is there
# But I want to automatically update submodules when checking out a branch
cd ..

# Trying again, adding --recursive during clone
git clone --recursive main clone2
cd clone2
git checkout with-submodule
# Submodule is still not automatically updated (sub folder is empty)
git submodule update --init --recursive
cd ..

# Trying again, adding --recurse-submodules during checkout
git clone --recursive main clone3
cd clone3
git checkout --recurse-submodules with-submodule
# Fails with these error messages :
#   fatal: not a git repository: ../.git/modules/sub
#   fatal: could not reset submodule index
# It seems like Git tries to update the submodule but without having 
initialized the submodule cd ..

# Trying again with submodule.recurse
git config --global submodule.recurse true
git clone main clone4 cd clone4
git checkout with-submodule
# Submodule is still not automatically updated (sub folder is empty)
# It seems like submodule.recurse does not affect git clone

# Trying again with both submodule.recurse and --recursive
git config --global submodule.recurse true
git clone --recursive main clone5 cd clone5
git checkout with-submodule
# Fails with these error messages :
#   fatal: not a git repository: ../.git/modules/sub
#   fatal: could not reset submodule index
# Same issue as with "git checkout --recurse-submodules"

Re: [PATCH v2 1/8] test-pkt-line: add unpack-sideband subcommand

[Stefan Beller]

On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams  wrote:
>
> Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
> enable unpacking packet line data sent multiplexed using a sideband.
>
> Signed-off-by: Brandon Williams 
> ---
>  t/helper/test-pkt-line.c | 37 +
>  1 file changed, 37 insertions(+)
>
> diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
> index 0f19e53c7..2a551 100644
> --- a/t/helper/test-pkt-line.c
> +++ b/t/helper/test-pkt-line.c
> @@ -1,3 +1,4 @@
> +#include "cache.h"
>  #include "pkt-line.h"
>
>  static void pack_line(const char *line)
> @@ -48,6 +49,40 @@ static void unpack(void)
> }
>  }
>
> +static void unpack_sideband(void)
> +{
> +   struct packet_reader reader;
> +   packet_reader_init(, 0, NULL, 0,
> +  PACKET_READ_GENTLE_ON_EOF |
> +  PACKET_READ_CHOMP_NEWLINE);
> +
> +   while (packet_reader_read() != PACKET_READ_EOF) {
> +   int band;
> +   int fd;
> +
> +   switch (reader.status) {
> +   case PACKET_READ_EOF:
> +   break;
> +   case PACKET_READ_NORMAL:
> +   band = reader.line[0] & 0xff;
> +   if (band == 1)
> +   fd = 1;
> +   else
> +   fd = 2;
> +
> +   write_or_die(fd, reader.line+1, reader.pktlen-1);

white space around + and - ?

> +
> +   if (band == 3)
> +   die("sind-band error");

s/sind/side/ ?

What values for band are possible?
e.g. band==4 would also just write to fd=1;
but I suspect we don't want that, yet.

So maybe

band = reader.line[0] & 0xff;
if (band < 1 || band > 2)
die("unexpected side band %d", band)
fd = band;

instead?

Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits

[Kirill Smelkov]

On Thu, Jun 14, 2018 at 09:07:26AM -0700, Junio C Hamano wrote:
> Kirill Smelkov  writes:
> 
> > Jeff, thanks for corrections. I originally tried to look into invoking
> > "git tag" two times, but since git tag always creates a reference it
> > would not be semantically the same as having one referenced tag object
> > pointing to another tag object which does not have anything in refs/
> > pointing to it directly.
> 
> Well, then you could remove it after you are done, no?  I.e.
> 
>   git tag -a -m "tag to commit" tag-to-commit HEAD
>   git tag -a -m "tag to commit (1)" temp-tag HEAD~1
>   git tag -a -m "tag to tag" tag-to-tag temp-tag
>   git tag -d temp-tag
> 
> would make the temp-tag object reachable only via refs/tags/tag-to-tag
> I think.

Yes, I could remove, and I though about it originally, but to me it is
less clean compared to just creating new tag object without any
reference to it in the first place.

Kirill

Re: [RFC PATCH 0/4] Fix occasional test failures in http tests

[Jeff King]

On Thu, Jun 14, 2018 at 02:31:03PM +0200, SZEDER Gábor wrote:

> 't5561-http-backend.sh' is prone to occasional failures; luckily it's
> not 'git-http-backend's fault, but the test script is a bit racy.  I
> won't go into the details here, patch 4/4's commit message discusses
> it at length.  4/4 also fixes this issue, but I'm not particularly
> happy with that fix, the note attached to that patch explains why,
> along with possible alternatives, hence the RFC.
> 
> Even if we settle for a different fix, I think the first two patches
> are worthy cleanups on their own right.

Yes, the first two look like improvements on their own (and I left more
detailed comments on the final one). Thanks for tackling this!

-Peff

Re: [PATCH v2 05/31] tree: add repository argument to lookup_tree

[Derrick Stolee]

On 6/13/2018 7:04 PM, Stefan Beller wrote:

diff --git a/commit-graph.c b/commit-graph.c
index 71125d7cbb6..88a4b0d2a47 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -261,7 +261,7 @@ static int fill_commit_in_graph(struct commit *item, struct 
commit_graph *g, uin
item->graph_pos = pos;
  
  	hashcpy(oid.hash, commit_data);

-   item->tree = lookup_tree();
+   item->tree = lookup_tree(the_repository, );
  
  	date_high = get_be32(commit_data + g->hash_len + 8) & 0x3;

date_low = get_be32(commit_data + g->hash_len + 12);
diff --git a/commit.c b/commit.c
index d76d64d4dfc..755b8b9d94f 100644
--- a/commit.c
+++ b/commit.c
@@ -354,7 +354,7 @@ int parse_commit_buffer(struct commit *item, const void 
*buffer, unsigned long s
if (get_sha1_hex(bufptr + 5, parent.hash) < 0)
return error("bad tree pointer in commit %s",
 oid_to_hex(>object.oid));
-   item->tree = lookup_tree();
+   item->tree = lookup_tree(the_repository, );
bufptr += tree_entry_len + 1; /* "tree " + "hex sha1" + "\n" */
pptr = >parents;
  
diff --git a/fsck.c b/fsck.c

index 2d372f2a3f3..b07abb9796c 100644


I'm a bit late here, but you don't have ds/lazy-load-trees merged in 
your history, so this will conflict with 'master'. I caught this as I 
was trying to merge ds/generation-numbers with your branch.


The 'tree' member of 'struct commit' was renamed to 'maybe_tree'.

Thanks,
-Stolee

Re: [RFC PATCH 4/4] t/lib-httpd: sort log based on timestamp to avoid occasional failure

[Jeff King]

On Thu, Jun 14, 2018 at 02:31:07PM +0200, SZEDER Gábor wrote:

> The last test of 't5561-http-backend.sh', 'server request log matches
> test results' may fail occasionally, because the order of entries in
> Apache's access log doesn't match the order of requests sent in the
> previous tests, although all the right requests are there.  I saw it
> fail on Travis CI five times in the span of about half a year, when
> the order of two subsequent requests was flipped, and could trigger
> the failure with a modified Git.  However, I was unable to trigger it
> with stock Git on my machine.  Three tests in
> 't5541-http-push-smart.sh' and 't5551-http-fetch-smart.sh' check
> requests in the log the same way, so they might be prone to a similar
> occasional failure as well.

I've occasionally run into these failures on my local box, too. I'm
happy somebody is looking into it (I have before, but eventually threw
up my hands in disgust).

> Now, by default the timestamp of a log entry marks the beginning of
> the request processing, not when the log entry gets written.  Since
> our requests are sent sequentially, sorting the log entries based on
> their timestamps would ensure that their order corresponds to the
> order of our requests.

That's a reasonably clever solution. One thing I wonder, though: are we
always guaranteed that the log entries are written _at all_ before we
look at them?

I.e., could we have a situation where we make a request, the client
finishes, and then we look at the logs, but nothing has been written by
apache yet?

> I don't really like the fix in this patch.  I think an unfortunate
> clock skew during the test run could mess up the sorting added in this
> patch and cause test failure.  Or if DST or even a leap second happen
> while the test is running.  Do we care?  Anyway, this occasional test
> failure apparently happens more often than DST and leap seconds
> combined.

We could probably eliminate DST issues by consistently using UTC for the
timestamps. Leap seconds are probably infrequent enough not to worry
about. More likely is something like clock adjustment due to ntp. Those
adjustments are usually small enough not to matter, but if we're talking
microseconds, it could trigger.

> An alternative I considered was that we could decide that the order of
> requests in the access log is not important as long as all the right
> requests are there. This would inherently eliminate the raciness
> issue, but if something were to go wrong, then it would become rather
> hard to find out e.g. which request from which test has gone missing,
> especially considering that several requests are sent in more than one
> test.  We could address this by truncating the access log at the
> beginning and checking its contents at the end of each test sending
> requests.  Unfortunately, this would raise additional difficulties,
> because all requests in t5561 come from tests included from
> 't556x-common', i.e. from tests shared with
> 't5560-http-backend-noserver.sh', which as its name suggests doesn't
> run Apache and doesn't have an access log at all.

What if the test script provides the in-order expectation, but we check
only the unordered version (by sorting both actual and expected output
on the fly)? That would give us a more relaxed check most of the time,
but somebody digging in to a failure could run the ordered diff (or we
could even show it automatically on failure instead of just using
test_cmp).

> Another alternative is to simply accept that these tests are racy, and
> that the resulting test failures are rare enough that it isn't worth
> the complexity of the workaround, but adding a comment to the affected
> tests warning about the raciness is sufficient.  (But I wrote this
> when I first saw and tracked down this failure; since then I observed
> it four more times... :)

It's definitely bugged me. I'd be happy to see some solution. I've been
close to suggesting that reading apache logs is simply not robust, and
we should focus our tests on the git-visible state changes (e.g., seeing
successful requests, updated refs, etc).

A side effect of that is that it would become a lot easier to support
other webservers in our test scripts (though that may still be a fool's
errand just due to the amount of custom config we seem to carry).

> Apache doesn't maintain 2.2 anymore; the final maintenance release
> 2.2.34 was released in July 2017, almost a year ago.  OTOH, our
> 't/lib-httpd/apache.conf' contains a couple of IfVersion directives
> dealing with versions <2.4, and one even with <2.1.  How much do we
> actually care about these unmaintained Apache versions, and how much
> of this is just bitrotting?

I strongly suspect bitrotting. It looks like most of the "< 2.4"
directives are from 5 years ago (when Debian switched to 2.4 by
default), so even long-term stable systems 

Re: [PATCH] t3200: clarify description of --set-upstream test

[Junio C Hamano]

Kaartic Sivaraam  writes:

> Support for the --set-upstream option was removed in 52668846ea
> (builtin/branch: stop supporting the "--set-upstream" option,
> 2017-08-17). The change did not completely remove the command
> due to an issue noted in the commit's log message.
>
> So, a test was added to ensure that a command which uses the
> '--set-upstream' option fails and doesn't silently act as an alias
> for the '--set-upstream-to' option due to option parsing features.
>
> To avoid confusion, clarify that the option is an unsupported one
> in the corresponding test description.
>
> Signed-off-by: Kaartic Sivaraam 
> ---
>  t/t3200-branch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

The above description is much clearer than what the test title after
the patch gives its readers.

It is technically correct to call --set-upstream "unsupported", but
the reason why we want to see it fail is not because it is
unsupported, but because we actively interfere with the usual
"unique prefix" logic parse-options API gives its users and make it
not to trigger the longer-and-unique --set-upstream-to logic.

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 6c0b7ea4a..d14de82ba 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -884,7 +884,7 @@ test_expect_success 'test --unset-upstream on a 
> particular branch' '
>   test_must_fail git config branch.my14.merge
>  '
>  
> -test_expect_success '--set-upstream fails' '
> +test_expect_success 'unsupported option --set-upstream fails' '

In other words, I am wondering if s/unsupported/disabled/ makes it
even more clear what is going on here.

>  test_must_fail git branch --set-upstream origin/master
>  '

Re: [PATCH v2 8/8] negotiator/default: use better style in comments

[Brandon Williams]

On 06/06, Jonathan Tan wrote:
> Signed-off-by: Jonathan Tan 
> ---
>  negotiator/default.c | 14 ++
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/negotiator/default.c b/negotiator/default.c
> index b8f45cf78..a9e52c943 100644
> --- a/negotiator/default.c
> +++ b/negotiator/default.c
> @@ -46,11 +46,10 @@ static int clear_marks(const char *refname, const struct 
> object_id *oid,
>  }
>  
>  /*
> -   This function marks a rev and its ancestors as common.
> -   In some cases, it is desirable to mark only the ancestors (for example
> -   when only the server does not yet know that they are common).
> -*/
> -
> + * This function marks a rev and its ancestors as common.
> + * In some cases, it is desirable to mark only the ancestors (for example
> + * when only the server does not yet know that they are common).
> + */
>  static void mark_common(struct data *data, struct commit *commit,
>   int ancestors_only, int dont_parse)
>  {
> @@ -80,9 +79,8 @@ static void mark_common(struct data *data, struct commit 
> *commit,
>  }
>  
>  /*
> -  Get the next rev to send, ignoring the common.
> -*/
> -
> + * Get the next rev to send, ignoring the common.
> + */
>  static const struct object_id *get_rev(struct data *data)
>  {
>   struct commit *commit = NULL;
> -- 
> 2.17.0.768.g1526ddbba1.dirty
> 

Don't have this be a separate patch, squash it into the previous patch.

-- 
Brandon Williams

Re: [PATCH v2 5/8] fetch-pack: make negotiation-related vars local

[Brandon Williams]

On 06/06, Jonathan Tan wrote:
> Reduce the number of global variables by making the priority queue and
> the count of non-common commits in it local, passing them as a struct to
> various functions where necessary.
> 
> This also helps in the case that fetch_pack() is invoked twice in the
> same process (when tag following is required when using a transport that
> does not support tag following), in that different priority queues will
> now be used in each invocation, instead of reusing the possibly
> non-empty one.
> 
> The struct containing these variables is named "data" to ease review of
> a subsequent patch in this series - in that patch, this struct
> definition and several functions will be moved to a negotiation-specific
> file, and this allows the move to be verbatim.

Ok I spoke too soon in my other mail.  You've cleaned this up by moving
these things to local stack vars but you still don't clear the priority
queue meaning that memory is now leaking.  I think you should insert a
call to clear the priority queue in the earlier patch before returning
from the fetch code.  This way you have a clear place to update that
call to clear in this patch so that you don't forget any memory leaks.

I know this may still change later on in this series but it should still
get taken care of to make the review process easier.

> 
> Signed-off-by: Jonathan Tan 
> ---
>  fetch-pack.c | 104 +--
>  1 file changed, 59 insertions(+), 45 deletions(-)
> 
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 114207b8e..c31644bb9 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -50,8 +50,12 @@ static int marked;
>   */
>  #define MAX_IN_VAIN 256
>  
> -static struct prio_queue rev_list = { compare_commits_by_commit_date };
> -static int non_common_revs, multi_ack, use_sideband;
> +struct data {
> + struct prio_queue rev_list;
> + int non_common_revs;
> +};
> +
> +static int multi_ack, use_sideband;
>  /* Allow specifying sha1 if it is a ref tip. */
>  #define ALLOW_TIP_SHA1   01
>  /* Allow request of a sha1 if it is reachable from a ref (possibly hidden 
> ref). */
> @@ -93,7 +97,8 @@ static void cache_one_alternate(const char *refname,
>   cache->items[cache->nr++] = obj;
>  }
>  
> -static void for_each_cached_alternate(void (*cb)(struct object *))
> +static void for_each_cached_alternate(struct data *data,
> +   void (*cb)(struct data *, struct object 
> *))
>  {
>   static int initialized;
>   static struct alternate_object_cache cache;
> @@ -105,10 +110,10 @@ static void for_each_cached_alternate(void (*cb)(struct 
> object *))
>   }
>  
>   for (i = 0; i < cache.nr; i++)
> - cb(cache.items[i]);
> + cb(data, cache.items[i]);
>  }
>  
> -static void rev_list_push(struct commit *commit, int mark)
> +static void rev_list_push(struct data *data, struct commit *commit, int mark)
>  {
>   if (!(commit->object.flags & mark)) {
>   commit->object.flags |= mark;
> @@ -116,19 +121,20 @@ static void rev_list_push(struct commit *commit, int 
> mark)
>   if (parse_commit(commit))
>   return;
>  
> - prio_queue_put(_list, commit);
> + prio_queue_put(>rev_list, commit);
>  
>   if (!(commit->object.flags & COMMON))
> - non_common_revs++;
> + data->non_common_revs++;
>   }
>  }
>  
> -static int rev_list_insert_ref(const char *refname, const struct object_id 
> *oid)
> +static int rev_list_insert_ref(struct data *data, const char *refname,
> +const struct object_id *oid)
>  {
>   struct object *o = deref_tag(parse_object(oid), refname, 0);
>  
>   if (o && o->type == OBJ_COMMIT)
> - rev_list_push((struct commit *)o, SEEN);
> + rev_list_push(data, (struct commit *)o, SEEN);
>  
>   return 0;
>  }
> @@ -136,7 +142,7 @@ static int rev_list_insert_ref(const char *refname, const 
> struct object_id *oid)
>  static int rev_list_insert_ref_oid(const char *refname, const struct 
> object_id *oid,
>  int flag, void *cb_data)
>  {
> - return rev_list_insert_ref(refname, oid);
> + return rev_list_insert_ref(cb_data, refname, oid);
>  }
>  
>  static int clear_marks(const char *refname, const struct object_id *oid,
> @@ -156,7 +162,7 @@ static int clear_marks(const char *refname, const struct 
> object_id *oid,
> when only the server does not yet know that they are common).
>  */
>  
> -static void mark_common(struct commit *commit,
> +static void mark_common(struct data *data, struct commit *commit,
>   int ancestors_only, int dont_parse)
>  {
>   if (commit != NULL && !(commit->object.flags & COMMON)) {
> @@ -166,12 +172,12 @@ static void mark_common(struct commit *commit,
>   o->flags |= COMMON;
>  
>   if (!(o->flags & 

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Junio C Hamano]

René Scharfe  writes:

>
> This adds a minor memory leak; fix below.
>
> -- >8 --
> Subject: [PATCH] blame: release string_list after use in parse_color_fields()
>
> Signed-off-by: Rene Scharfe 
> ---

Thanks.  Will apply.

>  builtin/blame.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4202584f97..3295718841 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -411,6 +411,7 @@ static void parse_color_fields(const char *s)
>   die (_("must end with a color"));
>  
>   colorfield[colorfield_nr].hop = TIME_MAX;
> + string_list_clear(, 0);
>  }
>  
>  static void setup_default_color_by_age(void)

[PATCH] t5526: test recursive submodules when fetching moved submodules

[Stefan Beller]

The topic merged in 0c7ecb7c311 (Merge branch 'sb/submodule-move-nested',
2018-05-08) provided support for moving nested submodules.

Remove the NEEDSWORK comment and implement the nested submodules test as
the comment hinted at.

Signed-off-by: Stefan Beller 
---

I found this when digging around for the previous patch.

Thanks,
Stefan

 t/t5526-fetch-submodules.sh | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 9cc4b569c05..359e03ff836 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -574,11 +574,7 @@ test_expect_success "fetch new commits when submodule got 
renamed" '
git clone . downstream_rename &&
(
cd downstream_rename &&
-   git submodule update --init &&
-# NEEDSWORK: we omitted --recursive for the submodule update here since
-# that does not work. See test 7001 for mv "moving nested submodules"
-# for details. Once that is fixed we should add the --recursive option
-# here.
+   git submodule update --init --recursive &&
git checkout -b rename &&
git mv submodule submodule_renamed &&
(
-- 
2.18.0.rc1.244.gcf134e6275-goog

Re: [PATCH 24/30] merge-recursive: Add computation of collisions due to dir rename & merging

[Junio C Hamano]

René Scharfe  writes:

> The value of PATH_MAX is platform-dependent, so it's easy to exceed when
> doing cross-platform development.  It's also not a hard limit on most
> operating systems, not even on Windows.  Further reading:
>
>https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html
>
> So using a fixed buffer is not a good idea, and writing to it without
> checking is dangerous.  Here's a fix:
>
> -- >8 --
> Subject: [PATCH] merge-recursive: use xstrdup() instead of fixed buffer
>
> Paths can be longer than PATH_MAX.  Avoid a buffer overrun in
> check_dir_renamed() by using xstrdup() to make a private copy safely.
>
> Signed-off-by: Rene Scharfe 
> ---

Thanks.  Makes sense.

>  merge-recursive.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index ac27abbd4c..db708176c5 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2211,18 +2211,18 @@ static struct hashmap *get_directory_renames(struct 
> diff_queue_struct *pairs,
>  static struct dir_rename_entry *check_dir_renamed(const char *path,
> struct hashmap *dir_renames)
>  {
> - char temp[PATH_MAX];
> + char *temp = xstrdup(path);
>   char *end;
> - struct dir_rename_entry *entry;
> + struct dir_rename_entry *entry = NULL;;
>  
> - strcpy(temp, path);
>   while ((end = strrchr(temp, '/'))) {
>   *end = '\0';
>   entry = dir_rename_find_entry(dir_renames, temp);
>   if (entry)
> - return entry;
> + break;
>   }
> - return NULL;
> + free(temp);
> + return entry;
>  }
>  
>  static void compute_collisions(struct hashmap *collisions,

Re: [PATCH v2 3/8] fetch-pack: directly end negotiation if ACK ready

[Brandon Williams]

On 06/14, Brandon Williams wrote:
> On 06/06, Jonathan Tan wrote:
> > When "ACK %s ready" is received, find_common() clears rev_list in an
> > attempt to stop further "have" lines from being sent [1]. It is much
> > more readable to explicitly break from the loop instead, so do this.
> > 
> > This means that the memory in priority queue will be reclaimed only upon
> > program exit, similar to the cases in which "ACK %s ready" is not
> 
> This seems fine for now though ideally we would remove the global
> priority queue and have it live on the stack somewhere in the call stack
> and it could be cleared unconditionally before returning.

Wait looks like a later commit does just this, maybe stick in a comment
saying a later patch is cleaning this up.

> 
> > received. (A related problem occurs when do_fetch_pack() is invoked a
> > second time in the same process with a possibly non-empty priority
> > queue, but this will be solved in a subsequent patch in this patch set.)
> > 
> > [1] The rationale is further described in the originating commit
> > f2cba9299b ("fetch-pack: Finish negotation if remote replies "ACK %s
> > ready"", 2011-03-14).
> > 
> > Signed-off-by: Jonathan Tan 
> > ---
> >  fetch-pack.c | 7 +++
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index 2812499a5..09f5c83c4 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -517,10 +517,8 @@ static int find_common(struct fetch_pack_args *args,
> > mark_common(commit, 0, 1);
> > retval = 0;
> > got_continue = 1;
> > -   if (ack == ACK_ready) {
> > -   clear_prio_queue(_list);
> > +   if (ack == ACK_ready)
> > got_ready = 1;
> > -   }
> > break;
> > }
> > }
> > @@ -530,6 +528,8 @@ static int find_common(struct fetch_pack_args *args,
> > print_verbose(args, _("giving up"));
> > break; /* give up */
> > }
> > +   if (got_ready)
> > +   break;
> > }
> > }
> >  done:
> > @@ -1300,7 +1300,6 @@ static int process_acks(struct packet_reader *reader, 
> > struct oidset *common)
> > }
> >  
> > if (!strcmp(reader->line, "ready")) {
> > -   clear_prio_queue(_list);
> > received_ready = 1;
> > continue;
> > }
> > -- 
> > 2.17.0.768.g1526ddbba1.dirty
> > 
> 
> -- 
> Brandon Williams

-- 
Brandon Williams

Re: [PATCH v2 4/8] fetch-pack: use ref adv. to prune "have" sent

[Brandon Williams]

On 06/06, Jonathan Tan wrote:
> In negotiation using protocol v2, fetch-pack sometimes does not make
> full use of the information obtained in the ref advertisement:
> specifically, that if the server advertises a commit that the client
> also has, the client never needs to inform the server that it has the
> commit's parents, since it can just tell the server that it has the
> advertised commit and it knows that the server can and will infer the
> rest.
> 
> This is because, in do_fetch_pack_v2(), rev_list_insert_ref_oid() is
> invoked before everything_local(). This means that if we have a commit
> that is both our ref and their ref, it would be enqueued by
> rev_list_insert_ref_oid() as SEEN, and since it is thus already SEEN,
> everything_local() would not enqueue it.

Thanks for fixing this slight issue with v2.  Though maybe we need to
update the commit message here because a previous patch in this version
of the series broke up everything_local() into various parts so that it
is no longer responsible for enqueueing commits?

> 
> If everything_local() were invoked first, as it is in do_fetch_pack()
> for protocol v0, then everything_local() would enqueue it with
> COMMON_REF | SEEN. The addition of COMMON_REF ensures that its parents
> are not sent as "have" lines.
> 
> Change the order in do_fetch_pack_v2() to be consistent with
> do_fetch_pack(), and to avoid sending unnecessary "have" lines.
> 
> Signed-off-by: Jonathan Tan 
> ---

-- 
Brandon Williams

[PATCH] submodule: fix NULL correctness in renamed broken submodules

[Stefan Beller]

When fetching with recursing into submodules, the fetch logic inspects
the superproject which submodules actually need to be fetched. This is
tricky for submodules that were renamed in the fetched range of commits.
This was implemented in c68f8375760 (implement fetching of moved
submodules, 2017-10-16), and this patch fixes a mistake in the logic
there.

When the warning is printed, the `name` might be NULL as
default_name_or_path can return NULL, so fix the warning to use the path
as obtained from the diff machinery, as that is not NULL.

While at it, make sure we only attempt to load the submodule if a git
directory of the submodule is found as default_name_or_path will return
NULL in case the git directory cannot be found. Note that passing NULL
to submodule_from_name is just a semantic error, as submodule_from_name
accepts NULL as a value, but then the return value is not the submodule
that was asked for, but some arbitrary other submodule. (Cf. 'config_from'
in submodule-config.c: "If any parameter except the cache is a NULL
pointer just return the first submodule. Can be used to check whether
there are any submodules parsed.")

Reported-by: Duy Nguyen 
Helped-by: Duy Nguyen 
Helped-by: Heiko Voigt 
Signed-off-by: Stefan Beller 
---
 submodule.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 939d6870ecd..0998ea23458 100644
--- a/submodule.c
+++ b/submodule.c
@@ -740,12 +740,14 @@ static void collect_changed_submodules_cb(struct 
diff_queue_struct *q,
else {
name = default_name_or_path(p->two->path);
/* make sure name does not collide with existing one */
-   submodule = submodule_from_name(the_repository, 
commit_oid, name);
+   if (name)
+   submodule = submodule_from_name(the_repository,
+   commit_oid, 
name);
if (submodule) {
warning("Submodule in commit %s at path: "
"'%s' collides with a submodule named "
"the same. Skipping it.",
-   oid_to_hex(commit_oid), name);
+   oid_to_hex(commit_oid), p->two->path);
name = NULL;
}
}
-- 
2.18.0.rc1.244.gcf134e6275-goog

Re: [PATCH v2 3/8] fetch-pack: directly end negotiation if ACK ready

[Brandon Williams]

On 06/06, Jonathan Tan wrote:
> When "ACK %s ready" is received, find_common() clears rev_list in an
> attempt to stop further "have" lines from being sent [1]. It is much
> more readable to explicitly break from the loop instead, so do this.
> 
> This means that the memory in priority queue will be reclaimed only upon
> program exit, similar to the cases in which "ACK %s ready" is not

This seems fine for now though ideally we would remove the global
priority queue and have it live on the stack somewhere in the call stack
and it could be cleared unconditionally before returning.

> received. (A related problem occurs when do_fetch_pack() is invoked a
> second time in the same process with a possibly non-empty priority
> queue, but this will be solved in a subsequent patch in this patch set.)
> 
> [1] The rationale is further described in the originating commit
> f2cba9299b ("fetch-pack: Finish negotation if remote replies "ACK %s
> ready"", 2011-03-14).
> 
> Signed-off-by: Jonathan Tan 
> ---
>  fetch-pack.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 2812499a5..09f5c83c4 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -517,10 +517,8 @@ static int find_common(struct fetch_pack_args *args,
>   mark_common(commit, 0, 1);
>   retval = 0;
>   got_continue = 1;
> - if (ack == ACK_ready) {
> - clear_prio_queue(_list);
> + if (ack == ACK_ready)
>   got_ready = 1;
> - }
>   break;
>   }
>   }
> @@ -530,6 +528,8 @@ static int find_common(struct fetch_pack_args *args,
>   print_verbose(args, _("giving up"));
>   break; /* give up */
>   }
> + if (got_ready)
> + break;
>   }
>   }
>  done:
> @@ -1300,7 +1300,6 @@ static int process_acks(struct packet_reader *reader, 
> struct oidset *common)
>   }
>  
>   if (!strcmp(reader->line, "ready")) {
> - clear_prio_queue(_list);
>   received_ready = 1;
>   continue;
>   }
> -- 
> 2.17.0.768.g1526ddbba1.dirty
> 

-- 
Brandon Williams

Re: [PATCH v2 1/8] fetch-pack: split up everything_local()

[Brandon Williams]

On 06/06, Jonathan Tan wrote:
> The function everything_local(), despite its name, also (1) marks
> commits as COMPLETE and COMMON_REF and (2) invokes filter_refs() as
> important side effects. Extract (1) into its own function
> (mark_complete_and_common_ref()) and remove
> (2).
> 
> The restoring of save_commit_buffer, which was introduced in a1c6d7c1a7
> ("fetch-pack: restore save_commit_buffer after use", 2017-12-08), is a
> concern of the parse_object() call in mark_complete_and_common_ref(), so
> it has been moved from the end of everything_local() to the end of
> mark_complete_and_common_ref().

Thanks, this is much cleaner.

> 
> Signed-off-by: Jonathan Tan 
> ---
>  fetch-pack.c | 39 ++-
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/fetch-pack.c b/fetch-pack.c
> index a320ce987..5c87bb8bb 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -734,12 +734,20 @@ static int add_loose_objects_to_set(const struct 
> object_id *oid,
>   return 0;
>  }
>  
> -static int everything_local(struct fetch_pack_args *args,
> - struct ref **refs,
> - struct ref **sought, int nr_sought)
> +/*
> + * Mark recent commits available locally and reachable from a local ref as
> + * COMPLETE. If args->no_dependents is false, also mark COMPLETE remote refs 
> as
> + * COMMON_REF (otherwise, we are not planning to participate in negotiation, 
> and
> + * thus do not need COMMON_REF marks).
> + *
> + * The cutoff time for recency is determined by this heuristic: it is the
> + * earliest commit time of the objects in refs that are commits and that we 
> know
> + * the commit time of.
> + */
> +static void mark_complete_and_common_ref(struct fetch_pack_args *args,
> +  struct ref **refs)
>  {
>   struct ref *ref;
> - int retval;
>   int old_save_commit_buffer = save_commit_buffer;
>   timestamp_t cutoff = 0;
>   struct oidset loose_oid_set = OIDSET_INIT;
> @@ -812,7 +820,18 @@ static int everything_local(struct fetch_pack_args *args,
>   }
>   }
>  
> - filter_refs(args, refs, sought, nr_sought);
> + save_commit_buffer = old_save_commit_buffer;
> +}
> +
> +/*
> + * Returns 1 if every object pointed to by the given remote refs is available
> + * locally and reachable from a local ref, and 0 otherwise.
> + */
> +static int everything_local(struct fetch_pack_args *args,
> + struct ref **refs)
> +{
> + struct ref *ref;
> + int retval;
>  
>   for (retval = 1, ref = *refs; ref ; ref = ref->next) {
>   const struct object_id *remote = >old_oid;
> @@ -829,8 +848,6 @@ static int everything_local(struct fetch_pack_args *args,
> ref->name);
>   }
>  
> - save_commit_buffer = old_save_commit_buffer;
> -
>   return retval;
>  }
>  
> @@ -1053,7 +1070,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args 
> *args,
>   if (!server_supports("deepen-relative") && args->deepen_relative)
>   die(_("Server does not support --deepen"));
>  
> - if (everything_local(args, , sought, nr_sought)) {
> + mark_complete_and_common_ref(args, );
> + filter_refs(args, , sought, nr_sought);
> + if (everything_local(args, )) {
>   packet_flush(fd[1]);
>   goto all_done;
>   }
> @@ -1377,7 +1396,9 @@ static struct ref *do_fetch_pack_v2(struct 
> fetch_pack_args *args,
>   for_each_cached_alternate(insert_one_alternate_object);
>  
>   /* Filter 'ref' by 'sought' and those that aren't local 
> */
> - if (everything_local(args, , sought, nr_sought))
> + mark_complete_and_common_ref(args, );
> + filter_refs(args, , sought, nr_sought);
> + if (everything_local(args, ))
>   state = FETCH_DONE;
>   else
>   state = FETCH_SEND_REQUEST;
> -- 
> 2.17.0.768.g1526ddbba1.dirty
> 

-- 
Brandon Williams

Re: [PATCH v4 00/23] Fix incorrect use of the_index

[Duy Nguyen]

On Mon, Jun 11, 2018 at 06:49:00PM +0200, Duy Nguyen wrote:
> On Mon, Jun 11, 2018 at 6:44 PM Elijah Newren  wrote:
> > > What about merge-recursive.c? Given that this whole thing will take
> > > many release cycles to finish, your work may get merged before mine
> > > and I could do the conversion now (and resolve conflicts and resubmit
> > > later). Of course if you like to keep merge-recursive.c the_index-free
> > > now, I will not stop you ;-)
> >
> > I was just worried that since I was making changes in
> > merge-recursive.c that it'd cause you conflict pain, so I offered to
> > convert it.  If that pain doesn't bother you, feel free to do the
> > conversion and we'll just work through the minor conflicts as we go.
> > Besides, sounds like you've converted nearly all of git, so it may
> > make sense to just keep it together in one big series.
> 
> OK let's just "quickly" get your series in then. I still have a few
> files to go and a couple more places to look back and ponder. I'm in
> no hurry to convert merge-recursive now :D

Actually could you quickly check if I pass the right index in the
following patch? I realize you only have one extra index, but I don't
know if it gets passed elsewhere and in some function I'm supposed to
use another index than the default "o->repo->index". Sometimes
o->repo->index is not the correct answer.

It takes many patches to get to this stage, so if you diff context
looks so strange to you, just ignore this for now. I'll eventually
send this one out in a series.

PS. I put a 'struct repository *' in 'struct merge_options' instead
because it turns out we do need full repo struct in some function.

-- 8< --
diff --git a/builtin/am.c b/builtin/am.c
index 3961878871..5d2ff9aa8c 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1592,7 +1592,7 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
 * changes.
 */
 
-   init_merge_options();
+   init_merge_options(, the_repository);
 
o.branch1 = "HEAD";
their_tree_name = xstrfmt("%.*s", linelen(state->msg), state->msg);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 528c6de7e1..4fd53cec6e 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -570,7 +570,7 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
 * a pain; plumb in an option to set
 * o.renormalize?
 */
-   init_merge_options();
+   init_merge_options(, the_repository);
o.verbosity = 0;
work = write_tree_from_memory();
 
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 0dd9021958..c4a9e575d6 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -28,7 +28,7 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
struct merge_options o;
struct commit *result;
 
-   init_merge_options();
+   init_merge_options(, the_repository);
if (argv[0] && ends_with(argv[0], "-subtree"))
o.subtree_shift = "";
 
diff --git a/builtin/merge.c b/builtin/merge.c
index db460a35cf..2ebd939b76 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -671,7 +671,7 @@ static int try_merge_strategy(const char *strategy, struct 
commit_list *common,
return 2;
}
 
-   init_merge_options();
+   init_merge_options(, the_repository);
if (!strcmp(strategy, "subtree"))
o.subtree_shift = "";
 
diff --git a/merge-recursive.c b/merge-recursive.c
index a296ba3874..fd6556b6de 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -309,26 +309,29 @@ static void output_commit_title(struct merge_options *o, 
struct commit *commit)
 }
 
 static int add_cacheinfo(struct merge_options *o,
-   unsigned int mode, const struct object_id *oid,
-   const char *path, int stage, int refresh, int options)
+unsigned int mode,
+const struct object_id *oid,
+const char *path, int stage,
+int refresh, int options)
 {
+   struct index_state *istate = o->repo->index;
struct cache_entry *ce;
int ret;
 
-   ce = make_index_entry(_index, mode, oid ? oid->hash : null_sha1, 
path, stage, 0);
+   ce = make_index_entry(istate, mode, oid ? oid->hash : null_sha1, path, 
stage, 0);
if (!ce)
return err(o, _("add_cacheinfo failed for path '%s'; merge 
aborting."), path);
 
-   ret = add_index_entry(_index, ce, options);
+   ret = add_index_entry(istate, ce, options);
if (refresh) {
struct cache_entry *nce;
 
-   nce = refresh_index_entry(_index, ce,
+   nce = refresh_index_entry(istate, ce,
  

Re: [RFC PATCH 0/4] Fix occasional test failures in http tests

[Junio C Hamano]

SZEDER Gábor  writes:

> 't5561-http-backend.sh' is prone to occasional failures; luckily it's
> not 'git-http-backend's fault, but the test script is a bit racy.  I
> won't go into the details here, patch 4/4's commit message discusses
> it at length.  4/4 also fixes this issue, but I'm not particularly
> happy with that fix, the note attached to that patch explains why,
> along with possible alternatives, hence the RFC.

Interesting.

> Even if we settle for a different fix, I think the first two patches
> are worthy cleanups on their own right.

Yeah, the first two are unquestionally good.  

The sed expression of the second one could use sq around (instead of
dq) to lose hard-to-read backslash quoting from there, but it
probably is OK to keep the second one a pure "refactoring" patch,
and leave it to a separate "further cleanup" patch to make such a
change.

> SZEDER Gábor (4):
>   t5541: avoid empty line when truncating access log
>   t/lib-httpd: add the strip_access_log() helper function
>   t/lib-httpd: add minor and patch version to $HTTPD_VERSION
>   t/lib-httpd: sort log based on timestamp to avoid occasional failure
>
>  t/lib-httpd.sh  | 24 ++--
>  t/lib-httpd/apache.conf |  5 +
>  t/t5541-http-push-smart.sh  | 17 +++--
>  t/t5551-http-fetch-smart.sh |  7 +--
>  t/t5561-http-backend.sh |  7 +--
>  5 files changed, 32 insertions(+), 28 deletions(-)

Re: [RFC PATCH 4/4] t/lib-httpd: sort log based on timestamp to avoid occasional failure

[SZEDER Gábor]

On Thu, Jun 14, 2018 at 2:31 PM, SZEDER Gábor  wrote:

> When a test sends a HTTP request, it can continue execution after
> 'git-http-backend' fulfilled that request, but Apache writes the
> corresponding access log entry only after 'git-http-backend' exited.
> Some time inevitably passes between fulfilling the request and writing
> the log entry, and, under unfavourable circumstances, enough time
> might pass for the subsequent request to be sent and fulfilled, and
> then Apache writes access log entries racily.
>
> This effect can be exacerbated by adding a bit of variable delay after
> the request is fulfilled but before 'git-http-backend' exits, e.g.
> like this:
>
>   diff --git a/http-backend.c b/http-backend.c
>   index f3dc218b2..bbf4c125b 100644
>   --- a/http-backend.c
>   +++ b/http-backend.c
>   @@ -709,5 +709,7 @@ int cmd_main(int argc, const char **argv)
>max_request_buffer);
>
> cmd->imp(, cmd_arg);
>   + if (getpid() % 2)
>   + sleep(1);
> return 0;
>}
>
> This delay considerably increases the chances of log entries being
> written out of order, and in turn makes t5561's last test fail almost
> every time.  Alas, it doesn't seem to be enough to trigger a similar
> failure in t5541 and t5551.

A bit of addendum here:

There are basically two "kinds" of requests:

  1. requests asking for regular files, e.g.:

 GET  /smart/repo.git/HEAD HTTP/1.1 200
 GET  /smart/repo.git/info/refs HTTP/1.1 200
 GET  /smart/repo.git/objects/info/packs HTTP/1.1 200

  2. requests asking for 'git-receive-pack' or 'git-upload-pack',
 e.g.:

 GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
 POST /smart/repo.git/git-receive-pack HTTP/1.1 200 -

This artificial variable delay shuffles only the access log entries
written for the first kind of requests, but doesn't appear to have any
effect on the order of log entries written for the second kind of
requests.

t5561 sends both kinds of requests, so this variable delay is
sufficient to make the test fail most of the time.

t5541 and t5551, however, send only requests of the second kind, so
that's why this variable delay can't trigger a similar failure in
these test scripts.

I haven't investigated any further what else would be necessary to
trigger a failure in t5541 and t5551.

Re: [PATCH v2 00/31] object-store: lookup_commit

[Duy Nguyen]

On Thu, Jun 14, 2018 at 1:08 AM Stefan Beller  wrote:
>
> * removed mentions of cooci patches
> * added forward declaration of commit buffer slabs.
> * dropped 3 patches that add the repository to lookup_unkonwn_object,
>   parse_commit and parse_commit_gently, but were not converting those
>   functions. We'll convert these in the next series, as this series is
>   growing big already.
> * This series can be found as branch 'object-store-lookup-commit' on github,
>   it applies on top of nd/commit-util-to-slab merged with 
> sb/object-store-grafts
>
> v1, https://public-inbox.org/git/20180530004810.30076-1-sbel...@google.com/
>
> This applies on the merge of nd/commit-util-to-slab and 
> sb/object-store-grafts,
> and is available at http://github.com/stefanbeller/ as branch 
> object-store-lookup-commit

I only looked at whole-series diff, not individual patches (sorry).
Besides the one comment I made elsewhere, it looks good.

You don't even need to reroll this series to address that one. A
follow up patch whenever you feel like it is fine, or I'll get to it
eventually (after the_index is gone, I plan to kill the_repository
outside builtin/ too, for the same reason).
-- 
Duy

Re: [PATCH 10/35] commit: add repository argument to lookup_commit

[Duy Nguyen]

On Wed, May 30, 2018 at 2:51 AM Stefan Beller  wrote:
> diff --git a/shallow.c b/shallow.c
> index 9bb07a56dca..60fe1fe1e58 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -31,7 +31,7 @@ int register_shallow(struct repository *r, const struct 
> object_id *oid)
>  {
> struct commit_graft *graft =
> xmalloc(sizeof(struct commit_graft));
> -   struct commit *commit = lookup_commit(oid);
> +   struct commit *commit = lookup_commit(the_repository, oid);

This looks wrong. register_shallow() has struct repository argument
'r' and it should be used here instead.

If this is a mechanical conversion, I will also be happy that the
switch from the_repo to r is done in a separate patch.

FYI I noticed this because I'm in a quest to kill the_index by passing
'struct index_state *' throughout library code, and sometimes I pass
'struct repository *' instead when I see that code uses more things
that just the index.  And I have started to replace the_repository in
some places with a function argument.

If some of my patches come first while you have not finished
repository conversion (very likely), you and I will have to pay
attention to this more often.
-- 
Duy

Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits

[Junio C Hamano]

Kirill Smelkov  writes:

> Jeff, thanks for corrections. I originally tried to look into invoking
> "git tag" two times, but since git tag always creates a reference it
> would not be semantically the same as having one referenced tag object
> pointing to another tag object which does not have anything in refs/
> pointing to it directly.

Well, then you could remove it after you are done, no?  I.e.

git tag -a -m "tag to commit" tag-to-commit HEAD
git tag -a -m "tag to commit (1)" temp-tag HEAD~1
git tag -a -m "tag to tag" tag-to-tag temp-tag
git tag -d temp-tag

would make the temp-tag object reachable only via refs/tags/tag-to-tag
I think.

Re: [PATCH 19/20] abbrev: support relative abbrev values

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Wed, Jun 13 2018, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  writes:
>>
>>> E.g. here's a breakdown of my dotfiles repo:
>>>
>>> $ git -c core.abbrev=4 log  --pretty=format:%h|perl -nE 'chomp;say 
>>> length'|sort|uniq -c|sort -nr
>>> 784 4
>>>  59 5
>>>   7 6
>>>
>>> I don't have a single commit that needs 7 characters, yet that's our
>>> default. This is a sane trade-off for the kernel, but for something
>>> that's just a toy or something you're playing around with something
>>> shorter can make sense.
>>>
>>> SHA-1s aren't just written down, but also e.g. remembered in wetware
>>> short-term memory.
>>
>> That's a fine example of what I called "supporting absurdly small
>> absolute values like 4"; I still do not see why you want "negative
>> relative values" from that example.
>
> Because hardcoding -2 is very different than setting it to 5, because
> the -2 will scale to the size of the repository, but 5 is just 7-2 where
> 7 is our default value.
>
> So, in general if you want less future proof hashes by some
> probabilistic metric (whether you use core.validateAbbrev or not) you'd
> use -2 or -3, just like you might use +2 or +3 if you'd like to have
> more future-proof hashes (especially with core.validateAbbrev=true).

That still does not make much sense to me at all.

I do agree that something shorter than the default 7 may be more
appropriate for our wetware short-term memory, and it would make
sense to grow the "riskier to collide than the default heuristics
but more memorable" variant as the project grows, _ONLY_ _IF_ our
wetware short-term memory scales with the project we happen to be
working on.  But our wetware does not scale with the project we work
on; at least mine does not.

So...

Re: BUG: submodule code prints '(null)'

[Duy Nguyen]

On Thu, Jun 14, 2018 at 5:15 PM Heiko Voigt  wrote:
> ...
> Would you want to update your patch? Or should I put one on top?

I think it's better that you make a proper patch. You can provide
explanation and all. I am more like a bug reporter :)
-- 
Duy

Re: BUG: submodule code prints '(null)'

[Heiko Voigt]

On Mon, Jun 11, 2018 at 03:56:16PM -0700, Stefan Beller wrote:
> On Sat, Jun 9, 2018 at 4:04 AM Duy Nguyen  wrote:
> >
> > On Tue, Jun 05, 2018 at 05:31:41PM +0200, Duy Nguyen wrote:
> > > I do not know how to reproduce this (and didn't bother to look deeply
> > > into it after I found it was not a trivial fix) but one of my "git
> > > fetch" showed
> > >
> > > warning: Submodule in commit be2db96a6c506464525f588da59cade0cedddb5e
> > > at path: '(null)' collides with a submodule named the same. Skipping
> > > it.
> >
> > The problem is default_name_or_path() can return NULL when a submodule
> > is not populated. The fix could simply be printing path instead of
> > name (because we are talking about path in the commit message), like
> > below.
> >
> > But I don't really understand c68f837576 (implement fetching of moved
> > submodules - 2017-10-16), the commit that made this change, and not
> > sure if we should be reporting name here or path. Heiko?
> >
> > diff --git a/submodule.c b/submodule.c
> > index 939d6870ec..61c2177755 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -745,7 +745,7 @@ static void collect_changed_submodules_cb(struct 
> > diff_queue_struct *q,
> 
> [Not in the context of this patch, but in the code right before the
> context starts:]
> 
> name = default_name_or_path(p->two->path);
> /* make sure name does not collide with existing one */
> submodule = submodule_from_name(the_repository, commit_oid, name);
> if (submodule) {
> 
> Currently I see 4 callers of default_name_or_path and the other 3 except this
> one have checks against NULL in place, which is good.
> However I think we have to guard this even more, and have to check
> for !name before we call submodule_from_name.
> 
> It is technically ok to call submodule_from_name with a NULL name,
> but it is semantically broken, see the comment in config_from that
> is called from submodule_from_name:
> 
> /*
>  * If any parameter except the cache is a NULL pointer just
>  * return the first submodule. Can be used to check whether
>  * there are any submodules parsed.
>  */
> if (!treeish_name || !key) {
> ...
> 
> 
> > warning("Submodule in commit %s at path: "
> > "'%s' collides with a submodule 
> > named "
> > "the same. Skipping it.",
> > -   oid_to_hex(commit_oid), name);
> > +   oid_to_hex(commit_oid), 
> > p->two->path);
> 
> This is correct for the error message, both in terms of not crashing as well
> as correctness, we really need to report a *path* here and no the 
> name_or_path,
> which  default_name_or_path gives.

Sorry for the late reply. I agree with Stefan, this change is correct,
since we are talking about the path in the warning message anyway. It
seems to me that this resulted from the name being the same as the path
in this location here.

I think we should report both here, if we can, the path and the name.

We are skipping the submodule if we can not get a name later:

if (!name) 
continue;

I also agree, that it does not make sense to call submodule_from_name
with a NULL name. I think we should simply skip calling it in case the
name is NULL and then let the code later handle it.

E.g.: 

...
/* make sure name does not collide with existing one */
if (name)   
submodule = submodule_from_name(the_repository, 
commit_oid, name);
if (submodule) {
warning("Submodule in commit %s at path: "
...

Would you want to update your patch? Or should I put one on top?

Cheers Heiko

Re: OAuth2 support in git?

[Jeff King]

On Thu, Jun 14, 2018 at 10:13:42AM +, brian m. carlson wrote:

> > I know that other git server environments like github support that on
> > client side by allowing tokens to be used as usernames in a BASIC
> > authentication flow. We could do the same but I am asking whether
> > there is also a way to transport tokens in a standard conform
> > "Authorization: Bearer ..." Header field.
> 
> There isn't any support for Bearer authentication in Git.  For HTTP, we
> use libcurl, which doesn't provide this natively.  While it could in
> theory be added, it would require some reworking of the auth code.
> 
> You are, of course, welcome to send a patch.

If it's just a custom Authorization header, we should be able to support
it with existing curl versions without _too_ much effort.

I think there are probably two possible directions:

 1. add a special "bearer" command line option, etc, as a string

 2. add a boolean option to send the existing "password" field as a
"bearer" header

I suspect (2) would fit in with the existing code better, as the special
case would mostly be limited to the manner in which we feed the
credential to curl. And you could probably just set a config option for
"this url's auth will be oauth2", and use the existing mechanisms for
providing the password.

We'd maybe also want to allow credential helpers to say "by the way,
this password should be treated as a bearer token", for cases where you
might sometimes use oauth2 and sometimes a real password.

-Peff

[Appointment Request] at LIFESTYLE EXPO TOKYO 2018 [July]

[Eiichi Hasegawa LIFESTYLE EXPO TOKYO Show Management]

Dear International Sales & Marketing Director, 
Zhejiang Wuchuan Industrial Co., Ltd

Hello, this is Eiichi Hasegawa from LIFESTYLE EXPO TOKYO 2018 Show Management.
This is to ask you for an appointment at LIFESTYLE EXPO TOKYO 2018 [July] (July 
4-6, 2018).

If you are planning on expanding your business, it would be the best 
opportunity for you to get ideas about the show to consider your future 
participation.

We are happy to give you detailed tips on how to yield results of the show, and 
propose the best location for your company.

If you are interested, please kindly reply with the form below.

(Or please kindly forward this message to the persons in charge.)


---
Appointment/Information Request Form
---
( ) I would like to have a brief meeting at LIFESTYLE EXPO TOKYO 2018 [July].
- Date: [ ] July 4(Wed.)
[ ] July 5(Thur.)
[ ] July 6(Fri.)

- Time: __:__ AM/ __:__PM


( ) We cannot meet you, send us exhibiting information.

Name:
Company:
Title/Div.:
Country:
TEL#:
Company Website:



We are looking forward to hearing from you soon.

Sincerely,

Eiichi Hasegawa (Mr.), Chisato Miyawaki (Ms.), Mikako Shimada (Ms.)
Qu Jun (Mr.), Choi Ilyong (Mr.)
LIFESTYLE EXPO TOKYO Show Management
Reed Exhibitions Japan Ltd.
TEL: +81-3-3349-8505
Mail:lifestyle-...@reedexpo.co.jp


--
LIFESTYLE EXPO TOKYO 2018 [July] 
Date: July 4 (Wed) - 6 (Fri), 2018
Venue: Tokyo Big Sight, Japan
Open hour: 10:00-18:00, (July 6 open until 17:00)
WEB: http://www.lifestyle-expo.jp/en/summer/
Scale: 2,440 exhibitors from 41 countries, 90,000 high quality buyers 
(concurrent shows included)
--

ID:E36-G1402-0075





This message is delivered to you to provide details of exhibitions and 
conferences organized, co-organized, or managed by Reed Exhibitions Japan Ltd.
If you would like to change your contact information, or prefer not to receive 
further information on this exhibition/conference, please follow the directions 
below.


Please click the URL below and follow the directions on the website to update 
your e-mail and other information.
https://contact.reedexpo.co.jp/expo/REED/?lg=en=ch=CHANGE


Please reply to this mail changing the subject to "REMOVE FROM LIST".
You will not receive any further information on this exhibition/conference.

[PATCH] t3200: clarify description of --set-upstream test

[Kaartic Sivaraam]

Support for the --set-upstream option was removed in 52668846ea
(builtin/branch: stop supporting the "--set-upstream" option,
2017-08-17). The change did not completely remove the command
due to an issue noted in the commit's log message.

So, a test was added to ensure that a command which uses the
'--set-upstream' option fails and doesn't silently act as an alias
for the '--set-upstream-to' option due to option parsing features.

To avoid confusion, clarify that the option is an unsupported one
in the corresponding test description.

Signed-off-by: Kaartic Sivaraam 
---
 t/t3200-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 6c0b7ea4a..d14de82ba 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -884,7 +884,7 @@ test_expect_success 'test --unset-upstream on a particular 
branch' '
test_must_fail git config branch.my14.merge
 '
 
-test_expect_success '--set-upstream fails' '
+test_expect_success 'unsupported option --set-upstream fails' '
 test_must_fail git branch --set-upstream origin/master
 '
 
-- 
2.18.0.rc1.242.g61856ae69

(Bug report + fix) gitk "IgnCase" search doesn't ignore case

[Juan Navarro]

Hi,

this question was originally posted on the Google Groups list, trying to 
get help 
(https://groups.google.com/d/topic/git-users/QAFKOQU4eUo/discussion).
Now that it's confirmed as a bug and I have a proposed solution, I'm 
posting here.


Gitk "find commit" search function doesn't follow the "IgnCase" option 
that is selectable with a combo selector on the right side of the 
window; it should be searching in a case-insensitive way, but it doesn't.


Steps to reproduce:
1. Clone any repo. I'm right now using 
https://github.com/Kurento/kms-core.git 


2. cd into the repo and launch gitk
3. In the "Find commit" bar, select "changing lines matching"
4. In the right side of the same bar, select "IgnCase"
5. Search for a term that you know exists in uppercase, but write the 
search in lowercase. In my example, I'm searching for "leaky_time".

6. No search results appear.
7. Change your search word to uppercase.
8. Some search results appear, thus proving that the search is being 
done case-sensitive.


The command that gets called when searching is in lines 4804 & 4809 
(https://github.com/git/git/blob/master/gitk-git/gitk#L4804 
):

git diff-tree -r -s --stdin -G

Proposed solution is almost trivial: check if the "IgnCase" option is 
enabled, and in that case add the flag "-i" to the git command. Now that 
we are at it, it's probably correct to add that option to all search modes.
A diff is attached to this email, hoping that someone is able to apply 
it (sorry I'm not familiarized with contributing with patch files, but 
the diff is pretty small anyways).


Regards,
Juan
4809c4809,4815
< set cmd [concat | git diff-tree -r -s --stdin $gdtargs]
---
> 
> set igncase_arg ""
> if {$findtype eq [mc "IgnCase"]} {
> set igncase_arg "-i"
> }
> 
> set cmd [concat | git diff-tree $igncase_arg -r -s --stdin $gdtargs]

[PATCH 3/4] t/lib-httpd: add minor and patch version to $HTTPD_VERSION

[SZEDER Gábor]

To fix an occasional test failure, the next patch will rely on an
Apache log format specifier that was introduced in version 2.2.30.
Consequently, 't/lib-httpd.sh' must be able to decide whether the
Apache version used for testing is equal or newer than that version to
be able to act accordingly when processing the webserver's access log.

$HTTPD_VERSION currently only contains Apache's major version, so
change how Apache's version string is parsed to store the minor and
patch versions as well.  Furthermore, update the only condition
looking at $HTTPD_VERSION to deal with the full version.

Signed-off-by: SZEDER Gábor 
---
 t/lib-httpd.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index b6788fea57..5915625631 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -99,13 +99,13 @@ then
 fi
 
 HTTPD_VERSION=$($LIB_HTTPD_PATH -v | \
-   sed -n 's/^Server version: Apache\/\([0-9]*\)\..*$/\1/p; q')
+   sed -n 's/^Server version: Apache\/\([0-9]*\.[0-9]*\.[0-9]*\).*$/\1/p; 
q')
 
 if test -n "$HTTPD_VERSION"
 then
if test -z "$LIB_HTTPD_MODULE_PATH"
then
-   if ! test $HTTPD_VERSION -ge 2
+   if ! test ${HTTPD_VERSION%%.*} -ge 2
then
test_skip_or_die $GIT_TEST_HTTPD \
"at least Apache version 2 is required"
-- 
2.18.0.rc0.207.ga6211da864

[RFC PATCH 4/4] t/lib-httpd: sort log based on timestamp to avoid occasional failure

[SZEDER Gábor]

The last test of 't5561-http-backend.sh', 'server request log matches
test results' may fail occasionally, because the order of entries in
Apache's access log doesn't match the order of requests sent in the
previous tests, although all the right requests are there.  I saw it
fail on Travis CI five times in the span of about half a year, when
the order of two subsequent requests was flipped, and could trigger
the failure with a modified Git.  However, I was unable to trigger it
with stock Git on my machine.  Three tests in
't5541-http-push-smart.sh' and 't5551-http-fetch-smart.sh' check
requests in the log the same way, so they might be prone to a similar
occasional failure as well.

When a test sends a HTTP request, it can continue execution after
'git-http-backend' fulfilled that request, but Apache writes the
corresponding access log entry only after 'git-http-backend' exited.
Some time inevitably passes between fulfilling the request and writing
the log entry, and, under unfavourable circumstances, enough time
might pass for the subsequent request to be sent and fulfilled, and
then Apache writes access log entries racily.

This effect can be exacerbated by adding a bit of variable delay after
the request is fulfilled but before 'git-http-backend' exits, e.g.
like this:

  diff --git a/http-backend.c b/http-backend.c
  index f3dc218b2..bbf4c125b 100644
  --- a/http-backend.c
  +++ b/http-backend.c
  @@ -709,5 +709,7 @@ int cmd_main(int argc, const char **argv)
   max_request_buffer);

cmd->imp(, cmd_arg);
  + if (getpid() % 2)
  + sleep(1);
return 0;
   }

This delay considerably increases the chances of log entries being
written out of order, and in turn makes t5561's last test fail almost
every time.  Alas, it doesn't seem to be enough to trigger a similar
failure in t5541 and t5551.

Now, by default the timestamp of a log entry marks the beginning of
the request processing, not when the log entry gets written.  Since
our requests are sent sequentially, sorting the log entries based on
their timestamps would ensure that their order corresponds to the
order of our requests.

Therefore, change Apache's log format to use microseconds since epoch
as timestamps, the finest resolution Apache has to offer.  This will:

  - give enough resolution to make reasonably sure that no two
requests have the same timestamp; the default timestamp format has
only one second resolution, which is not nearly enough.

  - make the timestamps sortable; the default timestamp format looks
like "[31/Jan/2018:16:33:52 +]", which would trip sorting if
the test script happens to run over month boundaries.

Modify the strip_access_log() helper function, used by all potentially
affected tests, to sort the access log entries based on their
timestamp before stripping the uninteresting bits from the entries.

Unfortunately, this "microseconds since epoch" is an extended time
format specifier which was only introduced in Apache 2.2.30 [1], and
standard strftime() doesn't have any format specifiers with
microsecond or finer precision, so we can't use the above solution
with Apache versions older than 2.2.30.  However, since Apache 2.2 is
no longer maintained anyway, it's not that big a deal to leave test
runs with those old versions prone to this occasional failure.  So use
microsecond timestamps and sort access log entries only if the test is
run with an Apache version supporting it; otherwise stick to the
default timestamp format and leave the order of access log entries
unchanged.

[1] http://httpd.apache.org/docs/2.2/mod/mod_log_config.html

Signed-off-by: SZEDER Gábor 
---

Notes:
The log of the latest of this test failures:

  
https://travis-ci.org/szeder/git-cooking-topics-for-travis-ci/jobs/391499735#L3130

I don't really like the fix in this patch.  I think an unfortunate
clock skew during the test run could mess up the sorting added in this
patch and cause test failure.  Or if DST or even a leap second happen
while the test is running.  Do we care?  Anyway, this occasional test
failure apparently happens more often than DST and leap seconds
combined.
Furthermore, in the future we might have computers fast enough to
fulfill more than one test requests in a single microsecond; but I
guess by that time we'll have higher resolution time formats as well.

An alternative I considered was that we could decide that the order of
requests in the access log is not important as long as all the right
requests are there.  This would inherently eliminate the raciness
issue, but if something were to go wrong, then it would become rather
hard to find out e.g. which request from which test has gone missing,
especially considering that several requests are sent in more than one
test.  We could address this by truncating the access log at the
beginning and checking its 

[PATCH 2/4] t/lib-httpd: add the strip_access_log() helper function

[SZEDER Gábor]

Four tests in three httpd-related test scripts check the contents of
Apache's 'access.log', and they all do so by running 'sed' with the
exact same script consisting of four s/// commands to strip
uninteresting log fields and to vertically align the requested URLs.

Extract this into a common helper function 'strip_access_log' in
'lib-httpd.sh', and use it in all of those tests.

Signed-off-by: SZEDER Gábor 
---
 t/lib-httpd.sh  |  9 +
 t/t5541-http-push-smart.sh  | 14 ++
 t/t5551-http-fetch-smart.sh |  7 +--
 t/t5561-http-backend.sh |  7 +--
 4 files changed, 13 insertions(+), 24 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465a..b6788fea57 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -287,3 +287,12 @@ expect_askpass() {
test_cmp "$TRASH_DIRECTORY/askpass-expect" \
 "$TRASH_DIRECTORY/askpass-query"
 }
+
+strip_access_log() {
+   sed -e "
+   s/^.* \"//
+   s/\"//
+   s/ [1-9][0-9]*\$//
+   s/^GET /GET  /
+   " "$HTTPD_ROOT_PATH"/access.log
+}
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index c961db814d..a1385800fa 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -43,12 +43,7 @@ test_expect_success 'no empty path components' '
cd "$ROOT_PATH" &&
git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
 
-   sed -e "
-   s/^.* \"//
-   s/\"//
-   s/ [1-9][0-9]*\$//
-   s/^GET /GET  /
-   " >act <"$HTTPD_ROOT_PATH"/access.log &&
+   strip_access_log >act &&
 
# Clear the log, so that it does not affect the "used receive-pack
# service" test which reads the log too.
@@ -137,12 +132,7 @@ GET  
/smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
 POST /smart/test_repo.git/git-receive-pack HTTP/1.1 200
 EOF
 test_expect_success 'used receive-pack service' '
-   sed -e "
-   s/^.* \"//
-   s/\"//
-   s/ [1-9][0-9]*\$//
-   s/^GET /GET  /
-   " >act <"$HTTPD_ROOT_PATH"/access.log &&
+   strip_access_log >act &&
test_cmp exp act
 '
 
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index f5721b4a59..82a4fb5e16 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -98,12 +98,7 @@ GET  /smart/repo.git/info/refs?service=git-upload-pack 
HTTP/1.1 200
 POST /smart/repo.git/git-upload-pack HTTP/1.1 200
 EOF
 test_expect_success 'used upload-pack service' '
-   sed -e "
-   s/^.* \"//
-   s/\"//
-   s/ [1-9][0-9]*\$//
-   s/^GET /GET  /
-   " >act <"$HTTPD_ROOT_PATH"/access.log &&
+   strip_access_log >act &&
test_cmp exp act
 '
 
diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index 90e0d6f0fe..6a0f240e6d 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -123,12 +123,7 @@ GET  /smart/repo.git/info/refs?service=git-receive-pack 
HTTP/1.1 403 -
 POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
 EOF
 test_expect_success 'server request log matches test results' '
-   sed -e "
-   s/^.* \"//
-   s/\"//
-   s/ [1-9][0-9]*\$//
-   s/^GET /GET  /
-   " >act <"$HTTPD_ROOT_PATH"/access.log &&
+   strip_access_log >act &&
test_cmp exp act
 '
 
-- 
2.18.0.rc0.207.ga6211da864

[PATCH 1/4] t5541: avoid empty line when truncating access log

[SZEDER Gábor]

The second test of 't5541-http-push-smart.sh', 'no empty path
components' truncates Apache's access log by running

  echo >.../access.log

which doesn't leave an empty file behind, like a proper truncation
would, but a file with a lone newline in it.  Consequently, a later
test checking the log's contents must consider this improper
truncation and include an empty line in the expected content.

There is no need for that newline at all, so drop the 'echo' from the
truncation and adjust the expected content accordingly.

Signed-off-by: SZEDER Gábor 
---

 t/t5541-http-push-smart.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 21340e89c9..c961db814d 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -54,7 +54,7 @@ test_expect_success 'no empty path components' '
# service" test which reads the log too.
#
# We do this before the actual comparison to ensure the log is cleared.
-   echo > "$HTTPD_ROOT_PATH"/access.log &&
+   >"$HTTPD_ROOT_PATH"/access.log &&
 
test_cmp exp act
 '
@@ -124,7 +124,6 @@ test_expect_success 'rejected update prints status' '
 rm -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update"
 
 cat >exp <

[RFC PATCH 0/4] Fix occasional test failures in http tests

[SZEDER Gábor]

't5561-http-backend.sh' is prone to occasional failures; luckily it's
not 'git-http-backend's fault, but the test script is a bit racy.  I
won't go into the details here, patch 4/4's commit message discusses
it at length.  4/4 also fixes this issue, but I'm not particularly
happy with that fix, the note attached to that patch explains why,
along with possible alternatives, hence the RFC.

Even if we settle for a different fix, I think the first two patches
are worthy cleanups on their own right.

SZEDER Gábor (4):
  t5541: avoid empty line when truncating access log
  t/lib-httpd: add the strip_access_log() helper function
  t/lib-httpd: add minor and patch version to $HTTPD_VERSION
  t/lib-httpd: sort log based on timestamp to avoid occasional failure

 t/lib-httpd.sh  | 24 ++--
 t/lib-httpd/apache.conf |  5 +
 t/t5541-http-push-smart.sh  | 17 +++--
 t/t5551-http-fetch-smart.sh |  7 +--
 t/t5561-http-backend.sh |  7 +--
 5 files changed, 32 insertions(+), 28 deletions(-)

-- 
2.18.0.rc0.207.ga6211da864

Re: OAuth2 support in git?

[brian m. carlson]

On Thu, Jun 14, 2018 at 10:09:39AM +0200, Christian Halstrick wrote:
> Can I use native git as client to contact a git server which does
> authentication with OAuth2 Client Credentials Grant [1]?
> 
> Background: We are running gerrit based git servers [2] in a cloud
> environment. That environment supports OAuth2 authorization for the
> apps running in the cloud. The idea is that clients (e.g. jenkins
> jobs) talking git over http with such git servers should be able to
> use OAuth2 tokens to authenticate clone/fetch requests. We would have
> to adapt gerrit source code for token handling/validation but I am
> asking here about the client side.
> 
> I know that other git server environments like github support that on
> client side by allowing tokens to be used as usernames in a BASIC
> authentication flow. We could do the same but I am asking whether
> there is also a way to transport tokens in a standard conform
> "Authorization: Bearer ..." Header field.

There isn't any support for Bearer authentication in Git.  For HTTP, we
use libcurl, which doesn't provide this natively.  While it could in
theory be added, it would require some reworking of the auth code.

You are, of course, welcome to send a patch.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [GSoC][PATCH v2 0/2] rebase -i: rewrite the edit-todo functionality in C

[Phillip Wood]

Hi Alban

On 13/06/18 16:22, Alban Gruin wrote:
> This patch rewrites the edit-todo functionality from shell to C. This is
> part of the effort to rewrite interactive rebase in C.
> 
> Changes since v1:
> 
>  - Add a new function to launch the sequence editor, as advised by
>Phillip Wood[0]

That's great, I think these look fine now

Best Wishes

Phillip

> [0] 
> https://public-inbox.org/git/3bfd3470-4482-fe6a-2cd9-08311a0bb...@talktalk.net/
> 
> Alban Gruin (2):
>   editor: add a function to launch the sequence editor
>   rebase--interactive: rewrite the edit-todo functionality in C
> 
>  builtin/rebase--helper.c   | 13 -
>  cache.h|  1 +
>  editor.c   | 27 +--
>  git-rebase--interactive.sh | 11 +--
>  sequencer.c| 31 +++
>  sequencer.h|  1 +
>  strbuf.h   |  2 ++
>  7 files changed, 69 insertions(+), 17 deletions(-)
> 

OAuth2 support in git?

[Christian Halstrick]

Can I use native git as client to contact a git server which does
authentication with OAuth2 Client Credentials Grant [1]?

Background: We are running gerrit based git servers [2] in a cloud
environment. That environment supports OAuth2 authorization for the
apps running in the cloud. The idea is that clients (e.g. jenkins
jobs) talking git over http with such git servers should be able to
use OAuth2 tokens to authenticate clone/fetch requests. We would have
to adapt gerrit source code for token handling/validation but I am
asking here about the client side.

I know that other git server environments like github support that on
client side by allowing tokens to be used as usernames in a BASIC
authentication flow. We could do the same but I am asking whether
there is also a way to transport tokens in a standard conform
"Authorization: Bearer ..." Header field.

[1] https://tools.ietf.org/html/rfc6749#section-4.4
[2] https://www.gerritcodereview.com/

Re: [PATCH 19/20] abbrev: support relative abbrev values

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 13 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  writes:
>
>> E.g. here's a breakdown of my dotfiles repo:
>>
>> $ git -c core.abbrev=4 log  --pretty=format:%h|perl -nE 'chomp;say 
>> length'|sort|uniq -c|sort -nr
>> 784 4
>>  59 5
>>   7 6
>>
>> I don't have a single commit that needs 7 characters, yet that's our
>> default. This is a sane trade-off for the kernel, but for something
>> that's just a toy or something you're playing around with something
>> shorter can make sense.
>>
>> SHA-1s aren't just written down, but also e.g. remembered in wetware
>> short-term memory.
>
> That's a fine example of what I called "supporting absurdly small
> absolute values like 4"; I still do not see why you want "negative
> relative values" from that example.

Because hardcoding -2 is very different than setting it to 5, because
the -2 will scale to the size of the repository, but 5 is just 7-2 where
7 is our default value.

So, in general if you want less future proof hashes by some
probabilistic metric (whether you use core.validateAbbrev or not) you'd
use -2 or -3, just like you might use +2 or +3 if you'd like to have
more future-proof hashes (especially with core.validateAbbrev=true).