Re: [PATCH 1/2] t1400: add some more tests of update-ref --stdin's verify command

[Brad King]

On 12/10/2014 6:47 PM, Michael Haggerty wrote:
 Two of the tests fail because
 
 verify refs/heads/foo
 
 with no argument (not even zeros) actually *deletes* refs/heads/foo.
 This problem will be fixed in the next commit.

Reviewed-by: Brad King brad.k...@kitware.com

-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] update-ref: fix verify command with missing oldvalue

[Brad King]

On 12/10/2014 6:47 PM, Michael Haggerty wrote:
 set have_old unconditionally and set old_sha1 to null_sha1.

Reviewed-by: Brad King brad.k...@kitware.com

-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] update-ref: fail create operation over stdin if ref already exists

[Brad King]

On 04/02/2014 04:09 AM, Michael Haggerty wrote:
 From: Aman Gupta a...@tmm1.net
[snip]
 @@ -147,6 +147,7 @@ static void parse_cmd_create(const char *next)
   struct ref_update *update;
  
   update = update_alloc();
 + update-have_old = 1;

Looks good.

 +test_expect_success 'stdin -z create ref fails when ref exists' '

Strictly speaking we should have a non-z mode test too.

Thanks,
-Brad

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/27] update-ref.c: Extract a new function, parse_next_sha1()

[Brad King]

On 03/24/2014 01:56 PM, Michael Haggerty wrote:
 +/*
 + * For backwards compatibility, accept an empty string for create's
 + * newvalue in binary mode to be equivalent to specifying zeros.
 + */
 +#define PARSE_SHA1_ALLOW_EMPTY 0x02

The comment should say update's, not create's.

-Brad

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 16/27] t1400: Test one mistake at a time

[Brad King]

On 03/24/2014 01:56 PM, Michael Haggerty wrote:
 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 
 t1400: Add a test of update with too few arguments
 
 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu

This looks like a stray squash message.

-Brad

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/27] Clean up update-refs --stdin and implement ref_transaction

[Brad King]

On 03/24/2014 01:56 PM, Michael Haggerty wrote:
 Changes relative to v1:
 
 * Rename the functions associated with ref_transactions to be more
   reminiscent of database transactions:
 
   * create_ref_transaction() - ref_transaction_begin()
   * free_ref_transaction() - ref_transaction_rollback()
   * queue_update_ref() - ref_transaction_update()
   * queue_create_ref() - ref_transaction_create()
   * queue_delete_ref() - ref_transaction_delete()
   * commit_ref_transaction() - ref_transaction_commit()

Those new names look better.

 * Fix backwards compatibility of git update-ref --stdin -z's
   handling of the create command: allow newvalue to be the empty
   string, treating it the same zeros.  But deprecate this usage.

The changes related to that look good.  The new documentation is
much clearer than my old wording.

Series v2 looks good to me except for my responses to individual
commits.

Thanks,
-Brad

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 19/27] refs: Add a concept of a reference transaction

[Brad King]

On 03/24/2014 01:56 PM, Michael Haggerty wrote:
 +void ref_transaction_update(struct ref_transaction *transaction,
 + const char *refname,
 + unsigned char *new_sha1, unsigned char *old_sha1,
 + int flags, int have_old);
[snip]
 +void ref_transaction_create(struct ref_transaction *transaction,
 + const char *refname,
 + unsigned char *new_sha1,
 + int flags);
[snip]
 +void ref_transaction_delete(struct ref_transaction *transaction,
 + const char *refname,
 + unsigned char *old_sha1,
 + int flags, int have_old);

Perhaps we also need:

void ref_transaction_verify(struct ref_transaction *transaction,
const char *refname,
unsigned char *old_sha1,
int flags, int have_old);

as equivalent to the verify command in update-ref --stdin?

-Brad

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/26] t1400: Pass a legitimate newvalue to update command

[Brad King]

On 03/10/2014 05:38 PM, Michael Haggerty wrote:
 It seems to me that -z input will nearly always be machine-generated,
 so there is not much reason to accept the empty string as shorthand for
 zeros.  So I think that my version of the rules, being simpler to
 explain, is a slight improvement.

I agree.

 But your version is already out in the wild, so backwards-compatibility
 is also a consideration, even though it is rather a fine point in a
 rather unlikely usage (why use update rather than delete to delete a
 reference?).

I'm not using empty==zero with -z in any deployment.  Since the feature
is quite new, the behavior change is not silent, and it is easy to
construct input that works with both versions, I do not think we need
to worry about compatibility.

 or rewrite the documentation to describe my rules.

I prefer this approach.

Thanks,
-Brad

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/26] t1400: Pass a legitimate newvalue to update command

[Brad King]

On Tue, Mar 11, 2014 at 4:06 PM, Junio C Hamano gits...@pobox.com wrote:
 I may be misremembering things, but your first sentence quoted above
 was exactly my reaction while reviewing the original change, and I
 might have even raised that as an issue myself, saying something
 like consistency across values is more important than type-saving
 in a machine format.

For reference, the original design discussion of the format was here:

 http://thread.gmane.org/gmane.comp.version-control.git/233842

I do not recall this issue being raised before, but now that it has
been raised I fully agree:

 http://thread.gmane.org/gmane.comp.version-control.git/243754/focus=243862

In -z mode an empty newvalue should be treated as missing just as
it is for oldvalue.  This is obvious now in hindsight and I wish I
had realized this at the time.  Back then I went through a lot of
iterations on the format and missed this simplification in the final
version :(

Moving forward:

The create command rejects a zero newvalue so the change in
question for that command is merely the wording of the error message
and there is no compatibility issue.

The update command supports a zero newvalue so that it can
be used for all operations (create, update, delete, verify) with
the proper combination of old and new values.  The change in question
makes an empty newvalue an error where it was previously treated
as zero.  (BTW, Michael, I do not see a test case for the new error
in your series.  Something like the patch below should work.)

 I am not against deprecating and removing
 the support for it in the longer term, though.

As I reported in my above-linked response, I'm not depending on
the old behavior myself.  Also if one were to start seeing this
error then generated input needs only trivial changes to avoid it.
If we do want to preserve compatibility for others then perhaps an
empty newvalue with -z should produce:

 warning: update $ref: missing newvalue, treating as zero

Then after a few releases it can be switched to an error.

Thanks,
-Brad


diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 3cc5c66..1e9fe7c 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -730,6 +730,12 @@ test_expect_success 'stdin -z fails update with bad ref 
name' '
grep fatal: invalid ref format: ~a err
 '

+test_expect_success 'stdin -z fails update with empty new value' '
+   printf $F update $a  stdin 
+   test_must_fail git update-ref -z --stdin stdin 2err 
+   grep fatal: update $a: missing newvalue err
+'
+
 test_expect_success 'stdin -z fails update with no new value' '
printf $F update $a stdin 
test_must_fail git update-ref -z --stdin stdin 2err 
-- 
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/26] t1400: Pass a legitimate newvalue to update command

[Brad King]

On 03/10/2014 08:46 AM, Michael Haggerty wrote:
 This test is trying to test a few ways to delete references using git
 update-ref -z --stdin.  The third line passed in is
 
 update SP /refs/heads/c NUL NUL sha1 NUL
 
 , which is not a correct way to delete a reference according to the
 documentation (the new value should be zeros, not empty).  Pass zeros
 instead as the new value to test the code correctly.

In my original work on this feature, an empty newvalue is allowed.
Since newvalue is not optional an empty value can be treated as zero.
The relevant documentation is:

 update::
 Set ref to newvalue after verifying oldvalue, if given.
 Specify a zero newvalue to ensure the ref does not exist

 ...

 Use 40 0 or the empty string to specify a zero value, except that
 with `-z` an empty oldvalue is considered missing.

The two together say that newvalue can be the empty string instead
of a literal zero.

-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/26] update-ref --stdin: Simplify error messages for missing oldvalues

[Brad King]

On 03/10/2014 01:08 PM, Brad King wrote:
 -die(update %s missing [oldvalue] NUL, update-ref_name);
 +die(update %s missing oldvalue, update-ref_name);
 
 The reason for the original wording is that the oldvalue is indeed
 optional.  This can only occur at end-of-input, and it is actually the
 *NUL* that is missing because an empty old value can be specified to
 mean that it it intentionally missing.

I see a following patch makes the wording even clearer about
unexpected end of input, so ignore my previous review.

-Brad

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/26] update-ref --stdin: Simplify error messages for missing oldvalues

[Brad King]

On 03/10/2014 08:46 AM, Michael Haggerty wrote:
 Instead of, for example,
 
 fatal: update refs/heads/master missing [oldvalue] NUL
 
 emit
 
 fatal: update refs/heads/master missing oldvalue
[snip]
 - die(update %s missing [oldvalue] NUL, update-ref_name);
 + die(update %s missing oldvalue, update-ref_name);

The reason for the original wording is that the oldvalue is indeed
optional.  This can only occur at end-of-input, and it is actually the
*NUL* that is missing because an empty old value can be specified to
mean that it it intentionally missing.

-Brad

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/26] Clean up update-refs --stdin and implement ref_transaction

[Brad King]

Hi Michael,

This is excellent work.

I haven't reviewed every line of logic in detail but the changes
look correct at a high level.  The only exception is that the empty
newvalue is supposed to be accepted and treated as zero even in
--stdin -z mode.  See my response to that individual change.

On 03/10/2014 08:46 AM, Michael Haggerty wrote:
 The new API for dealing with reference transactions is
 
 ref_transaction *transaction = create_ref_transaction();
 queue_create_ref(transaction, refname, new_sha1, ...);
 queue_update_ref(transaction, refname, new_sha1, old_sha1, ...);
 queue_delete_ref(transaction, refname, old_sha1, ...);
 ...
 if (commit_ref_transaction(transaction, msg, ...))
 die(...);

The layout of this API looks good.

The name queue is not fully representative of the current behavior.
It implies that the order is meaningful but we currently allow at most
one update to a ref and sort them by refname.  Does your follow-up work
define behavior for multiple updates to one ref?  Can it collapse them
into a single update after checking internal consistency of the sequence?

 So most of the commits in this series are actually cleanups in
 builtin/update-ref.c.  I also spend some time making the error
 messages emitted by that command more uniform.

All good cleanups, thanks.

 Finally, now that refs.c owns the data structures for dealing with
 transactions, it is possible to make a few simplifications.

Yes, it is much nicer to keep the data structures private, especially
as it avoids the copy of the transaction made before sorting.

Thanks,
-Brad

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/4] t3030-merge-recursive: Test known breakage with empty work tree

[Brad King]

Sometimes when working with a large repository it can be useful to try
out a merge and only check out conflicting files to disk (for example as
a speed optimization on a server).  Until v1.7.7-rc1~28^2~20
(merge-recursive: When we detect we can skip an update, actually skip
it, 2011-08-11), it was possible to do so with the following idiom:

# Prepare a temporary index and empty work tree.
GIT_INDEX_FILE=$PWD/tmp-$$-index 
export GIT_INDEX_FILE 
GIT_WORK_TREE=$PWD/tmp-$$-work 
export GIT_WORK_TREE 
mkdir $GIT_WORK_TREE 

# Convince the index that our side is on disk.
git read-tree -i -m $ours 
git update-index --ignore-missing --refresh 

# Merge their side into our side.
bases=$(git merge-base --all $ours $theirs) 
git merge-recursive $bases -- $ours $theirs 
tree=$(git write-tree)

Nowadays, that still works and the exit status is the same, but
merge-recursive produces a diagnostic if our side renamed a file:

error: addinfo_cache failed for path 'dst'

Add a test to document this regression.

Signed-off-by: Brad King brad.k...@kitware.com
---
 t/t3030-merge-recursive.sh | 47 ++
 1 file changed, 47 insertions(+)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 2f96100..3db3bf6 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -257,6 +257,7 @@ test_expect_success 'setup 8' '
git add e 
test_tick 
git commit -m rename a-e 
+   c7=$(git rev-parse --verify HEAD) 
git checkout rename-ln 
git mv a e 
test_ln_s_add e a 
@@ -517,6 +518,52 @@ test_expect_success 'reset and bind merge' '
 
 '
 
+test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+   (
+   GIT_WORK_TREE=$PWD/ours-has-rename-work 
+   export GIT_WORK_TREE 
+   GIT_INDEX_FILE=$PWD/ours-has-rename-index 
+   export GIT_INDEX_FILE 
+   mkdir $GIT_WORK_TREE 
+   git read-tree -i -m $c7 
+   git update-index --ignore-missing --refresh 
+   git merge-recursive $c0 -- $c7 $c3 
+   git ls-files -s actual-files
+   ) 2actual-err 
+   expected-err 
+   cat expected-files -EOF 
+   100644 $o3 0b/c
+   100644 $o0 0c
+   100644 $o0 0d/e
+   100644 $o0 0e
+   EOF
+   test_cmp expected-files actual-files 
+   test_cmp expected-err actual-err
+'
+
+test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
+   (
+   GIT_WORK_TREE=$PWD/theirs-has-rename-work 
+   export GIT_WORK_TREE 
+   GIT_INDEX_FILE=$PWD/theirs-has-rename-index 
+   export GIT_INDEX_FILE 
+   mkdir $GIT_WORK_TREE 
+   git read-tree -i -m $c3 
+   git update-index --ignore-missing --refresh 
+   git merge-recursive $c0 -- $c3 $c7 
+   git ls-files -s actual-files
+   ) 2actual-err 
+   expected-err 
+   cat expected-files -EOF 
+   100644 $o3 0b/c
+   100644 $o0 0c
+   100644 $o0 0d/e
+   100644 $o0 0e
+   EOF
+   test_cmp expected-files actual-files 
+   test_cmp expected-err actual-err
+'
+
 test_expect_success 'merge removes empty directories' '
 
git reset --hard master 
-- 
1.8.5.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 0/3] merge-recursive: Avoid diagnostic on empty work tree

[Brad King]

Hi Folks,

Here is the third revision of this series.  The previous
revisions can be found at $gmane/241009 and $gmane/241030.

Updates since the previous revision of the series:

* Handling of lstat ENOENT has been moved down into refresh_cache_ent
  and activated by a new CE_MATCH_IGNORE_MISSING option.

* Rather than adding a new argument to make_cache_entry, the existing
  'refresh' boolean argument has been generalized to a set of options.
  This required the addition of a new CE_MATCH_REFRESH option to
  enable refresh with no other options.

Thanks,
-Brad

Brad King (4):
  t3030-merge-recursive: Test known breakage with empty work tree
  read-cache.c: Refactor --ignore-missing implementation
  read-cache.c: Extend make_cache_entry refresh flag with options
  merge-recursive.c: Tolerate missing files while refreshing index

 cache.h|  6 +-
 merge-recursive.c  |  4 +++-
 read-cache.c   | 27 ++
 t/t3030-merge-recursive.sh | 47 ++
 4 files changed, 70 insertions(+), 14 deletions(-)

-- 
1.8.5.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/4] read-cache.c: Refactor --ignore-missing implementation

[Brad King]

Move lstat ENOENT handling from refresh_index to refresh_cache_ent and
activate it with a new CE_MATCH_IGNORE_MISSING option.  This will allow
other call paths into refresh_cache_ent to use the feature.

Signed-off-by: Brad King brad.k...@kitware.com
---
 cache.h  | 2 ++
 read-cache.c | 8 +---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index c9efe88..c96ada7 100644
--- a/cache.h
+++ b/cache.h
@@ -498,6 +498,8 @@ extern void *read_blob_data_from_index(struct index_state 
*, const char *, unsig
 #define CE_MATCH_RACY_IS_DIRTY 02
 /* do stat comparison even if CE_SKIP_WORKTREE is true */
 #define CE_MATCH_IGNORE_SKIP_WORKTREE  04
+/* ignore non-existent files during stat update  */
+#define CE_MATCH_IGNORE_MISSING0x08
 extern int ie_match_stat(const struct index_state *, const struct cache_entry 
*, struct stat *, unsigned int);
 extern int ie_modified(const struct index_state *, const struct cache_entry *, 
struct stat *, unsigned int);
 
diff --git a/read-cache.c b/read-cache.c
index 33dd676..d61846c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1031,6 +1031,7 @@ static struct cache_entry *refresh_cache_ent(struct 
index_state *istate,
int changed, size;
int ignore_valid = options  CE_MATCH_IGNORE_VALID;
int ignore_skip_worktree = options  CE_MATCH_IGNORE_SKIP_WORKTREE;
+   int ignore_missing = options  CE_MATCH_IGNORE_MISSING;
 
if (ce_uptodate(ce))
return ce;
@@ -1050,6 +1051,8 @@ static struct cache_entry *refresh_cache_ent(struct 
index_state *istate,
}
 
if (lstat(ce-name, st)  0) {
+   if (ignore_missing  errno == ENOENT)
+   return ce;
if (err)
*err = errno;
return NULL;
@@ -1127,7 +1130,8 @@ int refresh_index(struct index_state *istate, unsigned 
int flags,
int ignore_submodules = (flags  REFRESH_IGNORE_SUBMODULES) != 0;
int first = 1;
int in_porcelain = (flags  REFRESH_IN_PORCELAIN);
-   unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
+   unsigned int options = ((really ? CE_MATCH_IGNORE_VALID : 0) |
+   (not_new ? CE_MATCH_IGNORE_MISSING : 0));
const char *modified_fmt;
const char *deleted_fmt;
const char *typechange_fmt;
@@ -1176,8 +1180,6 @@ int refresh_index(struct index_state *istate, unsigned 
int flags,
if (!new) {
const char *fmt;
 
-   if (not_new  cache_errno == ENOENT)
-   continue;
if (really  cache_errno == EINVAL) {
/* If we are doing --really-refresh that
 * means the index is not valid anymore.
-- 
1.8.5.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/4] merge-recursive.c: Tolerate missing files while refreshing index

[Brad King]

Teach add_cacheinfo to tell make_cache_entry to skip refreshing stat
information when a file is missing from the work tree.  We do not want
the index to be stat-dirty after the merge but also do not want to fail
when a file happens to be missing.

This fixes the 'merge-recursive w/ empty work tree - ours has rename'
case in t3030-merge-recursive.

Suggested-by: Elijah Newren new...@gmail.com
Signed-off-by: Brad King brad.k...@kitware.com
---
 merge-recursive.c  | 3 ++-
 t/t3030-merge-recursive.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index c3753c8..b8ea172 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -202,7 +202,8 @@ static int add_cacheinfo(unsigned int mode, const unsigned 
char *sha1,
 {
struct cache_entry *ce;
ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage,
- (refresh ? CE_MATCH_REFRESH : 0 ));
+ (refresh ? (CE_MATCH_REFRESH |
+ CE_MATCH_IGNORE_MISSING) : 0 ));
if (!ce)
return error(_(addinfo_cache failed for path '%s'), path);
return add_cache_entry(ce, options);
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 3db3bf6..82e1854 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -518,7 +518,7 @@ test_expect_success 'reset and bind merge' '
 
 '
 
-test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
(
GIT_WORK_TREE=$PWD/ours-has-rename-work 
export GIT_WORK_TREE 
-- 
1.8.5.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/4] read-cache.c: Extend make_cache_entry refresh flag with options

[Brad King]

Convert the make_cache_entry boolean 'refresh' argument to a more
general 'refresh_options' argument.  Pass the value through to the
underlying refresh_cache_ent call.  Add option CE_MATCH_REFRESH to
enable stat refresh.  Update call sites to use the new signature.

Signed-off-by: Brad King brad.k...@kitware.com
---
 cache.h   |  4 +++-
 merge-recursive.c |  3 ++-
 read-cache.c  | 21 +++--
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/cache.h b/cache.h
index c96ada7..e8820e1 100644
--- a/cache.h
+++ b/cache.h
@@ -487,7 +487,7 @@ extern int remove_file_from_index(struct index_state *, 
const char *path);
 #define ADD_CACHE_IMPLICIT_DOT 32  /* internal to git add -u/-A */
 extern int add_to_index(struct index_state *, const char *path, struct stat *, 
int flags);
 extern int add_file_to_index(struct index_state *, const char *path, int 
flags);
-extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned 
char *sha1, const char *path, int stage, int refresh);
+extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned 
char *sha1, const char *path, int stage, unsigned int refresh_options);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry 
*b);
 extern int index_name_is_other(const struct index_state *, const char *, int);
 extern void *read_blob_data_from_index(struct index_state *, const char *, 
unsigned long *);
@@ -500,6 +500,8 @@ extern void *read_blob_data_from_index(struct index_state 
*, const char *, unsig
 #define CE_MATCH_IGNORE_SKIP_WORKTREE  04
 /* ignore non-existent files during stat update  */
 #define CE_MATCH_IGNORE_MISSING0x08
+/* enable stat refresh */
+#define CE_MATCH_REFRESH   0x10
 extern int ie_match_stat(const struct index_state *, const struct cache_entry 
*, struct stat *, unsigned int);
 extern int ie_modified(const struct index_state *, const struct cache_entry *, 
struct stat *, unsigned int);
 
diff --git a/merge-recursive.c b/merge-recursive.c
index a18bd15..c3753c8 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -201,7 +201,8 @@ static int add_cacheinfo(unsigned int mode, const unsigned 
char *sha1,
const char *path, int stage, int refresh, int options)
 {
struct cache_entry *ce;
-   ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage, 
refresh);
+   ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage,
+ (refresh ? CE_MATCH_REFRESH : 0 ));
if (!ce)
return error(_(addinfo_cache failed for path '%s'), path);
return add_cache_entry(ce, options);
diff --git a/read-cache.c b/read-cache.c
index d61846c..db3902e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -15,7 +15,8 @@
 #include strbuf.h
 #include varint.h
 
-static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int 
really);
+static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
+  unsigned int options);
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
@@ -696,7 +697,7 @@ int add_file_to_index(struct index_state *istate, const 
char *path, int flags)
 
 struct cache_entry *make_cache_entry(unsigned int mode,
const unsigned char *sha1, const char *path, int stage,
-   int refresh)
+   unsigned int refresh_options)
 {
int size, len;
struct cache_entry *ce;
@@ -716,10 +717,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,
ce-ce_namelen = len;
ce-ce_mode = create_ce_mode(mode);
 
-   if (refresh)
-   return refresh_cache_entry(ce, 0);
-
-   return ce;
+   return refresh_cache_entry(ce, refresh_options);
 }
 
 int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
@@ -1029,11 +1027,12 @@ static struct cache_entry *refresh_cache_ent(struct 
index_state *istate,
struct stat st;
struct cache_entry *updated;
int changed, size;
+   int refresh = options  CE_MATCH_REFRESH;
int ignore_valid = options  CE_MATCH_IGNORE_VALID;
int ignore_skip_worktree = options  CE_MATCH_IGNORE_SKIP_WORKTREE;
int ignore_missing = options  CE_MATCH_IGNORE_MISSING;
 
-   if (ce_uptodate(ce))
+   if (!refresh || ce_uptodate(ce))
return ce;
 
/*
@@ -1130,7 +1129,8 @@ int refresh_index(struct index_state *istate, unsigned 
int flags,
int ignore_submodules = (flags  REFRESH_IGNORE_SUBMODULES) != 0;
int first = 1;
int in_porcelain = (flags  REFRESH_IN_PORCELAIN);
-   unsigned int options = ((really ? CE_MATCH_IGNORE_VALID : 0) |
+   unsigned int options = (CE_MATCH_REFRESH |
+   (really ? CE_MATCH_IGNORE_VALID : 0) |
(not_new ? CE_MATCH_IGNORE_MISSING : 0));
const char *modified_fmt

[PATCH/RFC 1/3] t3030-merge-recursive: Test known breakage with empty work tree

[Brad King]

Add test cases that use 'merge-recursive' plumbing with a temporary
index and empty work tree.  Populate the index using 'read-tree' and
'update-index --ignore-missing --refresh' to prepare for merge without
actually checking all files out to disk.  Verify that each merge
produces its expected tree while displaying no error diagnostics.

This approach can be used to compute tree merges while checking out only
conflicting files to disk (which is useful for server-side scripts).
Prior to commit 5b448b85 (merge-recursive: When we detect we can skip an
update, actually skip it, 2011-08-11) this worked cleanly in all cases.
Since that commit, merge-recursive displays a diagnostic such as

 error: addinfo_cache failed for path 'e'

when our side has a rename (to 'e').  The diagnostic does not
influence the return code and the merge appears to succeed, but it
causes this test case to fail.

Signed-off-by: Brad King brad.k...@kitware.com
---
 t/t3030-merge-recursive.sh | 47 ++
 1 file changed, 47 insertions(+)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 2f96100..b6d3ed0 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -257,6 +257,7 @@ test_expect_success 'setup 8' '
git add e 
test_tick 
git commit -m rename a-e 
+   c7=$(git rev-parse --verify HEAD) 
git checkout rename-ln 
git mv a e 
test_ln_s_add e a 
@@ -517,6 +518,52 @@ test_expect_success 'reset and bind merge' '
 
 '
 
+test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+   (
+GIT_WORK_TREE=$PWD/ours-has-rename-work 
+export GIT_WORK_TREE 
+GIT_INDEX_FILE=$PWD/ours-has-rename-index 
+export GIT_INDEX_FILE 
+mkdir $GIT_WORK_TREE 
+git read-tree -i -m $c7 
+git update-index --ignore-missing --refresh 
+git merge-recursive $c0 -- $c7 $c3 
+git ls-files -s actual-files
+   ) 2actual-err 
+   expected-err 
+   cat expected-files -EOF 
+   100644 $o3 0b/c
+   100644 $o0 0c
+   100644 $o0 0d/e
+   100644 $o0 0e
+   EOF
+   test_cmp expected-files actual-files 
+   test_cmp expected-err actual-err
+'
+
+test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
+   (
+GIT_WORK_TREE=$PWD/theirs-has-rename-work 
+export GIT_WORK_TREE 
+GIT_INDEX_FILE=$PWD/theirs-has-rename-index 
+export GIT_INDEX_FILE 
+mkdir $GIT_WORK_TREE 
+git read-tree -i -m $c3 
+git update-index --ignore-missing --refresh 
+git merge-recursive $c0 -- $c3 $c7 
+git ls-files -s actual-files
+   ) 2actual-err 
+   expected-err 
+   cat expected-files -EOF 
+   100644 $o3 0b/c
+   100644 $o0 0c
+   100644 $o0 0d/e
+   100644 $o0 0e
+   EOF
+   test_cmp expected-files actual-files 
+   test_cmp expected-err actual-err
+'
+
 test_expect_success 'merge removes empty directories' '
 
git reset --hard master 
-- 
1.8.5.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 0/3] merge-recursive: Avoid diagnostic on empty work tree

[Brad King]

On 01/23/2014 07:24 PM, Elijah Newren wrote:
 Two options are just doing a stat to determine whether the file
 is present (which means we'll be stat'ing the file multiple times
 in these cases, which feels wasteful), or perhaps writing a
 modified make_cache_entry() with the behavior we want
 (seems like ugly code duplication).  Suggestions?

Perhaps we can thread enough information through the make_cache_entry
signature to allow the caller to know when lstat reported ENOENT.
Here is a series that takes such an approach.

* Patch 1 is the original test case from $gmane/240853.

* Patch 2 extends the make_cache_entry signature to return lstat errno.

* Patch 3 uses this information to silence the add_cacheinfo diagnostic

-Brad

Brad King (3):
  t3030-merge-recursive: Test known breakage with empty work tree
  read-cache.c: Thread lstat error through make_cache_entry signature
  merge-recursive: Tolerate missing file when HEAD is up to date

 builtin/apply.c|  2 +-
 builtin/checkout.c |  2 +-
 builtin/reset.c|  2 +-
 cache.h|  2 +-
 merge-recursive.c  | 22 ++
 read-cache.c   | 12 +++-
 resolve-undo.c |  2 +-
 t/t3030-merge-recursive.sh | 47 ++
 8 files changed, 73 insertions(+), 18 deletions(-)

-- 
1.8.5.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 3/3] merge-recursive: Tolerate missing file when HEAD is up to date

[Brad King]

Teach add_cacheinfo to optionally tolerate make_cache_entry failure when
the reason is ENOENT from lstat.  Tell it to do so in the call path when
the entry from HEAD is known to be up to date.

This fixes the 'merge-recursive w/ empty work tree - ours has rename'
case in t3030-merge-recursive.

Signed-off-by: Brad King brad.k...@kitware.com
---
 merge-recursive.c  | 21 +
 t/t3030-merge-recursive.sh |  2 +-
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 4394c44..6a2b962 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -198,13 +198,18 @@ static void output_commit_title(struct merge_options *o, 
struct commit *commit)
 }
 
 static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
-   const char *path, int stage, int refresh, int options)
+const char *path, int stage, int refresh,
+int options, int noent_okay)
 {
struct cache_entry *ce;
+   int cache_errno = 0;
ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path,
- stage, refresh, NULL);
-   if (!ce)
+ stage, refresh, cache_errno);
+   if (!ce) {
+   if(cache_errno == ENOENT  noent_okay)
+   return 0;
return error(_(addinfo_cache failed for path '%s'), path);
+   }
return add_cache_entry(ce, options);
 }
 
@@ -552,13 +557,13 @@ static int update_stages(const char *path, const struct 
diff_filespec *o,
if (remove_file_from_cache(path))
return -1;
if (o)
-   if (add_cacheinfo(o-mode, o-sha1, path, 1, 0, options))
+   if (add_cacheinfo(o-mode, o-sha1, path, 1, 0, options, 0))
return -1;
if (a)
-   if (add_cacheinfo(a-mode, a-sha1, path, 2, 0, options))
+   if (add_cacheinfo(a-mode, a-sha1, path, 2, 0, options, 0))
return -1;
if (b)
-   if (add_cacheinfo(b-mode, b-sha1, path, 3, 0, options))
+   if (add_cacheinfo(b-mode, b-sha1, path, 3, 0, options, 0))
return -1;
return 0;
 }
@@ -789,7 +794,7 @@ static void update_file_flags(struct merge_options *o,
}
  update_index:
if (update_cache)
-   add_cacheinfo(mode, sha, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD);
+   add_cacheinfo(mode, sha, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD, 0);
 }
 
 static void update_file(struct merge_options *o,
@@ -1624,7 +1629,7 @@ static int merge_content(struct merge_options *o,
path_renamed_outside_HEAD = !path2 || !strcmp(path, path2);
if (!path_renamed_outside_HEAD) {
add_cacheinfo(mfi.mode, mfi.sha, path,
- 0, (!o-call_depth), 0);
+ 0, (!o-call_depth), 0, 1);
return mfi.clean;
}
} else
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index b6d3ed0..c8ba895 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -518,7 +518,7 @@ test_expect_success 'reset and bind merge' '
 
 '
 
-test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
(
 GIT_WORK_TREE=$PWD/ours-has-rename-work 
 export GIT_WORK_TREE 
-- 
1.8.5.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 2/3] read-cache.c: Thread lstat error through make_cache_entry signature

[Brad King]

Add an 'int *err' argument to make_cache_entry to receive any error
that occurred when matching stat information for a file on disk.
Thread it through to the same argument of refresh_cache_ent.
This will allow callers of make_cache_entry to determine whether
failure was due to a missing file on disk.

Signed-off-by: Brad King brad.k...@kitware.com
---
 builtin/apply.c|  2 +-
 builtin/checkout.c |  2 +-
 builtin/reset.c|  2 +-
 cache.h|  2 +-
 merge-recursive.c  |  3 ++-
 read-cache.c   | 12 +++-
 resolve-undo.c |  2 +-
 7 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index b0d0986..15e14ce 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3675,7 +3675,7 @@ static void build_fake_ancestor(struct patch *list, const 
char *filename)
die(sha1 information is lacking or useless 
(%s)., name);
 
-   ce = make_cache_entry(patch-old_mode, sha1, name, 0, 0);
+   ce = make_cache_entry(patch-old_mode, sha1, name, 0, 0, NULL);
if (!ce)
die(_(make_cache_entry failed for path '%s'), name);
if (add_index_entry(result, ce, ADD_CACHE_OK_TO_ADD))
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5df3837..c7338bb 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -208,7 +208,7 @@ static int checkout_merged(int pos, struct checkout *state)
if (write_sha1_file(result_buf.ptr, result_buf.size,
blob_type, sha1))
die(_(Unable to add merge result for '%s'), path);
-   ce = make_cache_entry(mode, sha1, path, 2, 0);
+   ce = make_cache_entry(mode, sha1, path, 2, 0, NULL);
if (!ce)
die(_(make_cache_entry failed for path '%s'), path);
status = checkout_entry(ce, state, NULL);
diff --git a/builtin/reset.c b/builtin/reset.c
index 6004803..8e0375d 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -122,7 +122,7 @@ static void update_index_from_diff(struct diff_queue_struct 
*q,
if (one-mode  !is_null_sha1(one-sha1)) {
struct cache_entry *ce;
ce = make_cache_entry(one-mode, one-sha1, one-path,
-   0, 0);
+ 0, 0, NULL);
if (!ce)
die(_(make_cache_entry failed for path '%s'),
one-path);
diff --git a/cache.h b/cache.h
index c9efe88..8e4f17d 100644
--- a/cache.h
+++ b/cache.h
@@ -487,7 +487,7 @@ extern int remove_file_from_index(struct index_state *, 
const char *path);
 #define ADD_CACHE_IMPLICIT_DOT 32  /* internal to git add -u/-A */
 extern int add_to_index(struct index_state *, const char *path, struct stat *, 
int flags);
 extern int add_file_to_index(struct index_state *, const char *path, int 
flags);
-extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned 
char *sha1, const char *path, int stage, int refresh);
+extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned 
char *sha1, const char *path, int stage, int refresh, int *err);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry 
*b);
 extern int index_name_is_other(const struct index_state *, const char *, int);
 extern void *read_blob_data_from_index(struct index_state *, const char *, 
unsigned long *);
diff --git a/merge-recursive.c b/merge-recursive.c
index a18bd15..4394c44 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -201,7 +201,8 @@ static int add_cacheinfo(unsigned int mode, const unsigned 
char *sha1,
const char *path, int stage, int refresh, int options)
 {
struct cache_entry *ce;
-   ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage, 
refresh);
+   ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path,
+ stage, refresh, NULL);
if (!ce)
return error(_(addinfo_cache failed for path '%s'), path);
return add_cache_entry(ce, options);
diff --git a/read-cache.c b/read-cache.c
index 33dd676..8f16cee 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -15,7 +15,8 @@
 #include strbuf.h
 #include varint.h
 
