Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"

[Siddharth Kannan]

On 21 February 2017 at 02:00, Junio C Hamano  wrote:
> Siddharth Kannan  writes:
> > So, is it okay to stop with just supporting "-" and not support things
> > like "-@{yesterday}"?
>
> If the approach to turn "-" into "@{-1}" at that spot you did will
> cause "-@{yesterday}" to barf, then I'd say so be it for now ;-).
> We can later spread the understanding of "-" to functions deeper in
> the callchain and add support for that, no?

Yes, this can be done later. I will send these patches again, with
only the changes that are discussed here.

I will keep the tests for "-@{yesterday}" as failing tests, if that
would help in finding this again and fixing it later.

Thanks for your review, Junio!

-- 

Best Regards,

- Siddharth Kannan.

Re: url..insteadOf vs. submodules

[Junio C Hamano]

Stefan Beller  writes:

>> This is true even without any submodules.  The Git project itself
>> does not even care you are Stefan, but you still can and do add
>> [user] name = "Stefan Beller" to .git/config of your clone of the
>> Git project.  A clone of the project may want to know more than the
>> data project itself keeps track of to describe the context in which
>> the particular clone is being used.  And .git/config is a good place
>> to keep such pieces of information.
>
> This analogy is less clear to me than the kernel& appliance.
> When applying it to you (user.name=Junio) that has write powers
> over the blessed repository, the project cares a lot about you. ;)

The name that is recorded in the project history "Stefan Beller"
matters and the project cares about it, when the commit created in
that repository is pulled (or exported and imported via the e-mail
to "git am" route).  But what name you have configured in your
repository's .git/config, or the presense of your particular
repository for that matter, is much less significant (and that
applies to my primary working area as well).  The point is that the
project and a particular clone of it are entities at conceptually
different levels.

>> So I would think it is entirely reasonable if "git submodule init
>> sub" that is run in the superproject to initialize "sub" writes
>> something in "sub/.git" to tell that "sub" is used in the context of
>> that particular toplevel superproject and customize its behavour
>> accordingly.  Perhaps it may want to add the url.*.insteadOf that is
>> useful for updating the submodule repository when it does "submodule
>> init", for example.
>
> Do we want to invent a special value for url.*.insteadOf to mean
>   "look up in superproject, so I don't have to keep
>   a copy that may get stale" ?

My gut feeling is that we should do the selective/filtered include
Peff mentioned when a repository is known to be used as a submodule
of somebody else.

[PATCH] git svn branch fails with authenticaton failures

[Hiroshi Shirosaki]

I have the following authentication failure while svn rebase and
svn dcommit works fine without authentication failures.

$ git svn branch v7_3
Copying https://xxx at r27519
to https:///v7_3...
Can't create session: Unable to connect to a repository at URL
'https://': No more
credentials or we tried too many times.
Authentication failed at
C:\Program Files\Git\mingw64/libexec/git-core\git-svn line 1200.

I can workaround the issue to add auth configuration to
SVN::Client->new().
---
 git-svn.perl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index fa42364..13fa4ad 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1175,10 +1175,10 @@ sub cmd_branch {
::_req_svn();
require SVN::Client;
 
+   my ($config, $baton, $callbacks) = Git::SVN::Ra::prepare_config_once();
my $ctx = SVN::Client->new(
-   config => SVN::Core::config_get_config(
-   $Git::SVN::Ra::config_dir
-   ),
+   auth => $baton,
+   config => $config,
log_msg => sub {
${ $_[0] } = defined $_message
? $_message
-- 
2.7.4

Re: [PATCH] Documentation: Link git-ls-files to core.quotePath variable.

[Andreas Heiduk]

[PATCH] Documentation: Clarify core.quotePath, remove cruft in
 git-ls-files.

Signed-off-by: Andreas Heiduk 
---

I have merged the best parts about quoting into the core.quotePath
description and cleaned up the text in git-ls-files.txt regarding the
control characters.


 Documentation/config.txt   | 22 --
 Documentation/git-ls-files.txt | 11 ++-
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f4721a0..25e65ae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -340,16 +340,18 @@ core.checkStat::
all fields, including the sub-second part of mtime and ctime.

 core.quotePath::
-   The commands that output paths (e.g. 'ls-files',
-   'diff'), when not given the `-z` option, will quote
-   "unusual" characters in the pathname by enclosing the
-   pathname in a double-quote pair and with backslashes the
-   same way strings in C source code are quoted.  If this
-   variable is set to false, the bytes higher than 0x80 are
-   not quoted but output as verbatim.  Note that double
-   quote, backslash and control characters are always
-   quoted without `-z` regardless of the setting of this
-   variable.
+   Commands that output paths (e.g. 'ls-files', 'diff'), will
+   quote "unusual" characters in the pathname by enclosing the
+   pathname in double-quotes and escaping those characters with
+   backslashes in the same way C escapes control characters (e.g.
+   `\t` for TAB, `\n` for LF, `\\` for backslash) or bytes with
+   values larger than 0x80 (e.g. octal `\265` for "micro").  If
+   this variable is set to false, bytes higher than 0x80 are not
+   considered "unusual" any more.  Double-quotes, backslash and
+   control characters are always escaped regardless of the
+   setting of this variable.  Many commands can output pathnames
+   completely verbatim using the `-z` option. The default value is
+   true.

 core.eol::
Sets the line ending type to use in the working directory for
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index d2b17f2..88df561 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -76,7 +76,8 @@ OPTIONS
succeed.

 -z::
-   \0 line termination on output.
+   \0 line termination on output and do not quote filenames.
+   See OUTPUT below for more information.

 -x ::
 --exclude=::
@@ -192,10 +193,10 @@ the index records up to three such pairs; one from
tree O in stage
 the user (or the porcelain) to see what should eventually be recorded
at the
 path. (see linkgit:git-read-tree[1] for more information on state)

-When `-z` option is not used, TAB, LF, and backslash characters
-in pathnames are represented as `\t`, `\n`, and `\\`,
-respectively. The path is also quoted according to the
-configuration variable `core.quotePath` (see linkgit:git-config[1]).
+Without the `-z` option pathnamens with "unusual" characters are
+quoted as explained for the configuration variable `core.quotePath`
+(see linkgit:git-config[1]).  Using `-z` the filename is output
+verbatim and the line is terminated by a NUL byte.


 Exclude Patterns
-- 
2.7.4

Re: [PATCH] Documentation: Link git-ls-files to core.quotePath variable.

[Andreas Heiduk]

Am 21.02.2017 um 21:48 schrieb Junio C Hamano:
> 
> I was waiting for others to comment on this patch but nobody seems
> to be interested.  Which is a bit sad, as this may not be a bad
> idea.
> 
> If we refer to core.quotePath, the mention of control characters
> being quoted can also be omitted, I think, as that is part of what
> appears in the description of core.quotePath variable.
> 
> Alternatively, instead of referring to another page, we can spend
> the additional lines to say what is more interesting to most of the
> readers from that page, e.g.
> 
> When `-z` option is not used, a pathname with "unusual" characters
> in it is quoted by enclosing it in a double-quote pair and with
> backslashes the same way strings in C source code are quoted.  By
> setting core.quotePath configuration to false, the bytes whose
> values are higher than 0x80 are output verbatim.
>

Without `-z` but with core.quotePath=false the path may still be
surrounded with double-quotes if it contains control characters (and
some more). The documentation in `core.quotePath` mentions this, your
"inline" alternative does not.

I will send second patch. :-)

Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

Junio C Hamano  writes:

>   /* find the last dot (we start from the first dot we just found) */
> - for (last_dot = cp; *cp; cp++)
> + for (; *cp; cp++)
>   if (*cp == '.')
>   last_dot = cp;

This line probably needs this fix-up on top.

-- >8 --
Subject: [PATCH] config: squelch stupid compiler warnings

Some compilers do not realize that *cp is always '.' when the loop
to find the last dot begins, and instead gives a useless warning
that says last_dot may be uninitialized.

Squelch it by being a bit more explicit if stupid.

Signed-off-by: Junio C Hamano 
---
 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.c b/config.c
index e7f7ff1938..90de27853f 100644
--- a/config.c
+++ b/config.c
@@ -239,7 +239,7 @@ static int canonicalize_config_variable_name(char *varname)
return -1; /* no section? */
 
/* find the last dot (we start from the first dot we just found) */
-   for (; *cp; cp++)
+   for (last_dot = cp; *cp; cp++)
if (*cp == '.')
last_dot = cp;
 
-- 
2.12.0-rc2-231-g83a1c8597c

Re: Git trademark status and policy

[G. Sylvie Davies]

On Tue, Feb 21, 2017 at 7:55 AM, G. Sylvie Davies
 wrote:
> On Wed, Feb 1, 2017 at 6:26 PM, Jeff King  wrote:
>> As many of you already know, the Git project (as a member of Software
>> Freedom Conservancy) holds a trademark on "Git".  This email will try to
>> lay out a bit of the history and procedure around the enforcement of
>> that trademark, along with some open questions about policy.
>>
>> I'll use "we" in the text below, which will generally mean the Git
>> Project Leadership Committee (PLC). I.e., the people who represent the
>> Git project as part of Conservancy -- me, Junio Hamano, and Shawn
>> Pearce.
>>
>> We approached Conservancy in Feb 2013 about getting a trademark on Git
>> to ensure that anything calling itself "Git" remained interoperable with
>> Git. Conservancy's lawyer drafted the USPTO application and submitted it
>> that summer. The trademark was granted in late 2014 (more on that delay
>> in a moment).
>>
>> Concurrently, we developed a written trademark policy, which you can
>> find here:
>>
>>   https://git-scm.com/trademark
>>
>> This was started from a template that Conservancy uses and customized by
>> Conservancy and the Git PLC.
>>
>> While the original idea was to prevent people from forking the
>> software, breaking compatibility, and still calling it Git, the policy
>> covers several other cases.
>>
>> One is that you can't imply successorship. So you also can't fork the
>> software, call it "Git++", and then tell everybody your implementation
>> is the next big thing.
>>
>> Another is that you can't use the mark in a way that implies association
>> with or endorsement by the Git project. To some degree this is necessary
>> to prevent dilution of the mark for other uses, but there are also cases
>> we directly want to prevent.
>>
>> For example, imagine a software project which is only tangentially
>> related to Git. It might use Git as a side effect, or might just be
>> "Git-like" in the sense of being a distributed system with chained
>> hashes. Let's say as an example that it does backups. We'd prefer it
>> not call itself GitBackups. We don't endorse it, and it's just using the
>> name to imply association that isn't there. You can come up with similar
>> hypotheticals: GitMail that stores mailing list archives in Git, or
>> GitWiki that uses Git as a backing store.
>>
>> Those are all fictitious examples (actually, there _are_ real projects
>> that do each of those things, but they gave themselves much more unique
>> names). But they're indicative of some of the cases we've seen. I'm
>> intentionally not giving the real names here, because my point isn't to
>> shame any particular projects, but to discuss general policy.
>>
>> Careful readers among you may now be wondering about GitHub, GitLab,
>> Gitolite, etc. And now we get back to why it took over a year to get the
>> trademark granted.
>>
>> The USPTO initially rejected our application as confusingly similar to
>> the existing trademark on GitHub, which was filed in 2008. While one
>> might imagine where the "Git" in GitHub comes from, by the time we
>> applied to the USPTO, both marks had been widely used in parallel for
>> years.  So we worked out an agreement with GitHub which basically says
>> "we are mutually OK with the other trademark existing".
>>
>> (There was another delay caused by a competing application from a
>> proprietary version control company that wanted to re-brand portions of
>> their system as "GitFocused" (not the real name, but similar in spirit).
>> We argued our right to the name and refused to settle; they eventually
>> withdrew their application).
>>
>> So GitHub is essentially outside the scope of the trademark policy, due
>> to the history. We also decided to explicitly grandfather some major
>> projects that were using similar portmanteaus, but which had generally
>> been good citizens of the Git ecosystem (building on Git in a useful
>> way, not breaking compatibility). Those include GitLab, JGit, libgit2,
>> and some others. The reasoning was generally that it would be a big pain
>> for those projects, which have established their own brands, to have to
>> switch names. It's hard to hold them responsible for picking a name that
>> violated a policy that didn't yet exist.
>>
>> If the "libgit2" project were starting from scratch today, we'd probably
>> ask it to use a different name (because the name may imply that it's an
>> official successor). However, we effectively granted permission for this
>> use and it would be unfair to disrupt that.
>>
>> There's one other policy point that has come up: the written policy
>> disallows the use of "Git" or the logo on merchandise. This is something
>> people have asked about it (e.g., somebody made some Git stress balls,
>> and another person was printing keycaps with a Git logo). We have always
>> granted it, but wanted to reserve the right in case there was some use
>> that we hadn't anticipated 

Fwd: Typo in worktree man page

[Casey Rodarmor]

Hi there,

The git worktree man page mentions the `--detached` flag in the
section on `add`, but the flag is actually called `--detach`.

Best,
Casey

Hello

[university]

Re: [PATCH 12/15] unpack-trees: check if we can perform the operation for submodules

[Jacob Keller]

On Tue, Feb 21, 2017 at 3:44 PM, Stefan Beller  wrote:
> On Tue, Feb 21, 2017 at 3:35 PM, Jacob Keller  wrote:
>> On Tue, Feb 21, 2017 at 2:16 PM, Stefan Beller  wrote:
>>> On Fri, Feb 17, 2017 at 10:42 AM, Jacob Keller  
>>> wrote:
 On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller  wrote:
> +   if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED))

 Here, and in other cases where we use
 is_active_submodule_with_strategy(), why do we only ever check
 SM_UPDATE_UNSPECIFIED? It seems really weird that we're only going to
 check submodules who's strategy is unspecified, when that defaults to
 checkout if I recall correctly? Shouldn't we check both? This applies
 to pretty much everywhere that you call this function that I noticed,
 which is why I removed the context.
>>>
>>> I am torn between this.
>>>
>>> submodule..update = {rebase, merge, checkout, none !command}
>>> is currently documented in GIT-CONFIG(1) as
>>>
>>>submodule..update
>>>The default update procedure for a submodule. This variable is
>>>populated by git submodule init from the gitmodules(5) file. See
>>>description of update command in git-submodule(1).
>>>
>>> and in GIT-SUBMODULE(1) as
>>>
>>>update
>>>[...] can be done in several ways
>>>depending on command line options and the value of
>>>submodule..update configuration variable. Supported update
>>>procedures are:
>>>
>>>checkout
>>>[...] or no option is given, and
>>>submodule..update is unset, or if it is set to 
>>> checkout.
>>>
>>> So the "update" config clearly only applies to the "submodule update"
>>> command, right?
>>>
>>> Well no, "checkout --recurse-submodules" is very similar
>>> to running "submodule update", except with a bit more checks, so you could
>>> think that such an option applies to checkout as well. (and eventually
>>> rebase/merge etc. are supported as well.)
>>>
>>> So initially I assumed both "unspecified" as well as "checkout"
>>> are good matches to support in the first round.
>>>
>>> Then I flip flopped to think that we should not interfere with these
>>> settings at all (The checkout command does checkout and checkout only;
>>> no implicit rebase/merge ever in the future, because that would be
>>> confusing). So ignoring that option seemed like the way to go.
>>
>> Hmm. So it's a bit complicated.
>>
>>>
>>> But ignoring that option is also not the right approach.
>>> What if you have set it to "none" and really *expect* Git to not touch
>>> that submodule?
>>
>> Or set it to "rebase" and suddenly git-checkout is ignoring you and
>> just checking things out anyways.
>>
>>>
>>> So I dunno. Maybe it is a documentation issue, we need to spell out
>>> in the man page for checkout that --recurse-submodules is
>>> following one of these models. Now which is the best default model here?
>>
>> Personally, I would go with that the config option sets the general
>> strategy used by the submodule whenever its updated, regardless of
>> how.
>>
>> So, for example, setting it to none, means that recurse-submoduls will
>> ignore it when checking out. Setting it to rebase, or merge, and the
>> checkout will try to do those things?
>
> That is generally a sound idea when it comes to git-checkout.
>
> What about other future things like git-revert?
> (Ok I already brought up this example too many times; it should have
> a revert-submodules as well switch, which is neither of the current 
> strategies,
> so we'd have to invent a new strategy and make that the default for
> revert. That strategy would make no sense in any other command though)
>

