[PATCHv3 00/13] the refs-transactions-reflog series

[Stefan Beller]

This is the whole refs-transactions-reflog series[1],
which was in discussion for a bit already. It applies to origin/master.

The idea is to have the reflog being part of the transactions, which
the refs are already using, so the we're moving towards a database
like API in the long run. This makes git easier to maintain as well
as opening the possibility to replace the backend with a real database.

If you've followed the topic a bit, start reading at patch
[PATCH 06/13] refs.c: add a transaction function to append a reflog
as the first 5 patches have been discussed a lot separately and can be found in
origin/pu already[2].
The first two patches are deduplicating code.
The third patch is ripping some code out of log_ref_write and introduces
log_ref_write_fd, which does the actual writing.
The patches 4+5 are renaming variables for clarity.

The patch 6 and 7 are the entree in this series. Patch 6 adds two functions to
the refs API: transaction_truncate_reflog and transaction_update_reflog. Both
functions do not affect the files directy, but rather become effective once
transaction_commit function is called. The transaction_truncate_reflog will
wipe the reflog, which can be used for rebuilding the reflog, whereas the
transaction_update_reflog function will just append one entry to the reflog.
The patch 7 will make use of the functions introduced in patch 6. The command
git reflog expire will then use the reflog transactions.

Patch 8 is renaming log_ref_setup to create_reflog and should not

Patches 9-12 are removing functions from the public refs API, which are unused 
then.

Patch 13 is adding new functionality again, you can finally delete broken refs 
without
having to know the details on git.

[1] http://comments.gmane.org/gmane.comp.version-control.git/259712 or
http://www.spinics.net/lists/git/msg242502.html 

[2] patch 1 + 2:
origin/sb/ref-transaction-unify-to-update
patch 3:
origin/sb/log-ref-write-fd
patch 4 + 5:
origin/sb/ref-transaction-reflog
patch 1-5 is unchanged in this series.


Ronnie Sahlberg (9):
  refs.c: make ref_transaction_create a wrapper for
ref_transaction_update
  refs.c: make ref_transaction_delete a wrapper for
ref_transaction_update
  refs.c: add a function to append a reflog entry to a fd
  refs.c: rename the transaction functions
  reflog.c: use a reflog transaction when writing during expire
  refs.c: rename log_ref_setup to create_reflog
  refs.c: remove unlock_ref/close_ref/commit_ref from the refs api
  refs.c: remove lock_any_ref_for_update
  refs.c: allow deleting refs with a broken sha1

Stefan Beller (4):
  refs.c: rename transaction.updates to transaction.ref_updates
  refs.c: add a transaction function to truncate or append a reflog
entry
  refs.c: don't expose the internal struct ref_lock in the header file
  refs.c: use a bit for ref_update have_old

 branch.c   |  13 +-
 builtin/branch.c   |   5 +-
 builtin/checkout.c |   8 +-
 builtin/commit.c   |  10 +-
 builtin/fetch.c|  12 +-
 builtin/receive-pack.c |  13 +-
 builtin/reflog.c   |  84 +--
 builtin/replace.c  |  10 +-
 builtin/tag.c  |  10 +-
 builtin/update-ref.c   |  26 ++--
 cache.h|   7 +
 fast-import.c  |  22 +--
 refs.c | 371 +++--
 refs.h |  95 ++---
 sequencer.c|  12 +-
 t/t3200-branch.sh  |   8 ++
 walker.c   |  10 +-
 17 files changed, 408 insertions(+), 308 deletions(-)

-- 
2.2.0

--
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 01/13] refs.c: make ref_transaction_create a wrapper for ref_transaction_update

[Stefan Beller]

From: Ronnie Sahlberg sahlb...@google.com

The ref_transaction_update function can already be used to create refs by
passing null_sha1 as the old_sha1 parameter. Simplify by replacing
transaction_create with a thin wrapper.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
---

Notes:
origin/sb/ref-transaction-unify-to-update
as well as
origin/sb/ref-transaction-reflog

no changes since sending them last time

 refs.c | 27 ++-
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/refs.c b/refs.c
index 5ff457e..005eb18 100644
--- a/refs.c
+++ b/refs.c
@@ -3623,31 +3623,8 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
   int flags, const char *msg,
   struct strbuf *err)
 {
-   struct ref_update *update;
-
-   assert(err);
-
-   if (transaction-state != REF_TRANSACTION_OPEN)
-   die(BUG: create called for transaction that is not open);
-
-   if (!new_sha1 || is_null_sha1(new_sha1))
-   die(BUG: create ref with null new_sha1);
-
-   if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-   strbuf_addf(err, refusing to create ref with bad name %s,
-   refname);
-   return -1;
-   }
-
-   update = add_update(transaction, refname);
-
-   hashcpy(update-new_sha1, new_sha1);
-   hashclr(update-old_sha1);
-   update-flags = flags;
-   update-have_old = 1;
-   if (msg)
-   update-msg = xstrdup(msg);
-   return 0;
+   return ref_transaction_update(transaction, refname, new_sha1,
+ null_sha1, flags, 1, msg, err);
 }
 
 int ref_transaction_delete(struct ref_transaction *transaction,
-- 
2.2.0

--
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 12/13] refs.c: use a bit for ref_update have_old

[Stefan Beller]

Signed-off-by: Stefan Beller sbel...@google.com
---

Notes:
Also a patch, which hasn't been posted on the mailing list before.

 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index b54b5b3..2942227 100644
--- a/refs.c
+++ b/refs.c
@@ -3532,7 +3532,7 @@ struct ref_update {
unsigned char new_sha1[20];
unsigned char old_sha1[20];
int flags; /* REF_NODEREF? */
-   int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
+   int have_old:1; /* 1 if old_sha1 is valid, 0 otherwise */
struct ref_lock *lock;
int type;
char *msg;
-- 
2.2.0

--
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 10/13] refs.c: remove lock_any_ref_for_update

[Stefan Beller]

From: Ronnie Sahlberg sahlb...@google.com

No one is using this function so we can delete it.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
 refs.c | 7 ---
 refs.h | 9 +
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/refs.c b/refs.c
index 1468c00..796b7cc 100644
--- a/refs.c
+++ b/refs.c
@@ -2346,13 +2346,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
*refname,
return NULL;
 }
 
-struct ref_lock *lock_any_ref_for_update(const char *refname,
-const unsigned char *old_sha1,
-int flags, int *type_p)
-{
-   return lock_ref_sha1_basic(refname, old_sha1, NULL, flags, type_p);
-}
-
 /*
  * Write an entry to the packed-refs file for the specified refname.
  * If peeled is non-NULL, write it as the entry's peeled value.
diff --git a/refs.h b/refs.h
index fdbdea6..166aab8 100644
--- a/refs.h
+++ b/refs.h
@@ -181,8 +181,7 @@ extern int is_branch(const char *refname);
 extern int peel_ref(const char *refname, unsigned char *sha1);
 
 /*
- * Flags controlling lock_any_ref_for_update(), transaction_update_ref(),
- * transaction_create_ref(), etc.
+ * Flags controlling transaction_update_ref(), transaction_create_ref(), etc.
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *  symbolic references.
  * REF_DELETING: tolerate broken refs
@@ -191,12 +190,6 @@ extern int peel_ref(const char *refname, unsigned char 
*sha1);
  */
 #define REF_NODEREF0x01
 #define REF_DELETING   0x02
-/*
- * This function sets errno to something meaningful on failure.
- */
-extern struct ref_lock *lock_any_ref_for_update(const char *refname,
-   const unsigned char *old_sha1,
-   int flags, int *type_p);
 
 /** Reads log for the value of ref during at_time. **/
 extern int read_ref_at(const char *refname, unsigned int flags,
-- 
2.2.0

--
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 13/13] refs.c: allow deleting refs with a broken sha1

[Stefan Beller]

From: Ronnie Sahlberg sahlb...@google.com

Add back support to make it possible to delete refs that have a broken
sha1.

Add new internal flags REF_ALLOW_BROKEN and RESOLVE_REF_ALLOW_BAD_SHA1
to pass intent from branch.c that we are willing to allow
resolve_ref_unsafe and lock_ref_sha1_basic to allow broken refs.
Since these refs can not actually be resolved to a sha1, they instead resolve
to null_sha1 when these flags are used.

For example, the ref:

   echo Broken ref  .git/refs/heads/foo-broken-1

can now be deleted using git branch -d foo-broken-1

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
 builtin/branch.c  | 5 +++--
 cache.h   | 7 +++
 refs.c| 6 ++
 refs.h| 6 --
 t/t3200-branch.sh | 8 
 5 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 3b79c50..04f57d4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -238,7 +238,8 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
