[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, &lock->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(&update->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, &lock->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, &lock->old_oid, err))
-   return TRANSACTION_GENERIC_ERROR;
+   if (check_old_oid(update, &lock->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(&referent);
+   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

Re: [PATCH v4 10/16] refs: remove dead for_each_*_submodule()

[Michael Haggerty]

On 08/23/2017 02:36 PM, Nguyễn Thái Ngọc Duy wrote:
> These are used in revision.c. After the last patch they are replaced
> with the refs_ version. Delete them.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  Documentation/technical/api-ref-iteration.txt |  7 ++
>  refs.c| 33 
> ---
>  refs.h| 10 
>  3 files changed, 2 insertions(+), 48 deletions(-)

What a lovely diffstat :-)

Michael

Re: [PATCH v4 06/16] refs: move submodule slash stripping code to get_submodule_ref_store

[Michael Haggerty]

On 08/23/2017 02:36 PM, Nguyễn Thái Ngọc Duy wrote:
> This is a better place that will benefit all submodule callers instead
> of just resolve_gitlink_ref()

This is a nice sentiment, but I have to wonder whether we should rather
be requiring callers to use the "canonical" submodule name rather than
covering up their sloppiness for them (perhaps multiple times?). I
vaguely remember intentionally limiting the number of functions that do
this canonicalization, and having in mind the goal that the number
should become smaller over time, not larger.

For example, there could be some kind of `canonicalize_submodule_name()`
function that callers can use to clean up whatever submodule string they
have in hand, then let them use the cleaned up value from then on.

I don't really know much about the callers, though, so that is more of a
lazy philosophical speculation than a concrete suggestion.

Michael

RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Friday, September 8, 2017 9:18 PM
> 
> Kevin Willford  writes:
> 
> > 1. reset mixed when there were files that were added
> >
> > In this case the index will no longer have the entry at all because
> > the reset is making the index look like before the file was added
> > which didn't have it. When not using the sparse-checkout this is fine
> > because the file is in the working directory and the reset didn't touch
> > it.  But using the sparse-checkout on that file and if the file was not
> > in the working directory, the index gets reset and the entry for that
> > file is gone and if we don't put the index version of the file before the
> > reset into the working directory, then we have lost the content for
> > that file
> 
> I do not quite understand this argument.  If you do
> 
>   edit $path
>   git add $path
>   rm $path
>   git reset
> 
> for a $path that is not involved in the sparse thing, the version
> that was previously indexed will be lost, but that is fine---the
> user said that version is expendable by saying "reset".
> 
> How would that be different when the $path were not to be
> materialized in the working tree due to sparseness?  Where did that
> "blob" object in the index immediately before you called "reset"
> came from, and why do you say that the user does *not* consider that
> one expendable, unlike the case for non-sparse path example above?
> 

I guess that I should have said files that were newly added, meaning they
are new files that were created and added in the previous commit.
 
I think that the difference is that the user explicitly removed the file. When
using sparse it is git that is causing the removal of the file.  For example
if I have /file in my spare-checkout file so that I am only working on the one
file.  The previous commit had new file2 added and I run a git reset HEAD~1.
I as the user do not expect that file2 just disappear, but yet that is what
happens.  So from you example above if I do.

create $path
git add $path
git commit
git checkout // where $path is not in the sparse-checkout
git reset HEAD~1

$path will be gone yet I the user did not remove it.  I guess you could
argue that the user did when they specified their sparse-checkout and
ran checkout but I wouldn't know what would go missing unless I ran
a diff before the reset to see.  There could have been X number of
files created and added in the previous commit(s) and status after the
reset would not report them and they are gone.  So I could clone,
setup sparse-checkout, checkout, reset HEAD~X and possibly lose
data I didn't expect to.

In the modified case where the previous commits have modifications
to files outside the sparse-checkout at least the status after the reset reports
the file as deleted so the user sees that something has happened to it.

I suppose the entry could stay in the index with the skip-worktree bit
on and not removed like it is now so that a git reset will only apply
to the entries in the sparse-checkout?  That seems like it would be
changing the meaning of reset.

> I suspect that a similar reasoning would apply to your 2., but I
> didn't think it through.
> 
> The possible misconception, which I perceive in both of these, is
> that you are somehow disagreeing with this basic assumption: by
> saying "git reset []", the user is telling us that the
> version in the index, even if that is different from HEAD,
> , or the file in the working tree, is *unwanted* and be
> replaced with the one in HEAD (or  when given).  Touching
> the working tree files upon "git reset" is the last thing the user
> expects to happen.
> 

I agree with this when you are not dealing with a sparse-checkout.
When using a sparse-checkout I expect git not to touch things
outside of what I have specified in my sparse-checkout file.  If it
does, it should let me know or put my working directory in a
state that is expected.  Especially when it is changing the
skip-worktree bits causing files outside the sparse-checkout to be
reported incorrectly by status.

 

Re: [PATCH] commit-tree: don't append a newline with -F

[Junio C Hamano]

Ross Kabus  writes:

> On Thu, Sep 7, 2017 at 9:35 PM, Junio C Hamano  wrote:
>> commit-tree: do not complete line in -F input
>>
>> "git commit-tree -F ", unlike "cat  | git
>> commit-tree" (i.e. feeding the same contents from the standard
>> input), added a missing final newline when the input ended in an
>> incomplete line.
>>
>> Correct this inconsistency by leaving the incomplete line as-is,
>> as erring on the side of not touching the input is preferrable
>> and expected for a plumbing command like "commit-tree".
>
> That all makes sense to me and is clearer too, I like this change.
> Do I need to resubmit the patch or will you just use that text instead?

Unless the change is in the middle of a large series or require more
involved edit [*1*], it is perfectly fine to tell me the maintainer
to do these small menial changes on behalf of you---you explicitly
need to tell me what you want to happen, though.

Thanks.

[Footnote]

*1* ... in which case we have a larger chance of the maintainer
screwing up while attempting to do so, and I'd prefer to see an
updated version posted on the list.  That way, we can benefit
from extra set of eyeballs watching our exchange from the
sideline.

Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Junio C Hamano]

Kevin Willford  writes:

> 1. reset mixed when there were files that were added
>
> In this case the index will no longer have the entry at all because
> the reset is making the index look like before the file was added
> which didn't have it. When not using the sparse-checkout this is fine
> because the file is in the working directory and the reset didn't touch
> it.  But using the sparse-checkout on that file and if the file was not
> in the working directory, the index gets reset and the entry for that
> file is gone and if we don't put the index version of the file before the
> reset into the working directory, then we have lost the content for
> that file

I do not quite understand this argument.  If you do

edit $path
git add $path
rm $path
git reset

for a $path that is not involved in the sparse thing, the version
that was previously indexed will be lost, but that is fine---the
user said that version is expendable by saying "reset".

How would that be different when the $path were not to be
materialized in the working tree due to sparseness?  Where did that
"blob" object in the index immediately before you called "reset"
came from, and why do you say that the user does *not* consider that
one expendable, unlike the case for non-sparse path example above?

I suspect that a similar reasoning would apply to your 2., but I
didn't think it through.  

The possible misconception, which I perceive in both of these, is
that you are somehow disagreeing with this basic assumption: by
saying "git reset []", the user is telling us that the
version in the index, even if that is different from HEAD,
, or the file in the working tree, is *unwanted* and be
replaced with the one in HEAD (or  when given).  Touching
the working tree files upon "git reset" is the last thing the user
expects to happen.

Re: [PATCH] commit-tree: don't append a newline with -F

[Ross Kabus]

On Thu, Sep 7, 2017 at 9:35 PM, Junio C Hamano  wrote:
> commit-tree: do not complete line in -F input
>
> "git commit-tree -F ", unlike "cat  | git
> commit-tree" (i.e. feeding the same contents from the standard
> input), added a missing final newline when the input ended in an
> incomplete line.
>
> Correct this inconsistency by leaving the incomplete line as-is,
> as erring on the side of not touching the input is preferrable
> and expected for a plumbing command like "commit-tree".

That all makes sense to me and is clearer too, I like this change.
Do I need to resubmit the patch or will you just use that text instead?

RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

From: Junio C Hamano [mailto:gits...@pobox.com]
Sent: Friday, September 8, 2017 1:02 PM
> Kevin Willford  writes:
> 
> > diff --git a/builtin/reset.c b/builtin/reset.c
> > index d72c7d1c96..1b8bb45989 100644
> > --- a/builtin/reset.c
> > +++ b/builtin/reset.c
> > @@ -24,6 +24,7 @@
> >  #include "cache-tree.h"
> >  #include "submodule.h"
> >  #include "submodule-config.h"
> > +#include "dir.h"
> >
> >  static const char * const git_reset_usage[] = {
> > N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q]
> []"),
> > @@ -124,8 +125,32 @@ static void update_index_from_diff(struct
> diff_queue_struct *q,
> >
> > for (i = 0; i < q->nr; i++) {
> > struct diff_filespec *one = q->queue[i]->one;
> > +   struct diff_filespec *two = q->queue[i]->two;
> > +   int pos;
> > int is_missing = !(one->mode && !is_null_oid(&one->oid));
> > +   int was_missing = !two->mode && is_null_oid(&two->oid);
> > struct cache_entry *ce;
> > +   struct cache_entry *ceBefore;
> > +   struct checkout state = CHECKOUT_INIT;
> 
> The five new variables are only used in the new block, so it
> probably is better to limit their scope to the "we do something
> unusual when sparse checkout is in effect" block as well.  The scope
> for the latter two can further be narrowed down to "we do need to
> force a checkout of this entry" block.
> 
> We prefer to name our variables with underscore (e.g. ce_before)
> over camelCase (e.g. ceBefore) unless there is a compelling reason
> (e.g. a platform specific code in compat/ layer to match platform
> convention).
> 

Will update.

> > +
> > +   if (core_apply_sparse_checkout && !file_exists(two->path)) {
> > +   pos = cache_name_pos(two->path, strlen(two->path));
> > +   if ((pos >= 0 && ce_skip_worktree(active_cache[pos]))
> &&
> > +   (is_missing || !was_missing))
> > +   {
> > +   state.force = 1;
> > +   state.refresh_cache = 1;
> > +   state.istate = &the_index;
> > +   ceBefore = make_cache_entry(two->mode,
> > +   two->oid.hash,
> > +   two->path, 0, 0);
> > +   if (!ceBefore)
> > +   die(_("make_cache_entry failed for path
> '%s'"),
> > +   two->path);
> > +
> > +   checkout_entry(ceBefore, &state, NULL);
> > +   }
> > +   }
> 
> Can we tell between the case where the reason why the path was not
> there in the working tree was due to the path being excluded by the
> sparse checkout and the path being removed manually by the end user?
> 
> I guess ce_skip_worktree() check is sufficient; we force checkout only
> when the path is marked to be skipped due to "sparse" thing.
> 
> Do we have to worry about the reverse case, in which file_exists(two->path)
> is true (i.e. the user created a file there manually) even though
> the path is marked to be skipped due to "sparse" setting?
> 

I don't believe so because if the user has a file there whether they modified
it or not, it is what the user did and we just leave it there and a diff with
what the index gets reset to will show how the file is different from what the
index got reset to.

> Other than that, the patch looks quite cleanly done and well explained.
> 
> Thanks.
> 

Re: Re: cat-file timing window on Cygwin

[Ramsay Jones]

On 27/08/17 16:47, Ramsay Jones wrote:
> On 27/08/17 12:33, Adam Dinwoodie wrote:
[snip]

>> Cygwin 2.8.3 hasn't been released yet, 
> 
> Heh, yes, I found that out myself this afternoon. ;-)
> 
>> but I've just tested the latest
>> development snapshot with Jeff's simple test case, and it works as
>> expected, so I'm going to assume the Git test will start passing once
>> that version of the Cygwin DLL is released too.

I noticed that cygwin 2.9.0 has been released, so I updated and
ran test t8010-cat-file-filters.sh which, as expected, now passes.

Since the above failure was caused, in part, by an errant errno
value, I decided to try reverting the fix for t0301-credential-cache.sh.
(ie. commit 1f180e5eb9, "credential-cache: interpret an ECONNRESET
as an EOF", 27-07-2017). However, that re-introduces the failure! ;-)

[I haven't done a complete test-suite run yet, but I don't expect
to have any failures - famous last words!]

ATB,
Ramsay Jones

RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

From: Junio C Hamano [mailto:gits...@pobox.com]
Sent: Friday, September 8, 2017 1:12 PM
> Kevin Willford  writes:
> 
> > When using the sparse checkout feature the git reset command will add
> > entries to the index that will have the skip-worktree bit off but will
> > leave the working directory empty.  File data is lost because the index
> > version of the files have been changed but there is nothing that is in the
> > working directory.  This will cause the next status call to show either
> > deleted for files modified or deleting or nothing for files added.
> 
> Getting rid of sparseness may of course be one way to correct the
> discrepancy, but it is unclear to me if that is the best way to do
> so.  An alternative may be to add entries to the index that does
> have the bit on for paths that are meant to be skipped due to
> "sparse" thing, no?  Am I being naive, missing some reason why that
> would give us a worse result?

There are two cases that this is trying to solve.

1. reset mixed when there were files that were added

In this case the index will no longer have the entry at all because
the reset is making the index look like before the file was added
which didn't have it. When not using the sparse-checkout this is fine
because the file is in the working directory and the reset didn't touch
it.  But using the sparse-checkout on that file and if the file was not
in the working directory, the index gets reset and the entry for that
file is gone and if we don't put the index version of the file before the
reset into the working directory, then we have lost the content for
that file

2. reset mixed when there were files modified

This case is similar but with modified files there is an entry in the index
but it is getting changed to a previous version of the file.  If we don't get
the file on disk then the version of the file that, in the non sparse-checkout
case, would be on disk is lost and cannot be recovered.  So even if we turn
on the skip-worktree bit for this entry and it doesn't show up in status
calls, we lost the previous version of the file.

Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Junio C Hamano]

Kevin Willford  writes:

> When using the sparse checkout feature the git reset command will add
> entries to the index that will have the skip-worktree bit off but will
> leave the working directory empty.  File data is lost because the index
> version of the files have been changed but there is nothing that is in the
> working directory.  This will cause the next status call to show either
> deleted for files modified or deleting or nothing for files added.  

Getting rid of sparseness may of course be one way to correct the
discrepancy, but it is unclear to me if that is the best way to do
so.  An alternative may be to add entries to the index that does
have the bit on for paths that are meant to be skipped due to
"sparse" thing, no?  Am I being naive, missing some reason why that
would give us a worse result?

Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Junio C Hamano]

Torsten Bögershausen  writes:

>> +test_expect_success 'setup' '
>> +test_tick &&
>
> Do we need a test_tick here ?

As the test does not check against exact object names, and it does
not create commits, the order among which needs to be tiebroken by
using the committer timestamp, it is not strictly necessary, but I
do not think it would hurt, either.

>
>> +echo "checkout file" >c &&
>> +echo "modify file" >m &&
>> +echo "delete file" >d &&
>> +git add . &&
>> +git commit -m "initial commit" &&
>> +echo "added file" >a &&
>> +echo "modification of a file" >m &&
>> +git rm d &&
>> +git add . &&
>> +git commit -m "second commit" &&
>> +git checkout -b endCommit
>> +'
>> +
>> +test_expect_success 'reset when there is a sparse-checkout' '
>> +echo "/c" >.git/info/sparse-checkout &&
>> +test_config core.sparsecheckout true &&
>> +git checkout -b resetBranch &&
>> +test_path_is_missing m &&
>> +test_path_is_missing a &&
>> +test_path_is_missing d &&
>> +git reset HEAD~1 &&
>> +test "checkout file" = "$(cat c)" &&
>> +test "modification of a file" = "$(cat m)" &&
>> +test "added file" = "$(cat a)" &&
>> +test_path_is_missing d
>> +'
>> +

Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Junio C Hamano]

