Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep

[Junio C Hamano]

Ramsay Jones  writes:

> I must admit that I didn't think about the effect of the useless
> "| sort" on the exit status!  What I saw was: a process that
> received no input, sorted nothing and produced no output - pretty
> much the definition of useless! ;-)

I am not sure what you mean by "receive no input, sort nothing and
produce no output".

Ahh, OK, this is a funny one.  I think the original meant to do

grep ... | grep -v ... | sort >actual

but it did

grep ... | grep -v ... >actual | sort

instead by mistake.

And we have two possible "fixes" for that mistake.  Either removing
"|sort" (and replace its only effect, which is to hide brittleness
of relying on exit status of the second grep, with something else)
to declare that we do care about the order multiple warning messages
are given by the last test in the script (by the way, the script is
t5536, not t5556; the patch needs to be retitled), or keeping the "|
sort" and move the redirection into ">actual" to the correct place,
which is to follow through the intention of having that "sort" on
the pipeline in the first place.  I somewhat favor the former in
this particular case myself, but the preference is not a very strong
one.

Thanks.

Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook

[Eric Sunshine]

On Tue, Feb 13, 2018 at 2:27 AM, Johannes Sixt  wrote:
> Am 12.02.2018 um 04:15 schrieb Eric Sunshine:
>> +   echo $_z40 $(git rev-parse HEAD) 1 &&
>> +   echo $(pwd)/gumby
>
> $(pwd) is here and in the other tests correct. $PWD would be wrong on
> Windows. Thanks for being considerate.

Thanks for the confirmation. I'm always a bit leery of using "pwd" in
tests due to Windows concerns; doubly so since I don't have a Windows
installation on which to test it.

Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook

[Johannes Sixt]

Am 12.02.2018 um 04:15 schrieb Eric Sunshine:

--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -454,20 +454,29 @@ post_checkout_hook () {
test_when_finished "rm -f .git/hooks/post-checkout" &&
mkdir -p .git/hooks &&
write_script .git/hooks/post-checkout <<-\EOF
-   echo $* >hook.actual
+   {
+   echo $*
+   git rev-parse --show-toplevel
+   } >../hook.actual
EOF
  }
  
  test_expect_success '"add" invokes post-checkout hook (branch)' '

post_checkout_hook &&
-   printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect &&
+   {
+   echo $_z40 $(git rev-parse HEAD) 1 &&
+   echo $(pwd)/gumby


$(pwd) is here and in the other tests correct. $PWD would be wrong on 
Windows. Thanks for being considerate.



+   } >hook.expect &&
git worktree add gumby &&
test_cmp hook.expect hook.actual


-- Hannes

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Hi Jake,

Jacob Keller  writes:

> On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
>  wrote:
>> Hi Sergey,
>>
>> On Mon, 12 Feb 2018, Sergey Organov wrote:
>>> > Have a look at https://github.com/git/git/pull/447, especially the
>>> > latest commit in there which is an early version of the deprecation I
>>> > intend to bring about.
>>>
>>> You shouldn't want a deprecation at all should you have re-used
>>> --preserve-merges in the first place, and I still don't see why you
>>> haven't.
>>
>> Keep repeating it, and it won't become truer.
>>
>> If you break formats, you break scripts. Git has *so* many users, there
>> are very likely some who script *every* part of it.
>>
>> We simply cannot do that.
>>
>> What we can is deprecate designs which we learned on the way were not only
>> incomplete from the get-go, but bad overall and hard (or impossible) to
>> fix. Like --preserve-merges.
>>
>> Or for that matter like the design you proposed, to use --first-parent for
>> --recreate-merges. Or to use --first-parent for some --recreate-merges,
>> surprising users in very bad ways when it is not used (or when it is
>> used). I get the impression that you still think it would be a good idea,
>> even if it should be obvious that it is not.
>
> If we consider the addition of new todo list elements as "user
> breaking", then yes this change would be user-script breaking.

It _is_ user script breaking, provided such script exists. Has anybody
actually seen one? Not that it's wrong to be extra-cautious about it,
just curios. Note that to be actually affected, such a script must
invoke "git rebase -p" _command_ and then tweak its todo output to
produce outcome.

> Since we did not originally spell out that todo-list items are subject
> to enhancement by addition of operations in the future, scripts are
> likely not designed to allow addition of new elements.

Out of curiosity, are you going to spell it now, for the new todo
format?

> Thus, adding recreate-merges, and deprecating preserve-merges, seems
> to me to be the correct action to take here.

Yes, sure, provided there is actual breakage, or at least informed
suspicion there is one.

> One could argue that users should have expected new todo list elements
> to be added in the future and thus design their scripts to cope with
> such a thing. If you can convincingly argue this, then I don't
> necessarily see it as a complete user breaking change to fix
> preserve-merges in order to allow it to handle re-ordering properly..

I'd not argue this way myself. If there are out-of-git-tree non-human
users that accept and tweak todo _generated_ by current "git rebase -p"
_command_, I also vote for a new option.

> I think I lean towards agreeing with Johannes, and that adding
> recreate-merges and removing preserve-merges is the better solution.

On these grounds it is, no objections.

-- Sergey

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Hi Johannes,

Johannes Schindelin  writes:
> Hi Sergey,
>
> On Mon, 12 Feb 2018, Sergey Organov wrote:
>
>> Thanks for explanations, and could you please answer this one:
>> 
>> [...]
>> 
>> >> I also have trouble making sense of "Recreate merge commits instead of
>> >> flattening the history by replaying merges." Is it "> >> commits by replaying merges> instead of " or is it
>> >> rather " instead of > >> replaying merges>?
>
> I thought I had answered that one.

No, not really, but now you did, please see below.

>
> Flattening the history is what happens in regular rebase (i.e. without
> --recreate-merges and without --preserve-merges).
>
> The idea to recreate merges is of course to *not* flatten the history.

Sure. Never supposed it is.

> Maybe there should have been a comma after "history" to clarify what the
> sentence means.

That's the actual answer to my question, but it in turn raises another
one: why did you change wording of --preserve-merges description for
this new option?

> The wording is poor either way, but you are also not a native speaker so
> we have to rely on, say, Eric to help us out here.

Likely, but why didn't you keep original wording from --preserve-merges?
Do you feel it's somehow poor either?

Anyway, please also refer to wording suggestion in the another (lengthy)
answer in this thread.

-- Sergey

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Sergey Organov]

Hi Johannes,

Johannes Schindelin  writes:
> Hi Sergey,
>
> On Mon, 12 Feb 2018, Sergey Organov wrote:
>
>> Johannes Schindelin  writes:
>> >
>> > On Fri, 9 Feb 2018, Sergey Organov wrote:
>> >
>> >> Johannes Schindelin  writes:
>> >> 
>> >> [...]
>> >> 
>> >> > With this patch, the goodness of the Git garden shears comes to `git
>> >> > rebase -i` itself. Passing the `--recreate-merges` option will generate
>> >> > a todo list that can be understood readily, and where it is obvious
>> >> > how to reorder commits. New branches can be introduced by inserting
>> >> > `label` commands and calling `merge -  `. And once this
>> >> > mode has become stable and universally accepted, we can deprecate the
>> >> > design mistake that was `--preserve-merges`.
>> >> 
>> >> This doesn't explain why you introduced this new --recreate-merges. Why
>> >> didn't you rather fix --preserve-merges to generate and use new todo
>> >> list format?
>> >
>> > Because that would of course break existing users of
>> > --preserve-merges.
>> 
>> How exactly?
>
> Power users of interactive rebase use scripting to augment Git's
> functionality. One particularly powerful trick is to override
> GIT_SEQUENCER_EDITOR with an invocation of such a script, to perform
> automated edits. Such a script breaks when we change the format of the
> content to edit. If we change the format of the todo list generated in
> --preserve-merges mode, that is exactly what happens. We break existing
> users.

I didn't say a word against "--preserve-merges mode", whatever it is,
only about re-using "--preserve-merges" command-line option to "git
rebase", the git user interface. I'm sure you see the difference? Unless
there are out-of-git scripts that do use "git rebase --preserve-merges"
and simultaneously do rely on the todo list format this exact command
generates, there should be no breakage of existing users caused by
changing todo list format generated by  "git rebase --preserve-merges".

Old broken "--preserve-merges mode" could be then kept in the
implementation for ages, unused by the new fixed "git rebase
--preserve-merge", for the sake of compatibility.

> BTW it seems that you did not really read my previous reply carefully
> because I referenced such a use case: the Git garden shears.

I thought I did. You confirm below that this script doesn't use "git
rebase --preserve-merges" in the first place, nor will it break if "git
rebase --preserve-merges" starts to generate new todo format, yet you
expected I'd readily see how it's relevant? No, I'm not that clever, nor
am I a mind-reader.

> They do override the sequencer editor, and while they do not exactly
> edit the todo list (they simply through the generated one away), they
> generate a new todo list and would break if that format changes. Of
> course, the shears do not use the --preserve-merges mode,
> but from just reading about the way how the Git garden shears work, it
> is quite obvious how similar users of --preserve-merges are likely to
> exist?

Maybe, I dunno. If even "garden shears" won't break, then what will? Do
you know an example?

Anyway, as it seems it's too late already for such a change, let me stop
this and assume there are indeed such scripts that will break and that
it's indeed a good idea to introduce new option. Case closed. The manual
should still be fixed though, I think.

>> Doesn't "--recreate-merges" produce the same result as
>> "--preserve-merges" if run non-interactively?
>
> The final result of a rebase where you do not edit the todo list? Should
> be identical, indeed.

That's good to hear.

> But that is the most boring, most uninteresting, and least important use
> case.

For you. Do you suddenly stop caring about compatibility?

> So we might just as well forget about it when we focus on keeping
> Git's usage stable.

Why? It's good it behaves the same, so --preserve-merges could indeed be
deprecated, as you apparently intend.

>> > So why not --preserve-merges=v2? Because that would force me to
>> > maintain --preserve-merges forever. And I don't want to.
>> >
>> >> It doesn't seem likely that todo list created by one Git version is
>> >> to be ever used by another, right?
>> >
>> > No. But by scripts based on `git rebase -p`.
>> >
>> >> Is there some hidden reason here? Some tools outside of Git that use
>> >> old todo list format, maybe?
>> >
>> > Exactly.
>> >
>> > I did mention such a tool: the Git garden shears:
>> >
>> >https://github.com/git-for-windows/build-extra/blob/master/shears.sh
>> >
>> > Have a look at it. It will inform the discussion.
>> 
>> I've searched for "-p" in the script, but didn't find positives for
>> either "-p" or "--preserve-merges". How it would break if it doesn't use
>> them? What am I missing?
>
> *This* particular script does not use -p.
>
> But it is not *this* particular script that I do not want to break!

I thought that was an example 

Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook

[Eric Sunshine]

On Mon, Feb 12, 2018 at 11:42 PM, Eric Sunshine  wrote:
> So, either approach works: removing GIT_DIR or using "worktree add"'s
> existing GIT_DIR and GIT_WORK_TREE. I favor the latter since it is
> consistent with how "worktree add" invokes other command already and,
> especially, because it also addresses the issue Junio raised of
> user-defined GIT_DIR/GIT_WORK_TREE potentially polluting the hook's
> environment.

Just to be clear: Regardless of which fix is used, we still want to
chdir() to the new worktree to guarantee that the directory in which
the 'post-checkout' hook is run is predictable.

In the re-roll, I'm going with the latter approach of re-using
builtin/worktree.c's existing GIT_DIR/GIT_WORK_TREE which it already
exports to other commands it invokes.

Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook

[Eric Sunshine]

On Mon, Feb 12, 2018 at 3:01 PM, Lars Schneider
 wrote:
>> On 12 Feb 2018, at 04:15, Eric Sunshine  wrote:
>> Fix this by changing to the new worktree's directory before running
>> the hook, and adjust the tests to verify that the hook is indeed run
>> within the correct directory.
>
> Looks good but I think we are not quite there yet. It does not work
> for bare repos. You can test this if you apply the following patch on
> top of your changes.

Thanks for providing a useful test case.

The problem is that Git itself exports GIT_DIR with value "." (which
makes sense since "git worktree add" is invoked within a bare repo)
and that GIT_DIR leaks into the hook's environment. However, since the
hook is running within the worktree, into which we've chdir()'d, the
relative "." is wrong, and setup.c:is_git_directory() (invoked
indirectly by setup_bare_git_dir()) correctly reports that the
worktree itself is not a valid Git directory. As a result, 'rev-parse'
run by the hook fails with "fatal: Not a git repository: '.'". The fix
is either to remove GIT_DIR from the environment or make it absolute
so the chdir() doesn't invalidate it.

However, it turns out that builtin/worktree.c already _does_ export
GIT_DIR and GIT_WORK_TREE for the commands it invokes ('update-ref',
'symbolic-ref', 'reset --hard') to create the new worktree, which
makes perfect sense since these commands need to know the location of
the new worktree. So, a second approach/fix is also to use these
existing exports when running the hook. The (minor) catch is that they
are relative and break upon chdir() but that is easily fixed by making
them absolute.

So, either approach works: removing GIT_DIR or using "worktree add"'s
existing GIT_DIR and GIT_WORK_TREE. I favor the latter since it is
consistent with how "worktree add" invokes other command already and,
especially, because it also addresses the issue Junio raised of
user-defined GIT_DIR/GIT_WORK_TREE potentially polluting the hook's
environment.

> Please note that also '"add" within worktree invokes post-checkout hook'
> seems to fail with my extended test case.

Also fixed by either approach.

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Jacob Keller]

On Mon, Feb 12, 2018 at 12:39 PM, Johannes Schindelin
 wrote:
> Hi Sergey,
>
> On Mon, 12 Feb 2018, Sergey Organov wrote:
>> > Have a look at https://github.com/git/git/pull/447, especially the
>> > latest commit in there which is an early version of the deprecation I
>> > intend to bring about.
>>
>> You shouldn't want a deprecation at all should you have re-used
>> --preserve-merges in the first place, and I still don't see why you
>> haven't.
>
> Keep repeating it, and it won't become truer.
>
> If you break formats, you break scripts. Git has *so* many users, there
> are very likely some who script *every* part of it.
>
> We simply cannot do that.
>
> What we can is deprecate designs which we learned on the way were not only
> incomplete from the get-go, but bad overall and hard (or impossible) to
> fix. Like --preserve-merges.
>
> Or for that matter like the design you proposed, to use --first-parent for
> --recreate-merges. Or to use --first-parent for some --recreate-merges,
> surprising users in very bad ways when it is not used (or when it is
> used). I get the impression that you still think it would be a good idea,
> even if it should be obvious that it is not.

If we consider the addition of new todo list elements as "user
breaking", then yes this change would be user-script breaking.

Since we did not originally spell out that todo-list items are subject
to enhancement by addition of operations in the future, scripts are
likely not designed to allow addition of new elements.

Thus, adding recreate-merges, and deprecating preserve-merges, seems
to me to be the correct action to take here.

One could argue that users should have expected new todo list elements
to be added in the future and thus design their scripts to cope with
such a thing. If you can convincingly argue this, then I don't
necessarily see it as a complete user breaking change to fix
preserve-merges in order to allow it to handle re-ordering properly..

I think I lean towards agreeing with Johannes, and that adding
recreate-merges and removing preserve-merges is the better solution.

Thanks,
Jake

Re: [PATCH] color.h: document and modernize header

[Eric Sunshine]

On Mon, Feb 12, 2018 at 8:41 PM, Stefan Beller  wrote:
> Add documentation explaining the functions in color.h.
> While at it, migrate the function `color_set` into grep.c,
> where the only callers are.
>
> Signed-off-by: Stefan Beller 
> ---
> diff --git a/color.h b/color.h
> @@ -76,22 +76,46 @@ int git_color_config(const char *var, const char *value, 
> void *cb);
> +/*
> + * Output the formatted string in the specified color (and then reset to 
> normal
> + * color so subsequent output is uncolored). Omits the color encapsulation if
> + * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting
> + * the color.  BUG: The `color_print_strbuf` prints the given pre-formatted
> + * strbuf instead, up to its first NUL character.
> + */

"`color_print_strbuf` prints the given pre-formatted strbuf (BUG: but
only up to the first NUL character)."

Probably not worth a re-roll is Junio can amend it locally.

[PATCH v2] docs/interpret-trailers: fix agreement error

[brian m. carlson]

In the description of git interpret-trailers, we describe "a group…of
lines" that have certain characteristics.  Ensure both options
describing this group use a singular verb for parallelism.

Signed-off-by: brian m. carlson 
---
 Documentation/git-interpret-trailers.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index 9dd19a1dd9..ff446f15f7 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -51,7 +51,7 @@ with only spaces at the end of the commit message part, one 
blank line
 will be added before the new trailer.
 
 Existing trailers are extracted from the input message by looking for
-a group of one or more lines that (i) are all trailers, or (ii) contains at
+a group of one or more lines that (i) is all trailers, or (ii) contains at
 least one Git-generated or user-configured trailer and consists of at
 least 25% trailers.
 The group must be preceded by one or more empty (or whitespace-only) lines.

Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep

[Ramsay Jones]

On 12/02/18 20:18, Junio C Hamano wrote:
> Ramsay Jones  writes:
> 
>> Attempting to grep the output of test_i18ngrep will not work under a
>> poison build, since the output is (almost) guaranteed not to have the
>> string you are looking for. In this case, the output of test_i18ngrep
>> is further filtered by a simple piplined grep to exclude an '... remote
>> end hung up unexpectedly' warning message. Use a regular 'grep -E' to
>> replace the call to test_i18ngrep in the filter pipeline.
>> Also, remove a useless invocation of 'sort' as the final element of the
>> pipeline.
>>
>> Signed-off-by: Ramsay Jones 
>> ---
>>  t/t5536-fetch-conflicts.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh
>> index 2e42cf331..38381df5e 100755
>> --- a/t/t5536-fetch-conflicts.sh
>> +++ b/t/t5536-fetch-conflicts.sh
>> @@ -22,7 +22,7 @@ verify_stderr () {
>>  cat >expected &&
>>  # We're not interested in the error
>>  # "fatal: The remote end hung up unexpectedly":
>> -test_i18ngrep -E '^(fatal|warning):' actual 
>> | sort &&
>> +grep -E '^(fatal|warning):' actual &&
>>  test_i18ncmp expected actual
> 
> OK, but not quite OK.

:-D

> Two grep invocations will not leave anything useful in 'actual'
> under poison build, and is almost guaranteed that 'expected' would
> not match, but that is perfectly OK because the final comparison is
> done.

"...is done with i18ncmp.", indeed.

The contents of 'actual' would look like:

  warning: # GETTEXT POISON #
  warning: # GETTEXT POISON #

or

  fatal: # GETTEXT POISON #

... depending on which test we are looking at.

> However, it is brittle to rely on the latter "grep -v" to exit with
> status 0 for the &&-chain to work.  The original was most likely
> masked by the "| sort" exiting with 0 status all the time ;-)

I must admit that I didn't think about the effect of the useless
"| sort" on the exit status!  What I saw was: a process that
received no input, sorted nothing and produced no output - pretty
much the definition of useless! ;-)

Also, the "| grep -v" part does remove the "... hung up ..."
message (when present in the input), since that message has not
been i18n-ed. I thought this was deliberate - but maybe not.
(also, so long a _some_ output gets passed on by that grep, the
exit status will be 0).

> There needs an explanation why this commit thinks the invocation of
> "sort" useless.

You mean, other than the fact that it is? ;-)

>   I do not particularly think "suppressing a
> not-found non-zero exit from 'grep'" is a useful thing, but are we
> committed to show the two warnings seen in the last test in this
> file to always in the shown order (i.e. the same order as the
> refspecs are given to us)?  If not, then "sort" does serve a
> purpose.  Note that I do not think we want to be able to reorder the
> warning messages in future versions of Git for that last case, so
> making that explicit may be a good justification.

I did not look back at the history of this test, so I can't say
if that was the original _intent_ of the "| sort" part of the
pipeline. However, it is not serving any purpose now.

> The "sort" as the last step in the pipeline makes sure that we
> do not have to guarantee the order in which we give the two
> warning messages from the last test in this script, but
> processing the refspecs in the same order as they are given on
> the command line and warning against them as we discover
> problems is a property we would rather want to keep, so drop it
> to make sure that we would catch regression when we accidentally
> change the order in which these messages are given.
> 
> or something like that, perhaps.

Hmm, so do you want anything other than a commit message update?

ATB,
Ramsay Jones

[PATCH] color.h: document and modernize header

[Stefan Beller]

Add documentation explaining the functions in color.h.
While at it, migrate the function `color_set` into grep.c,
where the only callers are.

Signed-off-by: Stefan Beller 
---

 * fixed commit message
 * followed Jeffs/Erics advice  and rewrote want_color()s doc once again.

 color.c |  7 ---
 color.h | 34 +-
 grep.c  |  5 +
 3 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/color.c b/color.c
index d48dd947c9..f277e72e4c 100644
--- a/color.c
+++ b/color.c
@@ -161,11 +161,6 @@ int color_parse(const char *value, char *dst)
return color_parse_mem(value, strlen(value), dst);
 }
 
-void color_set(char *dst, const char *color_bytes)
-{
-   xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
-}
-
 /*
  * Write the ANSI color codes for "c" to "out"; the string should
  * already have the ANSI escape code in it. "out" should have enough
@@ -399,8 +394,6 @@ static int color_vfprintf(FILE *fp, const char *color, 
const char *fmt,
return r;
 }
 
-
-
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
 {
va_list args;
diff --git a/color.h b/color.h
index fd2b688dfb..763bef857d 100644
--- a/color.h
+++ b/color.h
@@ -76,22 +76,46 @@ int git_color_config(const char *var, const char *value, 
void *cb);
 int git_color_default_config(const char *var, const char *value, void *cb);
 
 /*
- * Set the color buffer (which must be COLOR_MAXLEN bytes)
- * to the raw color bytes; this is useful for initializing
- * default color variables.
+ * Parse a config option, which can be a boolean or one of
+ * "never", "auto", "always". Return a constant of
+ * GIT_COLOR_NEVER for "never" or negative boolean,
+ * GIT_COLOR_ALWAYS for "always" or a positive boolean,
+ * and GIT_COLOR_AUTO for "auto".
  */
-void color_set(char *dst, const char *color_bytes);
-
 int git_config_colorbool(const char *var, const char *value);
+
+/*
+ * Return a boolean whether to use color, where the argument 'var' is
+ * one of GIT_COLOR_UNKNOWN, GIT_COLOR_NEVER, GIT_COLOR_ALWAYS, GIT_COLOR_AUTO.
+ */
 int want_color(int var);
+
+/*
+ * Translate a Git color from 'value' into a string that the terminal can
+ * interpret and store it into 'dst'. The Git color values are of the form
+ * "foreground [background] [attr]" where fore- and background can be a color
+ * name ("red"), a RGB code (#0xFF) or a 256-color-mode from the terminal.
+ */
 int color_parse(const char *value, char *dst);
 int color_parse_mem(const char *value, int len, char *dst);
+
+/*
+ * Output the formatted string in the specified color (and then reset to normal
+ * color so subsequent output is uncolored). Omits the color encapsulation if
+ * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting
+ * the color.  BUG: The `color_print_strbuf` prints the given pre-formatted
+ * strbuf instead, up to its first NUL character.
+ */
 __attribute__((format (printf, 3, 4)))
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
 int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
 void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
 
+/*
+ * Check if the given color is GIT_COLOR_NIL that means "no color selected".
+ * The caller needs to replace the color with the actual desired color.
+ */
 int color_is_nil(const char *color);
 
 #endif /* COLOR_H */
diff --git a/grep.c b/grep.c
index 3d7cd0e96f..834b8eb439 100644
--- a/grep.c
+++ b/grep.c
@@ -18,6 +18,11 @@ static void std_output(struct grep_opt *opt, const void 
*buf, size_t size)
fwrite(buf, size, 1, stdout);
 }
 
