Re: What's cooking in git.git (May 2017, #08; Mon, 29)

[Stefan Beller]

On Tue, May 30, 2017 at 8:18 PM, Daniel Ferreira (theiostream)
 wrote:
> On Mon, May 29, 2017 at 3:23 AM, Junio C Hamano  wrote:
>> * df/dir-iter-remove-subtree (2017-05-29) 5 commits
>>  . remove_subtree(): reimplement using iterators
>>  . dir_iterator: rewrite state machine model
>>  . dir_iterator: refactor dir_iterator_advance
>>  . remove_subtree(): test removing nested directories
>>  . dir_iterator: add tests for dir_iterator API
>>
>>  Update the dir-iterator API and use it to reimplement
>>  remove_subtree().
>>
>>  GSoC microproject.
>>  Ejected as it conflicts with other topics in flight in a
>>  non-trivial way.
>
> I see this conflicts with Duy's
> fa7e9c0c24637d6b041a2919a33956b68bfd0869 ("files-backend: make reflog
> iterator go through per-worktree reflog", 2017-04-19) because his
> commit creates a new dir_iterator whose NULL value means something
> semantically. This would be perfectly OK with the old dir_iterator API
> (where NULL was not a possible return value from dir_iterator_begin()
> and could be "reserved" for this case), but will most probably
> generate issues with the new API, where NULL can *also* mean we failed
> to lstat() the directory we're trying to iterate over[1].
>
> I'll try to address this issue playing with pu, but I'm just wondering
> what would be the best way to send this upcoming not-based-on-master
> patch to the list. Should I just send it normally and signal it
> originates from pu rather than master?

pu is bad as that contains all series in flight.
If possible take the smallest set of series that this would depend on
(usually it is one in my experience and it seems as if you have identified
that series already) and just base it on top of that.

Thanks,
Stefan

>
> Thanks,
>
> [1]: 
> https://public-inbox.org/git/1493226219-33423-1-git-send-email-bnm...@gmail.com/T/#m68286d783b5dfbad6921fbf012f685a629645c61

Re: [Bug] git branch -v has problems with carriage returns

[Atousa Duprat]

Here is my first attempt at fixing the issue.

There are two problems in ref-filter.c:

First, copy_subject() has been modified to turn '\n' into a space and
every other ascii control character to be ignored.

Second, find_subpos() doesn't realize that a line that only contains a
'\r\n' is a blank line – at least when using crlf convention.
I have changed things so that a sequence of either '\n' or "\r\n"
separate the subject from the body of the commit message.
I am not looking at the crlf setting because it doesn't seem like a
useful distinction – when one would we ever care for \r\n not to be a
blank line?  But it could be done...

Both fixes are minimal, but it feels like they are a issues with the
specific encoding.  Does git mandate ascii or utf-8 commit messages?
If not, there may be a larger issue here with encodings and line-end
conventions at the very least in ref-filter.c
Guidance would be appreciated for how to deal with this issue...

Patch attached.


Atousa


On Fri, May 19, 2017 at 11:48 PM, Johannes Sixt  wrote:
> Am 19.05.2017 um 23:55 schrieb Atousa Duprat:
>>
>> I have tried to repro this issue but git goes out of its way to store
>> the commit messages using unix end-of-line format.
>> I think that git itself cannot create a repo exhibiting this problem.
>
>
> Here is a recipe to reproduce the error:
>
>   git init
>   git commit --allow-empty -m initial
>   git branch crlf $(printf '%s\r\n' subject '' line3_long line4 |
>git commit-tree HEAD:)
>
> The reason for the "bug" is obviously that a line having CR in addition to
> LF is not "an empty line". Consequently, the second line is not treated as a
> separator between subject and body, whereupon Git concatenates all lines
> into one large subject line. This strips the LFs but leaves the CRs in tact,
> which, when printed on a terminal move the cursor to the beginning of the
> line, so that text after the CRs overwrites what is already in the terminal.
>
> This is just to give you a head start. I'm not going to look into this.
>
> -- Hannes
>
>
>>> If I do `git branch -v` with such a subject line somehow the third and
>>> second line get combined before the hash. Example:
>>>
>>> $ git branch -v
>>> See merge request ! temp space 84e18d22fd Merge branch
>>> 'feature-XXX' into 'develop'
>>> #  >> third)>  
>>>
>>> Before git v2.13.0 `git branch -v` worked completely normal.


branch-crlf.patch
Description: Binary data

[PATCH] docs/config: mention protocol implications of url.insteadOf

[Jeff King]

On Fri, May 26, 2017 at 11:22:37AM -0500, Elliott Cable wrote:

> 1. Most simply, better documentation: mention `GIT_PROTOCOL_FROM_USER`
>explicitly in the documentation of/near `insteadOf`, most
>particularly in the README for `contrib/persistent-https`.

I agree that a hint in both places would be helpful.  The patch for that
is below.

> 2. Possibly, special-case “higher-security” porcelain (like
>`git-submodule`, as described in 33cfccbbf3) to ignore `insteadOf`
>rewrite-rules without additional, special configuration. This way,
>`git-submodule` works for ignorant users (like me) out of the box,
>just as it previously did, and there's no possible security
>compramise.

I don't think we can do that. Rewrites of "git://" to "ssh://" are
pretty common (and completely harmless). Besides, I think submodules are
a case where you really would want persistent-https to kick in. IIRC,
the original use case for that helper is Android development, where a
user is likely to update a ton of repositories from the same server all
at once. Right now the fetches are all done individually with the "repo"
tool, but in theory the whole thing could be set up as submodules.

-- >8 --
Subject: [PATCH] docs/config: mention protocol implications of url.insteadOf

If a URL rewrite switches the protocol to something
nonstandard (like "persistent-https" for "https"), the user
may be bitten by the fact that the default protocol
restrictions are different between the two. Let's drop a
note in insteadOf that points the user in the right
direction.

It would be nice if we could make this work out of the box,
but we can't without knowing the security implications of
the user's rewrite. Only the documentation for a particular
remote helper can advise one way or the other. Since we do
include the persistent-https helper in contrib/ (and since
it was the helper in the real-world case that inspired that
patch), let's also drop a note there.

Suggested-by: Elliott Cable 
Signed-off-by: Jeff King 
---
 Documentation/config.txt|  7 +++
 contrib/persistent-https/README | 10 ++
 2 files changed, 17 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 43d830ee3..5218ecd37 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3235,6 +3235,13 @@ url..insteadOf::
the best alternative for the particular user, even for a
never-before-seen repository on the site.  When more than one
insteadOf strings match a given URL, the longest match is used.
++
+Note that any protocol restrictions will be applied to the rewritten
+URL. If the rewrite changes the URL to use a custom protocol or remote
+helper, you may need to adjust the `protocol.*.allow` config to permit
+the request.  In particular, protocols you expect to use for submodules
+must be set to `always` rather than the default of `user`. See the
+description of `protocol.allow` above.
 
 url..pushInsteadOf::
Any URL that starts with this value will not be pushed to;
diff --git a/contrib/persistent-https/README b/contrib/persistent-https/README
index f784dd2e6..7c4cd8d25 100644
--- a/contrib/persistent-https/README
+++ b/contrib/persistent-https/README
@@ -35,6 +35,16 @@ to use persistent-https:
 [url "persistent-http"]
insteadof = http
 
+You may also want to allow the use of the persistent-https helper for
+submodule URLs (since any https URLs pointing to submodules will be
+rewritten, and Git's out-of-the-box defaults forbid submodules from
+using unknown remote helpers):
+
+[protocol "persistent-https"]
+   allow = always
+[protocol "persistent-http"]
+   allow = always
+
 
 #
 # BUILDING FROM SOURCE
-- 
2.13.0.678.ga17378094

Re: persistent-https, url insteadof, and `git submodule`

[Jeff King]

On Fri, May 26, 2017 at 11:22:37AM -0500, Elliott Cable wrote:

> Hi! Thanks for the responses (I hope reply-all isn't bad mailing-list
> etiquette? Feel free to yell at with a direct reply!). For whatever it's
> worth, as a random user, here's my thoughts:

No, reply-all is the preferred method on this list.

> > The other approach is to declare that a url rewrite resets the
> > protocol-from-user flag to 1. IOW, since the "persistent-https" protocol
> > comes from our local config, it's not dangerous and we should behave as
> > if the user themselves gave it to us. That makes Elliott's case work out
> > of the box.
> 
> Well, now that I'm aware of security concerns, `GIT_PROTOCOL_FROM_USER`
> and `GIT_ALLOW_PROTOCOL`, and so on, I wouldn't *at all* expect
> `insteadOf` to disable that behaviour. Instead, one of two things seems
> like a more ideal solution:
> 
> 1. Most simply, better documentation: mention `GIT_PROTOCOL_FROM_USER`
>explicitly in the documentation of/near `insteadOf`, most
>particularly in the README for `contrib/persistent-https`.
> 
> 2. Possibly, special-case “higher-security” porcelain (like
>`git-submodule`, as described in 33cfccbbf3) to ignore `insteadOf`
>rewrite-rules without additional, special configuration. This way,
>`git-submodule` works for ignorant users (like me) out of the box,
>just as it previously did, and there's no possible security
>compramise.

After my other email, I was all set to write a patch to set
"from_user=1" when we rewrite a URL. But I think it actually is a bit
risky, because we don't know which parts of the URL are
security-sensitive versus which parts were rewritten. A modification of
a tainted string doesn't necessarily untaint it (but sometimes it does,
as in your case).

We could actually have a flag as part of the rewrite config, like:

  [url "persistent-https"]
  insteadOf = "https"
  untaint = true

but I don't think that really buys anything. If you know about the
problem, you could just as easily do:

  [url "persistent-https"]
  insteadOf = "https"
  [protocol "persistent-https"]
  allow = always

It really is an issue of the user knowing about the problem (and how to
solve it), and I don't think we can get around that securely. So better
documentation probably is the right solution.

I'll see if I can cook something up.

-Peff

Re: [Non-Bug] cloning a repository with a default MASTER branch tries to check out the master branch

[Jeff King]

On Sun, May 28, 2017 at 12:21:50PM +0100, Philip Oakley wrote:

> > Are you aware of the symref capability that is already advertised in
> > the initial upload-pack response?  Right now, we do so only when
> > HEAD actually points at something, and the earlier suggestion by
> > Peff is to do so unconditionally, even when HEAD is dangling.
> 
> The suggestion is the otherway around. I would argue (as a viewpoint) that
> what we advertise are object IDs and their associated refs, sorted by ref
> name. (I'm thinking of the git/Documentation/technical/pack-protocol.txt
> here). My suggestion was that when we get to the sorted ref that HEAD points
> to (including the unborn oid) that we annotate that ref.

It's hard to do that in a backwards-compatible way. The reason the
symref capability works like it does (and supports only HEAD) is that
the bits after the NUL are treated as a capability string, and older
clients will actually _replace_ any earlier capability string they saw
with the new one. So:

  1234abcd refs/heads/a\0symref=refs/heads/target1
  5678abcd refs/heads/b\0symref=refs/heads/target2

doesn't work as you'd like.

But even if it did, I don't think that solves the dangling symref
problem by itself. As you note, we'd need to advertise a null sha1 line
and annotate that. I didn't test, but I'd suspect that's another
compatibility problem; clients will probably choke on the null sha1.

> I didn't quite follow Peff's suggestion as to where the list change
> went and if that was a protocol change.

The current protocol just advertises symref=source:target in the
capability line, where "source" does not have to be the ref that is
currently being advertised at all. So you are free to do:

  1234abcd refs/heads/unrelated\0...symref=HEAD:refs/heads/target

But the server does not bother to do so when the HEAD symref doesn't
point to anything. We could fix that without changing the protocol
syntax. It's possible that some picky client would complain that we
mentioned the HEAD symref even though it was not advertised, but
certainly older git.git clients are fine with it. They'd still need a
client-side update in order to do something useful with it, but at least
its presence would not make older clients behave any worse.

-Peff

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Christian Couder]

On Tue, May 30, 2017 at 11:21 PM, Ben Peart  wrote:
>
> On 5/30/2017 9:18 AM, Christian Couder wrote:
>>
>> On Thu, May 25, 2017 at 8:36 PM, Ben Peart  wrote:
>>
>>> +   printf "untracked\0"
>>> +   printf "dir1/untracked\0"
>>> +   printf "dir2/untracked\0"
>>> +   printf "modified\0"
>>> +   printf "dir1/modified\0"
>>> +   printf "dir2/modified\0"
>>> +   printf "new\0""
>>> +   printf "dir1/new\0"
>>> +   printf "dir2/new\0"
>>
>> Maybe something like the following to save a few lines and remove some
>> redundancies:
>>
>> printf "%s\0" untracked dir1/untracked dir2/untracked \
>>   modified dir1/modified dir2/modified \
>>   new dir1/new dir2/new
>>
>> or perhaps:
>>
>> for f in untracked modified new
>> do
>>printf "%s\0" "$f" "dir1/$f" "dir2/$f"
>> done
>
> That is a clever solution that certainly is fewer lines of code. However, I
> have to read the loop and think through the logic to figure out what it is
> doing vs the existing implementation where what it is doing is apparent from
> just glancing at the code.  I was also trying to maintain consistency with
> the other status test code in t7508-status.sh

Ok fair enough.

>>> +   EOF
>>> +   : >tracked &&
>>> +   : >modified &&
>>> +   mkdir dir1 &&
>>> +   : >dir1/tracked &&
>>> +   : >dir1/modified &&
>>> +   mkdir dir2 &&
>>> +   : >dir2/tracked &&
>>> +   : >dir2/modified &&
>>> +   git add . &&
>>> +   test_tick &&
>>> +   git commit -m initial &&
>>> +   dirty_repo
>>> +'
>>> +
>>> +cat >.gitignore <<\EOF
>>> +.gitignore
>>> +expect*
>>> +output*
>>> +marker*
>>> +EOF
>>
>> This could be part of the previous setup test.
>
> I had followed the pattern in t7508-status.sh with this but I can move it in
> if that is the preferred model.

Yeah, I think it is preferred these days to have all the setup code
inside tests.

>>> +   git config core.fsmonitor true  &&
>>> +   git config core.untrackedcache true &&
>>> +   git -c core.fsmonitor=false -c core.untrackedcache=true status
>>> >expect &&
>>
>> I don't understand why there is " -c core.untrackedcache=true" in the
>> above command as you already set core.untrackedcache to true on the
>> previous line.
>
> Defensive programming. :) The global setting was to ensure it was set when
> the test sub-functions clean and dirty were called and the command line
> settings were used to make it explicit what was being tested.  I can remove
> them if it is causing confusion.

I think it is a bit confusing indeed.

>>> +test_expect_success 'refresh_index() invalidates fsmonitor cache' '
>>> +   git config core.fsmonitor true &&
>>> +   git config core.untrackedcache true &&
>>> +   clean_repo &&
>>> +   git status &&
>>> +   test_path_is_missing marker &&
>>> +   dirty_repo &&
>>> +   write_script .git/hooks/query-fsmonitor<<-\EOF &&
>>> +   :>marker
>>> +   EOF
>>> +   git add . &&
>>> +   git commit -m "to reset" &&
>>> +   git status &&
>>> +   test_path_is_file marker &&

Ok so "marker" is there now.

>>> +   git reset HEAD~1 &&
>>> +   git status >output &&
>>> +   test_path_is_file marker &&
>>
>> You already checked that "marker" exists 3 lines above, and as far as
>> I can see nothing could remove this file since the previous test, as
>> the hook can only create it.
>> So I wonder if something is missing or if this test is redundant.
>
> Testing it each time ensures it is being created when it is supposed to be
> (ie when the test believes it is using the query-fsmonitor hook) and that it
> isn't when it isn't supposed to be (ie when the hook should not be called).

I would agree with that if the "marker" file was removed after the
previous "test_path_is_file marker", but I don't see any "clean_repo"
or "rm marker" call that removes it.

[PATCH] remote: drop free_refspecs() function

[Jeff King]

On Tue, May 30, 2017 at 09:11:28AM +0200, SZEDER Gábor wrote:

> I also dropped Peff's two patches that were included in v3, because:
> 
>  - his last patch doesn't apply anymore, because the variable it frees
>properly doesn't exist anymore, and
>  - with that patch gone his second patch is not tangled up with this
>topic, so I think it should go on its own branch.

Makes sense on both counts. Here's that stand-alone patch for reference,
which can go onto its own topic.

-- >8 --
Subject: [PATCH] remote: drop free_refspecs() function

We already have free_refspec(), a public function which does
the same thing as the static free_refspecs(). Let's just
keep one.  There are two minor differences between the
functions:

  1. free_refspecs() is a noop when the refspec argument is
 NULL. This probably doesn't matter in practice.  The
 nr_refspec parameter would presumably be 0 in that
 case, skipping the loop. And free(NULL) is explicitly
 OK. But it doesn't hurt for us to port this extra
 safety to free_refspec(), as one of the callers passes
 a funny "i+1" count.

  2. The order of arguments is reversed between the two
 functions. This patch uses the already-public order of
 free_refspec(), as it matches the argument order on the
 parsing side.

Signed-off-by: Jeff King 
---
 remote.c | 28 ++--
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/remote.c b/remote.c
index fdc52d802..2e39bf930 100644
--- a/remote.c
+++ b/remote.c
@@ -477,26 +477,6 @@ static void read_config(void)
alias_all_urls();
 }
 
-/*
- * This function frees a refspec array.
- * Warning: code paths should be checked to ensure that the src
- *  and dst pointers are always freeable pointers as well
- *  as the refspec pointer itself.
- */
-static void free_refspecs(struct refspec *refspec, int nr_refspec)
-{
-   int i;
-
-   if (!refspec)
-   return;
-
-   for (i = 0; i < nr_refspec; i++) {
-   free(refspec[i].src);
-   free(refspec[i].dst);
-   }
-   free(refspec);
-}
-
 static struct refspec *parse_refspec_internal(int nr_refspec, const char 
**refspec, int fetch, int verify)
 {
int i;
@@ -610,7 +590,7 @@ static struct refspec *parse_refspec_internal(int 
nr_refspec, const char **refsp
 * since it is only possible to reach this point from within
 * the for loop above.
 */
-   free_refspecs(rs, i+1);
+   free_refspec(i+1, rs);
return NULL;
}
die("Invalid refspec '%s'", refspec[i]);
@@ -621,7 +601,7 @@ int valid_fetch_refspec(const char *fetch_refspec_str)
struct refspec *refspec;
 
refspec = parse_refspec_internal(1, _refspec_str, 1, 1);
-   free_refspecs(refspec, 1);
+   free_refspec(1, refspec);
return !!refspec;
 }
 
@@ -638,6 +618,10 @@ struct refspec *parse_push_refspec(int nr_refspec, const 
char **refspec)
 void free_refspec(int nr_refspec, struct refspec *refspec)
 {
int i;
+
+   if (!refspec)
+   return;
+
for (i = 0; i < nr_refspec; i++) {
free(refspec[i].src);
free(refspec[i].dst);
-- 
2.13.0.676.ge5a8f17ed

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

[Jeff King]

On Tue, May 30, 2017 at 09:12:43AM +0200, SZEDER Gábor wrote:

> diff --git a/remote.c b/remote.c
> index ad6c5424e..b8fd09dc9 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, 
> const char **refspec)
>   return parse_refspec_internal(nr_refspec, refspec, 1, 0);
>  }
>  
> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
> +{
> + struct refspec *rs;
> +
> + add_fetch_refspec(remote, refspec);
> + rs = parse_fetch_refspec(1, );
> + REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
> + remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
> +
> + /* Not free_refspecs(), as we copied its pointers above */
> + free(rs);
> +}

What happens here if remote->fetch isn't already initialized? I think
we'd end up with a bunch of garbage values. That's what I was trying to
protect against in my original suggestion.

I'm not sure if that's possible or not. We seem to initialize it in both
remote_get() and for_each_remote(), and I don't think there are any
other ways to get a remote. (In fact, I kind of wondered why we do this
parsing lazily at all, but I think it's so that misconfigured remotes
don't cause us to die() if we aren't accessing them at all).

If we assume that the contract that remote.c provides is that the
fetch fields are always filled in before a "struct remote" is returned
to a caller, and that only such callers would use this function, it's
OK. It just seems more dangerous than it needs to be.

-Peff

Re: What's cooking in git.git (May 2017, #08; Mon, 29)

[Daniel Ferreira (theiostream)]

On Mon, May 29, 2017 at 3:23 AM, Junio C Hamano  wrote:
> * df/dir-iter-remove-subtree (2017-05-29) 5 commits
>  . remove_subtree(): reimplement using iterators
>  . dir_iterator: rewrite state machine model
>  . dir_iterator: refactor dir_iterator_advance
>  . remove_subtree(): test removing nested directories
>  . dir_iterator: add tests for dir_iterator API
>
>  Update the dir-iterator API and use it to reimplement
>  remove_subtree().
>
>  GSoC microproject.
>  Ejected as it conflicts with other topics in flight in a
>  non-trivial way.

