Re: difflame improvements

[Edmundo Carmona Antoranz]

On Thu, Feb 16, 2017 at 11:17 PM, Jeff King  wrote:

> This isn't difflame's fault; that's what "git blame" tells you about
> that line. But since I already told difflame "v2.6.5..HEAD", it would
> probably make sense to similarly limit the blame to that range. That
> turns up a boundary commit for the line. Which is _also_ not helpful,
> but at least the tool is telling me that the line came from before
> v2.6.5, and I don't really need to care much about it.


I'm running my own tests on difflame and I have a theory about "when"
it breaks at least one of the cases when it breaks:

Analysis for deleted lines is being driven by git blame --reverse.
What I have noticed is that it "breaks" when blame --reverse drives
the analysis into revisions where "treeish1" is not part of their
history (like, bringing analysis "to the sides" of treeish1 instead of
keeping analysis in revisions in the history of treeish2 that have
treeish1 as one of their ancestors which is definitely a valid
case for analysis, anyway). In this case, blame --reverse stops being
helpful.

Take this example (I just pushed a debug-deletion branch into gh...
probably more debugging messages will be needed):

$ difflame.py HEAD~100 HEAD -- Documentation/git-commit.txt
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f2ab0ee2e..4f8f20a36 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -265,7 +265,8 @@ FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].)
bcf9626a71 (Matthieu Moy  2016-06-28 13:40:11 +0200 265)   If this
option is specified together with `--amend`, then
04c8ce9c1c (Markus Heidelberg 2008-12-19 13:14:18 +0100 266)   no
paths need to be specified, which can be used to amend
d4ba07cac5 (Johannes Sixt 2008-04-10 13:33:09 +0200 267)   the
last commit without committing changes that have
   Range of revisions: 02db2d..066fb04
   Treeish1 02db2d04: 02db2d042 Merge branch 'ah/grammos'
   Treeish2 066fb0494: 066fb0494 blame: draft of line format
   Blamed Revision afe0e2a39: afe0e2a39 Merge branch
'da/difftool-dir-diff-fix'
   Original Filename a/Documentation/git-commit.txt Deleted Line 268
   Children revisions:
   3aead1cad7a: 3aead1cad Merge branch 'ak/commit-only-allow-empty'
   There's only one child revision on that revision the line
we are tracking is gone
   Parents of this child revision:
   afe0e2a39166: afe0e2a39 Merge branch 'da/difftool-dir-diff-fix'
   beb635ca9ce: beb635ca9 commit: remove 'Clever' message
for --only --amend
   Finding parent where the line has been deleted:
   beb635ca9: beb635ca9 commit: remove 'Clever' message
for --only --amend
   Range of revisions: 02db2d042..beb635c
   Treeish1 02db2d0: 02db2d042 Merge branch 'ah/grammos'
   Treeish2 beb635c: beb635ca9 commit: remove 'Clever'
message for --only --amend
   Blamed Revision 02db2d0: 02db2d042 Merge branch 'ah/grammos'
   Original Filename a/Documentation/git-commit.txt Deleted Line 268
   Children revisions:
   Found no children... will return the original blamed revision
(02db2d0) saying that the deleting revision could not be found
   beb635ca9 commit: remove 'Clever' message for --only --amend
-beb635ca9 (Andreas Krey 2016-12-09 05:10:21 +0100 268)
already been staged.
   319d83524 commit: make --only --allow-empty work without paths
+319d835240 (Andreas Krey  2016-12-02 23:15:13 +0100 268)
already been staged. ...
+319d835240 (Andreas Krey  2016-12-02 23:15:13 +0100 269)   paths
are also not requi...
d4ba07cac5 (Johannes Sixt 2008-04-10 13:33:09 +0200 270)
1947bdbc31 (Junio C Hamano2008-06-22 14:32:27 -0700 271) -u[]::
1947bdbc31 (Junio C Hamano2008-06-22 14:32:27 -0700 272)
--untracked-files[=]::



I know that line 268 was deleted on 319d835240.

So on the first round of merge analysis it says "let's go into
beb635ca9". That's fine. That's exactly the path that is required to
reach 319d835240. But then when using this new "range of revisions"
for git blame --reverse, we get that line 268 is not telling us
anything useful:

$ git blame --reverse -L268,268 02db2d042..beb635c --
Documentation/git-commit.txt
^02db2d042 (Junio C Hamano 2016-12-19 14:45:30 -0800 268)
already been staged.

So, instead of pointing to 319d835240 (the parent of beb635c), it's
basically saying something like "I give up". My hunch (haven't sat
down to digest all the details about the output of git blame
--reverse... YET) is that, given that 02db2d042 is _not_ part of the
history of beb635c, git blame reverse is trying to tell me just
that... and that means I'll have to "script around this scenario".

$ git merge-base 02db2d042 beb635c
0202c411edc25940cc381bf317badcdf67670be4


Thanks in advance.

Re: difflame improvements

[Jeff King]

On Tue, Feb 14, 2017 at 11:19:05PM -0600, Edmundo Carmona Antoranz wrote:

> I've been working on detecting revisions where a "real" deletion was
> made and I think I advanced a lot in that front. I still have to work
> on many scenarios (renamed files, for example... also performance) but
> at least I'm using a few runs against git-scm history and the results
> are "promising":

I played with this a bit more, and it did turn up the correct results
for some deletions in my experiments.

One thing I noticed is that it also turned up nonsense for lines that
blame in weird ways. For instance, I have a diff like this (these are
real examples, but don't pay attention to the sha1s; it's in a fork of
git, not upstream):

  $ git diff v2.6.5 builtin/prune-packed.c
  diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
  index 7cf900ea07..5e3727e841 100644
  --- a/builtin/prune-packed.c
  +++ b/builtin/prune-packed.c
  @@ -2,6 +2,7 @@
   #include "cache.h"
   #include "progress.h"
   #include "parse-options.h"
  +#include "gh-log.h"
   
   static const char * const prune_packed_usage[] = {
N_("git prune-packed [-n | --dry-run] [-q | --quiet]"),
  @@ -29,8 +30,11 @@ static int prune_object(const unsigned char *sha1, const 
char *path,
   
if (*opts & PRUNE_PACKED_DRY_RUN)
printf("rm -f %s\n", path);
  - else
  + else {
  + gh_logf("prune", "%s Duplicate loose object pruned\n",
  + sha1_to_hex(sha1));
unlink_or_warn(path);
  + }
return 0;
   }
   

Running difflame on it says this:

  $ python /path/to/difflame.py v2.6.5..HEAD -- builtin/prune-packed.c
  [...]
  -2c0b29e662 (Jeff King 2016-01-26 15:27:55 -0500 32)  else
  +d60032f8640 builtin/prune-packed.c (Jeff King2015-02-02 23:15:33 
-0500 33)   else {
  +d60032f8640 builtin/prune-packed.c (Jeff King2015-02-02 23:15:33 
-0500 34)   gh_logf("prune", "%s Duplicate loose object pruned\n",
  +d60032f8640 builtin/prune-packed.c (Jeff King2015-02-02 23:15:33 
-0500 35)   sha1_to_hex(sha1));
   0d3b729680e builtin/prune-packed.c (Jeff King2014-10-15 18:40:53 
-0400 36)   unlink_or_warn(path);
  +2396ec85bd1 prune-packed.c (Linus Torvalds   2005-07-03 14:27:34 
-0700 37)   }

There are two weird things. One is that the old "else" is attributed to
my 2c0b29e662. That's quite weird, because that is a merge commit which
did not touch the file at all. I haven't tracked it down, but presumably
that is weirdness with the --reverse blame.

But there's another one, that I think is easy to fix. The closing brace
is attributed to some ancient commit from Linus. Which yes, I'm sure had
a closing brace, but not _my_ closing brace that was added by
d60032f8640, that the rest of the lines got attributed to.

This isn't difflame's fault; that's what "git blame" tells you about
that line. But since I already told difflame "v2.6.5..HEAD", it would
probably make sense to similarly limit the blame to that range. That
turns up a boundary commit for the line. Which is _also_ not helpful,
but at least the tool is telling me that the line came from before
v2.6.5, and I don't really need to care much about it.

Part of this is that my use case may be a bit different than yours. I
don't actually want to look at the blame results directly. I just want
to see the set of commits that I'd need to look at and possibly
cherry-pick in order to re-create the diff.

-Peff

Re: [PATCH] show-branch: fix crash with long ref name

[Jeff King]

On Thu, Feb 16, 2017 at 01:40:00PM +0100, Christian Couder wrote:

> > I started to add some tests, but I had second thoughts. It _is_ nice
> > to show off the fix, but as far as regressions go, this specific case is
> > unlikely to come up again. What would be more valuable, I think, is a
> > test script which set up a very long refname (not just 150 bytes or
> > whatever) and ran it through a series of git commands.
> 
> I agree that a test script running through a series of command with
> long refnames would be great.
> 
> But I think the refname should not necesarily be too long. As I wrote
> in the commit message of my patch, if the ref name had been much
> longer the crash would not have happened because the ref could not
> have been created in the first place.

Right, I think there's a tension there. Too short and it is not
interesting, and too long and things start to fail for uninteresting
reasons (e.g., your filesystem can't handle the name).

> So the best would be to run through a series of commands with a
> refname ranging from let's say 80 chars to 300 chars.
> 
> That would have a chance to catch crashes due to legacy code using for
> example things like `char stuff[128]` or `char stuff[256]`.

But that doesn't catch `char stuff[512]`. I think you'd really want a
range of sizes, and to test all of them. Worse, though, is it's not
clear how you decide when a test has failed. Obviously smashing the
stack is a bad outcome, but part of the goal would presumably be to
flush out unnecessary length limitations elsewhere.

I got about that far in my thinking before saying "wow, this is getting
to be complicated for not much gain".

> > So I dunno. It seems like being thorough is a
> > lot of hassle for not much gain. Being not-thorough is easy, but is
> > mostly a token that is unlikely to find any real bugs.
> 
> Yeah, if we really care, it might be better to start using a fuzzer or
> a property based testing tool instead of bothering with these kind of
> tests by ourselves, which is also a different topic.

Yes, I'd agree that a fuzzer is probably a better choice. I played with
AFL a bit back when it came out, but I never got it to turn up anything
useful.

I am disappointed that this obvious memory problem survived for so long.
I did quite a bit of auditing for such problems a year or two ago, but I
focused on problematic functions like strcpy, sprintf, etc.

It's easy to use memcpy() wrong, too, but it's hard to audit. There are
a lot of calls, and you have to match up the copied length with a value
computed elsewhere. I traded a lot of them out back then for safer
variants (like the FLEX_ALLOC stuff), but many calls just fundamentally
need something unsafe like memcpy to get their job done.

-Peff

Re: `git show --oneline commit` does not do what's intuitively expected

[Jeff King]

On Fri, Feb 17, 2017 at 02:51:36AM +0100, Luna Kid wrote:

> tl;dr; --> Please add --no-diff (or --msg-only) to git show. We'll
> love you for that. :)

I think it is already spelled "--no-patch", or "-s" for short.

> Please note that "show" is such a profoundly generic command verb,
> probably also used heavily in git, especially to show commits, that it
> comes to mind as second nature, without thinking, as the primary (or
> even as "the only") choice for showing various items in various ways
> -- which, in fact, it already properly does, indeed.

Right. That's what's it for. The DESCRIPTION section of the manpage
starts with:

   Shows one or more objects (blobs, trees, tags and commits).

   For commits it shows the log message and textual diff. It also
   presents the merge commit in a special format as produced by git
   diff-tree --cc.

   For tags, it shows[...etc...]

I guess that second paragraph could mention "--no-patch" explicitly to
disable it. It's documented in the COMMON DIFF OPTIONS section, but of
course there are quite a few options listed, so it's easy to miss.

-Peff

[PATCH 1/3] delete_refs(): accept a reflog message argument

[Kyle Meyer]

When the current branch is renamed with 'git branch -m/-M' or deleted
with 'git update-ref -m -d', the event is recorded in HEAD's log
with an empty message.

In preparation for adding a more meaningful message to HEAD's log in
these cases, update delete_ref() to take a message argument and pass
it along to ref_transaction_delete().  Modify all callers to pass NULL
for the new message argument; no change in behavior is intended.

Signed-off-by: Kyle Meyer 
---
 builtin/am.c   | 4 ++--
 builtin/branch.c   | 2 +-
 builtin/notes.c| 4 ++--
 builtin/remote.c   | 4 ++--
 builtin/replace.c  | 2 +-
 builtin/reset.c| 2 +-
 builtin/symbolic-ref.c | 2 +-
 builtin/tag.c  | 2 +-
 builtin/update-ref.c   | 2 +-
 fast-import.c  | 2 +-
 refs.c | 4 ++--
 refs.h | 2 +-
 refs/files-backend.c   | 6 +++---
 transport.c| 2 +-
 14 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 31fb60578..e08c739d4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1049,7 +1049,7 @@ static void am_setup(struct am_state *state, enum 
patch_format patch_format,
} else {
write_state_text(state, "abort-safety", "");
if (!state->rebasing)
-   delete_ref("ORIG_HEAD", NULL, 0);
+   delete_ref("ORIG_HEAD", NULL, 0, NULL);
}
 
/*
@@ -2172,7 +2172,7 @@ static void am_abort(struct am_state *state)
has_curr_head ? _head : NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
else if (curr_branch)
-   delete_ref(curr_branch, NULL, REF_NODEREF);
+   delete_ref(curr_branch, NULL, REF_NODEREF, NULL);
 
free(curr_branch);
am_destroy(state);
diff --git a/builtin/branch.c b/builtin/branch.c
index 9d30f55b0..44f23208f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -252,7 +252,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
}
 
if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
-  REF_NODEREF)) {
+  REF_NODEREF, NULL)) {
error(remote_branch
  ? _("Error deleting remote-tracking branch '%s'")
  : _("Error deleting branch '%s'"),
diff --git a/builtin/notes.c b/builtin/notes.c
index 5248a9bad..991448d4e 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -681,9 +681,9 @@ static int merge_abort(struct notes_merge_options *o)
 * notes_merge_abort() to remove .git/NOTES_MERGE_WORKTREE.
 */
 
-   if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0))
+   if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0, NULL))
ret += error(_("failed to delete ref NOTES_MERGE_PARTIAL"));
-   if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF))
+   if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF, NULL))
ret += error(_("failed to delete ref NOTES_MERGE_REF"));
if (notes_merge_abort(o))
ret += error(_("failed to remove 'git notes merge' worktree"));
diff --git a/builtin/remote.c b/builtin/remote.c
index 5339ed6ad..bfa8a5189 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -691,7 +691,7 @@ static int mv(int argc, const char **argv)
read_ref_full(item->string, RESOLVE_REF_READING, oid.hash, 
);
if (!(flag & REF_ISSYMREF))
continue;
-   if (delete_ref(item->string, NULL, REF_NODEREF))
+   if (delete_ref(item->string, NULL, REF_NODEREF, NULL))
die(_("deleting '%s' failed"), item->string);
}
for (i = 0; i < remote_branches.nr; i++) {
@@ -1248,7 +1248,7 @@ static int set_head(int argc, const char **argv)
head_name = xstrdup(states.heads.items[0].string);
free_remote_ref_states();
} else if (opt_d && !opt_a && argc == 1) {
-   if (delete_ref(buf.buf, NULL, REF_NODEREF))
+   if (delete_ref(buf.buf, NULL, REF_NODEREF, NULL))
result |= error(_("Could not delete %s"), buf.buf);
} else
usage_with_options(builtin_remote_sethead_usage, options);
diff --git a/builtin/replace.c b/builtin/replace.c
index b58c714cb..d32d0a3ae 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -121,7 +121,7 @@ static int for_each_replace_name(const char **argv, 
each_replace_name_fn fn)
 static int delete_replace_ref(const char *name, const char *ref,
  const unsigned char *sha1)
 {
-   if (delete_ref(ref, sha1, 0))
+   if (delete_ref(ref, sha1, 0, NULL))
return 1;
printf("Deleted replace ref '%s'\n", name);
return 0;
diff --git a/builtin/reset.c b/builtin/reset.c
index 

[PATCH 2/3] update-ref: pass reflog message argument to delete_refs

[Kyle Meyer]

Now that delete_refs() accepts a reflog message, pass the
user-provided message to delete_refs() rather than silently dropping
it.  The doesn't matter for the deleted ref's log because the log is
deleted along with the ref, but this entry will show up in HEAD's
reflog when deleting a checked out branch.

Signed-off-by: Kyle Meyer 
---
I'm not sure if the test here (or in the next patch) is worth including.

 builtin/update-ref.c  | 2 +-
 t/t1400-update-ref.sh | 9 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a41f9adf1..f642acc22 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -435,7 +435,7 @@ int cmd_update_ref(int argc, const char **argv, const char 
*prefix)
 */
return delete_ref(refname,
  (oldval && !is_null_sha1(oldsha1)) ? oldsha1 
: NULL,
- flags, NULL);
+ flags, msg);
else
return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
  flags | create_reflog_flag,
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index b0ffc0b57..65918d984 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -85,6 +85,15 @@ test_expect_success "delete $m (by HEAD)" '
 '
 rm -f .git/$m
 
+test_expect_success "deleting current branch adds message to HEAD's log" '
+   git update-ref $m $A &&
+   git symbolic-ref HEAD $m &&
+   git update-ref -mdelmsg -d $m &&
+   ! test -f .git/$m &&
+   grep "delmsg$" .git/logs/HEAD >/dev/null
+'
+rm -f .git/$m
+
 test_expect_success 'update-ref does not create reflogs by default' '
test_when_finished "git update-ref -d $outside" &&
git update-ref $outside $A &&
-- 
2.11.1

[PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Kyle Meyer]

When the current branch is renamed, the deletion of the old ref is
recorded in HEAD's log with an empty message.  Now that delete_refs()
accepts a reflog message, provide a more descriptive message.  This
message will not be included in the reflog of the renamed branch, but
its log already covers the renaming event with a message of "Branch:
renamed ...".