This is where things get tricky, IMHO. The problem is that the
strategy now wants to encompass more things.

> What about "git-rebase --recurse-submodules"?
> Should git-rebase merge the submodules when it is configured to "merge"
> Or just "checkout" (the possibly non-fast-forward-y old sha1) ?
>
> The only sane option IMO is "rebase" as well in the submodules, rewriting
> the submodule pointers in the rebased commits in the superproject.
>

I'm not even really sure what rebase should do here at all. I assume
by this you mean "what would a git-rebase that also rebased submodules
do"

Ofcourse the sane answer might be something like "uhh you have to
decide that for yourself manually" I think this is a really complex
problem to solve, and in this case I do not think rebase should even
rely on the strategy. a "recurse-submodules rebase" would do something
like:

rebase parent as normal, but if a commit changes the submodule, then
it needs to re-create that submodule change using its own rebase
inside  the submodule based on the (new) parent from the parent
projects history change, and then commit that as the committed change?

But 

Re: url..insteadOf vs. submodules

[Stefan Beller]

On Tue, Feb 21, 2017 at 3:40 PM, Jeff King  wrote:

>> > One other caveat: I'm not sure if we do insteadOf recursively, but it
>> > may be surprising to the child "git clone" that we've already applied
>> > the insteadOf rewriting (especially if the rules are coming from
>> > ~/.gitconfig and may be applied twice).
>>
>> When a rule is having effect twice the rule sounds broken. (the outcome
>> ought to be sufficiently different from the original?)
>
> If you have:
>
>   url.bar.insteadOf=foo
>   url.baz.insteadOf=bar
>
> do we convert "foo" to "baz"? If so, then I think applying the rules
> again shouldn't matter. But if we don't, and only do a single level,
> then having the caller rewrite the URL before it hands it to "git clone"
> means we may end up unexpectedly doing two levels of rewriting.
>

I see. Thanks for the example. So really what we want is to record the
unencumbered URL (with no rewriting) and then at run time lookup various
places of url.*.insteadOf (which might change with the git version
that you use)

Thanks,
Stefan

Re: url..insteadOf vs. submodules

[Stefan Beller]

On Tue, Feb 21, 2017 at 3:37 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> On Tue, Feb 21, 2017 at 3:00 PM, Jeff King  wrote:
>> ...
>>> I guess one answer is that this is the wrong approach entirely, and the
>>> right one is something like: submodules should understand that they are
>>> part of a superproject, and respect some whitelisted set of config from
>>> the superproject .git/config file.
>>
>> This would break one of the core assumptions that submodules
>> are "independent" repos.
>>
>> The way of action is a one way street:
>> * The superproject is aware of the submodule and when you invoke a
>> command on the superproject, you may mess around with the submodule,
>> e.g. update/remove it; absorb its git directory.
>> * The submodule is "just" a repository with weird .git link file and a
>>   respective core.worktree setup. Currently it doesn't know if it is
>>   guided by a superproject.
>
> While that is a good discipline to follow, I think you need to
> differenciate the project that is bound as a submodule to a
> superproject, and a specific instance of a submodule repository,
> i.e. a clone of such a project.
>
> It is true that the Linux kernel project should *NEVER* know your
> appliance project only because you happen to use it as a component
> of your appliance that happens to use the kernel as one of its
> submodules.  But that does not mean your copy of the kernel that
> sits in your recursive checkout of your appliance project should
> not know anything about your superproject.

Oh, I see.  For this use case as well as the prompt indicator that
I mentioned in the previous email, the most basic question is
* Do we have a superproject? [yes/no]
The next level of awareness would be
* Where is the superproject? [ ]

These questions may not be interesting for a user (they ought to know
about that appliance;) ), but rather for scripted usage, which I think
hints at the lack of a submodule plumbing command.

Currently we only have git-submodule that is a helper used to somehow
cope with submodules. It is used by humans directly and it is listed
under "Main porcelain commands" in our man page.

Probably we'd also do not want to cram this stuff into the already bloated
rev-parse (that has --show-toplevel, which has nothing to do with
parsing revs, but as Jeff put it it is the kitchen sink of Git).

>
> This is true even without any submodules.  The Git project itself
> does not even care you are Stefan, but you still can and do add
> [user] name = "Stefan Beller" to .git/config of your clone of the
> Git project.  A clone of the project may want to know more than the
> data project itself keeps track of to describe the context in which
> the particular clone is being used.  And .git/config is a good place
> to keep such pieces of information.

This analogy is less clear to me than the kernel& appliance.
When applying it to you (user.name=Junio) that has write powers
over the blessed repository, the project cares a lot about you. ;)

> So I would think it is entirely reasonable if "git submodule init
> sub" that is run in the superproject to initialize "sub" writes
> something in "sub/.git" to tell that "sub" is used in the context of
> that particular toplevel superproject and customize its behavour
> accordingly.  Perhaps it may want to add the url.*.insteadOf that is
> useful for updating the submodule repository when it does "submodule
> init", for example.

Do we want to invent a special value for url.*.insteadOf to mean
  "look up in superproject, so I don't have to keep
  a copy that may get stale" ?

Re: url..insteadOf vs. submodules

[Junio C Hamano]

Junio C Hamano  writes:

> So I would think it is entirely reasonable if "git submodule init
> sub" that is run in the superproject to initialize "sub" writes
> something in "sub/.git" to tell that "sub" is used in the context of
> that particular toplevel superproject and customize its behavour
> accordingly.  Perhaps it may want to add the url.*.insteadOf that is
> useful for updating the submodule repository when it does "submodule
> init", for example.

Of course, "copying" is usually not very desirable, as it invites
one of the copies to go stale.  An actual implementation may just
say "the name of submodule the superproject uses this as is 'foo'".

That way, if such a configuration exists, Git can first do cd-up to
the root of the working tree, go one level up, verify that it is in
a worktree of its superproject, verify that the root of the working
tree it came from was indeed bound to the submodule called 'foo' and
then do the selective/filtered "config-include" Peff outlined.  That
would allow superproject to move submodules around (as opposed to
recording "this submodule is used at this/path of the superproject"
or "the superproject of this submodule is at ../../that/path"), and
does not penalize repositories that are not used as submodules of
any superproject (because the "cd-up, up, verify and include" won't
be done for them).  As opposed to "I am used as a submodule" bit,
recording the name the superproject uses to call the submodule would
also serve as a sanity check measure.

[PATCH v5 05/19] builtin/fast-export: convert to struct object_id

[brian m. carlson]

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

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

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

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

[brian m. carlson]

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

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

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

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

[PATCH v5 08/19] builtin/branch: convert to struct object_id

[brian m. carlson]

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

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

[PATCH v5 12/19] builtin/replace: convert to struct object_id

[brian m. carlson]

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

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

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

[PATCH v5 13/19] reflog-walk: convert struct reflog_info to struct object_id

[brian m. carlson]

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

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

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

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

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

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

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

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

[brian m. carlson]

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

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

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

[PATCH v5 11/19] Convert remaining callers of resolve_refdup to object_id

[brian m. carlson]

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

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

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

[PATCH v5 10/19] builtin/merge: convert to struct object_id

[brian m. carlson]

Additionally convert several uses of the constant 40 into
GIT_SHA1_HEXSZ.

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

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

[PATCH v5 14/19] refs: convert each_reflog_ent_fn to struct object_id

[brian m. carlson]

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

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

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

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

[brian m. carlson]

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

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

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

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

[brian m. carlson]

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

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

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

[PATCH v5 06/19] builtin/fmt-merge-message: convert to struct object_id

[brian m. carlson]

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

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

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

[PATCH v5 04/19] builtin/describe: convert to struct object_id

[brian m. carlson]

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

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

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

[PATCH v5 03/19] builtin/diff-tree: convert to struct object_id

[brian m. carlson]

Convert most leaf functions to struct object_id.  Change several
hardcoded numbers to uses of parse_oid_hex.  In doing so, verify that we
when we want two trees, we have exactly two trees.

Finally, in stdin_diff_commit, avoid accessing the byte after the NUL.
This will be a NUL as well, since the first NUL was a newline we
overwrote.  However, with parse_oid_hex, we no longer need to increment
the pointer directly, and can simply increment it as part of our check
for the space character.

Signed-off-by: brian m. carlson 
Signed-off-by: Jeff King 
---
 builtin/diff-tree.c | 61 ++---
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 8ce00480cd..326f88b657 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -7,46 +7,44 @@
 
 static struct rev_info log_tree_opt;
 
-static int diff_tree_commit_sha1(const unsigned char *sha1)
+static int diff_tree_commit_sha1(const struct object_id *oid)
 {
-   struct commit *commit = lookup_commit_reference(sha1);
+   struct commit *commit = lookup_commit_reference(oid->hash);
if (!commit)
return -1;
return log_tree_commit(_tree_opt, commit);
 }
 
 /* Diff one or more commits. */
-static int stdin_diff_commit(struct commit *commit, char *line, int len)
+static int stdin_diff_commit(struct commit *commit, const char *p)
 {
-   unsigned char sha1[20];
-   if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
-   /* Graft the fake parents locally to the commit */
-   int pos = 41;
-   struct commit_list **pptr;
+   struct object_id oid;
+   struct commit_list **pptr = NULL;
 
-   /* Free the real parent list */
-   free_commit_list(commit->parents);
-   commit->parents = NULL;
-   pptr = &(commit->parents);
-   while (line[pos] && !get_sha1_hex(line + pos, sha1)) {
-   struct commit *parent = lookup_commit(sha1);
-   if (parent) {
-   pptr = _list_insert(parent, pptr)->next;
-   }
-   pos += 41;
+   /* Graft the fake parents locally to the commit */
+   while (isspace(*p++) && !parse_oid_hex(p, , )) {
+   struct commit *parent = lookup_commit(oid.hash);
+   if (!pptr) {
+   /* Free the real parent list */
+   free_commit_list(commit->parents);
+   commit->parents = NULL;
+   pptr = &(commit->parents);
+   }
+   if (parent) {
+   pptr = _list_insert(parent, pptr)->next;
}
}
return log_tree_commit(_tree_opt, commit);
 }
 
 /* Diff two trees. */
-static int stdin_diff_trees(struct tree *tree1, char *line, int len)
+static int stdin_diff_trees(struct tree *tree1, const char *p)
 {
-   unsigned char sha1[20];
+   struct object_id oid;
struct tree *tree2;
-   if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
+   if (!isspace(*p++) || parse_oid_hex(p, , ) || *p)
return error("Need exactly two trees, separated by a space");
-   tree2 = lookup_tree(sha1);
+   tree2 = lookup_tree(oid.hash);
if (!tree2 || parse_tree(tree2))
return -1;
printf("%s %s\n", oid_to_hex(>object.oid),
@@ -60,23 +58,24 @@ static int stdin_diff_trees(struct tree *tree1, char *line, 
int len)
 static int diff_tree_stdin(char *line)
 {
int len = strlen(line);
-   unsigned char sha1[20];
+   struct object_id oid;
struct object *obj;
+   const char *p;
 
if (!len || line[len-1] != '\n')
return -1;
line[len-1] = 0;
-   if (get_sha1_hex(line, sha1))
+   if (parse_oid_hex(line, , ))
return -1;
-   obj = parse_object(sha1);
+   obj = parse_object(oid.hash);
if (!obj)
return -1;
if (obj->type == OBJ_COMMIT)
-   return stdin_diff_commit((struct commit *)obj, line, len);
+   return stdin_diff_commit((struct commit *)obj, p);
if (obj->type == OBJ_TREE)
-   return stdin_diff_trees((struct tree *)obj, line, len);
+   return stdin_diff_trees((struct tree *)obj, p);
error("Object %s is a %s, not a commit or tree",
- sha1_to_hex(sha1), typename(obj->type));
+ oid_to_hex(), typename(obj->type));
return -1;
 }
 
@@ -141,7 +140,7 @@ int cmd_diff_tree(int argc, const char **argv, const char 
*prefix)
break;
case 1:
tree1 = opt->pending.objects[0].item;
-   diff_tree_commit_sha1(tree1->oid.hash);
+   diff_tree_commit_sha1(>oid);
break;
 

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

[brian m. carlson]

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

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

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

[PATCH v5 09/19] builtin/clone: convert to struct object_id

[brian m. carlson]

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

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

[PATCH v5 01/19] hex: introduce parse_oid_hex

[brian m. carlson]

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

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

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

[PATCH v5 02/19] builtin/commit: convert to struct object_id

[brian m. carlson]

Convert most leaf functions to use struct object_id.

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

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

[PATCH v5 07/19] builtin/grep: convert to struct object_id

[brian m. carlson]

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

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

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

[PATCH v5 00/19] object_id part 6

[brian m. carlson]

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

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

I'll be submitting the test I wrote as a separate patch, but I did build
and test with it, so I feel confident that it works properly.

Changes from v4:
* Fix breakage in builtin/diff-tree pointed out by Peff.

Changes from v3:
* Move the parse_oid_hex patch earlier in the series.
* Use parse_oid_hex in builtin/diff-tree.c.
* Fix several warts with parse_oid_hex pointed out by Peff.

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

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

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

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

-- 
2.11.0

Re: [PATCH 12/15] unpack-trees: check if we can perform the operation for submodules

[Stefan Beller]

On Tue, Feb 21, 2017 at 3:35 PM, Jacob Keller  wrote:
> On Tue, Feb 21, 2017 at 2:16 PM, Stefan Beller  wrote:
>> On Fri, Feb 17, 2017 at 10:42 AM, Jacob Keller  
>> wrote:
>>> On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller  wrote:
 +   if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED))
>>>
>>> Here, and in other cases where we use
>>> is_active_submodule_with_strategy(), why do we only ever check
>>> SM_UPDATE_UNSPECIFIED? It seems really weird that we're only going to
>>> check submodules who's strategy is unspecified, when that defaults to
>>> checkout if I recall correctly? Shouldn't we check both? This applies
>>> to pretty much everywhere that you call this function that I noticed,
>>> which is why I removed the context.
>>
>> I am torn between this.
>>
>> submodule..update = {rebase, merge, checkout, none !command}
>> is currently documented in GIT-CONFIG(1) as
>>
>>submodule..update
>>The default update procedure for a submodule. This variable is
>>populated by git submodule init from the gitmodules(5) file. See
>>description of update command in git-submodule(1).
>>
>> and in GIT-SUBMODULE(1) as
>>
>>update
>>[...] can be done in several ways
>>depending on command line options and the value of
>>submodule..update configuration variable. Supported update
>>procedures are:
>>
>>checkout
>>[...] or no option is given, and
>>submodule..update is unset, or if it is set to checkout.
>>
>> So the "update" config clearly only applies to the "submodule update"
>> command, right?
>>
>> Well no, "checkout --recurse-submodules" is very similar
>> to running "submodule update", except with a bit more checks, so you could
>> think that such an option applies to checkout as well. (and eventually
>> rebase/merge etc. are supported as well.)
>>
>> So initially I assumed both "unspecified" as well as "checkout"
>> are good matches to support in the first round.
>>
>> Then I flip flopped to think that we should not interfere with these
>> settings at all (The checkout command does checkout and checkout only;
>> no implicit rebase/merge ever in the future, because that would be
>> confusing). So ignoring that option seemed like the way to go.
>
> Hmm. So it's a bit complicated.
>
>>
>> But ignoring that option is also not the right approach.
>> What if you have set it to "none" and really *expect* Git to not touch
>> that submodule?
>
> Or set it to "rebase" and suddenly git-checkout is ignoring you and
> just checking things out anyways.
>
>>
>> So I dunno. Maybe it is a documentation issue, we need to spell out
>> in the man page for checkout that --recurse-submodules is
>> following one of these models. Now which is the best default model here?
>
> Personally, I would go with that the config option sets the general
> strategy used by the submodule whenever its updated, regardless of
> how.
>
> So, for example, setting it to none, means that recurse-submoduls will
> ignore it when checking out. Setting it to rebase, or merge, and the
> checkout will try to do those things?