I see this conflicts with Duy's
fa7e9c0c24637d6b041a2919a33956b68bfd0869 ("files-backend: make reflog
iterator go through per-worktree reflog", 2017-04-19) because his
commit creates a new dir_iterator whose NULL value means something
semantically. This would be perfectly OK with the old dir_iterator API
(where NULL was not a possible return value from dir_iterator_begin()
and could be "reserved" for this case), but will most probably
generate issues with the new API, where NULL can *also* mean we failed
to lstat() the directory we're trying to iterate over[1].

I'll try to address this issue playing with pu, but I'm just wondering
what would be the best way to send this upcoming not-based-on-master
patch to the list. Should I just send it normally and signal it
originates from pu rather than master?

Thanks,

[1]: 
https://public-inbox.org/git/1493226219-33423-1-git-send-email-bnm...@gmail.com/T/#m68286d783b5dfbad6921fbf012f685a629645c61

[PATCH v2] pull: ff --rebase --autostash works in dirty repo

[Tyler Brazier]

When `git pull --rebase --autostash` in a dirty repository resulted in a
fast-forward, nothing was being autostashed and the pull failed. This
was due to a shortcut to avoid running rebase when we can fast-forward,
but autostash is ignored on that codepath.

Now we will only take the shortcut if autostash is not in effect.
Based on a few tests against the git.git repo, the shortcut does not
seem to give us significant performance benefits, on Linux at least.
Regardless, it is more important to be correct than to be fast.
---
 builtin/pull.c  | 25 ++---
 t/t5520-pull.sh | 18 ++
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index dd1a4a94e41e..42f0560252e0 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -772,6 +772,7 @@ int cmd_pull(int argc, const char **argv, const char 
*prefix)
struct oid_array merge_heads = OID_ARRAY_INIT;
struct object_id orig_head, curr_head;
struct object_id rebase_fork_point;
+   int autostash;
 
if (!getenv("GIT_REFLOG_ACTION"))
set_reflog_message(argc, argv);
@@ -800,8 +801,8 @@ int cmd_pull(int argc, const char **argv, const char 
*prefix)
if (!opt_rebase && opt_autostash != -1)
die(_("--[no-]autostash option is only valid with --rebase."));
 
+   autostash = config_autostash;
if (opt_rebase) {
-   int autostash = config_autostash;
if (opt_autostash != -1)
autostash = opt_autostash;
 
@@ -862,16 +863,18 @@ int cmd_pull(int argc, const char **argv, const char 
*prefix)
die(_("Cannot rebase onto multiple branches."));
 
if (opt_rebase) {
-   struct commit_list *list = NULL;
-   struct commit *merge_head, *head;
-
-   head = lookup_commit_reference(orig_head.hash);
-   commit_list_insert(head, );
-   merge_head = lookup_commit_reference(merge_heads.oid[0].hash);
-   if (is_descendant_of(merge_head, list)) {
-   /* we can fast-forward this without invoking rebase */
-   opt_ff = "--ff-only";
-   return run_merge();
+   if (!autostash) {
+   struct commit_list *list = NULL;
+   struct commit *merge_head, *head;
+
+   head = lookup_commit_reference(orig_head.hash);
+   commit_list_insert(head, );
+   merge_head = 
lookup_commit_reference(merge_heads.oid[0].hash);
+   if (is_descendant_of(merge_head, list)) {
+   /* we can fast-forward this without invoking 
rebase */
+   opt_ff = "--ff-only";
+   return run_merge();
+   }
}
return run_rebase(_head, merge_heads.oid, 
_fork_point);
} else {
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 17f4d0fe4e72..f15f7a332960 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -272,6 +272,24 @@ test_expect_success '--rebase fast forward' '
test_cmp reflog.expected reflog.fuzzy
 '
 
+test_expect_success '--rebase --autostash fast forward' '
+   test_when_finished "
+   git reset --hard
+   git checkout to-rebase
+   git branch -D to-rebase-ff
+   git branch -D behind" &&
+   git branch behind &&
+   git checkout -b to-rebase-ff &&
+   echo another modification >>file &&
+   git add file &&
+   git commit -m mod &&
+
+   git checkout behind &&
+   echo dirty >file &&
+   git pull --rebase --autostash . to-rebase-ff &&
+   test "$(git rev-parse HEAD)" = "$(git rev-parse to-rebase-ff)"
+'
+
 test_expect_success '--rebase with conflicts shows advice' '
test_when_finished "git rebase --abort; git checkout -f to-rebase" &&
git checkout -b seq &&

--
https://github.com/git/git/pull/365

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Ramsay Jones]

On 31/05/17 01:13, Ramsay Jones wrote:

> On 31/05/17 00:29, Stefan Beller wrote:
>  
>> In that test sm_path contains the relative path from $PWD to the
>> submodule. (It does NOT: "$[sm_]path is the name of the submodule
>> directory relative to the superproject" as documented but rather
>> ... relative to the $PWD)
> 
> In that case, the current user documentation does not agree with
> the current implementation, yes?
> 
> So, was the user documentation always wrong? (did git-ls-files work
> from a sub-directory, limiting its output to the cwd, or did it
> chdir() to the top of the worktree first?).

To answer my own question, I rebuilt git and tried directly:

  $ pwd
  /home/ramsay/git
  $ 
  $ ./git version
  git version 1.7.10.1.g64394
  $ 
  $ cd git_remote_helpers/
  $ ../git-ls-files --error-unmatch --stage --
  100644 2247d5f95a7193c7221b9464debe167763b4fae3 0 .gitignore
  100644 74b05dc91e42414147d5f3dc7b4fc66fb86c0eca 0 Makefile
  100644 00f69cbeda277b24e8ab35cb7db2c25cc0cc122e 0 __init__.py
  100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 git/__init__.py
  100644 9ee5f96d4ce313f4f94505ff65b560943bfd21cb 0 git/exporter.py
  100644 007a1bfdf37d231470f69d9d0cffa46e80127f34 0 git/git.py
  100644 5c6b595e16665bc508625ab0e96c95776bacba1a 0 git/importer.py
  100644 e70025095dcfb31d3944e72ac1f83dd7d4109103 0 git/non_local.py
  100644 acbf8d7785e2253777456f8910e2352992dda474 0 git/repo.py
  100644 4bff8878d14ccaf02c552073ef55d519df0b4cad 0 setup.cfg
  100644 4d434b65cbf5c42a455d5cd3bced030bfb51a245 0 setup.py
  100644 fbbb01b14619c1d2ed6bcc8f304f019fbe98697f 0 util.py
  $ 
  
Hmm, so it looks like $path was always relative to cwd!

(got to get some sleep now ...)

ATB,
Ramsay Jones

Re: [PATCH 16/33] diff: finish conversion for prepare_temp_file to struct object_id

[Stefan Beller]

On Tue, May 30, 2017 at 10:30 AM, Brandon Williams  wrote:
> Signed-off-by: Brandon Williams 

Up to (and including) this commit, the series looks good to me.
Will continue to review this series later.

Thanks,
Stefan

> ---
>  diff.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 084c8b2d0..a8ceeb024 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3030,13 +3030,13 @@ static struct diff_tempfile *prepare_temp_file(const 
> char *name,
> /* we can borrow from the file in the work tree */
> temp->name = name;
> if (!one->oid_valid)
> -   sha1_to_hex_r(temp->hex, null_sha1);
> +   oid_to_hex_r(temp->hex, _oid);
> else
> oid_to_hex_r(temp->hex, >oid);
> /* Even though we may sometimes borrow the
>  * contents from the work tree, we always want
>  * one->mode.  mode is trustworthy even when
> -* !(one->sha1_valid), as long as
> +* !(one->oid_valid), as long as
>  * DIFF_FILE_VALID(one).
>  */
> xsnprintf(temp->mode, sizeof(temp->mode), "%06o", 
> one->mode);
> --
> 2.13.0.219.gdb65acc882-goog
>

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Ramsay Jones]

On 31/05/17 00:29, Stefan Beller wrote:
 
>> As I said above, I can't remember how git-ls-files worked back then,
>> but it seems that I thought of it as the path to the submodule from
>> the root of the working tree. Again, by definition, $sm_path == $path
>> (as documented). Of course, that may have changed since then.
> 
> Documented in 64394e3 (git-submodule.sh: Don't use $path variable in
> eval_gettext string, by yourself)
> 
> What I intended to say above was "documented to the end user",
> and I do not count our commit messages as such. The end user facing
> documentation only talks about path, not mentioning sm_path.

Correct, and that is exactly what I was saying. ie. $path as
'documented to the end user'. (again by _definition_ $sm_path
_is_ $path).

>>> After rereading that test, I would think so?
>>
>> Really? So, if it differs from $path, then something changed between
>> commit 64394e3ae9 and commit 091a6eb0fe. I haven't really read that
>> commit/test, so take what I say with a pinch of salt ...
> 
> Well yes. I am specifically reading 091a6eb0fe, the changes to t7407.
> 
> In that test sm_path contains the relative path from $PWD to the
> submodule. (It does NOT: "$[sm_]path is the name of the submodule
> directory relative to the superproject" as documented but rather
> ... relative to the $PWD)

In that case, the current user documentation does not agree with
the current implementation, yes?

So, was the user documentation always wrong? (did git-ls-files work
from a sub-directory, limiting its output to the cwd, or did it
chdir() to the top of the worktree first?).

>> As I said in commit 64394e3ae9, $path was part of the API, so I could
>> not just rename it, without a deprecation period, etc ... So, I was
>> 'crossing my fingers' that nobody would export $path in their user
>> scripts (not very likely, after all).
> 
> Ok. So another approach to get away in the C conversion:
> * export the sm_path as all other environment variables
> * for "$path" we do not export it into the environment, but
>   prefix the command with it, i.e. we'd ask our shell to run
>   "path=%s; %s", sm_path, argv[0]
>   to preserve the historic behavior.

Yes, that would probably work.

ATB,
Ramsay Jones

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Ben Peart]

On 5/30/2017 6:37 PM, Junio C Hamano wrote:

Ben Peart  writes:


I did a quick search through the existing test scripts and the
majority do not link commands together with && when they are in a sub
function like this.  I find not having them linked together is easier
to write, maintain and is more readable.


I had an impression that it is true only when the series of commands
are not about Git.  When testing git commands, we should expect any
of them to be broken (by somebody else ;-) and prepare to notice it.



Fixed

Re: mergetool: what to do about deleting precious files?

[Junio C Hamano]

"Philip Oakley"  writes:

> From: "Junio C Hamano" 
>
> Thanks for the replies. Let's see if I've got it...
>
>> "Philip Oakley"  writes:
>>
>>> If I now understand correctly, the merge process flow is:
>>>
>>> * canonicalise content (eol, smudge-clean, $id, renormalise, etc)
>>> * diff the content (internal, or GIT_EXTERNAL_DIFF)
>>> * apply the diff
>>> * if conflicts, only then use merge-driver/tool
>>>
>>> Would that be a correct interpretation?
>>
>> Not quite.  There are a lot more going on before any of those steps:
>>
>> * Find the common ancestor commit (which could be many).
>
> IIUC Git selects one of them, rather than all if there are many (which
> then may not be the optimum)

Not quite.  The interface to "git merge-$backend" can take more than
one and "git merge" frontend does pass them to the backend.  How
they are used depends on the backend.  The "resolve" one tries to
use all of them at once; the "recursive" one tries merge across them
to come up with a tree to be used as a single "virtual common
ancestor".  But details does not matter for the purpose of analysing
the case that triggered this discussion.

>>
>> * Walk the three trees (the common ancestor's, ours and theirs) in
>>   parallel, noticing what happened to each path.  Depending on what
>>   happened to the path in each branch, the merge may or may not
>>   "conflict" (e.g. when both sides added exactly the same contents
>>   to the same path, they are not counted as conflicting.  when we
>>   removed while they modified, they show as conflicting).
>
> I'm assuming here that this is the sha-oid comparison, and then
> checking the tree/blob names that match them. (the top tree not having
> a name). So here "conflict free" is that the sha-oids match.
>
> Also, I thnk this is saying that added or removed trees or blobs are
> in some sense are 'conflict free' (though still subject to rename/move
> detection etc). An added file/blob would be conflict free for merging
> into it's tree, yes?

After "recursive" figures out the renames, an addition that still
remains (i.e. not matched up with a deletion elsewhere) would be a
candidate to be added silently (except that D/F conflict can still
be diagnosed).

>> * For paths that are conflicting, feed the canonicalized content of
>>   the versions from common, ours and theirs to the file-level merge
>>   driver.
>
> So this is where any .gitattibutes settings come in, or is the merge
> driver after the diff step? (which could also be a user diff?)

I think you answered this yourself in your "Ok, I think I can see
how I was confused..." paragraph.

Re: [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows

[Stefan Beller]

On Tue, May 30, 2017 at 4:17 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:

>> cc'd people knowledgeable of Windows.
>
> This has been resolved I think with J6t/Dscho's patch yesterday.
>
> Thanks.

Yeah I saw those patches after reviewing this series.

Sorry for the noise,
Stefan

Re: [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too

[Stefan Beller]

On Tue, May 30, 2017 at 4:14 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano  wrote:
>>
>> Do we have any reasons for that, or pointers on the mailing list, that this
>> is a good idea or needed? Does it fix a bug or enable a new feature on 
>> Darwin?
>
> BSD lets you open() a directory for reading and hence we require
> this workaround on it; I think somebody in an earlier round
> suspected that that Darwin would have inherited the same from its
> BSD lineage, and I don't see a reason to contradict with that
> suspicion, either.
>

Sorry about not being clear:
I - just like some future reader - did not have the context of the previous
round, so please put this information into the commit message.

Thanks,
Stefan

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Stefan Beller]

On Tue, May 30, 2017 at 4:07 PM, Ramsay Jones
 wrote:
>
>
> On 30/05/17 22:53, Stefan Beller wrote:
>> On Fri, May 26, 2017 at 6:10 PM, Ramsay Jones
>>  wrote:
>>> On 26/05/17 18:07, Stefan Beller wrote:
 On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
  wrote:
>
>>> Back in 2012, the submodule list was generated by filtering the
>>> output of 'git ls-files --error-unmatch --stage --'; but I don't
>>> recall if (at that time) git-ls-files required being at the top
>>> of the working tree, or if it would execute fine in a sub-directory.
>>> So, it's possible that the documentation of $path was wrong all along.
>>> ;-)
>>>
>>> At that time, by definition, $path == $sm_path. However, you know this
>>> stuff much better than me (I don't use git-submodule), so ...
>>
>> Don't take that stance. Sometimes I shoot quickly from the hip without
>> considering consequences (Figuratively).
>>
>> In a foreach command I can see value both in the 'displaypath'
>> (what sm_path would become here) and the 'submodule path'
>> from the superproject. The naming of 'sm_path' hints at that it ought
>> to be the 'submodule path'.
>
> Well, since I introduced it, I can confidently proclaim that it is
> indeed the 'submodule path'. :-D

ok. :)

> As I said above, I can't remember how git-ls-files worked back then,
> but it seems that I thought of it as the path to the submodule from
> the root of the working tree. Again, by definition, $sm_path == $path
> (as documented). Of course, that may have changed since then.

Documented in 64394e3 (git-submodule.sh: Don't use $path variable in
eval_gettext string, by yourself)

What I intended to say above was "documented to the end user",
and I do not count our commit messages as such. The end user facing
documentation only talks about path, not mentioning sm_path.

 With this patch it becomes less non-sensey and could be documented as:

 $sm_path is the relative path from the current working directory
 to the submodule (ignoring relations to the superproject or nesting
 of submodules).
>>>
>>> OK.
>>>
  This documentation also fits into the narrative of
 the test in t7407.
>>>
>>> Hmm, does it?
>>
>> After rereading that test, I would think so?
>
> Really? So, if it differs from $path, then something changed between
> commit 64394e3ae9 and commit 091a6eb0fe. I haven't really read that
> commit/test, so take what I say with a pinch of salt ...

Well yes. I am specifically reading 091a6eb0fe, the changes to t7407.

In that test sm_path contains the relative path from $PWD to the
submodule. (It does NOT: "$[sm_]path is the name of the submodule
directory relative to the superproject" as documented but rather
... relative to the $PWD)

>
>> Thanks for keeping discussing this.
>>
>> So maybe we want to
>> * keep path=sm_path
>
> As I said in commit 64394e3ae9, $path was part of the API, so I could
> not just rename it, without a deprecation period, etc ... So, I was
> 'crossing my fingers' that nobody would export $path in their user
> scripts (not very likely, after all).

Ok. So another approach to get away in the C conversion:
* export the sm_path as all other environment variables
* for "$path" we do not export it into the environment, but
  prefix the command with it, i.e. we'd ask our shell to run
  "path=%s; %s", sm_path, argv[0]
  to preserve the historic behavior.

>
>> * fix the documentation via s/$path/$sm_path/g in that section quoted above.
>
> So, "$path is the name of the submodule directory relative to the
> superproject", as currently documented in the man page, yes?

No, the documentation does not match reality. The reality is that
both sm_path as well as path give the display path.

> So, $sm_path == $path, at least for some period?

yes that is current reality.

>
>> * Introduce a new variable sm_display_path that contains what this patch
>>   proposes sm_path to be.
>
> So, this would be the path from the cwd to the submodule, yes?

yes.

>
> I don't know how windows folks will feel about simply
> removing $path, ...

I agree that this is a bad idea now. As said above we'd
just not export the path as an env variable and should be
"fine" in the sense that we do not break historical expectations,
but have to deal with slightly messy code.

Just digging further, there was ea2fa1040d (submodule foreach:
correct path display in recursive submodules, 2016-03-29), which
is tangent to the issue of whether we think it is a display path
or the superproject->submodule path.

Thanks,
Stefan

Re: [PATCH] doc: Improve description for rev-parse --short

[Junio C Hamano]

Andreas Heiduk  writes:

> ALso: Did you remove the `linkgit` by intention or just by accident?

By accident.  I agree that "Same as `--verify`" is a good way to do
this.

Re: [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows

[Junio C Hamano]

Stefan Beller  writes:

> On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano  wrote:
>> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
>> on the filesystem, Windows (correctly) fails to open it but sets
>> EINVAL to errno
>
> errno to EINVAL (as of now it sounds as if it is a EINVAL = errno,
> which makes no sense to me)
>
>> because the pathname has characters that cannot be
>> stored in its filesystem.
>>
>> As this is an expected failure, teach is_missing_file_error() helper
>> about this case.
>>
>> This is RFC,
>
> cc'd people knowledgeable of Windows.

This has been resolved I think with J6t/Dscho's patch yesterday.

Thanks.

Re: [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too

[Junio C Hamano]

Stefan Beller  writes:

> On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano  wrote:
>
> Do we have any reasons for that, or pointers on the mailing list, that this
> is a good idea or needed? Does it fix a bug or enable a new feature on Darwin?

BSD lets you open() a directory for reading and hence we require
this workaround on it; I think somebody in an earlier round
suspected that that Darwin would have inherited the same from its
BSD lineage, and I don't see a reason to contradict with that
suspicion, either.

>
>> Signed-off-by: Junio C Hamano 
>> ---
>>  config.mak.uname | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/config.mak.uname b/config.mak.uname
>> index a25ffddb3e..1743890164 100644
>> --- a/config.mak.uname
>> +++ b/config.mak.uname
>> @@ -110,6 +110,7 @@ ifeq ($(uname_S),Darwin)
>> BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
>> BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1
>> HAVE_BSD_SYSCTL = YesPlease
>> +   FREAD_READS_DIRECTORIES = UnfortunatelyYes
>>  endif
>>  ifeq ($(uname_S),SunOS)
>> NEEDS_SOCKET = YesPlease
>> --
>> 2.13.0-491-g71cfeddc25
>>

Re: [PATCH v2 0/6] Fast git status via a file system watcher

[Ben Peart]

On 5/30/2017 4:33 PM, Christian Couder wrote:

On Tue, May 30, 2017 at 8:05 PM, Ben Peart  wrote:



On 5/27/2017 2:57 AM, Christian Couder wrote:


On Thu, May 25, 2017 at 3:55 PM, Ben Peart  wrote:




On 5/24/2017 6:54 AM, Christian Couder wrote:



Design
~~

A new git hook (query-fsmonitor) must exist and be enabled
(core.fsmonitor=true) that takes a time_t formatted as a string and
outputs to stdout all files that have been modified since the requested
time.




Is there a reason why there is a new hook, instead of a
"core.fsmonitorquery" config option to which you could pass whatever
command line with options?




A hook is a simple and well defined way to integrate git with another
process.  If there is some fixed set of arguments that need to be passed
to
a file system monitor (beyond the timestamp stored in the index
extension),
they can be encoded in the integration script like I've done in the
Watchman
integration sample hook.



Yeah, they could be encoded in the integration script, but it could be
better if it was possible to just configure a generic command line.

For example if the directory that should be watched for filesystem
changes could be passed as well as the time since the last changes,
perhaps only a generic command line would be need.



Maybe I'm not understanding what you have in mind but I haven't found this
to be the case in the two integrations I've done with file system watchers
(one internal and Watchman).  They require you download, install, and
configure them by telling them about the folders you want monitored.  Then
you can start querying them for changes and processing the output to match
what git expects.  While the download and install steps vary, having that
query + process and return results wrapped up in an integration hook has
worked well.


It looks like one can also just ask watchman to monitor a directory with:

watchman watch /path/to/dir

or:

echo '["watch", "/path/to/dir"]' | watchman -j

Also for example on Linux people might want to use command line tools like:

https://linux.die.net/man/1/inotifywait

and you can pass the directories you want to be watched as arguments
to this kind of tools.

So it would be nice, if we didn't require the user to configure
anything and we could just configure the watching of what we need in
the hook (or a command configured using a config option). If the hook
(or configured command) could be passed the directory by git, it could
also be generic.



OK, I think I understand what you're attempting to accomplish now. 
Often, Watchman (and other similar tools) are used to do much more than 
speed up git (in fact, _all_ use cases today are not used for that since 
this patch series hasn't been accepted yet :)).  They trigger builds, 
run verification tools, test passes, or other tasks.


I'm afraid that attempting to have the user configure git to configure 
the tool "automatically" is just adding an extra layer of complexity 
rather than making it simpler.  I'll leave that to a future patch series 
to work out.



I am also wondering about sparse checkout, as we might want to pass
all the directories we are interested in.
How is it supposed to work with sparse checkout?


The fsmonitor code works well with or without a sparse-checkout.  The file
system monitor is unaware of the sparse checkout so will notify git about
any change irrespective of whether git will eventually ignore it because the
skip worktree bit is set.


I was wondering if it could ease the job for the monitoring service
and perhaps improve performance to just ask to watch the directories
we are interested in when using sparse checkout.
On Linux it looks like a separate inotify watch is created for every
subdirectory and there is maximum amount of inotify watches per user.
This can be increased by writing in
/proc/sys/fs/inotify/max_user_watches, but it is not nice to have to
ask admins to increase this.



Having a single instance that watches the root of the working directory 
is the simplest model and minimizes use of system resources like inotify 
as there is only one needed per clone.


In addition, when the sparse-checkout file is modified, there is no need 
to try and automatically update the monitor by adding and removing 
folders as necessary.


Finally, if files or directories are excluded via sparse-checkout, they 
are removed from the working directory at checkout time so don't add any 
additional overhead to the file system watcher anyway as they clearly 
can't generate write events if they don't exist.

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Ramsay Jones]

On 30/05/17 22:53, Stefan Beller wrote:
> On Fri, May 26, 2017 at 6:10 PM, Ramsay Jones
>  wrote:
>> On 26/05/17 18:07, Stefan Beller wrote:
>>> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
>>>  wrote:

>> Back in 2012, the submodule list was generated by filtering the
>> output of 'git ls-files --error-unmatch --stage --'; but I don't
>> recall if (at that time) git-ls-files required being at the top
>> of the working tree, or if it would execute fine in a sub-directory.
>> So, it's possible that the documentation of $path was wrong all along.
>> ;-)
>>
>> At that time, by definition, $path == $sm_path. However, you know this
>> stuff much better than me (I don't use git-submodule), so ...
> 
> Don't take that stance. Sometimes I shoot quickly from the hip without
> considering consequences (Figuratively).
> 
> In a foreach command I can see value both in the 'displaypath'
> (what sm_path would become here) and the 'submodule path'
> from the superproject. The naming of 'sm_path' hints at that it ought
> to be the 'submodule path'.

Well, since I introduced it, I can confidently proclaim that it is
indeed the 'submodule path'. :-D

As I said above, I can't remember how git-ls-files worked back then,
but it seems that I thought of it as the path to the submodule from
the root of the working tree. Again, by definition, $sm_path == $path
(as documented). Of course, that may have changed since then.

>>> With this patch it becomes less non-sensey and could be documented as:
>>>
>>> $sm_path is the relative path from the current working directory
>>> to the submodule (ignoring relations to the superproject or nesting
>>> of submodules).
>>
>> OK.
>>
>>>  This documentation also fits into the narrative of
>>> the test in t7407.
>>
>> Hmm, does it?
> 
> After rereading that test, I would think so?

Really? So, if it differs from $path, then something changed between
commit 64394e3ae9 and commit 091a6eb0fe. I haven't really read that
commit/test, so take what I say with a pinch of salt ...

> Thanks for keeping discussing this.
> 
> So maybe we want to
> * keep path=sm_path

As I said in commit 64394e3ae9, $path was part of the API, so I could
not just rename it, without a deprecation period, etc ... So, I was
'crossing my fingers' that nobody would export $path in their user
scripts (not very likely, after all).

> * fix the documentation via s/$path/$sm_path/g in that section quoted above.

So, "$path is the name of the submodule directory relative to the
superproject", as currently documented in the man page, yes?

So, $sm_path == $path, at least for some period?

> * Introduce a new variable sm_display_path that contains what this patch
>   proposes sm_path to be.

So, this would be the path from the cwd to the submodule, yes?

> * fix the test in t7407 by checking both sm_path (fixed) as well
>   as sm_display_path (what is currently recorded in sm_path)

Hmm, ...

> In the next patch:
> * Additionally in the rewrite in C, we would do an
> 
> #ifndef WINDOWS /* need to lookup the exact macro */
> argv_array_push(env_vars, "path=%s", sm_path);
> #endif
> 
> such that Windows users are forced to migrate to sm_path
> as path/Path is case sensitive there. sm_path being documented
> value, so it should work fine?

Well, as you saw in a separate thread, I can no longer get
cygwin to fail, so something (probably in the cygwin runtime)
has changed since 2012 to make this work now, despite the
case insensitive win32 environment block. (This may also be
true of MSYS2, but I haven't tested it).

I have not tested this on MYSY2/MinGW/Git-for-windows, but
Johannes Sixt was concerned about this, so I guess it may
still be a problem there.

I don't know how windows folks will feel about simply
removing $path, ...


ATB,
Ramsay Jones

Re: What's cooking in git.git (May 2017, #08; Mon, 29)

[Junio C Hamano]

Stefan Beller  writes:

>>
>> * sb/submodule-blanket-recursive (2017-05-23) 6 commits
> ...
>
> And the retraction is retracted by sending a new series.
> You remarked that it still misbehaves with other series in flight,
> so I'll inspect it again.

What I said (or at least what I ment to say) was "it looked like it
is based on an older codebase and I do not yet know how messy the
conflict resolution and resulting history would look like".

Re: mergetool: what to do about deleting precious files?

[Philip Oakley]

From: "Junio C Hamano" 

Thanks for the replies. Let's see if I've got it...


"Philip Oakley"  writes:


If I now understand correctly, the merge process flow is:

* canonicalise content (eol, smudge-clean, $id, renormalise, etc)
* diff the content (internal, or GIT_EXTERNAL_DIFF)
* apply the diff
* if conflicts, only then use merge-driver/tool

Would that be a correct interpretation?


Not quite.  There are a lot more going on before any of those steps:

* Find the common ancestor commit (which could be many).


IIUC Git selects one of them, rather than all if there are many (which then 
may not be the optimum)




* Walk the three trees (the common ancestor's, ours and theirs) in
  parallel, noticing what happened to each path.  Depending on what
  happened to the path in each branch, the merge may or may not
  "conflict" (e.g. when both sides added exactly the same contents
  to the same path, they are not counted as conflicting.  when we
  removed while they modified, they show as conflicting).


I'm assuming here that this is the sha-oid comparison, and then checking the 
tree/blob names that match them. (the top tree not having a name). So here 
"conflict free" is that the sha-oids match.


Also, I thnk this is saying that added or removed trees or blobs are in some 
sense are 'conflict free' (though still subject to rename/move detection 
etc). An added file/blob would be conflict free for merging into it's tree, 
yes?


IIUC, the comparison is therefore using the in-repo sha-oids; 
unless --renormalise was given which will do a smudge-clean washing cycle 
and recomute fresh canonical sha-oids for the comparison (rather than doing 
it later).




* For paths that are conflicting, feed the canonicalized content of
  the versions from common, ours and theirs to the file-level merge
  driver.


So this is where any .gitattibutes settings come in, or is the merge driver 
after the diff step? (which could also be a user diff?)



   The builtin file-level merge driver takes two xdiff (one
  between ancestor and ours, the other between ancestore and
  theirs) and reconciles them to produce the result.  But that is
  irrelevant in the context of "custom merge driver"; the builtin
  one is skipped altogether and the custom contents merge driver
  the user specified via the attributes is used instead.

Notice that the second step above has no customization knobs.  Any
path the second step deems not to conflict is "merged cleanly"
without even triggering the "oops, ours and theirs did conflicting
changes, to the content; let's see how the final content should look
like" (aka the third step).  This is *not* because "Git knows the
best"; it is merely that nobody felt the need for a mechanism to
allow customizing the second step.

And that is why I said you need a new customization mechanism if you
want to affect the outcome of the scenario that started this thread.


Ok, I think I can see how I was confused between the "tree merge" (oid 
conflict detection) and the more usual (to users) "file merge" (line by 
line, etc.). I wasn't sure where to find that as someone relatively new to 
Git.


Thanks for the explanations.
--
Philip 



---
This email has been checked for viruses by AVG.
http://www.avg.com

Re: [PATCH] C style: use standard style for "TRANSLATORS" comments

[Junio C Hamano]

Jiang Xin  writes:

> 2017-05-12 5:20 GMT+08:00 Ævar Arnfjörð Bjarmason :
>> Change all the "TRANSLATORS: [...]" comments in the C code to use the
>> regular Git coding style, and amend the style guide so that the
>> example there uses that style.
>>
>> This custom style was necessary back in 2010 when the gettext support
>> was initially added, and was subsequently documented in commit
>> cbcfd4e3ea ("i18n: mention "TRANSLATORS:" marker in
>> Documentation/CodingGuidelines", 2014-04-18).
>>
>> GNU xgettext hasn't had the parsing limitation that necessitated this
>> exception for almost 3 years. Since its 0.19 release on 2014-06-02
>> it's been able to recognize TRANSLATOR comments in the standard Git
>> comment syntax[1].
>
> My gettext version is 0.19.8.1.  I applied this patch and checked the
> new generated `git.pot` file, all "TRANSLATORS:" directions are well
> kept as usual.

Ævar, sorry that this patch fell through cracks about 20 days ago.
I'll queue with Acked-by by Jiang.

Thanks, both.

Re: revision API design, was Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Junio C Hamano]

Johannes Schindelin  writes:

>> See 66b2ed09 ("Fix "log" family not to be too agressive about
>> ...
>> ask the existing command line parser to set them for you.
>
> This is a very eloquent description of a problem with the API.

Yes, but ...

> The correct suggestion would be to fix the API, of course. Not to declare
> an interface intended for command-line parsing the internal API.
>
> Maybe the introduction of `pretty_given` was a strong hint at a design
> flaw to begin with, pointing to the fact that user_format is a singleton
> (yes, really, you can't have two different user_formats in the same Git
> process, what were we thinking)?

... this tells me that you do not understand the issue.  The
embarrasing but necessary pretty-given field was not about
user_format (let alone singleton-ness of it) at all.  It was about
the show_notes feature that wants to be on by default most of the
time, but needs to be defeated when the end user asked for certain
formats.

Quite frankly, I am not interested in listening to a proposal to
update API by a person who does not understand the issue in the API,
but that is a separate issue.  A more important thing is that the
update to "rebase -i" is important enough and we do not want to
delay it by introducing further dependency.  IOW, I do not want to
spend an extra development cycle or two to update the revision setup
API and have you rebase the series on top after the API update is
done.

The current API to hide such an embarrasing but necessary
implementation details from the code that does not need to know
them, i.e. the consumer of rev-info structure with various settings,
is to invoke the command line parser.  Bypassing and going directly
down to the internal implementation detail of rev_info _is_ being
sloppy.  I would strongly prefer to see that the current series
written for the API to allow us get it over with the "rebase -i"
thing, and think revision setup API separately and later.

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Junio C Hamano]

Jeff King  writes:

> And the idea is that ranges like "-.." should work. TBH, I'm not sure
> how I feel about that, for exactly the reason that came up here: it
> makes it hard to syntactically differentiate the "-" shorthand from
> actual options. We do have @{-1} already for this purpose. I don't mind
> "-" as a shortcut for things like "git checkout -" or "git show -", but
> it feels like most of the benefit is lost when you're combining it with
> other operators.

All correct and that is why I haven't seriously considered merging
the topic further down (yet).  Things like -.. makes readers of the
commands go "Huh?", and "git tbdiff ..- -.." does not even work ;-)

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Junio C Hamano]

Ben Peart  writes:

> I did a quick search through the existing test scripts and the
> majority do not link commands together with && when they are in a sub
> function like this.  I find not having them linked together is easier
> to write, maintain and is more readable.

I had an impression that it is true only when the series of commands
are not about Git.  When testing git commands, we should expect any
of them to be broken (by somebody else ;-) and prepare to notice it.

Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value

[Stefan Beller]

On Fri, May 26, 2017 at 6:10 PM, Ramsay Jones
 wrote:
>
>
> On 26/05/17 18:07, Stefan Beller wrote:
>> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
>>  wrote:
>>> Hmm, I'm not sure which documentation you are referring to,
>>
>> Quite likely our fine manual pages. ;)
>>
>>foreach [--recursive] 
>>Evaluates an arbitrary shell command in each checked out 
>> submodule.
>>The command has access to the variables $name, $path, $sha1 and
>>$toplevel: $name is the name of the relevant submodule section in
>>.gitmodules, $path is the name of the submodule directory relative
>>to the superproject, $sha1 is the commit as recorded in the
>>superproject, and $toplevel is the absolute path to the top-level
>>of the superproject. Any submodules defined in the superproject 
>> but
>>not checked out are ignored by this command. Unless given --quiet,
>>foreach prints the name of each submodule before evaluating the
>>command. If --recursive is given, submodules are traversed
>>recursively (i.e. the given shell command is evaluated in nested
>>submodules as well). A non-zero return from the command in any
>>submodule causes the processing to terminate. This can be
>>overridden by adding || : to the end of the command.
>
> I suspected as much, but I was wondering specifically if $sm_path
> had been documented anywhere. I didn't think so, but ...
>
>> As $path is documented and $sm_path is not, we should care about
>> $path first to be correct and either fix the documentation or the 
>> implementation
>> such that we have a consistent world view. :)
>
> Sure, but what is that world view? :-D
>
> I suspect that commit 091a6eb0fe did not intend (should not have)
> used $sm_path in that test. If we were to 'fix' that test, would
> it still work?
>
> Back in 2012, the submodule list was generated by filtering the
> output of 'git ls-files --error-unmatch --stage --'; but I don't
> recall if (at that time) git-ls-files required being at the top
> of the working tree, or if it would execute fine in a sub-directory.
> So, it's possible that the documentation of $path was wrong all along.
> ;-)
>
> At that time, by definition, $path == $sm_path. However, you know this
> stuff much better than me (I don't use git-submodule), so ...

Don't take that stance. Sometimes I shoot quickly from the hip without
considering consequences (Figuratively).

In a foreach command I can see value both in the 'displaypath'
(what sm_path would become here) and the 'submodule path'
from the superproject. The naming of 'sm_path' hints at that it ought
to be the 'submodule path'.

>>
>> $path (as documented) is the name of the submodule directory
>> relative to the direct superproject (so in nested submodules you
>> go up only one level).
>>
>> $sm_path on the other hand is not documented at all and yields
>> non-sense results in corner cases.
>
> Hmm, at what point did '$sm_path yields non-sense results' start
> being the case? (perhaps the corner cases need to be fixed first).

Well the corner case is described in the patchs notes.
So that patch would fix it to be consistent with the new world view
(that I have in mind) as I do not know about the 2012 ideas how submodules
ought to behave correctly.

>> With this patch it becomes less non-sensey and could be documented as:
>>
>> $sm_path is the relative path from the current working directory
>> to the submodule (ignoring relations to the superproject or nesting
>> of submodules).
>
> OK.
>
>>  This documentation also fits into the narrative of
>> the test in t7407.
>
> Hmm, does it?

After rereading that test, I would think so?

Thanks for keeping discussing this.

So maybe we want to
* keep path=sm_path
* fix the documentation via s/$path/$sm_path/g in that section quoted above.
* Introduce a new variable sm_display_path that contains what this patch
  proposes sm_path to be.
* fix the test in t7407 by checking both sm_path (fixed) as well
  as sm_display_path (what is currently recorded in sm_path)
---
In the next patch:
* Additionally in the rewrite in C, we would do an

#ifndef WINDOWS /* need to lookup the exact macro */
argv_array_push(env_vars, "path=%s", sm_path);
#endif

such that Windows users are forced to migrate to sm_path
as path/Path is case sensitive there. sm_path being documented
value, so it should work fine?

Thanks,
Stefan

Re: [PATCHv4 00/17] Diff machine: highlight moved lines.

[Stefan Beller]

On Fri, May 26, 2017 at 6:04 PM, Jacob Keller  wrote:
> On Mon, May 22, 2017 at 7:40 PM, Stefan Beller  wrote:
>> v4:
>> * interdiff to v3 (what is currently origin/sb/diff-color-move) below.
>> * renamed the "buffered_patch_line" to "diff_line". Originally I planned
>>   to not carry the "line" part as it can be a piece of a line as well.
>>   But for the intended functionality it is best to keep the name.
>>   If we'd want to add more functionality to say have a move detection
>>   for words as well, we'd rename the struct to have a better name then.
>>   For now diff_line is the best. (Thanks Jonathan Nieder!)
>> * tests to demonstrate it doesn't mess with --color-words as well as
>>   submodules. (Thanks Jonathan Tan!)
>> * added in the statics (Thanks Ramsay!)
>> * smaller scope for the hashmaps (Thanks Jonathan Tan!)
>> * some commit messages were updated, prior patch 4-7 is squashed into one
>>   (Thanks Jonathan Tan!)
>> * the tests added revealed an actual fault: now that the submodule process
>>   is not attached to a dupe of our stdout, it would stop coloring the
>>   output. We need to pass on use-color explicitly.
>> * updated the NEEDSWORK comment in the second last patch.
>>
>> Thanks for bearing,
>> Stefan
>>
>
> One thing to note when I was playing around with what's on pu right
> now, I noticed that the oldMovedAlternative and newMovedAlternative
> are the first moved colors to be used if there is only one move. (Ie:
> a simple case of literally one section moved) This is a bit weird that
> the alternative colors are used before the "main" colors. I would have
> thought it would be the other way.

While pu is not up-to-date, I double checked with the most recent
implementation and that is no longer the case.

> I noticed this because the default colors do not work well for my
> terminal color scheme and I had to configure but realized that I
> needed to configure the alternative ones to make a difference in the
> simple diff I was viewing.

The v4 that you tested, is the "alternate" scheme in the resend
https://public-inbox.org/git/20170527001820.25214-2-sbel...@google.com/

Thanks,
Stefan

Re: [PATCH 1/1] diff.c: color moved lines differently

[Stefan Beller]

On Sat, May 27, 2017 at 12:05 AM, Philip Oakley  wrote:
> a couple of mispellings in the doc parts:
>  s/on location/one location/
> [code not checked]

Thanks for proofreading the documentation!

>
> s/on location/one location/
>
>> + in another location will be colored with 'color.diff.newmoved'.
>> + Any line that is removed in on location and was added
>
> s/on location/one location/

For a moment I was worried that my 'e' key is stuck as it happens
to be the same misspelling here. However then I realized I copied
over part of the sentence and just replaced added/removed.

Stepping back a bit, maybe the second sentence can be partially
elided to be more concise. Maybe:

Similarly 'color.diff.oldmoved' will be used for removed lines
that are added somewhere else in the diff.

Thanks,
Stefan

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Ben Peart]

On 5/30/2017 9:18 AM, Christian Couder wrote:

On Thu, May 25, 2017 at 8:36 PM, Ben Peart  wrote:

[...]


diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
new file mode 100755
index 00..395db46d55
--- /dev/null
+++ b/t/t7519-status-fsmonitor.sh
@@ -0,0 +1,158 @@
+#!/bin/sh
+
+test_description='git status with file system watcher'
+
+. ./test-lib.sh
+
+clean_repo () {
+   git reset --hard HEAD
+   git clean -fd
+   rm -f marker
+}


Maybe link all the commands in the function with "&&".


+dirty_repo () {
+   : >untracked
+   : >dir1/untracked
+   : >dir2/untracked
+   echo 1 >modified
+   echo 2 >dir1/modified
+   echo 3 >dir2/modified
+   echo 4 >new
+   echo 5 >dir1/new
+   echo 6 >dir2/new
+   git add new
+   git add dir1/new
+   git add dir2/new
+}


Idem.



I did a quick search through the existing test scripts and the majority 
do not link commands together with && when they are in a sub function 
like this.  I find not having them linked together is easier to write, 
maintain and is more readable.



+# The test query-fsmonitor hook proc will output a marker file we can use to
+# ensure the hook was actually used to generate the correct results.
+
+test_expect_success 'setup' '
+   mkdir -p .git/hooks &&
+   write_script .git/hooks/query-fsmonitor<<-\EOF &&
+   if [ $1 -ne 1 ]
+   then
+   echo -e "Unsupported query-fsmonitor hook version.\n" >&2
+   exit 1;
+   fi
+   : >marker
+   printf "untracked\0"
+   printf "dir1/untracked\0"
+   printf "dir2/untracked\0"
+   printf "modified\0"
+   printf "dir1/modified\0"
+   printf "dir2/modified\0"
+   printf "new\0""
+   printf "dir1/new\0"
+   printf "dir2/new\0"


Maybe something like the following to save a few lines and remove some
redundancies:

printf "%s\0" untracked dir1/untracked dir2/untracked \
  modified dir1/modified dir2/modified \
  new dir1/new dir2/new

or perhaps:

for f in untracked modified new
do
   printf "%s\0" "$f" "dir1/$f" "dir2/$f"
done



That is a clever solution that certainly is fewer lines of code. 
However, I have to read the loop and think through the logic to figure 
out what it is doing vs the existing implementation where what it is 
doing is apparent from just glancing at the code.  I was also trying to 
maintain consistency with the other status test code in t7508-status.sh



+   EOF
+   : >tracked &&
+   : >modified &&
+   mkdir dir1 &&
+   : >dir1/tracked &&
+   : >dir1/modified &&
+   mkdir dir2 &&
+   : >dir2/tracked &&
+   : >dir2/modified &&
+   git add . &&
+   test_tick &&
+   git commit -m initial &&
+   dirty_repo
+'
+
+cat >.gitignore <<\EOF
+.gitignore
+expect*
+output*
+marker*
+EOF


This could be part of the previous setup test.



I had followed the pattern in t7508-status.sh with this but I can move 
it in if that is the preferred model.



+# Status is well tested elsewhere so we'll just ensure that the results are
+# the same when using core.fsmonitor. First call after turning on the option
+# does a complete scan so need to do two calls to ensure we test the new
+# codepath.
+
+test_expect_success 'status with core.untrackedcache true' '


If this test is using untracked cache, it should perhaps first check
that untracked cache can be used on the current file system.
t7063-status-untracked-cache.sh does that with the following:

test_lazy_prereq UNTRACKED_CACHE '
 { git update-index --test-untracked-cache; ret=$?; } &&
 test $ret -ne 1
'

if ! test_have_prereq UNTRACKED_CACHE; then
 skip_all='This system does not support untracked cache'
 test_done
fi



Good point. I'll change it so that untracked cache is only turned on if 
it is available and that the one test that requires it is skipped if it 
isn't available.



+   git config core.fsmonitor true  &&
+   git config core.untrackedcache true &&
+   git -c core.fsmonitor=false -c core.untrackedcache=true status >expect 
&&


I don't understand why there is " -c core.untrackedcache=true" in the
above command as you already set core.untrackedcache to true on the
previous line.



Defensive programming. :) The global setting was to ensure it was set 
when the test sub-functions clean and dirty were called and the command 
line settings were used to make it explicit what was being tested.  I 
can remove them if it is causing confusion.



+   clean_repo &&
+   git status &&
+   test_path_is_missing marker &&
+   dirty_repo &&
+   git status >output &&
+   test_path_is_file marker &&
+   test_i18ncmp expect output
+'
+
+


Spurious new line.



Fixed


+test_expect_success 'status with core.untrackedcache false' '
+   git config core.fsmonitor 

Re: [PATCH 6.5?/8] version: move --build-options to a test helper

[Jeff King]

On Tue, May 30, 2017 at 08:45:53PM +, Ævar Arnfjörð Bjarmason wrote:

> On Tue, May 30, 2017 at 7:17 AM, Jeff King  wrote:
> > The "git version" command didn't traditionally accept any
> > options, and in fact ignores any you give it. When we added
> > simple option parsing for "--build-options" in 6b9c38e14, we
> > didn't improve this; we just loop over the arguments and
> > pick out the one we recognize.
> >
> > Instead, let's move to a real parsing loop, complain about
> > nonsense options, and recognize conventions like "-h".
> >
> > Signed-off-by: Jeff King 
> > ---
> > I assume nobody was running "git version --foobar" and expecting it to
> > work. I guess we could also complain about "git version foobar" (no
> > dashes), but this patch doesn't. Mainly I wanted the auto-generated
> > options list.
> 
> Looks good to me. I started hacking this up the other day, but then
> thought "wait a minute, isn't this just a test helper?" and wrote this
> which I've rebased on top of your change.
> 
> I may be missing something here but isn't this a much straightforward
> way to accomplish this, or is this used by some external program
> outside of git.git that's going to rely on --build-options output?

My intent in putting it into the actual git binary was that it could
also be useful for collecting build-time knobs from users (who may be
using a binary package). Like:

  http://public-inbox.org/git/20160712035719.ga30...@sigill.intra.peff.net/

We haven't filled in that NEEDSWORK yet, but I'd rather see us go in
that direction than remove the option entirely.

-Peff

[PATCH 6.5?/8] version: move --build-options to a test helper

[Ævar Arnfjörð Bjarmason]

Move the undocumented --build-options argument to a test helper. It's
purely used for testing git itself, so it belongs in a test helper
instead of something that's part of the public plumbing.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---

On Tue, May 30, 2017 at 7:17 AM, Jeff King  wrote:
> The "git version" command didn't traditionally accept any
> options, and in fact ignores any you give it. When we added
> simple option parsing for "--build-options" in 6b9c38e14, we
> didn't improve this; we just loop over the arguments and
> pick out the one we recognize.
>
> Instead, let's move to a real parsing loop, complain about
> nonsense options, and recognize conventions like "-h".
>
> Signed-off-by: Jeff King 
> ---
> I assume nobody was running "git version --foobar" and expecting it to
> work. I guess we could also complain about "git version foobar" (no
> dashes), but this patch doesn't. Mainly I wanted the auto-generated
> options list.

Looks good to me. I started hacking this up the other day, but then
thought "wait a minute, isn't this just a test helper?" and wrote this
which I've rebased on top of your change.

I may be missing something here but isn't this a much straightforward
way to accomplish this, or is this used by some external program
outside of git.git that's going to rely on --build-options output?

 Makefile  | 1 +
 help.c| 7 ---
 t/helper/.gitignore   | 1 +
 t/helper/test-long-is-64bit.c | 6 ++
 t/test-lib.sh | 9 +
 5 files changed, 9 insertions(+), 15 deletions(-)
 create mode 100644 t/helper/test-long-is-64bit.c

diff --git a/Makefile b/Makefile
index 2ed6db728a..aa908ae75a 100644
--- a/Makefile
+++ b/Makefile
@@ -623,6 +623,7 @@ TEST_PROGRAMS_NEED_X += test-hashmap
 TEST_PROGRAMS_NEED_X += test-index-version
 TEST_PROGRAMS_NEED_X += test-lazy-init-name-hash
 TEST_PROGRAMS_NEED_X += test-line-buffer
+TEST_PROGRAMS_NEED_X += test-long-is-64bit
 TEST_PROGRAMS_NEED_X += test-match-trees
 TEST_PROGRAMS_NEED_X += test-mergesort
 TEST_PROGRAMS_NEED_X += test-mktemp
diff --git a/help.c b/help.c
index f637fc8006..0a7628a922 100644
--- a/help.c
+++ b/help.c
@@ -384,14 +384,11 @@ const char *help_unknown_cmd(const char *cmd)
 
 int cmd_version(int argc, const char **argv, const char *prefix)
 {
-   int build_options = 0;
const char * const usage[] = {
N_("git version []"),
NULL
};
struct option options[] = {
-   OPT_BOOL(0, "build-options", _options,
-"also print build options"),
OPT_END()
};
 
@@ -405,10 +402,6 @@ int cmd_version(int argc, const char **argv, const char 
*prefix)
 */
printf("git version %s\n", git_version_string);
 
-   if (build_options) {
-   printf("sizeof-long: %d\n", (int)sizeof(long));
-   /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
-   }
return 0;
 }
 
diff --git a/t/helper/.gitignore b/t/helper/.gitignore
index 721650256e..739c4c745c 100644
--- a/t/helper/.gitignore
+++ b/t/helper/.gitignore
@@ -13,6 +13,7 @@
 /test-index-version
 /test-lazy-init-name-hash
 /test-line-buffer
+/test-long-is-64bit
 /test-match-trees
 /test-mergesort
 /test-mktemp
diff --git a/t/helper/test-long-is-64bit.c b/t/helper/test-long-is-64bit.c
new file mode 100644
index 00..45fc120432
--- /dev/null
+++ b/t/helper/test-long-is-64bit.c
@@ -0,0 +1,6 @@
+#include "git-compat-util.h"
+
+int cmd_main(int argc, const char **argv)
+{
+   return (8 <= (int)sizeof(long)) ? 0 : 1;
+}
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ec2571f018..bf649fbc03 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1165,14 +1165,7 @@ run_with_limited_cmdline () {
 
 test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'
 
-build_option () {
-   git version --build-options |
-   sed -ne "s/^$1: //p"
-}
-
-test_lazy_prereq LONG_IS_64BIT '
-   test 8 -le "$(build_option sizeof-long)"
-'
+test_lazy_prereq LONG_IS_64BIT 'test-long-is-64bit'
 
 test_lazy_prereq TIME_IS_64BIT 'test-date is64bit'
 test_lazy_prereq TIME_T_IS_64BIT 'test-date time_t-is64bit'
-- 
2.13.0.303.g4ebf302169

Re: [PATCH] doc: Improve description for rev-parse --short

[Andreas Heiduk]

Am 30.05.2017 um 06:10 schrieb Junio C Hamano:
>>  --short=number::
>>  Instead of outputting the full SHA-1 values of object names try to
>>  abbreviate them to a shorter unique name. When no length is specified
>> -7 is used. The minimum length is 4.
>> +the effective value of the configuration variable `core.abbrev` (see
>> +linkgit:git-config[1]) is used.  The minimum length is 4.  The length
>> +may be exceeded to ensure unique object names.  Implies `--verify`.
> 
> "Implies --verify" is less important than the fact that multiple
> object names cannot be given from the end-users' (and readers')
> point of view, no?  The sentence in the pre-context still hints
> (incorrectly) that we might take multiple names---that would want to
> be corrected, no?
> 
> Let me try.
> 
> --short[=length]::
>   Take a single object name, and output a prefix of the object
>   name whose length is at least the specified length and
>   sufficient to ensure uniqueness of the name.  The minimum
>   length is 4.  When no length is given, the effective value
>   of the `core.abbrev` configuration variable is used.
> 
> Thanks.

Your are right about s/names/name/ in the pretext.

But I think that the link to the `--verify` option is still important.
The text there talks about when something is output, exit codes and
about `^{type}` peeling. Also `--quiet` is linked to
`--verify` and hence relevant here.

So I'd like to patch your text to this:

  --short[=length]::
Same as `--verify` but output only a prefix of the object
name whose length is at least the specified length and
sufficient to ensure uniqueness of the name.  The minimum
length is 4.  When no length is given, the effective value
of the `core.abbrev` configuration variable is used.

And I'd like to move the section up right to `--verify` and `--quiet`.
The options in this section are not sorted alphabetically anyways and
the relevant parts would be adjacent. Is that OK?

ALso: Did you remove the `linkgit` by intention or just by accident?

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Johannes Sixt]