Signed-off-by: Kyle Meyer 
---
 refs/files-backend.c | 8 +++-
 t/t3200-branch.sh| 4 
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index ffa75d816..bb5df7ee6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2598,6 +2598,7 @@ static int files_rename_ref(struct ref_store *ref_store,
struct stat loginfo;
int log = !lstat(git_path("logs/%s", oldrefname), );
struct strbuf err = STRBUF_INIT;
+   struct strbuf logmsg_del = STRBUF_INIT;
 
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
@@ -2616,10 +2617,15 @@ static int files_rename_ref(struct ref_store *ref_store,
return error("unable to move logfile logs/%s to 
"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
 
-   if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
+   strbuf_addf(_del, "Deleted %s", oldrefname);
+
+   if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, logmsg_del.buf)) {
error("unable to delete old %s", oldrefname);
+   strbuf_release(_del);
goto rollback;
}
+   strbuf_release(_del);
+
 
/*
 * Since we are doing a shallow lookup, sha1 is not the
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 8a833f354..4af7cd2b7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -139,6 +139,10 @@ test_expect_success 'git branch -M baz bam should succeed 
when baz is checked ou
test $(git rev-parse --abbrev-ref HEAD) = bam
 '
 
+test_expect_success 'git branch -M baz bam should add entry to .git/logs/HEAD' 
'
+   grep "Deleted refs/heads/baz$" .git/logs/HEAD >/dev/null
+'
+
 test_expect_success 'git branch -M baz bam should succeed when baz is checked 
out as linked working tree' '
git checkout master &&
git worktree add -b baz bazdir &&
-- 
2.11.1

[PATCH 0/3] delete_ref(): support reflog messages

[Kyle Meyer]

[Sorry for the slow response.]

Jeff King  writes:

> On Mon, Jan 16, 2017 at 06:17:29PM -0500, Kyle Meyer wrote:

[...]

>>$ cat .git/logs/refs/heads/new-master
>>0... 68730... Kyle Meyer  1484607020 -0500   commit 
>> (initial): Add file.txt
>>68730... 68730... Kyle Meyer  1484607020 -0500   Branch: 
>> renamed refs/heads/master to refs/heads/new-master
>> 
>>$ cat .git/logs/HEAD
>>0... 68730... Kyle Meyer  1484607020 -0500   commit 
>> (initial): Add file.txt
>>68730... 0... Kyle Meyer  1484607020 -0500
>> 
>> I expected the second line of .git/logs/HEAD to mirror the second line
>> of .git/logs/refs/heads/new-master.  Are the empty message and null sha1
>> in HEAD's entry intentional?
>
> The null sha1 is correct, I think. The branch we were on went away, and
> we use the null sha1 to indicate "no value" in both the creation and
> deletion cases.

I see, thanks.

> I'd say there are two potential improvements:
>
>   - delete_ref() should take a reflog message argument, in case it
> updates the HEAD reflog (as a bonus, this will future-proof us for a
> day when we might keep reflogs for deleted refs).

I've tried to do this in the following patches.

>   - "git branch -m" does seem to realize when we are renaming HEAD,
> because it updates HEAD to point to the new branch name. But it
> should probably insert another reflog entry mentioning the rename
> (we do for "git checkout foo", even when "foo" has the same sha1 as
> the current HEAD).

I haven't worked out how to do this part yet.  I'm guessing the change
will involve modifying split_head_update().

If this is added, should it be instead of, rather than in addition to,
the deletion entry?  If a "Branch: renamed ..." entry is present, it
doesn't seem like the deletion entry is providing any additional
information.

  delete_refs(): accept a reflog message argument
  update-ref: pass reflog message argument to delete_refs
  rename_ref: replace empty deletion message in HEAD's log

 builtin/am.c   |  4 ++--
 builtin/branch.c   |  2 +-
 builtin/notes.c|  4 ++--
 builtin/remote.c   |  4 ++--
 builtin/replace.c  |  2 +-
 builtin/reset.c|  2 +-
 builtin/symbolic-ref.c |  2 +-
 builtin/tag.c  |  2 +-
 builtin/update-ref.c   |  2 +-
 fast-import.c  |  2 +-
 refs.c |  4 ++--
 refs.h |  2 +-
 refs/files-backend.c   | 12 +---
 t/t1400-update-ref.sh  |  9 +
 t/t3200-branch.sh  |  4 
 transport.c|  2 +-
 16 files changed, 39 insertions(+), 20 deletions(-)

-- 
Kyle

`git show --oneline commit` does not do what's intuitively expected

[Luna Kid]

Hello,

tl;dr; --> Please add --no-diff (or --msg-only) to git show. We'll
love you for that. :)

Expanded:

(I've been a casual git user for 2-3 years; "casual" means using a
small subset, nowadays daily, with some less trivial things at times,
like per-commit patching for post-hoc auto-versioning, or
push-deploying (the old way), so not 100% newbie; doing SW for 25+
years. I'm telling this, because I'm probably a representative sample
of a huge number of experienced professionals, who are relatively new
to git (compared to their age...), but already routinely use it in
"survival mode", learning it gradually, on-demand.)

Today I bumped into this issue, which I then tried googling for, and
found it kinda' hanging:
http://stackoverflow.com/questions/17226117/git-show-commit-oneline-not-showing-oneline

The source of the confusion is that git show --oneline insists on
showing the diff, no matter what, while the man page is misleadingly
subtle on that, as illustrated best by a comment on that SO page,
exactly matching my case, too:

"the answer does not however explain why git show HEAD --oneline
does not produce an output as stated in the documentation:
  This is designed to be as compact as possible."

Note: I do understand now (after 5-10 minutes of googling in vain,
mucking around with git, re-reading the man page a few times, and
considering the fact that a bug like that in such a feature couldn't
exist for this long -- in fact, this clue helped me more than the docs
+ googling together), that it's indeed, not a bug, but our lack of
intimate familiarity with git and the terseness/wording of the man
page together cause the confusion.

(BTW, just realized: on the git show man page, also the strangely
implied "log of a commit" concept (i.e. in "Pretty-print the contents
of the commit logs") is partly responsible for the confusion. The term
"log" doesn't mean a single event-like item, but a series of entries,
a running record of events. In VCS lingo the established concept is
that "a log is a list of commits", and that's even the case with git,
too, of course. So, readers of the git show man page will have an
impedance match there, and unnecessary difficulties understanding what
is actually going on. My overloaded brain, for example, apparently
opted for just filtering out that part without warning, upon the first
(two) reading.)

Assuming that we (the SO guy + his upvoters + me) are not the only
ones, I'd suggest that instead of perhaps changing the man page,
there's an other easy, also backwardly compatible, and quite
straightforward way to address this: actually implementing that
"unexpectedly missing" feature people intuitively look for.

Please note that "show" is such a profoundly generic command verb,
probably also used heavily in git, especially to show commits, that it
comes to mind as second nature, without thinking, as the primary (or
even as "the only") choice for showing various items in various ways
-- which, in fact, it already properly does, indeed.

Forcing us to use a different command (git log) for a minor sub-case
of the main "show me a commit" scenario (of git show), is highly
unnatural.

Also, --oneline (as all the other formatting options) look and feel
global to the entire context (result) of the show command, so people
who have not yet unlearned to expect that, will be surprised
(unpleasantly).

However, simply adding --no-diff to the git show command (and the man
page) would help a lot. (Something like --message-only or --only-msg
etc., would be more correct (than the "potentially leaky" complement),
but I'm not familiar with the general use of the "only" modifier in
git options and cannot comment on that, but I've certainly seen the
--no-... form at least.)

Thanks a lot, and have a nice next message! :)
Szabolcs

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

[Junio C Hamano]

Jeff King  writes:

> On Thu, Feb 16, 2017 at 11:30:28AM +0100, Lars Schneider wrote:
>
>> 
>> > On 16 Feb 2017, at 00:48, Junio C Hamano  wrote:
>> > 
>> > The "git -c = cmd" mechanism is to pretend that a
>> 
>> The problem is also present for gitconfig variables e.g.
>> git config --local submodule.UPPERSUB.update none
>
> Hrm, is it?
>
>   $ git config --file foo submodule.UPPERSUB.update none
>   $ cat foo
>   [submodule "UPPERSUB"]
>   update = none
>
> I could believe that some of the submodule code may try to pass it
> through "-c", though, so certain config ends up being missed.

You are right.  

The builtin/config.c::get_value() codepath, when it is not using the
hacky regexp interface, uses config.c::git_config_parse_key(), and
it does the right thing.  git_config_parse_parameter(), which is the
broken one we found, is not used.

When I did the patch in response to Jonathan's observation, I did
briefly wonder if it should be using git_config_parse_key() instead
of doing its own thing, but I didn't follow it up fully before
deciding that it is the quickest to replace the tolower thing.  If I
at least tried to see if it is feasible, I would have noticed that
the query from the command line wouldn't share the same problem as
Lars reported.

I still haven't queued any of the variants I posted (and I do not
think other people sent their own versions, either).  I need to pick
one and queue, with a test or two.  Perhaps after -rc2.  

Others are welcome to work on it while I cut -rc2 tomorrow, so that
by the time I see their patch all that is left for me to do is to
apply it ;-)

Re: [PATCH V2 0/2] Fix l10n

[Junio C Hamano]

Unknown  writes:

> В Чт, 16/02/2017 в 15:33 -0800, Junio C Hamano пишет:
>> 
>> Thanks.  Queued with minor log message fixes and pushed out.
>
> If is not too late, please check the version 3.

I already squashed in the change s/bufp/buf/ in the version I queued
and pushed out on 'pu', so we should be good.  I compared the result
of applying your updated patches and did not see any other changes.

Thanks.

Dear Friend

[Fundacion Belou]

Dear Friend,

I would like to discuss a very important issue with you. I am writing to find 
out if this is your valid email. Please, let me know if this email is valid

Kind regards
Adrien Saif
Attorney to Qatif Group of Companies

Re: [PATCH V2 0/2] Fix l10n

[Unknown]

В Чт, 16/02/2017 в 15:33 -0800, Junio C Hamano пишет:
> Maxim Moseychuk  writes:
> 
> > In some places static size buffers can't store formatted string.
> > If it be happen then git die.
> > 
> > Jonathan Tan: Thanks a lot for your help.
> > 
> > Maxim Moseychuk (2):
> >   bisect_next_all: convert xsnprintf to xstrfmt
> >   stop_progress_msg: convert xsnprintf to xstrfmt
> > 
> >  bisect.c   | 9 +
> >  progress.c | 9 +++--
> >  2 files changed, 8 insertions(+), 10 deletions(-)
> > 
> > --
> > 2.11.1
> 
> Thanks.  Queued with minor log message fixes and pushed out.

If is not too late, please check the version 3.

Re: [PATCH V2 0/2] Fix l10n

[Junio C Hamano]

Maxim Moseychuk  writes:

> In some places static size buffers can't store formatted string.
> If it be happen then git die.
>
> Jonathan Tan: Thanks a lot for your help.
>
> Maxim Moseychuk (2):
>   bisect_next_all: convert xsnprintf to xstrfmt
>   stop_progress_msg: convert xsnprintf to xstrfmt
>
>  bisect.c   | 9 +
>  progress.c | 9 +++--
>  2 files changed, 8 insertions(+), 10 deletions(-)
>
> --
> 2.11.1

Thanks.  Queued with minor log message fixes and pushed out.

[PATCH v3 2/2] stop_progress_msg(): simplification function

[Maxim Moseychuk]

stop_progress_msg() is rarely used and is not demanding to
performance. Use dynamically allocates memory.

Signed-off-by: Maxim Moseychuk 
---
 progress.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/progress.c b/progress.c
index 76a88c573..29378caa0 100644
--- a/progress.c
+++ b/progress.c
@@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, 
const char *msg)
*p_progress = NULL;
if (progress->last_value != -1) {
/* Force the last update */
-   char buf[128], *bufp;
-   size_t len = strlen(msg) + 5;
+   char *buf;
struct throughput *tp = progress->throughput;
 
-   bufp = (len < sizeof(buf)) ? buf : xmallocz(len);
if (tp) {
unsigned int rate = !tp->avg_misecs ? 0 :
tp->avg_bytes / tp->avg_misecs;
throughput_string(>display, tp->curr_total, rate);
}
progress_update = 1;
-   xsnprintf(bufp, len + 1, ", %s.\n", msg);
-   display(progress, progress->last_value, bufp);
-   if (buf != bufp)
-   free(bufp);
+   buf = xstrfmt(", %s.\n", msg);
+   display(progress, progress->last_value, buf);
+   free(buf);
}
clear_progress_signal();
if (progress->throughput)
-- 
2.11.1

[PATCH v3 0/2] Fix l10n

[Maxim Moseychuk]

In some places fixed-size buffers can't store formatted string.
If it be happen then git die.

Jonathan Tan, Jeff King thanks a lot for your help.
This is really my first patches. Your help is invaluable.

Maxim Moseychuk (2):
  bisect_next_all(): fix bisect crash when used the gettext translation
  stop_progress_msg(): simplification function

 bisect.c   |  9 +
 progress.c | 11 ---
 2 files changed, 9 insertions(+), 11 deletions(-)

--
2.11.1

[PATCH v3 1/2] bisect_next_all(): fix bisect crash when used the gettext translation

[Maxim Moseychuk]

The buffer steps_msg[32] is too small.
Translated "(roughly %d step)" string can not be located in the buffer.

Solution: using xstrfmt which dynamically allocates memory.

Dummy solution: just increase steps_msg size but is not safe.
That feels pretty hacky, though. In practice the set of translations is
contained, but it doesn't have to be (and certainly nobody would notice
if a new translation was added with a longer name until a user
complained).

Reproduce bug: "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources.

Signed-off-by: Maxim Moseychuk 
---
 bisect.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/bisect.c b/bisect.c
index 21bc6daa4..787543cad 100644
--- a/bisect.c
+++ b/bisect.c
@@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
struct commit_list *tried;
int reaches = 0, all = 0, nr, steps;
const unsigned char *bisect_rev;
-   char steps_msg[32];
+   char *steps_msg;
 
read_bisect_terms(_bad, _good);
if (read_bisect_refs())
@@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)
 
nr = all - reaches - 1;
steps = estimate_bisect_steps(all);
-   xsnprintf(steps_msg, sizeof(steps_msg),
- Q_("(roughly %d step)", "(roughly %d steps)", steps),
- steps);
+
+   steps_msg = xstrfmt(Q_("(roughly %d step)", "(roughly %d steps)",
+ steps), steps);
/* TRANSLATORS: the last %s will be replaced with
   "(roughly %d steps)" translation */
printf(Q_("Bisecting: %d revision left to test after this %s\n",
  "Bisecting: %d revisions left to test after this %s\n",
  nr), nr, steps_msg);
+   free(steps_msg);
 
return bisect_checkout(bisect_rev, no_checkout);
 }
-- 
2.11.1

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

[Jeff King]

On Thu, Feb 16, 2017 at 11:30:28AM +0100, Lars Schneider wrote:

> 
> > On 16 Feb 2017, at 00:48, Junio C Hamano  wrote:
> > 
> > The "git -c = cmd" mechanism is to pretend that a
> 
> The problem is also present for gitconfig variables e.g.
> git config --local submodule.UPPERSUB.update none

Hrm, is it?

  $ git config --file foo submodule.UPPERSUB.update none
  $ cat foo
  [submodule "UPPERSUB"]
update = none

I could believe that some of the submodule code may try to pass it
through "-c", though, so certain config ends up being missed.

AFAICT, though, the writing code takes what you gave it verbatim. The
reader is responsible for downcasing everything but the subsection
before it hands it to a callback. Commands calling git-config for lookup
should generally ask for the canonical downcased name. There is some
code to downcase, but IIRC there are corner cases around some of the
regex lookup functions.

-Peff

Re: [RFCv4 PATCH 00/14] Checkout aware of Submodules!

[Jacob Keller]

On Thu, Feb 16, 2017 at 2:56 PM, Stefan Beller  wrote:
> On Thu, Feb 16, 2017 at 2:54 PM, Jacob Keller  wrote:
>> On Thu, Feb 16, 2017 at 2:00 PM, Junio C Hamano  wrote:
>>> Stefan Beller  writes:
>>>
 Integrate updating the submodules into git checkout,...
>>>
>>> It was more or less a pleasant read, once I decided to pretend that
>>> I were a machine who uses identifiers only to identify locations in
>>> the program ;-) IOW, for human consumption, the new names introduced
>>> were sometimes quite confusing and went against helping understanding.
>>>
>>
>> Based on my cursory reading, I agree. I had trouble understanding how
>> the process worked because the names were somewhat confusing. They
>> started to make some sense as I read more. I think v4 had better names
>> than v3, but they were still somewhat confusing to me.
>>
>
> Now if only you could tell me what names were better to understand. ;)
> I'll reply to the individual patch remarks and hopefully there we find
> good names for these functions.
>
> Thanks,
> Stefan

I'll try to read it again and see if I think of anything.

Thanks,
Jake

Re: [BUG] submodule config does not apply to upper case submodules?

[Jeff King]

On Wed, Feb 15, 2017 at 03:37:37PM -0800, Junio C Hamano wrote:

> Stefan Beller  writes:
> 
> > Yes; though I'd place it in strbuf.{c,h} as it is operating
> > on the internals of the strbuf. (Do we make any promises outside of
> > strbuf about the internals? I mean we use .buf all the time, so maybe
> > I am overly cautious here)
> 
> I'd rather have it not use struct strbuf as an interface.  It only
> needs to pass "char *" and its promise that it touches the string
> in-place without changing the length need to be documented as a
> comment before the function.

This code also uses the hacky strbuf_split() interface. It would be nice
to one day move off of it (the only other strbuf-specific function used
there is strbuf_trim).

One _could_ actually parse the whole thing left-to-right (soaking up
whitespace and doing the canonicalizing) instead of dealing with a split
function at all. But the canonicalize bit you added here would not be
reusable then. And it's probably not worth holding up the bugfix here.

> >> +static void canonicalize_config_variable_name(struct strbuf *var)
> >> +{
> >> +   char *first_dot = strchr(var->buf, '.');
> >> +   char *last_dot = strrchr(var->buf, '.');
> >
> > If first_dot != NULL, then last_dot !+ NULL as well.
> > (either both are NULL or none of them),
> > so we can loose one condition below.
> 
> I do not think it is worth it, though.

If you really want to be picky, you do not need to find the first dot
at all. You can downcase everything until you see a dot, and then
find the last dot (if any) from there.

I don't think it matters much in practice.

-Peff

What's cooking in git.git (Feb 2017, #05; Thu, 16)

[Junio C Hamano]

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

I'd like to get pull requests for gitk and git-gui updates soonish,
if we are to have one during this cycle.

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

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

--
[Graduated to "master"]

* cw/completion (2017-02-03) 7 commits
  (merged to 'next' on 2017-02-10 at b3a5cbf39c)
 + completion: recognize more long-options
 + completion: teach remote subcommands to complete options
 + completion: teach replace to complete options
 + completion: teach ls-remote to complete options
 + completion: improve bash completion for git-add
 + completion: add subcommand completion for rerere
 + completion: teach submodule subcommands to complete options

 More command line completion (in contrib/) for recent additions.


* dp/submodule-doc-markup-fix (2017-02-16) 1 commit
  (merged to 'next' on 2017-02-16 at 698cdcff0a)
 + config.txt: fix formatting of submodule.alternateErrorStrategy section

 Doc fix.


* jk/doc-remote-helpers-markup-fix (2017-02-13) 1 commit
  (merged to 'next' on 2017-02-14 at 95af1267c7)
 + docs/gitremote-helpers: fix unbalanced quotes

 Doc markup fix.


* jk/doc-submodule-markup-fix (2017-02-13) 1 commit
  (merged to 'next' on 2017-02-14 at b2f807f7d8)
 + docs/git-submodule: fix unbalanced quote

 Doc markup fix.


* jk/reset-to-break-a-commit-doc (2017-02-16) 1 commit
  (merged to 'next' on 2017-02-16 at 3203e91525)
 + Revert "reset: add an example of how to split a commit into two"
 (this branch is used by jk/reset-to-break-a-commit-doc-updated.)

 Doc update.


* jk/reset-to-break-a-commit-doc-updated (2017-02-16) 1 commit
  (merged to 'next' on 2017-02-16 at bf729e9e25)
 + reset: add an example of how to split a commit into two
 (this branch uses jk/reset-to-break-a-commit-doc.)

 Doc update.


* jk/tempfile-ferror-fclose-confusion (2017-02-16) 1 commit
  (merged to 'next' on 2017-02-16 at 86cc4e77d7)
 + tempfile: avoid "ferror | fclose" trick

 Code clean-up.


* js/mingw-isatty (2017-02-14) 1 commit
  (merged to 'next' on 2017-02-15 at f3d3ccc978)
 + mingw: make stderr unbuffered again

 A hotfix for a topic already in 'master'.


* ls/p4-path-encoding (2017-02-10) 1 commit
  (merged to 'next' on 2017-02-15 at 73af90bf0f)
 + git-p4: fix git-p4.pathEncoding for removed files

 When "git p4" imports changelist that removes paths, it failed to
 convert pathnames when the p4 used encoding different from the one
 used on the Git side.  This has been corrected.


* rs/cocci-check-free-only-null (2017-02-11) 1 commit
  (merged to 'next' on 2017-02-15 at a628ee7142)
 + cocci: detect useless free(3) calls

 A new coccinelle rule that catches a check of !pointer before the
 pointer is free(3)d, which most likely is a bug.


* rs/ls-files-partial-optim (2017-02-13) 2 commits
  (merged to 'next' on 2017-02-15 at 7a21b55424)
 + ls-files: move only kept cache entries in prune_cache()
 + ls-files: pass prefix length explicitly to prune_cache()

 "ls-files" run with pathspec has been micro-optimized to avoid
 having to memmove(3) unnecessary bytes.


* rs/strbuf-cleanup-in-rmdir-recursively (2017-02-13) 1 commit
  (merged to 'next' on 2017-02-15 at 920f58b3b1)
 + rm: reuse strbuf for all remove_dir_recursively() calls, again

 Code clean-up.


* rs/swap (2017-01-30) 5 commits
  (merged to 'next' on 2017-02-10 at 5253797d0a)
 + graph: use SWAP macro
 + diff: use SWAP macro
 + use SWAP macro
 + apply: use SWAP macro
 + add SWAP macro

 Code clean-up.


* sb/doc-unify-bottom (2017-02-09) 1 commit
  (merged to 'next' on 2017-02-10 at 7229c4c1f7)
 + Documentation: unify bottom "part of git suite" lines

 Doc clean-up.


* sb/push-options-via-transport (2017-02-08) 1 commit
  (merged to 'next' on 2017-02-10 at 3e2d08e1fa)
 + push options: pass push options to the transport helper

 The push-options given via the "--push-options" option were not
 passed through to external remote helpers such as "smart HTTP" that
 are invoked via the transport helper.


* sb/submodule-doc (2017-01-12) 2 commits
  (merged to 'next' on 2017-02-10 at 5bfad5f30e)
 + submodule update documentation: don't repeat ourselves
 + submodule documentation: add options to the subcommand

 Doc updates.


* tg/stash-doc-cleanup (2017-02-13) 1 commit
  (merged to 'next' on 2017-02-14 at 5b9ffbc741)
 + Documentation/stash: remove mention of git reset --hard
 (this branch is used by tg/stash-push.)

 The documentation explained what "git stash" does to the working
 tree (after stashing away the local changes) in terms of "reset
 --hard", which was exposing an unnecessary implementation detail.

Re: There are too many unreachable loose objects

[Jeff King]

On Thu, Feb 16, 2017 at 02:36:10PM -0800, Jacob Keller wrote:

> > Whenever I run "git push --force(-with-lease)" I get a variation of
> >
> > Counting objects: 187, done.
> > Delta compression using up to 12 threads.
> > Compressing objects: 100% (126/126), done.
> > Writing objects: 100% (187/187), 21.35 KiB | 0 bytes/s, done.
> > Total 187 (delta 78), reused 71 (delta 20)
> > warning: There are too many unreachable loose objects; run 'git prune'
> > to remove them.
> > To g...@git.company.com:project.git
> >  + 51338ea...b0ebe39 my-branch -> my-branch (forced update)
> >
> > So I'll run "git prune" and, for good measure, "git gc" (which even
> > includes "git prune"?). The first seems to do nothing, the latter does
> > its thing.
> >
> 
> It may be that it's the server side that needs to git-prune, and not
> your local side? I'm not really certain but you're doing a push which
> talks to a remote server.

Yes, certainly the position in the output implies that. These days you
should see:

  remote: warning: There are too many...

to make it more clear. Perhaps the server is too old to have 860a2ebec
(receive-pack: send auto-gc output over sideband 2, 2016-06-05).

-Peff

Re: [RFCv4 PATCH 00/14] Checkout aware of Submodules!

[Stefan Beller]

On Thu, Feb 16, 2017 at 2:54 PM, Jacob Keller  wrote:
> On Thu, Feb 16, 2017 at 2:00 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> Integrate updating the submodules into git checkout,...
>>
>> It was more or less a pleasant read, once I decided to pretend that
>> I were a machine who uses identifiers only to identify locations in
>> the program ;-) IOW, for human consumption, the new names introduced
>> were sometimes quite confusing and went against helping understanding.
>>
>
> Based on my cursory reading, I agree. I had trouble understanding how
> the process worked because the names were somewhat confusing. They
> started to make some sense as I read more. I think v4 had better names
> than v3, but they were still somewhat confusing to me.
>

Now if only you could tell me what names were better to understand. ;)
I'll reply to the individual patch remarks and hopefully there we find
good names for these functions.

Thanks,
Stefan

Re: [PATCH v2 00/16] Remove submodule from files-backend.c

[Stefan Beller]

This series looks good to me.

Thanks,
Stefan

Re: [RFCv4 PATCH 00/14] Checkout aware of Submodules!

[Jacob Keller]

On Thu, Feb 16, 2017 at 2:00 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Integrate updating the submodules into git checkout,...
>
> It was more or less a pleasant read, once I decided to pretend that
> I were a machine who uses identifiers only to identify locations in
> the program ;-) IOW, for human consumption, the new names introduced
> were sometimes quite confusing and went against helping understanding.
>