-static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int 
really);
+static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
+  int really, int* err);
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
@@ -696,7 +697,7 @@ int add_file_to_index(struct index_state *istate, const 
char *path, int flags)
 
 struct cache_entry *make_cache_entry(unsigned int mode,
const unsigned char *sha1, const char *path, int stage,
-   int refresh)
+   int refresh, int* err)
 {
int size, len;
struct

Re: [PATCH/RFC 1/3] t3030-merge-recursive: Test known breakage with empty work tree

[Brad King]

On 01/24/2014 11:51 AM, Jonathan Nieder wrote:
 a quick summary of the symptoms and when it came up?

You're suggested commit message correctly explains it:

 Do you mean something like the following?
 
   Sometimes when working with a large repository it can be useful to
   try out a merge and only check out conflicting files to disk (for
   example as a speed optimization on a server).  Until v1.7.7-rc1~28^2~20
   (merge-recursive: When we detect we can skip an update, actually
   skip it, 2011-08-11), it was possible to do so with the following
   idiom:
 
   ... summary of commands here ...
 
   Nowadays, that still works and the exit status is the same,
   but merge-recursive produces a diagnostic if our side renamed
   a file:
 
   error: addinfo_cache failed for path 'dst'
 
   Add a test to document this regression.

Yes, thanks.

 Elsewhere in the test, commands in a subshell are indented by another
 tab, so these new tests should probably follow suit.

Great.  I'll fold both of the above into the next revision of the series.

Thanks,
-Brad

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 3/3] merge-recursive: Tolerate missing file when HEAD is up to date

[Brad King]

On 01/24/2014 01:42 PM, Elijah Newren wrote:
 While this change does work for the particular new testcase you provided,
 there's a more complex case where merge-recursive is failing

I'm not surprised.  The change felt much like covering a symptom.

 it's just that we want the stat information refreshed if and only
 if the file happens to exist in the working tree.

We can add a refresh_flags argument to make_cache_entry to request
this behavior.  I'll send an updated series soon.

Thanks,
-Brad

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 3/3] merge-recursive: Tolerate missing file when HEAD is up to date

[Brad King]

On Fri, Jan 24, 2014 at 2:50 PM, Junio C Hamano gits...@pobox.com wrote:
 It somehow feels wrong to force callers of make_cache_entry() to be
 so intimate with the implementation details of refresh_cache_ent()
[snip]
 option bit CE_MATCH_MISSING_OK that asks it to treat a path that is
 missing from the working tree as if it is checked out unmodified.

I came to the same conclusion after reading Elijah's last response.
My next series revision adds an argument to make_cache_entry to
specify the refresh flags and honors REFRESH_IGNORE_MISSING.

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] merge-recursive.c: Tolerate missing files while refreshing index

[Brad King]

Teach add_cacheinfo to tell make_cache_entry to skip refreshing stat
information when a file is missing from the work tree.  We do not want
the index to be stat-dirty after the merge but also do not want to fail
when a file happens to be missing.

This fixes the 'merge-recursive w/ empty work tree - ours has rename'
case in t3030-merge-recursive.

Suggested-by: Elijah Newren new...@gmail.com
Signed-off-by: Brad King brad.k...@kitware.com
---
 merge-recursive.c  | 3 ++-
 t/t3030-merge-recursive.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index a6fe7f9..35935c5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -201,7 +201,8 @@ static int add_cacheinfo(unsigned int mode, const unsigned 
char *sha1,
const char *path, int stage, int refresh, int options)
 {
struct cache_entry *ce;
-   ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage, 
refresh, 0);
+   ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage,
+ refresh, REFRESH_IGNORE_MISSING);
if (!ce)
return error(_(addinfo_cache failed for path '%s'), path);
return add_cache_entry(ce, options);
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 3db3bf6..82e1854 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -518,7 +518,7 @@ test_expect_success 'reset and bind merge' '
 
 '
 
-test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
(
GIT_WORK_TREE=$PWD/ours-has-rename-work 
export GIT_WORK_TREE 
-- 
1.8.5.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/3] read-cache.c: Optionally tolerate missing files in make_cache_entry

[Brad King]

Add an 'int refresh_flags' argument to make_cache_entry to tell the
refresh step about caller preferences.  Teach it to honor the
REFRESH_IGNORE_MISSING flag to skip refreshing stat information when a
file is missing from the work tree on disk.

Signed-off-by: Brad King brad.k...@kitware.com
---
 builtin/apply.c|  2 +-
 builtin/checkout.c |  2 +-
 builtin/reset.c|  2 +-
 cache.h|  2 +-
 merge-recursive.c  |  2 +-
 read-cache.c   | 21 -
 resolve-undo.c |  2 +-
 7 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index b0d0986..64c04ec 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3675,7 +3675,7 @@ static void build_fake_ancestor(struct patch *list, const 
char *filename)
die(sha1 information is lacking or useless 
(%s)., name);
 
-   ce = make_cache_entry(patch-old_mode, sha1, name, 0, 0);
+   ce = make_cache_entry(patch-old_mode, sha1, name, 0, 0, 0);
if (!ce)
die(_(make_cache_entry failed for path '%s'), name);
if (add_index_entry(result, ce, ADD_CACHE_OK_TO_ADD))
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5df3837..d3d8640 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -208,7 +208,7 @@ static int checkout_merged(int pos, struct checkout *state)
if (write_sha1_file(result_buf.ptr, result_buf.size,
blob_type, sha1))
die(_(Unable to add merge result for '%s'), path);
-   ce = make_cache_entry(mode, sha1, path, 2, 0);
+   ce = make_cache_entry(mode, sha1, path, 2, 0, 0);
if (!ce)
die(_(make_cache_entry failed for path '%s'), path);
status = checkout_entry(ce, state, NULL);
diff --git a/builtin/reset.c b/builtin/reset.c
index 6004803..ac45056 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -122,7 +122,7 @@ static void update_index_from_diff(struct diff_queue_struct 
*q,
if (one-mode  !is_null_sha1(one-sha1)) {
struct cache_entry *ce;
ce = make_cache_entry(one-mode, one-sha1, one-path,
-   0, 0);
+ 0, 0, 0);
if (!ce)
die(_(make_cache_entry failed for path '%s'),
one-path);
diff --git a/cache.h b/cache.h
index c9efe88..653ede4 100644
--- a/cache.h
+++ b/cache.h
@@ -487,7 +487,7 @@ extern int remove_file_from_index(struct index_state *, 
const char *path);
 #define ADD_CACHE_IMPLICIT_DOT 32  /* internal to git add -u/-A */
 extern int add_to_index(struct index_state *, const char *path, struct stat *, 
int flags);
 extern int add_file_to_index(struct index_state *, const char *path, int 
flags);
-extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned 
char *sha1, const char *path, int stage, int refresh);
+extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned 
char *sha1, const char *path, int stage, int refresh, int refresh_flags);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry 
*b);
 extern int index_name_is_other(const struct index_state *, const char *, int);
 extern void *read_blob_data_from_index(struct index_state *, const char *, 
unsigned long *);
diff --git a/merge-recursive.c b/merge-recursive.c
index a18bd15..a6fe7f9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -201,7 +201,7 @@ static int add_cacheinfo(unsigned int mode, const unsigned 
char *sha1,
const char *path, int stage, int refresh, int options)
 {
struct cache_entry *ce;
-   ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage, 
refresh);
+   ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage, 
refresh, 0);
if (!ce)
return error(_(addinfo_cache failed for path '%s'), path);
return add_cache_entry(ce, options);
diff --git a/read-cache.c b/read-cache.c
index 33dd676..9ce7a9f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -15,7 +15,8 @@
 #include strbuf.h
 #include varint.h
 
-static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int 
really);
+static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int 
really,
+  int flags);
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
@@ -696,7 +697,7 @@ int add_file_to_index(struct index_state *istate, const 
char *path, int flags)
 
 struct cache_entry *make_cache_entry(unsigned int mode,
const unsigned char *sha1, const char *path, int stage,
-   int refresh)
+   int refresh, int refresh_flags)
 {
int size, len;
struct cache_entry *ce;
@@ -717,7 +718,7 @@ struct cache_entry

[PATCH v2 0/3] merge-recursive: Avoid diagnostic on empty work tree

[Brad King]

Hi Folks,

Here is the second revision of this series.  The previous
revision can be found at $gmane/241009.

Updates since the previous revision of the series:

* Patch 1 test indentation and commit message updated thanks to
  comments from Jonathan.

* Patch 2 now adds a different new argument to make_cache_entry.
  This one is to request certain refresh behavior instead of just
  to get an error value back.

* Patch 3 uses the new make_cache_entry feature in patch 2
  to fix the test case.  This approach is based on suggestions
  from Elijah and Junio.

Thanks,
-Brad

Brad King (3):
  t3030-merge-recursive: Test known breakage with empty work tree
  read-cache.c: Optionally tolerate missing files in make_cache_entry
  merge-recursive.c: Tolerate missing files while refreshing index

 builtin/apply.c|  2 +-
 builtin/checkout.c |  2 +-
 builtin/reset.c|  2 +-
 cache.h|  2 +-
 merge-recursive.c  |  3 ++-
 read-cache.c   | 21 -
 resolve-undo.c |  2 +-
 t/t3030-merge-recursive.sh | 47 ++
 8 files changed, 70 insertions(+), 11 deletions(-)

-- 
1.8.5.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] t3030-merge-recursive: Test known breakage with empty work tree

[Brad King]

Sometimes when working with a large repository it can be useful to try
out a merge and only check out conflicting files to disk (for example as
a speed optimization on a server).  Until v1.7.7-rc1~28^2~20
(merge-recursive: When we detect we can skip an update, actually skip
it, 2011-08-11), it was possible to do so with the following idiom:

# Prepare a temporary index and empty work tree.
GIT_INDEX_FILE=$PWD/tmp-$$-index 
export GIT_INDEX_FILE 
GIT_WORK_TREE=$PWD/tmp-$$-work 
export GIT_WORK_TREE 
mkdir $GIT_WORK_TREE 

# Convince the index that our side is on disk.
git read-tree -i -m $ours 
git update-index --ignore-missing --refresh 

# Merge their side into our side.
bases=$(git merge-base --all $ours $theirs) 
git merge-recursive $bases -- $ours $theirs 
tree=$(git write-tree)

Nowadays, that still works and the exit status is the same, but
merge-recursive produces a diagnostic if our side renamed a file:

error: addinfo_cache failed for path 'dst'

Add a test to document this regression.

Signed-off-by: Brad King brad.k...@kitware.com
---
 t/t3030-merge-recursive.sh | 47 ++
 1 file changed, 47 insertions(+)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 2f96100..3db3bf6 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -257,6 +257,7 @@ test_expect_success 'setup 8' '
git add e 
test_tick 
git commit -m rename a-e 
+   c7=$(git rev-parse --verify HEAD) 
git checkout rename-ln 
git mv a e 
test_ln_s_add e a 
@@ -517,6 +518,52 @@ test_expect_success 'reset and bind merge' '
 
 '
 
+test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+   (
+   GIT_WORK_TREE=$PWD/ours-has-rename-work 
+   export GIT_WORK_TREE 
+   GIT_INDEX_FILE=$PWD/ours-has-rename-index 
+   export GIT_INDEX_FILE 
+   mkdir $GIT_WORK_TREE 
+   git read-tree -i -m $c7 
+   git update-index --ignore-missing --refresh 
+   git merge-recursive $c0 -- $c7 $c3 
+   git ls-files -s actual-files
+   ) 2actual-err 
+   expected-err 
+   cat expected-files -EOF 
+   100644 $o3 0b/c
+   100644 $o0 0c
+   100644 $o0 0d/e
+   100644 $o0 0e
+   EOF
+   test_cmp expected-files actual-files 
+   test_cmp expected-err actual-err
+'
+
+test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
+   (
+   GIT_WORK_TREE=$PWD/theirs-has-rename-work 
+   export GIT_WORK_TREE 
+   GIT_INDEX_FILE=$PWD/theirs-has-rename-index 
+   export GIT_INDEX_FILE 
+   mkdir $GIT_WORK_TREE 
+   git read-tree -i -m $c3 
+   git update-index --ignore-missing --refresh 
+   git merge-recursive $c0 -- $c3 $c7 
+   git ls-files -s actual-files
+   ) 2actual-err 
+   expected-err 
+   cat expected-files -EOF 
+   100644 $o3 0b/c
+   100644 $o0 0c
+   100644 $o0 0d/e
+   100644 $o0 0e
+   EOF
+   test_cmp expected-files actual-files 
+   test_cmp expected-err actual-err
+'
+
 test_expect_success 'merge removes empty directories' '
 
git reset --hard master 
-- 
1.8.5.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] t3030-merge-recursive: Test known breakage with empty work tree

[Brad King]

Add test cases that use 'merge-recursive' plumbing with a temporary
index and empty work tree.  Populate the index using 'read-tree' and
'update-index --ignore-missing --refresh' to prepare for merge without
actually checking all files out to disk.  Verify that each merge
produces its expected tree while displaying no error diagnostics.

This approach can be used to compute tree merges while checking out only
conflicting files to disk (which is useful for server-side scripts).
Prior to commit 5b448b85 (merge-recursive: When we detect we can skip an
update, actually skip it, 2011-08-11) this worked cleanly in all cases.
Since that commit, merge-recursive displays a diagnostic such as

 error: addinfo_cache failed for path 'e'

when our side has a rename (to 'e').  The diagnostic does not
influence the return code and the merge appears to succeed, but it
causes this test case to fail.

Signed-off-by: Brad King brad.k...@kitware.com
---
 t/t3030-merge-recursive.sh | 47 ++
 1 file changed, 47 insertions(+)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 2f96100..b6d3ed0 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -257,6 +257,7 @@ test_expect_success 'setup 8' '
git add e 
test_tick 
git commit -m rename a-e 
+   c7=$(git rev-parse --verify HEAD) 
git checkout rename-ln 
git mv a e 
test_ln_s_add e a 
@@ -517,6 +518,52 @@ test_expect_success 'reset and bind merge' '
 
 '
 
+test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+   (
+GIT_WORK_TREE=$PWD/ours-has-rename-work 
+export GIT_WORK_TREE 
+GIT_INDEX_FILE=$PWD/ours-has-rename-index 
+export GIT_INDEX_FILE 
+mkdir $GIT_WORK_TREE 
+git read-tree -i -m $c7 
+git update-index --ignore-missing --refresh 
+git merge-recursive $c0 -- $c7 $c3 
+git ls-files -s actual-files
+   ) 2actual-err 
+   expected-err 
+   cat expected-files -EOF 
+   100644 $o3 0b/c
+   100644 $o0 0c
+   100644 $o0 0d/e
+   100644 $o0 0e
+   EOF
+   test_cmp expected-files actual-files 
+   test_cmp expected-err actual-err
+'
+
+test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
+   (
+GIT_WORK_TREE=$PWD/theirs-has-rename-work 
+export GIT_WORK_TREE 
+GIT_INDEX_FILE=$PWD/theirs-has-rename-index 
+export GIT_INDEX_FILE 
+mkdir $GIT_WORK_TREE 
+git read-tree -i -m $c3 
+git update-index --ignore-missing --refresh 
+git merge-recursive $c0 -- $c3 $c7 
+git ls-files -s actual-files
+   ) 2actual-err 
+   expected-err 
+   cat expected-files -EOF 
+   100644 $o3 0b/c
+   100644 $o0 0c
+   100644 $o0 0d/e
+   100644 $o0 0e
+   EOF
+   test_cmp expected-files actual-files 
+   test_cmp expected-err actual-err
+'
+
 test_expect_success 'merge removes empty directories' '
 
git reset --hard master 
-- 
1.8.5.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 7/8] update-ref: support multiple simultaneous updates

[Brad King]

On 09/10/2013 06:51 PM, Eric Sunshine wrote:
 On Mon, Sep 9, 2013 at 8:57 PM, Brad King brad.k...@kitware.com wrote:
 +Use 40 0 or the empty string to specify a zero value, except that
 
 Did you want an 's' after the 0?

The same description without 's' already appears in git-update-ref.txt
above this location in the existing documentation of the command-line
option behavior.  I see 0{40} in git-receive-pack.txt and also in
howto/update-hook-example.txt.  Perhaps a follow-up change can be made
to choose a consistent way to describe 40 0s.

-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6.1 8/8] update-ref: add test cases covering --stdin signature

[Brad King]

Extend t/t1400-update-ref.sh to cover cases using the --stdin option.

Signed-off-by: Brad King brad.k...@kitware.com
---

On 09/10/2013 06:46 PM, Eric Sunshine wrote:
 Thus printf provides all the functionality you require, and
 print_nul() function can be dropped. So:
 
 printf '%s\0' foo bar baz

Wonderful, thanks!  The single-quotes do not fit easily inside
test code blocks that are themselves single-quoted, so I packaged
the format up in a variable.  Here is a revised patch.

-Brad

 t/t1400-update-ref.sh | 632 ++
 1 file changed, 632 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index e415ee0..6ffd82f 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -302,4 +302,636 @@ test_expect_success \
'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER)' \
'test OTHER = $(git cat-file blob master@{2005-05-26 23:42}:F)'
 