Am 30.05.2017 um 06:46 schrieb Junio C Hamano:

Johannes Sixt  writes:


Doesn't this need test_i18ngrep?:


Good catch! It would be this one in warn_on_inaccessible:


  wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);


But actually, I'm more worried about the unholy mix of
one-test-first-then-skip_all-later that occurs in this test script (I
do not mean the skip_all that is visible in the context, there are
others later). I think there was some buzz recently that prove only
understands a summary line that reads "1..0", but here we would see
"1..1". What to do? Reorganize the test script? Dscho, any ideas?


For now I've queued this on top of 1/2, so that suggestions are not
lost, and then tweaked 2/2 (as context for the patch to the test
changes).

Either an ack or a reroll is appreciated (I do not think we'd
terribly mind if this test were added to another script, or if this
test were skipped when UNC path cannot be determined even though it
does not need that prereq.  Also UNC_PATH can become prereq that is
tested by individual test in this script and the new test can be
added without requiring that prereq).

Thanks.

  t/t5580-clone-push-unc.sh | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index fd719a209e..944730cddc 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -8,12 +8,6 @@ if ! test_have_prereq MINGW; then
test_done
  fi
  
-test_expect_failure 'remote nick cannot contain backslashes' '

-   BACKSLASHED="$(pwd | tr / )" &&
-   git ls-remote "$BACKSLASHED" >out 2>err &&
-   ! grep "unable to access" err
-'
-
  UNCPATH="$(pwd)"
  case "$UNCPATH" in
  [A-Z]:*)
@@ -51,4 +45,10 @@ test_expect_success push '
test "$rev" = "$(git rev-parse --verify refs/heads/to-push)"
  '
  
+test_expect_failure 'remote nick cannot contain backslashes' '

+   BACKSLASHED="$(pwd | tr / )" &&
+   git ls-remote "$BACKSLASHED" >out 2>err &&
+   test_i18ngrep ! "unable to access" err
+'
+
  test_done



ACK!

-- Hannes

Re: [PATCH v2 0/6] Fast git status via a file system watcher

[Christian Couder]

On Tue, May 30, 2017 at 8:05 PM, Ben Peart  wrote:
>
>
> On 5/27/2017 2:57 AM, Christian Couder wrote:
>>
>> On Thu, May 25, 2017 at 3:55 PM, Ben Peart  wrote:
>>>
>>>
>>>
>>> On 5/24/2017 6:54 AM, Christian Couder wrote:
>
>
> Design
> ~~
>
> A new git hook (query-fsmonitor) must exist and be enabled
> (core.fsmonitor=true) that takes a time_t formatted as a string and
> outputs to stdout all files that have been modified since the requested
> time.



 Is there a reason why there is a new hook, instead of a
 "core.fsmonitorquery" config option to which you could pass whatever
 command line with options?
>>>
>>>
>>>
>>> A hook is a simple and well defined way to integrate git with another
>>> process.  If there is some fixed set of arguments that need to be passed
>>> to
>>> a file system monitor (beyond the timestamp stored in the index
>>> extension),
>>> they can be encoded in the integration script like I've done in the
>>> Watchman
>>> integration sample hook.
>>
>>
>> Yeah, they could be encoded in the integration script, but it could be
>> better if it was possible to just configure a generic command line.
>>
>> For example if the directory that should be watched for filesystem
>> changes could be passed as well as the time since the last changes,
>> perhaps only a generic command line would be need.
>
>
> Maybe I'm not understanding what you have in mind but I haven't found this
> to be the case in the two integrations I've done with file system watchers
> (one internal and Watchman).  They require you download, install, and
> configure them by telling them about the folders you want monitored.  Then
> you can start querying them for changes and processing the output to match
> what git expects.  While the download and install steps vary, having that
> query + process and return results wrapped up in an integration hook has
> worked well.

It looks like one can also just ask watchman to monitor a directory with:

watchman watch /path/to/dir

or:

echo '["watch", "/path/to/dir"]' | watchman -j

Also for example on Linux people might want to use command line tools like:

https://linux.die.net/man/1/inotifywait

and you can pass the directories you want to be watched as arguments
to this kind of tools.

So it would be nice, if we didn't require the user to configure
anything and we could just configure the watching of what we need in
the hook (or a command configured using a config option). If the hook
(or configured command) could be passed the directory by git, it could
also be generic.

>> I am also wondering about sparse checkout, as we might want to pass
>> all the directories we are interested in.
>> How is it supposed to work with sparse checkout?
>
> The fsmonitor code works well with or without a sparse-checkout.  The file
> system monitor is unaware of the sparse checkout so will notify git about
> any change irrespective of whether git will eventually ignore it because the
> skip worktree bit is set.

I was wondering if it could ease the job for the monitoring service
and perhaps improve performance to just ask to watch the directories
we are interested in when using sparse checkout.
On Linux it looks like a separate inotify watch is created for every
subdirectory and there is maximum amount of inotify watches per user.
This can be increased by writing in
/proc/sys/fs/inotify/max_user_watches, but it is not nice to have to
ask admins to increase this.

Re: [PATCH v4 00/10] The final building block for a faster rebase -i

[Ævar Arnfjörð Bjarmason]

On Tue, May 30, 2017 at 5:44 PM, Johannes Schindelin
 wrote:
> Hi Ævar,
>
> On Mon, 29 May 2017, Ævar Arnfjörð Bjarmason wrote:
>
>> On Mon, May 29, 2017 at 12:51 PM, Johannes Schindelin
>>  wrote:
>> >
>> > On Sat, 27 May 2017, René Scharfe wrote:
>> >> Am 26.05.2017 um 05:15 schrieb Liam Beguin:
>> >> > I tried to time the execution on an interactive rebase (on Linux)
>> >> > but I did not notice a significant change in speed.  Do we have a
>> >> > way to measure performance / speed changes between version?
>> >>
>> >> Well, there's performance test script p3404-rebase-interactive.sh.
>> >> You could run it e.g. like this:
>> >>
>> >>   $ (cd t/perf && ./run origin/master HEAD ./p3404*.sh)
>> >>
>> >> This would compare the performance of master with the current branch
>> >> you're on.  The results of p3404 are quite noisy for me on master,
>> >> though (saw 15% difference between runs without any code changes), so
>> >> take them with a bag of salt.
>> >
>> > Indeed. Our performance tests are simply not very meaningful.
>> >
>> > Part of it is the use of shell scripting (which defeats performance
>> > testing pretty well),
>>
>> Don't the performance tests take long enough that the shellscripting
>> overhead gets lost in the noise?
>
> Okay, here you go, my weekly (or so) clarification about the POSIX
> emulation layer called MSYS2 (which itself kind of a portable Cygwin).
>
> Whenever Git for Windows has to execute Unix shell scripts (which are not
> native to Windows, as the "Unix" in "Unix shell scripts" so clearly
> suggests), we resort to calling the Bash from the MSYS2 project, which
> spins up a POSIX emulation layer. Git for Windows' own .exe files
> (and in particular, git.exe) is *not* affected by the POSIX emulation
> layer, as they are real Win32 programs.
>
> Whenever execution has to bridge into, or out of, the POSIX emulation
> layer, a few things need to be done. To emulate signal handling, for
> example, a completely new process has to be spun up that itself has the
> non-MSYS2 process as a child. The environment has to be converted, to
> reflect the fact that some things are Unix-y paths (or path lists) inside
> the POSIX emulation layer and Windows paths outside.
>
> Even when staying within the POSIX emulation layer, some operations are
> not as cheap as "Linux folks" are used to. For example, to spawn a
> subshell, due to the absence of a spawn syscall fork() is called, followed
> by exec(). However, fork() itself is not native to Windows and has to be
> emulated. The POSIX emulation layer spins up a new process, meticulously
> copies the entire memory, tries to reopen the file descriptors, network
> connections, etc (to emulate the fork() semantics).
>
> Obviously, this is anything but cheap.
>
> And this is only a glimpse into the entire problem, as I am not aware of
> any thorough analysis what is going on in msys-2.0.dll when shell scripts
> run. All I know is that it slows things down dramatically.
>
> As a consequence, even the simple act of creating a repository, or
> spawning Win32 processes from within a shell, become quite the
> contributing factors to the noise of the measurements.
>
>> E.g. on Windows what do you get when you run this in t/perf:
>>
>> $ GIT_PERF_REPEAT_COUNT=3 GIT_PERF_MAKE_OPTS="-j6 NO_OPENSSL=Y
>> BLK_SHA1=Y CFLAGS=-O3" ./run v2.10.0 v2.12.0 v2.13.0 p3400-rebase.sh
>
> In my hands, a repeat count of 3 always resulted in quite a bit of noise
> previously.
>
> Mind you, I am working my machine. It has to run two VMs in the
> background, has multiple browsers and dozens of tabs open, checks for
> mails and Tweets and RSS feeds and a couple of Skypes are open, too.
>
> So yeah, obviously there is a bit of noise involved.
>
>> I get split-index performance improving by 28% in 2.12 and 58% in
>> 2.13, small error bars even with just 3 runs. This is on Linux, but my
>> sense of fork overhead on Windows is that it isn't so bad as to matter
>> here.
>
> Test
> --
> 3400.2: rebase on top of a lot of unrelated changes
>
> v2.10.0v2.12.0  v2.13.0
> --
> 60.65(0.01+0.03)   55.75(0.01+0.07) -8.1%   55.97(0.04+0.09) -7.7%
>
> (wrapped myself, as the ./run output is a lot wider than the 80 columns
> allowed in plain text email format)
>
> And what does it tell you?

For all the above: Yes I get that various things are slower on
Windows, but I think that regardless of that by far the majority of
the time in most of the perf tests is spent on the code being tested,
so it doesn't come into play:

$ parallel -k -j4 '(printf "%s\t" {} && (time GIT_PERF_REPO=~/g/linux
GIT_PERF_LARGE_REPO=~/g/linux ./run {}) 2>&1 | grep ^real | grep -o
[0-9].*) | awk "{print \$2 \" \" \$1}"' ::: p[0-9]*sh
0m34.333s p-perf-lib-sanity.sh
4m26.415s p0001-rev-list.sh
1m41.647s 

