Re: --no-commit option does not work.

[Junio C Hamano]

BongHo Lee  writes:

> If I add --no-ff option, it works properly.
> I think --no-commit option should be worked without --no-ff.

It is understandable that this is confusing, but --no-commit is an
instruction not to create a new commit object.  As fast-forwarding
to the commit that is a strict descendant of your old tip does not
involve creation of any new commit, the command is working exactly
as instructed.  If you say "--no-ff", you are explicitly forbidding
the command to fast-forward, so the command attempts to create a
(needless) new commit that is a merge, and then --no-commit stops
the command after it prepared the tree state ready to be committed.

So with or without --no-ff, the option and the command are working
correctly.

Having said all that, my gut feeling is that a backward incompatible
change to make --no-commit "imply" --no-ff may not hurt too many
existing users, but I am saying this without thinking things through.
I may very well be missing a valid use case where --no-commit that
does not fail but does fast-forward when the user does not give --no-ff
is useful, so if that is the case, such a change will be breaking
those users.

Re: [PATCH v2 3/3] rev-parse: match ^{} case-insensitively

[Junio C Hamano]

Jeff King  writes:

> FWIW, I cannot see us ever adding TREE (or Tree) as a separate type.
> It's too confusing for no gain. We'd call it "tree2" or something more
> obvious.

In case it was not clear, I didn't mean to say I _want_ to leave
that door open.  Well, I cannot imagine it was unclear, as I said I
do not at all mind declaring that all object names will be lowercase
to allow us freely downcase what we got at the UI level.

> So I don't mind closing that door, but I'm not sure if a partial
> conversion (where some commands are case-insensitive but others aren't
> yet) might not leave us in a more confusing place.

Exactly.

> I dunno. I guess I have never wanted to type "^{Tree}" in the first
> place, so I do not personally see the _benefit_. Which makes it easy to
> see even small negatives as a net loss.

As to the potential _benefit_, I do not see much either myself, but
we already are seeing somebody cared enough to throw us a patch, so
to some people there are clearly perceived benefit.  I do not think
closing the door for typenames that are not lowercase is a negative
change at all.

I just wanted the patch to make it clear that it is making such a
system-wide design decision and casting it in stone.  Which includes
that "cat-file " and "hash-object -t " get the same
case-insensitivity update and probably writing that design decision
down somewhere in the documentation, perhaps in the glossary where
we talk about the "object type".

Re: [PATCH] pretty: add extra headers and MIME boundary directly

[Jeff King]

On Sun, Mar 26, 2017 at 03:41:14PM +0200, René Scharfe wrote:

> Am 25.03.2017 um 22:11 schrieb Jeff King:
> > The most correct way is that the caller of log_write_email_headers() and
> > diff_flush() should have a function-local strbuf which holds the data,
> > gets passed to diff_flush() as some kind opaque context, and then is
> > freed afterwards. We don't have such a context, but if we were to abuse
> > diff_options.stat_sep _temporarily_, that would still be a lot cleaner.
> > I.e., something like this:
> > 
> >   struct strbuf stat_sep = STRBUF_INIT;
> > 
> >   /* may write into stat_sep, depending on options */
> >   log_write_email_headers(..., _sep);
> >   opt->diffopt.stat_sep = stat_sep.buf;
> > 
> >   diff_flush(>diffopt);
> >   opt->diffopt.stat_sep = NULL;
> >   strbuf_release(_sep);
> > 
> > But it's a bit tricky because those two hunks happen in separate
> > functions, which means passing the strbuf around.
> 
> You could have a destructor callback, called at the end of diff_flush().

Yeah, though now we have lifetime rules around stat_sep. How long does
it stay good? Which functions decide it's now done? Are we sure they
alweays get called? We're just tacking that onto the end of diff_flush()
because the caller responsibilities are so unclear, but that's making an
assumption that it always gets called.

This case might be simple enough that it's true, but it feels like a
leaky module boundary.

> Hmm.  I'm a fan of callbacks, but using them can make the code a bit
> hard to follow.  And void context pointers add a type safety hazard.
> Do we need to be this generic?  How about switching stat_sep to strbuf?
> fmt_output_commit() requires an allocation anyway, so why not allocate
> stat_sep as well?

Right, I think that is the simplest thing, but it falls afoul of the
lifetime rules above.

If the rule is "the stat_sep gets printed once and then freed", that's
pretty reasonable. But I wonder if there are cases where we might not
print stat_sep (or call diff_flush at all for that matter). I'm not sure
if that's possible with --attach, though, which constrains us to
format-patch.

