Re: Non-zero exit code without error

[Serdar Sahin]

Thanks. I know it is hard to get an idea without being able to
reproduce it. I think there is no alternative without debugging it
with GDB.

I’ve tried your suggestions and nothing has changed.

I’ve also update the GIT as you suggested, but it is still the same.

Just to see, if GIT server causes some issues, I’ve pushed to repo to
github public as a private repo, and can reproduce the issue there as
well.

Thanks for your support.

On Tue, Feb 14, 2017 at 10:53 AM, Serdar Sahin  wrote:
> Hi Christian,
>
> Thanks. I know it is hard to get an idea without being able to reproduce it.
> I think there is no alternative without debugging it with GDB.
>
> I’ve tried your suggestions and nothing has changed.
>
> I’ve also update the GIT as you suggested, but it is still the same.
>
> Just to see, if GIT server causes some issues, I’ve pushed to repo to github
> public as a private repo, and can reproduce the issue there as well.
>
> Thanks for your support.
>
> --
> Serdar Sahin
> Peak Games
>
> On Saturday, 11 February 2017 at 15:28, Christian Couder wrote:
>
> On Wed, Feb 8, 2017 at 11:26 AM, Serdar Sahin  wrote:
>
> Hi Christian,
>
>
> We are using a private repo (Github Enterprise).
>
>
> Maybe you could try 'git fast-export --anonymize ...' on it.
>
> Let me give you the
> details you requested.
>
>
> On Git Server: git version 2.6.5.1574.g5e6a493
>
> On my client: git version 2.10.1 (Apple Git-78)
>
>
> I’ve tried to reproduce it with public repos, but couldn’t do so.
>
>
> You might try using the latest released version (2.11.1).
>
> For example you could install the last version on the client and then
> clone from the server with --bare and use this bare repo as if it was
> the server.
>
> You could also try `git fsck` to see if there are problems on your repo.
>
> Are there submodules or something a bit special?
>
> In the end it's difficult for us to help if we cannot reproduce, so
> your best bet might be to debug yourself using gdb for example.
>
> If I
> could get an error/log output, that would be sufficient.
>
>
> I am also including the full output below. (also git gc)
>
>
> MacOSX:test serdar$ git clone --mirror --depth 50 --no-single-branch
> g...@git.privateserver.com:Casual/code_repository.git
>
>
> You could also try with different options above...
>
> Cloning into bare repository 'code_repository.git'...
>
> remote: Counting objects: 3362, done.
>
> remote: Compressing objects: 100% (1214/1214), done.
>
> remote: Total 3362 (delta 2335), reused 2968 (delta 2094), pack-reused 0
>
> Receiving objects: 100% (3362/3362), 56.77 MiB | 1.83 MiB/s, done.
>
> Resolving deltas: 100% (2335/2335), done.
>
> MacOSX:test serdar$ cd code_repository.git/
>
> MacOSX:code_repository.git serdar$ GIT_CURL_VERBOSE=1 GIT_TRACE=1 git
> fetch --depth 50 origin cc086c96cdffe5c1ac78e6139a7a4b79e7c821ee
>
>
> ... and above.
>
> Also it looks like you use ssh so something like GIT_SSH_COMMAND="ssh
> -vv" might help more than GIT_CURL_VERBOSE=1
>
> 13:23:15.648337 git.c:350 trace: built-in: git 'fetch'
> '--depth' '50' 'origin' 'cc086c96cdffe5c1ac78e6139a7a4b79e7c821ee'
>
> 13:23:15.651127 run-command.c:336 trace: run_command: 'ssh'
> 'g...@git.privateserver.com' 'git-upload-pack
> '\''Casual/code_repository.git'\'''
>
> 13:23:17.750015 run-command.c:336 trace: run_command: 'gc' '--auto'
>
> 13:23:17.750829 exec_cmd.c:189 trace: exec: 'git' 'gc' '--auto'
>
> 13:23:17.753983 git.c:350 trace: built-in: git 'gc' '--auto'
>
> MacOSX:code_repository.git serdar$ echo $?
>
> 1
>
> MacOSX:code_repository.git serdar$ GIT_CURL_VERBOSE=1 GIT_TRACE=1 git gc
> --auto
>
> 13:23:45.899038 git.c:350 trace: built-in: git 'gc' '--auto'
>
> MacOSX:code_repository.git serdar$ echo $?
>
> 0
>
>

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Eric Wong]

Arif Khokar  wrote:
> On 02/13/2017 09:37 AM, Johannes Schindelin wrote:
> >I actually had expected *you* to put in a little bit of an effort, too. In
> >fact, I was very disappointed that you did not even look into porting that
> >script to use public-inbox instead of GMane.
> 
> I wasn't aware of that expectation.  My idea was to use NNTP as a way to
> facilitate the development of a new git utility that would serve as the
> inverse of git-send-email (sort of like the relationship between git
> format-patch and git am), rather than using a

Speaking for myself, I usually don't expect much, especially
from newcomers.  So I am disappointed to see Dscho's disappointment
aimed at you, Arif.  Especially since you're not a regular and
we have no idea how much free time, attention span, or familiarity
with Bourne shell you have.

> IIRC, I had posted some proof-of-concept Perl code to do so back in August
> in 
> 
> 
> Looking at public-inbox now at the archives of this group, it appears that
> several of the messages I sent weren't archived for some reason (and I
> didn't see any more responses to what I posted at the time).  The messages
> are accessible via NNTP when connecting to gmane though.

It looks like it went to gmane via the m...@public-inbox.org to
gmane.mail.public-inbox.general mirror, not via the git@vger mirror.
I can't find it on git@vger's mail-archive.com mirror, either:

https://mail-archive.com/search?q=Arif+Khokar&l=git%40vger.kernel.org

> Also, looking at the source of the message I referenced, it appears that my
> MUA decided to base64 encode the message for some reason (which may have
> resulted in it getting filtered by those who I sent the message to).

It probably wasn't base64, but maybe it was one of these:
http://vger.kernel.org/majordomo-taboos.txt

Or it was the SPF softfail which you can see in the headers on both
gmane and public-inbox.
It might even be the '_' (underscore) in your other address.
But even Junio gets dropped by vger sometimes:
https://public-inbox.org/git/20170127035753.GA2604@dcvr/

But if I had to guess, vger gets hit by truckloads of spam and
the the backscatter volume could become unimaginable, so perhaps
it has good reason to discard silently.



Anyways, the eventual goal of public-inbox is to flip the
mailing list model backwards into "archives first" mode,
so a message needs to make it into public archives before
it goes out to subscribers.  That might prevent or avoid
such problems... *shrug*

Re: Developing git with IDE

[Jeff King]

On Tue, Feb 14, 2017 at 12:15:00AM +0100, Oleg Taranenko wrote:

> being last decade working with java & javascript I completely lost
> relation to c/c++ world. Trying to get into git internals I'm facing
> with issue what IDE is more suitable for developing git @ macos ?
> 
> Have googled, but any my search queries following to non-relevant
> themes, like supporting git in IDEs etc.
> 
> my first attempt - CLion (as far as I'm Jetbrains fan) - got failed,
> as far as doesn't support makefile-based projects, only CMake.
> 
> There are a number of free C/C++ dev tools: Xcode, CodeBlocks,
> CodeLite. Gnat, Qt creator, Dev C++, C++ Builder (Borland? :),
> Eclipse, NetBeans... what else?
> 
> Because of  lack my modern C experience, could somebody share his own
> attempts/thoughts/use cases how more convenient dive into the git
> development process on the Mac?

I think most people just use a good editor (emacs or vim), and no IDE. I
do recommend using ctags or similar (and there is a "make tags" target
to build the tags) to help with jumping between functions.

> Tried to find in the git distribution Documentation more information
> about this, nothing as well...  Would be nice to have a kind of
> 'Getting Started Manual'

There is Documentation/CodingGuidelines, but that's mostly about how to
_write_ code, not read it. Some protocols and subsystems are covered in
Documentation/technical. If you want a "big picture", I think you'd do
best to read something like:

  https://git-scm.com/book/en/v2/Git-Internals-Plumbing-and-Porcelain

That talks about the system as a whole, not the code, but the layout of
the code follows the overall system design (e.g., the entry point for
the "log" command is cmd_log(), and you can see which subsystems it uses
from there).

-Peff

Re: [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR

[Jeff King]

On Thu, Dec 29, 2016 at 07:48:45PM -0500, Jeff King wrote:

> On Thu, Dec 29, 2016 at 04:11:14PM -0800, Jonathan Nieder wrote:
> 
> > Thanks.  Here's the patch again, now with commit messages and a test.
> > Thanks for the analysis and sorry for the slow turnaround.
> 
> Thanks for following up. While working on a similar one recently, I had
> the nagging feeling that there was a case we had found that was still to
> be dealt with, and I think this was it. :)
> 
> The patch to the C code looks good. I have a few comments on the tests:

I happened to run into this problem myself today, so I thought I'd prod
you. I think your patch looks good. Hopefully I didn't scare you off
with my comments. :)

-Peff

Re: [PATCH] mingw: use OpenSSL's SHA-1 routines

[Johannes Sixt]

Am 13.02.2017 um 23:38 schrieb Johannes Schindelin:

In addition, you build from a custom MINGW/MSys1 setup, correct?


Correct. Specifically, I use the build tools from "msysgit" times, but 
build outside the premanufactured build environement; i.e., the 
"THIS_IS_MSYSGIT" section in config.mak.uname is not used.


-- Hannes

Re: [PATCH 0/7] grep rev/path parsing fixes

[Jeff King]

On Tue, Feb 14, 2017 at 01:00:21AM -0500, Jeff King wrote:

> On Mon, Feb 13, 2017 at 08:20:37PM -0500, Jeff King wrote:
> 
> > > If there is a repo and "foo" is a rev, the "--no-index or --untracked
> > > cannot be used with revs." error would occur. If there is a repo and
> > > "foo" is not a rev, this command would proceed as usual. If there is no
> > > repo, the "setup_git_env called without repository" error would occur.
> > > (This is my understanding from reading the code - I haven't tested it
> > > out.)
> > 
> > Yes, it's easy to see that "git grep --no-index foo bar" outside of a
> > repo generates the same BUG. I suspect that "--no-index" should just
> > disable looking up revs entirely, even if we are actually in a
> > repository directory.
> 
> I've fixed that, along with a few other bugs and cleanups. The complete
> series is below. Patch 2 is your (untouched) patch. My suggestions for
> your test are in patch 3, which can either remain on its own or be
> squashed in.
> 
>   [1/7]: grep: move thread initialization a little lower
>   [2/7]: grep: do not unnecessarily query repo for "--"
>   [3/7]: t7810: make "--no-index --" test more robust
>   [4/7]: grep: re-order rev-parsing loop
>   [5/7]: grep: fix "--" rev/pathspec disambiguation
>   [6/7]: grep: avoid resolving revision names in --no-index case
>   [7/7]: grep: do not diagnose misspelt revs with --no-index
> 
>  builtin/grep.c  | 78 
> +++--
>  t/t7810-grep.sh | 66 
>  2 files changed, 119 insertions(+), 25 deletions(-)

Just to clarify: these are all existing bugs, and I think these are
probably maint-worthy patches (even the --no-index ones; though we don't
BUG on the out-of-repo without the patch from 'next', the code is still
doing the wrong thing in subtle ways).

But AFAIK they are all much older bugs than the upcoming v2.12, so there
is no pressing need to fit them into the upcoming release.

-Peff

[PATCH 7/7] grep: do not diagnose misspelt revs with --no-index

[Jeff King]

If we are using --no-index, then our arguments cannot be
revs in the first place. Not only is it pointless to
diagnose them, but if we are not in a repository, we should
not be trying to resolve any names.

Signed-off-by: Jeff King 
---
 builtin/grep.c  | 2 +-
 t/t7810-grep.sh | 5 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index c4c632594..1454bef49 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1201,7 +1201,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
if (!seen_dashdash) {
int j;
for (j = i; j < argc; j++)
-   verify_filename(prefix, argv[j], j == i);
+   verify_filename(prefix, argv[j], j == i && use_index);
}
 
parse_pathspec(&pathspec, 0,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index c051c7ee8..0ff9f6cae 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1043,6 +1043,11 @@ test_expect_success 'grep --no-index prefers paths to 
revs' '
test_cmp expect actual
 '
 
+test_expect_success 'grep --no-index does not "diagnose" revs' '
+   test_must_fail git grep --no-index o :1:hello.c 2>err &&
+   test_i18ngrep ! -i "did you mean" err
+'
+
 cat >expected <

[PATCH 6/7] grep: avoid resolving revision names in --no-index case

[Jeff King]

We disallow the use of revisions with --no-index, but we
don't actually check and complain until well after we've
parsed the revisions.

This is the cause of a few problems:

 1. We shouldn't be calling get_sha1() at all when we aren't
in a repository, as it might access the ref or object
databases. For now, this should generally just return
failure, but eventually it will become a BUG().

 2. When there's a "--" disambiguator and you're outside a
repository, we'll complain early with "unable to resolve
revision". But we can give a much more specific error.

 3. When there isn't a "--" disambiguator, we still do the
normal rev/path checks. This is silly, as we know we
cannot have any revs with --no-index. Everything we see
must be a path.

Outside of a repository this doesn't matter (since we
know it won't resolve), but inside one, we may complain
unnecessarily if a filename happens to also match a
refname.

This patch skips the get_sha1() call entirely in the
no-index case, and behaves as if it failed (with the
exception of giving a better error message).

Signed-off-by: Jeff King 
---
 builtin/grep.c  |  6 ++
 t/t7810-grep.sh | 13 +
 2 files changed, 19 insertions(+)

diff --git a/builtin/grep.c b/builtin/grep.c
index e83b33bda..c4c632594 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
break;
}
 
+   if (!use_index) {
+   if (seen_dashdash)
+   die(_("--no-index cannot be used with revs"));
+   break;
+   }
+
if (get_sha1_with_context(arg, 0, sha1, &oc)) {
if (seen_dashdash)
die(_("unable to resolve revision: %s"), arg);
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index a6011f9b1..c051c7ee8 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1030,6 +1030,19 @@ test_expect_success 'grep --no-index pattern -- path' '
)
 '
 
+test_expect_success 'grep --no-index complains of revs' '
+   test_must_fail git grep --no-index o master -- 2>err &&
+   test_i18ngrep "no-index cannot be used with revs" err
+'
+
+test_expect_success 'grep --no-index prefers paths to revs' '
+   test_when_finished "rm -f master" &&
+   echo content >master &&
+   echo master:content >expect &&
+   git grep --no-index o master >actual &&
+   test_cmp expect actual
+'
+
 cat >expected <

[PATCH 5/7] grep: fix "--" rev/pathspec disambiguation

[Jeff King]

If we see "git grep pattern rev -- file" then we apply the
usual rev/pathspec disambiguation rules: any "rev" before
the "--" must be a revision, and we do not need to apply the
verify_non_filename() check.

But there are two bugs here:

  1. We keep a seen_dashdash flag to handle this case, but
 we set it in the same left-to-right pass over the
 arguments in which we parse "rev".

 So when we see "rev", we do not yet know that there is
 a "--", and we mistakenly complain if there is a
 matching file.

 We can fix this by making a preliminary pass over the
 arguments to find the "--", and only then checking the rev
 arguments.

  2. If we can't resolve "rev" but there isn't a dashdash,
 that's OK. We treat it like a path, and complain later
 if it doesn't exist.

 But if there _is_ a dashdash, then we know it must be a
 rev, and should treat it as such, complaining if it
 does not resolve. The current code instead ignores it
 and tries to treat it like a path.

This patch fixes both bugs, and tries to comment the parsing
flow a bit better.

It adds tests that cover the two bugs, but also some related
situations (which already worked, but this confirms that our
fixes did not break anything).

Signed-off-by: Jeff King 
---
 builtin/grep.c  | 29 -
 t/t7810-grep.sh | 33 +
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 461347adb..e83b33bda 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1149,7 +1149,22 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
 
compile_grep_patterns(&opt);
 
-   /* Check revs and then paths */
+   /*
+* We have to find "--" in a separate pass, because its presence
+* influences how we will parse arguments that come before it.
+*/
+   for (i = 0; i < argc; i++) {
+   if (!strcmp(argv[i], "--")) {
+   seen_dashdash = 1;
+   break;
+   }
+   }
+
+   /*
+* Resolve any rev arguments. If we have a dashdash, then everything up
+* to it must resolve as a rev. If not, then we stop at the first
+* non-rev and assume everything else is a path.
+*/
for (i = 0; i < argc; i++) {
const char *arg = argv[i];
unsigned char sha1[20];
@@ -1158,13 +1173,14 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
 
if (!strcmp(arg, "--")) {
i++;
-   seen_dashdash = 1;
break;
}
 
-   /* Stop at the first non-rev */
-   if (get_sha1_with_context(arg, 0, sha1, &oc))
+   if (get_sha1_with_context(arg, 0, sha1, &oc)) {
+   if (seen_dashdash)
+   die(_("unable to resolve revision: %s"), arg);
break;
+   }
 
object = parse_object_or_die(sha1, arg);
if (!seen_dashdash)
@@ -1172,7 +1188,10 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
add_object_array_with_path(object, arg, &list, oc.mode, 
oc.path);
}
 