That is generally a sound idea when it comes to git-checkout.

What about other future things like git-revert?
(Ok I already brought up this example too many times; it should have
a revert-submodules as well switch, which is neither of the current strategies,
so we'd have to invent a new strategy and make that the default for
revert. That strategy would make no sense in any other command though)

What about "git-rebase --recurse-submodules"?
Should git-rebase merge the submodules when it is configured to "merge"
Or just "checkout" (the possibly non-fast-forward-y old sha1) ?

The only sane option IMO is "rebase" as well in the submodules, rewriting
the submodule pointers in the rebased commits in the superproject.

>
> Or, if that's not really feasible, have the checkout go "hey.. you
> asked me to recurse, but uhhh these submodules don't allow me to do
> checkout, so I'm gonna fail"? I think that's the best approach for
> now.

So you'd propose to generally use the submodule..update
strategies with aggressive error-out but also keeping in mind
that the strategies might grow by a lot in the future (well only revert
comes to mind here).

ok, let's do that then.

Thanks,
Stefan

Re: url..insteadOf vs. submodules

[Jeff King]

On Tue, Feb 21, 2017 at 03:16:27PM -0800, Stefan Beller wrote:

> > I guess one answer is that this is the wrong approach entirely, and the
> > right one is something like: submodules should understand that they are
> > part of a superproject, and respect some whitelisted set of config from
> > the superproject .git/config file.
> 
> This would break one of the core assumptions that submodules
> are "independent" repos.

Yeah, that was the "first half" that I said was hard. :)

You could rationalize it under the fact that they _are_ independent
repos; we're just adding a new config source.  Arguably it could be a
feature for any repository embedded inside the working tree of another,
submodule or not, to consider the outer repository as a (limited) source
of config.

But there are probably a lot of irritating corner cases with the whole
concept unless we apply a strict whitelist of keys (e.g., you probably
don't want remote.* to be propagated). And as the recent
GIT_CONFIG_PARAMETERS whitelist showed, that approach ended up confusing
and annoying.

So maybe the whole thing is insane, and the right answer is that config
values should go into ~/.gitconfig. And we may need better tools there
for limiting that global config to certain parts of the tree (like Duy's
conditional include thing).

> Though I do not know if this is actually a good assumption.
> e.g. "[PATCH v2] git-prompt.sh: add submodule indicator"
> https://public-inbox.org/git/1486075892-20676-2-git-send-email-em...@benjaminfuchs.de/
> really had trouble in the first version to nail down how to tell you are in
> a submodule, but people want to know that.

Right, I think it's an interesting thing to know, but I agree there are
probably a lot of corner cases.

> Maybe we need to change that fundamental assumption.
> So a more sophisticated way (thinking long term here) would be
> to include the superprojects config file (with exceptions), and that
> config file has more priority than e.g. the ~/.gitconfig file, but less
> than the submodules own $GIT_DIR/config file.

Yeah, that priority matches what I had been thinking.

> > One other caveat: I'm not sure if we do insteadOf recursively, but it
> > may be surprising to the child "git clone" that we've already applied
> > the insteadOf rewriting (especially if the rules are coming from
> > ~/.gitconfig and may be applied twice).
> 
> When a rule is having effect twice the rule sounds broken. (the outcome
> ought to be sufficiently different from the original?)

If you have:

  url.bar.insteadOf=foo
  url.baz.insteadOf=bar

do we convert "foo" to "baz"? If so, then I think applying the rules
again shouldn't matter. But if we don't, and only do a single level,
then having the caller rewrite the URL before it hands it to "git clone"
means we may end up unexpectedly doing two levels of rewriting.

-Peff

Re: url..insteadOf vs. submodules

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Feb 21, 2017 at 3:00 PM, Jeff King  wrote:
> ...
>> I guess one answer is that this is the wrong approach entirely, and the
>> right one is something like: submodules should understand that they are
>> part of a superproject, and respect some whitelisted set of config from
>> the superproject .git/config file.
>
> This would break one of the core assumptions that submodules
> are "independent" repos.
>
> The way of action is a one way street:
> * The superproject is aware of the submodule and when you invoke a
> command on the superproject, you may mess around with the submodule,
> e.g. update/remove it; absorb its git directory.
> * The submodule is "just" a repository with weird .git link file and a
>   respective core.worktree setup. Currently it doesn't know if it is
>   guided by a superproject.

While that is a good discipline to follow, I think you need to
differenciate the project that is bound as a submodule to a
superproject, and a specific instance of a submodule repository,
i.e. a clone of such a project.

It is true that the Linux kernel project should *NEVER* know your
appliance project only because you happen to use it as a component
of your appliance that happens to use the kernel as one of its
submodules.  But that does not mean your copy of the kernel that
sits in your recursive checkout of your appliance project should
not know anything about your superproject.

This is true even without any submodules.  The Git project itself
does not even care you are Stefan, but you still can and do add
[user] name = "Stefan Beller" to .git/config of your clone of the
Git project.  A clone of the project may want to know more than the
data project itself keeps track of to describe the context in which
the particular clone is being used.  And .git/config is a good place
to keep such pieces of information.

So I would think it is entirely reasonable if "git submodule init
sub" that is run in the superproject to initialize "sub" writes
something in "sub/.git" to tell that "sub" is used in the context of
that particular toplevel superproject and customize its behavour
accordingly.  Perhaps it may want to add the url.*.insteadOf that is
useful for updating the submodule repository when it does "submodule
init", for example.

Re: [PATCH 12/15] unpack-trees: check if we can perform the operation for submodules

[Jacob Keller]

On Tue, Feb 21, 2017 at 2:16 PM, Stefan Beller  wrote:
> On Fri, Feb 17, 2017 at 10:42 AM, Jacob Keller  wrote:
>> On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller  wrote:
>>> +   if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED))
>>
>> Here, and in other cases where we use
>> is_active_submodule_with_strategy(), why do we only ever check
>> SM_UPDATE_UNSPECIFIED? It seems really weird that we're only going to
>> check submodules who's strategy is unspecified, when that defaults to
>> checkout if I recall correctly? Shouldn't we check both? This applies
>> to pretty much everywhere that you call this function that I noticed,
>> which is why I removed the context.
>
> I am torn between this.
>
> submodule..update = {rebase, merge, checkout, none !command}
> is currently documented in GIT-CONFIG(1) as
>
>submodule..update
>The default update procedure for a submodule. This variable is
>populated by git submodule init from the gitmodules(5) file. See
>description of update command in git-submodule(1).
>
> and in GIT-SUBMODULE(1) as
>
>update
>[...] can be done in several ways
>depending on command line options and the value of
>submodule..update configuration variable. Supported update
>procedures are:
>
>checkout
>[...] or no option is given, and
>submodule..update is unset, or if it is set to checkout.
>
> So the "update" config clearly only applies to the "submodule update"
> command, right?
>
> Well no, "checkout --recurse-submodules" is very similar
> to running "submodule update", except with a bit more checks, so you could
> think that such an option applies to checkout as well. (and eventually
> rebase/merge etc. are supported as well.)
>
> So initially I assumed both "unspecified" as well as "checkout"
> are good matches to support in the first round.
>
> Then I flip flopped to think that we should not interfere with these
> settings at all (The checkout command does checkout and checkout only;
> no implicit rebase/merge ever in the future, because that would be
> confusing). So ignoring that option seemed like the way to go.

Hmm. So it's a bit complicated.

>
> But ignoring that option is also not the right approach.
> What if you have set it to "none" and really *expect* Git to not touch
> that submodule?

Or set it to "rebase" and suddenly git-checkout is ignoring you and
just checking things out anyways.

>
> So I dunno. Maybe it is a documentation issue, we need to spell out
> in the man page for checkout that --recurse-submodules is
> following one of these models. Now which is the best default model here?

Personally, I would go with that the config option sets the general
strategy used by the submodule whenever its updated, regardless of
how.

So, for example, setting it to none, means that recurse-submoduls will
ignore it when checking out. Setting it to rebase, or merge, and the
checkout will try to do those things?

Or, if that's not really feasible, have the checkout go "hey.. you
asked me to recurse, but uhhh these submodules don't allow me to do
checkout, so I'm gonna fail"? I think that's the best approach for
now.

>
> Thanks,
> Stefan

[PATCH] submodule init: warn about falling back to a local path

[Stefan Beller]

When a submodule is initialized, the config variable 'submodule..url'
is set depending on the value of the same variable in the .gitmodules
file. When the URL indicates to be relative, then the url is computed
relative to its default remote. The default remote cannot be determined
accurately in all cases, such that it falls back to 'origin'.

The 'origin' remote may not exist, though. In that case we give up looking
for a suitable remote and we'll just assume it to be a local relative path.

This can be confusing to users as there is a lot of guessing involved,
which is not obvious to the user.

So in the corner case of assuming a local autoritative truth, warn the
user to lessen the confusion.

This behavior was introduced in 4d6893200 (submodule add: allow relative
repository path even when no url is set, 2011-06-06), which shared the
code with submodule-init and then ported to C in 3604242f080a (submodule:
port init from shell to C, 2016-04-15).

In case of submodule-add, this behavior makes sense in some use cases[1],
however for submodule-init there does not seem to be an immediate obvious
use case to fall back to a local submodule. However there might be, so
warn instead of die here.

[1] e.g. 
http://stackoverflow.com/questions/8721984/git-ignore-files-for-public-repository-but-not-for-private
"store a secret locally in a submodule, with no intention to publish it"

Reported-by: Shawn Pearce 
Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 899dc334e3..44c11dd91e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -356,12 +356,10 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
strbuf_addf(, "remote.%s.url", remote);
free(remote);
 
-   if (git_config_get_string(remotesb.buf, ))
-   /*
-* The repository is its own
-* authoritative upstream
-*/
+   if (git_config_get_string(remotesb.buf, )) {
remoteurl = xgetcwd();
+   warning(_("could not lookup configuration '%s'. 
Assuming this repository is its own authoritative upstream."), remotesb.buf);
+   }
relurl = relative_url(remoteurl, url, NULL);
strbuf_release();
free(remoteurl);
-- 
2.12.0.rc1.16.ge4278d41a0.dirty

Re: url..insteadOf vs. submodules

[Stefan Beller]

On Tue, Feb 21, 2017 at 3:00 PM, Jeff King  wrote:
> On Tue, Feb 21, 2017 at 10:19:38AM -0800, Stefan Beller wrote:
>
>> On Mon, Feb 20, 2017 at 11:06 PM, Jeff King  wrote:
>> >
>> > We'll see if the submodule folks have any ideas on how to implement
>> > that.
>> >
>>
>> So from reading your discussion, the user expectation is to have
>> `git submodule {init, update --init, sync}`
>> to pay attention to url..insteadOf when setting up the
>> submodule..URL, such that the modified URL is used for the
>> initial clone of the submodule (and hence any subsequent usage within
>> the submodule).
>
> Yeah, that was what I was envisioning.
>
>> Two caveates:
>>
>> * After running `git submodule init`, you change url..insteadOf
>>   in the superproject. How do we need to word the documentation to
>>   have users expecting this change doesn't affect submodules?
>>   (See above Any vs. "Any except (initialized) submodules")
>
> Good question.
>
> I guess one answer is that this is the wrong approach entirely, and the
> right one is something like: submodules should understand that they are
> part of a superproject, and respect some whitelisted set of config from
> the superproject .git/config file.

This would break one of the core assumptions that submodules
are "independent" repos.

The way of action is a one way street:
* The superproject is aware of the submodule and when you invoke a
command on the superproject, you may mess around with the submodule,
e.g. update/remove it; absorb its git directory.
* The submodule is "just" a repository with weird .git link file and a
  respective core.worktree setup. Currently it doesn't know if it is
  guided by a superproject.


Though I do not know if this is actually a good assumption.
e.g. "[PATCH v2] git-prompt.sh: add submodule indicator"
https://public-inbox.org/git/1486075892-20676-2-git-send-email-em...@benjaminfuchs.de/
really had trouble in the first version to nail down how to tell you are in
a submodule, but people want to know that.

>
> The second half is pretty easy to do (use git_config_from_file on the
> super-project's $GIT_DIR

There goes the "pretty easy"; currently there is no concept to find out
the existence of a super-project.

> /config, and pass a callback which filters the
> keys before passing them along to the real callback).
>
> I'm not sure about the first half (submodules know about their
> superproject), though.

Maybe we need to change that fundamental assumption.
So a more sophisticated way (thinking long term here) would be
to include the superprojects config file (with exceptions), and that
config file has more priority than e.g. the ~/.gitconfig file, but less
than the submodules own $GIT_DIR/config file.
Then a setting like the url rewriting would be "inherited" by the
submodule, with the option to overwrite the default as given by the
superproject.

>
>> * So with the point above the insteadOf config only applies to the
>>   init/sync process, (i.e. once in time, ideally).
>>   Is that confusing or actually simplifying the submodule workflow?
>
> Not sure. That's why I asked you. :)

I think that would be ok. With the idea of inheriting the superprojects
config, we allow for not storing the rewritten url, so the submodule
handling is less of a corner case here, and as another advantage the
rewriting rule is applied in real time, e.g. you can change the superprojects
rule after the fact and the submodule would automagically make use of it.

>
> One other caveat: I'm not sure if we do insteadOf recursively, but it
> may be surprising to the child "git clone" that we've already applied
> the insteadOf rewriting (especially if the rules are coming from
> ~/.gitconfig and may be applied twice).

When a rule is having effect twice the rule sounds broken. (the outcome
ought to be sufficiently different from the original?)

>
> -Peff

Thanks,
Stefan

Re: url..insteadOf vs. submodules

[Jeff King]

On Tue, Feb 21, 2017 at 10:19:38AM -0800, Stefan Beller wrote:

> On Mon, Feb 20, 2017 at 11:06 PM, Jeff King  wrote:
> >
> > We'll see if the submodule folks have any ideas on how to implement
> > that.
> >
> 
> So from reading your discussion, the user expectation is to have
> `git submodule {init, update --init, sync}`
> to pay attention to url..insteadOf when setting up the
> submodule..URL, such that the modified URL is used for the
> initial clone of the submodule (and hence any subsequent usage within
> the submodule).

Yeah, that was what I was envisioning.

> Two caveates:
> 
> * After running `git submodule init`, you change url..insteadOf
>   in the superproject. How do we need to word the documentation to
>   have users expecting this change doesn't affect submodules?
>   (See above Any vs. "Any except (initialized) submodules")

Good question.

I guess one answer is that this is the wrong approach entirely, and the
right one is something like: submodules should understand that they are
part of a superproject, and respect some whitelisted set of config from
the superproject .git/config file.

The second half is pretty easy to do (use git_config_from_file on the
super-project's $GIT_DIR/config, and pass a callback which filters the
keys before passing them along to the real callback).

I'm not sure about the first half (submodules know about their
superproject), though.

