Re: [PATCH v2 08/11] t1404: demonstrate two problems with reference transactions

[Michael Haggerty]

On 09/09/2017 01:17 PM, Jeff King wrote:
> On Fri, Sep 08, 2017 at 03:51:50PM +0200, Michael Haggerty wrote:
> [...]
> So if we get what we want, we execute ":" which should be a successful
> exit code.

I think the `:` is superfluous even if we care about the exit code of
the `case`. I'll remove it.

>> +undefined)
>> +# This is not correct; it means the deletion has happened
>> +# already even though update-ref should not have been
>> +# able to acquire the lock yet.
>> +echo "$prefix/foo deleted prematurely" &&
>> +break
>> +;;
> 
> But if we don't, we hit a "break". But we're not in a loop, so the break
> does nothing. Is the intent to give a false value to the switch so that
> we fail the &&-chain? If so, I'd think "false" would be the right thing
> to use. It's more to the point, and from a few limited tests, it looks
> like "break" will return "0" even outside a loop (bash writes a
> complaint to stderr, but dash doesn't).
> 
> Or did you just forget that you're not writing C and that ";;" is the
> correct way to spell "break" here? :)

An earlier version of the patch used a loop and needed the `break`. But
when I removed the loop, I probably didn't notice the now-unneeded
breaks because of what you said. I'll take them out.

>> [...]
>> +esac >out &&
>> [...]
>> +test_must_be_empty out &&
> 
> The return value of "break" _doesn't_ matter, because you end up using
> the presence of the error message.
> 
> I think we could write this as just:
> 
>   case "$sha1" in
>   $D)
>   # good
>   ;;
>   undefined)
> echo >&2 this is bad
>   false
>   ;;
>   esac &&
> 
> I'm OK with it either way (testing the exit code or testing the output),
> but either way the "break" calls are doing nothing and can be dropped, I
> think.

Yes, using the exit code to decide success is simpler. I'll make that
change, too.

Thanks for your comments.

Michael

Re: [PATCH 00/12] Clean up notes-related code around `load_subtree()`

[Michael Haggerty]