target = resolve_ref_unsafe(name,
RESOLVE_REF_READING
| RESOLVE_REF_NO_RECURSE
-   | RESOLVE_REF_ALLOW_BAD_NAME,
+   | RESOLVE_REF_ALLOW_BAD_NAME
+   | RESOLVE_REF_ALLOW_BAD_SHA1,
sha1, flags);
if (!target) {
error(remote_branch
@@ -255,7 +256,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
continue;
}
 
-   if (delete_ref(name, sha1, REF_NODEREF)) {
+   if (delete_ref(name, sha1, REF_NODEREF|REF_ALLOW_BROKEN)) {
error(remote_branch
  ? _(Error deleting remote branch '%s')
  : _(Error deleting branch '%s'),
diff --git a/cache.h b/cache.h
index 99ed096..61e61af 100644
--- a/cache.h
+++ b/cache.h
@@ -1000,10 +1000,17 @@ extern int read_ref(const char *refname, unsigned char 
*sha1);
  * resolved. The function returns NULL for such ref names.
  * Caps and underscores refers to the special refs, such as HEAD,
  * FETCH_HEAD and friends, that all live outside of the refs/ directory.
+ *
+ * RESOLVE_REF_ALLOW_BAD_SHA1 when this flag is set and the ref contains
+ * an invalid sha1, resolve_ref_unsafe will clear the sha1 argument,
+ * set the REF_ISBROKEN flag and return the refname.
+ * This allows using resolve_ref_unsafe to check for existence of such
+ * broken refs.
  */
 #define RESOLVE_REF_READING 0x01
 #define RESOLVE_REF_NO_RECURSE 0x02
 #define RESOLVE_REF_ALLOW_BAD_NAME 0x04
+#define RESOLVE_REF_ALLOW_BAD_SHA1 0x08
 extern const char *resolve_ref_unsafe(const char *ref, int resolve_flags, 
unsigned char *sha1, int *flags);
 extern char *resolve_refdup(const char *ref, int resolve_flags, unsigned char 
*sha1, int *flags);
 
diff --git a/refs.c b/refs.c
index 2942227..759536c 100644
--- a/refs.c
+++ b/refs.c
@@ -1587,6 +1587,10 @@ const char *resolve_ref_unsafe(const char *refname, int 
resolve_flags, unsigned
(buffer[40] != '\0'  !isspace(buffer[40]))) {
if (flags)
*flags |= REF_ISBROKEN;
+   if (resolve_flags  RESOLVE_REF_ALLOW_BAD_SHA1) 
{
+   hashclr(sha1);
+   return refname;
+   }
errno = EINVAL;
return NULL;
}
@@ -2268,6 +2272,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
*refname,
if (flags  REF_NODEREF)
resolve_flags |= RESOLVE_REF_NO_RECURSE;
}
+   if (flags  REF_ALLOW_BROKEN)
+   resolve_flags |= RESOLVE_REF_ALLOW_BAD_SHA1;
 
refname = resolve_ref_unsafe(refname, resolve_flags,
 lock-old_sha1, type);
diff --git a/refs.h b/refs.h
index cfc8d90..735f59f 100644
--- a/refs.h
+++ b/refs.h
@@ -176,11 +176,13 @@ extern int peel_ref(const char *refname, unsigned char 
*sha1);
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *  symbolic references.
  * REF_DELETING: tolerate broken refs
+ * REF_ALLOW_BROKEN: allow locking refs that are broken.
  *
  * Flags = 0x100 are reserved for internal use.
  */
-#define REF_NODEREF0x01
-#define REF_DELETING   0x02
+#define REF_NODEREF0x01
+#define REF_DELETING   0x02
+#define REF_ALLOW_BROKEN   0x04
 
 /** Reads log for the value of ref during at_time. **/
 extern int read_ref_at(const char *refname, unsigned int flags,
diff 

[PATCH 09/13] refs.c: remove unlock_ref/close_ref/commit_ref from the refs api

[Stefan Beller]

From: Ronnie Sahlberg sahlb...@google.com

unlock|close|commit_ref can be made static since there are no more external
callers.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
 refs.c | 24 
 refs.h |  9 -
 2 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/refs.c b/refs.c
index bdd3f75..1468c00 100644
--- a/refs.c
+++ b/refs.c
@@ -2090,6 +2090,16 @@ int refname_match(const char *abbrev_name, const char 
*full_name)
return 0;
 }
 
+static void unlock_ref(struct ref_lock *lock)
+{
+   /* Do not free lock-lk -- atexit() still looks at them */
+   if (lock-lk)
+   rollback_lock_file(lock-lk);
+   free(lock-ref_name);
+   free(lock-orig_ref_name);
+   free(lock);
+}
+
 /* This function should make sure errno is meaningful on error */
 static struct ref_lock *verify_lock(struct ref_lock *lock,
const unsigned char *old_sha1, int mustexist)
@@ -2888,7 +2898,7 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
return 1;
 }
 
-int close_ref(struct ref_lock *lock)
+static int close_ref(struct ref_lock *lock)
 {
if (close_lock_file(lock-lk))
return -1;
@@ -2896,7 +2906,7 @@ int close_ref(struct ref_lock *lock)
return 0;
 }
 
-int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock)
 {
if (commit_lock_file(lock-lk))
return -1;
@@ -2904,16 +2914,6 @@ int commit_ref(struct ref_lock *lock)
return 0;
 }
 
-void unlock_ref(struct ref_lock *lock)
-{
-   /* Do not free lock-lk -- atexit() still looks at them */
-   if (lock-lk)
-   rollback_lock_file(lock-lk);
-   free(lock-ref_name);
-   free(lock-orig_ref_name);
-   free(lock);
-}
-
 /*
  * copy the reflog message msg to buf, which has been allocated sufficiently
  * large, while cleaning up the whitespaces.  Especially, convert LF to space,
diff --git a/refs.h b/refs.h
index 0c75217..fdbdea6 100644
--- a/refs.h
+++ b/refs.h
@@ -198,15 +198,6 @@ extern struct ref_lock *lock_any_ref_for_update(const char 
*refname,
const unsigned char *old_sha1,
int flags, int *type_p);
 
-/** Close the file descriptor owned by a lock and return the status */
-extern int close_ref(struct ref_lock *lock);
-
-/** Close and commit the ref locked by the lock */
-extern int commit_ref(struct ref_lock *lock);
-
-/** Release any lock taken but not written. **/
-extern void unlock_ref(struct ref_lock *lock);
-
 /** Reads log for the value of ref during at_time. **/
 extern int read_ref_at(const char *refname, unsigned int flags,
   unsigned long at_time, int cnt,
-- 
2.2.0

--
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 02/13] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update

[Stefan Beller]

From: Ronnie Sahlberg sahlb...@google.com

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
---

Notes:
origin/sb/ref-transaction-unify-to-update
as well as
origin/sb/ref-transaction-reflog

no changes since sending last time.

 refs.c | 22 ++
 refs.h |  2 +-
 2 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/refs.c b/refs.c
index 005eb18..05cb299 100644
--- a/refs.c
+++ b/refs.c
@@ -3633,26 +3633,8 @@ int ref_transaction_delete(struct ref_transaction 
*transaction,
   int flags, int have_old, const char *msg,
   struct strbuf *err)
 {
-   struct ref_update *update;
-
-   assert(err);
-
-   if (transaction-state != REF_TRANSACTION_OPEN)
-   die(BUG: delete called for transaction that is not open);
-
-   if (have_old  !old_sha1)
-   die(BUG: have_old is true but old_sha1 is NULL);
-
-   update = add_update(transaction, refname);
-   update-flags = flags;
-   update-have_old = have_old;
-   if (have_old) {
-   assert(!is_null_sha1(old_sha1));
-   hashcpy(update-old_sha1, old_sha1);
-   }
-   if (msg)
-   update-msg = xstrdup(msg);
-   return 0;
+   return ref_transaction_update(transaction, refname, null_sha1,
+ old_sha1, flags, have_old, msg, err);
 }
 
 int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index 2bc3556..7d675b7 100644
--- a/refs.h
+++ b/refs.h
@@ -283,7 +283,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf 
*err);
 
 /*
  * Add a reference update to transaction.  new_sha1 is the value that
- * the reference should have after the update, or zeros if it should
+ * the reference should have after the update, or null_sha1 if it should
  * be deleted.  If have_old is true, then old_sha1 holds the value
  * that the reference should have had before the update, or zeros if
  * it must not have existed beforehand.
-- 
2.2.0

--
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 07/13] reflog.c: use a reflog transaction when writing during expire

[Stefan Beller]

From: Ronnie Sahlberg sahlb...@google.com

Use a transaction for all updates during expire_reflog.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---

Notes:
Jonathan writes:
 This doesn't match the signature of each_reflog_ent_fn.  Would putting
 err in the cb_data struct work?

That's what I do in this update.

 builtin/reflog.c | 84 
 1 file changed, 36 insertions(+), 48 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 2d85d26..e13427c 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -33,7 +33,8 @@ struct cmd_reflog_expire_cb {
 };
 
 struct expire_reflog_cb {
-   FILE *newlog;
+   struct transaction *t;
+   const char *refname;
enum {
UE_NORMAL,
UE_ALWAYS,
@@ -43,6 +44,7 @@ struct expire_reflog_cb {
unsigned long mark_limit;
struct cmd_reflog_expire_cb *cmd;
unsigned char last_kept_sha1[20];
+   struct strbuf *err;
 };
 
 struct collected_reflog {
@@ -316,20 +318,18 @@ static int expire_reflog_ent(unsigned char *osha1, 
unsigned char *nsha1,
if (cb-cmd-recno  --(cb-cmd-recno) == 0)
goto prune;
 
-   if (cb-newlog) {
-   char sign = (tz  0) ? '-' : '+';
-   int zone = (tz  0) ? (-tz) : tz;
-   fprintf(cb-newlog, %s %s %s %lu %c%04d\t%s,
-   sha1_to_hex(osha1), sha1_to_hex(nsha1),
-   email, timestamp, sign, zone,
-   message);
+   if (cb-t) {
+   if (transaction_update_reflog(cb-t, cb-refname, nsha1, osha1,
+ email, timestamp, tz, message,
+ cb-err))
+   return -1;
hashcpy(cb-last_kept_sha1, nsha1);
}
if (cb-cmd-verbose)
printf(keep %s, message);
return 0;
  prune:
-   if (!cb-newlog)
+   if (!cb-t)
printf(would prune %s, message);
else if (cb-cmd-verbose)
printf(prune %s, message);
@@ -353,29 +353,26 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, int unused,
 {
struct cmd_reflog_expire_cb *cmd = cb_data;
struct expire_reflog_cb cb;
-   struct ref_lock *lock;
-   char *log_file, *newlog_path = NULL;
struct commit *tip_commit;
struct commit_list *tips;
+   struct strbuf err = STRBUF_INIT;
int status = 0;
 
memset(cb, 0, sizeof(cb));
+   cb.refname = ref;
+   cb.err = err;
 
-   /*
-* we take the lock for the ref itself to prevent it from
-* getting updated.
-*/
-   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);
if (!reflog_exists(ref))
goto finish;
-   if (!cmd-dry_run) {
-   newlog_path = git_pathdup(logs/%s.lock, ref);
-   cb.newlog = fopen(newlog_path, w);
+   cb.t = transaction_begin(err);
+   if (!cb.t) {
+   status |= error(%s, err.buf);
+   goto cleanup;
+   }
+   if (transaction_truncate_reflog(cb.t, cb.refname, err)) {
+   status |= error(%s, err.buf);
+   goto cleanup;
}
-
cb.cmd = cmd;
 
if (!cmd-expire_unreachable || !strcmp(ref, HEAD)) {
@@ -407,7 +404,10 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, int unused,
mark_reachable(cb);
}
 
-   for_each_reflog_ent(ref, expire_reflog_ent, cb);
+   if (for_each_reflog_ent(ref, expire_reflog_ent, cb)) {
+   status |= error(%s, err.buf);
+   goto cleanup;
+   }
 
if (cb.unreachable_expire_kind != UE_ALWAYS) {
if (cb.unreachable_expire_kind == UE_HEAD) {
@@ -420,32 +420,20 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, int unused,
}
}
  finish:
-   if (cb.newlog) {
-   if (fclose(cb.newlog)) {
-   status |= error(%s: %s, strerror(errno),
-   newlog_path);
-   unlink(newlog_path);
-   } else if (cmd-updateref 
-   (write_in_full(lock-lock_fd,
-   sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-write_str_in_full(lock-lock_fd, \n) != 1 ||
-close_ref(lock)  0)) {
-   status |= error(Couldn't write %s,
-   lock-lk-filename.buf);
-   unlink(newlog_path);
-   } else if (rename(newlog_path, log_file)) {
-   status |= error(cannot rename 

[PATCH 08/13] refs.c: rename log_ref_setup to create_reflog

[Stefan Beller]

From: Ronnie Sahlberg sahlb...@google.com

log_ref_setup is used to do several semi-related things:
* Sometimes it will create a new reflog including missing parent
  directories and cleaning up any conflicting stale directories
  in the path.
* Fill in a filename buffer for the full path to the reflog.
* Unconditionally re-adjust the permissions for the file.

This function is only called from two places: In checkout.c, where
it is always used to create a reflog and in refs.c log_ref_write,
where it is used to create a reflog sometimes, and sometimes just
to fill in the filename.

Rename log_ref_setup to create_reflog and change it to only take the
refname as an argument to make its signature similar to delete_reflog
and reflog_exists. Change create_reflog to ignore log_all_ref_updates
and unconditionally create the reflog when called. Since checkout.c
always wants to create a reflog we can call create_reflog directly and
avoid the temp-and-log_all_ref_update dance.

In log_ref_write, only call create_reflog, iff we want to create a reflog
and if the reflog does not yet exist. This means that for the common case
where the log already exists we now only need to perform a single lstat()
instead of a open(O_CREAT)+lstat()+close().

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
 builtin/checkout.c |  8 +---
 refs.c | 22 +-
 refs.h |  8 +++-
 3 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5410dac..8550b6d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -587,19 +587,13 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
if (opts-new_branch) {
if (opts-new_orphan_branch) {
if (opts-new_branch_log  !log_all_ref_updates) {
-   int temp;
-   char log_file[PATH_MAX];
char *ref_name = mkpath(refs/heads/%s, 
opts-new_orphan_branch);
 
-   temp = log_all_ref_updates;
-   log_all_ref_updates = 1;
-   if (log_ref_setup(ref_name, log_file, 
sizeof(log_file))) {
+   if (create_reflog(ref_name)) {
fprintf(stderr, _(Can not do reflog 
for '%s'\n),
opts-new_orphan_branch);
-   log_all_ref_updates = temp;
return;
}
-   log_all_ref_updates = temp;
}
}
else
diff --git a/refs.c b/refs.c
index c411af9..bdd3f75 100644
--- a/refs.c
+++ b/refs.c
@@ -2941,16 +2941,16 @@ static int copy_msg(char *buf, const char *msg)
 }
 
 /* This function must set a meaningful errno on failure */
-int log_ref_setup(const char *refname, char *logfile, int bufsize)
+int create_reflog(const char *refname)
 {
int logfd, oflags = O_APPEND | O_WRONLY;
+   char logfile[PATH_MAX];
 
-   git_snpath(logfile, bufsize, logs/%s, refname);
-   if (log_all_ref_updates 
-   (starts_with(refname, refs/heads/) ||
-starts_with(refname, refs/remotes/) ||
-starts_with(refname, refs/notes/) ||
-!strcmp(refname, HEAD))) {
+   git_snpath(logfile, sizeof(logfile), logs/%s, refname);
+   if (starts_with(refname, refs/heads/) ||
+   starts_with(refname, refs/remotes/) ||
+   starts_with(refname, refs/notes/) ||
+   !strcmp(refname, HEAD)) {
if (safe_create_leading_directories(logfile)  0) {
int save_errno = errno;
error(unable to create directory for %s, logfile);
@@ -3019,16 +3019,20 @@ static int log_ref_write_fd(int fd, const unsigned char 
*old_sha1,
 static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 const unsigned char *new_sha1, const char *msg)
 {
-   int logfd, result, oflags = O_APPEND | O_WRONLY;
+   int logfd, result = 0, oflags = O_APPEND | O_WRONLY;
char log_file[PATH_MAX];
 
if (log_all_ref_updates  0)
log_all_ref_updates = !is_bare_repository();
 
-   result = log_ref_setup(refname, log_file, sizeof(log_file));
+   if (log_all_ref_updates  !reflog_exists(refname))
+   result = create_reflog(refname);
+
if (result)
return result;
 
+   git_snpath(log_file, sizeof(log_file), logs/%s, refname);
+
logfd = open(log_file, oflags);
if (logfd  0)
return 0;
diff --git a/refs.h b/refs.h
index 1afc72c..0c75217 100644
--- a/refs.h
+++ b/refs.h
@@ -207,11 +207,6 @@ extern int commit_ref(struct ref_lock *lock);
 /** Release any lock 

[PATCH 04/13] refs.c: rename the transaction functions

[Stefan Beller]

From: Ronnie Sahlberg sahlb...@google.com

Rename the transaction functions. Remove the leading ref_ from the
names and append _ref to the names for functions that create/delete/
update sha1 refs.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
---

Notes:
remotes/origin/sb/ref-transaction-reflog

no changes since last review

This commit can be reproduced via
find . -name *.[ch] -print | xargs sed -i ' \
s/REF_TRANSACTION_OPEN/TRANSACTION_OPEN/; \
s/REF_TRANSACTION_CLOSED/TRANSACTION_CLOSED/; \
s/ref_transaction_begin/transaction_begin/; \
s/ref_transaction_commit/transaction_commit/; \
s/ref_transaction_create/transaction_create_ref/; \
s/ref_transaction_delete/transaction_delete_ref/; \
s/ref_transaction_free/transaction_free/; \
s/ref_transaction_update/transaction_update_ref/; \
s/ref_transaction/transaction/'
modulo white space changes for alignment.

 branch.c   | 13 +
 builtin/commit.c   | 10 +++
 builtin/fetch.c| 12 
 builtin/receive-pack.c | 13 -
 builtin/replace.c  | 10 +++
 builtin/tag.c  | 10 +++
 builtin/update-ref.c   | 26 -
 fast-import.c  | 22 +++---
 refs.c | 78 +-
 refs.h | 36 +++
 sequencer.c| 12 
 walker.c   | 10 +++
 12 files changed, 126 insertions(+), 126 deletions(-)

diff --git a/branch.c b/branch.c
index 4bab55a..c8462de 100644
--- a/branch.c
+++ b/branch.c
@@ -279,16 +279,17 @@ void create_branch(const char *head,
log_all_ref_updates = 1;
 
if (!dont_change_ref) {
-   struct ref_transaction *transaction;
+   struct transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
-   transaction = ref_transaction_begin(err);
+   transaction = transaction_begin(err);
if (!transaction ||
-   ref_transaction_update(transaction, ref.buf, sha1,
-  null_sha1, 0, !forcing, msg, err) ||
-   ref_transaction_commit(transaction, err))
+   transaction_update_ref(transaction, ref.buf, sha1,
+  null_sha1, 0, !forcing, msg,
+  err) ||
+   transaction_commit(transaction, err))
die(%s, err.buf);
-   ref_transaction_free(transaction);
+   transaction_free(transaction);
strbuf_release(err);
}
 
diff --git a/builtin/commit.c b/builtin/commit.c
index e108c53..f50b7df 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1673,7 +1673,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
-   struct ref_transaction *transaction;
+   struct transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
if (argc == 2  !strcmp(argv[1], -h))
@@ -1804,17 +1804,17 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(sb, strlen(reflog_msg), : , 2);
 
-   transaction = ref_transaction_begin(err);
+   transaction = transaction_begin(err);
if (!transaction ||
-   ref_transaction_update(transaction, HEAD, sha1,
+   transaction_update_ref(transaction, HEAD, sha1,
   current_head
   ? current_head-object.sha1 : NULL,
   0, !!current_head, sb.buf, err) ||
-   ref_transaction_commit(transaction, err)) {
+   transaction_commit(transaction, err)) {
rollback_index_files();
die(%s, err.buf);
}
-   ref_transaction_free(transaction);
+   transaction_free(transaction);
 
unlink(git_path(CHERRY_PICK_HEAD));
unlink(git_path(REVERT_HEAD));
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7b84d35..0be0b09 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -404,7 +404,7 @@ static int s_update_ref(const char *action,
 {
char msg[1024];
char *rla = getenv(GIT_REFLOG_ACTION);
-   struct ref_transaction *transaction;
+   struct transaction *transaction;
struct strbuf err = STRBUF_INIT;
int ret, df_conflict = 0;
 
@@ -414,23 +414,23 @@ static int s_update_ref(const char *action,
rla = default_rla.buf;
snprintf(msg, sizeof(msg), %s: %s, rla, action);
 
-   transaction = 

[PATCH 05/13] refs.c: rename transaction.updates to transaction.ref_updates

[Stefan Beller]

The updates are only holding refs not reflogs, so express it to the reader.

Signed-off-by: Stefan Beller sbel...@google.com
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
---

Notes:
remotes/origin/sb/ref-transaction-reflog

no changes since last review

 refs.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index f0f0d23..58de60c 100644
--- a/refs.c
+++ b/refs.c
@@ -3554,7 +3554,7 @@ enum transaction_state {
  * as atomically as possible.  This structure is opaque to callers.
  */
 struct transaction {
-   struct ref_update **updates;
+   struct ref_update **ref_updates;
size_t alloc;
size_t nr;
enum transaction_state state;
@@ -3575,10 +3575,10 @@ void transaction_free(struct transaction *transaction)
return;
 
for (i = 0; i  transaction-nr; i++) {
-   free(transaction-updates[i]-msg);
-   free(transaction-updates[i]);
+   free(transaction-ref_updates[i]-msg);
+   free(transaction-ref_updates[i]);
}
-   free(transaction-updates);
+   free(transaction-ref_updates);
free(transaction);
 }
 
@@ -3589,8 +3589,8 @@ static struct ref_update *add_update(struct transaction 
*transaction,
struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
 
strcpy((char *)update-refname, refname);
-   ALLOC_GROW(transaction-updates, transaction-nr + 1, 
transaction-alloc);
-   transaction-updates[transaction-nr++] = update;
+   ALLOC_GROW(transaction-ref_updates, transaction-nr + 1, 
transaction-alloc);
+   transaction-ref_updates[transaction-nr++] = update;
return update;
 }
 
@@ -3712,7 +3712,7 @@ int transaction_commit(struct transaction *transaction,
int ret = 0, delnum = 0, i;
const char **delnames;
int n = transaction-nr;
-   struct ref_update **updates = transaction-updates;
+   struct ref_update **updates = transaction-ref_updates;
 
assert(err);
 
-- 
2.2.0

--
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 11/13] refs.c: don't expose the internal struct ref_lock in the header file

[Stefan Beller]

Now the struct ref_lock is used completely internally, so let's
remove it from the header file.

Signed-off-by: Stefan Beller sbel...@google.com
---

Notes:
This patch is brand new in this series,
so digest it while it's hot!

 refs.c | 9 +
 refs.h | 9 -
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index 796b7cc..b54b5b3 100644
--- a/refs.c
+++ b/refs.c
@@ -6,6 +6,15 @@
 #include dir.h
 #include string-list.h
 
+struct ref_lock {
+   char *ref_name;
+   char *orig_ref_name;
+   struct lock_file *lk;
+   unsigned char old_sha1[20];
+   int lock_fd;
+   int force_write;
+};
+
 /*
  * How to handle various characters in refnames:
  * 0: An acceptable character for refs
diff --git a/refs.h b/refs.h
index 166aab8..cfc8d90 100644
--- a/refs.h
+++ b/refs.h
@@ -1,15 +1,6 @@
 #ifndef REFS_H
 #define REFS_H
 
-struct ref_lock {
-   char *ref_name;
-   char *orig_ref_name;
-   struct lock_file *lk;
-   unsigned char old_sha1[20];
-   int lock_fd;
-   int force_write;
-};
-
 /*
  * A transaction represents a collection of ref updates
  * that should succeed or fail together.
-- 
2.2.0

--
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 06/13] refs.c: add a transaction function to truncate or append a reflog entry

[Stefan Beller]

This patch introduces two transaction functions for dealing with reflog
changes. The first function transaction_truncate_reflog can be used,
when a rebuilding of the reflog is desired, e.g. on reflog expire.
The transaction_update_reflog function can be used to amend a line to the
reflog.

We cannot handle reflogs the same way we handle refs because
of naming conflicts in the file system.
If renaming a ref from foo/foo to foo or the other way round,
we need to be careful as we need the basic foo/ from being a
directory to be a file or vice versa. When handling the refs
this can be solved easily by first recording all we want into
the packed refs file and then deleting all the loose refs. By
doing it that way, we always have a valid state on disk.

We don't have an equivalent of a packed refs file when dealing
with reflog updates, but the API for updating the refs turned
out to be conveniant, so let's introduce an intermediate file
outside the $GIT_DIR/logs/refs/heads/ directory helping us
avoiding this naming conflict for the reflogs. The intermediate
lock file, in which we build up the new reflog will live under
$GIT_DIR/logs/lock/refs/heads/ so the files
$GIT_DIR/logs/lock/refs/heads/foo.lock and
$GIT_DIR/logs/lock/refs/heads/foo/foo.lock
do not collide.

When using transaction_update_reflog, only write to the reflog iff
msg is non-NULL.

During one transaction we allow to make multiple reflog updates to the
same ref. This means we only need to lock the reflog once, during the first
update that touches the reflog, and that all further updates can just write the
reflog entry since the reflog is already locked.

This allows us to write code such as:

  t = transaction_begin()
  transaction_truncate_reflog(t, foo);
  loop-over-something...
 if (want_reflog_entry(...))
 transaction_reflog_update(t, foo, 0, message);
  transaction_commit(t)

where we first truncate the reflog and then build the new content one line
at a time.

Signed-off-by: Stefan Beller sbel...@google.com
---

Notes:
This is a complete rewrite of this patch, lots of design ideas
have been born in fruitful discussions with Jonathan.

Personally I am not yet happy about the file copying in
transaction_update_reflog.

 refs.c | 128 +++--
 refs.h |  20 +++
 2 files changed, 146 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 58de60c..c411af9 100644
--- a/refs.c
+++ b/refs.c
@@ -3557,6 +3557,12 @@ struct transaction {
struct ref_update **ref_updates;
size_t alloc;
size_t nr;
+
+   /*
+* Sorted list of reflogs to be committed,
+* the util points to the lock_file.
+*/
+   struct string_list reflog_updates;
enum transaction_state state;
 };
 
@@ -3564,7 +3570,10 @@ struct transaction *transaction_begin(struct strbuf *err)
 {
assert(err);
 
-   return xcalloc(1, sizeof(struct transaction));
+   struct transaction *ret = xcalloc(1, sizeof(struct transaction));
+   ret-reflog_updates.strdup_strings = 1;
+
+   return ret;
 }
 
 void transaction_free(struct transaction *transaction)
@@ -3629,6 +3638,113 @@ int transaction_update_ref(struct transaction 
*transaction,
return 0;
 }
 
+int transaction_truncate_reflog(struct transaction *transaction,
+   const char *refname,
+   struct strbuf *err)
+{
+   struct lock_file *lock;
+   struct string_list_item *item;
+
+   if (transaction-state != TRANSACTION_OPEN)
+   die(BUG: update_reflog called for transaction that is not 
open);
+
+   item = string_list_insert(transaction-reflog_updates, refname);
+   if (item-util) {
+   lock = item-util;
+   if (lseek(lock-fd, 0, SEEK_SET)  0 ||
+   ftruncate(lock-fd, 0)) {
+   strbuf_addf(err, cannot truncate reflog '%s': %s,
+   refname, strerror(errno));
+   goto failure;
+   }
+
+   } else {
+   char *path = git_path(logs/locks/%s, refname);
+   item-util = xcalloc(1, sizeof(struct lock_file));
+   lock = item-util;
+   if (safe_create_leading_directories(path)) {
+   strbuf_addf(err, could not create leading directories 
of '%s': %s,
+   path, strerror(errno));
+   goto failure;
+   }
+
+   if (hold_lock_file_for_update(lock, path, 0)  0) {
+   unable_to_lock_message(path, errno, err);
+   goto failure;
+   }
+   /* The file is empty, no need to truncate. */
+   }
+   return 0;
+failure:
+   transaction-state = TRANSACTION_CLOSED;
+   return -1;
+}
+
+
+int transaction_update_reflog(struct transaction 

[PATCH 03/13] refs.c: add a function to append a reflog entry to a fd

[Stefan Beller]

From: Ronnie Sahlberg sahlb...@google.com

Break out the code to create the string and writing it to the file
descriptor from log_ref_write and add it into a dedicated function
log_ref_write_fd. For now this is only used from log_ref_write,
but later on we will call this function from reflog transactions too,
which means that we will end up with only a single place,
where we write a reflog entry to a file instead of the current two
places (log_ref_write and builtin/reflog.c).

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
---

Notes:
being part of origin/sb/log-ref-write-fd

no changes since sending last time.

 refs.c | 48 ++--
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/refs.c b/refs.c
index 05cb299..150c980 100644
--- a/refs.c
+++ b/refs.c
@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, 
int bufsize)
return 0;
 }
 
+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
+   const unsigned char *new_sha1,
+   const char *committer, const char *msg)
+{
+   int msglen, written;
+   unsigned maxlen, len;
+   char *logrec;
+
+   msglen = msg ? strlen(msg) : 0;
+   maxlen = strlen(committer) + msglen + 100;
+   logrec = xmalloc(maxlen);
+   len = sprintf(logrec, %s %s %s\n,
+ sha1_to_hex(old_sha1),
+ sha1_to_hex(new_sha1),
+ committer);
+   if (msglen)
+   len += copy_msg(logrec + len - 1, msg) - 1;
+
+   written = len = maxlen ? write_in_full(fd, logrec, len) : -1;
+   free(logrec);
+   if (written != len)
+   return -1;
+
+   return 0;
+}
+
 static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 const unsigned char *new_sha1, const char *msg)
 {
-   int logfd, result, written, oflags = O_APPEND | O_WRONLY;
-   unsigned maxlen, len;
-   int msglen;
+   int logfd, result, oflags = O_APPEND | O_WRONLY;
char log_file[PATH_MAX];
-   char *logrec;
-   const char *committer;
 
if (log_all_ref_updates  0)
log_all_ref_updates = !is_bare_repository();
@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const 
unsigned char *old_sha1,
logfd = open(log_file, oflags);
if (logfd  0)
return 0;
-   msglen = msg ? strlen(msg) : 0;
-   committer = git_committer_info(0);
-   maxlen = strlen(committer) + msglen + 100;
-   logrec = xmalloc(maxlen);
-   len = sprintf(logrec, %s %s %s\n,
- sha1_to_hex(old_sha1),
- sha1_to_hex(new_sha1),
- committer);
-   if (msglen)
-   len += copy_msg(logrec + len - 1, msg) - 1;
-   written = len = maxlen ? write_in_full(logfd, logrec, len) : -1;
-   free(logrec);
-   if (written != len) {
+   result = log_ref_write_fd(logfd, old_sha1, new_sha1,
+ git_committer_info(0), msg);
+   if (result) {
int save_errno = errno;
close(logfd);
error(Unable to append to %s, log_file);
-- 
2.2.0

--
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: Enhancement Request: locale git option

[Torsten Bögershausen]

On 12/04/2014 08:32 AM, Ulrich Windl wrote:

Hi!

I'm native German, but German git messages confuse me (yopu'll have to correlate them 
with the man pages). At the moment git uses the locale settings from the environment, so 
you can only change git's locale settings by changing the environment (like LANG= 
git ...).
OTOH Git has a flexible hierachical option setting mechanism. Why not allow a 
Git language (locale) setting system-wde, user-wide, or repository-specific.

Regards,
Ulrich

How about
alias git='LANGUAGE=de_DE.UTF-8 git'
in your ~/.profile ?
(Of course you need to change de to the language you want )

--
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/14] copy_fd: pass error message back through a strbuf

[Stefan Beller]

 Your solution adds a strbuf. That helps with context and stomping, but
 loses readability and adds allocation.

 If we changed the strbuf to a fixed-size buffer, that would help the
 allocation issue. Some messages might be truncated, but it seems
 unlikely in practice. It still loses readability, though.

 What about a struct that has an errno-like value _and_ a fixed-size
 buffer? I'm thinking something like:

What do you mean by the allocation being an issue?
We're only populating the error buffer in the error case, so you're
not talking about performance/speed I'd assume?
As error handling breaks in the least expected ways, I'd rather go
with well tested string buffer codes there?
--
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/14] copy_fd: pass error message back through a strbuf

[Jeff King]

On Thu, Dec 04, 2014 at 12:36:46AM -0800, Stefan Beller wrote:

  Your solution adds a strbuf. That helps with context and stomping, but
  loses readability and adds allocation.
 
  If we changed the strbuf to a fixed-size buffer, that would help the
  allocation issue. Some messages might be truncated, but it seems
  unlikely in practice. It still loses readability, though.
 
  What about a struct that has an errno-like value _and_ a fixed-size
  buffer? I'm thinking something like:
 
 What do you mean by the allocation being an issue?

I mean that the caller has to take care of releasing the memory. This
adds boilerplate to the caller, and is a potential source of leaks.

 We're only populating the error buffer in the error case, so you're
 not talking about performance/speed I'd assume?

No, I don't care about performance here. Only code maintainability.

 As error handling breaks in the least expected ways, I'd rather go
 with well tested string buffer codes there?

We'd still be building on snprintf. And with a known-size buffer, we can
wrap the formatting so that the callers don't even have to care (see the
mkerror example I posted).

-Peff
--
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

git add single file and git add list of files behave differentely for ignored files

[Guilherme]

Hello,

I reported this issue on the git-user mailing list and they redirected me here.

The problem I have observed is that with a ignored path `git add
single file` behaves differently then `git add list of files`.

I my git/info/excludes file i have

/COM/config
!COM/config/Project.gny

The file COM/config/Project.gny has already been added to the
repository via `git add -f`.

When doing

git add -- COM/config/Projec.gny

git will not complain but when doing

git add -- COM/config/Project.gny otherfiles.c

it will report:

The following paths are ignored by one of your .gitignore files:
COM/config
Use -f if you really want to add them.
fatal: no files added

This odd behaviour is also present in `git check-ignore`.

Before adding the file `git check-ignore` correctly reports the file
as ignored. After having added it via `git add -f` it won't report it
as ignored anymore.

Even if not a bug this behaviour is inconsistent and might want to be
addressed as it makes scripting a little bit harder.

Thank you.
--
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] gc: support temporarily preserving garbage

[Jeff King]

On Wed, Dec 03, 2014 at 01:21:03PM -0800, Brodie Rao wrote:

  I think it is also not sufficient. This patch seems to cover only
  objects. But we assume that we can atomically rename() new versions of
  files into place whenever we like without disrupting existing readers.
  This is the case for ref updates (and packed-refs), as well as the index
  file.  The destination end of the rename is an unlink() in disguise, and
  would be susceptible to the same problems.
 
 I'm not aware of renaming over files happening anywhere in gc-related
 code. Do you think that's something that would need to be addressed in
 the rest of the code base before going forward with this garbage
 directory approach? If so, do you have any suggestions on how to
 tackle that problem?

As an example, if you run git pack-refs --all --prune (which is run by
git gc), it will create a new pack-refs file and rename it into place.
Another git program (say, git for-each-ref) might be reading the file
at the same time. If you run pack-refs and for-each-ref simultaneously
in tight loops on your problematic NFS setup, what happens?

I have no idea if it breaks or not. I don't have such a misbehaving
system, and I don't know how rename() is implemented on it. But if it
_is_ a problem of the same variety, then I don't see much point in
making an invasive fix to address half of the problem areas, but not the
other half (i.e., if we are still left with a broken git in this setup,
was the invasive fix worth the cost?).

If it is a problem (and again, I am just guessing), I'd imagine you
would need a similar setup to what you proposed for unlink(): before
renaming packed-refs.lock into packed-refs, hard-link it into your
trash area. And you'd probably want to intercept rename() there, to
catch all places where we use this technique.

-Peff
--
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] introduce git root

[Jeff King]

On Tue, Dec 02, 2014 at 09:26:00AM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  There is also git var, which is a catch-all for printing some deduced
  environmental defaults. I'd be just as happy to see it go away, though.
  Having:
 
git --exec-path
git --toplevel
git --author-ident
 
  all work would make sense to me (I often get confused between git
  --foo and git rev-parse --foo when trying to get the exec-path and
  git-dir). And I don't think it's too late to move in this direction.
  We'd have to keep the old interfaces around, of course, but it would
  immediately improve discoverability and consistency.
 
 Yeah, I too think the above makes sense.  I forgot about var, but
 it should go at the same time we move kitchen-sink options out of
 rev-parse.  One less command to worry about at the UI level means
 you do not have to say if you want to learn about X, ask 'var', if
 you want to learn about Y, ask 'rev-parse', use 'git' itself for Z.

Christian raised the issue of cluttering the git --option namespace,
and I do agree that's a potential issue. My proposal was to drop git
var, but I'd also be OK with making git var the new kitchen sink.  It
doesn't accept many options now, so it's fairly wide open for changing
without losing backwards compatibility. AFAICT, the -l option is
utterly useless, but other than that, it just takes a variable name. We
could introduce dashed options, or just define a sane variable namespace.

Some of the discussion has involved mixing config options into this
kitchen sink, but that does not make any sense to me (and is why I find
git var -l so odd). Config options are fundamentally different, in
that they are set and retrieved, not computed (from other config
variables, or from hard-coded values). And we already have a nice tool
for working with them (well...nice-ish, let's say).

-Peff
--
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] remote: add --fetch and --both options to set-url

[Jeff King]

On Tue, Nov 25, 2014 at 12:48:26PM +0100, Peter Wu wrote:

 git remote set-url knew about the '--push' option to update just the
 pushurl, but it does not have a similar option for update fetch URL and
 leave whatever was in place for the push URL.
 
 This patch adds support for a '--fetch' option which implements that use
 case in a backwards compatible way: if no --both, --push or --fetch
 options are given, then the push URL is modified too if it was not set
 before. This is the case since the push URL is implicitly based on the
 fetch URL.
 
 A '--both' option is added to make the command independent of previous
 pushurl settings. For the --add and --delete set operations, it will
 always set the push and/ or the fetch URLs. For the primary mode of
 operation (without --add or --delete), it will drop pushurl as the
 implicit push URL is the (fetch) URL.
 
 The documentation has also been updated and a missing '--push' option
 is added to the 'git remote -h' command.
 
 Tests are also added to verify the documented behavior.
 
 Signed-off-by: Peter Wu pe...@lekensteyn.nl
 ---

Sorry for the slowness in reviewing. The design of this version makes
sense to me (not surprising, I guess, since it was in direct response to
my comments).

I didn't see anything glaringly wrong in the implementation, though I
did find it a little hard to follow, because of this:

 +#define MODIFY_TYPE_FETCH   (1  0)
 +#define MODIFY_TYPE_PUSH(1  1)
 +#define MODIFY_TYPE_BOTH(MODIFY_TYPE_FETCH | MODIFY_TYPE_PUSH)
 +#define MODIFY_TYPE_HISTORIC(MODIFY_TYPE_FETCH | (1  2))

When reading through the code, the distinction between

  modify_type  MODIFY_TYPE_FETCH

and

  modify_type == MODIFY_TYPE_FETCH

is significant, because the former matches HISTORIC, while the latter
does not. I imagine that a distinct bit value for HISTORIC would make
things a bit more verbose (you would have to add an extra OR in many
places), but I wonder if it would make the code easier to follow (one of
the things I wanted to check was that HISTORIC does the same thing that
it always did, and it is very hard to follow the HISTORIC behavior
reading the code linearly).

I dunno. I don't insist; just noting a difficulty I had while reading
it.  Maybe you went down that route already during development and found
it more painful.

-Peff
--
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: Enhancement Request: locale git option

[Jeff King]

On Thu, Dec 04, 2014 at 09:29:04AM +0100, Torsten Bögershausen wrote:

 How about
 alias git='LANGUAGE=de_DE.UTF-8 git'
 in your ~/.profile ?
 (Of course you need to change de to the language you want )

Besides being awkward in scripts (which will not respect the alias and
use a different language!), that variable will also be inherited by
programs git spawns. So the editor, for example, may end up in the wrong
language.

I think respecting core.locale would make sense (probably the change
would go into git_setup_gettext(), but you may have to fight with the
setup code over looking at config so early in the process).

However, I think the original question is not one of localizing git, but
rather of having it _not_ localized (avoiding the German translations).
There is a hack you can do that for that, which is to set
GIT_TEXTDOMAINDIR to something nonsensical (like /), which will mean
git cannot find the .po files, and just uses the builtin messages.

-Peff
--
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] notes: accept any ref for merge

[Jeff King]

On Sat, Nov 22, 2014 at 10:04:57AM -0800, Kyle J. McKay wrote:

  By stealth enabler I mean the removal of refs/notes/ restriction
  that was originally done as a safety measure to avoid mistakes of
  storing notes outside.  The refs/remote-notes/ future direction
  declares that it is no longer a mistake to store notes outside
  refs/notes/, but that does not necessarily have to mean that
  anywhere under refs/ is fine.  It may make more sense to be explicit
  with the code touched here to allow traditional refs/notes/ and the
  new hierarchy only.  That way, we will still keep the avoid
  mistakes safety and enable the new layout at the same time.
 
 This is the part where I want to lobby for inclusion of this change.   
 I do not believe it is consistent with existing Git practice to  
 enforce restrictions like this (you can only display/edit/etc. notes  
 under refs/notes/*).

Yeah, this is the compelling part to me. There is literally no way to
utilize the notes codes for any ref outside of refs/notes currently. We
don't know if refs/remote-notes is the future, or refs/remotes/origin/notes,
or something else. But we can't even experiment with it in a meaningful way
because the plumbing layer is so restrictive.

The notes feature has stagnated. Not many people use it because it requires
so much magic to set up and share notes. I think it makes sense to remove a
safety feature that is making it harder to experiment with. If the worst
case is that people start actually _using_ notes and we get proliferation of
places that people are sticking them in the refs hierarchy, that is vastly
preferable IMHO to the current state, in which few people use them and there
is little support for sharing them at all.

The original patch discussion sort of fizzled, and your response here
largely slipped through the cracks. I am not sure everyone even
remembers the exact patch under discussion. Maybe a better way to
re-kickstart the discussion is to repost the patch along with a synopsis
of the previous discussion and your arguments about moving it forward.

-Peff
--
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: git add single file and git add list of files behave differentely for ignored files

[Konstantin Khomoutov]

On Thu, 4 Dec 2014 10:06:23 +0100
Guilherme guibuf...@gmail.com wrote:

 I reported this issue on the git-user mailing list and they
 redirected me here.
 
 The problem I have observed is that with a ignored path `git add
 single file` behaves differently then `git add list of files`.
[...]

To those who's interested the original thread on git-users is
https://groups.google.com/d/topic/git-users/322tole9am8/discussion
--
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: git add single file and git add list of files behave differentely for ignored files

[Guilherme]

I forgot to mention:

Environment: Cygwin

Git version 2.1.1

On Thu, Dec 4, 2014 at 12:11 PM, Konstantin Khomoutov
flatw...@users.sourceforge.net wrote:
 On Thu, 4 Dec 2014 10:06:23 +0100
 Guilherme guibuf...@gmail.com wrote:

 I reported this issue on the git-user mailing list and they
 redirected me here.

 The problem I have observed is that with a ignored path `git add
 single file` behaves differently then `git add list of files`.
 [...]

 To those who's interested the original thread on git-users is
 https://groups.google.com/d/topic/git-users/322tole9am8/discussion
--
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

[RFC/PATCH 0/2] Make git branch -f forceful

[Michael J Gruber]

For many git commands, '-f/--force' is a way to force actions which
would otherwise error out. Way more than once, I've been trying this
with 'git branch -d' and 'git branch -m'...

I've had these two patches sitting in my tree for 3 years now it seems.
Here's a rebase.

Before applying these for food I should probably rename force_create to force.

Michael J Gruber (2):
  t3200-branch: test -M
  branch: allow -f with -m and -d

 builtin/branch.c  |  9 +++--
 t/t3200-branch.sh | 14 ++
 2 files changed, 21 insertions(+), 2 deletions(-)

-- 
2.2.0.rc3.286.g888a711

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

[RFC/PATCH 2/2] branch: allow -f with -m and -d

[Michael J Gruber]

-f/--force is the standard way to force an action, and is used by branch
for the recreation of existing branches, but not for deleting unmerged
branches nor for renaming to an existing branch.

Make -m -f equivalent to -M and -d -f equivalent to -D, i.e.
allow -f/--force to be used with -m/-d also.

Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net
---
 builtin/branch.c  | 9 +++--
 t/t3200-branch.sh | 5 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 3b79c50..8ea04d7 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -848,7 +848,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_BOOL('l', create-reflog, reflog, N_(create the branch's 
reflog)),
OPT_BOOL(0, edit-description, edit_description,
 N_(edit the description for the branch)),
-   OPT__FORCE(force_create, N_(force creation (when already 
exists))),
+   OPT__FORCE(force_create, N_(force creation, move/rename, 
deletion)),
{
OPTION_CALLBACK, 0, no-merged, merge_filter_ref,
N_(commit), N_(print only not merged branches),
@@ -891,7 +891,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
if (with_commit || merge_filter != NO_FILTER)
list = 1;
 
-   if (!!delete + !!rename + !!force_create + !!new_upstream +
+   if (!!delete + !!rename + !!new_upstream +
list + unset_upstream  1)
usage_with_options(builtin_branch_usage, options);
 
@@ -904,6 +904,11 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
colopts = 0;
}
 
+   if (force_create) {
+   delete *= 2;
+   rename *= 2;
+   }
+
if (delete) {
if (!argc)
die(_(branch name required));
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 0b3b8f5..ddea498 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -106,6 +106,11 @@ test_expect_success 'git branch -M o/q o/p should work 
when o/p exists' '
git branch -M o/q o/p
 '
 
+test_expect_success 'git branch -m -f o/q o/p should work when o/p exists' '
+   git branch o/q 
+   git branch -m -f o/q o/p
+'
+
 test_expect_success 'git branch -m q r/q should fail when r exists' '
git branch q 
git branch r 
-- 
2.2.0.rc3.286.g888a711

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

[RFC/PATCH 1/2] t3200-branch: test -M

[Michael J Gruber]

Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net
---
 t/t3200-branch.sh | 9 +
 1 file changed, 9 insertions(+)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 432921b..0b3b8f5 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -97,6 +97,15 @@ test_expect_success 'git branch -m o/o o should fail when 
o/p exists' '
test_must_fail git branch -m o/o o
 '
 
+test_expect_success 'git branch -m o/q o/p should fail when o/p exists' '
+   git branch o/q 
+   test_must_fail git branch -m o/q o/p
+'
+
+test_expect_success 'git branch -M o/q o/p should work when o/p exists' '
+   git branch -M o/q o/p
+'
+
 test_expect_success 'git branch -m q r/q should fail when r exists' '
git branch q 
git branch r 
-- 
2.2.0.rc3.286.g888a711

--
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: git add single file and git add list of files behave differentely for ignored files

[Michael J Gruber]

Guilherme schrieb am 04.12.2014 um 10:06:
 Hello,
 
 I reported this issue on the git-user mailing list and they redirected me 
 here.
 
 The problem I have observed is that with a ignored path `git add
 single file` behaves differently then `git add list of files`.
 
 I my git/info/excludes file i have
 
 /COM/config
 !COM/config/Project.gny
 
 The file COM/config/Project.gny has already been added to the
 repository via `git add -f`.
 
 When doing
 
 git add -- COM/config/Projec.gny
 
 git will not complain but when doing
 
 git add -- COM/config/Project.gny otherfiles.c
 
 it will report:
 
 The following paths are ignored by one of your .gitignore files:
 COM/config
 Use -f if you really want to add them.
 fatal: no files added

This is because git add assumes you specified on of the files in error,
and thus refuses to add the other one, too.

I found that behaviour surprising. There's already a patch which, in the
2nd case, would make git add only warn you about the ignored file but
add the other one anyways. It will probably make its way into the next
release.

For the case of a single file (or rather: ignored files only) I'm
wondering whether we should issue a warning, too.

 This odd behaviour is also present in `git check-ignore`.
 
 Before adding the file `git check-ignore` correctly reports the file
 as ignored. After having added it via `git add -f` it won't report it
 as ignored anymore.

This is different: Once a file is added, it is not ignored any more -
you explicitely told git to track that file (rather than ignoring it).
So, the output of git check-ignore is correct.

 Even if not a bug this behaviour is inconsistent and might want to be
 addressed as it makes scripting a little bit harder.
 
 Thank you.
 

I guess you want git check-ignore --no-index. That man page may be a
bit misleading - the description sounds as if only the patterns would
matter.

Michael

--
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] check-ignore: clarify treatment of tracked files

[Michael J Gruber]

By default, check-ignore does not list tracked files at all since
they are not subject to ignore patterns.

Make this clearer in the man page.

Reported-by: Guilherme guibuf...@gmail.com
Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net
---
That really is a bit confusing. Does this help?

 Documentation/git-check-ignore.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/git-check-ignore.txt 
b/Documentation/git-check-ignore.txt
index ee2e091..788a011 100644
--- a/Documentation/git-check-ignore.txt
+++ b/Documentation/git-check-ignore.txt
@@ -21,6 +21,9 @@ the exclude mechanism) that decides if the pathname is 
excluded or
 included.  Later patterns within a file take precedence over earlier
 ones.
 
+By default, tracked files are not shown at all since they are not
+subject to exclude rules; but see `--no-index'.
+
 OPTIONS
 ---
 -q, --quiet::
-- 
2.2.0.rc3.286.g888a711

--
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: Enhancement Request: locale git option

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 4, 2014 at 10:55 AM, Jeff King p...@peff.net wrote:
 On Thu, Dec 04, 2014 at 09:29:04AM +0100, Torsten Bögershausen wrote:

 How about
 alias git='LANGUAGE=de_DE.UTF-8 git'
 in your ~/.profile ?
 (Of course you need to change de to the language you want )

 Besides being awkward in scripts (which will not respect the alias and
 use a different language!), that variable will also be inherited by
 programs git spawns. So the editor, for example, may end up in the wrong
 language.

 I think respecting core.locale would make sense (probably the change
 would go into git_setup_gettext(), but you may have to fight with the
 setup code over looking at config so early in the process).

I think we should just stick to the standard *nix way of doing this:
Tell people to set their locale in their environment.

If someone's having this issue it's also happening for all the
binutils, and any other command-line and GUI program they use, unless
they override using the standard way of doing so, by setting the
relevant LC_* environment variables.

If you want Git in English then create an alias to override its locale
to be C, if you want the editor it spawns to be in some other language
alias that to something that explicitly sets LC_* for that editor.

Maybe I'm being overzealous about this (especially with the I
implemented this blinders on), but let's not have Git set the
precedent for other *nix programs that they all should come up with
some custom way to override locales, that's something to be done at
the OS locale library level, which we use.

 However, I think the original question is not one of localizing git, but
 rather of having it _not_ localized (avoiding the German translations).
 There is a hack you can do that for that, which is to set
 GIT_TEXTDOMAINDIR to something nonsensical (like /), which will mean
 git cannot find the .po files, and just uses the builtin messages.

You can, but the fact that that works is an internal implementation
detail we shouldn't document or support.
--
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

Antw: Re: Enhancement Request: locale git option

[Ulrich Windl]

 Torsten Bögershausen tbo...@web.de schrieb am 04.12.2014 um 09:29 in
Nachricht 54801b50.4080...@web.de:
 On 12/04/2014 08:32 AM, Ulrich Windl wrote:
 Hi!

 I'm native German, but German git messages confuse me (yopu'll have to 
 correlate them with the man pages). At the moment git uses the locale 
 settings from the environment, so you can only change git's locale settings

 by changing the environment (like LANG= git ...).
 OTOH Git has a flexible hierachical option setting mechanism. Why not allow

 a Git language (locale) setting system-wde, user-wide, or
repository-specific.

 Regards,
 Ulrich
 How about
 alias git='LANGUAGE=de_DE.UTF-8 git'
 in your ~/.profile ?
 (Of course you need to change de to the language you want )

That way no program ever needs configuration files ;-)





--
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 12/13] refs.c: use a bit for ref_update have_old

[Torsten Bögershausen]

On 12/04/2014 09:29 AM, Stefan Beller wrote:
Somewhat short commit message, especially the motivation is missing.

What do we gain with this patch ?
In the struct the compiler needs to layout 2*20 bytes for the sha's.
It follows an int, typically 4 bytes.
It follows another int, now with one bit.
Then we have the pointer struct ref_lock *lock,
which needs to be aligned on 4 byte boundary for a 32 bit processor,
or an 8 byte boundary for a 64 bit machine.

Our 1 bit int is padded with 31 bits.
We do not gain anything in memory consumption. (unless we declare int flags
to be 31 bits, and the compiler may join have_old and flags together
into one int in memory.

But there is a price to pay:
The generated code to fiddle out the bits from an int becomes more complicated.
You need to fetch the memory from one int, mask and shift.
Some processors can do this, out of my mind some ARM can, some can not.

We need to run the compiler to look at the generated code of course.
 


 Signed-off-by: Stefan Beller sbel...@google.com
 ---
 
 Notes:
 Also a patch, which hasn't been posted on the mailing list before.
 
  refs.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/refs.c b/refs.c
 index b54b5b3..2942227 100644
 --- a/refs.c
 +++ b/refs.c
 @@ -3532,7 +3532,7 @@ struct ref_update {
   unsigned char new_sha1[20];
   unsigned char old_sha1[20];
   int flags; /* REF_NODEREF? */
 - int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
 + int have_old:1; /* 1 if old_sha1 is valid, 0 otherwise */
   struct ref_lock *lock;
   int type;
   char *msg;
 

--
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: Enhancement Request: locale git option

[Michael J Gruber]

Ævar Arnfjörð Bjarmason schrieb am 04.12.2014 um 16:49:
 On Thu, Dec 4, 2014 at 10:55 AM, Jeff King p...@peff.net wrote:
 On Thu, Dec 04, 2014 at 09:29:04AM +0100, Torsten Bögershausen wrote:

 How about
 alias git='LANGUAGE=de_DE.UTF-8 git'
 in your ~/.profile ?
 (Of course you need to change de to the language you want )

 Besides being awkward in scripts (which will not respect the alias and
 use a different language!), that variable will also be inherited by
 programs git spawns. So the editor, for example, may end up in the wrong
 language.

 I think respecting core.locale would make sense (probably the change
 would go into git_setup_gettext(), but you may have to fight with the
 setup code over looking at config so early in the process).
 
 I think we should just stick to the standard *nix way of doing this:
 Tell people to set their locale in their environment.
 
 If someone's having this issue it's also happening for all the
 binutils, and any other command-line and GUI program they use, unless
 they override using the standard way of doing so, by setting the
 relevant LC_* environment variables.
 
 If you want Git in English then create an alias to override its locale
 to be C, if you want the editor it spawns to be in some other language
 alias that to something that explicitly sets LC_* for that editor.
 
 Maybe I'm being overzealous about this (especially with the I
 implemented this blinders on), but let's not have Git set the
 precedent for other *nix programs that they all should come up with
 some custom way to override locales, that's something to be done at
 the OS locale library level, which we use.
 
 However, I think the original question is not one of localizing git, but
 rather of having it _not_ localized (avoiding the German translations).
 There is a hack you can do that for that, which is to set
 GIT_TEXTDOMAINDIR to something nonsensical (like /), which will mean
 git cannot find the .po files, and just uses the builtin messages.
 
 You can, but the fact that that works is an internal implementation
 detail we shouldn't document or support.
 

The main issue at hand is really that we have localised git but not its
man pages. Even if you understand English, the man pages don't help you
at all if you can't connect the technical terms used there to their
localised counterparts in git's messages. (NO_GETTEXT=y is my solution.)

That is one of the many reasons why I proposed to have a dictionary of
the main technical terms for each language before we even localise git
in that language. In an ideal word, we would provide a simple solution
for looking these terms up both ways. I don't think we're going to have
localised man pages any time soon, are we?

Michael
--
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: Enhancement Request: locale git option

[Andreas Schwab]

Michael J Gruber g...@drmicha.warpmail.net writes:

 The main issue at hand is really that we have localised git but not its
 man pages. Even if you understand English, the man pages don't help you
 at all if you can't connect the technical terms used there to their
 localised counterparts in git's messages. (NO_GETTEXT=y is my solution.)

So the problem is just that the localisation is incomplete.  This is
unfortunate, but happens with a lot of software out there, and providing
a git-only solution doesn't help the case.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
And now for something completely different.
--
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 12/13] refs.c: use a bit for ref_update have_old

[Andreas Schwab]

Stefan Beller sbel...@google.com writes:

 diff --git a/refs.c b/refs.c
 index b54b5b3..2942227 100644
 --- a/refs.c
 +++ b/refs.c
 @@ -3532,7 +3532,7 @@ struct ref_update {
   unsigned char new_sha1[20];
   unsigned char old_sha1[20];
   int flags; /* REF_NODEREF? */
 - int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
 + int have_old:1; /* 1 if old_sha1 is valid, 0 otherwise */

A signed one-bit field cannot store a value of 1.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
And now for something completely different.
--
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: [PATCHv3 00/13] the refs-transactions-reflog series

[Michael Haggerty]

On 12/04/2014 09:29 AM, Stefan Beller wrote:
 This is the whole refs-transactions-reflog series[1],
 which was in discussion for a bit already. It applies to origin/master.

I am still unhappy with the approach of this series, for the reasons
that I explained earlier [1]. In short, I think that the abstraction
level is wrong. In my opinion, consumers of the refs API should barely
even have to *know* about reflogs, let alone implement reflog expiration
themselves.

Of course, reflog expiration *should* be done atomically. But that is
the business of the refs module; callers shouldn't have to do all the
complicated work of building the transaction themselves.

I spent the day working on an alternate proposal, to convert
expire_reflogs() to take callback functions that decide *what* to
expire, but which itself does the work of acquiring locks, iterating
through the reflog entries, rewriting the file, and overwriting the old
file with the new one. The goal is to move this function into refs.c and
make builtin/reflog.c much simpler--it will only have to implement the
callbacks that determine the expiration policy.

I'm almost done with the series but won't be able to finish it until
tomorrow. For those who are impatient, here is my work in progress [2].

Michael

[1]
http://thread.gmane.org/gmane.comp.version-control.git/259712/focus=259770
[2] https://github.com/mhagger/git, branch reflog-expire-api.

-- 
Michael Haggerty
mhag...@alum.mit.edu

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

git log --pretty=format:%H$t%aN$t%s$t%G? --show-signature

[Patrick Schleizer]

Dear git mailing list,
Dear Mike Gerwitz,

according to http://mikegerwitz.com/papers/git-horror-story#script-trust
the output of:

git log --pretty=format:%H$t%aN$t%s$t%G? --show-signature

should look like this:

-

f72924356896ab95a542c495b796555d016cbddd   Mike GerwitzYet
another foo
gpg: Signature made Sun 22 Apr 2012 01:37:26 PM EDT using RSA key ID
8EE30EAB
gpg: Good signature from Mike Gerwitz (Free Software Developer)
m...@mikegerwitz.com
gpg: WARNING: This key is not certified with a trusted signature!
gpg:  There is no indication that the signature belongs to the
owner.
Primary key fingerprint: 2217 5B02 E626 BC98 D7C0  C2E5 F22B B815 8EE3 0EAB
afb1e7373ae5e7dae3caab2c64cbb18db3d96fba   Mike GerwitzModified
barG

-

But when I run that command, spaces are missing. (Using a user that does
not know my gpg public key for testing purposes.) See output:

-

user2@host:/home/user/testrepo$ git log
--pretty=format:%H$t%aN$t%s$t%G? --show-signature
gpg: Signature made Thu 04 Dec 2014 04:37:58 PM UTC using RSA key ID
77BB3C48
gpg: Good signature from Patrick Schleizer adrela...@riseup.net
gpg: WARNING: This key is not certified with a trusted signature!
gpg:  There is no indication that the signature belongs to the
owner.
Primary key fingerprint: 916B 8D99 C38E AF5E 8ADC  7A2A 8D66 066A 2EEA CCDA
 Subkey fingerprint: 6E97 9B28 A6F3 7C43 BE30  AFA1 CB8D 50BB 77BB 3C48
529bbc076f05c13023580ea7be7ba63aba3e9672Patrick Schleizersigned commit 2U
gpg: Signature made Thu 04 Dec 2014 04:29:32 PM UTC using RSA key ID
77BB3C48
gpg: Good signature from Patrick Schleizer adrela...@riseup.net
gpg: WARNING: This key is not certified with a trusted signature!
gpg:  There is no indication that the signature belongs to the
owner.
Primary key fingerprint: 916B 8D99 C38E AF5E 8ADC  7A2A 8D66 066A 2EEA CCDA
 Subkey fingerprint: 6E97 9B28 A6F3 7C43 BE30  AFA1 CB8D 50BB 77BB 3C48
ea1615ac1a9fe9f957f91f54a33a60d57828a32fPatrick Schleizersigned commitU
75e79a211963907afd3a6d2f28c3571d37140231Patrick Schleizerreal long
commit msg Please enter the commit message for your changes. Lines
starting with '#' will be ignored, and an empt
30096d1633ef22463c1a770288755ae5325f1242Patrick Schleizer2N
e7be12378d2805bebe531bd01cbec9dec1f79032Patrick Schleizerinitial commitN
(END)

-

Any idea? Am I doing something wrong?

I am asking, because therefore Mike Gerwitz's Quote Signature Check
Script With Web Of Trust verification script (
http://mikegerwitz.com/papers/git-horror-story#script-trust ) does not
work for me.

Mike, could you please put your various git commit verification helper
scripts into a publicly visible?

By the way, any chance that these useful helper scripts could make their
way into the official distribution of git as a stopgap until native git
commit verification support gets improved?

Cheers,
Patrick
--
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: Enhancement Request: locale git option

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 4, 2014 at 5:12 PM, Michael J Gruber
g...@drmicha.warpmail.net wrote:
 Ævar Arnfjörð Bjarmason schrieb am 04.12.2014 um 16:49:
 On Thu, Dec 4, 2014 at 10:55 AM, Jeff King p...@peff.net wrote:
 On Thu, Dec 04, 2014 at 09:29:04AM +0100, Torsten Bögershausen wrote:

 How about
 alias git='LANGUAGE=de_DE.UTF-8 git'
 in your ~/.profile ?
 (Of course you need to change de to the language you want )

 Besides being awkward in scripts (which will not respect the alias and
 use a different language!), that variable will also be inherited by
 programs git spawns. So the editor, for example, may end up in the wrong
 language.

 I think respecting core.locale would make sense (probably the change
 would go into git_setup_gettext(), but you may have to fight with the
 setup code over looking at config so early in the process).

 I think we should just stick to the standard *nix way of doing this:
 Tell people to set their locale in their environment.

 If someone's having this issue it's also happening for all the
 binutils, and any other command-line and GUI program they use, unless
 they override using the standard way of doing so, by setting the
 relevant LC_* environment variables.

 If you want Git in English then create an alias to override its locale
 to be C, if you want the editor it spawns to be in some other language
 alias that to something that explicitly sets LC_* for that editor.

 Maybe I'm being overzealous about this (especially with the I
 implemented this blinders on), but let's not have Git set the
 precedent for other *nix programs that they all should come up with
 some custom way to override locales, that's something to be done at
 the OS locale library level, which we use.

 However, I think the original question is not one of localizing git, but
 rather of having it _not_ localized (avoiding the German translations).
 There is a hack you can do that for that, which is to set
 GIT_TEXTDOMAINDIR to something nonsensical (like /), which will mean
 git cannot find the .po files, and just uses the builtin messages.

 You can, but the fact that that works is an internal implementation
 detail we shouldn't document or support.


 The main issue at hand is really that we have localised git but not its
 man pages. Even if you understand English, the man pages don't help you
 at all if you can't connect the technical terms used there to their
 localised counterparts in git's messages. (NO_GETTEXT=y is my solution.)

 That is one of the many reasons why I proposed to have a dictionary of
 the main technical terms for each language before we even localise git
 in that language. In an ideal word, we would provide a simple solution
 for looking these terms up both ways. I don't think we're going to have
 localised man pages any time soon, are we?

I think that's a great idea, and one that's only blocked on someone
(hint hint) sending patches for it.

It would be neat-o to have something to make translating the docs
easier, i.e. PO files for sections of the man pages. There's tools to
help with that which we could use.

But there's no reason for us not to have translated glossaries in the meantime.
--
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: [PATCHv3 00/13] the refs-transactions-reflog series

[Jonathan Nieder]

Hi,

Michael Haggerty wrote:
 On 12/04/2014 09:29 AM, Stefan Beller wrote:

 This is the whole refs-transactions-reflog series[1],
 which was in discussion for a bit already. It applies to origin/master.

 I am still unhappy with the approach of this series, for the reasons
 that I explained earlier [1]. In short, I think that the abstraction
 level is wrong. In my opinion, consumers of the refs API should barely
 even have to *know* about reflogs, let alone implement reflog expiration
 themselves.

Would it make sense to propose competing documentation patches (to
Documentation/technical/api-ref-transactions.txt, or to refs.h), so
we can work out the API that way?

I don't think the API questions that we're talking about would end up
affecting the details of how the files backend implements them too
much.

Jonathan
--
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 06/14] lockfile: introduce flag for locks outside .git

[Jonathan Nieder]

Junio C Hamano wrote:

 We may want to see it done the other way around, though.  That is,
 to allow the pack-refs race fix, which was reviewed and has been
 cooking, graduate without having to depend on this API rewrite, so
 that we may be able to merge it down even to v2.2.x maintenance
 track.

Good idea.  When I send out the reroll of this API change today, it
will be based on top of master + that patch.

Thanks,
Jonathan
--
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] gc: support temporarily preserving garbage