> * So with the point above the insteadOf config only applies to the
>   init/sync process, (i.e. once in time, ideally).
>   Is that confusing or actually simplifying the submodule workflow?

Not sure. That's why I asked you. :)

One other caveat: I'm not sure if we do insteadOf recursively, but it
may be surprising to the child "git clone" that we've already applied
the insteadOf rewriting (especially if the rules are coming from
~/.gitconfig and may be applied twice).

-Peff

Re: Git bisect does not find commit introducing the bug

[Philip Oakley]

From: "Alex Hoffman" 

isn't that spelt `--ancestry-path` ?
(--ancestry-path has it's own issues such as needing 
an --first-parent-show

option, but that's possibly a by the by)


Indeed it is spelled `--ancestry-path`. And interestingly enough you
may use it multiple times with the wanted effect in our case (e.g when
the user has multiple good commit and a single bad commit before
running the bisect itself).



Also it is `--first-parent` (not `--first-parent-show`),


My spelling was deliberate ;-)

If you use the currently coded --first-parent with a properly relevant 
commit for --ancestry-path then you get nothing. The purpose of 
ancestry-path is to find the ancestry chain that deviates from being a 
first-parent traversal [1], but the first-parent want to hold the walk to 
just the first parent chain - a contradiction.


Adding the -show at the end is my attempt to indicate that it is for the 
second aspect, that of selecting which commits to show/use.


I had an initial discussion back at [2], but failed then too.


but I do not understand why do we need this
option? What kind of issues does `--ancestry-path` have?

Best regards,
VG


--
Philip

[1] \git\Documentation\technical\api-revision-walking.txt ["two 
diff_options, one for path limiting, another for output"]
[2] 
https://public-inbox.org/git/2FA1998250474E76A386B82AD635E56A@PhilipOakley/ 

Re: Git trademark status and policy

[Jeff King]

On Tue, Feb 21, 2017 at 07:55:15AM -0800, G. Sylvie Davies wrote:

> Is "Gitter" allowed?   (https://gitter.im/).
> 
> More info here:
> 
> https://en.wikipedia.org/wiki/Gitter
> 
> Also, their twitter handle is @gitchat.
> 
> Not sure I'd even classify "gitter" as a portmanteau.

I don't think the Git committee has discussed that one. I'll mention it
there.

I wouldn't get hung up on the "is it a strict portmanteau" question. I
think the more important question is whether it creates confusion about
endorsement or interoperability. The portmanteau thing is more of a rule
of thumb there. (That's all IMHO, of course, and not an official
statement of the committee).

-Peff

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

[Jeff King]

On Tue, Feb 21, 2017 at 01:47:37PM -0800, Junio C Hamano wrote:

> > So the best I could come up with is:
> >
> >   git config pretty.twoline '%C(auto)%h %s%C(auto)%+d'
> >   git log --format=twoline
> > [...]
>
> Yeah, I had a similar thought to use something around "%n%-d", but
> 
>  $ git log --format='%h%n%-d%C(auto) %s %C(auto)'
> 
> is not it.
> 
> I guess we could pile on another hack to make the sign between % and
> the format specifier cumulative and then "%n%-+d" may do what we
> want, but we need a true %(if)...%(then)...%(else)...%(end) support
> if we really want to do this thing properly.

Yeah, I'd rather not pile up more hacks. The for-each-ref placeholders
are more verbose, but I think the end result is a lot easier to read and
maintain, and the terseness doesn't matter if you're sticking it behind
an alias or config option.

(Don't get me wrong; I think the %(if) ones are pretty ugly, too, but
the next step beyond that is embedding some kind of templating or
scripting language, and that just seems like overkill).

-Peff

What's cooking in git.git (Feb 2017, #06; Tue, 21)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

The tip of the 'master' is still at -rc2 but with a git-svn update
from Eric.  I'd like to get pull requests for gitk and git-gui
updates soonish, if we are to have one during this cycle.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* jk/tempfile-ferror-fclose-confusion (2017-02-17) 1 commit
  (merged to 'next' on 2017-02-21 at 479ba0131f)
 + tempfile: set errno to a known value before calling ferror()

 A caller of tempfile API that uses stdio interface to write to
 files may ignore errors while writing, which is detected when
 tempfile is closed (with a call to ferror()).  By that time, the
 original errno that may have told us what went wrong is likely to
 be long gone and was overwritten by an irrelevant value.
 close_tempfile() now resets errno to EIO to make errno at least
 predictable.

 Will cook in 'next'.


* ah/doc-ls-files-quotepath (2017-02-20) 1 commit
 - Documentation: link git-ls-files to core.quotePath variable

 Documentation for "git ls-files" did not refer to core.quotePath

 Needs review.


* dr/doc-check-ref-format-normalize (2017-02-21) 1 commit
  (merged to 'next' on 2017-02-21 at 5e88b7a93d)
 + git-check-ref-format: clarify documentation for --normalize

 Doc update.

 Will cook in 'next'.


* gp/document-dotfiles-in-templates-are-not-copied (2017-02-17) 1 commit
  (merged to 'next' on 2017-02-21 at bbfa2bb7d4)
 + init: document dotfiles exclusion on template copy

 Doc update.

 Will cook in 'next'.


* jc/config-case-cmdline (2017-02-21) 2 commits
  (merged to 'next' on 2017-02-21 at 354f023a3a)
 + config: reject invalid VAR in 'git -c VAR=VAL command'
 + config: preserve  case for one-shot config on the command line

 The code to parse "git -c VAR=VAL cmd" and set configuration
 variable for the duration of cmd had two small bugs, which have
 been fixed.

 Will cook in 'next'.


* jh/memihash-opt (2017-02-17) 5 commits
 - name-hash: remember previous dir_entry during lazy_init_name_hash
 - name-hash: specify initial size for istate.dir_hash table
 - name-hash: precompute hash values during preload-index
 - hashmap: allow memihash computation to be continued
 - name-hash: eliminate duplicate memihash call

 Expecting an update for perf?


* km/delete-ref-reflog-message (2017-02-20) 4 commits
  (merged to 'next' on 2017-02-21 at 4ee4ce3f64)
 + branch: record creation of renamed branch in HEAD's log
 + rename_ref: replace empty message in HEAD's log
 + update-ref: pass reflog message to delete_ref()
 + delete_ref: accept a reflog message argument

 "git update-ref -d" and other operations to delete references did
 not leave any entry in HEAD's reflog when the reference being
 deleted was the current branch.  This is not a problem in practice
 because you do not want to delete the branch you are currently on,
 but caused renaming of the current branch to something else not to
 be logged in a useful way.

 Will cook in 'next'.


* nd/prune-in-worktree (2017-02-19) 15 commits
 - rev-list: expose and document --single-worktree
 - revision.c: --reflog add HEAD reflog from all worktrees
 - files-backend: make reflog iterator go through per-worktree reflog
 - refs: add refs_for_each_reflog[_ent]()
 - revision.c: --all adds HEAD from all worktrees
 - refs: remove dead for_each_*_submodule()
 - revision.c: use refs_for_each*() instead of for_each_*_submodule()
 - refs: add a refs_for_each_in() and friends
 - refs: add refs_for_each_ref()
 - refs: add refs_head_ref()
 - refs: add refs_read_ref[_full]()
 - refs: move submodule slash stripping code to get_submodule_ref_store
 - revision.c: --indexed-objects add objects from all worktrees
 - revision.c: refactor add_index_objects_to_pending()
 - revision.h: new flag in struct rev_info wrt. worktree-related refs
 (this branch uses mh/ref-remove-empty-directory, mh/submodule-hash, 
nd/files-backend-git-dir and nd/worktree-kill-parse-ref.)

 "git gc" and friends when multiple worktrees are used off of a
 single repository did not consider the index and per-worktree refs
 of other worktrees as the root for reachability traversal, making
 objects that are in use only in other worktrees to be subject to
 garbage collection.


* mm/fetch-show-error-message-on-unadvertised-object (2017-02-21) 1 commit
 - fetch: print an error when declining to request an unadvertised object

 "git fetch" that requests a commit by object name, when the other
 side does not allow such an request, failed without much
 explanation.

 Expecting a split series?


* rl/remote-allow-missing-branch-name-merge (2017-02-21) 1 commit
 - 

Re: [PATCH 12/15] unpack-trees: check if we can perform the operation for submodules

[Stefan Beller]

On Fri, Feb 17, 2017 at 10:42 AM, Jacob Keller  wrote:
> On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller  wrote:
>> +   if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED))
>
> Here, and in other cases where we use
> is_active_submodule_with_strategy(), why do we only ever check
> SM_UPDATE_UNSPECIFIED? It seems really weird that we're only going to
> check submodules who's strategy is unspecified, when that defaults to
> checkout if I recall correctly? Shouldn't we check both? This applies
> to pretty much everywhere that you call this function that I noticed,
> which is why I removed the context.

I am torn between this.

submodule..update = {rebase, merge, checkout, none !command}
is currently documented in GIT-CONFIG(1) as

   submodule..update
   The default update procedure for a submodule. This variable is
   populated by git submodule init from the gitmodules(5) file. See
   description of update command in git-submodule(1).

and in GIT-SUBMODULE(1) as

   update
   [...] can be done in several ways
   depending on command line options and the value of
   submodule..update configuration variable. Supported update
   procedures are:

   checkout
   [...] or no option is given, and
   submodule..update is unset, or if it is set to checkout.

So the "update" config clearly only applies to the "submodule update"
command, right?

Well no, "checkout --recurse-submodules" is very similar
to running "submodule update", except with a bit more checks, so you could
think that such an option applies to checkout as well. (and eventually
rebase/merge etc. are supported as well.)

So initially I assumed both "unspecified" as well as "checkout"
are good matches to support in the first round.

Then I flip flopped to think that we should not interfere with these
settings at all (The checkout command does checkout and checkout only;
no implicit rebase/merge ever in the future, because that would be
confusing). So ignoring that option seemed like the way to go.

But ignoring that option is also not the right approach.
What if you have set it to "none" and really *expect* Git to not touch
that submodule?

So I dunno. Maybe it is a documentation issue, we need to spell out
in the man page for checkout that --recurse-submodules is
following one of these models. Now which is the best default model here?

Thanks,
Stefan

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

[Junio C Hamano]

Ross Lagerwall  writes:

> On Tue, Feb 21, 2017 at 11:32:54AM -0800, Junio C Hamano wrote:
>
>> I was waiting for others to comment on this patch but nobody seems
>> to be interested.  Which is a bit sad, because except for minor
>> nits, this patch is very well done.
>> 
>> The explanation of the motivation and solution in the proposed log
>> message is excellent.  It would have been perfect if you described
>> HOW you get into a state where branch..remote is pointing at
>> the remote being removed, without having branch..merge in the
>> first place, but even if such a state is invalid or unplausible,
>> removing the remote should be a usable way to recover from such a
>> situation.
>
> I got into this situation by setting branch..remote directly.  I
> was using push.default=current, and wanted a bare "git push" on the
> branch to push to a different remote from origin (which it defaults to).
> Configuring branch..remote made git do the right thing.

Ah, OK.  As you may have seen from the test I sent, I thought the
user started with

git checkout -b  -t /

in which case both are always set, and removed only one of them,
and that is what I called "deliberate sabotage".

What you did does sound like a very valid use case.  Let's update
the test to use that pattern and document the intended use case to
help with this fix in the updated log message.

Here is what I tentatively queued.

Thanks.

-- >8 --
From: Ross Lagerwall 
Date: Sat, 18 Feb 2017 00:23:41 +
Subject: [PATCH] remote: ignore failure to remove missing branch..merge

It is not all too unusual for a branch to use "branch..remote"
without "branch..merge".  You may be using the 'push.default'
configuration set to 'current', for example, and do

$ git checkout -b side colleague/side
$ git config branch.side.remote colleague

However, "git remote rm" to remove the remote used in such a manner
fails with

"fatal: could not unset 'branch..merge'"

because it assumes that a branch that has .remote defined must also
have .merge defined.  Detect the "cannot unset because it is not set
to begin with" case and ignore it.

Signed-off-by: Ross Lagerwall 
Signed-off-by: Junio C Hamano 
---
 builtin/remote.c  |  4 +++-
 t/t5505-remote.sh | 19 +++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index e52cf3925b..01055b7272 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -769,7 +769,9 @@ static int rm(int argc, const char **argv)
strbuf_reset();
strbuf_addf(, "branch.%s.%s",
item->string, *k);
-   git_config_set(buf.buf, NULL);
+   result = git_config_set_gently(buf.buf, NULL);
+   if (result && result != CONFIG_NOTHING_SET)
+   die(_("could not unset '%s'"), buf.buf);
}
}
}
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 8198d8eb05..f558ad0b39 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -153,6 +153,25 @@ test_expect_success 'remove errors out early when deleting 
non-existent branch'
)
 '
 
+test_expect_success 'remove remote with a branch without configured merge' '
+   test_when_finished "(
+   git -C test checkout master;
+   git -C test branch -D two;
+   git -C test config --remove-section remote.two;
+   git -C test config --remove-section branch.second;
+   true
+   )" &&
+   (
+   cd test &&
+   git remote add two ../two &&
+   git fetch two &&
+   git checkout -b second two/master^0 &&
+   git config branch.second.remote two &&
+   git checkout master &&
+   git remote rm two
+   )
+'
+
 test_expect_success 'rename errors out early when deleting non-existent 
