Re: [PATCH 2/2] format-patch: add an option to suppress commit hash

[brian m. carlson]

On Mon, Dec 07, 2015 at 11:34:59AM -0800, Junio C Hamano wrote:
> Two (big) problems with the option name.
> 
>  - "--no-something" would mislead people to think you are removing
>something, not replacing it with something else.  This option
>does the latter (i.e. the first line of your output still has
>40-hex; it's just it no longer has a useful 40-hex).

Right.  I originally considered removing that line altogether, but other
parts of Git rely on that header to process patches correctly.

>  - There are many places we use hexadecimal strings in format-patch
>output and you are not removing or replacing all of them, only
>the commit object name on the fake "From " line.  Saying "hash"
>would mislead readers.

I'll reroll with the name --zero-commit, unless somebody comes up with a
better suggestion.

> > +test_expect_success 'format-patch --no-hash' '
> > +   git format-patch --no-hash --stdout v2..v1 >patch2 &&
> > +   cnt=$(egrep "^From 0+ Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&
> 
> Don't test "any number of '0'"; test 40 '0's.  This is because the
> line format was designed to be usable by things like /etc/magic to
> detect format-patch output, and we want to notice if/when we break
> that aspect of our output format.

My idea was to future-proof it against changes in the hash function,
although that's in the distant future.  I'll reroll with the change you
suggested.  I might drop in another patch to improve the existing tests
to cover the case without this option, as I think we just look for
"From " currently.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: GPG public keys

[brian m. carlson]

On Wed, Dec 09, 2015 at 05:43:36PM -0500, Jeff King wrote:
> On Wed, Dec 09, 2015 at 02:24:17PM -0800, Stefan Beller wrote:
> 
> > On Wed, Dec 9, 2015 at 2:04 PM, Jeff King <p...@peff.net> wrote:
> > >
> > > Of course you can't just fetch the v1.7.1.4 tag _now_, because the same
> > > person impersonating the most recent tag could also be impersonating
> > > (and back-dating) the older tags. But you could fetch it now, store it
> > > somewhere trusted (e.g., on your laptop), and wait two weeks. If you
> > > find no public outcry over hacked git, then it is probably OK to assume
> > > that is the real key.
> > >
> > 
> > With all of us pointing out 96AFE6CB being the right hash, you may or may 
> > not
> > trust the list enough to also trust the key now.
> 
> Who's to assume that I actually checked that 96AFE6CB is right? ;)
> 
> Actually, I don't typically verify Junio's tag signatures. I fetch and
> run "make" daily, far more often than he signs, so I would have been
> p0wned long ago.

It might also be worthwhile to check that the signatures on kernel.org
match the key in the repo.  kernel.org autosigns the tarballs as well,
so presumably that key matches what kernel.org has on file for Junio.

It may also be less important that the key really belongs to a human
named Junio C Hamano than that the same key consistently signs tags and
tarballs.  I can't personally vouch for the human behind the signatures,
but when building git from tarballs, I do check that the same key signed
them.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH v2 0/3] format-patch: introduce option to suppress commit hashes

[brian m. carlson]

On Mon, Dec 14, 2015 at 04:32:39PM -0500, Jeff King wrote:
> The intent here makes sense to me, and with the exception of the
> test_line_count thing that Torsten mentioned, the code looks good.
> 
> I briefly wondered if the option should simply be "--diffable" or
> something like that, and trigger this new behavior as well as implying
> --no-signature. Along with any other relevant options (if any; I don't
> recall if --stat-width is terminal-dependent for format-patch, for
> example).
> 
> But that is probably overkill. People can flip those switches
> individually if they want to (and even if somebody did want
> "--diffable", it may make sense to build it on top, so they can flip the
> zero-commit thing individually if they want).

That does sound like a potentially worthwhile thing to build on top at
some point.

I'll reroll with the other suggested changes and a slight tweak to make
the tests less dependent on the history in both cases.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

[PATCH v3 2/3] format-patch: add an option to suppress commit hash

[brian m. carlson]

Oftentimes, patches created by git format-patch will be stored in
version control or compared with diff.  In these cases, two otherwise
identical patches can have different commit hashes, leading to diff
noise.  Teach git format-patch a --zero-commit option that instead
produces an all-zero hash to avoid this diff noise.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 Documentation/git-format-patch.txt | 4 
 builtin/log.c  | 5 +
 log-tree.c | 3 ++-
 revision.h | 1 +
 t/t4014-format-patch.sh| 7 +++
 5 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 40356491..e3cdaeb9 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -256,6 +256,10 @@ you can use `--suffix=-patch` to get 
`0001-description-of-my-change-patch`.
using this option cannot be applied properly, but they are
still useful for code review.
 
+--zero-commit::
+  Output an all-zero hash in each patch's From header instead
+  of the hash of the commit.
+
 --root::
Treat the revision argument as a , even if it
is just a single commit (that would normally be treated as a
diff --git a/builtin/log.c b/builtin/log.c
index 069bd3a9..e00cea75 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1196,6 +1196,7 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
int cover_letter = -1;
int boundary_count = 0;
int no_binary_diff = 0;
+   int zero_commit = 0;
struct commit *origin = NULL;
const char *in_reply_to = NULL;
struct patch_ids ids;
@@ -1236,6 +1237,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_NOARG | PARSE_OPT_NONEG, keep_callback },
OPT_BOOL(0, "no-binary", _binary_diff,
 N_("don't output binary diffs")),
+   OPT_BOOL(0, "zero-commit", _commit,
+N_("output all-zero hash in From header")),
OPT_BOOL(0, "ignore-if-in-upstream", _if_in_upstream,
 N_("don't include a patch matching a commit 
upstream")),
{ OPTION_SET_INT, 'p', "no-stat", _patch_format, NULL,
@@ -1380,6 +1383,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
/* Always generate a patch */
rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
 
+   rev.zero_commit = zero_commit;
+
if (!DIFF_OPT_TST(, TEXT) && !no_binary_diff)
DIFF_OPT_SET(, BINARY);
 
diff --git a/log-tree.c b/log-tree.c
index 35e78017..f70a30e1 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -342,7 +342,8 @@ void log_write_email_headers(struct rev_info *opt, struct 
commit *commit,
 {
const char *subject = NULL;
const char *extra_headers = opt->extra_headers;
-   const char *name = oid_to_hex(>object.oid);
+   const char *name = oid_to_hex(opt->zero_commit ?
+ _oid : >object.oid);
 
*need_8bit_cte_p = 0; /* unknown */
if (opt->total > 0) {
diff --git a/revision.h b/revision.h
index 5bc96868..23857c0e 100644
--- a/revision.h
+++ b/revision.h
@@ -135,6 +135,7 @@ struct rev_info {
pretty_given:1,
abbrev_commit:1,
abbrev_commit_given:1,
+   zero_commit:1,
use_terminator:1,
missing_newline:1,
date_mode_explicit:1,
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 890db117..2737ca63 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1431,4 +1431,11 @@ test_expect_success 'cover letter auto user override' '
test_line_count = 2 list
 '
 
+test_expect_success 'format-patch --zero-commit' '
+   git format-patch --zero-commit --stdout v2..v1 >patch2 &&
+   grep "^From " patch2 | sort | uniq >actual &&
+   echo "From $_z40 Mon Sep 17 00:00:00 2001" >expect &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.7.0.rc0.194.g1187e4e.dirty

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

[PATCH v3 0/3] format-patch: introduce option to suppress commit hashes

[brian m. carlson]

git format-patch is often used to create patches that are then stored in
version control or displayed with diff.  Having the commit hash in the
"From " line usually just creates diff noise in these cases, so this
series introduces --zero-commit to set that to all zeros.

Changes from v2:
* Improve the tests to be more idiomatic and avoid hard-coding line
  counts.

brian m. carlson (3):
  Introduce a null_oid constant.
  format-patch: add an option to suppress commit hash
  format-patch: check that header line has expected format

 Documentation/git-format-patch.txt |  4 
 builtin/log.c  |  5 +
 cache.h|  1 +
 log-tree.c |  3 ++-
 revision.h |  1 +
 sha1_file.c|  1 +
 t/t4014-format-patch.sh| 14 ++
 7 files changed, 28 insertions(+), 1 deletion(-)

-- 
2.7.0.rc0.194.g1187e4e.dirty

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

[PATCH v3 1/3] Introduce a null_oid constant.

[brian m. carlson]

null_oid is the struct object_id equivalent to null_sha1.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 cache.h | 1 +
 sha1_file.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/cache.h b/cache.h
index 5ab6cb50..c63fcc11 100644
--- a/cache.h
+++ b/cache.h
@@ -831,6 +831,7 @@ extern const char *find_unique_abbrev(const unsigned char 
*sha1, int len);
 extern int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len);
 
 extern const unsigned char null_sha1[GIT_SHA1_RAWSZ];
+extern const struct object_id null_oid;
 
 static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
 {
diff --git a/sha1_file.c b/sha1_file.c
index 27ce7b70..a54deb05 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -36,6 +36,7 @@
 static inline uintmax_t sz_fmt(size_t s) { return s; }
 
 const unsigned char null_sha1[20];
+const struct object_id null_oid;
 
 /*
  * This is meant to hold a *small* number of objects that you would
-- 
2.7.0.rc0.194.g1187e4e.dirty

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

[PATCH v3 3/3] format-patch: check that header line has expected format

[brian m. carlson]

The format of the "From " header line is very specific to allow
utilities to detect Git-style patches.  Add a test that the patches
created are in the expected format.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 t/t4014-format-patch.sh | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 2737ca63..646c4750 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1438,4 +1438,11 @@ test_expect_success 'format-patch --zero-commit' '
test_cmp expect actual
 '
 
+test_expect_success 'From line has expected format' '
+   git format-patch --stdout v2..v1 >patch2 &&
+   grep "^From " patch2 >from &&
+   grep "^From $_x40 Mon Sep 17 00:00:00 2001$" patch2 >filtered &&
+   test_cmp from filtered
+'
+
 test_done
-- 
2.7.0.rc0.194.g1187e4e.dirty

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

[PATCH v2 1/3] Introduce a null_oid constant.

[brian m. carlson]

null_oid is the struct object_id equivalent to null_sha1.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 cache.h | 1 +
 sha1_file.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/cache.h b/cache.h
index 5ab6cb50..c63fcc11 100644
--- a/cache.h
+++ b/cache.h
@@ -831,6 +831,7 @@ extern const char *find_unique_abbrev(const unsigned char 
*sha1, int len);
 extern int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len);
 
 extern const unsigned char null_sha1[GIT_SHA1_RAWSZ];
+extern const struct object_id null_oid;
 
 static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
 {
diff --git a/sha1_file.c b/sha1_file.c
index 27ce7b70..a54deb05 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -36,6 +36,7 @@
 static inline uintmax_t sz_fmt(size_t s) { return s; }
 
 const unsigned char null_sha1[20];
+const struct object_id null_oid;
 
 /*
  * This is meant to hold a *small* number of objects that you would
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/3] format-patch: introduce option to suppress commit hashes

[brian m. carlson]

git format-patch is often used to create patches that are then stored in
version control or displayed with diff.  Having the commit hash in the
"From " line usually just creates diff noise in these cases, so this
series introduces --zero-commit to set that to all zeros.

Changes from v1:
* Rename the option --zero-commit.
* Improve the tests to look for a 40-hex hash value in "From " header.

brian m. carlson (3):
  Introduce a null_oid constant.
  format-patch: add an option to suppress commit hash
  format-patch: check that header line has expected format

 Documentation/git-format-patch.txt |  4 
 builtin/log.c  |  5 +
 cache.h|  1 +
 log-tree.c |  3 ++-
 revision.h |  1 +
 sha1_file.c|  1 +
 t/t4014-format-patch.sh| 12 
 7 files changed, 26 insertions(+), 1 deletion(-)

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

[PATCH v2 3/3] format-patch: check that header line has expected format

[brian m. carlson]

The format of the "From " header line is very specific to allow
utilities to detect Git-style patches.  Add a test that the patches
created are in the expected format.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 t/t4014-format-patch.sh | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index b740e3da..362bc228 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1437,4 +1437,10 @@ test_expect_success 'format-patch --zero-commit' '
test $cnt = 3
 '
 
+test_expect_success 'From line has expected format' '
+   git format-patch --stdout v2..v1 >patch2 &&
+   cnt=$(egrep "^From [0-9a-f]{40} Mon Sep 17 00:00:00 2001" patch2 | wc 
-l) &&
+   test $cnt = 3
+'
+
 test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/3] format-patch: add an option to suppress commit hash

[brian m. carlson]

Oftentimes, patches created by git format-patch will be stored in
version control or compared with diff.  In these cases, two otherwise
identical patches can have different commit hashes, leading to diff
noise.  Teach git format-patch a --zero-commit option that instead
produces an all-zero hash to avoid this diff noise.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 Documentation/git-format-patch.txt | 4 
 builtin/log.c  | 5 +
 log-tree.c | 3 ++-
 revision.h | 1 +
 t/t4014-format-patch.sh| 6 ++
 5 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 40356491..e3cdaeb9 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -256,6 +256,10 @@ you can use `--suffix=-patch` to get 
`0001-description-of-my-change-patch`.
using this option cannot be applied properly, but they are
still useful for code review.
 
+--zero-commit::
+  Output an all-zero hash in each patch's From header instead
+  of the hash of the commit.
+
 --root::
Treat the revision argument as a , even if it
is just a single commit (that would normally be treated as a
diff --git a/builtin/log.c b/builtin/log.c
index 069bd3a9..e00cea75 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1196,6 +1196,7 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
int cover_letter = -1;
int boundary_count = 0;
int no_binary_diff = 0;
+   int zero_commit = 0;
struct commit *origin = NULL;
const char *in_reply_to = NULL;
struct patch_ids ids;
@@ -1236,6 +1237,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_NOARG | PARSE_OPT_NONEG, keep_callback },
OPT_BOOL(0, "no-binary", _binary_diff,
 N_("don't output binary diffs")),
+   OPT_BOOL(0, "zero-commit", _commit,
+N_("output all-zero hash in From header")),
OPT_BOOL(0, "ignore-if-in-upstream", _if_in_upstream,
 N_("don't include a patch matching a commit 
upstream")),
{ OPTION_SET_INT, 'p', "no-stat", _patch_format, NULL,
@@ -1380,6 +1383,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
/* Always generate a patch */
rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
 
+   rev.zero_commit = zero_commit;
+
if (!DIFF_OPT_TST(, TEXT) && !no_binary_diff)
DIFF_OPT_SET(, BINARY);
 
diff --git a/log-tree.c b/log-tree.c
index 35e78017..f70a30e1 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -342,7 +342,8 @@ void log_write_email_headers(struct rev_info *opt, struct 
commit *commit,
 {
const char *subject = NULL;
const char *extra_headers = opt->extra_headers;
-   const char *name = oid_to_hex(>object.oid);
+   const char *name = oid_to_hex(opt->zero_commit ?
+ _oid : >object.oid);
 
*need_8bit_cte_p = 0; /* unknown */
if (opt->total > 0) {
diff --git a/revision.h b/revision.h
index 5bc96868..23857c0e 100644
--- a/revision.h
+++ b/revision.h
@@ -135,6 +135,7 @@ struct rev_info {
pretty_given:1,
abbrev_commit:1,
abbrev_commit_given:1,
+   zero_commit:1,
use_terminator:1,
missing_newline:1,
date_mode_explicit:1,
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 890db117..b740e3da 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1431,4 +1431,10 @@ test_expect_success 'cover letter auto user override' '
test_line_count = 2 list
 '
 
+test_expect_success 'format-patch --zero-commit' '
+   git format-patch --zero-commit --stdout v2..v1 >patch2 &&
+   cnt=$(egrep "^From 0{40} Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&
+   test $cnt = 3
+'
+
 test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: query regarding git merge

[brian m. carlson]

On Sun, Dec 13, 2015 at 05:55:59PM +, rohit gupta wrote:
> Hi,
> I am confused with git merge working.
> 
> Suppose I have these 3 files in master branch-
> a.txt
> b.txt
> d.txt
> 
> I create a branch, add c.txt to it and commit. So its final contents
> are-
> a.txt
> b.txt
> c.txt
> d.txt
> 
> Then, I checkout master branch, delete a.txt, add e.txt and commit. So
> final contents are-
> b.txt
> d.txt
> e.txt
> 
> Now when I merge branch in master,
> its result is-
> b.txt
> c.txt
> d.txt
> e.txt
> 
> Now suppose in branch, a.txt was needed for its working. And in master
> branch's latest commit a.txt was removed because maybe it wasn't needed
> or it was introducing bugs.
> Now, git merge removes that a.txt
> So now branch functionality wouldn't work.
> Isn't that wrong??

Instead of thinking of Git as merging two sets of files, think of it as
merging two sets of changes.  Git computes a merge base based on one or
more ancestors of both branches.  During a merge, Git takes the
differences on each side and combines them.  Logically, if a change is
made on one side but not the other, it will be preserved in the merge.

So in your case, you deleted a.txt on one side and did not modify it on
the other.  Git applied that change to the result of the merge.  Git has
no way of knowing that a.txt is still required in the result.

This is a very common question that comes up in a variety of different
forms.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: query regarding git merge

[brian m. carlson]

[Please don't top-post.]

On Mon, Dec 14, 2015 at 12:03:18AM +0530, Rohit Gupta wrote:
> Thanks brian. I understood my mistake in understanding the working of git
> merge.
> But isn't it wrong? As after merging, branch's logic can't work. How to get
> that right then ?

If you know that the merge didn't go the way you wanted, you can either
add a follow-up commit, or you can do "git commit --amend" on the merge
after making the necessary changes.  In such a case, it may be useful to
add a note to the commit message stating that you modified it from the
original merge.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH/RFC 0/2] add a perl compatible regex match flag to git describe

[brian m. carlson]

On Sun, Dec 27, 2015 at 11:59:50PM +0100, Mostyn Bramley-Moore wrote:
> git describe currently only supports glob matching with the --matches flag.
> It would be useful to support regular expressions.
> 
> PCRE is already an optional dependency from git grep, I wonder if posix or
> extended regexes would be preferable?
> 
> Some old discussion of this as a candidate feature is here, though nobody put
> together a patch as far as I can see:
> http://comments.gmane.org/gmane.comp.version-control.git/173873
> 
> Mostyn Bramley-Moore (2):
>   describe: mention glob in the --matches help text
>   describe: add --pcre-match option

If you're going to implement this, I'd recommend an option that adjusts
--matches to accept a PCRE regexp instead of adding a new option
accepting its own pattern.  I'd also recommend calling it --perl-regexp
for compatibility with git grep.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: What's cooking in git.git (Nov 2015, #04; Tue, 24)

[brian m. carlson]

On Sat, Nov 28, 2015 at 11:35:43AM -0500, Jeff King wrote:
> On Sat, Nov 28, 2015 at 03:40:10PM +0000, brian m. carlson wrote:
> 
> > On Tue, Nov 24, 2015 at 08:07:23PM -0500, Jeff King wrote:
> > > What's cooking in git.git (Nov 2015, #04; Tue, 24)
> > > --
> > > [New Topics]
> > 
> > I noticed the object_id series was missing from this list.  Was there
> > something that needed fixing or a reroll?
> 
> Thanks for bringing this up; I meant to send a note but forgot.
> 
> I got a bunch of conflicts trying to merge it into 'next' and 'pu' and
> punted on it. I think the tricky bits are coming from
> dt/refs-backend-pre-vtable, where there was a lot of code movement.

I think as for merging into the latest pu, the thing you want to do in
refs.c is simply take what pu has.  You'll have to fix up one additional
struct object call site.  The same thing goes for builtin/merge.c and
builtin/branch.c, where the code I changed has since been eliminated.

I also noticed that merge-recursive.c and builtin/ff-refs.c needed some
minor fixups as well, but a quick compile will show you where those are.
I've included a diff for those two below.

-%<-
diff --git a/builtin/ff-refs.c b/builtin/ff-refs.c
index ae68cfbc..c9d37092 100644
--- a/builtin/ff-refs.c
+++ b/builtin/ff-refs.c
@@ -84,19 +84,19 @@ static void do_ref_update(struct ff_ref_data *data, struct 
ff_ref_details *detai
set_git_dir(details->wt->git_dir);
read_index(_index);
 
-   if (checkout_fast_forward(details->branch_commit->object.sha1,
-   details->upstream_commit->object.sha1, 1))
+   if 
(checkout_fast_forward(details->branch_commit->object.oid.hash,
+   details->upstream_commit->object.oid.hash, 1))
details->result_type = NON_FAST_FORWARD;
-   else if (update_ref("ff-refs", refname, 
details->upstream_commit->object.sha1,
-   details->branch_commit->object.sha1, 0, 
UPDATE_REFS_QUIET_ON_ERR)) {
+   else if (update_ref("ff-refs", refname, 
details->upstream_commit->object.oid.hash,
+   details->branch_commit->object.oid.hash, 0, 
UPDATE_REFS_QUIET_ON_ERR)) {
details->result_type = UNABLE_TO_UPDATE;
run_hook_le(NULL, "post-merge", "0", NULL);
}
discard_index(_index);
chdir(path.buf);
strbuf_release();
-   } else if (update_ref("ff-refs", refname, 
details->upstream_commit->object.sha1,
-   details->branch_commit->object.sha1, 0, 
UPDATE_REFS_QUIET_ON_ERR))
+   } else if (update_ref("ff-refs", refname, 
details->upstream_commit->object.oid.hash,
+   details->branch_commit->object.oid.hash, 0, 
UPDATE_REFS_QUIET_ON_ERR))
details->result_type = UNABLE_TO_UPDATE;
 }
 
@@ -207,7 +207,7 @@ static int analize_refs(const char *refname,
details->upstream_commit);
details->merge_base = bases->item;
 
-   if (!hashcmp(upstream_hash, 
details->merge_base->object.sha1))
+   if (!hashcmp(upstream_hash, 
details->merge_base->object.oid.hash))
details->result_type = UP_TO_DATE;
 
else if (!in_merge_bases(details->branch_commit, 
details->upstream_commit))
diff --git a/merge-recursive.c b/merge-recursive.c
index 09d99640..50a16ebf 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1835,8 +1835,8 @@ int merge_trees(struct merge_options *o,
if (code != 0) {
if (o->gently)
return error(_("merging of trees %s and %s failed"),
-   sha1_to_hex(head->object.sha1),
-   sha1_to_hex(merge->object.sha1));
+   oid_to_hex(>object.oid),
+   oid_to_hex(>object.oid));
 
if (show(o, 4) || o->call_depth)
die(_("merging of trees %s and %s failed"),
-%<-
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: What's cooking in git.git (Nov 2015, #04; Tue, 24)

[brian m. carlson]

On Tue, Nov 24, 2015 at 08:07:23PM -0500, Jeff King wrote:
> What's cooking in git.git (Nov 2015, #04; Tue, 24)
> --
> [New Topics]

I noticed the object_id series was missing from this list.  Was there
something that needed fixing or a reroll?
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH v4 09/12] Add several uses of get_object_hash.

[brian m. carlson]

On Fri, Nov 20, 2015 at 06:38:30AM -0500, Jeff King wrote:
> On Tue, Nov 10, 2015 at 02:22:27AM +0000, brian m. carlson wrote:
> 
> > diff --git a/patch-ids.c b/patch-ids.c
> > index bf81b923..83229a0d 100644
> > --- a/patch-ids.c
> > +++ b/patch-ids.c
> > @@ -8,10 +8,10 @@ static int commit_patch_id(struct commit *commit, struct 
> > diff_options *options,
> > unsigned char *sha1)
> >  {
> > if (commit->parents)
> > -   diff_tree_sha1(commit->parents->item->object.sha1,
> > -  commit->object.sha1, "", options);
> > +   diff_tree_sha1(get_object_hash(commit->parents->item->object),
> > +  get_object_hash(commit->object), "", options);
> 
> I haven't looked carefully at this series yet, but while applying I
> noticed that git complained about whitespace here (long run of spaces
> which could be using tabs).

I believe that was in the original file, but I didn't fix it.  Git
doesn't show that by default, IIRC.  I probably should have put in a
preparatory patch.  My apologies.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Coccinelle for automated refactors

[brian m. carlson]

One thing that I've noticed with the struct object_id conversion is that
most of the work is mechanical transformations of a data member from one
type into another.  Doing this by hand is both boring and error-prone,
and it requires a tiresome review of nearly-identical changes.

I've noticed that Coccinelle[0], a tool for automated refactors, has
been used with great success on LKML, because it understands C well and
can perform the transformations precisely and rapidly.  It also does
nice things like indenting the code it modifies if necessary.

An example semantic patch looks like this:

@@
expression E1;
@@
- is_null_sha1(E1.hash)
+ is_null_oid()

@@
expression E1;
@@
- is_null_sha1(E1->hash)
+ is_null_oid(E1)

This does what you think it does: transforms calls to is_null_sha1 that
use the struct object_id hash member into calls to is_null_oid.

I'd like to use this for some of the struct object_id work if others
think this is a good idea.  I feel it's likely to reduce the reviewing
overhead and allow people to better reason about the quality and
behavior of the sent patches.  Of course, I would still review the
patches manually for errors and improvements, and would still accept
responsibility for the content of the patches.

If there's interest, I can send a patch with a set of basic object_id
transforms to make it easier for others to make those changes when
they're doing work elsewhere in the codebase.

[0] http://coccinelle.lip6.fr/
-- 
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 1/8] Add basic Coccinelle transforms.

[brian m. carlson]

On Mon, Jun 06, 2016 at 07:28:28PM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sand...@crustytoothpaste.net> writes:
> 
> > Coccinelle (http://coccinelle.lip6.fr/) is a program which performs
> > mechanical transformations on C programs using semantic patches.  These
> > semantic patches can be used to implement automatic refactoring and
> > maintenance tasks.
> >
> > Add a set of basic semantic patches to convert common patterns related
> > to the struct object_id transformation.
> >
> > Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
> > ---
> > I realize the name and location of this file might be suboptimal.
> > Suggestions on better locations and filenames would be appreciated.
> 
> Once is_null_sha1() is updated to is_null_oid(), the first rewrite
> definition would become useless, no?  I am not sure what the point
> is to keep this file in our history.

It might not be generally useful to keep forever.  I generally have
applied this set of transforms after each of the other semantic patches,
but I can simply refer to "a standard set of transforms" if you think
that's better, or I can post a URL with those somewhere to refer to.

My goal here is simply to avoid needing to include this set of
transformations in each commit message, which would tend to bloat it
considerably.
-- 
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 4/8] Rename struct diff_filespec's sha1_valid member.

[brian m. carlson]

On Tue, Jun 07, 2016 at 12:14:42PM -0700, Junio C Hamano wrote:
> Jeff King <p...@peff.net> writes:
> 
> > On Tue, Jun 07, 2016 at 08:21:26AM +0200, Johannes Sixt wrote:
> >
> >> > diff --git a/combine-diff.c b/combine-diff.c
> >> > index f39be434..a20caa80 100644
> >> > --- a/combine-diff.c
> >> > +++ b/combine-diff.c
> >> > @@ -1269,7 +1269,7 @@ static struct diff_filepair *combined_pair(struct 
> >> > combine_diff_path *p,
> >> >  pair->one[i].path = p->path;
> >> >  pair->one[i].mode = p->parent[i].mode;
> >> >  oidcpy(>one->oid, >parent[i].oid);
> >> > -pair->one[i].sha1_valid = 
> >> > !is_null_oid(>parent[i].oid);
> >> > +pair->one->oid_valid = !is_null_oid(>parent[i].oid);
> >> 
> >> Is this transformation correct?
> >
> > Or the oidcpy() above it, which was introduced in patch 3.
> 
> Indeed these look wrong.

Yes, those do look wrong.  I'll figure out where I went wrong when I
reroll.
-- 
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: Coccinelle for automated refactors

[brian m. carlson]

On Mon, Jun 06, 2016 at 11:55:50AM -0700, Junio C Hamano wrote:
> Is the plan for such a "refactor" patch to compose such a series as
> two patch series:
> 
>  [1/2] automatic refactor
> 
> which gives the "semantic patch" in the proposed log message as part
> of its description, and the automated result (with possible
> misconversions that may have come from bugs in the automated tools),
> with a separate
> 
>  [2/2] manual fixups
> 
> that corrects what was misconverted and what was missed?
> 
> As long as [2/2] can be kept to the minimum (and an automated tool
> that is worth using should make it so), I think that is a good way
> forward.  Another possibility would be to send the end-result as a
> single patch, with description on the manual fixups in the proposed
> log message, but it would be a lot more work to generate and review
> such a patch, I would think.

My hope is that I can make the automated changes such that manual fixups
are more along the lines of cleaning up related functions in the module,
fixing issues noticed during the refactor, and the like: in other words,
things that one might have done incidentally as part of the patch, but
could defensibly be done in a second patch anyway.

My goal is to make the series as much like a human-edited series as
possible, but with less work on all sides.

I can send an RFC series that demonstrates this a little later to see if
it's an acceptable direction for work.
-- 
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

[PATCH 6/8] merge-recursive: convert struct merge_file_info to object_id

[brian m. carlson]

Convert struct merge_file_info to use struct object_id.  The following
Coccinelle semantic patch was used to implement this, followed by the
transformations in standard.cocci:

@@
struct merge_file_info *p;
@@
- p->sha
+ p->oid.hash

@@
struct merge_file_info o;
@@
- o.sha
+ o.oid.hash

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 merge-recursive.c | 39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index a07050cd..bc455491 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -819,7 +819,7 @@ static void update_file(struct merge_options *o,
 /* Low level file merging, update and removal */
 
 struct merge_file_info {
-   unsigned char sha[20];
+   struct object_id oid;
unsigned mode;
unsigned clean:1,
 merge:1;
@@ -902,10 +902,10 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
result.clean = 0;
if (S_ISREG(a->mode)) {
result.mode = a->mode;
-   hashcpy(result.sha, a->oid.hash);
+   oidcpy(, >oid);
} else {
result.mode = b->mode;
-   hashcpy(result.sha, b->oid.hash);
+   oidcpy(, >oid);
}
} else {
if (!sha_eq(a->oid.hash, one->oid.hash) && !sha_eq(b->oid.hash, 
one->oid.hash))
@@ -925,9 +925,9 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
}
 
if (sha_eq(a->oid.hash, b->oid.hash) || sha_eq(a->oid.hash, 
one->oid.hash))
-   hashcpy(result.sha, b->oid.hash);
+   oidcpy(, >oid);
else if (sha_eq(b->oid.hash, one->oid.hash))
-   hashcpy(result.sha, a->oid.hash);
+   oidcpy(, >oid);
else if (S_ISREG(a->mode)) {
mmbuffer_t result_buf;
int merge_status;
@@ -939,21 +939,21 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
die(_("Failed to execute internal merge"));
 
if (write_sha1_file(result_buf.ptr, result_buf.size,
-   blob_type, result.sha))
+   blob_type, result.oid.hash))
die(_("Unable to add %s to database"),
a->path);
 
free(result_buf.ptr);
result.clean = (merge_status == 0);
} else if (S_ISGITLINK(a->mode)) {
-   result.clean = merge_submodule(result.sha,
+   result.clean = merge_submodule(result.oid.hash,
   one->path,
   one->oid.hash,
   a->oid.hash,
   b->oid.hash,
   !o->call_depth);
} else if (S_ISLNK(a->mode)) {
-   hashcpy(result.sha, a->oid.hash);
+   oidcpy(, >oid);
 
if (!sha_eq(a->oid.hash, b->oid.hash))
result.clean = 0;
@@ -1192,7 +1192,7 @@ static void conflict_rename_rename_1to2(struct 
merge_options *o,
 * pathname and then either rename the add-source file to that
 * unique path, or use that unique path instead of src here.
 */
-   update_file(o, 0, mfi.sha, mfi.mode, one->path);
+   update_file(o, 0, mfi.oid.hash, mfi.mode, one->path);
 
/*
 * Above, we put the merged content at the merge-base's
@@ -1255,16 +1255,16 @@ static void conflict_rename_rename_2to1(struct 
merge_options *o,
 * again later for the non-recursive merge.
 */
remove_file(o, 0, path, 0);
-   update_file(o, 0, mfi_c1.sha, mfi_c1.mode, a->path);
-   update_file(o, 0, mfi_c2.sha, mfi_c2.mode, b->path);
+   update_file(o, 0, mfi_c1.oid.hash, mfi_c1.mode, a->path);
+   update_file(o, 0, mfi_c2.oid.hash, mfi_c2.mode, b->path);
} else {
char *new_path1 = unique_path(o, path, ci->branch1);
char *new_path2 = unique_path(o, path, ci->branch2);
output(o, 1, _("Renaming %s to %s and %s to %s instead"),
   a->path, new_path1, b->path, new_path2);
remove_file(o, 0,

[PATCH 5/8] merge-recursive: convert struct stage_data to use object_id

[brian m. carlson]

Convert the anonymous struct within struct stage_data to use struct
object_id.  The following Coccinelle semantic patch was used to
implement this, followed by the transformations in standard.cocci:

@@
struct stage_data *p;
expression E1;
@@
- p->stages[E1].sha
+ p->stages[E1].oid.hash

@@
struct stage_data o;
expression E1;
@@
- o.stages[E1].sha
+ o.stages[E1].oid.hash

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 merge-recursive.c | 38 ++
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1e802097..a07050cd 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -90,7 +90,7 @@ struct rename_conflict_info {
 struct stage_data {
struct {
unsigned mode;
-   unsigned char sha[20];
+   struct object_id oid;
} stages[4];
struct rename_conflict_info *rename_conflict_info;
unsigned processed:1;
@@ -134,13 +134,11 @@ static inline void setup_rename_conflict_info(enum 
rename_type rename_type,
int ostage2 = ostage1 ^ 1;
 
ci->ren1_other.path = pair1->one->path;
-   hashcpy(ci->ren1_other.oid.hash,
-   src_entry1->stages[ostage1].sha);
+   oidcpy(>ren1_other.oid, _entry1->stages[ostage1].oid);
ci->ren1_other.mode = src_entry1->stages[ostage1].mode;
 
ci->ren2_other.path = pair2->one->path;
-   hashcpy(ci->ren2_other.oid.hash,
-   src_entry2->stages[ostage2].sha);
+   oidcpy(>ren2_other.oid, _entry2->stages[ostage2].oid);
ci->ren2_other.mode = src_entry2->stages[ostage2].mode;
}
 }
@@ -316,11 +314,11 @@ static struct stage_data *insert_stage_data(const char 
*path,
struct string_list_item *item;
struct stage_data *e = xcalloc(1, sizeof(struct stage_data));
get_tree_entry(o->object.oid.hash, path,
-   e->stages[1].sha, >stages[1].mode);
+   e->stages[1].oid.hash, >stages[1].mode);
get_tree_entry(a->object.oid.hash, path,
-   e->stages[2].sha, >stages[2].mode);
+   e->stages[2].oid.hash, >stages[2].mode);
get_tree_entry(b->object.oid.hash, path,
-   e->stages[3].sha, >stages[3].mode);
+   e->stages[3].oid.hash, >stages[3].mode);
item = string_list_insert(entries, path);
item->util = e;
return e;
@@ -351,7 +349,7 @@ static struct string_list *get_unmerged(void)
}
e = item->util;
e->stages[ce_stage(ce)].mode = ce->ce_mode;
-   hashcpy(e->stages[ce_stage(ce)].sha, ce->sha1);
+   hashcpy(e->stages[ce_stage(ce)].oid.hash, ce->sha1);
}
 
return unmerged;
@@ -574,9 +572,9 @@ static void update_entry(struct stage_data *entry,
entry->stages[1].mode = o->mode;
entry->stages[2].mode = a->mode;
entry->stages[3].mode = b->mode;
-   hashcpy(entry->stages[1].sha, o->oid.hash);
-   hashcpy(entry->stages[2].sha, a->oid.hash);
-   hashcpy(entry->stages[3].sha, b->oid.hash);
+   oidcpy(>stages[1].oid, >oid);
+   oidcpy(>stages[2].oid, >oid);
+   oidcpy(>stages[3].oid, >oid);
 }
 
 static int remove_file(struct merge_options *o, int clean,
@@ -,7 +1109,7 @@ static struct diff_filespec *filespec_from_entry(struct 
diff_filespec *target,
 struct stage_data *entry,
 int stage)
 {
-   unsigned char *sha = entry->stages[stage].sha;
+   unsigned char *sha = entry->stages[stage].oid.hash;
unsigned mode = entry->stages[stage].mode;
if (mode == 0 || is_null_sha1(sha))
return NULL;
@@ -1425,11 +1423,11 @@ static int process_renames(struct merge_options *o,
remove_file(o, 1, ren1_src,
renamed_stage == 2 || 
!was_tracked(ren1_src));
 
-   hashcpy(src_other.oid.hash,
-   ren1->src_entry->stages[other_stage].sha);
+   oidcpy(_other.oid,
+  >src_entry->stages[other_stage].oid);
src_other.mode = 
ren1->src_entry->stages[other_stage].mode;
-   hashcpy(dst_other.oid.hash,
-   ren1->dst_entry->stages[other_stage].sha);
+   oidcpy(_other.oid,
+  >dst_entry->stages[other_stage].oid);
dst_other.mode = 
ren1->dst_entry->s

[PATCH 7/8] merge-recursive: convert leaf functions to use struct object_id

[brian m. carlson]

Convert all but two of the static functions in this file to use struct
object_id.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 merge-recursive.c | 236 +++---
 1 file changed, 118 insertions(+), 118 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index bc455491..7bbd4aea 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -56,11 +56,11 @@ static struct commit *make_virtual_commit(struct tree 
*tree, const char *comment
  * Since we use get_tree_entry(), which does not put the read object into
  * the object pool, we cannot rely on a == b.
  */
-static int sha_eq(const unsigned char *a, const unsigned char *b)
+static int oid_eq(const struct object_id *a, const struct object_id *b)
 {
if (!a && !b)
return 2;
-   return a && b && hashcmp(a, b) == 0;
+   return a && b && oidcmp(a, b) == 0;
 }
 
 enum rename_type {
@@ -198,11 +198,11 @@ static void output_commit_title(struct merge_options *o, 
struct commit *commit)
}
 }
 
-static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
+static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
const char *path, int stage, int refresh, int options)
 {
struct cache_entry *ce;
-   ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage,
+   ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage,
  (refresh ? (CE_MATCH_REFRESH |
  CE_MATCH_IGNORE_MISSING) : 0 ));
if (!ce)
@@ -552,13 +552,13 @@ static int update_stages(const char *path, const struct 
diff_filespec *o,
if (remove_file_from_cache(path))
return -1;
if (o)
-   if (add_cacheinfo(o->mode, o->oid.hash, path, 1, 0, options))
+   if (add_cacheinfo(o->mode, >oid, path, 1, 0, options))
return -1;
if (a)
-   if (add_cacheinfo(a->mode, a->oid.hash, path, 2, 0, options))
+   if (add_cacheinfo(a->mode, >oid, path, 2, 0, options))
return -1;
if (b)
-   if (add_cacheinfo(b->mode, b->oid.hash, path, 3, 0, options))
+   if (add_cacheinfo(b->mode, >oid, path, 3, 0, options))
return -1;
return 0;
 }
@@ -736,7 +736,7 @@ static int make_room_for_path(struct merge_options *o, 
const char *path)
 }
 
 static void update_file_flags(struct merge_options *o,
- const unsigned char *sha,
+ const struct object_id *oid,
  unsigned mode,
  const char *path,
  int update_cache,
@@ -760,11 +760,11 @@ static void update_file_flags(struct merge_options *o,
goto update_index;
}
 
-   buf = read_sha1_file(sha, , );
+   buf = read_sha1_file(oid->hash, , );
if (!buf)
-   die(_("cannot read object %s '%s'"), sha1_to_hex(sha), 
path);
+   die(_("cannot read object %s '%s'"), oid_to_hex(oid), 
path);
if (type != OBJ_BLOB)
-   die(_("blob expected for %s '%s'"), sha1_to_hex(sha), 
path);
+   die(_("blob expected for %s '%s'"), oid_to_hex(oid), 
path);
if (S_ISREG(mode)) {
struct strbuf strbuf = STRBUF_INIT;
if (convert_to_working_tree(path, buf, size, )) {
@@ -799,21 +799,21 @@ static void update_file_flags(struct merge_options *o,
free(lnk);
} else
die(_("do not know what to do with %06o %s '%s'"),
-   mode, sha1_to_hex(sha), path);
+   mode, oid_to_hex(oid), path);
free(buf);
}
  update_index:
if (update_cache)
-   add_cacheinfo(mode, sha, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD);
+   add_cacheinfo(mode, oid, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD);
 }
 
 static void update_file(struct merge_options *o,
int clean,
-   const unsigned char *sha,
+   const struct object_id *oid,
unsigned mode,
const char *path)
 {
-   update_file_flags(o, sha, mode, path, o->call_depth || clean, 
!o->call_depth);
+   update_file_flags(o, oid, mode, path, o->call_depth || clean, 
!o->call_depth);
 }
 
 /* Low level file merging, update and removal */
@@ -908,7 +908,7 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
   

[RFC][PATCH 0/8] struct object_id, Part 4

[brian m. carlson]

This series is part 4 in a series of conversions to replace instances of
unsigned char [20] with struct object_id.  Most of this series touches
the merge-recursive code.

New in this series is the use of Coccinelle (http://coccinelle.lip6.fr/)
semantic patches.  These semantic patches can make automatic
transformations to C source code for cleanup or refactoring reasons.

This series introduces a set of transforms for the struct object_id
transition, cleans up some existing code with them, and applies a small
number of semantic patches to transform parts of the merge-recursive
code.  Some manual refactoring work follows.

Note that in the patches created with the semantic patches, the only
manual change was the definition of the struct member.  This series is
marked as RFC to see if this is a viable technique for further work to
ease both the creation and review of patches.  Opinions are of course
welcomed.

The testsuite continues to pass at each step.

brian m. carlson (8):
  Add basic Coccinelle transforms.
  Apply standard object_id Coccinelle transformations.
  Convert struct diff_filespec to struct object_id
  Rename struct diff_filespec's sha1_valid member.
  merge-recursive: convert struct stage_data to use object_id
  merge-recursive: convert struct merge_file_info to object_id
  merge-recursive: convert leaf functions to use struct object_id
  merge-recursive: convert merge_recursive_generic to object_id

 bisect.c  |   2 +-
 builtin/blame.c   |   6 +-
 builtin/fast-export.c |  10 +-
 builtin/merge-recursive.c |  20 +--
 builtin/merge.c   |  13 +-
 builtin/reset.c   |   4 +-
 combine-diff.c|  14 +--
 diff.c|  95 +++---
 diffcore-break.c  |   4 +-
 diffcore-rename.c |  16 +--
 diffcore.h|   4 +-
 line-log.c|  12 +-
 merge-recursive.c | 310 --
 merge-recursive.h |   6 +-
 notes-merge.c |  42 +++
 refs/files-backend.c  |   4 +-
 standard.cocci|  83 +
 submodule.c   |   4 +-
 wt-status.c   |   3 +-
 19 files changed, 376 insertions(+), 276 deletions(-)
 create mode 100644 standard.cocci

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

[PATCH 4/8] Rename struct diff_filespec's sha1_valid member.

[brian m. carlson]

Now that this struct's sha1 member is called "oid", update the comment
and the sha1_valid member to be called "oid_valid" instead.  The
following Coccinelle semantic patch was used to implement this, followed
by the transformations in standard.cocci:

@@
struct diff_filespec *p;
@@
- p->sha1_valid
+ p->oid_valid

@@
struct diff_filespec o;
@@
- o.sha1_valid
+ o.oid_valid

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 combine-diff.c|  4 ++--
 diff.c| 28 ++--
 diffcore-break.c  |  2 +-
 diffcore-rename.c |  4 ++--
 diffcore.h|  2 +-
 line-log.c| 10 +-
 6 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index f39be434..a20caa80 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1269,7 +1269,7 @@ static struct diff_filepair *combined_pair(struct 
combine_diff_path *p,
pair->one[i].path = p->path;
pair->one[i].mode = p->parent[i].mode;
oidcpy(>one->oid, >parent[i].oid);
-   pair->one[i].sha1_valid = !is_null_oid(>parent[i].oid);
+   pair->one->oid_valid = !is_null_oid(>parent[i].oid);
pair->one[i].has_more_entries = 1;
}
pair->one[num_parent - 1].has_more_entries = 0;
@@ -1277,7 +1277,7 @@ static struct diff_filepair *combined_pair(struct 
combine_diff_path *p,
pair->two->path = p->path;
pair->two->mode = p->mode;
oidcpy(>two->oid, >oid);
-   pair->two->sha1_valid = !is_null_oid(>oid);
+   pair->two->oid_valid = !is_null_oid(>oid);
return pair;
 }
 
diff --git a/diff.c b/diff.c
index 34a3875c..5cabaaf0 100644
--- a/diff.c
+++ b/diff.c
@@ -1933,7 +1933,7 @@ static void show_dirstat(struct diff_options *options)
 
name = p->two->path ? p->two->path : p->one->path;
 
-   if (p->one->sha1_valid && p->two->sha1_valid)
+   if (p->one->oid_valid && p->two->oid_valid)
content_changed = oidcmp(>one->oid, >two->oid);
else
content_changed = 1;
@@ -2640,7 +2640,7 @@ void fill_filespec(struct diff_filespec *spec, const 
unsigned char *sha1,
if (mode) {
spec->mode = canon_mode(mode);
hashcpy(spec->oid.hash, sha1);
-   spec->sha1_valid = sha1_valid;
+   spec->oid_valid = sha1_valid;
}
 }
 
@@ -2766,7 +2766,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
if (S_ISGITLINK(s->mode))
return diff_populate_gitlink(s, size_only);
 
-   if (!s->sha1_valid ||
+   if (!s->oid_valid ||
reuse_worktree_file(s->path, s->oid.hash, 0)) {
struct strbuf buf = STRBUF_INIT;
struct stat st;
@@ -2915,7 +2915,7 @@ static struct diff_tempfile *prepare_temp_file(const char 
*name,
}
 
if (!S_ISGITLINK(one->mode) &&
-   (!one->sha1_valid ||
+   (!one->oid_valid ||
 reuse_worktree_file(name, one->oid.hash, 1))) {
struct stat st;
if (lstat(name, ) < 0) {
@@ -2928,16 +2928,16 @@ static struct diff_tempfile *prepare_temp_file(const 
char *name,
if (strbuf_readlink(, name, st.st_size) < 0)
die_errno("readlink(%s)", name);
prep_temp_blob(name, temp, sb.buf, sb.len,
-  (one->sha1_valid ?
+  (one->oid_valid ?
one->oid.hash : null_sha1),
-  (one->sha1_valid ?
+  (one->oid_valid ?
one->mode : S_IFLNK));
strbuf_release();
}
else {
/* we can borrow from the file in the work tree */
temp->name = name;
-   if (!one->sha1_valid)
+   if (!one->oid_valid)
sha1_to_hex_r(temp->hex, null_sha1);
else
sha1_to_hex_r(temp->hex, one->oid.hash);
@@ -3134,7 +3134,7 @@ static void run_diff_cmd(const char *pgm,
 static void diff_fill_sha1_info(struct diff_filespec *one)
 {
if (DIFF_FILE_VALID(one)) {
-   if (!one->sha1_valid) {
+   if (!one->oid_valid) {
struct stat st;
if (one->is_stdin) {
hashcpy(one->oid.has

[PATCH 3/8] Convert struct diff_filespec to struct object_id

[brian m. carlson]

Convert struct diff_filespec's sha1 member to use a struct object_id
called "oid" instead.  The following Coccinelle semantic patch was used
to implement this, followed by the transformations in standard.cocci:

@@
struct diff_filespec *p;
@@
- p->sha1
+ p->oid.hash

@@
struct diff_filespec o;
@@
- o.sha1
+ o.oid.hash

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 builtin/blame.c   |   6 +--
 builtin/fast-export.c |  10 ++---
 builtin/reset.c   |   4 +-
 combine-diff.c|  10 ++---
 diff.c|  69 +---
 diffcore-break.c  |   2 +-
 diffcore-rename.c |  14 ---
 diffcore.h|   2 +-
 line-log.c|   2 +-
 merge-recursive.c | 107 +++---
 notes-merge.c |  42 ++--
 submodule.c   |   4 +-
 wt-status.c   |   3 +-
 13 files changed, 147 insertions(+), 128 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 21f42b0b..04bc4e0e 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -599,7 +599,7 @@ static struct origin *find_origin(struct scoreboard *sb,
p->status);
case 'M':
porigin = get_origin(sb, parent, origin->path);
-   hashcpy(porigin->blob_sha1, p->one->sha1);
+   hashcpy(porigin->blob_sha1, p->one->oid.hash);
porigin->mode = p->one->mode;
break;
case 'A':
@@ -645,7 +645,7 @@ static struct origin *find_rename(struct scoreboard *sb,
if ((p->status == 'R' || p->status == 'C') &&
!strcmp(p->two->path, origin->path)) {
porigin = get_origin(sb, parent, p->one->path);
-   hashcpy(porigin->blob_sha1, p->one->sha1);
+   hashcpy(porigin->blob_sha1, p->one->oid.hash);
porigin->mode = p->one->mode;
break;
}
@@ -1309,7 +1309,7 @@ static void find_copy_in_parent(struct scoreboard *sb,
continue;
 
norigin = get_origin(sb, parent, p->one->path);
-   hashcpy(norigin->blob_sha1, p->one->sha1);
+   hashcpy(norigin->blob_sha1, p->one->oid.hash);
norigin->mode = p->one->mode;
fill_origin_blob(>revs->diffopt, norigin, _p);
if (!file_p.ptr)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 8164b581..c0652a7e 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -368,7 +368,7 @@ static void show_filemodify(struct diff_queue_struct *q,
print_path(spec->path);
putchar('\n');
 
-   if (!hashcmp(ospec->sha1, spec->sha1) &&
+   if (!oidcmp(>oid, >oid) &&
ospec->mode == spec->mode)
break;
/* fallthrough */
@@ -383,10 +383,10 @@ 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->sha1) :
-  spec->sha1));
+  
anonymize_sha1(spec->oid.hash) :
+  spec->oid.hash));
else {
-   struct object *object = 
lookup_object(spec->sha1);
+   struct object *object = 
lookup_object(spec->oid.hash);
printf("M %06o :%d ", spec->mode,
   get_object_mark(object));
}
@@ -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->sha1);
+   export_blob(diff_queued_diff.queue[i]->two->oid.hash);
 
refname = commit->util;
if (anonymize) {
diff --git a/builtin/reset.c b/builtin/reset.c
index 092c3a53..cce64b53 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -121,7 +121,7 @@ static void update_index_from_diff(struct di

[PATCH 2/8] Apply standard object_id Coccinelle transformations.

[brian m. carlson]

Apply the standard set of semantic patches to convert some leftover
places using struct object_id's hash member to instead use the wrapper
functions that take struct object_id natively.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 bisect.c |  2 +-
 builtin/merge.c  | 13 ++---
 refs/files-backend.c |  4 ++--
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/bisect.c b/bisect.c
index 6d93edbc..ff147589 100644
--- a/bisect.c
+++ b/bisect.c
@@ -754,7 +754,7 @@ static void handle_bad_merge_base(void)
 static void handle_skipped_merge_base(const unsigned char *mb)
 {
char *mb_hex = sha1_to_hex(mb);
-   char *bad_hex = sha1_to_hex(current_bad_oid->hash);
+   char *bad_hex = oid_to_hex(current_bad_oid);
char *good_hex = join_sha1_array_hex(_revs, ' ');
 
warning("the merge base between %s and [%s] "
diff --git a/builtin/merge.c b/builtin/merge.c
index b555a1bf..1e7be852 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -501,7 +501,7 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
if (ref_exists(truname.buf)) {
strbuf_addf(msg,
"%s\t\tbranch '%s'%s of .\n",
-   sha1_to_hex(remote_head->object.oid.hash),
+   oid_to_hex(_head->object.oid),
truname.buf + 11,
(early ? " (early part)" : ""));
strbuf_release();
@@ -515,7 +515,7 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
desc = merge_remote_util(remote_head);
if (desc && desc->obj && desc->obj->type == OBJ_TAG) {
strbuf_addf(msg, "%s\t\t%s '%s'\n",
-   sha1_to_hex(desc->obj->oid.hash),
+   oid_to_hex(>obj->oid),
typename(desc->obj->type),
remote);
goto cleanup;
@@ -523,7 +523,7 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
}
 
strbuf_addf(msg, "%s\t\tcommit '%s'\n",
-   sha1_to_hex(remote_head->object.oid.hash), remote);
+   oid_to_hex(_head->object.oid), remote);
 cleanup:
strbuf_release();
strbuf_release();
@@ -1366,7 +1366,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
for (p = remoteheads; p; p = p->next) {
struct commit *commit = p->item;
strbuf_addf(, "GITHEAD_%s",
-   sha1_to_hex(commit->object.oid.hash));
+   oid_to_hex(>object.oid));
setenv(buf.buf, merge_remote_util(commit)->name, 1);
strbuf_reset();
if (fast_forward != FF_ONLY &&
@@ -1425,7 +1425,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
goto done;
} else if (fast_forward != FF_NO && !remoteheads->next &&
!common->next &&
-   !hashcmp(common->item->object.oid.hash, 
head_commit->object.oid.hash)) {
+   !oidcmp(>item->object.oid, 
_commit->object.oid)) {
/* Again the most common case of merging one remote. */
struct strbuf msg = STRBUF_INIT;
struct commit *commit;
@@ -1499,8 +1499,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
 * HEAD^^" would be missed.
 */
common_one = get_merge_bases(head_commit, j->item);
-   if (hashcmp(common_one->item->object.oid.hash,
-   j->item->object.oid.hash)) {
+   if (oidcmp(_one->item->object.oid, 
>item->object.oid)) {
up_to_date = 0;
break;
}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1f380764..dac3a222 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1725,14 +1725,14 @@ static int verify_lock(struct ref_lock *lock,
errno = save_errno;
return -1;
} else {
-   hashclr(lock->old_oid.hash);
+   oidclr(>old_oid);
return 0;
}
}
if (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) {
strbuf_addf(err, "ref %s is at %s but expected %s",
lock->ref_name,
- 

[PATCH 1/8] Add basic Coccinelle transforms.

[brian m. carlson]

Coccinelle (http://coccinelle.lip6.fr/) is a program which performs
mechanical transformations on C programs using semantic patches.  These
semantic patches can be used to implement automatic refactoring and
maintenance tasks.

Add a set of basic semantic patches to convert common patterns related
to the struct object_id transformation.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
I realize the name and location of this file might be suboptimal.
Suggestions on better locations and filenames would be appreciated.

 standard.cocci | 83 ++
 1 file changed, 83 insertions(+)
 create mode 100644 standard.cocci

diff --git a/standard.cocci b/standard.cocci
new file mode 100644
index ..0f068252
--- /dev/null
+++ b/standard.cocci
@@ -0,0 +1,83 @@
+@@
+expression E1;
+@@
+- is_null_sha1(E1.hash)
++ is_null_oid()
+
+@@
+expression E1;
+@@
+- is_null_sha1(E1->hash)
++ is_null_oid(E1)
+
+@@
+expression E1;
+@@
+- sha1_to_hex(E1.hash)
++ oid_to_hex()
+
+@@
+expression E1;
+@@
+- sha1_to_hex(E1->hash)
++ oid_to_hex(E1)
+
+@@
+expression E1;
+@@
+- hashclr(E1.hash)
++ oidclr()
+
+@@
+expression E1;
+@@
+- hashclr(E1->hash)
++ oidclr(E1)
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1.hash, E2.hash)
++ oidcmp(, )
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1->hash, E2->hash)
++ oidcmp(E1, E2)
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1->hash, E2.hash)
++ oidcmp(E1, )
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1.hash, E2->hash)
++ oidcmp(, E2)
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1.hash, E2.hash)
++ oidcpy(, )
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1->hash, E2->hash)
++ oidcpy(E1, E2)
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1->hash, E2.hash)
++ oidcpy(E1, )
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1.hash, E2->hash)
++ oidcpy(, E2)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 8/8] merge-recursive: convert merge_recursive_generic to object_id

[brian m. carlson]

Convert this function and the git merge-recursive subcommand to use
struct object_id.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 builtin/merge-recursive.c | 20 ++--
 merge-recursive.c | 14 +++---
 merge-recursive.h |  6 +++---
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 491efd55..fd2c4556 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -9,10 +9,10 @@ static const char builtin_merge_recursive_usage[] =
 
 static const char *better_branch_name(const char *branch)
 {
-   static char githead_env[8 + 40 + 1];
+   static char githead_env[8 + GIT_SHA1_HEXSZ + 1];
char *name;
 
-   if (strlen(branch) != 40)
+   if (strlen(branch) != GIT_SHA1_HEXSZ)
return branch;
xsnprintf(githead_env, sizeof(githead_env), "GITHEAD_%s", branch);
name = getenv(githead_env);
@@ -21,10 +21,10 @@ static const char *better_branch_name(const char *branch)
 
 int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 {
-   const unsigned char *bases[21];
+   const struct object_id *bases[21];
unsigned bases_count = 0;
int i, failed;
-   unsigned char h1[20], h2[20];
+   struct object_id h1, h2;
struct merge_options o;
struct commit *result;
 
@@ -46,10 +46,10 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
continue;
}
if (bases_count < ARRAY_SIZE(bases)-1) {
-   unsigned char *sha = xmalloc(20);
-   if (get_sha1(argv[i], sha))
+   struct object_id *oid = xmalloc(sizeof(struct 
object_id));
+   if (get_oid(argv[i], oid))
die("Could not parse object '%s'", argv[i]);
-   bases[bases_count++] = sha;
+   bases[bases_count++] = oid;
}
else
warning("Cannot handle more than %d bases. "
@@ -62,9 +62,9 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
o.branch1 = argv[++i];
o.branch2 = argv[++i];
 
-   if (get_sha1(o.branch1, h1))
+   if (get_oid(o.branch1, ))
die("Could not resolve ref '%s'", o.branch1);
-   if (get_sha1(o.branch2, h2))
+   if (get_oid(o.branch2, ))
die("Could not resolve ref '%s'", o.branch2);
 
o.branch1 = better_branch_name(o.branch1);
@@ -73,7 +73,7 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
if (o.verbosity >= 3)
printf("Merging %s with %s\n", o.branch1, o.branch2);
 
-   failed = merge_recursive_generic(, h1, h2, bases_count, bases, 
);
+   failed = merge_recursive_generic(, , , bases_count, bases, 
);
if (failed < 0)
return 128; /* die() error code */
return failed;
diff --git a/merge-recursive.c b/merge-recursive.c
index 7bbd4aea..48fe7e73 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1982,11 +1982,11 @@ int merge_recursive(struct merge_options *o,
return clean;
 }
 
-static struct commit *get_ref(const unsigned char *sha1, const char *name)
+static struct commit *get_ref(const struct object_id *oid, const char *name)
 {
struct object *object;
 
-   object = deref_tag(parse_object(sha1), name, strlen(name));
+   object = deref_tag(parse_object(oid->hash), name, strlen(name));
if (!object)
return NULL;
if (object->type == OBJ_TREE)
@@ -1999,10 +1999,10 @@ static struct commit *get_ref(const unsigned char 
*sha1, const char *name)
 }
 
 int merge_recursive_generic(struct merge_options *o,
-   const unsigned char *head,
-   const unsigned char *merge,
+   const struct object_id *head,
+   const struct object_id *merge,
int num_base_list,
-   const unsigned char **base_list,
+   const struct object_id **base_list,
struct commit **result)
 {
int clean;
@@ -2015,9 +2015,9 @@ int merge_recursive_generic(struct merge_options *o,
int i;
for (i = 0; i < num_base_list; ++i) {
struct commit *base;
-   if (!(base = get_ref(base_list[i], 
sha1_to_hex(base_list[i]
+   if (!(base = get_ref(base_list[i], 
oid_to_hex(base_list[i]
return error(_("Could not parse object '%s'"),
-   sha1_to_hex(base_list[i]));
+   

Re: [PATCH v2 0/8] object_id part 4

[brian m. carlson]

On Tue, Jun 21, 2016 at 02:22:50PM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sand...@crustytoothpaste.net> writes:
> 
> > This series is part 4 in a series of conversions to replace instances of
> > unsigned char [20] with struct object_id.  Most of this series touches
> > the merge-recursive code.
> 
> I've queued them in 'pu', but please read contrib/examples/README
> ;-) Tentatively, I used contrib/coccinelle instead, but something
> even shorter (e.g. contrib/cocci) might be sufficient.

That seems fine with me.  I did read contrib/examples/README, but
figured it might be okay nevertheless.  I think contrib/coccinelle is
better, 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 v2 0/8] object_id part 4

[brian m. carlson]

On Sun, Jun 19, 2016 at 05:24:48AM -0400, Jeff King wrote:
> On Sun, Jun 19, 2016 at 10:50:14AM +0200, Johannes Sixt wrote:
> > To avoid future mistakes, can you write down how "transform plain structs
> > before pointers to structs" looks like? Is it a particular order of
> > Coccinelle rules? Which part of the interdiff between the previous round and
> > this round makes the difference?
> 
> Yeah, I'd also like a better understanding of what went wrong in the
> original (just to be able to better evaluate the tool).

I'm not completely clear on why Coccinelle made the transform that it
did.  If you look at the commit messages, the new patch is this:

  @@
  struct diff_filespec o;
  @@
  - o.sha1
  + o.oid.hash

  @@
  struct diff_filespec *p;
  @@
  - p->sha1
  + p->oid.hash

whereas the two pieces were reversed before.  This fixes the problem
because it's never possible for the second transform to match after the
first has transformed a given piece of code.  The functional interdiff
shows that only the issues the two of you found are changed.

> Also, for the record, the issue was noticed by Johannes originally, not
> me, as indicated above (I just found a similar case after seeing his
> report).

I apologize for the misattribution.

> > On a tangent, I wondered recently, why we need oidcpy() and oidclr(). After
> > all, in place of, e.g.,
> > 
> > oidcpy(>two->oid, >oid);
> > oidclr(>oid);
> > 
> > we can write
> > 
> > pair->two->oid = p->oid;
> > one->oid = null_oid;
> > 
> > Is there a particular reason *not* to make this transition? I find the
> > latter less cluttered with equal clarity.
> 
> I think traditionally we've avoided struct assignment because some
> ancient compilers didn't do it. But it's in C89, and I suspect it's
> crept into the code base anyway over the years without anyone
> complaining.

I asked about this earlier and Junio felt that struct assignment was
less desirable.

Also, if we do struct assignment, if we add a new hash, we may copy more
than necessary (e.g. 64 bytes when we're only using SHA-1); however,
this may be offset by the ability of the compiler to compute the offset
at compile time.
-- 
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

[PATCH v2 8/8] merge-recursive: convert merge_recursive_generic to object_id

[brian m. carlson]

Convert this function and the git merge-recursive subcommand to use
struct object_id.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 builtin/merge-recursive.c | 20 ++--
 merge-recursive.c | 14 +++---
 merge-recursive.h |  6 +++---
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 491efd55..fd2c4556 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -9,10 +9,10 @@ static const char builtin_merge_recursive_usage[] =
 
 static const char *better_branch_name(const char *branch)
 {
-   static char githead_env[8 + 40 + 1];
+   static char githead_env[8 + GIT_SHA1_HEXSZ + 1];
char *name;
 
-   if (strlen(branch) != 40)
+   if (strlen(branch) != GIT_SHA1_HEXSZ)
return branch;
xsnprintf(githead_env, sizeof(githead_env), "GITHEAD_%s", branch);
name = getenv(githead_env);
@@ -21,10 +21,10 @@ static const char *better_branch_name(const char *branch)
 
 int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 {
-   const unsigned char *bases[21];
+   const struct object_id *bases[21];
unsigned bases_count = 0;
int i, failed;
-   unsigned char h1[20], h2[20];
+   struct object_id h1, h2;
struct merge_options o;
struct commit *result;
 
@@ -46,10 +46,10 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
continue;
}
if (bases_count < ARRAY_SIZE(bases)-1) {
-   unsigned char *sha = xmalloc(20);
-   if (get_sha1(argv[i], sha))
+   struct object_id *oid = xmalloc(sizeof(struct 
object_id));
+   if (get_oid(argv[i], oid))
die("Could not parse object '%s'", argv[i]);
-   bases[bases_count++] = sha;
+   bases[bases_count++] = oid;
}
else
warning("Cannot handle more than %d bases. "
@@ -62,9 +62,9 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
o.branch1 = argv[++i];
o.branch2 = argv[++i];
 
-   if (get_sha1(o.branch1, h1))
+   if (get_oid(o.branch1, ))
die("Could not resolve ref '%s'", o.branch1);
-   if (get_sha1(o.branch2, h2))
+   if (get_oid(o.branch2, ))
die("Could not resolve ref '%s'", o.branch2);
 
o.branch1 = better_branch_name(o.branch1);
@@ -73,7 +73,7 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
if (o.verbosity >= 3)
printf("Merging %s with %s\n", o.branch1, o.branch2);
 
-   failed = merge_recursive_generic(, h1, h2, bases_count, bases, 
);
+   failed = merge_recursive_generic(, , , bases_count, bases, 
);
if (failed < 0)
return 128; /* die() error code */
return failed;
diff --git a/merge-recursive.c b/merge-recursive.c
index 7bbd4aea..48fe7e73 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1982,11 +1982,11 @@ int merge_recursive(struct merge_options *o,
return clean;
 }
 
-static struct commit *get_ref(const unsigned char *sha1, const char *name)
+static struct commit *get_ref(const struct object_id *oid, const char *name)
 {
struct object *object;
 
-   object = deref_tag(parse_object(sha1), name, strlen(name));
+   object = deref_tag(parse_object(oid->hash), name, strlen(name));
if (!object)
return NULL;
if (object->type == OBJ_TREE)
@@ -1999,10 +1999,10 @@ static struct commit *get_ref(const unsigned char 
*sha1, const char *name)
 }
 
 int merge_recursive_generic(struct merge_options *o,
-   const unsigned char *head,
-   const unsigned char *merge,
+   const struct object_id *head,
+   const struct object_id *merge,
int num_base_list,
-   const unsigned char **base_list,
+   const struct object_id **base_list,
struct commit **result)
 {
int clean;
@@ -2015,9 +2015,9 @@ int merge_recursive_generic(struct merge_options *o,
int i;
for (i = 0; i < num_base_list; ++i) {
struct commit *base;
-   if (!(base = get_ref(base_list[i], 
sha1_to_hex(base_list[i]
+   if (!(base = get_ref(base_list[i], 
oid_to_hex(base_list[i]
return error(_("Could not parse object '%s'"),
-   sha1_to_hex(base_list[i]));
+   

[PATCH v2 1/8] Add basic Coccinelle transforms.

[brian m. carlson]

Coccinelle (http://coccinelle.lip6.fr/) is a program which performs
mechanical transformations on C programs using semantic patches.  These
semantic patches can be used to implement automatic refactoring and
maintenance tasks.

Add a set of basic semantic patches to convert common patterns related
to the struct object_id transformation, as well as a README.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 contrib/examples/coccinelle/README  |  2 +
 contrib/examples/coccinelle/object_id.cocci | 83 +
 2 files changed, 85 insertions(+)
 create mode 100644 contrib/examples/coccinelle/README
 create mode 100644 contrib/examples/coccinelle/object_id.cocci

diff --git a/contrib/examples/coccinelle/README 
b/contrib/examples/coccinelle/README
new file mode 100644
index ..9c2f8879
--- /dev/null
+++ b/contrib/examples/coccinelle/README
@@ -0,0 +1,2 @@
+This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/)
+semantic patches that might be useful to developers.
diff --git a/contrib/examples/coccinelle/object_id.cocci 
b/contrib/examples/coccinelle/object_id.cocci
new file mode 100644
index ..0f068252
--- /dev/null
+++ b/contrib/examples/coccinelle/object_id.cocci
@@ -0,0 +1,83 @@
+@@
+expression E1;
+@@
+- is_null_sha1(E1.hash)
++ is_null_oid()
+
+@@
+expression E1;
+@@
+- is_null_sha1(E1->hash)
++ is_null_oid(E1)
+
+@@
+expression E1;
+@@
+- sha1_to_hex(E1.hash)
++ oid_to_hex()
+
+@@
+expression E1;
+@@
+- sha1_to_hex(E1->hash)
++ oid_to_hex(E1)
+
+@@
+expression E1;
+@@
+- hashclr(E1.hash)
++ oidclr()
+
+@@
+expression E1;
+@@
+- hashclr(E1->hash)
++ oidclr(E1)
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1.hash, E2.hash)
++ oidcmp(, )
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1->hash, E2->hash)
++ oidcmp(E1, E2)
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1->hash, E2.hash)
++ oidcmp(E1, )
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1.hash, E2->hash)
++ oidcmp(, E2)
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1.hash, E2.hash)
++ oidcpy(, )
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1->hash, E2->hash)
++ oidcpy(E1, E2)
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1->hash, E2.hash)
++ oidcpy(E1, )
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1.hash, E2->hash)
++ oidcpy(, E2)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 7/8] merge-recursive: convert leaf functions to use struct object_id

[brian m. carlson]

Convert all but two of the static functions in this file to use struct
object_id.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 merge-recursive.c | 236 +++---
 1 file changed, 118 insertions(+), 118 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index bc455491..7bbd4aea 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -56,11 +56,11 @@ static struct commit *make_virtual_commit(struct tree 
*tree, const char *comment
  * Since we use get_tree_entry(), which does not put the read object into
  * the object pool, we cannot rely on a == b.
  */
-static int sha_eq(const unsigned char *a, const unsigned char *b)
+static int oid_eq(const struct object_id *a, const struct object_id *b)
 {
if (!a && !b)
return 2;
-   return a && b && hashcmp(a, b) == 0;
+   return a && b && oidcmp(a, b) == 0;
 }
 
 enum rename_type {
@@ -198,11 +198,11 @@ static void output_commit_title(struct merge_options *o, 
struct commit *commit)
}
 }
 
-static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
+static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
const char *path, int stage, int refresh, int options)
 {
struct cache_entry *ce;
-   ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage,
+   ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage,
  (refresh ? (CE_MATCH_REFRESH |
  CE_MATCH_IGNORE_MISSING) : 0 ));
if (!ce)
@@ -552,13 +552,13 @@ static int update_stages(const char *path, const struct 
diff_filespec *o,
if (remove_file_from_cache(path))
return -1;
if (o)
-   if (add_cacheinfo(o->mode, o->oid.hash, path, 1, 0, options))
+   if (add_cacheinfo(o->mode, >oid, path, 1, 0, options))
return -1;
if (a)
-   if (add_cacheinfo(a->mode, a->oid.hash, path, 2, 0, options))
+   if (add_cacheinfo(a->mode, >oid, path, 2, 0, options))
return -1;
if (b)
-   if (add_cacheinfo(b->mode, b->oid.hash, path, 3, 0, options))
+   if (add_cacheinfo(b->mode, >oid, path, 3, 0, options))
return -1;
return 0;
 }
@@ -736,7 +736,7 @@ static int make_room_for_path(struct merge_options *o, 
const char *path)
 }
 
 static void update_file_flags(struct merge_options *o,
- const unsigned char *sha,
+ const struct object_id *oid,
  unsigned mode,
  const char *path,
  int update_cache,
@@ -760,11 +760,11 @@ static void update_file_flags(struct merge_options *o,
goto update_index;
}
 
-   buf = read_sha1_file(sha, , );
+   buf = read_sha1_file(oid->hash, , );
if (!buf)
-   die(_("cannot read object %s '%s'"), sha1_to_hex(sha), 
path);
+   die(_("cannot read object %s '%s'"), oid_to_hex(oid), 
path);
if (type != OBJ_BLOB)
-   die(_("blob expected for %s '%s'"), sha1_to_hex(sha), 
path);
+   die(_("blob expected for %s '%s'"), oid_to_hex(oid), 
path);
if (S_ISREG(mode)) {
struct strbuf strbuf = STRBUF_INIT;
if (convert_to_working_tree(path, buf, size, )) {
@@ -799,21 +799,21 @@ static void update_file_flags(struct merge_options *o,
free(lnk);
} else
die(_("do not know what to do with %06o %s '%s'"),
-   mode, sha1_to_hex(sha), path);
+   mode, oid_to_hex(oid), path);
free(buf);
}
  update_index:
if (update_cache)
-   add_cacheinfo(mode, sha, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD);
+   add_cacheinfo(mode, oid, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD);
 }
 
 static void update_file(struct merge_options *o,
int clean,
-   const unsigned char *sha,
+   const struct object_id *oid,
unsigned mode,
const char *path)
 {
-   update_file_flags(o, sha, mode, path, o->call_depth || clean, 
!o->call_depth);
+   update_file_flags(o, oid, mode, path, o->call_depth || clean, 
!o->call_depth);
 }
 
 /* Low level file merging, update and removal */
@@ -908,7 +908,7 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
   

[PATCH v2 3/8] Convert struct diff_filespec to struct object_id

[brian m. carlson]

Convert struct diff_filespec's sha1 member to use a struct object_id
called "oid" instead.  The following Coccinelle semantic patch was used
to implement this, followed by the transformations in object_id.cocci:

@@
struct diff_filespec o;
@@
- o.sha1
+ o.oid.hash

@@
struct diff_filespec *p;
@@
- p->sha1
+ p->oid.hash

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 builtin/blame.c   |   6 +--
 builtin/fast-export.c |  10 ++---
 builtin/reset.c   |   4 +-
 combine-diff.c|  10 ++---
 diff.c|  69 +---
 diffcore-break.c  |   2 +-
 diffcore-rename.c |  14 ---
 diffcore.h|   2 +-
 line-log.c|   2 +-
 merge-recursive.c | 107 +++---
 notes-merge.c |  42 ++--
 submodule.c   |   4 +-
 wt-status.c   |   3 +-
 13 files changed, 147 insertions(+), 128 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 21f42b0b..04bc4e0e 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -599,7 +599,7 @@ static struct origin *find_origin(struct scoreboard *sb,
p->status);
case 'M':
porigin = get_origin(sb, parent, origin->path);
-   hashcpy(porigin->blob_sha1, p->one->sha1);
+   hashcpy(porigin->blob_sha1, p->one->oid.hash);
porigin->mode = p->one->mode;
break;
case 'A':
@@ -645,7 +645,7 @@ static struct origin *find_rename(struct scoreboard *sb,
if ((p->status == 'R' || p->status == 'C') &&
!strcmp(p->two->path, origin->path)) {
porigin = get_origin(sb, parent, p->one->path);
-   hashcpy(porigin->blob_sha1, p->one->sha1);
+   hashcpy(porigin->blob_sha1, p->one->oid.hash);
porigin->mode = p->one->mode;
break;
}
@@ -1309,7 +1309,7 @@ static void find_copy_in_parent(struct scoreboard *sb,
continue;
 
norigin = get_origin(sb, parent, p->one->path);
-   hashcpy(norigin->blob_sha1, p->one->sha1);
+   hashcpy(norigin->blob_sha1, p->one->oid.hash);
norigin->mode = p->one->mode;
fill_origin_blob(>revs->diffopt, norigin, _p);
if (!file_p.ptr)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 8164b581..c0652a7e 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -368,7 +368,7 @@ static void show_filemodify(struct diff_queue_struct *q,
print_path(spec->path);
putchar('\n');
 
-   if (!hashcmp(ospec->sha1, spec->sha1) &&
+   if (!oidcmp(>oid, >oid) &&
ospec->mode == spec->mode)
break;
/* fallthrough */
@@ -383,10 +383,10 @@ 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->sha1) :
-  spec->sha1));
+  
anonymize_sha1(spec->oid.hash) :
+  spec->oid.hash));
else {
-   struct object *object = 
lookup_object(spec->sha1);
+   struct object *object = 
lookup_object(spec->oid.hash);
printf("M %06o :%d ", spec->mode,
   get_object_mark(object));
}
@@ -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->sha1);
+   export_blob(diff_queued_diff.queue[i]->two->oid.hash);
 
refname = commit->util;
if (anonymize) {
diff --git a/builtin/reset.c b/builtin/reset.c
index 092c3a53..cce64b53 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -121,7 +121,7 @@ static void update_index_from_diff(struct di

[PATCH v2 4/8] Rename struct diff_filespec's sha1_valid member.

[brian m. carlson]

Now that this struct's sha1 member is called "oid", update the comment
and the sha1_valid member to be called "oid_valid" instead.  The
following Coccinelle semantic patch was used to implement this, followed
by the transformations in object_id.cocci:

@@
struct diff_filespec o;
@@
- o.sha1_valid
+ o.oid_valid

@@
struct diff_filespec *p;
@@
- p->sha1_valid
+ p->oid_valid

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 combine-diff.c|  4 ++--
 diff.c| 28 ++--
 diffcore-break.c  |  2 +-
 diffcore-rename.c |  4 ++--
 diffcore.h|  2 +-
 line-log.c| 10 +-
 6 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 3537209c..5940dc87 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1269,7 +1269,7 @@ static struct diff_filepair *combined_pair(struct 
combine_diff_path *p,
pair->one[i].path = p->path;
pair->one[i].mode = p->parent[i].mode;
oidcpy(>one[i].oid, >parent[i].oid);
-   pair->one[i].sha1_valid = !is_null_oid(>parent[i].oid);
+   pair->one[i].oid_valid = !is_null_oid(>parent[i].oid);
pair->one[i].has_more_entries = 1;
}
pair->one[num_parent - 1].has_more_entries = 0;
@@ -1277,7 +1277,7 @@ static struct diff_filepair *combined_pair(struct 
combine_diff_path *p,
pair->two->path = p->path;
pair->two->mode = p->mode;
oidcpy(>two->oid, >oid);
-   pair->two->sha1_valid = !is_null_oid(>oid);
+   pair->two->oid_valid = !is_null_oid(>oid);
return pair;
 }
 
diff --git a/diff.c b/diff.c
index 5a6d8654..a7a553b8 100644
--- a/diff.c
+++ b/diff.c
@@ -1933,7 +1933,7 @@ static void show_dirstat(struct diff_options *options)
 
name = p->two->path ? p->two->path : p->one->path;
 
-   if (p->one->sha1_valid && p->two->sha1_valid)
+   if (p->one->oid_valid && p->two->oid_valid)
content_changed = oidcmp(>one->oid, >two->oid);
else
content_changed = 1;
@@ -2640,7 +2640,7 @@ void fill_filespec(struct diff_filespec *spec, const 
unsigned char *sha1,
if (mode) {
spec->mode = canon_mode(mode);
hashcpy(spec->oid.hash, sha1);
-   spec->sha1_valid = sha1_valid;
+   spec->oid_valid = sha1_valid;
}
 }
 
@@ -2766,7 +2766,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
if (S_ISGITLINK(s->mode))
return diff_populate_gitlink(s, size_only);
 
-   if (!s->sha1_valid ||
+   if (!s->oid_valid ||
reuse_worktree_file(s->path, s->oid.hash, 0)) {
struct strbuf buf = STRBUF_INIT;
struct stat st;
@@ -2915,7 +2915,7 @@ static struct diff_tempfile *prepare_temp_file(const char 
*name,
}
 
if (!S_ISGITLINK(one->mode) &&
-   (!one->sha1_valid ||
+   (!one->oid_valid ||
 reuse_worktree_file(name, one->oid.hash, 1))) {
struct stat st;
if (lstat(name, ) < 0) {
@@ -2928,16 +2928,16 @@ static struct diff_tempfile *prepare_temp_file(const 
char *name,
if (strbuf_readlink(, name, st.st_size) < 0)
die_errno("readlink(%s)", name);
prep_temp_blob(name, temp, sb.buf, sb.len,
-  (one->sha1_valid ?
+  (one->oid_valid ?
one->oid.hash : null_sha1),
-  (one->sha1_valid ?
+  (one->oid_valid ?
one->mode : S_IFLNK));
strbuf_release();
}
else {
/* we can borrow from the file in the work tree */
temp->name = name;
-   if (!one->sha1_valid)
+   if (!one->oid_valid)
sha1_to_hex_r(temp->hex, null_sha1);
else
sha1_to_hex_r(temp->hex, one->oid.hash);
@@ -3134,7 +3134,7 @@ static void run_diff_cmd(const char *pgm,
 static void diff_fill_sha1_info(struct diff_filespec *one)
 {
if (DIFF_FILE_VALID(one)) {
-   if (!one->sha1_valid) {
+   if (!one->oid_valid) {
struct stat st;
if (one->is_stdin) {
hashcpy(one->oid.hash, nul

[PATCH v2 5/8] merge-recursive: convert struct stage_data to use object_id

[brian m. carlson]

Convert the anonymous struct within struct stage_data to use struct
object_id.  The following Coccinelle semantic patch was used to
implement this, followed by the transformations in object_id.cocci:

@@
struct stage_data o;
expression E1;
@@
- o.stages[E1].sha
+ o.stages[E1].oid.hash

@@
struct stage_data *p;
expression E1;
@@
- p->stages[E1].sha
+ p->stages[E1].oid.hash

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 merge-recursive.c | 38 ++
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1e802097..a07050cd 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -90,7 +90,7 @@ struct rename_conflict_info {
 struct stage_data {
struct {
unsigned mode;
-   unsigned char sha[20];
+   struct object_id oid;
} stages[4];
struct rename_conflict_info *rename_conflict_info;
unsigned processed:1;
@@ -134,13 +134,11 @@ static inline void setup_rename_conflict_info(enum 
rename_type rename_type,
int ostage2 = ostage1 ^ 1;
 
ci->ren1_other.path = pair1->one->path;
-   hashcpy(ci->ren1_other.oid.hash,
-   src_entry1->stages[ostage1].sha);
+   oidcpy(>ren1_other.oid, _entry1->stages[ostage1].oid);
ci->ren1_other.mode = src_entry1->stages[ostage1].mode;
 
ci->ren2_other.path = pair2->one->path;
-   hashcpy(ci->ren2_other.oid.hash,
-   src_entry2->stages[ostage2].sha);
+   oidcpy(>ren2_other.oid, _entry2->stages[ostage2].oid);
ci->ren2_other.mode = src_entry2->stages[ostage2].mode;
}
 }
@@ -316,11 +314,11 @@ static struct stage_data *insert_stage_data(const char 
*path,
struct string_list_item *item;
struct stage_data *e = xcalloc(1, sizeof(struct stage_data));
get_tree_entry(o->object.oid.hash, path,
-   e->stages[1].sha, >stages[1].mode);
+   e->stages[1].oid.hash, >stages[1].mode);
get_tree_entry(a->object.oid.hash, path,
-   e->stages[2].sha, >stages[2].mode);
+   e->stages[2].oid.hash, >stages[2].mode);
get_tree_entry(b->object.oid.hash, path,
-   e->stages[3].sha, >stages[3].mode);
+   e->stages[3].oid.hash, >stages[3].mode);
item = string_list_insert(entries, path);
item->util = e;
return e;
@@ -351,7 +349,7 @@ static struct string_list *get_unmerged(void)
}
e = item->util;
e->stages[ce_stage(ce)].mode = ce->ce_mode;
-   hashcpy(e->stages[ce_stage(ce)].sha, ce->sha1);
+   hashcpy(e->stages[ce_stage(ce)].oid.hash, ce->sha1);
}
 
return unmerged;
@@ -574,9 +572,9 @@ static void update_entry(struct stage_data *entry,
entry->stages[1].mode = o->mode;
entry->stages[2].mode = a->mode;
entry->stages[3].mode = b->mode;
-   hashcpy(entry->stages[1].sha, o->oid.hash);
-   hashcpy(entry->stages[2].sha, a->oid.hash);
-   hashcpy(entry->stages[3].sha, b->oid.hash);
+   oidcpy(>stages[1].oid, >oid);
+   oidcpy(>stages[2].oid, >oid);
+   oidcpy(>stages[3].oid, >oid);
 }
 
 static int remove_file(struct merge_options *o, int clean,
@@ -,7 +1109,7 @@ static struct diff_filespec *filespec_from_entry(struct 
diff_filespec *target,
 struct stage_data *entry,
 int stage)
 {
-   unsigned char *sha = entry->stages[stage].sha;
+   unsigned char *sha = entry->stages[stage].oid.hash;
unsigned mode = entry->stages[stage].mode;
if (mode == 0 || is_null_sha1(sha))
return NULL;
@@ -1425,11 +1423,11 @@ static int process_renames(struct merge_options *o,
remove_file(o, 1, ren1_src,
renamed_stage == 2 || 
!was_tracked(ren1_src));
 
-   hashcpy(src_other.oid.hash,
-   ren1->src_entry->stages[other_stage].sha);
+   oidcpy(_other.oid,
+  >src_entry->stages[other_stage].oid);
src_other.mode = 
ren1->src_entry->stages[other_stage].mode;
-   hashcpy(dst_other.oid.hash,
-   ren1->dst_entry->stages[other_stage].sha);
+   oidcpy(_other.oid,
+  >dst_entry->stages[other_stage].oid);
dst_other.mode = 
ren1->dst_entry->s

[PATCH v2 0/8] object_id part 4

[brian m. carlson]

This series is part 4 in a series of conversions to replace instances of
unsigned char [20] with struct object_id.  Most of this series touches
the merge-recursive code.

New in this series is the use of Coccinelle (http://coccinelle.lip6.fr/)
semantic patches.  These semantic patches can make automatic
transformations to C source code for cleanup or refactoring reasons.

This series introduces a set of transforms for the struct object_id
transition, cleans up some existing code with them, and applies a small
number of semantic patches to transform parts of the merge-recursive
code.  Some manual refactoring work follows.

Note that in the patches created with the semantic patches, the only manual
change was the definition of the struct member.  Opinions on whether this is a
viable technique for further work to ease both the creation and review of
patches are of course welcomed.

The testsuite continues to pass at each step, and this series rebases
cleanly on both pu and next.

I moved the Coccinelle transforms to contrib/examples/coccinelle, but if
it's decided that the Coccinelle transforms simply don't belong in the
repository, it's fine to simply drop the first patch (and maybe fix up
the commit messages).  I can create a GitHub Gist and let reviewers
refer to that at their convenience.

Changes from v1:
* Move the object ID transformations to contrib/examples/coccinelle.
* Add a README to that folder explaining what they are.
* Adjust the Coccinelle patches to transform plain structs before
  pointers to structs to avoid misconversions.  This addresses the issue
  that Peff caught originally.

brian m. carlson (8):
  Add basic Coccinelle transforms.
  Apply object_id Coccinelle transformations.
  Convert struct diff_filespec to struct object_id
  Rename struct diff_filespec's sha1_valid member.
  merge-recursive: convert struct stage_data to use object_id
  merge-recursive: convert struct merge_file_info to object_id
  merge-recursive: convert leaf functions to use struct object_id
  merge-recursive: convert merge_recursive_generic to object_id

 bisect.c|   2 +-
 builtin/blame.c |   6 +-
 builtin/fast-export.c   |  10 +-
 builtin/merge-recursive.c   |  20 +-
 builtin/merge.c |  13 +-
 builtin/reset.c |   4 +-
 combine-diff.c  |  14 +-
 contrib/examples/coccinelle/README  |   2 +
 contrib/examples/coccinelle/object_id.cocci |  83 
 diff.c  |  95 +
 diffcore-break.c|   4 +-
 diffcore-rename.c   |  16 +-
 diffcore.h  |   4 +-
 line-log.c  |  12 +-
 merge-recursive.c   | 310 ++--
 merge-recursive.h   |   6 +-
 notes-merge.c   |  42 ++--
 refs/files-backend.c|   4 +-
 submodule.c |   4 +-
 wt-status.c |   3 +-
 20 files changed, 378 insertions(+), 276 deletions(-)
 create mode 100644 contrib/examples/coccinelle/README
 create mode 100644 contrib/examples/coccinelle/object_id.cocci
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 6/8] merge-recursive: convert struct merge_file_info to object_id

[brian m. carlson]

Convert struct merge_file_info to use struct object_id.  The following
Coccinelle semantic patch was used to implement this, followed by the
transformations in object_id.cocci:

@@
struct merge_file_info o;
@@
- o.sha
+ o.oid.hash

@@
struct merge_file_info *p;
@@
- p->sha
+ p->oid.hash

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 merge-recursive.c | 39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index a07050cd..bc455491 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -819,7 +819,7 @@ static void update_file(struct merge_options *o,
 /* Low level file merging, update and removal */
 
 struct merge_file_info {
-   unsigned char sha[20];
+   struct object_id oid;
unsigned mode;
unsigned clean:1,
 merge:1;
@@ -902,10 +902,10 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
result.clean = 0;
if (S_ISREG(a->mode)) {
result.mode = a->mode;
-   hashcpy(result.sha, a->oid.hash);
+   oidcpy(, >oid);
} else {
result.mode = b->mode;
-   hashcpy(result.sha, b->oid.hash);
+   oidcpy(, >oid);
}
} else {
if (!sha_eq(a->oid.hash, one->oid.hash) && !sha_eq(b->oid.hash, 
one->oid.hash))
@@ -925,9 +925,9 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
}
 
if (sha_eq(a->oid.hash, b->oid.hash) || sha_eq(a->oid.hash, 
one->oid.hash))
-   hashcpy(result.sha, b->oid.hash);
+   oidcpy(, >oid);
else if (sha_eq(b->oid.hash, one->oid.hash))
-   hashcpy(result.sha, a->oid.hash);
+   oidcpy(, >oid);
else if (S_ISREG(a->mode)) {
mmbuffer_t result_buf;
int merge_status;
@@ -939,21 +939,21 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
die(_("Failed to execute internal merge"));
 
if (write_sha1_file(result_buf.ptr, result_buf.size,
-   blob_type, result.sha))
+   blob_type, result.oid.hash))
die(_("Unable to add %s to database"),
a->path);
 
free(result_buf.ptr);
result.clean = (merge_status == 0);
} else if (S_ISGITLINK(a->mode)) {
-   result.clean = merge_submodule(result.sha,
+   result.clean = merge_submodule(result.oid.hash,
   one->path,
   one->oid.hash,
   a->oid.hash,
   b->oid.hash,
   !o->call_depth);
} else if (S_ISLNK(a->mode)) {
-   hashcpy(result.sha, a->oid.hash);
+   oidcpy(, >oid);
 
if (!sha_eq(a->oid.hash, b->oid.hash))
result.clean = 0;
@@ -1192,7 +1192,7 @@ static void conflict_rename_rename_1to2(struct 
merge_options *o,
 * pathname and then either rename the add-source file to that
 * unique path, or use that unique path instead of src here.
 */
-   update_file(o, 0, mfi.sha, mfi.mode, one->path);
+   update_file(o, 0, mfi.oid.hash, mfi.mode, one->path);
 
/*
 * Above, we put the merged content at the merge-base's
@@ -1255,16 +1255,16 @@ static void conflict_rename_rename_2to1(struct 
merge_options *o,
 * again later for the non-recursive merge.
 */
remove_file(o, 0, path, 0);
-   update_file(o, 0, mfi_c1.sha, mfi_c1.mode, a->path);
-   update_file(o, 0, mfi_c2.sha, mfi_c2.mode, b->path);
+   update_file(o, 0, mfi_c1.oid.hash, mfi_c1.mode, a->path);
+   update_file(o, 0, mfi_c2.oid.hash, mfi_c2.mode, b->path);
} else {
char *new_path1 = unique_path(o, path, ci->branch1);
char *new_path2 = unique_path(o, path, ci->branch2);
output(o, 1, _("Renaming %s to %s and %s to %s instead"),
   a->path, new_path1, b->path, new_path2);
remove_file(o, 0,

[PATCH v2 2/8] Apply object_id Coccinelle transformations.

[brian m. carlson]

Apply the set of semantic patches from contrib/examples/coccinelle to
convert some leftover places using struct object_id's hash member to
instead use the wrapper functions that take struct object_id natively.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 bisect.c |  2 +-
 builtin/merge.c  | 13 ++---
 refs/files-backend.c |  4 ++--
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/bisect.c b/bisect.c
index 6d93edbc..ff147589 100644
--- a/bisect.c
+++ b/bisect.c
@@ -754,7 +754,7 @@ static void handle_bad_merge_base(void)
 static void handle_skipped_merge_base(const unsigned char *mb)
 {
char *mb_hex = sha1_to_hex(mb);
-   char *bad_hex = sha1_to_hex(current_bad_oid->hash);
+   char *bad_hex = oid_to_hex(current_bad_oid);
char *good_hex = join_sha1_array_hex(_revs, ' ');
 
warning("the merge base between %s and [%s] "
diff --git a/builtin/merge.c b/builtin/merge.c
index b555a1bf..1e7be852 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -501,7 +501,7 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
if (ref_exists(truname.buf)) {
strbuf_addf(msg,
"%s\t\tbranch '%s'%s of .\n",
-   sha1_to_hex(remote_head->object.oid.hash),
+   oid_to_hex(_head->object.oid),
truname.buf + 11,
(early ? " (early part)" : ""));
strbuf_release();
@@ -515,7 +515,7 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
desc = merge_remote_util(remote_head);
if (desc && desc->obj && desc->obj->type == OBJ_TAG) {
strbuf_addf(msg, "%s\t\t%s '%s'\n",
-   sha1_to_hex(desc->obj->oid.hash),
+   oid_to_hex(>obj->oid),
typename(desc->obj->type),
remote);
goto cleanup;
@@ -523,7 +523,7 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
}
 
strbuf_addf(msg, "%s\t\tcommit '%s'\n",
-   sha1_to_hex(remote_head->object.oid.hash), remote);
+   oid_to_hex(_head->object.oid), remote);
 cleanup:
strbuf_release();
strbuf_release();
@@ -1366,7 +1366,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
for (p = remoteheads; p; p = p->next) {
struct commit *commit = p->item;
strbuf_addf(, "GITHEAD_%s",
-   sha1_to_hex(commit->object.oid.hash));
+   oid_to_hex(>object.oid));
setenv(buf.buf, merge_remote_util(commit)->name, 1);
strbuf_reset();
if (fast_forward != FF_ONLY &&
@@ -1425,7 +1425,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
goto done;
} else if (fast_forward != FF_NO && !remoteheads->next &&
!common->next &&
-   !hashcmp(common->item->object.oid.hash, 
head_commit->object.oid.hash)) {
+   !oidcmp(>item->object.oid, 
_commit->object.oid)) {
/* Again the most common case of merging one remote. */
struct strbuf msg = STRBUF_INIT;
struct commit *commit;
@@ -1499,8 +1499,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
 * HEAD^^" would be missed.
 */
common_one = get_merge_bases(head_commit, j->item);
-   if (hashcmp(common_one->item->object.oid.hash,
-   j->item->object.oid.hash)) {
+   if (oidcmp(_one->item->object.oid, 
>item->object.oid)) {
up_to_date = 0;
break;
}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1f380764..dac3a222 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1725,14 +1725,14 @@ static int verify_lock(struct ref_lock *lock,
errno = save_errno;
return -1;
} else {
-   hashclr(lock->old_oid.hash);
+   oidclr(>old_oid);
return 0;
}
}
if (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) {
strbuf_addf(err, "ref %s is at %s but expected %s",
lock->re

Re: [PATCH v2 3/8] Convert struct diff_filespec to struct object_id

[brian m. carlson]

On Tue, Jun 21, 2016 at 03:22:04PM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sand...@crustytoothpaste.net> writes:
> 
> I was trying to make sure there is no misconversion, but some lines
> that got wrapped were distracting.  For example:
> 
> > @@ -2721,7 +2722,8 @@ static int diff_populate_gitlink(struct diff_filespec 
> > *s, int size_only)
> > if (s->dirty_submodule)
> > dirty = "-dirty";
> >  
> > -   strbuf_addf(, "Subproject commit %s%s\n", sha1_to_hex(s->sha1), 
> > dirty);
> > +   strbuf_addf(, "Subproject commit %s%s\n",
> > +   oid_to_hex(>oid), dirty);
> 
> This would have been
> 
> > -   strbuf_addf(, "Subproject commit %s%s\n", sha1_to_hex(s->sha1), 
> > dirty);
> > +   strbuf_addf(, "Subproject commit %s%s\n", oid_to_hex(>oid), 
> > dirty);
> 
> which the conversion made the line _shorter_.  If the original's
> line length was acceptable, there is no reason to wrap the result.

I do tend to agree.  Coccinelle wrapped the line automatically, but I'm
not sure why it did that.  I can see if there's an option that disables
that if you'd like.  I'm a bit reticent to manually fix up the line
wrapping as I want others to be able to run Coccinelle themselves and
get identical output.

> > @@ -2937,7 +2940,7 @@ static struct diff_tempfile *prepare_temp_file(const 
> > char *name,
> > if (!one->sha1_valid)
> > sha1_to_hex_r(temp->hex, null_sha1);
> > else
> > -   sha1_to_hex_r(temp->hex, one->sha1);
> > +   sha1_to_hex_r(temp->hex, one->oid.hash);
> 
> This suggests that oid_to_hex_r() is needed, perhaps?

Probably so.  I can add that in a reroll.

> > @@ -2952,7 +2955,7 @@ static struct diff_tempfile *prepare_temp_file(const 
> > char *name,
> > if (diff_populate_filespec(one, 0))
> > die("cannot read data blob for %s", one->path);
> > prep_temp_blob(name, temp, one->data, one->size,
> > -  one->sha1, one->mode);
> > +  one->oid.hash, one->mode);
> 
> prep_temp_blob() is a file-scope static with only two callers.  It
> probably would not take too much effort to make it take >oid
> instead?

I can certainly do that in a reroll.

> > @@ -3075,8 +3078,8 @@ static void fill_metainfo(struct strbuf *msg,
> > abbrev = 40;
> > }
> > strbuf_addf(msg, "%s%sindex %s..", line_prefix, set,
> > -   find_unique_abbrev(one->sha1, abbrev));
> > -   strbuf_addstr(msg, find_unique_abbrev(two->sha1, abbrev));
> > +   find_unique_abbrev(one->oid.hash, abbrev));
> > +   strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));
> 
> Good.  As more and more callers of find_unique_abbrev() is converted
> to pass oid.hash to it, eventually we will have only a handful of
> callers that have a raw "const unsigned char sha1[20]" to pass it,
> and we can eventually make the function take   It seems we are
> not quite there yet, by the looks of 'git grep find_unique_abbrev'
> output, but we are getting closer.

Yes, I'd noticed that as well.  One thing I observed from these
conversions is that it sometimes takes a huge amount of work to get all
the callers to change.  Often, it's only one or two call sites that end
up being a bunch of work.

> > @@ -3134,17 +3137,17 @@ static void diff_fill_sha1_info(struct 
> > diff_filespec *one)
> > if (!one->sha1_valid) {
> > struct stat st;
> > if (one->is_stdin) {
> > -   hashcpy(one->sha1, null_sha1);
> > +   hashcpy(one->oid.hash, null_sha1);
> > return;
> > }
> 
> oidclr()?
> 
> Perhaps a preparatory step of
> 
>   unsigned char *E1;
> 
>   -hashcpy(E1, null_sha1);
> +hashclr(E1)
> 
> would help?

Sure.  I can do that.

> > @@ -902,13 +904,13 @@ static struct merge_file_info merge_file_1(struct 
> > merge_options *o,
> > result.clean = 0;
> > if (S_ISREG(a->mode)) {
> > result.mode = a->mode;
> > -   hashcpy(result.sha, a->sha1);
> > +   hashcpy(result.sha, a->oid.hash);
> > } else {

Re: Migrating away from SHA-1?

[brian m. carlson]

On Sat, Jun 18, 2016 at 03:10:27AM +0100, Leo Gaspard wrote:
> First, sorry for not having this message threaded: I'm not subscribed to
> the list and haven't found a way to get a Message-Id from gmane.

Sorry it's taken so long to get back to this.  I've been at a
conference.

> So, my questions to the git team:
>  * Is there a consensus, that git should migrate away from SHA-1 before
> it gets a collision attack, because it would mean chosen-prefix
> collision isn't far away and people wouldn't have the time to upgrade?

I plan on adding support for a new hash as soon as that's possible, but
I don't have a firm timeline.  This is a volunteer effort in my own
limited free time.

>  * Is there a consensus, that Peter Anvin's amended transition plan is
> the way to go?

I'm not planning on changing algorithms in the middle of a repository.
This will only be available on new or imported repositories.

My current thinking on proposed algorithms is SHA3-256 or BLAKE2b-256.
The cryptanalysis on SHA-256 indicates that it may not be a great
long-term choice, and I expect people won't want to change algorithms
frequently.

If time becomes extremely urgent, we can always add support for a
160-bit hash first (e.g. BLAKE2b-160) and then finish the object_id
transition later as it becomes convenient.  I'd like to avoid that,
though.

>  * If the two conditions above are fulfilled, has work started on it
> yet? (I guess as Brian Carlson had started his work 9 weeks ago and he
> was speaking about working on it on the week-end he should have finished
> it now, so excluding this)

It takes a long time to get a patch series through.  I'm rather busy and
don't always have time to rebase and address issues during the week.

>  * If the two first conditions are fulfilled, is there anything I could
> do to help this transition? (including helping Brian if his work hasn't
> actually ended yet)

You're welcome to send patches if you like.  I try to avoid areas I know
are under heavy development, like the refs code.
-- 
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

[PATCH v3 11/11] diff: convert prep_temp_blob to struct object_id.

[brian m. carlson]

All of the callers of this function use struct object_id, so convert it
to use struct object_id in its arguments and internally.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 diff.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index 9abb54ad..8cdfdf32 100644
--- a/diff.c
+++ b/diff.c
@@ -2866,7 +2866,7 @@ void diff_free_filespec_data(struct diff_filespec *s)
 static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
   void *blob,
   unsigned long size,
-  const unsigned char *sha1,
+  const struct object_id *oid,
   int mode)
 {
int fd;
@@ -2891,7 +2891,7 @@ static void prep_temp_blob(const char *path, struct 
diff_tempfile *temp,
die_errno("unable to write temp-file");
close_tempfile(>tempfile);
temp->name = get_tempfile_path(>tempfile);
-   sha1_to_hex_r(temp->hex, sha1);
+   oid_to_hex_r(temp->hex, oid);
xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode);
strbuf_release();
strbuf_release();
@@ -2929,7 +2929,7 @@ static struct diff_tempfile *prepare_temp_file(const char 
*name,
die_errno("readlink(%s)", name);
prep_temp_blob(name, temp, sb.buf, sb.len,
   (one->oid_valid ?
-   one->oid.hash : null_sha1),
+   >oid : _oid),
   (one->oid_valid ?
one->mode : S_IFLNK));
strbuf_release();
@@ -2955,7 +2955,7 @@ static struct diff_tempfile *prepare_temp_file(const char 
*name,
if (diff_populate_filespec(one, 0))
die("cannot read data blob for %s", one->path);
prep_temp_blob(name, temp, one->data, one->size,
-  one->oid.hash, one->mode);
+  >oid, one->mode);
}
return temp;
 }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 09/11] merge-recursive: convert leaf functions to use struct object_id

[brian m. carlson]

Convert all but two of the static functions in this file to use struct
object_id.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 merge-recursive.c | 236 +++---
 1 file changed, 118 insertions(+), 118 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index bc455491..7bbd4aea 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -56,11 +56,11 @@ static struct commit *make_virtual_commit(struct tree 
*tree, const char *comment
  * Since we use get_tree_entry(), which does not put the read object into
  * the object pool, we cannot rely on a == b.
  */
-static int sha_eq(const unsigned char *a, const unsigned char *b)
+static int oid_eq(const struct object_id *a, const struct object_id *b)
 {
if (!a && !b)
return 2;
-   return a && b && hashcmp(a, b) == 0;
+   return a && b && oidcmp(a, b) == 0;
 }
 
 enum rename_type {
@@ -198,11 +198,11 @@ static void output_commit_title(struct merge_options *o, 
struct commit *commit)
}
 }
 
-static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
+static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
const char *path, int stage, int refresh, int options)
 {
struct cache_entry *ce;
-   ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage,
+   ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage,
  (refresh ? (CE_MATCH_REFRESH |
  CE_MATCH_IGNORE_MISSING) : 0 ));
if (!ce)
@@ -552,13 +552,13 @@ static int update_stages(const char *path, const struct 
diff_filespec *o,
if (remove_file_from_cache(path))
return -1;
if (o)
-   if (add_cacheinfo(o->mode, o->oid.hash, path, 1, 0, options))
+   if (add_cacheinfo(o->mode, >oid, path, 1, 0, options))
return -1;
if (a)
-   if (add_cacheinfo(a->mode, a->oid.hash, path, 2, 0, options))
+   if (add_cacheinfo(a->mode, >oid, path, 2, 0, options))
return -1;
if (b)
-   if (add_cacheinfo(b->mode, b->oid.hash, path, 3, 0, options))
+   if (add_cacheinfo(b->mode, >oid, path, 3, 0, options))
return -1;
return 0;
 }
@@ -736,7 +736,7 @@ static int make_room_for_path(struct merge_options *o, 
const char *path)
 }
 
 static void update_file_flags(struct merge_options *o,
- const unsigned char *sha,
+ const struct object_id *oid,
  unsigned mode,
  const char *path,
  int update_cache,
@@ -760,11 +760,11 @@ static void update_file_flags(struct merge_options *o,
goto update_index;
}
 
-   buf = read_sha1_file(sha, , );
+   buf = read_sha1_file(oid->hash, , );
if (!buf)
-   die(_("cannot read object %s '%s'"), sha1_to_hex(sha), 
path);
+   die(_("cannot read object %s '%s'"), oid_to_hex(oid), 
path);
if (type != OBJ_BLOB)
-   die(_("blob expected for %s '%s'"), sha1_to_hex(sha), 
path);
+   die(_("blob expected for %s '%s'"), oid_to_hex(oid), 
path);
if (S_ISREG(mode)) {
struct strbuf strbuf = STRBUF_INIT;
if (convert_to_working_tree(path, buf, size, )) {
@@ -799,21 +799,21 @@ static void update_file_flags(struct merge_options *o,
free(lnk);
} else
die(_("do not know what to do with %06o %s '%s'"),
-   mode, sha1_to_hex(sha), path);
+   mode, oid_to_hex(oid), path);
free(buf);
}
  update_index:
if (update_cache)
-   add_cacheinfo(mode, sha, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD);
+   add_cacheinfo(mode, oid, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD);
 }
 
 static void update_file(struct merge_options *o,
int clean,
-   const unsigned char *sha,
+   const struct object_id *oid,
unsigned mode,
const char *path)
 {
-   update_file_flags(o, sha, mode, path, o->call_depth || clean, 
!o->call_depth);
+   update_file_flags(o, oid, mode, path, o->call_depth || clean, 
!o->call_depth);
 }
 
 /* Low level file merging, update and removal */
@@ -908,7 +908,7 @@ static struct merge_file_info me

[PATCH v3 07/11] merge-recursive: convert struct stage_data to use object_id

[brian m. carlson]

Convert the anonymous struct within struct stage_data to use struct
object_id.  The following Coccinelle semantic patch was used to
implement this, followed by the transformations in object_id.cocci:

@@
struct stage_data o;
expression E1;
@@
- o.stages[E1].sha
+ o.stages[E1].oid.hash

@@
struct stage_data *p;
expression E1;
@@
- p->stages[E1].sha
+ p->stages[E1].oid.hash

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 merge-recursive.c | 38 ++
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1e802097..a07050cd 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -90,7 +90,7 @@ struct rename_conflict_info {
 struct stage_data {
struct {
unsigned mode;
-   unsigned char sha[20];
+   struct object_id oid;
} stages[4];
struct rename_conflict_info *rename_conflict_info;
unsigned processed:1;
@@ -134,13 +134,11 @@ static inline void setup_rename_conflict_info(enum 
rename_type rename_type,
int ostage2 = ostage1 ^ 1;
 
ci->ren1_other.path = pair1->one->path;
-   hashcpy(ci->ren1_other.oid.hash,
-   src_entry1->stages[ostage1].sha);
+   oidcpy(>ren1_other.oid, _entry1->stages[ostage1].oid);
ci->ren1_other.mode = src_entry1->stages[ostage1].mode;
 
ci->ren2_other.path = pair2->one->path;
-   hashcpy(ci->ren2_other.oid.hash,
-   src_entry2->stages[ostage2].sha);
+   oidcpy(>ren2_other.oid, _entry2->stages[ostage2].oid);
ci->ren2_other.mode = src_entry2->stages[ostage2].mode;
}
 }
@@ -316,11 +314,11 @@ static struct stage_data *insert_stage_data(const char 
*path,
struct string_list_item *item;
struct stage_data *e = xcalloc(1, sizeof(struct stage_data));
get_tree_entry(o->object.oid.hash, path,
-   e->stages[1].sha, >stages[1].mode);
+   e->stages[1].oid.hash, >stages[1].mode);
get_tree_entry(a->object.oid.hash, path,
-   e->stages[2].sha, >stages[2].mode);
+   e->stages[2].oid.hash, >stages[2].mode);
get_tree_entry(b->object.oid.hash, path,
-   e->stages[3].sha, >stages[3].mode);
+   e->stages[3].oid.hash, >stages[3].mode);
item = string_list_insert(entries, path);
item->util = e;
return e;
@@ -351,7 +349,7 @@ static struct string_list *get_unmerged(void)
}
e = item->util;
e->stages[ce_stage(ce)].mode = ce->ce_mode;
-   hashcpy(e->stages[ce_stage(ce)].sha, ce->sha1);
+   hashcpy(e->stages[ce_stage(ce)].oid.hash, ce->sha1);
}
 
return unmerged;
@@ -574,9 +572,9 @@ static void update_entry(struct stage_data *entry,
entry->stages[1].mode = o->mode;
entry->stages[2].mode = a->mode;
entry->stages[3].mode = b->mode;
-   hashcpy(entry->stages[1].sha, o->oid.hash);
-   hashcpy(entry->stages[2].sha, a->oid.hash);
-   hashcpy(entry->stages[3].sha, b->oid.hash);
+   oidcpy(>stages[1].oid, >oid);
+   oidcpy(>stages[2].oid, >oid);
+   oidcpy(>stages[3].oid, >oid);
 }
 
 static int remove_file(struct merge_options *o, int clean,
@@ -,7 +1109,7 @@ static struct diff_filespec *filespec_from_entry(struct 
diff_filespec *target,
 struct stage_data *entry,
 int stage)
 {
-   unsigned char *sha = entry->stages[stage].sha;
+   unsigned char *sha = entry->stages[stage].oid.hash;
unsigned mode = entry->stages[stage].mode;
if (mode == 0 || is_null_sha1(sha))
return NULL;
@@ -1425,11 +1423,11 @@ static int process_renames(struct merge_options *o,
remove_file(o, 1, ren1_src,
renamed_stage == 2 || 
!was_tracked(ren1_src));
 
-   hashcpy(src_other.oid.hash,
-   ren1->src_entry->stages[other_stage].sha);
+   oidcpy(_other.oid,
+  >src_entry->stages[other_stage].oid);
src_other.mode = 
ren1->src_entry->stages[other_stage].mode;
-   hashcpy(dst_other.oid.hash,
-   ren1->dst_entry->stages[other_stage].sha);
+   oidcpy(_other.oid,
+  >dst_entry->stages[other_stage].oid);
   

[PATCH v3 05/11] diff: convert struct diff_filespec to struct object_id

[brian m. carlson]

Convert struct diff_filespec's sha1 member to use a struct object_id
called "oid" instead.  The following Coccinelle semantic patch was used
to implement this, followed by the transformations in object_id.cocci:

@@
struct diff_filespec o;
@@
- o.sha1
+ o.oid.hash

@@
struct diff_filespec *p;
@@
- p->sha1
+ p->oid.hash

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 builtin/blame.c   |   6 +--
 builtin/fast-export.c |  10 ++---
 builtin/reset.c   |   4 +-
 combine-diff.c|  10 ++---
 diff.c|  69 +---
 diffcore-break.c  |   2 +-
 diffcore-rename.c |  14 ---
 diffcore.h|   2 +-
 line-log.c|   2 +-
 merge-recursive.c | 107 +++---
 notes-merge.c |  42 ++--
 submodule.c   |   4 +-
 wt-status.c   |   3 +-
 13 files changed, 147 insertions(+), 128 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 759d84af..c3459f54 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -599,7 +599,7 @@ static struct origin *find_origin(struct scoreboard *sb,
p->status);
case 'M':
porigin = get_origin(sb, parent, origin->path);
-   hashcpy(porigin->blob_sha1, p->one->sha1);
+   hashcpy(porigin->blob_sha1, p->one->oid.hash);
porigin->mode = p->one->mode;
break;
case 'A':
@@ -645,7 +645,7 @@ static struct origin *find_rename(struct scoreboard *sb,
if ((p->status == 'R' || p->status == 'C') &&
!strcmp(p->two->path, origin->path)) {
porigin = get_origin(sb, parent, p->one->path);
-   hashcpy(porigin->blob_sha1, p->one->sha1);
+   hashcpy(porigin->blob_sha1, p->one->oid.hash);
porigin->mode = p->one->mode;
break;
}
@@ -1309,7 +1309,7 @@ static void find_copy_in_parent(struct scoreboard *sb,
continue;
 
norigin = get_origin(sb, parent, p->one->path);
-   hashcpy(norigin->blob_sha1, p->one->sha1);
+   hashcpy(norigin->blob_sha1, p->one->oid.hash);
norigin->mode = p->one->mode;
fill_origin_blob(>revs->diffopt, norigin, _p);
if (!file_p.ptr)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 8164b581..c0652a7e 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -368,7 +368,7 @@ static void show_filemodify(struct diff_queue_struct *q,
print_path(spec->path);
putchar('\n');
 
-   if (!hashcmp(ospec->sha1, spec->sha1) &&
+   if (!oidcmp(>oid, >oid) &&
ospec->mode == spec->mode)
break;
/* fallthrough */
@@ -383,10 +383,10 @@ 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->sha1) :
-  spec->sha1));
+  
anonymize_sha1(spec->oid.hash) :
+  spec->oid.hash));
else {
-   struct object *object = 
lookup_object(spec->sha1);
+   struct object *object = 
lookup_object(spec->oid.hash);
printf("M %06o :%d ", spec->mode,
   get_object_mark(object));
}
@@ -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->sha1);
+   export_blob(diff_queued_diff.queue[i]->two->oid.hash);
 
refname = commit->util;
if (anonymize) {
diff --git a/builtin/reset.c b/builtin/reset.c
index acd62788..ab1fe575 100644
--- a/builtin/reset.c
+++ b/builtin/re

[PATCH v3 10/11] merge-recursive: convert merge_recursive_generic to object_id

[brian m. carlson]

Convert this function and the git merge-recursive subcommand to use
struct object_id.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 builtin/merge-recursive.c | 20 ++--
 merge-recursive.c | 14 +++---
 merge-recursive.h |  6 +++---
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 491efd55..fd2c4556 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -9,10 +9,10 @@ static const char builtin_merge_recursive_usage[] =
 
 static const char *better_branch_name(const char *branch)
 {
-   static char githead_env[8 + 40 + 1];
+   static char githead_env[8 + GIT_SHA1_HEXSZ + 1];
char *name;
 
-   if (strlen(branch) != 40)
+   if (strlen(branch) != GIT_SHA1_HEXSZ)
return branch;
xsnprintf(githead_env, sizeof(githead_env), "GITHEAD_%s", branch);
name = getenv(githead_env);
@@ -21,10 +21,10 @@ static const char *better_branch_name(const char *branch)
 
 int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 {
-   const unsigned char *bases[21];
+   const struct object_id *bases[21];
unsigned bases_count = 0;
int i, failed;
-   unsigned char h1[20], h2[20];
+   struct object_id h1, h2;
struct merge_options o;
struct commit *result;
 
@@ -46,10 +46,10 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
continue;
}
if (bases_count < ARRAY_SIZE(bases)-1) {
-   unsigned char *sha = xmalloc(20);
-   if (get_sha1(argv[i], sha))
+   struct object_id *oid = xmalloc(sizeof(struct 
object_id));
+   if (get_oid(argv[i], oid))
die("Could not parse object '%s'", argv[i]);
-   bases[bases_count++] = sha;
+   bases[bases_count++] = oid;
}
else
warning("Cannot handle more than %d bases. "
@@ -62,9 +62,9 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
o.branch1 = argv[++i];
o.branch2 = argv[++i];
 
-   if (get_sha1(o.branch1, h1))
+   if (get_oid(o.branch1, ))
die("Could not resolve ref '%s'", o.branch1);
-   if (get_sha1(o.branch2, h2))
+   if (get_oid(o.branch2, ))
die("Could not resolve ref '%s'", o.branch2);
 
o.branch1 = better_branch_name(o.branch1);
@@ -73,7 +73,7 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
if (o.verbosity >= 3)
printf("Merging %s with %s\n", o.branch1, o.branch2);
 
-   failed = merge_recursive_generic(, h1, h2, bases_count, bases, 
);
+   failed = merge_recursive_generic(, , , bases_count, bases, 
);
if (failed < 0)
return 128; /* die() error code */
return failed;
diff --git a/merge-recursive.c b/merge-recursive.c
index 7bbd4aea..48fe7e73 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1982,11 +1982,11 @@ int merge_recursive(struct merge_options *o,
return clean;
 }
 
-static struct commit *get_ref(const unsigned char *sha1, const char *name)
+static struct commit *get_ref(const struct object_id *oid, const char *name)
 {
struct object *object;
 
-   object = deref_tag(parse_object(sha1), name, strlen(name));
+   object = deref_tag(parse_object(oid->hash), name, strlen(name));
if (!object)
return NULL;
if (object->type == OBJ_TREE)
@@ -1999,10 +1999,10 @@ static struct commit *get_ref(const unsigned char 
*sha1, const char *name)
 }
 
 int merge_recursive_generic(struct merge_options *o,
-   const unsigned char *head,
-   const unsigned char *merge,
+   const struct object_id *head,
+   const struct object_id *merge,
int num_base_list,
-   const unsigned char **base_list,
+   const struct object_id **base_list,
struct commit **result)
 {
int clean;
@@ -2015,9 +2015,9 @@ int merge_recursive_generic(struct merge_options *o,
int i;
for (i = 0; i < num_base_list; ++i) {
struct commit *base;
-   if (!(base = get_ref(base_list[i], 
sha1_to_hex(base_list[i]
+   if (!(base = get_ref(base_list[i], 
oid_to_hex(base_list[i]
return error(_("Could not parse object '%s'"),
- 

[PATCH v3 00/11] struct object_id, Part 4

[brian m. carlson]

This series is part 4 in a series of conversions to replace instances of
unsigned char [20] with struct object_id.  Most of this series touches
the merge-recursive code.

New in this series is the use of Coccinelle (http://coccinelle.lip6.fr/)
semantic patches.  These semantic patches can make automatic
transformations to C source code for cleanup or refactoring reasons.

This series introduces a set of transforms for the struct object_id
transition, cleans up some existing code with them, and applies a small
number of semantic patches to transform parts of the merge-recursive
code.  Some manual refactoring work follows.

Note that in the patches created with the semantic patches, the only
manual change was the definition of the struct member.  Opinions on
whether this is a viable technique for further work to ease both the
creation and review of patches are of course welcomed.

The testsuite continues to pass at each step, and this series rebases
cleanly on next.

I picked up Junio's change from sha_eq to oid_eq, but didn't convert the
calls to oidcmp.  I think the current version (with oid_eq) is actually
more readable than using oidcmp, if slightly less efficient.

Changes from v2:
* Pick up improvements from Junio's version on pu.
* Add oid_to_hex_r.
* Add oid_to_hex_r to object_id.cocci.
* Convert prep_temp_blob as suggested by Junio.
* Converted hashcpy(.*, null_sha1) to hashclr.

Changes from v1:
* Move the object ID transformations to contrib/examples/coccinelle.
* Add a README to that folder explaining what they are.
* Adjust the Coccinelle patches to transform plain structs before
  pointers to structs to avoid misconversions.  This addresses the issue
  that Johannes Sixt caught originally.

brian m. carlson (11):
  hex: add oid_to_hex_r.
  contrib/coccinelle: add basic Coccinelle transforms
  Convert hashcpy with null_sha1 to hashclr.
  coccinelle: apply object_id Coccinelle transformations
  diff: convert struct diff_filespec to struct object_id
  diff: rename struct diff_filespec's sha1_valid member
  merge-recursive: convert struct stage_data to use object_id
  merge-recursive: convert struct merge_file_info to object_id
  merge-recursive: convert leaf functions to use struct object_id
  merge-recursive: convert merge_recursive_generic to object_id
  diff: convert prep_temp_blob to struct object_id.

 bisect.c   |   2 +-
 builtin/blame.c|   6 +-
 builtin/fast-export.c  |  10 +-
 builtin/merge-recursive.c  |  20 +--
 builtin/merge.c|  15 +-
 builtin/reset.c|   4 +-
 builtin/unpack-objects.c   |   4 +-
 cache.h|   1 +
 combine-diff.c |  14 +-
 contrib/coccinelle/README  |   2 +
 contrib/coccinelle/object_id.cocci |  95 
 diff.c |  99 ++--
 diffcore-break.c   |   4 +-
 diffcore-rename.c  |  16 +-
 diffcore.h |   4 +-
 hex.c  |   5 +
 line-log.c |  12 +-
 merge-recursive.c  | 310 +++--
 merge-recursive.h  |   6 +-
 notes-merge.c  |  42 ++---
 refs/files-backend.c   |   4 +-
 submodule-config.c |   2 +-
 submodule.c|   4 +-
 t/helper/test-submodule-config.c   |   2 +-
 wt-status.c|   3 +-
 25 files changed, 403 insertions(+), 283 deletions(-)
 create mode 100644 contrib/coccinelle/README
 create mode 100644 contrib/coccinelle/object_id.cocci

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

[PATCH v3 03/11] Convert hashcpy with null_sha1 to hashclr.

[brian m. carlson]

hashcpy with null_sha1 as the source is equivalent to hashclr.  In
addition to being simpler, using hashclr may give the compiler a chance
to optimize better.  Convert instances of hashcpy with the source
argument of null_sha1 to hashclr.

This transformation was implemented using the following semantic patch:

@@
expression E1;
@@
-hashcpy(E1, null_sha1);
+hashclr(E1);

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 builtin/merge.c  | 2 +-
 builtin/unpack-objects.c | 4 ++--
 diff.c   | 2 +-
 submodule-config.c   | 2 +-
 t/helper/test-submodule-config.c | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index b555a1bf..a9b99c9f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1530,7 +1530,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
 * Stash away the local changes so that we can try more than one.
 */
save_state(stash))
-   hashcpy(stash, null_sha1);
+   hashclr(stash);
 
for (i = 0; i < use_strategies_nr; i++) {
int ret;
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 875e7ed9..172470bf 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -355,7 +355,7 @@ static void unpack_delta_entry(enum object_type type, 
unsigned long delta_size,
return; /* we are done */
else {
/* cannot resolve yet --- queue it */
-   hashcpy(obj_list[nr].sha1, null_sha1);
+   hashclr(obj_list[nr].sha1);
add_delta_to_list(nr, base_sha1, 0, delta_data, 
delta_size);
return;
}
@@ -406,7 +406,7 @@ static void unpack_delta_entry(enum object_type type, 
unsigned long delta_size,
 * The delta base object is itself a delta that
 * has not been resolved yet.
 */
-   hashcpy(obj_list[nr].sha1, null_sha1);
+   hashclr(obj_list[nr].sha1);
add_delta_to_list(nr, null_sha1, base_offset, 
delta_data, delta_size);
return;
}
diff --git a/diff.c b/diff.c
index fa78fc18..f2234012 100644
--- a/diff.c
+++ b/diff.c
@@ -3134,7 +3134,7 @@ static void diff_fill_sha1_info(struct diff_filespec *one)
if (!one->sha1_valid) {
struct stat st;
if (one->is_stdin) {
-   hashcpy(one->sha1, null_sha1);
+   hashclr(one->sha1);
return;
}
if (lstat(one->path, ) < 0)
diff --git a/submodule-config.c b/submodule-config.c
index db1847ff..077db405 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -377,7 +377,7 @@ static int gitmodule_sha1_from_commit(const unsigned char 
*commit_sha1,
int ret = 0;
 
if (is_null_sha1(commit_sha1)) {
-   hashcpy(gitmodules_sha1, null_sha1);
+   hashclr(gitmodules_sha1);
return 1;
}
 
diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c
index dab8c277..7922ffba 100644
--- a/t/helper/test-submodule-config.c
+++ b/t/helper/test-submodule-config.c
@@ -49,7 +49,7 @@ int main(int argc, char **argv)
path_or_name = arg[1];
 
if (commit[0] == '\0')
-   hashcpy(commit_sha1, null_sha1);
+   hashclr(commit_sha1);
else if (get_sha1(commit, commit_sha1) < 0)
die_usage(argc, argv, "Commit not found.");
 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 01/11] hex: add oid_to_hex_r.

[brian m. carlson]

This function works just like sha1_to_hex_r, except that it takes a
pointer to struct object_id instead of a pointer to unsigned char.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 cache.h | 1 +
 hex.c   | 5 +
 2 files changed, 6 insertions(+)

diff --git a/cache.h b/cache.h
index 6049f867..7bd7c472 100644
--- a/cache.h
+++ b/cache.h
@@ -1193,6 +1193,7 @@ extern int get_oid_hex(const char *hex, struct object_id 
*sha1);
  *   printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
  */
 extern char *sha1_to_hex_r(char *out, const unsigned char *sha1);
+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 */
 
diff --git a/hex.c b/hex.c
index 0519f853..9619b67a 100644
--- a/hex.c
+++ b/hex.c
@@ -77,6 +77,11 @@ char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)
return buffer;
 }
 
+char *oid_to_hex_r(char *buffer, const struct object_id *oid)
+{
+   return sha1_to_hex_r(buffer, oid->hash);
+}
+
 char *sha1_to_hex(const unsigned char *sha1)
 {
static int bufno;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 04/11] coccinelle: apply object_id Coccinelle transformations

[brian m. carlson]

Apply the set of semantic patches from contrib/coccinelle to convert
some leftover places using struct object_id's hash member to instead
use the wrapper functions that take struct object_id natively.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 bisect.c |  2 +-
 builtin/merge.c  | 13 ++---
 refs/files-backend.c |  4 ++--
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/bisect.c b/bisect.c
index 6d93edbc..ff147589 100644
--- a/bisect.c
+++ b/bisect.c
@@ -754,7 +754,7 @@ static void handle_bad_merge_base(void)
 static void handle_skipped_merge_base(const unsigned char *mb)
 {
char *mb_hex = sha1_to_hex(mb);
-   char *bad_hex = sha1_to_hex(current_bad_oid->hash);
+   char *bad_hex = oid_to_hex(current_bad_oid);
char *good_hex = join_sha1_array_hex(_revs, ' ');
 
warning("the merge base between %s and [%s] "
diff --git a/builtin/merge.c b/builtin/merge.c
index a9b99c9f..f66d06ce 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -501,7 +501,7 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
if (ref_exists(truname.buf)) {
strbuf_addf(msg,
"%s\t\tbranch '%s'%s of .\n",
-   sha1_to_hex(remote_head->object.oid.hash),
+   oid_to_hex(_head->object.oid),
truname.buf + 11,
(early ? " (early part)" : ""));
strbuf_release();
@@ -515,7 +515,7 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
desc = merge_remote_util(remote_head);
if (desc && desc->obj && desc->obj->type == OBJ_TAG) {
strbuf_addf(msg, "%s\t\t%s '%s'\n",
-   sha1_to_hex(desc->obj->oid.hash),
+   oid_to_hex(>obj->oid),
typename(desc->obj->type),
remote);
goto cleanup;
@@ -523,7 +523,7 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
}
 
strbuf_addf(msg, "%s\t\tcommit '%s'\n",
-   sha1_to_hex(remote_head->object.oid.hash), remote);
+   oid_to_hex(_head->object.oid), remote);
 cleanup:
strbuf_release();
strbuf_release();
@@ -1366,7 +1366,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
for (p = remoteheads; p; p = p->next) {
struct commit *commit = p->item;
strbuf_addf(, "GITHEAD_%s",
-   sha1_to_hex(commit->object.oid.hash));
+   oid_to_hex(>object.oid));
setenv(buf.buf, merge_remote_util(commit)->name, 1);
strbuf_reset();
if (fast_forward != FF_ONLY &&
@@ -1425,7 +1425,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
goto done;
} else if (fast_forward != FF_NO && !remoteheads->next &&
!common->next &&
-   !hashcmp(common->item->object.oid.hash, 
head_commit->object.oid.hash)) {
+   !oidcmp(>item->object.oid, 
_commit->object.oid)) {
/* Again the most common case of merging one remote. */
struct strbuf msg = STRBUF_INIT;
struct commit *commit;
@@ -1499,8 +1499,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
 * HEAD^^" would be missed.
 */
common_one = get_merge_bases(head_commit, j->item);
-   if (hashcmp(common_one->item->object.oid.hash,
-   j->item->object.oid.hash)) {
+   if (oidcmp(_one->item->object.oid, 
>item->object.oid)) {
up_to_date = 0;
break;
}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1f380764..dac3a222 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1725,14 +1725,14 @@ static int verify_lock(struct ref_lock *lock,
errno = save_errno;
return -1;
} else {
-   hashclr(lock->old_oid.hash);
+   oidclr(>old_oid);
return 0;
}
}
if (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) {
strbuf_addf(err, "ref %s is at %s but expected 

[PATCH v3 06/11] diff: rename struct diff_filespec's sha1_valid member

[brian m. carlson]

Now that this struct's sha1 member is called "oid", update the comment
and the sha1_valid member to be called "oid_valid" instead.  The
following Coccinelle semantic patch was used to implement this, followed
by the transformations in object_id.cocci:

@@
struct diff_filespec o;
@@
- o.sha1_valid
+ o.oid_valid

@@
struct diff_filespec *p;
@@
- p->sha1_valid
+ p->oid_valid

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 combine-diff.c|  4 ++--
 diff.c| 28 ++--
 diffcore-break.c  |  2 +-
 diffcore-rename.c |  4 ++--
 diffcore.h|  2 +-
 line-log.c| 10 +-
 6 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 1e1ad1c0..8e2a577b 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1269,7 +1269,7 @@ static struct diff_filepair *combined_pair(struct 
combine_diff_path *p,
pair->one[i].path = p->path;
pair->one[i].mode = p->parent[i].mode;
oidcpy(>one[i].oid, >parent[i].oid);
-   pair->one[i].sha1_valid = !is_null_oid(>parent[i].oid);
+   pair->one[i].oid_valid = !is_null_oid(>parent[i].oid);
pair->one[i].has_more_entries = 1;
}
pair->one[num_parent - 1].has_more_entries = 0;
@@ -1277,7 +1277,7 @@ static struct diff_filepair *combined_pair(struct 
combine_diff_path *p,
pair->two->path = p->path;
pair->two->mode = p->mode;
oidcpy(>two->oid, >oid);
-   pair->two->sha1_valid = !is_null_oid(>oid);
+   pair->two->oid_valid = !is_null_oid(>oid);
return pair;
 }
 
diff --git a/diff.c b/diff.c
index ae9edc30..9abb54ad 100644
--- a/diff.c
+++ b/diff.c
@@ -1933,7 +1933,7 @@ static void show_dirstat(struct diff_options *options)
 
name = p->two->path ? p->two->path : p->one->path;
 
-   if (p->one->sha1_valid && p->two->sha1_valid)
+   if (p->one->oid_valid && p->two->oid_valid)
content_changed = oidcmp(>one->oid, >two->oid);
else
content_changed = 1;
@@ -2640,7 +2640,7 @@ void fill_filespec(struct diff_filespec *spec, const 
unsigned char *sha1,
if (mode) {
spec->mode = canon_mode(mode);
hashcpy(spec->oid.hash, sha1);
-   spec->sha1_valid = sha1_valid;
+   spec->oid_valid = sha1_valid;
}
 }
 
@@ -2766,7 +2766,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
if (S_ISGITLINK(s->mode))
return diff_populate_gitlink(s, size_only);
 
-   if (!s->sha1_valid ||
+   if (!s->oid_valid ||
reuse_worktree_file(s->path, s->oid.hash, 0)) {
struct strbuf buf = STRBUF_INIT;
struct stat st;
@@ -2915,7 +2915,7 @@ static struct diff_tempfile *prepare_temp_file(const char 
*name,
}
 
if (!S_ISGITLINK(one->mode) &&
-   (!one->sha1_valid ||
+   (!one->oid_valid ||
 reuse_worktree_file(name, one->oid.hash, 1))) {
struct stat st;
if (lstat(name, ) < 0) {
@@ -2928,16 +2928,16 @@ static struct diff_tempfile *prepare_temp_file(const 
char *name,
if (strbuf_readlink(, name, st.st_size) < 0)
die_errno("readlink(%s)", name);
prep_temp_blob(name, temp, sb.buf, sb.len,
-  (one->sha1_valid ?
+  (one->oid_valid ?
one->oid.hash : null_sha1),
-  (one->sha1_valid ?
+  (one->oid_valid ?
one->mode : S_IFLNK));
strbuf_release();
}
else {
/* we can borrow from the file in the work tree */
temp->name = name;
-   if (!one->sha1_valid)
+   if (!one->oid_valid)
sha1_to_hex_r(temp->hex, null_sha1);
else
sha1_to_hex_r(temp->hex, one->oid.hash);
@@ -3134,7 +3134,7 @@ static void run_diff_cmd(const char *pgm,
 static void diff_fill_sha1_info(struct diff_filespec *one)
 {
if (DIFF_FILE_VALID(one)) {
-   if (!one->sha1_valid) {
+   if (!one->oid_valid) {
struct stat st;
if (one->is_stdin) {

[PATCH v3 02/11] contrib/coccinelle: add basic Coccinelle transforms

[brian m. carlson]

Coccinelle (http://coccinelle.lip6.fr/) is a program which performs
mechanical transformations on C programs using semantic patches.  These
semantic patches can be used to implement automatic refactoring and
maintenance tasks.

Add a set of basic semantic patches to convert common patterns related
to the struct object_id transformation, as well as a README.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 contrib/coccinelle/README  |  2 +
 contrib/coccinelle/object_id.cocci | 95 ++
 2 files changed, 97 insertions(+)
 create mode 100644 contrib/coccinelle/README
 create mode 100644 contrib/coccinelle/object_id.cocci

diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README
new file mode 100644
index ..9c2f8879
--- /dev/null
+++ b/contrib/coccinelle/README
@@ -0,0 +1,2 @@
+This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/)
+semantic patches that might be useful to developers.
diff --git a/contrib/coccinelle/object_id.cocci 
b/contrib/coccinelle/object_id.cocci
new file mode 100644
index ..8ccdbb56
--- /dev/null
+++ b/contrib/coccinelle/object_id.cocci
@@ -0,0 +1,95 @@
+@@
+expression E1;
+@@
+- is_null_sha1(E1.hash)
++ is_null_oid()
+
+@@
+expression E1;
+@@
+- is_null_sha1(E1->hash)
++ is_null_oid(E1)
+
+@@
+expression E1;
+@@
+- sha1_to_hex(E1.hash)
++ oid_to_hex()
+
+@@
+expression E1;
+@@
+- sha1_to_hex(E1->hash)
++ oid_to_hex(E1)
+
+@@
+expression E1;
+@@
+- sha1_to_hex_r(E1.hash)
++ oid_to_hex_r()
+
+@@
+expression E1;
+@@
+- sha1_to_hex_r(E1->hash)
++ oid_to_hex_r(E1)
+
+@@
+expression E1;
+@@
+- hashclr(E1.hash)
++ oidclr()
+
+@@
+expression E1;
+@@
+- hashclr(E1->hash)
++ oidclr(E1)
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1.hash, E2.hash)
++ oidcmp(, )
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1->hash, E2->hash)
++ oidcmp(E1, E2)
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1->hash, E2.hash)
++ oidcmp(E1, )
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1.hash, E2->hash)
++ oidcmp(, E2)
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1.hash, E2.hash)
++ oidcpy(, )
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1->hash, E2->hash)
++ oidcpy(E1, E2)
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1->hash, E2.hash)
++ oidcpy(E1, )
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1.hash, E2->hash)
++ oidcpy(, E2)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 06/11] diff: rename struct diff_filespec's sha1_valid member

[brian m. carlson]

On Fri, Jun 24, 2016 at 05:29:18PM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sand...@crustytoothpaste.net> writes:
> 
> 
> > @@
> > struct diff_filespec o;
> > @@
> > - o.sha1_valid
> > + o.oid_valid
> >
> > @@
> > struct diff_filespec *p;
> > @@
> > - p->sha1_valid
> > + p->oid_valid
> 
> Totally offtopic (from Git's point of view), but why does Coccinelle
> need both of these?  I recall that between v1 and v2 there were some
> confusing discussions about the order of these equivalent conversions
> mattering and the tool producing an incorrect conversion in v1.

As I understand it, Coccinelle doesn't transform . into ->.  Granted,
the latter is a special case of the former, but it might break workflows
where you're trying to convert an inline struct to a pointer to struct
or such.  It errs on the side of you being more explicit to allow more
flexibility in usage.
-- 
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

[PATCH v3 08/11] merge-recursive: convert struct merge_file_info to object_id

[brian m. carlson]

Convert struct merge_file_info to use struct object_id.  The following
Coccinelle semantic patch was used to implement this, followed by the
transformations in object_id.cocci:

@@
struct merge_file_info o;
@@
- o.sha
+ o.oid.hash

@@
struct merge_file_info *p;
@@
- p->sha
+ p->oid.hash

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 merge-recursive.c | 39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index a07050cd..bc455491 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -819,7 +819,7 @@ static void update_file(struct merge_options *o,
 /* Low level file merging, update and removal */
 
 struct merge_file_info {
-   unsigned char sha[20];
+   struct object_id oid;
unsigned mode;
unsigned clean:1,
 merge:1;
@@ -902,10 +902,10 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
result.clean = 0;
if (S_ISREG(a->mode)) {
result.mode = a->mode;
-   hashcpy(result.sha, a->oid.hash);
+   oidcpy(, >oid);
} else {
result.mode = b->mode;
-   hashcpy(result.sha, b->oid.hash);
+   oidcpy(, >oid);
}
} else {
if (!sha_eq(a->oid.hash, one->oid.hash) && !sha_eq(b->oid.hash, 
one->oid.hash))
@@ -925,9 +925,9 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
}
 
if (sha_eq(a->oid.hash, b->oid.hash) || sha_eq(a->oid.hash, 
one->oid.hash))
-   hashcpy(result.sha, b->oid.hash);
+   oidcpy(, >oid);
else if (sha_eq(b->oid.hash, one->oid.hash))
-   hashcpy(result.sha, a->oid.hash);
+   oidcpy(, >oid);
else if (S_ISREG(a->mode)) {
mmbuffer_t result_buf;
int merge_status;
@@ -939,21 +939,21 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
die(_("Failed to execute internal merge"));
 
if (write_sha1_file(result_buf.ptr, result_buf.size,
-   blob_type, result.sha))
+   blob_type, result.oid.hash))
die(_("Unable to add %s to database"),
a->path);
 
free(result_buf.ptr);
result.clean = (merge_status == 0);
} else if (S_ISGITLINK(a->mode)) {
-   result.clean = merge_submodule(result.sha,
+   result.clean = merge_submodule(result.oid.hash,
   one->path,
   one->oid.hash,
   a->oid.hash,
   b->oid.hash,
   !o->call_depth);
} else if (S_ISLNK(a->mode)) {
-   hashcpy(result.sha, a->oid.hash);
+   oidcpy(, >oid);
 
if (!sha_eq(a->oid.hash, b->oid.hash))
result.clean = 0;
@@ -1192,7 +1192,7 @@ static void conflict_rename_rename_1to2(struct 
merge_options *o,
 * pathname and then either rename the add-source file to that
 * unique path, or use that unique path instead of src here.
 */
-   update_file(o, 0, mfi.sha, mfi.mode, one->path);
+   update_file(o, 0, mfi.oid.hash, mfi.mode, one->path);
 
/*
 * Above, we put the merged content at the merge-base's
@@ -1255,16 +1255,16 @@ static void conflict_rename_rename_2to1(struct 
merge_options *o,
 * again later for the non-recursive merge.
 */
remove_file(o, 0, path, 0);
-   update_file(o, 0, mfi_c1.sha, mfi_c1.mode, a->path);
-   update_file(o, 0, mfi_c2.sha, mfi_c2.mode, b->path);
+   update_file(o, 0, mfi_c1.oid.hash, mfi_c1.mode, a->path);
+   update_file(o, 0, mfi_c2.oid.hash, mfi_c2.mode, b->path);
} else {
char *new_path1 = unique_path(o, path, ci->branch1);
char *new_path2 = unique_path(o, path, ci->branch2);
output(o, 1, _("Renaming %s to %s and %s to %s instead"),
   a->path, new_pa

Re: Easiest way to clone over an existing directory?

[brian m. carlson]

On Wed, Jun 15, 2016 at 08:51:34AM -0700, Josh Triplett wrote:
> Currently, every time I set up a new system, I run the following:
> 
> git clone $MY_HOMEDIR
> mv home/.git .
> rm -r home
> git checkout -f
> 
> This seems like an odd dance to go through.  But I can't just git clone
> into ~ directly, because git clone will not clone into an existing
> non-empty directory.
> 
> (I could use "git clone -n" to avoid the unnecessary checkout, but the
> files are small, and it wouldn't remove the need to rmdir so the number
> of commands would remain the same.)
> 
> Does some better way exist to handle this?  And if not, would it make
> sense for git clone to have an option to clone into an existing
> directory (which should also avoid setting junk_work_tree)?

My typical technique is something like the following:

  git init
  git remote add origin https://git.crustytoothpaste.net/git/bmc/homedir.git
  git pull origin master

I'm not sure if that's the officially sanctioned way to do it, but it
does work reliably.
-- 
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] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[brian m. carlson]

On Fri, Feb 05, 2016 at 09:54:50AM -0800, Junio C Hamano wrote:
> OK, as Brian said, that use case would need to be in the log
> message, at least.  I am curious, though, if you can give just a
> random string to username, or at least that must match what the
> underlying authentication mechanism uses.

You can give any invalid credentials you like.  When using Kerberos, the
provided username and password are ignored, because all the
authentication information is in the ticket, and it's all encrypted.

I'm happy to send a documentation patch for this, as it seems to come up
a lot.

> Brian, I can see how this would work in that use case, but I haven't
> convinced myself that the change would not affect other existing use
> cases that are supported--do you think of any that would negatively
> affect the user expeerience?

I'd have to test how it works with Basic auth as a fallback.  I don't
normally use that on my servers, so I'd have to enable it and try it
out.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[brian m. carlson]

On Fri, Feb 05, 2016 at 01:02:58PM -0800, Junio C Hamano wrote:
> Hmph, so documenting that :@
> as a supported way might be an ugly-looking solution to the original
> problem.  A less ugly-looking solution might be a boolean that can
> be set per URL (we already have urlmatch-config infrastructure to
> help us do so) to tell us to pass the empty credential to lubCurl,
> bypassing the step to ask the user for password that we do not use.
> 
> The end-result of either of these solution would strictly be better
> than the patch we discussed in that the end user will not have to
> interact with the prompt at all, right?

Yes, that's true.  I'll try to come up with a patch this weekend that
implements that (maybe remote.forceAuth = true or somesuch).
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[brian m. carlson]

On Fri, Feb 05, 2016 at 12:18:22PM +0300, Dmitry Vilkov wrote:
> You are right, we are using a bare URL (without a username component).
> With username encoded in URL everything works just fine. But it's
> generally wrong to pass creds in URL (in my opinion) and security
> policy of my employer prohibits doing such thing.
> Anyway, as you said libcurl needs valid (not NULL) username/password
> to do GSS-Negotiate, so there is nothing wrong if I set empty
> username/password combination when git prompts for creds. Even more,
> there is no other way to let libcurl to use GSS-Negotiate without
> username in URL.

You can literally do https://:@git.crustytoothpaste.net/git/repo.git as
the URL, and that will work.  GSS-Negotiate using Kerberos passes the
ticket, which contains the principal name in it, so an actual username
and password is not needed at all.  libcurl just needs something to tell
it to do authentication.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[brian m. carlson]

On Tue, Feb 02, 2016 at 12:37:19PM -0800, Junio C Hamano wrote:
> Dmitry Vilkov <dmitry.a.vil...@gmail.com> writes:
> 
> > This is fix of bug introduced by 4dbe66464 commit.
> 
> That would be 4dbe6646 (remote-curl: fall back to Basic auth if
> Negotiate fails, 2015-01-08) that appears in v2.3.1 and onward.
> 
> > The problem is that when username/password combination was not set,
> > the first HTTP(S) request will fail and user will be asked for
> > credentials. As a side effect of first HTTP(S) request, libcurl auth
> > method GSS-Negotiate will be disabled unconditionally. Although, we
> > haven't tried yet provided credentials for this auth method.

I'm unclear in what case you'd need to have a username and password
combination with GSS-Negotiate.  Kerberos doesn't use your password,
although you need some indication of a username (valid or not) to get
libcurl to do authentication.

Are you basically using a bare URL (without a username component) and
waiting for git to prompt you for the username, so that it will then
enable authentication?  If so, this patch looks fine for that, although
I'd expand on the commit message.  If not, could you provide an example
of what you're trying to do?

> Brian, comments?  Here is what you wrote in that commit:
> 
> If Basic and something else are offered, libcurl will never
> attempt to use Basic, even if the other option fails.  Teach the
> HTTP client code to stop trying authentication mechanisms that
> don't use a password (currently Negotiate) after the first
> failure, since if they failed the first time, they will never
> succeed.

I think what's happening here is no username is ever provided, so
libcurl never tries authentication in the first place.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[brian m. carlson]

On Wed, Jan 20, 2016 at 10:22:16AM +0100, Lars Schneider wrote:
> I tested different settings and found that running prove with "-j5" seems to 
> be
> the fastest option for the Travis CI machines. However, I also noticed that
> I got more test failures with higher parallelism (Dscho reported similar
> observations [1]).
> 
> Especially t0025-crlf-auto.sh failed multiple times ([2], [3]) on the OS X 
> builds
> when I increase the parallelism:
> 
> not ok 4 - text=true causes a CRLF file to be normalized
> not ok 9 - text=auto, autocrlf=true _does_ normalize CRLF files
> 
> Anyone an idea why that might be the case?

I've seen this on my personal box too[0] when running make -j4 all test.
I wasn't able to pin down why it was occurring, but if we're going to
run the tests in parallel, it's probably worth spending some time
figuring it out.

[0] Debian amd64/sid, ThinkPad X220.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH] Documentation: remove unnecessary backslashes

[brian m. carlson]

On Wed, Jan 20, 2016 at 03:34:30PM -0500, Jeff King wrote:
> On Wed, Jan 20, 2016 at 12:28:53PM -0800, Junio C Hamano wrote:
> > On the other hand, if this line must be spelled like the above to
> > please asciidoctor, i.e. the first and the last must not have
> > backslashes and the second must have backslashes, I'd have to say
> > we have a bigger problem.  Perhaps asciidoctor needs to be fixed
> > until normal people like we can rely on it.
> 
> Yeah, that is the "insane" part I mentioned. It _does_ make sense
> syntactically ("-1" cannot possibly be an attribute name, so it does not
> parse as one), but I do not like the degree to which writers must know
> all of the arcane syntax rules (and cannot rely on something simple like
> "{ is special, so I must escape it, and over-escaping is not a
> problem").

The underlying issue is that both AsciiDoc and Asciidoctor use regexps
to parse their data, which we all know is a bad idea.  Asciidoctor does
less forward looking because it's much faster, so it's a bit less
flexible with overescaping.

There are plans for Asciidoctor to move to a defined grammar at some
point, which should hopefully make things a bit less insane.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

[PATCH] http: add option to try authentication without username

[brian m. carlson]

Performing GSS-Negotiate authentication using Kerberos does not require
specifying a username or password, since that information is already
included in the ticket itself.  However, libcurl refuses to perform
authentication if it has not been provided with a username and password.
Add an option, http.emptyAuth, that provides libcurl with an empty
username and password to make it attempt authentication anyway.
---
I'm not super excited about this name, but I couldn't think of a better
one.  Suggestions welcome.

 Documentation/config.txt |  6 ++
 http.c   | 13 +++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27f02be3..f11de77e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1648,6 +1648,12 @@ http.proxyAuthMethod::
 * `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)
 --
 
+http.emptyAuth::
+   Attempt authentication without seeking a username or password.  This
+   can be used to attempt GSS-Negotiate authentication without specifying
+   a username in the URL, as libcurl normally requires a username for
+   authentication.
+
 http.cookieFile::
File containing previously stored cookie lines which should be used
in the Git http session, if they match the server. The file format
diff --git a/http.c b/http.c
index dfc53c1e..a12a804b 100644
--- a/http.c
+++ b/http.c
@@ -87,6 +87,7 @@ static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
+static int curl_empty_auth;
 
 #if LIBCURL_VERSION_NUM >= 0x071700
 /* Use CURLOPT_KEYPASSWD as is */
@@ -299,14 +300,22 @@ static int http_options(const char *var, const char 
*value, void *cb)
if (!strcmp("http.useragent", var))
return git_config_string(_agent, var, value);
 
+   if (!strcmp("http.emptyauth", var)) {
+   curl_empty_auth = git_config_bool(var, value);
+   return 0;
+   }
+
/* Fall back on the default ones */
return git_default_config(var, value, cb);
 }
 
 static void init_curl_http_auth(CURL *result)
 {
-   if (!http_auth.username)
+   if (!http_auth.username) {
+   if (curl_empty_auth)
+   curl_easy_setopt(result, CURLOPT_USERPWD, ":");
return;
+   }
 
credential_fill(_auth);
 
@@ -827,7 +836,7 @@ struct active_request_slot *get_active_slot(void)
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
 #endif
-   if (http_auth.password)
+   if (http_auth.password || curl_empty_auth)
init_curl_http_auth(slot->curl);
 
return slot;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] http: add option to try authentication without username

[brian m. carlson]

On Mon, Feb 15, 2016 at 03:19:25PM -0500, Eric Sunshine wrote:
> On Mon, Feb 15, 2016 at 1:44 PM, brian m. carlson
> <sand...@crustytoothpaste.net> wrote:
> > Performing GSS-Negotiate authentication using Kerberos does not require
> > specifying a username or password, since that information is already
> > included in the ticket itself.  However, libcurl refuses to perform
> > authentication if it has not been provided with a username and password.
> > Add an option, http.emptyAuth, that provides libcurl with an empty
> > username and password to make it attempt authentication anyway.
> 
> I'm not familiar with this code, so let me know if my comments (below)
> are off the mark...
> 
> > ---
> > diff --git a/http.c b/http.c
> > +++ b/http.c
> > @@ -299,14 +300,22 @@ static int http_options(const char *var, const char 
> > *value, void *cb)
> >  static void init_curl_http_auth(CURL *result)
> >  {
> > -   if (!http_auth.username)
> > +   if (!http_auth.username) {
> > +   if (curl_empty_auth)
> > +   curl_easy_setopt(result, CURLOPT_USERPWD, ":");
> 
> Does this need to take LIBCURL_VERSION_NUM into account? Other code
> which uses CURLOPT_USERPWD only does so for certain versions of
> libcurl, otherwise CURLOPT_USERNAME and CURLOPT_PASSWORD is used.

This is the oldest version, which means it's the most compatible.  Since
we don't need to consider odd usernames, it seemed silly to have both
cases present in the code.  The benefit of using the pair of options is
that we don't leak memory in the non-empty auth case.

> > return;
> > +   }
> >
> > credential_fill(_auth);
> >
> > @@ -827,7 +836,7 @@ struct active_request_slot *get_active_slot(void)
> >  #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> > curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
> >  #endif
> > -   if (http_auth.password)
> > +   if (http_auth.password || curl_empty_auth)
> > init_curl_http_auth(slot->curl);
> >
> > return slot;
> 
> Rather than sprinkling curl_empty_auth special cases here and there,
> would it be possible to simply set http_auth.username and
> http_auth.password to empty strings early on if they are not already
> set and curl_empty_auth is true, and then let the:
> 
> strbuf_addf(, "%s:%s",
> http_auth.username, http_auth.password);
> 
> in init_curl_http_auth() handle them in the normal fashion, with the
> end result being the same ":" set explicitly by this patch?

That would work.  I was concerned about the credential_fill call
actually prompting the user, but it appears that it doesn't do that if
the password already exists.  I don't know if we want to rely on that
functionality, though.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH] http: add option to try authentication without username

[brian m. carlson]

On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
> On Mon, Feb 15, 2016 at 08:29:37PM +0000, brian m. carlson wrote:
> > That would work.  I was concerned about the credential_fill call
> > actually prompting the user, but it appears that it doesn't do that if
> > the password already exists.  I don't know if we want to rely on that
> > functionality, though.
> 
> Yeah, credential_fill() will treat that as a noop, as it is no different
> than getting "https://user:p...@example.com; in the URL in the first
> place. But it will _also_ send the result to credential_approve() and
> credential_reject(), which you probably don't want (because you do not
> want to store these useless dummy credentials in your keystore).

Correct.

> So I think this hack should remain purely at the curl level, and never
> touch the credential struct at all.
> 
> Which is a shame, because I think Eric's suggestion is otherwise much
> more readable. :)

Yes, I agree.  That would have been a much nicer and smaller change.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH] http: add option to try authentication without username

[brian m. carlson]

On Mon, Feb 15, 2016 at 01:39:30PM -0800, Junio C Hamano wrote:
> "brian m. carlson" <sand...@crustytoothpaste.net> writes:
> 
> > On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
> > ...
> >> So I think this hack should remain purely at the curl level, and never
> >> touch the credential struct at all.
> >> 
> >> Which is a shame, because I think Eric's suggestion is otherwise much
> >> more readable. :)
> >
> > Yes, I agree.  That would have been a much nicer and smaller change.
> 
> Alright, reading all reviews and taking them into account, the
> original, when a Sign-off is added, would be acceptable, it seems.

Sorry about that.  Please add my

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH] http: add option to try authentication without username

[brian m. carlson]

On Mon, Feb 15, 2016 at 04:46:43PM -0500, Eric Sunshine wrote:
> On Mon, Feb 15, 2016 at 4:39 PM, Junio C Hamano <gits...@pobox.com> wrote:
> > "brian m. carlson" <sand...@crustytoothpaste.net> writes:
> >> On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
> >>> So I think this hack should remain purely at the curl level, and never
> >>> touch the credential struct at all.
> >>>
> >>> Which is a shame, because I think Eric's suggestion is otherwise much
> >>> more readable. :)
> >>
> >> Yes, I agree.  That would have been a much nicer and smaller change.
> >
> > Alright, reading all reviews and taking them into account, the
> > original, when a Sign-off is added, would be acceptable, it seems.
> 
> One final question: Keeping in mind my lack of familiarity with this
> particular use-case, would it be possible to infer the need to employ
> this curl-specific workaround rather than making users tweak a config
> setting? Or would that be a security risk or an otherwise stupid idea?

It's not very easy to infer whether it's needed.  We'd need to know what
types of authentication are offered, and somehow we'd have to intuit
proper behavior when both GSS-Negotiate and Basic are enabled.  Some
sites do that because you can use Basic against the Kerberos database.
One user might legitimately want to always use Basic (e.g. with a
password manager) and another might always want to use Negotiate.
Setting this option is one way to ensure the latter.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[brian m. carlson]

On Sat, Feb 20, 2016 at 05:35:19PM +0300, Dmitry Vilkov wrote:
> Maybe you could accept my patch, so users would use
> "credential.helper=store" to avoid using ":@" in remote URL? At least
> for now, while there is no good solution to this issue? It would be
> very helpful because now we have to have our own version of patched
> Git :(

I sent in a patch (and I believe I CC'd you) that adds an option
http.emptyAuth that can be used in this case.  It should make its way to
a future release.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: Making git apply always work relative to current directory

[brian m. carlson]

On Sat, Mar 05, 2016 at 11:31:53AM -0800, Junio C Hamano wrote:
> "brian m. carlson" <sand...@crustytoothpaste.net> writes:
> 
> > If I run git apply --no-index --verbose , it succeeds in both
> > cases, but when I'm in the git repository, it *silently does nothing*.
> 
> That originally sounded peculiar to me and I suspected it to be a
> bug, but it looks like a designed-in feature and with us since
> 
> commit edf2e37002eeb30a2ccad5db3b3e1fe41cdc7eb0
> Author: Junio C Hamano <jun...@cox.net>
> Date:   Fri Nov 25 23:14:15 2005 -0800
> 
> git-apply: work from subdirectory.
> 
> When applying a patch to index file, we need to know where GIT_DIR is;
> use setup_git_directory() to find it out.  This also allows us to work
> from a subdirectory if we wanted to.
> 
> When git-apply is run from a subdirectory, it applies the given patch
> only to the files under the current directory and below.
> 
> Signed-off-by: Junio C Hamano <jun...@cox.net>
> 
> So exclusion by use_patch() for paths outside the current directory
> seems to be a feature; the log message does not say "why", but if I
> have to guess, the reasoning was probably "The old world order was
> that the command has to always be run from the top level.  A user
> who wants to run it from a subdirectory must be doing so for a
> reason, e.g. 'I am currently working in this directory, do not touch
> outside this area'".  In any case, I suspect that the existing tooling
> people built over the past 10 years around "git apply" already depends
> on this behaviour, so we cannot lightly change it.

I was planning to add a --here option (maybe spelled
--current-directory) that would change that behavior, since I figured
that people would be relying on the current behavior.  The man page
clearly indicates that some people are using it as a better GNU patch,
so an option that does that might be useful.

Regardless, I'd say that --verbose should cause git apply to say
something.  I'm not a newbie with Git, and I spent about an hour trying
to figure this out.  I've also been bitten by it before elsewhere.

> Is it so hard to temporarily go up to the root, run "git apply", and
> come back?  You can use "--no-index --directory=trash" for both
> cases that way.

The existing code used patch until we realized that older versions of
GNU patch can't apply certain git diffs.  It's not super easy to change
it, but I suppose we could.
-- 
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

Making git apply always work relative to current directory

[brian m. carlson]

I have a piece of software which must run out of a given directory.  In
development, this is a git repository, and in production it is not.  I
also have an ignored subdirectory where I would like to use git apply to
apply patches (in both environments).

If I run git apply --no-index --verbose , it succeeds in both
cases, but when I'm in the git repository, it *silently does nothing*.
I have to provide a --directory argument in order for it to function
underneath the repository, but of course that doesn't work when the
directory isn't within a repository.  --unsafe-paths did not seem to
make a difference.  I'm using Git 2.7.2.

Is there a way to tell git apply that it should apply relative to the
current working directory, no matter what?  I'm happy to send a patch to
either the documentation or git apply if necessary.
-- 
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

[PATCH v2 0/6] object_id Part 3

[brian m. carlson]

This is the third of a series of patches to convert instances of
unsigned char[20] to struct object_id.  The focus in this series was to
convert test-match-trees, related functions, and some dependencies.
struct name_entry was converted as part of these dependencies.

The riskiest part of this series is the conversion of struct name_entry.
Compatibility with unconverted code requires dereferencing the new oid
member, but there are at least some places where we explicitly check
that it is not NULL.  Most of these are protected by a check for a
nonzero mode.

This series rebases onto next cleanly and is not likely to conflict with
other topics in flight.  My intention is to send smaller series more
frequently, with the goal of avoiding or minimizing conflicts where
possible.

Changes from v1:
* Remove dereference of object_id pointer in boolean context.

brian m. carlson (6):
  Introduce a get_oid function.
  test-match-trees: convert to use struct object_id
  match-trees: convert shift_tree and shift_tree_by to object_id
  Convert struct name_entry to use struct object_id.
  tree-walk: convert tree_entry_extract to struct object_id
  match-trees: convert several leaf functions to struct object_id

 builtin/grep.c |  6 ++---
 builtin/merge-tree.c   | 18 +++
 builtin/pack-objects.c |  4 ++--
 builtin/reflog.c   |  4 ++--
 cache-tree.c   |  4 ++--
 cache.h|  6 +++--
 fsck.c | 10 -
 http-push.c|  4 ++--
 list-objects.c |  6 ++---
 match-trees.c  | 60 +-
 merge-recursive.c  |  4 ++--
 notes.c|  4 ++--
 revision.c |  4 ++--
 sha1_name.c|  9 
 test-match-trees.c | 14 ++--
 tree-diff.c|  8 +++
 tree-walk.c| 16 +++---
 tree-walk.h|  8 +++
 tree.c | 10 -
 unpack-trees.c |  4 ++--
 walker.c   |  4 ++--
 21 files changed, 109 insertions(+), 98 deletions(-)

-- 
2.8.1.369.geae769a

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

[PATCH v2 4/6] Convert struct name_entry to use struct object_id.

[brian m. carlson]

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 builtin/grep.c |  6 +++---
 builtin/merge-tree.c   | 18 +-
 builtin/pack-objects.c |  4 ++--
 builtin/reflog.c   |  4 ++--
 cache-tree.c   |  4 ++--
 fsck.c |  4 ++--
 http-push.c|  4 ++--
 list-objects.c |  6 +++---
 match-trees.c  |  2 +-
 notes.c|  4 ++--
 revision.c |  4 ++--
 tree-diff.c|  6 +++---
 tree-walk.c|  6 +++---
 tree-walk.h|  6 +++---
 tree.c | 10 +-
 unpack-trees.c |  4 ++--
 walker.c   |  4 ++--
 17 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 111b6f6c..462e6079 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -438,7 +438,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.sha1, base->buf, tn_len,
+   hit |= grep_sha1(opt, entry.oid->hash, base->buf, 
tn_len,
 check_attr ? base->buf + tn_len : 
NULL);
}
else if (S_ISDIR(entry.mode)) {
@@ -447,10 +447,10 @@ static int grep_tree(struct grep_opt *opt, const struct 
pathspec *pathspec,
void *data;
unsigned long size;
 
-   data = lock_and_read_sha1_file(entry.sha1, , 
);
+   data = lock_and_read_sha1_file(entry.oid->hash, , 
);
if (!data)
die(_("unable to read tree (%s)"),
-   sha1_to_hex(entry.sha1));
+   oid_to_hex(entry.oid));
 
strbuf_addch(base, '/');
init_tree_desc(, data, size);
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index ca570041..5b7ab9b9 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -150,15 +150,15 @@ static void show_result(void)
 /* An empty entry never compares same, not even to another empty entry */
 static int same_entry(struct name_entry *a, struct name_entry *b)
 {
-   return  a->sha1 &&
-   b->sha1 &&
-   !hashcmp(a->sha1, b->sha1) &&
+   return  a->oid &&
+   b->oid &&
+   !oidcmp(a->oid, b->oid) &&
a->mode == b->mode;
 }
 
 static int both_empty(struct name_entry *a, struct name_entry *b)
 {
-   return !(a->sha1 || b->sha1);
+   return !(a->oid || b->oid);
 }
 
 static struct merge_list *create_entry(unsigned stage, unsigned mode, const 
unsigned char *sha1, const char *path)
@@ -188,8 +188,8 @@ static void resolve(const struct traverse_info *info, 
struct name_entry *ours, s
return;
 
path = traverse_path(info, result);
-   orig = create_entry(2, ours->mode, ours->sha1, path);
-   final = create_entry(0, result->mode, result->sha1, path);
+   orig = create_entry(2, ours->mode, ours->oid->hash, path);
+   final = create_entry(0, result->mode, result->oid->hash, path);
 
final->link = orig;
 
@@ -213,7 +213,7 @@ static void unresolved_directory(const struct traverse_info 
*info,
 
newbase = traverse_path(info, p);
 
-#define ENTRY_SHA1(e) (((e)->mode && S_ISDIR((e)->mode)) ? (e)->sha1 : NULL)
+#define ENTRY_SHA1(e) (((e)->mode && S_ISDIR((e)->mode)) ? (e)->oid->hash : 
NULL)
buf0 = fill_tree_descriptor(t+0, ENTRY_SHA1(n + 0));
buf1 = fill_tree_descriptor(t+1, ENTRY_SHA1(n + 1));
buf2 = fill_tree_descriptor(t+2, ENTRY_SHA1(n + 2));
@@ -239,7 +239,7 @@ static struct merge_list *link_entry(unsigned stage, const 
struct traverse_info
path = entry->path;
else
path = traverse_path(info, n);
-   link = create_entry(stage, n->mode, n->sha1, path);
+   link = create_entry(stage, n->mode, n->oid->hash, path);
link->link = entry;
return link;
 }
@@ -314,7 +314,7 @@ static int threeway_callback(int n, unsigned long mask, 
unsigned long dirmask, s
}
 
if (same_entry(entry+0, entry+1)) {
-   if (entry[2].sha1 && !S_ISDIR(entry[2].mode)) {
+   if (entry[2].oid && !S_ISDIR(entry[2].mode)) {
/* We did not touch, they modified -- take theirs */
resolve(info, entry+1, entry+2);
return mask;
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a27de5b3..d56b2c2d 1006

[PATCH v2 5/6] tree-walk: convert tree_entry_extract to struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 fsck.c|  6 +++---
 match-trees.c | 12 ++--
 tree-diff.c   |  2 +-
 tree-walk.c   | 10 +-
 tree-walk.h   |  4 ++--
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/fsck.c b/fsck.c
index 606eba8c..92b17f5d 100644
--- a/fsck.c
+++ b/fsck.c
@@ -450,11 +450,11 @@ static int fsck_tree(struct tree *item, struct 
fsck_options *options)
while (desc.size) {
unsigned mode;
const char *name;
-   const unsigned char *sha1;
+   const struct object_id *oid;
 
-   sha1 = tree_entry_extract(, , );
+   oid = tree_entry_extract(, , );
 
-   has_null_sha1 |= is_null_sha1(sha1);
+   has_null_sha1 |= is_null_oid(oid);
has_full_path |= !!strchr(name, '/');
has_empty_name |= !*name;
has_dot |= !strcmp(name, ".");
diff --git a/match-trees.c b/match-trees.c
index 751f8f20..8ca7c68f 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -131,14 +131,14 @@ static void match_trees(const unsigned char *hash1,
 
while (one.size) {
const char *path;
-   const unsigned char *elem;
+   const struct object_id *elem;
unsigned mode;
int score;
 
elem = tree_entry_extract(, , );
if (!S_ISDIR(mode))
goto next;
-   score = score_trees(elem, hash2);
+   score = score_trees(elem->hash, hash2);
if (*best_score < score) {
free(*best_match);
*best_match = xstrfmt("%s%s", base, path);
@@ -146,7 +146,7 @@ static void match_trees(const unsigned char *hash1,
}
if (recurse_limit) {
char *newbase = xstrfmt("%s%s/", base, path);
-   match_trees(elem, hash2, best_score, best_match,
+   match_trees(elem->hash, hash2, best_score, best_match,
newbase, recurse_limit - 1);
free(newbase);
}
@@ -191,15 +191,15 @@ static int splice_tree(const unsigned char *hash1,
while (desc.size) {
const char *name;
unsigned mode;
-   const unsigned char *sha1;
+   const struct object_id *oid;
 
-   sha1 = tree_entry_extract(, , );
+   oid = tree_entry_extract(, , );
if (strlen(name) == toplen &&
!memcmp(name, prefix, toplen)) {
if (!S_ISDIR(mode))
die("entry %s in tree %s is not a tree",
name, sha1_to_hex(hash1));
-   rewrite_here = (unsigned char *) sha1;
+   rewrite_here = (unsigned char *) oid->hash;
break;
}
update_tree_entry();
diff --git a/tree-diff.c b/tree-diff.c
index 402f9ff2..ff4e0d3c 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -183,7 +183,7 @@ static struct combine_diff_path *emit_path(struct 
combine_diff_path *p,
 
if (t) {
/* path present in resulting tree */
-   sha1 = tree_entry_extract(t, , );
+   sha1 = tree_entry_extract(t, , )->hash;
pathlen = tree_entry_len(>entry);
isdir = S_ISDIR(mode);
} else {
diff --git a/tree-walk.c b/tree-walk.c
index fab57dd5..ce278424 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -433,10 +433,10 @@ static int find_tree_entry(struct tree_desc *t, const 
char *name, unsigned char
int namelen = strlen(name);
while (t->size) {
const char *entry;
-   const unsigned char *sha1;
+   const struct object_id *oid;
int entrylen, cmp;
 
-   sha1 = tree_entry_extract(t, , mode);
+   oid = tree_entry_extract(t, , mode);
entrylen = tree_entry_len(>entry);
update_tree_entry(t);
if (entrylen > namelen)
@@ -447,7 +447,7 @@ static int find_tree_entry(struct tree_desc *t, const char 
*name, unsigned char
if (cmp < 0)
break;
if (entrylen == namelen) {
-   hashcpy(result, sha1);
+   hashcpy(result, oid->hash);
return 0;
}
if (name[entrylen] != '/')
@@ -455,10 +455,10 @@ static int find_tree_entry(struct tree_desc *t, const 
char *name, unsigned char
if (!S_ISDIR(*mode))
break;
if (++entrylen == namelen) {
-   hashcpy(result, sha1

[PATCH v2 2/6] test-match-trees: convert to use struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 test-match-trees.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/test-match-trees.c b/test-match-trees.c
index 4dad7095..41aff841 100644
--- a/test-match-trees.c
+++ b/test-match-trees.c
@@ -3,24 +3,24 @@
 
 int main(int ac, char **av)
 {
-   unsigned char hash1[20], hash2[20], shifted[20];
+   struct object_id hash1, hash2, shifted;
struct tree *one, *two;
 
setup_git_directory();
 
-   if (get_sha1(av[1], hash1))
+   if (get_oid(av[1], ))
die("cannot parse %s as an object name", av[1]);
-   if (get_sha1(av[2], hash2))
+   if (get_oid(av[2], ))
die("cannot parse %s as an object name", av[2]);
-   one = parse_tree_indirect(hash1);
+   one = parse_tree_indirect(hash1.hash);
if (!one)
die("not a tree-ish %s", av[1]);
-   two = parse_tree_indirect(hash2);
+   two = parse_tree_indirect(hash2.hash);
if (!two)
die("not a tree-ish %s", av[2]);
 
-   shift_tree(one->object.oid.hash, two->object.oid.hash, shifted, -1);
-   printf("shifted: %s\n", sha1_to_hex(shifted));
+   shift_tree(one->object.oid.hash, two->object.oid.hash, shifted.hash, 
-1);
+   printf("shifted: %s\n", oid_to_hex());
 
exit(0);
 }
-- 
2.8.1.369.geae769a

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

[PATCH v2 1/6] Introduce a get_oid function.

[brian m. carlson]

The get_oid function is equivalent to the get_sha1 function, but uses a
struct object_id instead.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 cache.h | 2 ++
 sha1_name.c | 9 +
 2 files changed, 11 insertions(+)

diff --git a/cache.h b/cache.h
index 2711048c..22b73646 100644
--- a/cache.h
+++ b/cache.h
@@ -1156,6 +1156,8 @@ extern int get_sha1_blob(const char *str, unsigned char 
*sha1);
 extern void maybe_die_on_misspelt_object_name(const char *name, const char 
*prefix);
 extern int get_sha1_with_context(const char *str, unsigned flags, unsigned 
char *sha1, struct object_context *orc);
 
+extern int get_oid(const char *str, struct object_id *oid);
+
 typedef int each_abbrev_fn(const unsigned char *sha1, void *);
 extern int for_each_abbrev(const char *prefix, each_abbrev_fn, void *);
 
diff --git a/sha1_name.c b/sha1_name.c
index 776101e8..ca7ddd6f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1215,6 +1215,15 @@ int get_sha1(const char *name, unsigned char *sha1)
 }
 
 /*
+ * This is like "get_sha1()", but for struct object_id.
+ */
+int get_oid(const char *name, struct object_id *oid)
+{
+   return get_sha1(name, oid->hash);
+}
+
+
+/*
  * Many callers know that the user meant to name a commit-ish by
  * syntactical positions where the object name appears.  Calling this
  * function allows the machinery to disambiguate shorter-than-unique
-- 
2.8.1.369.geae769a

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

[PATCH v2 3/6] match-trees: convert shift_tree and shift_tree_by to object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 cache.h|  4 ++--
 match-trees.c  | 44 ++--
 merge-recursive.c  |  4 ++--
 test-match-trees.c |  2 +-
 4 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/cache.h b/cache.h
index 22b73646..70091e73 100644
--- a/cache.h
+++ b/cache.h
@@ -1768,8 +1768,8 @@ int add_files_to_cache(const char *prefix, const struct 
pathspec *pathspec, int
 extern int diff_auto_refresh_index;
 
 /* match-trees.c */
-void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, 
int);
-void shift_tree_by(const unsigned char *, const unsigned char *, unsigned char 
*, const char *);
+void shift_tree(const struct object_id *, const struct object_id *, struct 
object_id *, int);
+void shift_tree_by(const struct object_id *, const struct object_id *, struct 
object_id *, const char *);
 
 /*
  * whitespace rules.
diff --git a/match-trees.c b/match-trees.c
index 1ce0954a..9977752a 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -229,9 +229,9 @@ static int splice_tree(const unsigned char *hash1,
  * other hand, it could cover tree one and we might need to pick a
  * subtree of it.
  */
-void shift_tree(const unsigned char *hash1,
-   const unsigned char *hash2,
-   unsigned char *shifted,
+void shift_tree(const struct object_id *hash1,
+   const struct object_id *hash2,
+   struct object_id *shifted,
int depth_limit)
 {
char *add_prefix;
@@ -245,7 +245,7 @@ void shift_tree(const unsigned char *hash1,
if (!depth_limit)
depth_limit = 2;
 
-   add_score = del_score = score_trees(hash1, hash2);
+   add_score = del_score = score_trees(hash1->hash, hash2->hash);
add_prefix = xcalloc(1, 1);
del_prefix = xcalloc(1, 1);
 
@@ -253,16 +253,16 @@ void shift_tree(const unsigned char *hash1,
 * See if one's subtree resembles two; if so we need to prefix
 * two with a few fake trees to match the prefix.
 */
-   match_trees(hash1, hash2, _score, _prefix, "", depth_limit);
+   match_trees(hash1->hash, hash2->hash, _score, _prefix, "", 
depth_limit);
 
/*
 * See if two's subtree resembles one; if so we need to
 * pick only subtree of two.
 */
-   match_trees(hash2, hash1, _score, _prefix, "", depth_limit);
+   match_trees(hash2->hash, hash1->hash, _score, _prefix, "", 
depth_limit);
 
/* Assume we do not have to do any shifting */
-   hashcpy(shifted, hash2);
+   oidcpy(shifted, hash2);
 
if (add_score < del_score) {
/* We need to pick a subtree of two */
@@ -271,16 +271,16 @@ void shift_tree(const unsigned char *hash1,
if (!*del_prefix)
return;
 
-   if (get_tree_entry(hash2, del_prefix, shifted, ))
+   if (get_tree_entry(hash2->hash, del_prefix, shifted->hash, 
))
die("cannot find path %s in tree %s",
-   del_prefix, sha1_to_hex(hash2));
+   del_prefix, oid_to_hex(hash2));
return;
}
 
if (!*add_prefix)
return;
 
-   splice_tree(hash1, add_prefix, hash2, shifted);
+   splice_tree(hash1->hash, add_prefix, hash2->hash, shifted->hash);
 }
 
 /*
@@ -288,44 +288,44 @@ void shift_tree(const unsigned char *hash1,
  * Unfortunately we cannot fundamentally tell which one to
  * be prefixed, as recursive merge can work in either direction.
  */
-void shift_tree_by(const unsigned char *hash1,
-  const unsigned char *hash2,
-  unsigned char *shifted,
+void shift_tree_by(const struct object_id *hash1,
+  const struct object_id *hash2,
+  struct object_id *shifted,
   const char *shift_prefix)
 {
-   unsigned char sub1[20], sub2[20];
+   struct object_id sub1, sub2;
unsigned mode1, mode2;
unsigned candidate = 0;
 
/* Can hash2 be a tree at shift_prefix in tree hash1? */
-   if (!get_tree_entry(hash1, shift_prefix, sub1, ) &&
+   if (!get_tree_entry(hash1->hash, shift_prefix, sub1.hash, ) &&
S_ISDIR(mode1))
candidate |= 1;
 
/* Can hash1 be a tree at shift_prefix in tree hash2? */
-   if (!get_tree_entry(hash2, shift_prefix, sub2, ) &&
+   if (!get_tree_entry(hash2->hash, shift_prefix, sub2.hash, ) &&
S_ISDIR(mode2))
candidate |= 2;
 
if (candidate == 3) {
/* Both are plausible -- we need to evaluate the score */
-   int best_score = score_trees(hash1, hash2);
+   int best_score = score_trees(hash1->hash, hash2->hash);
 

[PATCH v2 6/6] match-trees: convert several leaf functions to struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 match-trees.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/match-trees.c b/match-trees.c
index 8ca7c68f..396b7338 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -48,17 +48,17 @@ static int score_matches(unsigned mode1, unsigned mode2, 
const char *path)
 }
 
 static void *fill_tree_desc_strict(struct tree_desc *desc,
-  const unsigned char *hash)
+  const struct object_id *hash)
 {
void *buffer;
enum object_type type;
unsigned long size;
 
-   buffer = read_sha1_file(hash, , );
+   buffer = read_sha1_file(hash->hash, , );
if (!buffer)
-   die("unable to read tree (%s)", sha1_to_hex(hash));
+   die("unable to read tree (%s)", oid_to_hex(hash));
if (type != OBJ_TREE)
-   die("%s is not a tree", sha1_to_hex(hash));
+   die("%s is not a tree", oid_to_hex(hash));
init_tree_desc(desc, buffer, size);
return buffer;
 }
@@ -73,7 +73,7 @@ static int base_name_entries_compare(const struct name_entry 
*a,
 /*
  * Inspect two trees, and give a score that tells how similar they are.
  */
-static int score_trees(const unsigned char *hash1, const unsigned char *hash2)
+static int score_trees(const struct object_id *hash1, const struct object_id 
*hash2)
 {
struct tree_desc one;
struct tree_desc two;
@@ -119,8 +119,8 @@ static int score_trees(const unsigned char *hash1, const 
unsigned char *hash2)
 /*
  * Match one itself and its subtrees with two and pick the best match.
  */
-static void match_trees(const unsigned char *hash1,
-   const unsigned char *hash2,
+static void match_trees(const struct object_id *hash1,
+   const struct object_id *hash2,
int *best_score,
char **best_match,
const char *base,
@@ -138,7 +138,7 @@ static void match_trees(const unsigned char *hash1,
elem = tree_entry_extract(, , );
if (!S_ISDIR(mode))
goto next;
-   score = score_trees(elem->hash, hash2);
+   score = score_trees(elem, hash2);
if (*best_score < score) {
free(*best_match);
*best_match = xstrfmt("%s%s", base, path);
@@ -146,7 +146,7 @@ static void match_trees(const unsigned char *hash1,
}
if (recurse_limit) {
char *newbase = xstrfmt("%s%s/", base, path);
-   match_trees(elem->hash, hash2, best_score, best_match,
+   match_trees(elem, hash2, best_score, best_match,
newbase, recurse_limit - 1);
free(newbase);
}
@@ -245,7 +245,7 @@ void shift_tree(const struct object_id *hash1,
if (!depth_limit)
depth_limit = 2;
 
-   add_score = del_score = score_trees(hash1->hash, hash2->hash);
+   add_score = del_score = score_trees(hash1, hash2);
add_prefix = xcalloc(1, 1);
del_prefix = xcalloc(1, 1);
 
@@ -253,13 +253,13 @@ void shift_tree(const struct object_id *hash1,
 * See if one's subtree resembles two; if so we need to prefix
 * two with a few fake trees to match the prefix.
 */
-   match_trees(hash1->hash, hash2->hash, _score, _prefix, "", 
depth_limit);
+   match_trees(hash1, hash2, _score, _prefix, "", depth_limit);
 
/*
 * See if two's subtree resembles one; if so we need to
 * pick only subtree of two.
 */
-   match_trees(hash2->hash, hash1->hash, _score, _prefix, "", 
depth_limit);
+   match_trees(hash2, hash1, _score, _prefix, "", depth_limit);
 
/* Assume we do not have to do any shifting */
oidcpy(shifted, hash2);
@@ -309,16 +309,16 @@ void shift_tree_by(const struct object_id *hash1,
 
if (candidate == 3) {
/* Both are plausible -- we need to evaluate the score */
-   int best_score = score_trees(hash1->hash, hash2->hash);
+   int best_score = score_trees(hash1, hash2);
int score;
 
candidate = 0;
-   score = score_trees(sub1.hash, hash2->hash);
+   score = score_trees(, hash2);
if (score > best_score) {
candidate = 1;
best_score = score;
}
-   score = score_trees(sub2.hash, hash1->hash);
+   score = score_trees(, hash1);
if (score > best_score)
candidate = 2;

Re: Migrating away from SHA-1?

[brian m. carlson]

On Tue, Apr 12, 2016 at 06:58:10PM -0700, H. Peter Anvin wrote:
> On April 12, 2016 6:51:12 PM PDT, Duy Nguyen <pclo...@gmail.com> wrote:
> >On Wed, Apr 13, 2016 at 5:38 AM, H. Peter Anvin <h...@zytor.com> wrote:
> >> OK, I'm going to open this can of worms...
> >>
> >> At what point do we migrate from SHA-1?
> >
> >Brian Carlson has been slowly refactoring git code base, abstracting
> >SHA-1 away. Once that work is done, I think we can talk about moving
> >away from SHA-1. The process is slow because it likely causes
> >conflicts with in-flight topics. A quick grep shows we still have
> >about 300 SHA-1 references, so it'll be quite some time.
> 
> Well, at least it sounds like work is underway.  That is a big deal.

Yes, it's a bunch of slow manual refactoring, and I've been busy as
we've been doing house- and car-related things recently.  I'll try to
spend a little more time on it this weekend.

The first step is to convert all of the individual places that use
unsigned char [20] to use struct object_id, which can then be extended
to use different hash algorithms.  There are also constants,
GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ, that abstract the 20 and 40 values in
the codebase so they can be changed in the future.

While this is a project I've been mostly working on, I have no objection
to other people sending in a patch or series as they feel like it.
-- 
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

[PATCH 2/6] test-match-trees: convert to use struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 test-match-trees.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/test-match-trees.c b/test-match-trees.c
index 4dad7095..41aff841 100644
--- a/test-match-trees.c
+++ b/test-match-trees.c
@@ -3,24 +3,24 @@
 
 int main(int ac, char **av)
 {
-   unsigned char hash1[20], hash2[20], shifted[20];
+   struct object_id hash1, hash2, shifted;
struct tree *one, *two;
 
setup_git_directory();
 
-   if (get_sha1(av[1], hash1))
+   if (get_oid(av[1], ))
die("cannot parse %s as an object name", av[1]);
-   if (get_sha1(av[2], hash2))
+   if (get_oid(av[2], ))
die("cannot parse %s as an object name", av[2]);
-   one = parse_tree_indirect(hash1);
+   one = parse_tree_indirect(hash1.hash);
if (!one)
die("not a tree-ish %s", av[1]);
-   two = parse_tree_indirect(hash2);
+   two = parse_tree_indirect(hash2.hash);
if (!two)
die("not a tree-ish %s", av[2]);
 
-   shift_tree(one->object.oid.hash, two->object.oid.hash, shifted, -1);
-   printf("shifted: %s\n", sha1_to_hex(shifted));
+   shift_tree(one->object.oid.hash, two->object.oid.hash, shifted.hash, 
-1);
+   printf("shifted: %s\n", oid_to_hex());
 
exit(0);
 }
-- 
2.8.0.rc3.226.g39d4020

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

[PATCH 0/6] object_id Part 3

[brian m. carlson]

This is the third of a series of patches to convert instances of
unsigned char[20] to struct object_id.  The focus in this series was to
convert test-match-trees, related functions, and some dependencies.
struct name_entry was converted as part of these dependencies.

The riskiest part of this series is the conversion of struct name_entry.
Compatibility with unconverted code requires dereferencing the new oid
member, but there are at least some places where we explicitly check
that it is not NULL.  These seem to be limited to empty entries in
merge-tree.c, and it doesn't appear that we ever pass an empty entry to
a function which might dereference it.

This series rebases onto next cleanly and is not likely to conflict with
other topics in flight.  My intention is to send smaller series more
frequently, with the goal of avoiding or minimizing conflicts where
possible.

brian m. carlson (6):
  Introduce a get_oid function.
  test-match-trees: convert to use struct object_id
  match-trees: convert shift_tree and shift_tree_by to object_id
  Convert struct name_entry to use struct object_id.
  tree-walk: convert tree_entry_extract to struct object_id
  match-trees: convert several leaf functions to struct object_id

 builtin/grep.c |  6 ++---
 builtin/merge-tree.c   | 18 +++
 builtin/pack-objects.c |  4 ++--
 builtin/reflog.c   |  4 ++--
 cache-tree.c   |  4 ++--
 cache.h|  6 +++--
 fsck.c | 10 -
 http-push.c|  4 ++--
 list-objects.c |  6 ++---
 match-trees.c  | 60 +-
 merge-recursive.c  |  4 ++--
 notes.c|  4 ++--
 revision.c |  4 ++--
 sha1_name.c|  9 
 test-match-trees.c | 14 ++--
 tree-diff.c|  8 +++
 tree-walk.c| 16 +++---
 tree-walk.h|  8 +++
 tree.c | 10 -
 unpack-trees.c |  4 ++--
 walker.c   |  4 ++--
 21 files changed, 109 insertions(+), 98 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

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

[PATCH 4/6] Convert struct name_entry to use struct object_id.

[brian m. carlson]

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 builtin/grep.c |  6 +++---
 builtin/merge-tree.c   | 18 +-
 builtin/pack-objects.c |  4 ++--
 builtin/reflog.c   |  4 ++--
 cache-tree.c   |  4 ++--
 fsck.c |  4 ++--
 http-push.c|  4 ++--
 list-objects.c |  6 +++---
 match-trees.c  |  2 +-
 notes.c|  4 ++--
 revision.c |  4 ++--
 tree-diff.c|  6 +++---
 tree-walk.c|  6 +++---
 tree-walk.h|  6 +++---
 tree.c | 10 +-
 unpack-trees.c |  4 ++--
 walker.c   |  4 ++--
 17 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 111b6f6c..462e6079 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -438,7 +438,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.sha1, base->buf, tn_len,
+   hit |= grep_sha1(opt, entry.oid->hash, base->buf, 
tn_len,
 check_attr ? base->buf + tn_len : 
NULL);
}
else if (S_ISDIR(entry.mode)) {
@@ -447,10 +447,10 @@ static int grep_tree(struct grep_opt *opt, const struct 
pathspec *pathspec,
void *data;
unsigned long size;
 
-   data = lock_and_read_sha1_file(entry.sha1, , 
);
+   data = lock_and_read_sha1_file(entry.oid->hash, , 
);
if (!data)
die(_("unable to read tree (%s)"),
-   sha1_to_hex(entry.sha1));
+   oid_to_hex(entry.oid));
 
strbuf_addch(base, '/');
init_tree_desc(, data, size);
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index ca570041..81cced50 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -150,15 +150,15 @@ static void show_result(void)
 /* An empty entry never compares same, not even to another empty entry */
 static int same_entry(struct name_entry *a, struct name_entry *b)
 {
-   return  a->sha1 &&
-   b->sha1 &&
-   !hashcmp(a->sha1, b->sha1) &&
+   return  a->oid &&
+   b->oid &&
+   !oidcmp(a->oid, b->oid) &&
a->mode == b->mode;
 }
 
 static int both_empty(struct name_entry *a, struct name_entry *b)
 {
-   return !(a->sha1 || b->sha1);
+   return !(a->oid || b->oid);
 }
 
 static struct merge_list *create_entry(unsigned stage, unsigned mode, const 
unsigned char *sha1, const char *path)
@@ -188,8 +188,8 @@ static void resolve(const struct traverse_info *info, 
struct name_entry *ours, s
return;
 
path = traverse_path(info, result);
-   orig = create_entry(2, ours->mode, ours->sha1, path);
-   final = create_entry(0, result->mode, result->sha1, path);
+   orig = create_entry(2, ours->mode, ours->oid->hash, path);
+   final = create_entry(0, result->mode, result->oid->hash, path);
 
final->link = orig;
 
@@ -213,7 +213,7 @@ static void unresolved_directory(const struct traverse_info 
*info,
 
newbase = traverse_path(info, p);
 
-#define ENTRY_SHA1(e) (((e)->mode && S_ISDIR((e)->mode)) ? (e)->sha1 : NULL)
+#define ENTRY_SHA1(e) (((e)->mode && S_ISDIR((e)->mode)) ? (e)->oid->hash : 
NULL)
buf0 = fill_tree_descriptor(t+0, ENTRY_SHA1(n + 0));
buf1 = fill_tree_descriptor(t+1, ENTRY_SHA1(n + 1));
buf2 = fill_tree_descriptor(t+2, ENTRY_SHA1(n + 2));
@@ -239,7 +239,7 @@ static struct merge_list *link_entry(unsigned stage, const 
struct traverse_info
path = entry->path;
else
path = traverse_path(info, n);
-   link = create_entry(stage, n->mode, n->sha1, path);
+   link = create_entry(stage, n->mode, n->oid->hash, path);
link->link = entry;
return link;
 }
@@ -314,7 +314,7 @@ static int threeway_callback(int n, unsigned long mask, 
unsigned long dirmask, s
}
 
if (same_entry(entry+0, entry+1)) {
-   if (entry[2].sha1 && !S_ISDIR(entry[2].mode)) {
+   if (entry[2].oid->hash && !S_ISDIR(entry[2].mode)) {
/* We did not touch, they modified -- take theirs */
resolve(info, entry+1, entry+2);
return mask;
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a27de5b3..d56b2c2d 1

[PATCH 3/6] match-trees: convert shift_tree and shift_tree_by to object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 cache.h|  4 ++--
 match-trees.c  | 44 ++--
 merge-recursive.c  |  4 ++--
 test-match-trees.c |  2 +-
 4 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/cache.h b/cache.h
index 22b73646..70091e73 100644
--- a/cache.h
+++ b/cache.h
@@ -1768,8 +1768,8 @@ int add_files_to_cache(const char *prefix, const struct 
pathspec *pathspec, int
 extern int diff_auto_refresh_index;
 
 /* match-trees.c */
-void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, 
int);
-void shift_tree_by(const unsigned char *, const unsigned char *, unsigned char 
*, const char *);
+void shift_tree(const struct object_id *, const struct object_id *, struct 
object_id *, int);
+void shift_tree_by(const struct object_id *, const struct object_id *, struct 
object_id *, const char *);
 
 /*
  * whitespace rules.
diff --git a/match-trees.c b/match-trees.c
index 1ce0954a..9977752a 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -229,9 +229,9 @@ static int splice_tree(const unsigned char *hash1,
  * other hand, it could cover tree one and we might need to pick a
  * subtree of it.
  */
-void shift_tree(const unsigned char *hash1,
-   const unsigned char *hash2,
-   unsigned char *shifted,
+void shift_tree(const struct object_id *hash1,
+   const struct object_id *hash2,
+   struct object_id *shifted,
int depth_limit)
 {
char *add_prefix;
@@ -245,7 +245,7 @@ void shift_tree(const unsigned char *hash1,
if (!depth_limit)
depth_limit = 2;
 
-   add_score = del_score = score_trees(hash1, hash2);
+   add_score = del_score = score_trees(hash1->hash, hash2->hash);
add_prefix = xcalloc(1, 1);
del_prefix = xcalloc(1, 1);
 
@@ -253,16 +253,16 @@ void shift_tree(const unsigned char *hash1,
 * See if one's subtree resembles two; if so we need to prefix
 * two with a few fake trees to match the prefix.
 */
-   match_trees(hash1, hash2, _score, _prefix, "", depth_limit);
+   match_trees(hash1->hash, hash2->hash, _score, _prefix, "", 
depth_limit);
 
/*
 * See if two's subtree resembles one; if so we need to
 * pick only subtree of two.
 */
-   match_trees(hash2, hash1, _score, _prefix, "", depth_limit);
+   match_trees(hash2->hash, hash1->hash, _score, _prefix, "", 
depth_limit);
 
/* Assume we do not have to do any shifting */
-   hashcpy(shifted, hash2);
+   oidcpy(shifted, hash2);
 
if (add_score < del_score) {
/* We need to pick a subtree of two */
@@ -271,16 +271,16 @@ void shift_tree(const unsigned char *hash1,
if (!*del_prefix)
return;
 
-   if (get_tree_entry(hash2, del_prefix, shifted, ))
+   if (get_tree_entry(hash2->hash, del_prefix, shifted->hash, 
))
die("cannot find path %s in tree %s",
-   del_prefix, sha1_to_hex(hash2));
+   del_prefix, oid_to_hex(hash2));
return;
}
 
if (!*add_prefix)
return;
 
-   splice_tree(hash1, add_prefix, hash2, shifted);
+   splice_tree(hash1->hash, add_prefix, hash2->hash, shifted->hash);
 }
 
 /*
@@ -288,44 +288,44 @@ void shift_tree(const unsigned char *hash1,
  * Unfortunately we cannot fundamentally tell which one to
  * be prefixed, as recursive merge can work in either direction.
  */
-void shift_tree_by(const unsigned char *hash1,
-  const unsigned char *hash2,
-  unsigned char *shifted,
+void shift_tree_by(const struct object_id *hash1,
+  const struct object_id *hash2,
+  struct object_id *shifted,
   const char *shift_prefix)
 {
-   unsigned char sub1[20], sub2[20];
+   struct object_id sub1, sub2;
unsigned mode1, mode2;
unsigned candidate = 0;
 
/* Can hash2 be a tree at shift_prefix in tree hash1? */
-   if (!get_tree_entry(hash1, shift_prefix, sub1, ) &&
+   if (!get_tree_entry(hash1->hash, shift_prefix, sub1.hash, ) &&
S_ISDIR(mode1))
candidate |= 1;
 
/* Can hash1 be a tree at shift_prefix in tree hash2? */
-   if (!get_tree_entry(hash2, shift_prefix, sub2, ) &&
+   if (!get_tree_entry(hash2->hash, shift_prefix, sub2.hash, ) &&
S_ISDIR(mode2))
candidate |= 2;
 
if (candidate == 3) {
/* Both are plausible -- we need to evaluate the score */
-   int best_score = score_trees(hash1, hash2);
+   int best_score = score_trees(hash1->hash, hash2->hash);
 

Re: Default authentication over https?

[brian m. carlson]

On Fri, Apr 15, 2016 at 06:43:35PM -0400, Jeff King wrote:
> Hmm. Looks like we already pull this out of the curl result for other
> reasons, but we never feed it back in to the next request. So if I do
> this:
> 
> diff --git a/http.c b/http.c
> index 9bedad7..add9bf2 100644
> --- a/http.c
> +++ b/http.c
> @@ -1132,6 +1132,8 @@ static int handle_curl_result(struct slot_results 
> *results)
>   return HTTP_NOAUTH;
>   } else {
>  #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> + if (results->auth_avail)
> + http_auth_methods = results->auth_avail;
>   http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
>  #endif
>   return HTTP_REAUTH;
> 
> that drops my test case down to two requests: once to find out that we
> need auth via the 401, and then we feed curl sufficient information to
> do the followup in a single request (the GSSNEGOTIATE thing there is a
> hack from 4dbe664, which we can ignore for now).
> 
> Interestingly, curl _does_ reuse the connection this time. I'm still not
> sure why it didn't in the original case. But this means the whole thing
> is happening over a single TCP session, which is good (and I didn't have
> to change my config at all).

This looks fine from my point of view.  GSS-Negotiate and Digest are
going to require at least one round-trip because they need the token or
nonce to authenticate.  Basic shouldn't, 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

[PATCH 1/6] Introduce a get_oid function.

[brian m. carlson]

The get_oid function is equivalent to the get_sha1 function, but uses a
struct object_id instead.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 cache.h | 2 ++
 sha1_name.c | 9 +
 2 files changed, 11 insertions(+)

diff --git a/cache.h b/cache.h
index 2711048c..22b73646 100644
--- a/cache.h
+++ b/cache.h
@@ -1156,6 +1156,8 @@ extern int get_sha1_blob(const char *str, unsigned char 
*sha1);
 extern void maybe_die_on_misspelt_object_name(const char *name, const char 
*prefix);
 extern int get_sha1_with_context(const char *str, unsigned flags, unsigned 
char *sha1, struct object_context *orc);
 
+extern int get_oid(const char *str, struct object_id *oid);
+
 typedef int each_abbrev_fn(const unsigned char *sha1, void *);
 extern int for_each_abbrev(const char *prefix, each_abbrev_fn, void *);
 
diff --git a/sha1_name.c b/sha1_name.c
index 776101e8..ca7ddd6f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1215,6 +1215,15 @@ int get_sha1(const char *name, unsigned char *sha1)
 }
 
 /*
+ * This is like "get_sha1()", but for struct object_id.
+ */
+int get_oid(const char *name, struct object_id *oid)
+{
+   return get_sha1(name, oid->hash);
+}
+
+
+/*
  * Many callers know that the user meant to name a commit-ish by
  * syntactical positions where the object name appears.  Calling this
  * function allows the machinery to disambiguate shorter-than-unique
-- 
2.8.0.rc3.226.g39d4020

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

[PATCH 6/6] match-trees: convert several leaf functions to struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 match-trees.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/match-trees.c b/match-trees.c
index 8ca7c68f..396b7338 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -48,17 +48,17 @@ static int score_matches(unsigned mode1, unsigned mode2, 
const char *path)
 }
 
 static void *fill_tree_desc_strict(struct tree_desc *desc,
-  const unsigned char *hash)
+  const struct object_id *hash)
 {
void *buffer;
enum object_type type;
unsigned long size;
 
-   buffer = read_sha1_file(hash, , );
+   buffer = read_sha1_file(hash->hash, , );
if (!buffer)
-   die("unable to read tree (%s)", sha1_to_hex(hash));
+   die("unable to read tree (%s)", oid_to_hex(hash));
if (type != OBJ_TREE)
-   die("%s is not a tree", sha1_to_hex(hash));
+   die("%s is not a tree", oid_to_hex(hash));
init_tree_desc(desc, buffer, size);
return buffer;
 }
@@ -73,7 +73,7 @@ static int base_name_entries_compare(const struct name_entry 
*a,
 /*
  * Inspect two trees, and give a score that tells how similar they are.
  */
-static int score_trees(const unsigned char *hash1, const unsigned char *hash2)
+static int score_trees(const struct object_id *hash1, const struct object_id 
*hash2)
 {
struct tree_desc one;
struct tree_desc two;
@@ -119,8 +119,8 @@ static int score_trees(const unsigned char *hash1, const 
unsigned char *hash2)
 /*
  * Match one itself and its subtrees with two and pick the best match.
  */
-static void match_trees(const unsigned char *hash1,
-   const unsigned char *hash2,
+static void match_trees(const struct object_id *hash1,
+   const struct object_id *hash2,
int *best_score,
char **best_match,
const char *base,
@@ -138,7 +138,7 @@ static void match_trees(const unsigned char *hash1,
elem = tree_entry_extract(, , );
if (!S_ISDIR(mode))
goto next;
-   score = score_trees(elem->hash, hash2);
+   score = score_trees(elem, hash2);
if (*best_score < score) {
free(*best_match);
*best_match = xstrfmt("%s%s", base, path);
@@ -146,7 +146,7 @@ static void match_trees(const unsigned char *hash1,
}
if (recurse_limit) {
char *newbase = xstrfmt("%s%s/", base, path);
-   match_trees(elem->hash, hash2, best_score, best_match,
+   match_trees(elem, hash2, best_score, best_match,
newbase, recurse_limit - 1);
free(newbase);
}
@@ -245,7 +245,7 @@ void shift_tree(const struct object_id *hash1,
if (!depth_limit)
depth_limit = 2;
 
-   add_score = del_score = score_trees(hash1->hash, hash2->hash);
+   add_score = del_score = score_trees(hash1, hash2);
add_prefix = xcalloc(1, 1);
del_prefix = xcalloc(1, 1);
 
@@ -253,13 +253,13 @@ void shift_tree(const struct object_id *hash1,
 * See if one's subtree resembles two; if so we need to prefix
 * two with a few fake trees to match the prefix.
 */
-   match_trees(hash1->hash, hash2->hash, _score, _prefix, "", 
depth_limit);
+   match_trees(hash1, hash2, _score, _prefix, "", depth_limit);
 
/*
 * See if two's subtree resembles one; if so we need to
 * pick only subtree of two.
 */
-   match_trees(hash2->hash, hash1->hash, _score, _prefix, "", 
depth_limit);
+   match_trees(hash2, hash1, _score, _prefix, "", depth_limit);
 
/* Assume we do not have to do any shifting */
oidcpy(shifted, hash2);
@@ -309,16 +309,16 @@ void shift_tree_by(const struct object_id *hash1,
 
if (candidate == 3) {
/* Both are plausible -- we need to evaluate the score */
-   int best_score = score_trees(hash1->hash, hash2->hash);
+   int best_score = score_trees(hash1, hash2);
int score;
 
candidate = 0;
-   score = score_trees(sub1.hash, hash2->hash);
+   score = score_trees(, hash2);
if (score > best_score) {
candidate = 1;
best_score = score;
}
-   score = score_trees(sub2.hash, hash1->hash);
+   score = score_trees(, hash1);
if (score > best_score)
candidate = 2;

[PATCH 5/6] tree-walk: convert tree_entry_extract to struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 fsck.c|  6 +++---
 match-trees.c | 12 ++--
 tree-diff.c   |  2 +-
 tree-walk.c   | 10 +-
 tree-walk.h   |  4 ++--
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/fsck.c b/fsck.c
index 606eba8c..92b17f5d 100644
--- a/fsck.c
+++ b/fsck.c
@@ -450,11 +450,11 @@ static int fsck_tree(struct tree *item, struct 
fsck_options *options)
while (desc.size) {
unsigned mode;
const char *name;
-   const unsigned char *sha1;
+   const struct object_id *oid;
 
-   sha1 = tree_entry_extract(, , );
+   oid = tree_entry_extract(, , );
 
-   has_null_sha1 |= is_null_sha1(sha1);
+   has_null_sha1 |= is_null_oid(oid);
has_full_path |= !!strchr(name, '/');
has_empty_name |= !*name;
has_dot |= !strcmp(name, ".");
diff --git a/match-trees.c b/match-trees.c
index 751f8f20..8ca7c68f 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -131,14 +131,14 @@ static void match_trees(const unsigned char *hash1,
 
while (one.size) {
const char *path;
-   const unsigned char *elem;
+   const struct object_id *elem;
unsigned mode;
int score;
 
elem = tree_entry_extract(, , );
if (!S_ISDIR(mode))
goto next;
-   score = score_trees(elem, hash2);
+   score = score_trees(elem->hash, hash2);
if (*best_score < score) {
free(*best_match);
*best_match = xstrfmt("%s%s", base, path);
@@ -146,7 +146,7 @@ static void match_trees(const unsigned char *hash1,
}
if (recurse_limit) {
char *newbase = xstrfmt("%s%s/", base, path);
-   match_trees(elem, hash2, best_score, best_match,
+   match_trees(elem->hash, hash2, best_score, best_match,
newbase, recurse_limit - 1);
free(newbase);
}
@@ -191,15 +191,15 @@ static int splice_tree(const unsigned char *hash1,
while (desc.size) {
const char *name;
unsigned mode;
-   const unsigned char *sha1;
+   const struct object_id *oid;
 
-   sha1 = tree_entry_extract(, , );
+   oid = tree_entry_extract(, , );
if (strlen(name) == toplen &&
!memcmp(name, prefix, toplen)) {
if (!S_ISDIR(mode))
die("entry %s in tree %s is not a tree",
name, sha1_to_hex(hash1));
-   rewrite_here = (unsigned char *) sha1;
+   rewrite_here = (unsigned char *) oid->hash;
break;
}
update_tree_entry();
diff --git a/tree-diff.c b/tree-diff.c
index 402f9ff2..ff4e0d3c 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -183,7 +183,7 @@ static struct combine_diff_path *emit_path(struct 
combine_diff_path *p,
 
if (t) {
/* path present in resulting tree */
-   sha1 = tree_entry_extract(t, , );
+   sha1 = tree_entry_extract(t, , )->hash;
pathlen = tree_entry_len(>entry);
isdir = S_ISDIR(mode);
} else {
diff --git a/tree-walk.c b/tree-walk.c
index fab57dd5..ce278424 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -433,10 +433,10 @@ static int find_tree_entry(struct tree_desc *t, const 
char *name, unsigned char
int namelen = strlen(name);
while (t->size) {
const char *entry;
-   const unsigned char *sha1;
+   const struct object_id *oid;
int entrylen, cmp;
 
-   sha1 = tree_entry_extract(t, , mode);
+   oid = tree_entry_extract(t, , mode);
entrylen = tree_entry_len(>entry);
update_tree_entry(t);
if (entrylen > namelen)
@@ -447,7 +447,7 @@ static int find_tree_entry(struct tree_desc *t, const char 
*name, unsigned char
if (cmp < 0)
break;
if (entrylen == namelen) {
-   hashcpy(result, sha1);
+   hashcpy(result, oid->hash);
return 0;
}
if (name[entrylen] != '/')
@@ -455,10 +455,10 @@ static int find_tree_entry(struct tree_desc *t, const 
char *name, unsigned char
if (!S_ISDIR(*mode))
break;
if (++entrylen == namelen) {
-   hashcpy(result, sha1

Re: [PATCH 4/6] Convert struct name_entry to use struct object_id.

[brian m. carlson]

On Tue, Apr 19, 2016 at 04:02:22PM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sand...@crustytoothpaste.net> writes:
> 
> > @@ -314,7 +314,7 @@ static int threeway_callback(int n, unsigned long mask, 
> > unsigned long dirmask, s
> > }
> >  
> > if (same_entry(entry+0, entry+1)) {
> > -   if (entry[2].sha1 && !S_ISDIR(entry[2].mode)) {
> > +   if (entry[2].oid->hash && !S_ISDIR(entry[2].mode)) {
> 
> Thanks for a warning in the cover letter.
> 
> "if (entry[2].oid && !S_ISDIR(entry[2].mode)" would be a faithful
> conversion, wouldn't it?

Yes, I think that would be a better conversion.  I'll reroll after
waiting for further comments.
-- 
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 1/2] pager: move pager-specific setup into the build

[brian m. carlson]

On Mon, Aug 01, 2016 at 01:05:56AM +, Eric Wong wrote:
> +static void setup_pager_env(struct argv_array *env)
> +{
> + const char *pager_env = stringify(PAGER_ENV);
> +
> + while (*pager_env) {
> + struct strbuf buf = STRBUF_INIT;
> + const char *cp = strchrnul(pager_env, '=');
> +
> + if (!*cp)
> + die("malformed build-time PAGER_ENV");
> + strbuf_add(, pager_env, cp - pager_env);
> + cp = strchrnul(pager_env, ' ');
> + if (!getenv(buf.buf)) {
> + strbuf_reset();
> + strbuf_add(, pager_env, cp - pager_env);
> + argv_array_push(env, strbuf_detach(, NULL));
> + }
> + strbuf_reset();
> + while (*cp && *cp == ' ')
> + cp++;
> + pager_env = cp;
> + }

So it looks like this function splits on spaces but doesn't provide any
escaping mechanism.  Is there any case in which we want to accept
environment variables containing whitespace?  I ask this as someone that
has EDITOR set to "gvim -f" on occasion and seeing how tools sometimes
handle that poorly.

Even without that, I think this series is probably an improvement over
the status quo.
-- 
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 1/2] pager: move pager-specific setup into the build

[brian m. carlson]

On Mon, Aug 01, 2016 at 10:57:02AM +0200, Jakub Narębski wrote:
> W dniu 01.08.2016 o 09:00, Eric Wong pisze:
> > "brian m. carlson" <sand...@crustytoothpaste.net> wrote:
> >> So it looks like this function splits on spaces but doesn't provide any
> >> escaping mechanism.  Is there any case in which we want to accept
> >> environment variables containing whitespace?  I ask this as someone that
> >> has EDITOR set to "gvim -f" on occasion and seeing how tools sometimes
> >> handle that poorly.
> 
> This is to handle environment variables holding program options,
> which are usually (but possibly not often) using single character
> options bundled together, that is, not using spaces.
> 
> Moreover, it is about holding program options to pager.

I understand that.  My point is that we should consider corner cases
like how we're going to handle spaces.

> > Yes, it's only split on spaces right now.  While I don't think
> > there's any current case where spaces would be useful/desirable;
> > I suppose a 3rd patch in this series could add support for using
> > split_cmdline (from alias.c)...
> 
> Is there any pager that needs spaces in options-set environment
> variable?  Does MORE allow option bundling?

We seem to accept GIT_PAGER="par | less" and par definitely accepts
spaces in its environment variables.  That seems to be a corner case,
though, and I haven't seen par practically used in years.

We may also want to consider EXINIT for people who pipe to vi.  Again,
I'm not sure this is very common; most people would use an .exrc or
.vimrc.

I'd say if we can't come up with any better examples, I'd skip handling
it for now.  I'll try to come up with a patch to add it later.
-- 
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] config.mak.uname: set PERL_PATH for FreeBSD 5.0+

[brian m. carlson]

On Wed, Jul 20, 2016 at 11:10:40AM -0700, Junio C Hamano wrote:
> On Wed, Jul 20, 2016 at 11:07 AM, Eric Wong <e...@80x24.org> wrote:
> > Also, my use of a numeric comparison may be more future-proof
> > in case FreeBSD decides to have /usr/bin/perl again.
> >
> > I also wonder why we don't use `which` to search for somewhat
> > standard path components, instead.  Something like:
> 
> Because historically output from "which" was not meant to be
> machine parseable (some implementation said 'perl is /usr/bin/perl')

The POSIXy way to write which is "command -v", which may or may not be
more portable.  It does have the desired output, 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: Git and SHA-1 security (again)

[brian m. carlson]

On Sun, Jul 17, 2016 at 10:01:38AM +0200, Johannes Schindelin wrote:
> Out of curiosity: have you considered something like padding the SHA-1s
> with, say 0xa1, to the size of the new hash and using that padding to
> distinguish between old vs new hash?

I'm going to end up having to do something similar because of the issue
of submodules.  Submodules may still be SHA-1, while the main repo may
be a newer hash.  I was going to zero-pad, however.  I was also, at
least at first, going to force a separate .git dir for those, to avoid
having to try to store two separate types of objects in the same repo.

The other limitation with this is that it isn't immediately obvious what
hash is in use just because it has a certain length.  For example, I
plan on implementing SHA3-256, but it's also possible I might add
BLAKE2b-256 for people for whom SHA3-256 is too slow.  There's no way to
distinguish between those two algorithms.  Thus allowing multiple hashes
in the same repo won't work without a format byte.

What I might do, however, is add multihash-style format information to
the on-disk format for non-SHA-1 repos.  Then SHA-1 compatibility could
come in a future iteration.  That would be compatible with the existing
refactor.

> I guess that it would also possible to introduce an opt-in "legacy mapper"
> which would generate a mapping locally of all objects' SHA-1 to whatever
> new hash you choose. Generating it locally would side-step the security
> issues of the SHA-1 algorithm. We would need to teach Git to pick that
> mapping up if available and use it, of course.

I think that might be easier.  Considering the number of tests that
hard-code object names, I might need that for the testsuite.
-- 
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: Git and SHA-1 security (again)

[brian m. carlson]

On Sun, Jul 17, 2016 at 05:19:02PM +0200, Duy Nguyen wrote:
> On Sun, Jul 17, 2016 at 4:21 PM, brian m. carlson
> <sand...@crustytoothpaste.net> wrote:
> > On Sun, Jul 17, 2016 at 10:01:38AM +0200, Johannes Schindelin wrote:
> >> Out of curiosity: have you considered something like padding the SHA-1s
> >> with, say 0xa1, to the size of the new hash and using that padding to
> >> distinguish between old vs new hash?
> >
> > I'm going to end up having to do something similar because of the issue
> > of submodules.  Submodules may still be SHA-1, while the main repo may
> > be a newer hash.  I was going to zero-pad, however.  I was also, at
> > least at first, going to force a separate .git dir for those, to avoid
> > having to try to store two separate types of objects in the same repo.
> 
> If it's just the external hash representation, can we go with a prefix
>  to identify the hash algorithm? For example
> sha256:1234... is SHA-256 while 1235... by default is SHA-1 (but we
> could switch the default to SHA-256 via config file later SHA-1 is
> dead and nobody wants to type sha256: every time). It catches
> incorrect hash algorithm references.

I'd make it such that the default is that of the repo.  If the current
repo is generating SHA-256, say, then 473a0f4 refers to the empty blob.
If you want to refer to an SHA-1 object, then you write sha-1:e69de29.

On disk, multihash[0] seems like the right way to go.  We'd serialize
references to the SHA-1 and SHA-256 empty blobs as
1114e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 and
1220473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813
respectively.  This makes parsing significantly easier.  On disk, we
could write them into the object database as 1114e6/9de2… and
122047/3a0f….

We could implement the default hash algorithm as extensions.hash and the
on-disk format (which would be a requirement for extensions.hash) as
extensions.explicitHash.

As I said, I'm not planning on multiple hash support at first, but it
doesn't appear impossible if we go this route.  We might still have to
rewrite objects, but we can verify signatures over the legacy SHA-1
objects by forcing them into the old-style object format.

[0] https://github.com/jbenet/multihash
-- 
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: Git and SHA-1 security (again)

[brian m. carlson]

On Sun, Jul 17, 2016 at 12:23:49PM -0400, Theodore Ts'o wrote:
> On Sun, Jul 17, 2016 at 03:42:34PM +0000, brian m. carlson wrote:
> > As I said, I'm not planning on multiple hash support at first, but it
> > doesn't appear impossible if we go this route.  We might still have to
> > rewrite objects, but we can verify signatures over the legacy SHA-1
> > objects by forcing them into the old-style object format.
> 
> How hard would it be to make the on-disk format be multihash, even if
> there is no support for anything other than a single hash, at least
> for now?  That way we won't have to rewrite the objects twice.

Other than the amount of work to change reading from the on-disk format,
nothing prevents us from doing that, although I would recommend storing
the object database with the tag prefix if we do so (i.e., instead of
.git/objects/17, writing .git/objects/111417).  That future-proofs us
for when we change the hash.

I will say that the pack format will likely require some changes,
because it assumes things are 4-byte aligned.  It also assumes you can
use the object ID in the mmaped pack directly (4-byte aligned), which
you can no longer do.  We have some cases where we cast that memory
directly to struct object_id, which will no longer be valid, and even if
we add the two prefix bytes to struct object_id, that doesn't guarantee
that struct won't be aligned differently.

We could require that the pack format have two NUL bytes before the
hash, which would force it to be aligned.  We'd still have to make the
Git protocol negotiate the new extension and fail gracefully if the
version is too old.  We could do this by requiring a pack version 5,
which would simply cause older Gits to report errors.

It's a lot of work, and it's definitely a flag day.  That's why I had
planned to only do it with a new hash format: it would impact only
people who were moving to the new hash.  It also means that we get to
work out any problems with the design at that point and not be committed
to a design that might be inadequate.  This is a place where I don't
want to mess up.

> Personally, so long as the newer versions of the tree are secured, I
> wouldn't mind if the older commits stayed using SHA1 only.  The newer
> commits are the ones that are most important and security-critical
> anyway.  It seems like the main reason to rewrite all of the objects
> is to simplify the initial rollout of a newer hash algorithm, no?

The reason is that we can't have an unambiguous parse of the current
objects if two hash algorithms are in use.  tree objects don't use a hex
encoding of hashes; they use a binary encoding.  It's therefore possible
to create an ambiguous tree representation.

So when we look at a new hash, we need to provide an unambiguous way to
know what hash is in use.  The two choices are to either require all
object use the new hash, or to extend the objects to include the hash.
Until a couple days ago, I had planned to do the former.  I had not even
considered using a multihash approach due to the complexity.
-- 
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: Git and SHA-1 security (again)

[brian m. carlson]

On Mon, Jul 18, 2016 at 09:00:06AM +0200, Johannes Schindelin wrote:
> Hi Brian,
> 
> On Sun, 17 Jul 2016, brian m. carlson wrote:
> 
> > On Sun, Jul 17, 2016 at 10:01:38AM +0200, Johannes Schindelin wrote:
> > > Out of curiosity: have you considered something like padding the SHA-1s
> > > with, say 0xa1, to the size of the new hash and using that padding to
> > > distinguish between old vs new hash?
> > 
> > I'm going to end up having to do something similar because of the issue
> > of submodules.  Submodules may still be SHA-1, while the main repo may
> > be a newer hash.  I was going to zero-pad, however.
> 
> I thought about zero-padding, but there are plenty of
> is_null_sha1()/is_null_oid() calls around. Of course, I assumed
> left-padding. But you may have thought of right-padding instead? That
> would make short name handling much easier, too.

I was going to right-pad.

> FWIW it never crossed my mind to allow different same-sized hash
> algorithms. So I never thought we'd need a way to distinguish, say,
> BLAKE2b-256 from SHA-256.
> 
> Is there a good reason to add the maintenance burden of several 256-bit
> hash algorithms, apart from speed (which in my mind should decide which
> one to use, always, rather than letting the user choose)? It would also
> complicate transport even further, let alone subtree merges from
> differently-hashed repositories.

There are really three candidates:

* SHA-256 (the SHA-2 algorithm): While this looks good right now,
  cryptanalysis is advancing.  This is not a good choice for a long-term
  solution.
* SHA3-256 (the SHA-3 algorithm): This is the conservative choice.  It's
  also faster than SHA-256 on 64-bit systems.  It has a very
  conservative security margin and is a good long-term choice.
* BLAKE2b-256: This is the blisteringly fast choice.  It outperforms
  SHA-1 and even MD5 on 64-bit systems.  This algorithm was designed so
  that nobody would have a reason to use an insecure algorithm.  It will
  probably be secure for some time, but maybe not as long as SHA3-256.

I'm only considering 256-bit hashes, because anything longer won't fit
on an 80-column terminal in hex form.

The reason I had considered implementing both SHA3-256 and BLAKE2b-256
is that I want there to be no reason not to upgrade.  People who need a
FIPS-approved algorithm or want a long-term, conservative choice should
use SHA3-256.  People who want even better performance than current Git
would use BLAKE2b-256.

Performance comparison (my implementations):
SHA-1: 437 MiB/s
SHA-256:   196 MiB/s
SHA3-256:  273 MiB/s
BLAKE2b:   649 MiB/s

I hadn't thought about subtree merges, 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: Git and SHA-1 security (again)

[brian m. carlson]

On Mon, Jul 18, 2016 at 11:00:35AM -0700, Junio C Hamano wrote:
> Continuing this thought process, I do not see a good way to allow us
> to wean ourselves off of the old hash, unless we _break_ the pack
> stream format so that each object in the pack carries not just the
> data but also the hash algorithm to be used to _name_ it, so that
> new objects will never be referred to using the old hash.

I think for this reason, I'm going to propose the following approach
when we get there:

* We serialize the hash in the object formats, using multihash or
  something similar.  This means that it is minimally painful if we ever
  need to change in the future[0].
* Each repository carries exactly one hash algorithm, except for
  submodule data.  If we don't do this, then some people will never
  switch because the submodules they depend on haven't.
* If people on the new format need to refer to submodule commits using
  SHA-1, then they have to use a prefix on the hash form; otherwise,
  they can use the raw hash value (without any multihash prefix).
* git fsck verifies one consistent algorithm (excepting submodule
  references).

This preserves the security benefits, avoids future-proofing problems,
and minimizes performance impacts due to naming like you mentioned.

[0] We are practically limited to 256-bit hashes because anything longer
will wrap on an 80-column terminal when in hex form.
-- 
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: Git and SHA-1 security (again)

[brian m. carlson]

On Sat, Jul 16, 2016 at 11:46:06PM +0200, Herczeg Zsolt wrote:
> Dear Brian,
> 
> Thank you for your response. It very good to hear that changing the
> hash is on the git project's list. I haven't found any official
> communication on that topic since 2006.

There's been some recent discussion on the list about it.  It is less on
the Git project's list and more on my personal list.  It's my hope that
Junio and other contributors will decide to accept my patches when they
are ready.  Also, the plan is to keep SHA-1 available, probably as the
default, for backwards compatibility.

> I'll look into the contributions guide and the source codes, to check
> if I can contribute to this transition. If you have any documentation
> or other related info, please point me towards it.

The major work at this point is turning instances of unsigned char [20]
into struct object_id, as well as converting hardcoded 20 and 40 (and
derivative values) to GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ.  This work
allows us to make as little code as possible know about the size of the
hash, as well as generally being easier to maintain.

You can look at the bc/cocci branch which was recently merged into next.
(It doesn't exist independently outside of next, so you'll have to
search through the history).  That work is what in my branches is called
object-id-part4.  I'm currently working on getting to the point of
converting get_tree_entry to use struct object_id, which is what will
become my object-id-part5.

I recommend if you're planning on doing some of this work that you try
to avoid areas which are under work by other developers, especially the
refs code, which is undergoing massive changes.  Other people will
appreciate it.
-- 
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: Git and SHA-1 security (again)

[brian m. carlson]

On Sat, Jul 16, 2016 at 03:48:49PM +0200, Herczeg Zsolt wrote:
> But - and that's the main idea i'm writing here - changing the storage
> keys does not mean you should drop your old hashes out. If you change
> the git data structure in a way, that it can keep multiple hashes for
> the same "link" in each objects (trees, commits, etc) you can keep the
> old ones right next to the new one. If you want to look up the
> referenced object, you must use the newest hash - which is the key.
> But if you want to verify some old hash, it's still possible! Just
> look up the objects by the new key, remove all the newer generation
> keys, and verify the old hash on that.
> 
> A storage structure like this would allow a very great flexibility:
>  - You can change your hash algorithm in the future. If SHA-256
> becomes broken, it's not a problem. Just re-hash the storage, and
> append the new hashes the git objects.
>  - You can still verify your old hashes after a hash change - removing
> the new hashes from the objects before hashing should give you back
> the old objects, thus giving you the same hash as before.
>  - That makes possible for signed tags, and commits to keep their
> validity after hash change! With a clever-enough new format, you can
> even keep the validity of current hashes and signs. (To be able to do
> that, you should be able to calculate back the current format from the
> new format.)
> 
> Moving git forward to a format like this would solve the weak-key
> problem in git forever. You would be able to configure your key algo
> on a per repository basis, you - and git - can do the daily work on
> the newest hashes, while still carrying the old hashes and signatures,
> in case you ever want to verify them. That would allow repositories to
> gracefully change hashes in case they need to, and to only
> compatibility limitation is that you must use a new enough git to
> understand the new storage format.
> 
> What are your thoughts on this approach? Will git ever reach a release
> with exchangeable hash algorithm? Or should someone look for
> alternatives if there's a need for cryptographic security?

I'm working on adding new hash algorithm support in Git.  However, it
requires a significant refactor of the code base.  My current plan is
not to implement side-by-side data, for a couple reasons.

One is that it requires significantly more work to implement and
complicates the code.  It's also incompatible with all the refactoring
I've done already.

The second is that it requires that Git have the ability to store
multiple hashes at once, which is very expensive in terms of memory.
Moving from a 160-bit hash to a 256-bit hash (my current plan is
SHA3-256) requires 1.6× the memory.  Storing both requires 2.6× the
memory.  If you add a third hash, it requires even more.  Memory is
often a constraint with using Git.

The current plan is to use git-fast-import and git-fast-export to handle
that conversion process, and then maybe provide wrappers to make it more
transparent.

Currently the process of the refactor is ongoing, but it is a free time
activity for me.

If you'd like to follow the progress roughly, you can do so by checking
the output of the following commands:

  git grep 'unsigned char.*20' | wc -l
  git grep 'struct object_id' | wc -l

You are also welcome to contribute, of course.
-- 
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: Dependencies required for offline installation

[brian m. carlson]

On Tue, Jul 05, 2016 at 09:53:15PM +0200, Dennis Kaarsemaker wrote:
> On di, 2016-07-05 at 14:06 -0400, Kevin Paxton wrote:
> > Thank you for the response.
> > 
> > I apologize. RHEL 6.5, not 5.5.
> 
> That's less ancient, but still not recommended. When using RHEL, try to
> stay with the latest point release so you get security updates.
> 
> > Would the same version be applicable to 6.5 as well as the
> > dependencies that you mentioned?
> 
> Red hat actually ships a version of git with RHEL 6.  So the 
> version will be different (I believe it's a 1.7.something). The
> dependencies should be similar, if not the same.

At $DAYJOB, we build and ship Git with our software, which runs on RHEL
6 and 7.  I'd have to check to be certain, but I'm pretty sure the
dependencies for core Git are the same as Red Hat's version.  We choose
to compile with PCRE because it enables git grep -P, which we use in
development.  If you want git-svn, you'll of course need subversion.

In case you do move back to RHEL 5 (which I don't recommend), be aware
that the libcurl version is old enough that some features (DAV is one,
if I remember correctly) don't work.
-- 
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] config.mak.uname: define NEEDS_LIBRT under Linux, for now

[brian m. carlson]

On Sun, Jul 10, 2016 at 10:16:44PM +, Eric Wong wrote:
> My Debian wheezy LTS system is still on glibc 2.13; and LTS
> distros may use older glibc, still, so lets not unnecessarily
> break things out-of-the-box.
> 
> We seem to assume Linux is using glibc in our Makefiles anyways,
> so I don't think this will introduce new breakage for users of
> alternative libc implementations.

This is a good idea.  If it's broken with Debian wheezy, it will also be
broken for RHEL/CentOS 6.  People who really want the slimmest linkage
can use the -Wl,--as-needed flag.
-- 
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