Based on my cursory reading, I agree. I had trouble understanding how
the process worked because the names were somewhat confusing. They
started to make some sense as I read more. I think v4 had better names
than v3, but they were still somewhat confusing to me.

Regards,
Jake

Re: There are too many unreachable loose objects

[Jacob Keller]

On Thu, Feb 16, 2017 at 1:58 PM, Hilco Wijbenga
 wrote:
> Hi all,
>
> Whenever I run "git push --force(-with-lease)" I get a variation of
>
> Counting objects: 187, done.
> Delta compression using up to 12 threads.
> Compressing objects: 100% (126/126), done.
> Writing objects: 100% (187/187), 21.35 KiB | 0 bytes/s, done.
> Total 187 (delta 78), reused 71 (delta 20)
> warning: There are too many unreachable loose objects; run 'git prune'
> to remove them.
> To g...@git.company.com:project.git
>  + 51338ea...b0ebe39 my-branch -> my-branch (forced update)
>
> So I'll run "git prune" and, for good measure, "git gc" (which even
> includes "git prune"?). The first seems to do nothing, the latter does
> its thing.
>

It may be that it's the server side that needs to git-prune, and not
your local side? I'm not really certain but you're doing a push which
talks to a remote server.

Thanks,
Jake

> And then the next time (which could be a few minutes later) I get the
> same warning. My branches aren't that big, the largest ever had 40-ish
> commits. So abandoning a few dozen commits should not lead to this
> warning, I would think.
>
> What am I doing wrong?
>
> Cheers,
> Hilco

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

[Jacob Keller]

On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller  wrote:
> +int touch_submodules_in_worktree(void)
> +{
> +   /*
> +* Update can't be "none", "merge" or "rebase",
> +* treat any value as OFF, except an explicit ON.
> +*/
> +   return config_update_recurse_submodules == RECURSE_SUBMODULES_ON;
> +}
> +

This function doesn't and the comment don't make sense to me. What do
you mean update can't be "none", "merge", or "rebase"? I'm thinking
this means that the update_recurse_submodules checks whether it's ok
for doing recursive update on submodules but only when the update type
is checkout? This appears to be connected directly to the previous
patch that reads the config value somehow. This is pretty convoluted
to me, and took me quite a while to understand. Is it possible to make
this more clear in the comments or in the name?

Re: [RFCv4 PATCH 00/14] Checkout aware of Submodules!

[Junio C Hamano]

Stefan Beller  writes:

> Integrate updating the submodules into git checkout,...

It was more or less a pleasant read, once I decided to pretend that
I were a machine who uses identifiers only to identify locations in
the program ;-) IOW, for human consumption, the new names introduced
were sometimes quite confusing and went against helping understanding.

I saw a few places where logic looked somewhat iffy, which I sent
separate comments on; I may spot more if the code used more
understandable names and calling conventions, but that is OK.  It is
an expected part of an iterative process.  

I can feel that this topic is getting closer to where we eventually
want to go.

Thanks.

There are too many unreachable loose objects

[Hilco Wijbenga]

Hi all,

Whenever I run "git push --force(-with-lease)" I get a variation of

Counting objects: 187, done.
Delta compression using up to 12 threads.
Compressing objects: 100% (126/126), done.
Writing objects: 100% (187/187), 21.35 KiB | 0 bytes/s, done.
Total 187 (delta 78), reused 71 (delta 20)
warning: There are too many unreachable loose objects; run 'git prune'
to remove them.
To g...@git.company.com:project.git
 + 51338ea...b0ebe39 my-branch -> my-branch (forced update)

So I'll run "git prune" and, for good measure, "git gc" (which even
includes "git prune"?). The first seems to do nothing, the latter does
its thing.

And then the next time (which could be a few minutes later) I get the
same warning. My branches aren't that big, the largest ever had 40-ish
commits. So abandoning a few dozen commits should not lead to this
warning, I would think.

What am I doing wrong?

Cheers,
Hilco

Kindly Call +226 65974887

[kabou...@ono.com]

Dear Friend,

My name is Dr.Kabo Uago, I have an inheritance fund for you, contact 
me  for more fund details.



Regards,

Dr.Kabo Uago

Re: [PATCH 10/15] update submodules: add submodule_go_from_to

[Stefan Beller]

On Thu, Feb 16, 2017 at 1:15 PM, Junio C Hamano  wrote:
> [administrivia: I've been seeing "unlisted-recipients:; (no To-header on 
> input)"
> for all of your recent patches.  Can it be corrected on your end, please?]

I cc'd everyone and had no to field, actually. Maybe git-send-email should
complain more loudly when I am holding it wrong.

>> + cp.git_cmd = 1;
>> + argv_array_pushl(, "diff-index", "--cached", "HEAD", NULL);
>
> We'd want to use the QUICK optimization here, I suspect.  This
> caller does not need to (or want to) learn which exact paths are
> modified, right?

ok

>> + if (run_command())
>> + die("could not clean submodule index");
>> +}
>
> Do s/clean/reset/ everywhere above.

ok

>
>> +/**
>> + * Moves a submodule at a given path from a given head to another new head.
>> + * For edge cases (a submodule coming into existence or removing a 
>> submodule)
>> + * pass NULL for old or new respectively.
>> + *
>> + * TODO: move dryrun and forced to flags.
>
> The reason why this seeingly trivial thing is left as TODO is...???

will do. The reason was to first get the grand design right before
spending more time in the details.

>
>> + */
>> +int submodule_go_from_to(const char *path,
>> +  const char *old,
>> +  const char *new,
>> +  int dry_run,
>> +  int force)
>> +{
>
> go-from-to does not tell me what it does, but my cursory read of the
> body of the function tells me that this is doing a checkout of a
> branch in the submodule?  The operation in builtin/checkout.c that
> conceptually correspond to this is called switch_branches(), I
> think, so perhaps submodule_switch_branches() is a better name?

Well as of now all submodule operations (submodule update mostly)
end up with detached HEADs in the submodule. So it is rather
going from one state (sha1) to another given sha1.

I would rather compare it to checkout_entry/write_entry in entry.c
except that there are more things to go wrong. A single file has no
notion of its own index or dirtiness.

>
>> + int ret = 0;
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + const struct submodule *sub;
>> +
>> + sub = submodule_from_path(null_sha1, path);
>> +
>> + if (!sub)
>> + die("BUG: could not get submodule information for '%s'", path);
>> +
>> + if (!dry_run) {
>> + if (old) {
>> + if (!submodule_uses_gitfile(path))
>> + absorb_git_dir_into_superproject("", path,
>> + ABSORB_GITDIR_RECURSE_SUBMODULES);
>> + } else {
>> + struct strbuf sb = STRBUF_INIT;
>> + strbuf_addf(, "%s/modules/%s",
>> + get_git_common_dir(), sub->name);
>> + connect_work_tree_and_git_dir(path, sb.buf);
>> + strbuf_release();
>> +
>> + /* make sure the index is clean as well */
>> + submodule_clean_index(path);
>> + }
>> + }
>> +
>> + if (old && !force) {
>> + /* Check if the submodule has a dirty index. */
>> + if (submodule_has_dirty_index(sub)) {
>> + /* print a thing here? */
>> + return -1;
>> + }
>
> Isn't it too late to do this here?  You already reset the index
> in the submodule, no?

Yes this is confusing.
We run this function first as a dry_run, and in a second pass
as a real run. So the order inside the function is confusing
as we would run this first in the dry run.

>
> Is the idea that changes that are only in the submodule's working
> tree are noticed by later "read-tree -u -m" down there?  Not
> complaining but trying to understand.

I think (as a first step) we only want to allow a clean index in
submodules, as then we have to implement less cases at first.

Re: [PATCH 13/15] read-cache: remove_marked_cache_entries to wipe selected submodules.

[Junio C Hamano]

Stefan Beller  writes:

The title was ungrokkable to me, but after reading the code I think
you are teaching the normal codepath where removal of working tree
is done to match what is done to the index that submodules are also
in the working tree and need to be removed when the corresopnding ce
is removed.

Which makes sense.

> Signed-off-by: Stefan Beller 
> ---
>  read-cache.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index 9054369dd0..b78a7f02e3 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -18,6 +18,7 @@
>  #include "varint.h"
>  #include "split-index.h"
>  #include "utf8.h"
> +#include "submodule.h"
>  
>  /* Mask for the name length in ce_flags in the on-disk index */
>  
> @@ -532,6 +533,8 @@ void remove_marked_cache_entries(struct index_state 
> *istate)
>  
>   for (i = j = 0; i < istate->cache_nr; i++) {
>   if (ce_array[i]->ce_flags & CE_REMOVE) {
> + if (is_active_submodule_with_strategy(ce_array[i], 
> SM_UPDATE_UNSPECIFIED))
> + submodule_go_from_to(ce_array[i]->name, "HEAD", 
> NULL, 0, 1);
>   remove_name_hash(istate, ce_array[i]);
>   save_or_free_index_entry(istate, ce_array[i]);
>   }

[PATCH] tempfile: avoid "ferror | fclose" trick

[Jeff King]

On Thu, Feb 16, 2017 at 11:43:59AM -0500, Jeff King wrote:

> On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:
> 
> > >>  int xfclose(FILE *fp)
> > >>  {
> > >>  return ferror(fp) | fclose(fp);
> > >>  }
> > >
> > > Yes, that's exactly what I had in mind (might be worth calling out the
> > > bitwise-OR, though, just to make it clear it's not a typo).
> > 
> > Since the order of evaluation is unspecified, it would be better to
> > force sequencing ferror before fclose.
> 
> Good point. Arguably the call in tempfile.c is buggy.

Here's a fix.

I think close_tempfile() suffers from the same errno problem discussed
earlier in this thread (i.e., that after calling it, you may get an
error return with a random, unrelated errno value if ferror() failed but
fclose() did not).

-- >8 --
Subject: [PATCH] tempfile: avoid "ferror | fclose" trick

The current code wants to record an error condition from
either ferror() or fclose(), but makes sure that we always
call both functions. So it can't use logical-OR "||", which
would short-circuit when ferror() is true. Instead, it uses
bitwise-OR "|" to evaluate both functions and set one or
more bits in the "err" flag if they reported a failure.

Unlike logical-OR, though, bitwise-OR does not introduce a
sequence point, and the order of evaluation for its operands
is unspecified. So a compiler would be free to generate code
which calls fclose() first, and then ferror() on the
now-freed filehandle.

There's no indication that this has happened in practice,
but let's write it out in a way that follows the standard.

Noticed-by: Andreas Schwab 
Signed-off-by: Jeff King 
---
 tempfile.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index 2990c9242..ffcc27237 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -247,12 +247,8 @@ int close_tempfile(struct tempfile *tempfile)
tempfile->fd = -1;
if (fp) {
tempfile->fp = NULL;
-
-   /*
-* Note: no short-circuiting here; we want to fclose()
-* in any case!
-*/
-   err = ferror(fp) | fclose(fp);
+   err = ferror(fp);
+   err |= fclose(fp);
} else {
err = close(fd);
}
-- 
2.12.0.rc1.559.gd292418bf

Re: [PATCH v3] reset: add an example of how to split a commit into two

[Jacob Keller]

On Thu, Feb 16, 2017 at 11:49 AM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Jacob Keller  writes:
>>
>>> The interdiff between v2 and v3 is not really worth showing since I
>>> basically re-wrote the entire section a bit.
>>
>> Could this be made into an incremental, now that v2 has been in
>> 'next' for about 10 days, please?
>
> Nah, I think it is easier to read "log -p" if I just revert v2 out
> of existence from 'next', and queue this (with a minor typofixes) as
> a different topic to be merged later to 'master'.
>

Ok. Yea, I didn't even realize it was put into next because of the
various comments I'd received. I guess I could have checked, but the
diff really is bad when seeing incrementally.

> So no need to resend and certainly no need to make it incremental.
>
>>> +Split a commit apart into a sequence of commits::
>>> ++
>>> +Suppose that you have create lots of logically separate changes and commit 
>>> them
>>
>> s/create//; s/commit//
>
> I'd do this myself while queuing.
>
> Thanks.

Thanks,
Jake

Re: [PATCH 05/15] connect_work_tree_and_git_dir: safely create leading directories

[Junio C Hamano]

Stefan Beller  writes:

>> The above does somewhat more than advertised and was a bit hard to
>> grok.  Initially I thought the reason why pathdup()s were delayed
>> was perhaps because you pathdup() something potentially different
>> from the given parameter to the function (i.e. new code before
>> pathdup() may tweak what is pathdup()ed).
>>
>> But that is not what is happening.  I suspect that you did this to
>> avoid leaking allocated memory when the code calls die().
>
> That is not what is happening, either.

That's a good sign that you need a bit more in the log message.

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

[Junio C Hamano]

Stefan Beller  writes:

> +
> + /* ERROR_WOULD_LOSE_UNTRACKED_SUBMODULE */
> + "Submodule '%s' cannot be deleted as it contains untracked files.",

OK.

> + msgs[ERROR_WOULD_LOSE_UNTRACKED_SUBMODULE] =
> + _("Submodule '%s' cannot be deleted as it contains untracked 
> files.");

OK again.

> @@ -240,12 +246,44 @@ static void display_error_msgs(struct 
> unpack_trees_options *o)
>   fprintf(stderr, _("Aborting\n"));
>  }
>  
> +static int submodule_check_from_to(const struct cache_entry *ce, const char 
> *old_id, const char *new_id, struct unpack_trees_options *o)
> +{
> + if (submodule_go_from_to(ce->name, old_id,
> +  new_id, 1, o->reset))
> + return o->gently ? -1 :
> + add_rejected_path(o, 
> ERROR_WOULD_LOSE_UNTRACKED_SUBMODULE, ce->name);

Is potential loss of untracked paths the only reason
submodule_go_from_to() would fail?  I somehow thought that it would
not even care about untracked paths but cared deeply about already
added changes.

Re: [PATCH 11/15] unpack-trees: pass old oid to verify_clean_submodule

[Junio C Hamano]

Stefan Beller  writes:

> The check (which uses the old oid) is yet to be implemented, but this part
> is just a refactor, so it can go separately first.

If this didn't pass an unused parameter, then the change is just a
refactor, and I do think such a "just a refactor" can be a good step
on its own to keep the future step to manageable complexity.

With an unused parameter being passed, I do not think it is a good
logical single step anymore, though.

> Signed-off-by: Stefan Beller 
> ---
>  unpack-trees.c | 17 ++---
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 3a8ee19fe8..616a0ae4b2 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1407,7 +1407,8 @@ static void invalidate_ce_path(const struct cache_entry 
> *ce,
>   * Currently, git does not checkout subprojects during a superproject
>   * checkout, so it is not going to overwrite anything.
>   */
> -static int verify_clean_submodule(const struct cache_entry *ce,
> +static int verify_clean_submodule(const char *old_sha1,
> +   const struct cache_entry *ce,
> enum unpack_trees_error_types error_type,
> struct unpack_trees_options *o)
>  {
> @@ -1427,16 +1428,18 @@ static int verify_clean_subdirectory(const struct 
> cache_entry *ce,
>   struct dir_struct d;
>   char *pathbuf;
>   int cnt = 0;
> - unsigned char sha1[20];
>  
> - if (S_ISGITLINK(ce->ce_mode) &&
> - resolve_gitlink_ref(ce->name, "HEAD", sha1) == 0) {
> - /* If we are not going to update the submodule, then
> + if (S_ISGITLINK(ce->ce_mode)) {
> + unsigned char sha1[20];
> + int sub_head = resolve_gitlink_ref(ce->name, "HEAD", sha1);
> + /*
> +  * If we are not going to update the submodule, then
>* we don't care.
>*/
> - if (!hashcmp(sha1, ce->oid.hash))
> + if (!sub_head && !hashcmp(sha1, ce->oid.hash))
>   return 0;
> - return verify_clean_submodule(ce, error_type, o);
> + return verify_clean_submodule(sub_head ? NULL : 
> sha1_to_hex(sha1),
> +   ce, error_type, o);
>   }
>  
>   /*

Re: [PATCH 05/15] connect_work_tree_and_git_dir: safely create leading directories

[Stefan Beller]

On Thu, Feb 16, 2017 at 12:54 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> In a later patch we'll use connect_work_tree_and_git_dir when the
>> directory for the gitlink file doesn't exist yet. Safely create
>> the directory first.
>>
>> One of the two users of 'connect_work_tree_and_git_dir' already checked
>> for the directory being there, so we can loose that check.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>  dir.c   | 32 +---
>>  submodule.c | 11 ++-
>>  2 files changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 4541f9e146..6f52af7abb 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -2728,23 +2728,33 @@ void untracked_cache_add_to_index(struct index_state 
>> *istate,
>>  /* Update gitfile and core.worktree setting to connect work tree and git 
>> dir */
>>  void connect_work_tree_and_git_dir(const char *work_tree_, const char 
>> *git_dir_)
>>  {
>> - struct strbuf file_name = STRBUF_INIT;
>> + struct strbuf gitfile_sb = STRBUF_INIT;
>> + struct strbuf cfg_sb = STRBUF_INIT;
>>   struct strbuf rel_path = STRBUF_INIT;
>> - char *git_dir = real_pathdup(git_dir_);
>> - char *work_tree = real_pathdup(work_tree_);
>> + char *git_dir, *work_tree;
>>
>> - /* Update gitfile */
>> - strbuf_addf(_name, "%s/.git", work_tree);
>> - write_file(file_name.buf, "gitdir: %s",
>> -relative_path(git_dir, work_tree, _path));
>> + /* Prepare .git file */
>> + strbuf_addf(_sb, "%s/.git", work_tree_);
>> + if (safe_create_leading_directories_const(gitfile_sb.buf))
>> + die(_("could not create directories for %s"), gitfile_sb.buf);
>> +
>> + /* Prepare config file */
>> + strbuf_addf(_sb, "%s/config", git_dir_);
>> + if (safe_create_leading_directories_const(cfg_sb.buf))
>> + die(_("could not create directories for %s"), cfg_sb.buf);
>>
>> + git_dir = real_pathdup(git_dir_);
>> + work_tree = real_pathdup(work_tree_);
>> +
>> + /* Write .git file */
>> + write_file(gitfile_sb.buf, "gitdir: %s",
>> +relative_path(git_dir, work_tree, _path));
>
> The above does somewhat more than advertised and was a bit hard to
> grok.  Initially I thought the reason why pathdup()s were delayed
> was perhaps because you pathdup() something potentially different
> from the given parameter to the function (i.e. new code before
> pathdup() may tweak what is pathdup()ed).
>
> But that is not what is happening.  I suspect that you did this to
> avoid leaking allocated memory when the code calls die().

That is not what is happening, either.
AFAICT real_pathdup resolves a path to its real path. But the path
*must* exist already (except the very last part, i.e. the file name).
So the SCLD must come before the real_pathdup, which has to come
before its consumers, which are the relative_path calls for write_file
as well  as git_config_set_in_file.

So structurally we need to:

1a) construct $GIT_DIR/modules//config
1b) SCLD 1a)
1c) get absolute path of 1a)

2a) construct /.git file
2b) SCLD 2a)
2c) get absolute path of 2a)

3) compute relative path of 1c and 2c
both ways, write to appropriate places.

I chose to structure it as:

1a
1b
2a
2b

1c
2c
3

because the a/b steps seemed very related and the order is valid.


>
> If the code was written like so from the beginning, I do not see a
> reason to move the pathdup() up to deliberately make it leak [*1*].
> But as a part of this change, I found it distracting and getting in
> the way of understanding the change.  If you really care, it is
> nicer to do it to reviewers as a separate preparatory clean-up step,
> or follow-up standalone clean-up patch after the series settles.

I considered doing it in multiple patches as we have different things
going on here:
1) as the commit claims have SCLD in connect_work_tree_and_git_dir
2) keep the dependencies of SCLD and real_pathdup in order.  C.f.
  the deleted lines of code in submodule.c

> It is a good thing to do to make sure git_dir_ exists and it should
> be mentioned in the log message.  Adding "Do the same for the place
> where the per-repo config file is created". at the end of the first
> paragraph should be sufficient.
>

will do.

Thanks for thorough review!
Stefan

Re: [PATCH 10/15] update submodules: add submodule_go_from_to

[Junio C Hamano]

Stefan Beller  writes:

[administrivia: I've been seeing "unlisted-recipients:; (no To-header on input)"
for all of your recent patches.  Can it be corrected on your end, please?]

> In later patches we introduce the options and flag for commands
> that modify the working directory, e.g. git-checkout.
>
> This piece of code will be used universally for
> all these working tree modifications as it
> * supports dry run to answer the question:
>   "Is it safe to change the submodule to this new state?"
>   e.g. is it overwriting untracked files or are there local
>   changes that would be overwritten?
> * supports a force flag that can be used for resetting
>   the tree.
>
> Signed-off-by: Stefan Beller 
> ---
>  submodule.c | 151 
> 
>  submodule.h |   5 ++
>  2 files changed, 156 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index b262c5b0ad..84cc62f3bb 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1250,6 +1250,157 @@ int bad_to_remove_submodule(const char *path, 
> unsigned flags)
>   return ret;
>  }
>  
> +static int submodule_has_dirty_index(const struct submodule *sub)
> +{
> + ssize_t len;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf buf = STRBUF_INIT;
> + int ret = 0;
> +
> + prepare_submodule_repo_env_no_git_dir(_array);
> +
> + cp.git_cmd = 1;
> + argv_array_pushl(, "diff-index", "--cached", "HEAD", NULL);

We'd want to use the QUICK optimization here, I suspect.  This
caller does not need to (or want to) learn which exact paths are
modified, right?

> +void submodule_clean_index(const char *path)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + prepare_submodule_repo_env_no_git_dir(_array);
> +
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> +
> + argv_array_pushf(, "--super-prefix=%s/", path);
> + argv_array_pushl(, "read-tree", "-u", "--reset", NULL);
> +
> + argv_array_push(, EMPTY_TREE_SHA1_HEX);
> +
> + if (run_command())
> + die("could not clean submodule index");
> +}

Do s/clean/reset/ everywhere above.