+a=refs/heads/a
+b=refs/heads/b
+c=refs/heads/c
+E=''
+F='%s\0'
+pws='path with space'
+
+test_expect_success 'stdin test setup' '
+   echo $pws $pws 
+   git add -- $pws 
+   git commit -m $pws
+'
+
+test_expect_success '-z fails without --stdin' '
+   test_must_fail git update-ref -z $m $m $m 2err 
+   grep usage: git update-ref err
+'
+
+test_expect_success 'stdin works with no input' '
+   stdin 
+   git update-ref --stdin stdin 
+   git rev-parse --verify -q $m
+'
+
+test_expect_success 'stdin fails on empty line' '
+   echo  stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: empty command in input err
+'
+
+test_expect_success 'stdin fails on only whitespace' '
+   echo   stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: whitespace before command:   err
+'
+
+test_expect_success 'stdin fails on leading whitespace' '
+   echo  create $a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: whitespace before command:  create $a $m err
+'
+
+test_expect_success 'stdin fails on unknown command' '
+   echo unknown $a stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: unknown command: unknown $a err
+'
+
+test_expect_success 'stdin fails on badly quoted input' '
+   echo create $a \master stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: badly quoted argument: \\\master err
+'
+
+test_expect_success 'stdin fails on arguments not separated by space' '
+   echo create \$a\master stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: expected SP but got: master err
+'
+
+test_expect_success 'stdin fails create with no ref' '
+   echo create  stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: create line missing ref err
+'
+
+test_expect_success 'stdin fails create with bad ref name' '
+   echo create ~a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid ref format: ~a err
+'
+
+test_expect_success 'stdin fails create with no new value' '
+   echo create $a stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: create $a missing newvalue err
+'
+
+test_expect_success 'stdin fails create with too many arguments' '
+   echo create $a $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: create $a has extra input:  $m err
+'
+
+test_expect_success 'stdin fails update with no ref' '
+   echo update  stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: update line missing ref err
+'
+
+test_expect_success 'stdin fails update with bad ref name' '
+   echo update ~a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid ref format: ~a err
+'
+
+test_expect_success 'stdin fails update with no new value' '
+   echo update $a stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: update $a missing newvalue err
+'
+
+test_expect_success 'stdin fails update with too many arguments' '
+   echo update $a $m $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: update $a has extra input:  $m err
+'
+
+test_expect_success 'stdin fails delete with no ref' '
+   echo delete  stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: delete line missing ref err
+'
+
+test_expect_success 'stdin fails delete with bad ref name' '
+   echo delete ~a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid ref format: ~a err
+'
+
+test_expect_success 'stdin fails delete with too many arguments' '
+   echo delete $a $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: delete $a has extra input:  $m err

Re: [PATCH v6 0/8] Multiple simultaneously locked ref updates

[Brad King]

On 09/10/2013 12:30 PM, Junio C Hamano wrote:
 Thanks.  I am not sure if I should rewind and rebuild the series
 with these patches, though.  This is a new feature and does not have
 to be merged to 'maint', so rebasing is perfectly fine, but it is
 not strictly necessary, either.

I just thought I'd help out with the conflict resolution.  If you're
happy with resolving the conflicts in v5 then there is no reason to
use v6.

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 8/8] update-ref: add test cases covering --stdin signature

[Brad King]

Extend t/t1400-update-ref.sh to cover cases using the --stdin option.

Signed-off-by: Brad King brad.k...@kitware.com
---
 t/t1400-update-ref.sh | 639 ++
 1 file changed, 639 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index e415ee0..a510500 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -302,4 +302,643 @@ test_expect_success \
'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER)' \
'test OTHER = $(git cat-file blob master@{2005-05-26 23:42}:F)'
 
+a=refs/heads/a
+b=refs/heads/b
+c=refs/heads/c
+E=''
+pws='path with space'
+
+print_nul() {
+   while test $# -gt 0; do
+   printf -- $1 
+   printf -- Q | q_to_nul 
+   shift || return
+   done
+}
+
+test_expect_success 'stdin test setup' '
+   echo $pws $pws 
+   git add -- $pws 
+   git commit -m $pws
+'
+
+test_expect_success '-z fails without --stdin' '
+   test_must_fail git update-ref -z $m $m $m 2err 
+   grep usage: git update-ref err
+'
+
+test_expect_success 'stdin works with no input' '
+   stdin 
+   git update-ref --stdin stdin 
+   git rev-parse --verify -q $m
+'
+
+test_expect_success 'stdin fails on empty line' '
+   echo  stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: empty command in input err
+'
+
+test_expect_success 'stdin fails on only whitespace' '
+   echo   stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: whitespace before command:   err
+'
+
+test_expect_success 'stdin fails on leading whitespace' '
+   echo  create $a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: whitespace before command:  create $a $m err
+'
+
+test_expect_success 'stdin fails on unknown command' '
+   echo unknown $a stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: unknown command: unknown $a err
+'
+
+test_expect_success 'stdin fails on badly quoted input' '
+   echo create $a \master stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: badly quoted argument: \\\master err
+'
+
+test_expect_success 'stdin fails on arguments not separated by space' '
+   echo create \$a\master stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: expected SP but got: master err
+'
+
+test_expect_success 'stdin fails create with no ref' '
+   echo create  stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: create line missing ref err
+'
+
+test_expect_success 'stdin fails create with bad ref name' '
+   echo create ~a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid ref format: ~a err
+'
+
+test_expect_success 'stdin fails create with no new value' '
+   echo create $a stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: create $a missing newvalue err
+'
+
+test_expect_success 'stdin fails create with too many arguments' '
+   echo create $a $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: create $a has extra input:  $m err
+'
+
+test_expect_success 'stdin fails update with no ref' '
+   echo update  stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: update line missing ref err
+'
+
+test_expect_success 'stdin fails update with bad ref name' '
+   echo update ~a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid ref format: ~a err
+'
+
+test_expect_success 'stdin fails update with no new value' '
+   echo update $a stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: update $a missing newvalue err
+'
+
+test_expect_success 'stdin fails update with too many arguments' '
+   echo update $a $m $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: update $a has extra input:  $m err
+'
+
+test_expect_success 'stdin fails delete with no ref' '
+   echo delete  stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: delete line missing ref err
+'
+
+test_expect_success 'stdin fails delete with bad ref name' '
+   echo delete ~a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid ref format: ~a err
+'
+
+test_expect_success 'stdin fails delete with too many arguments' '
+   echo delete $a $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: delete $a has extra input:  $m err
+'
+
+test_expect_success 'stdin fails verify with too many arguments' '
+   echo verify $a $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: verify $a has extra input:  $m err

[PATCH v5 0/8] Multiple simultaneously locked ref updates

[Brad King]

Hi Folks,

Here is the fifth revision of a series to support locking multiple
refs at the same time to update all of them consistently.  The
previous revisions of the series can be found at $gmane/233260,
$gmane/233458, $gmane/233647, and $gmane/233840.

Updates since the previous revision of the series:

* Patches 1-6 are identical to v4 so are not re-sent here.

* Patch 7 and 8 now implement and test the input format proposed and
  discussed at $gmane/233990.

-Brad

Brad King (2):
  update-ref: support multiple simultaneous updates
  update-ref: add test cases covering --stdin signature

 Documentation/git-update-ref.txt |  54 +++-
 builtin/update-ref.c | 252 ++-
 t/t1400-update-ref.sh| 639 +++
 3 files changed, 943 insertions(+), 2 deletions(-)

-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 7/8] update-ref: support multiple simultaneous updates

[Brad King]

Add a --stdin signature to read update instructions from standard input
and apply multiple ref updates together.  Use an input format that
supports any update that could be specified via the command-line,
including object names like branch:path with space.

Signed-off-by: Brad King brad.k...@kitware.com
---
 Documentation/git-update-ref.txt |  54 -
 builtin/update-ref.c | 252 ++-
 2 files changed, 304 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 0df13ff..0a0a551 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -8,7 +8,7 @@ git-update-ref - Update the object name stored in a ref safely
 SYNOPSIS
 
 [verse]
-'git update-ref' [-m reason] (-d ref [oldvalue] | [--no-deref] ref 
newvalue [oldvalue])
+'git update-ref' [-m reason] (-d ref [oldvalue] | [--no-deref] ref 
newvalue [oldvalue] | --stdin [-z])
 
 DESCRIPTION
 ---
@@ -58,6 +58,58 @@ archive by creating a symlink tree).
 With `-d` flag, it deletes the named ref after verifying it
 still contains oldvalue.
 
+With `--stdin`, update-ref reads instructions from standard input and
+performs all modifications together.  Specify commands of the form:
+
+   update SP ref SP newvalue [SP oldvalue] LF
+   create SP ref SP newvalue LF
+   delete SP ref [SP oldvalue] LF
+   verify SP ref [SP oldvalue] LF
+   option SP opt LF
+
+Quote fields containing whitespace as if they were strings in C source
+code.  Alternatively, use `-z` to specify commands without quoting:
+
+   update SP ref NUL newvalue NUL [oldvalue] NUL
+   create SP ref NUL newvalue NUL
+   delete SP ref NUL [oldvalue] NUL
+   verify SP ref NUL [oldvalue] NUL
+   option SP opt NUL
+
+Lines of any other format or a repeated ref produce an error.
+Command meanings are:
+
+update::
+   Set ref to newvalue after verifying oldvalue, if given.
+   Specify a zero newvalue to ensure the ref does not exist
+   after the update and/or a zero oldvalue to make sure the
+   ref does not exist before the update.
+
+create::
+   Create ref with newvalue after verifying it does not
+   exist.  The given newvalue may not be zero.
+
+delete::
+   Delete ref after verifying it exists with oldvalue, if
+   given.  If given, oldvalue may not be zero.
+
+verify::
+   Verify ref against oldvalue but do not change it.  If
+   oldvalue zero or missing, the ref must not exist.
+
+option::
+   Modify behavior of the next command naming a ref.
+   The only valid option is `no-deref` to avoid dereferencing
+   a symbolic ref.
+
+Use 40 0 or the empty string to specify a zero value, except that
+with `-z` an empty oldvalue is considered missing.
+
+If all refs can be locked with matching oldvalues
+simultaneously, all modifications are performed.  Otherwise, no
+modifications are performed.  Note that while each individual
+ref is updated or deleted atomically, a concurrent reader may
+still see a subset of the modifications.
 
 Logging Updates
 ---
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 51d2684..894f16b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -2,23 +2,261 @@
 #include refs.h
 #include builtin.h
 #include parse-options.h
+#include quote.h
+#include argv-array.h
 
 static const char * const git_update_ref_usage[] = {
N_(git update-ref [options] -d refname [oldval]),
N_(git update-ref [options]refname newval [oldval]),
+   N_(git update-ref [options] --stdin [-z]),
NULL
 };
 
+static int updates_alloc;
+static int updates_count;
+static const struct ref_update **updates;
+
+static char line_termination = '\n';
+static int update_flags;
+
+static struct ref_update *update_alloc(void)
+{
+   struct ref_update *update;
+
+   /* Allocate and zero-init a struct ref_update */
+   update = xcalloc(1, sizeof(*update));
+   ALLOC_GROW(updates, updates_count + 1, updates_alloc);
+   updates[updates_count++] = update;
+
+   /* Store and reset accumulated options */
+   update-flags = update_flags;
+   update_flags = 0;
+
+   return update;
+}
+
+static void update_store_ref_name(struct ref_update *update,
+ const char *ref_name)
+{
+   if (check_refname_format(ref_name, REFNAME_ALLOW_ONELEVEL))
+   die(invalid ref format: %s, ref_name);
+   update-ref_name = xstrdup(ref_name);
+}
+
+static void update_store_new_sha1(struct ref_update *update,
+ const char *newvalue)
+{
+   if (*newvalue  get_sha1(newvalue, update-new_sha1))
+   die(invalid new value for ref %s: %s,
+   update-ref_name, newvalue);
+}
+
+static void update_store_old_sha1(struct ref_update *update,
+ const char *oldvalue

[PATCH v6 0/8] Multiple simultaneously locked ref updates

[Brad King]

Hi Folks,

Here is the sixth revision of a series to support locking multiple
refs at the same time to update all of them consistently.  The
previous revisions of the series can be found at $gmane/233260,
$gmane/233458, $gmane/233647, $gmane/233840, and $gmane/234324.

Updates since the previous revision of the series:

* The entire series was rebased on master at bb80ee09; it was
  previously based on v1.8.4.

* A conflict in refs.c with 47a59185 was resolved by preserving
  the elimination of find_ref_by_name while adding our new content.

* A conflict in builtin/update-ref.c with d5d09d47 (Replace deprecated
  OPT_BOOLEAN by OPT_BOOL, 2013-08-03) was resolved by integrating
  both changes.  The new options added in patch 7 now use OPT_BOOL.

-Brad

Brad King (8):
  reset: rename update_refs to reset_refs
  refs: report ref type from lock_any_ref_for_update
  refs: factor update_ref steps into helpers
  refs: factor delete_ref loose ref step into a helper
  refs: add function to repack without multiple refs
  refs: add update_refs for multiple simultaneous updates
  update-ref: support multiple simultaneous updates
  update-ref: add test cases covering --stdin signature

 Documentation/git-update-ref.txt |  54 +++-
 branch.c |   2 +-
 builtin/commit.c |   2 +-
 builtin/fetch.c  |   3 +-
 builtin/receive-pack.c   |   3 +-
 builtin/reflog.c |   2 +-
 builtin/replace.c|   2 +-
 builtin/reset.c  |   4 +-
 builtin/tag.c|   2 +-
 builtin/update-ref.c | 252 ++-
 fast-import.c|   2 +-
 refs.c   | 195 ++--
 refs.h   |  22 +-
 sequencer.c  |   3 +-
 t/t1400-update-ref.sh| 639 +++
 15 files changed, 1146 insertions(+), 41 deletions(-)

-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 6/8] refs: add update_refs for multiple simultaneous updates

[Brad King]

Add 'struct ref_update' to encode the information needed to update or
delete a ref (name, new sha1, optional old sha1, no-deref flag).  Add
function 'update_refs' accepting an array of updates to perform.  First
sort the input array to order locks consistently everywhere and reject
multiple updates to the same ref.  Then acquire locks on all refs with
verified old values.  Then update or delete all refs accordingly.  Fail
if any one lock cannot be obtained or any one old value does not match.

Though the refs themselves cannot be modified together in a single
atomic transaction, this function does enable some useful semantics.
For example, a caller may create a new branch starting from the head of
another branch and rewind the original branch at the same time.  This
transfers ownership of commits between branches without risk of losing
commits added to the original branch by a concurrent process, or risk of
a concurrent process creating the new branch first.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c | 100 +
 refs.h |  20 +
 2 files changed, 120 insertions(+)

diff --git a/refs.c b/refs.c
index 4a63404..6383813 100644
--- a/refs.c
+++ b/refs.c
@@ -3237,6 +3237,106 @@ int update_ref(const char *action, const char *refname,
return update_ref_write(action, refname, sha1, lock, onerr);
 }
 
+static int ref_update_compare(const void *r1, const void *r2)
+{
+   const struct ref_update * const *u1 = r1;
+   const struct ref_update * const *u2 = r2;
+   return strcmp((*u1)-ref_name, (*u2)-ref_name);
+}
+
+static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+   enum action_on_err onerr)
+{
+   int i;
+   for (i = 1; i  n; i++)
+   if (!strcmp(updates[i - 1]-ref_name, updates[i]-ref_name)) {
+   const char *str =
+   Multiple updates for ref '%s' not allowed.;
+   switch (onerr) {
+   case MSG_ON_ERR:
+   error(str, updates[i]-ref_name); break;
+   case DIE_ON_ERR:
+   die(str, updates[i]-ref_name); break;
+   case QUIET_ON_ERR:
+   break;
+   }
+   return 1;
+   }
+   return 0;
+}
+
+int update_refs(const char *action, const struct ref_update **updates_orig,
+   int n, enum action_on_err onerr)
+{
+   int ret = 0, delnum = 0, i;
+   struct ref_update **updates;
+   int *types;
+   struct ref_lock **locks;
+   const char **delnames;
+
+   if (!updates_orig || !n)
+   return 0;
+
+   /* Allocate work space */
+   updates = xmalloc(sizeof(*updates) * n);
+   types = xmalloc(sizeof(*types) * n);
+   locks = xcalloc(n, sizeof(*locks));
+   delnames = xmalloc(sizeof(*delnames) * n);
+
+   /* Copy, sort, and reject duplicate refs */
+   memcpy(updates, updates_orig, sizeof(*updates) * n);
+   qsort(updates, n, sizeof(*updates), ref_update_compare);
+   ret = ref_update_reject_duplicates(updates, n, onerr);
+   if (ret)
+   goto cleanup;
+
+   /* Acquire all locks while verifying old values */
+   for (i = 0; i  n; i++) {
+   locks[i] = update_ref_lock(updates[i]-ref_name,
+  (updates[i]-have_old ?
+   updates[i]-old_sha1 : NULL),
+  updates[i]-flags,
+  types[i], onerr);
+   if (!locks[i]) {
+   ret = 1;
+   goto cleanup;
+   }
+   }
+
+   /* Perform updates first so live commits remain referenced */
+   for (i = 0; i  n; i++)
+   if (!is_null_sha1(updates[i]-new_sha1)) {
+   ret = update_ref_write(action,
+  updates[i]-ref_name,
+  updates[i]-new_sha1,
+  locks[i], onerr);
+   locks[i] = NULL; /* freed by update_ref_write */
+   if (ret)
+   goto cleanup;
+   }
+
+   /* Perform deletes now that updates are safely completed */
+   for (i = 0; i  n; i++)
+   if (locks[i]) {
+   delnames[delnum++] = locks[i]-ref_name;
+   ret |= delete_ref_loose(locks[i], types[i]);
+   }
+   ret |= repack_without_refs(delnames, delnum);
+   for (i = 0; i  delnum; i++)
+   unlink_or_warn(git_path(logs/%s, delnames[i]));
+   clear_loose_ref_cache(ref_cache);
+
+cleanup:
+   for (i = 0; i  n; i

[PATCH v6 7/8] update-ref: support multiple simultaneous updates

[Brad King]

Add a --stdin signature to read update instructions from standard input
and apply multiple ref updates together.  Use an input format that
supports any update that could be specified via the command-line,
including object names like branch:path with space.

Signed-off-by: Brad King brad.k...@kitware.com
---
 Documentation/git-update-ref.txt |  54 -
 builtin/update-ref.c | 252 ++-
 2 files changed, 304 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 0df13ff..0a0a551 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -8,7 +8,7 @@ git-update-ref - Update the object name stored in a ref safely
 SYNOPSIS
 
 [verse]
-'git update-ref' [-m reason] (-d ref [oldvalue] | [--no-deref] ref 
newvalue [oldvalue])
+'git update-ref' [-m reason] (-d ref [oldvalue] | [--no-deref] ref 
newvalue [oldvalue] | --stdin [-z])
 
 DESCRIPTION
 ---
@@ -58,6 +58,58 @@ archive by creating a symlink tree).
 With `-d` flag, it deletes the named ref after verifying it
 still contains oldvalue.
 
+With `--stdin`, update-ref reads instructions from standard input and
+performs all modifications together.  Specify commands of the form:
+
+   update SP ref SP newvalue [SP oldvalue] LF
+   create SP ref SP newvalue LF
+   delete SP ref [SP oldvalue] LF
+   verify SP ref [SP oldvalue] LF
+   option SP opt LF
+
+Quote fields containing whitespace as if they were strings in C source
+code.  Alternatively, use `-z` to specify commands without quoting:
+
+   update SP ref NUL newvalue NUL [oldvalue] NUL
+   create SP ref NUL newvalue NUL
+   delete SP ref NUL [oldvalue] NUL
+   verify SP ref NUL [oldvalue] NUL
+   option SP opt NUL
+
+Lines of any other format or a repeated ref produce an error.
+Command meanings are:
+
+update::
+   Set ref to newvalue after verifying oldvalue, if given.
+   Specify a zero newvalue to ensure the ref does not exist
+   after the update and/or a zero oldvalue to make sure the
+   ref does not exist before the update.
+
+create::
+   Create ref with newvalue after verifying it does not
+   exist.  The given newvalue may not be zero.
+
+delete::
+   Delete ref after verifying it exists with oldvalue, if
+   given.  If given, oldvalue may not be zero.
+
+verify::
+   Verify ref against oldvalue but do not change it.  If
+   oldvalue zero or missing, the ref must not exist.
+
+option::
+   Modify behavior of the next command naming a ref.
+   The only valid option is `no-deref` to avoid dereferencing
+   a symbolic ref.
+
+Use 40 0 or the empty string to specify a zero value, except that
+with `-z` an empty oldvalue is considered missing.
+
+If all refs can be locked with matching oldvalues
+simultaneously, all modifications are performed.  Otherwise, no
+modifications are performed.  Note that while each individual
+ref is updated or deleted atomically, a concurrent reader may
+still see a subset of the modifications.
 
 Logging Updates
 ---
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 7484d36..b5479e2 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -2,23 +2,261 @@
 #include refs.h
 #include builtin.h
 #include parse-options.h
+#include quote.h
+#include argv-array.h
 
 static const char * const git_update_ref_usage[] = {
N_(git update-ref [options] -d refname [oldval]),
N_(git update-ref [options]refname newval [oldval]),
+   N_(git update-ref [options] --stdin [-z]),
NULL
 };
 
+static int updates_alloc;
+static int updates_count;
+static const struct ref_update **updates;
+
+static char line_termination = '\n';
+static int update_flags;
+
+static struct ref_update *update_alloc(void)
+{
+   struct ref_update *update;
+
+   /* Allocate and zero-init a struct ref_update */
+   update = xcalloc(1, sizeof(*update));
+   ALLOC_GROW(updates, updates_count + 1, updates_alloc);
+   updates[updates_count++] = update;
+
+   /* Store and reset accumulated options */
+   update-flags = update_flags;
+   update_flags = 0;
+
+   return update;
+}
+
+static void update_store_ref_name(struct ref_update *update,
+ const char *ref_name)
+{
+   if (check_refname_format(ref_name, REFNAME_ALLOW_ONELEVEL))
+   die(invalid ref format: %s, ref_name);
+   update-ref_name = xstrdup(ref_name);
+}
+
+static void update_store_new_sha1(struct ref_update *update,
+ const char *newvalue)
+{
+   if (*newvalue  get_sha1(newvalue, update-new_sha1))
+   die(invalid new value for ref %s: %s,
+   update-ref_name, newvalue);
+}
+
+static void update_store_old_sha1(struct ref_update *update,
+ const char *oldvalue

[PATCH v6 4/8] refs: factor delete_ref loose ref step into a helper

[Brad King]

Factor loose ref deletion into helper function delete_ref_loose to allow
later use elsewhere.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c | 27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index f7d3c09..b14f59b 100644
--- a/refs.c
+++ b/refs.c
@@ -2450,24 +2450,31 @@ static int repack_without_ref(const char *refname)
return commit_packed_refs();
 }
 
+static int delete_ref_loose(struct ref_lock *lock, int flag)
+{
+   if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
+   /* loose */
+   int err, i = strlen(lock-lk-filename) - 5; /* .lock */
+
+   lock-lk-filename[i] = 0;
+   err = unlink_or_warn(lock-lk-filename);
+   lock-lk-filename[i] = '.';
+   if (err  errno != ENOENT)
+   return 1;
+   }
+   return 0;
+}
+
 int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 {
struct ref_lock *lock;
-   int err, i = 0, ret = 0, flag = 0;
+   int ret = 0, flag = 0;
 
lock = lock_ref_sha1_basic(refname, sha1, delopt, flag);
if (!lock)
return 1;
-   if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
-   /* loose */
-   i = strlen(lock-lk-filename) - 5; /* .lock */
-   lock-lk-filename[i] = 0;
-   err = unlink_or_warn(lock-lk-filename);
-   if (err  errno != ENOENT)
-   ret = 1;
+   ret |= delete_ref_loose(lock, flag);
 
-   lock-lk-filename[i] = '.';
-   }
/* removing the loose one could have resurrected an earlier
 * packed one.  Also, if it was not loose we need to repack
 * without it.
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 3/8] refs: factor update_ref steps into helpers

[Brad King]

Factor the lock and write steps and error handling into helper functions
update_ref_lock and update_ref_write to allow later use elsewhere.
Expose lock_any_ref_for_update's type_p to update_ref_lock callers.

While at it, drop static from the local lock variable as it is not
necessary to keep across invocations.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c | 30 --
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 5832a8f..f7d3c09 100644
--- a/refs.c
+++ b/refs.c
@@ -3170,12 +3170,13 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
 }
 
-int update_ref(const char *action, const char *refname,
-   const unsigned char *sha1, const unsigned char *oldval,
-   int flags, enum action_on_err onerr)
+static struct ref_lock *update_ref_lock(const char *refname,
+   const unsigned char *oldval,
+   int flags, int *type_p,
+   enum action_on_err onerr)
 {
-   static struct ref_lock *lock;
-   lock = lock_any_ref_for_update(refname, oldval, flags, NULL);
+   struct ref_lock *lock;
+   lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
if (!lock) {
const char *str = Cannot lock the ref '%s'.;
switch (onerr) {
@@ -3183,8 +3184,14 @@ int update_ref(const char *action, const char *refname,
case DIE_ON_ERR: die(str, refname); break;
case QUIET_ON_ERR: break;
}
-   return 1;
}
+   return lock;
+}
+
+static int update_ref_write(const char *action, const char *refname,
+   const unsigned char *sha1, struct ref_lock *lock,
+   enum action_on_err onerr)
+{
if (write_ref_sha1(lock, sha1, action)  0) {
const char *str = Cannot update the ref '%s'.;
switch (onerr) {
@@ -3197,6 +3204,17 @@ int update_ref(const char *action, const char *refname,
return 0;
 }
 
+int update_ref(const char *action, const char *refname,
+  const unsigned char *sha1, const unsigned char *oldval,
+  int flags, enum action_on_err onerr)
+{
+   struct ref_lock *lock;
+   lock = update_ref_lock(refname, oldval, flags, 0, onerr);
+   if (!lock)
+   return 1;
+   return update_ref_write(action, refname, sha1, lock, onerr);
+}
+
 /*
  * generate a format suitable for scanf from a ref_rev_parse_rules
  * rule, that is replace the %.*s spec with a %s spec
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 5/8] refs: add function to repack without multiple refs

[Brad King]

Generalize repack_without_ref as repack_without_refs to support a list
of refs and implement the former in terms of the latter.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c | 33 -
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index b14f59b..4a63404 100644
--- a/refs.c
+++ b/refs.c
@@ -2414,42 +2414,57 @@ static int curate_packed_ref_fn(struct ref_entry 
*entry, void *cb_data)
return 0;
 }
 
-static int repack_without_ref(const char *refname)
+static int repack_without_refs(const char **refnames, int n)
 {
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
+   int i, removed = 0;
+
+   /* Look for a packed ref */
+   for (i = 0; i  n; i++)
+   if (get_packed_ref(refnames[i]))
+   break;
 
