Client Relationship Manager!

[Jobs]

Dalian machine toolz limited seeks to recruit full and part time Managers, 
Representatives, Partners, Expatriates and Consultancy firms to render 
expertise and non-expertise services in our overseas new and emerging markets / 
offices located in Canada, Mexico and USA.





If you are Interested in working with us, You are advised to send your detailed 
Resume / Application or Contact details for immediate consideration.




Warm Regards,
HR Dept.

Re: [RFC PATCH] show decorations at the end of the line

[Junio C Hamano]

Linus Torvalds  writes:

> On Sat, Feb 11, 2017 at 10:02 AM, Linus Torvalds
>  wrote:
>>
>> I've signed off on this, because I think it's an "obvious" improvement,
>> but I'm putting the "RFC" in the subject line because this is clearly a
>> subjective thing.
>
> Side note: the one downside of showing the decorations at the end of
> the line is that now they are obviously at the end of the line - and
> thus likely to be more hidden by things like line truncation.

Side note: I refrained from commenting on this patch because
everybody knows that the what I would say anyway ;-) and I didn't
want to speak first to discourage others from raising their opinion.

An obvious downside is that people (against all recommendations) are
likely to have written a loose script expecting the --oneline format
is cast in stone.  I personally think it is OK to break them as long
as "workaround" (aka kosher way to do what they have been doing) is
obvious and easily doable, and in this case their script can switch
to use --format to keep using the order of fields and format they
have been relying on.

