Re: [RFC/PATCH 2/3] wildmatch: add interface for precompiling wildmatch() patterns
Ævar Arnfjörð Bjarmason writes:
> +struct wildmatch_compiled *wildmatch_compile(const char *pattern, unsigned
> int flags)
> +{
> + struct wildmatch_compiled *code = xmalloc(sizeof(struct
> wildmatch_compiled));
> + code->pattern = xstrdup(pattern);
> + code->flags = flags;
> +
> + return code;
> +}
> +
> +int wildmatch_match(struct wildmatch_compiled *code, const char *text)
> +{
> + return wildmatch(code->pattern, text, code->flags);
> +}
Is the far-in-the-future vision to make this the other way around?
That is, this being scaffolding, wildmatch_match() which is supposed
to be precompiled match actually uses wildmatch() as its underlying
engine, but when a viable compilation machinery is plugged in, the
wildmatch_match() that takes a precompiled pattern will call into
the machinery to execute the compiled pattern, and wildmatch() will
be reimplemented as "compile, call wildmatch_match() once and
discard" sequence?
Otherwise I'd be worried about wildmatch() vs wildmatch_match()
introducing subtle behaviour differences that leads to hard to debug
problems.
Re: [PATCH v2 00/29] Create a reference backend for packed refs
On Fri, Jun 23, 2017 at 02:47:01PM -0700, Junio C Hamano wrote: > > Speculating on my own question. I guess it would prepare us for a day > > when a possible ref store is to use a packed-refs _without_ loose refs. > > IOW, the property is defined on packed-refs today, but a possible future > > direction would be to use it by itself. But maybe I'm just making things > > up. > > OK. In other words, it's not a packed-refs's characteristics that > cruft are allowed. It's that a ref storage that is implemented as > an overlay of one storage (which happens to be the loose one) on top > of another (which happens to be the packed refs file) allows the > latter one to have cruft if (and only if) that broken one is covered > by the former one. Thanks, that's a much better way of saying what I was trying to get at. I don't know if that's Michael's argument or not, but it's certainly one I find reasonable. :) -Peff
Re: --color-moved feedback, was Re: [PATCH v2 19/29] packed-backend: new module for handling packed references
On Fri, Jun 23, 2017 at 01:23:52PM -0700, Stefan Beller wrote: > > In the end, I just did --color-moved=plain, ... > > "yep, this is all a giant moved chunk, so I don't have to look carefully > > at it". > > This is dangerous, as "plain" does not care about permutations. > See the 7f5af90798 (diff.c: color moved lines differently, 2017-06-19) > for details. You would want at least "zebra", which would show you > permutations. Ah, I see. I think what I'd really want is some way of correlating two particular hunks. That's hard to do with color, though. I guess that's the "you would need a ton of colors" discussion I saw going on before? It would depend on how many hunks there are, and how close together they are. For instance, your 6cd5757c8 seems like a good candidate, but I have to admit with --color-moved=zebra I have no idea what's going on. Some things are definitely colored, but I'm not sure what corresponds to what. > > That feels more dangerous to me, just because the heuristics seem to > > find a lot of "moves" of repeated code. Imagine a simple patch like > > "switch return 0 to return -1". If the code you're touching went away, > > there's a very good chance that another "return 0" popped up somewhere > > else in the code base. A conflict is what you want there; silently > > changing some other site would be not only wrong, but quite subtle and > > hard to find. > > I agree, that is the danger, but the scale of danger is the size of the moved > chunk. A file is usually a very large chunk, such that it is obviously a good > idea to fix that. Maybe we'd introduce a threshold, that the fix must not be > in > range of the block boundaries of say 3 lines. > (Then the moved block must be at least 7 lines of code such that a one liner > fix in the middle would kick in) Yes, I'd agree it's really a continuum from "one line" to "whole file". I think right now the --color-moved stuff is too close to the former to be safe, but pushing it farther towards the middle would remedy that. -Peff
Re: What's cooking in git.git (Jun 2017, #06; Thu, 22)
Junio C Hamano writes: > But for the purpose of this "moved line coloring", > excluding multiple copy destinations of the same thing may be a > simpler and more robust solution. It will not catch "somebody > stupidly removed one function and made two private copies", though. Let me take this one back. Treating multiple copy destinations and multiple copy sources differently is a bad idea. It is easy for a user to give "-R" option to "diff" to turn such a stupid patch into "somebody brilliantly consolidated two copies of the same thing into a single function", and we want to keep "diff" and "diff -R" output symmetric.
Re: [PATCH 2/3] builtin/fetch: parse recurse-submodules-default at default options parsing
Stefan Beller writes:
>>> if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
>>> - if (recurse_submodules_default) {
>>> - int arg =
>>> parse_fetch_recurse_submodules_arg("--recurse-submodules-default",
>>> recurse_submodules_default);
>>> - set_config_fetch_recurse_submodules(arg);
>>> - }
>>> + if (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)
>>> +
>>> set_config_fetch_recurse_submodules(recurse_submodules_default);
>>
>> This is not a new thing, and it may not even be a problem, but I
>> have to wonder why this needs to be done conditionally. "The
>> ...
>
> As far as I suspect, the original author considered
> evaluating the additional config too expensive.
>
> gitmodules_config(); // <- this specifically?
> git_config(submodule_config, NULL);
>
> And that is why we only react if any switch is
> given to recurse.
I am not talking about the outer "if" condition.
The inner
"if (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)"
block, is what I am questioning, i.e. I am wondering why
set_config_fetch_recurse_submodules() need to be conditionally
called.
The two statement you quoted above will be executed either way,
regardless of the value of recurse_submodule_default.
Unless --recurse-submodules=off is given, in which case the outer
"if" condition rejects. And I think you are explaining that part,
but that was not what I am questioning.
Re: What's cooking in git.git (Jun 2017, #06; Thu, 22)
Stefan Beller writes: > * add some heuristic to omit small blobs, (empty lines, closing braces) > Maybe this is can be solved by not considering anything > that occurs multiple times? I vaguely recall that we had to do something similar in "blame -C" where it tries to avoid passing blame for insignificant changes down as copied. But for the purpose of this "moved line coloring", excluding multiple copy destinations of the same thing may be a simpler and more robust solution. It will not catch "somebody stupidly removed one function and made two private copies", though.
Re: What's cooking in git.git (Jun 2017, #06; Thu, 22)
Ævar Arnfjörð Bjarmason writes:
> This is bad:
>
> $ ./git --exec-path=$PWD -c diff.colorMoved=crap show
> fatal: unable to parse 'diff.colormoved' from command-line config
>
> Fixed with:
>
> diff --git a/diff.c b/diff.c
> index 7cae4f1ddb..036dbc1c3c 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -278,7 +278,7 @@ int git_diff_ui_config(const char *var, const char
> *value, void *cb)
> if (!strcmp(var, "diff.colormoved")) {
> int cm = parse_color_moved(value);
> if (cm < 0)
> - return -1;
> + die("bad --color-moved argument: %s", value);
> diff_color_moved_default = cm;
> return 0;
> }
You would want to model this after an existing practice nearby.
$ git -c color.diff.new=frotz diff
error: error: invalid color value: frotz
fatal: unable to parse 'color.diff.new' from command-line config
It could be argued that it would even be better to show "here is the
list of valid values the configuration takes", but generally we do
not do that in other configuration variables.
Re: What's cooking in git.git (Jun 2017, #06; Thu, 22)
Junio C Hamano writes: >>> * ab/sha1dc (2017-06-07) 2 commits >>> ... >>> Will keep in 'pu'. >>> Impact to the various build and release infrastructure of using >>> submodule is not yet fully known, but this lets us dip our toes. >> ... >> But it's been 1 month kicking around in pu now. What are we gaining from >> keeping it there even longer at this point? > > I am sort of waiting for a success from Windows box at Travis. It > hasn't passed for other reasons for a while, though. https://travis-ci.org/git/git/jobs/246371006#L480 shows that on Linux, we are taking the code from the submodule and the compilation is happy. https://travis-ci.org/git/git/jobs/246371008#L25 shows that we do init and update the submodule but it does not appear that neither the bundled sha1dc/ nor the submodule gets used on MacOS. Probably we are using the hash from the platform? https://travis-ci.org/git/git/jobs/246371011#L724 shows that the bundled sha1dc/ is used on Windows, not from the submodule, for whatever reason. I trust what Dscho does for Windows platform well enough that I do not feel unconfortable for not knowing why the build there triggered by Travis does not use the submodule one. Whatever configuration he chooses would be the best for the platform. So the only nit I may have is that we may possibly want to turn this on in .travis.yml on MacOS before we move it forward (otherwise we'd be shipping bundled one and submodule one without doing any build on that platform)? Other than that, the topic seems ready to be merged down. Thanks.
Re: What's cooking in git.git (Jun 2017, #06; Thu, 22)
> This is good:
>
> $ ./git --exec-path=$PWD show --color-moved=crap
> fatal: bad --color-moved argument: crap
>
> This is bad:
>
> $ ./git --exec-path=$PWD -c diff.colorMoved=crap show
> fatal: unable to parse 'diff.colormoved' from command-line config
>
> Fixed with:
>
> diff --git a/diff.c b/diff.c
> index 7cae4f1ddb..036dbc1c3c 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -278,7 +278,7 @@ int git_diff_ui_config(const char *var, const char
> *value, void *cb)
> if (!strcmp(var, "diff.colormoved")) {
> int cm = parse_color_moved(value);
> if (cm < 0)
> - return -1;
> + die("bad --color-moved argument: %s", value);
> diff_color_moved_default = cm;
> return 0;
> }
>
> But I'm not familiar enough with the code to say if just dying here, as
> opposed to returning -1 is OK or not.
I think this one is 'not good', as it (a) does not help the user a lot
and (b) is not consistent with the rest around in that function, for example
$ git -c diff.interhunkcontext=-2 diff
fatal: unable to parse 'diff.interhunkcontext' from command-line config
So instead of return -1, we could issue an additional warning, but this could
be a generic warning?
Re: What's cooking in git.git (Jun 2017, #06; Thu, 22)
On Fri, Jun 23, 2017 at 2:59 PM, Ævar Arnfjörð Bjarmason
wrote:
>
> On Thu, Jun 22 2017, Junio C. Hamano jotted:
>
>> * sb/diff-color-move (2017-06-21) 25 commits
>> - diff: document the new --color-moved setting
>> - diff.c: add dimming to moved line detection
>> - diff.c: color moved lines differently, plain mode
>> - diff.c: color moved lines differently
>> - diff.c: buffer all output if asked to
>> - diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY
>> - diff.c: emit_diff_symbol learns about DIFF_SYMBOL_STAT_SEP
>> - diff.c: convert word diffing to use emit_diff_symbol
>> - diff.c: convert show_stats to use emit_diff_symbol
>> - diff.c: convert emit_binary_diff_body to use emit_diff_symbol
>> - submodule.c: migrate diff output to use emit_diff_symbol
>> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_REWRITE_DIFF
>> - diff.c: emit_diff_symbol learns about DIFF_SYMBOL_BINARY_FILES
>> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_HEADER
>> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_FILEPAIR
>> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_CONTEXT_INCOMPLETE
>> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_WORDS{_PORCELAIN}
>> - diff.c: migrate emit_line_checked to use emit_diff_symbol
>> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_NO_LF_EOF
>> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_CONTEXT_FRAGINFO
>> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_CONTEXT_MARKER
>> - diff.c: introduce emit_diff_symbol
>> - diff.c: factor out diff_flush_patch_all_file_pairs
>> - diff.c: move line ending check into emit_hunk_header
>> - diff.c: readability fix
>>
>> "git diff" has been taught to optionally paint new lines that are
>> the same as deleted lines elsewhere differently from genuinely new
>> lines.
>>
>> Is any more update coming?
>
> I guess here's as good a place for feedback is any, this feature's
> great, but I discovered some minor warts in it:
Thanks for reporting these. :)
So:
* have the boolean option as below
* fix error modes in config
* rewrite documentation for lazy skimming readers :)
I also consider
* changing the default to zebra (instead of dimmed_zebra)
Junio wrote (when reviewing Michaels series)
> This patch shows OK, but when applied to other changes in the
> series, e.g. "packed_ref_store: make class into a subclass of
> `ref_store`", it was somewhat irritating that all new blank lines
> are shown as a copy-move of a single deleted blank line (also a
> single "return refs" in an entirely new function being a copy/move
> looked confusing).
* add some heuristic to omit small blobs, (empty lines, closing braces)
Maybe this is can be solved by not considering anything
that occurs multiple times?
Re: [PATCH] xdiff: trim common tail with -U0 after diff
On 23.06.2017 22:39, René Scharfe wrote: > The changed test script passes just fine for me even without your change > to xdiff-interface.c, which is odd. Sorry, I've apparently messed this up - it seems to be the case for me, too. I would assume that using the functions.c context/diff in this test file might prove it, but when I wrote this test I was already unsure to base it on an experimental feature, that might just get removed again (with its tests). > Do you have another way to demonstrate the unexpected behavior? It was clearly reproducible with a local test case, based on "copying an existing test case, and modifying the header for it only". Given the other replies, it seems like it is more a question now if the trimming should get moved down, or removed completely it seems. So I will not update the patch/test for now. -- http://daniel.hahler.de/ signature.asc Description: OpenPGP digital signature
Re: [PATCH] submodule--helper: teach push-check to handle HEAD
Brandon Williams writes:
> In 06bf4ad1d (push: propagate remote and refspec with
> --recurse-submodules) push was taught how to propagate a refspec down to
> submodules when the '--recurse-submodules' flag is given. The only refspecs
> that are allowed to be propagated are ones which name a ref which exists
> in both the superproject and the submodule, with the caveat that 'HEAD'
> was disallowed.
>
> This patch teaches push-check (the submodule helper which determines if
> a refspec can be propagated to a submodule) to permit propagating 'HEAD'
> if and only if the superproject and the submodule both have the same
> named branch checked out and the submodule is not in a detached head
> state.
>
> Signed-off-by: Brandon Williams
> ---
> builtin/submodule--helper.c| 57
> +++---
> submodule.c| 18 ++---
> t/t5531-deep-submodule-push.sh | 25 +-
> 3 files changed, 82 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 1b4d2b346..fd5020036 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1107,24 +1107,41 @@ static int resolve_remote_submodule_branch(int argc,
> const char **argv,
> static int push_check(int argc, const char **argv, const char *prefix)
> {
> struct remote *remote;
> + const char *superproject_head;
> + char *head;
> + int detached_head = 0;
> + struct object_id head_oid;
>
> - if (argc < 2)
> - die("submodule--helper push-check requires at least 1
> argument");
> + if (argc < 3)
> + die("submodule--helper push-check requires at least 2
> argument");
"arguments"?
> +
> + /*
> + * superproject's resolved head ref.
> + * if HEAD then the superproject is in a detached head state, otherwise
> + * it will be the resolved head ref.
> + */
> + superproject_head = argv[1];
The above makes it sound like the caller gives either "HEAD" (when
detached) or "refs/heads/branch" (when on 'branch') in argv[1] and
you are stashing it away, but ...
> + /* Get the submodule's head ref and determine if it is detached */
> + head = resolve_refdup("HEAD", 0, head_oid.hash, NULL);
> + if (!head)
> + die(_("Failed to resolve HEAD as a valid ref."));
> + if (!strcmp(head, "HEAD"))
> + detached_head = 1;
... the work to see which branch we are on and if we are detached is
done by this code without consulting argv[1]. I cannot tell what is
going on. Is argv[1] assigned to superproject_head a red herring?
> /*
>* The remote must be configured.
>* This is to avoid pushing to the exact same URL as the parent.
>*/
> - remote = pushremote_get(argv[1]);
> + remote = pushremote_get(argv[2]);
> if (!remote || remote->origin == REMOTE_UNCONFIGURED)
> - die("remote '%s' not configured", argv[1]);
> + die("remote '%s' not configured", argv[2]);
>
> /* Check the refspec */
> - if (argc > 2) {
> - int i, refspec_nr = argc - 2;
> + if (argc > 3) {
> + int i, refspec_nr = argc - 3;
> struct ref *local_refs = get_local_heads();
> struct refspec *refspec = parse_push_refspec(refspec_nr,
> - argv + 2);
> + argv + 3);
If you have no need for argv[1] (and you don't, as you have stashed
it away in superproject_head), it may be less damage to the code if
you shifted argv upfront after grabbing superproject_head.
> for (i = 0; i < refspec_nr; i++) {
> struct refspec *rs = refspec + i;
> @@ -1132,18 +1149,30 @@ static int push_check(int argc, const char **argv,
> const char *prefix)
> if (rs->pattern || rs->matching)
> continue;
>
> - /*
> - * LHS must match a single ref
> - * NEEDSWORK: add logic to special case 'HEAD' once
> - * working with submodules in a detached head state
> - * ceases to be the norm.
> - */
> - if (count_refspec_match(rs->src, local_refs, NULL) != 1)
> + /* LHS must match a single ref */
> + switch(count_refspec_match(rs->src, local_refs, NULL)) {
"switch (count..."
> + case 1:
> + break;
> + case 0:
> + /*
> + * If LHS matches 'HEAD' then we need to ensure
> + * that it matches the same named branch
> + * checked out in the superproject.
> + */
> +
Re: [PATCH 2/3] builtin/fetch: parse recurse-submodules-default at default options parsing
On Fri, Jun 23, 2017 at 3:36 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> @@ -1333,10 +1336,8 @@ int cmd_fetch(int argc, const char **argv, const char
>> *prefix)
>> deepen = 1;
>>
>> if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
>> - if (recurse_submodules_default) {
>> - int arg =
>> parse_fetch_recurse_submodules_arg("--recurse-submodules-default",
>> recurse_submodules_default);
>> - set_config_fetch_recurse_submodules(arg);
>> - }
>> + if (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)
>> +
>> set_config_fetch_recurse_submodules(recurse_submodules_default);
>
> This is not a new thing, and it may not even be a problem, but I
> have to wonder why this needs to be done conditionally. "The
> command line did not explicitly say --no-recurse-submodules, so we
> would set what came from --recurse-submodule-default if and only if
> that is given---otherwise we leave it as a compiled-in default" is
> what the code before or after this patch tells me. But if we drop
> the "if (default is not DEFAULT)", the resulting code becomes "The
> command line did not explicitly say --no-recurse-submodules, so we
> would set whatever is the default (which may not have been given
> from the command line, but came built-in to the binary)". Aren't
> they saying the same thing?
I assumed so as well, yes. But the test suite pointed out there
is another subtlety going on, which I have not dug into, yet.
>
> It's not like there is a configuration knob that further interferes
> the interaction between these two. I am puzzled.
As far as I suspect, the original author considered
evaluating the additional config too expensive.
gitmodules_config(); // <- this specifically?
git_config(submodule_config, NULL);
And that is why we only react if any switch is
given to recurse. Before be254a0ea9 (Add the
'fetch.recurseSubmodules' config setting, 2010-11-11)
we would not load the config unless the user
asked for submodule action.
Then with the config switch we *had* to load the config
(both .gitmodules as well as .git/config), so at that commit
it would have made sense to inline this into the regular config
and command line argument parsing.
Re: [PATCH 2/3] builtin/fetch: parse recurse-submodules-default at default options parsing
Stefan Beller writes:
> @@ -1333,10 +1336,8 @@ int cmd_fetch(int argc, const char **argv, const char
> *prefix)
> deepen = 1;
>
> if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
> - if (recurse_submodules_default) {
> - int arg =
> parse_fetch_recurse_submodules_arg("--recurse-submodules-default",
> recurse_submodules_default);
> - set_config_fetch_recurse_submodules(arg);
> - }
> + if (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)
> +
> set_config_fetch_recurse_submodules(recurse_submodules_default);
This is not a new thing, and it may not even be a problem, but I
have to wonder why this needs to be done conditionally. "The
command line did not explicitly say --no-recurse-submodules, so we
would set what came from --recurse-submodule-default if and only if
that is given---otherwise we leave it as a compiled-in default" is
what the code before or after this patch tells me. But if we drop
the "if (default is not DEFAULT)", the resulting code becomes "The
command line did not explicitly say --no-recurse-submodules, so we
would set whatever is the default (which may not have been given
from the command line, but came built-in to the binary)". Aren't
they saying the same thing?
It's not like there is a configuration knob that further interferes
the interaction between these two. I am puzzled.
> gitmodules_config();
> git_config(submodule_config, NULL);
> }
Re: [PATCH 1/3] builtin/fetch: factor submodule recurse parsing out to submodule config
Stefan Beller writes:
> Later we want to access this parsing in builtin/pull as well.
>
> Signed-off-by: Stefan Beller
> ---
> builtin/fetch.c| 18 ++
> submodule-config.c | 22 ++
> submodule-config.h | 3 +++
> 3 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 100248c5af..9d58dc0a8a 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -53,20 +53,6 @@ static int shown_url = 0;
> static int refmap_alloc, refmap_nr;
> static const char **refmap_array;
>
> -static int option_parse_recurse_submodules(const struct option *opt,
> -const char *arg, int unset)
> -{
> - if (unset) {
> - recurse_submodules = RECURSE_SUBMODULES_OFF;
> - } else {
> - if (arg)
> - recurse_submodules =
> parse_fetch_recurse_submodules_arg(opt->long_name, arg);
> - else
> - recurse_submodules = RECURSE_SUBMODULES_ON;
> - }
> - return 0;
> -}
> -
> static int git_fetch_config(const char *k, const char *v, void *cb)
> {
> if (!strcmp(k, "fetch.prune")) {
> @@ -115,9 +101,9 @@ static struct option builtin_fetch_options[] = {
> N_("number of submodules fetched in parallel")),
> OPT_BOOL('p', "prune", &prune,
>N_("prune remote-tracking branches no longer on remote")),
> - { OPTION_CALLBACK, 0, "recurse-submodules", NULL, N_("on-demand"),
> + { OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules,
> N_("on-demand"),
> N_("control recursive fetching of submodules"),
> - PARSE_OPT_OPTARG, option_parse_recurse_submodules },
> + PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules },
> OPT_BOOL(0, "dry-run", &dry_run,
>N_("dry run")),
> OPT_BOOL('k', "keep", &keep, N_("keep downloaded pack")),
> diff --git a/submodule-config.c b/submodule-config.c
> index 4f58491ddb..265d036095 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -2,6 +2,7 @@
> #include "submodule-config.h"
> #include "submodule.h"
> #include "strbuf.h"
> +#include "parse-options.h"
>
> /*
> * submodule cache lookup structure
> @@ -234,6 +235,27 @@ int parse_fetch_recurse_submodules_arg(const char *opt,
> const char *arg)
> return parse_fetch_recurse(opt, arg, 1);
> }
>
> +int option_fetch_parse_recurse_submodules(const struct option *opt,
> + const char *arg, int unset)
> +{
> + int *v;
> +
> + if (!opt->value)
> + return -1;
It would have been easier to view this change if the original
already had used opt->value to specify where to place the parsed
value, but that is water under the bridge ;-)
Looks like a faithful converison to me.
> + v = opt->value;
> +
> + if (unset) {
> + *v = RECURSE_SUBMODULES_OFF;
> + } else {
> + if (arg)
> + *v = parse_fetch_recurse_submodules_arg(opt->long_name,
> arg);
> + else
> + *v = RECURSE_SUBMODULES_ON;
> + }
> + return 0;
> +}
> +
> static int parse_update_recurse(const char *opt, const char *arg,
> int die_on_error)
> {
> diff --git a/submodule-config.h b/submodule-config.h
> index d434ecdb45..1076a68653 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -23,6 +23,9 @@ struct submodule {
> };
>
> extern int parse_fetch_recurse_submodules_arg(const char *opt, const char
> *arg);
> +struct option;
> +extern int option_fetch_parse_recurse_submodules(const struct option *opt,
> + const char *arg, int unset);
> extern int parse_update_recurse_submodules_arg(const char *opt, const char
> *arg);
> extern int parse_push_recurse_submodules_arg(const char *opt, const char
> *arg);
> extern int parse_submodule_config_option(const char *var, const char *value);
Re: [PATCH v2 3/3] t1700: make sure split-index respects core.sharedrepository
Christian Couder writes: > Add a few tests to check that both the split-index file and the > shared-index file are created using the right permissions when > core.sharedrepository is set. > > Signed-off-by: Christian Couder > --- > t/t1700-split-index.sh | 17 + > 1 file changed, 17 insertions(+) > > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh > index af3ec0da5a..2c5be732e4 100755 > --- a/t/t1700-split-index.sh > +++ b/t/t1700-split-index.sh > @@ -370,4 +370,21 @@ test_expect_success 'check splitIndex.sharedIndexExpire > set to "never" and "now" > test $(ls .git/sharedindex.* | wc -l) -le 2 > ' > > +while read -r mode modebits filename; do Style. while read -r mode modebits filename do > + test_expect_success POSIXPERM "split index respects > core.sharedrepository $mode" ' > + git config core.sharedrepository "$mode" && > + : >"$filename" && > + git update-index --add "$filename" && > + echo "$modebits" >expect && > + test_modebits .git/index >actual && > + test_cmp expect actual && > + newest_shared_index=$(ls -t .git/sharedindex.* | head -1) && > + test_modebits "$newest_shared_index" >actual && > + test_cmp expect actual > + ' Running this twice in a loop would create two .git/sharedindex.* files in quick succession. I do not think we want to assume that the filesystem timestamp can keep up with us to allow "ls -t" to work reliably in the second round (if there is a leftover shared index from previous test, even the first round may not catch the latest one). How about doing each iteration this way instead? Which might be a better solution to work around that. - with core.sharedrepository set to false, force the index to be unsplit; "index" will have the default unshared permission bits (but we do not care what it is and no need to check it). - remove any leftover sharedindex.*, if any. - with core.sharedrepository set to whatever mode being tested, do the adding to force split. - test the permission of index file. - test the permission of sharedindex.* file; there should be only one instance, so erroring out when we see two or more is also a good test. The last two steps may look like: test_modebits .git/index >actual && test_cmp expect actual && shared=$(ls .git/sharedindex.*) && case "$shared" in *" "*) # we have more than one??? false ;; *) test_modebits "shared" >actual && test_cmp expect actual ;; esac > +done <<\EOF > +0666 -rw-rw-rw- seventeen > +0642 -rw-r---w- eightteen > +EOF > + > test_done
Re: [PATCH v2 1/3] read-cache: use shared perms when writing shared index
Christian Couder writes:
> Since f6ecc62dbf (write_shared_index(): use tempfile module, 2015-08-10)
> write_shared_index() has been using mks_tempfile() to create the
> temporary file that will become the shared index.
>
> But even before that, it looks like the functions used to create this
> file didn't call adjust_shared_perm(), which means that the shared
> index file has always been created with 600 permissions regardless
> of the shared permission settings.
>
> Because of that, on repositories created with `git init --shared=all`
> and using the split index feature, one gets an error like:
>
> fatal: .git/sharedindex.a52f910b489bc462f187ab572ba0086f7b5157de: index file
> open failed: Permission denied
>
> when another user performs any operation that reads the shared index.
>
> We could use create_tempfile() that calls adjust_shared_perm(), but
> unfortunately create_tempfile() doesn't replace the XX at the end
> of the path it is passed. So let's just call adjust_shared_perm() by
> ourselves.
Because create_tempfile() is not even a viable alternative, the
above sounds just as silly as saying "We could use X, but
unfortunately that X doesn't create a temporary file and return its
file descriptor" with X replaced with any one of about a dozen
functions that happen to call adjust_shared_perm().
Call adjust_shared_perm() on the temporary file created by
mks_tempfile() ourselves to adjust the permission bits.
should be sufficient.
Thanks.
>
> Signed-off-by: Christian Couder
> ---
> read-cache.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index bc156a133e..66f85f8d58 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2425,6 +2425,14 @@ static int write_shared_index(struct index_state
> *istate,
> delete_tempfile(&temporary_sharedindex);
> return ret;
> }
> + ret = adjust_shared_perm(temporary_sharedindex.filename.buf);
> + if (ret) {
> + int save_errno = errno;
> + error("cannot fix permission bits on %s",
> temporary_sharedindex.filename.buf);
> + delete_tempfile(&temporary_sharedindex);
> + errno = save_errno;
> + return ret;
> + }
> ret = rename_tempfile(&temporary_sharedindex,
> git_path("sharedindex.%s",
> sha1_to_hex(si->base->sha1)));
> if (!ret) {
Re: What's cooking in git.git (Jun 2017, #06; Thu, 22)
On Thu, Jun 22 2017, Junio C. Hamano jotted:
> * sb/diff-color-move (2017-06-21) 25 commits
> - diff: document the new --color-moved setting
> - diff.c: add dimming to moved line detection
> - diff.c: color moved lines differently, plain mode
> - diff.c: color moved lines differently
> - diff.c: buffer all output if asked to
> - diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY
> - diff.c: emit_diff_symbol learns about DIFF_SYMBOL_STAT_SEP
> - diff.c: convert word diffing to use emit_diff_symbol
> - diff.c: convert show_stats to use emit_diff_symbol
> - diff.c: convert emit_binary_diff_body to use emit_diff_symbol
> - submodule.c: migrate diff output to use emit_diff_symbol
> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_REWRITE_DIFF
> - diff.c: emit_diff_symbol learns about DIFF_SYMBOL_BINARY_FILES
> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_HEADER
> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_FILEPAIR
> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_CONTEXT_INCOMPLETE
> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_WORDS{_PORCELAIN}
> - diff.c: migrate emit_line_checked to use emit_diff_symbol
> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_NO_LF_EOF
> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_CONTEXT_FRAGINFO
> - diff.c: emit_diff_symbol learns DIFF_SYMBOL_CONTEXT_MARKER
> - diff.c: introduce emit_diff_symbol
> - diff.c: factor out diff_flush_patch_all_file_pairs
> - diff.c: move line ending check into emit_hunk_header
> - diff.c: readability fix
>
> "git diff" has been taught to optionally paint new lines that are
> the same as deleted lines elsewhere differently from genuinely new
> lines.
>
> Is any more update coming?
I guess here's as good a place for feedback is any, this feature's
great, but I discovered some minor warts in it:
This is good:
$ ./git --exec-path=$PWD show --color-moved=crap
fatal: bad --color-moved argument: crap
This is bad:
$ ./git --exec-path=$PWD -c diff.colorMoved=crap show
fatal: unable to parse 'diff.colormoved' from command-line config
Fixed with:
diff --git a/diff.c b/diff.c
index 7cae4f1ddb..036dbc1c3c 100644
--- a/diff.c
+++ b/diff.c
@@ -278,7 +278,7 @@ int git_diff_ui_config(const char *var, const char
*value, void *cb)
if (!strcmp(var, "diff.colormoved")) {
int cm = parse_color_moved(value);
if (cm < 0)
- return -1;
+ die("bad --color-moved argument: %s", value);
diff_color_moved_default = cm;
return 0;
}
But I'm not familiar enough with the code to say if just dying here, as
opposed to returning -1 is OK or not.
Also, I think something like this (very lighty tested) patch on top
makes sense:
diff --git a/diff.c b/diff.c
index 7cae4f1ddb..d195d304d3 100644
--- a/diff.c
+++ b/diff.c
@@ -257,6 +257,15 @@ int git_diff_heuristic_config(const char *var, const
char *value, void *cb)
static int parse_color_moved(const char *arg)
{
+ int v = git_parse_maybe_bool(arg);
+
+ if (v != -1) {
+ if (v == 0)
+ return COLOR_MOVED_NO;
+ else if (v == 1)
+ return COLOR_MOVED_PLAIN;
+ }
+
if (!strcmp(arg, "no"))
return COLOR_MOVED_NO;
else if (!strcmp(arg, "plain"))
I don't want to set this to a specific value, just "true" and it should
pick whatever the default is (and that in the config yields a very bad
error message, hence the first patch).
If you don't want to have a default for whatever reason I think the docs
need to change:
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ab7bdfb49..4b6f8c6d5c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1085,8 +1085,9 @@ This does not affect linkgit:git-format-patch[1] or
the
command line with the `--color[=]` option.
diff.colorMoved::
- If set moved lines in a diff are colored differently,
- for details see '--color-moved' in linkgit:git-diff[1].
+ If set to a valid `` moved lines in a diff are colored
+ differently, for details of valid modes see '--color-moved' in
+ linkgit:git-diff[1].
color.diff.::
Use customized color for diff colorization. `` specifies
Right now the lazy reader (i.e. me) just reads "if set..."
tries it out, and then gets a cryptic error. "If set" to me immediately
sounds like a bool variable (but then I read the diff docs and found
it's not). So with that bool parsing it could be changed to:
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ab7bdfb49..e62d926740 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1085,8 +1085,10 @@ This does not affect linkgit:g
Re: [PATCH v2 1/3] read-cache: use shared perms when writing shared index
Christian Couder writes:
> Since f6ecc62dbf (write_shared_index(): use tempfile module, 2015-08-10)
> write_shared_index() has been using mks_tempfile() to create the
> temporary file that will become the shared index.
>
> But even before that, it looks like the functions used to create this
> file didn't call adjust_shared_perm(), which means that the shared
> index file has always been created with 600 permissions regardless
> of the shared permission settings.
>
> Because of that, on repositories created with `git init --shared=all`
> and using the split index feature, one gets an error like:
>
> fatal: .git/sharedindex.a52f910b489bc462f187ab572ba0086f7b5157de: index file
> open failed: Permission denied
>
> when another user performs any operation that reads the shared index.
>
> We could use create_tempfile() that calls adjust_shared_perm(), but
> unfortunately create_tempfile() doesn't replace the XX at the end
> of the path it is passed. So let's just call adjust_shared_perm() by
> ourselves.
>
> Signed-off-by: Christian Couder
> ---
> read-cache.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index bc156a133e..66f85f8d58 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2425,6 +2425,14 @@ static int write_shared_index(struct index_state
> *istate,
> delete_tempfile(&temporary_sharedindex);
> return ret;
> }
> + ret = adjust_shared_perm(temporary_sharedindex.filename.buf);
Shouldn't we be using the API function get_tempfile_path() for this
instead of reaching into its implementation detail?
> + if (ret) {
> + int save_errno = errno;
> + error("cannot fix permission bits on %s",
> temporary_sharedindex.filename.buf);
> + delete_tempfile(&temporary_sharedindex);
> + errno = save_errno;
> + return ret;
> + }
> ret = rename_tempfile(&temporary_sharedindex,
> git_path("sharedindex.%s",
> sha1_to_hex(si->base->sha1)));
> if (!ret) {
Re: [PATCH v2 00/29] Create a reference backend for packed refs
Jeff King writes: > On Fri, Jun 23, 2017 at 03:01:59PM -0400, Jeff King wrote: > >> On Fri, Jun 23, 2017 at 09:01:18AM +0200, Michael Haggerty wrote: >> >> > * Change patch 17 "packed_ref_store: support iteration" to always >> > iterate over the packed refs using `DO_FOR_EACH_INCLUDE_BROKEN`. >> > This switches off the check in the packed-ref iterator of whether a >> > reference is broken. This is now checked only in >> > `files_ref_iterator_advance()`, after the packed and loose >> > references have been merged together. It also saves some work. >> >> I'm curious why you prefer this solution to just removing the code >> entirely. Wouldn't it be an error to call the packed ref iterator >> without INCLUDE_BROKEN? The "entries may not be valid" thing is a >> property of the packed-refs concept itself, not a particular caller's >> view of it. > > Speculating on my own question. I guess it would prepare us for a day > when a possible ref store is to use a packed-refs _without_ loose refs. > IOW, the property is defined on packed-refs today, but a possible future > direction would be to use it by itself. But maybe I'm just making things > up. OK. In other words, it's not a packed-refs's characteristics that cruft are allowed. It's that a ref storage that is implemented as an overlay of one storage (which happens to be the loose one) on top of another (which happens to be the packed refs file) allows the latter one to have cruft if (and only if) that broken one is covered by the former one. > At any rate, I've read through the whole series and dropped a few > comments in there. Overall it looks good. If I had one complaint, it's > that the individual commits all look obviously correct but it is hard to > judge whether the bigger steps they are taking are the right thing. I > mostly have faith in you, as I know that your end goal is a good one, > and that you're very familiar with this code. But just something I > noticed while reviewing. Thanks.
Re: [showing-off RFC/PATCH 26/26] diff.c: have a "machine parseable" move coloring
On Tue, Jun 20 2017, Stefan Beller jotted:
> + Ævar, who was not part of the email where I copied all recipients
> from for this series.
I played around with this a bit, it would be great to have something
like this on top of --color-moved eventually. It's a lot easier to /^_
in a pager than looking for color codes.
Of course it's diff-incompatible output, but it's fair enough to have
that hidden behind some option IMO.
> On Mon, Jun 19, 2017 at 7:48 PM, Stefan Beller wrote:
>> Ævar asked for it, this is how you would do it.
>> (plus documentation, tests, CLI knobs, options)
>>
>> Signed-off-by: Stefan Beller
>> ---
>> diff.c | 15 +++
>> diff.h | 2 ++
>> 2 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/diff.c b/diff.c
>> index 7756f7610c..61caa057ff 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -997,6 +997,7 @@ static void emit_diff_symbol_from_struct(struct
>> diff_options *o,
>> static const char *nneof = " No newline at end of file\n";
>> const char *context, *reset, *set, *meta, *fraginfo;
>> struct strbuf sb = STRBUF_INIT;
>> + int sign;
>>
>> enum diff_symbol s = eds->s;
>> const char *line = eds->line;
>> @@ -1058,8 +1059,11 @@ static void emit_diff_symbol_from_struct(struct
>> diff_options *o,
>> default:
>> set = diff_get_color_opt(o, DIFF_FILE_NEW);
>> }
>> + sign = '+';
>> + if (flags & DIFF_SYMBOL_MOVED_LINE &&
>> o->machine_readable_moves)
>> + sign = '*';
>> reset = diff_get_color_opt(o, DIFF_RESET);
>> - emit_line_ws_markup(o, set, reset, line, len, '+',
>> + emit_line_ws_markup(o, set, reset, line, len, sign,
>> flags & DIFF_SYMBOL_CONTENT_WS_MASK,
>> flags &
>> DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
>> break;
>> @@ -1086,8 +1090,11 @@ static void emit_diff_symbol_from_struct(struct
>> diff_options *o,
>> default:
>> set = diff_get_color_opt(o, DIFF_FILE_OLD);
>> }
>> + sign = '-';
>> + if (flags & DIFF_SYMBOL_MOVED_LINE &&
>> o->machine_readable_moves)
>> + sign = '_';
>> reset = diff_get_color_opt(o, DIFF_RESET);
>> - emit_line_ws_markup(o, set, reset, line, len, '-',
>> + emit_line_ws_markup(o, set, reset, line, len, sign,
>> flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
>> break;
>> case DIFF_SYMBOL_WORDS_PORCELAIN:
>> @@ -5475,7 +5482,7 @@ static void diff_flush_patch_all_file_pairs(struct
>> diff_options *o)
>> static struct emitted_diff_symbols esm = EMITTED_DIFF_SYMBOLS_INIT;
>> struct diff_queue_struct *q = &diff_queued_diff;
>>
>> - if (o->color_moved)
>> + if (o->color_moved || o->machine_readable_moves)
>> o->emitted_symbols = &esm;
>>
>> for (i = 0; i < q->nr; i++) {
>> @@ -5485,7 +5492,7 @@ static void diff_flush_patch_all_file_pairs(struct
>> diff_options *o)
>> }
>>
>> if (o->emitted_symbols) {
>> - if (o->color_moved) {
>> + if (o->color_moved || o->machine_readable_moves) {
>> struct hashmap add_lines, del_lines;
>> unsigned ignore_ws = DIFF_XDL_TST(o,
>> IGNORE_WHITESPACE);
>>
>> diff --git a/diff.h b/diff.h
>> index 98abd75521..b6c4d7ab0f 100644
>> --- a/diff.h
>> +++ b/diff.h
>> @@ -194,6 +194,8 @@ struct diff_options {
>> COLOR_MOVED_ZEBRA = 2,
>> COLOR_MOVED_ZEBRA_DIM = 3,
>> } color_moved;
>> +
>> + int machine_readable_moves;
>> };
>>
>> void diff_emit_submodule_del(struct diff_options *o, const char *line);
>> --
>> 2.12.2.575.gb14f27f917
>>
Re: [RFC PATCH] proposal for refs/tracking/* hierarchy
On Fri, Jun 23, 2017 at 1:54 PM, Junio C Hamano wrote: > Jacob Keller writes: > >> Instead, lets add support for a new refs/tracking/* hierarchy which is >> laid out in such a way to avoid this inconsistency. All refs in >> "refs/tracking//*" will include the complete ref, such that >> dropping the "tracking/" part will give the exact ref name as it >> is found in the upstream. Thus, we can track any ref here by simply >> fetching it into refs/tracking//*. > > I do think this overall is a good goal to aim for wrt "tracking", > i.e. keeping a pristine copy of what we observed from the outside > world. In addition to what you listed as "undecided" below, > however, I expect that this will highlight a lot of trouble in > "working together", i.e. reconciling the differences of various > world views and moving the project forward, in two orthogonal axes: > I agree, I think this is definitely a problem that we'd want/have to solve. > - Things pointed at by some refs (e.g. notes/, but I think the >".gitmodules equivalent that is not tied to any particular commit >in the superproject" Jonathan Nieder and friends advocate falls >into the same category) do not correspond to the working tree >contents, and merging their contents will always pose challenge >when we need to prepare for conflict resolution. This is part of why I started by looking at notes, since there is already some code for notes merge, it just doesn't have a standard place to store "remote" notes. I've wanted that ability to enable more ease of sharing. > > - Things pointed at by some other refs (e.g. tags/) do not make >sense to be merged. You may locally call a commit with a tag >"wip", while your friends may use the same "wip" tag to point at >a different one. Both are valid, and more importantly, there is >no point even trying to reconcile what the "wip" tag means in the >project globally. > > For the former, I expect that merging these non-working tree > contents will all have to require some specific tool that is > tailored for the meaning each hierarchy has, just like "git notes > merge" gives a solution that is very specific to the notes refs (I > do not know how well "notes merge" works in practice, though). Agreed. That's also part of why I'm looking at doing it as a partial "one hierarchy at a time" proposal, I've got a somewhat better format for that coming Soon(TM) > > For the latter, having a separate tracking hierarchy is a strict > improvement from "tracking" point of view, but I think "working > together" also needs a well designed set of new look-up rules, and a > new convention. For example, some tags may not want to be shared > (e.g. "wip" example above) even though they should be easy to access > by those who already have them (e.g. "git log ..wip" should work > without having to say "git log ..refs/local-tags/wip", even if we > decide to implement the "some tags may not want to be shared" > segregation by introducing a new hierarchy). And also a shared tag > that is copied to "refs/tracking/origin/tags/v1.0" would need a way > better than "git log tracking/origin/tags/v1.0" for this mechanism > to be useful in everyday workflow. > > Thanks. > My thoughts on tags are sort of the following: keep the default semantics of tags get copied into our local refs/tags, (if we're fetching them). However, create some mechanism for easily showing when you have a local tag with the same name as a remote tag which don't point at the same thing, by using the refs/tracking. Possibly, any time you grab a tag, check and see if the remote version is different and "do something" like complain, or show both, or give some advice (possibly with a new tool to "pull in the remote tag if you verify it's accurate, etc". This way, you generally work with tags the same way but get added protection for knowing when your local tags differ from what remotes show? Thanks, Jake
Re: [PATCH v2 19/29] packed-backend: new module for handling packed references
Jeff King writes: > On Fri, Jun 23, 2017 at 09:01:37AM +0200, Michael Haggerty wrote: > >> Now that the interface between `files_ref_store` and >> `packed_ref_store` is relatively narrow, move the latter into a new >> module, "refs/packed-backend.h" and "refs/packed-backend.c". It still >> doesn't quite implement the `ref_store` interface, but it will soon. >> >> This commit moves code around and adjusts its visibility, but doesn't >> change anything. > > Looks good. Stefan will be happy to know that I used --color-moved to > look at it. ;) This patch shows OK, but when applied to other changes in the series, e.g. "packed_ref_store: make class into a subclass of `ref_store`", it was somewhat irritating that all new blank lines are shown as a copy-move of a single deleted blank line (also a single "return refs" in an entirely new function being a copy/move looked confusing).
Re: [PATCH] pathspec: die on empty strings as pathspec
Torsten Bögershausen writes: > On 23/06/17 20:04, Junio C Hamano wrote: >... >> At this point in the test sequence, there is no modified path that >> need to be further added before committing; the working tree is >> empty except for .gitattributes which was just added to the index. >> So we could instead pass no pathspec, but this is a conversion more >> faithful to the original. >> >> Signed-off-by: Junio C Hamano >> --- >> t/t0027-auto-crlf.sh | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh >> index 93725895a4..e41c9b3bb2 100755 >> --- a/t/t0027-auto-crlf.sh >> +++ b/t/t0027-auto-crlf.sh >> @@ -322,7 +322,7 @@ test_expect_success 'setup master' ' >> echo >.gitattributes && >> git checkout -b master && >> git add .gitattributes && >> -git commit -m "add .gitattributes" "" && >> +git commit -m "add .gitattributes" . && > > Reading the context here, there shouldn't be a "pathspec" at all, > as .gitattributes had been added just one line above. > > The line should have been from the very beginning: > git commit -m "add .gitattributes" && > > # What do you think ? See above ;-) And to be more explicit, it is not a fault of your earlier change, either, that we didn't notice this problem since we started "warning". We need to see if we can update our test framework to help us better; I really think the test framework could and should have helped us to notice this soon after we went into "keep behaving as before but warn" stage.
Re: [RFC PATCH] proposal for refs/tracking/* hierarchy
Jacob Keller writes: > Instead, lets add support for a new refs/tracking/* hierarchy which is > laid out in such a way to avoid this inconsistency. All refs in > "refs/tracking//*" will include the complete ref, such that > dropping the "tracking/" part will give the exact ref name as it > is found in the upstream. Thus, we can track any ref here by simply > fetching it into refs/tracking//*. I do think this overall is a good goal to aim for wrt "tracking", i.e. keeping a pristine copy of what we observed from the outside world. In addition to what you listed as "undecided" below, however, I expect that this will highlight a lot of trouble in "working together", i.e. reconciling the differences of various world views and moving the project forward, in two orthogonal axes: - Things pointed at by some refs (e.g. notes/, but I think the ".gitmodules equivalent that is not tied to any particular commit in the superproject" Jonathan Nieder and friends advocate falls into the same category) do not correspond to the working tree contents, and merging their contents will always pose challenge when we need to prepare for conflict resolution. - Things pointed at by some other refs (e.g. tags/) do not make sense to be merged. You may locally call a commit with a tag "wip", while your friends may use the same "wip" tag to point at a different one. Both are valid, and more importantly, there is no point even trying to reconcile what the "wip" tag means in the project globally. For the former, I expect that merging these non-working tree contents will all have to require some specific tool that is tailored for the meaning each hierarchy has, just like "git notes merge" gives a solution that is very specific to the notes refs (I do not know how well "notes merge" works in practice, though). For the latter, having a separate tracking hierarchy is a strict improvement from "tracking" point of view, but I think "working together" also needs a well designed set of new look-up rules, and a new convention. For example, some tags may not want to be shared (e.g. "wip" example above) even though they should be easy to access by those who already have them (e.g. "git log ..wip" should work without having to say "git log ..refs/local-tags/wip", even if we decide to implement the "some tags may not want to be shared" segregation by introducing a new hierarchy). And also a shared tag that is copied to "refs/tracking/origin/tags/v1.0" would need a way better than "git log tracking/origin/tags/v1.0" for this mechanism to be useful in everyday workflow. Thanks.
Re: [PATCH] pathspec: die on empty strings as pathspec
On 23/06/17 20:04, Junio C Hamano wrote: Junio C Hamano writes: Sorry for my mess, see below I am not sure if we even want the dot there, but at least that is what the original author of the test intended to do when s/he decided to pass an empty string as the pathspec. t/t0027-auto-crlf.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) I'll queue the following _before_ your "final step", so that the whole thing can be advanced to 'next' and hopefully to 'master' in some future releases. Given that we ourselves did not even notice until now that one of our scripts get broken by this "final step", even though we kept issuing the warning message, we may want to re-think our overall strategy for deprecation. We've assumed that "keep behaving as before but warn for a few years and then finally give a hard failure" would be sufficient, but depending on how the script that employ our programs hide the warnings from the end users, that may not be a good enough transition strategy. At the same time we re-think the deprecation strategy, we also need to see if we can update our test framework to help us better. Ideally, we could have caught this existing breakage of passing "" as pathspec in the test soon after we went into "keep behaving as before but warn" stage. We didn't and found it out only when we switched to "finally give a hard failure" stage. That is very unsatisfactory. Needless to say, neither of the above two comes from _your_ change. It's just something we need to improve in a larger scope of the whole project I realized. Thanks. -- >8 -- Subject: [PATCH] t0027: do not use an empty string as a pathspec element In an upcoming update, we will finally make an empty string illegal as an element in a pathspec; it traditionally meant the same as ".", i.e. include everything, so update this test that passes "" to pass a dot instead. At this point in the test sequence, there is no modified path that need to be further added before committing; the working tree is empty except for .gitattributes which was just added to the index. So we could instead pass no pathspec, but this is a conversion more faithful to the original. Signed-off-by: Junio C Hamano --- t/t0027-auto-crlf.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh index 93725895a4..e41c9b3bb2 100755 --- a/t/t0027-auto-crlf.sh +++ b/t/t0027-auto-crlf.sh @@ -322,7 +322,7 @@ test_expect_success 'setup master' ' echo >.gitattributes && git checkout -b master && git add .gitattributes && - git commit -m "add .gitattributes" "" && + git commit -m "add .gitattributes" . && Reading the context here, there shouldn't be a "pathspec" at all, as .gitattributes had been added just one line above. The line should have been from the very beginning: git commit -m "add .gitattributes" && # What do you think ? printf "\$Id: \$\nLINEONE\nLINETWO\nLINETHREE" >LF && printf "\$Id: \$\r\nLINEONE\r\nLINETWO\r\nLINETHREE" >CRLF && printf "\$Id: \$\nLINEONE\r\nLINETWO\nLINETHREE" >CRLF_mix_LF &&
Re: [PATCH] xdiff: trim common tail with -U0 after diff
Am 23.06.2017 um 12:36 schrieb Daniel Hahler: When -U0 is used, trim_common_tail should be called after `xdl_diff`, so that `--indent-heuristic` (and other diff processing) works as expected. It also removes the check for `!(xecfg->flags & XDL_EMIT_FUNCCONTEXT)` added in e0876bca4, which does not appear to be necessary anymore after moving the trimming down. This only adds a test to t4061-diff-indent.sh, but should also have one for normal (i.e. non-experimental) diff mode probably?! Ref: https://github.com/tomtom/quickfixsigns_vim/issues/74#issue-237900460 --- t/t4061-diff-indent.sh | 15 +++ xdiff-interface.c | 7 --- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh index 2affd7a10..df3151393 100755 --- a/t/t4061-diff-indent.sh +++ b/t/t4061-diff-indent.sh @@ -116,6 +116,16 @@ test_expect_success 'prepare' ' 4 EOF + cat <<-\EOF >spaces-compacted-U0-expect && + diff --git a/spaces.txt b/spaces.txt + --- a/spaces.txt + +++ b/spaces.txt + @@ -4,0 +5,3 @@ a + +b + +a + + + EOF + tr "_" " " <<-\EOF >functions-expect && diff --git a/functions.c b/functions.c --- a/functions.c @@ -184,6 +194,11 @@ test_expect_success 'diff: --indent-heuristic with --histogram' ' compare_diff spaces-compacted-expect out-compacted4 ' +test_expect_success 'diff: --indent-heuristic with -U0' ' + git diff -U0 --indent-heuristic old new -- spaces.txt >out-compacted5 && + compare_diff spaces-compacted-U0-expect out-compacted5 +' + test_expect_success 'diff: ugly functions' ' git diff --no-indent-heuristic old new -- functions.c >out && compare_diff functions-expect out The changed test script passes just fine for me even without your change to xdiff-interface.c, which is odd. Do you have another way to demonstrate the unexpected behavior? And can someone replicate the failure? René
Re: [PATCH] submodule--helper: teach push-check to handle HEAD
On Fri, Jun 23, 2017 at 1:04 PM, Brandon Williams wrote: > In 06bf4ad1d (push: propagate remote and refspec with > --recurse-submodules) push was taught how to propagate a refspec down to > submodules when the '--recurse-submodules' flag is given. The only refspecs > that are allowed to be propagated are ones which name a ref which exists > in both the superproject and the submodule, with the caveat that 'HEAD' > was disallowed. > > This patch teaches push-check (the submodule helper which determines if > a refspec can be propagated to a submodule) to permit propagating 'HEAD' > if and only if the superproject and the submodule both have the same > named branch checked out and the submodule is not in a detached head > state. cont'd: We need this use case because Gerrit's documentation ingrains this workflow in its users to use git push HEAD:refs/for/ And when both the submodule as well as the superproject are still on a branch with the same name (and not detached) we'd not be misunderstood by the user on the syntax. More on the code later.
Re: [PATCH v2 00/29] Create a reference backend for packed refs
Jeff King writes: > On Fri, Jun 23, 2017 at 09:01:18AM +0200, Michael Haggerty wrote: > >> * Change patch 17 "packed_ref_store: support iteration" to always >> iterate over the packed refs using `DO_FOR_EACH_INCLUDE_BROKEN`. >> This switches off the check in the packed-ref iterator of whether a >> reference is broken. This is now checked only in >> `files_ref_iterator_advance()`, after the packed and loose >> references have been merged together. It also saves some work. > > I'm curious why you prefer this solution to just removing the code > entirely. Wouldn't it be an error to call the packed ref iterator > without INCLUDE_BROKEN? The "entries may not be valid" thing is a > property of the packed-refs concept itself, not a particular caller's > view of it. Thanks for pointing it out. I was wondering about the same thing and you phrased it a lot more succinctly than the draft I was writing.
Re: --color-moved feedback, was Re: [PATCH v2 19/29] packed-backend: new module for handling packed references
On Fri, Jun 23, 2017 at 1:10 PM, Jeff King wrote: > [I culled the cc list, as it was big and they don't all necessarily care > about this feature] Yeah I was on the verge to do that, but did not pull through. > > In the end, I just did --color-moved=plain, ... > "yep, this is all a giant moved chunk, so I don't have to look carefully > at it". This is dangerous, as "plain" does not care about permutations. See the 7f5af90798 (diff.c: color moved lines differently, 2017-06-19) for details. You would want at least "zebra", which would show you permutations. > I'm not sure what the default mode is. When I first used it it seemed to > italicize a bunch of text. Which I didn't even notice at first, so I > thought it was broken. ok, italics does not seem to be popular. I liked it as I could take the same color for these uninteresting lines. > I didn't try the "dim" feature; I don't think urxvt supports that > attribute (though maybe it was what was doing the italics?). That is on by default (with italics), maybe the default should rather be "zebra" as that has fewer colors but still contains all the information needed to be sure about "yep, this is all a giant moved chunk, so I don't have to look carefully at it". > > One minor quibble is that I used with "git log", so I was looking at all > the commits with that coloring. But the ones that didn't have movement > had a lot of noisy "moved" sections. E.g., one line moving here or > there, or even blank lines. It might be worth having some heuristics to > identify a chunk (I think I saw some discussion on that earlier, and > maybe it's even there and I didn't see it). It is not there, yet. It was Michaels suggestions. I'll give that a try, too. >> Let's take this patch as an example, if someone were to find a bug >> in one of the moved functions, they would send a fix based on the >> function in refs/files-backend.c, such that it can easily be merged down >> to maint, but when merging it forward with this, it may clash. > > That feels more dangerous to me, just because the heuristics seem to > find a lot of "moves" of repeated code. Imagine a simple patch like > "switch return 0 to return -1". If the code you're touching went away, > there's a very good chance that another "return 0" popped up somewhere > else in the code base. A conflict is what you want there; silently > changing some other site would be not only wrong, but quite subtle and > hard to find. I agree, that is the danger, but the scale of danger is the size of the moved chunk. A file is usually a very large chunk, such that it is obviously a good idea to fix that. Maybe we'd introduce a threshold, that the fix must not be in range of the block boundaries of say 3 lines. (Then the moved block must be at least 7 lines of code such that a one liner fix in the middle would kick in) Thanks, Stefan
Re: [PATCH v2 01/29] t1408: add a test of stale packed refs covered by loose refs
Jeff King writes: > On Fri, Jun 23, 2017 at 09:01:19AM +0200, Michael Haggerty wrote: > >> +test_expect_success setup ' >> +test_tick && >> +git commit --allow-empty -m one && >> +one=$(git rev-parse HEAD) && >> +git for-each-ref >actual && >> +echo "$one commit refs/heads/master" >expect && >> +test_cmp expect actual && >> + >> +git pack-refs --all && >> +git for-each-ref >actual && >> +echo "$one commit refs/heads/master" >expect && >> +test_cmp expect actual && >> + >> +cat .git/packed-refs && > > I think we'd usually drop debugging "cat"s like these in the name of > keeping the process count down. Unless they really are intended to > confirm that .git/packed-refs exists (although test_path_is_file is a > less expensive way of checking that). > > That's a minor nit, though. Sorry, these two cat's were what I used while debugging the test and should be dropped. Will remove while queuing. Thanks for spotting.
Re: [PATCHv2 04/25] diff.c: introduce emit_diff_symbol
On Fri, Jun 23, 2017 at 1:07 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> diff --git a/new.txt b/new.txt >> index fa69b07..412428c 100644 >> Binary files a/new.txt and b/new.txt differ >> >> could be buffered as >> DIFF_SYMBOL_DIFF_START + new.txt >> DIFF_SYMBOL_INDEX_MODE + fa69b07 412428c "non-executable" flag >> DIFF_SYMBOL_BINARY_FILES + new.txt > > Hopefully this is an oversimplified example and I'd assume that you > will do the right thing when showing a renamed filepair by allowing > both old and new paths that could be different? Yes, this is simplified to drive the point of having as little data as possible, so I thought we might have a flag that says "same file name" instead of giving the file name twice. We do not do such an optimization yet.
--color-moved feedback, was Re: [PATCH v2 19/29] packed-backend: new module for handling packed references
[I culled the cc list, as it was big and they don't all necessarily care about this feature] On Fri, Jun 23, 2017 at 12:46:47PM -0700, Stefan Beller wrote: > > Looks good. Stefan will be happy to know that I used --color-moved to > > look at it. ;) > > Hah! > > As a follow up on that, let's perform a user survey :-P > * How was your review experience? > * Were you annoyed by the default colors or mode? > (That is best expressed as a patch, as it seems like >bikeshedding to me, but I am far from being a UX expert ;) I'm not sure what the default mode is. When I first used it it seemed to italicize a bunch of text. Which I didn't even notice at first, so I thought it was broken. In the end, I just did --color-moved=plain, which seemed to color with magenta/cyan. I found that a bit loud and switched to "red 80" and "green 80" (i.e., a dark grey background, as I usually have a black one). I probably would have picked a darker grey, but I use urxvt and it doesn't seem to do 24-bit color codes. I didn't try the "dim" feature; I don't think urxvt supports that attribute (though maybe it was what was doing the italics?). Anyway, it worked fine after that. My main use for it was just seeing "yep, this is all a giant moved chunk, so I don't have to look carefully at it". One minor quibble is that I used with "git log", so I was looking at all the commits with that coloring. But the ones that didn't have movement had a lot of noisy "moved" sections. E.g., one line moving here or there, or even blank lines. It might be worth having some heuristics to identify a chunk (I think I saw some discussion on that earlier, and maybe it's even there and I didn't see it). > Just today I thought about that further: > While reviewing is one thing (which I do a lot), how can we make this > work with merging changes? > > I think the file rename detection is acknowledged by the merge code, > such that a plain file rename and a patch to said file is easy on the > maintainer, but we would want that for smaller code movements, too. > > Let's take this patch as an example, if someone were to find a bug > in one of the moved functions, they would send a fix based on the > function in refs/files-backend.c, such that it can easily be merged down > to maint, but when merging it forward with this, it may clash. That feels more dangerous to me, just because the heuristics seem to find a lot of "moves" of repeated code. Imagine a simple patch like "switch return 0 to return -1". If the code you're touching went away, there's a very good chance that another "return 0" popped up somewhere else in the code base. A conflict is what you want there; silently changing some other site would be not only wrong, but quite subtle and hard to find. -Peff
Re: [PATCHv2 04/25] diff.c: introduce emit_diff_symbol
Stefan Beller writes: > diff --git a/new.txt b/new.txt > index fa69b07..412428c 100644 > Binary files a/new.txt and b/new.txt differ > > could be buffered as > DIFF_SYMBOL_DIFF_START + new.txt > DIFF_SYMBOL_INDEX_MODE + fa69b07 412428c "non-executable" flag > DIFF_SYMBOL_BINARY_FILES + new.txt Hopefully this is an oversimplified example and I'd assume that you will do the right thing when showing a renamed filepair by allowing both old and new paths that could be different?
[PATCH] submodule--helper: teach push-check to handle HEAD
In 06bf4ad1d (push: propagate remote and refspec with
--recurse-submodules) push was taught how to propagate a refspec down to
submodules when the '--recurse-submodules' flag is given. The only refspecs
that are allowed to be propagated are ones which name a ref which exists
in both the superproject and the submodule, with the caveat that 'HEAD'
was disallowed.
This patch teaches push-check (the submodule helper which determines if
a refspec can be propagated to a submodule) to permit propagating 'HEAD'
if and only if the superproject and the submodule both have the same
named branch checked out and the submodule is not in a detached head
state.
Signed-off-by: Brandon Williams
---
builtin/submodule--helper.c| 57 +++---
submodule.c| 18 ++---
t/t5531-deep-submodule-push.sh | 25 +-
3 files changed, 82 insertions(+), 18 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1b4d2b346..fd5020036 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1107,24 +1107,41 @@ static int resolve_remote_submodule_branch(int argc,
const char **argv,
static int push_check(int argc, const char **argv, const char *prefix)
{
struct remote *remote;
+ const char *superproject_head;
+ char *head;
+ int detached_head = 0;
+ struct object_id head_oid;
- if (argc < 2)
- die("submodule--helper push-check requires at least 1
argument");
+ if (argc < 3)
+ die("submodule--helper push-check requires at least 2
argument");
+
+ /*
+* superproject's resolved head ref.
+* if HEAD then the superproject is in a detached head state, otherwise
+* it will be the resolved head ref.
+*/
+ superproject_head = argv[1];
+ /* Get the submodule's head ref and determine if it is detached */
+ head = resolve_refdup("HEAD", 0, head_oid.hash, NULL);
+ if (!head)
+ die(_("Failed to resolve HEAD as a valid ref."));
+ if (!strcmp(head, "HEAD"))
+ detached_head = 1;
/*
* The remote must be configured.
* This is to avoid pushing to the exact same URL as the parent.
*/
- remote = pushremote_get(argv[1]);
+ remote = pushremote_get(argv[2]);
if (!remote || remote->origin == REMOTE_UNCONFIGURED)
- die("remote '%s' not configured", argv[1]);
+ die("remote '%s' not configured", argv[2]);
/* Check the refspec */
- if (argc > 2) {
- int i, refspec_nr = argc - 2;
+ if (argc > 3) {
+ int i, refspec_nr = argc - 3;
struct ref *local_refs = get_local_heads();
struct refspec *refspec = parse_push_refspec(refspec_nr,
-argv + 2);
+argv + 3);
for (i = 0; i < refspec_nr; i++) {
struct refspec *rs = refspec + i;
@@ -1132,18 +1149,30 @@ static int push_check(int argc, const char **argv,
const char *prefix)
if (rs->pattern || rs->matching)
continue;
- /*
-* LHS must match a single ref
-* NEEDSWORK: add logic to special case 'HEAD' once
-* working with submodules in a detached head state
-* ceases to be the norm.
-*/
- if (count_refspec_match(rs->src, local_refs, NULL) != 1)
+ /* LHS must match a single ref */
+ switch(count_refspec_match(rs->src, local_refs, NULL)) {
+ case 1:
+ break;
+ case 0:
+ /*
+* If LHS matches 'HEAD' then we need to ensure
+* that it matches the same named branch
+* checked out in the superproject.
+*/
+ if (!strcmp(rs->src, "HEAD")) {
+ if (!detached_head &&
+ !strcmp(head, superproject_head))
+ break;
+ die("HEAD does not match the named
branch in the superproject");
+ }
+ default:
die("src refspec '%s' must name a ref",
rs->src);
+ }
}
free_refspec(refspec_nr, refspec);
}
+ free(head);
return 0;
}
diff --git a/submodule.c b/submodule.
Re: [PATCH v2 00/29] Create a reference backend for packed refs
On Fri, Jun 23, 2017 at 03:01:59PM -0400, Jeff King wrote: > On Fri, Jun 23, 2017 at 09:01:18AM +0200, Michael Haggerty wrote: > > > * Change patch 17 "packed_ref_store: support iteration" to always > > iterate over the packed refs using `DO_FOR_EACH_INCLUDE_BROKEN`. > > This switches off the check in the packed-ref iterator of whether a > > reference is broken. This is now checked only in > > `files_ref_iterator_advance()`, after the packed and loose > > references have been merged together. It also saves some work. > > I'm curious why you prefer this solution to just removing the code > entirely. Wouldn't it be an error to call the packed ref iterator > without INCLUDE_BROKEN? The "entries may not be valid" thing is a > property of the packed-refs concept itself, not a particular caller's > view of it. Speculating on my own question. I guess it would prepare us for a day when a possible ref store is to use a packed-refs _without_ loose refs. IOW, the property is defined on packed-refs today, but a possible future direction would be to use it by itself. But maybe I'm just making things up. At any rate, I've read through the whole series and dropped a few comments in there. Overall it looks good. If I had one complaint, it's that the individual commits all look obviously correct but it is hard to judge whether the bigger steps they are taking are the right thing. I mostly have faith in you, as I know that your end goal is a good one, and that you're very familiar with this code. But just something I noticed while reviewing. -Peff
Re: [PATCH v2 29/29] read_packed_refs(): die if `packed-refs` contains bogus data
On Fri, Jun 23, 2017 at 09:01:47AM +0200, Michael Haggerty wrote: > The old code ignored any lines that it didn't understand. This is > dangerous. Instead, `die()` if the `packed-refs` file contains any > lines that we don't know how to handle. This seems like a big improvement. Is it worth adding a test for a corrupted file? I assume this isn't something you saw in the wild, but just a deficiency you noticed while reading the code. I suspect this laxness may have been what allowed us to add the optional peeled values long ago. But I think I'd rather see us be more strict and notice corruption or nonsense rather than quietly ignoring (especially because an operation like "git pack-refs" would then overwrite it, dropping whatever entries we didn't understand). -Peff
Re: [PATCH v2 28/29] repack_without_refs(): don't lock or unlock the packed refs
On Fri, Jun 23, 2017 at 09:01:46AM +0200, Michael Haggerty wrote: > Change `repack_without_refs()` to expect the packed-refs lock to be > held already, and not to release the lock before returning. Change the > callers to deal with lock management. > > This change makes it possible for callers to hold the packed-refs lock > for a longer span of time, a possibility that will eventually make it > possible to fix some longstanding races. This is the sort of clue I was thinking about in my last email. :) > The only semantic change here is that `repack_without_refs()` used to > forgot to release the lock in the `if (!removed)` exit path. That > omission is now fixed. s/used to forgot/previously forgot/ or similar? > @@ -731,14 +717,12 @@ int repack_without_refs(struct ref_store *ref_store, >* All packed entries disappeared while we were >* acquiring the lock. >*/ > - rollback_packed_refs(refs); > + clear_packed_ref_cache(refs); > return 0; And this is the reason for the earlier "you should be able to clear the packed ref cache without holding the lock" commit, I guess. Makes sense. -Peff
Re: [PATCH v2 27/29] commit_packed_refs(): remove call to `packed_refs_unlock()`
On Fri, Jun 23, 2017 at 09:01:45AM +0200, Michael Haggerty wrote: > Instead, change the callers of `commit_packed_refs()` to call > `packed_refs_unlock()`. Why? I can guess that the reason is probably "because we're going to add a new caller that doesn't want to do that", but it's nice to hear even that. Those kinds of clues help make a more coherent narrative when you're reading through 29 refactoring patches. ;) -Peff
Re: [RFC/PATCH v4 09/49] Add initial external odb support
On 6/20/2017 3:54 AM, Christian Couder wrote:
From: Jeff King
Signed-off-by: Christian Couder
I'd suggest you make the function names consistent with the capabilities
flags (ie get, put, have) both here in odb_helper.c/h and in
external_odb.c/h.
+int odb_helper_has_object(struct odb_helper *o, const unsigned char *sha1);
+int odb_helper_fetch_object(struct odb_helper *o, const unsigned char *sha1,
+ int fd);
+
+#endif /* ODB_HELPER_H */
The following patch fixes a few compiler warnings/errors.
diff --git a/odb-helper.c b/odb-helper.c
index 01cd6a713c..ffbbd2fc87 100644
--- a/odb-helper.c
+++ b/odb-helper.c
@@ -118,7 +118,7 @@ static int check_object_process_error(int err,
unsigned int capability)
{
if (!err)
- return;
+ return 0;
if (!strcmp(status, "error")) {
/* The process signaled a problem with the file. */
@@ -192,7 +192,7 @@ static ssize_t
read_packetized_plain_object_to_fd(struct odb_helper *o,
git_SHA1_Init(&hash);
/* First header.. */
- hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", typename(type),
size) + 1;
+ hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %"PRIuMAX,
typename(type), size) + 1;
stream.next_in = (unsigned char *)hdr;
stream.avail_in = hdrlen;
while (git_deflate(&stream, 0) == Z_OK)
@@ -253,7 +253,7 @@ static ssize_t
read_packetized_plain_object_to_fd(struct odb_helper *o,
return -1;
}
if (total_got != size) {
- warning("size mismatch from odb helper '%s' for %s (%lu
!= %lu)",
+ warning("size mismatch from odb helper '%s' for %s (%lu
!= %"PRIuMAX")",
o->name, sha1_to_hex(sha1), total_got, size);
return -1;
}
@@ -587,7 +587,6 @@ static int have_object_process(struct odb_helper *o)
struct strbuf status = STRBUF_INIT;
const char *cmd = o->cmd;
uint64_t start;
- char *line;
int packet_len;
int total_got = 0;
@@ -946,7 +945,7 @@ int odb_helper_for_each_object(struct odb_helper *o,
return 0;
}
-int odb_helper_write_plain_object(struct odb_helper *o,
+static int odb_helper_write_plain_object(struct odb_helper *o,
const void *buf, size_t len,
const char *type, unsigned char *sha1)
{
Re: [PATCH v2 26/29] clear_packed_ref_cache(): don't protest if the lock is held
On Fri, Jun 23, 2017 at 09:01:44AM +0200, Michael Haggerty wrote: > The existing callers already check that the lock isn't held just > before calling `clear_packed_ref_cache()`, and in the near future we > want to be able to call this function when the lock is held. OK. It's not immediately obvious that this is true, because some of the relationships are a bit buried, but I double-checked and I think it is the case. -Peff
Re: [PATCH v2 19/29] packed-backend: new module for handling packed references
On Fri, Jun 23, 2017 at 12:35 PM, Jeff King wrote: > On Fri, Jun 23, 2017 at 09:01:37AM +0200, Michael Haggerty wrote: > >> Now that the interface between `files_ref_store` and >> `packed_ref_store` is relatively narrow, move the latter into a new >> module, "refs/packed-backend.h" and "refs/packed-backend.c". It still >> doesn't quite implement the `ref_store` interface, but it will soon. >> >> This commit moves code around and adjusts its visibility, but doesn't >> change anything. > > Looks good. Stefan will be happy to know that I used --color-moved to > look at it. ;) Hah! As a follow up on that, let's perform a user survey :-P * How was your review experience? * Were you annoyed by the default colors or mode? (That is best expressed as a patch, as it seems like bikeshedding to me, but I am far from being a UX expert ;) Just today I thought about that further: While reviewing is one thing (which I do a lot), how can we make this work with merging changes? I think the file rename detection is acknowledged by the merge code, such that a plain file rename and a patch to said file is easy on the maintainer, but we would want that for smaller code movements, too. Let's take this patch as an example, if someone were to find a bug in one of the moved functions, they would send a fix based on the function in refs/files-backend.c, such that it can easily be merged down to maint, but when merging it forward with this, it may clash.
Re: [PATCH v2 22/29] commit_packed_refs(): use a staging file separate from the lockfile
On Fri, Jun 23, 2017 at 09:01:40AM +0200, Michael Haggerty wrote:
> @@ -522,10 +529,16 @@ int lock_packed_refs(struct ref_store *ref_store, int
> flags)
> timeout_configured = 1;
> }
>
> + /*
> + * Note that we close the lockfile immediately because we
> + * don't write new content to it, but rather to a separate
> + * tempfile.
> + */
> if (hold_lock_file_for_update_timeout(
> &refs->lock,
> refs->path,
> - flags, timeout_value) < 0)
> + flags, timeout_value) < 0 ||
> + close_lock_file(&refs->lock))
> return -1;
I was wondering whether we'd catch a case which accidentally wrote to
the lockfile (instead of the new tempfile, but this close() is a good
safety check).
> - if (commit_lock_file(&refs->lock)) {
> - strbuf_addf(err, "error overwriting %s: %s",
> + if (rename_tempfile(&refs->tempfile, refs->path)) {
> + strbuf_addf(err, "error replacing %s: %s",
> refs->path, strerror(errno));
> goto out;
> }
So our idea of committing now is the tempfile rename, and then...
> @@ -617,9 +640,10 @@ int commit_packed_refs(struct ref_store *ref_store,
> struct strbuf *err)
> goto out;
>
> error:
> - rollback_lock_file(&refs->lock);
> + delete_tempfile(&refs->tempfile);
>
> out:
> + rollback_lock_file(&refs->lock);
We always rollback the lockfile regardless, because committing it would
overwrite our new content with an empty file. There's no real safety
against somebody calling commit_lock_file() on it, but it also seems
like an uncommon error to make.
So this all looks good to me.
-Peff
Re: [PATCH] xdiff: trim common tail with -U0 after diff
Stefan Beller writes: > So I tried finding out more about this hack, > and found the patch that introduced the common tail trimming at > https://public-inbox.org/git/7vmysez0oa@gitster.siamese.dyndns.org/ > 913b45f51b (xdi_diff: trim common trailing lines, 2007-12-13) A relevant one that makes me hesitate to take this kind of change is this: https://public-inbox.org/git/alpine.lfd.0..0712202009290.21...@woody.linux-foundation.org/#t that resulted in this change: commit d2f82950a9226ae1102a7a97f03440a4bf8c6c09 Author: Linus Torvalds Date: Thu Dec 20 20:22:46 2007 -0800 Re(-re)*fix trim_common_tail() The tar-ball and the git archive itself is fine, but yes, the diff from 2.6.23 to 2.6.24-rc6 is bad. It's the "trim_common_tail()" optimization that has caused way too much pain. Very interesting breakage. The patch was actually "correct" in a (rather limited) technical sense, but the context at the end was missing because while the trim_common_tail() code made sure to keep enough common context to allow a valid diff to be generated, the diff machinery itself could decide that it could generate the diff differently than the "obvious" solution. Thee sad fact is that the git optimization (which is very important for "git blame", which needs no context), is only really valid for that one case where we really don't need any context.
Re: [PATCH v2 20/29] packed_ref_store: make class into a subclass of `ref_store`
On Fri, Jun 23, 2017 at 09:01:38AM +0200, Michael Haggerty wrote: > Add the infrastructure to make `packed_ref_store` implement > `ref_store`, at least formally (few of the methods are actually > implemented yet). Change the functions in its interface to take > `ref_store *` arguments. Change `files_ref_store` to store a pointer > to `ref_store *` and to call functions via the virtual `ref_store` > interface where possible. This also means that a few > `packed_ref_store` functions can become static. By itself this looks correct (as do the patches leading up to it). But I think some of the "big picture" is lost. Why do we want it to look like a ref store? I suspect the answer is too big to go into this individual commit message. But I went back and re-read the cover letter, and I don't think it's really explained there, either. So I'm not really sure where it ought to be covered (or if I simply missed it somewhere, or if it's coming up; I'm reading the patches linearly). -Peff
Re: [PATCH] xdiff: trim common tail with -U0 after diff
On Fri, Jun 23, 2017 at 12:13 PM, Junio C Hamano wrote: > Daniel Hahler writes: > >> When -U0 is used, trim_common_tail should be called after `xdl_diff`, so >> that `--indent-heuristic` (and other diff processing) works as expected. > > I thought common-tail trimming was a hack/optimization to avoid > having to pass the whole thing to have xdl_diff() work on it? What > would we be gaining by unconditionally passing the whole thing first > and then still trimming? So I tried finding out more about this hack, and found the patch that introduced the common tail trimming at https://public-inbox.org/git/7vmysez0oa@gitster.siamese.dyndns.org/ 913b45f51b (xdi_diff: trim common trailing lines, 2007-12-13) In the early days there were much larger email threads apparently. I haven't seen such a big one in a while. I did not find the email that the commit message referenced to unfortunately. AFAICT we have 2 things going on: * swapping the order of xdl_diff and the tail trimming * change the condition under which the tail is trimmed (as a logical follow up/ cleanup because of the first point) I do not understand the first one, yet.
Re: [PATCH v2 19/29] packed-backend: new module for handling packed references
On Fri, Jun 23, 2017 at 09:01:37AM +0200, Michael Haggerty wrote: > Now that the interface between `files_ref_store` and > `packed_ref_store` is relatively narrow, move the latter into a new > module, "refs/packed-backend.h" and "refs/packed-backend.c". It still > doesn't quite implement the `ref_store` interface, but it will soon. > > This commit moves code around and adjusts its visibility, but doesn't > change anything. Looks good. Stefan will be happy to know that I used --color-moved to look at it. ;) -Peff
Re: [PATCH] xdiff: trim common tail with -U0 after diff
Daniel Hahler writes:
> When -U0 is used, trim_common_tail should be called after `xdl_diff`, so
> that `--indent-heuristic` (and other diff processing) works as expected.
I thought common-tail trimming was a hack/optimization to avoid
having to pass the whole thing to have xdl_diff() work on it? What
would we be gaining by unconditionally passing the whole thing first
and then still trimming?
> It also removes the check for `!(xecfg->flags & XDL_EMIT_FUNCCONTEXT)`
> added in e0876bca4, which does not appear to be necessary anymore after
> moving the trimming down.
The code was conditionally disabling the hack/optimization; with it
unconditionally disabled, of course it wouldn't be needed, no?
I could understand if the motivation and the proposed change were
"tail-trimming outlived its usefulness, so remove it altogether", or
"trail-trimming negatively affects the output by robbing useful
information that could be used by indent-heuristic, so disable it
when the heuristic is in use".
But neither is what this patch does, so I am sort of at loss.
> --- a/t/t4061-diff-indent.sh
> +++ b/t/t4061-diff-indent.sh
> @@ -116,6 +116,16 @@ test_expect_success 'prepare' '
>4
> EOF
>
> + cat <<-\EOF >spaces-compacted-U0-expect &&
> + diff --git a/spaces.txt b/spaces.txt
> + --- a/spaces.txt
> + +++ b/spaces.txt
> + @@ -4,0 +5,3 @@ a
> + +b
> + +a
> + +
> + EOF
> +
> tr "_" " " <<-\EOF >functions-expect &&
> diff --git a/functions.c b/functions.c
> --- a/functions.c
> @@ -184,6 +194,11 @@ test_expect_success 'diff: --indent-heuristic with
> --histogram' '
> compare_diff spaces-compacted-expect out-compacted4
> '
>
> +test_expect_success 'diff: --indent-heuristic with -U0' '
> + git diff -U0 --indent-heuristic old new -- spaces.txt >out-compacted5 &&
> + compare_diff spaces-compacted-U0-expect out-compacted5
> +'
> +
> test_expect_success 'diff: ugly functions' '
> git diff --no-indent-heuristic old new -- functions.c >out &&
> compare_diff functions-expect out
> diff --git a/xdiff-interface.c b/xdiff-interface.c
> index d3f78ca2a..a7e0e3583 100644
> --- a/xdiff-interface.c
> +++ b/xdiff-interface.c
> @@ -125,16 +125,17 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)
>
> int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
> xdemitconf_t const *xecfg, xdemitcb_t *xecb)
> {
> + int ret;
> mmfile_t a = *mf1;
> mmfile_t b = *mf2;
>
> if (mf1->size > MAX_XDIFF_SIZE || mf2->size > MAX_XDIFF_SIZE)
> return -1;
>
> - if (!xecfg->ctxlen && !(xecfg->flags & XDL_EMIT_FUNCCONTEXT))
> + ret = xdl_diff(&a, &b, xpp, xecfg, xecb);
> + if (ret && !xecfg->ctxlen)
> trim_common_tail(&a, &b);
> -
> - return xdl_diff(&a, &b, xpp, xecfg, xecb);
> + return ret;
> }
>
> int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
[PATCH 2/3] builtin/fetch: parse recurse-submodules-default at default options parsing
Instead of just storing the string and then later calling our own
parsing function 'parse_fetch_recurse_submodules_arg', make use of the
function callback 'option_fetch_parse_recurse_submodules' that was
introduced in the last patch. Also move all submodule recursing variables
in one spot at the top of the file.
Signed-off-by: Stefan Beller
---
builtin/fetch.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9d58dc0a8a..3cca568173 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -36,7 +36,7 @@ static int prune = -1; /* unspecified */
#define PRUNE_BY_DEFAULT 0 /* do we prune by default? */
static int all, append, dry_run, force, keep, multiple, update_head_ok,
verbosity, deepen_relative;
-static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+static int progress = -1;
static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
static int max_children = -1;
static enum transport_family family;
@@ -48,7 +48,8 @@ static struct strbuf default_rla = STRBUF_INIT;
static struct transport *gtransport;
static struct transport *gsecondary;
static const char *submodule_prefix = "";
-static const char *recurse_submodules_default;
+static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+static int recurse_submodules_default = RECURSE_SUBMODULES_DEFAULT;
static int shown_url = 0;
static int refmap_alloc, refmap_nr;
static const char **refmap_array;
@@ -123,9 +124,11 @@ static struct option builtin_fetch_options[] = {
PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },
{ OPTION_STRING, 0, "submodule-prefix", &submodule_prefix, N_("dir"),
N_("prepend this to submodule path output"),
PARSE_OPT_HIDDEN },
- { OPTION_STRING, 0, "recurse-submodules-default",
- &recurse_submodules_default, NULL,
- N_("default mode for recursion"), PARSE_OPT_HIDDEN },
+ { OPTION_CALLBACK, 0, "recurse-submodules-default",
+ &recurse_submodules_default, N_("on-demand"),
+ N_("default for recursive fetching of submodules "
+ "(lower priority than config files)"),
+ PARSE_OPT_HIDDEN, option_fetch_parse_recurse_submodules },
OPT_BOOL(0, "update-shallow", &update_shallow,
N_("accept refs that update .git/shallow")),
{ OPTION_CALLBACK, 0, "refmap", NULL, N_("refmap"),
@@ -1333,10 +1336,8 @@ int cmd_fetch(int argc, const char **argv, const char
*prefix)
deepen = 1;
if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
- if (recurse_submodules_default) {
- int arg =
parse_fetch_recurse_submodules_arg("--recurse-submodules-default",
recurse_submodules_default);
- set_config_fetch_recurse_submodules(arg);
- }
+ if (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)
+
set_config_fetch_recurse_submodules(recurse_submodules_default);
gitmodules_config();
git_config(submodule_config, NULL);
}
--
2.12.2.575.gb14f27f917
[PATCH 3/3] pull: optionally rebase submodules (remote submodule changes only)
Teach pull to optionally update submodules when '--recurse-submodules' is provided. This will teach pull to run 'submodule update --rebase' when the '--recurse-submodules' and '--rebase' flags are given under specific circumstances. On a rebase workflow: = 1. Both sides change the submodule -- Let's assume the following history in a submodule: H---I---J---K---L local branch \ M---N---O---P remote branch and the following in the superproject (recorded submodule in parens): A(H)---B(I)---F(K)---G(L) local branch \ C(N)---D(N)---E(P) remote branch In an ideal world this would rebase the submodule and rewrite the submodule pointers that the superproject points at such that the superproject looks like A(H)---B(I) F(K')---G(L') rebased branch \/ C(N)---D(N)---E(P) remote branch and the submodule as: J---K---L (old dangeling tip) / H---I J'---K'---L' rebased branch \ / M---N---O---P remote branch And if a conflict arises in the submodule the superproject rebase would stop at that commit at which the submodule conflict occurs. Currently a "pull --rebase" in the superproject produces a merge conflict as the submodule pointer changes are conflicting and cannot be resolved. 2. Local submodule changes only --- Assuming histories as above, except that the remote branch would not contain submodule changes, then a result as A(H)---B(I) F(K)---G(L) rebased branch \/ C(I)---D(I)---E(I) remote branch is desire-able. This is what currently happens in rebase. If the recursive flag is given, the ideal git would produce a superproject as: A(H)---B(I) F(K')---G(L') rebased branch (incl. sub rebase!) \/ C(I)---D(I)---E(I) remote branch and the submodule as: J---K---L (old dangeling tip) / H---I J'---K'---L' locally rebased branch \ / M---N---O---P advanced branch This patch doesn't address this issue, however a test is added that this fails up front. 3. Remote submodule changes only -- Assuming histories as in (1) except that the local superproject branch would not have touched the submodule the rebase already works out in the superproject with no conflicts: A(H)---B(I) F(P)---G(P) rebased branch (no sub changes) \ / C(N)---D(N)---E(P) remote branch The recurse flag as presented in this patch would additionally update the submodule as: H---I J'---K'---L' rebased branch \/ M---N---O---P remote branch As neither J, K, L nor J', K', L' are referred to from the superproject, no rewriting of the superproject commits is required. Conclusion for 'pull --rebase --recursive' - If there are no local superproject changes it is sufficient to call "submodule update --rebase" as this produces the desired results. In case of conflicts, the behavior is the same as in 'submodule update --recursive' which is assumed to be sane. This patch implements (3) only. On a merge workflow: We'll start off with the same underlying DAG as in (1) in the rebase workflow. So in an ideal world a 'pull --merge --recursive' would produce this: H---I---J---K---LX \ / M---N---O---P with X as the new merge-commit in the submodule and the superproject as: A(H)---B(I)---F(K)---G(L)---Y(X) \ / C(N)---D(N)---E(P) However modifying the submodules on the fly is not supported in git-merge such that Y(X) is not easy to produce in a single patch. In fact git-merge doesn't know about submodules at all. However when at least one side does not contain commits touching the submodule at all, then we do not need to perform the merge for the submodule but a fast-forward can be done via checking out either L or P in the submodule. This strategy is implemented in 68d03e4a6e (Implement automatic fast-forward merge for submodules, 2010-07-07) already, so to align with the rebase behavior we need to also update the worktree of the submodule. Signed-off-by: Brandon Williams Signed-off-by: Stefan Beller --- Documentation/git-pull.txt | 12 builtin/pull.c | 73 +++--- submodule.c| 26 + submodule.h| 4 +++ t/t5572-pull-submodule.sh | 58 5 files changed, 157 insertions(+), 16 deletions(-) diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt index e414185f5a..b201af6f19 100644 --- a/Documentation/git-pull.txt +++ b/Documentation/git-pull.txt @@ -86,12 +86,12 @@
[PATCH 1/3] builtin/fetch: factor submodule recurse parsing out to submodule config
Later we want to access this parsing in builtin/pull as well.
Signed-off-by: Stefan Beller
---
builtin/fetch.c| 18 ++
submodule-config.c | 22 ++
submodule-config.h | 3 +++
3 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 100248c5af..9d58dc0a8a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -53,20 +53,6 @@ static int shown_url = 0;
static int refmap_alloc, refmap_nr;
static const char **refmap_array;
-static int option_parse_recurse_submodules(const struct option *opt,
- const char *arg, int unset)
-{
- if (unset) {
- recurse_submodules = RECURSE_SUBMODULES_OFF;
- } else {
- if (arg)
- recurse_submodules =
parse_fetch_recurse_submodules_arg(opt->long_name, arg);
- else
- recurse_submodules = RECURSE_SUBMODULES_ON;
- }
- return 0;
-}
-
static int git_fetch_config(const char *k, const char *v, void *cb)
{
if (!strcmp(k, "fetch.prune")) {
@@ -115,9 +101,9 @@ static struct option builtin_fetch_options[] = {
N_("number of submodules fetched in parallel")),
OPT_BOOL('p', "prune", &prune,
N_("prune remote-tracking branches no longer on remote")),
- { OPTION_CALLBACK, 0, "recurse-submodules", NULL, N_("on-demand"),
+ { OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules,
N_("on-demand"),
N_("control recursive fetching of submodules"),
- PARSE_OPT_OPTARG, option_parse_recurse_submodules },
+ PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules },
OPT_BOOL(0, "dry-run", &dry_run,
N_("dry run")),
OPT_BOOL('k', "keep", &keep, N_("keep downloaded pack")),
diff --git a/submodule-config.c b/submodule-config.c
index 4f58491ddb..265d036095 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -2,6 +2,7 @@
#include "submodule-config.h"
#include "submodule.h"
#include "strbuf.h"
+#include "parse-options.h"
/*
* submodule cache lookup structure
@@ -234,6 +235,27 @@ int parse_fetch_recurse_submodules_arg(const char *opt,
const char *arg)
return parse_fetch_recurse(opt, arg, 1);
}
+int option_fetch_parse_recurse_submodules(const struct option *opt,
+ const char *arg, int unset)
+{
+ int *v;
+
+ if (!opt->value)
+ return -1;
+
+ v = opt->value;
+
+ if (unset) {
+ *v = RECURSE_SUBMODULES_OFF;
+ } else {
+ if (arg)
+ *v = parse_fetch_recurse_submodules_arg(opt->long_name,
arg);
+ else
+ *v = RECURSE_SUBMODULES_ON;
+ }
+ return 0;
+}
+
static int parse_update_recurse(const char *opt, const char *arg,
int die_on_error)
{
diff --git a/submodule-config.h b/submodule-config.h
index d434ecdb45..1076a68653 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -23,6 +23,9 @@ struct submodule {
};
extern int parse_fetch_recurse_submodules_arg(const char *opt, const char
*arg);
+struct option;
+extern int option_fetch_parse_recurse_submodules(const struct option *opt,
+const char *arg, int unset);
extern int parse_update_recurse_submodules_arg(const char *opt, const char
*arg);
extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
extern int parse_submodule_config_option(const char *var, const char *value);
--
2.12.2.575.gb14f27f917
[PATCH 0/3] pull: optionally rebase submodules
This supersedes the RFC in May by Brandon https://public-inbox.org/git/20170511172437.96878-1-bmw...@google.com/ It adds more (enough?) error checking, as explained in the last commit message. Documentation and issues raised in the review of that RFC have been addressed. The first two patches are a little refactoring needed for the last patch that adds the functionality. Thanks, Stefan Stefan Beller (3): builtin/fetch: factor submodule recurse parsing out to submodule config builtin/fetch: parse recurse-submodules-default at default options parsing pull: optionally rebase submodules (remote submodule changes only) Documentation/git-pull.txt | 12 builtin/fetch.c| 37 --- builtin/pull.c | 73 +++--- submodule-config.c | 22 ++ submodule-config.h | 3 ++ submodule.c| 26 + submodule.h| 4 +++ t/t5572-pull-submodule.sh | 58 8 files changed, 194 insertions(+), 41 deletions(-) -- 2.12.2.575.gb14f27f917
Re: [PATCH v2 17/29] packed_ref_store: support iteration
On Fri, Jun 23, 2017 at 09:01:35AM +0200, Michael Haggerty wrote:
> +static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)
> +{
> + struct packed_ref_iterator *iter =
> + (struct packed_ref_iterator *)ref_iterator;
I thought had some kind of safe downcasting mechanism for iterators, but
I think I was just thinking of files_downcast() for the ref-store.
I don't mind not having that kind of safety. It seems like an uncommon
error to call the packed-ref iterator function on the wrong type,
especially as it's static here and only accessible as a virtual function
for a packed_ref_iterator.
But something to think about, I guess, as we add more polymorphism.
-Peff
Re: [PATCH v2 00/29] Create a reference backend for packed refs
On Fri, Jun 23, 2017 at 09:01:18AM +0200, Michael Haggerty wrote: > * Change patch 17 "packed_ref_store: support iteration" to always > iterate over the packed refs using `DO_FOR_EACH_INCLUDE_BROKEN`. > This switches off the check in the packed-ref iterator of whether a > reference is broken. This is now checked only in > `files_ref_iterator_advance()`, after the packed and loose > references have been merged together. It also saves some work. I'm curious why you prefer this solution to just removing the code entirely. Wouldn't it be an error to call the packed ref iterator without INCLUDE_BROKEN? The "entries may not be valid" thing is a property of the packed-refs concept itself, not a particular caller's view of it. -Peff
Re: [PATCH v3 0/4] Add regression tests for recent rebase -i fixes
Junio C Hamano writes:
> For 3420, I can wrap the two-liner patch I showed here earlier into
> a commit on top of the series.
So, here is what I'll queue on top before merging the topic down to
'master'.
-- >8 --
Subject: [PATCH] t3420: fix under GETTEXT_POISON build
Newly added tests to t3420 in this series prepare expected
human-readable output from "git rebase -i" and then compare the
actual output with it. As the output from the command is designed
to go through i18n/l10n, we need to use test_i18ncmp to tell
GETTEXT_POISON build that it is OK the output does not match.
Signed-off-by: Junio C Hamano
---
t/t3420-rebase-autostash.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 6826c38cbd..e243700660 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -178,7 +178,7 @@ testrebase () {
test_when_finished git branch -D rebased-feature-branch &&
suffix=${type#\ --} && suffix=${suffix:-am} &&
create_expected_success_$suffix &&
- test_cmp expected actual
+ test_i18ncmp expected actual
'
test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
@@ -275,7 +275,7 @@ testrebase () {
test_when_finished git branch -D rebased-feature-branch &&
suffix=${type#\ --} && suffix=${suffix:-am} &&
create_expected_failure_$suffix &&
- test_cmp expected actual
+ test_i18ncmp expected actual
'
}
--
2.13.1-678-g93553a431c
Re: [PATCH v2 01/29] t1408: add a test of stale packed refs covered by loose refs
On Fri, Jun 23, 2017 at 09:01:19AM +0200, Michael Haggerty wrote: > +test_expect_success setup ' > + test_tick && > + git commit --allow-empty -m one && > + one=$(git rev-parse HEAD) && > + git for-each-ref >actual && > + echo "$one commit refs/heads/master" >expect && > + test_cmp expect actual && > + > + git pack-refs --all && > + git for-each-ref >actual && > + echo "$one commit refs/heads/master" >expect && > + test_cmp expect actual && > + > + cat .git/packed-refs && I think we'd usually drop debugging "cat"s like these in the name of keeping the process count down. Unless they really are intended to confirm that .git/packed-refs exists (although test_path_is_file is a less expensive way of checking that). That's a minor nit, though. -Peff
Re: [RFC/PATCH v4 07/49] Git/Packet.pm: add packet_initialize()
I like where this ends but it seems to me that patches 6, 7 and 8 should
just get merged into patch 4 and 5.
On 6/20/2017 3:54 AM, Christian Couder wrote:
Add a function to initialize the communication. And use this
function in 't/t0021/rot13-filter.pl'.
Signed-off-by: Christian Couder
---
perl/Git/Packet.pm | 13 +
t/t0021/rot13-filter.pl | 8 +---
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm
index 2ad6b00d6c..b0233caf37 100644
--- a/perl/Git/Packet.pm
+++ b/perl/Git/Packet.pm
@@ -19,6 +19,7 @@ our @EXPORT = qw(
packet_bin_write
packet_txt_write
packet_flush
+ packet_initialize
);
our @EXPORT_OK = @EXPORT;
@@ -70,3 +71,15 @@ sub packet_flush {
print STDOUT sprintf( "%04x", 0 );
STDOUT->flush();
}
+
+sub packet_initialize {
+ my ($name, $version) = @_;
+
+ ( packet_txt_read() eq ( 0, $name . "-client" ) ) || die "bad
initialize";
+ ( packet_txt_read() eq ( 0, "version=" . $version ) ) || die "bad
version";
+ ( packet_bin_read() eq ( 1, "" ) )|| die "bad version
end";
+
+ packet_txt_write( $name . "-server" );
+ packet_txt_write( "version=" . $version );
+ packet_flush();
+}
diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 36a9eb3608..5b05518640 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -40,13 +40,7 @@ sub rot13 {
print $debug "START\n";
$debug->flush();
-( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
-( packet_txt_read() eq ( 0, "version=2" ) ) || die "bad version";
-( packet_bin_read() eq ( 1, "" ) ) || die "bad version end";
-
-packet_txt_write("git-filter-server");
-packet_txt_write("version=2");
-packet_flush();
+packet_initialize("git-filter", 2);
( packet_txt_read() eq ( 0, "capability=clean" ) ) || die "bad capability";
( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
Re: [PATCH v3 0/4] Add regression tests for recent rebase -i fixes
Junio C Hamano writes:
> 3404 needs a similar fix-up for the series to be able to stand on
> its own. Alternatively, at least we need to understand what in 'pu'
> makes the result of the merge pass---the symptom indicates that this
> topic cannot be merged to a released version without that unknown
> other topic in 'pu' merged if we want to keep POISON build passing
> the tests.
Ah, no worries. I think I figured it out.
The topic "rebase -i regression fix", which this "regression fix
tests" builds on, is queued on an older codebase than 0d75bfe6
("tests: fix tests broken under GETTEXT_POISON=YesPlease",
2017-05-05); it is natural these old test breakages can be seen when
the topic is tested alone.
So we can safely merge this topic down.
Thanks for prodding me to take a deeper look.
Re: your mail
Jeff King writes: > On Thu, Jun 22, 2017 at 10:23:17PM -0700, Junio C Hamano wrote: > >> Otoh, "community" page does not encourage subscription as a way to >> ensure you'll see follow-up discussion, which may be a good thing to >> add. >> >> A tangent I just found funny is this paragraph on the "community" >> page: >> >> The archive can be found on public-inbox. Click here to >> subscribe. >> >> Of course clicking does not take you to a subscription page for >> public-inbox, even though the two sentences appear to be related. >> >> Perhaps swap the order of the two, like so, with a bit richer >> explanation taken from Ævar's version: >> >> ... disable HTML in your outgoing messages. >> >> By subscribing (click here), you can make sure you're not >> missing follow-up discussion and also learn from other >> development in the community. The list archive can be found >> on public-inbox. > > Yeah, I think that's a good suggestion. Do you want to phrase it in the > form of a patch? :) OK. Letme try.
Re: [RFC/PATCH v4 00/49] Add initial experimental external ODB support
On 6/20/2017 3:54 AM, Christian Couder wrote: Goal Git can store its objects only in the form of loose objects in separate files or packed objects in a pack file. To be able to better handle some kind of objects, for example big blobs, it would be nice if Git could store its objects in other object databases (ODB). To do that, this patch series makes it possible to register commands, also called "helpers", using "odb..command" config variables, to access external ODBs where objects can be stored and retrieved. External ODBs should be able to tranfer information about the blobs they store. This patch series shows how this is possible using kind of replace refs. Great to see this making progress! My thoughts and questions are mostly about the overall design tradeoffs. Is your intention to enable the ODB to completely replace the regular object store or just to supplement it? I think it would be good to ensure the interface is robust and performant enough to actually replace the current object store interface (even if we don't actually do that just yet). Another way of asking this is: do the 3 verbs (have, get, put) and the 3 types of "get" enable you to wrap the current loose object and pack file code as ODBs and run completely via the external ODB interface? If not, what is missing and can it be added? _Eventually_ it would be great to see the current object store(s) moved behind the new ODB interface. When there are multiple ODB providers, what is the order they are called? If one fails a request (get, have, put) are the others called to see if they can fulfill the request? Can the order they are called for various verb be configured explicitly? For example, it would be nice to have a "large object ODB handler" configured to get first try at all "put" verbs. Then if it meets it's size requirements, it will handle the verb, otherwise it fail and git will try the other ODBs. Design ~~ * The "helpers" (registered commands) Each helper manages access to one external ODB. There are now 2 different modes for helper: - When "odb..scriptMode" is set to "true", the helper is launched each time Git wants to communicate with the external ODB. - When "odb..scriptMode" is not set or set to "false", then the helper is launched once as a sub-process (using sub-process.h), and Git communicates with it using packet lines. Is it worth supporting two different modes long term? It seems that this could be simplified (less code to write, debug, document, support) by only supporting the 2nd that uses the sub-process. As far as I can tell, the capabilities are the same, it's just the second one is more performant when multiple calls are made. A helper can be given different instructions by Git. The instructions that are supported are negociated at the beginning of the communication using a capability mechanism. For now the following instructions are supported: - "have": the helper should respond with the sha1, size and type of all the objects the external ODB contains, one object per line. - "get ": the helper should then read from the external ODB the content of the object corresponding to and pass it to Git. - "put ": the helper should then read from from Git an object and store it in the external ODB. Currently "have" and "put" are optional. It's good the various verbs can be optional. That way any particular ODB only has to handle those it needs to provide a different behavior for. There are 3 different kinds of "get" instructions depending on how the helper passes objects to Git: - "fault_in": the helper will write the requested objects directly into the regular Git object database, and then Git will retry reading it from there. I think the "fault_in" behavior can be implemented efficiently without the overhead of a 3rd special "get" instruction if we enable some of the other capabilities discussed. For example, assume an ODB is setup to handle missing objects (by registering itself as "last" in the prioritized list of ODB handlers). If it is ever asked to retrieve a missing object, it can retrieve the object and return it as a "git_object" or "plain_object" and also cache it locally as a loose object, pack file, or any other ODB handler supported mechanism. Future requests will then provide that object via the locally cached copy and its associated ODB handler. - "git_object": the helper will send the object as a Git object. - "plain_object": the helper will send the object (a blob) as a raw object. (The blob content will be sent as is.) For now the kind of "get" that is supported is read from the "odb..fetchKind" configuration variable, but in the future it should be decided as part of the capability negociation. I agree it makes sense to move this into the capability negotiation but I also wonder if we really need to support both. Is there
Re: [PATCH] pathspec: die on empty strings as pathspec
Junio C Hamano writes: > I am not sure if we even want the dot there, but at least that is > what the original author of the test intended to do when s/he > decided to pass an empty string as the pathspec. > > t/t0027-auto-crlf.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) I'll queue the following _before_ your "final step", so that the whole thing can be advanced to 'next' and hopefully to 'master' in some future releases. Given that we ourselves did not even notice until now that one of our scripts get broken by this "final step", even though we kept issuing the warning message, we may want to re-think our overall strategy for deprecation. We've assumed that "keep behaving as before but warn for a few years and then finally give a hard failure" would be sufficient, but depending on how the script that employ our programs hide the warnings from the end users, that may not be a good enough transition strategy. At the same time we re-think the deprecation strategy, we also need to see if we can update our test framework to help us better. Ideally, we could have caught this existing breakage of passing "" as pathspec in the test soon after we went into "keep behaving as before but warn" stage. We didn't and found it out only when we switched to "finally give a hard failure" stage. That is very unsatisfactory. Needless to say, neither of the above two comes from _your_ change. It's just something we need to improve in a larger scope of the whole project I realized. Thanks. -- >8 -- Subject: [PATCH] t0027: do not use an empty string as a pathspec element In an upcoming update, we will finally make an empty string illegal as an element in a pathspec; it traditionally meant the same as ".", i.e. include everything, so update this test that passes "" to pass a dot instead. At this point in the test sequence, there is no modified path that need to be further added before committing; the working tree is empty except for .gitattributes which was just added to the index. So we could instead pass no pathspec, but this is a conversion more faithful to the original. Signed-off-by: Junio C Hamano --- t/t0027-auto-crlf.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh index 93725895a4..e41c9b3bb2 100755 --- a/t/t0027-auto-crlf.sh +++ b/t/t0027-auto-crlf.sh @@ -322,7 +322,7 @@ test_expect_success 'setup master' ' echo >.gitattributes && git checkout -b master && git add .gitattributes && - git commit -m "add .gitattributes" "" && + git commit -m "add .gitattributes" . && printf "\$Id: \$\nLINEONE\nLINETWO\nLINETHREE" >LF && printf "\$Id: \$\r\nLINEONE\r\nLINETWO\r\nLINETHREE" >CRLF && printf "\$Id: \$\nLINEONE\r\nLINETWO\nLINETHREE" >CRLF_mix_LF && -- 2.13.1-678-g93553a431c
Re: [PATCH v4 00/20] repository object
Jeff Hostetler writes: > On 6/22/2017 2:43 PM, Brandon Williams wrote: >> As before you can find this series at: >> https://github.com/bmwill/git/tree/repository-object >> >> Changes in v4: >> >> * Patch 11 is slightly different and turns off all path relocation when a >>worktree is provided instead of just for the index file (Thanks for the >> help >>Jonathan Nieder). >> * 'repo_init()' has a tighter API and now requires that the provided gitdir >> is >>a path to the gitdir instead of either a path to the gitdir or path to the >>worktree (which has a .git file or directory) (Thanks Jonathan Tan). >> * Minor comment and commit message chagnes >> >> Note: Like v3 this series is dependent on on 'bw/config-h' and >>'bw/ls-files-sans-the-index' >> >> Brandon Williams (20): > > I read thru the v1 and v4 versions. Very nice. > And thanks for splitting the earlier parts out > into independent patches. > > I didn't have any complaints, but did want to ACK > that I had looked at it. Thanks.
Re: [PATCH 1/3] list-objects: add filter_blob to traverse_commit_list
On 6/22/2017 6:10 PM, Jonathan Tan wrote: On Thu, 22 Jun 2017 14:45:26 -0700 Jonathan Tan wrote: On Thu, 22 Jun 2017 20:36:13 + Jeff Hostetler wrote: From: Jeff Hostetler In preparation for partial/sparse clone/fetch where the server is allowed to omit large/all blobs from the packfile, teach traverse_commit_list() to take a blob filter-proc that controls when blobs are shown and marked as SEEN. Normally, traverse_commit_list() always marks visited blobs as SEEN, so that the show_object() callback will never see duplicates. Since a single blob OID may be referenced by multiple pathnames, we may not be able to decide if a blob should be excluded until later pathnames have been traversed. With the filter-proc, the automatic deduping is disabled. Comparing this approach (this patch and the next one) against mine [1], I see that this has the advantage of (in pack-objects) avoiding the invocation of add_preferred_base_object() on blobs that are filtered out, avoiding polluting the "to_pack" data structure with information that we do not need. But it does add an extra place where blobs are filtered out (besides add_object_entry()), which makes it harder for the reader to keep track of what's going on. I took a brief look to see if filtering could be eliminated from add_object_entry(), but that function is called from many places, so I couldn't tell. Anyway, I think both approaches will work (this patch's and mine [1]). [1] https://public-inbox.org/git/6f7934621717141ce3bb6bc05cf1d59c7900ccc5.1496432147.git.jonathanta...@google.com/ Also it should be mentioned somewhere that this does not cover the bitmap case - a similar discussion should be included in one of the patches, like I did in [1]. And looking back at my original cover letter [2], I wrote: This is similar to [1] except that this [...] is slightly more comprehensive in that the read_object_list_from_stdin() codepath is also covered in addition to the get_object_list() codepath. (Although, to be clear, upload-pack always passes "--revs" and thus only needs the get_object_list() codepath). (The [1] in the quote above refers to one of Jeff Hostetler's patches, [QUOTE 1].) The same issue applies to this patch (since read_object_list_from_stdin() invokes add_object_entry() directly without going through the traversal mechanism) and probably warrants at least some description in one of the commit messages. [1] https://public-inbox.org/git/6f7934621717141ce3bb6bc05cf1d59c7900ccc5.1496432147.git.jonathanta...@google.com/ [2] https://public-inbox.org/git/cover.1496361873.git.jonathanta...@google.com/ [QUOTE 1] https://public-inbox.org/git/1488994685-37403-3-git-send-email-jeffh...@microsoft.com/ Thanks for the quick feedback. I'll dig into each of these comments as I work on my next draft. Jeff
Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Jeff King writes:
> The idea was that eventually the caller might be able to come up with a
> TZ that is not blank, but is also not what strftime("%Z") would produce.
> Conceivably that could be done if Git commits carried the "%Z"
> information (not likely), or if we used a reverse-lookup table (also not
> likely).
The former might be conceivable, but I do not think the latter is
workable. Knowing that a location happened to be using a particular
GMT offset at a specific point in time simply is not sufficient to
give us a zone name; the whole idea of a zone name being that it
will give us rules that would apply to other timestamps, not just
the one we are paring with GMT offset in our committer and author
timestamp fields.
A third possibility is the information may come out of band.
Something like "When gitster is in +0900 he is in JST" can be
maintained per project and supplied by the caller.
> This closes the door on that. Since we don't have immediate plans to go
> that route, I'm OK with this patch. It would be easy enough to re-open
> the door if we change our minds later.
I agree that it is not too much hassle to revert this change. I
actually would not have minded if René's original were written with
a boolean flag. But I do not see the value in flipping ("" vs NULL)
with a bool now. The benefit we are gaining (other than closing the
door) is unclear to me.
>> /**
>> * Add the time specified by `tm`, as formatted by `strftime`.
>> - * `tz_name` is used to expand %Z internally unless it's NULL.
>> * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
>> * of Greenwich, and it's used to expand %z internally. However, tokens
>> * with modifiers (e.g. %Ez) are passed to `strftime`.
>> + * `omit_strftime_tz_name` when set, means don't let `strftime` format
>> + * %Z, instead do our own formatting.
>
> Since we now always turn it into a blank string, perhaps "do our own
> formatting" could be more descriptive: we convert it into the empty
> string.
Yeah, that reads better. I am also OK if this said that an empty
string is an accepted POSIX way to fallback when the information is
unavailable---and we really are in that "information is unavailable"
situation in this code.
Re: [PATCH v3 0/4] Add regression tests for recent rebase -i fixes
Phillip Wood writes:
> t3404 passes for me,
> $ make GETTEXT_POISON=YesPlease
> $ cd t &&sh t3404-rebase-interactive.sh -i -v
> ...
> # still have 1 known breakage(s)
> # passed all remaining 95 test(s)
> 1..96
Interesting. The tip of 'pu', which includes this series, does pass
for me, too, but the tip of this topic 7d70e6b9 ("rebase: add more
regression tests for console output", 2017-06-19) tested in
isolation fails, and gives the output at the end of this message.
> Do you want me to submit a fixup patch for t3420 or have you
> got one already?
For 3420, I can wrap the two-liner patch I showed here earlier into
a commit on top of the series.
3404 needs a similar fix-up for the series to be able to stand on
its own. Alternatively, at least we need to understand what in 'pu'
makes the result of the merge pass---the symptom indicates that this
topic cannot be merged to a released version without that unknown
other topic in 'pu' merged if we want to keep POISON build passing
the tests.
Thanks.
-- output from 3404 follows --
Initialized empty Git repository in /home/gitster/git/t/trash
directory.t3404-rebase-interactive/.git/
expecting success:
test_commit A file1 &&
test_commit B file1 &&
test_commit C file2 &&
test_commit D file1 &&
test_commit E file3 &&
git checkout -b branch1 A &&
test_commit F file4 &&
test_commit G file1 &&
test_commit H file5 &&
git checkout -b branch2 F &&
test_commit I file6 &&
git checkout -b conflict-branch A &&
test_commit one conflict &&
test_commit two conflict &&
test_commit three conflict &&
test_commit four conflict &&
git checkout -b no-conflict-branch A &&
test_commit J fileJ &&
test_commit K fileK &&
test_commit L fileL &&
test_commit M fileM &&
git checkout -b no-ff-branch A &&
test_commit N fileN &&
test_commit O fileO &&
test_commit P fileP
[master# GETTEXT POISON # 6e62bf8] A
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 file1
[master 313fe96] B
Author: A U Thor
1 file changed, 1 insertion(+), 1 deletion(-)
[master d0f65f2] C
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 file2
[master 0547e3f] D
Author: A U Thor
1 file changed, 1 insertion(+), 1 deletion(-)
[master 8f99a4f] E
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 file3
# GETTEXT POISON #[branch1 cfefd94] F
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 file4
[branch1 83751a6] G
Author: A U Thor
1 file changed, 1 insertion(+), 1 deletion(-)
[branch1 4373208] H
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 file5
# GETTEXT POISON #[branch2 615be62] I
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 file6
# GETTEXT POISON #[conflict-branch b895952] one
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 conflict
[conflict-branch 766a798] two
Author: A U Thor
1 file changed, 1 insertion(+), 1 deletion(-)
[conflict-branch 1eadf03] three
Author: A U Thor
1 file changed, 1 insertion(+), 1 deletion(-)
[conflict-branch f91a2b3] four
Author: A U Thor
1 file changed, 1 insertion(+), 1 deletion(-)
# GETTEXT POISON #[no-conflict-branch 808874f] J
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 fileJ
[no-conflict-branch 265b89e] K
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 fileK
[no-conflict-branch 6b0f5e6] L
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 fileL
[no-conflict-branch 3389558] M
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 fileM
# GETTEXT POISON #[no-ff-branch 53b4423] N
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 fileN
[no-ff-branch cc47714] O
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 fileO
[no-ff-branch faef1a5] P
Author: A U Thor
1 file changed, 1 insertion(+)
create mode 100644 fileP
ok 1 - setup
expecting success:
git checkout -b emptybranch master &&
git commit --allow-empty -m "empty" &&
git rebase --keep-empty -i HEAD~2 &&
git log --oneline >actual &&
test_line_count = 6 actual
# GETTEXT POISON #[emptybranch da33401] empty
Author: A U Thor
Successfully rebased and updated refs/heads/emptybranch.
ok 2 - rebase --keep-empty
expecting success:
git checkout master &&
(
set_fake_editor &&
FAKE_LINES="1 exec_>touch-one
2 exec_>touch-two exec_false exec_>touch-three
3 4
exec_>\"touch-file__name_with_spaces\";_>touch-after-semicolon 5" &&
export FAKE_LINES &&
test_must_fail git rebase -i A
) &&
test_path_is_file touch-one &&
test_path_is_file touch-two &&
test_path_is_missing touch-three " (should have st
Re: your mail
On Thu, Jun 22, 2017 at 10:23:17PM -0700, Junio C Hamano wrote: > Otoh, "community" page does not encourage subscription as a way to > ensure you'll see follow-up discussion, which may be a good thing to > add. > > A tangent I just found funny is this paragraph on the "community" > page: > > The archive can be found on public-inbox. Click here to > subscribe. > > Of course clicking does not take you to a subscription page for > public-inbox, even though the two sentences appear to be related. > > Perhaps swap the order of the two, like so, with a bit richer > explanation taken from Ævar's version: > > ... disable HTML in your outgoing messages. > > By subscribing (click here), you can make sure you're not > missing follow-up discussion and also learn from other > development in the community. The list archive can be found > on public-inbox. Yeah, I think that's a good suggestion. Do you want to phrase it in the form of a patch? :) -Peff
Re: [PATCH v4 00/20] repository object
On 6/22/2017 2:43 PM, Brandon Williams wrote: As before you can find this series at: https://github.com/bmwill/git/tree/repository-object Changes in v4: * Patch 11 is slightly different and turns off all path relocation when a worktree is provided instead of just for the index file (Thanks for the help Jonathan Nieder). * 'repo_init()' has a tighter API and now requires that the provided gitdir is a path to the gitdir instead of either a path to the gitdir or path to the worktree (which has a .git file or directory) (Thanks Jonathan Tan). * Minor comment and commit message chagnes Note: Like v3 this series is dependent on on 'bw/config-h' and 'bw/ls-files-sans-the-index' Brandon Williams (20): I read thru the v1 and v4 versions. Very nice. And thanks for splitting the earlier parts out into independent patches. I didn't have any complaints, but did want to ACK that I had looked at it. Jeff
Re: [PATCH -v2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
On Fri, Jun 23, 2017 at 04:36:06PM +, Ævar Arnfjörð Bjarmason wrote:
> I believe this addresses the comments in the thread so far. Also Re:
> René's "why const?" in a2673ce4-5cf8-6b40-d4db-8e2a49518...@web.de:
> Because tzname_from_tz isn't changed in the body of the function, only
> read.
Sure, it's not wrong. But that property is also held by 99% of the
parameters that are passed by value. It's the normal style in our code
base (and in most C code bases I know of) to never declare pass-by-value
as const. It pollutes the interface and isn't something the caller cares
about.
Without passing judgement on whether that style is good or not (though
IMHO it is), making this one case different than all the others is a bad
idea. It makes the reader wonder why it's different.
> diff --git a/date.c b/date.c
> index 1fd6d66375..17db07d905 100644
> --- a/date.c
> +++ b/date.c
> @@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const
> struct date_mode *mode)
> tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
> else if (mode->type == DATE_STRFTIME)
> strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
> - mode->local ? NULL : "");
> + mode->local);
You flipped the boolean here. That's OK by me. But in the definition...
> void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
> - int tz_offset, const char *tz_name)
> + int tz_offset, const int tzname_from_tz)
Wouldn't tzname_from_tz only happen when we're _not_ in local mode? I
suggested that name anticipating your second patch to actually compute
it based on "tz". In local-mode it's not coming from tz, it's coming
from secret unportable magic (the combination of localtime() and
strftime()).
> @@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt,
> const struct tm *tm,
> fmt++;
> break;
> case 'Z':
> - if (tz_name) {
> - strbuf_addstr(&munged_fmt, tz_name);
> + if (!tzname_from_tz) {
> fmt++;
> break;
> }
This logic matches your inversion in the caller, so it does the right
thing. But I think the name is wrong, as above.
> index 4559035c47..eba5d59a77 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -340,14 +340,15 @@ extern void strbuf_vaddf(struct strbuf *sb, const char
> *fmt, va_list ap);
>
> /**
> * Add the time specified by `tm`, as formatted by `strftime`.
> - * `tz_name` is used to expand %Z internally unless it's NULL.
> * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
> * of Greenwich, and it's used to expand %z internally. However, tokens
> * with modifiers (e.g. %Ez) are passed to `strftime`.
> + * `tzname_from_tz` when set, means let `strftime` format %Z, instead
> + * of intercepting it and doing our own formatting.
> */
> extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
> const struct tm *tm, int tz_offset,
> - const char *tz_name);
> + const int omit_strftime_tz_name);
This would need the new name, too (whatever it is).
-Peff
Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool
On Fri, Jun 23, 2017 at 06:23:10PM +0200, René Scharfe wrote: > > > I have a WIP patch (which may not make it on-list, depending) playing > > > with the idea I proposed in > > > CACBZZX5OQc45fUyDVayE89rkT=+8m5s4efsxcabcy7upme5...@mail.gmail.com which > > > just inserts the custom TZ name based on the offset inside that `if > > > (omit_strftime_tz_name)` branch. > > > > OK. I'd assumed that would all happen outside of strbuf_addftime(). But > > if it happens inside, then I agree a flag is better. > > Oh, so the interface that was meant to allow better time zone names > without having to make strbuf_addftime() even bigger than it already is > turns out to be too ugly for its purpose? I'm sorry. :( I haven't seen Ævar's patch, but I agree that if the caller did: if (mode->local) tzname = NULL; /* let strftime handle it */ else tzname = fake_tz_from_offset(tz); ... strbuf_addftime(&buf, fmt, tm, tz, tzname); that would be pretty clean (and what I was expecting with the "I'd assumed" above). -Peff
[PATCH -v2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Change the code for deciding what's to be done about %Z to stop
passing always either a NULL or "" char * to
strbuf_addftime(). Instead pass a boolean int to indicate whether the
strftime() %Z format should be expanded to an empty string, which is
what this code is actually doing.
This code grew organically between the changes in 9eafe86d58 ("Merge
branch 'rs/strbuf-addftime-zZ'", 2017-06-22) yielding an end result
that wasn't very readable.
Out of context it looked as though the call to strbuf_addstr() might
be adding a custom tz_name to the string, but actually tz_name would
always be "", so the call to strbuf_addstr() just to add an empty
string to the format was pointless.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
I believe this addresses the comments in the thread so far. Also Re:
René's "why const?" in a2673ce4-5cf8-6b40-d4db-8e2a49518...@web.de:
Because tzname_from_tz isn't changed in the body of the function, only
read.
date.c | 2 +-
strbuf.c | 5 ++---
strbuf.h | 5 +++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/date.c b/date.c
index 1fd6d66375..17db07d905 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const
struct date_mode *mode)
tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
else if (mode->type == DATE_STRFTIME)
strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
- mode->local ? NULL : "");
+ mode->local);
else
strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index be3b9e37b1..92b7bda772 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -786,7 +786,7 @@ char *xstrfmt(const char *fmt, ...)
}
void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
-int tz_offset, const char *tz_name)
+int tz_offset, const int tzname_from_tz)
{
struct strbuf munged_fmt = STRBUF_INIT;
size_t hint = 128;
@@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm,
fmt++;
break;
case 'Z':
- if (tz_name) {
- strbuf_addstr(&munged_fmt, tz_name);
+ if (!tzname_from_tz) {
fmt++;
break;
}
diff --git a/strbuf.h b/strbuf.h
index 4559035c47..eba5d59a77 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -340,14 +340,15 @@ extern void strbuf_vaddf(struct strbuf *sb, const char
*fmt, va_list ap);
/**
* Add the time specified by `tm`, as formatted by `strftime`.
- * `tz_name` is used to expand %Z internally unless it's NULL.
* `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
* of Greenwich, and it's used to expand %z internally. However, tokens
* with modifiers (e.g. %Ez) are passed to `strftime`.
+ * `tzname_from_tz` when set, means let `strftime` format %Z, instead
+ * of intercepting it and doing our own formatting.
*/
extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm, int tz_offset,
- const char *tz_name);
+ const int omit_strftime_tz_name);
/**
* Read a given size of data from a FILE* pointer to the buffer.
--
2.13.1.611.g7e3b11ae1
Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Am 23.06.2017 um 17:23 schrieb Jeff King:
On Fri, Jun 23, 2017 at 05:13:38PM +0200, Ævar Arnfjörð Bjarmason wrote:
The idea was that eventually the caller might be able to come up with a
TZ that is not blank, but is also not what strftime("%Z") would produce.
Conceivably that could be done if Git commits carried the "%Z"
information (not likely), or if we used a reverse-lookup table (also not
likely).
This closes the door on that. Since we don't have immediate plans to go
that route, I'm OK with this patch. It would be easy enough to re-open
the door if we change our minds later.
Closes the door on doing that via passing the char * of the prepared
custom tz_name to strbuf_addftime().
I have a WIP patch (which may not make it on-list, depending) playing
with the idea I proposed in
CACBZZX5OQc45fUyDVayE89rkT=+8m5s4efsxcabcy7upme5...@mail.gmail.com which
just inserts the custom TZ name based on the offset inside that `if
(omit_strftime_tz_name)` branch.
OK. I'd assumed that would all happen outside of strbuf_addftime(). But
if it happens inside, then I agree a flag is better.
Oh, so the interface that was meant to allow better time zone names
without having to make strbuf_addftime() even bigger than it already is
turns out to be too ugly for its purpose? I'm sorry. :(
René
Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Am 23.06.2017 um 17:25 schrieb Jeff King: On Fri, Jun 23, 2017 at 05:20:19PM +0200, René Scharfe wrote: diff --git a/strbuf.c b/strbuf.c index be3b9e37b1..81ff3570e2 100644 --- a/strbuf.c +++ b/strbuf.c @@ -786,7 +786,7 @@ char *xstrfmt(const char *fmt, ...) } void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm, -int tz_offset, const char *tz_name) +int tz_offset, const int omit_strftime_tz_name) Why const? And as written above, naming the parameter local would make it easier to understand instead of exposing an implementation detail in the interface. I think calling it "local" isn't right. That's a decision the _caller_ is making about whether to pass through %Z. But the actual implementation is more like "should the function fill tzname based on tz?" So some name along those lines would make sense. In which case the caller would then pass "!mode->local" for the flag. We only have a single caller currently, so responsibilities can still be shifted, and it's a bit hard to draw the line. "Here's a format and all time information I have, expand!" is just as viable as "here's a format and most time information, expand, and handle %Z in this particular way when you see it!", I think. René
Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool
On Fri, Jun 23, 2017 at 05:20:19PM +0200, René Scharfe wrote: > > diff --git a/strbuf.c b/strbuf.c > > index be3b9e37b1..81ff3570e2 100644 > > --- a/strbuf.c > > +++ b/strbuf.c > > @@ -786,7 +786,7 @@ char *xstrfmt(const char *fmt, ...) > > } > > void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm > > *tm, > > -int tz_offset, const char *tz_name) > > +int tz_offset, const int omit_strftime_tz_name) > > Why const? And as written above, naming the parameter local would make > it easier to understand instead of exposing an implementation detail in > the interface. I think calling it "local" isn't right. That's a decision the _caller_ is making about whether to pass through %Z. But the actual implementation is more like "should the function fill tzname based on tz?" So some name along those lines would make sense. In which case the caller would then pass "!mode->local" for the flag. -Peff
Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool
On Fri, Jun 23, 2017 at 05:13:38PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > The idea was that eventually the caller might be able to come up with a
> > TZ that is not blank, but is also not what strftime("%Z") would produce.
> > Conceivably that could be done if Git commits carried the "%Z"
> > information (not likely), or if we used a reverse-lookup table (also not
> > likely).
> >
> > This closes the door on that. Since we don't have immediate plans to go
> > that route, I'm OK with this patch. It would be easy enough to re-open
> > the door if we change our minds later.
>
> Closes the door on doing that via passing the char * of the prepared
> custom tz_name to strbuf_addftime().
>
> I have a WIP patch (which may not make it on-list, depending) playing
> with the idea I proposed in
> CACBZZX5OQc45fUyDVayE89rkT=+8m5s4efsxcabcy7upme5...@mail.gmail.com which
> just inserts the custom TZ name based on the offset inside that `if
> (omit_strftime_tz_name)` branch.
OK. I'd assumed that would all happen outside of strbuf_addftime(). But
if it happens inside, then I agree a flag is better.
> >> * Add the time specified by `tm`, as formatted by `strftime`.
> >> - * `tz_name` is used to expand %Z internally unless it's NULL.
> >> * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
> >> * of Greenwich, and it's used to expand %z internally. However, tokens
> >> * with modifiers (e.g. %Ez) are passed to `strftime`.
> >> + * `omit_strftime_tz_name` when set, means don't let `strftime` format
> >> + * %Z, instead do our own formatting.
> >
> > Since we now always turn it into a blank string, perhaps "do our own
> > formatting" could be more descriptive: we convert it into the empty
> > string.
>
> Then we'd need to change this comment again if we had some patch like
> the one I mentioned above, I thought it was better to just leave this
> vague enough that we didn't need to do that.
Right, if you're going to do your own formatting inside the function,
then I agree the wording should be kept. But then "omit" is not really
the right word. Isn't it "tzname_from_tz" or something?
-Peff
Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Am 23.06.2017 um 16:46 schrieb Ævar Arnfjörð Bjarmason:
Change the code for deciding what's to be done about %Z to stop
passing always either a NULL or "" char * to
strbuf_addftime(). Instead pass a boolean int to indicate whether the
strftime() %Z format should be omitted, which is what this code is
actually doing.
"Omitting" sounds not quite right somehow. We expand %Z to the empty
string because that's the best we can do -- which amounts to a removal,
but that's not the intent, just an implementation detail. Calling it
"handling %Z internally" would be better, I think.
This code grew organically between the changes in 9eafe86d58 ("Merge
branch 'rs/strbuf-addftime-zZ'", 2017-06-22) yielding an end result
that wasn't very readable. Out of context it looked as though the call
to strbuf_addstr() might be adding a custom tz_name to the string, but
actually tz_name would always be "", so the call to strbuf_addstr()
just to add an empty string to the format was pointless.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
date.c | 2 +-
strbuf.c | 5 ++---
strbuf.h | 5 +++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/date.c b/date.c
index 1fd6d66375..5f09743bad 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const
struct date_mode *mode)
tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
else if (mode->type == DATE_STRFTIME)
strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
- mode->local ? NULL : "");
+ mode->local ? 0 : 1);
I don't see how this is more readable -- both look about equally ugly to
me. Passing mode->local unchanged would be better.
else
strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index be3b9e37b1..81ff3570e2 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -786,7 +786,7 @@ char *xstrfmt(const char *fmt, ...)
}
void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
-int tz_offset, const char *tz_name)
+int tz_offset, const int omit_strftime_tz_name)
Why const? And as written above, naming the parameter local would make
it easier to understand instead of exposing an implementation detail in
the interface.
{
struct strbuf munged_fmt = STRBUF_INIT;
size_t hint = 128;
@@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm,
fmt++;
break;
case 'Z':
- if (tz_name) {
- strbuf_addstr(&munged_fmt, tz_name);
+ if (omit_strftime_tz_name) {
Getting rid of this strbuf_addstr call is nice, but as Peff mentioned in
his reply it also reduces the flexibility of the function. While it's
unlikely to be needed I'm not convinced that we should already block
this path (even though it could be easily reopened).
fmt++;
break;
}
diff --git a/strbuf.h b/strbuf.h
index 4559035c47..bad698058a 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -340,14 +340,15 @@ extern void strbuf_vaddf(struct strbuf *sb, const char
*fmt, va_list ap);
/**
* Add the time specified by `tm`, as formatted by `strftime`.
- * `tz_name` is used to expand %Z internally unless it's NULL.
* `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
* of Greenwich, and it's used to expand %z internally. However, tokens
* with modifiers (e.g. %Ez) are passed to `strftime`.
+ * `omit_strftime_tz_name` when set, means don't let `strftime` format
+ * %Z, instead do our own formatting.
*/
extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm, int tz_offset,
- const char *tz_name);
+ const int omit_strftime_tz_name);
/**
* Read a given size of data from a FILE* pointer to the buffer.
[PATCH v2 2/3] t1301: move modebits() to test-lib-functions.sh
As the modebits() function can be useful outside t1301,
let's move it into test-lib-functions.sh, and while at
it let's rename it test_modebits().
Signed-off-by: Christian Couder
---
t/t1301-shared-repo.sh | 18 +++---
t/test-lib-functions.sh | 5 +
2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 1312004f8c..dfece751b5 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -19,10 +19,6 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
)
'
-modebits () {
- ls -l "$1" | sed -e 's|^\(..\).*|\1|'
-}
-
for u in 002 022
do
test_expect_success POSIXPERM "shared=1 does not clear bits preset by
umask $u" '
@@ -88,7 +84,7 @@ do
rm -f .git/info/refs &&
git update-server-info &&
- actual="$(modebits .git/info/refs)" &&
+ actual="$(test_modebits .git/info/refs)" &&
verbose test "x$actual" = "x-$y"
'
@@ -98,7 +94,7 @@ do
rm -f .git/info/refs &&
git update-server-info &&
- actual="$(modebits .git/info/refs)" &&
+ actual="$(test_modebits .git/info/refs)" &&
verbose test "x$actual" = "x-$x"
'
@@ -111,7 +107,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in
unshared repo' '
umask 002 &&
git update-server-info &&
echo "-rw-rw-r--" >expect &&
- modebits .git/info/refs >actual &&
+ test_modebits .git/info/refs >actual &&
test_cmp expect actual
'
@@ -177,7 +173,7 @@ test_expect_success POSIXPERM 'remote init does not use
config from cwd' '
umask 0022 &&
git init --bare child.git &&
echo "-rw-r--r--" >expect &&
- modebits child.git/config >actual &&
+ test_modebits child.git/config >actual &&
test_cmp expect actual
'
@@ -187,7 +183,7 @@ test_expect_success POSIXPERM 're-init respects
core.sharedrepository (local)' '
echo whatever >templates/foo &&
git init --template=templates &&
echo "-rw-rw-rw-" >expect &&
- modebits .git/foo >actual &&
+ test_modebits .git/foo >actual &&
test_cmp expect actual
'
@@ -198,7 +194,7 @@ test_expect_success POSIXPERM 're-init respects
core.sharedrepository (remote)'
test_path_is_missing child.git/foo &&
git init --bare --template=../templates child.git &&
echo "-rw-rw-rw-" >expect &&
- modebits child.git/foo >actual &&
+ test_modebits child.git/foo >actual &&
test_cmp expect actual
'
@@ -209,7 +205,7 @@ test_expect_success POSIXPERM 'template can set
core.sharedrepository' '
cp .git/config templates/config &&
git init --bare --template=../templates child.git &&
echo "-rw-rw-rw-" >expect &&
- modebits child.git/HEAD >actual &&
+ test_modebits child.git/HEAD >actual &&
test_cmp expect actual
'
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5ee124332a..db622c3555 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -216,6 +216,11 @@ test_chmod () {
git update-index --add "--chmod=$@"
}
+# Get the modebits from a file.
+test_modebits () {
+ ls -l "$1" | sed -e 's|^\(..\).*|\1|'
+}
+
# Unset a configuration variable, but don't fail if it doesn't exist.
test_unconfig () {
config_dir=
--
2.13.1.519.g0a0746bea4
[PATCH v2 3/3] t1700: make sure split-index respects core.sharedrepository
Add a few tests to check that both the split-index file and the shared-index file are created using the right permissions when core.sharedrepository is set. Signed-off-by: Christian Couder --- t/t1700-split-index.sh | 17 + 1 file changed, 17 insertions(+) diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh index af3ec0da5a..2c5be732e4 100755 --- a/t/t1700-split-index.sh +++ b/t/t1700-split-index.sh @@ -370,4 +370,21 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now" test $(ls .git/sharedindex.* | wc -l) -le 2 ' +while read -r mode modebits filename; do + test_expect_success POSIXPERM "split index respects core.sharedrepository $mode" ' + git config core.sharedrepository "$mode" && + : >"$filename" && + git update-index --add "$filename" && + echo "$modebits" >expect && + test_modebits .git/index >actual && + test_cmp expect actual && + newest_shared_index=$(ls -t .git/sharedindex.* | head -1) && + test_modebits "$newest_shared_index" >actual && + test_cmp expect actual + ' +done <<\EOF +0666 -rw-rw-rw- seventeen +0642 -rw-r---w- eightteen +EOF + test_done -- 2.13.1.519.g0a0746bea4
[PATCH v2 1/3] read-cache: use shared perms when writing shared index
Since f6ecc62dbf (write_shared_index(): use tempfile module, 2015-08-10)
write_shared_index() has been using mks_tempfile() to create the
temporary file that will become the shared index.
But even before that, it looks like the functions used to create this
file didn't call adjust_shared_perm(), which means that the shared
index file has always been created with 600 permissions regardless
of the shared permission settings.
Because of that, on repositories created with `git init --shared=all`
and using the split index feature, one gets an error like:
fatal: .git/sharedindex.a52f910b489bc462f187ab572ba0086f7b5157de: index file
open failed: Permission denied
when another user performs any operation that reads the shared index.
We could use create_tempfile() that calls adjust_shared_perm(), but
unfortunately create_tempfile() doesn't replace the XX at the end
of the path it is passed. So let's just call adjust_shared_perm() by
ourselves.
Signed-off-by: Christian Couder
---
read-cache.c | 8
1 file changed, 8 insertions(+)
diff --git a/read-cache.c b/read-cache.c
index bc156a133e..66f85f8d58 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2425,6 +2425,14 @@ static int write_shared_index(struct index_state *istate,
delete_tempfile(&temporary_sharedindex);
return ret;
}
+ ret = adjust_shared_perm(temporary_sharedindex.filename.buf);
+ if (ret) {
+ int save_errno = errno;
+ error("cannot fix permission bits on %s",
temporary_sharedindex.filename.buf);
+ delete_tempfile(&temporary_sharedindex);
+ errno = save_errno;
+ return ret;
+ }
ret = rename_tempfile(&temporary_sharedindex,
git_path("sharedindex.%s",
sha1_to_hex(si->base->sha1)));
if (!ret) {
--
2.13.1.519.g0a0746bea4
Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool
On Fri, Jun 23 2017, Jeff King jotted:
> On Fri, Jun 23, 2017 at 02:46:03PM +, Ævar Arnfjörð Bjarmason wrote:
>
>> Change the code for deciding what's to be done about %Z to stop
>> passing always either a NULL or "" char * to
>> strbuf_addftime(). Instead pass a boolean int to indicate whether the
>> strftime() %Z format should be omitted, which is what this code is
>> actually doing.
>>
>> This code grew organically between the changes in 9eafe86d58 ("Merge
>> branch 'rs/strbuf-addftime-zZ'", 2017-06-22) yielding an end result
>> that wasn't very readable. Out of context it looked as though the call
>> to strbuf_addstr() might be adding a custom tz_name to the string, but
>> actually tz_name would always be "", so the call to strbuf_addstr()
>> just to add an empty string to the format was pointless.
>
> The idea was that eventually the caller might be able to come up with a
> TZ that is not blank, but is also not what strftime("%Z") would produce.
> Conceivably that could be done if Git commits carried the "%Z"
> information (not likely), or if we used a reverse-lookup table (also not
> likely).
>
> This closes the door on that. Since we don't have immediate plans to go
> that route, I'm OK with this patch. It would be easy enough to re-open
> the door if we change our minds later.
Closes the door on doing that via passing the char * of the prepared
custom tz_name to strbuf_addftime().
I have a WIP patch (which may not make it on-list, depending) playing
with the idea I proposed in
CACBZZX5OQc45fUyDVayE89rkT=+8m5s4efsxcabcy7upme5...@mail.gmail.com which
just inserts the custom TZ name based on the offset inside that `if
(omit_strftime_tz_name)` branch.
That seems like a more straightforward way to do it than passing the
name to strbuf_addftime().
>> /**
>> * Add the time specified by `tm`, as formatted by `strftime`.
>> - * `tz_name` is used to expand %Z internally unless it's NULL.
>> * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
>> * of Greenwich, and it's used to expand %z internally. However, tokens
>> * with modifiers (e.g. %Ez) are passed to `strftime`.
>> + * `omit_strftime_tz_name` when set, means don't let `strftime` format
>> + * %Z, instead do our own formatting.
>
> Since we now always turn it into a blank string, perhaps "do our own
> formatting" could be more descriptive: we convert it into the empty
> string.
Then we'd need to change this comment again if we had some patch like
the one I mentioned above, I thought it was better to just leave this
vague enough that we didn't need to do that.
Re: [PATCH 1/3] read-cache: use shared perms when writing shared index
On Thu, Jun 22, 2017 at 9:51 PM, Junio C Hamano wrote:
> Christian Couder writes:
>> Let's fix that by using create_tempfile() instead of mks_tempfile()
>> to create the shared index file.
>>
>> ...
>> - fd = mks_tempfile(&temporary_sharedindex,
>> git_path("sharedindex_XX"));
>> + fd = create_tempfile(&temporary_sharedindex,
>> git_path("sharedindex_XX"));
>
> So we used to create a temporary file that made sure its name is
> unique but now we create sharedindex_XX with 6 X's literally
> at the end?
>
> Doesn't mks_tempfile() family include a variant where you can give
> custom mode? Better yet, perhaps you can call adjust_shared_perm()
> on the path _after_ seeing that mks_tempfile() succeeds (you can ask
> get_tempfile_path() which path to adjust, I presume)?
Yeah, adjust_shared_perm() is called after mks_tempfile() succeeds, in
the next version.
Re: [PATCH 2/3] t1301: move movebits() to test-lib-functions.sh
On Fri, Jun 23, 2017 at 1:09 AM, Ramsay Jones wrote: > > > On 22/06/17 20:52, Junio C Hamano wrote: >> Christian Couder writes: >> >>> As the movebits() function can be useful outside t1301, >>> let's move it into test-lib-functions.sh, and while at >>> it let's rename it test_movebits(). >> >> Good thinking, especially on the renaming. > > Err, except for the commit message! :-D > > Both the commit message subject and the commit message body > refer to _move_bits() rather than _mode_bits() etc. > (So, three instances of s/move/mode/). Yeah, sorry about that. This is fixed in the version I will send really soon now.
Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool
On Fri, Jun 23, 2017 at 02:46:03PM +, Ævar Arnfjörð Bjarmason wrote:
> Change the code for deciding what's to be done about %Z to stop
> passing always either a NULL or "" char * to
> strbuf_addftime(). Instead pass a boolean int to indicate whether the
> strftime() %Z format should be omitted, which is what this code is
> actually doing.
>
> This code grew organically between the changes in 9eafe86d58 ("Merge
> branch 'rs/strbuf-addftime-zZ'", 2017-06-22) yielding an end result
> that wasn't very readable. Out of context it looked as though the call
> to strbuf_addstr() might be adding a custom tz_name to the string, but
> actually tz_name would always be "", so the call to strbuf_addstr()
> just to add an empty string to the format was pointless.
The idea was that eventually the caller might be able to come up with a
TZ that is not blank, but is also not what strftime("%Z") would produce.
Conceivably that could be done if Git commits carried the "%Z"
information (not likely), or if we used a reverse-lookup table (also not
likely).
This closes the door on that. Since we don't have immediate plans to go
that route, I'm OK with this patch. It would be easy enough to re-open
the door if we change our minds later.
> /**
> * Add the time specified by `tm`, as formatted by `strftime`.
> - * `tz_name` is used to expand %Z internally unless it's NULL.
> * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
> * of Greenwich, and it's used to expand %z internally. However, tokens
> * with modifiers (e.g. %Ez) are passed to `strftime`.
> + * `omit_strftime_tz_name` when set, means don't let `strftime` format
> + * %Z, instead do our own formatting.
Since we now always turn it into a blank string, perhaps "do our own
formatting" could be more descriptive: we convert it into the empty
string.
-Peff
[PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Change the code for deciding what's to be done about %Z to stop
passing always either a NULL or "" char * to
strbuf_addftime(). Instead pass a boolean int to indicate whether the
strftime() %Z format should be omitted, which is what this code is
actually doing.
This code grew organically between the changes in 9eafe86d58 ("Merge
branch 'rs/strbuf-addftime-zZ'", 2017-06-22) yielding an end result
that wasn't very readable. Out of context it looked as though the call
to strbuf_addstr() might be adding a custom tz_name to the string, but
actually tz_name would always be "", so the call to strbuf_addstr()
just to add an empty string to the format was pointless.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
date.c | 2 +-
strbuf.c | 5 ++---
strbuf.h | 5 +++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/date.c b/date.c
index 1fd6d66375..5f09743bad 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const
struct date_mode *mode)
tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
else if (mode->type == DATE_STRFTIME)
strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
- mode->local ? NULL : "");
+ mode->local ? 0 : 1);
else
strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index be3b9e37b1..81ff3570e2 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -786,7 +786,7 @@ char *xstrfmt(const char *fmt, ...)
}
void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
-int tz_offset, const char *tz_name)
+int tz_offset, const int omit_strftime_tz_name)
{
struct strbuf munged_fmt = STRBUF_INIT;
size_t hint = 128;
@@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm,
fmt++;
break;
case 'Z':
- if (tz_name) {
- strbuf_addstr(&munged_fmt, tz_name);
+ if (omit_strftime_tz_name) {
fmt++;
break;
}
diff --git a/strbuf.h b/strbuf.h
index 4559035c47..bad698058a 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -340,14 +340,15 @@ extern void strbuf_vaddf(struct strbuf *sb, const char
*fmt, va_list ap);
/**
* Add the time specified by `tm`, as formatted by `strftime`.
- * `tz_name` is used to expand %Z internally unless it's NULL.
* `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
* of Greenwich, and it's used to expand %z internally. However, tokens
* with modifiers (e.g. %Ez) are passed to `strftime`.
+ * `omit_strftime_tz_name` when set, means don't let `strftime` format
+ * %Z, instead do our own formatting.
*/
extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm, int tz_offset,
- const char *tz_name);
+ const int omit_strftime_tz_name);
/**
* Read a given size of data from a FILE* pointer to the buffer.
--
2.13.1.611.g7e3b11ae1
Re: [RFC PATCH] proposal for refs/tracking/* hierarchy
On Fri, Jun 23, 2017 at 6:52 AM, Jacob Keller wrote: > From: Jacob Keller > > Historically, git has tracked the status of remote branches (heads) in > refs/remotes//*. This is necessary and useful as it allows users > to track the difference between their local work and the last known > status of the remote work. > > Unfortunately this hierarchy is limited in that it only tracks branches. > Additionally, it is not feasible to extend it directly, because the > layout would become ambiguous. For example, if a user happened to have > a local branch named "notes" it could be confusing if you tried to add > "refs/remotes/origin/notes/ with the already existing refs/remotes/origin/notes branch that existed. > > Instead, lets add support for a new refs/tracking/* hierarchy which is > laid out in such a way to avoid this inconsistency. All refs in > "refs/tracking//*" will include the complete ref, such that > dropping the "tracking/" part will give the exact ref name as it > is found in the upstream. Thus, we can track any ref here by simply > fetching it into refs/tracking//*. > > Add support to tell a new remote to start tracking remote hierarchies > via "--follow" which will track all refs under that section, ie: > > git remote add --follow notes origin will cause > > +refs/notes/*:refs/tracking/origin/notes/* to be added as a fetch > refspec for this remote. > > This ensures that any future refs which want a method of sharing the > current remote status separate from local status could use > refs/tracking > > A long term goal might be to deprecate refs/remotes/ in favor of > refs/tracking (possibly adding in magic to convert refs/remotes directly > into refs/tracking so that old stuff still works?). > > Things not yet thought through: > > 1) maybe we should create a configurable default so that some known set >of default refs get pulled (ie: notes, grafts, replace, etc?) >Possibly with some sort of easy way to add or subtract from the list >in config... > > 2) so far, there's no work done to figure out how to remove >refs/tracking when a remote is dropped. I *think* we can just delete >all refs under refs/tracking/ but I'm not completely certain > > 3) adding magic to complete refs under tracking, such as for git-notes >or similar may wish to understand shorthand for referencing the >remote version of notes > > 4) adding support for clone.. (this is weird because git-clone and >git-remote don't both use the same flow for setup.. oops??) > > 5) tests, documentation etc. (This is primarily an RFC, with the goal of >providing a known location for remote references such as notes to >reside) > > 6) we probably want to enable notes and grafts and other similar things >to use the remote tracking data if its available. > > 7) what about tags? Currently we fetch all tags into refs/tags directly, >which is a bit awkward, if for example you create a local tag and >a remote creates a tag with the same name, you simply don't get that >new version. This is good, but now you have no idea how to tell if >your tags are out of date or not. refs/tracking can partially resolve >this since remote tags will always be "exactly" what the remote has. >Unfortunately, I don't know how we'd resolve them into local tags... > Oops: 8) if we decided to go with "all refs always get tracked in refs/tracking" we probably want to either add a way to say "all refs except refs/tracking ones" or we want a way for servers to (by default) not advertise refs/tracking when clients fetch from them. That's partially why I went with the easier "only some hierarchies will get pulled by default" Otherwise, two remotes that fetch from each other could create a never ending cycle of refs/tracking/origin/tracking/origin/tracking/origin/ adding a new layer every time they fetched. Thanks, Jake
[RFC PATCH] proposal for refs/tracking/* hierarchy
From: Jacob Keller
Historically, git has tracked the status of remote branches (heads) in
refs/remotes//*. This is necessary and useful as it allows users
to track the difference between their local work and the last known
status of the remote work.
Unfortunately this hierarchy is limited in that it only tracks branches.
Additionally, it is not feasible to extend it directly, because the
layout would become ambiguous. For example, if a user happened to have
a local branch named "notes" it could be confusing if you tried to add
"refs/remotes/origin/notes//*" will include the complete ref, such that
dropping the "tracking/" part will give the exact ref name as it
is found in the upstream. Thus, we can track any ref here by simply
fetching it into refs/tracking//*.
Add support to tell a new remote to start tracking remote hierarchies
via "--follow" which will track all refs under that section, ie:
git remote add --follow notes origin will cause
+refs/notes/*:refs/tracking/origin/notes/* to be added as a fetch
refspec for this remote.
This ensures that any future refs which want a method of sharing the
current remote status separate from local status could use
refs/tracking
A long term goal might be to deprecate refs/remotes/ in favor of
refs/tracking (possibly adding in magic to convert refs/remotes directly
into refs/tracking so that old stuff still works?).
Things not yet thought through:
1) maybe we should create a configurable default so that some known set
of default refs get pulled (ie: notes, grafts, replace, etc?)
Possibly with some sort of easy way to add or subtract from the list
in config...
2) so far, there's no work done to figure out how to remove
refs/tracking when a remote is dropped. I *think* we can just delete
all refs under refs/tracking/ but I'm not completely certain
3) adding magic to complete refs under tracking, such as for git-notes
or similar may wish to understand shorthand for referencing the
remote version of notes
4) adding support for clone.. (this is weird because git-clone and
git-remote don't both use the same flow for setup.. oops??)
5) tests, documentation etc. (This is primarily an RFC, with the goal of
providing a known location for remote references such as notes to
reside)
6) we probably want to enable notes and grafts and other similar things
to use the remote tracking data if its available.
7) what about tags? Currently we fetch all tags into refs/tags directly,
which is a bit awkward, if for example you create a local tag and
a remote creates a tag with the same name, you simply don't get that
new version. This is good, but now you have no idea how to tell if
your tags are out of date or not. refs/tracking can partially resolve
this since remote tags will always be "exactly" what the remote has.
Unfortunately, I don't know how we'd resolve them into local tags...
Probably other things missing too...
Signed-off-by: Jacob Keller
---
builtin/remote.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/builtin/remote.c b/builtin/remote.c
index f1a88fe26589..dffe3892be11 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -122,6 +122,16 @@ static void add_branch(const char *key, const char
*branchname,
git_config_set_multivar(key, tmp->buf, "^$", 0);
}
+static void add_tracking(const char *key, const char *namespace,
+const char *remotename, struct strbuf *tmp)
+{
+ strbuf_reset(tmp);
+ strbuf_addch(tmp, '+');
+ strbuf_addf(tmp, "refs/%s/*:refs/tracking/%s/%s/*",
+ namespace, remotename, namespace);
+ git_config_set_multivar(key, tmp->buf, "^$", 0);
+}
+
static const char mirror_advice[] =
N_("--mirror is dangerous and deprecated; please\n"
"\t use --mirror=fetch or --mirror=push instead");
@@ -149,6 +159,7 @@ static int add(int argc, const char **argv)
int fetch = 0, fetch_tags = TAGS_DEFAULT;
unsigned mirror = MIRROR_NONE;
struct string_list track = STRING_LIST_INIT_NODUP;
+ struct string_list follow = STRING_LIST_INIT_NODUP;
const char *master = NULL;
struct remote *remote;
struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
@@ -164,6 +175,8 @@ static int add(int argc, const char **argv)
N_("or do not fetch any tag at all (--no-tags)"),
TAGS_UNSET),
OPT_STRING_LIST('t', "track", &track, N_("branch"),
N_("branch(es) to track")),
+ OPT_STRING_LIST(0, "follow", &follow, N_("namespace"),
+ N_("refs namespaces to follow in
refs/tracking")),
OPT_STRING('m', "master", &master, N_("branch"), N_("master
branch")),
{ OPTION_CALLBACK, 0, "mirror", &mirror, N_("push|fetch"),
N_("set up remote as a mirror to push to or fetch
from"),
@@ -207,6 +220,11 @@ static int add(int a
[PATCH] xdiff: trim common tail with -U0 after diff
When -U0 is used, trim_common_tail should be called after `xdl_diff`, so
that `--indent-heuristic` (and other diff processing) works as expected.
It also removes the check for `!(xecfg->flags & XDL_EMIT_FUNCCONTEXT)`
added in e0876bca4, which does not appear to be necessary anymore after
moving the trimming down.
This only adds a test to t4061-diff-indent.sh, but should also have one for
normal (i.e. non-experimental) diff mode probably?!
Ref: https://github.com/tomtom/quickfixsigns_vim/issues/74#issue-237900460
---
t/t4061-diff-indent.sh | 15 +++
xdiff-interface.c | 7 ---
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
index 2affd7a10..df3151393 100755
--- a/t/t4061-diff-indent.sh
+++ b/t/t4061-diff-indent.sh
@@ -116,6 +116,16 @@ test_expect_success 'prepare' '
4
EOF
+ cat <<-\EOF >spaces-compacted-U0-expect &&
+ diff --git a/spaces.txt b/spaces.txt
+ --- a/spaces.txt
+ +++ b/spaces.txt
+ @@ -4,0 +5,3 @@ a
+ +b
+ +a
+ +
+ EOF
+
tr "_" " " <<-\EOF >functions-expect &&
diff --git a/functions.c b/functions.c
--- a/functions.c
@@ -184,6 +194,11 @@ test_expect_success 'diff: --indent-heuristic with
--histogram' '
compare_diff spaces-compacted-expect out-compacted4
'
+test_expect_success 'diff: --indent-heuristic with -U0' '
+ git diff -U0 --indent-heuristic old new -- spaces.txt >out-compacted5 &&
+ compare_diff spaces-compacted-U0-expect out-compacted5
+'
+
test_expect_success 'diff: ugly functions' '
git diff --no-indent-heuristic old new -- functions.c >out &&
compare_diff functions-expect out
diff --git a/xdiff-interface.c b/xdiff-interface.c
index d3f78ca2a..a7e0e3583 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -125,16 +125,17 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)
int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t
const *xecfg, xdemitcb_t *xecb)
{
+ int ret;
mmfile_t a = *mf1;
mmfile_t b = *mf2;
if (mf1->size > MAX_XDIFF_SIZE || mf2->size > MAX_XDIFF_SIZE)
return -1;
- if (!xecfg->ctxlen && !(xecfg->flags & XDL_EMIT_FUNCCONTEXT))
+ ret = xdl_diff(&a, &b, xpp, xecfg, xecb);
+ if (ret && !xecfg->ctxlen)
trim_common_tail(&a, &b);
-
- return xdl_diff(&a, &b, xpp, xecfg, xecb);
+ return ret;
}
int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
--
2.13.1
Re: [PATCH v3 0/4] Add regression tests for recent rebase -i fixes
On 23/06/17 06:07, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>>> git-rebase.sh | 4 +-
>>> sequencer.c | 11 ++--
>>> t/t3404-rebase-interactive.sh | 7 +++
>>> t/t3420-rebase-autostash.sh | 136
>>> --
>>> 4 files changed, 147 insertions(+), 11 deletions(-)
>>
>> I've merged this to 'next' but I probably shouldn't have before
>> making sure that Travis tests passes 'pu' while this was still in
>> there.
>>
>> At least t3420 seems to fail under GETTEXT_POISON build.
>>
>> https://travis-ci.org/git/git/jobs/245990993
>
> This should be sufficient to make t3420 pass. It seems that t3404
> is also broken under GETTEXT_POISON build, but I won't have time to
> look at it, at least tonight.
>
> $ make GETTEXT_POISON=YesPlease
> $ cd t && sh ./t3404-*.sh -i -v
>
> to see how it breaks.
t3404 passes for me,
$ make GETTEXT_POISON=YesPlease
$ cd t &&sh t3404-rebase-interactive.sh -i -v
...
# still have 1 known breakage(s)
# passed all remaining 95 test(s)
1..96
Also as far as I can see it passes on travis -
https://travis-ci.org/git/git/jobs/245990993#L910 have I missed
something? Do you want me to submit a fixup patch for t3420 or have you
got one already?
Thanks
Phillip
> Thanks.
>
> t/t3420-rebase-autostash.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index 6826c38cbd..e243700660 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -178,7 +178,7 @@ testrebase () {
> test_when_finished git branch -D rebased-feature-branch &&
> suffix=${type#\ --} && suffix=${suffix:-am} &&
> create_expected_success_$suffix &&
> - test_cmp expected actual
> + test_i18ncmp expected actual
> '
>
> test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
> @@ -275,7 +275,7 @@ testrebase () {
> test_when_finished git branch -D rebased-feature-branch &&
> suffix=${type#\ --} && suffix=${suffix:-am} &&
> create_expected_failure_$suffix &&
> - test_cmp expected actual
> + test_i18ncmp expected actual
> '
> }
>
>
Re: What's cooking in git.git (Jun 2017, #06; Thu, 22)
> On 23 Jun 2017, at 00:35, Junio C Hamano wrote: > > Here are the topics that have been cooking. Commits prefixed with > '-' are only in 'pu' (proposed updates) while commits prefixed with > '+' are in 'next'. The ones marked with '.' do not appear in any of > the integration branches, but I am still holding onto them. > > You can find the changes described here in the integration branches > of the repositories listed at > >http://git-blame.blogspot.com/p/git-public-repositories.html > > -- > ... > > * ls/filter-process-delayed (2017-06-01) 5 commits > - convert: add "status=delayed" to filter process protocol > - convert: move multiple file filter error handling to separate function > - t0021: write "OUT" only on success > - t0021: make debug log file name configurable > - t0021: keep filter log files on comparison > > The filter-process interface learned to allow a process with long > latency give a "delayed" response. > > Needs review. I am still desperately looking for reviewers! It would be awesome if this feature would have a chance to go into 2.14 :-) > * pw/rebase-i-regression-fix-tests (2017-06-19) 4 commits > (merged to 'next' on 2017-06-22 at d1dde1672a) > + rebase: add more regression tests for console output > + rebase: add regression tests for console output > + rebase -i: add test for reflog message > + sequencer: print autostash messages to stderr > > Fix a recent regression to "git rebase -i" and add tests that would > have caught it and others. > > Will merge to 'master'. I think this series breaks t3420-rebase-autostash.sh with GETTEXT_POISON=YesPlease See: https://travis-ci.org/git/git/jobs/245990993 - Lars
[PATCH v2 25/29] packed_refs_unlock(), packed_refs_is_locked(): new functions
Add two new public functions, `packed_refs_unlock()` and
`packed_refs_is_locked()`, with which callers can manage and query the
`packed-refs` lock externally.
Call `packed_refs_unlock()` from `commit_packed_refs()` and
`rollback_packed_refs()`.
Signed-off-by: Michael Haggerty
---
refs/packed-backend.c | 31 +--
refs/packed-backend.h | 3 +++
2 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 78e877a9e3..f27943f9a1 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -563,6 +563,29 @@ int packed_refs_lock(struct ref_store *ref_store, int
flags, struct strbuf *err)
return 0;
}
+void packed_refs_unlock(struct ref_store *ref_store)
+{
+ struct packed_ref_store *refs = packed_downcast(
+ ref_store,
+ REF_STORE_READ | REF_STORE_WRITE,
+ "packed_refs_unlock");
+
+ if (!is_lock_file_locked(&refs->lock))
+ die("BUG: packed_refs_unlock() called when not locked");
+ rollback_lock_file(&refs->lock);
+ release_packed_ref_cache(refs->cache);
+}
+
+int packed_refs_is_locked(struct ref_store *ref_store)
+{
+ struct packed_ref_store *refs = packed_downcast(
+ ref_store,
+ REF_STORE_READ | REF_STORE_WRITE,
+ "packed_refs_is_locked");
+
+ return is_lock_file_locked(&refs->lock);
+}
+
/*
* The packed-refs header line that we write out. Perhaps other
* traits will be added later. The trailing space is required.
@@ -649,8 +672,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct
strbuf *err)
delete_tempfile(&refs->tempfile);
out:
- rollback_lock_file(&refs->lock);
- release_packed_ref_cache(packed_ref_cache);
+ packed_refs_unlock(ref_store);
return ret;
}
@@ -661,14 +683,11 @@ int commit_packed_refs(struct ref_store *ref_store,
struct strbuf *err)
*/
static void rollback_packed_refs(struct packed_ref_store *refs)
{
- struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
-
packed_assert_main_repository(refs, "rollback_packed_refs");
if (!is_lock_file_locked(&refs->lock))
die("BUG: packed-refs not locked");
- rollback_lock_file(&refs->lock);
- release_packed_ref_cache(packed_ref_cache);
+ packed_refs_unlock(&refs->base);
clear_packed_ref_cache(refs);
}
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index 210e3f35ce..03b7c1de95 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -11,6 +11,9 @@ struct ref_store *packed_ref_store_create(const char *path,
*/
int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf
*err);
+void packed_refs_unlock(struct ref_store *ref_store);
+int packed_refs_is_locked(struct ref_store *ref_store);
+
void add_packed_ref(struct ref_store *ref_store,
const char *refname, const struct object_id *oid);
--
2.11.0
[PATCH v2 23/29] packed_refs_lock(): function renamed from lock_packed_refs()
Rename `lock_packed_refs()` to `packed_refs_lock()` for consistency
with how other methods are named. Also, it's about to get some
companions.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 4 ++--
refs/packed-backend.c | 10 +-
refs/packed-backend.h | 2 +-
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2810785efc..88de907148 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1096,7 +1096,7 @@ static int files_pack_refs(struct ref_store *ref_store,
unsigned int flags)
struct ref_to_prune *refs_to_prune = NULL;
struct strbuf err = STRBUF_INIT;
- lock_packed_refs(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
+ packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);
while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
@@ -2679,7 +2679,7 @@ static int files_initial_transaction_commit(struct
ref_store *ref_store,
}
}
- if (lock_packed_refs(refs->packed_ref_store, 0)) {
+ if (packed_refs_lock(refs->packed_ref_store, 0)) {
strbuf_addf(err, "unable to lock packed-refs file: %s",
strerror(errno));
ret = TRANSACTION_GENERIC_ERROR;
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 71f92ed6f0..cd214e07a1 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -321,7 +321,7 @@ static struct ref_dir *get_packed_refs(struct
packed_ref_store *refs)
/*
* Add or overwrite a reference in the in-memory packed reference
* cache. This may only be called while the packed-refs file is locked
- * (see lock_packed_refs()). To actually write the packed-refs file,
+ * (see packed_refs_lock()). To actually write the packed-refs file,
* call commit_packed_refs().
*/
void add_packed_ref(struct ref_store *ref_store,
@@ -515,11 +515,11 @@ static int write_packed_entry(FILE *fh, const char
*refname,
return 0;
}
-int lock_packed_refs(struct ref_store *ref_store, int flags)
+int packed_refs_lock(struct ref_store *ref_store, int flags)
{
struct packed_ref_store *refs =
packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
- "lock_packed_refs");
+ "packed_refs_lock");
static int timeout_configured = 0;
static int timeout_value = 1000;
struct packed_ref_cache *packed_ref_cache;
@@ -567,7 +567,7 @@ static const char PACKED_REFS_HEADER[] =
/*
* Write the current version of the packed refs cache from memory to
* disk. The packed-refs file must already be locked for writing (see
- * lock_packed_refs()). Return zero on success. On errors, rollback
+ * packed_refs_lock()). Return zero on success. On errors, rollback
* the lockfile, write an error message to `err`, and return a nonzero
* value.
*/
@@ -698,7 +698,7 @@ int repack_without_refs(struct ref_store *ref_store,
if (!needs_repacking)
return 0; /* no refname exists in packed refs */
- if (lock_packed_refs(&refs->base, 0)) {
+ if (packed_refs_lock(&refs->base, 0)) {
unable_to_lock_message(refs->path, errno, err);
return -1;
}
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index 3d4057b65b..dbc00d3396 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -9,7 +9,7 @@ struct ref_store *packed_ref_store_create(const char *path,
* hold_lock_file_for_update(). Return 0 on success. On errors, set
* errno appropriately and return a nonzero value.
*/
-int lock_packed_refs(struct ref_store *ref_store, int flags);
+int packed_refs_lock(struct ref_store *ref_store, int flags);
void add_packed_ref(struct ref_store *ref_store,
const char *refname, const struct object_id *oid);
--
2.11.0
[PATCH v2 18/29] packed_read_raw_ref(): new function, replacing `resolve_packed_ref()`
Add a new function, `packed_read_raw_ref()`, which is nearly a
`read_raw_ref_fn`. Use it in place of `resolve_packed_ref()`.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 36 +---
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0490cc087e..346794cf7c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -608,27 +608,23 @@ static struct ref_entry *get_packed_ref(struct
packed_ref_store *refs,
return find_ref_entry(get_packed_refs(refs), refname);
}
-/*
- * A loose ref file doesn't exist; check for a packed ref.
- */
-static int resolve_packed_ref(struct files_ref_store *refs,
- const char *refname,
- unsigned char *sha1, unsigned int *flags)
+static int packed_read_raw_ref(struct packed_ref_store *refs,
+ const char *refname, unsigned char *sha1,
+ struct strbuf *referent, unsigned int *type)
{
struct ref_entry *entry;
- /*
-* The loose reference file does not exist; check for a packed
-* reference.
-*/
- entry = get_packed_ref(refs->packed_ref_store, refname);
- if (entry) {
- hashcpy(sha1, entry->u.value.oid.hash);
- *flags |= REF_ISPACKED;
- return 0;
+ *type = 0;
+
+ entry = get_packed_ref(refs, refname);
+ if (!entry) {
+ errno = ENOENT;
+ return -1;
}
- /* refname is not a packed reference. */
- return -1;
+
+ hashcpy(sha1, entry->u.value.oid.hash);
+ *type = REF_ISPACKED;
+ return 0;
}
static int files_read_raw_ref(struct ref_store *ref_store,
@@ -674,7 +670,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
if (lstat(path, &st) < 0) {
if (errno != ENOENT)
goto out;
- if (resolve_packed_ref(refs, refname, sha1, type)) {
+ if (packed_read_raw_ref(refs->packed_ref_store, refname,
+ sha1, referent, type)) {
errno = ENOENT;
goto out;
}
@@ -713,7 +710,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
* ref is supposed to be, there could still be a
* packed ref:
*/
- if (resolve_packed_ref(refs, refname, sha1, type)) {
+ if (packed_read_raw_ref(refs->packed_ref_store, refname,
+ sha1, referent, type)) {
errno = EISDIR;
goto out;
}
--
2.11.0
[PATCH v2 15/29] repack_without_refs(): take a `packed_ref_store *` parameter
It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2b9d93d3b6..c206791b91 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1621,19 +1621,19 @@ static int files_pack_refs(struct ref_store *ref_store,
unsigned int flags)
*
* The refs in 'refnames' needn't be sorted. `err` must not be NULL.
*/
-static int repack_without_refs(struct files_ref_store *refs,
+static int repack_without_refs(struct packed_ref_store *refs,
struct string_list *refnames, struct strbuf *err)
{
struct ref_dir *packed;
struct string_list_item *refname;
int ret, needs_repacking = 0, removed = 0;
- files_assert_main_repository(refs, "repack_without_refs");
+ packed_assert_main_repository(refs, "repack_without_refs");
assert(err);
/* Look for a packed ref */
for_each_string_list_item(refname, refnames) {
- if (get_packed_ref(refs->packed_ref_store, refname->string)) {
+ if (get_packed_ref(refs, refname->string)) {
needs_repacking = 1;
break;
}
@@ -1643,11 +1643,11 @@ static int repack_without_refs(struct files_ref_store
*refs,
if (!needs_repacking)
return 0; /* no refname exists in packed refs */
- if (lock_packed_refs(refs->packed_ref_store, 0)) {
- unable_to_lock_message(refs->packed_ref_store->path, errno,
err);
+ if (lock_packed_refs(refs, 0)) {
+ unable_to_lock_message(refs->path, errno, err);
return -1;
}
- packed = get_packed_refs(refs->packed_ref_store);
+ packed = get_packed_refs(refs);
/* Remove refnames from the cache */
for_each_string_list_item(refname, refnames)
@@ -1658,12 +1658,12 @@ static int repack_without_refs(struct files_ref_store
*refs,
* All packed entries disappeared while we were
* acquiring the lock.
*/
- rollback_packed_refs(refs->packed_ref_store);
+ rollback_packed_refs(refs);
return 0;
}
/* Write what remains */
- ret = commit_packed_refs(refs->packed_ref_store);
+ ret = commit_packed_refs(refs);
if (ret)
strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
strerror(errno));
@@ -1681,7 +1681,7 @@ static int files_delete_refs(struct ref_store *ref_store,
const char *msg,
if (!refnames->nr)
return 0;
- result = repack_without_refs(refs, refnames, &err);
+ result = repack_without_refs(refs->packed_ref_store, refnames, &err);
if (result) {
/*
* If we failed to rewrite the packed-refs file, then
@@ -3101,7 +3101,7 @@ static int files_transaction_finish(struct ref_store
*ref_store,
}
}
- if (repack_without_refs(refs, &refs_to_delete, err)) {
+ if (repack_without_refs(refs->packed_ref_store, &refs_to_delete, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
--
2.11.0
[PATCH v2 11/29] lock_packed_refs(): take a `packed_ref_store *` parameter
It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 31 ++-
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4943207098..0d8f39089f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -80,6 +80,19 @@ static struct packed_ref_store *packed_ref_store_create(
return refs;
}
+/*
+ * Die if refs is not the main ref store. caller is used in any
+ * necessary error messages.
+ */
+static void packed_assert_main_repository(struct packed_ref_store *refs,
+ const char *caller)
+{
+ if (refs->store_flags & REF_STORE_MAIN)
+ return;
+
+ die("BUG: operation %s only allowed for main ref store", caller);
+}
+
/*
* Future: need to be in "struct repository"
* when doing a full libification.
@@ -1334,13 +1347,13 @@ static void write_packed_entry(FILE *fh, const char
*refname,
* hold_lock_file_for_update(). Return 0 on success. On errors, set
* errno appropriately and return a nonzero value.
*/
-static int lock_packed_refs(struct files_ref_store *refs, int flags)
+static int lock_packed_refs(struct packed_ref_store *refs, int flags)
{
static int timeout_configured = 0;
static int timeout_value = 1000;
struct packed_ref_cache *packed_ref_cache;
- files_assert_main_repository(refs, "lock_packed_refs");
+ packed_assert_main_repository(refs, "lock_packed_refs");
if (!timeout_configured) {
git_config_get_int("core.packedrefstimeout", &timeout_value);
@@ -1348,8 +1361,8 @@ static int lock_packed_refs(struct files_ref_store *refs,
int flags)
}
if (hold_lock_file_for_update_timeout(
- &refs->packed_ref_store->lock,
- refs->packed_ref_store->path,
+ &refs->lock,
+ refs->path,
flags, timeout_value) < 0)
return -1;
@@ -1361,9 +1374,9 @@ static int lock_packed_refs(struct files_ref_store *refs,
int flags)
* cache is still valid. We've just locked the file, but it
* might have changed the moment *before* we locked it.
*/
- validate_packed_ref_cache(refs->packed_ref_store);
+ validate_packed_ref_cache(refs);
- packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);
+ packed_ref_cache = get_packed_ref_cache(refs);
/* Increment the reference count to prevent it from being freed: */
acquire_packed_ref_cache(packed_ref_cache);
return 0;
@@ -1560,7 +1573,7 @@ static int files_pack_refs(struct ref_store *ref_store,
unsigned int flags)
int ok;
struct ref_to_prune *refs_to_prune = NULL;
- lock_packed_refs(refs, LOCK_DIE_ON_ERROR);
+ lock_packed_refs(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);
while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
@@ -1629,7 +1642,7 @@ static int repack_without_refs(struct files_ref_store
*refs,
if (!needs_repacking)
return 0; /* no refname exists in packed refs */
- if (lock_packed_refs(refs, 0)) {
+ if (lock_packed_refs(refs->packed_ref_store, 0)) {
unable_to_lock_message(refs->packed_ref_store->path, errno,
err);
return -1;
}
@@ -3198,7 +3211,7 @@ static int files_initial_transaction_commit(struct
ref_store *ref_store,
}
}
- if (lock_packed_refs(refs, 0)) {
+ if (lock_packed_refs(refs->packed_ref_store, 0)) {
strbuf_addf(err, "unable to lock packed-refs file: %s",
strerror(errno));
ret = TRANSACTION_GENERIC_ERROR;
--
2.11.0
[PATCH v2 26/29] clear_packed_ref_cache(): don't protest if the lock is held
The existing callers already check that the lock isn't held just
before calling `clear_packed_ref_cache()`, and in the near future we
want to be able to call this function when the lock is held.
Signed-off-by: Michael Haggerty
---
refs/packed-backend.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index f27943f9a1..96d92a5eea 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -133,8 +133,6 @@ static void clear_packed_ref_cache(struct packed_ref_store
*refs)
if (refs->cache) {
struct packed_ref_cache *cache = refs->cache;
- if (is_lock_file_locked(&refs->lock))
- die("BUG: packed-ref cache cleared while locked");
refs->cache = NULL;
release_packed_ref_cache(cache);
}
--
2.11.0
[PATCH v2 10/29] add_packed_ref(): take a `packed_ref_store *` parameter
It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bc5c0de84e..4943207098 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -438,19 +438,19 @@ static struct ref_dir *get_packed_refs(struct
packed_ref_store *refs)
* (see lock_packed_refs()). To actually write the packed-refs file,
* call commit_packed_refs().
*/
-static void add_packed_ref(struct files_ref_store *refs,
+static void add_packed_ref(struct packed_ref_store *refs,
const char *refname, const struct object_id *oid)
{
struct ref_dir *packed_refs;
struct ref_entry *packed_entry;
- if (!is_lock_file_locked(&refs->packed_ref_store->lock))
+ if (!is_lock_file_locked(&refs->lock))
die("BUG: packed refs not locked");
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
die("Reference has invalid format: '%s'", refname);
- packed_refs = get_packed_refs(refs->packed_ref_store);
+ packed_refs = get_packed_refs(refs);
packed_entry = find_ref_entry(packed_refs, refname);
if (packed_entry) {
/* Overwrite the existing entry: */
@@ -1579,7 +1579,7 @@ static int files_pack_refs(struct ref_store *ref_store,
unsigned int flags)
* we don't copy the peeled status, because we want it
* to be re-peeled.
*/
- add_packed_ref(refs, iter->refname, iter->oid);
+ add_packed_ref(refs->packed_ref_store, iter->refname,
iter->oid);
/* Schedule the loose reference for pruning if requested. */
if ((flags & PACK_REFS_PRUNE)) {
@@ -3210,7 +3210,7 @@ static int files_initial_transaction_commit(struct
ref_store *ref_store,
if ((update->flags & REF_HAVE_NEW) &&
!is_null_oid(&update->new_oid))
- add_packed_ref(refs, update->refname,
+ add_packed_ref(refs->packed_ref_store, update->refname,
&update->new_oid);
}
--
2.11.0
[PATCH v2 08/29] get_packed_ref_cache(): take a `packed_ref_store *` parameter
It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 26 --
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f061506bf0..b2ef7b3bb9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -404,24 +404,22 @@ static void validate_packed_ref_cache(struct
packed_ref_store *refs)
}
/*
- * Get the packed_ref_cache for the specified files_ref_store,
+ * Get the packed_ref_cache for the specified packed_ref_store,
* creating and populating it if it hasn't been read before or if the
* file has been changed (according to its `validity` field) since it
* was last read. On the other hand, if we hold the lock, then assume
* that the file hasn't been changed out from under us, so skip the
* extra `stat()` call in `stat_validity_check()`.
*/
-static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store
*refs)
+static struct packed_ref_cache *get_packed_ref_cache(struct packed_ref_store
*refs)
{
- const char *packed_refs_file = refs->packed_ref_store->path;
+ if (!is_lock_file_locked(&refs->lock))
+ validate_packed_ref_cache(refs);
- if (!is_lock_file_locked(&refs->packed_ref_store->lock))
- validate_packed_ref_cache(refs->packed_ref_store);
-
- if (!refs->packed_ref_store->cache)
- refs->packed_ref_store->cache =
read_packed_refs(packed_refs_file);
+ if (!refs->cache)
+ refs->cache = read_packed_refs(refs->path);
- return refs->packed_ref_store->cache;
+ return refs->cache;
}
static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache
*packed_ref_cache)
@@ -431,7 +429,7 @@ static struct ref_dir *get_packed_ref_dir(struct
packed_ref_cache *packed_ref_ca
static struct ref_dir *get_packed_refs(struct files_ref_store *refs)
{
- return get_packed_ref_dir(get_packed_ref_cache(refs));
+ return get_packed_ref_dir(get_packed_ref_cache(refs->packed_ref_store));
}
/*
@@ -1151,7 +1149,7 @@ static struct ref_iterator *files_ref_iterator_begin(
loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
prefix, 1);
- iter->packed_ref_cache = get_packed_ref_cache(refs);
+ iter->packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);
acquire_packed_ref_cache(iter->packed_ref_cache);
packed_iter = cache_ref_iterator_begin(iter->packed_ref_cache->cache,
prefix, 0);
@@ -1365,7 +1363,7 @@ static int lock_packed_refs(struct files_ref_store *refs,
int flags)
*/
validate_packed_ref_cache(refs->packed_ref_store);
- packed_ref_cache = get_packed_ref_cache(refs);
+ packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);
/* Increment the reference count to prevent it from being freed: */
acquire_packed_ref_cache(packed_ref_cache);
return 0;
@@ -1380,7 +1378,7 @@ static int lock_packed_refs(struct files_ref_store *refs,
int flags)
static int commit_packed_refs(struct files_ref_store *refs)
{
struct packed_ref_cache *packed_ref_cache =
- get_packed_ref_cache(refs);
+ get_packed_ref_cache(refs->packed_ref_store);
int ok, error = 0;
int save_errno = 0;
FILE *out;
@@ -1426,7 +1424,7 @@ static int commit_packed_refs(struct files_ref_store
*refs)
static void rollback_packed_refs(struct files_ref_store *refs)
{
struct packed_ref_cache *packed_ref_cache =
- get_packed_ref_cache(refs);
+ get_packed_ref_cache(refs->packed_ref_store);
files_assert_main_repository(refs, "rollback_packed_refs");
--
2.11.0
[PATCH v2 29/29] read_packed_refs(): die if `packed-refs` contains bogus data
The old code ignored any lines that it didn't understand. This is
dangerous. Instead, `die()` if the `packed-refs` file contains any
lines that we don't know how to handle.
Signed-off-by: Michael Haggerty
---
refs/packed-backend.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 377c775adb..1cbaf036df 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -253,9 +253,7 @@ static struct packed_ref_cache *read_packed_refs(const char
*packed_refs_file)
(peeled == PEELED_TAGS && starts_with(refname,
"refs/tags/")))
last->flag |= REF_KNOWS_PEELED;
add_ref_entry(dir, last);
- continue;
- }
- if (last &&
+ } else if (last &&
line.buf[0] == '^' &&
line.len == PEELED_LINE_LENGTH &&
line.buf[PEELED_LINE_LENGTH - 1] == '\n' &&
@@ -267,6 +265,8 @@ static struct packed_ref_cache *read_packed_refs(const char
*packed_refs_file)
* reference:
*/
last->flag |= REF_KNOWS_PEELED;
+ } else {
+ die("unexpected line in %s: %s", packed_refs_file,
line.buf);
}
}
--
2.11.0
[PATCH v2 27/29] commit_packed_refs(): remove call to `packed_refs_unlock()`
Instead, change the callers of `commit_packed_refs()` to call
`packed_refs_unlock()`.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 2 ++
refs/packed-backend.c | 18 --
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8ea4e9ab05..93bdc8f0c8 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1131,6 +1131,7 @@ static int files_pack_refs(struct ref_store *ref_store,
unsigned int flags)
if (commit_packed_refs(refs->packed_ref_store, &err))
die("unable to overwrite old ref-pack file: %s", err.buf);
+ packed_refs_unlock(refs->packed_ref_store);
prune_refs(refs, refs_to_prune);
strbuf_release(&err);
@@ -2699,6 +2700,7 @@ static int files_initial_transaction_commit(struct
ref_store *ref_store,
}
cleanup:
+ packed_refs_unlock(refs->packed_ref_store);
transaction->state = REF_TRANSACTION_CLOSED;
string_list_clear(&affected_refnames, 0);
return ret;
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 96d92a5eea..5cf6b3d40e 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -606,7 +606,6 @@ int commit_packed_refs(struct ref_store *ref_store, struct
strbuf *err)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(refs);
int ok;
- int ret = -1;
struct strbuf sb = STRBUF_INIT;
FILE *out;
struct ref_iterator *iter;
@@ -619,7 +618,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct
strbuf *err)
strbuf_addf(err, "unable to create file %s: %s",
sb.buf, strerror(errno));
strbuf_release(&sb);
- goto out;
+ return -1;
}
strbuf_release(&sb);
@@ -660,18 +659,14 @@ int commit_packed_refs(struct ref_store *ref_store,
struct strbuf *err)
if (rename_tempfile(&refs->tempfile, refs->path)) {
strbuf_addf(err, "error replacing %s: %s",
refs->path, strerror(errno));
- goto out;
+ return -1;
}
- ret = 0;
- goto out;
+ return 0;
error:
delete_tempfile(&refs->tempfile);
-
-out:
- packed_refs_unlock(ref_store);
- return ret;
+ return -1;
}
/*
@@ -705,6 +700,7 @@ int repack_without_refs(struct ref_store *ref_store,
struct ref_dir *packed;
struct string_list_item *refname;
int needs_repacking = 0, removed = 0;
+ int ret;
packed_assert_main_repository(refs, "repack_without_refs");
assert(err);
@@ -740,7 +736,9 @@ int repack_without_refs(struct ref_store *ref_store,
}
/* Write what remains */
- return commit_packed_refs(&refs->base, err);
+ ret = commit_packed_refs(&refs->base, err);
+ packed_refs_unlock(ref_store);
+ return ret;
}
static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)
--
2.11.0
[PATCH v2 28/29] repack_without_refs(): don't lock or unlock the packed refs
Change `repack_without_refs()` to expect the packed-refs lock to be
held already, and not to release the lock before returning. Change the
callers to deal with lock management.
This change makes it possible for callers to hold the packed-refs lock
for a longer span of time, a possibility that will eventually make it
possible to fix some longstanding races.
The only semantic change here is that `repack_without_refs()` used to
forgot to release the lock in the `if (!removed)` exit path. That
omission is now fixed.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 47 +++
refs/packed-backend.c | 32
2 files changed, 39 insertions(+), 40 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 93bdc8f0c8..e9b95592b6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1149,24 +1149,16 @@ static int files_delete_refs(struct ref_store
*ref_store, const char *msg,
if (!refnames->nr)
return 0;
- result = repack_without_refs(refs->packed_ref_store, refnames, &err);
- if (result) {
- /*
-* If we failed to rewrite the packed-refs file, then
-* it is unsafe to try to remove loose refs, because
-* doing so might expose an obsolete packed value for
-* a reference that might even point at an object that
-* has been garbage collected.
-*/
- if (refnames->nr == 1)
- error(_("could not delete reference %s: %s"),
- refnames->items[0].string, err.buf);
- else
- error(_("could not delete references: %s"), err.buf);
+ if (packed_refs_lock(refs->packed_ref_store, 0, &err))
+ goto error;
- goto out;
+ if (repack_without_refs(refs->packed_ref_store, refnames, &err)) {
+ packed_refs_unlock(refs->packed_ref_store);
+ goto error;
}
+ packed_refs_unlock(refs->packed_ref_store);
+
for (i = 0; i < refnames->nr; i++) {
const char *refname = refnames->items[i].string;
@@ -1174,9 +1166,24 @@ static int files_delete_refs(struct ref_store
*ref_store, const char *msg,
result |= error(_("could not remove reference %s"),
refname);
}
-out:
strbuf_release(&err);
return result;
+
+error:
+ /*
+* If we failed to rewrite the packed-refs file, then it is
+* unsafe to try to remove loose refs, because doing so might
+* expose an obsolete packed value for a reference that might
+* even point at an object that has been garbage collected.
+*/
+ if (refnames->nr == 1)
+ error(_("could not delete reference %s: %s"),
+ refnames->items[0].string, err.buf);
+ else
+ error(_("could not delete references: %s"), err.buf);
+
+ strbuf_release(&err);
+ return -1;
}
/*
@@ -2569,11 +2576,19 @@ static int files_transaction_finish(struct ref_store
*ref_store,
}
}
+ if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
+
if (repack_without_refs(refs->packed_ref_store, &refs_to_delete, err)) {
ret = TRANSACTION_GENERIC_ERROR;
+ packed_refs_unlock(refs->packed_ref_store);
goto cleanup;
}
+ packed_refs_unlock(refs->packed_ref_store);
+
/* Delete the reflogs of any references that were deleted: */
for_each_string_list_item(ref_to_delete, &refs_to_delete) {
strbuf_reset(&sb);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 5cf6b3d40e..377c775adb 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -669,25 +669,12 @@ int commit_packed_refs(struct ref_store *ref_store,
struct strbuf *err)
return -1;
}
-/*
- * Rollback the lockfile for the packed-refs file, and discard the
- * in-memory packed reference cache. (The packed-refs file will be
- * read anew if it is needed again after this function is called.)
- */
-static void rollback_packed_refs(struct packed_ref_store *refs)
-{
- packed_assert_main_repository(refs, "rollback_packed_refs");
-
- if (!is_lock_file_locked(&refs->lock))
- die("BUG: packed-refs not locked");
- packed_refs_unlock(&refs->base);
- clear_packed_ref_cache(refs);
-}
-
/*
* Rewrite the packed-refs file, omitting any refs listed in
* 'refnames'. On error, leave packed-refs unchanged, write an error
- * message to 'err', and return a nonzero value.
+ * message to 'err', and return a nonzero value. The packed refs lock
+ * must be held when calling this function; it will still be held when
+ * the function r
[PATCH v2 24/29] packed_refs_lock(): report errors via a `struct strbuf *err`
That way the callers don't have to come up with error messages
themselves.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 6 ++
refs/packed-backend.c | 17 +++--
refs/packed-backend.h | 6 +++---
3 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 88de907148..8ea4e9ab05 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1096,7 +1096,7 @@ static int files_pack_refs(struct ref_store *ref_store,
unsigned int flags)
struct ref_to_prune *refs_to_prune = NULL;
struct strbuf err = STRBUF_INIT;
- packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
+ packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);
while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
@@ -2679,9 +2679,7 @@ static int files_initial_transaction_commit(struct
ref_store *ref_store,
}
}
- if (packed_refs_lock(refs->packed_ref_store, 0)) {
- strbuf_addf(err, "unable to lock packed-refs file: %s",
- strerror(errno));
+ if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index cd214e07a1..78e877a9e3 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -515,7 +515,7 @@ static int write_packed_entry(FILE *fh, const char *refname,
return 0;
}
-int packed_refs_lock(struct ref_store *ref_store, int flags)
+int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf
*err)
{
struct packed_ref_store *refs =
packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
@@ -537,9 +537,15 @@ int packed_refs_lock(struct ref_store *ref_store, int
flags)
if (hold_lock_file_for_update_timeout(
&refs->lock,
refs->path,
- flags, timeout_value) < 0 ||
- close_lock_file(&refs->lock))
+ flags, timeout_value) < 0) {
+ unable_to_lock_message(refs->path, errno, err);
+ return -1;
+ }
+
+ if (close_lock_file(&refs->lock)) {
+ strbuf_addf(err, "unable to close %s: %s", refs->path,
strerror(errno));
return -1;
+ }
/*
* Now that we hold the `packed-refs` lock, make sure that our
@@ -698,10 +704,9 @@ int repack_without_refs(struct ref_store *ref_store,
if (!needs_repacking)
return 0; /* no refname exists in packed refs */
- if (packed_refs_lock(&refs->base, 0)) {
- unable_to_lock_message(refs->path, errno, err);
+ if (packed_refs_lock(&refs->base, 0, err))
return -1;
- }
+
packed = get_packed_refs(refs);
/* Remove refnames from the cache */
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index dbc00d3396..210e3f35ce 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -6,10 +6,10 @@ struct ref_store *packed_ref_store_create(const char *path,
/*
* Lock the packed-refs file for writing. Flags is passed to
- * hold_lock_file_for_update(). Return 0 on success. On errors, set
- * errno appropriately and return a nonzero value.
+ * hold_lock_file_for_update(). Return 0 on success. On errors, write
+ * an error message to `err` and return a nonzero value.
*/
-int packed_refs_lock(struct ref_store *ref_store, int flags);
+int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf
*err);
void add_packed_ref(struct ref_store *ref_store,
const char *refname, const struct object_id *oid);
--
2.11.0