On 09/09/2017 12:31 PM, Jeff King wrote:
> On Sat, Aug 26, 2017 at 10:28:00AM +0200, Michael Haggerty wrote:
> 
>> It turns out that the comment is incorrect, but there was nevertheless
>> plenty that could be cleaned up in the area:
>>
>> * Make macro `GIT_NIBBLE` safer by adding some parentheses
>> * Remove some dead code
>> * Fix some memory leaks
>> * Fix some obsolete and incorrect comments
>> * Reject "notes" that are not blobs
>>
>> I hope the result is also easier to understand.
>>
>> This branch is also available from my Git fork [1] as branch
>> `load-subtree-cleanup`.
> 
> FYI, Coverity seems to complain about "pu" after this series is merged, but
> I think it's wrong.  It says:
> 
>   *** CID 1417630:  Memory - illegal accesses  (OVERRUN)
>   /notes.c: 458 in load_subtree()
>   452 
>   453 /*
>   454  * Pad the rest of the SHA-1 with zeros,
>   455  * except for the last byte, where we write
>   456  * the length:
>   457  */
>   >>> CID 1417630:  Memory - illegal accesses  (OVERRUN)
>   >>> Overrunning array of 20 bytes at byte offset 20 by dereferencing 
> pointer "_oid.hash[len]".
>   458 memset(object_oid.hash + len, 0, GIT_SHA1_RAWSZ 
> - len - 1);
>   459 object_oid.hash[KEY_INDEX] = (unsigned char)len;
>   460 
>   461 type = PTR_TYPE_SUBTREE;
>   462 } else {
>   463 /* This can't be part of a note */
> 
> I agree that if "len" were 20 here that would be a problem, but I don't
> think that's possible.
> 
> The tool correctly claims that prefix_len can be up to 19, due to the
> assert:
> 
>  3. cond_at_most: Checking prefix_len >= 20UL implies that prefix_len may 
> be up to 19 on the false branch.
>   420if (prefix_len >= GIT_SHA1_RAWSZ)
>   421BUG("prefix_len (%"PRIuMAX") is out of range", 
> (uintmax_t)prefix_len);
> 
> Then it claims:
> 
> 13. Condition path_len == 2 * (20 - prefix_len), taking false branch.
>   430if (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) {
>   431/* This is potentially the remainder of the 
> SHA-1 */
> 
> So we know that either prefix_len is not 19, or that path_len is not 2
> (since that combination would cause us to take the true branch here).
> But then it goes on to say:
> 
> 14. Condition path_len == 2, taking true branch.
>   442} else if (path_len == 2) {
>   443/* This is potentially an internal node */
> 
> which I believe must mean that prefix_len cannot be 19 here. And yet it
> says:
> 
> 15. assignment: Assigning: len = prefix_len. The value of len may now be 
> up to 19.
>   444size_t len = prefix_len;
>   445
>   [...]
>  17. incr: Incrementing len. The value of len may now be up to 20.
>  18. Condition hex_to_bytes(_oid.hash[len++], entry.path, 1), 
> taking false branch.
>   450if (hex_to_bytes(object_oid.hash + len++, 
> entry.path, 1))
>   451goto handle_non_note; /* entry.path is 
> not a SHA1 */
> 
> I think that's impossible, and Coverity simply isn't smart enough to
> shrink the set of possible values for prefix_len based on the set of
> if-else conditions.
> 
> So nothing to see here, but since I spent 20 minutes scratching my head
> (and I know others look at Coverity output and may scratch their heads
> too), I thought it was worth writing up. And also if I'm wrong, it would
> be good to know. ;)

Thanks for looking into this. I agree with your analysis.

I wonder whether it is the factor of two between path lengths and byte
lengths that is confusing Coverity. Perhaps the patch below would help.
It requires an extra, superfluous, check, but perhaps makes the code a
tad more readable. I'm neutral on whether we would want to make the change.

Is there a way to ask Coverity whether a hypothetical change would
remove the warning, short of merging the change to master?

Michael

diff --git a/notes.c b/notes.c
index 27d232f294..34f623f7b1 100644
--- a/notes.c
+++ b/notes.c
@@ -426,8 +426,14 @@ static void load_subtree(struct notes_tree *t,
struct leaf_node *subtree,
unsigned char type;
struct leaf_node *l;
size_t path_len = strlen(entry.path);
+   size_t path_bytes;

-   if (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) {
+   if (path_len % 2 != 0)
+   /* Path chunks must come in pairs of hex characters */
+   goto handle_non_note;
+
+   path_bytes = path_len / 2;
+   if (path_bytes == GIT_SHA1_RAWSZ - prefix_len) {
/* This is potentially the remainder of the SHA-1 */

if (!S_ISREG(entry.mode))

Hello

[DAVID JIM]

Hello ,

This is to inform you that this bank has appointed a New managing
Director and on this note informing you about your withheld fund
$950,000 Dollars been held by pass administration.

As the New appointed manager, collaboration with Benin republic
government and governor central bank directives. We have decided to
process your fund $950,000.( NINE HUNDRED AND FIFTY THOUSAND DOLLARS )
THROUGH  ATM CARD and send it to your Home address via delivery
courier service.

For safety delivery of your $950,000 Dollars  ATM card. We required
details such as.

Full Names..
Home address.
Mobile..
Occupation.
Office address

Immediately we received your details, your $950,000 ATM CARD shipment
will be provided Home address.

Thanks.

Mr.David Jim

+229 99192164

Unexpected pass for t6120-describe.sh on cygwin

[Ramsay Jones]

Hi Adam,

I ran the test-suite on the 'pu' branch last night (simply because
that was what I had built at the time!), which resulted in a PASS,
but t6120 was showing a 'TODO passed' for #52.

This is a test introduced by Michael's 'mg/name-rev-tests-with-short-stack'
branch, which uses 'ulimit -s' to try and force a stack overflow.
Unfortunately, 'ulimit -s' seems to have no effect on cygwin. I created
a test program (see below) to eat up the stack and tried running it
with various ulimit values (128, 12, 8), but it always seg-faulted
at the same stack-frame. (after using approx 2MB stack space).

So, it looks like all ULIMIT_STACK_SIZE tests need to be disabled
on cygwin. I also wonder about the ULIMIT_FILE_DESCRIPTORS tests,
but haven't looked into it.

Given that 'ulimit' is a bash built-in, this may also be a problem
on MinGW and Git-For-Windows, but I can't test on those platforms.

Unfortunately, I can't spend more time on git today, hence this
heads up! ;-)

ATB,
Ramsay Jones

-- >8 --
diff --git a/test.c b/test.c
new file mode 100644
index 000..bcbb805
--- /dev/null
+++ b/test.c
@@ -0,0 +1,21 @@
+#include 
+#include 
+#include 
+
+void test(uint64_t count)
+{
+   int i, junk[1024];
+
+   for (i = 0; i < 1024; i++)
+   junk[i] = count;
+   i = junk[count % 1024];
+   printf("%" PRIuMAX "\n", (uintmax_t)count);
+   fflush(stdout);
+   test(count + 1);
+}
+
+int main(int argc, char *argv[])
+{
+   test(0);
+   return 0;
+}

Re: Donation

[Mavis Wanczyk Foundation]

Greetings To You,

My Name is Mavis wanczyk , the winner of the Power ball jackpot of $ $758.7 
million  in the AUGUST 24, 2017, My jackpot was a gift from God to me hence my 
Entire family/foundation has AGREED to do this. My foundation is donating 
$500,000.00USD to you. please contac maviswanczyk...@gmail.com for full details 
and please accept this token as a gift from me and my family.

Read more: 
http://money.cnn.com/2017/08/23/news/powerball-700-million-jackpot/index.html

 Best Regards,
 Mavis Wanczyk

Re: git diff doesn't quite work as documented?

[Yubin Ruan]

2017-09-08 0:31 GMT+08:00 Olaf Klischat :
> oklischat@oklischat:/tmp$ mkdir gittest
> oklischat@oklischat:/tmp$ cd gittest/
> oklischat@oklischat:/tmp/gittest$ git init
> Initialized empty Git repository in /private/tmp/gittest/.git/
> oklischat@oklischat:/tmp/gittest$ echo foo > foo.txt
> oklischat@oklischat:/tmp/gittest$ git add foo.txt
> oklischat@oklischat:/tmp/gittest$ git commit -m foo
> [master (root-commit) 54d55f2] foo
>  1 file changed, 1 insertion(+)
>  create mode 100644 foo.txt
> oklischat@oklischat:/tmp/gittest$ echo bar > bar.txt
> oklischat@oklischat:/tmp/gittest$ git add bar.txt
> oklischat@oklischat:/tmp/gittest$ git commit -m bar
> [master 83b2e74] bar
>  1 file changed, 1 insertion(+)
>  create mode 100644 bar.txt
> oklischat@oklischat:/tmp/gittest$ git tag bar-added
> oklischat@oklischat:/tmp/gittest$ git rm bar.txt
> rm 'bar.txt'
> oklischat@oklischat:/tmp/gittest$ git commit -m 'rm bar'
> [master 3ca4ff9] rm bar
>  1 file changed, 1 deletion(-)
>  delete mode 100644 bar.txt
> oklischat@oklischat:/tmp/gittest$
> oklischat@oklischat:/tmp/gittest$ git checkout bar-added -- bar.txt
> oklischat@oklischat:/tmp/gittest$ git reset HEAD
> oklischat@oklischat:/tmp/gittest$ git status
> On branch master
> Untracked files:
>   (use "git add ..." to include in what will be committed)
>
> bar.txt
>
> nothing added to commit but untracked files present (use "git add" to track)
> oklischat@oklischat:/tmp/gittest$ git diff bar-added   # would expect this to 
> show no differences
> diff --git a/bar.txt b/bar.txt
> deleted file mode 100644
> index 5716ca5..000
> --- a/bar.txt
> +++ /dev/null
> @@ -1 +0,0 @@
> -bar
> oklischat@oklischat:/tmp/gittest$
> oklischat@oklischat:/tmp/gittest$ git diff bar-added  -- bar.txt   # dito
> diff --git a/bar.txt b/bar.txt
> deleted file mode 100644
> index 5716ca5..000
> --- a/bar.txt
> +++ /dev/null
> @@ -1 +0,0 @@
> -bar

Michael J Gruber is correct about the working-dir/working-tree things.
A quick example: add a new file with

$ echo bzz > bzz.txt

then do "git diff bar-added".

The result is the same, because "bzz.txt" is not in the working-tree.

Yubin

> oklischat@oklischat:/tmp/gittest$
>
>
> `git diff --help' says:
>
> git diff [--options]  [--] [...]
>This form is to view the changes you have in your working tree 
> relative to the named .
>
> If that were entirely true, the last two commands shouldn't have shown
> any differences, right?
>
> On closer inspection, it seems that what `git diff ' really
> does is take only those paths in the working directory that are also
> in  and compare the resulting tree against .
>
> We should add some option to that git diff form to make it really do
> what the docs claim it does.
>
> Or am I missing something?

Re: [PATCH v2 00/11] Implement transactions for the packed ref store

[Jeff King]

On Fri, Sep 08, 2017 at 03:51:42PM +0200, Michael Haggerty wrote:

> This is v2 of a patch series to implement reference transactions for
> the packed refs-store. Thanks to Stefan, Brandon, Junio, and Peff for
> your review of v1 [1]. I believe I have addressed all of your
> comments.
> 
> Changes since v1:
> 
> * Patch [01/11]: justify the change better in the log message. Add a
>   comment explaining why `get_packed_ref_cache()` is being called but
>   the return value discarded.
> 
> * Patch [05/11]: Lock the `packed-refs` file *after* successfully
>   creating the (empty) transaction object. This prevents leaving the
>   file locked if `ref_store_transaction_begin()` fails.
> 
> * Patch [06/11]: New patch, fixing a leak of the `refs_to_prune`
>   linked list.

These all look good to me, including the new patch.

> * Patch [07/11]: Reimplement test "no bogus intermediate values during
>   delete" to work without polling. Also incorporate Junio's change
>   `s/grep/test_i18ngrep/`.

I picked a few nits in the test script. Nothing too serious, but a few
things that might be worth addressing.

-Peff

Re: [PATCH v2 08/11] t1404: demonstrate two problems with reference transactions

[Jeff King]

On Fri, Sep 08, 2017 at 03:51:50PM +0200, Michael Haggerty wrote:

> +test_expect_failure 'no bogus intermediate values during delete' '
> + prefix=refs/slow-transaction &&
> + # Set up a reference with differing loose and packed versions:
> + git update-ref $prefix/foo $C &&
> + git pack-refs --all &&
> + git update-ref $prefix/foo $D &&
> + git for-each-ref $prefix >unchanged &&
> + # Now try to update the reference, but hold the `packed-refs` lock
> + # for a while to see what happens while the process is blocked:
> + : >.git/packed-refs.lock &&
> + test_when_finished "rm -f .git/packed-refs.lock" &&
> + {
> + # Note: the following command is intentionally run in the
> + # background. We increase the timeout so that `update-ref`
> + # attempts to acquire the `packed-refs` lock for longer than
> + # it takes for us to do the check then delete it:
> + git -c core.packedrefstimeout=3000 update-ref -d $prefix/foo &
> + } &&
> + pid2=$! &&

There's some timing trickiness in this test, so I want to take a close
look at possible races.

The point of this timeout is just to make sure that we end up blocking
"long enough" that the test code can ensure that we're blocked for a
certain period, during which we can look at the on-disk state.

So this timeout really could be as long as we want, and ideally longer
is better (since we would not want it to exit while we're examining the
state). Later we do a `wait` on this process, but only after removing
the lockfile, which should cause it to exit. So in theory we could make
this something silly like an hour that could not possibly race.

The only downside, I guess, is that if something goes horribly wrong, it
could take an hour to exit (but we put the "rm" into a
test_when_finished, so I think that would cover the test failing early).

> + # Give update-ref plenty of time to get to the point where it tries
> + # to lock packed-refs:
> + sleep 1 &&

Yuck. So this is definitely a potential race. On a busy system it could
take more than a second to try the lock.

But:

  1. Since we're looking for the on-disk state _not_ to change, when we
 lose the race the test still succeeds (it just tests nothing
 useful). So we shouldn't get false positives.

  2. I don't think we can do better. In the corrected state, the
 sub-process makes no externally visible change that we could wait
 on (unless we turned to unportable tools like strace).

So I think it's OK. I'm never excited about using sleep in our tests,
but I don't see a better option.

> + # Make sure that update-ref did not complete despite the lock:
> + kill -0 $pid2 &&

I'm not sure if "kill -0" is portable to Windows or not. I have no
specific knowledge that it _isn't_, but signals have been a problem area
for us in the past. I see we use it for some of the p4 tests, but I
wouldn't be surprised if those are already skipped on Windows.

I guess if it produces false positives then Windows folks can report and
mark it to be skipped. If it produces false negatives there, then nobody
will be the wiser, but there's not much we can do.

> + # Verify that the reference still has its old value:
> + sha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) &&
> + case "$sha1" in
> + $D)
> + # This is what we hope for; it means that nothing
> + # user-visible has changed yet.
> + : ;;

So if we get what we want, we execute ":" which should be a successful
exit code.

> + undefined)
> + # This is not correct; it means the deletion has happened
> + # already even though update-ref should not have been
> + # able to acquire the lock yet.
> + echo "$prefix/foo deleted prematurely" &&
> + break
> + ;;