-   if (!get_packed_ref(refname))
-   return 0; /* refname does not exist in packed refs */
+   /* Avoid locking if we have nothing to do */
+   if (i == n)
+   return 0; /* no refname exists in packed refs */
 
if (lock_packed_refs(0)) {
unable_to_lock_error(git_path(packed-refs), errno);
-   return error(cannot delete '%s' from packed refs, refname);
+   return error(cannot delete '%s' from packed refs, 
refnames[i]);
}
packed = get_packed_refs(ref_cache);
 
-   /* Remove refname from the cache: */
-   if (remove_entry(packed, refname) == -1) {
+   /* Remove refnames from the cache */
+   for (i = 0; i  n; i++)
+   if (remove_entry(packed, refnames[i]) != -1)
+   removed = 1;
+   if (!removed) {
/*
-* The packed entry disappeared while we were
+* All packed entries disappeared while we were
 * acquiring the lock.
 */
rollback_packed_refs();
return 0;
}
 
-   /* Remove any other accumulated cruft: */
+   /* Remove any other accumulated cruft */
do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, 
refs_to_delete);
for_each_string_list_item(ref_to_delete, refs_to_delete) {
if (remove_entry(packed, ref_to_delete-string) == -1)
die(internal error);
}
 
-   /* Write what remains: */
+   /* Write what remains */
return commit_packed_refs();
 }
 
+static int repack_without_ref(const char *refname)
+{
+   return repack_without_refs(refname, 1);
+}
+
 static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 8/8] update-ref: add test cases covering --stdin signature

[Brad King]

Extend t/t1400-update-ref.sh to cover cases using the --stdin option.

Signed-off-by: Brad King brad.k...@kitware.com
---
 t/t1400-update-ref.sh | 639 ++
 1 file changed, 639 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index e415ee0..a510500 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -302,4 +302,643 @@ test_expect_success \
'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER)' \
'test OTHER = $(git cat-file blob master@{2005-05-26 23:42}:F)'
 
+a=refs/heads/a
+b=refs/heads/b
+c=refs/heads/c
+E=''
+pws='path with space'
+
+print_nul() {
+   while test $# -gt 0; do
+   printf -- $1 
+   printf -- Q | q_to_nul 
+   shift || return
+   done
+}
+
+test_expect_success 'stdin test setup' '
+   echo $pws $pws 
+   git add -- $pws 
+   git commit -m $pws
+'
+
+test_expect_success '-z fails without --stdin' '
+   test_must_fail git update-ref -z $m $m $m 2err 
+   grep usage: git update-ref err
+'
+
+test_expect_success 'stdin works with no input' '
+   stdin 
+   git update-ref --stdin stdin 
+   git rev-parse --verify -q $m
+'
+
+test_expect_success 'stdin fails on empty line' '
+   echo  stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: empty command in input err
+'
+
+test_expect_success 'stdin fails on only whitespace' '
+   echo   stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: whitespace before command:   err
+'
+
+test_expect_success 'stdin fails on leading whitespace' '
+   echo  create $a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: whitespace before command:  create $a $m err
+'
+
+test_expect_success 'stdin fails on unknown command' '
+   echo unknown $a stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: unknown command: unknown $a err
+'
+
+test_expect_success 'stdin fails on badly quoted input' '
+   echo create $a \master stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: badly quoted argument: \\\master err
+'
+
+test_expect_success 'stdin fails on arguments not separated by space' '
+   echo create \$a\master stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: expected SP but got: master err
+'
+
+test_expect_success 'stdin fails create with no ref' '
+   echo create  stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: create line missing ref err
+'
+
+test_expect_success 'stdin fails create with bad ref name' '
+   echo create ~a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid ref format: ~a err
+'
+
+test_expect_success 'stdin fails create with no new value' '
+   echo create $a stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: create $a missing newvalue err
+'
+
+test_expect_success 'stdin fails create with too many arguments' '
+   echo create $a $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: create $a has extra input:  $m err
+'
+
+test_expect_success 'stdin fails update with no ref' '
+   echo update  stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: update line missing ref err
+'
+
+test_expect_success 'stdin fails update with bad ref name' '
+   echo update ~a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid ref format: ~a err
+'
+
+test_expect_success 'stdin fails update with no new value' '
+   echo update $a stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: update $a missing newvalue err
+'
+
+test_expect_success 'stdin fails update with too many arguments' '
+   echo update $a $m $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: update $a has extra input:  $m err
+'
+
+test_expect_success 'stdin fails delete with no ref' '
+   echo delete  stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: delete line missing ref err
+'
+
+test_expect_success 'stdin fails delete with bad ref name' '
+   echo delete ~a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid ref format: ~a err
+'
+
+test_expect_success 'stdin fails delete with too many arguments' '
+   echo delete $a $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: delete $a has extra input:  $m err
+'
+
+test_expect_success 'stdin fails verify with too many arguments' '
+   echo verify $a $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: verify $a has extra input:  $m err

Re: [PATCH v4 7/8] update-ref: support multiple simultaneous updates

[Brad King]

On 09/04/2013 05:27 PM, Junio C Hamano wrote:
 I am not saying the above is the best format, but the point is that
 the mode of the operation defines the structure

Great, thanks for your comments.  Based on that I've prototyped a
new format.  Rather than jumping straight to the patch, here is my
proposed format documentation for review:

-
With `--stdin`, update-ref reads instructions from standard input and
performs all modifications together.  Specify commands of the form:

create SP ref SP newvalue LF
update SP ref SP newvalue [SP oldvalue] LF
delete SP ref [SP oldvalue] LF
verify SP ref [SP oldvalue] LF
option SP opt LF

Quote fields containing whitespace as if they were strings in C source
code.  Alternatively, use `-z` to specify commands without quoting:

create SP ref NUL newvalue NUL
update SP ref NUL newvalue NUL [oldvalue] NUL
delete SP ref NUL [oldvalue] NUL
verify SP ref NUL [oldvalue] NUL
option SP opt NUL

Lines of any other format or a repeated ref produce an error.
Command meanings are:

create::
Create ref with newvalue only if it does not exist.

update::
Update ref to be newvalue, verifying oldvalue if given.
Specify a zero newvalue to delete a ref and/or a zero
oldvalue to make sure that a ref does not exist.

delete::
Delete ref, verifying oldvalue if given.

verify::
Verify ref against oldvalue but do not change it.  If
oldvalue zero or missing, the ref must not exist.

option::
Specify an option to take effect for following commands.
Valid options are `deref` and `no-deref` to specify whether
to dereference symbolic refs.

Use 40 0 or the empty string to specify a zero value, except that
with `-z` an empty oldvalue is considered missing.

If all refs can be locked with matching oldvalues
simultaneously, all modifications are performed.  Otherwise, no
modifications are performed.  Note that while each individual
ref is updated or deleted atomically, a concurrent reader may
still see a subset of the modifications.
-

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 7/8] update-ref: support multiple simultaneous updates

[Brad King]

On 9/5/2013 5:23 PM, Junio C Hamano wrote:
 Brad King brad.k...@kitware.com writes:
  create SP ref NUL newvalue NUL
  update SP ref NUL newvalue NUL [oldvalue] NUL
 
 That SP in '-z' format looks strange.  Was there a reason why NUL
 was inappropriate?

The precedent I saw in the -z survey I posted is that NUL is used
to terminate fields that can contain arbitrary text but otherwise
SP or TAB is still used to terminate simple fields.  I recall no
cases that use NUL after fields that only contain simple values.

 option::
  Specify an option to take effect for following commands.
 
 This last one is somewhat peculiar, especially because it says
 following command*s*.
 
 How would I request to update refs HEAD and next in an all-or-none
 fashion, while applying 'no-deref' only to HEAD but not next?

You're right.  It will be simpler for clients to generate each
sequence of options followed by a ref command without worrying
about options possibly generated for preceding refs.  So:

option::
Modify behavior of the next command naming a ref.
The only valid option is `no-deref` to avoid dereferencing
a symbolic ref.

 When I said create or update in the message you are responding to,
 I did not mean that we should have two separate commands.

The nice thing about create is that it implies oldvalue=zero, just
as delete implies newvalue=zero.  The symmetry feels clean.  Also,
in -z mode we need to treat an empty string oldvalue as missing
rather than zero.  The only way to specify create with the update
command is with literal 40 0 as oldvalue.

 The regular command line does create-or-update; if it exists already,
 it is an update, and if it does not, it is a create.

The proposed update command can be used for create, update, delete,
or verify with proper arguments and use of 40 0.  The other ref
commands are for convenience, shorter input, and statement of intent.

   create-or-update-no-deref ref newvalue [oldvalue]
 delete-no-deref ref [oldvalue]
 
 Also how would one set the reflog message for the proposed update?

This is why I proposed a separate option command.  It can be used
both for no-deref and for other options we want to add later e.g.

  option SP message SP reason LF

and with -z:

  option SP message SP reason NUL

For now I think it is sufficient for the -m reason command-line
option to set the same message for all updates.  The option command
leaves room to add a per-ref message later.

BTW, the reasons I propose message as an option rather than its own
command are:

* It is simpler to document the reach of the one option command
  (affects next ref) in one place rather than separately for
  each option-like command.

* It reduces the number of commands that do not take a ref.

-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 8/8] update-ref: add test cases covering --stdin signature

[Brad King]

Extend t/t1400-update-ref.sh to cover cases using the --stdin option.

Signed-off-by: Brad King brad.k...@kitware.com
---
 t/t1400-update-ref.sh | 445 ++
 1 file changed, 445 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index e415ee0..e8ba0d2 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -302,4 +302,449 @@ test_expect_success \
'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER)' \
'test OTHER = $(git cat-file blob master@{2005-05-26 23:42}:F)'
 
+a=refs/heads/a
+b=refs/heads/b
+c=refs/heads/c
+E=''
+pws='path with space'
+
+print_nul() {
+   while test $# -gt 0; do
+   printf -- $1 
+   printf -- Q | q_to_nul 
+   shift || return
+   done
+}
+
+test_expect_success '-z fails without --stdin' '
+   test_must_fail git update-ref -z $m $m $m 2err 
+   grep usage: git update-ref err
+'
+
+test_expect_success 'stdin test setup' '
+   echo $pws $pws 
+   git add -- $pws 
+   git commit -m $pws
+'
+
+test_expect_success 'stdin works with no input' '
+   stdin 
+   git update-ref --stdin stdin 
+   git rev-parse --verify -q $m
+'
+
+test_expect_success 'stdin works with whitespace-only input' '
+   echo   stdin 
+   git update-ref --stdin stdin 2err 
+   git rev-parse --verify -q $m
+'
+
+test_expect_success 'stdin fails on bad input line with only --' '
+   echo -- stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: input line with no ref err
+'
+
+test_expect_success 'stdin fails on bad input line with only --bad-option' '
+   echo --bad-option stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: unknown option --bad-option err
+'
+
+test_expect_success 'stdin fails on bad ref name' '
+   echo ~a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid ref format: ~a err
+'
+
+test_expect_success 'stdin fails on badly quoted input' '
+   echo $a \master stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: badly quoted argument: \\\master err
+'
+
+test_expect_success 'stdin fails on bad input line with too many arguments' '
+   echo $a $m $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: too many arguments for ref $a err
+'
+
+test_expect_success 'stdin fails on bad input line with too few arguments' '
+   echo $a stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: missing new value for ref $a err
+'
+
+test_expect_success 'stdin fails with duplicate refs' '
+   cat stdin -EOF 
+   $a $m
+   $b $m
+   $a $m
+   EOF
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: Multiple updates for ref '''$a''' not allowed. err
+'
+
+test_expect_success 'stdin create ref works with no old value' '
+   echo $a $m stdin 
+   git update-ref --stdin stdin 
+   git rev-parse $m expect 
+   git rev-parse $a actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'stdin create ref works with zero old value' '
+   echo $b $m $Z stdin 
+   git update-ref --stdin stdin 
+   git rev-parse $m expect 
+   git rev-parse $b actual 
+   test_cmp expect actual 
+   git update-ref -d $b
+'
+
+test_expect_success 'stdin create ref works with empty old value' '
+   echo $b $m $E stdin 
+   git update-ref --stdin stdin 
+   git rev-parse $m expect 
+   git rev-parse $b actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'stdin create ref works with path with space to blob' '
+   echo refs/blobs/pws \$m:$pws\ stdin 
+   git update-ref --stdin stdin 
+   git rev-parse $m:$pws expect 
+   git rev-parse refs/blobs/pws actual 
+   test_cmp expect actual 
+   git update-ref -d refs/blobs/pws
+'
+
+test_expect_success 'stdin create ref fails with wrong old value' '
+   echo $c $m $m~1 stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: Cannot lock the ref '''$c''' err 
+   test_must_fail git rev-parse --verify -q $c
+'
+
+test_expect_success 'stdin create ref fails with bad old value' '
+   echo $c $m does-not-exist stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid old value for ref $c: does-not-exist err 
+   test_must_fail git rev-parse --verify -q $c
+'
+
+test_expect_success 'stdin create ref fails with bad new value' '
+   echo $c does-not-exist stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid new value for ref $c: does-not-exist err 
+   test_must_fail git rev-parse --verify -q $c
+'
+
+test_expect_success 'stdin update ref works with right old value' '
+   echo $b $m~1 $m stdin 
+   git update-ref --stdin stdin

[PATCH v4 5/8] refs: add function to repack without multiple refs

[Brad King]

Generalize repack_without_ref as repack_without_refs to support a list
of refs and implement the former in terms of the latter.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c | 33 -
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index ab9d22e..92d801c 100644
--- a/refs.c
+++ b/refs.c
@@ -2414,42 +2414,57 @@ static int curate_packed_ref_fn(struct ref_entry 
*entry, void *cb_data)
return 0;
 }
 
-static int repack_without_ref(const char *refname)
+static int repack_without_refs(const char **refnames, int n)
 {
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
+   int i, removed = 0;
+
+   /* Look for a packed ref */
+   for (i = 0; i  n; i++)
+   if (get_packed_ref(refnames[i]))
+   break;
 
-   if (!get_packed_ref(refname))
-   return 0; /* refname does not exist in packed refs */
+   /* Avoid locking if we have nothing to do */
+   if (i == n)
+   return 0; /* no refname exists in packed refs */
 
if (lock_packed_refs(0)) {
unable_to_lock_error(git_path(packed-refs), errno);
-   return error(cannot delete '%s' from packed refs, refname);
+   return error(cannot delete '%s' from packed refs, 
refnames[i]);
}
packed = get_packed_refs(ref_cache);
 
-   /* Remove refname from the cache: */
-   if (remove_entry(packed, refname) == -1) {
+   /* Remove refnames from the cache */
+   for (i = 0; i  n; i++)
+   if (remove_entry(packed, refnames[i]) != -1)
+   removed = 1;
+   if (!removed) {
/*
-* The packed entry disappeared while we were
+* All packed entries disappeared while we were
 * acquiring the lock.
 */
rollback_packed_refs();
return 0;
}
 
-   /* Remove any other accumulated cruft: */
+   /* Remove any other accumulated cruft */
do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, 
refs_to_delete);
for_each_string_list_item(ref_to_delete, refs_to_delete) {
if (remove_entry(packed, ref_to_delete-string) == -1)
die(internal error);
}
 
-   /* Write what remains: */
+   /* Write what remains */
return commit_packed_refs();
 }
 
+static int repack_without_ref(const char *refname)
+{
+   return repack_without_refs(refname, 1);
+}
+
 static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 6/8] refs: add update_refs for multiple simultaneous updates

[Brad King]

Add 'struct ref_update' to encode the information needed to update or
delete a ref (name, new sha1, optional old sha1, no-deref flag).  Add
function 'update_refs' accepting an array of updates to perform.  First
sort the input array to order locks consistently everywhere and reject
multiple updates to the same ref.  Then acquire locks on all refs with
verified old values.  Then update or delete all refs accordingly.  Fail
if any one lock cannot be obtained or any one old value does not match.

Though the refs themselves cannot be modified together in a single
atomic transaction, this function does enable some useful semantics.
For example, a caller may create a new branch starting from the head of
another branch and rewind the original branch at the same time.  This
transfers ownership of commits between branches without risk of losing
commits added to the original branch by a concurrent process, or risk of
a concurrent process creating the new branch first.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c | 100 +
 refs.h |  20 +
 2 files changed, 120 insertions(+)

diff --git a/refs.c b/refs.c
index 92d801c..46177ad 100644
--- a/refs.c
+++ b/refs.c
@@ -3237,6 +3237,106 @@ int update_ref(const char *action, const char *refname,
return update_ref_write(action, refname, sha1, lock, onerr);
 }
 