-   /* The rest are paths */
+   /*
+* Anything left over is presumed to be a path. But in the non-dashdash
+* "do what I mean" case, we verify and complain when that isn't true.
+*/
if (!seen_dashdash) {
int j;
for (j = i; j < argc; j++)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 2c1f7373e..a6011f9b1 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -982,6 +982,39 @@ test_expect_success 'grep -e -- -- path' '
test_cmp expected actual
 '
 
+test_expect_success 'dashdash disambiguates rev as rev' '
+   test_when_finished "rm -f master" &&
+   echo content >master &&
+   echo master:hello.c >expect &&
+   git grep -l o master -- hello.c >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'dashdash disambiguates pathspec as pathspec' '
+   test_when_finished "git rm -f master" &&
+   echo content >master &&
+   git add master &&
+   echo master:content >expect &&
+   git grep o -- master >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'report bogus arg without dashdash' '
+   test_must_fail git grep o does-not-exist
+'
+
+test_expect_success 'report bogus rev with dashdash' '
+   test_must_fail git grep o hello.c --
+'
+
+test_expect_success 'allow non-existent path with dashdash' '
+   # We need a real match so grep exits with success.
+   tree=$(git ls-tree HEAD |
+  sed s/hello.c/not-in-working-tree/ |
+  git mktree) &&
+   git grep o "$tree" -- not-in-working-tree
+'
+
 test_expect_success 'grep --no-index pattern -- path' '
rm 

[PATCH 4/7] grep: re-order rev-parsing loop

[Jeff King]

We loop over the arguments, but every branch of the loop
hits either a "continue" or a "break". Surely we can make
this simpler.

The final conditional is:

  if (arg is a rev) {
  ... handle rev ...
  continue;
  }
  break;

We can rewrite this as:

  if (arg is not a rev)
  break;

  ... handle rev ...

That makes the flow a little bit simpler, and will make
things much easier to follow when we add more logic in
future patches.

Signed-off-by: Jeff King 
---
 builtin/grep.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 081e1b57a..461347adb 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1154,20 +1154,22 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
const char *arg = argv[i];
unsigned char sha1[20];
struct object_context oc;
+   struct object *object;
+
if (!strcmp(arg, "--")) {
i++;
seen_dashdash = 1;
break;
}
-   /* Is it a rev? */
-   if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
-   struct object *object = parse_object_or_die(sha1, arg);
-   if (!seen_dashdash)
-   verify_non_filename(prefix, arg);
-   add_object_array_with_path(object, arg, &list, oc.mode, 
oc.path);
-   continue;
-   }
-   break;
+
+   /* Stop at the first non-rev */
+   if (get_sha1_with_context(arg, 0, sha1, &oc))
+   break;
+
+   object = parse_object_or_die(sha1, arg);
+   if (!seen_dashdash)
+   verify_non_filename(prefix, arg);
+   add_object_array_with_path(object, arg, &list, oc.mode, 
oc.path);
}
 
/* The rest are paths */
-- 
2.12.0.rc1.471.ga79ec8999

[PATCH 3/7] t7810: make "--no-index --" test more robust

[Jeff King]

This makes sure that we actually use the pathspecs after
"--" correctly (as opposed to just seeing if grep errored
out).

Signed-off-by: Jeff King 
---
This could be squashed into the previous patch.

I didn't end up using the "nongit" helper, because we actually have to
do some other setup inside the subshell (this is the reason the earlier
tests in this script don't use the helper, either).

 t/t7810-grep.sh | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 29202f0e7..2c1f7373e 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -990,7 +990,10 @@ test_expect_success 'grep --no-index pattern -- path' '
export GIT_CEILING_DIRECTORIES &&
cd non/git &&
echo hello >hello &&
-   git grep --no-index o -- .
+   echo goodbye >goodbye &&
+   echo hello:hello >expect &&
+   git grep --no-index o -- hello >actual &&
+   test_cmp expect actual
)
 '
 
-- 
2.12.0.rc1.471.ga79ec8999

[PATCH 2/7] grep: do not unnecessarily query repo for "--"

[Jeff King]

From: Jonathan Tan 

When running a command of the form

  git grep --no-index pattern -- path

in the absence of a Git repository, an error message will be printed:

  fatal: BUG: setup_git_env called without repository

This is because "git grep" tries to interpret "--" as a rev. "git grep"
has always tried to first interpret "--" as a rev for at least a few
years, but this issue was upgraded from a pessimization to a bug in
commit 59332d1 ("Resurrect "git grep --no-index"", 2010-02-06), which
calls get_sha1 regardless of whether --no-index was specified. This bug
appeared to be benign until commit b1ef400 ("setup_git_env: avoid blind
fall-back to ".git"", 2016-10-20) when Git was taught to die in this
situation.  (This "git grep" bug appears to be one of the bugs that
commit b1ef400 is meant to flush out.)

Therefore, always interpret "--" as signaling the end of options,
instead of trying to interpret it as a rev first.

Signed-off-by: Jonathan Tan 
Signed-off-by: Jeff King 
---
Unchanged from your original.

 builtin/grep.c  |  9 +
 t/t7810-grep.sh | 12 
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 5a282c4d0..081e1b57a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
const char *arg = argv[i];
unsigned char sha1[20];
struct object_context oc;
+   if (!strcmp(arg, "--")) {
+   i++;
+   seen_dashdash = 1;
+   break;
+   }
/* Is it a rev? */
if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
struct object *object = parse_object_or_die(sha1, arg);
@@ -1162,10 +1167,6 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
add_object_array_with_path(object, arg, &list, oc.mode, 
oc.path);
continue;
}
-   if (!strcmp(arg, "--")) {
-   i++;
-   seen_dashdash = 1;
-   }
break;
}
 
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 19f0108f8..29202f0e7 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '
test_cmp expected actual
 '
 
+test_expect_success 'grep --no-index pattern -- path' '
+   rm -fr non &&
+   mkdir -p non/git &&
+   (
+   GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+   export GIT_CEILING_DIRECTORIES &&
+   cd non/git &&
+   echo hello >hello &&
+   git grep --no-index o -- .
+   )
+'
+
 cat >expected <

[PATCH 1/7] grep: move thread initialization a little lower

[Jeff King]

Originally, we set up the threads for grep before parsing
the non-option arguments. In 53b8d931b (grep: disable
threading in non-worktree case, 2011-12-12), the thread code
got bumped lower in the function because it now needed to
know whether we got any revision arguments.

That put a big block of code in between the parsing of revs
and the parsing of pathspecs, both of which share some loop
variables. That makes it harder to read the code than the
original, where the shared loops were right next to each
other.

Let's bump the thread initialization until after all of the
parsing is done.

Signed-off-by: Jeff King 
---
I double-checked to make sure no other code was relying on
the thread setup having happened. I think we could actually
bump it quite a bit lower (to right before we actually start
grepping), but I doubt it matters much in practice.

 builtin/grep.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef49..5a282c4d0 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1169,6 +1169,20 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
break;
}
 
+   /* The rest are paths */
+   if (!seen_dashdash) {
+   int j;
+   for (j = i; j < argc; j++)
+   verify_filename(prefix, argv[j], j == i);
+   }
+
+   parse_pathspec(&pathspec, 0,
+  PATHSPEC_PREFER_CWD |
+  (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
+  prefix, argv + i);
+   pathspec.max_depth = opt.max_depth;
+   pathspec.recursive = 1;
+
 #ifndef NO_PTHREADS
if (list.nr || cached || show_in_pager)
num_threads = 0;
@@ -1190,20 +1204,6 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
}
 #endif
 
-   /* The rest are paths */
-   if (!seen_dashdash) {
-   int j;
-   for (j = i; j < argc; j++)
-   verify_filename(prefix, argv[j], j == i);
-   }
-
-   parse_pathspec(&pathspec, 0,
-  PATHSPEC_PREFER_CWD |
-  (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
-  prefix, argv + i);
-   pathspec.max_depth = opt.max_depth;
-   pathspec.recursive = 1;
-
if (recurse_submodules) {
gitmodules_config();
compile_submodule_options(&opt, &pathspec, cached, untracked,
-- 
2.12.0.rc1.471.ga79ec8999

[PATCH 0/7] grep rev/path parsing fixes

[Jeff King]

On Mon, Feb 13, 2017 at 08:20:37PM -0500, Jeff King wrote:

> > If there is a repo and "foo" is a rev, the "--no-index or --untracked
> > cannot be used with revs." error would occur. If there is a repo and
> > "foo" is not a rev, this command would proceed as usual. If there is no
> > repo, the "setup_git_env called without repository" error would occur.
> > (This is my understanding from reading the code - I haven't tested it
> > out.)
> 
> Yes, it's easy to see that "git grep --no-index foo bar" outside of a
> repo generates the same BUG. I suspect that "--no-index" should just
> disable looking up revs entirely, even if we are actually in a
> repository directory.

I've fixed that, along with a few other bugs and cleanups. The complete
series is below. Patch 2 is your (untouched) patch. My suggestions for
your test are in patch 3, which can either remain on its own or be
squashed in.

  [1/7]: grep: move thread initialization a little lower
  [2/7]: grep: do not unnecessarily query repo for "--"
  [3/7]: t7810: make "--no-index --" test more robust
  [4/7]: grep: re-order rev-parsing loop
  [5/7]: grep: fix "--" rev/pathspec disambiguation
  [6/7]: grep: avoid resolving revision names in --no-index case
  [7/7]: grep: do not diagnose misspelt revs with --no-index

 builtin/grep.c  | 78 +++--
 t/t7810-grep.sh | 66 
 2 files changed, 119 insertions(+), 25 deletions(-)

-Peff

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Jeff King]

On Mon, Feb 13, 2017 at 08:41:51PM -0800, Junio C Hamano wrote:

> Arif Khokar  writes:
> 
> > One concern I have regarding this idea is whether or not SMTP servers
> > typically replace a Message-Id header set by the client.
> 
> The clients are supposed to give Message-IDs, but because some
> clients fail to do so, SMTP server implementations are allowed to
> add an ID to avoid leaving a message nameless (IIRC, 6.3 in
> RFC2821).  So "replace" would be in violation.
> 
> But some parts of the world ignore RFCs, so...

I know there are some terrible servers out there, but I think we can
discount any such server as horribly broken. Rewriting message-ids would
cause threading problems any time the sender referred to their own
messages. So "format-patch --thread" would fail to work, and even
replying to your own message from your "sent" folder would fail.

-Peff

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Arif Khokar]

On 02/13/2017 11:41 PM, Junio C Hamano wrote:

Arif Khokar  writes:


One concern I have regarding this idea is whether or not SMTP servers
typically replace a Message-Id header set by the client.


The clients are supposed to give Message-IDs, but because some
clients fail to do so, SMTP server implementations are allowed to
add an ID to avoid leaving a message nameless (IIRC, 6.3 in
RFC2821).  So "replace" would be in violation.

But some parts of the world ignore RFCs, so...


Based on my testing, gmail and comcast (and my work email) will preserve 
the Message-Id header set by the client, but 
hotmail.com/live.com/outlook.com will replace it with their generated value.


Based on a small sample of email addresses of those who post to this 
list, it appears that most people are using their own MTA to send email, 
or are using gmail, so they probably wouldn't be affected by the issue.

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Junio C Hamano]

Arif Khokar  writes:

> One concern I have regarding this idea is whether or not SMTP servers
> typically replace a Message-Id header set by the client.

The clients are supposed to give Message-IDs, but because some
clients fail to do so, SMTP server implementations are allowed to
add an ID to avoid leaving a message nameless (IIRC, 6.3 in
RFC2821).  So "replace" would be in violation.

But some parts of the world ignore RFCs, so...

Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision

[Siddharth Kannan]

Hey Junio,
> 
> See the 3-patch series I just sent out.  I didn't think it through
> very carefully (especially the error message the other caller
> produces), but the whole thing _smells_ correct to me.

Okay, got it! I will write-up those changes, and make sure nothing bad
happens. (Also, the one other function that calls handle_revision_opt,
parse_revision_opt needs to be fixed for any changes in
handle_revision_opt.)

I will do all of this in the next week (Unfortunately, exams!) and
submit a new version of this patch (Also, I need to update tests, add
documentation, and remove code that does this shorthand stuff for
other commands as per Matthieu's comments)

--
Best Regards,

Siddharth Kannan.

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Arif Khokar]

On 02/13/2017 10:56 PM, Arif Khokar wrote:


I wasn't aware of that expectation.  My idea was to use NNTP as a way to
facilitate the development of a new git utility that would serve as the
inverse of git-send-email (sort of like the relationship between git
format-patch and git am), rather than using a


...custom script that's tightly coupled to gmane and public-inbox.org

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Arif Khokar]

On 02/13/2017 09:37 AM, Johannes Schindelin wrote:

Hi Arif,

On Mon, 13 Feb 2017, Arif Khokar wrote:



Thanks for the link.  One thing that comes to mind that is that it may
be better to just download the patches and then manually apply them
afterwords rather than doing it in the script itself.  Or at least add
an option to the script to not automatically invoke git am.


I actually had expected *you* to put in a little bit of an effort, too. In
fact, I was very disappointed that you did not even look into porting that
script to use public-inbox instead of GMane.


I wasn't aware of that expectation.  My idea was to use NNTP as a way to 
facilitate the development of a new git utility that would serve as the 
inverse of git-send-email (sort of like the relationship between git 
format-patch and git am), rather than using a


IIRC, I had posted some proof-of-concept Perl code to do so back in 
August in 



Looking at public-inbox now at the archives of this group, it appears 
that several of the messages I sent weren't archived for some reason 
(and I didn't see any more responses to what I posted at the time).  The 
messages are accessible via NNTP when connecting to gmane though.


Also, looking at the source of the message I referenced, it appears that 
my MUA decided to base64 encode the message for some reason (which may 
have resulted in it getting filtered by those who I sent the message to).


I will look into this more now (given yours and Junio's responses).


Getting back to the point I made when this thread was still active, I
still think it would be better to be able to list the message-id values
in the header or body of the cover letter message of a patch series
(preferably the former) in order to facilitate downloading the patches
via NNTP from gmane or public-inbox.org.  That would make it easier
compared to the different, ad-hoc, methods that exist for each email
client.


You can always do that yourself: you can modify your cover letter to
include that information.


Certainly, but it would be nice to be able to have it done automatically 
by git format-patch (which I'll look into).



Note that doing this automatically in format-patch may not be appropriate,
as 1) the Message-ID could be modified depending on the mail client used
to send the mails


I think the best approach would be not to make including the message-id 
values the default behavior.  Specifying a new command-line option to 
enable that behavior should address those concerns I think.



and 2) it is not unheard of that a developer
finds a bug in the middle of sending a patch series, fixes that bug, and
regenerates the remainder of the patch series, completely rewriting those
Message-IDs.


Perhaps, but should something like that not warrant a re-roll of sorts. 
That is, one should reply to the partial patch series stating that there 
is a bug that renders this particular patch (series) un-usable and the 
re-roll could be posted as a reply to the original cover letter?



Alternatively, or perhaps in addition to the list of message-ids, a list
of URLs to public-inbox.org or gmane messages could also be provided for
those who prefer to download patches via HTTP.


At this point, I am a little disinterested in a discussion without code. I
brought some code to the table, after all.


If you have the time, please take a look at the message-id I referenced. 
 If you need, I can re-post the proof-of-concept code.

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Arif Khokar]

On 02/13/2017 02:21 PM, Junio C Hamano wrote:

Arif Khokar  writes:


... I
still think it would be better to be able to list the message-id
values in the header or body of the cover letter message of a patch
series (preferably the former) in order to facilitate downloading the
patches via NNTP from gmane or public-inbox.org.


You are looking at builtin/log.c::make_cover_letter()?  Patches
welcome, but I think you'd need a bit of preparatory refactoring
to allow gen_message_id() be called for all messages _before_ this
function is called, as currently we generate them as we emit each
patch.


Thank you for the advice; I'll look into it.

One concern I have regarding this idea is whether or not SMTP servers 
typically replace a Message-Id header set by the client.  If that's the 
case, then this approach may unexpectedly fail for some people depending 
on what email provider they use (if they don't maintain their own MTA 
setup).



Alternatively, or perhaps in addition to the list of message-ids, a
list of URLs to public-inbox.org or gmane messages could also be
provided for those who prefer to download patches via HTTP.


Many people around here do not want to repeat the mistake of relying
too much on one provider.  Listing Message-IDs may be a good idea,
listing URLs that are tied to one provider is less so.


I agree.  I've actually thought that it would be useful to mirror a 
read-only copy of the mailing list on a public newsgroup that could be 
accessed through a free provider such as eternal-september.org or the 
Google groups interface.  It certainly would reduce the potential losing 
easy access to the archives of this email list if gmane and 
public-inbox.org fail.  But I suspect doing something like that would 
potentially increase the spam volume substantially for regular 
participants/contributors.

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