Re: [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows

[Stefan Beller]

On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano  wrote:
> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
> on the filesystem, Windows (correctly) fails to open it but sets
> EINVAL to errno

errno to EINVAL (as of now it sounds as if it is a EINVAL = errno,
which makes no sense to me)

> because the pathname has characters that cannot be
> stored in its filesystem.
>
> As this is an expected failure, teach is_missing_file_error() helper
> about this case.
>
> This is RFC,

cc'd people knowledgeable of Windows.

> as there may be a case where we get EINVAL from
> open/fopen for reasons other than "the filesystem does not like this
> pathname" that may be worth reporting to the user, and this change
> is sweeping such an error under the rug.
>
> Signed-off-by: Junio C Hamano 
> ---
>  wrapper.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/wrapper.c b/wrapper.c
> index f1c87ec7ea..74aa3b7803 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -434,6 +434,10 @@ static void warn_on_inaccessible(const char *path)
>   * see if the errno indicates a missing file that we can safely ignore.
>   */
>  static int is_missing_file_error(int errno_) {
> +#ifdef GIT_WINDOWS_NATIVE
> +   if (errno_ == EINVAL)
> +   return 1;
> +#endif
> return (errno_ == ENOENT || errno_ == ENOTDIR);
>  }
>
> --
> 2.13.0-491-g71cfeddc25
>

Re: [PATCH v3 10/13] rerere.c: move error_errno() closer to the source system call

[Stefan Beller]

On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano  wrote:
> From: Nguyễn Thái Ngọc Duy 
>
> We are supposed to report errno from fopen(). fclose() between fopen()
> and the report function could either change errno or reset it.
>
> Signed-off-by: Junio C Hamano 
> ---
>  rerere.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/rerere.c b/rerere.c
> index 1351b0c3fb..c26c29f87a 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -489,8 +489,9 @@ static int handle_file(const char *path, unsigned char 
> *sha1, const char *output
> if (output) {
> io.io.output = fopen(output, "w");
> if (!io.io.output) {
> +   error_errno("Could not write %s", output);
> fclose(io.input);
> -   return error_errno("Could not write %s", output);
> +   return -1;

and here we assume fclose doesn't error out.

This is ok as we are sure it cannot be EBADF as we just opened that
fd and as we did not read from io.input yet, any errno from close(2), fflush(3)
are not expected?

close(2) may yield an EINTR, in which case we may want to

  while (fclose(io.input) && errno != EINTR)
; /*do nothing*/

On the other hand this is already in the error path, so maybe we're ok
leaking an fd here in case of a signal?

Re: [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too

[Stefan Beller]

On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano  wrote:

Do we have any reasons for that, or pointers on the mailing list, that this
is a good idea or needed? Does it fix a bug or enable a new feature on Darwin?

> Signed-off-by: Junio C Hamano 
> ---
>  config.mak.uname | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index a25ffddb3e..1743890164 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -110,6 +110,7 @@ ifeq ($(uname_S),Darwin)
> BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
> BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1
> HAVE_BSD_SYSCTL = YesPlease
> +   FREAD_READS_DIRECTORIES = UnfortunatelyYes
>  endif
>  ifeq ($(uname_S),SunOS)
> NEEDS_SOCKET = YesPlease
> --
> 2.13.0-491-g71cfeddc25
>

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[liam Beguin]

Hi Johannes,

Johannes Schindelin  writes:
> The first step of an interactive rebase is to generate the so-called "todo
> script", to be stored in the state directory as "git-rebase-todo" and to
> be edited by the user.
>
> Originally, we adjusted the output of `git log ` using a simple
> sed script. Over the course of the years, the code became more
> complicated. We now use shell scripting to edit the output of `git log`
> conditionally, depending whether to keep "empty" commits (i.e. commits
> that do not change any files).
>
> On platforms where shell scripting is not native, this can be a serious
> drag. And it opens the door for incompatibilities between platforms when
> it comes to shell scripting or to Unix-y commands.
>
> Let's just re-implement the todo script generation in plain C, using the
> revision machinery directly.
>
> This is substantially faster, improving the speed relative to the
> shell script version of the interactive rebase from 2x to 3x on Windows.
>
> Note that the rearrange_squash() function in git-rebase--interactive
> relied on the fact that we set the "format" variable to the config setting
> rebase.instructionFormat. Relying on a side effect like this is no good,
> hence we explicitly perform that assignment (possibly again) in
> rearrange_squash().
>
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/rebase--helper.c   |  8 +++-
>  git-rebase--interactive.sh | 44 +
>  sequencer.c| 49 
> ++
>  sequencer.h|  3 +++
>  4 files changed, 82 insertions(+), 22 deletions(-)
>
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index ca1ebb2fa18..821058d452d 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -11,15 +11,19 @@ static const char * const builtin_rebase_helper_usage[] = 
> {
>  int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  {
>   struct replay_opts opts = REPLAY_OPTS_INIT;
> + int keep_empty = 0;
>   enum {
> - CONTINUE = 1, ABORT
> + CONTINUE = 1, ABORT, MAKE_SCRIPT
>   } command = 0;
>   struct option options[] = {
>   OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
> + OPT_BOOL(0, "keep-empty", _empty, N_("keep empty 
> commits")),
>   OPT_CMDMODE(0, "continue", , N_("continue rebase"),
>   CONTINUE),
>   OPT_CMDMODE(0, "abort", , N_("abort rebase"),
>   ABORT),
> + OPT_CMDMODE(0, "make-script", ,
> + N_("make rebase script"), MAKE_SCRIPT),
>   OPT_END()
>   };

This is probably being picky, but we could also use a different name
here instead of 'rebase script'. This would help keep the name of this
script consistent as you already pointed out.
maybe something like 'make-todo-list' or just 'make-list'?

>  
> @@ -36,5 +40,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   return !!sequencer_continue();
>   if (command == ABORT && argc == 1)
>   return !!sequencer_remove_state();
> + if (command == MAKE_SCRIPT && argc > 1)
> + return !!sequencer_make_script(keep_empty, stdout, argc, argv);

same here.

>   usage_with_options(builtin_rebase_helper_usage, options);
>  }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 2c9c0165b5a..609e150d38f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -785,6 +785,7 @@ collapse_todo_ids() {
>  # each log message will be re-retrieved in order to normalize the
>  # autosquash arrangement
>  rearrange_squash () {
> + format=$(git config --get rebase.instructionFormat)
>   # extract fixup!/squash! lines and resolve any referenced sha1's
>   while read -r pick sha1 message
>   do
> @@ -1210,26 +1211,27 @@ else
>   revisions=$onto...$orig_head
>   shortrevisions=$shorthead
>  fi
> -format=$(git config --get rebase.instructionFormat)
> -# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H 
> to parse
> -git rev-list $merges_option --format="%m%H ${format:-%s}" \
> - --reverse --left-right --topo-order \
> - $revisions ${restrict_revision+^$restrict_revision} | \
> - sed -n "s/^>//p" |
> -while read -r sha1 rest
> -do
> -
> - if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit 
> $sha1
> - then
> - comment_out="$comment_char "
> - else
> - comment_out=
> - fi
> +if test t != "$preserve_merges"
> +then
> + git rebase--helper --make-script ${keep_empty:+--keep-empty} \
> + $revisions ${restrict_revision+^$restrict_revision} >"$todo"
> +else
> + format=$(git config --get rebase.instructionFormat)
> + # the 'rev-list .. | 

Re: git push recurse.submodules behavior changed in 2.13

[Jonathan Nieder]

Hi John,

John Shahid wrote:

> It looks like the git push recurse-submodules behavior has changed.
> Currently with 2.13 you cannot run "git push
> --recurse-submodules=on-demand" if the parent repo is on a different
> branch than the sub repos, e.g. parent repo is on "develop" and
> sub-repo on "master". I created a test that can be found here [1].
>
> A bisect shows that the change to propagate refspec [2] to the
> submodules is the culprit. imho this is an undesired change in
> behavior. I looked at the code but couldn't see an easy way to fix
> this issue without breaking the feature mentioned above. The only
> option I can think of is to control the refspec propagation behavior
> using a flag, e.g. "--propagate-refspecs" or add another
> recurse-submodules option, e.g. "--recurse-submodules=propagate"

Thanks for reporting.  The old behavior of "git push
--recurse-submodules" was a little loose (it would run "git push" in
the submodule and rely on defaults to determine the behavior) and when
propagating refspecs we considered taking the opportunity to tighten
that by propagating the default refspec when you run "git push
--recurse-submodules" without a refspec.

We didn't make that change.  If starting over, it might be a natural
thing to do, but we didn't want to break existing users.  So

  git push --recurse-submodules=on-demand

should continue to work as before.  *puzzled*

[...]
> We have a parent repo on a branch called "develop" and a submodule on
> a branch called "master". Prior to git version 2.13 if we had an
> unpushed commit in the submodule and ran "git push origin develop
> --recurse-submodules=on-demand" git would happily push the develop
> branch of the parent repo and the master branch of the submodule,
[...]
> pushing to ref refs/heads/develop:refs/heads/develop
> pushging to remote origin
> fatal: src refspec 'refs/heads/develop' must name a ref
> fatal: process for submodule 'sub' failed
> fatal: The remote end hung up unexpectedly

Aha, you are passing a refspec!

Can you say more about how this change affects you?  Would you be able
to push without a refspec, or do you e.g. have scripting that was
relying on the existing loose behavior?

To put this in more context: part of the ideal that
--recurse-submodules options are trying to achieve is to allow
thinking of a repository as a single unit, including submodules, most
of the time.  To this end, "git clone --recurse-submodules" clones a
repository including its submodules, "git checkout
--recurse-submodules" checks out submodules as part of a checkout
operation, avoiding a separate "git submodule update --init" operation,
"git fetch --recurse-submodules" fetches upstream changes for both the
parent repository and submodules, and so on.

Unfortunately "git checkout --recurse-submodules" was missing a piece,
impacting this workflow.  If I run

git checkout --recurse-submodules -b develop

then today this creates a "develop" branch in the superproject but not
within submodules. This hurts the illusion of the project being a single
unit and makes the meaning of operations like

git push --recurse-submodules develop:master

unclear.

Ideas for next steps:

 1. If "git checkout --recurse-submodules -b develop" does the right
thing, then this situation is harder to get in in the first place.
The series at [3] is meant to do that.

 2. The error message shown above is very unclear. It leaks
implementation details and doesn't give the user a clear hint
about what to do to resolve it.  Would some message like the
following have helped?

$ git push --recurse-submodules origin develop
fatal: branch "develop" does not exist in submodule "sub"
hint: use "git -C sub branch develop " to create such a branch
hint: or push without a branch name to push the current branch

 3. Outside of the output from any particular command, we are missing
an equivalent of "man gitworkflows" to describe how the
--recurse-submodules options are meant to work.  The series at [4]
aims to start addressing that.

Given all the above, this looks like a real and serious bug but I
think to find a different way from --recurse-submodules=propagate to
fix it.  The idea, especially with [5], is for submodules to work
intuitively without requiring special options.  This can require more
work in the short term getting the default behavior to work well with
a variety of use cases but I think it is time well spent.

Thoughts?

Thanks and hope that helps,
Jonathan

> [1] https://gist.github.com/jvshahid/b778702cc3d825c6887d2707e866a9c8
> [2] https://github.com/git/git/commit/06bf4ad1db92c32af38e16d9b7f928edbd647780
[3] http://public-inbox.org/git/20170501180058.8063-1-sbel...@google.com/
[4] http://public-inbox.org/git/20170209020855.23486-1-sbel...@google.com/
[5] http://public-inbox.org/git/20170526191017.19155-1-sbel...@google.com/

Re: [PATCH v2 0/6] Fast git status via a file system watcher

[Ben Peart]

On 5/27/2017 2:57 AM, Christian Couder wrote:

On Thu, May 25, 2017 at 3:55 PM, Ben Peart  wrote:



On 5/24/2017 6:54 AM, Christian Couder wrote:


Design
~~

A new git hook (query-fsmonitor) must exist and be enabled
(core.fsmonitor=true) that takes a time_t formatted as a string and
outputs to stdout all files that have been modified since the requested
time.



Is there a reason why there is a new hook, instead of a
"core.fsmonitorquery" config option to which you could pass whatever
command line with options?



A hook is a simple and well defined way to integrate git with another
process.  If there is some fixed set of arguments that need to be passed to
a file system monitor (beyond the timestamp stored in the index extension),
they can be encoded in the integration script like I've done in the Watchman
integration sample hook.


Yeah, they could be encoded in the integration script, but it could be
better if it was possible to just configure a generic command line.

For example if the directory that should be watched for filesystem
changes could be passed as well as the time since the last changes,
perhaps only a generic command line would be need.


Maybe I'm not understanding what you have in mind but I haven't found 
this to be the case in the two integrations I've done with file system 
watchers (one internal and Watchman).  They require you download, 
install, and configure them by telling them about the folders you want 
monitored.  Then you can start querying them for changes and processing 
the output to match what git expects.  While the download and install 
steps vary, having that query + process and return results wrapped up in 
an integration hook has worked well.




I am also wondering about sparse checkout, as we might want to pass
all the directories we are interested in.
How is it supposed to work with sparse checkout?



The fsmonitor code works well with or without a sparse-checkout.  The 
file system monitor is unaware of the sparse checkout so will notify git 
about any change irrespective of whether git will eventually ignore it 
because the skip worktree bit is set.



A new 'fsmonitor' index extension has been added to store the time the
fsmonitor hook was last queried and a ewah bitmap of the current
'fsmonitor-dirty' files. Unmarked entries are 'fsmonitor-clean', marked
entries are 'fsmonitor-dirty.'

As needed, git will call the query-fsmonitor hook proc for the set of
changes since the index was last updated. Git then uses this set of
files along with the list saved in the fsmonitor index extension to flag
the potentially dirty index and untracked cache entries.



So this can work only if "core.untrackedCache" is set to true?



This works with core.untrackedCache set to true or false.  If it is set to
false, you get valid results, you just don't get the speed up when checking
for untracked files.


Great!

Re: What's cooking in git.git (May 2017, #08; Mon, 29)

[Stefan Beller]

On Sun, May 28, 2017 at 11:23 PM, Junio C Hamano  wrote:

> A bit more topics are now in 'master'.  One unfortunate thing is
> that the SHA1 breakage in 2.13 for big-endian platforms were lost in
> the noise with excitement felt by some subset of contributors with
> the possible use of submodules.

I am sorry about being excited, without considering the immediate
pressing issues.

> The first step in the series is
> neutral to the excitement, and should be fast-tracked down to
> 'maint' soonish.

Yes I agree on that. Thanks for being calm and unexcited about that!

>
>
> * sb/diff-color-move (2017-05-25) 17 commits
>  - diff.c: color moved lines differently
...
>
>  "git diff" has been taught to optionally paint new lines that are
>  the same as deleted lines elsewhere differently from genuinely new
>  lines.

My current understanding is that we agree on having the first n-1 patches
in good shape[1] and are only discussing how the exact line coloring
algorithm should look like, so I resent that separately[2]. While
it has better documentation and tests (also a command line option)
Philip still found some issues in there, so I will revisit that patch once
more.

[1] https://public-inbox.org/git/xmqq7f15e8pu@gitster.mtv.corp.google.com/
[2] https://public-inbox.org/git/20170527001820.25214-2-sbel...@google.com/

>
> * sb/submodule-blanket-recursive (2017-05-23) 6 commits
...
>
>  Retracted for now.
>  cf. 

[PATCH 12/33] diff: convert run_diff_files to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 diff-lib.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 7984ff962..c82b07dc1 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -101,7 +101,7 @@ int run_diff_files(struct rev_info *revs, unsigned int 
option)
struct cache_entry *ce = active_cache[i];
int changed;
unsigned dirty_submodule = 0;
-   const unsigned char *old_sha1, *new_sha1;
+   const struct object_id *old_oid, *new_oid;
 
if (diff_can_quit_early(>diffopt))
break;
@@ -233,12 +233,12 @@ int run_diff_files(struct rev_info *revs, unsigned int 
option)
continue;
}
oldmode = ce->ce_mode;
-   old_sha1 = ce->oid.hash;
-   new_sha1 = changed ? null_sha1 : ce->oid.hash;
+   old_oid = >oid;
+   new_oid = changed ? _oid : >oid;
diff_change(>diffopt, oldmode, newmode,
-   old_sha1, new_sha1,
-   !is_null_sha1(old_sha1),
-   !is_null_sha1(new_sha1),
+   old_oid->hash, new_oid->hash,
+   !is_null_oid(old_oid),
+   !is_null_oid(new_oid),
ce->name, 0, dirty_submodule);
 
}
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 11/33] diff: convert diff_addremove to struct object_id

[Brandon Williams]

Convert diff_addremove to take a struct object_id.  In addtion convert
the function pointer type 'add_remove_fn_t' to also take a struct
object_id.

Signed-off-by: Brandon Williams 
---
 diff-lib.c  | 6 +++---
 diff.c  | 8 
 diff.h  | 8 
 revision.c  | 4 ++--
 tree-diff.c | 8 
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 2c838aaf4..7984ff962 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -210,14 +210,14 @@ int run_diff_files(struct rev_info *revs, unsigned int 
option)
continue;
}
diff_addremove(>diffopt, '-', ce->ce_mode,
-  ce->oid.hash,
+  >oid,
   !is_null_oid(>oid),
   ce->name, 0);
continue;
} else if (revs->diffopt.ita_invisible_in_index &&
   ce_intent_to_add(ce)) {
diff_addremove(>diffopt, '+', ce->ce_mode,
-  EMPTY_BLOB_SHA1_BIN, 0,
+  _tree_oid, 0,
   ce->name, 0);
continue;
}
@@ -260,7 +260,7 @@ static void diff_index_show_file(struct rev_info *revs,
 unsigned dirty_submodule)
 {
diff_addremove(>diffopt, prefix[0], mode,
-  oid->hash, oid_valid, ce->name, dirty_submodule);
+  oid, oid_valid, ce->name, dirty_submodule);
 }
 
 static int get_stat_data(const struct cache_entry *ce,
diff --git a/diff.c b/diff.c
index f3546536b..3fa335f44 100644
--- a/diff.c
+++ b/diff.c
@@ -5081,8 +5081,8 @@ static int is_submodule_ignored(const char *path, struct 
diff_options *options)
 
 void diff_addremove(struct diff_options *options,
int addremove, unsigned mode,
-   const unsigned char *sha1,
-   int sha1_valid,
+   const struct object_id *oid,
+   int oid_valid,
const char *concatpath, unsigned dirty_submodule)
 {
struct diff_filespec *one, *two;
@@ -5114,9 +5114,9 @@ void diff_addremove(struct diff_options *options,
two = alloc_filespec(concatpath);
 
if (addremove != '+')
-   fill_filespec(one, sha1, sha1_valid, mode);
+   fill_filespec(one, oid->hash, oid_valid, mode);
if (addremove != '-') {
-   fill_filespec(two, sha1, sha1_valid, mode);
+   fill_filespec(two, oid->hash, oid_valid, mode);
two->dirty_submodule = dirty_submodule;
}
 
diff --git a/diff.h b/diff.h
index d75e6d15e..1086975a5 100644
--- a/diff.h
+++ b/diff.h
@@ -31,8 +31,8 @@ typedef void (*change_fn_t)(struct diff_options *options,
 
 typedef void (*add_remove_fn_t)(struct diff_options *options,
int addremove, unsigned mode,
-   const unsigned char *sha1,
-   int sha1_valid,
+   const struct object_id *oid,
+   int oid_valid,
const char *fullpath, unsigned dirty_submodule);
 
 typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
@@ -247,8 +247,8 @@ extern int diff_can_quit_early(struct diff_options *);
 extern void diff_addremove(struct diff_options *,
   int addremove,
   unsigned mode,
-  const unsigned char *sha1,
-  int sha1_valid,
+  const struct object_id *oid,
+  int oid_valid,
   const char *fullpath, unsigned dirty_submodule);
 
 extern void diff_change(struct diff_options *,
diff --git a/revision.c b/revision.c
index 475d5b2dc..71519193c 100644
--- a/revision.c
+++ b/revision.c
@@ -401,8 +401,8 @@ static int tree_difference = REV_TREE_SAME;
 
 static void file_add_remove(struct diff_options *options,
int addremove, unsigned mode,
-   const unsigned char *sha1,
-   int sha1_valid,
+   const struct object_id *oid,
+   int oid_valid,
const char *fullpath, unsigned dirty_submodule)
 {
int diff = addremove == '+' ? REV_TREE_NEW : REV_TREE_OLD;
diff --git a/tree-diff.c b/tree-diff.c
index e164e532b..f2c747ea5 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -78,21 +78,21 @@ static int emit_diff_first_parent_only(struct diff_options 
*opt, struct combine_
1, 1, p->path, 0, 0);
}
else {
-   const unsigned char *sha1;
+   const struct 

[PATCH 32/33] diffcore-rename: use is_empty_blob_oid

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 diffcore-rename.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 3d9719dad..03d1e8d40 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -464,7 +464,7 @@ void diffcore_rename(struct diff_options *options)
 strcmp(options->single_follow, p->two->path))
continue; /* not interested */
else if (!DIFF_OPT_TST(options, RENAME_EMPTY) &&
-is_empty_blob_sha1(p->two->oid.hash))
+is_empty_blob_oid(>two->oid))
continue;
else if (add_rename_dst(p->two) < 0) {
warning("skipping rename detection, detected"
@@ -474,7 +474,7 @@ void diffcore_rename(struct diff_options *options)
}
}
else if (!DIFF_OPT_TST(options, RENAME_EMPTY) &&
-is_empty_blob_sha1(p->one->oid.hash))
+is_empty_blob_oid(>one->oid))
continue;
else if (!DIFF_PAIR_UNMERGED(p) && !DIFF_FILE_VALID(p->two)) {
/*
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 28/33] builtin/diff-tree: cleanup references to sha1

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 builtin/diff-tree.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index aef167619..8b26a66a9 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -7,7 +7,7 @@
 
 static struct rev_info log_tree_opt;
 
-static int diff_tree_commit_sha1(const struct object_id *oid)
+static int diff_tree_commit_oid(const struct object_id *oid)
 {
struct commit *commit = lookup_commit_reference(oid);
if (!commit)
@@ -98,7 +98,6 @@ static void diff_tree_tweak_rev(struct rev_info *rev, struct 
setup_revision_opt
 
 int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 {
-   int nr_sha1;
char line[1000];
struct object *tree1, *tree2;
static struct rev_info *opt = _tree_opt;
@@ -132,15 +131,14 @@ int cmd_diff_tree(int argc, const char **argv, const char 
*prefix)
 * reverse the order of the objects if the second one
 * is marked UNINTERESTING.
 */
-   nr_sha1 = opt->pending.nr;
-   switch (nr_sha1) {
+   switch (opt->pending.nr) {
case 0:
if (!read_stdin)
usage(diff_tree_usage);
break;
case 1:
tree1 = opt->pending.objects[0].item;
-   diff_tree_commit_sha1(>oid);
+   diff_tree_commit_oid(>oid);
break;
case 2:
tree1 = opt->pending.objects[0].item;
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 27/33] diff-tree: convert diff_tree_sha1 to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 builtin/blame.c   | 20 ++--
 builtin/diff-tree.c   |  8 +++-
 builtin/diff.c|  2 +-
 builtin/fast-export.c |  4 ++--
 builtin/log.c |  6 +++---
 builtin/merge.c   |  2 +-
 combine-diff.c|  4 ++--
 diff.h|  5 +++--
 line-log.c|  4 ++--
 log-tree.c|  8 
 merge-recursive.c |  2 +-
 notes-merge.c |  4 ++--
 patch-ids.c   |  4 ++--
 revision.c|  4 ++--
 sequencer.c   |  4 ++--
 tree-diff.c   | 12 +++-
 16 files changed, 47 insertions(+), 46 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 5ad435380..7645aa991 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -565,9 +565,9 @@ static struct origin *find_origin(struct scoreboard *sb,
if (is_null_oid(>commit->object.oid))
do_diff_cache(>tree->object.oid, _opts);
else
-   diff_tree_sha1(parent->tree->object.oid.hash,
-  origin->commit->tree->object.oid.hash,
-  "", _opts);
+   diff_tree_oid(>tree->object.oid,
+ >commit->tree->object.oid,
+ "", _opts);
diffcore_std(_opts);
 
if (!diff_queued_diff.nr) {
@@ -635,9 +635,9 @@ static struct origin *find_rename(struct scoreboard *sb,
if (is_null_oid(>commit->object.oid))
do_diff_cache(>tree->object.oid, _opts);
else
-   diff_tree_sha1(parent->tree->object.oid.hash,
-  origin->commit->tree->object.oid.hash,
-  "", _opts);
+   diff_tree_oid(>tree->object.oid,
+ >commit->tree->object.oid,
+ "", _opts);
diffcore_std(_opts);
 
for (i = 0; i < diff_queued_diff.nr; i++) {
@@ -1262,7 +1262,7 @@ static void find_copy_in_parent(struct scoreboard *sb,
/* Try "find copies harder" on new path if requested;
 * we do not want to use diffcore_rename() actually to
 * match things up; find_copies_harder is set only to
-* force diff_tree_sha1() to feed all filepairs to diff_queue,
+* force diff_tree_oid() to feed all filepairs to diff_queue,
 * and this code needs to be after diff_setup_done(), which
 * usually makes find-copies-harder imply copy detection.
 */
@@ -1274,9 +1274,9 @@ static void find_copy_in_parent(struct scoreboard *sb,
if (is_null_oid(>commit->object.oid))
do_diff_cache(>tree->object.oid, _opts);
else
-   diff_tree_sha1(parent->tree->object.oid.hash,
-  target->commit->tree->object.oid.hash,
-  "", _opts);
+   diff_tree_oid(>tree->object.oid,
+ >commit->tree->object.oid,
+ "", _opts);
 
if (!DIFF_OPT_TST(_opts, FIND_COPIES_HARDER))
diffcore_std(_opts);
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 5ea1c1431..aef167619 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -49,8 +49,8 @@ static int stdin_diff_trees(struct tree *tree1, const char *p)
return -1;
printf("%s %s\n", oid_to_hex(>object.oid),
  oid_to_hex(>object.oid));
-   diff_tree_sha1(tree1->object.oid.hash, tree2->object.oid.hash,
-  "", _tree_opt.diffopt);
+   diff_tree_oid(>object.oid, >object.oid,
+ "", _tree_opt.diffopt);
log_tree_diff_flush(_tree_opt);
return 0;
 }
@@ -148,9 +148,7 @@ int cmd_diff_tree(int argc, const char **argv, const char 
*prefix)
if (tree2->flags & UNINTERESTING) {
SWAP(tree2, tree1);
}
-   diff_tree_sha1(tree1->oid.hash,
-  tree2->oid.hash,
-  "", >diffopt);
+   diff_tree_oid(>oid, >oid, "", >diffopt);
log_tree_diff_flush(opt);
break;
}
diff --git a/builtin/diff.c b/builtin/diff.c
index 73b4ff3db..4c6a1a962 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -174,7 +174,7 @@ static int builtin_diff_tree(struct rev_info *revs,
swap = 1;
oid[swap] = >item->oid;
oid[1 - swap] = >item->oid;
-   diff_tree_sha1(oid[0]->hash, oid[1]->hash, "", >diffopt);
+   diff_tree_oid(oid[0], oid[1], "", >diffopt);
log_tree_diff_flush(revs);
return 0;
 }
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e242726f0..d57f36c43 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -562,8 +562,8 @@ static void handle_commit(struct commit *commit, struct 
rev_info *rev)
get_object_mark(>parents->item->object) != 0 &&

[PATCH 13/33] diff: convert diff_change to struct object_id

[Brandon Williams]

Convert diff_change to take a struct object_id.  In addition convert the
function pointer type 'change_fn_t' to also take a struct object_id.

Signed-off-by: Brandon Williams 
---
 diff-lib.c  |  4 ++--
 diff.c  | 14 +++---
 diff.h  | 13 ++---
 revision.c  |  6 +++---
 tree-diff.c |  2 +-
 5 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index c82b07dc1..1e8215df5 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -236,7 +236,7 @@ int run_diff_files(struct rev_info *revs, unsigned int 
option)
old_oid = >oid;
new_oid = changed ? _oid : >oid;
diff_change(>diffopt, oldmode, newmode,
-   old_oid->hash, new_oid->hash,
+   old_oid, new_oid,
!is_null_oid(old_oid),
!is_null_oid(new_oid),
ce->name, 0, dirty_submodule);
@@ -367,7 +367,7 @@ static int show_modified(struct rev_info *revs,
return 0;
 
diff_change(>diffopt, oldmode, mode,
-   old->oid.hash, oid->hash, 1, !is_null_oid(oid),
+   >oid, oid, 1, !is_null_oid(oid),
old->name, 0, dirty_submodule);
return 0;
 }
diff --git a/diff.c b/diff.c
index 3fa335f44..b9bb3f6ca 100644
--- a/diff.c
+++ b/diff.c
@@ -5127,9 +5127,9 @@ void diff_addremove(struct diff_options *options,
 
 void diff_change(struct diff_options *options,
 unsigned old_mode, unsigned new_mode,
-const unsigned char *old_sha1,
-const unsigned char *new_sha1,
-int old_sha1_valid, int new_sha1_valid,
+const struct object_id *old_oid,
+const struct object_id *new_oid,
+int old_oid_valid, int new_oid_valid,
 const char *concatpath,
 unsigned old_dirty_submodule, unsigned new_dirty_submodule)
 {
@@ -5142,8 +5142,8 @@ void diff_change(struct diff_options *options,
 
if (DIFF_OPT_TST(options, REVERSE_DIFF)) {
SWAP(old_mode, new_mode);
-   SWAP(old_sha1, new_sha1);
-   SWAP(old_sha1_valid, new_sha1_valid);
+   SWAP(old_oid, new_oid);
+   SWAP(old_oid_valid, new_oid_valid);
SWAP(old_dirty_submodule, new_dirty_submodule);
}
 
@@ -5153,8 +5153,8 @@ void diff_change(struct diff_options *options,
 
one = alloc_filespec(concatpath);
two = alloc_filespec(concatpath);
-   fill_filespec(one, old_sha1, old_sha1_valid, old_mode);
-   fill_filespec(two, new_sha1, new_sha1_valid, new_mode);
+   fill_filespec(one, old_oid->hash, old_oid_valid, old_mode);
+   fill_filespec(two, new_oid->hash, new_oid_valid, new_mode);
one->dirty_submodule = old_dirty_submodule;
two->dirty_submodule = new_dirty_submodule;
p = diff_queue(_queued_diff, one, two);
diff --git a/diff.h b/diff.h
index 1086975a5..fcf334bb6 100644
--- a/diff.h
+++ b/diff.h
@@ -23,9 +23,9 @@ typedef int (*pathchange_fn_t)(struct diff_options *options,
 
 typedef void (*change_fn_t)(struct diff_options *options,
 unsigned old_mode, unsigned new_mode,
-const unsigned char *old_sha1,
-const unsigned char *new_sha1,
-int old_sha1_valid, int new_sha1_valid,
+const struct object_id *old_oid,
+const struct object_id *new_oid,
+int old_oid_valid, int new_oid_valid,
 const char *fullpath,
 unsigned old_dirty_submodule, unsigned new_dirty_submodule);
 
@@ -253,10 +253,9 @@ extern void diff_addremove(struct diff_options *,
 
 extern void diff_change(struct diff_options *,
unsigned mode1, unsigned mode2,
-   const unsigned char *sha1,
-   const unsigned char *sha2,
-   int sha1_valid,
-   int sha2_valid,
+   const struct object_id *old_oid,
+   const struct object_id *new_oid,
+   int old_oid_valid, int new_oid_valid,
const char *fullpath,
unsigned dirty_submodule1, unsigned dirty_submodule2);
 
diff --git a/revision.c b/revision.c
index 71519193c..7637e7556 100644
--- a/revision.c
+++ b/revision.c
@@ -414,9 +414,9 @@ static void file_add_remove(struct diff_options *options,
 
 static void file_change(struct diff_options *options,
 unsigned old_mode, unsigned new_mode,
-const unsigned char *old_sha1,
-const unsigned char *new_sha1,
-int old_sha1_valid, int new_sha1_valid,
+const struct object_id *old_oid,
+const struct object_id *new_oid,
+int old_oid_valid, int new_oid_valid,
 

[PATCH 16/33] diff: finish conversion for prepare_temp_file to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 084c8b2d0..a8ceeb024 100644
--- a/diff.c
+++ b/diff.c
@@ -3030,13 +3030,13 @@ static struct diff_tempfile *prepare_temp_file(const 
char *name,
/* we can borrow from the file in the work tree */
temp->name = name;
if (!one->oid_valid)
-   sha1_to_hex_r(temp->hex, null_sha1);
+   oid_to_hex_r(temp->hex, _oid);
else
oid_to_hex_r(temp->hex, >oid);
/* Even though we may sometimes borrow the
 * contents from the work tree, we always want
 * one->mode.  mode is trustworthy even when
-* !(one->sha1_valid), as long as
+* !(one->oid_valid), as long as
 * DIFF_FILE_VALID(one).
 */
xsnprintf(temp->mode, sizeof(temp->mode), "%06o", 
one->mode);
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 17/33] patch-ids: convert to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 builtin/log.c |  2 +-
 patch-ids.c   | 20 ++--
 patch-ids.h   |  4 ++--
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index a440601ef..6bdba3444 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1354,7 +1354,7 @@ static void prepare_bases(struct base_tree_info *bases,
struct object_id *patch_id;
if (commit->util)
continue;
-   if (commit_patch_id(commit, , oid.hash, 0))
+   if (commit_patch_id(commit, , , 0))
die(_("cannot get patch id"));
ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, 
bases->alloc_patch_id);
patch_id = bases->patch_id + bases->nr_patch_id;
diff --git a/patch-ids.c b/patch-ids.c
index 92eba7a05..aab26cbbb 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -11,7 +11,7 @@ static int patch_id_defined(struct commit *commit)
 }
 
 int commit_patch_id(struct commit *commit, struct diff_options *options,
-   unsigned char *sha1, int diff_header_only)
+   struct object_id *oid, int diff_header_only)
 {
if (!patch_id_defined(commit))
return -1;
@@ -22,7 +22,7 @@ int commit_patch_id(struct commit *commit, struct 
diff_options *options,
else
diff_root_tree_sha1(commit->object.oid.hash, "", options);
diffcore_std(options);
-   return diff_flush_patch_id(options, sha1, diff_header_only);
+   return diff_flush_patch_id(options, oid->hash, diff_header_only);
 }
 
 /*
@@ -39,15 +39,15 @@ static int patch_id_cmp(struct patch_id *a,
struct patch_id *b,
struct diff_options *opt)
 {
-   if (is_null_sha1(a->patch_id) &&
-   commit_patch_id(a->commit, opt, a->patch_id, 0))
+   if (is_null_oid(>patch_id) &&
+   commit_patch_id(a->commit, opt, >patch_id, 0))
return error("Could not get patch ID for %s",
oid_to_hex(>commit->object.oid));
-   if (is_null_sha1(b->patch_id) &&
-   commit_patch_id(b->commit, opt, b->patch_id, 0))
+   if (is_null_oid(>patch_id) &&
+   commit_patch_id(b->commit, opt, >patch_id, 0))
return error("Could not get patch ID for %s",
oid_to_hex(>commit->object.oid));
-   return hashcmp(a->patch_id, b->patch_id);
+   return oidcmp(>patch_id, >patch_id);
 }
 
 int init_patch_ids(struct patch_ids *ids)
@@ -71,13 +71,13 @@ static int init_patch_id_entry(struct patch_id *patch,
   struct commit *commit,
   struct patch_ids *ids)
 {
-   unsigned char header_only_patch_id[GIT_MAX_RAWSZ];
+   struct object_id header_only_patch_id;
 
patch->commit = commit;
-   if (commit_patch_id(commit, >diffopts, header_only_patch_id, 1))
+   if (commit_patch_id(commit, >diffopts, _only_patch_id, 1))
return -1;
 
-   hashmap_entry_init(patch, sha1hash(header_only_patch_id));
+   hashmap_entry_init(patch, sha1hash(header_only_patch_id.hash));
return 0;
 }
 
diff --git a/patch-ids.h b/patch-ids.h
index b9e5751f8..bec0f727a 100644
--- a/patch-ids.h
+++ b/patch-ids.h
@@ -3,7 +3,7 @@
 
 struct patch_id {
struct hashmap_entry ent;
-   unsigned char patch_id[GIT_MAX_RAWSZ];
+   struct object_id patch_id;
struct commit *commit;
 };
 
@@ -13,7 +13,7 @@ struct patch_ids {
 };
 
 int commit_patch_id(struct commit *commit, struct diff_options *options,
-   unsigned char *sha1, int);
+   struct object_id *oid, int);
 int init_patch_ids(struct patch_ids *);
 int free_patch_ids(struct patch_ids *);
 struct patch_id *add_commit_patch_id(struct commit *, struct patch_ids *);
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 33/33] diff: rename diff_fill_sha1_info to diff_fill_oid_info

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 diff.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/diff.c b/diff.c
index dd325e616..c758a0d73 100644
--- a/diff.c
+++ b/diff.c
@@ -3239,7 +3239,7 @@ static void run_diff_cmd(const char *pgm,
fprintf(o->file, "* Unmerged path %s\n", name);
 }
 
-static void diff_fill_sha1_info(struct diff_filespec *one)
+static void diff_fill_oid_info(struct diff_filespec *one)
 {
if (DIFF_FILE_VALID(one)) {
if (!one->oid_valid) {
@@ -3298,8 +3298,8 @@ static void run_diff(struct diff_filepair *p, struct 
diff_options *o)
return;
}
 
-   diff_fill_sha1_info(one);
-   diff_fill_sha1_info(two);
+   diff_fill_oid_info(one);
+   diff_fill_oid_info(two);
 
if (!pgm &&
DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two) &&
@@ -3344,8 +3344,8 @@ static void run_diffstat(struct diff_filepair *p, struct 
diff_options *o,
if (o->prefix_length)
strip_prefix(o->prefix_length, , );
 
-   diff_fill_sha1_info(p->one);
-   diff_fill_sha1_info(p->two);
+   diff_fill_oid_info(p->one);
+   diff_fill_oid_info(p->two);
 
builtin_diffstat(name, other, p->one, p->two, diffstat, o, p);
 }
@@ -3368,8 +3368,8 @@ static void run_checkdiff(struct diff_filepair *p, struct 
diff_options *o)
if (o->prefix_length)
strip_prefix(o->prefix_length, , );
 
-   diff_fill_sha1_info(p->one);
-   diff_fill_sha1_info(p->two);
+   diff_fill_oid_info(p->one);
+   diff_fill_oid_info(p->two);
 
builtin_checkdiff(name, other, attr_path, p->one, p->two, o);
 }
@@ -4616,8 +4616,8 @@ static int diff_get_patch_id(struct diff_options 
*options, struct object_id *oid
if (DIFF_PAIR_UNMERGED(p))
continue;
 
-   diff_fill_sha1_info(p->one);
-   diff_fill_sha1_info(p->two);
+   diff_fill_oid_info(p->one);
+   diff_fill_oid_info(p->two);
 
len1 = remove_space(p->one->path, strlen(p->one->path));
len2 = remove_space(p->two->path, strlen(p->two->path));
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 31/33] tree-diff: convert path_appendnew to object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 tree-diff.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index 6a960f569..467e38172 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -132,7 +132,7 @@ static int emit_diff_first_parent_only(struct diff_options 
*opt, struct combine_
  */
 static struct combine_diff_path *path_appendnew(struct combine_diff_path *last,
int nparent, const struct strbuf *base, const char *path, int pathlen,
-   unsigned mode, const unsigned char *sha1)
+   unsigned mode, const struct object_id *oid)
 {
struct combine_diff_path *p;
size_t len = st_add(base->len, pathlen);
@@ -162,7 +162,7 @@ static struct combine_diff_path *path_appendnew(struct 
combine_diff_path *last,
memcpy(p->path + base->len, path, pathlen);
p->path[len] = 0;
p->mode = mode;
-   hashcpy(p->oid.hash, sha1 ? sha1 : null_sha1);
+   oidcpy(>oid, oid ? oid : _oid);
 
return p;
 }
@@ -221,7 +221,7 @@ static struct combine_diff_path *emit_path(struct 
combine_diff_path *p,
if (emitthis) {
int keep;
struct combine_diff_path *pprev = p;
-   p = path_appendnew(p, nparent, base, path, pathlen, mode, oid ? 
oid->hash : NULL);
+   p = path_appendnew(p, nparent, base, path, pathlen, mode, oid);
 
for (i = 0; i < nparent; ++i) {
/*
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 22/33] notes-merge: convert notes_merge* to struct object_id

[Brandon Williams]

Convert notes_merge and notes_merge_commit to use struct object_id.


Signed-off-by: Brandon Williams 
---
 builtin/notes.c |  6 +++---
 notes-merge.c   | 58 -
 notes-merge.h   | 12 ++--
 3 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index b13fc8789..2ebc2b7c4 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -724,7 +724,7 @@ static int merge_commit(struct notes_merge_options *o)
if (!o->local_ref)
die(_("failed to resolve NOTES_MERGE_REF"));
 
-   if (notes_merge_commit(o, t, partial, oid.hash))
+   if (notes_merge_commit(o, t, partial, ))
die(_("failed to finalize notes merge"));
 
/* Reuse existing commit message in reflog message */
@@ -842,9 +842,9 @@ static int merge(int argc, const char **argv, const char 
*prefix)
remote_ref.buf, default_notes_ref());
strbuf_add(&(o.commit_msg), msg.buf + 7, msg.len - 7); /* skip "notes: 
" */
 
-   result = notes_merge(, t, result_oid.hash);
+   result = notes_merge(, t, _oid);
 
-   if (result >= 0) /* Merge resulted (trivially) in result_sha1 */
+   if (result >= 0) /* Merge resulted (trivially) in result_oid */
/* Update default notes ref with new commit */
update_ref(msg.buf, default_notes_ref(), result_oid.hash, NULL,
   0, UPDATE_REFS_DIE_ON_ERR);
diff --git a/notes-merge.c b/notes-merge.c
index 9a1a49506..9dbf7f6a3 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -533,17 +533,17 @@ static int merge_from_diffs(struct notes_merge_options *o,
 
 int notes_merge(struct notes_merge_options *o,
struct notes_tree *local_tree,
-   unsigned char *result_sha1)
+   struct object_id *result_oid)
 {
struct object_id local_oid, remote_oid;
struct commit *local, *remote;
struct commit_list *bases = NULL;
-   const unsigned char *base_sha1, *base_tree_sha1;
+   const struct object_id *base_oid, *base_tree_oid;
int result = 0;
 
assert(o->local_ref && o->remote_ref);
assert(!strcmp(o->local_ref, local_tree->ref));
-   hashclr(result_sha1);
+   oidclr(result_oid);
 
trace_printf("notes_merge(o->local_ref = %s, o->remote_ref = %s)\n",
   o->local_ref, o->remote_ref);
@@ -553,16 +553,16 @@ int notes_merge(struct notes_merge_options *o,
die("Failed to resolve local notes ref '%s'", o->local_ref);
else if (!check_refname_format(o->local_ref, 0) &&
is_null_oid(_oid))
-   local = NULL; /* local_sha1 == null_sha1 indicates unborn ref */
+   local = NULL; /* local_oid == null_oid indicates unborn ref */
else if (!(local = lookup_commit_reference(_oid)))
die("Could not parse local commit %s (%s)",
oid_to_hex(_oid), o->local_ref);
trace_printf("\tlocal commit: %.7s\n", oid_to_hex(_oid));
 
-   /* Dereference o->remote_ref into remote_sha1 */
+   /* Dereference o->remote_ref into remote_oid */
if (get_oid(o->remote_ref, _oid)) {
/*
-* Failed to get remote_sha1. If o->remote_ref looks like an
+* Failed to get remote_oid. If o->remote_ref looks like an
 * unborn ref, perform the merge using an empty notes tree.
 */
if (!check_refname_format(o->remote_ref, 0)) {
@@ -583,12 +583,12 @@ int notes_merge(struct notes_merge_options *o,
"(%s)", o->remote_ref, o->local_ref);
if (!local) {
/* result == remote commit */
-   hashcpy(result_sha1, remote_oid.hash);
+   oidcpy(result_oid, _oid);
goto found_result;
}
if (!remote) {
/* result == local commit */
-   hashcpy(result_sha1, local_oid.hash);
+   oidcpy(result_oid, _oid);
goto found_result;
}
assert(local && remote);
@@ -596,47 +596,47 @@ int notes_merge(struct notes_merge_options *o,
/* Find merge bases */
bases = get_merge_bases(local, remote);
if (!bases) {
-   base_sha1 = null_sha1;
-   base_tree_sha1 = EMPTY_TREE_SHA1_BIN;
+   base_oid = _oid;
+   base_tree_oid = _tree_oid;
if (o->verbosity >= 4)
printf("No merge base found; doing history-less 
merge\n");
} else if (!bases->next) {
-   base_sha1 = bases->item->object.oid.hash;
-   base_tree_sha1 = bases->item->tree->object.oid.hash;
+   base_oid = >item->object.oid;
+   base_tree_oid = >item->tree->object.oid;
if (o->verbosity >= 4)
printf("One merge base found (%.7s)\n",
-   

[PATCH 18/33] diff: convert diff_flush_patch_id to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 diff.c  | 12 ++--
 diff.h  |  2 +-
 patch-ids.c |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index a8ceeb024..dd325e616 100644
--- a/diff.c
+++ b/diff.c
@@ -4584,7 +4584,7 @@ static void patch_id_add_mode(git_SHA_CTX *ctx, unsigned 
mode)
 }
 
 /* returns 0 upon success, and writes result into sha1 */
-static int diff_get_patch_id(struct diff_options *options, unsigned char 
*sha1, int diff_header_only)
+static int diff_get_patch_id(struct diff_options *options, struct object_id 
*oid, int diff_header_only)
 {
struct diff_queue_struct *q = _queued_diff;
int i;
@@ -4656,9 +4656,9 @@ static int diff_get_patch_id(struct diff_options 
*options, unsigned char *sha1,
if (diff_filespec_is_binary(p->one) ||
diff_filespec_is_binary(p->two)) {
git_SHA1_Update(, oid_to_hex(>one->oid),
-   40);
+   GIT_SHA1_HEXSZ);
git_SHA1_Update(, oid_to_hex(>two->oid),
-   40);
+   GIT_SHA1_HEXSZ);
continue;
}
 
@@ -4671,15 +4671,15 @@ static int diff_get_patch_id(struct diff_options 
*options, unsigned char *sha1,
 p->one->path);
}
 
-   git_SHA1_Final(sha1, );
+   git_SHA1_Final(oid->hash, );
return 0;
 }
 
-int diff_flush_patch_id(struct diff_options *options, unsigned char *sha1, int 
diff_header_only)
+int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, 
int diff_header_only)
 {
struct diff_queue_struct *q = _queued_diff;
int i;
-   int result = diff_get_patch_id(options, sha1, diff_header_only);
+   int result = diff_get_patch_id(options, oid, diff_header_only);
 
for (i = 0; i < q->nr; i++)
diff_free_filepair(q->queue[i]);
diff --git a/diff.h b/diff.h
index fcf334bb6..150f1c5a1 100644
--- a/diff.h
+++ b/diff.h
@@ -354,7 +354,7 @@ extern int run_diff_files(struct rev_info *revs, unsigned 
int option);
 extern int run_diff_index(struct rev_info *revs, int cached);
 
 extern int do_diff_cache(const struct object_id *, struct diff_options *);
-extern int diff_flush_patch_id(struct diff_options *, unsigned char *, int);
+extern int diff_flush_patch_id(struct diff_options *, struct object_id *, int);
 
 extern int diff_result_code(struct diff_options *, int);
 
diff --git a/patch-ids.c b/patch-ids.c
index aab26cbbb..a70a291d8 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -22,7 +22,7 @@ int commit_patch_id(struct commit *commit, struct 
diff_options *options,
else
diff_root_tree_sha1(commit->object.oid.hash, "", options);
diffcore_std(options);
-   return diff_flush_patch_id(options, oid->hash, diff_header_only);
+   return diff_flush_patch_id(options, oid, diff_header_only);
 }
 
 /*
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 23/33] notes-merge: convert merge_from_diffs to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 notes-merge.c | 31 ---
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/notes-merge.c b/notes-merge.c
index 9dbf7f6a3..be78f1954 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -114,8 +114,8 @@ static struct object_id uninitialized = {
 };
 
 static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,
-const unsigned char *base,
-const unsigned char *remote,
+const struct object_id *base,
+const struct object_id *remote,
 int *num_changes)
 {
struct diff_options opt;
@@ -123,13 +123,13 @@ static struct notes_merge_pair *diff_tree_remote(struct 
notes_merge_options *o,
int i, len = 0;
 
trace_printf("\tdiff_tree_remote(base = %.7s, remote = %.7s)\n",
-  sha1_to_hex(base), sha1_to_hex(remote));
+  oid_to_hex(base), oid_to_hex(remote));
 
diff_setup();
DIFF_OPT_SET(, RECURSIVE);
opt.output_format = DIFF_FORMAT_NO_OUTPUT;
diff_setup_done();
-   diff_tree_sha1(base, remote, "", );
+   diff_tree_sha1(base->hash, remote->hash, "", );
diffcore_std();
 
changes = xcalloc(diff_queued_diff.nr, sizeof(struct notes_merge_pair));
@@ -179,20 +179,20 @@ static struct notes_merge_pair *diff_tree_remote(struct 
notes_merge_options *o,
 
 static void diff_tree_local(struct notes_merge_options *o,
struct notes_merge_pair *changes, int len,
-   const unsigned char *base,
-   const unsigned char *local)
+   const struct object_id *base,
+   const struct object_id *local)
 {
struct diff_options opt;
int i;
 
trace_printf("\tdiff_tree_local(len = %i, base = %.7s, local = %.7s)\n",
-  len, sha1_to_hex(base), sha1_to_hex(local));
+  len, oid_to_hex(base), oid_to_hex(local));
 
diff_setup();
DIFF_OPT_SET(, RECURSIVE);
opt.output_format = DIFF_FORMAT_NO_OUTPUT;
diff_setup_done();
-   diff_tree_sha1(base, local, "", );
+   diff_tree_sha1(base->hash, local->hash, "", );
diffcore_std();
 
for (i = 0; i < diff_queued_diff.nr; i++) {
@@ -505,16 +505,17 @@ static int merge_changes(struct notes_merge_options *o,
 }
 
 static int merge_from_diffs(struct notes_merge_options *o,
-   const unsigned char *base,
-   const unsigned char *local,
-   const unsigned char *remote, struct notes_tree *t)
+   const struct object_id *base,
+   const struct object_id *local,
+   const struct object_id *remote,
+   struct notes_tree *t)
 {
struct notes_merge_pair *changes;
int num_changes, conflicts;
 
trace_printf("\tmerge_from_diffs(base = %.7s, local = %.7s, "
-  "remote = %.7s)\n", sha1_to_hex(base), sha1_to_hex(local),
-  sha1_to_hex(remote));
+  "remote = %.7s)\n", oid_to_hex(base), oid_to_hex(local),
+  oid_to_hex(remote));
 
changes = diff_tree_remote(o, base, remote, _changes);
diff_tree_local(o, changes, num_changes, base, local);
@@ -636,8 +637,8 @@ int notes_merge(struct notes_merge_options *o,
goto found_result;
}
 
-   result = merge_from_diffs(o, base_tree_oid->hash, 
local->tree->object.oid.hash,
- remote->tree->object.oid.hash, local_tree);
+   result = merge_from_diffs(o, base_tree_oid, >tree->object.oid,
+ >tree->object.oid, local_tree);
 
if (result != 0) { /* non-trivial merge (with or without conflicts) */
/* Commit (partial) result */
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 20/33] combine-diff: convert find_paths_* to struct object_id

[Brandon Williams]

Convert find_paths_generic and find_paths_multitree to use struct
object_id.


Signed-off-by: Brandon Williams 
---
 combine-diff.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 84981df75..c82364510 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1311,7 +1311,7 @@ static const char *path_path(void *obj)
 
 
 /* find set of paths that every parent touches */
-static struct combine_diff_path *find_paths_generic(const unsigned char *sha1,
+static struct combine_diff_path *find_paths_generic(const struct object_id 
*oid,
const struct oid_array *parents, struct diff_options *opt)
 {
struct combine_diff_path *paths = NULL;
@@ -1336,7 +1336,7 @@ static struct combine_diff_path *find_paths_generic(const 
unsigned char *sha1,
opt->output_format = stat_opt;
else
opt->output_format = DIFF_FORMAT_NO_OUTPUT;
-   diff_tree_sha1(parents->oid[i].hash, sha1, "", opt);
+   diff_tree_sha1(parents->oid[i].hash, oid->hash, "", opt);
diffcore_std(opt);
paths = intersect_paths(paths, i, num_parent);
 
@@ -1360,7 +1360,7 @@ static struct combine_diff_path *find_paths_generic(const 
unsigned char *sha1,
  * rename/copy detection, etc, comparing all trees simultaneously (= faster).
  */
 static struct combine_diff_path *find_paths_multitree(
-   const unsigned char *sha1, const struct oid_array *parents,
+   const struct object_id *oid, const struct oid_array *parents,
struct diff_options *opt)
 {
int i, nparent = parents->nr;
@@ -1376,7 +1376,7 @@ static struct combine_diff_path *find_paths_multitree(
paths_head.next = NULL;
 
strbuf_init(, PATH_MAX);
-   diff_tree_paths(_head, sha1, parents_sha1, nparent, , opt);
+   diff_tree_paths(_head, oid->hash, parents_sha1, nparent, , 
opt);
 
strbuf_release();
free(parents_sha1);
@@ -1448,11 +1448,11 @@ void diff_tree_combined(const struct object_id *oid,
 * diff(sha1,parent_i) for all i to do the job, specifically
 * for parent0.
 */
-   paths = find_paths_generic(oid->hash, parents, );
+   paths = find_paths_generic(oid, parents, );
}
else {
int stat_opt;
-   paths = find_paths_multitree(oid->hash, parents, );
+   paths = find_paths_multitree(oid, parents, );
 
/*
 * show stat against the first parent even
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 29/33] tree-diff: convert try_to_follow_renames to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 tree-diff.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index fc020d76d..29e3f6144 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -577,7 +577,9 @@ static inline int diff_might_be_rename(void)
!DIFF_FILE_VALID(diff_queued_diff.queue[0]->one);
 }
 
-static void try_to_follow_renames(const unsigned char *old, const unsigned 
char *new, struct strbuf *base, struct diff_options *opt)
+static void try_to_follow_renames(const struct object_id *old_oid,
+ const struct object_id *new_oid,
+ struct strbuf *base, struct diff_options *opt)
 {
struct diff_options diff_opts;
struct diff_queue_struct *q = _queued_diff;
@@ -615,7 +617,7 @@ static void try_to_follow_renames(const unsigned char *old, 
const unsigned char
diff_opts.break_opt = opt->break_opt;
diff_opts.rename_score = opt->rename_score;
diff_setup_done(_opts);
-   ll_diff_tree_sha1(old, new, base, _opts);
+   ll_diff_tree_sha1(old_oid->hash, new_oid->hash, base, _opts);
diffcore_std(_opts);
clear_pathspec(_opts.pathspec);
 
@@ -706,7 +708,7 @@ int diff_tree_oid(const struct object_id *old_oid,
 
retval = ll_diff_tree_sha1(old_oid->hash, new_oid->hash, , opt);
if (!*base_str && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && 
diff_might_be_rename())
-   try_to_follow_renames(old_oid->hash, new_oid->hash, , opt);
+   try_to_follow_renames(old_oid, new_oid, , opt);
 
strbuf_release();
 
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 30/33] tree-diff: convert diff_tree_paths to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 combine-diff.c | 10 +-
 diff.h |  4 ++--
 tree-diff.c| 63 +-
 3 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 04c4ae856..ec9d93044 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1364,22 +1364,22 @@ static struct combine_diff_path *find_paths_multitree(
struct diff_options *opt)
 {
int i, nparent = parents->nr;
-   const unsigned char **parents_sha1;
+   const struct object_id **parents_oid;
struct combine_diff_path paths_head;
struct strbuf base;
 
-   ALLOC_ARRAY(parents_sha1, nparent);
+   ALLOC_ARRAY(parents_oid, nparent);
for (i = 0; i < nparent; i++)
-   parents_sha1[i] = parents->oid[i].hash;
+   parents_oid[i] = >oid[i];
 
/* fake list head, so worker can assume it is non-NULL */
paths_head.next = NULL;
 
strbuf_init(, PATH_MAX);
-   diff_tree_paths(_head, oid->hash, parents_sha1, nparent, , 
opt);
+   diff_tree_paths(_head, oid, parents_oid, nparent, , opt);
 
strbuf_release();
-   free(parents_sha1);
+   free(parents_oid);
return paths_head.next;
 }
 
diff --git a/diff.h b/diff.h
index e0b503460..0d0c14f28 100644
--- a/diff.h
+++ b/diff.h
@@ -210,8 +210,8 @@ const char *diff_line_prefix(struct diff_options *);
 extern const char mime_boundary_leader[];
 
 extern struct combine_diff_path *diff_tree_paths(
-   struct combine_diff_path *p, const unsigned char *sha1,
-   const unsigned char **parent_sha1, int nparent,
+   struct combine_diff_path *p, const struct object_id *oid,
+   const struct object_id **parents_oid, int nparent,
struct strbuf *base, struct diff_options *opt);
 extern int diff_tree_oid(const struct object_id *old_oid,
 const struct object_id *new_oid,
diff --git a/tree-diff.c b/tree-diff.c
index 29e3f6144..6a960f569 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -26,11 +26,12 @@
 } while(0)
 
 static struct combine_diff_path *ll_diff_tree_paths(
-   struct combine_diff_path *p, const unsigned char *sha1,
-   const unsigned char **parents_sha1, int nparent,
+   struct combine_diff_path *p, const struct object_id *oid,
+   const struct object_id **parents_oid, int nparent,
struct strbuf *base, struct diff_options *opt);
-static int ll_diff_tree_sha1(const unsigned char *old, const unsigned char 
*new,
-struct strbuf *base, struct diff_options *opt);
+static int ll_diff_tree_oid(const struct object_id *old_oid,
+   const struct object_id *new_oid,
+   struct strbuf *base, struct diff_options *opt);
 
 /*
  * Compare two tree entries, taking into account only path/S_ISDIR(mode),
@@ -183,7 +184,7 @@ static struct combine_diff_path *emit_path(struct 
combine_diff_path *p,
 {
unsigned mode;
const char *path;
-   const unsigned char *sha1;
+   const struct object_id *oid;
int pathlen;
int old_baselen = base->len;
int i, isdir, recurse = 0, emitthis = 1;
@@ -193,7 +194,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, , )->hash;
+   oid = tree_entry_extract(t, , );
pathlen = tree_entry_len(>entry);
isdir = S_ISDIR(mode);
} else {
@@ -208,7 +209,7 @@ static struct combine_diff_path *emit_path(struct 
combine_diff_path *p,
pathlen = tree_entry_len([imin].entry);
 
isdir = S_ISDIR(mode);
-   sha1 = NULL;
+   oid = NULL;
mode = 0;
}
 
@@ -220,7 +221,7 @@ static struct combine_diff_path *emit_path(struct 
combine_diff_path *p,
if (emitthis) {
int keep;
struct combine_diff_path *pprev = p;
-   p = path_appendnew(p, nparent, base, path, pathlen, mode, sha1);
+   p = path_appendnew(p, nparent, base, path, pathlen, mode, oid ? 
oid->hash : NULL);
 
for (i = 0; i < nparent; ++i) {
/*
@@ -229,7 +230,7 @@ static struct combine_diff_path *emit_path(struct 
combine_diff_path *p,
 */
int tpi_valid = tp && !(tp[i].entry.mode & 
S_IFXMIN_NEQ);
 
-   const unsigned char *sha1_i;
+   const struct object_id *oid_i;
unsigned mode_i;
 
p->parent[i].status =
@@ -239,16 +240,16 @@ static struct combine_diff_path *emit_path(struct 
combine_diff_path *p,
DIFF_STATUS_ADDED;
 
if (tpi_valid) {
-  

[PATCH 26/33] notes-merge: convert write_note_to_worktree to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 notes-merge.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/notes-merge.c b/notes-merge.c
index 962e9b1bc..7d88857a8 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -292,11 +292,11 @@ static void check_notes_merge_worktree(struct 
notes_merge_options *o)
git_path(NOTES_MERGE_WORKTREE));
 }
 
-static void write_buf_to_worktree(const unsigned char *obj,
+static void write_buf_to_worktree(const struct object_id *obj,
  const char *buf, unsigned long size)
 {
int fd;
-   char *path = git_pathdup(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
+   char *path = git_pathdup(NOTES_MERGE_WORKTREE "/%s", oid_to_hex(obj));
if (safe_create_leading_directories_const(path))
die_errno("unable to create directory for '%s'", path);
 
@@ -320,19 +320,19 @@ static void write_buf_to_worktree(const unsigned char 
*obj,
free(path);
 }
 
-static void write_note_to_worktree(const unsigned char *obj,
-  const unsigned char *note)
+static void write_note_to_worktree(const struct object_id *obj,
+  const struct object_id *note)
 {
enum object_type type;
unsigned long size;
-   void *buf = read_sha1_file(note, , );
+   void *buf = read_sha1_file(note->hash, , );
 
if (!buf)
die("cannot read note %s for object %s",
-   sha1_to_hex(note), sha1_to_hex(obj));
+   oid_to_hex(note), oid_to_hex(obj));
if (type != OBJ_BLOB)
die("blob expected in note %s for object %s",
-   sha1_to_hex(note), sha1_to_hex(obj));
+   oid_to_hex(note), oid_to_hex(obj));
write_buf_to_worktree(obj, buf, size);
free(buf);
 }
@@ -358,7 +358,7 @@ static int ll_merge_in_worktree(struct notes_merge_options 
*o,
if ((status < 0) || !result_buf.ptr)
die("Failed to execute internal merge");
 
-   write_buf_to_worktree(p->obj.hash, result_buf.ptr, result_buf.size);
+   write_buf_to_worktree(>obj, result_buf.ptr, result_buf.size);
free(result_buf.ptr);
 
return status;
@@ -393,7 +393,7 @@ static int merge_one_change_manual(struct 
notes_merge_options *o,
"deleted in %s and modified in %s. Version from 
%s "
"left in tree.\n",
oid_to_hex(>obj), lref, rref, rref);
-   write_note_to_worktree(p->obj.hash, p->remote.hash);
+   write_note_to_worktree(>obj, >remote);
} else if (is_null_oid(>remote)) {
/* D/F conflict, checkout p->local */
assert(!is_null_oid(>local));
@@ -402,7 +402,7 @@ static int merge_one_change_manual(struct 
notes_merge_options *o,
"deleted in %s and modified in %s. Version from 
%s "
"left in tree.\n",
oid_to_hex(>obj), rref, lref, lref);
-   write_note_to_worktree(p->obj.hash, p->local.hash);
+   write_note_to_worktree(>obj, >local);
} else {
/* "regular" conflict, checkout result of ll_merge() */
const char *reason = "content";
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 03/33] notes: convert for_each_note to struct object_id

[Brandon Williams]

From: "brian m. carlson" 

Convert for_each_note and each of the callbacks to use struct object_id.

Signed-off-by: brian m. carlson 
Signed-off-by: Brandon Williams 
---
 builtin/notes.c  |  6 +++---
 notes.c  | 24 
 notes.h  |  4 ++--
 remote-testsvn.c |  8 
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index f2847c41e..53fe6d34d 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -109,11 +109,11 @@ static void free_note_data(struct note_data *d)
strbuf_release(>buf);
 }
 
-static int list_each_note(const unsigned char *object_sha1,
-   const unsigned char *note_sha1, char *note_path,
+static int list_each_note(const struct object_id *object_oid,
+   const struct object_id *note_oid, char *note_path,
void *cb_data)
 {
-   printf("%s %s\n", sha1_to_hex(note_sha1), sha1_to_hex(object_sha1));
+   printf("%s %s\n", oid_to_hex(note_oid), oid_to_hex(object_oid));
return 0;
 }
 
diff --git a/notes.c b/notes.c
index babe0c0eb..e881c10ee 100644
--- a/notes.c
+++ b/notes.c
@@ -610,7 +610,7 @@ static int for_each_note_helper(struct notes_tree *t, 
struct int_node *tree,
if (path[path_len - 1] != '/')
path[path_len++] = '/';
path[path_len] = '\0';
-   ret = fn(l->key_oid.hash, l->val_oid.hash,
+   ret = fn(>key_oid, >val_oid,
 path,
 cb_data);
}
@@ -627,7 +627,7 @@ static int for_each_note_helper(struct notes_tree *t, 
struct int_node *tree,
l = (struct leaf_node *) CLR_PTR_TYPE(p);
construct_path_with_fanout(l->key_oid.hash, fanout,
   path);
-   ret = fn(l->key_oid.hash, l->val_oid.hash, path,
+   ret = fn(>key_oid, >val_oid, path,
 cb_data);
break;
}
@@ -698,7 +698,7 @@ static int tree_write_stack_finish_subtree(struct 
tree_write_stack *tws)
 
 static int write_each_note_helper(struct tree_write_stack *tws,
const char *path, unsigned int mode,
-   const unsigned char *sha1)
+   const struct object_id *oid)
 {
size_t path_len = strlen(path);
unsigned int n = 0;
@@ -728,7 +728,7 @@ static int write_each_note_helper(struct tree_write_stack 
*tws,
 
/* Finally add given entry to the current tree object */
write_tree_entry(>buf, mode, path + 3 * n, path_len - (3 * n),
-sha1);
+oid->hash);
 
return 0;
 }
@@ -748,7 +748,7 @@ static int write_each_non_note_until(const char *note_path,
; /* do nothing, prefer note to non-note */
else {
ret = write_each_note_helper(d->root, n->path, n->mode,
-n->oid.hash);
+>oid);
if (ret)
return ret;
}
@@ -758,8 +758,8 @@ static int write_each_non_note_until(const char *note_path,
return 0;
 }
 
-static int write_each_note(const unsigned char *object_sha1,
-   const unsigned char *note_sha1, char *note_path,
+static int write_each_note(const struct object_id *object_oid,
+   const struct object_id *note_oid, char *note_path,
void *cb_data)
 {
struct write_each_note_data *d =
@@ -777,7 +777,7 @@ static int write_each_note(const unsigned char *object_sha1,
 
/* Weave non-note entries into note entries */
return  write_each_non_note_until(note_path, d) ||
-   write_each_note_helper(d->root, note_path, mode, note_sha1);
+   write_each_note_helper(d->root, note_path, mode, note_oid);
 }
 
 struct note_delete_list {
@@ -785,20 +785,20 @@ struct note_delete_list {
const unsigned char *sha1;
 };
 
-static int prune_notes_helper(const unsigned char *object_sha1,
-   const unsigned char *note_sha1, char *note_path,
+static int prune_notes_helper(const struct object_id *object_oid,
+   const struct object_id *note_oid, char *note_path,
void *cb_data)
 {
struct note_delete_list **l = (struct note_delete_list **) cb_data;
struct note_delete_list *n;
 
-   if (has_sha1_file(object_sha1))
+   if (has_object_file(object_oid))
return 0; /* nothing to do for this note */
 
/* failed to find object => prune this note */
n = (struct 

[PATCH 19/33] combine-diff: convert diff_tree_combined to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 builtin/diff.c |  2 +-
 combine-diff.c | 10 +-
 diff.h |  2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index b2d7c32cd..73b4ff3db 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -194,7 +194,7 @@ static int builtin_diff_combined(struct rev_info *revs,
revs->dense_combined_merges = revs->combine_merges = 1;
for (i = 1; i < ents; i++)
oid_array_append(, [i].item->oid);
-   diff_tree_combined(ent[0].item->oid.hash, ,
+   diff_tree_combined([0].item->oid, ,
   revs->dense_combined_merges, revs);
oid_array_clear();
return 0;
diff --git a/combine-diff.c b/combine-diff.c
index ad063ecb1..84981df75 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1384,7 +1384,7 @@ static struct combine_diff_path *find_paths_multitree(
 }
 
 
-void diff_tree_combined(const unsigned char *sha1,
+void diff_tree_combined(const struct object_id *oid,
const struct oid_array *parents,
int dense,
struct rev_info *rev)
@@ -1448,11 +1448,11 @@ void diff_tree_combined(const unsigned char *sha1,
 * diff(sha1,parent_i) for all i to do the job, specifically
 * for parent0.
 */
-   paths = find_paths_generic(sha1, parents, );
+   paths = find_paths_generic(oid->hash, parents, );
}
else {
int stat_opt;
-   paths = find_paths_multitree(sha1, parents, );
+   paths = find_paths_multitree(oid->hash, parents, );
 
/*
 * show stat against the first parent even
@@ -1463,7 +1463,7 @@ void diff_tree_combined(const unsigned char *sha1,
if (stat_opt) {
diffopts.output_format = stat_opt;
 
-   diff_tree_sha1(parents->oid[0].hash, sha1, "", 
);
+   diff_tree_sha1(parents->oid[0].hash, oid->hash, "", 
);
diffcore_std();
if (opt->orderfile)
diffcore_order(opt->orderfile);
@@ -1539,6 +1539,6 @@ void diff_tree_combined_merge(const struct commit 
*commit, int dense,
oid_array_append(, >item->object.oid);
parent = parent->next;
}
-   diff_tree_combined(commit->object.oid.hash, , dense, rev);
+   diff_tree_combined(>object.oid, , dense, rev);
oid_array_clear();
 }
diff --git a/diff.h b/diff.h
index 150f1c5a1..6aeeda035 100644
--- a/diff.h
+++ b/diff.h
@@ -236,7 +236,7 @@ struct combine_diff_path {
 extern void show_combined_diff(struct combine_diff_path *elem, int num_parent,
  int dense, struct rev_info *);
 
-extern void diff_tree_combined(const unsigned char *sha1, const struct 
oid_array *parents, int dense, struct rev_info *rev);
+extern void diff_tree_combined(const struct object_id *oid, const struct 
oid_array *parents, int dense, struct rev_info *rev);
 
 extern void diff_tree_combined_merge(const struct commit *commit, int dense, 
struct rev_info *rev);
 
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 25/33] notes-merge: convert verify_notes_filepair to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 notes-merge.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/notes-merge.c b/notes-merge.c
index 55dbb3659..962e9b1bc 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -22,21 +22,21 @@ void init_notes_merge_options(struct notes_merge_options *o)
o->verbosity = NOTES_MERGE_VERBOSITY_DEFAULT;
 }
 
-static int path_to_sha1(const char *path, unsigned char *sha1)
+static int path_to_oid(const char *path, struct object_id *oid)
 {
-   char hex_sha1[40];
+   char hex_oid[GIT_SHA1_HEXSZ];
int i = 0;
-   while (*path && i < 40) {
+   while (*path && i < GIT_SHA1_HEXSZ) {
if (*path != '/')
-   hex_sha1[i++] = *path;
+   hex_oid[i++] = *path;
path++;
}
-   if (*path || i != 40)
+   if (*path || i != GIT_SHA1_HEXSZ)
return -1;
-   return get_sha1_hex(hex_sha1, sha1);
+   return get_oid_hex(hex_oid, oid);
 }
 
-static int verify_notes_filepair(struct diff_filepair *p, unsigned char *sha1)
+static int verify_notes_filepair(struct diff_filepair *p, struct object_id 
*oid)
 {
switch (p->status) {
case DIFF_STATUS_MODIFIED:
@@ -54,7 +54,7 @@ static int verify_notes_filepair(struct diff_filepair *p, 
unsigned char *sha1)
return -1;
}
assert(!strcmp(p->one->path, p->two->path));
-   return path_to_sha1(p->one->path, sha1);
+   return path_to_oid(p->one->path, oid);
 }
 
 static struct notes_merge_pair *find_notes_merge_pair_pos(
@@ -140,7 +140,7 @@ static struct notes_merge_pair *diff_tree_remote(struct 
notes_merge_options *o,
int occupied;
struct object_id obj;
 
-   if (verify_notes_filepair(p, obj.hash)) {
+   if (verify_notes_filepair(p, )) {
trace_printf("\t\tCannot merge entry '%s' (%c): "
   "%.7s -> %.7s. Skipping!\n", p->one->path,
   p->status, oid_to_hex(>one->oid),
@@ -201,7 +201,7 @@ static void diff_tree_local(struct notes_merge_options *o,
int match;
struct object_id obj;
 
-   if (verify_notes_filepair(p, obj.hash)) {
+   if (verify_notes_filepair(p, )) {
trace_printf("\t\tCannot merge entry '%s' (%c): "
   "%.7s -> %.7s. Skipping!\n", p->one->path,
   p->status, oid_to_hex(>one->oid),
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 21/33] tree-diff: convert diff_root_tree_sha1 to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 builtin/fast-export.c | 4 ++--
 diff.h| 4 ++--
 log-tree.c| 2 +-
 patch-ids.c   | 2 +-
 tree-diff.c   | 4 ++--
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 24e29ad7e..e242726f0 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -566,8 +566,8 @@ static void handle_commit(struct commit *commit, struct 
rev_info *rev)
   commit->tree->object.oid.hash, "", 
>diffopt);
}
else
-   diff_root_tree_sha1(commit->tree->object.oid.hash,
-   "", >diffopt);
+   diff_root_tree_oid(>tree->object.oid,
+  "", >diffopt);
 
/* Export the referenced blobs, and remember the marks. */
for (i = 0; i < diff_queued_diff.nr; i++)
diff --git a/diff.h b/diff.h
index 6aeeda035..8d46a6709 100644
--- a/diff.h
+++ b/diff.h
@@ -215,8 +215,8 @@ extern struct combine_diff_path *diff_tree_paths(
struct strbuf *base, struct diff_options *opt);
 extern int diff_tree_sha1(const unsigned char *old, const unsigned char *new,
  const char *base, struct diff_options *opt);
-extern int diff_root_tree_sha1(const unsigned char *new, const char *base,
-   struct diff_options *opt);
+extern int diff_root_tree_oid(const struct object_id *new_oid, const char 
*base,
+ struct diff_options *opt);
 
 struct combine_diff_path {
struct combine_diff_path *next;
diff --git a/log-tree.c b/log-tree.c
index 9c0c64a2d..b40242534 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -803,7 +803,7 @@ static int log_tree_diff(struct rev_info *opt, struct 
commit *commit, struct log
parents = get_saved_parents(opt, commit);
if (!parents) {
if (opt->show_root_diff) {
-   diff_root_tree_sha1(oid->hash, "", >diffopt);
+   diff_root_tree_oid(oid, "", >diffopt);
log_tree_diff_flush(opt);
}
return !opt->loginfo;
diff --git a/patch-ids.c b/patch-ids.c
index a70a291d8..aaf462c03 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -20,7 +20,7 @@ int commit_patch_id(struct commit *commit, struct 
diff_options *options,
diff_tree_sha1(commit->parents->item->object.oid.hash,
   commit->object.oid.hash, "", options);
else
-   diff_root_tree_sha1(commit->object.oid.hash, "", options);
+   diff_root_tree_oid(>object.oid, "", options);
diffcore_std(options);
return diff_flush_patch_id(options, oid, diff_header_only);
 }
diff --git a/tree-diff.c b/tree-diff.c
index 7ae1f10b2..f9bbaf3c4 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -711,7 +711,7 @@ int diff_tree_sha1(const unsigned char *old, const unsigned 
char *new, const cha
return retval;
 }
 
-int diff_root_tree_sha1(const unsigned char *new, const char *base, struct 
diff_options *opt)
+int diff_root_tree_oid(const struct object_id *new_oid, const char *base, 
struct diff_options *opt)
 {
-   return diff_tree_sha1(NULL, new, base, opt);
+   return diff_tree_sha1(NULL, new_oid->hash, base, opt);
 }
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 24/33] notes-merge: convert find_notes_merge_pair_ps to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 notes-merge.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/notes-merge.c b/notes-merge.c
index be78f1954..55dbb3659 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -58,7 +58,7 @@ static int verify_notes_filepair(struct diff_filepair *p, 
unsigned char *sha1)
 }
 
 static struct notes_merge_pair *find_notes_merge_pair_pos(
-   struct notes_merge_pair *list, int len, unsigned char *obj,
+   struct notes_merge_pair *list, int len, struct object_id *obj,
int insert_new, int *occupied)
 {
/*
@@ -75,7 +75,7 @@ static struct notes_merge_pair *find_notes_merge_pair_pos(
int i = last_index < len ? last_index : len - 1;
int prev_cmp = 0, cmp = -1;
while (i >= 0 && i < len) {
-   cmp = hashcmp(obj, list[i].obj.hash);
+   cmp = oidcmp(obj, [i].obj);
if (!cmp) /* obj belongs @ i */
break;
else if (cmp < 0 && prev_cmp <= 0) /* obj belongs < i */
@@ -138,19 +138,19 @@ static struct notes_merge_pair *diff_tree_remote(struct 
notes_merge_options *o,
struct diff_filepair *p = diff_queued_diff.queue[i];
struct notes_merge_pair *mp;
int occupied;
-   unsigned char obj[20];
+   struct object_id obj;
 
-   if (verify_notes_filepair(p, obj)) {
+   if (verify_notes_filepair(p, obj.hash)) {
trace_printf("\t\tCannot merge entry '%s' (%c): "
   "%.7s -> %.7s. Skipping!\n", p->one->path,
   p->status, oid_to_hex(>one->oid),
   oid_to_hex(>two->oid));
continue;
}
-   mp = find_notes_merge_pair_pos(changes, len, obj, 1, );
+   mp = find_notes_merge_pair_pos(changes, len, , 1, 
);
if (occupied) {
/* We've found an addition/deletion pair */
-   assert(!hashcmp(mp->obj.hash, obj));
+   assert(!oidcmp(>obj, ));
if (is_null_oid(>one->oid)) { /* addition */
assert(is_null_oid(>remote));
oidcpy(>remote, >two->oid);
@@ -160,7 +160,7 @@ static struct notes_merge_pair *diff_tree_remote(struct 
notes_merge_options *o,
} else
assert(!"Invalid existing change recorded");
} else {
-   hashcpy(mp->obj.hash, obj);
+   oidcpy(>obj, );
oidcpy(>base, >one->oid);
oidcpy(>local, );
oidcpy(>remote, >two->oid);
@@ -199,25 +199,25 @@ static void diff_tree_local(struct notes_merge_options *o,
struct diff_filepair *p = diff_queued_diff.queue[i];
struct notes_merge_pair *mp;
int match;
-   unsigned char obj[20];
+   struct object_id obj;
 
-   if (verify_notes_filepair(p, obj)) {
+   if (verify_notes_filepair(p, obj.hash)) {
trace_printf("\t\tCannot merge entry '%s' (%c): "
   "%.7s -> %.7s. Skipping!\n", p->one->path,
   p->status, oid_to_hex(>one->oid),
   oid_to_hex(>two->oid));
continue;
}
-   mp = find_notes_merge_pair_pos(changes, len, obj, 0, );
+   mp = find_notes_merge_pair_pos(changes, len, , 0, );
if (!match) {
trace_printf("\t\tIgnoring local-only change for %s: "
-  "%.7s -> %.7s\n", sha1_to_hex(obj),
+  "%.7s -> %.7s\n", oid_to_hex(),
   oid_to_hex(>one->oid),
   oid_to_hex(>two->oid));
continue;
}
 
-   assert(!hashcmp(mp->obj.hash, obj));
+   assert(!oidcmp(>obj, ));
if (is_null_oid(>two->oid)) { /* deletion */
/*
 * Either this is a true deletion (1), or it is part
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 01/33] notes: convert internal structures to struct object_id

[Brandon Williams]

From: "brian m. carlson" 

Convert the internal structures using unsigned char [20] to take
struct object_id using the following semantic patch and the standard
object_id transforms:

@@
struct leaf_node E1;
@@
- E1.key_sha1
+ E1.key_oid.hash

@@
struct leaf_node *E1;
@@
- E1->key_sha1
+ E1->key_oid.hash

@@
struct leaf_node E1;
@@
- E1.key_sha1
+ E1.key_oid.hash

@@
struct leaf_node *E1;
@@
- E1->key_sha1
+ E1->key_oid.hash

@@
struct non_note E1;
@@
- E1.sha1
+ E1.oid.hash

@@
struct non_note *E1;
@@
- E1->sha1
+ E1->oid.hash

Signed-off-by: brian m. carlson 
Signed-off-by: Brandon Williams 
---
 notes.c | 98 ++---
 1 file changed, 51 insertions(+), 47 deletions(-)

diff --git a/notes.c b/notes.c
index 542563b28..251cf11c9 100644
--- a/notes.c
+++ b/notes.c
@@ -35,8 +35,8 @@ struct int_node {
  * subtree.
  */
 struct leaf_node {
-   unsigned char key_sha1[20];
-   unsigned char val_sha1[20];
+   struct object_id key_oid;
+   struct object_id val_oid;
 };
 
 /*
@@ -51,7 +51,7 @@ struct non_note {
struct non_note *next; /* grounded (last->next == NULL) */
char *path;
unsigned int mode;
-   unsigned char sha1[20];
+   struct object_id oid;
 };
 
 #define PTR_TYPE_NULL 0
@@ -100,7 +100,7 @@ static void **note_tree_search(struct notes_tree *t, struct 
int_node **tree,
 
if (GET_PTR_TYPE(p) == PTR_TYPE_SUBTREE) {
l = (struct leaf_node *) CLR_PTR_TYPE(p);
-   if (!SUBTREE_SHA1_PREFIXCMP(key_sha1, l->key_sha1)) {
+   if (!SUBTREE_SHA1_PREFIXCMP(key_sha1, l->key_oid.hash)) {
/* unpack tree and resume search */
(*tree)->a[0] = NULL;
load_subtree(t, l, *tree, *n);
@@ -118,7 +118,7 @@ static void **note_tree_search(struct notes_tree *t, struct 
int_node **tree,
return note_tree_search(t, tree, n, key_sha1);
case PTR_TYPE_SUBTREE:
l = (struct leaf_node *) CLR_PTR_TYPE(p);
-   if (!SUBTREE_SHA1_PREFIXCMP(key_sha1, l->key_sha1)) {
+   if (!SUBTREE_SHA1_PREFIXCMP(key_sha1, l->key_oid.hash)) {
/* unpack tree and resume search */
(*tree)->a[i] = NULL;
load_subtree(t, l, *tree, *n);
@@ -143,7 +143,7 @@ static struct leaf_node *note_tree_find(struct notes_tree 
*t,
void **p = note_tree_search(t, , , key_sha1);
if (GET_PTR_TYPE(*p) == PTR_TYPE_NOTE) {
struct leaf_node *l = (struct leaf_node *) CLR_PTR_TYPE(*p);
-   if (!hashcmp(key_sha1, l->key_sha1))
+   if (!hashcmp(key_sha1, l->key_oid.hash))
return l;
}
return NULL;
@@ -196,17 +196,17 @@ static void note_tree_remove(struct notes_tree *t,
struct leaf_node *l;
struct int_node *parent_stack[20];
unsigned char i, j;
-   void **p = note_tree_search(t, , , entry->key_sha1);
+   void **p = note_tree_search(t, , , entry->key_oid.hash);
 
assert(GET_PTR_TYPE(entry) == 0); /* no type bits set */
if (GET_PTR_TYPE(*p) != PTR_TYPE_NOTE)
return; /* type mismatch, nothing to remove */
l = (struct leaf_node *) CLR_PTR_TYPE(*p);
-   if (hashcmp(l->key_sha1, entry->key_sha1))
+   if (oidcmp(>key_oid, >key_oid))
return; /* key mismatch, nothing to remove */
 
/* we have found a matching entry */
-   hashcpy(entry->val_sha1, l->val_sha1);
+   oidcpy(>val_oid, >val_oid);
free(l);
*p = SET_PTR_TYPE(NULL, PTR_TYPE_NULL);
 
@@ -216,14 +216,14 @@ static void note_tree_remove(struct notes_tree *t,
/* first, build stack of ancestors between root and current node */
parent_stack[0] = t->root;
for (i = 0; i < n; i++) {
-   j = GET_NIBBLE(i, entry->key_sha1);
+   j = GET_NIBBLE(i, entry->key_oid.hash);
parent_stack[i + 1] = CLR_PTR_TYPE(parent_stack[i]->a[j]);
}
assert(i == n && parent_stack[i] == tree);
/* next, unwind stack until note_tree_consolidate() is done */
while (i > 0 &&
   !note_tree_consolidate(parent_stack[i], parent_stack[i - 1],
- GET_NIBBLE(i - 1, entry->key_sha1)))
+ GET_NIBBLE(i - 1, entry->key_oid.hash)))
i--;
 }
 
@@ -246,7 +246,7 @@ static int note_tree_insert(struct notes_tree *t, struct 
int_node *tree,
 {
struct int_node *new_node;
struct leaf_node *l;
-   void **p = note_tree_search(t, , , entry->key_sha1);
+   void **p = note_tree_search(t, , , entry->key_oid.hash);
int ret = 0;
 
assert(GET_PTR_TYPE(entry) == 0); /* no type bits set */
@@ -254,7 +254,7 @@ static int 

[PATCH 14/33] diff: convert fill_filespec to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 builtin/blame.c   |  2 +-
 builtin/diff.c|  4 ++--
 combine-diff.c|  4 ++--
 diff-lib.c|  2 +-
 diff-no-index.c   |  2 +-
 diff.c| 16 
 diffcore-rename.c |  2 +-
 diffcore.h|  2 +-
 grep.c|  2 +-
 line-log.c|  6 +++---
 10 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 1043e5376..5ad435380 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -163,7 +163,7 @@ int textconv_object(const char *path,
struct userdiff_driver *textconv;
 
df = alloc_filespec(path);
-   fill_filespec(df, oid->hash, oid_valid, mode);
+   fill_filespec(df, oid, oid_valid, mode);
textconv = get_textconv(df);
if (!textconv) {
free_filespec(df);
diff --git a/builtin/diff.c b/builtin/diff.c
index 8c03ddaf5..b2d7c32cd 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -57,8 +57,8 @@ static void stuff_change(struct diff_options *opt,
 
one = alloc_filespec(old_name);
two = alloc_filespec(new_name);
-   fill_filespec(one, old_oid->hash, old_oid_valid, old_mode);
-   fill_filespec(two, new_oid->hash, new_oid_valid, new_mode);
+   fill_filespec(one, old_oid, old_oid_valid, old_mode);
+   fill_filespec(two, new_oid, new_oid_valid, new_mode);
 
diff_queue(_queued_diff, one, two);
 }
diff --git a/combine-diff.c b/combine-diff.c
index 2848034fe..ad063ecb1 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -302,7 +302,7 @@ static char *grab_blob(const struct object_id *oid, 
unsigned int mode,
return xcalloc(1, 1);
} else if (textconv) {
struct diff_filespec *df = alloc_filespec(path);
-   fill_filespec(df, oid->hash, 1, mode);
+   fill_filespec(df, oid, 1, mode);
*size = fill_textconv(textconv, df, );
free_filespec(df);
} else {
@@ -1022,7 +1022,7 @@ static void show_patch_diff(struct combine_diff_path 
*elem, int num_parent,
   _size, NULL, NULL);
} else if (textconv) {
struct diff_filespec *df = alloc_filespec(elem->path);
-   fill_filespec(df, null_sha1, 0, st.st_mode);
+   fill_filespec(df, _oid, 0, st.st_mode);
result_size = fill_textconv(textconv, df, );
free_filespec(df);
} else if (0 <= (fd = open(elem->path, O_RDONLY))) {
diff --git a/diff-lib.c b/diff-lib.c
index 1e8215df5..9e076a488 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -409,7 +409,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
struct diff_filepair *pair;
pair = diff_unmerge(>diffopt, idx->name);
if (tree)
-   fill_filespec(pair->one, tree->oid.hash, 1,
+   fill_filespec(pair->one, >oid, 1,
  tree->ce_mode);
return;
}
diff --git a/diff-no-index.c b/diff-no-index.c
index 79229382b..80ff17d46 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -82,7 +82,7 @@ static struct diff_filespec *noindex_filespec(const char 
*name, int mode)
if (!name)
name = "/dev/null";
s = alloc_filespec(name);
-   fill_filespec(s, null_sha1, 0, mode);
+   fill_filespec(s, _oid, 0, mode);
if (name == file_from_standard_input)
populate_from_stdin(s);
return s;
diff --git a/diff.c b/diff.c
index b9bb3f6ca..e0c179f5f 100644
--- a/diff.c
+++ b/diff.c
@@ -2702,13 +2702,13 @@ void free_filespec(struct diff_filespec *spec)
}
 }
 
-void fill_filespec(struct diff_filespec *spec, const unsigned char *sha1,
-  int sha1_valid, unsigned short mode)
+void fill_filespec(struct diff_filespec *spec, const struct object_id *oid,
+  int oid_valid, unsigned short mode)
 {
if (mode) {
spec->mode = canon_mode(mode);
-   hashcpy(spec->oid.hash, sha1);
-   spec->oid_valid = sha1_valid;
+   oidcpy(>oid, oid);
+   spec->oid_valid = oid_valid;
}
 }
 
@@ -5114,9 +5114,9 @@ void diff_addremove(struct diff_options *options,
two = alloc_filespec(concatpath);
 
if (addremove != '+')
-   fill_filespec(one, oid->hash, oid_valid, mode);
+   fill_filespec(one, oid, oid_valid, mode);
if (addremove != '-') {
-   fill_filespec(two, oid->hash, oid_valid, mode);
+   fill_filespec(two, oid, oid_valid, mode);
two->dirty_submodule = dirty_submodule;
}
 
@@ -5153,8 +5153,8 @@ void diff_change(struct diff_options *options,
 
one = alloc_filespec(concatpath);
two = alloc_filespec(concatpath);
-   fill_filespec(one, 

[PATCH 05/33] notes: convert format_display_notes to struct object_id

[Brandon Williams]

From: "brian m. carlson" 

Signed-off-by: brian m. carlson 
Signed-off-by: Brandon Williams 
---
 log-tree.c | 2 +-
 notes.c| 8 
 notes.h| 2 +-
 revision.c | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index a4ec11c2b..9c0c64a2d 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -655,7 +655,7 @@ void show_log(struct rev_info *opt)
struct strbuf notebuf = STRBUF_INIT;
 
raw = (opt->commit_format == CMIT_FMT_USERFORMAT);
-   format_display_notes(commit->object.oid.hash, ,
+   format_display_notes(>object.oid, ,
 get_log_output_encoding(), raw);
ctx.notes_message = notebuf.len
? strbuf_detach(, NULL)
diff --git a/notes.c b/notes.c
index fe4db2c1e..b5cabafde 100644
--- a/notes.c
+++ b/notes.c
@@ -1215,7 +1215,7 @@ void free_notes(struct notes_tree *t)
  * (raw != 0) gives the %N userformat; otherwise, the note message is given
  * for human consumption.
  */
-static void format_note(struct notes_tree *t, const unsigned char *object_sha1,
+static void format_note(struct notes_tree *t, const struct object_id 
*object_oid,
struct strbuf *sb, const char *output_encoding, int raw)
 {
static const char utf8[] = "utf-8";
@@ -1229,7 +1229,7 @@ static void format_note(struct notes_tree *t, const 
unsigned char *object_sha1,
if (!t->initialized)
init_notes(t, NULL, NULL, 0);
 
-   oid = get_note(t, object_sha1);
+   oid = get_note(t, object_oid->hash);
if (!oid)
return;
 
@@ -1277,13 +1277,13 @@ static void format_note(struct notes_tree *t, const 
unsigned char *object_sha1,
free(msg);
 }
 
-void format_display_notes(const unsigned char *object_sha1,
+void format_display_notes(const struct object_id *object_oid,
  struct strbuf *sb, const char *output_encoding, int 
raw)
 {
int i;
assert(display_notes_trees);
for (i = 0; display_notes_trees[i]; i++)
-   format_note(display_notes_trees[i], object_sha1, sb,
+   format_note(display_notes_trees[i], object_oid, sb,
output_encoding, raw);
 }
 
diff --git a/notes.h b/notes.h
index c72bb9710..a66532103 100644
--- a/notes.h
+++ b/notes.h
@@ -277,7 +277,7 @@ void init_display_notes(struct display_notes_opt *opt);
  *
  * You *must* call init_display_notes() before using this function.
  */
-void format_display_notes(const unsigned char *object_sha1,
+void format_display_notes(const struct object_id *object_oid,
  struct strbuf *sb, const char *output_encoding, int 
raw);
 
 /*
diff --git a/revision.c b/revision.c
index b02394530..475d5b2dc 100644
--- a/revision.c
+++ b/revision.c
@@ -2908,7 +2908,7 @@ static int commit_match(struct commit *commit, struct 
rev_info *opt)
if (opt->show_notes) {
if (!buf.len)
strbuf_addstr(, message);
-   format_display_notes(commit->object.oid.hash, , encoding, 
1);
+   format_display_notes(>object.oid, , encoding, 1);
}
 
/*
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 00/33] object id conversion (grep and diff)

[Brandon Williams]

A month or so ago I thought I would lend a hand to Brian and do a round of
conversions from sha1 -> struct object_id.  Now that Brian's latest series has
hit master I can finally send these patches out.

The first couple patches are from Brian which convert some of the notes logic
to using 'struct object_id'.  The remaining patches are to convert the grep and
diff machinery to using 'struct object_id'.

Brandon Williams (26):
  grep: convert to struct object_id
  diff: convert get_stat_data to struct object_id
  diff: convert diff_index_show_file to struct object_id
  diff: convert diff_addremove to struct object_id
  diff: convert run_diff_files to struct object_id
  diff: convert diff_change to struct object_id
  diff: convert fill_filespec to struct object_id
  diff: convert reuse_worktree_file to struct object_id
  diff: finish conversion for prepare_temp_file to struct object_id
  patch-ids: convert to struct object_id
  diff: convert diff_flush_patch_id to struct object_id
  combine-diff: convert diff_tree_combined to struct object_id
  combine-diff: convert find_paths_* to struct object_id
  tree-diff: convert diff_root_tree_sha1 to struct object_id
  notes-merge: convert notes_merge* to struct object_id
  notes-merge: convert merge_from_diffs to struct object_id
  notes-merge: convert find_notes_merge_pair_ps to struct object_id
  notes-merge: convert verify_notes_filepair to struct object_id
  notes-merge: convert write_note_to_worktree to struct object_id
  diff-tree: convert diff_tree_sha1 to struct object_id
  builtin/diff-tree: cleanup references to sha1
  tree-diff: convert try_to_follow_renames to struct object_id
  tree-diff: convert diff_tree_paths to struct object_id
  tree-diff: convert path_appendnew to object_id
  diffcore-rename: use is_empty_blob_oid
  diff: rename diff_fill_sha1_info to diff_fill_oid_info

brian m. carlson (7):
  notes: convert internal structures to struct object_id
  notes: convert internal parts to struct object_id
  notes: convert for_each_note to struct object_id
  notes: make get_note return pointer to struct object_id
  notes: convert format_display_notes to struct object_id
  builtin/notes: convert to struct object_id
  notes: convert some accessor functions to struct object_id

 builtin/am.c  |   2 +-
 builtin/blame.c   |  22 +++---
 builtin/commit.c  |   2 +-
 builtin/diff-tree.c   |  16 ++--
 builtin/diff.c|   8 +-
 builtin/fast-export.c |   8 +-
 builtin/grep.c|  22 +++---
 builtin/log.c |   8 +-
 builtin/merge.c   |   2 +-
 builtin/notes.c   | 136 -
 cache.h   |   7 ++
 combine-diff.c|  30 
 diff-lib.c|  52 ++---
 diff-no-index.c   |   2 +-
 diff.c|  74 +-
 diff.h|  38 +-
 diffcore-rename.c |   6 +-
 diffcore.h|   2 +-
 grep.c|  19 +++--
 grep.h|   2 +-
 line-log.c|  10 +--
 log-tree.c|  12 +--
 merge-recursive.c |   2 +-
 notes-cache.c |  10 +--
 notes-merge.c | 165 +
 notes-merge.h |  12 +--
 notes-utils.c |   2 +-
 notes-utils.h |   2 +-
 notes.c   | 202 ++
 notes.h   |  16 ++--
 patch-ids.c   |  26 +++
 patch-ids.h   |   4 +-
 remote-testsvn.c  |  22 +++---
 revision.c|  16 ++--
 sequencer.c   |   4 +-
 tree-diff.c   |  93 ---
 36 files changed, 535 insertions(+), 521 deletions(-)

-- 
2.13.0.219.gdb65acc882-goog

[PATCH 06/33] builtin/notes: convert to struct object_id

[Brandon Williams]

From: "brian m. carlson" 

Convert most of the static functions to use struct object_id.  In
addition, convert copy_notes_for_rewrite and its callers.

Signed-off-by: brian m. carlson 
Signed-off-by: Brandon Williams 
---
 builtin/am.c |   2 +-
 builtin/commit.c |   2 +-
 builtin/notes.c  | 110 +++
 notes-utils.c|   4 +-
 notes-utils.h|   2 +-
 5 files changed, 60 insertions(+), 60 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 0f63dcab1..d9fdddac4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -563,7 +563,7 @@ static int copy_notes_for_rebase(const struct am_state 
*state)
goto finish;
}
 
-   if (copy_note_for_rewrite(c, from_obj.hash, to_obj.hash))
+   if (copy_note_for_rewrite(c, _obj, _obj))
ret = error(_("Failed to copy notes from '%s' to '%s'"),
oid_to_hex(_obj), 
oid_to_hex(_obj));
}
diff --git a/builtin/commit.c b/builtin/commit.c
index da1ba4c86..758781004 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1809,7 +1809,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
cfg = init_copy_notes_for_rewrite("amend");
if (cfg) {
/* we are amending, so current_head is not NULL */
-   copy_note_for_rewrite(cfg, 
current_head->object.oid.hash, oid.hash);
+   copy_note_for_rewrite(cfg, _head->object.oid, 
);
finish_copy_notes_for_rewrite(cfg, "Notes added by 'git 
commit --amend'");
}
run_rewrite_hook(_head->object.oid, );
diff --git a/builtin/notes.c b/builtin/notes.c
index 3d9005b8f..7947a16ed 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -129,10 +129,10 @@ static void copy_obj_to_fd(int fd, const unsigned char 
*sha1)
}
 }
 
-static void write_commented_object(int fd, const unsigned char *object)
+static void write_commented_object(int fd, const struct object_id *object)
 {
const char *show_args[5] =
-   {"show", "--stat", "--no-notes", sha1_to_hex(object), NULL};
+   {"show", "--stat", "--no-notes", oid_to_hex(object), NULL};
struct child_process show = CHILD_PROCESS_INIT;
struct strbuf buf = STRBUF_INIT;
struct strbuf cbuf = STRBUF_INIT;
@@ -145,7 +145,7 @@ static void write_commented_object(int fd, const unsigned 
char *object)
show.git_cmd = 1;
if (start_command())
die(_("unable to start 'show' for object '%s'"),
-   sha1_to_hex(object));
+   oid_to_hex(object));
 
if (strbuf_read(, show.out, 0) < 0)
die_errno(_("could not read 'show' output"));
@@ -157,10 +157,10 @@ static void write_commented_object(int fd, const unsigned 
char *object)
 
if (finish_command())
die(_("failed to finish 'show' for object '%s'"),
-   sha1_to_hex(object));
+   oid_to_hex(object));
 }
 
-static void prepare_note_data(const unsigned char *object, struct note_data *d,
+static void prepare_note_data(const struct object_id *object, struct note_data 
*d,
const unsigned char *old_note)
 {
if (d->use_editor || !d->given) {
@@ -243,16 +243,16 @@ static int parse_reuse_arg(const struct option *opt, 
const char *arg, int unset)
 {
struct note_data *d = opt->value;
char *buf;
-   unsigned char object[20];
+   struct object_id object;
enum object_type type;
unsigned long len;
 
if (d->buf.len)
strbuf_addch(>buf, '\n');
 
-   if (get_sha1(arg, object))
+   if (get_oid(arg, ))
die(_("failed to resolve '%s' as a valid ref."), arg);
-   if (!(buf = read_sha1_file(object, , ))) {
+   if (!(buf = read_sha1_file(object.hash, , ))) {
free(buf);
die(_("failed to read object '%s'."), arg);
}
@@ -292,7 +292,7 @@ static int notes_copy_from_stdin(int force, const char 
*rewrite_cmd)
}
 
while (strbuf_getline_lf(, stdin) != EOF) {
-   unsigned char from_obj[20], to_obj[20];
+   struct object_id from_obj, to_obj;
struct strbuf **split;
int err;
 
@@ -301,15 +301,15 @@ static int notes_copy_from_stdin(int force, const char 
*rewrite_cmd)
die(_("malformed input line: '%s'."), buf.buf);
strbuf_rtrim(split[0]);
strbuf_rtrim(split[1]);
-   if (get_sha1(split[0]->buf, from_obj))
+   if (get_oid(split[0]->buf, _obj))
die(_("failed to resolve '%s' as a valid ref."), 
split[0]->buf);
-   if (get_sha1(split[1]->buf, to_obj))
+   

[PATCH 08/33] grep: convert to struct object_id

[Brandon Williams]

Convert the remaining parts of grep to use struct object_id.

Signed-off-by: Brandon Williams 
---
 builtin/grep.c | 22 +++---
 cache.h|  7 +++
 grep.c | 17 -
 grep.h |  2 +-
 4 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index c6c26e9b9..623c13a93 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -327,7 +327,7 @@ static int grep_oid(struct grep_opt *opt, const struct 
object_id *oid,
 
 #ifndef NO_PTHREADS
if (num_threads) {
-   add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, oid);
+   add_work(opt, GREP_SOURCE_OID, pathbuf.buf, path, oid);
strbuf_release();
return 0;
} else
@@ -336,7 +336,7 @@ static int grep_oid(struct grep_opt *opt, const struct 
object_id *oid,
struct grep_source gs;
int hit;
 
-   grep_source_init(, GREP_SOURCE_SHA1, pathbuf.buf, path, oid);
+   grep_source_init(, GREP_SOURCE_OID, pathbuf.buf, path, oid);
strbuf_release();
hit = grep_source(opt, );
 
@@ -570,7 +570,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
 * with the object's name: 'tree-name:filename'.  In order to
 * provide uniformity of output we want to pass the name of the
 * parent project's object name to the submodule so the submodule can
-* prefix its output with the parent's name and not its own SHA1.
+* prefix its output with the parent's name and not its own OID.
 */
if (gs->identifier && end_of_base)
argv_array_pushf(, "--parent-basename=%.*s",
@@ -583,12 +583,12 @@ static int grep_submodule_launch(struct grep_opt *opt,
 * If there is a tree identifier for the submodule, add the
 * rev after adding the submodule options but before the
 * pathspecs.  To do this we listen for the '--' and insert the
-* sha1 before pushing the '--' onto the child process argv
+* oid before pushing the '--' onto the child process argv
 * array.
 */
if (gs->identifier &&
!strcmp("--", submodule_options.argv[i])) {
-   argv_array_push(, sha1_to_hex(gs->identifier));
+   argv_array_push(, oid_to_hex(gs->identifier));
}
 
argv_array_push(, submodule_options.argv[i]);
@@ -618,11 +618,11 @@ static int grep_submodule_launch(struct grep_opt *opt,
 
 /*
  * Prep grep structures for a submodule grep
- * sha1: the sha1 of the submodule or NULL if using the working tree
+ * oid: the oid of the submodule or NULL if using the working tree
  * filename: name of the submodule including tree name of parent
  * path: location of the submodule
  */
-static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
+static int grep_submodule(struct grep_opt *opt, const struct object_id *oid,
  const char *filename, const char *path)
 {
if (!is_submodule_initialized(path))
@@ -632,7 +632,7 @@ static int grep_submodule(struct grep_opt *opt, const 
unsigned char *sha1,
 * If searching history, check for the presense of the
 * submodule's gitdir before skipping the submodule.
 */
-   if (sha1) {
+   if (oid) {
const struct submodule *sub =
submodule_from_path(null_sha1, path);
if (sub)
@@ -647,7 +647,7 @@ static int grep_submodule(struct grep_opt *opt, const 
unsigned char *sha1,
 
 #ifndef NO_PTHREADS
if (num_threads) {
-   add_work(opt, GREP_SOURCE_SUBMODULE, filename, path, sha1);
+   add_work(opt, GREP_SOURCE_SUBMODULE, filename, path, oid);
return 0;
} else
 #endif
@@ -656,7 +656,7 @@ static int grep_submodule(struct grep_opt *opt, const 
unsigned char *sha1,
int hit;
 
grep_source_init(, GREP_SOURCE_SUBMODULE,
-filename, path, sha1);
+filename, path, oid);
hit = grep_submodule_launch(opt, );
 
grep_source_clear();
@@ -775,7 +775,7 @@ static int grep_tree(struct grep_opt *opt, const struct 
pathspec *pathspec,
 check_attr);
free(data);
} else if (recurse_submodules && S_ISGITLINK(entry.mode)) {
-   hit |= grep_submodule(opt, entry.oid->hash, base->buf,
+   hit |= grep_submodule(opt, entry.oid, base->buf,
  base->buf + tn_len);
}
 
diff --git a/cache.h b/cache.h
index ae4c45d37..50fd2b3cc 100644
--- a/cache.h

[PATCH 15/33] diff: convert reuse_worktree_file to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 diff.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/diff.c b/diff.c
index e0c179f5f..084c8b2d0 100644
--- a/diff.c
+++ b/diff.c
@@ -2717,7 +2717,7 @@ void fill_filespec(struct diff_filespec *spec, const 
struct object_id *oid,
  * the work tree has that object contents, return true, so that
  * prepare_temp_file() does not have to inflate and extract.
  */
-static int reuse_worktree_file(const char *name, const unsigned char *sha1, 
int want_file)
+static int reuse_worktree_file(const char *name, const struct object_id *oid, 
int want_file)
 {
const struct cache_entry *ce;
struct stat st;
@@ -2748,7 +2748,7 @@ static int reuse_worktree_file(const char *name, const 
unsigned char *sha1, int
 * objects however would tend to be slower as they need
 * to be individually opened and inflated.
 */
-   if (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1))
+   if (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(oid->hash))
return 0;
 
/*
@@ -2768,7 +2768,7 @@ static int reuse_worktree_file(const char *name, const 
unsigned char *sha1, int
 * This is not the sha1 we are looking for, or
 * unreusable because it is not a regular file.
 */
-   if (hashcmp(sha1, ce->oid.hash) || !S_ISREG(ce->ce_mode))
+   if (oidcmp(oid, >oid) || !S_ISREG(ce->ce_mode))
return 0;
 
/*
@@ -2842,7 +2842,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
return diff_populate_gitlink(s, size_only);
 
if (!s->oid_valid ||
-   reuse_worktree_file(s->path, s->oid.hash, 0)) {
+   reuse_worktree_file(s->path, >oid, 0)) {
struct strbuf buf = STRBUF_INIT;
struct stat st;
int fd;
@@ -3008,7 +3008,7 @@ static struct diff_tempfile *prepare_temp_file(const char 
*name,
 
if (!S_ISGITLINK(one->mode) &&
(!one->oid_valid ||
-reuse_worktree_file(name, one->oid.hash, 1))) {
+reuse_worktree_file(name, >oid, 1))) {
struct stat st;
if (lstat(name, ) < 0) {
if (errno == ENOENT)
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 07/33] notes: convert some accessor functions to struct object_id

[Brandon Williams]

From: "brian m. carlson" 

Convert add_note, get_note, and copy_note to take struct object_id.

Signed-off-by: brian m. carlson 
Signed-off-by: Brandon Williams 
---
 builtin/notes.c  | 20 ++--
 notes-cache.c|  4 ++--
 notes-merge.c| 18 +-
 notes-utils.c|  2 +-
 notes.c  | 20 ++--
 notes.h  |  8 
 remote-testsvn.c | 10 +-
 7 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 7947a16ed..b13fc8789 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -309,7 +309,7 @@ static int notes_copy_from_stdin(int force, const char 
*rewrite_cmd)
if (rewrite_cmd)
err = copy_note_for_rewrite(c, _obj, _obj);
else
-   err = copy_note(t, from_obj.hash, to_obj.hash, force,
+   err = copy_note(t, _obj, _obj, force,
combine_notes_overwrite);
 
if (err) {
@@ -370,7 +370,7 @@ static int list(int argc, const char **argv, const char 
*prefix)
if (argc) {
if (get_oid(argv[0], ))
die(_("failed to resolve '%s' as a valid ref."), 
argv[0]);
-   note = get_note(t, object.hash);
+   note = get_note(t, );
if (note) {
puts(oid_to_hex(note));
retval = 0;
@@ -427,7 +427,7 @@ static int add(int argc, const char **argv, const char 
*prefix)
die(_("failed to resolve '%s' as a valid ref."), object_ref);
 
t = init_notes_check("add", NOTES_INIT_WRITABLE);
-   note = get_note(t, object.hash);
+   note = get_note(t, );
 
if (note) {
if (!force) {
@@ -456,7 +456,7 @@ static int add(int argc, const char **argv, const char 
*prefix)
prepare_note_data(, , note->hash);
if (d.buf.len || allow_empty) {
write_note_data(, new_note.hash);
-   if (add_note(t, object.hash, new_note.hash, 
combine_notes_overwrite))
+   if (add_note(t, , _note, combine_notes_overwrite))
die("BUG: combine_notes_overwrite failed");
commit_notes(t, "Notes added by 'git notes add'");
} else {
@@ -518,7 +518,7 @@ static int copy(int argc, const char **argv, const char 
*prefix)
die(_("failed to resolve '%s' as a valid ref."), object_ref);
 
t = init_notes_check("copy", NOTES_INIT_WRITABLE);
-   note = get_note(t, object.hash);
+   note = get_note(t, );
 
if (note) {
if (!force) {
@@ -532,14 +532,14 @@ static int copy(int argc, const char **argv, const char 
*prefix)
oid_to_hex());
}
 
-   from_note = get_note(t, from_obj.hash);
+   from_note = get_note(t, _obj);
if (!from_note) {
retval = error(_("missing notes on source object %s. Cannot "
   "copy."), oid_to_hex(_obj));
goto out;
}
 
-   if (add_note(t, object.hash, from_note->hash, combine_notes_overwrite))
+   if (add_note(t, , from_note, combine_notes_overwrite))
die("BUG: combine_notes_overwrite failed");
commit_notes(t, "Notes added by 'git notes copy'");
 out:
@@ -596,7 +596,7 @@ static int append_edit(int argc, const char **argv, const 
char *prefix)
die(_("failed to resolve '%s' as a valid ref."), object_ref);
 
t = init_notes_check(argv[0], NOTES_INIT_WRITABLE);
-   note = get_note(t, object.hash);
+   note = get_note(t, );
 
prepare_note_data(, , edit && note ? note->hash : NULL);
 
@@ -616,7 +616,7 @@ static int append_edit(int argc, const char **argv, const 
char *prefix)
 
if (d.buf.len || allow_empty) {
write_note_data(, new_note.hash);
-   if (add_note(t, object.hash, new_note.hash, 
combine_notes_overwrite))
+   if (add_note(t, , _note, combine_notes_overwrite))
die("BUG: combine_notes_overwrite failed");
logmsg = xstrfmt("Notes added by 'git notes %s'", argv[0]);
} else {
@@ -658,7 +658,7 @@ static int show(int argc, const char **argv, const char 
*prefix)
die(_("failed to resolve '%s' as a valid ref."), object_ref);
 
t = init_notes_check("show", 0);
-   note = get_note(t, object.hash);
+   note = get_note(t, );
 
if (!note)
retval = error(_("no note found for object %s."),
diff --git a/notes-cache.c b/notes-cache.c
index 6e84a748f..29b4cede5 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -74,7 +74,7 @@ char *notes_cache_get(struct notes_cache *c, struct object_id 
*key_oid,
char *value;
unsigned long size;
 
-   value_oid = get_note(>tree, 

[PATCH 10/33] diff: convert diff_index_show_file to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 diff-lib.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index a3bc78162..2c838aaf4 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -255,12 +255,12 @@ int run_diff_files(struct rev_info *revs, unsigned int 
option)
 static void diff_index_show_file(struct rev_info *revs,
 const char *prefix,
 const struct cache_entry *ce,
-const unsigned char *sha1, int sha1_valid,
+const struct object_id *oid, int oid_valid,
 unsigned int mode,
 unsigned dirty_submodule)
 {
diff_addremove(>diffopt, prefix[0], mode,
-  sha1, sha1_valid, ce->name, dirty_submodule);
+  oid->hash, oid_valid, ce->name, dirty_submodule);
 }
 
 static int get_stat_data(const struct cache_entry *ce,
@@ -315,7 +315,7 @@ static void show_new_file(struct rev_info *revs,
_submodule, >diffopt) < 0)
return;
 
-   diff_index_show_file(revs, "+", new, oid->hash, !is_null_oid(oid), 
mode, dirty_submodule);
+   diff_index_show_file(revs, "+", new, oid, !is_null_oid(oid), mode, 
dirty_submodule);
 }
 
 static int show_modified(struct rev_info *revs,
@@ -332,7 +332,7 @@ static int show_modified(struct rev_info *revs,
  _submodule, >diffopt) < 0) {
if (report_missing)
diff_index_show_file(revs, "-", old,
-old->oid.hash, 1, old->ce_mode,
+>oid, 1, old->ce_mode,
 0);
return -1;
}
@@ -426,7 +426,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 * Something removed from the tree?
 */
if (!idx) {
-   diff_index_show_file(revs, "-", tree, tree->oid.hash, 1,
+   diff_index_show_file(revs, "-", tree, >oid, 1,
 tree->ce_mode, 0);
return;
}
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 04/33] notes: make get_note return pointer to struct object_id

[Brandon Williams]

From: "brian m. carlson" 

Make get_note return a pointer to a const struct object_id.  Add a
defensive check to ensure we don't accidentally dereference a NULL
pointer.

Signed-off-by: brian m. carlson 
Signed-off-by: Brandon Williams 
---
 builtin/notes.c  | 22 +++---
 notes-cache.c|  8 
 notes.c  | 18 +-
 notes.h  |  2 +-
 remote-testsvn.c |  6 +++---
 5 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 53fe6d34d..3d9005b8f 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -351,7 +351,7 @@ static int list(int argc, const char **argv, const char 
*prefix)
 {
struct notes_tree *t;
unsigned char object[20];
-   const unsigned char *note;
+   const struct object_id *note;
int retval = -1;
struct option options[] = {
OPT_END()
@@ -372,7 +372,7 @@ static int list(int argc, const char **argv, const char 
*prefix)
die(_("failed to resolve '%s' as a valid ref."), 
argv[0]);
note = get_note(t, object);
if (note) {
-   puts(sha1_to_hex(note));
+   puts(oid_to_hex(note));
retval = 0;
} else
retval = error(_("no note found for object %s."),
@@ -392,7 +392,7 @@ static int add(int argc, const char **argv, const char 
*prefix)
const char *object_ref;
struct notes_tree *t;
unsigned char object[20], new_note[20];
-   const unsigned char *note;
+   const struct object_id *note;
struct note_data d = { 0, 0, NULL, STRBUF_INIT };
struct option options[] = {
{ OPTION_CALLBACK, 'm', "message", , N_("message"),
@@ -453,7 +453,7 @@ static int add(int argc, const char **argv, const char 
*prefix)
sha1_to_hex(object));
}
 
-   prepare_note_data(object, , note);
+   prepare_note_data(object, , note->hash);
if (d.buf.len || allow_empty) {
write_note_data(, new_note);
if (add_note(t, object, new_note, combine_notes_overwrite))
@@ -474,7 +474,7 @@ static int add(int argc, const char **argv, const char 
*prefix)
 static int copy(int argc, const char **argv, const char *prefix)
 {
int retval = 0, force = 0, from_stdin = 0;
-   const unsigned char *from_note, *note;
+   const struct object_id *from_note, *note;
const char *object_ref;
unsigned char object[20], from_obj[20];
struct notes_tree *t;
@@ -539,7 +539,7 @@ static int copy(int argc, const char **argv, const char 
*prefix)
goto out;
}
 
-   if (add_note(t, object, from_note, combine_notes_overwrite))
+   if (add_note(t, object, from_note->hash, combine_notes_overwrite))
die("BUG: combine_notes_overwrite failed");
commit_notes(t, "Notes added by 'git notes copy'");
 out:
@@ -553,7 +553,7 @@ static int append_edit(int argc, const char **argv, const 
char *prefix)
const char *object_ref;
struct notes_tree *t;
unsigned char object[20], new_note[20];
-   const unsigned char *note;
+   const struct object_id *note;
char *logmsg;
const char * const *usage;
struct note_data d = { 0, 0, NULL, STRBUF_INIT };
@@ -598,13 +598,13 @@ static int append_edit(int argc, const char **argv, const 
char *prefix)
t = init_notes_check(argv[0], NOTES_INIT_WRITABLE);
note = get_note(t, object);
 
-   prepare_note_data(object, , edit ? note : NULL);
+   prepare_note_data(object, , edit && note ? note->hash : NULL);
 
if (note && !edit) {
/* Append buf to previous note contents */
unsigned long size;
enum object_type type;
-   char *prev_buf = read_sha1_file(note, , );
+   char *prev_buf = read_sha1_file(note->hash, , );
 
strbuf_grow(, size + 1);
if (d.buf.len && prev_buf && size)
@@ -638,7 +638,7 @@ static int show(int argc, const char **argv, const char 
*prefix)
const char *object_ref;
struct notes_tree *t;
unsigned char object[20];
-   const unsigned char *note;
+   const struct object_id *note;
int retval;
struct option options[] = {
OPT_END()
@@ -664,7 +664,7 @@ static int show(int argc, const char **argv, const char 
*prefix)
retval = error(_("no note found for object %s."),
   sha1_to_hex(object));
else {
-   const char *show_args[3] = {"show", sha1_to_hex(note), NULL};
+   const char *show_args[3] = {"show", oid_to_hex(note), NULL};
retval = execv_git_cmd(show_args);
}
free_notes(t);
diff --git 

[PATCH 02/33] notes: convert internal parts to struct object_id

[Brandon Williams]

From: "brian m. carlson" 

Convert several portions of the internals of the code to struct
object_id.  Introduce two macros to denote the different constants in
the code: KEY_INDEX for the last byte of the object ID, and
FANOUT_PATH_SEPARATORS for the number of possible path separators (on
Unix, "/").  While these constants are both 19 (one less than the number
of bytes in the hash), distinguish them to make the code more
understandable, and define them logically based on their intended
purpose.

Signed-off-by: brian m. carlson 
Signed-off-by: Brandon Williams 
---
 notes.c | 64 +---
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/notes.c b/notes.c
index 251cf11c9..babe0c0eb 100644
--- a/notes.c
+++ b/notes.c
@@ -65,8 +65,10 @@ struct non_note {
 
 #define GET_NIBBLE(n, sha1) (((sha1[(n) >> 1]) >> ((~(n) & 0x01) << 2)) & 0x0f)
 
+#define KEY_INDEX (GIT_SHA1_RAWSZ - 1)
+#define FANOUT_PATH_SEPARATORS ((GIT_SHA1_HEXSZ / 2) - 1)
 #define SUBTREE_SHA1_PREFIXCMP(key_sha1, subtree_sha1) \
-   (memcmp(key_sha1, subtree_sha1, subtree_sha1[19]))
+   (memcmp(key_sha1, subtree_sha1, subtree_sha1[KEY_INDEX]))
 
 struct notes_tree default_notes_tree;
 
@@ -194,7 +196,7 @@ static void note_tree_remove(struct notes_tree *t,
struct leaf_node *entry)
 {
struct leaf_node *l;
-   struct int_node *parent_stack[20];
+   struct int_node *parent_stack[GIT_SHA1_RAWSZ];
unsigned char i, j;
void **p = note_tree_search(t, , , entry->key_oid.hash);
 
@@ -341,21 +343,21 @@ static void note_tree_free(struct int_node *tree)
  * Otherwise, returns number of bytes written to sha1 (i.e. hex_len / 2).
  * Pads sha1 with NULs up to sha1_len (not included in returned length).
  */
-static int get_sha1_hex_segment(const char *hex, unsigned int hex_len,
-   unsigned char *sha1, unsigned int sha1_len)
+static int get_oid_hex_segment(const char *hex, unsigned int hex_len,
+   unsigned char *oid, unsigned int oid_len)
 {
unsigned int i, len = hex_len >> 1;
-   if (hex_len % 2 != 0 || len > sha1_len)
+   if (hex_len % 2 != 0 || len > oid_len)
return -1;
for (i = 0; i < len; i++) {
unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);
if (val & ~0xff)
return -1;
-   *sha1++ = val;
+   *oid++ = val;
hex += 2;
}
-   for (; i < sha1_len; i++)
-   *sha1++ = 0;
+   for (; i < oid_len; i++)
+   *oid++ = 0;
return len;
 }
 
@@ -413,7 +415,7 @@ static void add_non_note(struct notes_tree *t, char *path,
 static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
struct int_node *node, unsigned int n)
 {
-   unsigned char object_sha1[20];
+   struct object_id object_oid;
unsigned int prefix_len;
void *buf;
struct tree_desc desc;
@@ -427,13 +429,13 @@ static void load_subtree(struct notes_tree *t, struct 
leaf_node *subtree,
die("Could not read %s for notes-index",
 oid_to_hex(>val_oid));
 
-   prefix_len = subtree->key_oid.hash[19];
+   prefix_len = subtree->key_oid.hash[KEY_INDEX];
assert(prefix_len * 2 >= n);
-   memcpy(object_sha1, subtree->key_oid.hash, prefix_len);
+   memcpy(object_oid.hash, subtree->key_oid.hash, prefix_len);
while (tree_entry(, )) {
path_len = strlen(entry.path);
-   len = get_sha1_hex_segment(entry.path, path_len,
-   object_sha1 + prefix_len, 20 - prefix_len);
+   len = get_oid_hex_segment(entry.path, path_len,
+   object_oid.hash + prefix_len, GIT_SHA1_RAWSZ - 
prefix_len);
if (len < 0)
goto handle_non_note; /* entry.path is not a SHA1 */
len += prefix_len;
@@ -443,16 +445,16 @@ static void load_subtree(struct notes_tree *t, struct 
leaf_node *subtree,
 * If object SHA1 is incomplete (len < 20), and current
 * component consists of 2 hex chars, assume note subtree
 */
-   if (len <= 20) {
+   if (len <= GIT_SHA1_RAWSZ) {
type = PTR_TYPE_NOTE;
l = (struct leaf_node *)
xcalloc(1, sizeof(struct leaf_node));
-   hashcpy(l->key_oid.hash, object_sha1);
+   oidcpy(>key_oid, _oid);
oidcpy(>val_oid, entry.oid);
-   if (len < 20) {
+   if (len < GIT_SHA1_RAWSZ) {
if (!S_ISDIR(entry.mode) || path_len != 2)
goto 

[PATCH 09/33] diff: convert get_stat_data to struct object_id

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 diff-lib.c | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 2982bf055..a3bc78162 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -264,12 +264,12 @@ static void diff_index_show_file(struct rev_info *revs,
 }
 
 static int get_stat_data(const struct cache_entry *ce,
-const unsigned char **sha1p,
+const struct object_id **oidp,
 unsigned int *modep,
 int cached, int match_missing,
 unsigned *dirty_submodule, struct diff_options 
*diffopt)
 {
-   const unsigned char *sha1 = ce->oid.hash;
+   const struct object_id *oid = >oid;
unsigned int mode = ce->ce_mode;
 
if (!cached && !ce_uptodate(ce)) {
@@ -280,7 +280,7 @@ static int get_stat_data(const struct cache_entry *ce,
return -1;
else if (changed) {
if (match_missing) {
-   *sha1p = sha1;
+   *oidp = oid;
*modep = mode;
return 0;
}
@@ -290,11 +290,11 @@ static int get_stat_data(const struct cache_entry *ce,
0, dirty_submodule);
if (changed) {
mode = ce_mode_from_stat(ce, st.st_mode);
-   sha1 = null_sha1;
+   oid = _oid;
}
}
 
-   *sha1p = sha1;
+   *oidp = oid;
*modep = mode;
return 0;
 }
@@ -303,7 +303,7 @@ static void show_new_file(struct rev_info *revs,
  const struct cache_entry *new,
  int cached, int match_missing)
 {
-   const unsigned char *sha1;
+   const struct object_id *oid;
unsigned int mode;
unsigned dirty_submodule = 0;
 
@@ -311,11 +311,11 @@ static void show_new_file(struct rev_info *revs,
 * New file in the index: it might actually be different in
 * the working tree.
 */
-   if (get_stat_data(new, , , cached, match_missing,
+   if (get_stat_data(new, , , cached, match_missing,
_submodule, >diffopt) < 0)
return;
 
-   diff_index_show_file(revs, "+", new, sha1, !is_null_sha1(sha1), mode, 
dirty_submodule);
+   diff_index_show_file(revs, "+", new, oid->hash, !is_null_oid(oid), 
mode, dirty_submodule);
 }
 
 static int show_modified(struct rev_info *revs,
@@ -325,10 +325,10 @@ static int show_modified(struct rev_info *revs,
 int cached, int match_missing)
 {
unsigned int mode, oldmode;
-   const unsigned char *sha1;
+   const struct object_id *oid;
unsigned dirty_submodule = 0;
 
-   if (get_stat_data(new, , , cached, match_missing,
+   if (get_stat_data(new, , , cached, match_missing,
  _submodule, >diffopt) < 0) {
if (report_missing)
diff_index_show_file(revs, "-", old,
@@ -338,7 +338,7 @@ static int show_modified(struct rev_info *revs,
}
 
if (revs->combine_merges && !cached &&
-   (hashcmp(sha1, old->oid.hash) || oidcmp(>oid, >oid))) {
+   (oidcmp(oid, >oid) || oidcmp(>oid, >oid))) {
struct combine_diff_path *p;
int pathlen = ce_namelen(new);
 
@@ -362,12 +362,12 @@ static int show_modified(struct rev_info *revs,
}
 
oldmode = old->ce_mode;
-   if (mode == oldmode && !hashcmp(sha1, old->oid.hash) && 
!dirty_submodule &&
+   if (mode == oldmode && !oidcmp(oid, >oid) && !dirty_submodule &&
!DIFF_OPT_TST(>diffopt, FIND_COPIES_HARDER))
return 0;
 
diff_change(>diffopt, oldmode, mode,
-   old->oid.hash, sha1, 1, !is_null_sha1(sha1),
+   old->oid.hash, oid->hash, 1, !is_null_oid(oid),
old->name, 0, dirty_submodule);
return 0;
 }
-- 
2.13.0.219.gdb65acc882-goog

Re: [WIP/RFC 00/23] repository object

[Brandon Williams]

On 05/29, Duy Nguyen wrote:
> On Mon, May 29, 2017 at 6:23 PM, Ævar Arnfjörð Bjarmason
>  wrote:
> >>> That said, even if we never reached the point where we could handle all
> >>> submodule requests in-process, I think sticking the repo-related global
> >>> state in a struct certainly could not hurt general readability. So it's
> >>> a good direction regardless of whether we take it all the way.
> >>
> >> I doubt we would reach the point where libgit.a can handle all
> >> submodule operations in-process either. That would put libgit.a in a
> >> direct competitor position with libgit2.
> >
> > Wouldn't that be a good thing? We already have some users (e.g.
> > Microsoft) choosing not to use libgit and instead use git.git because
> > the latter is generally more mature, if git.git gains more libgit
> > features without harming other things it'll be more useful to more
> > people.
> 
> Whether it's a good thing might depend on view point. Similar to linux
> kernel, we might want some freedom to break ABI and refactor the code.
> But I think it's too early  to discuss this right now. There's lots of
> work (the "die()" minefield comes to mind) before we reach the point
> where libgit.a may be good enough for external use.

My personal end goal is making the code easier to parse and to make
submodule work easier.  I know I've only worked on the project for a
short time but I already regret some of the submodule changes that I've
made because they are straight up hacks which I want to eradicate sooner
rather than later (just look at my patches to get 'git grep' to handle
submodules...).

I wasn't thinking about libgit2 and don't really haven't thought too
much about the possibility of being in competition with that project.

-- 
Brandon Williams

Re: git push recurse.submodules behavior changed in 2.13

[Brandon Williams]

On 05/30, John Shahid wrote:
> Junio, sorry for the poor report. I totally forgot to describe the
> behavior that i'm currently getting vs what i expect.
> 
> Expected behavior:
> 
> We have a parent repo on a branch called "develop" and a submodule on
> a branch called "master". Prior to git version 2.13 if we had an
> unpushed commit in the submodule and ran "git push origin develop
> --recurse-submodules=on-demand" git would happily push the develop
> branch of the parent repo and the master branch of the submodule,
> e.g.:

Yeah my patches would definitely break that kind of workflow because
they assumed that if you actually provided a refspec + --recurse that
you would want it propagated down.  When developing those patches I was
trying to avoid needing to add an additional flag to do the propagation
but given people were already relying on this behavior it looks like
that may be the only course of action.

> 
> > Pushing submodule 'sub'
> > Counting objects: 2, done.
> > Delta compression using up to 4 threads.
> > Compressing objects: 100% (2/2), done.
> > Writing objects: 100% (2/2), 242 bytes | 0 bytes/s, done.
> > Total 2 (delta 0), reused 0 (delta 0)
> > To /home/jvshahid/codez/git/t/trash 
> > directory.t9904-diff-branch-submodule-push/sub.git
> >3cd2129..69cbc06  master -> master
> > Counting objects: 2, done.
> > Delta compression using up to 4 threads.
> > Compressing objects: 100% (2/2), done.
> > Writing objects: 100% (2/2), 283 bytes | 0 bytes/s, done.
> > Total 2 (delta 0), reused 0 (delta 0)
> > To ../pub.git
> >7ff6fca..945bc12  develop -> develop
> > ok 2 - push if submodule has is on a different branch
> 
> Actual behavior:
> 
> After the change mentioned in my previous email, git would propagate
> the refspec from the parent repo to the submodule, i.e. it would try
> to push a branch called "develop" in the submodule which would error
> since no branch with that name exist in the submodule. Here is a
> sample output with git v2.13.0:
> 
> > pushing to ref refs/heads/develop:refs/heads/develop
> > pushging to remote origin
> > fatal: src refspec 'refs/heads/develop' must name a ref
> > fatal: process for submodule 'sub' failed
> > fatal: The remote end hung up unexpectedly
> 
> I hope this clarifies my bug report.
> 
> Stefan, one little correction. I don't think the commit called out
> this behavior. The commit message was talking about unconfigured
> remotes (i.e. pushing to a url or local path) to not propagate the
> refspec and preserve the current behavior. Judging from the code and a
> test case that I wrote, this behavior is working as expected. That is,
> git won't propagate the refspec.
> 
> Cheers,
> 
> JS
> 
> On Mon, May 29, 2017 at 12:20 AM, Stefan Beller  wrote:
> > On Sun, May 28, 2017 at 7:44 PM, Junio C Hamano  wrote:
> >> John Shahid  writes:
> >>
> >>> It looks like the git push recurse-submodules behavior has changed.
> >>> Currently with 2.13 you cannot run "git push
> >>> --recurse-submodules=on-demand" if the parent repo is on a different
> >>> branch than the sub repos, e.g. parent repo is on "develop" and
> >>> sub-repo on "master". I created a test that can be found here [1].
> >>>
> >>> A bisect shows that the change to propagate refspec [2] to the
> >>> submodules is the culprit. imho this is an undesired change in
> >>> behavior. I looked at the code but couldn't see an easy way to fix
> >>> this issue without breaking the feature mentioned above. The only
> >>> option I can think of is to control the refspec propagation behavior
> >>> using a flag, e.g. "--propagate-refspecs" or add another
> >>> recurse-submodules option, e.g. "--recurse-submodules=propagate"
> >>>
> >>> What do you all think ?
> >>>
> >>> [1] https://gist.github.com/jvshahid/b778702cc3d825c6887d2707e866a9c8
> >>> [2] 
> >>> https://github.com/git/git/commit/06bf4ad1db92c32af38e16d9b7f928edbd647780
> >>
> >> Brandon?  I cannot quite tell from the report what "has changed"
> >> refers to, what failures "you cannot run" gets, and if that is a
> >> desirable thing to do (i.e. if letting the user run it in such a
> >> configuration would somehow break things, actively erroring out may
> >> be a deliberate change) or not (i.e. an unintended regression).
> >>
> >
> > Before the refspec was passed down into the submodules,
> > we'd just invoke "git push" in the submodule assuming the user
> > setup a remote tracking branch and a push strategy such that
> > "git push" would do the right thing.
> > And because the submodule is configured independently, it
> > doesn't matter which branch you're on in the superproject.
> >
> > Looking at the test[1], you run "git push --recurse-submodules"
> > without any remote/branch that was called out in the commit
> > message[2] to not have changed. Is that understanding correct?
> >
> > Looking at the test cases of [2] we did not test for explicit
> > "still works with no args given", though 

Re: [PATCH] C style: use standard style for "TRANSLATORS" comments

[Jiang Xin]

2017-05-12 5:20 GMT+08:00 Ævar Arnfjörð Bjarmason :
> Change all the "TRANSLATORS: [...]" comments in the C code to use the
> regular Git coding style, and amend the style guide so that the
> example there uses that style.
>
> This custom style was necessary back in 2010 when the gettext support
> was initially added, and was subsequently documented in commit
> cbcfd4e3ea ("i18n: mention "TRANSLATORS:" marker in
> Documentation/CodingGuidelines", 2014-04-18).
>
> GNU xgettext hasn't had the parsing limitation that necessitated this
> exception for almost 3 years. Since its 0.19 release on 2014-06-02
> it's been able to recognize TRANSLATOR comments in the standard Git
> comment syntax[1].

My gettext version is 0.19.8.1.  I applied this patch and checked the
new generated `git.pot` file, all "TRANSLATORS:" directions are well
kept as usual.

This patch is nice.

-- 
Jiang Xin

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[liam Beguin]

Hi Johannes,

On 29/05/17 06:59 AM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Thu, 25 May 2017, Liam Beguin wrote:
> 
>> Johannes Schindelin  writes:
>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 130cc868e51..88819a1a2a9 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -2388,3 +2388,52 @@ void append_signoff(struct strbuf *msgbuf, int 
>>> ignore_footer, unsigned flag)
>>>  
>>> strbuf_release();
>>>  }
>>> +
>>> +int sequencer_make_script(int keep_empty, FILE *out,
>>> +   int argc, const char **argv)
>>> +{
>>> +   char *format = NULL;
>>> +   struct pretty_print_context pp = {0};
>>> +   struct strbuf buf = STRBUF_INIT;
>>> +   struct rev_info revs;
>>> +   struct commit *commit;
>>> +
>>> +   init_revisions(, NULL);
>>> +   revs.verbose_header = 1;
>>> +   revs.max_parents = 1;
>>> +   revs.cherry_pick = 1;
>>> +   revs.limited = 1;
>>> +   revs.reverse = 1;
>>> +   revs.right_only = 1;
>>> +   revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
>>> +   revs.topo_order = 1;
>>> +
>>> +   revs.pretty_given = 1;
>>> +   git_config_get_string("rebase.instructionFormat", );
>>> +   if (!format || !*format) {
>>> +   free(format);
>>> +   format = xstrdup("%s");
>>> +   }
>>> +   get_commit_format(format, );
>>> +   free(format);
>>> +   pp.fmt = revs.commit_format;
>>> +   pp.output_encoding = get_log_output_encoding();
>>> +
>>> +   if (setup_revisions(argc, argv, , NULL) > 1)
>>> +   return error(_("make_script: unhandled options"));
>>> +
>>> +   if (prepare_revision_walk() < 0)
>>> +   return error(_("make_script: error preparing revisions"));
>>> +
>>> +   while ((commit = get_revision())) {
>>> +   strbuf_reset();
>>> +   if (!keep_empty && is_original_commit_empty(commit))
>>> +   strbuf_addf(, "%c ", comment_line_char);
>>
>> I've never had to use empty commits before, but while testing this, I
>> noticed that `git rebase -i --keep-empty` behaves differently if using
>> the --root option instead of a branch or something like 'HEAD~10'.
>> I also tested this on v2.13.0 and the behavior is the same.
> 
> FWIW the terminology "empty commit" is a pretty poor naming choice. This
> is totally not your fault at all. I just wish we had a much more intuitive
> name to describe a commit that does not introduce any changes to the tree.
> 
> And yes, doing this with --root is a bit of a hack. That's because --root
> is a bit of a hack.
> 
> I am curious, though, as to the exact differences you experienced. I mean,
> it could be buggy behavior that needs to be fixed (independently of this
> patch series, of course).
> 

Sorry, reading this I realized that I didn't give any details!
When using --root, --keep-empty has no effect. The empty commits
do not appear in the todo list and they are removed.
Also, when using --root without --keep-empty, the empty commits
do not show up as comments in the list.

> Ciao,
> Johannes
> 

Liam 

Re: [PATCH v4 00/10] The final building block for a faster rebase -i

[Johannes Schindelin]

Hi Ævar,

On Mon, 29 May 2017, Ævar Arnfjörð Bjarmason wrote:

> On Mon, May 29, 2017 at 12:51 PM, Johannes Schindelin
>  wrote:
> >
> > On Sat, 27 May 2017, René Scharfe wrote:
> >> Am 26.05.2017 um 05:15 schrieb Liam Beguin:
> >> > I tried to time the execution on an interactive rebase (on Linux)
> >> > but I did not notice a significant change in speed.  Do we have a
> >> > way to measure performance / speed changes between version?
> >>
> >> Well, there's performance test script p3404-rebase-interactive.sh.
> >> You could run it e.g. like this:
> >>
> >>   $ (cd t/perf && ./run origin/master HEAD ./p3404*.sh)
> >>
> >> This would compare the performance of master with the current branch
> >> you're on.  The results of p3404 are quite noisy for me on master,
> >> though (saw 15% difference between runs without any code changes), so
> >> take them with a bag of salt.
> >
> > Indeed. Our performance tests are simply not very meaningful.
> >
> > Part of it is the use of shell scripting (which defeats performance
> > testing pretty well),
> 
> Don't the performance tests take long enough that the shellscripting
> overhead gets lost in the noise?

Okay, here you go, my weekly (or so) clarification about the POSIX
emulation layer called MSYS2 (which itself kind of a portable Cygwin).

Whenever Git for Windows has to execute Unix shell scripts (which are not
native to Windows, as the "Unix" in "Unix shell scripts" so clearly
suggests), we resort to calling the Bash from the MSYS2 project, which
spins up a POSIX emulation layer. Git for Windows' own .exe files
(and in particular, git.exe) is *not* affected by the POSIX emulation
layer, as they are real Win32 programs.

Whenever execution has to bridge into, or out of, the POSIX emulation
layer, a few things need to be done. To emulate signal handling, for
example, a completely new process has to be spun up that itself has the
non-MSYS2 process as a child. The environment has to be converted, to
reflect the fact that some things are Unix-y paths (or path lists) inside
the POSIX emulation layer and Windows paths outside.

Even when staying within the POSIX emulation layer, some operations are
not as cheap as "Linux folks" are used to. For example, to spawn a
subshell, due to the absence of a spawn syscall fork() is called, followed
by exec(). However, fork() itself is not native to Windows and has to be
emulated. The POSIX emulation layer spins up a new process, meticulously
copies the entire memory, tries to reopen the file descriptors, network
connections, etc (to emulate the fork() semantics).

Obviously, this is anything but cheap.

And this is only a glimpse into the entire problem, as I am not aware of
any thorough analysis what is going on in msys-2.0.dll when shell scripts
run. All I know is that it slows things down dramatically.

As a consequence, even the simple act of creating a repository, or
spawning Win32 processes from within a shell, become quite the
contributing factors to the noise of the measurements.

> E.g. on Windows what do you get when you run this in t/perf:
> 
> $ GIT_PERF_REPEAT_COUNT=3 GIT_PERF_MAKE_OPTS="-j6 NO_OPENSSL=Y
> BLK_SHA1=Y CFLAGS=-O3" ./run v2.10.0 v2.12.0 v2.13.0 p3400-rebase.sh

In my hands, a repeat count of 3 always resulted in quite a bit of noise
previously.

Mind you, I am working my machine. It has to run two VMs in the
background, has multiple browsers and dozens of tabs open, checks for
mails and Tweets and RSS feeds and a couple of Skypes are open, too.

So yeah, obviously there is a bit of noise involved.

> I get split-index performance improving by 28% in 2.12 and 58% in
> 2.13, small error bars even with just 3 runs. This is on Linux, but my
> sense of fork overhead on Windows is that it isn't so bad as to matter
> here.

Test
--
3400.2: rebase on top of a lot of unrelated changes

v2.10.0v2.12.0  v2.13.0
--
60.65(0.01+0.03)   55.75(0.01+0.07) -8.1%   55.97(0.04+0.09) -7.7%

(wrapped myself, as the ./run output is a lot wider than the 80 columns
allowed in plain text email format)

And what does it tell you?

Not much, right? You have no idea about the trend line of the three tests,
not even of the standard deviation (not that it would be meaningful for
N=3). It is not immediately obvious whether the first run is always a tad
slower (or faster), or whether there is no noticable difference between
the first and any subsequent runs.

In other words, there is no measure of confidence in those results. We
can't say how reliable those numbers are.

And we certainly can't know how much the shell scripting hurts.

Although... let's try something. Let's run the same command in a *Linux
VM* on the same machine! Yes, that should give us an idea. So here goes:

Test
--
3400.2: rebase on top 

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jeff King]

[+cc Siddharth, so quoting copiously]

On Tue, May 30, 2017 at 11:27:56AM -0400, Jeff King wrote:

> > Travis seems to be seeing the same failure.  Curiously, the topic by
> > itself passes for me; iow, pu fails, pu^2 doesn't fail.
> > 
> > git.git/pu$ ./git rev-list -h
> > BUG: environment.c:173: setup_git_env called without repository
> > Aborted (core dumped)
> > 
> > Hmph...
> 
> Ah, OK, I can reproduce when merged with pu. Bisecting it was tricky.
> To see the problem, you need both my new test _and_ b1ef400ee
> (setup_git_env: avoid blind fall-back to ".git", 2016-10-20). The latter
> is only in v2.13, so topics forked from v2.12 need that commit applied.
> 
> Anyway, the problem is sk/dash-is-previous, specifically fc5684b47
> (revision.c: args starting with "-" might be a revision, 2017-02-25). It
> looks like the revision parser used to just bail on "-h", because
> revision.c would say "I don't recognize this" and then cmd_rev_list()
> would similarly say "I don't recognize this" and call usage(). But now
> we actually try to read it as a ref, which obviously requires being
> inside a repository.
> 
> Normally that's OK, but because of the "-h doesn't set up the repo"
> thing from 99caeed05, we may not have setup the repo, and so looking up
> refs is forbidden. The fix is probably to have revision.c explicitly
> recognize "-h" and bail on it as an unknown option (it can't handle
> the flag itself because only the caller knows the full usage()).
> 
> I do wonder, though, if there's any other downside to trying to look up
> other options as revisions (at least it wastes time doing nonsense
> revision lookups on options known only to cmd_rev_list()).  I'm not sure
> why that commit passes everything starting with a dash as a possible
> revision, rather than just "-".
> 
> I.e.:
> 
> diff --git a/revision.c b/revision.c
> index 5470c33ac..1e26c3951 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2233,7 +2233,14 @@ int setup_revisions(int argc, const char **argv, 
> struct rev_info *revs, struct s
>   }
>   if (opts < 0)
>   exit(128);
> - maybe_opt = 1;
> + if (arg[1]) {
> + /* arg is an unknown option */
> + argv[left++] = arg;
> + continue;
> + } else {
> + /* special token "-" */
> + maybe_opt = 1;
> + }
>   }
>  
>  
> 
> I don't see anything in the commit message, but I didn't dig in the
> mailing list.

I think this line of reasoning comes from

  
http://public-inbox.org/git/20170206181026.GA4010@ubuntu-512mb-blr1-01.localdomain/

And the idea is that ranges like "-.." should work. TBH, I'm not sure
how I feel about that, for exactly the reason that came up here: it
makes it hard to syntactically differentiate the "-" shorthand from
actual options. We do have @{-1} already for this purpose. I don't mind
"-" as a shortcut for things like "git checkout -" or "git show -", but
it feels like most of the benefit is lost when you're combining it with
other operators.

-Peff

Re: 2.13.0-rc1 is broken on SPARC

[Liam R. Howlett]

* Junio C Hamano  [170529 00:11]:
> "Liam R. Howlett"  writes:
> 
> > My SPARC build does not function and seg bus terminates on any command.
> 
> Sorry, known issue in the released version 2.13 (we would have
> appreciated if a bug report for -rc1 came way before the decision to
> tag the final release).
> 
> A fix exists on 'pu' (you can merge or cherry-pick a0103914c2); it
> was unfortunately held up with an unrelated enthusiasm around an
> attempt to use submodule to manage this codebase in our project.
> 
> I'll try to untangle the fix proper and quickly merge down to the
> maintenance track.
> 
> Thanks for a report.


Thank you for your prompt response.  I appreciate your enthusiasm around
submodule code as I've used them extensively in the past.  I'm happy to
hear you have a fix planned for the stable release branch.

Cheers,
Liam

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jeff King]

On Tue, May 30, 2017 at 10:23:54PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Nope, I have those patches directly on your e83352ef23, and it passes. I
> > wonder if there's something funny between our environments. What does
> > the failure look like for you?
> 
> Travis seems to be seeing the same failure.  Curiously, the topic by
> itself passes for me; iow, pu fails, pu^2 doesn't fail.
> 
> git.git/pu$ ./git rev-list -h
> BUG: environment.c:173: setup_git_env called without repository
> Aborted (core dumped)
> 
> Hmph...

Ah, OK, I can reproduce when merged with pu. Bisecting it was tricky.
To see the problem, you need both my new test _and_ b1ef400ee
(setup_git_env: avoid blind fall-back to ".git", 2016-10-20). The latter
is only in v2.13, so topics forked from v2.12 need that commit applied.

Anyway, the problem is sk/dash-is-previous, specifically fc5684b47
(revision.c: args starting with "-" might be a revision, 2017-02-25). It
looks like the revision parser used to just bail on "-h", because
revision.c would say "I don't recognize this" and then cmd_rev_list()
would similarly say "I don't recognize this" and call usage(). But now
we actually try to read it as a ref, which obviously requires being
inside a repository.

Normally that's OK, but because of the "-h doesn't set up the repo"
thing from 99caeed05, we may not have setup the repo, and so looking up
refs is forbidden. The fix is probably to have revision.c explicitly
recognize "-h" and bail on it as an unknown option (it can't handle
the flag itself because only the caller knows the full usage()).

I do wonder, though, if there's any other downside to trying to look up
other options as revisions (at least it wastes time doing nonsense
revision lookups on options known only to cmd_rev_list()).  I'm not sure
why that commit passes everything starting with a dash as a possible
revision, rather than just "-".

I.e.:

diff --git a/revision.c b/revision.c
index 5470c33ac..1e26c3951 100644
--- a/revision.c
+++ b/revision.c
@@ -2233,7 +2233,14 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
}
if (opts < 0)
exit(128);
-   maybe_opt = 1;
+   if (arg[1]) {
+   /* arg is an unknown option */
+   argv[left++] = arg;
+   continue;
+   } else {
+   /* special token "-" */
+   maybe_opt = 1;
+   }
}
 
 

I don't see anything in the commit message, but I didn't dig in the
mailing list. 


-Peff

revision API design, was Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

Hi Junio,

On Tue, 30 May 2017, Junio C Hamano wrote:

> We do not want these [revision API] implementation details to code that
> does not implement command line parsing.  This one is not parsing
> anybody's set of options and should not be mucking with the low level
> implementation details.

Just to make sure we are on the same level: you want the argc & argv to be
the free-form API of setup_revisions(), rather than code setting fields
in struct rev_info whose names can be verified at compile time, and whose
names also suggest their intended use.

I am still flabberghasted that any seasoned software developer sincerely
would suggest this.

> See 66b2ed09 ("Fix "log" family not to be too agressive about
> showing notes", 2010-01-20) and poinder.  Back then, nobody outside
> builtin/log.c and revision.c (these are the two primary things that
> implement command line parsing; "log.c" needs access to the low
> level details because it wants to establish custom default that is
> applied before it reads options given by the end-user) mucked
> directly with verbose_header, which allowed the addition of
> "pretty_given" to be limited only to these places that actually do
> the parsing.  If the above patch to sequencer.c existed before
> 66b2ed09, it would have required unnecessary change to tweak
> "pretty_given" in there too when 66b2ed09 was done.  That is forcing
> a totally unnecesary work.  And there is no reason to expect that
> the kind of change 66b2ed09 made to the general command line parsing
> will not happen in the future.  Adding more code like the above that
> knows the implementation detail is unwarranted, when you can just
> ask the existing command line parser to set them for you.

This is a very eloquent description of a problem with the API.

The correct suggestion would be to fix the API, of course. Not to declare
an interface intended for command-line parsing the internal API.

Maybe the introduction of `pretty_given` was a strong hint at a design
flaw to begin with, pointing to the fact that user_format is a singleton
(yes, really, you can't have two different user_formats in the same Git
process, what were we thinking)?

Ciao,
Dscho

Re: FORMAT_PATCH_NAME_MAX increase

[Laszlo Ersek]

On 05/30/17 16:35, Ævar Arnfjörð Bjarmason wrote:
> On Tue, May 30, 2017 at 3:37 PM, Junio C Hamano  wrote:
>> Ævar Arnfjörð Bjarmason  writes:
>>
>>> Just curious do you know about https://github.com/trast/tbdiff ? If
>>> not it might have a high overlap with what you're doing.

Thank you for the suggestion, it does exactly what I need. It solves the
Assignment Problem a whole lot better than my current, crude script.

https://en.wikipedia.org/wiki/Assignment_problem
https://en.wikipedia.org/wiki/Hungarian_algorithm

(Mildly amusing, to me anyway: I happen to be Hungarian.)

The output of git-tbdiff is not exactly the way I like it (I like to
review interdiffs colorized, and fed to $PAGER individually), but I can
easily do that on top of the "--no-patches" output.

>> Yes, that is a very good suggestion.  You'd need to be able to
>> actually apply the patches but the way I often do a review is very
>> similar to (actually, I'd say it is identical workflow) Laszlo's,
>> and it goes like this:
>>
>> $ git checkout topic ;# previous round
>> $ git checkout master... ;# check out the fork point of previous one
>> $ git am mbox ;# apply the updated one
>> $ git tbdiff ..@{-1} @{-1}..
> 
> I wonder if this tool seemingly everyone on-list uses should just be
> integrated into git.git.
> 
> I only learned about it <2 weeks ago and it's been great. The diff
> output was a bit nonsensical in some cases because it uses python's
> diff engine instead of git's.

Yes, I'd probably plug in git's diff engine (the patience algorithm at
that).

> 
> Would you mind patches to just integrate it to git in python as-is,
> then we could slowly convert it to C as we're doing with everything
> else.

That would be wonderful. My preference would be to use core git features
for solving the problem; as a second preference, it would be great if
git-tbdiff were packaged & shipped by GNU/Linux distros.

Thomas (I see you are on To:/Cc: now -- I meant to CC you, snarfing your
email from the git.git history :) ), are you aware of any distros
already packaging git-tbdiff?

Still the best would be if git provided this feature out of the box;
incremental review of rerolled series appears a common activity for any
serious reviewer / subsystem co-maintainer.

Thanks!
Laszlo

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

Hi Junio,

On Tue, 30 May 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > You still ask me to pass options in plain text that has to be parsed at
> > run-time, rather than compile-time-verifiable flags.
> 
> Absolutely.  

In other words, you want me to put my name to sloppy code.

Sorry, not interested,
Dscho

Re: FORMAT_PATCH_NAME_MAX increase

[Ævar Arnfjörð Bjarmason]

On Tue, May 30, 2017 at 3:37 PM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason  writes:
>
>> Just curious do you know about https://github.com/trast/tbdiff ? If
>> not it might have a high overlap with what you're doing.
>
> Yes, that is a very good suggestion.  You'd need to be able to
> actually apply the patches but the way I often do a review is very
> similar to (actually, I'd say it is identical workflow) Laszlo's,
> and it goes like this:
>
> $ git checkout topic ;# previous round
> $ git checkout master... ;# check out the fork point of previous one
> $ git am mbox ;# apply the updated one
> $ git tbdiff ..@{-1} @{-1}..

I wonder if this tool seemingly everyone on-list uses should just be
integrated into git.git.

I only learned about it <2 weeks ago and it's been great. The diff
output was a bit nonsensical in some cases because it uses python's
diff engine instead of git's.

Would you mind patches to just integrate it to git in python as-is,
then we could slowly convert it to C as we're doing with everything
else.

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Ramsay Jones]

On 30/05/17 01:03, Junio C Hamano wrote:
> Ramsay Jones  writes:
> 
>> See commit c7018be509 ("test: allow skipping the remainder", 18-05-2017)
>> which is currently merged to the 'next' branch (merge 03b8a61e47 of the
>> 'jc/skip-test-in-the-middle' branch).
>>
>> (see also http://testanything.org)
>>
>> If you look at http://testanything.org//tap-specification.html, it shows
>> that you are allowed to annotate a plan of '1..0' with a SKIP directive
>> to explain why no tests in a file were run. However, a plan with '1..n'
>> (for any n > 0) must not have any annotation. (Back in 2012, when I wrote
>> commit bf4b721932, I found much better documentation than the above!)
>>
>> So, after commit c7018be509, you can now use the 'skip_all' facility
>> after having run some tests; it now converts that into an 'SKIP comment'
>> just before the test plan, effectively skipping the remainder of the
>> tests in the file. (since we are using a 'trailing plan', and have thus
>> not declared how many tests we had intended to run, we can output an
>> accurate plan).
> 
> Yes, but I consider that c7018be509 is an ugly workaround, not a
> part of a properly designed framework.  Unless it is absolutely
> necessary to run some tests before we may conditionally want to say
> "skip_all/test_done", we should strive to add tests _after_ these
> conditional skil_all/test_done is done.

yes, I don't disagree with that (which is why I said that I would
have split t5545 into two files). ;-)

> In this case, I do not see there is a strong reason why the new test
> must come before the "setup" test.  Sure, it does not use UNCPATH so
> the new test may be able to run even when the current path cannot be
> spelled as UNC, but that is a natural fallout of (ab)using the test
> script that was meant for UNC testing for something else, so I think
> a proper way out would be either (1) to use a separate script, if
> the new test wants to run whether UNC path can be determined,

Yes, I had intended to suggest this (or an existing script, protected
by the MINGW prerequisite), but forgot!

>  or (2)
> just accept the fact that the new test will only be run when UNC
> paths are tested.

I prefer (1).

>   Relying on the hack c7018be509 did is much less
> appealing.

ATB,
Ramsay Jones

Re: FORMAT_PATCH_NAME_MAX increase

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> Just curious do you know about https://github.com/trast/tbdiff ? If
> not it might have a high overlap with what you're doing.

Yes, that is a very good suggestion.  You'd need to be able to
actually apply the patches but the way I often do a review is very
similar to (actually, I'd say it is identical workflow) Laszlo's,
and it goes like this:

$ git checkout topic ;# previous round
$ git checkout master... ;# check out the fork point of previous one
$ git am mbox ;# apply the updated one
$ git tbdiff ..@{-1} @{-1}..

With the second step, the commit immediately before the previous
round of patches were applied to is checked out as a detached HEAD,
and then with the third step, the updated patches are applied.
After these two steps, the history leading to HEAD is the latest
patches, and the history leading to topic (which can be referred to
as @{-1}, i.e. the branch we were previously on) is the previous
round.

"git tbdiff" takes two ranges and compares these two series of
commits.  The first one says "commits included in the branch we are
previously on (i.e. topic), excluding the ones on the current HEAD",
which means "the patches from the previous round", and the second
one says "commits included in the current HEAD, excluding the ones
on the previous branch (i.e. topic)", which means "the patches from
this latest round".

After that, I may conclude

$ git checkout -B @{-1}

to update the tip of 'topic' with the latest set of patches (the
reason why I type @{-1} is because that can stay constant in the
workflow, no matter what the actual topic branch is called).

Re: FORMAT_PATCH_NAME_MAX increase

[Junio C Hamano]

Laszlo Ersek  writes:

> The problem is that I can't really automate the subject munging. The
> concrete subjects in this case were:
>
>> OvmfPkg/QemuFwCfgLib: Implement SEV internal function for SEC phase
>> OvmfPkg/QemuFwCfgLib: Implement SEV internal functions for PEI phase
>> OvmfPkg/QemuFwCfgLib: Implement SEV internal function for Dxe phase
> ...
> So, even in kernel land, if subjects up to 75 columns are permitted, but
> FORMAT_PATCH_NAME_MAX is 64, conflicts are possible, at least in theory,
> aren't they? With the numbers stripped, of course.

Yup, configurable lengthening or unconditional lengthening to 75 or
so do not sound _too_ bad.

If I sounded like I was opposed to lengthening, that wasn't what I
meant.  It was more like "if you can meaningfully abbreviate, you
may help not just format-patch filenames but other use cases, and
you might even be able to get away without lengthening"; if there
is no meaningful way to abbreviate, raising the max length may be
the only workable solution.

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Junio C Hamano]

Jeff King  writes:

> Nope, I have those patches directly on your e83352ef23, and it passes. I
> wonder if there's something funny between our environments. What does
> the failure look like for you?

Travis seems to be seeing the same failure.  Curiously, the topic by
itself passes for me; iow, pu fails, pu^2 doesn't fail.

git.git/pu$ ./git rev-list -h
BUG: environment.c:173: setup_git_env called without repository
Aborted (core dumped)

Hmph...

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Christian Couder]

On Thu, May 25, 2017 at 8:36 PM, Ben Peart  wrote:

[...]

> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
> new file mode 100755
> index 00..395db46d55
> --- /dev/null
> +++ b/t/t7519-status-fsmonitor.sh
> @@ -0,0 +1,158 @@
> +#!/bin/sh
> +
> +test_description='git status with file system watcher'
> +
> +. ./test-lib.sh
> +
> +clean_repo () {
> +   git reset --hard HEAD
> +   git clean -fd
> +   rm -f marker
> +}

Maybe link all the commands in the function with "&&".

> +dirty_repo () {
> +   : >untracked
> +   : >dir1/untracked
> +   : >dir2/untracked
> +   echo 1 >modified
> +   echo 2 >dir1/modified
> +   echo 3 >dir2/modified
> +   echo 4 >new
> +   echo 5 >dir1/new
> +   echo 6 >dir2/new
> +   git add new
> +   git add dir1/new
> +   git add dir2/new
> +}

Idem.

> +# The test query-fsmonitor hook proc will output a marker file we can use to
> +# ensure the hook was actually used to generate the correct results.
> +
> +test_expect_success 'setup' '
> +   mkdir -p .git/hooks &&
> +   write_script .git/hooks/query-fsmonitor<<-\EOF &&
> +   if [ $1 -ne 1 ]
> +   then
> +   echo -e "Unsupported query-fsmonitor hook version.\n" >&2
> +   exit 1;
> +   fi
> +   : >marker
> +   printf "untracked\0"
> +   printf "dir1/untracked\0"
> +   printf "dir2/untracked\0"
> +   printf "modified\0"
> +   printf "dir1/modified\0"
> +   printf "dir2/modified\0"
> +   printf "new\0""
> +   printf "dir1/new\0"
> +   printf "dir2/new\0"

Maybe something like the following to save a few lines and remove some
redundancies:

   printf "%s\0" untracked dir1/untracked dir2/untracked \
 modified dir1/modified dir2/modified \
 new dir1/new dir2/new

or perhaps:

   for f in untracked modified new
   do
  printf "%s\0" "$f" "dir1/$f" "dir2/$f"
   done

> +   EOF
> +   : >tracked &&
> +   : >modified &&
> +   mkdir dir1 &&
> +   : >dir1/tracked &&
> +   : >dir1/modified &&
> +   mkdir dir2 &&
> +   : >dir2/tracked &&
> +   : >dir2/modified &&
> +   git add . &&
> +   test_tick &&
> +   git commit -m initial &&
> +   dirty_repo
> +'
> +
> +cat >.gitignore <<\EOF
> +.gitignore
> +expect*
> +output*
> +marker*
> +EOF

This could be part of the previous setup test.

> +# Status is well tested elsewhere so we'll just ensure that the results are
> +# the same when using core.fsmonitor. First call after turning on the option
> +# does a complete scan so need to do two calls to ensure we test the new
> +# codepath.
> +
> +test_expect_success 'status with core.untrackedcache true' '

If this test is using untracked cache, it should perhaps first check
that untracked cache can be used on the current file system.
t7063-status-untracked-cache.sh does that with the following:

test_lazy_prereq UNTRACKED_CACHE '
{ git update-index --test-untracked-cache; ret=$?; } &&
test $ret -ne 1
'

if ! test_have_prereq UNTRACKED_CACHE; then
skip_all='This system does not support untracked cache'
test_done
fi

> +   git config core.fsmonitor true  &&
> +   git config core.untrackedcache true &&
> +   git -c core.fsmonitor=false -c core.untrackedcache=true status 
> >expect &&

I don't understand why there is " -c core.untrackedcache=true" in the
above command as you already set core.untrackedcache to true on the
previous line.

> +   clean_repo &&
> +   git status &&
> +   test_path_is_missing marker &&
> +   dirty_repo &&
> +   git status >output &&
> +   test_path_is_file marker &&
> +   test_i18ncmp expect output
> +'
> +
> +

Spurious new line.

> +test_expect_success 'status with core.untrackedcache false' '
> +   git config core.fsmonitor true &&
> +   git config core.untrackedcache false &&
> +   git -c core.fsmonitor=false -c core.untrackedcache=false status 
> >expect &&

Again core.untrackedcache is already set to false on the previous line.

> +   clean_repo &&
> +   git status &&
> +   test_path_is_missing marker &&
> +   dirty_repo &&
> +   git status >output &&
> +   test_path_is_file marker &&
> +   test_i18ncmp expect output
> +'
> +
> +# Ensure commands that call refresh_index() to move the index back in time
> +# properly invalidate the fsmonitor cache
> +
> +test_expect_success 'refresh_index() invalidates fsmonitor cache' '
> +   git config core.fsmonitor true &&
> +   git config core.untrackedcache true &&
> +   clean_repo &&
> +   git status &&
> +   test_path_is_missing marker &&
> +   dirty_repo &&
> +   write_script .git/hooks/query-fsmonitor<<-\EOF &&
> +   :>marker
> +   EOF
> +   git add . &&
> +   git commit -m "to reset" &&
> +   git status &&
> +   

Re: FORMAT_PATCH_NAME_MAX increase

[Ævar Arnfjörð Bjarmason]

On Tue, May 30, 2017 at 2:33 PM, Laszlo Ersek  wrote:
> (apologies for the self-followup:)
>
> On 05/30/17 14:28, Laszlo Ersek wrote:
>
>> Note that in such an incremental review, I specifically wish to compare
>> patches against each other (i.e., I'd like to see diffs of diffs, AKA
>> interdiffs), and not the source tree at two, v1<->v2, commits into the
>> two series. The latter would only give me the difference between the v1
>> and v2 patch application results, and while that is valuable as well,
>> I'd really like to diff the actual patches.
>
> ... One (but not the only) reason for that is that I'd like to see the
> v1->v2 commit message improvements as well. I quite frequently suggest
> commit message improvements in review.

This all makes sense, I think a way to make this length configurable
is the easiest/best solution to this.

Just curious do you know about https://github.com/trast/tbdiff ? If
not it might have a high overlap with what you're doing.

Re: FORMAT_PATCH_NAME_MAX increase

[Laszlo Ersek]

(apologies for the self-followup:)

On 05/30/17 14:28, Laszlo Ersek wrote:

> Note that in such an incremental review, I specifically wish to compare
> patches against each other (i.e., I'd like to see diffs of diffs, AKA
> interdiffs), and not the source tree at two, v1<->v2, commits into the
> two series. The latter would only give me the difference between the v1
> and v2 patch application results, and while that is valuable as well,
> I'd really like to diff the actual patches.

... One (but not the only) reason for that is that I'd like to see the
v1->v2 commit message improvements as well. I quite frequently suggest
commit message improvements in review.

Thanks
Laszlo

Re: FORMAT_PATCH_NAME_MAX increase

[Laszlo Ersek]

On 05/30/17 13:36, Ævar Arnfjörð Bjarmason wrote:
> On Mon, May 29, 2017 at 10:49 AM, Laszlo Ersek  wrote:
>> Hi,
>>
>> would it be possible to
>>
>> - increase the FORMAT_PATCH_NAME_MAX macro from 64 to, say, 128?
>>
>> - Or else to introduce a new git-config knob for it?
>>
>> I have a small review-helper / interdiff script that matches patches
>> from adjacent versions of a series against each other, based on subject
>> line. (Using the formatted file name with the patch number stripped.)
>> The project in question uses long common prefixes in subjects, and the
>> current limit of 64 does not always ensure unicity (again, with the
>> number stripped).
> 
> I don't see why this shouldn't be made configurable, but more
> generally if you have a script like this it seems like a futile effort
> in general to just make the subject longer to solve the general case,
> consider:
> 
> (cd /tmp/; rm -rf test; git init test; cd test && for i in {1..3};
> do touch $i && git add $i && git commit -m"test"; done && git
> format-patch -2 && git reset --hard HEAD~2 && git am *patch)
> 
> Which now yields:
> 
> 0001-test.patch
> 0002-test.patch
> 
> Git projects in general will have plenty of patches like this, e.g.
> "fix build" or "update tests" or whatever. Isn't a better solution for
> your script to e.g. key on git-patch-id?
> 
> $ grep "^From " *patch
> 0001-test.patch:From 870e37afa1a5aeb7eef76e607345adcfd4a9022d Mon
> Sep 17 00:00:00 2001
> 0002-test.patch:From de8c37a1532a4f6ae71ffa65400479ba77438f3b Mon
> Sep 17 00:00:00 2001
> $ cat *patch | git patch-id
> c71eb8f2c8c461ba6040668e9d79f996f5a04a61
> 870e37afa1a5aeb7eef76e607345adcfd4a9022d
> 735aff6fb601d7ce99506dc7701be3e8a9b5d38c
> de8c37a1532a4f6ae71ffa65400479ba77438f3b
> 
> Other potential heuristics could be keying not just on the subject but
> on the subject + subject of the last N commits for each commit, which
> should give more of a unique key, or key on the whole commit message
> etc.
> 

My use case is the following:

- Contributor posts version 1 of his/her patch series to the mailing
list, and also pushes the series to his/her publicly accessible git repo
on some branch. I review it and suggest a number of improvements.

- Contributor posts version 2 of his/her patch series to the mailing
list, and pushes the series to another branch in his/her repo too. I
want to review the v2 series incrementally; that is, I'd like to diff
each individual v2 patch against its v1 counterpart.

While patches may be dropped, added, replaced between patch set
versions, the general case is that most patches remain in the series,
and they preserve their original subjects. Thus, finding the v1
counterpart for a v2 patch, based on the subject, as formatted by
git-format-patch into filenames, works reliably most of the time.

Note that in such an incremental review, I specifically wish to compare
patches against each other (i.e., I'd like to see diffs of diffs, AKA
interdiffs), and not the source tree at two, v1<->v2, commits into the
two series. The latter would only give me the difference between the v1
and v2 patch application results, and while that is valuable as well,
I'd really like to diff the actual patches.

Of course I could technically parse the Subject: header of the formatted
patch files, and rename the files for interdiffing using full-length
filenames. But then I'd have to care about filesystem safety and such,
and that's already handled perfectly by git-format-patch itself. The
only bit I'm missing is the arbitrary length in the formatted file
names. (I had patched git earlier to use 128 for FORMAT_PATCH_NAME_MAX,
but I'd like to stop carrying that change.)

"git-patch-id" doesn't look applicable here, as I'd like to assign
patches (across v1 and v2) to each other based on "purpose". I.e., the
assignment should occur regardless of even functional changes between
corresponding v1 and v2 patches. I need the coupling exactly so I can
review those differences.

I use the "join" utility on the number-stripped patch filenames to find
the pairs of patches between v1 and v2 that should be compared.

Thanks!
Laszlo