+static int ref_update_compare(const void *r1, const void *r2)
+{
+   const struct ref_update * const *u1 = r1;
+   const struct ref_update * const *u2 = r2;
+   return strcmp((*u1)-ref_name, (*u2)-ref_name);
+}
+
+static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+   enum action_on_err onerr)
+{
+   int i;
+   for (i = 1; i  n; i++)
+   if (!strcmp(updates[i - 1]-ref_name, updates[i]-ref_name)) {
+   const char *str =
+   Multiple updates for ref '%s' not allowed.;
+   switch (onerr) {
+   case MSG_ON_ERR:
+   error(str, updates[i]-ref_name); break;
+   case DIE_ON_ERR:
+   die(str, updates[i]-ref_name); break;
+   case QUIET_ON_ERR:
+   break;
+   }
+   return 1;
+   }
+   return 0;
+}
+
+int update_refs(const char *action, const struct ref_update **updates_orig,
+   int n, enum action_on_err onerr)
+{
+   int ret = 0, delnum = 0, i;
+   struct ref_update **updates;
+   int *types;
+   struct ref_lock **locks;
+   const char **delnames;
+
+   if (!updates_orig || !n)
+   return 0;
+
+   /* Allocate work space */
+   updates = xmalloc(sizeof(*updates) * n);
+   types = xmalloc(sizeof(*types) * n);
+   locks = xcalloc(n, sizeof(*locks));
+   delnames = xmalloc(sizeof(*delnames) * n);
+
+   /* Copy, sort, and reject duplicate refs */
+   memcpy(updates, updates_orig, sizeof(*updates) * n);
+   qsort(updates, n, sizeof(*updates), ref_update_compare);
+   ret = ref_update_reject_duplicates(updates, n, onerr);
+   if (ret)
+   goto cleanup;
+
+   /* Acquire all locks while verifying old values */
+   for (i = 0; i  n; i++) {
+   locks[i] = update_ref_lock(updates[i]-ref_name,
+  (updates[i]-have_old ?
+   updates[i]-old_sha1 : NULL),
+  updates[i]-flags,
+  types[i], onerr);
+   if (!locks[i]) {
+   ret = 1;
+   goto cleanup;
+   }
+   }
+
+   /* Perform updates first so live commits remain referenced */
+   for (i = 0; i  n; i++)
+   if (!is_null_sha1(updates[i]-new_sha1)) {
+   ret = update_ref_write(action,
+  updates[i]-ref_name,
+  updates[i]-new_sha1,
+  locks[i], onerr);
+   locks[i] = NULL; /* freed by update_ref_write */
+   if (ret)
+   goto cleanup;
+   }
+
+   /* Perform deletes now that updates are safely completed */
+   for (i = 0; i  n; i++)
+   if (locks[i]) {
+   delnames[delnum++] = locks[i]-ref_name;
+   ret |= delete_ref_loose(locks[i], types[i]);
+   }
+   ret |= repack_without_refs(delnames, delnum);
+   for (i = 0; i  delnum; i++)
+   unlink_or_warn(git_path(logs/%s, delnames[i]));
+   clear_loose_ref_cache(ref_cache);
+
+cleanup:
+   for (i = 0; i  n; i

[PATCH v4 7/8] update-ref: support multiple simultaneous updates

[Brad King]

Add a --stdin signature to read update instructions from standard input
and apply multiple ref updates together.  Use an input format that
supports any update that could be specified via the command-line,
including object names like branch:path with space.

Signed-off-by: Brad King brad.k...@kitware.com
---
 Documentation/git-update-ref.txt |  22 +-
 builtin/update-ref.c | 144 ++-
 2 files changed, 164 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 0df13ff..11dd9d3 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -8,7 +8,7 @@ git-update-ref - Update the object name stored in a ref safely
 SYNOPSIS
 
 [verse]
-'git update-ref' [-m reason] (-d ref [oldvalue] | [--no-deref] ref 
newvalue [oldvalue])
+'git update-ref' [-m reason] (-d ref [oldvalue] | [--no-deref] ref 
newvalue [oldvalue] | --stdin)
 
 DESCRIPTION
 ---
@@ -58,6 +58,26 @@ archive by creating a symlink tree).
 With `-d` flag, it deletes the named ref after verifying it
 still contains oldvalue.
 
+With `--stdin`, update-ref reads instructions from standard input and
+performs all modifications together.  Empty lines are ignored.
+Each non-empty line is parsed as whitespace-separated arguments.
+Quote arguments containing whitespace as if in C source code.
+Specify updates with lines of the form:
+
+   [--no-deref] [--] ref newvalue [oldvalue]
+
+Lines of any other format or a repeated ref produce an error.
+Specify a zero newvalue to delete a ref and/or a zero oldvalue
+to make sure that a ref not exist.  Use either 40 0 or the
+empty string (written as ) to specify a zero value.  Use `-z`
+to specify input with no whitespace, quoting, or escaping, and
+terminate each argument by NUL and each line by LF NUL.
+
+If all refs can be locked with matching oldvalues
+simultaneously, all modifications are performed.  Otherwise, no
+modifications are performed.  Note that while each individual
+ref is updated or deleted atomically, a concurrent reader may
+still see a subset of the modifications.
 
 Logging Updates
 ---
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 51d2684..9c348fb 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -2,23 +2,152 @@
 #include refs.h
 #include builtin.h
 #include parse-options.h
+#include quote.h
+#include argv-array.h
 
 static const char * const git_update_ref_usage[] = {
N_(git update-ref [options] -d refname [oldval]),
N_(git update-ref [options]refname newval [oldval]),
+   N_(git update-ref [options] --stdin [-z]),
NULL
 };
 
+static int updates_alloc;
+static int updates_count;
+static const struct ref_update **updates;
+
+static void update_refs_stdin(int argc, const char **argv)
+{
+   struct ref_update *update;
+
+   /* Skip blank lines */
+   if (!argc)
+   return;
+
+   /* Allocate and zero-init a struct ref_update */
+   update = xcalloc(1, sizeof(*update));
+   ALLOC_GROW(updates, updates_count+1, updates_alloc);
+   updates[updates_count++] = update;
+
+   /* Process options */
+   while (argc  0  argv[0][0] == '-') {
+   const char *arg = argv[0];
+   --argc;
+   ++argv;
+   if (!strcmp(arg, --no-deref))
+   update-flags |= REF_NODEREF;
+   else if (!strcmp(arg, --))
+   break;
+   else
+   die(unknown option %s, arg);
+   }
+
+   /* Set the update ref_name */
+   if (argc  1)
+   die(input line with no ref!);
+   if (check_refname_format(argv[0], REFNAME_ALLOW_ONELEVEL))
+   die(invalid ref format: %s, argv[0]);
+   update-ref_name = xstrdup(argv[0]);
+
+   /* Set the update new_sha1 and, if specified, old_sha1 */
+   if (argc  2)
+   die(missing new value for ref %s, update-ref_name);
+   if (*argv[1]  get_sha1(argv[1], update-new_sha1))
+   die(invalid new value for ref %s: %s,
+   update-ref_name, argv[1]);
+   if (argc = 3) {
+   update-have_old = 1;
+   if (*argv[2]  get_sha1(argv[2], update-old_sha1))
+   die(invalid old value for ref %s: %s,
+   update-ref_name, argv[2]);
+   }
+
+   if (argc  3)
+   die(too many arguments for ref %s, update-ref_name);
+}
+
+static const char *update_refs_stdin_parse_arg(const char *next,
+  struct strbuf *arg)
+{
+   /* Skip leading whitespace */
+   while (isspace(*next))
+   ++next;
+
+   /* Return NULL when no argument is found */
+   if (!*next)
+   return NULL;
+
+   /* Parse the argument */
+   strbuf_reset(arg);
+   if (*next

[PATCH v4 1/8] reset: rename update_refs to reset_refs

[Brad King]

The function resets refs rather than doing arbitrary updates.
Rename it to allow a future general-purpose update_refs function
to be added.

Signed-off-by: Brad King brad.k...@kitware.com
---
 builtin/reset.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index afa6e02..789ee48 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -219,7 +219,7 @@ static const char **parse_args(const char **argv, const 
char *prefix, const char
return argv[0] ? get_pathspec(prefix, argv) : NULL;
 }
 
-static int update_refs(const char *rev, const unsigned char *sha1)
+static int reset_refs(const char *rev, const unsigned char *sha1)
 {
int update_ref_status;
struct strbuf msg = STRBUF_INIT;
@@ -350,7 +350,7 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
if (!pathspec  !unborn) {
/* Any resets without paths update HEAD to the head being
 * switched to, saving the previous head in ORIG_HEAD before. */
-   update_ref_status = update_refs(rev, sha1);
+   update_ref_status = reset_refs(rev, sha1);
 
if (reset_type == HARD  !update_ref_status  !quiet)
print_new_head_line(lookup_commit_reference(sha1));
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 0/8] Multiple simultaneously locked ref updates

[Brad King]

Hi Folks,

Here is the fourth revision of a series to support locking multiple
refs at the same time to update all of them consistently.  The
previous revisions of the series can be found at $gmane/233260,
$gmane/233458, and $gmane/233647.

Updates since the previous revision of the series:

* Patches 1-4 are identical

* Patches 5-7 no longer have : at the end of comments, a style I
  learned from the context of patch 5 but that I saw Junio squash
  out of patch 5 v2 when he queued it as 53237ae4.

* Patch 7 has a re-organized parser and now defines a -z format for
  stdin that terminates arguments with NUL and lines with LF NUL:

   ... ref NUL newvalue NUL [ oldvalue NUL ] LF NUL

* Patch 8 now has test cases for -z mode and updated error cases for
  the re-organized parser.

Note to maintainer:

* Patch 4 needs to be re-queued to replace c7c80f49 due to the
  tweak suggested in $gmane/233521 and made in v3 and kept in v4.

Thanks,
-Brad

Brad King (8):
  reset: rename update_refs to reset_refs
  refs: report ref type from lock_any_ref_for_update
  refs: factor update_ref steps into helpers
  refs: factor delete_ref loose ref step into a helper
  refs: add function to repack without multiple refs
  refs: add update_refs for multiple simultaneous updates
  update-ref: support multiple simultaneous updates
  update-ref: add test cases covering --stdin signature

 Documentation/git-update-ref.txt |  22 +-
 branch.c |   2 +-
 builtin/commit.c |   2 +-
 builtin/fetch.c  |   3 +-
 builtin/receive-pack.c   |   3 +-
 builtin/reflog.c |   2 +-
 builtin/replace.c|   2 +-
 builtin/reset.c  |   4 +-
 builtin/tag.c|   2 +-
 builtin/update-ref.c | 144 -
 fast-import.c|   2 +-
 refs.c   | 195 ++---
 refs.h   |  22 +-
 sequencer.c  |   3 +-
 t/t1400-update-ref.sh| 445 +++
 15 files changed, 812 insertions(+), 41 deletions(-)

-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/8] refs: report ref type from lock_any_ref_for_update

[Brad King]

Expose lock_ref_sha1_basic's type_p argument to callers of
lock_any_ref_for_update.  Update all call sites to ignore it by passing
NULL for now.

Signed-off-by: Brad King brad.k...@kitware.com
---
 branch.c   | 2 +-
 builtin/commit.c   | 2 +-
 builtin/fetch.c| 3 ++-
 builtin/receive-pack.c | 3 ++-
 builtin/reflog.c   | 2 +-
 builtin/replace.c  | 2 +-
 builtin/tag.c  | 2 +-
 fast-import.c  | 2 +-
 refs.c | 7 ---
 refs.h | 2 +-
 sequencer.c| 3 ++-
 11 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/branch.c b/branch.c
index c5c6984..f2d383f 100644
--- a/branch.c
+++ b/branch.c
@@ -291,7 +291,7 @@ void create_branch(const char *head,
hashcpy(sha1, commit-object.sha1);
 
if (!dont_change_ref) {
-   lock = lock_any_ref_for_update(ref.buf, NULL, 0);
+   lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
if (!lock)
die_errno(_(Failed to lock ref for update));
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 10acc53..be08f41 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1618,7 +1618,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
   !current_head
   ? NULL
   : current_head-object.sha1,
-  0);
+  0, NULL);
 
nl = strchr(sb.buf, '\n');
if (nl)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index d784b2e..28e4025 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -246,7 +246,8 @@ static int s_update_ref(const char *action,
rla = default_rla.buf;
snprintf(msg, sizeof(msg), %s: %s, rla, action);
lock = lock_any_ref_for_update(ref-name,
-  check_old ? ref-old_sha1 : NULL, 0);
+  check_old ? ref-old_sha1 : NULL,
+  0, NULL);
if (!lock)
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
  STORE_REF_ERROR_OTHER;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e3eb5fc..a323070 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -524,7 +524,8 @@ static const char *update(struct command *cmd)
return NULL; /* good */
}
else {
-   lock = lock_any_ref_for_update(namespaced_name, old_sha1, 0);
+   lock = lock_any_ref_for_update(namespaced_name, old_sha1,
+  0, NULL);
if (!lock) {
rp_error(failed to lock %s, name);
return failed to lock;
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 54184b3..28d756a 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -366,7 +366,7 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, int unused,
 * we take the lock for the ref itself to prevent it from
 * getting updated.
 */
-   lock = lock_any_ref_for_update(ref, sha1, 0);
+   lock = lock_any_ref_for_update(ref, sha1, 0, NULL);
if (!lock)
return error(cannot lock ref '%s', ref);
log_file = git_pathdup(logs/%s, ref);
diff --git a/builtin/replace.c b/builtin/replace.c
index 59d3115..1ecae8d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -105,7 +105,7 @@ static int replace_object(const char *object_ref, const 
char *replace_ref,
else if (!force)
die(replace ref '%s' already exists, ref);
 
-   lock = lock_any_ref_for_update(ref, prev, 0);
+   lock = lock_any_ref_for_update(ref, prev, 0, NULL);
if (!lock)
die(%s: cannot lock the ref, ref);
if (write_ref_sha1(lock, repl, NULL)  0)
diff --git a/builtin/tag.c b/builtin/tag.c
index af3af3f..2c867d2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -577,7 +577,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (annotate)
create_tag(object, tag, buf, opt, prev, object);
 
-   lock = lock_any_ref_for_update(ref.buf, prev, 0);
+   lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
if (!lock)
die(_(%s: cannot lock the ref), ref.buf);
if (write_ref_sha1(lock, object, NULL)  0)
diff --git a/fast-import.c b/fast-import.c
index 23f625f..5c329f6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1678,7 +1678,7 @@ static int update_branch(struct branch *b)
return 0;
if (read_ref(b-name, old_sha1))
hashclr(old_sha1);
-   lock = lock_any_ref_for_update(b-name, old_sha1, 0);
+   lock = lock_any_ref_for_update(b-name, old_sha1, 0, NULL);
if (!lock

[PATCH v4 4/8] refs: factor delete_ref loose ref step into a helper

[Brad King]

Factor loose ref deletion into helper function delete_ref_loose to allow
later use elsewhere.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c | 27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index 4347826..ab9d22e 100644
--- a/refs.c
+++ b/refs.c
@@ -2450,24 +2450,31 @@ static int repack_without_ref(const char *refname)
return commit_packed_refs();
 }
 
+static int delete_ref_loose(struct ref_lock *lock, int flag)
+{
+   if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
+   /* loose */
+   int err, i = strlen(lock-lk-filename) - 5; /* .lock */
+
+   lock-lk-filename[i] = 0;
+   err = unlink_or_warn(lock-lk-filename);
+   lock-lk-filename[i] = '.';
+   if (err  errno != ENOENT)
+   return 1;
+   }
+   return 0;
+}
+
 int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 {
struct ref_lock *lock;
-   int err, i = 0, ret = 0, flag = 0;
+   int ret = 0, flag = 0;
 
lock = lock_ref_sha1_basic(refname, sha1, delopt, flag);
if (!lock)
return 1;
-   if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
-   /* loose */
-   i = strlen(lock-lk-filename) - 5; /* .lock */
-   lock-lk-filename[i] = 0;
-   err = unlink_or_warn(lock-lk-filename);
-   if (err  errno != ENOENT)
-   ret = 1;
+   ret |= delete_ref_loose(lock, flag);
 
-   lock-lk-filename[i] = '.';
-   }
/* removing the loose one could have resurrected an earlier
 * packed one.  Also, if it was not loose we need to repack
 * without it.
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 7/8] update-ref: support multiple simultaneous updates

[Brad King]

On 09/04/2013 03:17 PM, Junio C Hamano wrote:
 Brad King brad.k...@kitware.com writes:
 +static void update_refs_stdin_read_n()
 +static void update_refs_stdin_read_z()
 
 These need to be defined with explicit (void) argument list.

Oops, fixed.

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 7/8] update-ref: support multiple simultaneous updates

[Brad King]

On 09/04/2013 02:23 PM, Junio C Hamano wrote:
 whitespace-separated implies that we may allow fields separated with not a 
 single SP, but with double SPs or even HTs between them. I personally do not 
 think we should be so loose

Okay, I will look at making it more strict.  See proposed format
below.

 +Quote arguments containing whitespace as if in C source code.
 
 Probably as if they were strings in C source code.

Fixed.

 +terminate each argument by NUL and each line by LF NUL.
 
 This is a somewhat interesting choice of the record terminator. Do we have a 
 precedent to use LF NUL elsewhere?  If this is the first such case where we 
 need to express variable number of NUL-separated fields in a record, I think 
 I am fine with LF NUL (but I am sure other people would give us a
 better convention if we ask them politely ;-)), but I just want to make sure 
 we do it the same way as other codepaths, if exist, that have to handle this 
 kind of thing.

Nothing else uses LF NUL.  I chose it as a starting point for
this very discussion, which I asked about in $gmane/233653.
In this particular use case we know the last field will never
be LF but that may not be so for future cases.  There is no way
to represent sentinel-terminated arbitrary variable-width records
of NUL-terminated fields without some kind of escaping for the
sentinel value, but the whole point of -z is to avoid escaping.

Below is a survey of all mentions of NUL and \0 in documented
formats as of v1.8.4.  The summary is that most are fixed-width
records but a few have variable width allowing n or n+1 fields.
In all variable-width cases there is structured information in
the first field that indicates the number of NUL-terminated
fields to expect.

In the motivating case here, we could use a --no-old or
--have-old option to indicate in one field how many more to
expect in the record, but that will be quite verbose.

Side note: I'd like to reserve room for the leading options to
include things like -m NUL reason NUL so we cannot keep them
all in in a single NUL-terminated, SP-separated field.

Another approach is to introduce a way to represent not here
for the oldvalue argument that is not an otherwise valid value.
This would make the non-option part of the record have a fixed
width of 3 fields.  For example, we could use SP in -z mode:

 [-opt NUL]... ref NUL new NUL (old|SP) NUL

and the last field can be optional in non-z mode anyway:

 [-opt SP]... ref SP new [SP old] LF

Or we could use a character like ~ (other ideas?):

 [-opt NUL]... ref NUL new NUL (old|~) NUL

and make it available in non-z mode too:

 [-opt SP]... ref SP new [SP (old|~)] LF

Thoughts?
-Brad


Survey of NUL in documented formats:

* Documentation/diff-format.txt: The -z mode for --numstat prints
  NUL-terminated lines but there is exactly one path at the end of
  each entry and the earlier fields are separated by TAB because
  they are structured.

* Documentation/diff-options.txt: The -z mode for diff-tree output
  prints structured SP/TAB-separated fields in a NUL-terminated
  field followed by either one or two NUL-terminated paths.  This
  is variable width but the first field tells us how wide.

* Documentation/git-apply.txt: The -z mode forwards to --numstat
  diff options.

* Documentation/git-check-attr.txt: The -z mode for stdin reads
  NUL-terminated paths.

* Documentation/git-check-ignore.txt: The -z mode for stdin reads
  NUL-terminated paths.  The -z mode for output prints a fixed-width
  table with every group of four NUL-terminated fields forming a row.

* Documentation/git-checkout-index.txt: The -z mode reads
  NUL-terminated paths.

* Documentation/git-commit.txt: The -z mode forwards to git-status.

* Documentation/git-grep.txt: The -z mode separates file names from
  the matched line by a NUL.  Therefore NUL divides LF-terminated
  lines into two pieces.

* Documentation/git-ls-files.txt: The -z mode prints NUL-terminated
  lines but there is exactly one path at the end of each entry and the
  earlier fields are separated by SP and TAB because they are
  structured.

* Documentation/git-ls-tree.txt: The -z mode prints NUL-terminated
  lines but there is exactly one path at the end of each entry and the
  earlier fields are separated by SP and TAB because they are
  structured.

* Documentation/git-mktree.txt: The -z mode reads NUL-terminated lines
  as output by ls-tree -z.

* Documentation/git-status.txt: The -z mode of --porcelain separates
  a variable number of entries by NUL.  The beginning of each entry
  allows one to know the number of NUL-terminated fields to expect
  (A = 1 total NUL, R = 2 total NULs, etc.).

* Documentation/git-update-index.txt: The -z mode of --stdin separates
  paths by NUL.  The -z mode of --index-info separates entries by NUL
  but there is exactly one path at the end of each entry and the earlier
  fields are separated by SP and TAB 

Re: [PATCH v2 6/8] refs: add update_refs for multiple simultaneous updates

[Brad King]

On 09/03/2013 12:43 AM, Michael Haggerty wrote:
 Hmmm, I see that you changed the signature of update_refs() to take an
 array of pointers.  My suggestion was unclear, but I didn't mean that
 the function signature had to be changed.
[snip]
 However, your approach is also fine.

Okay.  Thanks for reviewing!

-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 8/8] update-ref: add test cases covering --stdin signature

[Brad King]

On 09/03/2013 04:16 AM, Eric Sunshine wrote:
 When you decomposed the monolithic test from v1 into individual tests,
 you dropped a couple cases (fatal: unknown option' and fatal:
 unterminated single-quote). Was this intentional?

Yes.  The v3 patch 7 changed the set of error messages to be covered.

 The leading '-' on '-EOF' allows you to indent the content of the
 heredoc and the terminating EOF, which makes the test read nicely:

Very nice.  Fixed for next iteration.

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/8] refs: factor update_ref steps into helpers

[Brad King]

On 09/01/2013 02:08 AM, Junio C Hamano wrote:
 Brad King brad.k...@kitware.com writes:
  static struct ref_lock *lock;
 
 Not the fault of this patch, as the original update_ref() had it
 this way, but it is not necessary to keep the value of this variable
 across invocations.  Let's drop static from here, and also the
 corresponding variable in the new update_ref().

Fixed in next revision.

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/8] refs: factor delete_ref loose ref step into a helper

[Brad King]

On 08/31/2013 12:30 PM, Michael Haggerty wrote:
 Given that ret is only returned, you could restore the filename before
 the if statement and replace the ret variable with an immediate return
 statement:

Good idea.  Fixed in next revision.

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/8] refs: add update_refs for multiple simultaneous updates

[Brad King]

On 08/31/2013 02:19 PM, Michael Haggerty wrote:
 s/themeselves/themselves/

Fixed.

 +struct ref_update *u1 = (struct ref_update *)(r1);
 +struct ref_update *u2 = (struct ref_update *)(r2);
 
 If you declare u1 and u2 to be const struct ref_update * (i.e., add
 const), then you have const correctness and don't need the explicit
 casts.  (And the parentheses around r1 and r2 are superfluous in any case.)

Fixed.

 +ret = strcmp(u1-ref_name, u2-ref_name);
 
 Is there a need to compare more than ref_name?  If two entries are found
 with the same name, then ref_update_reject_duplicates() will error out

Junio mentioned possibility of auto-combining identical entries which would
need full ordering.  I think that can be added later so for now we can sort
only by ref name.  Thanks.

 +if (!strcmp(updates[i - 1].ref_name, updates[i].ref_name))
 +break;
 
 The error handling code could be right here instead of the break
 statement, removing the need for the if conditional.

Fixed.

 +/* Allocate work space: */
 +updates = xmalloc(sizeof(struct ref_update) * n);
 
 It seems preferred here to write
 
   updates = xmalloc(sizeof(*updates) * n);
 
 as this will continue to work if the type of updates is ever changed.

Yes, thanks.

 Similarly for the next lines.
 
 +types = xmalloc(sizeof(int) * n);
 +locks = xmalloc(sizeof(struct ref_lock *) * n);
 +delnames = xmalloc(sizeof(const char *) * n);
 
 An alternative to managing separate arrays to hold types and locks would
 be to include the scratch space in struct ref_update and document it
 for internal use only; need not be initialized by caller.  On the one
 hand it's ugly to cruft up the interface with internal implementation
 details; on the other hand there is precedent for this sort of thing
 (e.g., ref_lock::force_write or lock_file::on_list) and it would
 simplify the code.

I think the goto cleanup reorganization simplifies the code enough
to not need this.  After changing updates to an array of pointers
it needs to be separate so we can sort.  Also delnames needs to be
a separate array to pass to repack_without_refs.

 +/* Copy, sort, and reject duplicate refs: */
 +memcpy(updates, updates_orig, sizeof(struct ref_update) * n);
 +qsort(updates, n, sizeof(struct ref_update), ref_update_compare);
 
 You could save some space and memory shuffling (during memcpy() and
 qsort()) if you would declare updates to be an array of pointers to
 struct ref_update rather than an array of structs.  Sorting could then
 be done by moving pointers around instead of moving the structs.  This
 would also make it easier for update_refs() to pass information about
 the references back to its caller, should that ever be needed.

Good idea.  Changed in next revision.

 +ret |= update_ref_write(action,
 +updates[i].ref_name,
 +updates[i].new_sha1,
 +locks[i], onerr);
 +locks[i] = 0; /* freed by update_ref_write */
 +}
 +
 
 Hmmm, if one of the calls to update_ref_write() fails, would it be safer
 to abort the rest of the work (especially the reference deletions)?

Yes.  Since we already have the lock at this point something must be
going pretty wrong if this fails so it is best to abort altogether.

 +free(updates);
 +free(types);
 +free(locks);
 +free(delnames);
 +return ret;
 +}
 
 There's a lot of duplicated cleanup code in the function.  If you put a
 label before the final for loop, and if you initialize the locks array
 to zeros (e.g., by using xcalloc()), then the three exits could all
 share the same code ret = 1; goto cleanup;.

Done, thanks.

 +struct ref_update {
 
 Please document this structure, especially the relationship between
 have_old and old_sha1.

Done.  I also moved it to the top of the header just under ref_lock
so it can be used by other APIs later.

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/8] refs: add update_refs for multiple simultaneous updates

[Brad King]

On 09/01/2013 02:08 AM, Junio C Hamano wrote:
 Though the refs themeselves cannot be modified together in a single
 
 themselves.

Fixed.

 I notice that we are using an array of structures and letting qsort
 swap 50~64 bytes of data

Michael suggested this too, so fixed.

 Optionally we could silently dedup multiple identical updates and
 not fail it in ref-update-reject-duplicates.  But that does not have
 to be done until we find people's script would benefit from such a
 nicety.

We can always be less strict about input later, so I'd like to keep
the implementation simpler for now.

 By the way, unless there is a strong reason not to do so,
 post-increment i++ (and pre-decrement --i, if you use it) is the
 norm around here.

Okay, fixed in entire series.

 +locks[i] = 0; /* freed by update_ref_write */
 
 I think what is assigned here is a NULL pointer.

Yes, fixed.

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 7/8] update-ref: support multiple simultaneous updates

[Brad King]

On 08/31/2013 02:42 PM, Michael Haggerty wrote:
 On 08/30/2013 08:12 PM, Brad King wrote:
 +If all refs can be locked with matching oldvalues
 +simultaneously all modifications are performed.  Otherwise, no
 
 Comma after simultaneously.

Fixed.

 I agree with Junio that your quoting rules are peculiar.

I won't disagree.  That's why I asked for suggestions in the
original PATCH/RFC cover letter ;)

 +/* Allocate and zero-init a struct ref_update: */
 
 Here you can use ARRAY_GROW().  See
 
 Documentation/technical/api-allocation-growing.txt

Fixed.

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 7/8] update-ref: support multiple simultaneous updates

[Brad King]

On 08/30/2013 06:51 PM, Junio C Hamano wrote:
 Brad King brad.k...@kitware.com writes:
 +With `--stdin`, update-ref reads instructions from standard input and
 +performs all modifications together.  Empty lines are ignored.
 +Each non-empty line is parsed as whitespace-separated arguments.
 +Use single-quotes to enclose whitespace and backslashes and an
 +unquoted backslash to escape a single quote.
 
 That is somewhat unusual.
 
 When we need to deal with arbitrary strings (like pathnames), other
 parts of the system usually give the user two interfaces, --stdin
 with and without -z, and the strings are C-quoted when run without
 the -z option, and terminated with NUL when run with the -z option.

Great, this was the kind of suggestion I was looking for in the original
PATCH/RFC cover letter.  Thanks.  I'll start with the C-quoted version
and think about adding -z once we've agreed on that format.

 +Specify updates with
 +lines of the form:
 +
 +[--no-deref] [--] ref newvalue [oldvalue]
 
 What is -- doing here?  refs given to update-ref begin with refs/
 (otherwise it is HEAD), no?

The existing update-ref command line can be used to create all kinds
of refs, even starting in -.  I didn't want to add any restriction
in the stdin format.  I'm not opposed to it though.

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 8/8] update-ref: add test cases covering --stdin signature

[Brad King]

On 08/31/2013 11:41 PM, Eric Sunshine wrote:
 +   rm -f stdin 
 +   touch stdin 
 
 Unless the timestamp of 'stdin' has particular significance, modern
 git tests avoid 'touch' in favor of creating the empty file like this
 
 stdin 

Fixed.

 +   git update-ref --stdin  stdin 
 
 Style: Git test scripts omit whitespace following , , , and .

Fixed.

 +test_expect_success 'stdin fails with bad line lines' '
 
 Despite the semantic relationship between all these cases, if there is
 a regression in one case, the person reading the verbose output has to
 study it carefully to determine the offending case. If you decompose
 this monolith so that each case is in its own test_expect_success,
 then the regressed case becomes immediately obvious.

Yes, of course.  Fixed.

 multi-line preparations of 'stdin' might be more readable with a heredoc:
 
 cat stdin -EOF 
 $a $m
 $b $m
 $a $m
 EOF

Fixed.

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/8] reset: rename update_refs to reset_refs

[Brad King]

The function resets refs rather than doing arbitrary updates.
Rename it to allow a future general-purpose update_refs function
to be added.

Signed-off-by: Brad King brad.k...@kitware.com
---
 builtin/reset.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index afa6e02..789ee48 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -219,7 +219,7 @@ static const char **parse_args(const char **argv, const 
char *prefix, const char
return argv[0] ? get_pathspec(prefix, argv) : NULL;
 }
 
-static int update_refs(const char *rev, const unsigned char *sha1)
+static int reset_refs(const char *rev, const unsigned char *sha1)
 {
int update_ref_status;
struct strbuf msg = STRBUF_INIT;
@@ -350,7 +350,7 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
if (!pathspec  !unborn) {
/* Any resets without paths update HEAD to the head being
 * switched to, saving the previous head in ORIG_HEAD before. */
-   update_ref_status = update_refs(rev, sha1);
+   update_ref_status = reset_refs(rev, sha1);
 
if (reset_type == HARD  !update_ref_status  !quiet)
print_new_head_line(lookup_commit_reference(sha1));
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 8/8] update-ref: add test cases covering --stdin signature

[Brad King]

Extend t/t1400-update-ref.sh to cover cases using the --stdin option.

Signed-off-by: Brad King brad.k...@kitware.com
---
 t/t1400-update-ref.sh |  256 +
 1 file changed, 256 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index e415ee0..b6d7dfa 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -302,4 +302,260 @@ test_expect_success \
'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER)' \
'test OTHER = $(git cat-file blob master@{2005-05-26 23:42}:F)'
 
+a=refs/heads/a
+b=refs/heads/b
+c=refs/heads/c
+z=
+e=''
+pws='path with space'
+
+test_expect_success 'stdin test setup' '
+   echo $pws $pws 
+   git add -- $pws 
+   git commit -m $pws
+'
+
+test_expect_success 'stdin works with no input' '
+   stdin 
+   git update-ref --stdin stdin 
+   git rev-parse --verify -q $m
+'
+
+test_expect_success 'stdin fails on bad input line with only whitespace' '
+   echo   stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: no ref on line:   err
+'
+
+test_expect_success 'stdin fails on bad input line with only --' '
+   echo -- stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: no ref on line: -- err
+'
+
+test_expect_success 'stdin fails on bad input line with only --bad-option' '
+   echo --bad-option stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: unknown option --bad-option err
+'
+
+test_expect_success 'stdin fails on bad ref name' '
+   echo ~a $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid ref format on line: ~a $m err
+'
+
+test_expect_success 'stdin fails on badly quoted input' '
+   echo $a \master stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: badly quoted argument: \\\master err
+'
+
+test_expect_success 'stdin fails on bad input line with too many arguments' '
+   echo $a $m $m $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: too many arguments on line: $a $m $m $m err
+'
+
+test_expect_success 'stdin fails on bad input line with too few arguments' '
+   echo $a stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: missing new value on line: $a err
+'
+
+test_expect_success 'stdin fails with duplicate refs' '
+   cat stdin -EOF 
+$a $m
+$b $m
+$a $m
+EOF
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: Multiple updates for ref '''$a''' not allowed. err
+'
+
+test_expect_success 'stdin create ref works with no old value' '
+   echo $a $m stdin 
+   git update-ref --stdin stdin 
+   git rev-parse $m expect 
+   git rev-parse $a actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'stdin create ref works with zero old value' '
+   echo $b $m $z stdin 
+   git update-ref --stdin stdin 
+   git rev-parse $m expect 
+   git rev-parse $b actual 
+   test_cmp expect actual 
+   git update-ref -d $b 
+   echo $b $m $e stdin 
+   git update-ref --stdin stdin 
+   git rev-parse $m expect 
+   git rev-parse $b actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'stdin create ref works with path with space to blob' '
+   echo refs/blobs/pws \$m:$pws\ stdin 
+   git update-ref --stdin stdin 
+   git rev-parse $m:$pws expect 
+   git rev-parse refs/blobs/pws actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'stdin create ref fails with wrong old value' '
+   echo $c $m $m~1 stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: Cannot lock the ref '''$c''' err 
+   test_must_fail git rev-parse --verify -q $c
+'
+
+test_expect_success 'stdin create ref fails with bad old value' '
+   echo $c $m does-not-exist stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid old value on line: $c $m does-not-exist err 
+   test_must_fail git rev-parse --verify -q $c
+'
+
+test_expect_success 'stdin create ref fails with bad new value' '
+   echo $c does-not-exist stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: invalid new value on line: $c does-not-exist err 
+   test_must_fail git rev-parse --verify -q $c
+'
+
+test_expect_success 'stdin update ref works with right old value' '
+   echo $b $m~1 $m stdin 
+   git update-ref --stdin stdin 
+   git rev-parse $m~1 expect 
+   git rev-parse $b actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'stdin update ref fails with wrong old value' '
+   echo $b $m~1 $m stdin 
+   test_must_fail git update-ref --stdin stdin 2err 
+   grep fatal: Cannot lock the ref '''$b''' err 
+   git rev-parse $m~1 expect 
+   git

[PATCH v3 4/8] refs: factor delete_ref loose ref step into a helper

[Brad King]

Factor loose ref deletion into helper function delete_ref_loose to allow
later use elsewhere.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c |   27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index 4347826..ab9d22e 100644
--- a/refs.c
+++ b/refs.c
@@ -2450,24 +2450,31 @@ static int repack_without_ref(const char *refname)
return commit_packed_refs();
 }
 