It would be nice if we can have that --format string they can use
somewhere in the log message, so that I can cut & paste it into the
release notes that contains this change (i.e. "those who want to
keep using the traditional --oneline --decorate can use this string
as pretty.my1line configuration variable and use --pretty=my1line
instead").

[PATCH] clean: use warning_errno() when appropriate

[Nguyễn Thái Ngọc Duy]

All these warning() calls are preceded by a system call. Report the
actual error to help the user understand why we fail to remove
something.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/clean.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index d6bc3aaaea..dc1168747e 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -175,7 +175,7 @@ static int remove_dirs(struct strbuf *path, const char 
*prefix, int force_flag,
res = dry_run ? 0 : rmdir(path->buf);
if (res) {
quote_path_relative(path->buf, prefix, "ed);
-   warning(_(msg_warn_remove_failed), quoted.buf);
+   warning_errno(_(msg_warn_remove_failed), quoted.buf);
*dir_gone = 0;
}
return res;
@@ -209,7 +209,7 @@ static int remove_dirs(struct strbuf *path, const char 
*prefix, int force_flag,
string_list_append(&dels, quoted.buf);
} else {
quote_path_relative(path->buf, prefix, "ed);
-   warning(_(msg_warn_remove_failed), quoted.buf);
+   warning_errno(_(msg_warn_remove_failed), 
quoted.buf);
*dir_gone = 0;
ret = 1;
}
@@ -231,7 +231,7 @@ static int remove_dirs(struct strbuf *path, const char 
*prefix, int force_flag,
*dir_gone = 1;
else {
quote_path_relative(path->buf, prefix, "ed);
-   warning(_(msg_warn_remove_failed), quoted.buf);
+   warning_errno(_(msg_warn_remove_failed), quoted.buf);
*dir_gone = 0;
ret = 1;
}
@@ -982,7 +982,7 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
res = dry_run ? 0 : unlink(abs_path.buf);
if (res) {
qname = quote_path_relative(item->string, NULL, 
&buf);
-   warning(_(msg_warn_remove_failed), qname);
+   warning_errno(_(msg_warn_remove_failed), qname);
errors++;
} else if (!quiet) {
qname = quote_path_relative(item->string, NULL, 
&buf);
-- 
2.11.0.157.gd943d85

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Johannes Schindelin]

Hi Arif,

On Mon, 13 Feb 2017, Arif Khokar wrote:

> On 02/10/2017 11:10 AM, Johannes Schindelin wrote:
> >
> > On Wed, 24 Aug 2016, Johannes Schindelin wrote:
> 
> > > I recently adapted an old script I had to apply an entire patch
> > > series given the GMane link to its cover letter:
> > >
> > > https://github.com/git-for-windows/build-extra/blob/master/apply-from-gmane.sh
> > >
> > > Maybe you find it in you to adapt that to work with
> > > public-inbox.org?
> >
> > Oh well. That would have been too easy a task, right?
> >
> > As it happens, I needed this functionality myself (when reworking my
> > git-path-in-subdir patch to include Mike Rappazzo's previous patch
> > series that tried to fix the same bug).
> >
> > I copy-edited the script to work with public-inbox.org, it accepts a
> > Message-ID or URL or GMane URL and will try to apply the patch (or
> > patch series) on top of the current revision:
> >
> > https://github.com/git-for-windows/build-extra/blob/2268850552c7/apply-from-public-inbox.sh
> 
> Thanks for the link.  One thing that comes to mind that is that it may
> be better to just download the patches and then manually apply them
> afterwords rather than doing it in the script itself.  Or at least add
> an option to the script to not automatically invoke git am.

I actually had expected *you* to put in a little bit of an effort, too. In
fact, I was very disappointed that you did not even look into porting that
script to use public-inbox instead of GMane.

> Getting back to the point I made when this thread was still active, I
> still think it would be better to be able to list the message-id values
> in the header or body of the cover letter message of a patch series
> (preferably the former) in order to facilitate downloading the patches
> via NNTP from gmane or public-inbox.org.  That would make it easier
> compared to the different, ad-hoc, methods that exist for each email
> client.

You can always do that yourself: you can modify your cover letter to
include that information.

Note that doing this automatically in format-patch may not be appropriate,
as 1) the Message-ID could be modified depending on the mail client used
to send the mails, and 2) it is not unheard of that a developer
finds a bug in the middle of sending a patch series, fixes that bug, and
regenerates the remainder of the patch series, completely rewriting those
Message-IDs.

> Alternatively, or perhaps in addition to the list of message-ids, a list
> of URLs to public-inbox.org or gmane messages could also be provided for
> those who prefer to download patches via HTTP.

At this point, I am a little disinterested in a discussion without code. I
brought some code to the table, after all.

Ciao,
Johannes

Re: Hi

[Jessy]

Hello,



I wish to seek for your assistance in a deal that will be of mutual benefit for 
the both of us from Camp Stanley in Uijeongbu, South Korea. Please contact me 
for details, God bless you.

[PATCH/RFC 00/11] Remove submodule from files-backend.c

[Nguyễn Thái Ngọc Duy]

This is on top of mh/submodule-hash, it:

 - centralizes path manipulation around submodule, $GIT_DIR... in
   files-backend.c to a new function files_path() with the intention
   to make it easier to change later. files_path() is destined to
   become

  strbuf_addbuf(&sb, refs->gitdir);
  strbuf_complete(&sb, '/');
  strbuf_vaddf(&sb, fmt, vap);

   but that can only be achieved once we get compound ref store.

 - removes git_path() and submodule_git_path() from files-backend.c. No
   more magic path translation as far as ref backend is concerned.

 - moves submodule path resolution code outside the backend.
   files-backend is now oblivious of submodules and in theory a
   submodule ref-store supports all operations (but I could be wrong,
   I didn't stare hard)

 - exposes get_submodule_ref_store() and get_main_ref_store() as public
   API. A new set of API around ref-store will be added. And
   get_worktree_ref_store() of course. The *_submodule() API might be
   removed, we'll see.

The problem with set_worktree_head_symref() (which peeks into another
gitdir) should be solved once we have get_worktree_ref_store(). That's
my next step.

Compound ref store will have to wait until I'm done with my
gc-in-worktree problem as I think I can live without it for now. I
think after compound ref store is in place, adding lmdb backend back
should be much cleaner because it does not care about either
submodules or worktrees.

Nguyễn Thái Ngọc Duy (11):
  refs-internal.c: make files_log_ref_write() static
  files-backend: convert git_path() to strbuf_git_path()
  files-backend: add files_path()
  files-backend: replace *git_path*() with files_path()
  refs.c: share is_per_worktree_ref() to files-backend.c
  refs-internal.h: correct is_per_worktree_ref()
  files-backend: remove the use of git_path()
  refs.c: factor submodule code out of get_ref_store()
  refs: move submodule code out of files-backend.c
  files-backend: remove submodule_allowed from files_downcast()
  refs: split and make get_*_ref_store() public API

 refs.c   | 125 --
 refs.h   |  13 ++
 refs/files-backend.c | 350 ++-
 refs/refs-internal.h |  28 ++---
 4 files changed, 316 insertions(+), 200 deletions(-)

-- 
2.11.0.157.gd943d85

[PATCH 08/11] refs.c: factor submodule code out of get_ref_store()

[Nguyễn Thái Ngọc Duy]

This code is going to be expanded a bit soon. Keep it out for
better readability later.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 2cacd934e..8ef7a52ba 100644
--- a/refs.c
+++ b/refs.c
@@ -1445,6 +1445,18 @@ static struct ref_store *lookup_ref_store(const char 
*submodule)
return entry ? entry->refs : NULL;
 }
 
+static struct ref_store *init_submodule_ref_store(const char *submodule)
+{
+   struct strbuf submodule_sb = STRBUF_INIT;
+   struct ref_store *refs = NULL;
+
+   strbuf_addstr(&submodule_sb, submodule);
+   if (is_nonbare_repository_dir(&submodule_sb))
+   refs = ref_store_init(submodule);
+   strbuf_release(&submodule_sb);
+   return refs;
+}
+
 struct ref_store *get_ref_store(const char *submodule)
 {
struct ref_store *refs;
@@ -1457,14 +1469,8 @@ struct ref_store *get_ref_store(const char *submodule)
} else {
refs = lookup_ref_store(submodule);
 
-   if (!refs) {
-   struct strbuf submodule_sb = STRBUF_INIT;
-
-   strbuf_addstr(&submodule_sb, submodule);
-   if (is_nonbare_repository_dir(&submodule_sb))
-   refs = ref_store_init(submodule);
-   strbuf_release(&submodule_sb);
-   }
+   if (!refs)
+   refs = init_submodule_ref_store(submodule);
}
 
return refs;
-- 
2.11.0.157.gd943d85

[PATCH 10/11] files-backend: remove submodule_allowed from files_downcast()

[Nguyễn Thái Ngọc Duy]

Since submodule or not is irrelevant to files-backend after the last
patch, remove the parameter from files_downcast() and kill
files_assert_main_repository().

PS. This implies that all ref operations are allowed for submodules. But
we may need to look more closely to see if that's really true...

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 69 
 1 file changed, 21 insertions(+), 48 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 834bc6fdf..2fb270b6f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1019,23 +1019,13 @@ static struct ref_store *files_ref_store_create(const 
char *gitdir)
 }
 
 /*
- * Die if refs is for a submodule (i.e., not for the main repository).
- * caller is used in any necessary error messages.
- */
-static void files_assert_main_repository(struct files_ref_store *refs,
-const char *caller)
-{
-}
-
-/*
  * Downcast ref_store to files_ref_store. Die if ref_store is not a
  * files_ref_store. If submodule_allowed is not true, then also die if
  * files_ref_store is for a submodule (i.e., not for the main
  * repository). caller is used in any necessary error messages.
  */
-static struct files_ref_store *files_downcast(
-   struct ref_store *ref_store, int submodule_allowed,
-   const char *caller)
+static struct files_ref_store *files_downcast(struct ref_store *ref_store,
+ const char *caller)
 {
struct files_ref_store *refs;
 
@@ -1045,9 +1035,6 @@ static struct files_ref_store *files_downcast(
 
refs = (struct files_ref_store *)ref_store;
 
-   if (!submodule_allowed)
-   files_assert_main_repository(refs, caller);
-
return refs;
 }
 
@@ -1383,7 +1370,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
  struct strbuf *referent, unsigned int *type)
 {
struct files_ref_store *refs =
-   files_downcast(ref_store, 1, "read_raw_ref");
+   files_downcast(ref_store, "read_raw_ref");
struct strbuf sb_contents = STRBUF_INIT;
struct strbuf sb_path = STRBUF_INIT;
const char *path;
@@ -1576,7 +1563,6 @@ static int lock_raw_ref(struct files_ref_store *refs,
int ret = TRANSACTION_GENERIC_ERROR;
 
assert(err);
-   files_assert_main_repository(refs, "lock_raw_ref");
 
*type = 0;
 
@@ -1800,7 +1786,7 @@ static enum peel_status peel_entry(struct ref_entry 
*entry, int repeel)
 static int files_peel_ref(struct ref_store *ref_store,
  const char *refname, unsigned char *sha1)
 {
-   struct files_ref_store *refs = files_downcast(ref_store, 0, "peel_ref");
+   struct files_ref_store *refs = files_downcast(ref_store, "peel_ref");
int flag;
unsigned char base[20];
 
@@ -1909,7 +1895,7 @@ static struct ref_iterator *files_ref_iterator_begin(
const char *prefix, unsigned int flags)
 {
struct files_ref_store *refs =
-   files_downcast(ref_store, 1, "ref_iterator_begin");
+   files_downcast(ref_store, "ref_iterator_begin");
struct ref_dir *loose_dir, *packed_dir;
struct ref_iterator *loose_iter, *packed_iter;
struct files_ref_iterator *iter;
@@ -2042,7 +2028,6 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
int attempts_remaining = 3;
int resolved;
 
-   files_assert_main_repository(refs, "lock_ref_sha1_basic");
assert(err);
 
lock = xcalloc(1, sizeof(struct ref_lock));
@@ -2190,8 +2175,6 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
struct strbuf sb = STRBUF_INIT;
int ret;
 
-   files_assert_main_repository(refs, "lock_packed_refs");
-
if (!timeout_configured) {
git_config_get_int("core.packedrefstimeout", &timeout_value);
timeout_configured = 1;
@@ -2231,8 +2214,6 @@ static int commit_packed_refs(struct files_ref_store 
*refs)
int save_errno = 0;
FILE *out;
 
-   files_assert_main_repository(refs, "commit_packed_refs");
-
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
 
@@ -2264,8 +2245,6 @@ static void rollback_packed_refs(struct files_ref_store 
*refs)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(refs);
 
-   files_assert_main_repository(refs, "rollback_packed_refs");
-
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
rollback_lock_file(packed_ref_cache->lock);
@@ -2411,7 +2390,7 @@ static void prune_refs(struct files_ref_store *refs, 
struct ref_to_prune *r)
 static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 {
struct files_ref_store *refs =
-  

[PATCH 11/11] refs: split and make get_*_ref_store() public API

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 59 ++--
 refs.h   | 13 
 refs/files-backend.c |  2 +-
 refs/refs-internal.h | 12 ---
 4 files changed, 48 insertions(+), 38 deletions(-)

diff --git a/refs.c b/refs.c
index 9ac194945..48350da87 100644
--- a/refs.c
+++ b/refs.c
@@ -1160,7 +1160,7 @@ int head_ref(each_ref_fn fn, void *cb_data)
 static int do_for_each_ref(const char *submodule, const char *prefix,
   each_ref_fn fn, int trim, int flags, void *cb_data)
 {
-   struct ref_store *refs = get_ref_store(submodule);
+   struct ref_store *refs = get_submodule_ref_store(submodule);
struct ref_iterator *iter;
 
if (!refs)
@@ -1304,7 +1304,7 @@ const char *resolve_ref_recursively(struct ref_store 
*refs,
 /* backend functions */
 int refs_init_db(struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->init_db(refs, err);
 }
@@ -1312,7 +1312,7 @@ int refs_init_db(struct strbuf *err)
 const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
   unsigned char *sha1, int *flags)
 {
-   return resolve_ref_recursively(get_ref_store(NULL), refname,
+   return resolve_ref_recursively(get_main_ref_store(), refname,
   resolve_flags, sha1, flags);
 }
 
@@ -1333,10 +1333,10 @@ int resolve_gitlink_ref(const char *submodule, const 
char *refname,
/* We need to strip off one or more trailing slashes */
char *stripped = xmemdupz(submodule, len);
 
-   refs = get_ref_store(stripped);
+   refs = get_submodule_ref_store(stripped);
free(stripped);
} else {
-   refs = get_ref_store(submodule);
+   refs = get_submodule_ref_store(submodule);
}
 
if (!refs)
@@ -1491,15 +1491,24 @@ static struct ref_store *init_submodule_ref_store(const 
char *path)
return refs;
 }
 
-struct ref_store *get_ref_store(const char *submodule)
+struct ref_store *get_main_ref_store(void)
 {
struct ref_store *refs;
 
-   if (!submodule || !*submodule) {
-   refs = lookup_ref_store(NULL);
+   refs = lookup_ref_store(NULL);
 
-   if (!refs)
-   refs = ref_store_init(NULL, NULL);
+   if (!refs)
+   refs = ref_store_init(NULL, NULL);
+
+   return refs;
+}
+
+struct ref_store *get_submodule_ref_store(const char *submodule)
+{
+   struct ref_store *refs;
+
+   if (!submodule || !*submodule) {
+   return get_main_ref_store();
} else {
refs = lookup_ref_store(submodule);
 
@@ -1519,14 +1528,14 @@ void base_ref_store_init(struct ref_store *refs,
 /* backend functions */
 int pack_refs(unsigned int flags)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->pack_refs(refs, flags);
 }
 
 int peel_ref(const char *refname, unsigned char *sha1)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->peel_ref(refs, refname, sha1);
 }
@@ -1534,7 +1543,7 @@ int peel_ref(const char *refname, unsigned char *sha1)
 int create_symref(const char *ref_target, const char *refs_heads_master,
  const char *logmsg)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->create_symref(refs, ref_target, refs_heads_master,
   logmsg);
@@ -1543,7 +1552,7 @@ int create_symref(const char *ref_target, const char 
*refs_heads_master,
 int ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->transaction_commit(refs, transaction, err);
 }
@@ -1553,14 +1562,14 @@ int verify_refname_available(const char *refname,
 const struct string_list *skip,
 struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->verify_refname_available(refs, refname, extra, skip, 
err);
 }
 
 int for_each_reflog(each_ref_fn fn, void *cb_data)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
struct ref_iterator *iter;
 
iter = refs->be->reflog_iterator_begin(refs);
@@ -1571,7 +1580,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn,
 

[PATCH 09/11] refs: move submodule code out of files-backend.c

[Nguyễn Thái Ngọc Duy]

files-backend is now initialized with a $GIT_DIR. Converting a submodule
path to where real submodule gitdir is located is done in get_ref_store().

The new code in init_submodule_ref_store() is basically a copy of
strbuf_git_path_submodule().

This gives a slight performance improvement for submodules since we
don't convert submodule path to gitdir at every backend call like
before. We pay that once at ref-store creation.

More cleanup in files_downcast() follows shortly. It's separate to keep
noises from this patch.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 48 +---
 refs/files-backend.c | 25 +++--
 refs/refs-internal.h |  6 +++---
 3 files changed, 51 insertions(+), 28 deletions(-)

diff --git a/refs.c b/refs.c
index 8ef7a52ba..9ac194945 100644
--- a/refs.c
+++ b/refs.c
@@ -9,6 +9,7 @@
 #include "refs/refs-internal.h"
 #include "object.h"
 #include "tag.h"
+#include "submodule-config.h"
 
 /*
  * List of all available backends
@@ -1410,7 +1411,7 @@ static void register_ref_store(struct ref_store *refs, 
const char *submodule)
  * Create, record, and return a ref_store instance for the specified
  * submodule (or the main repository if submodule is NULL).
  */
-static struct ref_store *ref_store_init(const char *submodule)
+static struct ref_store *ref_store_init(const char *submodule, const char 
*gitdir)
 {
const char *be_name = "files";
struct ref_storage_be *be = find_ref_storage_backend(be_name);
@@ -1419,7 +1420,7 @@ static struct ref_store *ref_store_init(const char 
*submodule)
if (!be)
die("BUG: reference backend %s is unknown", be_name);
 
-   refs = be->init(submodule);
+   refs = be->init(gitdir);
register_ref_store(refs, submodule);
return refs;
 }
@@ -1445,15 +1446,48 @@ static struct ref_store *lookup_ref_store(const char 
*submodule)
return entry ? entry->refs : NULL;
 }
 
-static struct ref_store *init_submodule_ref_store(const char *submodule)
+static struct ref_store *init_submodule_ref_store(const char *path)
 {
struct strbuf submodule_sb = STRBUF_INIT;
+   struct strbuf git_submodule_common_dir = STRBUF_INIT;
+   struct strbuf git_submodule_dir = STRBUF_INIT;
+   struct strbuf buf = STRBUF_INIT;
+   const char *git_dir;
+   const struct submodule *sub;
struct ref_store *refs = NULL;
 
-   strbuf_addstr(&submodule_sb, submodule);
-   if (is_nonbare_repository_dir(&submodule_sb))
-   refs = ref_store_init(submodule);
+   strbuf_addstr(&submodule_sb, path);
+   if (!is_nonbare_repository_dir(&submodule_sb))
+   goto done;
+
+   strbuf_addstr(&buf, path);
+   strbuf_complete(&buf, '/');
+   strbuf_addstr(&buf, ".git");
+
+   git_dir = read_gitfile(buf.buf);
+   if (git_dir) {
+   strbuf_reset(&buf);
+   strbuf_addstr(&buf, git_dir);
+   }
+   if (!is_git_directory(buf.buf)) {
+   gitmodules_config();
+   sub = submodule_from_path(null_sha1, path);
+   if (!sub)
+   goto done;
+   strbuf_reset(&buf);
+   strbuf_git_path(&buf, "%s/%s", "modules", sub->name);
+   }
+   strbuf_addch(&buf, '/');
+   strbuf_addbuf(&git_submodule_dir, &buf);
+
+   refs = ref_store_init(path, git_submodule_dir.buf);
+
+done:
+   strbuf_release(&git_submodule_dir);
+   strbuf_release(&git_submodule_common_dir);
strbuf_release(&submodule_sb);
+   strbuf_release(&buf);
+
return refs;
 }
 
@@ -1465,7 +1499,7 @@ struct ref_store *get_ref_store(const char *submodule)
refs = lookup_ref_store(NULL);
 
if (!refs)
-   refs = ref_store_init(NULL);
+   refs = ref_store_init(NULL, NULL);
} else {
refs = lookup_ref_store(submodule);
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 50eb9edb6..834bc6fdf 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -920,13 +920,6 @@ struct packed_ref_cache {
 struct files_ref_store {
struct ref_store base;
 
-   /*
-* The name of the submodule represented by this object, or
-* NULL if it represents the main repository's reference
-* store:
-*/
-   const char *submodule;
-
struct strbuf gitdir;
struct strbuf gitcommondir;
 
@@ -947,12 +940,9 @@ static void files_path(struct files_ref_store *refs, 
struct strbuf *sb,
va_start(vap, fmt);
strbuf_vaddf(&tmp, fmt, vap);
va_end(vap);
-   if (refs->submodule)
-   strbuf_git_path_submodule(sb, refs->submodule,
- "%s", tmp.buf);
-   else if (is_per_worktree_ref(tmp.buf) ||
-(skip_prefix(tmp.buf, "logs/", &ref) &&
- is_per_worktree_ref(ref)))
+

[PATCH 01/11] refs-internal.c: make files_log_ref_write() static

[Nguyễn Thái Ngọc Duy]

Created in 5f3c3a4e6f (files_log_ref_write: new function - 2015-11-10)
but probably never used outside refs-internal.c

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 3 +++
 refs/refs-internal.h | 4 
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index cdb6b8ff5..75565c3aa 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -165,6 +165,9 @@ static struct ref_entry *create_dir_entry(struct 
files_ref_store *ref_store,
  const char *dirname, size_t len,
  int incomplete);
 static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
+static int files_log_ref_write(const char *refname, const unsigned char 
*old_sha1,
+  const unsigned char *new_sha1, const char *msg,
+  int flags, struct strbuf *err);
 
 static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 {
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 33adbf93b..59e65958a 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -222,10 +222,6 @@ struct ref_transaction {
enum ref_transaction_state state;
 };
 
-int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
-   const unsigned char *new_sha1, const char *msg,
-   int flags, struct strbuf *err);
-
 /*
  * Check for entries in extras that are within the specified
  * directory, where dirname is a reference directory name including
-- 
2.11.0.157.gd943d85

[PATCH 04/11] files-backend: replace *git_path*() with files_path()

[Nguyễn Thái Ngọc Duy]

This centralizes all path rewriting of files-backend.c in one place so
we have easier time removing the path rewriting later. There could be
some hidden indirect git_path() though, I didn't audit the code to the
bottom.

Side note: set_worktree_head_symref() is a bad boy and should not be in
files-backend.c (probably should not exist in the first place). But
we'll leave it there until we have better multi-worktree support in refs
before we update it.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 193 ++-
 1 file changed, 98 insertions(+), 95 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 39217a2ca..c69e4fe84 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -165,12 +165,13 @@ static struct ref_entry *create_dir_entry(struct 
files_ref_store *ref_store,
  const char *dirname, size_t len,
  int incomplete);
 static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
-static int files_log_ref_write(const char *refname, const unsigned char 
*old_sha1,
+static int files_log_ref_write(struct files_ref_store *refs,
+  const char *refname, const unsigned char 
*old_sha1,
   const unsigned char *new_sha1, const char *msg,
   int flags, struct strbuf *err);
 
-void files_path(struct files_ref_store *refs, struct strbuf *sb,
-   const char *fmt, ...) __attribute__((format (printf, 3, 4)));
+static void files_path(struct files_ref_store *refs, struct strbuf *sb,
+  const char *fmt, ...) __attribute__((format (printf, 3, 
4)));
 
 static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 {
@@ -933,8 +934,8 @@ struct files_ref_store {
 /* Lock used for the main packed-refs file: */
 static struct lock_file packlock;
 
-void files_path(struct files_ref_store *refs, struct strbuf *sb,
-   const char *fmt, ...)
+static void files_path(struct files_ref_store *refs, struct strbuf *sb,
+  const char *fmt, ...)
 {
struct strbuf tmp = STRBUF_INIT;
va_list vap;
@@ -1180,12 +1181,10 @@ static void read_packed_refs(FILE *f, struct ref_dir 
*dir)
 static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store 
*refs)
 {
char *packed_refs_file;
+   struct strbuf sb = STRBUF_INIT;
 
-   if (refs->submodule)
-   packed_refs_file = git_pathdup_submodule(refs->submodule,
-"packed-refs");
-   else
-   packed_refs_file = git_pathdup("packed-refs");
+   files_path(refs, &sb, "packed-refs");
+   packed_refs_file = strbuf_detach(&sb, NULL);
 
if (refs->packed &&
!stat_validity_check(&refs->packed->validity, packed_refs_file))
@@ -1251,10 +1250,7 @@ static void read_loose_refs(const char *dirname, struct 
ref_dir *dir)
size_t path_baselen;
int err = 0;
 
-   if (refs->submodule)
-   err = strbuf_git_path_submodule(&path, refs->submodule, "%s", 
dirname);
-   else
-   strbuf_git_path(&path, "%s", dirname);
+   files_path(refs, &path, "%s", dirname);
path_baselen = path.len;
 
if (err) {
@@ -1396,10 +1392,7 @@ static int files_read_raw_ref(struct ref_store 
*ref_store,
*type = 0;
strbuf_reset(&sb_path);
 
-   if (refs->submodule)
-   strbuf_git_path_submodule(&sb_path, refs->submodule, "%s", 
refname);
-   else
-   strbuf_git_path(&sb_path, "%s", refname);
+   files_path(refs, &sb_path, "%s", refname);
 
path = sb_path.buf;
 
@@ -1587,7 +1580,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
*lock_p = lock = xcalloc(1, sizeof(*lock));
 
lock->ref_name = xstrdup(refname);
-   strbuf_git_path(&ref_file, "%s", refname);
+   files_path(refs, &ref_file, "%s", refname);
 
 retry:
switch (safe_create_leading_directories(ref_file.buf)) {
@@ -2054,7 +2047,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
if (flags & REF_DELETING)
resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
 
-   strbuf_git_path(&ref_file, "%s", refname);
+   files_path(refs, &ref_file, "%s", refname);
resolved = !!resolve_ref_unsafe(refname, resolve_flags,
lock->old_oid.hash, type);
if (!resolved && errno == EISDIR) {
@@ -2199,7 +2192,7 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
timeout_configured = 1;
}
 
-   strbuf_git_path(&sb, "packed-refs");
+   files_path(refs, &sb, "packed-refs");
ret = hold_lock_file_for_update_timeout(&packlock, sb.buf,
flags, timeout_value);
strbuf

[PATCH 06/11] refs-internal.h: correct is_per_worktree_ref()

[Nguyễn Thái Ngọc Duy]

All refs outside refs/ directory is per-worktree, not just HEAD.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/refs-internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index f4aed49f5..69d02b6ba 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -653,7 +653,7 @@ const char *resolve_ref_recursively(struct ref_store *refs,
 
 static inline int is_per_worktree_ref(const char *refname)
 {
-   return !strcmp(refname, "HEAD") ||
+   return !starts_with(refname, "refs/") ||
starts_with(refname, "refs/bisect/");
 }
 
-- 
2.11.0.157.gd943d85

[PATCH 02/11] files-backend: convert git_path() to strbuf_git_path()

[Nguyễn Thái Ngọc Duy]

git_path() and friends are going to be killed in files-backend.c in near
future. And because there's a risk with overwriting buffer in
git_path(), let's convert them all to strbuf_git_path(). We'll have
easier time killing/converting strbuf_git_path() then because we won't
have to worry about memory management again.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 114 ---
 1 file changed, 90 insertions(+), 24 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 75565c3aa..6582c9b2d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2169,6 +2169,8 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
static int timeout_configured = 0;
static int timeout_value = 1000;
struct packed_ref_cache *packed_ref_cache;
+   struct strbuf sb = STRBUF_INIT;
+   int ret;
 
files_assert_main_repository(refs, "lock_packed_refs");
 
@@ -2177,10 +2179,13 @@ static int lock_packed_refs(struct files_ref_store 
*refs, int flags)
timeout_configured = 1;
}
 
-   if (hold_lock_file_for_update_timeout(
-   &packlock, git_path("packed-refs"),
-   flags, timeout_value) < 0)
+   strbuf_git_path(&sb, "packed-refs");
+   ret = hold_lock_file_for_update_timeout(&packlock, sb.buf,
+   flags, timeout_value);
+   strbuf_release(&sb);
+   if (ret < 0)
return -1;
+
/*
 * Get the current packed-refs while holding the lock.  If the
 * packed-refs file has been modified since we last read it,
@@ -2335,6 +2340,9 @@ static void try_remove_empty_parents(char *name)
for (q = p; *q; q++)
;
while (1) {
+   struct strbuf sb = STRBUF_INIT;
+   int ret;
+
while (q > p && *q != '/')
q--;
while (q > p && *(q-1) == '/')
@@ -2342,7 +2350,10 @@ static void try_remove_empty_parents(char *name)
if (q == p)
break;
*q = '\0';
-   if (rmdir(git_path("%s", name)))
+   strbuf_git_path(&sb, "%s", name);
+   ret = rmdir(sb.buf);
+   strbuf_release(&sb);
+   if (ret)
break;
}
 }
@@ -2431,7 +2442,11 @@ static int repack_without_refs(struct files_ref_store 
*refs,
return 0; /* no refname exists in packed refs */
 
if (lock_packed_refs(refs, 0)) {
-   unable_to_lock_message(git_path("packed-refs"), errno, err);
+   struct strbuf sb = STRBUF_INIT;
+
+   strbuf_git_path(&sb, "packed-refs");
+   unable_to_lock_message(sb.buf, errno, err);
+   strbuf_release(&sb);
return -1;
}
packed = get_packed_refs(refs);
@@ -2529,11 +2544,12 @@ static int rename_tmp_log(const char *newrefname)
 {
int attempts_remaining = 4;
struct strbuf path = STRBUF_INIT;
+   struct strbuf tmp_renamed_log = STRBUF_INIT;
int ret = -1;
 
- retry:
-   strbuf_reset(&path);
strbuf_git_path(&path, "logs/%s", newrefname);
+   strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
+ retry:
switch (safe_create_leading_directories_const(path.buf)) {
case SCLD_OK:
break; /* success */
@@ -2546,7 +2562,7 @@ static int rename_tmp_log(const char *newrefname)
goto out;
}
 
-   if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
+   if (rename(tmp_renamed_log.buf, path.buf)) {
if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 
0) {
/*
 * rename(a, b) when b is an existing
@@ -2574,6 +2590,7 @@ static int rename_tmp_log(const char *newrefname)
ret = 0;
 out:
strbuf_release(&path);
+   strbuf_release(&tmp_renamed_log);
return ret;
 }
 
@@ -2614,9 +2631,15 @@ static int files_rename_ref(struct ref_store *ref_store,
int flag = 0, logmoved = 0;
struct ref_lock *lock;
struct stat loginfo;
-   int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
+   struct strbuf sb_oldref = STRBUF_INIT;
+   struct strbuf sb_newref = STRBUF_INIT;
+   struct strbuf tmp_renamed_log = STRBUF_INIT;
+   int log, ret;
struct strbuf err = STRBUF_INIT;
 
+   strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
+   log = !lstat(sb_oldref.buf, &loginfo);
+   strbuf_release(&sb_oldref);
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
 
@@ -2630,7 +2653,12 @@ static int files_rename_ref(struct ref_store *ref_store,
if (!rename_ref_available(oldrefname, newrefname))
return 1;
 
-   i

[PATCH 07/11] files-backend: remove the use of git_path()

[Nguyễn Thái Ngọc Duy]

Given $GIT_DIR and $GIT_COMMON_DIR, files-backend is now in charge of
deciding what goes where. The end goal is to pass $GIT_DIR only. A
refs "view" of a linked worktree is a logical ref store that combines
two files backends together.

(*) Not entirely true since strbuf_git_path_submodule() still does path
translation underneath. But that's for another patch.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index c69e4fe84..50eb9edb6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -927,6 +927,9 @@ struct files_ref_store {
 */
const char *submodule;
 
+   struct strbuf gitdir;
+   struct strbuf gitcommondir;
+
struct ref_entry *loose;
struct packed_ref_cache *packed;
 };
@@ -939,6 +942,7 @@ static void files_path(struct files_ref_store *refs, struct 
strbuf *sb,
 {
struct strbuf tmp = STRBUF_INIT;
va_list vap;
+   const char *ref;
 
va_start(vap, fmt);
strbuf_vaddf(&tmp, fmt, vap);
@@ -946,8 +950,12 @@ static void files_path(struct files_ref_store *refs, 
struct strbuf *sb,
if (refs->submodule)
strbuf_git_path_submodule(sb, refs->submodule,
  "%s", tmp.buf);
+   else if (is_per_worktree_ref(tmp.buf) ||
+(skip_prefix(tmp.buf, "logs/", &ref) &&
+ is_per_worktree_ref(ref)))
+   strbuf_addf(sb, "%s/%s", refs->gitdir.buf, tmp.buf);
else
-   strbuf_git_path(sb, "%s", tmp.buf);
+   strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
strbuf_release(&tmp);
 }
 
@@ -1006,7 +1014,15 @@ static struct ref_store *files_ref_store_create(const 
char *submodule)
 
base_ref_store_init(ref_store, &refs_be_files);
 
-   refs->submodule = xstrdup_or_null(submodule);
+   strbuf_init(&refs->gitdir, 0);
+   strbuf_init(&refs->gitcommondir, 0);
+
+   if (submodule) {
+   refs->submodule = xstrdup_or_null(submodule);
+   } else {
+   strbuf_addstr(&refs->gitdir, get_git_dir());
+   strbuf_addstr(&refs->gitcommondir, get_git_common_dir());
+   }
 
return ref_store;
 }
-- 
2.11.0.157.gd943d85

[PATCH 05/11] refs.c: share is_per_worktree_ref() to files-backend.c

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 6 --
 refs/refs-internal.h | 6 ++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index f03dcf58b..2cacd934e 100644
--- a/refs.c
+++ b/refs.c
@@ -489,12 +489,6 @@ int dwim_log(const char *str, int len, unsigned char 
*sha1, char **log)
return logs_found;
 }
 
-static int is_per_worktree_ref(const char *refname)
-{
-   return !strcmp(refname, "HEAD") ||
-   starts_with(refname, "refs/bisect/");
-}
-
 static int is_pseudoref_syntax(const char *refname)
 {
const char *c;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 59e65958a..f4aed49f5 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -651,4 +651,10 @@ const char *resolve_ref_recursively(struct ref_store *refs,
int resolve_flags,
unsigned char *sha1, int *flags);
 
+static inline int is_per_worktree_ref(const char *refname)
+{
+   return !strcmp(refname, "HEAD") ||
+   starts_with(refname, "refs/bisect/");
+}
+
 #endif /* REFS_REFS_INTERNAL_H */
-- 
2.11.0.157.gd943d85

[PATCH 03/11] files-backend: add files_path()

[Nguyễn Thái Ngọc Duy]

This will be the replacement for both git_path() and git_path_submodule()
in this file. The idea is backend takes a git path and use that,
oblivious of submodule, linked worktrees and such.

This is the middle step towards that. Eventually the "submodule" field
in 'struct files_ref_store' should be replace by "gitdir". And a
compound ref_store is created to combine two files backends together,
one represents the shared refs in $GIT_COMMON_DIR, one per-worktree. At
that point, files_path() becomes a wrapper of strbuf_vaddf().

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6582c9b2d..39217a2ca 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -169,6 +169,9 @@ static int files_log_ref_write(const char *refname, const 
unsigned char *old_sha
   const unsigned char *new_sha1, const char *msg,
   int flags, struct strbuf *err);
 
+void files_path(struct files_ref_store *refs, struct strbuf *sb,
+   const char *fmt, ...) __attribute__((format (printf, 3, 4)));
+
 static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 {
struct ref_dir *dir;
@@ -930,6 +933,23 @@ struct files_ref_store {
 /* Lock used for the main packed-refs file: */
 static struct lock_file packlock;
 
+void files_path(struct files_ref_store *refs, struct strbuf *sb,
+   const char *fmt, ...)
+{
+   struct strbuf tmp = STRBUF_INIT;
+   va_list vap;
+
+   va_start(vap, fmt);
+   strbuf_vaddf(&tmp, fmt, vap);
+   va_end(vap);
+   if (refs->submodule)
+   strbuf_git_path_submodule(sb, refs->submodule,
+ "%s", tmp.buf);
+   else
+   strbuf_git_path(sb, "%s", tmp.buf);
+   strbuf_release(&tmp);
+}
+
 /*
  * Increment the reference count of *packed_refs.
  */
-- 
2.11.0.157.gd943d85

Re: Bug in 'git describe' if I have two tags on the same commit.

[Kevin Daudt]

On Sun, Feb 12, 2017 at 01:15:22PM +0100, Istvan Pato wrote:
> I didn't get back the latest tag by 'git describe --tags --always' if
> I have two tags on the same commit.
> 
> // repository ppa:git-core/ppa
> 
> (master)⚡ % cat /etc/lsb-release
> DISTRIB_ID=Ubuntu
> DISTRIB_RELEASE=16.04
> DISTRIB_CODENAME=xenial
> DISTRIB_DESCRIPTION="Ubuntu 16.04.2 LTS"
> 
> (master)⚡ % git --version
> git version 2.11.0
> 
> (master) [1] % git show-ref --tag
> 76c634390... refs/tags/1.0.0
> b77c7cd17... refs/tags/1.1.0
> b77c7cd17... refs/tags/1.2.0
> 
> (master) % git describe --tags --always
> 1.1.0-1-ge9e9ced
> 
> ### Expected: 1.2.0
> 
> References:
> 
> https://www.kernel.org/pub/software/scm/git/docs/RelNotes-1.7.1.1.txt
> 
> * "git describe" did not tie-break tags that point at the same commit
>   correctly; newer ones are preferred by paying attention to the
>   tagger date now.
> 
> http://stackoverflow.com/questions/8089002/git-describe-with-two-tags-on-the-same-commit
> 
> Thanks,
> Istvan Pato

Are these lightweight tags? Only annotated tags have a date associated
to them, which is where the rel-notes refers to. 

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Johannes Schindelin]

Hi René,

On Fri, 10 Feb 2017, René Scharfe wrote:

> Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:
> > It is curious that only MacOSX builds trigger an error about this, both
> > GCC and Clang, but not Linux GCC nor Clang (see
> > https://travis-ci.org/git/git/jobs/200182819#L1152 for details):
> >
> > builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
> >   uninitialized whenever 'if' condition is true
> >   [-Werror,-Wsometimes-uninitialized]
> > if (missing_good && !missing_bad && current_term &&
> > ^~~
> > builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
> > if (!good_syn)
> >  ^~~~
> 
> The only way that good_syn could be used in the if block is by going to the
> label finish, which does the following before returning:
> 
>   if (!bad_ref)
>   free(bad_ref);
>   if (!good_glob)
>   free(good_glob);
>   if (!bad_syn)
>   free(bad_syn);
>   if (!good_syn)
>   free(good_syn);
> 
> On Linux that code is elided completely -- freeing NULL is a no-op.  I guess
> free(3) has different attributes on OS X and compilers don't dare to optimize
> it away there.
> 
> So instead of calling free(3) only in the case when we did not allocate memory
> (which makes no sense and leaks) we should either call it in the opposite
> case, or (preferred) unconditionally, as it can handle the NULL case itself.
> Once that's fixed initialization will be required even on Linux.

Exactly, free(NULL) is a no-op. The problem before this fixup was that
good_syn was not initialized to NULL.

Ciao,
Dscho

git remote rename problem with trailing \\ for remote.url config entries (on Windows)

[Marc Strapetz]

One of our users has just reported that:

$ git remote rename origin origin2

will turn following remote entry:

[remote "origin"]
url = c:\\repo\\
fetch = +refs/heads/*:refs/remotes/origin/*

into following entry for which the url is skipped:

[remote "origin2"]
[remote "origin2"]
fetch = +refs/heads/*:refs/remotes/origin2/*

I understand that this is caused by the trailing \\ and it's easy to 
fix, but 'git push' and 'git pull' work properly with such URLs and a


$ git clone c:\repo\

will even result in the problematic remote-entry. So I guess some kind 
of validation could be helpful.


Tested with git version 2.11.0.windows.1

-Marc

Re: [PATCH] mingw: use OpenSSL's SHA-1 routines

[Johannes Schindelin]

Hi Hannes,

On Sat, 11 Feb 2017, Johannes Sixt wrote:

> Am 10.02.2017 um 00:41 schrieb Junio C Hamano:
> > Johannes Schindelin  writes:
> >
> > > From: Jeff Hostetler 
> > >
> > > Use OpenSSL's SHA-1 routines rather than builtin block-sha1
> > > routines.  This improves performance on SHA1 operations on Intel
> > > processors.
> > > ...
> > >
> > > Signed-off-by: Jeff Hostetler 
> > > Signed-off-by: Johannes Schindelin 
> > > ---
> >
> > Nice.  Will queue as jh/mingw-openssl-sha1 topic; it is a bit too
> > late for today's integration cycle to be merged to 'next', but let's
> > have this by the end of the week in 'master'.
> 
> Please don't rush this through. I didn't have a chance to cross-check the
> patch; it will have to wait for Monday. I would like to address Peff's
> concerns about additional runtime dependencies.

I never meant this to be fast-tracked into git.git. We have all the time
in our lives to get this in, as Git for Windows already carries this patch
for a while, and shall continue to do so.

Ciao,
Dscho

Re: [PATCH] mingw: use OpenSSL's SHA-1 routines

[Johannes Sixt]

Am 10.02.2017 um 17:04 schrieb Jeff King:

On Fri, Feb 10, 2017 at 04:49:02PM +0100, Johannes Schindelin wrote:


I think this is only half the story. A heavy-sha1 workload is faster,
which is good. But one of the original reasons to prefer blk-sha1 (at
least on Linux) is that resolving libcrypto.so symbols takes a
non-trivial amount of time. I just timed it again, and it seems to be
consistently 1ms slower to run "git rev-parse --git-dir" on my machine
(from the top-level of a repo).

1ms is mostly irrelevant, but it adds up on scripted workloads that
start a lot of git processes.


You know my answer to that. If scripting slows things down, we should
avoid it in production code. As it is, scripting slows us down. Therefore
I work slowly but steadily to get rid of scripting where it hurts most.


Well, yes. My question is more "what does it look like on normal Git
workloads?". Are you trading off an optimization for your giant 450MB
index workload (which _also_ could be fixed by trying do the slow
operation less, rather than micro-optimizing it) in a way that hurts
people working with more normal sized repos?

For instance, "make BLK_SHA1=Yes test" is measurably faster for me than
"make BLK_SHA1= test".


The patch does add a new runtime dependency on libcrypto.dll in my 
environment. I would be surprised if it does not also with your modern 
build tools.


I haven't had time to compare test suite runtimes.


I'm open to the argument that it doesn't matter in practice for normal
git users. But it's not _just_ scripting. It depends on the user's
pattern of invoking git commands (and how expensive the symbol
resolution is on their system).


It can be argued that in normal interactive use, it is hard to notice 
that another DLL is loaded. Don't forget, though, that on Windows it is 
not only the pure time to resolve the entry points, but also that 
typically virus scanners inspect every executable file that is loaded, 
which adds another share of time.


I'll use the patch for daily work for a while to see whether it hurts.

-- Hannes

Small regression on Windows

[Johannes Sixt]

Please type this, preferably outside of any repo to avoid that it is 
changed; note the command typo:


   git -c help.autocorrect=40 ceckout

Git waits for 4 seconds, but does not write anything until just before 
it exits. It bisects to


a9b8a09c3c30886c79133da9f48ef9f98c21c3b2 is the first bad commit
commit a9b8a09c3c30886c79133da9f48ef9f98c21c3b2
Author: Jeff Hostetler 
Date:   Thu Dec 22 18:09:23 2016 +0100

mingw: replace isatty() hack

Git for Windows has carried a patch that depended on internals
of MSVC runtime, but it does not work correctly with recent MSVC
runtime. A replacement was written originally for compiling
with VC++. The patch in this message is a backport of that
replacement, and it also fixes the previous attempt to make
isatty() tell that /dev/null is *not* an interactive terminal.

Signed-off-by: Jeff Hostetler 
Tested-by: Beat Bolli 
Signed-off-by: Johannes Schindelin 
Signed-off-by: Junio C Hamano 

:04 04 bc3c75bdfc766fe478e160a9452c31bfef50b505 
f7183c3a0726fef7161d5f9ff8c997260025f1bb M  compat


Any ideas? I haven't had time to dig any further.

-- Hannes

Assalamu`Alaikum.

[mohammad ouattara]

Dear Sir/Madam.

Assalamu`Alaikum.

I am Dr mohammad ouattara, I have  ($10.6 Million us dollars) to transfer into 
your account,

I will send you more details about this deal and the procedures to follow when 
I receive a positive response from you, 

Have a great day,
Dr mohammad ouattara.

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[René Scharfe]

Am 13.02.2017 um 17:23 schrieb Johannes Schindelin:
> Hi René,
> 
> On Fri, 10 Feb 2017, René Scharfe wrote:
> 
>> Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:
>>> It is curious that only MacOSX builds trigger an error about this, both
>>> GCC and Clang, but not Linux GCC nor Clang (see
>>> https://travis-ci.org/git/git/jobs/200182819#L1152 for details):
>>>
>>> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
>>>   uninitialized whenever 'if' condition is true
>>>   [-Werror,-Wsometimes-uninitialized]
>>> if (missing_good && !missing_bad && current_term &&
>>> ^~~
>>> builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
>>> if (!good_syn)
>>>  ^~~~
>>
>> The only way that good_syn could be used in the if block is by going to the
>> label finish, which does the following before returning:
>>
>>  if (!bad_ref)
>>  free(bad_ref);
>>  if (!good_glob)
>>  free(good_glob);
>>  if (!bad_syn)
>>  free(bad_syn);
>>  if (!good_syn)
>>  free(good_syn);
>>
>> On Linux that code is elided completely -- freeing NULL is a no-op.  I guess
>> free(3) has different attributes on OS X and compilers don't dare to optimize
>> it away there.
>>
>> So instead of calling free(3) only in the case when we did not allocate 
>> memory
>> (which makes no sense and leaks) we should either call it in the opposite
>> case, or (preferred) unconditionally, as it can handle the NULL case itself.
>> Once that's fixed initialization will be required even on Linux.
> 
> Exactly, free(NULL) is a no-op. The problem before this fixup was that
> good_syn was not initialized to NULL.

Strictly speaking: no.  The value doesn't matter -- the free(3) calls
above will be done with NULL regardless, due to the conditionals.
Setting bad_syn and good_syn to an invalid pointer would have calmed
the compiler just as well, and would have had no ill side effect (i.e.
no invalid free(3) call).

Initializing to NULL is still the correct thing to do, of course --
together with removing the conditionals (or at least the negations).

But back to the topic of why the compilers on OS X didn't optimize out
the free(3) calls with their conditionals.  AFAICS no attributes are
set for the function in stdlib.h of in glibc[1] or Darwin[2].  And I
can't see any relevant option in config.mak.uname (e.g. -no-builtin).
It's not terribly important, but does anyone know what prevents the
elision of "if (!p) free(p);" on OS X?

René


[1] 
https://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/stdlib.h;h=292c6a2f053a2a578cd09d75307c26ed191e1c00;hb=b987917e6aa7ffe2fd74f0b6a989438e6edd0727
[2] 
https://opensource.apple.com/source/Libc/Libc-1158.30.7/include/stdlib.h.auto.html

Re: [PATCH] clean: use warning_errno() when appropriate

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> All these warning() calls are preceded by a system call. Report the
> actual error to help the user understand why we fail to remove
> something.

I think this patch is probably correct in the current code, but I
say this only after following what quote_path_relative() and
relative_path() that is called from it.  These warnings are preceded
by a call to a system library function, but it is not apparent that
they are immediately preceded without anything else that could have
failed in between.

Side note.  There are many calls into strbuf API in these two
functions.  Any calls to xmalloc() and friends made by strbuf
functions may see ENOMEM from underlying malloc() and recover by
releasing cached resources, by which time the original errno is
unrecoverable.  So the above "probably correct" is not strictly
true.

If we care deeply enough that we want to reliably show the errno we
got from the preceding call to a system library function even after
whatever comes in between, I think you'd need the usual saved_errno
trick.  Is that worth it?---I do not offhand have an opinion.

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  builtin/clean.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index d6bc3aaaea..dc1168747e 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -175,7 +175,7 @@ static int remove_dirs(struct strbuf *path, const char 
> *prefix, int force_flag,
>   res = dry_run ? 0 : rmdir(path->buf);
>   if (res) {
>   quote_path_relative(path->buf, prefix, "ed);
> - warning(_(msg_warn_remove_failed), quoted.buf);
> + warning_errno(_(msg_warn_remove_failed), quoted.buf);
>   *dir_gone = 0;
>   }
>   return res;
> @@ -209,7 +209,7 @@ static int remove_dirs(struct strbuf *path, const char 
> *prefix, int force_flag,
>   string_list_append(&dels, quoted.buf);
>   } else {
>   quote_path_relative(path->buf, prefix, "ed);
> - warning(_(msg_warn_remove_failed), quoted.buf);
> + warning_errno(_(msg_warn_remove_failed), 
> quoted.buf);
>   *dir_gone = 0;
>   ret = 1;
>   }
> @@ -231,7 +231,7 @@ static int remove_dirs(struct strbuf *path, const char 
> *prefix, int force_flag,
>   *dir_gone = 1;
>   else {
>   quote_path_relative(path->buf, prefix, "ed);
> - warning(_(msg_warn_remove_failed), quoted.buf);
> + warning_errno(_(msg_warn_remove_failed), quoted.buf);
>   *dir_gone = 0;
>   ret = 1;
>   }
> @@ -982,7 +982,7 @@ int cmd_clean(int argc, const char **argv, const char 
> *prefix)
>   res = dry_run ? 0 : unlink(abs_path.buf);
>   if (res) {
>   qname = quote_path_relative(item->string, NULL, 
> &buf);
> - warning(_(msg_warn_remove_failed), qname);
> + warning_errno(_(msg_warn_remove_failed), qname);
>   errors++;
>   } else if (!quiet) {
>   qname = quote_path_relative(item->string, NULL, 
> &buf);

Re: git remote rename problem with trailing \\ for remote.url config entries (on Windows)

[Junio C Hamano]

Marc Strapetz  writes:

> One of our users has just reported that:
>
> $ git remote rename origin origin2
>
> will turn following remote entry:
>
> [remote "origin"]
>   url = c:\\repo\\
>   fetch = +refs/heads/*:refs/remotes/origin/*
>
> into following entry for which the url is skipped:
>
> [remote "origin2"]
> [remote "origin2"]
>   fetch = +refs/heads/*:refs/remotes/origin2/*
>
> I understand that this is caused by the trailing \\ and it's easy to
> fix, but 'git push' and 'git pull' work properly with such URLs and a
>
> $ git clone c:\repo\
>
> will even result in the problematic remote-entry. So I guess some kind
> of validation could be helpful.

If you change the original definition of the "origin" to

[remote "origin"]
   url = "c:\\repo\\"
   fetch = +refs/heads/*:refs/remotes/origin/*

or

[remote "origin"]
   url = c:\\repo\\ # ends with bs
   fetch = +refs/heads/*:refs/remotes/origin/*

it seems to give me a better result.  I didn't dig to determine
where things go wrong in "remote rename", and it is possible that
the problem is isolated to that command, or the same problem exists
with any value that ends with a backslash.  If the latter, "git clone"
and anything that writes to configuration such a value needs to be
taught about this glitch and learn a workaround, I would think.

Dscho Cc'ed, not for Windows expertise, but as somebody who has done
a lot in .

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Junio C Hamano]

René Scharfe  writes:

> Initializing to NULL is still the correct thing to do, of course --
> together with removing the conditionals (or at least the negations).

So, let's give Pranit a concrete "here is what we want to see
squashed in", while you guys discuss peculiarity with various
platforms and their system headers, which admittedly is a more
interesting tangent ;-)

There are early returns with "goto finish" even before _syn
variables are first assigned to, so they would need to be
initialized to NULL.  The other two get their initial values
right at the beginning, so they are OK.

 builtin/bisect--helper.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8cd6527bd1..6949e8e5ca 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -280,7 +280,7 @@ static int bisect_next_check(const struct bisect_terms 
*terms,
int missing_good = 1, missing_bad = 1, retval = 0;
char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
char *good_glob = xstrfmt("%s-*", terms->term_good);
-   char *bad_syn, *good_syn;
+   char *bad_syn = NULL, *good_syn = NULL;
 
if (ref_exists(bad_ref))
missing_bad = 0;
@@ -341,14 +341,10 @@ static int bisect_next_check(const struct bisect_terms 
*terms,
}
goto finish;
 finish:
-   if (!bad_ref)
-   free(bad_ref);
-   if (!good_glob)
-   free(good_glob);
-   if (!bad_syn)
-   free(bad_syn);
-   if (!good_syn)
-   free(good_syn);
+   free(bad_ref);
+   free(good_glob);
+   free(bad_syn);
+   free(good_syn);
return retval;
 }
 

Re: [PATCH] clean: use warning_errno() when appropriate

[Jeff King]

On Mon, Feb 13, 2017 at 10:34:06AM -0800, Junio C Hamano wrote:

> Nguyễn Thái Ngọc Duy   writes:
> 
> > All these warning() calls are preceded by a system call. Report the
> > actual error to help the user understand why we fail to remove
> > something.
> 
> I think this patch is probably correct in the current code, but I
> say this only after following what quote_path_relative() and
> relative_path() that is called from it.  These warnings are preceded
> by a call to a system library function, but it is not apparent that
> they are immediately preceded without anything else that could have
> failed in between.
> 
> Side note.  There are many calls into strbuf API in these two
> functions.  Any calls to xmalloc() and friends made by strbuf
> functions may see ENOMEM from underlying malloc() and recover by
> releasing cached resources, by which time the original errno is
> unrecoverable.  So the above "probably correct" is not strictly
> true.
> 
> If we care deeply enough that we want to reliably show the errno we
> got from the preceding call to a system library function even after
> whatever comes in between, I think you'd need the usual saved_errno
> trick.  Is that worth it?---I do not offhand have an opinion.

I wonder if xmalloc() should be the one doing the saved_errno trick.
After all, it has only two outcomes: we successfully allocated the
memory, or we called die().

And that would transitively make most of the strbuf calls errno-safe
(except for obvious syscall-related ones like strbuf_read_file). And in
turn that makes quote_path_relative() pretty safe (at least when writing
to a strbuf).

-Peff

[PATCH] completion: restore removed line continuating backslash

[SZEDER Gábor]

Recent commit 1cd23e9e0 (completion: don't use __gitdir() for git
commands, 2017-02-03) rewrapped a couple of long lines, and while
doing so it inadvertently removed a '\' from the end of a line, thus
breaking completion for 'git config remote.name.push '.

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

I wanted this to be a fixup!, but then noticed that this series is
already in next, hence the proper commit message.

I see the last What's cooking marks this series as "will merge to
master".  That doesn't mean that it will be in v2.12, does it?

 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 993085af6..f2b294aac 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2086,7 +2086,7 @@ _git_config ()
remote.*.push)
local remote="${prev#remote.}"
remote="${remote%.push}"
-   __gitcomp_nl "$(__git for-each-ref
+   __gitcomp_nl "$(__git for-each-ref \
--format='%(refname):%(refname)' refs/heads)"
return
;;
-- 
2.11.1.499.ge87add2e7

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Junio C Hamano]

Arif Khokar  writes:

> ... I
> still think it would be better to be able to list the message-id
> values in the header or body of the cover letter message of a patch
> series (preferably the former) in order to facilitate downloading the
> patches via NNTP from gmane or public-inbox.org.

You are looking at builtin/log.c::make_cover_letter()?  Patches
welcome, but I think you'd need a bit of preparatory refactoring
to allow gen_message_id() be called for all messages _before_ this
function is called, as currently we generate them as we emit each
patch.

> Alternatively, or perhaps in addition to the list of message-ids, a
> list of URLs to public-inbox.org or gmane messages could also be
> provided for those who prefer to download patches via HTTP.

Many people around here do not want to repeat the mistake of relying
too much on one provider.  Listing Message-IDs may be a good idea,
listing URLs that are tied to one provider is less so.

Re: Incorrect pipe for git log graph when using --name-status option

[Junio C Hamano]

hIpPy  writes:

> The `git log` command with `graph` and pretty format works correctly
> as expected.
>
> $ git log --graph --pretty=format:'%h' -2
> * 714a14e
> * 87dce5f
>
>
> However, with `--name-status` option added, there is a pipe
> incorrectly placed after the commit hash (example below).
>
> $ git log --graph --pretty=format:'%h' -2 --name-status
> * 714a14e|
> | M README.md

Is it a --name-status, or is it your own custom format, that is
causing the above issue?

 - What happens if you stop using --pretty=format:%h and replace it
   with something like --oneline?

 - What happens if you keep using --pretty=format:%h but replace
   --name-status with something else, e.g. --raw or --stat?

Re: [PATCH] squash! completion: fill COMPREPLY directly when completing refs

[SZEDER Gábor]

On Fri, Feb 10, 2017 at 10:44 PM, Junio C Hamano  wrote:

> Should I expect a reroll to come, or is this the only fix-up to the
> series that begins at <20170203025405.8242-1-szeder@gmail.com>?
>
> No hurries.

Yes, definitely.

I found a minor bug in the middle of the series, and haven't quite
made up my mind about it and its fix yet.  Perhaps in a day or three.

Regarding the 'strip' -> 'lstrip' 'for-each-ref' format modifier
rename that broke pu: I should keep using 'strip', right?

Gábor

Re: [RFC PATCH] show decorations at the end of the line

[Linus Torvalds]

On Mon, Feb 13, 2017 at 12:30 AM, Junio C Hamano  wrote:
>
> An obvious downside is that people (against all recommendations) are
> likely to have written a loose script expecting the --oneline format
> is cast in stone.

Actually, I don't believe that is the case wrt decorations.

Why?

If you script the --oneline format and parse the output, you won't
have any decorations at all unless you are crazy (you can set
"log.decorations=true", but that will truly screw up any scripting).

And if you actually want decorations, and you're parsing them, you are
*not* going to script it with "--oneline --decorations", because the
end result is basically impossible to parse already (because it's
ambiguous - think about parentheses in the commit message).

So if you actually want decorations for parsing, you'd do something like

   git log --pretty="%h '%D' %s"

which is at least parseable (because now the decoration separator is
unconditional.

Yeah, I guess you could use "--decorations --color=always" and then
use the color codes to parse the decorations, but that's so
complicated as to be unrealistic.

And I considered adding a format string explanation, something along
the lines of

 - oneline used to mean "--pretty=%h%d %s", now it means "%h %s%d" instead

but that's actually not true. The "oneline" format was much more
complex than that, in that it has special rules for "-g", and it has
all those colorization ones too.

   Linus

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Pranit Bauva]

Hey Junio,

On Tue, Feb 14, 2017 at 12:44 AM, Junio C Hamano  wrote:
> René Scharfe  writes:
>
>> Initializing to NULL is still the correct thing to do, of course --
>> together with removing the conditionals (or at least the negations).
>
> So, let's give Pranit a concrete "here is what we want to see
> squashed in", while you guys discuss peculiarity with various
> platforms and their system headers, which admittedly is a more
> interesting tangent ;-)
>
> There are early returns with "goto finish" even before _syn
> variables are first assigned to, so they would need to be
> initialized to NULL.  The other two get their initial values
> right at the beginning, so they are OK.
>
>  builtin/bisect--helper.c | 14 +-
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 8cd6527bd1..6949e8e5ca 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -280,7 +280,7 @@ static int bisect_next_check(const struct bisect_terms 
> *terms,
> int missing_good = 1, missing_bad = 1, retval = 0;
> char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> char *good_glob = xstrfmt("%s-*", terms->term_good);
> -   char *bad_syn, *good_syn;
> +   char *bad_syn = NULL, *good_syn = NULL;
>
> if (ref_exists(bad_ref))
> missing_bad = 0;
> @@ -341,14 +341,10 @@ static int bisect_next_check(const struct bisect_terms 
> *terms,
> }
> goto finish;
>  finish:
> -   if (!bad_ref)
> -   free(bad_ref);
> -   if (!good_glob)
> -   free(good_glob);
> -   if (!bad_syn)
> -   free(bad_syn);
> -   if (!good_syn)
> -   free(good_syn);
> +   free(bad_ref);
> +   free(good_glob);
> +   free(bad_syn);
> +   free(good_syn);
> return retval;
>  }

This helps a lot ;) Thanks!

Regards,
Pranit Bauva

Re: [PATCH] mingw: use OpenSSL's SHA-1 routines

[Junio C Hamano]

Johannes Sixt  writes:

> The patch does add a new runtime dependency on libcrypto.dll in my
> environment. I would be surprised if it does not also with your modern
> build tools.
>
> I haven't had time to compare test suite runtimes.
>
>> I'm open to the argument that it doesn't matter in practice for normal
>> git users. But it's not _just_ scripting. It depends on the user's
>> pattern of invoking git commands (and how expensive the symbol
>> resolution is on their system).
>
> It can be argued that in normal interactive use, it is hard to notice
> that another DLL is loaded. Don't forget, though, that on Windows it
> is not only the pure time to resolve the entry points, but also that
> typically virus scanners inspect every executable file that is loaded,
> which adds another share of time.
>
> I'll use the patch for daily work for a while to see whether it hurts.

Thanks.

I need to ask an unrelated question at a bit higher level, though.

I have been operating under the assumption that everybody on Windows
who builds Git works off of Dscho's Git for Windows tree, and
patches that are specific to Windows from Dscho's are sent to me via
the list only after they have been in Git for Windows and proven to
help Windows users in the wild.  

The consequence of these two assumptions is that I would feel safe
to treat Windows specific changes that do not touch generic part of
the codebase from Dscho just like updates from any other subsystem
maintainers (any git-svn thing from Eric, any gitk thing from Paul,
any p4 thing Luke and Lars are both happy with, etc.).

You seem to be saying that the first of the two assumptions does not
hold.  Should I change my expectations while queuing Windows specific
patches from Dscho?

Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"

[Junio C Hamano]

Matthieu Moy  writes:

> Siddharth Kannan  writes:
>
>> +if (!strcmp(name, "-")) {
>> +name = "@{-1}";
>> +len = 5;
>> +}
>
> One drawback of this approach is that further error messages will be
> given from the "@{-1}" string that the user never typed.

Right.

> There are at least:
>
> $ git grep -n -A1 'strcmp.*"-"' | grep -B 1 '@\{1\}'
> builtin/checkout.c:975: if (!strcmp(arg, "-"))
> builtin/checkout.c-976- arg = "@{-1}";

I didn't check the surrounding context to be sure, but I think this
"- to @{-1}" conversion cannot be delegated down to revision parsing
that eventually wants to return a 40-hex as the result.  

We do want a branch _name_ sometimes when we say "@{-1}"; "checkout
master" (i.e. checkout by name) and "checkout master^0" (i.e. the
same commit object, but not by name) do different things.

> builtin/merge.c:1231:   } else if (argc == 1 && !strcmp(argv[0], "-")) {
> builtin/merge.c-1232-   argv[0] = "@{-1}";
> --
> builtin/revert.c:158:   if (!strcmp(argv[1], "-"))
> builtin/revert.c-159-   argv[1] = "@{-1}";

These should be safe to delegate down.

> builtin/worktree.c:344: if (!strcmp(branch, "-"))
> builtin/worktree.c-345- branch = "@{-1}";

I do not know about this one, but it smells like a branch name that
wants to be used before it gets turned into 40-hex.

Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"

[Junio C Hamano]

Junio C Hamano  writes:

> Matthieu Moy  writes:
>
>> Siddharth Kannan  writes:
>>
>>> +   if (!strcmp(name, "-")) {
>>> +   name = "@{-1}";
>>> +   len = 5;
>>> +   }
>>
>> One drawback of this approach is that further error messages will be
>> given from the "@{-1}" string that the user never typed.
>
> Right.
>
>> There are at least:
>>
>> $ git grep -n -A1 'strcmp.*"-"' | grep -B 1 '@\{1\}'
>> builtin/checkout.c:975: if (!strcmp(arg, "-"))
>> builtin/checkout.c-976- arg = "@{-1}";
>
> I didn't check the surrounding context to be sure, but I think this
> "- to @{-1}" conversion cannot be delegated down to revision parsing
> that eventually wants to return a 40-hex as the result.  
>
> We do want a branch _name_ sometimes when we say "@{-1}"; "checkout
> master" (i.e. checkout by name) and "checkout master^0" (i.e. the
> same commit object, but not by name) do different things.

FYI, the "@{-} to branch name" translation happens in
interpret_branch_name().  I do not offhand recall if any callers
protect their calls to the function with conditionals that assume
the thing must begin with "@{" or cannot begin with "-" (the latter
of which is similar to the topic of patch 1/2 of this series), but I
suspect that teaching the function that "-" means the same as
"@{-1}" would bring us closer to where we want to go.

Re: [PATCH v3 0/5] stash: support pathspec argument

[Jeff King]

On Mon, Feb 13, 2017 at 03:14:55PM +0100, Matthieu Moy wrote:

> I don't remember doing this intentionally. The goal was really to
> prevent typos like "git stash -p drop" (which would have been
> interpreted as "git stash save -p drop" previously). So, let's consider
> this "only messages starting with - are accepted" as a bug: moving to
> the new "push" command it a nice opportunity to fix it without worrying
> about compatibility. A bit more about this in this old thread:

Yeah, I consider it a bug that we treat "-foo" as a message there.
Fortunately I think you really have to try hard to abuse it:

  # doesn't work, because "save" complains about "-foo" as an option
  git stash -foo

  # does save stash with message "-foo"
  git stash -- -foo

  # doesn't work, because _any_ non-option argument is rejected
  git stash -- use --foo option

> I think we can safely allow both
> 
>   git stash -p -- 
>   git stash push -p 
> 
> But allowing the same without -- is a bit more dangerous for the reason
> above:
> 
>   git stash -p drop
> 
> would be interpreted as
> 
>   git stash push -p drop
> 
> (i.e. limit the stash to file named "drop"), and this would almost
> certainly not be what the user wanted.

Is it really that dangerous, though? The likely outcome is Git saying
"nope, you don't have any changes to the file named drop". Of course the
user may have meant something different, but I feel like "-p" is a good
indicator that they are interested in making an actual stash.

Think about this continuum of commands:

  1. git stash droop

  2. git stash -p drop

  3. git stash -p -- drop

  4. git stash push -p drop

I think we can all agree that (4) is reasonable and safe. And I suspect
we'd all agree that (1) is suspect (you probably meant "drop", not "push
droop").

But between (2) and (3), I don't see much difference. The interesting
difference between (1) and (2) is the addition of "-p", which tells us
that the user expects to save a stash.

Another way of thinking of it is that "-p" as a command name is an alias
for "push -p". If you wanted to err on the conservative side, you could
literally implement it that way (so "git stash -k -p foo" would not
work; you'd have to use "git stash -p -k foo").

> > The other question is how we would deal with the -m flag for
> > specifying a stash message.  I think we could special case it, but
> > that would allow users to do things such as git stash -m apply, which
> > would make the interface a bit more error prone.  So I'm leaning
> > towards disallowing that for now.
> 
> We already have "git commit -m  ", so I think stash
> should just do the same thing as commit. Or, did I miss something?

The complexity is that right now, the first-level decision of "which
stash sub-command am I running?" doesn't know about any options. So "git
stash -m foo" would be rejected in the name of typo prevention, unless
that outer decision learns about "-m" as an option.

-Peff

Re: [PATCH 01/11] refs-internal.c: make files_log_ref_write() static

[Ramsay Jones]

On 13/02/17 15:20, Nguyễn Thái Ngọc Duy wrote:
> Created in 5f3c3a4e6f (files_log_ref_write: new function - 2015-11-10)
> but probably never used outside refs-internal.c
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c | 3 +++
>  refs/refs-internal.h | 4 
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index cdb6b8ff5..75565c3aa 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -165,6 +165,9 @@ static struct ref_entry *create_dir_entry(struct 
> files_ref_store *ref_store,
> const char *dirname, size_t len,
> int incomplete);
>  static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
> +static int files_log_ref_write(const char *refname, const unsigned char 
> *old_sha1,
> +const unsigned char *new_sha1, const char *msg,
> +int flags, struct strbuf *err);

Why? I don't like adding forward declarations unless it
is absolutely necessary (ie mutually recursive functions),
and even in the current 'pu' branch (@c04899d50), the
definition of this function appears before all uses in
this file. (ie, just add static to the definition).

What am I missing?

ATB,
Ramsay Jones

>  
>  static struct ref_dir *get_ref_dir(struct ref_entry *entry)
>  {
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 33adbf93b..59e65958a 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -222,10 +222,6 @@ struct ref_transaction {
>   enum ref_transaction_state state;
>  };
>  
> -int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
> - const unsigned char *new_sha1, const char *msg,
> - int flags, struct strbuf *err);
> -
>  /*
>   * Check for entries in extras that are within the specified
>   * directory, where dirname is a reference directory name including
> 

Re: [PATCH] squash! completion: fill COMPREPLY directly when completing refs

[Junio C Hamano]

SZEDER Gábor  writes:

> On Fri, Feb 10, 2017 at 10:44 PM, Junio C Hamano  wrote:
>
>> Should I expect a reroll to come, or is this the only fix-up to the
>> series that begins at <20170203025405.8242-1-szeder@gmail.com>?
>>
>> No hurries.
>
> Yes, definitely.
>
> I found a minor bug in the middle of the series, and haven't quite
> made up my mind about it and its fix yet.  Perhaps in a day or three.
>
> Regarding the 'strip' -> 'lstrip' 'for-each-ref' format modifier
> rename that broke pu: I should keep using 'strip', right?

Right.  I view the removal of 'strip' as an accident when 'lstrip'
was introduced, not an intended change.

Re: Bug in 'git describe' if I have two tags on the same commit.

[Junio C Hamano]

Kevin Daudt  writes:

> On Sun, Feb 12, 2017 at 01:15:22PM +0100, Istvan Pato wrote:
>
>> (master) [1] % git show-ref --tag
>> 76c634390... refs/tags/1.0.0
>> b77c7cd17... refs/tags/1.1.0
>> b77c7cd17... refs/tags/1.2.0
>> 
>> (master) % git describe --tags --always
>> 1.1.0-1-ge9e9ced
>> 
>> ### Expected: 1.2.0
>> ...
>
> Are these lightweight tags? Only annotated tags have a date associated
> to them, which is where the rel-notes refers to. 

Good eyes.  The fact that the two points at the same object means
that even if they were both annotated tags, they have the same
tagger dates.

If the code that compares the candidates and picks better tag to
describe the object with knows the refnames of these "tags", I'd
imagine that we could use the versioncmp() logic as the final tie
breaker, but I do not offhand remember if the original refname we
took the tag (or commit) from is propagated that deep down the
callchain.  It probably does not, so some code refactoring may be
needed if somebody wants to go in that direction.

[PATCH] docs/gitremote-helpers: fix unbalanced quotes

[Jeff King]

Each of these options is missing the closing single-quote on
the option name. This understandably confuses asciidoc,
which ends up rendering a stray quote, like:

  option cloning {'true|false}

Signed-off-by: Jeff King 
---
 Documentation/gitremote-helpers.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/gitremote-helpers.txt 
b/Documentation/gitremote-helpers.txt
index 9e8681f9e..7e59c50b1 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -452,14 +452,14 @@ set by Git if the remote helper has the 'option' 
capability.
Request the helper to perform a force update.  Defaults to
'false'.
 
-'option cloning {'true'|'false'}::
+'option cloning' {'true'|'false'}::
Notify the helper this is a clone request (i.e. the current
repository is guaranteed empty).
 
-'option update-shallow {'true'|'false'}::
+'option update-shallow' {'true'|'false'}::
Allow to extend .git/shallow if the new refs require it.
 
-'option pushcert {'true'|'false'}::
+'option pushcert' {'true'|'false'}::
GPG sign pushes.
 
 SEE ALSO
-- 
2.12.0.rc1.466.g70234cfd8

Re: [PATCH 02/11] files-backend: convert git_path() to strbuf_git_path()

[Ramsay Jones]

On 13/02/17 15:20, Nguyễn Thái Ngọc Duy wrote:
> git_path() and friends are going to be killed in files-backend.c in near
> future. And because there's a risk with overwriting buffer in
> git_path(), let's convert them all to strbuf_git_path(). We'll have
> easier time killing/converting strbuf_git_path() then because we won't
> have to worry about memory management again.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c | 114 
> ---
>  1 file changed, 90 insertions(+), 24 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 75565c3aa..6582c9b2d 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2169,6 +2169,8 @@ static int lock_packed_refs(struct files_ref_store 
> *refs, int flags)
>   static int timeout_configured = 0;
>   static int timeout_value = 1000;
>   struct packed_ref_cache *packed_ref_cache;
> + struct strbuf sb = STRBUF_INIT;
> + int ret;
>  
>   files_assert_main_repository(refs, "lock_packed_refs");
>  
> @@ -2177,10 +2179,13 @@ static int lock_packed_refs(struct files_ref_store 
> *refs, int flags)
>   timeout_configured = 1;
>   }
>  
> - if (hold_lock_file_for_update_timeout(
> - &packlock, git_path("packed-refs"),
> - flags, timeout_value) < 0)
> + strbuf_git_path(&sb, "packed-refs");
> + ret = hold_lock_file_for_update_timeout(&packlock, sb.buf,
> + flags, timeout_value);
> + strbuf_release(&sb);
> + if (ret < 0)
>   return -1;
> +
>   /*
>* Get the current packed-refs while holding the lock.  If the
>* packed-refs file has been modified since we last read it,
> @@ -2335,6 +2340,9 @@ static void try_remove_empty_parents(char *name)
>   for (q = p; *q; q++)
>   ;
>   while (1) {
> + struct strbuf sb = STRBUF_INIT;
> + int ret;
> +
>   while (q > p && *q != '/')
>   q--;
>   while (q > p && *(q-1) == '/')
> @@ -2342,7 +2350,10 @@ static void try_remove_empty_parents(char *name)
>   if (q == p)
>   break;
>   *q = '\0';
> - if (rmdir(git_path("%s", name)))
> + strbuf_git_path(&sb, "%s", name);
> + ret = rmdir(sb.buf);
> + strbuf_release(&sb);
> + if (ret)
>   break;
>   }
>  }
> @@ -2431,7 +2442,11 @@ static int repack_without_refs(struct files_ref_store 
> *refs,
>   return 0; /* no refname exists in packed refs */
>  
>   if (lock_packed_refs(refs, 0)) {
> - unable_to_lock_message(git_path("packed-refs"), errno, err);
> + struct strbuf sb = STRBUF_INIT;
> +
> + strbuf_git_path(&sb, "packed-refs");
> + unable_to_lock_message(sb.buf, errno, err);
> + strbuf_release(&sb);
>   return -1;
>   }
>   packed = get_packed_refs(refs);
> @@ -2529,11 +2544,12 @@ static int rename_tmp_log(const char *newrefname)
>  {
>   int attempts_remaining = 4;
>   struct strbuf path = STRBUF_INIT;
> + struct strbuf tmp_renamed_log = STRBUF_INIT;
>   int ret = -1;
>  
> - retry:
> - strbuf_reset(&path);
>   strbuf_git_path(&path, "logs/%s", newrefname);
> + strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
> + retry:
>   switch (safe_create_leading_directories_const(path.buf)) {
>   case SCLD_OK:
>   break; /* success */
> @@ -2546,7 +2562,7 @@ static int rename_tmp_log(const char *newrefname)
>   goto out;
>   }
>  
> - if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
> + if (rename(tmp_renamed_log.buf, path.buf)) {
>   if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 
> 0) {
>   /*
>* rename(a, b) when b is an existing
> @@ -2574,6 +2590,7 @@ static int rename_tmp_log(const char *newrefname)
>   ret = 0;
>  out:
>   strbuf_release(&path);
> + strbuf_release(&tmp_renamed_log);
>   return ret;
>  }
>  
> @@ -2614,9 +2631,15 @@ static int files_rename_ref(struct ref_store 
> *ref_store,
>   int flag = 0, logmoved = 0;
>   struct ref_lock *lock;
>   struct stat loginfo;
> - int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
> + struct strbuf sb_oldref = STRBUF_INIT;
> + struct strbuf sb_newref = STRBUF_INIT;
> + struct strbuf tmp_renamed_log = STRBUF_INIT;
> + int log, ret;
>   struct strbuf err = STRBUF_INIT;
>  
> + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
> + log = !lstat(sb_oldref.buf, &loginfo);
> + strbuf_release(&sb_oldref);
>   if (log && S_ISLNK(loginfo.st_mode))
>   return error("reflog for %s is a symlink", oldrefname);
>  
> @@ -2630,7 +2653,12 @@ stat

Re: [PATCH] clean: use warning_errno() when appropriate

[Junio C Hamano]

Jeff King  writes:

> I wonder if xmalloc() should be the one doing the saved_errno trick.
> After all, it has only two outcomes: we successfully allocated the
> memory, or we called die().

I would be lying if I said I did not considered it when I wrote the
message you are responding to, but I rejected it because that would
be optimizing for a wrong case, in that most callers of xmalloc()
and friends do not do so in the error codepath, and we would be
penalizing them by doing the saved_errno dance unconditionally.

Re: Small regression on Windows

[Jeff Hostetler]

On 2/13/2017 1:00 PM, Johannes Sixt wrote:
Please type this, preferably outside of any repo to avoid that it is 
changed; note the command typo:


   git -c help.autocorrect=40 ceckout

Git waits for 4 seconds, but does not write anything until just before 
it exits. It bisects to


a9b8a09c3c30886c79133da9f48ef9f98c21c3b2 is the first bad commit
commit a9b8a09c3c30886c79133da9f48ef9f98c21c3b2
Author: Jeff Hostetler 
Date:   Thu Dec 22 18:09:23 2016 +0100

mingw: replace isatty() hack

Git for Windows has carried a patch that depended on internals
of MSVC runtime, but it does not work correctly with recent MSVC
runtime. A replacement was written originally for compiling
with VC++. The patch in this message is a backport of that
replacement, and it also fixes the previous attempt to make
isatty() tell that /dev/null is *not* an interactive terminal.

Signed-off-by: Jeff Hostetler 
Tested-by: Beat Bolli 
Signed-off-by: Johannes Schindelin 
Signed-off-by: Junio C Hamano 

:04 04 bc3c75bdfc766fe478e160a9452c31bfef50b505 
f7183c3a0726fef7161d5f9ff8c997260025f1bb M  compat


Any ideas? I haven't had time to dig any further.


I'm investigating now.

The warning text is being written to stderr and then main thread sleeps 
for the 4 seconds.
However, stderr has been redirected to the pipe connected to the 
console_thread that
handles the coloring/pager.  It is in a blocking ReadFile on the pipe 
and is thus stuck until
the main thread runs the corrected command and closes the pipe.  It then 
sees the EOF

and prints everything in the pipe buffer.

I'll look into fixing this now.

Jeff

IT System Alerts

[Melanie Fries]

Help Desk

This e-mail has been sent to you by Outlook Web App If you do not agree to 
update your account, your email account will be blocked.

Click Her to update

Sincerely,
IT-Service Help Desk

Re: [PATCH 03/11] files-backend: add files_path()

[Ramsay Jones]

On 13/02/17 15:20, Nguyễn Thái Ngọc Duy wrote:
> This will be the replacement for both git_path() and git_path_submodule()
> in this file. The idea is backend takes a git path and use that,
> oblivious of submodule, linked worktrees and such.
> 
> This is the middle step towards that. Eventually the "submodule" field
> in 'struct files_ref_store' should be replace by "gitdir". And a
> compound ref_store is created to combine two files backends together,
> one represents the shared refs in $GIT_COMMON_DIR, one per-worktree. At
> that point, files_path() becomes a wrapper of strbuf_vaddf().
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6582c9b2d..39217a2ca 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -169,6 +169,9 @@ static int files_log_ref_write(const char *refname, const 
> unsigned char *old_sha
>  const unsigned char *new_sha1, const char *msg,
>  int flags, struct strbuf *err);
>  
> +void files_path(struct files_ref_store *refs, struct strbuf *sb,
> + const char *fmt, ...) __attribute__((format (printf, 3, 4)));
> +

Why?

>  static struct ref_dir *get_ref_dir(struct ref_entry *entry)
>  {
>   struct ref_dir *dir;
> @@ -930,6 +933,23 @@ struct files_ref_store {
>  /* Lock used for the main packed-refs file: */
>  static struct lock_file packlock;
>  
> +void files_path(struct files_ref_store *refs, struct strbuf *sb,
> + const char *fmt, ...)
> +{
> + struct strbuf tmp = STRBUF_INIT;
> + va_list vap;
> +
> + va_start(vap, fmt);
> + strbuf_vaddf(&tmp, fmt, vap);
> + va_end(vap);
> + if (refs->submodule)
> + strbuf_git_path_submodule(sb, refs->submodule,
> +   "%s", tmp.buf);
> + else
> + strbuf_git_path(sb, "%s", tmp.buf);
> + strbuf_release(&tmp);
> +}
> +
>  /*
>   * Increment the reference count of *packed_refs.
>   */
> 

ATB,
Ramsay Jones

Re: [PATCH] completion: restore removed line continuating backslash

[Junio C Hamano]

SZEDER Gábor  writes:

> Recent commit 1cd23e9e0 (completion: don't use __gitdir() for git
> commands, 2017-02-03) rewrapped a couple of long lines, and while
> doing so it inadvertently removed a '\' from the end of a line, thus
> breaking completion for 'git config remote.name.push '.
>
> Signed-off-by: SZEDER Gábor 
> ---
>
> I wanted this to be a fixup!, but then noticed that this series is
> already in next, hence the proper commit message.

We get "--format=... : command not found"?
Thanks for a set of sharp eyes.

> I see the last What's cooking marks this series as "will merge to
> master".  That doesn't mean that it will be in v2.12, does it?

I actually was hoping that these can go in.  Others that can (or
should) wait are marked as "Will cook in 'next'".

If you feel uncomfortable and want these to cook longer, please tell
me so.

Thanks.

>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 993085af6..f2b294aac 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2086,7 +2086,7 @@ _git_config ()
>   remote.*.push)
>   local remote="${prev#remote.}"
>   remote="${remote%.push}"
> - __gitcomp_nl "$(__git for-each-ref
> + __gitcomp_nl "$(__git for-each-ref \
>   --format='%(refname):%(refname)' refs/heads)"
>   return
>   ;;

Re: Small regression on Windows

[Junio C Hamano]

Jeff Hostetler  writes:

> The warning text is being written to stderr and then main thread
> sleeps for the 4 seconds.  However, stderr has been redirected to
> the pipe connected to the console_thread that handles the
> coloring/pager.  It is in a blocking ReadFile on the pipe and is
> thus stuck until the main thread runs the corrected command and
> closes the pipe.  It then sees the EOF and prints everything in
> the pipe buffer.

IOW, somehow your stderr is not flushing automatically?

> I'll look into fixing this now.

Thanks.

Re: Incorrect pipe for git log graph when using --name-status option

[hIpPy]

On Mon, Feb 13, 2017 at 11:25 AM, Junio C Hamano  wrote:
> hIpPy  writes:
>
>> The `git log` command with `graph` and pretty format works correctly
>> as expected.
>>
>> $ git log --graph --pretty=format:'%h' -2
>> * 714a14e
>> * 87dce5f
>>
>>
>> However, with `--name-status` option added, there is a pipe
>> incorrectly placed after the commit hash (example below).
>>
>> $ git log --graph --pretty=format:'%h' -2 --name-status
>> * 714a14e|
>> | M README.md
>
> Is it a --name-status, or is it your own custom format, that is
> causing the above issue?
>
>  - What happens if you stop using --pretty=format:%h and replace it
>with something like --oneline?
>
>  - What happens if you keep using --pretty=format:%h but replace
>--name-status with something else, e.g. --raw or --stat?
>



 - What happens if you stop using --pretty=format:%h and replace it
   with something like --oneline?
--oneline works correctly as expected (example below).

$ git log --graph --oneline -2 --name-status
* bf7ace7daf (HEAD -> rm/option-for-name-status) wip: --ns for --name-status
| M diff.c
*   592e5c5bce (origin/master, origin/HEAD, master) Merge pull request
#994 from jeffhostetler/jeffhostetler/fscache_nfd
|\


 - What happens if you keep using --pretty=format:%h but replace
   --name-status with something else, e.g. --raw or --stat?
I see the same issue with --raw and --stat (examples below).

$ git log --graph --pretty=format:'%h' -2 --raw
* bf7ace7daf|
| :100644 100644 84dba60c40... 87dfabf4a9... M  diff.c

*   592e5c5bce
|\

$ git log --graph --pretty=format:'%h' -2 --stat
* bf7ace7daf|
|  diff.c | 2 +-
|  1 file changed, 1 insertion(+), 1 deletion(-)

*   592e5c5bce
|\

I believe it's not my custom format that is causing the issue.
I'm including this information that may not be relevant. I apologize
in advance for that. I had simplified the custom format in my
previous email. For below custom format I still see the pipe
incorrectly placed.

$ git log --graph --date=relative --decorate --abbrev-commit
--format=format:'%h - %aD (%ar)%d%n %s - %an' -2 --name-status
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar|
| M diff.c

*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin


If I were to put a '%n' at the end (example below), the pipe is
correctly placed with or without the --name-status option. But in
case of "without the --name-status option", there is an extra blank
line. It seems that my custom format is requiring a %n at the end. I
consider my custom format as a --twoline option and I feel the
behavior should match --oneline when using options.

With --name-status: This works correctly.

$ git log --graph --date=relative --decorate --abbrev-commit
--format=format:'%h - %aD (%ar)%d%n %s - %an%n' -2 --name-status
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar
|
| M diff.c
|
*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin

Without --name-status: This works but has extra blank line between
commits though.

$ git log --graph --date=relative --decorate --abbrev-commit
--format=format:'%h - %aD (%ar)%d%n %s - %an%n' -2
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar
|
*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin

I think that requiring to end custom formats with %n with options
is not good. So I think this is a bug.

Thanks,
RM

Re: [PATCH 04/11] files-backend: replace *git_path*() with files_path()

[Ramsay Jones]

On 13/02/17 15:20, Nguyễn Thái Ngọc Duy wrote:
> This centralizes all path rewriting of files-backend.c in one place so
> we have easier time removing the path rewriting later. There could be
> some hidden indirect git_path() though, I didn't audit the code to the
> bottom.
> 
> Side note: set_worktree_head_symref() is a bad boy and should not be in
> files-backend.c (probably should not exist in the first place). But
> we'll leave it there until we have better multi-worktree support in refs
> before we update it.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c | 193 
> ++-
>  1 file changed, 98 insertions(+), 95 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 39217a2ca..c69e4fe84 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -165,12 +165,13 @@ static struct ref_entry *create_dir_entry(struct 
> files_ref_store *ref_store,
> const char *dirname, size_t len,
> int incomplete);
>  static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
> -static int files_log_ref_write(const char *refname, const unsigned char 
> *old_sha1,
> +static int files_log_ref_write(struct files_ref_store *refs,
> +const char *refname, const unsigned char 
> *old_sha1,
>  const unsigned char *new_sha1, const char *msg,
>  int flags, struct strbuf *err);
>  
> -void files_path(struct files_ref_store *refs, struct strbuf *sb,
> - const char *fmt, ...) __attribute__((format (printf, 3, 4)));

You only added this in the last commit, so maybe mark it static in
the previous patch! Also, just in case you were wondering, the 'Why?'
of the previous email was, "Why do you need this forward declaration?"
(hint: you don't ;-)

> +static void files_path(struct files_ref_store *refs, struct strbuf *sb,
> +const char *fmt, ...) __attribute__((format (printf, 3, 
> 4)));
>  
>  static struct ref_dir *get_ref_dir(struct ref_entry *entry)
>  {
> @@ -933,8 +934,8 @@ struct files_ref_store {
>  /* Lock used for the main packed-refs file: */
>  static struct lock_file packlock;
>  
> -void files_path(struct files_ref_store *refs, struct strbuf *sb,
> - const char *fmt, ...)
> +static void files_path(struct files_ref_store *refs, struct strbuf *sb,
> +const char *fmt, ...)
>  {
>   struct strbuf tmp = STRBUF_INIT;
>   va_list vap;

ATB,
Ramsay Jones

Re: [PATCH] rm: reuse strbuf for all remove_dir_recursively() calls, again

[Stefan Beller]

On Sat, Feb 11, 2017 at 11:51 AM, René Scharfe  wrote:
> Don't throw the memory allocated for remove_dir_recursively() away after
> a single call, use it for the other entries as well instead.
>
> This change was done before in deb8e15a (rm: reuse strbuf for all
> remove_dir_recursively() calls), but was reverted as a side-effect of
> 55856a35 (rm: absorb a submodules git dir before deletion). Reinstate
> the optimization.
>
> Signed-off-by: Rene Scharfe 
> ---
> Was deb8e15a a rebase victim?

(I do not recall it off the top of my head)

That commit was merged at 03f25e85,
Merge branch 'rs/rm-strbuf-optim', but it looks
like it was reverted as part of 55856a35b2
(rm: absorb a submodules git dir before deletion)

Looking through the discussion at
https://public-inbox.org/git/xmqqmvfich2e@gitster.mtv.corp.google.com/
there was no apparent signs of confusion, but a reroll was promised, that
I cannot find on the list.

Anyway, the patch below looks good to me.

Thanks,
Stefan

>
>  builtin/rm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 452170a3ab..fb79dcab18 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -360,15 +360,14 @@ int cmd_rm(int argc, const char **argv, const char 
> *prefix)
>  */
> if (!index_only) {
> int removed = 0, gitmodules_modified = 0;
> +   struct strbuf buf = STRBUF_INIT;
> for (i = 0; i < list.nr; i++) {
> const char *path = list.entry[i].name;
> if (list.entry[i].is_submodule) {
> -   struct strbuf buf = STRBUF_INIT;
> -
> +   strbuf_reset(&buf);
> strbuf_addstr(&buf, path);
> if (remove_dir_recursively(&buf, 0))
> die(_("could not remove '%s'"), path);
> -   strbuf_release(&buf);
>
> removed = 1;
> if (!remove_path_from_gitmodules(path))
> @@ -382,6 +381,7 @@ int cmd_rm(int argc, const char **argv, const char 
> *prefix)
> if (!removed)
> die_errno("git rm: '%s'", path);
> }
> +   strbuf_release(&buf);
> if (gitmodules_modified)
> stage_updated_gitmodules();
> }
> --
> 2.11.1
>

Re: [PATCH] mingw: use OpenSSL's SHA-1 routines

[Johannes Sixt]

Am 13.02.2017 um 20:42 schrieb Junio C Hamano:

I have been operating under the assumption that everybody on Windows
who builds Git works off of Dscho's Git for Windows tree, and
patches that are specific to Windows from Dscho's are sent to me via
the list only after they have been in Git for Windows and proven to
help Windows users in the wild.

The consequence of these two assumptions is that I would feel safe
to treat Windows specific changes that do not touch generic part of
the codebase from Dscho just like updates from any other subsystem
maintainers (any git-svn thing from Eric, any gitk thing from Paul,
any p4 thing Luke and Lars are both happy with, etc.).

You seem to be saying that the first of the two assumptions does not
hold.  Should I change my expectations while queuing Windows specific
patches from Dscho?


Your first assumption is incorrect as far as I am concerned. I build 
from your tree plus some topics. During -rc period, I build off of 
master; after a release, I build off of next. I merge some of the topics 
that you carry in pu when I find them interesting or when I suspect them 
to regress on Windows. Then I carry around a few additional patches that 
the public has never seen, and these days I also merge Dscho's rebase-i 
topic.


-- Hannes

Re: [RFC PATCH] show decorations at the end of the line

[Junio C Hamano]

Linus Torvalds  writes:

> And if you actually want decorations, and you're parsing them, you are
> *not* going to script it with "--oneline --decorations", because the
> end result is basically impossible to parse already (because it's
> ambiguous - think about parentheses in the commit message).

OK.  So let's wait to hear from others if they like the "obviously"
improved output.  Even though I find the decorations indispensable
in my "git log" output, I personally do not have much preference
either way, as my screen is often wide enough ;-)

Thanks.  We'd need to update the tests that expects the old style
output, though.

Re: [PATCH] clean: use warning_errno() when appropriate

[Jeff King]

On Mon, Feb 13, 2017 at 12:38:47PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I wonder if xmalloc() should be the one doing the saved_errno trick.
> > After all, it has only two outcomes: we successfully allocated the
> > memory, or we called die().
> 
> I would be lying if I said I did not considered it when I wrote the
> message you are responding to, but I rejected it because that would
> be optimizing for a wrong case, in that most callers of xmalloc()
> and friends do not do so in the error codepath, and we would be
> penalizing them by doing the saved_errno dance unconditionally.

Yes, that also occurred to me. I'm not sure if two integer swaps is
enough to care about when compared to the cost of a malloc(), though.

IOW, I think this may be a case where we should be optimizing for
programmer time (fewer lines of code, and one less thing to worry about
in the callers) versus squeezing out every instruction.

-Peff

[PATCH] docs/git-submodule: fix unbalanced quote

[Jeff King]

The documentation gives an example of the submodule foreach
command that uses both backticks and single-quotes. We stick
the whole thing inside "+" markers to make it monospace, but
the inside punctuation still needs escaping. We handle the
backticks with "{backtick}", and use backslash-escaping for
the single-quotes.

But we missed the escaping on the second quote. Fortunately,
asciidoc renders this unbalanced quote as we want (showing
the quote), but asciidoctor does not. We could fix it by
adding the missing backslash.

However, let's take a step back. Even when rendered
correctly, it's hard to read a long command stuck into the
middle of a paragraph, and the important punctuation is hard
to notice. Let's instead bump it into its own single-line
code block. That makes both the source and the rendered
result more readable, and as a bonus we don't have to worry
about quoting at all.

Signed-off-by: Jeff King 
---
Not textually related to the previous fix, but obviously along the same
lines.

 Documentation/git-submodule.txt | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 918bd1d1b..a8eb1c7ce 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -228,9 +228,12 @@ foreach::
the processing to terminate. This can be overridden by adding '|| :'
to the end of the command.
 +
-As an example, +git submodule foreach \'echo $path {backtick}git
-rev-parse HEAD{backtick}'+ will show the path and currently checked out
-commit for each submodule.
+As an example, the command below will show the path and currently
+checked out commit for each submodule:
++
+--
+git submodule foreach 'echo $path `git rev-parse HEAD`'
+--
 
 sync::
Synchronizes submodules' remote URL configuration setting
-- 
2.12.0.rc1.466.g70234cfd8

Re: Incorrect pipe for git log graph when using --name-status option

[Jeff King]

On Mon, Feb 13, 2017 at 12:55:07PM -0800, hIpPy wrote:

> I think that requiring to end custom formats with %n with options
> is not good. So I think this is a bug.

Yes, you don't normally need to do that.

I think the problem is specifically related to the "terminator" versus
"separator" semantics. Try:

  git log --graph --name-status --pretty=format:%h

versus:

  git log --graph --name-status --pretty=tformat:%h

The latter works fine. The problem seems to happen with all diff
formats, so I think the issue is that git is not aggressive enough about
inserting the newline at the right spot when using separator mode.

-Peff

Re: Incorrect pipe for git log graph when using --name-status option

[Junio C Hamano]

Jeff King  writes:

> I think the problem is specifically related to the "terminator" versus
> "separator" semantics. Try:
>
>   git log --graph --name-status --pretty=format:%h
>
> versus:
>
>   git log --graph --name-status --pretty=tformat:%h
>
> The latter works fine. The problem seems to happen with all diff
> formats, so I think the issue is that git is not aggressive enough about
> inserting the newline at the right spot when using separator mode.

I guess that is one of the reasons why we made --format=%h a synonym
to --pretty=tformat:%h and not --pretty=format:%h ;-)

Re: Incorrect pipe for git log graph when using --name-status option

[Jeff King]

On Mon, Feb 13, 2017 at 01:18:40PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I think the problem is specifically related to the "terminator" versus
> > "separator" semantics. Try:
> >
> >   git log --graph --name-status --pretty=format:%h
> >
> > versus:
> >
> >   git log --graph --name-status --pretty=tformat:%h
> >
> > The latter works fine. The problem seems to happen with all diff
> > formats, so I think the issue is that git is not aggressive enough about
> > inserting the newline at the right spot when using separator mode.
> 
> I guess that is one of the reasons why we made --format=%h a synonym
> to --pretty=tformat:%h and not --pretty=format:%h ;-)

Yeah. I have never found "--pretty=format" to do what I want versus
tformat. I wish we could just get rid of it, but I think it is likely to
be used as a plumbing interface.

So I think there probably _is_ a bug in it to be fixed, but my immediate
response is "don't ever use --pretty=format:".

-Peff

Re: [RFH] Request for Git Merge 2017 impressions for Git Rev News

[Christian Couder]

Hi,

On Sat, Feb 11, 2017 at 5:33 PM, Jakub Narębski  wrote:
> Hello,
>
> Git Rev News #24 is planned to be released on February 15. It is meant
> to cover what happened during the month of January 2017 (and earely
> February 2017) and the Git Merge 2017 conference that happened on
> February 2nd and 3rd 2017.

Yeah, I would have liked to release Git Rev News #24 on February 15
but I was busy and tired this week end, so let it be on Wednesday
February 22.

> A draft of a new Git Rev News edition is available here:
>
>   
> https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-24.md
>
> I would like to ask everyone who attended the conference (and the
> GitMerge 2017 Contributors’s Summit day before it), or watched it live
> at http://git-merge.com/watch to write his or her impressions.
>
> You can contribute either by replying to this email, or by editing the
> above page on GitHub and sending a pull request, or by commenting on
> the following GitHub issue about Git Rev News 24:
>
>   https://github.com/git/git.github.io/issues/221
>
> If you prefer to post on your own blog (or if you have did it
> already), please send an URL.

Yeah, any material (link, short impression, article, ...) would be very nice.
Thanks Jakub for the links you already added to the draft, by the way!

> P.S. I wonder if there should be not a separate section on
> https://git.github.io/ about recollection from various Git-related
> events, with Git Merge 2017 as the first one.  This way we can wait
> for later response, and incorporate videos and slides from events, as
> they begin to be available.

Jakub, if you are willing to create and maintain this section, that
would be great!

> P.P.S. Please distribute this information more widely.

Thanks,
Christian.

Employee interviews

[CG]

Is GIT SCM conducting interviews via Google chat? I'm responding to a zip 
recruiter job listing. 
Thank You. Just looking for some official company information such as an office 
address, work phone number, or business email domain. Thank You. 

CG 

Re: Incorrect pipe for git log graph when using --name-status option

[hIpPy]

On Mon, Feb 13, 2017 at 1:27 PM, Jeff King  wrote:
> On Mon, Feb 13, 2017 at 01:18:40PM -0800, Junio C Hamano wrote:
>
>> Jeff King  writes:
>>
>> > I think the problem is specifically related to the "terminator" versus
>> > "separator" semantics. Try:
>> >
>> >   git log --graph --name-status --pretty=format:%h
>> >
>> > versus:
>> >
>> >   git log --graph --name-status --pretty=tformat:%h
>> >
>> > The latter works fine. The problem seems to happen with all diff
>> > formats, so I think the issue is that git is not aggressive enough about
>> > inserting the newline at the right spot when using separator mode.
>>
>> I guess that is one of the reasons why we made --format=%h a synonym
>> to --pretty=tformat:%h and not --pretty=format:%h ;-)
>
> Yeah. I have never found "--pretty=format" to do what I want versus
> tformat. I wish we could just get rid of it, but I think it is likely to
> be used as a plumbing interface.
>
> So I think there probably _is_ a bug in it to be fixed, but my immediate
> response is "don't ever use --pretty=format:".
>
> -Peff

I have 1 issue with tformat in that I feel it reduces
readability a bit when using options. But I can use it if that
is recommended over the other.

Without --name-status: This is OK.

$ git log --graph --date=relative --decorate --abbrev-commit
--format='%h - %aD (%ar)%d%n %s - %an' -2
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar
*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin


With --name-status: I'm sorry if I nitpick here but I think the
--name-status items should either be preceeded and followed by
blank line OR not (as in oneline) but not just preceded (example
below).

$ git log --graph --date=relative --decorate --abbrev-commit
--format='%h - %aD (%ar)%d%n %s - %an' -2 --name-status
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar
|
| M diff.c
*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin


So either of below is better that above (I feel):

Blank lines before and after --name-status items:

$ git log --graph --date=relative --decorate --abbrev-commit
--format='%h - %aD (%ar)%d%n %s - %an' -2 --name-status
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar
|
| M diff.c
|
*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin

No blank lines before and after --name-status items:

$ git log --graph --date=relative --decorate --abbrev-commit
--format='%h - %aD (%ar)%d%n %s - %an' -2 --name-status
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar
| M diff.c
*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin

What do guys think?

If there is a way to get what I want using tformat?


Thanks,
RM

Re: [PATCH v3 0/5] stash: support pathspec argument

[Jeff King]

On Mon, Feb 13, 2017 at 10:35:31PM +0100, Matthieu Moy wrote:

> > Is it really that dangerous, though? The likely outcome is Git saying
> > "nope, you don't have any changes to the file named drop". Of course the
> > user may have meant something different, but I feel like "-p" is a good
> > indicator that they are interested in making an actual stash.
> 
> Indeed -p is not the best example. In the old thread, I used -q which is
> much more problematic:
> 
>   git stash -q drop => interpreted as: git stash push -q drop
>   git stash drop -q => drop with option -q

Yeah, I'd agree with that. I wouldn't propose to loosen it entirely, but
rather to treat "-p" specially.

> It's not really "dangerous" at least in this case, since we misinterpret
> a destructive command for a less destructive one, but it is rather
> confusing that changing the order between command and options change the
> behavior.
> 
> I actually find it a reasonable expectation to allow swapping commands
> and options, some programs other than git allow it.

I think we may have already crossed that bridge with "git -p stash".

Not to mention that the ordering already _is_ relevant (we disallow one
order but not the other). If we really wanted to allow swapping, it
would mean making:

  git stash -p drop

the same as:

  git stash drop -p

I actually find _that_ more confusing. It would perhaps make more sense
with something like "-q", which is more of a "global" option than a
command-specific one. But I think we'd want to whitelist such global
options (and "-p" would not be on that list).

> > The complexity is that right now, the first-level decision of "which
> > stash sub-command am I running?" doesn't know about any options. So "git
> > stash -m foo" would be rejected in the name of typo prevention, unless
> > that outer decision learns about "-m" as an option.
> 
> Ah, OK. But that's not really hard to implement: when going through the
> option list looking for non-option, shift one more time when finding -m.

No, it's not hard conceptually. It just means implementing the
option-parsing policy in two places. That's not too bad now, but if we
started using rev-parse's options helper, then I think you have corner
cases like "git stash -km foo".

My "-p" suggestion suffers from a similar problem if you treat it as
"you can omit the 'push' if you say "-p", rather than "if -p is the
first option, it is a synonym for 'push -p'".

-Peff

Re: Incorrect pipe for git log graph when using --name-status option

[Jeff King]

On Mon, Feb 13, 2017 at 01:43:45PM -0800, hIpPy wrote:

> With --name-status: I'm sorry if I nitpick here but I think the
> --name-status items should either be preceeded and followed by
> blank line OR not (as in oneline) but not just preceded (example
> below).

Yeah, I agree the output is a bit ugly.

I don't think there is a way to do what you want currently. The
--oneline code path has some internal magic that suppresses some of the
newlines.

I think it is a good goal that anything that can be expressed via one of
the regular formats (like --oneline) can be expressed via custom
formats. I don't know offhand how hard it would be to make this
particular case work. If somebody is interested in tackling this, they'd
probably need to dig around in pretty.c to see where the ONELINE code
path diverge from the USER_FORMAT one, and then figure out how a user
can specify that they want the ONELINE-ish semantics for their format.

-Peff

Re: [PATCH] clean: use warning_errno() when appropriate

[Junio C Hamano]

Jeff King  writes:

> IOW, I think this may be a case where we should be optimizing for
> programmer time (fewer lines of code, and one less thing to worry about
> in the callers) versus squeezing out every instruction.

Fair enough.

Unless we do the save_errno dance in all the helper functions we
commonly use to safely stash away errno as necessary and tell
developers that they can depend on it, the code in the patch that
began this discussion still needs its own saved_errno dance to be
safe, though.  I do not have a feeling that we are not there yet,
even after we teach xmalloc() and its family to do so.

Re: [PATCH v3 4/5] stash: introduce new format create

[Jeff King]

On Sat, Feb 11, 2017 at 02:51:27PM +, Thomas Gummerer wrote:

> > How do we tell the difference between new-style invocations, and
> > old-style ones that look new-style? IOW, I think:
> > 
> >   git stash create -m works
> > 
> > currently treats "-m works" as the full message, and it would now become
> > just "works".
> > 
> > That may be an acceptable loss for the benefit we are getting. The
> > alternative is to make yet another verb for create, as we did with
> > save/push). I have a feeling that hardly anybody uses "create", though,
> > and it's mostly an implementation detail. So given the obscure nature,
> > maybe it's an acceptable level of regression. I dunno.
> 
> Right.  So I did a quick search on google and github for this, and
> there seems one place where git stash create -m is used [1].  From a
> quick look it does however not seem like the -m in the stash message
> is of any significance there, but rather that the intention was to use
> a flag that doesn't exist.

Yeah, I think your patch is actually fixing that case. But your search
is only part of the story. You found somebody using "-m" explicitly, but
what about somebody blindly calling:

  git stash create $*

That's now surprising to somebody who puts "-m" in their message.

> I *think* this regression is acceptable, but I'm happy to introduce
> another verb if people think otherwise.

Despite what I wrote above, I'm still inclined to say that this isn't an
important regression. I'd be surprised if "stash create" is used
independently much at all.

-Peff

Re: [PATCH] docs/git-submodule: fix unbalanced quote

[Junio C Hamano]

Jeff King  writes:

> However, let's take a step back. Even when rendered
> correctly, it's hard to read a long command stuck into the
> middle of a paragraph, and the important punctuation is hard
> to notice.

Yes, I like this reasoning behind the solution very much.

Thanks.

>  Documentation/git-submodule.txt | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 918bd1d1b..a8eb1c7ce 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -228,9 +228,12 @@ foreach::
>   the processing to terminate. This can be overridden by adding '|| :'
>   to the end of the command.
>  +
> -As an example, +git submodule foreach \'echo $path {backtick}git
> -rev-parse HEAD{backtick}'+ will show the path and currently checked out
> -commit for each submodule.
> +As an example, the command below will show the path and currently
> +checked out commit for each submodule:
> ++
> +--
> +git submodule foreach 'echo $path `git rev-parse HEAD`'
> +--
>  
>  sync::
>   Synchronizes submodules' remote URL configuration setting

Re: [PATCH v3 2/5] stash: introduce push verb

[Thomas Gummerer]

On 02/13, Matthieu Moy wrote:
> Thomas Gummerer  writes:
> 
> > Introduce a new git stash push verb in addition to git stash save.  The
> > push verb is used to transition from the current command line arguments
> > to a more conventional way, in which the message is given as an argument
> > to the -m option.
> 
> Sorry if this has been discussed before, but I find 'push' rather
> confusing here. It took me a while to understand that it meant "opposite
> of pop", because in the context of Git, "push" usually means "send to
> remote repository".

There wasn't much of a discussion about it, but it was pretty much the
only thing that came to my mind, and nobody complained or suggested
anything different, so I just went with it.  No other verb came to my
mind yet, but if someone has a better suggestion, I'd be happy to
change.

> Unfortunately, I didn't come up with a better name. "create" is already
> taken ...
> 
> Another think to have in mind: changing the option name to break
> backward compatibility is something we can't do often, so if there's
> anything else we should change about the UI, we should do it now. I
> don't have anything particular in mind, just thinking aloud.

Now that you mention this, there actually is one inconsistency that I
introduced, which I shouldn't have.  git stash push works with
--include-untracked and --all to decide whether or not to include
untracked files, and if also ignored files should be included.  I also
added a --include-untracked {untracked,all} argument to git stash
create, which is clearly inconsistent.

There really should only be one way.  I'd be fine with either way, but
I think using --include-untracked and --all is the better choice,
because it's easy to understand, and also makes it easier to switch
git stash without a verb over to use push_stash internally.

> -- 
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

Re: [PATCH] clean: use warning_errno() when appropriate

[Jeff King]

On Mon, Feb 13, 2017 at 01:53:33PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > IOW, I think this may be a case where we should be optimizing for
> > programmer time (fewer lines of code, and one less thing to worry about
> > in the callers) versus squeezing out every instruction.
> 
> Fair enough.
> 
> Unless we do the save_errno dance in all the helper functions we
> commonly use to safely stash away errno as necessary and tell
> developers that they can depend on it, the code in the patch that
> began this discussion still needs its own saved_errno dance to be
> safe, though.  I do not have a feeling that we are not there yet,
> even after we teach xmalloc() and its family to do so.

Yeah, I certainly agree that is a potential blocker. Even if it is true
today, there's nothing guaranteeing that the quote functions don't grow
a new internal detail that violates.

So in that sense doing the errno dance as close to the caller who cares
is the most _obvious_ thing, even if it isn't the shortest.

It would be nice if there was a way to annotate a function as
errno-safe, and then transitively compute which other functions were
errno-safe when they do not call any errno-unsafe function. I don't know
if any static analyzers allow that kind of custom annotation, though
(and also I wonder whether the cost/benefit of maintaining those
annotations would be worth it).

-Peff

Re: [PATCH v3 0/5] stash: support pathspec argument

[Thomas Gummerer]

On 02/13, Jeff King wrote:
> On Mon, Feb 13, 2017 at 10:35:31PM +0100, Matthieu Moy wrote:
> 
> > > Is it really that dangerous, though? The likely outcome is Git saying
> > > "nope, you don't have any changes to the file named drop". Of course the
> > > user may have meant something different, but I feel like "-p" is a good
> > > indicator that they are interested in making an actual stash.
> > 
> > Indeed -p is not the best example. In the old thread, I used -q which is
> > much more problematic:
> > 
> >   git stash -q drop => interpreted as: git stash push -q drop
> >   git stash drop -q => drop with option -q
> 
> Yeah, I'd agree with that. I wouldn't propose to loosen it entirely, but
> rather to treat "-p" specially.
> 
> > It's not really "dangerous" at least in this case, since we misinterpret
> > a destructive command for a less destructive one, but it is rather
> > confusing that changing the order between command and options change the
> > behavior.
> > 
> > I actually find it a reasonable expectation to allow swapping commands
> > and options, some programs other than git allow it.
> 
> I think we may have already crossed that bridge with "git -p stash".
> 
> Not to mention that the ordering already _is_ relevant (we disallow one
> order but not the other). If we really wanted to allow swapping, it
> would mean making:
> 
>   git stash -p drop
> 
> the same as:
> 
>   git stash drop -p
> 
> I actually find _that_ more confusing. It would perhaps make more sense
> with something like "-q", which is more of a "global" option than a
> command-specific one. But I think we'd want to whitelist such global
> options (and "-p" would not be on that list).
>
> > > The complexity is that right now, the first-level decision of "which
> > > stash sub-command am I running?" doesn't know about any options. So "git
> > > stash -m foo" would be rejected in the name of typo prevention, unless
> > > that outer decision learns about "-m" as an option.
> > 
> > Ah, OK. But that's not really hard to implement: when going through the
> > option list looking for non-option, shift one more time when finding -m.
> 
> No, it's not hard conceptually. It just means implementing the
> option-parsing policy in two places. That's not too bad now, but if we
> started using rev-parse's options helper, then I think you have corner
> cases like "git stash -km foo".
> 
> My "-p" suggestion suffers from a similar problem if you treat it as
> "you can omit the 'push' if you say "-p", rather than "if -p is the
> first option, it is a synonym for 'push -p'".

I'm almost convinced of special casing "-p".  (Maybe I'm easy to
convince as well, because it would be convenient ;) ) However it's a
bit weird that now "git stash -p file" would work, but "git stash -m
message" wouldn't.  Maybe we should do it the other way around, and
only special case "-q", and see if there is an non option argument
after that?  From a glance at the options that's the only one where
"git stash - " could make sense to the user.

[PATCH] mingw: make stderr unbuffered again

[Johannes Schindelin]

When removing the hack for isatty(), we actually removed more than just
an isatty() hack: we removed the hack where internal data structures of
the MSVC runtime are modified in order to redirect stdout/stderr.

Instead of using that hack (that does not work with newer versions of
the runtime, anyway), we replaced it by reopening the respective file
descriptors.

What we forgot was to mark stderr as unbuffered again.

Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.

Signed-off-by: Johannes Schindelin 
---
Published-As: 
https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-unbuffered-stderr-v1

 compat/winansi.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/compat/winansi.c b/compat/winansi.c
index 82b89ab1376..793420f9d0d 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -510,6 +510,8 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
 */
close(new_fd);
 
+   if (fd == 2)
+   setvbuf(stderr, NULL, _IONBF, BUFSIZ);
fd_is_interactive[fd] |= FD_SWAPPED;
 
return duplicate;
@@ -547,6 +549,8 @@ static void detect_msys_tty(int fd)
!wcsstr(name, L"-pty"))
return;
 
+   if (fd == 2)
+   setvbuf(stderr, NULL, _IONBF, BUFSIZ);
fd_is_interactive[fd] |= FD_MSYS;
 }
 

base-commit: 5588dbffbd61e4906e453808c6ad32f792fea521
-- 
2.11.1.windows.1

Re: [PATCH 06/11] refs-internal.h: correct is_per_worktree_ref()

[Stefan Beller]

On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy  wrote:
> All refs outside refs/ directory is per-worktree, not just HEAD.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/refs-internal.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index f4aed49f5..69d02b6ba 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -653,7 +653,7 @@ const char *resolve_ref_recursively(struct ref_store 
> *refs,
>
>  static inline int is_per_worktree_ref(const char *refname)
>  {
> -   return !strcmp(refname, "HEAD") ||
> +   return !starts_with(refname, "refs/") ||
> starts_with(refname, "refs/bisect/");

you're loosing HEAD here? (assuming we get HEAD in
short form here, as well as long form refs/HEAD)

>  }
>
> --
> 2.11.0.157.gd943d85
>

Re: [PATCH] mingw: use OpenSSL's SHA-1 routines

[Johannes Schindelin]

Hi,

On Mon, 13 Feb 2017, Johannes Sixt wrote:

> Am 13.02.2017 um 20:42 schrieb Junio C Hamano:
> > I have been operating under the assumption that everybody on Windows
> > who builds Git works off of Dscho's Git for Windows tree, and patches
> > that are specific to Windows from Dscho's are sent to me via the list
> > only after they have been in Git for Windows and proven to help
> > Windows users in the wild.
> >
> > The consequence of these two assumptions is that I would feel safe to
> > treat Windows specific changes that do not touch generic part of the
> > codebase from Dscho just like updates from any other subsystem
> > maintainers (any git-svn thing from Eric, any gitk thing from Paul,
> > any p4 thing Luke and Lars are both happy with, etc.).
> >
> > You seem to be saying that the first of the two assumptions does not
> > hold.  Should I change my expectations while queuing Windows specific
> > patches from Dscho?
> 
> Your first assumption is incorrect as far as I am concerned. I build
> from your tree plus some topics. During -rc period, I build off of
> master; after a release, I build off of next. I merge some of the topics
> that you carry in pu when I find them interesting or when I suspect them
> to regress on Windows.  Then I carry around a few additional patches
> that the public has never seen, and these days I also merge Dscho's
> rebase-i topic.

In addition, you build from a custom MINGW/MSys1 setup, correct?

Ciao,
Johannes

Re: [RFC PATCH] show decorations at the end of the line

[Jeff King]

On Mon, Feb 13, 2017 at 01:01:49PM -0800, Junio C Hamano wrote:

> Linus Torvalds  writes:
> 
> > And if you actually want decorations, and you're parsing them, you are
> > *not* going to script it with "--oneline --decorations", because the
> > end result is basically impossible to parse already (because it's
> > ambiguous - think about parentheses in the commit message).
> 
> OK.  So let's wait to hear from others if they like the "obviously"
> improved output.  Even though I find the decorations indispensable
> in my "git log" output, I personally do not have much preference
> either way, as my screen is often wide enough ;-)

I have a slight preference for the new output (decorations at the end)
versus the original, but I could go either way.

I don't think the scripting compatibility concerns are an issue, for all
the reasons given in the thread.

There is one related option, --source, which also puts its data between
the hash and the subject in --oneline. In theory that should be treated
similarly, though:

  1. It's already really ugly, as it does not even get the parentheses
 and coloring.

  2. It's perhaps more likely to get scripted, as it really is parseable
 in the current state.

I'm not sure if a better path forward would be to just extend the idea
of "decorator" to possibly include more than just ref-tips. On the other
hand, if you really want to get fancy with formatting, we already have a
complete formatting language. Perhaps it should learn a placeholder for
the "--source" data.

Similarly, I've often wanted a "contained in this tags/branches"
annotation for each commit. It's not too expensive to compute if you
topo-sort the set of commits and just paint down as you traverse.

Anyway, I think none of that needs to block changes to --decorate
output. Just thinking out loud.

-Peff

Re: [PATCH] mingw: make stderr unbuffered again

[Junio C Hamano]

Johannes Schindelin  writes:

> When removing the hack for isatty(), we actually removed more than just
> an isatty() hack: we removed the hack where internal data structures of
> the MSVC runtime are modified in order to redirect stdout/stderr.
>
> Instead of using that hack (that does not work with newer versions of
> the runtime, anyway), we replaced it by reopening the respective file
> descriptors.
>
> What we forgot was to mark stderr as unbuffered again.
>
> Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.
>
> Signed-off-by: Johannes Schindelin 
> ---
> Published-As: 
> https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git 
> mingw-unbuffered-stderr-v1

OK.  Should this go directly to 'master', as the isatty thing is
already in?

>
>  compat/winansi.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/compat/winansi.c b/compat/winansi.c
> index 82b89ab1376..793420f9d0d 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -510,6 +510,8 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
>*/
>   close(new_fd);
>  
> + if (fd == 2)
> + setvbuf(stderr, NULL, _IONBF, BUFSIZ);
>   fd_is_interactive[fd] |= FD_SWAPPED;
>  
>   return duplicate;
> @@ -547,6 +549,8 @@ static void detect_msys_tty(int fd)
>   !wcsstr(name, L"-pty"))
>   return;
>  
> + if (fd == 2)
> + setvbuf(stderr, NULL, _IONBF, BUFSIZ);
>   fd_is_interactive[fd] |= FD_MSYS;
>  }
>  
>
> base-commit: 5588dbffbd61e4906e453808c6ad32f792fea521

Re: [PATCH v3 4/5] stash: introduce new format create

[Jeff King]

On Mon, Feb 13, 2017 at 04:57:34PM -0500, Jeff King wrote:

> Yeah, I think your patch is actually fixing that case. But your search
> is only part of the story. You found somebody using "-m" explicitly, but
> what about somebody blindly calling:
> 
>   git stash create $*
> 
> That's now surprising to somebody who puts "-m" in their message.
> 
> > I *think* this regression is acceptable, but I'm happy to introduce
> > another verb if people think otherwise.
> 
> Despite what I wrote above, I'm still inclined to say that this isn't an
> important regression. I'd be surprised if "stash create" is used
> independently much at all.

Just thinking on this more...do we really care about "fixing" the
interface of "stash create"? This is really just about refactoring what
underlies the new "push", right?

So we could just do:

diff --git a/git-stash.sh b/git-stash.sh
index 6d629fc43..ee37db135 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -711,7 +711,7 @@ clear)
;;
 create)
shift
-   create_stash "$@" && echo "$w_commit"
+   create_stash -m "$*" && echo "$w_commit"
;;
 store)
shift

on top of your patch and keep the external interface the same.

It might be nice to clean up the interface for "create" to match other
ones, but from this discussion I think it is mostly a historical wart
for scripting, and we are OK to just leave its slightly-broken interface
in place forever.

I could go either way.

-Peff

Re: [PATCH 07/11] files-backend: remove the use of git_path()

[Stefan Beller]

> +
> +   if (submodule) {
> +   refs->submodule = xstrdup_or_null(submodule);

drop the _or_null here?

So in this patch we have either
* submodule set
* or gitdir/gitcommondir set

which means that we exercise the commondir for regular repos.
In the future when we want to be able to have a combination of worktrees
and submodules this ought to be possible by setting submodule != NULL
and still populating the gitdir/commondir buffers.

Thanks,
Stefan

new git-diff switch to eliminate leading "+" and "-" characters

[Vanderhoof, Tzadik]

The output of git-diff includes lines beginning with "+" and "-" to indicate 
added and deleted lines.  A somewhat common task (at least for me) is to want 
to copy output from a "diff" (usually the deleted lines) and paste it back into 
my code.

This is quite inconvenient because of the leading "+" and "-" characters.  I 
know there are shell and IDE / editor workarounds but it would be nice if there 
was a switch to git-diff to make it leave out those characters, especially 
since "--color" kind of makes those leading characters obsolete.

Would it make sense to develop such a switch or has there been work on that 
already?
__
Tzadik Vanderhoof | Optum360 
Sr Software Engineer, NLP Innovation
www.optum360.com

This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or his or her authorized agent, the reader is hereby notified
that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.

Re: [PATCH 08/11] refs.c: factor submodule code out of get_ref_store()

[Stefan Beller]

On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy  wrote:
> This code is going to be expanded a bit soon. Keep it out for
> better readability later.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

Looks good,
Thanks,
Stefan

Developing git with IDE

[Oleg Taranenko]

Hi *,

being last decade working with java & javascript I completely lost
relation to c/c++ world. Trying to get into git internals I'm facing
with issue what IDE is more suitable for developing git @ macos ?

Have googled, but any my search queries following to non-relevant
themes, like supporting git in IDEs etc.

my first attempt - CLion (as far as I'm Jetbrains fan) - got failed,
as far as doesn't support makefile-based projects, only CMake.

There are a number of free C/C++ dev tools: Xcode, CodeBlocks,
CodeLite. Gnat, Qt creator, Dev C++, C++ Builder (Borland? :),
Eclipse, NetBeans... what else?

Because of  lack my modern C experience, could somebody share his own
attempts/thoughts/use cases how more convenient dive into the git
development process on the Mac?

Tried to find in the git distribution Documentation more information
about this, nothing as well...  Would be nice to have a kind of
'Getting Started Manual'


Thanks for your time,

Oleg Taranenko

Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()

[Junio C Hamano]

Michael Haggerty  writes:

> I pushed the fixed commit to branch `submodule-hash` in my fork [1]. If
> you'd like me to send it to the mailing list again, please let me know.

I was tempted to ask you to send it again, because fetching,
comparing and then cherry-picking is a lot more work than just
replacing one, but just to make sure my intuition about the
work required is correct, I did it anyway and yes, fetching,
comparing, cherry-picking and then amending the sign-off in was a
lot more work ;-)

So no need to resend.  Thanks.

Re: [PATCH v6] gc: ignore old gc.log files

[Junio C Hamano]

David Turner  writes:

> +static unsigned long gc_log_expire_time;
> +static const char *gc_log_expire = "1.day.ago";

OK.

> @@ -113,6 +133,8 @@ static void gc_config(void)
>   git_config_get_bool("gc.autodetach", &detach_auto);
>   git_config_date_string("gc.pruneexpire", &prune_expire);
>   git_config_date_string("gc.worktreepruneexpire", 
> &prune_worktrees_expire);
> + git_config_date_string("gc.logexpiry", &gc_log_expire);
> +

OK.

I think I had a stale one queued; will replace.

Thanks.

[PATCH] completion: complete modified files for checkout with '--'

[cornelius . weig]

From: Cornelius Weig 

The command line completion for git-checkout bails out when seeing '--'
as an isolated argument. For git-checkout this signifies the start of a
list of files which are to be checked out. Checkout of files makes only
sense for modified files, therefore completion can be a bit smarter:
Instead of bailing out, offer modified files for completion.

Signed-off-by: Cornelius Weig 
---
 contrib/completion/git-completion.bash | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 6c6e1c7..d6523fd 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1059,7 +1059,10 @@ _git_bundle ()
 
 _git_checkout ()
 {
-   __git_has_doubledash && return
+   __git_has_doubledash && {
+   __git_complete_index_file "--modified"
+   return
+   }
 
case "$cur" in
--conflict=*)
-- 
2.10.2

Re: [PATCH 09/11] refs: move submodule code out of files-backend.c

[Stefan Beller]

On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy  wrote:
> files-backend is now initialized with a $GIT_DIR. Converting a submodule
> path to where real submodule gitdir is located is done in get_ref_store().
>
> The new code in init_submodule_ref_store() is basically a copy of
> strbuf_git_path_submodule().
>
> This gives a slight performance improvement for submodules since we
> don't convert submodule path to gitdir at every backend call like
> before. We pay that once at ref-store creation.
>
> More cleanup in files_downcast() follows shortly. It's separate to keep
> noises from this patch.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs.c   | 48 +---
>  refs/files-backend.c | 25 +++--
>  refs/refs-internal.h |  6 +++---
>  3 files changed, 51 insertions(+), 28 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 8ef7a52ba..9ac194945 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -9,6 +9,7 @@
>  #include "refs/refs-internal.h"
>  #include "object.h"
>  #include "tag.h"
> +#include "submodule-config.h"
>
>  /*
>   * List of all available backends
> @@ -1410,7 +1411,7 @@ static void register_ref_store(struct ref_store *refs, 
> const char *submodule)
>   * Create, record, and return a ref_store instance for the specified
>   * submodule (or the main repository if submodule is NULL).
>   */
> -static struct ref_store *ref_store_init(const char *submodule)
> +static struct ref_store *ref_store_init(const char *submodule, const char 
> *gitdir)
>  {
> const char *be_name = "files";
> struct ref_storage_be *be = find_ref_storage_backend(be_name);
> @@ -1419,7 +1420,7 @@ static struct ref_store *ref_store_init(const char 
> *submodule)
> if (!be)
> die("BUG: reference backend %s is unknown", be_name);
>
> -   refs = be->init(submodule);
> +   refs = be->init(gitdir);
> register_ref_store(refs, submodule);
> return refs;
>  }
> @@ -1445,15 +1446,48 @@ static struct ref_store *lookup_ref_store(const char 
> *submodule)
> return entry ? entry->refs : NULL;
>  }
>
> -static struct ref_store *init_submodule_ref_store(const char *submodule)
> +static struct ref_store *init_submodule_ref_store(const char *path)
>  {
> struct strbuf submodule_sb = STRBUF_INIT;
> +   struct strbuf git_submodule_common_dir = STRBUF_INIT;
> +   struct strbuf git_submodule_dir = STRBUF_INIT;
> +   struct strbuf buf = STRBUF_INIT;
> +   const char *git_dir;
> +   const struct submodule *sub;
> struct ref_store *refs = NULL;
>
> -   strbuf_addstr(&submodule_sb, submodule);
> -   if (is_nonbare_repository_dir(&submodule_sb))
> -   refs = ref_store_init(submodule);
> +   strbuf_addstr(&submodule_sb, path);
> +   if (!is_nonbare_repository_dir(&submodule_sb))
> +   goto done;
> +
> +   strbuf_addstr(&buf, path);
> +   strbuf_complete(&buf, '/');
> +   strbuf_addstr(&buf, ".git");
> +
> +   git_dir = read_gitfile(buf.buf);

if buf.buf is a (git) directory as opposed to a git file,
we error out in read_gitfile. Did you mean to use
read_gitfile_gently here or rather even resolve_gitdir_gently ?

> +   if (git_dir) {

when not using the _gently version git_dir is always
non NULL here (or we're dead)?

> +   strbuf_reset(&buf);
> +   strbuf_addstr(&buf, git_dir);
> +   }
> +   if (!is_git_directory(buf.buf)) {
> +   gitmodules_config();
> +   sub = submodule_from_path(null_sha1, path);
> +   if (!sub)
> +   goto done;
> +   strbuf_reset(&buf);
> +   strbuf_git_path(&buf, "%s/%s", "modules", sub->name);

You can inline "modules" into the format string?

> +   }
> +   strbuf_addch(&buf, '/');
> +   strbuf_addbuf(&git_submodule_dir, &buf);
> +
> +   refs = ref_store_init(path, git_submodule_dir.buf);

strbuf_detach (git_submodule_dir) here, such that we keep
the string alive despite the release of the strbuf below?

so essentially this function
* takes a submodule path
* checks if there is a repo at the given path in the working tree
* resolves the gitfile if any
* if the gitfile could not resolve to a valid repo just make up the
  location to be $GIT_DIR/modules/

sounds confusing to me. I need to reread it later.

>
> -   if (submodule) {
> -   refs->submodule = xstrdup_or_null(submodule);
> +   if (gitdir) {
> +   strbuf_addstr(&refs->gitdir, gitdir);
> +   get_common_dir_noenv(&refs->gitcommondir, gitdir);

Oh I see. we loose the _or_null here, so my remark on the previous patch
might be just unneeded work.

> } else {
> strbuf_addstr(&refs->gitdir, get_git_dir());
> strbuf_addstr(&refs->gitcommondir, get_git_common_dir());
> @@ -1034,8 +1025,6 @@ static struct ref_store *files_ref_store_cre

Re: [PATCH 10/11] files-backend: remove submodule_allowed from files_downcast()

[Stefan Beller]

On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy  wrote:
> Since submodule or not is irrelevant to files-backend after the last
> patch, remove the parameter from files_downcast() and kill
> files_assert_main_repository().
>
> PS. This implies that all ref operations are allowed for submodules. But
> we may need to look more closely to see if that's really true...
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

Looks good,
Stefan

Re: Continuous Testing of Git on Windows

[Junio C Hamano]

Johannes Schindelin  writes:

> That is why I taught the Git for Windows CI job that tests the four
> upstream Git integration branches to *also* bisect test breakages and then
> upload comments to the identified commit on GitHub

Good.  I do not think it is useful to try 'pu' as an aggregate and
expect it to always build and work [*1*], but your "bisect and
pinpoint" approach makes it useful to identify individual topic that
brings in a breakage.  I wouldn't be surprised if original submitter
and I were the only two people who actually compiled the patches on
a topic in isolation while a topic is in 'pu', and chances are that
these two people didn't try their builds on Windows.  A CI like this
one will help the coverage to stop premature topics from advancing
to 'pu' without getting any Windows exposure.

Thanks.


[Footnote]

*1* The reason why topics not in 'next' but in 'pu', especially the
ones merged near the tip of 'pu', exist in 'pu' are because they
are interesting enough and could be polished to become eligible
for 'next' but known to be premature for 'next' yet.  They are
there primarily to give human contributors an easier way to
download them as a whole and help polish them.  And I have to be
selective when I queue things on 'pu'; it is not like I have
infinite amount of time to pick up any cruft that is sent to the
list.

Re: [PATCH] clean: use warning_errno() when appropriate

[Junio C Hamano]

Jeff King  writes:

> So in that sense doing the errno dance as close to the caller who cares
> is the most _obvious_ thing, even if it isn't the shortest.

Yup.

> It would be nice if there was a way to annotate a function as
> errno-safe, and then transitively compute which other functions were
> errno-safe when they do not call any errno-unsafe function. I don't know
> if any static analyzers allow that kind of custom annotation, though
> (and also I wonder whether the cost/benefit of maintaining those
> annotations would be worth it).

Double yup.

Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API

[Stefan Beller]

>
> +/*
> + * Return the ref_store instance for the specified submodule. For the
> + * main repository, use submodule==NULL; such a call cannot fail.

So now we have both a get_main as well as a get_submodule function,
but the submodule function can return the main as well?

I'd rather see this as a BUG; or asking another way:
What is the difference between get_submodule_ref_store(NULL)
and get_main_ref_store() ?

As you went through all call sites (by renaming the function), we'd
be able to tell that there is no caller with NULL, or is it?

Stefan

> For
> + * a submodule, the submodule must exist and be a nonbare repository,
> + * otherwise return NULL. If the requested reference store has not yet
> + * been initialized, initialize it first.
> + *
> + * For backwards compatibility, submodule=="" is treated the same as
> + * submodule==NULL.
> + */
> +struct ref_store *get_submodule_ref_store(const char *submodule);
> +struct ref_store *get_main_ref_store(void);

[PATCH for NEXT] grep: do not unnecessarily query repo for "--"

[Jonathan Tan]

When running a command of the form

  git grep --no-index pattern -- path

in the absence of a Git repository, an error message will be printed:

  fatal: BUG: setup_git_env called without repository

This is because "git grep" tries to interpret "--" as a rev. "git grep"
has always tried to first interpret "--" as a rev for at least a few
years, but this issue was upgraded from a pessimization to a bug in
commit 59332d1 ("Resurrect "git grep --no-index"", 2010-02-06), which
calls get_sha1 regardless of whether --no-index was specified. This bug
appeared to be benign until commit b1ef400 ("setup_git_env: avoid blind
fall-back to ".git"", 2016-10-20) when Git was taught to die in this
situation.  (This "git grep" bug appears to be one of the bugs that
commit b1ef400 is meant to flush out.)

Therefore, always interpret "--" as signaling the end of options,
instead of trying to interpret it as a rev first.

Signed-off-by: Jonathan Tan 
---

There is probably a similar bug for commands of the form:

  git grep --no-index pattern foo

If there is a repo and "foo" is a rev, the "--no-index or --untracked
cannot be used with revs." error would occur. If there is a repo and
"foo" is not a rev, this command would proceed as usual. If there is no
repo, the "setup_git_env called without repository" error would occur.
(This is my understanding from reading the code - I haven't tested it
out.)

This patch does not fix this similar bug, but I decided to send it out
anyway because it still fixes a bug and unlocks the ability to
specify paths with "git grep --no-index".

 builtin/grep.c  |  9 +
 t/t7810-grep.sh | 12 
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef49..1b68d1638 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
const char *arg = argv[i];
unsigned char sha1[20];
struct object_context oc;
+   if (!strcmp(arg, "--")) {
+   i++;
+   seen_dashdash = 1;
+   break;
+   }
/* Is it a rev? */
if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
struct object *object = parse_object_or_die(sha1, arg);
@@ -1162,10 +1167,6 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
add_object_array_with_path(object, arg, &list, oc.mode, 
oc.path);
continue;
}
-   if (!strcmp(arg, "--")) {
-   i++;
-   seen_dashdash = 1;
-   }
break;
}
 
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 19f0108f8..29202f0e7 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '
test_cmp expected actual
 '
 
+test_expect_success 'grep --no-index pattern -- path' '
+   rm -fr non &&
+   mkdir -p non/git &&
+   (
+   GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+   export GIT_CEILING_DIRECTORIES &&
+   cd non/git &&
+   echo hello >hello &&
+   git grep --no-index o -- .
+   )
+'
+
 cat >expected <

Re: [PATCH v3 0/5] stash: support pathspec argument

[Jeff King]

On Mon, Feb 13, 2017 at 03:50:43PM -0800, Junio C Hamano wrote:

> Thomas Gummerer  writes:
> 
> >> My "-p" suggestion suffers from a similar problem if you treat it as
> >> "you can omit the 'push' if you say "-p", rather than "if -p is the
> >> first option, it is a synonym for 'push -p'".
> >
> > I'm almost convinced of special casing "-p".  (Maybe I'm easy to
> > convince as well, because it would be convenient ;) ) However it's a
> > bit weird that now "git stash -p file" would work, but "git stash -m
> > message" wouldn't.
> 
> I am not sure why this matters.  The original "git stash " was
> just "Are you being extremely busy and cannot even afford to type
> 'save'?  Ok, let me assume you meant that!".  Now we are talking
> about picking and choosing hunks carefully going through interactive
> process, I really do not think there is any justification to infer
> 'push' when 'push' was omitted in "git stash push -p" the user wants
> to do.

Maybe it is just me and my muscle memory, but "git stash -p" is quite a
common command for me[1]. And I have typed "git stash -p foo" many times
and been annoyed that it didn't work. I was hoping to end that
annoyance.

I guess I could make an alias and retrain my fingers.

-Peff

[1] I almost never run "reset --hard", preferring instead to stash away
changes just in case I would change my mind later and want them. And
I quite often use "stash -p" because I like to double check what I
am throwing away.

I also use "stash -p" heavily when picking apart changes from the
working tree.

Re: [PATCH v3 0/5] stash: support pathspec argument

[Jeff King]

On Mon, Feb 13, 2017 at 04:36:22PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Maybe it is just me and my muscle memory, but "git stash -p" is quite a
> > common command for me[1]. And I have typed "git stash -p foo" many times
> > and been annoyed that it didn't work. I was hoping to end that
> > annoyance.
> 
> OK, I never run "stash -p" and did not realize people already find
> it useful that it becomes "stash save -p".  Please ignore me, then.
> I agree that breaking the established use is not nice.

To be fair, I don't think anybody is proposing breaking the established
use of "stash -p". I was just hoping the new pathspec feature could
trickle down to it, as well. :)

-Peff

Re: [RFC-PATCHv2] submodules: add a background story

[Brandon Williams]

On 02/08, Stefan Beller wrote:
> +STATES
> +--
> +
> +When working with submodules, you can think of them as in a state machine.
> +So each submodule can be in a different state, the following indicators are 
> used:
> +
> +* the existence of the setting of 'submodule..url' in the
> +  superprojects configuration
> +* the existence of the submodules working tree within the
> +  working tree of the superproject
> +* the existence of the submodules git directory within the superprojects
> +  git directory at $GIT_DIR/modules/ or within the submodules working
> +  tree
> +
> +  State  URL configworking tree git dir
> +  -
> +  uninitializedno   no   no
> +  initialized yes   no   no
> +  populated   yes  yes  yes
> +  depopulated yes   no  yes
> +  deinitializedno   no  yes
> +  uninterestingno  yes  yes
> +
> +  invalid  no  yes   no
> +  invalid yes  yes   no
> +  -
> +
> +The first six states can be reached by normal git usage, the latter two are
> +only shown for completeness to show all possible eight states with 3 binary
> +indicators. The states in detail:
> +
> +uninitialized::
> +The uninitialized state is the default state if no
> +'--recurse-submodules' / '--recursive'. An empty directory will be put in
> +the working tree as a place holder, such that you are reminded of the
> +existence of the submodule.
> +---
> +To transition into the initialized state
> +you can use 'git submodule init', which copies the presets from the
> +.gitmodules file into the config.
> +
> +initialized::
> +Users transitioned from the uninitialized state to this state via
> +'git submodule init', which preset the URL configuration. As these URLs
> +may not be desired in certain scenarios, this state allows to change the
> +URLs.  For example in a corporate environment you may want to run
> +
> +sed -i s/example.org/$internal-mirror/ .git/config
> ++

Maybe we can try to brainstorm and come up with some clearer terminology
while we are at it.  I was trying to think about the "initialized" state
and I may be the only one but it seems unclear what "initialized" means.
I mean I already have all the information about a submodule in the
.gitmodules file, isn't it already initialized then?   Maybe this state
would be better named "(in)active" as a module that is interesting to a
user is "active"?

-- 
Brandon Williams

Re: [PATCH] completion: complete modified files for checkout with '--'

[SZEDER Gábor]

On Tue, Feb 14, 2017 at 12:33 AM,   wrote:
> From: Cornelius Weig 
>
> The command line completion for git-checkout bails out when seeing '--'
> as an isolated argument. For git-checkout this signifies the start of a
> list of files which are to be checked out. Checkout of files makes only
> sense for modified files,

No, there is e.g. 'git checkout that-branch this-path', too.


> therefore completion can be a bit smarter:
> Instead of bailing out, offer modified files for completion.
>
> Signed-off-by: Cornelius Weig 
> ---
>  contrib/completion/git-completion.bash | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 6c6e1c7..d6523fd 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1059,7 +1059,10 @@ _git_bundle ()
>
>  _git_checkout ()
>  {
> -   __git_has_doubledash && return
> +   __git_has_doubledash && {
> +   __git_complete_index_file "--modified"
> +   return
> +   }
>
> case "$cur" in
> --conflict=*)
> --
> 2.10.2
>

Re: [PATCH for NEXT] grep: do not unnecessarily query repo for "--"

[Jeff King]

On Mon, Feb 13, 2017 at 04:11:59PM -0800, Jonathan Tan wrote:

> When running a command of the form
> 
>   git grep --no-index pattern -- path
> 
> in the absence of a Git repository, an error message will be printed:
> 
>   fatal: BUG: setup_git_env called without repository
> 
> This is because "git grep" tries to interpret "--" as a rev. "git grep"
> has always tried to first interpret "--" as a rev for at least a few
> years, but this issue was upgraded from a pessimization to a bug in
> commit 59332d1 ("Resurrect "git grep --no-index"", 2010-02-06), which
> calls get_sha1 regardless of whether --no-index was specified. This bug
> appeared to be benign until commit b1ef400 ("setup_git_env: avoid blind
> fall-back to ".git"", 2016-10-20) when Git was taught to die in this
> situation.  (This "git grep" bug appears to be one of the bugs that
> commit b1ef400 is meant to flush out.)
> 
> Therefore, always interpret "--" as signaling the end of options,
> instead of trying to interpret it as a rev first.

Nicely explained.

> There is probably a similar bug for commands of the form:
> 
>   git grep --no-index pattern foo
> 
> If there is a repo and "foo" is a rev, the "--no-index or --untracked
> cannot be used with revs." error would occur. If there is a repo and
> "foo" is not a rev, this command would proceed as usual. If there is no
> repo, the "setup_git_env called without repository" error would occur.
> (This is my understanding from reading the code - I haven't tested it
> out.)

Yes, it's easy to see that "git grep --no-index foo bar" outside of a
repo generates the same BUG. I suspect that "--no-index" should just
disable looking up revs entirely, even if we are actually in a
repository directory.

> This patch does not fix this similar bug, but I decided to send it out
> anyway because it still fixes a bug and unlocks the ability to
> specify paths with "git grep --no-index".

Yes, I think even if we fix the other bug, fixing this "--" thing is an
improvement.

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 2c727ef49..1b68d1638 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char 
> *prefix)
>   const char *arg = argv[i];
>   unsigned char sha1[20];
>   struct object_context oc;
> + if (!strcmp(arg, "--")) {
> + i++;
> + seen_dashdash = 1;
> + break;
> + }
>   /* Is it a rev? */
>   if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
>   struct object *object = parse_object_or_die(sha1, arg);

So I think this is a definite improvement, but I see a few leftover
oddities:

  - the end logic for this loop is now:

  if (arg is a rev) {
 ... handle rev ...
 continue;
  }
  break;

It would probably be more obvious as:

  if (arg is not a rev)
  break;
  ... handle rev ...

  - the rev-handling code does:

  if (!seen_dashdash)
  verify_non_filename(prefix, arg);

But I do not see how seen_dashdash could ever be untrue. We set it
inside this loop, and break immediately when we see it. And indeed,
running:

  echo content >master
  git grep content master --

does not work. The "--" should tell us that "master" is a rev, but
we don't know yet that we have a dashdash.

I think we need a separate loop to find the "--" first, and _then_
walk through the arguments, treating them as revs or paths as
appropriate. This is how setup_revisions() does it.

So this isn't a problem introduced by your patch, but it's
intimately related.

> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 19f0108f8..29202f0e7 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '
>   test_cmp expected actual
>  '
>  
> +test_expect_success 'grep --no-index pattern -- path' '
> + rm -fr non &&
> + mkdir -p non/git &&
> + (
> + GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd non/git &&
> + echo hello >hello &&
> + git grep --no-index o -- .
> + )
> +'

Since de95302a4, you can do:

  nongit git grep --no-index -o -- .

Though if this is destined for maint, it might need to be done
separately this way and cleaned up later.

It might also be a good idea to confirm that the pathspec is actually
being respected in the --no-index case. Something like:

  echo hello >hello &&
  echo goodbye >goodbye &&
  echo hello:hello >expect &&
  git grep --no-index o -- hello >actual &&
  test_cmp expect actual

-Peff

[PATCH v2 09/19] builtin/merge: convert to struct object_id

[brian m. carlson]

Additionally convert several uses of the constant 40 into
GIT_SHA1_HEXSZ.

Signed-off-by: brian m. carlson 
---
 builtin/merge.c | 134 
 1 file changed, 66 insertions(+), 68 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index a96d4fb501..099cfab447 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -244,7 +244,7 @@ static void drop_save(void)
unlink(git_path_merge_mode());
 }
 
-static int save_state(unsigned char *stash)
+static int save_state(struct object_id *stash)
 {
int len;
struct child_process cp = CHILD_PROCESS_INIT;
@@ -265,7 +265,7 @@ static int save_state(unsigned char *stash)
else if (!len)  /* no changes */
return -1;
strbuf_setlen(&buffer, buffer.len-1);
-   if (get_sha1(buffer.buf, stash))
+   if (get_oid(buffer.buf, stash))
die(_("not a valid object: %s"), buffer.buf);
return 0;
 }
@@ -305,18 +305,18 @@ static void reset_hard(unsigned const char *sha1, int 
verbose)
die(_("read-tree failed"));
 }
 
-static void restore_state(const unsigned char *head,
- const unsigned char *stash)
+static void restore_state(const struct object_id *head,
+ const struct object_id *stash)
 {
struct strbuf sb = STRBUF_INIT;
const char *args[] = { "stash", "apply", NULL, NULL };
 
-   if (is_null_sha1(stash))
+   if (is_null_oid(stash))
return;
 
-   reset_hard(head, 1);
+   reset_hard(head->hash, 1);
 
-   args[2] = sha1_to_hex(stash);
+   args[2] = oid_to_hex(stash);
 
/*
 * It is OK to ignore error here, for example when there was
@@ -376,10 +376,10 @@ static void squash_message(struct commit *commit, struct 
commit_list *remotehead
 
 static void finish(struct commit *head_commit,
   struct commit_list *remoteheads,
-  const unsigned char *new_head, const char *msg)
+  const struct object_id *new_head, const char *msg)
 {
struct strbuf reflog_message = STRBUF_INIT;
-   const unsigned char *head = head_commit->object.oid.hash;
+   const struct object_id *head = &head_commit->object.oid;
 
if (!msg)
strbuf_addstr(&reflog_message, getenv("GIT_REFLOG_ACTION"));
@@ -397,7 +397,7 @@ static void finish(struct commit *head_commit,
else {
const char *argv_gc_auto[] = { "gc", "--auto", NULL };
update_ref(reflog_message.buf, "HEAD",
-   new_head, head, 0,
+   new_head->hash, head->hash, 0,
UPDATE_REFS_DIE_ON_ERR);
/*
 * We ignore errors in 'gc --auto', since the
@@ -416,7 +416,7 @@ static void finish(struct commit *head_commit,
DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
opts.detect_rename = DIFF_DETECT_RENAME;
diff_setup_done(&opts);
-   diff_tree_sha1(head, new_head, "", &opts);
+   diff_tree_sha1(head->hash, new_head->hash, "", &opts);
diffcore_std(&opts);
diff_flush(&opts);
}
@@ -431,7 +431,7 @@ static void finish(struct commit *head_commit,
 static void merge_name(const char *remote, struct strbuf *msg)
 {
struct commit *remote_head;
-   unsigned char branch_head[20];
+   struct object_id branch_head;
struct strbuf buf = STRBUF_INIT;
struct strbuf bname = STRBUF_INIT;
const char *ptr;
@@ -441,25 +441,25 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
strbuf_branchname(&bname, remote);
remote = bname.buf;
 
-   memset(branch_head, 0, sizeof(branch_head));
+   oidclr(&branch_head);
remote_head = get_merge_parent(remote);
if (!remote_head)
die(_("'%s' does not point to a commit"), remote);
 
-   if (dwim_ref(remote, strlen(remote), branch_head, &found_ref) > 0) {
+   if (dwim_ref(remote, strlen(remote), branch_head.hash, &found_ref) > 0) 
{
if (starts_with(found_ref, "refs/heads/")) {
strbuf_addf(msg, "%s\t\tbranch '%s' of .\n",
-   sha1_to_hex(branch_head), remote);
+   oid_to_hex(&branch_head), remote);
goto cleanup;
}
if (starts_with(found_ref, "refs/tags/")) {
strbuf_addf(msg, "%s\t\ttag '%s' of .\n",
-   sha1_to_hex(branch_head), remote);
+   oid_to_hex(&branch_head), remote);
goto cleanup;
}
if (starts_with(found_ref, "refs/remotes/")) {
strbuf_addf(msg, "%s\t\tremote

[PATCH v2 03/19] builtin/describe: convert to struct object_id

[brian m. carlson]

Convert the functions in this file and struct commit_name  to struct
object_id.

Signed-off-by: brian m. carlson 
---
 builtin/describe.c | 50 +-
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a157e..738e68f95b 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -39,11 +39,11 @@ static const char *diff_index_args[] = {
 
 struct commit_name {
struct hashmap_entry entry;
-   unsigned char peeled[20];
+   struct object_id peeled;
struct tag *tag;
unsigned prio:2; /* annotated tag = 2, tag = 1, head = 0 */
unsigned name_checked:1;
-   unsigned char sha1[20];
+   struct object_id oid;
char *path;
 };
 
@@ -54,17 +54,17 @@ static const char *prio_names[] = {
 static int commit_name_cmp(const struct commit_name *cn1,
const struct commit_name *cn2, const void *peeled)
 {
-   return hashcmp(cn1->peeled, peeled ? peeled : cn2->peeled);
+   return oidcmp(&cn1->peeled, peeled ? peeled : &cn2->peeled);
 }
 
-static inline struct commit_name *find_commit_name(const unsigned char *peeled)
+static inline struct commit_name *find_commit_name(const struct object_id 
*peeled)
 {
-   return hashmap_get_from_hash(&names, sha1hash(peeled), peeled);
+   return hashmap_get_from_hash(&names, sha1hash(peeled->hash), 
peeled->hash);
 }
 
 static int replace_name(struct commit_name *e,
   int prio,
-  const unsigned char *sha1,
+  const struct object_id *oid,
   struct tag **tag)
 {
if (!e || e->prio < prio)
@@ -77,13 +77,13 @@ static int replace_name(struct commit_name *e,
struct tag *t;
 
if (!e->tag) {
-   t = lookup_tag(e->sha1);
+   t = lookup_tag(e->oid.hash);
if (!t || parse_tag(t))
return 1;
e->tag = t;
}
 
-   t = lookup_tag(sha1);
+   t = lookup_tag(oid->hash);
if (!t || parse_tag(t))
return 0;
*tag = t;
@@ -96,24 +96,24 @@ static int replace_name(struct commit_name *e,
 }
 
 static void add_to_known_names(const char *path,
-  const unsigned char *peeled,
+  const struct object_id *peeled,
   int prio,
-  const unsigned char *sha1)
+  const struct object_id *oid)
 {
struct commit_name *e = find_commit_name(peeled);
struct tag *tag = NULL;
-   if (replace_name(e, prio, sha1, &tag)) {
+   if (replace_name(e, prio, oid, &tag)) {
if (!e) {
e = xmalloc(sizeof(struct commit_name));
-   hashcpy(e->peeled, peeled);
-   hashmap_entry_init(e, sha1hash(peeled));
+   oidcpy(&e->peeled, peeled);
+   hashmap_entry_init(e, sha1hash(peeled->hash));
hashmap_add(&names, e);
e->path = NULL;
}
e->tag = tag;
e->prio = prio;
e->name_checked = 0;
-   hashcpy(e->sha1, sha1);
+   oidcpy(&e->oid, oid);
free(e->path);
e->path = xstrdup(path);
}
@@ -154,7 +154,7 @@ static int get_name(const char *path, const struct 
object_id *oid, int flag, voi
else
prio = 0;
 
-   add_to_known_names(all ? path + 5 : path + 10, peeled.hash, prio, 
oid->hash);
+   add_to_known_names(all ? path + 5 : path + 10, &peeled, prio, oid);
return 0;
 }
 
@@ -212,7 +212,7 @@ static unsigned long finish_depth_computation(
 static void display_name(struct commit_name *n)
 {
if (n->prio == 2 && !n->tag) {
-   n->tag = lookup_tag(n->sha1);
+   n->tag = lookup_tag(n->oid.hash);
if (!n->tag || parse_tag(n->tag))
die(_("annotated tag %s not available"), n->path);
}
@@ -230,14 +230,14 @@ static void display_name(struct commit_name *n)
printf("%s", n->path);
 }
 
-static void show_suffix(int depth, const unsigned char *sha1)
+static void show_suffix(int depth, const struct object_id *oid)
 {
-   printf("-%d-g%s", depth, find_unique_abbrev(sha1, abbrev));
+   printf("-%d-g%s", depth, find_unique_abbrev(oid->hash, abbrev));
 }
 
 static void describe(const char *arg, int last_one)
 {
-   unsigned char sha1[20];
+   struct object_id oid;
struct commit *cmit, *gave_up_on = NULL;
struct commit_list *list;
struct commit_name *n;
@@ -246,20 +246,20 @@ static void describe(const char *arg, int last_o

[PATCH v2 12/19] reflog-walk: convert struct reflog_info to struct object_id

[brian m. carlson]

Convert struct reflog_info to use struct object_id by changing the
structure definition and applying the following semantic patch:

@@
struct reflog_info E1;
@@
- E1.osha1
+ E1.ooid.hash

@@
struct reflog_info *E1;
@@
- E1->osha1
+ E1->ooid.hash

@@
struct reflog_info E1;
@@
- E1.nsha1
+ E1.noid.hash

@@
struct reflog_info *E1;
@@
- E1->nsha1
+ E1->noid.hash

Signed-off-by: brian m. carlson 
---
 reflog-walk.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index f98748e2ae..fe5be41471 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -10,7 +10,7 @@ struct complete_reflogs {
char *ref;
const char *short_ref;
struct reflog_info {
-   unsigned char osha1[20], nsha1[20];
+   struct object_id ooid, noid;
char *email;
unsigned long timestamp;
int tz;
@@ -28,8 +28,8 @@ static int read_one_reflog(unsigned char *osha1, unsigned 
char *nsha1,
 
ALLOC_GROW(array->items, array->nr + 1, array->alloc);
item = array->items + array->nr;
-   hashcpy(item->osha1, osha1);
-   hashcpy(item->nsha1, nsha1);
+   hashcpy(item->ooid.hash, osha1);
+   hashcpy(item->noid.hash, nsha1);
item->email = xstrdup(email);
item->timestamp = timestamp;
item->tz = tz;
@@ -238,13 +238,13 @@ void fake_reflog_parent(struct reflog_walk_info *info, 
struct commit *commit)
do {
reflog = &commit_reflog->reflogs->items[commit_reflog->recno];
commit_reflog->recno--;
-   logobj = parse_object(reflog->osha1);
+   logobj = parse_object(reflog->ooid.hash);
} while (commit_reflog->recno && (logobj && logobj->type != 
OBJ_COMMIT));
 
-   if (!logobj && commit_reflog->recno >= 0 && 
is_null_sha1(reflog->osha1)) {
+   if (!logobj && commit_reflog->recno >= 0 && 
is_null_sha1(reflog->ooid.hash)) {
/* a root commit, but there are still more entries to show */
reflog = &commit_reflog->reflogs->items[commit_reflog->recno];
-   logobj = parse_object(reflog->nsha1);
+   logobj = parse_object(reflog->noid.hash);
}
 
if (!logobj || logobj->type != OBJ_COMMIT) {
-- 
2.11.0

[PATCH v2 16/19] sha1_file: introduce an nth_packed_object_oid function

[brian m. carlson]

There are places in the code where we would like to provide a struct
object_id *, yet read the hash directly from the pack.  Provide an
nth_packed_object_oid function that is similar to the
nth_packed_object_sha1 function.

In order to avoid a potentially invalid cast, nth_packed_object_oid
provides a variable into which to store the value, which it returns on
success; on error, it returns NULL, as nth_packed_object_sha1 does.

Signed-off-by: brian m. carlson 
---
 cache.h |  6 ++
 sha1_file.c | 17 ++---
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 5dc89a058c..04b1d923f3 100644
--- a/cache.h
+++ b/cache.h
@@ -1607,6 +1607,12 @@ extern void check_pack_index_ptr(const struct packed_git 
*p, const void *ptr);
  * error.
  */
 extern const unsigned char *nth_packed_object_sha1(struct packed_git *, 
uint32_t n);
+/*
+ * Like nth_packed_object_oid, but write the data into the object specified by
+ * the the first argument.  Returns the first argument on success, and NULL on
+ * error.
+ */
+extern const struct object_id *nth_packed_object_oid(struct object_id *, 
struct packed_git *, uint32_t n);
 
 /*
  * Return the offset of the nth object within the specified packfile.
diff --git a/sha1_file.c b/sha1_file.c
index ec957db5e1..777b8e8eae 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2628,6 +2628,17 @@ const unsigned char *nth_packed_object_sha1(struct 
packed_git *p,
}
 }
 
+const struct object_id *nth_packed_object_oid(struct object_id *oid,
+ struct packed_git *p,
+ uint32_t n)
+{
+   const unsigned char *hash = nth_packed_object_sha1(p, n);
+   if (!hash)
+   return NULL;
+   hashcpy(oid->hash, hash);
+   return oid;
+}
+
 void check_pack_index_ptr(const struct packed_git *p, const void *vptr)
 {
const unsigned char *ptr = vptr;
@@ -3788,13 +3799,13 @@ static int for_each_object_in_pack(struct packed_git 
*p, each_packed_object_fn c
int r = 0;
 
for (i = 0; i < p->num_objects; i++) {
-   const unsigned char *sha1 = nth_packed_object_sha1(p, i);
+   struct object_id oid;
 
-   if (!sha1)
+   if (!nth_packed_object_oid(&oid, p, i))
return error("unable to get sha1 of object %u in %s",
 i, p->pack_name);
 
-   r = cb(sha1, p, i, data);
+   r = cb(oid.hash, p, i, data);
if (r)
break;
}
-- 
2.11.0

[PATCH v2 06/19] builtin/grep: convert to struct object_id

[brian m. carlson]

Convert several functions to use struct object_id, and rename them so
that they no longer refer to SHA-1.

Signed-off-by: brian m. carlson 
---
 builtin/grep.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef499..0393b0fdc4 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -294,17 +294,17 @@ static int grep_cmd_config(const char *var, const char 
*value, void *cb)
return st;
 }
 
-static void *lock_and_read_sha1_file(const unsigned char *sha1, enum 
object_type *type, unsigned long *size)
+static void *lock_and_read_oid_file(const struct object_id *oid, enum 
object_type *type, unsigned long *size)
 {
void *data;
 
grep_read_lock();
-   data = read_sha1_file(sha1, type, size);
+   data = read_sha1_file(oid->hash, type, size);
grep_read_unlock();
return data;
 }
 
-static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
+static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
 const char *filename, int tree_name_len,
 const char *path)
 {
@@ -323,7 +323,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned 
char *sha1,
 
 #ifndef NO_PTHREADS
if (num_threads) {
-   add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);
+   add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, oid);
strbuf_release(&pathbuf);
return 0;
} else
@@ -332,7 +332,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned 
char *sha1,
struct grep_source gs;
int hit;
 
-   grep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, path, 
sha1);
+   grep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, path, oid);
strbuf_release(&pathbuf);
hit = grep_source(opt, &gs);
 
@@ -690,7 +690,7 @@ static int grep_cache(struct grep_opt *opt, const struct 
pathspec *pathspec,
ce_skip_worktree(ce)) {
if (ce_stage(ce) || ce_intent_to_add(ce))
continue;
-   hit |= grep_sha1(opt, ce->oid.hash, ce->name,
+   hit |= grep_oid(opt, &ce->oid, ce->name,
 0, ce->name);
} else {
hit |= grep_file(opt, ce->name);
@@ -750,7 +750,7 @@ static int grep_tree(struct grep_opt *opt, const struct 
pathspec *pathspec,
strbuf_add(base, entry.path, te_len);
 
if (S_ISREG(entry.mode)) {
-   hit |= grep_sha1(opt, entry.oid->hash, base->buf, 
tn_len,
+   hit |= grep_oid(opt, entry.oid, base->buf, tn_len,
 check_attr ? base->buf + tn_len : 
NULL);
} else if (S_ISDIR(entry.mode)) {
enum object_type type;
@@ -758,7 +758,7 @@ static int grep_tree(struct grep_opt *opt, const struct 
pathspec *pathspec,
void *data;
unsigned long size;
 
-   data = lock_and_read_sha1_file(entry.oid->hash, &type, 
&size);
+   data = lock_and_read_oid_file(entry.oid, &type, &size);
if (!data)
die(_("unable to read tree (%s)"),
oid_to_hex(entry.oid));
@@ -787,7 +787,7 @@ static int grep_object(struct grep_opt *opt, const struct 
pathspec *pathspec,
   struct object *obj, const char *name, const char *path)
 {
if (obj->type == OBJ_BLOB)
-   return grep_sha1(opt, obj->oid.hash, name, 0, path);
+   return grep_oid(opt, &obj->oid, name, 0, path);
if (obj->type == OBJ_COMMIT || obj->type == OBJ_TREE) {
struct tree_desc tree;
void *data;
@@ -1152,11 +1152,11 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
/* Check revs and then paths */
for (i = 0; i < argc; i++) {
const char *arg = argv[i];
-   unsigned char sha1[20];
+   struct object_id oid;
struct object_context oc;
/* Is it a rev? */
-   if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
-   struct object *object = parse_object_or_die(sha1, arg);
+   if (!get_sha1_with_context(arg, 0, oid.hash, &oc)) {
+   struct object *object = parse_object_or_die(oid.hash, 
arg);
if (!seen_dashdash)
verify_non_filename(prefix, arg);
add_object_array_with_path(object, arg, &list, oc.mode, 
oc.path);
-- 
2.11.0