+static void color_set(char *dst, const char *color_bytes)
+{
+   xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
+}
+
 /*
  * Initialize the grep_defaults template with hardcoded defaults.
  * We could let the compiler do this, but without C99 initializers
-- 
2.16.1.73.ga2c3e9663f.dirty

Re: [PATCH 00/26] Moving global state into the repository object (part 1)

[Stefan Beller]

On Mon, Feb 12, 2018 at 5:22 PM, Stefan Beller  wrote:

> I developed this series [...] on top of current master.

also available at https://github.com/stefanbeller/git/tree/object-store-part1

[PATCH 04/26] object-store: move packed_git and packed_git_mru to object store

[Stefan Beller]

In a process with multiple repositories open, packfile accessors
should be associated to a single repository and not shared globally.
Move packed_git and packed_git_mru into the_repository and adjust
callers to reflect this.

Patch generated by

 1. Moving the struct packed_git declaration to object-store.h
and packed_git, packed_git_mru globals to struct object_store.

 2. Applying the semantic patch
contrib/coccinelle/refactoring/packed_git.cocci to adjust callers.
This semantic patch is placed in a sub directory of the coccinelle
contrib dir, as this semantic patch is not expected to be of general
usefulness; it is only useful during developing this series and
merging it with other topics in flight. At a later date, just
delete that semantic patch.

 3. Applying line wrapping fixes from "make style" to break the
resulting long lines.

 4. Adding missing #includes of repository.h and object-store.h
where needed.

 5. As the packfiles are now owned by an objectstore/repository, which
is ephemeral unlike globals, we introduce memory leaks. So address
them in raw_object_store_clear().

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 builtin/count-objects.c |  6 ++--
 builtin/fsck.c  |  7 +++--
 builtin/gc.c|  4 ++-
 builtin/index-pack.c|  1 +
 builtin/pack-objects.c  | 21 -
 builtin/pack-redundant.c|  6 ++--
 builtin/receive-pack.c  |  1 +
 cache.h | 28 --
 contrib/coccinelle/refactoring/packed_git.cocci |  7 +
 fast-import.c   |  6 ++--
 http-backend.c  |  6 ++--
 http-push.c |  1 +
 http-walker.c   |  1 +
 http.c  |  1 +
 mru.h   |  1 +
 object-store.h  | 32 +++-
 object.c|  6 
 pack-bitmap.c   |  4 ++-
 pack-check.c|  1 +
 pack-revindex.c |  1 +
 packfile.c  | 39 +
 reachable.c |  1 +
 server-info.c   |  6 ++--
 sha1_name.c |  5 ++--
 24 files changed, 120 insertions(+), 72 deletions(-)
 create mode 100644 contrib/coccinelle/refactoring/packed_git.cocci

diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 33343818c8..9334648dcc 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -7,6 +7,8 @@
 #include "cache.h"
 #include "config.h"
 #include "dir.h"
+#include "repository.h"
+#include "object-store.h"
 #include "builtin.h"
 #include "parse-options.h"
 #include "quote.h"
@@ -120,9 +122,9 @@ int cmd_count_objects(int argc, const char **argv, const 
char *prefix)
struct strbuf loose_buf = STRBUF_INIT;
struct strbuf pack_buf = STRBUF_INIT;
struct strbuf garbage_buf = STRBUF_INIT;
-   if (!packed_git)
+   if (!the_repository->objects.packed_git)
prepare_packed_git();
-   for (p = packed_git; p; p = p->next) {
+   for (p = the_repository->objects.packed_git; p; p = p->next) {
if (!p->pack_local)
continue;
if (open_pack_index(p))
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 1048255da1..d4d249c2ed 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "repository.h"
 #include "config.h"
+#include "object-store.h"
 #include "commit.h"
 #include "tree.h"
 #include "blob.h"
@@ -707,7 +708,8 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
prepare_packed_git();
 
if (show_progress) {
-   for (p = packed_git; p; p = p->next) {
+   for (p = the_repository->objects.packed_git; p;
+p = p->next) {
if (open_pack_index(p))
continue;
total += p->num_objects;
@@ -715,7 +717,8 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
 
progress = start_progress(_("Checking 
objects"), total);
}
-   for (p = packed_git; p; p = p->next) {
+   for (p = 

[PATCH 06/26] pack: move prepare_packed_git_run_once to object store

[Stefan Beller]

From: Jonathan Nieder 

Each repository's object store can be initialized independently, so
they must not share a run_once variable.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 object-store.h | 8 +++-
 packfile.c | 7 +++
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/object-store.h b/object-store.h
index 8778ab9685..9fd109fa9c 100644
--- a/object-store.h
+++ b/object-store.h
@@ -20,8 +20,14 @@ struct raw_object_store {
 
struct alternate_object_database *alt_odb_list;
struct alternate_object_database **alt_odb_tail;
+
+   /*
+* Whether packed_git has already been populated with this repository's
+* packs.
+*/
+   unsigned packed_git_initialized : 1;
 };
-#define RAW_OBJECT_STORE_INIT { NULL, NULL, MRU_INIT, NULL, NULL }
+#define RAW_OBJECT_STORE_INIT { NULL, NULL, MRU_INIT, NULL, NULL, 0 }
 
 void raw_object_store_clear(struct raw_object_store *o);
 
diff --git a/packfile.c b/packfile.c
index 2ce1b2cef7..c107fce171 100644
--- a/packfile.c
+++ b/packfile.c
@@ -873,12 +873,11 @@ static void prepare_packed_git_mru(void)
mru_append(_repository->objects.packed_git_mru, p);
 }
 
-static int prepare_packed_git_run_once = 0;
 void prepare_packed_git(void)
 {
struct alternate_object_database *alt;
 
-   if (prepare_packed_git_run_once)
+   if (the_repository->objects.packed_git_initialized)
return;
prepare_packed_git_one(get_object_directory(), 1);
prepare_alt_odb();
@@ -886,13 +885,13 @@ void prepare_packed_git(void)
prepare_packed_git_one(alt->path, 0);
rearrange_packed_git();
prepare_packed_git_mru();
-   prepare_packed_git_run_once = 1;
+   the_repository->objects.packed_git_initialized = 1;
 }
 
 void reprepare_packed_git(void)
 {
approximate_object_count_valid = 0;
-   prepare_packed_git_run_once = 0;
+   the_repository->objects.packed_git_initialized = 0;
prepare_packed_git();
 }
 
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 22/26] sha1_file: allow stat_sha1_file to handle arbitrary repositories

[Stefan Beller]

From: Jonathan Nieder 

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index fddada5756..21ddbff846 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -874,19 +874,18 @@ int git_open_cloexec(const char *name, int flags)
  * Note that it may point to static storage and is only valid until another
  * call to sha1_file_name(), etc.
  */