> +/**
> + * Moves a submodule at a given path from a given head to another new head.
> + * For edge cases (a submodule coming into existence or removing a submodule)
> + * pass NULL for old or new respectively.
> + *
> + * TODO: move dryrun and forced to flags.

The reason why this seeingly trivial thing is left as TODO is...???

> + */
> +int submodule_go_from_to(const char *path,
> +  const char *old,
> +  const char *new,
> +  int dry_run,
> +  int force)
> +{

go-from-to does not tell me what it does, but my cursory read of the
body of the function tells me that this is doing a checkout of a
branch in the submodule?  The operation in builtin/checkout.c that
conceptually correspond to this is called switch_branches(), I
think, so perhaps submodule_switch_branches() is a better name?

> + int ret = 0;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + const struct submodule *sub;
> +
> + sub = submodule_from_path(null_sha1, path);
> +
> + if (!sub)
> + die("BUG: could not get submodule information for '%s'", path);
> +
> + if (!dry_run) {
> + if (old) {
> + if (!submodule_uses_gitfile(path))
> + absorb_git_dir_into_superproject("", path,
> + ABSORB_GITDIR_RECURSE_SUBMODULES);
> + } else {
> + struct strbuf sb = STRBUF_INIT;
> + strbuf_addf(, "%s/modules/%s",
> + get_git_common_dir(), sub->name);
> + connect_work_tree_and_git_dir(path, sb.buf);
> + strbuf_release();
> +
> + /* make sure the index is clean as well */
> + submodule_clean_index(path);
> + }
> + }
> +
> + if (old && !force) {
> + /* Check if the submodule has a dirty index. */
> + if (submodule_has_dirty_index(sub)) {
> + /* print a thing here? */
> + return -1;
> + }

Isn't it too late to do this here?  You already reset the index
in the submodule, no?

Is the idea that changes that are only in the submodule's working
tree are noticed by later "read-tree -u -m" down there?  Not
complaining but trying to understand.

> + }
> +
> + prepare_submodule_repo_env_no_git_dir(_array);
> +
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> +
> + argv_array_pushf(, "--super-prefix=%s/", path);
> + argv_array_pushl(, "read-tree", NULL);
> +
> + if (dry_run)
> + argv_array_push(, "-n");
> + else
> + argv_array_push(, "-u");
> +
> + if (force)
> + 

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

[Junio C Hamano]

Stefan Beller  writes:

> In later patches we introduce the --recurse-submodule flag for commands
> that modify the working directory, e.g. git-checkout.
>
> It is potentially expensive to check if a submodule needs an update,
> because a common theme to interact with submodules is to spawn a child
> process for each interaction.
>
> So let's introduce a function that checks if a submodule needs
> to be checked for an update before attempting the update.
>
> Signed-off-by: Stefan Beller 
> ---
>  submodule.c | 27 +++
>  submodule.h | 13 +
>  2 files changed, 40 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index 591f4a694e..2a37e03420 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -548,6 +548,33 @@ void set_config_update_recurse_submodules(int value)
>   config_update_recurse_submodules = value;
>  }
>  
> +int touch_submodules_in_worktree(void)
> +{
> + /*
> +  * Update can't be "none", "merge" or "rebase",
> +  * treat any value as OFF, except an explicit ON.
> +  */
> + return config_update_recurse_submodules == RECURSE_SUBMODULES_ON;
> +}

I somehow sense a somewhat misnamed function.

> +int is_active_submodule_with_strategy(const struct cache_entry *ce,
> +   enum submodule_update_type strategy)
> +{
> + const struct submodule *sub;
> +
> + if (!S_ISGITLINK(ce->ce_mode))
> + return 0;
> +
> + if (!touch_submodules_in_worktree())
> + return 0;

Reading this caller alone, it is totally unclear what this !touch is
about.  "We try to touch it by calling this function, and if the
function successfullly touches it, we return without doing anything
else?"

Would it help to avoid confusion, if the helper function is named to
be clearly a boolean?  should_update_submodules_in_worktree() or
something along those lines?

> + sub = submodule_from_path(null_sha1, ce->name);
> + if (!sub)
> + return 0;
> +
> + return sub->update_strategy.type == strategy;
> +}

I am not sure if this is a good API design; if it were "static int"
contained inside the module I wouldn't care, but wouldn't it be more
natural for the caller of this function to say

if (get_submodule_update_strategy(ce) == STRATEGY_I_WANT)
do something;
else
do something else;

rather than forced to say:

if (is_active_submodule_with_strategy(ce, STRATEGY_I_WANT))
do something;
else
do something else;

no?  The caller can easily be extended to

switch (get_submodule_update_strategy(ce)) {
case STRATEGY_I_WANT:
case STRATEGY_I_TOLERATE:
do something; 
break;
default:
do something else;
break;
}

if the function does not insist taking a single allowed strategy and
return just yes/no as its answer.

Re: Back quote typo in error messages (?)

[Fabrizio Cucci]

On 16 February 2017 at 19:02, Jeff King  wrote:
> Try the "Quoted Text" section of the original asciidoc user manual:
>
>   http://www.methods.co.nz/asciidoc/userguide.html#X51
>
> Asciidoctor has introduced some new syntax (almost certainly because the
> original asymmetric formulations have a bunch of ambiguous corner
> cases). By default, it disables the asymmetric versions, but they work
> in "compat" mode (and the newer ones do not).

I can't say I had the pleasure of using Asciidoctor 0.1.4 or earlier! :)

> Git's documentation is all written for the original asciidoc. If you
> build it with asciidoctor, it must be done in compat mode. This is the
> default when asciidoctor sees a two-line (i.e., underlined) section
> title, which all of our manpages have.

And I definitely didn't know that, but now I'm glad we went OT! :)
Thanks a lot for the clarifications.

Re: [PATCH 05/15] connect_work_tree_and_git_dir: safely create leading directories

[Junio C Hamano]

Stefan Beller  writes:

> In a later patch we'll use connect_work_tree_and_git_dir when the
> directory for the gitlink file doesn't exist yet. Safely create
> the directory first.
>
> One of the two users of 'connect_work_tree_and_git_dir' already checked
> for the directory being there, so we can loose that check.
>
> Signed-off-by: Stefan Beller 
> ---
>  dir.c   | 32 +---
>  submodule.c | 11 ++-
>  2 files changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 4541f9e146..6f52af7abb 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2728,23 +2728,33 @@ void untracked_cache_add_to_index(struct index_state 
> *istate,
>  /* Update gitfile and core.worktree setting to connect work tree and git dir 
> */
>  void connect_work_tree_and_git_dir(const char *work_tree_, const char 
> *git_dir_)
>  {
> - struct strbuf file_name = STRBUF_INIT;
> + struct strbuf gitfile_sb = STRBUF_INIT;
> + struct strbuf cfg_sb = STRBUF_INIT;
>   struct strbuf rel_path = STRBUF_INIT;
> - char *git_dir = real_pathdup(git_dir_);
> - char *work_tree = real_pathdup(work_tree_);
> + char *git_dir, *work_tree;
>  
> - /* Update gitfile */
> - strbuf_addf(_name, "%s/.git", work_tree);
> - write_file(file_name.buf, "gitdir: %s",
> -relative_path(git_dir, work_tree, _path));
> + /* Prepare .git file */
> + strbuf_addf(_sb, "%s/.git", work_tree_);
> + if (safe_create_leading_directories_const(gitfile_sb.buf))
> + die(_("could not create directories for %s"), gitfile_sb.buf);
> +
> + /* Prepare config file */
> + strbuf_addf(_sb, "%s/config", git_dir_);
> + if (safe_create_leading_directories_const(cfg_sb.buf))
> + die(_("could not create directories for %s"), cfg_sb.buf);
>  
> + git_dir = real_pathdup(git_dir_);
> + work_tree = real_pathdup(work_tree_);
> +
> + /* Write .git file */
> + write_file(gitfile_sb.buf, "gitdir: %s",
> +relative_path(git_dir, work_tree, _path));

The above does somewhat more than advertised and was a bit hard to
grok.  Initially I thought the reason why pathdup()s were delayed
was perhaps because you pathdup() something potentially different
from the given parameter to the function (i.e. new code before
pathdup() may tweak what is pathdup()ed).

But that is not what is happening.  I suspect that you did this to
avoid leaking allocated memory when the code calls die().

If the code was written like so from the beginning, I do not see a
reason to move the pathdup() up to deliberately make it leak [*1*].
But as a part of this change, I found it distracting and getting in
the way of understanding the change.  If you really care, it is
nicer to do it to reviewers as a separate preparatory clean-up step,
or follow-up standalone clean-up patch after the series settles.

The comment "prepare config file" was misleading; it is preparing
the place the config file will be created (same for "prepare .git
file").

It is a good thing to do to make sure git_dir_ exists and it should
be mentioned in the log message.  Adding "Do the same for the place
where the per-repo config file is created". at the end of the first
paragraph should be sufficient.

Thanks.


[Footnote]

*1* it is arguable a small piece of unfreed memory before exit() or
die() is worth called "leak", though.

Re: [PATCH 03/15] lib-submodule-update.sh: define tests for recursing into submodules

[Junio C Hamano]

Stefan Beller  writes:

> Currently lib-submodule-update.sh provides 2 functions
> test_submodule_switch and test_submodule_forced_switch that are used by a
> variety of tests to ensure that submodules behave as expected. The current
> expected behavior is that submodules are not touched at all (see
> 42639d2317a for the exact setup).
>
> In the future we want to teach all these commands to properly recurse
> into submodules. To do that, we'll add two testing functions to
> submodule-update-lib.sh test_submodule_switch_recursing and
> test_submodule_forced_switch_recursing.

I'd remove "properly" and insert a colon after "add two ... to
submodule-update-lib.sh" before the names of two functions.

> +reset_work_tree_to_interested () {
> + reset_work_tree_to $1 &&
> + # indicate we are interested in the submodule:
> + git -C submodule_update config submodule.sub1.url "bogus" &&
> + # also have it available:
> + if ! test -d submodule_update/.git/modules/sub1
> + then
> + mkdir submodule_update/.git/modules &&

Would we want "mkdir -p" here to be safe?

> + cp -r submodule_update_repo/.git/modules/sub1 
> submodule_update/.git/modules/sub1

... ahh, wouldn't matter that much, we checked that modules/sub1
does not exist, and as long as nobody creates modules/ or modules/somethingelse
we are OK.

> + fi
> +}
> +

> @@ -695,3 +736,443 @@ test_submodule_forced_switch () {
>   )
>   '
>  }
> +
> +# Test that submodule contents are correctly updated when switching
> +# between commits that change a submodule.
> +# Test that the following transitions are correctly handled:
> +# (These tests are also above in the case where we expect no change
> +#  in the submodule)
> +# - Updated submodule
> +# - New submodule
> +# - Removed submodule
> +# - Directory containing tracked files replaced by submodule
> +# - Submodule replaced by tracked files in directory
> +# - Submodule replaced by tracked file with the same name
> +# - tracked file replaced by submodule

These should work without trouble only when the paths involved in
the operation in the working tree are clean, right?  Just double
checking.  If they are dirty we should expect a failure, instead of
silent loss of information.

> +# New test cases
> +# - Removing a submodule with a git directory absorbs the submodules
> +#   git directory first into the superproject.
> +
> +test_submodule_switch_recursing () {
> + command="$1"

The dq-pair is not strictly needed on the RHS of the assignment, but
it is a good way to signal that we considered that we might receive
an argument with $IFS in it...

> + # Appearing submodule #
> + # Switching to a commit letting a submodule appear checks it out ...
> + test_expect_success "$command: added submodule is checked out" '
> + prolog &&
> + reset_work_tree_to_interested no_submodule &&
> + (
> + cd submodule_update &&
> + git branch -t add_sub1 origin/add_sub1 &&
> + $command add_sub1 &&

... and after doing so, not quoting $command here signals that we
expect command line splitting to happen.  Am I reading it correctly?
Without an actual caller appearing in this step, it is rather hard
to judge.

> + test_superproject_content origin/add_sub1 &&
> + test_submodule_content sub1 origin/add_sub1
> + )
> ...
> + # ... but an ignored file is fine.
> + test_expect_success "$command: added submodule removes an untracked 
> ignored file" '
> + test_when_finished "rm submodule_update/.git/info/exclude" &&
> + prolog &&
> + reset_work_tree_to_interested no_submodule &&
> + (
> + cd submodule_update &&
> + git branch -t add_sub1 origin/add_sub1 &&
> + : >sub1 &&
> + echo sub1 > .git/info/exclude

">.git/info/exclude"

> + $command add_sub1 &&
> + test_superproject_content origin/add_sub1 &&
> + test_submodule_content sub1 origin/add_sub1
> + )
> + '

Re: [PATCH v3] reset: add an example of how to split a commit into two

[Junio C Hamano]

Junio C Hamano  writes:

> Jacob Keller  writes:
>
>> The interdiff between v2 and v3 is not really worth showing since I
>> basically re-wrote the entire section a bit.
>
> Could this be made into an incremental, now that v2 has been in
> 'next' for about 10 days, please?

Nah, I think it is easier to read "log -p" if I just revert v2 out
of existence from 'next', and queue this (with a minor typofixes) as
a different topic to be merged later to 'master'.

So no need to resend and certainly no need to make it incremental.

>> +Split a commit apart into a sequence of commits::
>> ++
>> +Suppose that you have create lots of logically separate changes and commit 
>> them
>
> s/create//; s/commit//

I'd do this myself while queuing.

Thanks.

>
>> +together. Then, later you decide that it might be better to have each 
>> logical
>> +chunk associated with its own commit. ...

Re: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"

[Siddharth Kannan]

Hey Junio and Matthieu,

On 17 February 2017 at 00:19, Junio C Hamano  wrote:
> Matthieu Moy  writes:
>
>> Siddharth Kannan  writes:
>>
>>> This is as per our discussion[1]. The patches and commit messages are based 
>>> on
>>> Junio's patches that were posted as a reply to
>>> <20170212184132.12375-1-gits...@pobox.com>.
>>>
>>> As per Matthieu's comments, I have updated the tests, but there is still one
>>> thing that is not working: log -@{yesterday} or log -@{2.days.ago}
>>
>> Note that I did not request that these things work, just that they seem
>> to be relevant tests: IMHO it's OK to reject them, but for example we
>> don't want them to segfault. And having a test is a good hint that you
>> thought about what could happen and to document it.
>
> The branch we were on before would be a ref, and the ref may know
> where it was yesterday?  If @{-1}@{1.day} works it would be natural
> to expect -@{1.day} to, too, but there probably is some disambiguity
> or other reasons that they cannot or should not work that way I am
> missing, in which case it is fine ("too much work for too obscure
> feature that is not expected to be used often" is also an acceptable
> reason) to punt or deliberately not support it, as long as it is
> explained in the log and/or doc (future developers need to know if
> we are simply punting, or if we found a case where it would hurt end
> user experience if we supported the feature), and as long as it does
> not do a wrong thing (dying with "we do not support it" is OK,
> segfaulting or doing random other things is not).
>

Right now, these commands die with an "fatal: unrecognized argument:
-@{yesterday}" or a "fatal: unrecognized argument: -@{2.days.ago}".
So, it is definitely not doing anything "random" :)

I will wait for consensus on whether these should or should not be
supported.

-- 

Best Regards,

- Siddharth.

Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option

[Siddharth Kannan]

Hey Matthieu,

On 16 February 2017 at 23:52, Matthieu Moy  wrote:
>
> Indeed, I misread the patch. The explanation could be a little bit more
> "tired-reviewer-proof" by not using a past tone, perhaps
>
> 1. setup_revision, which is changed to ...

Oh, okay! Sorry about the confusion!

Yes, I used the past perfect tense to refer to changes that were made
in this particular patch!

I will change the message in the next version to something that's in
present tense.

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



-- 

Best Regards,

- Siddharth.

Re: [PATCH 1/1] config.txt: Fix formatting of submodule.alternateErrorStrategy section

[Junio C Hamano]

Stefan Beller  writes:

> On Wed, Feb 15, 2017 at 9:05 PM, David Pursehouse
>  wrote:
>> From: David Pursehouse 
>>
>> Add missing `::` after the title.
>>
>> Signed-off-by: David Pursehouse 
>
> Thanks!
> Stefan

Thanks, both.

Re: [PATCH v3] reset: add an example of how to split a commit into two

[Junio C Hamano]

Jacob Keller  writes:

> The interdiff between v2 and v3 is not really worth showing since I
> basically re-wrote the entire section a bit.

Could this be made into an incremental, now that v2 has been in
'next' for about 10 days, please?

> +Split a commit apart into a sequence of commits::
> ++
> +Suppose that you have create lots of logically separate changes and commit 
> them

s/create//; s/commit//

> +together. Then, later you decide that it might be better to have each logical
> +chunk associated with its own commit. You can use git reset to rewind history
> +without changing the contents of your local files, and then successively use
> +git add -p to interactively select which hunks to include into each commit,
> +using git commit -c to pre-populate the commit message.
> ++
> +
> +$ git reset -N HEAD^<1>
> +$ git add -p<2>
> +$ git diff --cached <3>
> +$ git commit -c HEAD@{1}<4>
> +... <5>
> +$ git add ...   <6>
> +$ git diff --cached <7>
> +$ git commit ...<8>
> +
> ++
> +<1> First, reset the history back one commit so that we remove the original
> +commit, but leave the working tree with all the changes. The -N ensures
> +that any new files added with HEAD are still marked so that git add -p
> +will find them.
> +<2> Next, we interactively select diff hunks to add using the git add -p
> +facility. This will ask you about each diff hunk in sequence and you can
> +use simple commands such as "yes, include this", "No don't include this"
> +or even the very powerful "edit" facility.
> +<3> Once satisfied with the hunks you want to include, you should verify what
> +has been prepared for the first commit by using git diff --cached. This
> +shows all the changes that have been moved into the index and are about
> +to be committed.
> +<4> Next, commit the changes stored in the index. The -c option specifies to
> +pre-populate the commit message from the original message that you 
> started
> +with in the first commit. This is helpful to avoid retyping it. The 
> HEAD@{1}
> +is a special notation for the commit that HEAD used to be at prior to the
> +original reset commit (1 change ago). See linkgit:git-reflog[1] for more
> +details. You may also use any other valid commit reference.
> +<5> You can repeat steps 2-4 multiple times to break the original code into
> +any number of commits.
> +<6> Now you've split out many of the changes into their own commits, and 
> might
> +no longer use the patch mode of git add, in order to select all remaining
> +uncommitted changes.
> +<7> Once again, check to verify that you've included what you want to. You 
> may
> +also wish to verify that git diff doesn't show any remaining changes to 
> be
> +committed later.
> +<8> And finally create the final commit.
> +

Nicely done.  We could talk more "best practice" things in this
sequence (e.g. "'stash --keep' then test in isolation"), but it is
already sufficiently long, so extending it may hurt the readability
more than it helps by guiding the readers to better ways.

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

[Junio C Hamano]

Siddharth Kannan  writes:

> Instead of replacing the whole string, we would expand it accordingly using:
>
> if (*name == '-') {
>   if (len == 1) {
> name = "@{-1}";
> len = 5;
>   } else {
> struct strbuf changed_argument = STRBUF_INIT;
>
> strbuf_addstr(_argument, "@{-1}");
> strbuf_addstr(_argument, name + 1);
>
> strbuf_setlen(_argument, strlen(name) + 4);
>
> name = strbuf_detach(_argument, NULL);
>   }
> }
>
> Junio's comments on a previous version of the patch which used this same
> approach but inside setup_revisions [1]
>
> [1]: 

What I said is that when we know we got "-", there is no reason to
replace it with and textually parse "@{-1}".

> + if (*name == '-' && len == 1) {
> + name = "@{-1}";
> + len = 5;
> + }
> +
>   ret = get_sha1_basic(name, len, sha1, lookup_flags);

If we look at get_sha1_basic(), it obviously is not prepared to
understand "-" as "@{-1}", and the primary obstacle is that the
underlying interpret_nth_prior_checkout() does two things.  It
expects to take "@{-}" as a string, and the first half parses
the  into "long nth".  The latter half then finds the nth prior
checkout.  We probably should factor out the latter half into a
separate function find_nth_prior_checkout() that takes "long nth" as
input, and call it from interpret_nth_prior_checkout(), as a
preparatory step.  Once it is done, get_sha1_basic() can notice that
it was fed (len == 1 && str[0] == '-') and make a direct call to
find_nth_prior_checkout() without going through the "pass '@{-1}' as
text, have interpret_nth_prior_checkout() to parse it to recover 1",
which is a roundabout way to do what you want to do.

Having said all that, I do not think the remainder of the code is
prepared to take "-", not yet anyway [*1*], so turning "-" into
"@{-1}" this patch does before it calls get_sha1_basic(), while it
is not an ideal final state, is probably an acceptable milestone to
stop at.

It is a separate matter if this patch is sufficient to produce
correct results, though.  I haven't studied the callers of this
change to make sure yet, and may find bugs in this approach later.


[Footnote]

*1* For example, the existing callsite in get_sha1_basic() that
calls interpret_nth_prior_checkout() does not replace "str" with
what was returned when the HEAD is not detached.  The callpath
then depends on dwim_ref() to also understand "@{-1}" it got
from the caller.  If we really want to keep what came from the
end user as-is so that error message can include it, we'd need
to teach dwim_ref() about the new "-" convention.  The extent of
necessary change will become a lot larger.  On the other hand,
if we allow error messages and reports to use a real refname
instead of parrotting exactly what the user gave us, I think we
may be able to arrange to replace str/len in get_sha1_basic()
when we call interpret/find_nth_prior_checkout() and get a ref,
without having to teach the new "-" convention all over the
place.

Re: Back quote typo in error messages (?)

[Jeff King]

On Thu, Feb 16, 2017 at 06:41:26PM +, Fabrizio Cucci wrote:

> On 15 February 2017 at 21:56, Jeff King  wrote:
> > Grep for "``" in Git's documentation directory, and you will see many
> > examples (asciidoc only accepts the double-quote form, not singles).
> >
> > You can also try:
> >
> >   echo "this is \`\`quoted'' text" >foo.txt
> >   asciidoc foo.txt
> >
> > and then open "foo.html" in your browser.
> 
> We are probably going a bit OT here :) but AFAIK there is no such
> thing as non-symmetric start/end quotes in AsciiDoc.
> 
> Even enclosing something in curved quotes is done as follows:
> 
> '`single curved quotes`'
> 
> "`double curved quotes`"
> 
> (http://asciidoctor.org/docs/asciidoc-syntax-quick-reference/)

Try the "Quoted Text" section of the original asciidoc user manual:

  http://www.methods.co.nz/asciidoc/userguide.html#X51

Asciidoctor has introduced some new syntax (almost certainly because the
original asymmetric formulations have a bunch of ambiguous corner
cases). By default, it disables the asymmetric versions, but they work
in "compat" mode (and the newer ones do not).

Git's documentation is all written for the original asciidoc. If you
build it with asciidoctor, it must be done in compat mode. This is the
default when asciidoctor sees a two-line (i.e., underlined) section
title, which all of our manpages have.

More details at:

  http://asciidoctor.org/docs/migration/#migration-cheatsheet

Yes, we are off-topic,. but I think it is worth clarifying Git's
asciidoc compatibility expectations. :)

-Peff

Re: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"

[Junio C Hamano]

Matthieu Moy  writes:

> Siddharth Kannan  writes:
>
>> This is as per our discussion[1]. The patches and commit messages are based 
>> on
>> Junio's patches that were posted as a reply to
>> <20170212184132.12375-1-gits...@pobox.com>.
>>
>> As per Matthieu's comments, I have updated the tests, but there is still one
>> thing that is not working: log -@{yesterday} or log -@{2.days.ago}
>
> Note that I did not request that these things work, just that they seem
> to be relevant tests: IMHO it's OK to reject them, but for example we
> don't want them to segfault. And having a test is a good hint that you
> thought about what could happen and to document it.

The branch we were on before would be a ref, and the ref may know
where it was yesterday?  If @{-1}@{1.day} works it would be natural
to expect -@{1.day} to, too, but there probably is some disambiguity
or other reasons that they cannot or should not work that way I am
missing, in which case it is fine ("too much work for too obscure
feature that is not expected to be used often" is also an acceptable
reason) to punt or deliberately not support it, as long as it is
explained in the log and/or doc (future developers need to know if
we are simply punting, or if we found a case where it would hurt end
user experience if we supported the feature), and as long as it does
not do a wrong thing (dying with "we do not support it" is OK,
segfaulting or doing random other things is not).

Thanks.

Re: [BUG] submodule config does not apply to upper case submodules?

[Junio C Hamano]

Jonathan Tan  writes:

> On 02/15/2017 03:11 PM, Junio C Hamano wrote:
>> Junio C Hamano  writes:
>>
>> Perhaps something like this?
>
> This looks good. I was hoping to unify the processing logic between
> this CLI parsing and the usual stream parsing, but this approach is
> probably simpler.

I looked at the stream parsing before I wrote the patch for the same
reason, and I agree that the stream parsing of course does not get a
single variable name at one place [*1*], so there is nothing that
can be shared.

[Footnote]

*1* Instead "[ ""]  = " is split and
each piece is assembled into section.subsection.var with its own
downcasing.

Re: [PATCH 1/1] config.txt: Fix formatting of submodule.alternateErrorStrategy section

[Stefan Beller]

On Wed, Feb 15, 2017 at 9:05 PM, David Pursehouse
 wrote:
> From: David Pursehouse 
>
> Add missing `::` after the title.
>
> Signed-off-by: David Pursehouse 

Thanks!
Stefan

Re: Back quote typo in error messages (?)

[Fabrizio Cucci]

On 15 February 2017 at 21:56, Jeff King  wrote:
> Grep for "``" in Git's documentation directory, and you will see many
> examples (asciidoc only accepts the double-quote form, not singles).
>
> You can also try:
>
>   echo "this is \`\`quoted'' text" >foo.txt
>   asciidoc foo.txt
>
> and then open "foo.html" in your browser.

We are probably going a bit OT here :) but AFAIK there is no such
thing as non-symmetric start/end quotes in AsciiDoc.