branch' '
(
cd test &&
-- 
2.12.0-rc2-231-g83a1c8597c

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

[Junio C Hamano]

Jeff King  writes:

> The for-each-ref formatting code has %(if), but it's not unified with
> the commit-format ones.
>
> So the best I could come up with is:
>
>   git config pretty.twoline '%C(auto)%h %s%C(auto)%+d'
>   git log --format=twoline
>
> which looks like:
>
>   80ba04ed9 Merge branch 'svn-escape-backslash' of git://bogomips.org/git-svn
>(origin/master, origin/HEAD)
>   20769079d Git 2.12-rc2
>(tag: v2.12.0-rc2)
>   076c05393 Hopefully the final batch of mini-topics before the final
>   c5b22b819 Merge branch 'jk/tempfile-ferror-fclose-confusion'
>   62fef5c56 Merge branch 'dp/submodule-doc-markup-fix'
>   1f73ff080 Merge branch 'jk/reset-to-break-a-commit-doc-updated'

Yeah, I had a similar thought to use something around "%n%-d", but

 $ git log --format='%h%n%-d%C(auto) %s %C(auto)'

is not it.

I guess we could pile on another hack to make the sign between % and
the format specifier cumulative and then "%n%-+d" may do what we
want, but we need a true %(if)...%(then)...%(else)...%(end) support
if we really want to do this thing properly.

[PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

The parsing of one-shot assignments of configuration variables that
come from the command line historically was quite loose and allowed
anything to pass.

The configuration variable names that come from files are validated
in git_config_parse_source(), which uses get_base_var() that grabs
the  (and subsection) while making sure that 
consists of iskeychar() letters, the function itself that makes sure
that the first letter in  is isalpha(), and get_value()
that grabs the remainder of the  name while making sure
that it consists of iskeychar() letters.

Perform an equivalent check in canonicalize_config_variable_name()
to catch invalid configuration variable names that come from the
command line.

Signed-off-by: Junio C Hamano 
---

 * Now with an updated test; while writing it it uncovered a bug in
   the original test that expected to fail---they failed alright but
   sometimes failed for a wrong reason.

 config.c   | 48 +---
 t/t1300-repo-config.sh | 16 
 2 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 4128debc71..e7f7ff1938 100644
--- a/config.c
+++ b/config.c
@@ -199,32 +199,62 @@ void git_config_push_parameter(const char *text)
strbuf_release();
 }
 
+static inline int iskeychar(int c)
+{
+   return isalnum(c) || c == '-';
+}
+
 /*
  * downcase the  and  in . or
  * .. and do so in place.  
  * is left intact.
+ *
+ * The configuration variable names that come from files are validated
+ * in git_config_parse_source(), which uses get_base_var() that grabs
+ * the  (and subsection) while making sure that 
+ * consists of iskeychar() letters, the function itself that makes
+ * sure that the first letter in  is isalpha(), and
+ * get_value() that grabs the remainder of the  name while
+ * making sure that it consists of iskeychar() letters.  Perform a
+ * matching validation for configuration variables that come from
+ * the command line.
  */
-static void canonicalize_config_variable_name(char *varname)
+static int canonicalize_config_variable_name(char *varname)
 {
-   char *cp, *last_dot;
+   char *cp, *first_dot, *last_dot;
 
/* downcase the first segment */
for (cp = varname; *cp; cp++) {
if (*cp == '.')
break;
+   if (!iskeychar(*cp))
+   return -1;
*cp = tolower(*cp);
}
if (!*cp)
-   return;
+   return -1; /* no dot anywhere? */
+
+   first_dot = cp;
+   if (first_dot == varname)
+   return -1; /* no section? */
 
/* find the last dot (we start from the first dot we just found) */
-   for (last_dot = cp; *cp; cp++)
+   for (; *cp; cp++)
if (*cp == '.')
last_dot = cp;
 
+   if (!last_dot[1])
+   return -1; /* no variable? */
+
/* downcase the last segment */
-   for (cp = last_dot; *cp; cp++)
+   for (cp = last_dot + 1; *cp; cp++) {
+   if (cp == last_dot + 1 && !isalpha(*cp))
+   return -1;
+   else if (!iskeychar(*cp))
+   return -1;
*cp = tolower(*cp);
+   }
+   return 0;
 }
 
 int git_config_parse_parameter(const char *text,
@@ -249,7 +279,8 @@ int git_config_parse_parameter(const char *text,
strbuf_list_free(pair);
return error("bogus config parameter: %s", text);
}
-   canonicalize_config_variable_name(pair[0]->buf);
+   if (canonicalize_config_variable_name(pair[0]->buf))
+   return error("bogus config parameter: %s", text);
if (fn(pair[0]->buf, value, data) < 0) {
strbuf_list_free(pair);
return -1;
@@ -382,11 +413,6 @@ static char *parse_value(void)
}
 }
 
-static inline int iskeychar(int c)
-{
-   return isalnum(c) || c == '-';
-}
-
 static int get_value(config_fn_t fn, void *data, struct strbuf *name)
 {
int c;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 7a16f66a9d..ea371020fa 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1143,6 +1143,22 @@ test_expect_success 'last one wins: three level vars' '
test_cmp expect actual
 '
 
+for VAR in a .a a. a.0b a."b c". a."b c".0d
+do
+   test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" '
+   test_must_fail git -c "$VAR=VAL" config -l
+   '
+done
+
+for VAR in a.b a."b c".d
+do
+   test_expect_success "git -c $VAR=VAL works with valid '$VAR'" '
+   echo VAL >expect &&
+   git -c "$VAR=VAL" config --get "$VAR" >actual &&
+   test_cmp expect actual
+   '
+done
+
 test_expect_success 'git -c is not confused by empty environment' '
GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
 '
-- 
2.12.0-rc2-222-gff02733afe

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

[Jeff King]

On Tue, Feb 21, 2017 at 12:40:11PM -0800, Linus Torvalds wrote:

> In fact, I played around with some formats, and the one I lines the
> most was actually one that split the line for decorations, but that
> one was admittedly pretty funky. It gives output like
> 
>   b9df16a4c (HEAD -> master)
> pathspec: don't error out on all-exclusionary pathspec patterns
>   91a491f05 pathspec magic: add '^' as alias for '!'
>   c8e05fd6d ls-remote: add "--diff" option to show only refs that differ
>   20769079d (tag: v2.12.0-rc2, origin/master, origin/HEAD)
> Git 2.12-rc2
>   076c05393 Hopefully the final batch of mini-topics before the final
>   c5b22b819 Merge branch 'jk/tempfile-ferror-fclose-confusion'
>   62fef5c56 Merge branch 'dp/submodule-doc-markup-fix'
>   1f73ff080 Merge branch 'jk/reset-to-break-a-commit-doc-updated'
>   bf5f11918 Merge branch 'jk/reset-to-break-a-commit-doc'
>   e048a257b Merge branch 'js/mingw-isatty'
> 
> (which looks better with colorization than it looks in the email).
> 
> But I'm not even going to send out that patch, because it was such an
> atrocious hack to line things up.

I was going to suggest a custom format string that does the same, but
what we have is not _quite_ flexible enough.

You can use "%+d" to insert a newline only when "%d" is not empty. But
it always inserts _before_ the decoration, not after. Likewise, you
cannot say "if it's not empty, then insert %d and a leading tab".

The for-each-ref formatting code has %(if), but it's not unified with
the commit-format ones.

So the best I could come up with is:

  git config pretty.twoline '%C(auto)%h %s%C(auto)%+d'
  git log --format=twoline

which looks like:

  80ba04ed9 Merge branch 'svn-escape-backslash' of git://bogomips.org/git-svn
   (origin/master, origin/HEAD)
  20769079d Git 2.12-rc2
   (tag: v2.12.0-rc2)
  076c05393 Hopefully the final batch of mini-topics before the final
  c5b22b819 Merge branch 'jk/tempfile-ferror-fclose-confusion'
  62fef5c56 Merge branch 'dp/submodule-doc-markup-fix'
  1f73ff080 Merge branch 'jk/reset-to-break-a-commit-doc-updated'

-Peff

Re: [PATCH 08/15] submodules: introduce check to see whether to touch a submodule

[Stefan Beller]

On Fri, Feb 17, 2017 at 10:36 AM, Jacob Keller  wrote:
> On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller  wrote:
>> In later patches we introduce the --recurse-submodule flag for commands
>> that modify the working directory, e.g. git-checkout.
>>
>> It is potentially expensive to check if a submodule needs an update,
>> because a common theme to interact with submodules is to spawn a child
>> process for each interaction.
>>
>> So let's introduce a function that checks if a submodule needs
>> to be checked for an update before attempting the update.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>  submodule.c | 27 +++
>>  submodule.h | 13 +
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/submodule.c b/submodule.c
>> index 591f4a694e..2a37e03420 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -548,6 +548,33 @@ void set_config_update_recurse_submodules(int value)
>> config_update_recurse_submodules = value;
>>  }
>>
>> +int touch_submodules_in_worktree(void)
>> +{
>> +   /*
>> +* Update can't be "none", "merge" or "rebase",
>> +* treat any value as OFF, except an explicit ON.
>> +*/
>> +   return config_update_recurse_submodules == RECURSE_SUBMODULES_ON;
>> +}
>
> Ok, so here, we're just checking whether the value is
> RECURSE_SUBMODULES_ON. The comment doesn't make sense to me at all.

Yes the comment was not updated in the last round of patches and is
now out of context.

> What is "update" and why "can't" it be those values? How is this code
> treating thise values as OFF exept for an explicit ON?

Please disregard the comment; I'll remove it in the next reroll.
The submodule API is in such a way that
config_update_recurse_submodules

>
> At first I thought this comment was related to check in the previous
> patch. I think I see the patch is "recurse submodules = true" or
> "recurse submodules = checkout" as a specific strategy? Ie:
> recurse-submodules=checkout" means "recurse into submodules and update
> them using checkout strategy?

Yes that is what I had in mind. See previous comment, in a later series
we could extend that to other strategies such as "revert-in-submodules"
for git-revert or "rebase", "merge" as we curreently have for
"git submodule update".

> Maybe this should be called something like
> recurse_into_submodules_in_worktree() though that is pretty verbose.

I like that. (It's less than double the number of characters, so it's
fine, isn't it?)
Maybe we can abbreviate worktree by "wt" ans "submodules by subs:

/* recurse into submodules in the worktree? */
int rec_subs_wt;

That looks short enough to qualify as non-Java.

> I'm not sure I have a good name really. But I wonder why we need this
> in the first place. Basically, we set RECURSE_SUBMODULES_* value which
> could be OFF, ON, or even future extensions of CHECKOUT, REBASE,
> MERGE, etc?
>
> But shouldn't we just return the mode, and let the later code decide
> "oh. actually I don't support this mode". For now, obviously we
> wouldn't set any of the new modes yet.

Mh, makes sense. Maybe I tricked myself into premature optimization,
because I'd expect most of the users not caring about submodules, such
that we want to have a *really* cheap way of saying "no, not interesting in
submodules", which is what this method mainly offers.

Junio also remarked this and the following
"is_active_submodule_with_strategy" to be bad design.

I'll redo those, such that the caller decides what to do with each strategy.

>
>> +
>> +int is_active_submodule_with_strategy(const struct cache_entry *ce,
>> + enum submodule_update_type strategy)
>> +{
>> +   const struct submodule *sub;
>> +
>> +   if (!S_ISGITLINK(ce->ce_mode))
>> +   return 0;
>> +
>> +   if (!touch_submodules_in_worktree())
>> +   return 0;
>> +
>> +   sub = submodule_from_path(null_sha1, ce->name);
>> +   if (!sub)
>> +   return 0;
>> +
>> +   return sub->update_strategy.type == strategy;
>> +}
>> +
>
> I liked Junio's suggestion where this just returns the strategy that
> it can use, or 0 if it shouldn't be updated. Then, other code just
> decides: Yes, I can use this strategy or no I cannot.
>
> Should this be tied in with the strategy used by the
> recurse_submodules above? ie: when/if we support recursing using other
> strategies, shouldn't we make these match here, so that if the recurse
> mode is "checkout" we don't checkout a submodule that was configured
> to be rebased? Or do you want to blindly apply checkout to all
> submodules even if they don't have strategy?
>
> I assume you do not, since you check here with passing a strategy
> value, and then see if it matches.
>
> this could be named something like:
>
> get_active_submodule_strategy() and return the strategy directly. It
> would then return 0 in any case where it 

Re: [PATCH 06/15] update submodules: add submodule config parsing

[Jacob Keller]

On Tue, Feb 21, 2017 at 11:42 AM, Stefan Beller  wrote:
> On Fri, Feb 17, 2017 at 10:24 AM, Jacob Keller  wrote:
>>
>> Ok so this function here reads a recurse submodules parameter which is
>> a boolean or it can be set to the word "checkout"? Why does checkout
>> need its own value separate from true? Just so that we have a synonym?
>> or so that we can expand on it in the future?
>
> I think eventually we want all the commands that touch the worktree to
> be able to cope with submodules.
>
>   Now what should e.g. git-revert --recurse-submodules do?
>   yes == "checkout" means we'd revert the superproject commit and
>   if that commit changed any submodule pointers we'd just "checkout"
>   those states in the submodule.
>
>   For revert you could also imagine to have
>   git-revert --recurse-submodules=revert-in-subs
>   that would not repoint the submodule pointer to the old state, but
>   would try to revert $OLD..$NEW in the submodule and take the newly
>   reverted state as the new submodule pointer.
>
> As I want to focus on checkout first, I went with "yes == checkout"
> here (or rather the other way round).

Ok I understand, but this seems like the variable could eventually
start to included more and more complex things? For now, "checkout"
means "when changing submodules prefer to check out contents" right?

I guess that sort of makes some sense.

Thanks,
Jake

Re: [PATCH] Documentation: Link git-ls-files to core.quotePath variable.

[Junio C Hamano]

Andreas Heiduk  writes:

> Add a hint for script writers where additional quoting can be configured.
>
> Signed-off-by: Andreas Heiduk 
> ---
>  Documentation/git-ls-files.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 446209e..19e0636 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -198,7 +198,8 @@ path. (see linkgit:git-read-tree[1] for more information 
> on state)
>  
>  When `-z` option is not used, TAB, LF, and backslash characters
>  in pathnames are represented as `\t`, `\n`, and `\\`,
> -respectively.
> +respectively. The path is also quoted according to the
> +configuration variable `core.quotePath` (see linkgit:git-config[1]).

I was waiting for others to comment on this patch but nobody seems
to be interested.  Which is a bit sad, as this may not be a bad
idea.

If we refer to core.quotePath, the mention of control characters
being quoted can also be omitted, I think, as that is part of what
appears in the description of core.quotePath variable.

Alternatively, instead of referring to another page, we can spend
the additional lines to say what is more interesting to most of the
readers from that page, e.g.

When `-z` option is not used, a pathname with "unusual" characters
in it is quoted by enclosing it in a double-quote pair and with
backslashes the same way strings in C source code are quoted.  By
setting core.quotePath configuration to false, the bytes whose
values are higher than 0x80 are output verbatim.

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

[Linus Torvalds]

On Tue, Feb 21, 2017 at 12:11 PM, Junio C Hamano  wrote:
>
> The updated organization smells a lot better to me ;-)

So I have been using the original patch for a bit over a week now, and
I have to say that I'm not sure it's the right thing to do after all.

Most of the time I much prefer the "decorations at the end" thing,
because it just looks better, and the commit log oneliners line up
nicely.

But then occasionally I end up liking the old interface better, just
because there's long commit lines, and showing the decoration at the
end effectively hides it.

So I vacillate between the two formats, and so I'm not sure this patch
is worth the change in behavior after all.

In fact, I played around with some formats, and the one I lines the
most was actually one that split the line for decorations, but that
one was admittedly pretty funky. It gives output like

  b9df16a4c (HEAD -> master)
pathspec: don't error out on all-exclusionary pathspec patterns
  91a491f05 pathspec magic: add '^' as alias for '!'
  c8e05fd6d ls-remote: add "--diff" option to show only refs that differ
  20769079d (tag: v2.12.0-rc2, origin/master, origin/HEAD)
Git 2.12-rc2
  076c05393 Hopefully the final batch of mini-topics before the final
  c5b22b819 Merge branch 'jk/tempfile-ferror-fclose-confusion'
  62fef5c56 Merge branch 'dp/submodule-doc-markup-fix'
  1f73ff080 Merge branch 'jk/reset-to-break-a-commit-doc-updated'
  bf5f11918 Merge branch 'jk/reset-to-break-a-commit-doc'
  e048a257b Merge branch 'js/mingw-isatty'

(which looks better with colorization than it looks in the email).

But I'm not even going to send out that patch, because it was such an
atrocious hack to line things up.

  Linus

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

[Ross Lagerwall]

On Tue, Feb 21, 2017 at 11:32:54AM -0800, Junio C Hamano wrote:
> Ross Lagerwall  writes:
> 
> > If a branch is configured with a default remote but no
> > branch..merge and then the remote is removed, git fails to remove
> > the remote with:
> > "fatal: could not unset 'branch..merge'"
> >
> > Instead, ignore this since it is not an error and shouldn't prevent the
> > remote from being removed.
> >
> > Signed-off-by: Ross Lagerwall 
> > ---
> 
> I was waiting for others to comment on this patch but nobody seems
> to be interested.  Which is a bit sad, because except for minor
> nits, this patch is very well done.
> 
> The explanation of the motivation and solution in the proposed log
> message is excellent.  It would have been perfect if you described
> HOW you get into a state where branch..remote is pointing at
> the remote being removed, without having branch..merge in the
> first place, but even if such a state is invalid or unplausible,
> removing the remote should be a usable way to recover from such a
> situation.

I got into this situation by setting branch..remote directly.  I
was using push.default=current, and wanted a bare "git push" on the
branch to push to a different remote from origin (which it defaults to).
Configuring branch..remote made git do the right thing.

Perhaps what I did was invalid, I'm not sure, but it achieved what I
wanted.

> 
> And the proposed solution in the diff seems to correctly implement
> what the description of the solution in the log message (modulo a
> minor nit).
> 
> >  builtin/remote.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/remote.c b/builtin/remote.c
> > index e52cf3925..5dd22c2eb 100644
> > --- a/builtin/remote.c
> > +++ b/builtin/remote.c
> > @@ -769,7 +769,9 @@ static int rm(int argc, const char **argv)
> > strbuf_reset();
> > strbuf_addf(, "branch.%s.%s",
> > item->string, *k);
> > -   git_config_set(buf.buf, NULL);
> > +   result = git_config_set_gently(buf.buf, NULL);
> > +   if (result && result != CONFIG_NOTHING_SET)
> > +   die(_("COULd not unset '%s'"), buf.buf);
> 
> With s/COUL/coul/, the result would be more in line with our
> existing practice.

Oops. That's what I get for coding when I should have been sleeping.

> 
> > }
> > }
> > }
> 
> We do want an additional test so that this fix will not be broken
> again in the future by mistake, perhaps in t5505.
> 
> As it is unclear to me how you got into a state where branch.*.remote
> exists without branch.*.merge, the attached patch to the test manually
> removes it, which probably falls into a "deliberate sabotage" category.
> If there are a valid sequence of operations that leads to such a state
> without being a deliberate sabotage, we should use it instead in the
> real test.
> 