Kevin Willford  writes:

> diff --git a/builtin/reset.c b/builtin/reset.c
> index d72c7d1c96..1b8bb45989 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -24,6 +24,7 @@
>  #include "cache-tree.h"
>  #include "submodule.h"
>  #include "submodule-config.h"
> +#include "dir.h"
>  
>  static const char * const git_reset_usage[] = {
>   N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] 
> []"),
> @@ -124,8 +125,32 @@ static void update_index_from_diff(struct 
> diff_queue_struct *q,
>  
>   for (i = 0; i < q->nr; i++) {
>   struct diff_filespec *one = q->queue[i]->one;
> + struct diff_filespec *two = q->queue[i]->two;
> + int pos;
>   int is_missing = !(one->mode && !is_null_oid(&one->oid));
> + int was_missing = !two->mode && is_null_oid(&two->oid);
>   struct cache_entry *ce;
> + struct cache_entry *ceBefore;
> + struct checkout state = CHECKOUT_INIT;

The five new variables are only used in the new block, so it
probably is better to limit their scope to the "we do something
unusual when sparse checkout is in effect" block as well.  The scope
for the latter two can further be narrowed down to "we do need to
force a checkout of this entry" block.

We prefer to name our variables with underscore (e.g. ce_before)
over camelCase (e.g. ceBefore) unless there is a compelling reason
(e.g. a platform specific code in compat/ layer to match platform
convention).

> +
> + if (core_apply_sparse_checkout && !file_exists(two->path)) {
> + pos = cache_name_pos(two->path, strlen(two->path));
> + if ((pos >= 0 && ce_skip_worktree(active_cache[pos])) &&
> + (is_missing || !was_missing))
> + {
> + state.force = 1;
> + state.refresh_cache = 1;
> + state.istate = &the_index;
> + ceBefore = make_cache_entry(two->mode,
> + two->oid.hash,
> + two->path, 0, 0);
> + if (!ceBefore)
> + die(_("make_cache_entry failed for path 
> '%s'"),
> + two->path);
> +
> + checkout_entry(ceBefore, &state, NULL);
> + }
> + }

Can we tell between the case where the reason why the path was not
there in the working tree was due to the path being excluded by the
sparse checkout and the path being removed manually by the end user?

I guess ce_skip_worktree() check is sufficient; we force checkout only
when the path is marked to be skipped due to "sparse" thing.

Do we have to worry about the reverse case, in which file_exists(two->path)
is true (i.e. the user created a file there manually) even though
the path is marked to be skipped due to "sparse" setting?

Other than that, the patch looks quite cleanly done and well explained.

Thanks.

> diff --git a/t/t7114-reset-sparse-checkout.sh 
> b/t/t7114-reset-sparse-checkout.sh
> new file mode 100755
> index 00..f2a5426847
> --- /dev/null
> +++ b/t/t7114-reset-sparse-checkout.sh
> @@ -0,0 +1,60 @@
> +#!/bin/sh
> +
> +test_description='reset when using a sparse-checkout'
> +
> +. ./test-lib.sh
> +
> +# reset using a sparse-checkout file
> +
> +test_expect_success 'setup' '
> + test_tick &&
> + echo "checkout file" >c &&
> + echo "modify file" >m &&
> + echo "delete file" >d &&
> + git add . &&
> + git commit -m "initial commit" &&
> + echo "added file" >a &&
> + echo "modification of a file" >m &&
> + git rm d &&
> + git add . &&
> + git commit -m "second commit" &&
> + git checkout -b endCommit
> +'
> +
> +test_expect_success 'reset when there is a sparse-checkout' '
> + echo "/c" >.git/info/sparse-checkout &&
> + test_config core.sparsecheckout true &&
> + git checkout -b resetBranch &&
> + test_path_is_missing m &&
> + test_path_is_missing a &&
> + test_path_is_missing d &&
> + git reset HEAD~1 &&
> + test "checkout file" = "$(cat c)" &&
> + test "modification of a file" = "$(cat m)" &&
> + test "added file" = "$(cat a)" &&
> + test_path_is_missing d
> +'
> +
> +test_expect_success 'reset after deleting file without skip-worktree bit' '
> + git checkout -f endCommit &&
> + git clean -xdf &&
> + cat >.git/info/sparse-checkout <<-\EOF &&
> + /c
> + /m
> + EOF
> + test_config core.sparsecheckout true &&
> + git checkout -b resetAfterDelete &&
> + test_path_is_file m &&
> + test_path_is_missing a &&
> + test_path_is_missing d &&
> + rm -f m &&
> + git reset HEAD~1 &&
> + test "checkout file" = "$(cat c)" &&
> + test "added file" = "$(cat a)" &&
>

Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Torsten Bögershausen]

On Fri, Sep 08, 2017 at 12:00:50PM -0600, Kevin Willford wrote:
> When using the sparse checkout feature the git reset command will add
> entries to the index that will have the skip-worktree bit off but will
> leave the working directory empty.  File data is lost because the index
> version of the files have been changed but there is nothing that is in the
> working directory.  This will cause the next status call to show either
> deleted for files modified or deleting or nothing for files added.  The
> added files should be shown as untracked and modified files should be
> shown as modified.
> 
> To fix this when the reset is running if there is not a file in the working
> directory and if it will be missing with the new index entry or was not
> missing in the previous version, we create the previous index version of
> the file in the working directory so that status will report correctly
> and the files will be availble for the user to deal with.
> 
> Signed-off-by: Kevin Willford 
[]
> diff --git a/t/t7114-reset-sparse-checkout.sh 
> b/t/t7114-reset-sparse-checkout.sh
> new file mode 100755
> index 00..f2a5426847
> --- /dev/null
> +++ b/t/t7114-reset-sparse-checkout.sh
> @@ -0,0 +1,60 @@
> +#!/bin/sh
> +
> +test_description='reset when using a sparse-checkout'
> +
> +. ./test-lib.sh
> +
> +# reset using a sparse-checkout file
> +
> +test_expect_success 'setup' '
> + test_tick &&

Do we need a test_tick here ?

> + echo "checkout file" >c &&
> + echo "modify file" >m &&
> + echo "delete file" >d &&
> + git add . &&
> + git commit -m "initial commit" &&
> + echo "added file" >a &&
> + echo "modification of a file" >m &&
> + git rm d &&
> + git add . &&
> + git commit -m "second commit" &&
> + git checkout -b endCommit
> +'
> +
> +test_expect_success 'reset when there is a sparse-checkout' '
> + echo "/c" >.git/info/sparse-checkout &&
> + test_config core.sparsecheckout true &&
> + git checkout -b resetBranch &&
> + test_path_is_missing m &&
> + test_path_is_missing a &&
> + test_path_is_missing d &&
> + git reset HEAD~1 &&
> + test "checkout file" = "$(cat c)" &&
> + test "modification of a file" = "$(cat m)" &&
> + test "added file" = "$(cat a)" &&
> + test_path_is_missing d
> +'
> +

[PATCH 0/1] reset: fix mixed reset when using sparse-checkout

[Kevin Willford]

Original discussion is here
https://public-inbox.org/git/20170407192357.948-4-kewi...@microsoft.com/

When running a reset mixed and using the sparse-checkout the working
directory needs to be updated so that there is not data loss when the
index is updated.  This is because the index is getting updated
potentially removing entries without changing the working directory.
When using the sparse-checkout feature the entries removed might not
be on disk and are lost.

This patch writes the before version of the file to disk if the mixed
reset is going to change the index of a file that had the skip-wortree
bit so that the file contents before the reset is preserved on disk and
status will reports the correct results.

Kevin Willford (1):
  reset: fix reset when using the sparse-checkout feature.

 builtin/reset.c  | 25 +
 t/t7114-reset-sparse-checkout.sh | 60 
 2 files changed, 85 insertions(+)
 create mode 100755 t/t7114-reset-sparse-checkout.sh

-- 
2.14.1.474.g0558484247.dirty

[PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

When using the sparse checkout feature the git reset command will add
entries to the index that will have the skip-worktree bit off but will
leave the working directory empty.  File data is lost because the index
version of the files have been changed but there is nothing that is in the
working directory.  This will cause the next status call to show either
deleted for files modified or deleting or nothing for files added.  The
added files should be shown as untracked and modified files should be
shown as modified.

To fix this when the reset is running if there is not a file in the working
directory and if it will be missing with the new index entry or was not
missing in the previous version, we create the previous index version of
the file in the working directory so that status will report correctly
and the files will be availble for the user to deal with.

Signed-off-by: Kevin Willford 
---
 builtin/reset.c  | 25 +
 t/t7114-reset-sparse-checkout.sh | 60 
 2 files changed, 85 insertions(+)
 create mode 100755 t/t7114-reset-sparse-checkout.sh

diff --git a/builtin/reset.c b/builtin/reset.c
index d72c7d1c96..1b8bb45989 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -24,6 +24,7 @@
 #include "cache-tree.h"
 #include "submodule.h"
 #include "submodule-config.h"
+#include "dir.h"
 
 static const char * const git_reset_usage[] = {
N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] 
[]"),
@@ -124,8 +125,32 @@ static void update_index_from_diff(struct 
diff_queue_struct *q,
 
for (i = 0; i < q->nr; i++) {
struct diff_filespec *one = q->queue[i]->one;
+   struct diff_filespec *two = q->queue[i]->two;
+   int pos;
int is_missing = !(one->mode && !is_null_oid(&one->oid));
+   int was_missing = !two->mode && is_null_oid(&two->oid);
struct cache_entry *ce;
+   struct cache_entry *ceBefore;
+   struct checkout state = CHECKOUT_INIT;
+
+   if (core_apply_sparse_checkout && !file_exists(two->path)) {
+   pos = cache_name_pos(two->path, strlen(two->path));
+   if ((pos >= 0 && ce_skip_worktree(active_cache[pos])) &&
+   (is_missing || !was_missing))
+   {
+   state.force = 1;
+   state.refresh_cache = 1;
+   state.istate = &the_index;
+   ceBefore = make_cache_entry(two->mode,
+   two->oid.hash,
+   two->path, 0, 0);
+   if (!ceBefore)
+   die(_("make_cache_entry failed for path 
'%s'"),
+   two->path);
+
+   checkout_entry(ceBefore, &state, NULL);
+   }
+   }
 
if (is_missing && !intent_to_add) {
remove_file_from_cache(one->path);
diff --git a/t/t7114-reset-sparse-checkout.sh b/t/t7114-reset-sparse-checkout.sh
new file mode 100755
index 00..f2a5426847
--- /dev/null
+++ b/t/t7114-reset-sparse-checkout.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='reset when using a sparse-checkout'
+
+. ./test-lib.sh
+
+# reset using a sparse-checkout file
+
+test_expect_success 'setup' '
+   test_tick &&
+   echo "checkout file" >c &&
+   echo "modify file" >m &&
+   echo "delete file" >d &&
+   git add . &&
+   git commit -m "initial commit" &&
+   echo "added file" >a &&
+   echo "modification of a file" >m &&
+   git rm d &&
+   git add . &&
+   git commit -m "second commit" &&
+   git checkout -b endCommit
+'
+
+test_expect_success 'reset when there is a sparse-checkout' '
+   echo "/c" >.git/info/sparse-checkout &&
+   test_config core.sparsecheckout true &&
+   git checkout -b resetBranch &&
+   test_path_is_missing m &&
+   test_path_is_missing a &&
+   test_path_is_missing d &&
+   git reset HEAD~1 &&
+   test "checkout file" = "$(cat c)" &&
+   test "modification of a file" = "$(cat m)" &&
+   test "added file" = "$(cat a)" &&
+   test_path_is_missing d
+'
+
+test_expect_success 'reset after deleting file without skip-worktree bit' '
+   git checkout -f endCommit &&
+   git clean -xdf &&
+   cat >.git/info/sparse-checkout <<-\EOF &&
+   /c
+   /m
+   EOF
+   test_config core.sparsecheckout true &&
+   git checkout -b resetAfterDelete &&
+   test_path_is_file m &&
+   test_path_is_missing a &&
+   test_path_is_missing d &&
+   rm -f m &&
+   git reset HEAD~1 &&
+   test "checkout file" = "$(cat c)" &&
+   test "added file" = "$(cat a)

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

[Junio C Hamano]

Jeff King  writes:

> Yeah. It depends on exactly how such a fix is made. I think one
> improvement would be to actually bump --follow handling into the
> limit_list() stage, so that we properly handle history simplification
> over followed paths. In which case get_revision() would just never
> return the uninteresting commits, and the current shortlog code would
> Just Work.
>
> 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.

Re: [PATCH] refs: make sure we never pass NULL to hashcpy

[Junio C Hamano]

Michael Haggerty  writes:

> So `ref_transaction_update()` *does* need to set or clear the `HAVE_NEW`
> and `HAVE_OLD` bits as I sketched, to impedance-match between the two
> conventions.

OK, so ignoring HAVE_NEW/HAVE_OLD bits that the callers of
ref_transaction_update() may set in flags, and having
ref_transaction_update() compute these bits based on new/old_sha1
pointers from scratch, would be the right thing to do.

IOW

flags &= ~(REF_HAVE_NEW|REF_HAVE_OLD);
if (new_sha1)
flags |= REF_HAVE_NEW;
if (old_sha1)
flags |= REF_HAVE_OLD;

and your earlier "Does the warning go away if you change the line
to" does essentially the same thing.

> It's a shame how much time we've wasted discussing this. Maybe the code
> is trying to be too clever/efficient and needs a rethink.

It might be the case, but I do not know what to blame is "the two
conventions", an over-eager compiler, or a confused commenter on the
thread (that's me), though ;-).

[PATCH] load_subtree(): check that `prefix_len` is in the expected range

[Michael Haggerty]

This value, which is stashed in the last byte of an object_id hash,
gets handed around a lot. So add a sanity check before using it in
`load_subtree()`.

Signed-off-by: Michael Haggerty 
---
This patch is an addendum to v1 of the mh/notes-cleanup patch series
[1]. It adds the assertion that was suggested by Junio [2].

Since the first patch series is already in next, this patch is
constructed to apply on top of that branch.

Thanks to Junio and Johan for their review of v1.

Michael

[1] https://public-inbox.org/git/cover.1503734566.git.mhag...@alum.mit.edu/
[2] https://public-inbox.org/git/xmqqh8wuqo6e@gitster.mtv.corp.google.com/

 notes.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/notes.c b/notes.c
index 40d9ba6252..27d232f294 100644
--- a/notes.c
+++ b/notes.c
@@ -417,7 +417,10 @@ static void load_subtree(struct notes_tree *t, struct 
leaf_node *subtree,
 oid_to_hex(&subtree->val_oid));
 
prefix_len = subtree->key_oid.hash[KEY_INDEX];
-   assert(prefix_len * 2 >= n);
+   if (prefix_len >= GIT_SHA1_RAWSZ)
+   BUG("prefix_len (%"PRIuMAX") is out of range", 
(uintmax_t)prefix_len);
+   if (prefix_len * 2 < n)
+   BUG("prefix_len (%"PRIuMAX") is too small", 
(uintmax_t)prefix_len);
memcpy(object_oid.hash, subtree->key_oid.hash, prefix_len);
while (tree_entry(&desc, &entry)) {
unsigned char type;
-- 
2.14.1

Re: [PATCH] refs: make sure we never pass NULL to hashcpy

[Michael Haggerty]

On 09/08/2017 02:46 AM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> I did just realize one thing: `ref_transaction_update()` takes `flags`
>> as an argument and alters it using
>>
>>> flags |= (new_sha1 ? REF_HAVE_NEW : 0) | (old_sha1 ? REF_HAVE_OLD : 
>>> 0);
>>
>> Perhaps gcc is *more* intelligent than we give it credit for, and is
>> actually worried that the `flags` argument passed in by the caller
>> might *already* have one of these bits set. In that case
>> `ref_transaction_add_update()` would indeed be called incorrectly.
>> Does the warning go away if you change that line to
>>
>>> if (new_sha1)
>>> flags |=REF_HAVE_NEW;
>>> else
>>> flags &= ~REF_HAVE_NEW;
>>> if (old_sha1)
>>> flags |=REF_HAVE_OLD;
>>> else
>>> flags &= ~REF_HAVE_OLD;
>>
>> ? This might be a nice change to have anyway, to isolate
>> `ref_transaction_update()` from mistakes by its callers.
> 
> I understand "drop HAVE_NEW bit if new_sha1 is NULL" part, but not
> the other side "add HAVE_NEW if new_SHA1 is not NULL"---doesn't the
> NEW/OLD flag exist exactly because some callers pass the address of
> an embedded oid.hash[] or null_sha1, instead of NULL, when one side 
> does not exist?  So new|old being NULL is a definite signal that we
> need to drop HAVE_NEW|OLD, but the reverse may not be true, no?  Is
> it OK to overwrite null_sha1[] that is passed from some codepaths?
> 
> ref_transaction_create and _delete pass null_sha1 on the missing
> side, while ref_transaction_verify passes NULL, while calling
> _update().  Should this distinction affect how _add_update() gets
> called?

There are two functions under discussion:

* `ref_transaction_add_update()` is the low-level, private function that
uses the `HAVE_{NEW,OLD}` bits to decide what to do.

* `ref_transaction_update()` (like
`ref_transaction_{create,delete,verify}()`) are public functions that
ignore the `HAVE_{NEW,OLD}` bits and base their behavior on whether
`new_sha1` and `old_sha1` are NULL.

Each of these functions has to support three possibilities for its SHA-1
arguments:

1. The SHA-1 is provided and not `null_sha1`—in this case it must match
the old value (if `old_sha1`) or it is the value to be set as the new
value (if `new_sha1`).

2. The SHA-1 is provided and is equal to `null_sha1`—in this case the
reference must not already exist (if `old_sha1` is `null_sha1`) or it
will be deleted (if `new_sha1` is `null_sha1`).

3. The SHA-1 is not provided at all—in this case the old value is
ignored (if `old_sha1` is not provided) or the reference is left
unchanged (if `new_sha1` is not provided).

Much of the current confusion stems because
`ref_transaction_add_update()` encodes the third condition using the
`REF_HAVE_*` bits, whereas `ref_transaction_update()` and its friends
encode the third condition by setting `old_sha1` or `new_sha1` to `NULL`.

So `ref_transaction_update()` *does* need to set or clear the `HAVE_NEW`
and `HAVE_OLD` bits as I sketched, to impedance-match between the two
conventions.

It's a shame how much time we've wasted discussing this. Maybe the code
is trying to be too clever/efficient and needs a rethink.

Michael

[PATCH v2 07/11] files_initial_transaction_commit(): use a transaction for packed refs

[Michael Haggerty]

Use a `packed_ref_store` transaction in the implementation of
`files_initial_transaction_commit()` rather than using internal
features of the packed ref store. This further decouples
`files_ref_store` from `packed_ref_store`.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 29 +++--
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 60031fe3ae..2700e3b5d5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2669,6 +2669,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
size_t i;
int ret = 0;
struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
+   struct ref_transaction *packed_transaction = NULL;
 
assert(err);
 
@@ -2701,6 +2702,12 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
 &affected_refnames))
die("BUG: initial ref transaction called with existing refs");
 
+   packed_transaction = 
ref_store_transaction_begin(refs->packed_ref_store, err);
+   if (!packed_transaction) {
+   ret = TRANSACTION_GENERIC_ERROR;
+   goto cleanup;
+   }
+
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
 
@@ -2713,6 +2720,15 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
ret = TRANSACTION_NAME_CONFLICT;
goto cleanup;
}
+
+   /*
+* Add a reference creation for this reference to the
+* packed-refs transaction:
+*/
+   ref_transaction_add_update(packed_transaction, update->refname,
+  update->flags & ~REF_HAVE_OLD,
+  update->new_oid.hash, 
update->old_oid.hash,
+  NULL);
}
 
if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
@@ -2720,21 +2736,14 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
goto cleanup;
}
 
-   for (i = 0; i < transaction->nr; i++) {
-   struct ref_update *update = transaction->updates[i];
-
-   if ((update->flags & REF_HAVE_NEW) &&
-   !is_null_oid(&update->new_oid))
-   add_packed_ref(refs->packed_ref_store, update->refname,
-  &update->new_oid);
-   }
-
-   if (commit_packed_refs(refs->packed_ref_store, err)) {
+   if (initial_ref_transaction_commit(packed_transaction, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
 
 cleanup:
+   if (packed_transaction)
+   ref_transaction_free(packed_transaction);
packed_refs_unlock(refs->packed_ref_store);
transaction->state = REF_TRANSACTION_CLOSED;
string_list_clear(&affected_refnames, 0);
-- 
2.14.1

[PATCH v2 11/11] files_transaction_finish(): delete reflogs before references

[Michael Haggerty]

If the deletion steps unexpectedly fail, it is less bad to leave a
reference without its reflog than it is to leave a reflog without its
reference, since the latter is an invalid repository state.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 35 +--
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 29eb5e826f..961424a4ea 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2636,6 +2636,27 @@ static int files_transaction_finish(struct ref_store 
*ref_store,
}
}
 
+   /*
+* Now that updates are safely completed, we can perform
+* deletes. First delete the reflogs of any references that
+* will be deleted, since (in the unexpected event of an
+* error) leaving a reference without a reflog is less bad
+* than leaving a reflog without a reference (the latter is a
+* mildly invalid repository state):
+*/
+   for (i = 0; i < transaction->nr; i++) {
+   struct ref_update *update = transaction->updates[i];
+   if (update->flags & REF_DELETING &&
+   !(update->flags & REF_LOG_ONLY) &&
+   !(update->flags & REF_ISPRUNING)) {
+   strbuf_reset(&sb);
+   files_reflog_path(refs, &sb, update->refname);
+   if (!unlink_or_warn(sb.buf))
+   try_remove_empty_parents(refs, update->refname,
+
REMOVE_EMPTY_PARENTS_REFLOG);
+   }
+   }
+
/*
 * Perform deletes now that updates are safely completed.
 *
@@ -2672,20 +2693,6 @@ static int files_transaction_finish(struct ref_store 
*ref_store,
}
}
 
-   /* Delete the reflogs of any references that were deleted: */
-   for (i = 0; i < transaction->nr; i++) {
-   struct ref_update *update = transaction->updates[i];
-   if (update->flags & REF_DELETING &&
-   !(update->flags & REF_LOG_ONLY) &&
-   !(update->flags & REF_ISPRUNING)) {
-   strbuf_reset(&sb);
-   files_reflog_path(refs, &sb, update->refname);
-   if (!unlink_or_warn(sb.buf))
-   try_remove_empty_parents(refs, update->refname,
-
REMOVE_EMPTY_PARENTS_REFLOG);
-   }
-   }
-
clear_loose_ref_cache(refs);
 
 cleanup:
-- 
2.14.1

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

[Michael Haggerty]

Currently, a loose reference is deleted even before locking the
`packed-refs` file, let alone deleting any packed version of the
reference. This leads to two problems, demonstrated by two new tests:

* While a reference is being deleted, other processes might see the
  old, packed value of the reference for a moment before the packed
  version is deleted. Normally this would be hard to observe, but we
  can prolong the window by locking the `packed-refs` file externally
  before running `update-ref`, then unlocking it before `update-ref`'s
  attempt to acquire the lock times out.

* If the `packed-refs` file is locked so long that `update-ref` fails
  to lock it, then the reference can be left permanently in the
  incorrect state described in the previous point.

In a moment, both problems will be fixed.

Signed-off-by: Michael Haggerty 
---
 t/t1404-update-ref-errors.sh | 73 
 1 file changed, 73 insertions(+)

diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index c34ece48f5..64a81345a8 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -404,4 +404,77 @@ test_expect_success 'broken reference blocks indirect 
create' '
test_cmp expected output.err
 '
 
+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=$! &&
+   # Give update-ref plenty of time to get to the point where it tries
+   # to lock packed-refs:
+   sleep 1 &&
+   # Make sure that update-ref did not complete despite the lock:
+   kill -0 $pid2 &&
+   # 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.
+   : ;;
+   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
+   ;;
+   $C)
+   # This value should never be seen. Probably the loose
+   # reference has been deleted but the packed reference
+   # is still there:
+   echo "$prefix/foo incorrectly observed to be C" &&
+   break
+   ;;
+   *)
+   # WTF?
+   echo "unexpected value observed for $prefix/foo: $sha1" &&
+   break
+   ;;
+   esac >out &&
+   rm -f .git/packed-refs.lock &&
+   wait $pid2 &&
+   test_must_be_empty out &&
+   test_must_fail git rev-parse --verify --quiet $prefix/foo
+'
+
+test_expect_failure 'delete fails cleanly if packed-refs file is locked' '
+   prefix=refs/locked-packed-refs &&
+   # 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 delete it while the `packed-refs` lock is held:
+   : >.git/packed-refs.lock &&
+   test_when_finished "rm -f .git/packed-refs.lock" &&
+   test_must_fail git update-ref -d $prefix/foo >out 2>err &&
+   git for-each-ref $prefix >actual &&
+   test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q: File exists" 
err &&
+   test_cmp unchanged actual
+'
+
 test_done
-- 
2.14.1

[PATCH v2 06/11] prune_refs(): also free the linked list

[Michael Haggerty]

At least since v1.7, the elements of the `refs_to_prune` linked list
have been leaked. Fix the leak by teaching `prune_refs()` to free the
list elements as it processes them.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3475c6f8a2..60031fe3ae 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1057,11 +1057,17 @@ static void prune_ref(struct files_ref_store *refs, 
struct ref_to_prune *r)
strbuf_release(&err);
 }
 
-static void prune_refs(struct files_ref_store *refs, struct ref_to_prune *r)
+/*
+ * Prune the loose versions of the references in the linked list
+ * `*refs_to_prune`, freeing the entries in the list as we go.
+ */
+static void prune_refs(struct files_ref_store *refs, struct ref_to_prune 
**refs_to_prune)
 {
-   while (r) {
+   while (*refs_to_prune) {
+   struct ref_to_prune *r = *refs_to_prune;
+   *refs_to_prune = r->next;
prune_ref(refs, r);
-   r = r->next;
+   free(r);
}
 }
 
@@ -1148,7 +1154,7 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
 
packed_refs_unlock(refs->packed_ref_store);
 
-   prune_refs(refs, refs_to_prune);
+   prune_refs(refs, &refs_to_prune);
strbuf_release(&err);
return 0;
 }
-- 
2.14.1

[PATCH v2 10/11] packed-backend: rip out some now-unused code

[Michael Haggerty]

Now the outside world interacts with the packed ref store only via the
generic refs API plus a few lock-related functions. This allows us to
delete some functions that are no longer used, thereby completing the
encapsulation of the packed ref store.

Signed-off-by: Michael Haggerty 
---
 refs/packed-backend.c | 193 --
 refs/packed-backend.h |   8 ---
 2 files changed, 201 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 9d5f76b1dc..0279aeceea 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -91,19 +91,6 @@ struct ref_store *packed_ref_store_create(const char *path,
return ref_store;
 }
 
-/*
- * Die if refs is not the main ref store. caller is used in any
- * necessary error messages.
- */
-static void packed_assert_main_repository(struct packed_ref_store *refs,
- const char *caller)
-{
-   if (refs->store_flags & REF_STORE_MAIN)
-   return;
-
-   die("BUG: operation %s only allowed for main ref store", caller);
-}
-
 /*
  * Downcast `ref_store` to `packed_ref_store`. Die if `ref_store` is
  * not a `packed_ref_store`. Also die if `packed_ref_store` doesn't
@@ -321,40 +308,6 @@ static struct ref_dir *get_packed_refs(struct 
packed_ref_store *refs)
return get_packed_ref_dir(get_packed_ref_cache(refs));
 }
 
-/*
- * Add or overwrite a reference in the in-memory packed reference
- * cache. This may only be called while the packed-refs file is locked
- * (see packed_refs_lock()). To actually write the packed-refs file,
- * call commit_packed_refs().
- */
-void add_packed_ref(struct ref_store *ref_store,
-   const char *refname, const struct object_id *oid)
-{
-   struct packed_ref_store *refs =
-   packed_downcast(ref_store, REF_STORE_WRITE,
-   "add_packed_ref");
-   struct ref_dir *packed_refs;
-   struct ref_entry *packed_entry;
-
-   if (!is_lock_file_locked(&refs->lock))
-   die("BUG: packed refs not locked");
-
-   if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
-   die("Reference has invalid format: '%s'", refname);
-
-   packed_refs = get_packed_refs(refs);
-   packed_entry = find_ref_entry(packed_refs, refname);
-   if (packed_entry) {
-   /* Overwrite the existing entry: */
-   oidcpy(&packed_entry->u.value.oid, oid);
-   packed_entry->flag = REF_ISPACKED;
-   oidclr(&packed_entry->u.value.peeled);
-   } else {
-   packed_entry = create_ref_entry(refname, oid, REF_ISPACKED);
-   add_ref_entry(packed_refs, packed_entry);
-   }
-}
-
 /*
  * Return the ref_entry for the given refname from the packed
  * references.  If it does not exist, return NULL.
@@ -596,152 +549,6 @@ int packed_refs_is_locked(struct ref_store *ref_store)
 static const char PACKED_REFS_HEADER[] =
"# pack-refs with: peeled fully-peeled \n";
 
-/*
- * Write the current version of the packed refs cache from memory to
- * disk. The packed-refs file must already be locked for writing (see
- * packed_refs_lock()). Return zero on success. On errors, rollback
- * the lockfile, write an error message to `err`, and return a nonzero
- * value.
- */
-int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
-{
-   struct packed_ref_store *refs =
-   packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
-   "commit_packed_refs");
-   struct packed_ref_cache *packed_ref_cache =
-   get_packed_ref_cache(refs);
-   int ok;
-   int ret = -1;
-   struct strbuf sb = STRBUF_INIT;
-   FILE *out;
-   struct ref_iterator *iter;
-   char *packed_refs_path;
-
-   if (!is_lock_file_locked(&refs->lock))
-   die("BUG: commit_packed_refs() called when unlocked");
-
-   /*
-* If packed-refs is a symlink, we want to overwrite the
-* symlinked-to file, not the symlink itself. Also, put the
-* staging file next to it:
-*/
-   packed_refs_path = get_locked_file_path(&refs->lock);
-   strbuf_addf(&sb, "%s.new", packed_refs_path);
-   if (create_tempfile(&refs->tempfile, sb.buf) < 0) {
-   strbuf_addf(err, "unable to create file %s: %s",
-   sb.buf, strerror(errno));
-   strbuf_release(&sb);
-   goto out;
-   }
-   strbuf_release(&sb);
-
-   out = fdopen_tempfile(&refs->tempfile, "w");
-   if (!out) {
-   strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s",
-   strerror(errno));
-   goto error;
-   }
-
-   if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0) {
-   strbuf_addf(err, "error writing to %s: %s",
-   get_tempfile_path(&refs->tempfile), 
strerror(er

[PATCH v2 09/11] files_ref_store: use a transaction to update packed refs

[Michael Haggerty]

When processing a `files_ref_store` transaction, it is sometimes
necessary to delete some references from the "packed-refs" file. Do
that using a reference transaction conducted against the
`packed_ref_store`.

This change further decouples `files_ref_store` from
`packed_ref_store`. It also fixes multiple problems, including the two
revealed by test cases added in the previous commit.

First, the old code didn't obtain the `packed-refs` lock until
`files_transaction_finish()`. This means that a failure to acquire the
`packed-refs` lock (e.g., due to contention with another process)
wasn't detected until it was too late (problems like this are supposed
to be detected in the "prepare" phase). The new code acquires the
`packed-refs` lock in `files_transaction_prepare()`, the same stage of
the processing when the loose reference locks are being acquired,
removing another reason why the "prepare" phase might succeed and the
"finish" phase might nevertheless fail.

Second, the old code deleted the loose version of a reference before
deleting any packed version of the same reference. This left a moment
when another process might think that the packed version of the
reference is current, which is incorrect. (Even worse, the packed
version of the reference can be arbitrarily old, and might even point
at an object that has since been garbage-collected.)

Third, if a reference deletion fails to acquire the `packed-refs` lock
altogether, then the old code might leave the repository in the
incorrect state (possibly corrupt) described in the previous
paragraph.

Now we activate the new "packed-refs" file (sans any references that
are being deleted) *before* deleting the corresponding loose
references. But we hold the "packed-refs" lock until after the loose
references have been finalized, thus preventing a simultaneous
"pack-refs" process from packing the loose version of the reference in
the time gap, which would otherwise defeat our attempt to delete it.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 132 +--
 t/t1404-update-ref-errors.sh |   4 +-
 2 files changed, 103 insertions(+), 33 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2700e3b5d5..29eb5e826f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2397,13 +2397,22 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
return 0;
 }
 
+struct files_transaction_backend_data {
+   struct ref_transaction *packed_transaction;
+   int packed_refs_locked;
+};
+
 /*
  * Unlock any references in `transaction` that are still locked, and
  * mark the transaction closed.
  */
-static void files_transaction_cleanup(struct ref_transaction *transaction)
+static void files_transaction_cleanup(struct files_ref_store *refs,
+ struct ref_transaction *transaction)
 {
size_t i;
+   struct files_transaction_backend_data *backend_data =
+   transaction->backend_data;
+   struct strbuf err = STRBUF_INIT;
 
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
@@ -2415,6 +2424,17 @@ static void files_transaction_cleanup(struct 
ref_transaction *transaction)
}
}
 
+   if (backend_data->packed_transaction &&
+   ref_transaction_abort(backend_data->packed_transaction, &err)) {
+   error("error aborting transaction: %s", err.buf);
+   strbuf_release(&err);
+   }
+
+   if (backend_data->packed_refs_locked)
+   packed_refs_unlock(refs->packed_ref_store);
+
+   free(backend_data);
+
transaction->state = REF_TRANSACTION_CLOSED;
 }
 
