[RFC PATCH 5/5] ref-filter: add docs for new options
Add documentation for formatting options objectsize:disk and deltabase. Signed-off-by: Olga Telezhnaia --- Documentation/git-for-each-ref.txt | 21 - 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 901faef1bfdce..22d2ff88110cd 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -128,13 +128,18 @@ objecttype:: objectsize:: The size of the object (the same as 'git cat-file -s' reports). - + Append `:disk` to get the size, in bytes, that the object takes up on + disk. See the note about on-disk sizes in the `CAVEATS` section below. objectname:: The object name (aka SHA-1). For a non-ambiguous abbreviation of the object name append `:short`. For an abbreviation of the object name with desired length append `:short=`, where the minimum length is MINIMUM_ABBREV. The length may be exceeded to ensure unique object names. +deltabase:: + If the object is stored as a delta on-disk, this expands to the 40-hex + sha1 of the delta base object. Otherwise, expands to the null sha1 + (40 zeroes). See `CAVEATS` section below. upstream:: The name of a local ref which can be considered ``upstream'' @@ -361,6 +366,20 @@ This prints the authorname, if present. git for-each-ref --format="%(refname)%(if)%(authorname)%(then) Authored by: %(authorname)%(end)" +CAVEATS +--- + +Note that the sizes of objects on disk are reported accurately, but care +should be taken in drawing conclusions about which refs or objects are +responsible for disk usage. The size of a packed non-delta object may be +much larger than the size of objects which delta against it, but the +choice of which object is the base and which is the delta is arbitrary +and is subject to change during a repack. + +Note also that multiple copies of an object may be present in the object +database; in this case, it is undefined which copy's size or delta base +will be reported. + SEE ALSO linkgit:git-show-ref[1] -- https://github.com/git/git/pull/552
[RFC PATCH 1/5] ref-filter: add objectsize:disk option
Add new formatting option objectsize:disk to know
exact size that object takes up on disk.
Signed-off-by: Olga Telezhnaia
---
ref-filter.c | 26 +-
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 0c45ed9d94a4b..8ba1a4e72f2c3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -231,12 +231,18 @@ static int objecttype_atom_parser(const struct ref_format
*format, struct used_a
static int objectsize_atom_parser(const struct ref_format *format, struct
used_atom *atom,
const char *arg, struct strbuf *err)
{
- if (arg)
- return strbuf_addf_ret(err, -1, _("%%(objectsize) does not take
arguments"));
- if (*atom->name == '*')
- oi_deref.info.sizep = _deref.size;
- else
- oi.info.sizep =
+ if (!arg) {
+ if (*atom->name == '*')
+ oi_deref.info.sizep = _deref.size;
+ else
+ oi.info.sizep =
+ } else if (!strcmp(arg, "disk")) {
+ if (*atom->name == '*')
+ oi_deref.info.disk_sizep = _deref.disk_size;
+ else
+ oi.info.disk_sizep = _size;
+ } else
+ return strbuf_addf_ret(err, -1, _("unrecognized %%(objectsize)
argument: %s"), arg);
return 0;
}
@@ -876,11 +882,13 @@ static void grab_common_values(struct atom_value *val,
int deref, struct expand_
name++;
if (!strcmp(name, "objecttype"))
v->s = xstrdup(type_name(oi->type));
- else if (!strcmp(name, "objectsize")) {
+ else if (!strcmp(name, "objectsize:disk")) {
+ v->value = oi->disk_size;
+ v->s = xstrfmt("%lld", (long long)oi->disk_size);
+ } else if (!strcmp(name, "objectsize")) {
v->value = oi->size;
v->s = xstrfmt("%lu", oi->size);
- }
- else if (deref)
+ } else if (deref)
grab_objectname(name, >oid, v, _atom[i]);
}
}
--
https://github.com/git/git/pull/552
[RFC PATCH 3/5] ref-filter: add deltabase option
Add new formatting option: deltabase.
If the object is stored as a delta on-disk, this expands
to the 40-hex sha1 of the delta base object.
Otherwise, expands to the null sha1 (40 zeroes).
We have same option in cat-file command.
Hopefully, in the end I will remove formatting code from
cat-file and reuse formatting parts from ref-filter.
Signed-off-by: Olga Telezhnaia
---
ref-filter.c | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/ref-filter.c b/ref-filter.c
index 8ba1a4e72f2c3..3cfe01a039f8b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -246,6 +246,18 @@ static int objectsize_atom_parser(const struct ref_format
*format, struct used_a
return 0;
}
+static int deltabase_atom_parser(const struct ref_format *format, struct
used_atom *atom,
+const char *arg, struct strbuf *err)
+{
+ if (arg)
+ return strbuf_addf_ret(err, -1, _("%%(deltabase) does not take
arguments"));
+ if (*atom->name == '*')
+ oi_deref.info.delta_base_sha1 = oi_deref.delta_base_oid.hash;
+ else
+ oi.info.delta_base_sha1 = oi.delta_base_oid.hash;
+ return 0;
+}
+
static int body_atom_parser(const struct ref_format *format, struct used_atom
*atom,
const char *arg, struct strbuf *err)
{
@@ -437,6 +449,7 @@ static struct {
{ "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
{ "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
{ "objectname", SOURCE_OTHER, FIELD_STR, objectname_atom_parser },
+ { "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
{ "tree", SOURCE_OBJ },
{ "parent", SOURCE_OBJ },
{ "numparent", SOURCE_OBJ, FIELD_ULONG },
@@ -888,7 +901,9 @@ static void grab_common_values(struct atom_value *val, int
deref, struct expand_
} else if (!strcmp(name, "objectsize")) {
v->value = oi->size;
v->s = xstrfmt("%lu", oi->size);
- } else if (deref)
+ } else if (!strcmp(name, "deltabase"))
+ v->s = xstrdup(oid_to_hex(>delta_base_oid));
+ else if (deref)
grab_objectname(name, >oid, v, _atom[i]);
}
}
--
https://github.com/git/git/pull/552
[RFC PATCH 2/5] ref-filter: add tests for objectsize:disk
Test new formatting atom. Signed-off-by: Olga Telezhnaia --- t/t6300-for-each-ref.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 97bfbee6e8d69..097fdf21fe196 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -83,6 +83,7 @@ test_atom head push:strip=1 remotes/myfork/master test_atom head push:strip=-1 master test_atom head objecttype commit test_atom head objectsize 171 +test_atom head objectsize:disk 138 test_atom head objectname $(git rev-parse refs/heads/master) test_atom head objectname:short $(git rev-parse --short refs/heads/master) test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master) @@ -124,6 +125,8 @@ test_atom tag upstream '' test_atom tag push '' test_atom tag objecttype tag test_atom tag objectsize 154 +test_atom tag objectsize:disk 138 +test_atom tag '*objectsize:disk' 138 test_atom tag objectname $(git rev-parse refs/tags/testtag) test_atom tag objectname:short $(git rev-parse --short refs/tags/testtag) test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master) -- https://github.com/git/git/pull/552
[RFC PATCH 4/5] ref-filter: add tests for deltabase
Test new formatting option deltabase. Signed-off-by: Olga Telezhnaia --- t/t6300-for-each-ref.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 097fdf21fe196..0ffd63071392e 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -84,6 +84,7 @@ test_atom head push:strip=-1 master test_atom head objecttype commit test_atom head objectsize 171 test_atom head objectsize:disk 138 +test_atom head deltabase test_atom head objectname $(git rev-parse refs/heads/master) test_atom head objectname:short $(git rev-parse --short refs/heads/master) test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master) @@ -127,6 +128,8 @@ test_atom tag objecttype tag test_atom tag objectsize 154 test_atom tag objectsize:disk 138 test_atom tag '*objectsize:disk' 138 +test_atom tag deltabase +test_atom tag '*deltabase' test_atom tag objectname $(git rev-parse refs/tags/testtag) test_atom tag objectname:short $(git rev-parse --short refs/tags/testtag) test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master) -- https://github.com/git/git/pull/552
[RFC PATCH 0/5] ref-filter: add new formatting options
Add formatting options %(objectsize:disk) and %(deltabase), as in cat-file command. I can not test %(deltabase) properly (I mean, I want to have test with meaningful deltabase in the result - now we have only with zeros). I tested it manually on my git repo, and I have not-null deltabases there. We have "t/t1006-cat-file.sh" with similar case, but it is about blobs. ref-filter does not work with blobs, I need to write test about refs, and I feel that I can't catch the idea (and it is hard for me to write in Shell). Finally, I want to remove formatting logic in cat-file and use functions from ref-filter (we are almost there, so many work was done for this). I had an idea to make this migration in this patch (and stop worrying about bad tests about deltabase: we already have such test for cat-file and hopefully that could be enough). But I have another question there. cat-file has one more formatting option: "rest" [1]. Do we want such formatting option in ref-filter? It's easier for me to support that in ref-filter than to leave it only specifically for cat-file. Thank you! [1] https://git-scm.com/docs/git-cat-file#git-cat-file-coderestcode
Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options
On Fri, Nov 09 2018, Eric Sunshine wrote: > On Thu, Nov 8, 2018 at 5:34 PM Ævar Arnfjörð Bjarmason > wrote: >> On Thu, Nov 08 2018, Eric Sunshine wrote: >> > Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather >> > than introducing this new conditional, I'm thinking that a more >> > correct fix would be: >> > >> > opts.output_format |= DIFF_FORMAT_PATCH; >> > >> > (note the '|=' operator). This would result in 'opts.output_format' >> > containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did >> > prior to 73a834e9e2 when --no-patch was specified. >> >> Maybe I'm misunderstanding, but if you mean this on top: >> >> - if (!opts.output_format) >> - opts.output_format = DIFF_FORMAT_PATCH; >> + opts.output_format |= DIFF_FORMAT_PATCH; > > That is indeed what I mean. *Nod* >> Then the --stat test I've added here fails, because unlike "diff" the >> "--stat" (and others) will implicitly "--patch" and you need >> "--no-patch" as well (again, unlike with "diff"). > > This new --stat test also fails with Dscho's original git-range-diff > implementation, even before 73a834e9e2 regressed the --no-patch case. > The commit message seems to sell this patch as a pure regression-fix, > so it feels wrong for it to be conflating a regression fix > (--no-patch) with preparation for potential future improvements to > other options (--stat, etc.). > > I could see this as a two-patch series in which patch 1/2 fixes the > regression (with, say, "|="), and patch 2/2 generalizes setting > 'opts.output_format' for the future. Alternately, if left as a single > patch, perhaps the commit message could be fleshed out to better > explain that it is doing more than merely fixing a regression (since > it wasn't at all obvious to me, even after digging into the code > earlier to come up with "|=", or now when trying to understand your > response). Yeah that makes sense. I did not see (but see now) that the --stat behavior was different now v.s. before your 73a834e9e2. >> Right now --stat is pretty useless, but it could be made to make sense, >> and at that point (and earlier) I think it would be confusing if >> "range-diff" had different semantics with no options v.s. one option >> like "--stat" v.s. "--stat -p" compared to "diff". > > Perhaps this sort of rationale, along with some explanatory examples > could be added to the commit message to help the reader more fully > understand the situation. *Nod*
Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options
On Thu, Nov 8, 2018 at 5:34 PM Ævar Arnfjörð Bjarmason wrote: > On Thu, Nov 08 2018, Eric Sunshine wrote: > > Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather > > than introducing this new conditional, I'm thinking that a more > > correct fix would be: > > > > opts.output_format |= DIFF_FORMAT_PATCH; > > > > (note the '|=' operator). This would result in 'opts.output_format' > > containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did > > prior to 73a834e9e2 when --no-patch was specified. > > Maybe I'm misunderstanding, but if you mean this on top: > > - if (!opts.output_format) > - opts.output_format = DIFF_FORMAT_PATCH; > + opts.output_format |= DIFF_FORMAT_PATCH; That is indeed what I mean. > Then the --stat test I've added here fails, because unlike "diff" the > "--stat" (and others) will implicitly "--patch" and you need > "--no-patch" as well (again, unlike with "diff"). This new --stat test also fails with Dscho's original git-range-diff implementation, even before 73a834e9e2 regressed the --no-patch case. The commit message seems to sell this patch as a pure regression-fix, so it feels wrong for it to be conflating a regression fix (--no-patch) with preparation for potential future improvements to other options (--stat, etc.). I could see this as a two-patch series in which patch 1/2 fixes the regression (with, say, "|="), and patch 2/2 generalizes setting 'opts.output_format' for the future. Alternately, if left as a single patch, perhaps the commit message could be fleshed out to better explain that it is doing more than merely fixing a regression (since it wasn't at all obvious to me, even after digging into the code earlier to come up with "|=", or now when trying to understand your response). > Right now --stat is pretty useless, but it could be made to make sense, > and at that point (and earlier) I think it would be confusing if > "range-diff" had different semantics with no options v.s. one option > like "--stat" v.s. "--stat -p" compared to "diff". Perhaps this sort of rationale, along with some explanatory examples could be added to the commit message to help the reader more fully understand the situation. Thanks for working on this.
[PATCH v3] remote: add --save-to-push option to git remote set-url
This adds the --save-to-push option to `git remote set-url` such that
when executed, we move the remote.*.url to remote.*.pushurl and set
remote.*.url to the given url argument.
For example, if we have the following config:
[remote "origin"]
url = g...@github.com:git/git.git
`git remote set-url --save-to-push origin https://github.com/git/git.git`
would change the config to the following:
[remote "origin"]
url = https://github.com/git/git.git
pushurl = g...@github.com:git/git.git
Signed-off-by: Denton Liu
---
On Fri, Nov 09, 2018 at 12:15:22PM +0900, Junio C Hamano wrote:
> This sounds more like "saving to push" (i.e. what you are saving is
> the existing "url" and the "push" is a shorthand for "pushURL",
> which is the location the old value of "url" is aved to), not "save
> (the) push(URL)". So if adding this option makes sense, I would say
> "--save-to-push" (or even "--save-to-pushURL") may be a more
> appropriate name for it.
>
My original intention was for it to mean "save push behavior" but I
agree with you that it's unclear so I'm changing it to --save-to-push.
> Ambigous in what way? You asked the current URL to be saved as a
> pushURL, so existing pushURL destinations should not come into play,
> I would think. If there are more than one URL (not pushURL), on the
> other hand, I think you have a bigger problem (where would "git fetch"
> fetch from, and how would these multiple URLs are prevented from
> trashing refs/remotes/$remote/* with each other's refs?), so
> stopping the operation before "set-url" makes the problem worse is
> probably a good idea, but I think that is true with or without this
> new option.
>
> I _think_ in the future (if this option turns out to be widely used)
> people may ask for this condition to be loosened somewhat, but it is
> relatively easy to start restrictive and then to loosen later, so I
> think this is OK for now.
>
I agree, there's no reason why we shouldn't allow appending to the push
URLs if one already exists so I removed that removed that restriction.
---
Documentation/git-remote.txt | 5 +
builtin/remote.c | 26 +-
t/t5505-remote.sh| 11 +++
3 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 0cad37fb81..8f9d700252 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -19,6 +19,7 @@ SYNOPSIS
'git remote set-url' [--push] []
'git remote set-url --add' [--push]
'git remote set-url --delete' [--push]
+'git remote set-url --save-to-push'
'git remote' [-v | --verbose] 'show' [-n] ...
'git remote prune' [-n | --dry-run] ...
'git remote' [-v | --verbose] 'update' [-p | --prune] [( | )...]
@@ -155,6 +156,10 @@ With `--delete`, instead of changing existing URLs, all
URLs matching
regex are deleted for remote . Trying to delete all
non-push URLs is an error.
+
+With `--save-to-push`, the current URL is saved into the push URL before
+setting the URL to . Note that this command will not work if more than one
+URL is defined because the behavior would be ambiguous.
++
Note that the push URL and the fetch URL, even though they can
be set differently, must still refer to the same place. What you
pushed to the push URL should be what you would see if you
diff --git a/builtin/remote.c b/builtin/remote.c
index f7edf7f2cb..3249c3face 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -24,8 +24,9 @@ static const char * const builtin_remote_usage[] = {
N_("git remote set-branches [--add] ..."),
N_("git remote get-url [--push] [--all] "),
N_("git remote set-url [--push] []"),
- N_("git remote set-url --add "),
- N_("git remote set-url --delete "),
+ N_("git remote set-url --add [--push] "),
+ N_("git remote set-url --delete [--push] "),
+ N_("git remote set-url --save-to-push "),
NULL
};
@@ -77,8 +78,9 @@ static const char * const builtin_remote_geturl_usage[] = {
static const char * const builtin_remote_seturl_usage[] = {
N_("git remote set-url [--push] []"),
- N_("git remote set-url --add "),
- N_("git remote set-url --delete "),
+ N_("git remote set-url --add [--push] "),
+ N_("git remote set-url --delete [--push] "),
+ N_("git remote set-url --save-to-push "),
NULL
};
@@ -1519,7 +1521,7 @@ static int get_url(int argc, const char **argv)
static int set_url(int argc, const char **argv)
{
- int i, push_mode = 0, add_mode = 0, delete_mode = 0;
+ int i, push_mode = 0, save_to_push = 0, add_mode = 0, delete_mode = 0;
int matches = 0, negative_matches = 0;
const char *remotename = NULL;
const char *newurl = NULL;
@@ -1532,6 +1534,8 @@ static int set_url(int argc, const char **argv)
struct option options[] = {
Re: [PATCH] Makefile: add pending semantic patches
Stefan Beller writes:
> From: SZEDER Gábor
>
> Add a description and place on how to use coccinelle for large refactorings
> that happen only once.
>
> Based-on-work-by: SZEDER Gábor
> Signed-off-by: Stefan Beller
> ---
>
> I consider including this patch in a resend instead.
> It outlays the basics of such a new workflow, which we can refine later.
Thanks for tying loose ends.
> diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README
> index 9c2f8879c2..fa09d1abcc 100644
> --- a/contrib/coccinelle/README
> +++ b/contrib/coccinelle/README
> @@ -1,2 +1,62 @@
> This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/)
> semantic patches that might be useful to developers.
> +
> +There are two types of semantic patches:
> +
> + * Using the semantic transformation to check for bad patterns in the code;
> + This is what the original target 'make coccicheck' is designed to do and
> + it is expected that any resulting patch indicates a regression.
> + The patches resulting from 'make coccicheck' are small and infrequent,
> + so once they are found, they can be sent to the mailing list as per usual.
> +
> + Example for introducing new patterns:
> + 67947c34ae (convert "hashcmp() != 0" to "!hasheq()", 2018-08-28)
> + b84c783882 (fsck: s/++i > 1/i++/, 2018-10-24)
> +
> + Example of fixes using this approach:
> + 248f66ed8e (run-command: use strbuf_addstr() for adding a string to
> + a strbuf, 2018-03-25)
> + f919ffebed (Use MOVE_ARRAY, 2018-01-22)
> +
> + These types of semantic patches are usually part of testing, c.f.
> + 0860a7641b (travis-ci: fail if Coccinelle static analysis found something
> + to transform, 2018-07-23)
Yup, and I think what we have in 'pu' (including your the_repository
stuff) falls into this category.
I've been paying attention to "make coccicheck" produced patches for
the past few weeks, and so far, I found it _much_ _much_ more
pleasant, compared to having to worry about merge conflicts with the
topics in flight that changes day to day (not just because we add
new topics or update existing topics, but also the order of the
merge changes as topics mature at different rates and jumps over
each other in master..pu history), that "make coccicheck" after
topics are merged to integration branches fixes these issues up as
needed.
> + 3) Apply the semantic patch only partially, where needed for the patch
> series
> + that motivates the large scale refactoring and then build that series
> + on top of it.
It is not quite clear what "needed for the patch series" really
means in the context of this paragraph. What are the changes that
are not needed, that is still produced if we ran "make coccicheck"?
Are they wrong changes (e.g. a carelessly written read_cache() to
read_index(_index) conversion may munge the implementation of
read_cache(...) { return read_index(_index, ...); } and make
inifinite recursion)? Or are they "correct but not immediately
necessary" (e.g. because calling read_cache() does not hurt until
that function gets removed, so rewriting the callers to call
read_index() with _index may be correct but not immediately
necessary)?
> + By applying the semantic patch only partially where needed, the series
> + is less likely to conflict with other series in flight.
That is correct.
> + To make it possible to apply the semantic patch partially, there needs
> + to be mechanism for backwards compatibility to keep those places
> working
> + where the semantic patch is not applied. This can be done via a
> + wrapper function that has the exact name and signature as the function
> + to be changed.
OK, so this argues for leaving read_cache()-like things to help
other in-flight topics, while a change to encourage the use of
read_index() takes place. That also makes sense, and this directly
relates to "less likely to conflict" benefit you mentioned above.
But it is still unclear to me then what are "necessary".
... goes and thinks ...
OK, so a series that allows a codepath to work on an arbitrary
in-core istate, for example, may need to update a function to take
istate and use it to call read_index(istate...), and the old code in
such a call chain must have used read_cache(), always operating on
_index. For the purpose of that series, it does not matter if
other codepaths that are not involved in the callchain the series
wants to update are still only working on _index, so a change to
turn read_cache() into read_index(_index) is *not* necessary
(but still would be correct) and should be left out of the series.
But any change the series makes to the callchain in question that
turns read_cache() into read_index() with something call-specific
(not _index) is a necesary one. Is that a reasonable example
of what these paragraphs wanted to convey with the distinction
between "needed for the patch series" and other changes?
Re: [PATCH] Makefile: add pending semantic patches
On Thu, 8 Nov 2018 at 21:53, Stefan Beller wrote: > > From: SZEDER Gábor > I haven't followed the original discussion too carefully, so I'll read this like someone new to the topic probably would. A nit, perhaps, but I was genuinely confused at first. The subject is "Makefile: add pending semantic patches", but this patch doesn't add any. It adds Makefile-support for such patches though, and it defines the entire concept of pending semantic patches. How about "coccicheck: introduce 'pending' semantic patches"? > Add a description and place on how to use coccinelle for large refactorings > that happen only once. A bit confused about "and place". Based on my understanding from reading the remainder of this patch, maybe: Teach `make coccicheck` to avoid patches named "*.pending.cocci" and handle them separately in a new `make coccicheck-pending` instead. This means that we can separate "critical" patches from "FYI" patches. The former target can continue causing Travis to fail its static analysis job, while the latter can let us keep an eye on ongoing (pending) transitions without them causing too much fallout. Document the intended use-cases and processes around these two targets. > This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/) > semantic patches that might be useful to developers. > + > +There are two types of semantic patches: > + > + * Using the semantic transformation to check for bad patterns in the code; > + This is what the original target 'make coccicheck' is designed to do and Is it relevant that this was the "original" target? (Genuine question.) > + it is expected that any resulting patch indicates a regression. > + The patches resulting from 'make coccicheck' are small and infrequent, > + so once they are found, they can be sent to the mailing list as per usual. > + > + Example for introducing new patterns: > + 67947c34ae (convert "hashcmp() != 0" to "!hasheq()", 2018-08-28) > + b84c783882 (fsck: s/++i > 1/i++/, 2018-10-24) > + > + Example of fixes using this approach: > + 248f66ed8e (run-command: use strbuf_addstr() for adding a string to > + a strbuf, 2018-03-25) > + f919ffebed (Use MOVE_ARRAY, 2018-01-22) > + > + These types of semantic patches are usually part of testing, c.f. > + 0860a7641b (travis-ci: fail if Coccinelle static analysis found something > + to transform, 2018-07-23) Very nicely described, nice with the examples to quickly give a feeling about how/when to use this. > + * Using semantic transformations in large scale refactorings throughout > + the code base. > + > + When applying the semantic patch into a real patch, sending it to the > + mailing list in the usual way, such a patch would be expected to have a > + lot of textual and semantic conflicts as such large scale refactorings > + change function signatures that are used widely in the code base. > + A textual conflict would arise if surrounding code near any call of such > + function changes. A semantic conflict arises when other patch series in > + flight introduce calls to such functions. OK, I'm with you. > + So to aid these large scale refactorings, semantic patches can be used, > + using the process as follows: > + > + 1) Figure out what kind of large scale refactoring we need > + -> This is usually done by another unrelated series. "This"? The figuring out, or the refactoring? Also, "unrelated"? > + 2) Create the sematic patch needed for the large scale refactoring s/sematic/semantic/ > + and store it in contrib/coccinelle/*.pending.cocci > + -> The suffix containing 'pending' is important to differentiate > + this case from the other use case of checking for bad patterns. Good. > + 3) Apply the semantic patch only partially, where needed for the patch > series > + that motivates the large scale refactoring and then build that series > + on top of it. > + By applying the semantic patch only partially where needed, the series > + is less likely to conflict with other series in flight. > + To make it possible to apply the semantic patch partially, there needs > + to be mechanism for backwards compatibility to keep those places > working > + where the semantic patch is not applied. This can be done via a > + wrapper function that has the exact name and signature as the function > + to be changed. > + > + 4) Send the series as usual, including only the needed parts of the > + large scale refactoring Trailing period. OK, I think I get it, but I wonder if this might not work equally well or better under certain circumstances: - introduce new API - add pending semantic patch - convert quiet areas to use the new API On the other hand, listing all possible flows might be needlessly limiting. I guess it boils down to this: "Create a pending semantic patch. Make sure the old way of doing things
Re: [PATCH v7 1/1] http: add support selecting http version
"Force Charlie via GitGitGadget" writes:
> +http.version::
> + Use the specified HTTP protocol version when communicating with a
> server.
> + If you want to force the default. The available and default version
> depend
> + on libcurl. Actually the possible values of
> + this option are:
> +
> + - HTTP/2
> + - HTTP/1.1
> +
I just wanted to make sure this formats well; it uses the same
construct as used to make the list of allowed values for the next
entry (sslVersion), so this should be fine.
Thanks.
> http.sslVersion::
> The SSL version to use when negotiating an SSL connection, if you
> want to force the default. The available and default version
> diff --git a/http.c b/http.c
> index 3dc8c560d6..c22275bdee 100644
> --- a/http.c
> +++ b/http.c
> @@ -48,6 +48,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
>
> static int curl_ssl_verify = -1;
> static int curl_ssl_try;
> +static const char *curl_http_version = NULL;
> static const char *ssl_cert;
> static const char *ssl_cipherlist;
> static const char *ssl_version;
> @@ -284,6 +285,9 @@ static void process_curl_messages(void)
>
> static int http_options(const char *var, const char *value, void *cb)
> {
> + if (!strcmp("http.version", var)) {
> + return git_config_string(_http_version, var, value);
> + }
> if (!strcmp("http.sslverify", var)) {
> curl_ssl_verify = git_config_bool(var, value);
> return 0;
> @@ -789,6 +793,30 @@ static long get_curl_allowed_protocols(int from_user)
> }
> #endif
>
> +#if LIBCURL_VERSION_NUM >=0x072f00
> +static int get_curl_http_version_opt(const char *version_string, long *opt)
> +{
> + int i;
> + static struct {
> + const char *name;
> + long opt_token;
> + } choice[] = {
> + { "HTTP/1.1", CURL_HTTP_VERSION_1_1 },
> + { "HTTP/2", CURL_HTTP_VERSION_2 }
> + };
> +
> + for (i = 0; i < ARRAY_SIZE(choice); i++) {
> + if (!strcmp(version_string, choice[i].name)) {
> + *opt = choice[i].opt_token;
> + return 0;
> + }
> + }
> +
> + return -1; /* not found */
> +}
> +
> +#endif
> +
> static CURL *get_curl_handle(void)
> {
> CURL *result = curl_easy_init();
> @@ -806,6 +834,16 @@ static CURL *get_curl_handle(void)
> curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 2);
> }
>
> +#if LIBCURL_VERSION_NUM >= 0x072f00 // 7.47.0
> +if (curl_http_version) {
> + long opt;
> + if (!get_curl_http_version_opt(curl_http_version, )) {
> + /* Set request use http version */
> + curl_easy_setopt(result, CURLOPT_HTTP_VERSION, opt);
> + }
> +}
> +#endif
> +
> #if LIBCURL_VERSION_NUM >= 0x070907
> curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
> #endif
[PATCH v8 0/1] http: add support selecting http version
Usually we don't need to set libcurl to choose which version of the HTTP
protocol to use to communicate with a server. But different versions of
libcurl, the default value is not the same.
CURL >= 7.62.0: CURL_HTTP_VERSION_2TLS CURL < 7.62: CURL_HTTP_VERSION_1_1
In order to give users the freedom to control the HTTP version, we need to
add a setting to choose which HTTP version to use.
This patch support force enable HTTP/2 or HTTP/1.1.
example:
GIT_CURL_VERBOSE=1 git2 -c http.version=HTTP/2 ls-remote
https://bitbucket.org/aquariusjay/deeplab-public-ver2.git
Force Charlie (1):
http: add support selecting http version
Documentation/config.txt | 9 +
http.c | 39 +++
2 files changed, 48 insertions(+)
base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-69%2Ffcharlie%2Fmaster-v8
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-69/fcharlie/master-v8
Pull-Request: https://github.com/gitgitgadget/git/pull/69
Range-diff vs v7:
1: e26fc0d8c7 ! 1: 71f8b71b34 http: add support selecting http version
@@ -78,6 +78,7 @@
+ }
+ }
+
++ warning("unknown value given to http.version: '%s'", version_string);
+ return -1; /* not found */
+}
+
--
gitgitgadget
[PATCH v8 1/1] http: add support selecting http version
From: Force Charlie
Usually we don't need to set libcurl to choose which version of the
HTTP protocol to use to communicate with a server.
But different versions of libcurl, the default value is not the same.
CURL >= 7.62.0: CURL_HTTP_VERSION_2TLS
CURL < 7.62: CURL_HTTP_VERSION_1_1
In order to give users the freedom to control the HTTP version,
we need to add a setting to choose which HTTP version to use.
Signed-off-by: Force Charlie
---
Documentation/config.txt | 9 +
http.c | 39 +++
2 files changed, 48 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 41a9ff2b6a..f397942128 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1935,6 +1935,15 @@ http.saveCookies::
If set, store cookies received during requests to the file specified by
http.cookieFile. Has no effect if http.cookieFile is unset.
+http.version::
+ Use the specified HTTP protocol version when communicating with a
server.
+ If you want to force the default. The available and default version
depend
+ on libcurl. Actually the possible values of
+ this option are:
+
+ - HTTP/2
+ - HTTP/1.1
+
http.sslVersion::
The SSL version to use when negotiating an SSL connection, if you
want to force the default. The available and default version
diff --git a/http.c b/http.c
index 3dc8c560d6..bc3274804e 100644
--- a/http.c
+++ b/http.c
@@ -48,6 +48,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
static int curl_ssl_verify = -1;
static int curl_ssl_try;
+static const char *curl_http_version = NULL;
static const char *ssl_cert;
static const char *ssl_cipherlist;
static const char *ssl_version;
@@ -284,6 +285,9 @@ static void process_curl_messages(void)
static int http_options(const char *var, const char *value, void *cb)
{
+ if (!strcmp("http.version", var)) {
+ return git_config_string(_http_version, var, value);
+ }
if (!strcmp("http.sslverify", var)) {
curl_ssl_verify = git_config_bool(var, value);
return 0;
@@ -789,6 +793,31 @@ static long get_curl_allowed_protocols(int from_user)
}
#endif
+#if LIBCURL_VERSION_NUM >=0x072f00
+static int get_curl_http_version_opt(const char *version_string, long *opt)
+{
+ int i;
+ static struct {
+ const char *name;
+ long opt_token;
+ } choice[] = {
+ { "HTTP/1.1", CURL_HTTP_VERSION_1_1 },
+ { "HTTP/2", CURL_HTTP_VERSION_2 }
+ };
+
+ for (i = 0; i < ARRAY_SIZE(choice); i++) {
+ if (!strcmp(version_string, choice[i].name)) {
+ *opt = choice[i].opt_token;
+ return 0;
+ }
+ }
+
+ warning("unknown value given to http.version: '%s'", version_string);
+ return -1; /* not found */
+}
+
+#endif
+
static CURL *get_curl_handle(void)
{
CURL *result = curl_easy_init();
@@ -806,6 +835,16 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 2);
}
+#if LIBCURL_VERSION_NUM >= 0x072f00 // 7.47.0
+if (curl_http_version) {
+ long opt;
+ if (!get_curl_http_version_opt(curl_http_version, )) {
+ /* Set request use http version */
+ curl_easy_setopt(result, CURLOPT_HTTP_VERSION, opt);
+ }
+}
+#endif
+
#if LIBCURL_VERSION_NUM >= 0x070907
curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
#endif
--
gitgitgadget
Re: [RFC PATCH v2] remote: add --save-push option to git remote set-url
Denton Liu writes:
> This adds the --save-push option to `git remote set-url` such that when
> executed, we move the remote.*.url to remote.*.pushurl and set
> remote.*.url to the given url argument.
>
> For example, if we have the following config:
>
> [remote "origin"]
> url = g...@github.com:git/git.git
>
> `git remote set-url --save-push origin https://github.com/git/git.git`
> would change the config to the following:
>
> [remote "origin"]
> url = https://github.com/git/git.git
> pushurl = g...@github.com:git/git.git
This sounds more like "saving to push" (i.e. what you are saving is
the existing "url" and the "push" is a shorthand for "pushURL",
which is the location the old value of "url" is aved to), not "save
(the) push(URL)". So if adding this option makes sense, I would say
"--save-to-push" (or even "--save-to-pushURL") may be a more
appropriate name for it.
> +With `--save-push`, the current URL is saved into the push URL before setting
> +the URL to . Note that this command will not work if more than one URL
> is
> +defined or if any push URLs are defined because behavior would be ambiguous.
Ambigous in what way? You asked the current URL to be saved as a
pushURL, so existing pushURL destinations should not come into play,
I would think. If there are more than one URL (not pushURL), on the
other hand, I think you have a bigger problem (where would "git fetch"
fetch from, and how would these multiple URLs are prevented from
trashing refs/remotes/$remote/* with each other's refs?), so
stopping the operation before "set-url" makes the problem worse is
probably a good idea, but I think that is true with or without this
new option.
> diff --git a/builtin/remote.c b/builtin/remote.c
> index f7edf7f2cb..0eaec7ef38 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -24,8 +24,9 @@ static const char * const builtin_remote_usage[] = {
> N_("git remote set-branches [--add] ..."),
> N_("git remote get-url [--push] [--all] "),
> N_("git remote set-url [--push] []"),
> - N_("git remote set-url --add "),
> - N_("git remote set-url --delete "),
> + N_("git remote set-url --add [--push] "),
> + N_("git remote set-url --delete [--push] "),
> + N_("git remote set-url --save-push "),
> NULL
> };
Needs update?
> @@ -77,8 +78,9 @@ static const char * const builtin_remote_geturl_usage[] = {
>
> static const char * const builtin_remote_seturl_usage[] = {
> N_("git remote set-url [--push] []"),
> - N_("git remote set-url --add "),
> - N_("git remote set-url --delete "),
> + N_("git remote set-url --add [--push] "),
> + N_("git remote set-url --delete [--push] "),
> + N_("git remote set-url --save-push "),
> NULL
> };
Needs update?
> + if (save_push) {
> + if (remote->url_nr != 1 || remote->pushurl_nr != 0)
> + die(_("--save-push can only be used when one
> url and no pushurl is defined"), remotename);
I _think_ in the future (if this option turns out to be widely used)
people may ask for this condition to be loosened somewhat, but it is
relatively easy to start restrictive and then to loosen later, so I
think this is OK for now.
Re: [PATCH] travis-ci: install packages in 'ci/install-dependencies.sh'
SZEDER Gábor writes: >> > I'm not sure about the last paragraph, because: >> > >> > - It talks about presumed benefits for a currently still >> > work-in-progress patch series of an other contributor, and I'm not >> > really sure that that's a good thing. Perhaps I should have >> > rather put it below the '---'. >> > >> > - I'm confused about the name of this Azure thing. The cover letter >> > mentions "Azure Pipelines", the file is called >> > 'azure-pipelines.yml', but the relevant patch I link to talks >> > about "Azure DevOps" in the commit message. >> > >> > Anyway, keep that last paragraph or drop it as you see fit. >> >> I hope we'll hear from Dscho in one or two revolutions of the Earth >> ;-) > > ... revolutions around what? :) Originally I meant its own axis, but perhaps the moon.
Re: [PATCH v6 1/1] http: add support selecting http version
Eric Sunshine writes:
>> @@ -284,6 +285,9 @@ static void process_curl_messages(void)
>> static int http_options(const char *var, const char *value, void *cb)
>> {
>> + if (!strcmp("http.version",var)) {
>
> Style: space after comma
>
>> + return git_config_string(_http_version, var, value);
>> + }
>> @@ -806,6 +834,16 @@ static CURL *get_curl_handle(void)
>> +if (curl_http_version) {
>> + long opt;
>> + if (!get_curl_http_version_opt(curl_http_version, )) {
>> + /* Set request use http version */
>> + curl_easy_setopt(result, CURLOPT_HTTP_VERSION,opt);
>
> Style: space after comma
>
>> + }
>> +}
Thanks, both. This is almost done, I think.
Re: [PATCH v6 1/1] http: add support selecting http version
"Force Charlie via GitGitGadget" writes:
> +#if LIBCURL_VERSION_NUM >=0x072f00
> +static int get_curl_http_version_opt(const char *version_string, long *opt)
> +{
> + int i;
> + static struct {
> + const char *name;
> + long opt_token;
> + } choice[] = {
> + { "HTTP/1.1", CURL_HTTP_VERSION_1_1 },
> + { "HTTP/2", CURL_HTTP_VERSION_2 }
> + };
> +
> + for (i = 0; i < ARRAY_SIZE(choice); i++) {
> + if (!strcmp(version_string, choice[i].name)) {
> + *opt = choice[i].opt_token;
> + return 0;
> + }
> + }
> +
I wonder if we need to give a warning here about an unknown and
ignored value, by calling something like
warning("unknown value given to http.version: '%s'", version_string);
here. We should not trigger noisy warning while reading the
configuration file looking for other variables unrelated to
http.version but this codepath is followed only when we know
we need to find out what value the variable is set to, so it
probably is a good thing to do.
Thoughts?
Re: [PATCH 1/1] Update .mailmap
"Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > This patch makes the output of `git shortlog -nse v2.10.0` > duplicate-free. Did you mean "v2.10.0..master" or did you really mean this covers authors recorded up to v2.10.0? Judging from the cover letter, I think you meant the former. Can you say a bit more about how one among multiple addresses for each person was chosen in the log message? E.g. "After asking each author which one is the preferred one", "Picked the one with the most recent committer timestamps", "There were two for each but one of them were bouncing", etc. > @@ -150,6 +155,7 @@ Mark Levedahl > Mark Rada > Martin Langhoff > Martin von Zweigbergk > > +Masaya Suzuki It is a bit surprising that we can take an entry without SP in between two addresses and still behave sensibley, but it probably makes more sense to add one just to look similar to other entries. Thanks for working on this.
[RFC PATCH v2] remote: add --save-push option to git remote set-url
This adds the --save-push option to `git remote set-url` such that when
executed, we move the remote.*.url to remote.*.pushurl and set
remote.*.url to the given url argument.
For example, if we have the following config:
[remote "origin"]
url = g...@github.com:git/git.git
`git remote set-url --save-push origin https://github.com/git/git.git`
would change the config to the following:
[remote "origin"]
url = https://github.com/git/git.git
pushurl = g...@github.com:git/git.git
Signed-off-by: Denton Liu
---
I decided to address your comments and reroll the patch one more time.
Even though the option isn't _that_ commonly used, I've managed to use
it a couple of times since I've implemented so I believe that this
option should be included.
---
Documentation/git-remote.txt | 5 +
builtin/remote.c | 25 -
2 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 0cad37fb81..8ce85fe2f2 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -19,6 +19,7 @@ SYNOPSIS
'git remote set-url' [--push] []
'git remote set-url --add' [--push]
'git remote set-url --delete' [--push]
+'git remote set-url --save-push'
'git remote' [-v | --verbose] 'show' [-n] ...
'git remote prune' [-n | --dry-run] ...
'git remote' [-v | --verbose] 'update' [-p | --prune] [( | )...]
@@ -155,6 +156,10 @@ With `--delete`, instead of changing existing URLs, all
URLs matching
regex are deleted for remote . Trying to delete all
non-push URLs is an error.
+
+With `--save-push`, the current URL is saved into the push URL before setting
+the URL to . Note that this command will not work if more than one URL is
+defined or if any push URLs are defined because behavior would be ambiguous.
++
Note that the push URL and the fetch URL, even though they can
be set differently, must still refer to the same place. What you
pushed to the push URL should be what you would see if you
diff --git a/builtin/remote.c b/builtin/remote.c
index f7edf7f2cb..0eaec7ef38 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -24,8 +24,9 @@ static const char * const builtin_remote_usage[] = {
N_("git remote set-branches [--add] ..."),
N_("git remote get-url [--push] [--all] "),
N_("git remote set-url [--push] []"),
- N_("git remote set-url --add "),
- N_("git remote set-url --delete "),
+ N_("git remote set-url --add [--push] "),
+ N_("git remote set-url --delete [--push] "),
+ N_("git remote set-url --save-push "),
NULL
};
@@ -77,8 +78,9 @@ static const char * const builtin_remote_geturl_usage[] = {
static const char * const builtin_remote_seturl_usage[] = {
N_("git remote set-url [--push] []"),
- N_("git remote set-url --add "),
- N_("git remote set-url --delete "),
+ N_("git remote set-url --add [--push] "),
+ N_("git remote set-url --delete [--push] "),
+ N_("git remote set-url --save-push "),
NULL
};
@@ -1519,7 +1521,7 @@ static int get_url(int argc, const char **argv)
static int set_url(int argc, const char **argv)
{
- int i, push_mode = 0, add_mode = 0, delete_mode = 0;
+ int i, push_mode = 0, save_push = 0, add_mode = 0, delete_mode = 0;
int matches = 0, negative_matches = 0;
const char *remotename = NULL;
const char *newurl = NULL;
@@ -1532,6 +1534,8 @@ static int set_url(int argc, const char **argv)
struct option options[] = {
OPT_BOOL('\0', "push", _mode,
N_("manipulate push URLs")),
+ OPT_BOOL('\0', "save-push", _push,
+N_("change fetching URL behavior")),
OPT_BOOL('\0', "add", _mode,
N_("add URL")),
OPT_BOOL('\0', "delete", _mode,
@@ -1543,6 +1547,8 @@ static int set_url(int argc, const char **argv)
if (add_mode && delete_mode)
die(_("--add --delete doesn't make sense"));
+ if (save_push && (push_mode || add_mode || delete_mode))
+ die(_("--save-push cannot be used with other options"));
if (argc < 3 || argc > 4 || ((add_mode || delete_mode) && argc != 3))
usage_with_options(builtin_remote_seturl_usage, options);
@@ -1564,6 +1570,15 @@ static int set_url(int argc, const char **argv)
urlset = remote->pushurl;
urlset_nr = remote->pushurl_nr;
} else {
+ if (save_push) {
+ if (remote->url_nr != 1 || remote->pushurl_nr != 0)
+ die(_("--save-push can only be used when one
url and no pushurl is defined"), remotename);
+
+ strbuf_addf(_buf, "remote.%s.pushurl", remotename);
+ git_config_set(name_buf.buf,
RE: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Can someone please tell me how to unsubscribe from this email. I am no longer interested in receiving these emails, and cannot find how to unsubscribe. Thank you in advance. Joseph Moisan | Software Engineer Building Technologies and Solutions Tel: +1-978-731-8950 joseph.moi...@jci.com -Original Message- From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf Of Junio C Hamano Sent: Wednesday, November 7, 2018 7:17 PM To: Ramsay Jones Cc: Johannes Schindelin ; Johannes Schindelin via GitGitGadget ; git@vger.kernel.org Subject: Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() Ramsay Jones writes: >> The cute thing is: your absolute paths would not be moved because we >> are talking about Windows. Therefore your absolute paths would not >> start with a forward slash. > > Ah, sorry, I must have misunderstood a comment in your cover letter: > > The reason is this: something like this (make paths specified e.g. via > http.sslCAInfo relative to the runtime prefix) is potentially useful > also in the non-Windows context, as long as Git was built with the > runtime prefix feature. > > ... so I thought you meant to add this code for POSIX systems as well. > > My mistake. :( Well, I do not think you should feel so bad about it, as you are not alone. It wasn't clear to me either that it was to introduce a new syntax that users would not have otherwise used before (i.e. avoids negatively affecting existing settings---because the users would have used a path that begins with a backslash if they meant "full path down from the top of the current drive") to mean a new thing (i.e. "relative to the runtime prefix") that the patch wanted to do. If that is the motivation, then it does make sense to extend this function, and possibly rename it, like this patch does, as we would want this new syntax available in anywhere we take a pathname to an untracked thing. And for such a pathname, we should be consistently using expand_user_path() *anyway* even without this new "now we know how to spell 'relative to the runtime prefix'" feature. So I agree with the patch that the issue it wants to address is worth addressing, and the function to enhance is this one. I am still unsure about the solution, though. I suspect that any build that uses runtime prefix would want access to this feature, regardless of the platform. The new syntax that is "a pathname that begins with a slash is not an absolute path but is relative to the runtime prefix" cannot avoid ambiguity with users' existing settings on platforms other than Windows, which means this feature cannot be made platform neutral in its posted form. The documentation must say "On windows, the way to spell runtime prefix relative paths is this; on macs, you must spell it differently like this." etc. Which is rather unfortunate. Perhaps we need to make an effort to find a syntax that is universally unambiguous and use it consistently across platforms? I am tempted to say "///" might also be such a way, even in the POSIX world, but am not brave enough to do so, as I suspect that may have a fallout in the Windows world X-<. An earlier suggestion by Duy in [*1*] to pretend as if we take $VARIABLE and define special variables might be a better direction. Are there security implications if we started allowing references to environment varibables in strings we pass expand_user_path()? If we cannot convince ourselves that it is safe to allow access to any and all environment variables, then we probably can start by specific "pseudo variables" under our control, like GIT_EXEC_PATH and GIT_INSTALL_ROOT, that do not even have to be tied to environment variables, perhaps with further restriction to allow it only at the beginning of the string, or something like that, if necessary. [References] *1*
[PATCH v7 1/1] http: add support selecting http version
From: Force Charlie
Usually we don't need to set libcurl to choose which version of the
HTTP protocol to use to communicate with a server.
But different versions of libcurl, the default value is not the same.
CURL >= 7.62.0: CURL_HTTP_VERSION_2TLS
CURL < 7.62: CURL_HTTP_VERSION_1_1
In order to give users the freedom to control the HTTP version,
we need to add a setting to choose which HTTP version to use.
Signed-off-by: Force Charlie
---
Documentation/config.txt | 9 +
http.c | 38 ++
2 files changed, 47 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 41a9ff2b6a..f397942128 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1935,6 +1935,15 @@ http.saveCookies::
If set, store cookies received during requests to the file specified by
http.cookieFile. Has no effect if http.cookieFile is unset.
+http.version::
+ Use the specified HTTP protocol version when communicating with a
server.
+ If you want to force the default. The available and default version
depend
+ on libcurl. Actually the possible values of
+ this option are:
+
+ - HTTP/2
+ - HTTP/1.1
+
http.sslVersion::
The SSL version to use when negotiating an SSL connection, if you
want to force the default. The available and default version
diff --git a/http.c b/http.c
index 3dc8c560d6..c22275bdee 100644
--- a/http.c
+++ b/http.c
@@ -48,6 +48,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
static int curl_ssl_verify = -1;
static int curl_ssl_try;
+static const char *curl_http_version = NULL;
static const char *ssl_cert;
static const char *ssl_cipherlist;
static const char *ssl_version;
@@ -284,6 +285,9 @@ static void process_curl_messages(void)
static int http_options(const char *var, const char *value, void *cb)
{
+ if (!strcmp("http.version", var)) {
+ return git_config_string(_http_version, var, value);
+ }
if (!strcmp("http.sslverify", var)) {
curl_ssl_verify = git_config_bool(var, value);
return 0;
@@ -789,6 +793,30 @@ static long get_curl_allowed_protocols(int from_user)
}
#endif
+#if LIBCURL_VERSION_NUM >=0x072f00
+static int get_curl_http_version_opt(const char *version_string, long *opt)
+{
+ int i;
+ static struct {
+ const char *name;
+ long opt_token;
+ } choice[] = {
+ { "HTTP/1.1", CURL_HTTP_VERSION_1_1 },
+ { "HTTP/2", CURL_HTTP_VERSION_2 }
+ };
+
+ for (i = 0; i < ARRAY_SIZE(choice); i++) {
+ if (!strcmp(version_string, choice[i].name)) {
+ *opt = choice[i].opt_token;
+ return 0;
+ }
+ }
+
+ return -1; /* not found */
+}
+
+#endif
+
static CURL *get_curl_handle(void)
{
CURL *result = curl_easy_init();
@@ -806,6 +834,16 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 2);
}
+#if LIBCURL_VERSION_NUM >= 0x072f00 // 7.47.0
+if (curl_http_version) {
+ long opt;
+ if (!get_curl_http_version_opt(curl_http_version, )) {
+ /* Set request use http version */
+ curl_easy_setopt(result, CURLOPT_HTTP_VERSION, opt);
+ }
+}
+#endif
+
#if LIBCURL_VERSION_NUM >= 0x070907
curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
#endif
--
gitgitgadget
[PATCH v7 0/1] http: add support selecting http version
Usually we don't need to set libcurl to choose which version of the HTTP
protocol to use to communicate with a server. But different versions of
libcurl, the default value is not the same.
CURL >= 7.62.0: CURL_HTTP_VERSION_2TLS CURL < 7.62: CURL_HTTP_VERSION_1_1
In order to give users the freedom to control the HTTP version, we need to
add a setting to choose which HTTP version to use.
This patch support force enable HTTP/2 or HTTP/1.1.
example:
GIT_CURL_VERBOSE=1 git2 -c http.version=HTTP/2 ls-remote
https://bitbucket.org/aquariusjay/deeplab-public-ver2.git
Force Charlie (1):
http: add support selecting http version
Documentation/config.txt | 9 +
http.c | 38 ++
2 files changed, 47 insertions(+)
base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-69%2Ffcharlie%2Fmaster-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-69/fcharlie/master-v7
Pull-Request: https://github.com/gitgitgadget/git/pull/69
Range-diff vs v6:
1: 93fda67198 ! 1: e26fc0d8c7 http: add support selecting http version
@@ -49,7 +49,7 @@
static int http_options(const char *var, const char *value, void *cb)
{
-+ if (!strcmp("http.version",var)) {
++ if (!strcmp("http.version", var)) {
+ return git_config_string(_http_version, var, value);
+ }
if (!strcmp("http.sslverify", var)) {
@@ -95,7 +95,7 @@
+ long opt;
+ if (!get_curl_http_version_opt(curl_http_version, )) {
+ /* Set request use http version */
-+ curl_easy_setopt(result, CURLOPT_HTTP_VERSION,opt);
++ curl_easy_setopt(result, CURLOPT_HTTP_VERSION, opt);
+ }
+}
+#endif
--
gitgitgadget
Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options
On Thu, Nov 08 2018, Eric Sunshine wrote:
> On Wed, Nov 7, 2018 at 7:22 AM Ævar Arnfjörð Bjarmason
> wrote:
>> In 73a834e9e2 ("range-diff: relieve callers of low-level configuration
>> burden", 2018-07-22) we broke passing down options like --no-patch,
>> --stat etc. Fix that regression, and add a test for some of these
>> options being passed down.
>
> Thanks both (Ævar and Dscho) for cleaning up my mess, and sorry for
> not responding sooner; I only just found time to read the discussion
> thread. One comment below...
>
>> Signed-off-by: Ævar Arnfjörð Bjarmason
>> ---
>> diff --git a/range-diff.c b/range-diff.c
>> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char
>> *range2,
>> memcpy(, diffopt, sizeof(opts));
>> - opts.output_format = DIFF_FORMAT_PATCH;
>> + if (!opts.output_format)
>> + opts.output_format = DIFF_FORMAT_PATCH;
>
> Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather
> than introducing this new conditional, I'm thinking that a more
> correct fix would be:
>
> opts.output_format |= DIFF_FORMAT_PATCH;
>
> (note the '|=' operator). This would result in 'opts.output_format'
> containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did
> prior to 73a834e9e2 when --no-patch was specified.
Maybe I'm misunderstanding, but if you mean this on top:
diff --git a/range-diff.c b/range-diff.c
index 488844c0af..ea317f92f9 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -453,8 +453,7 @@ int show_range_diff(const char *range1, const char
*range2,
struct strbuf indent = STRBUF_INIT;
memcpy(, diffopt, sizeof(opts));
- if (!opts.output_format)
- opts.output_format = DIFF_FORMAT_PATCH;
+ opts.output_format |= DIFF_FORMAT_PATCH;
opts.flags.suppress_diff_headers = 1;
opts.flags.dual_color_diffed_diffs = dual_color;
opts.output_prefix = output_prefix_cb;
Then the --stat test I've added here fails, because unlike "diff" the
"--stat" (and others) will implicitly "--patch" and you need
"--no-patch" as well (again, unlike with "diff").
Right now --stat is pretty useless, but it could be made to make sense,
and at that point (and earlier) I think it would be confusing if
"range-diff" had different semantics with no options v.s. one option
like "--stat" v.s. "--stat -p" compared to "diff".
Re: [RFC PATCH] index-pack: improve performance on NFS
On Thu, Nov 08 2018, Jeff King wrote: > On Wed, Nov 07, 2018 at 10:55:24PM +, Geert Jansen wrote: > >> On Mon, Oct 29, 2018 at 07:27:39PM -0400, Jeff King wrote: >> >> > On Mon, Oct 29, 2018 at 08:36:07PM +0100, Ævar Arnfjörð Bjarmason wrote: >> > > * Re-roll my 4 patch series to include the patch you have in >> > ><20181027093300.ga23...@sigill.intra.peff.net> >> > >> > I don't think it's quite ready for inclusion as-is. I hope to brush it >> > up a bit, but I have quite a backlog of stuff to review, as well. >> >> We're still quite keen to get this patch included. Is there anything I can do >> to help? > > Yes, testing and review. :) > > I won't send the series out just yet, as I suspect it could use another > read-through on my part. But if you want to peek at it or try some > timings, it's available at: > > https://github.com/peff/git jk/loose-cache Just a comment on this from the series: Note that it is possible for this to actually be _slower_. We'll do a full readdir() to fill the cache, so if you have a very large number of loose objects and a very small number of lookups, that readdir() may end up more expensive. In practice, though, having a large number of loose objects is already a performance problem, which should be fixed by repacking or pruning via git-gc. So on balance, this should be a good tradeoff. Our biggest repo has a very large number of loose objects at any given time, but the vast majority of these are because gc *is* happening very frequently and the default expiry policy of 2wks is in effect. Having a large number of loose objects is not per-se a performance problem. It's a problem if you end up "faulting" to from packs to the loose object directory a lot because those objects are still reachable, but if they're not reachable that number can grow very large if your ref churn is large (so lots of expired loose object production). Anyway, the series per-se looks good to me. It's particularly nice to have some of the ODB cleanup + cleanup in fetch-pack.c Just wanted to note that in our default (reasonable) config we do produce scenarios where this change can still be somewhat pathological, so I'm still interested in disabling it entirely given the implausibility of what it's guarding against.
Re: [RFC PATCH] index-pack: improve performance on NFS
On Thu, Nov 08, 2018 at 04:18:24PM -0500, Jeff King wrote: > Heh, indeed. Try this on top: > > diff --git a/sha1-file.c b/sha1-file.c > index bc35b28e17..9ff27f92ed 100644 > --- a/sha1-file.c > +++ b/sha1-file.c > @@ -692,6 +692,7 @@ void prepare_alt_odb(struct repository *r) > link_alt_odb_entries(r, r->objects->alternate_db, PATH_SEP, NULL, 0); > > read_info_alternates(r, r->objects->odb->path, 0); > + r->objects->loaded_alternates = 1; > } > > /* Returns 1 if we have successfully freshened the file, 0 otherwise. */ Thanks, this did it. Performance is now back at the level of the previous patch.
Re: [PATCH] travis-ci: install packages in 'ci/install-dependencies.sh'
On Fri, Nov 02, 2018 at 11:25:17AM +0900, Junio C Hamano wrote: > SZEDER Gábor writes: > > > Ever since we started using Travis CI, we specified the list of > > packages to install in '.travis.yml' via the APT addon. While running > > our builds on Travis CI's container-based infrastructure we didn't > > have another choice, because that environment didn't support 'sudo', > > and thus we didn't have permission to install packages ourselves. With > > the switch to the VM-based infrastructure in the previous patch we do > > get a working 'sudo', so we can install packages by running 'sudo > > apt-get -y install ...' as well. > > OK, so far we learned that this is now _doable_; but not enough to > decide if this is a good thing to do or not. Let's read on to find > out. Yeah, this paragraph is just a bit of background about how the current situation came to be and what recent change made the switch possible. > > Let's make use of this and install necessary packages in > > 'ci/install-dependencies.sh', so all the dependencies (i.e. both > > packages and "non-packages" (P4 and Git-LFS)) are handled in the same > > file. > > So we used to have two waysto prepare the test environment; non > packaged software were done via install-dependencies.sh, but > packaged ones weren't. Unifying them so that the script installs > both would be a good change to simplify the procedure. > > Is that how this sentence argues for this change? Yes. > > Install gcc-8 only in the 'linux-gcc' build job; so far it has > > been unnecessarily installed in the 'linux-clang' build job as well. > > Is this "unneeded gcc-8 was installed" something we can fix only > because we now stopped doing the installation via apt addon? Now that you mention it: no. It would have been possible to install gcc-8 only in the 'linux-gcc' build job even via the apt addon, namely by removing the two Linux build jobs from the implicit build matrix and adding them as two independent build jobs in the 'matrix.include' section of '.travis.yml'. The drawback is that all the extra packages used in both build jobs would have to be duplicated. > Or we > could have fixed it while we were on apt addon but we didn't bother, > and this patch fixes it "while at it"---primarily because the shell > script is far more flexible to work with than travis.yml matrix and > this kind of customization is far easier to do? Basically yes (though I think it's not about not bothering; I don't know about others, but it just occured to me that it would have been doable, however, even if it occured to me earlier, because of the duplicated list of common packages I wouldn't have done it). Doing it in good old shell is indeed easier and the common packages are then only listed once. > > Print the versions of P4 and Git-LFS conditionally, i.e. only when > > they have been installed; with this change even the static analysis > > and documentation build jobs start using 'ci/install-dependencies.sh' > > to install packages, and neither of these two build jobs depend on and > > thus install those. > > > > This change will presumably be beneficial for the upcoming Azure > > Pipelines integration [1]: preliminary versions of that patch series > > run a couple of 'apt-get' commands to install the necessary packages > > before running 'ci/install-dependencies.sh', but with this patch it > > will be sufficient to run only 'ci/install-dependencies.sh'. > > So the main point of this change is to have less knowledge to > prepare the target configuration in the .travis.yml file and keep > them all in ci/install-dependencies.sh, which hopefully is more > reusable than .travis.yml in a non Travis environment? Oh, "more reusable" indeed, that's a more eloquent way to put it. > If that is the case, it makes sense to me. > > > This patch should go on top of 'ss/travis-ci-force-vm-mode'. > > > > I'm not sure about the last paragraph, because: > > > > - It talks about presumed benefits for a currently still > > work-in-progress patch series of an other contributor, and I'm not > > really sure that that's a good thing. Perhaps I should have > > rather put it below the '---'. > > > > - I'm confused about the name of this Azure thing. The cover letter > > mentions "Azure Pipelines", the file is called > > 'azure-pipelines.yml', but the relevant patch I link to talks > > about "Azure DevOps" in the commit message. > > > > Anyway, keep that last paragraph or drop it as you see fit. > > I hope we'll hear from Dscho in one or two revolutions of the Earth > ;-) ... revolutions around what? :)
Re: [RFC PATCH] index-pack: improve performance on NFS
On Thu, Nov 08, 2018 at 08:58:19PM +, Geert Jansen wrote: > On Thu, Nov 08, 2018 at 07:02:57AM -0500, Jeff King wrote: > > > Yes, testing and review. :) > > > > I won't send the series out just yet, as I suspect it could use another > > read-through on my part. But if you want to peek at it or try some > > timings, it's available at: > > > > https://github.com/peff/git jk/loose-cache > > I gave this branch a go. There's a performance regression as I'm getting a > clone speed of about 100 KiB/s while with the previous patch I got around 20 > MiB/s. The culprint appears to be a very large number of stat() calls on > ".git/objects/info/alternates". The call stack is: > > -> quick_has_loose() > -> prepare_alt_odb() > -> read_info_alternates() Heh, indeed. Try this on top: diff --git a/sha1-file.c b/sha1-file.c index bc35b28e17..9ff27f92ed 100644 --- a/sha1-file.c +++ b/sha1-file.c @@ -692,6 +692,7 @@ void prepare_alt_odb(struct repository *r) link_alt_odb_entries(r, r->objects->alternate_db, PATH_SEP, NULL, 0); read_info_alternates(r, r->objects->odb->path, 0); + r->objects->loaded_alternates = 1; } /* Returns 1 if we have successfully freshened the file, 0 otherwise. */ -Peff
[PATCH v4 2/2] Makefile: ease dynamic-gettext-poison transition
From: Junio C Hamano Earlier we made the entire build to fail when GETTEXT_POISON=Yes is given to make, to notify those who did not notice that text poisoning is now a runtime behaviour. It turns out that this is too irritating for those who need to build and test different versions of Git that cross the boundary between history with and without this topic to switch between two environment variables. Demote the error to a warning, so that you can say something like make GETTEXT_POISON=Yes GIT_TEST_GETTEXT_POISON=Yes test during the transition period, without having to worry about whether exact version you are testing has or does not have this topic. Signed-off-by: Junio C Hamano Signed-off-by: Ævar Arnfjörð Bjarmason --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index f3a9995e50..6b492f44a6 100644 --- a/Makefile +++ b/Makefile @@ -1447,7 +1447,7 @@ ifdef NO_SYMLINK_HEAD BASIC_CFLAGS += -DNO_SYMLINK_HEAD endif ifdef GETTEXT_POISON -$(error The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!) +$(warning The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!) endif ifdef NO_GETTEXT BASIC_CFLAGS += -DNO_GETTEXT -- 2.19.1.930.g4563a0d9d0
[PATCH v4 0/2] i18n: make GETTEXT_POISON a runtime option
Addresses all the feedback against v3. Includes a patch by Junio
sitting in "pu" (and I fixed the grammar error Eric pointed out in its
commit message).
Range-diff:
1: cc24ba8de8 ! 1: 2210ee8bd9 i18n: make GETTEXT_POISON a runtime option
@@ -34,11 +34,11 @@
Notes on the implementation:
- * We still compile a dedicated GETTEXT_POISON build in Travis CI.
- This is probably the wrong thing to do and should be followed-up
- with something similar to ae59a4e44f ("travis: run tests with
- GIT_TEST_SPLIT_INDEX", 2018-01-07) to re-use an existing test setup
- for running in the GIT_TEST_GETTEXT_POISON mode.
+ * We still compile a dedicated GETTEXT_POISON build in Travis
+ CI. Perhaps this should be revisited and integrated into the
+ "linux-gcc" build, see ae59a4e44f ("travis: run tests with
+ GIT_TEST_SPLIT_INDEX", 2018-01-07) for prior art in that area. Then
+ again maybe not, see [2].
* We now skip a test in t-basic.sh under
GIT_TEST_GETTEXT_POISON=YesPlease that wasn't skipped before. This
@@ -56,12 +56,22 @@
so we populate the "poison_requested" variable in a codepath that's
won't suffer from that race condition.
-See also [3] for more on the motivation behind this patch, and the
+ * We error out in the Makefile if you're still saying
+ GETTEXT_POISON=YesPlease to prompt users to change their
+ invocation.
+
+ * We should not print out poisoned messages during the test
+ initialization itself to keep it more readable, so the test library
+ hides the variable if set in $GIT_TEST_GETTEXT_POISON_ORIG during
+ setup. See [3].
+
+See also [4] for more on the motivation behind this patch, and the
history of the GETTEXT_POISON facility.
1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/
-2. https://public-inbox.org/git/87woq7b58k@evledraar.gmail.com/
-3. https://public-inbox.org/git/878t2pd6yu@evledraar.gmail.com/
+2. https://public-inbox.org/git/20181102163725.gy30...@szeder.dev/
+3.
https://public-inbox.org/git/20181022202241.18629-2-szeder@gmail.com/
+4. https://public-inbox.org/git/878t2pd6yu@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason
@@ -181,7 +191,7 @@
#else
static inline void git_setup_gettext(void)
{
-+ use_gettext_poison();; /* getenv() reentrancy paranoia */
++ use_gettext_poison(); /* getenv() reentrancy paranoia */
}
static inline int gettext_width(const char *s)
{
@@ -392,8 +402,32 @@
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@
+ TZ=UTC
+ export LANG LC_ALL PAGER TZ
+ EDITOR=:
++
++# GIT_TEST_GETTEXT_POISON should not influence git commands executed
++# during initialization of test-lib and the test repo. Back it up,
++# unset and then restore after initialization is finished.
++if test -n "$GIT_TEST_GETTEXT_POISON"
++then
++ GIT_TEST_GETTEXT_POISON_ORIG=$GIT_TEST_GETTEXT_POISON
++ unset GIT_TEST_GETTEXT_POISON
++fi
++
+ # A call to "unset" with no arguments causes at least Solaris 10
+ # /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets
+ # deriving from the command substitution clustered with the other
+@@
+ test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2
test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
++if test -n "$GIT_TEST_GETTEXT_POISON_ORIG"
++then
++ GIT_TEST_GETTEXT_POISON=$GIT_TEST_GETTEXT_POISON_ORIG
++ unset GIT_TEST_GETTEXT_POISON_ORIG
++fi
++
# Can we rely on git's output in the C locale?
-if test -n "$GETTEXT_POISON"
+if test -z "$GIT_TEST_GETTEXT_POISON"
-: -- > 2: a6948d936a Makefile: ease dynamic-gettext-poison transition
Junio C Hamano (1):
Makefile: ease dynamic-gettext-poison transition
Ævar Arnfjörð Bjarmason (1):
i18n: make GETTEXT_POISON a runtime option
.travis.yml | 2 +-
Makefile | 8 +---
ci/lib-travisci.sh| 4 ++--
gettext.c | 11 +++
gettext.h | 9 +++--
git-sh-i18n.sh| 2 +-
po/README | 13 -
t/README | 6 ++
t/lib-gettext.sh | 2 +-
t/t-basic.sh | 2 +-
t/t0205-gettext-poison.sh | 8 +---
t/t3406-rebase-message.sh | 2 +-
t/t7201-co.sh | 6 +++---
t/t9902-completion.sh | 3 ++-
t/test-lib-functions.sh | 8
t/test-lib.sh | 22 +-
16 files changed, 59 insertions(+), 49 deletions(-)
--
2.19.1.930.g4563a0d9d0
[PATCH v4 1/2] i18n: make GETTEXT_POISON a runtime option
Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON
test parameter to only be a GIT_TEST_GETTEXT_POISON=
runtime parameter, to be consistent with other parameters documented
in "Running tests with special setups" in t/README.
When I added GETTEXT_POISON in bb946bba76 ("i18n: add GETTEXT_POISON
to simulate unfriendly translator", 2011-02-22) I was concerned with
ensuring that the _() function would get constant folded if NO_GETTEXT
was defined, and likewise that GETTEXT_POISON would be compiled out
unless it was defined.
But as the benchmark in my [1] shows doing a one-off runtime
getenv("GIT_TEST_[...]") is trivial, and since GETTEXT_POISON was
originally added the GIT_TEST_* env variables have become the common
idiom for turning on special test setups.
So change GETTEXT_POISON to work the same way. Now the
GETTEXT_POISON=YesPlease compile-time option is gone, and running the
tests with GIT_TEST_GETTEXT_POISON=[YesPlease|] can be toggled on/off
without recompiling.
This allows for conditionally amending tests to test with/without
poison, similar to what 859fdc0c3c ("commit-graph: define
GIT_TEST_COMMIT_GRAPH", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do
some of that, now we e.g. always run the t0205-gettext-poison.sh test.
I did enough there to remove the GETTEXT_POISON prerequisite, but its
inverse C_LOCALE_OUTPUT is still around, and surely some tests using
it can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=.
Notes on the implementation:
* We still compile a dedicated GETTEXT_POISON build in Travis
CI. Perhaps this should be revisited and integrated into the
"linux-gcc" build, see ae59a4e44f ("travis: run tests with
GIT_TEST_SPLIT_INDEX", 2018-01-07) for prior art in that area. Then
again maybe not, see [2].
* We now skip a test in t-basic.sh under
GIT_TEST_GETTEXT_POISON=YesPlease that wasn't skipped before. This
test relies on C locale output, but due to an edge case in how the
previous implementation of GETTEXT_POISON worked (reading it from
GIT-BUILD-OPTIONS) wasn't enabling poison correctly. Now it does,
and needs to be skipped.
* The getenv() function is not reentrant, so out of paranoia about
code of the form:
printf(_("%s"), getenv("some-env"));
call use_gettext_poison() in our early setup in git_setup_gettext()
so we populate the "poison_requested" variable in a codepath that's
won't suffer from that race condition.
* We error out in the Makefile if you're still saying
GETTEXT_POISON=YesPlease to prompt users to change their
invocation.
* We should not print out poisoned messages during the test
initialization itself to keep it more readable, so the test library
hides the variable if set in $GIT_TEST_GETTEXT_POISON_ORIG during
setup. See [3].
See also [4] for more on the motivation behind this patch, and the
history of the GETTEXT_POISON facility.
1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/
2. https://public-inbox.org/git/20181102163725.gy30...@szeder.dev/
3. https://public-inbox.org/git/20181022202241.18629-2-szeder@gmail.com/
4. https://public-inbox.org/git/878t2pd6yu@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason
---
.travis.yml | 2 +-
Makefile | 8 +---
ci/lib-travisci.sh| 4 ++--
gettext.c | 11 +++
gettext.h | 9 +++--
git-sh-i18n.sh| 2 +-
po/README | 13 -
t/README | 6 ++
t/lib-gettext.sh | 2 +-
t/t-basic.sh | 2 +-
t/t0205-gettext-poison.sh | 8 +---
t/t3406-rebase-message.sh | 2 +-
t/t7201-co.sh | 6 +++---
t/t9902-completion.sh | 3 ++-
t/test-lib-functions.sh | 8
t/test-lib.sh | 22 +-
16 files changed, 59 insertions(+), 49 deletions(-)
diff --git a/.travis.yml b/.travis.yml
index 8d2499739e..a329a0add6 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -24,7 +24,7 @@ addons:
matrix:
include:
-- env: jobname=GETTEXT_POISON
+- env: jobname=GIT_TEST_GETTEXT_POISON
os: linux
compiler:
addons:
diff --git a/Makefile b/Makefile
index bbfbb4292d..f3a9995e50 100644
--- a/Makefile
+++ b/Makefile
@@ -362,11 +362,6 @@ all::
# Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the
# user.
#
-# Define GETTEXT_POISON if you are debugging the choice of strings marked
-# for translation. In a GETTEXT_POISON build, you can turn all strings marked
-# for translation into gibberish by setting the GIT_GETTEXT_POISON variable
-# (to any value) in your environment.
-#
# Define JSMIN to point to JavaScript minifier that functions as
# a filter to have gitweb.js minified.
#
@@ -1452,7 +1447,7 @@ ifdef NO_SYMLINK_HEAD
BASIC_CFLAGS += -DNO_SYMLINK_HEAD
endif
ifdef GETTEXT_POISON
- BASIC_CFLAGS += -DGETTEXT_POISON
+$(error The
Re: [RFC PATCH] index-pack: improve performance on NFS
On Thu, Nov 08, 2018 at 07:02:57AM -0500, Jeff King wrote: > Yes, testing and review. :) > > I won't send the series out just yet, as I suspect it could use another > read-through on my part. But if you want to peek at it or try some > timings, it's available at: > > https://github.com/peff/git jk/loose-cache I gave this branch a go. There's a performance regression as I'm getting a clone speed of about 100 KiB/s while with the previous patch I got around 20 MiB/s. The culprint appears to be a very large number of stat() calls on ".git/objects/info/alternates". The call stack is: -> quick_has_loose() -> prepare_alt_odb() -> read_info_alternates()
[PATCH] Makefile: add pending semantic patches
From: SZEDER Gábor Add a description and place on how to use coccinelle for large refactorings that happen only once. Based-on-work-by: SZEDER Gábor Signed-off-by: Stefan Beller --- I consider including this patch in a resend instead. It outlays the basics of such a new workflow, which we can refine later. Thanks, Stefan Makefile | 7 +++-- contrib/coccinelle/README | 60 +++ 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index b08d5ea258..e5abfe4cef 100644 --- a/Makefile +++ b/Makefile @@ -2739,9 +2739,12 @@ endif then \ echo '' SPATCH result: $@; \ fi -coccicheck: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.cocci)) +coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci))) -.PHONY: coccicheck +# See contrib/coccinelle/README +coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci)) + +.PHONY: coccicheck coccicheck-pending ### Installation rules diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README index 9c2f8879c2..fa09d1abcc 100644 --- a/contrib/coccinelle/README +++ b/contrib/coccinelle/README @@ -1,2 +1,62 @@ This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/) semantic patches that might be useful to developers. + +There are two types of semantic patches: + + * Using the semantic transformation to check for bad patterns in the code; + This is what the original target 'make coccicheck' is designed to do and + it is expected that any resulting patch indicates a regression. + The patches resulting from 'make coccicheck' are small and infrequent, + so once they are found, they can be sent to the mailing list as per usual. + + Example for introducing new patterns: + 67947c34ae (convert "hashcmp() != 0" to "!hasheq()", 2018-08-28) + b84c783882 (fsck: s/++i > 1/i++/, 2018-10-24) + + Example of fixes using this approach: + 248f66ed8e (run-command: use strbuf_addstr() for adding a string to + a strbuf, 2018-03-25) + f919ffebed (Use MOVE_ARRAY, 2018-01-22) + + These types of semantic patches are usually part of testing, c.f. + 0860a7641b (travis-ci: fail if Coccinelle static analysis found something + to transform, 2018-07-23) + + * Using semantic transformations in large scale refactorings throughout + the code base. + + When applying the semantic patch into a real patch, sending it to the + mailing list in the usual way, such a patch would be expected to have a + lot of textual and semantic conflicts as such large scale refactorings + change function signatures that are used widely in the code base. + A textual conflict would arise if surrounding code near any call of such + function changes. A semantic conflict arises when other patch series in + flight introduce calls to such functions. + + So to aid these large scale refactorings, semantic patches can be used, + using the process as follows: + + 1) Figure out what kind of large scale refactoring we need + -> This is usually done by another unrelated series. + 2) Create the sematic patch needed for the large scale refactoring + and store it in contrib/coccinelle/*.pending.cocci + -> The suffix containing 'pending' is important to differentiate + this case from the other use case of checking for bad patterns. + 3) Apply the semantic patch only partially, where needed for the patch series + that motivates the large scale refactoring and then build that series + on top of it. + By applying the semantic patch only partially where needed, the series + is less likely to conflict with other series in flight. + To make it possible to apply the semantic patch partially, there needs + to be mechanism for backwards compatibility to keep those places working + where the semantic patch is not applied. This can be done via a + wrapper function that has the exact name and signature as the function + to be changed. + + 4) Send the series as usual, including only the needed parts of the + large scale refactoring + + Later steps (not necessarily by the original author) are to apply the + semantic patch in a way that do not produce lots of conflicts, for example + by consulting `git diff --numstat origin/master..origin/pu` and the changes + of the semantic patch. -- 2.19.1.930.g4563a0d9d0-goog
Re: [PATCH 1/2] ls-remote: do not send ref prefixes for patterns
> Jeff King writes:
>
> > Since b4be74105f (ls-remote: pass ref prefixes when requesting a
> > remote's refs, 2018-03-15), "ls-remote foo" will pass "refs/heads/foo",
> > "refs/tags/foo", etc to the transport code in an attempt to let the
> > other side reduce the size of its advertisement.
>
> Jonathan, seeing 2b554353 ("fetch: send "refs/tags/" prefix upon CLI
> refspecs", 2018-06-05), I am guessing that you are doing the proto v2
> work inherited from Brandon?
Sorry for the late reply - I had some personal events, but I should be
able to look more at Git stuff from now on.
Well, it's true that I have been fixing some bugs related to protocol
v2.
> Having to undo this is unfortunate, but
> I agree with this patch that we need to do this until ref prefix learns
> to grok wildcards.
It is unfortunate, although as far as I can tell, the performance
improvements from "git fetch" (which I think is the more frequent
command called) remain, so it might not be so bad. I see from your
What's Cooking that these patches are to be merged to master, which I
agree with.
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
On Thu, Nov 08, 2018 at 09:26:19PM +0100, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Nov 02 2018, SZEDER Gábor wrote:
> >> * We error out in the Makefile if you're still saying
> >>GETTEXT_POISON=YesPlease.
> >>
> >>This makes more sense than just making it a synonym since now this
> >>also needs to be defined at runtime.
> >
> > OK, I think erroring out is better than silently ignoring
> > GETTEXT_POISON=YesPlease. However, the commit message only mentions
> > that GETTEXT_POISON is gone, but perhaps this should be mentioned
> > there as well.
>
> Will mention.
With the benefit of hindsight gained from looking into a failing
GETTEXT_POISON build [1], I have to agree with Junio that flat-out
erroring out when GETTEXT_POISON=YesPlease is set is really a hassle
[2].
[1] https://public-inbox.org/git/20181107130950.ga30...@szeder.dev/
(the failure is not related to this patch)
[2] https://public-inbox.org/git/xmqqpnvg8d5z@gitster-ct.c.googlers.com/
> >> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> >> index 78d8c3783b..2f42b3653c 100644
> >> --- a/t/test-lib-functions.sh
> >> +++ b/t/test-lib-functions.sh
> >> @@ -755,16 +755,16 @@ test_cmp_bin() {
> >>
> >> # Use this instead of test_cmp to compare files that contain expected and
> >> # actual output from git commands that can be translated. When running
> >> -# under GETTEXT_POISON this pretends that the command produced expected
> >> +# under GIT_TEST_GETTEXT_POISON this pretends that the command produced
> >> expected
> >> # results.
> >> test_i18ncmp () {
> >> - test -n "$GETTEXT_POISON" || test_cmp "$@"
> >> + ! test_have_prereq C_LOCALE_OUTPUT || test_cmp "$@"
> >> }
> >
> >> test_i18ngrep () {
> >>eval "last_arg=\${$#}"
> >> @@ -779,7 +779,7 @@ test_i18ngrep () {
> >>error "bug in the test script: too few parameters to
> >> test_i18ngrep"
> >>fi
> >>
> >> - if test -n "$GETTEXT_POISON"
> >> + if test_have_prereq !C_LOCALE_OUTPUT
> >
> > Why do these two helper functions call test_have_prereq (a function
> > that doesn't even fit on my screen) instead of a simple
> >
> > test -n "$GIT_TEST_GETTEXT_POISON"
>
> I'm going to keep the call to test_have_prereq, it's consistent with
> other use of the helper in the test lib, and doesn't confuse the reader
> by giving the impression that we're in some really early setup where we
> haven't set the prereq at this point (we have).
Using the prereq in the first place is what confused (and is still
confusing...) the reader.
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
On Fri, Nov 02 2018, SZEDER Gábor wrote:
> On Thu, Nov 01, 2018 at 07:31:15PM +, Ævar Arnfjörð Bjarmason wrote:
>> Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON
>> test parameter to only be a GIT_TEST_GETTEXT_POISON=
>> runtime parameter, to be consistent with other parameters documented
>> in "Running tests with special setups" in t/README.
>>
>> When I added GETTEXT_POISON in bb946bba76 ("i18n: add GETTEXT_POISON
>> to simulate unfriendly translator", 2011-02-22) I was concerned with
>> ensuring that the _() function would get constant folded if NO_GETTEXT
>> was defined, and likewise that GETTEXT_POISON would be compiled out
>> unless it was defined.
>>
>> But as the benchmark in my [1] shows doing a one-off runtime
>> getenv("GIT_TEST_[...]") is trivial, and since GETTEXT_POISON was
>> originally added the GIT_TEST_* env variables have become the common
>> idiom for turning on special test setups.
>>
>> So change GETTEXT_POISON to work the same way. Now the
>> GETTEXT_POISON=YesPlease compile-time option is gone, and running the
>> tests with GIT_TEST_GETTEXT_POISON=[YesPlease|] can be toggled on/off
>> without recompiling.
>>
>> This allows for conditionally amending tests to test with/without
>> poison, similar to what 859fdc0c3c ("commit-graph: define
>> GIT_TEST_COMMIT_GRAPH", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do
>> some of that, now we e.g. always run the t0205-gettext-poison.sh test.
>>
>> I did enough there to remove the GETTEXT_POISON prerequisite, but its
>> inverse C_LOCALE_OUTPUT is still around, and surely some tests using
>> it can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=.
>>
>> Notes on the implementation:
>>
>> * We still compile a dedicated GETTEXT_POISON build in Travis CI.
>>This is probably the wrong thing to do and should be followed-up
>>with something similar to ae59a4e44f ("travis: run tests with
>>GIT_TEST_SPLIT_INDEX", 2018-01-07) to re-use an existing test setup
>>for running in the GIT_TEST_GETTEXT_POISON mode.
>
> I'm of two minds about this. Sure, now it's not a compile time
> option, so we can spare a dedicated compilation, and sparing resources
> is good, of course.
>
> However, checking test failures in the 'linux-gcc' build job is always
> a bit of a hassle, because it's not enough to look at the output of
> the failed test, but I also have to check which one of the two test
> runs failed (i.e. the "regular" or one with all the GIT_TEST_* knobs
> turned on). Adding a second test run with GIT_TEST_GETTEXT_POISON
> enabled to an other build job (e.g. 'linux-clang') would then bring
> this hassle to that build job as well.
>
> Furthermore, occasionally a build job is slower than usual (whatever
> the reason might be), which can be an issue when running the test
> suite twice, as it can exceed the time limit.
>
> Anyway, we can think about this later.
>
> In any case, GIT_TEST_GETTEXT_POISON definitely should not be enabled
> in the same "catch-all" test run with all other GIT_TEST_* variables,
> because it would mess up any translated error messages that might pop
> up in a test failure, and then we wouldn't have any idea about what
> went wrong.
Will clarify this in v3. I.e. "let's think about this...".
>> * We now skip a test in t-basic.sh under
>>GIT_TEST_GETTEXT_POISON=YesPlease that wasn't skipped before. This
>>test relies on C locale output, but due to an edge case in how the
>>previous implementation of GETTEXT_POISON worked (reading it from
>>GIT-BUILD-OPTIONS) wasn't enabling poison correctly. Now it does,
>>and needs to be skipped.
>>
>> * The getenv() function is not reentrant, so out of paranoia about
>>code of the form:
>>
>>printf(_("%s"), getenv("some-env"));
>>
>>call use_gettext_poison() in our early setup in git_setup_gettext()
>>so we populate the "poison_requested" variable in a codepath that's
>>won't suffer from that race condition.
>>
>> See also [3] for more on the motivation behind this patch, and the
>> history of the GETTEXT_POISON facility.
>>
>> 1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/
>> 2. https://public-inbox.org/git/87woq7b58k@evledraar.gmail.com/
>> 3. https://public-inbox.org/git/878t2pd6yu@evledraar.gmail.com/
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason
>> ---
>>
>> Now:
>>
>> * The new i18n helper is gone. We just use "test -n" semantics for
>>$GIT_TEST_GETTEXT_POISON
>>
>> * We error out in the Makefile if you're still saying
>>GETTEXT_POISON=YesPlease.
>>
>>This makes more sense than just making it a synonym since now this
>>also needs to be defined at runtime.
>
> OK, I think erroring out is better than silently ignoring
> GETTEXT_POISON=YesPlease. However, the commit message only mentions
> that GETTEXT_POISON is gone, but perhaps this should be mentioned
> there as well.
Will mention.
>> * The caveat with avoiding test_lazy_prereq is gone (although
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
On Wed, Nov 7, 2018 at 10:24 PM Junio C Hamano wrote: > Makefile: ease dynamic-gettext-poison transition > > Earlier we made the entire build to fail when GETTEXT_POISON=Yes is > given to make, to notify those who did not notice that text poisoning > is now a runtime behaviour. > > It turns out that this too irritating for those who need to build s/too/is &/ > and test different versions of Git that cross the boundary between > history with and without this topic to switch between two > environment variables. Demote the error to a warning, so that you > can say something like > > make GETTEXT_POISON=Yes GIT_TEST_GETTEXT_POISON test > > during the transition period, without having to worry about whether > exact version you are testing has or does not have this topic. > > Signed-off-by: Junio C Hamano
Re: how does "clone --filter=sparse:path" work?
On 11/8/2018 12:07 AM, Jeff King wrote: [cc-ing people who worked on or seem interested in partial-clone filters] I've been exploring the partial-clone options a bit, and took a look at the "sparse:path" option. I had some confusion initially, because I expected it work something like this: git clone --filter=sparse:path=Documentation . But it really doesn't take an in-repo path. You have to specify a path to a file that contains a file with .gitignore patterns. Except they're actually _inverted_ patterns (i.e., what to include). Which confused me again, but I guess makes sense if these are meant to be adapted from sparse-checkout files. So my first question is: how is this meant to be used? I guess the idea is that somebody (the repo admin?) makes a set of pre-made profiles, with each profile mentioning some subset of paths. And then when you clone, you say, "I want the foo profile". How is that profile stored and accessed? Yes, the basic idea was if you had a large tree and various groups of users that tended to only need their group's portion of the tree, then you could (or your repo admin could) create several profiles. And users could use a profile to request just the subset of trees and blobs they need. During a clone/fetch users could ask for the profile by OID or by :. It would be a local/custom detail where the repo admin collects the various profiles. (Or at least, that was the thought.) Likewise, a user could create their own profile(s) by committing a sparse-checkout file spec to a personal branch. Again, a local convention. If it's a blob in the repository, I think you can use something like "--filter=sparse:oid=profiles:foo". And then after cloning, you'd do "git cat-file blob profiles:foo >.git/sparse-checkout" or similar. That seems like something that could be wrapped up in a single clone option, but I can't find one; I won't be surprised if it simply hasn't happened yet. Right, TBD. But if it's a path, then what? I'd imagine that you'd "somehow" get a copy of the sparse profile you want out-of-band. And then you'd say "I want to clone with the profile in this file", and then copy it into the sparse-checkout file to do the checkout. But the sparse-checkout file in the filter is not expanded locally, with patterns sent over the wire. The _filename_ is sent over the wire, and then upload-pack expands it. So you can't specify an arbitrary set of patterns; you have to know about the profile names (how?) on the server. That's not very flexible, though I imagine it may make certain things easier on the server (e.g., if you pack in such a way to efficiently serve only particular profiles). But I'm also concerned it's dangerous. We're reading gitignore patterns from an arbitrary name on the server's filesystem, with that name coming from an untrusted client. So I can do: git clone --filter=sparse:path=/etc/passwd $remote and the server will read /etc/passwd. There's probably a lot of mischief you can get up to with that. Sadly reading /proc/self/mem doesn't work, because the gitignore code fstat()s the file to find out how much to read, and the st_size there is 0. But there are probably others (/proc/kcore is a fun one, but nobody is running their git server as root, right?). Below is a proof of concept script that uses this as an oracle to explore the filesystem, as well as individual lines of files. Should we simply be disallowing sparse:path filters over upload-pack? The option to allow an absolute path over the wire probably needs more thought as you suggest. Having it in the traverse code was useful for local testing in the client. But mainly I was thinking of a use case on the client of the form: git rev-list --objects --filter=spec:path=.git/sparse-checkout --missing=print and get a list of the blobs that you don't have and would need before you could checkout using the current sparse-checkout definition. You could then have a pre-checkout hook that would bulk fetch them before starting the actual checkout. Since that would be more efficient than demand-loading blobs individually during the checkout. There's more work to do in this area, but that was the idea. But back to your point, yes, I think we should restrict this over the wire. Thanks, Jeff -Peff -- >8 -- # Set this to host:repo.git to see a real cross-machine connection (which makes # it more obvious which side is reading which files). For a simulated local # one, we'll use --no-local to make sure we really run upload-pack. SERVER=server.git # Do these steps manually on the remote side if you're trying it cross-server. case "$SERVER" in *:*) ;; *) # Imagine a server with a repository users can push to, with filters enabled. git init -q --bare $SERVER git -C $SERVER config uploadpack.allowfilter true # Imagine the server has a file outside of the repo we're interested in. echo foo
Re: [PATCH v6 1/1] http: add support selecting http version
On Thu, Nov 8, 2018 at 2:00 AM Force Charlie via GitGitGadget
wrote:
> In order to give users the freedom to control the HTTP version,
> we need to add a setting to choose which HTTP version to use.
>
> Signed-off-by: Force Charlie
> ---
> diff --git a/http.c b/http.c
> @@ -284,6 +285,9 @@ static void process_curl_messages(void)
> static int http_options(const char *var, const char *value, void *cb)
> {
> + if (!strcmp("http.version",var)) {
Style: space after comma
> + return git_config_string(_http_version, var, value);
> + }
> @@ -806,6 +834,16 @@ static CURL *get_curl_handle(void)
> +if (curl_http_version) {
> + long opt;
> + if (!get_curl_http_version_opt(curl_http_version, )) {
> + /* Set request use http version */
> + curl_easy_setopt(result, CURLOPT_HTTP_VERSION,opt);
Style: space after comma
> + }
> +}
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
On Thu, Nov 8, 2018 at 10:45 AM Johannes Schindelin wrote: > On Thu, 8 Nov 2018, Duy Nguyen wrote: > > One thing I had in mind when proposing $VARIABLE is that it opens up a > > namespace for us to expand more things (*) for example $GIT_DIR (from > > ~/.gitconfig). > > > > (*) but in a controlled way, it may look like an environment variable, > > but we only accept a selected few. And it's only expanded at the > > beginning of a path. > > I understand that desire, and I even agree. But the fact that it looks > like an environment variable, but is not, is actually a point in favor of > *not* doing this. It would violate the principle of least astonishment. Perhaps something like $:VARIABLE, which gives the convenience of $VARIABLE, but looks sufficiently different that it shouldn't trip up readers into thinking it is one. (Well-written code checking for a DOS/Windows drive letter at the beginning of a path shouldn't be confused by it.)
Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options
On Wed, Nov 7, 2018 at 7:22 AM Ævar Arnfjörð Bjarmason wrote:
> In 73a834e9e2 ("range-diff: relieve callers of low-level configuration
> burden", 2018-07-22) we broke passing down options like --no-patch,
> --stat etc. Fix that regression, and add a test for some of these
> options being passed down.
Thanks both (Ævar and Dscho) for cleaning up my mess, and sorry for
not responding sooner; I only just found time to read the discussion
thread. One comment below...
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> diff --git a/range-diff.c b/range-diff.c
> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char
> *range2,
> memcpy(, diffopt, sizeof(opts));
> - opts.output_format = DIFF_FORMAT_PATCH;
> + if (!opts.output_format)
> + opts.output_format = DIFF_FORMAT_PATCH;
Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather
than introducing this new conditional, I'm thinking that a more
correct fix would be:
opts.output_format |= DIFF_FORMAT_PATCH;
(note the '|=' operator). This would result in 'opts.output_format'
containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did
prior to 73a834e9e2 when --no-patch was specified.
[PATCH 1/1] Update .mailmap
From: Johannes Schindelin This patch makes the output of `git shortlog -nse v2.10.0` duplicate-free. Signed-off-by: Johannes Schindelin --- .mailmap | 13 + 1 file changed, 13 insertions(+) diff --git a/.mailmap b/.mailmap index bef3352b0d..1d8310073a 100644 --- a/.mailmap +++ b/.mailmap @@ -21,6 +21,8 @@ Anders Kaseorg Aneesh Kumar K.V Amos Waterland Amos Waterland +Ben Peart +Ben Peart Ben Walton Benoit Sigoure Bernt Hansen @@ -32,6 +34,7 @@ Bryan Larsen Cheng Renquan Chris Shoemaker Chris Wright +Christian Ludwig Cord Seele Christian Couder Christian Stimming @@ -51,6 +54,7 @@ David Reiss David S. Miller David Turner David Turner +Derrick Stolee Deskin Miller Dirk Süsserott Eric Blake @@ -98,6 +102,7 @@ Jens Axboe Jens Lindström Jens Lindstrom Jim Meyering Joachim Berdal Haga +Joachim Jablon Johannes Schindelin Johannes Sixt Johannes Sixt @@ -150,6 +155,7 @@ Mark Levedahl Mark Rada Martin Langhoff Martin von Zweigbergk +Masaya Suzuki Matt Draisey Matt Kraai Matt McCutchen @@ -157,6 +163,7 @@ Matthias Kestenholz Matthias Rüster Matthias Ruester Matthias Urlichs Matthias Urlichs +Matthieu Moy Michael Coleman Michael J Gruber Michael J Gruber @@ -180,7 +187,11 @@ Nick Stokoe Nick Woolley Nick Stokoe Nick Woolley Nicolas Morey-Chaisemartin Nicolas Morey-Chaisemartin +Nicolas Morey-Chaisemartin +Nicolas Morey-Chaisemartin +Nicolas Morey-Chaisemartin Nicolas Sebrecht +Orgad Shaneh Paolo Bonzini Pascal Obry Pascal Obry @@ -200,6 +211,7 @@ Philipp A. Hartmann Philippe Bruhat Ralf Thielow Ramsay Jones +Randall S. Becker René Scharfe René Scharfe Rene Scharfe Richard Hansen @@ -238,6 +250,7 @@ Steven Walter Sven Verdoolaege Sven Verdoolaege SZEDER Gábor +Tao Qingyun <845767...@qq.com> Tay Ray Chuan Ted Percival Theodore Ts'o -- gitgitgadget
[PATCH 0/1] Update .mailmap
I noticed that there were a couple of duplicate entries in git shortlog -nse v2.19.1.., and then continued a bit to remove the duplicate entries even for v2.10.0.. Johannes Schindelin (1): Update .mailmap .mailmap | 13 + 1 file changed, 13 insertions(+) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-71%2Fdscho%2Fmailmap-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-71/dscho/mailmap-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/71 -- gitgitgadget
Re: git-rebase is ignoring working-tree-encoding
On Wed, Nov 07, 2018 at 05:38:18AM +0100, Adrián Gimeno Balaguer wrote: > Hello Torsten, > > Thanks for answering. > > Answering to your question, I removed the comments with "rebase" since > my reported encoding issue happens on more simpler operations > (described in the PR), and the problem is not directly related to > rebasing, so I considered it better in order to avoid unrelated > confusions. > > Let's get back to the problem. Each system has a default endianness. > Also, in .gitattributes's working-tree-encoding, Git behaves > differently depending on the attribute's value and the contents of the > referenced entry file. When I put the value "UTF-16", then the file > must have a BOM, or Git complains. Otherwise, if I put the value > "UTF-16BE" or "UTF-16LE", then Git prohibites operations if file has a > BOM for that main encoding (UTF-16 here), which can be relate to any > endianness. > > My very initial goal was, given a UTF-16LE file, to be able to view > human-readable diffs whenever I make a change on it (and yes, it must > be Little Endian). Plus, this file had a BOM. Now, what are the > options with Git currently (consider only working-tree-encoding)? If I > put working-tree-encoding=UTF-16, then I could view readable diffs and > commit the file, but here is the main problem: Git looses information > about what initial endianness the file had, therefore, after > staging/committing it re-encodes the file from UTF-8 (as stored > internally) to UTF-16 and the default system endianness. In my case it > did to Big Endian, thus affecting the project's requirement. That is > why I ended up writing a fixup script to change the encoding back to > UTF-16LE. OK, I think I understand your problem now. The file format which you ask for could be named "UTF-16-BOM-LE", but that does not exist in reality. If you use UTF-16, then there must be a BOM, and if there is a BOM, then a Unicode-aware application -should- be able to handle it. Why does your project require such a format ? > > On the other hand, once I set working-tree-encoding=UTF-16LE, then Git > prohibited me from committing the file and even viewing human-readable > diffs (the output simply tells it's a binary file). In this sense, the > internal location of these errors is within the function of utf8.c I > made changes to in the PR. I hope I was clearer! > > Finally, Git behaviour around this is based on Unicode standards, > which is why I acknowledged that my changes violated them after > refering to a link which is present in the ut8.h file. []
Re: What's cooking in git.git (Nov 2018, #03; Wed, 7)
On 07/11/2018 09:41, 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 -- * pw/add-p-select (2018-07-26) 4 commits - add -p: optimize line selection for short hunks - add -p: allow line selection to be inverted - add -p: select modified lines correctly - add -p: select individual hunk lines "git add -p" interactive interface learned to let users choose individual added/removed lines to be used in the operation, instead of accepting or rejecting a whole hunk. Will discard. No further feedbacks on the topic for quite some time. Unfortunately I've not found time to work on this recently and don't see myself doing so before the new year so I think it makes sense to drop it. Hopefully I can come up with something next year, it would be nice for users to avoid having to edit patches where possible. Best Wishes Phillip
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi Junio, On Thu, 8 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? > > The `~` prefix is *already* a reserved character,... > > We would need to prepare for a future where we need yet another > special thing to be expanded, and it will quickly become cryptic if > you said "~/ is HOME, ~USER/ is USER's HOME, ~~/ is runtime prefix, > and ~~~/ is this new thing...". ~runtime_prefix~/ (i.e. carve out > the namespace for USERNAME by reserving any names that ends with a > tilde) may be a viable way to do this, though. It is just as good > as your other idea, , in an earlier message. Indeed. Your "cryptic" matches my "unintuitive". Ciao, Dscho
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi Duy, On Thu, 8 Nov 2018, Duy Nguyen wrote: > On Thu, Nov 8, 2018 at 2:14 PM Johannes Schindelin > wrote: > > > > On Wed, 7 Nov 2018, Jeff King wrote: > > > > > All that said, if we're just interested in allowing this for config, > > > then we already have such a wrapper function: git_config_pathname(). > > > > Good point. I agree that `git_config_pathname()` is a better home for this > > feature than `expand_user_path()`. > > > > But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? > > The `~` prefix is *already* a reserved character, and while it would > > probably not be super intuitive to have `~~` be expanded to the runtime > > prefix, at least it would be less ambiguous than `$RUNTIME_PREFIX` (which > > *is* a valid directory name). > > One thing I had in mind when proposing $VARIABLE is that it opens up a > namespace for us to expand more things (*) for example $GIT_DIR (from > ~/.gitconfig). > > (*) but in a controlled way, it may look like an environment variable, > but we only accept a selected few. And it's only expanded at the > beginning of a path. I understand that desire, and I even agree. But the fact that it looks like an environment variable, but is not, is actually a point in favor of *not* doing this. It would violate the principle of least astonishment. The form `/abc/def` would not be confused with anything that it is not, I would think. The only thing against this form (at least that I can think of) is that some people use this way to talk about paths that vary between different setups, with the implicit assumption that the reader will "interpolate" this *before* applying. So for example, when I tell a user to please edit their /config, I implicitly assume that they know to not type out, literally, but instead fill in the correct path. Ciao, Dscho > > Of course, `~~` is also a valid directory name, but then, so is `~` and we > > do not have any way to specify *that* because `expand_user_path()` will > > always expand it to the home directory. Or am I wrong? Do we have a way to > > disable the expansion? > > > > Ciao, > > Dscho > > > > -- > Duy >
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi Junio, On Thu, 8 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > On Thu, 8 Nov 2018, Junio C Hamano wrote: > > > >> I am tempted to say "///" might also be such a > >> way, even in the POSIX world, but am not brave enough to do so, as I > >> suspect that may have a fallout in the Windows world X-<. > > > > It does. //server/share is the way we refer to UNC paths (AKA network > > drives). > > Shucks. That would mean the patch that started this thread would > not be a good idea, as an end-user could already be writing > "//server/share/some/path" and the code with the patch would see '/' > that begins it, and start treating it differently than the code > before the patch X-<. Ouch. You're right! > > Granted, this is a highly unlikely scenario, but I would feel a bit more > > comfortable with something like > > > > /ssl/certs/ca-bundle.crt > > > > Of course, `` is *also* a perfectly valid directory name, > > but I would argue that it is even less likely to exist than > > `$RUNTIME_PREFIX` because the user would have to escape *two* characters > > rather than one. > > Yes, and it is naturally extensible by allowing > inside the special bra-ket pair (just like $OTHER_THINGS can be a > way to extend the system if we used a special variable syntax). True. > >> Are there security implications if we started allowing references to > >> environment varibables in strings we pass expand_user_path()? > > > > Probably. But then, the runtime prefix is not even available as > > environment variable... > > Ah, sorry. I thought it was clear that I would next be suggesting to > add an environmet variable for it, _if_ the approach to allow env > references turns out to be viable. Of course, I should have assumed that. Sorry! Ciao, Dscho
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Johannes Schindelin writes: > But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? > The `~` prefix is *already* a reserved character,... We would need to prepare for a future where we need yet another special thing to be expanded, and it will quickly become cryptic if you said "~/ is HOME, ~USER/ is USER's HOME, ~~/ is runtime prefix, and ~~~/ is this new thing...". ~runtime_prefix~/ (i.e. carve out the namespace for USERNAME by reserving any names that ends with a tilde) may be a viable way to do this, though. It is just as good as your other idea, , in an earlier message.
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Johannes Schindelin writes: > Hi, > > On Thu, 8 Nov 2018, Junio C Hamano wrote: > >> I am tempted to say "///" might also be such a >> way, even in the POSIX world, but am not brave enough to do so, as I >> suspect that may have a fallout in the Windows world X-<. > > It does. //server/share is the way we refer to UNC paths (AKA network > drives). Shucks. That would mean the patch that started this thread would not be a good idea, as an end-user could already be writing "//server/share/some/path" and the code with the patch would see '/' that begins it, and start treating it differently than the code before the patch X-<. > Granted, this is a highly unlikely scenario, but I would feel a bit more > comfortable with something like > > /ssl/certs/ca-bundle.crt > > Of course, `` is *also* a perfectly valid directory name, > but I would argue that it is even less likely to exist than > `$RUNTIME_PREFIX` because the user would have to escape *two* characters > rather than one. Yes, and it is naturally extensible by allowing inside the special bra-ket pair (just like $OTHER_THINGS can be a way to extend the system if we used a special variable syntax). >> Are there security implications if we started allowing references to >> environment varibables in strings we pass expand_user_path()? > > Probably. But then, the runtime prefix is not even available as > environment variable... Ah, sorry. I thought it was clear that I would next be suggesting to add an environmet variable for it, _if_ the approach to allow env references turns out to be viable.
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
On Thu, Nov 8, 2018 at 2:14 PM Johannes Schindelin wrote: > > Hi Peff, > > On Wed, 7 Nov 2018, Jeff King wrote: > > > All that said, if we're just interested in allowing this for config, > > then we already have such a wrapper function: git_config_pathname(). > > Good point. I agree that `git_config_pathname()` is a better home for this > feature than `expand_user_path()`. > > But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? > The `~` prefix is *already* a reserved character, and while it would > probably not be super intuitive to have `~~` be expanded to the runtime > prefix, at least it would be less ambiguous than `$RUNTIME_PREFIX` (which > *is* a valid directory name). One thing I had in mind when proposing $VARIABLE is that it opens up a namespace for us to expand more things (*) for example $GIT_DIR (from ~/.gitconfig). (*) but in a controlled way, it may look like an environment variable, but we only accept a selected few. And it's only expanded at the beginning of a path. > Of course, `~~` is also a valid directory name, but then, so is `~` and we > do not have any way to specify *that* because `expand_user_path()` will > always expand it to the home directory. Or am I wrong? Do we have a way to > disable the expansion? > > Ciao, > Dscho -- Duy
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi Peff, On Wed, 7 Nov 2018, Jeff King wrote: > All that said, if we're just interested in allowing this for config, > then we already have such a wrapper function: git_config_pathname(). Good point. I agree that `git_config_pathname()` is a better home for this feature than `expand_user_path()`. But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? The `~` prefix is *already* a reserved character, and while it would probably not be super intuitive to have `~~` be expanded to the runtime prefix, at least it would be less ambiguous than `$RUNTIME_PREFIX` (which *is* a valid directory name). Of course, `~~` is also a valid directory name, but then, so is `~` and we do not have any way to specify *that* because `expand_user_path()` will always expand it to the home directory. Or am I wrong? Do we have a way to disable the expansion? Ciao, Dscho
From:Miss:Fatima Yusuf.
From:Miss:Fatima Yusuf. For sure this mail would definitely come to you as a surprise, but do take your good time to go through it, My name is Ms.Fatima Yusuf,i am from Ivory Coast. I lost my parents a year and couple of months ago. My father was a serving director of the Agro-exporting board until his death. He was assassinated by his business partners.Before his death, he made a deposit of US$9.7 Million Dollars here in Cote d'ivoire which was for the purchase of cocoa processing machine and development of another factory before his untimely death. Being that this part of the world experiences political and crises time without number, there is no guarantee of lives and properties. I cannot invest this money here any long, despite the fact it had been my late father's industrial plans. I want you to do me a favor to receive this funds into your country or any safer place as the beneficiary, I have plans to invest this money in continuation with the investment vision of my late father, but not in this place again rather in your country. I have the vision of going into real estate and industrial production or any profitable business venture. I will be ready to compensate you with 20% of the total Amount, now all my hope is banked on you and i really wants to invest this money in your country, where there is stability of Government, political and economic welfare. My greatest worry now is how to move out of this country because my uncle is threatening to kill me as he killed my father,Please do not let anybody hear about this, it is between me and you alone because of my security reason. I am waiting to hear from you. Yours Sincerely, Miss.Fatima Yusuf.
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi, On Thu, 8 Nov 2018, Junio C Hamano wrote: > I am tempted to say "///" might also be such a > way, even in the POSIX world, but am not brave enough to do so, as I > suspect that may have a fallout in the Windows world X-<. It does. //server/share is the way we refer to UNC paths (AKA network drives). > An earlier suggestion by Duy in [*1*] to pretend as if we take > $VARIABLE and define special variables might be a better direction. My only qualm with this is that `$VARIABLE` is a perfectly allowed part of a path. That is, you *could* create a directory called `$VARIABLE` and reference that, and then the expand_user_path() function (or whatever name we will give it) would always expand this, with no way to switch it off. Granted, this is a highly unlikely scenario, but I would feel a bit more comfortable with something like /ssl/certs/ca-bundle.crt Of course, `` is *also* a perfectly valid directory name, but I would argue that it is even less likely to exist than `$RUNTIME_PREFIX` because the user would have to escape *two* characters rather than one. > Are there security implications if we started allowing references to > environment varibables in strings we pass expand_user_path()? Probably. But then, the runtime prefix is not even available as environment variable... Ciao, Dscho > If we cannot convince ourselves that it is safe to allow access to any > and all environment variables, then we probably can start by specific > "pseudo variables" under our control, like GIT_EXEC_PATH and > GIT_INSTALL_ROOT, that do not even have to be tied to environment > variables, perhaps with further restriction to allow it only at the > beginning of the string, or something like that, if necessary. > > [References] > > *1* >
Re: [RFC PATCH] index-pack: improve performance on NFS
On Wed, Nov 07, 2018 at 10:55:24PM +, Geert Jansen wrote: > On Mon, Oct 29, 2018 at 07:27:39PM -0400, Jeff King wrote: > > > On Mon, Oct 29, 2018 at 08:36:07PM +0100, Ævar Arnfjörð Bjarmason wrote: > > > * Re-roll my 4 patch series to include the patch you have in > > ><20181027093300.ga23...@sigill.intra.peff.net> > > > > I don't think it's quite ready for inclusion as-is. I hope to brush it > > up a bit, but I have quite a backlog of stuff to review, as well. > > We're still quite keen to get this patch included. Is there anything I can do > to help? Yes, testing and review. :) I won't send the series out just yet, as I suspect it could use another read-through on my part. But if you want to peek at it or try some timings, it's available at: https://github.com/peff/git jk/loose-cache It's quite a bit bigger than the original patch, as some refactoring was necessary to reuse the existing cache in alternate_object_directories. I'm rather pleased with how it turned out; it unifies the handling of alternates and the main object directory, which is a cleanup I've been wanting to do for some time. > Also I just re-read your comments on maximum cache size. I think you were > arguing both sides of the equation and I wasn't sure where you'd ended up. :) > A larger cache size potentially takes more time to fill up especially on NFS > while a smaller cache size obviously would less effective. That said a small > cache is still effective for the "clone" case where the repo is empty. I ended up thinking that a large cache is going to be fine. So I didn't even bother implementing a limit in my series, which makes things a bit simpler (it's one less state to deal with). Since it reuses the existing cache code, it's better in a few ways than my earlier patch: 1. If a program uses OBJECT_INFO_QUICK and prints abbreviated sha1s, we only have to load the cache once (I think fetch does this, but I didn't test it). 2. The cache is filled one directory at a time, which avoids unnecessary work when there are only a few lookups. 3. The cache is per-object-directory. So if a request can be filled without looking at an alternate, we avoid looking at the alternate. I doubt this matters much in practice (the case we care about is when we _don't_ have the object, and there you have to look everywhere). The one thing I didn't implement is a config option to disable this. That would be pretty easy to add. I don't think it's necessary, but it would make testing before/after behavior easier if somebody thinks it's slowing down their particular case. > It also occurred to me that as a performance optimization your patch could > read > the the loose object directories in parallel using a thread pool. At least on > Amazon EFS this should result in al almost linear performance increase. I'm > not > sure how much this would help for local file systems. In any case this may be > best done as a follow-up patch (that I'd be happy to volunteer for). Yeah, I suspect it could make things faster in some cases. But it also implies filling all of the cache directories at once up front. The code I have now tries to avoid unnecessary cache fills. But it would be pretty easy to kick off a full fill. I agree it would make more sense as a follow-up patch (and probably controlled by a config option, since it likely only makes sense when you have a really high-latency readdir). -Peff
GREETHING FROM MISS ZAFRAT,
-- GREETHING FROM MISS ZAFRAT, I hope you are fine and all is well with you, I got your contact as I was looking for a good relationship in Google search. I am sorry to bother you with my proposal for a relationship with you, but I know that you will grant my request in good faith and understanding. My name is Miss Zafrat, I am an honest, sincere and God fearing lady. I believe that color, religion, language, age, country, tribe, distance etc has nothing to do with real friendship, I believe that a real friendship is all about love and understanding, care, trust etc for each other. I believe we can move from here. I would like to know more about you; as soon as I receive your mail I will tell you more about myself and send you my pictures. I will be waiting for your mail as soon as possible. Till I hear from you remain blessed and have a happy day, Yours Sincerely Zafrat,
Attention: E-mail Address Owner,
-- Website: www.westernunion.com Address: Plot 1261, Adela Hopewell Street CO/B/REP, Republic Of Benin. Email: westernunibe...@seznam.cz Attention: E-mail Address Owner, Sequel to the meeting held with Federal Bureau of Investigation, The International Monetary Fund (IMF) is compensating all the scam victims and some email users which your name and email address was found on the list. However, we have concluded to effect your own payment through Western Union® Money Transfer, $5,000 daily until the total sum of your compensation fund is transferred to you. This is your first payment information: MTCN#: 6486232830 Amount Programmed: $5.000 Track your first payment on-line now https://www.westernunion.com/gb/en/self-service/app/tracktransfer You are advised to get back to the contact person trough the email below for more direction on how to be receiving your payment Contact person: . . SIR. INNOCENT JOHNSON Email address: . . (westernunibe...@seznam.cz) Thanks, SIR.INNOCENT JOHNSON Director Western Union Money Transfer, Head Office Benin Republic.
