Re: git alias for options

[Jeff King]

On Fri, Feb 17, 2017 at 02:34:15PM -0800, hIpPy wrote:

> I think the conversation has drifted away from what I am asking / hoping for.

Yeah, the usual answer to "can we have custom options" is "use a custom
alias". But I agree they are not quite the same thing.

> Say I want an alias for option --name-status as -s, so I can type:
> $ git log -s
> 
> But there is already a -s option and that wins so the built-in option
> alias wins.
> 
> However, I think I should be able alias it as --ns.
> $ git log --ns

To be honest, I am not that enthused about the idea, but I don't have an
real objection beyond "meh, that looks like an unnecessary
complication".

If anybody wants to pursue it, the simplest way would probably be to
teach parse-options to take a callback for an unknown option, which
could then do a config lookup to transmute the argument into another
option-name.

Though many of the options (notably the revision-walker and diff ones)
are not handled by parse-options. So that might present a challenge.

-Peff

Re: [PATCH 0/5] A series of performance enhancements in the memihash and name-cache area

[Jeff King]

On Fri, Feb 17, 2017 at 09:58:21PM -0800, Junio C Hamano wrote:

> Jeff Hostetler  writes:
> 
> > I'll try to put together a before/after perf-test to better
> > demonstrate this.
> 
> I didn't pick up the series while watching these exchanges, as I
> didn't know how quick your turnaround would be, but now a few days
> have passed.  Just to make sure we won't forget this topic, I'll
> pick up these 5 patches in the meantime.

Yeah, to be clear my question was not an objection, but mostly
curiosity and interest.

-Peff

Re: [PATCH 0/5] A series of performance enhancements in the memihash and name-cache area

[Junio C Hamano]

Jeff Hostetler  writes:

> I'll try to put together a before/after perf-test to better
> demonstrate this.

I didn't pick up the series while watching these exchanges, as I
didn't know how quick your turnaround would be, but now a few days
have passed.  Just to make sure we won't forget this topic, I'll
pick up these 5 patches in the meantime.

Thanks.

Re: [PATCH 0/5] A series of performance enhancements in the memihash and name-cache area

[Junio C Hamano]

Jeff King  writes:

> On Wed, Feb 15, 2017 at 09:27:53AM -0500, Jeff Hostetler wrote:
>
>> I have some informal numbers in a spreadsheet.  I was seeing
>> a 8-9% speed up on a status on my gigantic repo.
>> 
>> I'll try to put together a before/after perf-test to better
>> demonstrate this.
>
> Thanks. What I'm mostly curious about is how much each individual step
> buys. Sometimes when doing a long optimization series, I find that some
> of the optimizations make other ones somewhat redundant (e.g., if patch
> 2 causes us to call the optimized code from patch 3 less often).

I am curious too.

To me 1/5 (reduction of redundant calls), 4/5 (correctly size the
hash that would grow to a known size anyway) and 5/5 (take advantage
of the fact that adjacent cache entries are often in the same
directory) look like no brainers to take, regardless of the others
(including themselves).

It is not clear to me if 3/5 (preload-index uses available cores to
compute hashes) is an unconditional win (an operation that is
pathspec limited may need hashes for only a small fraction of the
index---would it still be a win to compute the hash for all entries
upon loading the index, even if we are using otherwise-idel cores?).

Of course 2/5 is a prerequisite step for 3/5 and 5/5, so if we want
either of the latter two, we cannot avoid it.

Re: [PATCH 3/5] name-hash: precompute hash values during preload-index

[Junio C Hamano]

Johannes Schindelin  writes:

> +void precompute_istate_hashes(struct cache_entry *ce)
> +{
> + int namelen = ce_namelen(ce);
> +
> + while (namelen > 0 && !is_dir_sep(ce->name[namelen - 1]))
> + namelen--;
> +
> + if (namelen <= 0) {
> + ce->precomputed_hash.name = memihash(ce->name, ce_namelen(ce));
> + ce->precomputed_hash.root_entry = 1;
> + } else {
> + namelen--;
> + ce->precomputed_hash.dir = memihash(ce->name, namelen);
> + ce->precomputed_hash.name = memihash_continue(
> + ce->precomputed_hash.dir, ce->name + namelen,
> + ce_namelen(ce) - namelen);
> + ce->precomputed_hash.root_entry = 0;
> + }
> + ce->precomputed_hash.initialized = 1;
> +}
> diff --git a/preload-index.c b/preload-index.c
> index c1fe3a3ef9c..602737f9d0f 100644
> --- a/preload-index.c
> +++ b/preload-index.c
> @@ -47,6 +47,8 @@ static void *preload_thread(void *_data)
>   struct cache_entry *ce = *cep++;
>   struct stat st;
>  
> + precompute_istate_hashes(ce);
> +

The fact that each preload_thread() still walks the index in-order
makes me wonder if it may allow us to further optimize the "dir"
part of the hash by passing the previous ce for which we already
precomputed hash values.  While the loop is iterating over the paths
in the same directory, .dir component from the previous ce can be
reused and .name component can "continue", no?

It's possible that you already tried such an optimization and
rejected it after finding that the cost of comparison of pathnames
to tell if ce and previous ce are still in the same directory is
more than unconditionally memihash() the directory part, and I am in
no way saying that I found a missed optimization opportunity you
must pursue.  I am just being curious.

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

[Junio C Hamano]

Jeff King  writes:

>> The updates to the expectation look like this (already squashed).
>> The --source decorations in 4202 are also shown at the end, which
>> probably is in line with the way --show-decorations adds them at the
>> end of the line, but was somewhat surprising from reading only the
>> log message.
>
> Hrm, that does surprise me. I'm not sure if that's desirable or not. I
> do think some of the "nobody could possibly be parsing these" arguments
> about decorations do not apply to --source (and also, they're harder for
> humans to pick out from the end of the line as they lack punctuation and
> color).

I just got bitten by a fallout.  I have

$ git recent --help
`git recent' is aliased to `log --oneline --branches --no-merges \
 --source --since=3.weeks'

and often do

$ git recent name-hash.c

primarily to see if I already queued a patch series to a topic (and
forgot about it), and/or what other recent topics in flight touch
the same thing.

I'd need that the topic name to be shown rather prominently for this
use case, i.e.

eb2263adb1  jh/memihash-opt name-hash: remember previous dir_...
0c04267dc8  jh/memihash-opt name-hash: specify initial size f...
57463ce445  jh/memihash-opt name-hash: precompute hash values...
dd3170e2cf  jh/memihash-opt name-hash: eliminate duplicate me...

but now the branch names are shown at the end, which defeats the
whole point of the alias.

If nobody gets around to fixing it, I may take a look at it when
able, but for now let me just vent^Wreport a regression first.

Re: [PATCH 2/5] hashmap: allow memihash computation to be continued

[Junio C Hamano]

Johannes Schindelin  writes:

> diff --git a/hashmap.c b/hashmap.c
> index b10b642229c..061b7d61da6 100644
> --- a/hashmap.c
> +++ b/hashmap.c
> @@ -50,6 +50,20 @@ unsigned int memihash(const void *buf, size_t len)
>   return hash;
>  }
>  
> +/* Incoporate another chunk of data into a memihash computation. */
> +unsigned int memihash_continue(unsigned int hash,
> +const void *buf, size_t len)
> +{
> + const unsigned char *p = buf;
> + while (len--) {
> + unsigned int c = *p++;
> + if (c >= 'a' && c <= 'z')
> + c -= 'a' - 'A';
> + hash = (hash * FNV32_PRIME) ^ c;
> + }
> + return hash;
> +}

This makes me wonder if we want to reduce the duplication (primarily
to avoid risking the loop body to go out of sync) by doing:

unsigned int memihash(const void *buf, size_t len)
{
return memihash_continue(buf, len, FNV32_BASE);
}

If an extra call level really matters, its "inline" equivalent in
the header would probably be good.

Re: [PATCH v3 02/19] builtin/diff-tree: convert to struct object_id

[Jeff King]

On Fri, Feb 17, 2017 at 08:42:17PM -0500, Jeff King wrote:

> > I'm wondering if parse_oid_hex could be useful here as well.
> 
> I know I haven't looked at this chunk nearly as carefully as you have,
> but it seems somewhat crazy to me that these functions get the original
> "line" in the first place. Shouldn't they get line+40 from the caller
> (who in turn should be using parse_oid_hex to compute that)?
> 
> And then each function should subsequently parse left-to-right with
> a mix of isspace() and parse_oid_hex(), and probably doesn't even need
> to care about the original "len" at all (yes, you can quit early if you
> know your len isn't long enough, but that's the unusual error case
> anyway; it's not a big deal to find that out while parsing).
> 
> In general, I think this sort of left-to-right incremental pointer
> movement is safe and simple. There may be a few cases where it doesn't
> apply (i.e., where you need to look at the end of the string to know how
> to parse the beginning), but that should be relatively rare.

So for this case, something like the patch below.

Incidentally, there's an off-by-one in the original loop of
stdin_diff_commit that reads past the end of the trailing NUL for the
final sha1 on the line. The problem is the:

  pos += GIT_SHA1_HEXSZ + 1;

which assumes we're slurping up the trailing space. This works in
practice because the caller will only permit a string which had a
newline (which it converted into a NUL).

I suspect that function could be more aggressive about complaining about
nonsense on the line, rather than silently ignoring it.

 builtin/diff-tree.c | 43 ---
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 1f1573bb2..222c671f2 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -16,37 +16,33 @@ static int diff_tree_commit_sha1(const struct object_id 
*oid)
 }
 
 /* Diff one or more commits. */