[Junio C Hamano]

Jeff King p...@peff.net writes:

 On Wed, Dec 03, 2014 at 01:21:03PM -0800, Brodie Rao wrote:

  I think it is also not sufficient. This patch seems to cover only
  objects. But we assume that we can atomically rename() new versions of
  files into place whenever we like without disrupting existing readers.
  This is the case for ref updates (and packed-refs), as well as the index
  file.  The destination end of the rename is an unlink() in disguise, and
  would be susceptible to the same problems.
 
 I'm not aware of renaming over files happening anywhere in gc-related
 code. Do you think that's something that would need to be addressed in
 the rest of the code base before going forward with this garbage
 directory approach? If so, do you have any suggestions on how to
 tackle that problem?

 As an example, if you run git pack-refs --all --prune (which is run by
 git gc), it will create a new pack-refs file and rename it into place.
 Another git program (say, git for-each-ref) might be reading the file
 at the same time. If you run pack-refs and for-each-ref simultaneously
 in tight loops on your problematic NFS setup, what happens?

 I have no idea if it breaks or not. I don't have such a misbehaving
 system, and I don't know how rename() is implemented on it. But if it
 _is_ a problem of the same variety, then I don't see much point in
 making an invasive fix to address half of the problem areas, but not the
 other half (i.e., if we are still left with a broken git in this setup,
 was the invasive fix worth the cost?).

 If it is a problem (and again, I am just guessing), I'd imagine you
 would need a similar setup to what you proposed for unlink(): before
 renaming packed-refs.lock into packed-refs, hard-link it into your
 trash area. And you'd probably want to intercept rename() there, to
 catch all places where we use this technique.