@@ -2431,12 +2451,17 @@ static int files_transaction_prepare(struct ref_store 
*ref_store,
char *head_ref = NULL;
int head_type;
struct object_id head_oid;
+   struct files_transaction_backend_data *backend_data;
+   struct ref_transaction *packed_transaction = NULL;
 
assert(err);
 
if (!transaction->nr)
goto cleanup;
 
+   backend_data = xcalloc(1, sizeof(*backend_data));
+   transaction->backend_data = backend_data;
+
/*
 * Fail if a refname appears more than once in the
 * transaction. (If we end up splitting up any updates using
@@ -2503,6 +2528,41 @@ static int files_transaction_prepare(struct ref_store 
*ref_store,
  head_ref, &affected_refnames, err);
if (ret)
break;
+
+   if (update->flags & REF_DELETING &&
+   !(update->flags & REF_LOG_ONLY) &&
+   !(update->flags & REF_ISPRUNING)) {
+   /*
+* This reference has to be deleted from
+* packed-refs if it exists there.
+*/
+   if (!packed_

[PATCH v2 04/11] packed_delete_refs(): implement method

[Michael Haggerty]

Implement `packed_delete_refs()` using a reference transaction. This
means that `files_delete_refs()` can use `refs_delete_refs()` instead
of `repack_without_refs()` to delete any packed references, decreasing
the coupling between the classes.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c  |  2 +-
 refs/packed-backend.c | 45 -
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index fccbc24ac4..2c78f63494 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1157,7 +1157,7 @@ static int files_delete_refs(struct ref_store *ref_store, 
const char *msg,
if (packed_refs_lock(refs->packed_ref_store, 0, &err))
goto error;
 
-   if (repack_without_refs(refs->packed_ref_store, refnames, &err)) {
+   if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {
packed_refs_unlock(refs->packed_ref_store);
goto error;
}
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 9ab65c5a0a..9d5f76b1dc 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1086,7 +1086,50 @@ static int packed_initial_transaction_commit(struct 
ref_store *ref_store,
 static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
 struct string_list *refnames, unsigned int flags)
 {
-   die("BUG: not implemented yet");
+   struct packed_ref_store *refs =
+   packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
+   struct strbuf err = STRBUF_INIT;
+   struct ref_transaction *transaction;
+   struct string_list_item *item;
+   int ret;
+
+   (void)refs; /* We need the check above, but don't use the variable */
+
+   if (!refnames->nr)
+   return 0;
+
+   /*
+* Since we don't check the references' old_oids, the
+* individual updates can't fail, so we can pack all of the
+* updates into a single transaction.
+*/
+
+   transaction = ref_store_transaction_begin(ref_store, &err);
+   if (!transaction)
+   return -1;
+
+   for_each_string_list_item(item, refnames) {
+   if (ref_transaction_delete(transaction, item->string, NULL,
+  flags, msg, &err)) {
+   warning(_("could not delete reference %s: %s"),
+   item->string, err.buf);
+   strbuf_reset(&err);
+   }
+   }
+
+   ret = ref_transaction_commit(transaction, &err);
+
+   if (ret) {
+   if (refnames->nr == 1)
+   error(_("could not delete reference %s: %s"),
+ refnames->items[0].string, err.buf);
+   else
+   error(_("could not delete references: %s"), err.buf);
+   }
+
+   ref_transaction_free(transaction);
+   strbuf_release(&err);
+   return ret;
 }
 
 static int packed_pack_refs(struct ref_store *ref_store, unsigned int flags)
-- 
2.14.1

[PATCH v2 02/11] struct ref_transaction: add a place for backends to store data

[Michael Haggerty]

`packed_ref_store` is going to want to store some transaction-wide
data, so make a place for it.

Signed-off-by: Michael Haggerty 
---
 refs/refs-internal.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index b02dc5a7e3..d7d344de73 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -242,6 +242,7 @@ struct ref_transaction {
size_t alloc;
size_t nr;
enum ref_transaction_state state;
+   void *backend_data;
 };
 
 /*
-- 
2.14.1

[PATCH v2 03/11] packed_ref_store: implement reference transactions

[Michael Haggerty]

Implement the methods needed to support reference transactions for
the packed-refs backend. The new methods are not yet used.

Signed-off-by: Michael Haggerty 
---
 refs/packed-backend.c | 313 +-
 refs/packed-backend.h |   9 ++
 2 files changed, 319 insertions(+), 3 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index b76f14e5b3..9ab65c5a0a 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -748,25 +748,332 @@ static int packed_init_db(struct ref_store *ref_store, 
struct strbuf *err)
return 0;
 }
 
+/*
+ * Write the packed-refs from the cache to the packed-refs tempfile,
+ * incorporating any changes from `updates`. `updates` must be a
+ * sorted string list whose keys are the refnames and whose util
+ * values are `struct ref_update *`. On error, rollback the tempfile,
+ * write an error message to `err`, and return a nonzero value.
+ *
+ * The packfile must be locked before calling this function and will
+ * remain locked when it is done.
+ */
+static int write_with_updates(struct packed_ref_store *refs,
+ struct string_list *updates,
+ struct strbuf *err)
+{
+   struct ref_iterator *iter = NULL;
+   size_t i;
+   int ok;
+   FILE *out;
+   struct strbuf sb = STRBUF_INIT;
+   char *packed_refs_path;
+
+   if (!is_lock_file_locked(&refs->lock))
+   die("BUG: write_with_updates() called while unlocked");
+
+   /*
+* If packed-refs is a symlink, we want to overwrite the
+* symlinked-to file, not the symlink itself. Also, put the
+* staging file next to it:
+*/
+   packed_refs_path = get_locked_file_path(&refs->lock);
+   strbuf_addf(&sb, "%s.new", packed_refs_path);
+   free(packed_refs_path);
+   if (create_tempfile(&refs->tempfile, sb.buf) < 0) {
+   strbuf_addf(err, "unable to create file %s: %s",
+   sb.buf, strerror(errno));
+   strbuf_release(&sb);
+   return -1;
+   }
+   strbuf_release(&sb);
+
+   out = fdopen_tempfile(&refs->tempfile, "w");
+   if (!out) {
+   strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s",
+   strerror(errno));
+   goto error;
+   }
+
+   if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0)
+   goto write_error;
+
+   /*
+* We iterate in parallel through the current list of refs and
+* the list of updates, processing an entry from at least one
+* of the lists each time through the loop. When the current
+* list of refs is exhausted, set iter to NULL. When the list
+* of updates is exhausted, leave i set to updates->nr.
+*/
+   iter = packed_ref_iterator_begin(&refs->base, "",
+DO_FOR_EACH_INCLUDE_BROKEN);
+   if ((ok = ref_iterator_advance(iter)) != ITER_OK)
+   iter = NULL;
+
+   i = 0;
+
+   while (iter || i < updates->nr) {
+   struct ref_update *update = NULL;
+   int cmp;
+
+   if (i >= updates->nr) {
+   cmp = -1;
+   } else {
+   update = updates->items[i].util;
+
+   if (!iter)
+   cmp = +1;
+   else
+   cmp = strcmp(iter->refname, update->refname);
+   }
+
+   if (!cmp) {
+   /*
+* There is both an old value and an update
+* for this reference. Check the old value if
+* necessary:
+*/
+   if ((update->flags & REF_HAVE_OLD)) {
+   if (is_null_oid(&update->old_oid)) {
+   strbuf_addf(err, "cannot update ref 
'%s': "
+   "reference already exists",
+   update->refname);
+   goto error;
+   } else if (oidcmp(&update->old_oid, iter->oid)) 
{
+   strbuf_addf(err, "cannot update ref 
'%s': "
+   "is at %s but expected %s",
+   update->refname,
+   oid_to_hex(iter->oid),
+   
oid_to_hex(&update->old_oid));
+   goto error;
+   }
+   }
+
+   /* Now figure out what to use for the new value: */
+   if ((update->flags & REF_HAVE_NEW)) {
+   /*
+ 

[PATCH v2 05/11] files_pack_refs(): use a reference transaction to write packed refs

[Michael Haggerty]

Now that the packed reference store supports transactions, we can use
a transaction to write the packed versions of references that we want
to pack. This decreases the coupling between `files_ref_store` and
`packed_ref_store`.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 24 +---
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2c78f63494..3475c6f8a2 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1100,6 +1100,11 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
int ok;
struct ref_to_prune *refs_to_prune = NULL;
struct strbuf err = STRBUF_INIT;
+   struct ref_transaction *transaction;
+
+   transaction = ref_store_transaction_begin(refs->packed_ref_store, &err);
+   if (!transaction)
+   return -1;
 
packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
 
@@ -1115,12 +1120,14 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
continue;
 
/*
-* Create an entry in the packed-refs cache equivalent
-* to the one from the loose ref cache, except that
-* we don't copy the peeled status, because we want it
-* to be re-peeled.
+* Add a reference creation for this reference to the
+* packed-refs transaction:
 */
-   add_packed_ref(refs->packed_ref_store, iter->refname, 
iter->oid);
+   if (ref_transaction_update(transaction, iter->refname,
+  iter->oid->hash, NULL,
+  REF_NODEREF, NULL, &err))
+   die("failure preparing to create packed reference %s: 
%s",
+   iter->refname, err.buf);
 
/* Schedule the loose reference for pruning if requested. */
if ((flags & PACK_REFS_PRUNE)) {
@@ -1134,8 +1141,11 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
if (ok != ITER_DONE)
die("error while iterating over references");
 
-   if (commit_packed_refs(refs->packed_ref_store, &err))
-   die("unable to overwrite old ref-pack file: %s", err.buf);
+   if (ref_transaction_commit(transaction, &err))
+   die("unable to write new packed-refs: %s", err.buf);
+
+   ref_transaction_free(transaction);
+
packed_refs_unlock(refs->packed_ref_store);
 
prune_refs(refs, refs_to_prune);
-- 
2.14.1

[PATCH v2 01/11] packed-backend: don't adjust the reference count on lock/unlock

[Michael Haggerty]

The old code incremented the packed ref cache reference count when
acquiring the packed-refs lock, and decremented the count when
releasing the lock. This is unnecessary because:

* Another process cannot change the packed-refs file because it is
  locked.

* When we ourselves change the packed-refs file, we do so by first
  modifying the packed ref-cache, and then writing the data from the
  ref-cache to disk. So the packed ref-cache remains fresh because any
  changes that we plan to make to the file are made in the cache first
  anyway.

So there is no reason for the cache to become stale.

Moreover, the extra reference count causes a problem if we
intentionally clear the packed refs cache, as we sometimes need to do
if we change the cache in anticipation of writing a change to disk,
but then the write to disk fails. In that case, `packed_refs_unlock()`
would have no easy way to find the cache whose reference count it
needs to decrement.

This whole issue will soon become moot due to upcoming changes that
avoid changing the in-memory cache as part of updating the packed-refs
on disk, but this change makes that transition easier.

Signed-off-by: Michael Haggerty 
---
 refs/packed-backend.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 412c85034f..b76f14e5b3 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -525,7 +525,6 @@ int packed_refs_lock(struct ref_store *ref_store, int 
flags, struct strbuf *err)
"packed_refs_lock");
static int timeout_configured = 0;
static int timeout_value = 1000;
-   struct packed_ref_cache *packed_ref_cache;
 
if (!timeout_configured) {
git_config_get_int("core.packedrefstimeout", &timeout_value);
@@ -560,9 +559,11 @@ int packed_refs_lock(struct ref_store *ref_store, int 
flags, struct strbuf *err)
 */
validate_packed_ref_cache(refs);
 
-   packed_ref_cache = get_packed_ref_cache(refs);
-   /* Increment the reference count to prevent it from being freed: */
-   acquire_packed_ref_cache(packed_ref_cache);
+   /*
+* Now make sure that the packed-refs file as it exists in the
+* locked state is loaded into the cache:
+*/
+   get_packed_ref_cache(refs);
return 0;
 }
 
@@ -576,7 +577,6 @@ void packed_refs_unlock(struct ref_store *ref_store)
if (!is_lock_file_locked(&refs->lock))
die("BUG: packed_refs_unlock() called when not locked");
rollback_lock_file(&refs->lock);
-   release_packed_ref_cache(refs->cache);
 }
 
 int packed_refs_is_locked(struct ref_store *ref_store)
-- 
2.14.1

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

[Michael Haggerty]

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.

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

These changes are also available as branch `packed-ref-transactions`
from my GitHub repo [2].

Michael

[1] https://public-inbox.org/git/cover.1503993268.git.mhag...@alum.mit.edu/
[2] https://github.com/mhagger/git

Michael Haggerty (11):
  packed-backend: don't adjust the reference count on lock/unlock
  struct ref_transaction: add a place for backends to store data
  packed_ref_store: implement reference transactions
  packed_delete_refs(): implement method
  files_pack_refs(): use a reference transaction to write packed refs
  prune_refs(): also free the linked list
  files_initial_transaction_commit(): use a transaction for packed refs
  t1404: demonstrate two problems with reference transactions
  files_ref_store: use a transaction to update packed refs
  packed-backend: rip out some now-unused code
  files_transaction_finish(): delete reflogs before references

 refs/files-backend.c | 214 ++--
 refs/packed-backend.c| 461 +--
 refs/packed-backend.h|  17 +-
 refs/refs-internal.h |   1 +
 t/t1404-update-ref-errors.sh |  73 +++
 5 files changed, 550 insertions(+), 216 deletions(-)

-- 
2.14.1

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 08.09.2017 03:26:
> Olaf Klischat  writes:
> 
>> `git diff --help' says:
>>
>> git diff [--options]  [--] [...]
>>This form is to view the changes you have in your
>>working tree relative to the named .
> 
> That help text is poorly phrased.  
> 
> When "git diff" talks about files in your working tree, it always
> looks them _through_ the index.  As far as the command is concerned,
> a cruft left in your working tree that is not in the index does
> *not* exist.
> 
> So when your index does not have bar.txt, even if you have an
> untracked bar.txt in your directory, i.e.
> 
>> oklischat@oklischat:/tmp/gittest$ git status
>> On branch master
>> Untracked files:
>>   (use "git add ..." to include in what will be committed)
>>
>>  bar.txt
> 
> and you have a commit that _has_ that file, then the command thinks
>  has the path, and your working tree does *not*.  IOW, this
> is...
> 
>> oklischat@oklischat:/tmp/gittest$ git diff bar-added
>> diff --git a/bar.txt b/bar.txt
>> deleted file mode 100644
> 
> ... totally expected and intended output.
> 
> Hope the above explanation clarifies.  A documentation update might
> be helpful to new users.

Well, there's a difference between "working tree" and  "working dir".
The wt is "the tree of actual checked out files" per our glossary. So
maybe the doc could point to the glossary (see the glossary for the
difference to the work dir).

But really, this type of misunderstandings comes up often: people try to
understand the doc based on common language terms (which is okay, of
course) and are unaware of the defined meanings of technical terms.
Explaining them in every place where they are used simply does not scale.

Maybe we should make more use of our glossary (extend it, enhance it,
promote it) and somehow mark all technical terms as such when they are
used (say, italics in HTML), or at least when the exact meaning is
relevant like in the case above, and possibly link to the glossary (-item).

Michael

Re: [PATCH 08/10] files_ref_store: use a transaction to update packed refs

[Jeff King]

On Fri, Sep 08, 2017 at 02:44:57PM +0200, Michael Haggerty wrote:

> > That means we're holding the packed-refs lock for a slightly longer
> > period. I think this could mean worse lock contention between otherwise
> > unrelated transactions over the packed-refs file. I wonder if the
> > lock-retry timeout might need to be increased to accommodate this. On
> > the other hand, it looks like we take it after getting the individual
> > locks, which I'd think would be the expensive part.
> 
> That was my thinking, yes. While the packed-refs lock is held, the
> references being created/updated have their reflogs written and are
> renamed into place. I don't see how that can be shortened without
> compromising on correctness (in particular, that we want to process
> creates/updates before deletions to try to preserve reachability as much
> as possible during the transaction). As an added optimization, the
> packed-refs lock is not acquired at all if no references are being deleted.

Makes sense.  I guess in theory a process could do significant unrelated
work between the "prepare" and "finish" steps, while holding the lock.
But I don't know why it would, and arguably that itself is a bug.

> The packed-refs file and loose references are never locked at the same
> time during pack-refs, so no deadlock is possible.
> 
> But you are right to assume that it *should* be so. The algorithm
> written above is a tiny bit unsafe (and has been for years). It is
> possible, though admittedly very unlikely, for the following to happen
> in the gap between steps 5 and 6:

Thanks for explaining (and I think that your "should" is why I thought
it was so; we've discussed this race before).

> This would leave "foo" at the obsolete value "B" (i.e., the value
> written to the `packed-refs` file for it by the nested `pack-refs` process.
> 
> I think that fixing this problem would require the `packed-refs` lock to
> be held while `pack-refs` is pruning the loose references. But given how
> unlikely that chain of events seems, and that fixing it would increase
> contention on the `packed-refs` file and allow the deadlock that you
> described, I lean towards leaving it as-is. Though admittedly,
> contention over a loose reference lock could make the race more likely
> to be hit.

Agreed. It's a pretty unlikely sequence of events. And IMHO the real
solution is a new storage format that's easier to reason about.

-Peff

Re: [PATCH 08/10] files_ref_store: use a transaction to update packed refs

[Michael Haggerty]

On 09/08/2017 09:38 AM, Jeff King wrote:
> On Tue, Aug 29, 2017 at 10:20:32AM +0200, Michael Haggerty wrote:
> 
>> First, the old code didn't obtain the `packed-refs` lock until
>> `files_transaction_finish()`. This means that a failure to acquire the
>> `packed-refs` lock (e.g., due to contention with another process)
>> wasn't detected until it was too late (problems like this are supposed
>> to be detected in the "prepare" phase). The new code acquires the
>> `packed-refs` lock in `files_transaction_prepare()`, the same stage of
>> the processing when the loose reference locks are being acquired,
>> removing another reason why the "prepare" phase might succeed and the
>> "finish" phase might nevertheless fail.
> 
> That means we're holding the packed-refs lock for a slightly longer
> period. I think this could mean worse lock contention between otherwise
> unrelated transactions over the packed-refs file. I wonder if the
> lock-retry timeout might need to be increased to accommodate this. On
> the other hand, it looks like we take it after getting the individual
> locks, which I'd think would be the expensive part.

That was my thinking, yes. While the packed-refs lock is held, the
references being created/updated have their reflogs written and are
renamed into place. I don't see how that can be shortened without
compromising on correctness (in particular, that we want to process
creates/updates before deletions to try to preserve reachability as much
as possible during the transaction). As an added optimization, the
packed-refs lock is not acquired at all if no references are being deleted.

> Are there other callers who take the packed-refs and individual locks in
> the reverse order? I think git-pack-refs might. Could we "deadlock" with
> pack-refs? There are timeouts so it would resolve itself quickly, but I
> wonder if we'd have a case that would deadlock-until-timeout that
> otherwise could succeed.

That's not true. `files_pack_refs()` does the following:

1. Lock the `packed-refs` file.

2. Start a packed ref-store transaction.

3. Iterate over the loose ref cache. For each reference that should
   be packed:
   * add it to the packed-refs transaction as an update to set it
 to the loose value (without specifying an old_sha1)
   * if pruning is on, schedule the loose reference to be pruned
 (in an internal data structure, without locking the file).

4. Commit the packed ref-store transaction.

5. Release the packed-refs lock.

6. For each to-prune loose ref:
   * lock the loose reference
   * verify that it still has the value that was just written to
 the `packed-refs` file
   * if so, delete the loose version of the reference
   * unlock the loose reference

The packed-refs file and loose references are never locked at the same
time during pack-refs, so no deadlock is possible.

But you are right to assume that it *should* be so. The algorithm
written above is a tiny bit unsafe (and has been for years). It is
possible, though admittedly very unlikely, for the following to happen
in the gap between steps 5 and 6:

1. One process updates the value of branch "foo" from A to B.
   (Remember that A is the value that was just written to the
   `packed-refs` file by step 4.)

2. `pack-refs` is run *again* (while the first `pack-refs` is out
   on its lunch break), writes value B to the `packed-refs` file
   for "foo", and deletes the loose version of "foo".

3. Yet *another* process changes "foo" back from B to A. This
   creates a loose version of "foo" with value A, which overwrites
   the packed version with value B.

4. The first `pack-refs` process resumes at step 6, sees that
   "foo" "still" has the value "A", so deletes the loose reference.

This would leave "foo" at the obsolete value "B" (i.e., the value
written to the `packed-refs` file for it by the nested `pack-refs` process.

I think that fixing this problem would require the `packed-refs` lock to
be held while `pack-refs` is pruning the loose references. But given how
unlikely that chain of events seems, and that fixing it would increase
contention on the `packed-refs` file and allow the deadlock that you
described, I lean towards leaving it as-is. Though admittedly,
contention over a loose reference lock could make the race more likely
to be hit.

I also just noticed that the `refs_to_prune` linked list is leaked here
(as has also been the case for many years). I'll fix that separately.

>> Second, the old code deleted the loose version of a reference before
>> deleting any packed version of the same reference. This left a moment
>> when another process might think that the packed version of the
>> reference is current, which is incorrect. (Even worse, the packed
>> version of the reference can be arbitrarily old, and might even point
>> at an object that has since been garbage-collected.)
>>
>> Third, if a reference deletion fails to acquire the `packed-refs` lock
>> altogether, then the old code might leave the repositor

Re: [PATCH 0/4] Test name-rev with small stack

[Michael J Gruber]

Jeff King venit, vidit, dixit 07.09.2017 16:54:
> On Thu, Sep 07, 2017 at 04:02:19PM +0200, Michael J Gruber wrote:
> 
>> name-rev segfaults for me in emacs.git with the typical 8102 stack size.
>> The reason is the recursive walk that name-rev uses.
>>
>> This series adds a test to mark this as known failure, after some
>> clean-ups.
> 
> These all look reasonable to me. The size of the test case in the final
> one is presumably arbitrary and just copied from t7004. I don't know if
> it's worth trying to shrink it. It could shorten a rather expensive
> test. OTOH, if we shorten it too much then we might get a false pass
> (e.g., if the algorithm remains recursive but has a smaller stack
> footprint).
> 
>> Michael J Gruber (4):
>>   t7004: move limited stack prereq to test-lib
>>   t6120: test name-rev --all and --stdin
>>   t6120: clean up state after breaking repo
>>   t6120: test describe and name-rev with deep repos
> 
> Now comes the hard part: rewriting the C code. :)

Looking at it more closely, the solution in cbc60b6720 ("git tag
--contains: avoid stack overflow", 2014-04-24) seems to be a bit "ad
hoc" to me:

First of all, there is more than "tag --contains" that may exceed the
stack by recursion. So I would expect the solution to be more general,
and not localised and specialised to builtin/tag.c

Second, this is a walk, so I'm wondering whether our revision walk
machinery should be the place to add missing functionality (if any).
That way, everything would benefit from possible or existing
improvements there. For example, I think some of our "extra walkers"
don't heed object replacements. (I need to test more.)

Michael

[no subject]

[Hernandez, Brooke [UCH]]

I have a charity proposal worth 100million. Alice Walton. CONTACT ( 
cont...@alicewonders.us ) FOR YOUR FUNDS.

Re: [PATCH 07/10] t1404: demonstrate two problems with reference transactions

[Michael Haggerty]

On 09/08/2017 06:44 AM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> +git for-each-ref $prefix >actual &&
>> +grep "Unable to create $Q.*packed-refs.lock$Q: File exists" err &&
> 
> I added a squash for doing s/grep/test_i18n&/ here

Thanks for the fix. I always forget that gotcha.

> are there other
> issues in the series, or is the series more or less ready to be
> polished in 'next'?

I'm working on v2 right now.

Michael

Re: [PATCH 06/10] files_initial_transaction_commit(): use a transaction for packed refs

[Michael Haggerty]

On 09/08/2017 09:27 AM, Jeff King wrote:
> On Tue, Aug 29, 2017 at 10:20:30AM +0200, Michael Haggerty wrote:
> 
>> Used a `packed_ref_store` transaction in the implementation of
>> `files_initial_transaction_commit()` rather than using internal
>> features of the packed ref store. This further decouples
>> `files_ref_store` from `packed_ref_store`.
> 
> Very nice to see these couplings going away.
> 
> Minor nit: s/Used/Use/ in the commit message.

Thanks; will fix.

Michael

Re: [PATCH 01/10] packed-backend: don't adjust the reference count on lock/unlock

[Michael Haggerty]

On 09/08/2017 08:52 AM, Jeff King wrote:
> On Tue, Aug 29, 2017 at 10:20:25AM +0200, Michael Haggerty wrote:
> 
>> The old code incremented the packed ref cache reference count when
>> acquiring the packed-refs lock, and decremented the count when
>> releasing the lock. This is unnecessary because a locked packed-refs
>> file cannot be changed, so there is no reason for the cache to become
>> stale.
> 
> Hmm, I thought that after your last series, we might hold the lock but
> update the packed-refs from a separate tempfile. I.e., 42dfa7ecef
> (commit_packed_refs(): use a staging file separate from the lockfile,
> 2017-06-23).
> 
>> Moreover, the extra reference count causes a problem if we
>> intentionally clear the packed refs cache, as we sometimes need to do
>> if we change the cache in anticipation of writing a change to disk,
>> but then the write to disk fails. In that case, `packed_refs_unlock()`
>> would have no easy way to find the cache whose reference count it
>> needs to decrement.
>>
>> This whole issue will soon become moot due to upcoming changes that
>> avoid changing the in-memory cache as part of updating the packed-refs
>> on disk, but this change makes that transition easier.
> 
> All of this makes sense, and I'm happy this complexity is going away in
> the long run. But I guess what I'm wondering is in the meantime if we
> can have a sequence like:
> 
>   1. We hold packed-refs.lock
> 
>   2. We update the file without releasing the lock, via 42dfa7ecef.
> 
>   3. Still holding the lock, we try to look at packed-refs. The
>  stat_validity code says no, we're not up to date.
> 
>   4. We discard the old packed_ref_cache and reload it. Because its
>  reference count was not incremented during step 1, we actually
>  free() it.
> 
>   5. We try to look at at the old freed pointer.
> 
> There are several steps in there that might be implausible. So I'm
> mostly playing devil's advocate here.
> 
> I'm wondering if the "don't validate while we hold the lock" logic in
> get_packed_refs_cache() means that step 3 is impossible.

That's one of the reasons your scenario can't happen, but that just begs
the question of whether the "don't validate while we hold the lock" code
is wrongheaded.

In fact it's OK. The method by which the packed-refs file on disk is
modified at this point in the patch series is by modifying the packed
ref-cache and then writing the data from the ref-cache to disk. So the
packed ref-cache remains fresh because any changes that we plan to make
to the file are made in the cache first anyway.

I'll explain that a bit better in the log message.

The next question is whether this change interacts badly with changes
later in the patch series, when we cease to modify the ref-cache before
writing the new packed-refs file. Here we're OK, too, because
immediately after `rename_tempfile()` is used to rename the new
packed-refs file into place, we clear the packed ref-cache, so no
subsequent callers of `get_packed_refs_cache()` should see the stale cache.

Which in turn is partly because your step 5 is also implausible: code
shouldn't be holding a pointer to the packed ref-cache across operations
that might change the file. (Code that calls `get_packed_refs_cache()`
is OK because that function would see that `refs->cache` is NULL and
reload it regardless of whether we hold the lock.)

So I think everything is OK, but thanks for making me think through and
explain it better :-)

>> @@ -560,9 +559,7 @@ int packed_refs_lock(struct ref_store *ref_store, int 
>> flags, struct strbuf *err)
>>   */
>>  validate_packed_ref_cache(refs);
>>  
>> -packed_ref_cache = get_packed_ref_cache(refs);
>> -/* Increment the reference count to prevent it from being freed: */
>> -acquire_packed_ref_cache(packed_ref_cache);
>> +get_packed_ref_cache(refs);
> 
> It seems a bit funny to call a "get" function and throw away the return
> value. Presumably we care about its side effect of updating refs->cache.
> That might be worth a comment (though if this is all going away soon, I
> care a lot less about niceties like that).

I'm rerolling anyway, so I'll add a comment. Thanks.

Michael

[PATCH] shortlog: skip format/parse roundtrip for internal traversal

[Jeff King]

On Fri, Sep 08, 2017 at 03:39:36PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > IOW, something like the patch below, which pushes the re-parsing out to
> > the stdin code-path, and lets the internal traversal format directly
> > into the final buffer. It seems to be about 3% faster than the existing
> > code, and fixes the leak (by dropping that variable entirely).
> 
> Wow, that is s logical a conclusion that I somewhat feel ashamed
> that I didn't think of it myself.
> 
> Nicely done.

Thanks. Here it is with a commit message.

Note that the non-stdin path no longer looks at the "mailmap" entry of
"struct shortlog" (instead we use the one cached inside pretty.c). But
we still waste time loading it. I'm not sure if it's worth addressing
that. It's only once per program invocation, and it's a little tricky to
fix (we do shortlog_init() before we know whether or not we're using
stdin). We could just load it lazily, though, which would cover the
stdin case.

-- >8 --
Subject: shortlog: skip format/parse roundtrip for internal traversal

The original git-shortlog command parsed the output of
git-log, and the logic went something like this:

  1. Read stdin looking for "author" lines.

  2. Parse the identity into its name/email bits.

  3. Apply mailmap to the name/email.

  4. Reformat the identity into a single buffer that is our
 "key" for grouping entries (either a name by default,
 or "name " if --email was given).

The first part happens in read_from_stdin(), and the other
three steps are part of insert_one_record().

When we do an internal traversal, we just swap out the stdin
read in step 1 for reading the commit objects ourselves.
Prior to 2db6b83d18 (shortlog: replace hand-parsing of
author with pretty-printer, 2016-01-18), that made sense; we
still had to parse the ident in the commit message.

But after that commit, we use pretty.c's "%an <%ae>" to get
the author ident (for simplicity). Which means that the
pretty printer is doing a parse/format under the hood, and
then we parse the result, apply the mailmap, and format the
result again.

Instead, we can just ask pretty.c to do all of those steps
for us (including the mailmap via "%aN <%aE>", and not
formatting the address when --email is missing).

And then we can push steps 2-4 into read_from_stdin(). This
speeds up "git shortlog -ns" on linux.git by about 3%, and
eliminates a leak in insert_one_record() of the namemailbuf
strbuf.

Signed-off-by: Jeff King 
---
 builtin/shortlog.c | 56 ++
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 43c4799ea9..e29875b843 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -52,26 +52,8 @@ static void insert_one_record(struct shortlog *log,
  const char *oneline)
 {
struct string_list_item *item;
-   const char *mailbuf, *namebuf;
-   size_t namelen, maillen;
-   struct strbuf namemailbuf = STRBUF_INIT;
-   struct ident_split ident;
 
-   if (split_ident_line(&ident, author, strlen(author)))
-   return;
-
-   namebuf = ident.name_begin;
-   mailbuf = ident.mail_begin;
-   namelen = ident.name_end - ident.name_begin;
-   maillen = ident.mail_end - ident.mail_begin;
-
-   map_user(&log->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
-   strbuf_add(&namemailbuf, namebuf, namelen);
-
-   if (log->email)
-   strbuf_addf(&namemailbuf, " <%.*s>", (int)maillen, mailbuf);
-
-   item = string_list_insert(&log->list, namemailbuf.buf);
+   item = string_list_insert(&log->list, author);
 
if (log->summary)
item->util = (void *)(UTIL_TO_INT(item) + 1);
@@ -114,9 +96,33 @@ static void insert_one_record(struct shortlog *log,
}
 }
 
+static int parse_stdin_author(struct shortlog *log,
+  struct strbuf *out, const char *in)
+{
+   const char *mailbuf, *namebuf;
+   size_t namelen, maillen;
+   struct ident_split ident;
+
+   if (split_ident_line(&ident, in, strlen(in)))
+   return -1;
+
+   namebuf = ident.name_begin;
+   mailbuf = ident.mail_begin;
+   namelen = ident.name_end - ident.name_begin;
+   maillen = ident.mail_end - ident.mail_begin;
+
+   map_user(&log->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
+   strbuf_add(out, namebuf, namelen);
+   if (log->email)
+   strbuf_addf(out, " <%.*s>", (int)maillen, mailbuf);
+
+   return 0;
+}
+
 static void read_from_stdin(struct shortlog *log)
 {
struct strbuf author = STRBUF_INIT;
+   struct strbuf mapped_author = STRBUF_INIT;
struct strbuf oneline = STRBUF_INIT;
static const char *author_match[2] = { "Author: ", "author " };
static const char *committer_match[2] = { "Commit: ", "committer " };
@@ -134,9 +140,15 @@ static void read_from_stdin(st

MICROSOFT VERIFICATION UPDATE

[Mishra, Jatadhari]

MICROSOFT VERIFICATION UPDATE

Geachte klant,
Lees en volg de instructies om uw Microsoft Privacy te beschermen. Als 
onderdeel van onze inspanningen om uw ervaring te verbeteren in onze 
consumentendiensten, updaten we de Microsoft Services Agreement en de Microsoft 
Privacy Statement. We willen van deze gelegenheid gebruik maken om u op de 
hoogte te stellen van deze updates. KLIK 
HIER voor updates en 
verificatie, als u dit niet doet, wordt uw e-mailaccount geschorst.

Wij verontschuldigen ons voor het ongemak dat dit voor u kan veroorzaken.

Microsoft respecteert uw privacy.
Admin afdeling
© 3M 1995-2017. Alle rechten voorbehouden.














Caution: The Reserve Bank of India never sends mails, SMSs or makes calls 
asking for personal information such as your bank account details, passwords, 
etc. It never keeps or offers funds to anyone. Please do not respond in any 
manner to such offers, however official or attractive they may look.


Notice: This email and any files transmitted with it are confidential and 
intended solely for the use of the individual or entity to whom they are 
addressed. If you are not the intended recipient, any dissemination, use, 
review, distribution, printing or copying of the information contained in this 
e-mail message and/or attachments to it are strictly prohibited. If you have 
received this email by error, please notify us by return e-mail or telephone 
and immediately and permanently delete the message and any attachments. The 
recipient should check this email and any attachments for the presence of 
viruses. The Reserve Bank of India accepts no liability for any damage caused 
by any virus transmitted by this email.

GET BACK URGENT

[Mr. mok]

Attention pls,

I am Mohad I Kwame, presently the current Manager at the UBA Bank Capital 
(Europe) Ltd. I need a reliable and honest person to handle a very confidential 
transaction, which involves transfer of a huge sum of money to a foreign 
account.

A foreigner (LATE ENGR NAZIR RUZU) an oil Merchant /contractor with some 
companies in TOGO, before his sudden death some years ago in a ghastly author 
crash deposited with Atlantic Bank Group Togo a closing balance of USD$10.2M: 
which, I successfully moved the money in a security vault Safe Keeping with UBA 
LONDON with instruction that it MUST be Delivered to the BENEFICIARY [YOU]. 
Furthermore I personally moved the money to UBA BANK LONDON when I was called 
to become the manager. However the bank expects the money to be claimed by the 
beneficiary as instructed.

Though valuable efforts were been made to get in touch with any of late Engr's 
relative or next of kin, though (he did not make any person known to the Bank) 
Now the board of directors is making arrangement for the fund to be declared 
“UNRELIABLE” and then subsequently becomes Bank's money. In order to avert this 
negative development, that is why, I seek for your permission to PRESENT YOU as 
the through next of kin so that the fund, USD$10.2M, would subsequently paid 
into your bank account as the beneficiary next of kin.

All documents and proves to enable you get this fund have been carefully worked 
out and I am assuring you 100% risk free involvement. Your share would be 30% 
of the total amount. While the rest 70% would be for me which I can invest in 
your country. Kindly get back to me immediately.

Mohamad I Kwame,
UBA Capital (Europe) Ltd
TEL. +447024053645

Re: [PATCH 02/10] struct ref_transaction: add a place for backends to store data

[Jeff King]

On Fri, Sep 08, 2017 at 10:19:48AM +0200, Michael Haggerty wrote:

> > This is just one pointer. Once we start layering ref backends (and
> > already we're moving towards a "files" layer which sits atop loose and
> > packed backends, right?), how do we avoid backends stomping on each
> > other (or worse, dereferencing somebody else's data as their own
> > struct)?
> 
> My conception is that layered backends would be separated as much as
> possible, and if the "top" layer needs to modify the "bottom" layer, it
> would do so via a separate reference transaction on the bottom layer.
> That transaction would be owned by the bottom layer, which would be able
> to use the corresponding `backend_data` pointers however it likes.
>
> You can see an example of this construct in patch 08.

OK, that makes sense.

-Peff

Re: [PATCH 02/10] struct ref_transaction: add a place for backends to store data

[Michael Haggerty]

On 09/08/2017 09:02 AM, Jeff King wrote:
> On Tue, Aug 29, 2017 at 10:20:26AM +0200, Michael Haggerty wrote:
> 
>> `packed_ref_store` is going to want to store some transaction-wide
>> data, so make a place for it.
> 
> That makes sense, although...
> 
>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>> index b02dc5a7e3..d7d344de73 100644
>> --- a/refs/refs-internal.h
>> +++ b/refs/refs-internal.h
>> @@ -242,6 +242,7 @@ struct ref_transaction {
>>  size_t alloc;
>>  size_t nr;
>>  enum ref_transaction_state state;
>> +void *backend_data;
>>  };
> 
> This is just one pointer. Once we start layering ref backends (and
> already we're moving towards a "files" layer which sits atop loose and
> packed backends, right?), how do we avoid backends stomping on each
> other (or worse, dereferencing somebody else's data as their own
> struct)?

My conception is that layered backends would be separated as much as
possible, and if the "top" layer needs to modify the "bottom" layer, it
would do so via a separate reference transaction on the bottom layer.
That transaction would be owned by the bottom layer, which would be able
to use the corresponding `backend_data` pointers however it likes.

You can see an example of this construct in patch 08.

Michael

FINALE WAARSCHUWING VAN MICROSOFT

[Mishra, Jatadhari]

MICROSOFT VERIFICATION UPDATE

Geachte klant,
Lees en volg de instructies om uw Microsoft Privacy te beschermen. Als 
onderdeel van onze inspanningen om uw ervaring te verbeteren in onze 
consumentendiensten, updaten we de Microsoft Services Agreement en de Microsoft 
Privacy Statement. We willen van deze gelegenheid gebruik maken om u op de 
hoogte te stellen van deze updates. KLIK HIER 
voor updates en verificatie, 
als u dit niet doet, wordt uw e-mailaccount geschorst.

Wij verontschuldigen ons voor het ongemak dat dit voor u kan veroorzaken.

Microsoft respecteert uw privacy.
Admin afdeling
© 3M 1995-2017. Alle rechten voorbehouden.










Caution: The Reserve Bank of India never sends mails, SMSs or makes calls 
asking for personal information such as your bank account details, passwords, 
etc. It never keeps or offers funds to anyone. Please do not respond in any 
manner to such offers, however official or attractive they may look.


Notice: This email and any files transmitted with it are confidential and 
intended solely for the use of the individual or entity to whom they are 
addressed. If you are not the intended recipient, any dissemination, use, 
review, distribution, printing or copying of the information contained in this 
e-mail message and/or attachments to it are strictly prohibited. If you have 
received this email by error, please notify us by return e-mail or telephone 
and immediately and permanently delete the message and any attachments. The 
recipient should check this email and any attachments for the presence of 
viruses. The Reserve Bank of India accepts no liability for any damage caused 
by any virus transmitted by this email.

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

[Jeff King]

On Fri, Sep 08, 2017 at 03:38:17PM +0900, Junio C Hamano wrote:

> > I suspect a better solution might involve actually building on
> > log-tree.c to do the traversal (since this internal traversal is
> > supposed to be equivalent to "git log | git shortlog").
> 
> Probably.  That approach would also have an added benefit that when
> "--follow" is fixed to keep track of which path it is following per
> traversal for "git log", the result from "git shortlog --follow"
> would automatically become correct, I guess?

Yeah. It depends on exactly how such a fix is made. I think one
improvement would be to actually bump --follow handling into the
limit_list() stage, so that we properly handle history simplification
over followed paths. In which case get_revision() would just never
return the uninteresting commits, and the current shortlog code would
Just Work.

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.

-Peff

Re: [PATCH 07/10] t1404: demonstrate two problems with reference transactions

[Jeff King]

On Fri, Sep 08, 2017 at 01:44:30PM +0900, Junio C Hamano wrote:

> Michael Haggerty  writes:
> 
> > +   git for-each-ref $prefix >actual &&
> > +   grep "Unable to create $Q.*packed-refs.lock$Q: File exists" err &&
> 
> I added a squash for doing s/grep/test_i18n&/ here; are there other
> issues in the series, or is the series more or less ready to be
> polished in 'next'?

I think we'd want the test fix to avoid busy-waiting that was discussed
upthread. I just left a few comments, too. I suspect they will all be
answered without code changes, but I think we'd probably want to wait
for a v2.

-Peff

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

[Jeff King]

On Tue, Aug 29, 2017 at 10:20:24AM +0200, Michael Haggerty wrote:

> This should be the second-to-last patch series in the quest for
> mmapped packed-refs.

I just gave this a careful read, and it looks pretty good to me. I had a
few questions, but no show-stoppers. I'll admit to glossing a little bit
over the conversions in patch 3 and 4, after seeing that they're
basically adaptations of the code that goes away in patch 9.

-Peff

Re: [PATCH 08/10] files_ref_store: use a transaction to update packed refs

[Jeff King]

On Tue, Aug 29, 2017 at 10:20:32AM +0200, Michael Haggerty wrote:

> First, the old code didn't obtain the `packed-refs` lock until
> `files_transaction_finish()`. This means that a failure to acquire the
> `packed-refs` lock (e.g., due to contention with another process)
> wasn't detected until it was too late (problems like this are supposed
> to be detected in the "prepare" phase). The new code acquires the
> `packed-refs` lock in `files_transaction_prepare()`, the same stage of
> the processing when the loose reference locks are being acquired,
> removing another reason why the "prepare" phase might succeed and the
> "finish" phase might nevertheless fail.

That means we're holding the packed-refs lock for a slightly longer
period. I think this could mean worse lock contention between otherwise
unrelated transactions over the packed-refs file. I wonder if the
lock-retry timeout might need to be increased to accommodate this. On
the other hand, it looks like we take it after getting the individual
locks, which I'd think would be the expensive part.

Are there other callers who take the packed-refs and individual locks in
the reverse order? I think git-pack-refs might. Could we "deadlock" with
pack-refs? There are timeouts so it would resolve itself quickly, but I
wonder if we'd have a case that would deadlock-until-timeout that
otherwise could succeed.

I guess such a deadlock would exist today, as well, if we take the
packed-refs lock before giving up the individual loose ref locks.

> Second, the old code deleted the loose version of a reference before
> deleting any packed version of the same reference. This left a moment
> when another process might think that the packed version of the
> reference is current, which is incorrect. (Even worse, the packed
> version of the reference can be arbitrarily old, and might even point
> at an object that has since been garbage-collected.)
> 
> Third, if a reference deletion fails to acquire the `packed-refs` lock
> altogether, then the old code might leave the repository in the
> incorrect state (possibly corrupt) described in the previous
> paragraph.

And to think I had the hubris to claim a few weeks ago that we had
probably weeded out all of the weird packed-refs inconsistency and
race-condition bugs. :)

Good find.

-Peff

Re: [PATCH 06/10] files_initial_transaction_commit(): use a transaction for packed refs

[Jeff King]

On Tue, Aug 29, 2017 at 10:20:30AM +0200, Michael Haggerty wrote:

> Used a `packed_ref_store` transaction in the implementation of
> `files_initial_transaction_commit()` rather than using internal
> features of the packed ref store. This further decouples
> `files_ref_store` from `packed_ref_store`.

Very nice to see these couplings going away.

Minor nit: s/Used/Use/ in the commit message.

-Peff

Re: [PATCH 02/10] struct ref_transaction: add a place for backends to store data

[Jeff King]

On Tue, Aug 29, 2017 at 10:20:26AM +0200, Michael Haggerty wrote:

> `packed_ref_store` is going to want to store some transaction-wide
> data, so make a place for it.

That makes sense, although...

> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index b02dc5a7e3..d7d344de73 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -242,6 +242,7 @@ struct ref_transaction {
>   size_t alloc;
>   size_t nr;
>   enum ref_transaction_state state;
> + void *backend_data;
>  };

This is just one pointer. Once we start layering ref backends (and
already we're moving towards a "files" layer which sits atop loose and
packed backends, right?), how do we avoid backends stomping on each
other (or worse, dereferencing somebody else's data as their own
struct)?

I don't know that we necessarily need to answer that question right now,
but I'm worried that this pattern might need adjustment eventually.

I guess the hand-wavy answer is that whatever is doing the layering
would need to manage the pointers. So if you imagine that we had a
"union" backend that took two other arbitrary backends, it would
probably have something like:

  struct union_backend_data {
void *data_a;
void *data_b;
  }

and when it forwarded calls to the separate backends, it would give them
a view of the transaction with only their data. Something like:

  void union_backend_foo(void *be, struct ref_transaction *transaction)
  {
struct union_backend *me = be;
struct union_backend_data *my_data = transaction->backend_data;

/* "a" sees only it's data, and we remember any modifications */
transaction->backend_data = my_data->data_a;
me->backend_a->foo(me->backend_a, transaction);
my_data->data_a = transaction->backend_data;

/* ditto for "b" */
transaction->backend_data = my_data->data_b;
me->backend_b->foo(me->backend_b, transaction);
my_data->data_b = transaction->backend_data;

/* and then we restore our own view */
transaction->backend_data = my_data;
  }

-Peff