But if we don't, we hit a "break". But we're not in a loop, so the break
does nothing. Is the intent to give a false value to the switch so that
we fail the &&-chain? If so, I'd think "false" would be the right thing
to use. It's more to the point, and from a few limited tests, it looks
like "break" will return "0" even outside a loop (bash writes a
complaint to stderr, but dash doesn't).

Or did you just forget that you're not writing C and that ";;" is the
correct way to spell "break" here? :)

> [...]
> + esac >out &&
> [...]
> + test_must_be_empty out &&

The return value of "break" _doesn't_ matter, because you end up using
the presence of the error message.

I think we could write this as just:

  case "$sha1" in
  $D)
# good
;;
  undefined)
echo >&2 this is bad
false
;;
  esac &&

I'm OK with it either way (testing the exit code or testing the output),
but either way the "break" calls are doing nothing and can be dropped, I
think.

-Peff

Re: [PATCH v4 0/4] Rerolling ma/split-symref-update-fix

[Jeff King]

On Sat, Sep 09, 2017 at 08:57:14AM +0200, Martin Ågren wrote:

> > I'll take Peff's hint, tweak/add comments for correctness and symmetry
> > with the previous patch and add an if-BUG for symmetry.
> 
> Here's a reroll of ma/split-symref-update-fix. The first three patches
> are v3 plus Michael's Reviewed-By.
> 
> The fourth is the conceptual fix of adding `refname` instead of "HEAD"
> into the list of affected refnames.
> 
> Thanks all for comments, suggestions and help along the way.