See my explanation above. I wouldn't call it "deliberate sabotage", but
rather using config knobs in unexpected ways.

The test case looks reasonable. Do you want me to resend a patch with
the test case included (and nit fixed), or will you fix it up?

Thanks,
-- 
Ross Lagerwall

Re: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

Stefan Beller  writes:

> Combining this thought with another email[1] that flew by,
> do we also need to have this treatment for "git clone -c"

You tell me ;-) 

Do we share the same parser?  If not, should we make them share the
same code?

>> +for VAR in a .a a. a.0b a."b c". a."b c".0d
>> +do
>> +   test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" '
>> +   test_must_fail git -c $VAR=VAL config -l
>> +   '
>> +done
>> +
>>  test_expect_success 'git -c is not confused by empty environment' '
>> GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
>
> Do we also want to test obscure cases of expected success?
> e.g. I suspect we never use a."b c".d in the test suite elsewhere but it
> would be a valid key to be handed to git?

I wasn't aiming for anything obscure (and a."b c".d is not at all
obscure); as the new tests like "git -c V.a.R config --get V.A.R"
added in the previous step makes sure that the second level is not
molested and passed as is, so it is less urgent to see what can and
cannot come at the second level.

I didn't check if the existing coverage was sufficient, but we
certainly should test that three-level names with non-alpha and
non-keychar letters in the second are allowed in the overall "git
config" test, not limited to the case where the configuration comes
on a one-shot command line but from files.  I tend to think that is
a separate issue, though.

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

[Junio C Hamano]

Linus Torvalds  writes:

> That source showing should never have been in "show_decorations()" in
> the first place. It just happened to be a convenient place for it.
>
> So this attached patch is just my original patch updated to split up
> "show_source()" from "show_decorations()", and show it where it used
> to be.

The updated organization smells a lot better to me ;-) 

Most of the time it is convenient to have "show source" at the
beginning of a single helper that is to show both, but oneline
format is so special that it makes it inconvenient to have them at
the same place.

I can lose the patch to 4202 (update the expectation for --source)
I added to the previous one, but the patch to 4207 (update the
expectation for --decorate) needs to be kept with this round.

Will replace; thanks.

​​Hallo Freund

[Wang Jianlin]

Hallo Freund
Ich beabsichtige, Ihnen einen Teil meines Reichtums als freiwillige finanzielle 
Spende an Sie zu geben, Reagieren Sie, um teilzunehmen.
Wang Jianlin
Wanda Gruppe

Re: [PATCH 06/15] update submodules: add submodule config parsing

[Stefan Beller]

On Fri, Feb 17, 2017 at 10:24 AM, Jacob Keller  wrote:
>
> Ok so this function here reads a recurse submodules parameter which is
> a boolean or it can be set to the word "checkout"? Why does checkout
> need its own value separate from true? Just so that we have a synonym?
> or so that we can expand on it in the future?

I think eventually we want all the commands that touch the worktree to
be able to cope with submodules.

  Now what should e.g. git-revert --recurse-submodules do?
  yes == "checkout" means we'd revert the superproject commit and
  if that commit changed any submodule pointers we'd just "checkout"
  those states in the submodule.

  For revert you could also imagine to have
  git-revert --recurse-submodules=revert-in-subs
  that would not repoint the submodule pointer to the old state, but
  would try to revert $OLD..$NEW in the submodule and take the newly
  reverted state as the new submodule pointer.

As I want to focus on checkout first, I went with "yes == checkout"
here (or rather the other way round).

Re: Git bisect does not find commit introducing the bug

[Alex Hoffman]

> isn't that spelt `--ancestry-path` ?
> (--ancestry-path has it's own issues such as needing an --first-parent-show
> option, but that's possibly a by the by)

Indeed it is spelled `--ancestry-path`. And interestingly enough you
may use it multiple times with the wanted effect in our case (e.g when
the user has multiple good commit and a single bad commit before
running the bisect itself). Also it is `--first-parent` (not
`--first-parent-show`), but I do not understand why do we need this
option? What kind of issues does `--ancestry-path` have?

Best regards,
VG

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

[Junio C Hamano]

Ross Lagerwall  writes:

> If a branch is configured with a default remote but no
> branch..merge and then the remote is removed, git fails to remove
> the remote with:
> "fatal: could not unset 'branch..merge'"
>
> Instead, ignore this since it is not an error and shouldn't prevent the
> remote from being removed.
>
> Signed-off-by: Ross Lagerwall 
> ---

I was waiting for others to comment on this patch but nobody seems
to be interested.  Which is a bit sad, because except for minor
nits, this patch is very well done.

The explanation of the motivation and solution in the proposed log
message is excellent.  It would have been perfect if you described
HOW you get into a state where branch..remote is pointing at
the remote being removed, without having branch..merge in the
first place, but even if such a state is invalid or unplausible,
removing the remote should be a usable way to recover from such a
situation.

And the proposed solution in the diff seems to correctly implement
what the description of the solution in the log message (modulo a
minor nit).

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

With s/COUL/coul/, the result would be more in line with our
existing practice.

>   }
>   }
>   }

We do want an additional test so that this fix will not be broken
again in the future by mistake, perhaps in t5505.

As it is unclear to me how you got into a state where branch.*.remote
exists without branch.*.merge, the attached patch to the test manually
removes it, which probably falls into a "deliberate sabotage" category.
If there are a valid sequence of operations that leads to such a state
without being a deliberate sabotage, we should use it instead in the
real test.

Thanks.

 builtin/remote.c  |  4 +++-
 t/t5505-remote.sh | 19 +++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index e52cf3925b..01055b7272 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -769,7 +769,9 @@ static int rm(int argc, const char **argv)
strbuf_reset();
strbuf_addf(, "branch.%s.%s",
item->string, *k);
-   git_config_set(buf.buf, NULL);
+   result = git_config_set_gently(buf.buf, NULL);
+   if (result && result != CONFIG_NOTHING_SET)
+   die(_("could not unset '%s'"), buf.buf);
}
}
}
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 8198d8eb05..af85a624fc 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -153,6 +153,25 @@ test_expect_success 'remove errors out early when deleting 
non-existent branch'
)
 '
 
+test_expect_success 'remove remote with a branch without configured merge' '
+   test_when_finished "(
+   git -C test checkout master;
+   git -C test branch -D two;
+   git -C test config --remove-section remote.two;
+   git -C test config --remove-section branch.second;
+   true
+   )" &&
+   (
+   cd test &&
+   git remote add two ../two &&
+   git fetch two &&
+   git checkout -t -b second two/master &&
+   git checkout master &&
+   git config --unset branch.second.merge &&
+   git remote rm two
+   )
+'
+
 test_expect_success 'rename errors out early when deleting non-existent 