Also we need to take it into account that it is not an issue unique
to Git that the server side may expire these .nfsX entries left
by an NFS client (silly rename) to keep files that have been
removed or renamed away alive.  Aren't there a knob on the NFS
server end to control how long these are kept unexpired to avoid
stale filehandle errors, so that not just Git but all applications
running on NFS client machines will not be hurt by it?

Working it around at the application program level for each and
every application that runs on a machine that can NFS mount
filesystems from elsewhere may be simply madness, no?
--
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: git log --pretty=format:%H$t%aN$t%s$t%G? --show-signature

[Mike Gerwitz]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Hey Patrick,

On Thu, Dec 04, 2014 at 17:21:06 +, Patrick Schleizer wrote:
 git log --pretty=format:%H$t%aN$t%s$t%G? --show-signature
 [...]
 But when I run that command, spaces are missing. (Using a user that does
 not know my gpg public key for testing purposes.) See output:

 -

 user2@host:/home/user/testrepo$ git log
 --pretty=format:%H$t%aN$t%s$t%G? --show-signature

That is because the variable `$t' is defined in my script on the
preceding line, as a tab character.  You can insert it directly using
C-V TAB, or $'\t' in bash.

 Mike, could you please put your various git commit verification helper
 scripts into a publicly visible?

You can use this:

  
https://gitorious.org/easejs/easejs/source/ee85b058df783ffaa9f8d5ae58f9eb6d7586b0ca:tools/signchk

But note that the default value of the `chkafter' var is
ease.js-specific.

 By the way, any chance that these useful helper scripts could make their
 way into the official distribution of git as a stopgap until native git
 commit verification support gets improved?

It has since improved; I'm looking for the time to update the article,
or write a follow-up.

Git has since added other pretty formatting options as well:

  https://github.com/git/git/blob/master/Documentation/pretty-formats.txt#L140

Git v2.1.0 also added a `verify-commit' command:

  https://github.com/git/git/blob/master/Documentation/git-verify-commit.txt

I haven't used it yet.

Hope that helps.

- -- 
Mike Gerwitz
Free Software Hacker | GNU Maintainer
http://mikegerwitz.com
FSF Member #5804 | GPG Key ID: 0x8EE30EAB
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJUgKJyAAoJEPIruBWO4w6rdS8QAMEtQhklDe4zXju0uc6ksqYl
aXdXhE7HcyUDl6yWXEheXH4oCRLSthS+8MPQfuY8gae1eRvyHx3rViGpMEyB8s5B
xhAQpOLVmro0QIwIZ/HGX4IKoGVq/QyqvLNR8iqnV8GXPu+ckGIG/UvrkFFSaLW8
eFUvQLbNITViVgQljCzzfptL9dQvdra0D1EXxRk8+h8Sw4vKRN54h0tqKVw5PcsT
4sFUBVwgmzILNKydFkMu1C+pDwnemhS04PtcrpmUTniOzLPhWJiZwzgDV5j9tOPq
7noLrnw0kpm6PbX90i2+uSVGmh6zgoR69h7SAZGJEiHQj4BiZetLMwxJL25o73/c
1/9tWT/7kAcpvzAjPjRMS3BqV7AVwNqTKKblCszfunS87aWLs1t/bgUg4e6x3lTJ
JxyxkKnSnn3dzntMfB9UuuJ6bdtn1pJci4Ptvl2yzKHaZv7ImV78UIuxdthtMgMn
eBawq3wm7HBMETkDDyRSpuPOEycBSnWZL2dL4Xc9IxPKDTJUvHTRUXxy4v2Juiv9
Pogao25j6EpTlOqx29Y9Y95ITw/UdQU7NjPAGFNxIZZTgjzHrcIlaEKuoHp+t6oh
s8OPhD+FMNWBFdAda+zP785sUbyF93/2xBK/HFyTUinOLn1/BJBC0FqHfeYd1hQe
cJ0rYnOtckycQv+re9hz
=+Rub
-END PGP SIGNATURE-
--
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] completion: add git-tag options

[Ralf Thielow]

Add completion for git-tag options including
all options that are currently shown in git tag -h.

Signed-off-by: Ralf Thielow ralf.thie...@gmail.com
---
 contrib/completion/git-completion.bash | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 2fece98..cb32dc1 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2548,6 +2548,16 @@ _git_tag ()
__gitcomp_nl $(__git_refs)
;;
esac
+
+   case $cur in
+   --*)
+   __gitcomp 
+   --list --delete --verify --annotate --message --file
+   --sign --cleanup --local-user --force --column --sort
+   --contains --points-at
+   
+   ;;
+   esac
 }
 
 _git_whatchanged ()
-- 
2.2.0.269.ge4ffef3

--
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: [PATCHv3 00/13] the refs-transactions-reflog series

[Jonathan Nieder]

Michael Haggerty wrote:

 I am still unhappy with the approach of this series, for the reasons
 that I explained earlier [1]. In short, I think that the abstraction
 level is wrong. In my opinion, consumers of the refs API should barely
 even have to *know* about reflogs, let alone implement reflog expiration
 themselves.

Okay, now returning to the substance of this comment.  This is
revisiting themes from [3], so my opinions are probably not a
surprise.

I think that the API changes that you and Stefan are proposing are
consistent and could both go in.

You suggested refactoring expire_reflogs() to use a callback that
decides what to expire.  Then it doesn't have to care that the
expiration happens by creating a new reflog and copying over the
reflog entries that are not being expired.  The result is a clean
reflog expiration API.

The ref-transaction-reflog series allows those low-level steps to be
part of a ref transaction.  Any ref backend (the current files-based
backend or a future other one) would get a chance to reimplement those
low-level steps, which are part of what happens during ref updates and
reflog deletion.  The goal is for all reflog updates to use the
transaction API, so that new ref/reflog backends only need to
implement the transaction functions.

So *both* are making good changes, with different goals.

The implementation of the reflog expiration API can use the ref
transaction API.

 Of course, reflog expiration *should* be done atomically. But that is
 the business of the refs module; callers shouldn't have to do all the
 complicated work of building the transaction themselves.

I don't understand this comment.  After the ref-transaction-reflog
series, a transaction_update_ref() call still takes care of the
corresponding reflog update without the caller having to worry about
it.

Thanks for looking it over,
Jonathan

 [1] http://thread.gmane.org/gmane.comp.version-control.git/259712/focus=259770
[3] http://thread.gmane.org/gmane.comp.version-control.git/259939/focus=259967
--
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] prompt: respect GIT_TERMINAL_PROMPT to disable terminal prompts

[Junio C Hamano]

Jeff King p...@peff.net writes:

 diff --git a/prompt.c b/prompt.c
 index e5b4938..8181eeb 100644
 --- a/prompt.c
 +++ b/prompt.c
 @@ -57,11 +57,19 @@ char *git_prompt(const char *prompt, int flags)
   r = do_askpass(askpass, prompt);
   }
  
 - if (!r)
 - r = git_terminal_prompt(prompt, flags  PROMPT_ECHO);
   if (!r) {
 - /* prompts already contain :  at the end */
 - die(could not read %s%s, prompt, strerror(errno));
 + const char *err;
 +
 + if (git_env_bool(GIT_TERMINAL_PROMPT, 1)) {
 + r = git_terminal_prompt(prompt, flags  PROMPT_ECHO);
 + err = strerror(errno);
 + } else {
 + err = terminal prompts disabled;
 + }
 + if (!r) {
 + /* prompts already contain :  at the end */
 + die(could not read %s%s, prompt, err);
 + }
   }
   return r;
  }

I wish this covered a lot more than just this part from an
end-user's point of view, but this is definitely one of the most
important code paths the mechanism should cover.

Thanks, will queue.
--
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: [PATCHv3 00/13] the refs-transactions-reflog series

[Stefan Beller]

On Thu, Dec 04, 2014 at 10:14:04AM -0800, Jonathan Nieder wrote:
 Michael Haggerty wrote:
 
  I am still unhappy with the approach of this series, for the reasons
  that I explained earlier [1]. In short, I think that the abstraction
  level is wrong. In my opinion, consumers of the refs API should barely
  even have to *know* about reflogs, let alone implement reflog expiration
  themselves.
 
Ok, what is a consumer for you? In my understanding the builtin/reflog.c is
a consumer of the refs API, so there we want to see clean code just telling the
refs backend to do its thing.

I think Jonathan made a good point by saying our patch series have 
different goals.

So I really like the code, which leads to 

    reflog_expiry_prepare(refname, sha1, cb.policy_cb);
    for_each_reflog_ent(refname, expire_reflog_ent, cb);
    reflog_expiry_cleanup(cb.policy_cb);

but look at the surrounding code:

if (!(flags  EXPIRE_REFLOGS_DRY_RUN)) {
if (hold_lock_file_for_update(reflog_lock, log_file, 0)  0)
...
}


if (!(flags  EXPIRE_REFLOGS_DRY_RUN)) {
if (close_lock_file(reflog_lock)) {
...
}
}

That part should also go into the refs.c backend, so in the expire_reflog
function you can just write:

transaction_begin(...)  // This does all the hold_lock_file_for_update 
magic 
// lines 457-464 in mhagger/reflog-expire-api

reflog_expiry_prepare(refname, sha1, cb.policy_cb);
for_each_reflog_ent(refname, expire_reflog_ent, cb);
reflog_expiry_cleanup(cb.policy_cb);

transaction_commit(...) // This does all the 
close_lock_file/rename/write_in_full
// lines 470-488 in mhagger/reflog-expire-api


 
 So *both* are making good changes, with different goals.

If you want to I can rebase the reflog series on top of yours to show
they can work together quite nicely.

Thanks for your draft series and feedback,
Stefan
--
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: [PATCHv3 00/13] the refs-transactions-reflog series

[Junio C Hamano]

Michael Haggerty mhag...@alum.mit.edu writes:

 ... an alternate proposal, to convert
 expire_reflogs() to take callback functions that decide *what* to
 expire, but which itself does the work of acquiring locks, iterating
 through the reflog entries, rewriting the file, and overwriting the old
 file with the new one. The goal is to move this function into refs.c and
 make builtin/reflog.c much simpler--it will only have to implement the
 callbacks that determine the expiration policy.

It sounds like a very sensible approach to me.
--
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: [PATCHv3 00/13] the refs-transactions-reflog series

[Junio C Hamano]

Stefan Beller sbel...@google.com writes:

 This is the whole refs-transactions-reflog series[1],
 which was in discussion for a bit already. It applies to origin/master.

 The idea is to have the reflog being part of the transactions, which
 the refs are already using, so the we're moving towards a database
 like API in the long run. This makes git easier to maintain as well
 as opening the possibility to replace the backend with a real database.

 If you've followed the topic a bit, start reading at patch
 [PATCH 06/13] refs.c: add a transaction function to append a reflog
 as the first 5 patches have been discussed a lot separately and
 can be found in origin/pu already[2].

 The first two patches are deduplicating code.
 The third patch is ripping some code out of log_ref_write and introduces
 log_ref_write_fd, which does the actual writing.
 The patches 4+5 are renaming variables for clarity.

Thanks.  It seems that we have a bit of hashing out the approaches
that is necessary between Michael's and this one.  I'll refrain from
picking this up for today (even though I would read it through as
time permits).  It might turn out to be necessary to drop those five
early patches from my tree when this series gets rerolled to take
input from the alternative Michael's working on, but that droppage
does not have to happen today.

--
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: [PATCHv3 00/13] the refs-transactions-reflog series

[Stefan Beller]

On Thu, Dec 4, 2014 at 10:41 AM, Junio C Hamano gits...@pobox.com wrote:
 Stefan Beller sbel...@google.com writes:

 This is the whole refs-transactions-reflog series[1],
 which was in discussion for a bit already. It applies to origin/master.

 The idea is to have the reflog being part of the transactions, which
 the refs are already using, so the we're moving towards a database
 like API in the long run. This makes git easier to maintain as well
 as opening the possibility to replace the backend with a real database.

 If you've followed the topic a bit, start reading at patch
 [PATCH 06/13] refs.c: add a transaction function to append a reflog
 as the first 5 patches have been discussed a lot separately and
 can be found in origin/pu already[2].

 The first two patches are deduplicating code.
 The third patch is ripping some code out of log_ref_write and introduces
 log_ref_write_fd, which does the actual writing.
 The patches 4+5 are renaming variables for clarity.

 Thanks.  It seems that we have a bit of hashing out the approaches
 that is necessary between Michael's and this one.  I'll refrain from
 picking this up for today (even though I would read it through as
 time permits).  It might turn out to be necessary to drop those five
 early patches from my tree when this series gets rerolled to take
 input from the alternative Michael's working on, but that droppage
 does not have to happen today.


Ok, let's see how Michaels series evolves and if I can base the
transactions series
on that afterwards. Michael has picked up 3 out the 5 patches to get
his series started,
so maybe we can resolve it nicely.
--
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: Enhancement Request: locale git option

[Ralf Thielow]

Hi Ulrich,

2014-12-04 8:32 GMT+01:00 Ulrich Windl ulrich.wi...@rz.uni-regensburg.de:
 Hi!

 I'm native German, but German git messages confuse me (yopu'll have to 
 correlate them with the man pages). At the moment git uses the

What in particular makes the German git messages confusing you? What
`git version` do you use?
Maybe we can find something to improve in the translation.

Thanks,
Ralf
--
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] introduce git root

[Junio C Hamano]

Jeff King p...@peff.net writes:

 On Tue, Dec 02, 2014 at 09:26:00AM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  There is also git var, which is a catch-all for printing some deduced
  environmental defaults. I'd be just as happy to see it go away, though.
  Having:
 
git --exec-path
git --toplevel
git --author-ident
 
  all work would make sense to me (I often get confused between git
  --foo and git rev-parse --foo when trying to get the exec-path and
  git-dir). And I don't think it's too late to move in this direction.
  We'd have to keep the old interfaces around, of course, but it would
  immediately improve discoverability and consistency.
 
 Yeah, I too think the above makes sense.  I forgot about var, but
 it should go at the same time we move kitchen-sink options out of
 rev-parse.  One less command to worry about at the UI level means
 you do not have to say if you want to learn about X, ask 'var', if
 you want to learn about Y, ask 'rev-parse', use 'git' itself for Z.

 Christian raised the issue of cluttering the git --option namespace,
 and I do agree that's a potential issue. 

I am not sure if that is an issue at all.  You will need the same
number of options to cover all the necessary computables somewhere
anyway.

git --show-this-or-that-computable is not more or not less
cluttering compared to git var --show-this-or-that-computable.

If we were to move to git var, which takes variables of these
forms:

GIT_AUTHOR_IDENT
GIT_COMMITTER_IDENT
GIT_EDITOR
GIT_PAGER

then scripts that currently use git --exec-path need to be
encouraged to instead use git var GIT_EXEC_PATH.  If we have so
many computables that cluttering may become an issue, then we
would need to come up with many new GIT_$COMPUTABLE_NAME fake
variables for consistency if we were to go with git var, no?

I understand we are not talking about removing git --exec-path,
but the desire is to have one single command the user can go to ask
about all the computables.  If var is to become that single
command, then we need to keep the interface to it uniform and
consistent, and telling the users to use git var GIT_PAGER and
git var --exec-path in the same script will not fly well.  Also
these GIT_$COMPUTABLE_NAME appear as if they can be influenced by
setting environment variables of the same name, which invites
further confusion.  This is especially bad because some of then do
get affected by environment (i.e. GIT_EDITOR=vi has effect, but
GIT_AUTHOR_IDENT=Gitster gits...@pobox.com does not).

 ... My proposal was to drop git
 var, but I'd also be OK with making git var the new kitchen sink.  It
 doesn't accept many options now, so it's fairly wide open for changing
 without losing backwards compatibility. AFAICT, the -l option is
 utterly useless, but other than that, it just takes a variable name. We
 could introduce dashed options, or just define a sane variable namespace.

If we admit that git var was a failed experiment that gained only
four fake variables for the past 10 years, it will not be too much
trouble and transition pain to turn the existing ones into option
form, like --author-ident etc., like your original proposal did, I
would think.

If we are going to deprecate git var GIT_AUTHOR_IDENT form,
i.e. the form that uses fake variable-looking strings, and uniformly
use git var --author-ident, git var --exec-path, etc., then I
would think it would work, too.  I still do not know what you gain
by using git var --exec-path over git --exec-path, though.
--
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: [RFC/PATCH 2/2] branch: allow -f with -m and -d

[Junio C Hamano]

Michael J Gruber g...@drmicha.warpmail.net writes:

 -f/--force is the standard way to force an action, and is used by branch
 for the recreation of existing branches, but not for deleting unmerged
 branches nor for renaming to an existing branch.

 Make -m -f equivalent to -M and -d -f equivalent to -D, i.e.
 allow -f/--force to be used with -m/-d also.

I like that goal.  And I agree with your s/force_create/force/g
remark on the cover, too.



 Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net
 ---
  builtin/branch.c  | 9 +++--
  t/t3200-branch.sh | 5 +
  2 files changed, 12 insertions(+), 2 deletions(-)

 diff --git a/builtin/branch.c b/builtin/branch.c
 index 3b79c50..8ea04d7 100644
 --- a/builtin/branch.c
 +++ b/builtin/branch.c
 @@ -848,7 +848,7 @@ int cmd_branch(int argc, const char **argv, const char 
 *prefix)
   OPT_BOOL('l', create-reflog, reflog, N_(create the branch's 
 reflog)),
   OPT_BOOL(0, edit-description, edit_description,
N_(edit the description for the branch)),
 - OPT__FORCE(force_create, N_(force creation (when already 
 exists))),
 + OPT__FORCE(force_create, N_(force creation, move/rename, 
 deletion)),
   {
   OPTION_CALLBACK, 0, no-merged, merge_filter_ref,
   N_(commit), N_(print only not merged branches),
 @@ -891,7 +891,7 @@ int cmd_branch(int argc, const char **argv, const char 
 *prefix)
   if (with_commit || merge_filter != NO_FILTER)
   list = 1;
  
 - if (!!delete + !!rename + !!force_create + !!new_upstream +
 + if (!!delete + !!rename + !!new_upstream +

This puzzled me but earlier -f implied creation and no other mode
(hence it was an error to give it together with delete and other
modes), but now -f is merely a do forcibly whatever mode of
operation other option determines that does not conflict.

What should -f -u and -f -l do, then, though?

   list + unset_upstream  1)
   usage_with_options(builtin_branch_usage, options);
  
 @@ -904,6 +904,11 @@ int cmd_branch(int argc, const char **argv, const char 
 *prefix)
   colopts = 0;
   }
  
 + if (force_create) {
 + delete *= 2;
 + rename *= 2;
 + }
 +
   if (delete) {
   if (!argc)
   die(_(branch name required));
 diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
 index 0b3b8f5..ddea498 100755
 --- a/t/t3200-branch.sh
 +++ b/t/t3200-branch.sh
 @@ -106,6 +106,11 @@ test_expect_success 'git branch -M o/q o/p should work 
 when o/p exists' '
   git branch -M o/q o/p
  '
  
 +test_expect_success 'git branch -m -f o/q o/p should work when o/p exists' '
 + git branch o/q 
 + git branch -m -f o/q o/p
 +'
 +
  test_expect_success 'git branch -m q r/q should fail when r exists' '
   git branch q 
   git branch r 
--
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: [RFC][PATCH] send-email: add --[no-]xmailer option

[Luis Henriques]

On Wed, Dec 03, 2014 at 08:56:45AM -0800, Junio C Hamano wrote:
 Eric Wong normalper...@yhbt.net writes:
 
  Luis Henriques hen...@camandro.org wrote:
  On Mon, Mar 24, 2014 at 09:38:27PM +, Luis Henriques wrote:
   Add --[no-]xmailer that allows a user to disable adding the 'X-Mailer:'
   header to the email being sent.
  
  
  Ping
  
  It's been a while since I sent this patch.  Is there any interest in
  having this switch in git-send-email?
 
  I wasn't paying attention when the original was sent, but this
  looks good to me.
 
  Acked-by: Eric Wong normalper...@yhbt.net
 
  I honestly don't like disclosing too much information about my system,
  in this case which MUA I'm using and its version.
 
  Right on.  I would even favor this being the default.
 
  Auto-generated Message-Id headers also shows the use of git-send-email;
  perhaps there can be a way to configure that, too.  However,
  git-send-email respects manually-added Message-Id headers in the
  original patch, so it's less of a problem, I suppose.
 
 I actually do not think this is a good idea from debuggability.

Do you think this could be merged with yet another switch?  I can't
think of a name for the switch, something like... --hide-msgid?

Another option would be to re-work the --no-xmailer switch to change
it into a --hide-id (or something), where both the X-Mailer:
header would be dropped and the Message-id would be obfuscated.

Cheers,
-- 
Luís
--
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] compat: convert modes to use portable file type values

[David Michael]

On Wed, Dec 3, 2014 at 9:24 PM, David Michael fedora@gmail.com wrote:
 --- /dev/null
 +++ b/compat/stat.c
 @@ -0,0 +1,49 @@
 +#define _POSIX_C_SOURCE 200112L
 +#include stddef.h/* NULL */
 +#include sys/stat.h  /* *stat, S_IS* */
 +#include sys/types.h /* mode_t   */

Oops, the stddef.h line can be removed now that this is no longer
testing for NULL.  Let me know if this warrants a v3 if there is no
other feedback.

Thanks.

David
--
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: [PATCHv3 00/13] the refs-transactions-reflog series

[Jonathan Nieder]

Stefan Beller wrote:

 Ok, let's see how Michaels series evolves and if I can base the
 transactions series
 on that

That sounds good to me, too, FWIW.  That way we don't have to fuss
with conflicts in refs.c. :)

Thanks,
Jonathan
--
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: [RFC][PATCH] send-email: add --[no-]xmailer option

[Junio C Hamano]

Luis Henriques hen...@camandro.org writes:

 On Wed, Dec 03, 2014 at 08:56:45AM -0800, Junio C Hamano wrote:

 I actually do not think this is a good idea from debuggability.

 Do you think this could be merged with yet another switch?  I can't
 think of a name for the switch, something like... --hide-msgid?

In case it wasn't clear, by this I meant the removal of
X-Mailer:, iow, Adding --no-xmailer option is a bad idea from
debuggability's point of view.

Not adding message-id is not an option; MSAs are supposed to always
add one if they want to be RFC compliant, aren't they?
--
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] test/send-email: --[no-]xmailer tests

[Luis Henriques]

Add tests for the --[no-]xmailer option.

Signed-off-by: Luis Henriques hen...@camandro.org
---
 t/t9001-send-email.sh | 32 
 1 file changed, 32 insertions(+)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index e37efef..7a3f996 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1556,5 +1556,37 @@ test_expect_success $PREREQ 
'sendemail.aliasfile=~/.mailrc' '
2errors out 
grep ^!someone@example\.org!$ commandline1
 '
+do_xmailer_test() {
+   expected=$1
+   params=$2
+   git format-patch -1
+   git send-email \
+   --from=Example nob...@example.com \
+   --to=some...@example.com \
+   --smtp-server=$(pwd)/fake.sendmail \
+   $params \
+   0001-*.patch \
+   2errors out
+   test z$(grep ^X-Mailer: out | wc -l) = z$expected
+   return $?
+}
+
+test_expect_success $PREREQ '--xmailer uses X-Mailer header' '
+   do_xmailer_test 1 --xmailer
+'
+
+test_expect_success $PREREQ '--no-xmailer supresses X-Mailer header' '
+   do_xmailer_test 0 --no-xmailer
+'
+
+test_expect_success $PREREQ 'sendemail.xmailer=true uses X-Mailer header' '
+   git config sendemail.xmailer true 
+   do_xmailer_test 1 
+'
+
+test_expect_success $PREREQ 'sendemail.xmailer=false supresses X-Mailer 
header' '
+   git config sendemail.xmailer false 
+   do_xmailer_test 0 
+'
 
 test_done
--
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] compat: convert modes to use portable file type values

[Junio C Hamano]

David Michael fedora@gmail.com writes:

 On Wed, Dec 3, 2014 at 9:24 PM, David Michael fedora@gmail.com wrote:
 --- /dev/null
 +++ b/compat/stat.c
 @@ -0,0 +1,49 @@
 +#define _POSIX_C_SOURCE 200112L
 +#include stddef.h/* NULL */
 +#include sys/stat.h  /* *stat, S_IS* */
 +#include sys/types.h /* mode_t   */

 Oops, the stddef.h line can be removed now that this is no longer
 testing for NULL.  Let me know if this warrants a v3 if there is no
 other feedback.

Let me queue with this squashed in for now.

 compat/stat.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/compat/stat.c b/compat/stat.c
index c2d4711..a2d3931 100644
--- a/compat/stat.c
+++ b/compat/stat.c
@@ -1,5 +1,4 @@
 #define _POSIX_C_SOURCE 200112L
-#include stddef.h/* NULL */
 #include sys/stat.h  /* *stat, S_IS* */
 #include sys/types.h /* mode_t   */
 
--
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: [RFC][PATCH] send-email: add --[no-]xmailer option

[Luis Henriques]

On Thu, Dec 04, 2014 at 11:33:24AM -0800, Junio C Hamano wrote:
 Luis Henriques hen...@camandro.org writes:
 
  On Wed, Dec 03, 2014 at 08:56:45AM -0800, Junio C Hamano wrote:
 
  I actually do not think this is a good idea from debuggability.
 
  Do you think this could be merged with yet another switch?  I can't
  think of a name for the switch, something like... --hide-msgid?
 
 In case it wasn't clear, by this I meant the removal of
 X-Mailer:, iow, Adding --no-xmailer option is a bad idea from
 debuggability's point of view.


Oh, ok.  I thought you were talking about the message-id.

 Not adding message-id is not an option; MSAs are supposed to always
 add one if they want to be RFC compliant, aren't they?

Yes, of course -- having a message ID is a requirement.  But I was
hoping you could accept a solution similar to the one suggested by
Kyle (adding him to Cc): he was suggesting hashing the message ID,
which would be a good compromise, I believe.

Cheers,
-- 
Luís
--
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] introduce git root

[Matthieu Moy]

Junio C Hamano gits...@pobox.com writes:

 Jeff King p...@peff.net writes:

 Christian raised the issue of cluttering the git --option
 namespace,
 and I do agree that's a potential issue. 

 I am not sure if that is an issue at all. You will need the same
 number of options to cover all the necessary computables somewhere
 anyway.

 git --show-this-or-that-computable is not more or not less
 cluttering compared to git var --show-this-or-that-computable.

I disagree.

Right now, a user reading man git sees --version, --help, -C,
--exec-path, --html-path, --man-path, ... at a flat list (it's actually
the first thing he can read from the man page).

The point of having commands is to make the features hierarchic. git
rebase do many things, but these things are all grouped under the
command git rebase.

Indeed, I would find git var to be a nice place to group the show me
such or such path. Not sure it's worth the trouble of changing the
existing git --*-path, but I think git var should be the place for
new things.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 v2] Squashed changes for multiple worktrees vs. submodules

[Jens Lehmann]

Am 02.12.2014 um 23:16 schrieb Max Kirillov:

On Tue, Dec 02, 2014 at 09:45:24PM +0100, Jens Lehmann wrote:

But, while hacking the submodule init I became more
convinced that the modules directory should be common and
submodules in checkout should be a checkouts of the
submodule. Because this is looks like concept of
submodules, that they are unique for the lifetime of
repository, even if they do not exist in all revisions.
And if anybody want to use fully independent checkout
they can be always checked out manually. Actually, after
a submodule is initialized and have a proper gitlink, it
can be updated and inquired regardless of where it points
to.


If I understand you correctly you want to put the
submodule's common git dir under the superproject's common
git dir. I agree that that makes most sense as the
default, but having the possibility to use a common git
dir for submodule's of different superprojects would be
nice to have for some setups, e.g. CI-servers. But that
can be added later.


So far there is no separation of .git/config for different
worktrees. As submodules rely on the config their separation
cannot be done fully without changing that. So this should
be really left for some later improvements.


Wow, so the .git/config is shared between all worktrees? I
suspect you have very good reasons for that, but I believe
that'll make multiple work trees surprise the user from time
to time when used with submodules. Because initializing a
submodule in one worktree initializes it in all other
worktrees too (I suspect other users regard git submodule
init being a worktree local command too). And setting
submodule.name.update to none will also affect all
other worktrees. But I'd need to have separate settings for
our CI server, e.g. to checkout the sources without the
largish documentation submodule in one test job (=worktree)
while checking out the whole documentation for another job
building the setup in another worktree.

And if I understand the checkout: reject if the branch is
already checked out elsewhere thread correctly, I won't be
able to build master in two jobs at the same time?

So two reasons against using multiple worktrees on our CI
server to save quite some disk space :-(


Thanks. I just didn't quite understand why you had to do so many
changes to git-submodule.sh. Wouldn't it be sufficient to just
update module_clone()?


Thanks, I should try it.

Probably I had the opposite idea in mind - keep module_clone
as untouched as possible. Maybe I should see how it's going
to look if I move all worktrees logic there.


Thanks. But I changed my mind about the details (now that I
know about .git/config and multiple worktrees). I think you'd
have to connect a .git directory in the submodule to the
common git dir directly, as you cannot use the core.worktree
setting (which could be different between commits due to
renames) when putting it into worktree/.git/modules. And
then you couldn't remove or rename a submodule anymore,
because that fails when it contains a .git directory.

Seems like we should put a Warning: may do unexpected things
when used with submodules (with some examples about what might
happen) in the multiple worktrees documentation. And I don't
believe anymore that teaching submodules to use the common git
dir makes that much sense after I know about the restrictions
it imposes.

Or am I misunderstanding anything?
--
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] test/send-email: --[no-]xmailer tests

[Junio C Hamano]

Luis Henriques hen...@camandro.org writes:

 Add tests for the --[no-]xmailer option.

 Signed-off-by: Luis Henriques hen...@camandro.org

Thanks.  Let's squash this in, too.  We care about command line
options taking precedence over configured default.

 t/t9001-send-email.sh | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index bcd5bad..bb573ef 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1406,6 +1406,7 @@ test_expect_success $PREREQ 
'sendemail.aliasfile=~/.mailrc' '
  2errors out 
grep ^!someone@example\.org!$ commandline1
 '
+
 do_xmailer_test() {
expected=$1
params=$2
@@ -1421,22 +1422,23 @@ do_xmailer_test() {
return $?
 }
 
-test_expect_success $PREREQ '--xmailer uses X-Mailer header' '
-   do_xmailer_test 1 --xmailer
-'
-
-test_expect_success $PREREQ '--no-xmailer supresses X-Mailer header' '
-   do_xmailer_test 0 --no-xmailer
+test_expect_success $PREREQ '--[no-]xmailer without any configuration' '
+   do_xmailer_test 1 --xmailer 
+   do_xmailer_test 0 --no-xmailer
 '
 
-test_expect_success $PREREQ 'sendemail.xmailer=true uses X-Mailer header' '
-   git config sendemail.xmailer true 
-   do_xmailer_test 1 
+test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=true' '
+   test_config sendemail.xmailer true 
+   do_xmailer_test 1  
+   do_xmailer_test 0 --no-xmailer 
+   do_xmailer_test 1 --xmailer
 '
 
-test_expect_success $PREREQ 'sendemail.xmailer=false supresses X-Mailer 
header' '
-   git config sendemail.xmailer false 
-   do_xmailer_test 0 
+test_expect_success $PREREQ '--[no-]xmailer with sendemail.xmailer=false' '
+   test_config sendemail.xmailer false 
+   do_xmailer_test 0  
+   do_xmailer_test 0 --no-xmailer 
+   do_xmailer_test 1 --xmailer
 '
 
 test_done
--
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] check-ignore: clarify treatment of tracked files

[Junio C Hamano]

Michael J Gruber g...@drmicha.warpmail.net writes:

 By default, check-ignore does not list tracked files at all since
 they are not subject to ignore patterns.

 Make this clearer in the man page.

 Reported-by: Guilherme guibuf...@gmail.com
 Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net
 ---
 That really is a bit confusing. Does this help?

Thanks.

git check-ignore is a tool to debug your .gitignore settings when
your expectation does not match the reality, so having this new
sentence here is a good thing to do, but I wonder if there is a more
prominent and central place where people learn about the ignore
mechanism the first place.  If we had this sentence there, too, that
may reduce the need to debug their .gitignore settings in the first
place.

Perhaps Documentation/gitignore.txt?  Documentation/user-manual.txt?



  Documentation/git-check-ignore.txt | 3 +++
  1 file changed, 3 insertions(+)

 diff --git a/Documentation/git-check-ignore.txt 
 b/Documentation/git-check-ignore.txt
 index ee2e091..788a011 100644
 --- a/Documentation/git-check-ignore.txt
 +++ b/Documentation/git-check-ignore.txt
 @@ -21,6 +21,9 @@ the exclude mechanism) that decides if the pathname is 
 excluded or
  included.  Later patterns within a file take precedence over earlier
  ones.
  
 +By default, tracked files are not shown at all since they are not
 +subject to exclude rules; but see `--no-index'.
 +
  OPTIONS
  ---
  -q, --quiet::