-#define stat_sha1_file(r, s, st, p) stat_sha1_file_##r(s, st, p)
-static int stat_sha1_file_the_repository(const unsigned char *sha1,
-struct stat *st, const char **path)
+static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
+ struct stat *st, const char **path)
 {
struct alternate_object_database *alt;
 
-   *path = sha1_file_name(the_repository, sha1);
+   *path = sha1_file_name(r, sha1);
if (!lstat(*path, st))
return 0;
 
-   prepare_alt_odb(the_repository);
+   prepare_alt_odb(r);
errno = ENOENT;
-   for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next) {
+   for (alt = r->objects.alt_odb_list; alt; alt = alt->next) {
*path = alt_sha1_path(alt, sha1);
if (!lstat(*path, st))
return 0;
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 23/26] sha1_file: allow open_sha1_file to handle arbitrary repositories

[Stefan Beller]

From: Jonathan Nieder 

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 21ddbff846..50202f0959 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -898,22 +898,21 @@ static int stat_sha1_file(struct repository *r, const 
unsigned char *sha1,
  * Like stat_sha1_file(), but actually open the object and return the
  * descriptor. See the caveats on the "path" parameter above.
  */
-#define open_sha1_file(r, s, p) open_sha1_file_##r(s, p)
-static int open_sha1_file_the_repository(const unsigned char *sha1,
-const char **path)
+static int open_sha1_file(struct repository *r,
+ const unsigned char *sha1, const char **path)
 {
int fd;
struct alternate_object_database *alt;
int most_interesting_errno;
 
-   *path = sha1_file_name(the_repository, sha1);
+   *path = sha1_file_name(r, sha1);
fd = git_open(*path);
if (fd >= 0)
return fd;
most_interesting_errno = errno;
 
-   prepare_alt_odb(the_repository);
-   for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next) {
+   prepare_alt_odb(r);
+   for (alt = r->objects.alt_odb_list; alt; alt = alt->next) {
*path = alt_sha1_path(alt, sha1);
fd = git_open(*path);
if (fd >= 0)
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 17/26] sha1_file: add repository argument to open_sha1_file

[Stefan Beller]

Add a repository argument to allow the open_sha1_file caller to be
more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 1d81bafe56..e9e01fb471 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -899,7 +899,9 @@ static int stat_sha1_file_the_repository(const unsigned 
char *sha1,
  * Like stat_sha1_file(), but actually open the object and return the
  * descriptor. See the caveats on the "path" parameter above.
  */
-static int open_sha1_file(const unsigned char *sha1, const char **path)
+#define open_sha1_file(r, s, p) open_sha1_file_##r(s, p)
+static int open_sha1_file_the_repository(const unsigned char *sha1,
+const char **path)
 {
int fd;
struct alternate_object_database *alt;
@@ -938,7 +940,7 @@ static void *map_sha1_file_1(const char *path,
if (path)
fd = git_open(path);
else
-   fd = open_sha1_file(sha1, );
+   fd = open_sha1_file(the_repository, sha1, );
map = NULL;
if (fd >= 0) {
struct stat st;
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 16/26] sha1_file: add repository argument to stat_sha1_file

[Stefan Beller]

Add a repository argument to allow the stat_sha1_file caller to be
more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 65fefdf4ac..1d81bafe56 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -874,8 +874,9 @@ int git_open_cloexec(const char *name, int flags)
  * Note that it may point to static storage and is only valid until another
  * call to sha1_file_name(), etc.
  */
-static int stat_sha1_file(const unsigned char *sha1, struct stat *st,
- const char **path)
+#define stat_sha1_file(r, s, st, p) stat_sha1_file_##r(s, st, p)
+static int stat_sha1_file_the_repository(const unsigned char *sha1,
+struct stat *st, const char **path)
 {
struct alternate_object_database *alt;
 
@@ -1173,7 +1174,7 @@ static int sha1_loose_object_info(const unsigned char 
*sha1,
if (!oi->typep && !oi->typename && !oi->sizep && !oi->contentp) {
const char *path;
struct stat st;
-   if (stat_sha1_file(sha1, , ) < 0)
+   if (stat_sha1_file(the_repository, sha1, , ) < 0)
return -1;
if (oi->disk_sizep)
*oi->disk_sizep = st.st_size;
@@ -1372,7 +1373,7 @@ void *read_sha1_file_extended(const unsigned char *sha1,
die("replacement %s not found for %s",
sha1_to_hex(repl), sha1_to_hex(sha1));
 
-   if (!stat_sha1_file(repl, , ))
+   if (!stat_sha1_file(the_repository, repl, , ))
die("loose object %s (stored in %s) is corrupt",
sha1_to_hex(repl), path);
 
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 25/26] sha1_file: allow map_sha1_file to handle arbitrary repositories

[Stefan Beller]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 object-store.h | 3 +--
 sha1_file.c| 5 +++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/object-store.h b/object-store.h
index b4756444bb..f164c4e5c9 100644
--- a/object-store.h
+++ b/object-store.h
@@ -68,8 +68,7 @@ struct packed_git {
  * is overwritten each time the function is called.
  */
 const char *sha1_file_name(struct repository *r, const unsigned char *sha1);
-#define map_sha1_file(r, s, sz) map_sha1_file_##r(s, sz)
-void *map_sha1_file_the_repository(const unsigned char *sha1, unsigned long 
*size);
+extern void *map_sha1_file(struct repository *r, const unsigned char *sha1, 
unsigned long *size);
 
 void prepare_alt_odb(struct repository *r);
 
diff --git a/sha1_file.c b/sha1_file.c
index 86b0ca7089..6237d59a59 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -956,9 +956,10 @@ static void *map_sha1_file_1(struct repository *r, const 
char *path,
return map;
 }
 
-void *map_sha1_file_the_repository(const unsigned char *sha1, unsigned long 
*size)
+void *map_sha1_file(struct repository *r,
+   const unsigned char *sha1, unsigned long *size)
 {
-   return map_sha1_file_1(the_repository, NULL, sha1, size);
+   return map_sha1_file_1(r, NULL, sha1, size);
 }
 
 static int unpack_sha1_short_header(git_zstream *stream,
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 18/26] sha1_file: add repository argument to map_sha1_file_1

[Stefan Beller]

Add a repository argument to allow the map_sha1_file_1 caller to be
more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index e9e01fb471..f706526bc2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -930,9 +930,10 @@ static int open_sha1_file_the_repository(const unsigned 
char *sha1,
  * Map the loose object at "path" if it is not NULL, or the path found by
  * searching for a loose object named "sha1".
  */
-static void *map_sha1_file_1(const char *path,
-const unsigned char *sha1,
-unsigned long *size)
+#define map_sha1_file_1(r, p, s, si) map_sha1_file_1_##r(p, s, si)
+static void *map_sha1_file_1_the_repository(const char *path,
+   const unsigned char *sha1,
+   unsigned long *size)
 {
void *map;
int fd;
@@ -961,7 +962,7 @@ static void *map_sha1_file_1(const char *path,
 
 void *map_sha1_file(const unsigned char *sha1, unsigned long *size)
 {
-   return map_sha1_file_1(NULL, sha1, size);
+   return map_sha1_file_1(the_repository, NULL, sha1, size);
 }
 
 static int unpack_sha1_short_header(git_zstream *stream,
@@ -2173,7 +2174,7 @@ int read_loose_object(const char *path,
 
*contents = NULL;
 
-   map = map_sha1_file_1(path, NULL, );
+   map = map_sha1_file_1(the_repository, path, NULL, );
if (!map) {
error_errno("unable to mmap %s", path);
goto out;
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 12/26] sha1_file: add repository argument to prepare_alt_odb

[Stefan Beller]

See previous patch for explanation.

While at it, move the declaration to object-store.h,
where it should be easier to find.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 builtin/fsck.c |  2 +-
 cache.h|  1 -
 object-store.h |  3 +++
 packfile.c |  2 +-
 sha1_file.c| 13 +++--
 sha1_name.c|  3 ++-
 6 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index d4d249c2ed..00ec8eecf0 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -695,7 +695,7 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
} else {
fsck_object_dir(get_object_directory());
 
-   prepare_alt_odb();
+   prepare_alt_odb(the_repository);
for (alt = the_repository->objects.alt_odb_list;
alt; alt = alt->next)
fsck_object_dir(alt->path);
diff --git a/cache.h b/cache.h
index 459fd01dbb..70518e24ce 100644
--- a/cache.h
+++ b/cache.h
@@ -1592,7 +1592,6 @@ struct alternate_object_database {
 
char path[FLEX_ARRAY];
 };
-extern void prepare_alt_odb(void);
 extern char *compute_alternate_path(const char *path, struct strbuf *err);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 extern int foreach_alt_odb(alt_odb_fn, void*);
diff --git a/object-store.h b/object-store.h
index 86f1ec2cd0..d96a16edd1 100644
--- a/object-store.h
+++ b/object-store.h
@@ -61,4 +61,7 @@ struct packed_git {
char pack_name[FLEX_ARRAY]; /* more */
 };
 
+#define prepare_alt_odb(r) prepare_alt_odb_##r()
+extern void prepare_alt_odb_the_repository(void);
+
 #endif /* OBJECT_STORE_H */
diff --git a/packfile.c b/packfile.c
index a53706cf16..9c49b34d2b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -879,7 +879,7 @@ void prepare_packed_git(void)
if (the_repository->objects.packed_git_initialized)
return;
prepare_packed_git_one(get_object_directory(), 1);
-   prepare_alt_odb();
+   prepare_alt_odb(the_repository);
for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next)
prepare_packed_git_one(alt->path, 0);
rearrange_packed_git();
diff --git a/sha1_file.c b/sha1_file.c
index b090f403d8..d7f271fe6e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -23,6 +23,7 @@
 #include "sha1-lookup.h"
 #include "bulk-checkin.h"
 #include "repository.h"
+#include "object-store.h"
 #include "streaming.h"
 #include "dir.h"
 #include "mru.h"
@@ -585,7 +586,7 @@ void add_to_alternates_memory(const char *reference)
 * Make sure alternates are initialized, or else our entry may be
 * overwritten when they are.
 */
-   prepare_alt_odb();
+   prepare_alt_odb(the_repository);
 
link_alt_odb_entries(the_repository, reference,
 '\n', NULL, 0);
@@ -671,7 +672,7 @@ int foreach_alt_odb(alt_odb_fn fn, void *cb)
struct alternate_object_database *ent;
int r = 0;
 
-   prepare_alt_odb();
+   prepare_alt_odb(the_repository);
for (ent = the_repository->objects.alt_odb_list; ent; ent = ent->next) {
r = fn(ent, cb);
if (r)
@@ -680,7 +681,7 @@ int foreach_alt_odb(alt_odb_fn fn, void *cb)
return r;
 }
 
-void prepare_alt_odb(void)
+void prepare_alt_odb_the_repository(void)
 {
const char *alt;
 
@@ -729,7 +730,7 @@ static int check_and_freshen_local(const unsigned char 
*sha1, int freshen)
 static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen)
 {
struct alternate_object_database *alt;
-   prepare_alt_odb();
+   prepare_alt_odb(the_repository);
for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next) {
const char *path = alt_sha1_path(alt, sha1);
if (check_and_freshen_file(path, freshen))
@@ -884,7 +885,7 @@ static int stat_sha1_file(const unsigned char *sha1, struct 
stat *st,
if (!lstat(*path, st))
return 0;
 
-   prepare_alt_odb();
+   prepare_alt_odb(the_repository);
errno = ENOENT;
for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next) {
*path = alt_sha1_path(alt, sha1);
@@ -911,7 +912,7 @@ static int open_sha1_file(const unsigned char *sha1, const 
char **path)
return fd;
most_interesting_errno = errno;
 
-   prepare_alt_odb();
+   prepare_alt_odb(the_repository);
for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next) {
*path = alt_sha1_path(alt, sha1);
fd = git_open(*path);
diff --git a/sha1_name.c b/sha1_name.c
index 016c883b5c..3e490ee8f6 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -11,6 +11,7 @@
 #include "sha1-array.h"
 #include "packfile.h"
 #include "repository.h"
+#include "object-store.h"
 
 static int get_oid_oneline(const 

[PATCH 13/26] sha1_file: allow link_alt_odb_entries to handle arbitrary repositories

[Stefan Beller]

Actually this also allows read_info_alternates and link_alt_odb_entry to
handle arbitrary repositories, but link_alt_odb_entries is the most
interesting function in this set of functions, hence the commit subject.

These functions span a strongly connected component in the function
graph, i.e. the recursive call chain might look like

  -> link_alt_odb_entries
-> link_alt_odb_entry
  -> read_info_alternates
-> link_alt_odb_entries

That is why we need to convert all these functions at the same time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 cache.h |  4 
 sha1_file.c | 36 
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/cache.h b/cache.h
index 70518e24ce..09d37cb651 100644
--- a/cache.h
+++ b/cache.h
@@ -1590,6 +1590,10 @@ struct alternate_object_database {
char loose_objects_subdir_seen[256];
struct oid_array loose_objects_cache;
 
+   /*
+* Path to the alternative object store. If this is a relative path,
+* it is relative to the current working directory.
+*/
char path[FLEX_ARRAY];
 };
 extern char *compute_alternate_path(const char *path, struct strbuf *err);
diff --git a/sha1_file.c b/sha1_file.c
index d7f271fe6e..d18ce3aeba 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -394,11 +394,10 @@ static int alt_odb_usable(struct raw_object_store *o,
  * SHA1, an extra slash for the first level indirection, and the
  * terminating NUL.
  */
-#define read_info_alternates(r, rb, d) read_info_alternates_##r(rb, d)
-static void read_info_alternates_the_repository(const char *relative_base,
-   int depth);
-#define link_alt_odb_entry(r, e, rb, d, n) link_alt_odb_entry_##r(e, rb, d, n)
-static int link_alt_odb_entry_the_repository(const char *entry,
+static void read_info_alternates(struct repository *r,
+const char *relative_base,
+int depth);
+static int link_alt_odb_entry(struct repository *r, const char *entry,
const char *relative_base, int depth, const char *normalized_objdir)
 {
struct alternate_object_database *ent;
@@ -424,7 +423,7 @@ static int link_alt_odb_entry_the_repository(const char 
*entry,
while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')
strbuf_setlen(, pathbuf.len - 1);
 
-   if (!alt_odb_usable(_repository->objects, , 
normalized_objdir)) {
+   if (!alt_odb_usable(>objects, , normalized_objdir)) {
strbuf_release();
return -1;
}
@@ -432,12 +431,12 @@ static int link_alt_odb_entry_the_repository(const char 
*entry,
ent = alloc_alt_odb(pathbuf.buf);
 
/* add the alternate entry */
-   *the_repository->objects.alt_odb_tail = ent;
-   the_repository->objects.alt_odb_tail = &(ent->next);
+   *r->objects.alt_odb_tail = ent;
+   r->objects.alt_odb_tail = &(ent->next);
ent->next = NULL;
 
/* recursively add alternates */
-   read_info_alternates(the_repository, pathbuf.buf, depth + 1);
+   read_info_alternates(r, pathbuf.buf, depth + 1);
 
strbuf_release();
return 0;
@@ -472,12 +471,8 @@ static const char *parse_alt_odb_entry(const char *string,
return end;
 }
 
-#define link_alt_odb_entries(r, a, s, rb, d) \
-   link_alt_odb_entries_##r(a, s, rb, d)
-static void link_alt_odb_entries_the_repository(const char *alt,
-   int sep,
-   const char *relative_base,
-   int depth)
+static void link_alt_odb_entries(struct repository *r, const char *alt,
+int sep, const char *relative_base, int depth)
 {
struct strbuf objdirbuf = STRBUF_INIT;
struct strbuf entry = STRBUF_INIT;
@@ -491,7 +486,7 @@ static void link_alt_odb_entries_the_repository(const char 
*alt,
return;
}
 
-   strbuf_add_absolute_path(, get_object_directory());
+   strbuf_add_absolute_path(, r->objects.objectdir);
if (strbuf_normalize_path() < 0)
die("unable to normalize object directory: %s",
objdirbuf.buf);
@@ -500,15 +495,16 @@ static void link_alt_odb_entries_the_repository(const 
char *alt,
alt = parse_alt_odb_entry(alt, sep, );
if (!entry.len)
continue;
-   link_alt_odb_entry(the_repository, entry.buf,
+   link_alt_odb_entry(r, entry.buf,
   relative_base, depth, objdirbuf.buf);
}
strbuf_release();
strbuf_release();
 }
 
-static void read_info_alternates_the_repository(const char *relative_base,
-   int depth)
+static void 

[PATCH 24/26] sha1_file: allow map_sha1_file_1 to handle arbitrary repositories

[Stefan Beller]

From: Jonathan Nieder 

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 50202f0959..86b0ca7089 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -928,10 +928,8 @@ static int open_sha1_file(struct repository *r,
  * Map the loose object at "path" if it is not NULL, or the path found by
  * searching for a loose object named "sha1".
  */
-#define map_sha1_file_1(r, p, s, si) map_sha1_file_1_##r(p, s, si)
-static void *map_sha1_file_1_the_repository(const char *path,
-   const unsigned char *sha1,
-   unsigned long *size)
+static void *map_sha1_file_1(struct repository *r, const char *path,
+const unsigned char *sha1, unsigned long *size)
 {
void *map;
int fd;
@@ -939,7 +937,7 @@ static void *map_sha1_file_1_the_repository(const char 
*path,
if (path)
fd = git_open(path);
else
-   fd = open_sha1_file(the_repository, sha1, );
+   fd = open_sha1_file(r, sha1, );
map = NULL;
if (fd >= 0) {
struct stat st;
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 26/26] sha1_file: allow sha1_loose_object_info to handle arbitrary repositories

[Stefan Beller]

From: Jonathan Nieder 

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 6237d59a59..c3f35914ce 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1148,10 +1148,9 @@ int parse_sha1_header(const char *hdr, unsigned long 
*sizep)
return parse_sha1_header_extended(hdr, , 0);
 }
 
-#define sha1_loose_object_info(r, s, o, f) sha1_loose_object_info_##r(s, o, f)
-static int sha1_loose_object_info_the_repository(const unsigned char *sha1,
-struct object_info *oi,
-int flags)
+static int sha1_loose_object_info(struct repository *r,
+ const unsigned char *sha1,
+ struct object_info *oi, int flags)
 {
int status = 0;
unsigned long mapsize;
@@ -1175,14 +1174,14 @@ static int sha1_loose_object_info_the_repository(const 
unsigned char *sha1,
if (!oi->typep && !oi->typename && !oi->sizep && !oi->contentp) {
const char *path;
struct stat st;
-   if (stat_sha1_file(the_repository, sha1, , ) < 0)
+   if (stat_sha1_file(r, sha1, , ) < 0)
return -1;
if (oi->disk_sizep)
*oi->disk_sizep = st.st_size;
return 0;
}
 
-   map = map_sha1_file(the_repository, sha1, );
+   map = map_sha1_file(r, sha1, );
if (!map)
return -1;
 
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 11/26] sha1_file: add repository argument to link_alt_odb_entries

[Stefan Beller]

See previous patch for explanation.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 4fdfdd945a..b090f403d8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -471,8 +471,12 @@ static const char *parse_alt_odb_entry(const char *string,
return end;
 }
 
-static void link_alt_odb_entries(const char *alt, int sep,
-const char *relative_base, int depth)
+#define link_alt_odb_entries(r, a, s, rb, d) \
+   link_alt_odb_entries_##r(a, s, rb, d)
+static void link_alt_odb_entries_the_repository(const char *alt,
+   int sep,
+   const char *relative_base,
+   int depth)
 {
struct strbuf objdirbuf = STRBUF_INIT;
struct strbuf entry = STRBUF_INIT;
@@ -515,7 +519,7 @@ static void read_info_alternates_the_repository(const char 
*relative_base,
return;
}
 
-   link_alt_odb_entries(buf.buf, '\n', relative_base, depth);
+   link_alt_odb_entries(the_repository, buf.buf, '\n', relative_base, 
depth);
strbuf_release();
free(path);
 }
@@ -569,7 +573,8 @@ void add_to_alternates_file(const char *reference)
if (commit_lock_file())
die_errno("unable to move new alternates file into 
place");
if (the_repository->objects.alt_odb_tail)
-   link_alt_odb_entries(reference, '\n', NULL, 0);
+   link_alt_odb_entries(the_repository, reference,
+'\n', NULL, 0);
}
free(alts);
 }
@@ -582,7 +587,8 @@ void add_to_alternates_memory(const char *reference)
 */
prepare_alt_odb();
 
-   link_alt_odb_entries(reference, '\n', NULL, 0);
+   link_alt_odb_entries(the_repository, reference,
+'\n', NULL, 0);
 }
 
 /*
@@ -685,7 +691,8 @@ void prepare_alt_odb(void)
 
the_repository->objects.alt_odb_tail =
_repository->objects.alt_odb_list;
-   link_alt_odb_entries(alt, PATH_SEP, NULL, 0);
+   link_alt_odb_entries(the_repository, alt,
+PATH_SEP, NULL, 0);
 
read_info_alternates(the_repository, get_object_directory(), 0);
 }
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 10/26] sha1_file: add repository argument to read_info_alternates

[Stefan Beller]

See previous patch for explanation.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 7dc5f690e2..4fdfdd945a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -393,7 +393,9 @@ static int alt_odb_usable(struct raw_object_store *o,
  * SHA1, an extra slash for the first level indirection, and the
  * terminating NUL.
  */
-static void read_info_alternates(const char * relative_base, int depth);
+#define read_info_alternates(r, rb, d) read_info_alternates_##r(rb, d)
+static void read_info_alternates_the_repository(const char *relative_base,
+   int depth);
 #define link_alt_odb_entry(r, e, rb, d, n) link_alt_odb_entry_##r(e, rb, d, n)
 static int link_alt_odb_entry_the_repository(const char *entry,
const char *relative_base, int depth, const char *normalized_objdir)
@@ -434,7 +436,7 @@ static int link_alt_odb_entry_the_repository(const char 
*entry,
ent->next = NULL;
 
/* recursively add alternates */
-   read_info_alternates(pathbuf.buf, depth + 1);
+   read_info_alternates(the_repository, pathbuf.buf, depth + 1);
 
strbuf_release();
return 0;
@@ -500,7 +502,8 @@ static void link_alt_odb_entries(const char *alt, int sep,
strbuf_release();
 }
 
-static void read_info_alternates(const char * relative_base, int depth)
+static void read_info_alternates_the_repository(const char *relative_base,
+   int depth)
 {
char *path;
struct strbuf buf = STRBUF_INIT;
@@ -684,7 +687,7 @@ void prepare_alt_odb(void)
_repository->objects.alt_odb_list;
link_alt_odb_entries(alt, PATH_SEP, NULL, 0);
 
-   read_info_alternates(get_object_directory(), 0);
+   read_info_alternates(the_repository, get_object_directory(), 0);
 }
 
 /* Returns 1 if we have successfully freshened the file, 0 otherwise. */
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 19/26] sha1_file: add repository argument to map_sha1_file

[Stefan Beller]

From: Jonathan Nieder 

Add a repository argument to allow map_sha1_file callers to be more
specific about which repository to handle. This is a small mechanical
change; it doesn't change the implementation to handle repositories
other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

While at it, move the declaration to object-store.h, where it should
be easier to find.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 cache.h| 1 -
 object-store.h | 2 ++
 sha1_file.c| 4 ++--
 streaming.c| 5 -
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 4b01c0de10..d0a1236dac 100644
--- a/cache.h
+++ b/cache.h
@@ -1237,7 +1237,6 @@ extern int pretend_sha1_file(void *, unsigned long, enum 
object_type, unsigned c
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);
 extern int git_open_cloexec(const char *name, int flags);
 #define git_open(name) git_open_cloexec(name, O_RDONLY)
-extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
 extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, 
unsigned long mapsize, void *buffer, unsigned long bufsiz);
 extern int parse_sha1_header(const char *hdr, unsigned long *sizep);
 
diff --git a/object-store.h b/object-store.h
index 5422e80c08..0a4561f476 100644
--- a/object-store.h
+++ b/object-store.h
@@ -69,6 +69,8 @@ struct packed_git {
  */
 #define sha1_file_name(r, s) sha1_file_name_##r(s)
 const char *sha1_file_name_the_repository(const unsigned char *sha1);
+#define map_sha1_file(r, s, sz) map_sha1_file_##r(s, sz)
+void *map_sha1_file_the_repository(const unsigned char *sha1, unsigned long 
*size);
 
 void prepare_alt_odb(struct repository *r);
 
diff --git a/sha1_file.c b/sha1_file.c
index f706526bc2..1ce4058644 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -960,7 +960,7 @@ static void *map_sha1_file_1_the_repository(const char 
*path,
return map;
 }
 
-void *map_sha1_file(const unsigned char *sha1, unsigned long *size)
+void *map_sha1_file_the_repository(const unsigned char *sha1, unsigned long 
*size)
 {
return map_sha1_file_1(the_repository, NULL, sha1, size);
 }
@@ -1184,7 +1184,7 @@ static int sha1_loose_object_info(const unsigned char 
*sha1,
return 0;
}
 
-   map = map_sha1_file(sha1, );
+   map = map_sha1_file(the_repository, sha1, );
if (!map)
return -1;
 
diff --git a/streaming.c b/streaming.c
index 5892b50bd8..22d27df55e 100644
--- a/streaming.c
+++ b/streaming.c
@@ -3,6 +3,8 @@
  */
 #include "cache.h"
 #include "streaming.h"
+#include "repository.h"
+#include "object-store.h"
 #include "packfile.h"
 
 enum input_source {
@@ -335,7 +337,8 @@ static struct stream_vtbl loose_vtbl = {
 
 static open_method_decl(loose)
 {
-   st->u.loose.mapped = map_sha1_file(sha1, >u.loose.mapsize);
+   st->u.loose.mapped = map_sha1_file(the_repository,
+  sha1, >u.loose.mapsize);
if (!st->u.loose.mapped)
return -1;
if ((unpack_sha1_header(>z,
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 09/26] sha1_file: add repository argument to link_alt_odb_entry

[Stefan Beller]

Add a repository argument to allow the link_alt_odb_entry caller to be
more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

Since the implementation does not yet work with other repositories,
use a wrapper macro to enforce that the caller passes in
the_repository as the first argument. It would be more appealing to
use BUILD_ASSERT_OR_ZERO to enforce this, but that doesn't work
because it requires a compile-time constant and common compilers like
gcc 4.8.4 do not consider "r == the_repository" a compile-time
constant.

This and the following three patches add repository arguments to
link_alt_odb_entry, read_info_alternates, link_alt_odb_entries
and prepare_alt_odb. Three out of the four functions are found
in a recursive call chain, calling each other, and one of them
accesses the repositories `objectdir` (which was migrated; it
was an obvious choice) and `ignore_env` (which we need to keep in
the repository struct for clarify); hence we will pass through the
repository unlike just the object store object + the ignore_env flag.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index d25b68a8fb..7dc5f690e2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -394,8 +394,9 @@ static int alt_odb_usable(struct raw_object_store *o,
  * terminating NUL.
  */
 static void read_info_alternates(const char * relative_base, int depth);
-static int link_alt_odb_entry(const char *entry, const char *relative_base,
-   int depth, const char *normalized_objdir)
+#define link_alt_odb_entry(r, e, rb, d, n) link_alt_odb_entry_##r(e, rb, d, n)
+static int link_alt_odb_entry_the_repository(const char *entry,
+   const char *relative_base, int depth, const char *normalized_objdir)
 {
struct alternate_object_database *ent;
struct strbuf pathbuf = STRBUF_INIT;
@@ -492,7 +493,8 @@ static void link_alt_odb_entries(const char *alt, int sep,
alt = parse_alt_odb_entry(alt, sep, );
if (!entry.len)
continue;
-   link_alt_odb_entry(entry.buf, relative_base, depth, 
objdirbuf.buf);
+   link_alt_odb_entry(the_repository, entry.buf,
+  relative_base, depth, objdirbuf.buf);
}
strbuf_release();
strbuf_release();
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 15/26] sha1_file: add repository argument to sha1_file_name

[Stefan Beller]

From: Jonathan Nieder 

Add a repository argument to allow sha1_file_name callers to be more
specific about which repository to handle. This is a small mechanical
change; it doesn't change the implementation to handle repositories
other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

While at it, move the declaration to object-store.h, where it should
be easier to find.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 cache.h|  8 
 http-walker.c  |  3 ++-
 http.c |  5 +++--
 object-store.h |  9 +
 sha1_file.c| 11 ++-
 5 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/cache.h b/cache.h
index 09d37cb651..4b01c0de10 100644
--- a/cache.h
+++ b/cache.h
@@ -956,14 +956,6 @@ extern void check_repository_format(void);
 #define DATA_CHANGED0x0020
 #define TYPE_CHANGED0x0040
 
-/*
- * Return the name of the file in the local object database that would
- * be used to store a loose object with the specified sha1.  The
- * return value is a pointer to a statically allocated buffer that is
- * overwritten each time the function is called.
- */
-extern const char *sha1_file_name(const unsigned char *sha1);
-
 /*
  * Return an abbreviated sha1 unique within this repository's object database.
  * The result will be at least `len` characters long, and will be NUL
diff --git a/http-walker.c b/http-walker.c
index a1c6f2639b..96873bdfed 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "repository.h"
 #include "commit.h"
 #include "walker.h"
 #include "http.h"
@@ -546,7 +547,7 @@ static int fetch_object(struct walker *walker, unsigned 
char *sha1)
ret = error("File %s has bad hash", hex);
} else if (req->rename < 0) {
ret = error("unable to write sha1 filename %s",
-   sha1_file_name(req->sha1));
+   sha1_file_name(the_repository, req->sha1));
}
 
release_http_object_request(req);
diff --git a/http.c b/http.c
index ab989b88dd..c7d2b74ce2 100644
--- a/http.c
+++ b/http.c
@@ -2181,7 +2181,7 @@ struct http_object_request *new_http_object_request(const 
char *base_url,
hashcpy(freq->sha1, sha1);
freq->localfile = -1;
 
-   filename = sha1_file_name(sha1);
+   filename = sha1_file_name(the_repository, sha1);
snprintf(freq->tmpfile, sizeof(freq->tmpfile),
 "%s.temp", filename);
 
@@ -2329,7 +2329,8 @@ int finish_http_object_request(struct http_object_request 
*freq)
return -1;
}
freq->rename =
-   finalize_object_file(freq->tmpfile, sha1_file_name(freq->sha1));
+   finalize_object_file(freq->tmpfile,
+sha1_file_name(the_repository, 
freq->sha1));
 
return freq->rename;
 }
diff --git a/object-store.h b/object-store.h
index add1d4e27c..5422e80c08 100644
--- a/object-store.h
+++ b/object-store.h
@@ -61,6 +61,15 @@ struct packed_git {
char pack_name[FLEX_ARRAY]; /* more */
 };
 
+/*
+ * Return the name of the file in a repository's local object database
+ * that would be used to store a loose object with the specified sha1.
+ * The return value is a pointer to a statically allocated buffer that
+ * is overwritten each time the function is called.
+ */
+#define sha1_file_name(r, s) sha1_file_name_##r(s)
+const char *sha1_file_name_the_repository(const unsigned char *sha1);
+
 void prepare_alt_odb(struct repository *r);
 
 #endif /* OBJECT_STORE_H */
diff --git a/sha1_file.c b/sha1_file.c
index f046d560f8..65fefdf4ac 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -323,7 +323,7 @@ static void fill_sha1_path(struct strbuf *buf, const 
unsigned char *sha1)
}
 }
 
-const char *sha1_file_name(const unsigned char *sha1)
+const char *sha1_file_name_the_repository(const unsigned char *sha1)
 {
static struct strbuf buf = STRBUF_INIT;
 
@@ -721,7 +721,8 @@ int check_and_freshen_file(const char *fn, int freshen)
 
 static int check_and_freshen_local(const unsigned char *sha1, int freshen)
 {
-   return check_and_freshen_file(sha1_file_name(sha1), freshen);
+   return check_and_freshen_file(sha1_file_name(the_repository, sha1),
+ freshen);
 }
 
 static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen)
@@ -878,7 +879,7 @@ static int stat_sha1_file(const unsigned char *sha1, struct 
stat *st,
 {
struct alternate_object_database *alt;
 
-   *path = sha1_file_name(sha1);
+   *path = sha1_file_name(the_repository, sha1);
if (!lstat(*path, st))
return 0;
 
@@ -903,7 +904,7 @@ static int open_sha1_file(const unsigned char *sha1, const 
char **path)
struct alternate_object_database *alt;

[PATCH 20/26] sha1_file: add repository argument to sha1_loose_object_info

[Stefan Beller]

Add a repository argument to allow the sha1_loose_object_info caller
to be more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 1ce4058644..d0bc09a089 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1151,9 +1151,10 @@ int parse_sha1_header(const char *hdr, unsigned long 
*sizep)
return parse_sha1_header_extended(hdr, , 0);
 }
 
-static int sha1_loose_object_info(const unsigned char *sha1,
- struct object_info *oi,
- int flags)
+#define sha1_loose_object_info(r, s, o, f) sha1_loose_object_info_##r(s, o, f)
+static int sha1_loose_object_info_the_repository(const unsigned char *sha1,
+struct object_info *oi,
+int flags)
 {
int status = 0;
unsigned long mapsize;
@@ -1266,7 +1267,7 @@ int sha1_object_info_extended(const unsigned char *sha1, 
struct object_info *oi,
 
if (!find_pack_entry(real, )) {
/* Most likely it's a loose object. */
-   if (!sha1_loose_object_info(real, oi, flags))
+   if (!sha1_loose_object_info(the_repository, real, oi, flags))
return 0;
 
/* Not a loose object; someone else may have just packed it. */
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 21/26] sha1_file: allow sha1_file_name to handle arbitrary repositories

[Stefan Beller]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 object-store.h | 3 +--
 sha1_file.c| 4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/object-store.h b/object-store.h
index 0a4561f476..b4756444bb 100644
--- a/object-store.h
+++ b/object-store.h
@@ -67,8 +67,7 @@ struct packed_git {
  * The return value is a pointer to a statically allocated buffer that
  * is overwritten each time the function is called.
  */
-#define sha1_file_name(r, s) sha1_file_name_##r(s)
-const char *sha1_file_name_the_repository(const unsigned char *sha1);
+const char *sha1_file_name(struct repository *r, const unsigned char *sha1);
 #define map_sha1_file(r, s, sz) map_sha1_file_##r(s, sz)
 void *map_sha1_file_the_repository(const unsigned char *sha1, unsigned long 
*size);
 
diff --git a/sha1_file.c b/sha1_file.c
index d0bc09a089..fddada5756 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -323,12 +323,12 @@ static void fill_sha1_path(struct strbuf *buf, const 
unsigned char *sha1)
}
 }
 
-const char *sha1_file_name_the_repository(const unsigned char *sha1)
+const char *sha1_file_name(struct repository *r, const unsigned char *sha1)
 {
static struct strbuf buf = STRBUF_INIT;
 
strbuf_reset();
-   strbuf_addf(, "%s/", get_object_directory());
+   strbuf_addf(, "%s/", r->objects.objectdir);
 
fill_sha1_path(, sha1);
return buf.buf;
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 14/26] sha1_file: allow prepare_alt_odb to handle arbitrary repositories

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 object-store.h |  3 +--
 sha1_file.c| 21 +++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/object-store.h b/object-store.h
index d96a16edd1..add1d4e27c 100644
--- a/object-store.h
+++ b/object-store.h
@@ -61,7 +61,6 @@ struct packed_git {
char pack_name[FLEX_ARRAY]; /* more */
 };
 
-#define prepare_alt_odb(r) prepare_alt_odb_##r()
-extern void prepare_alt_odb_the_repository(void);
+void prepare_alt_odb(struct repository *r);
 
 #endif /* OBJECT_STORE_H */
diff --git a/sha1_file.c b/sha1_file.c
index d18ce3aeba..f046d560f8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -677,21 +677,22 @@ int foreach_alt_odb(alt_odb_fn fn, void *cb)
return r;
 }
 
-void prepare_alt_odb_the_repository(void)
+void prepare_alt_odb(struct repository *r)
 {
-   const char *alt;
-
-   if (the_repository->objects.alt_odb_tail)
+   if (r->objects.alt_odb_tail)
return;
 
-   alt = getenv(ALTERNATE_DB_ENVIRONMENT);
+   r->objects.alt_odb_tail = >objects.alt_odb_list;
+
+   if (!r->ignore_env) {
+   const char *alt = getenv(ALTERNATE_DB_ENVIRONMENT);
+   if (!alt)
+   alt = "";
 
-   the_repository->objects.alt_odb_tail =
-   _repository->objects.alt_odb_list;
-   link_alt_odb_entries(the_repository, alt,
-PATH_SEP, NULL, 0);
+   link_alt_odb_entries(r, alt, PATH_SEP, NULL, 0);
+   }
 
-   read_info_alternates(the_repository, get_object_directory(), 0);
+   read_info_alternates(r, r->objects.objectdir, 0);
 }
 
 /* Returns 1 if we have successfully freshened the file, 0 otherwise. */
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 08/26] sha1_file: add raw_object_store argument to alt_odb_usable

[Stefan Beller]

Add a raw_object_store to alt_odb_usable to be more specific about which
repository to act on. The choice of the repository is delegated to its
only caller link_alt_odb_entry.

Signed-off-by: Stefan Beller 
---
 sha1_file.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 2826d5d6ed..d25b68a8fb 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -350,7 +350,9 @@ static const char *alt_sha1_path(struct 
alternate_object_database *alt,
 /*
  * Return non-zero iff the path is usable as an alternate object database.
  */
-static int alt_odb_usable(struct strbuf *path, const char *normalized_objdir)
+static int alt_odb_usable(struct raw_object_store *o,
+ struct strbuf *path,
+ const char *normalized_objdir)
 {
struct alternate_object_database *alt;
 
@@ -366,7 +368,7 @@ static int alt_odb_usable(struct strbuf *path, const char 
*normalized_objdir)
 * Prevent the common mistake of listing the same
 * thing twice, or object directory itself.
 */
-   for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next) {
+   for (alt = o->alt_odb_list; alt; alt = alt->next) {
if (!fspathcmp(path->buf, alt->path))
return 0;
}
@@ -418,7 +420,7 @@ static int link_alt_odb_entry(const char *entry, const char 
*relative_base,
while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')
strbuf_setlen(, pathbuf.len - 1);
 
-   if (!alt_odb_usable(, normalized_objdir)) {
+   if (!alt_odb_usable(_repository->objects, , 
normalized_objdir)) {
strbuf_release();
return -1;
}
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 00/26] Moving global state into the repository object (part 1)

[Stefan Beller]

This is a real take on the first part of the recent RFC[1].

Jonathan Tan suggested[2] that "sha1_loose_object_info to handle arbitrary 
repositories"
might be a good breaking point for a first part at that RFC at patch 38.
This series is smaller and contains only 26 patches as the patches in the big
RFC were slightly out of order.

I developed this series partly by writing patches, but mostly by cherrypicking
from that RFC on top of current master. I noticed no external conflicts apart
from one addition to the repositories _INIT macro, which was easy to resolve.

Comments in the early range of that RFC were on 003 where Junio pointed out
that the coccinelle patch ought to be not in contrib/coccinelle, so I put it
in a sub directory there, as 'make coccicheck' doesn't traverse subdirs.

brian had a questoin on patch 25 in the RFC, but that seemed to resolve itself
without any suggestion to include into this series[3].

Duy suggested that we shall not use the repository blindly, but should carefully
examine whether to pass on an object store or the refstore or such[4], which 
I agree with if it makes sense. This series unfortunately has an issue with that
as I would not want to pass down the `ignore_env` flag separately from the 
object
store, so I made all functions that only take the object store to have the raw
object store as the first parameter, and others using the full repository.

Eric Sunshine brought up memory leaks with the RFC, and I would think to
have plugged all holes.

[1] https://public-inbox.org/git/20180205235508.216277-1-sbel...@google.com/
[2] 
https://public-inbox.org/git/20180207143300.ce1c39ca07f6a0d64fe0e...@google.com/
[3] 
https://public-inbox.org/git/20180206011940.gd7...@genre.crustytoothpaste.net/
[4] 
https://public-inbox.org/git/cacsjy8cggekpx4czkyytsprj87uqvkzsol7fyt__p2dh_1l...@mail.gmail.com/

Thanks,
Stefan

Jonathan Nieder (8):
  pack: move prepare_packed_git_run_once to object store
  pack: move approximate object count to object store
  sha1_file: add repository argument to sha1_file_name
  sha1_file: add repository argument to map_sha1_file
  sha1_file: allow stat_sha1_file to handle arbitrary repositories
  sha1_file: allow open_sha1_file to handle arbitrary repositories
  sha1_file: allow map_sha1_file_1 to handle arbitrary repositories
  sha1_file: allow sha1_loose_object_info to handle arbitrary
repositories

Stefan Beller (18):
  repository: introduce raw object store field
  object-store: move alt_odb_list and alt_odb_tail to object store
  object-store: free alt_odb_list
  object-store: move packed_git and packed_git_mru to object store
  object-store: close all packs upon clearing the object store
  sha1_file: add raw_object_store argument to alt_odb_usable
  sha1_file: add repository argument to link_alt_odb_entry
  sha1_file: add repository argument to read_info_alternates
  sha1_file: add repository argument to link_alt_odb_entries
  sha1_file: add repository argument to prepare_alt_odb
  sha1_file: allow link_alt_odb_entries to handle arbitrary repositories
  sha1_file: allow prepare_alt_odb to handle arbitrary repositories
  sha1_file: add repository argument to stat_sha1_file
  sha1_file: add repository argument to open_sha1_file
  sha1_file: add repository argument to map_sha1_file_1
  sha1_file: add repository argument to sha1_loose_object_info
  sha1_file: allow sha1_file_name to handle arbitrary repositories
  sha1_file: allow map_sha1_file to handle arbitrary repositories

 builtin/am.c|   2 +-
 builtin/clone.c |   2 +-
 builtin/count-objects.c |   6 +-
 builtin/fetch.c |   2 +-
 builtin/fsck.c  |  13 ++-
 builtin/gc.c|   4 +-
 builtin/grep.c  |   2 +-
 builtin/index-pack.c|   1 +
 builtin/merge.c |   2 +-
 builtin/pack-objects.c  |  21 ++--
 builtin/pack-redundant.c|   6 +-
 builtin/receive-pack.c  |   3 +-
 cache.h |  46 ++--
 contrib/coccinelle/refactoring/packed_git.cocci |   7 ++
 environment.c   |   5 +-
 fast-import.c   |   6 +-
 http-backend.c  |   6 +-
 http-push.c |   1 +
 http-walker.c   |   4 +-
 http.c  |   6 +-
 mru.h   |   1 +
 object-store.h  |  75 +
 object.c|  26 +
 pack-bitmap.c   |   4 +-
 pack-check.c|   1 +
 pack-revindex.c

[PATCH 07/26] pack: move approximate object count to object store

[Stefan Beller]

From: Jonathan Nieder 

The approximate_object_count() function maintains a rough count of
objects in a repository to estimate how long object name abbreviates
should be.  Object names are scoped to a repository and the
appropriate length may differ by repository, so the object count
should not be global.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 object-store.h | 10 +-
 packfile.c | 11 +--
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/object-store.h b/object-store.h
index 9fd109fa9c..86f1ec2cd0 100644
--- a/object-store.h
+++ b/object-store.h
@@ -21,13 +21,21 @@ struct raw_object_store {
struct alternate_object_database *alt_odb_list;
struct alternate_object_database **alt_odb_tail;
 
+   /*
+* A fast, rough count of the number of objects in the repository.
+* These two fields are not meant for direct access. Use
+* approximate_object_count() instead.
+*/
+   unsigned long approximate_object_count;
+   unsigned approximate_object_count_valid : 1;
+
/*
 * Whether packed_git has already been populated with this repository's
 * packs.
 */
unsigned packed_git_initialized : 1;
 };
-#define RAW_OBJECT_STORE_INIT { NULL, NULL, MRU_INIT, NULL, NULL, 0 }
+#define RAW_OBJECT_STORE_INIT { NULL, NULL, MRU_INIT, NULL, NULL, 0, 0, 0 }
 
 void raw_object_store_clear(struct raw_object_store *o);
 
diff --git a/packfile.c b/packfile.c
index c107fce171..a53706cf16 100644
--- a/packfile.c
+++ b/packfile.c
@@ -793,8 +793,6 @@ static void prepare_packed_git_one(char *objdir, int local)
strbuf_release();
 }
 
-static int approximate_object_count_valid;
-
 /*
  * Give a fast, rough count of the number of objects in the repository. This
  * ignores loose objects completely. If you have a lot of them, then either
@@ -804,8 +802,8 @@ static int approximate_object_count_valid;
  */
 unsigned long approximate_object_count(void)
 {
-   static unsigned long count;
-   if (!approximate_object_count_valid) {
+   if (!the_repository->objects.approximate_object_count_valid) {
+   unsigned long count;
struct packed_git *p;
 
prepare_packed_git();
@@ -815,8 +813,9 @@ unsigned long approximate_object_count(void)
continue;
count += p->num_objects;
}
+   the_repository->objects.approximate_object_count = count;
}
-   return count;
+   return the_repository->objects.approximate_object_count;
 }
 
 static void *get_next_packed_git(const void *p)
@@ -890,7 +889,7 @@ void prepare_packed_git(void)
 
 void reprepare_packed_git(void)
 {
-   approximate_object_count_valid = 0;
+   the_repository->objects.approximate_object_count_valid = 0;
the_repository->objects.packed_git_initialized = 0;
prepare_packed_git();
 }
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 05/26] object-store: close all packs upon clearing the object store

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 builtin/am.c   | 2 +-
 builtin/clone.c| 2 +-
 builtin/fetch.c| 2 +-
 builtin/merge.c| 2 +-
 builtin/receive-pack.c | 2 +-
 object.c   | 6 ++
 packfile.c | 4 ++--
 packfile.h | 2 +-
 8 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index acfe9d3c8c..75e16dfec6 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1859,7 +1859,7 @@ static void am_run(struct am_state *state, int resume)
 */
if (!state->rebasing) {
am_destroy(state);
-   close_all_packs();
+   close_all_packs(_repository->objects);
run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
}
 }
diff --git a/builtin/clone.c b/builtin/clone.c
index 284651797e..7ec08d4696 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1200,7 +1200,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
transport_disconnect(transport);
 
if (option_dissociate) {
-   close_all_packs();
+   close_all_packs(_repository->objects);
dissociate_from_references();
}
 
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7bbcd26faf..70015ccdc7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1407,7 +1407,7 @@ int cmd_fetch(int argc, const char **argv, const char 
*prefix)
 
string_list_clear(, 0);
 
-   close_all_packs();
+   close_all_packs(_repository->objects);
 
argv_array_pushl(_gc_auto, "gc", "--auto", NULL);
if (verbosity < 0)
diff --git a/builtin/merge.c b/builtin/merge.c
index 30264cfd7c..907ae44ab5 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -411,7 +411,7 @@ static void finish(struct commit *head_commit,
 * We ignore errors in 'gc --auto', since the
 * user should see them.
 */
-   close_all_packs();
+   close_all_packs(_repository->objects);
run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
}
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b2eac79a6e..954fc72c7c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2027,7 +2027,7 @@ int cmd_receive_pack(int argc, const char **argv, const 
char *prefix)
proc.git_cmd = 1;
proc.argv = argv_gc_auto;
 
-   close_all_packs();
+   close_all_packs(_repository->objects);
if (!start_command()) {
if (use_sideband)
copy_to_sideband(proc.err, -1, NULL);
diff --git a/object.c b/object.c
index 81d083bec1..79c2c447bc 100644
--- a/object.c
+++ b/object.c
@@ -4,6 +4,7 @@
 #include "tree.h"
 #include "commit.h"
 #include "tag.h"
+#include "packfile.h"
 
 static struct object **obj_hash;
 static int nr_objs, obj_hash_size;
@@ -468,8 +469,5 @@ void raw_object_store_clear(struct raw_object_store *o)
o->alt_odb_tail = NULL;
 
mru_clear(>packed_git_mru);
-   /*
-* TODO: call close_all_packs once migrated to
-* take an object store argument
-*/
+   close_all_packs(o);
 }
diff --git a/packfile.c b/packfile.c
index 11b0e6613c..2ce1b2cef7 100644
--- a/packfile.c
+++ b/packfile.c
@@ -306,11 +306,11 @@ static void close_pack(struct packed_git *p)
close_pack_index(p);
 }
 
-void close_all_packs(void)
+void close_all_packs(struct raw_object_store *o)
 {
struct packed_git *p;
 
-   for (p = the_repository->objects.packed_git; p; p = p->next)
+   for (p = o->packed_git; p; p = p->next)
if (p->do_not_close)
die("BUG: want to close pack marked 'do-not-close'");
else
diff --git a/packfile.h b/packfile.h
index 0cdeb54dcd..fa547226b7 100644
--- a/packfile.h
+++ b/packfile.h
@@ -61,7 +61,7 @@ extern void close_pack_index(struct packed_git *);
 
 extern unsigned char *use_pack(struct packed_git *, struct pack_window **, 
off_t, unsigned long *);
 extern void close_pack_windows(struct packed_git *);
-extern void close_all_packs(void);
+extern void close_all_packs(struct raw_object_store *o);
 extern void unuse_pack(struct pack_window **);
 extern void clear_delta_base_cache(void);
 extern struct packed_git *add_packed_git(const char *path, size_t path_len, 
int local);
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 03/26] object-store: free alt_odb_list

[Stefan Beller]

Free the memory and reset alt_odb_{list, tail} to NULL.

Signed-off-by: Stefan Beller 
---
 object.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/object.c b/object.c
index 9b5b65e189..d778f09717 100644
--- a/object.c
+++ b/object.c
@@ -446,7 +446,24 @@ void clear_commit_marks_all(unsigned int flags)
}
 }
 
+static void free_alt_odb(struct alternate_object_database *alt)
+{
+   strbuf_release(>scratch);
+   oid_array_clear(>loose_objects_cache);
+}
+
+static void free_alt_odbs(struct raw_object_store *o)
+{
+   while (o->alt_odb_list) {
+   free_alt_odb(o->alt_odb_list);
+   o->alt_odb_list = o->alt_odb_list->next;
+   }
+}
+
 void raw_object_store_clear(struct raw_object_store *o)
 {
free(o->objectdir);
+
+   free_alt_odbs(o);
+   o->alt_odb_tail = NULL;
 }
-- 
2.16.1.73.ga2c3e9663f.dirty

[PATCH 02/26] object-store: move alt_odb_list and alt_odb_tail to object store

[Stefan Beller]

In a process with multiple repositories open, alternates should be
associated to a single repository and not shared globally. Move
alt_odb_list and alt_odb_tail into the_repository and adjust callers
to reflect this.

Now that the alternative object data base is per repository, we're
leaking its memory upon freeing a repository. The next patch plugs
this hole.

No functional change intended.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 builtin/fsck.c |  4 +++-
 cache.h|  4 ++--
 object-store.h |  7 ++-
 packfile.c |  3 ++-
 sha1_file.c| 25 -
 sha1_name.c|  3 ++-
 6 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 04846d46f9..1048255da1 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "repository.h"
 #include "config.h"
 #include "commit.h"
 #include "tree.h"
@@ -694,7 +695,8 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
fsck_object_dir(get_object_directory());
 
prepare_alt_odb();
-   for (alt = alt_odb_list; alt; alt = alt->next)
+   for (alt = the_repository->objects.alt_odb_list;
+   alt; alt = alt->next)
fsck_object_dir(alt->path);
 
if (check_full) {
diff --git a/cache.h b/cache.h
index d8b975a571..918c2f15b4 100644
--- a/cache.h
+++ b/cache.h
@@ -1573,7 +1573,7 @@ extern int has_dirs_only_path(const char *name, int len, 
int prefix_len);
 extern void schedule_dir_for_removal(const char *name, int len);
 extern void remove_scheduled_dirs(void);
 
-extern struct alternate_object_database {
+struct alternate_object_database {
struct alternate_object_database *next;
 
/* see alt_scratch_buf() */
@@ -1591,7 +1591,7 @@ extern struct alternate_object_database {
struct oid_array loose_objects_cache;
 
char path[FLEX_ARRAY];
-} *alt_odb_list;
+};
 extern void prepare_alt_odb(void);
 extern char *compute_alternate_path(const char *path, struct strbuf *err);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
diff --git a/object-store.h b/object-store.h
index cf35760ceb..44b8f84753 100644
--- a/object-store.h
+++ b/object-store.h
@@ -1,14 +1,19 @@
 #ifndef OBJECT_STORE_H
 #define OBJECT_STORE_H
 
+#include "cache.h"
+
 struct raw_object_store {
/*
 * Path to the repository's object store.
 * Cannot be NULL after initialization.
 */
char *objectdir;
+
+   struct alternate_object_database *alt_odb_list;
+   struct alternate_object_database **alt_odb_tail;
 };
-#define RAW_OBJECT_STORE_INIT { NULL }
+#define RAW_OBJECT_STORE_INIT { NULL, NULL, NULL }
 
 void raw_object_store_clear(struct raw_object_store *o);
 
diff --git a/packfile.c b/packfile.c
index 4a5fe7ab18..d61076faaf 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "mru.h"
 #include "pack.h"
+#include "repository.h"
 #include "dir.h"
 #include "mergesort.h"
 #include "packfile.h"
@@ -880,7 +881,7 @@ void prepare_packed_git(void)
return;
prepare_packed_git_one(get_object_directory(), 1);
prepare_alt_odb();
-   for (alt = alt_odb_list; alt; alt = alt->next)
+   for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next)
prepare_packed_git_one(alt->path, 0);
rearrange_packed_git();
prepare_packed_git_mru();
diff --git a/sha1_file.c b/sha1_file.c
index 3da70ac650..2826d5d6ed 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -22,6 +22,7 @@
 #include "pack-revindex.h"
 #include "sha1-lookup.h"
 #include "bulk-checkin.h"
+#include "repository.h"
 #include "streaming.h"
 #include "dir.h"
 #include "mru.h"
@@ -346,9 +347,6 @@ static const char *alt_sha1_path(struct 
alternate_object_database *alt,
return buf->buf;
 }
 
-struct alternate_object_database *alt_odb_list;
-static struct alternate_object_database **alt_odb_tail;
-
 /*
  * Return non-zero iff the path is usable as an alternate object database.
  */
@@ -368,7 +366,7 @@ static int alt_odb_usable(struct strbuf *path, const char 
*normalized_objdir)
 * Prevent the common mistake of listing the same
 * thing twice, or object directory itself.
 */
-   for (alt = alt_odb_list; alt; alt = alt->next) {
+   for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next) {
if (!fspathcmp(path->buf, alt->path))
return 0;
}
@@ -428,8 +426,8 @@ static int link_alt_odb_entry(const char *entry, const char 
*relative_base,
ent = alloc_alt_odb(pathbuf.buf);
 
/* add the alternate entry */
-   *alt_odb_tail = ent;
-   alt_odb_tail = &(ent->next);
+   *the_repository->objects.alt_odb_tail = ent;
+   

[PATCH 01/26] repository: introduce raw object store field

[Stefan Beller]

The raw object store field will contain any objects needed for
access to objects in a given repository.

This patch introduces the raw object store and populates it with the
`objectdir`, which used to be part of the repository struct.

As the struct gains members, we'll also populate the function to clear
the memory for these members.

In a later we'll introduce a struct object_parser, that will complement
the object parsing in a repository struct: The raw object parser is the
layer that will provide access to raw object content, while the higher
level object parser code will parse raw objects and keeps track of
parenthood and other object relationships using 'struct object'.
For now only add the lower level to the repository struct.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 builtin/grep.c |  2 +-
 environment.c  |  5 +++--
 object-store.h | 15 +++
 object.c   |  5 +
 path.c |  2 +-
 repository.c   | 17 +
 repository.h   |  7 ---
 7 files changed, 42 insertions(+), 11 deletions(-)
 create mode 100644 object-store.h

diff --git a/builtin/grep.c b/builtin/grep.c
index 3ca4ac80d8..0f0c195705 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -432,7 +432,7 @@ static int grep_submodule(struct grep_opt *opt, struct 
repository *superproject,
 * object.
 */
grep_read_lock();
-   add_to_alternates_memory(submodule.objectdir);
+   add_to_alternates_memory(submodule.objects.objectdir);
grep_read_unlock();
 
if (oid) {
diff --git a/environment.c b/environment.c
index 63ac38a46f..5d7ffb3a3f 100644
--- a/environment.c
+++ b/environment.c
@@ -13,6 +13,7 @@
 #include "refs.h"
 #include "fmt-merge-msg.h"
 #include "commit.h"
+#include "object-store.h"
 
 int trust_executable_bit = 1;
 int trust_ctime = 1;
@@ -242,9 +243,9 @@ const char *get_git_work_tree(void)
 
 char *get_object_directory(void)
 {
-   if (!the_repository->objectdir)
+   if (!the_repository->objects.objectdir)
BUG("git environment hasn't been setup");
-   return the_repository->objectdir;
+   return the_repository->objects.objectdir;
 }
 
 int odb_mkstemp(struct strbuf *template, const char *pattern)
diff --git a/object-store.h b/object-store.h
new file mode 100644
index 00..cf35760ceb
--- /dev/null
+++ b/object-store.h
@@ -0,0 +1,15 @@
+#ifndef OBJECT_STORE_H
+#define OBJECT_STORE_H
+
+struct raw_object_store {
+   /*
+* Path to the repository's object store.
+* Cannot be NULL after initialization.
+*/
+   char *objectdir;
+};
+#define RAW_OBJECT_STORE_INIT { NULL }
+
+void raw_object_store_clear(struct raw_object_store *o);
+
+#endif /* OBJECT_STORE_H */
diff --git a/object.c b/object.c
index 0afdfd19b7..9b5b65e189 100644
--- a/object.c
+++ b/object.c
@@ -445,3 +445,8 @@ void clear_commit_marks_all(unsigned int flags)
obj->flags &= ~flags;
}
 }
+
+void raw_object_store_clear(struct raw_object_store *o)
+{
+   free(o->objectdir);
+}
diff --git a/path.c b/path.c
index da8b655730..81a42d9115 100644
--- a/path.c
+++ b/path.c
@@ -382,7 +382,7 @@ static void adjust_git_path(const struct repository *repo,
strbuf_splice(buf, 0, buf->len,
  repo->index_file, strlen(repo->index_file));
else if (dir_prefix(base, "objects"))
-   replace_dir(buf, git_dir_len + 7, repo->objectdir);
+   replace_dir(buf, git_dir_len + 7, repo->objects.objectdir);
else if (git_hooks_path && dir_prefix(base, "hooks"))
replace_dir(buf, git_dir_len + 5, git_hooks_path);
else if (repo->different_commondir)
diff --git a/repository.c b/repository.c
index f66fcb1342..bd2ad578de 100644
--- a/repository.c
+++ b/repository.c
@@ -1,11 +1,18 @@
 #include "cache.h"
 #include "repository.h"
+#include "object-store.h"
 #include "config.h"
 #include "submodule-config.h"
 
 /* The main repository */
 static struct repository the_repo = {
-   NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, _index, 
_algos[GIT_HASH_SHA1], 0, 0
+   NULL, NULL,
+   RAW_OBJECT_STORE_INIT,
+   NULL, NULL, NULL,
+   NULL, NULL, NULL,
+   _index,
+   _algos[GIT_HASH_SHA1],
+   0, 0
 };
 struct repository *the_repository = _repo;
 
@@ -42,8 +49,8 @@ static void repo_setup_env(struct repository *repo)
!repo->ignore_env);
free(repo->commondir);
repo->commondir = strbuf_detach(, NULL);
-   free(repo->objectdir);
-   repo->objectdir = git_path_from_env(DB_ENVIRONMENT, repo->commondir,
+   free(repo->objects.objectdir);
+   repo->objects.objectdir = git_path_from_env(DB_ENVIRONMENT, 
repo->commondir,
"objects", !repo->ignore_env);
free(repo->graft_file);
repo->graft_file = 

Re: please change stash

[Andrew Ardill]

Hi Karsten,

> Normal git tooling creates different files file.ORIG file.LOCAL
> file.REMOTE in case of conflicts.

Which tools are you referring to here? Can you give a short sequence
of commands that show what you mean?

> However `git stash pop` manipulates your files directly resulting in
> lines like:
>
> <<< Updated upstream
>
 Stashed changes
>
> This can seriously corrupt files and workflows.

This looks like a normal merge conflict. I suspect that you are using
tools that know how to deal with this format when it used the merge
conflict markers, but maybe not the equivalent markers you get when
popping a conflicting stash.

To demonstrate, here is a short script:

git init test
cd test
echo "base file" >test
git commit -m "base file"
git add test
git commit -m "base file"
git checkout -b conflict_branch
echo "conflicting file" >test
git commit -am "conflict file"
git checkout master
echo "updated file" >test
git commit -am "updated file"
git merge conflict_branch


This merge fails, and the file 'test' looks like this:

<<< HEAD
updated file
===
conflicting file
>>> conflict_branch

As you can see, this sequence of actions doesn't result in 3 different files.

The merge conflict format is a relatively old one, and lots of tools
know how to use it in different ways (such as the tool you are using,
I presume) but say this was to be changed for the stash operation -
what would you propose replace it?
Some options might be to:
- instead of placing the conflicts in the original file, place the
different conflicting versions into different files
- warn when adding/committing/pushing files with conflict markers in them
- teach the tool you are using to handle the stash conflict markers in
a nicer way

Some of these may be possible to do with little work.
This link[0] on stack overflow deals with creating separate files, and
it looks like it might work for stash pop conflicts.
This one[1] shows how to create hooks that catch any conflicts that
are being committed, and would also probably work with stash
conflicts.

Teaching the tool to handle stash conflicts, or making any of the
above changes to the base distribution of git would be significantly
harder, but maybe this can help you in the meantime.

Regards,

Andrew Ardill

[0] 
https://stackoverflow.com/questions/47512337/configure-git-to-create-multiple-files-for-merge-conflicts
[1] 
https://stackoverflow.com/questions/24213948/prevent-file-with-merge-conflicts-from-getting-committed-in-git

Re: [PATCH v3 02/35] pkt-line: introduce struct packet_reader

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

> Subject: pkt-line: introduce struct packet_reader

nit: this subject line doesn't describe what the purpose/intent behind
the patch is.  Maybe something like

pkt-line: allow peeking at a packet line without consuming it

would make it clearer.

> Sometimes it is advantageous to be able to peek the next packet line
> without consuming it (e.g. to be able to determine the protocol version
> a server is speaking).  In order to do that introduce 'struct
> packet_reader' which is an abstraction around the normal packet reading
> logic.  This enables a caller to be able to peek a single line at a time
> using 'packet_reader_peek()' and having a caller consume a line by
> calling 'packet_reader_read()'.

Makes sense.  The packet_reader owns a buffer to support the peek
operation and make buffer reuse a little easier.

[...]
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -111,6 +111,64 @@ char *packet_read_line_buf(char **src_buf, size_t 
> *src_len, int *size);
>   */
>  ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
>  
> +struct packet_reader {
> + /* source file descriptor */
> + int fd;
> +
> + /* source buffer and its size */
> + char *src_buffer;
> + size_t src_len;

Can or should this be a strbuf?

> +
> + /* buffer that pkt-lines are read into and its size */
> + char *buffer;
> + unsigned buffer_size;

Likewise.

> +
> + /* options to be used during reads */
> + int options;

What option values are possible?

> +
> + /* status of the last read */
> + enum packet_read_status status;

This reminds me of FILE*'s status value, ferror, etc.  I'm mildly
nervous about it --- it encourages a style of error handling where you
ignore errors from an individual operation and hope the recorded
status later has the most relevant error.

I think it is being used to support peek --- you need to record the
error to reply it.  Is that right?  I wonder if it would make sense to
structure as

struct packet_read_result last_line_read;
};

struct packet_read_result {
enum packet_read_status status;

const char *line;
int len;
};

What you have here also seems fine.  I think what would help most
for readability is if the comment mentioned the purpose --- e.g.

/* status of the last read, to support peeking */

Or if the contract were tied to the purpose:

/* status of the last read, only valid if line_peeked is true */

[...]
> +/*
> + * Initialize a 'struct packet_reader' object which is an
> + * abstraction around the 'packet_read_with_status()' function.
> + */
> +extern void packet_reader_init(struct packet_reader *reader, int fd,
> +char *src_buffer, size_t src_len,
> +int options);

This comment doesn't describe how I should use the function.  Is the
intent to point the reader to packet_read_with_status for more details
about the arguments?

Can src_buffer be a const char *?

[...]
> +/*
> + * Perform a packet read and return the status of the read.

nit: s/Perform a packet read/Read one pkt-line/

> + * The values of 'pktlen' and 'line' are updated based on the status of the
> + * read as follows:
> + *
> + * PACKET_READ_ERROR: 'pktlen' is set to '-1' and 'line' is set to NULL
> + * PACKET_READ_NORMAL: 'pktlen' is set to the number of bytes read
> + *  'line' is set to point at the read line
> + * PACKET_READ_FLUSH: 'pktlen' is set to '0' and 'line' is set to NULL
> + */
> +extern enum packet_read_status packet_reader_read(struct packet_reader 
> *reader);

This is reasonable.  As described above an alternative would be
possible to have a separate packet_read_result output parameter but
the interface described here looks pretty easy/pleasant to use.

> +
> +/*
> + * Peek the next packet line without consuming it and return the status.

nit: s/Peek/Peek at/, or s/Peek/Read/

> + * The next call to 'packet_reader_read()' will perform a read of the same 
> line
> + * that was peeked, consuming the line.

nit: s/peeked/peeked at/

> + *
> + * Peeking multiple times without calling 'packet_reader_read()' will return
> + * the same result.
> + */
> +extern enum packet_read_status packet_reader_peek(struct packet_reader 
> *reader);

Nice.

[...]
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -406,3 +406,62 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct 
> strbuf *sb_out)
>   }
>   return sb_out->len - orig_len;
>  }
> +
> +/* Packet Reader Functions */
> +void packet_reader_init(struct packet_reader *reader, int fd,
> + char *src_buffer, size_t src_len,
> + int options)

This comment looks like it's attached to packet_reader_init, but it's
meant to be attached to the whole collection.  It's possible that this
title-above-multiple-functions won't be maintained, but that's okay.

> +{
> + 

[PATCH] t6300-for-each-ref: fix "more than one quoting style" tests

[SZEDER Gábor]

'git for-each-ref' should error out when invoked with more than one
quoting style options.  The tests checking this have two issues:

  - They run 'git for-each-ref' upstream of a pipe, hiding its exit
code, thus don't actually checking that 'git for-each-ref' exits
with error code.

  - They check the error message in a rather roundabout way.

Ensure that 'git for-each-ref' exits with an error code using the
'test_must_fail' helper function, and check its error message by
grepping its saved standard error.

Signed-off-by: SZEDER Gábor 
---

Those tests were like this since ancient times, c9ecf4f12a
(for-each-ref: Fix quoting style constants., 2007-12-06).


 t/t6300-for-each-ref.sh | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index c128dfc579..295d1475bd 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -373,11 +373,8 @@ test_expect_success 'Quoting style: tcl' '
 
 for i in "--perl --shell" "-s --python" "--python --tcl" "--tcl --perl"; do
test_expect_success "more than one quoting style: $i" "
-   git for-each-ref $i 2>&1 | (read line &&
-   case \$line in
-   \"error: more than one quoting style\"*) : happy;;
-   *) false
-   esac)
+   test_must_fail git for-each-ref $i 2>err &&
+   grep '^error: more than one quoting style' err
"
 done
 
-- 
2.16.1.181.g4b60b0bfb6

Re: Question about rebasing - unexpected result, can't figure out why

[brian m. carlson]

On Sat, Feb 10, 2018 at 09:47:57PM +0100, Jonas Thiem wrote:
> == Why did I expect that ==
> 
> Of course after the client rebase, C3.txt should be gone (since it's
> gone at the original last commit of the client branch).
> 
> But since it still exists in the server branch at the final commit,
> after rebasing & reapplying that onto the master, shouldn't it be put
> back in? Also, I would expect C3 -> C4 -> C10 as the complete chain of
> commits of the remaining server branch to be attached at the end, not
> just C4 -> C10...
> 
> Does git somehow detect C3 would be applied twice? Doesn't the commit
> hash of C3 change after it is rebased? So how exactly would it figure
> that out? I'm really unsure about what is going on.

Yes.  The git rebase documentation says, "Note that any commits in HEAD
which introduce the same textual changes as a commit in HEAD..
are omitted (i.e., a patch already accepted upstream with a different
commit message or timestamp will be skipped)."

If you want to be very explicit about replaying these commits, you can
say "git rebase --onto master $(git merge-base master HEAD)", in which
case C3 will be replayed.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v3 01/35] pkt-line: introduce packet_read_with_status

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

> The current pkt-line API encodes the status of a pkt-line read in the
> length of the read content.  An error is indicated with '-1', a flush
> with '0' (which can be confusing since a return value of '0' can also
> indicate an empty pkt-line), and a positive integer for the length of
> the read content otherwise.  This doesn't leave much room for allowing
> the addition of additional special packets in the future.
>
> To solve this introduce 'packet_read_with_status()' which reads a packet
> and returns the status of the read encoded as an 'enum packet_status'
> type.  This allows for easily identifying between special and normal
> packets as well as errors.  It also enables easily adding a new special
> packet in the future.

Makes sense, thanks.  Using an enum return value is less opaque, too.

[...]
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -280,28 +280,33 @@ static int packet_length(const char *linelen)
>   return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
>  }
>  
> -int packet_read(int fd, char **src_buf, size_t *src_len,
> - char *buffer, unsigned size, int options)
> +enum packet_read_status packet_read_with_status(int fd, char **src_buffer, 
> size_t *src_len,
> + char *buffer, unsigned size, 
> int *pktlen,
> + int options)

This function definition straddles two worlds: it is line-wrapped as
though there are a limited number of columns, but it goes far past 80
columns.

Can "make style" or a similar tool take care of rewrapping it?


>  {
> - int len, ret;
> + int len;
>   char linelen[4];
>  
> - ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
> - if (ret < 0)
> - return ret;
> + if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0)
> + return PACKET_READ_EOF;
> +

EOF is indeed the only error that get_packet_data can return.

Could be worth a doc comment on get_packet_data to make that clearer.
It's not too important since it's static, though.

>   len = packet_length(linelen);
> - if (len < 0)
> +
> + if (len < 0) {
>   die("protocol error: bad line length character: %.4s", linelen);
> - if (!len) {
> + } else if (!len) {
>   packet_trace("", 4, 0);
> - return 0;
> + return PACKET_READ_FLUSH;

The advertised change. Makes sense.

[...]
> - if (len >= size)
> + if ((unsigned)len >= size)
>   die("protocol error: bad line length %d", len);

The comparison is safe since we just checked that len >= 0.

Is there some static analysis that can make this kind of operation
easier?

[...]
> @@ -309,7 +314,31 @@ int packet_read(int fd, char **src_buf, size_t *src_len,
>  
>   buffer[len] = 0;
>   packet_trace(buffer, len, 0);
> - return len;
> + *pktlen = len;
> + return PACKET_READ_NORMAL;
> +}
> +
> +int packet_read(int fd, char **src_buffer, size_t *src_len,
> + char *buffer, unsigned size, int options)
> +{
> + enum packet_read_status status;
> + int pktlen;
> +
> + status = packet_read_with_status(fd, src_buffer, src_len,
> +  buffer, size, ,
> +  options);
> + switch (status) {
> + case PACKET_READ_EOF:
> + pktlen = -1;
> + break;
> + case PACKET_READ_NORMAL:
> + break;
> + case PACKET_READ_FLUSH:
> + pktlen = 0;
> + break;
> + }
> +
> + return pktlen;

nit: can simplify by avoiding the status temporary:

int pktlen;

switch (packet_read_with_status(...)) {
case PACKET_READ_EOF:
return -1;
case PACKET_READ_FLUSH:
return 0;
case PACKET_READ_NORMAL:
return pktlen;
}

As a bonus, that lets static analyzers check that the cases are
exhaustive.  (On the other hand, C doesn't guarantee that an enum can
only have the values listed as enumerators.  Did we end up figuring
out a way to handle that, beyond always including a 'default: BUG()'?)

> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -65,6 +65,21 @@ int write_packetized_from_buf(const char *src_in, size_t 
> len, int fd_out);
>  int packet_read(int fd, char **src_buffer, size_t *src_len, char
>   *buffer, unsigned size, int options);
>  
> +/*
> + * Read a packetized line into a buffer like the 'packet_read()' function but
> + * returns an 'enum packet_read_status' which indicates the status of the 
> read.
> + * The number of bytes read will be assigined to *pktlen if the status of the
> + * read was 'PACKET_READ_NORMAL'.
> + */
> +enum packet_read_status {
> + PACKET_READ_EOF = -1,
> + PACKET_READ_NORMAL,
> + PACKET_READ_FLUSH,
> +};

nit: do any callers treat the return value as a number?  It would be
less magical if the 

Re: [PATCH 3/7] worktree move: new command

[Duy Nguyen]

On Mon, Feb 12, 2018 at 11:15:06PM +0100, Martin Ågren wrote:
> On 12 February 2018 at 10:56, Duy Nguyen  wrote:
> > On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren  wrote:
> >> On 6 February 2018 at 03:13, Jeff King  wrote:
> >>> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:
>  I learned SANITIZE=leak today! It not only catches this but also "dst".
> 
>  Jeff is there any ongoing effort to make the test suite pass with
>  SANITIZE=leak? My t2038 passed, so I went ahead with the full test
>  suite and saw so many failures. I did see in your original mails that
>  you focused on t and t0001 only..
> >>>
> >>> Yeah, I did those two scripts to try to prove to myself that the
> >>> approach was good. But I haven't really pushed it any further.
> >>>
> >>> Martin Ågren (cc'd) did some follow-up work, but I think we still have a
> >>> long way to go.
> >>
> >> Agreed. :-)
> >
> > Should we mark the test files that pass SANITIZE=leak and run as a sub
> > testsuite? This way we can start growing it until it covers everything
> > (unlikely) and make sure people don't break what's already passed.
> >
> > Of course I don't expect everybody to run this new make target with
> > SANITIZE=leak. Travis can do that for us and hopefully have some way
> > to tell git@vger about new breakages.
> 
> Makes sense to try to make sure that we don't regress leak-free tests. I
> don't know what our Travis-budget looks like, but I would volunteer to
> run something like this periodically using my own cycles.
> 
> My experience with the innards of our Makefiles and test-lib.sh is
> non-existant, but from a very cursory look it seems like something as
> simple as loading GIT_SKIP_TESTS from a blacklist-file might do the
> trick.

Yeah my first throught was something along that line too. But maybe
it's nicer to mark a test file leak-free like this:

-- 8< --
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 459f676683..1446f075b7 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -2,6 +2,8 @@
 
 test_description='test git worktree move, remove, lock and unlock'
 
+test_leak_free=yes
+
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 8< --

And we can run these test files with a new option --leak-check (or
something like that, we already support a similar option --valgrind).

Most of the work will be in test-lib.sh. If we detect --leak-check and
test_leak_free is not set, we skip the whole file. In the far far far
future when all files have test_leak_free=yes, we can flip the default
and delete these lines.

It would be nice if test-lib.sh can determine if 'git' binary is built
with SANITIZE=yes, but I think we can live without it.

> I could try to look into it in the next couple of days.

Have fun :) Let me know if you give up though, I'll give it a shot.
--
Duy

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Mauro Carvalho Chehab]

Em Mon, 12 Feb 2018 15:42:44 -0800
Junio C Hamano  escreveu:

> Linus Torvalds  writes:
> 
> > And some maintainers end up using multiple repositories as branches
> > (the old _original_ git model). Again, you can just use "git fetch +
> > git reset", of course, but that's a bit unsafe. In contrast, doing
> > "git pull --ff-only" is a safe convenient operation that does both the
> > fetch and the update to whatever state.
> >
> > But you do need that "--ff-only" to avoid the merge.  
> 
> OK.  I guess it is legit (and semi-sensible) for downstream
> contributors to "git pull --ff-only $upstream $release_tag_X" to
> bring their long-running topic currently based on release X-1 up to
> date with respect to release X.  It probably makes more sense than
> rebasing on top of release X, even though it makes a lot less sense
> than merging their topics into release X.
> 
> As you said, pull of a tag that forbids fast-forward by default is
> rather old development (I am kind of surprised that it was so old,
> in v1.7.9), so it may be a bit difficult to transition.
> 
> There is 
> 
>   [pull]
> ff = only
> 
> but pull.ff is quite global, and not good for intermediate level
> maintainers who pull to integrate work of their downstream (for
> which they do want the current "do not ff, record the tag in a merge
> commit" behaviour) and also pull to catch up from their upstream
> (which they want "ff-when-able").  They need to control between
> ff=only and ff=when-able, depending on whom they are pulling from.

Yes, that's my pain. I don't want ff only when pulling from others,
only when pulling from upstream tree.

> 
> We may want per-remote equivalent for it, i.e. e.g.
> 
>   [pull]
>   ff=false ;# good default for collecting contributions
> 
>   [remote "torvalds"] 
>   pullFF = only ;# good default for catching up
> 
> or something like that, perhaps?


Yeah, something like that works. Please notice, however, that what I
usually do is:

$ git remote update torvalds
$ git merge 
  (or git pull . )

So, for the above to work, it should store somehow the remote from
where a tag came from.




The reason is that I keep locally a cache with several tree clones
(in bare mode) s that I bother enough to cache (linus, -stable, -next),
as pulling from BR is time consuming, and I want to do it only once
and use the same "cache" for all my git clones.

I have a few git workdirs for my upstream work, but, as a patch
developer, I also have "independent"[1] git repositories.

[1] Due to disk constraints, the clones actually use --shared. So,
the common objects are actually stored inside a single tree.

Thanks,
Mauro

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Junio C Hamano]

Linus Torvalds  writes:

> And some maintainers end up using multiple repositories as branches
> (the old _original_ git model). Again, you can just use "git fetch +
> git reset", of course, but that's a bit unsafe. In contrast, doing
> "git pull --ff-only" is a safe convenient operation that does both the
> fetch and the update to whatever state.
>
> But you do need that "--ff-only" to avoid the merge.

OK.  I guess it is legit (and semi-sensible) for downstream
contributors to "git pull --ff-only $upstream $release_tag_X" to
bring their long-running topic currently based on release X-1 up to
date with respect to release X.  It probably makes more sense than
rebasing on top of release X, even though it makes a lot less sense
than merging their topics into release X.

As you said, pull of a tag that forbids fast-forward by default is
rather old development (I am kind of surprised that it was so old,
in v1.7.9), so it may be a bit difficult to transition.

There is 

[pull]
ff = only

but pull.ff is quite global, and not good for intermediate level
maintainers who pull to integrate work of their downstream (for
which they do want the current "do not ff, record the tag in a merge
commit" behaviour) and also pull to catch up from their upstream
(which they want "ff-when-able").  They need to control between
ff=only and ff=when-able, depending on whom they are pulling from.

We may want per-remote equivalent for it, i.e. e.g.

[pull]
ff=false ;# good default for collecting contributions

[remote "torvalds"] 
pullFF = only ;# good default for catching up

or something like that, perhaps?

Re: [PATCH 1/3] t7006: add tests for how git config paginates

[Martin Ågren]

On 12 February 2018 at 23:17, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> +test_expect_success TTY 'git config respects pager.config when setting' '
>> + rm -f paginated.out &&
>> + test_terminal git -c pager.config config foo.bar bar &&
>> + test -e paginated.out
>> +'
>
> I am debating myself if this test should instead spell out what we
> eventually want from the above test and make it expect_failure, just
> like the next one.

That's a valid point. I was coming at it from the point of view of "the
current behavior is well-defined and non-broken -- we'll soon change it,
but that's more a redefinition, not a bugfix (as with --edit)". But I
could go either way.

There is some prior art in ma/branch-list-paginate, where I handled `git
branch --set-upstream-to` similar to here, cf. d74b541e0b (branch:
respect `pager.branch` in list-mode only, 2017-11-19).

> In addition to setting (which will start ignoring pager in later
> steps), unsetting, replacing of a variable and renaming/removing a
> section in a configuration should not page, I would suspect.  Should
> we test them all?

I actually had several more tests in an early draft, including --unset.
Similarly, testing all the --get-* would be possible but feels like
overkill.  From the implementation, it's "obvious enough" (famous last
words) that there are two classes of arguments, and by testing a few
from each class we should be home free.

This now comes to `git config` after `git tag` and `git branch`, where
the "complexity" of the problem has been steadily increasing. (Off the
top of my head, tag has two modes, branch has three, config has lots.)
That the tests don't get more complex might be bad, or good. But all of
these use the same basic API (DELAY_PAGER_CONFIG) in the same rather
simple way. I actually almost had the feeling that these tests here were
too much, considering that DELAY_PAGER_CONFIG gets tested quite a few
times by now.

Thanks for your comments. I'll ponder this, and see what I come up with.
Maybe a changed approach, or maybe an extended commit message. Any
further input welcome, as always.

Martin

Re: [PATCH 1/3] t7006: add tests for how git config paginates

[Junio C Hamano]

Martin Ågren  writes:

> +test_expect_success TTY 'git config respects pager.config when setting' '
> + rm -f paginated.out &&
> + test_terminal git -c pager.config config foo.bar bar &&
> + test -e paginated.out
> +'

I am debating myself if this test should instead spell out what we
eventually want from the above test and make it expect_failure, just
like the next one.

In addition to setting (which will start ignoring pager in later
steps), unsetting, replacing of a variable and renaming/removing a
section in a configuration should not page, I would suspect.  Should
we test them all?

> +test_expect_failure TTY 'git config --edit ignores pager.config' '
> + rm -f paginated.out editor.used &&
> + write_script editor <<-\EOF &&
> + touch editor.used
> + EOF
> + EDITOR=./editor test_terminal git -c pager.config config --edit &&
> + ! test -e paginated.out &&
> + test -e editor.used
> +'
> +
> +test_expect_success TTY 'git config --get defaults to not paging' '
> + rm -f paginated.out &&
> + test_terminal git config --get foo.bar &&
> + ! test -e paginated.out
> +'
> +
> +test_expect_success TTY 'git config --get respects pager.config' '
> + rm -f paginated.out &&
> + test_terminal git -c pager.config config --get foo.bar &&
> + test -e paginated.out
> +'
> +
> +test_expect_success TTY 'git config --list defaults to not paging' '
> + rm -f paginated.out &&
> + test_terminal git config --list &&
> + ! test -e paginated.out
> +'
> +
> +
>  # A colored commit log will begin with an appropriate ANSI escape
>  # for the first color; the text "commit" comes later.
>  colorful() {

Re: [PATCH 3/7] worktree move: new command

[Martin Ågren]

On 12 February 2018 at 10:56, Duy Nguyen  wrote:
> On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren  wrote:
>> On 6 February 2018 at 03:13, Jeff King  wrote:
>>> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:
 I learned SANITIZE=leak today! It not only catches this but also "dst".

 Jeff is there any ongoing effort to make the test suite pass with
 SANITIZE=leak? My t2038 passed, so I went ahead with the full test
 suite and saw so many failures. I did see in your original mails that
 you focused on t and t0001 only..
>>>
>>> Yeah, I did those two scripts to try to prove to myself that the
>>> approach was good. But I haven't really pushed it any further.
>>>
>>> Martin Ågren (cc'd) did some follow-up work, but I think we still have a
>>> long way to go.
>>
>> Agreed. :-)
>
> Should we mark the test files that pass SANITIZE=leak and run as a sub
> testsuite? This way we can start growing it until it covers everything
> (unlikely) and make sure people don't break what's already passed.
>
> Of course I don't expect everybody to run this new make target with
> SANITIZE=leak. Travis can do that for us and hopefully have some way
> to tell git@vger about new breakages.

Makes sense to try to make sure that we don't regress leak-free tests. I
don't know what our Travis-budget looks like, but I would volunteer to
run something like this periodically using my own cycles.

My experience with the innards of our Makefiles and test-lib.sh is
non-existant, but from a very cursory look it seems like something as
simple as loading GIT_SKIP_TESTS from a blacklist-file might do the
trick. I could try to look into it in the next couple of days.

Martin

Re: [PATCH] color.h: document and modernize header

[Eric Sunshine]

On Mon, Feb 12, 2018 at 3:19 PM, Stefan Beller  wrote:
> Add documentation explaining the functions in color.h.
> While at it, mark them extern and migrate the function `color_set`
> into grep.c, where the only callers are.

This re-roll no longer marks functions as 'extern', so the commit
message saying that it does seems rather odd.

> Signed-off-by: Stefan Beller 
> ---
> diff --git a/color.h b/color.h
> @@ -76,22 +76,47 @@ int git_color_config(const char *var, const char *value, 
> void *cb);
> +/*
> + * Return a boolean whether to use color,
> + * resolving ambigious settings such as GIT_COLOR_AUTO, which is returned
> + * by git_config_colorbool().
> + */
>  int want_color(int var);

I'm probably being dense, but (if I hadn't had Peff's explanation[1]
to fall back on), based upon the comment block alone, I'd still have
no idea what I'm supposed to pass in as 'var'. Partly this is due to
the comment block not mentioning 'var' explicitly; it talks about
GIT_COLOR_AUTO and git_config_colorbool() abstractly, and, as a
reader, I can't tell if those are supposed to be passed in as 'var' or
if the function somehow grabs them out of the environment. Partly it's
due to the name "var" not conveying any useful meaning. Perhaps take
Peff's hint and state explicitly that the passed-in value is one of
GIT_COLOR_UNKNOWN, GIT_COLOR_NEVER, GIT_COLOR_ALWAYS, GIT_COLOR_AUTO,
then further explain that that value comes from
git_config_colorbool(), possibly modified by local option processing,
such as --color.

[1]: https://public-inbox.org/git/20180208222851.ga8...@sigill.intra.peff.net/

> +/*
> + * Output the formatted string in the specified color (and then reset to 
> normal
> + * color so subsequent output is uncolored). Omits the color encapsulation if
> + * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting
> + * the color. The `color_print_strbuf` prints the given pre-formatted strbuf
> + * instead, up to its first NUL character.
> + */

Perhaps prefix the last sentence with "BUG:" since we don't want
people relying on this NUL bug/misfeature.

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Linus Torvalds]

On Mon, Feb 12, 2018 at 1:44 PM, Junio C Hamano  wrote:
>
> But I wonder why "update to upstream" is merging a signed tag in the
> first place.  Wouldn't downstream's "try to keep up with" pull be
> grabbing from branch tips, not tags?

I'm actually encouraging maintainers to *not* start their work on some
random "kernel of the day".

Particularly during the kernel merge window, the upstream master
branch can be pretty flaky. It's *not* a good point to start new
development on, so if you're a maintainer, you really don't want to
use that as the basis for your work for the next merge window.

So I encourage people to use a stable point for new development, and
particularly actual release kernels. The natural way to do that is
obviously just to create a new branch:

   git checkout -b topicbranch v4.15

and now you have a good new clean branch for your development.

But clearly we've got a few people who have gotten used to the whole
"git pull" convenience of both fetching and updating.

Some maintainers don't even use topic branches, because their main
work is just merging work by subdevelepoers (that goes for my own
tree: I use topic branches for merging patch queues and to
occasionally track my own experimental patches, but 99% of the time
I'm just on "master" and obviously pull other peoples branches).

And some maintainers end up using multiple repositories as branches
(the old _original_ git model). Again, you can just use "git fetch +
git reset", of course, but that's a bit unsafe. In contrast, doing
"git pull --ff-only" is a safe convenient operation that does both the
fetch and the update to whatever state.

But you do need that "--ff-only" to avoid the merge.

  Linus

Re: [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees

[Eric Sunshine]

On Mon, Feb 12, 2018 at 3:58 PM, Lars Schneider
 wrote:
>> On 12 Feb 2018, at 04:15, Eric Sunshine  wrote:
>> +int run_hook_ve(const char *const *env, const char *name, va_list args)
>> +{
>> + return run_hook_cd_ve(NULL, env, name, args);
>> +}
>
> I think we have only one more user for this function:
> builtin/commit.c:   ret = run_hook_ve(hook_env.argv,name, args);
>
> Would it be an option to just use the new function signature
> everywhere and remove the wrapper? Or do we value the old interface?

I did note that there was only one existing caller and considered
simply modifying run_hook_ve()'s signature to accept the new 'dir'
argument. I don't feel strongly one way or the other.

However, as mentioned elsewhere[1], I'm still not convinced that this
one-off case of needing to chdir() warrants adding these overloads to
'run-command' at all, so I'm strongly considering (and was considering
even for v1) instead just running this hook manually in
builtin/worktree.c.

[1]: 
https://public-inbox.org/git/CAPig+cQ6Tq3J=bS8ymDqiXqUvoUiP59T=FGZgMw2FOAx0vyo=q...@mail.gmail.com/

Re: [PATCH 003/194] object-store: move packed_git and packed_git_mru to object store

[Junio C Hamano]

René Scharfe  writes:

> Am 12.02.2018 um 22:04 schrieb Junio C Hamano:
>> Stefan Beller  writes:
>> 
>>> I thought it may be a helpful
>>> for merging this series with the rest of the evolved code base which
>>> may make use of one of the converted functions. So instead of fixing
>>> that new instance manually, cocinelle could do that instead.
>> 
>> Having the .cocci used for the conversion *somewhere* would indeed
>> be helpful, as it allows me to (1) try reproducing this patch by
>> somebody else using the file and following the steps in order to
>> audit this patch and (2) catch new places that need to be migrated
>> in in-flight topics.
>> 
>> But placing it in contrib/coccinelle/ has other side effects.
>
> Running "make coccicheck" takes longer.  What other downsides are
> there?

Once the global variable packed_git has been migrated out of
existence, no new code that relies on it would be referring to that
global variable.  If coccicheck finds something, the suggested rewrite 
would be turning an unrelated packed_git (which may not even be the
right type) to a reference to a field in a global variable, that
would certainly be wrong.

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Junio C Hamano]

Linus Torvalds  writes:

> Maybe we could just tell people to have something like
>
>git config --global alias.update pull --ff-only
>
> and use that for "try to update to upstream".

I guess our mails crossed.  I admit that I indeed wondered why you
were not giving your usual "downstream shouldn't do pointless pull
from upstream" briefly but focused too much on how to tweak the
default without thinking through.

But I wonder why "update to upstream" is merging a signed tag in the
first place.  Wouldn't downstream's "try to keep up with" pull be
grabbing from branch tips, not tags?

Re: [PATCH 003/194] object-store: move packed_git and packed_git_mru to object store

[René Scharfe]

[removed rene.scha...@lsrfire.ath.cx from cc:; I lost that domain a few
 years ago.  Thanks for the heads-up, Stefan!]

Am 12.02.2018 um 20:00 schrieb Stefan Beller:
> On Fri, Feb 9, 2018 at 2:09 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> Patch generated by
>>>
>>>   2. Applying the semantic patch contrib/coccinelle/packed_git.cocci
>>>  to adjust callers.
>>
>> About this part...
>>
>>> diff --git a/contrib/coccinelle/packed_git.cocci 
>>> b/contrib/coccinelle/packed_git.cocci
>>> new file mode 100644
>>> index 00..da317a51a9
>>> --- /dev/null
>>> +++ b/contrib/coccinelle/packed_git.cocci
>>> @@ -0,0 +1,7 @@
>>> +@@ @@
>>> +- packed_git
>>> ++ the_repository->objects.packed_git
>>> +
>>> +@@ @@
>>> +- packed_git_mru
>>> ++ the_repository->objects.packed_git_mru
>>
>> The above is correct for one-time transition turning pre-transition
>> code to post the_repository world, but I am not sure if we want to
>> have it in contrib/coccinelle/, where "make coccicheck" looks at, as
>> a way to continuously keep an eye on "style" violations like using
>> strbuf_addf() for a constant when strbuf_addstr() suffices.
>>
>> Wouldn't we need a mechanism to ensure that this file will *not* be
>> used in "make coccicheck" somehow?

object_id.cocci is similar in this regard -- once the conversion is
done, we won't need it anymore.

> I can omit the cocci files from the patches, if that is better for 
> maintenance.
> 
> I thought it may be a helpful
> for merging this series with the rest of the evolved code base which
> may make use of one of the converted functions. So instead of fixing
> that new instance manually, cocinelle could do that instead.

Right, merging should be easier -- instead of fixing conflicts manually,
Coccinelle could regenerate the patch.

René

Re: [PATCH 003/194] object-store: move packed_git and packed_git_mru to object store

[René Scharfe]

Am 12.02.2018 um 22:04 schrieb Junio C Hamano:
> Stefan Beller  writes:
> 
>> I thought it may be a helpful
>> for merging this series with the rest of the evolved code base which
>> may make use of one of the converted functions. So instead of fixing
>> that new instance manually, cocinelle could do that instead.
> 
> Having the .cocci used for the conversion *somewhere* would indeed
> be helpful, as it allows me to (1) try reproducing this patch by
> somebody else using the file and following the steps in order to
> audit this patch and (2) catch new places that need to be migrated
> in in-flight topics.
> 
> But placing it in contrib/coccinelle/ has other side effects.

Running "make coccicheck" takes longer.  What other downsides are
there?

> I can think of two precedents in this project, namely:
> 
>   - fixup-builtins in 36e5e70e ("Start deprecating "git-command" in
> favor of "git command"", 2007-06-30)
> 
>   - convert-cache in d98b46f8 ("Do SHA1 hash _before_ compression.",
> 2005-04-20)
> 
> that are about tools that is useful during a transition period but
> can and should be removed after transition is over.  These two were
> done as one-off and added at the top-level, but perhaps we want a
> new directory at the top (e.g. devtools/) to add things like this
> and hold them while they are relevant?

Semantic patches for completed transformations can be removed as
well (or archived, e.g. by renaming to .cocci.done or so).

René

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Linus Torvalds]

On Mon, Feb 12, 2018 at 1:15 PM, Linus Torvalds
 wrote:
>
> The reasoning is to avoid losing the signature from the tag (when
> merging a signed tag, the signature gets inserted into the merge
> commit itself - use "git log --show-signature" to see them).

I think the commit that actually introduced the behavior was

fab47d057: merge: force edit and no-ff mode when merging a tag object

back in 2011, so we've had this behavior for a long time. So it's
probably not be worth tweaking the behavior any more, and maybe we
need to educate people to not update to other peoples state with "git
pull".

Maybe we could just tell people to have something like

   git config --global alias.update pull --ff-only

and use that for "try to update to upstream".

   Linus

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Junio C Hamano]

Linus Torvalds  writes:

> On Mon, Feb 12, 2018 at 1:00 PM, Stephen Rothwell  
> wrote:
>
> The problem, of course, is that since git is distributed, git doesn't
> know who is "upstream" and who is "downstream", so there's no
> _technical_ difference between merging a development tree, and a
> development tree doing a back-merge of the upstream tree.
>
> Maybe it was a mistake to make signed tag merges non-fast-forward,
> since they cause these kinds of issues with people who use "pull" to
> update their otherwise unmodified trees.
>
> I can always teach myself to just use --no-ff, since I end up doing
> things like verifying at the signatures anyway.
>
> Junio, comments?

I have a slight suspicion that allowing 'pull' to fast-forward even
when merging a signed tag when it is pulling from a configured
default remote for the branch the user is on, and otherwise keeping
the current behaviour, would make majority of people from both camps
happier, but I also have a strong conviction that it is being too
clever and making it hard to explain to people to do such a dwim
that tries to guess which way is 'upstream'.

Another clue we _might_ be able to take advantage of is that when
upstream maintainers merge a signed tag, we do *not* fetch and store
the tag from downstream contributers in our local repository (it is
likely that we have --no-tags in remote..tagopt), but when
downstream contributers sync from us with "git pull", they do fetch
and store our tags in their local repository.

So "git pull $somewhere $tag" that defaults to "--ff" when the tag
gets stored somewhere in refs/ (or more explicitly, in refs/tags/)
and defaults to "--no-ff" otherwise (i.e. the tag is fetched only to
be recorded in the resulting merge, without ever stored in any of
our refs), might be a good balance.  

And it is easy to explain: "We realize that it was a mistake to
unconditionally default to --no-ff and we are reverting the default
to --ff, but with a twist.  When we tell 'pull' to grab a tag, if
we do not store it anywhere in our local ref space, that would mean
the tag is totally lost if the pull fast-forwards.  That is why we
still use --no-ff in such a case."

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Mauro Carvalho Chehab]

Em Mon, 12 Feb 2018 13:15:04 -0800
Linus Torvalds  escreveu:

> On Mon, Feb 12, 2018 at 1:00 PM, Stephen Rothwell  
> wrote:
> >
> > Linus, this happens a bit after the merge window, so I am wondering
> > about the rational of not doing a fast forward merge when merging a
> > signed tag (I forget the reasoning).  
> 
> The reasoning is to avoid losing the signature from the tag (when
> merging a signed tag, the signature gets inserted into the merge
> commit itself - use "git log --show-signature" to see them).
> 
> So when I merge a signed tag, I do *not* want to fast-forward to the
> top commit, because then I'd lose the signature from the tag. Thus the
> "merging signed tags are non-fast-forward by default" reasoning.
> 
> But, yes, that reasoning is really only valid for proper merges of new
> features, not for back-merges.
> 
> The problem, of course, is that since git is distributed, git doesn't
> know who is "upstream" and who is "downstream", so there's no
> _technical_ difference between merging a development tree, and a
> development tree doing a back-merge of the upstream tree.
> 
> Maybe it was a mistake to make signed tag merges non-fast-forward,
> since they cause these kinds of issues with people who use "pull" to
> update their otherwise unmodified trees.
> 
> I can always teach myself to just use --no-ff, since I end up doing
> things like verifying at the signatures anyway.

Hmm... at least at git version 2.14.3, git documentation doesn't
mention that signed pull requests won't do fast forward. Instead,
it says that --ff is the default behavior:


   --ff
   When the merge resolves as a fast-forward, only update the branch 
pointer, without creating a merge commit. This is the
   default behavior.

Btw, even doing:

$ git merge -ff v4.16-rc1

it will still produce a git commit for the merge.


-- 
Thanks,
Mauro

Re: [PATCH v3 07/14] commit-graph: update graph-head during write

[Derrick Stolee]

On 2/12/2018 3:37 PM, Junio C Hamano wrote:

Junio C Hamano  writes:


Derrick Stolee  writes:


It is possible to have multiple commit graph files in a pack directory,
but only one is important at a time. Use a 'graph_head' file to point
to the important file. Teach git-commit-graph to write 'graph_head' upon
writing a new commit graph file.

Why this design, instead of what "repack -a" would do, iow, if there
always is a singleton that is the only one that matters, shouldn't
the creation of that latest singleton just clear the older ones
before it returns control?

Note that I am not complaining---I am just curious why we want to
expose this "there is one relevant one but we keep irrelevant ones
we usually do not look at and need to be garbage collected" to end
users, and also expect readers of the series, resulting code and
docs would have the same puzzled feeling.



Aside: I forgot to mention in my cover letter that the experience around 
the "--delete-expired" flag for "git commit-graph write" is different 
than v2. If specified, we delete all ".graph" files in the pack 
directory other than the one referenced by "graph_head" at the beginning 
of the process or the one written by the process. If these deletes fail, 
then we ignore the failure (assuming that they are being used by another 
Git process). In usual cases, we will delete these expired files in the 
next instance. I believe this matches similar behavior in gc and repack.


-- Back to discussion about the value of "graph_head" --

The current design of using a pointer file (graph_head) is intended to 
have these benefits:


1. We do not need to rely on a directory listing and mtimes to determine 
which graph file to use.


2. If we write a new graph file while another git process is reading the 
existing graph file, we can update the graph_head pointer without 
deleting the file that is currently memory-mapped. (This is why we 
cannot just rely on a canonical file name, such as "the_graph", to store 
the data.)


3. We can atomically change the 'graph_head' file without interrupting 
concurrent git processes. I think this is different from the "repack" 
situation because a concurrent process would load all packfiles in the 
pack directory and possibly have open handles when the repack is trying 
to delete them.


4. We remain open to making the graph file incremental (as the MIDX 
feature is designed to do; see [1]). It is less crucial to have an 
incremental graph file structure (the graph file for the Windows 
repository is currently ~120MB versus a MIDX file of 1.25 GB), but the 
graph_head pattern makes this a possibility.


I tried to avoid item 1 due to personal taste, and since I am storing 
the files in the objects/pack directory (so that listing may be very 
large with a lot of wasted entries). This is less important with our 
pending change of moving the graph files to a different directory. This 
also satisfies items 2 and 3, as long as we never write graph files so 
quickly that we have a collision on mtime.


I cannot think of another design that satisfies item 4.

As for your end user concerns: My philosophy with this feature is that 
end users will never interact with the commit-graph builtin. 99% of 
users will benefit from a repack or GC automatically computing a commit 
graph (when we add that integration point). The other uses for the 
builtin are for users who want extreme control over their data, such as 
code servers and build agents.


Perhaps someone with experience managing large repositories with git in 
a server environment could chime in with some design requirements they 
would need.


Thanks,
-Stolee

[1] 
https://public-inbox.org/git/20180107181459.222909-2-dsto...@microsoft.com/

    [RFC PATCH 01/18] docs: Multi-Pack Index (MIDX) Design Notes

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Linus Torvalds]

On Mon, Feb 12, 2018 at 1:00 PM, Stephen Rothwell  wrote:
>
> Linus, this happens a bit after the merge window, so I am wondering
> about the rational of not doing a fast forward merge when merging a
> signed tag (I forget the reasoning).

The reasoning is to avoid losing the signature from the tag (when
merging a signed tag, the signature gets inserted into the merge
commit itself - use "git log --show-signature" to see them).

So when I merge a signed tag, I do *not* want to fast-forward to the
top commit, because then I'd lose the signature from the tag. Thus the
"merging signed tags are non-fast-forward by default" reasoning.

But, yes, that reasoning is really only valid for proper merges of new
features, not for back-merges.

The problem, of course, is that since git is distributed, git doesn't
know who is "upstream" and who is "downstream", so there's no
_technical_ difference between merging a development tree, and a
development tree doing a back-merge of the upstream tree.

Maybe it was a mistake to make signed tag merges non-fast-forward,
since they cause these kinds of issues with people who use "pull" to
update their otherwise unmodified trees.

I can always teach myself to just use --no-ff, since I end up doing
things like verifying at the signatures anyway.

Junio, comments?

   Linus

Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep

[Junio C Hamano]

Junio C Hamano  writes:

>> -test_i18ngrep -E '^(fatal|warning):' actual 
>> | sort &&
>> +grep -E '^(fatal|warning):' actual &&
>>  test_i18ncmp expected actual
>
> OK, but not quite OK.
>
> Two grep invocations will not leave anything useful in 'actual'
> under poison build, and is almost guaranteed that 'expected' would
> not match, but that is perfectly OK because the final comparison is
> done.

Sorry.  s/is done./is done with i18ncmp./ is what I wanted to say.

Re: [PATCH 003/194] object-store: move packed_git and packed_git_mru to object store

[Junio C Hamano]

Stefan Beller  writes:

> I thought it may be a helpful
> for merging this series with the rest of the evolved code base which
> may make use of one of the converted functions. So instead of fixing
> that new instance manually, cocinelle could do that instead.

Having the .cocci used for the conversion *somewhere* would indeed
be helpful, as it allows me to (1) try reproducing this patch by
somebody else using the file and following the steps in order to
audit this patch and (2) catch new places that need to be migrated
in in-flight topics.

But placing it in contrib/coccinelle/ has other side effects.

I can think of two precedents in this project, namely:

 - fixup-builtins in 36e5e70e ("Start deprecating "git-command" in
   favor of "git command"", 2007-06-30)

 - convert-cache in d98b46f8 ("Do SHA1 hash _before_ compression.",
   2005-04-20)

that are about tools that is useful during a transition period but
can and should be removed after transition is over.  These two were
done as one-off and added at the top-level, but perhaps we want a
new directory at the top (e.g. devtools/) to add things like this
and hold them while they are relevant?

Re: [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees

[Lars Schneider]

> On 12 Feb 2018, at 04:15, Eric Sunshine  wrote:
> 
> Git commands which run hooks do so at the top level of the worktree in
> which the command itself was invoked. However, the 'git worktree'
> command may need to run hooks within some other directory. For
> instance, when "git worktree add" runs the 'post-checkout' hook, the
> hook must be run within the newly-created worktree, not within the
> worktree from which "git worktree add" was invoked.
> 
> To support this case, add 'run-hook' overloads which allow the
> worktree directory to be specified.
> 
> Signed-off-by: Eric Sunshine 
> ---
> run-command.c | 23 +--
> run-command.h |  4 
> 2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/run-command.c b/run-command.c
> index 31fc5ea86e..0e3995bbf9 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1197,7 +1197,8 @@ const char *find_hook(const char *name)
>   return path.buf;
> }
> 
> -int run_hook_ve(const char *const *env, const char *name, va_list args)
> +int run_hook_cd_ve(const char *dir, const char *const *env, const char *name,
> +va_list args)
> {
>   struct child_process hook = CHILD_PROCESS_INIT;
>   const char *p;
> @@ -1206,9 +1207,10 @@ int run_hook_ve(const char *const *env, const char 
> *name, va_list args)
>   if (!p)
>   return 0;
> 
> - argv_array_push(, p);
> + argv_array_push(, absolute_path(p));
>   while ((p = va_arg(args, const char *)))
>   argv_array_push(, p);
> + hook.dir = dir;
>   hook.env = env;
>   hook.no_stdin = 1;
>   hook.stdout_to_stderr = 1;
> @@ -1216,6 +1218,23 @@ int run_hook_ve(const char *const *env, const char 
> *name, va_list args)
>   return run_command();
> }
> 
> +int run_hook_ve(const char *const *env, const char *name, va_list args)
> +{
> + return run_hook_cd_ve(NULL, env, name, args);
> +}

I think we have only one more user for this function:
builtin/commit.c:   ret = run_hook_ve(hook_env.argv,name, args);

The other function 'run_hook_le' is used in a few places:
builtin/am.c:   ret = run_hook_le(NULL, "applypatch-msg", 
am_path(state, "final-commit"), NULL);
builtin/am.c:   if (run_hook_le(NULL, "pre-applypatch", NULL))
builtin/am.c:   run_hook_le(NULL, "post-applypatch", NULL);
builtin/checkout.c: return run_hook_le(NULL, "post-checkout",
builtin/clone.c:err |= run_hook_le(NULL, "post-checkout", 
sha1_to_hex(null_sha1),
builtin/gc.c:   if (run_hook_le(NULL, "pre-auto-gc", NULL))
builtin/merge.c:run_hook_le(NULL, "post-merge", squash ? "1" : 
"0", NULL);
builtin/receive-pack.c: if (run_hook_le(env->argv, 
push_to_checkout_hook,

Would it be an option to just use the new function signature
everywhere and remove the wrapper? Or do we value the old interface?

- Lars



> +
> +int run_hook_cd_le(const char *dir, const char *const *env, const char 
> *name, ...)
> +{
> + va_list args;
> + int ret;
> +
> + va_start(args, name);
> + ret = run_hook_cd_ve(dir, env, name, args);
> + va_end(args);
> +
> + return ret;
> +}
> +
> int run_hook_le(const char *const *env, const char *name, ...)
> {
>   va_list args;
> diff --git a/run-command.h b/run-command.h
> index 3932420ec8..8beddffea8 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -66,7 +66,11 @@ int run_command(struct child_process *);
> extern const char *find_hook(const char *name);
> LAST_ARG_MUST_BE_NULL
> extern int run_hook_le(const char *const *env, const char *name, ...);
> +extern int run_hook_cd_le(const char *dir, const char *const *env,
> +   const char *name, ...);
> extern int run_hook_ve(const char *const *env, const char *name, va_list 
> args);
> +extern int run_hook_cd_ve(const char *dir, const char *const *env,
> +   const char *name, va_list args);
> 
> #define RUN_COMMAND_NO_STDIN 1
> #define RUN_GIT_CMD2  /*If this is to be git sub-command */
> -- 
> 2.16.1.291.g4437f3f132
> 

Re: [PATCH v3 04/12] sequencer: introduce new commands to reset the revision

[Johannes Schindelin]

Hi Eric,

On Mon, 12 Feb 2018, Eric Sunshine wrote:

> On Sat, Feb 10, 2018 at 7:10 PM, Johannes Schindelin
>  wrote:
> > [...]
> > This commit implements the commands to label, and to reset to, given
> > revisions. The syntax is:
> >
> > label 
> > reset 
> > [...]
> > Signed-off-by: Johannes Schindelin 
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -1922,6 +1951,151 @@ static int do_exec(const char *command_line)
> > +static int safe_append(const char *filename, const char *fmt, ...)
> > +{
> > +   [...]
> > +   if (write_in_full(fd, buf.buf, buf.len) < 0) {
> > +   error_errno(_("could not write to '%s'"), filename);
> > +   rollback_lock_file();
> 
> strbuf_release();
> 
> > +   return -1;
> > +   }
> > +   if (commit_lock_file() < 0) {
> > +   rollback_lock_file();
> 
> strbuf_release();
> 
> > +   return error(_("failed to finalize '%s'"), filename);
> > +   }
> > +
> 
> strbuf_release();
> 
> > +   return 0;
> > +}
> > +
> > +static int do_reset(const char *name, int len, struct replay_opts *opts)
> > +{
> > +   [...]
> > +   unpack_tree_opts.reset = 1;
> > +
> > +   if (read_cache_unmerged())
> 
> rollback_lock_file();
> strbuf_release(_name);

Thank you very much! I fixed these locally and force-pushed the
recreate-merges branch to https://github.com/dscho/git. These fixes will
be part of v4.

Ciao,
Dscho

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi Sergey,

On Mon, 12 Feb 2018, Sergey Organov wrote:

> Johannes Schindelin  writes:
> >
> > On Fri, 9 Feb 2018, Sergey Organov wrote:
> >
> >> Johannes Schindelin  writes:
> >> 
> >> [...]
> >> 
> >> > With this patch, the goodness of the Git garden shears comes to `git
> >> > rebase -i` itself. Passing the `--recreate-merges` option will generate
> >> > a todo list that can be understood readily, and where it is obvious
> >> > how to reorder commits. New branches can be introduced by inserting
> >> > `label` commands and calling `merge -  `. And once this
> >> > mode has become stable and universally accepted, we can deprecate the
> >> > design mistake that was `--preserve-merges`.
> >> 
> >> This doesn't explain why you introduced this new --recreate-merges. Why
> >> didn't you rather fix --preserve-merges to generate and use new todo
> >> list format?
> >
> > Because that would of course break existing users of
> > --preserve-merges.
> 
> How exactly?

Power users of interactive rebase use scripting to augment Git's
functionality. One particularly powerful trick is to override
GIT_SEQUENCER_EDITOR with an invocation of such a script, to perform
automated edits. Such a script breaks when we change the format of the
content to edit. If we change the format of the todo list generated in
--preserve-merges mode, that is exactly what happens. We break existing
users.

BTW it seems that you did not really read my previous reply carefully
because I referenced such a use case: the Git garden shears. They do
override the sequencer editor, and while they do not exactly edit the todo
list (they simply through the generated one away), they generate a new
todo list and would break if that format changes. Of course, the shears do
not use the --preserve-merges mode, but from just reading about the way
how the Git garden shears work, it is quite obvious how similar users of
--preserve-merges are likely to exist?

> Doesn't "--recreate-merges" produce the same result as
> "--preserve-merges" if run non-interactively?

The final result of a rebase where you do not edit the todo list? Should
be identical, indeed.

But that is the most boring, most uninteresting, and least important use
case. So we might just as well forget about it when we focus on keeping
Git's usage stable.

> > So why not --preserve-merges=v2? Because that would force me to
> > maintain --preserve-merges forever. And I don't want to.
> >
> >> It doesn't seem likely that todo list created by one Git version is
> >> to be ever used by another, right?
> >
> > No. But by scripts based on `git rebase -p`.
> >
> >> Is there some hidden reason here? Some tools outside of Git that use
> >> old todo list format, maybe?
> >
> > Exactly.
> >
> > I did mention such a tool: the Git garden shears:
> >
> > https://github.com/git-for-windows/build-extra/blob/master/shears.sh
> >
> > Have a look at it. It will inform the discussion.
> 
> I've searched for "-p" in the script, but didn't find positives for
> either "-p" or "--preserve-merges". How it would break if it doesn't use
> them? What am I missing?

*This* particular script does not use -p.

But it is not *this* particular script that I do not want to break! It is
*all* scripts that use interactive rebase! Don't you also care about not
breaking existing users?

> >> Then, if new option indeed required, please look at the resulting manual:
> >> 
> >> --recreate-merges::
> >>Recreate merge commits instead of flattening the history by replaying
> >>merges. Merge conflict resolutions or manual amendments to merge
> >>commits are not preserved.
> >> 
> >> -p::
> >> --preserve-merges::
> >>Recreate merge commits instead of flattening the history by replaying
> >>commits a merge commit introduces. Merge conflict resolutions or manual
> >>amendments to merge commits are not preserved.
> >
> > As I stated in the cover letter, there are more patches lined up after
> > this patch series.
> 
> Good, but I thought this one should better be self-consistent anyway.
> What if those that come later aren't included?

Right, let's just rip apart the partial progress because the latter
patches might not make it in?

I cannot work on that basis, and I also do not want to work on that basis.

If you do not like how the documentation is worded, fine, suggest a better
alternative.

> > Have a look at https://github.com/git/git/pull/447, especially the
> > latest commit in there which is an early version of the deprecation I
> > intend to bring about.
> 
> You shouldn't want a deprecation at all should you have re-used
> --preserve-merges in the first place, and I still don't see why you
> haven't. 

Keep repeating it, and it won't become truer.

If you break formats, you break scripts. Git has *so* many users, there
are very likely some who script *every* part of it.

We simply cannot do that.

What we can is deprecate designs which we learned on the 

Re: [PATCH v3 07/14] commit-graph: update graph-head during write

[Junio C Hamano]

Junio C Hamano  writes:

> Derrick Stolee  writes:
>
>> It is possible to have multiple commit graph files in a pack directory,
>> but only one is important at a time. Use a 'graph_head' file to point
>> to the important file. Teach git-commit-graph to write 'graph_head' upon
>> writing a new commit graph file.
>
> Why this design, instead of what "repack -a" would do, iow, if there
> always is a singleton that is the only one that matters, shouldn't
> the creation of that latest singleton just clear the older ones
> before it returns control?

Note that I am not complaining---I am just curious why we want to
expose this "there is one relevant one but we keep irrelevant ones
we usually do not look at and need to be garbage collected" to end
users, and also expect readers of the series, resulting code and
docs would have the same puzzled feeling.

Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook

[Eric Sunshine]

On Mon, Feb 12, 2018 at 2:37 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> Fix this by changing to the new worktree's directory before running
>> the hook, and adjust the tests to verify that the hook is indeed run
>> within the correct directory.
>
> I like the approach taken by this replacement better.  Just to make
> sure I understand the basic idea, let me rephrase what these two
> patches are doing:
>
>  - "path" that is made absolute in this step is where the new
>worktree is created, i.e. the top-level of the working tree in
>the new worktree.  We chdir there and then run the hook script.

Sorry for misleading. The "absolute path" stuff in this patch is
unnecessary; it's probably just left-over from Lars's proposal which
did need to make it absolute when setting GIT_WORK_TREE, and I likely
didn't think hard enough to realize that it doesn't need to be
absolute just for chdir(). I'll drop the unnecessary
absolute_pathdup() in the re-roll.

(The hook path in patch 1/2, on the other hand, does need to be made
absolute since find_hook() returns a relative path before we've
chdir()'d into the new worktree.)

>  - Even though we often see hooks executed inside .git/ directory,
>for post-checkout, the top-level of the working tree is the right
>place, as that is where the hook is run by "git checkout" [...]

Patch 1/2's commit message is a bit sloppy in its description of this.
I'll tighten it up in the re-roll.

I'm also not fully convinced that these new overloads of run_hook_*()
are warranted since it's hard to imagine any other case when they
would be useful. It may make sense just to have builtin/worktree.c run
the hook itself for this one-off case.

> I wonder if we need to clear existing GIT_DIR/GIT_WORK_TREE from the
> environment, though.  When a user with a funny configuration (where
> these two environment variables are pointing at unusual places) uses
> "git worktree add" to create another worktree for the repository, it
> would not be sufficient to chdir to defeat them that are appropriate
> for the original, and not for the new, worktree, would it?

Good point. I'll look into sanitizing the environment.

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Schindelin]

Hi Sergey,

On Mon, 12 Feb 2018, Sergey Organov wrote:

> Thanks for explanations, and could you please answer this one:
> 
> [...]
> 
> >> I also have trouble making sense of "Recreate merge commits instead of
> >> flattening the history by replaying merges." Is it " >> commits by replaying merges> instead of " or is it
> >> rather " instead of  >> replaying merges>?

I thought I had answered that one.

Flattening the history is what happens in regular rebase (i.e. without
--recreate-merges and without --preserve-merges).

The idea to recreate merges is of course to *not* flatten the history.

Maybe there should have been a comma after "history" to clarify what the
sentence means.

The wording is poor either way, but you are also not a native speaker so
we have to rely on, say, Eric to help us out here.

Ciao,
Johannes

[PATCH] color.h: document and modernize header

[Stefan Beller]

Add documentation explaining the functions in color.h.
While at it, mark them extern and migrate the function `color_set`
into grep.c, where the only callers are.

Signed-off-by: Stefan Beller 
---

* removed the extern keyword
* reworded the docs for want_color once again.

 color.c |  7 ---
 color.h | 35 ++-
 grep.c  |  5 +
 3 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/color.c b/color.c
index d48dd947c9..f277e72e4c 100644
--- a/color.c
+++ b/color.c
@@ -161,11 +161,6 @@ int color_parse(const char *value, char *dst)
return color_parse_mem(value, strlen(value), dst);
 }
 
-void color_set(char *dst, const char *color_bytes)
-{
-   xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
-}
-
 /*
  * Write the ANSI color codes for "c" to "out"; the string should
  * already have the ANSI escape code in it. "out" should have enough
@@ -399,8 +394,6 @@ static int color_vfprintf(FILE *fp, const char *color, 
const char *fmt,
return r;
 }
 
-
-
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
 {
va_list args;
diff --git a/color.h b/color.h
index fd2b688dfb..40a40f31a4 100644
--- a/color.h
+++ b/color.h
@@ -76,22 +76,47 @@ int git_color_config(const char *var, const char *value, 
void *cb);
 int git_color_default_config(const char *var, const char *value, void *cb);
 
 /*
- * Set the color buffer (which must be COLOR_MAXLEN bytes)
- * to the raw color bytes; this is useful for initializing
- * default color variables.
+ * Parse a config option, which can be a boolean or one of
+ * "never", "auto", "always". Return a constant of
+ * GIT_COLOR_NEVER for "never" or negative boolean,
+ * GIT_COLOR_ALWAYS for "always" or a positive boolean,
+ * and GIT_COLOR_AUTO for "auto".
  */
-void color_set(char *dst, const char *color_bytes);
-
 int git_config_colorbool(const char *var, const char *value);
+
+/*
+ * Return a boolean whether to use color,
+ * resolving ambigious settings such as GIT_COLOR_AUTO, which is returned
+ * by git_config_colorbool().
+ */
 int want_color(int var);
+
+/*
+ * Translate a Git color from 'value' into a string that the terminal can
+ * interpret and store it into 'dst'. The Git color values are of the form
+ * "foreground [background] [attr]" where fore- and background can be a color
+ * name ("red"), a RGB code (#0xFF) or a 256-color-mode from the terminal.
+ */
 int color_parse(const char *value, char *dst);
 int color_parse_mem(const char *value, int len, char *dst);
+
+/*
+ * Output the formatted string in the specified color (and then reset to normal
+ * color so subsequent output is uncolored). Omits the color encapsulation if
+ * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting
+ * the color. The `color_print_strbuf` prints the given pre-formatted strbuf
+ * instead, up to its first NUL character.
+ */
 __attribute__((format (printf, 3, 4)))
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
 int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
 void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
 
+/*
+ * Check if the given color is GIT_COLOR_NIL that means "no color selected".
+ * The caller needs to replace the color with the actual desired color.
+ */
 int color_is_nil(const char *color);
 
 #endif /* COLOR_H */
diff --git a/grep.c b/grep.c
index 3d7cd0e96f..834b8eb439 100644
--- a/grep.c
+++ b/grep.c
@@ -18,6 +18,11 @@ static void std_output(struct grep_opt *opt, const void 
*buf, size_t size)
fwrite(buf, size, 1, stdout);
 }
 
+static void color_set(char *dst, const char *color_bytes)
+{
+   xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
+}
+
 /*
  * Initialize the grep_defaults template with hardcoded defaults.
  * We could let the compiler do this, but without C99 initializers
-- 
2.16.1.73.ga2c3e9663f.dirty

Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep

[Junio C Hamano]

Ramsay Jones  writes:

> Attempting to grep the output of test_i18ngrep will not work under a
> poison build, since the output is (almost) guaranteed not to have the
> string you are looking for. In this case, the output of test_i18ngrep
> is further filtered by a simple piplined grep to exclude an '... remote
> end hung up unexpectedly' warning message. Use a regular 'grep -E' to
> replace the call to test_i18ngrep in the filter pipeline.
> Also, remove a useless invocation of 'sort' as the final element of the
> pipeline.
>
> Signed-off-by: Ramsay Jones 
> ---
>  t/t5536-fetch-conflicts.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh
> index 2e42cf331..38381df5e 100755
> --- a/t/t5536-fetch-conflicts.sh
> +++ b/t/t5536-fetch-conflicts.sh
> @@ -22,7 +22,7 @@ verify_stderr () {
>   cat >expected &&
>   # We're not interested in the error
>   # "fatal: The remote end hung up unexpectedly":
> - test_i18ngrep -E '^(fatal|warning):' actual 
> | sort &&
> + grep -E '^(fatal|warning):' actual &&
>   test_i18ncmp expected actual

OK, but not quite OK.

Two grep invocations will not leave anything useful in 'actual'
under poison build, and is almost guaranteed that 'expected' would
not match, but that is perfectly OK because the final comparison is
done.

However, it is brittle to rely on the latter "grep -v" to exit with
status 0 for the &&-chain to work.  The original was most likely
masked by the "| sort" exiting with 0 status all the time ;-)

There needs an explanation why this commit thinks the invocation of
"sort" useless.  I do not particularly think "suppressing a
not-found non-zero exit from 'grep'" is a useful thing, but are we
committed to show the two warnings seen in the last test in this
file to always in the shown order (i.e. the same order as the
refspecs are given to us)?  If not, then "sort" does serve a
purpose.  Note that I do not think we want to be able to reorder the
warning messages in future versions of Git for that last case, so
making that explicit may be a good justification.

The "sort" as the last step in the pipeline makes sure that we
do not have to guarantee the order in which we give the two
warning messages from the last test in this script, but
processing the refspecs in the same order as they are given on
the command line and warning against them as we discover
problems is a property we would rather want to keep, so drop it
to make sure that we would catch regression when we accidentally
change the order in which these messages are given.

or something like that, perhaps.

Re: [PATCH v3 05/12] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Eric,

On Mon, 12 Feb 2018, Eric Sunshine wrote:

> On Sat, Feb 10, 2018 at 7:10 PM, Johannes Schindelin
>  wrote:
> > This patch is part of the effort to reimplement `--preserve-merges` with
> > a substantially improved design, a design that has been developed in the
> > Git for Windows project to maintain the dozens of Windows-specific patch
> > series on top of upstream Git.
> >
> > The previous patch implemented the `label` and `reset` commands to label
> > commits and to reset to a labeled commits. This patch adds the `merge`
> 
> s/to a/to/

Fixed locally. Will be part of the next iteration, if one is necessary.
Otherwise I will first ask Junio whether he can touch up the commit
message before applying.

Ciao,
Dscho

[no subject]

[Elizabeth M. Philips]

Belove

My name is Elizabeth M. Philips, i am going on a radical hysterectomy
cervical cancer surgery today. I have WILLED £12,379,000.00 British pounds
to you for the work of the lord. Contact my attorney with my reference number
(NW/XXR/017/053K/PDQ/613X1/UK) for further info. Barr.Luis Jason.
email:luis347ja...@gmail.com

Sincerely Yours,
Mrs.Elizabeth M. Philips.

Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook

[Lars Schneider]

> On 12 Feb 2018, at 04:15, Eric Sunshine  wrote:
> 
> Although "git worktree add" learned to run the 'post-checkout' hook in
> ade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it
> neglects to change to the directory of the newly-created worktree
> before running the hook. Instead, the hook is run within the directory
> from which the "git worktree add" command itself was invoked, which
> effectively neuters the hook since it knows nothing about the new
> worktree directory.
> 
> Fix this by changing to the new worktree's directory before running
> the hook, and adjust the tests to verify that the hook is indeed run
> within the correct directory.
> 
> While at it, also add a test to verify that the hook is run within the
> correct directory even when the new worktree is created from a sibling
> worktree (as opposed to the main worktree).
> 
> Reported-by: Lars Schneider 
> Signed-off-by: Eric Sunshine 
> ---
> builtin/worktree.c  | 11 ---
> t/t2025-worktree-add.sh | 25 ++---
> 2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 7cef5b120b..b55c55a26c 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -345,9 +345,14 @@ static int add_worktree(const char *path, const char 
> *refname,
>* Hook failure does not warrant worktree deletion, so run hook after
>* is_junk is cleared, but do return appropriate code when hook fails.
>*/
> - if (!ret && opts->checkout)
> - ret = run_hook_le(NULL, "post-checkout", oid_to_hex(_oid),
> -   oid_to_hex(>object.oid), "1", NULL);
> + if (!ret && opts->checkout) {
> + char *p = absolute_pathdup(path);
> + ret = run_hook_cd_le(p, NULL, "post-checkout",
> +  oid_to_hex(_oid),
> +  oid_to_hex(>object.oid),
> +  "1", NULL);
> + free(p);
> + }
> 
>   argv_array_clear(_env);
>   strbuf_release();
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 2b95944973..cf0aaeaf88 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -454,20 +454,29 @@ post_checkout_hook () {
>   test_when_finished "rm -f .git/hooks/post-checkout" &&
>   mkdir -p .git/hooks &&
>   write_script .git/hooks/post-checkout <<-\EOF
> - echo $* >hook.actual
> + {
> + echo $*
> + git rev-parse --show-toplevel
> + } >../hook.actual
>   EOF
> }
> 
> test_expect_success '"add" invokes post-checkout hook (branch)' '
>   post_checkout_hook &&
> - printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect &&
> + {
> + echo $_z40 $(git rev-parse HEAD) 1 &&
> + echo $(pwd)/gumby
> + } >hook.expect &&
>   git worktree add gumby &&
>   test_cmp hook.expect hook.actual
> '
> 
> test_expect_success '"add" invokes post-checkout hook (detached)' '
>   post_checkout_hook &&
> - printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect &&
> + {
> + echo $_z40 $(git rev-parse HEAD) 1 &&
> + echo $(pwd)/grumpy
> + } >hook.expect &&
>   git worktree add --detach grumpy &&
>   test_cmp hook.expect hook.actual
> '
> @@ -479,4 +488,14 @@ test_expect_success '"add --no-checkout" suppresses 
> post-checkout hook' '
>   test_path_is_missing hook.actual
> '
> 
> +test_expect_success '"add" within worktree invokes post-checkout hook' '
> + post_checkout_hook &&
> + {
> + echo $_z40 $(git rev-parse HEAD) 1 &&
> + echo $(pwd)/guppy
> + } >hook.expect &&
> + git -C gloopy worktree add --detach ../guppy &&
> + test_cmp hook.expect hook.actual
> +'
> +
> test_done
> -- 
> 2.16.1.291.g4437f3f132
> 

Looks good but I think we are not quite there yet. It does not work
for bare repos. You can test this if you apply the following patch on
top of your changes. Or get the patch from here:
https://github.com/larsxschneider/git/commit/253130e65b37a2ef250e9c6369d292ec725e62e7.patch

Please note that also '"add" within worktree invokes post-checkout hook'
seems to fail with my extended test case.

Thanks,
Lars


diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index cf0aaeaf88..0580b12d50 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -451,20 +451,41 @@ test_expect_success 'git worktree --no-guess-remote 
option overrides config' '
 '
 
 post_checkout_hook () {
-   test_when_finished "rm -f .git/hooks/post-checkout" &&
-   mkdir -p .git/hooks &&
-   write_script .git/hooks/post-checkout <<-\EOF
+   test_when_finished "rm -f $1/hooks/post-checkout" &&
+   mkdir -p $1/hooks &&
+   write_script $1/hooks/post-checkout <<-\EOF
{

[PATCH] send-email: error out when relogin delay is missing

[Stefan Beller]

When the batch size is neither configured nor given on the command
line, but the relogin delay is given, then the current code ignores
the relogin delay setting.

This is unsafe as there was some intention when setting the batch size.
One workaround would be to just assume a batch size of 1 as a default.
This however may be bad UX, as then the user may wonder why it is sending
slowly without apparent batching.

Error out for now instead of potentially confusing the user.
As 5453b83bdf (send-email: --batch-size to work around some SMTP
server limit, 2017-05-21) lays out, we rather want to not have this
interface anyway and would rather want to react on the server throttling
dynamically.

Helped-by: Eric Sunshine 
Signed-off-by: Stefan Beller 
---
 git-send-email.perl | 4 
 1 file changed, 4 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 340b5c8482..f7913f7c2c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -379,6 +379,10 @@ unless ($rc) {
 die __("Cannot run git format-patch from outside a repository\n")
if $format_patch and not $repo;
 
+die __("`batch-size` and `relogin` must be specified together " .
+   "(via command-line or configuration option)\n")
+   if defined $relogin_delay and not defined $batch_size;
+
 # Now, let's fill any that aren't set in with defaults:
 
 sub read_config {
-- 
2.15.1.433.g936d1b9894.dirty

Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook

[Junio C Hamano]

Eric Sunshine  writes:

> Although "git worktree add" learned to run the 'post-checkout' hook in
> ade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it
> neglects to change to the directory of the newly-created worktree
> before running the hook. Instead, the hook is run within the directory
> from which the "git worktree add" command itself was invoked, which
> effectively neuters the hook since it knows nothing about the new
> worktree directory.
>
> Fix this by changing to the new worktree's directory before running
> the hook, and adjust the tests to verify that the hook is indeed run
> within the correct directory.

I like the approach taken by this replacement better.  Just to make
sure I understand the basic idea, let me rephrase what these two
patches are doing:

 - "path" that is made absolute in this step is where the new
   worktree is created, i.e. the top-level of the working tree in
   the new worktree.  We chdir there and then run the hook script.

 - Even though we often see hooks executed inside .git/ directory,
   for post-checkout, the top-level of the working tree is the right
   place, as that is where the hook is run by "git checkout" (which
   does the "cd to the toplevel" thing upfront and then runs hooks
   without doing anything special) and "git clone" (which goes to
   the newly created repository's working tree by calling
   setup.c::setup_work_tree() before builtin/clone.c::checkout(),
   which may call post-checkout hook).
 
I wonder if we need to clear existing GIT_DIR/GIT_WORK_TREE from the
environment, though.  When a user with a funny configuration (where
these two environment variables are pointing at unusual places) uses
"git worktree add" to create another worktree for the repository, it
would not be sufficient to chdir to defeat them that are appropriate
for the original, and not for the new, worktree, would it?

The Minerals, Metals & Materials Society Annual - TMS

[Sheryl Droege]

Hi,

 

 I understand your company is exhibiting in The Minerals, Metals & Materials
Society Annual on MAR/11 - MAR/15/2018.

 

Would you be interested in the complete contact information with email
addresses of Materials scientists and Engineers?

 

Available Data Fields: Practice Name, Web Address/URL, Contact name (First
name, middle name and last name), Specialty,, Postal address (street
address, city, state, zip code, and country), Phone, and Direct email
address.

 

Let me know your interest so that I can send you additional Information in
my next email.

 

Match Test for Appending : Just send us now 10 to 15 contacts in an excel
sheet from your in-house database with missing email address, telephone
numbers, fax numbers or mailing addresses, we can append it for you at no
cost, this will help you check the quality of our services.

 

Regards,  

Sheryl Droege

Marketing Associate

 

Please "Unsubscribe" if not interested in receiving further emails.

<>

Re: [PATCH v3 04/12] sequencer: introduce new commands to reset the revision

[Eric Sunshine]

On Sat, Feb 10, 2018 at 7:10 PM, Johannes Schindelin
 wrote:
> [...]
> This commit implements the commands to label, and to reset to, given
> revisions. The syntax is:
>
> label 
> reset 
> [...]
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -1922,6 +1951,151 @@ static int do_exec(const char *command_line)
> +static int safe_append(const char *filename, const char *fmt, ...)
> +{
> +   [...]
> +   if (write_in_full(fd, buf.buf, buf.len) < 0) {
> +   error_errno(_("could not write to '%s'"), filename);
> +   rollback_lock_file();

strbuf_release();

> +   return -1;
> +   }
> +   if (commit_lock_file() < 0) {
> +   rollback_lock_file();

strbuf_release();

> +   return error(_("failed to finalize '%s'"), filename);
> +   }
> +

strbuf_release();

> +   return 0;
> +}
> +
> +static int do_reset(const char *name, int len, struct replay_opts *opts)
> +{
> +   [...]
> +   unpack_tree_opts.reset = 1;
> +
> +   if (read_cache_unmerged())

rollback_lock_file();
strbuf_release(_name);

> +   return error_resolve_conflict(_(action_name(opts)));
> +
> +   if (!fill_tree_descriptor(, )) {
> +   error(_("failed to find tree of %s"), oid_to_hex());
> +   rollback_lock_file();
> +   free((void *)desc.buffer);
> +   strbuf_release(_name);
> +   return -1;
> +   }

Re: Fetch-hooks

[Brandon Williams]

On 02/10, Leo Gaspard wrote:
> On 02/10/2018 01:21 PM, Jeff King wrote:
> > On Sat, Feb 10, 2018 at 01:37:20AM +0100, Leo Gaspard wrote:
> > 
> >>> Yeah, tag-following may be a little tricky, because it usually wants to
> >>> write to refs/tags/. One workaround would be to have your config look
> >>> like this:
> >>>
> >>>   [remote "origin"]
> >>>   fetch = +refs/heads/*:refs/quarantine/origin/heads/*
> >>>   fetch = +refs/tags/*:refs/quarantine/origin/tags/*
> >>>   tagOpt = --no-tags
> >>>
> >>> That's not exactly the same thing, because it would fetch all tags, not
> >>> just those that point to the history on the branches. But in most
> >>> repositories and workflows the distinction doesn't matter.
> >>
> >> Hmm... apart from the implementation complexity (of which I have no
> >> idea), is there an argument against the idea of adding a
> >> remote..fetchTagsTo refmap similar to remote..fetch but used
> >> every time a tag is fetched? (well, maybe not exactly similar to
> >> remote..fetch because we know the source is going to be
> >> refs/tags/*; so just having the part of .fetch past the ':' would be
> >> more like what's needed I guess)
> > 
> > I don't think it would be too hard to implement, and is at least
> > logically consistent with the way we handle tags.
> > 
> > If we were designing from scratch, I do think this would all be helped
> > immensely by having more separation of refs fetched from remotes. There
> > was a proposal in the v1.8 era to fetch everything for a remote,
> > including tags, into "refs/remotes/origin/heads/",
> > "refs/remotes/origin/tags/", etc. And then we'd teach the lookup side to
> > look for tags in each of the remote-tag namespaces.
> > 
> > I still think that would be a good direction to go, but it would be a
> > big project (which is why the original stalled).
> 
> Hmm... would this also drown the remote..fetch map? Also, I think
> this behavior could be emulated with fetch and fetchTagsTo, and it would
> look like:
> [remote "my-remote"]
> fetch = +refs/heads/*:refs/remotes/my-remote/heads/*
> fetchTagsTo = refs/remotes/my-remote/tags/*
> The remaining issue being to teach the lookup side to look for tags in
> all the remote-tag namespaces (and the fact it's a breaking change).
> 
> That said, actually I just noticed an issue in the “add a
> remote..fetch option to fetch to refs/quarantine then have the
> post-fetch hook do the work”: it means if I run `git pull`, then:
>  1. The remote references will be pulled to refs/quarantine/...
>  2. The post-fetch hook will copy the accepted ones to refs/remotes/...
>  3. The `git merge FETCH_HEAD` called by pull will merge FETCH_HEAD into
> local branches... and so merge from refs/quarantine.
> 
> A solution would be to also update FETCH_HEAD in the post-fetch hook,
> but then we're back to the issue raised by Junio after the “*HOWEVER*”
> of [1]: the hook writer has to maintain consistency between the “copied”
> references and FETCH_HEAD.
> 
> So, when thinking about it, I'm back to thinking the proper hook
> interface should be the one of the tweak-fetch hook, but its
> implementation should make it not go crazy on remote servers. And so
> that the implementation should do all this refs/quarantine wizardry
> inside git itself.
> 
> So basically what I'm getting at at the moment is that git fetch should:
>  1. fetch all the references to refs/real-remotes//{insert here a
> copy of the refs/ tree of }
>  2. figure out what the “expected” names for these references will by,
> by looking at remote..fetch (and at whether --tags was passed)
>  3. for each “expected” name,
>  1. if a tweak-fetch hook is present, call it with the
> refs/real-remotes/... refname and the “expected end-name” from
> remote..fetch
>  2. based on the hook's result, potentially to move the “expected
> end-name” to another commit than the one referenced by refs/real-remotes/...
>  3. move the “expected” name to the commit referenced in
> refs/real-remotes
> 
> Which would make the tweak-fetch hook interface simpler (though more
> restrictive, but I don't have any real use case for the other change
> possibilities) than it is now:
>  1. feed the hook with lines of
> “refs/real-remotes/my-remote/heads/my-branch
> refs/remotes/my-remote/my-branch” (assuming remote.my-remote.fetch is
> “normal”) or “refs/real-remotes/my-remote/tags/my-tag refs/tags/my-tag”
> (if my-tag is being fetched from my-remote)
>  2. read lines of “ refs/remotes/my-remote/my-branch”, that
> will re-point my-branch to  instead of
> refs/real-remotes/my-remote/heads/my-branch. In order to avoid any
> issue, the hook is not allowed to pass as second output a reference that
> was not passed as second input.
> 
> This way, the behavior of the tweak-fetch hook is reasonably preserved
> (at least for my use case), and there is no additional load on the
> servers thanks to the up-to-date references being stored in
> refs/real-remotes//
> 
> Does this reasoning make 

Re: [PATCH 046/194] object-store: move replace_objects back to object-store

[Stefan Beller]

On Fri, Feb 9, 2018 at 3:15 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> @@ -32,7 +31,15 @@ struct object_store {
>>* Objects that should be substituted by other objects
>>* (see git-replace(1)).
>>*/
>> - struct replace_objects replacements;
>> + struct replace_objects {
>> + /*
>> +  * An array of replacements.  The array is kept sorted by the 
>> original
>> +  * sha1.
>> +  */
>> + struct replace_object **items;
>> +
>> + int alloc, nr;
>> + } replacements;
>>
>>   /*
>>* A fast, rough count of the number of objects in the repository.
>> @@ -49,7 +56,7 @@ struct object_store {
>>   unsigned packed_git_initialized : 1;
>>  };
>>  #define OBJECT_STORE_INIT \
>> - { NULL, MRU_INIT, ALTERNATES_INIT, REPLACE_OBJECTS_INIT, 0, 0, 0 }
>> + { NULL, MRU_INIT, ALTERNATES_INIT, { NULL, 0, 0 }, 0, 0, 0 }

Maybe we can move the REPLACE_OBJECTS_INIT just before the
definition of OBJECT_STORE_INIT, so we'd not need to touch this line here.

>
> Not the primary thrust of this topic, but we may want to convert
> these to use designated initializers after this series is done.

I agree.

I feel cbc0f81d96 (strbuf: use designated initializers in STRBUF_INIT,
2017-07-10)
may need longer cooking until all the interesting architectures have
tried picking up
the latest release, though.

Stefan

Re: [PATCH 003/194] object-store: move packed_git and packed_git_mru to object store

[Stefan Beller]

On Fri, Feb 9, 2018 at 2:09 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Patch generated by
>>
>>  2. Applying the semantic patch contrib/coccinelle/packed_git.cocci
>> to adjust callers.
>
> About this part...
>
>> diff --git a/contrib/coccinelle/packed_git.cocci 
>> b/contrib/coccinelle/packed_git.cocci
>> new file mode 100644
>> index 00..da317a51a9
>> --- /dev/null
>> +++ b/contrib/coccinelle/packed_git.cocci
>> @@ -0,0 +1,7 @@
>> +@@ @@
>> +- packed_git
>> ++ the_repository->objects.packed_git
>> +
>> +@@ @@
>> +- packed_git_mru
>> ++ the_repository->objects.packed_git_mru
>
> The above is correct for one-time transition turning pre-transition
> code to post the_repository world, but I am not sure if we want to
> have it in contrib/coccinelle/, where "make coccicheck" looks at, as
> a way to continuously keep an eye on "style" violations like using
> strbuf_addf() for a constant when strbuf_addstr() suffices.
>
> Wouldn't we need a mechanism to ensure that this file will *not* be
> used in "make coccicheck" somehow?
>

I can omit the cocci files from the patches, if that is better for maintenance.

I thought it may be a helpful
for merging this series with the rest of the evolved code base which
may make use of one of the converted functions. So instead of fixing
that new instance manually, cocinelle could do that instead.

Stefan

Re: [PATCH v3 07/14] commit-graph: update graph-head during write

[Junio C Hamano]

Derrick Stolee  writes:

> It is possible to have multiple commit graph files in a pack directory,
> but only one is important at a time. Use a 'graph_head' file to point
> to the important file. Teach git-commit-graph to write 'graph_head' upon
> writing a new commit graph file.

Why this design, instead of what "repack -a" would do, iow, if there
always is a singleton that is the only one that matters, shouldn't
the creation of that latest singleton just clear the older ones
before it returns control?

Re: REQUEST NEW TRANSLATION (INDONESIAN/id_ID)

[Stefan Beller]

On Sun, Feb 11, 2018 at 9:53 AM,   wrote:
> Hello git-l10n Team

cc'd Jiang Xi, who coordinates the git-l10n team.

>
> I want to join to this project as a translator for Indonesian language (ID)
> I have read the README file located in the
> https://github.com/git-l10n/git-po/blob/master/po/README directory
>
> I also have a fork repository master (git-l10n) to my repository (anaufalm),
> and also I have edited the TEAMS file by adding my name as a translator for
> Indonesia (id). And also I created a new file `id.po` which I copy from
> file` ca.po` as the source. Because I not find original file as english,
> like `en.po`.

cool!

>
> Furthermore, if approved, I will translate the file asap.

I don't think you need approval here, just do it. :)

Thanks,
Stefan

>
> Thank you.
>
> ---
>
> My repository (fork): https://github.com/anaufalm/git-id
> PR link request TEAMS: https://github.com/git-l10n/git-po/pull/288
> PR link add id.po file: https://github.com/git-l10n/git-po/pull/289

Re: [PATCH] describe: confirm that blobs actually exist

[Stefan Beller]

On Mon, Feb 12, 2018 at 9:23 AM, Jeff King  wrote:
> Prior to 644eb60bd0 (builtin/describe.c: describe a blob,
> 2017-11-15), we noticed and complained about missing
> objects, since they were not valid commits:
>
>   $ git describe 
>   fatal:  is not a valid 'commit' 
> object
>
> After that commit, we feed any non-commit to lookup_blob(),
> and complain only if it returns NULL. But the lookup_*
> functions do not actually look at the on-disk object
> database at all. They return an entry from the in-memory
> object hash if present (and if it matches the requested
> type), and otherwise auto-create a "struct object" of the
> requested type.
>
> A missing object would hit that latter case: we create a
> bogus blob struct, walk all of history looking for it, and
> then exit successfully having produced no output.
>
> One reason nobody may have noticed this is that some related
> cases do still work OK:
>
>   1. If we ask for a tree by sha1, then the call to
>  lookup_commit_referecne_gently() would have parsed it,

lookup_commit_reference_gently

>  and we would have its true type in the in-memory object
>  hash.
>
>   2. If we ask for a name that doesn't exist but isn't a
>  40-hex sha1, then get_oid() would complain before we
>  even look at the objects at all.
>
> We can fix this by replacing the lookup_blob() call with a
> check of the true type via sha1_object_info(). This is not
> quite as efficient as we could possibly make this check. We
> know in most cases that the object was already parsed in the
> earlier commit lookup, so we could call lookup_object(),
> which does auto-create, and check the resulting struct's
> type (or NULL).  However it's not worth the fragility nor
> code complexity to save a single object lookup.
>
> The new tests cover this case, as well as that of a
> tree-by-sha1 (which does work as described above, but was
> not explicitly tested).
>
> Noticed-by: Michael Haggerty 
> Signed-off-by: Jeff King 

This makes sense.
Reviewed-by: Stefan Beller 

Thanks!
Stefan

Re: What's cooking in git.git (Feb 2018, #01; Wed, 7)

[Jeff Hostetler]

* jh/status-no-ahead-behind (2018-01-24) 4 commits
  - status: support --no-ahead-behind in long format
  - status: update short status to respect --no-ahead-behind
  - status: add --[no-]ahead-behind to status and commit for V2 format.
  - stat_tracking_info: return +1 when branches not equal

  "git status" can spend a lot of cycles to compute the relation
  between the current branch and its upstream, which can now be
  disabled with "--no-ahead-behind" option.

  At v5; is this ready for 'next'?



I believe so.  I don't recall any further discussions on it.

The only open question was the idea of trying to walk 100 or so
commits and see if one was "close" to being an ancestor of the
other, but we thought that Stolee's graph cache would be a better
solution for that.

So, yes, I think it is ready for 'next'.

Thanks,
Jeff

Re: partial fetch

[Jeff Hostetler]

On 2/12/2018 11:24 AM, Basin Ilya wrote:

Hi.
In 2017 a set of patches titled "add object filtering for partial fetch" was 
accepted. Is it what I think it is? Will we be able to download only a subdirectory from a
large project?



yes, that is the goal.
there are several caveats, but yes, that is the goal.

Jeff

Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache

[Ben Peart]

On 2/12/2018 5:20 AM, Duy Nguyen wrote:

On Wed, Feb 7, 2018 at 9:13 PM, Ben Peart  wrote:


On 2/6/2018 7:27 AM, Duy Nguyen wrote:


This is another thing that bugs me. I know you're talking about huge
index files, but at what size should we start this sort of
optimization? Writing down a few MBs on linux is cheap enough that I
won't bother optimizing (and I get my UNTR extension repaired all the
time, so reduced lstat calls and stuff). This "typically" only comes
at certain size, what size?


It's important to identify what we're trading off here.  With the proposed
optimization, we'll end up doing less writes of the index but potentially
more lstat calls.  Even with a small index, writing the index is much more
expensive than calling lstat on a file.  Exactly how much more expensive
depends on a lot of variables but even with a SSD its still orders of
magnitude difference.

Keep in mind it's not just about lstat() calls. Processing ignore
patterns also takes a big chunk of CPU, especially when you have more
than a couple ignore rules.


Yes, I'm very familiar with the cost of the exclude list pattern 
matching code.  I've rewritten it to use a hashmap (for those patterns 
where it is possible) that dramatically speeds that aspect up as we tend 
to abuse it pretty heavily (~60K entries on average).




I'm not convinced that writing index files is that expensive for small
files. I don't know about Windows, with Linux it seems fast to me.
Actually, for small scale repos, performance probably does not differ
much either.


I agree.  For small scale repos, none of this is significant enough to 
matter.  Untracked caching should help most as your working directory 
starts to get large.



That means we could potentially lstat hundreds or thousands of files and
still come out ahead.  Given the untracked cache works at the directory
level, the lstat cost actually scales with the number of directories that
have had changes (ie changing a 2nd file in the same directory doesn't add
any additional cost).  In "typical" workflows, developers don't often change
hundreds of files across different directories so we'd "typically" still
come out ahead.

I agree. But we must deal with the bad case when someone
"accidentally" does that. We should not wait until them yell up "it's
too slow now" and tell them to disable/enable untracked cache again.

Another case that could touches a lot of directories over time is
switch trees (e.g. "git checkout master" then "checkout next" or worse
"checkout v1.0").


You're example above makes me wonder if you understand what my patch is 
doing.  If the index is flagged as dirty for _any_ reason, the entire 
index is written to disk with the latest information - including the 
updated untracked cache and all other extensions.  So in your checkout 
examples above, the index will still get written to disk with the 
updated untracked cache extension.  There would be zero change in 
behavior or performance.  The _only_ time the index is not written to 
disk after my patch is if there were no other changes to the index.  In 
my experience, that is only status calls.


To suffer any degradation in the untracked cache would be if a user 
edited a bunch of files across multiple directories and called status 
repeatedly.  As soon as they did add, checkout, rm, rebase, etc (ie most 
git commands other than status) the index would get flagged as dirty and 
the latest untracked cache extension would get written to disk.



We have internal performance tests based on common developer sequences of
commands (status, edit a file, status, add, status, commit, log, push, etc)
that show that having the untracked cache turned on actually makes these
sequences slower.  At the time, we didn't have time to investigate why so we
just turned off the untracked cache.

When writing the fsmonitor patch series which can utilize the untracked
cache, I did some investigation into why the untracked cache was slowing
things down in these tests and discovered the cause was the additional index
writes.  That is what led to this patch.

I've been sitting on it for months now because I didn't have the time to
write some performance tests for the git perf framework to demonstrate the
problem and how this helps solve it.  With the discussion about the
fsmonitor extension, I thought this might be a good time to send it out
there.

Hopefully you have time to get some of those numbers :) A patch is
more convincing when it's backed by numbers. And I'm still not
convinced that never ever letting untracked cache be written to the
index again is a right thing to do [1].


I agree that any performance patch should be accompanied by a 
performance test to demonstrate it actually performs as promised.  I 
looked for but didn't find a performance test for the untracked cache so 
will have to create one from scratch.  In order to make that as 
realistic as possible, it needs to simulate (as much as possible) 

Re: [PATCH] describe: confirm that blobs actually exist

[Jeff King]

On Mon, Feb 12, 2018 at 12:23:06PM -0500, Jeff King wrote:

> We can fix this by replacing the lookup_blob() call with a
> check of the true type via sha1_object_info(). This is not
> quite as efficient as we could possibly make this check. We
> know in most cases that the object was already parsed in the
> earlier commit lookup, so we could call lookup_object(),
> which does auto-create, and check the resulting struct's
> type (or NULL).  However it's not worth the fragility nor
> code complexity to save a single object lookup.

By the way, I did notice one other inefficiency here: we always call
lookup_commit_reference_gently() first, which will call parse_object().
So if you were to "git describe" an enormous blob, we'd load the whole
thing into memory for no purpose. We could structure this as:

  type = sha1_object_info(oid.hash, NULL);
  if (type == OBJ_BLOB)
  describe_blob();
  else if (lookup_commit_reference_gently(, 1))
  describe_commit();
  else
  describe("neither commit nor blob");

That incurs an extra object lookup for the commit case, but potentially
saves reading the blob. We could have our cake and eat it, too, if
sha1_file.c had a function like "parse this object unless it's a blob,
in which case just fill in the type info".

Arguably that should be the default when parse_object() is called on a
blob, but I suspect some older code may rely on parse_object() to check
that the object is present and consistent.

Anyway, I don't know that it's really worth caring about too much, but
just something I noticed.

Maybe a #leftoverbits if somebody cares.

-Peff

[PATCH] describe: confirm that blobs actually exist

[Jeff King]

Prior to 644eb60bd0 (builtin/describe.c: describe a blob,
2017-11-15), we noticed and complained about missing
objects, since they were not valid commits:

  $ git describe 
  fatal:  is not a valid 'commit' object

After that commit, we feed any non-commit to lookup_blob(),
and complain only if it returns NULL. But the lookup_*
functions do not actually look at the on-disk object
database at all. They return an entry from the in-memory
object hash if present (and if it matches the requested
type), and otherwise auto-create a "struct object" of the
requested type.

A missing object would hit that latter case: we create a
bogus blob struct, walk all of history looking for it, and
then exit successfully having produced no output.

One reason nobody may have noticed this is that some related
cases do still work OK:

  1. If we ask for a tree by sha1, then the call to
 lookup_commit_referecne_gently() would have parsed it,
 and we would have its true type in the in-memory object
 hash.

  2. If we ask for a name that doesn't exist but isn't a
 40-hex sha1, then get_oid() would complain before we
 even look at the objects at all.

We can fix this by replacing the lookup_blob() call with a
check of the true type via sha1_object_info(). This is not
quite as efficient as we could possibly make this check. We
know in most cases that the object was already parsed in the
earlier commit lookup, so we could call lookup_object(),
which does auto-create, and check the resulting struct's
type (or NULL).  However it's not worth the fragility nor
code complexity to save a single object lookup.

The new tests cover this case, as well as that of a
tree-by-sha1 (which does work as described above, but was
not explicitly tested).

Noticed-by: Michael Haggerty 
Signed-off-by: Jeff King 
---
 builtin/describe.c  | 2 +-
 t/t6120-describe.sh | 8 
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 6fe1c51281..18c68ec7a4 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -502,7 +502,7 @@ static void describe(const char *arg, int last_one)
 
if (cmit)
describe_commit(, );
-   else if (lookup_blob())
+   else if (sha1_object_info(oid.hash, NULL) == OBJ_BLOB)
describe_blob(oid, );
else
die(_("%s is neither a commit nor blob"), arg);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index a5d9015024..bae78c4e89 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -378,4 +378,12 @@ check_describe tags/A --all A
 check_describe tags/c --all c
 check_describe heads/branch_A --all --match='branch_*' branch_A
 
+test_expect_success 'describe complains about tree object' '
+   test_must_fail git describe HEAD^{tree}
+'
+
+test_expect_success 'describe complains about missing object' '
+   test_must_fail git describe $_z40
+'
+
 test_done
-- 
2.16.1.464.gc4bae515b7

partial fetch

[Basin Ilya]

Hi.
In 2017 a set of patches titled "add object filtering for partial fetch" was 
accepted. Is it what I think it is? Will we be able to download only a 
subdirectory from a
large project?

Re: totally confused as to what "git bisect skip" is supposed to do

[Christian Couder]

On Mon, Feb 12, 2018 at 11:44 AM, Robert P. J. Day
 wrote:
> On Fri, 9 Feb 2018, Junio C Hamano wrote:
>
>> "Robert P. J. Day"  writes:
>>
>> >   i'm confused ... why, after skipping a good chunk in the interval
>> > [v4.13,v4.14], do i still have exactly 7300 revisions to bisect? what
>> > am i so hopelessly misunderstanding here?
>>
>> Are you really "skipping" a chunk in the interval?
>>
>> I thought that "git bisect skip" is a way for you to respond, when
>> "git bisect" gave you a commit to test, saying "sorry, I cannot test
>> that exact version, please offer me something else to test".  And
>> each time you say that, you are not narrowing the search space in
>> any way, so it is understandable that the numver of candidate bad
>> commits will not decrease.
>
>   this might be an issue of terminology, then, as "man git-bisect"
> clearly suggests you can skip a range:
>
> You can also skip a range of commits, instead of just one
> commit, using range notation. For example:
>
>$ git bisect skip v2.5..v2.6
>
> This tells the bisect process that no commit after v2.5, up to
> and including v2.6, should be tested.

Yeah, I think this is kind of a terminology related.

First when git bisect says "Bisecting: XXX revisions left to test
after this" it doesn't mean that all those revisions left will
actually be tested, as git bisect's purpose is to avoid testing as
many revisions as possible.

So the XXX revisions are actually the revisions that possibly contain
the first bad commit.

And, as Junio wrote, when you tell git bisect that you cannot test
some revisions, it doesn't mean that those revisions cannot contain
the first bad commit.

> my issue (if this is indeed an issue) is that if i select to skip a
> sizable range of commits to test, should that not result in git bisect
> telling me it now has far fewer revisions to test? if i, in fact,
> manage to "disqualify" a number of commits from testing, is there no
> visual confirmation that i now have fewer commits to test?

I hope that the above clarification I gave is enough, but maybe the
following will help you.

If you cannot test let's say 20 commits because there is build problem
in those commits, and in the end Git tells you that the first bad
commit could be any of 3 commits, 2 of them that were previously
marked with skip, then you could still, if you wanted, fix those
commits, so that they can be built and test them.

So yeah if we only talk about the current bisection, the skipped
commits will not be tested, but if we talk about completely finishing
the bisection and finding the first bad commit, then those commits
could still be tested.

Re: [PATCH 1/1] Mark messages for translations

[Jeff King]

On Mon, Feb 12, 2018 at 04:03:49PM +0100, Alexander Shopov wrote:

> @Jeff:
> > we may want to avoid this anti-pattern
> Current state of these tests is wrong and I should rework them.
> 
> Here is what I intend to do:
> 1. Fix the commit message
> 2. Check whether I can get the tests in t0002-gitfile.sh to the
> standard `test_i18ngrep !` negative (i.e. without using `if`)
> 3. Post and ask for feedback again

See the patch I posted earlier. For (2), "test_i18ngrep !" would be the
wrong thing. I think you should either:

  - keep your patch as-is, and let Junio resolve the conflict when the
two are merged

  - rebase your patch on top of mine. That's slightly less work for
Junio, but it means that your topic cannot graduate until mine does
(though hopefully mine is pretty non-controversial).

I'd probably just do the first in your place, since the conflict is easy
to resolve.

-Peff

please change stash

[Karsten Fluegge]

Dear great team,

Normal git tooling creates different files file.ORIG file.LOCAL
file.REMOTE in case of conflicts.

However `git stash pop` manipulates your files directly resulting in
lines like:

<<< Updated upstream

>>> Stashed changes

This can seriously corrupt files and workflows.

If it is «the user's fault» or negligence then at least we're not the
only one:

https://github.com/search?q=Stashed+changes=Code

30 'idiots' might hint at a UX problem. (factor 10 in darknet)

-- 
Kind regards,
Karsten Flügge, CEO
GmbH

Hagenkampsweg 10
25474 Hasloh
Germany

Mobile +49-176-64638989   
Support +1-855-447-2666
E-Mail i...@pannous.com
Homepage pannous.com

Handelsregister: Amtsgericht Pinneberg HRB 7795 PI
Sitz der Gesellschaft: Hasloh
Steuernummer: 18/291/16961
USt-Id Nr: DE264064657
CEO: Karsten Flügge


-- 
Kind regards,
Karsten Flügge, CEO
GmbH

Hagenkampsweg 10
25474 Hasloh
Germany

Mobile +49-176-64638989   
Support +1-855-447-2666
E-Mail i...@pannous.com
Homepage pannous.com

Handelsregister: Amtsgericht Pinneberg HRB 7795 PI
Sitz der Gesellschaft: Hasloh
Steuernummer: 18/291/16961
USt-Id Nr: DE264064657
CEO: Karsten Flügge

Re: [PATCH 1/1] Mark messages for translations

[Alexander Shopov]

Let me repeat what you said so I know how to improve the patch:
@Junio:
> Perhaps end each sentence with a full-stop?
I should end each sentence in the *log* message with "." (rather than
the translatable strings in the patch)

> Shouldn't this rather be like so instead?
> if test_i18ngrep ! "invalid gitfile format" .err
...
> These two ones want to be written
The standard negation form is:
   test_i18ngrep !
but I should leave the `!` in front of `test_i18ngrep` in this particular case

@Jeff:
> we may want to avoid this anti-pattern
Current state of these tests is wrong and I should rework them.

Here is what I intend to do:
1. Fix the commit message
2. Check whether I can get the tests in t0002-gitfile.sh to the
standard `test_i18ngrep !` negative (i.e. without using `if`)
3. Post and ask for feedback again

Kind regards:
al_shopov

Re: [PATCH v3 00/35] protocol version 2

[Derrick Stolee]

On 2/6/2018 8:12 PM, Brandon Williams wrote:

Changes in v3:
  * There were some comments about how the protocol should be designed
stateless first.  I've made this change and instead of having to
supply the `stateless-rpc=true` capability to force stateless
behavior, the protocol just requires all commands to be stateless.
  
  * Added some patches towards the end of the series to force the client

to not request to use protocol v2 when pushing (even if configured to
use v2).  This is to ease the roll-out process of a push command in
protocol v2.  This way when servers gain the ability to accept
pushing in v2 (and they start responding using v2 when requests are
sent to the git-receive-pack endpoint) that clients who still don't
understand how to push using v2 won't request to use v2 and then die
when they recognize that the server does indeed know how to accept a
push under v2.

  * I implemented the `shallow` feature for fetch.  This feature
encapsulates the existing functionality of all the shallow/deepen
capabilities in v0.  So now a server can process shallow requests.

  * Various other small tweaks that I can't remember :)

After all of that I think the series is in a pretty good state, baring
any more critical reviewing feedback.

Thanks!

Brandon Williams (35):
   pkt-line: introduce packet_read_with_status
   pkt-line: introduce struct packet_reader
   pkt-line: add delim packet support
   upload-pack: convert to a builtin
   upload-pack: factor out processing lines
   transport: use get_refs_via_connect to get refs
   connect: convert get_remote_heads to use struct packet_reader
   connect: discover protocol version outside of get_remote_heads
   transport: store protocol version
   protocol: introduce enum protocol_version value protocol_v2
   test-pkt-line: introduce a packet-line test helper
   serve: introduce git-serve
   ls-refs: introduce ls-refs server command
   connect: request remote refs using v2
   transport: convert get_refs_list to take a list of ref patterns
   transport: convert transport_get_remote_refs to take a list of ref
 patterns
   ls-remote: pass ref patterns when requesting a remote's refs
   fetch: pass ref patterns when fetching
   push: pass ref patterns when pushing
   upload-pack: introduce fetch server command
   fetch-pack: perform a fetch using v2
   upload-pack: support shallow requests
   fetch-pack: support shallow requests
   connect: refactor git_connect to only get the protocol version once
   connect: don't request v2 when pushing
   transport-helper: remove name parameter
   transport-helper: refactor process_connect_service
   transport-helper: introduce stateless-connect
   pkt-line: add packet_buf_write_len function
   remote-curl: create copy of the service name
   remote-curl: store the protocol version the server responded with
   http: allow providing extra headers for http requests
   http: don't always add Git-Protocol header
   remote-curl: implement stateless-connect command
   remote-curl: don't request v2 when pushing

  .gitignore  |   1 +
  Documentation/technical/protocol-v2.txt | 338 +
  Makefile|   7 +-
  builtin.h   |   2 +
  builtin/clone.c |   2 +-
  builtin/fetch-pack.c|  21 +-
  builtin/fetch.c |  14 +-
  builtin/ls-remote.c |   7 +-
  builtin/receive-pack.c  |   6 +
  builtin/remote.c|   2 +-
  builtin/send-pack.c |  20 +-
  builtin/serve.c |  30 ++
  builtin/upload-pack.c   |  74 
  connect.c   | 352 +-
  connect.h   |   7 +
  fetch-pack.c| 319 +++-
  fetch-pack.h|   4 +-
  git.c   |   2 +
  http.c  |  25 +-
  http.h  |   2 +
  ls-refs.c   |  96 +
  ls-refs.h   |   9 +
  pkt-line.c  | 149 +++-
  pkt-line.h  |  77 
  protocol.c  |   2 +
  protocol.h  |   1 +
  remote-curl.c   | 257 -
  remote.h|   9 +-
  serve.c | 260 +
  serve.h |  15 +
  t/helper/test-pkt-line.c|  64 
  t/t5701-git-serve.sh| 176 +
  t/t5702-protocol-v2.sh  | 239 
  transport-helper.c  |  84 +++--
  transport-internal.h|   4 +-
  transport.c

*Beachtung*

[Euro Millions]

Herzlichen Glückwunsch, Sie haben am 31. Januar 2018 in den monatlichen 
Gewinnspielen von Euro Millions/Google Promo 650.000 gewonnen.

Kontaktieren Sie unseren Schadenregulierungsbeauftragten mit den folgenden 
Informationen

1. Vollst?ndiger Name
2. Adresse
3. Geschlecht
4. Alter
5. Beruf
6. Telefon

Pianese Germano
Online-Koordinator

Hi Dear.

[Jennifer Williams]

Hi Dear,

How are you today I hope that everything is OK with you as it is my great 
pleasure to contact you in having communication with you starting from today, I 
was just going through the Internet search when I found your email address, I 
want to make a very new and special friend, so I decided to contact you to see 
how we can make it work if we can. Please I wish you will have the desire with 
me so that we can get to know each other better and see what happens in future.

My name is Jennifer Williams, I am an American  presently I live in the UK, I 
will be very happy if you can write me through my private email address() for 
easy communication so that we can know each other, I will give you my pictures 
and details about me.

Bye
Jennifer.