Even enclosing something in curved quotes is done as follows:

'`single curved quotes`'

"`double curved quotes`"

(http://asciidoctor.org/docs/asciidoc-syntax-quick-reference/)

> I think patches would be welcome, but as Junio said, it probably should
> wait for the next cycle.

It can definitely wait and I would be glad to contribute!

Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option

[Matthieu Moy]

Junio C Hamano  writes:

> Matthieu Moy  writes:
>
>> Siddharth Kannan  writes:
>>
>>> handle_revision_opt() tries to recognize and handle the given argument. If 
>>> an
>>> option was unknown to it, it used to add the option to unkv[(*unkc)++].  
>>> This
>>> increment of unkc causes the variable in the caller to change.
>>>
>>> Teach handle_revision_opt to not update unknown arguments inside unkc 
>>> anymore.
>>> This is now the responsibility of the caller.
>>>
>>> There are two callers of this function:
>>>
>>> 1. setup_revision: Changes have been made so that setup_revision will now
>>> update the unknown option in argv
>>
>> You're writting "Changes have been made", but I did not see any up to
>> this point in the series.
>
> Actually, I think you misread the patch and explanation.
> handle_revision_opt() used to be responsible for stuffing unknown
> ones to unkv[] array passed from the caller even when it returns 0
> (i.e. "I do not know what they are" case, as opposed to "I know what
> they are, I am not handling them here and leaving them in unkv[]"
> case--the latter returns non-zero).  The first hunk makes the
> function stop doing so, and to compensate, the second hunk, which is
> in setup_revisions()

Indeed, I misread the patch. The explanation could be a little bit more
"tired-reviewer-proof" by not using a past tone, perhaps

1. setup_revision, which is changed to ...

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

Re: body-CC-comment regression

[Matthieu Moy]

Johan Hovold  writes:

> Hi,
>
> I recently noticed that after an upgrade, git-send-email (2.10.2)
> started aborting when trying to send patches that had a linux-kernel
> stable-tag in its body. For example,
>
>   Cc: # 4.4
>
> was now parsed as
>
>   "sta...@vger.kernel.org#4.4"
>
> which resulted in
>
>   Died at /usr/libexec/git-core/git-send-email line 1332,  line 1.

This has changed in e3fdbcc8e1 (parse_mailboxes: accept extra text after
<...> address, 2016-10-13), released v2.11.0 as you noticed:

> The problem with the resulting fixes that are now in 2.11.1 is that
> git-send-email no longer discards the trailing comment but rather
> shoves it into the name after adding some random white space:
>
>   "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" 
> "
>
> This example is based on the example from
> Documentation/process/stable-kernel-rules.rst:
>
>   Cc:  # 3.3.x: 1b9508f: sched: Rate-limit newidle
>
> and this format for stable-tags has been documented at least since 2009
> and 8e9b9362266d ("Doc/stable rules: add new cherry-pick logic"), and
> has been supported by git since 2012 and 831a488b76e0 ("git-send-email:
> remove garbage after email address") I believe.
>
> Can we please revert to the old behaviour of simply discarding such
> comments (from body-CC:s) or at least make it configurable through a
> configuration option?

The problem is that we now accept list of emails instead of just one
email, so it's hard to define what "comments after the email", for
example

Cc:  # , 

Is not accepted as two emails.

So, just stripping whatever comes after # before parsing the list of
emails would change the behavior once more, and possibly break other
user's flow. Dropping the garbage after the email while parsing is
possible, but only when we use our in-house parser (and we currently use
Perl's Mail::Address when available).

So, a proper fix is far from obvious, and unfortunately I won't have
time to work on that, at least not before a while.

OTOH, the current behavior isn't that bad. It accepts the input, and
extracts a valid email out of it. Just the display name is admitedly
suboptimal ...

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

Re: body-CC-comment regression

[Johan Hovold]

On Thu, Feb 16, 2017 at 09:59:25AM -0800, Junio C Hamano wrote:
> Johan Hovold  writes:
> 
> > I recently noticed that after an upgrade, git-send-email (2.10.2)
> > started aborting when trying to send patches that had a linux-kernel
> > stable-tag in its body. For example,
> >
> > Cc: # 4.4
> >
> > was now parsed as
> >
> > "sta...@vger.kernel.org#4.4"
> > ...
> 
> It sounds like a fallout of this:
> 
>   
> https://public-inbox.org/git/41164484-309b-bfff-ddbb-55153495d...@lwfinger.net/#t
>  
> 
> and any change to "fix" you may break the other person.

Yes, that's the thread I was referring to as well, and the reported
breakage is the same even if the reporter used a non-standard stable-tag
format (e.g. a "[4.8+]" suffix).

What I'm wondering is whether the alternative fix proposed in that
thread, to revert to the old behaviour of discarding trailing comments,
should be considered instead of what was implemented.

> > Can we please revert to the old behaviour of simply discarding such
> > comments (from body-CC:s) or at least make it configurable through a
> > configuration option?
> 
> If I recall the old thread correctly, it was reported that using
> Mail::Address without forcing git-send-email fall back to its own
> non-parsing-but-paste-address-looking-things-together code would
> solve it, so can the "make it configurable" be just "install
> Mail::Address"?

I believe git-send-email's parser was changed to mimic Mail::Address,
and installing it does not seem to change the behaviour of including any
trailing comments in the name.

Thanks,
Johan

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

[Johannes Sixt]

Am 13.02.2017 um 18:16 schrieb Johannes Schindelin:

On Sat, 11 Feb 2017, Johannes Sixt wrote:

Am 10.02.2017 um 00:41 schrieb Junio C Hamano:

Johannes Schindelin  writes:


From: Jeff Hostetler 

Use OpenSSL's SHA-1 routines rather than builtin block-sha1
routines.  This improves performance on SHA1 operations on Intel
processors.
...

Signed-off-by: Jeff Hostetler 
Signed-off-by: Johannes Schindelin 
---


Nice.  Will queue as jh/mingw-openssl-sha1 topic; it is a bit too
late for today's integration cycle to be merged to 'next', but let's
have this by the end of the week in 'master'.


Please don't rush this through. I didn't have a chance to cross-check the
patch; it will have to wait for Monday. I would like to address Peff's
concerns about additional runtime dependencies.


I never meant this to be fast-tracked into git.git. We have all the time
in our lives to get this in, as Git for Windows already carries this patch
for a while, and shall continue to do so.


I've been working with this patch for the past few days, and I did not 
notice any disadvantage during interactive work even though there is a 
new dependency on libcrypto.dll.


Here are some unscientific numbers collected during test suite runs:

bash -c "time make -j4 -k test"

with this patch:

real34m47.242s
user9m55.827s
sys 25m20.483s

without this patch:

real34m2.330s
user9m56.556s
sys 25m5.520s

It looks like BLK_SHA1 has some advantage, but I would not count on 
these figures too much. (I certainly did not sit idly in front of the 
workstation during these tests, for example. That may have skewed the 
numbers somewhat.) (And, no, I'm not going to measure best-of-five 
timings, not even best-of-two. ;)


In summary: Interactive response times do not decline noticably. I do 
not object the patch.


-- Hannes

Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option

[Junio C Hamano]

Matthieu Moy  writes:

> Siddharth Kannan  writes:
>
>> handle_revision_opt() tries to recognize and handle the given argument. If an
>> option was unknown to it, it used to add the option to unkv[(*unkc)++].  This
>> increment of unkc causes the variable in the caller to change.
>>
>> Teach handle_revision_opt to not update unknown arguments inside unkc 
>> anymore.
>> This is now the responsibility of the caller.
>>
>> There are two callers of this function:
>>
>> 1. setup_revision: Changes have been made so that setup_revision will now
>> update the unknown option in argv
>
> You're writting "Changes have been made", but I did not see any up to
> this point in the series.

Actually, I think you misread the patch and explanation.
handle_revision_opt() used to be responsible for stuffing unknown
ones to unkv[] array passed from the caller even when it returns 0
(i.e. "I do not know what they are" case, as opposed to "I know what
they are, I am not handling them here and leaving them in unkv[]"
case--the latter returns non-zero).  The first hunk makes the
function stop doing so, and to compensate, the second hunk, which is
in setup_revisions() that calls the function, now makes the caller
do the equivalent "argv[left++] = arg" there after it receives 0.

So "Changes have been made" to setup_revisions() to compensate for
the change of behaviour in the called function.

The enumerated point 2. (not in your response) explains why such a
corresponding compensatory change is not there for the other caller
of this function whose behaviour has changed.

> We write patch series so that they are bisectable, i.e. each commit
> should be correct (compileable, pass tests, consistent
> documentation, ...). Here, it seems you are introducing a breakage to
> repair it later.

That is a very good point to stress, but 1. is exactly to avoid
breakage in this individual step (and 2. is an explanation why the
change does not break the other caller).

Re: [PATCH] mingw: make stderr unbuffered again

[Junio C Hamano]

Johannes Sixt  writes:

> Am 16.02.2017 um 18:10 schrieb Johannes Schindelin:
>> On Wed, 15 Feb 2017, Johannes Sixt wrote:
>>> I do not see how dup2() causes a change in stdio state. I
>>> must be missing something (and that may be a basic misunderstanding of how
>>> stdio is initialized).
>>
>> It appears that dup2()ing fd 2 resets that stdio state.
>
> OK, thanks. It's surprising, but so be it.
>
> Thank you for the quick fix!

I am happy to see two Windows folks being happy.  I tentatively
marked this as "undecided" after merging it to 'next', but let's
have it in the -rc2 and final.  Two people familiar with Windows
agreeing that there is no longer mystery why it fixes, after seeing
that the update fixes a regression, is a strong enough sign to me
that including it is better than leaving it out.

Thanks.

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

[Brandon Williams]

On 02/15, Stefan Beller wrote:
> +static void reload_gitmodules_file(struct index_state *index,
> +struct checkout *state)
> +{
> + int i;
> + for (i = 0; i < index->cache_nr; i++) {
> + struct cache_entry *ce = index->cache[i];
> + if (ce->ce_flags & CE_UPDATE) {
> +
> + int r = strcmp(ce->name, ".gitmodules");
> + if (r < 0)
> + continue;
> + else if (r == 0) {
> + checkout_entry(ce, state, NULL);
> + } else
> + break;
> + }
> + }
> + gitmodules_config();
> + git_config(submodule_config, NULL);
> +}

If we are reloading the gitmodules file do you think it would makes
sense to add in a call to 'submodule_free()' to clear the cache used to
store the gitmodules config?

-- 
Brandon Williams

Re: body-CC-comment regression

[Junio C Hamano]

Johan Hovold  writes:

> I recently noticed that after an upgrade, git-send-email (2.10.2)
> started aborting when trying to send patches that had a linux-kernel
> stable-tag in its body. For example,
>
>   Cc: # 4.4
>
> was now parsed as
>
>   "sta...@vger.kernel.org#4.4"
> ...

It sounds like a fallout of this:

  
https://public-inbox.org/git/41164484-309b-bfff-ddbb-55153495d...@lwfinger.net/#t
 

and any change to "fix" you may break the other person.

> Can we please revert to the old behaviour of simply discarding such
> comments (from body-CC:s) or at least make it configurable through a
> configuration option?

If I recall the old thread correctly, it was reported that using
Mail::Address without forcing git-send-email fall back to its own
non-parsing-but-paste-address-looking-things-together code would
solve it, so can the "make it configurable" be just "install
Mail::Address"?

Re: [PATCH] mingw: make stderr unbuffered again

[Johannes Sixt]

Am 16.02.2017 um 18:10 schrieb Johannes Schindelin:

On Wed, 15 Feb 2017, Johannes Sixt wrote:

I do not see how dup2() causes a change in stdio state. I
must be missing something (and that may be a basic misunderstanding of how
stdio is initialized).


It appears that dup2()ing fd 2 resets that stdio state.


OK, thanks. It's surprising, but so be it.

Thank you for the quick fix!

-- Hannes

body-CC-comment regression

[Johan Hovold]

Hi,

I recently noticed that after an upgrade, git-send-email (2.10.2)
started aborting when trying to send patches that had a linux-kernel
stable-tag in its body. For example,

Cc: # 4.4

was now parsed as

"sta...@vger.kernel.org#4.4"

which resulted in

Died at /usr/libexec/git-core/git-send-email line 1332,  line 1.

This tends to happen in the middle of a series after the cover letter
had been sent just to make things worse...

I finally got around to looking into this today and discovered this
thread ("Re: Formatting problem send_mail in version 2.10.0"):

https://marc.info/?l=git=147633706724793=2

The problem with the resulting fixes that are now in 2.11.1 is that
git-send-email no longer discards the trailing comment but rather
shoves it into the name after adding some random white space:

"# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" 
"

This example is based on the example from
Documentation/process/stable-kernel-rules.rst:

Cc:  # 3.3.x: 1b9508f: sched: Rate-limit newidle

and this format for stable-tags has been documented at least since 2009
and 8e9b9362266d ("Doc/stable rules: add new cherry-pick logic"), and
has been supported by git since 2012 and 831a488b76e0 ("git-send-email:
remove garbage after email address") I believe.

Can we please revert to the old behaviour of simply discarding such
comments (from body-CC:s) or at least make it configurable through a
configuration option?

Thanks,
Johan

Re: [PATCH 0/3] Fix l10n

[Jeff King]

On Thu, Feb 16, 2017 at 02:28:26PM +0300, Maxim Moseychuk wrote:

> In some places static size buffers can't store formatted string.
> If it be happen then git die.
> 
> Maxim Moseychuk (3):
>   add git_psprintf helper function
>   bisect_next_all: convert xsnprintf to git_psprintf
>   stop_progress_msg: convert xsnprintf to git_psprintf

Thanks for providing a series (and I think this is your first series, so
welcome!).

The overall goal looks good, and I dropped a few comments in reply to
the individual patches.

-Peff

Re: [PATCH 3/3] stop_progress_msg: convert xsnprintf to git_psprintf

[Jeff King]

On Thu, Feb 16, 2017 at 02:28:29PM +0300, Maxim Moseychuk wrote:

> Replace local safe string buffer allocation by git_psprintf.

Hmm. Is this one actually broken in practice?

I see:

> @@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, 
> const char *msg)
>   *p_progress = NULL;
>   if (progress->last_value != -1) {
>   /* Force the last update */
> - char buf[128], *bufp;
> - size_t len = strlen(msg) + 5;
> + char *bufp;

We have a fixed-size buffer here, but...

>   struct throughput *tp = progress->throughput;
>  
> - bufp = (len < sizeof(buf)) ? buf : xmallocz(len);

If it's not big enough we allocate a new one. So this works for any size
of translated string. It just tries to skip the allocation in the
"short" case.

If this were in the normal progress code, I might care more about trying
to skip an allocation for performance. But since this is just in
stop_progress_msg(), I don't think the complexity is worth it.  I'm
especially happy to see the magic "5" above go away.

So I think this is worth doing, but the motivation is a simplification,
not a bugfix.

>   if (tp) {
>   unsigned int rate = !tp->avg_misecs ? 0 :
>   tp->avg_bytes / tp->avg_misecs;
>   throughput_string(>display, tp->curr_total, rate);
>   }
>   progress_update = 1;
> - xsnprintf(bufp, len + 1, ", %s.\n", msg);
> + bufp = git_psprintf(", %s.\n", msg);
>   display(progress, progress->last_value, bufp);
> - if (buf != bufp)
> - free(bufp);
> + free(bufp);

This parts looks good (modulo using xstrfmt). It's probably worth
renaming "bufp" to the more usual "buf", I would think.

I do wonder if the punctuation here will give translators headaches.
E.g., in a RTL language you probably wouldn't want that period at the
end. I don't know enough to say, so I'm willing to punt on that for now.
But I suspect in the long run we may need to just take the whole
translated message, punctuation included, from the caller. There are
only two callers that actually provide a string.

-Peff

Re: [PATCH 2/3] bisect_next_all: convert xsnprintf to git_psprintf

[Jeff King]

On Thu, Feb 16, 2017 at 02:28:28PM +0300, Maxim Moseychuk wrote:

> Git can't run bisect between 2048+ commits if use russian translation.
> Reproduce "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources.
> 
> Dummy solution: just increase buffer size but is not safe.
> Size gettext string is a runtime value.

Hmm. I wondered if this used to work (because xsnprintf operated on a
fixed-size fmt) and was broken in the translation. And as a consequence,
if we needed to be searching for other cases with similar bugs.

But no, in this case the fixed-size buffer was actually introduced
during the i18n step from 14dc4899e (i18n: bisect: mark strings for
translation, 2016-06-17).

I guess the other type of bug could still exist, though.

> diff --git a/bisect.c b/bisect.c
> index 21bc6daa4..8670cc97a 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
>   struct commit_list *tried;
>   int reaches = 0, all = 0, nr, steps;
>   const unsigned char *bisect_rev;
> - char steps_msg[32];
> + char *steps_msg;

So one solution would be to just bump the "32" higher here. The format
comes from the translation, so in practice it only needs to be large
enough to fit any of our translations.

That feels pretty hacky, though. In practice the set of translations is
contained, but it doesn't have to be (and certainly nobody would notice
if a new translation was added with a longer name until a user
complained).

> @@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)
>  
>   nr = all - reaches - 1;
>   steps = estimate_bisect_steps(all);
> - xsnprintf(steps_msg, sizeof(steps_msg),
> -   Q_("(roughly %d step)", "(roughly %d steps)", steps),
> -   steps);
> +
> + steps_msg = git_psprintf(Q_("(roughly %d step)", "(roughly %d steps)",
> +   steps), steps);
>   /* TRANSLATORS: the last %s will be replaced with
>  "(roughly %d steps)" translation */
>   printf(Q_("Bisecting: %d revision left to test after this %s\n",
> "Bisecting: %d revisions left to test after this %s\n",
> nr), nr, steps_msg);
> + free(steps_msg);

I wondered if a viable solution would be to make the whole thing one
single translatable string. It would avoid the need for the TRANSLATORS
comment. But I guess we have two "Q_" invocations here, and they might
need to be handled separately (e.g., "2 revisions", "1 step").

I also wondered if we could just make this into two printf statements
("revisions left to test", followed by "roughly N steps").  But the
commit message for 14dc4899e mentions RTL languages. So I think we
really do need to build it up block by block and let the translators
adjust the ordering.

So I think your solution is the best we can do.

It's probably worth outlining these alternatives in the commit message.

-Peff

Re: What's cooking in git.git (Feb 2017, #04; Tue, 14)

[Junio C Hamano]

René Scharfe  writes:

> Am 14.02.2017 um 23:59 schrieb Junio C Hamano:
>> * rs/ls-files-partial-optim (2017-02-13) 2 commits
>>  - ls-files: move only kept cache entries in prune_cache()
>>  - ls-files: pass prefix length explicitly to prune_cache()
>>
>>  "ls-files" run with pathspec has been micro-optimized to avoid one
>>  extra call to memmove().
>
> Nit: The number of memmove(3) calls stays the same, but the number of
> bytes that are moved is reduced.

Blush X-< you are right of course.  Will fix to say "... to avoid
having to memmove(3) unnecessary bytes."

Thanks.

Re: [PATCH] mingw: make stderr unbuffered again

[Johannes Schindelin]

Hi Hannes,

On Wed, 15 Feb 2017, Johannes Sixt wrote:

> Am 15.02.2017 um 13:32 schrieb Johannes Schindelin:
> > On Tue, 14 Feb 2017, Johannes Sixt wrote:
> > > Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:
> > > > On Mon, 13 Feb 2017, Junio C Hamano wrote:
> > > > > Johannes Schindelin  writes:
> > > > > > What we forgot was to mark stderr as unbuffered again.
> > >
> > > I do not see how the earlier patch turned stderr from unbuffered to
> > > buffered, as it did not add or remove any setvbuf() call. Can you
> > > explain?
> >
> > [ motivation and history of js/mingw-isatty snipped ]
> >
> > So instead of "bending" the target HANDLE of the existing
> > stdout/stderr (which would *naturally* have kept the
> > buffered/unbuffered nature as-is), we now redirect with correct API
> > calls.
> 
> Your statement implies that at the time when winansi_init() begins,
> stdio is already initialized and the buffered/unbuffered state has been
> set for stderr.  I would think that this is true.
> 
> Then we swap out the file handle underlying stderr in swap_osfhnd()
> using dup2(). Why would that change the buffered state of stdio?

The file handle we swap in for stderr points to the pipe that a
freshly-started thread consumes for parsing the ANSI color sequences. This
handle is used both for stdout and stderr. The dup2() call then implicitly
reopens stderr, with the default buffering.

> > And the patch I provided at the bottom of this mail thread reinstates
> > the unbuffered nature of stderr now that it gets reopened.
> >
> > Hopefully that makes it clear why the setvbuf() call is required now,
> > but was previously unnecessary?
> 
> Unfortunately, no. I do not see how dup2() causes a change in stdio state. I
> must be missing something (and that may be a basic misunderstanding of how
> stdio is initialized).

It appears that dup2()ing fd 2 resets that stdio state.

Ciao,
Dscho

[PATCH V2 0/2] Fix l10n

[Maxim Moseychuk]

In some places static size buffers can't store formatted string.
If it be happen then git die.

Jonathan Tan: Thanks a lot for your help.

Maxim Moseychuk (2):
  bisect_next_all: convert xsnprintf to xstrfmt
  stop_progress_msg: convert xsnprintf to xstrfmt

 bisect.c   | 9 +
 progress.c | 9 +++--
 2 files changed, 8 insertions(+), 10 deletions(-)

--
2.11.1

[PATCH V2 1/2] bisect_next_all: convert xsnprintf to xstrfmt

[Maxim Moseychuk]

Git can't run bisect between 2048+ commits if use russian translation.
Reproduce "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources.

Dummy solution: just increase buffer size but is not safe.
Size gettext string is a runtime value.

Signed-off-by: Maxim Moseychuk 
---
 bisect.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/bisect.c b/bisect.c
index 21bc6daa4..787543cad 100644
--- a/bisect.c
+++ b/bisect.c
@@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
struct commit_list *tried;
int reaches = 0, all = 0, nr, steps;
const unsigned char *bisect_rev;
-   char steps_msg[32];
+   char *steps_msg;
 
read_bisect_terms(_bad, _good);
if (read_bisect_refs())
@@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)
 
nr = all - reaches - 1;
steps = estimate_bisect_steps(all);
-   xsnprintf(steps_msg, sizeof(steps_msg),
- Q_("(roughly %d step)", "(roughly %d steps)", steps),
- steps);
+
+   steps_msg = xstrfmt(Q_("(roughly %d step)", "(roughly %d steps)",
+ steps), steps);
/* TRANSLATORS: the last %s will be replaced with
   "(roughly %d steps)" translation */
printf(Q_("Bisecting: %d revision left to test after this %s\n",
  "Bisecting: %d revisions left to test after this %s\n",
  nr), nr, steps_msg);
+   free(steps_msg);
 
return bisect_checkout(bisect_rev, no_checkout);
 }