[brian m. carlson]

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

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

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 1e815b5577..e0220630d0 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -212,7 +212,7 @@ static char *anonymize_blob(unsigned long *size)
return strbuf_detach(&out, 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(&size);
-   object = (struct object *)lookup_blob(sha1);
+   object = (struct object *)lookup_blob(oid->hash);
eaten = 0;
} else {
-   buf = read_sha1_file(sha1, &type, &size);
+   buf = read_sha1_file(oid->hash, &type, &size);
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, &eaten);
+   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, 
&eaten);
}
 
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(&sha1s, generate_fake_sha1, sha1, &len);
+   size_t len = GIT_SHA1_RAWSZ;
+   return anonymize_mem(&sha1s, generate_fake_oid, oid, &len);
 }
 
 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(&spec->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(&diff_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_na

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

[brian m. carlson]

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

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

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

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

[brian m. carlson]

Convert various uses of unsigned char [20] to struct object_id.  Rename
replace_object_sha1 to rename_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, &oid)) {
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(&oid);
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, &oid))
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, &prev, ref, sizeof(ref), force);
 
transaction = ref_transaction_begin(&err);
if (!transaction ||
-   ref_transaction_update(transaction, ref, repl, prev,
+   ref_transaction_update(transaction, ref, repl->hash, prev.hash,
   0, NULL, &err) ||
ref_transaction_commit(transaction, 

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

[brian m. carlson]

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

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

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

[brian m. carlson]

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

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

diff --git a/builtin/notes.c b/builtin/notes.c
index 5248a9bad8..8c569a49a0 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -693,7 +693,7 @@ static int merge_abort(struct notes_merge_options *o)
 static int merge_commit(struct notes_merge_options *o)
 {
struct strbuf msg = STRBUF_INIT;
-   unsigned char sha1[20], parent_sha1[20];
+   struct object_id oid, parent_oid;
struct notes_tree *t;
struct commit *partial;
struct pretty_print_context pretty_ctx;
@@ -705,27 +705,27 @@ static int merge_commit(struct notes_merge_options *o)
 * and target notes ref from .git/NOTES_MERGE_REF.
 */
 
-   if (get_sha1("NOTES_MERGE_PARTIAL", sha1))
+   if (get_oid("NOTES_MERGE_PARTIAL", &oid))
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(&parent_oid, &partial->parents->item->object.oid);
else
-   hashclr(parent_sha1);
+   oidclr(&parent_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", &msg, &pretty_ctx);
strbuf_trim(&msg);
strbuf_insert(&msg, 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(&parent_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) {
-

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

[brian m. carlson]

Convert most leaf functions to use struct object_id.

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

diff --git a/builtin/commit.c b/builtin/commit.c
index 2de5f6cc64..4e288bc513 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -496,7 +496,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
 static int run_status(FILE *fp, const char *index_file, const char *prefix, 
int nowarn,
  struct wt_status *s)
 {
-   unsigned char sha1[20];
+   struct object_id oid;
 
if (s->relative_paths)
s->prefix = prefix;
@@ -509,9 +509,9 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
s->index_file = index_file;
s->fp = fp;
s->nowarn = nowarn;
-   s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+   s->is_initial = get_sha1(s->reference, oid.hash) ? 1 : 0;
if (!s->is_initial)
-   hashcpy(s->sha1_commit, sha1);
+   hashcpy(s->sha1_commit, oid.hash);
s->status_format = status_format;
s->ignore_submodule_arg = ignore_submodule_arg;
 
@@ -885,7 +885,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
commitable = run_status(s->fp, index_file, prefix, 1, s);
s->use_color = saved_color_setting;
} else {
-   unsigned char sha1[20];
+   struct object_id oid;
const char *parent = "HEAD";
 
if (!active_nr && read_cache() < 0)
@@ -894,7 +894,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
if (amend)
parent = "HEAD^1";
 
-   if (get_sha1(parent, sha1)) {
+   if (get_sha1(parent, oid.hash)) {
int i, ita_nr = 0;
 
for (i = 0; i < active_nr; i++)
@@ -1332,7 +1332,7 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
 {
static struct wt_status s;
int fd;
-   unsigned char sha1[20];
+   struct object_id oid;
static struct option builtin_status_options[] = {
OPT__VERBOSE(&verbose, N_("be verbose")),
OPT_SET_INT('s', "short", &status_format,
@@ -1382,9 +1382,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
 
fd = hold_locked_index(&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(&rev.diffopt);
 
-   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(&proc);
if (code)
return code;
-   strbuf_addf(&sb, "%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+   strbuf_addf(&sb, "%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 v2 13/19] refs: convert each_reflog_ent_fn to struct object_id

[brian m. carlson]

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

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

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

[PATCH v2 00/19] object_id part 6

[brian m. carlson]

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

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

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

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

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

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

-- 
2.11.0

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

[brian m. carlson]

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

diff --git a/builtin/branch.c b/builtin/branch.c
index 9d30f55b0b..faf472ff8f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -32,7 +32,7 @@ static const char * const builtin_branch_usage[] = {
 };
 
 static const char *head;
-static unsigned char head_sha1[20];
+static struct object_id head_oid;
 
 static int branch_use_color = -1;
 static char branch_colors[][COLOR_MAXLEN] = {
@@ -117,13 +117,13 @@ static int branch_merged(int kind, const char *name,
if (kind == FILTER_REFS_BRANCHES) {
struct branch *branch = branch_get(name);
const char *upstream = branch_get_upstream(branch, NULL);
-   unsigned char sha1[20];
+   struct object_id oid;
 
if (upstream &&
(reference_name = reference_name_to_free =
 resolve_refdup(upstream, RESOLVE_REF_READING,
-   sha1, NULL)) != NULL)
-   reference_rev = lookup_commit_reference(sha1);
+   oid.hash, NULL)) != NULL)
+   reference_rev = lookup_commit_reference(oid.hash);
}
if (!reference_rev)
reference_rev = head_rev;
@@ -153,10 +153,10 @@ static int branch_merged(int kind, const char *name,
 }
 
 static int check_branch_commit(const char *branchname, const char *refname,
-  const unsigned char *sha1, struct commit 
*head_rev,
+  const struct object_id *oid, struct commit 
*head_rev,
   int kinds, int force)
 {
-   struct commit *rev = lookup_commit_reference(sha1);
+   struct commit *rev = lookup_commit_reference(oid->hash);
if (!rev) {
error(_("Couldn't look up commit object for '%s'"), refname);
return -1;
@@ -183,7 +183,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
   int quiet)
 {
struct commit *head_rev = NULL;
-   unsigned char sha1[20];
+   struct object_id oid;
char *name = NULL;
const char *fmt;
int i;
@@ -207,7 +207,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
}
 
if (!force) {
-   head_rev = lookup_commit_reference(head_sha1);
+   head_rev = lookup_commit_reference(head_oid.hash);
if (!head_rev)
die(_("Couldn't look up commit object for HEAD"));
}
@@ -235,7 +235,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
RESOLVE_REF_READING
| RESOLVE_REF_NO_RECURSE
| RESOLVE_REF_ALLOW_BAD_NAME,
-   sha1, &flags);
+   oid.hash, &flags);
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, &oid, 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(&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 v2 05/19] builtin/fmt-merge-message: convert to struct object_id

[brian m. carlson]

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

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

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index efab62fd85..6faa3c0d24 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -41,7 +41,7 @@ struct src_data {
 };
 
 struct origin_data {
-   unsigned char sha1[20];
+   struct object_id oid;
unsigned is_local_branch:1;
 };
 
@@ -59,8 +59,8 @@ static struct string_list origins = STRING_LIST_INIT_DUP;
 struct merge_parents {
int alloc, nr;
struct merge_parent {
-   unsigned char given[20];
-   unsigned char commit[20];
+   struct object_id given;
+   struct object_id commit;
unsigned char used;
} *item;
 };
@@ -70,14 +70,14 @@ struct merge_parents {
  * hundreds of heads at a time anyway.
  */
 static struct merge_parent *find_merge_parent(struct merge_parents *table,
- unsigned char *given,
- unsigned char *commit)
+ struct object_id *given,
+ struct object_id *commit)
 {
int i;
for (i = 0; i < table->nr; i++) {
-   if (given && hashcmp(table->item[i].given, given))
+   if (given && oidcmp(&table->item[i].given, given))
continue;
-   if (commit && hashcmp(table->item[i].commit, commit))
+   if (commit && oidcmp(&table->item[i].commit, commit))
continue;
return &table->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(&table->item[table->nr].given, given);
+   oidcpy(&table->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, &oid);
if (i)
return 3;
 
-   if (!find_merge_parent(merge_parents, sha1, NULL))
+   if (!find_merge_parent(merge_parents, &oid, NULL))
return 0; /* subsumed by other parents */
 
origin_data = xcalloc(1, sizeof(struct origin_data));
-   hashcpy(origin_data->sha1, sha1);
+   oidcpy(&origin_data->oid, &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 = &origin_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 

[PATCH v2 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 | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index d7a5fd2a7c..09227a3f63 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3117,12 +3117,14 @@ 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;
+   const int minlen = 2 * GIT_SHA1_HEXSZ + 3;
 
/* 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, &ooid) || sb->buf[40] != ' ' ||
-   get_oid_hex(sb->buf + 41, &noid) || sb->buf[81] != ' ' ||
-   !(email_end = strchr(sb->buf + 82, '>')) ||
+   if (sb->len < minlen || sb->buf[sb->len - 1] != '\n' ||
+   parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||
+   parse_oid_hex(p, &noid, &p) || *p++ != ' ' ||
+   !(email_end = strchr(p, '>')) ||
email_end[1] != ' ' ||
!(timestamp = strtoul(email_end + 2, &message, 10)) ||
!message || message[0] != ' ' ||
@@ -3136,7 +3138,7 @@ static int show_one_reflog_ent(struct strbuf *sb, 
each_reflog_ent_fn fn, void *c
message += 6;
else
message += 7;
-   return fn(&ooid, &noid, sb->buf + 82, timestamp, tz, message, cb_data);
+   return fn(&ooid, &noid, sb->buf + minlen - 1, timestamp, tz, message, 
cb_data);
 }
 
 static char *find_beginning_of_line(char *bob, char *scan)
-- 
2.11.0

[PATCH v2 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, &revkey))
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, &refname)) {
+   switch (dwim_ref(argv[0], strlen(argv[0]), oid.hash, &refname)) {
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, &oid))
die("Not a valid object name: '%s'", commitname);
 
-   derived = lookup_commit_reference(sha1);
+   derived = lookup_commit_reference(oid.hash);
memset(&revs, 0, sizeof(revs));
revs.initial = 1;
for_each_reflog_ent(refname, collect_one_reflog_ent, &revs);
 
-   if (!revs.nr && !get_sha1(refname, sha1))
-   add_one_commit(sha1, &revs);
+   if (!revs.nr && !get_oid(refname, &oid))
+   add_one_commit(&oid, &revs);
 
for (i = 0; i < revs.nr; i++)
revs.commit[i]->object.flags &= ~TMP_MARK;
-- 
2.11.0

[PATCH v2 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, &oid)) {
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(&sb, 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(&sb, 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, &oid)) {
strbuf_reset(&sb);
-   strbuf_add_unique_abbrev(&sb, sha1, DEFAULT_ABBREV);
+   strbuf_add_unique_abbrev(&sb, 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(&cb->buf);
-   hashcpy(cb->nsha1, noid->hash);
+   oidcpy(&cb->noid, noid);
end = strchrnul(target, '\n');
strbuf_add(&cb->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(&cb.buf, 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, &ref) == 1 &&
+   if (dwim_ref(cb.buf.buf, cb.buf.len, oid.hash, &ref) == 1 &&
/* sha1 is a commit? match without further lookup */
-   (!hashcmp(cb.nsha1, sha1) ||
+   (!oidcmp(&cb.noid, &oid) ||
 /* 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(&cb.noid, &commit->object.oid {
const char *from = ref;
if (!skip_prefix(from, "refs/tags/", &from))
skip_prefix(from, "refs/remotes/", &from);
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", &oid) &&
+!hashcmp(oid.hash, state->detached_sha1);
 
free(ref);
strbuf_release(&cb.buf);
@@ -1476,22 +1476,22 @@ void wt_status_get_state(struct wt_status_state *state,
 int get_detached_from)
 {
struct stat st;
-   unsigned char sha1[

[PATCH v2 14/19] hex: introduce parse_oid_hex

[brian m. carlson]

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

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

diff --git a/cache.h b/cache.h
index 61fc86e6d7..5dc89a058c 100644
--- a/cache.h
+++ b/cache.h
@@ -1319,6 +1319,14 @@ 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 hexadecimal object ID starting from hex, updating the pointer
+ * specified by p 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.
+ */
+extern int parse_oid_hex(const char *hex, struct object_id *oid, const char 
**p);
+
 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..54252b1445 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 **p)
+{
+   int ret = get_oid_hex(hex, oid);
+   if (!ret)
+   *p = 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 v2 06/19] builtin/grep: convert to struct object_id

[brian m. carlson]

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

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

diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef499..0393b0fdc4 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -294,17 +294,17 @@ static int grep_cmd_config(const char *var, const char 
*value, void *cb)
return st;
 }
 
-static void *lock_and_read_sha1_file(const unsigned char *sha1, enum 
object_type *type, unsigned long *size)
+static void *lock_and_read_oid_file(const struct object_id *oid, enum 
object_type *type, unsigned long *size)
 {
void *data;
 
grep_read_lock();
-   data = read_sha1_file(sha1, type, size);
+   data = read_sha1_file(oid->hash, type, size);
grep_read_unlock();
return data;
 }
 
-static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
+static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
 const char *filename, int tree_name_len,
 const char *path)
 {
@@ -323,7 +323,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned 
char *sha1,
 
 #ifndef NO_PTHREADS
if (num_threads) {
-   add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);
+   add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, oid);
strbuf_release(&pathbuf);
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(&gs, GREP_SOURCE_SHA1, pathbuf.buf, path, 
sha1);
+   grep_source_init(&gs, GREP_SOURCE_SHA1, pathbuf.buf, path, oid);
strbuf_release(&pathbuf);
hit = grep_source(opt, &gs);
 
@@ -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, &ce->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, &type, 
&size);
+   data = lock_and_read_oid_file(entry.oid, &type, &size);
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, &obj->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, &oc)) {
-   struct object *object = parse_object_or_die(sha1, arg);
+   if (!get_sha1_with_context(arg, 0, oid.hash, &oc)) {
+   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, &list, oc.mode, 
oc.path);
-- 
2.11.0

[PATCH v2 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, &type, &size, &contents) < 0)
+   if (read_loose_object(path, oid->hash, &type, &size, &contents) < 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, &eaten);
+   obj = parse_object_buffer(oid->hash, type, size, contents, &eaten);
 
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 v2 09/19] builtin/merge: convert to struct object_id

[brian m. carlson]

Additionally convert several uses of the constant 40 into
GIT_SHA1_HEXSZ.

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

diff --git a/builtin/merge.c b/builtin/merge.c
index a96d4fb501..099cfab447 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -244,7 +244,7 @@ static void drop_save(void)
unlink(git_path_merge_mode());
 }
 
-static int save_state(unsigned char *stash)
+static int save_state(struct object_id *stash)
 {
int len;
struct child_process cp = CHILD_PROCESS_INIT;
@@ -265,7 +265,7 @@ static int save_state(unsigned char *stash)
else if (!len)  /* no changes */
return -1;
strbuf_setlen(&buffer, 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 = &head_commit->object.oid;
 
if (!msg)
strbuf_addstr(&reflog_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(&opts);
-   diff_tree_sha1(head, new_head, "", &opts);
+   diff_tree_sha1(head->hash, new_head->hash, "", &opts);
diffcore_std(&opts);
diff_flush(&opts);
}
@@ -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(&bname, remote);
remote = bname.buf;
 
-   memset(branch_head, 0, sizeof(branch_head));
+   oidclr(&branch_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, &found_ref) > 0) {
+   if (dwim_ref(remote, strlen(remote), branch_head.hash, &found_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(&branch_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(&branch_head), remote);
goto cleanup;
}
if (starts_with(found_ref, "refs/remotes/")) {
strbuf_addf(msg, "%s\t\tremote

[PATCH v2 03/19] builtin/describe: convert to struct object_id

[brian m. carlson]

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

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

diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a157e..738e68f95b 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -39,11 +39,11 @@ static const char *diff_index_args[] = {
 
 struct commit_name {
struct hashmap_entry entry;
-   unsigned char peeled[20];
+   struct object_id peeled;
struct tag *tag;
unsigned prio:2; /* annotated tag = 2, tag = 1, head = 0 */
unsigned name_checked:1;
-   unsigned char sha1[20];
+   struct object_id oid;
char *path;
 };
 
@@ -54,17 +54,17 @@ static const char *prio_names[] = {
 static int commit_name_cmp(const struct commit_name *cn1,
const struct commit_name *cn2, const void *peeled)
 {
-   return hashcmp(cn1->peeled, peeled ? peeled : cn2->peeled);
+   return oidcmp(&cn1->peeled, peeled ? peeled : &cn2->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(&names, sha1hash(peeled), peeled);
+   return hashmap_get_from_hash(&names, 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, &tag)) {
+   if (replace_name(e, prio, oid, &tag)) {
if (!e) {
e = xmalloc(sizeof(struct commit_name));
-   hashcpy(e->peeled, peeled);
-   hashmap_entry_init(e, sha1hash(peeled));
+   oidcpy(&e->peeled, peeled);
+   hashmap_entry_init(e, sha1hash(peeled->hash));
hashmap_add(&names, e);
e->path = NULL;
}
e->tag = tag;
e->prio = prio;
e->name_checked = 0;
-   hashcpy(e->sha1, sha1);
+   oidcpy(&e->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, &peeled, 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_o

[PATCH v2 12/19] reflog-walk: convert struct reflog_info to struct object_id

[brian m. carlson]

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

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

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

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

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

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

diff --git a/reflog-walk.c b/reflog-walk.c
index f98748e2ae..fe5be41471 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -10,7 +10,7 @@ struct complete_reflogs {
char *ref;
const char *short_ref;
struct reflog_info {
-   unsigned char osha1[20], nsha1[20];
+   struct object_id ooid, noid;
char *email;
unsigned long timestamp;
int tz;
@@ -28,8 +28,8 @@ static int read_one_reflog(unsigned char *osha1, unsigned 
char *nsha1,
 
ALLOC_GROW(array->items, array->nr + 1, array->alloc);
item = array->items + array->nr;
-   hashcpy(item->osha1, osha1);
-   hashcpy(item->nsha1, nsha1);
+   hashcpy(item->ooid.hash, osha1);
+   hashcpy(item->noid.hash, nsha1);
item->email = xstrdup(email);
item->timestamp = timestamp;
item->tz = tz;
@@ -238,13 +238,13 @@ void fake_reflog_parent(struct reflog_walk_info *info, 
struct commit *commit)
do {
reflog = &commit_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 = &commit_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 v2 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 5dc89a058c..04b1d923f3 100644
--- a/cache.h
+++ b/cache.h
@@ -1607,6 +1607,12 @@ extern void check_pack_index_ptr(const struct packed_git 
*p, const void *ptr);
  * error.
  */
 extern const unsigned char *nth_packed_object_sha1(struct packed_git *, 
uint32_t n);
+/*
+ * Like nth_packed_object_oid, but write the data into the object specified by
+ * the the first argument.  Returns the first argument on success, and NULL on
+ * error.
+ */
+extern const struct object_id *nth_packed_object_oid(struct object_id *, 
struct packed_git *, uint32_t n);
 
 /*
  * Return the offset of the nth object within the specified packfile.
diff --git a/sha1_file.c b/sha1_file.c
index ec957db5e1..777b8e8eae 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2628,6 +2628,17 @@ const unsigned char *nth_packed_object_sha1(struct 
packed_git *p,
}
 }
 
+const struct object_id *nth_packed_object_oid(struct object_id *oid,
+ struct packed_git *p,
+ uint32_t n)
+{
+   const unsigned char *hash = nth_packed_object_sha1(p, n);
+   if (!hash)
+   return NULL;
+   hashcpy(oid->hash, hash);
+   return oid;
+}
+
 void check_pack_index_ptr(const struct packed_git *p, const void *vptr)
 {
const unsigned char *ptr = vptr;
@@ -3788,13 +3799,13 @@ static int for_each_object_in_pack(struct packed_git 
*p, each_packed_object_fn c
int r = 0;
 
for (i = 0; i < p->num_objects; i++) {
-   const unsigned char *sha1 = nth_packed_object_sha1(p, i);
+   struct object_id oid;
 
-   if (!sha1)
+   if (!nth_packed_object_oid(&oid, p, i))
return error("unable to get sha1 of object %u in %s",
 i, p->pack_name);
 
-   r = cb(sha1, p, i, data);
+   r = cb(oid.hash, p, i, data);
if (r)
break;
}
-- 
2.11.0

Re: [PATCH for NEXT] grep: do not unnecessarily query repo for "--"

[Jeff King]

On Mon, Feb 13, 2017 at 04:11:59PM -0800, Jonathan Tan wrote:

> When running a command of the form
> 
>   git grep --no-index pattern -- path
> 
> in the absence of a Git repository, an error message will be printed:
> 
>   fatal: BUG: setup_git_env called without repository
> 
> This is because "git grep" tries to interpret "--" as a rev. "git grep"
> has always tried to first interpret "--" as a rev for at least a few
> years, but this issue was upgraded from a pessimization to a bug in
> commit 59332d1 ("Resurrect "git grep --no-index"", 2010-02-06), which
> calls get_sha1 regardless of whether --no-index was specified. This bug
> appeared to be benign until commit b1ef400 ("setup_git_env: avoid blind
> fall-back to ".git"", 2016-10-20) when Git was taught to die in this
> situation.  (This "git grep" bug appears to be one of the bugs that
> commit b1ef400 is meant to flush out.)
> 
> Therefore, always interpret "--" as signaling the end of options,
> instead of trying to interpret it as a rev first.

Nicely explained.

> There is probably a similar bug for commands of the form:
> 
>   git grep --no-index pattern foo
> 
> If there is a repo and "foo" is a rev, the "--no-index or --untracked
> cannot be used with revs." error would occur. If there is a repo and
> "foo" is not a rev, this command would proceed as usual. If there is no
> repo, the "setup_git_env called without repository" error would occur.
> (This is my understanding from reading the code - I haven't tested it
> out.)

Yes, it's easy to see that "git grep --no-index foo bar" outside of a
repo generates the same BUG. I suspect that "--no-index" should just
disable looking up revs entirely, even if we are actually in a
repository directory.

> This patch does not fix this similar bug, but I decided to send it out
> anyway because it still fixes a bug and unlocks the ability to
> specify paths with "git grep --no-index".

Yes, I think even if we fix the other bug, fixing this "--" thing is an
improvement.

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 2c727ef49..1b68d1638 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char 
> *prefix)
>   const char *arg = argv[i];
>   unsigned char sha1[20];
>   struct object_context oc;
> + if (!strcmp(arg, "--")) {
> + i++;
> + seen_dashdash = 1;
> + break;
> + }
>   /* Is it a rev? */
>   if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
>   struct object *object = parse_object_or_die(sha1, arg);

So I think this is a definite improvement, but I see a few leftover
oddities:

  - the end logic for this loop is now:

  if (arg is a rev) {
 ... handle rev ...
 continue;
  }
  break;

It would probably be more obvious as:

  if (arg is not a rev)
  break;
  ... handle rev ...

  - the rev-handling code does:

  if (!seen_dashdash)
  verify_non_filename(prefix, arg);

But I do not see how seen_dashdash could ever be untrue. We set it
inside this loop, and break immediately when we see it. And indeed,
running:

  echo content >master
  git grep content master --

does not work. The "--" should tell us that "master" is a rev, but
we don't know yet that we have a dashdash.

I think we need a separate loop to find the "--" first, and _then_
walk through the arguments, treating them as revs or paths as
appropriate. This is how setup_revisions() does it.

So this isn't a problem introduced by your patch, but it's
intimately related.

> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 19f0108f8..29202f0e7 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '
>   test_cmp expected actual
>  '
>  
> +test_expect_success 'grep --no-index pattern -- path' '
> + rm -fr non &&
> + mkdir -p non/git &&
> + (
> + GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd non/git &&
> + echo hello >hello &&
> + git grep --no-index o -- .
> + )
> +'

Since de95302a4, you can do:

  nongit git grep --no-index -o -- .

Though if this is destined for maint, it might need to be done
separately this way and cleaned up later.

It might also be a good idea to confirm that the pathspec is actually
being respected in the --no-index case. Something like:

  echo hello >hello &&
  echo goodbye >goodbye &&
  echo hello:hello >expect &&
  git grep --no-index o -- hello >actual &&
  test_cmp expect actual

-Peff

Re: [PATCH] completion: complete modified files for checkout with '--'

[SZEDER Gábor]

On Tue, Feb 14, 2017 at 12:33 AM,   wrote:
> From: Cornelius Weig 
>
> The command line completion for git-checkout bails out when seeing '--'
> as an isolated argument. For git-checkout this signifies the start of a
> list of files which are to be checked out. Checkout of files makes only
> sense for modified files,

No, there is e.g. 'git checkout that-branch this-path', too.


> therefore completion can be a bit smarter:
> Instead of bailing out, offer modified files for completion.
>
> Signed-off-by: Cornelius Weig 
> ---
>  contrib/completion/git-completion.bash | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 6c6e1c7..d6523fd 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1059,7 +1059,10 @@ _git_bundle ()
>
>  _git_checkout ()
>  {
> -   __git_has_doubledash && return
> +   __git_has_doubledash && {
> +   __git_complete_index_file "--modified"
> +   return
> +   }
>
> case "$cur" in
> --conflict=*)
> --
> 2.10.2
>

Re: [RFC-PATCHv2] submodules: add a background story

[Brandon Williams]

On 02/08, Stefan Beller wrote:
> +STATES
> +--
> +
> +When working with submodules, you can think of them as in a state machine.
> +So each submodule can be in a different state, the following indicators are 
> used:
> +
> +* the existence of the setting of 'submodule..url' in the
> +  superprojects configuration
> +* the existence of the submodules working tree within the
> +  working tree of the superproject
> +* the existence of the submodules git directory within the superprojects
> +  git directory at $GIT_DIR/modules/ or within the submodules working
> +  tree
> +
> +  State  URL configworking tree git dir
> +  -
> +  uninitializedno   no   no
> +  initialized yes   no   no
> +  populated   yes  yes  yes
> +  depopulated yes   no  yes
> +  deinitializedno   no  yes
> +  uninterestingno  yes  yes
> +
> +  invalid  no  yes   no
> +  invalid yes  yes   no
> +  -
> +
> +The first six states can be reached by normal git usage, the latter two are
> +only shown for completeness to show all possible eight states with 3 binary
> +indicators. The states in detail:
> +
> +uninitialized::
> +The uninitialized state is the default state if no
> +'--recurse-submodules' / '--recursive'. An empty directory will be put in
> +the working tree as a place holder, such that you are reminded of the
> +existence of the submodule.
> +---
> +To transition into the initialized state
> +you can use 'git submodule init', which copies the presets from the
> +.gitmodules file into the config.
> +
> +initialized::
> +Users transitioned from the uninitialized state to this state via
> +'git submodule init', which preset the URL configuration. As these URLs
> +may not be desired in certain scenarios, this state allows to change the
> +URLs.  For example in a corporate environment you may want to run
> +
> +sed -i s/example.org/$internal-mirror/ .git/config
> ++

Maybe we can try to brainstorm and come up with some clearer terminology
while we are at it.  I was trying to think about the "initialized" state
and I may be the only one but it seems unclear what "initialized" means.
I mean I already have all the information about a submodule in the
.gitmodules file, isn't it already initialized then?   Maybe this state
would be better named "(in)active" as a module that is interesting to a
user is "active"?

-- 
Brandon Williams

Re: [PATCH v3 0/5] stash: support pathspec argument

[Jeff King]

On Mon, Feb 13, 2017 at 04:36:22PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Maybe it is just me and my muscle memory, but "git stash -p" is quite a
> > common command for me[1]. And I have typed "git stash -p foo" many times
> > and been annoyed that it didn't work. I was hoping to end that
> > annoyance.
> 
> OK, I never run "stash -p" and did not realize people already find
> it useful that it becomes "stash save -p".  Please ignore me, then.
> I agree that breaking the established use is not nice.

To be fair, I don't think anybody is proposing breaking the established
use of "stash -p". I was just hoping the new pathspec feature could
trickle down to it, as well. :)

-Peff

Re: [PATCH v3 0/5] stash: support pathspec argument

[Jeff King]

On Mon, Feb 13, 2017 at 03:50:43PM -0800, Junio C Hamano wrote:

> Thomas Gummerer  writes:
> 
> >> My "-p" suggestion suffers from a similar problem if you treat it as
> >> "you can omit the 'push' if you say "-p", rather than "if -p is the
> >> first option, it is a synonym for 'push -p'".
> >
> > I'm almost convinced of special casing "-p".  (Maybe I'm easy to
> > convince as well, because it would be convenient ;) ) However it's a
> > bit weird that now "git stash -p file" would work, but "git stash -m
> > message" wouldn't.
> 
> I am not sure why this matters.  The original "git stash " was
> just "Are you being extremely busy and cannot even afford to type
> 'save'?  Ok, let me assume you meant that!".  Now we are talking
> about picking and choosing hunks carefully going through interactive
> process, I really do not think there is any justification to infer
> 'push' when 'push' was omitted in "git stash push -p" the user wants
> to do.

Maybe it is just me and my muscle memory, but "git stash -p" is quite a
common command for me[1]. And I have typed "git stash -p foo" many times
and been annoyed that it didn't work. I was hoping to end that
annoyance.

I guess I could make an alias and retrain my fingers.

-Peff

[1] I almost never run "reset --hard", preferring instead to stash away
changes just in case I would change my mind later and want them. And
I quite often use "stash -p" because I like to double check what I
am throwing away.

I also use "stash -p" heavily when picking apart changes from the
working tree.

[PATCH for NEXT] grep: do not unnecessarily query repo for "--"

[Jonathan Tan]

When running a command of the form

  git grep --no-index pattern -- path

in the absence of a Git repository, an error message will be printed:

  fatal: BUG: setup_git_env called without repository

This is because "git grep" tries to interpret "--" as a rev. "git grep"
has always tried to first interpret "--" as a rev for at least a few
years, but this issue was upgraded from a pessimization to a bug in
commit 59332d1 ("Resurrect "git grep --no-index"", 2010-02-06), which
calls get_sha1 regardless of whether --no-index was specified. This bug
appeared to be benign until commit b1ef400 ("setup_git_env: avoid blind
fall-back to ".git"", 2016-10-20) when Git was taught to die in this
situation.  (This "git grep" bug appears to be one of the bugs that
commit b1ef400 is meant to flush out.)

Therefore, always interpret "--" as signaling the end of options,
instead of trying to interpret it as a rev first.

Signed-off-by: Jonathan Tan 
---

There is probably a similar bug for commands of the form:

  git grep --no-index pattern foo

If there is a repo and "foo" is a rev, the "--no-index or --untracked
cannot be used with revs." error would occur. If there is a repo and
"foo" is not a rev, this command would proceed as usual. If there is no
repo, the "setup_git_env called without repository" error would occur.
(This is my understanding from reading the code - I haven't tested it
out.)

This patch does not fix this similar bug, but I decided to send it out
anyway because it still fixes a bug and unlocks the ability to
specify paths with "git grep --no-index".

 builtin/grep.c  |  9 +
 t/t7810-grep.sh | 12 
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef49..1b68d1638 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
const char *arg = argv[i];
unsigned char sha1[20];
struct object_context oc;
+   if (!strcmp(arg, "--")) {
+   i++;
+   seen_dashdash = 1;
+   break;
+   }
/* Is it a rev? */
if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
struct object *object = parse_object_or_die(sha1, arg);
@@ -1162,10 +1167,6 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
add_object_array_with_path(object, arg, &list, oc.mode, 
oc.path);
continue;
}
-   if (!strcmp(arg, "--")) {
-   i++;
-   seen_dashdash = 1;
-   }
break;
}
 
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 19f0108f8..29202f0e7 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '
test_cmp expected actual
 '
 
+test_expect_success 'grep --no-index pattern -- path' '
+   rm -fr non &&
+   mkdir -p non/git &&
+   (
+   GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+   export GIT_CEILING_DIRECTORIES &&
+   cd non/git &&
+   echo hello >hello &&
+   git grep --no-index o -- .
+   )
+'
+
 cat >expected <

Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API

[Stefan Beller]

>
> +/*
> + * Return the ref_store instance for the specified submodule. For the
> + * main repository, use submodule==NULL; such a call cannot fail.

So now we have both a get_main as well as a get_submodule function,
but the submodule function can return the main as well?

I'd rather see this as a BUG; or asking another way:
What is the difference between get_submodule_ref_store(NULL)
and get_main_ref_store() ?

As you went through all call sites (by renaming the function), we'd
be able to tell that there is no caller with NULL, or is it?

Stefan

> For
> + * a submodule, the submodule must exist and be a nonbare repository,
> + * otherwise return NULL. If the requested reference store has not yet
> + * been initialized, initialize it first.
> + *
> + * For backwards compatibility, submodule=="" is treated the same as
> + * submodule==NULL.
> + */
> +struct ref_store *get_submodule_ref_store(const char *submodule);
> +struct ref_store *get_main_ref_store(void);

Re: [PATCH] clean: use warning_errno() when appropriate

[Junio C Hamano]

Jeff King  writes:

> So in that sense doing the errno dance as close to the caller who cares
> is the most _obvious_ thing, even if it isn't the shortest.

Yup.

> It would be nice if there was a way to annotate a function as
> errno-safe, and then transitively compute which other functions were
> errno-safe when they do not call any errno-unsafe function. I don't know
> if any static analyzers allow that kind of custom annotation, though
> (and also I wonder whether the cost/benefit of maintaining those
> annotations would be worth it).

Double yup.

Re: Continuous Testing of Git on Windows

[Junio C Hamano]

Johannes Schindelin  writes:

> That is why I taught the Git for Windows CI job that tests the four
> upstream Git integration branches to *also* bisect test breakages and then
> upload comments to the identified commit on GitHub

Good.  I do not think it is useful to try 'pu' as an aggregate and
expect it to always build and work [*1*], but your "bisect and
pinpoint" approach makes it useful to identify individual topic that
brings in a breakage.  I wouldn't be surprised if original submitter
and I were the only two people who actually compiled the patches on
a topic in isolation while a topic is in 'pu', and chances are that
these two people didn't try their builds on Windows.  A CI like this
one will help the coverage to stop premature topics from advancing
to 'pu' without getting any Windows exposure.

Thanks.


[Footnote]

*1* The reason why topics not in 'next' but in 'pu', especially the
ones merged near the tip of 'pu', exist in 'pu' are because they
are interesting enough and could be polished to become eligible
for 'next' but known to be premature for 'next' yet.  They are
there primarily to give human contributors an easier way to
download them as a whole and help polish them.  And I have to be
selective when I queue things on 'pu'; it is not like I have
infinite amount of time to pick up any cruft that is sent to the
list.

Re: [PATCH 10/11] files-backend: remove submodule_allowed from files_downcast()

[Stefan Beller]

On Mon, Feb 13, 2017 at 7:20 AM, 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...
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

Looks good,
Stefan

[PATCH] completion: complete modified files for checkout with '--'

[cornelius . weig]

From: Cornelius Weig 

The command line completion for git-checkout bails out when seeing '--'
as an isolated argument. For git-checkout this signifies the start of a
list of files which are to be checked out. Checkout of files makes only
sense for modified files, therefore completion can be a bit smarter:
Instead of bailing out, offer modified files for completion.

Signed-off-by: Cornelius Weig 
---
 contrib/completion/git-completion.bash | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 6c6e1c7..d6523fd 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1059,7 +1059,10 @@ _git_bundle ()
 
 _git_checkout ()
 {
-   __git_has_doubledash && return
+   __git_has_doubledash && {
+   __git_complete_index_file "--modified"
+   return
+   }
 
case "$cur" in
--conflict=*)
-- 
2.10.2

Re: [PATCH 09/11] refs: move submodule code out of files-backend.c

[Stefan Beller]

On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy  wrote:
> files-backend is now initialized with a $GIT_DIR. Converting a submodule
> path to where real submodule gitdir is located is done in get_ref_store().
>
> The new code in init_submodule_ref_store() is basically a copy of
> strbuf_git_path_submodule().
>
> This gives a slight performance improvement for submodules since we
> don't convert submodule path to gitdir at every backend call like
> before. We pay that once at ref-store creation.
>
> More cleanup in files_downcast() follows shortly. It's separate to keep
> noises from this patch.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs.c   | 48 +---
>  refs/files-backend.c | 25 +++--
>  refs/refs-internal.h |  6 +++---
>  3 files changed, 51 insertions(+), 28 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 8ef7a52ba..9ac194945 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -9,6 +9,7 @@
>  #include "refs/refs-internal.h"
>  #include "object.h"
>  #include "tag.h"
> +#include "submodule-config.h"
>
>  /*
>   * List of all available backends
> @@ -1410,7 +1411,7 @@ static void register_ref_store(struct ref_store *refs, 
> const char *submodule)
>   * Create, record, and return a ref_store instance for the specified
>   * submodule (or the main repository if submodule is NULL).
>   */
> -static struct ref_store *ref_store_init(const char *submodule)
> +static struct ref_store *ref_store_init(const char *submodule, const char 
> *gitdir)
>  {
> const char *be_name = "files";
> struct ref_storage_be *be = find_ref_storage_backend(be_name);
> @@ -1419,7 +1420,7 @@ static struct ref_store *ref_store_init(const char 
> *submodule)
> if (!be)
> die("BUG: reference backend %s is unknown", be_name);
>
> -   refs = be->init(submodule);
> +   refs = be->init(gitdir);
> register_ref_store(refs, submodule);
> return refs;
>  }
> @@ -1445,15 +1446,48 @@ static struct ref_store *lookup_ref_store(const char 
> *submodule)
> return entry ? entry->refs : NULL;
>  }
>
> -static struct ref_store *init_submodule_ref_store(const char *submodule)
> +static struct ref_store *init_submodule_ref_store(const char *path)
>  {
> struct strbuf submodule_sb = STRBUF_INIT;
> +   struct strbuf git_submodule_common_dir = STRBUF_INIT;
> +   struct strbuf git_submodule_dir = STRBUF_INIT;
> +   struct strbuf buf = STRBUF_INIT;
> +   const char *git_dir;
> +   const struct submodule *sub;
> struct ref_store *refs = NULL;
>
> -   strbuf_addstr(&submodule_sb, submodule);
> -   if (is_nonbare_repository_dir(&submodule_sb))
> -   refs = ref_store_init(submodule);
> +   strbuf_addstr(&submodule_sb, path);
> +   if (!is_nonbare_repository_dir(&submodule_sb))
> +   goto done;
> +
> +   strbuf_addstr(&buf, path);
> +   strbuf_complete(&buf, '/');
> +   strbuf_addstr(&buf, ".git");
> +
> +   git_dir = read_gitfile(buf.buf);

if buf.buf is a (git) directory as opposed to a git file,
we error out in read_gitfile. Did you mean to use
read_gitfile_gently here or rather even resolve_gitdir_gently ?

> +   if (git_dir) {

when not using the _gently version git_dir is always
non NULL here (or we're dead)?

> +   strbuf_reset(&buf);
> +   strbuf_addstr(&buf, git_dir);
> +   }
> +   if (!is_git_directory(buf.buf)) {
> +   gitmodules_config();
> +   sub = submodule_from_path(null_sha1, path);
> +   if (!sub)
> +   goto done;
> +   strbuf_reset(&buf);
> +   strbuf_git_path(&buf, "%s/%s", "modules", sub->name);

You can inline "modules" into the format string?

> +   }
> +   strbuf_addch(&buf, '/');
> +   strbuf_addbuf(&git_submodule_dir, &buf);
> +
> +   refs = ref_store_init(path, git_submodule_dir.buf);

strbuf_detach (git_submodule_dir) here, such that we keep
the string alive despite the release of the strbuf below?

so essentially this function
* takes a submodule path
* checks if there is a repo at the given path in the working tree
* resolves the gitfile if any
* if the gitfile could not resolve to a valid repo just make up the
  location to be $GIT_DIR/modules/

sounds confusing to me. I need to reread it later.

>
> -   if (submodule) {
> -   refs->submodule = xstrdup_or_null(submodule);
> +   if (gitdir) {
> +   strbuf_addstr(&refs->gitdir, gitdir);
> +   get_common_dir_noenv(&refs->gitcommondir, gitdir);

Oh I see. we loose the _or_null here, so my remark on the previous patch
might be just unneeded work.

> } else {
> strbuf_addstr(&refs->gitdir, get_git_dir());
> strbuf_addstr(&refs->gitcommondir, get_git_common_dir());
> @@ -1034,8 +1025,6 @@ static struct ref_store *files_ref_store_cre

Re: [PATCH v6] gc: ignore old gc.log files

[Junio C Hamano]

David Turner  writes:

> +static unsigned long gc_log_expire_time;
> +static const char *gc_log_expire = "1.day.ago";

OK.

> @@ -113,6 +133,8 @@ static void gc_config(void)
>   git_config_get_bool("gc.autodetach", &detach_auto);
>   git_config_date_string("gc.pruneexpire", &prune_expire);
>   git_config_date_string("gc.worktreepruneexpire", 
> &prune_worktrees_expire);
> + git_config_date_string("gc.logexpiry", &gc_log_expire);
> +

OK.

I think I had a stale one queued; will replace.

Thanks.

Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()

[Junio C Hamano]

Michael Haggerty  writes:

> I pushed the fixed commit to branch `submodule-hash` in my fork [1]. If
> you'd like me to send it to the mailing list again, please let me know.

I was tempted to ask you to send it again, because fetching,
comparing and then cherry-picking is a lot more work than just
replacing one, but just to make sure my intuition about the
work required is correct, I did it anyway and yes, fetching,
comparing, cherry-picking and then amending the sign-off in was a
lot more work ;-)

So no need to resend.  Thanks.

Developing git with IDE

[Oleg Taranenko]

Hi *,

being last decade working with java & javascript I completely lost
relation to c/c++ world. Trying to get into git internals I'm facing
with issue what IDE is more suitable for developing git @ macos ?

Have googled, but any my search queries following to non-relevant
themes, like supporting git in IDEs etc.

my first attempt - CLion (as far as I'm Jetbrains fan) - got failed,
as far as doesn't support makefile-based projects, only CMake.

There are a number of free C/C++ dev tools: Xcode, CodeBlocks,
CodeLite. Gnat, Qt creator, Dev C++, C++ Builder (Borland? :),
Eclipse, NetBeans... what else?

Because of  lack my modern C experience, could somebody share his own
attempts/thoughts/use cases how more convenient dive into the git
development process on the Mac?

Tried to find in the git distribution Documentation more information
about this, nothing as well...  Would be nice to have a kind of
'Getting Started Manual'


Thanks for your time,

Oleg Taranenko

Re: [PATCH 08/11] refs.c: factor submodule code out of get_ref_store()

[Stefan Beller]

On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy  wrote:
> This code is going to be expanded a bit soon. Keep it out for
> better readability later.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

Looks good,
Thanks,
Stefan

new git-diff switch to eliminate leading "+" and "-" characters

[Vanderhoof, Tzadik]

The output of git-diff includes lines beginning with "+" and "-" to indicate 
added and deleted lines.  A somewhat common task (at least for me) is to want 
to copy output from a "diff" (usually the deleted lines) and paste it back into 
my code.

This is quite inconvenient because of the leading "+" and "-" characters.  I 
know there are shell and IDE / editor workarounds but it would be nice if there 
was a switch to git-diff to make it leave out those characters, especially 
since "--color" kind of makes those leading characters obsolete.

Would it make sense to develop such a switch or has there been work on that 
already?
__
Tzadik Vanderhoof | Optum360 
Sr Software Engineer, NLP Innovation
www.optum360.com

This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or his or her authorized agent, the reader is hereby notified
that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.

Re: [PATCH 07/11] files-backend: remove the use of git_path()

[Stefan Beller]

> +
> +   if (submodule) {
> +   refs->submodule = xstrdup_or_null(submodule);

drop the _or_null here?

So in this patch we have either
* submodule set
* or gitdir/gitcommondir set

which means that we exercise the commondir for regular repos.
In the future when we want to be able to have a combination of worktrees
and submodules this ought to be possible by setting submodule != NULL
and still populating the gitdir/commondir buffers.

Thanks,
Stefan

Re: [PATCH v3 4/5] stash: introduce new format create

[Jeff King]

On Mon, Feb 13, 2017 at 04:57:34PM -0500, Jeff King wrote:

> Yeah, I think your patch is actually fixing that case. But your search
> is only part of the story. You found somebody using "-m" explicitly, but
> what about somebody blindly calling:
> 
>   git stash create $*
> 
> That's now surprising to somebody who puts "-m" in their message.
> 
> > I *think* this regression is acceptable, but I'm happy to introduce
> > another verb if people think otherwise.
> 
> Despite what I wrote above, I'm still inclined to say that this isn't an
> important regression. I'd be surprised if "stash create" is used
> independently much at all.

Just thinking on this more...do we really care about "fixing" the
interface of "stash create"? This is really just about refactoring what
underlies the new "push", right?

So we could just do:

diff --git a/git-stash.sh b/git-stash.sh
index 6d629fc43..ee37db135 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -711,7 +711,7 @@ clear)
;;
 create)
shift
-   create_stash "$@" && echo "$w_commit"
+   create_stash -m "$*" && echo "$w_commit"
;;
 store)
shift

on top of your patch and keep the external interface the same.

It might be nice to clean up the interface for "create" to match other
ones, but from this discussion I think it is mostly a historical wart
for scripting, and we are OK to just leave its slightly-broken interface
in place forever.

I could go either way.

-Peff

Re: [PATCH] mingw: make stderr unbuffered again

[Junio C Hamano]

Johannes Schindelin  writes:

> When removing the hack for isatty(), we actually removed more than just
> an isatty() hack: we removed the hack where internal data structures of
> the MSVC runtime are modified in order to redirect stdout/stderr.
>
> Instead of using that hack (that does not work with newer versions of
> the runtime, anyway), we replaced it by reopening the respective file
> descriptors.
>
> What we forgot was to mark stderr as unbuffered again.
>
> Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.
>
> Signed-off-by: Johannes Schindelin 
> ---
> Published-As: 
> https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git 
> mingw-unbuffered-stderr-v1

OK.  Should this go directly to 'master', as the isatty thing is
already in?

>
>  compat/winansi.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/compat/winansi.c b/compat/winansi.c
> index 82b89ab1376..793420f9d0d 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -510,6 +510,8 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
>*/
>   close(new_fd);
>  
> + if (fd == 2)
> + setvbuf(stderr, NULL, _IONBF, BUFSIZ);
>   fd_is_interactive[fd] |= FD_SWAPPED;
>  
>   return duplicate;
> @@ -547,6 +549,8 @@ static void detect_msys_tty(int fd)
>   !wcsstr(name, L"-pty"))
>   return;
>  
> + if (fd == 2)
> + setvbuf(stderr, NULL, _IONBF, BUFSIZ);
>   fd_is_interactive[fd] |= FD_MSYS;
>  }
>  
>
> base-commit: 5588dbffbd61e4906e453808c6ad32f792fea521

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

[Jeff King]

On Mon, Feb 13, 2017 at 01:01:49PM -0800, Junio C Hamano wrote:

> Linus Torvalds  writes:
> 
> > And if you actually want decorations, and you're parsing them, you are
> > *not* going to script it with "--oneline --decorations", because the
> > end result is basically impossible to parse already (because it's
> > ambiguous - think about parentheses in the commit message).
> 
> OK.  So let's wait to hear from others if they like the "obviously"
> improved output.  Even though I find the decorations indispensable
> in my "git log" output, I personally do not have much preference
> either way, as my screen is often wide enough ;-)

I have a slight preference for the new output (decorations at the end)
versus the original, but I could go either way.

I don't think the scripting compatibility concerns are an issue, for all
the reasons given in the thread.

There is one related option, --source, which also puts its data between
the hash and the subject in --oneline. In theory that should be treated
similarly, though:

  1. It's already really ugly, as it does not even get the parentheses
 and coloring.

  2. It's perhaps more likely to get scripted, as it really is parseable
 in the current state.

I'm not sure if a better path forward would be to just extend the idea
of "decorator" to possibly include more than just ref-tips. On the other
hand, if you really want to get fancy with formatting, we already have a
complete formatting language. Perhaps it should learn a placeholder for
the "--source" data.

Similarly, I've often wanted a "contained in this tags/branches"
annotation for each commit. It's not too expensive to compute if you
topo-sort the set of commits and just paint down as you traverse.

Anyway, I think none of that needs to block changes to --decorate
output. Just thinking out loud.

-Peff

Re: [PATCH] mingw: use OpenSSL's SHA-1 routines

[Johannes Schindelin]

Hi,

On Mon, 13 Feb 2017, Johannes Sixt wrote:

> Am 13.02.2017 um 20:42 schrieb Junio C Hamano:
> > I have been operating under the assumption that everybody on Windows
> > who builds Git works off of Dscho's Git for Windows tree, and patches
> > that are specific to Windows from Dscho's are sent to me via the list
> > only after they have been in Git for Windows and proven to help
> > Windows users in the wild.
> >
> > The consequence of these two assumptions is that I would feel safe to
> > treat Windows specific changes that do not touch generic part of the
> > codebase from Dscho just like updates from any other subsystem
> > maintainers (any git-svn thing from Eric, any gitk thing from Paul,
> > any p4 thing Luke and Lars are both happy with, etc.).
> >
> > You seem to be saying that the first of the two assumptions does not
> > hold.  Should I change my expectations while queuing Windows specific
> > patches from Dscho?
> 
> Your first assumption is incorrect as far as I am concerned. I build
> from your tree plus some topics. During -rc period, I build off of
> master; after a release, I build off of next. I merge some of the topics
> that you carry in pu when I find them interesting or when I suspect them
> to regress on Windows.  Then I carry around a few additional patches
> that the public has never seen, and these days I also merge Dscho's
> rebase-i topic.

In addition, you build from a custom MINGW/MSys1 setup, correct?

Ciao,
Johannes

Re: [PATCH 06/11] refs-internal.h: correct is_per_worktree_ref()

[Stefan Beller]

On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy  wrote:
> All refs outside refs/ directory is per-worktree, not just HEAD.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/refs-internal.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index f4aed49f5..69d02b6ba 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -653,7 +653,7 @@ const char *resolve_ref_recursively(struct ref_store 
> *refs,
>
>  static inline int is_per_worktree_ref(const char *refname)
>  {
> -   return !strcmp(refname, "HEAD") ||
> +   return !starts_with(refname, "refs/") ||
> starts_with(refname, "refs/bisect/");

you're loosing HEAD here? (assuming we get HEAD in
short form here, as well as long form refs/HEAD)

>  }
>
> --
> 2.11.0.157.gd943d85
>

[PATCH] mingw: make stderr unbuffered again

[Johannes Schindelin]

When removing the hack for isatty(), we actually removed more than just
an isatty() hack: we removed the hack where internal data structures of
the MSVC runtime are modified in order to redirect stdout/stderr.

Instead of using that hack (that does not work with newer versions of
the runtime, anyway), we replaced it by reopening the respective file
descriptors.

What we forgot was to mark stderr as unbuffered again.

Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.

Signed-off-by: Johannes Schindelin 
---
Published-As: 
https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-unbuffered-stderr-v1

 compat/winansi.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/compat/winansi.c b/compat/winansi.c
index 82b89ab1376..793420f9d0d 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -510,6 +510,8 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
 */
close(new_fd);
 
+   if (fd == 2)
+   setvbuf(stderr, NULL, _IONBF, BUFSIZ);
fd_is_interactive[fd] |= FD_SWAPPED;
 
return duplicate;
@@ -547,6 +549,8 @@ static void detect_msys_tty(int fd)
!wcsstr(name, L"-pty"))
return;
 
+   if (fd == 2)
+   setvbuf(stderr, NULL, _IONBF, BUFSIZ);
fd_is_interactive[fd] |= FD_MSYS;
 }
 

base-commit: 5588dbffbd61e4906e453808c6ad32f792fea521
-- 
2.11.1.windows.1

Re: [PATCH v3 0/5] stash: support pathspec argument

[Thomas Gummerer]

On 02/13, Jeff King wrote:
> On Mon, Feb 13, 2017 at 10:35:31PM +0100, Matthieu Moy wrote:
> 
> > > Is it really that dangerous, though? The likely outcome is Git saying
> > > "nope, you don't have any changes to the file named drop". Of course the
> > > user may have meant something different, but I feel like "-p" is a good
> > > indicator that they are interested in making an actual stash.
> > 
> > Indeed -p is not the best example. In the old thread, I used -q which is
> > much more problematic:
> > 
> >   git stash -q drop => interpreted as: git stash push -q drop
> >   git stash drop -q => drop with option -q
> 
> Yeah, I'd agree with that. I wouldn't propose to loosen it entirely, but
> rather to treat "-p" specially.
> 
> > It's not really "dangerous" at least in this case, since we misinterpret
> > a destructive command for a less destructive one, but it is rather
> > confusing that changing the order between command and options change the
> > behavior.
> > 
> > I actually find it a reasonable expectation to allow swapping commands
> > and options, some programs other than git allow it.
> 
> I think we may have already crossed that bridge with "git -p stash".
> 
> Not to mention that the ordering already _is_ relevant (we disallow one
> order but not the other). If we really wanted to allow swapping, it
> would mean making:
> 
>   git stash -p drop
> 
> the same as:
> 
>   git stash drop -p
> 
> I actually find _that_ more confusing. It would perhaps make more sense
> with something like "-q", which is more of a "global" option than a
> command-specific one. But I think we'd want to whitelist such global
> options (and "-p" would not be on that list).
>
> > > The complexity is that right now, the first-level decision of "which
> > > stash sub-command am I running?" doesn't know about any options. So "git
> > > stash -m foo" would be rejected in the name of typo prevention, unless
> > > that outer decision learns about "-m" as an option.
> > 
> > Ah, OK. But that's not really hard to implement: when going through the
> > option list looking for non-option, shift one more time when finding -m.
> 
> No, it's not hard conceptually. It just means implementing the
> option-parsing policy in two places. That's not too bad now, but if we
> started using rev-parse's options helper, then I think you have corner
> cases like "git stash -km foo".
> 
> My "-p" suggestion suffers from a similar problem if you treat it as
> "you can omit the 'push' if you say "-p", rather than "if -p is the
> first option, it is a synonym for 'push -p'".

I'm almost convinced of special casing "-p".  (Maybe I'm easy to
convince as well, because it would be convenient ;) ) However it's a
bit weird that now "git stash -p file" would work, but "git stash -m
message" wouldn't.  Maybe we should do it the other way around, and
only special case "-q", and see if there is an non option argument
after that?  From a glance at the options that's the only one where
"git stash - " could make sense to the user.

Re: [PATCH] clean: use warning_errno() when appropriate

[Jeff King]

On Mon, Feb 13, 2017 at 01:53:33PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > IOW, I think this may be a case where we should be optimizing for
> > programmer time (fewer lines of code, and one less thing to worry about
> > in the callers) versus squeezing out every instruction.
> 
> Fair enough.
> 
> Unless we do the save_errno dance in all the helper functions we
> commonly use to safely stash away errno as necessary and tell
> developers that they can depend on it, the code in the patch that
> began this discussion still needs its own saved_errno dance to be
> safe, though.  I do not have a feeling that we are not there yet,
> even after we teach xmalloc() and its family to do so.

Yeah, I certainly agree that is a potential blocker. Even if it is true
today, there's nothing guaranteeing that the quote functions don't grow
a new internal detail that violates.

So in that sense doing the errno dance as close to the caller who cares
is the most _obvious_ thing, even if it isn't the shortest.

It would be nice if there was a way to annotate a function as
errno-safe, and then transitively compute which other functions were
errno-safe when they do not call any errno-unsafe function. I don't know
if any static analyzers allow that kind of custom annotation, though
(and also I wonder whether the cost/benefit of maintaining those
annotations would be worth it).

-Peff

Re: [PATCH v3 2/5] stash: introduce push verb

[Thomas Gummerer]

On 02/13, Matthieu Moy wrote:
> Thomas Gummerer  writes:
> 
> > Introduce a new git stash push verb in addition to git stash save.  The
> > push verb is used to transition from the current command line arguments
> > to a more conventional way, in which the message is given as an argument
> > to the -m option.
> 
> Sorry if this has been discussed before, but I find 'push' rather
> confusing here. It took me a while to understand that it meant "opposite
> of pop", because in the context of Git, "push" usually means "send to
> remote repository".

There wasn't much of a discussion about it, but it was pretty much the
only thing that came to my mind, and nobody complained or suggested
anything different, so I just went with it.  No other verb came to my
mind yet, but if someone has a better suggestion, I'd be happy to
change.

> Unfortunately, I didn't come up with a better name. "create" is already
> taken ...
> 
> Another think to have in mind: changing the option name to break
> backward compatibility is something we can't do often, so if there's
> anything else we should change about the UI, we should do it now. I
> don't have anything particular in mind, just thinking aloud.

Now that you mention this, there actually is one inconsistency that I
introduced, which I shouldn't have.  git stash push works with
--include-untracked and --all to decide whether or not to include
untracked files, and if also ignored files should be included.  I also
added a --include-untracked {untracked,all} argument to git stash
create, which is clearly inconsistent.

There really should only be one way.  I'd be fine with either way, but
I think using --include-untracked and --all is the better choice,
because it's easy to understand, and also makes it easier to switch
git stash without a verb over to use push_stash internally.

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

Re: [PATCH] docs/git-submodule: fix unbalanced quote

[Junio C Hamano]

Jeff King  writes:

> However, let's take a step back. Even when rendered
> correctly, it's hard to read a long command stuck into the
> middle of a paragraph, and the important punctuation is hard
> to notice.

Yes, I like this reasoning behind the solution very much.

Thanks.

>  Documentation/git-submodule.txt | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 918bd1d1b..a8eb1c7ce 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -228,9 +228,12 @@ foreach::
>   the processing to terminate. This can be overridden by adding '|| :'
>   to the end of the command.
>  +
> -As an example, +git submodule foreach \'echo $path {backtick}git
> -rev-parse HEAD{backtick}'+ will show the path and currently checked out
> -commit for each submodule.
> +As an example, the command below will show the path and currently
> +checked out commit for each submodule:
> ++
> +--
> +git submodule foreach 'echo $path `git rev-parse HEAD`'
> +--
>  
>  sync::
>   Synchronizes submodules' remote URL configuration setting

Re: [PATCH v3 4/5] stash: introduce new format create

[Jeff King]

On Sat, Feb 11, 2017 at 02:51:27PM +, Thomas Gummerer wrote:

> > How do we tell the difference between new-style invocations, and
> > old-style ones that look new-style? IOW, I think:
> > 
> >   git stash create -m works
> > 
> > currently treats "-m works" as the full message, and it would now become
> > just "works".
> > 
> > That may be an acceptable loss for the benefit we are getting. The
> > alternative is to make yet another verb for create, as we did with
> > save/push). I have a feeling that hardly anybody uses "create", though,
> > and it's mostly an implementation detail. So given the obscure nature,
> > maybe it's an acceptable level of regression. I dunno.
> 
> Right.  So I did a quick search on google and github for this, and
> there seems one place where git stash create -m is used [1].  From a
> quick look it does however not seem like the -m in the stash message
> is of any significance there, but rather that the intention was to use
> a flag that doesn't exist.

Yeah, I think your patch is actually fixing that case. But your search
is only part of the story. You found somebody using "-m" explicitly, but
what about somebody blindly calling:

  git stash create $*

That's now surprising to somebody who puts "-m" in their message.

> I *think* this regression is acceptable, but I'm happy to introduce
> another verb if people think otherwise.

Despite what I wrote above, I'm still inclined to say that this isn't an
important regression. I'd be surprised if "stash create" is used
independently much at all.

-Peff

Re: [PATCH] clean: use warning_errno() when appropriate

[Junio C Hamano]

Jeff King  writes:

> IOW, I think this may be a case where we should be optimizing for
> programmer time (fewer lines of code, and one less thing to worry about
> in the callers) versus squeezing out every instruction.

Fair enough.

Unless we do the save_errno dance in all the helper functions we
commonly use to safely stash away errno as necessary and tell
developers that they can depend on it, the code in the patch that
began this discussion still needs its own saved_errno dance to be
safe, though.  I do not have a feeling that we are not there yet,
even after we teach xmalloc() and its family to do so.

Re: Incorrect pipe for git log graph when using --name-status option

[Jeff King]

On Mon, Feb 13, 2017 at 01:43:45PM -0800, hIpPy wrote:

> With --name-status: I'm sorry if I nitpick here but I think the
> --name-status items should either be preceeded and followed by
> blank line OR not (as in oneline) but not just preceded (example
> below).

Yeah, I agree the output is a bit ugly.

I don't think there is a way to do what you want currently. The
--oneline code path has some internal magic that suppresses some of the
newlines.

I think it is a good goal that anything that can be expressed via one of
the regular formats (like --oneline) can be expressed via custom
formats. I don't know offhand how hard it would be to make this
particular case work. If somebody is interested in tackling this, they'd
probably need to dig around in pretty.c to see where the ONELINE code
path diverge from the USER_FORMAT one, and then figure out how a user
can specify that they want the ONELINE-ish semantics for their format.

-Peff

Re: [PATCH v3 0/5] stash: support pathspec argument

[Jeff King]

On Mon, Feb 13, 2017 at 10:35:31PM +0100, Matthieu Moy wrote:

> > Is it really that dangerous, though? The likely outcome is Git saying
> > "nope, you don't have any changes to the file named drop". Of course the
> > user may have meant something different, but I feel like "-p" is a good
> > indicator that they are interested in making an actual stash.
> 
> Indeed -p is not the best example. In the old thread, I used -q which is
> much more problematic:
> 
>   git stash -q drop => interpreted as: git stash push -q drop
>   git stash drop -q => drop with option -q

Yeah, I'd agree with that. I wouldn't propose to loosen it entirely, but
rather to treat "-p" specially.

> It's not really "dangerous" at least in this case, since we misinterpret
> a destructive command for a less destructive one, but it is rather
> confusing that changing the order between command and options change the
> behavior.
> 
> I actually find it a reasonable expectation to allow swapping commands
> and options, some programs other than git allow it.

I think we may have already crossed that bridge with "git -p stash".

Not to mention that the ordering already _is_ relevant (we disallow one
order but not the other). If we really wanted to allow swapping, it
would mean making:

  git stash -p drop

the same as:

  git stash drop -p

I actually find _that_ more confusing. It would perhaps make more sense
with something like "-q", which is more of a "global" option than a
command-specific one. But I think we'd want to whitelist such global
options (and "-p" would not be on that list).

> > The complexity is that right now, the first-level decision of "which
> > stash sub-command am I running?" doesn't know about any options. So "git
> > stash -m foo" would be rejected in the name of typo prevention, unless
> > that outer decision learns about "-m" as an option.
> 
> Ah, OK. But that's not really hard to implement: when going through the
> option list looking for non-option, shift one more time when finding -m.

No, it's not hard conceptually. It just means implementing the
option-parsing policy in two places. That's not too bad now, but if we
started using rev-parse's options helper, then I think you have corner
cases like "git stash -km foo".

My "-p" suggestion suffers from a similar problem if you treat it as
"you can omit the 'push' if you say "-p", rather than "if -p is the
first option, it is a synonym for 'push -p'".

-Peff

Re: Incorrect pipe for git log graph when using --name-status option

[hIpPy]

On Mon, Feb 13, 2017 at 1:27 PM, Jeff King  wrote:
> On Mon, Feb 13, 2017 at 01:18:40PM -0800, Junio C Hamano wrote:
>
>> Jeff King  writes:
>>
>> > I think the problem is specifically related to the "terminator" versus
>> > "separator" semantics. Try:
>> >
>> >   git log --graph --name-status --pretty=format:%h
>> >
>> > versus:
>> >
>> >   git log --graph --name-status --pretty=tformat:%h
>> >
>> > The latter works fine. The problem seems to happen with all diff
>> > formats, so I think the issue is that git is not aggressive enough about
>> > inserting the newline at the right spot when using separator mode.
>>
>> I guess that is one of the reasons why we made --format=%h a synonym
>> to --pretty=tformat:%h and not --pretty=format:%h ;-)
>
> Yeah. I have never found "--pretty=format" to do what I want versus
> tformat. I wish we could just get rid of it, but I think it is likely to
> be used as a plumbing interface.
>
> So I think there probably _is_ a bug in it to be fixed, but my immediate
> response is "don't ever use --pretty=format:".
>
> -Peff

I have 1 issue with tformat in that I feel it reduces
readability a bit when using options. But I can use it if that
is recommended over the other.

Without --name-status: This is OK.

$ git log --graph --date=relative --decorate --abbrev-commit
--format='%h - %aD (%ar)%d%n %s - %an' -2
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar
*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin


With --name-status: I'm sorry if I nitpick here but I think the
--name-status items should either be preceeded and followed by
blank line OR not (as in oneline) but not just preceded (example
below).

$ git log --graph --date=relative --decorate --abbrev-commit
--format='%h - %aD (%ar)%d%n %s - %an' -2 --name-status
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar
|
| M diff.c
*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin


So either of below is better that above (I feel):

Blank lines before and after --name-status items:

$ git log --graph --date=relative --decorate --abbrev-commit
--format='%h - %aD (%ar)%d%n %s - %an' -2 --name-status
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar
|
| M diff.c
|
*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin

No blank lines before and after --name-status items:

$ git log --graph --date=relative --decorate --abbrev-commit
--format='%h - %aD (%ar)%d%n %s - %an' -2 --name-status
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar
| M diff.c
*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin

What do guys think?

If there is a way to get what I want using tformat?


Thanks,
RM

Employee interviews

[CG]

Is GIT SCM conducting interviews via Google chat? I'm responding to a zip 
recruiter job listing. 
Thank You. Just looking for some official company information such as an office 
address, work phone number, or business email domain. Thank You. 

CG 

Re: Incorrect pipe for git log graph when using --name-status option

[Jeff King]

On Mon, Feb 13, 2017 at 01:18:40PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I think the problem is specifically related to the "terminator" versus
> > "separator" semantics. Try:
> >
> >   git log --graph --name-status --pretty=format:%h
> >
> > versus:
> >
> >   git log --graph --name-status --pretty=tformat:%h
> >
> > The latter works fine. The problem seems to happen with all diff
> > formats, so I think the issue is that git is not aggressive enough about
> > inserting the newline at the right spot when using separator mode.
> 
> I guess that is one of the reasons why we made --format=%h a synonym
> to --pretty=tformat:%h and not --pretty=format:%h ;-)

Yeah. I have never found "--pretty=format" to do what I want versus
tformat. I wish we could just get rid of it, but I think it is likely to
be used as a plumbing interface.

So I think there probably _is_ a bug in it to be fixed, but my immediate
response is "don't ever use --pretty=format:".

-Peff

Re: [RFH] Request for Git Merge 2017 impressions for Git Rev News

[Christian Couder]

Hi,

On Sat, Feb 11, 2017 at 5:33 PM, Jakub Narębski  wrote:
> Hello,
>
> Git Rev News #24 is planned to be released on February 15. It is meant
> to cover what happened during the month of January 2017 (and earely
> February 2017) and the Git Merge 2017 conference that happened on
> February 2nd and 3rd 2017.

Yeah, I would have liked to release Git Rev News #24 on February 15
but I was busy and tired this week end, so let it be on Wednesday
February 22.

> A draft of a new Git Rev News edition is available here:
>
>   
> https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-24.md
>
> I would like to ask everyone who attended the conference (and the
> GitMerge 2017 Contributors’s Summit day before it), or watched it live
> at http://git-merge.com/watch to write his or her impressions.
>
> You can contribute either by replying to this email, or by editing the
> above page on GitHub and sending a pull request, or by commenting on
> the following GitHub issue about Git Rev News 24:
>
>   https://github.com/git/git.github.io/issues/221
>
> If you prefer to post on your own blog (or if you have did it
> already), please send an URL.

Yeah, any material (link, short impression, article, ...) would be very nice.
Thanks Jakub for the links you already added to the draft, by the way!

> P.S. I wonder if there should be not a separate section on
> https://git.github.io/ about recollection from various Git-related
> events, with Git Merge 2017 as the first one.  This way we can wait
> for later response, and incorporate videos and slides from events, as
> they begin to be available.

Jakub, if you are willing to create and maintain this section, that
would be great!

> P.P.S. Please distribute this information more widely.

Thanks,
Christian.

Re: Incorrect pipe for git log graph when using --name-status option

[Junio C Hamano]

Jeff King  writes:

> I think the problem is specifically related to the "terminator" versus
> "separator" semantics. Try:
>
>   git log --graph --name-status --pretty=format:%h
>
> versus:
>
>   git log --graph --name-status --pretty=tformat:%h
>
> The latter works fine. The problem seems to happen with all diff
> formats, so I think the issue is that git is not aggressive enough about
> inserting the newline at the right spot when using separator mode.

I guess that is one of the reasons why we made --format=%h a synonym
to --pretty=tformat:%h and not --pretty=format:%h ;-)

Re: Incorrect pipe for git log graph when using --name-status option

[Jeff King]

On Mon, Feb 13, 2017 at 12:55:07PM -0800, hIpPy wrote:

> I think that requiring to end custom formats with %n with options
> is not good. So I think this is a bug.

Yes, you don't normally need to do that.

I think the problem is specifically related to the "terminator" versus
"separator" semantics. Try:

  git log --graph --name-status --pretty=format:%h

versus:

  git log --graph --name-status --pretty=tformat:%h

The latter works fine. The problem seems to happen with all diff
formats, so I think the issue is that git is not aggressive enough about
inserting the newline at the right spot when using separator mode.

-Peff

[PATCH] docs/git-submodule: fix unbalanced quote

[Jeff King]

The documentation gives an example of the submodule foreach
command that uses both backticks and single-quotes. We stick
the whole thing inside "+" markers to make it monospace, but
the inside punctuation still needs escaping. We handle the
backticks with "{backtick}", and use backslash-escaping for
the single-quotes.

But we missed the escaping on the second quote. Fortunately,
asciidoc renders this unbalanced quote as we want (showing
the quote), but asciidoctor does not. We could fix it by
adding the missing backslash.

However, let's take a step back. Even when rendered
correctly, it's hard to read a long command stuck into the
middle of a paragraph, and the important punctuation is hard
to notice. Let's instead bump it into its own single-line
code block. That makes both the source and the rendered
result more readable, and as a bonus we don't have to worry
about quoting at all.

Signed-off-by: Jeff King 
---
Not textually related to the previous fix, but obviously along the same
lines.

 Documentation/git-submodule.txt | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 918bd1d1b..a8eb1c7ce 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -228,9 +228,12 @@ foreach::
the processing to terminate. This can be overridden by adding '|| :'
to the end of the command.
 +
-As an example, +git submodule foreach \'echo $path {backtick}git
-rev-parse HEAD{backtick}'+ will show the path and currently checked out
-commit for each submodule.
+As an example, the command below will show the path and currently
+checked out commit for each submodule:
++
+--
+git submodule foreach 'echo $path `git rev-parse HEAD`'
+--
 
 sync::
Synchronizes submodules' remote URL configuration setting
-- 
2.12.0.rc1.466.g70234cfd8

Re: [PATCH] clean: use warning_errno() when appropriate

[Jeff King]

On Mon, Feb 13, 2017 at 12:38:47PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I wonder if xmalloc() should be the one doing the saved_errno trick.
> > After all, it has only two outcomes: we successfully allocated the
> > memory, or we called die().
> 
> I would be lying if I said I did not considered it when I wrote the
> message you are responding to, but I rejected it because that would
> be optimizing for a wrong case, in that most callers of xmalloc()
> and friends do not do so in the error codepath, and we would be
> penalizing them by doing the saved_errno dance unconditionally.

Yes, that also occurred to me. I'm not sure if two integer swaps is
enough to care about when compared to the cost of a malloc(), though.

IOW, I think this may be a case where we should be optimizing for
programmer time (fewer lines of code, and one less thing to worry about
in the callers) versus squeezing out every instruction.

-Peff

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

[Junio C Hamano]

Linus Torvalds  writes:

> And if you actually want decorations, and you're parsing them, you are
> *not* going to script it with "--oneline --decorations", because the
> end result is basically impossible to parse already (because it's
> ambiguous - think about parentheses in the commit message).

OK.  So let's wait to hear from others if they like the "obviously"
improved output.  Even though I find the decorations indispensable
in my "git log" output, I personally do not have much preference
either way, as my screen is often wide enough ;-)

Thanks.  We'd need to update the tests that expects the old style
output, though.

Re: [PATCH] mingw: use OpenSSL's SHA-1 routines

[Johannes Sixt]

Am 13.02.2017 um 20:42 schrieb Junio C Hamano:

I have been operating under the assumption that everybody on Windows
who builds Git works off of Dscho's Git for Windows tree, and
patches that are specific to Windows from Dscho's are sent to me via
the list only after they have been in Git for Windows and proven to
help Windows users in the wild.

The consequence of these two assumptions is that I would feel safe
to treat Windows specific changes that do not touch generic part of
the codebase from Dscho just like updates from any other subsystem
maintainers (any git-svn thing from Eric, any gitk thing from Paul,
any p4 thing Luke and Lars are both happy with, etc.).

You seem to be saying that the first of the two assumptions does not
hold.  Should I change my expectations while queuing Windows specific
patches from Dscho?


Your first assumption is incorrect as far as I am concerned. I build 
from your tree plus some topics. During -rc period, I build off of 
master; after a release, I build off of next. I merge some of the topics 
that you carry in pu when I find them interesting or when I suspect them 
to regress on Windows. Then I carry around a few additional patches that 
the public has never seen, and these days I also merge Dscho's rebase-i 
topic.


-- Hannes

Re: [PATCH] rm: reuse strbuf for all remove_dir_recursively() calls, again

[Stefan Beller]

On Sat, Feb 11, 2017 at 11:51 AM, René Scharfe  wrote:
> Don't throw the memory allocated for remove_dir_recursively() away after
> a single call, use it for the other entries as well instead.
>
> This change was done before in deb8e15a (rm: reuse strbuf for all
> remove_dir_recursively() calls), but was reverted as a side-effect of
> 55856a35 (rm: absorb a submodules git dir before deletion). Reinstate
> the optimization.
>
> Signed-off-by: Rene Scharfe 
> ---
> Was deb8e15a a rebase victim?

(I do not recall it off the top of my head)

That commit was merged at 03f25e85,
Merge branch 'rs/rm-strbuf-optim', but it looks
like it was reverted as part of 55856a35b2
(rm: absorb a submodules git dir before deletion)

Looking through the discussion at
https://public-inbox.org/git/xmqqmvfich2e@gitster.mtv.corp.google.com/
there was no apparent signs of confusion, but a reroll was promised, that
I cannot find on the list.

Anyway, the patch below looks good to me.

Thanks,
Stefan

>
>  builtin/rm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 452170a3ab..fb79dcab18 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -360,15 +360,14 @@ int cmd_rm(int argc, const char **argv, const char 
> *prefix)
>  */
> if (!index_only) {
> int removed = 0, gitmodules_modified = 0;
> +   struct strbuf buf = STRBUF_INIT;
> for (i = 0; i < list.nr; i++) {
> const char *path = list.entry[i].name;
> if (list.entry[i].is_submodule) {
> -   struct strbuf buf = STRBUF_INIT;
> -
> +   strbuf_reset(&buf);
> strbuf_addstr(&buf, path);
> if (remove_dir_recursively(&buf, 0))
> die(_("could not remove '%s'"), path);
> -   strbuf_release(&buf);
>
> removed = 1;
> if (!remove_path_from_gitmodules(path))
> @@ -382,6 +381,7 @@ int cmd_rm(int argc, const char **argv, const char 
> *prefix)
> if (!removed)
> die_errno("git rm: '%s'", path);
> }
> +   strbuf_release(&buf);
> if (gitmodules_modified)
> stage_updated_gitmodules();
> }
> --
> 2.11.1
>

Re: [PATCH 04/11] files-backend: replace *git_path*() with files_path()

[Ramsay Jones]

On 13/02/17 15:20, Nguyễn Thái Ngọc Duy wrote:
> This centralizes all path rewriting of files-backend.c in one place so
> we have easier time removing the path rewriting later. There could be
> some hidden indirect git_path() though, I didn't audit the code to the
> bottom.
> 
> Side note: set_worktree_head_symref() is a bad boy and should not be in
> files-backend.c (probably should not exist in the first place). But
> we'll leave it there until we have better multi-worktree support in refs
> before we update it.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c | 193 
> ++-
>  1 file changed, 98 insertions(+), 95 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 39217a2ca..c69e4fe84 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -165,12 +165,13 @@ static struct ref_entry *create_dir_entry(struct 
> files_ref_store *ref_store,
> const char *dirname, size_t len,
> int incomplete);
>  static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
> -static int files_log_ref_write(const char *refname, const unsigned char 
> *old_sha1,
> +static int files_log_ref_write(struct files_ref_store *refs,
> +const char *refname, const unsigned char 
> *old_sha1,
>  const unsigned char *new_sha1, const char *msg,
>  int flags, struct strbuf *err);
>  
> -void files_path(struct files_ref_store *refs, struct strbuf *sb,
> - const char *fmt, ...) __attribute__((format (printf, 3, 4)));

You only added this in the last commit, so maybe mark it static in
the previous patch! Also, just in case you were wondering, the 'Why?'
of the previous email was, "Why do you need this forward declaration?"
(hint: you don't ;-)

> +static void files_path(struct files_ref_store *refs, struct strbuf *sb,
> +const char *fmt, ...) __attribute__((format (printf, 3, 
> 4)));
>  
>  static struct ref_dir *get_ref_dir(struct ref_entry *entry)
>  {
> @@ -933,8 +934,8 @@ struct files_ref_store {
>  /* Lock used for the main packed-refs file: */
>  static struct lock_file packlock;
>  
> -void files_path(struct files_ref_store *refs, struct strbuf *sb,
> - const char *fmt, ...)
> +static void files_path(struct files_ref_store *refs, struct strbuf *sb,
> +const char *fmt, ...)
>  {
>   struct strbuf tmp = STRBUF_INIT;
>   va_list vap;

ATB,
Ramsay Jones

Re: Incorrect pipe for git log graph when using --name-status option

[hIpPy]

On Mon, Feb 13, 2017 at 11:25 AM, Junio C Hamano  wrote:
> hIpPy  writes:
>
>> The `git log` command with `graph` and pretty format works correctly
>> as expected.
>>
>> $ git log --graph --pretty=format:'%h' -2
>> * 714a14e
>> * 87dce5f
>>
>>
>> However, with `--name-status` option added, there is a pipe
>> incorrectly placed after the commit hash (example below).
>>
>> $ git log --graph --pretty=format:'%h' -2 --name-status
>> * 714a14e|
>> | M README.md
>
> Is it a --name-status, or is it your own custom format, that is
> causing the above issue?
>
>  - What happens if you stop using --pretty=format:%h and replace it
>with something like --oneline?
>
>  - What happens if you keep using --pretty=format:%h but replace
>--name-status with something else, e.g. --raw or --stat?
>



 - What happens if you stop using --pretty=format:%h and replace it
   with something like --oneline?
--oneline works correctly as expected (example below).

$ git log --graph --oneline -2 --name-status
* bf7ace7daf (HEAD -> rm/option-for-name-status) wip: --ns for --name-status
| M diff.c
*   592e5c5bce (origin/master, origin/HEAD, master) Merge pull request
#994 from jeffhostetler/jeffhostetler/fscache_nfd
|\


 - What happens if you keep using --pretty=format:%h but replace
   --name-status with something else, e.g. --raw or --stat?
I see the same issue with --raw and --stat (examples below).

$ git log --graph --pretty=format:'%h' -2 --raw
* bf7ace7daf|
| :100644 100644 84dba60c40... 87dfabf4a9... M  diff.c

*   592e5c5bce
|\

$ git log --graph --pretty=format:'%h' -2 --stat
* bf7ace7daf|
|  diff.c | 2 +-
|  1 file changed, 1 insertion(+), 1 deletion(-)

*   592e5c5bce
|\

I believe it's not my custom format that is causing the issue.
I'm including this information that may not be relevant. I apologize
in advance for that. I had simplified the custom format in my
previous email. For below custom format I still see the pipe
incorrectly placed.

$ git log --graph --date=relative --decorate --abbrev-commit
--format=format:'%h - %aD (%ar)%d%n %s - %an' -2 --name-status
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar|
| M diff.c

*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin


If I were to put a '%n' at the end (example below), the pipe is
correctly placed with or without the --name-status option. But in
case of "without the --name-status option", there is an extra blank
line. It seems that my custom format is requiring a %n at the end. I
consider my custom format as a --twoline option and I feel the
behavior should match --oneline when using options.

With --name-status: This works correctly.

$ git log --graph --date=relative --decorate --abbrev-commit
--format=format:'%h - %aD (%ar)%d%n %s - %an%n' -2 --name-status
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar
|
| M diff.c
|
*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin

Without --name-status: This works but has extra blank line between
commits though.

$ git log --graph --date=relative --decorate --abbrev-commit
--format=format:'%h - %aD (%ar)%d%n %s - %an%n' -2
* bf7ace7daf - Mon, 6 Feb 2017 13:09:32 -0800 (7 days ago) (HEAD ->
rm/option-for-name-status)
|  wip: --ns for --name-status - Rishikesh Mandvikar
|
*   592e5c5bce - Wed, 1 Feb 2017 16:35:30 +0100 (12 days ago)
(origin/master, origin/HEAD, master)
|\   Merge pull request #994 from
jeffhostetler/jeffhostetler/fscache_nfd - Johannes Schindelin

I think that requiring to end custom formats with %n with options
is not good. So I think this is a bug.

Thanks,
RM

Re: Small regression on Windows

[Junio C Hamano]

Jeff Hostetler  writes:

> The warning text is being written to stderr and then main thread
> sleeps for the 4 seconds.  However, stderr has been redirected to
> the pipe connected to the console_thread that handles the
> coloring/pager.  It is in a blocking ReadFile on the pipe and is
> thus stuck until the main thread runs the corrected command and
> closes the pipe.  It then sees the EOF and prints everything in
> the pipe buffer.

IOW, somehow your stderr is not flushing automatically?

> I'll look into fixing this now.

Thanks.

Re: [PATCH] completion: restore removed line continuating backslash

[Junio C Hamano]

SZEDER Gábor  writes:

> Recent commit 1cd23e9e0 (completion: don't use __gitdir() for git
> commands, 2017-02-03) rewrapped a couple of long lines, and while
> doing so it inadvertently removed a '\' from the end of a line, thus
> breaking completion for 'git config remote.name.push '.
>
> Signed-off-by: SZEDER Gábor 
> ---
>
> I wanted this to be a fixup!, but then noticed that this series is
> already in next, hence the proper commit message.

We get "--format=... : command not found"?
Thanks for a set of sharp eyes.

> I see the last What's cooking marks this series as "will merge to
> master".  That doesn't mean that it will be in v2.12, does it?

I actually was hoping that these can go in.  Others that can (or
should) wait are marked as "Will cook in 'next'".

If you feel uncomfortable and want these to cook longer, please tell
me so.

Thanks.

>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 993085af6..f2b294aac 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2086,7 +2086,7 @@ _git_config ()
>   remote.*.push)
>   local remote="${prev#remote.}"
>   remote="${remote%.push}"
> - __gitcomp_nl "$(__git for-each-ref
> + __gitcomp_nl "$(__git for-each-ref \
>   --format='%(refname):%(refname)' refs/heads)"
>   return
>   ;;

Re: [PATCH 03/11] files-backend: add files_path()

[Ramsay Jones]

On 13/02/17 15:20, Nguyễn Thái Ngọc Duy wrote:
> This will be the replacement for both git_path() and git_path_submodule()
> in this file. The idea is backend takes a git path and use that,
> oblivious of submodule, linked worktrees and such.
> 
> This is the middle step towards that. Eventually the "submodule" field
> in 'struct files_ref_store' should be replace by "gitdir". And a
> compound ref_store is created to combine two files backends together,
> one represents the shared refs in $GIT_COMMON_DIR, one per-worktree. At
> that point, files_path() becomes a wrapper of strbuf_vaddf().
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6582c9b2d..39217a2ca 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -169,6 +169,9 @@ static int files_log_ref_write(const char *refname, const 
> unsigned char *old_sha
>  const unsigned char *new_sha1, const char *msg,
>  int flags, struct strbuf *err);
>  
> +void files_path(struct files_ref_store *refs, struct strbuf *sb,
> + const char *fmt, ...) __attribute__((format (printf, 3, 4)));
> +

Why?

>  static struct ref_dir *get_ref_dir(struct ref_entry *entry)
>  {
>   struct ref_dir *dir;
> @@ -930,6 +933,23 @@ struct files_ref_store {
>  /* Lock used for the main packed-refs file: */
>  static struct lock_file packlock;
>  
> +void files_path(struct files_ref_store *refs, struct strbuf *sb,
> + const char *fmt, ...)
> +{
> + struct strbuf tmp = STRBUF_INIT;
> + va_list vap;
> +
> + va_start(vap, fmt);
> + strbuf_vaddf(&tmp, fmt, vap);
> + va_end(vap);
> + if (refs->submodule)
> + strbuf_git_path_submodule(sb, refs->submodule,
> +   "%s", tmp.buf);
> + else
> + strbuf_git_path(sb, "%s", tmp.buf);
> + strbuf_release(&tmp);
> +}
> +
>  /*
>   * Increment the reference count of *packed_refs.
>   */
> 

ATB,
Ramsay Jones

IT System Alerts

[Melanie Fries]

Help Desk

This e-mail has been sent to you by Outlook Web App If you do not agree to 
update your account, your email account will be blocked.

Click Her to update

Sincerely,
IT-Service Help Desk

Re: Small regression on Windows

[Jeff Hostetler]

On 2/13/2017 1:00 PM, Johannes Sixt wrote:
Please type this, preferably outside of any repo to avoid that it is 
changed; note the command typo:


   git -c help.autocorrect=40 ceckout

Git waits for 4 seconds, but does not write anything until just before 
it exits. It bisects to


a9b8a09c3c30886c79133da9f48ef9f98c21c3b2 is the first bad commit
commit a9b8a09c3c30886c79133da9f48ef9f98c21c3b2
Author: Jeff Hostetler 
Date:   Thu Dec 22 18:09:23 2016 +0100

mingw: replace isatty() hack

Git for Windows has carried a patch that depended on internals
of MSVC runtime, but it does not work correctly with recent MSVC
runtime. A replacement was written originally for compiling
with VC++. The patch in this message is a backport of that
replacement, and it also fixes the previous attempt to make
isatty() tell that /dev/null is *not* an interactive terminal.

Signed-off-by: Jeff Hostetler 
Tested-by: Beat Bolli 
Signed-off-by: Johannes Schindelin 
Signed-off-by: Junio C Hamano 

:04 04 bc3c75bdfc766fe478e160a9452c31bfef50b505 
f7183c3a0726fef7161d5f9ff8c997260025f1bb M  compat


Any ideas? I haven't had time to dig any further.


I'm investigating now.

The warning text is being written to stderr and then main thread sleeps 
for the 4 seconds.
However, stderr has been redirected to the pipe connected to the 
console_thread that
handles the coloring/pager.  It is in a blocking ReadFile on the pipe 
and is thus stuck until
the main thread runs the corrected command and closes the pipe.  It then 
sees the EOF

and prints everything in the pipe buffer.

I'll look into fixing this now.

Jeff

Re: [PATCH] clean: use warning_errno() when appropriate

[Junio C Hamano]

Jeff King  writes:

> I wonder if xmalloc() should be the one doing the saved_errno trick.
> After all, it has only two outcomes: we successfully allocated the
> memory, or we called die().

I would be lying if I said I did not considered it when I wrote the
message you are responding to, but I rejected it because that would
be optimizing for a wrong case, in that most callers of xmalloc()
and friends do not do so in the error codepath, and we would be
penalizing them by doing the saved_errno dance unconditionally.

Re: [PATCH 02/11] files-backend: convert git_path() to strbuf_git_path()

[Ramsay Jones]

On 13/02/17 15:20, Nguyễn Thái Ngọc Duy wrote:
> git_path() and friends are going to be killed in files-backend.c in near
> future. And because there's a risk with overwriting buffer in
> git_path(), let's convert them all to strbuf_git_path(). We'll have
> easier time killing/converting strbuf_git_path() then because we won't
> have to worry about memory management again.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c | 114 
> ---
>  1 file changed, 90 insertions(+), 24 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 75565c3aa..6582c9b2d 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2169,6 +2169,8 @@ static int lock_packed_refs(struct files_ref_store 
> *refs, int flags)
>   static int timeout_configured = 0;
>   static int timeout_value = 1000;
>   struct packed_ref_cache *packed_ref_cache;
> + struct strbuf sb = STRBUF_INIT;
> + int ret;
>  
>   files_assert_main_repository(refs, "lock_packed_refs");
>  
> @@ -2177,10 +2179,13 @@ static int lock_packed_refs(struct files_ref_store 
> *refs, int flags)
>   timeout_configured = 1;
>   }
>  
> - if (hold_lock_file_for_update_timeout(
> - &packlock, git_path("packed-refs"),
> - flags, timeout_value) < 0)
> + strbuf_git_path(&sb, "packed-refs");
> + ret = hold_lock_file_for_update_timeout(&packlock, sb.buf,
> + flags, timeout_value);
> + strbuf_release(&sb);
> + if (ret < 0)
>   return -1;
> +
>   /*
>* Get the current packed-refs while holding the lock.  If the
>* packed-refs file has been modified since we last read it,
> @@ -2335,6 +2340,9 @@ static void try_remove_empty_parents(char *name)
>   for (q = p; *q; q++)
>   ;
>   while (1) {
> + struct strbuf sb = STRBUF_INIT;
> + int ret;
> +
>   while (q > p && *q != '/')
>   q--;
>   while (q > p && *(q-1) == '/')
> @@ -2342,7 +2350,10 @@ static void try_remove_empty_parents(char *name)
>   if (q == p)
>   break;
>   *q = '\0';
> - if (rmdir(git_path("%s", name)))
> + strbuf_git_path(&sb, "%s", name);
> + ret = rmdir(sb.buf);
> + strbuf_release(&sb);
> + if (ret)
>   break;
>   }
>  }
> @@ -2431,7 +2442,11 @@ static int repack_without_refs(struct files_ref_store 
> *refs,
>   return 0; /* no refname exists in packed refs */
>  
>   if (lock_packed_refs(refs, 0)) {
> - unable_to_lock_message(git_path("packed-refs"), errno, err);
> + struct strbuf sb = STRBUF_INIT;
> +
> + strbuf_git_path(&sb, "packed-refs");
> + unable_to_lock_message(sb.buf, errno, err);
> + strbuf_release(&sb);
>   return -1;
>   }
>   packed = get_packed_refs(refs);
> @@ -2529,11 +2544,12 @@ static int rename_tmp_log(const char *newrefname)
>  {
>   int attempts_remaining = 4;
>   struct strbuf path = STRBUF_INIT;
> + struct strbuf tmp_renamed_log = STRBUF_INIT;
>   int ret = -1;
>  
> - retry:
> - strbuf_reset(&path);
>   strbuf_git_path(&path, "logs/%s", newrefname);
> + strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
> + retry:
>   switch (safe_create_leading_directories_const(path.buf)) {
>   case SCLD_OK:
>   break; /* success */
> @@ -2546,7 +2562,7 @@ static int rename_tmp_log(const char *newrefname)
>   goto out;
>   }
>  
> - if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
> + if (rename(tmp_renamed_log.buf, path.buf)) {
>   if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 
> 0) {
>   /*
>* rename(a, b) when b is an existing
> @@ -2574,6 +2590,7 @@ static int rename_tmp_log(const char *newrefname)
>   ret = 0;
>  out:
>   strbuf_release(&path);
> + strbuf_release(&tmp_renamed_log);
>   return ret;
>  }
>  
> @@ -2614,9 +2631,15 @@ static int files_rename_ref(struct ref_store 
> *ref_store,
>   int flag = 0, logmoved = 0;
>   struct ref_lock *lock;
>   struct stat loginfo;
> - int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
> + struct strbuf sb_oldref = STRBUF_INIT;
> + struct strbuf sb_newref = STRBUF_INIT;
> + struct strbuf tmp_renamed_log = STRBUF_INIT;
> + int log, ret;
>   struct strbuf err = STRBUF_INIT;
>  
> + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
> + log = !lstat(sb_oldref.buf, &loginfo);
> + strbuf_release(&sb_oldref);
>   if (log && S_ISLNK(loginfo.st_mode))
>   return error("reflog for %s is a symlink", oldrefname);
>  
> @@ -2630,7 +2653,12 @@ stat

[PATCH] docs/gitremote-helpers: fix unbalanced quotes

[Jeff King]

Each of these options is missing the closing single-quote on
the option name. This understandably confuses asciidoc,
which ends up rendering a stray quote, like:

  option cloning {'true|false}

Signed-off-by: Jeff King 
---
 Documentation/gitremote-helpers.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/gitremote-helpers.txt 
b/Documentation/gitremote-helpers.txt
index 9e8681f9e..7e59c50b1 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -452,14 +452,14 @@ set by Git if the remote helper has the 'option' 
capability.
Request the helper to perform a force update.  Defaults to
'false'.
 
-'option cloning {'true'|'false'}::
+'option cloning' {'true'|'false'}::
Notify the helper this is a clone request (i.e. the current
repository is guaranteed empty).
 
-'option update-shallow {'true'|'false'}::
+'option update-shallow' {'true'|'false'}::
Allow to extend .git/shallow if the new refs require it.
 
-'option pushcert {'true'|'false'}::
+'option pushcert' {'true'|'false'}::
GPG sign pushes.
 
 SEE ALSO
-- 
2.12.0.rc1.466.g70234cfd8

Re: Bug in 'git describe' if I have two tags on the same commit.

[Junio C Hamano]

Kevin Daudt  writes:

> On Sun, Feb 12, 2017 at 01:15:22PM +0100, Istvan Pato wrote:
>
>> (master) [1] % git show-ref --tag
>> 76c634390... refs/tags/1.0.0
>> b77c7cd17... refs/tags/1.1.0
>> b77c7cd17... refs/tags/1.2.0
>> 
>> (master) % git describe --tags --always
>> 1.1.0-1-ge9e9ced
>> 
>> ### Expected: 1.2.0
>> ...
>
> Are these lightweight tags? Only annotated tags have a date associated
> to them, which is where the rel-notes refers to. 

Good eyes.  The fact that the two points at the same object means
that even if they were both annotated tags, they have the same
tagger dates.

If the code that compares the candidates and picks better tag to
describe the object with knows the refnames of these "tags", I'd
imagine that we could use the versioncmp() logic as the final tie
breaker, but I do not offhand remember if the original refname we
took the tag (or commit) from is propagated that deep down the
callchain.  It probably does not, so some code refactoring may be
needed if somebody wants to go in that direction.

Re: [PATCH] squash! completion: fill COMPREPLY directly when completing refs

[Junio C Hamano]

SZEDER Gábor  writes:

> On Fri, Feb 10, 2017 at 10:44 PM, Junio C Hamano  wrote:
>
>> Should I expect a reroll to come, or is this the only fix-up to the
>> series that begins at <20170203025405.8242-1-szeder@gmail.com>?
>>
>> No hurries.
>
> Yes, definitely.
>
> I found a minor bug in the middle of the series, and haven't quite
> made up my mind about it and its fix yet.  Perhaps in a day or three.
>
> Regarding the 'strip' -> 'lstrip' 'for-each-ref' format modifier
> rename that broke pu: I should keep using 'strip', right?

Right.  I view the removal of 'strip' as an accident when 'lstrip'
was introduced, not an intended change.

Re: [PATCH 01/11] refs-internal.c: make files_log_ref_write() static

[Ramsay Jones]

On 13/02/17 15:20, Nguyễn Thái Ngọc Duy wrote:
> Created in 5f3c3a4e6f (files_log_ref_write: new function - 2015-11-10)
> but probably never used outside refs-internal.c
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c | 3 +++
>  refs/refs-internal.h | 4 
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index cdb6b8ff5..75565c3aa 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -165,6 +165,9 @@ static struct ref_entry *create_dir_entry(struct 
> files_ref_store *ref_store,
> const char *dirname, size_t len,
> int incomplete);
>  static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
> +static int files_log_ref_write(const char *refname, const unsigned char 
> *old_sha1,
> +const unsigned char *new_sha1, const char *msg,
> +int flags, struct strbuf *err);

Why? I don't like adding forward declarations unless it
is absolutely necessary (ie mutually recursive functions),
and even in the current 'pu' branch (@c04899d50), the
definition of this function appears before all uses in
this file. (ie, just add static to the definition).

What am I missing?

ATB,
Ramsay Jones

>  
>  static struct ref_dir *get_ref_dir(struct ref_entry *entry)
>  {
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 33adbf93b..59e65958a 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -222,10 +222,6 @@ struct ref_transaction {
>   enum ref_transaction_state state;
>  };
>  
> -int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
> - const unsigned char *new_sha1, const char *msg,
> - int flags, struct strbuf *err);
> -
>  /*
>   * Check for entries in extras that are within the specified
>   * directory, where dirname is a reference directory name including
> 

Re: [PATCH v3 0/5] stash: support pathspec argument

[Jeff King]

On Mon, Feb 13, 2017 at 03:14:55PM +0100, Matthieu Moy wrote:

> I don't remember doing this intentionally. The goal was really to
> prevent typos like "git stash -p drop" (which would have been
> interpreted as "git stash save -p drop" previously). So, let's consider
> this "only messages starting with - are accepted" as a bug: moving to
> the new "push" command it a nice opportunity to fix it without worrying
> about compatibility. A bit more about this in this old thread:

Yeah, I consider it a bug that we treat "-foo" as a message there.
Fortunately I think you really have to try hard to abuse it:

  # doesn't work, because "save" complains about "-foo" as an option
  git stash -foo

  # does save stash with message "-foo"
  git stash -- -foo

  # doesn't work, because _any_ non-option argument is rejected
  git stash -- use --foo option

> I think we can safely allow both
> 
>   git stash -p -- 
>   git stash push -p 
> 
> But allowing the same without -- is a bit more dangerous for the reason
> above:
> 
>   git stash -p drop
> 
> would be interpreted as
> 
>   git stash push -p drop
> 
> (i.e. limit the stash to file named "drop"), and this would almost
> certainly not be what the user wanted.

Is it really that dangerous, though? The likely outcome is Git saying
"nope, you don't have any changes to the file named drop". Of course the
user may have meant something different, but I feel like "-p" is a good
indicator that they are interested in making an actual stash.

Think about this continuum of commands:

  1. git stash droop

  2. git stash -p drop

  3. git stash -p -- drop

  4. git stash push -p drop

I think we can all agree that (4) is reasonable and safe. And I suspect
we'd all agree that (1) is suspect (you probably meant "drop", not "push
droop").

But between (2) and (3), I don't see much difference. The interesting
difference between (1) and (2) is the addition of "-p", which tells us
that the user expects to save a stash.

Another way of thinking of it is that "-p" as a command name is an alias
for "push -p". If you wanted to err on the conservative side, you could
literally implement it that way (so "git stash -k -p foo" would not
work; you'd have to use "git stash -p -k foo").

> > The other question is how we would deal with the -m flag for
> > specifying a stash message.  I think we could special case it, but
> > that would allow users to do things such as git stash -m apply, which
> > would make the interface a bit more error prone.  So I'm leaning
> > towards disallowing that for now.
> 
> We already have "git commit -m  ", so I think stash
> should just do the same thing as commit. Or, did I miss something?

The complexity is that right now, the first-level decision of "which
stash sub-command am I running?" doesn't know about any options. So "git
stash -m foo" would be rejected in the name of typo prevention, unless
that outer decision learns about "-m" as an option.

-Peff

Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"

[Junio C Hamano]

Junio C Hamano  writes:

> Matthieu Moy  writes:
>
>> Siddharth Kannan  writes:
>>
>>> +   if (!strcmp(name, "-")) {
>>> +   name = "@{-1}";
>>> +   len = 5;
>>> +   }
>>
>> One drawback of this approach is that further error messages will be
>> given from the "@{-1}" string that the user never typed.
>
> Right.
>
>> There are at least:
>>
>> $ git grep -n -A1 'strcmp.*"-"' | grep -B 1 '@\{1\}'
>> builtin/checkout.c:975: if (!strcmp(arg, "-"))
>> builtin/checkout.c-976- arg = "@{-1}";
>
> I didn't check the surrounding context to be sure, but I think this
> "- to @{-1}" conversion cannot be delegated down to revision parsing
> that eventually wants to return a 40-hex as the result.  
>
> We do want a branch _name_ sometimes when we say "@{-1}"; "checkout
> master" (i.e. checkout by name) and "checkout master^0" (i.e. the
> same commit object, but not by name) do different things.

FYI, the "@{-} to branch name" translation happens in
interpret_branch_name().  I do not offhand recall if any callers
protect their calls to the function with conditionals that assume
the thing must begin with "@{" or cannot begin with "-" (the latter
of which is similar to the topic of patch 1/2 of this series), but I
suspect that teaching the function that "-" means the same as
"@{-1}" would bring us closer to where we want to go.

Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"

[Junio C Hamano]

Matthieu Moy  writes:

> Siddharth Kannan  writes:
>
>> +if (!strcmp(name, "-")) {
>> +name = "@{-1}";
>> +len = 5;
>> +}
>
> One drawback of this approach is that further error messages will be
> given from the "@{-1}" string that the user never typed.

Right.

> There are at least:
>
> $ git grep -n -A1 'strcmp.*"-"' | grep -B 1 '@\{1\}'
> builtin/checkout.c:975: if (!strcmp(arg, "-"))
> builtin/checkout.c-976- arg = "@{-1}";

I didn't check the surrounding context to be sure, but I think this
"- to @{-1}" conversion cannot be delegated down to revision parsing
that eventually wants to return a 40-hex as the result.  

We do want a branch _name_ sometimes when we say "@{-1}"; "checkout
master" (i.e. checkout by name) and "checkout master^0" (i.e. the
same commit object, but not by name) do different things.

> builtin/merge.c:1231:   } else if (argc == 1 && !strcmp(argv[0], "-")) {
> builtin/merge.c-1232-   argv[0] = "@{-1}";
> --
> builtin/revert.c:158:   if (!strcmp(argv[1], "-"))
> builtin/revert.c-159-   argv[1] = "@{-1}";

These should be safe to delegate down.

> builtin/worktree.c:344: if (!strcmp(branch, "-"))
> builtin/worktree.c-345- branch = "@{-1}";

I do not know about this one, but it smells like a branch name that
wants to be used before it gets turned into 40-hex.