-static int stdin_diff_commit(struct commit *commit, char *line, int len)
+static int stdin_diff_commit(struct commit *commit, const char *p)
 {
struct object_id oid;
-   if (isspace(line[GIT_SHA1_HEXSZ]) && 
!get_oid_hex(line+GIT_SHA1_HEXSZ+1, )) {
-   /* Graft the fake parents locally to the commit */
-   int pos = GIT_SHA1_HEXSZ + 1;
-   struct commit_list **pptr;
-
-   /* Free the real parent list */
-   free_commit_list(commit->parents);
-   commit->parents = NULL;
-   pptr = &(commit->parents);
-   while (line[pos] && !get_oid_hex(line + pos, )) {
-   struct commit *parent = lookup_commit(oid.hash);
-   if (parent) {
-   pptr = _list_insert(parent, pptr)->next;
-   }
-   pos += GIT_SHA1_HEXSZ + 1;
+   struct commit_list **pptr = NULL;
+
+   /* Graft the fake parents locally to the commit */
+   while (isspace(*p++) && !parse_oid_hex(p, , )) {
+   struct commit *parent = lookup_commit(oid.hash);
+   if (!pptr) {
+   /* Free the real parent list */
+   free_commit_list(commit->parents);
+   commit->parents = NULL;
+   pptr = &(commit->parents);
+   }
+   if (parent) {
+   pptr = _list_insert(parent, pptr)->next;
}
}
return log_tree_commit(_tree_opt, commit);
 }
 
 /* Diff two trees. */
-static int stdin_diff_trees(struct tree *tree1, char *line, int len)
+static int stdin_diff_trees(struct tree *tree1, const char *p)
 {
struct object_id oid;
struct tree *tree2;
-   const int chunksz = GIT_SHA1_HEXSZ + 1;
-   if (len != 2 * chunksz || !isspace(line[chunksz-1]) ||
-   get_sha1_hex(line + chunksz, oid.hash))
+   if (!isspace(*p++) || parse_oid_hex(p, , ) || *p)
return error("Need exactly two trees, separated by a space");
tree2 = lookup_tree(oid.hash);
if (!tree2 || parse_tree(tree2))
@@ -64,19 +60,20 @@ static int diff_tree_stdin(char *line)
int len = strlen(line);
struct object_id oid;
struct object *obj;
+   const char *rest;
 
if (!len || line[len-1] != '\n')
return -1;
line[len-1] = 0;
-   if (get_oid_hex(line, ))
+   if (parse_oid_hex(line, , ))
return -1;
obj = parse_object(oid.hash);
if (!obj)
return -1;
if (obj->type == OBJ_COMMIT)
-   return stdin_diff_commit((struct commit *)obj, line, len);
+   return stdin_diff_commit((struct commit *)obj, rest);
if (obj->type == OBJ_TREE)
-   return stdin_diff_trees((struct tree *)obj, line, len);
+   return stdin_diff_trees((struct tree *)obj, rest);

Re: [PATCH v3 02/19] builtin/diff-tree: convert to struct object_id

[Jeff King]

On Sat, Feb 18, 2017 at 01:26:07AM +, brian m. carlson wrote:

> > > + struct object_id oid;
> > >   struct tree *tree2;
> > > - if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
> > > + const int chunksz = GIT_SHA1_HEXSZ + 1;
> > > + if (len != 2 * chunksz || !isspace(line[chunksz-1]) ||
> > > + get_sha1_hex(line + chunksz, oid.hash))
> > 
> > I'm not sure that this is an improvement. The input expected in 'line'
> > is supposed to look like: ' +  +  + <\n>'. So your
> > 'chunk' would be a  plus one 'char' of some sort. Except that the
> > caller of this function has already replaced the newline character with
> > a '\0' char (so strlen(line) would return 81), but still passes the
> > original line length! Also, note that this (and other functions in this
> > file) actually test for 'isspace(char)' rather than for a ' ' char!
> > 
> > Hmm, maybe just:
> > 
> > if (len < (2 * GIT_SHA1_HEXSZ + 1) || line[GIT_SHA1_HEXSZ] != ' ' ||
> > get_sha1_hex(line + GIT_SHA1_HEXSZ + 1, oid.hash))
> > 
> > (or, perhaps, still call isspace() in this patch ...)
> 
> Well, I think it's strictly an improvement in that we have avoided
> writing hardcoded constants[0].  I did intend it as a "hash plus one"
> chunk, which is actually quite common throughout the code.
> 
> I'm wondering if parse_oid_hex could be useful here as well.

I know I haven't looked at this chunk nearly as carefully as you have,
but it seems somewhat crazy to me that these functions get the original
"line" in the first place. Shouldn't they get line+40 from the caller
(who in turn should be using parse_oid_hex to compute that)?

And then each function should subsequently parse left-to-right with
a mix of isspace() and parse_oid_hex(), and probably doesn't even need
to care about the original "len" at all (yes, you can quit early if you
know your len isn't long enough, but that's the unusual error case
anyway; it's not a big deal to find that out while parsing).

In general, I think this sort of left-to-right incremental pointer
movement is safe and simple. There may be a few cases where it doesn't
apply (i.e., where you need to look at the end of the string to know how
to parse the beginning), but that should be relatively rare.

> [0] If we change the hash size, all of the GIT_SHA1_HEXSZ constants can
> be replaced with a variable that varies based on hash size, and the code
> still works.

I am happy to see fewer magic numbers. But I think with incremental
pointer-movements, we don't even need to use the numeric constants,
either. If one day we can parse "sha256:1234abcd..." as an oid, then the
existing code would "just work".

-Peff

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

[brian m. carlson]

On Sat, Feb 18, 2017 at 01:24:34AM +, Ramsay Jones wrote:
> 
> 
> On 18/02/17 00:06, brian m. carlson wrote:
> > 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 e03a672d15..4f3bfc5ee7 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -1608,6 +1608,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
>  ^^^
> ... Like nth_packed_object_sha1, but ...

Good catch.

> Having said that, if the intent is to eventually replace that function with
> the new nth_packed_object_oid(), then it is probably not a good idea to
> describe this function in terms of the function it will obsolete. ;-)

I've chosen to define them that way for now, in the hopes that it will
make them easier to use (as people are already familiar with the _sha1
version).  When I remove nth_packed_object_sha1 eventually, I'll update
the docstring so that the oid version is standalone.  I think I've
already done that somewhere else before (although it may be in a patch
that I haven't sent yet).
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v3 02/19] builtin/diff-tree: convert to struct object_id

[brian m. carlson]

On Sat, Feb 18, 2017 at 01:18:11AM +, Ramsay Jones wrote:
> 
> 
> On 18/02/17 00:06, brian m. carlson wrote:
> > Convert most leaf functions to struct object_id.  Rewrite several
> > hardcoded numbers in terms of GIT_SHA1_HEXSZ, using an intermediate
> > variable where that makes sense.
> > 
> > Signed-off-by: brian m. carlson 
> > ---
> >  builtin/diff-tree.c | 38 --
> >  1 file changed, 20 insertions(+), 18 deletions(-)
> > 
> > diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> > index 8ce00480cd..1f1573bb2a 100644
> > --- a/builtin/diff-tree.c
> > +++ b/builtin/diff-tree.c
> > @@ -7,9 +7,9 @@
> >  
> >  static struct rev_info log_tree_opt;
> >  
> > -static int diff_tree_commit_sha1(const unsigned char *sha1)
> > +static int diff_tree_commit_sha1(const struct object_id *oid)
> >  {
> > -   struct commit *commit = lookup_commit_reference(sha1);
> > +   struct commit *commit = lookup_commit_reference(oid->hash);
> > if (!commit)
> > return -1;
> > return log_tree_commit(_tree_opt, commit);
> > @@ -18,22 +18,22 @@ static int diff_tree_commit_sha1(const unsigned char 
> > *sha1)
> >  /* Diff one or more commits. */
> >  static int stdin_diff_commit(struct commit *commit, char *line, int len)
> >  {
> > -   unsigned char sha1[20];
> > -   if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
> > +   struct object_id oid;
> > +   if (isspace(line[GIT_SHA1_HEXSZ]) && 
> > !get_oid_hex(line+GIT_SHA1_HEXSZ+1, )) {
> > /* Graft the fake parents locally to the commit */
> > -   int pos = 41;
> > +   int pos = GIT_SHA1_HEXSZ + 1;
> > struct commit_list **pptr;
> >  
> > /* Free the real parent list */
> > free_commit_list(commit->parents);
> > commit->parents = NULL;
> > pptr = &(commit->parents);
> > -   while (line[pos] && !get_sha1_hex(line + pos, sha1)) {
> > -   struct commit *parent = lookup_commit(sha1);
> > +   while (line[pos] && !get_oid_hex(line + pos, )) {
> > +   struct commit *parent = lookup_commit(oid.hash);
> > if (parent) {
> > pptr = _list_insert(parent, pptr)->next;
> > }
> > -   pos += 41;
> > +   pos += GIT_SHA1_HEXSZ + 1;
> > }
> > }
> > return log_tree_commit(_tree_opt, commit);
> > @@ -42,11 +42,13 @@ static int stdin_diff_commit(struct commit *commit, 
> > char *line, int len)
> >  /* Diff two trees. */
> >  static int stdin_diff_trees(struct tree *tree1, char *line, int len)
> >  {
> > -   unsigned char sha1[20];
> > +   struct object_id oid;
> > struct tree *tree2;
> > -   if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
> > +   const int chunksz = GIT_SHA1_HEXSZ + 1;
> > +   if (len != 2 * chunksz || !isspace(line[chunksz-1]) ||
> > +   get_sha1_hex(line + chunksz, oid.hash))
> 
> I'm not sure that this is an improvement. The input expected in 'line'
> is supposed to look like: ' +  +  + <\n>'. So your
> 'chunk' would be a  plus one 'char' of some sort. Except that the
> caller of this function has already replaced the newline character with
> a '\0' char (so strlen(line) would return 81), but still passes the
> original line length! Also, note that this (and other functions in this
> file) actually test for 'isspace(char)' rather than for a ' ' char!
> 
> Hmm, maybe just:
> 
> if (len < (2 * GIT_SHA1_HEXSZ + 1) || line[GIT_SHA1_HEXSZ] != ' ' ||
> get_sha1_hex(line + GIT_SHA1_HEXSZ + 1, oid.hash))
> 
> (or, perhaps, still call isspace() in this patch ...)

Well, I think it's strictly an improvement in that we have avoided
writing hardcoded constants[0].  I did intend it as a "hash plus one"
chunk, which is actually quite common throughout the code.

I'm wondering if parse_oid_hex could be useful here as well.

[0] If we change the hash size, all of the GIT_SHA1_HEXSZ constants can
be replaced with a variable that varies based on hash size, and the code
still works.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

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

[Ramsay Jones]

On 18/02/17 00:06, brian m. carlson wrote:
> 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 e03a672d15..4f3bfc5ee7 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1608,6 +1608,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
 ^^^
... Like nth_packed_object_sha1, but ...

Having said that, if the intent is to eventually replace that function with
the new nth_packed_object_oid(), then it is probably not a good idea to
describe this function in terms of the function it will obsolete. ;-)

ATB,
Ramsay Jones

> + * 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(, 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;
>   }
> 

Re: [PATCH v3 02/19] builtin/diff-tree: convert to struct object_id

[Ramsay Jones]

On 18/02/17 00:06, brian m. carlson wrote:
> Convert most leaf functions to struct object_id.  Rewrite several
> hardcoded numbers in terms of GIT_SHA1_HEXSZ, using an intermediate
> variable where that makes sense.
> 
> Signed-off-by: brian m. carlson 
> ---
>  builtin/diff-tree.c | 38 --
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> index 8ce00480cd..1f1573bb2a 100644
> --- a/builtin/diff-tree.c
> +++ b/builtin/diff-tree.c
> @@ -7,9 +7,9 @@
>  
>  static struct rev_info log_tree_opt;
>  
> -static int diff_tree_commit_sha1(const unsigned char *sha1)
> +static int diff_tree_commit_sha1(const struct object_id *oid)
>  {
> - struct commit *commit = lookup_commit_reference(sha1);
> + struct commit *commit = lookup_commit_reference(oid->hash);
>   if (!commit)
>   return -1;
>   return log_tree_commit(_tree_opt, commit);
> @@ -18,22 +18,22 @@ static int diff_tree_commit_sha1(const unsigned char 
> *sha1)
>  /* Diff one or more commits. */
>  static int stdin_diff_commit(struct commit *commit, char *line, int len)
>  {
> - unsigned char sha1[20];
> - if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
> + struct object_id oid;
> + if (isspace(line[GIT_SHA1_HEXSZ]) && 
> !get_oid_hex(line+GIT_SHA1_HEXSZ+1, )) {
>   /* Graft the fake parents locally to the commit */
> - int pos = 41;
> + int pos = GIT_SHA1_HEXSZ + 1;
>   struct commit_list **pptr;
>  
>   /* Free the real parent list */
>   free_commit_list(commit->parents);
>   commit->parents = NULL;
>   pptr = &(commit->parents);
> - while (line[pos] && !get_sha1_hex(line + pos, sha1)) {
> - struct commit *parent = lookup_commit(sha1);
> + while (line[pos] && !get_oid_hex(line + pos, )) {
> + struct commit *parent = lookup_commit(oid.hash);
>   if (parent) {
>   pptr = _list_insert(parent, pptr)->next;
>   }
> - pos += 41;
> + pos += GIT_SHA1_HEXSZ + 1;
>   }
>   }
>   return log_tree_commit(_tree_opt, commit);
> @@ -42,11 +42,13 @@ static int stdin_diff_commit(struct commit *commit, char 
> *line, int len)
>  /* Diff two trees. */
>  static int stdin_diff_trees(struct tree *tree1, char *line, int len)
>  {
> - unsigned char sha1[20];
> + struct object_id oid;
>   struct tree *tree2;
> - if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
> + const int chunksz = GIT_SHA1_HEXSZ + 1;
> + if (len != 2 * chunksz || !isspace(line[chunksz-1]) ||
> + get_sha1_hex(line + chunksz, oid.hash))

I'm not sure that this is an improvement. The input expected in 'line'
is supposed to look like: ' +  +  + <\n>'. So your
'chunk' would be a  plus one 'char' of some sort. Except that the
caller of this function has already replaced the newline character with
a '\0' char (so strlen(line) would return 81), but still passes the
original line length! Also, note that this (and other functions in this
file) actually test for 'isspace(char)' rather than for a ' ' char!

Hmm, maybe just:

if (len < (2 * GIT_SHA1_HEXSZ + 1) || line[GIT_SHA1_HEXSZ] != ' ' ||
get_sha1_hex(line + GIT_SHA1_HEXSZ + 1, oid.hash))

(or, perhaps, still call isspace() in this patch ...)

ATB,
Ramsay Jones

>   return error("Need exactly two trees, separated by a space");
> - tree2 = lookup_tree(sha1);
> + tree2 = lookup_tree(oid.hash);
>   if (!tree2 || parse_tree(tree2))
>   return -1;
>   printf("%s %s\n", oid_to_hex(>object.oid),
> @@ -60,15 +62,15 @@ static int stdin_diff_trees(struct tree *tree1, char 
> *line, int len)
>  static int diff_tree_stdin(char *line)
>  {
>   int len = strlen(line);
> - unsigned char sha1[20];
> + struct object_id oid;
>   struct object *obj;
>  
>   if (!len || line[len-1] != '\n')
>   return -1;
>   line[len-1] = 0;
> - if (get_sha1_hex(line, sha1))
> + if (get_oid_hex(line, ))
>   return -1;
> - obj = parse_object(sha1);
> + obj = parse_object(oid.hash);
>   if (!obj)
>   return -1;
>   if (obj->type == OBJ_COMMIT)
> @@ -76,7 +78,7 @@ static int diff_tree_stdin(char *line)
>   if (obj->type == OBJ_TREE)
>   return stdin_diff_trees((struct tree *)obj, line, len);
>   error("Object %s is a %s, not a commit or tree",
> -   sha1_to_hex(sha1), typename(obj->type));
> +   oid_to_hex(), typename(obj->type));
>   return -1;
>  }
>  
> @@ -141,7 +143,7 @@ int cmd_diff_tree(int argc, const char **argv, const char 
> *prefix)
>   break;
>   case 1:
>   tree1 = 

[PATCH] remote: Ignore failure to remove missing branch..merge

[Ross Lagerwall]

If a branch is configured with a default remote but no
branch..merge and then the remote is removed, git fails to remove
the remote with:
"fatal: could not unset 'branch..merge'"

Instead, ignore this since it is not an error and shouldn't prevent the
remote from being removed.

Signed-off-by: Ross Lagerwall 
---
 builtin/remote.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index e52cf3925..5dd22c2eb 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -769,7 +769,9 @@ static int rm(int argc, const char **argv)
strbuf_reset();
strbuf_addf(, "branch.%s.%s",
item->string, *k);
-   git_config_set(buf.buf, NULL);
+   result = git_config_set_gently(buf.buf, NULL);
+   if (result && result != CONFIG_NOTHING_SET)
+   die(_("COULd not unset '%s'"), buf.buf);
}
}
}
-- 
2.11.0

[PATCH v3 01/19] builtin/commit: convert to struct object_id

[brian m. carlson]

Convert most leaf functions to use struct object_id.

Signed-off-by: brian m. carlson 
---
 builtin/commit.c | 46 +++---
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 2de5f6cc64..4e288bc513 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -496,7 +496,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
 static int run_status(FILE *fp, const char *index_file, const char *prefix, 
int nowarn,
  struct wt_status *s)
 {
-   unsigned char sha1[20];
+   struct object_id oid;
 
if (s->relative_paths)
s->prefix = prefix;
@@ -509,9 +509,9 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
s->index_file = index_file;
s->fp = fp;
s->nowarn = nowarn;
-   s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+   s->is_initial = get_sha1(s->reference, oid.hash) ? 1 : 0;
if (!s->is_initial)
-   hashcpy(s->sha1_commit, sha1);
+   hashcpy(s->sha1_commit, oid.hash);
s->status_format = status_format;
s->ignore_submodule_arg = ignore_submodule_arg;
 
@@ -885,7 +885,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
commitable = run_status(s->fp, index_file, prefix, 1, s);
s->use_color = saved_color_setting;
} else {
-   unsigned char sha1[20];
+   struct object_id oid;
const char *parent = "HEAD";
 
if (!active_nr && read_cache() < 0)
@@ -894,7 +894,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
if (amend)
parent = "HEAD^1";
 
-   if (get_sha1(parent, sha1)) {
+   if (get_sha1(parent, oid.hash)) {
int i, ita_nr = 0;
 
for (i = 0; i < active_nr; i++)
@@ -1332,7 +1332,7 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
 {
static struct wt_status s;
int fd;
-   unsigned char sha1[20];
+   struct object_id oid;
static struct option builtin_status_options[] = {
OPT__VERBOSE(, N_("be verbose")),
OPT_SET_INT('s', "short", _format,
@@ -1382,9 +1382,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
 
fd = hold_locked_index(_lock, 0);
 
-   s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
+   s.is_initial = get_sha1(s.reference, oid.hash) ? 1 : 0;
if (!s.is_initial)
-   hashcpy(s.sha1_commit, sha1);
+   hashcpy(s.sha1_commit, oid.hash);
 
s.ignore_submodule_arg = ignore_submodule_arg;
s.status_format = status_format;
@@ -1418,19 +1418,19 @@ static const char *implicit_ident_advice(void)
 
 }
 
-static void print_summary(const char *prefix, const unsigned char *sha1,
+static void print_summary(const char *prefix, const struct object_id *oid,
  int initial_commit)
 {
struct rev_info rev;
struct commit *commit;
struct strbuf format = STRBUF_INIT;
-   unsigned char junk_sha1[20];
+   struct object_id junk_oid;
const char *head;
struct pretty_print_context pctx = {0};
struct strbuf author_ident = STRBUF_INIT;
struct strbuf committer_ident = STRBUF_INIT;
 
-   commit = lookup_commit(sha1);
+   commit = lookup_commit(oid->hash);
if (!commit)
die(_("couldn't look up newly created commit"));
if (parse_commit(commit))
@@ -1477,7 +1477,7 @@ static void print_summary(const char *prefix, const 
unsigned char *sha1,
rev.diffopt.break_opt = 0;
diff_setup_done();
 
-   head = resolve_ref_unsafe("HEAD", 0, junk_sha1, NULL);
+   head = resolve_ref_unsafe("HEAD", 0, junk_oid.hash, NULL);
if (!strcmp(head, "HEAD"))
head = _("detached HEAD");
else
@@ -1522,8 +1522,8 @@ static int git_commit_config(const char *k, const char 
*v, void *cb)
return git_status_config(k, v, s);
 }
 
-static int run_rewrite_hook(const unsigned char *oldsha1,
-   const unsigned char *newsha1)
+static int run_rewrite_hook(const struct object_id *oldoid,
+   const struct object_id *newoid)
 {
struct child_process proc = CHILD_PROCESS_INIT;
const char *argv[3];
@@ -1544,7 +1544,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
code = start_command();
if (code)
return code;
-   strbuf_addf(, "%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+   strbuf_addf(, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
sigchain_push(SIGPIPE, SIG_IGN);
write_in_full(proc.in, sb.buf, sb.len);

[PATCH v3 19/19] wt-status: convert to struct object_id

[brian m. carlson]

Convert the remaining uses of unsigned char [20] to struct object_id.

Signed-off-by: brian m. carlson 
---
 wt-status.c | 44 ++--
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 5fac8437b0..a8d1faf80d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1115,16 +1115,16 @@ static void abbrev_sha1_in_line(struct strbuf *line)
 
split = strbuf_split_max(line, ' ', 3);
if (split[0] && split[1]) {
-   unsigned char sha1[20];
+   struct object_id oid;
 
/*
 * strbuf_split_max left a space. Trim it and re-add
 * it after abbreviation.
 */
strbuf_trim(split[1]);
-   if (!get_sha1(split[1]->buf, sha1)) {
+   if (!get_oid(split[1]->buf, )) {
strbuf_reset(split[1]);
-   strbuf_add_unique_abbrev(split[1], sha1,
+   strbuf_add_unique_abbrev(split[1], oid.hash,
 DEFAULT_ABBREV);
strbuf_addch(split[1], ' ');
strbuf_reset(line);
@@ -1340,7 +1340,7 @@ static void show_bisect_in_progress(struct wt_status *s,
 static char *get_branch(const struct worktree *wt, const char *path)
 {
struct strbuf sb = STRBUF_INIT;
-   unsigned char sha1[20];
+   struct object_id oid;
const char *branch_name;
 
if (strbuf_read_file(, worktree_git_path(wt, "%s", path), 0) <= 0)
@@ -1354,9 +1354,9 @@ static char *get_branch(const struct worktree *wt, const 
char *path)
strbuf_remove(, 0, branch_name - sb.buf);
else if (starts_with(sb.buf, "refs/"))
;
-   else if (!get_sha1_hex(sb.buf, sha1)) {
+   else if (!get_oid_hex(sb.buf, )) {
strbuf_reset();
-   strbuf_add_unique_abbrev(, sha1, DEFAULT_ABBREV);
+   strbuf_add_unique_abbrev(, oid.hash, DEFAULT_ABBREV);
} else if (!strcmp(sb.buf, "detached HEAD")) /* rebase */
goto got_nothing;
else/* bisect */
@@ -1370,7 +1370,7 @@ static char *get_branch(const struct worktree *wt, const 
char *path)
 
 struct grab_1st_switch_cbdata {
struct strbuf buf;
-   unsigned char nsha1[20];
+   struct object_id noid;
 };
 
 static int grab_1st_switch(struct object_id *ooid, struct object_id *noid,
@@ -1387,7 +1387,7 @@ static int grab_1st_switch(struct object_id *ooid, struct 
object_id *noid,
return 0;
target += strlen(" to ");
strbuf_reset(>buf);
-   hashcpy(cb->nsha1, noid->hash);
+   oidcpy(>noid, noid);
end = strchrnul(target, '\n');
strbuf_add(>buf, target, end - target);
if (!strcmp(cb->buf.buf, "HEAD")) {
@@ -1402,7 +1402,7 @@ static void wt_status_get_detached_from(struct 
wt_status_state *state)
 {
struct grab_1st_switch_cbdata cb;
struct commit *commit;
-   unsigned char sha1[20];
+   struct object_id oid;
char *ref = NULL;
 
strbuf_init(, 0);
@@ -1411,22 +1411,22 @@ static void wt_status_get_detached_from(struct 
wt_status_state *state)
return;
}
 
-   if (dwim_ref(cb.buf.buf, cb.buf.len, sha1, ) == 1 &&
+   if (dwim_ref(cb.buf.buf, cb.buf.len, oid.hash, ) == 1 &&
/* sha1 is a commit? match without further lookup */
-   (!hashcmp(cb.nsha1, sha1) ||
+   (!oidcmp(, ) ||
 /* perhaps sha1 is a tag, try to dereference to a commit */
-((commit = lookup_commit_reference_gently(sha1, 1)) != NULL &&
- !hashcmp(cb.nsha1, commit->object.oid.hash {
+((commit = lookup_commit_reference_gently(oid.hash, 1)) != NULL &&
+ !oidcmp(, >object.oid {
const char *from = ref;
if (!skip_prefix(from, "refs/tags/", ))
skip_prefix(from, "refs/remotes/", );
state->detached_from = xstrdup(from);
} else
state->detached_from =
-   xstrdup(find_unique_abbrev(cb.nsha1, DEFAULT_ABBREV));
-   hashcpy(state->detached_sha1, cb.nsha1);
-   state->detached_at = !get_sha1("HEAD", sha1) &&
-!hashcmp(sha1, state->detached_sha1);
+   xstrdup(find_unique_abbrev(cb.noid.hash, 
DEFAULT_ABBREV));
+   hashcpy(state->detached_sha1, cb.noid.hash);
+   state->detached_at = !get_oid("HEAD", ) &&
+!hashcmp(oid.hash, state->detached_sha1);
 
free(ref);
strbuf_release();
@@ -1476,22 +1476,22 @@ void wt_status_get_state(struct wt_status_state *state,
 int get_detached_from)
 {
struct stat st;
-   unsigned char sha1[20];
+   struct object_id oid;
 
if 

[PATCH v3 05/19] builtin/fmt-merge-message: convert to struct object_id

[brian m. carlson]

Convert most of the code to use struct object_id, including struct
origin_data and struct merge_parents.  Convert several instances of
hardcoded numbers into references to GIT_SHA1_HEXSZ.

Signed-off-by: brian m. carlson 
---
 builtin/fmt-merge-msg.c | 70 -
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index efab62fd85..6faa3c0d24 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -41,7 +41,7 @@ struct src_data {
 };
 
 struct origin_data {
-   unsigned char sha1[20];
+   struct object_id oid;
unsigned is_local_branch:1;
 };
 
@@ -59,8 +59,8 @@ static struct string_list origins = STRING_LIST_INIT_DUP;
 struct merge_parents {
int alloc, nr;
struct merge_parent {
-   unsigned char given[20];
-   unsigned char commit[20];
+   struct object_id given;
+   struct object_id commit;
unsigned char used;
} *item;
 };
@@ -70,14 +70,14 @@ struct merge_parents {
  * hundreds of heads at a time anyway.
  */
 static struct merge_parent *find_merge_parent(struct merge_parents *table,
- unsigned char *given,
- unsigned char *commit)
+ struct object_id *given,
+ struct object_id *commit)
 {
int i;
for (i = 0; i < table->nr; i++) {
-   if (given && hashcmp(table->item[i].given, given))
+   if (given && oidcmp(>item[i].given, given))
continue;
-   if (commit && hashcmp(table->item[i].commit, commit))
+   if (commit && oidcmp(>item[i].commit, commit))
continue;
return >item[i];
}
@@ -85,14 +85,14 @@ static struct merge_parent *find_merge_parent(struct 
merge_parents *table,
 }
 
 static void add_merge_parent(struct merge_parents *table,
-unsigned char *given,
-unsigned char *commit)
+struct object_id *given,
+struct object_id *commit)
 {
if (table->nr && find_merge_parent(table, given, commit))
return;
ALLOC_GROW(table->item, table->nr + 1, table->alloc);
-   hashcpy(table->item[table->nr].given, given);
-   hashcpy(table->item[table->nr].commit, commit);
+   oidcpy(>item[table->nr].given, given);
+   oidcpy(>item[table->nr].commit, commit);
table->item[table->nr].used = 0;
table->nr++;
 }
@@ -106,30 +106,30 @@ static int handle_line(char *line, struct merge_parents 
*merge_parents)
struct src_data *src_data;
struct string_list_item *item;
int pulling_head = 0;
-   unsigned char sha1[20];
+   struct object_id oid;
 
-   if (len < 43 || line[40] != '\t')
+   if (len < GIT_SHA1_HEXSZ + 3 || line[GIT_SHA1_HEXSZ] != '\t')
return 1;
 
-   if (starts_with(line + 41, "not-for-merge"))
+   if (starts_with(line + GIT_SHA1_HEXSZ + 1, "not-for-merge"))
return 0;
 
-   if (line[41] != '\t')
+   if (line[GIT_SHA1_HEXSZ + 1] != '\t')
return 2;
 
-   i = get_sha1_hex(line, sha1);
+   i = get_oid_hex(line, );
if (i)
return 3;
 
-   if (!find_merge_parent(merge_parents, sha1, NULL))
+   if (!find_merge_parent(merge_parents, , NULL))
return 0; /* subsumed by other parents */
 
origin_data = xcalloc(1, sizeof(struct origin_data));
-   hashcpy(origin_data->sha1, sha1);
+   oidcpy(_data->oid, );
 
if (line[len - 1] == '\n')
line[len - 1] = 0;
-   line += 42;
+   line += GIT_SHA1_HEXSZ + 2;
 
/*
 * At this point, line points at the beginning of comment e.g.
@@ -338,10 +338,10 @@ static void shortlog(const char *name,
struct string_list committers = STRING_LIST_INIT_DUP;
int flags = UNINTERESTING | TREESAME | SEEN | SHOWN | ADDED;
struct strbuf sb = STRBUF_INIT;
-   const unsigned char *sha1 = origin_data->sha1;
+   const struct object_id *oid = _data->oid;
int limit = opts->shortlog_len;
 
-   branch = deref_tag(parse_object(sha1), sha1_to_hex(sha1), 40);
+   branch = deref_tag(parse_object(oid->hash), oid_to_hex(oid), 
GIT_SHA1_HEXSZ);
if (!branch || branch->type != OBJ_COMMIT)
return;
 
@@ -531,7 +531,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 }
 
 static void find_merge_parents(struct merge_parents *result,
-  struct strbuf *in, unsigned char *head)
+  struct strbuf *in, struct object_id *head)
 {
struct commit_list *parents;

[PATCH v3 08/19] builtin/clone: convert to struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson 
---
 builtin/clone.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 3f63edbbf9..b4c929bb8a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -681,7 +681,7 @@ static void update_head(const struct ref *our, const struct 
ref *remote,
 
 static int checkout(int submodule_progress)
 {
-   unsigned char sha1[20];
+   struct object_id oid;
char *head;
struct lock_file *lock_file;
struct unpack_trees_options opts;
@@ -692,7 +692,7 @@ static int checkout(int submodule_progress)
if (option_no_checkout)
return 0;
 
-   head = resolve_refdup("HEAD", RESOLVE_REF_READING, sha1, NULL);
+   head = resolve_refdup("HEAD", RESOLVE_REF_READING, oid.hash, NULL);
if (!head) {
warning(_("remote HEAD refers to nonexistent ref, "
  "unable to checkout.\n"));
@@ -700,7 +700,7 @@ static int checkout(int submodule_progress)
}
if (!strcmp(head, "HEAD")) {
if (advice_detached_head)
-   detach_advice(sha1_to_hex(sha1));
+   detach_advice(oid_to_hex());
} else {
if (!starts_with(head, "refs/heads/"))
die(_("HEAD not found below refs/heads!"));
@@ -721,7 +721,7 @@ static int checkout(int submodule_progress)
opts.src_index = _index;
opts.dst_index = _index;
 
-   tree = parse_tree_indirect(sha1);
+   tree = parse_tree_indirect(oid.hash);
parse_tree(tree);
init_tree_desc(, tree->buffer, tree->size);
if (unpack_trees(1, , ) < 0)
@@ -731,7 +731,7 @@ static int checkout(int submodule_progress)
die(_("unable to write new index file"));
 
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
-  sha1_to_hex(sha1), "1", NULL);
+  oid_to_hex(), "1", NULL);
 
if (!err && option_recursive) {
struct argv_array args = ARGV_ARRAY_INIT;
-- 
2.11.0

[PATCH v3 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();
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(, GREP_SOURCE_SHA1, pathbuf.buf, path, 
sha1);
+   grep_source_init(, GREP_SOURCE_SHA1, pathbuf.buf, path, oid);
strbuf_release();
hit = grep_source(opt, );
 
@@ -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, >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, , 
);
+   data = lock_and_read_oid_file(entry.oid, , );
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, >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, )) {
-   struct object *object = parse_object_or_die(sha1, arg);
+   if (!get_sha1_with_context(arg, 0, oid.hash, )) {
+   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, , oc.mode, 
oc.path);
-- 
2.11.0

[PATCH v3 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(>peeled, peeled ? peeled : >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(, sha1hash(peeled), peeled);
+   return hashmap_get_from_hash(, 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, )) {
+   if (replace_name(e, prio, oid, )) {
if (!e) {
e = xmalloc(sizeof(struct commit_name));
-   hashcpy(e->peeled, peeled);
-   hashmap_entry_init(e, sha1hash(peeled));
+   oidcpy(>peeled, peeled);
+   hashmap_entry_init(e, sha1hash(peeled->hash));
hashmap_add(, e);
e->path = NULL;
}
e->tag = tag;
e->prio = prio;
e->name_checked = 0;
-   hashcpy(e->sha1, sha1);
+   oidcpy(>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, , 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_one)

[PATCH v3 04/19] builtin/fast-export: convert to struct object_id

[brian m. carlson]

In addition to converting to struct object_id, write some hardcoded
buffer sizes in terms of GIT_SHA1_RAWSZ.

Signed-off-by: brian m. carlson 
---
 builtin/fast-export.c | 58 +--
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 1e815b5577..e0220630d0 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -212,7 +212,7 @@ static char *anonymize_blob(unsigned long *size)
return strbuf_detach(, NULL);
 }
 
-static void export_blob(const unsigned char *sha1)
+static void export_blob(const struct object_id *oid)
 {
unsigned long size;
enum object_type type;
@@ -223,34 +223,34 @@ static void export_blob(const unsigned char *sha1)
if (no_data)
return;
 
-   if (is_null_sha1(sha1))
+   if (is_null_oid(oid))
return;
 
-   object = lookup_object(sha1);
+   object = lookup_object(oid->hash);
if (object && object->flags & SHOWN)
return;
 
if (anonymize) {
buf = anonymize_blob();
-   object = (struct object *)lookup_blob(sha1);
+   object = (struct object *)lookup_blob(oid->hash);
eaten = 0;
} else {
-   buf = read_sha1_file(sha1, , );
+   buf = read_sha1_file(oid->hash, , );
if (!buf)
-   die ("Could not read blob %s", sha1_to_hex(sha1));
-   if (check_sha1_signature(sha1, buf, size, typename(type)) < 0)
-   die("sha1 mismatch in blob %s", sha1_to_hex(sha1));
-   object = parse_object_buffer(sha1, type, size, buf, );
+   die ("Could not read blob %s", oid_to_hex(oid));
+   if (check_sha1_signature(oid->hash, buf, size, typename(type)) 
< 0)
+   die("sha1 mismatch in blob %s", oid_to_hex(oid));
+   object = parse_object_buffer(oid->hash, type, size, buf, 
);
}
 
if (!object)
-   die("Could not read blob %s", sha1_to_hex(sha1));
+   die("Could not read blob %s", oid_to_hex(oid));
 
mark_next_object(object);
 
printf("blob\nmark :%"PRIu32"\ndata %lu\n", last_idnum, size);
if (size && fwrite(buf, size, 1, stdout) != 1)
-   die_errno ("Could not write blob '%s'", sha1_to_hex(sha1));
+   die_errno ("Could not write blob '%s'", oid_to_hex(oid));
printf("\n");
 
show_progress();
@@ -323,19 +323,19 @@ static void print_path(const char *path)
}
 }
 
-static void *generate_fake_sha1(const void *old, size_t *len)
+static void *generate_fake_oid(const void *old, size_t *len)
 {
static uint32_t counter = 1; /* avoid null sha1 */
-   unsigned char *out = xcalloc(20, 1);
-   put_be32(out + 16, counter++);
+   unsigned char *out = xcalloc(GIT_SHA1_RAWSZ, 1);
+   put_be32(out + GIT_SHA1_RAWSZ - 4, counter++);
return out;
 }
 
-static const unsigned char *anonymize_sha1(const unsigned char *sha1)
+static const unsigned char *anonymize_sha1(const struct object_id *oid)
 {
static struct hashmap sha1s;
-   size_t len = 20;
-   return anonymize_mem(, generate_fake_sha1, sha1, );
+   size_t len = GIT_SHA1_RAWSZ;
+   return anonymize_mem(, generate_fake_oid, oid, );
 }
 
 static void show_filemodify(struct diff_queue_struct *q,
@@ -383,7 +383,7 @@ static void show_filemodify(struct diff_queue_struct *q,
if (no_data || S_ISGITLINK(spec->mode))
printf("M %06o %s ", spec->mode,
   sha1_to_hex(anonymize ?
-  
anonymize_sha1(spec->oid.hash) :
+  anonymize_sha1(>oid) :
   spec->oid.hash));
else {
struct object *object = 
lookup_object(spec->oid.hash);
@@ -572,7 +572,7 @@ static void handle_commit(struct commit *commit, struct 
rev_info *rev)
/* Export the referenced blobs, and remember the marks. */
for (i = 0; i < diff_queued_diff.nr; i++)
if (!S_ISGITLINK(diff_queued_diff.queue[i]->two->mode))
-   export_blob(diff_queued_diff.queue[i]->two->oid.hash);
+   export_blob(_queued_diff.queue[i]->two->oid);
 
refname = commit->util;
if (anonymize) {
@@ -797,14 +797,14 @@ static void get_tags_and_duplicates(struct 
rev_cmdline_info *info)
 
for (i = 0; i < info->nr; i++) {
struct rev_cmdline_entry *e = info->rev + i;
-   unsigned char sha1[20];
+   struct object_id oid;
struct commit *commit;
char *full_name;
 
if (e->flags & 

[PATCH v3 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.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 = _commit->object.oid;
 
if (!msg)
strbuf_addstr(_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();
-   diff_tree_sha1(head, new_head, "", );
+   diff_tree_sha1(head->hash, new_head->hash, "", );
diffcore_std();
diff_flush();
}
@@ -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(, remote);
remote = bname.buf;
 
-   memset(branch_head, 0, sizeof(branch_head));
+   oidclr(_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, _ref) > 0) {
+   if (dwim_ref(remote, strlen(remote), branch_head.hash, _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(_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(_head), remote);
goto cleanup;
}
if (starts_with(found_ref, "refs/remotes/")) {
strbuf_addf(msg, "%s\t\tremote-tracking branch '%s' of 
.\n",
-

[PATCH v3 00/19] object_id part 6

[brian m. carlson]

This is another series in the continuing conversion to struct object_id.

This series converts more of the builtin directory and some of the refs
code to use struct object_id. Additionally, it implements an
nth_packed_object_oid function which provides a struct object_id version
of the nth_packed_object function, and a parse_oid_hex function that
makes parsing easier.

The patch to use parse_oid_hex in the refs code has been split out into
its own patch, just because I'm wary of that code and potentially
breaking things, and I want it to be easy to revert in case things go
wrong.  I have no reason to believe it is anything other than fully
functional, however.

The conflicts in pu with v2 are probably still present, but I expect the
resolution should be easy (and I suspect Junio uses git rerere, so they'll be
even easier).

Changes from v2:
* Fix misnamed function in commit message.
* Improve parameter name of parse_oid_hex.
* Improve docstring of parse_oid_hex.
* Remove needless variable.
* Rebase on master.

Changes from v1:
* Implement parse_oid_hex and use it.
* Make nth_packed_object_oid take a variable into which to store the
  object ID.  This avoids concerns about unsafe casts.
* Rebase on master.

brian m. carlson (19):
  builtin/commit: convert to struct object_id
  builtin/diff-tree: convert to struct object_id
  builtin/describe: convert to struct object_id
  builtin/fast-export: convert to struct object_id
  builtin/fmt-merge-message: convert to struct object_id
  builtin/grep: convert to struct object_id
  builtin/branch: convert to struct object_id
  builtin/clone: convert to struct object_id
  builtin/merge: convert to struct object_id
  Convert remaining callers of resolve_refdup to object_id
  builtin/replace: convert to struct object_id
  reflog-walk: convert struct reflog_info to struct object_id
  refs: convert each_reflog_ent_fn to struct object_id
  hex: introduce parse_oid_hex
  refs: simplify parsing of reflog entries
  sha1_file: introduce an nth_packed_object_oid function
  Convert object iteration callbacks to struct object_id
  builtin/merge-base: convert to struct object_id
  wt-status: convert to struct object_id

 builtin/branch.c|  26 +-
 builtin/cat-file.c  |   8 +--
 builtin/clone.c |  10 ++--
 builtin/commit.c|  46 -
 builtin/count-objects.c |   4 +-
 builtin/describe.c  |  50 +-
 builtin/diff-tree.c |  38 +++---
 builtin/fast-export.c   |  58 ++---
 builtin/fmt-merge-msg.c |  70 -
 builtin/fsck.c  |  40 +++
 builtin/grep.c  |  24 -
 builtin/merge-base.c|  30 +--
 builtin/merge.c | 134 
 builtin/notes.c |  18 +++
 builtin/pack-objects.c  |   6 +--
 builtin/prune-packed.c  |   4 +-
 builtin/prune.c |   8 +--
 builtin/receive-pack.c  |   4 +-
 builtin/reflog.c|   2 +-
 builtin/replace.c   | 112 
 cache.h |  19 ++-
 hex.c   |   8 +++
 reachable.c |  30 +--
 ref-filter.c|   4 +-
 reflog-walk.c   |  26 +-
 refs.c  |  24 -
 refs.h  |   2 +-
 refs/files-backend.c|  29 ++-
 revision.c  |  12 ++---
 sha1_file.c |  27 +++---
 sha1_name.c |   2 +-
 transport.c |   4 +-
 wt-status.c |  52 +--
 33 files changed, 483 insertions(+), 448 deletions(-)

-- 
2.11.0

[PATCH v3 10/19] Convert remaining callers of resolve_refdup to object_id

[brian m. carlson]

There are a few leaf functions in various files that call
resolve_refdup.  Convert these functions to use struct object_id
internally to prepare for transitioning resolve_refdup itself.

Signed-off-by: brian m. carlson 
---
 builtin/notes.c| 18 +-
 builtin/receive-pack.c |  4 ++--
 ref-filter.c   |  4 ++--
 reflog-walk.c  | 12 ++--
 transport.c|  4 ++--
 wt-status.c|  4 ++--
 6 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 5248a9bad8..8c569a49a0 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -693,7 +693,7 @@ static int merge_abort(struct notes_merge_options *o)
 static int merge_commit(struct notes_merge_options *o)
 {
struct strbuf msg = STRBUF_INIT;
-   unsigned char sha1[20], parent_sha1[20];
+   struct object_id oid, parent_oid;
struct notes_tree *t;
struct commit *partial;
struct pretty_print_context pretty_ctx;
@@ -705,27 +705,27 @@ static int merge_commit(struct notes_merge_options *o)
 * and target notes ref from .git/NOTES_MERGE_REF.
 */
 
-   if (get_sha1("NOTES_MERGE_PARTIAL", sha1))
+   if (get_oid("NOTES_MERGE_PARTIAL", ))
die(_("failed to read ref NOTES_MERGE_PARTIAL"));
-   else if (!(partial = lookup_commit_reference(sha1)))
+   else if (!(partial = lookup_commit_reference(oid.hash)))
die(_("could not find commit from NOTES_MERGE_PARTIAL."));
else if (parse_commit(partial))
die(_("could not parse commit from NOTES_MERGE_PARTIAL."));
 
if (partial->parents)
-   hashcpy(parent_sha1, partial->parents->item->object.oid.hash);
+   oidcpy(_oid, >parents->item->object.oid);
else
-   hashclr(parent_sha1);
+   oidclr(_oid);
 
t = xcalloc(1, sizeof(struct notes_tree));
init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0);
 
o->local_ref = local_ref_to_free =
-   resolve_refdup("NOTES_MERGE_REF", 0, sha1, NULL);
+   resolve_refdup("NOTES_MERGE_REF", 0, oid.hash, NULL);
if (!o->local_ref)
die(_("failed to resolve NOTES_MERGE_REF"));
 
-   if (notes_merge_commit(o, t, partial, sha1))
+   if (notes_merge_commit(o, t, partial, oid.hash))
die(_("failed to finalize notes merge"));
 
/* Reuse existing commit message in reflog message */
@@ -733,8 +733,8 @@ static int merge_commit(struct notes_merge_options *o)
format_commit_message(partial, "%s", , _ctx);
strbuf_trim();
strbuf_insert(, 0, "notes: ", 7);
-   update_ref(msg.buf, o->local_ref, sha1,
-  is_null_sha1(parent_sha1) ? NULL : parent_sha1,
+   update_ref(msg.buf, o->local_ref, oid.hash,
+  is_null_oid(_oid) ? NULL : parent_oid.hash,
   0, UPDATE_REFS_DIE_ON_ERR);
 
free_notes(t);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 1dbb8a0692..7966f4f4df 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1414,7 +1414,7 @@ static void execute_commands(struct command *commands,
 {
struct check_connected_options opt = CHECK_CONNECTED_INIT;
struct command *cmd;
-   unsigned char sha1[20];
+   struct object_id oid;
struct iterate_data data;
struct async muxer;
int err_fd = 0;
@@ -1471,7 +1471,7 @@ static void execute_commands(struct command *commands,
check_aliased_updates(commands);
 
free(head_name_to_free);
-   head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
+   head_name = head_name_to_free = resolve_refdup("HEAD", 0, oid.hash, 
NULL);
 
if (use_atomic)
execute_commands_atomic(commands, si);
diff --git a/ref-filter.c b/ref-filter.c
index 3820b21cc7..f0de30e2ef 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -961,9 +961,9 @@ static void populate_value(struct ref_array_item *ref)
ref->value = xcalloc(used_atom_cnt, sizeof(struct atom_value));
 
if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
-   unsigned char unused1[20];
+   struct object_id unused1;
ref->symref = resolve_refdup(ref->refname, RESOLVE_REF_READING,
-unused1, NULL);
+unused1.hash, NULL);
if (!ref->symref)
ref->symref = "";
}
diff --git a/reflog-walk.c b/reflog-walk.c
index a246af2767..f98748e2ae 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -45,11 +45,11 @@ static struct complete_reflogs *read_complete_reflog(const 
char *ref)
reflogs->ref = xstrdup(ref);
for_each_reflog_ent(ref, read_one_reflog, reflogs);
if (reflogs->nr == 0) {
-   unsigned 

[PATCH v3 15/19] refs: simplify parsing of reflog entries

[brian m. carlson]

The current code for reflog entries uses a lot of hard-coded constants,
making it hard to read and modify.  Use parse_oid_hex and two temporary
variables to simplify the code and reduce the use of magic constants.

Signed-off-by: brian m. carlson 
---
 refs/files-backend.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index d7a5fd2a7c..fea20e99fe 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3117,12 +3117,13 @@ static int show_one_reflog_ent(struct strbuf *sb, 
each_reflog_ent_fn fn, void *c
char *email_end, *message;
unsigned long timestamp;
int tz;
+   const char *p = sb->buf;
 
/* old SP new SP name  SP time TAB msg LF */
-   if (sb->len < 83 || sb->buf[sb->len - 1] != '\n' ||
-   get_oid_hex(sb->buf, ) || sb->buf[40] != ' ' ||
-   get_oid_hex(sb->buf + 41, ) || sb->buf[81] != ' ' ||
-   !(email_end = strchr(sb->buf + 82, '>')) ||
+   if (!sb->len || sb->buf[sb->len - 1] != '\n' ||
+   parse_oid_hex(p, , ) || *p++ != ' ' ||
+   parse_oid_hex(p, , ) || *p++ != ' ' ||
+   !(email_end = strchr(p, '>')) ||
email_end[1] != ' ' ||
!(timestamp = strtoul(email_end + 2, , 10)) ||
!message || message[0] != ' ' ||
@@ -3136,7 +3137,7 @@ static int show_one_reflog_ent(struct strbuf *sb, 
each_reflog_ent_fn fn, void *c
message += 6;
else
message += 7;
-   return fn(, , sb->buf + 82, timestamp, tz, message, cb_data);
+   return fn(, , p, timestamp, tz, message, cb_data);
 }
 
 static char *find_beginning_of_line(char *bob, char *scan)
-- 
2.11.0

[PATCH v3 11/19] builtin/replace: convert to struct object_id

[brian m. carlson]

Convert various uses of unsigned char [20] to struct object_id.  Rename
replace_object_sha1 to replace_object_oid.  Finally, specify a constant
in terms of GIT_SHA1_HEXSZ.

Signed-off-by: brian m. carlson 
---
 builtin/replace.c | 112 +++---
 1 file changed, 56 insertions(+), 56 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index b58c714cb8..f7716a5472 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -88,78 +88,78 @@ static int list_replace_refs(const char *pattern, const 
char *format)
 }
 
 typedef int (*each_replace_name_fn)(const char *name, const char *ref,
-   const unsigned char *sha1);
+   const struct object_id *oid);
 
 static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
 {
const char **p, *full_hex;
char ref[PATH_MAX];
int had_error = 0;
-   unsigned char sha1[20];
+   struct object_id oid;
 
for (p = argv; *p; p++) {
-   if (get_sha1(*p, sha1)) {
+   if (get_oid(*p, )) {
error("Failed to resolve '%s' as a valid ref.", *p);
had_error = 1;
continue;
}
-   full_hex = sha1_to_hex(sha1);
+   full_hex = oid_to_hex();
snprintf(ref, sizeof(ref), "%s%s", git_replace_ref_base, 
full_hex);
/* read_ref() may reuse the buffer */
full_hex = ref + strlen(git_replace_ref_base);
-   if (read_ref(ref, sha1)) {
+   if (read_ref(ref, oid.hash)) {
error("replace ref '%s' not found.", full_hex);
had_error = 1;
continue;
}
-   if (fn(full_hex, ref, sha1))
+   if (fn(full_hex, ref, ))
had_error = 1;
}
return had_error;
 }
 
 static int delete_replace_ref(const char *name, const char *ref,
- const unsigned char *sha1)
+ const struct object_id *oid)
 {
-   if (delete_ref(ref, sha1, 0))
+   if (delete_ref(ref, oid->hash, 0))
return 1;
printf("Deleted replace ref '%s'\n", name);
return 0;
 }
 
-static void check_ref_valid(unsigned char object[20],
-   unsigned char prev[20],
+static void check_ref_valid(struct object_id *object,
+   struct object_id *prev,
char *ref,
int ref_size,
int force)
 {
if (snprintf(ref, ref_size,
 "%s%s", git_replace_ref_base,
-sha1_to_hex(object)) > ref_size - 1)
+oid_to_hex(object)) > ref_size - 1)
die("replace ref name too long: %.*s...", 50, ref);
if (check_refname_format(ref, 0))
die("'%s' is not a valid ref name.", ref);
 
-   if (read_ref(ref, prev))
-   hashclr(prev);
+   if (read_ref(ref, prev->hash))
+   oidclr(prev);
else if (!force)
die("replace ref '%s' already exists", ref);
 }
 
-static int replace_object_sha1(const char *object_ref,
-  unsigned char object[20],
+static int replace_object_oid(const char *object_ref,
+  struct object_id *object,
   const char *replace_ref,
-  unsigned char repl[20],
+  struct object_id *repl,
   int force)
 {
-   unsigned char prev[20];
+   struct object_id prev;
enum object_type obj_type, repl_type;
char ref[PATH_MAX];
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
-   obj_type = sha1_object_info(object, NULL);
-   repl_type = sha1_object_info(repl, NULL);
+   obj_type = sha1_object_info(object->hash, NULL);
+   repl_type = sha1_object_info(repl->hash, NULL);
if (!force && obj_type != repl_type)
die("Objects must be of the same type.\n"
"'%s' points to a replaced object of type '%s'\n"
@@ -167,11 +167,11 @@ static int replace_object_sha1(const char *object_ref,
object_ref, typename(obj_type),
replace_ref, typename(repl_type));
 
-   check_ref_valid(object, prev, ref, sizeof(ref), force);
+   check_ref_valid(object, , ref, sizeof(ref), force);
 
transaction = ref_transaction_begin();
if (!transaction ||
-   ref_transaction_update(transaction, ref, repl, prev,
+   ref_transaction_update(transaction, ref, repl->hash, prev.hash,
   0, NULL, ) ||

[PATCH v3 02/19] builtin/diff-tree: convert to struct object_id

[brian m. carlson]

Convert most leaf functions to struct object_id.  Rewrite several
hardcoded numbers in terms of GIT_SHA1_HEXSZ, using an intermediate
variable where that makes sense.

Signed-off-by: brian m. carlson 
---
 builtin/diff-tree.c | 38 --
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 8ce00480cd..1f1573bb2a 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -7,9 +7,9 @@
 
 static struct rev_info log_tree_opt;
 
-static int diff_tree_commit_sha1(const unsigned char *sha1)
+static int diff_tree_commit_sha1(const struct object_id *oid)
 {
-   struct commit *commit = lookup_commit_reference(sha1);
+   struct commit *commit = lookup_commit_reference(oid->hash);
if (!commit)
return -1;
return log_tree_commit(_tree_opt, commit);
@@ -18,22 +18,22 @@ static int diff_tree_commit_sha1(const unsigned char *sha1)
 /* Diff one or more commits. */
 static int stdin_diff_commit(struct commit *commit, char *line, int len)
 {
-   unsigned char sha1[20];
-   if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
+   struct object_id oid;
+   if (isspace(line[GIT_SHA1_HEXSZ]) && 
!get_oid_hex(line+GIT_SHA1_HEXSZ+1, )) {
/* Graft the fake parents locally to the commit */
-   int pos = 41;
+   int pos = GIT_SHA1_HEXSZ + 1;
struct commit_list **pptr;
 
/* Free the real parent list */
free_commit_list(commit->parents);
commit->parents = NULL;
pptr = &(commit->parents);
-   while (line[pos] && !get_sha1_hex(line + pos, sha1)) {
-   struct commit *parent = lookup_commit(sha1);
+   while (line[pos] && !get_oid_hex(line + pos, )) {
+   struct commit *parent = lookup_commit(oid.hash);
if (parent) {
pptr = _list_insert(parent, pptr)->next;
}
-   pos += 41;
+   pos += GIT_SHA1_HEXSZ + 1;
}
}
return log_tree_commit(_tree_opt, commit);
@@ -42,11 +42,13 @@ static int stdin_diff_commit(struct commit *commit, char 
*line, int len)
 /* Diff two trees. */
 static int stdin_diff_trees(struct tree *tree1, char *line, int len)
 {
-   unsigned char sha1[20];
+   struct object_id oid;
struct tree *tree2;
-   if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
+   const int chunksz = GIT_SHA1_HEXSZ + 1;
+   if (len != 2 * chunksz || !isspace(line[chunksz-1]) ||
+   get_sha1_hex(line + chunksz, oid.hash))
return error("Need exactly two trees, separated by a space");
-   tree2 = lookup_tree(sha1);
+   tree2 = lookup_tree(oid.hash);
if (!tree2 || parse_tree(tree2))
return -1;
printf("%s %s\n", oid_to_hex(>object.oid),
@@ -60,15 +62,15 @@ static int stdin_diff_trees(struct tree *tree1, char *line, 
int len)
 static int diff_tree_stdin(char *line)
 {
int len = strlen(line);
-   unsigned char sha1[20];
+   struct object_id oid;
struct object *obj;
 
if (!len || line[len-1] != '\n')
return -1;
line[len-1] = 0;
-   if (get_sha1_hex(line, sha1))
+   if (get_oid_hex(line, ))
return -1;
-   obj = parse_object(sha1);
+   obj = parse_object(oid.hash);
if (!obj)
return -1;
if (obj->type == OBJ_COMMIT)
@@ -76,7 +78,7 @@ static int diff_tree_stdin(char *line)
if (obj->type == OBJ_TREE)
return stdin_diff_trees((struct tree *)obj, line, len);
error("Object %s is a %s, not a commit or tree",
- sha1_to_hex(sha1), typename(obj->type));
+ oid_to_hex(), typename(obj->type));
return -1;
 }
 
@@ -141,7 +143,7 @@ int cmd_diff_tree(int argc, const char **argv, const char 
*prefix)
break;
case 1:
tree1 = opt->pending.objects[0].item;
-   diff_tree_commit_sha1(tree1->oid.hash);
+   diff_tree_commit_sha1(>oid);
break;
case 2:
tree1 = opt->pending.objects[0].item;
@@ -164,9 +166,9 @@ int cmd_diff_tree(int argc, const char **argv, const char 
*prefix)
opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |
   DIFF_SETUP_USE_CACHE);
while (fgets(line, sizeof(line), stdin)) {
-   unsigned char sha1[20];
+   struct object_id oid;
 
-   if (get_sha1_hex(line, sha1)) {
+   if (get_oid_hex(line, )) {
fputs(line, stdout);
fflush(stdout);
   

[PATCH v3 17/19] Convert object iteration callbacks to struct object_id

[brian m. carlson]

Convert each_loose_object_fn and each_packed_object_fn to take a pointer
to struct object_id.  Update the various callbacks.  Convert several
40-based constants to use GIT_SHA1_HEXSZ.

Signed-off-by: brian m. carlson 
---
 builtin/cat-file.c  |  8 
 builtin/count-objects.c |  4 ++--
 builtin/fsck.c  | 24 
 builtin/pack-objects.c  |  6 +++---
 builtin/prune-packed.c  |  4 ++--
 builtin/prune.c |  8 
 cache.h |  4 ++--
 reachable.c | 30 +++---
 sha1_file.c | 12 ++--
 9 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 30383e9eb4..8b85cb8cf0 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -409,20 +409,20 @@ static int batch_object_cb(const unsigned char sha1[20], 
void *vdata)
return 0;
 }
 
-static int batch_loose_object(const unsigned char *sha1,
+static int batch_loose_object(const struct object_id *oid,
  const char *path,
  void *data)
 {
-   sha1_array_append(data, sha1);
+   sha1_array_append(data, oid->hash);
return 0;
 }
 
-static int batch_packed_object(const unsigned char *sha1,
+static int batch_packed_object(const struct object_id *oid,
   struct packed_git *pack,
   uint32_t pos,
   void *data)
 {
-   sha1_array_append(data, sha1);
+   sha1_array_append(data, oid->hash);
return 0;
 }
 
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index a04b4f2ef3..acb05940fc 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -53,7 +53,7 @@ static void loose_garbage(const char *path)
report_garbage(PACKDIR_FILE_GARBAGE, path);
 }
 
-static int count_loose(const unsigned char *sha1, const char *path, void *data)
+static int count_loose(const struct object_id *oid, const char *path, void 
*data)
 {
struct stat st;
 
@@ -62,7 +62,7 @@ static int count_loose(const unsigned char *sha1, const char 
*path, void *data)
else {
loose_size += on_disk_bytes(st);
loose++;
-   if (verbose && has_sha1_pack(sha1))
+   if (verbose && has_sha1_pack(oid->hash))
packed_loose++;
}
return 0;
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 9b37606858..f76e4163ab 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -491,7 +491,7 @@ static void get_default_heads(void)
}
 }
 
-static struct object *parse_loose_object(const unsigned char *sha1,
+static struct object *parse_loose_object(const struct object_id *oid,
 const char *path)
 {
struct object *obj;
@@ -500,27 +500,27 @@ static struct object *parse_loose_object(const unsigned 
char *sha1,
unsigned long size;
int eaten;
 
-   if (read_loose_object(path, sha1, , , ) < 0)
+   if (read_loose_object(path, oid->hash, , , ) < 0)
return NULL;
 
if (!contents && type != OBJ_BLOB)
die("BUG: read_loose_object streamed a non-blob");
 
-   obj = parse_object_buffer(sha1, type, size, contents, );
+   obj = parse_object_buffer(oid->hash, type, size, contents, );
 
if (!eaten)
free(contents);
return obj;
 }
 
-static int fsck_loose(const unsigned char *sha1, const char *path, void *data)
+static int fsck_loose(const struct object_id *oid, const char *path, void 
*data)
 {
-   struct object *obj = parse_loose_object(sha1, path);
+   struct object *obj = parse_loose_object(oid, path);
 
if (!obj) {
errors_found |= ERROR_OBJECT;
error("%s: object corrupt or missing: %s",
- sha1_to_hex(sha1), path);
+ oid_to_hex(oid), path);
return 0; /* keep checking other objects */
}
 
@@ -619,26 +619,26 @@ static int fsck_cache_tree(struct cache_tree *it)
return err;
 }
 
-static void mark_object_for_connectivity(const unsigned char *sha1)
+static void mark_object_for_connectivity(const struct object_id *oid)
 {
-   struct object *obj = lookup_unknown_object(sha1);
+   struct object *obj = lookup_unknown_object(oid->hash);
obj->flags |= HAS_OBJ;
 }
 
-static int mark_loose_for_connectivity(const unsigned char *sha1,
+static int mark_loose_for_connectivity(const struct object_id *oid,
   const char *path,
   void *data)
 {
-   mark_object_for_connectivity(sha1);
+   mark_object_for_connectivity(oid);
return 0;
 }
 
-static int mark_packed_for_connectivity(const unsigned char *sha1,
+static int mark_packed_for_connectivity(const struct object_id *oid,
 

[PATCH v3 18/19] builtin/merge-base: convert to struct object_id

[brian m. carlson]

Convert the remaining uses of unsigned char [20] to struct object_id.

Signed-off-by: brian m. carlson 
---
 builtin/merge-base.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index db95bc29cf..cfe2a796f8 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -36,12 +36,12 @@ static const char * const merge_base_usage[] = {
 
 static struct commit *get_commit_reference(const char *arg)
 {
-   unsigned char revkey[20];
+   struct object_id revkey;
struct commit *r;
 
-   if (get_sha1(arg, revkey))
+   if (get_oid(arg, ))
die("Not a valid object name %s", arg);
-   r = lookup_commit_reference(revkey);
+   r = lookup_commit_reference(revkey.hash);
if (!r)
die("Not a valid commit name %s", arg);
 
@@ -113,14 +113,14 @@ struct rev_collect {
unsigned int initial : 1;
 };
 
-static void add_one_commit(unsigned char *sha1, struct rev_collect *revs)
+static void add_one_commit(struct object_id *oid, struct rev_collect *revs)
 {
struct commit *commit;
 
-   if (is_null_sha1(sha1))
+   if (is_null_oid(oid))
return;
 
-   commit = lookup_commit(sha1);
+   commit = lookup_commit(oid->hash);
if (!commit ||
(commit->object.flags & TMP_MARK) ||
parse_commit(commit))
@@ -139,15 +139,15 @@ static int collect_one_reflog_ent(struct object_id *ooid, 
struct object_id *noid
 
if (revs->initial) {
revs->initial = 0;
-   add_one_commit(ooid->hash, revs);
+   add_one_commit(ooid, revs);
}
-   add_one_commit(noid->hash, revs);
+   add_one_commit(noid, revs);
return 0;
 }
 
 static int handle_fork_point(int argc, const char **argv)
 {
-   unsigned char sha1[20];
+   struct object_id oid;
char *refname;
const char *commitname;
struct rev_collect revs;
@@ -155,7 +155,7 @@ static int handle_fork_point(int argc, const char **argv)
struct commit_list *bases;
int i, ret = 0;
 
-   switch (dwim_ref(argv[0], strlen(argv[0]), sha1, )) {
+   switch (dwim_ref(argv[0], strlen(argv[0]), oid.hash, )) {
case 0:
die("No such ref: '%s'", argv[0]);
case 1:
@@ -165,16 +165,16 @@ static int handle_fork_point(int argc, const char **argv)
}
 
commitname = (argc == 2) ? argv[1] : "HEAD";
-   if (get_sha1(commitname, sha1))
+   if (get_oid(commitname, ))
die("Not a valid object name: '%s'", commitname);
 
-   derived = lookup_commit_reference(sha1);
+   derived = lookup_commit_reference(oid.hash);
memset(, 0, sizeof(revs));
revs.initial = 1;
for_each_reflog_ent(refname, collect_one_reflog_ent, );
 
-   if (!revs.nr && !get_sha1(refname, sha1))
-   add_one_commit(sha1, );
+   if (!revs.nr && !get_oid(refname, ))
+   add_one_commit(, );
 
for (i = 0; i < revs.nr; i++)
revs.commit[i]->object.flags &= ~TMP_MARK;
-- 
2.11.0

[PATCH v3 07/19] builtin/branch: convert to struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson 
---
 builtin/branch.c | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 9d30f55b0b..faf472ff8f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -32,7 +32,7 @@ static const char * const builtin_branch_usage[] = {
 };
 
 static const char *head;
-static unsigned char head_sha1[20];
+static struct object_id head_oid;
 
 static int branch_use_color = -1;
 static char branch_colors[][COLOR_MAXLEN] = {
@@ -117,13 +117,13 @@ static int branch_merged(int kind, const char *name,
if (kind == FILTER_REFS_BRANCHES) {
struct branch *branch = branch_get(name);
const char *upstream = branch_get_upstream(branch, NULL);
-   unsigned char sha1[20];
+   struct object_id oid;
 
if (upstream &&
(reference_name = reference_name_to_free =
 resolve_refdup(upstream, RESOLVE_REF_READING,
-   sha1, NULL)) != NULL)
-   reference_rev = lookup_commit_reference(sha1);
+   oid.hash, NULL)) != NULL)
+   reference_rev = lookup_commit_reference(oid.hash);
}
if (!reference_rev)
reference_rev = head_rev;
@@ -153,10 +153,10 @@ static int branch_merged(int kind, const char *name,
 }
 
 static int check_branch_commit(const char *branchname, const char *refname,
-  const unsigned char *sha1, struct commit 
*head_rev,
+  const struct object_id *oid, struct commit 
*head_rev,
   int kinds, int force)
 {
-   struct commit *rev = lookup_commit_reference(sha1);
+   struct commit *rev = lookup_commit_reference(oid->hash);
if (!rev) {
error(_("Couldn't look up commit object for '%s'"), refname);
return -1;
@@ -183,7 +183,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
   int quiet)
 {
struct commit *head_rev = NULL;
-   unsigned char sha1[20];
+   struct object_id oid;
char *name = NULL;
const char *fmt;
int i;
@@ -207,7 +207,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
}
 
if (!force) {
-   head_rev = lookup_commit_reference(head_sha1);
+   head_rev = lookup_commit_reference(head_oid.hash);
if (!head_rev)
die(_("Couldn't look up commit object for HEAD"));
}
@@ -235,7 +235,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
RESOLVE_REF_READING
| RESOLVE_REF_NO_RECURSE
| RESOLVE_REF_ALLOW_BAD_NAME,
-   sha1, );
+   oid.hash, );
if (!target) {
error(remote_branch
  ? _("remote-tracking branch '%s' not found.")
@@ -245,13 +245,13 @@ static int delete_branches(int argc, const char **argv, 
int force, int kinds,
}
 
if (!(flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
-   check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
+   check_branch_commit(bname.buf, name, , head_rev, kinds,
force)) {
ret = 1;
goto next;
}
 
-   if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
+   if (delete_ref(name, is_null_oid() ? NULL : oid.hash,
   REF_NODEREF)) {
error(remote_branch
  ? _("Error deleting remote-tracking branch '%s'")
@@ -267,7 +267,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
   bname.buf,
   (flags & REF_ISBROKEN) ? "broken"
   : (flags & REF_ISSYMREF) ? target
-  : find_unique_abbrev(sha1, DEFAULT_ABBREV));
+  : find_unique_abbrev(oid.hash, DEFAULT_ABBREV));
}
delete_branch_config(bname.buf);
 
@@ -693,7 +693,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
 
track = git_branch_track;
 
-   head = resolve_refdup("HEAD", 0, head_sha1, NULL);
+   head = resolve_refdup("HEAD", 0, head_oid.hash, NULL);
if (!head)
die(_("Failed to resolve HEAD as a valid ref."));
if (!strcmp(head, "HEAD"))
-- 
2.11.0

[PATCH v3 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 = _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 = _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 v3 13/19] refs: convert each_reflog_ent_fn to struct object_id

[brian m. carlson]

Make each_reflog_ent_fn take two struct object_id pointers instead of
two pointers to unsigned char.  Convert the various callbacks to use
struct object_id as well.  Also, rename fsck_handle_reflog_sha1 to
fsck_handle_reflog_oid.

Signed-off-by: brian m. carlson 
---
 builtin/fsck.c   | 16 
 builtin/merge-base.c |  6 +++---
 builtin/reflog.c |  2 +-
 reflog-walk.c|  6 +++---
 refs.c   | 24 
 refs.h   |  2 +-
 refs/files-backend.c | 24 
 revision.c   | 12 ++--
 sha1_name.c  |  2 +-
 wt-status.c  |  6 +++---
 10 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 1a5caccd0f..9b37606858 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -396,13 +396,13 @@ static int fsck_obj_buffer(const unsigned char *sha1, 
enum object_type type,
 
 static int default_refs;
 
-static void fsck_handle_reflog_sha1(const char *refname, unsigned char *sha1,
+static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid,
unsigned long timestamp)
 {
struct object *obj;
 
-   if (!is_null_sha1(sha1)) {
-   obj = lookup_object(sha1);
+   if (!is_null_oid(oid)) {
+   obj = lookup_object(oid->hash);
if (obj && (obj->flags & HAS_OBJ)) {
if (timestamp && name_objects)
add_decoration(fsck_walk_options.object_names,
@@ -411,13 +411,13 @@ static void fsck_handle_reflog_sha1(const char *refname, 
unsigned char *sha1,
obj->used = 1;
mark_object_reachable(obj);
} else {
-   error("%s: invalid reflog entry %s", refname, 
sha1_to_hex(sha1));
+   error("%s: invalid reflog entry %s", refname, 
oid_to_hex(oid));
errors_found |= ERROR_REACHABLE;
}
}
 }
 
-static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id 
*noid,
const char *email, unsigned long timestamp, int tz,
const char *message, void *cb_data)
 {
@@ -425,10 +425,10 @@ static int fsck_handle_reflog_ent(unsigned char *osha1, 
unsigned char *nsha1,
 
if (verbose)
fprintf(stderr, "Checking reflog %s->%s\n",
-   sha1_to_hex(osha1), sha1_to_hex(nsha1));
+   oid_to_hex(ooid), oid_to_hex(noid));
 
-   fsck_handle_reflog_sha1(refname, osha1, 0);
-   fsck_handle_reflog_sha1(refname, nsha1, timestamp);
+   fsck_handle_reflog_oid(refname, ooid, 0);
+   fsck_handle_reflog_oid(refname, noid, timestamp);
return 0;
 }
 
diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index b572a37c26..db95bc29cf 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -131,7 +131,7 @@ static void add_one_commit(unsigned char *sha1, struct 
rev_collect *revs)
commit->object.flags |= TMP_MARK;
 }
 
-static int collect_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+static int collect_one_reflog_ent(struct object_id *ooid, struct object_id 
*noid,
  const char *ident, unsigned long timestamp,
  int tz, const char *message, void *cbdata)
 {
@@ -139,9 +139,9 @@ static int collect_one_reflog_ent(unsigned char *osha1, 
unsigned char *nsha1,
 
if (revs->initial) {
revs->initial = 0;
-   add_one_commit(osha1, revs);
+   add_one_commit(ooid->hash, revs);
}
-   add_one_commit(nsha1, revs);
+   add_one_commit(noid->hash, revs);
return 0;
 }
 
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 7a7136e53e..7472775778 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -615,7 +615,7 @@ static int cmd_reflog_expire(int argc, const char **argv, 
const char *prefix)
return status;
 }
 
-static int count_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+static int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
const char *email, unsigned long timestamp, int tz,
const char *message, void *cb_data)
 {
diff --git a/reflog-walk.c b/reflog-walk.c
index fe5be41471..99679f5825 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -19,7 +19,7 @@ struct complete_reflogs {
int nr, alloc;
 };
 
-static int read_one_reflog(unsigned char *osha1, unsigned char *nsha1,
+static int read_one_reflog(struct object_id *ooid, struct object_id *noid,
const char *email, unsigned long timestamp, int tz,
const char *message, void *cb_data)
 {
@@ -28,8 +28,8 @@ static int read_one_reflog(unsigned char *osha1, unsigned 
char *nsha1,
 
ALLOC_GROW(array->items, array->nr + 

[PATCH v3 14/19] hex: introduce parse_oid_hex

[brian m. carlson]

Introduce a function, parse_oid_hex, which parses a hexadecimal object
ID and if successful, sets a pointer to just beyond the last character.
This allows for simpler, more robust parsing without needing to
hard-code integer values throughout the codebase.

Signed-off-by: brian m. carlson 
---
 cache.h | 9 +
 hex.c   | 8 
 2 files changed, 17 insertions(+)

diff --git a/cache.h b/cache.h
index 61fc86e6d7..e03a672d15 100644
--- a/cache.h
+++ b/cache.h
@@ -1319,6 +1319,15 @@ extern char *oid_to_hex_r(char *out, const struct 
object_id *oid);
 extern char *sha1_to_hex(const unsigned char *sha1);   /* static buffer 
result! */
 extern char *oid_to_hex(const struct object_id *oid);  /* same static buffer 
as sha1_to_hex */
 
+/*
+ * Parse a 40-character hexadecimal object ID starting from hex, updating the
+ * pointer specified by end when parsing stops.  The resulting object ID is
+ * stored in oid.  Returns 0 on success.  Parsing will stop on the first NUL or
+ * other invalid character.  end is only updated on success; otherwise, it is
+ * unmodified.
+ */
+extern int parse_oid_hex(const char *hex, struct object_id *oid, const char 
**end);
+
 extern int interpret_branch_name(const char *str, int len, struct strbuf *);
 extern int get_oid_mb(const char *str, struct object_id *oid);
 
diff --git a/hex.c b/hex.c
index 845b01a874..eab7b626ee 100644
--- a/hex.c
+++ b/hex.c
@@ -53,6 +53,14 @@ int get_oid_hex(const char *hex, struct object_id *oid)
return get_sha1_hex(hex, oid->hash);
 }
 
+int parse_oid_hex(const char *hex, struct object_id *oid, const char **end)
+{
+   int ret = get_oid_hex(hex, oid);
+   if (!ret)
+   *end = hex + GIT_SHA1_HEXSZ;
+   return ret;
+}
+
 char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)
 {
static const char hex[] = "0123456789abcdef";
-- 
2.11.0

[PATCH v3 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 e03a672d15..4f3bfc5ee7 100644
--- a/cache.h
+++ b/cache.h
@@ -1608,6 +1608,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(, 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

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Junio C Hamano]

Jeff King  writes:

> I'm also not sure that it's all that useful to distinguish errors from
> fwrite() versus fclose(). In practice, errors will come from write() in
> either case, and the caller does not have much control over when the
> flushing happens. So any error saying "error closing file" is probably
> assuming too much anyway. It should be "error writing file".

Yes.

>> We _could_ clear errno to allow caller to tell them apart, though,
>> if we wanted to ;-)
>
> Hmm. So basically "errno = 0" instead of EIO? That at least has the
> advantage that you can tell it apart from a real EIO, and a caller
> _could_ if they chose do:
>
>   if (commit_lock_file()) {
>   if (!errno)
>   error("error writing to file");
>   else
>   error_errno("error closing file");
>   }

Exactly.

> But I am not sure I would want to annotate all such callers that way. It
> would probably be less bad to just pass down a "quiet" flag or a strbuf
> and have the low-level code fill it in. And that solves this problem
> _and_ the rename() thing above.
>
> But TBH, I am not sure if it is worth it. Nobody is complaining. This is
> just a thing we noticed. I think setting errno to EIO or to "0" is a
> strict improvement over what is there (where the errno is essentially
> random) and the complexity doesn't escape the function.
>
> So I think that "easy" thing falls far short of a solution, but it's at
> least easy. I could take it or leave it at this point.

Well, I already said that earlier in this thread, and ended up
queuing your patch on 'pu' ;-).

Re: [PATCH] Document dotfiles exclusion on template copy

[Junio C Hamano]

Thanks, will queue.

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Jeff King]

On Fri, Feb 17, 2017 at 03:52:36PM -0800, Junio C Hamano wrote:

> > So I think that "easy" thing falls far short of a solution, but it's at
> > least easy. I could take it or leave it at this point.
> 
> Well, I already said that earlier in this thread, and ended up
> queuing your patch on 'pu' ;-).

We're slowly converging on consensus in our apathy. ;)

-Peff

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Jeff King]

On Fri, Feb 17, 2017 at 06:41:32PM -0500, Kyle Meyer wrote:

> Junio C Hamano  writes:
> 
> [...]
> 
> > Do we even want these "internal" delete_ref() invocations to be
> > logged in HEAD's reflog?  I understand that this is inside the
> > implementation of renaming an old ref to a new ref, and reflog
> > message given to delete_ref() would matter only if the HEAD happens
> > to be pointing at old ref---but then HEAD will be repointed to the
> > new ref by somebody else [*1*] that called this function to rename
> > old to new and it _will_ log it.
> 
> I know the discussion has developed further, but just a note that I
> think the last statement is inaccurate: currently, a rename will not be
> recorded in HEAD's log.  "git branch -m" will show a renaming event in
> the new branch's log, but the only trace of the event in HEAD's log is
> the deletion entry with an empty message.

Right. I assumed Junio was talking about the hypothetical behavior we'd
want.

Your response did make me think of one other option: if we updated HEAD
_before_ writing the new ref, then it would automatically get the
"create" half of the rename. IOW, if the rename process were:

  1. delete old; this writes a reflog to HEAD, per the magic
 HEAD-detection in commit_ref_update().

  2. update HEAD to point to new

  3. re-create new; this uses the same magic to write a HEAD reflog.

That's probably a crazy path to go, though. Right now steps (1) and (3)
happen in a low-level function, and step (2) happens outside of it
completely. Arguably it would be good to change that, but I think we
want to think of (1) and (3) as an atomic operation. Putting more things
which might fail between them is a bad idea.

So I think your existing patches (modulo the review comments), plus the
"something like this" that I sent on top are probably a better end
state.

-Peff

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Junio C Hamano]

Kyle Meyer  writes:

> Junio C Hamano  writes:
>
> [...]
>
>> Do we even want these "internal" delete_ref() invocations to be
>> logged in HEAD's reflog?  I understand that this is inside the
>> implementation of renaming an old ref to a new ref, and reflog
>> message given to delete_ref() would matter only if the HEAD happens
>> to be pointing at old ref---but then HEAD will be repointed to the
>> new ref by somebody else [*1*] that called this function to rename
>> old to new and it _will_ log it.
>
> I know the discussion has developed further, but just a note that I
> think the last statement is inaccurate: currently, a rename will not be
> recorded in HEAD's log.  "git branch -m" will show a renaming event in
> the new branch's log, but the only trace of the event in HEAD's log is
> the deletion entry with an empty message.

Yes, I think Peff diagnosed it and suggested a fix.

Re: [PATCH] mingw: make stderr unbuffered again

[Junio C Hamano]

Johannes Schindelin  writes:

> ... there is a different problem here: you stated very explicitly
> that it is okay for `pu` to be broken [*1*]. If it were different, if any
> breakage would imply that a fix is really, really required lest the patch
> series be evicted from `pu`, we could easily modify my Continuous Testing
> setup to report more visibly what is broken.

I think you misread me.

Regarding 'pu', there is a chicken-and-egg problem involved.  Most
of the days, the tip of 'pu' passes the test at least for me.

At the end of day's integration cycle, I may find that 'pu' does not
pass the test for me [*1*].  It is not unusual to see new multiple
topics come to 'pu', each of which may test well in isolation but
have hidden interactions with topics already in 'next' or 'pu' that
are exposed only when merged to 'pu', and it may be too late in the
day for me to find which one of these new topics is problematic.

I could choose not to merge any of them and push 'pu' out with no
new topics.  Or I may choose to push it out anyway, so that other
people who are working during the remainder of 24-hour in different
timezones can notice and find which new topic is broken [*2*].

And that is why I explicitly say that 'pu' may not even build.  It
shouldn't be taken as a discouragement to people from looking at it,
which seems to be how I read you to misinterpret it.  It is the
opposite---a breakage at the tip of 'pu' is an invitation for people
to help.

If nobody steps up and says "the tip of 'pu' does not build and that
is because of this topic", I'd be irritated enough to find which
topic is broken myself and then ask the author of the offencing
topic for help.  If the topic is left broken after that, it will be
ejected from 'pu', because I cannot use 'pu' for helping to polish
other new topics that wants to cook there unless I do so.

Of course there are platform- or environment-dependent breakages
that would not cause my tests to fail [*3*]. Again, people with
different environment can step up to help, if these topics are
included in 'pu'.  You would recall that you called out one topic
[*4*] that did not build in your environment and the topic was
ejected after your message (but not before---there wasn't a way for
Window-less folks to tell that it was broken IIRC) from 'pu'.

In a near-by thread, somebody wondered if we can add an integration
branch 'pr' that is beyond 'pu' and includes everything ever posted
to the list, and I explained why I won't have time for that.  But I
think the spirit of suggested 'pr' is what 'pu' already is---it is a
collection of promising topics that can be merged into a seemingly
consistent whole, which may or may not build and the purpose of the
branch is to contain them in one place so that people can find which
ones need unbreaking.  Testing the tip of it alone and complaining
that it is broken does not help much, but figuiring out which topic
merged to it is broken does, by unblocking other topics in 'pu'.


[Footnote]

*1* If any other integration branch (including 'jch', which is
somewhere between 'master' and 'pu' and beyond the commit with a
tree that matches 'next' aka "pu^{/^### match next}") does not
build, I pull overtime ;-)


*2* When there is only a new topic that breaks 'pu' for me, it is
easy to decide not to queue it and send a note to the author of
the offending topic, and that does happen a lot, but not always.

*3* I do not have p4 and I may not have svn installed so my tests
may not even cover them.

*4* I think it was nd/worktree-move but may have been another topic.

Re: [PATCH v5 3/6] stash: refactor stash_create

[Jeff King]

On Fri, Feb 17, 2017 at 10:41:38PM +, Thomas Gummerer wrote:

> Refactor the internal stash_create function to use a -m flag for
> specifying the message and -u flag to indicate whether untracked files
> should be added to the stash.
> 
> This makes it easier to pass a pathspec argument to stash_create in the
> next patch.
> 
> The user interface for git stash create stays the same.

Sounds good, but...

> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 2e9cef06e6..d93c47446a 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -17,6 +17,7 @@ SYNOPSIS
>[-u|--include-untracked] [-a|--all] []]
>  'git stash' clear
>  'git stash' create []
> +'git stash' create [-m ] [-u|--include-untracked ]
>  'git stash' store [-m|--message ] [-q|--quiet] 

Should this hunk be dropped from the manpage, then?

I think there is a similar one in the next patch that adds the
"pathspec" argument, and should be dropped, too.

-Peff

Re: [PATCH v5 6/6] stash: allow pathspecs in the no verb form

[Jeff King]

On Fri, Feb 17, 2017 at 10:41:41PM +, Thomas Gummerer wrote:

> Now that stash_push is used in the no verb form of stash, allow
> specifying the command line for this form as well.  Always use -- to
> disambiguate pathspecs from other non-option arguments.

I think that makes sense.

> Also make git stash -p an alias for git stash push -p.  This allows
> users to use git stash -p .

And I think of all the options we discussed for handling "-p", I think
this one makes the most sense.

It may be worth calling out in the documentation that this is how it
works though, so people do not think that:

  git stash -k -p 

would work ("git stash -k -p" _does_ happen to work due to the old
options-only rule, but I think we should advertise the general form as
"-p is an alias for "push -p").

-Peff

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Kyle Meyer]

Jeff King  writes:

> On Fri, Feb 17, 2017 at 02:43:50PM -0500, Jeff King wrote:
>
>> Yes. I think the options are basically (in order of decreasing
>> preference in my opinion):
>> 
>>   1. Log a rename entry (same sha1, but note the rename in the free-form
>>  text).
>> 
>>   2. Log a delete (sha1 goes to null) followed by a creation (from null
>>  back to the original sha1).
>> 
>>   3. Log nothing at all for HEAD.
>> 
>> This does half of (2). If we do the second half, then I'd prefer it to
>> (3). But if we can do (1), that is better still (IMHO).

[...]

>> I'm actually confused about which bit of code is updating HEAD. I do not
>> see it either in files_rename_ref() or in the caller. Yet it clearly
>> happens. But that is the code that would know enough to do (1) or the
>> second half of (2) above.
>
> Ah, I found it. It's in replace_each_worktree_head_symref() these days,
> which does not bother to pass a log message.
> 
> So I think the second half of (2) is probably something like the patch
> below.
>
> Thinking on it more, we probably _do_ want two entries. Because the
> operations are not atomic, it's possible that we may end up in a
> half-way state after the first entry is written. And when debugging such
> a case, I'd much rather see the first half of the operation logged than
> nothing at all.

OK, I'll have a go at replacing patch 3 with this approach.

-- 
Kyle

Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs

[Jeff King]

On Fri, Feb 17, 2017 at 06:40:28PM -0500, Kyle Meyer wrote:

> > For reference, the two things I notice are:
> >
> >   - we prefer test_path_is_missing to "! test -f" these days.
> >
> >   - we don't redirect the output of grep (it's handled already in
> > non-verbose mode, and in verbose mode we try to be...verbose).
> 
> Would moving cleanup like "rm -f .git/$m" within the test's body using
> test_when_finished also be preferred?

Yeah, that too.  I forgot to mention it.

-Peff

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Kyle Meyer]

Junio C Hamano  writes:

[...]

> Do we even want these "internal" delete_ref() invocations to be
> logged in HEAD's reflog?  I understand that this is inside the
> implementation of renaming an old ref to a new ref, and reflog
> message given to delete_ref() would matter only if the HEAD happens
> to be pointing at old ref---but then HEAD will be repointed to the
> new ref by somebody else [*1*] that called this function to rename
> old to new and it _will_ log it.

I know the discussion has developed further, but just a note that I
think the last statement is inaccurate: currently, a rename will not be
recorded in HEAD's log.  "git branch -m" will show a renaming event in
the new branch's log, but the only trace of the event in HEAD's log is
the deletion entry with an empty message.

-- 
Kyle

Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs

[Kyle Meyer]

Jeff King  writes:

[...]

>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index b0ffc0b57..65918d984 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -85,6 +85,15 @@ test_expect_success "delete $m (by HEAD)" '
>>  '
>>  rm -f .git/$m
>>  
>> +test_expect_success "deleting current branch adds message to HEAD's log" '
>> +git update-ref $m $A &&
>> +git symbolic-ref HEAD $m &&
>> +git update-ref -mdelmsg -d $m &&
>> +! test -f .git/$m &&
>> +grep "delmsg$" .git/logs/HEAD >/dev/null
>> +'
>> +rm -f .git/$m
>
> I think covering this with a test is good.
>
> I don't know if it's also worth testing that deleting via HEAD also
> writes the reflog. I.e.,:
>
>   git update-ref -m delete-by-head -d HEAD

Seems reasonable to cover this case as well.

> Some of the style here is a bit out-dated, but I think you are just
> matching the surrounding tests.  So that's OK by me (though a patch to
> modernize the whole thing would be welcome, too).

Right.  I'd be happy to follow up with a patch updating the style in
t1400-update-ref.sh.

> For reference, the two things I notice are:
>
>   - we prefer test_path_is_missing to "! test -f" these days.
>
>   - we don't redirect the output of grep (it's handled already in
> non-verbose mode, and in verbose mode we try to be...verbose).

Would moving cleanup like "rm -f .git/$m" within the test's body using
test_when_finished also be preferred?

-- 
Kyle

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Jeff King]

On Fri, Feb 17, 2017 at 02:40:19PM -0800, Junio C Hamano wrote:

> > Right. EIO is almost certainly _not_ the error we saw. But I would
> > rather consistently say "I/O error" and have the user scratch their
> > head, look up this thread, and say "ah, it was probably a deferred
> > error", as opposed to the alternative: the user sees something
> > nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
> > may change from run to run based on what other code runs or fails in
> > between.
> 
> My point was actually not what errno we feed to strerror().  In that
> example, what is more misleading is the fixed part of the error
> message the caller of close_tempfile() used after seeing the funcion
> fail, i.e. "failed to close".  strerror() part is used to explain
> why we "failed to close", and of course any nonsensical errno that
> we did not get from the failed stdio call would not explain it, but
> a more misleading part is that we did not even "failed to close" it.
> 
> We just noticed an earlier error while attempting to close it.
> strerror() in the message does not even have to be related to the
> closing of the file handle.

Ah, I see.  I think the errno thing is a strict improvement over what is
there now. Actually having a separate error message is even better, but
it does end up rather verbose in the callers.

I'm also not sure that it's all that useful to distinguish errors from
fwrite() versus fclose(). In practice, errors will come from write() in
either case, and the caller does not have much control over when the
flushing happens. So any error saying "error closing file" is probably
assuming too much anyway. It should be "error writing file".

And I think in practice the messages end up quite generic anyway, as
they are really calling commit_lock_file(), which may also fail due to a
rename. So you get something like "unable to write 'foo': ", with errno
appended. That's _also_ potentially confusing when rename() fails.

Solving that would probably require passing down an "err" strbuf (or
other data structure) for the low-level code to fill in.

> > The only reason I do not think we should do so for close_tempfile() is
> > that the fclose is typically far away from the code that actually calls
> > error(). We'd have to pass the tristate (success, fail, fail-with-errno)
> > state up through the stack (most of the calls indirectly come from
> > commit_lock_file(), I would think).
> 
> We _could_ clear errno to allow caller to tell them apart, though,
> if we wanted to ;-)

Hmm. So basically "errno = 0" instead of EIO? That at least has the
advantage that you can tell it apart from a real EIO, and a caller
_could_ if they chose do:

  if (commit_lock_file()) {
if (!errno)
error("error writing to file");
else
error_errno("error closing file");
  }

But I am not sure I would want to annotate all such callers that way. It
would probably be less bad to just pass down a "quiet" flag or a strbuf
and have the low-level code fill it in. And that solves this problem
_and_ the rename() thing above.

But TBH, I am not sure if it is worth it. Nobody is complaining. This is
just a thing we noticed. I think setting errno to EIO or to "0" is a
strict improvement over what is there (where the errno is essentially
random) and the complexity doesn't escape the function.

So I think that "easy" thing falls far short of a solution, but it's at
least easy. I could take it or leave it at this point.

-Peff

Re: dotfiles in git template dir are not copied

[Grégoire PARIS]

>There was no 'bug' either. It's just the way it is ;-)

Sure !

[PATCH] Document dotfiles exclusion on template copy

[Grégoire Paris]

Since there is no dotfile in the default template directory, there was
no point in making the check for . or .. more accurate when copying. Now
that you can customize the template directory, it would make sense, but
it's actually a good thing to at this because you would not want to have
your git directory copied in every git directory that is created should
you decide to put your template directory under version control. Plus, it
might be used as a feature by people who would want to exclude some
files.

Signed-off-by: Grégoire Paris 
---
Here is a better version.
 Documentation/git-init.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index 9d27197..3c5a67f 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -116,8 +116,8 @@ does not exist, it will be created.
 TEMPLATE DIRECTORY
 --
 
-The template directory contains files and directories that will be copied to
-the `$GIT_DIR` after it is created.
+Files and directories in the template directory whose name do not start with a
+dot will be copied to the `$GIT_DIR` after it is created.
 
 The template directory will be one of the following (in order):
 
-- 
2.9.3

Re: [PATCH 1/3] delete_refs(): accept a reflog message argument

[Kyle Meyer]

Junio C Hamano  writes:

> Jeff King  writes:
>
>>> diff --git a/refs.h b/refs.h
>>> index 9fbff90e7..81627a63d 100644
>>> --- a/refs.h
>>> +++ b/refs.h
>>> @@ -277,7 +277,7 @@ int reflog_exists(const char *refname);
>>>   * be NULL_SHA1. flags is passed through to ref_transaction_delete().
>>>   */
>>>  int delete_ref(const char *refname, const unsigned char *old_sha1,
>>> -  unsigned int flags);
>>> +  unsigned int flags, const char *msg);
>>
>> Should the "msg" argument go at the beginning, to match update_ref()?
>
> Probably.  rename/create have the message at the end but their
> parameters are very different from update/delete.  The parameters
> update and delete take are not identical, but we can view them as a
> lot more similar than the other two.  So I think it makes sense for
> delete to try matching update, even though trying to make all four
> the same may proabably be pointless.

I put "msg" after "flags" because that's where it occurs in
ref_transaction_delete(), but matching update_ref() makes sense.

-- 
Kyle

Re: [PATCH] Document dotfiles exclusion on template copy

[Grégoire PARIS]

  
-The template directory contains files and directories that will be copied to

-the `$GIT_DIR` after it is created.
+The template directory contains files and directories whose name do not start
+with a dot will be copied to the `$GIT_DIR` after it is created.
  
  The template directory will be one of the following (in order):
  

This sentence does not make any sense, I realize that now.

Re: dotfiles in git template dir are not copied

[Grégoire PARIS]

> Thanks for your guidance. I believe I just sent the patch to the 
mailing list.


I just retried with the Gmail SMTP, and now I am sure it worked. Sorry 
for the noobery.

[PATCH] Document dotfiles exclusion on template copy

[Grégoire Paris]

Since there is no dotfile in the default template directory, there was
no point in making the check for . or .. more accurate when copying. Now
that you can customize the template directory, it would make sense, but
it's actually a good thing to at this because you would not want to have
your git directory copied in every git directory that is created should
you decide to put your template directory under version control. Plus, it
might be used as a feature by people who would want to exclude some
files.

Signed-off-by: Grégoire Paris 
---
I hope I'm getting it right, this is my first time using format-patch and
send-email…
 Documentation/git-init.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index 9d27197..7605742 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -116,8 +116,8 @@ does not exist, it will be created.
 TEMPLATE DIRECTORY
 --
 
-The template directory contains files and directories that will be copied to
-the `$GIT_DIR` after it is created.
+The template directory contains files and directories whose name do not start
+with a dot will be copied to the `$GIT_DIR` after it is created.
 
 The template directory will be one of the following (in order):
 
-- 
2.9.3

Re: dotfiles in git template dir are not copied

[Grégoire PARIS]

Thanks for your guidance. I believe I just sent the patch to the mailing 
list.

Re: Git bisect does not find commit introducing the bug

[Stephan Beyer]

Hi,

On 02/17/2017 11:29 PM, Alex Hoffman wrote:
> According to the documentation "git bisect" is designed "to find the
> commit that introduced a bug" .
> I have found a situation in which it does not returns the commit I expected.
> In order to reproduce the problem:

For the others who are too lazy to clone your repo and run the scripts,
the history is like that (read from bottom to top) and I marked the
commit found by git bisect and the on you expected:

*   7a9e952 (bisect bad) 
|\
| *   671cec2  <--- expected
| |\
| * | 04c6f4b  <--- found
* | |   3915157 
|\ \ \
| | |/
| |/|
| * | f4154e9 (bisect good) 
| * | 85855bf 
| |/
* | f1a36f5 
|/
* 1b7fb88 

The  and  markers are set by your definition of what good and
what bad commits are.

> First of all this is confusing, as this commit cannot be reached
> starting from "v.good".

Hm, IMHO it shows that your example is pretty artificial (although you
might have come across it in a real-world scenario): you introduced a
new feature in f4154e9 (and it worked) and you broke that feature by
making the merge 671cec2. However, the feature (that broke in 671cec2)
did not even exist in 04c6f4b; so a test on the feature would not fail
(leading to "bisect bad" as in the example), it would not exist (leading
to "bisect skip").

And if we are not talking about passing or failing tests but about
crashing, bisect finds the right thing: f4154e9 was not crashing, but
04c6f4b is crashing. Yes, it's not the commit that introduced the crash
(which would be the first commit in the repo) but it's the first
crashing commit after the one marked as good.

So I'd consider this a feature or rather correct behavior, not a bug.

In other words: bisect assumes that your repo is usually in a good state
and you have a commit that changes it to a bad state. In your case you
have a repo that is in a bad state and you have a commit that switches
it to a good state and later you merge a bad-state branch and you have a
bad state again. It is not made for that use-case, I think.

Cheers
  Stephan

Re: [PATCH v5 0/6] stash: support pathspec argument

[Junio C Hamano]

Thomas Gummerer  writes:

> On 02/17, Junio C Hamano wrote:
>> Thomas Gummerer  writes:
>> 
>> [some people may see this message twice, as I forgot to check if the
>> copy I received had "Some A . Body" not enclosed in dq; blindly
>> doing "Reply-All" ended up listing an invalid address on my Cc: line
>> and dropped by vger. apologies]
>> 
>> > diff --git a/git-stash.sh b/git-stash.sh
>> > index a184b1e274..1446fbe2e8 100755
>> > --- a/git-stash.sh
>> > +++ b/git-stash.sh
>> > @@ -67,51 +67,20 @@ create_stash () {
>> >case "$1" in
>> >-m|--message)
>> >shift
>> > -  test -z ${1+x} && usage
>> > -  stash_msg="$1"
>> > -  new_style=t
>> > +  stash_msg=${1-"BUG: create_stash () -m requires an 
>> > argument"}
>> >;;
>> 
>> Did you mean ${1?"BUG: ..."} here and also "-u" below?
>
> Yeah, shell scripts are still confusing me sometimes.  Thanks for
> catching.  Would you mind fixing this up while queuing or do you want
> me to resend?

I'll fix it up myself (please remind me if you notice that I forgot ;-).

[ANNOUNCE] Git v2.12.0-rc2

[Junio C Hamano]

A release candidate Git v2.12.0-rc2 is now available for testing
at the usual places.  It is comprised of 487 non-merge commits
since v2.11.0, contributed by 67 people, 21 of which are new faces.

The tarballs are found at:

https://www.kernel.org/pub/software/scm/git/testing/

The following public repositories all have a copy of the
'v2.12.0-rc2' tag and the 'master' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

New contributors whose contributions weren't in v2.11.0 are as follows.
Welcome to the Git development community!

  Alan Davies, Andreas Krey, Cornelius Weig, David Pursehouse,
  Denton Liu, George Vanburgh, Igor Kushnir, Jack Bates,
  Kristoffer Haugsbakk, Kyle Meyer, Luis Ressel, Lukas Puehringer,
  Markus Hitter, Peter Law, Rasmus Villemoes, Rogier Goossens,
  Stefan Dotterweich, Steven Penny, Vinicius Kursancew, Vladimir
  Panteleev, and Wolfram Sang.

Returning contributors who helped this release are as follows.
Thanks for your continued support.

  마누엘, Alex Henrie, Beat Bolli, Brandon Williams, brian
  m. carlson, Chris Packham, Christian Couder, David Aguilar, David
  Turner, Dennis Kaarsemaker, Dimitriy Ryazantcev, Elia Pinto,
  Eric Wong, Heiko Voigt, Jacob Keller, Jeff Hostetler, Jeff King,
  Johannes Schindelin, Johannes Sixt, Jonathan Tan, Junio C Hamano,
  Kyle J. McKay, Lars Schneider, Linus Torvalds, Luke Diamand, Matt
  McCutchen, Max Kirillov, Mike Hommey, Nguyễn Thái Ngọc Duy,
  Patrick Steinhardt, Paul Mackerras, Philip Oakley, Pranit Bauva,
  Ramsay Jones, René Scharfe, Richard Hansen, Santiago Torres,
  Satoshi Yasushima, Stefan Beller, Stephan Beyer, SZEDER Gábor,
  Thomas Gummerer, Torsten Bögershausen, Vasco Almeida, Vegard
  Nossum, and Vitaly "_Vi" Shukela.



Git 2.12 Release Notes (draft)
==

Backward compatibility notes.

 * Use of an empty string that is used for 'everything matches' is
   still warned and Git asks users to use a more explicit '.' for that
   instead.  The hope is that existing users will not mind this
   change, and eventually the warning can be turned into a hard error,
   upgrading the deprecation into removal of this (mis)feature.  That
   is not scheduled to happen in the upcoming release (yet).

 * The historical argument order "git merge  HEAD ..."
   has been deprecated for quite some time, and will be removed in a
   future release.

 * An ancient script "git relink" has been removed.


Updates since v2.11
---

UI, Workflows & Features

 * Various updates to "git p4".

 * "git p4" didn't interact with the internal of .git directory
   correctly in the modern "git-worktree"-enabled world.

 * "git branch --list" and friends learned "--ignore-case" option to
   optionally sort branches and tags case insensitively.

 * In addition to %(subject), %(body), "log --pretty=format:..."
   learned a new placeholder %(trailers).

 * "git rebase" learned "--quit" option, which allows a user to
   remove the metadata left by an earlier "git rebase" that was
   manually aborted without using "git rebase --abort".

 * "git clone --reference $there --recurse-submodules $super" has been
   taught to guess repositories usable as references for submodules of
   $super that are embedded in $there while making a clone of the
   superproject borrow objects from $there; extend the mechanism to
   also allow submodules of these submodules to borrow repositories
   embedded in these clones of the submodules embedded in the clone of
   the superproject.

 * Porcelain scripts written in Perl are getting internationalized.

 * "git merge --continue" has been added as a synonym to "git commit"
   to conclude a merge that has stopped due to conflicts.

 * Finer-grained control of what protocols are allowed for transports
   during clone/fetch/push have been enabled via a new configuration
   mechanism.

 * "git shortlog" learned "--committer" option to group commits by
   committer, instead of author.

 * GitLFS integration with "git p4" has been updated.

 * The isatty() emulation for Windows has been updated to eradicate
   the previous hack that depended on internals of (older) MSVC
   runtime.

 * Some platforms no longer understand "latin-1" that is still seen in
   the wild in e-mail headers; replace them with "iso-8859-1" that is
   more widely known when conversion fails from/to it.

 * "git grep" has been taught to optionally recurse into submodules.

 * "git rm" used to refuse to remove a submodule when it has its own
   git repository embedded in its working tree.  It learned to move
   the repository away to $GIT_DIR/modules/ of the superproject
   instead, and allow the submodule to be deleted (as long 

Re: [PATCH v5 0/6] stash: support pathspec argument

[Thomas Gummerer]

On 02/17, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> [some people may see this message twice, as I forgot to check if the
> copy I received had "Some A . Body" not enclosed in dq; blindly
> doing "Reply-All" ended up listing an invalid address on my Cc: line
> and dropped by vger. apologies]
> 
> > diff --git a/git-stash.sh b/git-stash.sh
> > index a184b1e274..1446fbe2e8 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -67,51 +67,20 @@ create_stash () {
> > case "$1" in
> > -m|--message)
> > shift
> > -   test -z ${1+x} && usage
> > -   stash_msg="$1"
> > -   new_style=t
> > +   stash_msg=${1-"BUG: create_stash () -m requires an 
> > argument"}
> > ;;
> 
> Did you mean ${1?"BUG: ..."} here and also "-u" below?

Yeah, shell scripts are still confusing me sometimes.  Thanks for
catching.  Would you mind fixing this up while queuing or do you want
me to resend?

> > -u|--include-untracked)
> > shift
> > -   test -z ${1+x} && usage
> > -   untracked="$1"
> > -   new_style=t
> > +   untracked=${1-"BUG: create_stash () -u requires an 
> > argument"}
> > ;;
> 
> Other than that the whole series looked sensible to me.
> 
> Thanks, will replace but that may not happen today.

Thanks!

Re: body-CC-comment regression

[Junio C Hamano]

Junio C Hamano  writes:

> Matthieu Moy  writes:
>
>> Junio C Hamano  writes:
>>
>>> That approach may still constrain what those in the former camp can
>>> write in the "cruft" part, like they cannot write comma or semicolon
>>> as part of the "cruft", no?
>>
>> Right. Indeed, this may be a problem since the use of "#" for stable
>> seem to include commit message, and they may contain commas.
>>
>> So, maybe Johan's patch is better indeed.
>
> OK, so I'll queue that one with your Ack for now so that we won't
> forget.  I guess we still want a few tests?

It seems that there is an expectation in one of the tests that needs
to be adjusted.

Re: [PATCH v5 0/6] stash: support pathspec argument

[Junio C Hamano]

Thomas Gummerer  writes:

[some people may see this message twice, as I forgot to check if the
copy I received had "Some A . Body" not enclosed in dq; blindly
doing "Reply-All" ended up listing an invalid address on my Cc: line
and dropped by vger. apologies]

> diff --git a/git-stash.sh b/git-stash.sh
> index a184b1e274..1446fbe2e8 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -67,51 +67,20 @@ create_stash () {
>   case "$1" in
>   -m|--message)
>   shift
> - test -z ${1+x} && usage
> - stash_msg="$1"
> - new_style=t
> + stash_msg=${1-"BUG: create_stash () -m requires an 
> argument"}
>   ;;

Did you mean ${1?"BUG: ..."} here and also "-u" below?

>   -u|--include-untracked)
>   shift
> - test -z ${1+x} && usage
> - untracked="$1"
> - new_style=t
> + untracked=${1-"BUG: create_stash () -u requires an 
> argument"}
>   ;;

Other than that the whole series looked sensible to me.

Thanks, will replace but that may not happen today.

Re: [PATCH] submodule.c: Add missing quotation marks

[Brandon Williams]

On 02/18, Phillip Sz wrote:
> Hi,
> 
> just found this while translating, sorry if this is intended.
> 
> Best regards,
> 
> Phillip

Definitely not intended.  Thanks for catching that.

> ---
>  submodule.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 3b98766a6..b064ed080 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1181,7 +1181,7 @@ int bad_to_remove_submodule(const char *path, unsigned 
> flags)
>   cp.dir = path;
>   if (start_command()) {
>   if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
> - die(_("could not start 'git status in submodule '%s'"),
> + die(_("could not start 'git status' in submodule '%s'"),
>   path);
>   ret = -1;
>   goto out;
> @@ -1194,7 +1194,7 @@ int bad_to_remove_submodule(const char *path, unsigned 
> flags)
>  
>   if (finish_command()) {
>   if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
> - die(_("could not run 'git status in submodule '%s'"),
> + die(_("could not run 'git status' in submodule '%s'"),
>   path);
>   ret = -1;
>   }
> -- 
> 2.11.1
> 

-- 
Brandon Williams

Re: [PATCH v5 0/6] stash: support pathspec argument

[Thomas Gummerer]

On 02/17, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > Thanks Matthieu, Peff and Junio for the discussion on v3 and v4.
> >
> > Changes since v4:
> > Dropped patch 1 from the series, as it's already in master
> >
> > Instead of changing the external interface to git stash create, only
> > refactor the internal create_stash() function to take -m and -u
> > arguments.  This also simplifies the internal option parsing.
> 
> Yay.
> 
> > Make git stash -p an alias for git stash push -p, so git stash -p
> >  is allowed.
> 
> Nice.
> 
> >
> > Interdiff below:
> >
> > diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> > index b0825f4aca..97194576ef 100644
> > --- a/Documentation/git-stash.txt
> > +++ b/Documentation/git-stash.txt
> > @@ -53,9 +53,8 @@ OPTIONS
> >  save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] 
> > [-a|--all] [-q|--quiet] []::
> >  push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] 
> > [-a|--all] [-q|--quiet] [-m|--message ] [--] [...]::
> >  
> > -   Save your local modifications to a new 'stash' and roll them
> > -   back to HEAD (in the working tree and in the index).
> > -   The  part is optional and gives
> > +   Save your local modifications to a new 'stash', and run `git reset
> > +   --hard` to revert them.  The  part is optional and gives
> > the description along with the stashed state.  For quickly making
> > a snapshot, you can omit _both_ "save" and , but giving
> > only  does not trigger this action to prevent a misspelled
> > diff --git a/git-stash.sh b/git-stash.sh
> > index a184b1e274..1446fbe2e8 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -67,51 +67,20 @@ create_stash () {
> > case "$1" in
> > -m|--message)
> > shift
> > -   test -z ${1+x} && usage
> > -   stash_msg="$1"
> > -   new_style=t
> > +   stash_msg=${1-"BUG: create_stash () -m requires an 
> > argument"}
> 
> Hmph, did you mean ${1?"BUG: ..."}?  The same for the one in "-u".

Ah yes sorry, you're right course.  Thanks.

[PATCH] submodule.c: Add missing quotation marks

[Phillip Sz]

Hi,

just found this while translating, sorry if this is intended.

Best regards,

Phillip
---
 submodule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3b98766a6..b064ed080 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1181,7 +1181,7 @@ int bad_to_remove_submodule(const char *path, unsigned 
flags)
cp.dir = path;
if (start_command()) {
if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
-   die(_("could not start 'git status in submodule '%s'"),
+   die(_("could not start 'git status' in submodule '%s'"),
path);
ret = -1;
goto out;
@@ -1194,7 +1194,7 @@ int bad_to_remove_submodule(const char *path, unsigned 
flags)
 
if (finish_command()) {
if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
-   die(_("could not run 'git status in submodule '%s'"),
+   die(_("could not run 'git status' in submodule '%s'"),
path);
ret = -1;
}
-- 
2.11.1

Re: dotfiles in git template dir are not copied

[Junio C Hamano]

Grégoire PARIS  writes:

>> You could, for example, have your template directory itself be a git 
> repository.
>
> I can and I do and indeed, that might be the reason behind this.
> I made a PR to document this : https://github.com/git/git/pull/325

Let's take a look.

> From: Grégoire PARIS 
> Date: Fri, 17 Feb 2017 22:33:40 +0100
> Subject: [PATCH] Document dotfiles exclusion on template copy
> 
> Since there is no dotfile in the default template directory, there was
> no point in making the check for . or .. more accurate when copying. Now
> that you can customize the template directory, it would make sense, but
> it's actually a good thing to at this because you would not want to have
> your git directory copied in every git directory that is created should
> you decide to put your template directory under version control. Plus it
> might be used as a feature by people who would want to exclude some
> files.

OK.

> See 
> https://public-inbox.org/git/20170217204411.2yixhuazgczxm...@sigill.intra.peff.net/T/#t

I do not think what was discussed there adds much.  Drop this line.

Instead, add your sign-off (Documentation/SubmittingPatches) here

Signed-off-by: Grégoire PARIS 

> ---
>  Documentation/git-init.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
> index 9d27197de8..e3b185cf86 100644
> --- a/Documentation/git-init.txt
> +++ b/Documentation/git-init.txt
> @@ -117,7 +117,7 @@ TEMPLATE DIRECTORY
>  --
>  
>  The template directory contains files and directories that will be copied to
> -the `$GIT_DIR` after it is created.
> +the `$GIT_DIR` after it is created, unless their name starts with a dot.

"... that will be copied ..., unless they are dot" is not incorrect
per-se, but perhaps rewriting the whole sentence, e.g. "Files and
directories in the template directory whose name do not start with a
dot will be copied to ...", may make it easier to read, I suspect.

Re: dotfiles in git template dir are not copied

[Junio C Hamano]

Grégoire PARIS  writes:

>> I do not think we should change the behaviour
>> to copy files whose names begin with a dot.
>
> So bug turned feature it is :)

There was no 'bug' either.  It's just the way it is ;-)

Re: dotfiles in git template dir are not copied

[Grégoire PARIS]

Le 17/02/2017 à 23:39, Philip Oakley a écrit :

From: "Grégoire PARIS" 

> You could, for example, have your template directory itself be a git
repository.

I can and I do and indeed, that might be the reason behind this.
I made a PR to document this : https://github.com/git/git/pull/325

While the PR is a simple one line change to the documention, the patch 
should be submitted to the list.

See Documenation/SubmittingPatches

Don't forget to Sign Off your patch (see the Developer's Certificate 
of Origin section).

--
Philip


Ok sorry, I'll fix this rightaway!
--
greg0ire

[PATCH v5 5/6] stash: use stash_push for no verb form

[Thomas Gummerer]

Now that we have stash_push, which accepts pathspec arguments, use
it instead of stash_save in git stash without any additional verbs.

Previously we allowed git stash -- -message, which is no longer allowed
after this patch.  Messages starting with a hyphen was allowed since
3c2eb80f, ("stash: simplify defaulting to "save" and reject unknown
options").  However it was never the intent to allow that, but rather it
was allowed accidentally.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-stash.txt |  8 
 git-stash.sh| 16 
 t/t3903-stash.sh|  4 +---
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 086c229db0..97194576ef 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,11 +13,11 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] []
 'git stash' ( pop | apply ) [--index] [-q|--quiet] []
 'git stash' branch  []
-'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
-[-u|--include-untracked] [-a|--all] []]
-'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+'git stash' save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+[-u|--include-untracked] [-a|--all] []
+'git stash' [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 [-u|--include-untracked] [-a|--all] [-m|--message ]]
-[--] [...]
+[--] [...]]
 'git stash' clear
 'git stash' create []
 'git stash' create [-m ] [-u|--include-untracked ]
diff --git a/git-stash.sh b/git-stash.sh
index 6b56f84d81..2a33614cb7 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,11 +7,11 @@ USAGE="list []
or: $dashless drop [-q|--quiet] []
or: $dashless ( pop | apply ) [--index] [-q|--quiet] []
or: $dashless branch  []
-   or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
-  [-u|--include-untracked] [-a|--all] []]
-   or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
- [-u|--include-untracked] [-a|--all] [-m ]
- [-- ...]
+   or: $dashless save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] []
+   or: $dashless [push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+  [-u|--include-untracked] [-a|--all] [-m ]
+  [-- ...]]
or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -668,7 +668,7 @@ apply_to_branch () {
 }
 
 PARSE_CACHE='--not-parsed'
-# The default command is "save" if nothing but options are given
+# The default command is "push" if nothing but options are given
 seen_non_option=
 for opt
 do
@@ -678,7 +678,7 @@ do
esac
 done
 
-test -n "$seen_non_option" || set "save" "$@"
+test -n "$seen_non_option" || set "push" "$@"
 
 # Main command set
 case "$1" in
@@ -729,7 +729,7 @@ branch)
 *)
case $# in
0)
-   save_stash &&
+   push_stash &&
say "$(gettext "(To restore them type \"git stash apply\")")"
;;
*)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 4fb800eec8..7f90a247b4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -274,9 +274,7 @@ test_expect_success 'stash --invalid-option' '
git add file2 &&
test_must_fail git stash --invalid-option &&
test_must_fail git stash save --invalid-option &&
-   test bar5,bar6 = $(cat file),$(cat file2) &&
-   git stash -- -message-starting-with-dash &&
-   test bar,bar2 = $(cat file),$(cat file2)
+   test bar5,bar6 = $(cat file),$(cat file2)
 '
 
 test_expect_success 'stash an added file' '
-- 
2.11.0.301.g27b9849079.dirty

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Junio C Hamano]

Jeff King  writes:

>> If we are trying to make sure that the caller would not say "failed
>> to close tempfile: ERRNO" with an ERRNO that is unrelated to any
>> stdio opration, I am not sure if the patch improves things.  The
>> caller did not fail to close (most likely we successfully closed
>> it), and no matter what futzing we do to errno, the message supplied
>> by such a caller will not be improved.
>
> Right. EIO is almost certainly _not_ the error we saw. But I would
> rather consistently say "I/O error" and have the user scratch their
> head, look up this thread, and say "ah, it was probably a deferred
> error", as opposed to the alternative: the user sees something
> nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
> may change from run to run based on what other code runs or fails in
> between.

My point was actually not what errno we feed to strerror().  In that
example, what is more misleading is the fixed part of the error
message the caller of close_tempfile() used after seeing the funcion
fail, i.e. "failed to close".  strerror() part is used to explain
why we "failed to close", and of course any nonsensical errno that
we did not get from the failed stdio call would not explain it, but
a more misleading part is that we did not even "failed to close" it.

We just noticed an earlier error while attempting to close it.
strerror() in the message does not even have to be related to the
closing of the file handle.

>> If the caller used "noticed an earlier error while closing tempfile:
>> ERRNO", such a message would describe the situation more correctly,
>> but then ERRNO that is not about stdio is probably acceptable in the
>> context of that message (the original ERRNO might be ENOSPC that is
>> even more specific than EIO, FWIW).  So I am not sure if the things
>> will improve from the status quo.
>
> Yes, that's I suggested that xfclose() is probably not a good direction.
> The _best_ thing we can do is have the caller not report errno at all
> (or even say "there was an earlier error, I have no idea what errno
> was"). And xfclose() works in the opposite direction.

I think we are in agreement on this point ;-)

> The only reason I do not think we should do so for close_tempfile() is
> that the fclose is typically far away from the code that actually calls
> error(). We'd have to pass the tristate (success, fail, fail-with-errno)
> state up through the stack (most of the calls indirectly come from
> commit_lock_file(), I would think).

We _could_ clear errno to allow caller to tell them apart, though,
if we wanted to ;-)

[PATCH v5 0/6] stash: support pathspec argument

[Thomas Gummerer]

Thanks Matthieu, Peff and Junio for the discussion on v3 and v4.

Changes since v4:
Dropped patch 1 from the series, as it's already in master

Instead of changing the external interface to git stash create, only
refactor the internal create_stash() function to take -m and -u
arguments.  This also simplifies the internal option parsing.

Make git stash -p an alias for git stash push -p, so git stash -p
 is allowed.

Interdiff below:

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index b0825f4aca..97194576ef 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -53,9 +53,8 @@ OPTIONS
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] []::
 push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] [-m|--message ] [--] [...]::
 
-   Save your local modifications to a new 'stash' and roll them
-   back to HEAD (in the working tree and in the index).
-   The  part is optional and gives
+   Save your local modifications to a new 'stash', and run `git reset
+   --hard` to revert them.  The  part is optional and gives
the description along with the stashed state.  For quickly making
a snapshot, you can omit _both_ "save" and , but giving
only  does not trigger this action to prevent a misspelled
diff --git a/git-stash.sh b/git-stash.sh
index a184b1e274..1446fbe2e8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -67,51 +67,20 @@ create_stash () {
case "$1" in
-m|--message)
shift
-   test -z ${1+x} && usage
-   stash_msg="$1"
-   new_style=t
+   stash_msg=${1-"BUG: create_stash () -m requires an 
argument"}
;;
-u|--include-untracked)
shift
-   test -z ${1+x} && usage
-   untracked="$1"
-   new_style=t
+   untracked=${1-"BUG: create_stash () -u requires an 
argument"}
;;
--)
shift
-   new_style=t
-   break
-   ;;
-   *)
-   if test -n "$new_style"
-   then
-   echo "invalid argument"
-   option="$1"
-   # TRANSLATORS: $option is an invalid option, 
like
-   # `--blah-blah'. The 7 spaces at the beginning 
of the
-   # second line correspond to "error: ". So you 
should line
-   # up the second line with however many 
characters the
-   # translation of "error: " takes in your 
language. E.g. in
-   # English this is:
-   #
-   #$ git stash save --blah-blah 2>&1 | head 
-n 2
-   #error: unknown option for 'stash save': 
--blah-blah
-   #   To provide a message, use git stash 
save -- '--blah-blah'
-   eval_gettextln "error: unknown option for 
'stash create': \$option"
-   usage
-   fi
break
;;
esac
shift
done
 
-   if test -z "$new_style"
-   then
-   stash_msg="$*"
-   set --
-   fi
-
git update-index -q --refresh
if no_changes "$@"
then
@@ -698,6 +667,8 @@ apply_to_branch () {
}
 }
 
+test "$1" = "-p" && set "push" "$@"
+
 PARSE_CACHE='--not-parsed'
 # The default command is "push" if nothing but options are given
 seen_non_option=
@@ -740,7 +711,7 @@ clear)
;;
 create)
shift
-   create_stash "$@" && echo "$w_commit"
+   create_stash -m "$*" && echo "$w_commit"
;;
 store)
shift
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 22ac75377b..c0ae41e724 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -800,15 +800,6 @@ test_expect_success 'create with multiple arguments for 
the message' '
test_cmp expect actual
 '
 
-test_expect_success 'new style stash create stores correct message' '
-   >foo &&
-   git add foo &&
-   STASH_ID=$(git stash create -m "create test message new style") &&
-   echo "On master: create test message new style" >expect &&
-   git show --pretty=%s -s ${STASH_ID} >actual &&
-   test_cmp expect actual
-'
-
 test_expect_success 'stash --  stashes and restores the file' '
>foo &&
>bar &&

Thomas Gummerer (6):
  stash: introduce push verb
  stash: add test for the create command line arguments
  stash: refactor 

[PATCH v5 6/6] stash: allow pathspecs in the no verb form

[Thomas Gummerer]

Now that stash_push is used in the no verb form of stash, allow
specifying the command line for this form as well.  Always use -- to
disambiguate pathspecs from other non-option arguments.

Also make git stash -p an alias for git stash push -p.  This allows
users to use git stash -p .

Signed-off-by: Thomas Gummerer 
---
 git-stash.sh |  3 +++
 t/t3903-stash.sh | 15 +++
 2 files changed, 18 insertions(+)

diff --git a/git-stash.sh b/git-stash.sh
index 2a33614cb7..1446fbe2e8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -667,12 +667,15 @@ apply_to_branch () {
}
 }
 
+test "$1" = "-p" && set "push" "$@"
+
 PARSE_CACHE='--not-parsed'
 # The default command is "push" if nothing but options are given
 seen_non_option=
 for opt
 do
case "$opt" in
+   --) break ;;
-*) ;;
*) seen_non_option=t; break ;;
esac
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 7f90a247b4..c0ae41e724 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -872,4 +872,19 @@ test_expect_success 'untracked files are left in place 
when -u is not given' '
test_path_is_file untracked
 '
 
+test_expect_success 'stash without verb with pathspec' '
+   >"foo bar" &&
+   >foo &&
+   >bar &&
+   git add foo* &&
+   git stash -- "foo b*" &&
+   test_path_is_missing "foo bar" &&
+   test_path_is_file foo &&
+   test_path_is_file bar &&
+   git stash pop &&
+   test_path_is_file "foo bar" &&
+   test_path_is_file foo &&
+   test_path_is_file bar
+'
+
 test_done
-- 
2.11.0.301.g27b9849079.dirty

[PATCH v5 3/6] stash: refactor stash_create

[Thomas Gummerer]

Refactor the internal stash_create function to use a -m flag for
specifying the message and -u flag to indicate whether untracked files
should be added to the stash.

This makes it easier to pass a pathspec argument to stash_create in the
next patch.

The user interface for git stash create stays the same.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-stash.txt |  1 +
 git-stash.sh| 23 +++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9cef06e6..d93c47446a 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -17,6 +17,7 @@ SYNOPSIS
 [-u|--include-untracked] [-a|--all] []]
 'git stash' clear
 'git stash' create []
+'git stash' create [-m ] [-u|--include-untracked ]
 'git stash' store [-m|--message ] [-q|--quiet] 
 
 DESCRIPTION
diff --git a/git-stash.sh b/git-stash.sh
index 8365ebba2a..e76741d37d 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -58,8 +58,23 @@ clear_stash () {
 }
 
 create_stash () {
-   stash_msg="$1"
-   untracked="$2"
+   stash_msg=
+   untracked=
+   new_style=
+   while test $# != 0
+   do
+   case "$1" in
+   -m|--message)
+   shift
+   stash_msg=${1-"BUG: create_stash () -m requires an 
argument"}
+   ;;
+   -u|--include-untracked)
+   shift
+   untracked=${1-"BUG: create_stash () -u requires an 
argument"}
+   ;;
+   esac
+   shift
+   done
 
git update-index -q --refresh
if no_changes
@@ -268,7 +283,7 @@ push_stash () {
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
 
-   create_stash "$stash_msg" $untracked
+   create_stash -m "$stash_msg" -u "$untracked"
store_stash -m "$stash_msg" -q $w_commit ||
die "$(gettext "Cannot save the current status")"
say "$(eval_gettext "Saved working directory and index state 
\$stash_msg")"
@@ -667,7 +682,7 @@ clear)
;;
 create)
shift
-   create_stash "$*" && echo "$w_commit"
+   create_stash -m "$*" && echo "$w_commit"
;;
 store)
shift
-- 
2.11.0.301.g27b9849079.dirty

[PATCH v5 1/6] stash: introduce push verb

[Thomas Gummerer]

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.

This allows us to have pathspecs at the end of the command line
arguments like other Git commands do, so that the user can say which
subset of paths to stash (and leave others behind).

Helped-by: Junio C Hamano 
Signed-off-by: Thomas Gummerer 
---
 git-stash.sh | 46 +++---
 t/t3903-stash.sh |  9 +
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1aa..8365ebba2a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -9,6 +9,8 @@ USAGE="list []
or: $dashless branch  []
or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
   [-u|--include-untracked] [-a|--all] []]
+   or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [-m ]
or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -189,10 +191,11 @@ store_stash () {
return $ret
 }
 
-save_stash () {
+push_stash () {
keep_index=
patch_mode=
untracked=
+   stash_msg=
while test $# != 0
do
case "$1" in
@@ -216,6 +219,11 @@ save_stash () {
-a|--all)
untracked=all
;;
+   -m|--message)
+   shift
+   test -z ${1+x} && usage
+   stash_msg=$1
+   ;;
--help)
show_help
;;
@@ -251,8 +259,6 @@ save_stash () {
die "$(gettext "Can't use --patch and --include-untracked or 
--all at the same time")"
fi
 
-   stash_msg="$*"
-
git update-index -q --refresh
if no_changes
then
@@ -291,6 +297,36 @@ save_stash () {
fi
 }
 
+save_stash () {
+   push_options=
+   while test $# != 0
+   do
+   case "$1" in
+   --)
+   shift
+   break
+   ;;
+   -*)
+   # pass all options through to push_stash
+   push_options="$push_options $1"
+   ;;
+   *)
+   break
+   ;;
+   esac
+   shift
+   done
+
+   stash_msg="$*"
+
+   if test -z "$stash_msg"
+   then
+   push_stash $push_options
+   else
+   push_stash $push_options -m "$stash_msg"
+   fi
+}
+
 have_stash () {
git rev-parse --verify --quiet $ref_stash >/dev/null
 }
@@ -617,6 +653,10 @@ save)
shift
save_stash "$@"
;;
+push)
+   shift
+   push_stash "$@"
+   ;;
 apply)
shift
apply_stash "$@"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2de3e18ce6..3577115807 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -775,4 +775,13 @@ test_expect_success 'stash is not confused by partial 
renames' '
test_path_is_missing file
 '
 
+test_expect_success 'push -m shows right message' '
+   >foo &&
+   git add foo &&
+   git stash push -m "test message" &&
+   echo "stash@{0}: On master: test message" >expect &&
+   git stash list -1 >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.301.g27b9849079.dirty

[PATCH v5 2/6] stash: add test for the create command line arguments

[Thomas Gummerer]

Currently there is no test showing the expected behaviour of git stash
create's command line arguments.  Add a test for that to show the
current expected behaviour and to make sure future refactorings don't
break those expectations.

Signed-off-by: Thomas Gummerer 
---
 t/t3903-stash.sh | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 3577115807..ffe3549ea5 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -784,4 +784,22 @@ test_expect_success 'push -m shows right message' '
test_cmp expect actual
 '
 
+test_expect_success 'create stores correct message' '
+   >foo &&
+   git add foo &&
+   STASH_ID=$(git stash create "create test message") &&
+   echo "On master: create test message" >expect &&
+   git show --pretty=%s -s ${STASH_ID} >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'create with multiple arguments for the message' '
+   >foo &&
+   git add foo &&
+   STASH_ID=$(git stash create test untracked) &&
+   echo "On master: test untracked" >expect &&
+   git show --pretty=%s -s ${STASH_ID} >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.301.g27b9849079.dirty

[PATCH v5 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec

[Thomas Gummerer]

While working on a repository, it's often helpful to stash the changes
of a single or multiple files, and leave others alone.  Unfortunately
git currently offers no such option.  git stash -p can be used to work
around this, but it's often impractical when there are a lot of changes
over multiple files.

Allow 'git stash push' to take pathspec to specify which paths to stash.

Helped-by: Junio C Hamano 
Signed-off-by: Thomas Gummerer 
---
 Documentation/git-stash.txt| 11 ++
 git-stash.sh   | 48 +++--
 t/t3903-stash.sh   | 72 ++
 t/t3905-stash-include-untracked.sh | 26 ++
 4 files changed, 146 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index d93c47446a..086c229db0 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -15,9 +15,13 @@ SYNOPSIS
 'git stash' branch  []
 'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 [-u|--include-untracked] [-a|--all] []]
+'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+[-u|--include-untracked] [-a|--all] [-m|--message ]]
+[--] [...]
 'git stash' clear
 'git stash' create []
 'git stash' create [-m ] [-u|--include-untracked ]
+[-- ...]
 'git stash' store [-m|--message ] [-q|--quiet] 
 
 DESCRIPTION
@@ -47,6 +51,7 @@ OPTIONS
 ---
 
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] []::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] [-m|--message ] [--] [...]::
 
Save your local modifications to a new 'stash', and run `git reset
--hard` to revert them.  The  part is optional and gives
@@ -55,6 +60,12 @@ save [-p|--patch] [-k|--[no-]keep-index] 
[-u|--include-untracked] [-a|--all] [-q
only  does not trigger this action to prevent a misspelled
subcommand from making an unwanted stash.
 +
+When pathspec is given to 'git stash push', the new stash records the
+modified states only for the files that match the pathspec.  The index
+entries and working tree files are then rolled back to the state in
+HEAD only for these files, too, leaving files that do not match the
+pathspec intact.
++
 If the `--keep-index` option is used, all changes already added to the
 index are left intact.
 +
diff --git a/git-stash.sh b/git-stash.sh
index e76741d37d..6b56f84d81 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -11,6 +11,7 @@ USAGE="list []
   [-u|--include-untracked] [-a|--all] []]
or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
  [-u|--include-untracked] [-a|--all] [-m ]
+ [-- ...]
or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -35,15 +36,15 @@ else
 fi
 
 no_changes () {
-   git diff-index --quiet --cached HEAD --ignore-submodules -- &&
-   git diff-files --quiet --ignore-submodules &&
+   git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
+   git diff-files --quiet --ignore-submodules -- "$@" &&
(test -z "$untracked" || test -z "$(untracked_files)")
 }
 
 untracked_files () {
excl_opt=--exclude-standard
test "$untracked" = "all" && excl_opt=
-   git ls-files -o -z $excl_opt
+   git ls-files -o -z $excl_opt -- "$@"
 }
 
 clear_stash () {
@@ -72,12 +73,16 @@ create_stash () {
shift
untracked=${1-"BUG: create_stash () -u requires an 
argument"}
;;
+   --)
+   shift
+   break
+   ;;
esac
shift
done
 
git update-index -q --refresh
-   if no_changes
+   if no_changes "$@"
then
exit 0
fi
@@ -109,7 +114,7 @@ create_stash () {
# Untracked files are stored by themselves in a parentless 
commit, for
# ease of unpacking later.
u_commit=$(
-   untracked_files | (
+   untracked_files "$@" | (
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
rm -f "$TMPindex" &&
@@ -132,7 +137,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
-   git diff-index --name-only -z HEAD -- 
>"$TMP-stagenames" &&
+   git diff-index --name-only -z HEAD -- "$@" 
>"$TMP-stagenames" &&
git update-index -z --add --remove --stdin 
<"$TMP-stagenames" &&
git write-tree &&
 

Re: Re: dotfiles in git template dir are not copied

[Philip Oakley]

From: "Grégoire PARIS" 

> You could, for example, have your template directory itself be a git
repository.

I can and I do and indeed, that might be the reason behind this.
I made a PR to document this : https://github.com/git/git/pull/325

While the PR is a simple one line change to the documention, the patch 
should be submitted to the list.

See Documenation/SubmittingPatches

Don't forget to Sign Off your patch (see the Developer's Certificate of 
Origin section).

--
Philip 

Re: dotfiles in git template dir are not copied

[Grégoire PARIS]

> I do not think we should change the behaviour
> to copy files whose names begin with a dot.

So bug turned feature it is :)

I amended my commit message accordingly.

In my case, my template dir is not at the root of the repository where 
it is versioned, so it would not be a problem for me, but I think some 
people might use this feature to add tests/docs without having them copied.


my template dir repository : https://github.com/greg0ire/git_template

Re: git alias for options

[hIpPy]

I think the conversation has drifted away from what I am asking / hoping for.

I apologize but I do not feel in the position of submitting patches
yet. I'll first need to read some code first before that.

I'm coming from the angle where currently I can alias just the command
(ex: l for log) not considering the 'command with options'.

[alias]
  l = log

Currently, the order is as follows:

If built-in git command then it wins.
If custom git command (script as git-l) then it wins.
If alias for git command then it wins.
Else throw as unknown command.

In short, built-in git command > custom git command > git alias.

So I think, the same could also be used for options.

Say I want an alias for option --name-status as -s, so I can type:
$ git log -s

But there is already a -s option and that wins so the built-in option
alias wins.

However, I think I should be able alias it as --ns.
$ git log --ns


Thanks,
hippy

Re: git alias for options

[Junio C Hamano]

Jeff King  writes:

> On Fri, Feb 17, 2017 at 11:10:08PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> The most gentle first step would be to try to turn the existing config
>> options where you can override cli-options into some declarative thing
>> from the current ad-hoc code we have for each one.
>> 
>> That would be no change in behavior, but would make it easier to
>> migrate more things in the future.
>
> Yeah, I'd agree with that. It does not change anything for the users,
> but it makes the implementation less annoying.

Yup, as long as that declarative thing (presumably it would hook
into parse-options that is already sort of declarative) allows some
command line options not overridable with configuration, I think it
would be OK.

I do not think anybody wants to see "reset --hard" and turn it into
"[reset] hard = yes" configuration, and we should not even allow for
that.

Git bisect does not find commit introducing the bug

[Alex Hoffman]

Hi there,

According to the documentation "git bisect" is designed "to find the
commit that introduced a bug" .
I have found a situation in which it does not returns the commit I expected.
In order to reproduce the problem:

1. mkdir test; cd test;
git clone https://github.com/entbugger/git-bisect-issue.git
cd git-bisect-issue

The tag "v.bad" is one bad version and tag "v.good" is the first good
version. A good version is one having the line "FEATURE2" in
file1.txt, which was introduced in "v.good".

2. Copy test scripts to another folder to make sure they are not
overridden by 'git bisect'
cp *.sh ../
cd ..
./search-bug-git.sh

3. Run ./search-bug-git.sh to search for the commit introducing the
bug. It finds that the commit 04c6f4b9ed with the message "Feature 1"
is the first one introducing the bug.

First of all this is confusing, as this commit cannot be reached
starting from "v.good". Then I expected the commit with the message
"Introduce bug" to be returned by 'git bisect', as it is the first
commit  between "v.good" and "v.bad" that does not contain the line
"FEATURE2" in file1.txt, which is what I understand by the commit
"that introduced a bug" (cited from the manpage of git bisect).

Thanks for looking to this,

Best regards,
VG

Re: git diff --quiet exits with 1 on clean tree with CRLF conversions

[Mike Crowe]

On Friday 17 February 2017 at 14:05:17 -0800, Junio C Hamano wrote:
> Mike Crowe  writes:
> 
> > If "git diff --quiet" finds it necessary to compare actual file contents,
> > and a file requires CRLF conversion, then it incorrectly exits with an exit
> > code of 1 even if there have been no changes.
> >
> > The patch below adds a test file that shows the problem.
> 
> If "git diff" does not show any output and "git diff --exit-code" or
> "git diff --quiet" says there are differences, then it is a bug.
> 
> I would however have expected that any culprit would involve a code
> that says "under QUICK option, we do not have to bother doing
> this".  The part you quoted:
> 
> > if (!DIFF_FILE_VALID(p->one) || /* (1) */
> > !DIFF_FILE_VALID(p->two) ||
> > (p->one->oid_valid && p->two->oid_valid) ||
> > (p->one->mode != p->two->mode) ||
> > diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||
> > diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||
> > (p->one->size != p->two->size) ||
> > !diff_filespec_is_identical(p->one, p->two)) /* (2) */
> > p->skip_stat_unmatch_result = 1;
> 
> is used by "git diff" with and without "--quiet", afacr, so I
> suspect that the bug lies somewhere else.

I can't say that I really understand the code fully, but it appears that
the first pass generates a queue of files that contain differences. The
result of the quiet diff comes from the size of that queue,
diff_queued_diff.nr, being non-zero in diffcore_std. I'm assuming that the
result of the noisy diff comes from actually comparing the files.

But, I've only spent a short while looking so I may have got the wrong end
of the stick.

Thanks.

Mike.

Re: body-CC-comment regression

[Junio C Hamano]

Matthieu Moy  writes:

> Junio C Hamano  writes:
>
>> That approach may still constrain what those in the former camp can
>> write in the "cruft" part, like they cannot write comma or semicolon
>> as part of the "cruft", no?
>
> Right. Indeed, this may be a problem since the use of "#" for stable
> seem to include commit message, and they may contain commas.
>
> So, maybe Johan's patch is better indeed.

OK, so I'll queue that one with your Ack for now so that we won't
forget.  I guess we still want a few tests?

Thanks.

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Junio C Hamano]

Jeff King  writes:

> Thinking on it more, we probably _do_ want two entries. Because the
> operations are not atomic, it's possible that we may end up in a
> half-way state after the first entry is written. And when debugging such
> a case, I'd much rather see the first half of the operation logged than
> nothing at all.

Yes, that sounds right.

Re: git alias for options

[Ævar Arnfjörð Bjarmason]

On Fri, Feb 17, 2017 at 9:42 PM, Jeff King  wrote:
> On Fri, Feb 17, 2017 at 02:50:23PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> On Fri, Feb 17, 2017 at 9:23 AM, hIpPy  wrote:
>> > Git has aliases for git commands. Is there a (an inbuilt) way to alias
>> > options? If not, what is the reason?
>>
>> This has long been on my  wishlist, because there's a lot of
>> copy/pasted logic all over Git to make git foo --whatever aliased to
>> foo.whatever in the config, but only for some options.
>>
>> It should ideally be part of something every option just supports, via
>> the getopts struct.
>>
>> However, it can't allow you to modify whatever option you want,
>> because some things like git-commit-tree should not be customized
>> based on config, it would break things that rely on the plumbing
>> commands.
>>
>> So it would have to be a whitelist for each option we allow to be
>> configured like this via the getopts struct.
>>
>> Also there are surely other edge cases, like maybe the config option
>> now doesn't 1=1 map to the name for the option in some cases, or the
>> flag should be config-able but is has no long form (which we'd like
>> for the config), then we'd want to add that etc.
>
> I think your idea is roughly equivalent in functionality to just
> allowing aliases to override command names. E.g., anything you could do
> with:
>
>   [log]
>   follow = true
>   decorate = false
>
> could be done with:
>
>   [alias]
>   log = "log --follow --no-decorate"

Indeed, exact same thing, different syntax. Mostly I like this
suggestion better, although the bad side of it is that it's not as
easy to introspect with a dump of git-config -l.

> The reason we have historically not allowed that is for the
> "commit-tree" plumbing reason you gave above. One option would be to
> relax it for a whitelist of porcelain commands. Then your whitelist at
> least only has to cover commands, and not each individual option.
>
> I think there are a lot of corner cases in that whitelist, though. A lot
> of commands serve dual porcelain/plumbing purposes. E.g., "log" and
> "tag" are commonly used by both humans and by scripts.
>
> A first step in that direction would probably be an environment variable
> to tell Git to suppress command-aliases. Scripts would set that to
> ensure a more vanilla experience. It doesn't fix _existing_ scripts, but
> if that option were introduced, then over time scripts would start to
> use it. Then eventually it would be safe(r) to introduce something like
> command aliases.

The most gentle first step would be to try to turn the existing config
options where you can override cli-options into some declarative thing
from the current ad-hoc code we have for each one.

That would be no change in behavior, but would make it easier to
migrate more things in the future.

Anyway, words are cheap. Just replied because to the extent that hIpPy
wants to work on this I thought I'd sprinkle some historical caveats
from memory. Other than that no point in keeping talking about this
without patches.

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Jeff King]

On Fri, Feb 17, 2017 at 01:42:21PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:
> >
> >> Stepping back a bit, would this be really needed?  Even if the ferror()
> >> does not update errno, the original stdio operation that failed
> >> would have, no?
> >
> > Sure, but we have no clue what happened in between.
> 
> Hmm, so we are protecting against somebody who does "errno = 0"
> explicitly, because she knows that she's dealt with the error from
> stdio earlier?  Such a careful person would have called clearerr()
> as well, I would guess.

I'm not sure I understand what you are saying here. If somebody calls
clearerr(), our ferror() handling does not trigger at all, and do not
care what is in errno either way. They can reset errno or not when they
clearerr(), but it is not relevant.

If you are asking about somebody who sets errno to "0" and _doesn't_
call clearerr(), then I don't know what that person is trying to
accomplish. Setting errno to "0" is not the right way to clear an error.
And they certainly should not be relying on it not to get overwritten
before we make it to the final ferror()/fclose().

> So let's assume we only care about the case where some other error
> was detected and errno was updated by a system library call.

Right.

> > I think our emails crossed, but our patches are obviously quite similar.
> > My commit message maybe explains a bit more of my thinking.
> 
> Yes, but ;-)
> 
> If we are trying to make sure that the caller would not say "failed
> to close tempfile: ERRNO" with an ERRNO that is unrelated to any
> stdio opration, I am not sure if the patch improves things.  The
> caller did not fail to close (most likely we successfully closed
> it), and no matter what futzing we do to errno, the message supplied
> by such a caller will not be improved.

Right. EIO is almost certainly _not_ the error we saw. But I would
rather consistently say "I/O error" and have the user scratch their
head, look up this thread, and say "ah, it was probably a deferred
error", as opposed to the alternative: the user sees something
nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
may change from run to run based on what other code runs or fails in
between.

> If the caller used "noticed an earlier error while closing tempfile:
> ERRNO", such a message would describe the situation more correctly,
> but then ERRNO that is not about stdio is probably acceptable in the
> context of that message (the original ERRNO might be ENOSPC that is
> even more specific than EIO, FWIW).  So I am not sure if the things
> will improve from the status quo.

Yes, that's I suggested that xfclose() is probably not a good direction.
The _best_ thing we can do is have the caller not report errno at all
(or even say "there was an earlier error, I have no idea what errno
was"). And xfclose() works in the opposite direction.

The only reason I do not think we should do so for close_tempfile() is
that the fclose is typically far away from the code that actually calls
error(). We'd have to pass the tristate (success, fail, fail-with-errno)
state up through the stack (most of the calls indirectly come from
commit_lock_file(), I would think).

-Peff

Re: git alias for options

[Jeff King]

On Fri, Feb 17, 2017 at 11:10:08PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > A first step in that direction would probably be an environment variable
> > to tell Git to suppress command-aliases. Scripts would set that to
> > ensure a more vanilla experience. It doesn't fix _existing_ scripts, but
> > if that option were introduced, then over time scripts would start to
> > use it. Then eventually it would be safe(r) to introduce something like
> > command aliases.
> 
> The most gentle first step would be to try to turn the existing config
> options where you can override cli-options into some declarative thing
> from the current ad-hoc code we have for each one.
> 
> That would be no change in behavior, but would make it easier to
> migrate more things in the future.

Yeah, I'd agree with that. It does not change anything for the users,
but it makes the implementation less annoying.

-Peff

Re: dotfiles in git template dir are not copied

[Junio C Hamano]

Grégoire PARIS  writes:

>> You could, for example, have your template directory itself be a git 
> repository.
>
> I can and I do and indeed, that might be the reason behind this.

An embedded .git was _not_ the reason behind the current behaviour
when we wrote it.  The primary reason is because we did not ship and
did not plan to ship anything .dot in our standard template set and
there wasn't any reason to copy what we were not going to ever ship
;-).

As a justification after the fact, "you wouldn't want to copy .git
do you?" is a good one, though, and I do not think we should change
the behaviour to copy files whose names begin with a dot.

Re: git diff --quiet exits with 1 on clean tree with CRLF conversions

[Junio C Hamano]

Mike Crowe  writes:

> If "git diff --quiet" finds it necessary to compare actual file contents,
> and a file requires CRLF conversion, then it incorrectly exits with an exit
> code of 1 even if there have been no changes.
>
> The patch below adds a test file that shows the problem.

If "git diff" does not show any output and "git diff --exit-code" or
"git diff --quiet" says there are differences, then it is a bug.

I would however have expected that any culprit would involve a code
that says "under QUICK option, we do not have to bother doing
this".  The part you quoted:

>   if (!DIFF_FILE_VALID(p->one) || /* (1) */
>   !DIFF_FILE_VALID(p->two) ||
>   (p->one->oid_valid && p->two->oid_valid) ||
>   (p->one->mode != p->two->mode) ||
>   diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||
>   diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||
>   (p->one->size != p->two->size) ||
>   !diff_filespec_is_identical(p->one, p->two)) /* (2) */
>   p->skip_stat_unmatch_result = 1;

is used by "git diff" with and without "--quiet", afacr, so I
suspect that the bug lies somewhere else.

Re: [PATCH] l10n: de.po: translate 241 messages

[Phillip Sz]

Hey,

> @@ -916,17 +916,17 @@ msgstr ""
>  msgid ""
>  "The merge base %s is %s.\n"
>  "This means the first '%s' commit is between %s and [%s].\n"
>  msgstr ""
>  "Die Merge-Basis %s ist %s.\n"
>  "Das bedeutet, der erste '%s' Commit befindet sich zwischen %s und [%s]\n"

"Das bedeutet, der erste '%s' Commit befindet sich zwischen %s und [%s].\n"

Period is missing.


>  #: remote.c:2092
>  #, c-format
>  msgid "Your branch is ahead of '%s' by %d commit.\n"
>  msgid_plural "Your branch is ahead of '%s' by %d commits.\n"
> -msgstr[0] "Ihr Branch ist vor '%s' um %d Commit.\n"
> -msgstr[1] "Ihr Branch ist vor '%s' um %d Commits.\n"
> +msgstr[0] "Ihr Branch ist %2$d Commit vor '%1$s'.\n"
> +msgstr[1] "Ihr Branch ist %2$d Commits vor '%1$s'.\n"
>  

Does this "%2$d" works and why not use '%s'?

>  #: sequencer.c:840
> -#, fuzzy
>  msgid "could not read HEAD's commit message"
> -msgstr "Konnte Commit-Beschreibung nicht lesen: %s"
> +msgstr "Konnte Commit-Beschreibung von HEAD nicht lesen"
>

>  #: sequencer.c:846
> -#, fuzzy, c-format
> +#, c-format
>  msgid "cannot write '%s'"
> -msgstr "kann '%s' nicht erstellen"
> +msgstr "kann '%s' nicht schreiben"

I think we should either use "kann" or "konnte".
We have used both and maybe we should unify it? What do you think?

>  #: sequencer.c:1341
> -#, fuzzy
>  msgid "please fix this using 'git rebase --edit-todo'."
>  msgstr "Bitte beheben Sie das, indem Sie 'git rebase --edit-todo' ausführen."

Maybe: "Bitte beheben Sie dieses, indem Sie 'git rebase --edit-todo'
ausführen."

>  #: git-add--interactive.perl:1074
>  #, perl-format
>  msgid ""
>  "---\n"
>  "To remove '%s' lines, make them ' ' lines (context).\n"
>  "To remove '%s' lines, delete them.\n"
>  "Lines starting with %s will be removed.\n"
>  msgstr ""
> +"---\n"
> +"Um '%s' Zeilen zu entfernen, machen Sie aus diesen ' ' Zeilen (Kontext).\n"
> +"Um '%s' Zeilen zu entferenen, löschen Sie diese.\n"
> +"Zeilen, die mit %s beginnen, werden entfernt.\n"
>  

"Um '%s' Zeilen zu entfernen, löschen Sie diese.\n"

Anyway thanks a lot for your awesome work!

Phillip

Re: Re: dotfiles in git template dir are not copied

[Grégoire PARIS]

> You could, for example, have your template directory itself be a git 
repository.


I can and I do and indeed, that might be the reason behind this.
I made a PR to document this : https://github.com/git/git/pull/325

--
greg0ire

Re: [PATCH v2 00/19] object_id part 6

[brian m. carlson]

On Fri, Feb 17, 2017 at 10:55:03AM +0100, Michael Haggerty wrote:
> On 02/14/2017 03:31 AM, brian m. carlson wrote:
> > This is another series in the continuing conversion to struct object_id.
> > 
> > This series converts more of the builtin directory and some of the refs
> > code to use struct object_id. Additionally, it implements an
> > nth_packed_object_oid function which provides a struct object_id version
> > of the nth_packed_object function, and a parse_oid_hex function that
> > makes parsing easier.
> > 
> > The patch to use parse_oid_hex in the refs code has been split out into
> > its own patch, just because I'm wary of that code and potentially
> > breaking things, and I want it to be easy to revert in case things go
> > wrong.  I have no reason to believe it is anything other than fully
> > functional, however.
> > 
> > Changes from v1:
> > * Implement parse_oid_hex and use it.
> > * Make nth_packed_object_oid take a variable into which to store the
> >   object ID.  This avoids concerns about unsafe casts.
> > * Rebase on master.
> 
> Thanks as always for working on this!
> 
> I skimmed over the patches (looking more carefully at the refs-related
> ones) and left a few minor comments but didn't find anything serious.

I'll send out a new series shortly with those changes.

> I'm curious; what fraction of the overall convert-to-object_id campaign
> do you estimate is done so far? Are you getting close to the promised
> land yet?

So I think that the current scope left is best estimated by the
following command:

  git grep -P 'unsigned char\s+(\*|.*20)' | grep -v '^Documentation'

So there are approximately 1200 call sites left, which is quite a bit of
work.  I estimate between the work I've done and other people's
refactoring work (such as the refs backend refactor), we're about 40%
done.

I'm hoping to send out more smaller series in the future, since running
make test on a huge series takes a long time.

Part of the problem is that some places are almost completely
convertible, except for one or two components that rely on something
like sha1_array.  I have patches somewhere that convert that to
oid_array, but I'm not sure how well they'll be received at this point.
I think converting a lot of shallow-related code requires that
conversion, though.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Junio C Hamano]

Jeff King  writes:

> On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:
>
>> Stepping back a bit, would this be really needed?  Even if the ferror()
>> does not update errno, the original stdio operation that failed
>> would have, no?
>
> Sure, but we have no clue what happened in between.

Hmm, so we are protecting against somebody who does "errno = 0"
explicitly, because she knows that she's dealt with the error from
stdio earlier?  Such a careful person would have called clearerr()
as well, I would guess.

So let's assume we only care about the case where some other error
was detected and errno was updated by a system library call.

> I think our emails crossed, but our patches are obviously quite similar.
> My commit message maybe explains a bit more of my thinking.

Yes, but ;-)

If we are trying to make sure that the caller would not say "failed
to close tempfile: ERRNO" with an ERRNO that is unrelated to any
stdio opration, I am not sure if the patch improves things.  The
caller did not fail to close (most likely we successfully closed
it), and no matter what futzing we do to errno, the message supplied
by such a caller will not be improved.

If the caller used "noticed an earlier error while closing tempfile:
ERRNO", such a message would describe the situation more correctly,
but then ERRNO that is not about stdio is probably acceptable in the
context of that message (the original ERRNO might be ENOSPC that is
even more specific than EIO, FWIW).  So I am not sure if the things
will improve from the status quo.

It's easy for us to either apply the patch and be done with it (or
drop and do the same), and in the bigger picture it wouldn't make
that much of a difference, I would think, so I can go either way.

git diff --quiet exits with 1 on clean tree with CRLF conversions

[Mike Crowe]

If "git diff --quiet" finds it necessary to compare actual file contents,
and a file requires CRLF conversion, then it incorrectly exits with an exit
code of 1 even if there have been no changes.

The patch below adds a test file that shows the problem.

The first test of diff without --quiet correctly has an exit status of zero
on both invocations.

The second test of diff with --quiet has an exit code of zero on the first
invocation, but an exit code of one on the second invocation. Further
invocations (not included in the test) may yield an exit code of 1. Calling
"git status" always fixes things.

(The touching with "tomorrow" was my attempt to avoid the sleep, but that
didn't seem to help - it appears that time must pass in order to ensure
that the cache is not used.)

The culprit would appear to be in diff_filespec_check_stat_unmatch where it
assumes that the files are different if their sizes are different:

if (!DIFF_FILE_VALID(p->one) || /* (1) */
!DIFF_FILE_VALID(p->two) ||
(p->one->oid_valid && p->two->oid_valid) ||
(p->one->mode != p->two->mode) ||
diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||
diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||
(p->one->size != p->two->size) ||
!diff_filespec_is_identical(p->one, p->two)) /* (2) */
p->skip_stat_unmatch_result = 1;

In the failing case p->one->size == 14 and p->two->size == 12.

Mike.

diff --git a/t/t4063-diff-converted.sh b/t/t4063-diff-converted.sh
new file mode 100755
index 000..a108dfb
--- /dev/null
+++ b/t/t4063-diff-converted.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+#
+# Copyright (c) 2017 Mike Crowe
+#
+
+test_description='git diff with files that require CRLF conversion'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+   echo "* text=auto" > .gitattributes &&
+   printf "Hello\r\nWorld\r\n" > crlf.txt &&
+   git add .gitattributes crlf.txt &&
+   git commit -m "initial"
+'
+test_expect_success 'noisy diff works on file that requires CRLF conversion' '
+   git status >/dev/null &&
+   git diff >/dev/null &&
+   sleep 1 &&
+   touch --date=tomorrow crlf.txt &&
+   git diff >/dev/null
+'
+# The sleep is required for reasons I don't fully understand
+test_expect_success 'quiet diff works on file that requires CRLF conversion 
with no changes' '
+   git status &&
+   git diff --quiet &&
+   sleep 1 &&
+   touch --date=tomorrow crlf.txt &&
+   git diff --quiet
+'
+
+test_done

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Jeff King]

On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:

> > I guess we are simultaneously assuming that it is OK to munge errno on
> > success in our function, but that fclose() will not do so. Which seems a
> > bit hypocritical. Maybe the "if" dance is better.
> 
> Yes.  When both ferror() and fclose() are successful, we would
> prefer to see the original errno unmolested, so the "if" dance,
> even though it looks uglier, is better.  The ugliness is limited
> to the implementation anyway ;-)
> 
> But it does look ugly, especially when nested inside the existing
> code like so.

I think our emails crossed, but our patches are obviously quite similar.
My commit message maybe explains a bit more of my thinking.

> Stepping back a bit, would this be really needed?  Even if the ferror()
> does not update errno, the original stdio operation that failed
> would have, no?

Sure, but we have no clue what happened in between.

-Peff

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Junio C Hamano]

Jeff King  writes:

> On Fri, Feb 17, 2017 at 11:42:25AM +0100, Michael Haggerty wrote:
>
>> On 02/17/2017 09:07 AM, Jeff King wrote:
>> > [...]
>> > That's similar to what I wrote earlier, but if we don't mind overwriting
>> > errno unconditionally, I think just:
>> > 
>> >   errno = EIO; /* covers ferror(), overwritten by failing fclose() */
>> >   err |= ferror(fp);
>> >   err |= fclose(fp);
>> > 
>> > does the same thing.
>> 
>> True; I'd forgotten the convention that non-failing functions are
>> allowed to change errno. Your solution is obviously simpler and faster.
>
> I guess we are simultaneously assuming that it is OK to munge errno on
> success in our function, but that fclose() will not do so. Which seems a
> bit hypocritical. Maybe the "if" dance is better.

Yes.  When both ferror() and fclose() are successful, we would
prefer to see the original errno unmolested, so the "if" dance,
even though it looks uglier, is better.  The ugliness is limited
to the implementation anyway ;-)

But it does look ugly, especially when nested inside the existing
code like so.

Stepping back a bit, would this be really needed?  Even if the ferror()
does not update errno, the original stdio operation that failed
would have, no?

-- >8 --
Subject: close_tempfile(): set errno when ferror() notices a previous error

In close_tempfile(), we may notice that previous stdio operations
failed when we inspect ferror(tempfile->fp).  As ferror() does not
set errno, and the caller of close_tempfile(), since it encountered
and ignored the original error, is likely to have called other
system library functions to cause errno to be modified, the caller
cannot really tell anything meaningful by looking at errno after we
return an error from here.  

Set errno to an arbitrary value EIO when ferror() sees an error but
fclose() succeeds.  If fclose() fails, we just let the caller see
errno from that failure.

---
 tempfile.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index ffcc272375..d2c6de83a9 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -247,8 +247,20 @@ int close_tempfile(struct tempfile *tempfile)
tempfile->fd = -1;
if (fp) {
tempfile->fp = NULL;
-   err = ferror(fp);
-   err |= fclose(fp);
+   if (ferror(fp)) {
+   err = -1;
+   if (!fclose(fp))
+   /*
+* There was some error detected by ferror()
+* but it is likely that the true errno has
+* long gone.  Leave something generic to make
+* it clear that the caller cannot rely on errno
+* at this point.
+*/
+   errno = EIO;
+   } else {
+   err = fclose(fp);
+   }
} else {
err = close(fd);
}

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Jeff King]

On Fri, Feb 17, 2017 at 03:54:42PM -0500, Jeff King wrote:

> I guess we are simultaneously assuming that it is OK to munge errno on
> success in our function, but that fclose() will not do so. Which seems a
> bit hypocritical. Maybe the "if" dance is better.

So here's that patch with a justification.

At this point, this snippet of code would be appropriate to pull into
xfclose() if we wanted. But possibly that is the wrong direction, as it
encourages callers to do:

  if (xfclose(fp))
err = error_errno("failure writing to ...");

when they could do:

  if (ferror(fp))
err = error("failure writing to ...");
  if (fclose(fp))
err = error_errno("failure writing to ...");

While longer, it's arguably better for them to distinguish the two
cases. It's only worth doing the errno magic when the close is deep
inside a callstack, and passing out the two cases is awkward.

-- >8 --
Subject: tempfile: set errno to a known value before calling ferror()

In close_tempfile(), we return an error if ferror()
indicated a previous failure, or if fclose() failed. In the
latter case, errno is set and it is useful for callers to
report it.

However, if _only_ ferror() triggers, then the value of
errno is based on whatever syscall happened to last fail,
which may not be related to our filehandle at all. A caller
cannot tell the difference between the two cases, and may
use "die_errno()" or similar to report a nonsense errno value.

One solution would be to actually pass back separate return
values for the two cases, so a caller can write a more
appropriate message for each case. But that makes the
interface clunky.

Instead, let's just set errno to the generic EIO in this case.
That's not as descriptive as we'd like, but at least it's
predictable. So it's better than the status quo in all cases
but one: when the last syscall really did involve a failure
on our filehandle, we'll be wiping that out. But that's a
fragile thing for us to rely on.

In any case, we'll let the errno result from fclose() take
precedence over our value, as we know that's recent and
accurate (and many I/O errors will persist through the
fclose anyway).

Signed-off-by: Jeff King 
---
 tempfile.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index ffcc27237..684371067 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -247,8 +247,13 @@ int close_tempfile(struct tempfile *tempfile)
tempfile->fd = -1;
if (fp) {
tempfile->fp = NULL;
-   err = ferror(fp);
-   err |= fclose(fp);
+   if (ferror(fp)) {
+   err = -1;
+   if (!fclose(fp))
+   errno = EIO;
+   } else {
+   err = fclose(fp);
+   }
} else {
err = close(fd);
}
-- 
2.12.0.rc1.612.ga5f664feb

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Jeff King]

On Fri, Feb 17, 2017 at 11:42:25AM +0100, Michael Haggerty wrote:

> On 02/17/2017 09:07 AM, Jeff King wrote:
> > [...]
> > That's similar to what I wrote earlier, but if we don't mind overwriting
> > errno unconditionally, I think just:
> > 
> >   errno = EIO; /* covers ferror(), overwritten by failing fclose() */
> >   err |= ferror(fp);
> >   err |= fclose(fp);
> > 
> > does the same thing.
> 
> True; I'd forgotten the convention that non-failing functions are
> allowed to change errno. Your solution is obviously simpler and faster.

I guess we are simultaneously assuming that it is OK to munge errno on
success in our function, but that fclose() will not do so. Which seems a
bit hypocritical. Maybe the "if" dance is better.

-Peff

Re: [PATCH v2 00/16] Remove submodule from files-backend.c

[Junio C Hamano]

Junio C Hamano  writes:

> Nguyễn Thái Ngọc Duy   writes:
>
>> I'll be sending two more follow-up series, if you are interested, soon:
>>
>> 1) v2 of nd/worktree-gc-protection
>> ...
>> 2) the real worktree-gc-protection
>>
>> This series adds a bunch of new refs API, enough for revision.c to
>> traverses all sorts of refs with a ref store instead of "submodule".
>> Many _submodule API are removed as a result because they no longer
>> have any callers (yay!). One of them remains though.
>
> Yay indeed.

Having said all that, I have a bad news.

I won't be able to queue this as-is, as this heavily conflicts with
mh/ref-remove-empty-directory that has already been cooking in
'next' for some time.  I started trying conflict resolution myself
and it seemed doable, but stopped after finishing perhaps 40% of it,
as I cannot afford the same amount of time for conflict resolution
every time the series gets rerolled.

If the topic is rebased on the result of doing:

 - checking out master^0
 - merging mh/ref-remove-empty-directory on it
 - mergeing mh/submodule-hash on the result

I suspect that we might be able start cooking it in 'pu' sooner, but
I cannot spend any more time on this topic today so I'd stop analyzing
the situation for now.

Re: dotfiles in git template dir are not copied

[Jeff King]

On Fri, Feb 17, 2017 at 10:31:37AM +0100, greg0ire wrote:

> I noticed yesterday that dotfiles inside the directory configured in
> init.templatedir are not copied when creating a new repository.
> 
> Here is the line I think is responsible for this behavior :
> https://github.com/git/git/blob/master/builtin/init-db.c#L48
> 
> Is it a bug or a feature?

I think it's probably a feature. You could, for example, have your
template directory itself be a git repository. You would not want to
copy the ".git" directory around.

I wouldn't be surprised if the documentation could be improved, though.

-Peff

Re: git alias for options

[Jeff King]

On Fri, Feb 17, 2017 at 02:50:23PM +0100, Ævar Arnfjörð Bjarmason wrote:

> On Fri, Feb 17, 2017 at 9:23 AM, hIpPy  wrote:
> > Git has aliases for git commands. Is there a (an inbuilt) way to alias
> > options? If not, what is the reason?
> 
> This has long been on my  wishlist, because there's a lot of
> copy/pasted logic all over Git to make git foo --whatever aliased to
> foo.whatever in the config, but only for some options.
> 
> It should ideally be part of something every option just supports, via
> the getopts struct.
> 
> However, it can't allow you to modify whatever option you want,
> because some things like git-commit-tree should not be customized
> based on config, it would break things that rely on the plumbing
> commands.
> 
> So it would have to be a whitelist for each option we allow to be
> configured like this via the getopts struct.
> 
> Also there are surely other edge cases, like maybe the config option
> now doesn't 1=1 map to the name for the option in some cases, or the
> flag should be config-able but is has no long form (which we'd like
> for the config), then we'd want to add that etc.

I think your idea is roughly equivalent in functionality to just
allowing aliases to override command names. E.g., anything you could do
with:

  [log]
  follow = true
  decorate = false

could be done with:

  [alias]
  log = "log --follow --no-decorate"

The reason we have historically not allowed that is for the
"commit-tree" plumbing reason you gave above. One option would be to
relax it for a whitelist of porcelain commands. Then your whitelist at
least only has to cover commands, and not each individual option.

I think there are a lot of corner cases in that whitelist, though. A lot
of commands serve dual porcelain/plumbing purposes. E.g., "log" and
"tag" are commonly used by both humans and by scripts.

A first step in that direction would probably be an environment variable
to tell Git to suppress command-aliases. Scripts would set that to
ensure a more vanilla experience. It doesn't fix _existing_ scripts, but
if that option were introduced, then over time scripts would start to
use it. Then eventually it would be safe(r) to introduce something like
command aliases.

But nobody has ever taken that first step, so here we are.

-Peff

Re: body-CC-comment regression

[Matthieu Moy]

Junio C Hamano  writes:

> That approach may still constrain what those in the former camp can
> write in the "cruft" part, like they cannot write comma or semicolon
> as part of the "cruft", no?

Right. Indeed, this may be a problem since the use of "#" for stable
seem to include commit message, and they may contain commas.

So, maybe Johan's patch is better indeed.

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