-- 
2.11.1

[PATCH V2 2/2] stop_progress_msg: convert xsnprintf to xstrfmt

[Maxim Moseychuk]

Replace local safe string buffer allocation by xstrfmt.

Signed-off-by: Maxim Moseychuk 
---
 progress.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/progress.c b/progress.c
index 76a88c573..29d0dad58 100644
--- a/progress.c
+++ b/progress.c
@@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, 
const char *msg)
*p_progress = NULL;
if (progress->last_value != -1) {
/* Force the last update */
-   char buf[128], *bufp;
-   size_t len = strlen(msg) + 5;
+   char *bufp;
struct throughput *tp = progress->throughput;
 
-   bufp = (len < sizeof(buf)) ? buf : xmallocz(len);
if (tp) {
unsigned int rate = !tp->avg_misecs ? 0 :
tp->avg_bytes / tp->avg_misecs;
throughput_string(>display, tp->curr_total, rate);
}
progress_update = 1;
-   xsnprintf(bufp, len + 1, ", %s.\n", msg);
+   bufp = xstrfmt(", %s.\n", msg);
display(progress, progress->last_value, bufp);
-   if (buf != bufp)
-   free(bufp);
+   free(bufp);
}
clear_progress_signal();
if (progress->throughput)
-- 
2.11.1

Re: [PATCH 1/3] add git_psprintf helper function

[Jeff King]

On Thu, Feb 16, 2017 at 07:51:14AM -0800, Jonathan Tan wrote:

> On 02/16/2017 03:28 AM, Maxim Moseychuk wrote:
> > There are a number of places in the code where we call
> > xsnprintf(), with the assumption that the output will fit into
> > the buffer. If the buffer is small, then git die.
> > In many places buffers have compile-time size, but generated string
> > depends from current system locale (gettext)and can have size
> > greater the buffer.
> > Just run "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8"
> > on linux sources - it impossible.
> > 
> > git_psprintf is similar to the standard C sprintf() function
> > but safer, since it calculates the maximum space required
> > and allocates memory to hold the result.
> > The returned string should be freed with free() when no longer needed.
> 
> If I understand this correctly, xstrfmt (in strbuf.h) should already do what
> you need, so you do not need a new function.

Yes, this is exactly what xstrfmt is for.

-Peff

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

[Junio C Hamano]

Lars Schneider  writes:

>> On 16 Feb 2017, at 00:48, Junio C Hamano  wrote:
>> 
>> The "git -c = cmd" mechanism is to pretend that a
>
> The problem is also present for gitconfig variables e.g.
> git config --local submodule.UPPERSUB.update none

Yuck.

> But your patch below fixes that, too!

Heh.

> Should we add a test case to this patch?
> If not, do you want me to improve my test case patch [1] 
> or do you want to ditch the test?

I think a new test for submodule (although it already exists thanks
t you) is  a bit too roundabout way to demonstrate the breakage and
protect the fix.

We'd perhaps want some tests for "git config", probably.

In a repository with /etc/gitconfig and $HOME/ that are tightly
controlled (I think our test framework already do so), running these
would demonstrate both breakages, I would think, but I am sure
people can come up with other forms that are a lot easier to read..

$ git -c v.A.r=val -c v.a.r=ue config --get-all v.a.r
$ git -c v.A.r=val -c v.a.r=ue config --get-all v.A.r
$ git -c v.a.r=val -c v.A.r=ue config --get-all v.a.r
$ git -c v.a.r=val -c v.A.r=ue config --get-all v.A.r

Thanks.

Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option

[Matthieu Moy]

Siddharth Kannan  writes:

> handle_revision_opt() tries to recognize and handle the given argument. If an
> option was unknown to it, it used to add the option to unkv[(*unkc)++].  This
> increment of unkc causes the variable in the caller to change.
>
> Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
> This is now the responsibility of the caller.
>
> There are two callers of this function:
>
> 1. setup_revision: Changes have been made so that setup_revision will now
> update the unknown option in argv

You're writting "Changes have been made", but I did not see any up to
this point in the series.

We write patch series so that they are bisectable, i.e. each commit
should be correct (compileable, pass tests, consistent
documentation, ...). Here, it seems you are introducing a breakage to
repair it later.

Other that bisectability, this makes review harder: at this point the
reader knows it's broken, guesses that it will be repaired later, but
does not know in which patch.

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

Re: Confusing git messages when disk is full.

[Jeff King]

On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:

> >>int xfclose(FILE *fp)
> >>{
> >>return ferror(fp) | fclose(fp);
> >>}
> >
> > Yes, that's exactly what I had in mind (might be worth calling out the
> > bitwise-OR, though, just to make it clear it's not a typo).
> 
> Since the order of evaluation is unspecified, it would be better to
> force sequencing ferror before fclose.

Good point. Arguably the call in tempfile.c is buggy.

-Peff

Re: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"

[Matthieu Moy]

Siddharth Kannan  writes:

> This is as per our discussion[1]. The patches and commit messages are based on
> Junio's patches that were posted as a reply to
> <20170212184132.12375-1-gits...@pobox.com>.
>
> As per Matthieu's comments, I have updated the tests, but there is still one
> thing that is not working: log -@{yesterday} or log -@{2.days.ago}

Note that I did not request that these things work, just that they seem
to be relevant tests: IMHO it's OK to reject them, but for example we
don't want them to segfault. And having a test is a good hint that you
thought about what could happen and to document it.

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

Re: [PATCH 1/3] add git_psprintf helper function

[Jonathan Tan]

On 02/16/2017 03:28 AM, Maxim Moseychuk wrote:

There are a number of places in the code where we call
xsnprintf(), with the assumption that the output will fit into
the buffer. If the buffer is small, then git die.
In many places buffers have compile-time size, but generated string
depends from current system locale (gettext)and can have size
greater the buffer.
Just run "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8"
on linux sources - it impossible.

git_psprintf is similar to the standard C sprintf() function
but safer, since it calculates the maximum space required
and allocates memory to hold the result.
The returned string should be freed with free() when no longer needed.


If I understand this correctly, xstrfmt (in strbuf.h) should already do 
what you need, so you do not need a new function.

[PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"

[Siddharth Kannan]

This is as per our discussion[1]. The patches and commit messages are based on
Junio's patches that were posted as a reply to
<20170212184132.12375-1-gits...@pobox.com>.

As per Matthieu's comments, I have updated the tests, but there is still one
thing that is not working: log -@{yesterday} or log -@{2.days.ago}

For the other kinds of suffixes, such as -^ or -~ or -~N, the suffix
information is first extracted and then, the function get_sha1_1 is called with
name="-^" and len=1 (which is the reason for the changed condition inside Patch
4 of this series).

For -@{yesterday} kind of queries, the functions dwim_log,
interpret_branch_name and interpret_nth_prior_checkout are called.

1. A nice way to solve this would be to extend the replacement of "-" with
"@{-1}" one step further. Using strbuf, instead of replacing the whole string
with "@{-1}" we would simply replace "-" with "@{-1}" expanding the string
appropriately. This will ensure that all the code is inside the function
get_sha1_1. The code to do this is in the cover section of the 4th patch in this
series.

2. we could go down the dwim_log codepath, and find another suitable place to
make the same "-" -> "@{-1}" replacement. In the time that I spent till now, it
seems that the suffix information (i.e.  @{yesterday} or @{2.days.ago}) is
extracted _after_ the branch information has been extracted, so I suspect that
we will have to keep that part intact even in this solution.  (I am not too
sure about this. If this is the preferred solution, then I will dig deeper and
find the right place as I did for the first part of this patch)

Matthieu: Thanks a lot for your comments on the tests! test_commit has made the
tests a lot cleaner!

[1]: 

Siddharth Kannan (4):
  revision.c: do not update argv with unknown option
  revision.c: swap if/else blocks
  revision.c: args starting with "-" might be a revision
  sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"

 revision.c   |  15 ---
 sha1_name.c  |   5 +++
 t/t4214-log-shorthand.sh | 106 +++
 3 files changed, 120 insertions(+), 6 deletions(-)
 create mode 100755 t/t4214-log-shorthand.sh

-- 
2.1.4

[PATCH 1/4 v4] revision.c: do not update argv with unknown option

[Siddharth Kannan]

handle_revision_opt() tries to recognize and handle the given argument. If an
option was unknown to it, it used to add the option to unkv[(*unkc)++].  This
increment of unkc causes the variable in the caller to change.

Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
This is now the responsibility of the caller.

There are two callers of this function:

1. setup_revision: Changes have been made so that setup_revision will now
update the unknown option in argv

2. parse_revision_opt: No changes are required here. This function throws an
error whenever the option provided as argument was unknown to
handle_revision_opt().

Signed-off-by: Siddharth Kannan 
---
 revision.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index b37dbec..5674a9a 100644
--- a/revision.c
+++ b/revision.c
@@ -2016,8 +2016,6 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
revs->ignore_missing = 1;
} else {
int opts = diff_opt_parse(>diffopt, argv, argc, 
revs->prefix);
-   if (!opts)
-   unkv[(*unkc)++] = arg;
return opts;
}
if (revs->graph && revs->track_linear)
@@ -2234,6 +2232,8 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
}
if (opts < 0)
exit(128);
+   /* arg is an unknown option */
+   argv[left++] = arg;
continue;
}
 
-- 
2.1.4

[PATCH 3/4 v4] revision.c: args starting with "-" might be a revision

[Siddharth Kannan]

setup_revisions used to consider any argument starting with "-" to be either a
valid or an unknown option.

Teach setup_revisions to check if an argument is a revision before adding it as
an unknown option (something that setup_revisions didn't understand) to argv,
and moving on to the next argument.

This patch prepares the addition of "-" as a shorthand for "previous branch".

Signed-off-by: Siddharth Kannan 
---
 revision.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/revision.c b/revision.c
index 8d4ddae..5470c33 100644
--- a/revision.c
+++ b/revision.c
@@ -2203,6 +2203,7 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
read_from_stdin = 0;
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
+   int maybe_opt = 0;
if (*arg == '-') {
int opts;
 
@@ -2232,15 +2233,17 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
}
if (opts < 0)
exit(128);
-   /* arg is an unknown option */
-   argv[left++] = arg;
-   continue;
+   maybe_opt = 1;
}
 
 
if (!handle_revision_arg(arg, revs, flags, revarg_opt))
got_rev_arg = 1;
-   else {
+   else if (maybe_opt) {
+   /* arg is an unknown option */
+   argv[left++] = arg;
+   continue;
+   } else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
-- 
2.1.4

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

[Siddharth Kannan]

This patch introduces "-" as a method to refer to a revision, and adds tests to
test that git-log works with this shorthand.

This change will touch the following commands (through
revision.c:setup_revisions):

* builtin/blame.c
* builtin/diff.c
* builtin/diff-files.c
* builtin/diff-index.c
* builtin/diff-tree.c
* builtin/log.c
* builtin/rev-list.c
* builtin/shortlog.c
* builtin/fast-export.c
* builtin/fmt-merge-msg.c
builtin/add.c
builtin/checkout.c
builtin/commit.c
builtin/merge.c
builtin/pack-objects.c
builtin/revert.c

* marked commands are information-only.

As most commands in this list are not of the rm-variety, (i.e a command that
would delete something), this change does not make it easier for people to
delete. (eg: "git branch -d -" is *not* enabled by this patch)

Signed-off-by: Siddharth Kannan 
---

Instead of replacing the whole string, we would expand it accordingly using:

if (*name == '-') {
  if (len == 1) {
name = "@{-1}";
len = 5;
  } else {
struct strbuf changed_argument = STRBUF_INIT;

strbuf_addstr(_argument, "@{-1}");
strbuf_addstr(_argument, name + 1);

strbuf_setlen(_argument, strlen(name) + 4);

name = strbuf_detach(_argument, NULL);
  }
}

Junio's comments on a previous version of the patch which used this same
approach but inside setup_revisions [1]

[1]: 

 sha1_name.c  |   5 +++
 t/t4214-log-shorthand.sh | 106 +++
 2 files changed, 111 insertions(+)
 create mode 100755 t/t4214-log-shorthand.sh

diff --git a/sha1_name.c b/sha1_name.c
index 73a915f..2f86bc9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned 
char *sha1, unsigned l
if (!ret)
return 0;
 
+   if (*name == '-' && len == 1) {
+   name = "@{-1}";
+   len = 5;
+   }
+
ret = get_sha1_basic(name, len, sha1, lookup_flags);
if (!ret)
return 0;
diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
new file mode 100755
index 000..659b100
--- /dev/null
+++ b/t/t4214-log-shorthand.sh
@@ -0,0 +1,106 @@
+#!/bin/sh
+
+test_description='log can show previous branch using shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+   test_commit first &&
+   test_commit second &&
+   test_commit third &&
+   test_commit fourth &&
+   test_commit fifth &&
+   test_commit sixth &&
+   test_commit seventh
+'
+
+test_expect_success '"log -" should not work initially' '
+   test_must_fail git log -
+'
+
+test_expect_success 'setup branches for testing' '
+   git checkout -b testing-1 master^ &&
+   git checkout -b testing-2 master~2 &&
+   git checkout master
+'
+
+test_expect_success '"log -" should work' '
+   git log testing-2 >expect &&
+   git log - >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'symmetric revision range should work when one end is left 
empty' '
+   git checkout testing-2 &&
+   git checkout master &&
+   git log ...@{-1} >expect.first_empty &&
+   git log @{-1}... >expect.last_empty &&
+   git log ...- >actual.first_empty &&
+   git log -... >actual.last_empty &&
+   test_cmp expect.first_empty actual.first_empty &&
+   test_cmp expect.last_empty actual.last_empty
+'
+
+test_expect_success 'asymmetric revision range should work when one end is 
left empty' '
+   git checkout testing-2 &&
+   git checkout master &&
+   git log ..@{-1} >expect.first_empty &&
+   git log @{-1}.. >expect.last_empty &&
+   git log ..- >actual.first_empty &&
+   git log -.. >actual.last_empty &&
+   test_cmp expect.first_empty actual.first_empty &&
+   test_cmp expect.last_empty actual.last_empty
+'
+
+test_expect_success 'symmetric revision range should work when both ends are 
given' '
+   git checkout testing-2 &&
+   git checkout master &&
+   git log -...testing-1 >expect &&
+   git log testing-2...testing-1 >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'asymmetric revision range should work when both ends are 
given' '
+   git checkout testing-2 &&
+   git checkout master &&
+   git log -..testing-1 >expect &&
+   git log testing-2..testing-1 >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'multiple separate arguments should be handled properly' '
+   git checkout testing-2 &&
+   git checkout master &&
+   git log - - >expect.1 &&
+   git log @{-1} @{-1} >actual.1 &&
+   git log - HEAD >expect.2 &&
+   git log @{-1} HEAD >actual.2 &&
+   test_cmp expect.1 actual.1 &&
+   test_cmp expect.2 actual.2
+'
+
+test_expect_success 'revision ranges with same start and end should be empty' '
+   git checkout testing-2 &&
+   git checkout master &&

[PATCH 2/4 v4] revision.c: swap if/else blocks

[Siddharth Kannan]

Swap the condition and bodies of an "if (A) do_A else do_B" in
setup_revisions() to "if (!A) do_B else do_A", to make the change in
the the next step easier to read.

No behaviour change is intended in this step.

Signed-off-by: Siddharth Kannan 
---
 revision.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index 5674a9a..8d4ddae 100644
--- a/revision.c
+++ b/revision.c
@@ -2238,7 +2238,9 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
}
 
 
-   if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+   if (!handle_revision_arg(arg, revs, flags, revarg_opt))
+   got_rev_arg = 1;
+   else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
@@ -2255,8 +2257,6 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
append_prune_data(_data, argv + i);
break;
}
-   else
-   got_rev_arg = 1;
}
 