+static int delete_ref_loose(struct ref_lock *lock, int flag)
+{
+   if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
+   /* loose */
+   int err, i = strlen(lock-lk-filename) - 5; /* .lock */
+
+   lock-lk-filename[i] = 0;
+   err = unlink_or_warn(lock-lk-filename);
+   lock-lk-filename[i] = '.';
+   if (err  errno != ENOENT)
+   return 1;
+   }
+   return 0;
+}
+
 int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 {
struct ref_lock *lock;
-   int err, i = 0, ret = 0, flag = 0;
+   int ret = 0, flag = 0;
 
lock = lock_ref_sha1_basic(refname, sha1, delopt, flag);
if (!lock)
return 1;
-   if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
-   /* loose */
-   i = strlen(lock-lk-filename) - 5; /* .lock */
-   lock-lk-filename[i] = 0;
-   err = unlink_or_warn(lock-lk-filename);
-   if (err  errno != ENOENT)
-   ret = 1;
+   ret |= delete_ref_loose(lock, flag);
 
-   lock-lk-filename[i] = '.';
-   }
/* removing the loose one could have resurrected an earlier
 * packed one.  Also, if it was not loose we need to repack
 * without it.
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 5/8] refs: add function to repack without multiple refs

[Brad King]

Generalize repack_without_ref as repack_without_refs to support a list
of refs and implement the former in terms of the latter.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c |   29 ++---
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index ab9d22e..599504b 100644
--- a/refs.c
+++ b/refs.c
@@ -2414,25 +2414,35 @@ static int curate_packed_ref_fn(struct ref_entry 
*entry, void *cb_data)
return 0;
 }
 
-static int repack_without_ref(const char *refname)
+static int repack_without_refs(const char **refnames, int n)
 {
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
+   int i, removed = 0;
+
+   /* Look for a packed ref: */
+   for (i = 0; i  n; i++)
+   if (get_packed_ref(refnames[i]))
+   break;
 
-   if (!get_packed_ref(refname))
-   return 0; /* refname does not exist in packed refs */
+   /* Avoid locking if we have nothing to do: */
+   if (i == n)
+   return 0; /* no refname exists in packed refs */
 
if (lock_packed_refs(0)) {
unable_to_lock_error(git_path(packed-refs), errno);
-   return error(cannot delete '%s' from packed refs, refname);
+   return error(cannot delete '%s' from packed refs, 
refnames[i]);
}
packed = get_packed_refs(ref_cache);
 
-   /* Remove refname from the cache: */
-   if (remove_entry(packed, refname) == -1) {
+   /* Remove refnames from the cache: */
+   for (i = 0; i  n; i++)
+   if (remove_entry(packed, refnames[i]) != -1)
+   removed = 1;
+   if (!removed) {
/*
-* The packed entry disappeared while we were
+* All packed entries disappeared while we were
 * acquiring the lock.
 */
rollback_packed_refs();
@@ -2450,6 +2460,11 @@ static int repack_without_ref(const char *refname)
return commit_packed_refs();
 }
 
+static int repack_without_ref(const char *refname)
+{
+   return repack_without_refs(refname, 1);
+}
+
 static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 6/8] refs: add update_refs for multiple simultaneous updates

[Brad King]

Add 'struct ref_update' to encode the information needed to update or
delete a ref (name, new sha1, optional old sha1, no-deref flag).  Add
function 'update_refs' accepting an array of updates to perform.  First
sort the input array to order locks consistently everywhere and reject
multiple updates to the same ref.  Then acquire locks on all refs with
verified old values.  Then update or delete all refs accordingly.  Fail
if any one lock cannot be obtained or any one old value does not match.

Though the refs themselves cannot be modified together in a single
atomic transaction, this function does enable some useful semantics.
For example, a caller may create a new branch starting from the head of
another branch and rewind the original branch at the same time.  This
transfers ownership of commits between branches without risk of losing
commits added to the original branch by a concurrent process, or risk of
a concurrent process creating the new branch first.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c |  100 
 refs.h |   20 +
 2 files changed, 120 insertions(+)

diff --git a/refs.c b/refs.c
index 599504b..53e8774 100644
--- a/refs.c
+++ b/refs.c
@@ -3237,6 +3237,106 @@ int update_ref(const char *action, const char *refname,
return update_ref_write(action, refname, sha1, lock, onerr);
 }
 
+static int ref_update_compare(const void *r1, const void *r2)
+{
+   const struct ref_update * const *u1 = r1;
+   const struct ref_update * const *u2 = r2;
+   return strcmp((*u1)-ref_name, (*u2)-ref_name);
+}
+
+static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+   enum action_on_err onerr)
+{
+   int i;
+   for (i = 1; i  n; i++)
+   if (!strcmp(updates[i - 1]-ref_name, updates[i]-ref_name)) {
+   const char *str =
+   Multiple updates for ref '%s' not allowed.;
+   switch (onerr) {
+   case MSG_ON_ERR:
+   error(str, updates[i]-ref_name); break;
+   case DIE_ON_ERR:
+   die(str, updates[i]-ref_name); break;
+   case QUIET_ON_ERR:
+   break;
+   }
+   return 1;
+   }
+   return 0;
+}
+
+int update_refs(const char *action, const struct ref_update **updates_orig,
+   int n, enum action_on_err onerr)
+{
+   int ret = 0, delnum = 0, i;
+   struct ref_update **updates;
+   int *types;
+   struct ref_lock **locks;
+   const char **delnames;
+
+   if (!updates_orig || !n)
+   return 0;
+
+   /* Allocate work space: */
+   updates = xmalloc(sizeof(*updates) * n);
+   types = xmalloc(sizeof(*types) * n);
+   locks = xcalloc(n, sizeof(*locks));
+   delnames = xmalloc(sizeof(*delnames) * n);
+
+   /* Copy, sort, and reject duplicate refs: */
+   memcpy(updates, updates_orig, sizeof(*updates) * n);
+   qsort(updates, n, sizeof(*updates), ref_update_compare);
+   ret = ref_update_reject_duplicates(updates, n, onerr);
+   if (ret)
+   goto cleanup;
+
+   /* Acquire all locks while verifying old values: */
+   for (i = 0; i  n; i++) {
+   locks[i] = update_ref_lock(updates[i]-ref_name,
+  (updates[i]-have_old ?
+   updates[i]-old_sha1 : NULL),
+  updates[i]-flags,
+  types[i], onerr);
+   if (!locks[i]) {
+   ret = 1;
+   goto cleanup;
+   }
+   }
+
+   /* Perform updates first so live commits remain referenced: */
+   for (i = 0; i  n; i++)
+   if (!is_null_sha1(updates[i]-new_sha1)) {
+   ret = update_ref_write(action,
+  updates[i]-ref_name,
+  updates[i]-new_sha1,
+  locks[i], onerr);
+   locks[i] = NULL; /* freed by update_ref_write */
+   if (ret)
+   goto cleanup;
+   }
+
+   /* Perform deletes now that updates are safely completed: */
+   for (i = 0; i  n; i++)
+   if (locks[i]) {
+   delnames[delnum++] = locks[i]-ref_name;
+   ret |= delete_ref_loose(locks[i], types[i]);
+   }
+   ret |= repack_without_refs(delnames, delnum);
+   for (i = 0; i  delnum; i++)
+   unlink_or_warn(git_path(logs/%s, delnames[i]));
+   clear_loose_ref_cache(ref_cache);
+
+cleanup:
+   for (i = 0; i  n; i

[PATCH v3 0/8] Multiple simultaneously locked ref updates

[Brad King]

Hi Folks,

Here is the third revision of a series to support locking multiple
refs at the same time to update all of them consistently.  The
previous revisions of the series can be found at $gmane/233260 and
$gmane/233458.

Updates since the previous revision of the series:

* Incorporated style fixes suggested in patches 6-8.

* In patch 3, the local lock variables in update_ref_lock and
  update_ref now drop the existing static declaration.

* In patch 4, delete_ref_loose internals have been cleaned up
  as Michael suggested.

* In patch 6:

  - struct ref_update has been documented
  - update_refs now takes an array of pointers to struct ref_update
as Michael and Junio both suggested
  - update_refs return cases were simplified with a label and goto
  - update_refs now stops immediately if any ref write fails
  - ref_update_compare now compares only the ref name

* In patch 7, another new input format is proposed.  It now uses
  quoting based on unquote_c_style.

* In patch 8, more new test cases have been added.  Failure cases
  are now covered in separate steps to simplify diagnosis.

-Brad

Brad King (8):
  reset: rename update_refs to reset_refs
  refs: report ref type from lock_any_ref_for_update
  refs: factor update_ref steps into helpers
  refs: factor delete_ref loose ref step into a helper
  refs: add function to repack without multiple refs
  refs: add update_refs for multiple simultaneous updates
  update-ref: support multiple simultaneous updates
  update-ref: add test cases covering --stdin signature

 Documentation/git-update-ref.txt |   20 ++-
 branch.c |2 +-
 builtin/commit.c |2 +-
 builtin/fetch.c  |3 +-
 builtin/receive-pack.c   |3 +-
 builtin/reflog.c |2 +-
 builtin/replace.c|2 +-
 builtin/reset.c  |4 +-
 builtin/tag.c|2 +-
 builtin/update-ref.c |  103 ++-
 fast-import.c|2 +-
 refs.c   |  191 
 refs.h   |   22 +++-
 sequencer.c  |3 +-
 t/t1400-update-ref.sh|  256 ++
 15 files changed, 578 insertions(+), 39 deletions(-)

-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 7/8] update-ref: support multiple simultaneous updates

[Brad King]

Add a --stdin signature to read update instructions from standard input
and apply multiple ref updates together.  Use an input format that
supports any update that could be specified via the command-line,
including object names like branch:path with space.

Signed-off-by: Brad King brad.k...@kitware.com
---
 Documentation/git-update-ref.txt |   20 +++-
 builtin/update-ref.c |  103 +-
 2 files changed, 121 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 0df13ff..01019f1 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -8,7 +8,7 @@ git-update-ref - Update the object name stored in a ref safely
 SYNOPSIS
 
 [verse]
-'git update-ref' [-m reason] (-d ref [oldvalue] | [--no-deref] ref 
newvalue [oldvalue])
+'git update-ref' [-m reason] (-d ref [oldvalue] | [--no-deref] ref 
newvalue [oldvalue] | --stdin)
 
 DESCRIPTION
 ---
@@ -58,6 +58,24 @@ archive by creating a symlink tree).
 With `-d` flag, it deletes the named ref after verifying it
 still contains oldvalue.
 
+With `--stdin`, update-ref reads instructions from standard input and
+performs all modifications together.  Empty lines are ignored.
+Each non-empty line is parsed as whitespace-separated arguments.
+Quote arguments containing whitespace as if in C source code.
+Specify updates with lines of the form:
+
+   [--no-deref] [--] ref newvalue [oldvalue]
+
+Lines of any other format or a repeated ref produce an error.
+Specify a zero newvalue to delete a ref and/or a zero oldvalue
+to make sure that a ref not exist.  Use either 40 0 or the
+empty string (written as ) to specify a zero value.
+
+If all refs can be locked with matching oldvalues
+simultaneously, all modifications are performed.  Otherwise, no
+modifications are performed.  Note that while each individual
+ref is updated or deleted atomically, a concurrent reader may
+still see a subset of the modifications.
 
 Logging Updates
 ---
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 51d2684..12a3c76 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -2,23 +2,115 @@
 #include refs.h
 #include builtin.h
 #include parse-options.h
+#include quote.h
 
 static const char * const git_update_ref_usage[] = {
N_(git update-ref [options] -d refname [oldval]),
N_(git update-ref [options]refname newval [oldval]),
+   N_(git update-ref [options] --stdin),
NULL
 };
 
