Re: Google Doc about the Contributors' Summit

[Junio C Hamano]

Johannes Schindelin  writes:

> Technically, it is not a write-up, and I never meant it to be that. I
> intended this document to help me remember what had been discussed, and I
> doubt it is useful at all to anybody who has not been there.
>
> I abused the Git mailing list to share that link, what I really should
> have done is to use an URL shortener and jot the result down on the
> whiteboard.
>
> Very sorry for that,

Heh, no need to apologize.

I saw your  that was
sent to the list long after the event, which obviously no longer
meant for collaborative note taking and thought that you are
inviting others to read the result of that note taking, and that is
why I commented on that.  I've hopefully touched some "ask Junio
what he thinks of this" items and the whole thing was not wasted ;-)

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

[Junio C Hamano]

René Scharfe  writes:

> Am 10.02.2017 um 23:24 schrieb Junio C Hamano:
>> * vn/xdiff-func-context (2017-01-15) 1 commit
>>  - xdiff -W: relax end-of-file function detection
>>
>>  "git diff -W" has been taught to handle the case where a new
>>  function is added at the end of the file better.
>>
>>  Will hold.
>>  Discussion on an follow-up change to go back from the line that
>>  matches the funcline to show comments before the function
>>  definition has not resulted in an actionable conclusion.
>
> This one is a bug fix and can be merged already IMHO.

Absolutely.  I was just waiting if the follow-up discussion would
easily and quickly lead to another patch, forgot about what exactly
I was waiting for (i.e. the gravity of not having the follow-up),
and have left it in "Will hold" status forever.

Let's merge it to 'next' and then decide if we want to also merge it
to 'master' before the final.  The above step alone is a lot less
contriversial and tricky bugfix.

Thanks.

Re: fuzzy patch application

[Junio C Hamano]

Nick Desaulniers  writes:

> For the dangers related to fuzzing, is there more info?  I found
> and old post on this from Linus calling fuzzing dangerous but
> from what I could tell about my patch that wouldn't apply
> without fuzzing, the only difference in bad hunks was
> whitespace that had diverged somehow.

If the "old post" is the one he explains why he chose not to allow
fuzz by default, I think you got all what you need.  Basically, he
wanted you and his users to make sure that the patch they are having
trouble with applying can be due to only insignificant difference
and it is safe to apply with reduced context, instead of blindly
accepting a fuzzy patch application.

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

[Junio C Hamano]

Siddharth Kannan  writes:

> @@ -2234,11 +2235,18 @@ int setup_revisions(int argc, const char **argv, 
> struct rev_info *revs, struct s
>   }
>   if (opts < 0)
>   exit(128);
> - continue;
> +
> + args = handle_revision_arg(arg, revs, flags, 
> revarg_opt);
> + handle_rev_arg_called = 1;
> + if (args)
> + continue;
> + else
> + --left;
>   }
>  
>  
> - if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
> + if ((handle_rev_arg_called && args) ||
> + handle_revision_arg(arg, revs, flags, 
> revarg_opt)) {

Naively I would have expected that removing the "continue" at the
end and letting the control go to the existing

if (handle_revision_arg(arg, revs, flags, revarg_opt)) {

would be all that is needed.  The latter half of the patch is an
artifact of having ane xtra "handle_revision_arg() calls inside the
"if it begins with dash" block to avoid calling it twice.

So the difference is just "--left" (by the way, our codebase seem to
prefer "left--" when there is no difference between pre- or post-
decrement/increment) that adjusts the slot in argv[] where the next
unknown argument is stuffed to.

The adjustment is needed as the call to handle_revision_opt() that
is before the pre-context of this hunk stuffed the unknown thing
that begins with "-" into argv[left++]; if that thing turns out to
be a valid rev, then you would need to take it back, because after
all, that is not an unknown command line argument.

I am wondering if writing it like the following is easier to
understand.  I had a hard time figuring out what you are trying to
do, partly because "args" is quite a misnomer---implying "how many
arguments did we see" that is similar to opts that does mean "how
many options did handle_revision_opts() see?"  The variable means
means "yes we saw a valid rev" when it is zero.  The rewrite
below may avoid such a confusion.  I dunno.

 revision.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/revision.c b/revision.c
index b37dbec378..e238430948 100644
--- a/revision.c
+++ b/revision.c
@@ -2204,6 +2204,7 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
revarg_opt |= REVARG_CANNOT_BE_FILENAME;
read_from_stdin = 0;
for (left = i = 1; i < argc; i++) {
+   int maybe_rev = 0;
const char *arg = argv[i];
if (*arg == '-') {
int opts;
@@ -2234,11 +2235,16 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
}
if (opts < 0)
exit(128);
-   continue;
+   maybe_rev = 1;
+   left--; /* tentatively cancel "unknown opt" */
}
 
-
-   if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+   if (!handle_revision_arg(arg, revs, flags, revarg_opt)) {
+   got_rev_arg = 1;
+   } else if (maybe_rev) {
+   left++; /* it turns out that it was "unknown opt" */
+   continue;
+   } else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
@@ -2255,8 +2261,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) {

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

[René Scharfe]

Am 10.02.2017 um 23:24 schrieb Junio C Hamano:

* vn/xdiff-func-context (2017-01-15) 1 commit
 - xdiff -W: relax end-of-file function detection

 "git diff -W" has been taught to handle the case where a new
 function is added at the end of the file better.

 Will hold.
 Discussion on an follow-up change to go back from the line that
 matches the funcline to show comments before the function
 definition has not resulted in an actionable conclusion.


This one is a bug fix and can be merged already IMHO.


I have raw patches for showing comments before functions with -W, but 
they don't handle the case of a change being within such a comment. 
We'd want to show the trailing function which it is referring to in such 
a case, right?


And that's a bit tricky because it requires a more complicated model: 
Currently we distinguish only between function lines and the rest, while 
the new way would require identifying leading comment lines and probably 
also trailing function body lines in addition to that, and neither can 
be classified simply by looking at the line in question -- their type 
depends on that of neighboring lines.


René

Re: fuzzy patch application

[Nick Desaulniers]

It's not my call about the defaults, but users can be surprised by
such changes.

For the dangers related to fuzzing, is there more info?  I found
and old post on this from Linus calling fuzzing dangerous but
from what I could tell about my patch that wouldn't apply
without fuzzing, the only difference in bad hunks was
whitespace that had diverged somehow.

On Fri, Feb 10, 2017 at 2:53 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Making "am -3" default may also scare users who are not exactly
>> comfortable with reading "git diff" output during a conflicted merge
>> and resolving a conflict, but other than that there shouldn't be any
>> downside.
>
> Another obvious downside is that there are those of us who prefer to
> run "am" without "-3" first to notice that the patch we expect to
> apply cleanly does apply cleanly, which gives us a chance to catch
> mistakes.  I personally feel that as long as there is a configuration
> that makes -3 a personal default (with --no-3way override from the
> command line), it is a better design not to enable "-3" by default
> for everybody.  New people can first learn using both forms and then
> after they got comfortable with resolving merges, they can choose to
> flip the default for themselves.



-- 
Thanks,
~Nick Desaulniers

Re: fuzzy patch application

[Junio C Hamano]

Junio C Hamano  writes:

> Making "am -3" default may also scare users who are not exactly
> comfortable with reading "git diff" output during a conflicted merge
> and resolving a conflict, but other than that there shouldn't be any
> downside.

Another obvious downside is that there are those of us who prefer to
run "am" without "-3" first to notice that the patch we expect to
apply cleanly does apply cleanly, which gives us a chance to catch
mistakes.  I personally feel that as long as there is a configuration
that makes -3 a personal default (with --no-3way override from the
command line), it is a better design not to enable "-3" by default
for everybody.  New people can first learn using both forms and then
after they got comfortable with resolving merges, they can choose to
flip the default for themselves.

Re: [PATCH 2/2] ls-files: move only kept cache entries in prune_cache()

[Brandon Williams]

On 02/10, René Scharfe wrote:
> prune_cache() first identifies those entries at the start of the sorted
> array that can be discarded.  Then it moves the rest of the entries up.
> Last it identifies the unwanted trailing entries among the moved ones
> and cuts them off.
> 
> Change the order: Identify both start *and* end of the range to keep
> first and then move only those entries to the top.  The resulting code
> is slightly shorter and a bit more efficient.
> 
> Signed-off-by: Rene Scharfe 
> ---
> The performance impact is probably only measurable with a *really* big
> index.

Well there's been a lot of talk recently about *really* big indexes, so
I'm sure someone out there will be happy :)

> 
>  builtin/ls-files.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 18105ec7ea..1c0f057d02 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -379,10 +379,7 @@ static void prune_cache(const char *prefix, size_t 
> prefixlen)
>   pos = cache_name_pos(prefix, prefixlen);
>   if (pos < 0)
>   pos = -pos-1;
> - memmove(active_cache, active_cache + pos,
> - (active_nr - pos) * sizeof(struct cache_entry *));
> - active_nr -= pos;
> - first = 0;
> + first = pos;
>   last = active_nr;
>   while (last > first) {
>   int next = (last + first) >> 1;
> @@ -393,7 +390,9 @@ static void prune_cache(const char *prefix, size_t 
> prefixlen)
>   }
>   last = next;
>   }
> - active_nr = last;
> + memmove(active_cache, active_cache + pos,
> + (last - pos) * sizeof(struct cache_entry *));
> + active_nr = last - pos;
>  }
>  
>  /*
> -- 
> 2.11.1
> 

Both these patches look good to me.

-- 
Brandon Williams

Re: [PATCH] preload-index: avoid lstat for skip-worktree items

[Junio C Hamano]

Johannes Schindelin  writes:

> Teach preload-index to avoid lstat() calls for index-entries
> with skip-worktree bit set.  This is a performance optimization.
> ...
> diff --git a/preload-index.c b/preload-index.c
> index c1fe3a3ef9c..70a4c808783 100644
> --- a/preload-index.c
> +++ b/preload-index.c
> @@ -53,6 +53,8 @@ static void *preload_thread(void *_data)
>   continue;
>   if (ce_uptodate(ce))
>   continue;
> + if (ce_skip_worktree(ce))
> + continue;
>   if (!ce_path_match(ce, >pathspec, NULL))
>   continue;
>   if (threaded_has_symlink_leading_path(, ce->name, 
> ce_namelen(ce)))

Because we are only interested in marking the ones that match
between the index and the working tree as "up-to-date", and we are
not doing the opposite (i.e. toggle "up-to-date" bit off by noticing
that things are now different) in this codepath, this change does
make sense.  The ones marked as "skip", even if there were an
unrelated file or directory at the path where the index expects a
regular file, can be safely ignored.

Thanks.

Hello

[university]

Hello,

I'M Anessa female 25 years old single, I am contacting you because I will be 
relocating to your country.
I will send you my photos soon as i hear from you and also tell you much about 
my self.
Thanks.

Sincerely yours
Anessa.

Re: [PATCH v2] git-p4: fix git-p4.pathEncoding for removed files

[Junio C Hamano]

Luke Diamand  writes:

> On 9 February 2017 at 23:39, Junio C Hamano  wrote:
>> Lars Schneider  writes:
>>
>>> unfortunately, I missed to send this v2. I agree with Luke's review and
>>> I moved the re-encode of the path name to the `streamOneP4File` and
>>> `streamOneP4Deletion` explicitly.
>>>
>>> Discussion:
>>> http://public-inbox.org/git/CAE5ih7-=bd_zol5pfyfd2qvy-xe24v_cgge0xoavuotk02e...@mail.gmail.com/
>>>
>>> Thanks,
>>> Lars
>>
>> Thanks.  Will replace but will not immediately merge to 'next' yet,
>> just in case Luke wants to tell me add his "Reviewed-by:".
>
> Yes, this looks good to me now.

Thanks.

Re: fuzzy patch application

[Junio C Hamano]

Jeff King  writes:

> I dunno. I always use it, but I'm not sure if there are any downsides,
> aside from a little extra processing time. It does have some
> incompatibilities with other options. And I think it kicks in rename
> detection (but I might be mis-remembering another feature). That could
> be surprising, I guess.
>
> The original dates all the way back to 47f0b6d5d (Fall back to three-way
> merge when applying a patch., 2005-10-06), but I don't see any rationale
> for not making it the default. Junio probably could give a better
> answer.

Nothing deep.  Just being cautious by not to enable extra frills by
default, making it strictly o-p-t i-n.  As that was a strict fall
back (i.e. there was no "if we are going to fall back, we need to
spend extra cycles to do this extra preparation before we attempt
the regular application"), extra-processing cost was not a concern,
at least back I wrote it the first time.  I do not offhand know if
that still holds in the current one that was rewritten in C.

Making "am -3" default may also scare users who are not exactly
comfortable with reading "git diff" output during a conflicted merge
and resolving a conflict, but other than that there shouldn't be any
downside.

What's cooking in git.git (Feb 2017, #03; Fri, 10)

[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.

2.12-rc1 has been tagged and most of the big things are behind us (I
am reluctant to merge anything large-ish after -rc0 and that was why
-rc0 preview had topics that cooked in 'next' only for a few days
in---usually my rule is to keep any non-trivial topic for about a
week in 'next').  There still are interesting and/or exciting things
coming into 'next', hopefully they will be the "killer" additions
for the release after the upcoming 2.12.  In the meantime, lets make
sure that we catch regressions in -rc1 and the tip of 'master'.

Oh, also 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"]

* bw/push-submodule-only (2017-02-01) 2 commits
  (merged to 'next' on 2017-02-06 at 851edafb14)
 + completion: add completion for --recurse-submodules=only
 + doc: add doc for git-push --recurse-submodules=only

 Add missing documentation update to a recent topic.


* da/t7800-cleanup (2017-02-08) 2 commits
  (merged to 'next' on 2017-02-10 at c983b65d33)
 + t7800: replace "wc -l" with test_line_count
 + Merge branch 'da/difftool-dir-diff-fix' into da/t7800-cleanup
 (this branch uses js/difftool-builtin.)

 Test updates.


* dl/difftool-doc-no-gui-option (2017-02-08) 1 commit
  (merged to 'next' on 2017-02-10 at 3a3496a740)
 + Document the --no-gui option in difftool

 Doc update.


* ew/complete-svn-authorship-options (2017-02-06) 1 commit
  (merged to 'next' on 2017-02-06 at dca324db7c)
 + completion: fix git svn authorship switches

 Correct command line completion (in contrib/) on "git svn"


* jk/log-graph-name-only (2017-02-08) 1 commit
  (merged to 'next' on 2017-02-10 at 2ab7ed99f4)
 + diff: print line prefix for --name-only output

 "git log --graph" did not work well with "--name-only", even though
 other forms of "diff" output were handled correctly.


* jk/reset-to-break-a-commit-doc (2017-02-03) 1 commit
  (merged to 'next' on 2017-02-06 at 7f571e62e9)
 + reset: add an example of how to split a commit into two

 A minor doc update.


* js/difftool-builtin (2017-02-08) 2 commits
  (merged to 'next' on 2017-02-10 at 30b3f383e8)
 + t7800: simplify basic usage test
  (merged to 'next' on 2017-02-06 at 6a90549f38)
 + difftool: fix bug when printing usage
 (this branch is used by da/t7800-cleanup.)

 A few hot-fixes to C-rewrite of "git difftool".


* nd/rev-list-all-includes-HEAD-doc (2017-02-08) 1 commit
  (merged to 'next' on 2017-02-10 at 5df8b81b57)
 + rev-list-options.txt: update --all about HEAD

 Doc update.


* ps/worktree-prune-help-fix (2017-02-06) 1 commit
  (merged to 'next' on 2017-02-06 at eeb96f1677)
 + worktree: fix option descriptions for `prune`

 Incorrect usage help message for "git worktree prune" has been fixed.


* rs/fill-directory-optim (2017-02-08) 1 commit
  (merged to 'next' on 2017-02-10 at 71047464df)
 + dir: avoid allocation in fill_directory()

 Code clean-up.


* rs/p5302-create-repositories-before-tests (2017-02-06) 1 commit
  (merged to 'next' on 2017-02-06 at f93bd2ed47)
 + p5302: create repositories for index-pack results explicitly

 Adjust a perf test to new world order where commands that do
 require a repository are really strict about having a repository.

--
[New Topics]

* jk/alternate-ref-optim (2017-02-08) 11 commits
  (merged to 'next' on 2017-02-10 at f26f32cff6)
 + receive-pack: avoid duplicates between our refs and alternates
 + receive-pack: treat namespace .have lines like alternates
 + receive-pack: fix misleading namespace/.have comment
 + receive-pack: use oidset to de-duplicate .have lines
 + add oidset API
 + fetch-pack: cache results of for_each_alternate_ref
 + for_each_alternate_ref: replace transport code with for-each-ref
 + for_each_alternate_ref: pass name/oid instead of ref struct
 + for_each_alternate_ref: use strbuf for path allocation
 + for_each_alternate_ref: stop trimming trailing slashes
 + for_each_alternate_ref: handle failure from real_pathdup()

 Optimizes resource usage while enumerating refs from alternate
 object store, to help receiving end of "push" that hosts a
 repository with many "forks".

 Will cook in 'next'.


* lt/pathspec-negative (2017-02-10) 2 commits
  (merged to 'next' on 2017-02-10 at 8ea7874076)
 + pathspec: don't error out on all-exclusionary pathspec patterns
 + pathspec magic: add '^' as alias for '!'

 The "negative" pathspec feature was somewhat more cumbersome to use
 than necessary in that its short-hand 

Re: [PATCH v2] git-p4: fix git-p4.pathEncoding for removed files

[Luke Diamand]

On 9 February 2017 at 23:39, Junio C Hamano  wrote:
> Lars Schneider  writes:
>
>> unfortunately, I missed to send this v2. I agree with Luke's review and
>> I moved the re-encode of the path name to the `streamOneP4File` and
>> `streamOneP4Deletion` explicitly.
>>
>> Discussion:
>> http://public-inbox.org/git/CAE5ih7-=bd_zol5pfyfd2qvy-xe24v_cgge0xoavuotk02e...@mail.gmail.com/
>>
>> Thanks,
>> Lars
>
> Thanks.  Will replace but will not immediately merge to 'next' yet,
> just in case Luke wants to tell me add his "Reviewed-by:".

Yes, this looks good to me now.

Luke

Re: fuzzy patch application

[Jeff King]

On Fri, Feb 10, 2017 at 01:37:12PM -0800, Stefan Beller wrote:

> > This is not exactly an answer to your question, but "git am -3" is often
> > a better solution than trying to fuzz patches. It assumes the patches
> > are Git patches (and record their origin blobs), and that you have that
> > blob (which should be true if the patches are based on the normal kernel
> > history, and you just fetch that history into your repository).
> >
> > I've found that this often manages to apply patches that "git apply"
> > will not by itself. And I also find the resulting conflicts to be much
> > easier to deal with than patch's ".rej" files.
> 
> I have been told this a couple of times before; do we want to make -3
> the default (in 2.13 then) ?

I dunno. I always use it, but I'm not sure if there are any downsides,
aside from a little extra processing time. It does have some
incompatibilities with other options. And I think it kicks in rename
detection (but I might be mis-remembering another feature). That could
be surprising, I guess.

The original dates all the way back to 47f0b6d5d (Fall back to three-way
merge when applying a patch., 2005-10-06), but I don't see any rationale
for not making it the default. Junio probably could give a better
answer.

-Peff

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

[Junio C Hamano]

SZEDER Gábor  writes:

> Care should be taken, though, because that prefix might contain
> 'for-each-ref' format specifiers as part of the left hand side of a
> '..' range or '...' symmetric difference notation or fetch/push/etc.
> refspec, e.g. 'git log "evil-%(refname)..br'.  Doubling every '%'
> in the prefix will prevent 'git for-each-ref' from interpolating any
> of those contained format specifiers.
> ---
>
> This is really pathological, and I'm sure this has nothing to do
> with whatever breakage Jacob experienced.  The shell
> metacharacters '(' and ')' still cause us trouble in various ways,
> but that's nothing new and has been the case for quite a while
> (always?).
>
> It's already incorporated into (the rewritten)
>
>   https://github.com/szeder/git completion-refs-speedup

Should I expect a reroll to come, or is this the only fix-up to the
series that begins at <20170203025405.8242-1-szeder@gmail.com>?

No hurries.

Re: fuzzy patch application

[Stefan Beller]

On Fri, Feb 10, 2017 at 12:57 PM, Jeff King  wrote:
> On Fri, Feb 10, 2017 at 11:20:59AM -0800, Nick Desaulniers wrote:
>
>> I frequently need to backport patches from the Linux kernel to older
>> kernel versions (Android Security).  My usual workflow for simple
>> patches is:
>>
>> 1. try `git am patch.txt`.
>
> This is not exactly an answer to your question, but "git am -3" is often
> a better solution than trying to fuzz patches. It assumes the patches
> are Git patches (and record their origin blobs), and that you have that
> blob (which should be true if the patches are based on the normal kernel
> history, and you just fetch that history into your repository).
>
> I've found that this often manages to apply patches that "git apply"
> will not by itself. And I also find the resulting conflicts to be much
> easier to deal with than patch's ".rej" files.
>
> -Peff

I have been told this a couple of times before; do we want to make -3
the default (in 2.13 then) ?

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

[Jeff King]

On Fri, Feb 10, 2017 at 09:23:15PM +, David Turner wrote:

> > Speaking of stderr, I wonder if this function should be calling
> > fflush(stderr) before looking at the fstat result. There could be contents 
> > buffered
> > there that haven't been written out yet (not from child processes, but 
> > perhaps
> > ones written in this process itself).
> > Probably unlikely in practice, since stderr is typically unbuffered by 
> > default.
> 
> Process_log_file_at_exit calls fflush.  Will fix the other.

Ah, good. That makes sense, since we might deadlock if we do it in a
signal handler. Perhaps that is a reason not to use stderr here again
(though if we want to be that careful, a new fdopen() call is also a bad
idea, as we can deadlock over the malloc() lock; you'd have to snprintf
to a small buffer and dump it with write()).

-Peff

RE: [PATCH v5] gc: ignore old gc.log files

[David Turner]

> -Original Message-
> From: Jeff King [mailto:p...@peff.net]
> Sent: Friday, February 10, 2017 4:15 PM
> To: David Turner 
> Cc: git@vger.kernel.org; pclo...@gmail.com; Junio C Hamano
> 
> Subject: Re: [PATCH v5] gc: ignore old gc.log files
> 
> > @@ -76,10 +78,30 @@ static void git_config_date_string(const char
> > *key, const char **output)  static void process_log_file(void)  {
> > struct stat st;
> > -   if (!fstat(get_lock_file_fd(_lock), ) && st.st_size)
> > +   if (fstat(get_lock_file_fd(_lock), )) {
> > +   /*
> > +* Perhaps there was an i/o error or another
> > +* unlikely situation.  Try to make a note of
> > +* this in gc.log along with any existing
> > +* messages.
> > +*/
> > +   FILE *fp;
> > +   int saved_errno = errno;
> > +   fp = fdopen(log_lock.tempfile.fd, "a");
> 
> We usually use xfdopen() to handle (unlikely) errors rather than segfaulting. 
>  But
> I think you'd actually want fdopen_lock_file(), which attaches the fd to the
> tempfile for flushing and cleanup purposes.
> 
> That said, I'm not sure I understand why you're opening a new stdio 
> filehandle.
> We know that stderr already points to our logfile (that's how content gets 
> there
> in the first place). If there's a problem with the file or the descriptor, 
> opening a
> new filehandle around the same descriptor won't help.
> 
> Speaking of stderr, I wonder if this function should be calling
> fflush(stderr) before looking at the fstat result. There could be contents 
> buffered
> there that haven't been written out yet (not from child processes, but perhaps
> ones written in this process itself).
> Probably unlikely in practice, since stderr is typically unbuffered by 
> default.