if (prune_data.nr) {
-- 
2.1.4

Re: [PATCH] show-branch: fix crash with long ref name

[Christian Couder]

On Wed, Feb 15, 2017 at 10:40 PM, Jeff King  wrote:
> On Tue, Feb 14, 2017 at 10:29:46PM +0100, Christian Couder wrote:
>
>> > I notice Christian's patch added a few tests. I don't know if we'd want
>> > to squash them in (I didn't mean to override his patch at all; I was
>> > about to send mine out when I noticed his, and I wondered if we wanted
>> > to combine the two efforts).
>>
>> I think it would be nice to have at least one test. Feel free to
>> squash mine if you want.
>
> I started to add some tests, but I had second thoughts. It _is_ nice
> to show off the fix, but as far as regressions go, this specific case is
> unlikely to come up again. What would be more valuable, I think, is a
> test script which set up a very long refname (not just 150 bytes or
> whatever) and ran it through a series of git commands.

I agree that a test script running through a series of command with
long refnames would be great.

But I think the refname should not necesarily be too long. As I wrote
in the commit message of my patch, if the ref name had been much
longer the crash would not have happened because the ref could not
have been created in the first place.

So the best would be to run through a series of commands with a
refname ranging from let's say 80 chars to 300 chars.

That would have a chance to catch crashes due to legacy code using for
example things like `char stuff[128]` or `char stuff[256]`.

Implementing those tests could have started with something like the
test case I sent, but as it would in the end be about many different
commands, one can see it as part of a different topic.

> But then you run into all sorts of portability annoyances with pathname
> restrictions (you can hack around creation by writing the refname
> directly into packed-refs, but most manipulations will want to take the
> .lock in the filesystem).

Yeah, but if a crash doesn't happen because we die() as the ref is too
long for the file system, we could detect that and make the test
succeed.

> So I dunno. It seems like being thorough is a
> lot of hassle for not much gain. Being not-thorough is easy, but is
> mostly a token that is unlikely to find any real bugs.

Yeah, if we really care, it might be better to start using a fuzzer or
a property based testing tool instead of bothering with these kind of
tests by ourselves, which is also a different topic.

> So I punted, at least for now.

Ok, no problem.

[PATCH v2 5/5] refs: kill set_worktree_head_symref()

[Nguyễn Thái Ngọc Duy]

70999e9cec (branch -m: update all per-worktree HEADs - 2016-03-27)
added this function in order to update HEADs of all relevant
worktrees, when a branch is renamed.

It, as a public ref api, kind of breaks abstraction when it uses
internal functions of files backend. With the introduction of
refs_create_symref(), we can move back pretty close to the code before
70999e9cec, where create_symref() was used for updating HEAD.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 branch.c | 12 ++--
 refs.h   |  9 -
 refs/files-backend.c | 41 -
 3 files changed, 6 insertions(+), 56 deletions(-)

diff --git a/branch.c b/branch.c
index fcb4a765c..ad0cc0489 100644
--- a/branch.c
+++ b/branch.c
@@ -352,18 +352,18 @@ int replace_each_worktree_head_symref(const char *oldref, 
const char *newref)
int i;
 
for (i = 0; worktrees[i]; i++) {
+   struct ref_store *refs;
+
if (worktrees[i]->is_detached)
continue;
if (worktrees[i]->head_ref &&
strcmp(oldref, worktrees[i]->head_ref))
continue;
 
-   if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
-newref)) {
-   ret = -1;
-   error(_("HEAD of working tree %s is not updated"),
- worktrees[i]->path);
-   }
+   refs = get_worktree_ref_store(worktrees[i]);
+   if (refs_create_symref(refs, "HEAD", newref, NULL))
+   ret = error(_("HEAD of working tree %s is not updated"),
+   worktrees[i]->path);
}
 
free_worktrees(worktrees);
diff --git a/refs.h b/refs.h
index 694769963..bce77891a 100644
--- a/refs.h
+++ b/refs.h
@@ -325,15 +325,6 @@ int rename_ref(const char *oldref, const char *newref, 
const char *logmsg);
 
 int create_symref(const char *refname, const char *target, const char *logmsg);
 
-/*
- * Update HEAD of the specified gitdir.
- * Similar to create_symref("relative-git-dir/HEAD", target, NULL), but
- * this can update the main working tree's HEAD regardless of where
- * $GIT_DIR points to.
- * Return 0 if successful, non-zero otherwise.
- * */
-int set_worktree_head_symref(const char *gitdir, const char *target);
-
 enum action_on_err {
UPDATE_REFS_MSG_ON_ERR,
UPDATE_REFS_DIE_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f3be620ab..ba56e46d4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3108,47 +3108,6 @@ static int files_create_symref(struct ref_store 
*ref_store,
return ret;
 }
 
-int set_worktree_head_symref(const char *gitdir, const char *target)
-{
-   /*
-* FIXME: this obviously will not work well for future refs
-* backends. This function needs to die.
-*/
-   struct files_ref_store *refs =
-   files_downcast(get_main_ref_store(), "set_head_symref");
-   static struct lock_file head_lock;
-   struct ref_lock *lock;
-   struct strbuf head_path = STRBUF_INIT;
-   const char *head_rel;
-   int ret;
-
-   strbuf_addf(_path, "%s/HEAD", absolute_path(gitdir));
-   if (hold_lock_file_for_update(_lock, head_path.buf,
- LOCK_NO_DEREF) < 0) {
-   struct strbuf err = STRBUF_INIT;
-   unable_to_lock_message(head_path.buf, errno, );
-   error("%s", err.buf);
-   strbuf_release();
-   strbuf_release(_path);
-   return -1;
-   }
-
-   /* head_rel will be "HEAD" for the main tree, "worktrees/wt/HEAD" for
-  linked trees */
-   head_rel = remove_leading_path(head_path.buf,
-  absolute_path(get_git_common_dir()));
-   /* to make use of create_symref_locked(), initialize ref_lock */
-   lock = xcalloc(1, sizeof(struct ref_lock));
-   lock->lk = _lock;
-   lock->ref_name = xstrdup(head_rel);
-
-   ret = create_symref_locked(refs, lock, head_rel, target, NULL);
-
-   unlock_ref(lock); /* will free lock */
-   strbuf_release(_path);
-   return ret;
-}
-
 static int files_reflog_exists(struct ref_store *ref_store,
   const char *refname)
 {
-- 
2.11.0.157.gd943d85

[PATCH v2 1/5] refs: introduce get_worktree_ref_store()

[Nguyễn Thái Ngọc Duy]

files-backend at this point is still aware of the per-repo/worktree
separation in refs, so it can handle a linked worktree.

Note: accessing a worktree of a submodule remains unaddressed. Perhaps
after get_worktrees() can access submodule (or rather a new function
get_submodule_worktrees(), that lists worktrees of a submodule), we can
update this function to work with submodules as well.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 27 +++
 refs.h |  2 ++
 2 files changed, 29 insertions(+)

diff --git a/refs.c b/refs.c
index e7206a420..ba4d9420c 100644
--- a/refs.c
+++ b/refs.c
@@ -10,6 +10,7 @@
 #include "object.h"
 #include "tag.h"
 #include "submodule.h"
+#include "worktree.h"
 
 /*
  * List of all available backends
@@ -1486,6 +1487,32 @@ struct ref_store *get_submodule_ref_store(const char 
*submodule)
return refs;
 }
 
+struct ref_store *get_worktree_ref_store(const struct worktree *wt)
+{
+   struct ref_store *refs;
+
+   if (wt->is_current)
+   return get_main_ref_store();
+
+   /*
+* We share the same hash map with submodules for
+* now. submodule paths are always relative (to topdir) while
+* worktree paths are always absolute. No chance of conflict.
+*/
+   refs = lookup_submodule_ref_store(wt->path);
+   if (refs)
+   return refs;
+
+   if (wt->id)
+   refs = ref_store_init(git_common_path("worktrees/%s", wt->id));
+   else
+   refs = ref_store_init(get_git_common_dir());
+
+   if (refs)
+   register_submodule_ref_store(refs, wt->path);
+   return refs;
+}
+
 void base_ref_store_init(struct ref_store *refs,
 const struct ref_storage_be *be)
 {
diff --git a/refs.h b/refs.h
index 1287ba59c..464cc384a 100644
--- a/refs.h
+++ b/refs.h
@@ -565,5 +565,7 @@ struct ref_store *get_main_ref_store(void);
  * submodule==NULL.
  */
 struct ref_store *get_submodule_ref_store(const char *submodule);
+struct worktree;
+struct ref_store *get_worktree_ref_store(const struct worktree *wt);
 
 #endif /* REFS_H */
-- 
2.11.0.157.gd943d85

[PATCH v2 0/5] Kill manual ref parsing code in worktree.c

[Nguyễn Thái Ngọc Duy]

v2 still kills parse_ref(), but the series now depends on my other
series [1] and as a result does not make any confusing "submodule"
calls any more.

It also kills another function, introduced for multi-worktree, that
should not have been there to begin with. Good riddance.

Again, the naming convention with refs_ prefix for new APIs may not be
the best idea...

[1] public-inbox.org/git/20170213152011.12050-1-pclo...@gmail.com

Nguyễn Thái Ngọc Duy (5):
  refs: introduce get_worktree_ref_store()
  refs.c: add refs_resolve_ref_unsafe()
  worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
  refs: add refs_create_symref()
  refs: kill set_worktree_head_symref()

 branch.c |  15 
 refs.c   |  58 -
 refs.h   |  21 ++-
 refs/files-backend.c |  43 +-
 refs/refs-internal.h |   5 ---
 worktree.c   | 102 ++-
 worktree.h   |   2 +-
 7 files changed, 98 insertions(+), 148 deletions(-)

-- 
2.11.0.157.gd943d85

[PATCH v2 3/5] worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()

[Nguyễn Thái Ngọc Duy]

The manual parsing code is replaced with a call to refs_resolve_ref_unsafe().
The manual parsing code must die because only refs/files-backend.c
should do that.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 branch.c   |   3 +-
 worktree.c | 102 -
 worktree.h |   2 +-
 3 files changed, 30 insertions(+), 77 deletions(-)

diff --git a/branch.c b/branch.c
index c431cbf6a..fcb4a765c 100644
--- a/branch.c
+++ b/branch.c
@@ -354,7 +354,8 @@ int replace_each_worktree_head_symref(const char *oldref, 
const char *newref)
for (i = 0; worktrees[i]; i++) {
if (worktrees[i]->is_detached)
continue;
-   if (strcmp(oldref, worktrees[i]->head_ref))
+   if (worktrees[i]->head_ref &&
+   strcmp(oldref, worktrees[i]->head_ref))
continue;
 
if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
diff --git a/worktree.c b/worktree.c
index eb6121263..f8f520036 100644
--- a/worktree.c
+++ b/worktree.c
@@ -19,54 +19,25 @@ void free_worktrees(struct worktree **worktrees)
free (worktrees);
 }
 
-/*
- * read 'path_to_ref' into 'ref'.  Also if is_detached is not NULL,
- * set is_detached to 1 (0) if the ref is detached (is not detached).
- *
- * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so
- * for linked worktrees, `resolve_ref_unsafe()` won't work (it uses
- * git_path). Parse the ref ourselves.
- *
- * return -1 if the ref is not a proper ref, 0 otherwise (success)
- */
-static int parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
-{
-   if (is_detached)
-   *is_detached = 0;
-   if (!strbuf_readlink(ref, path_to_ref, 0)) {
-   /* HEAD is symbolic link */
-   if (!starts_with(ref->buf, "refs/") ||
-   check_refname_format(ref->buf, 0))
-   return -1;
-   } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
-   /* textual symref or detached */
-   if (!starts_with(ref->buf, "ref:")) {
-   if (is_detached)
-   *is_detached = 1;
-   } else {
-   strbuf_remove(ref, 0, strlen("ref:"));
-   strbuf_trim(ref);
-   if (check_refname_format(ref->buf, 0))
-   return -1;
-   }
-   } else
-   return -1;
-   return 0;
-}
-
 /**
- * Add the head_sha1 and head_ref (if not detached) to the given worktree
+ * Update head_sha1, head_ref and is_detached of the given worktree
  */
-static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)
+static void add_head_info(struct worktree *wt)
 {
-   if (head_ref->len) {
-   if (worktree->is_detached) {
-   get_sha1_hex(head_ref->buf, worktree->head_sha1);
-   } else {
-   resolve_ref_unsafe(head_ref->buf, 0, 
worktree->head_sha1, NULL);
-   worktree->head_ref = strbuf_detach(head_ref, NULL);
-   }
-   }
+   int flags;
+   const char *target;
+
+   target = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
+"HEAD",
+RESOLVE_REF_READING,
+wt->head_sha1, );
+   if (!target)
+   return;
+
+   if (flags & REF_ISSYMREF)
+   wt->head_ref = xstrdup(target);
+   else
+   wt->is_detached = 1;
 }
 
 /**
@@ -77,9 +48,7 @@ static struct worktree *get_main_worktree(void)
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
struct strbuf worktree_path = STRBUF_INIT;
-   struct strbuf head_ref = STRBUF_INIT;
int is_bare = 0;
-   int is_detached = 0;
 
strbuf_add_absolute_path(_path, get_git_common_dir());
is_bare = !strbuf_strip_suffix(_path, "/.git");
@@ -91,13 +60,10 @@ static struct worktree *get_main_worktree(void)
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(_path, NULL);
worktree->is_bare = is_bare;
-   worktree->is_detached = is_detached;
-   if (!parse_ref(path.buf, _ref, _detached))
-   add_head_info(_ref, worktree);
+   add_head_info(worktree);
 
strbuf_release();
strbuf_release(_path);
-   strbuf_release(_ref);
return worktree;
 }
 
@@ -106,8 +72,6 @@ static struct worktree *get_linked_worktree(const char *id)
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
struct strbuf worktree_path = STRBUF_INIT;
-   struct strbuf head_ref = STRBUF_INIT;
-   int is_detached = 0;
 
if (!id)
die("Missing linked worktree name");

[PATCH v2 2/5] refs.c: add refs_resolve_ref_unsafe()

[Nguyễn Thái Ngọc Duy]

Surprise surprise. This is just rename and introduce
resolve_ref_recursively() to the public.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 9 +
 refs.h   | 6 ++
 refs/files-backend.c | 2 +-
 refs/refs-internal.h | 5 -
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index ba4d9420c..e0e191107 100644
--- a/refs.c
+++ b/refs.c
@@ -1226,7 +1226,7 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 }
 
 /* This function needs to return a meaningful errno on failure */