--
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: Bug in reflog of length 0x2BFF

[Junio C Hamano]

Jonathan Nieder jrnie...@gmail.com writes:

 Christoph Mallon wrote:

 % git rev-parse 'master@{52}'
 warning: Log for ref refs/heads/master has gap after Thu, 1 Jan 1970 
 00:00:01 +.
 0036

 Can you say more?  What output did you expect and how does this differ
 from it?

 I tried, with git 2.2.0,

   git init gitbug 
   cd gitbug 
   git commit --allow-empty -m a 
   wget http://tron.yamagi.org/zeug/reflog.bad 
   mv reflog.bad .git/logs/refs/heads/master 
   sha1sum .git/logs/refs/heads/master 
   git rev-parse 'master@{52}'

 The output:

  9ffe44715d0e542a60916255f144c74e6760ffd0  .git/logs/refs/heads/master
  0035

 Could you make a test script that illustrates and reproduces the
 problem?  I.e., a patch to a file like t/t1410-reflog.sh, such that
 if I run

   cd git
   make
   cd t
   ./t1410-reflog.sh

 then I can reproduce the bug?

Amen to that.  I am getting the same thing.
--
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] introduce git root

[Junio C Hamano]

Matthieu Moy matthieu@grenoble-inp.fr writes:

 Junio C Hamano gits...@pobox.com writes:

 Jeff King p...@peff.net writes:

 Christian raised the issue of cluttering the git --option
 namespace,
 and I do agree that's a potential issue. 

 I am not sure if that is an issue at all. You will need the same
 number of options to cover all the necessary computables somewhere
 anyway.

 git --show-this-or-that-computable is not more or not less
 cluttering compared to git var --show-this-or-that-computable.

 I disagree.

 Right now, a user reading man git sees --version, --help, -C,
 --exec-path, --html-path, --man-path, ... at a flat list (it's actually
 the first thing he can read from the man page).

Ahh, OK, you are worried about these --give-me-computed-values
mixed with other kinds of options.  I didn't consider that part of
the equation.

OK, git var --exec-path (and --friends), that is.
--
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: [Whonix-devel] git log --pretty=format:%H$t%aN$t%s$t%G? --show-signature

[Patrick Schleizer]

Thanks Mike!

Mike Gerwitz wrote:
 Mike, could you please put your various git commit verification helper
 scripts into a publicly visible?

 You can use this:


https://gitorious.org/easejs/easejs/source/ee85b058df783ffaa9f8d5ae58f9eb6d7586b0ca:tools/signchk

 But note that the default value of the `chkafter' var is
 ease.js-specific.

Do you have script somewhere in public git that also checks the web of
trust?

For your blog post it would be nice to have a ease.js unspecific one.

Otherwise I just wait for our updated blog post and/or for my distro to
upgrade to git 2.1.

Cheers,
Patrick
--
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: Bug in reflog of length 0x2BFF

[Christoph Mallon]

Am 04.12.14 21:18, schrieb Junio C Hamano:
 Jonathan Nieder jrnie...@gmail.com writes:
 Could you make a test script that illustrates and reproduces the
 problem?  I.e., a patch to a file like t/t1410-reflog.sh, such that
 if I run

  cd git
  make
  cd t
  ./t1410-reflog.sh

 then I can reproduce the bug?
 
 Amen to that.  I am getting the same thing.

I ran reproduce it reliably on multiple machines (OS X, FreeBSD, ia32,
amd64), a friend of mine can, too.
I already sent a test-patch, here it is again:

http://tron.yamagi.org/zeug/0001-t1410-Test-erroneous-skipping-of-reflog-entries.patch
Using this test, bisect reliably gives
4207ed285f31ad3e04f08254237c0c1a1609642b
as culprit.
It seems that Linux does not exhibit this particular behaviour.
Maybe there are differences in memory allocation, which mask the symptom.
Stefan Beller experienced some other sporadic bug regarding the reflog:
http://marc.info/?l=gitm=141748434801505w=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 2/2] prompt: respect GIT_TERMINAL_PROMPT to disable terminal prompts

[Jeff King]

On Thu, Dec 04, 2014 at 10:24:09AM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  diff --git a/prompt.c b/prompt.c
  index e5b4938..8181eeb 100644
  --- a/prompt.c
  +++ b/prompt.c
  @@ -57,11 +57,19 @@ char *git_prompt(const char *prompt, int flags)
  r = do_askpass(askpass, prompt);
  }
   
  -   if (!r)
  -   r = git_terminal_prompt(prompt, flags  PROMPT_ECHO);
  if (!r) {
  -   /* prompts already contain :  at the end */
  -   die(could not read %s%s, prompt, strerror(errno));
  +   const char *err;
  +
  +   if (git_env_bool(GIT_TERMINAL_PROMPT, 1)) {
  +   r = git_terminal_prompt(prompt, flags  PROMPT_ECHO);
  +   err = strerror(errno);
  +   } else {
  +   err = terminal prompts disabled;
  +   }
  +   if (!r) {
  +   /* prompts already contain :  at the end */
  +   die(could not read %s%s, prompt, err);
  +   }
  }
  return r;
   }
 
 I wish this covered a lot more than just this part from an
 end-user's point of view, but this is definitely one of the most
 important code paths the mechanism should cover.

Which parts do you mean? Stuff like git add -i?  I agree it might be
nice to turn that off, but it is a little bit of a different beast, in
that it reads from stdin. The git_prompt code is unique in accessing
/dev/tty directly, which makes it hard to shut off.

I don't know of any other code in git that  (and it is used in
many spots due to calls into the credential_fill code).

I suspect there's similar code in git-svn that comes from the svn
library, and it might be nice to cover that, too.

Anyway, I'm happy to give other prompts the same treatment, but I think
we can wait and add them as they are noticed.

-Peff
--
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: git log --pretty=format:%H$t%aN$t%s$t%G? --show-signature

[Junio C Hamano]

Mike Gerwitz mikegerw...@gnu.org writes:

 It has since improved; I'm looking for the time to update the article,
 or write a follow-up.

Thanks for an amusing read.  We also let you merge a signed tag
these days, so that in a variant of your merge scenario #2, your
merge commit can carry your GPG signature, made when you do the git
merge -S $other_history to merge $other_history you obtained from
your trusted colleague, as well as the signed tag your trusted
colleague made with her GPG signature.  That way, upon seeing that
merge, a third-party can verify that the merge was made by you, and
also the history of the side branch integrated to your history with
that merge is vouched by your trustred colleague.

I am however not quite sure what conclusion you are trying to drive
at by contrasting approaches #2 and #3.  The perceived problem of
approach #2, if I am reading you correctly, is that the merge is
what you vouch for but the commits on the side branch are not signed
so there is no way for you (as the merge creator) to point fingers
to when the result of the merge turns out to be problematic.  The
argument for approach #3 would be that it would give you (as the
merge creator) somebody to point fingers to if you forced others who
ask you to pull from them to sign their commits.

But I am not sure if that is the right way to look at the bigger
picture.

Imagine you are working on a project with two branches, maint and
master.  The policy adopted by the project is to use the maint
branch to prepare for the next maintenance release, which should
never add new features.  New features are to be merged to master
for the next feature release.

And imagine that you made a mistake of merging somebody else's
branch that adds a new feature, which happens to be perfectly done
and introduces no bug, to the maint branch.  Your merge is signed by
your GPG key.

Does it absolve you from blame if you can say with certainty (thanks
to GPG keys on them) that those commits on the side branch that adds
unwanted (from 'maint' policy's point of view) new feature were made
by somebody else, because the project used the approach #3?

Not really.

How would that case be any different from the case where the side
branch you merged were buggy or even malicious?  After all, your GPG
signature carries more weight than Yes, I did this random merge but
I did so without thinking about what damage it causes to the history
by pulling in other peoples' changes.  Or at least it should carry
more weight to your users who trust a history having your GPG
signature.  This history is coming from Mike whom we trust is what
your users expect, no?  When you sign your merge with merge -S,
you are vouching for the contents of the whole tree, not just I
made this merge, but I don't have anything to do with what it pulled
in.  It does not really matter to the end users where the changes
came from.  You are certifying that git diff HEAD^ HEAD after
making the merge is what you are pleased with by signing the merge.

Having said that, what approach #3 (or merging a signed tag) does
give you as the merge creator is a distrubution of trust.  You may
not have to be _so_ careful verifying git diff HEAD^ HEAD of the
merge when you know you can trust the side branch you are merging
into your history was done by somebody you trust.

But ultimately, the responsibility lies on the person who creates
the topmost merge and advances the tip of the history the users of
the end product of the project considers the authoritative one.

--
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] introduce git root

[Jeff King]

On Thu, Dec 04, 2014 at 11:02:37AM -0800, Junio C Hamano wrote:

  Christian raised the issue of cluttering the git --option namespace,
  and I do agree that's a potential issue. 
 
 I am not sure if that is an issue at all.  You will need the same
 number of options to cover all the necessary computables somewhere
 anyway.
 
 git --show-this-or-that-computable is not more or not less
 cluttering compared to git var --show-this-or-that-computable.

My issue is only that git --foo has other options besides computables.
So you need to name each option in a way that makes it clear it is
reporting a computable and not doing something else.

Take git --pager for instance. That would be a natural choice to
replace git var GIT_PAGER. But shouldn't --pager be the opposite of
the existing --no-pager?

So instead we probably need some namespace to indicate that it is a
showing option. Like --show-pager. And then for consistency, we
would probably want to move --exec-path to --show-exec-path,
creating a new --show- namespace. Or we could call that namespace
git var. :)

 I understand we are not talking about removing git --exec-path,
 but the desire is to have one single command the user can go to ask
 about all the computables.  If var is to become that single
 command, then we need to keep the interface to it uniform and
 consistent, and telling the users to use git var GIT_PAGER and
 git var --exec-path in the same script will not fly well.  Also
 these GIT_$COMPUTABLE_NAME appear as if they can be influenced by
 setting environment variables of the same name, which invites
 further confusion.  This is especially bad because some of then do
 get affected by environment (i.e. GIT_EDITOR=vi has effect, but
 GIT_AUTHOR_IDENT=Gitster gits...@pobox.com does not).

I do not think git var --exec-path is a good idea, nor GIT_EXEC_PATH
for the environment-variable confusion you mentioned. I was thinking of
just creating a new namespace, like:

  git var exec-path
  git var author-ident

and deprecating the 4 existing GIT_* variables.

 If we admit that git var was a failed experiment that gained only
 four fake variables for the past 10 years, it will not be too much
 trouble and transition pain to turn the existing ones into option
 form, like --author-ident etc., like your original proposal did, I
 would think.

I am also OK with that, if the details turn out to be not too ugly once
somebody starts digging in. I was just anticipating some ugliness in
advance. :) But I am not planning to work on it in the immediate future,
so whoever does can make that call.

-Peff
--
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: [PATCHv3 00/13] the refs-transactions-reflog series

[Michael Haggerty]

On 12/04/2014 07:32 PM, Stefan Beller wrote:
 On Thu, Dec 04, 2014 at 10:14:04AM -0800, Jonathan Nieder wrote:
 Michael Haggerty wrote:

 I am still unhappy with the approach of this series, for the reasons
 that I explained earlier [1]. In short, I think that the abstraction
 level is wrong. In my opinion, consumers of the refs API should barely
 even have to *know* about reflogs, let alone implement reflog expiration
 themselves.

 Ok, what is a consumer for you? In my understanding the builtin/reflog.c is
 a consumer of the refs API, so there we want to see clean code just telling 
 the
 refs backend to do its thing.
 
 I think Jonathan made a good point by saying our patch series have 
 different goals.
 
 So I really like the code, which leads to 
 
 reflog_expiry_prepare(refname, sha1, cb.policy_cb);
 for_each_reflog_ent(refname, expire_reflog_ent, cb);
 reflog_expiry_cleanup(cb.policy_cb);
 
 but look at the surrounding code:
 
   if (!(flags  EXPIRE_REFLOGS_DRY_RUN)) {
   if (hold_lock_file_for_update(reflog_lock, log_file, 0)  0)
   ...
   }
 
 
   if (!(flags  EXPIRE_REFLOGS_DRY_RUN)) {
   if (close_lock_file(reflog_lock)) {
   ...
   }
   }
 
 That part should also go into the refs.c backend, so in the expire_reflog
 function you can just write:
 
   transaction_begin(...)  // This does all the hold_lock_file_for_update 
 magic 
   // lines 457-464 in mhagger/reflog-expire-api
 
   reflog_expiry_prepare(refname, sha1, cb.policy_cb);
   for_each_reflog_ent(refname, expire_reflog_ent, cb);
   reflog_expiry_cleanup(cb.policy_cb);
 
   transaction_commit(...) // This does all the 
 close_lock_file/rename/write_in_full
   // lines 470-488 in mhagger/reflog-expire-api

I'm pleasantly surprised that you guys have already looked at my work in
progress. I wish I had had more time earlier today to explain what is
still to be done:

The whole point of all of the refactoring is to move expire_reflog()
(and supporting functions like expire_reflog_ent()) to refs.c. The
surrounding code that you mention above would be moved there and would
*not* need to be done by callers.

expire_reflog() will gain three callback function pointers as
parameters. The caller (in this case reflog.c) will pass pointers to
reflog_expiry_prepare(), reflog_expiry_cleanup(), and
should_expire_reflog_ent() into expire_reflog().

There is no need to wrap the code in a transaction, because the
surrounding code that you mentioned implements all the transaction
that is needed! There is no need to complicated the *ref_transaction*
interface to allow arbitrary reflog updates when all we need is this one
very special case, plus of course the reflog appends that (I claim)
should happen implicitly whenever a reference is updated [1].

 So *both* are making good changes, with different goals.
 
 If you want to I can rebase the reflog series on top of yours to show
 they can work together quite nicely.

Feel free to do what you want, but I really don't think we will ever
need transactions to handle generic reflog updates.

Meanwhile I'll try to get my series finished, including API docs as
Jonathan requested. I hope the code will be more convincing than my
prose :-)

Michael

[1] Of course, only for references that have reflogs enabled.

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
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] introduce git root

[Junio C Hamano]

Jeff King p...@peff.net writes:

 ... I was thinking of
 just creating a new namespace, like:

   git var exec-path
   git var author-ident

 and deprecating the 4 existing GIT_* variables.

I'm fine with that.  As I wrote in my response to MMoy, I forgot
about other kinds of options the git potty takes, and git var
name-without-needless-double-dashes is a perfectly fine way to
consolidate the querying of computed values in a single place.

Thanks.

--
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] prompt: respect GIT_TERMINAL_PROMPT to disable terminal prompts

[Junio C Hamano]

Jeff King p...@peff.net writes:

 On Thu, Dec 04, 2014 at 10:24:09AM -0800, Junio C Hamano wrote:

 I wish this covered a lot more than just this part from an
 end-user's point of view, but this is definitely one of the most
 important code paths the mechanism should cover.

 Which parts do you mean? Stuff like git add -i?

No, more like tag -s that eventually leads to somebody prompting
for the passphrase to unlock your GPG key---and from an end user's
point of view, that somebody is Git.

Of course, from _our_ point of view, that somebody is not us.  We do
not have direct control, certainly from this codepath.
--
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: FW: [cygwin] Cygwin's git says error: failed to read delta-pack base object

[Jason Pyeron]

 -Original Message-
 From: brian m. carlson
 Sent: Wednesday, December 03, 2014 19:55
 
 On Wed, Dec 03, 2014 at 06:31:18PM -0500, Jason Pyeron wrote:
  I remember hitting this a while ago, but just gave up.
  
  It seems to be a problem for others too.
  
  Any ideas on how to debug this so it can be patched?
  
  -Original Message-
  From: Dave Lindbergh
  Sent: Wednesday, December 03, 2014 18:07
  To: cygwin
  
  Aha - you're right.
  
  It works fine on a local NTFS volume.
  
  I get the error when I do it on Z:, which is mapped to a 
 network drive
  (on another Windows box).
  
  Is there a workaround? Why does this happen?
 
 I don't think anyone is sure.  My wild guess is that there's something
 about the way that Cygwin wraps Windows calls that makes it 
 fail in this
 case.  It might be interesting to run the Windows and Cygwin versions
 under an strace equivalent and see where things differ.

[this was posted to the cygwin list]

http://nerdfever.com/files/strace.txt

 
 It's an interesting problem, but I don't have any Windows 
 systems, so I
 can't look into it.

If it becomes a little less magic, I will chomp on the problem, but I cannot 
make a predictable test case.

-Jason

--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
-   -
- Jason Pyeron  PD Inc. http://www.pdinc.us -
- Principal Consultant  10 West 24th Street #100-
- +1 (443) 269-1555 x333Baltimore, Maryland 21218   -
-   -
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
This message is copyright PD Inc, subject to license 20080407P00. 

--
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: Bug in reflog of length 0x2BFF

[Jeff King]

On Thu, Dec 04, 2014 at 09:37:34PM +0100, Christoph Mallon wrote:

 Am 04.12.14 21:18, schrieb Junio C Hamano:
  Jonathan Nieder jrnie...@gmail.com writes:
  Could you make a test script that illustrates and reproduces the
  problem?  I.e., a patch to a file like t/t1410-reflog.sh, such that
  if I run
 
 cd git
 make
 cd t
 ./t1410-reflog.sh
 
  then I can reproduce the bug?
  
  Amen to that.  I am getting the same thing.
 
 I ran reproduce it reliably on multiple machines (OS X, FreeBSD, ia32,
 amd64), a friend of mine can, too.

Thanks, I was able to reproduce this easily on an OS X machine.

Does this patch fix your problem?

diff --git a/refs.c b/refs.c
index f1afec5..42e3a30 100644
--- a/refs.c
+++ b/refs.c
@@ -3052,7 +3052,7 @@ static int show_one_reflog_ent(struct strbuf *sb, 
each_reflog_ent_fn fn, void *c
int tz;
 
/* old SP new SP name email SP time TAB msg LF */
-   if (sb-len  83 || sb-buf[sb-len - 1] != '\n' ||
+   if (sb-len  83 ||
get_sha1_hex(sb-buf, osha1) || sb-buf[40] != ' ' ||
get_sha1_hex(sb-buf + 41, nsha1) || sb-buf[81] != ' ' ||
!(email_end = strchr(sb-buf + 82, '')) ||


I think the bug is in the reverse-reflog reader in
for_each_reflog_ent_reverse. It reads BUFSIZ chunks of the file in
reverse order, and then parses them individually. If the trailing
newline for a line falls directly on the block boundary, we may not have
it in our current block, and pass the line to show_one_reflog_ent
without a trailing newline. That function is picky about making sure it
got a full line.

So this is a long-standing bug in for_each_reflog_ent_reverse. It just
showed up recently because we started using that function for
read_ref_at_ent.

I haven't confirmed yet, but I suspect the problem shows up on OS X and
FreeBSD but not Linux because of the definition of BUFSIZ (so it is
really probably glibc versus BSD libc). The same bug exists on Linux,
but you would need different input to stimulate the newline at the right
spot.

The above is a workaround. I think the right solution is probably to
teach for_each_reflog_ent_reverse to makes sure the trailing newline is
included (either by tweaking the reverse code, or conditionally adding
it to the parsed buffer).

-Peff
--
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: Bug in reflog of length 0x2BFF

[Junio C Hamano]

Christoph Mallon mal...@cs.uni-saarland.de writes:

 Amen to that.  I am getting the same thing.

 I ran reproduce it reliably on multiple machines (OS X, FreeBSD, ia32,

Oh, I do not doubt you see a problem.  I am just saying that I don't
have an easy entry to the issue without it reproducing for me.
--
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: Bug in reflog of length 0x2BFF

[Junio C Hamano]

Jeff King p...@peff.net writes:

 I think the bug is in the reverse-reflog reader in
 for_each_reflog_ent_reverse. It reads BUFSIZ chunks of the file in
 reverse order, and then parses them individually. If the trailing
 newline for a line falls directly on the block boundary, we may not have
 it in our current block, and pass the line to show_one_reflog_ent
 without a trailing newline.

Ahh, thanks for helping spot it.  A code that uses BUFSIZ explains
why a single reproduction recipe is platform dependent.

 So this is a long-standing bug in for_each_reflog_ent_reverse. It just
 showed up recently because we started using that function for
 read_ref_at_ent.
 ...
 The above is a workaround. I think the right solution is probably to
 teach for_each_reflog_ent_reverse to makes sure the trailing newline is
 included (either by tweaking the reverse code, or conditionally adding
 it to the parsed buffer).

Sounds correct.  Unfortunately I no longer remember how I decided to
deal with a line that spans the block boundary in that piece of code
X-.


--
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 01/23] refs.c: make ref_transaction_create a wrapper for ref_transaction_update

[Michael Haggerty]

From: Ronnie Sahlberg sahlb...@google.com

The ref_transaction_update function can already be used to create refs by
passing null_sha1 as the old_sha1 parameter. Simplify by replacing
transaction_create with a thin wrapper.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 refs.c | 27 ++-
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/refs.c b/refs.c
index 5ff457e..005eb18 100644
--- a/refs.c
+++ b/refs.c
@@ -3623,31 +3623,8 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
   int flags, const char *msg,
   struct strbuf *err)
 {
-   struct ref_update *update;
-
-   assert(err);
-
-   if (transaction-state != REF_TRANSACTION_OPEN)
-   die(BUG: create called for transaction that is not open);
-
-   if (!new_sha1 || is_null_sha1(new_sha1))
-   die(BUG: create ref with null new_sha1);
-
-   if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-   strbuf_addf(err, refusing to create ref with bad name %s,
-   refname);
-   return -1;
-   }
-
-   update = add_update(transaction, refname);
-
-   hashcpy(update-new_sha1, new_sha1);
-   hashclr(update-old_sha1);
-   update-flags = flags;
-   update-have_old = 1;
-   if (msg)
-   update-msg = xstrdup(msg);
-   return 0;
+   return ref_transaction_update(transaction, refname, new_sha1,
+ null_sha1, flags, 1, msg, err);
 }
 
 int ref_transaction_delete(struct ref_transaction *transaction,
-- 
2.1.3

--
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 07/23] expire_reflog(): use a lock_file for rewriting the reflog file

[Michael Haggerty]

We don't actually need the locking functionality, because we already
hold the lock on the reference itself, which is how the reflog file is
locked. But the lock_file code still does some of the bookkeeping for
us and is more careful than the old code here was.

For example:

* Correctly handle the case that the reflog lock file already exists
  for some reason or cannot be opened.

* Correctly clean up the lockfile if the program dies.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 37 ++---
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index a282e60..d344d45 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const 
unsigned char *sha1, int
return 0;
 }
 
+static struct lock_file reflog_lock;
+
 static int expire_reflog(const char *refname, const unsigned char *sha1, void 
*cb_data)
 {
struct cmd_reflog_expire_cb *cmd = cb_data;
struct expire_reflog_cb cb;
struct ref_lock *lock;
-   char *log_file, *newlog_path = NULL;
+   char *log_file;
struct commit *tip_commit;
struct commit_list *tips;
int status = 0;
@@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1, void *c
unlock_ref(lock);
return 0;
}
+
log_file = git_pathdup(logs/%s, refname);
if (!cmd-dry_run) {
-   newlog_path = git_pathdup(logs/%s.lock, refname);
-   cb.newlog = fopen(newlog_path, w);
+   if (hold_lock_file_for_update(reflog_lock, log_file, 0)  0)
+   goto failure;
+   cb.newlog = fdopen_lock_file(reflog_lock, w);
+   if (!cb.newlog)
+   goto failure;
}
 
cb.cmd = cmd;
@@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1, void *c
}
 
if (cb.newlog) {
-   if (fclose(cb.newlog)) {
-   status |= error(%s: %s, strerror(errno),
-   newlog_path);
-   unlink(newlog_path);
+   if (close_lock_file(reflog_lock)) {
+   status |= error(Couldn't write %s: %s, log_file,
+   strerror(errno));
} else if (cmd-updateref 
(write_in_full(lock-lock_fd,
sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
@@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1, void *c
 close_ref(lock)  0)) {
status |= error(Couldn't write %s,
lock-lk-filename.buf);
-   unlink(newlog_path);
-   } else if (rename(newlog_path, log_file)) {
-   status |= error(cannot rename %s to %s,
-   newlog_path, log_file);
-   unlink(newlog_path);
+   rollback_lock_file(reflog_lock);
+   } else if (commit_lock_file(reflog_lock)) {
+   status |= error(cannot rename %s.lock to %s,
+   log_file, log_file);
} else if (cmd-updateref  commit_ref(lock)) {
status |= error(Couldn't set %s, lock-ref_name);
-   } else {
-   adjust_shared_perm(log_file);
}
}
-   free(newlog_path);
free(log_file);
unlock_ref(lock);
return status;
+
+ failure:
+   rollback_lock_file(reflog_lock);
+   free(log_file);
+   unlock_ref(lock);
+   return -1;
 }
 
 static int collect_reflog(const char *ref, const unsigned char *sha1, int 
unused, void *cb_data)
-- 
2.1.3

--
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 12/23] expire_reflog(): move updateref to flags argument

[Michael Haggerty]

The policy objects don't care about --updateref. So move it to
expire_reflog()'s flags parameter.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index a490193..597c547 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -24,7 +24,6 @@ struct cmd_reflog_expire_cb {
struct rev_info revs;
int stalefix;
int rewrite;
-   int updateref;
int verbose;
unsigned long expire_total;
unsigned long expire_unreachable;
@@ -415,7 +414,8 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb 
*cb)
 static struct lock_file reflog_lock;
 
 enum expire_reflog_flags {
-   EXPIRE_REFLOGS_DRY_RUN = 1  0
+   EXPIRE_REFLOGS_DRY_RUN = 1  0,
+   EXPIRE_REFLOGS_UPDATE_REF = 1  1
 };
 
 static int expire_reflog(const char *refname, const unsigned char *sha1,
@@ -460,7 +460,7 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1,
if (close_lock_file(reflog_lock)) {
status |= error(Couldn't write %s: %s, log_file,
strerror(errno));
-   } else if (cmd-updateref 
+   } else if ((flags  EXPIRE_REFLOGS_UPDATE_REF) 
(write_in_full(lock-lock_fd,
sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
 write_str_in_full(lock-lock_fd, \n) != 1 ||
@@ -471,7 +471,7 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1,
} else if (commit_lock_file(reflog_lock)) {
status |= error(cannot rename %s.lock to %s,
log_file, log_file);
-   } else if (cmd-updateref  commit_ref(lock)) {
+   } else if ((flags  EXPIRE_REFLOGS_UPDATE_REF)  
commit_ref(lock)) {
status |= error(Couldn't set %s, lock-ref_name);
}
}
@@ -663,7 +663,7 @@ static int cmd_reflog_expire(int argc, const char **argv, 
const char *prefix)
else if (!strcmp(arg, --rewrite))
cb.rewrite = 1;
else if (!strcmp(arg, --updateref))
-   cb.updateref = 1;
+   flags |= EXPIRE_REFLOGS_UPDATE_REF;
else if (!strcmp(arg, --all))
do_all = 1;
else if (!strcmp(arg, --verbose))
@@ -745,7 +745,7 @@ static int cmd_reflog_delete(int argc, const char **argv, 
const char *prefix)
else if (!strcmp(arg, --rewrite))
cb.rewrite = 1;
else if (!strcmp(arg, --updateref))
-   cb.updateref = 1;
+   flags |= EXPIRE_REFLOGS_UPDATE_REF;
else if (!strcmp(arg, --verbose))
cb.verbose = 1;
else if (!strcmp(arg, --)) {
-- 
2.1.3

--
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 20/23] reflog_expire(): new function in the reference API

[Michael Haggerty]

Move expire_reflog() into refs.c and rename it to reflog_expire().
Turn the three policy functions into function pointers that are passed
into reflog_expire(). Add function prototypes and documentation to
refs.h.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 133 +++
 refs.c   | 114 +++
 refs.h   |  45 +++
 3 files changed, 174 insertions(+), 118 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index c30936bb..49c64f9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -20,13 +20,6 @@ static const char reflog_delete_usage[] =
 static unsigned long default_reflog_expire;
 static unsigned long default_reflog_expire_unreachable;
 
-enum expire_reflog_flags {
-   EXPIRE_REFLOGS_DRY_RUN = 1  0,
-   EXPIRE_REFLOGS_UPDATE_REF = 1  1,
-   EXPIRE_REFLOGS_VERBOSE = 1  2,
-   EXPIRE_REFLOGS_REWRITE = 1  3
-};
-
 struct cmd_reflog_expire_cb {
struct rev_info revs;
int stalefix;
@@ -48,13 +41,6 @@ struct expire_reflog_policy_cb {
struct commit_list *tips;
 };
 
-struct expire_reflog_cb {
-   unsigned int flags;
-   void *policy_cb;
-   FILE *newlog;
-   unsigned char last_kept_sha1[20];
-};
-
 struct collected_reflog {
unsigned char sha1[20];
char reflog[FLEX_ARRAY];
@@ -330,38 +316,6 @@ static int should_expire_reflog_ent(unsigned char *osha1, 
unsigned char *nsha1,
return 0;
 }
 
-static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
-   const char *email, unsigned long timestamp, int tz,
-   const char *message, void *cb_data)
-{
-   struct expire_reflog_cb *cb = cb_data;
-   struct expire_reflog_policy_cb *policy_cb = cb-policy_cb;
-
-   if (cb-flags  EXPIRE_REFLOGS_REWRITE)
-   osha1 = cb-last_kept_sha1;
-
-   if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
-message, policy_cb)) {
-   if (!cb-newlog)
-   printf(would prune %s, message);
-   else if (cb-flags  EXPIRE_REFLOGS_VERBOSE)
-   printf(prune %s, message);
-   } else {
-   if (cb-newlog) {
-   char sign = (tz  0) ? '-' : '+';
-   int zone = (tz  0) ? (-tz) : tz;
-   fprintf(cb-newlog, %s %s %s %lu %c%04d\t%s,
-   sha1_to_hex(osha1), sha1_to_hex(nsha1),
-   email, timestamp, sign, zone,
-   message);
-   hashcpy(cb-last_kept_sha1, nsha1);
-   }
-   if (cb-flags  EXPIRE_REFLOGS_VERBOSE)
-   printf(keep %s, message);
-   }
-   return 0;
-}
-
 static int push_tip_to_list(const char *refname, const unsigned char *sha1,
int flags, void *cb_data)
 {
@@ -428,75 +382,6 @@ static void reflog_expiry_cleanup(void *cb_data)
}
 }
 
-static struct lock_file reflog_lock;
-
-static int expire_reflog(const char *refname, const unsigned char *sha1,
-unsigned int flags, void *policy_cb_data)
-{
-   struct expire_reflog_cb cb;
-   struct ref_lock *lock;
-   char *log_file;
-   int status = 0;
-
-   memset(cb, 0, sizeof(cb));
-   cb.flags = flags;
-   cb.policy_cb = policy_cb_data;
-
-   /*
-* we take the lock for the ref itself to prevent it from
-* getting updated.
-*/
-   lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
-   if (!lock)
-   return error(cannot lock ref '%s', refname);
-   if (!reflog_exists(refname)) {
-   unlock_ref(lock);
-   return 0;
-   }
-
-   log_file = git_pathdup(logs/%s, refname);
-   if (!(flags  EXPIRE_REFLOGS_DRY_RUN)) {
-   if (hold_lock_file_for_update(reflog_lock, log_file, 0)  0)
-   goto failure;
-   cb.newlog = fdopen_lock_file(reflog_lock, w);
-   if (!cb.newlog)
-   goto failure;
-   }
-
-   reflog_expiry_prepare(refname, sha1, cb.policy_cb);
-   for_each_reflog_ent(refname, expire_reflog_ent, cb);
-   reflog_expiry_cleanup(cb.policy_cb);
-
-   if (!(flags  EXPIRE_REFLOGS_DRY_RUN)) {
-   if (close_lock_file(reflog_lock)) {
-   status |= error(Couldn't write %s: %s, log_file,
-   strerror(errno));
-   } else if ((flags  EXPIRE_REFLOGS_UPDATE_REF) 
-   (write_in_full(lock-lock_fd,
-   sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-write_str_in_full(lock-lock_fd, \n) != 1 ||
-close_ref(lock)  0)) {
-   status |= 

[PATCH 22/23] lock_any_ref_for_update(): inline function

[Michael Haggerty]

From: Ronnie Sahlberg sahlb...@google.com

Inline the function at its one remaining caller (which is within
refs.c) and remove it.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 refs.c | 9 +
 refs.h | 9 +
 2 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/refs.c b/refs.c
index c6ee775..f3564cf 100644
--- a/refs.c
+++ b/refs.c
@@ -2346,13 +2346,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
*refname,
return NULL;
 }
 
-struct ref_lock *lock_any_ref_for_update(const char *refname,
-const unsigned char *old_sha1,
-int flags, int *type_p)
-{
-   return lock_ref_sha1_basic(refname, old_sha1, NULL, flags, type_p);
-}
-
 /*
  * Write an entry to the packed-refs file for the specified refname.
  * If peeled is non-NULL, write it as the entry's peeled value.
@@ -4007,7 +4000,7 @@ extern int reflog_expire(const char *refname, const 
unsigned char *sha1,
 * we take the lock for the ref itself to prevent it from
 * getting updated.
 */
-   lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
+   lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, NULL);
if (!lock)
return error(cannot lock ref '%s', refname);
if (!reflog_exists(refname)) {
diff --git a/refs.h b/refs.h
index 82611b5..174863d 100644
--- a/refs.h
+++ b/refs.h
@@ -181,8 +181,7 @@ extern int is_branch(const char *refname);
 extern int peel_ref(const char *refname, unsigned char *sha1);
 
 /*
- * Flags controlling lock_any_ref_for_update(), ref_transaction_update(),
- * ref_transaction_create(), etc.
+ * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *  symbolic references.
  * REF_DELETING: tolerate broken refs
@@ -191,12 +190,6 @@ extern int peel_ref(const char *refname, unsigned char 
*sha1);
  */
 #define REF_NODEREF0x01
 #define REF_DELETING   0x02
-/*
- * This function sets errno to something meaningful on failure.
- */
-extern struct ref_lock *lock_any_ref_for_update(const char *refname,
-   const unsigned char *old_sha1,
-   int flags, int *type_p);
 
 /*
  * Setup reflog before using. Set errno to something meaningful on failure.
-- 
2.1.3

--
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 10/23] expire_reflog(): add a flags argument

[Michael Haggerty]

We want to separate the options relevant to the expiry machinery from
the options affecting the expiration policy. So add a flags argument
to expire_reflog() to hold the former.

The argument doesn't yet do anything.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index ebfa635..319f0d2 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -415,7 +415,8 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb 
*cb)
 
 static struct lock_file reflog_lock;
 
-static int expire_reflog(const char *refname, const unsigned char *sha1, void 
*cb_data)
+static int expire_reflog(const char *refname, const unsigned char *sha1,
+unsigned int flags, void *cb_data)
 {
struct cmd_reflog_expire_cb *cmd = cb_data;
struct expire_reflog_cb cb;
@@ -627,6 +628,7 @@ static int cmd_reflog_expire(int argc, const char **argv, 
const char *prefix)
unsigned long now = time(NULL);
int i, status, do_all;
int explicit_expiry = 0;
+   unsigned int flags = 0;
 
default_reflog_expire_unreachable = now - 30 * 24 * 3600;
default_reflog_expire = now - 90 * 24 * 3600;
@@ -696,7 +698,7 @@ static int cmd_reflog_expire(int argc, const char **argv, 
const char *prefix)
for (i = 0; i  collected.nr; i++) {
struct collected_reflog *e = collected.e[i];
set_reflog_expiry_param(cb, explicit_expiry, 
e-reflog);
-   status |= expire_reflog(e-reflog, e-sha1, cb);
+   status |= expire_reflog(e-reflog, e-sha1, flags, cb);
free(e);
}
free(collected.e);
@@ -710,7 +712,7 @@ static int cmd_reflog_expire(int argc, const char **argv, 
const char *prefix)
continue;
}
set_reflog_expiry_param(cb, explicit_expiry, ref);
-   status |= expire_reflog(ref, sha1, cb);
+   status |= expire_reflog(ref, sha1, flags, cb);
}
return status;
 }
@@ -729,6 +731,7 @@ static int cmd_reflog_delete(int argc, const char **argv, 
const char *prefix)
 {
struct cmd_reflog_expire_cb cb;
int i, status = 0;
+   unsigned int flags = 0;
 
memset(cb, 0, sizeof(cb));
 
@@ -781,7 +784,7 @@ static int cmd_reflog_delete(int argc, const char **argv, 
const char *prefix)
cb.expire_total = 0;
}
 
-   status |= expire_reflog(ref, sha1, cb);
+   status |= expire_reflog(ref, sha1, flags, cb);
free(ref);
}
return status;
-- 
2.1.3

--
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 00/23] Add reflog_expire() to the references API

[Michael Haggerty]

I propose this patch series as an alternative to Ronnie's reflog
transactions series.

Both alternatives have been discussed exhaustively on the list [1,2].
My opinion is that there is no need to allow arbitrary reflog changes
via the ref_transaction API, because just about the only things we
want to do are

* Append an entry to a reflog when a reference is updated. This should
  (and is already) done as a side effect of the reference update.

* Expire or delete old reflog entries based on relatively complicated
  logic, possibly repairing the remaining entries so as to preserve
  the continuity of the reflog chain.

This patch series shows that the latter can be done with a single,
fairly simple, purpose-made function, expire_reflog(), in the
references API. The policy for what reflog entries should be expired
is specified by the caller via three callback functions that don't
have to know anything about how reflogs are stored. The locking,
iteration, repair, and writing is implemented within the references
module, in a function that can easily be swapped out when pluggable
reference backends are implemented.

The remaining reflog operations (enabling/disabling reflogs for a
reference, renaming the reflog when a reference is renamed) are not
especially difficult but will be brought into the same framework in a
future patch series.

The first few patches and the last few are taken from Ronnie's and
Stefan's work. I chose *not* to rename the ref_transaction functions
for obvious reasons. A couple of the later patches from their series
would make sense but are not duplicated here.

This branch is also available on GitHub:

https://github.com/mhagger/git.git, branch reflog-expire-api-v1

Michael

[1] http://thread.gmane.org/gmane.comp.version-control.git/259712/focus=259770
[2] http://thread.gmane.org/gmane.comp.version-control.git/260731/focus=260767

Michael Haggerty (17):
  expire_reflog(): remove unused parameter
  expire_reflog(): rename ref parameter to refname
  expire_reflog(): exit early if the reference has no reflog
  expire_reflog(): use a lock_file for rewriting the reflog file
  Extract function should_expire_reflog_ent()
  expire_reflog(): extract two policy-related functions
  expire_reflog(): add a flags argument
  expire_reflog(): move dry_run to flags argument
  expire_reflog(): move updateref to flags argument
  Rename expire_reflog_cb to expire_reflog_policy_cb
  struct expire_reflog_cb: a new callback data type
  expire_reflog(): pass flags through to expire_reflog_ent()
  expire_reflog(): move verbose to flags argument
  expire_reflog(): move rewrite to flags argument
  Move newlog and last_kept_sha1 to struct expire_reflog_cb
  expire_reflog(): treat the policy callback data as opaque
  reflog_expire(): new function in the reference API

Ronnie Sahlberg (5):
  refs.c: make ref_transaction_create a wrapper for
ref_transaction_update
  refs.c: make ref_transaction_delete a wrapper for
ref_transaction_update
  refs.c: add a function to append a reflog entry to a fd
  refs.c: remove unlock_ref/close_ref/commit_ref from the refs api
  lock_any_ref_for_update(): inline function

Stefan Beller (1):
  refs.c: don't expose the internal struct ref_lock in the header file

 builtin/reflog.c | 259 ++-
 refs.c   | 251 +++--
 refs.h   |  74 ++--
 3 files changed, 319 insertions(+), 265 deletions(-)

-- 
2.1.3

--
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 13/23] Rename expire_reflog_cb to expire_reflog_policy_cb

[Michael Haggerty]

This is the first step towards separating the data needed by the
policy code from the data needed by the reflog expiration machinery.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 597c547..3538e4b 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -30,7 +30,7 @@ struct cmd_reflog_expire_cb {
int recno;
 };
 
-struct expire_reflog_cb {
+struct expire_reflog_policy_cb {
FILE *newlog;
enum {
UE_NORMAL,
@@ -220,7 +220,7 @@ static int keep_entry(struct commit **it, unsigned char 
*sha1)
  * the expire_limit and queue them back, so that the caller can call
  * us again to restart the traversal with longer expire_limit.
  */
-static void mark_reachable(struct expire_reflog_cb *cb)
+static void mark_reachable(struct expire_reflog_policy_cb *cb)
 {
struct commit *commit;
struct commit_list *pending;
@@ -259,7 +259,7 @@ static void mark_reachable(struct expire_reflog_cb *cb)
cb-mark_list = leftover;
 }
 
-static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, 
unsigned char *sha1)
+static int unreachable(struct expire_reflog_policy_cb *cb, struct commit 
*commit, unsigned char *sha1)
 {
/*
 * We may or may not have the commit yet - if not, look it
@@ -295,7 +295,7 @@ static int should_expire_reflog_ent(unsigned char *osha1, 
unsigned char *nsha1,
const char *email, unsigned long timestamp, 
int tz,
const char *message, void *cb_data)
 {
-   struct expire_reflog_cb *cb = cb_data;
+   struct expire_reflog_policy_cb *cb = cb_data;
struct commit *old, *new;
 
if (timestamp  cb-cmd-expire_total)
@@ -323,7 +323,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned 
char *nsha1,
const char *email, unsigned long timestamp, int tz,
const char *message, void *cb_data)
 {
-   struct expire_reflog_cb *cb = cb_data;
+   struct expire_reflog_policy_cb *cb = cb_data;
 
if (cb-cmd-rewrite)
osha1 = cb-last_kept_sha1;
@@ -350,7 +350,8 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned 
char *nsha1,
return 0;
 }
 
-static int push_tip_to_list(const char *refname, const unsigned char *sha1, 
int flags, void *cb_data)
+static int push_tip_to_list(const char *refname, const unsigned char *sha1,
+   int flags, void *cb_data)
 {
struct commit_list **list = cb_data;
struct commit *tip_commit;
@@ -365,7 +366,7 @@ static int push_tip_to_list(const char *refname, const 
unsigned char *sha1, int
 
 static void reflog_expiry_prepare(const char *refname,
  const unsigned char *sha1,
- struct expire_reflog_cb *cb)
+ struct expire_reflog_policy_cb *cb)
 {
if (!cb-cmd-expire_unreachable || !strcmp(refname, HEAD)) {
cb-tip_commit = NULL;
@@ -397,7 +398,7 @@ static void reflog_expiry_prepare(const char *refname,
}
 }
 
-static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
+static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
 {
if (cb-unreachable_expire_kind != UE_ALWAYS) {
if (cb-unreachable_expire_kind == UE_HEAD) {
@@ -422,7 +423,7 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1,
 unsigned int flags, void *cb_data)
 {
struct cmd_reflog_expire_cb *cmd = cb_data;
-   struct expire_reflog_cb cb;
+   struct expire_reflog_policy_cb cb;
struct ref_lock *lock;
char *log_file;
int status = 0;
-- 
2.1.3

--
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 09/23] expire_reflog(): extract two policy-related functions

[Michael Haggerty]

Extract two functions, reflog_expiry_prepare() and
reflog_expiry_cleanup(), from expire_reflog(). This is a further step
towards separating the code for deciding on expiration policy from the
code that manages the physical expiration.

This change requires a couple of local variables from expire_reflog()
to be turned into fields of struct expire_reflog_cb. More
reorganization of the callback data will follow in later commits.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
In fact, the work done in reflog_expire_cleanup() doesn't really need
to be done via a callback, because it doesn't need to be done while
the reference lock is held. But the symmetry between prepare and
cleanup is kindof nice. Perhaps some future policy decision will want
to do some final work under the reference lock?

But it would be easy to get rid of this third callback function and
have the callers do the work themselves after calling expire_reflog().
I don't have a string feeling either way.

 builtin/reflog.c | 94 +++-
 1 file changed, 52 insertions(+), 42 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 7bc6e0f..ebfa635 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -43,6 +43,8 @@ struct expire_reflog_cb {
unsigned long mark_limit;
struct cmd_reflog_expire_cb *cmd;
unsigned char last_kept_sha1[20];
+   struct commit *tip_commit;
+   struct commit_list *tips;
 };
 
 struct collected_reflog {
@@ -363,6 +365,54 @@ static int push_tip_to_list(const char *refname, const 
unsigned char *sha1, int
return 0;
 }
 
+static void reflog_expiry_prepare(const char *refname,
+ const unsigned char *sha1,
+ struct expire_reflog_cb *cb)
+{
+   if (!cb-cmd-expire_unreachable || !strcmp(refname, HEAD)) {
+   cb-tip_commit = NULL;
+   cb-unreachable_expire_kind = UE_HEAD;
+   } else {
+   cb-tip_commit = lookup_commit_reference_gently(sha1, 1);
+   if (!cb-tip_commit)
+   cb-unreachable_expire_kind = UE_ALWAYS;
+   else
+   cb-unreachable_expire_kind = UE_NORMAL;
+   }
+
+   if (cb-cmd-expire_unreachable = cb-cmd-expire_total)
+   cb-unreachable_expire_kind = UE_ALWAYS;
+
+   cb-mark_list = NULL;
+   cb-tips = NULL;
+   if (cb-unreachable_expire_kind != UE_ALWAYS) {
+   if (cb-unreachable_expire_kind == UE_HEAD) {
+   struct commit_list *elem;
+   for_each_ref(push_tip_to_list, cb-tips);
+   for (elem = cb-tips; elem; elem = elem-next)
+   commit_list_insert(elem-item, cb-mark_list);
+   } else {
+   commit_list_insert(cb-tip_commit, cb-mark_list);
+   }
+   cb-mark_limit = cb-cmd-expire_total;
+   mark_reachable(cb);
+   }
+}
+
+static void reflog_expiry_cleanup(struct expire_reflog_cb *cb)
+{
+   if (cb-unreachable_expire_kind != UE_ALWAYS) {
+   if (cb-unreachable_expire_kind == UE_HEAD) {
+   struct commit_list *elem;
+   for (elem = cb-tips; elem; elem = elem-next)
+   clear_commit_marks(elem-item, REACHABLE);
+   free_commit_list(cb-tips);
+   } else {
+   clear_commit_marks(cb-tip_commit, REACHABLE);
+   }
+   }
+}
+
 static struct lock_file reflog_lock;
 
 static int expire_reflog(const char *refname, const unsigned char *sha1, void 
*cb_data)
@@ -371,8 +421,6 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1, void *c
struct expire_reflog_cb cb;
struct ref_lock *lock;
char *log_file;
-   struct commit *tip_commit;
-   struct commit_list *tips;
int status = 0;
 
memset(cb, 0, sizeof(cb));
@@ -400,47 +448,9 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1, void *c
 
cb.cmd = cmd;
 
-   if (!cmd-expire_unreachable || !strcmp(refname, HEAD)) {
-   tip_commit = NULL;
-   cb.unreachable_expire_kind = UE_HEAD;
-   } else {
-   tip_commit = lookup_commit_reference_gently(sha1, 1);
-   if (!tip_commit)
-   cb.unreachable_expire_kind = UE_ALWAYS;
-   else
-   cb.unreachable_expire_kind = UE_NORMAL;
-   }
-
-   if (cmd-expire_unreachable = cmd-expire_total)
-   cb.unreachable_expire_kind = UE_ALWAYS;
-
-   cb.mark_list = NULL;
-   tips = NULL;
-   if (cb.unreachable_expire_kind != UE_ALWAYS) {
-   if (cb.unreachable_expire_kind == UE_HEAD) {
-   struct commit_list *elem;
-   for_each_ref(push_tip_to_list, 

[PATCH 04/23] expire_reflog(): remove unused parameter

[Michael Haggerty]

It was called unused, so at least it was self-consistent.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 2d85d26..3e11bee 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -349,7 +349,7 @@ static int push_tip_to_list(const char *refname, const 
unsigned char *sha1, int
return 0;
 }
 
-static int expire_reflog(const char *ref, const unsigned char *sha1, int 
unused, void *cb_data)
+static int expire_reflog(const char *ref, const unsigned char *sha1, void 
*cb_data)
 {
struct cmd_reflog_expire_cb *cmd = cb_data;
struct expire_reflog_cb cb;
@@ -663,7 +663,7 @@ static int cmd_reflog_expire(int argc, const char **argv, 
const char *prefix)
for (i = 0; i  collected.nr; i++) {
struct collected_reflog *e = collected.e[i];
set_reflog_expiry_param(cb, explicit_expiry, 
e-reflog);
-   status |= expire_reflog(e-reflog, e-sha1, 0, cb);
+   status |= expire_reflog(e-reflog, e-sha1, cb);
free(e);
}
free(collected.e);
@@ -677,7 +677,7 @@ static int cmd_reflog_expire(int argc, const char **argv, 
const char *prefix)
continue;
}
set_reflog_expiry_param(cb, explicit_expiry, ref);
-   status |= expire_reflog(ref, sha1, 0, cb);
+   status |= expire_reflog(ref, sha1, cb);
}
return status;
 }
@@ -748,7 +748,7 @@ static int cmd_reflog_delete(int argc, const char **argv, 
const char *prefix)
cb.expire_total = 0;
}
 
-   status |= expire_reflog(ref, sha1, 0, cb);
+   status |= expire_reflog(ref, sha1, cb);
free(ref);
}
return status;
-- 
2.1.3

--
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 19/23] expire_reflog(): treat the policy callback data as opaque

[Michael Haggerty]

Now that expire_reflog() doesn't actually look in the
expire_reflog_policy_cb data structure, we can make it opaque:

* Change its callers to pass it a pointer to an entire struct
  expire_reflog_policy_cb.

* Change it to pass the pointer through as a void *.

* Change the policy functions, reflog_expiry_prepare(),
  reflog_expiry_cleanup(), and should_expire_reflog_ent(), to accept
  void *cb_data arguments and cast them to struct
  expire_reflog_policy_cb internally.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 73 
 1 file changed, 36 insertions(+), 37 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 01b76d0..c30936bb 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -43,7 +43,7 @@ struct expire_reflog_policy_cb {
} unreachable_expire_kind;
struct commit_list *mark_list;
unsigned long mark_limit;
-   struct cmd_reflog_expire_cb *cmd;
+   struct cmd_reflog_expire_cb cmd;
struct commit *tip_commit;
struct commit_list *tips;
 };
@@ -309,22 +309,22 @@ static int should_expire_reflog_ent(unsigned char *osha1, 
unsigned char *nsha1,
struct expire_reflog_policy_cb *cb = cb_data;
struct commit *old, *new;
 
-   if (timestamp  cb-cmd-expire_total)
+   if (timestamp  cb-cmd.expire_total)
return 1;
 
old = new = NULL;
-   if (cb-cmd-stalefix 
+   if (cb-cmd.stalefix 
(!keep_entry(old, osha1) || !keep_entry(new, nsha1)))
return 1;
 
-   if (timestamp  cb-cmd-expire_unreachable) {
+   if (timestamp  cb-cmd.expire_unreachable) {
if (cb-unreachable_expire_kind == UE_ALWAYS)
return 1;
if (unreachable(cb, old, osha1) || unreachable(cb, new, nsha1))
return 1;
}
 
-   if (cb-cmd-recno  --(cb-cmd-recno) == 0)
+   if (cb-cmd.recno  --(cb-cmd.recno) == 0)
return 1;
 
return 0;
@@ -378,9 +378,11 @@ static int push_tip_to_list(const char *refname, const 
unsigned char *sha1,
 
 static void reflog_expiry_prepare(const char *refname,
  const unsigned char *sha1,
- struct expire_reflog_policy_cb *cb)
+ void *cb_data)
 {
-   if (!cb-cmd-expire_unreachable || !strcmp(refname, HEAD)) {
+   struct expire_reflog_policy_cb *cb = cb_data;
+
+   if (!cb-cmd.expire_unreachable || !strcmp(refname, HEAD)) {
cb-tip_commit = NULL;
cb-unreachable_expire_kind = UE_HEAD;
} else {
@@ -391,7 +393,7 @@ static void reflog_expiry_prepare(const char *refname,
cb-unreachable_expire_kind = UE_NORMAL;
}
 
-   if (cb-cmd-expire_unreachable = cb-cmd-expire_total)
+   if (cb-cmd.expire_unreachable = cb-cmd.expire_total)
cb-unreachable_expire_kind = UE_ALWAYS;
 
cb-mark_list = NULL;
@@ -405,13 +407,15 @@ static void reflog_expiry_prepare(const char *refname,
} else {
commit_list_insert(cb-tip_commit, cb-mark_list);
}
-   cb-mark_limit = cb-cmd-expire_total;
+   cb-mark_limit = cb-cmd.expire_total;
mark_reachable(cb);
}
 }
 
-static void reflog_expiry_cleanup(struct expire_reflog_policy_cb *cb)
+static void reflog_expiry_cleanup(void *cb_data)
 {
+   struct expire_reflog_policy_cb *cb = cb_data;
+
if (cb-unreachable_expire_kind != UE_ALWAYS) {
if (cb-unreachable_expire_kind == UE_HEAD) {
struct commit_list *elem;
@@ -427,19 +431,16 @@ static void reflog_expiry_cleanup(struct 
expire_reflog_policy_cb *cb)
 static struct lock_file reflog_lock;
 
 static int expire_reflog(const char *refname, const unsigned char *sha1,
-unsigned int flags, void *cb_data)
+unsigned int flags, void *policy_cb_data)
 {
-   struct cmd_reflog_expire_cb *cmd = cb_data;
struct expire_reflog_cb cb;
-   struct expire_reflog_policy_cb policy_cb;
struct ref_lock *lock;
char *log_file;
int status = 0;
 
memset(cb, 0, sizeof(cb));
-   memset(policy_cb, 0, sizeof(policy_cb));
cb.flags = flags;
-   cb.policy_cb = policy_cb;
+   cb.policy_cb = policy_cb_data;
 
/*
 * we take the lock for the ref itself to prevent it from
@@ -462,11 +463,9 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1,
goto failure;
}
 
-   policy_cb.cmd = cmd;
-
-   reflog_expiry_prepare(refname, sha1, policy_cb);
+   reflog_expiry_prepare(refname, sha1, cb.policy_cb);
for_each_reflog_ent(refname, expire_reflog_ent, cb);
-   reflog_expiry_cleanup(policy_cb);
+   

[PATCH 17/23] expire_reflog(): move rewrite to flags argument

[Michael Haggerty]

The policy objects don't care about --rewrite. So move it to
expire_reflog()'s flags parameter.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index cc7a220..6294406 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -23,13 +23,13 @@ static unsigned long default_reflog_expire_unreachable;
 enum expire_reflog_flags {
EXPIRE_REFLOGS_DRY_RUN = 1  0,
EXPIRE_REFLOGS_UPDATE_REF = 1  1,
-   EXPIRE_REFLOGS_VERBOSE = 1  2
+   EXPIRE_REFLOGS_VERBOSE = 1  2,
+   EXPIRE_REFLOGS_REWRITE = 1  3
 };
 
 struct cmd_reflog_expire_cb {
struct rev_info revs;
int stalefix;
-   int rewrite;
unsigned long expire_total;
unsigned long expire_unreachable;
int recno;
@@ -337,7 +337,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned 
char *nsha1,
struct expire_reflog_cb *cb = cb_data;
struct expire_reflog_policy_cb *policy_cb = cb-policy_cb;
 
-   if (policy_cb-cmd-rewrite)
+   if (cb-flags  EXPIRE_REFLOGS_REWRITE)
osha1 = policy_cb-last_kept_sha1;
 
if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
@@ -673,7 +673,7 @@ static int cmd_reflog_expire(int argc, const char **argv, 
const char *prefix)
else if (!strcmp(arg, --stale-fix))
cb.stalefix = 1;
else if (!strcmp(arg, --rewrite))
-   cb.rewrite = 1;
+   flags |= EXPIRE_REFLOGS_REWRITE;
else if (!strcmp(arg, --updateref))
flags |= EXPIRE_REFLOGS_UPDATE_REF;
else if (!strcmp(arg, --all))
@@ -755,7 +755,7 @@ static int cmd_reflog_delete(int argc, const char **argv, 
const char *prefix)
if (!strcmp(arg, --dry-run) || !strcmp(arg, -n))
flags |= EXPIRE_REFLOGS_DRY_RUN;
else if (!strcmp(arg, --rewrite))
-   cb.rewrite = 1;
+   flags |= EXPIRE_REFLOGS_REWRITE;
else if (!strcmp(arg, --updateref))
flags |= EXPIRE_REFLOGS_UPDATE_REF;
else if (!strcmp(arg, --verbose))
-- 
2.1.3

--
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 02/23] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update

[Michael Haggerty]

From: Ronnie Sahlberg sahlb...@google.com

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 refs.c | 22 ++
 refs.h |  2 +-
 2 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/refs.c b/refs.c
index 005eb18..05cb299 100644
--- a/refs.c
+++ b/refs.c
@@ -3633,26 +3633,8 @@ int ref_transaction_delete(struct ref_transaction 
*transaction,
   int flags, int have_old, const char *msg,
   struct strbuf *err)
 {
-   struct ref_update *update;
-
-   assert(err);
-
-   if (transaction-state != REF_TRANSACTION_OPEN)
-   die(BUG: delete called for transaction that is not open);
-
-   if (have_old  !old_sha1)
-   die(BUG: have_old is true but old_sha1 is NULL);
-
-   update = add_update(transaction, refname);
-   update-flags = flags;
-   update-have_old = have_old;
-   if (have_old) {
-   assert(!is_null_sha1(old_sha1));
-   hashcpy(update-old_sha1, old_sha1);
-   }
-   if (msg)
-   update-msg = xstrdup(msg);
-   return 0;
+   return ref_transaction_update(transaction, refname, null_sha1,
+ old_sha1, flags, have_old, msg, err);
 }
 
 int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index 2bc3556..7d675b7 100644
--- a/refs.h
+++ b/refs.h
@@ -283,7 +283,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf 
*err);
 
 /*
  * Add a reference update to transaction.  new_sha1 is the value that
- * the reference should have after the update, or zeros if it should
+ * the reference should have after the update, or null_sha1 if it should
  * be deleted.  If have_old is true, then old_sha1 holds the value
  * that the reference should have had before the update, or zeros if
  * it must not have existed beforehand.
-- 
2.1.3

--
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 08/23] Extract function should_expire_reflog_ent()

[Michael Haggerty]

Extracted from expire_reflog_ent() a function that is solely
responsible for deciding whether a reflog entry should be expired. By
separating this business logic from the mechanics of actually
expiring entries, we are working towards the goal of encapsulating
reflog expiry within the refs API, with policy decided by a callback
function passed to it by its caller.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 70 +---
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index d344d45..7bc6e0f 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -288,51 +288,65 @@ static int unreachable(struct expire_reflog_cb *cb, 
struct commit *commit, unsig
return !(commit-object.flags  REACHABLE);
 }
 
-static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
-   const char *email, unsigned long timestamp, int tz,
-   const char *message, void *cb_data)
+/*
+ * Return true iff the specified reflog entry should be expired.
+ */
+static int should_expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+   const char *email, unsigned long timestamp, 
int tz,
+   const char *message, void *cb_data)
 {
struct expire_reflog_cb *cb = cb_data;
struct commit *old, *new;
 
if (timestamp  cb-cmd-expire_total)
-   goto prune;
-
-   if (cb-cmd-rewrite)
-   osha1 = cb-last_kept_sha1;
+   return 1;
 
old = new = NULL;
if (cb-cmd-stalefix 
(!keep_entry(old, osha1) || !keep_entry(new, nsha1)))
-   goto prune;
+   return 1;
 
if (timestamp  cb-cmd-expire_unreachable) {
if (cb-unreachable_expire_kind == UE_ALWAYS)
-   goto prune;
+   return 1;
if (unreachable(cb, old, osha1) || unreachable(cb, new, nsha1))
-   goto prune;
+   return 1;
}
 
if (cb-cmd-recno  --(cb-cmd-recno) == 0)
-   goto prune;
-
-   if (cb-newlog) {
-   char sign = (tz  0) ? '-' : '+';
-   int zone = (tz  0) ? (-tz) : tz;
-   fprintf(cb-newlog, %s %s %s %lu %c%04d\t%s,
-   sha1_to_hex(osha1), sha1_to_hex(nsha1),
-   email, timestamp, sign, zone,
-   message);
-   hashcpy(cb-last_kept_sha1, nsha1);
-   }
-   if (cb-cmd-verbose)
-   printf(keep %s, message);
+   return 1;
+
return 0;
- prune:
-   if (!cb-newlog)
-   printf(would prune %s, message);
-   else if (cb-cmd-verbose)
-   printf(prune %s, message);
+}
+
+static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+   const char *email, unsigned long timestamp, int tz,
+   const char *message, void *cb_data)
+{
+   struct expire_reflog_cb *cb = cb_data;
+
+   if (cb-cmd-rewrite)
+   osha1 = cb-last_kept_sha1;
+
+   if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
+message, cb_data)) {
+   if (!cb-newlog)
+   printf(would prune %s, message);
+   else if (cb-cmd-verbose)
+   printf(prune %s, message);
+   } else {
+   if (cb-newlog) {
+   char sign = (tz  0) ? '-' : '+';
+   int zone = (tz  0) ? (-tz) : tz;
+   fprintf(cb-newlog, %s %s %s %lu %c%04d\t%s,
+   sha1_to_hex(osha1), sha1_to_hex(nsha1),
+   email, timestamp, sign, zone,
+   message);
+   hashcpy(cb-last_kept_sha1, nsha1);
+   }
+   if (cb-cmd-verbose)
+   printf(keep %s, message);
+   }
return 0;
 }
 
-- 
2.1.3

--
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 03/23] refs.c: add a function to append a reflog entry to a fd

[Michael Haggerty]

From: Ronnie Sahlberg sahlb...@google.com

Break out the code to create the string and writing it to the file
descriptor from log_ref_write and add it into a dedicated function
log_ref_write_fd. For now this is only used from log_ref_write,
but later on we will call this function from reflog transactions too,
which means that we will end up with only a single place,
where we write a reflog entry to a file instead of the current two
places (log_ref_write and builtin/reflog.c).

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 refs.c | 48 ++--
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/refs.c b/refs.c
index 05cb299..150c980 100644
--- a/refs.c
+++ b/refs.c
@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, 
int bufsize)
return 0;
 }
 
+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
+   const unsigned char *new_sha1,
+   const char *committer, const char *msg)
+{
+   int msglen, written;
+   unsigned maxlen, len;
+   char *logrec;
+
+   msglen = msg ? strlen(msg) : 0;
+   maxlen = strlen(committer) + msglen + 100;
+   logrec = xmalloc(maxlen);
+   len = sprintf(logrec, %s %s %s\n,
+ sha1_to_hex(old_sha1),
+ sha1_to_hex(new_sha1),
+ committer);
+   if (msglen)
+   len += copy_msg(logrec + len - 1, msg) - 1;
+
+   written = len = maxlen ? write_in_full(fd, logrec, len) : -1;
+   free(logrec);
+   if (written != len)
+   return -1;
+
+   return 0;
+}
+
 static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 const unsigned char *new_sha1, const char *msg)
 {
-   int logfd, result, written, oflags = O_APPEND | O_WRONLY;
-   unsigned maxlen, len;
-   int msglen;
+   int logfd, result, oflags = O_APPEND | O_WRONLY;
char log_file[PATH_MAX];
-   char *logrec;
-   const char *committer;
 
if (log_all_ref_updates  0)
log_all_ref_updates = !is_bare_repository();
@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const 
unsigned char *old_sha1,
logfd = open(log_file, oflags);
if (logfd  0)
return 0;
-   msglen = msg ? strlen(msg) : 0;
-   committer = git_committer_info(0);
-   maxlen = strlen(committer) + msglen + 100;
-   logrec = xmalloc(maxlen);
-   len = sprintf(logrec, %s %s %s\n,
- sha1_to_hex(old_sha1),
- sha1_to_hex(new_sha1),
- committer);
-   if (msglen)
-   len += copy_msg(logrec + len - 1, msg) - 1;
-   written = len = maxlen ? write_in_full(logfd, logrec, len) : -1;
-   free(logrec);
-   if (written != len) {
+   result = log_ref_write_fd(logfd, old_sha1, new_sha1,
+ git_committer_info(0), msg);
+   if (result) {
int save_errno = errno;
close(logfd);
error(Unable to append to %s, log_file);
-- 
2.1.3

--
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 16/23] expire_reflog(): move verbose to flags argument

[Michael Haggerty]

The policy objects don't care about --verbose. So move it to
expire_reflog()'s flags parameter.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 1512b67..cc7a220 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -20,11 +20,16 @@ static const char reflog_delete_usage[] =
 static unsigned long default_reflog_expire;
 static unsigned long default_reflog_expire_unreachable;
 
+enum expire_reflog_flags {
+   EXPIRE_REFLOGS_DRY_RUN = 1  0,
+   EXPIRE_REFLOGS_UPDATE_REF = 1  1,
+   EXPIRE_REFLOGS_VERBOSE = 1  2
+};
+
 struct cmd_reflog_expire_cb {
struct rev_info revs;
int stalefix;
int rewrite;
-   int verbose;
unsigned long expire_total;
unsigned long expire_unreachable;
int recno;
@@ -339,7 +344,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned 
char *nsha1,
 message, policy_cb)) {
if (!policy_cb-newlog)
printf(would prune %s, message);
-   else if (policy_cb-cmd-verbose)
+   else if (cb-flags  EXPIRE_REFLOGS_VERBOSE)
printf(prune %s, message);
} else {
if (policy_cb-newlog) {
@@ -351,7 +356,7 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned 
char *nsha1,
message);
hashcpy(policy_cb-last_kept_sha1, nsha1);
}
-   if (policy_cb-cmd-verbose)
+   if (cb-flags  EXPIRE_REFLOGS_VERBOSE)
printf(keep %s, message);
}
return 0;
@@ -421,11 +426,6 @@ static void reflog_expiry_cleanup(struct 
expire_reflog_policy_cb *cb)
 
 static struct lock_file reflog_lock;
 
-enum expire_reflog_flags {
-   EXPIRE_REFLOGS_DRY_RUN = 1  0,
-   EXPIRE_REFLOGS_UPDATE_REF = 1  1
-};
-
 static int expire_reflog(const char *refname, const unsigned char *sha1,
 unsigned int flags, void *cb_data)
 {
@@ -679,7 +679,7 @@ static int cmd_reflog_expire(int argc, const char **argv, 
const char *prefix)
else if (!strcmp(arg, --all))
do_all = 1;
else if (!strcmp(arg, --verbose))
-   cb.verbose = 1;
+   flags |= EXPIRE_REFLOGS_VERBOSE;
else if (!strcmp(arg, --)) {
i++;
break;
@@ -697,10 +697,10 @@ static int cmd_reflog_expire(int argc, const char **argv, 
const char *prefix)
 */
if (cb.stalefix) {
init_revisions(cb.revs, prefix);
-   if (cb.verbose)
+   if (flags  EXPIRE_REFLOGS_VERBOSE)
printf(Marking reachable objects...);
mark_reachable_objects(cb.revs, 0, 0, NULL);
-   if (cb.verbose)
+   if (flags  EXPIRE_REFLOGS_VERBOSE)
putchar('\n');
}
 
@@ -759,7 +759,7 @@ static int cmd_reflog_delete(int argc, const char **argv, 
const char *prefix)
else if (!strcmp(arg, --updateref))
flags |= EXPIRE_REFLOGS_UPDATE_REF;
else if (!strcmp(arg, --verbose))
-   cb.verbose = 1;
+   flags |= EXPIRE_REFLOGS_VERBOSE;
else if (!strcmp(arg, --)) {
i++;
break;
-- 
2.1.3

--
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 14/23] struct expire_reflog_cb: a new callback data type

[Michael Haggerty]

Add a new data type, struct expire_reflog_cb, for holding the data
that expire_reflog() passes to expire_reflog_ent() via
for_each_reflog_ent(). For now it only holds a pointer to struct
expire_reflog_policy_cb. In future commits we will move some data
from the latter to the former.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 43 ++-
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3538e4b..5dfa53a 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -45,10 +45,15 @@ struct expire_reflog_policy_cb {
struct commit_list *tips;
 };
 
+struct expire_reflog_cb {
+   void *policy_cb;
+};
+
 struct collected_reflog {
unsigned char sha1[20];
char reflog[FLEX_ARRAY];
 };
+
 struct collect_reflog_cb {
struct collected_reflog **e;
int alloc;
@@ -323,28 +328,29 @@ static int expire_reflog_ent(unsigned char *osha1, 
unsigned char *nsha1,
const char *email, unsigned long timestamp, int tz,
const char *message, void *cb_data)
 {
-   struct expire_reflog_policy_cb *cb = cb_data;
+   struct expire_reflog_cb *cb = cb_data;
+   struct expire_reflog_policy_cb *policy_cb = cb-policy_cb;
 
-   if (cb-cmd-rewrite)
-   osha1 = cb-last_kept_sha1;
+   if (policy_cb-cmd-rewrite)
+   osha1 = policy_cb-last_kept_sha1;
 
if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
-message, cb_data)) {
-   if (!cb-newlog)
+message, policy_cb)) {
+   if (!policy_cb-newlog)
printf(would prune %s, message);
-   else if (cb-cmd-verbose)
+   else if (policy_cb-cmd-verbose)
printf(prune %s, message);
} else {
-   if (cb-newlog) {
+   if (policy_cb-newlog) {
char sign = (tz  0) ? '-' : '+';
int zone = (tz  0) ? (-tz) : tz;
-   fprintf(cb-newlog, %s %s %s %lu %c%04d\t%s,
+   fprintf(policy_cb-newlog, %s %s %s %lu %c%04d\t%s,
sha1_to_hex(osha1), sha1_to_hex(nsha1),
email, timestamp, sign, zone,
message);
-   hashcpy(cb-last_kept_sha1, nsha1);
+   hashcpy(policy_cb-last_kept_sha1, nsha1);
}
-   if (cb-cmd-verbose)
+   if (policy_cb-cmd-verbose)
printf(keep %s, message);
}
return 0;
@@ -423,12 +429,15 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1,
 unsigned int flags, void *cb_data)
 {
struct cmd_reflog_expire_cb *cmd = cb_data;
-   struct expire_reflog_policy_cb cb;
+   struct expire_reflog_cb cb;
+   struct expire_reflog_policy_cb policy_cb;
struct ref_lock *lock;
char *log_file;
int status = 0;
 
memset(cb, 0, sizeof(cb));
+   memset(policy_cb, 0, sizeof(policy_cb));
+   cb.policy_cb = policy_cb;
 
/*
 * we take the lock for the ref itself to prevent it from
@@ -446,16 +455,16 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1,
if (!(flags  EXPIRE_REFLOGS_DRY_RUN)) {
if (hold_lock_file_for_update(reflog_lock, log_file, 0)  0)
goto failure;
-   cb.newlog = fdopen_lock_file(reflog_lock, w);
-   if (!cb.newlog)
+   policy_cb.newlog = fdopen_lock_file(reflog_lock, w);
+   if (!policy_cb.newlog)
goto failure;
}
 
-   cb.cmd = cmd;
+   policy_cb.cmd = cmd;
 
-   reflog_expiry_prepare(refname, sha1, cb);
+   reflog_expiry_prepare(refname, sha1, policy_cb);
for_each_reflog_ent(refname, expire_reflog_ent, cb);
-   reflog_expiry_cleanup(cb);
+   reflog_expiry_cleanup(policy_cb);
 
if (!(flags  EXPIRE_REFLOGS_DRY_RUN)) {
if (close_lock_file(reflog_lock)) {
@@ -463,7 +472,7 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1,
strerror(errno));
} else if ((flags  EXPIRE_REFLOGS_UPDATE_REF) 
(write_in_full(lock-lock_fd,
-   sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
+   sha1_to_hex(policy_cb.last_kept_sha1), 40) != 
40 ||
 write_str_in_full(lock-lock_fd, \n) != 1 ||
 close_ref(lock)  0)) {
status |= error(Couldn't write %s,
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to 

[PATCH 23/23] refs.c: don't expose the internal struct ref_lock in the header file

[Michael Haggerty]

From: Stefan Beller sbel...@google.com

Now the struct ref_lock is used completely internally, so let's
remove it from the header file.

Signed-off-by: Stefan Beller sbel...@google.com
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 refs.c | 9 +
 refs.h | 9 -
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index f3564cf..40c5591 100644
--- a/refs.c
+++ b/refs.c
@@ -6,6 +6,15 @@
 #include dir.h
 #include string-list.h
 
+struct ref_lock {
+   char *ref_name;
+   char *orig_ref_name;
+   struct lock_file *lk;
+   unsigned char old_sha1[20];
+   int lock_fd;
+   int force_write;
+};
+
 /*
  * How to handle various characters in refnames:
  * 0: An acceptable character for refs
diff --git a/refs.h b/refs.h
index 174863d..2957641 100644
--- a/refs.h
+++ b/refs.h
@@ -1,15 +1,6 @@
 #ifndef REFS_H
 #define REFS_H
 
-struct ref_lock {
-   char *ref_name;
-   char *orig_ref_name;
-   struct lock_file *lk;
-   unsigned char old_sha1[20];
-   int lock_fd;
-   int force_write;
-};
-
 /*
  * A ref_transaction represents a collection of ref updates
  * that should succeed or fail together.
-- 
2.1.3

--
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 15/23] expire_reflog(): pass flags through to expire_reflog_ent()

[Michael Haggerty]

Add a flags field to struct expire_reflog_cb, and pass the flags
argument through to expire_reflog_ent(). In a moment we will start
using it to pass through flags that expire_reflog_ent() needs.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 5dfa53a..1512b67 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -46,6 +46,7 @@ struct expire_reflog_policy_cb {
 };
 
 struct expire_reflog_cb {
+   unsigned int flags;
void *policy_cb;
 };
 
@@ -437,6 +438,7 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1,
 
memset(cb, 0, sizeof(cb));
memset(policy_cb, 0, sizeof(policy_cb));
+   cb.flags = flags;
cb.policy_cb = policy_cb;
 
/*
-- 
2.1.3

--
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 06/23] expire_reflog(): exit early if the reference has no reflog

[Michael Haggerty]

There is very little cleanup needed if the reference has no reflog. If
we move the initialization of log_file down a bit, there's even less.
So instead of jumping to the cleanup code at the end of the function,
just do the cleanup and return inline.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index df302f4..a282e60 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -368,9 +368,11 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1, void *c
lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
if (!lock)
return error(cannot lock ref '%s', refname);
+   if (!reflog_exists(refname)) {
+   unlock_ref(lock);
+   return 0;
+   }
log_file = git_pathdup(logs/%s, refname);
-   if (!reflog_exists(refname))
-   goto finish;
if (!cmd-dry_run) {
newlog_path = git_pathdup(logs/%s.lock, refname);
cb.newlog = fopen(newlog_path, w);
@@ -419,7 +421,7 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1, void *c
clear_commit_marks(tip_commit, REACHABLE);
}
}
- finish:
+
if (cb.newlog) {
if (fclose(cb.newlog)) {
status |= error(%s: %s, strerror(errno),
-- 
2.1.3

--
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 18/23] Move newlog and last_kept_sha1 to struct expire_reflog_cb

[Michael Haggerty]

These members are not needed by the policy functions.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 6294406..01b76d0 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -36,7 +36,6 @@ struct cmd_reflog_expire_cb {
 };
 
 struct expire_reflog_policy_cb {
-   FILE *newlog;
enum {
UE_NORMAL,
UE_ALWAYS,
@@ -45,7 +44,6 @@ struct expire_reflog_policy_cb {
struct commit_list *mark_list;
unsigned long mark_limit;
struct cmd_reflog_expire_cb *cmd;
-   unsigned char last_kept_sha1[20];
struct commit *tip_commit;
struct commit_list *tips;
 };
@@ -53,6 +51,8 @@ struct expire_reflog_policy_cb {
 struct expire_reflog_cb {
unsigned int flags;
void *policy_cb;
+   FILE *newlog;
+   unsigned char last_kept_sha1[20];
 };
 
 struct collected_reflog {
@@ -338,23 +338,23 @@ static int expire_reflog_ent(unsigned char *osha1, 
unsigned char *nsha1,
struct expire_reflog_policy_cb *policy_cb = cb-policy_cb;
 
if (cb-flags  EXPIRE_REFLOGS_REWRITE)
-   osha1 = policy_cb-last_kept_sha1;
+   osha1 = cb-last_kept_sha1;
 
if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
 message, policy_cb)) {
-   if (!policy_cb-newlog)
+   if (!cb-newlog)
printf(would prune %s, message);
else if (cb-flags  EXPIRE_REFLOGS_VERBOSE)
printf(prune %s, message);
} else {
-   if (policy_cb-newlog) {
+   if (cb-newlog) {
char sign = (tz  0) ? '-' : '+';
int zone = (tz  0) ? (-tz) : tz;
-   fprintf(policy_cb-newlog, %s %s %s %lu %c%04d\t%s,
+   fprintf(cb-newlog, %s %s %s %lu %c%04d\t%s,
sha1_to_hex(osha1), sha1_to_hex(nsha1),
email, timestamp, sign, zone,
message);
-   hashcpy(policy_cb-last_kept_sha1, nsha1);
+   hashcpy(cb-last_kept_sha1, nsha1);
}
if (cb-flags  EXPIRE_REFLOGS_VERBOSE)
printf(keep %s, message);
@@ -457,8 +457,8 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1,
if (!(flags  EXPIRE_REFLOGS_DRY_RUN)) {
if (hold_lock_file_for_update(reflog_lock, log_file, 0)  0)
goto failure;
-   policy_cb.newlog = fdopen_lock_file(reflog_lock, w);
-   if (!policy_cb.newlog)
+   cb.newlog = fdopen_lock_file(reflog_lock, w);
+   if (!cb.newlog)
goto failure;
}
 
@@ -474,7 +474,7 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1,
strerror(errno));
} else if ((flags  EXPIRE_REFLOGS_UPDATE_REF) 
(write_in_full(lock-lock_fd,
-   sha1_to_hex(policy_cb.last_kept_sha1), 40) != 
40 ||
+   sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
 write_str_in_full(lock-lock_fd, \n) != 1 ||
 close_ref(lock)  0)) {
status |= error(Couldn't write %s,
-- 
2.1.3

--
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 05/23] expire_reflog(): rename ref parameter to refname

[Michael Haggerty]

This is our usual convention.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/reflog.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3e11bee..df302f4 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -349,7 +349,7 @@ static int push_tip_to_list(const char *refname, const 
unsigned char *sha1, int
return 0;
 }
 
-static int expire_reflog(const char *ref, const unsigned char *sha1, void 
*cb_data)
+static int expire_reflog(const char *refname, const unsigned char *sha1, void 
*cb_data)
 {
struct cmd_reflog_expire_cb *cmd = cb_data;
struct expire_reflog_cb cb;
@@ -365,20 +365,20 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, void *cb_da
 * we take the lock for the ref itself to prevent it from
 * getting updated.
 */
-   lock = lock_any_ref_for_update(ref, sha1, 0, NULL);
+   lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
if (!lock)
-   return error(cannot lock ref '%s', ref);
-   log_file = git_pathdup(logs/%s, ref);
-   if (!reflog_exists(ref))
+   return error(cannot lock ref '%s', refname);
+   log_file = git_pathdup(logs/%s, refname);
+   if (!reflog_exists(refname))
goto finish;
if (!cmd-dry_run) {
-   newlog_path = git_pathdup(logs/%s.lock, ref);
+   newlog_path = git_pathdup(logs/%s.lock, refname);
cb.newlog = fopen(newlog_path, w);
}
 
cb.cmd = cmd;
 
-   if (!cmd-expire_unreachable || !strcmp(ref, HEAD)) {
+   if (!cmd-expire_unreachable || !strcmp(refname, HEAD)) {
tip_commit = NULL;
cb.unreachable_expire_kind = UE_HEAD;
} else {
@@ -407,7 +407,7 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, void *cb_da
mark_reachable(cb);
}
 
-   for_each_reflog_ent(ref, expire_reflog_ent, cb);
+   for_each_reflog_ent(refname, expire_reflog_ent, cb);
 
if (cb.unreachable_expire_kind != UE_ALWAYS) {
if (cb.unreachable_expire_kind == UE_HEAD) {
-- 
2.1.3

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