Process_log_file_at_exit calls fflush.  Will fix the other.

[ANNOUNCE] Git v2.12.0-rc1

[Junio C Hamano]

A release candidate Git v2.12.0-rc1 is now available for testing
at the usual places.  It is comprised of 455 non-merge commits
since v2.11.0, contributed by 65 people, 20 of which are new faces.

The tarballs are found at:

https://www.kernel.org/pub/software/scm/git/testing/

The following public repositories all have a copy of the
'v2.12.0-rc1' tag and the 'master' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

New contributors whose contributions weren't in v2.11.0 are as follows.
Welcome to the Git development community!

  Alan Davies, Andreas Krey, Cornelius Weig, Denton Liu, George
  Vanburgh, Igor Kushnir, Jack Bates, Kristoffer Haugsbakk, Kyle
  Meyer, Luis Ressel, Lukas Puehringer, Markus Hitter, Peter Law,
  Rasmus Villemoes, Rogier Goossens, Stefan Dotterweich, Steven
  Penny, Vinicius Kursancew, Vladimir Panteleev, and Wolfram Sang.

Returning contributors who helped this release are as follows.
Thanks for your continued support.

  마누엘, Alex Henrie, Beat Bolli, Brandon Williams, brian
  m. carlson, Chris Packham, Christian Couder, David Aguilar, David
  Turner, Dennis Kaarsemaker, Dimitriy Ryazantcev, Elia Pinto,
  Eric Wong, Heiko Voigt, Jacob Keller, Jeff Hostetler, Jeff King,
  Johannes Schindelin, Johannes Sixt, Jonathan Tan, Junio C Hamano,
  Kyle J. McKay, Lars Schneider, Linus Torvalds, Luke Diamand, Matt
  McCutchen, Max Kirillov, Mike Hommey, Nguyễn Thái Ngọc Duy,
  Patrick Steinhardt, Paul Mackerras, Philip Oakley, Pranit Bauva,
  Ramsay Jones, René Scharfe, Richard Hansen, Santiago Torres,
  Satoshi Yasushima, Stefan Beller, Stephan Beyer, SZEDER Gábor,
  Torsten Bögershausen, Vasco Almeida, Vegard Nossum, and Vitaly
  "_Vi" Shukela.



Git 2.12 Release Notes (draft)
==

Backward compatibility notes.

 * Use of an empty string that is used for 'everything matches' is
   still warned and Git asks users to use a more explicit '.' for that
   instead.  The hope is that existing users will not mind this
   change, and eventually the warning can be turned into a hard error,
   upgrading the deprecation into removal of this (mis)feature.  That
   is not scheduled to happen in the upcoming release (yet).

 * The historical argument order "git merge  HEAD ..."
   has been deprecated for quite some time, and will be removed in a
   future release.

 * An ancient script "git relink" has been removed.


Updates since v2.11
---

UI, Workflows & Features

 * Various updates to "git p4".

 * "git p4" didn't interact with the internal of .git directory
   correctly in the modern "git-worktree"-enabled world.

 * "git branch --list" and friends learned "--ignore-case" option to
   optionally sort branches and tags case insensitively.

 * In addition to %(subject), %(body), "log --pretty=format:..."
   learned a new placeholder %(trailers).

 * "git rebase" learned "--quit" option, which allows a user to
   remove the metadata left by an earlier "git rebase" that was
   manually aborted without using "git rebase --abort".

 * "git clone --reference $there --recurse-submodules $super" has been
   taught to guess repositories usable as references for submodules of
   $super that are embedded in $there while making a clone of the
   superproject borrow objects from $there; extend the mechanism to
   also allow submodules of these submodules to borrow repositories
   embedded in these clones of the submodules embedded in the clone of
   the superproject.

 * Porcelain scripts written in Perl are getting internationalized.

 * "git merge --continue" has been added as a synonym to "git commit"
   to conclude a merge that has stopped due to conflicts.

 * Finer-grained control of what protocols are allowed for transports
   during clone/fetch/push have been enabled via a new configuration
   mechanism.

 * "git shortlog" learned "--committer" option to group commits by
   committer, instead of author.

 * GitLFS integration with "git p4" has been updated.

 * The isatty() emulation for Windows has been updated to eradicate
   the previous hack that depended on internals of (older) MSVC
   runtime.

 * Some platforms no longer understand "latin-1" that is still seen in
   the wild in e-mail headers; replace them with "iso-8859-1" that is
   more widely known when conversion fails from/to it.

 * "git grep" has been taught to optionally recurse into submodules.

 * "git rm" used to refuse to remove a submodule when it has its own
   git repository embedded in its working tree.  It learned to move
   the repository away to $GIT_DIR/modules/ of the superproject
   instead, and allow the submodule to be deleted (as long as there
   will be no loss of local 

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

[David Turner]

A server can end up in a state where there are lots of unreferenced
loose objects (say, because many users are doing a bunch of rebasing
and pushing their rebased branches).  Running "git gc --auto" in
this state would cause a gc.log file to be created, preventing
future auto gcs, causing pack files to pile up.  Since many git
operations are O(n) in the number of pack files, this would lead to
poor performance.

Git should never get itself into a state where it refuses to do any
maintenance, just because at some point some piece of the maintenance
didn't make progress.

Teach Git to ignore gc.log files which are older than (by default)
one day old, which can be tweaked via the gc.logExpiry configuration
variable.  That way, these pack files will get cleaned up, if
necessary, at least once per day.  And operators who find a need for
more-frequent gcs can adjust gc.logExpiry to meet their needs.

There is also some cleanup: a successful manual gc, or a
warning-free auto gc with an old log file, will remove any old
gc.log files.

It might still happen that manual intervention is required
(e.g. because the repo is corrupt), but at the very least it won't
be because Git is too dumb to try again.

Signed-off-by: David Turner 
Helped-by: Jeff King 
Signed-off-by: Junio C Hamano 
---
 Documentation/config.txt |  6 +
 builtin/gc.c | 57 ++--
 t/t6500-gc.sh| 15 +
 3 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fc5a28a32..a684b7e3e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1402,6 +1402,12 @@ gc.autoDetach::
Make `git gc --auto` return immediately and run in background
if the system supports it. Default is true.
 
+gc.logExpiry::
+   If the file gc.log exists, then `git gc --auto` won't run
+   unless that file is more than 'gc.logExpiry' old.  Default is
+   "1.day".  See `gc.pruneExpire` for more ways to specify its
+   value.
+
 gc.packRefs::
Running `git pack-refs` in a repository renders it
unclonable by Git versions prior to 1.5.1.2 over dumb
diff --git a/builtin/gc.c b/builtin/gc.c
index 331f21926..a2b9e8924 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -33,6 +33,8 @@ static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
 static int detach_auto = 1;
+static unsigned long gc_log_expire_time;
+static const char *gc_log_expire = "1.day.ago";
 static const char *prune_expire = "2.weeks.ago";
 static const char *prune_worktrees_expire = "3.months.ago";
 
@@ -76,10 +78,28 @@ static void git_config_date_string(const char *key, const 
char **output)
 static void process_log_file(void)
 {
struct stat st;
-   if (!fstat(get_lock_file_fd(_lock), ) && st.st_size)
+   if (fstat(get_lock_file_fd(_lock), )) {
+   /*
+* Perhaps there was an i/o error or another
+* unlikely situation.  Try to make a note of
+* this in gc.log along with any existing
+* messages.
+*/
+   int saved_errno = errno;
+   fprintf(stderr, _("Failed to fstat %s: %s"),
+   get_tempfile_path(_lock.tempfile),
+   strerror(saved_errno));
+   fflush(stderr);
commit_lock_file(_lock);
-   else
+   errno = saved_errno;
+   } else if (st.st_size) {
+   /* There was some error recorded in the lock file */
+   commit_lock_file(_lock);
+   } else {
+   /* No error, clean up any old gc.log */
+   unlink(git_path("gc.log"));
rollback_lock_file(_lock);
+   }
 }
 
 static void process_log_file_at_exit(void)
@@ -113,6 +133,8 @@ static void gc_config(void)
git_config_get_bool("gc.autodetach", _auto);
git_config_date_string("gc.pruneexpire", _expire);
git_config_date_string("gc.worktreepruneexpire", 
_worktrees_expire);
+   git_config_date_string("gc.logexpiry", _log_expire);
+
git_config(git_default_config, NULL);
 }
 