-const char *resolve_ref_recursively(struct ref_store *refs,
+const char *refs_resolve_ref_unsafe(struct ref_store *refs,
const char *refname,
int resolve_flags,
unsigned char *sha1, int *flags)
@@ -1313,8 +1313,9 @@ int refs_init_db(struct strbuf *err)
 const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
   unsigned char *sha1, int *flags)
 {
-   return resolve_ref_recursively(get_main_ref_store(), refname,
-  resolve_flags, sha1, flags);
+   return refs_resolve_ref_unsafe(get_main_ref_store(),
+  refname, resolve_flags,
+  sha1, flags);
 }
 
 int resolve_gitlink_ref(const char *submodule, const char *refname,
@@ -1343,7 +1344,7 @@ int resolve_gitlink_ref(const char *submodule, const char 
*refname,
if (!refs)
return -1;
 
-   if (!resolve_ref_recursively(refs, refname, 0, sha1, ) ||
+   if (!refs_resolve_ref_unsafe(refs, refname, 0, sha1, ) ||
is_null_sha1(sha1))
return -1;
return 0;
diff --git a/refs.h b/refs.h
index 464cc384a..10c2cfc00 100644
--- a/refs.h
+++ b/refs.h
@@ -568,4 +568,10 @@ struct ref_store *get_submodule_ref_store(const char 
*submodule);
 struct worktree;
 struct ref_store *get_worktree_ref_store(const struct worktree *wt);
 
+const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+   const char *refname,
+   int resolve_flags,
+   unsigned char *sha1,
+   int *flags);
+
 #endif /* REFS_H */
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 685ea5c14..f3be620ab 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1276,7 +1276,7 @@ static void read_loose_refs(const char *dirname, struct 
ref_dir *dir)
 create_dir_entry(refs, refname.buf,
  refname.len, 1));
} else {
-   if (!resolve_ref_recursively(>base,
+   if (!refs_resolve_ref_unsafe(>base,
 refname.buf,
 RESOLVE_REF_READING,
 sha1, )) {
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index cb6882779..6b29dc3b1 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -634,11 +634,6 @@ struct ref_store {
 void base_ref_store_init(struct ref_store *refs,
 const struct ref_storage_be *be);
 
-const char *resolve_ref_recursively(struct ref_store *refs,
-   const char *refname,
-   int resolve_flags,
-   unsigned char *sha1, int *flags);
-
 static inline int is_per_worktree_ref(const char *refname)
 {
return !starts_with(refname, "refs/") ||
-- 
2.11.0.157.gd943d85

[PATCH v2 4/5] refs: add refs_create_symref()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 22 +-
 refs.h |  4 
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index e0e191107..23e0a8eda 100644
--- a/refs.c
+++ b/refs.c
@@ -1535,13 +1535,25 @@ int peel_ref(const char *refname, unsigned char *sha1)
return refs->be->peel_ref(refs, refname, sha1);
 }
 
-int create_symref(const char *ref_target, const char *refs_heads_master,
+int refs_create_symref(struct ref_store *refs,
+  const char *ref_target,
+  const char *refs_heads_master,
+  const char *logmsg)
+{
+   return refs->be->create_symref(refs,
+  ref_target,
+  refs_heads_master,
+  logmsg);
+}
+
+int create_symref(const char *ref_target,
+ const char *refs_heads_master,
  const char *logmsg)
 {
-   struct ref_store *refs = get_main_ref_store();
-
-   return refs->be->create_symref(refs, ref_target, refs_heads_master,
-  logmsg);
+   return refs_create_symref(get_main_ref_store(),
+ ref_target,
+ refs_heads_master,
+ logmsg);
 }
 
 int ref_transaction_commit(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 10c2cfc00..694769963 100644
--- a/refs.h
+++ b/refs.h
@@ -573,5 +573,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
int resolve_flags,
unsigned char *sha1,
int *flags);
+int refs_create_symref(struct ref_store *refs,
+  const char *refname,
+  const char *target,
+  const char *logmsg);
 
 #endif /* REFS_H */
-- 
2.11.0.157.gd943d85

[PATCH v2 15/16] files-backend: remove submodule_allowed from files_downcast()

[Nguyễn Thái Ngọc Duy]

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 
---
 refs/files-backend.c | 70 
 1 file changed, 21 insertions(+), 49 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index e8946e638..d9fc29d8d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1017,24 +1017,13 @@ static struct ref_store *files_ref_store_create(const 
char *gitdir)
 }
 
 /*
- * Die if refs is for a submodule (i.e., not for the main repository).
- * caller is used in any necessary error messages.
- */
-static void files_assert_main_repository(struct files_ref_store *refs,
-const char *caller)
-{
-   /* This function is to be deleted in the next patch */
-}
-
-/*
  * Downcast ref_store to files_ref_store. Die if ref_store is not a
  * files_ref_store. If submodule_allowed is not true, then also die if
  * files_ref_store is for a submodule (i.e., not for the main
  * repository). caller is used in any necessary error messages.
  */
-static struct files_ref_store *files_downcast(
-   struct ref_store *ref_store, int submodule_allowed,
-   const char *caller)
+static struct files_ref_store *files_downcast(struct ref_store *ref_store,
+ const char *caller)
 {
struct files_ref_store *refs;
 
@@ -1044,9 +1033,6 @@ static struct files_ref_store *files_downcast(
 
refs = (struct files_ref_store *)ref_store;
 
-   if (!submodule_allowed)
-   files_assert_main_repository(refs, caller);
-
return refs;
 }
 
@@ -1382,7 +1368,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
  struct strbuf *referent, unsigned int *type)
 {
struct files_ref_store *refs =
-   files_downcast(ref_store, 1, "read_raw_ref");
+   files_downcast(ref_store, "read_raw_ref");
struct strbuf sb_contents = STRBUF_INIT;
struct strbuf sb_path = STRBUF_INIT;
const char *path;
@@ -1575,7 +1561,6 @@ static int lock_raw_ref(struct files_ref_store *refs,
int ret = TRANSACTION_GENERIC_ERROR;
 
assert(err);
-   files_assert_main_repository(refs, "lock_raw_ref");
 
*type = 0;
 
@@ -1799,7 +1784,7 @@ static enum peel_status peel_entry(struct ref_entry 
*entry, int repeel)
 static int files_peel_ref(struct ref_store *ref_store,
  const char *refname, unsigned char *sha1)
 {
-   struct files_ref_store *refs = files_downcast(ref_store, 0, "peel_ref");
+   struct files_ref_store *refs = files_downcast(ref_store, "peel_ref");
int flag;
unsigned char base[20];
 
@@ -1908,7 +1893,7 @@ static struct ref_iterator *files_ref_iterator_begin(
const char *prefix, unsigned int flags)
 {
struct files_ref_store *refs =
-   files_downcast(ref_store, 1, "ref_iterator_begin");
+   files_downcast(ref_store, "ref_iterator_begin");
struct ref_dir *loose_dir, *packed_dir;
struct ref_iterator *loose_iter, *packed_iter;
struct files_ref_iterator *iter;
@@ -2041,7 +2026,6 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
int attempts_remaining = 3;
int resolved;
 
-   files_assert_main_repository(refs, "lock_ref_sha1_basic");
assert(err);
 
lock = xcalloc(1, sizeof(struct ref_lock));
@@ -2189,8 +2173,6 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
struct strbuf sb = STRBUF_INIT;
int ret;
 
-   files_assert_main_repository(refs, "lock_packed_refs");
-
if (!timeout_configured) {
git_config_get_int("core.packedrefstimeout", _value);
timeout_configured = 1;
@@ -2230,8 +2212,6 @@ static int commit_packed_refs(struct files_ref_store 
*refs)
int save_errno = 0;
FILE *out;
 
-   files_assert_main_repository(refs, "commit_packed_refs");
-
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
 
@@ -2263,8 +2243,6 @@ static void rollback_packed_refs(struct files_ref_store 
*refs)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(refs);
 
-   files_assert_main_repository(refs, "rollback_packed_refs");
-
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
rollback_lock_file(packed_ref_cache->lock);
@@ -2410,7 +2388,7 @@ static void prune_refs(struct files_ref_store *refs, 
struct ref_to_prune *r)
 static int files_pack_refs(struct ref_store 

[PATCH v2 14/16] refs: move submodule code out of files-backend.c

[Nguyễn Thái Ngọc Duy]

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   | 18 +-
 refs/files-backend.c | 26 --
 refs/refs-internal.h |  6 +++---
 3 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/refs.c b/refs.c
index 25f657a6f..58ac9a2a8 100644
--- a/refs.c
+++ b/refs.c
@@ -9,6 +9,7 @@
 #include "refs/refs-internal.h"
 #include "object.h"
 #include "tag.h"
+#include "submodule.h"
 
 /*
  * List of all available backends
@@ -1419,9 +1420,9 @@ static void register_submodule_ref_store(struct ref_store 
*refs,
 
 /*
  * Create, record, and return a ref_store instance for the specified
- * submodule (or the main repository if submodule is NULL).
+ * gitdir (or the main repository if gitdir is NULL).
  */
-static struct ref_store *ref_store_init(const char *submodule)
+static struct ref_store *ref_store_init(const char *gitdir)
 {
const char *be_name = "files";
struct ref_storage_be *be = find_ref_storage_backend(be_name);
@@ -1430,7 +1431,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);
return refs;
 }
 
@@ -1455,6 +1456,7 @@ struct ref_store *get_ref_store(const char *submodule)
 {
struct strbuf submodule_sb = STRBUF_INIT;
struct ref_store *refs;
+   int ret;
 
if (!submodule || !*submodule) {
return get_main_ref_store();
@@ -1465,8 +1467,14 @@ struct ref_store *get_ref_store(const char *submodule)
return refs;
 
strbuf_addstr(_sb, submodule);
-   if (is_nonbare_repository_dir(_sb))
-   refs = ref_store_init(submodule);
+   ret = is_nonbare_repository_dir(_sb);
+   strbuf_release(_sb);
+   if (!ret)
+   return refs;
+
+   ret = submodule_to_gitdir(_sb, submodule);
+   if (!ret)
+   refs = ref_store_init(submodule_sb.buf);
strbuf_release(_sb);
 
if (refs)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 74e31d041..e8946e638 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -917,13 +917,6 @@ struct packed_ref_cache {
 struct files_ref_store {
struct ref_store base;
 
-   /*
-* The name of the submodule represented by this object, or
-* NULL if it represents the main repository's reference
-* store:
-*/
-   const char *submodule;
-
struct strbuf gitdir;
struct strbuf gitcommondir;
 
@@ -945,12 +938,9 @@ static void files_path(struct files_ref_store *refs, 
struct strbuf *sb,
va_start(vap, fmt);
strbuf_vaddf(, fmt, vap);
va_end(vap);
-   if (refs->submodule)
-   strbuf_git_path_submodule(sb, refs->submodule,
- "%s", tmp.buf);
-   else if (is_per_worktree_ref(tmp.buf) ||
-(skip_prefix(tmp.buf, "logs/", ) &&
- is_per_worktree_ref(ref)))
+   if (is_per_worktree_ref(tmp.buf) ||
+   (skip_prefix(tmp.buf, "logs/", ) &&
+is_per_worktree_ref(ref)))
strbuf_addf(sb, "%s/%s", refs->gitdir.buf, tmp.buf);
else
strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
@@ -1005,7 +995,7 @@ static void clear_loose_ref_cache(struct files_ref_store 
*refs)
  * Create a new submodule ref cache and add it to the internal
  * set of caches.
  */
-static struct ref_store *files_ref_store_create(const char *submodule)
+static struct ref_store *files_ref_store_create(const char *gitdir)
 {
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
@@ -1015,8 +1005,9 @@ static struct ref_store *files_ref_store_create(const 
char *submodule)
strbuf_init(>gitdir, 0);
strbuf_init(>gitcommondir, 0);
 
-   if (submodule) {
-   refs->submodule = xstrdup(submodule);
+   if (gitdir) {
+   strbuf_addstr(>gitdir, gitdir);
+   get_common_dir_noenv(>gitcommondir, gitdir);
} else {
strbuf_addstr(>gitdir, get_git_dir());
strbuf_addstr(>gitcommondir, get_git_common_dir());
@@ -1032,8 +1023,7 @@ static struct ref_store *files_ref_store_create(const 
char *submodule)
 static void 

[PATCH v2 16/16] refs: rename get_ref_store() to get_submodule_ref_store() and make it public

[Nguyễn Thái Ngọc Duy]

This function is intended to replace *_submodule() refs API. It provides
a ref store for a specific submodule, which can be operated on by a new
set of refs API.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 12 
 refs.h   | 11 +++
 refs/files-backend.c |  2 +-
 refs/refs-internal.h | 12 
 4 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/refs.c b/refs.c
index 58ac9a2a8..e7206a420 100644
--- a/refs.c
+++ b/refs.c
@@ -1160,7 +1160,7 @@ int head_ref(each_ref_fn fn, void *cb_data)
 static int do_for_each_ref(const char *submodule, const char *prefix,
   each_ref_fn fn, int trim, int flags, void *cb_data)
 {
-   struct ref_store *refs = get_ref_store(submodule);
+   struct ref_store *refs = get_submodule_ref_store(submodule);
struct ref_iterator *iter;
 
if (!refs)
@@ -1333,10 +1333,10 @@ int resolve_gitlink_ref(const char *submodule, const 
char *refname,
/* We need to strip off one or more trailing slashes */
char *stripped = xmemdupz(submodule, len);
 
-   refs = get_ref_store(stripped);
+   refs = get_submodule_ref_store(stripped);
free(stripped);
} else {
-   refs = get_ref_store(submodule);
+   refs = get_submodule_ref_store(submodule);
}
 
if (!refs)
@@ -1452,13 +1452,17 @@ struct ref_store *get_main_ref_store(void)
return refs;
 }
 
-struct ref_store *get_ref_store(const char *submodule)
+struct ref_store *get_submodule_ref_store(const char *submodule)
 {
struct strbuf submodule_sb = STRBUF_INIT;
struct ref_store *refs;
int ret;
 
if (!submodule || !*submodule) {
+   /*
+* FIXME: This case is ideally not allowed. But that
+* can't happen until we clean up all the callers.
+*/
return get_main_ref_store();
}
 
diff --git a/refs.h b/refs.h
index f803528fc..1287ba59c 100644
--- a/refs.h
+++ b/refs.h
@@ -554,5 +554,16 @@ int reflog_expire(const char *refname, const unsigned char 
*sha1,
 int ref_storage_backend_exists(const char *name);
 
 struct ref_store *get_main_ref_store(void);
+/*
+ * Return the ref_store instance for the specified submodule. For the
+ * main repository, use submodule==NULL; such a call cannot fail. 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);
 
 #endif /* REFS_H */
diff --git a/refs/files-backend.c b/refs/files-backend.c
index d9fc29d8d..685ea5c14 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3115,7 +3115,7 @@ int set_worktree_head_symref(const char *gitdir, const 
char *target)
 * backends. This function needs to die.
 */
struct files_ref_store *refs =
-   files_downcast(get_ref_store(NULL), "set_head_symref");
+   files_downcast(get_main_ref_store(), "set_head_symref");
static struct lock_file head_lock;
struct ref_lock *lock;
struct strbuf head_path = STRBUF_INIT;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index d7112770d..cb6882779 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -634,18 +634,6 @@ struct ref_store {
 void base_ref_store_init(struct ref_store *refs,
 const struct ref_storage_be *be);
 
-/*
- * Return the ref_store instance for the specified submodule. For the
- * main repository, use submodule==NULL; such a call cannot fail. 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_ref_store(const char *submodule);
-
 const char *resolve_ref_recursively(struct ref_store *refs,
const char *refname,
int resolve_flags,
-- 
2.11.0.157.gd943d85

[PATCH v2 12/16] refs.c: make get_main_ref_store() public and use it

[Nguyễn Thái Ngọc Duy]

get_ref_store() will soon be renamed to get_submodule_ref_store().
Together with future get_worktree_ref_store(), the three functions
provide an appropriate ref store for different operation modes. New APIs
will be added to operate directly on ref stores.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 36 ++--
 refs.h |  2 ++
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/refs.c b/refs.c
index 55a80a83d..25f657a6f 100644
--- a/refs.c
+++ b/refs.c
@@ -1303,7 +1303,7 @@ const char *resolve_ref_recursively(struct ref_store 
*refs,
 /* backend functions */
 int refs_init_db(struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->init_db(refs, err);
 }
@@ -1311,7 +1311,7 @@ int refs_init_db(struct strbuf *err)
 const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
   unsigned char *sha1, int *flags)
 {
-   return resolve_ref_recursively(get_ref_store(NULL), refname,
+   return resolve_ref_recursively(get_main_ref_store(), refname,
   resolve_flags, sha1, flags);
 }
 
@@ -1434,7 +1434,7 @@ static struct ref_store *ref_store_init(const char 
*submodule)
return refs;
 }
 
-static struct ref_store *get_main_ref_store(void)
+struct ref_store *get_main_ref_store(void)
 {
struct ref_store *refs;
 
@@ -1483,14 +1483,14 @@ void base_ref_store_init(struct ref_store *refs,
 /* backend functions */
 int pack_refs(unsigned int flags)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->pack_refs(refs, flags);
 }
 
 int peel_ref(const char *refname, unsigned char *sha1)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->peel_ref(refs, refname, sha1);
 }
@@ -1498,7 +1498,7 @@ int peel_ref(const char *refname, unsigned char *sha1)
 int create_symref(const char *ref_target, const char *refs_heads_master,
  const char *logmsg)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->create_symref(refs, ref_target, refs_heads_master,
   logmsg);
@@ -1507,7 +1507,7 @@ int create_symref(const char *ref_target, const char 
*refs_heads_master,
 int ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->transaction_commit(refs, transaction, err);
 }
@@ -1517,14 +1517,14 @@ int verify_refname_available(const char *refname,
 const struct string_list *skip,
 struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->verify_refname_available(refs, refname, extra, skip, 
err);
 }
 
 int for_each_reflog(each_ref_fn fn, void *cb_data)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
struct ref_iterator *iter;
 
iter = refs->be->reflog_iterator_begin(refs);
@@ -1535,7 +1535,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn,
void *cb_data)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->for_each_reflog_ent_reverse(refs, refname,
 fn, cb_data);
@@ -1544,14 +1544,14 @@ int for_each_reflog_ent_reverse(const char *refname, 
each_reflog_ent_fn fn,
 int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn,
void *cb_data)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->for_each_reflog_ent(refs, refname, fn, cb_data);
 }
 
 int reflog_exists(const char *refname)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->reflog_exists(refs, refname);
 }
@@ -1559,14 +1559,14 @@ int reflog_exists(const char *refname)
 int safe_create_reflog(const char *refname, int force_create,
   struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->create_reflog(refs, refname, force_create, err);
 }
 
 int delete_reflog(const char *refname)
 {
-   struct ref_store *refs = 

[PATCH v2 13/16] path.c: move some code out of strbuf_git_path_submodule()

[Nguyễn Thái Ngọc Duy]

refs is learning to avoid path rewriting that is done by
strbuf_git_path_submodule(). Factor out this code so it could be reused
by refs*

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 path.c  | 34 +++---
 submodule.c | 31 +++
 submodule.h |  1 +
 3 files changed, 39 insertions(+), 27 deletions(-)

diff --git a/path.c b/path.c
index efcedafba..3451d2916 100644
--- a/path.c
+++ b/path.c
@@ -475,35 +475,16 @@ const char *worktree_git_path(const struct worktree *wt, 
const char *fmt, ...)
 static int do_submodule_path(struct strbuf *buf, const char *path,
 const char *fmt, va_list args)
 {
-   const char *git_dir;
struct strbuf git_submodule_common_dir = STRBUF_INIT;
struct strbuf git_submodule_dir = STRBUF_INIT;
-   const struct submodule *sub;
-   int err = 0;
+   int ret;
 
-   strbuf_addstr(buf, path);
-   strbuf_complete(buf, '/');
-   strbuf_addstr(buf, ".git");
-
-   git_dir = read_gitfile(buf->buf);
-   if (git_dir) {
-   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) {
-   err = SUBMODULE_PATH_ERR_NOT_CONFIGURED;
-   goto cleanup;
-   }
-   strbuf_reset(buf);
-   strbuf_git_path(buf, "%s/%s", "modules", sub->name);
-   }
-
-   strbuf_addch(buf, '/');
-   strbuf_addbuf(_submodule_dir, buf);
+   ret = submodule_to_gitdir(_submodule_dir, path);
+   if (ret)
+   goto cleanup;
 
+   strbuf_complete(_submodule_dir, '/');
+   strbuf_addbuf(buf, _submodule_dir);
strbuf_vaddf(buf, fmt, args);
 
if (get_common_dir_noenv(_submodule_common_dir, 
git_submodule_dir.buf))
@@ -514,8 +495,7 @@ static int do_submodule_path(struct strbuf *buf, const char 
*path,
 cleanup:
strbuf_release(_submodule_dir);
strbuf_release(_submodule_common_dir);
-
-   return err;
+   return ret;
 }
 
 char *git_pathdup_submodule(const char *path, const char *fmt, ...)
diff --git a/submodule.c b/submodule.c
index ece17315d..3ce589d55 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1335,3 +1335,34 @@ void prepare_submodule_repo_env(struct argv_array *out)
}
argv_array_push(out, "GIT_DIR=.git");
 }
+
+int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
+{
+   const struct submodule *sub;
+   const char *git_dir;
+   int ret = 0;
+
+   strbuf_reset(buf);
+   strbuf_addstr(buf, submodule);
+   strbuf_complete(buf, '/');
+   strbuf_addstr(buf, ".git");
+
+   git_dir = read_gitfile(buf->buf);
+   if (git_dir) {
+   strbuf_reset(buf);
+   strbuf_addstr(buf, git_dir);
+   }
+   if (!is_git_directory(buf->buf)) {
+   gitmodules_config();
+   sub = submodule_from_path(null_sha1, submodule);
+   if (!sub) {
+   ret = -1;
+   goto cleanup;
+   }
+   strbuf_reset(buf);
+   strbuf_git_path(buf, "%s/%s", "modules", sub->name);
+   }
+
+cleanup:
+   return ret;
+}
diff --git a/submodule.h b/submodule.h
index 23d76682b..2728494ce 100644
--- a/submodule.h
+++ b/submodule.h
@@ -70,6 +70,7 @@ extern int push_unpushed_submodules(struct sha1_array 
*commits,
int dry_run);
 void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
 int parallel_submodules(void);
+int submodule_to_gitdir(struct strbuf *buf, const char *submodule);
 
 /*
  * Prepare the "env_array" parameter of a "struct child_process" for executing
-- 
2.11.0.157.gd943d85

[PATCH v2 11/16] refs.c: kill register_ref_store(), add register_submodule_ref_store()

[Nguyễn Thái Ngọc Duy]

This is the last function in this code (besides public API) that takes
submodule argument and handles both main/submodule cases. Break it down,
move main store registration in get_main_ref_store() and keep the rest
in register_submodule_ref_store().

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 36 +++-
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/refs.c b/refs.c
index 76a0e7b5a..55a80a83d 100644
--- a/refs.c
+++ b/refs.c
@@ -1402,25 +1402,19 @@ static struct ref_store 
*lookup_submodule_ref_store(const char *submodule)
 
 /*
  * Register the specified ref_store to be the one that should be used
- * for submodule (or the main repository if submodule is NULL). It is
- * a fatal error to call this function twice for the same submodule.
+ * for submodule. It is a fatal error to call this function twice for
+ * the same submodule.
  */
-static void register_ref_store(struct ref_store *refs, const char *submodule)
+static void register_submodule_ref_store(struct ref_store *refs,
+const char *submodule)
 {
-   if (!submodule) {
-   if (main_ref_store)
-   die("BUG: main_ref_store initialized twice");
-
-   main_ref_store = refs;
-   } else {
-   if (!submodule_ref_stores.tablesize)
-   hashmap_init(_ref_stores, submodule_hash_cmp, 
0);
+   if (!submodule_ref_stores.tablesize)
+   hashmap_init(_ref_stores, submodule_hash_cmp, 0);
 
-   if (hashmap_put(_ref_stores,
-   alloc_submodule_hash_entry(submodule, refs)))
-   die("BUG: ref_store for submodule '%s' initialized 
twice",
-   submodule);
-   }
+   if (hashmap_put(_ref_stores,
+   alloc_submodule_hash_entry(submodule, refs)))
+   die("BUG: ref_store for submodule '%s' initialized twice",
+   submodule);
 }
 
 /*
@@ -1437,7 +1431,6 @@ static struct ref_store *ref_store_init(const char 
*submodule)
die("BUG: reference backend %s is unknown", be_name);
 
refs = be->init(submodule);
-   register_ref_store(refs, submodule);
return refs;
 }
 
@@ -1449,6 +1442,12 @@ static struct ref_store *get_main_ref_store(void)
return main_ref_store;
 
refs = ref_store_init(NULL);
+   if (refs) {
+   if (main_ref_store)
+   die("BUG: main_ref_store initialized twice");
+
+   main_ref_store = refs;
+   }
return refs;
 }
 
@@ -1469,6 +1468,9 @@ struct ref_store *get_ref_store(const char *submodule)
if (is_nonbare_repository_dir(_sb))
refs = ref_store_init(submodule);
strbuf_release(_sb);
+
+   if (refs)
+   register_submodule_ref_store(refs, submodule);
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v2 08/16] refs.c: introduce get_main_ref_store()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 7a474198e..10994d992 100644
--- a/refs.c
+++ b/refs.c
@@ -1445,15 +1445,23 @@ static struct ref_store *ref_store_init(const char 
*submodule)
return refs;
 }
 
+static struct ref_store *get_main_ref_store(void)
+{
+   struct ref_store *refs;
+
+   if (main_ref_store)
+   return main_ref_store;
+
+   refs = ref_store_init(NULL);
+   return refs;
+}
+
 struct ref_store *get_ref_store(const char *submodule)
 {
struct ref_store *refs;
 
if (!submodule || !*submodule) {
-   refs = lookup_ref_store(NULL);
-
-   if (!refs)
-   refs = ref_store_init(NULL);
+   return get_main_ref_store();
} else {
refs = lookup_ref_store(submodule);
 
-- 
2.11.0.157.gd943d85

[PATCH v2 09/16] refs: rename lookup_ref_store() to lookup_submodule_ref_store()

[Nguyễn Thái Ngọc Duy]

With get_main_ref_store() being used inside get_ref_store(),
lookup_ref_store() is only used for submodule code path. Rename to
reflect that and delete dead code.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index 10994d992..ea13a5b86 100644
--- a/refs.c
+++ b/refs.c
@@ -1384,17 +1384,13 @@ static struct ref_store *main_ref_store;
 static struct hashmap submodule_ref_stores;
 
 /*
- * Return the ref_store instance for the specified submodule (or the
- * main repository if submodule is NULL). If that ref_store hasn't
- * been initialized yet, return NULL.
+ * Return the ref_store instance for the specified submodule. If that
+ * ref_store hasn't been initialized yet, return NULL.
  */
-static struct ref_store *lookup_ref_store(const char *submodule)
+static struct ref_store *lookup_submodule_ref_store(const char *submodule)
 {
struct submodule_hash_entry *entry;
 
-   if (!submodule)
-   return main_ref_store;
-
if (!submodule_ref_stores.tablesize)
/* It's initialized on demand in register_ref_store(). */
return NULL;
@@ -1463,7 +1459,7 @@ struct ref_store *get_ref_store(const char *submodule)
if (!submodule || !*submodule) {
return get_main_ref_store();
} else {
-   refs = lookup_ref_store(submodule);
+   refs = lookup_submodule_ref_store(submodule);
 
if (!refs) {
struct strbuf submodule_sb = STRBUF_INIT;
@@ -1474,7 +1470,6 @@ struct ref_store *get_ref_store(const char *submodule)
strbuf_release(_sb);
}
}
-
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v2 10/16] refs.c: flatten get_ref_store() a bit

[Nguyễn Thái Ngọc Duy]

This helps the future changes in this code. And because get_ref_store()
is destined to become get_submodule_ref_store(), the "get main store"
code path will be removed eventually. After this the patch to delete
that code will be cleaner.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index ea13a5b86..76a0e7b5a 100644
--- a/refs.c
+++ b/refs.c
@@ -1454,22 +1454,21 @@ static struct ref_store *get_main_ref_store(void)
 
 struct ref_store *get_ref_store(const char *submodule)
 {
+   struct strbuf submodule_sb = STRBUF_INIT;
struct ref_store *refs;
 
if (!submodule || !*submodule) {
return get_main_ref_store();
-   } else {
-   refs = lookup_submodule_ref_store(submodule);
+   }
 
-   if (!refs) {
-   struct strbuf submodule_sb = STRBUF_INIT;
+   refs = lookup_submodule_ref_store(submodule);
+   if (refs)
+   return refs;
 
-   strbuf_addstr(_sb, submodule);
-   if (is_nonbare_repository_dir(_sb))
-   refs = ref_store_init(submodule);
-   strbuf_release(_sb);
-   }
-   }
+   strbuf_addstr(_sb, submodule);
+   if (is_nonbare_repository_dir(_sb))
+   refs = ref_store_init(submodule);
+   strbuf_release(_sb);
return refs;
 }
 
-- 
2.11.0.157.gd943d85

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

[Nguyễn Thái Ngọc Duy]

Given $GIT_DIR and $GIT_COMMON_DIR, files-backend is now in charge of
deciding what goes where. The end goal is to pass $GIT_DIR only. A
refs "view" of a linked worktree is a logical ref store that combines
two files backends together.

(*) Not entirely true since strbuf_git_path_submodule() still does path
translation underneath. But that's for another patch.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 24f5bf7f1..74e31d041 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -924,6 +924,9 @@ struct files_ref_store {
 */
const char *submodule;
 
+   struct strbuf gitdir;
+   struct strbuf gitcommondir;
+
struct ref_entry *loose;
struct packed_ref_cache *packed;
 };
@@ -937,6 +940,7 @@ static void files_path(struct files_ref_store *refs, struct 
strbuf *sb,
 {
struct strbuf tmp = STRBUF_INIT;
va_list vap;
+   const char *ref;
 
va_start(vap, fmt);
strbuf_vaddf(, fmt, vap);
@@ -944,8 +948,12 @@ static void files_path(struct files_ref_store *refs, 
struct strbuf *sb,
if (refs->submodule)
strbuf_git_path_submodule(sb, refs->submodule,
  "%s", tmp.buf);
+   else if (is_per_worktree_ref(tmp.buf) ||
+(skip_prefix(tmp.buf, "logs/", ) &&
+ is_per_worktree_ref(ref)))
+   strbuf_addf(sb, "%s/%s", refs->gitdir.buf, tmp.buf);
else
-   strbuf_git_path(sb, "%s", tmp.buf);
+   strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
strbuf_release();
 }
 
@@ -1004,7 +1012,15 @@ static struct ref_store *files_ref_store_create(const 
char *submodule)
 
base_ref_store_init(ref_store, _be_files);
 
-   refs->submodule = xstrdup_or_null(submodule);
+   strbuf_init(>gitdir, 0);
+   strbuf_init(>gitcommondir, 0);
+
+   if (submodule) {
+   refs->submodule = xstrdup(submodule);
+   } else {
+   strbuf_addstr(>gitdir, get_git_dir());
+   strbuf_addstr(>gitcommondir, get_git_common_dir());
+   }
 
return ref_store;
 }
-- 
2.11.0.157.gd943d85