This looks good to me, including the explanation in the new patch.
Thanks!

-Peff

Re: [PATCH 00/12] Clean up notes-related code around `load_subtree()`

[Jeff King]

On Sat, Aug 26, 2017 at 10:28:00AM +0200, Michael Haggerty wrote:

> It turns out that the comment is incorrect, but there was nevertheless
> plenty that could be cleaned up in the area:
> 
> * Make macro `GIT_NIBBLE` safer by adding some parentheses
> * Remove some dead code
> * Fix some memory leaks
> * Fix some obsolete and incorrect comments
> * Reject "notes" that are not blobs
> 
> I hope the result is also easier to understand.
> 
> This branch is also available from my Git fork [1] as branch
> `load-subtree-cleanup`.

FYI, Coverity seems to complain about "pu" after this series is merged, but
I think it's wrong.  It says:

  *** CID 1417630:  Memory - illegal accesses  (OVERRUN)
  /notes.c: 458 in load_subtree()
  452 
  453   /*
  454* Pad the rest of the SHA-1 with zeros,
  455* except for the last byte, where we write
  456* the length:
  457*/
  >>> CID 1417630:  Memory - illegal accesses  (OVERRUN)
  >>> Overrunning array of 20 bytes at byte offset 20 by dereferencing 
pointer "_oid.hash[len]".
  458   memset(object_oid.hash + len, 0, GIT_SHA1_RAWSZ 
- len - 1);
  459   object_oid.hash[KEY_INDEX] = (unsigned char)len;
  460 
  461   type = PTR_TYPE_SUBTREE;
  462   } else {
  463   /* This can't be part of a note */

I agree that if "len" were 20 here that would be a problem, but I don't
think that's possible.

The tool correctly claims that prefix_len can be up to 19, due to the
assert:

 3. cond_at_most: Checking prefix_len >= 20UL implies that prefix_len may 
be up to 19 on the false branch.
  420if (prefix_len >= GIT_SHA1_RAWSZ)
  421BUG("prefix_len (%"PRIuMAX") is out of range", 
(uintmax_t)prefix_len);

Then it claims:

13. Condition path_len == 2 * (20 - prefix_len), taking false branch.
  430if (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) {
  431/* This is potentially the remainder of the SHA-1 
*/

So we know that either prefix_len is not 19, or that path_len is not 2
(since that combination would cause us to take the true branch here).
But then it goes on to say:

14. Condition path_len == 2, taking true branch.
  442} else if (path_len == 2) {
  443/* This is potentially an internal node */

which I believe must mean that prefix_len cannot be 19 here. And yet it
says:

15. assignment: Assigning: len = prefix_len. The value of len may now be up 
to 19.
  444size_t len = prefix_len;
  445
  [...]
 17. incr: Incrementing len. The value of len may now be up to 20.
 18. Condition hex_to_bytes(_oid.hash[len++], entry.path, 1), taking 
false branch.
  450if (hex_to_bytes(object_oid.hash + len++, 
entry.path, 1))
  451goto handle_non_note; /* entry.path is not 
a SHA1 */

I think that's impossible, and Coverity simply isn't smart enough to
shrink the set of possible values for prefix_len based on the set of
if-else conditions.

So nothing to see here, but since I spent 20 minutes scratching my head
(and I know others look at Coverity output and may scratch their heads
too), I thought it was worth writing up. And also if I'm wrong, it would
be good to know. ;)

-Peff

Funds

[Barrister Henry van loo]

I am Henry Ivan Loo,an Attorney at law have a Next of Kin Proposal for you. 
A deceased client of mine left a total sum of ($50,600,000.00 USD) without any 
next of Kin as regards his funds. This totally legal with all documents to back 
you up as the next of kin. for more details if interested, contact Henry Ivan 
Loo
only on henryivanloo1...@yahoo.com with your personal or private email. 
Best Regards
Henry Ivan Loo

[PATCH v4 0/4] Rerolling ma/split-symref-update-fix

[Martin Ågren]

> I'll take Peff's hint, tweak/add comments for correctness and symmetry
> with the previous patch and add an if-BUG for symmetry.

Here's a reroll of ma/split-symref-update-fix. The first three patches
are v3 plus Michael's Reviewed-By.

The fourth is the conceptual fix of adding `refname` instead of "HEAD"
into the list of affected refnames.

Thanks all for comments, suggestions and help along the way.

Martin

Martin Ågren (4):
  refs/files-backend: add longer-scoped copy of string to list
  refs/files-backend: fix memory leak in lock_ref_for_update
  refs/files-backend: correct return value in lock_ref_for_update
  refs/files-backend: add `refname`, not "HEAD", to list

 refs/files-backend.c | 62 +---
 1 file changed, 44 insertions(+), 18 deletions(-)

-- 
2.14.1.460.g848a19d64

[PATCH v4 4/4] refs/files-backend: add `refname`, not "HEAD", to list

[Martin Ågren]

An earlier patch rewrote `split_symref_update()` to add a copy of a
string to a string list instead of adding the original string. That was
so that the original string could be freed in a later patch, but it is
also conceptually cleaner, since now all calls to `string_list_insert()`
and `string_list_append()` add `update->refname`. --- Except a literal
"HEAD" is added in `split_head_update()`.

Restructure `split_head_update()` in the same way as the earlier patch
did for `split_symref_update()`. This does not correct any practical
problem, but makes things conceptually cleaner. The downside is a call
to `string_list_has_string()`, which should be relatively cheap.

Signed-off-by: Jeff King 
Signed-off-by: Martin Ågren 
---
 refs/files-backend.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 03df00275..926f87b94 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2589,11 +2589,10 @@ static int split_head_update(struct ref_update *update,
 
/*
 * First make sure that HEAD is not already in the
-* transaction. This insertion is O(N) in the transaction
+* transaction. This check is O(lg N) in the transaction
 * size, but it happens at most once per transaction.
 */
-   item = string_list_insert(affected_refnames, "HEAD");
-   if (item->util) {
+   if (string_list_has_string(affected_refnames, "HEAD")) {
/* An entry already existed */
strbuf_addf(err,
"multiple updates for 'HEAD' (including one "
@@ -2608,6 +2607,14 @@ static int split_head_update(struct ref_update *update,
update->new_oid.hash, update->old_oid.hash,
update->msg);
 
+   /*
+* Add "HEAD". This insertion is O(N) in the transaction
+* size, but it happens at most once per transaction.
+* Add new_update->refname instead of a literal "HEAD".
+*/
+   if (strcmp(new_update->refname, "HEAD"))
+   BUG("%s unexpectedly not 'HEAD'", new_update->refname);
+   item = string_list_insert(affected_refnames, new_update->refname);
item->util = new_update;
 
return 0;
-- 
2.14.1.460.g848a19d64

[PATCH v4 3/4] refs/files-backend: correct return value in lock_ref_for_update

[Martin Ågren]

In one code path we return a literal -1 and not a symbolic constant. The
value -1 would be interpreted as TRANSACTION_NAME_CONFLICT, which is
wrong. Use TRANSACTION_GENERIC_ERROR instead (that is the only other
return value we have to choose from).

Noticed-by: Michael Haggerty 
Reviewed-by: Michael Haggerty 
Signed-off-by: Martin Ågren 
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3d6363966..03df00275 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2801,7 +2801,7 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
strbuf_addf(err, "cannot lock ref '%s': 
"
"error reading reference",

original_update_refname(update));
-   ret = -1;
+   ret = TRANSACTION_GENERIC_ERROR;
goto out;
}
} else if (check_old_oid(update, >old_oid, err)) {
-- 
2.14.1.460.g848a19d64

[PATCH v4 2/4] refs/files-backend: fix memory leak in lock_ref_for_update

[Martin Ågren]

After the previous patch, none of the functions we call hold on to
`referent.buf`, so we can safely release the string buffer before
returning.

Reviewed-by: Michael Haggerty 
Signed-off-by: Martin Ågren 
---
 refs/files-backend.c | 31 ---
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index baceef3b3..3d6363966 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2756,7 +2756,7 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
struct strbuf referent = STRBUF_INIT;
int mustexist = (update->flags & REF_HAVE_OLD) &&
!is_null_oid(>old_oid);
-   int ret;
+   int ret = 0;
struct ref_lock *lock;
 
files_assert_main_repository(refs, "lock_ref_for_update");
@@ -2768,7 +2768,7 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
ret = split_head_update(update, transaction, head_ref,
affected_refnames, err);
if (ret)
-   return ret;
+   goto out;
}
 
ret = lock_raw_ref(refs, update->refname, mustexist,
@@ -2782,7 +2782,7 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
strbuf_addf(err, "cannot lock ref '%s': %s",
original_update_refname(update), reason);
free(reason);
-   return ret;
+   goto out;
}
 
update->backend_data = lock;
@@ -2801,10 +2801,12 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
strbuf_addf(err, "cannot lock ref '%s': 
"
"error reading reference",

original_update_refname(update));
-   return -1;
+   ret = -1;
+   goto out;
}
} else if (check_old_oid(update, >old_oid, err)) {
-   return TRANSACTION_GENERIC_ERROR;
+   ret = TRANSACTION_GENERIC_ERROR;
+   goto out;
}
} else {
/*
@@ -2818,13 +2820,15 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
  referent.buf, transaction,
  affected_refnames, err);
if (ret)
-   return ret;
+   goto out;
}
} else {
struct ref_update *parent_update;
 
-   if (check_old_oid(update, >old_oid, err))
-   return TRANSACTION_GENERIC_ERROR;
+   if (check_old_oid(update, >old_oid, err)) {
+   ret = TRANSACTION_GENERIC_ERROR;
+   goto out;
+   }
 
/*
 * If this update is happening indirectly because of a
@@ -2861,7 +2865,8 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
"cannot update ref '%s': %s",
update->refname, write_err);
free(write_err);
-   return TRANSACTION_GENERIC_ERROR;
+   ret = TRANSACTION_GENERIC_ERROR;
+   goto out;
} else {
update->flags |= REF_NEEDS_COMMIT;
}
@@ -2875,10 +2880,14 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
if (close_ref(lock)) {
strbuf_addf(err, "couldn't close '%s.lock'",
update->refname);
-   return TRANSACTION_GENERIC_ERROR;
+   ret = TRANSACTION_GENERIC_ERROR;
+   goto out;
}
}
-   return 0;
+
+out:
+   strbuf_release();
+   return ret;
 }
 
 /*
-- 
2.14.1.460.g848a19d64

[PATCH v4 1/4] refs/files-backend: add longer-scoped copy of string to list

[Martin Ågren]

split_symref_update() receives a string-pointer `referent` and adds it
to the list of `affected_refnames`. The list simply holds on to the
pointers it is given, it does not copy the strings and it does not ever
free them. The `referent` string in split_symref_update() belongs to a
string buffer in the caller. After we return, the string will be leaked.

In the next patch, we want to properly release the string buffer in the
caller, but we can't safely do so until we've made sure that
`affected_refnames` will not be holding on to a pointer to the string.
We could configure the list to handle its own resources, but it would
mean some alloc/free-churning. The list is already handling other
strings (through other code paths) which we do not need to worry about,
and we'd be memory-churning those strings too, completely unnecessary.

Observe that split_symref_update() creates a `new_update`-object through
ref_transaction_add_update(), after which `new_update->refname` is a
copy of `referent`. The difference is, this copy will be freed, and it
will be freed *after* `affected_refnames` has been cleared.

Rearrange the handling of `referent`, so that we don't add it directly
to `affected_refnames`. Instead, first just check whether `referent`
exists in the string list, and later add `new_update->refname`.

Helped-by: Michael Haggerty 
Reviewed-by: Michael Haggerty 
Signed-off-by: Martin Ågren 
---
 refs/files-backend.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0404f2c23..baceef3b3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2634,13 +2634,12 @@ static int split_symref_update(struct files_ref_store 
*refs,
 
/*
 * First make sure that referent is not already in the
-* transaction. This insertion is O(N) in the transaction
+* transaction. This check is O(lg N) in the transaction
 * size, but it happens at most once per symref in a
 * transaction.
 */
-   item = string_list_insert(affected_refnames, referent);
-   if (item->util) {
-   /* An entry already existed */
+   if (string_list_has_string(affected_refnames, referent)) {
+   /* An entry already exists */
strbuf_addf(err,
"multiple updates for '%s' (including one "
"via symref '%s') are not allowed",
@@ -2675,6 +2674,17 @@ static int split_symref_update(struct files_ref_store 
*refs,
update->flags |= REF_LOG_ONLY | REF_NODEREF;
update->flags &= ~REF_HAVE_OLD;
 
+   /*
+* Add the referent. This insertion is O(N) in the transaction
+* size, but it happens at most once per symref in a
+* transaction. Make sure to add new_update->refname, which will
+* be valid as long as affected_refnames is in use, and NOT
+* referent, which might soon be freed by our caller.
+*/
+   item = string_list_insert(affected_refnames, new_update->refname);
+   if (item->util)
+   BUG("%s unexpectedly found in affected_refnames",
+   new_update->refname);
item->util = new_update;
 
return 0;
-- 
2.14.1.460.g848a19d64

Re: "git shortlog -sn --follow -- " counts all commits to entire repo

[Jeff King]

On Sat, Sep 09, 2017 at 02:37:20AM +0900, Junio C Hamano wrote:

> > That said, I don't think we can go wrong by making shortlog's traversal
> > more like log's. Any changes we make to --follow will be aimed at and
> > tested with git-log, so the more code they share the more likely it is
> > that shortlog won't bitrot.
> 
> Both true.  
> 
> Using log-tree traversal machinery instead of just get_revision()
> would probably mean we would slow it down quite a bit unless we are
> careful, but at the same time, things like "git shortlog -G"
> would suddenly start working, so this is not just helping the
> "--follow" hack.

I didn't notice that, but I'm not surprised that there are more options
that shortlog doesn't quite work with.

I don't plan on working on this myself any time soon, so maybe it's a
good #leftoverbits candidate (though it's perhaps a little more involved
than some).

-Peff

Re: [PATCH v4 00/16] Fix git-gc losing objects in multi worktree

[Michael Haggerty]

On 08/23/2017 02:36 PM, Nguyễn Thái Ngọc Duy wrote:
> "git gc" when used in multiple worktrees ignore some per-worktree
> references: object references in the index, HEAD and reflog. This
> series fixes it by making the revision walker include these from all
> worktrees by default (and the series is basically split in three parts
> in the same order). There's a couple more cleanups in refs.c. Luckily
> it does not conflict with anything in 'pu'.
> 
> Compared to v3 [1], the largest change is supporting multi worktree in
> the reflog iterator. The merge iterator is now used (Micheal was right
> all along).

I read over all of the refs-related changes in this patch series. Aside
from the comments that I left, they look good to me. Thanks!

Michael

Re: [PATCH v4 16/16] refs.c: reindent get_submodule_ref_store()

[Michael Haggerty]

On 08/23/2017 02:37 PM, Nguyễn Thái Ngọc Duy wrote:
> With the new "if (!submodule) return NULL;" code added in the previous
> commit, we don't need to check if submodule is not NULL anymore.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs.c | 12 +---
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index a0c5078901..206af61d62 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1590,13 +1590,11 @@ struct ref_store *get_submodule_ref_store(const char 
> *submodule)
>   if (!submodule)
>   return NULL;
>  
> - if (submodule) {
> - len = strlen(submodule);
> - while (len && is_dir_sep(submodule[len - 1]))
> - len--;
> - if (!len)
> - return NULL;
> - }
> + len = strlen(submodule);
> + while (len && is_dir_sep(submodule[len - 1]))
> + len--;
> + if (!len)
> + return NULL;
>  
>   if (submodule[len])
>   /* We need to strip off one or more trailing slashes */
> 

Now I'm confused. Is it still allowed to call
`get_submodule_ref_store(NULL)`? If not, then the previous commit should
probably do

if (!submdule)
BUG(...);

Either way, it seems like it would be natural to combine this commit
with the previous one.

Michael

Re: [PATCH v4 15/16] refs.c: remove fallback-to-main-store code get_submodule_ref_store()

[Michael Haggerty]

On 08/23/2017 02:37 PM, Nguyễn Thái Ngọc Duy wrote:
> At this state, there are three get_submodule_ref_store() callers:
> 
>  - for_each_remote_ref_submodule()
>  - handle_revision_pseudo_opt()
>  - resolve_gitlink_ref()
> 
> The first two deal explicitly with submodules (and we should never fall
> back to the main ref store as a result). They are only called from
> submodule.c:
> 
>  - find_first_merges()
>  - submodule_needs_pushing()
>  - push_submodule()
> 
> The last one, as its name implies, deals only with submodules too, and
> the "submodule" (path) argument must be a non-NULL, non-empty string.
> 
> So, this "if NULL or empty string" code block should never ever
> trigger. And it's wrong to fall back to the main ref store
> anyway. Delete it.

Nice! Thanks for the cleanup.

Michael

Re: [PATCH v4 12/16] files-backend: make reflog iterator go through per-worktree reflog

[Michael Haggerty]

On 08/23/2017 02:37 PM, Nguyễn Thái Ngọc Duy wrote:
> refs/bisect is unfortunately per-worktree, so we need to look in
> per-worktree logs/refs/bisect in addition to per-repo logs/refs. The
> current iterator only goes through per-repo logs/refs.
> 
> Use merge iterator to walk two ref stores at the same time and pick
> per-worktree refs from the right iterator.
> 
> PS. Note the unsorted order of for_each_reflog in the test. This is
> supposed to be OK, for now. If we enforce order on for_each_reflog()
> then some more work will be required.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c  | 59 
> +--
>  t/t1407-worktree-ref-store.sh | 30 ++
>  2 files changed, 75 insertions(+), 14 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 5cca55510b..d4d22882ef 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> [...]
> +static enum iterator_selection reflog_iterator_select(
> + struct ref_iterator *iter_worktree,
> + struct ref_iterator *iter_common,
> + void *cb_data)
> +{
> + if (iter_worktree) {
> + /*
> +  * We're a bit loose here. We probably should ignore
> +  * common refs if they are accidentally added as
> +  * per-worktree refs.
> +  */
> + return ITER_SELECT_0;

I don't understand the point of the comment. If we should ignore common
refs here, why not do it rather than commenting about it? Wouldn't it be
really easy to implement? OTOH if it's not needed, then why the comment?

> [...]

Michael

Re: [PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees

[Michael Haggerty]

On 08/23/2017 02:36 PM, Nguyễn Thái Ngọc Duy wrote:
> [...]
> diff --git a/revision.c b/revision.c
> index 8d04516266..0e98444857 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2133,6 +2133,14 @@ static int handle_revision_pseudo_opt(const char 
> *submodule,
>   int argcount;
>  
>   if (submodule) {
> + /*
> +  * We need some something like get_submodule_worktrees()
> +  * before we can go through all worktrees of a submodule,
> +  * .e.g with adding all HEADs from --all, which is not
> +  * supported right now, so stick to single worktree.
> +  */
> + if (!revs->single_worktree)
> + die("BUG: --single-worktree cannot be used together 
> with submodule");
>   refs = get_submodule_ref_store(submodule);
>   } else
>   refs = get_main_ref_store();

Tiny nit: s/.e.g/e.g./

> [...]

Michael