>   if (opt->mime_boundary) {
> - static char buffer[1024];
>   struct strbuf filename =  STRBUF_INIT;

Actually, I guess the absolute simplest thing would be swap this out for
a static strbuf, and leave the ownership with the function. That's ugly,
but it's how it works now, and lets us drop the fixed buffer.

Another option is to put it in rev_info, and make the rev_info owner
free it (which we know is restricted to cmd_format_patch(), as it is the
only one who uses mime_boundary).

-Peff

Re: [PATCH v2 3/3] rev-parse: match ^{} case-insensitively

[Jeff King]

On Sun, Mar 26, 2017 at 05:36:21PM -0700, Junio C Hamano wrote:

> It's not "potential confusion".  This closes the door for us to
> introduce "TREE" as a separate object type in the future.
> 
> If we agree to make a declaration that all typenames are officially
> spelled in lowercase [*1*] and at the UI level we accept typenames
> spelled in any case, then we can adopt this change (and then we need
> to update "cat-file -t" etc. to match it).
> 
> I do not at all mind to see if the list concensus is to make such a
> declaration and permanently close the door for typenames that are
> not lowercase, and after seeing such a concensus I'd gladly
> appreciate this patch, but I do not want to see a change like this
> that decides the future of the system, pretending as an innocuous
> change, sneaked in without making sure that everybody is aware of
> its implications.

FWIW, I cannot see us ever adding TREE (or Tree) as a separate type.
It's too confusing for no gain. We'd call it "tree2" or something more
obvious.

So I don't mind closing that door, but I'm not sure if a partial
conversion (where some commands are case-insensitive but others aren't
yet) might not leave us in a more confusing place.

I dunno. I guess I have never wanted to type "^{Tree}" in the first
place, so I do not personally see the _benefit_. Which makes it easy to
see even small negatives as a net loss.

-Peff

Re: [PATCH v2 2/3] rev-parse: add @{p} as a synonym for @{push}

[Jeff King]

On Sun, Mar 26, 2017 at 12:16:53PM +, Ævar Arnfjörð Bjarmason wrote:

> Add @{p} as a shorthand for @{push} for consistency with the @{u}
> shorthand for @{upstream}.
> 
> This wasn't added when @{push} was introduced in commit
> adfe5d0434 ("sha1_name: implement @{push} shorthand", 2015-05-21), but
> it can be added without any ambiguity and saves the user some typing.

It _can_ be added, but it was intentionally avoided at the time because
there was discussion of adding other p-words, including:

  - @{pull} as a synonym for @{upstream} (and to better match @{push})

  - @{publish}, which was some similar-ish system that was based on
per-branch config, but the patches were never merged.

It's been a few years with neither of those things happening, so in a
sense it may be safe to add it now. OTOH, if users are not clamoring for
@{p} and it is just being added "because we can", maybe that is not a
good reason.

> -'@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
> -  The suffix '@\{push}' reports the branch "where we would push to" if
> +'@\{push\}', e.g. 'master@\{push\}', '@\{p\}'::
> +  The suffix '@\{push}' (short form '@\{push}') reports the branch "where we 
> would push to" if

Did you mean to say "short form '@\{p}'"?

-Peff

Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag

[Jeff King]

On Sun, Mar 26, 2017 at 04:16:06PM -0700, Junio C Hamano wrote:

> > I don't think this case really matters for collision detection. What's
> > important is what Git does when it receives a brand-new packfile that
> > would overwrite an existing one. It _should_ keep the old one, under the
> > usual "existing data wins" rule.
> 
> If a malicious site can craft two packfiles that hash to the same,
> then it can first feed one against a fetch request, then feed the
> other one when a later fetch request comes, and then the later pack
> is discarded by the "existing data wins" rule.  Even though this
> later pack may have all the objects necessary to complete the refs
> this later fetch request asked for, and an earlier pack that
> happened to have the same pack trailer hash doesn't have these
> necessary objects, the receiver ends up discarding this necessary
> pack.  The end result is that the receiver's repository is now
> corrupt, lacking some objects.

No, I don't think so. We don't trust the trailer hash for anything to do
with corruption; we actually inflate the objects and see which ones we
got. So the victim will notice immediately that what the attacker sent
it is insufficient to complete the fetch (or push), and will refuse to
update the refs. The fetch transfer, but nobody gets corrupted.

Or another way to think about it: we don't care what the trailer hash
is. It comes from the untrusted attacker in the first place. So this
attack is no different than the other side just sending a bogus pack
that omits an object (but has a valid checksum).  The fact that it
happens to use the same on-disk name as something we already have is
irrelevant. We'll still notice that we don't have everything we need to
update the refs.

So the best you could do is probably a mini-DoS. Something like:

  0. Attacker generates two colliding packs, A and B. Only pack B has
 object X.

  1. Somehow, the attacker convinces upstream to take history with X
 _and_ to repack such that serving a fetch will yield pack B.

  2. You fetch from the attacker, who feeds you A.

  3. Now you fetch from upstream, who tries to send you B. You refuse
 it, thinking you already have it (but you really have A). The fetch
 fails because you are missing X.

That situation persists until either you repack (at which point you no
longer have A, unless your repack happens to generate the exact same
pack again!), or upstream history moves (or is repacked), causing them
to send a different pack.

I think arranging for (1) is exceedingly unlikely, and the fact that
your investment in (0) is lost the moment either the victim or the
upstream changes a single bit makes it an unlikely attack.

> I highlighted this case as notable because the way the trailing hash
> is also used as the name of packfile makes this fall into the same
> category as object hash, in that the hash is used for identification
> and deduplication (because we have a rule that says there can be
> only one _thing_ that hashes to one value), for which we do want to
> use the collision-attack detecting kind of hashing, even though it
> otherwise should fall into the other category (i.e. use of csum-file
> API to compute trailer hash), where the hash is used merely for
> bit-flip detection (we are perfectly OK if you have multiple index
> files with different contents that happen to have the same hash in
> the trailer, because the hash is not used for identificaiton and
> deduplication).

If we really wanted to address this (and I remain unconvinced that it's
worth caring about), the simplest thing is not to do collision
detection, but simply to give the packs a less predictable name. There
is nothing at all that cares about the filenames beyond the syntactic
"pack-[0-9a-f]{40}.pack", and it does not need to match the trailer
checksum (and as you know, we switched it recently without ill effect).

So we could literally just switch to writing out pack-$x.pack, where $x
is something like the 160-bit truncated sha-256, and the readers would
be none the wiser.

-Peff

Re: [PATCH v2 2/3] sequencer: make commit options more extensible

[Junio C Hamano]

Johannes Schindelin  writes:

>> Making "flags" unsigned was a correct change, but this is now wrong,
>> as "allow" is made unsigned by accident.
> ...
>
> Your patch looks good, you could do even better by reverting that move
> (IIRC it was at the end of the line, and it was set to 0 by default).

I do not think the variable needs to be initialized to anything (it
is not looked at until it gets the result from allow_empty() call).

Anyway, the series is not yet in 'next', so you can replace it to
the shape you would have made it into if you noticed that "allow"
shouldn't be unsigned, while updating the log message to explain
what the change is about (instead of only attempting to justify the
past, which is not interesting in the context of understanding what
this change is about).

As this thing is about fixing a regression visible to end users, I
may get around to fixing things up myself, but I have other topics
to attend to, so...

Re: [PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

[Junio C Hamano]

René Scharfe  writes:

> FreeBSD implements getcwd(3) as a syscall, but falls back to a version
> based on readdir(3) if it fails for some reason.  The latter requires
> permissions to read and execute path components, while the former does
> not.  That means that if our buffer is too small and we're missing
> rights we could get EACCES, but we may succeed with a bigger buffer.

WOW.  Just WOW.  Looking at the debugging exchange from the
sideline, I didn't expect this end result.

> Keep retrying if getcwd(3) indicates lack of permissions until our
> buffer can fit PATH_MAX bytes, as that's the maximum supported by the
> syscall on FreeBSD anyway.  This way we do what we can to be able to
> benefit from the syscall, but we also won't loop forever if there is a
> real permission issue.
>
> This fixes a regression introduced with 7333ed17 (setup: convert
> setup_git_directory_gently_1 et al. to strbuf, 2014-07-28) for paths
> longer than 127 bytes with components that miss read or execute
> permissions (e.g. 0711 on /home for privacy reasons); we used a fixed
> PATH_MAX-sized buffer before.
>
> Reported-by: Zenobiusz Kunegunda 
> Signed-off-by: Rene Scharfe 
> ---


Nicely analysed and fixed (or is the right word "worked around"?)

Thanks, will queue.

Re: [PATCH v2 3/3] rev-parse: match ^{} case-insensitively

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Change the revision parsing logic to match ^{commit}, ^{tree}, ^{blob}
> etc. case-insensitively.
>
> Before this change supplying anything except the lower-case forms
> emits an "unknown revision or path not in the working tree"
> error. This change makes upper-case & mixed-case versions equivalent
> to the lower-case versions.
>
> The rationale for this change is the same as for making @{upstream}
> and related suffixes case-insensitive in "rev-parse: match
> @{upstream}, @{u} and @{push} case-insensitively", but unlike those
> suffixes this change introduces the potential confusion of accepting
> TREE or BLOB here, but not as an argument to e.g. "cat-file -t "
> or "hash-object -t ".

It's not "potential confusion".  This closes the door for us to
introduce "TREE" as a separate object type in the future.

If we agree to make a declaration that all typenames are officially
spelled in lowercase [*1*] and at the UI level we accept typenames
spelled in any case, then we can adopt this change (and then we need
to update "cat-file -t" etc. to match it).

I do not at all mind to see if the list concensus is to make such a
declaration and permanently close the door for typenames that are
not lowercase, and after seeing such a concensus I'd gladly
appreciate this patch, but I do not want to see a change like this
that decides the future of the system, pretending as an innocuous
change, sneaked in without making sure that everybody is aware of
its implications.


[Footnote]

*1* "officially spelled in lowercase" is necessary to avoid
confusion, because we are not making typenames case insensitive,
allowing an object whose raw-bytes in its loose object
representation before deflating begins with "COMMIT" and take it
as an object of  type.  Instead, such a declaration will
make such an object an invalid one (as opposed to "object of an
unknown type", as it currently is).

Re: [PATCH v2 1/3] rev-parse: match @{upstream}, @{u} and @{push} case-insensitively

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> The ^{} suffix could be changed to be case-insensitive as well
> without introducing any ambiguities. That was included in an earlier
> version of this patch, but Junio objected to its inclusion in [2].

We try not to be personal in our log message.  It's not like my
objection weighs heavier than objections from others.  The same
number of bytes in the log message can better to spent to allow
readers of "git log" independently to judge the rationle without
referring to external material.

Perhaps replace this entire paragraph, including the URL in [2],
with something like

The ^{type} suffix is not made case-insensitive, because other
places that take  like "cat-file -t " do want them
case sensitively (after all we never declared that type names
are case insensitive). Allowing case-insensitive typename only
with this syntax will make the resulting Git as a whole
inconsistent.

Other than that, the change to the code and the new tests all makes
sense to me.

Thanks.

--no-commit option does not work.

[BongHo Lee]

Setup
Version : git version 2.12.1.windows.1

OS Version : Microsoft Windows [Version 10.0.14393]

Options 
$ cat /etc/install-options.txt
Path Option: Cmd
SSH Option: OpenSSH
CURL Option: OpenSSL
CRLF Option: CRLFCommitAsIs
Bash Terminal Option: MinTTY
Performance Tweaks FSCache: Enabled
Use Credential Manager: Enabled
Enable Symlinks: Disabled


Git Command : git merge --no-commit origin/workingBranch

. What did you expect to occur after running these commands?
merge but no commit.
. What actually happened instead?
commited

Reference URL : https://github.com/techcap/nf-interpreter

Details
I tried to merge from MergeTest to master.
After checking out master, I run below command.

C:\work\nf-interpreter-techcap>git branch
  MergeTest
  TemplateModification
  UpdateBuildInstruction
* master

C:\work\nf-interpreter-techcap>git merge --no-commit origin/MergeTest
Updating 197ad33..028f9f5
Fast-forward
mergeTest.txt | 1 +
1 file changed, 1 insertion(+)
create mode 100644 mergeTest.txt

C:\work\nf-interpreter-techcap>git status
On branch master
Your branch is ahead of 'origin/master' by 1 commit.
  (use "git push" to publish your local commits)
nothing to commit, working tree clean

If I add --no-ff option, it works properly.
I think --no-commit option should be worked without --no-ff.

https://github.com/git-for-windows/git/issues/1102#issuecomment-289206961

Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag

[Junio C Hamano]

Jeff King  writes:

> On Fri, Mar 24, 2017 at 11:37:54PM -0700, Junio C Hamano wrote:
>
>> The hash that names a packfile is constructed by sorting all the
>> names of the objects contained in the packfile and running SHA-1
>> hash over it.  I think this MUST be hashed with collision-attack
>> detection.  A malicious site can feed you a packfile that contains
>> objects the site crafts so that the sorted object names would result
>> in a collision-attack, ending up with one pack that contains a sets
>> of objects different from another pack that happens to have the same
>> packname, causing Git to say "Ah, this new pack must have the same
>> set of objects as the pack we already have" and discard it,
>> resulting in lost objects and a corrupt repository with missing
>> objects.
>
> I don't think this case really matters for collision detection. What's
> important is what Git does when it receives a brand-new packfile that
> would overwrite an existing one. It _should_ keep the old one, under the
> usual "existing data wins" rule.

If a malicious site can craft two packfiles that hash to the same,
then it can first feed one against a fetch request, then feed the
other one when a later fetch request comes, and then the later pack
is discarded by the "existing data wins" rule.  Even though this
later pack may have all the objects necessary to complete the refs
this later fetch request asked for, and an earlier pack that
happened to have the same pack trailer hash doesn't have these
necessary objects, the receiver ends up discarding this necessary
pack.  The end result is that the receiver's repository is now
corrupt, lacking some objects.

It is a different matter if such an "attack" is a useful one to
conduct from attacker's point of view. 

I highlighted this case as notable because the way the trailing hash
is also used as the name of packfile makes this fall into the same
category as object hash, in that the hash is used for identification
and deduplication (because we have a rule that says there can be
only one _thing_ that hashes to one value), for which we do want to
use the collision-attack detecting kind of hashing, even though it
otherwise should fall into the other category (i.e. use of csum-file
API to compute trailer hash), where the hash is used merely for
bit-flip detection (we are perfectly OK if you have multiple index
files with different contents that happen to have the same hash in
the trailer, because the hash is not used for identificaiton and
deduplication).

Re: [PATCH] git-p4: Add failing test case for name-rev vs symbolic-ref

[Junio C Hamano]

Luke Diamand  writes:

> As per the discussion about use of "git name-rev" vs "git symbolic-ref" in
> git-p4 here:
>
> http://marc.info/?l=git=148979063421355
>
> git-p4 could get confused about the branch it is on and affects
> the git-p4.allowSubmit  option. This adds a failing
> test case for the problem.
>
> Luke Diamand (1):
>   git-p4: add failing test for name-rev rather than symbolic-ref
>
>  t/t9807-git-p4-submit.sh | 16 
>  1 file changed, 16 insertions(+)

Ahh, thanks for tying loose ends.  I completely forgot about that
topic.

If we queue this and then update the function in git-p4.py that
(mis)uses name-rev to learn the current branch to use symbolic-ref
instead, can we flip the "expect_failure" to "expect_success"?

IOW, can we have a follow up to this patch that fixes a known
breakage the patch documents ;-)?

Thanks.

Re: What's cooking in git.git (Mar 2017, #10; Fri, 24)

[Junio C Hamano]

Brandon Williams  writes:

> On 03/24, Junio C Hamano wrote:
>> * bw/recurse-submodules-relative-fix (2017-03-17) 5 commits
>>  - ls-files: fix bug when recursing with relative pathspec
>>  - ls-files: fix typo in variable name
>>  - grep: fix bug when recursing with relative pathspec
>>  - setup: allow for prefix to be passed to git commands
>>  - grep: fix help text typo
>> 
>>  A few commands that recently learned the "--recurse-submodule"
>>  option misbehaved when started from a subdirectory of the
>>  superproject.
>
> Anything more you think needs to be done about this?  I noticed that
> Dscho's config series hit master so I could rebase against that (as
> there is a small conflict).  Aside from that it didn't seem like there
> were many complaints with the proposed fix.

I saw no particular issues myself.  Do others find this series good
(not just "meh--it is harmless" but I want to hear a positive "yes
these are good changes")?

Re: [PATCH v3 2/2] l10n: Add git-add.txt to localized man pages

[Junio C Hamano]

Jean-Noël AVILA  writes:

> ... So I would 
> think the other way around: for those interested in translated the 
> documentation, some script would allow to checkout the git project inside the 
> gitman-l10n project (like a kind of library).
>
> This would be mainly transparent for the git developers.

As long as the resulting layout would help all groups (1) developers
who do not worry about documentation l10n (2) documentation i18n
coordinator and transltors (3) those who build and ship binary
packages, I personally am OK either way.

Having said that, I am not sure if I understand your "translators do
not have a fixed version of git.git to work with and po4a cannot
work well" as a real concern.  Wouldn't the l10n of documentation
use a similar workflow as used for the translation of in-code
strings we do in po/?  Namely, *.pot files are *NOT* updated by
individual translators by picking up a random version of git.git and
running xgettext.  Instead, i18n coordinator is the only person who
runs xgettext to update *.pot for the upcoming release of Git being
prepared, and then translators work off of that *.pot file.  Which
means they do not have to worry about in-code strings that gets
updated in the meantime; instead they work on a stable known
snapshot of *.pot and wait for the next sync with i18n coordinator
whose running of xgettext would update *.pot files with updated
in-code strings.  Doesn't that workflow apply equally well for the
documentation l10n?

Re: [PATCH v3 1/2] [GSoC] dir_iterator: iterate over dir after its contents

[Michael Haggerty]

On 03/25/2017 07:12 PM, Daniel Ferreira wrote:
> Create an option for the dir_iterator API to iterate over a directory
> path only after having iterated through its contents. This feature was
> predicted, although not implemented by 0fe5043 ("dir_iterator: new API
> for iterating over a directory tree", 2016-06-18).
> 
> This is useful for recursively removing a directory and calling rmdir()
> on a directory only after all of its contents have been wiped.
> 
> An "options" member has been added to the dir_iterator struct. It
> contains the "iterate_dirs_after_files" flag, that enables the feature
> when set to 1. Default behavior continues to be iterating over directory
> paths before its contents.
> 
> Two inline functions have been added to dir_iterator's code to avoid
> code repetition inside dir_iterator_advance() and make code more clear.
> 
> No particular functions or wrappers for setting the options struct's
> fields have been added to avoid either breaking the current dir_iterator
> API or over-engineering an extremely simple option architecture.
> 
> Signed-off-by: Daniel Ferreira 
> ---
>  dir-iterator.c | 100 
> -
>  dir-iterator.h |   7 
>  2 files changed, 84 insertions(+), 23 deletions(-)
> 
> [...]
> diff --git a/dir-iterator.h b/dir-iterator.h
> index 27739e6..4304913 100644
> --- a/dir-iterator.h
> +++ b/dir-iterator.h
> @@ -38,7 +38,14 @@
>   * dir_iterator_advance() again.
>   */
> 
> +struct dir_iterator_options {
> + unsigned iterate_dirs_after_files : 1;
> +};
> +
>  struct dir_iterator {
> + /* Options for dir_iterator */
> + struct dir_iterator_options options;
> +
>   /* The current path: */
>   struct strbuf path;

Another thing I noticed: the name of this option,
`iterate_dirs_after_files`, is a little bit misleading. If I understand
correctly, it doesn't make the iteration process files before
directories within a single directory; rather, it ensures that
subdirectories and their contents are processed before the containing
directory. Therefore, a better name might be something like "depth_first".

I should mention that I like the overall idea to add this new feature
and use it to simplify `remove_subtree()`.

Michael

Re: [PATCH v3 1/2] [GSoC] dir_iterator: iterate over dir after its contents

[Michael Haggerty]

On 03/25/2017 07:12 PM, Daniel Ferreira wrote:
> Create an option for the dir_iterator API to iterate over a directory
> path only after having iterated through its contents. This feature was
> predicted, although not implemented by 0fe5043 ("dir_iterator: new API
> for iterating over a directory tree", 2016-06-18).
> 
> This is useful for recursively removing a directory and calling rmdir()
> on a directory only after all of its contents have been wiped.
> 
> An "options" member has been added to the dir_iterator struct. It
> contains the "iterate_dirs_after_files" flag, that enables the feature
> when set to 1. Default behavior continues to be iterating over directory
> paths before its contents.
> 
> Two inline functions have been added to dir_iterator's code to avoid
> code repetition inside dir_iterator_advance() and make code more clear.
> 
> No particular functions or wrappers for setting the options struct's
> fields have been added to avoid either breaking the current dir_iterator
> API or over-engineering an extremely simple option architecture.

This patch would be easier to read if it were split into two: one
extracting the new functions and changing old code to use them, and a
second adding the new functionality. As one patch, is is hard to see
quickly which changes have what purpose.

I also suggest adding a new `unsigned int flags` parameter to
`dir_iterator_begin`. I think that's more natural, because it doesn't
make sense to change the iteration order during an iteration. It's not
much of a problem to change the API given that all callers are in the
same codebase. If you were to forget to convert any callers (or if a
different in-flight patch series were to add a new caller using the old
call style), the compiler would complain, and the problem would be
obvious and easy to fix.

I didn't actually read the patch carefully yet because I don't have time
this evening to seek out the interesting parts in the long diff.

Michael

> [...]

Re: [Resurrection Remix] #substratum # nice? Come on 100 +1 xx

[نورالدين بالحاج]

مساء الخير

بتاريخ 25‏/3‏/2017، كتب Google+ (‪Hugo R3s3nd3 BeZaInA‬‏)‏
:
> نشر ‪Hugo R3s3nd3 BeZaInA‬‏ مشاركة مع Resurrection Remix. عرض:
> https://plus.google.com/_/notifications/emlink?emr=15213587310654228599=COiM8rO_8tICFcagHgodYCgCMA=%2F102357686384107376925%2Fposts%2FTfsFVEa6BpS=1490473751046=SQUARE_SUBSCRIPTION
>
> يتم استلام هذه الرسالة الإلكترونية لأنك مشترك في Resurrection Remix على
> ‪Google+‬‏. Resurrection Remix
> https://plus.google.com/_/notifications/emlink?emr=15213587310654228599=COiM8rO_8tICFcagHgodYCgCMA=%2Fcommunities%2F109352646351468373340=1490473751046=SQUARE_SUBSCRIPTION
>
>  تغيير الإشعارات التي تصلك من هذا المنتدى:
> https://plus.google.com/_/notifications/emlink?emr=15213587310654228599=COiM8rO_8tICFcagHgodYCgCMA=%2Fcommunities%2F109352646351468373340%3Fpromo%3Dnotifications=1490473751046=SQUARE_SUBSCRIPTION
>
> تم إرسال هذا الإشعار إلى 7890nd...@gmail.com. يمكنك الانتقال إلى إعدادات
> إشعار التسليم لتحديث عنوانك:
> https://plus.google.com/_/notifications/emlink?emr=15213587310654228599=COiM8rO_8tICFcagHgodYCgCMA=%2Fsettings%2Fplus=1490473751046=SQUARE_SUBSCRIPTION
>
> إلغاء الاشتراك من هذه الرسائل الإلكترونية:
> https://notifications.google.com/unsubscribe/AJ7SsMmYslyYROjkqOAI-hxwJIBV8RSFJCcFyAaJh6ZGa-U_SwevmrYPV8yZK70pGt2Puq8OZTmFqJrEX9IFs6uA9WBB4yRp9WYPZP0E-pUx6QP3tvdh2Fii1_8kLFkg0G3NWl-fJehNJ6vHRRpuqp-SHXtD-Yx4Qv-JLyCOb17H4crxDHAQDG4
>
>

[PATCH v2 11/21] sha1_name: convert struct disambiguate_state to object_id

[brian m. carlson]

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

@@
struct disambiguate_state E1;
@@
- E1.bin_pfx
+ E1.bin_pfx.hash

@@
struct disambiguate_state *E1;
@@
- E1->bin_pfx
+ E1->bin_pfx.hash

@@
struct disambiguate_state E1;
@@
- E1.candidate
+ E1.candidate.hash

@@
struct disambiguate_state *E1;
@@
- E1->candidate
+ E1->candidate.hash

This conversion is needed so we can convert disambiguate_hint_fn later.

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

diff --git a/sha1_name.c b/sha1_name.c
index 3db166b40b..cf6f4be0c6 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -16,11 +16,11 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, 
void *);
 struct disambiguate_state {
int len; /* length of prefix in hex chars */
char hex_pfx[GIT_MAX_HEXSZ + 1];
-   unsigned char bin_pfx[GIT_MAX_RAWSZ];
+   struct object_id bin_pfx;
 
disambiguate_hint_fn fn;
void *cb_data;
-   unsigned char candidate[GIT_MAX_RAWSZ];
+   struct object_id candidate;
unsigned candidate_exists:1;
unsigned candidate_checked:1;
unsigned candidate_ok:1;
@@ -37,10 +37,10 @@ static void update_candidates(struct disambiguate_state 
*ds, const unsigned char
}
if (!ds->candidate_exists) {
/* this is the first candidate */
-   hashcpy(ds->candidate, current);
+   hashcpy(ds->candidate.hash, current);
ds->candidate_exists = 1;
return;
-   } else if (!hashcmp(ds->candidate, current)) {
+   } else if (!hashcmp(ds->candidate.hash, current)) {
/* the same as what we already have seen */
return;
}
@@ -52,14 +52,14 @@ static void update_candidates(struct disambiguate_state 
*ds, const unsigned char
}
 
if (!ds->candidate_checked) {
-   ds->candidate_ok = ds->fn(ds->candidate, ds->cb_data);
+   ds->candidate_ok = ds->fn(ds->candidate.hash, ds->cb_data);
ds->disambiguate_fn_used = 1;
ds->candidate_checked = 1;
}
 
if (!ds->candidate_ok) {
/* discard the candidate; we know it does not satisfy fn */
-   hashcpy(ds->candidate, current);
+   hashcpy(ds->candidate.hash, current);
ds->candidate_checked = 0;
return;
}
@@ -151,7 +151,7 @@ static void unique_in_pack(struct packed_git *p,
int cmp;
 
current = nth_packed_object_sha1(p, mid);
-   cmp = hashcmp(ds->bin_pfx, current);
+   cmp = hashcmp(ds->bin_pfx.hash, current);
if (!cmp) {
first = mid;
break;
@@ -170,7 +170,7 @@ static void unique_in_pack(struct packed_git *p,
 */
for (i = first; i < num && !ds->ambiguous; i++) {
current = nth_packed_object_sha1(p, i);
-   if (!match_sha(ds->len, ds->bin_pfx, current))
+   if (!match_sha(ds->len, ds->bin_pfx.hash, current))
break;
update_candidates(ds, current);
}
@@ -213,12 +213,12 @@ static int finish_object_disambiguation(struct 
disambiguate_state *ds,
 * same repository!
 */
ds->candidate_ok = (!ds->disambiguate_fn_used ||
-   ds->fn(ds->candidate, ds->cb_data));
+   ds->fn(ds->candidate.hash, ds->cb_data));
 
if (!ds->candidate_ok)
return SHORT_NAME_AMBIGUOUS;
 
-   hashcpy(sha1, ds->candidate);
+   hashcpy(sha1, ds->candidate.hash);
return 0;
 }
 
@@ -332,7 +332,7 @@ static int init_object_disambiguation(const char *name, int 
len,
ds->hex_pfx[i] = c;
if (!(i & 1))
val <<= 4;
-   ds->bin_pfx[i >> 1] |= val;
+   ds->bin_pfx.hash[i >> 1] |= val;
}
 
ds->len = len;

[PATCH v2 03/21] Convert GIT_SHA1_RAWSZ used for allocation to GIT_MAX_RAWSZ

[brian m. carlson]

Since we will likely be introducing a new hash function at some point,
and that hash function might be longer than 20 bytes, use the constant
GIT_MAX_RAWSZ, which is designed to be suitable for allocations, instead
of GIT_SHA1_RAWSZ.  This will ease the transition down the line by
distinguishing between places where we need to allocate memory suitable
for the largest hash from those where we need to handle the current
hash.

Signed-off-by: brian m. carlson 
---
 builtin/patch-id.c | 2 +-
 builtin/receive-pack.c | 2 +-
 cache.h| 2 +-
 patch-ids.c| 2 +-
 patch-ids.h| 2 +-
 sha1_file.c| 4 ++--
 sha1_name.c| 4 ++--
 wt-status.h| 2 +-
 8 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index a84d0003a3..81552e02e4 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -55,7 +55,7 @@ static int scan_hunk_header(const char *p, int *p_before, int 
*p_after)
 
 static void flush_one_hunk(struct object_id *result, git_SHA_CTX *ctx)
 {
-   unsigned char hash[GIT_SHA1_RAWSZ];
+   unsigned char hash[GIT_MAX_RAWSZ];
unsigned short carry = 0;
int i;
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index fb2a090a0c..feafb076a4 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1162,7 +1162,7 @@ static void check_aliased_update(struct command *cmd, 
struct string_list *list)
const char *dst_name;
struct string_list_item *item;
struct command *dst_cmd;
-   unsigned char sha1[GIT_SHA1_RAWSZ];
+   unsigned char sha1[GIT_MAX_RAWSZ];
int flag;
 
strbuf_addf(, "%s%s", get_git_namespace(), cmd->ref_name);
diff --git a/cache.h b/cache.h
index af4b2c78cf..179bdc5da2 100644
--- a/cache.h
+++ b/cache.h
@@ -968,7 +968,7 @@ extern char *sha1_pack_index_name(const unsigned char 
*sha1);
 extern const char *find_unique_abbrev(const unsigned char *sha1, int len);
 extern int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len);
 
-extern const unsigned char null_sha1[GIT_SHA1_RAWSZ];
+extern const unsigned char null_sha1[GIT_MAX_RAWSZ];
 extern const struct object_id null_oid;
 
 static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
diff --git a/patch-ids.c b/patch-ids.c
index ce285c2e0c..fa8f11de82 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -71,7 +71,7 @@ static int init_patch_id_entry(struct patch_id *patch,
   struct commit *commit,
   struct patch_ids *ids)
 {
-   unsigned char header_only_patch_id[GIT_SHA1_RAWSZ];
+   unsigned char header_only_patch_id[GIT_MAX_RAWSZ];
 
patch->commit = commit;
if (commit_patch_id(commit, >diffopts, header_only_patch_id, 1))
diff --git a/patch-ids.h b/patch-ids.h
index 0f34ea11ea..b9e5751f8e 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_SHA1_RAWSZ];
+   unsigned char patch_id[GIT_MAX_RAWSZ];
struct commit *commit;
 };
 
diff --git a/sha1_file.c b/sha1_file.c
index e763000d34..b4666ee5c2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1606,7 +1606,7 @@ static void mark_bad_packed_object(struct packed_git *p,
if (!hashcmp(sha1, p->bad_object_sha1 + GIT_SHA1_RAWSZ * i))
return;
p->bad_object_sha1 = xrealloc(p->bad_object_sha1,
- st_mult(GIT_SHA1_RAWSZ,
+ st_mult(GIT_MAX_RAWSZ,
  st_add(p->num_bad_objects, 1)));
hashcpy(p->bad_object_sha1 + GIT_SHA1_RAWSZ * p->num_bad_objects, sha1);
p->num_bad_objects++;
@@ -3913,7 +3913,7 @@ static int check_stream_sha1(git_zstream *stream,
 const unsigned char *expected_sha1)
 {
git_SHA_CTX c;
-   unsigned char real_sha1[GIT_SHA1_RAWSZ];
+   unsigned char real_sha1[GIT_MAX_RAWSZ];
unsigned char buf[4096];
unsigned long total_read;
int status = Z_OK;
diff --git a/sha1_name.c b/sha1_name.c
index 964201bc26..3db166b40b 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -16,11 +16,11 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, 
void *);
 struct disambiguate_state {
int len; /* length of prefix in hex chars */
char hex_pfx[GIT_MAX_HEXSZ + 1];
-   unsigned char bin_pfx[GIT_SHA1_RAWSZ];
+   unsigned char bin_pfx[GIT_MAX_RAWSZ];
 
disambiguate_hint_fn fn;
void *cb_data;
-   unsigned char candidate[GIT_SHA1_RAWSZ];
+   unsigned char candidate[GIT_MAX_RAWSZ];
unsigned candidate_exists:1;
unsigned candidate_checked:1;
unsigned candidate_ok:1;
diff --git a/wt-status.h b/wt-status.h
index 54fec77032..6018c627b1 100644
--- a/wt-status.h
+++ b/wt-status.h

[PATCH v2 13/21] submodule: convert check_for_new_submodule_commits to object_id

[brian m. carlson]

All of the callers of this function have been converted, so convert this
function and update the callers.  This function also calls
sha1_array_append, which we'll convert shortly.

Signed-off-by: brian m. carlson 
---
 builtin/fetch.c | 6 +++---
 submodule.c | 4 ++--
 submodule.h | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b5ad09d046..a41b892dcc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -659,7 +659,7 @@ static int update_local_ref(struct ref *ref,
 
if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
(recurse_submodules != RECURSE_SUBMODULES_ON))
-   check_for_new_submodule_commits(ref->new_oid.hash);
+   check_for_new_submodule_commits(>new_oid);
r = s_update_ref(msg, ref, 0);
format_display(display, r ? '!' : '*', what,
   r ? _("unable to update local ref") : NULL,
@@ -675,7 +675,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, ref->new_oid.hash, 
DEFAULT_ABBREV);
if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
(recurse_submodules != RECURSE_SUBMODULES_ON))
-   check_for_new_submodule_commits(ref->new_oid.hash);
+   check_for_new_submodule_commits(>new_oid);
r = s_update_ref("fast-forward", ref, 1);
format_display(display, r ? '!' : ' ', quickref.buf,
   r ? _("unable to update local ref") : NULL,
@@ -690,7 +690,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, ref->new_oid.hash, 
DEFAULT_ABBREV);
if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
(recurse_submodules != RECURSE_SUBMODULES_ON))
-   check_for_new_submodule_commits(ref->new_oid.hash);
+   check_for_new_submodule_commits(>new_oid);
r = s_update_ref("forced-update", ref, 1);
format_display(display, r ? '!' : '+', quickref.buf,
   r ? _("unable to update local ref") : _("forced 
update"),
diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..5c5c18ec3d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -821,14 +821,14 @@ static int add_sha1_to_array(const char *ref, const 
struct object_id *oid,
return 0;
 }
 
-void check_for_new_submodule_commits(unsigned char new_sha1[20])
+void check_for_new_submodule_commits(struct object_id *oid)
 {
if (!initialized_fetch_ref_tips) {
for_each_ref(add_sha1_to_array, _tips_before_fetch);
initialized_fetch_ref_tips = 1;
}
 
-   sha1_array_append(_tips_after_fetch, new_sha1);
+   sha1_array_append(_tips_after_fetch, oid->hash);
 }
 
 static int add_sha1_to_argv(const unsigned char sha1[20], void *data)
diff --git a/submodule.h b/submodule.h
index c8a0c9cb29..9c32b28b12 100644
--- a/submodule.h
+++ b/submodule.h
@@ -58,7 +58,7 @@ extern void show_submodule_inline_diff(FILE *f, const char 
*path,
const char *del, const char *add, const char *reset,
const struct diff_options *opt);
 extern void set_config_fetch_recurse_submodules(int value);
-extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
+extern void check_for_new_submodule_commits(struct object_id *oid);
 extern int fetch_populated_submodules(const struct argv_array *options,
   const char *prefix, int command_line_option,
   int quiet, int max_parallel_jobs);

[PATCH v2 04/21] builtin/diff: convert to struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson 
---
 builtin/diff.c | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 3d64b85337..398eee00d5 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -21,7 +21,7 @@
 #define DIFF_NO_INDEX_IMPLICIT 2
 
 struct blobinfo {
-   unsigned char sha1[20];
+   struct object_id oid;
const char *name;
unsigned mode;
 };
@@ -31,22 +31,22 @@ static const char builtin_diff_usage[] =
 
 static void stuff_change(struct diff_options *opt,
 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 *old_name,
 const char *new_name)
 {
struct diff_filespec *one, *two;
 
-   if (!is_null_sha1(old_sha1) && !is_null_sha1(new_sha1) &&
-   !hashcmp(old_sha1, new_sha1) && (old_mode == new_mode))
+   if (!is_null_oid(old_oid) && !is_null_oid(new_oid) &&
+   !oidcmp(old_oid, new_oid) && (old_mode == new_mode))
return;
 
if (DIFF_OPT_TST(opt, REVERSE_DIFF)) {
SWAP(old_mode, new_mode);
-   SWAP(old_sha1, new_sha1);
+   SWAP(old_oid, new_oid);
SWAP(old_name, new_name);
}
 
@@ -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_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);
 
diff_queue(_queued_diff, one, two);
 }
@@ -89,7 +89,7 @@ static int builtin_diff_b_f(struct rev_info *revs,
 
stuff_change(>diffopt,
 blob[0].mode, canon_mode(st.st_mode),
-blob[0].sha1, null_sha1,
+[0].oid, _oid,
 1, 0,
 path, path);
diffcore_std(>diffopt);
@@ -114,7 +114,7 @@ static int builtin_diff_blobs(struct rev_info *revs,
 
stuff_change(>diffopt,
 blob[0].mode, blob[1].mode,
-blob[0].sha1, blob[1].sha1,
+[0].oid, [1].oid,
 1, 1,
 blob[0].name, blob[1].name);
diffcore_std(>diffopt);
@@ -160,7 +160,7 @@ static int builtin_diff_tree(struct rev_info *revs,
 struct object_array_entry *ent0,
 struct object_array_entry *ent1)
 {
-   const unsigned char *(sha1[2]);
+   const struct object_id *(oid[2]);
int swap = 0;
 
if (argc > 1)
@@ -172,9 +172,9 @@ static int builtin_diff_tree(struct rev_info *revs,
 */
if (ent1->item->flags & UNINTERESTING)
swap = 1;
-   sha1[swap] = ent0->item->oid.hash;
-   sha1[1 - swap] = ent1->item->oid.hash;
-   diff_tree_sha1(sha1[0], sha1[1], "", >diffopt);
+   oid[swap] = >item->oid;
+   oid[1 - swap] = >item->oid;
+   diff_tree_sha1(oid[0]->hash, oid[1]->hash, "", >diffopt);
log_tree_diff_flush(revs);
return 0;
 }
@@ -408,7 +408,7 @@ int cmd_diff(int argc, const char **argv, const char 
*prefix)
} else if (obj->type == OBJ_BLOB) {
if (2 <= blobs)
die(_("more than two blobs given: '%s'"), name);
-   hashcpy(blob[blobs].sha1, obj->oid.hash);
+   hashcpy(blob[blobs].oid.hash, obj->oid.hash);
blob[blobs].name = name;
blob[blobs].mode = entry->mode;
blobs++;

[PATCH v2 06/21] builtin/receive-pack: fix incorrect pointer arithmetic

[brian m. carlson]

If we had already processed the last newline in a push certificate, we
would end up subtracting NULL from the end-of-certificate pointer when
computing the length of the line.  This would have resulted in an
absurdly large length, and possibly a buffer overflow.  Instead,
subtract the beginning-of-certificate pointer from the
end-of-certificate pointer, which is what's expected.

Note that this situation should never occur, since not only do we
require the certificate to be newline terminated, but the signature will
only be read from the beginning of a line.  Nevertheless, it seems
prudent to correct it.

Signed-off-by: brian m. carlson 
---
 builtin/receive-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index feafb076a4..116f3177a1 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1524,7 +1524,7 @@ static void queue_commands_from_cert(struct command 
**tail,
 
while (boc < eoc) {
const char *eol = memchr(boc, '\n', eoc - boc);
-   tail = queue_command(tail, boc, eol ? eol - boc : eoc - eol);
+   tail = queue_command(tail, boc, eol ? eol - boc : eoc - boc);
boc = eol ? eol + 1 : eoc;
}
 }

[PATCH v2 15/21] sha1-array: convert internal storage for struct sha1_array to object_id

[brian m. carlson]

Make the internal storage for struct sha1_array use an array of struct
object_id internally.  Update the users of this struct which inspect its
internals.

Signed-off-by: brian m. carlson 
---
 bisect.c   | 14 +++---
 builtin/pull.c | 22 +++---
 builtin/receive-pack.c |  4 ++--
 combine-diff.c |  6 +++---
 fetch-pack.c   | 12 ++--
 fsck.c |  4 ++--
 remote-curl.c  |  2 +-
 send-pack.c|  2 +-
 sha1-array.c   | 22 +++---
 sha1-array.h   |  2 +-
 shallow.c  | 26 +-
 11 files changed, 58 insertions(+), 58 deletions(-)

diff --git a/bisect.c b/bisect.c
index 21c3e34636..ebaf7b05ba 100644
--- a/bisect.c
+++ b/bisect.c
@@ -457,7 +457,7 @@ static char *join_sha1_array_hex(struct sha1_array *array, 
char delim)
int i;
 
for (i = 0; i < array->nr; i++) {
-   strbuf_addstr(_hexs, sha1_to_hex(array->sha1[i]));
+   strbuf_addstr(_hexs, oid_to_hex(array->oid + i));
if (i + 1 < array->nr)
strbuf_addch(_hexs, delim);
}
@@ -621,7 +621,7 @@ static void bisect_rev_setup(struct rev_info *revs, const 
char *prefix,
argv_array_pushf(_argv, bad_format, oid_to_hex(current_bad_oid));
for (i = 0; i < good_revs.nr; i++)
argv_array_pushf(_argv, good_format,
-sha1_to_hex(good_revs.sha1[i]));
+oid_to_hex(good_revs.oid + i));
argv_array_push(_argv, "--");
if (read_paths)
read_bisect_paths(_argv);
@@ -701,11 +701,11 @@ static int bisect_checkout(const unsigned char 
*bisect_rev, int no_checkout)
return run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
 }
 
-static struct commit *get_commit_reference(const unsigned char *sha1)
+static struct commit *get_commit_reference(const struct object_id *oid)
 {
-   struct commit *r = lookup_commit_reference(sha1);
+   struct commit *r = lookup_commit_reference(oid->hash);
if (!r)
-   die(_("Not a valid commit name %s"), sha1_to_hex(sha1));
+   die(_("Not a valid commit name %s"), oid_to_hex(oid));
return r;
 }
 
@@ -715,9 +715,9 @@ static struct commit **get_bad_and_good_commits(int *rev_nr)
int i, n = 0;
 
ALLOC_ARRAY(rev, 1 + good_revs.nr);
-   rev[n++] = get_commit_reference(current_bad_oid->hash);
+   rev[n++] = get_commit_reference(current_bad_oid);
for (i = 0; i < good_revs.nr; i++)
-   rev[n++] = get_commit_reference(good_revs.sha1[i]);
+   rev[n++] = get_commit_reference(good_revs.oid + i);
*rev_nr = n;
 
return rev;
diff --git a/builtin/pull.c b/builtin/pull.c
index 704ce1f042..c007900ab5 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -514,7 +514,7 @@ static int run_fetch(const char *repo, const char 
**refspecs)
 /**
  * "Pulls into void" by branching off merge_head.
  */
-static int pull_into_void(const unsigned char *merge_head,
+static int pull_into_void(const struct object_id *merge_head,
const struct object_id *curr_head)
 {
/*
@@ -523,10 +523,10 @@ static int pull_into_void(const unsigned char *merge_head,
 * index/worktree changes that the user already made on the unborn
 * branch.
 */
-   if (checkout_fast_forward(EMPTY_TREE_SHA1_BIN, merge_head, 0))
+   if (checkout_fast_forward(EMPTY_TREE_SHA1_BIN, merge_head->hash, 0))
return 1;
 
-   if (update_ref("initial pull", "HEAD", merge_head, curr_head->hash, 0, 
UPDATE_REFS_DIE_ON_ERR))
+   if (update_ref("initial pull", "HEAD", merge_head->hash, 
curr_head->hash, 0, UPDATE_REFS_DIE_ON_ERR))
return 1;
 
return 0;
@@ -693,13 +693,13 @@ static int get_rebase_fork_point(struct object_id 
*fork_point, const char *repo,
  */
 static int get_octopus_merge_base(struct object_id *merge_base,
const struct object_id *curr_head,
-   const unsigned char *merge_head,
+   const struct object_id *merge_head,
const struct object_id *fork_point)
 {
struct commit_list *revs = NULL, *result;
 
commit_list_insert(lookup_commit_reference(curr_head->hash), );
-   commit_list_insert(lookup_commit_reference(merge_head), );
+   commit_list_insert(lookup_commit_reference(merge_head->hash), );
if (!is_null_oid(fork_point))
commit_list_insert(lookup_commit_reference(fork_point->hash), 
);
 
@@ -718,7 +718,7 @@ static int get_octopus_merge_base(struct object_id 
*merge_base,
  * appropriate arguments and returns its exit status.
  */
 static int run_rebase(const struct object_id *curr_head,
-   const unsigned char *merge_head,
+   const struct object_id *merge_head,
const struct 

[PATCH v2 01/21] Define new hash-size constants for allocating memory

[brian m. carlson]

Since we will want to transition to a new hash at some point in the
future, and that hash may be larger in size than 160 bits, introduce two
constants that can be used for allocating a sufficient amount of memory.
They can be increased to reflect the largest supported hash size.

Signed-off-by: brian m. carlson 
---
 cache.h | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 2214d52f61..af4b2c78cf 100644
--- a/cache.h
+++ b/cache.h
@@ -66,8 +66,12 @@ unsigned long git_deflate_bound(git_zstream *, unsigned 
long);
 #define GIT_SHA1_RAWSZ 20
 #define GIT_SHA1_HEXSZ (2 * GIT_SHA1_RAWSZ)
 
+/* The length in byte and in hex digits of the largest possible hash value. */
+#define GIT_MAX_RAWSZ GIT_SHA1_RAWSZ
+#define GIT_MAX_HEXSZ GIT_SHA1_HEXSZ
+
 struct object_id {
-   unsigned char hash[GIT_SHA1_RAWSZ];
+   unsigned char hash[GIT_MAX_RAWSZ];
 };
 
 #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)

[PATCH v2 02/21] Convert GIT_SHA1_HEXSZ used for allocation to GIT_MAX_HEXSZ

[brian m. carlson]

Since we will likely be introducing a new hash function at some point,
and that hash function might be longer than 40 hex characters, use the
constant GIT_MAX_HEXSZ, which is designed to be suitable for
allocations, instead of GIT_SHA1_HEXSZ.  This will ease the transition
down the line by distinguishing between places where we need to allocate
memory suitable for the largest hash from those where we need to handle
the current hash.

Signed-off-by: brian m. carlson 
---
 bisect.c  | 2 +-
 builtin/blame.c   | 4 ++--
 builtin/merge-index.c | 2 +-
 builtin/merge.c   | 2 +-
 builtin/rev-list.c| 2 +-
 diff.c| 4 ++--
 hex.c | 2 +-
 sha1_file.c   | 2 +-
 sha1_name.c   | 6 +++---
 transport.c   | 2 +-
 10 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/bisect.c b/bisect.c
index 30808cadf7..21c3e34636 100644
--- a/bisect.c
+++ b/bisect.c
@@ -682,7 +682,7 @@ static int is_expected_rev(const struct object_id *oid)
 
 static int bisect_checkout(const unsigned char *bisect_rev, int no_checkout)
 {
-   char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
+   char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
 
memcpy(bisect_rev_hex, sha1_to_hex(bisect_rev), GIT_SHA1_HEXSZ + 1);
update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, 
UPDATE_REFS_DIE_ON_ERR);
diff --git a/builtin/blame.c b/builtin/blame.c
index f7aa95f4ba..07506a3e45 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1890,7 +1890,7 @@ static void emit_porcelain(struct scoreboard *sb, struct 
blame_entry *ent,
int cnt;
const char *cp;
struct origin *suspect = ent->suspect;
-   char hex[GIT_SHA1_HEXSZ + 1];
+   char hex[GIT_MAX_HEXSZ + 1];
 
oid_to_hex_r(hex, >commit->object.oid);
printf("%s %d %d %d\n",
@@ -1928,7 +1928,7 @@ static void emit_other(struct scoreboard *sb, struct 
blame_entry *ent, int opt)
const char *cp;
struct origin *suspect = ent->suspect;
struct commit_info ci;
-   char hex[GIT_SHA1_HEXSZ + 1];
+   char hex[GIT_MAX_HEXSZ + 1];
int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
 
get_commit_info(suspect->commit, , 1);
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index 2d1b6db6bd..c99443b095 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -9,7 +9,7 @@ static int merge_entry(int pos, const char *path)
 {
int found;
const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
-   char hexbuf[4][GIT_SHA1_HEXSZ + 1];
+   char hexbuf[4][GIT_MAX_HEXSZ + 1];
char ownbuf[4][60];
 
if (pos >= active_nr)
diff --git a/builtin/merge.c b/builtin/merge.c
index 7554b8d412..a2cceea3fb 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1296,7 +1296,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
if (verify_signatures) {
for (p = remoteheads; p; p = p->next) {
struct commit *commit = p->item;
-   char hex[GIT_SHA1_HEXSZ + 1];
+   char hex[GIT_MAX_HEXSZ + 1];
struct signature_check signature_check;
memset(_check, 0, sizeof(signature_check));
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0aa93d5891..bcf77f0b8a 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -212,7 +212,7 @@ static void print_var_int(const char *var, int val)
 static int show_bisect_vars(struct rev_list_info *info, int reaches, int all)
 {
int cnt, flags = info->flags;
-   char hex[GIT_SHA1_HEXSZ + 1] = "";
+   char hex[GIT_MAX_HEXSZ + 1] = "";
struct commit_list *tried;
struct rev_info *revs = info->revs;
 
diff --git a/diff.c b/diff.c
index a628ac3a95..330b640c68 100644
--- a/diff.c
+++ b/diff.c
@@ -398,7 +398,7 @@ static struct diff_tempfile {
 */
const char *name;
 
-   char hex[GIT_SHA1_HEXSZ + 1];
+   char hex[GIT_MAX_HEXSZ + 1];
char mode[10];
 
/*
@@ -4219,7 +4219,7 @@ const char *diff_aligned_abbrev(const struct object_id 
*oid, int len)
 * uniqueness across all objects (statistically speaking).
 */
if (abblen < GIT_SHA1_HEXSZ - 3) {
-   static char hex[GIT_SHA1_HEXSZ + 1];
+   static char hex[GIT_MAX_HEXSZ + 1];
if (len < abblen && abblen <= len + 2)
xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, 
len+3-abblen, "..");
else
diff --git a/hex.c b/hex.c
index eab7b626ee..28b44118cb 100644
--- a/hex.c
+++ b/hex.c
@@ -85,7 +85,7 @@ char *oid_to_hex_r(char *buffer, const struct object_id *oid)
 char *sha1_to_hex(const unsigned char *sha1)
 {
static int bufno;
-   static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
+   static char hexbuffer[4][GIT_MAX_HEXSZ + 1];
bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);

[PATCH v2 14/21] builtin/pull: convert to struct object_id

[brian m. carlson]

Convert virtually all uses of unsigned char [20] to struct object_id.
Leave all the arguments that come from struct sha1_array, as these will
be converted in a later patch.

Signed-off-by: brian m. carlson 
---
 builtin/pull.c | 72 +-
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index a9f7553f30..704ce1f042 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -515,7 +515,7 @@ static int run_fetch(const char *repo, const char 
**refspecs)
  * "Pulls into void" by branching off merge_head.
  */
 static int pull_into_void(const unsigned char *merge_head,
-   const unsigned char *curr_head)
+   const struct object_id *curr_head)
 {
/*
 * Two-way merge: we treat the index as based on an empty tree,
@@ -526,7 +526,7 @@ static int pull_into_void(const unsigned char *merge_head,
if (checkout_fast_forward(EMPTY_TREE_SHA1_BIN, merge_head, 0))
return 1;
 
-   if (update_ref("initial pull", "HEAD", merge_head, curr_head, 0, 
UPDATE_REFS_DIE_ON_ERR))
+   if (update_ref("initial pull", "HEAD", merge_head, curr_head->hash, 0, 
UPDATE_REFS_DIE_ON_ERR))
return 1;
 
return 0;
@@ -647,7 +647,7 @@ static const char *get_tracking_branch(const char *remote, 
const char *refspec)
  * current branch forked from its remote tracking branch. Returns 0 on success,
  * -1 on failure.
  */
-static int get_rebase_fork_point(unsigned char *fork_point, const char *repo,
+static int get_rebase_fork_point(struct object_id *fork_point, const char 
*repo,
const char *refspec)
 {
int ret;
@@ -678,7 +678,7 @@ static int get_rebase_fork_point(unsigned char *fork_point, 
const char *repo,
if (ret)
goto cleanup;
 
-   ret = get_sha1_hex(sb.buf, fork_point);
+   ret = get_oid_hex(sb.buf, fork_point);
if (ret)
goto cleanup;
 
@@ -691,24 +691,24 @@ static int get_rebase_fork_point(unsigned char 
*fork_point, const char *repo,
  * Sets merge_base to the octopus merge base of curr_head, merge_head and
  * fork_point. Returns 0 if a merge base is found, 1 otherwise.
  */
-static int get_octopus_merge_base(unsigned char *merge_base,
-   const unsigned char *curr_head,
+static int get_octopus_merge_base(struct object_id *merge_base,
+   const struct object_id *curr_head,
const unsigned char *merge_head,
-   const unsigned char *fork_point)
+   const struct object_id *fork_point)
 {
struct commit_list *revs = NULL, *result;
 
-   commit_list_insert(lookup_commit_reference(curr_head), );
+   commit_list_insert(lookup_commit_reference(curr_head->hash), );
commit_list_insert(lookup_commit_reference(merge_head), );
-   if (!is_null_sha1(fork_point))
-   commit_list_insert(lookup_commit_reference(fork_point), );
+   if (!is_null_oid(fork_point))
+   commit_list_insert(lookup_commit_reference(fork_point->hash), 
);
 
result = reduce_heads(get_octopus_merge_bases(revs));
free_commit_list(revs);
if (!result)
return 1;
 
-   hashcpy(merge_base, result->item->object.oid.hash);
+   oidcpy(merge_base, >item->object.oid);
return 0;
 }
 
@@ -717,16 +717,16 @@ static int get_octopus_merge_base(unsigned char 
*merge_base,
  * fork point calculated by get_rebase_fork_point(), runs git-rebase with the
  * appropriate arguments and returns its exit status.
  */
-static int run_rebase(const unsigned char *curr_head,
+static int run_rebase(const struct object_id *curr_head,
const unsigned char *merge_head,
-   const unsigned char *fork_point)
+   const struct object_id *fork_point)
 {
int ret;
-   unsigned char oct_merge_base[GIT_SHA1_RAWSZ];
+   struct object_id oct_merge_base;
struct argv_array args = ARGV_ARRAY_INIT;
 
-   if (!get_octopus_merge_base(oct_merge_base, curr_head, merge_head, 
fork_point))
-   if (!is_null_sha1(fork_point) && !hashcmp(oct_merge_base, 
fork_point))
+   if (!get_octopus_merge_base(_merge_base, curr_head, merge_head, 
fork_point))
+   if (!is_null_oid(fork_point) && !oidcmp(_merge_base, 
fork_point))
fork_point = NULL;
 
argv_array_push(, "rebase");
@@ -756,8 +756,8 @@ static int run_rebase(const unsigned char *curr_head,
argv_array_push(, "--onto");
argv_array_push(, sha1_to_hex(merge_head));
 
-   if (fork_point && !is_null_sha1(fork_point))
-   argv_array_push(, sha1_to_hex(fork_point));
+   if (fork_point && !is_null_oid(fork_point))
+   argv_array_push(, oid_to_hex(fork_point));
else
argv_array_push(, sha1_to_hex(merge_head));
 
@@ -770,8 +770,8 @@ int cmd_pull(int 

[PATCH v2 20/21] Rename sha1_array to oid_array

[brian m. carlson]

Since this structure handles an array of object IDs, rename it to struct
oid_array.  Also rename the accessor functions and the initialization
constant.

This commit was produced mechanically by providing non-Documentation
files to the following Perl one-liners:

perl -pi -E 's/struct sha1_array/struct oid_array/g'
perl -pi -E 's/\bsha1_array_/oid_array_/g'
perl -pi -E 's/SHA1_ARRAY_INIT/OID_ARRAY_INIT/g'

Signed-off-by: brian m. carlson 
---
 bisect.c   | 16 
 builtin/cat-file.c | 10 +-
 builtin/diff.c |  6 +++---
 builtin/fetch-pack.c   |  2 +-
 builtin/pack-objects.c | 10 +-
 builtin/pull.c |  6 +++---
 builtin/receive-pack.c | 24 +++
 builtin/send-pack.c|  4 ++--
 combine-diff.c | 12 ++--
 commit.h   | 14 +++---
 connect.c  |  8 
 diff.h |  4 ++--
 fetch-pack.c   | 26 -
 fetch-pack.h   |  4 ++--
 fsck.c |  6 +++---
 fsck.h |  2 +-
 parse-options-cb.c |  4 ++--
 ref-filter.c   |  6 +++---
 ref-filter.h   |  2 +-
 remote-curl.c  |  2 +-
 remote.h   |  6 +++---
 send-pack.c|  4 ++--
 send-pack.h|  2 +-
 sha1-array.c   | 14 +++---
 sha1-array.h   | 12 ++--
 sha1_name.c|  8 
 shallow.c  | 12 ++--
 submodule.c| 48 +++---
 submodule.h|  6 +++---
 t/helper/test-sha1-array.c | 10 +-
 transport.c| 20 +--
 31 files changed, 155 insertions(+), 155 deletions(-)

diff --git a/bisect.c b/bisect.c
index f193257509..54d69e77b9 100644
--- a/bisect.c
+++ b/bisect.c
@@ -12,8 +12,8 @@
 #include "sha1-array.h"
 #include "argv-array.h"
 
-static struct sha1_array good_revs;
-static struct sha1_array skipped_revs;
+static struct oid_array good_revs;
+static struct oid_array skipped_revs;
 
 static struct object_id *current_bad_oid;
 
@@ -413,9 +413,9 @@ static int register_ref(const char *refname, const struct 
object_id *oid,
current_bad_oid = xmalloc(sizeof(*current_bad_oid));
oidcpy(current_bad_oid, oid);
} else if (starts_with(refname, good_prefix.buf)) {
-   sha1_array_append(_revs, oid);
+   oid_array_append(_revs, oid);
} else if (starts_with(refname, "skip-")) {
-   sha1_array_append(_revs, oid);
+   oid_array_append(_revs, oid);
}
 
strbuf_release(_prefix);
@@ -451,7 +451,7 @@ static void read_bisect_paths(struct argv_array *array)
fclose(fp);
 }
 
-static char *join_sha1_array_hex(struct sha1_array *array, char delim)
+static char *join_sha1_array_hex(struct oid_array *array, char delim)
 {
struct strbuf joined_hexs = STRBUF_INIT;
int i;
@@ -499,7 +499,7 @@ struct commit_list *filter_skipped(struct commit_list *list,
while (list) {
struct commit_list *next = list->next;
list->next = NULL;
-   if (0 <= sha1_array_lookup(_revs, 
>item->object.oid)) {
+   if (0 <= oid_array_lookup(_revs, 
>item->object.oid)) {
if (skipped_first && !*skipped_first)
*skipped_first = 1;
/* Move current to tried list */
@@ -789,9 +789,9 @@ static void check_merge_bases(int no_checkout)
const struct object_id *mb = >item->object.oid;
if (!oidcmp(mb, current_bad_oid)) {
handle_bad_merge_base();
-   } else if (0 <= sha1_array_lookup(_revs, mb)) {
+   } else if (0 <= oid_array_lookup(_revs, mb)) {
continue;
-   } else if (0 <= sha1_array_lookup(_revs, mb)) {
+   } else if (0 <= oid_array_lookup(_revs, mb)) {
handle_skipped_merge_base(mb);
} else {
printf(_("Bisecting: a merge base must be tested\n"));
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index eb0043231d..1890d7a639 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -413,7 +413,7 @@ static int batch_loose_object(const struct object_id *oid,
  const char *path,
  void *data)
 {
-   sha1_array_append(data, oid);
+   oid_array_append(data, oid);
return 0;
 }
 
@@ -422,7 +422,7 @@ static int batch_packed_object(const struct object_id *oid,
   uint32_t pos,
   void *data)
 {
-   sha1_array_append(data, oid);
+   oid_array_append(data, oid);
return 0;
 

[PATCH v2 10/21] test-sha1-array: convert most code to struct object_id

[brian m. carlson]

This helper is very small, so convert the entire thing.

Signed-off-by: brian m. carlson 
---
 t/helper/test-sha1-array.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/helper/test-sha1-array.c b/t/helper/test-sha1-array.c
index f7a53c4ad6..b4bb97fccc 100644
--- a/t/helper/test-sha1-array.c
+++ b/t/helper/test-sha1-array.c
@@ -14,16 +14,16 @@ int cmd_main(int argc, const char **argv)
 
while (strbuf_getline(, stdin) != EOF) {
const char *arg;
-   unsigned char sha1[20];
+   struct object_id oid;
 
if (skip_prefix(line.buf, "append ", )) {
-   if (get_sha1_hex(arg, sha1))
+   if (get_oid_hex(arg, ))
die("not a hexadecimal SHA1: %s", arg);
-   sha1_array_append(, sha1);
+   sha1_array_append(, oid.hash);
} else if (skip_prefix(line.buf, "lookup ", )) {
-   if (get_sha1_hex(arg, sha1))
+   if (get_oid_hex(arg, ))
die("not a hexadecimal SHA1: %s", arg);
-   printf("%d\n", sha1_array_lookup(, sha1));
+   printf("%d\n", sha1_array_lookup(, oid.hash));
} else if (!strcmp(line.buf, "clear"))
sha1_array_clear();
else if (!strcmp(line.buf, "for_each_unique"))

[PATCH v2 17/21] Convert remaining callers of sha1_array_lookup to object_id

[brian m. carlson]

There are a very small number of callers which don't already use struct
object_id.  Convert them.

Signed-off-by: brian m. carlson 
---
 bisect.c   | 14 +++---
 builtin/pack-objects.c | 16 
 ref-filter.c   | 22 +++---
 3 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/bisect.c b/bisect.c
index 886e630884..a25d008693 100644
--- a/bisect.c
+++ b/bisect.c
@@ -754,9 +754,9 @@ static void handle_bad_merge_base(void)
exit(1);
 }
 
-static void handle_skipped_merge_base(const unsigned char *mb)
+static void handle_skipped_merge_base(const struct object_id *mb)
 {
-   char *mb_hex = sha1_to_hex(mb);
+   char *mb_hex = oid_to_hex(mb);
char *bad_hex = oid_to_hex(current_bad_oid);
char *good_hex = join_sha1_array_hex(_revs, ' ');
 
@@ -787,16 +787,16 @@ static void check_merge_bases(int no_checkout)
result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1);
 
for (; result; result = result->next) {
-   const unsigned char *mb = result->item->object.oid.hash;
-   if (!hashcmp(mb, current_bad_oid->hash)) {
+   const struct object_id *mb = >item->object.oid;
+   if (!oidcmp(mb, current_bad_oid)) {
handle_bad_merge_base();
-   } else if (0 <= sha1_array_lookup(_revs, mb)) {
+   } else if (0 <= sha1_array_lookup(_revs, mb->hash)) {
continue;
-   } else if (0 <= sha1_array_lookup(_revs, mb)) {
+   } else if (0 <= sha1_array_lookup(_revs, mb->hash)) {
handle_skipped_merge_base(mb);
} else {
printf(_("Bisecting: a merge base must be tested\n"));
-   exit(bisect_checkout(mb, no_checkout));
+   exit(bisect_checkout(mb->hash, no_checkout));
}
}
 
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index dfeacd5c37..dca1b68e69 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2670,14 +2670,14 @@ static int has_sha1_pack_kept_or_nonlocal(const 
unsigned char *sha1)
  */
 static struct sha1_array recent_objects;
 
-static int loosened_object_can_be_discarded(const unsigned char *sha1,
+static int loosened_object_can_be_discarded(const struct object_id *oid,
unsigned long mtime)
 {
if (!unpack_unreachable_expiration)
return 0;
if (mtime > unpack_unreachable_expiration)
return 0;
-   if (sha1_array_lookup(_objects, sha1) >= 0)
+   if (sha1_array_lookup(_objects, oid->hash) >= 0)
return 0;
return 1;
 }
@@ -2686,7 +2686,7 @@ static void loosen_unused_packed_objects(struct rev_info 
*revs)
 {
struct packed_git *p;
uint32_t i;
-   const unsigned char *sha1;
+   struct object_id oid;
 
for (p = packed_git; p; p = p->next) {
if (!p->pack_local || p->pack_keep)
@@ -2696,11 +2696,11 @@ static void loosen_unused_packed_objects(struct 
rev_info *revs)
die("cannot open pack index");
 
for (i = 0; i < p->num_objects; i++) {
-   sha1 = nth_packed_object_sha1(p, i);
-   if (!packlist_find(_pack, sha1, NULL) &&
-   !has_sha1_pack_kept_or_nonlocal(sha1) &&
-   !loosened_object_can_be_discarded(sha1, p->mtime))
-   if (force_object_loose(sha1, p->mtime))
+   nth_packed_object_oid(, p, i);
+   if (!packlist_find(_pack, oid.hash, NULL) &&
+   !has_sha1_pack_kept_or_nonlocal(oid.hash) &&
+   !loosened_object_can_be_discarded(, p->mtime))
+   if (force_object_loose(oid.hash, p->mtime))
die("unable to force loose object");
}
}
diff --git a/ref-filter.c b/ref-filter.c
index 9c82b5b9d6..d3dcb53dd5 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1677,22 +1677,22 @@ static int filter_pattern_match(struct ref_filter 
*filter, const char *refname)
  * the need to parse the object via parse_object(). peel_ref() might be a
  * more efficient alternative to obtain the pointee.
  */
-static const unsigned char *match_points_at(struct sha1_array *points_at,
-   const unsigned char *sha1,
-   const char *refname)
+static const struct object_id *match_points_at(struct sha1_array *points_at,
+  const struct object_id *oid,
+  const char *refname)
 {
-   const unsigned char *tagged_sha1 = NULL;
+   const struct object_id *tagged_oid = NULL;

[PATCH v2 05/21] builtin/pull: convert portions to struct object_id

[brian m. carlson]

Convert the caller of sha1_array_append to struct object_id.

Signed-off-by: brian m. carlson 
---
 builtin/pull.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 3ecb881b0b..a9f7553f30 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -335,16 +335,16 @@ static void get_merge_heads(struct sha1_array 
*merge_heads)
const char *filename = git_path("FETCH_HEAD");
FILE *fp;
struct strbuf sb = STRBUF_INIT;
-   unsigned char sha1[GIT_SHA1_RAWSZ];
+   struct object_id oid;
 
if (!(fp = fopen(filename, "r")))
die_errno(_("could not open '%s' for reading"), filename);
while (strbuf_getline_lf(, fp) != EOF) {
-   if (get_sha1_hex(sb.buf, sha1))
+   if (get_oid_hex(sb.buf, ))
continue;  /* invalid line: does not start with SHA1 */
if (starts_with(sb.buf + GIT_SHA1_HEXSZ, "\tnot-for-merge\t"))
continue;  /* ref is not-for-merge */
-   sha1_array_append(merge_heads, sha1);
+   sha1_array_append(merge_heads, oid.hash);
}
fclose(fp);
strbuf_release();

[PATCH v2 19/21] Convert sha1_array_for_each_unique and for_each_abbrev to object_id

[brian m. carlson]

Make sha1_array_for_each_unique take a callback using struct object_id.
Since one of these callbacks is an argument to for_each_abbrev, convert
those as well.  Rename various functions, replacing "sha1" with "oid".

Signed-off-by: brian m. carlson 
---
 builtin/cat-file.c |  4 ++--
 builtin/receive-pack.c | 12 ++--
 builtin/rev-parse.c|  4 ++--
 cache.h|  2 +-
 sha1-array.c   |  4 ++--
 sha1-array.h   |  6 +++---
 sha1_name.c| 14 ++
 submodule.c| 20 ++--
 t/helper/test-sha1-array.c |  6 +++---
 9 files changed, 35 insertions(+), 37 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 8fbb667170..eb0043231d 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -401,10 +401,10 @@ struct object_cb_data {
struct expand_data *expand;
 };
 
-static int batch_object_cb(const unsigned char sha1[20], void *vdata)
+static int batch_object_cb(const struct object_id *oid, void *vdata)
 {
struct object_cb_data *data = vdata;
-   hashcpy(data->expand->oid.hash, sha1);
+   oidcpy(>expand->oid, oid);
batch_object_write(NULL, data->opt, data->expand);
return 0;
 }
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 56d1a59922..5b83e4c87b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -225,10 +225,10 @@ static int receive_pack_config(const char *var, const 
char *value, void *cb)
return git_default_config(var, value, cb);
 }
 
-static void show_ref(const char *path, const unsigned char *sha1)
+static void show_ref(const char *path, const struct object_id *oid)
 {
if (sent_capabilities) {
-   packet_write_fmt(1, "%s %s\n", sha1_to_hex(sha1), path);
+   packet_write_fmt(1, "%s %s\n", oid_to_hex(oid), path);
} else {
struct strbuf cap = STRBUF_INIT;
 
@@ -244,7 +244,7 @@ static void show_ref(const char *path, const unsigned char 
*sha1)
strbuf_addstr(, " push-options");
strbuf_addf(, " agent=%s", git_user_agent_sanitized());
packet_write_fmt(1, "%s %s%c%s\n",
-sha1_to_hex(sha1), path, 0, cap.buf);
+oid_to_hex(oid), path, 0, cap.buf);
strbuf_release();
sent_capabilities = 1;
}
@@ -271,7 +271,7 @@ static int show_ref_cb(const char *path_full, const struct 
object_id *oid,
} else {
oidset_insert(seen, oid);
}
-   show_ref(path, oid->hash);
+   show_ref(path, oid);
return 0;
 }
 
@@ -284,7 +284,7 @@ static void show_one_alternate_ref(const char *refname,
if (oidset_insert(seen, oid))
return;
 
-   show_ref(".have", oid->hash);
+   show_ref(".have", oid);
 }
 
 static void write_head_info(void)
@@ -295,7 +295,7 @@ static void write_head_info(void)
for_each_alternate_ref(show_one_alternate_ref, );
oidset_clear();
if (!sent_capabilities)
-   show_ref("capabilities^{}", null_sha1);
+   show_ref("capabilities^{}", _oid);
 
advertise_shallow_grafts(1);
 
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 1e5bdea0d5..8d635add8f 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -205,9 +205,9 @@ static int anti_reference(const char *refname, const struct 
object_id *oid, int
return 0;
 }
 
-static int show_abbrev(const unsigned char *sha1, void *cb_data)
+static int show_abbrev(const struct object_id *oid, void *cb_data)
 {
-   show_rev(NORMAL, sha1, NULL);
+   show_rev(NORMAL, oid->hash, NULL);
return 0;
 }
 
diff --git a/cache.h b/cache.h
index 179bdc5da2..93e7f0d000 100644
--- a/cache.h
+++ b/cache.h
@@ -1350,7 +1350,7 @@ extern int get_sha1_with_context(const char *str, 
unsigned flags, unsigned char
 
 extern int get_oid(const char *str, struct object_id *oid);
 
-typedef int each_abbrev_fn(const unsigned char *sha1, void *);
+typedef int each_abbrev_fn(const struct object_id *oid, void *);
 extern int for_each_abbrev(const char *prefix, each_abbrev_fn, void *);
 
 extern int set_disambiguate_hint_config(const char *var, const char *value);
diff --git a/sha1-array.c b/sha1-array.c
index 1082b3dc11..82a7f4435c 100644
--- a/sha1-array.c
+++ b/sha1-array.c
@@ -43,7 +43,7 @@ void sha1_array_clear(struct sha1_array *array)
 }
 
 int sha1_array_for_each_unique(struct sha1_array *array,
-   for_each_sha1_fn fn,
+   for_each_oid_fn fn,
void *data)
 {
int i;
@@ -55,7 +55,7 @@ int sha1_array_for_each_unique(struct sha1_array *array,
int ret;
if (i > 0 && !oidcmp(array->oid + i, array->oid + i - 1))
continue;
-   ret = 

[PATCH v2 07/21] builtin/receive-pack: convert portions to struct object_id

[brian m. carlson]

Convert some hardcoded constants into uses of parse_oid_hex.
Additionally, convert all uses of struct command, and miscellaneous
other functions necessary for that.  This work is necessary to be able
to convert sha1_array_append later on.

To avoid needing to specify a constant, reject shallow lines with the
wrong length instead of simply ignoring them.

Note that in queue_command we are guaranteed to have a NUL-terminated
buffer or at least one byte of overflow that we can safely read, so the
linelen check can be elided.  We would die in such a case, but not read
invalid memory.

Signed-off-by: brian m. carlson 
---
 builtin/receive-pack.c | 98 +-
 1 file changed, 49 insertions(+), 49 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 116f3177a1..e3dc3e184d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -309,8 +309,8 @@ struct command {
unsigned int skip_update:1,
 did_not_exist:1;
int index;
-   unsigned char old_sha1[20];
-   unsigned char new_sha1[20];
+   struct object_id old_oid;
+   struct object_id new_oid;
char ref_name[FLEX_ARRAY]; /* more */
 };
 
@@ -723,7 +723,7 @@ static int feed_receive_hook(void *state_, const char 
**bufp, size_t *sizep)
return -1; /* EOF */
strbuf_reset(>buf);
strbuf_addf(>buf, "%s %s %s\n",
-   sha1_to_hex(cmd->old_sha1), sha1_to_hex(cmd->new_sha1),
+   oid_to_hex(>old_oid), oid_to_hex(>new_oid),
cmd->ref_name);
state->cmd = cmd->next;
if (bufp) {
@@ -764,8 +764,8 @@ static int run_update_hook(struct command *cmd)
return 0;
 
argv[1] = cmd->ref_name;
-   argv[2] = sha1_to_hex(cmd->old_sha1);
-   argv[3] = sha1_to_hex(cmd->new_sha1);
+   argv[2] = oid_to_hex(>old_oid);
+   argv[3] = oid_to_hex(>new_oid);
argv[4] = NULL;
 
proc.no_stdin = 1;
@@ -988,8 +988,8 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
const char *name = cmd->ref_name;
struct strbuf namespaced_name_buf = STRBUF_INIT;
const char *namespaced_name, *ret;
-   unsigned char *old_sha1 = cmd->old_sha1;
-   unsigned char *new_sha1 = cmd->new_sha1;
+   struct object_id *old_oid = >old_oid;
+   struct object_id *new_oid = >new_oid;
 
/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -1014,20 +1014,20 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
refuse_unconfigured_deny();
return "branch is currently checked out";
case DENY_UPDATE_INSTEAD:
-   ret = update_worktree(new_sha1);
+   ret = update_worktree(new_oid->hash);
if (ret)
return ret;
break;
}
}
 
-   if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
+   if (!is_null_oid(new_oid) && !has_object_file(new_oid)) {
error("unpack should have generated %s, "
- "but I can't find it!", sha1_to_hex(new_sha1));
+ "but I can't find it!", oid_to_hex(new_oid));
return "bad pack";
}
 
-   if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
+   if (!is_null_oid(old_oid) && is_null_oid(new_oid)) {
if (deny_deletes && starts_with(name, "refs/heads/")) {
rp_error("denying ref deletion for %s", name);
return "deletion prohibited";
@@ -1053,14 +1053,14 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
}
}
 
-   if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
-   !is_null_sha1(old_sha1) &&
+   if (deny_non_fast_forwards && !is_null_oid(new_oid) &&
+   !is_null_oid(old_oid) &&
starts_with(name, "refs/heads/")) {
struct object *old_object, *new_object;
struct commit *old_commit, *new_commit;
 
-   old_object = parse_object(old_sha1);
-   new_object = parse_object(new_sha1);
+   old_object = parse_object(old_oid->hash);
+   new_object = parse_object(new_oid->hash);
 
if (!old_object || !new_object ||
old_object->type != OBJ_COMMIT ||
@@ -1081,10 +1081,10 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
return "hook declined";
}
 
-   if (is_null_sha1(new_sha1)) {
+   if (is_null_oid(new_oid)) {
struct strbuf err = STRBUF_INIT;
-   if (!parse_object(old_sha1)) {
-   old_sha1 = NULL;

[PATCH v2 00/21] object_id part 7

[brian m. carlson]

This is part 7 in the continuing transition to use struct object_id.

This series focuses on two main areas: adding two constants for the
maximum hash size we'll be using (which will be suitable for allocating
memory) and converting struct sha1_array to struct oid_array.

The rationale for adding separate constants for allocating memory is
that with a new 256-bit hash function, we're going to need two different
items: a constant for allocating memory that's as large as the largest
hash, and a global variable telling us size the current hash is.  I've
opted to provide GIT_MAX_RAWSZ and GIT_MAX_HEXSZ for allocating memory,
and leave GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ as values that can be later
replaced by the aforementioned global.

Replacing struct sha1_array with struct oid_array necessarily involves
converting the shallow code, so I did that.  The structure now handles
objects of struct object_id.  While I renamed the documentation (since
people will search for that), I chose not to rename the sha1-array.[ch]
files or the test helper because I didn't think it was worth the hassle,
especially for people who don't have rename support turned on by
default.

There is also a patch for fixing some broken pointer arithmetic that was
discovered during review of v1.  I don't think it's exploitable, but it
seems good to fix anyway.  Additional eyes on this patch are welcomed.

I chose to use Coccinelle quite a bit in this series, as it automates a
lot of the manual work and aides in review.  There is also some use of
Perl one-liners.

This series is available at https://github.com/bk2204/git under
object-id-part7; it may be rebased.

Changes from v1:
* Rebase on current master (no changes).
* Remove check for empty line in queue_command.
* Add patch 6 to fix invalid pointer arithmetic.

brian m. carlson (21):
  Define new hash-size constants for allocating memory
  Convert GIT_SHA1_HEXSZ used for allocation to GIT_MAX_HEXSZ
  Convert GIT_SHA1_RAWSZ used for allocation to GIT_MAX_RAWSZ
  builtin/diff: convert to struct object_id
  builtin/pull: convert portions to struct object_id
  builtin/receive-pack: fix incorrect pointer arithmetic
  builtin/receive-pack: convert portions to struct object_id
  fsck: convert init_skiplist to struct object_id
  parse-options-cb: convert sha1_array_append caller to struct object_id
  test-sha1-array: convert most code to struct object_id
  sha1_name: convert struct disambiguate_state to object_id
  sha1_name: convert disambiguate_hint_fn to take object_id
  submodule: convert check_for_new_submodule_commits to object_id
  builtin/pull: convert to struct object_id
  sha1-array: convert internal storage for struct sha1_array to
object_id
  Make sha1_array_append take a struct object_id *
  Convert remaining callers of sha1_array_lookup to object_id
  Convert sha1_array_lookup to take struct object_id
  Convert sha1_array_for_each_unique and for_each_abbrev to object_id
  Rename sha1_array to oid_array
  Documentation: update and rename api-sha1-array.txt

 .../{api-sha1-array.txt => api-oid-array.txt}  |  44 +++
 bisect.c   |  43 ---
 builtin/blame.c|   4 +-
 builtin/cat-file.c |  14 +--
 builtin/diff.c |  40 +++---
 builtin/fetch-pack.c   |   2 +-
 builtin/fetch.c|   6 +-
 builtin/merge-index.c  |   2 +-
 builtin/merge.c|   2 +-
 builtin/pack-objects.c |  24 ++--
 builtin/patch-id.c |   2 +-
 builtin/pull.c |  98 +++
 builtin/receive-pack.c | 136 ++---
 builtin/rev-list.c |   2 +-
 builtin/rev-parse.c|   4 +-
 builtin/send-pack.c|   4 +-
 cache.h|  10 +-
 combine-diff.c |  18 +--
 commit.h   |  14 +--
 connect.c  |   8 +-
 diff.c |   4 +-
 diff.h |   4 +-
 fetch-pack.c   |  32 ++---
 fetch-pack.h   |   4 +-
 fsck.c |  17 +--
 fsck.h |   2 +-
 hex.c  |   2 +-
 parse-options-cb.c |   8 +-
 patch-ids.c|   2 +-
 patch-ids.h|   2 +-
 ref-filter.c   |  22 ++--
 

[PATCH v2 16/21] Make sha1_array_append take a struct object_id *

[brian m. carlson]

Convert the callers to pass struct object_id by changing the function
declaration and definition and applying the following semantic patch:

@@
expression E1, E2, E3;
@@
- sha1_array_append(E1, E2[E3].hash)
+ sha1_array_append(E1, E2 + E3)

@@
expression E1, E2;
@@
- sha1_array_append(E1, E2.hash)
+ sha1_array_append(E1, )

@@
expression E1, E2;
@@
- sha1_array_append(E1, E2->hash)
+ sha1_array_append(E1, E2)

Signed-off-by: brian m. carlson 
---
 bisect.c   | 4 ++--
 builtin/cat-file.c | 4 ++--
 builtin/diff.c | 2 +-
 builtin/pack-objects.c | 4 ++--
 builtin/pull.c | 2 +-
 builtin/receive-pack.c | 6 +++---
 combine-diff.c | 2 +-
 connect.c  | 4 ++--
 fetch-pack.c   | 8 
 fsck.c | 2 +-
 parse-options-cb.c | 2 +-
 sha1-array.c   | 4 ++--
 sha1-array.h   | 2 +-
 sha1_name.c| 2 +-
 submodule.c| 6 +++---
 t/helper/test-sha1-array.c | 2 +-
 transport.c| 6 --
 17 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/bisect.c b/bisect.c
index ebaf7b05ba..886e630884 100644
--- a/bisect.c
+++ b/bisect.c
@@ -413,9 +413,9 @@ static int register_ref(const char *refname, const struct 
object_id *oid,
current_bad_oid = xmalloc(sizeof(*current_bad_oid));
oidcpy(current_bad_oid, oid);
} else if (starts_with(refname, good_prefix.buf)) {
-   sha1_array_append(_revs, oid->hash);
+   sha1_array_append(_revs, oid);
} else if (starts_with(refname, "skip-")) {
-   sha1_array_append(_revs, oid->hash);
+   sha1_array_append(_revs, oid);
}
 
strbuf_release(_prefix);
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 8b85cb8cf0..8fbb667170 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -413,7 +413,7 @@ static int batch_loose_object(const struct object_id *oid,
  const char *path,
  void *data)
 {
-   sha1_array_append(data, oid->hash);
+   sha1_array_append(data, oid);
return 0;
 }
 
@@ -422,7 +422,7 @@ static int batch_packed_object(const struct object_id *oid,
   uint32_t pos,
   void *data)
 {
-   sha1_array_append(data, oid->hash);
+   sha1_array_append(data, oid);
return 0;
 }
 
diff --git a/builtin/diff.c b/builtin/diff.c
index 398eee00d5..a5b34eb156 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -193,7 +193,7 @@ static int builtin_diff_combined(struct rev_info *revs,
if (!revs->dense_combined_merges && !revs->combine_merges)
revs->dense_combined_merges = revs->combine_merges = 1;
for (i = 1; i < ents; i++)
-   sha1_array_append(, ent[i].item->oid.hash);
+   sha1_array_append(, [i].item->oid);
diff_tree_combined(ent[0].item->oid.hash, ,
   revs->dense_combined_merges, revs);
sha1_array_clear();
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 16517f2637..dfeacd5c37 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2739,12 +2739,12 @@ static void record_recent_object(struct object *obj,
 const char *name,
 void *data)
 {
-   sha1_array_append(_objects, obj->oid.hash);
+   sha1_array_append(_objects, >oid);
 }
 
 static void record_recent_commit(struct commit *commit, void *data)
 {
-   sha1_array_append(_objects, commit->object.oid.hash);
+   sha1_array_append(_objects, >object.oid);
 }
 
 static void get_object_list(int ac, const char **av)
diff --git a/builtin/pull.c b/builtin/pull.c
index c007900ab5..183e377147 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -344,7 +344,7 @@ static void get_merge_heads(struct sha1_array *merge_heads)
continue;  /* invalid line: does not start with SHA1 */
if (starts_with(sb.buf + GIT_SHA1_HEXSZ, "\tnot-for-merge\t"))
continue;  /* ref is not-for-merge */
-   sha1_array_append(merge_heads, oid.hash);
+   sha1_array_append(merge_heads, );
}
fclose(fp);
strbuf_release();
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 85046607fe..56d1a59922 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -842,7 +842,7 @@ static int update_shallow_ref(struct command *cmd, struct 
shallow_info *si)
if (si->used_shallow[i] &&
(si->used_shallow[i][cmd->index / 32] & mask) &&
!delayed_reachability_test(si, i))
-   sha1_array_append(, si->shallow->oid[i].hash);
+   sha1_array_append(, si->shallow->oid + i);
 
opt.env = 

[PATCH v2 18/21] Convert sha1_array_lookup to take struct object_id

[brian m. carlson]

Convert this function by changing the declaration and definition and
applying the following semantic patch to update the callers:

@@
expression E1, E2;
@@
- sha1_array_lookup(E1, E2.hash)
+ sha1_array_lookup(E1, )

@@
expression E1, E2;
@@
- sha1_array_lookup(E1, E2->hash)
+ sha1_array_lookup(E1, E2)

Signed-off-by: brian m. carlson 
---
 bisect.c   | 7 +++
 builtin/pack-objects.c | 2 +-
 fsck.c | 2 +-
 ref-filter.c   | 4 ++--
 sha1-array.c   | 4 ++--
 sha1-array.h   | 2 +-
 t/helper/test-sha1-array.c | 2 +-
 7 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/bisect.c b/bisect.c
index a25d008693..f193257509 100644
--- a/bisect.c
+++ b/bisect.c
@@ -499,8 +499,7 @@ struct commit_list *filter_skipped(struct commit_list *list,
while (list) {
struct commit_list *next = list->next;
list->next = NULL;
-   if (0 <= sha1_array_lookup(_revs,
-  list->item->object.oid.hash)) {
+   if (0 <= sha1_array_lookup(_revs, 
>item->object.oid)) {
if (skipped_first && !*skipped_first)
*skipped_first = 1;
/* Move current to tried list */
@@ -790,9 +789,9 @@ static void check_merge_bases(int no_checkout)
const struct object_id *mb = >item->object.oid;
if (!oidcmp(mb, current_bad_oid)) {
handle_bad_merge_base();
-   } else if (0 <= sha1_array_lookup(_revs, mb->hash)) {
+   } else if (0 <= sha1_array_lookup(_revs, mb)) {
continue;
-   } else if (0 <= sha1_array_lookup(_revs, mb->hash)) {
+   } else if (0 <= sha1_array_lookup(_revs, mb)) {
handle_skipped_merge_base(mb);
} else {
printf(_("Bisecting: a merge base must be tested\n"));
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index dca1b68e69..028c7be9a2 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2677,7 +2677,7 @@ static int loosened_object_can_be_discarded(const struct 
object_id *oid,
return 0;
if (mtime > unpack_unreachable_expiration)
return 0;
-   if (sha1_array_lookup(_objects, oid->hash) >= 0)
+   if (sha1_array_lookup(_objects, oid) >= 0)
return 0;
return 1;
 }
diff --git a/fsck.c b/fsck.c
index 6682de1de5..24daedd6cc 100644
--- a/fsck.c
+++ b/fsck.c
@@ -280,7 +280,7 @@ static int report(struct fsck_options *options, struct 
object *object,
return 0;
 
if (options->skiplist && object &&
-   sha1_array_lookup(options->skiplist, object->oid.hash) 
>= 0)
+   sha1_array_lookup(options->skiplist, >oid) >= 0)
return 0;
 
if (msg_type == FSCK_FATAL)
diff --git a/ref-filter.c b/ref-filter.c
index d3dcb53dd5..4ee7ebcda3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1684,14 +1684,14 @@ static const struct object_id *match_points_at(struct 
sha1_array *points_at,
const struct object_id *tagged_oid = NULL;
struct object *obj;
 
-   if (sha1_array_lookup(points_at, oid->hash) >= 0)
+   if (sha1_array_lookup(points_at, oid) >= 0)
return oid;
obj = parse_object(oid->hash);
if (!obj)
die(_("malformed object at '%s'"), refname);
if (obj->type == OBJ_TAG)
tagged_oid = &((struct tag *)obj)->tagged->oid;
-   if (tagged_oid && sha1_array_lookup(points_at, tagged_oid->hash) >= 0)
+   if (tagged_oid && sha1_array_lookup(points_at, tagged_oid) >= 0)
return tagged_oid;
return NULL;
 }
diff --git a/sha1-array.c b/sha1-array.c
index 26e596b264..1082b3dc11 100644
--- a/sha1-array.c
+++ b/sha1-array.c
@@ -26,11 +26,11 @@ static const unsigned char *sha1_access(size_t index, void 
*table)
return array[index].hash;
 }
 
-int sha1_array_lookup(struct sha1_array *array, const unsigned char *sha1)
+int sha1_array_lookup(struct sha1_array *array, const struct object_id *oid)
 {
if (!array->sorted)
sha1_array_sort(array);
-   return sha1_pos(sha1, array->oid, array->nr, sha1_access);
+   return sha1_pos(oid->hash, array->oid, array->nr, sha1_access);
 }
 
 void sha1_array_clear(struct sha1_array *array)
diff --git a/sha1-array.h b/sha1-array.h
index 7b06fbf1c1..4cc55b15af 100644
--- a/sha1-array.h
+++ b/sha1-array.h
@@ -11,7 +11,7 @@ struct sha1_array {
 #define SHA1_ARRAY_INIT { NULL, 0, 0, 0 }
 
 void sha1_array_append(struct sha1_array *array, const struct object_id *sha1);
-int sha1_array_lookup(struct sha1_array *array, const unsigned char *sha1);
+int sha1_array_lookup(struct sha1_array *array, const struct object_id *oid);
 void 

[PATCH v2 08/21] fsck: convert init_skiplist to struct object_id

[brian m. carlson]

Convert a hardcoded constant buffer size to a use of GIT_MAX_HEXSZ, and
use parse_oid_hex to reduce the dependency on the size of the hash.
This function is a caller of sha1_array_append, which will be converted
later.

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

diff --git a/fsck.c b/fsck.c
index 939792752b..aff4ae6fd4 100644
--- a/fsck.c
+++ b/fsck.c
@@ -134,8 +134,8 @@ static void init_skiplist(struct fsck_options *options, 
const char *path)
 {
static struct sha1_array skiplist = SHA1_ARRAY_INIT;
int sorted, fd;
-   char buffer[41];
-   unsigned char sha1[20];
+   char buffer[GIT_MAX_HEXSZ + 1];
+   struct object_id oid;
 
if (options->skiplist)
sorted = options->skiplist->sorted;
@@ -148,17 +148,18 @@ static void init_skiplist(struct fsck_options *options, 
const char *path)
if (fd < 0)
die("Could not open skip list: %s", path);
for (;;) {
+   const char *p;
int result = read_in_full(fd, buffer, sizeof(buffer));
if (result < 0)
die_errno("Could not read '%s'", path);
if (!result)
break;
-   if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n')
+   if (parse_oid_hex(buffer, , ) || *p != '\n')
die("Invalid SHA-1: %s", buffer);
-   sha1_array_append(, sha1);
+   sha1_array_append(, oid.hash);
if (sorted && skiplist.nr > 1 &&
hashcmp(skiplist.sha1[skiplist.nr - 2],
-   sha1) > 0)
+   oid.hash) > 0)
sorted = 0;
}
close(fd);

[PATCH v2 21/21] Documentation: update and rename api-sha1-array.txt

[brian m. carlson]

Since the structure and functions have changed names, update the code
examples and the documentation.  Rename the file to match the new name
of the API.

Signed-off-by: brian m. carlson 
---
 .../{api-sha1-array.txt => api-oid-array.txt}  | 44 +++---
 1 file changed, 22 insertions(+), 22 deletions(-)
 rename Documentation/technical/{api-sha1-array.txt => api-oid-array.txt} (61%)

diff --git a/Documentation/technical/api-sha1-array.txt 
b/Documentation/technical/api-oid-array.txt
similarity index 61%
rename from Documentation/technical/api-sha1-array.txt
rename to Documentation/technical/api-oid-array.txt
index dcc52943a5..b0c11f868d 100644
--- a/Documentation/technical/api-sha1-array.txt
+++ b/Documentation/technical/api-oid-array.txt
@@ -1,7 +1,7 @@
-sha1-array API
+oid-array API
 ==
 
-The sha1-array API provides storage and manipulation of sets of SHA-1
+The oid-array API provides storage and manipulation of sets of object
 identifiers. The emphasis is on storage and processing efficiency,
 making them suitable for large lists. Note that the ordering of items is
 not preserved over some operations.
@@ -9,10 +9,10 @@ not preserved over some operations.
 Data Structures
 ---
 
-`struct sha1_array`::
+`struct oid_array`::
 
-   A single array of SHA-1 hashes. This should be initialized by
-   assignment from `SHA1_ARRAY_INIT`.  The `sha1` member contains
+   A single array of object IDs. This should be initialized by
+   assignment from `OID_ARRAY_INIT`.  The `oid` member contains
the actual data. The `nr` member contains the number of items in
the set.  The `alloc` and `sorted` members are used internally,
and should not be needed by API callers.
@@ -20,22 +20,22 @@ Data Structures
 Functions
 -
 
-`sha1_array_append`::
-   Add an item to the set. The sha1 will be placed at the end of
+`oid_array_append`::
+   Add an item to the set. The object ID will be placed at the end of
the array (but note that some operations below may lose this
ordering).
 
-`sha1_array_lookup`::
-   Perform a binary search of the array for a specific sha1.
+`oid_array_lookup`::
+   Perform a binary search of the array for a specific object ID.
If found, returns the offset (in number of elements) of the
-   sha1. If not found, returns a negative integer. If the array is
-   not sorted, this function has the side effect of sorting it.
+   object ID. If not found, returns a negative integer. If the array
+   is not sorted, this function has the side effect of sorting it.
 
-`sha1_array_clear`::
+`oid_array_clear`::
Free all memory associated with the array and return it to the
initial, empty state.
 
-`sha1_array_for_each_unique`::
+`oid_array_for_each_unique`::
Efficiently iterate over each unique element of the list,
executing the callback function for each one. If the array is
not sorted, this function has the side effect of sorting it. If
@@ -47,25 +47,25 @@ Examples
 
 
 -
-int print_callback(const unsigned char sha1[20],
+int print_callback(const struct object_id *oid,
void *data)
 {
-   printf("%s\n", sha1_to_hex(sha1));
+   printf("%s\n", oid_to_hex(oid));
return 0; /* always continue */
 }
 
 void some_func(void)
 {
-   struct sha1_array hashes = SHA1_ARRAY_INIT;
-   unsigned char sha1[20];
+   struct sha1_array hashes = OID_ARRAY_INIT;
+   struct object_id oid;
 
/* Read objects into our set */
-   while (read_object_from_stdin(sha1))
-   sha1_array_append(, sha1);
+   while (read_object_from_stdin(oid.hash))
+   oid_array_append(, );
 
/* Check if some objects are in our set */
-   while (read_object_from_stdin(sha1)) {
-   if (sha1_array_lookup(, sha1) >= 0)
+   while (read_object_from_stdin(oid.hash)) {
+   if (oid_array_lookup(, ) >= 0)
printf("it's in there!\n");
 
/*
@@ -75,6 +75,6 @@ void some_func(void)
 * Instead, this will sort once and then skip duplicates
 * in linear time.
 */
-   sha1_array_for_each_unique(, print_callback, NULL);
+   oid_array_for_each_unique(, print_callback, NULL);
 }
 -

[PATCH v2 12/21] sha1_name: convert disambiguate_hint_fn to take object_id

[brian m. carlson]

Convert this function pointer type and the functions that implement it
to take a struct object_id.  Introduce a temporary in
show_ambiguous_object to avoid having to convert for_each_abbrev at this
point.

Signed-off-by: brian m. carlson 
---
 sha1_name.c | 64 -
 1 file changed, 34 insertions(+), 30 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index cf6f4be0c6..2e38aedfa5 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -11,7 +11,7 @@
 
 static int get_sha1_oneline(const char *, unsigned char *, struct commit_list 
*);
 
-typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
+typedef int (*disambiguate_hint_fn)(const struct object_id *, void *);
 
 struct disambiguate_state {
int len; /* length of prefix in hex chars */
@@ -29,7 +29,7 @@ struct disambiguate_state {
unsigned always_call_fn:1;
 };
 
-static void update_candidates(struct disambiguate_state *ds, const unsigned 
char *current)
+static void update_candidates(struct disambiguate_state *ds, const struct 
object_id *current)
 {
if (ds->always_call_fn) {
ds->ambiguous = ds->fn(current, ds->cb_data) ? 1 : 0;
@@ -37,10 +37,10 @@ static void update_candidates(struct disambiguate_state 
*ds, const unsigned char
}
if (!ds->candidate_exists) {
/* this is the first candidate */
-   hashcpy(ds->candidate.hash, current);
+   oidcpy(>candidate, current);
ds->candidate_exists = 1;
return;
-   } else if (!hashcmp(ds->candidate.hash, current)) {
+   } else if (!oidcmp(>candidate, current)) {
/* the same as what we already have seen */
return;
}
@@ -52,14 +52,14 @@ static void update_candidates(struct disambiguate_state 
*ds, const unsigned char
}
 
if (!ds->candidate_checked) {
-   ds->candidate_ok = ds->fn(ds->candidate.hash, ds->cb_data);
+   ds->candidate_ok = ds->fn(>candidate, ds->cb_data);
ds->disambiguate_fn_used = 1;
ds->candidate_checked = 1;
}
 
if (!ds->candidate_ok) {
/* discard the candidate; we know it does not satisfy fn */
-   hashcpy(ds->candidate.hash, current);
+   oidcpy(>candidate, current);
ds->candidate_checked = 0;
return;
}
@@ -107,15 +107,15 @@ static void find_short_object_filename(struct 
disambiguate_state *ds)
continue;
 
while (!ds->ambiguous && (de = readdir(dir)) != NULL) {
-   unsigned char sha1[20];
+   struct object_id oid;
 
-   if (strlen(de->d_name) != 38)
+   if (strlen(de->d_name) != GIT_SHA1_HEXSZ - 2)
continue;
if (memcmp(de->d_name, ds->hex_pfx + 2, ds->len - 2))
continue;
-   memcpy(hex + 2, de->d_name, 38);
-   if (!get_sha1_hex(hex, sha1))
-   update_candidates(ds, sha1);
+   memcpy(hex + 2, de->d_name, GIT_SHA1_HEXSZ - 2);
+   if (!get_oid_hex(hex, ))
+   update_candidates(ds, );
}
closedir(dir);
}
@@ -140,7 +140,7 @@ static void unique_in_pack(struct packed_git *p,
   struct disambiguate_state *ds)
 {
uint32_t num, last, i, first = 0;
-   const unsigned char *current = NULL;
+   const struct object_id *current = NULL;
 
open_pack_index(p);
num = p->num_objects;
@@ -169,8 +169,9 @@ static void unique_in_pack(struct packed_git *p,
 * 0, 1 or more objects that actually match(es).
 */
for (i = first; i < num && !ds->ambiguous; i++) {
-   current = nth_packed_object_sha1(p, i);
-   if (!match_sha(ds->len, ds->bin_pfx.hash, current))
+   struct object_id oid;
+   current = nth_packed_object_oid(, p, i);
+   if (!match_sha(ds->len, ds->bin_pfx.hash, current->hash))
break;
update_candidates(ds, current);
}
@@ -213,7 +214,7 @@ static int finish_object_disambiguation(struct 
disambiguate_state *ds,
 * same repository!
 */
ds->candidate_ok = (!ds->disambiguate_fn_used ||
-   ds->fn(ds->candidate.hash, ds->cb_data));
+   ds->fn(>candidate, ds->cb_data));
 
if (!ds->candidate_ok)
return SHORT_NAME_AMBIGUOUS;
@@ -222,57 +223,57 @@ static int finish_object_disambiguation(struct 
disambiguate_state *ds,
return 0;
 }
 
-static int disambiguate_commit_only(const unsigned char *sha1, 

[PATCH v2 09/21] parse-options-cb: convert sha1_array_append caller to struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson 
---
 parse-options-cb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index b7d8f7dcb2..40ece4d8c2 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -96,7 +96,7 @@ int parse_opt_commits(const struct option *opt, const char 
*arg, int unset)
 
 int parse_opt_object_name(const struct option *opt, const char *arg, int unset)
 {
-   unsigned char sha1[20];
+   struct object_id oid;
 
if (unset) {
sha1_array_clear(opt->value);
@@ -104,9 +104,9 @@ int parse_opt_object_name(const struct option *opt, const 
char *arg, int unset)
}
if (!arg)
return -1;
-   if (get_sha1(arg, sha1))
+   if (get_oid(arg, ))
return error(_("malformed object name '%s'"), arg);
-   sha1_array_append(opt->value, sha1);
+   sha1_array_append(opt->value, oid.hash);
return 0;
 }
 

[PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

[René Scharfe]

FreeBSD implements getcwd(3) as a syscall, but falls back to a version
based on readdir(3) if it fails for some reason.  The latter requires
permissions to read and execute path components, while the former does
not.  That means that if our buffer is too small and we're missing
rights we could get EACCES, but we may succeed with a bigger buffer.

Keep retrying if getcwd(3) indicates lack of permissions until our
buffer can fit PATH_MAX bytes, as that's the maximum supported by the
syscall on FreeBSD anyway.  This way we do what we can to be able to
benefit from the syscall, but we also won't loop forever if there is a
real permission issue.

This fixes a regression introduced with 7333ed17 (setup: convert
setup_git_directory_gently_1 et al. to strbuf, 2014-07-28) for paths
longer than 127 bytes with components that miss read or execute
permissions (e.g. 0711 on /home for privacy reasons); we used a fixed
PATH_MAX-sized buffer before.

Reported-by: Zenobiusz Kunegunda 
Signed-off-by: Rene Scharfe 
---
 strbuf.c| 11 +++
 t/t0001-init.sh | 14 ++
 2 files changed, 25 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index ace58e7367..00457940cf 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -449,6 +449,17 @@ int strbuf_getcwd(struct strbuf *sb)
strbuf_setlen(sb, strlen(sb->buf));
return 0;
}
+
+   /*
+* If getcwd(3) is implemented as a syscall that falls
+* back to a regular lookup using readdir(3) etc. then
+* we may be able to avoid EACCES by providing enough
+* space to the syscall as it's not necessarily bound
+* to the same restrictions as the fallback.
+*/
+   if (errno == EACCES && guessed_len < PATH_MAX)
+   continue;
+
if (errno != ERANGE)
break;
}
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index e424de5363..5f81fbe07c 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -315,6 +315,20 @@ test_expect_success 'init with separate gitdir' '
test_path_is_dir realgitdir/refs
 '
 
+test_expect_success 'init in long base path' '
+   # exceed initial buffer size of strbuf_getcwd()
+   component=123456789abcdef &&
+   test_when_finished "chmod 0700 $component; rm -rf $component" &&
+   p31=$component/$component &&
+   p127=$p31/$p31/$p31/$p31 &&
+   mkdir -p $p127 &&
+   chmod 0111 $component &&
+   (
+   cd $p127 &&
+   git init newdir
+   )
+'
+
 test_expect_success 're-init on .git file' '
( cd newdir && git init )
 '
-- 
2.12.2

Re: [PATCH] pretty: add extra headers and MIME boundary directly

[René Scharfe]

Am 25.03.2017 um 22:11 schrieb Jeff King:
> The most correct way is that the caller of log_write_email_headers() and
> diff_flush() should have a function-local strbuf which holds the data,
> gets passed to diff_flush() as some kind opaque context, and then is
> freed afterwards. We don't have such a context, but if we were to abuse
> diff_options.stat_sep _temporarily_, that would still be a lot cleaner.
> I.e., something like this:
> 
>   struct strbuf stat_sep = STRBUF_INIT;
> 
>   /* may write into stat_sep, depending on options */
>   log_write_email_headers(..., _sep);
>   opt->diffopt.stat_sep = stat_sep.buf;
> 
>   diff_flush(>diffopt);
>   opt->diffopt.stat_sep = NULL;
>   strbuf_release(_sep);
> 
> But it's a bit tricky because those two hunks happen in separate
> functions, which means passing the strbuf around.

You could have a destructor callback, called at the end of diff_flush().

> Anyway. Here's my attempt at the callback version of stat_sep.
> 
> ---
> diff --git a/diff.c b/diff.c
> index a628ac3a9..d061f9e18 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4819,10 +4819,9 @@ void diff_flush(struct diff_options *options)
>   fprintf(options->file, "%s%c",
>   diff_line_prefix(options),
>   options->line_termination);
> - if (options->stat_sep) {
> - /* attach patch instead of inline */
> - fputs(options->stat_sep, options->file);
> - }
> + if (options->stat_sep)
> + options->stat_sep(options->file,
> +   options->stat_sep_data);
>   }
>  
>   for (i = 0; i < q->nr; i++) {
> diff --git a/diff.h b/diff.h
> index e9ccb38c2..4785f3b23 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -154,9 +154,11 @@ struct diff_options {
>   unsigned ws_error_highlight;
>   const char *prefix;
>   int prefix_length;
> - const char *stat_sep;
>   long xdl_opts;
>  
> + void (*stat_sep)(FILE *, void *);
> + void *stat_sep_data;
> +
>   int stat_width;
>   int stat_name_width;
>   int stat_graph_width;
> diff --git a/log-tree.c b/log-tree.c
> index 7049a1778..5cf825c41 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -348,6 +348,31 @@ void fmt_output_email_subject(struct strbuf *sb, struct 
> rev_info *opt)
>   }
>  }
>  
> +static void show_mime_attachment(FILE *out, void *data)
> +{
> + struct rev_info *opt = data;
> + struct strbuf filename = STRBUF_INIT;
> +
> + if (opt->numbered_files)
> + strbuf_addf(, "%d", opt->nr);
> + else
> + fmt_output_commit(, opt->commit_for_mime, opt);
> +
> + fprintf(out,
> + "\n--%s%s\n"
> + "Content-Type: text/x-patch;"
> + " name=\"%s\"\n"
> + "Content-Transfer-Encoding: 8bit\n"
> + "Content-Disposition: %s;"
> + " filename=\"%s\"\n\n",
> + mime_boundary_leader, opt->mime_boundary,
> + filename.buf,
> + opt->no_inline ? "attachment" : "inline",
> + filename.buf);
> +
> + strbuf_release();
> +}
> +
>  void log_write_email_headers(struct rev_info *opt, struct commit *commit,
>int *need_8bit_cte_p)
>  {
> @@ -372,27 +397,10 @@ void log_write_email_headers(struct rev_info *opt, 
> struct commit *commit,
>   graph_show_oneline(opt->graph);
>   }
>   if (opt->mime_boundary) {
> - static char buffer[1024];
> - struct strbuf filename =  STRBUF_INIT;
>   *need_8bit_cte_p = -1; /* NEVER */
> -
> - if (opt->numbered_files)
> - strbuf_addf(, "%d", opt->nr);
> - else
> - fmt_output_commit(, commit, opt);
> - snprintf(buffer, sizeof(buffer) - 1,
> -  "\n--%s%s\n"
> -  "Content-Type: text/x-patch;"
> -  " name=\"%s\"\n"
> -  "Content-Transfer-Encoding: 8bit\n"
> -  "Content-Disposition: %s;"
> -  " filename=\"%s\"\n\n",
> -  mime_boundary_leader, opt->mime_boundary,
> -  filename.buf,
> -  opt->no_inline ? "attachment" : "inline",
> -  filename.buf);
> - opt->diffopt.stat_sep = buffer;
> - strbuf_release();
> + opt->diffopt.stat_sep = show_mime_attachment;
> + opt->diffopt.stat_sep_data = opt;
> + opt->commit_for_mime = commit;
>   }
>  }
>  
> diff --git a/revision.h b/revision.h
> index 14886ec92..46ca45d96 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -156,6 +156,7 @@ struct rev_info {
>   struct log_info *loginfo;
>   int nr, total;
>   const char  

[PATCH v2 1/3] rev-parse: match @{upstream}, @{u} and @{push} case-insensitively

[Ævar Arnfjörð Bjarmason]

Change the revision parsing logic to match @{upstream}, @{u} & @{push}
case-insensitively.

Before this change supplying anything except the lower-case forms
emits an "unknown revision or path not in the working tree"
error. This change makes upper-case & mixed-case versions equivalent
to the lower-case versions.

The use-case for this is being able to hold the shift key down while
typing @{u} on certain keyboard layouts, which makes the sequence
easier to type, and reduces cases where git throws an error at the
user where it could do what he means instead.

These suffixes now join various other suffixes & special syntax
documented in gitrevisions(7) that matches case-insensitively. A table
showing the status of the various forms documented there before &
after this patch is shown below. The key for the table is:

 - CI  = Case Insensitive
 - CIP = Case Insensitive Possible (without ambiguities)
 - AG  = Accepts Garbage (.e.g. @{./.4.minutes./.})

Before this change:

|+-+--+-|
| What?  | CI? | CIP? | AG? |
|+-+--+-|
| sha1   | Y   | -| N   |
| describeOutput | N   | N| N   |
| refname| N   | N| N   |
| @{}  | Y   | Y| Y   |
| @{} | N/A | N/A  | N   |
| @{-}| N/A | N/A  | N   |
| @{upstream}| N   | Y| N   |
| @{push}| N   | Y| N   |
| ^{}  | N   | Y| N   |
| ^{/regex}  | N   | N| N   |
|+-+--+-|

After it:

|+-+--+-|
| What?  | CI? | CIP? | AG? |
|+-+--+-|
| sha1   | Y   | -| N   |
| describeOutput | N   | N| N   |
| refname| N   | N| N   |
| @{}  | Y   | Y| Y   |
| @{} | N/A | N/A  | N   |
| @{-}| N/A | N/A  | N   |
| @{upstream}| Y   | -| N   |
| @{push}| Y   | -| N   |
| ^{}  | N   | Y| N   |
| ^{/regex}  | N   | N| N   |
|+-+--+-|

The ^{} suffix could be changed to be case-insensitive as well
without introducing any ambiguities. That was included in an earlier
version of this patch, but Junio objected to its inclusion in [2].

This change was independently authored to scratch a longtime itch, but
when I was about to submit it I discovered that a similar patch had
been submitted unsuccessfully before by Conrad Irwin in August 2011 as
"rev-parse: Allow @{U} as a synonym for @{u}"[1].

The tests for this patch are more exhaustive than in the 2011
submission. The starting point for them was to first change the code
to only support upper-case versions of the existing words, seeing what
broke, and amending the breaking tests to check upper case & mixed
case as appropriate, and where not redundant to other similar
tests. The implementation itself is equivalent.

1. <1313287071-7851-1-git-send-email-conrad.ir...@gmail.com>
2. 

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/revisions.txt   |  6 +-
 sha1_name.c   |  2 +-
 t/t1507-rev-parse-upstream.sh | 15 +++
 t/t1514-rev-parse-push.sh |  8 ++--
 4 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index ba11b9c95e..09e0d51b9e 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -96,7 +96,8 @@ some output processing may assume ref names in UTF-8.
   refers to the branch that the branch specified by branchname is set to build 
on
   top of (configured with `branch..remote` and
   `branch..merge`).  A missing branchname defaults to the
-  current one.
+  current one. These suffixes are accepted when spelled in uppercase, and
+  they mean the same thing no matter the case.
 
 '@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
   The suffix '@\{push}' reports the branch "where we would push to" if
@@ -122,6 +123,9 @@ refs/remotes/myfork/mybranch
 Note in the example that we set up a triangular workflow, where we pull
 from one location and push to another. In a non-triangular workflow,
 '@\{push}' is the same as '@\{upstream}', and there is no need for it.
++
+This suffix is accepted when spelled in uppercase, and means the same
+thing no matter the case.
 
 '{caret}', e.g. 'HEAD{caret}, v1.5.1{caret}0'::
   A suffix '{caret}' to a revision parameter means the first parent of
diff --git a/sha1_name.c b/sha1_name.c
index cda9e49b12..d9d1b2fce8 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -549,7 +549,7 @@ static inline int at_mark(const char *string, int len,
for (i = 0; i < nr; i++) {
int suffix_len = strlen(suffix[i]);
if (suffix_len <= len
-   && !memcmp(string, suffix[i], suffix_len))
+   && !strncasecmp(string, 

[PATCH v2 3/3] rev-parse: match ^{} case-insensitively

[Ævar Arnfjörð Bjarmason]

Change the revision parsing logic to match ^{commit}, ^{tree}, ^{blob}
etc. case-insensitively.

Before this change supplying anything except the lower-case forms
emits an "unknown revision or path not in the working tree"
error. This change makes upper-case & mixed-case versions equivalent
to the lower-case versions.

The rationale for this change is the same as for making @{upstream}
and related suffixes case-insensitive in "rev-parse: match
@{upstream}, @{u} and @{push} case-insensitively", but unlike those
suffixes this change introduces the potential confusion of accepting
TREE or BLOB here, but not as an argument to e.g. "cat-file -t "
or "hash-object -t ".

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/revisions.txt |  5 +
 git-compat-util.h   |  1 +
 sha1_name.c | 10 +-
 strbuf.c|  9 +
 t/t1450-fsck.sh |  7 +++
 t/t1511-rev-parse-caret.sh  | 13 +
 6 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 5fe90e411d..136e26c05d 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -162,6 +162,11 @@ it does not have to be dereferenced even once to get to an 
object.
 +
 'rev{caret}\{tag\}' can be used to ensure that 'rev' identifies an
 existing tag object.
++
+The {caret}{} part is matched case-insensitively. So
+e.g. '{caret}\{commit\}' can be equivalently specified as
+'{caret}\{COMMIT\}', '{caret}\{Commit\}' etc., '{caret}\{tree\}' as
+'{caret}\{TREE\}' and so forth.
 
 '{caret}{}', e.g. 'v0.99.8{caret}{}'::
   A suffix '{caret}' followed by an empty brace pair
diff --git a/git-compat-util.h b/git-compat-util.h
index 8a4a3f85e7..4a03934ef3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -448,6 +448,7 @@ extern void set_die_is_recursing_routine(int 
(*routine)(void));
 extern void set_error_handle(FILE *);
 
 extern int starts_with(const char *str, const char *prefix);
+extern int starts_with_icase(const char *str, const char *prefix);
 
 /*
  * If the string "str" begins with the string found in "prefix", return 1.
diff --git a/sha1_name.c b/sha1_name.c
index 2deb9bfdf6..107246bd2d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -821,15 +821,15 @@ static int peel_onion(const char *name, int len, unsigned 
char *sha1,
return -1;
 
sp++; /* beginning of type name, or closing brace for empty */
-   if (starts_with(sp, "commit}"))
+   if (starts_with_icase(sp, "commit}"))
expected_type = OBJ_COMMIT;
-   else if (starts_with(sp, "tag}"))
+   else if (starts_with_icase(sp, "tag}"))
expected_type = OBJ_TAG;
-   else if (starts_with(sp, "tree}"))
+   else if (starts_with_icase(sp, "tree}"))
expected_type = OBJ_TREE;
-   else if (starts_with(sp, "blob}"))
+   else if (starts_with_icase(sp, "blob}"))
expected_type = OBJ_BLOB;
-   else if (starts_with(sp, "object}"))
+   else if (starts_with_icase(sp, "object}"))
expected_type = OBJ_ANY;
else if (sp[0] == '}')
expected_type = OBJ_NONE;
diff --git a/strbuf.c b/strbuf.c
index ace58e7367..7d4a59bca6 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -11,6 +11,15 @@ int starts_with(const char *str, const char *prefix)
return 0;
 }
 
+int starts_with_icase(const char *str, const char *prefix)
+{
+   for (; ; str++, prefix++)
+   if (!*prefix)
+   return 1;
+   else if (tolower(*str) != tolower(*prefix))
+   return 0;
+}
+
 /*
  * Used as the default ->buf value, so that people can always assume
  * buf is non NULL and ->buf is NUL terminated even for a freshly
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 33a51c9a67..b6c1989671 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -505,6 +505,13 @@ test_expect_success 'fsck notices missing tagged object' '
test_must_fail git -C missing fsck
 '
 
+test_expect_success 'fsck notices missing tagged object with case insensitive 
{blob}' '
+   create_repo_missing tag^{BLOB} &&
+   test_must_fail git -C missing fsck &&
+   create_repo_missing tag^{BloB} &&
+   test_must_fail git -C missing fsck
+'
+
 test_expect_success 'fsck notices ref pointing to missing commit' '
create_repo_missing HEAD &&
test_must_fail git -C missing fsck
diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh
index e0a49a651f..56750f99c6 100755
--- a/t/t1511-rev-parse-caret.sh
+++ b/t/t1511-rev-parse-caret.sh
@@ -48,6 +48,10 @@ test_expect_success 'ref^{commit}' '
git rev-parse ref >expected &&
git rev-parse ref^{commit} >actual &&
test_cmp expected actual &&
+   git rev-parse ref^{COMMIT} >actual &&
+   test_cmp expected actual &&
+   git rev-parse ref^{CoMMiT} >actual &&
+   

[PATCH v2 0/3] rev-parse case insensitivity & @{p} synonym

[Ævar Arnfjörð Bjarmason]

This v2 addresses the feedback on my "rev-parse: match @{u}, @{push}
and ^{} case-insensitively" patch. I've split this into a 3
patch series:

Ævar Arnfjörð Bjarmason (3):
  rev-parse: match @{upstream}, @{u} and @{push} case-insensitively

Reworded the documentation as Junio suggested in
.

  rev-parse: add @{p} as a synonym for @{push}

While I'm at it why don't we have a shorthand for @{push} like
@{upstream}? Add it as @{p}.

  rev-parse: match ^{} case-insensitively

Junio didn't want ^{} case-insensitive "for now", so I split it
out of the first patch.

I'm not overly excited about it, neither is Junio, so this'll probably
be dropped, but I wanted to submit it as a standalone patch in case
anyone wanted to pick this up in the future.

 Documentation/revisions.txt   | 15 ---
 git-compat-util.h |  1 +
 sha1_name.c   | 14 +++---
 strbuf.c  |  9 +
 t/t1450-fsck.sh   |  7 +++
 t/t1507-rev-parse-upstream.sh | 15 +++
 t/t1511-rev-parse-caret.sh| 13 +
 t/t1514-rev-parse-push.sh | 10 --
 8 files changed, 68 insertions(+), 16 deletions(-)

-- 
2.11.0

[PATCH v2 2/3] rev-parse: add @{p} as a synonym for @{push}

[Ævar Arnfjörð Bjarmason]

Add @{p} as a shorthand for @{push} for consistency with the @{u}
shorthand for @{upstream}.

This wasn't added when @{push} was introduced in commit
adfe5d0434 ("sha1_name: implement @{push} shorthand", 2015-05-21), but
it can be added without any ambiguity and saves the user some typing.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/revisions.txt | 8 
 sha1_name.c | 2 +-
 t/t1514-rev-parse-push.sh   | 2 ++
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 09e0d51b9e..5fe90e411d 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -99,8 +99,8 @@ some output processing may assume ref names in UTF-8.
   current one. These suffixes are accepted when spelled in uppercase, and
   they mean the same thing no matter the case.
 
-'@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
-  The suffix '@\{push}' reports the branch "where we would push to" if
+'@\{push\}', e.g. 'master@\{push\}', '@\{p\}'::
+  The suffix '@\{push}' (short form '@\{push}') reports the branch "where we 
would push to" if
   `git push` were run while `branchname` was checked out (or the current
   `HEAD` if no branchname is specified). Since our push destination is
   in a remote repository, of course, we report the local tracking branch
@@ -124,8 +124,8 @@ Note in the example that we set up a triangular workflow, 
where we pull
 from one location and push to another. In a non-triangular workflow,
 '@\{push}' is the same as '@\{upstream}', and there is no need for it.
 +
-This suffix is accepted when spelled in uppercase, and means the same
-thing no matter the case.
+These suffixes are accepted when spelled in uppercase, and they mean
+the same thing no matter the case.
 
 '{caret}', e.g. 'HEAD{caret}, v1.5.1{caret}0'::
   A suffix '{caret}' to a revision parameter means the first parent of
diff --git a/sha1_name.c b/sha1_name.c
index d9d1b2fce8..2deb9bfdf6 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -563,7 +563,7 @@ static inline int upstream_mark(const char *string, int len)
 
 static inline int push_mark(const char *string, int len)
 {
-   const char *suffix[] = { "@{push}" };
+   const char *suffix[] = { "@{push}", "@{p}" };
return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
 }
 
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index 788cc91e45..db9aaf88f8 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -31,6 +31,8 @@ test_expect_success '@{push} with default=nothing' '
 
 test_expect_success '@{push} with default=simple' '
test_config push.default simple &&
+   resolve master@{p} refs/remotes/origin/master &&
+   resolve master@{P} refs/remotes/origin/master &&
resolve master@{push} refs/remotes/origin/master &&
resolve master@{PUSH} refs/remotes/origin/master &&
resolve master@{pUSh} refs/remotes/origin/master
-- 
2.11.0

[PATCH] git-p4: Add failing test case for name-rev vs symbolic-ref

[Luke Diamand]

As per the discussion about use of "git name-rev" vs "git symbolic-ref" in
git-p4 here:

http://marc.info/?l=git=148979063421355

git-p4 could get confused about the branch it is on and affects
the git-p4.allowSubmit  option. This adds a failing
test case for the problem.

Luke Diamand (1):
  git-p4: add failing test for name-rev rather than symbolic-ref

 t/t9807-git-p4-submit.sh | 16 
 1 file changed, 16 insertions(+)

-- 
2.8.2.703.g78b384c.dirty

[PATCH] git-p4: add failing test for name-rev rather than symbolic-ref

[Luke Diamand]

Using name-rev to find the current git branch means that git-p4
does not correctly get the current branch name if there are
multiple branches pointing at HEAD, or a tag.

This change adds a test case which demonstrates the problem.
Configuring which branches are allowed to be submitted from goes
wrong, as git-p4 gets confused about which branch is in use.

This appears to be the only place that git-p4 actually cares
about the current branch.

Signed-off-by: Luke Diamand 
---
 t/t9807-git-p4-submit.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index e37239e..ae05816 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -139,6 +139,22 @@ test_expect_success 'submit with master branch name from 
argv' '
)
 '
 
+test_expect_failure 'allow submit from branch with same revision but different 
name' '
+   test_when_finished cleanup_git &&
+   git p4 clone --dest="$git" //depot &&
+   (
+   cd "$git" &&
+   test_commit "file8" &&
+   git checkout -b branch1 &&
+   git checkout -b branch2 &&
+   git config git-p4.skipSubmitEdit true &&
+   git config git-p4.allowSubmit "branch1" &&
+   test_must_fail git p4 submit &&
+   git checkout branch1 &&
+   git p4 submit
+   )
+'
+
 #
 # Basic submit tests, the five handled cases
 #
-- 
2.8.2.703.g78b384c.dirty

Re: [PATCH v3 2/4] refs: introduce get_worktree_ref_store()

[Duy Nguyen]

On Mon, Mar 20, 2017 at 9:25 PM, Michael Haggerty  wrote:
> Instead of moving all of the `for_each_*_submodule()` functions over, I
> encourage you to consider getting rid of them entirely and let the
> end-users call the `refs_for_each_*()` versions of the functions. Again,
> I'm not sure that there won't be friction in doing so, but it seems like
> it's worth a try.

They are getting rid of. If you look at pu (or nd/prune-in-worktree
actually) there's only head_ref_submodule() and
for_each_remote_ref_submodule() left. head_ref_submodule() has no
caller but is still there, I'll need to kill it. Killing the latter
can be done separately since the callers in submodule.c need some
update.
-- 
Duy

[PATCH] contrib/subtree: add "--no-commit" flag for merge and pull

[Mike Lewis]

Allows the user to verify and/or change the contents of the merge
before committing as necessary

Signed-off-by: Mike Lewis 
---
 contrib/subtree/git-subtree.sh | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index dec085a23..c30087485 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -29,6 +29,8 @@ onto= try connecting new tree to an existing one
 rejoinmerge the new branch back into HEAD
  options for 'add', 'merge', and 'pull'
 squashmerge subtree changes as a single commit
+ options for 'merge' and 'pull'
+no-commit perform the merge, but don't commit
 "
 eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
 
@@ -48,6 +50,7 @@ annotate=
 squash=
 message=
 prefix=
+commit_option="--commit"
 
 debug () {
if test -n "$debug"
@@ -137,6 +140,12 @@ do
--no-squash)
squash=
;;
+   --no-commit)
+   commit_option="--no-commit"
+   ;;
+   --no-no-commit)
+   commit_option="--commit"
+   ;;
--)
break
;;
@@ -815,17 +824,17 @@ cmd_merge () {
then
if test -n "$message"
then
-   git merge -s subtree --message="$message" "$rev"
+   git merge -s subtree --message="$message" 
"$commit_option" "$rev"
else
-   git merge -s subtree "$rev"
+   git merge -s subtree "$commit_option" "$rev"
fi
else
if test -n "$message"
then
git merge -Xsubtree="$prefix" \
-   --message="$message" "$rev"
+   --message="$message" "$commit_option" "$rev"
else
-   git merge -Xsubtree="$prefix" $rev
+   git merge -Xsubtree="$prefix" "$commit_option" $rev
fi
fi
 }
-- 
2.12.2

Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag

[Jeff King]

On Fri, Mar 24, 2017 at 11:37:54PM -0700, Junio C Hamano wrote:

> The hash that names a packfile is constructed by sorting all the
> names of the objects contained in the packfile and running SHA-1
> hash over it.  I think this MUST be hashed with collision-attack
> detection.  A malicious site can feed you a packfile that contains
> objects the site crafts so that the sorted object names would result
> in a collision-attack, ending up with one pack that contains a sets
> of objects different from another pack that happens to have the same
> packname, causing Git to say "Ah, this new pack must have the same
> set of objects as the pack we already have" and discard it,
> resulting in lost objects and a corrupt repository with missing
> objects.

I don't think this case really matters for collision detection. What's
important is what Git does when it receives a brand-new packfile that
would overwrite an existing one. It _should_ keep the old one, under the
usual "existing data wins" rule.

It should be easy to test, though:

  $ git init tmp && cd tmp
  $ git commit --allow-empty -m foo
  $ git gc

  $ touch -d yesterday .git/objects/pack/*
  $ ls -l .git/objects/pack
  -r--r--r-- 1 peff peff 1128 Mar 25 02:10 
pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.idx
  -r--r--r-- 1 peff peff  153 Mar 25 02:10 
pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.pack

  $ git index-pack --stdin <.git/objects/pack/*.pack
  pack  7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b

  $ ls -l .git/objects/pack
  -r--r--r-- 1 peff peff 1128 Mar 25 02:10 
pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.idx
  -r--r--r-- 1 peff peff  153 Mar 25 02:10 
pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.pack

Looks like the timestamps were retained.  And if we use strace, we
can see what happens:

  $ strace git index-pac k--stdin <.git/objects/pack/*.pack
  link(".git/objects/pack/tmp_pack_YSrdWU", 
".git/objects/pack/pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.pack") = -1 
EEXIST (File exists)
  unlink(".git/objects/pack/tmp_pack_YSrdWU") = 0
  link(".git/objects/pack/tmp_idx_O94NNU", 
".git/objects/pack/pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.idx") = -1 
EEXIST (File exists)
  unlink(".git/objects/pack/tmp_idx_O94NNU") = 0

This is due to the link()/EEXIST handling in finalize_object_file. It
has a FIXME for a collision check, so we could actually detect at that
point whether we have a real collision, or if the other side just
happened to send us the same pack.

I wouldn't be surprised if the dumb-http walker is not so careful,
though (but the solution is to make it careful, not to worry about
a weak hash algorithm).

The rest of your email all made sense to me.

-Peff