+static int updates_alloc;
+static int updates_count;
+static const struct ref_update **updates;
+
+static const char* update_refs_stdin_next_arg(const char* next,
+ struct strbuf *arg)
+{
+   /* Skip leading whitespace: */
+   while (isspace(*next))
+   ++next;
+
+   /* Return NULL when no argument is found: */
+   if (!*next)
+   return NULL;
+
+   /* Parse the argument: */
+   strbuf_reset(arg);
+   if (*next == '') {
+   if (unquote_c_style(arg, next, next))
+   die(badly quoted argument: %s, next);
+   return next;
+   }
+   while (*next  !isspace(*next))
+   strbuf_addch(arg, *next++);
+   return next;
+}
+
+static void update_refs_stdin(const char *line)
+{
+   int options = 1, flags = 0, argc = 0;
+   char *argv[3] = {0, 0, 0};
+   struct strbuf arg = STRBUF_INIT;
+   struct ref_update *update;
+   const char *next = line;
+
+   /* Skip blank lines: */
+   if (!line[0])
+   return;
+
+   /* Parse arguments on this line: */
+   while ((next = update_refs_stdin_next_arg(next, arg)) != NULL) {
+   if (options  arg.buf[0] == '-')
+   if (!strcmp(arg.buf, --no-deref))
+   flags |= REF_NODEREF;
+   else if (!strcmp(arg.buf, --))
+   options = 0;
+   else
+   die(unknown option %s, arg.buf);
+   else if (argc = 3)
+   die(too many arguments on line: %s, line);
+   else {
+   argv[argc++] = xstrdup(arg.buf);
+   options = 0;
+   }
+   }
+   strbuf_release(arg);
+
+   /* Allocate and zero-init a struct ref_update: */
+   update = xcalloc(1, sizeof(*update));
+   ALLOC_GROW(updates, updates_count+1, updates_alloc);
+   updates[updates_count++] = update;
+
+   /* Set the update ref_name: */
+   if (!argv[0])
+   die(no ref on line: %s, line);
+   if (check_refname_format(argv[0], REFNAME_ALLOW_ONELEVEL))
+   die(invalid ref format on line: %s, line);
+   update-ref_name = argv[0];
+   argv[0] = 0;
+
+   /* Set the update new_sha1 and, if specified, old_sha1

[PATCH v3 2/8] refs: report ref type from lock_any_ref_for_update

[Brad King]

Expose lock_ref_sha1_basic's type_p argument to callers of
lock_any_ref_for_update.  Update all call sites to ignore it by passing
NULL for now.

Signed-off-by: Brad King brad.k...@kitware.com
---
 branch.c   |2 +-
 builtin/commit.c   |2 +-
 builtin/fetch.c|3 ++-
 builtin/receive-pack.c |3 ++-
 builtin/reflog.c   |2 +-
 builtin/replace.c  |2 +-
 builtin/tag.c  |2 +-
 fast-import.c  |2 +-
 refs.c |7 ---
 refs.h |2 +-
 sequencer.c|3 ++-
 11 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/branch.c b/branch.c
index c5c6984..f2d383f 100644
--- a/branch.c
+++ b/branch.c
@@ -291,7 +291,7 @@ void create_branch(const char *head,
hashcpy(sha1, commit-object.sha1);
 
if (!dont_change_ref) {
-   lock = lock_any_ref_for_update(ref.buf, NULL, 0);
+   lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
if (!lock)
die_errno(_(Failed to lock ref for update));
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 10acc53..be08f41 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1618,7 +1618,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
   !current_head
   ? NULL
   : current_head-object.sha1,
-  0);
+  0, NULL);
 
nl = strchr(sb.buf, '\n');
if (nl)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index d784b2e..28e4025 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -246,7 +246,8 @@ static int s_update_ref(const char *action,
rla = default_rla.buf;
snprintf(msg, sizeof(msg), %s: %s, rla, action);
lock = lock_any_ref_for_update(ref-name,
-  check_old ? ref-old_sha1 : NULL, 0);
+  check_old ? ref-old_sha1 : NULL,
+  0, NULL);
if (!lock)
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
  STORE_REF_ERROR_OTHER;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e3eb5fc..a323070 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -524,7 +524,8 @@ static const char *update(struct command *cmd)
return NULL; /* good */
}
else {
-   lock = lock_any_ref_for_update(namespaced_name, old_sha1, 0);
+   lock = lock_any_ref_for_update(namespaced_name, old_sha1,
+  0, NULL);
if (!lock) {
rp_error(failed to lock %s, name);
return failed to lock;
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 54184b3..28d756a 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -366,7 +366,7 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, int unused,
 * we take the lock for the ref itself to prevent it from
 * getting updated.
 */
-   lock = lock_any_ref_for_update(ref, sha1, 0);
+   lock = lock_any_ref_for_update(ref, sha1, 0, NULL);
if (!lock)
return error(cannot lock ref '%s', ref);
log_file = git_pathdup(logs/%s, ref);
diff --git a/builtin/replace.c b/builtin/replace.c
index 59d3115..1ecae8d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -105,7 +105,7 @@ static int replace_object(const char *object_ref, const 
char *replace_ref,
else if (!force)
die(replace ref '%s' already exists, ref);
 
-   lock = lock_any_ref_for_update(ref, prev, 0);
+   lock = lock_any_ref_for_update(ref, prev, 0, NULL);
if (!lock)
die(%s: cannot lock the ref, ref);
if (write_ref_sha1(lock, repl, NULL)  0)
diff --git a/builtin/tag.c b/builtin/tag.c
index af3af3f..2c867d2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -577,7 +577,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (annotate)
create_tag(object, tag, buf, opt, prev, object);
 
-   lock = lock_any_ref_for_update(ref.buf, prev, 0);
+   lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
if (!lock)
die(_(%s: cannot lock the ref), ref.buf);
if (write_ref_sha1(lock, object, NULL)  0)
diff --git a/fast-import.c b/fast-import.c
index 23f625f..5c329f6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1678,7 +1678,7 @@ static int update_branch(struct branch *b)
return 0;
if (read_ref(b-name, old_sha1))
hashclr(old_sha1);
-   lock = lock_any_ref_for_update(b-name, old_sha1, 0);
+   lock = lock_any_ref_for_update(b-name, old_sha1

[PATCH v3 3/8] refs: factor update_ref steps into helpers

[Brad King]

Factor the lock and write steps and error handling into helper functions
update_ref_lock and update_ref_write to allow later use elsewhere.
Expose lock_any_ref_for_update's type_p to update_ref_lock callers.

While at it, drop static from the local lock variable as it is not
necessary to keep across invocations.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c |   30 --
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index c69fd68..4347826 100644
--- a/refs.c
+++ b/refs.c
@@ -3170,12 +3170,13 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
 }
 
-int update_ref(const char *action, const char *refname,
-   const unsigned char *sha1, const unsigned char *oldval,
-   int flags, enum action_on_err onerr)
+static struct ref_lock *update_ref_lock(const char *refname,
+   const unsigned char *oldval,
+   int flags, int *type_p,
+   enum action_on_err onerr)
 {
-   static struct ref_lock *lock;
-   lock = lock_any_ref_for_update(refname, oldval, flags, NULL);
+   struct ref_lock *lock;
+   lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
if (!lock) {
const char *str = Cannot lock the ref '%s'.;
switch (onerr) {
@@ -3183,8 +3184,14 @@ int update_ref(const char *action, const char *refname,
case DIE_ON_ERR: die(str, refname); break;
case QUIET_ON_ERR: break;
}
-   return 1;
}
+   return lock;
+}
+
+static int update_ref_write(const char *action, const char *refname,
+   const unsigned char *sha1, struct ref_lock *lock,
+   enum action_on_err onerr)
+{
if (write_ref_sha1(lock, sha1, action)  0) {
const char *str = Cannot update the ref '%s'.;
switch (onerr) {
@@ -3197,6 +3204,17 @@ int update_ref(const char *action, const char *refname,
return 0;
 }
 
+int update_ref(const char *action, const char *refname,
+  const unsigned char *sha1, const unsigned char *oldval,
+  int flags, enum action_on_err onerr)
+{
+   struct ref_lock *lock;
+   lock = update_ref_lock(refname, oldval, flags, 0, onerr);
+   if (!lock)
+   return 1;
+   return update_ref_write(action, refname, sha1, lock, onerr);
+}
+
 struct ref *find_ref_by_name(const struct ref *list, const char *name)
 {
for ( ; list; list = list-next)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/8] update-ref: support multiple simultaneous updates

[Brad King]

On 09/02/2013 01:48 PM, Brad King wrote:
 + /* Parse the argument: */
 + strbuf_reset(arg);
 + if (*next == '') {
 + if (unquote_c_style(arg, next, next))
 + die(badly quoted argument: %s, next);
 + return next;
 + }
 + while (*next  !isspace(*next))
 + strbuf_addch(arg, *next++);
 + return next;

This quoting proposal was written in response to $gmane/233479:

On 08/30/2013 06:51 PM, Junio C Hamano wrote:
 When we need to deal with arbitrary strings (like pathnames), other
 parts of the system usually give the user two interfaces, --stdin
 with and without -z, and the strings are C-quoted when run without
 the -z option, and terminated with NUL when run with the -z option.

1. Do we want to allow arbitrary non-space characters in unquoted
arguments (while loop above) or reserve some syntax for future use?

2. Thinking about how the -z variation might work, I ran:

$ git grep '\[0\] ==  -- '*.c'
builtin/check-attr.c:   if (line_termination  buf.buf[0] == '') {
builtin/check-ignore.c: if (line_termination  buf.buf[0] == '') {
builtin/checkout-index.c:   if (line_termination  
buf.buf[0] == '') {
builtin/hash-object.c:  if (buf.buf[0] == '') {
builtin/mktree.c:   if (line_termination  path[0] == '') {
builtin/update-index.c: if (line_termination  path_name[0] == '') {
builtin/update-index.c: if (line_termination  buf.buf[0] == 
'') {

All of these support quoting only in the non-z mode (the hash-object.c
line follows a getline using hard-coded '\n').  However, they are
all in cases looking for one value on a line or at the end of a line
so their -z option allows NUL-terminated lines containing LF.

What distinguishes the update-ref --stdin case is that we want to
represent multiple arguments on one line, each allowing arbitrary
characters or an empty string.  From a brief search a couple places
I found that do something related are:

* apply: Read multiple paths from a diff header, using unquote_c_style
  for quoted paths and separated by spaces.  There is no -z input mode.

* config: Output keyword=value\n becomes keyword\nvalue\0 in -z mode.
  This works because the first piece (keyword) cannot have a LF
  and there is at most one value so all LFs belong to it.

* quote.c: sq_dequote_to_argv handles single quotes like a shell
  would but allows only one space between arguments.  No -z mode.
  This is similar to my v2 proposal.

If we use unquote_c_style and spaces to divide LF-terminated lines,
how shall we divide arguments on NUL-terminated lines?

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/8] Multiple simultaneously locked ref updates

[Brad King]

Hi Folks,

Here is the second revision of a series to support locking multiple
refs at the same time to update all of them consistently.  The first
series can be found at $gmane/233260.  This revision is ready to
consider for integration.

Updates since the previous revision of the series:

* Incorporated style fixes and cleanups suggested by Junio.

* In patch 6, the new update_refs function now sorts the updates
  so that locks are acquired in a consistent order by competing
  processes.  Then it uses a simple linear scan to reject input
  containing duplicate refs (which are adjacent after sorting).
  Also, struct ref_update now has a symmetric representation
  for new_sha1 and old_sha1.

* In patch 7, I propose a new format for instructions read from
  standard input that is much more robust and extensible.

* Patch 8 is new and adds test cases covering new features
  and error cases.

-Brad

Brad King (8):
  reset: rename update_refs to reset_refs
  refs: report ref type from lock_any_ref_for_update
  refs: factor update_ref steps into helpers
  refs: factor delete_ref loose ref step into a helper
  refs: add function to repack without multiple refs
  refs: add update_refs for multiple simultaneous updates
  update-ref: support multiple simultaneous updates
  update-ref: add test cases covering --stdin signature

 Documentation/git-update-ref.txt |   21 +++-
 branch.c |2 +-
 builtin/commit.c |2 +-
 builtin/fetch.c  |3 +-
 builtin/receive-pack.c   |3 +-
 builtin/reflog.c |2 +-
 builtin/replace.c|2 +-
 builtin/reset.c  |4 +-
 builtin/tag.c|2 +-
 builtin/update-ref.c |  121 +-
 fast-import.c|2 +-
 refs.c   |  203 +
 refs.h   |   16 ++-
 sequencer.c  |3 +-
 t/t1400-update-ref.sh|  206 ++
 15 files changed, 558 insertions(+), 34 deletions(-)

-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/8] reset: rename update_refs to reset_refs

[Brad King]

The function resets refs rather than doing arbitrary updates.
Rename it to allow a future general-purpose update_refs function
to be added.

Signed-off-by: Brad King brad.k...@kitware.com
---
 builtin/reset.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index afa6e02..789ee48 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -219,7 +219,7 @@ static const char **parse_args(const char **argv, const 
char *prefix, const char
return argv[0] ? get_pathspec(prefix, argv) : NULL;
 }
 
-static int update_refs(const char *rev, const unsigned char *sha1)
+static int reset_refs(const char *rev, const unsigned char *sha1)
 {
int update_ref_status;
struct strbuf msg = STRBUF_INIT;
@@ -350,7 +350,7 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
if (!pathspec  !unborn) {
/* Any resets without paths update HEAD to the head being
 * switched to, saving the previous head in ORIG_HEAD before. */
-   update_ref_status = update_refs(rev, sha1);
+   update_ref_status = reset_refs(rev, sha1);
 
if (reset_type == HARD  !update_ref_status  !quiet)
print_new_head_line(lookup_commit_reference(sha1));
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 8/8] update-ref: add test cases covering --stdin signature

[Brad King]

Extend t/t1400-update-ref.sh to cover cases using the --stdin option.

Signed-off-by: Brad King brad.k...@kitware.com
---
 t/t1400-update-ref.sh |  206 +
 1 file changed, 206 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index e415ee0..9fd03fc 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -302,4 +302,210 @@ test_expect_success \
'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER)' \
'test OTHER = $(git cat-file blob master@{2005-05-26 23:42}:F)'
 
+a=refs/heads/a
+b=refs/heads/b
+c=refs/heads/c
+z=
+e=''
+
+test_expect_success 'stdin works with no input' '
+   rm -f stdin 
+   touch stdin 
+   git update-ref --stdin  stdin 
+   git rev-parse --verify -q $m
+'
+
+test_expect_success 'stdin fails with bad line lines' '
+   echostdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: no ref on line:   err 
+   echo --  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: no ref on line: -- err 
+   echo --bad-option  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: unknown option --bad-option err 
+   echo -\''' $a $m  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: unknown option -''' err 
+   echo ~a $m  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: invalid ref format on line: ~a $m err 
+   echo $a '''master  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: unterminated single-quote: '''master err 
+   echo $a \master  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: unquoted backslash not escaping single-quote: master 
err 
+   echo $a $m $m $m  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: too many arguments on line: $a $m $m $m err 
+   echo $a  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: missing new value on line: $a err
+'
+
+test_expect_success 'stdin fails with duplicate refs' '
+   echo $a $m  stdin 
+   echo $b $m  stdin 
+   echo $a $m  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: Multiple updates for ref '''$a''' not allowed. err
+'
+
+test_expect_success 'stdin create ref works with no old value' '
+   echo $a $m  stdin 
+   git update-ref --stdin  stdin 
+   git rev-parse $m  expect 
+   git rev-parse $a  actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'stdin create ref works with zero old value' '
+   echo $b $m $z  stdin 
+   git update-ref --stdin  stdin 
+   git rev-parse $m  expect 
+   git rev-parse $b  actual 
+   test_cmp expect actual 
+   git update-ref -d $b 
+   echo $b $m $e  stdin 
+   git update-ref --stdin  stdin 
+   git rev-parse $m  expect 
+   git rev-parse $b  actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'stdin create ref fails with wrong old value' '
+   echo $c $m $m~1  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: Cannot lock the ref '''$c''' err 
+   test_must_fail git rev-parse --verify -q $c
+'
+
+test_expect_success 'stdin create ref fails with bad old value' '
+   echo $c $m does-not-exist  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: invalid old value on line: $c $m does-not-exist err 
+   test_must_fail git rev-parse --verify -q $c
+'
+
+test_expect_success 'stdin create ref fails with bad new value' '
+   echo $c does-not-exist  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: invalid new value on line: $c does-not-exist err 
+   test_must_fail git rev-parse --verify -q $c
+'
+
+test_expect_success 'stdin update ref works with right old value' '
+   echo $b $m~1 $m  stdin 
+   git update-ref --stdin  stdin 
+   git rev-parse $m~1  expect 
+   git rev-parse $b  actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'stdin update ref fails with wrong old value' '
+   echo $b $m~1 $m  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: Cannot lock the ref '''$b''' err 
+   git rev-parse $m~1  expect 
+   git rev-parse $b  actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'stdin delete ref fails with wrong old value' '
+   echo $a $e $m~1  stdin 
+   test_must_fail git update-ref --stdin  stdin 2 err 
+   grep fatal: Cannot lock the ref '''$a''' err 
+   git rev-parse $m  expect 
+   git rev-parse $a  actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'stdin update symref works with --no-deref

[PATCH v2 4/8] refs: factor delete_ref loose ref step into a helper

[Brad King]

Factor loose ref deletion into helper function delete_ref_loose to allow
later use elsewhere.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c |   22 +++---
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 2e755b4..5dd86ee 100644
--- a/refs.c
+++ b/refs.c
@@ -2450,14 +2450,9 @@ static int repack_without_ref(const char *refname)
return commit_packed_refs();
 }
 
-int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
+static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
-   struct ref_lock *lock;
-   int err, i = 0, ret = 0, flag = 0;
-
-   lock = lock_ref_sha1_basic(refname, sha1, delopt, flag);
-   if (!lock)
-   return 1;
+   int err, i, ret = 0;
if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
/* loose */
i = strlen(lock-lk-filename) - 5; /* .lock */
@@ -2468,6 +2463,19 @@ int delete_ref(const char *refname, const unsigned char 
*sha1, int delopt)
 
lock-lk-filename[i] = '.';
}
+   return ret;
+}
+
+int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
+{
+   struct ref_lock *lock;
+   int ret = 0, flag = 0;
+
+   lock = lock_ref_sha1_basic(refname, sha1, delopt, flag);
+   if (!lock)
+   return 1;
+   ret |= delete_ref_loose(lock, flag);
+
/* removing the loose one could have resurrected an earlier
 * packed one.  Also, if it was not loose we need to repack
 * without it.
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/8] refs: factor update_ref steps into helpers

[Brad King]

Factor the lock and write steps and error handling into helper functions
update_ref_lock and update_ref_write to allow later use elsewhere.
Expose lock_any_ref_for_update's type_p to update_ref_lock callers.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c |   28 +++-
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index c69fd68..2e755b4 100644
--- a/refs.c
+++ b/refs.c
@@ -3170,12 +3170,13 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
 }
 
-int update_ref(const char *action, const char *refname,
-   const unsigned char *sha1, const unsigned char *oldval,
-   int flags, enum action_on_err onerr)
+static struct ref_lock *update_ref_lock(const char *refname,
+   const unsigned char *oldval,
+   int flags, int *type_p,
+   enum action_on_err onerr)
 {
static struct ref_lock *lock;
-   lock = lock_any_ref_for_update(refname, oldval, flags, NULL);
+   lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
if (!lock) {
const char *str = Cannot lock the ref '%s'.;
switch (onerr) {
@@ -3183,8 +3184,14 @@ int update_ref(const char *action, const char *refname,
case DIE_ON_ERR: die(str, refname); break;
case QUIET_ON_ERR: break;
}
-   return 1;
}
+   return lock;
+}
+
+static int update_ref_write(const char *action, const char *refname,
+   const unsigned char *sha1, struct ref_lock *lock,
+   enum action_on_err onerr)
+{
if (write_ref_sha1(lock, sha1, action)  0) {
const char *str = Cannot update the ref '%s'.;
switch (onerr) {
@@ -3197,6 +3204,17 @@ int update_ref(const char *action, const char *refname,
return 0;
 }
 
+int update_ref(const char *action, const char *refname,
+  const unsigned char *sha1, const unsigned char *oldval,
+  int flags, enum action_on_err onerr)
+{
+   static struct ref_lock *lock;
+   lock = update_ref_lock(refname, oldval, flags, 0, onerr);
+   if (!lock)
+   return 1;
+   return update_ref_write(action, refname, sha1, lock, onerr);
+}
+
 struct ref *find_ref_by_name(const struct ref *list, const char *name)
 {
for ( ; list; list = list-next)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/8] refs: report ref type from lock_any_ref_for_update

[Brad King]

Expose lock_ref_sha1_basic's type_p argument to callers of
lock_any_ref_for_update.  Update all call sites to ignore it by passing
NULL for now.

Signed-off-by: Brad King brad.k...@kitware.com
---
 branch.c   |2 +-
 builtin/commit.c   |2 +-
 builtin/fetch.c|3 ++-
 builtin/receive-pack.c |3 ++-
 builtin/reflog.c   |2 +-
 builtin/replace.c  |2 +-
 builtin/tag.c  |2 +-
 fast-import.c  |2 +-
 refs.c |7 ---
 refs.h |2 +-
 sequencer.c|3 ++-
 11 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/branch.c b/branch.c
index c5c6984..f2d383f 100644
--- a/branch.c
+++ b/branch.c
@@ -291,7 +291,7 @@ void create_branch(const char *head,
hashcpy(sha1, commit-object.sha1);
 
if (!dont_change_ref) {
-   lock = lock_any_ref_for_update(ref.buf, NULL, 0);
+   lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
if (!lock)
die_errno(_(Failed to lock ref for update));
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 10acc53..be08f41 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1618,7 +1618,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
   !current_head
   ? NULL
   : current_head-object.sha1,
-  0);
+  0, NULL);
 
nl = strchr(sb.buf, '\n');
if (nl)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index d784b2e..28e4025 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -246,7 +246,8 @@ static int s_update_ref(const char *action,
rla = default_rla.buf;
snprintf(msg, sizeof(msg), %s: %s, rla, action);
lock = lock_any_ref_for_update(ref-name,
-  check_old ? ref-old_sha1 : NULL, 0);
+  check_old ? ref-old_sha1 : NULL,
+  0, NULL);
if (!lock)
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
  STORE_REF_ERROR_OTHER;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e3eb5fc..a323070 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -524,7 +524,8 @@ static const char *update(struct command *cmd)
return NULL; /* good */
}
else {
-   lock = lock_any_ref_for_update(namespaced_name, old_sha1, 0);
+   lock = lock_any_ref_for_update(namespaced_name, old_sha1,
+  0, NULL);
if (!lock) {
rp_error(failed to lock %s, name);
return failed to lock;
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 54184b3..28d756a 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -366,7 +366,7 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, int unused,
 * we take the lock for the ref itself to prevent it from
 * getting updated.
 */
-   lock = lock_any_ref_for_update(ref, sha1, 0);
+   lock = lock_any_ref_for_update(ref, sha1, 0, NULL);
if (!lock)
return error(cannot lock ref '%s', ref);
log_file = git_pathdup(logs/%s, ref);
diff --git a/builtin/replace.c b/builtin/replace.c
index 59d3115..1ecae8d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -105,7 +105,7 @@ static int replace_object(const char *object_ref, const 
char *replace_ref,
else if (!force)
die(replace ref '%s' already exists, ref);
 
-   lock = lock_any_ref_for_update(ref, prev, 0);
+   lock = lock_any_ref_for_update(ref, prev, 0, NULL);
if (!lock)
die(%s: cannot lock the ref, ref);
if (write_ref_sha1(lock, repl, NULL)  0)
diff --git a/builtin/tag.c b/builtin/tag.c
index af3af3f..2c867d2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -577,7 +577,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (annotate)
create_tag(object, tag, buf, opt, prev, object);
 
-   lock = lock_any_ref_for_update(ref.buf, prev, 0);
+   lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
if (!lock)
die(_(%s: cannot lock the ref), ref.buf);
if (write_ref_sha1(lock, object, NULL)  0)
diff --git a/fast-import.c b/fast-import.c
index 23f625f..5c329f6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1678,7 +1678,7 @@ static int update_branch(struct branch *b)
return 0;
if (read_ref(b-name, old_sha1))
hashclr(old_sha1);
-   lock = lock_any_ref_for_update(b-name, old_sha1, 0);
+   lock = lock_any_ref_for_update(b-name, old_sha1

[PATCH v2 5/8] refs: add function to repack without multiple refs

[Brad King]

Generalize repack_without_ref as repack_without_refs to support a list
of refs and implement the former in terms of the latter.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c |   29 ++---
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 5dd86ee..3bcd26e 100644
--- a/refs.c
+++ b/refs.c
@@ -2414,25 +2414,35 @@ static int curate_packed_ref_fn(struct ref_entry 
*entry, void *cb_data)
return 0;
 }
 
-static int repack_without_ref(const char *refname)
+static int repack_without_refs(const char **refnames, int n)
 {
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
+   int i, removed = 0;
+
+   /* Look for a packed ref: */
+   for (i = 0; i  n; ++i)
+   if (get_packed_ref(refnames[i]))
+   break;
 
-   if (!get_packed_ref(refname))
-   return 0; /* refname does not exist in packed refs */
+   /* Avoid locking if we have nothing to do: */
+   if (i == n)
+   return 0; /* no refname exists in packed refs */
 
if (lock_packed_refs(0)) {
unable_to_lock_error(git_path(packed-refs), errno);
-   return error(cannot delete '%s' from packed refs, refname);
+   return error(cannot delete '%s' from packed refs, 
refnames[i]);
}
packed = get_packed_refs(ref_cache);
 
-   /* Remove refname from the cache: */
-   if (remove_entry(packed, refname) == -1) {
+   /* Remove refnames from the cache: */
+   for (i = 0; i  n; ++i)
+   if (remove_entry(packed, refnames[i]) != -1)
+   removed = 1;
+   if (!removed) {
/*
-* The packed entry disappeared while we were
+* All packed entries disappeared while we were
 * acquiring the lock.
 */
rollback_packed_refs();
@@ -2450,6 +2460,11 @@ static int repack_without_ref(const char *refname)
return commit_packed_refs();
 }
 
+static int repack_without_ref(const char *refname)
+{
+   return repack_without_refs(refname, 1);
+}
+
 static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
int err, i, ret = 0;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 6/8] refs: add update_refs for multiple simultaneous updates

[Brad King]

Add 'struct ref_update' to encode the information needed to update or
delete a ref (name, new sha1, optional old sha1, no-deref flag).  Add
function 'update_refs' accepting an array of updates to perform.  First
sort the input array to order locks consistently everywhere and reject
multiple updates to the same ref.  Then acquire locks on all refs with
verified old values.  Then update or delete all refs accordingly.  Fail
if any one lock cannot be obtained or any one old value does not match.

Though the refs themeselves cannot be modified together in a single
atomic transaction, this function does enable some useful semantics.
For example, a caller may create a new branch starting from the head of
another branch and rewind the original branch at the same time.  This
transfers ownership of commits between branches without risk of losing
commits added to the original branch by a concurrent process, or risk of
a concurrent process creating the new branch first.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c |  121 
 refs.h |   14 
 2 files changed, 135 insertions(+)

diff --git a/refs.c b/refs.c
index 3bcd26e..901a38a 100644
--- a/refs.c
+++ b/refs.c
@@ -3238,6 +3238,127 @@ int update_ref(const char *action, const char *refname,
return update_ref_write(action, refname, sha1, lock, onerr);
 }
 
+static int ref_update_compare(const void *r1, const void *r2)
+{
+   struct ref_update *u1 = (struct ref_update *)(r1);
+   struct ref_update *u2 = (struct ref_update *)(r2);
+   int ret;
+   ret = strcmp(u1-ref_name, u2-ref_name);
+   if (ret)
+   return ret;
+   ret = hashcmp(u1-new_sha1, u2-new_sha1);
+   if (ret)
+   return ret;
+   ret = hashcmp(u1-old_sha1, u2-old_sha1);
+   if (ret)
+   return ret;
+   ret = u1-flags - u2-flags;
+   if (ret)
+   return ret;
+   return u1-have_old - u2-have_old;
+}
+
+static int ref_update_reject_duplicates(struct ref_update *updates, int n,
+   enum action_on_err onerr)
+{
+   int i;
+   for (i = 1; i  n; ++i)
+   if (!strcmp(updates[i - 1].ref_name, updates[i].ref_name))
+   break;
+   if (i  n) {
+   const char *str = Multiple updates for ref '%s' not allowed.;
+   switch (onerr) {
+   case MSG_ON_ERR: error(str, updates[i].ref_name); break;
+   case DIE_ON_ERR: die(str, updates[i].ref_name); break;
+   case QUIET_ON_ERR: break;
+   }
+   return 1;
+   }
+   return 0;
+}
+
+int update_refs(const char *action, const struct ref_update *updates_orig,
+   int n, enum action_on_err onerr)
+{
+   int ret = 0, delnum = 0, i;
+   struct ref_update *updates;
+   int *types;
+   struct ref_lock **locks;
+   const char **delnames;
+
+   if (!updates_orig || !n)
+   return 0;
+
+   /* Allocate work space: */
+   updates = xmalloc(sizeof(struct ref_update) * n);
+   types = xmalloc(sizeof(int) * n);
+   locks = xmalloc(sizeof(struct ref_lock *) * n);
+   delnames = xmalloc(sizeof(const char *) * n);
+
+   /* Copy, sort, and reject duplicate refs: */
+   memcpy(updates, updates_orig, sizeof(struct ref_update) * n);
+   qsort(updates, n, sizeof(struct ref_update), ref_update_compare);
+   if (ref_update_reject_duplicates(updates, n, onerr)) {
+   free(updates);
+   free(types);
+   free(locks);
+   free(delnames);
+   return 1;
+   }
+
+   /* Acquire all locks while verifying old values: */
+   for (i = 0; i  n; ++i) {
+   locks[i] = update_ref_lock(updates[i].ref_name,
+  (updates[i].have_old ?
+   updates[i].old_sha1 : NULL),
+  updates[i].flags,
+  types[i], onerr);
+   if (!locks[i])
+   break;
+   }
+
+   /* Abort if we did not get all locks: */
+   if (i  n) {
+   while (--i = 0)
+   unlock_ref(locks[i]);
+   free(updates);
+   free(types);
+   free(locks);
+   free(delnames);
+   return 1;
+   }
+
+   /* Perform updates first so live commits remain referenced: */
+   for (i = 0; i  n; ++i)
+   if (!is_null_sha1(updates[i].new_sha1)) {
+   ret |= update_ref_write(action,
+   updates[i].ref_name,
+   updates[i].new_sha1,
+   locks[i], onerr);
+   locks[i] = 0; /* freed

[PATCH v2 7/8] update-ref: support multiple simultaneous updates

[Brad King]

Add a --stdin signature to read update instructions from standard input
and apply multiple ref updates together.  Use an input format that
supports any update that could be specified via the command-line,
including object names like 'branch:path with space'.

Signed-off-by: Brad King brad.k...@kitware.com
---
 Documentation/git-update-ref.txt |   21 ++-
 builtin/update-ref.c |  121 +-
 2 files changed, 140 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 0df13ff..295d0bb 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -8,7 +8,7 @@ git-update-ref - Update the object name stored in a ref safely
 SYNOPSIS
 
 [verse]
-'git update-ref' [-m reason] (-d ref [oldvalue] | [--no-deref] ref 
newvalue [oldvalue])
+'git update-ref' [-m reason] (-d ref [oldvalue] | [--no-deref] ref 
newvalue [oldvalue] | --stdin)
 
 DESCRIPTION
 ---
@@ -58,6 +58,25 @@ archive by creating a symlink tree).
 With `-d` flag, it deletes the named ref after verifying it
 still contains oldvalue.
 
+With `--stdin`, update-ref reads instructions from standard input and
+performs all modifications together.  Empty lines are ignored.
+Each non-empty line is parsed as whitespace-separated arguments.
+Use single-quotes to enclose whitespace and backslashes and an
+unquoted backslash to escape a single quote.  Specify updates with
+lines of the form:
+
+   [--no-deref] [--] ref newvalue [oldvalue]
+
+Lines of any other format or a repeated ref produce an error.
+Specify a zero newvalue to delete a ref and/or a zero oldvalue
+to make sure that a ref not exist.  Use either 40 0 or the
+empty string (written as '') to specify a zero value.
+
+If all refs can be locked with matching oldvalues
+simultaneously all modifications are performed.  Otherwise, no
+modifications are performed.  Note that while each individual
+ref is updated or deleted atomically, a concurrent reader may
+still see a subset of the modifications.
 
 Logging Updates
 ---
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 51d2684..eb8db85 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -6,19 +6,129 @@
 static const char * const git_update_ref_usage[] = {
N_(git update-ref [options] -d refname [oldval]),
N_(git update-ref [options]refname newval [oldval]),
+   N_(git update-ref [options] --stdin),
NULL
 };
 
+static const char blank[] =  \t\r\n;
+
+static int updates_size;
+static int updates_count;
+static struct ref_update *updates;
+
+static const char* update_refs_stdin_next_arg(const char* next,
+ struct strbuf *arg)
+{
+   /* Skip leading whitespace: */
+   while (isspace(*next))
+   ++next;
+
+   /* Return NULL when no argument is found: */
+   if (!*next)
+   return NULL;
+
+   /* Parse the argument: */
+   strbuf_reset(arg);
+   for (;;) {
+   char c = *next;
+   if (!c || isspace(c))
+   break;
+   ++next;
+   if (c == '\'') {
+   size_t len = strcspn(next, ');
+   if (!next[len])
+   die(unterminated single-quote: '%s, next);
+   strbuf_add(arg, next, len);
+   next += len + 1;
+   continue;
+   }
+   if (c == '\\') {
+   if (*next == '\'')
+   c = *next++;
+   else
+   die(unquoted backslash not escaping 
+   single-quote: \\%s, next);
+   }
+   strbuf_addch(arg, c);
+   }
+   return next;
+}
+
+static void update_refs_stdin(const char *line)
+{
+   int options = 1, flags = 0, argc = 0;
+   char *argv[3] = {0, 0, 0};
+   struct strbuf arg = STRBUF_INIT;
+   struct ref_update *update;
+   const char *next = line;
+
+   /* Skip blank lines: */
+   if (!line[0])
+   return;
+
+   /* Parse arguments on this line: */
+   while ((next = update_refs_stdin_next_arg(next, arg)) != NULL) {
+   if (options  arg.buf[0] == '-')
+   if (!strcmp(arg.buf, --no-deref))
+   flags |= REF_NODEREF;
+   else if (!strcmp(arg.buf, --))
+   options = 0;
+   else
+   die(unknown option %s, arg.buf);
+   else if (argc = 3)
+   die(too many arguments on line: %s, line);
+   else {
+   argv[argc++] = xstrdup(arg.buf);
+   options = 0;
+   }
+   }
+   strbuf_release(arg

[PATCH/RFC 7/7] update-ref: support multiple simultaneous updates

[Brad King]

Add a --stdin signature to read update instructions from standard input
and apply multiple ref updates and deletes together.

Signed-off-by: Brad King brad.k...@kitware.com
---
 Documentation/git-update-ref.txt |   19 +++-
 builtin/update-ref.c |   93 +-
 2 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 0df13ff..a79afe8 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -8,7 +8,7 @@ git-update-ref - Update the object name stored in a ref safely
 SYNOPSIS
 
 [verse]
-'git update-ref' [-m reason] (-d ref [oldvalue] | [--no-deref] ref 
newvalue [oldvalue])
+'git update-ref' [-m reason] (-d ref [oldvalue] | [--no-deref] ref 
newvalue [oldvalue] | --stdin)
 
 DESCRIPTION
 ---
@@ -58,6 +58,23 @@ archive by creating a symlink tree).
 With `-d` flag, it deletes the named ref after verifying it
 still contains oldvalue.
 
+With `--stdin`, update-ref reads instructions from standard input
+and performs all modifications together.  Specify updates with
+lines of the form:
+
+   [ --no-deref SP ] ref SP newvalue [ SP oldvalue ] LF
+
+and deletes with lines of the form:
+
+   [ --no-deref SP ] -d SP ref [ SP oldvalue ] LF
+
+or as updates with 40 0 as newvalue.  Blank lines are ignored.
+Lines of any other format or a repeated ref produce an error.
+If all refs can be locked with matching oldvalues
+simultaneously all modifications are performed.  Otherwise, no
+modifications are performed.  Note that while each individual
+ref is updated or deleted atomically, a concurrent reader may
+still see a subset of the modifications.
 
 Logging Updates
 ---
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 51d2684..2f0d34c 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -6,19 +6,102 @@
 static const char * const git_update_ref_usage[] = {
N_(git update-ref [options] -d refname [oldval]),
N_(git update-ref [options]refname newval [oldval]),
+   N_(git update-ref [options] --stdin),
NULL
 };
 
+static const char blank[] =  \t\r\n;
+
+static int updates_size;
+static int updates_count;
+static struct ref_update *updates;
+
+static void update_refs_stdin(const char *line)
+{
+   int delete = 0, i;
+   const char *c, *s, *oldvalue, *value[2] = {0,0};
+   struct ref_update *update;
+   c = line;
+
+   /* Skip blank lines: */
+   if (*c == '\n')
+   return;
+
+   /* Allocate a ref_update struct: */
+   if (updates_count == updates_size) {
+   updates_size = updates_size? updates_size*2 : 16;
+   updates = xrealloc(updates, sizeof(*updates)*updates_size);
+   memset(updates + updates_count, 0,
+  sizeof(*updates)*(updates_size-updates_count));
+   }
+   update = updates[updates_count++];
+
+   /* --no-deref SP */
+   if (strncmp(c, --no-deref , 11) == 0) {
+   c += 11;
+   update-flags |= REF_NODEREF;
+   }
+
+   /* -d SP */
+   if (strncmp(c, -d , 3) == 0) {
+   c += 3;
+   delete = 1;
+   }
+
+   /* ref */
+   s = c;
+   c = s + strcspn(s, blank);
+   update-ref_name = xstrndup(s, c-s);
+
+   /* [ SP value ]... */
+   for (i=0; i  2; ++i) {
+   if (*c != ' ')
+   break;
+   ++c;
+   s = c;
+   c = s + strcspn(s, blank);
+   value[i] = xstrndup(s, c-s);
+   }
+
+   if (*c  *c != '\n')
+   die(unrecognized input line: %s, line);
+
+   if (check_refname_format(update-ref_name, REFNAME_ALLOW_ONELEVEL))
+   die(invalid ref format on input line: %s, line);
+
+   if (delete) {
+   hashclr(update-new_sha1);
+   oldvalue = value[0];
+   if (value[1])
+   die(both newvalue and oldvalue on delete line: %s,
+   line);
+   } else {
+   if (!value[0])
+   die(missing newvalue on update line: %s, line);
+   if (get_sha1(value[0], update-new_sha1))
+   die(invalid newvalue on update line: %s, line);
+   oldvalue = value[1];
+   }
+   if (oldvalue) {
+   update-old_sha1 = xmalloc(20);
+   if (get_sha1(oldvalue, update-old_sha1))
+   die(invalid oldvalue on %s line: %s,
+   delete? delete:update, line);
+   }
+}
+
 int cmd_update_ref(int argc, const char **argv, const char *prefix)
 {
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
-   int delete = 0, no_deref = 0, flags = 0;
+   int delete = 0, no_deref = 0, read_stdin = 0, flags = 0;
+   char line[1000

[PATCH/RFC 6/7] refs: add update_refs for multiple simultaneous updates

[Brad King]

Add 'struct ref_update' to encode the information needed to update or
delete a ref (name, new sha1, optional old sha1, no-deref flag).  Add
function 'update_refs' accepting an array of updates to perform.  First
acquire locks on all refs with verified old values.  Then update or
delete all refs accordingly.  Fail if any one lock cannot be obtained or
any one old value does not match.

Though the refs themeselves cannot be modified together in a single
atomic transaction, this function does enable some useful semantics.
For example, a caller may create a new branch starting from the head of
another branch and rewind the original branch at the same time.  This
transfers ownership of commits between branches without risk of losing
commits added to the original branch by a concurrent process, or risk of
a concurrent process creating the new branch first.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c |   66 
 refs.h |   11 +++
 2 files changed, 77 insertions(+)

diff --git a/refs.c b/refs.c
index 5a6c14e..0a0c19e 100644
--- a/refs.c
+++ b/refs.c
@@ -3238,6 +3238,72 @@ int update_ref(const char *action, const char *refname,
return update_ref_write(action, refname, sha1, lock, onerr);
 }
 
+int update_refs(const char *action, struct ref_update *updates,
+   int n, enum action_on_err onerr)
+{
+   int ret = 0, delnum = 0, i;
+   int *types;
+   struct ref_lock **locks;
+   const char **delnames;
+
+   if (!updates || !n)
+   return 0;
+
+   /* Allocate work space: */
+   types = xmalloc(sizeof(int)*n);
+   locks = xmalloc(sizeof(struct ref_lock*)*n);
+   delnames = xmalloc(sizeof(const char*)*n);
+
+   /* Acquire all locks while verifying old values: */
+   for (i=0; i  n; ++i) {
+   locks[i] = update_ref_lock(updates[i].ref_name,
+  updates[i].old_sha1,
+  updates[i].flags,
+  types[i], onerr);
+   if (!locks[i])
+   break;
+   }
+
+   /* Abort if we did not get all locks: */
+   if (i  n) {
+   while (--i = 0)
+   unlock_ref(locks[i]);
+   free(types);
+   free(locks);
+   free(delnames);
+   return 1;
+   }
+
+   /* Perform updates first so live commits remain referenced: */
+   for (i=0; i  n; ++i)
+   if (!is_null_sha1(updates[i].new_sha1)) {
+   ret |= update_ref_write(action,
+   updates[i].ref_name,
+   updates[i].new_sha1,
+   locks[i], onerr);
+   locks[i] = 0; /* freed by update_ref_write */
+   }
+
+   /* Perform deletes now that updates are safely completed: */
+   for (i=0; i  n; ++i)
+   if (locks[i]) {
+   delnames[delnum++] = locks[i]-ref_name;
+   ret |= delete_ref_loose(locks[i], types[i]);
+   }
+   ret |= repack_without_refs(delnames, delnum);
+   for (i=0; i  delnum; ++i)
+   unlink_or_warn(git_path(logs/%s, delnames[i]));
+   clear_loose_ref_cache(ref_cache);
+   for (i=0; i  n; ++i)
+   if (locks[i])
+   unlock_ref(locks[i]);
+
+   free(types);
+   free(locks);
+   free(delnames);
+   return ret;
+}
+
 struct ref *find_ref_by_name(const struct ref *list, const char *name)
 {
for ( ; list; list = list-next)
diff --git a/refs.h b/refs.h
index 2cd307a..5763f3a 100644
--- a/refs.h
+++ b/refs.h
@@ -214,6 +214,17 @@ int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
int flags, enum action_on_err onerr);
 
+struct ref_update {
+   const char *ref_name;
+   unsigned char new_sha1[20];
+   unsigned char *old_sha1;
+   int flags;
+};
+
+/** lock all refs and then write all of them */
+int update_refs(const char *action, struct ref_update *updates,
+   int n, enum action_on_err onerr);
+
 extern int parse_hide_refs_config(const char *var, const char *value, const 
char *);
 extern int ref_is_hidden(const char *);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 1/7] reset: rename update_refs to reset_refs

[Brad King]

Get it out of the way for a future refs.h function.

Signed-off-by: Brad King brad.k...@kitware.com
---
 builtin/reset.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index afa6e02..789ee48 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -219,7 +219,7 @@ static const char **parse_args(const char **argv, const 
char *prefix, const char
return argv[0] ? get_pathspec(prefix, argv) : NULL;
 }
 
-static int update_refs(const char *rev, const unsigned char *sha1)
+static int reset_refs(const char *rev, const unsigned char *sha1)
 {
int update_ref_status;
struct strbuf msg = STRBUF_INIT;
@@ -350,7 +350,7 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
if (!pathspec  !unborn) {
/* Any resets without paths update HEAD to the head being
 * switched to, saving the previous head in ORIG_HEAD before. */
-   update_ref_status = update_refs(rev, sha1);
+   update_ref_status = reset_refs(rev, sha1);
 
if (reset_type == HARD  !update_ref_status  !quiet)
print_new_head_line(lookup_commit_reference(sha1));
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 4/7] refs: factor delete_ref loose ref step into a helper

[Brad King]

Factor loose ref deletion into helper function delete_ref_loose to allow
later use elsewhere.  While at it, rename local names 'flag = type' and
'delopt = flags' for consistency with callers and called functions.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c |   24 
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 2e755b4..5908648 100644
--- a/refs.c
+++ b/refs.c
@@ -2450,15 +2450,10 @@ static int repack_without_ref(const char *refname)
return commit_packed_refs();
 }
 
-int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
+static int delete_ref_loose(struct ref_lock *lock, int type)
 {
-   struct ref_lock *lock;
-   int err, i = 0, ret = 0, flag = 0;
-
-   lock = lock_ref_sha1_basic(refname, sha1, delopt, flag);
-   if (!lock)
-   return 1;
-   if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
+   int err, i, ret = 0;
+   if (!(type  REF_ISPACKED) || type  REF_ISSYMREF) {
/* loose */
i = strlen(lock-lk-filename) - 5; /* .lock */
lock-lk-filename[i] = 0;
@@ -2468,6 +2463,19 @@ int delete_ref(const char *refname, const unsigned char 
*sha1, int delopt)
 
lock-lk-filename[i] = '.';
}
+   return ret;
+}
+
+int delete_ref(const char *refname, const unsigned char *sha1, int flags)
+{
+   struct ref_lock *lock;
+   int ret = 0, type = 0;
+
+   lock = lock_ref_sha1_basic(refname, sha1, flags, type);
+   if (!lock)
+   return 1;
+   ret |= delete_ref_loose(lock, type);
+
/* removing the loose one could have resurrected an earlier
 * packed one.  Also, if it was not loose we need to repack
 * without it.
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 5/7] refs: add function to repack without multiple refs

[Brad King]

Generalize repack_without_ref as repack_without_refs to support a list
of refs and implement the former in terms of the latter.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c |   29 ++---
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 5908648..5a6c14e 100644
--- a/refs.c
+++ b/refs.c
@@ -2414,25 +2414,35 @@ static int curate_packed_ref_fn(struct ref_entry 
*entry, void *cb_data)
return 0;
 }
 
-static int repack_without_ref(const char *refname)
+static int repack_without_refs(const char **refnames, int n)
 {
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
+   int i, removed = 0;
+
+   /* Look for a packed ref: */
+   for (i = 0; i  n; ++i)
+   if (get_packed_ref(refnames[i]))
+   break;
 
-   if (!get_packed_ref(refname))
-   return 0; /* refname does not exist in packed refs */
+   /* Avoid locking if we have nothing to do: */
+   if(i == n)
+   return 0; /* no refname exists in packed refs */
 
if (lock_packed_refs(0)) {
unable_to_lock_error(git_path(packed-refs), errno);
-   return error(cannot delete '%s' from packed refs, refname);
+   return error(cannot delete '%s' from packed refs, 
refnames[i]);
}
packed = get_packed_refs(ref_cache);
 
-   /* Remove refname from the cache: */
-   if (remove_entry(packed, refname) == -1) {
+   /* Remove refnames from the cache: */
+   for (i = 0; i  n; ++i)
+   if (remove_entry(packed, refnames[i]) != -1)
+   removed = 1;
+   if (!removed) {
/*
-* The packed entry disappeared while we were
+* All packed entries disappeared while we were
 * acquiring the lock.
 */
rollback_packed_refs();
@@ -2450,6 +2460,11 @@ static int repack_without_ref(const char *refname)
return commit_packed_refs();
 }
 
+static int repack_without_ref(const char *refname)
+{
+   return repack_without_refs(refname, 1);
+}
+
 static int delete_ref_loose(struct ref_lock *lock, int type)
 {
int err, i, ret = 0;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 2/7] refs: report ref type from lock_any_ref_for_update

[Brad King]

Expose lock_ref_sha1_basic's type_p argument to callers of
lock_any_ref_for_update.  Update all call sites to ignore it; we will
use it later.

Signed-off-by: Brad King brad.k...@kitware.com
---
 branch.c   |2 +-
 builtin/commit.c   |2 +-
 builtin/fetch.c|2 +-
 builtin/receive-pack.c |2 +-
 builtin/reflog.c   |2 +-
 builtin/replace.c  |2 +-
 builtin/tag.c  |2 +-
 fast-import.c  |2 +-
 refs.c |7 ---
 refs.h |2 +-
 sequencer.c|2 +-
 11 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/branch.c b/branch.c
index c5c6984..c244483 100644
--- a/branch.c
+++ b/branch.c
@@ -291,7 +291,7 @@ void create_branch(const char *head,
hashcpy(sha1, commit-object.sha1);
 
if (!dont_change_ref) {
-   lock = lock_any_ref_for_update(ref.buf, NULL, 0);
+   lock = lock_any_ref_for_update(ref.buf, NULL, 0, 0);
if (!lock)
die_errno(_(Failed to lock ref for update));
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 10acc53..78d773f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1618,7 +1618,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
   !current_head
   ? NULL
   : current_head-object.sha1,
-  0);
+  0, 0);
 
nl = strchr(sb.buf, '\n');
if (nl)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index d784b2e..34903ef 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -246,7 +246,7 @@ static int s_update_ref(const char *action,
rla = default_rla.buf;
snprintf(msg, sizeof(msg), %s: %s, rla, action);
lock = lock_any_ref_for_update(ref-name,
-  check_old ? ref-old_sha1 : NULL, 0);
+  check_old ? ref-old_sha1 : NULL, 0, 0);
if (!lock)
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
  STORE_REF_ERROR_OTHER;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e3eb5fc..dd61234 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -524,7 +524,7 @@ static const char *update(struct command *cmd)
return NULL; /* good */
}
else {
-   lock = lock_any_ref_for_update(namespaced_name, old_sha1, 0);
+   lock = lock_any_ref_for_update(namespaced_name, old_sha1, 0, 0);
if (!lock) {
rp_error(failed to lock %s, name);
return failed to lock;
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 54184b3..11b30f9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -366,7 +366,7 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, int unused,
 * we take the lock for the ref itself to prevent it from
 * getting updated.
 */
-   lock = lock_any_ref_for_update(ref, sha1, 0);
+   lock = lock_any_ref_for_update(ref, sha1, 0, 0);
if (!lock)
return error(cannot lock ref '%s', ref);
log_file = git_pathdup(logs/%s, ref);
diff --git a/builtin/replace.c b/builtin/replace.c
index 59d3115..e2e2002 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -105,7 +105,7 @@ static int replace_object(const char *object_ref, const 
char *replace_ref,
else if (!force)
die(replace ref '%s' already exists, ref);
 
-   lock = lock_any_ref_for_update(ref, prev, 0);
+   lock = lock_any_ref_for_update(ref, prev, 0, 0);
if (!lock)
die(%s: cannot lock the ref, ref);
if (write_ref_sha1(lock, repl, NULL)  0)
diff --git a/builtin/tag.c b/builtin/tag.c
index af3af3f..c261469 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -577,7 +577,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (annotate)
create_tag(object, tag, buf, opt, prev, object);
 
-   lock = lock_any_ref_for_update(ref.buf, prev, 0);
+   lock = lock_any_ref_for_update(ref.buf, prev, 0, 0);
if (!lock)
die(_(%s: cannot lock the ref), ref.buf);
if (write_ref_sha1(lock, object, NULL)  0)
diff --git a/fast-import.c b/fast-import.c
index 23f625f..5f7ef82 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1678,7 +1678,7 @@ static int update_branch(struct branch *b)
return 0;
if (read_ref(b-name, old_sha1))
hashclr(old_sha1);
-   lock = lock_any_ref_for_update(b-name, old_sha1, 0);
+   lock = lock_any_ref_for_update(b-name, old_sha1, 0, 0);
if (!lock)
return error(Unable to lock %s, b-name);
if (!force_update

[PATCH/RFC 0/7] Multiple simultaneously locked ref updates

[Brad King]

Hi Folks,

While thinking about some how some server-side branch management
services might work, I came across a need to update multiple refs
locked with verified old values simultaneously.  For example, to
transfer ownership of some commits by rewinding a branch and creating
a new branch at the original head, one must lock both refs.
Otherwise, depending on the order of updates another process could
create the new branch after we've rewound the original, or add commits
to the original after we've created the new branch.

This series teaches update-ref a new --stdin option to read update and
delete instructions from lines of standard input, lock all refs up
front with verified old values, and then perform the modifications.
This is still work in progress, but it is ready for comments and
feedback.  The series is based on master as of v1.8.4.

Notable unfinished work:

* I propose a format for stdin lines in the last commit of the series
  as a proof-of-concept but I invite suggestions of better formats.
  The format must be able to specify updates and deletes with optional
  old values and optional no-deref.

* No tests for new features, though existing tests pass for me.

* No check for duplicate refs in input.  Currently a duplicate ref
  will result in a failure message like:

   fatal: Unable to create 'lock': File exists.
   If no other git process is currently running, this probably means a
   git process crashed in this repository earlier. Make sure no other git
   process is running and remove the file manually to continue.

  Instead we should reject duplicate ref names up front.  I would
  appreciate suggestions about an efficient data structure already
  available in Git to perform this lookup.

I welcome feedback on the approach, interface, and implementation.

Thanks,
-Brad

Brad King (7):
  reset: rename update_refs to reset_refs
  refs: report ref type from lock_any_ref_for_update
  refs: factor update_ref steps into helpers
  refs: factor delete_ref loose ref step into a helper
  refs: add function to repack without multiple refs
  refs: add update_refs for multiple simultaneous updates
  update-ref: support multiple simultaneous updates

 Documentation/git-update-ref.txt |   19 -
 branch.c |2 +-
 builtin/commit.c |2 +-
 builtin/fetch.c  |2 +-
 builtin/receive-pack.c   |2 +-
 builtin/reflog.c |2 +-
 builtin/replace.c|2 +-
 builtin/reset.c  |4 +-
 builtin/tag.c|2 +-
 builtin/update-ref.c |   93 ++-
 fast-import.c|2 +-
 refs.c   |  150 --
 refs.h   |   13 +++-
 sequencer.c  |2 +-
 14 files changed, 262 insertions(+), 35 deletions(-)

-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 3/7] refs: factor update_ref steps into helpers

[Brad King]

Factor the lock and write steps and error handling into helper functions
update_ref_lock and update_ref_write to allow later use elsewhere.
Expose lock_any_ref_for_update's type_p to update_ref_lock callers.

Signed-off-by: Brad King brad.k...@kitware.com
---
 refs.c |   28 +++-
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 3a7b597..2e755b4 100644
--- a/refs.c
+++ b/refs.c
@@ -3170,12 +3170,13 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
 }
 
-int update_ref(const char *action, const char *refname,
-   const unsigned char *sha1, const unsigned char *oldval,
-   int flags, enum action_on_err onerr)
+static struct ref_lock *update_ref_lock(const char *refname,
+   const unsigned char *oldval,
+   int flags, int *type_p,
+   enum action_on_err onerr)
 {
static struct ref_lock *lock;
-   lock = lock_any_ref_for_update(refname, oldval, flags, 0);
+   lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
if (!lock) {
const char *str = Cannot lock the ref '%s'.;
switch (onerr) {
@@ -3183,8 +3184,14 @@ int update_ref(const char *action, const char *refname,
case DIE_ON_ERR: die(str, refname); break;
case QUIET_ON_ERR: break;
}
-   return 1;
}
+   return lock;
+}
+
+static int update_ref_write(const char *action, const char *refname,
+   const unsigned char *sha1, struct ref_lock *lock,
+   enum action_on_err onerr)
+{
if (write_ref_sha1(lock, sha1, action)  0) {
const char *str = Cannot update the ref '%s'.;
switch (onerr) {
@@ -3197,6 +3204,17 @@ int update_ref(const char *action, const char *refname,
return 0;
 }
 
+int update_ref(const char *action, const char *refname,
+  const unsigned char *sha1, const unsigned char *oldval,
+  int flags, enum action_on_err onerr)
+{
+   static struct ref_lock *lock;
+   lock = update_ref_lock(refname, oldval, flags, 0, onerr);
+   if (!lock)
+   return 1;
+   return update_ref_write(action, refname, sha1, lock, onerr);
+}
+
 struct ref *find_ref_by_name(const struct ref *list, const char *name)
 {
for ( ; list; list = list-next)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 0/7] Multiple simultaneously locked ref updates

[Brad King]

On 08/29/2013 11:32 AM, Martin Fick wrote:
 On Thursday, August 29, 2013 08:11:48 am Brad King wrote:

fatal: Unable to create 'lock': File exists.
If no other git process is currently running, this
 probably means a git process crashed in this repository
 earlier. Make sure no other git process is running and
 remove the file manually to continue.
 
 I don't believe git currently tries to do any form of stale 
 lock recovery since it is racy and unreliable (both single 
 server or on a multi-server shared repo),

Nor should it in this case.  I was saying that the front-end
needs to reject duplicate ref names from the stdin lines before
trying to lock the ref twice to avoid this message.  I'm asking
for a suggestion for existing data structure capabilities in
Git's source to efficiently detect the duplicate name.

-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 0/7] Multiple simultaneously locked ref updates

[Brad King]

On 08/29/2013 12:21 PM, Junio C Hamano wrote:
 Brad King brad.k...@kitware.com writes:
 needs to reject duplicate ref names from the stdin lines before
 trying to lock the ref twice to avoid this message.
 
 How about trying not to feed duplicates?

Sure, perhaps it is simplest to push the responsibility on the user
to avoid duplicates.  However, the error message will need to be
re-worded to distinguish this case from a stale lock or competing
process since both locks may come from the same update-ref process.

Without checking the input for duplicates ourselves we cannot
distinguish these cases to provide a more informative error message.
However, such a check would add runtime overhead even for valid input.
If we prefer to avoid input validation then here is proposed new
wording for the lock failure message:


fatal: Unable to create 'lock': File exists.

The lock file may exist because:
- another running git process already has the lock, or
- this process already has the lock because it was asked to
  update the same file multiple times simultaneously, or
- a stale lock is left from a git process that crashed earlier.
In the last case, make sure no other git process is running and
remove the file manually to continue.


IIUC the message cannot say anything about a 'ref' because it is
used for other file type lock failures too.

Comments?
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 1/7] reset: rename update_refs to reset_refs

[Brad King]

On 08/29/2013 01:17 PM, Junio C Hamano wrote:
 Brad King brad.k...@kitware.com writes:
 
 Get it out of the way for a future refs.h function.
 
 Readers do not know if update_refs() is a good name for that
 future refs.h function at this point, so evict squatter is not a
 very good justification by itself.  I do agree that this static
 function is resetting a ref, not doing an arbitrary update, and the
 new name is a better match than the old one for what it does, though.

Okay, I've revised the commit message for the next revision.

-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 2/7] refs: report ref type from lock_any_ref_for_update

[Brad King]

On 08/29/2013 01:22 PM, Junio C Hamano wrote:
 If you are passing an NULL as a new parameter, please spell it
 NULL, not 0.

Fixed at all updated call sites.

-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 4/7] refs: factor delete_ref loose ref step into a helper

[Brad King]

On 08/29/2013 01:28 PM, Junio C Hamano wrote:
 Brad King brad.k...@kitware.com writes:
 -if (!(flag  REF_ISPACKED) || flag  REF_ISSYMREF) {
 +if (!(type  REF_ISPACKED) || type  REF_ISSYMREF) {
 
 Hits from git grep REF_IS tell me that all users of REF_IS* symbol
 that check if a bit is on in a word does so against flag (either a
 variable called flag, flags, or a structure member .flag).
 
 This change is making things less consistent, not more, I am afraid.

Okay, I removed this part of the change.  It makes the commit
simpler anyway.

-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 5/7] refs: add function to repack without multiple refs

[Brad King]

On 08/29/2013 01:34 PM, Junio C Hamano wrote:
 Brad King brad.k...@kitware.com writes:
 +if(i == n)
 
 Style:
   if (i == n)

Fixed in next revision.

-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 6/7] refs: add update_refs for multiple simultaneous updates

[Brad King]

On 08/29/2013 01:39 PM, Junio C Hamano wrote:
 Brad King brad.k...@kitware.com writes:
 +for (i=0; i  n; ++i) {
 
 Style:
 
   for (i = 0; i  n; i++) {

Fixed.

 Is it asking for AB-BA deadlock?  If so, is the caller responsible
 for avoiding it?

Since we don't actually block waiting for locks we won't really
deadlock.  However, if two competing callers keep repeating they
might never finish.  In order to avoid this one must sort the refs
so that locks are always acquired in a consistent order.

For Git's internal API I think we can document this in a comment so
that update_refs does not have to sort.  Then we can add a new
ref_update_sort function to sort an array of struct ref_update.
The user-facing update-ref --stdin can then use ref_update_sort.

The sort logic may subsume duplicate ref update detection too.
After sorting a simple linear-time scan can detect duplicates.

 +unsigned char new_sha1[20];
 +unsigned char *old_sha1;
 
 This asymmetry is rather curious and will later become problematic
 when we have more users of this API.  Maybe your callers happen have
 static storage to keep old_sha1[] outside this structure but do not
 have it for new_sha1[] for some unknown reason, which may not apply
 to later callers we may want to add.

I wasn't happy with the asymmetry either but forgot to raise it in
the cover letter.  We need a way to represent old value not given
as different from old value is_null_sha1.

One approach is to use a bit in the flags member that already
stores REF_NODEREF.  However, that will be inconsistent with the
other ref update APIs that check whether old_sha1 is NULL.  We could
still do it and document the bit to mean ignore old_sha1 member of
struct ref_update.

Another approach is to add a dedicated member to struct ref_update.

Comments?
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 0/7] Multiple simultaneously locked ref updates

[Brad King]

On 08/29/2013 02:07 PM, Junio C Hamano wrote:
 I didn't mean to force the caller of new update-ref --stdin; the
 new code you wrote for it is what feeds the input to update_refs()
 function, and that is one place you can make sure you do not lock
 yourself out.
 
 Besides, if you get two updates to the same ref from --stdin, you
 would need to verify these are identical (i.e. updating to the same
 new object name from the same old object name; otherwise the requests
 are conflicting and do not make sense), so the code to collect the
 requests from stdin needs to match potential duplicates anyway.
 
 One clean way to do so may be to put an update request (old and new
 sha1) in a structure, and use a string_list to hold list of refs,
 with the util field pointing at the update request data.
 
 - this process already has the lock because it was asked to
   update the same file multiple times simultaneously, or
 
 The second case is like left hand not knowing what right hand is
 doing, no?  It should not happen if we code it right.

Yes.  All of the above is what I originally intended when asking
the question in the cover letter.  Using string_list and its util
field may be useful.  However, see also my response at
$gmane/233260 about how it may fold into sorting.

Thanks for the reviews!
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 6/7] refs: add update_refs for multiple simultaneous updates

[Brad King]

On 08/29/2013 02:32 PM, Junio C Hamano wrote:
 But it may not be a bad idea to keep the callers dumb and have this
 function always sort, dedup, *and* fail inconsistent request.

I agree.  I was just starting to write the comment for update_refs
and it basically would have said always use ref_update_sort and
check for duplicates first.  We might was well build it into the
function.  If some call site in the future wants to optimize a case
known to be sorted it can be refactored later.

Thanks,
-Brad
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html