@@ -290,19 +312,34 @@ static const char *lock_repo_for_gc(int force, pid_t* 
ret_pid)
 static int report_last_gc_error(void)
 {
struct strbuf sb = STRBUF_INIT;
-   int ret;
+   int ret = 0;
+   struct stat st;
+   char *gc_log_path = git_pathdup("gc.log");
 
-   ret = strbuf_read_file(, git_path("gc.log"), 0);
+   if (stat(gc_log_path, )) {
+   if (errno == ENOENT)
+   goto done;
+
+   ret = error_errno(_("Can't stat %s"), gc_log_path);
+   goto done;
+   }
+
+   if (st.st_mtime < gc_log_expire_time)
+   goto done;
+
+   ret = strbuf_read_file(, gc_log_path, 0);
if (ret > 

Re: What's cooking in git.git (Feb 2017, #02; Mon, 6)

[Junio C Hamano]

Junio C Hamano  writes:

> Jeff King  writes:
>
>> On Mon, Feb 06, 2017 at 02:34:08PM -0800, Junio C Hamano wrote:
>>
>>> * sk/parse-remote-cleanup (2017-02-06) 1 commit
>>>   (merged to 'next' on 2017-02-06 at 6ec89f72d5)
>>>  + parse-remote: remove reference to unused op_prep
>>> 
>>>  Code clean-up.
>>> 
>>>  Will merge to 'master'.
>>
>> Hrm. Are the functions in git-parse-remote.sh part of the public API?
>> That is, do we expect third-party scripts to do:
>>
>>   . "$(git rev-parse --exec)/git-parse-remote.sh
>>   error_on_missing_default_upstream "$a" "$b" "$c" "$d"
>>
>> ? If so, then they may be surprised by the change in function signature.
>>
>> I generally think of git-sh-setup as the one that external scripts would
>> use. There _is_ a manpage for git-parse-remote, but it doesn't list any
>> functions. So maybe they're all fair game for changing?
>>
>> I just didn't see any discussion of this in the original patch thread,
>> so I wanted to make sure we were making that decision consciously, and
>> not accidentally. :)
>
> Ummm, yes, I admit that this was accidental.  I didn't really think
> of parse-remote as an externally visible and supported interface,
> but users have tendency to break our expectations, so, I dunno.

After sleeping on this, I doubt that the value of this "code
clean-up" is worth the trouble of waiting to see if a third-party
who dot sources parse-remote steps up, which may never materialize
while the topic is cooking in 'next' and more importantly risking
breakage on such a third-party.

Let's drop the topic and excise it from 'next' at the next version
boundary.

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

[Jeff King]

> @@ -76,10 +78,30 @@ static void git_config_date_string(const char *key, const 
> char **output)
>  static void process_log_file(void)
>  {
>   struct stat st;
> - if (!fstat(get_lock_file_fd(_lock), ) && st.st_size)
> + if (fstat(get_lock_file_fd(_lock), )) {
> + /*
> +  * Perhaps there was an i/o error or another
> +  * unlikely situation.  Try to make a note of
> +  * this in gc.log along with any existing
> +  * messages.
> +  */
> + FILE *fp;
> + int saved_errno = errno;
> + fp = fdopen(log_lock.tempfile.fd, "a");

We usually use xfdopen() to handle (unlikely) errors rather than
segfaulting.  But I think you'd actually want fdopen_lock_file(), which
attaches the fd to the tempfile for flushing and cleanup purposes.

That said, I'm not sure I understand why you're opening a new stdio
filehandle. We know that stderr already points to our logfile (that's
how content gets there in the first place). If there's a problem with
the file or the descriptor, opening a new filehandle around the same
descriptor won't help.

Speaking of stderr, I wonder if this function should be calling
fflush(stderr) before looking at the fstat result. There could be
contents buffered there that haven't been written out yet (not from
child processes, but perhaps ones written in this process itself).
Probably unlikely in practice, since stderr is typically unbuffered by
default.

-Peff

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

[Junio C Hamano]

Jeff King  writes:

> I dunno. This seems like a lot of manual scrambling to try to overcome
> unlikely errors just to make our stderr heard (and we'd still fail in
> some cases). It seems like:
>
>   if (fstat(...)) {
>   /* weird, fstat failed; try our best to mention it */
>   error_errno("unable to fstat gc.log.lock");
>   commit_lock_file(_lock));
>   } else if (st.st_size) {
>   /* we have new errors to report */
>   commit_lock_file(_lock);
>   } else {
>   /* no new errors; clean up old ones */
>   unlink(git_path("gc.log"));
>   rollback_lock_file(_lock);
>   }
>
> would be sufficient.

Yeah, that should be sufficient.

RE: [PATCH v3] gc: ignore old gc.log files

[David Turner]

> -Original Message-
> From: Jeff King [mailto:p...@peff.net]
> Sent: Friday, February 10, 2017 3:09 PM
> To: David Turner 
> Cc: git@vger.kernel.org; pclo...@gmail.com
> Subject: Re: [PATCH v3] gc: ignore old gc.log files
> 
> On Fri, Feb 10, 2017 at 02:20:19PM -0500, David Turner wrote:
> 
> > @@ -76,10 +77,47 @@ static void git_config_date_string(const char
> > *key, const char **output)  static void process_log_file(void)  {
> > struct stat st;
> > -   if (!fstat(get_lock_file_fd(_lock), ) && st.st_size)
> > +   if (fstat(get_lock_file_fd(_lock), )) {
> > +   if (errno == ENOENT) {
> > +   /*
> > +* The user has probably intentionally deleted
> > +* gc.log.lock (perhaps because they're blowing
> > +* away the whole repo), so thre's no need to
> > +* report anything here.  But we also won't
> > +* delete gc.log, because we don't know what
> > +* the user's intentions are.
> > +*/
> 
> Hrm. Does fstat actually trigger ENOENT in that case? There's no pathname
> lookup happening at all. A simple test on Linux seems to show that it does 
> not.
> Build:
> 
>   #include 
>   #include 
>   #include 
> 
>   int main(int argc, char **argv)
>   {
>   struct stat st;
>   int fd = open(argv[1], O_WRONLY|O_CREAT|O_EXCL, 0600);
>   unlink(argv[1]);
>   fstat(fd, );
>   return 0;
>   }
> 
> and run:
> 
>   strace ./a.out tmp
> 
> which shows:
> 
>   open("tmp", O_WRONLY|O_CREAT|O_EXCL, 056660) = 3
>   unlink("tmp")   = 0
>   fstat(3, {st_mode=S_IFREG|S_ISUID|S_ISGID|0640, st_size=0, ...}) = 0
> 
> But maybe there are other systems emulating fstat() would trigger here.
> I dunno.

Yeah, I'm also not sure.  On the other hand, if we're going to catch fstat 
errors anyway, we might as well do something sensible with this one.

> > +   } else {
> > +   FILE *fp;
> > +   int fd;
> > +   int saved_errno = errno;
> > +   /*
> > +* Perhaps there was an i/o error or another
> > +* unlikely situation.  Try to make a note of
> > +* this in gc.log.  If this fails again,
> > +* give up and leave gc.log as it was.
> > +*/
> > +   rollback_lock_file(_lock);
> > +   fd = hold_lock_file_for_update(_lock,
> > +  git_path("gc.log"),
> > +  LOCK_DIE_ON_ERROR);
> 
> If there was an i/o error, then gc.log.lock still exists. And this attempt to 
> lock will
> therefore fail, calling die(). Which would trigger our atexit() to call 
> process_log(),
> which would hit this code again, and so forth. I'm not sure if we'd actually
> recurse when an atexit handler calls exit(). But it seems questionable.

No, because  we call rollback_log_file first.

> I'm also not sure why we need to re-open the file in the first place. We have 
> an
> open descriptor (and we even redirected stderr to it already).
> Why don't we just write to it?

If fstat failed, that probably indicates something bad about the old fd.  I'm 
not 
actually sure why fstat would ever fail, since in all likelihood, the kernel 
keeps 
information about inodes corresponding to open fds in-memory.  Maybe someone
forcibly unmounted the drive?  

> > @@ -113,6 +151,9 @@ static void gc_config(void)
> > git_config_get_bool("gc.autodetach", _auto);
> > git_config_date_string("gc.pruneexpire", _expire);
> > git_config_date_string("gc.worktreepruneexpire",
> > _worktrees_expire);
> > +   if (!git_config_get_value("gc.logexpiry", ))
> > +   parse_expiry_date(value, _log_expire_time);
> > +
> 
> Should you be using git_config_date_string() here? It looks like it does some
> extra sanity-checking. It annoyingly just gets the string, and doesn't parse 
> it.
> Perhaps it would be worth adding a
> git_config_date_value() helper.

That seems like a good idea, but I'm going to skip it for now and promise to
do it next time I need a date config.

> Or alternatively, save the date string here, and then parse once later on, 
> after
> having resolved all config (and overwritten the default value).

Sure.

> > @@ -448,5 +506,8 @@ int cmd_gc(int argc, const char **argv, const char
> *prefix)
> > warning(_("There are too many unreachable loose objects; "
> > "run 'git prune' to remove them."));
> >
> > +   if (!daemonized)
> > +   unlink(git_path("gc.log"));
> > +
> 
> I think this is a good thing to do, though I'd have probably put it in a 
> separate
> patch. I guess that's a matter of taste.

I could go either way, but since I've already 

[PATCH v5] gc: ignore old gc.log files

[David Turner]

A server can end up in a state where there are lots of unreferenced
loose objects (say, because many users are doing a bunch of rebasing
and pushing their rebased branches).  Running "git gc --auto" in
this state would cause a gc.log file to be created, preventing
future auto gcs, causing pack files to pile up.  Since many git
operations are O(n) in the number of pack files, this would lead to
poor performance.

Git should never get itself into a state where it refuses to do any
maintenance, just because at some point some piece of the maintenance
didn't make progress.

Teach Git to ignore gc.log files which are older than (by default)
one day old, which can be tweaked via the gc.logExpiry configuration
variable.  That way, these pack files will get cleaned up, if
necessary, at least once per day.  And operators who find a need for
more-frequent gcs can adjust gc.logExpiry to meet their needs.

There is also some cleanup: a successful manual gc, or a
warning-free auto gc with an old log file, will remove any old
gc.log files.

It might still happen that manual intervention is required
(e.g. because the repo is corrupt), but at the very least it won't
be because Git is too dumb to try again.

Signed-off-by: David Turner 
Helped-by: Jeff King 
Signed-off-by: Junio C Hamano 
---
 Documentation/config.txt |  6 +
 builtin/gc.c | 59 ++--
 t/t6500-gc.sh| 15 
 3 files changed, 73 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fc5a28a32..a684b7e3e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1402,6 +1402,12 @@ gc.autoDetach::
Make `git gc --auto` return immediately and run in background
if the system supports it. Default is true.
 
+gc.logExpiry::
+   If the file gc.log exists, then `git gc --auto` won't run
+   unless that file is more than 'gc.logExpiry' old.  Default is
+   "1.day".  See `gc.pruneExpire` for more ways to specify its
+   value.
+
 gc.packRefs::
Running `git pack-refs` in a repository renders it
unclonable by Git versions prior to 1.5.1.2 over dumb
diff --git a/builtin/gc.c b/builtin/gc.c
index 331f21926..8d355feb0 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -33,6 +33,8 @@ static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
 static int detach_auto = 1;
+static unsigned long gc_log_expire_time;
+static const char *gc_log_expire = "1.day.ago";
 static const char *prune_expire = "2.weeks.ago";
 static const char *prune_worktrees_expire = "3.months.ago";
 
@@ -76,10 +78,30 @@ static void git_config_date_string(const char *key, const 
char **output)
 static void process_log_file(void)
 {
struct stat st;
-   if (!fstat(get_lock_file_fd(_lock), ) && st.st_size)
+   if (fstat(get_lock_file_fd(_lock), )) {
+   /*
+* Perhaps there was an i/o error or another
+* unlikely situation.  Try to make a note of
+* this in gc.log along with any existing
+* messages.
+*/
+   FILE *fp;
+   int saved_errno = errno;
+   fp = fdopen(log_lock.tempfile.fd, "a");
+   fprintf(fp, _("Failed to fstat %s: %s"),
+   get_tempfile_path(_lock.tempfile),
+   strerror(saved_errno));
+   fclose(fp);
commit_lock_file(_lock);
-   else
+   errno = saved_errno;
+   } else if (st.st_size) {
+   /* There was some error recorded in the lock file */
+   commit_lock_file(_lock);
+   } else {
+   /* No error, clean up any old gc.log */
+   unlink(git_path("gc.log"));
rollback_lock_file(_lock);
+   }
 }
 
 static void process_log_file_at_exit(void)
@@ -113,6 +135,8 @@ static void gc_config(void)
git_config_get_bool("gc.autodetach", _auto);
git_config_date_string("gc.pruneexpire", _expire);
git_config_date_string("gc.worktreepruneexpire", 
_worktrees_expire);
+   git_config_date_string("gc.logexpiry", _log_expire);
+
git_config(git_default_config, NULL);
 }
 
@@ -290,19 +314,34 @@ static const char *lock_repo_for_gc(int force, pid_t* 
ret_pid)
 static int report_last_gc_error(void)
 {
struct strbuf sb = STRBUF_INIT;
-   int ret;
+   int ret = 0;
+   struct stat st;
+   char *gc_log_path = git_pathdup("gc.log");
 
-   ret = strbuf_read_file(, git_path("gc.log"), 0);
+   if (stat(gc_log_path, )) {
+   if (errno == ENOENT)
+   goto done;
+
+   ret = error_errno(_("Can't stat %s"), gc_log_path);
+   goto done;
+   }
+
+   if (st.st_mtime < gc_log_expire_time)
+   goto 

Re: fuzzy patch application

[Jeff King]

On Fri, Feb 10, 2017 at 11:20:59AM -0800, Nick Desaulniers wrote:

> I frequently need to backport patches from the Linux kernel to older
> kernel versions (Android Security).  My usual workflow for simple
> patches is:
> 
> 1. try `git am patch.txt`.

This is not exactly an answer to your question, but "git am -3" is often
a better solution than trying to fuzz patches. It assumes the patches
are Git patches (and record their origin blobs), and that you have that
blob (which should be true if the patches are based on the normal kernel
history, and you just fetch that history into your repository).

I've found that this often manages to apply patches that "git apply"
will not by itself. And I also find the resulting conflicts to be much
easier to deal with than patch's ".rej" files.

-Peff

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

[Jeff King]

On Fri, Feb 10, 2017 at 08:44:27PM +, David Turner wrote:

> > But maybe there are other systems emulating fstat() would trigger here.
> > I dunno.
> 
> Yeah, I'm also not sure.  On the other hand, if we're going to catch fstat 
> errors anyway, we might as well do something sensible with this one.

I'd say it's probably not worth worrying about here. We don't think it
can happen, and it would just fall-through to the "woah, fstat failed"
code path if it does.

> > If there was an i/o error, then gc.log.lock still exists. And this attempt 
> > to lock will
> > therefore fail, calling die(). Which would trigger our atexit() to call 
> > process_log(),
> > which would hit this code again, and so forth. I'm not sure if we'd actually
> > recurse when an atexit handler calls exit(). But it seems questionable.
> 
> No, because  we call rollback_log_file first.

Ah, right, sorry I missed that.

> > I'm also not sure why we need to re-open the file in the first place. We 
> > have an
> > open descriptor (and we even redirected stderr to it already).
> > Why don't we just write to it?
> 
> If fstat failed, that probably indicates something bad about the old fd.  I'm 
> not 
> actually sure why fstat would ever fail, since in all likelihood, the kernel 
> keeps 
> information about inodes corresponding to open fds in-memory.  Maybe someone
> forcibly unmounted the drive?

It seems like the re-open would fail then, too. And the error message
for that would go to stderr, which goes to...the old file.

I dunno. This seems like a lot of manual scrambling to try to overcome
unlikely errors just to make our stderr heard (and we'd still fail in
some cases). It seems like:

  if (fstat(...)) {
/* weird, fstat failed; try our best to mention it */
error_errno("unable to fstat gc.log.lock");
commit_lock_file(_lock));
  } else if (st.st_size) {
/* we have new errors to report */
commit_lock_file(_lock);
  } else {
/* no new errors; clean up old ones */
unlink(git_path("gc.log"));
rollback_lock_file(_lock);
  }

would be sufficient.

-Peff

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[René Scharfe]

Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:

It is curious that only MacOSX builds trigger an error about this, both
GCC and Clang, but not Linux GCC nor Clang (see
https://travis-ci.org/git/git/jobs/200182819#L1152 for details):

builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
uninitialized whenever 'if' condition is true
[-Werror,-Wsometimes-uninitialized]
if (missing_good && !missing_bad && current_term &&
^~~
builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
if (!good_syn)
 ^~~~


The only way that good_syn could be used in the if block is by going to 
the label finish, which does the following before returning:


if (!bad_ref)
free(bad_ref);
if (!good_glob)
free(good_glob);
if (!bad_syn)
free(bad_syn);
if (!good_syn)
free(good_syn);

On Linux that code is elided completely -- freeing NULL is a no-op.  I 
guess free(3) has different attributes on OS X and compilers don't dare 
to optimize it away there.


So instead of calling free(3) only in the case when we did not allocate 
memory (which makes no sense and leaks) we should either call it in the 
opposite case, or (preferred) unconditionally, as it can handle the NULL 
case itself.  Once that's fixed initialization will be required even on 
Linux.


René

Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory

[Junio C Hamano]

Johannes Schindelin  writes:

> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index 292a0720fcc..1d6e27a09d8 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -200,4 +200,21 @@ EOF
>   test_cmp expect actual
>  '
>  
> +test_expect_failure 'rev-parse --shared-index-path' '
> + rm -rf .git &&
> + test_create_repo . &&

Another thing that I notice only after merging this and other topics
to 'pu' was that this piece needs to always come at the end of the
script because of this removal.  It would make the test more robust
to create a test repository for this test and work inside it.

> + git update-index --split-index &&
> + ls -t .git/sharedindex* | tail -n 1 >expect &&
> + git rev-parse --shared-index-path >actual &&
> + test_cmp expect actual &&
> + mkdir work &&
> + test_when_finished "rm -rf work" &&
> + (
> + cd work &&
> + ls -t ../.git/sharedindex* | tail -n 1 >expect &&
> + git rev-parse --shared-index-path >actual &&
> + test_cmp expect actual
> + )
> +'
> +
>  test_done

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

[Jeff King]

On Fri, Feb 10, 2017 at 02:20:19PM -0500, David Turner wrote:

> @@ -76,10 +77,47 @@ static void git_config_date_string(const char *key, const 
> char **output)
>  static void process_log_file(void)
>  {
>   struct stat st;
> - if (!fstat(get_lock_file_fd(_lock), ) && st.st_size)
> + if (fstat(get_lock_file_fd(_lock), )) {
> + if (errno == ENOENT) {
> + /*
> +  * The user has probably intentionally deleted
> +  * gc.log.lock (perhaps because they're blowing
> +  * away the whole repo), so thre's no need to
> +  * report anything here.  But we also won't
> +  * delete gc.log, because we don't know what
> +  * the user's intentions are.
> +  */

Hrm. Does fstat actually trigger ENOENT in that case? There's no
pathname lookup happening at all. A simple test on Linux seems to show
that it does not. Build:

#include 
#include 
#include 

int main(int argc, char **argv)
{
struct stat st;
int fd = open(argv[1], O_WRONLY|O_CREAT|O_EXCL, 0600);
unlink(argv[1]);
fstat(fd, );
return 0;
}

and run:

  strace ./a.out tmp

which shows:

  open("tmp", O_WRONLY|O_CREAT|O_EXCL, 056660) = 3
  unlink("tmp")   = 0
  fstat(3, {st_mode=S_IFREG|S_ISUID|S_ISGID|0640, st_size=0, ...}) = 0

But maybe there are other systems emulating fstat() would trigger here.
I dunno.

> + } else {
> + FILE *fp;
> + int fd;
> + int saved_errno = errno;
> + /*
> +  * Perhaps there was an i/o error or another
> +  * unlikely situation.  Try to make a note of
> +  * this in gc.log.  If this fails again,
> +  * give up and leave gc.log as it was.
> +  */
> + rollback_lock_file(_lock);
> + fd = hold_lock_file_for_update(_lock,
> +git_path("gc.log"),
> +LOCK_DIE_ON_ERROR);

If there was an i/o error, then gc.log.lock still exists. And this
attempt to lock will therefore fail, calling die(). Which would trigger
our atexit() to call process_log(), which would hit this code again, and
so forth. I'm not sure if we'd actually recurse when an atexit handler
calls exit(). But it seems questionable.

I'm also not sure why we need to re-open the file in the first place. We
have an open descriptor (and we even redirected stderr to it already).
Why don't we just write to it?

> @@ -113,6 +151,9 @@ static void gc_config(void)
>   git_config_get_bool("gc.autodetach", _auto);
>   git_config_date_string("gc.pruneexpire", _expire);
>   git_config_date_string("gc.worktreepruneexpire", 
> _worktrees_expire);
> + if (!git_config_get_value("gc.logexpiry", ))
> + parse_expiry_date(value, _log_expire_time);
> +

Should you be using git_config_date_string() here? It looks like it does
some extra sanity-checking. It annoyingly just gets the string, and
doesn't parse it. Perhaps it would be worth adding a
git_config_date_value() helper.

Or alternatively, save the date string here, and then parse once later
on, after having resolved all config (and overwritten the default
value).

> @@ -448,5 +506,8 @@ int cmd_gc(int argc, const char **argv, const char 
> *prefix)
>   warning(_("There are too many unreachable loose objects; "
>   "run 'git prune' to remove them."));
>  
> + if (!daemonized)
> + unlink(git_path("gc.log"));
> +

I think this is a good thing to do, though I'd have probably put it in a
separate patch. I guess that's a matter of taste.

> +test_expect_success 'background auto gc does not run if gc.log is present 
> and recent but does if it is old' '
> + keep=$(ls .git/objects/pack/*.pack|head -1|sed -e "s/pack$/keep/") &&
> + test_commit foo &&
> + test_commit bar &&
> + git repack &&
> + test_config gc.autopacklimit 1 &&
> + test_config gc.autodetach true &&
> + echo fleem >.git/gc.log &&
> + test_must_fail git gc --auto 2>err &&
> + test_i18ngrep "^error:" err &&
> + test-chmtime =-86401 .git/gc.log &&
> + git gc --auto
> +'

This gives only 1 second of leeway. I wonder if we could end up getting
bogus failures due to system clock adjustments, or even skew between the
filesystem and OS clocks. Perhaps we should set it farther back, like a
few days.

(It also relies on the 1-day default. That's probably OK, though we
could also set an explicit default for the test, which would exercise
the config code path, too).

-Peff

[PATCH 2/2] ls-files: move only kept cache entries in prune_cache()

[René Scharfe]

prune_cache() first identifies those entries at the start of the sorted
array that can be discarded.  Then it moves the rest of the entries up.
Last it identifies the unwanted trailing entries among the moved ones
and cuts them off.

Change the order: Identify both start *and* end of the range to keep
first and then move only those entries to the top.  The resulting code
is slightly shorter and a bit more efficient.

Signed-off-by: Rene Scharfe 
---
The performance impact is probably only measurable with a *really* big
index.

 builtin/ls-files.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 18105ec7ea..1c0f057d02 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -379,10 +379,7 @@ static void prune_cache(const char *prefix, size_t 
prefixlen)
pos = cache_name_pos(prefix, prefixlen);
if (pos < 0)
pos = -pos-1;
-   memmove(active_cache, active_cache + pos,
-   (active_nr - pos) * sizeof(struct cache_entry *));
-   active_nr -= pos;
-   first = 0;
+   first = pos;
last = active_nr;
while (last > first) {
int next = (last + first) >> 1;
@@ -393,7 +390,9 @@ static void prune_cache(const char *prefix, size_t 
prefixlen)
}
last = next;
}
-   active_nr = last;
+   memmove(active_cache, active_cache + pos,
+   (last - pos) * sizeof(struct cache_entry *));
+   active_nr = last - pos;
 }
 
 /*
-- 
2.11.1

[PATCH v4] gc: ignore old gc.log files

[David Turner]

Git should never get itself into a state where it refuses to do any
maintenance, just because at some point some piece of the maintenance
didn't make progress.

In this commit, git learns to ignore gc.log files which are older than
(by default) one day old.  It also learns about a config, gc.logExpiry
to manage this.  There is also some cleanup: a successful manual gc,
or a warning-free auto gc, will remove any old gc.log files.

It might still happen that manual intervention is required
(e.g. because the repo is corrupt), but at the very least it won't be
because Git is too dumb to try again.

Automatic gc was intended to make client repositories be
self-maintaining.  It would be good if automatic gc were also useful
to server operators.  A server can end up in a state whre there are
lots of unreferenced loose objects (say, because many users are doing
a bunch of rebasing and pushing their rebased branches). Before this
patch, this state would cause a gc.log file to be created, preventing
future auto gcs.  Then pack files could pile up.  Since many git
operations are O(n) in the number of pack files, this would lead to
poor performance.  Now, the pack files will get cleaned up, if
necessary, at least once per day.  And operators who find a need for
more-frequent gcs can adjust gc.logExpiry to meet their needs.

Signed-off-by: David Turner 
Helped-by: Jeff King 
---
 Documentation/config.txt |  6 
 builtin/gc.c | 76 +++-
 t/t6500-gc.sh| 12 
 3 files changed, 87 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fc5a28a32..a684b7e3e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1402,6 +1402,12 @@ gc.autoDetach::
Make `git gc --auto` return immediately and run in background
if the system supports it. Default is true.
 
+gc.logExpiry::
+   If the file gc.log exists, then `git gc --auto` won't run
+   unless that file is more than 'gc.logExpiry' old.  Default is
+   "1.day".  See `gc.pruneExpire` for more ways to specify its
+   value.
+
 gc.packRefs::
Running `git pack-refs` in a repository renders it
unclonable by Git versions prior to 1.5.1.2 over dumb
diff --git a/builtin/gc.c b/builtin/gc.c
index 331f21926..55c441115 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -33,6 +33,7 @@ static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
 static int detach_auto = 1;
+static unsigned long gc_log_expire_time;
 static const char *prune_expire = "2.weeks.ago";
 static const char *prune_worktrees_expire = "3.months.ago";
 
@@ -76,10 +77,48 @@ static void git_config_date_string(const char *key, const 
char **output)
 static void process_log_file(void)
 {
struct stat st;
-   if (!fstat(get_lock_file_fd(_lock), ) && st.st_size)
+   if (fstat(get_lock_file_fd(_lock), )) {
+   if (errno == ENOENT) {
+   /*
+* The user has probably intentionally deleted
+* gc.log.lock (perhaps because they're blowing
+* away the whole repo), so thre's no need to
+* report anything here.  But we also won't
+* delete gc.log, because we don't know what
+* the user's intentions are.
+*/
+   } else {
+   FILE *fp;
+   int fd;
+   int saved_errno = errno;
+   /*
+* Perhaps there was an i/o error or another
+* unlikely situation.  Try to make a note of
+* this in gc.log.  If this fails again,
+* give up and leave gc.log as it was.
+*/
+   rollback_lock_file(_lock);
+   fd = hold_lock_file_for_update(_lock,
+  git_path("gc.log"),
+  LOCK_DIE_ON_ERROR);
+
+   fp = fdopen(fd, "w");
+   fprintf(fp, _("Failed to fstat %s: %s"),
+   get_tempfile_path(_lock.tempfile),
+   strerror(saved_errno));
+   fclose(fp);
+   commit_lock_file(_lock);
+   errno = saved_errno;
+   }
+
+   } else if (st.st_size) {
+   /* There was some error recorded in the lock file */
commit_lock_file(_lock);
-   else
+   } else {
+   /* No error, clean up any old gc.log */
+   unlink(git_path("gc.log"));
rollback_lock_file(_lock);
+   }
 }
 
 static void 

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

[Junio C Hamano]

Junio C Hamano  writes:

> David Turner  writes:
> ...
>> It might still happen that manual intervention is required
>> (e.g. because the repo is corrupt), but at the very least it won't be
>> because Git is too dumb to try again.
>
> Thanks, nicely explained.

Sorry but I spotted a typo "whre" and ended up updating the proposed
log message by reordering them, omitting redundant info, etc.  Here
is a proposed amend with the "where does saved-errno go?" and "we do
not seem to use keep" fix-ups squashed in.

-- >8 --
Subject: gc: ignore old gc.log files

A server can end up in a state where there are lots of unreferenced
loose objects (say, because many users are doing a bunch of rebasing
and pushing their rebased branches).  Running "git gc --auto" in
this state would cause a gc.log file to be created, preventing
future auto gcs, causing pack files to pile up.  Since many git
operations are O(n) in the number of pack files, this would lead to
poor performance.

Git should never get itself into a state where it refuses to do any
maintenance, just because at some point some piece of the maintenance
didn't make progress.

Teach Git to ignore gc.log files which are older than (by default)
one day old, which can be tweaked via the gc.logExpiry configuration
variable.  That way, these pack files will get cleaned up, if
necessary, at least once per day.  And operators who find a need for
more-frequent gcs can adjust gc.logExpiry to meet their needs.

There is also some cleanup: a successful manual gc, or a
warning-free auto gc with an old log file, will remove any old
gc.log files.

It might still happen that manual intervention is required
(e.g. because the repo is corrupt), but at the very least it won't
be because Git is too dumb to try again.

Signed-off-by: David Turner 
Helped-by: Jeff King 
Signed-off-by: Junio C Hamano 
---
 Documentation/config.txt |  6 
 builtin/gc.c | 76 +++-
 t/t6500-gc.sh| 12 
 3 files changed, 87 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1fee83ca42..d385711b70 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1397,6 +1397,12 @@ gc.autoDetach::
Make `git gc --auto` return immediately and run in background
if the system supports it. Default is true.
 
+gc.logExpiry::
+   If the file gc.log exists, then `git gc --auto` won't run
+   unless that file is more than 'gc.logExpiry' old.  Default is
+   "1.day".  See `gc.pruneExpire` for more ways to specify its
+   value.
+
 gc.packRefs::
Running `git pack-refs` in a repository renders it
unclonable by Git versions prior to 1.5.1.2 over dumb
diff --git a/builtin/gc.c b/builtin/gc.c
index 331f219260..55c441115d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -33,6 +33,7 @@ static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
 static int detach_auto = 1;
+static unsigned long gc_log_expire_time;
 static const char *prune_expire = "2.weeks.ago";
 static const char *prune_worktrees_expire = "3.months.ago";
 
@@ -76,10 +77,48 @@ static void git_config_date_string(const char *key, const 
char **output)
 static void process_log_file(void)
 {
struct stat st;
-   if (!fstat(get_lock_file_fd(_lock), ) && st.st_size)
+   if (fstat(get_lock_file_fd(_lock), )) {
+   if (errno == ENOENT) {
+   /*
+* The user has probably intentionally deleted
+* gc.log.lock (perhaps because they're blowing
+* away the whole repo), so thre's no need to
+* report anything here.  But we also won't
+* delete gc.log, because we don't know what
+* the user's intentions are.
+*/
+   } else {
+   FILE *fp;
+   int fd;
+   int saved_errno = errno;
+   /*
+* Perhaps there was an i/o error or another
+* unlikely situation.  Try to make a note of
+* this in gc.log.  If this fails again,
+* give up and leave gc.log as it was.
+*/
+   rollback_lock_file(_lock);
+   fd = hold_lock_file_for_update(_lock,
+  git_path("gc.log"),
+  LOCK_DIE_ON_ERROR);
+
+   fp = fdopen(fd, "w");
+   fprintf(fp, _("Failed to fstat %s: %s"),
+   get_tempfile_path(_lock.tempfile),
+   

[PATCH 1/2] ls-files: pass prefix length explicitly to prune_cache()

[René Scharfe]

The function prune_cache() relies on the fact that it is only called on
max_prefix and sneakily uses the matching global variable max_prefix_len
directly.  Tighten its interface by passing both the string and its
length as parameters.  While at it move the NULL check into the function
to collect all cache-pruning related logic in one place.

Signed-off-by: Rene Scharfe 
---
Not urgent, of course.

 builtin/ls-files.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 1592290815..18105ec7ea 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -369,11 +369,14 @@ static void show_files(struct dir_struct *dir)
 /*
  * Prune the index to only contain stuff starting with "prefix"
  */
-static void prune_cache(const char *prefix)
+static void prune_cache(const char *prefix, size_t prefixlen)
 {
-   int pos = cache_name_pos(prefix, max_prefix_len);
+   int pos;
unsigned int first, last;
 
+   if (!prefix)
+   return;
+   pos = cache_name_pos(prefix, prefixlen);
if (pos < 0)
pos = -pos-1;
memmove(active_cache, active_cache + pos,
@@ -384,7 +387,7 @@ static void prune_cache(const char *prefix)
while (last > first) {
int next = (last + first) >> 1;
const struct cache_entry *ce = active_cache[next];
-   if (!strncmp(ce->name, prefix, max_prefix_len)) {
+   if (!strncmp(ce->name, prefix, prefixlen)) {
first = next+1;
continue;
}
@@ -641,8 +644,7 @@ int cmd_ls_files(int argc, const char **argv, const char 
*cmd_prefix)
  show_killed || show_modified || show_resolve_undo))
show_cached = 1;
 
-   if (max_prefix)
-   prune_cache(max_prefix);
+   prune_cache(max_prefix, max_prefix_len);
if (with_tree) {
/*
 * Basic sanity check; show-stages and show-unmerged
-- 
2.11.1

Re: [PATCH] dir: avoid allocation in fill_directory()

[René Scharfe]

Am 08.02.2017 um 07:22 schrieb Duy Nguyen:

On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe  wrote:

Pass the match member of the first pathspec item directly to
read_directory() instead of using common_prefix() to duplicate it first,
thus avoiding memory duplication, strlen(3) and free(3).


How about killing common_prefix()? There are two other callers in
ls-files.c and commit.c and it looks safe to do (but I didn't look
very hard).


I would like that, but it doesn't look like it's worth it.  I have a 
patch series for making overlay_tree_on_cache() take pointer+length, but 
it's surprisingly long and bloats the code.  Duplicating a small piece 
of memory once per command doesn't look so bad in comparison.


(The payoff for avoiding an allocation is higher for library functions 
like fill_directory().)


But while working on that I found two opportunities for improvement in 
prune_cache().  I'll send patches shortly.



There's a subtle difference. Before the patch, prefix[prefix_len] is
NUL. After the patch, it's not always true. If some code (incorrectly)
depends on that, this patch exposes it. I had a look inside
read_directory() though and it looks like no such code exists. So, all
good.


Thanks for checking.

NB: The code before 966de302 (dir: convert fill_directory to use the 
pathspec struct interface, committed 2017-01-04) made the same 
assumption, i.e. that NUL is not needed.


René

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

[Junio C Hamano]

David Turner  writes:

> It would be good if automatic gc were useful to server operators.
> A server can end up in a state whre there are
> lots of unreferenced loose objects (say, because many users are doing
> a bunch of rebasing and pushing their rebased branches). Before this
> patch, this state would cause a gc.log file to be created, preventing
> future auto gcs.  Then pack files could pile up.  Since many git
> operations are O(n) in the number of pack files, this would lead to
> poor performance.  Now, the pack files will get cleaned up, if
> necessary, at least once per day.  And operators who find a need for
> more-frequent gcs can adjust gc.logExpiry to meet their needs.
>
> Git should never get itself into a state where it refuses to do any
> maintenance, just because at some point some piece of the maintenance
> didn't make progress.
>
> In this commit, git learns to ignore gc.log files which are older than
> (by default) one day old.  It also learns about a config, gc.logExpiry
> to manage this.  There is also some cleanup: a successful manual gc,
> or a warning-free auto gc with an old log file, will remove any old
> gc.log files.
>
> It might still happen that manual intervention is required
> (e.g. because the repo is corrupt), but at the very least it won't be
> because Git is too dumb to try again.

Thanks, nicely explained.

> + if (fstat(get_lock_file_fd(_lock), )) {
> + if (errno == ENOENT) {
> + /*
> +  * The user has probably intentionally deleted
> +  * gc.log.lock (perhaps because they're blowing
> +  * away the whole repo), so thre's no need to
> +  * report anything here.  But we also won't
> +  * delete gc.log, because we don't know what
> +  * the user's intentions are.
> +  */

OK.

> + } else {
> + FILE *fp;
> + int fd;
> + int saved_errno = errno;
> + /*
> +  * Perhaps there was an i/o error or another
> +  * unlikely situation.  Try to make a note of
> +  * this in gc.log.  If this fails again,
> +  * give up and leave gc.log as it was.
> +  */
> + rollback_lock_file(_lock);
> + fd = hold_lock_file_for_update(_lock,
> +git_path("gc.log"),
> +LOCK_DIE_ON_ERROR);
> +
> + fp = fdopen(fd, "w");
> + fprintf(fp, _("Failed to fstat %s: %s"),
> + get_tempfile_path(_lock.tempfile),
> + strerror(errno));

I think you meant to use saved_errno in this message and then

> + fclose(fp);
> + commit_lock_file(_lock);

possibly assign it back to errno around here?

> + }
> +
> + } else if (st.st_size) {
> + /* There was some error recorded in the lock file */
>   commit_lock_file(_lock);
> - else
> + } else {
> + /* No error, clean up any old gc.log */
> + unlink(git_path("gc.log"));
>   rollback_lock_file(_lock);
> + }
>  }

> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> index 1762dfa6a..84ad07eb2 100755
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
> @@ -67,5 +67,18 @@ test_expect_success 'auto gc with too many loose objects 
> does not attempt to cre
>   test_line_count = 2 new # There is one new pack and its .idx
>  '
>  
> +test_expect_success 'background auto gc does not run if gc.log is present 
> and recent but does if it is old' '
> + keep=$(ls .git/objects/pack/*.pack|head -1|sed -e "s/pack$/keep/") &&

You could save one process with:

ls .git/objects/pack/*.pack | sed -e "s/pack$/keep/" -e q

but do you even need $keep?  I do not see it used below.

> + test_commit foo &&
> + test_commit bar &&
> + git repack &&
> + test_config gc.autopacklimit 1 &&
> + test_config gc.autodetach true &&
> + echo fleem >.git/gc.log &&
> + test_must_fail git gc --auto 2>err &&
> + test_i18ngrep "^error:" err &&
> + test-chmtime =-86401 .git/gc.log &&
> + git gc --auto
> +'
>  
>  test_done

Other than that I didn't spot anything suspicious.  I'll wait to see
what others may want to add.

Thanks.

Re: fuzzy patch application

[Junio C Hamano]

Nick Desaulniers  writes:

> I frequently need to backport patches from the Linux kernel to older
> kernel versions (Android Security)
> ...
> My question is, why does `patch` seem to do a better job at applying
> patches than `git am`?  It's almost like the `git` tools don't try to fuzz
> the offsets.

You diagnosed correctly.  We do allow offsets but by default no fuzz
and that is a deliberate design decision made in very early days.
You can pass option to reduce context, but that is not the default.

Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list

[Junio C Hamano]

Michael Haggerty  writes:

> This is v2 of the patch series, considerably reorganized but not that
> different codewise.

Thanks.  The way the series loses "!*submodule and !submodule are
the same thing, sometimes" is easier to follow when presented in
this order, at least to me.

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

[Junio C Hamano]

Michael Haggerty  writes:

> There is no need to call read_ref_full() or resolve_gitlink_ref() from
> read_loose_refs(), because we already have a ref_store object in hand.
> So we can call resolve_ref_recursively() ourselves. Happily, this
> unifies the code for the submodule vs. non-submodule cases.
>
> This requires resolve_ref_recursively() to be exposed to the refs
> subsystem, though not to non-refs code.
>
> Signed-off-by: Michael Haggerty 
> ---
>  refs.c   | 50 +-
>  refs/files-backend.c | 18 --
>  refs/refs-internal.h |  5 +
>  3 files changed, 34 insertions(+), 39 deletions(-)

OK, but one thing puzzles me...

> @@ -1390,27 +1390,6 @@ 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.
> - */
> -static struct ref_store *lookup_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;
> -
> - entry = hashmap_get_from_hash(_ref_stores,
> -   strhash(submodule), submodule);
> - return entry ? entry->refs : NULL;
> -}
> -
> -/*
>   * 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.
> @@ -1451,6 +1430,27 @@ static struct ref_store *ref_store_init(const char 
> *submodule)
>   return refs;
>  }
>  
> +/*
> + * 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.
> + */
> +static struct ref_store *lookup_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;
> +
> + entry = hashmap_get_from_hash(_ref_stores,
> +   strhash(submodule), submodule);
> + return entry ? entry->refs : NULL;
> +}
> +

I somehow thought that we had an early "reorder the code" step to
avoid hunks like these?  Am I missing some subtle changes made while
moving the function down?

fuzzy patch application

[Nick Desaulniers]

I frequently need to backport patches from the Linux kernel to older
kernel versions (Android Security).  My usual workflow for simple
patches is:

1. try `git am patch.txt`.
2. if that fails try `git apply -v patch.txt` then add commit message
manually.
3. if that fails try `patch -p1 < patch.txt` then add commit message
manually.
4. if that fails, manually fix bug based on patch.

My question is, why does `patch` seem to do a better job at applying
patches than `git am`?  It's almost like the `git` tools don't try to fuzz
the offsets.  Is there a way to tell git to try fuzzing offsets when
applying patches?

>From the output of `patch` I ran recently (for a patch that
`git apply` would not apply):

patching file 
Hunk #1 succeeded at 113 (offset -1 lines).
Hunk #2 succeeded at 435 (offset -3 lines).
Hunk #3 succeeded at 693 (offset 2 lines).
Hunk #4 succeeded at 1535 (offset -41 lines).
Hunk #5 succeeded at 1551 (offset -41 lines).
Hunk #6 succeeded at 1574 with fuzz 2 (offset -42 lines).
Hunk #7 succeeded at 1614 (offset -42 lines).

In fact, `git apply -v` errors for hunk #6.

I would guess maybe that there's an upper limit on the offset searched?
Also, I'm not sure what the `fuzz 2` part means exactly, but it seems like
`git apply` chokes when fuzzing is needed to properly apply a patch.

http://stackoverflow.com/q/6336440/1027966

-- 
Thanks,
~Nick Desaulniers

[PATCH v3] gc: ignore old gc.log files

[David Turner]

Git should never get itself into a state where it refuses to do any
maintenance, just because at some point some piece of the maintenance
didn't make progress.

In this commit, git learns to ignore gc.log files which are older than
(by default) one day old.  It also learns about a config, gc.logExpiry
to manage this.  There is also some cleanup: a successful manual gc,
or a warning-free auto gc with an old log file, will remove any old
gc.log files.

It might still happen that manual intervention is required
(e.g. because the repo is corrupt), but at the very least it won't be
because Git is too dumb to try again.

Automatic gc was intended to make client repositories be
self-maintaining.  It would be good if automatic gc were also useful
to server operators.  A server can end up in a state whre there are
lots of unreferenced loose objects (say, because many users are doing
a bunch of rebasing and pushing their rebased branches). Before this
patch, this state would cause a gc.log file to be created, preventing
future auto gcs.  Then pack files could pile up.  Since many git
operations are O(n) in the number of pack files, this would lead to
poor performance.  Now, the pack files will get cleaned up, if
necessary, at least once per day.  And operators who find a need for
more-frequent gcs can adjust gc.logExpiry to meet their needs.

Signed-off-by: David Turner 
Helped-by: Jeff King 
---
 Documentation/config.txt |  6 
 builtin/gc.c | 75 +++-
 t/t6500-gc.sh| 13 +
 3 files changed, 87 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fc5a28a32..a684b7e3e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1402,6 +1402,12 @@ gc.autoDetach::
Make `git gc --auto` return immediately and run in background
if the system supports it. Default is true.
 
+gc.logExpiry::
+   If the file gc.log exists, then `git gc --auto` won't run
+   unless that file is more than 'gc.logExpiry' old.  Default is
+   "1.day".  See `gc.pruneExpire` for more ways to specify its
+   value.
+
 gc.packRefs::
Running `git pack-refs` in a repository renders it
unclonable by Git versions prior to 1.5.1.2 over dumb
diff --git a/builtin/gc.c b/builtin/gc.c
index 331f21926..6f297aa91 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -33,6 +33,7 @@ static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
 static int detach_auto = 1;
+static unsigned long gc_log_expire_time;
 static const char *prune_expire = "2.weeks.ago";
 static const char *prune_worktrees_expire = "3.months.ago";
 
@@ -76,10 +77,47 @@ static void git_config_date_string(const char *key, const 
char **output)
 static void process_log_file(void)
 {
struct stat st;
-   if (!fstat(get_lock_file_fd(_lock), ) && st.st_size)
+   if (fstat(get_lock_file_fd(_lock), )) {
+   if (errno == ENOENT) {
+   /*
+* The user has probably intentionally deleted
+* gc.log.lock (perhaps because they're blowing
+* away the whole repo), so thre's no need to
+* report anything here.  But we also won't
+* delete gc.log, because we don't know what
+* the user's intentions are.
+*/
+   } else {
+   FILE *fp;
+   int fd;
+   int saved_errno = errno;
+   /*
+* Perhaps there was an i/o error or another
+* unlikely situation.  Try to make a note of
+* this in gc.log.  If this fails again,
+* give up and leave gc.log as it was.
+*/
+   rollback_lock_file(_lock);
+   fd = hold_lock_file_for_update(_lock,
+  git_path("gc.log"),
+  LOCK_DIE_ON_ERROR);
+
+   fp = fdopen(fd, "w");
+   fprintf(fp, _("Failed to fstat %s: %s"),
+   get_tempfile_path(_lock.tempfile),
+   strerror(errno));
+   fclose(fp);
+   commit_lock_file(_lock);
+   }
+
+   } else if (st.st_size) {
+   /* There was some error recorded in the lock file */
commit_lock_file(_lock);
-   else
+   } else {
+   /* No error, clean up any old gc.log */
+   unlink(git_path("gc.log"));
rollback_lock_file(_lock);
+   }
 }
 
 static void process_log_file_at_exit(void)
@@ -113,6 +151,9 @@ 

Re: [PATCH v2 1/9] refs: reorder some function definitions

[Junio C Hamano]

Michael Haggerty  writes:

> This avoids the need to add forward declarations in the next step.
>
> Signed-off-by: Michael Haggerty 
> ---
>  refs.c | 64 
>  1 file changed, 32 insertions(+), 32 deletions(-)

Makes sense, but the patch itself looks like ... unreadble ;-)

>
> diff --git a/refs.c b/refs.c
> index 9bd0bc1..707092f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1358,27 +1358,19 @@ static struct ref_store *main_ref_store;
>  /* A linked list of ref_stores for submodules: */
>  static struct ref_store *submodule_ref_stores;
>  
> -void base_ref_store_init(struct ref_store *refs,
> -  const struct ref_storage_be *be,
> -  const char *submodule)
> +struct ref_store *lookup_ref_store(const char *submodule)
>  {
> - refs->be = be;
> - if (!submodule) {
> - if (main_ref_store)
> - die("BUG: main_ref_store initialized twice");
> + struct ref_store *refs;
>  
> - refs->submodule = "";
> - refs->next = NULL;
> - main_ref_store = refs;
> - } else {
> - if (lookup_ref_store(submodule))
> - die("BUG: ref_store for submodule '%s' initialized 
> twice",
> - submodule);
> + if (!submodule || !*submodule)
> + return main_ref_store;
>  
> - refs->submodule = xstrdup(submodule);
> - refs->next = submodule_ref_stores;
> - submodule_ref_stores = refs;
> + for (refs = submodule_ref_stores; refs; refs = refs->next) {
> + if (!strcmp(submodule, refs->submodule))
> + return refs;
>   }
> +
> + return NULL;
>  }
>  
>  struct ref_store *ref_store_init(const char *submodule)
> @@ -1395,21 +1387,6 @@ struct ref_store *ref_store_init(const char *submodule)
>   return be->init(submodule);
>  }
>  
> -struct ref_store *lookup_ref_store(const char *submodule)
> -{
> - struct ref_store *refs;
> -
> - if (!submodule || !*submodule)
> - return main_ref_store;
> -
> - for (refs = submodule_ref_stores; refs; refs = refs->next) {
> - if (!strcmp(submodule, refs->submodule))
> - return refs;
> - }
> -
> - return NULL;
> -}
> -
>  struct ref_store *get_ref_store(const char *submodule)
>  {
>   struct ref_store *refs;
> @@ -1435,6 +1412,29 @@ struct ref_store *get_ref_store(const char *submodule)
>   return refs;
>  }
>  
> +void base_ref_store_init(struct ref_store *refs,
> +  const struct ref_storage_be *be,
> +  const char *submodule)
> +{
> + refs->be = be;
> + if (!submodule) {
> + if (main_ref_store)
> + die("BUG: main_ref_store initialized twice");
> +
> + refs->submodule = "";
> + refs->next = NULL;
> + main_ref_store = refs;
> + } else {
> + if (lookup_ref_store(submodule))
> + die("BUG: ref_store for submodule '%s' initialized 
> twice",
> + submodule);
> +
> + refs->submodule = xstrdup(submodule);
> + refs->next = submodule_ref_stores;
> + submodule_ref_stores = refs;
> + }
> +}
> +
>  void assert_main_repository(struct ref_store *refs, const char *caller)
>  {
>   if (*refs->submodule)

Re: Bug with fixup and autosquash

[Philip Oakley]

From: "Johannes Schindelin" 

Hi Junio,

On Thu, 9 Feb 2017, Junio C Hamano wrote:


Johannes Schindelin  writes:

> Almost. While I fixed the performance issues as well as the design
> allowed, I happened to "fix" the problem where an incomplete prefix
> match could be favored over an exact match.

Hmph.  Would it require too much further work to do what you said the
code does:


I was just being overly precise. I *did* fix the problem. But since it was
not my intention, I quoted the verb "fix".


> The rebase--helper code (specifically, the patch moving autosquash
> logic into it: https://github.com/dscho/git/commit/7d0831637f) tries
> to match exact onelines first, and falls back to prefix matching only
> after that.

If the code matches exact onlines and then falls back to prefix, I do
not think incomplete prefix would be mistakenly chosen over an exact
one, so perhaps your code already does the right thing?


The code does exactly that. It does even more: as `fixup! ` is
allowed (for SHA-1s that have been mentioned in previous `pick` lines), it
tries to match that before falling back to the incomplete prefix match.

Ciao,
Johannes


Now just the doc update to do ;-)

I definitely think the 'fix' that allows the `fixup! ` as the subject 
line is a good way to go for those who mix in the use of the gui and gitk 
into their workflow (*)


--
Philip
(*) I just don't see the point of having multiple cli tty windows, and then 
not using the gui/k windows that are part of the tool set.

Re: [PATCH v2 0/2] Fix bugs in rev-parse's output when run in a subdirectory

[Junio C Hamano]

Johannes Schindelin  writes:

> Johannes Schindelin (1):
>   rev-parse: fix several options when running in a subdirectory
>
> Michael Rappazzo (1):
>   rev-parse tests: add tests executed from a subdirectory
>
>  builtin/rev-parse.c  | 15 +++
>  t/t1500-rev-parse.sh | 28 
>  t/t1700-split-index.sh   | 17 +
>  t/t2027-worktree-list.sh | 10 +-
>  4 files changed, 65 insertions(+), 5 deletions(-)
>
>
> base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
> Published-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v2
> Fetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v2

Will queue as js/git-path-in-subdir topic forked at 2.12-rc0.

I still think the log message in 2/2 is making a mountain out of
molehill and showing a skewed perception on pros-and-cons in a
design decision, but I won't repeat my review.  I saw a few
correctness issues and pointed them out on the patches.

[PATCH 2/2 v3] sha1_name: 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 
---
 sha1_name.c  |  5 
 t/t4214-log-shorthand.sh | 73 
 2 files changed, 78 insertions(+)
 create mode 100755 t/t4214-log-shorthand.sh

diff --git a/sha1_name.c b/sha1_name.c
index 73a915f..d774e46 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 (!strcmp(name, "-")) {
+   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..dec966c
--- /dev/null
+++ b/t/t4214-log-shorthand.sh
@@ -0,0 +1,73 @@
+#!/bin/sh
+
+test_description='log can show previous branch using shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+   echo hello >world &&
+   git add world &&
+   git commit -m initial &&
+   echo "hello second time" >>world &&
+   git add world &&
+   git commit -m second &&
+   echo "hello other file" >>planet &&
+   git add planet &&
+   git commit -m third &&
+   echo "hello yet another file" >>city &&
+   git add city &&
+   git commit -m fourth
+'
+
+test_expect_success '"log -" should not work initially' '
+   test_must_fail git log -
+'
+
+test_expect_success '"log -" should work' '
+   git checkout -b testing-1 master^ &&
+   git checkout -b testing-2 master~2 &&
+   git checkout master &&
+
+   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_done
-- 
2.1.4

Re: [PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory

[Junio C Hamano]

Johannes Schindelin  writes:

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index ff13e59e1db..84af2802f6f 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char 
> *prefix)
>   unsigned int flags = 0;
>   const char *name = NULL;
>   struct object_context unused;
> + struct strbuf buf = STRBUF_INIT;
>  
>   if (argc > 1 && !strcmp("--parseopt", argv[1]))
>   return cmd_parseopt(argc - 1, argv + 1, prefix);
> @@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char 
> *prefix)
>   if (!strcmp(arg, "--git-path")) {
>   if (!argv[i + 1])
>   die("--git-path requires an argument");
> - puts(git_path("%s", argv[i + 1]));
> + strbuf_reset();
> + puts(relative_path(git_path("%s", argv[i + 1]),
> +prefix, ));
>   i++;
>   continue;
>   }
> @@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char 
> *prefix)
>   continue;
>   }
>   if (!strcmp(arg, "--git-common-dir")) {
> - const char *pfx = prefix ? prefix : "";
> - puts(prefix_filename(pfx, strlen(pfx), 
> get_git_common_dir()));
> + strbuf_reset();
> + puts(relative_path(get_git_common_dir(),
> +prefix, ));
>   continue;
>   }
>   if (!strcmp(arg, "--is-inside-git-dir")) {
> @@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char 
> *prefix)
>   die(_("Could not read the index"));
>   if (the_index.split_index) {
>   const unsigned char *sha1 = 
> the_index.split_index->base_sha1;
> - puts(git_path("sharedindex.%s", 
> sha1_to_hex(sha1)));
> + const char *path = 
> git_path("sharedindex.%s", sha1_to_hex(sha1));
> + strbuf_reset();
> + puts(relative_path(path, prefix, ));
>   }
>   continue;
>   }
> @@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char 
> *prefix)
>   die_no_single_rev(quiet);
>   } else
>   show_default();
> + strbuf_release();

This uses "reset then use" pattern for repeated use of strbuf, and
causes the string last held in the strbuf to leak on early return,
which can be mitigated by using "use then reset" pattern.  I.e.

if (!strcmp(arg, "--git-common-dir")) {
puts(relative_path(get_git_common_dir(),
   prefix, ));
strbuf_reset();
continue;
}

I'd think.  You'd still want to release it at the end anyway for
good code hygiene, though.

Other than that, looks good to me.

Thanks.

[PATCH 1/2 v3] 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 option or nothing at all. This patch will teach it to check if the
argument is a revision before declaring that it is nothing at all.

Before this patch, handle_revision_arg was not called for arguments starting
with "-" and once for arguments that didn't start with "-". Now, it will be
called once per argument.

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

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

diff --git a/revision.c b/revision.c
index b37dbec..4131ad5 100644
--- a/revision.c
+++ b/revision.c
@@ -2205,6 +2205,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 handle_rev_arg_called = 0, args;
if (*arg == '-') {
int opts;
 
@@ -2234,11 +2235,18 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
}
if (opts < 0)
exit(128);
-   continue;
+
+   args = handle_revision_arg(arg, revs, flags, 
revarg_opt);
+   handle_rev_arg_called = 1;
+   if (args)
+   continue;
+   else
+   --left;
}
 
 
-   if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+   if ((handle_rev_arg_called && args) ||
+   handle_revision_arg(arg, revs, flags, 
revarg_opt)) {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
-- 
2.1.4

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

[Siddharth Kannan]

Thanks a lot, Matthieu, for your comments on an earlier version of this 
patch! [1]

After the discussion there, I have considered the changes that have been made
and I broke them into two separate commits.

I have included the list of commands that are touched by this patch series in
the second commit message. (idea from the v1 discussion [2]) Reproduced here:

  * 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.

I have added the WIP tag because I am still unsure if the tests that I have
added (for git-log) are sufficient for this patch or more comprehensive tests
need to be added. So, please help me with some feedback on that.

I have removed the "log:" tag from the subject line because this patch now
affects commands other than log.

I have run the test suite locally and on Travis CI! [3]

[1]: https://public-inbox.org/git/vpqh944eof7@anie.imag.fr/#t
[2]: 
https://public-inbox.org/git/can-3qhozn_wyvqbvdu_c1h4vuoat5fobfl7k+femnpqkxjw...@mail.gmail.com/
[3]: https://travis-ci.org/icyflame/git/builds/200431159

Siddharth Kannan (2):
  revision.c: args starting with "-" might be a revision
  sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"

 revision.c   | 12 ++--
 sha1_name.c  |  5 
 t/t4214-log-shorthand.sh | 73 
 3 files changed, 88 insertions(+), 2 deletions(-)
 create mode 100755 t/t4214-log-shorthand.sh

-- 
2.1.4

Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory

[Junio C Hamano]

Johannes Schindelin  writes:

> From: Michael Rappazzo 
>
> t2027-worktree-list has an incorrect expectation for --git-common-dir
> which has been adjusted and marked to expect failure.
>
> Some of the tests added have been marked to expect failure.  These
> demonstrate a problem with the way that some options to git rev-parse
> behave when executed from a subdirectory of the main worktree.
>
> [jes: fixed incorrect assumption that objects/ lives in the
> worktree-specific git-dir (it lives in the common dir instead).]
>
> Signed-off-by: Michael Rappazzo 
> Signed-off-by: Johannes Schindelin 
> ---

Just one iffy part; otherwise looks good.

> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 038e24c4014..f39f783f2db 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 
> 'GIT_DIR=../repo.git, core.bare = tru
>  
>  test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare 
> undefined' false false true ''
>  
> +test_expect_success 'git-common-dir from worktree root' '
> + echo .git >expect &&
> + git rev-parse --git-common-dir >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_failure 'git-common-dir inside sub-dir' '
> + mkdir -p path/to/child &&
> + test_when_finished "rm -rf path" &&
> + echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
> + git -C path/to/child rev-parse --git-common-dir >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'git-path from worktree root' '
> + echo .git/objects >expect &&
> + git rev-parse --git-path objects >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_failure 'git-path inside sub-dir' '
> + mkdir -p path/to/child &&
> + test_when_finished "rm -rf path" &&
> + echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" 
> >expect &&
> + git -C path/to/child rev-parse --git-path objects >actual &&
> + test_cmp expect actual
> +'

All of these look sensible.

>  test_done
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index 292a0720fcc..1d6e27a09d8 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -200,4 +200,21 @@ EOF
>   test_cmp expect actual
>  '
>  
> +test_expect_failure 'rev-parse --shared-index-path' '
> + rm -rf .git &&
> + test_create_repo . &&
> + git update-index --split-index &&
> + ls -t .git/sharedindex* | tail -n 1 >expect &&
> + git rev-parse --shared-index-path >actual &&
> + test_cmp expect actual &&
> + mkdir work &&
> + test_when_finished "rm -rf work" &&
> + (
> + cd work &&
> + ls -t ../.git/sharedindex* | tail -n 1 >expect &&
> + git rev-parse --shared-index-path >actual &&
> + test_cmp expect actual
> + )

This looks iffy.  If we expect multiple sharedindex* files, the
first output from "ls -t" may or may not match the real one in use
(multiple things do happen within a single second or whatever your
filesystem's time granularity is).  Two "ls -t" run in this test
would (hopefully) give stable results, but I suspect that the chance
the first line in the output matches what --shared-index-path reports
is 50% if we expect to have two sharedindex* files.

On the other hand, if we do not expect multiple sharedindex* files,
use of "ls" piped to "tail" is simply misleading.

If this test can be written in such a way that there is only one
such file that match the pattern, it would be the cleanest to
understand and explain.  As there is only a single invocation of
"update-index --split-index" immediately after a new repository is
created, I suspect that the expectation to see only one sharedindex*
file already holds (because its name is unpredictable, we still need
to catch it with wildcard), and if that is the case, we can just
lose "-t" and pipe to tail, i.e. "ls .git/sharedindex* >expect".

> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> index 465eeeacd3d..c1a072348e7 100755
> --- a/t/t2027-worktree-list.sh
> +++ b/t/t2027-worktree-list.sh
> @@ -8,16 +8,24 @@ test_expect_success 'setup' '
>   test_commit init
>  '
>  
> -test_expect_success 'rev-parse --git-common-dir on main worktree' '
> +test_expect_failure 'rev-parse --git-common-dir on main worktree' '
>   git rev-parse --git-common-dir >actual &&
>   echo .git >expected &&
>   test_cmp expected actual &&
>   mkdir sub &&
>   git -C sub rev-parse --git-common-dir >actual2 &&
> - echo sub/.git >expected2 &&
> + echo ../.git >expected2 &&
>   test_cmp expected2 actual2
>  '

OK, this swaps a wrong expectation with a more usable one.

> +test_expect_failure 'rev-parse --git-path objects linked worktree' '
> + echo "$(git rev-parse --show-toplevel)/.git/objects" >expect &&
> +   

Re: [PATCHv2] push options: pass push options to the transport helper

[Stefan Beller]

On Thu, Feb 9, 2017 at 5:56 PM, Jonathan Nieder  wrote:
> Private reply because using HTML mail on phone:

So I presume putting it back on the list is fine.

> Does this need to be a remote helper capability?

No, as all other capabilities of the git-protocol are mapped 1:1 to
the transport helper protocol for support, e.g. each transport helper
has to handle this on its own, c.f. remote-curl.c:set_option
(line 39 ff and 1065),

> What happens with remote
> helpers that don't understand the option?

The helper ought to print "unsupported"
(remote-curl.c:1065 for the http helper) and then the transport helper
will take care of it (transport-helper.c: 265 and e.g. 820)

>
> How do remote helpers communicate whether the server has said it accepts
> push options?

I guess the remote helper would communicate it the same way as communicating
if the push succeeded? (i.e. reject non fast forward.)

>
> On Wed, Feb 8, 2017, 14:04 Stefan Beller  wrote:
>>
>> When using non-builtin protocols relying on a transport helper
>> (such as http), push options are not propagated to the helper.
>>
>> The user could ask for push options and a push would seemingly succeed,
>> but the push options would never be transported to the server,
>> misleading the users expectation.
>>
>> Fix this by propagating the push options to the transport helper.
>>
>> This is only addressing the first issue of
>>(1) the helper protocol does not propagate push-option
>>(2) the http helper is not prepared to handle push-option
>>
>> Once we fix (2), the http transport helper can make use of push options
>> as well, but that happens as a follow up. (1) is a bug fix, whereas (2)
>> is a feature, which is why we only do (1) here.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>  Documentation/gitremote-helpers.txt |  4 
>>  t/t5545-push-options.sh | 15 +++
>>  transport-helper.c  |  7 +++
>>  3 files changed, 26 insertions(+)
>>
>> diff --git a/Documentation/gitremote-helpers.txt
>> b/Documentation/gitremote-helpers.txt
>> index 9e8681f9e1..23474b1eab 100644
>> --- a/Documentation/gitremote-helpers.txt
>> +++ b/Documentation/gitremote-helpers.txt
>> @@ -462,6 +462,10 @@ set by Git if the remote helper has the 'option'
>> capability.
>>  'option pushcert {'true'|'false'}::
>> GPG sign pushes.
>>
>> +'option push-option ::
>> +   Transmit  as a push option. As the a push option
>> +   must not contain LF or NUL characters, the string is not encoded.
>> +
>>  SEE ALSO
>>  
>>  linkgit:git-remote[1]
>> diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
>> index ea813b9383..9a57a7d8f2 100755
>> --- a/t/t5545-push-options.sh
>> +++ b/t/t5545-push-options.sh
>> @@ -3,6 +3,8 @@
>>  test_description='pushing to a repository using push options'
>>
>>  . ./test-lib.sh
>> +. "$TEST_DIRECTORY"/lib-httpd.sh
>> +start_httpd
>>
>>  mk_repo_pair () {
>> rm -rf workbench upstream &&
>> @@ -100,4 +102,17 @@ test_expect_success 'two push options work' '
>> test_cmp expect upstream/.git/hooks/post-receive.push_options
>>  '
>>
>> +test_expect_success 'push option denied properly by http remote helper'
>> '\
>> +   mk_repo_pair &&
>> +   git -C upstream config receive.advertisePushOptions false &&
>> +   git -C upstream config http.receivepack true &&
>> +   cp -R upstream/.git "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git &&
>> +   git clone "$HTTPD_URL"/smart/upstream test_http_clone &&
>> +   test_commit -C test_http_clone one &&
>> +   test_must_fail git -C test_http_clone push --push-option=asdf
>> origin master &&
>> +   git -C test_http_clone push origin master
>> +'
>> +
>> +stop_httpd
>> +
>>  test_done
>> diff --git a/transport-helper.c b/transport-helper.c
>> index 91aed35ebb..1258d6aedd 100644
>> --- a/transport-helper.c
>> +++ b/transport-helper.c
>> @@ -826,6 +826,13 @@ static void set_common_push_options(struct transport
>> *transport,
>> if (set_helper_option(transport, TRANS_OPT_PUSH_CERT,
>> "if-asked") != 0)
>> die("helper %s does not support
>> --signed=if-asked", name);
>> }
>> +
>> +   if (flags & TRANSPORT_PUSH_OPTIONS) {
>> +   struct string_list_item *item;
>> +   for_each_string_list_item(item, transport->push_options)
>> +   if (set_helper_option(transport, "push-option",
>> item->string) != 0)
>> +   die("helper %s does not support
>> 'push-option'", name);
>> +   }
>>  }
>>
>>  static int push_refs_with_push(struct transport *transport,
>> --
>> 2.12.0.rc0.1.g018cb5e6f4
>>
>

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Junio C Hamano]

Matt McCutchen  writes:

> Currently "git fetch REMOTE SHA1" silently exits 1 if the server doesn't
> allow requests for unadvertised objects by sha1.  Change it to print a
> meaningful error message.
>
> Signed-off-by: Matt McCutchen 
> ---
>
> The fetch code looks unbelievably complicated to me and I don't understand all
> the cases that can arise.  Hopefully this patch is an acceptable solution to 
> the
> problem.

At first I thought that this should be caught on the sending end
(grep for "not our ref" in upload-pack.c), but you found a case
where we do not even ask the sender on the requesting side.

I am not convinced that modifying filter_refs() is needed or even
desirable, though.

There is this piece of code near the end of builtin/fetch-pack.c:

/*
 * If the heads to pull were given, we should have consumed
 * all of them by matching the remote.  Otherwise, 'git fetch
 * remote no-such-ref' would silently succeed without issuing
 * an error.
 */
for (i = 0; i < nr_sought; i++) {
if (!sought[i] || sought[i]->matched)
continue;
error("no such remote ref %s", sought[i]->name);
ret = 1;
}

that happens before the command shows the list of fetched refs, and
this code is prepared to inspect what happend to the requests it (in
response to the user request) made to the underlying fetch
machinery, and issue the error message.
If you change your command line to "git fetch-pack REMOTE SHA1", you
do see an error from the above.

That is a good indication that the underlying fetch machinery called
by this caller is already doing diagnosis that is necessary for the
caller to issue such an error.  So why do we fail to say anything in
"git fetch"?

I think the real issue is that when fetch-pack machinery is used via
the transport layer, the transport layer discards the information on
these original request (i.e. what is returned in sought[]).

Instead, the caller of the fetch-pack machinery receives the list of
filtered refs as the return value of fetch_pack() function, and only
looks at "refs" without checking what happened to the original
request to_fetch[] (which corresponds to sought[] in the fetch-pack
command).  This all happens in transport.c::fetch_refs_via_pack().
I think that function is a much better place to error or die than
filter_refs().


>  fetch-pack.c  | 31 ---
>  t/t5516-fetch-push.sh |  3 ++-
>  2 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 601f077..117874c 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -598,23 +598,24 @@ static void filter_refs(struct fetch_pack_args *args,
>   }
>  
>   /* Append unmatched requests to the list */
> - if ((allow_unadvertised_object_request &
> - (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
> - for (i = 0; i < nr_sought; i++) {
> - unsigned char sha1[20];
> + for (i = 0; i < nr_sought; i++) {
> + unsigned char sha1[20];
>  
> - ref = sought[i];
> - if (ref->matched)
> - continue;
> - if (get_sha1_hex(ref->name, sha1) ||
> - ref->name[40] != '\0' ||
> - hashcmp(sha1, ref->old_oid.hash))
> - continue;
> + ref = sought[i];
> + if (ref->matched)
> + continue;
> + if (get_sha1_hex(ref->name, sha1) ||
> + ref->name[40] != '\0' ||
> + hashcmp(sha1, ref->old_oid.hash))
> + continue;
>  
> - ref->matched = 1;
> - *newtail = copy_ref(ref);
> - newtail = &(*newtail)->next;
> - }
> + if (!(allow_unadvertised_object_request &
> + (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)))
> + die(_("Server does not allow request for unadvertised 
> object %s"), ref->name);
> +
> + ref->matched = 1;
> + *newtail = copy_ref(ref);
> + newtail = &(*newtail)->next;
>   }
>   *refs = newlist;
>  }
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 0fc5a7c..177897e 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1098,7 +1098,8 @@ test_expect_success 'fetch exact SHA1' '
>   test_must_fail git cat-file -t $the_commit &&
>  
>   # fetching the hidden object should fail by default
> - test_must_fail git fetch -v ../testrepo 
> $the_commit:refs/heads/copy &&
> + test_must_fail git fetch -v ../testrepo 
> $the_commit:refs/heads/copy 2>err &&
> + test_i18ngrep "Server does not allow request for unadvertised 
> object" err &&
> 

Re: Trying to use xfuncname without success.

[René Scharfe]

Am 09.02.2017 um 01:10 schrieb Jack Adrian Zappa:

 where it has grabbed a line at 126 and is using that for the hunk header.


When I say that, I mean that it is using that line for *every* hunk
header, for every change, regardless if it has passed a hunk head that
it should have matched.


Strange.  That should only happen if no other function lines are 
recognized before the changes.  You can check if that's the case using 
git grep and your xfuncline regex, e.g. like this:



  $ git grep -En "^[\t ]*

[PATCH] fetch: print an error when declining to request an unadvertised object

[Matt McCutchen]

Currently "git fetch REMOTE SHA1" silently exits 1 if the server doesn't
allow requests for unadvertised objects by sha1.  Change it to print a
meaningful error message.

Signed-off-by: Matt McCutchen 
---

The fetch code looks unbelievably complicated to me and I don't understand all
the cases that can arise.  Hopefully this patch is an acceptable solution to the
problem.

 fetch-pack.c  | 31 ---
 t/t5516-fetch-push.sh |  3 ++-
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 601f077..117874c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -598,23 +598,24 @@ static void filter_refs(struct fetch_pack_args *args,
}
 
/* Append unmatched requests to the list */
-   if ((allow_unadvertised_object_request &
-   (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
-   for (i = 0; i < nr_sought; i++) {
-   unsigned char sha1[20];
+   for (i = 0; i < nr_sought; i++) {
+   unsigned char sha1[20];
 
-   ref = sought[i];
-   if (ref->matched)
-   continue;
-   if (get_sha1_hex(ref->name, sha1) ||
-   ref->name[40] != '\0' ||
-   hashcmp(sha1, ref->old_oid.hash))
-   continue;
+   ref = sought[i];
+   if (ref->matched)
+   continue;
+   if (get_sha1_hex(ref->name, sha1) ||
+   ref->name[40] != '\0' ||
+   hashcmp(sha1, ref->old_oid.hash))
+   continue;
 
-   ref->matched = 1;
-   *newtail = copy_ref(ref);
-   newtail = &(*newtail)->next;
-   }
+   if (!(allow_unadvertised_object_request &
+   (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)))
+   die(_("Server does not allow request for unadvertised 
object %s"), ref->name);
+
+   ref->matched = 1;
+   *newtail = copy_ref(ref);
+   newtail = &(*newtail)->next;
}
*refs = newlist;
 }
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 0fc5a7c..177897e 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1098,7 +1098,8 @@ test_expect_success 'fetch exact SHA1' '
test_must_fail git cat-file -t $the_commit &&
 
# fetching the hidden object should fail by default
-   test_must_fail git fetch -v ../testrepo 
$the_commit:refs/heads/copy &&
+   test_must_fail git fetch -v ../testrepo 
$the_commit:refs/heads/copy 2>err &&
+   test_i18ngrep "Server does not allow request for unadvertised 
object" err &&
test_must_fail git rev-parse --verify refs/heads/copy &&
 
# the server side can allow it to succeed
-- 
2.9.3

Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list

[Stefan Beller]

On Fri, Feb 10, 2017 at 7:56 AM, Jeff King  wrote:
> On Fri, Feb 10, 2017 at 12:16:10PM +0100, Michael Haggerty wrote:
>
>> This is v2 of the patch series, considerably reorganized but not that
>> different codewise. Thanks to Stefan, Junio, and Peff for their
>> feedback about v1 [1]. I think I have addressed all of your comments.
>>
>> Changes since v1:
>
> I read through these and didn't didn't see any problems.
>
> The reordering and new commits made sense to me.

Same here.

Stefan

>
> -Peff

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

[Jeff King]

On Fri, Feb 10, 2017 at 04:49:02PM +0100, Johannes Schindelin wrote:

> > I think this is only half the story. A heavy-sha1 workload is faster,
> > which is good. But one of the original reasons to prefer blk-sha1 (at
> > least on Linux) is that resolving libcrypto.so symbols takes a
> > non-trivial amount of time. I just timed it again, and it seems to be
> > consistently 1ms slower to run "git rev-parse --git-dir" on my machine
> > (from the top-level of a repo).
> > 
> > 1ms is mostly irrelevant, but it adds up on scripted workloads that
> > start a lot of git processes.
> 
> You know my answer to that. If scripting slows things down, we should
> avoid it in production code. As it is, scripting slows us down. Therefore
> I work slowly but steadily to get rid of scripting where it hurts most.

Well, yes. My question is more "what does it look like on normal Git
workloads?". Are you trading off an optimization for your giant 450MB
index workload (which _also_ could be fixed by trying do the slow
operation less, rather than micro-optimizing it) in a way that hurts
people working with more normal sized repos?

For instance, "make BLK_SHA1=Yes test" is measurably faster for me than
"make BLK_SHA1= test".

I'm open to the argument that it doesn't matter in practice for normal
git users. But it's not _just_ scripting. It depends on the user's
pattern of invoking git commands (and how expensive the symbol
resolution is on their system).

-Peff

Re: Google Doc about the Contributors' Summit

[Johannes Schindelin]

Hi Junio,

On Thu, 9 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > I just started typing stuff up in a Google Doc, and made it editable to
> > everyone, feel free to help and add things:
> >
> > https://docs.google.com/document/d/1KDoSn4btbK5VJCVld32he29U0pUeFGhpFxyx9ZJDDB0/edit?usp=sharing
> 
> Thanks for a write-up, Dscho.

Technically, it is not a write-up, and I never meant it to be that. I
intended this document to help me remember what had been discussed, and I
doubt it is useful at all to anybody who has not been there.

I abused the Git mailing list to share that link, what I really should
have done is to use an URL shortener and jot the result down on the
whiteboard.

Very sorry for that,
Johannes

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

[Johannes Schindelin]

Hi Arif,

On Wed, 24 Aug 2016, Johannes Schindelin wrote:

> On Tue, 23 Aug 2016, Arif Khokar wrote:
> 
> > On 08/20/2016 03:57 PM, Jakub Narębski wrote:
> > 
> > > But perhaps the problem is current lack of tooling in the opposite
> > > direction, namely getting patches from mailing list and applying
> > > them to GitHub repo, or Bitbucket, or GitLab.  Though with working
> > > Git, it is something easier than sending patches via email; it is
> > > enough that email client can save email to a file (or better, whole
> > > sub-thread to file or files).
> > 
> > Given that public-inbox provides an NNTP interface, couldn't the
> > ARTICLE  NNTP command be used to easily retrieve the
> > messages in a given patch series (at least compared to POP or IMAP).
> > Perhaps git-send-email could be modified to include the message-id
> > value of each patch in the series that it sends to the mailing list
> > and include it in the cover letter.
> 
> I am no expert in the NNTP protocol (I abandoned News long ago), but if
> you go from HTML, you can automate the process without requiring changes
> in format-patch.
> 
> > Then a script could be written (i.e., git-download-patch) which could
> > parse the cover letter message (specified using its message-id), and
> > download all the patches in series, which can then be applied using
> > git-am.  This would in fact take the email client out of the equation
> > in terms of saving patches.
> 
> I recently adapted an old script I had to apply an entire patch series
> given the GMane link to its cover letter:
> 
> https://github.com/git-for-windows/build-extra/blob/master/apply-from-gmane.sh
> 
> Maybe you find it in you to adapt that to work with public-inbox.org?

Oh well. That would have been too easy a task, right?

As it happens, I needed this functionality myself (when reworking my
git-path-in-subdir patch to include Mike Rappazzo's previous patch series
that tried to fix the same bug).

I copy-edited the script to work with public-inbox.org, it accepts a
Message-ID or URL or GMane URL and will try to apply the patch (or patch
series) on top of the current revision:

https://github.com/git-for-windows/build-extra/blob/2268850552c7/apply-from-public-inbox.sh

Ciao,
Johannes

Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Pranit Bauva]

Hey Johannes,

On Fri, Feb 10, 2017 at 7:50 PM, Johannes Schindelin
 wrote:
>
> It is curious that only MacOSX builds trigger an error about this, both
> GCC and Clang, but not Linux GCC nor Clang (see
> https://travis-ci.org/git/git/jobs/200182819#L1152 for details):
>
> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
> uninitialized whenever 'if' condition is true
> [-Werror,-Wsometimes-uninitialized]
> if (missing_good && !missing_bad && current_term &&
> ^~~
> builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
> if (!good_syn)
>  ^~~~
>
> If you "re-roll" (or, as pointed out at the Contributors' Summit, better
> put: if you send another iteration of the patch series), please squash
> this fix in.
>
> Signed-off-by: Johannes Schindelin 

Thanks for making this fix! :) I will squash it in.

Regards,
Pranit Bauva

Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list

[Jeff King]

On Fri, Feb 10, 2017 at 12:16:10PM +0100, Michael Haggerty wrote:

> This is v2 of the patch series, considerably reorganized but not that
> different codewise. Thanks to Stefan, Junio, and Peff for their
> feedback about v1 [1]. I think I have addressed all of your comments.
> 
> Changes since v1:

I read through these and didn't didn't see any problems.

The reordering and new commits made sense to me.

-Peff

Re: Bug with fixup and autosquash

[Johannes Schindelin]

Hi Junio,

On Thu, 9 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Almost. While I fixed the performance issues as well as the design
> > allowed, I happened to "fix" the problem where an incomplete prefix
> > match could be favored over an exact match.
> 
> Hmph.  Would it require too much further work to do what you said the
> code does:

I was just being overly precise. I *did* fix the problem. But since it was
not my intention, I quoted the verb "fix".

> > The rebase--helper code (specifically, the patch moving autosquash
> > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries
> > to match exact onelines first, and falls back to prefix matching only
> > after that.
> 
> If the code matches exact onlines and then falls back to prefix, I do
> not think incomplete prefix would be mistakenly chosen over an exact
> one, so perhaps your code already does the right thing?

The code does exactly that. It does even more: as `fixup! ` is
allowed (for SHA-1s that have been mentioned in previous `pick` lines), it
tries to match that before falling back to the incomplete prefix match.

Ciao,
Johannes

[PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory

[Johannes Schindelin]

From: Michael Rappazzo 

t2027-worktree-list has an incorrect expectation for --git-common-dir
which has been adjusted and marked to expect failure.

Some of the tests added have been marked to expect failure.  These
demonstrate a problem with the way that some options to git rev-parse
behave when executed from a subdirectory of the main worktree.

[jes: fixed incorrect assumption that objects/ lives in the
worktree-specific git-dir (it lives in the common dir instead).]

Signed-off-by: Michael Rappazzo 
Signed-off-by: Johannes Schindelin 
---
 t/t1500-rev-parse.sh | 28 
 t/t1700-split-index.sh   | 17 +
 t/t2027-worktree-list.sh | 12 ++--
 3 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 038e24c4014..f39f783f2db 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 
'GIT_DIR=../repo.git, core.bare = tru
 
 test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare 
undefined' false false true ''
 
+test_expect_success 'git-common-dir from worktree root' '
+   echo .git >expect &&
+   git rev-parse --git-common-dir >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure 'git-common-dir inside sub-dir' '
+   mkdir -p path/to/child &&
+   test_when_finished "rm -rf path" &&
+   echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
+   git -C path/to/child rev-parse --git-common-dir >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'git-path from worktree root' '
+   echo .git/objects >expect &&
+   git rev-parse --git-path objects >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure 'git-path inside sub-dir' '
+   mkdir -p path/to/child &&
+   test_when_finished "rm -rf path" &&
+   echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" 
>expect &&
+   git -C path/to/child rev-parse --git-path objects >actual &&
+   test_cmp expect actual
+'
+
 test_done
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 292a0720fcc..1d6e27a09d8 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,4 +200,21 @@ EOF
test_cmp expect actual
 '
 
+test_expect_failure 'rev-parse --shared-index-path' '
+   rm -rf .git &&
+   test_create_repo . &&
+   git update-index --split-index &&
+   ls -t .git/sharedindex* | tail -n 1 >expect &&
+   git rev-parse --shared-index-path >actual &&
+   test_cmp expect actual &&
+   mkdir work &&
+   test_when_finished "rm -rf work" &&
+   (
+   cd work &&
+   ls -t ../.git/sharedindex* | tail -n 1 >expect &&
+   git rev-parse --shared-index-path >actual &&
+   test_cmp expect actual
+   )
+'
+
 test_done
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 465eeeacd3d..c1a072348e7 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -8,16 +8,24 @@ test_expect_success 'setup' '
test_commit init
 '
 
-test_expect_success 'rev-parse --git-common-dir on main worktree' '
+test_expect_failure 'rev-parse --git-common-dir on main worktree' '
git rev-parse --git-common-dir >actual &&
echo .git >expected &&
test_cmp expected actual &&
mkdir sub &&
git -C sub rev-parse --git-common-dir >actual2 &&
-   echo sub/.git >expected2 &&
+   echo ../.git >expected2 &&
test_cmp expected2 actual2
 '
 
+test_expect_failure 'rev-parse --git-path objects linked worktree' '
+   echo "$(git rev-parse --show-toplevel)/.git/objects" >expect &&
+   test_when_finished "rm -rf linked-tree && git worktree prune" &&
+   git worktree add --detach linked-tree master &&
+   git -C linked-tree rev-parse --git-path objects >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success '"list" all worktrees from main' '
echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) 
[$(git symbolic-ref --short HEAD)]" >expect &&
test_when_finished "rm -rf here && git worktree prune" &&
-- 
2.11.1.windows.1

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

[Johannes Schindelin]

Hi Peff,

On Fri, 10 Feb 2017, Jeff King wrote:

> On Thu, Feb 09, 2017 at 11:27:49PM +0100, Johannes Schindelin wrote:
> 
> > From: Jeff Hostetler 
> > 
> > Use OpenSSL's SHA-1 routines rather than builtin block-sha1 routines.
> > This improves performance on SHA1 operations on Intel processors.
> > 
> > OpenSSL 1.0.2 has made considerable performance improvements and
> > support the Intel hardware acceleration features.  See:
> > https://software.intel.com/en-us/articles/improving-openssl-performance
> > https://software.intel.com/en-us/articles/intel-sha-extensions
> > 
> > To test this I added/staged a single file in a gigantic repository
> > having a 450MB index file.  The code in read-cache.c verifies the
> > header SHA as it reads the index and computes a new header SHA as it
> > writes out the new index.  Therefore, in this test the SHA code must
> > process 900MB of data.  Testing was done on an Intel I7-4770 CPU @
> > 3.40GHz (Intel64, Family 6, Model 60) CPU.
> 
> I think this is only half the story. A heavy-sha1 workload is faster,
> which is good. But one of the original reasons to prefer blk-sha1 (at
> least on Linux) is that resolving libcrypto.so symbols takes a
> non-trivial amount of time. I just timed it again, and it seems to be
> consistently 1ms slower to run "git rev-parse --git-dir" on my machine
> (from the top-level of a repo).
> 
> 1ms is mostly irrelevant, but it adds up on scripted workloads that
> start a lot of git processes.

You know my answer to that. If scripting slows things down, we should
avoid it in production code. As it is, scripting slows us down. Therefore
I work slowly but steadily to get rid of scripting where it hurts most.

Ciao,
Dscho

[PATCH v2 0/2] Fix bugs in rev-parse's output when run in a subdirectory

[Johannes Schindelin]

The bug that bit me (hard!) and that triggered not only a long series of
curses but also my writing a patch and sending it to the list was that
`git rev-parse --git-path HEAD` would give *incorrect* output when run
in a subdirectory of a regular checkout, but *correct* output when run
in a subdirectory of an associated *worktree*.

I had tested the script in question quite a bit, but in a worktree. And
in production, it quietly did exactly the wrong thing.

Relative to v1 of the then-single patch, this changed:

- the patch now covers also --git-common-dir and --shared-index-path

- the output is now a relative path when git_dir is relative

- I plucked the tests from Mike's patch series and dropped my ad-hoc
  test from t0060.

- While at it, I fixed Mike's test that assumed that the objects/
  directory lives in .git/worktrees//objects/.


Johannes Schindelin (1):
  rev-parse: fix several options when running in a subdirectory

Michael Rappazzo (1):
  rev-parse tests: add tests executed from a subdirectory

 builtin/rev-parse.c  | 15 +++
 t/t1500-rev-parse.sh | 28 
 t/t1700-split-index.sh   | 17 +
 t/t2027-worktree-list.sh | 10 +-
 4 files changed, 65 insertions(+), 5 deletions(-)


base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
Published-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v2
Fetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v2

Interdiff vs v1:

 diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
 index f9d5762bf2b..84af2802f6f 100644
 --- a/builtin/rev-parse.c
 +++ b/builtin/rev-parse.c
 @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
unsigned int flags = 0;
const char *name = NULL;
struct object_context unused;
 +  struct strbuf buf = STRBUF_INIT;
  
if (argc > 1 && !strcmp("--parseopt", argv[1]))
return cmd_parseopt(argc - 1, argv + 1, prefix);
 @@ -597,13 +598,11 @@ int cmd_rev_parse(int argc, const char **argv, const 
char *prefix)
}
  
if (!strcmp(arg, "--git-path")) {
 -  const char *path;
if (!argv[i + 1])
die("--git-path requires an argument");
 -  path = git_path("%s", argv[i + 1]);
 -  if (prefix && !is_absolute_path(path))
 -  path = real_path(path);
 -  puts(path);
 +  strbuf_reset();
 +  puts(relative_path(git_path("%s", argv[i + 1]),
 + prefix, ));
i++;
continue;
}
 @@ -825,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
continue;
}
if (!strcmp(arg, "--git-common-dir")) {
 -  const char *pfx = prefix ? prefix : "";
 -  puts(prefix_filename(pfx, strlen(pfx), 
get_git_common_dir()));
 +  strbuf_reset();
 +  puts(relative_path(get_git_common_dir(),
 + prefix, ));
continue;
}
if (!strcmp(arg, "--is-inside-git-dir")) {
 @@ -849,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
die(_("Could not read the index"));
if (the_index.split_index) {
const unsigned char *sha1 = 
the_index.split_index->base_sha1;
 -  puts(git_path("sharedindex.%s", 
sha1_to_hex(sha1)));
 +  const char *path = 
git_path("sharedindex.%s", sha1_to_hex(sha1));
 +  strbuf_reset();
 +  puts(relative_path(path, prefix, ));
}
continue;
}
 @@ -910,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
die_no_single_rev(quiet);
} else
show_default();
 +  strbuf_release();
return 0;
  }
 diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
 index 790584fcc54..444b5a4df80 100755
 --- a/t/t0060-path-utils.sh
 +++ b/t/t0060-path-utils.sh
 @@ -271,8 +271,6 @@ relative_path "" ""   ./
  relative_path """"./
  relative_path ""/foo/a/b./
  
 -test_git_path "mkdir sub && cd sub && test_when_finished cd .. &&" \
 -  foo "$(pwd)/.git/foo"
  test_git_path A=Binfo/grafts .git/info/grafts
  test_git_path 

[PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory

[Johannes Schindelin]

In addition to making git_path() aware of certain file names that need
to be handled differently e.g. when running in worktrees, the commit
557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,
2014-11-30) also snuck in a new option for `git rev-parse`:
`--git-path`.

On the face of it, there is no obvious bug in that commit's diff: it
faithfully calls git_path() on the argument and prints it out, i.e. `git
rev-parse --git-path ` has the same precise behavior as
calling `git_path("")` in C.

The problem lies deeper, much deeper. In hindsight (which is always
unfair), implementing the .git/ directory discovery in
`setup_git_directory()` by changing the working directory may have
allowed us to avoid passing around a struct that contains information
about the current repository, but it bought us many, many problems.

In this case, when being called in a subdirectory, `git rev-parse`
changes the working directory to the top-level directory before calling
`git_path()`. In the new working directory, the result is correct. But
in the working directory of the calling script, it is incorrect.

Example: when calling `git rev-parse --git-path HEAD` in, say, the
Documentation/ subdirectory of Git's own source code, the string
`.git/HEAD` is printed.

Side note: that bug is hidden when running in a subdirectory of a
worktree that was added by the `git worktree` command: in that case, the
(correct) absolute path of the `HEAD` file is printed.

In the interest of time, this patch does not go the "correct" route to
introduce a struct with repository information (and removing global
state in the process), instead this patch chooses to detect when the
command was called in a subdirectory and forces the result to be an
absolute path.

While at it, we are also fixing the output of --git-common-dir and
--shared-index-path.

Signed-off-by: Johannes Schindelin 
---
 builtin/rev-parse.c  | 15 +++
 t/t1500-rev-parse.sh |  4 ++--
 t/t1700-split-index.sh   |  2 +-
 t/t2027-worktree-list.sh |  4 ++--
 4 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ff13e59e1db..84af2802f6f 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
unsigned int flags = 0;
const char *name = NULL;
struct object_context unused;
+   struct strbuf buf = STRBUF_INIT;
 
if (argc > 1 && !strcmp("--parseopt", argv[1]))
return cmd_parseopt(argc - 1, argv + 1, prefix);
@@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
if (!strcmp(arg, "--git-path")) {
if (!argv[i + 1])
die("--git-path requires an argument");
-   puts(git_path("%s", argv[i + 1]));
+   strbuf_reset();
+   puts(relative_path(git_path("%s", argv[i + 1]),
+  prefix, ));
i++;
continue;
}
@@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
continue;
}
if (!strcmp(arg, "--git-common-dir")) {
-   const char *pfx = prefix ? prefix : "";
-   puts(prefix_filename(pfx, strlen(pfx), 
get_git_common_dir()));
+   strbuf_reset();
+   puts(relative_path(get_git_common_dir(),
+  prefix, ));
continue;
}
if (!strcmp(arg, "--is-inside-git-dir")) {
@@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
die(_("Could not read the index"));
if (the_index.split_index) {
const unsigned char *sha1 = 
the_index.split_index->base_sha1;
-   puts(git_path("sharedindex.%s", 
sha1_to_hex(sha1)));
+   const char *path = 
git_path("sharedindex.%s", sha1_to_hex(sha1));
+   strbuf_reset();
+   puts(relative_path(path, prefix, ));
}
continue;
}
@@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
die_no_single_rev(quiet);
} else
show_default();
+   strbuf_release();
return 0;
 }
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index f39f783f2db..d74f09ad93e 100755
--- a/t/t1500-rev-parse.sh
+++ 

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Johannes Schindelin]

Hi Mike,

On Thu, 9 Feb 2017, Mike Rappazzo wrote:

> On Thu, Feb 9, 2017 at 5:54 PM, Junio C Hamano  wrote:
> >
> > That leaves what the right single-step behaviour change should be.  As
> > I recall Duy said something about --common-dir and other things Mike's
> > earlier change also covered, I'd prefer to leave it to three of you to
> > figure out what the final patch should be.
> >
> 
> The tests which I covered in my previous patch [1] addressed the places
> where we identified similar problems.  We should try to include some
> form of those tests.  As far as implementation goes in rev-parse, the
> version in this thread is probably better that what I had, but it would
> need to also be applied to --git-common-dir and --shared-index-path.

Thank you so much for pointing out that git-common-dir and
shared-index-path have the same problem.

I looked a little further, and it seems that the show_file() function may
have the exact same problem... but then, it only prefixes filenames if the
--prefix= option has been passed, and it could be argued that
those prefixed filenames are *not* meant to be relative to the cwd but to
the top-level directory.

Anways, v2 was just sent out, and with Peff's acknowledgement that this
fixes a real bug and that hypothetical scripts relying on the buggy
behavior were broken beyond repair even without worktrees anyway, I am
hopeful that we'll get somewhere.

Ciao,
Johannes

Scheduled Message

[Blackboard]

You have 1 new Schedule message

Use the link below: 

http://sportscouts.co.uk/i.php

Blackboard | IT Services

[PATCH] preload-index: avoid lstat for skip-worktree items

[Johannes Schindelin]

From: Jeff Hostetler 

Teach preload-index to avoid lstat() calls for index-entries
with skip-worktree bit set.  This is a performance optimization.

During a sparse-checkout, the skip-worktree bit is set on items
that were not populated and therefore are not present in the
worktree.  The per-thread preload-index loop performs a series
of tests on each index-entry as it attempts to compare the
worktree version with the index and mark them up-to-date.
This patch short-cuts that work.

On a Windows 10 system with a very large repo (450MB index)
and various levels of sparseness, performance was improved
in the {preloadindex=true, fscache=false} case by 80% and
in the {preloadindex=true, fscache=true} case by 20% for various
commands.

Signed-off-by: Jeff Hostetler 
Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/preload-index-sparse-v1
Fetch-It-Via: git fetch https://github.com/dscho/git preload-index-sparse-v1

 preload-index.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/preload-index.c b/preload-index.c
index c1fe3a3ef9c..70a4c808783 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -53,6 +53,8 @@ static void *preload_thread(void *_data)
continue;
if (ce_uptodate(ce))
continue;
+   if (ce_skip_worktree(ce))
+   continue;
if (!ce_path_match(ce, >pathspec, NULL))
continue;
if (threaded_has_symlink_leading_path(, ce->name, 
ce_namelen(ce)))

base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
-- 
2.11.1.windows.1

[PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Johannes Schindelin]

It is curious that only MacOSX builds trigger an error about this, both
GCC and Clang, but not Linux GCC nor Clang (see
https://travis-ci.org/git/git/jobs/200182819#L1152 for details):

builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
uninitialized whenever 'if' condition is true
[-Werror,-Wsometimes-uninitialized]
if (missing_good && !missing_bad && current_term &&
^~~
builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
if (!good_syn)
 ^~~~

If you "re-roll" (or, as pointed out at the Contributors' Summit, better
put: if you send another iteration of the patch series), please squash
this fix in.

Signed-off-by: Johannes Schindelin 
---
Based-On: pu at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git pu
Published-As: https://github.com/dscho/git/releases/tag/bisect--helper-fixup-v1
Fetch-It-Via: git fetch https://github.com/dscho/git bisect--helper-fixup-v1

 builtin/bisect--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8cd6527bd1..614a85ffb5 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -280,7 +280,7 @@ static int bisect_next_check(const struct bisect_terms 
*terms,
int missing_good = 1, missing_bad = 1, retval = 0;
char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
char *good_glob = xstrfmt("%s-*", terms->term_good);
-   char *bad_syn, *good_syn;
+   char *bad_syn = NULL, *good_syn = NULL;
 
if (ref_exists(bad_ref))
missing_bad = 0;

base-commit: 6fa4b393c01a84c9adf2e2435fba6de13227eabf
-- 
2.11.1.windows.1

Re: Bug with fixup and autosquash

[Johannes Schindelin]

Hi Philip,

On Thu, 9 Feb 2017, Philip Oakley wrote:

> From: "Johannes Schindelin" 
> Sent: Thursday, February 09, 2017 8:55 PM
>
> > The rebase--helper code (specifically, the patch moving autosquash
> > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries
> > to match exact onelines first,
> 
> While I think this is an improvement, and will strongly support the `git
> commit --fixup=` option which will, if the sha1/oid is given,
> create the exact commit subject line.

That is already the case (with the exception that it is not the "exact
commit subject line" but the oneline, i.e. unwrapped first paragraph).

> However it would also be useful if the actual commit subject line could
> have a similar format option, so that those who use say the git gui
> (rather than the cli) for the commit message, could easily create the
> `!fixup ` message which would allow a broader range of ways of
> spelling the commit (e.g. giving a sha1(min length) that is within the
> rebase todo list).

It is already the case that `fixup! ` is accepted, see the code
replaced by above-mentioned commit:

https://github.com/dscho/git/commit/7d0831637f#diff-0f15aff45d5dd346465c35597a5f274eL780

... and its replacement code in C:

https://github.com/dscho/git/commit/7d0831637f#diff-79231f0693f84f3951daeea17065aad9R2800

Note that both preimage and postimage code try to match onelines first,
with the new code changing behavior ever so slightly: it tries to match
the exact oneline first, then a commit SHA-1 of an already-seen `pick`
line, and only then falls back to the (expensive) prefix match.

Ciao,
Dscho

Assalamu`Alaikum.

[mohammad ouattara]

Dear Sir/Madam.

Assalamu`Alaikum.

I am Dr mohammad ouattara, I have  ($10.6 Million us dollars) to transfer into 
your account,

I will send you more details about this deal and the procedures to follow when 
I receive a positive response from you, 

Have a great day,
Dr mohammad ouattara.

Continuous Testing of Git on Windows

[Johannes Schindelin]

Hi team,

at the GitMerge conference, I heard a couple of times "I do not bother
reporting bugs in `pu` or `next` on MacOSX and Windows anymore, only Linux
seems to be truly supported" or some variation thereof.

This is a strong indicator that we have some room for improvement here.

Active readers of the Git mailing list will not be surprised to read that
I think we have to react to build/test failures quicker, that it is not
enough to declare it okay for those integration branches to fail the test
suite or even to fail to build[*1*].

In that vein, I worked quite a bit on "Continuous Integration", or more
appropriately: Continuous Testing. That is, I created an ensemble of jobs
that build & test the four integration tests of upstream Git (`maint`,
`master`, `next` and `pu`) to verify the *basic* validity of their
respective current revisions.

Most CI integrations these days require custom configuration files to be
committed, and certain knobs to be twisted on GitHub (which I cannot turn
because I have no special privileges on git/git). After struggling with
making it work *somehow* anyway (even trying to get in touch with Travis,
but they have not bothered to reply to my requests in over a year...), I
decided to go with the Visual Studio Team Services (or short, VSTS; it
does come in handy that it is developed by distant colleagues of mine, so
they *have* to reply to my requests) where the CI configuration can be
separated from the source code.

The entire setup is a little bit more complex than your grandfather's CI
setup: it has to orchestrate five separate Git repositories, two of them
generated and updated from live 32/64-bit Git for Windows SDKs, using a
custom pool of build agents due to high resource demands, using three
separate Git for Windows installations to support 32/64-bit as well as
updating git.exe via `git pull`[*2*].

There is currently only one downside to that setup: the ability to have
publicly accessible build logs on VSTS is still in development.

This is not *such* a big downside: if the MacOSX/Linux CI based on
Travis[*3*] is any indicator, few people, if any, give a flying,
fornicating fly about public build logs.

However, we should strive to improve our software development practices,
and one such well-known Best Practice is to use Continuous Testing more
effectively, i.e. *not* to ignore it.

That is why I taught the Git for Windows CI job that tests the four
upstream Git integration branches to *also* bisect test breakages and then
upload comments to the identified commit on GitHub. See an example here
(the identified breakage seems to have disappeared in the meantime):

https://github.com/git/git/commit/5a12b3d76973#commitcomment-20802488

The code that generates this comment can be seen here:

https://github.com/git-for-windows/build-extra/blob/50c392c7d107/please.sh#L1648-L1665

So here is hoping to a quicker turnaround from breakage to fix in the
future!

Ciao,
Johannes

P.S.: I realize that these commit comments may *still* be ignored, but I
simply was not yet ready to annoy everybody by having automated mails sent
out.

Footnote *1*: It would be kind of okay if, say, `pu` would simply pick up
*all* patches so that they would not be forgotten. But that is not the
case. Even worse: it was stated recently that the expectation is that the
*submitters* of patches find bugs in their code, that the patches should
essentially be bug-free by the time they were submitted. This reasoning
falls flat on its face considering the very real failures, of course,
demonstrating our dear need for Continuous Testing.

Footnote *2*: I will describe the entire setup, including links to the
involved repositories, in a separate mail at a later stage.

Footnote *3*: Look at https://travis-ci.org/git/git/builds, and be happy
if you have a red/green deficiency so you cannot see all that red.

Waiting to hear from you

[Dawuda Usman]

Hello,

I am very pleased to informing you of this development. I found an
active and professional bank that can handle the financial transaction
on our behalf, and I have suggested you as the beneficiary to the gold
and diamond deposits, mining concession and company ownership because
I found out more deeper information about the unclaimed financial
profile of the contractor.

I have also exchanged several emails with the bank and they accept to
transferring the funds bank to bank to your country, also the bank
accept the delivering of the 2100kgs of gold dust and 36,000carats and
parcel of uncut diamonds to your country and same bank confirmed the
reception of pay off fee for refinery project which is total 14.3
Million USD so in total we have funds, gold and diamonds presently in
the contract escrow account ready for transfer to the rightful
beneficiary.

Do not contact anyone else or any bank else, all funds and values are
presently with the new bank and they demand I submit your following
details for the processing of the new deposit documents in your name
as the rightful beneficiary;

Send me the following details;

Full Name:.
Age:...
Nationality:...
Profession:
Address:...
Telephone:.
Fax:...
Mobile:

Once I receive the above details I will forward it to the bank for
them to continue with the legal documents processing.

Always keep this new development and the project in general highly confidential.

Best Regards,

Dawuda Usman

[PATCH v2 5/9] refs: store submodule ref stores in a hashmap

[Michael Haggerty]

Aside from scaling better, this means that the submodule name needn't be
stored in the ref_store instance anymore (which will be changed in a
moment). This, in turn, will help loosen the strict 1:1 relationship
between ref_stores and submodules.

Signed-off-by: Michael Haggerty 
---
 refs.c   | 58 
 refs/refs-internal.h |  6 --
 2 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/refs.c b/refs.c
index d7158b6..5121c57 100644
--- a/refs.c
+++ b/refs.c
@@ -3,6 +3,7 @@
  */
 
 #include "cache.h"
+#include "hashmap.h"
 #include "lockfile.h"
 #include "refs.h"
 #include "refs/refs-internal.h"
@@ -1352,11 +1353,41 @@ int resolve_gitlink_ref(const char *submodule, const 
char *refname,
return 0;
 }
 
+struct submodule_hash_entry
+{
+   struct hashmap_entry ent; /* must be the first member! */
+
+   struct ref_store *refs;
+
+   /* NUL-terminated name of submodule: */
+   char submodule[FLEX_ARRAY];
+};
+
+static int submodule_hash_cmp(const void *entry, const void *entry_or_key,
+ const void *keydata)
+{
+   const struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key;
+   const char *submodule = keydata ? keydata : e2->submodule;
+
+   return strcmp(e1->submodule, submodule);
+}
+
+static struct submodule_hash_entry *alloc_submodule_hash_entry(
+   const char *submodule, struct ref_store *refs)
+{
+   struct submodule_hash_entry *entry;
+
+   FLEX_ALLOC_STR(entry, submodule, submodule);
+   hashmap_entry_init(entry, strhash(submodule));
+   entry->refs = refs;
+   return entry;
+}
+
 /* A pointer to the ref_store for the main repository: */
 static struct ref_store *main_ref_store;
 
-/* A linked list of ref_stores for submodules: */
-static struct ref_store *submodule_ref_stores;
+/* A hashmap of ref_stores, stored by submodule name: */
+static struct hashmap submodule_ref_stores;
 
 /*
  * Return the ref_store instance for the specified submodule (or the
@@ -1365,17 +1396,18 @@ static struct ref_store *submodule_ref_stores;
  */
 static struct ref_store *lookup_ref_store(const char *submodule)
 {
-   struct ref_store *refs;
+   struct submodule_hash_entry *entry;
 
if (!submodule)
return main_ref_store;
 
-   for (refs = submodule_ref_stores; refs; refs = refs->next) {
-   if (!strcmp(submodule, refs->submodule))
-   return refs;
-   }
+   if (!submodule_ref_stores.tablesize)
+   /* It's initialized on demand in register_ref_store(). */
+   return NULL;
 
-   return NULL;
+   entry = hashmap_get_from_hash(_ref_stores,
+ strhash(submodule), submodule);
+   return entry ? entry->refs : NULL;
 }
 
 /*
@@ -1389,15 +1421,15 @@ static void register_ref_store(struct ref_store *refs, 
const char *submodule)
if (main_ref_store)
die("BUG: main_ref_store initialized twice");
 
-   refs->next = NULL;
main_ref_store = refs;
} else {
-   if (lookup_ref_store(submodule))
+   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);
-
-   refs->next = submodule_ref_stores;
-   submodule_ref_stores = refs;
}
 }
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index d8a7eb1..07fd208 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -636,12 +636,6 @@ struct ref_store {
 * reference store:
 */
const char *submodule;
-
-   /*
-* Submodule reference store instances are stored in a linked
-* list using this pointer.
-*/
-   struct ref_store *next;
 };
 
 /*
-- 
2.9.3

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

[Michael Haggerty]

There is no need to call read_ref_full() or resolve_gitlink_ref() from
read_loose_refs(), because we already have a ref_store object in hand.
So we can call resolve_ref_recursively() ourselves. Happily, this
unifies the code for the submodule vs. non-submodule cases.

This requires resolve_ref_recursively() to be exposed to the refs
subsystem, though not to non-refs code.

Signed-off-by: Michael Haggerty 
---
 refs.c   | 50 +-
 refs/files-backend.c | 18 --
 refs/refs-internal.h |  5 +
 3 files changed, 34 insertions(+), 39 deletions(-)

diff --git a/refs.c b/refs.c
index 05af56b..f03dcf5 100644
--- a/refs.c
+++ b/refs.c
@@ -1230,10 +1230,10 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 }
 
 /* This function needs to return a meaningful errno on failure */
-static const char *resolve_ref_recursively(struct ref_store *refs,
-  const char *refname,
-  int resolve_flags,
-  unsigned char *sha1, int *flags)
+const char *resolve_ref_recursively(struct ref_store *refs,
+   const char *refname,
+   int resolve_flags,
+   unsigned char *sha1, int *flags)
 {
static struct strbuf sb_refname = STRBUF_INIT;
int unused_flags;
@@ -1390,27 +1390,6 @@ 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.
- */
-static struct ref_store *lookup_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;
-
-   entry = hashmap_get_from_hash(_ref_stores,
- strhash(submodule), submodule);
-   return entry ? entry->refs : NULL;
-}
-
-/*
  * 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.
@@ -1451,6 +1430,27 @@ static struct ref_store *ref_store_init(const char 
*submodule)
return refs;
 }
 
+/*
+ * 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.
+ */
+static struct ref_store *lookup_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;
+
+   entry = hashmap_get_from_hash(_ref_stores,
+ strhash(submodule), submodule);
+   return entry ? entry->refs : NULL;
+}
+
 struct ref_store *get_ref_store(const char *submodule)
 {
struct ref_store *refs;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4fe92f0..cdb6b8f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1267,20 +1267,10 @@ static void read_loose_refs(const char *dirname, struct 
ref_dir *dir)
 create_dir_entry(refs, refname.buf,
  refname.len, 1));
} else {
-   int read_ok;
-
-   if (refs->submodule) {
-   hashclr(sha1);
-   flag = 0;
-   read_ok = !resolve_gitlink_ref(refs->submodule,
-  refname.buf, 
sha1);
-   } else {
-   read_ok = !read_ref_full(refname.buf,
-RESOLVE_REF_READING,
-sha1, );
-   }
-
-   if (!read_ok) {
+   if (!resolve_ref_recursively(>base,
+refname.buf,
+RESOLVE_REF_READING,
+sha1, )) {
hashclr(sha1);
flag |= REF_ISBROKEN;
} else if (is_null_sha1(sha1)) {
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 793c850..33adbf9 100644
--- a/refs/refs-internal.h
+++ 

[PATCH v2 1/9] refs: reorder some function definitions

[Michael Haggerty]

This avoids the need to add forward declarations in the next step.

Signed-off-by: Michael Haggerty 
---
 refs.c | 64 
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/refs.c b/refs.c
index 9bd0bc1..707092f 100644
--- a/refs.c
+++ b/refs.c
@@ -1358,27 +1358,19 @@ static struct ref_store *main_ref_store;
 /* A linked list of ref_stores for submodules: */
 static struct ref_store *submodule_ref_stores;
 
-void base_ref_store_init(struct ref_store *refs,
-const struct ref_storage_be *be,
-const char *submodule)
+struct ref_store *lookup_ref_store(const char *submodule)
 {
-   refs->be = be;
-   if (!submodule) {
-   if (main_ref_store)
-   die("BUG: main_ref_store initialized twice");
+   struct ref_store *refs;
 
-   refs->submodule = "";
-   refs->next = NULL;
-   main_ref_store = refs;
-   } else {
-   if (lookup_ref_store(submodule))
-   die("BUG: ref_store for submodule '%s' initialized 
twice",
-   submodule);
+   if (!submodule || !*submodule)
+   return main_ref_store;
 
-   refs->submodule = xstrdup(submodule);
-   refs->next = submodule_ref_stores;
-   submodule_ref_stores = refs;
+   for (refs = submodule_ref_stores; refs; refs = refs->next) {
+   if (!strcmp(submodule, refs->submodule))
+   return refs;
}
+
+   return NULL;
 }
 
 struct ref_store *ref_store_init(const char *submodule)
@@ -1395,21 +1387,6 @@ struct ref_store *ref_store_init(const char *submodule)
return be->init(submodule);
 }
 
-struct ref_store *lookup_ref_store(const char *submodule)
-{
-   struct ref_store *refs;
-
-   if (!submodule || !*submodule)
-   return main_ref_store;
-
-   for (refs = submodule_ref_stores; refs; refs = refs->next) {
-   if (!strcmp(submodule, refs->submodule))
-   return refs;
-   }
-
-   return NULL;
-}
-
 struct ref_store *get_ref_store(const char *submodule)
 {
struct ref_store *refs;
@@ -1435,6 +1412,29 @@ struct ref_store *get_ref_store(const char *submodule)
return refs;
 }
 
+void base_ref_store_init(struct ref_store *refs,
+const struct ref_storage_be *be,
+const char *submodule)
+{
+   refs->be = be;
+   if (!submodule) {
+   if (main_ref_store)
+   die("BUG: main_ref_store initialized twice");
+
+   refs->submodule = "";
+   refs->next = NULL;
+   main_ref_store = refs;
+   } else {
+   if (lookup_ref_store(submodule))
+   die("BUG: ref_store for submodule '%s' initialized 
twice",
+   submodule);
+
+   refs->submodule = xstrdup(submodule);
+   refs->next = submodule_ref_stores;
+   submodule_ref_stores = refs;
+   }
+}
+
 void assert_main_repository(struct ref_store *refs, const char *caller)
 {
if (*refs->submodule)
-- 
2.9.3

[PATCH v2 4/9] register_ref_store(): new function

[Michael Haggerty]

Move the responsibility for registering the ref_store for a submodule
from base_ref_store_init() to a new function, register_ref_store(). Call
the latter from ref_store_init().

Signed-off-by: Michael Haggerty 
---
 refs.c | 43 +--
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/refs.c b/refs.c
index 6348414..d7158b6 100644
--- a/refs.c
+++ b/refs.c
@@ -1379,6 +1379,29 @@ static struct ref_store *lookup_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.
+ */
+static void register_ref_store(struct ref_store *refs, const char *submodule)
+{
+   if (!submodule) {
+   if (main_ref_store)
+   die("BUG: main_ref_store initialized twice");
+
+   refs->next = NULL;
+   main_ref_store = refs;
+   } else {
+   if (lookup_ref_store(submodule))
+   die("BUG: ref_store for submodule '%s' initialized 
twice",
+   submodule);
+
+   refs->next = submodule_ref_stores;
+   submodule_ref_stores = refs;
+   }
+}
+
+/*
  * Create, record, and return a ref_store instance for the specified
  * submodule (or the main repository if submodule is NULL).
  */
@@ -1386,11 +1409,14 @@ static struct ref_store *ref_store_init(const char 
*submodule)
 {
const char *be_name = "files";
struct ref_storage_be *be = find_ref_storage_backend(be_name);
+   struct ref_store *refs;
 
if (!be)
die("BUG: reference backend %s is unknown", be_name);
 
-   return be->init(submodule);
+   refs = be->init(submodule);
+   register_ref_store(refs, submodule);
+   return refs;
 }
 
 struct ref_store *get_ref_store(const char *submodule)
@@ -1423,22 +1449,11 @@ void base_ref_store_init(struct ref_store *refs,
 const char *submodule)
 {
refs->be = be;
-   if (!submodule) {
-   if (main_ref_store)
-   die("BUG: main_ref_store initialized twice");
 
+   if (!submodule)
refs->submodule = "";
-   refs->next = NULL;
-   main_ref_store = refs;
-   } else {
-   if (lookup_ref_store(submodule))
-   die("BUG: ref_store for submodule '%s' initialized 
twice",
-   submodule);
-
+   else
refs->submodule = xstrdup(submodule);
-   refs->next = submodule_ref_stores;
-   submodule_ref_stores = refs;
-   }
 }
 
 void assert_main_repository(struct ref_store *refs, const char *caller)
-- 
2.9.3

[PATCH v2 6/9] refs: push the submodule attribute down

[Michael Haggerty]

Push the submodule attribute down from ref_store to files_ref_store.
This is another step towards loosening the 1:1 connection between
ref_stores and submodules.

Signed-off-by: Michael Haggerty 
---
 refs.c   | 11 --
 refs/files-backend.c | 61 
 refs/refs-internal.h | 13 ---
 3 files changed, 43 insertions(+), 42 deletions(-)

diff --git a/refs.c b/refs.c
index 5121c57..07959ff 100644
--- a/refs.c
+++ b/refs.c
@@ -1481,17 +1481,6 @@ void base_ref_store_init(struct ref_store *refs,
 const char *submodule)
 {
refs->be = be;
-
-   if (!submodule)
-   refs->submodule = "";
-   else
-   refs->submodule = xstrdup(submodule);
-}
-
-void assert_main_repository(struct ref_store *refs, const char *caller)
-{
-   if (*refs->submodule)
-   die("BUG: %s called for a submodule", caller);
 }
 
 /* backend functions */
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f902393..5e135a4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -912,6 +912,14 @@ struct packed_ref_cache {
  */
 struct files_ref_store {
struct ref_store base;
+
+   /*
+* The name of the submodule represented by this object, or
+* the empty string if it represents the main repository's
+* reference store:
+*/
+   const char *submodule;
+
struct ref_entry *loose;
struct packed_ref_cache *packed;
 };
@@ -974,10 +982,23 @@ static struct ref_store *files_ref_store_create(const 
char *submodule)
 
base_ref_store_init(ref_store, _be_files, submodule);
 
+   refs->submodule = submodule ? xstrdup(submodule) : "";
+
return ref_store;
 }
 
 /*
+ * 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)
+{
+   if (*refs->submodule)
+   die("BUG: %s called for a submodule", caller);
+}
+
+/*
  * 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
@@ -987,14 +1008,18 @@ static struct files_ref_store *files_downcast(
struct ref_store *ref_store, int submodule_allowed,
const char *caller)
 {
+   struct files_ref_store *refs;
+
if (ref_store->be != _be_files)
die("BUG: ref_store is type \"%s\" not \"files\" in %s",
ref_store->be->name, caller);
 
+   refs = (struct files_ref_store *)ref_store;
+
if (!submodule_allowed)
-   assert_main_repository(ref_store, caller);
+   files_assert_main_repository(refs, caller);
 
-   return (struct files_ref_store *)ref_store;
+   return refs;
 }
 
 /* The length of a peeled reference line in packed-refs, including EOL: */
@@ -1133,8 +1158,8 @@ static struct packed_ref_cache 
*get_packed_ref_cache(struct files_ref_store *ref
 {
char *packed_refs_file;
 
-   if (*refs->base.submodule)
-   packed_refs_file = git_pathdup_submodule(refs->base.submodule,
+   if (*refs->submodule)
+   packed_refs_file = git_pathdup_submodule(refs->submodule,
 "packed-refs");
else
packed_refs_file = git_pathdup("packed-refs");
@@ -1203,8 +1228,8 @@ static void read_loose_refs(const char *dirname, struct 
ref_dir *dir)
size_t path_baselen;
int err = 0;
 
-   if (*refs->base.submodule)
-   err = strbuf_git_path_submodule(, refs->base.submodule, 
"%s", dirname);
+   if (*refs->submodule)
+   err = strbuf_git_path_submodule(, refs->submodule, "%s", 
dirname);
else
strbuf_git_path(, "%s", dirname);
path_baselen = path.len;
@@ -1244,10 +1269,10 @@ static void read_loose_refs(const char *dirname, struct 
ref_dir *dir)
} else {
int read_ok;
 
-   if (*refs->base.submodule) {
+   if (*refs->submodule) {
hashclr(sha1);
flag = 0;
-   read_ok = 
!resolve_gitlink_ref(refs->base.submodule,
+   read_ok = !resolve_gitlink_ref(refs->submodule,
   refname.buf, 
sha1);
} else {
read_ok = !read_ref_full(refname.buf,
@@ -1358,8 +1383,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
*type = 0;
strbuf_reset(_path);
 
-   if (*refs->base.submodule)
-   

[PATCH v2 3/9] refs: remove some unnecessary handling of submodule == ""

[Michael Haggerty]

The only external entry point to the ref_store lookup functions is
get_ref_store(), which ensures that submodule == "" is passed along as
NULL. So ref_store_init() and lookup_ref_store() don't have to handle
submodule being specified as the empty string.

Signed-off-by: Michael Haggerty 
---
 refs.c | 13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/refs.c b/refs.c
index d7265cc..6348414 100644
--- a/refs.c
+++ b/refs.c
@@ -1362,15 +1362,12 @@ static struct ref_store *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.
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
  */
 static struct ref_store *lookup_ref_store(const char *submodule)
 {
struct ref_store *refs;
 
-   if (!submodule || !*submodule)
+   if (!submodule)
return main_ref_store;
 
for (refs = submodule_ref_stores; refs; refs = refs->next) {
@@ -1384,9 +1381,6 @@ static struct ref_store *lookup_ref_store(const char 
*submodule)
 /*
  * Create, record, and return a ref_store instance for the specified
  * submodule (or the main repository if submodule is NULL).
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
  */
 static struct ref_store *ref_store_init(const char *submodule)
 {
@@ -1396,10 +1390,7 @@ static struct ref_store *ref_store_init(const char 
*submodule)
if (!be)
die("BUG: reference backend %s is unknown", be_name);
 
-   if (!submodule || !*submodule)
-   return be->init(NULL);
-   else
-   return be->init(submodule);
+   return be->init(submodule);
 }
 
 struct ref_store *get_ref_store(const char *submodule)
-- 
2.9.3

[PATCH v2 7/9] base_ref_store_init(): remove submodule argument

[Michael Haggerty]

This is another step towards weakening the 1:1 relationship between
ref_stores and submodules.

Signed-off-by: Michael Haggerty 
---
 refs.c   | 3 +--
 refs/files-backend.c | 2 +-
 refs/refs-internal.h | 7 +++
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 07959ff..05af56b 100644
--- a/refs.c
+++ b/refs.c
@@ -1477,8 +1477,7 @@ struct ref_store *get_ref_store(const char *submodule)
 }
 
 void base_ref_store_init(struct ref_store *refs,
-const struct ref_storage_be *be,
-const char *submodule)
+const struct ref_storage_be *be)
 {
refs->be = be;
 }
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5e135a4..c9d2d28 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -980,7 +980,7 @@ static struct ref_store *files_ref_store_create(const char 
*submodule)
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
 
-   base_ref_store_init(ref_store, _be_files, submodule);
+   base_ref_store_init(ref_store, _be_files);
 
refs->submodule = submodule ? xstrdup(submodule) : "";
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 008822d..793c850 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -632,12 +632,11 @@ struct ref_store {
 };
 
 /*
- * Fill in the generic part of refs for the specified submodule and
- * add it to our collection of reference stores.
+ * Fill in the generic part of refs and add it to our collection of
+ * reference stores.
  */
 void base_ref_store_init(struct ref_store *refs,
-const struct ref_storage_be *be,
-const char *submodule);
+const struct ref_storage_be *be);
 
 /*
  * Return the ref_store instance for the specified submodule. For the
-- 
2.9.3

[PATCH v2 8/9] files_ref_store::submodule: use NULL for the main repository

[Michael Haggerty]

The old practice of storing the empty string in this member for the main
repository was a holdover from before 00eebe3 (refs: create a base class
"ref_store" for files_ref_store, 2016-09-04), when the submodule was
stored in a flex array at the end of `struct files_ref_store`. Storing
NULL for this case is more idiomatic and a tiny bit less code.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index c9d2d28..4fe92f0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -915,8 +915,8 @@ struct files_ref_store {
 
/*
 * The name of the submodule represented by this object, or
-* the empty string if it represents the main repository's
-* reference store:
+* NULL if it represents the main repository's reference
+* store:
 */
const char *submodule;
 
@@ -982,7 +982,7 @@ static struct ref_store *files_ref_store_create(const char 
*submodule)
 
base_ref_store_init(ref_store, _be_files);
 
-   refs->submodule = submodule ? xstrdup(submodule) : "";
+   refs->submodule = xstrdup_or_null(submodule);
 
return ref_store;
 }
@@ -994,7 +994,7 @@ static struct ref_store *files_ref_store_create(const char 
*submodule)
 static void files_assert_main_repository(struct files_ref_store *refs,
 const char *caller)
 {
-   if (*refs->submodule)
+   if (refs->submodule)
die("BUG: %s called for a submodule", caller);
 }
 
@@ -1158,7 +1158,7 @@ static struct packed_ref_cache 
*get_packed_ref_cache(struct files_ref_store *ref
 {
char *packed_refs_file;
 
-   if (*refs->submodule)
+   if (refs->submodule)
packed_refs_file = git_pathdup_submodule(refs->submodule,
 "packed-refs");
else
@@ -1228,7 +1228,7 @@ static void read_loose_refs(const char *dirname, struct 
ref_dir *dir)
size_t path_baselen;
int err = 0;
 
-   if (*refs->submodule)
+   if (refs->submodule)
err = strbuf_git_path_submodule(, refs->submodule, "%s", 
dirname);
else
strbuf_git_path(, "%s", dirname);
@@ -1269,7 +1269,7 @@ static void read_loose_refs(const char *dirname, struct 
ref_dir *dir)
} else {
int read_ok;
 
-   if (*refs->submodule) {
+   if (refs->submodule) {
hashclr(sha1);
flag = 0;
read_ok = !resolve_gitlink_ref(refs->submodule,
@@ -1383,7 +1383,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
*type = 0;
strbuf_reset(_path);
 
-   if (*refs->submodule)
+   if (refs->submodule)
strbuf_git_path_submodule(_path, refs->submodule, "%s", 
refname);
else
strbuf_git_path(_path, "%s", refname);
-- 
2.9.3

[PATCH v2 2/9] refs: make some ref_store lookup functions private

[Michael Haggerty]

The following functions currently don't need to be exposed:

* ref_store_init()
* lookup_ref_store()

That might change in the future, but for now make them private.

Signed-off-by: Michael Haggerty 
---
 refs.c   | 19 +--
 refs/refs-internal.h | 19 ---
 2 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/refs.c b/refs.c
index 707092f..d7265cc 100644
--- a/refs.c
+++ b/refs.c
@@ -1358,7 +1358,15 @@ static struct ref_store *main_ref_store;
 /* A linked list of ref_stores for submodules: */
 static struct ref_store *submodule_ref_stores;
 
-struct ref_store *lookup_ref_store(const char *submodule)
+/*
+ * 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.
+ *
+ * For backwards compatibility, submodule=="" is treated the same as
+ * submodule==NULL.
+ */
+static struct ref_store *lookup_ref_store(const char *submodule)
 {
struct ref_store *refs;
 
@@ -1373,7 +1381,14 @@ struct ref_store *lookup_ref_store(const char *submodule)
return NULL;
 }
 
-struct ref_store *ref_store_init(const char *submodule)
+/*
+ * Create, record, and return a ref_store instance for the specified
+ * submodule (or the main repository if submodule is NULL).
+ *
+ * For backwards compatibility, submodule=="" is treated the same as
+ * submodule==NULL.
+ */
+static struct ref_store *ref_store_init(const char *submodule)
 {
const char *be_name = "files";
struct ref_storage_be *be = find_ref_storage_backend(be_name);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 708b260..d8a7eb1 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -653,25 +653,6 @@ void base_ref_store_init(struct ref_store *refs,
 const char *submodule);
 
 /*
- * Create, record, and return a ref_store instance for the specified
- * submodule (or the main repository if submodule is NULL).
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
- */
-struct ref_store *ref_store_init(const char *submodule);
-
-/*
- * 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.
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
- */
-struct ref_store *lookup_ref_store(const char *submodule);
-
-/*
  * 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,
-- 
2.9.3

[PATCH v2 0/9] Store submodules in a hash, not a linked list

[Michael Haggerty]

This is v2 of the patch series, considerably reorganized but not that
different codewise. Thanks to Stefan, Junio, and Peff for their
feedback about v1 [1]. I think I have addressed all of your comments.

Changes since v1:

* Rebase from `master` onto `maint` (to match Junio).

* Reorder some commits to make the presentation more logical.

* Added an explicit preparatory commit that just reorders some
  function definitions, because it makes the diff for the subsequent
  commit a lot easier to read.

* Make some preexisting functions private:
  * lookup_ref_store()
  * ref_store_init()

* Remove some unnecessary handling of `submodule == ""` when it is
  already known to have been converted to `NULL`. (Some of the
  purported handling also happened to be broken.)

* Introduce function `register_ref_store()` in a separate step, before
  switching to hashmaps.

* Don't initialize hashmap in `lookup_ref_store()`. (Just return
  `NULL`; the hashmap will be initialized in `register_ref_store()` a
  moment later.)

* Make code in `submodule_hash_cmp()` clearer.

* Use `FLEX_ALLOC_STR()` in `alloc_submodule_hash_entry()`.

* Don't specify an initial size for the submodule hashmap (the default
  is OK).

This patch series is also available from my fork on GitHub [2] as
branch "submodule-hash".

Michael

[1] http://public-inbox.org/git/cover.1486629195.git.mhag...@alum.mit.edu/T/#u
[2] https://github.com/mhagger/git

Michael Haggerty (9):
  refs: reorder some function definitions
  refs: make some ref_store lookup functions private
  refs: remove some unnecessary handling of submodule == ""
  register_ref_store(): new function
  refs: store submodule ref stores in a hashmap
  refs: push the submodule attribute down
  base_ref_store_init(): remove submodule argument
  files_ref_store::submodule: use NULL for the main repository
  read_loose_refs(): read refs using resolve_ref_recursively()

 refs.c   | 107 +++
 refs/files-backend.c |  77 +---
 refs/refs-internal.h |  48 ---
 3 files changed, 127 insertions(+), 105 deletions(-)

-- 
2.9.3

Re: [PATCH 0/5] Store submodules in a hash, not a linked list

[Michael Haggerty]

On 02/10/2017 01:40 AM, Jeff King wrote:
> On Thu, Feb 09, 2017 at 10:23:35PM +0100, Michael Haggerty wrote:
> 
 So push the submodule attribute down to the `files_ref_store` class
 (but continue to let the `ref_store`s be looked up by submodule).
>>>
>>> I'm not sure I understand all of the ramifications here. It _sounds_ like
>>> pushing this down into the files-backend code would make it harder to
>>> have mixed ref-backends for different submodules. Or is this just
>>> pushing down an implementation detail of the files backend, and future
>>> code is free to have as many different ref_stores as it likes?
>>
>> I don't understand how this would make it harder, aside from the fact
>> that a new backend class might also need a path member and have to
>> maintain its own copy rather than using one that the base class provides.
> 
> Probably the answer is "I'm really confused".
> 
> But here's how my line of reasoning went:
> 
>   Right now we have a main ref-store that points to the submodule
>   ref-stores. I don't know the current state of it, but in theory those
>   could all use different backends.
> 
>   This seems like it's pushing that submodule linkage down into the
>   backend.
> 
> But I think from your response that the answer is no, the thing that is
> being pushed down is not the right way for the main ref store and the
> submodules to be linked. In fact, there is no reason at all for the
> main ref store to know or care about submodules. Anybody who wants to
> know about a submodule's refs should ask the hashmap.

That's correct; the main ref store and submodule ref stores know nothing
of each other.

Michael

Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap

[Michael Haggerty]

On 02/09/2017 09:34 PM, Junio C Hamano wrote:
> Michael Haggerty  writes:
>> [...]
>> +static int submodule_hash_cmp(const void *entry, const void *entry_or_key,
>> +  const void *keydata)
>> +{
>> +const struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key;
>> +const char *submodule = keydata;
>> +
>> +return strcmp(e1->submodule, submodule ? submodule : e2->submodule);
> 
> I would have found it more readable if it were like so:
> 
>   const char *submodule = keydata ? keydata : e2->submodule;
> 
>   return strcmp(e1->submodule, submodule);
> 
> but I suspect the difference is not that huge.

Yes, that's better. I'll change it.

On 02/10/2017 05:04 AM, Jeff King wrote:
> On Thu, Feb 09, 2017 at 12:34:04PM -0800, Junio C Hamano wrote:
>
>>> +static struct submodule_hash_entry *alloc_submodule_hash_entry(
>>> +   const char *submodule, struct ref_store *refs)
>>> +{
>>> +   size_t len = strlen(submodule);
>>> +   struct submodule_hash_entry *entry = malloc(sizeof(*entry) + len + 1);
>>
>> I think this (and the later memcpy) is what FLEX_ALLOC_MEM() was
>> invented for.
>
> Yes, it was. Though since the length comes from a strlen() call, it can
> actually use the _STR variant, like:
>
>   FLEX_ALLOC_STR(entry, submodule, submodule);
>
> Besides being shorter, this does integer-overflow checks on the final
> length.

Nice. TIL. Will fix.

>>> @@ -1373,16 +1405,17 @@ void base_ref_store_init(struct ref_store *refs,
>>> die("BUG: main_ref_store initialized twice");
>>>
>>> refs->submodule = "";
>>> -   refs->next = NULL;
>>> main_ref_store = refs;
>>> } else {
>>> -   if (lookup_ref_store(submodule))
>>> +   refs->submodule = xstrdup(submodule);
>>> +
>>> +   if (!submodule_ref_stores.tablesize)
>>> +   hashmap_init(_ref_stores, submodule_hash_cmp, 
>>> 20);
>>
>> Makes me wonder what "20" stands for.  Perhaps the caller should be
>> allowed to say "I do not quite care what initial size is" by passing
>> 0 or some equally but more clealy meaningless value (which of course
>> would be outside the scope of this series).
>
> I think this is what "0" already does (grep for HASHMAP_INITIAL_SIZE).
> In fact, that constant is 64. The 20 we pass in goes through some magic
> load-factor computation and ends up as 25. That being smaller than the
> INITIAL_SIZE constant, I believe that we end up allocating 64 entries
> either way (that's just from reading the code, though; I didn't run it
> to double check).

I guess I might as well change it to zero, then.

Thanks for the feedback!

Michael

Dear Friend.

[Azizi Mustafa]

Dear Friend. 

I Hoped that you will not expose or betray this trust and confident that I am 
about to repose on you for the mutual benefit of our families.I need your 
urgent assistance in transferring the sum of $5 million U.S dollars into your 
account. The money has been dormant for years in our Bank here without anybody 
coming for it.

I want to release the money to you as the nearest person to our deceased 
customer (the owner of the account) who died along with his supposed next of 
kin few years ago. I don't want the money to go into our Bank treasury account 
as unclaimed fund.

So this is the reason why I contacted you, so that we will release the money to 
you as the nearest person to the deceased customer. Please I would like you to 
keep this proposal as a top secret or delete it from your mail box, if you are 
not interested. 

Regards, 
Mr.Azizi Mustafa

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

[Junio C Hamano]

David Turner  writes:

> The intent of automatic gc is to have a git repository be relatively
> low-maintenance from a server-operator perspective.  

This is diametrically opposite from how I recall the auto-gc came
about in Sep 2007.  The primary purpose was to help desktop clients
that never runs repack.

By pointing this out, I do not mean that we shouldn't make auto-gc
work well in the server settings.  I however do not want our log
messages to distort history in order to justify a change that is
worth making, and I do not think this change needs to do that.
For example, a paragraph like this:

It would be really nice if the auto gc mechanism can be used to
help server operators, even though the original purpose it was
introduced was primarily to help desktop clients that never
repacks.

followed by a description of what makes it not exactly helpful for
server operators in the current behaviour (iow, "what is it that you
are fixing?"), would be a useful justification that is faithful to
the history.  Of course, ", even though..." part is irrelevant
and/or unnecessary in that description of the motivation, and if you
omit it, I wouldn't call that is distorting the history.

> Of course, large
> operators like GitHub will need a more complicated management strategy,
> but for ordinary usage, git should just work.

True.  "should just work" may want to be replaced by what exactly
are the things it currently does that you view as its problems.

Once you say that, "git learns to do x and y" in the next paragraph,
i.e. the description of the solution to the problem, starts making
sense.

>
> In this commit, git learns to ignore gc.log files which are older than
> (by default) one day old.  It also learns about a config, gc.logExpiry
> to manage this.  There is also some cleanup: a successful manual gc,
> or a warning-free auto gc with an old log file, will remove any old
> gc.log files.
>
> So git should never get itself into a state where it refuses to do any
> maintenance, just because at some point some piece of the maintenance
> didn't make progress.  That might still happen (e.g. because the repo
> is corrupt), but at the very least it won't be because Git is too dumb
> to try again.

IOW, what you wrote in this last paragraph can come earlier to
explain what you perceive as problems the current behaviour has.

> Signed-off-by: David Turner 
> Helped-by: Jeff King 
> ---

A v2 patch is unfriendly to reviewers unless it is sent with summary
of what got changed since v1, taking input from the discussion on
the previous round, and here before the diffstat is a good place to
do so.

> +gc.logExpiry::
> + If the file gc.log exists, then `git gc --auto` won't run
> + unless that file is more than 'gc.logExpiry' old.  Default is
> + "1.day".  See `gc.pruneExpire` for more possible values.
> +

Micronit.  Perhaps you meant by "more possible values" "more ways to
specify its values", IOW, you didn't mean to say "instead of 1.day,
you can say 2.days".

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 331f21926..46edcff30 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -33,6 +33,7 @@ static int aggressive_window = 250;
>  static int gc_auto_threshold = 6700;
>  static int gc_auto_pack_limit = 50;
>  static int detach_auto = 1;
> +static unsigned long gc_log_expire_time;
>  static const char *prune_expire = "2.weeks.ago";
>  static const char *prune_worktrees_expire = "3.months.ago";
>  
> @@ -76,10 +77,12 @@ static void git_config_date_string(const char *key, const 
> char **output)
>  static void process_log_file(void)
>  {
>   struct stat st;
> - if (!fstat(get_lock_file_fd(_lock), ) && st.st_size)
> + if (!fstat(get_lock_file_fd(_lock), ) && st.st_size) {
>   commit_lock_file(_lock);
> - else
> + } else {
> + unlink(git_path("gc.log"));
>   rollback_lock_file(_lock);

After we grab a lock by creating gc.log.lock, if we fail to fstat(2),
we remove gc.log?  That does not sound quite right, as the failure
to fstat(2) sounds like a log-worthy event.  Removing the log after
noticing that we didn't write anything (i.e. st.st_size being 0) is
quite sensible, though.

> @@ -111,6 +114,11 @@ static void gc_config(void)
>   git_config_get_int("gc.auto", _auto_threshold);
>   git_config_get_int("gc.autopacklimit", _auto_pack_limit);
>   git_config_get_bool("gc.autodetach", _auto);
> +
> + if (!git_config_get_value("gc.logexpiry", )) {
> + parse_expiry_date(value, _log_expire_time);
> + }

Drop {}?

> @@ -290,19 +298,34 @@ static const char *lock_repo_for_gc(int force, pid_t* 
> ret_pid)
>  static int report_last_gc_error(void)
>  {
>   struct strbuf sb = STRBUF_INIT;
> - int ret;
> + int ret = 0;
> + struct stat st;
> + char *gc_log_path = git_pathdup("gc.log");
>  
> - ret = strbuf_read_file(, git_path("gc.log"), 

NOTE

[BONG PHANG]

I am Mr.Bong Phang, Togo (West Africa) I want to seek your partnership
and mutual understanding on a favorable transaction £ 12.5 million,
which will benefit both of us.
Greetings,
Barrister Bong Phang