branch' '
(
cd test &&

Re: [PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'

[Stefan Beller]

On Tue, Feb 21, 2017 at 10:53 AM, Junio C Hamano  wrote:
> The parsing of one-shot assignments of configuration variables that
> come from the command line historically was quite loose and allowed
> anything to pass.
>
> The configuration variable names that come from files are validated
> in git_config_parse_source(), which uses get_base_var() that grabs
> the  (and subsection) while making sure that 
> consists of iskeychar() letters, the function itself that makes sure
> that the first letter in  is isalpha(), and get_value()
> that grabs the remainder of the  name while making sure
> that it consists of iskeychar() letters.
>
> Perform an equivalent check in canonicalize_config_variable_name()
> to catch invalid configuration variable names that come from the
> command line.
>
> Signed-off-by: Junio C Hamano 
> ---
>
> Junio C Hamano  writes:
>
> > One thing I noticed is that "git config --get X" will correctly
> > diagnose that a dot-less X is not a valid variable name, but we do
> > not seem to diagnose "git -c X=V " as invalid.
> >
> > Perhaps we should, but it is not the focus of this topic.
>
> And here is a follow-up on that tangent.

Combining this thought with another email[1] that flew by,
do we also need to have this treatment for "git clone -c"
[1] http://public-inbox.org/git/ec270e42-9431-446c-96f9-e1a0c3e45...@gmail.com/

>
> +for VAR in a .a a. a.0b a."b c". a."b c".0d
> +do
> +   test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" '
> +   test_must_fail git -c $VAR=VAL config -l
> +   '
> +done
> +
>  test_expect_success 'git -c is not confused by empty environment' '
> GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list

Do we also want to test obscure cases of expected success?
e.g. I suspect we never use a."b c".d in the test suite elsewhere but it
would be a valid key to be handed to git?

Re: Partnership with Git

[Junio C Hamano]

Dov Grobgeld  writes:

> As git is free software, you are free to use it in any way you see fit, as
> long as you adhere to its licensing terms, and to the copyright
> restrictions on using the term "git". Thus there is no need to ask
> permission and there does not on the git side exist any entity interested
> in "cross marketing activities".

s/copyright/trademark/.  

As one of Software Freedom Conservancy projects, I suspect that the
Git PLC g...@sfconservancy.org may be the closest to such "entity"
that represents open source Git community's interest to the outside
world with help from lawyers.

Not that I think Git PLC is interested in such a cross marketting
arrangement, but if Devart wants to advertise on their webpage
saying "we support Git by making contributions to SFC" or something
like that, they are the people to talk to.

[PATCH] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

The parsing of one-shot assignments of configuration variables that
come from the command line historically was quite loose and allowed
anything to pass.

The configuration variable names that come from files are validated
in git_config_parse_source(), which uses get_base_var() that grabs
the  (and subsection) while making sure that 
consists of iskeychar() letters, the function itself that makes sure
that the first letter in  is isalpha(), and get_value()
that grabs the remainder of the  name while making sure
that it consists of iskeychar() letters.

Perform an equivalent check in canonicalize_config_variable_name()
to catch invalid configuration variable names that come from the
command line.

Signed-off-by: Junio C Hamano 
---

Junio C Hamano  writes:

> One thing I noticed is that "git config --get X" will correctly
> diagnose that a dot-less X is not a valid variable name, but we do
> not seem to diagnose "git -c X=V " as invalid.
>
> Perhaps we should, but it is not the focus of this topic.

And here is a follow-up on that tangent.

 config.c   | 48 +---
 t/t1300-repo-config.sh |  7 +++
 2 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 4128debc71..e7f7ff1938 100644
--- a/config.c
+++ b/config.c
@@ -199,32 +199,62 @@ void git_config_push_parameter(const char *text)
strbuf_release();
 }
 
+static inline int iskeychar(int c)
+{
+   return isalnum(c) || c == '-';
+}
+
 /*
  * downcase the  and  in . or
  * .. and do so in place.  
  * is left intact.
+ *
+ * The configuration variable names that come from files are validated
+ * in git_config_parse_source(), which uses get_base_var() that grabs
+ * the  (and subsection) while making sure that 
+ * consists of iskeychar() letters, the function itself that makes
+ * sure that the first letter in  is isalpha(), and
+ * get_value() that grabs the remainder of the  name while
+ * making sure that it consists of iskeychar() letters.  Perform a
+ * matching validation for configuration variables that come from
+ * the command line.
  */
-static void canonicalize_config_variable_name(char *varname)
+static int canonicalize_config_variable_name(char *varname)
 {
-   char *cp, *last_dot;
+   char *cp, *first_dot, *last_dot;
 
/* downcase the first segment */
for (cp = varname; *cp; cp++) {
if (*cp == '.')
break;
+   if (!iskeychar(*cp))
+   return -1;
*cp = tolower(*cp);
}
if (!*cp)
-   return;
+   return -1; /* no dot anywhere? */
+
+   first_dot = cp;
+   if (first_dot == varname)
+   return -1; /* no section? */
 
/* find the last dot (we start from the first dot we just found) */
-   for (last_dot = cp; *cp; cp++)
+   for (; *cp; cp++)
if (*cp == '.')
last_dot = cp;
 
+   if (!last_dot[1])
+   return -1; /* no variable? */
+
/* downcase the last segment */
-   for (cp = last_dot; *cp; cp++)
+   for (cp = last_dot + 1; *cp; cp++) {
+   if (cp == last_dot + 1 && !isalpha(*cp))
+   return -1;
+   else if (!iskeychar(*cp))
+   return -1;
*cp = tolower(*cp);
+   }
+   return 0;
 }
 
 int git_config_parse_parameter(const char *text,
@@ -249,7 +279,8 @@ int git_config_parse_parameter(const char *text,
strbuf_list_free(pair);
return error("bogus config parameter: %s", text);
}
-   canonicalize_config_variable_name(pair[0]->buf);
+   if (canonicalize_config_variable_name(pair[0]->buf))
+   return error("bogus config parameter: %s", text);
if (fn(pair[0]->buf, value, data) < 0) {
strbuf_list_free(pair);
return -1;
@@ -382,11 +413,6 @@ static char *parse_value(void)
}
 }
 
-static inline int iskeychar(int c)
-{
-   return isalnum(c) || c == '-';
-}
-
 static int get_value(config_fn_t fn, void *data, struct strbuf *name)
 {
int c;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 7a16f66a9d..92a5d0dfb1 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1143,6 +1143,13 @@ test_expect_success 'last one wins: three level vars' '
test_cmp expect actual
 '
 
+for VAR in a .a a. a.0b a."b c". a."b c".0d
+do
+   test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" '
+   test_must_fail git -c $VAR=VAL config -l
+   '
+done
+
 test_expect_success 'git -c is not confused by empty environment' '
GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
 '
-- 
2.12.0-rc2-221-ga92f6f5816

Re: url..insteadOf vs. submodules

[Stefan Beller]

On Mon, Feb 20, 2017 at 11:06 PM, Jeff King  wrote:
>
> We'll see if the submodule folks have any ideas on how to implement
> that.
>

So from reading your discussion, the user expectation is to have
`git submodule {init, update --init, sync}`
to pay attention to url..insteadOf when setting up the
submodule..URL, such that the modified URL is used for the
initial clone of the submodule (and hence any subsequent usage within
the submodule).

That sounds like a good idea to me.

Two caveates:

* After running `git submodule init`, you change url..insteadOf
  in the superproject. How do we need to word the documentation to
  have users expecting this change doesn't affect submodules?
  (See above Any vs. "Any except (initialized) submodules")

* So with the point above the insteadOf config only applies to the
  init/sync process, (i.e. once in time, ideally).
  Is that confusing or actually simplifying the submodule workflow?

Thanks,
Stefan

Re: [PATCH v2] config: preserve case for one-shot config on the command line

[Stefan Beller]

On Tue, Feb 21, 2017 at 9:50 AM, Jeff King  wrote:
> On Tue, Feb 21, 2017 at 09:17:07AM -0800, Junio C Hamano wrote:
>
>>  * Changes from v1:
>>
>>- update the comment before the second loop to find the last
>>  dot.
>>
>>- move two new tests into existing t1300 (at least for now).
>>
>>- make it clear that the second of the new tests check two
>>  aspects of "three level vars" by setting up the expectation for
>>  the first half near the actual tests and adding comments on
>>  what it tests before the second half.
>
> Thanks, this addresses all of my (admittedly minor) concerns.
>
> -Peff

This patch looks different than what I last looked at. I like it.
Though the manual search of the last dot instead of using
strrchr seems to be over-optimizing to me.
Anyway it is still very understandable.

Thanks,
Stefan

Re: [PATCH v2] config: preserve case for one-shot config on the command line

[Jeff King]

On Tue, Feb 21, 2017 at 09:17:07AM -0800, Junio C Hamano wrote:

>  * Changes from v1:
> 
>- update the comment before the second loop to find the last
>  dot.
> 
>- move two new tests into existing t1300 (at least for now).
> 
>- make it clear that the second of the new tests check two
>  aspects of "three level vars" by setting up the expectation for
>  the first half near the actual tests and adding comments on
>  what it tests before the second half.

Thanks, this addresses all of my (admittedly minor) concerns.

-Peff

Re: Inconsistent results of git blame --porcelain when detecting copies from other files

[Jeff King]

On Tue, Feb 21, 2017 at 12:25:37PM +, Sokolov, Konstantin wrote:

> Thanks for going into the issue. As far as I understand 2.12 won't
> change the discussed behavior of --procelain. We will switch to
> --line-procelain. After the current discussion it seems to be less
> error prone, more future-proof and our current parser can handle it
> without any changes.

Right, the 2.12 change is only fixing a case where the "previous"
key/value line was not repeated at the correct spots. The same parsing
rules still hold.

-Peff

Re: [RFC] Subtle differences in passing configs to git clone

[Jeff King]

On Tue, Feb 21, 2017 at 12:36:25PM +0100, Lars Schneider wrote:

> I stumbled across the following today:
> 
> (1) git -c foo.bar="foobar" clone 
> 
> --> uses the config temporarily
> 
> 
> (2) git clone -c foo.bar="foobar" 
> 
> --> uses the config and writes it to .git/config
> 
> This was introduced in 84054f7 ("clone: accept config options on the 
> command line") and it makes total sense.

Yep, they were designed to match.

> However, I think this subtitle difference can easily confuse users.
> 
> I think we should tell the users that we've written to .git/config.
> Maybe something like this:
> 
> git clone -c foo.bar="foobar" 
> Cloning into 'test'...
> Writing foo.bar="foobar" to local config...
> remote: Counting objects: 2152, done.
> remote: Compressing objects: 100% (33/33), done.
> remote: Total 2152 (delta 19), reused 0 (delta 0), pack-reused 2119
> Receiving objects: 100% (2152/2152), 328.66 KiB | 217.00 KiB/s, done.
> Resolving deltas: 100% (1289/1289), done.
> 
> What do you think?

 I don't find it confusing, but I can see how one might. Since
"clone" is already pretty chatty, I don't mind adding the extra message.

-Peff

[PATCH v2] config: preserve case for one-shot config on the command line

[Junio C Hamano]

The "git -c = cmd" mechanism is to pretend that a
configuration variable  is set to  while the cmd is
running.  The code to do so however downcased  in its entirety,
which is wrong for a three-level ...

The  part needs to stay as-is.

Reported-by: Lars Schneider 
Diagnosed-by: Jonathan Tan 
Helped-by: Jeff King 
Signed-off-by: Junio C Hamano 
---

 * Changes from v1:

   - update the comment before the second loop to find the last
 dot.

   - move two new tests into existing t1300 (at least for now).

   - make it clear that the second of the new tests check two
 aspects of "three level vars" by setting up the expectation for
 the first half near the actual tests and adding comments on
 what it tests before the second half.

 config.c   | 30 +-
 t/t1300-repo-config.sh | 46 ++
 2 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 0dfed682b8..4128debc71 100644
--- a/config.c
+++ b/config.c
@@ -199,6 +199,34 @@ void git_config_push_parameter(const char *text)
strbuf_release();
 }
 
+/*
+ * downcase the  and  in . or
+ * .. and do so in place.  
+ * is left intact.
+ */
+static void canonicalize_config_variable_name(char *varname)
+{
+   char *cp, *last_dot;
+
+   /* downcase the first segment */
+   for (cp = varname; *cp; cp++) {
+   if (*cp == '.')
+   break;
+   *cp = tolower(*cp);
+   }
+   if (!*cp)
+   return;
+
+   /* find the last dot (we start from the first dot we just found) */
+   for (last_dot = cp; *cp; cp++)
+   if (*cp == '.')
+   last_dot = cp;
+
+   /* downcase the last segment */
+   for (cp = last_dot; *cp; cp++)
+   *cp = tolower(*cp);
+}
+
 int git_config_parse_parameter(const char *text,
   config_fn_t fn, void *data)
 {
@@ -221,7 +249,7 @@ int git_config_parse_parameter(const char *text,
strbuf_list_free(pair);
return error("bogus config parameter: %s", text);
}
-   strbuf_tolower(pair[0]);
+   canonicalize_config_variable_name(pair[0]->buf);
if (fn(pair[0]->buf, value, data) < 0) {
strbuf_list_free(pair);
return -1;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 923bfc5a26..7a16f66a9d 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1097,6 +1097,52 @@ test_expect_success 'multiple git -c appends config' '
test_cmp expect actual
 '
 
+test_expect_success 'last one wins: two level vars' '
+
+   # sec.var and sec.VAR are the same variable, as the first
+   # and the last level of a configuration variable name is
+   # case insensitive.
+
+   echo VAL >expect &&
+
+   git -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&
+   test_cmp expect actual &&
+   git -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&
+   test_cmp expect actual &&
+
+   git -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&
+   test_cmp expect actual &&
+   git -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'last one wins: three level vars' '
+
+   # v.a.r and v.A.r are not the same variable, as the middle
+   # level of a three-level configuration variable name is
+   # case sensitive.
+
+   echo val >expect &&
+   git -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&
+   test_cmp expect actual &&
+   git -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&
+   test_cmp expect actual &&
+
+   # v.a.r and V.a.R are the same variable, as the first
+   # and the last level of a configuration variable name is
+   # case insensitive.
+
+   echo VAL >expect &&
+   git -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&
+   test_cmp expect actual &&
+   git -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&
+   test_cmp expect actual &&
+   git -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&
+   test_cmp expect actual &&
+   git -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'git -c is not confused by empty environment' '
GIT_CONFIG_PARAMETERS="" git -c x.one=1 config --list
 '
-- 
2.12.0-rc2-221-ga92f6f5816

Re: [PATCH] config: preserve case for one-shot config on the command line

[Junio C Hamano]

Jeff King  writes:

>> +. ./test-lib.sh
>
> There are a bunch of other "git -c" tests inside t1300. I don't know if
> it would be better to put them all together.

I considered it, but it is already very long and I suspect it would
be better in the longer term to split the command-line one out into
a separate script, for the reasons you state.

I can move these at the end of 1300 in the meantime, and leave the
split for somebody else to be done later.

> Arguably, those other ones should get pulled out here to the new script.
> More scripts means that the tests have fewer hidden dependencies, and we
> can parallelize the run more. I admit I've shied away from new scripts
> in the past because the number allocation is such a pain.
>
>> +test_expect_success 'last one wins: two level vars' '
>> +echo VAL >expect &&
>> +
>> +# sec.var and sec.VAR are the same variable, as the first
>> +# and the last level of a configuration variable name is
>> +# case insensitive.  Test both setting and querying.
>
> I assume by "setting and querying" here you mean "setting via -c,
> querying via git-config".

Yes.  Should I update it to be more explicit?

> There are two blocks of tests, each with their own "expect" value.
> Should the top "expect" here be moved down with its block to make that
> more clear?

Perhaps.  Let me try that one.

Thanks.

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

[Junio C Hamano]

Thomas Gummerer  writes:

> - git reset --hard ${GIT_QUIET:+-q}

This hunk is probably the most important one to review in the whole
series, in the sense that these are entirely new code that didn't
exist in the original.

> + if test $# != 0
> + then
> + saved_untracked=
> + if test -n "$(git ls-files --others -- "$@")"

I notice that "ls-files -o" used in the code before this series are
almost always used with --exclude-standard but we do not set up the
standard exclude pattern here.  Is there a good reason to use (or
not to use) it here as well?

> + then
> + saved_untracked=$(
> + git ls-files -z --others -- "$@" |
> + xargs -0 git stash create -u all --)
> + fi

Running the same ls-files twice look somewhat wasteful.

I suspect that we avoid "xargs -0" in our code from portability
concern (isn't it a GNU invention?)

> + git ls-files -z -- "$@" | xargs -0 git reset 
> ${GIT_QUIET:+-q} --

Hmm, am I being naive to suspect that the above is a roundabout way
to say:

git reset ${GIT_QUIET:+-q} -- "$@"

or is an effect quite different from that intended here?

> + git ls-files -z --modified -- "$@" | xargs -0 git 
> checkout ${GIT_QUIET:+-q} HEAD --

Likewise.  Wouldn't the above be equivalent to:

git checkout ${GIT_QUIET:+-q} HEAD -- "$@"

Or is this trying to preserve paths modified in the working tree and
fully added to the index?


> + if test -n "$(git ls-files -z --others -- "$@")"
> + then
> + git ls-files -z --others -- "$@" | xargs -0 git 
> clean --force -d ${GIT_QUIET:+-q} --

Likewise.  "ls-files --others" being the major part of what "clean"
is about, I suspect the "ls-files piped to clean" is redundant.  Do
you even need a test?  IOW, doesn't "git clean" with a pathspec that
does not match anything silently succeed without doing anything
harmful?

> + fi
> + if test -n "$saved_untracked"
> + then
> + git stash pop -q $saved_untracked

I see this thing was "created", and the whole point of "create" is
to be different from "save/push" that automatically adds the result
to the stash reflog.  Should we be "pop"ing it, or did you mean to
just call apply_stash on it?

> + fi
> + else
> + git reset --hard ${GIT_QUIET:+-q}
> + fi

Re: [PATCH v2 0/4] delete_ref: support reflog messages

[Kyle Meyer]

Junio C Hamano  writes:

> These looked reasonable.  I had to resolve conflicts with another
> topic in flight that removed set_worktree_head_symref() function
> while queuing, and I think I resolved it correctly, but please
> double check what is pushed out on 'pu'.

The merge looks right to me, thanks.  Thanks also for touching up the
tab/space issue in t3200-branch.sh.

-- 
Kyle

Re: Git trademark status and policy

[G. Sylvie Davies]

On Wed, Feb 1, 2017 at 6:26 PM, Jeff King  wrote:
> As many of you already know, the Git project (as a member of Software
> Freedom Conservancy) holds a trademark on "Git".  This email will try to
> lay out a bit of the history and procedure around the enforcement of
> that trademark, along with some open questions about policy.
>
> I'll use "we" in the text below, which will generally mean the Git
> Project Leadership Committee (PLC). I.e., the people who represent the
> Git project as part of Conservancy -- me, Junio Hamano, and Shawn
> Pearce.
>
> We approached Conservancy in Feb 2013 about getting a trademark on Git
> to ensure that anything calling itself "Git" remained interoperable with
> Git. Conservancy's lawyer drafted the USPTO application and submitted it
> that summer. The trademark was granted in late 2014 (more on that delay
> in a moment).
>
> Concurrently, we developed a written trademark policy, which you can
> find here:
>
>   https://git-scm.com/trademark
>
> This was started from a template that Conservancy uses and customized by
> Conservancy and the Git PLC.
>
> While the original idea was to prevent people from forking the
> software, breaking compatibility, and still calling it Git, the policy
> covers several other cases.
>
> One is that you can't imply successorship. So you also can't fork the
> software, call it "Git++", and then tell everybody your implementation
> is the next big thing.
>
> Another is that you can't use the mark in a way that implies association
> with or endorsement by the Git project. To some degree this is necessary
> to prevent dilution of the mark for other uses, but there are also cases
> we directly want to prevent.
>
> For example, imagine a software project which is only tangentially
> related to Git. It might use Git as a side effect, or might just be
> "Git-like" in the sense of being a distributed system with chained
> hashes. Let's say as an example that it does backups. We'd prefer it
> not call itself GitBackups. We don't endorse it, and it's just using the
> name to imply association that isn't there. You can come up with similar
> hypotheticals: GitMail that stores mailing list archives in Git, or
> GitWiki that uses Git as a backing store.
>
> Those are all fictitious examples (actually, there _are_ real projects
> that do each of those things, but they gave themselves much more unique
> names). But they're indicative of some of the cases we've seen. I'm
> intentionally not giving the real names here, because my point isn't to
> shame any particular projects, but to discuss general policy.
>
> Careful readers among you may now be wondering about GitHub, GitLab,
> Gitolite, etc. And now we get back to why it took over a year to get the
> trademark granted.
>
> The USPTO initially rejected our application as confusingly similar to
> the existing trademark on GitHub, which was filed in 2008. While one
> might imagine where the "Git" in GitHub comes from, by the time we
> applied to the USPTO, both marks had been widely used in parallel for
> years.  So we worked out an agreement with GitHub which basically says
> "we are mutually OK with the other trademark existing".
>
> (There was another delay caused by a competing application from a
> proprietary version control company that wanted to re-brand portions of
> their system as "GitFocused" (not the real name, but similar in spirit).
> We argued our right to the name and refused to settle; they eventually
> withdrew their application).
>
> So GitHub is essentially outside the scope of the trademark policy, due
> to the history. We also decided to explicitly grandfather some major
> projects that were using similar portmanteaus, but which had generally
> been good citizens of the Git ecosystem (building on Git in a useful
> way, not breaking compatibility). Those include GitLab, JGit, libgit2,
> and some others. The reasoning was generally that it would be a big pain
> for those projects, which have established their own brands, to have to
> switch names. It's hard to hold them responsible for picking a name that
> violated a policy that didn't yet exist.
>
> If the "libgit2" project were starting from scratch today, we'd probably
> ask it to use a different name (because the name may imply that it's an
> official successor). However, we effectively granted permission for this
> use and it would be unfair to disrupt that.
>
> There's one other policy point that has come up: the written policy
> disallows the use of "Git" or the logo on merchandise. This is something
> people have asked about it (e.g., somebody made some Git stress balls,
> and another person was printing keycaps with a Git logo). We have always
> granted it, but wanted to reserve the right in case there was some use
> that we hadn't anticipated that would be confusing or unsavory.
>
> Enforcement of the policy is done as cases are brought to the attention
> of Conservancy and the Git PLC. Sometimes people mail 

Re: Partnership with Git

[Dov Grobgeld]

Nikita,

As git is free software, you are free to use it in any way you see fit, as
long as you adhere to its licensing terms, and to the copyright
restrictions on using the term "git". Thus there is no need to ask
permission and there does not on the git side exist any entity interested
in "cross marketing activities". If your want to use git in your products,
you are free to do so without asking, and if you need technical advice for
how to go about integrating git in your products, you can either ask here,
on stackoverflow, (and probably lots of other places) or hire a consultant
that can help you.

Hope this helps,
Dov

On Tue, Feb 21, 2017 at 4:08 PM, Nikita Malikov  wrote:
>
> Konstantin,
>
> My goal is to establish partnership relations with Git because some of
> Devart's products support Git version control system (for example dbForge
> Studio for SQL Server https://www.devart.com/dbforge/sql/studio/).
> My team and I would be glad to come up with cross-marketing activities
> between Devart and Git. We think that some users of our development products
> would be interested to switch to Git version control system.
>
> Best regards
> Nikita Malikov
>
> --
> From: "Konstantin Khomoutov" 
> Sent: 21 February, 2017 14:22
> To: "Nikita Malikov" 
> Cc: 
> Subject: Re: Partnership with Git
>
>
>> On Tue, 21 Feb 2017 13:21:38 +0200
>> "Nikita Malikov"  wrote:
>>
>>> My name is Nikita and I'm from Devart https://www.devart.com/.
>>> I would like to contact someone from executive staff of Git. I would
>>> be very thankful for some information how to do this or if someone
>>> contacts me.
>>
>>
>> Git is a free software volunteer project and as such, it has no
>> "executive staff" in the sense enterprises put into it.
>>
>> Can you maybe state your actual goals please?
>>
>> Say, if you're looking for commercial support, we could possibly
>> suggests a couple of pointers.
>>
>>
>
>

Re: Partnership with Git

[Nikita Malikov]

Konstantin,

My goal is to establish partnership relations with Git because some of 
Devart's products support Git version control system (for example dbForge 
Studio for SQL Server https://www.devart.com/dbforge/sql/studio/).
My team and I would be glad to come up with cross-marketing activities 
between Devart and Git. We think that some users of our development products 
would be interested to switch to Git version control system.


Best regards
Nikita Malikov

--
From: "Konstantin Khomoutov" 
Sent: 21 February, 2017 14:22
To: "Nikita Malikov" 
Cc: 
Subject: Re: Partnership with Git


On Tue, 21 Feb 2017 13:21:38 +0200
"Nikita Malikov"  wrote:


My name is Nikita and I'm from Devart https://www.devart.com/.
I would like to contact someone from executive staff of Git. I would
be very thankful for some information how to do this or if someone
contacts me.


Git is a free software volunteer project and as such, it has no
"executive staff" in the sense enterprises put into it.

Can you maybe state your actual goals please?

Say, if you're looking for commercial support, we could possibly
suggests a couple of pointers.

Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()

[Duy Nguyen]

On Mon, Feb 20, 2017 at 7:11 PM, Michael Haggerty  wrote:
> On 02/18/2017 02:33 PM, Nguyễn Thái Ngọc Duy wrote:
>> Since submodule or not is irrelevant to files-backend after the last
>> patch, remove the parameter from files_downcast() and kill
>> files_assert_main_repository().
>>
>> PS. This implies that all ref operations are allowed for submodules. But
>> we may need to look more closely to see if that's really true...
>
> I think you are jumping the gun here.
>
> Even after your changes, there is still a significant difference between
> the main repository and submodules: we have access to the object
> database for the main repository, but not for submodules.

I did jump the gun for another reason: files-backend makes function
calls to the frontend, which unconditionally target the main ref store
(e.g. resolve_ref_unsafe, delete_ref...). Of course, because
store-aware api does not exist. My decision (off-list) to add
test-ref-store was the right call. I would not have seen these because
I was not (and still am not) familiar with files-backend.c enough to
see its dark corners.

files-backend.c is not all unicorn and rainbow :(
-- 
Duy

AW: Inconsistent results of git blame --porcelain when detecting copies from other files

[Sokolov, Konstantin]

Thanks for going into the issue. As far as I understand 2.12 won't change the 
discussed behavior of --procelain. We will switch to --line-procelain. After 
the current discussion it seems to be less error prone, more future-proof and 
our current parser can handle it without any changes.

Regards
Konstantin

-Ursprüngliche Nachricht-
Von: Jeff King [mailto:p...@peff.net]
Gesendet: Montag, 20. Februar 2017 23:16
An: Junio C Hamano
Cc: Sokolov, Konstantin (ext) (CT RDA SSI ADM-DE); git@vger.kernel.org
Betreff: Re: Inconsistent results of git blame --porcelain when detecting 
copies from other files

On Mon, Feb 20, 2017 at 01:30:29PM -0800, Junio C Hamano wrote:

> "Sokolov, Konstantin"  writes:
>
> > However, when using --porcelain DirectoryReader.java is reported as the 
> > origin of lines 502-504:
> > ...
> > This is not only inconsistent with the other outputs but the output is also 
> > inconsistent in itself because lines 496 -498 do not even exist in a 
> > previous version of DirectoryReader.java.
>
> Hmph, this sounds vaguely familiar with
>
>
> http://public-inbox.org/git/20170106042051.nwjiuyyp7ljhs...@sigill.int
> ra.peff.net
>
> which is part of Git 2.12-rc0

Yeah, I had the same thought while reading Konstantin's report.

I'm not sure Git is wrong, though. I think it's just the way the porcelain 
output works.

Here's a minimal reproduction; the interesting thing is when a commit is 
mentioned twice (as happens on lines 1 and 5 here):

  git init repo
  cd repo

  # use long lines to make sure we trigger content-movement detection
  for i in $(seq 1 5); do
echo this is really long line number $i
  done >file
  git add file
  git commit -m initial

  sed 's/1/one/; s/5/five/' renamed
  git rm file
  git add renamed
  git commit -m 'rename and use english'

  git blame renamed
  git blame --line-porcelain renamed
  git blame --porcelain renamed

The first blame output looks something like this:

  bab03701 renamed ... line number 1
  ^dda1349 file... line number 2
  ^dda1349 file... line number 3
  ^dda1349 file... line number 4
  bab03701 renamed ... line number 5

so we can see it's the same case. The --line-porcelain similarly matches the 
commits and filenames.

But the --porcelain output is:

  bab037010dcabaf0509db27bf232d25659b180fa 1 1 1
  ...
  filename renamed
  this is really long line number one
  dda1349d41da859f4c37e018dbed714ba6c1aa18 2 2 3
  ...
  filename file
  this is really long line number 2
  dda1349d41da859f4c37e018dbed714ba6c1aa18 3 3
  this is really long line number 3
  dda1349d41da859f4c37e018dbed714ba6c1aa18 4 4
  this is really long line number 4
  bab037010dcabaf0509db27bf232d25659b180fa 5 5 1
  this is really long line number five

You might be tempted to say that the fifth line comes from "filename file", 
because that was the last "filename" entry we saw. But that's _not_ how the 
porcelain output works. That "filename" entry was associated with dda1349, but 
the line comes from bab0370 here.

The simplest way (IMHO) to parse --porcelain output is:

  - maintain a mapping of commit sha1s to the commit's details

  - whenever you see a "   []"
line, any key-value fields which follow impact _only_ that sha1, and
you should update the details for that map entry

  - when you see the actual tab-indented line content, you have gotten
all of the key-value updates for that sha1. You can now safely do
what you like with the line entry.

Another way, if you don't want to update your mapping, is to actually pay 
attention to the size-of-hunk headers. In this case the middle three lines come 
in their own hunk (which you can see from the "2 2 3" header on the second 
line). The "filename" field we get applies to that hunk, but once we switch to 
a different one, the filename field needs to be looked up in the commit mapping.

But it's definitely not correct to blindly apply one "filename" field to 
subsequent lines in other hunks.

And yes, I do think this is probably more complex than it needs to be.
I didn't write it. And I don't think it is worth the backwards compatibility 
headache of trying to change it now. It's possible this could be better 
documented (I didn't look at the documentation to write that explanation; I 
happened to puzzle it out for somebody else recently who had a similar case. 
That's what led to the bug-fix in the message you linked).

-Peff

Re: Partnership with Git

[Konstantin Khomoutov]

On Tue, 21 Feb 2017 13:21:38 +0200
"Nikita Malikov"  wrote:

> My name is Nikita and I'm from Devart https://www.devart.com/.
> I would like to contact someone from executive staff of Git. I would
> be very thankful for some information how to do this or if someone
> contacts me.

Git is a free software volunteer project and as such, it has no
"executive staff" in the sense enterprises put into it.

Can you maybe state your actual goals please?

Say, if you're looking for commercial support, we could possibly
suggests a couple of pointers.

Partnership with Git

[Nikita Malikov]

Hello,

My name is Nikita and I'm from Devart https://www.devart.com/.
I would like to contact someone from executive staff of Git. I would be very 
thankful for some information how to do this or if someone contacts me.


Thanks for attention and sorry if I disturbed someone.

Best regards
Nikita Malikov 

[RFC] Subtle differences in passing configs to git clone

[Lars Schneider]

Hi,

I stumbled across the following today:

(1) git -c foo.bar="foobar" clone 

--> uses the config temporarily


(2) git clone -c foo.bar="foobar" 

--> uses the config and writes it to .git/config


This was introduced in 84054f7 ("clone: accept config options on the 
command line") and it makes total sense. However, I think this subtitle
difference can easily confuse users.

I think we should tell the users that we've written to .git/config.
Maybe something like this:

git clone -c foo.bar="foobar" 
Cloning into 'test'...
Writing foo.bar="foobar" to local config...
remote: Counting objects: 2152, done.
remote: Compressing objects: 100% (33/33), done.
remote: Total 2152 (delta 19), reused 0 (delta 0), pack-reused 2119
Receiving objects: 100% (2152/2152), 328.66 KiB | 217.00 KiB/s, done.
Resolving deltas: 100% (1289/1289), done.

What do you think?

Thanks,
Lars

Not expected merge conflict output

[KES]

Hi. I have merge conflict and this output:

--- a/crypto/lib/Crypto/Routes.pm
+++ b/crypto/lib/Crypto/Routes.pm
@@@ -98,17 -94,16 +98,36 @@@ sub register 
  ,payment_cancel_landing  =>  '/payment-cancel'
  }});
  # Route configuration is moved from plugin:
++<<< ours
 +$rn->get( '/stripe/payment_success' )->to( 'Stripe#payment_success' 
)->name( 'stripe_payment_s
 +$rn->get( '/stripe/payment_cancel'  )->to( 'Stripe#payment_cancel'  
)->name( 'stripe_payment_c
 +$rn->options( '/stripe' )->to( 'Stripe#options' )->name( 'stripe_options' 
);
 +$rn->post( '/stripe/payment/create', { required_level => 'user' } )->to( 
'Stripe#payment_creat
 +$rn->post( '/stripe/payment/execute', { required_level => 'user' } )->to( 
'Stripe#payment_exec
++||| base
++$guest->get( '/stripe/payment_success' )->to( 'Stripe#payment_success' 
)->name( 'stripe_paymen
++$guest->get( '/stripe/payment_cancel'  )->to( 'Stripe#payment_cancel'  
)->name( 'stripe_paymen
++$guest->options( '/stripe' )->to( 'Stripe#options' )->name( 
'stripe_options' );
++$user->post( '/stripe/payment/create'  )->to( 'Stripe#payment_create'  
)->name( 'stripe_paymen
++$user->post( '/stripe/payment/execute' )->to( 'Stripe#payment_execute' 
)->name( 'stripe_paymen
++
++$guest->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' 
);
++===
+ $guest->get( '/stripe/payment_success' )->to( 'Stripe#payment_success' 
)->name( 'stripe_paymen
+ $guest->get( '/stripe/payment_cancel'  )->to( 'Stripe#payment_cancel'  
)->name( 'stripe_paymen
+ $guest->options( '/stripe' )->to( 'Stripe#options' )->name( 
'stripe_options' );
+ $user->post( '/stripe/payment/create'  )->to( 'Stripe#payment_create'  
)->name( 'stripe_paymen
+ $user->post( '/stripe/payment/execute' )->to( 'Stripe#payment_execute' 
)->name( 'stripe_paymen
+ 
+ $guest->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' 
);
+ $guest->get ( '/stripe/:form' )->to( 'Stripe#button' );
++>>> theirs
  
 +$rn->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' );
  
 +# Static routes
 +# QR code dowlnload url
 +$app->routes->get('/uploads/qrcode/:file')->name('qr_url');


But I expect this output:

--- a/crypto/lib/Crypto/Routes.pm
+++ b/crypto/lib/Crypto/Routes.pm
@@@ -98,17 -94,16 +98,36 @@@ sub register 
  ,payment_cancel_landing  =>  '/payment-cancel'
  }});
  # Route configuration is moved from plugin:
++<<< ours
 +$rn->get( '/stripe/payment_success' )->to( 'Stripe#payment_success' 
)->name( 'stripe_payment_s
 +$rn->get( '/stripe/payment_cancel'  )->to( 'Stripe#payment_cancel'  
)->name( 'stripe_payment_c
 +$rn->options( '/stripe' )->to( 'Stripe#options' )->name( 'stripe_options' 
);
 +$rn->post( '/stripe/payment/create', { required_level => 'user' } )->to( 
'Stripe#payment_creat
 +$rn->post( '/stripe/payment/execute', { required_level => 'user' } )->to( 
'Stripe#payment_exec
  
 +$rn->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' );
++||| base
++$guest->get( '/stripe/payment_success' )->to( 'Stripe#payment_success' 
)->name( 'stripe_paymen
++$guest->get( '/stripe/payment_cancel'  )->to( 'Stripe#payment_cancel'  
)->name( 'stripe_paymen
++$guest->options( '/stripe' )->to( 'Stripe#options' )->name( 
'stripe_options' );
++$user->post( '/stripe/payment/create'  )->to( 'Stripe#payment_create'  
)->name( 'stripe_paymen
++$user->post( '/stripe/payment/execute' )->to( 'Stripe#payment_execute' 
)->name( 'stripe_paymen
++
++$guest->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' 
);
++===
+ $guest->get( '/stripe/payment_success' )->to( 'Stripe#payment_success' 
)->name( 'stripe_paymen
+ $guest->get( '/stripe/payment_cancel'  )->to( 'Stripe#payment_cancel'  
)->name( 'stripe_paymen
+ $guest->options( '/stripe' )->to( 'Stripe#options' )->name( 
'stripe_options' );
+ $user->post( '/stripe/payment/create'  )->to( 'Stripe#payment_create'  
)->name( 'stripe_paymen
+ $user->post( '/stripe/payment/execute' )->to( 'Stripe#payment_execute' 
)->name( 'stripe_paymen
+ 
+ $guest->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' 
);
+ $guest->get ( '/stripe/:form' )->to( 'Stripe#button' );
++>>> theirs
  
 +# Static routes
 +# QR code dowlnload url
 +$app->routes->get('/uploads/qrcode/:file')->name('qr_url');

Because this diff has less difference between ours&, Also 

This (second expected patch)
 +# Static routes
 +# QR code dowlnload url
 +$app->routes->get('/uploads/qrcode/:file')->name('qr_url');
is less in compare to (first not expected):
  
 +$rn->post( '/stripe/hook' )->to( 'Stripe#hook' )->name( 'stripe_hook' );
  
 +# Static routes
 +# QR code dowlnload url
 +$app->routes->get('/uploads/qrcode/:file')->name('qr_url');


Did I wrong and first output is right thing or maybe I 

Did You Get My Message This Time?

[Friedrich Mayrhofer]

-- 

This is the second time i am sending you this mail.I, Friedrich Mayrhofer 
Donate $ 1,000,000.00 to You, Email  Me personally for more details.

Regards.
Friedrich Mayrhofer